[PATCH v3 0/3] Revamp k3-socinfo driver

[PATCH v3 0/3] Revamp k3-socinfo driver

[Neha Malcom Francis]

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 v2:
	- Nishanth:
		- update commit message
		- move from double Signed-off-by to Co-developed-by
		- make j721e_rev_string_map[] a const char
		- drop k3_rev_string_map[] and continue using old
		  "variant++" logic for the typical cases
		- appropriate error handling with no overrides
		  distinguishing between ENODEV and ENOMEM
		- add patch for error handling initial cleanup
		- reorder patches

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)

Boot log: https://gist.github.com/nehamalcom/ff9375dcde681dd78712ee8473b24a50
(See relevant k3-socinfo dev_info print on line 276)

v2: https://lore.kernel.org/lkml/20230915064650.2287638-1-n-francis@ti.com/T/
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 (3):
  soc: ti k3-socinfo: Fix typo
  soc: ti: k3-socinfo: Avoid overriding ret
  soc: ti: k3-socinfo: Revamp driver to accommodate different rev
    structs

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

-- 
2.34.1

[PATCH v3 1/3] soc: ti k3-socinfo: Fix typo

[Neha Malcom Francis]

Fix typo in driver that comments out wrong bit.

Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
No change since v2

 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 6ea9b8c7d335..d45f5cb955a6 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

[PATCH v3 2/3] soc: ti: k3-socinfo: Avoid overriding ret

[Neha Malcom Francis]

Avoid overriding the return value and make sure the right error code is
reflected. Here, if the partno is none of the identified partnos present
in k3_soc_ids[], return -ENODEV.

Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
---
Changes since v2:
	- new patch

 drivers/soc/ti/k3-socinfo.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
index d45f5cb955a6..7fc3548e084c 100644
--- a/drivers/soc/ti/k3-socinfo.c
+++ b/drivers/soc/ti/k3-socinfo.c
@@ -60,7 +60,7 @@ k3_chipinfo_partno_to_names(unsigned int partno,
 			return 0;
 		}
 
-	return -EINVAL;
+	return -ENODEV;
 }
 
 static int k3_chipinfo_probe(struct platform_device *pdev)
@@ -111,8 +111,7 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
 
 	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;
+		dev_err(dev, "Unknown SoC JTAGID[0x%08X]: %d\n", jtag_id, ret);
 		goto err_free_rev;
 	}
 
-- 
2.34.1

[PATCH v3 3/3] soc: ti: k3-socinfo: Revamp driver to accommodate different rev structs

[Neha Malcom Francis]

k3-socinfo.c driver assumes silicon revisions for every platform are
incremental and one-to-one, corresponding to JTAG_ID's variant field:
1.0, 2.0 etc. 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: Neha Malcom Francis <n-francis@ti.com>
Co-developed-by: Thejasvi Konduru <t-konduru@ti.com>
Signed-off-by: Thejasvi Konduru <t-konduru@ti.com>
---
Changes since v2:
	- update commit message
	- move from double Signed-off-by to Co-developed-by
	- make j721e_rev_string_map[] a const char
	- drop k3_rev_string_map[] and continue using old
	  "variant++" logic for the typical cases
	- appropriate error handling with no overrides distinguishing
	  between ENODEV and ENOMEM
 
 drivers/soc/ti/k3-socinfo.c | 71 ++++++++++++++++++++++++++++---------
 1 file changed, 55 insertions(+), 16 deletions(-)

diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
index 7fc3548e084c..7517a9c8c8fa 100644
--- a/drivers/soc/ti/k3-socinfo.c
+++ b/drivers/soc/ti/k3-socinfo.c
@@ -33,19 +33,33 @@
 
 #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 const char * const j721e_rev_string_map[] = {
+	"1.0", "1.1",
 };
 
 static int
@@ -63,6 +77,32 @@ k3_chipinfo_partno_to_names(unsigned int partno,
 	return -ENODEV;
 }
 
+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 err_unknown_variant;
+		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
+						   j721e_rev_string_map[variant]);
+		break;
+	default:
+		variant++;
+		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0",
+						   variant);
+	}
+
+	if (!soc_dev_attr->revision)
+		return -ENOMEM;
+
+	return 0;
+
+err_unknown_variant:
+	return -ENODEV;
+}
+
 static int k3_chipinfo_probe(struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
@@ -94,7 +134,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,16 +142,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]: %d\n", jtag_id, ret);
 		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]: %d\n", jtag_id, ret);
-		goto err_free_rev;
+		dev_err(dev, "Unknown SoC SR[0x%08X]: %d\n", jtag_id, ret);
+		goto err;
 	}
 
 	node = of_find_node_by_path("/");
-- 
2.34.1

Re: [PATCH v3 0/3] Revamp k3-socinfo driver

[Nishanth Menon]

Hi Neha Malcom Francis,

On Mon, 16 Oct 2023 15:46:05 +0530, Neha Malcom Francis wrote:
> 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 v2:
> 	- Nishanth:
> 		- update commit message
> 		- move from double Signed-off-by to Co-developed-by
> 		- make j721e_rev_string_map[] a const char
> 		- drop k3_rev_string_map[] and continue using old
> 		  "variant++" logic for the typical cases
> 		- appropriate error handling with no overrides
> 		  distinguishing between ENODEV and ENOMEM
> 		- add patch for error handling initial cleanup
> 		- reorder patches
> 
> [...]

I have applied the following to branch ti-drivers-soc-next on [1] with minor
fixups.

Thank you!

[1/3] soc: ti k3-socinfo: Fix typo
      commit: 8dec342ead710dace27dc82096144bf7a1011827
[2/3] soc: ti: k3-socinfo: Avoid overriding ret
      commit: 3aeb0d3694e16b5066db82aa1152884f2e6aace0
[3/3] soc: ti: k3-socinfo: Revamp driver to accommodate different rev structs
      commit: 07e651db2d78eac4c41d7144eb5ea734af2328fc

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent up the chain during
the next merge window (or sooner if it is a relevant bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/ti/linux.git
-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v3 3/3] soc: ti: k3-socinfo: Revamp driver to accommodate different rev structs

[Nishanth Menon]

On 15:46-20231016, Neha Malcom Francis wrote:
> k3-socinfo.c driver assumes silicon revisions for every platform are
> incremental and one-to-one, corresponding to JTAG_ID's variant field:
> 1.0, 2.0 etc. 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: Neha Malcom Francis <n-francis@ti.com>
> Co-developed-by: Thejasvi Konduru <t-konduru@ti.com>
> Signed-off-by: Thejasvi Konduru <t-konduru@ti.com>
> ---
> Changes since v2:
> 	- update commit message
> 	- move from double Signed-off-by to Co-developed-by
> 	- make j721e_rev_string_map[] a const char
> 	- drop k3_rev_string_map[] and continue using old
> 	  "variant++" logic for the typical cases
> 	- appropriate error handling with no overrides distinguishing
> 	  between ENODEV and ENOMEM
>  

Thanks to linux-next testing, looks like this created regression in networking.

https://lore.kernel.org/all/20231018140009.1725-1-r-gunasekaran@ti.com/

I will drop this patch for now from my queue. I suggest the following
for the eventual resolution:
a) Do all the prep work in a series of atomic bisectable patches prior to introducing
j721e SR change
b) Please sync with Ravi and netdev maintainers as to how to introduce
the changes
c) also introduce changes (if required) to other SoCs as required.

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

Re: (subset) [PATCH v3 0/3] Revamp k3-socinfo driver

[Nishanth Menon]

Hi Neha Malcom Francis,

On Mon, 16 Oct 2023 15:46:05 +0530, Neha Malcom Francis wrote:
> 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 v2:
> 	- Nishanth:
> 		- update commit message
> 		- move from double Signed-off-by to Co-developed-by
> 		- make j721e_rev_string_map[] a const char
> 		- drop k3_rev_string_map[] and continue using old
> 		  "variant++" logic for the typical cases
> 		- appropriate error handling with no overrides
> 		  distinguishing between ENODEV and ENOMEM
> 		- add patch for error handling initial cleanup
> 		- reorder patches
> 
> [...]

I have applied the following to branch ti-drivers-soc-next on [1].
Thank you!

[3/3] soc: ti: k3-socinfo: Revamp driver to accommodate different rev structs
      commit: 3ba080bf46e4a9039d0d41356f4a2515e00bf747

The networking dependency previously identified has been sorted out
and available in linux-next[2] since next-20231206. So, this is a good
time for us to merge the patch that I have been keeping held up. At least
on my local scan, I don't see any other in-tree driver impacted by this.

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent up the chain during
the next merge window (or sooner if it is a relevant bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/ti/linux.git
[2] https://lore.kernel.org/all/20231201132033.29576-1-r-gunasekaran@ti.com/
-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D