| Message ID | 20230510110835.26115-7-AVKrasnov@sberdevices.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refactoring and fix for Meson NAND | expand |
Hello Arseniy, On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov <AVKrasnov@sberdevices.ru> wrote: > > This renames node with values for chip select from "reg" to "cs". It is > needed because when OTP access is enabled on the attached storage, MTD > subsystem registers this storage in the NVMEM subsystem. NVMEM in turn > tries to use "reg" node in its own manner, supposes that it has another > layout. All of this leads to device initialization failure. In general: if we change the device-tree interface (in this case: replacing a "reg" with a "cs" property) the dt-bindings have to be updated as well. Documentation/devicetree/bindings/mtd/nand-controller.yaml and Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show that the chip select of a NAND chip is specified with a "reg" property. Also the code has to be backwards compatible with old .dtbs. > Example: > > [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/... > [...] mtd mtd0: Failed to register OTP NVMEM device > [...] meson-nand ffe07800.nfc: failed to register MTD device: -22 > [...] meson-nand ffe07800.nfc: failed to init NAND chips > [...] meson-nand: probe of ffe07800.nfc failed with error -22 This is odd - can you please share your definition of the &nfc node? &nfc { nand_chip0: nand@0 { reg = <0>; }; }; This should result in nand_set_flash_node() being called with &nand_chip0 (if it's called with &nfc then something is buggy in our driver). If there's no child nodes within &nand_chip0 then why would the MTD-to-NVMEM code think that it has to parse something? If you do have child nodes and those are partitions, then make sure that the structure is correct (see the extra "partitions" node inside which all partitions are nested): &nand_chip0 { partitions { compatible = "fixed-partitions"; #address-cells = <1>; #size-cells = <1>; partition@0 { label = "u-boot"; reg = <0x0000000 0x4000>; read-only; }; }; }; Best regards,, Martin
Hi Martin & Arseniy, martin.blumenstingl@googlemail.com wrote on Wed, 10 May 2023 22:40:37 +0200: > Hello Arseniy, > > On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov > <AVKrasnov@sberdevices.ru> wrote: > > > > This renames node with values for chip select from "reg" to "cs". It is > > needed because when OTP access is enabled on the attached storage, MTD > > subsystem registers this storage in the NVMEM subsystem. NVMEM in turn > > tries to use "reg" node in its own manner, supposes that it has another > > layout. All of this leads to device initialization failure. > In general: if we change the device-tree interface (in this case: > replacing a "reg" with a "cs" property) the dt-bindings have to be > updated as well. True, and I would add, bindings should not be broken. > Documentation/devicetree/bindings/mtd/nand-controller.yaml and > Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show > that the chip select of a NAND chip is specified with a "reg" > property. All NAND controller binding expect the chip-select to be in the 'reg' property, very much like a spi device would use reg to store the cs as well: the reg property tells you how you address the device. I also fully agree with Martin's comments below. Changing reg is likely a wrong approach :) > Also the code has to be backwards compatible with old .dtbs. > > > Example: > > > > [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/... > > [...] mtd mtd0: Failed to register OTP NVMEM device > > [...] meson-nand ffe07800.nfc: failed to register MTD device: -22 > > [...] meson-nand ffe07800.nfc: failed to init NAND chips > > [...] meson-nand: probe of ffe07800.nfc failed with error -22 > This is odd - can you please share your definition of the &nfc node? > > &nfc { > nand_chip0: nand@0 { > reg = <0>; > }; > }; > > This should result in nand_set_flash_node() being called with > &nand_chip0 (if it's called with &nfc then something is buggy in our > driver). > If there's no child nodes within &nand_chip0 then why would the > MTD-to-NVMEM code think that it has to parse something? > If you do have child nodes and those are partitions, then make sure > that the structure is correct (see the extra "partitions" node inside > which all partitions are nested): > &nand_chip0 { > partitions { > compatible = "fixed-partitions"; > #address-cells = <1>; > #size-cells = <1>; > > partition@0 { > label = "u-boot"; > reg = <0x0000000 0x4000>; > read-only; > }; > }; > }; > > > Best regards,, > Martin Thanks, Miquèl
On 10.05.2023 23:53, Miquel Raynal wrote: Hello Martin, Miquel > Hi Martin & Arseniy, > > martin.blumenstingl@googlemail.com wrote on Wed, 10 May 2023 22:40:37 > +0200: > >> Hello Arseniy, >> >> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov >> <AVKrasnov@sberdevices.ru> wrote: >>> >>> This renames node with values for chip select from "reg" to "cs". It is >>> needed because when OTP access is enabled on the attached storage, MTD >>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn >>> tries to use "reg" node in its own manner, supposes that it has another >>> layout. All of this leads to device initialization failure. >> In general: if we change the device-tree interface (in this case: >> replacing a "reg" with a "cs" property) the dt-bindings have to be >> updated as well. > > True, and I would add, bindings should not be broken. I see, that's true. That is bad way to change bindings. > >> Documentation/devicetree/bindings/mtd/nand-controller.yaml and >> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show >> that the chip select of a NAND chip is specified with a "reg" >> property. > > All NAND controller binding expect the chip-select to be in the > 'reg' property, very much like a spi device would use reg to store the > cs as well: the reg property tells you how you address the device. > > I also fully agree with Martin's comments below. Changing reg is likely > a wrong approach :) > >> Also the code has to be backwards compatible with old .dtbs. >> >>> Example: >>> >>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/... >>> [...] mtd mtd0: Failed to register OTP NVMEM device >>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22 >>> [...] meson-nand ffe07800.nfc: failed to init NAND chips >>> [...] meson-nand: probe of ffe07800.nfc failed with error -22 >> This is odd - can you please share your definition of the &nfc node? Sure, here it is: mtd_nand: nfc@7800 { compatible = "amlogic,meson-axg-nfc"; ... nand@0 { reg = <0>; }; } I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ? >> >> &nfc { >> nand_chip0: nand@0 { >> reg = <0>; >> }; >> }; >> >> This should result in nand_set_flash_node() being called with >> &nand_chip0 (if it's called with &nfc then something is buggy in our >> driver). >> If there's no child nodes within &nand_chip0 then why would the >> MTD-to-NVMEM code think that it has to parse something? >> If you do have child nodes and those are partitions, then make sure >> that the structure is correct (see the extra "partitions" node inside >> which all partitions are nested): >> &nand_chip0 { >> partitions { >> compatible = "fixed-partitions"; >> #address-cells = <1>; >> #size-cells = <1>; >> >> partition@0 { >> label = "u-boot"; >> reg = <0x0000000 0x4000>; >> read-only; >> }; >> }; >> }; No, partition nodes are disabled in this case. Thanks, Arseniy >> >> >> Best regards,, >> Martin > > > Thanks, > Miquèl
Hi Arseniy, avkrasnov@sberdevices.ru wrote on Thu, 11 May 2023 11:59:07 +0300: > On 10.05.2023 23:53, Miquel Raynal wrote: > > Hello Martin, Miquel > > > Hi Martin & Arseniy, > > > > martin.blumenstingl@googlemail.com wrote on Wed, 10 May 2023 22:40:37 > > +0200: > > > >> Hello Arseniy, > >> > >> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov > >> <AVKrasnov@sberdevices.ru> wrote: > >>> > >>> This renames node with values for chip select from "reg" to "cs". It is > >>> needed because when OTP access is enabled on the attached storage, MTD > >>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn > >>> tries to use "reg" node in its own manner, supposes that it has another > >>> layout. All of this leads to device initialization failure. > >> In general: if we change the device-tree interface (in this case: > >> replacing a "reg" with a "cs" property) the dt-bindings have to be > >> updated as well. > > > > True, and I would add, bindings should not be broken. > > I see, that's true. That is bad way to change bindings. > > > > >> Documentation/devicetree/bindings/mtd/nand-controller.yaml and > >> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show > >> that the chip select of a NAND chip is specified with a "reg" > >> property. > > > > All NAND controller binding expect the chip-select to be in the > > 'reg' property, very much like a spi device would use reg to store the > > cs as well: the reg property tells you how you address the device. > > > > I also fully agree with Martin's comments below. Changing reg is likely > > a wrong approach :) > > > >> Also the code has to be backwards compatible with old .dtbs. > >> > >>> Example: > >>> > >>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/... > >>> [...] mtd mtd0: Failed to register OTP NVMEM device > >>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22 > >>> [...] meson-nand ffe07800.nfc: failed to init NAND chips > >>> [...] meson-nand: probe of ffe07800.nfc failed with error -22 > >> This is odd - can you please share your definition of the &nfc node? > > Sure, here it is: > > mtd_nand: nfc@7800 { > compatible = "amlogic,meson-axg-nfc"; > ... > nand@0 { > reg = <0>; > }; > } > > I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose > that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called > with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such > situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ? We recently had issues with nvmem parsing, but I believe a mainline kernel should now be perfectly working on this regard. What version of the Linux kernel are you using? Thanks, Miquèl
On 11.05.2023 12:12, Miquel Raynal wrote: > Hi Arseniy, > > avkrasnov@sberdevices.ru wrote on Thu, 11 May 2023 11:59:07 +0300: > >> On 10.05.2023 23:53, Miquel Raynal wrote: >> >> Hello Martin, Miquel >> >>> Hi Martin & Arseniy, >>> >>> martin.blumenstingl@googlemail.com wrote on Wed, 10 May 2023 22:40:37 >>> +0200: >>> >>>> Hello Arseniy, >>>> >>>> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov >>>> <AVKrasnov@sberdevices.ru> wrote: >>>>> >>>>> This renames node with values for chip select from "reg" to "cs". It is >>>>> needed because when OTP access is enabled on the attached storage, MTD >>>>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn >>>>> tries to use "reg" node in its own manner, supposes that it has another >>>>> layout. All of this leads to device initialization failure. >>>> In general: if we change the device-tree interface (in this case: >>>> replacing a "reg" with a "cs" property) the dt-bindings have to be >>>> updated as well. >>> >>> True, and I would add, bindings should not be broken. >> >> I see, that's true. That is bad way to change bindings. >> >>> >>>> Documentation/devicetree/bindings/mtd/nand-controller.yaml and >>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show >>>> that the chip select of a NAND chip is specified with a "reg" >>>> property. >>> >>> All NAND controller binding expect the chip-select to be in the >>> 'reg' property, very much like a spi device would use reg to store the >>> cs as well: the reg property tells you how you address the device. >>> >>> I also fully agree with Martin's comments below. Changing reg is likely >>> a wrong approach :) >>> >>>> Also the code has to be backwards compatible with old .dtbs. >>>> >>>>> Example: >>>>> >>>>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/... >>>>> [...] mtd mtd0: Failed to register OTP NVMEM device >>>>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22 >>>>> [...] meson-nand ffe07800.nfc: failed to init NAND chips >>>>> [...] meson-nand: probe of ffe07800.nfc failed with error -22 >>>> This is odd - can you please share your definition of the &nfc node? >> >> Sure, here it is: >> >> mtd_nand: nfc@7800 { >> compatible = "amlogic,meson-axg-nfc"; >> ... >> nand@0 { >> reg = <0>; >> }; >> } >> >> I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose >> that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called >> with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such >> situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ? > > We recently had issues with nvmem parsing, but I believe a mainline > kernel should now be perfectly working on this regard. What version of > the Linux kernel are you using? My current version is: VERSION = 6 PATCHLEVEL = 2 SUBLEVEL = 0 EXTRAVERSION = -rc8 Fix was in drivers/nvmem/* ? Thanks, Arseniy > > Thanks, > Miquèl
On 11.05.2023 12:17, Arseniy Krasnov wrote: > > > On 11.05.2023 12:12, Miquel Raynal wrote: >> Hi Arseniy, >> >> avkrasnov@sberdevices.ru wrote on Thu, 11 May 2023 11:59:07 +0300: >> >>> On 10.05.2023 23:53, Miquel Raynal wrote: >>> >>> Hello Martin, Miquel >>> >>>> Hi Martin & Arseniy, >>>> >>>> martin.blumenstingl@googlemail.com wrote on Wed, 10 May 2023 22:40:37 >>>> +0200: >>>> >>>>> Hello Arseniy, >>>>> >>>>> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov >>>>> <AVKrasnov@sberdevices.ru> wrote: >>>>>> >>>>>> This renames node with values for chip select from "reg" to "cs". It is >>>>>> needed because when OTP access is enabled on the attached storage, MTD >>>>>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn >>>>>> tries to use "reg" node in its own manner, supposes that it has another >>>>>> layout. All of this leads to device initialization failure. >>>>> In general: if we change the device-tree interface (in this case: >>>>> replacing a "reg" with a "cs" property) the dt-bindings have to be >>>>> updated as well. >>>> >>>> True, and I would add, bindings should not be broken. >>> >>> I see, that's true. That is bad way to change bindings. >>> >>>> >>>>> Documentation/devicetree/bindings/mtd/nand-controller.yaml and >>>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show >>>>> that the chip select of a NAND chip is specified with a "reg" >>>>> property. >>>> >>>> All NAND controller binding expect the chip-select to be in the >>>> 'reg' property, very much like a spi device would use reg to store the >>>> cs as well: the reg property tells you how you address the device. >>>> >>>> I also fully agree with Martin's comments below. Changing reg is likely >>>> a wrong approach :) >>>> >>>>> Also the code has to be backwards compatible with old .dtbs. >>>>> >>>>>> Example: >>>>>> >>>>>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/... >>>>>> [...] mtd mtd0: Failed to register OTP NVMEM device >>>>>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22 >>>>>> [...] meson-nand ffe07800.nfc: failed to init NAND chips >>>>>> [...] meson-nand: probe of ffe07800.nfc failed with error -22 >>>>> This is odd - can you please share your definition of the &nfc node? >>> >>> Sure, here it is: >>> >>> mtd_nand: nfc@7800 { >>> compatible = "amlogic,meson-axg-nfc"; >>> ... >>> nand@0 { >>> reg = <0>; >>> }; >>> } >>> >>> I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose >>> that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called >>> with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such >>> situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ? >> >> We recently had issues with nvmem parsing, but I believe a mainline >> kernel should now be perfectly working on this regard. What version of >> the Linux kernel are you using? > > My current version is: > > VERSION = 6 > PATCHLEVEL = 2 > SUBLEVEL = 0 > EXTRAVERSION = -rc8 > > Fix was in drivers/nvmem/* ? > > Thanks, Arseniy Upd: I resolved problem in the following way: nand@0 { reg = <0>;//chip select otp@0 { #address-cells = <2>; #size-cells = <0>; compatible = "user-otp"; reg = <A B>; }; otp@1 { #address-cells = <2>; #size-cells = <0>; compatible = "factory-otp"; reg = <C D>; }; }; Now nvmem subsystem parses 'otp@0' and 'otp@1' and error is gone. 'compatible' values are the same as in drivers/mtd/mtdcore.c:mtd_otp_nvmem_add(). 'reg' in 'nand@0' is used as chip select as supposed. I think, this patch should be abandoned in the next version. Thanks, Arseniy > >> >> Thanks, >> Miquèl
Hi Arseniy, avkrasnov@sberdevices.ru wrote on Thu, 11 May 2023 13:16:59 +0300: > On 11.05.2023 12:17, Arseniy Krasnov wrote: > > > > > > On 11.05.2023 12:12, Miquel Raynal wrote: > >> Hi Arseniy, > >> > >> avkrasnov@sberdevices.ru wrote on Thu, 11 May 2023 11:59:07 +0300: > >> > >>> On 10.05.2023 23:53, Miquel Raynal wrote: > >>> > >>> Hello Martin, Miquel > >>> > >>>> Hi Martin & Arseniy, > >>>> > >>>> martin.blumenstingl@googlemail.com wrote on Wed, 10 May 2023 22:40:37 > >>>> +0200: > >>>> > >>>>> Hello Arseniy, > >>>>> > >>>>> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov > >>>>> <AVKrasnov@sberdevices.ru> wrote: > >>>>>> > >>>>>> This renames node with values for chip select from "reg" to "cs". It is > >>>>>> needed because when OTP access is enabled on the attached storage, MTD > >>>>>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn > >>>>>> tries to use "reg" node in its own manner, supposes that it has another > >>>>>> layout. All of this leads to device initialization failure. > >>>>> In general: if we change the device-tree interface (in this case: > >>>>> replacing a "reg" with a "cs" property) the dt-bindings have to be > >>>>> updated as well. > >>>> > >>>> True, and I would add, bindings should not be broken. > >>> > >>> I see, that's true. That is bad way to change bindings. > >>> > >>>> > >>>>> Documentation/devicetree/bindings/mtd/nand-controller.yaml and > >>>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show > >>>>> that the chip select of a NAND chip is specified with a "reg" > >>>>> property. > >>>> > >>>> All NAND controller binding expect the chip-select to be in the > >>>> 'reg' property, very much like a spi device would use reg to store the > >>>> cs as well: the reg property tells you how you address the device. > >>>> > >>>> I also fully agree with Martin's comments below. Changing reg is likely > >>>> a wrong approach :) > >>>> > >>>>> Also the code has to be backwards compatible with old .dtbs. > >>>>> > >>>>>> Example: > >>>>>> > >>>>>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/... > >>>>>> [...] mtd mtd0: Failed to register OTP NVMEM device > >>>>>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22 > >>>>>> [...] meson-nand ffe07800.nfc: failed to init NAND chips > >>>>>> [...] meson-nand: probe of ffe07800.nfc failed with error -22 > >>>>> This is odd - can you please share your definition of the &nfc node? > >>> > >>> Sure, here it is: > >>> > >>> mtd_nand: nfc@7800 { > >>> compatible = "amlogic,meson-axg-nfc"; > >>> ... > >>> nand@0 { > >>> reg = <0>; > >>> }; > >>> } > >>> > >>> I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose > >>> that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called > >>> with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such > >>> situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ? > >> > >> We recently had issues with nvmem parsing, but I believe a mainline > >> kernel should now be perfectly working on this regard. What version of > >> the Linux kernel are you using? > > > > My current version is: > > > > VERSION = 6 > > PATCHLEVEL = 2 > > SUBLEVEL = 0 > > EXTRAVERSION = -rc8 > > > > Fix was in drivers/nvmem/* ? > > > > Thanks, Arseniy > > Upd: I resolved problem in the following way: > > nand@0 { > reg = <0>;//chip select > partitions { compatible = ... > otp@0 { > #address-cells = <2>; > #size-cells = <0>; #address/size-cells is not needed here > compatible = "user-otp"; > reg = <A B>; > }; > otp@1 { > #address-cells = <2>; > #size-cells = <0>; Ditto > compatible = "factory-otp"; > reg = <C D>; > }; }; > }; > > Now nvmem subsystem parses 'otp@0' and 'otp@1' and error is gone. 'compatible' values are > the same as in drivers/mtd/mtdcore.c:mtd_otp_nvmem_add(). 'reg' in 'nand@0' is used as > chip select as supposed. I don't fully get it. The parsing on the nvmem side should not fail if there is no subpartition/otp-region defined. Can you confirm an empty NAND device node works? Because your last e-mail suggested the opposite. > > I think, this patch should be abandoned in the next version. > > Thanks, Arseniy > > > > >> > >> Thanks, > >> Miquèl Thanks, Miquèl
On 11.05.2023 15:11, Miquel Raynal wrote: > Hi Arseniy, > > avkrasnov@sberdevices.ru wrote on Thu, 11 May 2023 13:16:59 +0300: > >> On 11.05.2023 12:17, Arseniy Krasnov wrote: >>> >>> >>> On 11.05.2023 12:12, Miquel Raynal wrote: >>>> Hi Arseniy, >>>> >>>> avkrasnov@sberdevices.ru wrote on Thu, 11 May 2023 11:59:07 +0300: >>>> >>>>> On 10.05.2023 23:53, Miquel Raynal wrote: >>>>> >>>>> Hello Martin, Miquel >>>>> >>>>>> Hi Martin & Arseniy, >>>>>> >>>>>> martin.blumenstingl@googlemail.com wrote on Wed, 10 May 2023 22:40:37 >>>>>> +0200: >>>>>> >>>>>>> Hello Arseniy, >>>>>>> >>>>>>> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov >>>>>>> <AVKrasnov@sberdevices.ru> wrote: >>>>>>>> >>>>>>>> This renames node with values for chip select from "reg" to "cs". It is >>>>>>>> needed because when OTP access is enabled on the attached storage, MTD >>>>>>>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn >>>>>>>> tries to use "reg" node in its own manner, supposes that it has another >>>>>>>> layout. All of this leads to device initialization failure. >>>>>>> In general: if we change the device-tree interface (in this case: >>>>>>> replacing a "reg" with a "cs" property) the dt-bindings have to be >>>>>>> updated as well. >>>>>> >>>>>> True, and I would add, bindings should not be broken. >>>>> >>>>> I see, that's true. That is bad way to change bindings. >>>>> >>>>>> >>>>>>> Documentation/devicetree/bindings/mtd/nand-controller.yaml and >>>>>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show >>>>>>> that the chip select of a NAND chip is specified with a "reg" >>>>>>> property. >>>>>> >>>>>> All NAND controller binding expect the chip-select to be in the >>>>>> 'reg' property, very much like a spi device would use reg to store the >>>>>> cs as well: the reg property tells you how you address the device. >>>>>> >>>>>> I also fully agree with Martin's comments below. Changing reg is likely >>>>>> a wrong approach :) >>>>>> >>>>>>> Also the code has to be backwards compatible with old .dtbs. >>>>>>> >>>>>>>> Example: >>>>>>>> >>>>>>>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/... >>>>>>>> [...] mtd mtd0: Failed to register OTP NVMEM device >>>>>>>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22 >>>>>>>> [...] meson-nand ffe07800.nfc: failed to init NAND chips >>>>>>>> [...] meson-nand: probe of ffe07800.nfc failed with error -22 >>>>>>> This is odd - can you please share your definition of the &nfc node? >>>>> >>>>> Sure, here it is: >>>>> >>>>> mtd_nand: nfc@7800 { >>>>> compatible = "amlogic,meson-axg-nfc"; >>>>> ... >>>>> nand@0 { >>>>> reg = <0>; >>>>> }; >>>>> } >>>>> >>>>> I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose >>>>> that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called >>>>> with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such >>>>> situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ? >>>> >>>> We recently had issues with nvmem parsing, but I believe a mainline >>>> kernel should now be perfectly working on this regard. What version of >>>> the Linux kernel are you using? >>> >>> My current version is: >>> >>> VERSION = 6 >>> PATCHLEVEL = 2 >>> SUBLEVEL = 0 >>> EXTRAVERSION = -rc8 >>> >>> Fix was in drivers/nvmem/* ? >>> >>> Thanks, Arseniy >> >> Upd: I resolved problem in the following way: >> >> nand@0 { >> reg = <0>;//chip select >> > partitions { > compatible = ... > >> otp@0 { >> #address-cells = <2>; >> #size-cells = <0>; > > #address/size-cells is not needed here > >> compatible = "user-otp"; >> reg = <A B>; >> }; >> otp@1 { >> #address-cells = <2>; >> #size-cells = <0>; > > Ditto > >> compatible = "factory-otp"; >> reg = <C D>; >> }; > > }; > >> }; >> >> Now nvmem subsystem parses 'otp@0' and 'otp@1' and error is gone. 'compatible' values are >> the same as in drivers/mtd/mtdcore.c:mtd_otp_nvmem_add(). 'reg' in 'nand@0' is used as >> chip select as supposed. > > I don't fully get it. The parsing on the nvmem side should not fail if > there is no subpartition/otp-region defined. Can you confirm an empty > NAND device node works? Because your last e-mail suggested the opposite. Ok, so i'll describe what happens in my case. Let's NAND node be like this (IIUC this is considered as empty NAND device): mtd_nand: nfc@7800 { compatible = "amlogic,meson-axg-nfc"; ... nand@0 { reg = <0>; }; } I see, that 1) 'mtd_otp_nvmem_add()' calls 'mtd_otp_nvmem_register()' twice for two types of OTP memory "user-otp" and "factory-otp". Let's take a look only on "user-otp". 2) 'mtd_otp_nvmem_register()' tries to lookup for node in 'nand@0' which is compatible with "user-otp" and then passes found (or not found, e.g. NULL) node to 'nvmem_register()'. 3) 'nvmem_register()' uses this node iterating over its childs and searching value "reg" in each child. If "user-otp" node is not found in 2), 'nvmem_register()' uses node 'nfc@7800' also looking for "reg" value in each of its child. In this case it found "reg" in 'nand@0' and fails. Now, if i add "compatible = "user-otp";" to 'nand@0', in step 2) search will be successful, and "reg" value will be used from this new node (or we remove "reg" from it - nothing happens as You wrote). So, problem is that nvmem tries to parse node with invalid "reg" value. Also I see, that 'nvmem_register()' is called earlier in 'mtd_nvmem_add()', but with no effect. I think, that it is not related with enabled OTP feature. Thanks, Arseniy > >> >> I think, this patch should be abandoned in the next version. >> >> Thanks, Arseniy >> >>> >>>> >>>> Thanks, >>>> Miquèl > > > Thanks, > Miquèl
Hi Arseniy, I'm adding Rafał & Michael: any idea what could be wrong? The behavior below does not look expected at all, but I thought we (= Rafał, mainly) already sorted this out? > >>>>>>> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov > >>>>>>> <AVKrasnov@sberdevices.ru> wrote: > >>>>>>>> > >>>>>>>> This renames node with values for chip select from "reg" to "cs". It is > >>>>>>>> needed because when OTP access is enabled on the attached storage, MTD > >>>>>>>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn > >>>>>>>> tries to use "reg" node in its own manner, supposes that it has another > >>>>>>>> layout. All of this leads to device initialization failure. > >>>>>>> In general: if we change the device-tree interface (in this case: > >>>>>>> replacing a "reg" with a "cs" property) the dt-bindings have to be > >>>>>>> updated as well. > >>>>>> > >>>>>> True, and I would add, bindings should not be broken. > >>>>> > >>>>> I see, that's true. That is bad way to change bindings. > >>>>> > >>>>>> > >>>>>>> Documentation/devicetree/bindings/mtd/nand-controller.yaml and > >>>>>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show > >>>>>>> that the chip select of a NAND chip is specified with a "reg" > >>>>>>> property. > >>>>>> > >>>>>> All NAND controller binding expect the chip-select to be in the > >>>>>> 'reg' property, very much like a spi device would use reg to store the > >>>>>> cs as well: the reg property tells you how you address the device. > >>>>>> > >>>>>> I also fully agree with Martin's comments below. Changing reg is likely > >>>>>> a wrong approach :) > >>>>>> > >>>>>>> Also the code has to be backwards compatible with old .dtbs. > >>>>>>> > >>>>>>>> Example: > >>>>>>>> > >>>>>>>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/... > >>>>>>>> [...] mtd mtd0: Failed to register OTP NVMEM device > >>>>>>>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22 > >>>>>>>> [...] meson-nand ffe07800.nfc: failed to init NAND chips > >>>>>>>> [...] meson-nand: probe of ffe07800.nfc failed with error -22 > >>>>>>> This is odd - can you please share your definition of the &nfc node? > >>>>> > >>>>> Sure, here it is: > >>>>> > >>>>> mtd_nand: nfc@7800 { > >>>>> compatible = "amlogic,meson-axg-nfc"; > >>>>> ... > >>>>> nand@0 { > >>>>> reg = <0>; > >>>>> }; > >>>>> } > >>>>> > >>>>> I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose > >>>>> that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called > >>>>> with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such > >>>>> situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ? > >>>> > >>>> We recently had issues with nvmem parsing, but I believe a mainline > >>>> kernel should now be perfectly working on this regard. What version of > >>>> the Linux kernel are you using? > >>> > >>> My current version is: > >>> > >>> VERSION = 6 > >>> PATCHLEVEL = 2 > >>> SUBLEVEL = 0 > >>> EXTRAVERSION = -rc8 > >>> > >>> Fix was in drivers/nvmem/* ? > >>> > >>> Thanks, Arseniy > >> > >> Upd: I resolved problem in the following way: > >> > >> nand@0 { > >> reg = <0>;//chip select > >> > > partitions { > > compatible = ... > > > >> otp@0 { > >> #address-cells = <2>; > >> #size-cells = <0>; > > > > #address/size-cells is not needed here > > > >> compatible = "user-otp"; > >> reg = <A B>; > >> }; > >> otp@1 { > >> #address-cells = <2>; > >> #size-cells = <0>; > > > > Ditto > > > >> compatible = "factory-otp"; > >> reg = <C D>; > >> }; > > > > }; > > > >> }; > >> > >> Now nvmem subsystem parses 'otp@0' and 'otp@1' and error is gone. 'compatible' values are > >> the same as in drivers/mtd/mtdcore.c:mtd_otp_nvmem_add(). 'reg' in 'nand@0' is used as > >> chip select as supposed. > > > > I don't fully get it. The parsing on the nvmem side should not fail if > > there is no subpartition/otp-region defined. Can you confirm an empty > > NAND device node works? Because your last e-mail suggested the opposite. > > Ok, so i'll describe what happens in my case. Let's NAND node be like this (IIUC this is > considered as empty NAND device): > > mtd_nand: nfc@7800 { > compatible = "amlogic,meson-axg-nfc"; > ... > nand@0 { > reg = <0>; > }; > } > > I see, that > > 1) 'mtd_otp_nvmem_add()' calls 'mtd_otp_nvmem_register()' twice for two types of > OTP memory "user-otp" and "factory-otp". Let's take a look only on "user-otp". > 2) 'mtd_otp_nvmem_register()' tries to lookup for node in 'nand@0' which is compatible with > "user-otp" and then passes found (or not found, e.g. NULL) node to 'nvmem_register()'. > 3) 'nvmem_register()' uses this node iterating over its childs and searching value "reg" in > each child. If "user-otp" node is not found in 2), 'nvmem_register()' uses node 'nfc@7800' > also looking for "reg" value in each of its child. In this case it found "reg" in 'nand@0' > and fails. > > Now, if i add "compatible = "user-otp";" to 'nand@0', in step 2) search will be successful, > and "reg" value will be used from this new node (or we remove "reg" from it - nothing happens > as You wrote). So, problem is that nvmem tries to parse node with invalid "reg" value. > > Also I see, that 'nvmem_register()' is called earlier in 'mtd_nvmem_add()', but with no effect. > I think, that it is not related with enabled OTP feature. > > Thanks, Arseniy
On 12.05.2023 17:49, Miquel Raynal wrote: > Hi Arseniy, > > I'm adding Rafał & Michael: any idea what could be wrong? The behavior > below does not look expected at all, but I thought we (= Rafał, mainly) > already sorted this out? > Hi Miquel, thanks for help! just to clarify an expected behaviour: if i have the following layout in the device tree mtd_nand: nfc@7800 { compatible = "amlogic,meson-axg-nfc"; ... nand@0 { reg = <0>; }; } node used by 'nvmem_add_cells_from_of()' must be NULL? or 'nand@0'? I guess, that in above dts I have node 'nfc@7800' in use, because 'mtd_otp_nvmem_register()' uses the following device before passing 'config' to 'nvmem_register()': /* OTP nvmem will be registered on the physical device */ config.dev = mtd->dev.parent; 'mtd->dev.parent' is 'nfc@7800'. May be 'mtd_otp_nvmem_register()' must initialize 'no_of_node' field of 'config' like in 'mtd_nvmem_add()' ? This field is documented as: * @no_of_node: Device should not use the parent's of_node even if it's !NULL. In this case node passed to 'nvmem_add_cells_from_of()' will be NULL. Thanks, Arseniy >>>>>>>>> On Wed, May 10, 2023 at 1:13 PM Arseniy Krasnov >>>>>>>>> <AVKrasnov@sberdevices.ru> wrote: >>>>>>>>>> >>>>>>>>>> This renames node with values for chip select from "reg" to "cs". It is >>>>>>>>>> needed because when OTP access is enabled on the attached storage, MTD >>>>>>>>>> subsystem registers this storage in the NVMEM subsystem. NVMEM in turn >>>>>>>>>> tries to use "reg" node in its own manner, supposes that it has another >>>>>>>>>> layout. All of this leads to device initialization failure. >>>>>>>>> In general: if we change the device-tree interface (in this case: >>>>>>>>> replacing a "reg" with a "cs" property) the dt-bindings have to be >>>>>>>>> updated as well. >>>>>>>> >>>>>>>> True, and I would add, bindings should not be broken. >>>>>>> >>>>>>> I see, that's true. That is bad way to change bindings. >>>>>>> >>>>>>>> >>>>>>>>> Documentation/devicetree/bindings/mtd/nand-controller.yaml and >>>>>>>>> Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml show >>>>>>>>> that the chip select of a NAND chip is specified with a "reg" >>>>>>>>> property. >>>>>>>> >>>>>>>> All NAND controller binding expect the chip-select to be in the >>>>>>>> 'reg' property, very much like a spi device would use reg to store the >>>>>>>> cs as well: the reg property tells you how you address the device. >>>>>>>> >>>>>>>> I also fully agree with Martin's comments below. Changing reg is likely >>>>>>>> a wrong approach :) >>>>>>>> >>>>>>>>> Also the code has to be backwards compatible with old .dtbs. >>>>>>>>> >>>>>>>>>> Example: >>>>>>>>>> >>>>>>>>>> [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/... >>>>>>>>>> [...] mtd mtd0: Failed to register OTP NVMEM device >>>>>>>>>> [...] meson-nand ffe07800.nfc: failed to register MTD device: -22 >>>>>>>>>> [...] meson-nand ffe07800.nfc: failed to init NAND chips >>>>>>>>>> [...] meson-nand: probe of ffe07800.nfc failed with error -22 >>>>>>>>> This is odd - can you please share your definition of the &nfc node? >>>>>>> >>>>>>> Sure, here it is: >>>>>>> >>>>>>> mtd_nand: nfc@7800 { >>>>>>> compatible = "amlogic,meson-axg-nfc"; >>>>>>> ... >>>>>>> nand@0 { >>>>>>> reg = <0>; >>>>>>> }; >>>>>>> } >>>>>>> >>>>>>> I checked, that 'nand_set_flash_node()' is called with 'nand@0' and i suppose >>>>>>> that it is correct (as You mentioned below). But, 'nvmem_add_cells_from_of()' is called >>>>>>> with parent: 'nfc@7800', then it iterates over its childs, e.g. 'nand@0' and thus i get such >>>>>>> situation. I guess, that 'nvmem_add_cells_from_of()' must be called with 'nand@0' ? >>>>>> >>>>>> We recently had issues with nvmem parsing, but I believe a mainline >>>>>> kernel should now be perfectly working on this regard. What version of >>>>>> the Linux kernel are you using? >>>>> >>>>> My current version is: >>>>> >>>>> VERSION = 6 >>>>> PATCHLEVEL = 2 >>>>> SUBLEVEL = 0 >>>>> EXTRAVERSION = -rc8 >>>>> >>>>> Fix was in drivers/nvmem/* ? >>>>> >>>>> Thanks, Arseniy >>>> >>>> Upd: I resolved problem in the following way: >>>> >>>> nand@0 { >>>> reg = <0>;//chip select >>>> >>> partitions { >>> compatible = ... >>> >>>> otp@0 { >>>> #address-cells = <2>; >>>> #size-cells = <0>; >>> >>> #address/size-cells is not needed here >>> >>>> compatible = "user-otp"; >>>> reg = <A B>; >>>> }; >>>> otp@1 { >>>> #address-cells = <2>; >>>> #size-cells = <0>; >>> >>> Ditto >>> >>>> compatible = "factory-otp"; >>>> reg = <C D>; >>>> }; >>> >>> }; >>> >>>> }; >>>> >>>> Now nvmem subsystem parses 'otp@0' and 'otp@1' and error is gone. 'compatible' values are >>>> the same as in drivers/mtd/mtdcore.c:mtd_otp_nvmem_add(). 'reg' in 'nand@0' is used as >>>> chip select as supposed. >>> >>> I don't fully get it. The parsing on the nvmem side should not fail if >>> there is no subpartition/otp-region defined. Can you confirm an empty >>> NAND device node works? Because your last e-mail suggested the opposite. >> >> Ok, so i'll describe what happens in my case. Let's NAND node be like this (IIUC this is >> considered as empty NAND device): >> >> mtd_nand: nfc@7800 { >> compatible = "amlogic,meson-axg-nfc"; >> ... >> nand@0 { >> reg = <0>; >> }; >> } >> >> I see, that >> >> 1) 'mtd_otp_nvmem_add()' calls 'mtd_otp_nvmem_register()' twice for two types of >> OTP memory "user-otp" and "factory-otp". Let's take a look only on "user-otp". >> 2) 'mtd_otp_nvmem_register()' tries to lookup for node in 'nand@0' which is compatible with >> "user-otp" and then passes found (or not found, e.g. NULL) node to 'nvmem_register()'. >> 3) 'nvmem_register()' uses this node iterating over its childs and searching value "reg" in >> each child. If "user-otp" node is not found in 2), 'nvmem_register()' uses node 'nfc@7800' >> also looking for "reg" value in each of its child. In this case it found "reg" in 'nand@0' >> and fails. >> >> Now, if i add "compatible = "user-otp";" to 'nand@0', in step 2) search will be successful, >> and "reg" value will be used from this new node (or we remove "reg" from it - nothing happens >> as You wrote). So, problem is that nvmem tries to parse node with invalid "reg" value. >> >> Also I see, that 'nvmem_register()' is called earlier in 'mtd_nvmem_add()', but with no effect. >> I think, that it is not related with enabled OTP feature. >> >> Thanks, Arseniy
diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c index bc99a098f3ca..28371919a6c5 100644 --- a/drivers/mtd/nand/raw/meson_nand.c +++ b/drivers/mtd/nand/raw/meson_nand.c @@ -1419,7 +1419,7 @@ meson_nfc_nand_chip_init(struct device *dev, int ret, i; u32 tmp, nsels; - nsels = of_property_count_elems_of_size(np, "reg", sizeof(u32)); + nsels = of_property_count_elems_of_size(np, "cs", sizeof(u32)); if (!nsels || nsels > MAX_CE_NUM) { dev_err(dev, "invalid register property size\n"); return -EINVAL; @@ -1433,7 +1433,7 @@ meson_nfc_nand_chip_init(struct device *dev, meson_chip->nsels = nsels; for (i = 0; i < nsels; i++) { - ret = of_property_read_u32_index(np, "reg", i, &tmp); + ret = of_property_read_u32_index(np, "cs", i, &tmp); if (ret) { dev_err(dev, "could not retrieve register property: %d\n", ret);
This renames node with values for chip select from "reg" to "cs". It is needed because when OTP access is enabled on the attached storage, MTD subsystem registers this storage in the NVMEM subsystem. NVMEM in turn tries to use "reg" node in its own manner, supposes that it has another layout. All of this leads to device initialization failure. Example: [...] nvmem mtd0-user-otp: nvmem: invalid reg on /soc/bus@ffe00000/... [...] mtd mtd0: Failed to register OTP NVMEM device [...] meson-nand ffe07800.nfc: failed to register MTD device: -22 [...] meson-nand ffe07800.nfc: failed to init NAND chips [...] meson-nand: probe of ffe07800.nfc failed with error -22 Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> --- drivers/mtd/nand/raw/meson_nand.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)