Skip to contentSkip to conversation

dt-bindings: nvmem: Remove fsl,t1023-sfp in favor of fsl,layerscape-sfp

PATCH In review Sean Anderson 5 participants 16 messages Mar 16, 2024 – Mar 22, 2024

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/2 dt-bindings: nvmem: Remove fsl,t1023-sfp in favor of fsl,layerscape-sfp

These bindings document the same hardware (just different minor revisions). Remove the newer fsl,t1023-sfp.

Fixes aa1ed6047107 ("dt-bindings: nvmem: Add t1023-sfp efuse support")

---

 .../bindings/nvmem/fsl,layerscape-sfp.yaml    | 21 ++++++++++-
 .../bindings/nvmem/fsl,t1023-sfp.yaml         | 37 -------------------
 2 files changed, 19 insertions(+), 39 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/nvmem/fsl,t1023-sfp.yaml
Documentation/devicetree/bindings/nvmem/fsl,layerscape-sfp.yaml +19 -2
index 70fb2ad25103..8a54dd1328ea 100644
@@ -19,6 +19,13 @@ allOf:
 properties:
   compatible:
     oneOf:
+      - description: Trust architecture 2.0 SFP
+        items:
+          enum:
+            - fsl,t1023-sfp
+            - fsl,t1040-sfp
+            - fsl,t2080-sfp
+            - fsl,t4240-sfp
       - description: Trust architecture 2.1 SFP
         items:
           - const: fsl,ls1021a-sfp
@@ -45,8 +52,18 @@ properties:
 required:
   - compatible
   - reg
-  - clock-names
-  - clocks
+
+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - fsl,ls1021a-sfp
+          - fsl,ls1028a-sfp
+then:
+  required:
+    - clocks
+    - clock-names
 
 unevaluatedProperties: false
 
Documentation/devicetree/bindings/nvmem/fsl,t1023-sfp.yaml deleted +0 -37
deleted file mode 100644
index df826b40d8ca..000000000000
@@ -1,37 +0,0 @@
-# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
-%YAML 1.2
----
-$id: http://devicetree.org/schemas/nvmem/fsl,t1023-sfp.yaml#
-$schema: http://devicetree.org/meta-schemas/core.yaml#
-
-title: NXP QorIQ eFuse support
-
-maintainers:
-  - Richard Alpe <[email protected]>
-
-description:
-  Read support for the eFuses (SFP) on NXP QorIQ series SoC's.
-
-allOf:
-  - $ref: nvmem.yaml#
-
-properties:
-  compatible:
-    const: fsl,t1023-sfp
-
-  reg:
-    maxItems: 1
-
-required:
-  - compatible
-  - reg
-
-unevaluatedProperties: false
-
-examples:
-  - |
-    efuse@e8000 {
-        compatible = "fsl,t1023-sfp";
-        reg = <0xe8000 0x1000>;
-    };
-...
Signature · 1 line
2.35.1.1320.gc452695387.dirty
#1 lore raw

PATCH 2/2 nvmem: Remove qoriq-efuse in favor of layerscape-sfp

The qoriq-efuse driver is a duplicate of layerscape-sfp.c. The T-series uses TA 2.0, while Layerscape uses TA 2.1 or 3.0 (depending on the chip). Add appropriate compatibles to the layerscape-sfp driver and remove the qoriq-efuse driver. I did not add support for P-series SoCs, since they use TA 1.0 which doesn't share a major version with either of the existing implementations.

The qoriq-efuse driver does not properly abstract the location/offset of the fuses properly, instead exposing the device's whole address range to userspace. This is not appropriate, as the fuses only occupy a small portion of this range. The layerscape-sfp module correctly constrains the nvmem size to the fuses size. This represents a (necessary) compatibility break. The qoriq-efuse driver has been in-tree for around six months. Hopefully this will limit the fallout.

I would appreciate if someone with access to trust architecture 2.0 user guide could confirm the number of fuses.

Fixes 0861110bb421 ("nvmem: add new NXP QorIQ eFuse driver")

---

 drivers/nvmem/Kconfig          | 14 +-----
 drivers/nvmem/Makefile         |  2 -
 drivers/nvmem/layerscape-sfp.c | 27 +++++++++---
 drivers/nvmem/qoriq-efuse.c    | 78 ----------------------------------
 4 files changed, 22 insertions(+), 99 deletions(-)
 delete mode 100644 drivers/nvmem/qoriq-efuse.c
drivers/nvmem/Kconfig +1 -13
index 5bc9c4874fe3..81531ebb073d 100644
@@ -122,7 +122,7 @@ config NVMEM_LAN9662_OTPC
 
 config NVMEM_LAYERSCAPE_SFP
 	tristate "Layerscape SFP (Security Fuse Processor) support"
-	depends on ARCH_LAYERSCAPE || COMPILE_TEST
+	depends on ARCH_LAYERSCAPE || PPC_85xx || COMPILE_TEST
 	depends on HAS_IOMEM
 	select REGMAP_MMIO
 	help
@@ -405,16 +405,4 @@ config NVMEM_ZYNQMP
 
 	  If sure, say yes. If unsure, say no.
 
-config NVMEM_QORIQ_EFUSE
-	tristate "NXP QorIQ eFuse support"
-	depends on PPC_85xx || COMPILE_TEST
-	depends on HAS_IOMEM
-	help
-	  This driver provides read support for the eFuses (SFP) on NXP QorIQ
-	  series SoC's. This includes secure boot settings, the globally unique
-	  NXP ID 'FUIDR' and the OEM unique ID 'OUIDR'.
-
-	  This driver can also be built as a module. If so, the module
-	  will be called nvmem_qoriq_efuse.
-
 endif
drivers/nvmem/Makefile +0 -2
index 423baf089515..a276447b8c2f 100644
@@ -79,5 +79,3 @@ obj-$(CONFIG_NVMEM_VF610_OCOTP)		+= nvmem-vf610-ocotp.o
 nvmem-vf610-ocotp-y			:= vf610-ocotp.o
 obj-$(CONFIG_NVMEM_ZYNQMP)		+= nvmem_zynqmp_nvmem.o
 nvmem_zynqmp_nvmem-y			:= zynqmp_nvmem.o
-obj-$(CONFIG_NVMEM_QORIQ_EFUSE)		+= nvmem-qoriq-efuse.o
-nvmem-qoriq-efuse-y			:= qoriq-efuse.o
drivers/nvmem/layerscape-sfp.c +21 -6
index e2b424561949..7e32133e492a 100644
@@ -22,8 +22,9 @@ struct layerscape_sfp_priv {
 };
 
 struct layerscape_sfp_data {
-	int size;
+	int size, id;
 	enum regmap_endian endian;
+	const char *name;
 };
 
 static int layerscape_sfp_read(void *context, unsigned int offset, void *val,
@@ -37,7 +38,6 @@ static int layerscape_sfp_read(void *context, unsigned int offset, void *val,
 }
 
 static struct nvmem_config layerscape_sfp_nvmem_config = {
-	.name = "fsl-sfp",
 	.reg_read = layerscape_sfp_read,
 	.word_size = 4,
 	.stride = 4,
@@ -69,6 +69,8 @@ static int layerscape_sfp_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->regmap))
 		return PTR_ERR(priv->regmap);
 
+	layerscape_sfp_nvmem_config.name = data->name;
+	layerscape_sfp_nvmem_config.id = data->id;
 	layerscape_sfp_nvmem_config.size = data->size;
 	layerscape_sfp_nvmem_config.dev = &pdev->dev;
 	layerscape_sfp_nvmem_config.priv = priv;
@@ -78,19 +80,32 @@ static int layerscape_sfp_probe(struct platform_device *pdev)
 	return PTR_ERR_OR_ZERO(nvmem);
 }
 
-static const struct layerscape_sfp_data ls1021a_data = {
+static const struct layerscape_sfp_data ta2_0_data = {
 	.size = 0x88,
 	.endian = REGMAP_ENDIAN_BIG,
+	.name = "qoriq_efuse_read",
+	.id = NVMEM_DEVID_AUTO,
 };
 
-static const struct layerscape_sfp_data ls1028a_data = {
+static const struct layerscape_sfp_data ta2_1_data = {
+	.size = 0x88,
+	.endian = REGMAP_ENDIAN_BIG,
+	.name = "fsl-sfp",
+};
+
+static const struct layerscape_sfp_data ta3_0_data = {
 	.size = 0x88,
 	.endian = REGMAP_ENDIAN_LITTLE,
+	.name = "fsl-sfp",
 };
 
 static const struct of_device_id layerscape_sfp_dt_ids[] = {
-	{ .compatible = "fsl,ls1021a-sfp", .data = &ls1021a_data },
-	{ .compatible = "fsl,ls1028a-sfp", .data = &ls1028a_data },
+	{ .compatible = "fsl,ls1021a-sfp", .data = &ta2_1_data },
+	{ .compatible = "fsl,ls1028a-sfp", .data = &ta3_0_data },
+	{ .compatible = "fsl,t1023-sfp", .data = &ta2_0_data },
+	{ .compatible = "fsl,t1040-sfp", .data = &ta2_0_data },
+	{ .compatible = "fsl,t2080-sfp", .data = &ta2_0_data },
+	{ .compatible = "fsl,t4240-sfp", .data = &ta2_0_data },
 	{},
 };
 MODULE_DEVICE_TABLE(of, layerscape_sfp_dt_ids);
drivers/nvmem/qoriq-efuse.c deleted +0 -78

78 lines not on this page · raw message

Signature · 1 line
2.35.1.1320.gc452695387.dirty
Acked-by on v1 2/2
#2 lore raw

Re: [PATCH 2/2] nvmem: Remove qoriq-efuse in favor of layerscape-sfp

Quoted 15 lines · Sean Anderson

The qoriq-efuse driver is a duplicate of layerscape-sfp.c. The T-series uses TA 2.0, while Layerscape uses TA 2.1 or 3.0 (depending on the chip). Add appropriate compatibles to the layerscape-sfp driver and remove the qoriq-efuse driver. I did not add support for P-series SoCs, since they use TA 1.0 which doesn't share a major version with either of the existing implementations.

The qoriq-efuse driver does not properly abstract the location/offset of the fuses properly, instead exposing the device's whole address range to userspace. This is not appropriate, as the fuses only occupy a small portion of this range. The layerscape-sfp module correctly constrains the nvmem size to the fuses size. This represents a (necessary) compatibility break. The qoriq-efuse driver has been in-tree for around six months. Hopefully this will limit the fallout.

Quoted 5 lines

I would appreciate if someone with access to trust architecture 2.0 user guide could confirm the number of fuses.

Fixes 0861110bb421 ("nvmem: add new NXP QorIQ eFuse driver")

I don't think Fixes is appropriate here. Apart from that:

#3 lore raw

Re: [PATCH 2/2] nvmem: Remove qoriq-efuse in favor of layerscape-sfp

Quoted 16 lines · On 3/18/24 04:32, [email protected] wrote:
Quoted 15 lines · Sean Anderson

The qoriq-efuse driver is a duplicate of layerscape-sfp.c. The T-series uses TA 2.0, while Layerscape uses TA 2.1 or 3.0 (depending on the chip). Add appropriate compatibles to the layerscape-sfp driver and remove the qoriq-efuse driver. I did not add support for P-series SoCs, since they use TA 1.0 which doesn't share a major version with either of the existing implementations.

The qoriq-efuse driver does not properly abstract the location/offset of the fuses properly, instead exposing the device's whole address range to userspace. This is not appropriate, as the fuses only occupy a small portion of this range. The layerscape-sfp module correctly constrains the nvmem size to the fuses size. This represents a (necessary) compatibility break. The qoriq-efuse driver has been in-tree for around six months. Hopefully this will limit the fallout.

Quoted 6 lines
Quoted 5 lines

I would appreciate if someone with access to trust architecture 2.0 user guide could confirm the number of fuses.

Fixes 0861110bb421 ("nvmem: add new NXP QorIQ eFuse driver")

I don't think Fixes is appropriate here. Apart from that:

As mentioned in the second paragraph, the original driver exposes the whole register space in the nvmem. I consider this a bug.

--Sean

Quoted 1 line
#4 lore raw
In reply to Sean Anderson
Quoted 18 lines · Sean Anderson

These bindings document the same hardware (just different minor revisions). Remove the newer fsl,t1023-sfp.

Fixes aa1ed6047107 ("dt-bindings: nvmem: Add t1023-sfp efuse support")

---

 .../bindings/nvmem/fsl,layerscape-sfp.yaml    | 21 ++++++++++-
 .../bindings/nvmem/fsl,t1023-sfp.yaml         | 37 -------------------
 2 files changed, 19 insertions(+), 39 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/nvmem/fsl,t1023-sfp.yaml
Documentation/devicetree/bindings/nvmem/fsl,layerscape-sfp.yaml +0 -0
index 70fb2ad25103..8a54dd1328ea 100644
@@ -19,6 +19,13 @@ allOf:
 properties:

Documentation/devicetree/bindings/nvmem/fsl,layerscape-sfp.yaml allOf: · line 29

Quoted 10 lines
Documentation/devicetree/bindings/nvmem/fsl,layerscape-sfp.yaml +7 -0
   compatible:
     oneOf:
+      - description: Trust architecture 2.0 SFP
+        items:
+          enum:
+            - fsl,t1023-sfp
 
+            - fsl,t1040-sfp
+            - fsl,t2080-sfp
+            - fsl,t4240-sfp

You should at least mention where these three compatibles came from, given they were not documented in t1023-sfp.yaml. Additionally, should they fall back to t1023-sfp? I see that there's already some dts files with these compatibles in them but seemingly no driver support as there is for the t1023-sfp.

Cheers,
Conor.

Quoted 70 lines
Documentation/devicetree/bindings/nvmem/fsl,layerscape-sfp.yaml +0 -0
       - description: Trust architecture 2.1 SFP
         items:

- const: fsl,ls1021a-sfp

Documentation/devicetree/bindings/nvmem/fsl,layerscape-sfp.yaml +12 -2
@@ -45,8 +52,18 @@ properties:
 required:
   - compatible
   - reg
-  - clock-names
-  - clocks
+
+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - fsl,ls1021a-sfp
+          - fsl,ls1028a-sfp
+then:
+  required:
+    - clocks
+    - clock-names
 
 unevaluatedProperties: false
 
Documentation/devicetree/bindings/nvmem/fsl,t1023-sfp.yaml deleted +0 -37
deleted file mode 100644
index df826b40d8ca..000000000000
@@ -1,37 +0,0 @@
-# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
-%YAML 1.2
----
-$id: http://devicetree.org/schemas/nvmem/fsl,t1023-sfp.yaml#
-$schema: http://devicetree.org/meta-schemas/core.yaml#
-
-title: NXP QorIQ eFuse support
-
-maintainers:
-  - Richard Alpe <[email protected]>
-
-description:
-  Read support for the eFuses (SFP) on NXP QorIQ series SoC's.
-
-allOf:
-  - $ref: nvmem.yaml#
-
-properties:
-  compatible:
-    const: fsl,t1023-sfp
-
-  reg:
-    maxItems: 1
-
-required:
-  - compatible
-  - reg
-
-unevaluatedProperties: false
-
-examples:
-  - |
-    efuse@e8000 {
-        compatible = "fsl,t1023-sfp";
-        reg = <0xe8000 0x1000>;
-    };
-...
Signature · 1 line
2.35.1.1320.gc452695387.dirty
#5 lore raw
Quoted 26 lines · Conor Dooley
Quoted 24 lines · Sean Anderson

These bindings document the same hardware (just different minor revisions). Remove the newer fsl,t1023-sfp.

Fixes aa1ed6047107 ("dt-bindings: nvmem: Add t1023-sfp efuse support")

---

 .../bindings/nvmem/fsl,layerscape-sfp.yaml    | 21 ++++++++++-
 .../bindings/nvmem/fsl,t1023-sfp.yaml         | 37 -------------------
 2 files changed, 19 insertions(+), 39 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/nvmem/fsl,t1023-sfp.yaml
Documentation/devicetree/bindings/nvmem/fsl,layerscape-sfp.yaml +4 -0
index 70fb2ad25103..8a54dd1328ea 100644
@@ -19,6 +19,13 @@ allOf:
 properties:
   compatible:
     oneOf:
+      - description: Trust architecture 2.0 SFP
+        items:
+          enum:
+            - fsl,t1023-sfp
Quoted 6 lines

Documentation/devicetree/bindings/nvmem/fsl,layerscape-sfp.yaml allOf: · line 28

Quoted 3 lines
Documentation/devicetree/bindings/nvmem/fsl,layerscape-sfp.yaml +3 -0
+            - fsl,t1040-sfp
+            - fsl,t2080-sfp
+            - fsl,t4240-sfp

You should at least mention where these three compatibles came from, given they were not documented in t1023-sfp.yaml

They come from the existing DTSs for these processors. Will add this to the commit message.

Quoted 4 lines

Additionally, should
they fall back to t1023-sfp? I see that there's already some dts files with these compatibles in them but seemingly no driver support as there is for the t1023-sfp.

I checked the reference manuals for these processors, and all of them use TA 2.0.

--Sean

Quoted 70 lines
Quoted 70 lines
Documentation/devicetree/bindings/nvmem/fsl,layerscape-sfp.yaml +12 -2
       - description: Trust architecture 2.1 SFP
         items:
           - const: fsl,ls1021a-sfp
@@ -45,8 +52,18 @@ properties:
 required:
   - compatible
   - reg
-  - clock-names
-  - clocks
+
+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - fsl,ls1021a-sfp
+          - fsl,ls1028a-sfp
+then:
+  required:
+    - clocks
+    - clock-names
 
 unevaluatedProperties: false
 
Documentation/devicetree/bindings/nvmem/fsl,t1023-sfp.yaml deleted +0 -37
deleted file mode 100644
index df826b40d8ca..000000000000
@@ -1,37 +0,0 @@
-# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
-%YAML 1.2
----
-$id: http://devicetree.org/schemas/nvmem/fsl,t1023-sfp.yaml#
-$schema: http://devicetree.org/meta-schemas/core.yaml#
-
-title: NXP QorIQ eFuse support
-
-maintainers:
-  - Richard Alpe <[email protected]>
-
-description:
-  Read support for the eFuses (SFP) on NXP QorIQ series SoC's.
-
-allOf:
-  - $ref: nvmem.yaml#
-
-properties:
-  compatible:
-    const: fsl,t1023-sfp
-
-  reg:
-    maxItems: 1
-
-required:
-  - compatible
-  - reg
-
-unevaluatedProperties: false
-
-examples:
-  - |
-    efuse@e8000 {
-        compatible = "fsl,t1023-sfp";
-        reg = <0xe8000 0x1000>;
-    };
-...
Signature · 1 line
2.35.1.1320.gc452695387.dirty
#6 lore raw
Quoted 8 lines · Sean Anderson
Quoted 4 lines · Conor Dooley

Additionally, should
they fall back to t1023-sfp? I see that there's already some dts files with these compatibles in them but seemingly no driver support as there is for the t1023-sfp.

I checked the reference manuals for these processors, and all of them use TA 2.0.

Sounds like a fallback is suitable then, although that will require updating the various dts files.

#7 lore raw
Quoted 8 lines · Conor Dooley
Quoted 7 lines · Sean Anderson
Quoted 4 lines · Conor Dooley

Additionally, should
they fall back to t1023-sfp? I see that there's already some dts files with these compatibles in them but seemingly no driver support as there is for the t1023-sfp.

Quoted 4 lines
Quoted 1 line

I checked the reference manuals for these processors, and all of them use TA 2.0.

Sounds like a fallback is suitable then, although that will require updating the various dts files.

Yes, a fallback (like what is done for the T-series) would be suitable, but given that these devicetrees have been in-tree for eight years I think it would be preferable to support the existing bindings for compatibility purposes.

--Sean

#8 lore raw
Quoted 11 lines · Sean Anderson
Quoted 10 lines · Conor Dooley
Quoted 8 lines · Sean Anderson
Quoted 4 lines · Conor Dooley

Additionally, should
they fall back to t1023-sfp? I see that there's already some dts files with these compatibles in them but seemingly no driver support as there is for the t1023-sfp.

I checked the reference manuals for these processors, and all of them use TA 2.0.

Quoted 7 lines
Quoted 2 lines

Sounds like a fallback is suitable then, although that will require updating the various dts files.

Yes, a fallback (like what is done for the T-series) would be suitable, but given that these devicetrees have been in-tree for eight years I think it would be preferable to support the existing bindings for compatibility purposes.

Just cos stuff snuck into the tree in dts files doesn't make it right though, I'd rather the bindings were done correctly. I don't care if you want to support all of the compatibles in the driver so that it works with the existing devicetrees though, as long as you mention the rationale in the commit message.

#9 lore raw
Quoted 20 lines · Conor Dooley
Quoted 11 lines · Sean Anderson
Quoted 10 lines · Conor Dooley
Quoted 8 lines · Sean Anderson
Quoted 4 lines · Conor Dooley

Additionally, should
they fall back to t1023-sfp? I see that there's already some dts files with these compatibles in them but seemingly no driver support as there is for the t1023-sfp.

I checked the reference manuals for these processors, and all of them use TA 2.0.

Quoted 7 lines
Quoted 2 lines

Sounds like a fallback is suitable then, although that will require updating the various dts files.

Yes, a fallback (like what is done for the T-series) would be suitable, but given that these devicetrees have been in-tree for eight years I think it would be preferable to support the existing bindings for compatibility purposes.

Quoted 5 lines

Just cos stuff snuck into the tree in dts files doesn't make it right though, I'd rather the bindings were done correctly. I don't care if you want to support all of the compatibles in the driver so that it works with the existing devicetrees though, as long as you mention the rationale in the commit message.

It doesn't really matter what the schema has as long as the driver supports existing device trees.

--Sean

#10 lore raw
Quoted 21 lines · Sean Anderson
Quoted 20 lines · Conor Dooley
Quoted 18 lines · Sean Anderson
Quoted 8 lines · Conor Dooley
Quoted 7 lines · Sean Anderson
Quoted 4 lines · Conor Dooley

Additionally, should
they fall back to t1023-sfp? I see that there's already some dts files with these compatibles in them but seemingly no driver support as there is for the t1023-sfp.

Quoted 4 lines
Quoted 1 line

I checked the reference manuals for these processors, and all of them use TA 2.0.

Sounds like a fallback is suitable then, although that will require updating the various dts files.

Yes, a fallback (like what is done for the T-series) would be suitable, but given that these devicetrees have been in-tree for eight years I think it would be preferable to support the existing bindings for compatibility purposes.

Quoted 8 lines
Quoted 5 lines

Just cos stuff snuck into the tree in dts files doesn't make it right though, I'd rather the bindings were done correctly. I don't care if you want to support all of the compatibles in the driver so that it works with the existing devicetrees though, as long as you mention the rationale in the commit message.

It doesn't really matter what the schema has as long as the driver supports existing device trees.

We do not talk about driver now but bindings. You add new compatibles on a basis that they were already used. This cannot bypass regular review comments, so if during regular review process we would require fallbacks, then you are expected to listen to review also when documenting existing compatibles. Otherwise everyone would prefer to snuck in incorrect code and later document it "it was there!".

Best regards,
Krzysztof

#11 lore raw
Quoted 31 lines · Krzysztof Kozlowski
Quoted 21 lines · Sean Anderson
Quoted 20 lines · Conor Dooley
Quoted 18 lines · Sean Anderson
Quoted 8 lines · Conor Dooley
Quoted 7 lines · Sean Anderson
Quoted 4 lines · Conor Dooley

Additionally, should
they fall back to t1023-sfp? I see that there's already some dts files with these compatibles in them but seemingly no driver support as there is for the t1023-sfp.

Quoted 4 lines
Quoted 1 line

I checked the reference manuals for these processors, and all of them use TA 2.0.

Sounds like a fallback is suitable then, although that will require updating the various dts files.

Yes, a fallback (like what is done for the T-series) would be suitable, but given that these devicetrees have been in-tree for eight years I think it would be preferable to support the existing bindings for compatibility purposes.

Quoted 8 lines
Quoted 5 lines

Just cos stuff snuck into the tree in dts files doesn't make it right though, I'd rather the bindings were done correctly. I don't care if you want to support all of the compatibles in the driver so that it works with the existing devicetrees though, as long as you mention the rationale in the commit message.

It doesn't really matter what the schema has as long as the driver supports existing device trees.

Quoted 6 lines

We do not talk about driver now but bindings. You add new compatibles on a basis that they were already used. This cannot bypass regular review comments, so if during regular review process we would require fallbacks, then you are expected to listen to review also when documenting existing compatibles. Otherwise everyone would prefer to snuck in incorrect code and later document it "it was there!".

To be clear, the existing nodes look like

	sfp: sfp@e8000 {
		compatible = "fsl,t1040-sfp";
		reg	   = <0xe8000 0x1000>;
	};

which is perfectly serviceable for read-only use (as the clock is only necessary for writing). As these devices are effectively identical, the compatible could also look like what the P-series has:

	sfp: sfp@e8000 {
		compatible = "fsl,p2041-sfp", "fsl,qoriq-sfp-1.0";
		reg	   = <0xe8000 0x1000>;
	};

but in either case, it is desirable for the driver to match based on the more-specific compatible (as well as the less-specific compatible) as we already have enough information from the more-specific compatible to select the correct implementation.

--Sean

#12 lore raw
Quoted 41 lines · Sean Anderson
Quoted 31 lines · Krzysztof Kozlowski
Quoted 21 lines · Sean Anderson
Quoted 20 lines · Conor Dooley
Quoted 18 lines · Sean Anderson
Quoted 8 lines · Conor Dooley
Quoted 7 lines · Sean Anderson
Quoted 4 lines · Conor Dooley

Additionally, should
they fall back to t1023-sfp? I see that there's already some dts files with these compatibles in them but seemingly no driver support as there is for the t1023-sfp.

Quoted 4 lines
Quoted 1 line

I checked the reference manuals for these processors, and all of them use TA 2.0.

Sounds like a fallback is suitable then, although that will require updating the various dts files.

Yes, a fallback (like what is done for the T-series) would be suitable, but given that these devicetrees have been in-tree for eight years I think it would be preferable to support the existing bindings for compatibility purposes.

Quoted 8 lines
Quoted 5 lines

Just cos stuff snuck into the tree in dts files doesn't make it right though, I'd rather the bindings were done correctly. I don't care if you want to support all of the compatibles in the driver so that it works with the existing devicetrees though, as long as you mention the rationale in the commit message.

It doesn't really matter what the schema has as long as the driver supports existing device trees.

Quoted 6 lines

We do not talk about driver now but bindings. You add new compatibles on a basis that they were already used. This cannot bypass regular review comments, so if during regular review process we would require fallbacks, then you are expected to listen to review also when documenting existing compatibles. Otherwise everyone would prefer to snuck in incorrect code and later document it "it was there!".

To be clear, the existing nodes look like

Quoted 8 lines
	sfp: sfp@e8000 {
		compatible = "fsl,t1040-sfp";
		reg	   = <0xe8000 0x1000>;
	};

which is perfectly serviceable for read-only use (as the clock is only necessary for writing). As these devices are effectively identical, the compatible could also look like what the P-series has:

I'd rather you just picked the oldest device and used that as the fallback, rather than introducing an "<blah>-sfp-2.0" compatible. The 2.1 and 3.0 ones don't have anything like that.

Quoted 4 lines
	sfp: sfp@e8000 {
		compatible = "fsl,p2041-sfp", "fsl,qoriq-sfp-1.0";
		reg	   = <0xe8000 0x1000>;
	};

None of the seem to be documented either btw:
rg fsl,qoriq-sfp-1.0
arch/powerpc/boot/dts/fsl/p5020si-post.dtsi
371: compatible = "fsl,p5020-sfp", "fsl,qoriq-sfp-1.0";

arch/powerpc/boot/dts/fsl/p2041si-post.dtsi
339: compatible = "fsl,p2041-sfp", "fsl,qoriq-sfp-1.0";

arch/powerpc/boot/dts/fsl/p4080si-post.dtsi
386: compatible = "fsl,p4080-sfp", "fsl,qoriq-sfp-1.0";

arch/powerpc/boot/dts/fsl/p3041si-post.dtsi
366: compatible = "fsl,p3041-sfp", "fsl,qoriq-sfp-1.0";

arch/powerpc/boot/dts/fsl/p5040si-post.dtsi
331: compatible = "fsl,p5040-sfp", "fsl,qoriq-sfp-1.0";

Quoted 7 lines

but in either case, it is desirable for the driver to match based on the more-specific compatible (as well as the less-specific compatible) as we already have enough information from the more-specific compatible to select the correct implementation.

--Sean

#13 lore raw
Quoted 47 lines · Conor Dooley
Quoted 46 lines · Sean Anderson
Quoted 31 lines · Krzysztof Kozlowski
Quoted 21 lines · Sean Anderson
Quoted 20 lines · Conor Dooley
Quoted 18 lines · Sean Anderson
Quoted 8 lines · Conor Dooley
Quoted 7 lines · Sean Anderson
Quoted 4 lines · Conor Dooley

Additionally, should
they fall back to t1023-sfp? I see that there's already some dts files with these compatibles in them but seemingly no driver support as there is for the t1023-sfp.

Quoted 4 lines
Quoted 1 line

I checked the reference manuals for these processors, and all of them use TA 2.0.

Sounds like a fallback is suitable then, although that will require updating the various dts files.

Yes, a fallback (like what is done for the T-series) would be suitable, but given that these devicetrees have been in-tree for eight years I think it would be preferable to support the existing bindings for compatibility purposes.

Quoted 8 lines
Quoted 5 lines

Just cos stuff snuck into the tree in dts files doesn't make it right though, I'd rather the bindings were done correctly. I don't care if you want to support all of the compatibles in the driver so that it works with the existing devicetrees though, as long as you mention the rationale in the commit message.

It doesn't really matter what the schema has as long as the driver supports existing device trees.

Quoted 6 lines

We do not talk about driver now but bindings. You add new compatibles on a basis that they were already used. This cannot bypass regular review comments, so if during regular review process we would require fallbacks, then you are expected to listen to review also when documenting existing compatibles. Otherwise everyone would prefer to snuck in incorrect code and later document it "it was there!".

To be clear, the existing nodes look like

	sfp: sfp@e8000 {
		compatible = "fsl,t1040-sfp";
		reg	   = <0xe8000 0x1000>;
	};
Quoted 7 lines
Quoted 3 lines

which is perfectly serviceable for read-only use (as the clock is only necessary for writing). As these devices are effectively identical, the compatible could also look like what the P-series has:

I'd rather you just picked the oldest device and used that as the fallback, rather than introducing an "<blah>-sfp-2.0" compatible. The 2.1 and 3.0 ones don't have anything like that.

Because of course no one looked around at the other SoCs and noticed that they all have the same TA version and use the same document for this device. I would rather use a common name based on the version, rather than arbitrarily picking a SoC to be "compatible" with. This doesn't make sense to me, since each SoC's reference manual just calls out to the appropriate TA manual for all register definitions.

Quoted 13 lines
Quoted 4 lines
	sfp: sfp@e8000 {
		compatible = "fsl,p2041-sfp", "fsl,qoriq-sfp-1.0";
		reg	   = <0xe8000 0x1000>;
	};

None of the seem to be documented either btw:
rg fsl,qoriq-sfp-1.0
arch/powerpc/boot/dts/fsl/p5020si-post.dtsi
371: compatible = "fsl,p5020-sfp", "fsl,qoriq-sfp-1.0";

arch/powerpc/boot/dts/fsl/p2041si-post.dtsi
339: compatible = "fsl,p2041-sfp", "fsl,qoriq-sfp-1.0";

Quoted 8 lines

arch/powerpc/boot/dts/fsl/p4080si-post.dtsi
386: compatible = "fsl,p4080-sfp", "fsl,qoriq-sfp-1.0";

arch/powerpc/boot/dts/fsl/p3041si-post.dtsi
366: compatible = "fsl,p3041-sfp", "fsl,qoriq-sfp-1.0";

arch/powerpc/boot/dts/fsl/p5040si-post.dtsi
331: compatible = "fsl,p5040-sfp", "fsl,qoriq-sfp-1.0";

Yeah. As noted in the next patch, someone with access to the TA 1.0 manual would need to have a look.

--Sean

Quoted 7 lines
Quoted 7 lines

but in either case, it is desirable for the driver to match based on the more-specific compatible (as well as the less-specific compatible) as we already have enough information from the more-specific compatible to select the correct implementation.

--Sean

maintainer Reviewed-by on v1 1/2
#14 lore raw
In reply to Sean Anderson
Quoted 4 lines · Sean Anderson

These bindings document the same hardware (just different minor revisions). Remove the newer fsl,t1023-sfp.

Fixes aa1ed6047107 ("dt-bindings: nvmem: Add t1023-sfp efuse support")

I don't think Fixes is appropriate here. What was broken?

Quoted 7 lines

---

 .../bindings/nvmem/fsl,layerscape-sfp.yaml    | 21 ++++++++++-
 .../bindings/nvmem/fsl,t1023-sfp.yaml         | 37 -------------------
 2 files changed, 19 insertions(+), 39 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/nvmem/fsl,t1023-sfp.yaml

Otherwise,

#15 lore raw
Quoted 7 lines · Rob Herring
Quoted 4 lines · Sean Anderson

These bindings document the same hardware (just different minor revisions). Remove the newer fsl,t1023-sfp.

Fixes aa1ed6047107 ("dt-bindings: nvmem: Add t1023-sfp efuse support")

I don't think Fixes is appropriate here. What was broken?

I also don't think Fixes is appropriate. Apart from that: