All of lore.kernel.org
 help / color / mirror / Atom feed
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Maxim Kiselev <bigunclemax@gmail.com>
Cc: "Maxim Kochetkov" <fido_max@inbox.ru>,
	"Thorsten Leemhuis" <regressions@leemhuis.info>,
	linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
	"Vignesh Raghavendra" <vigneshr@ti.com>,
	"Richard Weinberger" <richard@nod.at>,
	"Rafał Miłecki" <rafal@milecki.pl>,
	"regressions@lists.linux.dev" <regressions@lists.linux.dev>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	"Saravana Kannan" <saravanak@google.com>
Subject: Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions'
Date: Tue, 13 Dec 2022 17:54:24 +0100	[thread overview]
Message-ID: <20221213175424.79895b63@xps-13> (raw)
In-Reply-To: <CALHCpMhOku2b+1y5Z=N5RJ6SvCvNakD2rSyRAqp6Gz3JWVvXGg@mail.gmail.com>

Hi Maxim,

bigunclemax@gmail.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@bootlin.com>:
> >
> > Hi Maxim,
> >
> > fido_max@inbox.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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

WARNING: multiple messages have this Message-ID (diff)
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Maxim Kiselev <bigunclemax@gmail.com>
Cc: "Maxim Kochetkov" <fido_max@inbox.ru>,
	"Thorsten Leemhuis" <regressions@leemhuis.info>,
	linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
	"Vignesh Raghavendra" <vigneshr@ti.com>,
	"Richard Weinberger" <richard@nod.at>,
	"Rafał Miłecki" <rafal@milecki.pl>,
	"regressions@lists.linux.dev" <regressions@lists.linux.dev>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	"Saravana Kannan" <saravanak@google.com>
Subject: Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions'
Date: Tue, 13 Dec 2022 17:54:24 +0100	[thread overview]
Message-ID: <20221213175424.79895b63@xps-13> (raw)
In-Reply-To: <CALHCpMhOku2b+1y5Z=N5RJ6SvCvNakD2rSyRAqp6Gz3JWVvXGg@mail.gmail.com>

Hi Maxim,

bigunclemax@gmail.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@bootlin.com>:
> >
> > Hi Maxim,
> >
> > fido_max@inbox.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

  reply	other threads:[~2022-12-13 16:55 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CALHCpMgSZOZdOGpLwTYf0sFD5EMNL7CuqHuFJV_6w5VPSWZnUw@mail.gmail.com>
2022-12-10  9:52 ` Fwd: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' Maxim Kiselev
2022-12-10 12:35   ` Thorsten Leemhuis
2022-12-10 12:35     ` Thorsten Leemhuis
2022-12-11  8:26     ` Maxim Kiselev
2022-12-11  8:26       ` Maxim Kiselev
2022-12-12  9:14       ` Miquel Raynal
2022-12-12  9:14         ` Miquel Raynal
2022-12-12 13:06         ` Maxim Kiselev
2022-12-12 13:06           ` Maxim Kiselev
2022-12-12 16:37           ` Miquel Raynal
2022-12-12 16:37             ` Miquel Raynal
2022-12-12 17:57             ` Maxim Kochetkov
2022-12-12 17:57               ` Maxim Kochetkov
2022-12-13  9:46               ` Miquel Raynal
2022-12-13  9:46                 ` Miquel Raynal
2022-12-13 11:02                 ` Maxim Kiselev
2022-12-13 11:02                   ` Maxim Kiselev
2022-12-13 16:54                   ` Miquel Raynal [this message]
2022-12-13 16:54                     ` Miquel Raynal
2022-12-14 21:53                     ` Saravana Kannan
2022-12-14 21:53                       ` Saravana Kannan
2022-12-16 11:04                       ` Miquel Raynal
2022-12-16 11:04                         ` Miquel Raynal
2022-12-16 11:33                         ` Maxim Kiselev
2022-12-16 11:33                           ` Maxim Kiselev
2022-12-16 13:13                           ` Maxim Kiselev
2022-12-16 13:13                             ` Maxim Kiselev
2022-12-17  1:44                             ` Saravana Kannan
2022-12-17  1:44                               ` Saravana Kannan
2022-12-17  1:45                         ` Saravana Kannan
2022-12-17  1:45                           ` Saravana Kannan
2023-02-24 16:15     ` Fwd: " Linux regression tracking #update (Thorsten Leemhuis)
2023-02-24 16:15       ` Linux regression tracking #update (Thorsten Leemhuis)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221213175424.79895b63@xps-13 \
    --to=miquel.raynal@bootlin.com \
    --cc=bigunclemax@gmail.com \
    --cc=fido_max@inbox.ru \
    --cc=gregkh@linuxfoundation.org \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=rafael@kernel.org \
    --cc=rafal@milecki.pl \
    --cc=regressions@leemhuis.info \
    --cc=regressions@lists.linux.dev \
    --cc=richard@nod.at \
    --cc=robh+dt@kernel.org \
    --cc=saravanak@google.com \
    --cc=vigneshr@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.