Messages in this thread | | | From | Saravana Kannan <> | Date | Fri, 16 Dec 2022 17:44:51 -0800 | Subject | Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' |
| |
On Fri, Dec 16, 2022 at 5:13 AM Maxim Kiselev <bigunclemax@gmail.com> wrote: > > > Maxim, any chance you give this a try? > > [1] - https://lore.kernel.org/lkml/20220810060040.321697-1-saravanak@google.com/ > > OK, I tried it and it didn't seem to help me.
Did you include the fixes to the v1 series I gave as replies to some of the emails in that thread?
-Saravana
> I'm still getting an unresolved dependence for 'f1070000.ethernet' > from 'mac@6' nvmem-cell. > Here is a related part of kernel log: > > [ 0.035553] ethernet@70000 Linked as a fwnode consumer to > main-interrupt-ctrl@20200 > [ 0.035601] ethernet@70000 Linked as a fwnode consumer to > clock-gating-control@1821c > [ 0.035713] ethernet@70000 Linked as a fwnode consumer to mac@6 > > [ 0.046423] device: 'f1070000.ethernet': device_add > [ 0.046689] platform f1070000.ethernet: Not linking > /ocp@f1000000/clock-gating-control@1821c - dev might never probe > [ 0.046747] ethernet@70000 Dropping the fwnode link to > clock-gating-control@1821c > [ 0.046788] platform f1070000.ethernet: Not linking > /ocp@f1000000/main-interrupt-ctrl@20200 - might never become dev > [ 0.046835] ethernet@70000 Dropping the fwnode link to > main-interrupt-ctrl@20200 > > [ 0.113759] device: 'spi0': device_add > [ 0.114523] device: 'spi0.0': device_add > [ 0.115349] spi-nor spi0.0: mx66l51235f (65536 Kbytes) > [ 0.116833] 7 fixed-partitions partitions found on MTD device spi0.0 > [ 0.117026] device: 'f1010600.spi:m25p80@0:partitions:partition@1': > device_add > [ 0.117416] Creating 7 MTD partitions on "spi0.0": > > [ 3.537102] devices_kset: Moving f1070000.ethernet to end of list > [ 3.537391] platform f1070000.ethernet: error -EPROBE_DEFER: wait > for supplier mac@6 > > пт, 16 дек. 2022 г. в 14:33, Maxim Kiselev <bigunclemax@gmail.com>: > > > > > Maxim, any chance you give this a try? > > Ok, I'll try it as soon as possible. > > > > пт, 16 дек. 2022 г. в 14:04, Miquel Raynal <miquel.raynal@bootlin.com>: > > > > > > Hi Saravana, Maxim, Maxim, > > > > > > saravanak@google.com wrote on Wed, 14 Dec 2022 13:53:54 -0800: > > > > > > > On Tue, Dec 13, 2022 at 8:54 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > > > 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. > > > > > > > > > > > > > > > Responding to the whole thread. > > > > > > > > I'm going by Miquel's first email in which he cc'ed me and haven't > > > > actually looked at the mtd code. Couple of comments: > > > > > > > > Independent of mtd/nvmem-cell, I generally frown on having a > > > > compatible string for a child node that you don't treat as a device. > > > > Even more so if you actually create a struct device for it and then > > > > don't do anything else with it. That's just a waste of memory. So, in > > > > general try to avoid that in the future if you can. > > > > > > Agreed, it didn't triggered any warnings in my head in the first place, > > > sorry about that. > > > > > > > Also, there are flags the parent device's driver can set that'll tell > > > > fw_devlink not to treat a specific DT node as a real device. So, if we > > > > really need that I'll dig up and suggest a fix. > > > > > > Interesting, that would indeed very likely fix it. > > > > > > > Lastly and more importantly, I've a series[1] that stops depending on > > > > the compatible property for fw_devlink to work. So it should be > > > > smarter than it is today. But that series has known bugs for which I > > > > gave test fixes in that thread. I plan to make a v2 of that series > > > > with that fix and I'm expecting it'll fix a bunch of fw_devlink > > > > issues. > > > > > > > > Feel free to give v1 + squashing the fixes a shot if you are excited > > > > to try it. Otherwise, I'll try my best to get around to it this week > > > > (kinda swamped though + holidays coming up, so no promises). > > > > > > Can you please include us in your next submission? > > > * Maxim Kiselev <bigunclemax@gmail.com> > > > * Maxim Kochetkov <fido_max@inbox.ru> > > > * Miquel Raynal <miquel.raynal@bootlin.com> > > > > > > > [1] - https://lore.kernel.org/lkml/20220810060040.321697-1-saravanak@google.com/ > > > > > > Maxim, any chance you give this a try? > > > > > > Thanks, > > > Miquèl
| |