* [PATCH] mtd: nand: denali: allow to override max_banks from DT property @ 2016-03-24 12:24 ` Masahiro Yamada 0 siblings, 0 replies; 10+ messages in thread From: Masahiro Yamada @ 2016-03-24 12:24 UTC (permalink / raw) To: linux-mtd Cc: Graham Moore, Dinh Nguyen, Masahiro Yamada, devicetree, linux-kernel, Richard Weinberger, Kumar Gala, Boris Brezillon, Ian Campbell, Brian Norris, Rob Herring, David Woodhouse, Pawel Moll, Mark Rutland Commit 271707b1d817 ("mtd: nand: denali: max_banks calculation changed in revision 5.1") supported the new encoding of the "n_banks" bits of the "features" register, but there is an unfortunate case that is not covered by that commit. Panasonic (its System LSI division is now Socionext) bought several versions of this IP. The IP released for Panasonic around Feb. 2012 is revision 5 and uses the old encoding for n_banks (2 << n_banks). While the one released around Nov. 2012 is also revision 5, but it uses the new encoding (1 << n_banks). The revision register cannot distinguish these two incompatible hardware. I guess this IP series is not well-organized. I could not find any alternative but giving max_banks from DT property. This commit works around the problem by allowing DT to set the max_banks forcibly. Of course, this DT property can be optional if the auto detection based on the hardware registers works well. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- Documentation/devicetree/bindings/mtd/denali-nand.txt | 4 ++++ drivers/mtd/nand/denali.c | 3 ++- drivers/mtd/nand/denali_dt.c | 3 +++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt index 785b825..78c250d 100644 --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt @@ -7,6 +7,10 @@ Required properties: - interrupts : The interrupt number. - dma-mask : DMA bit mask +Optional properties: + - max-banks : Maximum number of banks supported by hardware. If not + specified, it is determined based on the "features" register of hardware. + The device tree may optionally contain sub-nodes describing partitions of the address space. See partition.txt for more detail. diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c index 30bf5f6..e18b569 100644 --- a/drivers/mtd/nand/denali.c +++ b/drivers/mtd/nand/denali.c @@ -1353,7 +1353,8 @@ static void denali_hw_init(struct denali_nand_info *denali) */ denali->bbtskipbytes = ioread32(denali->flash_reg + SPARE_AREA_SKIP_BYTES); - detect_max_banks(denali); + if (!denali->max_banks) + detect_max_banks(denali); denali_nand_reset(denali); iowrite32(0x0F, denali->flash_reg + RB_PIN_ENABLED); iowrite32(CHIP_EN_DONT_CARE__FLAG, diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c index 0cb1e8d..be55db8 100644 --- a/drivers/mtd/nand/denali_dt.c +++ b/drivers/mtd/nand/denali_dt.c @@ -85,6 +85,9 @@ static int denali_dt_probe(struct platform_device *ofdev) denali->dev->dma_mask = NULL; } + of_property_read_u32(ofdev->dev.of_node, "max-banks", + &denali->max_banks); + dt->clk = devm_clk_get(&ofdev->dev, NULL); if (IS_ERR(dt->clk)) { dev_err(&ofdev->dev, "no clk available\n"); -- 1.9.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] mtd: nand: denali: allow to override max_banks from DT property @ 2016-03-24 12:24 ` Masahiro Yamada 0 siblings, 0 replies; 10+ messages in thread From: Masahiro Yamada @ 2016-03-24 12:24 UTC (permalink / raw) To: linux-mtd Cc: Mark Rutland, devicetree, Pawel Moll, Boris Brezillon, Richard Weinberger, Ian Campbell, linux-kernel, Masahiro Yamada, Rob Herring, Kumar Gala, Dinh Nguyen, Brian Norris, David Woodhouse, Graham Moore Commit 271707b1d817 ("mtd: nand: denali: max_banks calculation changed in revision 5.1") supported the new encoding of the "n_banks" bits of the "features" register, but there is an unfortunate case that is not covered by that commit. Panasonic (its System LSI division is now Socionext) bought several versions of this IP. The IP released for Panasonic around Feb. 2012 is revision 5 and uses the old encoding for n_banks (2 << n_banks). While the one released around Nov. 2012 is also revision 5, but it uses the new encoding (1 << n_banks). The revision register cannot distinguish these two incompatible hardware. I guess this IP series is not well-organized. I could not find any alternative but giving max_banks from DT property. This commit works around the problem by allowing DT to set the max_banks forcibly. Of course, this DT property can be optional if the auto detection based on the hardware registers works well. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- Documentation/devicetree/bindings/mtd/denali-nand.txt | 4 ++++ drivers/mtd/nand/denali.c | 3 ++- drivers/mtd/nand/denali_dt.c | 3 +++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt index 785b825..78c250d 100644 --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt @@ -7,6 +7,10 @@ Required properties: - interrupts : The interrupt number. - dma-mask : DMA bit mask +Optional properties: + - max-banks : Maximum number of banks supported by hardware. If not + specified, it is determined based on the "features" register of hardware. + The device tree may optionally contain sub-nodes describing partitions of the address space. See partition.txt for more detail. diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c index 30bf5f6..e18b569 100644 --- a/drivers/mtd/nand/denali.c +++ b/drivers/mtd/nand/denali.c @@ -1353,7 +1353,8 @@ static void denali_hw_init(struct denali_nand_info *denali) */ denali->bbtskipbytes = ioread32(denali->flash_reg + SPARE_AREA_SKIP_BYTES); - detect_max_banks(denali); + if (!denali->max_banks) + detect_max_banks(denali); denali_nand_reset(denali); iowrite32(0x0F, denali->flash_reg + RB_PIN_ENABLED); iowrite32(CHIP_EN_DONT_CARE__FLAG, diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c index 0cb1e8d..be55db8 100644 --- a/drivers/mtd/nand/denali_dt.c +++ b/drivers/mtd/nand/denali_dt.c @@ -85,6 +85,9 @@ static int denali_dt_probe(struct platform_device *ofdev) denali->dev->dma_mask = NULL; } + of_property_read_u32(ofdev->dev.of_node, "max-banks", + &denali->max_banks); + dt->clk = devm_clk_get(&ofdev->dev, NULL); if (IS_ERR(dt->clk)) { dev_err(&ofdev->dev, "no clk available\n"); -- 1.9.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property @ 2016-03-25 14:37 ` Rob Herring 0 siblings, 0 replies; 10+ messages in thread From: Rob Herring @ 2016-03-25 14:37 UTC (permalink / raw) To: Masahiro Yamada Cc: linux-mtd, Graham Moore, Dinh Nguyen, devicetree, linux-kernel, Richard Weinberger, Kumar Gala, Boris Brezillon, Ian Campbell, Brian Norris, David Woodhouse, Pawel Moll, Mark Rutland On Thu, Mar 24, 2016 at 09:24:37PM +0900, Masahiro Yamada wrote: > Commit 271707b1d817 ("mtd: nand: denali: max_banks calculation > changed in revision 5.1") supported the new encoding of the "n_banks" > bits of the "features" register, but there is an unfortunate case > that is not covered by that commit. > > Panasonic (its System LSI division is now Socionext) bought several > versions of this IP. The IP released for Panasonic around Feb. 2012 > is revision 5 and uses the old encoding for n_banks (2 << n_banks). > While the one released around Nov. 2012 is also revision 5, but it > uses the new encoding (1 << n_banks). > > The revision register cannot distinguish these two incompatible > hardware. I guess this IP series is not well-organized. I could not > find any alternative but giving max_banks from DT property. > > This commit works around the problem by allowing DT to set the > max_banks forcibly. Of course, this DT property can be optional if > the auto detection based on the hardware registers works well. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > Documentation/devicetree/bindings/mtd/denali-nand.txt | 4 ++++ Acked-by: Rob Herring <robh@kernel.org> > drivers/mtd/nand/denali.c | 3 ++- > drivers/mtd/nand/denali_dt.c | 3 +++ > 3 files changed, 9 insertions(+), 1 deletion(-) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property @ 2016-03-25 14:37 ` Rob Herring 0 siblings, 0 replies; 10+ messages in thread From: Rob Herring @ 2016-03-25 14:37 UTC (permalink / raw) To: Masahiro Yamada Cc: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Graham Moore, Dinh Nguyen, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Richard Weinberger, Kumar Gala, Boris Brezillon, Ian Campbell, Brian Norris, David Woodhouse, Pawel Moll, Mark Rutland On Thu, Mar 24, 2016 at 09:24:37PM +0900, Masahiro Yamada wrote: > Commit 271707b1d817 ("mtd: nand: denali: max_banks calculation > changed in revision 5.1") supported the new encoding of the "n_banks" > bits of the "features" register, but there is an unfortunate case > that is not covered by that commit. > > Panasonic (its System LSI division is now Socionext) bought several > versions of this IP. The IP released for Panasonic around Feb. 2012 > is revision 5 and uses the old encoding for n_banks (2 << n_banks). > While the one released around Nov. 2012 is also revision 5, but it > uses the new encoding (1 << n_banks). > > The revision register cannot distinguish these two incompatible > hardware. I guess this IP series is not well-organized. I could not > find any alternative but giving max_banks from DT property. > > This commit works around the problem by allowing DT to set the > max_banks forcibly. Of course, this DT property can be optional if > the auto detection based on the hardware registers works well. > > Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> > --- > > Documentation/devicetree/bindings/mtd/denali-nand.txt | 4 ++++ Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> > drivers/mtd/nand/denali.c | 3 ++- > drivers/mtd/nand/denali_dt.c | 3 +++ > 3 files changed, 9 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property 2016-03-24 12:24 ` Masahiro Yamada (?) (?) @ 2016-03-25 14:45 ` Boris Brezillon 2016-03-26 4:27 ` Masahiro Yamada -1 siblings, 1 reply; 10+ messages in thread From: Boris Brezillon @ 2016-03-25 14:45 UTC (permalink / raw) To: Masahiro Yamada Cc: linux-mtd, Graham Moore, Dinh Nguyen, devicetree, linux-kernel, Richard Weinberger, Kumar Gala, Ian Campbell, Brian Norris, Rob Herring, David Woodhouse, Pawel Moll, Mark Rutland On Thu, 24 Mar 2016 21:24:37 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Commit 271707b1d817 ("mtd: nand: denali: max_banks calculation > changed in revision 5.1") supported the new encoding of the "n_banks" > bits of the "features" register, but there is an unfortunate case > that is not covered by that commit. > > Panasonic (its System LSI division is now Socionext) bought several > versions of this IP. The IP released for Panasonic around Feb. 2012 > is revision 5 and uses the old encoding for n_banks (2 << n_banks). > While the one released around Nov. 2012 is also revision 5, but it > uses the new encoding (1 << n_banks). > > The revision register cannot distinguish these two incompatible > hardware. I guess this IP series is not well-organized. I could not > find any alternative but giving max_banks from DT property. Hm, shouldn't that be addressed with a new compatible instead of adding a extra property? > > This commit works around the problem by allowing DT to set the > max_banks forcibly. Of course, this DT property can be optional if > the auto detection based on the hardware registers works well. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > Documentation/devicetree/bindings/mtd/denali-nand.txt | 4 ++++ > drivers/mtd/nand/denali.c | 3 ++- > drivers/mtd/nand/denali_dt.c | 3 +++ > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt > index 785b825..78c250d 100644 > --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt > +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt > @@ -7,6 +7,10 @@ Required properties: > - interrupts : The interrupt number. > - dma-mask : DMA bit mask > > +Optional properties: > + - max-banks : Maximum number of banks supported by hardware. If not > + specified, it is determined based on the "features" register of hardware. > + You might want to prefix that with "denali,". > The device tree may optionally contain sub-nodes describing partitions of the > address space. See partition.txt for more detail. > > diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c > index 30bf5f6..e18b569 100644 > --- a/drivers/mtd/nand/denali.c > +++ b/drivers/mtd/nand/denali.c > @@ -1353,7 +1353,8 @@ static void denali_hw_init(struct denali_nand_info *denali) > */ > denali->bbtskipbytes = ioread32(denali->flash_reg + > SPARE_AREA_SKIP_BYTES); > - detect_max_banks(denali); > + if (!denali->max_banks) > + detect_max_banks(denali); > denali_nand_reset(denali); > iowrite32(0x0F, denali->flash_reg + RB_PIN_ENABLED); > iowrite32(CHIP_EN_DONT_CARE__FLAG, > diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c > index 0cb1e8d..be55db8 100644 > --- a/drivers/mtd/nand/denali_dt.c > +++ b/drivers/mtd/nand/denali_dt.c > @@ -85,6 +85,9 @@ static int denali_dt_probe(struct platform_device *ofdev) > denali->dev->dma_mask = NULL; > } > > + of_property_read_u32(ofdev->dev.of_node, "max-banks", > + &denali->max_banks); > + > dt->clk = devm_clk_get(&ofdev->dev, NULL); > if (IS_ERR(dt->clk)) { > dev_err(&ofdev->dev, "no clk available\n"); -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property 2016-03-25 14:45 ` Boris Brezillon @ 2016-03-26 4:27 ` Masahiro Yamada 2016-03-29 7:53 ` Boris Brezillon 0 siblings, 1 reply; 10+ messages in thread From: Masahiro Yamada @ 2016-03-26 4:27 UTC (permalink / raw) To: Boris Brezillon Cc: linux-mtd, Graham Moore, Dinh Nguyen, devicetree, Linux Kernel Mailing List, Richard Weinberger, Kumar Gala, Ian Campbell, Brian Norris, Rob Herring, David Woodhouse, Pawel Moll, Mark Rutland 2016-03-25 23:45 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > On Thu, 24 Mar 2016 21:24:37 +0900 > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > >> Commit 271707b1d817 ("mtd: nand: denali: max_banks calculation >> changed in revision 5.1") supported the new encoding of the "n_banks" >> bits of the "features" register, but there is an unfortunate case >> that is not covered by that commit. >> >> Panasonic (its System LSI division is now Socionext) bought several >> versions of this IP. The IP released for Panasonic around Feb. 2012 >> is revision 5 and uses the old encoding for n_banks (2 << n_banks). >> While the one released around Nov. 2012 is also revision 5, but it >> uses the new encoding (1 << n_banks). >> >> The revision register cannot distinguish these two incompatible >> hardware. I guess this IP series is not well-organized. I could not >> find any alternative but giving max_banks from DT property. > > Hm, shouldn't that be addressed with a new compatible instead of adding > a extra property? > >> >> This commit works around the problem by allowing DT to set the >> max_banks forcibly. Of course, this DT property can be optional if >> the auto detection based on the hardware registers works well. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> --- >> >> Documentation/devicetree/bindings/mtd/denali-nand.txt | 4 ++++ >> drivers/mtd/nand/denali.c | 3 ++- >> drivers/mtd/nand/denali_dt.c | 3 +++ >> 3 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt >> index 785b825..78c250d 100644 >> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt >> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt >> @@ -7,6 +7,10 @@ Required properties: >> - interrupts : The interrupt number. >> - dma-mask : DMA bit mask >> >> +Optional properties: >> + - max-banks : Maximum number of banks supported by hardware. If not >> + specified, it is determined based on the "features" register of hardware. >> + > > You might want to prefix that with "denali,". > >> The device tree may optionally contain sub-nodes describing partitions of the >> address space. See partition.txt for more detail. In which case, do we have to add a vendor prefix to DT properties? I do not know a clear rule about this. -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property @ 2016-03-29 7:53 ` Boris Brezillon 0 siblings, 0 replies; 10+ messages in thread From: Boris Brezillon @ 2016-03-29 7:53 UTC (permalink / raw) To: Masahiro Yamada Cc: linux-mtd, Graham Moore, Dinh Nguyen, devicetree, Linux Kernel Mailing List, Richard Weinberger, Kumar Gala, Ian Campbell, Brian Norris, Rob Herring, David Woodhouse, Pawel Moll, Mark Rutland Hi, I'm answering to this one, but I already saw your v2. On Sat, 26 Mar 2016 13:27:50 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2016-03-25 23:45 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > > On Thu, 24 Mar 2016 21:24:37 +0900 > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > > >> Commit 271707b1d817 ("mtd: nand: denali: max_banks calculation > >> changed in revision 5.1") supported the new encoding of the "n_banks" > >> bits of the "features" register, but there is an unfortunate case > >> that is not covered by that commit. > >> > >> Panasonic (its System LSI division is now Socionext) bought several > >> versions of this IP. The IP released for Panasonic around Feb. 2012 > >> is revision 5 and uses the old encoding for n_banks (2 << n_banks). > >> While the one released around Nov. 2012 is also revision 5, but it > >> uses the new encoding (1 << n_banks). > >> > >> The revision register cannot distinguish these two incompatible > >> hardware. I guess this IP series is not well-organized. I could not > >> find any alternative but giving max_banks from DT property. > > > > Hm, shouldn't that be addressed with a new compatible instead of adding > > a extra property? You didn't answer to this suggestion. I see several advantages in this approach: 1/ You'll only break the DT once (to add your new compatible) even if you got your logic wrong. All you'll have to do in this case is patch your driver to change the compatible <-> capabilities association 2/ This may not be the only difference between the 2 revisions, and in this case, putting the compatible <-> capabilities association directly in your driver will allow you to easily tweak capabilities per-revision > > > >> > >> This commit works around the problem by allowing DT to set the > >> max_banks forcibly. Of course, this DT property can be optional if > >> the auto detection based on the hardware registers works well. > >> > >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > >> --- > >> > >> Documentation/devicetree/bindings/mtd/denali-nand.txt | 4 ++++ > >> drivers/mtd/nand/denali.c | 3 ++- > >> drivers/mtd/nand/denali_dt.c | 3 +++ > >> 3 files changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt > >> index 785b825..78c250d 100644 > >> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt > >> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt > >> @@ -7,6 +7,10 @@ Required properties: > >> - interrupts : The interrupt number. > >> - dma-mask : DMA bit mask > >> > >> +Optional properties: > >> + - max-banks : Maximum number of banks supported by hardware. If not > >> + specified, it is determined based on the "features" register of hardware. > >> + > > > > You might want to prefix that with "denali,". > > > >> The device tree may optionally contain sub-nodes describing partitions of the > >> address space. See partition.txt for more detail. > > > In which case, do we have to add a vendor prefix to DT properties? > > I do not know a clear rule about this. Usually you add a vendor prefix when the property only applies to a specific controller/IP. In the NAND specific case, most NAND controllers have a fixed number of banks which can be deduced from the IP revision/version. I'd like to keep it like that and avoid seeing other drivers use this max-banks property to deduce the number of available banks, hence my suggestion to, at least, prefix your property with "denali,". But I'd still prefer to see the max-banks value be associated to a new compatible. Thanks, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property @ 2016-03-29 7:53 ` Boris Brezillon 0 siblings, 0 replies; 10+ messages in thread From: Boris Brezillon @ 2016-03-29 7:53 UTC (permalink / raw) To: Masahiro Yamada Cc: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Graham Moore, Dinh Nguyen, devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Kernel Mailing List, Richard Weinberger, Kumar Gala, Ian Campbell, Brian Norris, Rob Herring, David Woodhouse, Pawel Moll, Mark Rutland Hi, I'm answering to this one, but I already saw your v2. On Sat, 26 Mar 2016 13:27:50 +0900 Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote: > 2016-03-25 23:45 GMT+09:00 Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>: > > On Thu, 24 Mar 2016 21:24:37 +0900 > > Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> wrote: > > > >> Commit 271707b1d817 ("mtd: nand: denali: max_banks calculation > >> changed in revision 5.1") supported the new encoding of the "n_banks" > >> bits of the "features" register, but there is an unfortunate case > >> that is not covered by that commit. > >> > >> Panasonic (its System LSI division is now Socionext) bought several > >> versions of this IP. The IP released for Panasonic around Feb. 2012 > >> is revision 5 and uses the old encoding for n_banks (2 << n_banks). > >> While the one released around Nov. 2012 is also revision 5, but it > >> uses the new encoding (1 << n_banks). > >> > >> The revision register cannot distinguish these two incompatible > >> hardware. I guess this IP series is not well-organized. I could not > >> find any alternative but giving max_banks from DT property. > > > > Hm, shouldn't that be addressed with a new compatible instead of adding > > a extra property? You didn't answer to this suggestion. I see several advantages in this approach: 1/ You'll only break the DT once (to add your new compatible) even if you got your logic wrong. All you'll have to do in this case is patch your driver to change the compatible <-> capabilities association 2/ This may not be the only difference between the 2 revisions, and in this case, putting the compatible <-> capabilities association directly in your driver will allow you to easily tweak capabilities per-revision > > > >> > >> This commit works around the problem by allowing DT to set the > >> max_banks forcibly. Of course, this DT property can be optional if > >> the auto detection based on the hardware registers works well. > >> > >> Signed-off-by: Masahiro Yamada <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org> > >> --- > >> > >> Documentation/devicetree/bindings/mtd/denali-nand.txt | 4 ++++ > >> drivers/mtd/nand/denali.c | 3 ++- > >> drivers/mtd/nand/denali_dt.c | 3 +++ > >> 3 files changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt > >> index 785b825..78c250d 100644 > >> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt > >> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt > >> @@ -7,6 +7,10 @@ Required properties: > >> - interrupts : The interrupt number. > >> - dma-mask : DMA bit mask > >> > >> +Optional properties: > >> + - max-banks : Maximum number of banks supported by hardware. If not > >> + specified, it is determined based on the "features" register of hardware. > >> + > > > > You might want to prefix that with "denali,". > > > >> The device tree may optionally contain sub-nodes describing partitions of the > >> address space. See partition.txt for more detail. > > > In which case, do we have to add a vendor prefix to DT properties? > > I do not know a clear rule about this. Usually you add a vendor prefix when the property only applies to a specific controller/IP. In the NAND specific case, most NAND controllers have a fixed number of banks which can be deduced from the IP revision/version. I'd like to keep it like that and avoid seeing other drivers use this max-banks property to deduce the number of available banks, hence my suggestion to, at least, prefix your property with "denali,". But I'd still prefer to see the max-banks value be associated to a new compatible. Thanks, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property 2016-03-29 7:53 ` Boris Brezillon (?) @ 2016-04-02 5:14 ` Masahiro Yamada 2016-04-02 14:03 ` Boris Brezillon -1 siblings, 1 reply; 10+ messages in thread From: Masahiro Yamada @ 2016-04-02 5:14 UTC (permalink / raw) To: Boris Brezillon Cc: linux-mtd, Graham Moore, Dinh Nguyen, devicetree, Linux Kernel Mailing List, Richard Weinberger, Kumar Gala, Ian Campbell, Brian Norris, Rob Herring, David Woodhouse, Pawel Moll, Mark Rutland Hi Boris, 2016-03-29 16:53 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > Hi, > > I'm answering to this one, but I already saw your v2. > > On Sat, 26 Mar 2016 13:27:50 +0900 > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > >> 2016-03-25 23:45 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: >> > On Thu, 24 Mar 2016 21:24:37 +0900 >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: >> > >> >> Commit 271707b1d817 ("mtd: nand: denali: max_banks calculation >> >> changed in revision 5.1") supported the new encoding of the "n_banks" >> >> bits of the "features" register, but there is an unfortunate case >> >> that is not covered by that commit. >> >> >> >> Panasonic (its System LSI division is now Socionext) bought several >> >> versions of this IP. The IP released for Panasonic around Feb. 2012 >> >> is revision 5 and uses the old encoding for n_banks (2 << n_banks). >> >> While the one released around Nov. 2012 is also revision 5, but it >> >> uses the new encoding (1 << n_banks). >> >> >> >> The revision register cannot distinguish these two incompatible >> >> hardware. I guess this IP series is not well-organized. I could not >> >> find any alternative but giving max_banks from DT property. >> > >> > Hm, shouldn't that be addressed with a new compatible instead of adding >> > a extra property? > > You didn't answer to this suggestion. I see several advantages in this > approach: > > 1/ You'll only break the DT once (to add your new compatible) even if > you got your logic wrong. All you'll have to do in this case is patch > your driver to change the compatible <-> capabilities association > > 2/ This may not be the only difference between the 2 revisions, and > in this case, putting the compatible <-> capabilities association > directly in your driver will allow you to easily tweak capabilities > per-revision > >> > >> >> >> >> This commit works around the problem by allowing DT to set the >> >> max_banks forcibly. Of course, this DT property can be optional if >> >> the auto detection based on the hardware registers works well. >> >> >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> >> --- >> >> >> >> Documentation/devicetree/bindings/mtd/denali-nand.txt | 4 ++++ >> >> drivers/mtd/nand/denali.c | 3 ++- >> >> drivers/mtd/nand/denali_dt.c | 3 +++ >> >> 3 files changed, 9 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt >> >> index 785b825..78c250d 100644 >> >> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt >> >> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt >> >> @@ -7,6 +7,10 @@ Required properties: >> >> - interrupts : The interrupt number. >> >> - dma-mask : DMA bit mask >> >> >> >> +Optional properties: >> >> + - max-banks : Maximum number of banks supported by hardware. If not >> >> + specified, it is determined based on the "features" register of hardware. >> >> + >> > >> > You might want to prefix that with "denali,". >> > >> >> The device tree may optionally contain sub-nodes describing partitions of the >> >> address space. See partition.txt for more detail. >> >> >> In which case, do we have to add a vendor prefix to DT properties? >> >> I do not know a clear rule about this. > > Usually you add a vendor prefix when the property only applies to a > specific controller/IP. In the NAND specific case, most NAND > controllers have a fixed number of banks which can be deduced from the > IP revision/version. I'd like to keep it like that and avoid seeing > other drivers use this max-banks property to deduce the number of > available banks, hence my suggestion to, at least, prefix your property > with "denali,". But I'd still prefer to see the max-banks value be > associated to a new compatible. > > Thanks, > > Boris I want to use this driver for ARM-based UniPhier SoCs. Their DT files are located as follows: arch/arm/boot/dts/uniphier-* arch/arm64/boot/dts/socionext/uniphier-* If we decided to add new DT compatible strings rather than DT property, which string would you suggest? Should it include "denali" or just SoC name "uniphier-*"? How about the vendor prefix? "denali," or "socionext," ? like this? I am not sure... nand: nand@68000000 { compatible = "socionext,uniphier-pro5-nand", "denali,denali-nand-dt"; reg-names = "nand_data", "denali_reg"; reg = <0x68000000 0x20>, <0x68100000 0x1000>; interrupts = <0 65 4>; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_nand>; clocks = <&nandclk>; }; At least, I need two DT compatible strings, for old/new "n_banks" encoding. The following part is also a problem for me because platform-specific ECC bit is hard-coded in the driver. The comment claims that Intel MRST supports 8bit and 15bit for ecc.strenth, while the Denali IP on UniPhier SoCs supports 8bit, 16bit, and 24bit ECC. I need to do something with it to proceed, but the code is crappy. /* * Denali Controller only support 15bit and 8bit ECC in MRST, * so just let controller do 15bit ECC for MLC and 8bit ECC for * SLC if possible. * */ if (!nand_is_slc(&denali->nand) && (mtd->oobsize > (denali->bbtskipbytes + ECC_15BITS * (mtd->writesize / ECC_SECTOR_SIZE)))) { /* if MLC OOB size is large enough, use 15bit ECC*/ denali->nand.ecc.strength = 15; denali->nand.ecc.layout = &nand_15bit_oob; denali->nand.ecc.bytes = ECC_15BITS; iowrite32(15, denali->flash_reg + ECC_CORRECTION); } else if (mtd->oobsize < (denali->bbtskipbytes + ECC_8BITS * (mtd->writesize / ECC_SECTOR_SIZE))) { pr_err("Your NAND chip OOB is not large enough to contain 8bit ECC correction codes"); goto failed_req_irq; } else { denali->nand.ecc.strength = 8; denali->nand.ecc.layout = &nand_8bit_oob; denali->nand.ecc.bytes = ECC_8BITS; iowrite32(8, denali->flash_reg + ECC_CORRECTION); } -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property 2016-04-02 5:14 ` Masahiro Yamada @ 2016-04-02 14:03 ` Boris Brezillon 0 siblings, 0 replies; 10+ messages in thread From: Boris Brezillon @ 2016-04-02 14:03 UTC (permalink / raw) To: Masahiro Yamada Cc: linux-mtd, Graham Moore, Dinh Nguyen, devicetree, Linux Kernel Mailing List, Richard Weinberger, Kumar Gala, Ian Campbell, Brian Norris, Rob Herring, David Woodhouse, Pawel Moll, Mark Rutland Hi Masahiro, On Sat, 2 Apr 2016 14:14:39 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Boris, > > > 2016-03-29 16:53 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > > Hi, > > > > I'm answering to this one, but I already saw your v2. > > > > On Sat, 26 Mar 2016 13:27:50 +0900 > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > > >> 2016-03-25 23:45 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > >> > On Thu, 24 Mar 2016 21:24:37 +0900 > >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > >> > > >> >> Commit 271707b1d817 ("mtd: nand: denali: max_banks calculation > >> >> changed in revision 5.1") supported the new encoding of the "n_banks" > >> >> bits of the "features" register, but there is an unfortunate case > >> >> that is not covered by that commit. > >> >> > >> >> Panasonic (its System LSI division is now Socionext) bought several > >> >> versions of this IP. The IP released for Panasonic around Feb. 2012 > >> >> is revision 5 and uses the old encoding for n_banks (2 << n_banks). > >> >> While the one released around Nov. 2012 is also revision 5, but it > >> >> uses the new encoding (1 << n_banks). > >> >> > >> >> The revision register cannot distinguish these two incompatible > >> >> hardware. I guess this IP series is not well-organized. I could not > >> >> find any alternative but giving max_banks from DT property. > >> > > >> > Hm, shouldn't that be addressed with a new compatible instead of adding > >> > a extra property? > > > > You didn't answer to this suggestion. I see several advantages in this > > approach: > > > > 1/ You'll only break the DT once (to add your new compatible) even if > > you got your logic wrong. All you'll have to do in this case is patch > > your driver to change the compatible <-> capabilities association > > > > 2/ This may not be the only difference between the 2 revisions, and > > in this case, putting the compatible <-> capabilities association > > directly in your driver will allow you to easily tweak capabilities > > per-revision > > > >> > > >> >> > >> >> This commit works around the problem by allowing DT to set the > >> >> max_banks forcibly. Of course, this DT property can be optional if > >> >> the auto detection based on the hardware registers works well. > >> >> > >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > >> >> --- > >> >> > >> >> Documentation/devicetree/bindings/mtd/denali-nand.txt | 4 ++++ > >> >> drivers/mtd/nand/denali.c | 3 ++- > >> >> drivers/mtd/nand/denali_dt.c | 3 +++ > >> >> 3 files changed, 9 insertions(+), 1 deletion(-) > >> >> > >> >> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt > >> >> index 785b825..78c250d 100644 > >> >> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt > >> >> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt > >> >> @@ -7,6 +7,10 @@ Required properties: > >> >> - interrupts : The interrupt number. > >> >> - dma-mask : DMA bit mask > >> >> > >> >> +Optional properties: > >> >> + - max-banks : Maximum number of banks supported by hardware. If not > >> >> + specified, it is determined based on the "features" register of hardware. > >> >> + > >> > > >> > You might want to prefix that with "denali,". > >> > > >> >> The device tree may optionally contain sub-nodes describing partitions of the > >> >> address space. See partition.txt for more detail. > >> > >> > >> In which case, do we have to add a vendor prefix to DT properties? > >> > >> I do not know a clear rule about this. > > > > Usually you add a vendor prefix when the property only applies to a > > specific controller/IP. In the NAND specific case, most NAND > > controllers have a fixed number of banks which can be deduced from the > > IP revision/version. I'd like to keep it like that and avoid seeing > > other drivers use this max-banks property to deduce the number of > > available banks, hence my suggestion to, at least, prefix your property > > with "denali,". But I'd still prefer to see the max-banks value be > > associated to a new compatible. > > > > Thanks, > > > > Boris > > I want to use this driver for ARM-based UniPhier SoCs. > Their DT files are located as follows: > > arch/arm/boot/dts/uniphier-* > arch/arm64/boot/dts/socionext/uniphier-* > > > If we decided to add new DT compatible strings rather than DT property, > which string would you suggest? > Should it include "denali" or just SoC name "uniphier-*"? > How about the vendor prefix? "denali," or "socionext," ? Ok, I'm not a DT expert, but I'd say "denali,uniphier-pro5-nand". Rob, can you confirm that please? > > > like this? I am not sure... > > > nand: nand@68000000 { > compatible = "socionext,uniphier-pro5-nand", "denali,denali-nand-dt"; Be careful, if you you this kind of inheritance, this means your controller has to work with the default "denali,denali-nand-dt" implementation. Not sure which one currently exposes the biggest number of banks, but when only denali,denali-nand-dt is specified, you should limit to the minimum value. > reg-names = "nand_data", "denali_reg"; > reg = <0x68000000 0x20>, <0x68100000 0x1000>; > interrupts = <0 65 4>; > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_nand>; > clocks = <&nandclk>; > }; > > > At least, I need two DT compatible strings, > for old/new "n_banks" encoding. > > The following part is also a problem for me because > platform-specific ECC bit is hard-coded in the driver. > > The comment claims that Intel MRST supports 8bit and 15bit for ecc.strenth, > while the Denali IP on UniPhier SoCs supports 8bit, 16bit, and 24bit ECC. Another reason to use compatible instead of an extra property. This way you can associate the supported ECC configs to a compatible... > > I need to do something with it to proceed, but the code is crappy. > > > /* > * Denali Controller only support 15bit and 8bit ECC in MRST, > * so just let controller do 15bit ECC for MLC and 8bit ECC for > * SLC if possible. > * */ > if (!nand_is_slc(&denali->nand) && > (mtd->oobsize > (denali->bbtskipbytes + > ECC_15BITS * (mtd->writesize / > ECC_SECTOR_SIZE)))) { > /* if MLC OOB size is large enough, use 15bit ECC*/ > denali->nand.ecc.strength = 15; > denali->nand.ecc.layout = &nand_15bit_oob; > denali->nand.ecc.bytes = ECC_15BITS; > iowrite32(15, denali->flash_reg + ECC_CORRECTION); > } else if (mtd->oobsize < (denali->bbtskipbytes + > ECC_8BITS * (mtd->writesize / > ECC_SECTOR_SIZE))) { > pr_err("Your NAND chip OOB is not large enough to > contain 8bit ECC correction codes"); > goto failed_req_irq; > } else { > denali->nand.ecc.strength = 8; > denali->nand.ecc.layout = &nand_8bit_oob; > denali->nand.ecc.bytes = ECC_8BITS; > iowrite32(8, denali->flash_reg + ECC_CORRECTION); > } > I'll have a closer look at this code later. Best Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-04-02 14:03 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-24 12:24 [PATCH] mtd: nand: denali: allow to override max_banks from DT property Masahiro Yamada 2016-03-24 12:24 ` Masahiro Yamada 2016-03-25 14:37 ` Rob Herring 2016-03-25 14:37 ` Rob Herring 2016-03-25 14:45 ` Boris Brezillon 2016-03-26 4:27 ` Masahiro Yamada 2016-03-29 7:53 ` Boris Brezillon 2016-03-29 7:53 ` Boris Brezillon 2016-04-02 5:14 ` Masahiro Yamada 2016-04-02 14:03 ` Boris Brezillon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.