| Message ID | 1432492349-18517-1-git-send-email-hauke@hauke-m.de |
|---|---|
| State | Not Applicable |
| Headers | show |
On 24 May 2015 at 20:32, Hauke Mehrtens <hauke@hauke-m.de> wrote: > This adds the NAND flash chip description for a standard chip found > connected to this SoC. This makes use of generic Broadcom NAND driver > with the iProc interface. I still wait/hope for Brian's reply in [PATCH 5/7] mtd: brcmnand: add bcma driver
On 2015-05-26 04:20 PM, Rafał Miłecki wrote: > On 24 May 2015 at 20:32, Hauke Mehrtens <hauke@hauke-m.de> wrote: >> This adds the NAND flash chip description for a standard chip found >> connected to this SoC. This makes use of generic Broadcom NAND driver >> with the iProc interface. > > I still wait/hope for Brian's reply in > [PATCH 5/7] mtd: brcmnand: add bcma driver > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > Rafal, How long has it been as if its over a week I would like resend the patch. Nick
On Sun, May 24, 2015 at 08:32:29PM +0200, Hauke Mehrtens wrote: > This adds the NAND flash chip description for a standard chip found > connected to this SoC. This makes use of generic Broadcom NAND driver > with the iProc interface. > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > --- > > This would be the patch when we completely change to device tree only > for the nand flash controller. > > arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts | 4 +++ > arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts | 4 +++ > arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts | 4 +++ > arch/arm/boot/dts/bcm4708-luxul-xwc-1000.dts | 8 ++--- > arch/arm/boot/dts/bcm4708-netgear-r6250.dts | 4 +++ > arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts | 4 +++ > arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts | 4 +++ > arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts | 4 +++ > arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts | 4 +++ > arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts | 4 +++ > arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts | 4 +++ > arch/arm/boot/dts/bcm4709-netgear-r8000.dts | 4 +++ > arch/arm/boot/dts/bcm5301x.dtsi | 38 +++++++++++++++------- I'm curious: what tree are you looking at? I don't see most of these board DTS files. Or are they not intended for upstream, and you're just using them as examples? (Edit: found them in linux-next. And I see you're using unreviewed DT snippets, partly by virtue of not using a 'compatible' string for BCMA peripherals.) > 13 files changed, 74 insertions(+), 16 deletions(-) > > diff --git a/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts b/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts > index 71cff8d..f9e187f 100644 > --- a/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts > +++ b/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts > @@ -23,6 +23,10 @@ > reg = <0x00000000 0x08000000>; > }; > > + nand: nand@18028000 { > + status = "ok"; > + }; Are there any chips that don't have NAND enabled? I would expect the presence of the NAND controller to be determined at the chip level, not the board level. (I alluded to this in my comments to Rafal [1].) So maybe migrate this to bcm4708.dtsi or bcm5301x.dtsi, depending on whether there are any bcm5301x variants that have NAND disabled. Or maybe better: just put the 'status = "ok"' part alongside wherever you put the chip-select node, so you both enable the controller and a particular chip-select at the same time. > + > leds { > compatible = "gpio-leds"; > [...] > diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi > index 78aec62..cb86748 100644 > --- a/arch/arm/boot/dts/bcm5301x.dtsi > +++ b/arch/arm/boot/dts/bcm5301x.dtsi ... > @@ -143,4 +133,30 @@ > #gpio-cells = <2>; > }; > }; > + > + nand: nand@18028000 { > + compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1", "brcm,brcmnand"; > + reg = <0x18028000 0x600>, <0x1811a408 0x600>, <0x18028f00 0x20>; > + reg-names = "nand", "iproc-idm", "iproc-ext"; > + interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>; > + > + status = "disabled"; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + brcm,nand-has-wp; > + > + nandcs@0 { > + compatible = "brcm,nandcs"; > + reg = <0>; So every board that uses a BCM5301x chips must use chip-select 0? What if the board has NAND wired up on CS1? (I see this all the time on BCM7xxx boards.) > + #address-cells = <1>; > + #size-cells = <1>; > + > + nand-ecc-strength = <8>; > + nand-ecc-step-size = <512>; So, every board using the BCM5301x family must use BCH-8? What happens if there are boards that use a less reliable flash that needs, e.g., BCH-16? IOW, none of the nandcs@0 node belongs in the top-level chip description. This all belongs in the board description. If it really is repeated on tons of boards, then maybe you just need a separate 'nand-cs0-bch8.dtsi' or something like that. > + > + linux,part-probe = "ofpart", "bcm47xxpart"; ^ NAK to this line. You still haven't documented any semantics for this property. And I gave you several comments on your previous patch about what would need to change before I'd accept this property. > + }; > + }; > }; Brian [1] http://lists.infradead.org/pipermail/linux-mtd/2015-May/059419.html
On Tue, May 26, 2015 at 05:41:44PM -0700, Brian Norris wrote: > > + linux,part-probe = "ofpart", "bcm47xxpart"; > > ^ NAK to this line. You still haven't documented any semantics for this > property. And I gave you several comments on your previous patch about > what would need to change before I'd accept this property. Just noticed you sent v2 of your previous patch over the weekend, which does include documentation for this. That's a start, but I'm still not very happy with the property. I'll put my full comments there eventually. Brian
On Sunday 24 May 2015 20:32:29 Hauke Mehrtens wrote: > @@ -124,17 +124,7 @@ > <0x00026000 0 &gic GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>, > > /* Ethernet Controller 3 */ > - <0x00027000 0 &gic GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>, > - > - /* NAND Controller */ > - <0x00028000 0 &gic GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>, > - <0x00028000 1 &gic GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, > - <0x00028000 2 &gic GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>, > - <0x00028000 3 &gic GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>, > - <0x00028000 4 &gic GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>, > - <0x00028000 5 &gic GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>, > - <0x00028000 6 &gic GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>, > - <0x00028000 7 &gic GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>; > + <0x00027000 0 &gic GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>; > > chipcommon: chipcommon@0 { > reg = <0x00000000 0x1000>; > @@ -143,4 +133,30 @@ > #gpio-cells = <2>; > }; > }; > + > + nand: nand@18028000 { > + compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1", "brcm,brcmnand"; > + reg = <0x18028000 0x600>, <0x1811a408 0x600>, <0x18028f00 0x20>; > + reg-names = "nand", "iproc-idm", "iproc-ext"; > + interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>; I think I'd rather leave the interrupt-map in the brcm node, and use interrupts = <0>; here. > + status = "disabled"; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + brcm,nand-has-wp; > + > + nandcs@0 { > + compatible = "brcm,nandcs"; > + reg = <0>; > + #address-cells = <1>; > + #size-cells = <1>; > + > + nand-ecc-strength = <8>; > + nand-ecc-step-size = <512>; > + > + linux,part-probe = "ofpart", "bcm47xxpart"; > + }; > + }; This seems fine in principle, once the exact binding has been nailed down. "bcm47xxpart" does not seem like an appropriate string here. Arnd
On 05/27/2015 09:37 AM, Arnd Bergmann wrote: > On Sunday 24 May 2015 20:32:29 Hauke Mehrtens wrote: >> @@ -124,17 +124,7 @@ >> <0x00026000 0 &gic GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>, >> >> /* Ethernet Controller 3 */ >> - <0x00027000 0 &gic GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>, >> - >> - /* NAND Controller */ >> - <0x00028000 0 &gic GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>, >> - <0x00028000 1 &gic GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, >> - <0x00028000 2 &gic GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>, >> - <0x00028000 3 &gic GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>, >> - <0x00028000 4 &gic GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>, >> - <0x00028000 5 &gic GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>, >> - <0x00028000 6 &gic GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>, >> - <0x00028000 7 &gic GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>; >> + <0x00027000 0 &gic GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>; >> >> chipcommon: chipcommon@0 { >> reg = <0x00000000 0x1000>; >> @@ -143,4 +133,30 @@ >> #gpio-cells = <2>; >> }; >> }; >> + >> + nand: nand@18028000 { >> + compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1", "brcm,brcmnand"; >> + reg = <0x18028000 0x600>, <0x1811a408 0x600>, <0x18028f00 0x20>; >> + reg-names = "nand", "iproc-idm", "iproc-ext"; >> + interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>; > > I think I'd rather leave the interrupt-map in the brcm node, and use > > interrupts = <0>; > > here. There are two conflicting interests ;-) In the first approach we probed the NAND controller through bcma and just added the additional information we can not probe through device tree. That was this version: https://patchwork.ozlabs.org/patch/473181/ This version needs some changes to the brcmnand driver and this additional part: https://patchwork.ozlabs.org/patch/473182/ This code is simular to the iproc_nand.c Brian wanted us to use the iproc_nand.c which is possible, but then we completely ignore what we have probed in bcma and only use device tree. And this nand driver is not a subnode of the bcma bus any more so we can not use the irqs defined there. > >> + status = "disabled"; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + brcm,nand-has-wp; >> + >> + nandcs@0 { >> + compatible = "brcm,nandcs"; >> + reg = <0>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + >> + nand-ecc-strength = <8>; >> + nand-ecc-step-size = <512>; >> + >> + linux,part-probe = "ofpart", "bcm47xxpart"; >> + }; >> + }; > > This seems fine in principle, once the exact binding has been nailed down. > > "bcm47xxpart" does not seem like an appropriate string here. We are already thinking and talking about how to solve this problem. There are some parsers that are able to parse some partition layout based on information various sources or some vendor headers and some are also guessing in addition, like they are assuming that the bootloader is in the first block and n bytes long. The current module is that the flash driver defines which partition parsers to take, it is hard coded into the code. I do not like that. ;-) I am trying to get a simple patch in which makes it possible to overwrite the defaults from the flash driver with device tree. As it uses the same format it should be easy to convert devices from the old style to the new one. This deice tree attribute is already supported by one driver I moved the code to make it available for all drivers. The patch I am talking about: https://patchwork.ozlabs.org/patch/475988/ Hauke
On Wed, May 27, 2015 at 11:46:48PM +0200, Hauke Mehrtens wrote: > This deice tree attribute is already supported > by one driver I moved the code to make it available for all drivers. To be clear here, though, the original binding was barely reviewed, let alone documented. So I'd consider a redefinition is perfectly acceptable if we plan to make wider use of it. Brian
On 05/27/2015 02:41 AM, Brian Norris wrote: > On Sun, May 24, 2015 at 08:32:29PM +0200, Hauke Mehrtens wrote: >> This adds the NAND flash chip description for a standard chip found >> connected to this SoC. This makes use of generic Broadcom NAND driver >> with the iProc interface. >> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >> --- >> >> This would be the patch when we completely change to device tree only >> for the nand flash controller. >> >> arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts | 4 +++ >> arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts | 4 +++ >> arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts | 4 +++ >> arch/arm/boot/dts/bcm4708-luxul-xwc-1000.dts | 8 ++--- >> arch/arm/boot/dts/bcm4708-netgear-r6250.dts | 4 +++ >> arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts | 4 +++ >> arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts | 4 +++ >> arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts | 4 +++ >> arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts | 4 +++ >> arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts | 4 +++ >> arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts | 4 +++ >> arch/arm/boot/dts/bcm4709-netgear-r8000.dts | 4 +++ >> arch/arm/boot/dts/bcm5301x.dtsi | 38 +++++++++++++++------- > > I'm curious: what tree are you looking at? I don't see most of these > board DTS files. Or are they not intended for upstream, and you're just > using them as examples? (Edit: found them in linux-next. And I see > you're using unreviewed DT snippets, partly by virtue of not using a > 'compatible' string for BCMA peripherals.) This should go through Florian's Broadcom device tree tree and is based on that. These additional device are in his tree. > >> 13 files changed, 74 insertions(+), 16 deletions(-) >> >> diff --git a/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts b/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts >> index 71cff8d..f9e187f 100644 >> --- a/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts >> +++ b/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts >> @@ -23,6 +23,10 @@ >> reg = <0x00000000 0x08000000>; >> }; >> >> + nand: nand@18028000 { >> + status = "ok"; >> + }; > > Are there any chips that don't have NAND enabled? I would expect the > presence of the NAND controller to be determined at the chip level, not > the board level. (I alluded to this in my comments to Rafal [1].) So > maybe migrate this to bcm4708.dtsi or bcm5301x.dtsi, depending on > whether there are any bcm5301x variants that have NAND disabled. Or > maybe better: just put the 'status = "ok"' part alongside wherever you > put the chip-select node, so you both enable the controller and a > particular chip-select at the same time. To my knowledge all supported SoCs have a NAND controller, but some are not using it. There are some device with only serial flash, but I do not have one with only serial flash. If it is save we can activate it on all SoCs and boards. > >> + >> leds { >> compatible = "gpio-leds"; >> > [...] >> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi >> index 78aec62..cb86748 100644 >> --- a/arch/arm/boot/dts/bcm5301x.dtsi >> +++ b/arch/arm/boot/dts/bcm5301x.dtsi > ... >> @@ -143,4 +133,30 @@ >> #gpio-cells = <2>; >> }; >> }; >> + >> + nand: nand@18028000 { >> + compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1", "brcm,brcmnand"; >> + reg = <0x18028000 0x600>, <0x1811a408 0x600>, <0x18028f00 0x20>; >> + reg-names = "nand", "iproc-idm", "iproc-ext"; >> + interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>; >> + >> + status = "disabled"; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + brcm,nand-has-wp; >> + >> + nandcs@0 { >> + compatible = "brcm,nandcs"; >> + reg = <0>; > > So every board that uses a BCM5301x chips must use chip-select 0? What > if the board has NAND wired up on CS1? (I see this all the time on > BCM7xxx boards.) > >> + #address-cells = <1>; >> + #size-cells = <1>; >> + >> + nand-ecc-strength = <8>; >> + nand-ecc-step-size = <512>; > > So, every board using the BCM5301x family must use BCH-8? What happens > if there are boards that use a less reliable flash that needs, e.g., > BCH-16? > > IOW, none of the nandcs@0 node belongs in the top-level chip > description. This all belongs in the board description. If it really is > repeated on tons of boards, then maybe you just need a separate > 'nand-cs0-bch8.dtsi' or something like that. Ok, I can change it that way. The nand-cs0-bch8.dtsi sonds good. Currently I do not have a complete overview, but I think at least the vendor driver only supports Chip select 0 as far as I think and the ecc values are auto detected and calculated by some strange algorithm I haven't fully understood. > >> + >> + linux,part-probe = "ofpart", "bcm47xxpart"; > > ^ NAK to this line. You still haven't documented any semantics for this > property. And I gave you several comments on your previous patch about > what would need to change before I'd accept this property. We should talk about this in the patch adding the binding, I will adapt this depending on the result of the discussion. > >> + }; >> + }; >> }; > > Brian > > [1] http://lists.infradead.org/pipermail/linux-mtd/2015-May/059419.html >
On Wednesday 27 May 2015 23:46:48 Hauke Mehrtens wrote: > On 05/27/2015 09:37 AM, Arnd Bergmann wrote: > > On Sunday 24 May 2015 20:32:29 Hauke Mehrtens wrote: > >> @@ -124,17 +124,7 @@ > >> <0x00026000 0 &gic GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>, > >> > >> /* Ethernet Controller 3 */ > >> - <0x00027000 0 &gic GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>, > >> - > >> - /* NAND Controller */ > >> - <0x00028000 0 &gic GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>, > >> - <0x00028000 1 &gic GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, > >> - <0x00028000 2 &gic GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>, > >> - <0x00028000 3 &gic GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>, > >> - <0x00028000 4 &gic GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>, > >> - <0x00028000 5 &gic GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>, > >> - <0x00028000 6 &gic GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>, > >> - <0x00028000 7 &gic GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>; > >> + <0x00027000 0 &gic GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>; > >> > >> chipcommon: chipcommon@0 { > >> reg = <0x00000000 0x1000>; > >> @@ -143,4 +133,30 @@ > >> #gpio-cells = <2>; > >> }; > >> }; > >> + > >> + nand: nand@18028000 { > >> + compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1", "brcm,brcmnand"; > >> + reg = <0x18028000 0x600>, <0x1811a408 0x600>, <0x18028f00 0x20>; > >> + reg-names = "nand", "iproc-idm", "iproc-ext"; > >> + interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>; > > > > I think I'd rather leave the interrupt-map in the brcm node, and use > > > > interrupts = <0>; > > > > here. > > There are two conflicting interests ;-) > > In the first approach we probed the NAND controller through bcma and > just added the additional information we can not probe through device > tree. That was this version: > https://patchwork.ozlabs.org/patch/473181/ > > This version needs some changes to the brcmnand driver and this > additional part: > https://patchwork.ozlabs.org/patch/473182/ > > This code is simular to the iproc_nand.c > > Brian wanted us to use the iproc_nand.c which is possible, but then we > completely ignore what we have probed in bcma and only use device tree. > And this nand driver is not a subnode of the bcma bus any more so we can > not use the irqs defined there. Ok, I see. Maybe we should however leave the interrupt map entries in the bcma node, to leave the option of doing it either way? > > > >> + status = "disabled"; > >> + > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + > >> + brcm,nand-has-wp; > >> + > >> + nandcs@0 { > >> + compatible = "brcm,nandcs"; > >> + reg = <0>; > >> + #address-cells = <1>; > >> + #size-cells = <1>; > >> + > >> + nand-ecc-strength = <8>; > >> + nand-ecc-step-size = <512>; > >> + > >> + linux,part-probe = "ofpart", "bcm47xxpart"; > >> + }; > >> + }; > > > > This seems fine in principle, once the exact binding has been nailed down. > > > > "bcm47xxpart" does not seem like an appropriate string here. > > We are already thinking and talking about how to solve this problem. > There are some parsers that are able to parse some partition layout > based on information various sources or some vendor headers and some are > also guessing in addition, like they are assuming that the bootloader is > in the first block and n bytes long. The current module is that the > flash driver defines which partition parsers to take, it is hard coded > into the code. I do not like that. ;-) > > I am trying to get a simple patch in which makes it possible to > overwrite the defaults from the flash driver with device tree. > As it uses the same format it should be easy to convert devices from the > old style to the new one. This deice tree attribute is already supported > by one driver I moved the code to make it available for all drivers. > > The patch I am talking about: https://patchwork.ozlabs.org/patch/475988/ I was mainly referring to the use of 'xx' wildcards in the bcm47xxpart string. We try to avoid wildcards in compatible strings, and I think it makes sense to follow the same model here. Arnd
On 05/28/2015 10:21 AM, Arnd Bergmann wrote: > On Wednesday 27 May 2015 23:46:48 Hauke Mehrtens wrote: >> On 05/27/2015 09:37 AM, Arnd Bergmann wrote: >>> On Sunday 24 May 2015 20:32:29 Hauke Mehrtens wrote: >>>> @@ -124,17 +124,7 @@ >>>> <0x00026000 0 &gic GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>, >>>> >>>> /* Ethernet Controller 3 */ >>>> - <0x00027000 0 &gic GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>, >>>> - >>>> - /* NAND Controller */ >>>> - <0x00028000 0 &gic GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>, >>>> - <0x00028000 1 &gic GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, >>>> - <0x00028000 2 &gic GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>, >>>> - <0x00028000 3 &gic GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>, >>>> - <0x00028000 4 &gic GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>, >>>> - <0x00028000 5 &gic GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>, >>>> - <0x00028000 6 &gic GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>, >>>> - <0x00028000 7 &gic GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>; >>>> + <0x00027000 0 &gic GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>; >>>> >>>> chipcommon: chipcommon@0 { >>>> reg = <0x00000000 0x1000>; >>>> @@ -143,4 +133,30 @@ >>>> #gpio-cells = <2>; >>>> }; >>>> }; >>>> + >>>> + nand: nand@18028000 { >>>> + compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1", "brcm,brcmnand"; >>>> + reg = <0x18028000 0x600>, <0x1811a408 0x600>, <0x18028f00 0x20>; >>>> + reg-names = "nand", "iproc-idm", "iproc-ext"; >>>> + interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>; >>> >>> I think I'd rather leave the interrupt-map in the brcm node, and use >>> >>> interrupts = <0>; >>> >>> here. >> >> There are two conflicting interests ;-) >> >> In the first approach we probed the NAND controller through bcma and >> just added the additional information we can not probe through device >> tree. That was this version: >> https://patchwork.ozlabs.org/patch/473181/ >> >> This version needs some changes to the brcmnand driver and this >> additional part: >> https://patchwork.ozlabs.org/patch/473182/ >> >> This code is simular to the iproc_nand.c >> >> Brian wanted us to use the iproc_nand.c which is possible, but then we >> completely ignore what we have probed in bcma and only use device tree. >> And this nand driver is not a subnode of the bcma bus any more so we can >> not use the irqs defined there. > > Ok, I see. Maybe we should however leave the interrupt map entries > in the bcma node, to leave the option of doing it either way? Ok, I will do that as there are already there. > >>> >>>> + status = "disabled"; >>>> + >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + >>>> + brcm,nand-has-wp; >>>> + >>>> + nandcs@0 { >>>> + compatible = "brcm,nandcs"; >>>> + reg = <0>; >>>> + #address-cells = <1>; >>>> + #size-cells = <1>; >>>> + >>>> + nand-ecc-strength = <8>; >>>> + nand-ecc-step-size = <512>; >>>> + >>>> + linux,part-probe = "ofpart", "bcm47xxpart"; >>>> + }; >>>> + }; >>> >>> This seems fine in principle, once the exact binding has been nailed down. >>> >>> "bcm47xxpart" does not seem like an appropriate string here. >> >> We are already thinking and talking about how to solve this problem. >> There are some parsers that are able to parse some partition layout >> based on information various sources or some vendor headers and some are >> also guessing in addition, like they are assuming that the bootloader is >> in the first block and n bytes long. The current module is that the >> flash driver defines which partition parsers to take, it is hard coded >> into the code. I do not like that. ;-) >> >> I am trying to get a simple patch in which makes it possible to >> overwrite the defaults from the flash driver with device tree. >> As it uses the same format it should be easy to convert devices from the >> old style to the new one. This deice tree attribute is already supported >> by one driver I moved the code to make it available for all drivers. >> >> The patch I am talking about: https://patchwork.ozlabs.org/patch/475988/ > > I was mainly referring to the use of 'xx' wildcards in the bcm47xxpart > string. We try to avoid wildcards in compatible strings, and I think it > makes sense to follow the same model here. Ah, ok currently this is referring to the old names of the partition parsers and this code was added before we know that there will be arm SoCs using this. As it is not clear how this will be represented in device tree I would remove it for now form our device tree. Hauke
diff --git a/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts b/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts index 71cff8d..f9e187f 100644 --- a/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts +++ b/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts @@ -23,6 +23,10 @@ reg = <0x00000000 0x08000000>; }; + nand: nand@18028000 { + status = "ok"; + }; + leds { compatible = "gpio-leds"; diff --git a/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts b/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts index 8b62836..0559dcf 100644 --- a/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts +++ b/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts @@ -23,6 +23,10 @@ reg = <0x00000000 0x08000000>; }; + nand: nand@18028000 { + status = "ok"; + }; + leds { compatible = "gpio-leds"; diff --git a/arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts b/arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts index 78e95c0..6e1ab06 100644 --- a/arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts +++ b/arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts @@ -23,6 +23,10 @@ reg = <0x00000000 0x08000000>; }; + nand: nand@18028000 { + status = "ok"; + }; + spi { compatible = "spi-gpio"; num-chipselects = <1>; diff --git a/arch/arm/boot/dts/bcm4708-luxul-xwc-1000.dts b/arch/arm/boot/dts/bcm4708-luxul-xwc-1000.dts index 946c728..1aa141c 100644 --- a/arch/arm/boot/dts/bcm4708-luxul-xwc-1000.dts +++ b/arch/arm/boot/dts/bcm4708-luxul-xwc-1000.dts @@ -23,12 +23,10 @@ reg = <0x00000000 0x08000000>; }; - axi@18000000 { - nand@28000 { - reg = <0x00028000 0x1000>; - #address-cells = <1>; - #size-cells = <1>; + nand: nand@18028000 { + status = "ok"; + nandcs@0 { partition@0 { label = "ubi"; reg = <0x00000000 0x08000000>; diff --git a/arch/arm/boot/dts/bcm4708-netgear-r6250.dts b/arch/arm/boot/dts/bcm4708-netgear-r6250.dts index 2ed9e57..236e0ee 100644 --- a/arch/arm/boot/dts/bcm4708-netgear-r6250.dts +++ b/arch/arm/boot/dts/bcm4708-netgear-r6250.dts @@ -33,6 +33,10 @@ }; }; + nand: nand@18028000 { + status = "ok"; + }; + leds { compatible = "gpio-leds"; diff --git a/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts b/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts index 3991042..d36eee5 100644 --- a/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts +++ b/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts @@ -23,6 +23,10 @@ reg = <0x00000000 0x08000000>; }; + nand: nand@18028000 { + status = "ok"; + }; + leds { compatible = "gpio-leds"; diff --git a/arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts b/arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts index 66dfb53..bd4ad04 100644 --- a/arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts +++ b/arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts @@ -23,6 +23,10 @@ reg = <0x00000000 0x08000000>; }; + nand: nand@18028000 { + status = "ok"; + }; + leds { compatible = "gpio-leds"; diff --git a/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts b/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts index 0ee85ea..815d3b7 100644 --- a/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts +++ b/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts @@ -23,6 +23,10 @@ reg = <0x00000000 0x08000000>; }; + nand: nand@18028000 { + status = "ok"; + }; + leds { compatible = "gpio-leds"; diff --git a/arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts b/arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts index db9131e..488d2b6 100644 --- a/arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts +++ b/arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts @@ -23,6 +23,10 @@ reg = <0x00000000 0x08000000>; }; + nand: nand@18028000 { + status = "ok"; + }; + spi { compatible = "spi-gpio"; num-chipselects = <1>; diff --git a/arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts b/arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts index 7d6868a..547a70a 100644 --- a/arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts +++ b/arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts @@ -23,6 +23,10 @@ reg = <0x00000000 0x08000000>; }; + nand: nand@18028000 { + status = "ok"; + }; + gpio-keys { compatible = "gpio-keys"; #address-cells = <1>; diff --git a/arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts b/arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts index 548e93b..ad2b27f 100644 --- a/arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts +++ b/arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts @@ -23,6 +23,10 @@ reg = <0x00000000 0x08000000>; }; + nand: nand@18028000 { + status = "ok"; + }; + leds { compatible = "gpio-leds"; diff --git a/arch/arm/boot/dts/bcm4709-netgear-r8000.dts b/arch/arm/boot/dts/bcm4709-netgear-r8000.dts index ea26dd3..ff50fd4 100644 --- a/arch/arm/boot/dts/bcm4709-netgear-r8000.dts +++ b/arch/arm/boot/dts/bcm4709-netgear-r8000.dts @@ -23,6 +23,10 @@ reg = <0x00000000 0x08000000>; }; + nand: nand@18028000 { + status = "ok"; + }; + leds { compatible = "gpio-leds"; diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi index 78aec62..cb86748 100644 --- a/arch/arm/boot/dts/bcm5301x.dtsi +++ b/arch/arm/boot/dts/bcm5301x.dtsi @@ -124,17 +124,7 @@ <0x00026000 0 &gic GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>, /* Ethernet Controller 3 */ - <0x00027000 0 &gic GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>, - - /* NAND Controller */ - <0x00028000 0 &gic GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>, - <0x00028000 1 &gic GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, - <0x00028000 2 &gic GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>, - <0x00028000 3 &gic GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>, - <0x00028000 4 &gic GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>, - <0x00028000 5 &gic GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>, - <0x00028000 6 &gic GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>, - <0x00028000 7 &gic GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>; + <0x00027000 0 &gic GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>; chipcommon: chipcommon@0 { reg = <0x00000000 0x1000>; @@ -143,4 +133,30 @@ #gpio-cells = <2>; }; }; + + nand: nand@18028000 { + compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1", "brcm,brcmnand"; + reg = <0x18028000 0x600>, <0x1811a408 0x600>, <0x18028f00 0x20>; + reg-names = "nand", "iproc-idm", "iproc-ext"; + interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>; + + status = "disabled"; + + #address-cells = <1>; + #size-cells = <0>; + + brcm,nand-has-wp; + + nandcs@0 { + compatible = "brcm,nandcs"; + reg = <0>; + #address-cells = <1>; + #size-cells = <1>; + + nand-ecc-strength = <8>; + nand-ecc-step-size = <512>; + + linux,part-probe = "ofpart", "bcm47xxpart"; + }; + }; };
This adds the NAND flash chip description for a standard chip found connected to this SoC. This makes use of generic Broadcom NAND driver with the iProc interface. Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> --- This would be the patch when we completely change to device tree only for the nand flash controller. arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts | 4 +++ arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts | 4 +++ arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts | 4 +++ arch/arm/boot/dts/bcm4708-luxul-xwc-1000.dts | 8 ++--- arch/arm/boot/dts/bcm4708-netgear-r6250.dts | 4 +++ arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts | 4 +++ arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts | 4 +++ arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts | 4 +++ arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts | 4 +++ arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts | 4 +++ arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts | 4 +++ arch/arm/boot/dts/bcm4709-netgear-r8000.dts | 4 +++ arch/arm/boot/dts/bcm5301x.dtsi | 38 +++++++++++++++------- 13 files changed, 74 insertions(+), 16 deletions(-)