Message-Id: <DAKC3DIYRP6K.1G9HTSVXDJOLB@kernel.org>
Date: Thu, 12 Jun 2025 08:17:31 +0200
From: "Michael Walle" <mwalle@...nel.org>
To: <Manikandan.M@...rochip.com>, <tudor.ambarus@...aro.org>,
 <robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
 <Nicolas.Ferre@...rochip.com>, <alexandre.belloni@...tlin.com>,
 <claudiu.beznea@...on.dev>, <pratyush@...nel.org>,
 <miquel.raynal@...tlin.com>, <richard@....at>, <vigneshr@...com>,
 <devicetree@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
 <linux-kernel@...r.kernel.org>, <linux-mtd@...ts.infradead.org>
Subject: Re: [PATCH v3 3/3] ARM: dts: microchip: sama5d27_wlsom1: Add
 nvmem-layout in QSPI for EUI48 MAC Address

Hi,

> >>>> Add nvmem-layout in QSPI to read the EUI48 Mac address by the
> >>>> net drivers using the nvmem property.The offset is set to 0x0
> >>>> since the factory programmed address is available in the
> >>>> resource managed space and the size determine if the requested
> >>>> address is of EUI48 (0x6) or EUI-64 (0x8) type.
> >>>> This is useful for cases where U-Boot is skipped and the Ethernet
> >>>> MAC address is needed to be configured by the kernel
> >>>>
> >>>> Signed-off-by: Manikandan Muralidharan <manikandan.m@...rochip.com>
> >>>> ---
> >>>>    .../boot/dts/microchip/at91-sama5d27_wlsom1.dtsi    | 13 +++++++++++++
> >>>>    1 file changed, 13 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm/boot/dts/microchip/at91-sama5d27_wlsom1.dtsi b/arch/arm/boot/dts/microchip/at91-sama5d27_wlsom1.dtsi
> >>>> index b34c5072425a..be06df1b7d66 100644
> >>>> --- a/arch/arm/boot/dts/microchip/at91-sama5d27_wlsom1.dtsi
> >>>> +++ b/arch/arm/boot/dts/microchip/at91-sama5d27_wlsom1.dtsi
> >>>> @@ -210,6 +210,9 @@ &macb0 {
> >>>>         #size-cells = <0>;
> >>>>         phy-mode = "rmii";
> >>>>
> >>>> +     nvmem-cells = <&mac_address_eui48>;
> >>>> +     nvmem-cell-names = "mac-address";
> >>>> +
> >>>>         ethernet-phy@0 {
> >>>>                 reg = <0x0>;
> >>>>                 interrupt-parent = <&pioA>;
> >>>> @@ -238,6 +241,16 @@ qspi1_flash: flash@0 {
> >>>>                 m25p,fast-read;
> >>>>                 status = "disabled";
> >>>>
> >>>> +             nvmem-layout {
> > 
> > IMHO this should be "sfdp {".
> > 
> >>>> +                     compatible = "fixed-layout";
> > 
> > Please read my feedback on the first version again..
> > 
> > For the DT maintainers. The SFDP is a small table based content that
> > provide basic information about the flash. There are standard tables
> > which are specified by the JEDEC standard and there are vendor
> > tables, most of the time without proper documentation (or none at
> > all).
> > 
> > Somehow we need to specify at what table we want to look at. I'd
> > like to see a binding which can potentially expose anything inside
> > the SFDP.
> > 
> > So I've suggested to use "compatible = jedec,sfdp-vendor-table-NNNN"
> > where NNNN is the table parameter id. Additionally, the standard ids
> > could have names like "jedec,sfdp-bfpt" (basic flash parameter table).
> > 
> > So in your case that would be:
> > 
> > flash {
> > 	sfdp {
> > 		mac_address: table-1 {
> > 			compatible = "jedec,sfdp-idNNNN";
> > 		};
> > 	};
>
> Should the nvmem-layout be included as a child node under sfdp {}, or 
> should it be implemented as a separate vendor-specific driver to handle 
> the changes introduced in patch 1/3 ?

There is no nvmem-layout involved here.

But another possibility is to make it one. Then you have to
 (1) expose the *entire* sfpd as a nvmem device
 (2a) write an nvmem-layouts driver (in drivers/nvmem/layouts/)
 (2b) come up with a DT binding that is generic enough to expose
      various parameters of the SFDP, not just a one-off for the
      MAC address use case.

Maybe that is even a better fit.

> > };
> > 
> > I don't know what NNNN is. Could you please provide a dump of the
> > sfdp of your flash.
>
> Please find the entire SFDP data of SST26VF064BEUI flash in Table 11.1 
> of 11.0 APPENDIX

Please dump it according to [1], so we have it in a machine readable
format.

-michael

[1] https://docs.kernel.org/driver-api/mtd/spi-nor.html


> > On Mon Jun 9, 2025 at 11:27 AM CEST, Manikandan.M wrote:

>
> https://ww1.microchip.com/downloads/aemDocuments/documents/MPD/ProductDocuments/DataSheets/SST26VF064BEUI-Data-Sheet-DS20006138.pdf
>
>
> The vendor parameter ID is 'BF' if I am not wrong.
> > 
> > -michael
> > 
> >>>> +                     #address-cells = <1>;
> >>>> +                     #size-cells = <1>;
> >>>> +
> >>>> +                     mac_address_eui48: mac-address@0 {
> >>>> +                             reg = <0x0 0x6>;
> >>>> +                     };
> >>>
> >>> How would this work if in the future the mchp vendor table adds some
> >>> other info that needs to be referenced as nvmem? How do you distinguish
> >>> the info from the table?
> >>> Would it be possible to have some kind of address and size to reference
> >>> the SFDP?
> >>
> >> I was previously advised not to hardcode the offset in the Device Tree
> >> [1]. In the current implementation (patch 1/3), the read callback for
> >> the MCHP vendor table distinguishes between EUI-48 and EUI-64 MAC
> >> addresses based on the nvmem size, which corresponds to the size of the
> >> respective MAC address.
> >>
> >> [1] --> https://lore.kernel.org/lkml/D889HZF97H8U.1UUX54BAVLAC3@kernel.org/
> >>
> >>>
> >>>> +             };
> >>>> +
> >>>>                 partitions {
> >>>>                         compatible = "fixed-partitions";
> >>>>                         #address-cells = <1>;
> >>>
> > 


Download attachment "signature.asc" of type "application/pgp-signature" (298 bytes)

Powered by blists - more mailing lists