Re: [PATCH v2] arm64: dts: ti: k3-am62p: use eFuse MAC Address for CPSW3G Port 1

[Krzysztof Kozlowski]

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@xxxxxx>
>>> ---
>>>
>>> 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@xxxxxx/
>>> 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@xxxxxx/
>>> 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@xxxxxx/
> 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@xxxxxx/
>>>   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@xxxxxxxxxx/
> "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