[PATCH 1/2] dt-bindings: mtd: brcmnand: add "no-wp" property

[PATCH 1/2] dt-bindings: mtd: brcmnand: add "no-wp" property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

It's required to properly describe boards without connected WP pin (e.g.
Asus GT-AC5300).

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
index dd5a64969e37..49c7860c0dad 100644
--- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
+++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
@@ -111,6 +111,11 @@ properties:
       earlier versions of this core that include WP
     type: boolean
 
+  no-wp:
+    description:
+      This property marks boards with WP pin not connected to the NAND chip.
+    type: boolean
+
 patternProperties:
   "^nand@[a-f0-9]$":
     type: object
-- 
2.31.1

[PATCH 2/2] mtd: rawnand: brcmnand: support "no-wp" DT property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Some boards may use WP-capable controller but still have WP not
connected. This change fixes:
[    1.175550] bcm63138_nand ff801800.nand: timeout on status poll (expected c0000040 got c00000c0)
[    1.184524] bcm63138_nand ff801800.nand: nand #WP expected on
[    1.285547] bcm63138_nand ff801800.nand: timeout on status poll (expected c0000040 got c00000c0)
[    1.294516] bcm63138_nand ff801800.nand: nand #WP expected on
[    1.395548] bcm63138_nand ff801800.nand: timeout on status poll (expected c0000040 got c00000c0)
[    1.404517] bcm63138_nand ff801800.nand: nand #WP expected on

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index f75929783b94..8b6167457f0c 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -714,7 +714,8 @@ static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
 	if (ctrl->nand_version >= 0x0500)
 		ctrl->features |= BRCMNAND_HAS_1K_SECTORS;
 
-	if (ctrl->nand_version >= 0x0700)
+	if (ctrl->nand_version >= 0x0700 &&
+	    !of_property_read_bool(ctrl->dev->of_node, "no-wp"))
 		ctrl->features |= BRCMNAND_HAS_WP;
 	else if (of_property_read_bool(ctrl->dev->of_node, "brcm,nand-has-wp"))
 		ctrl->features |= BRCMNAND_HAS_WP;
-- 
2.31.1

Re: [PATCH 2/2] mtd: rawnand: brcmnand: support "no-wp" DT property

[Florian Fainelli]

On 11/3/21 8:11 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Some boards may use WP-capable controller but still have WP not
> connected. This change fixes:
> [    1.175550] bcm63138_nand ff801800.nand: timeout on status poll (expected c0000040 got c00000c0)
> [    1.184524] bcm63138_nand ff801800.nand: nand #WP expected on
> [    1.285547] bcm63138_nand ff801800.nand: timeout on status poll (expected c0000040 got c00000c0)
> [    1.294516] bcm63138_nand ff801800.nand: nand #WP expected on
> [    1.395548] bcm63138_nand ff801800.nand: timeout on status poll (expected c0000040 got c00000c0)
> [    1.404517] bcm63138_nand ff801800.nand: nand #WP expected on
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index f75929783b94..8b6167457f0c 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -714,7 +714,8 @@ static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
>  	if (ctrl->nand_version >= 0x0500)
>  		ctrl->features |= BRCMNAND_HAS_1K_SECTORS;
>  
> -	if (ctrl->nand_version >= 0x0700)
> +	if (ctrl->nand_version >= 0x0700 &&
> +	    !of_property_read_bool(ctrl->dev->of_node, "no-wp"))
>  		ctrl->features |= BRCMNAND_HAS_WP;

Should not this be a logical OR here or rather, should it be moved out
of the check on ctrl->nand_version entirely? What revision of the NAND
controller do you have on that chip?

>  	else if (of_property_read_bool(ctrl->dev->of_node, "brcm,nand-has-wp"))
>  		ctrl->features |= BRCMNAND_HAS_WP;
> 


-- 
Florian

Re: [PATCH 2/2] mtd: rawnand: brcmnand: support "no-wp" DT property

[Rafał Miłecki]

On 2021-11-03 18:50, Florian Fainelli wrote:
> On 11/3/21 8:11 AM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>> 
>> Some boards may use WP-capable controller but still have WP not
>> connected. This change fixes:
>> [    1.175550] bcm63138_nand ff801800.nand: timeout on status poll 
>> (expected c0000040 got c00000c0)
>> [    1.184524] bcm63138_nand ff801800.nand: nand #WP expected on
>> [    1.285547] bcm63138_nand ff801800.nand: timeout on status poll 
>> (expected c0000040 got c00000c0)
>> [    1.294516] bcm63138_nand ff801800.nand: nand #WP expected on
>> [    1.395548] bcm63138_nand ff801800.nand: timeout on status poll 
>> (expected c0000040 got c00000c0)
>> [    1.404517] bcm63138_nand ff801800.nand: nand #WP expected on
>> 
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c 
>> b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> index f75929783b94..8b6167457f0c 100644
>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> @@ -714,7 +714,8 @@ static int brcmnand_revision_init(struct 
>> brcmnand_controller *ctrl)
>>  	if (ctrl->nand_version >= 0x0500)
>>  		ctrl->features |= BRCMNAND_HAS_1K_SECTORS;
>> 
>> -	if (ctrl->nand_version >= 0x0700)
>> +	if (ctrl->nand_version >= 0x0700 &&
>> +	    !of_property_read_bool(ctrl->dev->of_node, "no-wp"))
>>  		ctrl->features |= BRCMNAND_HAS_WP;
> 
> Should not this be a logical OR here or rather, should it be moved out
> of the check on ctrl->nand_version entirely? What revision of the NAND
> controller do you have on that chip?

It's NAND controller version 0x0701 (v7.1) and I think it's correct.

I think we want WP enabled on rev 7.0+ unless it was explicitly 
disabled.

Re: [PATCH 2/2] mtd: rawnand: brcmnand: support "no-wp" DT property

[Florian Fainelli]

On 11/3/21 11:03 AM, Rafał Miłecki wrote:
> On 2021-11-03 18:50, Florian Fainelli wrote:
>> On 11/3/21 8:11 AM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> Some boards may use WP-capable controller but still have WP not
>>> connected. This change fixes:
>>> [    1.175550] bcm63138_nand ff801800.nand: timeout on status poll
>>> (expected c0000040 got c00000c0)
>>> [    1.184524] bcm63138_nand ff801800.nand: nand #WP expected on
>>> [    1.285547] bcm63138_nand ff801800.nand: timeout on status poll
>>> (expected c0000040 got c00000c0)
>>> [    1.294516] bcm63138_nand ff801800.nand: nand #WP expected on
>>> [    1.395548] bcm63138_nand ff801800.nand: timeout on status poll
>>> (expected c0000040 got c00000c0)
>>> [    1.404517] bcm63138_nand ff801800.nand: nand #WP expected on
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>> b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>> index f75929783b94..8b6167457f0c 100644
>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>> @@ -714,7 +714,8 @@ static int brcmnand_revision_init(struct
>>> brcmnand_controller *ctrl)
>>>      if (ctrl->nand_version >= 0x0500)
>>>          ctrl->features |= BRCMNAND_HAS_1K_SECTORS;
>>>
>>> -    if (ctrl->nand_version >= 0x0700)
>>> +    if (ctrl->nand_version >= 0x0700 &&
>>> +        !of_property_read_bool(ctrl->dev->of_node, "no-wp"))
>>>          ctrl->features |= BRCMNAND_HAS_WP;
>>
>> Should not this be a logical OR here or rather, should it be moved out
>> of the check on ctrl->nand_version entirely? What revision of the NAND
>> controller do you have on that chip?
> 
> It's NAND controller version 0x0701 (v7.1) and I think it's correct.
> 
> I think we want WP enabled on rev 7.0+ unless it was explicitly disabled.

True, I somehow got confused about the negative logic, so maybe we need
to combine the logic of all of these lines into a single one:

	if ((ctrl->nand_version >= 0x700 ||
of_property_read_bool(ctrl->dev->of_node, "brcm,nand-has-wp")) &&
!of_property_read_bool(ctrl->dev->of_node, "no-wp")

such that it is clearer, Kamal, what do you think?
-- 
Florian

Re: [PATCH 1/2] dt-bindings: mtd: brcmnand: add "no-wp" property

[Rafał Miłecki]

On 03.11.2021 16:11, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> It's required to properly describe boards without connected WP pin (e.g.
> Asus GT-AC5300).
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>   Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> index dd5a64969e37..49c7860c0dad 100644
> --- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> +++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> @@ -111,6 +111,11 @@ properties:
>         earlier versions of this core that include WP
>       type: boolean
>   
> +  no-wp:
> +    description:
> +      This property marks boards with WP pin not connected to the NAND chip.
> +    type: boolean

I started rethinking this. Since we already hav "brcm,nand-has-wp"
(boolean), would makes more sense:
1. Add "no-wp" boolean (as proposed in this patch)
2. Add "wp" (or similar) with [0, 1] and deprecate "brcm,nand-has-wp"

Re: [PATCH 1/2] dt-bindings: mtd: brcmnand: add "no-wp" property

[Miquel Raynal]

Hello,

rafal@milecki.pl wrote on Tue, 9 Nov 2021 08:49:36 +0100:

> On 03.11.2021 16:11, Rafał Miłecki wrote:
> > From: Rafał Miłecki <rafal@milecki.pl>
> > 
> > It's required to properly describe boards without connected WP pin (e.g.
> > Asus GT-AC5300).
> > 
> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> > ---
> >   Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> > index dd5a64969e37..49c7860c0dad 100644
> > --- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> > +++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> > @@ -111,6 +111,11 @@ properties:
> >         earlier versions of this core that include WP
> >       type: boolean  
> >   > +  no-wp:  
> > +    description:
> > +      This property marks boards with WP pin not connected to the NAND chip.
> > +    type: boolean  
> 
> I started rethinking this. Since we already hav "brcm,nand-has-wp"
> (boolean), would makes more sense:
> 1. Add "no-wp" boolean (as proposed in this patch)
> 2. Add "wp" (or similar) with [0, 1] and deprecate "brcm,nand-has-wp"

Maybe this should be a raw NAND wide property, at least in the bindings
for now: nand-wp (such as nand-rb) and this property should contain the
wp line id.

For me, brcm,nand-has-wp means that the nand wp is connected, not that
it "can be" connected. The fact that the controller has a wp pin or not
should be internal to the controller driver (different compatible or hw
version check).

Thanks,
Miquèl