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

[Siddharth Vadapalli <s-vadapalli@ti.com>]

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.