| Message ID | 20240404081845.622707-1-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm64: dts: ti: k3-am62p: use eFuse MAC Address for CPSW3G Port 1 | expand |
On 04/04/2024 10:18, Siddharth Vadapalli wrote: > Add the "cpsw-mac-efuse" node within "wkup_conf" node corresponding to the > CTRLMMR_MAC_IDx registers within the CTRL_MMR space. Assign the compatible > "ti,am62p-cpsw-mac-efuse" to enable "syscon_regmap" operations on these > registers. The MAC Address programmed in the eFuse is accessible through > the CTRLMMR_MAC_IDx registers. The "ti,syscon-efuse" device-tree property > points to the CTRLMMR_MAC_IDx registers, allowing the CPSW driver to fetch > the MAC Address and assign it to the network interface associated with > CPSW3G MAC Port 1. > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > > This patch is based on linux-next tagged next-20240404. > Patch depends on: > https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402105708.4114146-1-s-vadapalli@ti.com/ > for the newly added "ti,am62p-cpsw-mac-efuse" compatible. > > v1: > https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402094200.4036076-1-s-vadapalli@ti.com/ > Changes since v1: > - Since "wkup_conf" is modelled as a "simple-bus" rather than being And maybe the hardware representation is not correct? What bus is it? > modelled as a System Controller node with the "syscon" compatible, > directly passing the reference to the "wkup_conf" node using the > "ti,syscon-efuse" device-tree property will not work. > Therefore, I posted the patch at: > https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402105708.4114146-1-s-vadapalli@ti.com/ > in order to add a new compatible to be used for modelling the > CTRLMMR_MAC_IDx registers as System Controller nodes, thereby > allowing the existing "ti,syscon-efuse" property to be used. > Now, "ti,syscon-efuse" points to the "cpsw_mac_efuse" node within > "wkup_conf" node, with "cpsw_mac_efuse" being a "syscon" node. > > Logs verifying that the CPSW driver assigns the MAC Address from the > eFuse based on the CTRLMMR_MAC_IDx registers at 0x43000200 and 0x43000204 > to the interface eth0 corresponding to CPSW3G MAC Port 1: > https://gist.github.com/Siddharth-Vadapalli-at-TI/9982c6f13bf9b8cfaf97e8517e7dea13 > > Regards, > Siddharth. > > arch/arm64/boot/dts/ti/k3-am62p-main.dtsi | 1 + > arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi | 5 +++++ > 2 files changed, 6 insertions(+) > > diff --git a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi > index 7337a9e13535..848ca454a411 100644 > --- a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi > +++ b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi > @@ -696,6 +696,7 @@ cpsw_port1: port@1 { > label = "port1"; > phys = <&phy_gmii_sel 1>; > mac-address = [00 00 00 00 00 00]; > + ti,syscon-efuse = <&cpsw_mac_efuse 0x0>; Why this is not nvmem cell, like or efuses? > }; > > cpsw_port2: port@2 { > diff --git a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi > index a84756c336d0..df9d40f64e3b 100644 > --- a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi > +++ b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi > @@ -18,6 +18,11 @@ chipid: chipid@14 { > reg = <0x14 0x4>; > bootph-all; > }; > + > + cpsw_mac_efuse: cpsw-mac-efuse@200 { Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + compatible = "ti,am62p-cpsw-mac-efuse", "syscon"; > + reg = <0x200 0x8>; > + }; > }; > > wkup_uart0: serial@2b300000 { Best regards, Krzysztof
On Thu, Apr 04, 2024 at 10:43:04AM +0200, Krzysztof Kozlowski wrote: > On 04/04/2024 10:18, Siddharth Vadapalli wrote: > > Add the "cpsw-mac-efuse" node within "wkup_conf" node corresponding to the > > CTRLMMR_MAC_IDx registers within the CTRL_MMR space. Assign the compatible > > "ti,am62p-cpsw-mac-efuse" to enable "syscon_regmap" operations on these > > registers. The MAC Address programmed in the eFuse is accessible through > > the CTRLMMR_MAC_IDx registers. The "ti,syscon-efuse" device-tree property > > points to the CTRLMMR_MAC_IDx registers, allowing the CPSW driver to fetch > > the MAC Address and assign it to the network interface associated with > > CPSW3G MAC Port 1. > > > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > --- > > > > This patch is based on linux-next tagged next-20240404. > > Patch depends on: > > https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402105708.4114146-1-s-vadapalli@ti.com/ > > for the newly added "ti,am62p-cpsw-mac-efuse" compatible. > > > > v1: > > https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402094200.4036076-1-s-vadapalli@ti.com/ > > Changes since v1: > > - Since "wkup_conf" is modelled as a "simple-bus" rather than being > > And maybe the hardware representation is not correct? What bus is it? I will let Andrew comment on it. Andrew had posted a patch at: https://lore.kernel.org/r/20240124184722.150615-10-afd@ti.com/ to convert an equivalent "main_conf" node for AM62 SoC to "simple-bus" from the existing "syscon". > > > modelled as a System Controller node with the "syscon" compatible, > > directly passing the reference to the "wkup_conf" node using the > > "ti,syscon-efuse" device-tree property will not work. > > Therefore, I posted the patch at: > > https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402105708.4114146-1-s-vadapalli@ti.com/ > > in order to add a new compatible to be used for modelling the > > CTRLMMR_MAC_IDx registers as System Controller nodes, thereby > > allowing the existing "ti,syscon-efuse" property to be used. > > Now, "ti,syscon-efuse" points to the "cpsw_mac_efuse" node within > > "wkup_conf" node, with "cpsw_mac_efuse" being a "syscon" node. > > > > Logs verifying that the CPSW driver assigns the MAC Address from the > > eFuse based on the CTRLMMR_MAC_IDx registers at 0x43000200 and 0x43000204 > > to the interface eth0 corresponding to CPSW3G MAC Port 1: > > https://gist.github.com/Siddharth-Vadapalli-at-TI/9982c6f13bf9b8cfaf97e8517e7dea13 > > > > Regards, > > Siddharth. > > > > arch/arm64/boot/dts/ti/k3-am62p-main.dtsi | 1 + > > arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi | 5 +++++ > > 2 files changed, 6 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi > > index 7337a9e13535..848ca454a411 100644 > > --- a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi > > +++ b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi > > @@ -696,6 +696,7 @@ cpsw_port1: port@1 { > > label = "port1"; > > phys = <&phy_gmii_sel 1>; > > mac-address = [00 00 00 00 00 00]; > > + ti,syscon-efuse = <&cpsw_mac_efuse 0x0>; > > Why this is not nvmem cell, like or efuses? Since it belongs to the MMIO register set. You had recommended *not* using nvmem for such MMIO registers at: https://lore.kernel.org/r/48902771-5d3b-448a-8a74-ac18fb4f1a86@linaro.org/ "nvmem is for non-volatile memory, like OCOTP and eFUSE. This is not for accessing regular MMIO registers of system-controller..." Despite the "ti,syscon-efuse" property containing the term "efuse" in its name, it is reading the CTRLMMR_MAC_IDx MMIO registers. So I assumed that the existing approach which has been used on all K3 SoCs apart from this one, will be suitable for this SoC as well. > > > }; > > > > cpsw_port2: port@2 { > > diff --git a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi > > index a84756c336d0..df9d40f64e3b 100644 > > --- a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi > > +++ b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi > > @@ -18,6 +18,11 @@ chipid: chipid@14 { > > reg = <0x14 0x4>; > > bootph-all; > > }; > > + > > + cpsw_mac_efuse: cpsw-mac-efuse@200 { > > Node names should be generic. See also an explanation and list of > examples (not exhaustive) in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation I was following the convention that other mfd-syscon compatible nodes seemed to be using: https://github.com/torvalds/linux/blob/41bccc98fb7931d63d03f326a746ac4d429c1dd3/arch/arm64/boot/dts/ti/k3-am65-main.dtsi#L502 The node is: dss_oldi_io_ctrl: dss-oldi-io-ctrl@41e0 corresponding to the compatible: "ti,am654-dss-oldi-io-ctrl" which was added by commit: https://github.com/torvalds/linux/commit/cb523495ee2a5938fbdd30b8a35094d386c55c12 Regards, Siddharth.
On 04/04/2024 11:12, Siddharth Vadapalli wrote: > On Thu, Apr 04, 2024 at 10:43:04AM +0200, Krzysztof Kozlowski wrote: >> On 04/04/2024 10:18, Siddharth Vadapalli wrote: >>> Add the "cpsw-mac-efuse" node within "wkup_conf" node corresponding to the >>> CTRLMMR_MAC_IDx registers within the CTRL_MMR space. Assign the compatible >>> "ti,am62p-cpsw-mac-efuse" to enable "syscon_regmap" operations on these >>> registers. The MAC Address programmed in the eFuse is accessible through >>> the CTRLMMR_MAC_IDx registers. The "ti,syscon-efuse" device-tree property >>> points to the CTRLMMR_MAC_IDx registers, allowing the CPSW driver to fetch >>> the MAC Address and assign it to the network interface associated with >>> CPSW3G MAC Port 1. >>> >>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>> --- >>> >>> This patch is based on linux-next tagged next-20240404. >>> Patch depends on: >>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402105708.4114146-1-s-vadapalli@ti.com/ >>> for the newly added "ti,am62p-cpsw-mac-efuse" compatible. >>> >>> v1: >>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402094200.4036076-1-s-vadapalli@ti.com/ >>> Changes since v1: >>> - Since "wkup_conf" is modelled as a "simple-bus" rather than being >> >> And maybe the hardware representation is not correct? What bus is it? > > I will let Andrew comment on it. Andrew had posted a patch at: > https://lore.kernel.org/r/20240124184722.150615-10-afd@ti.com/ > to convert an equivalent "main_conf" node for AM62 SoC to "simple-bus" > from the existing "syscon". > >> >>> modelled as a System Controller node with the "syscon" compatible, >>> directly passing the reference to the "wkup_conf" node using the >>> "ti,syscon-efuse" device-tree property will not work. >>> Therefore, I posted the patch at: >>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402105708.4114146-1-s-vadapalli@ti.com/ >>> in order to add a new compatible to be used for modelling the >>> CTRLMMR_MAC_IDx registers as System Controller nodes, thereby >>> allowing the existing "ti,syscon-efuse" property to be used. >>> Now, "ti,syscon-efuse" points to the "cpsw_mac_efuse" node within >>> "wkup_conf" node, with "cpsw_mac_efuse" being a "syscon" node. >>> >>> Logs verifying that the CPSW driver assigns the MAC Address from the >>> eFuse based on the CTRLMMR_MAC_IDx registers at 0x43000200 and 0x43000204 >>> to the interface eth0 corresponding to CPSW3G MAC Port 1: >>> https://gist.github.com/Siddharth-Vadapalli-at-TI/9982c6f13bf9b8cfaf97e8517e7dea13 >>> >>> Regards, >>> Siddharth. >>> >>> arch/arm64/boot/dts/ti/k3-am62p-main.dtsi | 1 + >>> arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi | 5 +++++ >>> 2 files changed, 6 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi >>> index 7337a9e13535..848ca454a411 100644 >>> --- a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi >>> +++ b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi >>> @@ -696,6 +696,7 @@ cpsw_port1: port@1 { >>> label = "port1"; >>> phys = <&phy_gmii_sel 1>; >>> mac-address = [00 00 00 00 00 00]; >>> + ti,syscon-efuse = <&cpsw_mac_efuse 0x0>; >> >> Why this is not nvmem cell, like or efuses? > > Since it belongs to the MMIO register set. You had recommended *not* > using nvmem for such MMIO registers at: > https://lore.kernel.org/r/48902771-5d3b-448a-8a74-ac18fb4f1a86@linaro.org/ > "nvmem is for non-volatile memory, like OCOTP and eFUSE. This is not for > accessing regular MMIO registers of system-controller..." > > Despite the "ti,syscon-efuse" property containing the term "efuse" in its > name, it is reading the CTRLMMR_MAC_IDx MMIO registers. So I assumed that > the existing approach which has been used on all K3 SoCs apart from this > one, will be suitable for this SoC as well. OK, I totally forgot we discussed this. > >> >>> }; >>> >>> cpsw_port2: port@2 { >>> diff --git a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi >>> index a84756c336d0..df9d40f64e3b 100644 >>> --- a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi >>> +++ b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi >>> @@ -18,6 +18,11 @@ chipid: chipid@14 { >>> reg = <0x14 0x4>; >>> bootph-all; >>> }; >>> + >>> + cpsw_mac_efuse: cpsw-mac-efuse@200 { >> >> Node names should be generic. See also an explanation and list of >> examples (not exhaustive) in DT specification: >> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > I was following the convention that other mfd-syscon compatible nodes > seemed to be using: > https://github.com/torvalds/linux/blob/41bccc98fb7931d63d03f326a746ac4d429c1dd3/arch/arm64/boot/dts/ti/k3-am65-main.dtsi#L502 > The node is: > dss_oldi_io_ctrl: dss-oldi-io-ctrl@41e0 > corresponding to the compatible: > "ti,am654-dss-oldi-io-ctrl" > which was added by commit: > https://github.com/torvalds/linux/commit/cb523495ee2a5938fbdd30b8a35094d386c55c12 So if that one was wrong, then what? I don't know really what type of device is it, but just because one contributor called it that way, does not mean you should keep going. Maybe investigate why that contributor did not decide to follow Devicetree spec recommendation? Best regards, Krzysztof
On Thu, Apr 04, 2024 at 12:00:09PM +0200, Krzysztof Kozlowski wrote: > On 04/04/2024 11:12, Siddharth Vadapalli wrote: > > On Thu, Apr 04, 2024 at 10:43:04AM +0200, Krzysztof Kozlowski wrote: > >> On 04/04/2024 10:18, Siddharth Vadapalli wrote: > >>> Add the "cpsw-mac-efuse" node within "wkup_conf" node corresponding to the > >>> CTRLMMR_MAC_IDx registers within the CTRL_MMR space. Assign the compatible > >>> "ti,am62p-cpsw-mac-efuse" to enable "syscon_regmap" operations on these > >>> registers. The MAC Address programmed in the eFuse is accessible through > >>> the CTRLMMR_MAC_IDx registers. The "ti,syscon-efuse" device-tree property > >>> points to the CTRLMMR_MAC_IDx registers, allowing the CPSW driver to fetch > >>> the MAC Address and assign it to the network interface associated with > >>> CPSW3G MAC Port 1. > >>> > >>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > >>> --- > >>> ... > > > > >> > >>> }; > >>> > >>> cpsw_port2: port@2 { > >>> diff --git a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi > >>> index a84756c336d0..df9d40f64e3b 100644 > >>> --- a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi > >>> +++ b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi > >>> @@ -18,6 +18,11 @@ chipid: chipid@14 { > >>> reg = <0x14 0x4>; > >>> bootph-all; > >>> }; > >>> + > >>> + cpsw_mac_efuse: cpsw-mac-efuse@200 { > >> > >> Node names should be generic. See also an explanation and list of > >> examples (not exhaustive) in DT specification: > >> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > > > I was following the convention that other mfd-syscon compatible nodes > > seemed to be using: > > https://github.com/torvalds/linux/blob/41bccc98fb7931d63d03f326a746ac4d429c1dd3/arch/arm64/boot/dts/ti/k3-am65-main.dtsi#L502 > > The node is: > > dss_oldi_io_ctrl: dss-oldi-io-ctrl@41e0 > > corresponding to the compatible: > > "ti,am654-dss-oldi-io-ctrl" > > which was added by commit: > > https://github.com/torvalds/linux/commit/cb523495ee2a5938fbdd30b8a35094d386c55c12 > > So if that one was wrong, then what? I don't know really what type of > device is it, but just because one contributor called it that way, does > not mean you should keep going. Maybe investigate why that contributor > did not decide to follow Devicetree spec recommendation? Yes, it doesn't justify the convention. I seem to have picked a wrong example when figuring out the convention for naming the node. I plan to name it as: ethernet-mac-efuse while retaining the label "cpsw_mac_efuse" since CPSW is the name of the Ethernet Switch on the SoC. Please let me know if it is acceptable. I will post the v3 patch based on your feedback. Regards, Siddharth.
On 04/04/2024 12:16, Siddharth Vadapalli wrote: >>> I was following the convention that other mfd-syscon compatible nodes >>> seemed to be using: >>> https://github.com/torvalds/linux/blob/41bccc98fb7931d63d03f326a746ac4d429c1dd3/arch/arm64/boot/dts/ti/k3-am65-main.dtsi#L502 >>> The node is: >>> dss_oldi_io_ctrl: dss-oldi-io-ctrl@41e0 >>> corresponding to the compatible: >>> "ti,am654-dss-oldi-io-ctrl" >>> which was added by commit: >>> https://github.com/torvalds/linux/commit/cb523495ee2a5938fbdd30b8a35094d386c55c12 >> >> So if that one was wrong, then what? I don't know really what type of >> device is it, but just because one contributor called it that way, does >> not mean you should keep going. Maybe investigate why that contributor >> did not decide to follow Devicetree spec recommendation? > > Yes, it doesn't justify the convention. I seem to have picked a wrong > example when figuring out the convention for naming the node. I plan to > name it as: > ethernet-mac-efuse > while retaining the label "cpsw_mac_efuse" since CPSW is the name of the > Ethernet Switch on the SoC. Please let me know if it is acceptable. I > will post the v3 patch based on your feedback. Label is fine, there is no restriction/guideline on labels, so choose descriptive or something useful for you. Just the node name. If this is syscon, then usually system-controller. If this is efuse, then maybe efuse, even though previously I was looking at this more as a syscon. Best regards, Krzysztof
On Thu, Apr 04, 2024 at 12:31:47PM +0200, Krzysztof Kozlowski wrote: > On 04/04/2024 12:16, Siddharth Vadapalli wrote: > >>> I was following the convention that other mfd-syscon compatible nodes > >>> seemed to be using: > >>> https://github.com/torvalds/linux/blob/41bccc98fb7931d63d03f326a746ac4d429c1dd3/arch/arm64/boot/dts/ti/k3-am65-main.dtsi#L502 > >>> The node is: > >>> dss_oldi_io_ctrl: dss-oldi-io-ctrl@41e0 > >>> corresponding to the compatible: > >>> "ti,am654-dss-oldi-io-ctrl" > >>> which was added by commit: > >>> https://github.com/torvalds/linux/commit/cb523495ee2a5938fbdd30b8a35094d386c55c12 > >> > >> So if that one was wrong, then what? I don't know really what type of > >> device is it, but just because one contributor called it that way, does > >> not mean you should keep going. Maybe investigate why that contributor > >> did not decide to follow Devicetree spec recommendation? > > > > Yes, it doesn't justify the convention. I seem to have picked a wrong > > example when figuring out the convention for naming the node. I plan to > > name it as: > > ethernet-mac-efuse > > while retaining the label "cpsw_mac_efuse" since CPSW is the name of the > > Ethernet Switch on the SoC. Please let me know if it is acceptable. I > > will post the v3 patch based on your feedback. > > Label is fine, there is no restriction/guideline on labels, so choose > descriptive or something useful for you. Just the node name. If this is > syscon, then usually system-controller. If this is efuse, then maybe > efuse, even though previously I was looking at this more as a syscon. I will change it to "ethernet-mac-syscon" to indicate the MMIO nature of the node. eFuse might give the wrong impression, despite the fact that the contents of the register are based on the contents of an eFuse. I will post the v3 patch with the following changes: 1. Rename "cpsw-mac-efuse" as "ethernet-mac-syscon" 2. Rename "cpsw_mac_efuse" as "cpsw_mac_syscon" Regards, Siddharth.
On 4/4/24 5:00 AM, Krzysztof Kozlowski wrote: > On 04/04/2024 11:12, Siddharth Vadapalli wrote: >> On Thu, Apr 04, 2024 at 10:43:04AM +0200, Krzysztof Kozlowski wrote: >>> On 04/04/2024 10:18, Siddharth Vadapalli wrote: >>>> Add the "cpsw-mac-efuse" node within "wkup_conf" node corresponding to the >>>> CTRLMMR_MAC_IDx registers within the CTRL_MMR space. Assign the compatible >>>> "ti,am62p-cpsw-mac-efuse" to enable "syscon_regmap" operations on these >>>> registers. The MAC Address programmed in the eFuse is accessible through >>>> the CTRLMMR_MAC_IDx registers. The "ti,syscon-efuse" device-tree property >>>> points to the CTRLMMR_MAC_IDx registers, allowing the CPSW driver to fetch >>>> the MAC Address and assign it to the network interface associated with >>>> CPSW3G MAC Port 1. >>>> >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>> --- >>>> >>>> This patch is based on linux-next tagged next-20240404. >>>> Patch depends on: >>>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402105708.4114146-1-s-vadapalli@ti.com/ >>>> for the newly added "ti,am62p-cpsw-mac-efuse" compatible. >>>> >>>> v1: >>>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402094200.4036076-1-s-vadapalli@ti.com/ >>>> Changes since v1: >>>> - Since "wkup_conf" is modelled as a "simple-bus" rather than being >>> >>> And maybe the hardware representation is not correct? What bus is it? >> >> I will let Andrew comment on it. Andrew had posted a patch at: >> https://lore.kernel.org/r/20240124184722.150615-10-afd@ti.com/ >> to convert an equivalent "main_conf" node for AM62 SoC to "simple-bus" >> from the existing "syscon". >> >>> >>>> modelled as a System Controller node with the "syscon" compatible, >>>> directly passing the reference to the "wkup_conf" node using the >>>> "ti,syscon-efuse" device-tree property will not work. >>>> Therefore, I posted the patch at: >>>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402105708.4114146-1-s-vadapalli@ti.com/ >>>> in order to add a new compatible to be used for modelling the >>>> CTRLMMR_MAC_IDx registers as System Controller nodes, thereby >>>> allowing the existing "ti,syscon-efuse" property to be used. >>>> Now, "ti,syscon-efuse" points to the "cpsw_mac_efuse" node within >>>> "wkup_conf" node, with "cpsw_mac_efuse" being a "syscon" node. >>>> >>>> Logs verifying that the CPSW driver assigns the MAC Address from the >>>> eFuse based on the CTRLMMR_MAC_IDx registers at 0x43000200 and 0x43000204 >>>> to the interface eth0 corresponding to CPSW3G MAC Port 1: >>>> https://gist.github.com/Siddharth-Vadapalli-at-TI/9982c6f13bf9b8cfaf97e8517e7dea13 >>>> >>>> Regards, >>>> Siddharth. >>>> >>>> arch/arm64/boot/dts/ti/k3-am62p-main.dtsi | 1 + >>>> arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi | 5 +++++ >>>> 2 files changed, 6 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi >>>> index 7337a9e13535..848ca454a411 100644 >>>> --- a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi >>>> +++ b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi >>>> @@ -696,6 +696,7 @@ cpsw_port1: port@1 { >>>> label = "port1"; >>>> phys = <&phy_gmii_sel 1>; >>>> mac-address = [00 00 00 00 00 00]; >>>> + ti,syscon-efuse = <&cpsw_mac_efuse 0x0>; >>> >>> Why this is not nvmem cell, like or efuses? >> >> Since it belongs to the MMIO register set. You had recommended *not* >> using nvmem for such MMIO registers at: >> https://lore.kernel.org/r/48902771-5d3b-448a-8a74-ac18fb4f1a86@linaro.org/ >> "nvmem is for non-volatile memory, like OCOTP and eFUSE. This is not for >> accessing regular MMIO registers of system-controller..." >> >> Despite the "ti,syscon-efuse" property containing the term "efuse" in its >> name, it is reading the CTRLMMR_MAC_IDx MMIO registers. So I assumed that >> the existing approach which has been used on all K3 SoCs apart from this >> one, will be suitable for this SoC as well. > > OK, I totally forgot we discussed this. > Discussed but never finalized, here is the last message[0] but with no response. You even asked above, "Why this is not nvmem cell", you should trust your instincts, this *should* be a NVMEM cell. That is how everyone else handles eFused MACs, no clue why you want us to use syscon?? We would have no way forward in removing all our DT check warnings with syscon. Syscon is a hacky dead-end filled with custom compatible strings like "ti,am62p-cpsw-mac-efuse" and custom properties like "ti,syscon-efuse". NVMEM is a standard, forcing us to use TI custom syscon properties will prevent our DT from working on anything other than Linux (unless we go manually add support for every TI custom property to every DT using SW, defeats the whole purpose DT). Andrew [0] https://lore.kernel.org/all/e7114cb4-e24f-4e78-a89f-4e2e2e704b8a@ti.com/ >> >>> >>>> }; >>>> >>>> cpsw_port2: port@2 { >>>> diff --git a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi >>>> index a84756c336d0..df9d40f64e3b 100644 >>>> --- a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi >>>> +++ b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi >>>> @@ -18,6 +18,11 @@ chipid: chipid@14 { >>>> reg = <0x14 0x4>; >>>> bootph-all; >>>> }; >>>> + >>>> + cpsw_mac_efuse: cpsw-mac-efuse@200 { >>> >>> Node names should be generic. See also an explanation and list of >>> examples (not exhaustive) in DT specification: >>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation >> >> I was following the convention that other mfd-syscon compatible nodes >> seemed to be using: >> https://github.com/torvalds/linux/blob/41bccc98fb7931d63d03f326a746ac4d429c1dd3/arch/arm64/boot/dts/ti/k3-am65-main.dtsi#L502 >> The node is: >> dss_oldi_io_ctrl: dss-oldi-io-ctrl@41e0 >> corresponding to the compatible: >> "ti,am654-dss-oldi-io-ctrl" >> which was added by commit: >> https://github.com/torvalds/linux/commit/cb523495ee2a5938fbdd30b8a35094d386c55c12 > > So if that one was wrong, then what? I don't know really what type of > device is it, but just because one contributor called it that way, does > not mean you should keep going. Maybe investigate why that contributor > did not decide to follow Devicetree spec recommendation? > > Best regards, > Krzysztof >
diff --git a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi index 7337a9e13535..848ca454a411 100644 --- a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi +++ b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi @@ -696,6 +696,7 @@ cpsw_port1: port@1 { label = "port1"; phys = <&phy_gmii_sel 1>; mac-address = [00 00 00 00 00 00]; + ti,syscon-efuse = <&cpsw_mac_efuse 0x0>; }; cpsw_port2: port@2 { diff --git a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi index a84756c336d0..df9d40f64e3b 100644 --- a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi +++ b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi @@ -18,6 +18,11 @@ chipid: chipid@14 { reg = <0x14 0x4>; bootph-all; }; + + cpsw_mac_efuse: cpsw-mac-efuse@200 { + compatible = "ti,am62p-cpsw-mac-efuse", "syscon"; + reg = <0x200 0x8>; + }; }; wkup_uart0: serial@2b300000 {
Add the "cpsw-mac-efuse" node within "wkup_conf" node corresponding to the CTRLMMR_MAC_IDx registers within the CTRL_MMR space. Assign the compatible "ti,am62p-cpsw-mac-efuse" to enable "syscon_regmap" operations on these registers. The MAC Address programmed in the eFuse is accessible through the CTRLMMR_MAC_IDx registers. The "ti,syscon-efuse" device-tree property points to the CTRLMMR_MAC_IDx registers, allowing the CPSW driver to fetch the MAC Address and assign it to the network interface associated with CPSW3G MAC Port 1. Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> --- This patch is based on linux-next tagged next-20240404. Patch depends on: https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402105708.4114146-1-s-vadapalli@ti.com/ for the newly added "ti,am62p-cpsw-mac-efuse" compatible. v1: https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402094200.4036076-1-s-vadapalli@ti.com/ Changes since v1: - Since "wkup_conf" is modelled as a "simple-bus" rather than being modelled as a System Controller node with the "syscon" compatible, directly passing the reference to the "wkup_conf" node using the "ti,syscon-efuse" device-tree property will not work. Therefore, I posted the patch at: https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402105708.4114146-1-s-vadapalli@ti.com/ in order to add a new compatible to be used for modelling the CTRLMMR_MAC_IDx registers as System Controller nodes, thereby allowing the existing "ti,syscon-efuse" property to be used. Now, "ti,syscon-efuse" points to the "cpsw_mac_efuse" node within "wkup_conf" node, with "cpsw_mac_efuse" being a "syscon" node. Logs verifying that the CPSW driver assigns the MAC Address from the eFuse based on the CTRLMMR_MAC_IDx registers at 0x43000200 and 0x43000204 to the interface eth0 corresponding to CPSW3G MAC Port 1: https://gist.github.com/Siddharth-Vadapalli-at-TI/9982c6f13bf9b8cfaf97e8517e7dea13 Regards, Siddharth. arch/arm64/boot/dts/ti/k3-am62p-main.dtsi | 1 + arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi | 5 +++++ 2 files changed, 6 insertions(+)