[PATCH] mtd: nand: denali: allow to override max_banks from DT property

[PATCH] mtd: nand: denali: allow to override max_banks from DT property

[Masahiro Yamada]

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

[PATCH] mtd: nand: denali: allow to override max_banks from DT property

[Masahiro Yamada]

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/

Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property

[Rob Herring]

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(-)

Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property

[Rob Herring]

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

Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property

[Boris Brezillon]

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

Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property

[Masahiro Yamada]

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

Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property

[Boris Brezillon]

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

Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property

[Boris Brezillon]

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

Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property

[Masahiro Yamada]

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

Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property

[Boris Brezillon]

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