* Fwd: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' [not found] <CALHCpMgSZOZdOGpLwTYf0sFD5EMNL7CuqHuFJV_6w5VPSWZnUw@mail.gmail.com> @ 2022-12-10 9:52 ` Maxim Kiselev 2022-12-10 12:35 ` Thorsten Leemhuis 0 siblings, 1 reply; 17+ messages in thread From: Maxim Kiselev @ 2022-12-10 9:52 UTC (permalink / raw) To: linux-mtd Cc: linux-kernel, Vignesh Raghavendra, Richard Weinberger, Miquel Raynal, Maxim Kochetkov, Maksim Kiselev Hi, friends. After applying this commit 'mtd: call of_platform_populate() for MTD partitions' (bcdf0315), I faced with a problem that my ethernet device can't be probed because it wait when 'nvmem-cells' device will be probed first. But there is no such driver which is compatible with 'nvmem-cells' because 'nvmem-cells' is just a mark used by the 'mtd_nvmem_add' function. So this leads to appeating of unresolved dependency for the ethernet device. And that's why the ethernet device can't be added and probed. Here is a part of kernel log when spi flash probe start: > device: 'spi0': device_add > device: 'spi0.0': device_add > spi-nor spi0.0: mx66l51235f (65536 Kbytes) > 7 fixed-partitions partitions found on MTD device spi0.0 After 'm25p80' probe 'f1070000.ethernet' linked to 'partition@1' : > device: 'f1010600.spi:m25p80@0: partitions:partition@1': device_add > device: 'platform:f1010600.spi:m25p80@0:partitions:partition@1--platform:f1070000.ethernet': device_add > devices_kset: Moving f1070000.ethernet to end of list > platform f1070000.ethernet: Linked as a consumer to f1010600.spi:m25p80@0:partitions:partition@1 > ethernet@70000 Dropping the fwnode link to partition@1 And as a result I got `-EPROBE_DEFER` for `f1070000.ethernet` > platform f1070000.ethernet: error -EPROBE_DEFER: supplier f1010600.spi:m25p80@0:partitions:partition@1 not ready Here is a part of my device tree: enet1: ethernet@70000 { status = "okay"; nvmem-cells = <&macaddr>; nvmem-cell-names = "mac-address"; phy-mode = "rgmii"; phy = <&phy0>; }; spi@10600 { status = "okay"; m25p80@0 { compatible = "mx66l51235l"; reg = <0>; #address-cells = <1>; #size-cells = <1>; partitions { compatible = "fixed-partitions"; #address-cells = <1>; #size-cells = <1>; partition@0 { reg = <0x00000000 0x000080000>; label = "SPI.U_BOOT"; }; partition@1 { compatible = "nvmem-cells"; reg = <0x000A0000 0x00020000>; label = "SPI.INV_INFO"; #address-cells = <1>; #size-cells = <1>; ranges = <0 0x000A0000 0x00020000>; macaddr: mac@6 { reg = <0x6 0x6>; }; }; }; }; }; In the example above 'ethernet@70000' requires 'macaddr: mac@6' which is located inside mtd 'partition@1' of 'm25p80@0' spi flash. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 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-11 8:26 ` Maxim Kiselev 2023-02-24 16:15 ` Fwd: " Linux regression tracking #update (Thorsten Leemhuis) 0 siblings, 2 replies; 17+ messages in thread From: Thorsten Leemhuis @ 2022-12-10 12:35 UTC (permalink / raw) To: Maxim Kiselev, linux-mtd Cc: linux-kernel, Vignesh Raghavendra, Richard Weinberger, Miquel Raynal, Maxim Kochetkov, Rafał Miłecki, regressions [CCing the regression mailing list, as it should be in the loop for all regressions, as explained in https://docs.kernel.org/admin-guide/reporting-regressions.html ] Hi, this is your Linux kernel regression tracker. Thx for the report. On 10.12.22 10:52, Maxim Kiselev wrote: > > After applying This makes me wonder: "applying" as in "applying it to some version that doesn't contain this change normally" or as it "after it was applied to mainline I have the following problem with vanilla kernel version <???>"? > this commit 'mtd: call of_platform_populate() for MTD > partitions' (bcdf0315), CCing Rafał, who authored bcdf0315. > I faced with a problem that my ethernet device can't be probed because it > wait when 'nvmem-cells' device will be probed first. FWIW, there is a discussion about a problems that at least to my untrained eyes looks similar: https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ Rafał, has some progress been made to resolve this? To me it sounds like this might warrant a "revert, and reapply later when the cause for the regression was addressed". Rafał, it seems you suggested something like that, but it doesn't look like that happened for one reason or another. Or am I missing something? Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) P.S.: As the Linux kernel's regression tracker I deal with a lot of reports and sometimes miss something important when writing mails like this. If that's the case here, don't hesitate to tell me in a public reply, it's in everyone's interest to set the public record straight. > But there is no such driver which is compatible with 'nvmem-cells' because > 'nvmem-cells' is just a mark used by the 'mtd_nvmem_add' function. > > So this leads to appeating of unresolved dependency for the ethernet device. > And that's why the ethernet device can't be added and probed. > > Here is a part of kernel log when spi flash probe start: > >> device: 'spi0': device_add >> device: 'spi0.0': device_add >> spi-nor spi0.0: mx66l51235f (65536 Kbytes) >> 7 fixed-partitions partitions found on MTD device spi0.0 > > After 'm25p80' probe 'f1070000.ethernet' linked to 'partition@1' : > >> device: 'f1010600.spi:m25p80@0: > partitions:partition@1': device_add >> device: 'platform:f1010600.spi:m25p80@0:partitions:partition@1--platform:f1070000.ethernet': device_add >> devices_kset: Moving f1070000.ethernet to end of list >> platform f1070000.ethernet: Linked as a consumer to f1010600.spi:m25p80@0:partitions:partition@1 >> ethernet@70000 Dropping the fwnode link to partition@1 > > And as a result I got `-EPROBE_DEFER` for `f1070000.ethernet` > >> platform f1070000.ethernet: error -EPROBE_DEFER: supplier f1010600.spi:m25p80@0:partitions:partition@1 not ready > > Here is a part of my device tree: > > enet1: ethernet@70000 { > status = "okay"; > nvmem-cells = <&macaddr>; > nvmem-cell-names = "mac-address"; > phy-mode = "rgmii"; > phy = <&phy0>; > }; > > spi@10600 { > status = "okay"; > > m25p80@0 { > compatible = "mx66l51235l"; > reg = <0>; > #address-cells = <1>; > #size-cells = <1>; > > partitions { > compatible = "fixed-partitions"; > #address-cells = <1>; > #size-cells = <1>; > > partition@0 { > reg = <0x00000000 0x000080000>; > label = "SPI.U_BOOT"; > }; > > partition@1 { > compatible = "nvmem-cells"; > reg = <0x000A0000 0x00020000>; > label = "SPI.INV_INFO"; > #address-cells = <1>; > #size-cells = <1>; > ranges = <0 0x000A0000 0x00020000>; > > macaddr: mac@6 { > reg = <0x6 0x6>; > }; > }; > > }; > }; > }; > > In the example above 'ethernet@70000' requires 'macaddr: mac@6' which is > located inside mtd 'partition@1' of 'm25p80@0' spi flash. P.P.S.: let me add this to the regression tracking: #regzbot ^introduced bcdf0315 #regzbot title mtd: ethernet device can't be probed anymore due to broken nvmem-cells dep #regzbot monitor: https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ #regzbot ignore-activity ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-10 12:35 ` Thorsten Leemhuis @ 2022-12-11 8:26 ` Maxim Kiselev 2022-12-12 9:14 ` Miquel Raynal 2023-02-24 16:15 ` Fwd: " Linux regression tracking #update (Thorsten Leemhuis) 1 sibling, 1 reply; 17+ messages in thread From: Maxim Kiselev @ 2022-12-11 8:26 UTC (permalink / raw) To: Thorsten Leemhuis Cc: linux-mtd, linux-kernel, Vignesh Raghavendra, Richard Weinberger, Miquel Raynal, Maxim Kochetkov, Rafał Miłecki, regressions >On 10.12.22 10:52, Maxim Kiselev wrote: >> >> After applying > >This makes me wonder: "applying" as in "applying it to some version that >doesn't contain this change normally" or as it "after it was applied to >mainline I have the following problem with vanilla kernel version <???>"? Sorry for confusing you, I mean "after it was applied to mainline". I have this problem with vanilla kernel version 6.0. >>> I faced with a problem that my ethernet device can't be probed because it >>> wait when 'nvmem-cells' device will be probed first. >> >>FWIW, there is a discussion about a problems that at least to my >>untrained eyes looks similar: >>https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ Yes it looks like the same issue. I think the root of the problem was the choice of 'compatible' device tree property to mark the mtd partition node as a nvmem provider. I'm talking about this part in 'mtd_nvmem_add' function. > config.no_of_node = !of_device_is_compatible(node, "nvmem-cells"); Maybe we should change the 'compatible' property to something else? сб, 10 дек. 2022 г. в 15:35, Thorsten Leemhuis <regressions@leemhuis.info>: > > [CCing the regression mailing list, as it should be in the loop for all > regressions, as explained in > https://docs.kernel.org/admin-guide/reporting-regressions.html ] > > Hi, this is your Linux kernel regression tracker. Thx for the report. > > On 10.12.22 10:52, Maxim Kiselev wrote: > > > > After applying > > This makes me wonder: "applying" as in "applying it to some version that > doesn't contain this change normally" or as it "after it was applied to > mainline I have the following problem with vanilla kernel version <???>"? > > > this commit 'mtd: call of_platform_populate() for MTD > > partitions' (bcdf0315), > > CCing Rafał, who authored bcdf0315. > > > I faced with a problem that my ethernet device can't be probed because it > > wait when 'nvmem-cells' device will be probed first. > > FWIW, there is a discussion about a problems that at least to my > untrained eyes looks similar: > https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ > > Rafał, has some progress been made to resolve this? > > To me it sounds like this might warrant a "revert, and reapply later > when the cause for the regression was addressed". Rafał, it seems you > suggested something like that, but it doesn't look like that happened > for one reason or another. Or am I missing something? > > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) > > P.S.: As the Linux kernel's regression tracker I deal with a lot of > reports and sometimes miss something important when writing mails like > this. If that's the case here, don't hesitate to tell me in a public > reply, it's in everyone's interest to set the public record straight. > > > But there is no such driver which is compatible with 'nvmem-cells' because > > 'nvmem-cells' is just a mark used by the 'mtd_nvmem_add' function. > > > > So this leads to appeating of unresolved dependency for the ethernet device. > > And that's why the ethernet device can't be added and probed. > > > > Here is a part of kernel log when spi flash probe start: > > > >> device: 'spi0': device_add > >> device: 'spi0.0': device_add > >> spi-nor spi0.0: mx66l51235f (65536 Kbytes) > >> 7 fixed-partitions partitions found on MTD device spi0.0 > > > > After 'm25p80' probe 'f1070000.ethernet' linked to 'partition@1' : > > > >> device: 'f1010600.spi:m25p80@0: > > partitions:partition@1': device_add > >> device: 'platform:f1010600.spi:m25p80@0:partitions:partition@1--platform:f1070000.ethernet': device_add > >> devices_kset: Moving f1070000.ethernet to end of list > >> platform f1070000.ethernet: Linked as a consumer to f1010600.spi:m25p80@0:partitions:partition@1 > >> ethernet@70000 Dropping the fwnode link to partition@1 > > > > And as a result I got `-EPROBE_DEFER` for `f1070000.ethernet` > > > >> platform f1070000.ethernet: error -EPROBE_DEFER: supplier f1010600.spi:m25p80@0:partitions:partition@1 not ready > > > > Here is a part of my device tree: > > > > enet1: ethernet@70000 { > > status = "okay"; > > nvmem-cells = <&macaddr>; > > nvmem-cell-names = "mac-address"; > > phy-mode = "rgmii"; > > phy = <&phy0>; > > }; > > > > spi@10600 { > > status = "okay"; > > > > m25p80@0 { > > compatible = "mx66l51235l"; > > reg = <0>; > > #address-cells = <1>; > > #size-cells = <1>; > > > > partitions { > > compatible = "fixed-partitions"; > > #address-cells = <1>; > > #size-cells = <1>; > > > > partition@0 { > > reg = <0x00000000 0x000080000>; > > label = "SPI.U_BOOT"; > > }; > > > > partition@1 { > > compatible = "nvmem-cells"; > > reg = <0x000A0000 0x00020000>; > > label = "SPI.INV_INFO"; > > #address-cells = <1>; > > #size-cells = <1>; > > ranges = <0 0x000A0000 0x00020000>; > > > > macaddr: mac@6 { > > reg = <0x6 0x6>; > > }; > > }; > > > > }; > > }; > > }; > > > > In the example above 'ethernet@70000' requires 'macaddr: mac@6' which is > > located inside mtd 'partition@1' of 'm25p80@0' spi flash. > > P.P.S.: let me add this to the regression tracking: > > #regzbot ^introduced bcdf0315 > #regzbot title mtd: ethernet device can't be probed anymore due to > broken nvmem-cells dep > #regzbot monitor: https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ > #regzbot ignore-activity ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-11 8:26 ` Maxim Kiselev @ 2022-12-12 9:14 ` Miquel Raynal 2022-12-12 13:06 ` Maxim Kiselev 0 siblings, 1 reply; 17+ messages in thread From: Miquel Raynal @ 2022-12-12 9:14 UTC (permalink / raw) To: Maxim Kiselev Cc: Thorsten Leemhuis, linux-mtd, linux-kernel, Vignesh Raghavendra, Richard Weinberger, Maxim Kochetkov, Rafał Miłecki, regressions Hi Maxim, bigunclemax@gmail.com wrote on Sun, 11 Dec 2022 11:26:29 +0300: > >On 10.12.22 10:52, Maxim Kiselev wrote: > >> > >> After applying > > > >This makes me wonder: "applying" as in "applying it to some version that > >doesn't contain this change normally" or as it "after it was applied to > >mainline I have the following problem with vanilla kernel version <???>"? > > Sorry for confusing you, I mean "after it was applied to mainline". > I have this problem with vanilla kernel version 6.0. > > >>> I faced with a problem that my ethernet device can't be probed because it > >>> wait when 'nvmem-cells' device will be probed first. > >> > >>FWIW, there is a discussion about a problems that at least to my > >>untrained eyes looks similar: > >>https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ > > Yes it looks like the same issue. > > > I think the root of the problem was the choice of 'compatible' > device tree property to mark the mtd partition node as a nvmem provider. > > I'm talking about this part in 'mtd_nvmem_add' function. > > config.no_of_node = !of_device_is_compatible(node, "nvmem-cells"); > > Maybe we should change the 'compatible' property to something else? At a first glance I don't get why the compatible would matter so much here, can you point to some core DT logic that would have an effect? I mean besides leading to the creation of a cell. IOW, what would be done differently if the compatible was different? Can you also dump the device links (if you can reach a prompt) from sysfs? In theory there should be a link between ethernet and spi-nor which is fulfilled when the spi-nor device probes and leads to the creation of device links. Maybe there is "something else" that the mtd core should do, because this just works with eeproms (non-mtd cells), so let's find out. > сб, 10 дек. 2022 г. в 15:35, Thorsten Leemhuis <regressions@leemhuis.info>: > > > > [CCing the regression mailing list, as it should be in the loop for all > > regressions, as explained in > > https://docs.kernel.org/admin-guide/reporting-regressions.html ] > > > > Hi, this is your Linux kernel regression tracker. Thx for the report. > > > > On 10.12.22 10:52, Maxim Kiselev wrote: > > > > > > After applying > > > > This makes me wonder: "applying" as in "applying it to some version that > > doesn't contain this change normally" or as it "after it was applied to > > mainline I have the following problem with vanilla kernel version <???>"? > > > > > this commit 'mtd: call of_platform_populate() for MTD > > > partitions' (bcdf0315), > > > > CCing Rafał, who authored bcdf0315. > > > > > I faced with a problem that my ethernet device can't be probed because it > > > wait when 'nvmem-cells' device will be probed first. > > > > FWIW, there is a discussion about a problems that at least to my > > untrained eyes looks similar: > > https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ > > > > Rafał, has some progress been made to resolve this? > > > > To me it sounds like this might warrant a "revert, and reapply later > > when the cause for the regression was addressed". Rafał, it seems you > > suggested something like that, but it doesn't look like that happened > > for one reason or another. Or am I missing something? > > > > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) > > > > P.S.: As the Linux kernel's regression tracker I deal with a lot of > > reports and sometimes miss something important when writing mails like > > this. If that's the case here, don't hesitate to tell me in a public > > reply, it's in everyone's interest to set the public record straight. > > > > > But there is no such driver which is compatible with 'nvmem-cells' because > > > 'nvmem-cells' is just a mark used by the 'mtd_nvmem_add' function. > > > > > > So this leads to appeating of unresolved dependency for the ethernet device. > > > And that's why the ethernet device can't be added and probed. > > > > > > Here is a part of kernel log when spi flash probe start: > > > > > >> device: 'spi0': device_add > > >> device: 'spi0.0': device_add > > >> spi-nor spi0.0: mx66l51235f (65536 Kbytes) > > >> 7 fixed-partitions partitions found on MTD device spi0.0 > > > > > > After 'm25p80' probe 'f1070000.ethernet' linked to 'partition@1' : > > > > > >> device: 'f1010600.spi:m25p80@0: > > > partitions:partition@1': device_add > > >> device: 'platform:f1010600.spi:m25p80@0:partitions:partition@1--platform:f1070000.ethernet': device_add > > >> devices_kset: Moving f1070000.ethernet to end of list > > >> platform f1070000.ethernet: Linked as a consumer to f1010600.spi:m25p80@0:partitions:partition@1 > > >> ethernet@70000 Dropping the fwnode link to partition@1 > > > > > > And as a result I got `-EPROBE_DEFER` for `f1070000.ethernet` > > > > > >> platform f1070000.ethernet: error -EPROBE_DEFER: supplier f1010600.spi:m25p80@0:partitions:partition@1 not ready > > > > > > Here is a part of my device tree: > > > > > > enet1: ethernet@70000 { > > > status = "okay"; > > > nvmem-cells = <&macaddr>; > > > nvmem-cell-names = "mac-address"; > > > phy-mode = "rgmii"; > > > phy = <&phy0>; > > > }; > > > > > > spi@10600 { > > > status = "okay"; > > > > > > m25p80@0 { > > > compatible = "mx66l51235l"; > > > reg = <0>; > > > #address-cells = <1>; > > > #size-cells = <1>; > > > > > > partitions { > > > compatible = "fixed-partitions"; > > > #address-cells = <1>; > > > #size-cells = <1>; > > > > > > partition@0 { > > > reg = <0x00000000 0x000080000>; > > > label = "SPI.U_BOOT"; > > > }; > > > > > > partition@1 { > > > compatible = "nvmem-cells"; > > > reg = <0x000A0000 0x00020000>; > > > label = "SPI.INV_INFO"; > > > #address-cells = <1>; > > > #size-cells = <1>; > > > ranges = <0 0x000A0000 0x00020000>; > > > > > > macaddr: mac@6 { > > > reg = <0x6 0x6>; > > > }; > > > }; > > > > > > }; > > > }; > > > }; > > > > > > In the example above 'ethernet@70000' requires 'macaddr: mac@6' which is > > > located inside mtd 'partition@1' of 'm25p80@0' spi flash. > > > > P.P.S.: let me add this to the regression tracking: > > > > #regzbot ^introduced bcdf0315 > > #regzbot title mtd: ethernet device can't be probed anymore due to > > broken nvmem-cells dep > > #regzbot monitor: https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ > > #regzbot ignore-activity Thanks, Miquèl ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-12 9:14 ` Miquel Raynal @ 2022-12-12 13:06 ` Maxim Kiselev 2022-12-12 16:37 ` Miquel Raynal 0 siblings, 1 reply; 17+ messages in thread From: Maxim Kiselev @ 2022-12-12 13:06 UTC (permalink / raw) To: Miquel Raynal Cc: Thorsten Leemhuis, linux-mtd, linux-kernel, Vignesh Raghavendra, Richard Weinberger, Maxim Kochetkov, Rafał Miłecki, regressions > At a first glance I don't get why the compatible would matter so much > here, can you point to some core DT logic that would have an effect? I > mean besides leading to the creation of a cell. IOW, what would be done > differently if the compatible was different? After adding a call of_platform_populate() for MTD partitions (bcdf0315) we got a call of 'device_add' for each mtd partition which have a 'compatible' property in its DT node. As a result any other devices which have a DT reference to 'nvmem-cell' compatible mtd partition got dependence from it (see attached log below). Now they will be waiting until a 'nvmem-cell' device will be probed. But this will never happen because there is no 'nvmem-cell' driver. Here is part of kernel log after bcdf0315 commit: device: 'spi0.0': device_add spi-nor spi0.0: mx66l51235f (65536 Kbytes) 7 fixed-partitions partitions found on MTD device spi0.0 device: 'f1010600.spi:m25p80@0:partitions:partition@1': device_add device: 'platform:f1010600.spi:m25p80@0:partitions:partition@1--platform:f1070000.ethernet': device_add devices_kset: Moving f1070000.ethernet to end of list platform f1070000.ethernet: Linked as a consumer to f1010600.spi:m25p80@0:partitions:partition@1 ethernet@70000 Dropping the fwnode link to partition@1 Creating 7 MTD partitions on "spi0.0": platform f1070000.ethernet: error -EPROBE_DEFER: supplier f1010600.spi:m25p80@0:partitions:partition@1 not ready devices_kset: Moving f1070000.ethernet to end of list platform f1070000.ethernet: error -EPROBE_DEFER: supplier f1010600.spi:m25p80@0:partitions:partition@1 not ready Here is part of kernel log with reverted bcdf0315 commit: device: 'spi0.0': device_add spi-nor spi0.0: mx66l51235f (65536 Kbytes) 7 fixed-partitions partitions found on MTD device spi0.0 Creating 7 MTD partitions on "spi0.0": ethernet@70000 Dropping the fwnode link to partition@1 device: 'eth0': device_add mvneta f1070000.ethernet eth0: Using device tree mac address de:fa:ce:db:ab:e1 Look at strings below which exists only in the first log (with bcdf0315): device: 'f1010600.spi:m25p80@0:partitions:partition@1': device_add device: 'platform:f1010600.spi:m25p80@0:partitions:partition@1--platform:f1070000.ethernet': device_add platform f1070000.ethernet: error -EPROBE_DEFER: supplier f1010600.spi:m25p80@0:partitions:partition@1 not ready > because this just works with eeproms (non-mtd cells), so let's find out. This works for eeproms and even for whole mtd partitions because there are compatible drivers and those devices can be probed, ulike a mtd subnode which is compatible with non-existent 'nvmem-cell' driver. пн, 12 дек. 2022 г. в 12:14, Miquel Raynal <miquel.raynal@bootlin.com>: > > Hi Maxim, > > bigunclemax@gmail.com wrote on Sun, 11 Dec 2022 11:26:29 +0300: > > > >On 10.12.22 10:52, Maxim Kiselev wrote: > > >> > > >> After applying > > > > > >This makes me wonder: "applying" as in "applying it to some version that > > >doesn't contain this change normally" or as it "after it was applied to > > >mainline I have the following problem with vanilla kernel version <???>"? > > > > Sorry for confusing you, I mean "after it was applied to mainline". > > I have this problem with vanilla kernel version 6.0. > > > > >>> I faced with a problem that my ethernet device can't be probed because it > > >>> wait when 'nvmem-cells' device will be probed first. > > >> > > >>FWIW, there is a discussion about a problems that at least to my > > >>untrained eyes looks similar: > > >>https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ > > > > Yes it looks like the same issue. > > > > > > I think the root of the problem was the choice of 'compatible' > > device tree property to mark the mtd partition node as a nvmem provider. > > > > I'm talking about this part in 'mtd_nvmem_add' function. > > > config.no_of_node = !of_device_is_compatible(node, "nvmem-cells"); > > > > Maybe we should change the 'compatible' property to something else? > > At a first glance I don't get why the compatible would matter so much > here, can you point to some core DT logic that would have an effect? I > mean besides leading to the creation of a cell. IOW, what would be done > differently if the compatible was different? > > Can you also dump the device links (if you can reach a prompt) from > sysfs? > > In theory there should be a link between ethernet and spi-nor which is > fulfilled when the spi-nor device probes and leads to the creation of > device links. Maybe there is "something else" that the mtd core should > do, because this just works with eeproms (non-mtd cells), so let's find > out. > > > сб, 10 дек. 2022 г. в 15:35, Thorsten Leemhuis <regressions@leemhuis.info>: > > > > > > [CCing the regression mailing list, as it should be in the loop for all > > > regressions, as explained in > > > https://docs.kernel.org/admin-guide/reporting-regressions.html ] > > > > > > Hi, this is your Linux kernel regression tracker. Thx for the report. > > > > > > On 10.12.22 10:52, Maxim Kiselev wrote: > > > > > > > > After applying > > > > > > This makes me wonder: "applying" as in "applying it to some version that > > > doesn't contain this change normally" or as it "after it was applied to > > > mainline I have the following problem with vanilla kernel version <???>"? > > > > > > > this commit 'mtd: call of_platform_populate() for MTD > > > > partitions' (bcdf0315), > > > > > > CCing Rafał, who authored bcdf0315. > > > > > > > I faced with a problem that my ethernet device can't be probed because it > > > > wait when 'nvmem-cells' device will be probed first. > > > > > > FWIW, there is a discussion about a problems that at least to my > > > untrained eyes looks similar: > > > https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ > > > > > > Rafał, has some progress been made to resolve this? > > > > > > To me it sounds like this might warrant a "revert, and reapply later > > > when the cause for the regression was addressed". Rafał, it seems you > > > suggested something like that, but it doesn't look like that happened > > > for one reason or another. Or am I missing something? > > > > > > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) > > > > > > P.S.: As the Linux kernel's regression tracker I deal with a lot of > > > reports and sometimes miss something important when writing mails like > > > this. If that's the case here, don't hesitate to tell me in a public > > > reply, it's in everyone's interest to set the public record straight. > > > > > > > But there is no such driver which is compatible with 'nvmem-cells' because > > > > 'nvmem-cells' is just a mark used by the 'mtd_nvmem_add' function. > > > > > > > > So this leads to appeating of unresolved dependency for the ethernet device. > > > > And that's why the ethernet device can't be added and probed. > > > > > > > > Here is a part of kernel log when spi flash probe start: > > > > > > > >> device: 'spi0': device_add > > > >> device: 'spi0.0': device_add > > > >> spi-nor spi0.0: mx66l51235f (65536 Kbytes) > > > >> 7 fixed-partitions partitions found on MTD device spi0.0 > > > > > > > > After 'm25p80' probe 'f1070000.ethernet' linked to 'partition@1' : > > > > > > > >> device: 'f1010600.spi:m25p80@0: > > > > partitions:partition@1': device_add > > > >> device: 'platform:f1010600.spi:m25p80@0:partitions:partition@1--platform:f1070000.ethernet': device_add > > > >> devices_kset: Moving f1070000.ethernet to end of list > > > >> platform f1070000.ethernet: Linked as a consumer to f1010600.spi:m25p80@0:partitions:partition@1 > > > >> ethernet@70000 Dropping the fwnode link to partition@1 > > > > > > > > And as a result I got `-EPROBE_DEFER` for `f1070000.ethernet` > > > > > > > >> platform f1070000.ethernet: error -EPROBE_DEFER: supplier f1010600.spi:m25p80@0:partitions:partition@1 not ready > > > > > > > > Here is a part of my device tree: > > > > > > > > enet1: ethernet@70000 { > > > > status = "okay"; > > > > nvmem-cells = <&macaddr>; > > > > nvmem-cell-names = "mac-address"; > > > > phy-mode = "rgmii"; > > > > phy = <&phy0>; > > > > }; > > > > > > > > spi@10600 { > > > > status = "okay"; > > > > > > > > m25p80@0 { > > > > compatible = "mx66l51235l"; > > > > reg = <0>; > > > > #address-cells = <1>; > > > > #size-cells = <1>; > > > > > > > > partitions { > > > > compatible = "fixed-partitions"; > > > > #address-cells = <1>; > > > > #size-cells = <1>; > > > > > > > > partition@0 { > > > > reg = <0x00000000 0x000080000>; > > > > label = "SPI.U_BOOT"; > > > > }; > > > > > > > > partition@1 { > > > > compatible = "nvmem-cells"; > > > > reg = <0x000A0000 0x00020000>; > > > > label = "SPI.INV_INFO"; > > > > #address-cells = <1>; > > > > #size-cells = <1>; > > > > ranges = <0 0x000A0000 0x00020000>; > > > > > > > > macaddr: mac@6 { > > > > reg = <0x6 0x6>; > > > > }; > > > > }; > > > > > > > > }; > > > > }; > > > > }; > > > > > > > > In the example above 'ethernet@70000' requires 'macaddr: mac@6' which is > > > > located inside mtd 'partition@1' of 'm25p80@0' spi flash. > > > > > > P.P.S.: let me add this to the regression tracking: > > > > > > #regzbot ^introduced bcdf0315 > > > #regzbot title mtd: ethernet device can't be probed anymore due to > > > broken nvmem-cells dep > > > #regzbot monitor: https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ > > > #regzbot ignore-activity > > > Thanks, > Miquèl ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-12 13:06 ` Maxim Kiselev @ 2022-12-12 16:37 ` Miquel Raynal 2022-12-12 17:57 ` Maxim Kochetkov 0 siblings, 1 reply; 17+ messages in thread From: Miquel Raynal @ 2022-12-12 16:37 UTC (permalink / raw) To: Maxim Kiselev Cc: Thorsten Leemhuis, linux-mtd, linux-kernel, Vignesh Raghavendra, Richard Weinberger, Maxim Kochetkov, Rafał Miłecki, regressions, Rob Herring, Krzysztof Kozlowski, Greg Kroah-Hartman, Rafael J. Wysocki, Saravana Kannan Hi Maxim, + Rob & Krzysztof for the binding side + Greg, Rafael & Saravana for the device links side bigunclemax@gmail.com wrote on Mon, 12 Dec 2022 16:06:35 +0300: > > At a first glance I don't get why the compatible would matter so much > > here, can you point to some core DT logic that would have an effect? I > > mean besides leading to the creation of a cell. IOW, what would be done > > differently if the compatible was different? > > After adding a call of_platform_populate() for MTD partitions (bcdf0315) > we got a call of 'device_add' for each mtd partition which have > a 'compatible' property in its DT node. As a result any other devices > which have a DT reference to 'nvmem-cell' compatible mtd partition > got dependence from it (see attached log below). Now they will be waiting until > a 'nvmem-cell' device will be probed. But this will never happen because there > is no 'nvmem-cell' driver. > > > Here is part of kernel log after bcdf0315 commit: > device: 'spi0.0': device_add > spi-nor spi0.0: mx66l51235f (65536 Kbytes) > 7 fixed-partitions partitions found on MTD device spi0.0 > device: 'f1010600.spi:m25p80@0:partitions:partition@1': device_add > device: 'platform:f1010600.spi:m25p80@0:partitions:partition@1--platform:f1070000.ethernet': > device_add > devices_kset: Moving f1070000.ethernet to end of list > platform f1070000.ethernet: Linked as a consumer to > f1010600.spi:m25p80@0:partitions:partition@1 > ethernet@70000 Dropping the fwnode link to partition@1 > Creating 7 MTD partitions on "spi0.0": > platform f1070000.ethernet: error -EPROBE_DEFER: supplier > f1010600.spi:m25p80@0:partitions:partition@1 not ready > devices_kset: Moving f1070000.ethernet to end of list > platform f1070000.ethernet: error -EPROBE_DEFER: supplier > f1010600.spi:m25p80@0:partitions:partition@1 not ready > > Here is part of kernel log with reverted bcdf0315 commit: > device: 'spi0.0': device_add > spi-nor spi0.0: mx66l51235f (65536 Kbytes) > 7 fixed-partitions partitions found on MTD device spi0.0 > Creating 7 MTD partitions on "spi0.0": > ethernet@70000 Dropping the fwnode link to partition@1 > device: 'eth0': device_add > mvneta f1070000.ethernet eth0: Using device tree mac address de:fa:ce:db:ab:e1 > > Look at strings below which exists only in the first log (with bcdf0315): > device: 'f1010600.spi:m25p80@0:partitions:partition@1': device_add > device: 'platform:f1010600.spi:m25p80@0:partitions:partition@1--platform:f1070000.ethernet': > device_add > platform f1070000.ethernet: error -EPROBE_DEFER: supplier > f1010600.spi:m25p80@0:partitions:partition@1 not ready > > > > because this just works with eeproms (non-mtd cells), so let's find out. > This works for eeproms and even for whole mtd partitions because there > are compatible drivers and those devices > can be probed, ulike a mtd subnode which is compatible with > non-existent 'nvmem-cell' driver. 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: 1. compatible = "nvmem-cells" was maybe not the right way to say "this is an nvmem-cells producer": 1a. Maybe we could use the (new) nvmem-layout container [1]? 1b. Or we could force users to use the user-otp container [2]? 1c. Or replace the compatible by a property? 1c is ugly, all three solutions would break the current bindings, hence I would really prefer to got for #2. 2. I don't understand the device link state/flags deeply enough to understand what they all convey exactly, but I guess we should be able to tell the driver model that the "device" (ie. the partition) was created by a driver which owns this device. In this case an mtd device owned by the mtd core was created by a parser, and there is no additional probe step that should be expected. Maybe there is a way to tell this to the driver core at device registration time? Or perhaps we should workaround it by calling one of: - device_links_driver_bound() - device_links_no_driver() from the mtd core, after registering all the mtd partition devices? I am really interested to understand better how the device links are expected to evolve, so any pointers would be appreciated. [1] https://lore.kernel.org/all/20221114085659.847611-1-miquel.raynal@bootlin.com/T/#m5c6ed1efd675fda0e2cd174502d38c1565f0274b [2] https://lore.kernel.org/all/20210424110608.15748-4-michael@walle.cc/ Thanks, Miquèl > > пн, 12 дек. 2022 г. в 12:14, Miquel Raynal <miquel.raynal@bootlin.com>: > > > > Hi Maxim, > > > > bigunclemax@gmail.com wrote on Sun, 11 Dec 2022 11:26:29 +0300: > > > > > >On 10.12.22 10:52, Maxim Kiselev wrote: > > > >> > > > >> After applying > > > > > > > >This makes me wonder: "applying" as in "applying it to some version that > > > >doesn't contain this change normally" or as it "after it was applied to > > > >mainline I have the following problem with vanilla kernel version <???>"? > > > > > > Sorry for confusing you, I mean "after it was applied to mainline". > > > I have this problem with vanilla kernel version 6.0. > > > > > > >>> I faced with a problem that my ethernet device can't be probed because it > > > >>> wait when 'nvmem-cells' device will be probed first. > > > >> > > > >>FWIW, there is a discussion about a problems that at least to my > > > >>untrained eyes looks similar: > > > >>https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ > > > > > > Yes it looks like the same issue. > > > > > > > > > I think the root of the problem was the choice of 'compatible' > > > device tree property to mark the mtd partition node as a nvmem provider. > > > > > > I'm talking about this part in 'mtd_nvmem_add' function. > > > > config.no_of_node = !of_device_is_compatible(node, "nvmem-cells"); > > > > > > Maybe we should change the 'compatible' property to something else? > > > > At a first glance I don't get why the compatible would matter so much > > here, can you point to some core DT logic that would have an effect? I > > mean besides leading to the creation of a cell. IOW, what would be done > > differently if the compatible was different? > > > > Can you also dump the device links (if you can reach a prompt) from > > sysfs? > > > > In theory there should be a link between ethernet and spi-nor which is > > fulfilled when the spi-nor device probes and leads to the creation of > > device links. Maybe there is "something else" that the mtd core should > > do, because this just works with eeproms (non-mtd cells), so let's find > > out. > > > > > сб, 10 дек. 2022 г. в 15:35, Thorsten Leemhuis <regressions@leemhuis.info>: > > > > > > > > [CCing the regression mailing list, as it should be in the loop for all > > > > regressions, as explained in > > > > https://docs.kernel.org/admin-guide/reporting-regressions.html ] > > > > > > > > Hi, this is your Linux kernel regression tracker. Thx for the report. > > > > > > > > On 10.12.22 10:52, Maxim Kiselev wrote: > > > > > > > > > > After applying > > > > > > > > This makes me wonder: "applying" as in "applying it to some version that > > > > doesn't contain this change normally" or as it "after it was applied to > > > > mainline I have the following problem with vanilla kernel version <???>"? > > > > > > > > > this commit 'mtd: call of_platform_populate() for MTD > > > > > partitions' (bcdf0315), > > > > > > > > CCing Rafał, who authored bcdf0315. > > > > > > > > > I faced with a problem that my ethernet device can't be probed because it > > > > > wait when 'nvmem-cells' device will be probed first. > > > > > > > > FWIW, there is a discussion about a problems that at least to my > > > > untrained eyes looks similar: > > > > https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ > > > > > > > > Rafał, has some progress been made to resolve this? > > > > > > > > To me it sounds like this might warrant a "revert, and reapply later > > > > when the cause for the regression was addressed". Rafał, it seems you > > > > suggested something like that, but it doesn't look like that happened > > > > for one reason or another. Or am I missing something? > > > > > > > > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) > > > > > > > > P.S.: As the Linux kernel's regression tracker I deal with a lot of > > > > reports and sometimes miss something important when writing mails like > > > > this. If that's the case here, don't hesitate to tell me in a public > > > > reply, it's in everyone's interest to set the public record straight. > > > > > > > > > But there is no such driver which is compatible with 'nvmem-cells' because > > > > > 'nvmem-cells' is just a mark used by the 'mtd_nvmem_add' function. > > > > > > > > > > So this leads to appeating of unresolved dependency for the ethernet device. > > > > > And that's why the ethernet device can't be added and probed. > > > > > > > > > > Here is a part of kernel log when spi flash probe start: > > > > > > > > > >> device: 'spi0': device_add > > > > >> device: 'spi0.0': device_add > > > > >> spi-nor spi0.0: mx66l51235f (65536 Kbytes) > > > > >> 7 fixed-partitions partitions found on MTD device spi0.0 > > > > > > > > > > After 'm25p80' probe 'f1070000.ethernet' linked to 'partition@1' : > > > > > > > > > >> device: 'f1010600.spi:m25p80@0: > > > > > partitions:partition@1': device_add > > > > >> device: 'platform:f1010600.spi:m25p80@0:partitions:partition@1--platform:f1070000.ethernet': device_add > > > > >> devices_kset: Moving f1070000.ethernet to end of list > > > > >> platform f1070000.ethernet: Linked as a consumer to f1010600.spi:m25p80@0:partitions:partition@1 > > > > >> ethernet@70000 Dropping the fwnode link to partition@1 > > > > > > > > > > And as a result I got `-EPROBE_DEFER` for `f1070000.ethernet` > > > > > > > > > >> platform f1070000.ethernet: error -EPROBE_DEFER: supplier f1010600.spi:m25p80@0:partitions:partition@1 not ready > > > > > > > > > > Here is a part of my device tree: > > > > > > > > > > enet1: ethernet@70000 { > > > > > status = "okay"; > > > > > nvmem-cells = <&macaddr>; > > > > > nvmem-cell-names = "mac-address"; > > > > > phy-mode = "rgmii"; > > > > > phy = <&phy0>; > > > > > }; > > > > > > > > > > spi@10600 { > > > > > status = "okay"; > > > > > > > > > > m25p80@0 { > > > > > compatible = "mx66l51235l"; > > > > > reg = <0>; > > > > > #address-cells = <1>; > > > > > #size-cells = <1>; > > > > > > > > > > partitions { > > > > > compatible = "fixed-partitions"; > > > > > #address-cells = <1>; > > > > > #size-cells = <1>; > > > > > > > > > > partition@0 { > > > > > reg = <0x00000000 0x000080000>; > > > > > label = "SPI.U_BOOT"; > > > > > }; > > > > > > > > > > partition@1 { > > > > > compatible = "nvmem-cells"; > > > > > reg = <0x000A0000 0x00020000>; > > > > > label = "SPI.INV_INFO"; > > > > > #address-cells = <1>; > > > > > #size-cells = <1>; > > > > > ranges = <0 0x000A0000 0x00020000>; > > > > > > > > > > macaddr: mac@6 { > > > > > reg = <0x6 0x6>; > > > > > }; > > > > > }; > > > > > > > > > > }; > > > > > }; > > > > > }; > > > > > > > > > > In the example above 'ethernet@70000' requires 'macaddr: mac@6' which is > > > > > located inside mtd 'partition@1' of 'm25p80@0' spi flash. > > > > > > > > P.P.S.: let me add this to the regression tracking: > > > > > > > > #regzbot ^introduced bcdf0315 > > > > #regzbot title mtd: ethernet device can't be probed anymore due to > > > > broken nvmem-cells dep > > > > #regzbot monitor: https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ > > > > #regzbot ignore-activity > > > > > > Thanks, > > Miquèl ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-12 16:37 ` Miquel Raynal @ 2022-12-12 17:57 ` Maxim Kochetkov 2022-12-13 9:46 ` Miquel Raynal 0 siblings, 1 reply; 17+ messages in thread From: Maxim Kochetkov @ 2022-12-12 17:57 UTC (permalink / raw) To: Miquel Raynal, Maxim Kiselev Cc: Thorsten Leemhuis, linux-mtd, linux-kernel, Vignesh Raghavendra, Richard Weinberger, Rafał Miłecki, regressions, Rob Herring, Krzysztof Kozlowski, Greg Kroah-Hartman, Rafael J. Wysocki, Saravana Kannan 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? Thanks, Maxim. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-12 17:57 ` Maxim Kochetkov @ 2022-12-13 9:46 ` Miquel Raynal 2022-12-13 11:02 ` Maxim Kiselev 0 siblings, 1 reply; 17+ messages in thread From: Miquel Raynal @ 2022-12-13 9:46 UTC (permalink / raw) To: Maxim Kochetkov Cc: Maxim Kiselev, Thorsten Leemhuis, linux-mtd, linux-kernel, Vignesh Raghavendra, Richard Weinberger, Rafał Miłecki, regressions, Rob Herring, Krzysztof Kozlowski, Greg Kroah-Hartman, Rafael J. Wysocki, Saravana Kannan 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-13 9:46 ` Miquel Raynal @ 2022-12-13 11:02 ` Maxim Kiselev 2022-12-13 16:54 ` Miquel Raynal 0 siblings, 1 reply; 17+ messages in thread From: Maxim Kiselev @ 2022-12-13 11:02 UTC (permalink / raw) To: Miquel Raynal Cc: Maxim Kochetkov, Thorsten Leemhuis, linux-mtd, linux-kernel, Vignesh Raghavendra, Richard Weinberger, Rafał Miłecki, regressions, Rob Herring, Krzysztof Kozlowski, Greg Kroah-Hartman, Rafael J. Wysocki, Saravana Kannan 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 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-13 11:02 ` Maxim Kiselev @ 2022-12-13 16:54 ` Miquel Raynal 2022-12-14 21:53 ` Saravana Kannan 0 siblings, 1 reply; 17+ messages in thread From: Miquel Raynal @ 2022-12-13 16:54 UTC (permalink / raw) To: Maxim Kiselev Cc: Maxim Kochetkov, Thorsten Leemhuis, linux-mtd, linux-kernel, Vignesh Raghavendra, Richard Weinberger, Rafał Miłecki, regressions, Rob Herring, Krzysztof Kozlowski, Greg Kroah-Hartman, Rafael J. Wysocki, Saravana Kannan 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-13 16:54 ` Miquel Raynal @ 2022-12-14 21:53 ` Saravana Kannan 2022-12-16 11:04 ` Miquel Raynal 0 siblings, 1 reply; 17+ messages in thread From: Saravana Kannan @ 2022-12-14 21:53 UTC (permalink / raw) To: Miquel Raynal Cc: Maxim Kiselev, Maxim Kochetkov, Thorsten Leemhuis, linux-mtd, linux-kernel, Vignesh Raghavendra, Richard Weinberger, Rafał Miłecki, regressions, Rob Herring, Krzysztof Kozlowski, Greg Kroah-Hartman, Rafael J. Wysocki 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. 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. 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). [1] - https://lore.kernel.org/lkml/20220810060040.321697-1-saravanak@google.com/ Thanks, Saravana ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-14 21:53 ` Saravana Kannan @ 2022-12-16 11:04 ` Miquel Raynal 2022-12-16 11:33 ` Maxim Kiselev 2022-12-17 1:45 ` Saravana Kannan 0 siblings, 2 replies; 17+ messages in thread From: Miquel Raynal @ 2022-12-16 11:04 UTC (permalink / raw) To: Saravana Kannan Cc: Maxim Kiselev, Maxim Kochetkov, Thorsten Leemhuis, linux-mtd, linux-kernel, Vignesh Raghavendra, Richard Weinberger, Rafał Miłecki, regressions, Rob Herring, Krzysztof Kozlowski, Greg Kroah-Hartman, Rafael J. Wysocki 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-16 11:04 ` Miquel Raynal @ 2022-12-16 11:33 ` Maxim Kiselev 2022-12-16 13:13 ` Maxim Kiselev 2022-12-17 1:45 ` Saravana Kannan 1 sibling, 1 reply; 17+ messages in thread From: Maxim Kiselev @ 2022-12-16 11:33 UTC (permalink / raw) To: Miquel Raynal Cc: Saravana Kannan, Maxim Kochetkov, Thorsten Leemhuis, linux-mtd, linux-kernel, Vignesh Raghavendra, Richard Weinberger, Rafał Miłecki, regressions, Rob Herring, Krzysztof Kozlowski, Greg Kroah-Hartman, Rafael J. Wysocki > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-16 11:33 ` Maxim Kiselev @ 2022-12-16 13:13 ` Maxim Kiselev 2022-12-17 1:44 ` Saravana Kannan 0 siblings, 1 reply; 17+ messages in thread From: Maxim Kiselev @ 2022-12-16 13:13 UTC (permalink / raw) To: Miquel Raynal Cc: Saravana Kannan, Maxim Kochetkov, Thorsten Leemhuis, linux-mtd, linux-kernel, Vignesh Raghavendra, Richard Weinberger, Rafał Miłecki, regressions, Rob Herring, Krzysztof Kozlowski, Greg Kroah-Hartman, Rafael J. Wysocki > 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. 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-16 13:13 ` Maxim Kiselev @ 2022-12-17 1:44 ` Saravana Kannan 0 siblings, 0 replies; 17+ messages in thread From: Saravana Kannan @ 2022-12-17 1:44 UTC (permalink / raw) To: Maxim Kiselev Cc: Miquel Raynal, Maxim Kochetkov, Thorsten Leemhuis, linux-mtd, linux-kernel, Vignesh Raghavendra, Richard Weinberger, Rafał Miłecki, regressions, Rob Herring, Krzysztof Kozlowski, Greg Kroah-Hartman, Rafael J. Wysocki 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-16 11:04 ` Miquel Raynal 2022-12-16 11:33 ` Maxim Kiselev @ 2022-12-17 1:45 ` Saravana Kannan 1 sibling, 0 replies; 17+ messages in thread From: Saravana Kannan @ 2022-12-17 1:45 UTC (permalink / raw) To: Miquel Raynal Cc: Maxim Kiselev, Maxim Kochetkov, Thorsten Leemhuis, linux-mtd, linux-kernel, Vignesh Raghavendra, Richard Weinberger, Rafał Miłecki, regressions, Rob Herring, Krzysztof Kozlowski, Greg Kroah-Hartman, Rafael J. Wysocki On Fri, Dec 16, 2022 at 3:05 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > 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> Will do. -Saravana > > > [1] - https://lore.kernel.org/lkml/20220810060040.321697-1-saravanak@google.com/ > > Maxim, any chance you give this a try? > > Thanks, > Miquèl ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Fwd: nvmem-cells regression after adding 'call of_platform_populate() for MTD partitions' 2022-12-10 12:35 ` Thorsten Leemhuis 2022-12-11 8:26 ` Maxim Kiselev @ 2023-02-24 16:15 ` Linux regression tracking #update (Thorsten Leemhuis) 1 sibling, 0 replies; 17+ messages in thread From: Linux regression tracking #update (Thorsten Leemhuis) @ 2023-02-24 16:15 UTC (permalink / raw) To: Maxim Kiselev, linux-mtd Cc: linux-kernel, Vignesh Raghavendra, Richard Weinberger, Miquel Raynal, Maxim Kochetkov, Rafał Miłecki, regressions [TLDR: This mail in primarily relevant for Linux regression tracking. A change or fix related to the regression discussed in this thread was posted or applied, but it did not use a Link: tag to point to the report, as Linus and the documentation call for. Things happen, no worries -- but now the regression tracking bot needs to be told manually about the fix. See link in footer if these mails annoy you.] On 10.12.22 13:35, Thorsten Leemhuis wrote: > [...] > P.P.S.: let me add this to the regression tracking: > > #regzbot ^introduced bcdf0315 > #regzbot title mtd: ethernet device can't be probed anymore due to > broken nvmem-cells dep > #regzbot monitor: https://lore.kernel.org/all/Yyj7wJlqJkCwObRn@lx2k/ > #regzbot ignore-activity #regzbot fix: mtd: mtdpart: Don't create platform device that'll never probe #regzbot ignore-activity Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr That page also explains what to do if mails like this annoy you. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-02-24 16:15 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [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-11 8:26 ` Maxim Kiselev 2022-12-12 9:14 ` Miquel Raynal 2022-12-12 13:06 ` Maxim Kiselev 2022-12-12 16:37 ` Miquel Raynal 2022-12-12 17:57 ` Maxim Kochetkov 2022-12-13 9:46 ` Miquel Raynal 2022-12-13 11:02 ` Maxim Kiselev 2022-12-13 16:54 ` Miquel Raynal 2022-12-14 21:53 ` Saravana Kannan 2022-12-16 11:04 ` Miquel Raynal 2022-12-16 11:33 ` Maxim Kiselev 2022-12-16 13:13 ` Maxim Kiselev 2022-12-17 1:44 ` Saravana Kannan 2022-12-17 1:45 ` Saravana Kannan 2023-02-24 16:15 ` Fwd: " Linux regression tracking #update (Thorsten Leemhuis)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).