All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Revamp k3-socinfo driver
@ 2023-09-15  6:46 ` Neha Malcom Francis
  0 siblings, 0 replies; 12+ messages in thread
From: Neha Malcom Francis @ 2023-09-15  6:46 UTC (permalink / raw)
  To: nm, ssantosh, t-konduru
  Cc: linux-kernel, linux-arm-kernel, n-francis, u-kumar1

k3-socinfo driver doesn't account for difference series of silicon
revisions instead of the typical 1.0, 2.0 etc case. This exception is
currently already seen in J721E. This series aims to modify the driver
to account for those exceptions as well as clean things up a bit.

Changes since v1:
	- Nishanth:
		- undo churning of family attribute
		- remove unnecessary code relocation
		- add Thejasvi to Signed-off-by as we are now similar to
		  the initial attempt [1]
		- separate out typo fixes to another patch (2/2)

v1: https://lore.kernel.org/linux-arm-kernel/20230914074426.1901226-1-n-francis@ti.com/T/

[1] https://lore.kernel.org/all/20230607080349.26671-1-t-konduru@ti.com/

Neha Malcom Francis (2):
  soc: ti: k3-socinfo: Revamp driver to accommodate different rev
    structs
  soc: ti k3-socinfo: Fix typo

 drivers/soc/ti/k3-socinfo.c | 73 +++++++++++++++++++++++++++++--------
 1 file changed, 57 insertions(+), 16 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH v2 0/2] Revamp k3-socinfo driver
@ 2023-09-15  6:46 ` Neha Malcom Francis
  0 siblings, 0 replies; 12+ messages in thread
From: Neha Malcom Francis @ 2023-09-15  6:46 UTC (permalink / raw)
  To: nm, ssantosh, t-konduru
  Cc: linux-kernel, linux-arm-kernel, n-francis, u-kumar1

k3-socinfo driver doesn't account for difference series of silicon
revisions instead of the typical 1.0, 2.0 etc case. This exception is
currently already seen in J721E. This series aims to modify the driver
to account for those exceptions as well as clean things up a bit.

Changes since v1:
	- Nishanth:
		- undo churning of family attribute
		- remove unnecessary code relocation
		- add Thejasvi to Signed-off-by as we are now similar to
		  the initial attempt [1]
		- separate out typo fixes to another patch (2/2)

v1: https://lore.kernel.org/linux-arm-kernel/20230914074426.1901226-1-n-francis@ti.com/T/

[1] https://lore.kernel.org/all/20230607080349.26671-1-t-konduru@ti.com/

Neha Malcom Francis (2):
  soc: ti: k3-socinfo: Revamp driver to accommodate different rev
    structs
  soc: ti k3-socinfo: Fix typo

 drivers/soc/ti/k3-socinfo.c | 73 +++++++++++++++++++++++++++++--------
 1 file changed, 57 insertions(+), 16 deletions(-)

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH v2 1/2] soc: ti: k3-socinfo: Revamp driver to accommodate different rev structs
  2023-09-15  6:46 ` Neha Malcom Francis
@ 2023-09-15  6:46   ` Neha Malcom Francis
  -1 siblings, 0 replies; 12+ messages in thread
From: Neha Malcom Francis @ 2023-09-15  6:46 UTC (permalink / raw)
  To: nm, ssantosh, t-konduru
  Cc: linux-kernel, linux-arm-kernel, n-francis, u-kumar1

k3-socinfo.c driver assumes silicon revisions to be 1.0, 2.0 etc. for
every platform. However this typical style is not followed by J721E
(1.0, 1.1) and need not be followed by upcoming silicon revisions as
well. Adapt the driver to be similar to its U-Boot counterpart
(drivers/soc/soc_ti_k3.c) that accounts for this difference on the
basis of partno/family.

Note that we change the order of invocation of
k3_chipinfo_partno_to_names before k3_chipinfo_variant_to_sr so we
have the family name in case error is returned.

Signed-off-by: Thejasvi Konduru <t-konduru@ti.com>
Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
 drivers/soc/ti/k3-socinfo.c | 71 +++++++++++++++++++++++++++++--------
 1 file changed, 56 insertions(+), 15 deletions(-)

diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
index 6ea9b8c7d335..6de1e3531af9 100644
--- a/drivers/soc/ti/k3-socinfo.c
+++ b/drivers/soc/ti/k3-socinfo.c
@@ -33,19 +33,37 @@
 
 #define CTRLMMR_WKUP_JTAGID_MFG_TI		0x17
 
+#define JTAG_ID_PARTNO_AM65X		0xBB5A
+#define JTAG_ID_PARTNO_J721E		0xBB64
+#define JTAG_ID_PARTNO_J7200		0xBB6D
+#define JTAG_ID_PARTNO_AM64X		0xBB38
+#define JTAG_ID_PARTNO_J721S2		0xBB75
+#define JTAG_ID_PARTNO_AM62X		0xBB7E
+#define JTAG_ID_PARTNO_J784S4		0xBB80
+#define JTAG_ID_PARTNO_AM62AX		0xBB8D
+#define JTAG_ID_PARTNO_AM62PX		0xBB9D
+
 static const struct k3_soc_id {
 	unsigned int id;
 	const char *family_name;
 } k3_soc_ids[] = {
-	{ 0xBB5A, "AM65X" },
-	{ 0xBB64, "J721E" },
-	{ 0xBB6D, "J7200" },
-	{ 0xBB38, "AM64X" },
-	{ 0xBB75, "J721S2"},
-	{ 0xBB7E, "AM62X" },
-	{ 0xBB80, "J784S4" },
-	{ 0xBB8D, "AM62AX" },
-	{ 0xBB9D, "AM62PX" },
+	{ JTAG_ID_PARTNO_AM65X, "AM65X" },
+	{ JTAG_ID_PARTNO_J721E, "J721E" },
+	{ JTAG_ID_PARTNO_J7200, "J7200" },
+	{ JTAG_ID_PARTNO_AM64X, "AM64X" },
+	{ JTAG_ID_PARTNO_J721S2, "J721S2"},
+	{ JTAG_ID_PARTNO_AM62X, "AM62X" },
+	{ JTAG_ID_PARTNO_J784S4, "J784S4" },
+	{ JTAG_ID_PARTNO_AM62AX, "AM62AX" },
+	{ JTAG_ID_PARTNO_AM62PX, "AM62PX" },
+};
+
+static char *j721e_rev_string_map[] = {
+	"1.0", "1.1",
+};
+
+static char *k3_rev_string_map[] = {
+	"1.0", "2.0", "3.0",
 };
 
 static int
@@ -63,6 +81,29 @@ k3_chipinfo_partno_to_names(unsigned int partno,
 	return -EINVAL;
 }
 
+static int
+k3_chipinfo_variant_to_sr(unsigned int partno, unsigned int variant,
+			  struct soc_device_attribute *soc_dev_attr)
+{
+	switch (partno) {
+	case JTAG_ID_PARTNO_J721E:
+		if (variant >= ARRAY_SIZE(j721e_rev_string_map))
+			goto bail;
+		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
+						   j721e_rev_string_map[variant]);
+		break;
+	default:
+		if (variant >= ARRAY_SIZE(k3_rev_string_map))
+			goto bail;
+		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
+						   k3_rev_string_map[variant]);
+	}
+	return 0;
+
+bail:
+	return -EINVAL;
+}
+
 static int k3_chipinfo_probe(struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
@@ -94,7 +135,6 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
 
 	variant = (jtag_id & CTRLMMR_WKUP_JTAGID_VARIANT_MASK) >>
 		  CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT;
-	variant++;
 
 	partno_id = (jtag_id & CTRLMMR_WKUP_JTAGID_PARTNO_MASK) >>
 		 CTRLMMR_WKUP_JTAGID_PARTNO_SHIFT;
@@ -103,15 +143,16 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
 	if (!soc_dev_attr)
 		return -ENOMEM;
 
-	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
-	if (!soc_dev_attr->revision) {
-		ret = -ENOMEM;
+	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
+	if (ret) {
+		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);
+		ret = -ENODEV;
 		goto err;
 	}
 
-	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
+	ret = k3_chipinfo_variant_to_sr(partno_id, variant, soc_dev_attr);
 	if (ret) {
-		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);
+		dev_err(dev, "Unknown revision for %s\n", soc_dev_attr->family);
 		ret = -ENODEV;
 		goto err_free_rev;
 	}
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH v2 1/2] soc: ti: k3-socinfo: Revamp driver to accommodate different rev structs
@ 2023-09-15  6:46   ` Neha Malcom Francis
  0 siblings, 0 replies; 12+ messages in thread
From: Neha Malcom Francis @ 2023-09-15  6:46 UTC (permalink / raw)
  To: nm, ssantosh, t-konduru
  Cc: linux-kernel, linux-arm-kernel, n-francis, u-kumar1

k3-socinfo.c driver assumes silicon revisions to be 1.0, 2.0 etc. for
every platform. However this typical style is not followed by J721E
(1.0, 1.1) and need not be followed by upcoming silicon revisions as
well. Adapt the driver to be similar to its U-Boot counterpart
(drivers/soc/soc_ti_k3.c) that accounts for this difference on the
basis of partno/family.

Note that we change the order of invocation of
k3_chipinfo_partno_to_names before k3_chipinfo_variant_to_sr so we
have the family name in case error is returned.

Signed-off-by: Thejasvi Konduru <t-konduru@ti.com>
Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
 drivers/soc/ti/k3-socinfo.c | 71 +++++++++++++++++++++++++++++--------
 1 file changed, 56 insertions(+), 15 deletions(-)

diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
index 6ea9b8c7d335..6de1e3531af9 100644
--- a/drivers/soc/ti/k3-socinfo.c
+++ b/drivers/soc/ti/k3-socinfo.c
@@ -33,19 +33,37 @@
 
 #define CTRLMMR_WKUP_JTAGID_MFG_TI		0x17
 
+#define JTAG_ID_PARTNO_AM65X		0xBB5A
+#define JTAG_ID_PARTNO_J721E		0xBB64
+#define JTAG_ID_PARTNO_J7200		0xBB6D
+#define JTAG_ID_PARTNO_AM64X		0xBB38
+#define JTAG_ID_PARTNO_J721S2		0xBB75
+#define JTAG_ID_PARTNO_AM62X		0xBB7E
+#define JTAG_ID_PARTNO_J784S4		0xBB80
+#define JTAG_ID_PARTNO_AM62AX		0xBB8D
+#define JTAG_ID_PARTNO_AM62PX		0xBB9D
+
 static const struct k3_soc_id {
 	unsigned int id;
 	const char *family_name;
 } k3_soc_ids[] = {
-	{ 0xBB5A, "AM65X" },
-	{ 0xBB64, "J721E" },
-	{ 0xBB6D, "J7200" },
-	{ 0xBB38, "AM64X" },
-	{ 0xBB75, "J721S2"},
-	{ 0xBB7E, "AM62X" },
-	{ 0xBB80, "J784S4" },
-	{ 0xBB8D, "AM62AX" },
-	{ 0xBB9D, "AM62PX" },
+	{ JTAG_ID_PARTNO_AM65X, "AM65X" },
+	{ JTAG_ID_PARTNO_J721E, "J721E" },
+	{ JTAG_ID_PARTNO_J7200, "J7200" },
+	{ JTAG_ID_PARTNO_AM64X, "AM64X" },
+	{ JTAG_ID_PARTNO_J721S2, "J721S2"},
+	{ JTAG_ID_PARTNO_AM62X, "AM62X" },
+	{ JTAG_ID_PARTNO_J784S4, "J784S4" },
+	{ JTAG_ID_PARTNO_AM62AX, "AM62AX" },
+	{ JTAG_ID_PARTNO_AM62PX, "AM62PX" },
+};
+
+static char *j721e_rev_string_map[] = {
+	"1.0", "1.1",
+};
+
+static char *k3_rev_string_map[] = {
+	"1.0", "2.0", "3.0",
 };
 
 static int
@@ -63,6 +81,29 @@ k3_chipinfo_partno_to_names(unsigned int partno,
 	return -EINVAL;
 }
 
+static int
+k3_chipinfo_variant_to_sr(unsigned int partno, unsigned int variant,
+			  struct soc_device_attribute *soc_dev_attr)
+{
+	switch (partno) {
+	case JTAG_ID_PARTNO_J721E:
+		if (variant >= ARRAY_SIZE(j721e_rev_string_map))
+			goto bail;
+		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
+						   j721e_rev_string_map[variant]);
+		break;
+	default:
+		if (variant >= ARRAY_SIZE(k3_rev_string_map))
+			goto bail;
+		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
+						   k3_rev_string_map[variant]);
+	}
+	return 0;
+
+bail:
+	return -EINVAL;
+}
+
 static int k3_chipinfo_probe(struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
@@ -94,7 +135,6 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
 
 	variant = (jtag_id & CTRLMMR_WKUP_JTAGID_VARIANT_MASK) >>
 		  CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT;
-	variant++;
 
 	partno_id = (jtag_id & CTRLMMR_WKUP_JTAGID_PARTNO_MASK) >>
 		 CTRLMMR_WKUP_JTAGID_PARTNO_SHIFT;
@@ -103,15 +143,16 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
 	if (!soc_dev_attr)
 		return -ENOMEM;
 
-	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
-	if (!soc_dev_attr->revision) {
-		ret = -ENOMEM;
+	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
+	if (ret) {
+		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);
+		ret = -ENODEV;
 		goto err;
 	}
 
-	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
+	ret = k3_chipinfo_variant_to_sr(partno_id, variant, soc_dev_attr);
 	if (ret) {
-		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);
+		dev_err(dev, "Unknown revision for %s\n", soc_dev_attr->family);
 		ret = -ENODEV;
 		goto err_free_rev;
 	}
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH v2 2/2] soc: ti k3-socinfo: Fix typo
  2023-09-15  6:46 ` Neha Malcom Francis
@ 2023-09-15  6:46   ` Neha Malcom Francis
  -1 siblings, 0 replies; 12+ messages in thread
From: Neha Malcom Francis @ 2023-09-15  6:46 UTC (permalink / raw)
  To: nm, ssantosh, t-konduru
  Cc: linux-kernel, linux-arm-kernel, n-francis, u-kumar1

Fix typo in driver that comments out wrong bit.

Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
 drivers/soc/ti/k3-socinfo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
index 6de1e3531af9..417f3f33cd01 100644
--- a/drivers/soc/ti/k3-socinfo.c
+++ b/drivers/soc/ti/k3-socinfo.c
@@ -20,7 +20,7 @@
  *  31-28 VARIANT	Device variant
  *  27-12 PARTNO	Part number
  *  11-1  MFG		Indicates TI as manufacturer (0x17)
- *  1			Always 1
+ *  0			Always 1
  */
 #define CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT	(28)
 #define CTRLMMR_WKUP_JTAGID_VARIANT_MASK	GENMASK(31, 28)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH v2 2/2] soc: ti k3-socinfo: Fix typo
@ 2023-09-15  6:46   ` Neha Malcom Francis
  0 siblings, 0 replies; 12+ messages in thread
From: Neha Malcom Francis @ 2023-09-15  6:46 UTC (permalink / raw)
  To: nm, ssantosh, t-konduru
  Cc: linux-kernel, linux-arm-kernel, n-francis, u-kumar1

Fix typo in driver that comments out wrong bit.

Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
 drivers/soc/ti/k3-socinfo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
index 6de1e3531af9..417f3f33cd01 100644
--- a/drivers/soc/ti/k3-socinfo.c
+++ b/drivers/soc/ti/k3-socinfo.c
@@ -20,7 +20,7 @@
  *  31-28 VARIANT	Device variant
  *  27-12 PARTNO	Part number
  *  11-1  MFG		Indicates TI as manufacturer (0x17)
- *  1			Always 1
+ *  0			Always 1
  */
 #define CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT	(28)
 #define CTRLMMR_WKUP_JTAGID_VARIANT_MASK	GENMASK(31, 28)
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] soc: ti: k3-socinfo: Revamp driver to accommodate different rev structs
  2023-09-15  6:46   ` Neha Malcom Francis
@ 2023-09-15 12:21     ` Nishanth Menon
  -1 siblings, 0 replies; 12+ messages in thread
From: Nishanth Menon @ 2023-09-15 12:21 UTC (permalink / raw)
  To: Neha Malcom Francis
  Cc: ssantosh, t-konduru, linux-kernel, linux-arm-kernel, u-kumar1

On 12:16-20230915, Neha Malcom Francis wrote:
> k3-socinfo.c driver assumes silicon revisions to be 1.0, 2.0 etc. for
> every platform. However this typical style is not followed by J721E
> (1.0, 1.1) and need not be followed by upcoming silicon revisions as
> well. Adapt the driver to be similar to its U-Boot counterpart
> (drivers/soc/soc_ti_k3.c) that accounts for this difference on the
> basis of partno/family.
> 
> Note that we change the order of invocation of
> k3_chipinfo_partno_to_names before k3_chipinfo_variant_to_sr so we
> have the family name in case error is returned.

Drop reference to U-boot and others. How about this:

The driver assumes that the silicon revisions for every platform are
incremental and one-to-one, corresponding to JTAG_ID's variant field:
1.0, 2.0, and so on. This assumption is wrong for SoCs such as J721E,
where the variant field to revision mapping is 1,0, 1.1. Further,
there are SoCs such as AM65x where the sub-variant version requires
custom decoding of other registers.

Address this by using conditional handling per JTAG ID that requires
an exception with J721E as the first example. To facilitate this
conversion, use macros to identify the JTAG_ID part number and map them
to predefined string array.

> 
> Signed-off-by: Thejasvi Konduru <t-konduru@ti.com>

Maintain original From or drop this or use Co-developed-by as applicable?

> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
>  drivers/soc/ti/k3-socinfo.c | 71 +++++++++++++++++++++++++++++--------
>  1 file changed, 56 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
> index 6ea9b8c7d335..6de1e3531af9 100644
> --- a/drivers/soc/ti/k3-socinfo.c
> +++ b/drivers/soc/ti/k3-socinfo.c
> @@ -33,19 +33,37 @@
>  
>  #define CTRLMMR_WKUP_JTAGID_MFG_TI		0x17
>  
> +#define JTAG_ID_PARTNO_AM65X		0xBB5A
> +#define JTAG_ID_PARTNO_J721E		0xBB64
> +#define JTAG_ID_PARTNO_J7200		0xBB6D
> +#define JTAG_ID_PARTNO_AM64X		0xBB38
> +#define JTAG_ID_PARTNO_J721S2		0xBB75
> +#define JTAG_ID_PARTNO_AM62X		0xBB7E
> +#define JTAG_ID_PARTNO_J784S4		0xBB80
> +#define JTAG_ID_PARTNO_AM62AX		0xBB8D
> +#define JTAG_ID_PARTNO_AM62PX		0xBB9D
> +
>  static const struct k3_soc_id {
>  	unsigned int id;
>  	const char *family_name;
>  } k3_soc_ids[] = {
> -	{ 0xBB5A, "AM65X" },
> -	{ 0xBB64, "J721E" },
> -	{ 0xBB6D, "J7200" },
> -	{ 0xBB38, "AM64X" },
> -	{ 0xBB75, "J721S2"},
> -	{ 0xBB7E, "AM62X" },
> -	{ 0xBB80, "J784S4" },
> -	{ 0xBB8D, "AM62AX" },
> -	{ 0xBB9D, "AM62PX" },
> +	{ JTAG_ID_PARTNO_AM65X, "AM65X" },
> +	{ JTAG_ID_PARTNO_J721E, "J721E" },
> +	{ JTAG_ID_PARTNO_J7200, "J7200" },
> +	{ JTAG_ID_PARTNO_AM64X, "AM64X" },
> +	{ JTAG_ID_PARTNO_J721S2, "J721S2"},
> +	{ JTAG_ID_PARTNO_AM62X, "AM62X" },
> +	{ JTAG_ID_PARTNO_J784S4, "J784S4" },
> +	{ JTAG_ID_PARTNO_AM62AX, "AM62AX" },
> +	{ JTAG_ID_PARTNO_AM62PX, "AM62PX" },
> +};
> +
> +static char *j721e_rev_string_map[] = {

static const?

> +	"1.0", "1.1",
> +};
> +
> +static char *k3_rev_string_map[] = {

We can drop this (See below)

> +	"1.0", "2.0", "3.0",
>  };
>  
>  static int
> @@ -63,6 +81,29 @@ k3_chipinfo_partno_to_names(unsigned int partno,
>  	return -EINVAL;
>  }
>  
> +static int
> +k3_chipinfo_variant_to_sr(unsigned int partno, unsigned int variant,
> +			  struct soc_device_attribute *soc_dev_attr)
> +{
> +	switch (partno) {
> +	case JTAG_ID_PARTNO_J721E:
> +		if (variant >= ARRAY_SIZE(j721e_rev_string_map))
> +			goto bail;
> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
> +						   j721e_rev_string_map[variant]);
> +		break;
> +	default:
> +		if (variant >= ARRAY_SIZE(k3_rev_string_map))
> +			goto bail;
> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
> +						   k3_rev_string_map[variant]);

How about retaining the old logic as is?

soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);

> +	}

what if !soc_dev_attr->revision error handling?

> +	return 0;
> +
> +bail:

Rename to something like err_unknown_variant ?

> +	return -EINVAL;
return -ENODEV instead to help distinguish between not having memory Vs
not finding a match?

> +}
> +
>  static int k3_chipinfo_probe(struct platform_device *pdev)
>  {
>  	struct device_node *node = pdev->dev.of_node;
> @@ -94,7 +135,6 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>  
>  	variant = (jtag_id & CTRLMMR_WKUP_JTAGID_VARIANT_MASK) >>
>  		  CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT;
> -	variant++;
>  
>  	partno_id = (jtag_id & CTRLMMR_WKUP_JTAGID_PARTNO_MASK) >>
>  		 CTRLMMR_WKUP_JTAGID_PARTNO_SHIFT;
> @@ -103,15 +143,16 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>  	if (!soc_dev_attr)
>  		return -ENOMEM;
>  
> -	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
> -	if (!soc_dev_attr->revision) {
> -		ret = -ENOMEM;
> +	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
> +	if (ret) {
> +		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);

Might be worthwhile to print the errno to distinguish between no mem
fail vs nodev match fail. - see below for k3_chipinfo_partno_to_names

> +		ret = -ENODEV;

don't over-ride the return value - that is probably a preceding cleanup patch
for k3_chipinfo_partno_to_names - also to distinguish between -ENOMEM vs
-ENODEV.

>  		goto err;
>  	}
>  
> -	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
> +	ret = k3_chipinfo_variant_to_sr(partno_id, variant, soc_dev_attr);
>  	if (ret) {
> -		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);
> +		dev_err(dev, "Unknown revision for %s\n", soc_dev_attr->family);
>  		ret = -ENODEV;
>  		goto err_free_rev;
>  	}
> -- 
> 2.34.1
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] soc: ti: k3-socinfo: Revamp driver to accommodate different rev structs
@ 2023-09-15 12:21     ` Nishanth Menon
  0 siblings, 0 replies; 12+ messages in thread
From: Nishanth Menon @ 2023-09-15 12:21 UTC (permalink / raw)
  To: Neha Malcom Francis
  Cc: ssantosh, t-konduru, linux-kernel, linux-arm-kernel, u-kumar1

On 12:16-20230915, Neha Malcom Francis wrote:
> k3-socinfo.c driver assumes silicon revisions to be 1.0, 2.0 etc. for
> every platform. However this typical style is not followed by J721E
> (1.0, 1.1) and need not be followed by upcoming silicon revisions as
> well. Adapt the driver to be similar to its U-Boot counterpart
> (drivers/soc/soc_ti_k3.c) that accounts for this difference on the
> basis of partno/family.
> 
> Note that we change the order of invocation of
> k3_chipinfo_partno_to_names before k3_chipinfo_variant_to_sr so we
> have the family name in case error is returned.

Drop reference to U-boot and others. How about this:

The driver assumes that the silicon revisions for every platform are
incremental and one-to-one, corresponding to JTAG_ID's variant field:
1.0, 2.0, and so on. This assumption is wrong for SoCs such as J721E,
where the variant field to revision mapping is 1,0, 1.1. Further,
there are SoCs such as AM65x where the sub-variant version requires
custom decoding of other registers.

Address this by using conditional handling per JTAG ID that requires
an exception with J721E as the first example. To facilitate this
conversion, use macros to identify the JTAG_ID part number and map them
to predefined string array.

> 
> Signed-off-by: Thejasvi Konduru <t-konduru@ti.com>

Maintain original From or drop this or use Co-developed-by as applicable?

> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
>  drivers/soc/ti/k3-socinfo.c | 71 +++++++++++++++++++++++++++++--------
>  1 file changed, 56 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
> index 6ea9b8c7d335..6de1e3531af9 100644
> --- a/drivers/soc/ti/k3-socinfo.c
> +++ b/drivers/soc/ti/k3-socinfo.c
> @@ -33,19 +33,37 @@
>  
>  #define CTRLMMR_WKUP_JTAGID_MFG_TI		0x17
>  
> +#define JTAG_ID_PARTNO_AM65X		0xBB5A
> +#define JTAG_ID_PARTNO_J721E		0xBB64
> +#define JTAG_ID_PARTNO_J7200		0xBB6D
> +#define JTAG_ID_PARTNO_AM64X		0xBB38
> +#define JTAG_ID_PARTNO_J721S2		0xBB75
> +#define JTAG_ID_PARTNO_AM62X		0xBB7E
> +#define JTAG_ID_PARTNO_J784S4		0xBB80
> +#define JTAG_ID_PARTNO_AM62AX		0xBB8D
> +#define JTAG_ID_PARTNO_AM62PX		0xBB9D
> +
>  static const struct k3_soc_id {
>  	unsigned int id;
>  	const char *family_name;
>  } k3_soc_ids[] = {
> -	{ 0xBB5A, "AM65X" },
> -	{ 0xBB64, "J721E" },
> -	{ 0xBB6D, "J7200" },
> -	{ 0xBB38, "AM64X" },
> -	{ 0xBB75, "J721S2"},
> -	{ 0xBB7E, "AM62X" },
> -	{ 0xBB80, "J784S4" },
> -	{ 0xBB8D, "AM62AX" },
> -	{ 0xBB9D, "AM62PX" },
> +	{ JTAG_ID_PARTNO_AM65X, "AM65X" },
> +	{ JTAG_ID_PARTNO_J721E, "J721E" },
> +	{ JTAG_ID_PARTNO_J7200, "J7200" },
> +	{ JTAG_ID_PARTNO_AM64X, "AM64X" },
> +	{ JTAG_ID_PARTNO_J721S2, "J721S2"},
> +	{ JTAG_ID_PARTNO_AM62X, "AM62X" },
> +	{ JTAG_ID_PARTNO_J784S4, "J784S4" },
> +	{ JTAG_ID_PARTNO_AM62AX, "AM62AX" },
> +	{ JTAG_ID_PARTNO_AM62PX, "AM62PX" },
> +};
> +
> +static char *j721e_rev_string_map[] = {

static const?

> +	"1.0", "1.1",
> +};
> +
> +static char *k3_rev_string_map[] = {

We can drop this (See below)

> +	"1.0", "2.0", "3.0",
>  };
>  
>  static int
> @@ -63,6 +81,29 @@ k3_chipinfo_partno_to_names(unsigned int partno,
>  	return -EINVAL;
>  }
>  
> +static int
> +k3_chipinfo_variant_to_sr(unsigned int partno, unsigned int variant,
> +			  struct soc_device_attribute *soc_dev_attr)
> +{
> +	switch (partno) {
> +	case JTAG_ID_PARTNO_J721E:
> +		if (variant >= ARRAY_SIZE(j721e_rev_string_map))
> +			goto bail;
> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
> +						   j721e_rev_string_map[variant]);
> +		break;
> +	default:
> +		if (variant >= ARRAY_SIZE(k3_rev_string_map))
> +			goto bail;
> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
> +						   k3_rev_string_map[variant]);

How about retaining the old logic as is?

soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);

> +	}

what if !soc_dev_attr->revision error handling?

> +	return 0;
> +
> +bail:

Rename to something like err_unknown_variant ?

> +	return -EINVAL;
return -ENODEV instead to help distinguish between not having memory Vs
not finding a match?

> +}
> +
>  static int k3_chipinfo_probe(struct platform_device *pdev)
>  {
>  	struct device_node *node = pdev->dev.of_node;
> @@ -94,7 +135,6 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>  
>  	variant = (jtag_id & CTRLMMR_WKUP_JTAGID_VARIANT_MASK) >>
>  		  CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT;
> -	variant++;
>  
>  	partno_id = (jtag_id & CTRLMMR_WKUP_JTAGID_PARTNO_MASK) >>
>  		 CTRLMMR_WKUP_JTAGID_PARTNO_SHIFT;
> @@ -103,15 +143,16 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>  	if (!soc_dev_attr)
>  		return -ENOMEM;
>  
> -	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
> -	if (!soc_dev_attr->revision) {
> -		ret = -ENOMEM;
> +	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
> +	if (ret) {
> +		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);

Might be worthwhile to print the errno to distinguish between no mem
fail vs nodev match fail. - see below for k3_chipinfo_partno_to_names

> +		ret = -ENODEV;

don't over-ride the return value - that is probably a preceding cleanup patch
for k3_chipinfo_partno_to_names - also to distinguish between -ENOMEM vs
-ENODEV.

>  		goto err;
>  	}
>  
> -	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
> +	ret = k3_chipinfo_variant_to_sr(partno_id, variant, soc_dev_attr);
>  	if (ret) {
> -		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);
> +		dev_err(dev, "Unknown revision for %s\n", soc_dev_attr->family);
>  		ret = -ENODEV;
>  		goto err_free_rev;
>  	}
> -- 
> 2.34.1
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] soc: ti k3-socinfo: Fix typo
  2023-09-15  6:46   ` Neha Malcom Francis
@ 2023-09-15 12:21     ` Nishanth Menon
  -1 siblings, 0 replies; 12+ messages in thread
From: Nishanth Menon @ 2023-09-15 12:21 UTC (permalink / raw)
  To: Neha Malcom Francis
  Cc: ssantosh, t-konduru, linux-kernel, linux-arm-kernel, u-kumar1

On 12:16-20230915, Neha Malcom Francis wrote:
> Fix typo in driver that comments out wrong bit.
> 
> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
>  drivers/soc/ti/k3-socinfo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
> index 6de1e3531af9..417f3f33cd01 100644
> --- a/drivers/soc/ti/k3-socinfo.c
> +++ b/drivers/soc/ti/k3-socinfo.c
> @@ -20,7 +20,7 @@
>   *  31-28 VARIANT	Device variant
>   *  27-12 PARTNO	Part number
>   *  11-1  MFG		Indicates TI as manufacturer (0x17)
> - *  1			Always 1
> + *  0			Always 1
>   */
>  #define CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT	(28)
>  #define CTRLMMR_WKUP_JTAGID_VARIANT_MASK	GENMASK(31, 28)
> -- 
> 2.34.1
> 

Always do basic cleanup early in your series. reverse the order of the
patches.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] soc: ti k3-socinfo: Fix typo
@ 2023-09-15 12:21     ` Nishanth Menon
  0 siblings, 0 replies; 12+ messages in thread
From: Nishanth Menon @ 2023-09-15 12:21 UTC (permalink / raw)
  To: Neha Malcom Francis
  Cc: ssantosh, t-konduru, linux-kernel, linux-arm-kernel, u-kumar1

On 12:16-20230915, Neha Malcom Francis wrote:
> Fix typo in driver that comments out wrong bit.
> 
> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
>  drivers/soc/ti/k3-socinfo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
> index 6de1e3531af9..417f3f33cd01 100644
> --- a/drivers/soc/ti/k3-socinfo.c
> +++ b/drivers/soc/ti/k3-socinfo.c
> @@ -20,7 +20,7 @@
>   *  31-28 VARIANT	Device variant
>   *  27-12 PARTNO	Part number
>   *  11-1  MFG		Indicates TI as manufacturer (0x17)
> - *  1			Always 1
> + *  0			Always 1
>   */
>  #define CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT	(28)
>  #define CTRLMMR_WKUP_JTAGID_VARIANT_MASK	GENMASK(31, 28)
> -- 
> 2.34.1
> 

Always do basic cleanup early in your series. reverse the order of the
patches.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] soc: ti: k3-socinfo: Revamp driver to accommodate different rev structs
  2023-09-15 12:21     ` Nishanth Menon
@ 2023-09-19  3:47       ` Neha Malcom Francis
  -1 siblings, 0 replies; 12+ messages in thread
From: Neha Malcom Francis @ 2023-09-19  3:47 UTC (permalink / raw)
  To: Nishanth Menon
  Cc: ssantosh, t-konduru, linux-kernel, linux-arm-kernel, u-kumar1

Hi Nishanth

On 15/09/23 17:51, Nishanth Menon wrote:
> On 12:16-20230915, Neha Malcom Francis wrote:
>> k3-socinfo.c driver assumes silicon revisions to be 1.0, 2.0 etc. for
>> every platform. However this typical style is not followed by J721E
>> (1.0, 1.1) and need not be followed by upcoming silicon revisions as
>> well. Adapt the driver to be similar to its U-Boot counterpart
>> (drivers/soc/soc_ti_k3.c) that accounts for this difference on the
>> basis of partno/family.
>>
>> Note that we change the order of invocation of
>> k3_chipinfo_partno_to_names before k3_chipinfo_variant_to_sr so we
>> have the family name in case error is returned.
> 
> Drop reference to U-boot and others. How about this:
> 
> The driver assumes that the silicon revisions for every platform are
> incremental and one-to-one, corresponding to JTAG_ID's variant field:
> 1.0, 2.0, and so on. This assumption is wrong for SoCs such as J721E,
> where the variant field to revision mapping is 1,0, 1.1. Further,
> there are SoCs such as AM65x where the sub-variant version requires
> custom decoding of other registers.
> 
> Address this by using conditional handling per JTAG ID that requires
> an exception with J721E as the first example. To facilitate this
> conversion, use macros to identify the JTAG_ID part number and map them
> to predefined string array.
> 
>>
>> Signed-off-by: Thejasvi Konduru <t-konduru@ti.com>
> 
> Maintain original From or drop this or use Co-developed-by as applicable?
> 
>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
>> ---
>>   drivers/soc/ti/k3-socinfo.c | 71 +++++++++++++++++++++++++++++--------
>>   1 file changed, 56 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
>> index 6ea9b8c7d335..6de1e3531af9 100644
>> --- a/drivers/soc/ti/k3-socinfo.c
>> +++ b/drivers/soc/ti/k3-socinfo.c
>> @@ -33,19 +33,37 @@
>>   
>>   #define CTRLMMR_WKUP_JTAGID_MFG_TI		0x17
>>   
>> +#define JTAG_ID_PARTNO_AM65X		0xBB5A
>> +#define JTAG_ID_PARTNO_J721E		0xBB64
>> +#define JTAG_ID_PARTNO_J7200		0xBB6D
>> +#define JTAG_ID_PARTNO_AM64X		0xBB38
>> +#define JTAG_ID_PARTNO_J721S2		0xBB75
>> +#define JTAG_ID_PARTNO_AM62X		0xBB7E
>> +#define JTAG_ID_PARTNO_J784S4		0xBB80
>> +#define JTAG_ID_PARTNO_AM62AX		0xBB8D
>> +#define JTAG_ID_PARTNO_AM62PX		0xBB9D
>> +
>>   static const struct k3_soc_id {
>>   	unsigned int id;
>>   	const char *family_name;
>>   } k3_soc_ids[] = {
>> -	{ 0xBB5A, "AM65X" },
>> -	{ 0xBB64, "J721E" },
>> -	{ 0xBB6D, "J7200" },
>> -	{ 0xBB38, "AM64X" },
>> -	{ 0xBB75, "J721S2"},
>> -	{ 0xBB7E, "AM62X" },
>> -	{ 0xBB80, "J784S4" },
>> -	{ 0xBB8D, "AM62AX" },
>> -	{ 0xBB9D, "AM62PX" },
>> +	{ JTAG_ID_PARTNO_AM65X, "AM65X" },
>> +	{ JTAG_ID_PARTNO_J721E, "J721E" },
>> +	{ JTAG_ID_PARTNO_J7200, "J7200" },
>> +	{ JTAG_ID_PARTNO_AM64X, "AM64X" },
>> +	{ JTAG_ID_PARTNO_J721S2, "J721S2"},
>> +	{ JTAG_ID_PARTNO_AM62X, "AM62X" },
>> +	{ JTAG_ID_PARTNO_J784S4, "J784S4" },
>> +	{ JTAG_ID_PARTNO_AM62AX, "AM62AX" },
>> +	{ JTAG_ID_PARTNO_AM62PX, "AM62PX" },
>> +};
>> +
>> +static char *j721e_rev_string_map[] = {
> 
> static const?
> 
>> +	"1.0", "1.1",
>> +};
>> +
>> +static char *k3_rev_string_map[] = {
> 
> We can drop this (See below)
> 
>> +	"1.0", "2.0", "3.0",
>>   };
>>   
>>   static int
>> @@ -63,6 +81,29 @@ k3_chipinfo_partno_to_names(unsigned int partno,
>>   	return -EINVAL;
>>   }
>>   
>> +static int
>> +k3_chipinfo_variant_to_sr(unsigned int partno, unsigned int variant,
>> +			  struct soc_device_attribute *soc_dev_attr)
>> +{
>> +	switch (partno) {
>> +	case JTAG_ID_PARTNO_J721E:
>> +		if (variant >= ARRAY_SIZE(j721e_rev_string_map))
>> +			goto bail;
>> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
>> +						   j721e_rev_string_map[variant]);
>> +		break;
>> +	default:
>> +		if (variant >= ARRAY_SIZE(k3_rev_string_map))
>> +			goto bail;
>> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
>> +						   k3_rev_string_map[variant]);
> 
> How about retaining the old logic as is?
> 
> soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
> 
>> +	}
> 
> what if !soc_dev_attr->revision error handling?
> 
>> +	return 0;
>> +
>> +bail:
> 
> Rename to something like err_unknown_variant ?
> 
>> +	return -EINVAL;
> return -ENODEV instead to help distinguish between not having memory Vs
> not finding a match?
> 
>> +}
>> +
>>   static int k3_chipinfo_probe(struct platform_device *pdev)
>>   {
>>   	struct device_node *node = pdev->dev.of_node;
>> @@ -94,7 +135,6 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>>   
>>   	variant = (jtag_id & CTRLMMR_WKUP_JTAGID_VARIANT_MASK) >>
>>   		  CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT;
>> -	variant++;
>>   
>>   	partno_id = (jtag_id & CTRLMMR_WKUP_JTAGID_PARTNO_MASK) >>
>>   		 CTRLMMR_WKUP_JTAGID_PARTNO_SHIFT;
>> @@ -103,15 +143,16 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>>   	if (!soc_dev_attr)
>>   		return -ENOMEM;
>>   
>> -	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
>> -	if (!soc_dev_attr->revision) {
>> -		ret = -ENOMEM;
>> +	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
>> +	if (ret) {
>> +		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);
> 
> Might be worthwhile to print the errno to distinguish between no mem
> fail vs nodev match fail. - see below for k3_chipinfo_partno_to_names
> 
>> +		ret = -ENODEV;
> 
> don't over-ride the return value - that is probably a preceding cleanup patch
> for k3_chipinfo_partno_to_names - also to distinguish between -ENOMEM vs
> -ENODEV.
> 
>>   		goto err;
>>   	}
>>   
>> -	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
>> +	ret = k3_chipinfo_variant_to_sr(partno_id, variant, soc_dev_attr);
>>   	if (ret) {
>> -		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);
>> +		dev_err(dev, "Unknown revision for %s\n", soc_dev_attr->family);
>>   		ret = -ENODEV;
>>   		goto err_free_rev;
>>   	}
>> -- 
>> 2.34.1
>>
> 

Thanks for the detailed review! I will work on it and send v3.

-- 
Thanking You
Neha Malcom Francis

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] soc: ti: k3-socinfo: Revamp driver to accommodate different rev structs
@ 2023-09-19  3:47       ` Neha Malcom Francis
  0 siblings, 0 replies; 12+ messages in thread
From: Neha Malcom Francis @ 2023-09-19  3:47 UTC (permalink / raw)
  To: Nishanth Menon
  Cc: ssantosh, t-konduru, linux-kernel, linux-arm-kernel, u-kumar1

Hi Nishanth

On 15/09/23 17:51, Nishanth Menon wrote:
> On 12:16-20230915, Neha Malcom Francis wrote:
>> k3-socinfo.c driver assumes silicon revisions to be 1.0, 2.0 etc. for
>> every platform. However this typical style is not followed by J721E
>> (1.0, 1.1) and need not be followed by upcoming silicon revisions as
>> well. Adapt the driver to be similar to its U-Boot counterpart
>> (drivers/soc/soc_ti_k3.c) that accounts for this difference on the
>> basis of partno/family.
>>
>> Note that we change the order of invocation of
>> k3_chipinfo_partno_to_names before k3_chipinfo_variant_to_sr so we
>> have the family name in case error is returned.
> 
> Drop reference to U-boot and others. How about this:
> 
> The driver assumes that the silicon revisions for every platform are
> incremental and one-to-one, corresponding to JTAG_ID's variant field:
> 1.0, 2.0, and so on. This assumption is wrong for SoCs such as J721E,
> where the variant field to revision mapping is 1,0, 1.1. Further,
> there are SoCs such as AM65x where the sub-variant version requires
> custom decoding of other registers.
> 
> Address this by using conditional handling per JTAG ID that requires
> an exception with J721E as the first example. To facilitate this
> conversion, use macros to identify the JTAG_ID part number and map them
> to predefined string array.
> 
>>
>> Signed-off-by: Thejasvi Konduru <t-konduru@ti.com>
> 
> Maintain original From or drop this or use Co-developed-by as applicable?
> 
>> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
>> ---
>>   drivers/soc/ti/k3-socinfo.c | 71 +++++++++++++++++++++++++++++--------
>>   1 file changed, 56 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
>> index 6ea9b8c7d335..6de1e3531af9 100644
>> --- a/drivers/soc/ti/k3-socinfo.c
>> +++ b/drivers/soc/ti/k3-socinfo.c
>> @@ -33,19 +33,37 @@
>>   
>>   #define CTRLMMR_WKUP_JTAGID_MFG_TI		0x17
>>   
>> +#define JTAG_ID_PARTNO_AM65X		0xBB5A
>> +#define JTAG_ID_PARTNO_J721E		0xBB64
>> +#define JTAG_ID_PARTNO_J7200		0xBB6D
>> +#define JTAG_ID_PARTNO_AM64X		0xBB38
>> +#define JTAG_ID_PARTNO_J721S2		0xBB75
>> +#define JTAG_ID_PARTNO_AM62X		0xBB7E
>> +#define JTAG_ID_PARTNO_J784S4		0xBB80
>> +#define JTAG_ID_PARTNO_AM62AX		0xBB8D
>> +#define JTAG_ID_PARTNO_AM62PX		0xBB9D
>> +
>>   static const struct k3_soc_id {
>>   	unsigned int id;
>>   	const char *family_name;
>>   } k3_soc_ids[] = {
>> -	{ 0xBB5A, "AM65X" },
>> -	{ 0xBB64, "J721E" },
>> -	{ 0xBB6D, "J7200" },
>> -	{ 0xBB38, "AM64X" },
>> -	{ 0xBB75, "J721S2"},
>> -	{ 0xBB7E, "AM62X" },
>> -	{ 0xBB80, "J784S4" },
>> -	{ 0xBB8D, "AM62AX" },
>> -	{ 0xBB9D, "AM62PX" },
>> +	{ JTAG_ID_PARTNO_AM65X, "AM65X" },
>> +	{ JTAG_ID_PARTNO_J721E, "J721E" },
>> +	{ JTAG_ID_PARTNO_J7200, "J7200" },
>> +	{ JTAG_ID_PARTNO_AM64X, "AM64X" },
>> +	{ JTAG_ID_PARTNO_J721S2, "J721S2"},
>> +	{ JTAG_ID_PARTNO_AM62X, "AM62X" },
>> +	{ JTAG_ID_PARTNO_J784S4, "J784S4" },
>> +	{ JTAG_ID_PARTNO_AM62AX, "AM62AX" },
>> +	{ JTAG_ID_PARTNO_AM62PX, "AM62PX" },
>> +};
>> +
>> +static char *j721e_rev_string_map[] = {
> 
> static const?
> 
>> +	"1.0", "1.1",
>> +};
>> +
>> +static char *k3_rev_string_map[] = {
> 
> We can drop this (See below)
> 
>> +	"1.0", "2.0", "3.0",
>>   };
>>   
>>   static int
>> @@ -63,6 +81,29 @@ k3_chipinfo_partno_to_names(unsigned int partno,
>>   	return -EINVAL;
>>   }
>>   
>> +static int
>> +k3_chipinfo_variant_to_sr(unsigned int partno, unsigned int variant,
>> +			  struct soc_device_attribute *soc_dev_attr)
>> +{
>> +	switch (partno) {
>> +	case JTAG_ID_PARTNO_J721E:
>> +		if (variant >= ARRAY_SIZE(j721e_rev_string_map))
>> +			goto bail;
>> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
>> +						   j721e_rev_string_map[variant]);
>> +		break;
>> +	default:
>> +		if (variant >= ARRAY_SIZE(k3_rev_string_map))
>> +			goto bail;
>> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
>> +						   k3_rev_string_map[variant]);
> 
> How about retaining the old logic as is?
> 
> soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
> 
>> +	}
> 
> what if !soc_dev_attr->revision error handling?
> 
>> +	return 0;
>> +
>> +bail:
> 
> Rename to something like err_unknown_variant ?
> 
>> +	return -EINVAL;
> return -ENODEV instead to help distinguish between not having memory Vs
> not finding a match?
> 
>> +}
>> +
>>   static int k3_chipinfo_probe(struct platform_device *pdev)
>>   {
>>   	struct device_node *node = pdev->dev.of_node;
>> @@ -94,7 +135,6 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>>   
>>   	variant = (jtag_id & CTRLMMR_WKUP_JTAGID_VARIANT_MASK) >>
>>   		  CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT;
>> -	variant++;
>>   
>>   	partno_id = (jtag_id & CTRLMMR_WKUP_JTAGID_PARTNO_MASK) >>
>>   		 CTRLMMR_WKUP_JTAGID_PARTNO_SHIFT;
>> @@ -103,15 +143,16 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>>   	if (!soc_dev_attr)
>>   		return -ENOMEM;
>>   
>> -	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
>> -	if (!soc_dev_attr->revision) {
>> -		ret = -ENOMEM;
>> +	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
>> +	if (ret) {
>> +		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);
> 
> Might be worthwhile to print the errno to distinguish between no mem
> fail vs nodev match fail. - see below for k3_chipinfo_partno_to_names
> 
>> +		ret = -ENODEV;
> 
> don't over-ride the return value - that is probably a preceding cleanup patch
> for k3_chipinfo_partno_to_names - also to distinguish between -ENOMEM vs
> -ENODEV.
> 
>>   		goto err;
>>   	}
>>   
>> -	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
>> +	ret = k3_chipinfo_variant_to_sr(partno_id, variant, soc_dev_attr);
>>   	if (ret) {
>> -		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);
>> +		dev_err(dev, "Unknown revision for %s\n", soc_dev_attr->family);
>>   		ret = -ENODEV;
>>   		goto err_free_rev;
>>   	}
>> -- 
>> 2.34.1
>>
> 

Thanks for the detailed review! I will work on it and send v3.

-- 
Thanking You
Neha Malcom Francis

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-09-19  3:48 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-15  6:46 [PATCH v2 0/2] Revamp k3-socinfo driver Neha Malcom Francis
2023-09-15  6:46 ` Neha Malcom Francis
2023-09-15  6:46 ` [PATCH v2 1/2] soc: ti: k3-socinfo: Revamp driver to accommodate different rev structs Neha Malcom Francis
2023-09-15  6:46   ` Neha Malcom Francis
2023-09-15 12:21   ` Nishanth Menon
2023-09-15 12:21     ` Nishanth Menon
2023-09-19  3:47     ` Neha Malcom Francis
2023-09-19  3:47       ` Neha Malcom Francis
2023-09-15  6:46 ` [PATCH v2 2/2] soc: ti k3-socinfo: Fix typo Neha Malcom Francis
2023-09-15  6:46   ` Neha Malcom Francis
2023-09-15 12:21   ` Nishanth Menon
2023-09-15 12:21     ` Nishanth Menon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.