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 <[email protected]>
---
This patch is based on linux-next tagged next-20240404.
Patch depends on:
https://patchwork.kernel.org/project/linux-arm-kernel/patch/[email protected]/
for the newly added "ti,am62p-cpsw-mac-efuse" compatible.
v1:
https://patchwork.kernel.org/project/linux-arm-kernel/patch/[email protected]/
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/[email protected]/
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>;
};
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 {
--
2.40.1
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 <[email protected]>
> ---
>
> This patch is based on linux-next tagged next-20240404.
> Patch depends on:
> https://patchwork.kernel.org/project/linux-arm-kernel/patch/[email protected]/
> for the newly added "ti,am62p-cpsw-mac-efuse" compatible.
>
> v1:
> https://patchwork.kernel.org/project/linux-arm-kernel/patch/[email protected]/
> 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/[email protected]/
> 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 <[email protected]>
> > ---
> >
> > This patch is based on linux-next tagged next-20240404.
> > Patch depends on:
> > https://patchwork.kernel.org/project/linux-arm-kernel/patch/[email protected]/
> > for the newly added "ti,am62p-cpsw-mac-efuse" compatible.
> >
> > v1:
> > https://patchwork.kernel.org/project/linux-arm-kernel/patch/[email protected]/
> > 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/[email protected]/
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/[email protected]/
> > 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/[email protected]/
"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 <[email protected]>
>>> ---
>>>
>>> This patch is based on linux-next tagged next-20240404.
>>> Patch depends on:
>>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/[email protected]/
>>> for the newly added "ti,am62p-cpsw-mac-efuse" compatible.
>>>
>>> v1:
>>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/[email protected]/
>>> 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/[email protected]/
> 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/[email protected]/
>>> 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/[email protected]/
> "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 <[email protected]>
> >>> ---
> >>>
..
>
> >
> >>
> >>> };
> >>>
> >>> 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 <[email protected]>
>>>> ---
>>>>
>>>> This patch is based on linux-next tagged next-20240404.
>>>> Patch depends on:
>>>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/[email protected]/
>>>> for the newly added "ti,am62p-cpsw-mac-efuse" compatible.
>>>>
>>>> v1:
>>>> https://patchwork.kernel.org/project/linux-arm-kernel/patch/[email protected]/
>>>> 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/[email protected]/
>> 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/[email protected]/
>>>> 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/[email protected]/
>> "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/[email protected]/
>>
>>>
>>>> };
>>>>
>>>> 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
>