Message-ID: <20221213175424.79895b63@xps-13>
Date:   Tue, 13 Dec 2022 17:54:24 +0100
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Maxim Kiselev <bigunclemax@...il.com>
Cc:     Maxim Kochetkov <fido_max@...ox.ru>,
        Thorsten Leemhuis <regressions@...mhuis.info>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Vignesh Raghavendra <vigneshr@...com>,
        Richard Weinberger <richard@....at>,
        Rafał Miłecki <rafal@...ecki.pl>,
        "regressions@...ts.linux.dev" <regressions@...ts.linux.dev>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Saravana Kannan <saravanak@...gle.com>
Subject: Re: nvmem-cells regression after adding 'call
 of_platform_populate() for MTD partitions'

Hi Maxim,

bigunclemax@...il.com wrote on Tue, 13 Dec 2022 14:02:34 +0300:

> I looked closer at commit 658c4448bbbf and bcdf0315a61a, 5db1c2dbc04c16 commits.
> Looks like we have two different features binded to one property - "compatible".
> 
> From one side it is the ability to forward the subnode of the mtd
> partition to the nvmem subsystem (658c4448bbbf and ac42c46f983e).
> And from another side is the ability to use custom initialization of
> the mtd partition (bcdf0315a61a and 5db1c2dbc04c16).
> 
> What I mean:
> According to ac42c46f983e I can create DT like this:
>  - |
>     partitions {
>         compatible = "fixed-partitions";
>         #address-cells = <1>;
>         #size-cells = <1>;
> 
>         partition@0 {
>             compatible = "nvmem-cells";
>             reg = <0x40000 0x10000>;
>             #address-cells = <1>;
>             #size-cells = <1>;
>             macaddr_gmac1: macaddr_gmac1@0 {
>                 reg = <0x0 0x6>;
>             };
>         };
>     };
> 
> 
> And according to 5db1c2dbc04c16 I can create DT like this:
>  - |
>     partitions {
>         compatible = "fixed-partitions";
>         #address-cells = <1>;
>         #size-cells = <1>;
> 
>         partition@0 {
>             compatible = "u-boot,env";
>             reg = <0x40000 0x10000>;
>         };
>     };
> 
> But I can not use them both, because only one "compatible" property allowed.
> This will be incorrect:
>  - |
>     partitions {
>         compatible = "fixed-partitions";
>         #address-cells = <1>;
>         #size-cells = <1>;
> 
>         partition@0 {
>             compatible = "u-boot,env";  # from ac42c46f983e
>             compatible = "nvmem-cells"; # from 5db1c2dbc04c

What about:

	      compatible = "u-boot,env", "nvmem-cells";

instead? that should actually work.

>             reg = <0x40000 0x10000>;
>             #address-cells = <1>;
>             #size-cells = <1>;
>             macaddr_gmac1: macaddr_gmac1@0 {
>                 reg = <0x0 0x6>;
>             };
>         };
>     };
> 
> > compatible: Duplicate property name  
> 
> вт, 13 дек. 2022 г. в 12:46, Miquel Raynal <miquel.raynal@...tlin.com>:
> >
> > Hi Maxim,
> >
> > fido_max@...ox.ru wrote on Mon, 12 Dec 2022 20:57:49 +0300:
> >  
> > > Hi, Miquel!
> > >
> > > On 12.12.2022 19:37, Miquel Raynal wrote:
> > >  
> > > > Let me try to recap the situation for all the people I just involved:
> > > >
> > > > * An Ethernet driver gets its mac address from an nvmem cell. The
> > > >    Ethernet controller DT node then has an "nvmem-cells" property
> > > >    pointing towards an nvmem cell.
> > > > * The nvmem cell comes from an mtd partition.
> > > > * The mtd partition is flagged with a particular compatible
> > > >    (which is also named "nvmem-cells") to tell the kernel that the node
> > > >    produces nvmem cells.
> > > > * The mtd partition itself has no driver, but is the child node of a
> > > >    "partitions" container which has one (in this case,
> > > >    "fixed-partitions", see the snippet below).
> > > >
> > > > Because the "nvmem-cells" property of the Ethernet node points at the
> > > > nvmem-cell node, the core create a device link between the Ethernet
> > > > controller (consumer) and the mtd partition (producer).
> > > >
> > > > The device link in this case will never be satisfied because no driver
> > > > matches the "nvmem-cells" compatible of the partition node.
> > > >
> > > > Reverting commit bcdf0315a61a ("mtd: call of_platform_populate() for MTD
> > > > partitions") would IMHO not make much sense, the problem comes from the
> > > > device link side and even there, there is nothing really "wrong",
> > > > because I really expect the mtd device to be ready before the
> > > > Ethernet controller probe, the device link is legitimate.
> > > >
> > > > So I would like to explore other alternatives. Here are a bunch of
> > > > ideas, but I'm open:  
> > >
> > > How about to create simple driver with compatible="nvmem-cell" and to move all the suff from main mtd driver which serves nvmem-cell to the probe function?  
> >
> > This is probably worth the try but I doubt you can make it work without
> > regressions because IIRC the nvmem registration happens no matter the
> > compatible (not mentioning the user-otp and factory-otp cases). You can
> > definitely try this out if you think you can come up with something
> > though.
> >
> > But I would like to hear from the device-link gurus :) because even if
> > we fix mtd with a "trick" like above, I guess we'll very likely find
> > other corner cases like that and I am interested in understanding the
> > rationale of what could be a proper fix.
> >
> > Thanks,
> > Miquèl  


Thanks,
Miquèl

Powered by blists - more mailing lists