[PATCH] mtd: rawnand: marvell: Use correct logic for nand-keep-config

[PATCH] mtd: rawnand: marvell: Use correct logic for nand-keep-config

[Chris Packham]

From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>

Originally the absence of the marvell,nand-keep-config property caused
the setup_data_interface function to be provided. However when
setup_data_interface was moved into nand_controller_ops the logic was
unintentionally inverted. Update the logic so that only if the
marvell,nand-keep-config property is present the bootloader NAND config
kept.

Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops")
Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    I think this is a bug that's been lurking for 4 years or so. I'm not
    sure that's particularly long in the life of an embedded device but it
    does make me wonder if there have been other bug reports about it.
    
    We noticed this because we had a bootloader that used maxed out NAND
    timings which made the time it took the kernel to do anything on the
    file system longer than we expected.

 drivers/mtd/nand/raw/marvell_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index 2455a581fd70..b248c5f657d5 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc,
 	chip->controller = &nfc->controller;
 	nand_set_flash_node(chip, np);
 
-	if (!of_property_read_bool(np, "marvell,nand-keep-config"))
+	if (of_property_read_bool(np, "marvell,nand-keep-config"))
 		chip->options |= NAND_KEEP_TIMINGS;
 
 	mtd = nand_to_mtd(chip);
-- 
2.37.3

Re: [PATCH] mtd: rawnand: marvell: Use correct logic for nand-keep-config

[Chris Packham]

On 27/09/22 15:47, Chris Packham wrote:
> From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
>
> Originally the absence of the marvell,nand-keep-config property caused
> the setup_data_interface function to be provided. However when
> setup_data_interface was moved into nand_controller_ops the logic was
> unintentionally inverted. Update the logic so that only if the
> marvell,nand-keep-config property is present the bootloader NAND config
> kept.
>
> Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops")
> Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>
> Notes:
>      I think this is a bug that's been lurking for 4 years or so. I'm not
>      sure that's particularly long in the life of an embedded device but it
>      does make me wonder if there have been other bug reports about it.
>      
>      We noticed this because we had a bootloader that used maxed out NAND
>      timings which made the time it took the kernel to do anything on the
>      file system longer than we expected.

I think there might be a similar logic inversion bug in 
drivers/mtd/nand/raw/denali.c but I lack the ability to test for that 
platform.

>   drivers/mtd/nand/raw/marvell_nand.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
> index 2455a581fd70..b248c5f657d5 100644
> --- a/drivers/mtd/nand/raw/marvell_nand.c
> +++ b/drivers/mtd/nand/raw/marvell_nand.c
> @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc,
>   	chip->controller = &nfc->controller;
>   	nand_set_flash_node(chip, np);
>   
> -	if (!of_property_read_bool(np, "marvell,nand-keep-config"))
> +	if (of_property_read_bool(np, "marvell,nand-keep-config"))
>   		chip->options |= NAND_KEEP_TIMINGS;
>   
>   	mtd = nand_to_mtd(chip);

Re: [PATCH] mtd: rawnand: marvell: Use correct logic for nand-keep-config

[Miquel Raynal]

Hi Chris,

Chris.Packham@alliedtelesis.co.nz wrote on Tue, 27 Sep 2022 02:54:40
+0000:

> On 27/09/22 15:47, Chris Packham wrote:
> > From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
> >
> > Originally the absence of the marvell,nand-keep-config property caused
> > the setup_data_interface function to be provided. However when
> > setup_data_interface was moved into nand_controller_ops the logic was
> > unintentionally inverted. Update the logic so that only if the
> > marvell,nand-keep-config property is present the bootloader NAND config
> > kept.
> >
> > Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops")
> > Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > ---
> >
> > Notes:
> >      I think this is a bug that's been lurking for 4 years or so. I'm not
> >      sure that's particularly long in the life of an embedded device but it
> >      does make me wonder if there have been other bug reports about it.
> >      
> >      We noticed this because we had a bootloader that used maxed out NAND
> >      timings which made the time it took the kernel to do anything on the
> >      file system longer than we expected.  
> 
> I think there might be a similar logic inversion bug in 
> drivers/mtd/nand/raw/denali.c but I lack the ability to test for that 
> platform.

Agreed, the denali driver has the same issue. Could you please send a
patch?
 
> 
> >   drivers/mtd/nand/raw/marvell_nand.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
> > index 2455a581fd70..b248c5f657d5 100644
> > --- a/drivers/mtd/nand/raw/marvell_nand.c
> > +++ b/drivers/mtd/nand/raw/marvell_nand.c
> > @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc,
> >   	chip->controller = &nfc->controller;
> >   	nand_set_flash_node(chip, np);
> >   
> > -	if (!of_property_read_bool(np, "marvell,nand-keep-config"))
> > +	if (of_property_read_bool(np, "marvell,nand-keep-config"))
> >   		chip->options |= NAND_KEEP_TIMINGS;
> >   
> >   	mtd = nand_to_mtd(chip)  


Thanks,
Miquèl

Re: [PATCH] mtd: rawnand: marvell: Use correct logic for nand-keep-config

[Chris Packham]

On 4/10/22 23:02, Miquel Raynal wrote:
> Hi Chris,
>
> Chris.Packham@alliedtelesis.co.nz wrote on Tue, 27 Sep 2022 02:54:40
> +0000:
>
>> On 27/09/22 15:47, Chris Packham wrote:
>>> From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
>>>
>>> Originally the absence of the marvell,nand-keep-config property caused
>>> the setup_data_interface function to be provided. However when
>>> setup_data_interface was moved into nand_controller_ops the logic was
>>> unintentionally inverted. Update the logic so that only if the
>>> marvell,nand-keep-config property is present the bootloader NAND config
>>> kept.
>>>
>>> Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops")
>>> Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> ---
>>>
>>> Notes:
>>>       I think this is a bug that's been lurking for 4 years or so. I'm not
>>>       sure that's particularly long in the life of an embedded device but it
>>>       does make me wonder if there have been other bug reports about it.
>>>       
>>>       We noticed this because we had a bootloader that used maxed out NAND
>>>       timings which made the time it took the kernel to do anything on the
>>>       file system longer than we expected.
>> I think there might be a similar logic inversion bug in
>> drivers/mtd/nand/raw/denali.c but I lack the ability to test for that
>> platform.
> Agreed, the denali driver has the same issue. Could you please send a
> patch?
Sure although it'll be compile tested only.
>>>    drivers/mtd/nand/raw/marvell_nand.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
>>> index 2455a581fd70..b248c5f657d5 100644
>>> --- a/drivers/mtd/nand/raw/marvell_nand.c
>>> +++ b/drivers/mtd/nand/raw/marvell_nand.c
>>> @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc,
>>>    	chip->controller = &nfc->controller;
>>>    	nand_set_flash_node(chip, np);
>>>    
>>> -	if (!of_property_read_bool(np, "marvell,nand-keep-config"))
>>> +	if (of_property_read_bool(np, "marvell,nand-keep-config"))
>>>    		chip->options |= NAND_KEEP_TIMINGS;
>>>    
>>>    	mtd = nand_to_mtd(chip)
>
> Thanks,
> Miquèl

Re: [PATCH] mtd: rawnand: marvell: Use correct logic for nand-keep-config

[Chris Packham]

On 5/10/22 08:41, Chris Packham wrote:
>
> On 4/10/22 23:02, Miquel Raynal wrote:
>> Hi Chris,
>>
>> Chris.Packham@alliedtelesis.co.nz wrote on Tue, 27 Sep 2022 02:54:40
>> +0000:
>>
>>> On 27/09/22 15:47, Chris Packham wrote:
>>>> From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
>>>>
>>>> Originally the absence of the marvell,nand-keep-config property caused
>>>> the setup_data_interface function to be provided. However when
>>>> setup_data_interface was moved into nand_controller_ops the logic was
>>>> unintentionally inverted. Update the logic so that only if the
>>>> marvell,nand-keep-config property is present the bootloader NAND 
>>>> config
>>>> kept.
>>>>
>>>> Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() 
>>>> to nand_controller_ops")
>>>> Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>> ---
>>>>
>>>> Notes:
>>>>       I think this is a bug that's been lurking for 4 years or so. 
>>>> I'm not
>>>>       sure that's particularly long in the life of an embedded 
>>>> device but it
>>>>       does make me wonder if there have been other bug reports 
>>>> about it.
>>>>             We noticed this because we had a bootloader that used 
>>>> maxed out NAND
>>>>       timings which made the time it took the kernel to do anything 
>>>> on the
>>>>       file system longer than we expected.
>>> I think there might be a similar logic inversion bug in
>>> drivers/mtd/nand/raw/denali.c but I lack the ability to test for that
>>> platform.
>> Agreed, the denali driver has the same issue. Could you please send a
>> patch?
> Sure although it'll be compile tested only.
Actually looks like it was already fixed in commit d311e0c27b8f ("mtd: 
rawnand: denali: get ->setup_data_interface() working again").
>>>>    drivers/mtd/nand/raw/marvell_nand.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/marvell_nand.c 
>>>> b/drivers/mtd/nand/raw/marvell_nand.c
>>>> index 2455a581fd70..b248c5f657d5 100644
>>>> --- a/drivers/mtd/nand/raw/marvell_nand.c
>>>> +++ b/drivers/mtd/nand/raw/marvell_nand.c
>>>> @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct 
>>>> device *dev, struct marvell_nfc *nfc,
>>>>        chip->controller = &nfc->controller;
>>>>        nand_set_flash_node(chip, np);
>>>>    -    if (!of_property_read_bool(np, "marvell,nand-keep-config"))
>>>> +    if (of_property_read_bool(np, "marvell,nand-keep-config"))
>>>>            chip->options |= NAND_KEEP_TIMINGS;
>>>>           mtd = nand_to_mtd(chip)
>>
>> Thanks,
>> Miquèl

Re: [PATCH] mtd: rawnand: marvell: Use correct logic for nand-keep-config

[Miquel Raynal]

Hi Chris,

Chris.Packham@alliedtelesis.co.nz wrote on Tue, 4 Oct 2022 21:21:37
+0000:

> On 5/10/22 08:41, Chris Packham wrote:
> >
> > On 4/10/22 23:02, Miquel Raynal wrote:  
> >> Hi Chris,
> >>
> >> Chris.Packham@alliedtelesis.co.nz wrote on Tue, 27 Sep 2022 02:54:40
> >> +0000:
> >>  
> >>> On 27/09/22 15:47, Chris Packham wrote:  
> >>>> From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
> >>>>
> >>>> Originally the absence of the marvell,nand-keep-config property caused
> >>>> the setup_data_interface function to be provided. However when
> >>>> setup_data_interface was moved into nand_controller_ops the logic was
> >>>> unintentionally inverted. Update the logic so that only if the
> >>>> marvell,nand-keep-config property is present the bootloader NAND 
> >>>> config
> >>>> kept.
> >>>>
> >>>> Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() 
> >>>> to nand_controller_ops")
> >>>> Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
> >>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> >>>> ---
> >>>>
> >>>> Notes:
> >>>>       I think this is a bug that's been lurking for 4 years or so. 
> >>>> I'm not
> >>>>       sure that's particularly long in the life of an embedded 
> >>>> device but it
> >>>>       does make me wonder if there have been other bug reports 
> >>>> about it.
> >>>>             We noticed this because we had a bootloader that used 
> >>>> maxed out NAND
> >>>>       timings which made the time it took the kernel to do anything 
> >>>> on the
> >>>>       file system longer than we expected.  
> >>> I think there might be a similar logic inversion bug in
> >>> drivers/mtd/nand/raw/denali.c but I lack the ability to test for that
> >>> platform.  
> >> Agreed, the denali driver has the same issue. Could you please send a
> >> patch?  
> > Sure although it'll be compile tested only.  
> Actually looks like it was already fixed in commit d311e0c27b8f ("mtd: 
> rawnand: denali: get ->setup_data_interface() working again").

Ok, perfect.

Thanks,
Miquèl

Re: [PATCH] mtd: rawnand: marvell: Use correct logic for nand-keep-config

[Chris Packham]

Hi All,

On 27/09/22 15:47, Chris Packham wrote:
> From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
>
> Originally the absence of the marvell,nand-keep-config property caused
> the setup_data_interface function to be provided. However when
> setup_data_interface was moved into nand_controller_ops the logic was
> unintentionally inverted. Update the logic so that only if the
> marvell,nand-keep-config property is present the bootloader NAND config
> kept.
>
> Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops")
> Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Just following up on this. I know things have probably been busy with 
the 6.0 release but it's been a week so I figured I'd give this a bump.

> ---
>
> Notes:
>      I think this is a bug that's been lurking for 4 years or so. I'm not
>      sure that's particularly long in the life of an embedded device but it
>      does make me wonder if there have been other bug reports about it.
>      
>      We noticed this because we had a bootloader that used maxed out NAND
>      timings which made the time it took the kernel to do anything on the
>      file system longer than we expected.
>
>   drivers/mtd/nand/raw/marvell_nand.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
> index 2455a581fd70..b248c5f657d5 100644
> --- a/drivers/mtd/nand/raw/marvell_nand.c
> +++ b/drivers/mtd/nand/raw/marvell_nand.c
> @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc,
>   	chip->controller = &nfc->controller;
>   	nand_set_flash_node(chip, np);
>   
> -	if (!of_property_read_bool(np, "marvell,nand-keep-config"))
> +	if (of_property_read_bool(np, "marvell,nand-keep-config"))
>   		chip->options |= NAND_KEEP_TIMINGS;
>   
>   	mtd = nand_to_mtd(chip);

Re: [PATCH] mtd: rawnand: marvell: Use correct logic for nand-keep-config

[Boris Brezillon]

On Mon, 3 Oct 2022 21:46:31 +0000
Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote:

> Hi All,
> 
> On 27/09/22 15:47, Chris Packham wrote:
> > From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
> >
> > Originally the absence of the marvell,nand-keep-config property caused
> > the setup_data_interface function to be provided. However when
> > setup_data_interface was moved into nand_controller_ops the logic was
> > unintentionally inverted. Update the logic so that only if the
> > marvell,nand-keep-config property is present the bootloader NAND config
> > kept.
> >
> > Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops")
> > Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>  
> 
> Just following up on this. I know things have probably been busy with 
> the 6.0 release but it's been a week so I figured I'd give this a bump.

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> 
> > ---
> >
> > Notes:
> >      I think this is a bug that's been lurking for 4 years or so. I'm not
> >      sure that's particularly long in the life of an embedded device but it
> >      does make me wonder if there have been other bug reports about it.
> >      
> >      We noticed this because we had a bootloader that used maxed out NAND
> >      timings which made the time it took the kernel to do anything on the
> >      file system longer than we expected.
> >
> >   drivers/mtd/nand/raw/marvell_nand.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
> > index 2455a581fd70..b248c5f657d5 100644
> > --- a/drivers/mtd/nand/raw/marvell_nand.c
> > +++ b/drivers/mtd/nand/raw/marvell_nand.c
> > @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc,
> >   	chip->controller = &nfc->controller;
> >   	nand_set_flash_node(chip, np);
> >   
> > -	if (!of_property_read_bool(np, "marvell,nand-keep-config"))
> > +	if (of_property_read_bool(np, "marvell,nand-keep-config"))
> >   		chip->options |= NAND_KEEP_TIMINGS;
> >   
> >   	mtd = nand_to_mtd(chip)  

Re: [PATCH] mtd: rawnand: marvell: Use correct logic for nand-keep-config

[Miquel Raynal]

Hi Chris,

Chris.Packham@alliedtelesis.co.nz wrote on Mon, 3 Oct 2022 21:46:31
+0000:

> Hi All,
> 
> On 27/09/22 15:47, Chris Packham wrote:
> > From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
> >
> > Originally the absence of the marvell,nand-keep-config property caused
> > the setup_data_interface function to be provided. However when
> > setup_data_interface was moved into nand_controller_ops the logic was
> > unintentionally inverted. Update the logic so that only if the
> > marvell,nand-keep-config property is present the bootloader NAND config
> > kept.
> >
> > Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops")
> > Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>  
> 
> Just following up on this. I know things have probably been busy with 
> the 6.0 release but it's been a week so I figured I'd give this a bump.

I was just off the past week :)

I will queue it soon.

> 
> > ---
> >
> > Notes:
> >      I think this is a bug that's been lurking for 4 years or so. I'm not
> >      sure that's particularly long in the life of an embedded device but it
> >      does make me wonder if there have been other bug reports about it.

I don't remember any... Indeed this must be fixed.

> >      We noticed this because we had a bootloader that used maxed out NAND
> >      timings which made the time it took the kernel to do anything on the
> >      file system longer than we expected.
> >
> >   drivers/mtd/nand/raw/marvell_nand.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
> > index 2455a581fd70..b248c5f657d5 100644
> > --- a/drivers/mtd/nand/raw/marvell_nand.c
> > +++ b/drivers/mtd/nand/raw/marvell_nand.c
> > @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc,
> >   	chip->controller = &nfc->controller;
> >   	nand_set_flash_node(chip, np);
> >   
> > -	if (!of_property_read_bool(np, "marvell,nand-keep-config"))
> > +	if (of_property_read_bool(np, "marvell,nand-keep-config"))
> >   		chip->options |= NAND_KEEP_TIMINGS;
> >   
> >   	mtd = nand_to_mtd(chip)  


Thanks,
Miquèl

Re: [PATCH] mtd: rawnand: marvell: Use correct logic for nand-keep-config

[Miquel Raynal]

On Tue, 2022-09-27 at 02:47:28 UTC, Chris Packham wrote:
> From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
> 
> Originally the absence of the marvell,nand-keep-config property caused
> the setup_data_interface function to be provided. However when
> setup_data_interface was moved into nand_controller_ops the logic was
> unintentionally inverted. Update the logic so that only if the
> marvell,nand-keep-config property is present the bootloader NAND config
> kept.
> 
> Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops")
> Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks.

Miquel