[PATCH] mtd: rawnand: marvell: ensure timing values are written

[PATCH] mtd: rawnand: marvell: ensure timing values are written

[Chris Packham]

When new timing values are calculated in marvell_nfc_setup_interface()
ensure that they will be applied in marvell_nfc_select_target() by
clearing the selected_chip pointer.

Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    This at least gets me to a point where I can illustrated the problem
    reported to me. It appears that despite the chip correctly reporting
    support for SDR timing modes up to 4 the observed tWC is 20ns. I've not
    seen any actual problem running in this state the only complaint is that
    the datasheet says the minimum tWC is 25ns.
    
    If I make a change to my bootloader such that the NAND Clock Frequency
    Select bit (0xF2440700:0) to 1 before booting the kernel _and_ I remove
    the extra factor of 2 from the period_ns calculation I observe tWC of
    about 60ns. If I don't remove the factor of 2 the NAND interface doesn't
    work (can't write BBT).

 drivers/mtd/nand/raw/marvell_nand.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index afb424579f0b..3b5e4d5d220f 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -2457,6 +2457,12 @@ static int marvell_nfc_setup_interface(struct nand_chip *chip, int chipnr,
 			NDTR1_WAIT_MODE;
 	}
 
+	/*
+	 * Reset nfc->selected_chip so the next command will cause the timing
+	 * registers to be restored in marvell_nfc_select_target().
+	 */
+	nfc->selected_chip = NULL;
+
 	return 0;
 }
 
-- 
2.40.1

Re: [PATCH] mtd: rawnand: marvell: ensure timing values are written

[Miquel Raynal]

Hi Chris,

chris.packham@alliedtelesis.co.nz wrote on Tue, 23 May 2023 15:21:03
+1200:

> When new timing values are calculated in marvell_nfc_setup_interface()
> ensure that they will be applied in marvell_nfc_select_target() by
> clearing the selected_chip pointer.
> 
> Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

I believe this patch deserves Fixes+Cc:stable?

> ---
> 
> Notes:
>     This at least gets me to a point where I can illustrated the problem
>     reported to me. It appears that despite the chip correctly reporting
>     support for SDR timing modes up to 4 the observed tWC is 20ns. I've not
>     seen any actual problem running in this state the only complaint is that
>     the datasheet says the minimum tWC is 25ns.
>     
>     If I make a change to my bootloader such that the NAND Clock Frequency
>     Select bit (0xF2440700:0) to 1 before booting the kernel _and_ I remove
>     the extra factor of 2 from the period_ns calculation I observe tWC of
>     about 60ns. If I don't remove the factor of 2 the NAND interface doesn't
>     work (can't write BBT).
> 
>  drivers/mtd/nand/raw/marvell_nand.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
> index afb424579f0b..3b5e4d5d220f 100644
> --- a/drivers/mtd/nand/raw/marvell_nand.c
> +++ b/drivers/mtd/nand/raw/marvell_nand.c
> @@ -2457,6 +2457,12 @@ static int marvell_nfc_setup_interface(struct nand_chip *chip, int chipnr,
>  			NDTR1_WAIT_MODE;
>  	}
>  
> +	/*
> +	 * Reset nfc->selected_chip so the next command will cause the timing
> +	 * registers to be restored in marvell_nfc_select_target().
> +	 */

s/restored/updated/ ?

Restored feels like the content vanished.

> +	nfc->selected_chip = NULL;
> +
>  	return 0;
>  }
>  


Thanks,
Miquèl

Re: [PATCH] mtd: rawnand: marvell: ensure timing values are written

[Chris Packham]

On 23/05/23 21:39, Miquel Raynal wrote:
> Hi Chris,
>
> chris.packham@alliedtelesis.co.nz wrote on Tue, 23 May 2023 15:21:03
> +1200:
>
>> When new timing values are calculated in marvell_nfc_setup_interface()
>> ensure that they will be applied in marvell_nfc_select_target() by
>> clearing the selected_chip pointer.
>>
>> Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> I believe this patch deserves Fixes+Cc:stable?

I agree. I just wasn't sure what to point at with the fixes tag since 
you mentioned that it's probably a result in some of the core NAND 
infrastructure changing.

Maybe b25251414f6e00 ("mtd: rawnand: marvell: Stop implementing 
->select_chip()") is the correct change to point at.

>> ---
>>
>> Notes:
>>      This at least gets me to a point where I can illustrated the problem
>>      reported to me. It appears that despite the chip correctly reporting
>>      support for SDR timing modes up to 4 the observed tWC is 20ns. I've not
>>      seen any actual problem running in this state the only complaint is that
>>      the datasheet says the minimum tWC is 25ns.
>>      
>>      If I make a change to my bootloader such that the NAND Clock Frequency
>>      Select bit (0xF2440700:0) to 1 before booting the kernel _and_ I remove
>>      the extra factor of 2 from the period_ns calculation I observe tWC of
>>      about 60ns. If I don't remove the factor of 2 the NAND interface doesn't
>>      work (can't write BBT).
>>
>>   drivers/mtd/nand/raw/marvell_nand.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
>> index afb424579f0b..3b5e4d5d220f 100644
>> --- a/drivers/mtd/nand/raw/marvell_nand.c
>> +++ b/drivers/mtd/nand/raw/marvell_nand.c
>> @@ -2457,6 +2457,12 @@ static int marvell_nfc_setup_interface(struct nand_chip *chip, int chipnr,
>>   			NDTR1_WAIT_MODE;
>>   	}
>>   
>> +	/*
>> +	 * Reset nfc->selected_chip so the next command will cause the timing
>> +	 * registers to be restored in marvell_nfc_select_target().
>> +	 */
> s/restored/updated/ ?
>
> Restored feels like the content vanished.
OK. Will send a v2 later today.
>> +	nfc->selected_chip = NULL;
>> +
>>   	return 0;
>>   }
>>   
>
> Thanks,
> Miquèl

Re: [PATCH] mtd: rawnand: marvell: ensure timing values are written

[Miquel Raynal]

Hi Chris,

Chris.Packham@alliedtelesis.co.nz wrote on Tue, 23 May 2023 20:42:45
+0000:

> On 23/05/23 21:39, Miquel Raynal wrote:
> > Hi Chris,
> >
> > chris.packham@alliedtelesis.co.nz wrote on Tue, 23 May 2023 15:21:03
> > +1200:
> >  
> >> When new timing values are calculated in marvell_nfc_setup_interface()
> >> ensure that they will be applied in marvell_nfc_select_target() by
> >> clearing the selected_chip pointer.
> >>
> >> Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>  
> > I believe this patch deserves Fixes+Cc:stable?  
> 
> I agree. I just wasn't sure what to point at with the fixes tag since 
> you mentioned that it's probably a result in some of the core NAND 
> infrastructure changing.
> 
> Maybe b25251414f6e00 ("mtd: rawnand: marvell: Stop implementing 
> ->select_chip()") is the correct change to point at.  

Sounds reasonable.

> 
> >> ---
> >>
> >> Notes:
> >>      This at least gets me to a point where I can illustrated the problem
> >>      reported to me. It appears that despite the chip correctly reporting
> >>      support for SDR timing modes up to 4 the observed tWC is 20ns. I've not
> >>      seen any actual problem running in this state the only complaint is that
> >>      the datasheet says the minimum tWC is 25ns.
> >>      
> >>      If I make a change to my bootloader such that the NAND Clock Frequency
> >>      Select bit (0xF2440700:0) to 1 before booting the kernel _and_ I remove
> >>      the extra factor of 2 from the period_ns calculation I observe tWC of
> >>      about 60ns. If I don't remove the factor of 2 the NAND interface doesn't
> >>      work (can't write BBT).
> >>
> >>   drivers/mtd/nand/raw/marvell_nand.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
> >> index afb424579f0b..3b5e4d5d220f 100644
> >> --- a/drivers/mtd/nand/raw/marvell_nand.c
> >> +++ b/drivers/mtd/nand/raw/marvell_nand.c
> >> @@ -2457,6 +2457,12 @@ static int marvell_nfc_setup_interface(struct nand_chip *chip, int chipnr,
> >>   			NDTR1_WAIT_MODE;
> >>   	}
> >>   
> >> +	/*
> >> +	 * Reset nfc->selected_chip so the next command will cause the timing
> >> +	 * registers to be restored in marvell_nfc_select_target().
> >> +	 */  
> > s/restored/updated/ ?
> >
> > Restored feels like the content vanished.  
> OK. Will send a v2 later today.
> >> +	nfc->selected_chip = NULL;
> >> +
> >>   	return 0;
> >>   }
> >>     
> >
> > Thanks,
> > Miquè  


Thanks,
Miquèl