Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
@ 2020-02-05  7:08 Marek Vasut
  2020-02-05  9:12 ` Miquel Raynal
  0 siblings, 1 reply; 31+ messages in thread
From: Marek Vasut @ 2020-02-05  7:08 UTC (permalink / raw)
  To: linux-mtd
  Cc: Marek Vasut, Masahiro Yamada, Dinh Nguyen, Boris Brezillon,
	Tim Sander, Miquel Raynal

This reverts commit d311e0c27b8fcc27f707f8cac48cd8bdc4155224, which
completely breaks NAND access on Altera SoCFPGA (detected on ArriaV
SoC).

On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
hence the driver sets NAND_KEEP_TIMINGS flag. This did not happen before
and is actually incorrect, as on SoCFPGA we do not want to retain timings
from previous stage (the timings might be incorrect or outright invalid).

Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Dinh Nguyen <dinguyen@kernel.org>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Tim Sander <tim@krieglstein.org>
To: linux-mtd <linux-mtd@lists.infradead.org>
---
 drivers/mtd/nand/raw/denali.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
index b6c463d02167..5fe3c62a756e 100644
--- a/drivers/mtd/nand/raw/denali.c
+++ b/drivers/mtd/nand/raw/denali.c
@@ -1209,7 +1209,7 @@ int denali_chip_init(struct denali_controller *denali,
 	}
 
 	/* clk rate info is needed for setup_data_interface */
-	if (!denali->clk_rate || !denali->clk_x_rate)
+	if (denali->clk_rate && denali->clk_x_rate)
 		chip->options |= NAND_KEEP_TIMINGS;
 
 	chip->bbt_options |= NAND_BBT_USE_FLASH;
-- 
2.24.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-05  7:08 [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again" Marek Vasut
@ 2020-02-05  9:12 ` Miquel Raynal
  2020-02-05  9:41   ` Marek Vasut
  0 siblings, 1 reply; 31+ messages in thread
From: Miquel Raynal @ 2020-02-05  9:12 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Masahiro Yamada, Boris Brezillon, linux-mtd, Tim Sander

Hi Marek,

Marek Vasut <marex@denx.de> wrote on Wed,  5 Feb 2020 08:08:34 +0100:

> This reverts commit d311e0c27b8fcc27f707f8cac48cd8bdc4155224, which
> completely breaks NAND access on Altera SoCFPGA (detected on ArriaV
> SoC).
> 
> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
> hence the driver sets NAND_KEEP_TIMINGS flag. This did not happen before
> and is actually incorrect, as on SoCFPGA we do not want to retain timings
> from previous stage (the timings might be incorrect or outright invalid).
> 
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Dinh Nguyen <dinguyen@kernel.org>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Tim Sander <tim@krieglstein.org>
> To: linux-mtd <linux-mtd@lists.infradead.org>
> ---
>  drivers/mtd/nand/raw/denali.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> index b6c463d02167..5fe3c62a756e 100644
> --- a/drivers/mtd/nand/raw/denali.c
> +++ b/drivers/mtd/nand/raw/denali.c
> @@ -1209,7 +1209,7 @@ int denali_chip_init(struct denali_controller *denali,
>  	}
>  
>  	/* clk rate info is needed for setup_data_interface */
> -	if (!denali->clk_rate || !denali->clk_x_rate)

I don't get it, if both clk_rate and clk_x_rate are set, the if
condition will not be entered, right?

> +	if (denali->clk_rate && denali->clk_x_rate)
>  		chip->options |= NAND_KEEP_TIMINGS;


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-05  9:12 ` Miquel Raynal
@ 2020-02-05  9:41   ` Marek Vasut
  2020-02-05  9:50     ` Miquel Raynal
  0 siblings, 1 reply; 31+ messages in thread
From: Marek Vasut @ 2020-02-05  9:41 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Dinh Nguyen, Masahiro Yamada, Boris Brezillon, linux-mtd, Tim Sander

On 2/5/20 10:12 AM, Miquel Raynal wrote:
> Hi Marek,
> 
> Marek Vasut <marex@denx.de> wrote on Wed,  5 Feb 2020 08:08:34 +0100:
> 
>> This reverts commit d311e0c27b8fcc27f707f8cac48cd8bdc4155224, which
>> completely breaks NAND access on Altera SoCFPGA (detected on ArriaV
>> SoC).
>>
>> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
>> hence the driver sets NAND_KEEP_TIMINGS flag. This did not happen before
>> and is actually incorrect, as on SoCFPGA we do not want to retain timings
>> from previous stage (the timings might be incorrect or outright invalid).
>>
>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
>> Cc: Dinh Nguyen <dinguyen@kernel.org>
>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>> Cc: Tim Sander <tim@krieglstein.org>
>> To: linux-mtd <linux-mtd@lists.infradead.org>
>> ---
>>  drivers/mtd/nand/raw/denali.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
>> index b6c463d02167..5fe3c62a756e 100644
>> --- a/drivers/mtd/nand/raw/denali.c
>> +++ b/drivers/mtd/nand/raw/denali.c
>> @@ -1209,7 +1209,7 @@ int denali_chip_init(struct denali_controller *denali,
>>  	}
>>  
>>  	/* clk rate info is needed for setup_data_interface */
>> -	if (!denali->clk_rate || !denali->clk_x_rate)
> 
> I don't get it, if both clk_rate and clk_x_rate are set, the if
> condition will not be entered, right?

Err, then it's the other way around and I need to keep the timings on
socfpga ?

>> +	if (denali->clk_rate && denali->clk_x_rate)
>>  		chip->options |= NAND_KEEP_TIMINGS;

[...]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-05  9:41   ` Marek Vasut
@ 2020-02-05  9:50     ` Miquel Raynal
  2020-02-05 10:05       ` Boris Brezillon
  2020-02-05 10:08       ` Marek Vasut
  0 siblings, 2 replies; 31+ messages in thread
From: Miquel Raynal @ 2020-02-05  9:50 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Masahiro Yamada, Boris Brezillon, linux-mtd, Tim Sander

Hi Marek,

Marek Vasut <marex@denx.de> wrote on Wed, 5 Feb 2020 10:41:05 +0100:

> On 2/5/20 10:12 AM, Miquel Raynal wrote:
> > Hi Marek,
> > 
> > Marek Vasut <marex@denx.de> wrote on Wed,  5 Feb 2020 08:08:34 +0100:
> >   
> >> This reverts commit d311e0c27b8fcc27f707f8cac48cd8bdc4155224, which
> >> completely breaks NAND access on Altera SoCFPGA (detected on ArriaV
> >> SoC).
> >>
> >> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
> >> hence the driver sets NAND_KEEP_TIMINGS flag. This did not happen before
> >> and is actually incorrect, as on SoCFPGA we do not want to retain timings
> >> from previous stage (the timings might be incorrect or outright invalid).
> >>
> >> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> >> Cc: Dinh Nguyen <dinguyen@kernel.org>
> >> Cc: Masahiro Yamada <masahiroy@kernel.org>
> >> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> >> Cc: Tim Sander <tim@krieglstein.org>
> >> To: linux-mtd <linux-mtd@lists.infradead.org>
> >> ---
> >>  drivers/mtd/nand/raw/denali.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> >> index b6c463d02167..5fe3c62a756e 100644
> >> --- a/drivers/mtd/nand/raw/denali.c
> >> +++ b/drivers/mtd/nand/raw/denali.c
> >> @@ -1209,7 +1209,7 @@ int denali_chip_init(struct denali_controller *denali,
> >>  	}
> >>  
> >>  	/* clk rate info is needed for setup_data_interface */
> >> -	if (!denali->clk_rate || !denali->clk_x_rate)  
> > 
> > I don't get it, if both clk_rate and clk_x_rate are set, the if
> > condition will not be entered, right?  
> 
> Err, then it's the other way around and I need to keep the timings on
> socfpga ?

Ok.

Do you have a different compatible? Or a register to check? How do you
discriminate the different platforms by software? The quick and dirty
solution is to add a special case for your platform and specifically
use the NAND_KEEP_TIMINGS horror.

But I think using ->software_data_interface is the right solution. So
I would highly recommend fixing the implementation of this hook
for your platform and in this case the commit reverted is not the
culprit, the one introducing setup_data_interface is (for the Fixes:
tag).

> 
> >> +	if (denali->clk_rate && denali->clk_x_rate)
> >>  		chip->options |= NAND_KEEP_TIMINGS;  
> 
> [...]

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-05  9:50     ` Miquel Raynal
@ 2020-02-05 10:05       ` Boris Brezillon
  2020-02-05 10:08       ` Marek Vasut
  1 sibling, 0 replies; 31+ messages in thread
From: Boris Brezillon @ 2020-02-05 10:05 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Marek Vasut, Dinh Nguyen, Masahiro Yamada, linux-mtd, Tim Sander

On Wed, 5 Feb 2020 10:50:45 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Marek,
> 
> Marek Vasut <marex@denx.de> wrote on Wed, 5 Feb 2020 10:41:05 +0100:
> 
> > On 2/5/20 10:12 AM, Miquel Raynal wrote:  
> > > Hi Marek,
> > > 
> > > Marek Vasut <marex@denx.de> wrote on Wed,  5 Feb 2020 08:08:34 +0100:
> > >     
> > >> This reverts commit d311e0c27b8fcc27f707f8cac48cd8bdc4155224, which
> > >> completely breaks NAND access on Altera SoCFPGA (detected on ArriaV
> > >> SoC).
> > >>
> > >> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
> > >> hence the driver sets NAND_KEEP_TIMINGS flag. This did not happen before
> > >> and is actually incorrect, as on SoCFPGA we do not want to retain timings
> > >> from previous stage (the timings might be incorrect or outright invalid).
> > >>
> > >> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> > >> Cc: Dinh Nguyen <dinguyen@kernel.org>
> > >> Cc: Masahiro Yamada <masahiroy@kernel.org>
> > >> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > >> Cc: Tim Sander <tim@krieglstein.org>
> > >> To: linux-mtd <linux-mtd@lists.infradead.org>
> > >> ---
> > >>  drivers/mtd/nand/raw/denali.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> > >> index b6c463d02167..5fe3c62a756e 100644
> > >> --- a/drivers/mtd/nand/raw/denali.c
> > >> +++ b/drivers/mtd/nand/raw/denali.c
> > >> @@ -1209,7 +1209,7 @@ int denali_chip_init(struct denali_controller *denali,
> > >>  	}
> > >>  
> > >>  	/* clk rate info is needed for setup_data_interface */
> > >> -	if (!denali->clk_rate || !denali->clk_x_rate)    
> > > 
> > > I don't get it, if both clk_rate and clk_x_rate are set, the if
> > > condition will not be entered, right?    
> > 
> > Err, then it's the other way around and I need to keep the timings on
> > socfpga ?  
> 
> Ok.
> 
> Do you have a different compatible? Or a register to check? How do you
> discriminate the different platforms by software? The quick and dirty
> solution is to add a special case for your platform and specifically
> use the NAND_KEEP_TIMINGS horror.
> 
> But I think using ->software_data_interface is the right solution. So

You probably mean ->setup_data_interface() :-).

> I would highly recommend fixing the implementation of this hook
> for your platform and in this case the commit reverted is not the
> culprit, the one introducing setup_data_interface is (for the Fixes:
> tag).

+1. If ->setup_data_interface() is buggy, it should be fixed.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-05  9:50     ` Miquel Raynal
  2020-02-05 10:05       ` Boris Brezillon
@ 2020-02-05 10:08       ` Marek Vasut
  2020-02-11 10:04         ` Marek Vasut
  1 sibling, 1 reply; 31+ messages in thread
From: Marek Vasut @ 2020-02-05 10:08 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Dinh Nguyen, Masahiro Yamada, Boris Brezillon, linux-mtd, Tim Sander

On 2/5/20 10:50 AM, Miquel Raynal wrote:
> Hi Marek,
> 
> Marek Vasut <marex@denx.de> wrote on Wed, 5 Feb 2020 10:41:05 +0100:
> 
>> On 2/5/20 10:12 AM, Miquel Raynal wrote:
>>> Hi Marek,
>>>
>>> Marek Vasut <marex@denx.de> wrote on Wed,  5 Feb 2020 08:08:34 +0100:
>>>   
>>>> This reverts commit d311e0c27b8fcc27f707f8cac48cd8bdc4155224, which
>>>> completely breaks NAND access on Altera SoCFPGA (detected on ArriaV
>>>> SoC).
>>>>
>>>> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
>>>> hence the driver sets NAND_KEEP_TIMINGS flag. This did not happen before
>>>> and is actually incorrect, as on SoCFPGA we do not want to retain timings
>>>> from previous stage (the timings might be incorrect or outright invalid).
>>>>
>>>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
>>>> Cc: Dinh Nguyen <dinguyen@kernel.org>
>>>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>>> Cc: Tim Sander <tim@krieglstein.org>
>>>> To: linux-mtd <linux-mtd@lists.infradead.org>
>>>> ---
>>>>  drivers/mtd/nand/raw/denali.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
>>>> index b6c463d02167..5fe3c62a756e 100644
>>>> --- a/drivers/mtd/nand/raw/denali.c
>>>> +++ b/drivers/mtd/nand/raw/denali.c
>>>> @@ -1209,7 +1209,7 @@ int denali_chip_init(struct denali_controller *denali,
>>>>  	}
>>>>  
>>>>  	/* clk rate info is needed for setup_data_interface */
>>>> -	if (!denali->clk_rate || !denali->clk_x_rate)  
>>>
>>> I don't get it, if both clk_rate and clk_x_rate are set, the if
>>> condition will not be entered, right?  
>>
>> Err, then it's the other way around and I need to keep the timings on
>> socfpga ?
> 
> Ok.
> 
> Do you have a different compatible? Or a register to check? How do you
> discriminate the different platforms by software? The quick and dirty
> solution is to add a special case for your platform and specifically
> use the NAND_KEEP_TIMINGS horror.

Sure, there's a socfpga compatible and at least two for uniphier.

> But I think using ->software_data_interface is the right solution. So
> I would highly recommend fixing the implementation of this hook
> for your platform and in this case the commit reverted is not the
> culprit, the one introducing setup_data_interface is (for the Fixes:
> tag).

I'll leave the details to Yamada-san.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-05 10:08       ` Marek Vasut
@ 2020-02-11 10:04         ` Marek Vasut
  2020-02-11 16:07           ` Miquel Raynal
  0 siblings, 1 reply; 31+ messages in thread
From: Marek Vasut @ 2020-02-11 10:04 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Dinh Nguyen, Masahiro Yamada, Boris Brezillon, linux-mtd, Tim Sander

On 2/5/20 11:08 AM, Marek Vasut wrote:
> On 2/5/20 10:50 AM, Miquel Raynal wrote:
>> Hi Marek,
>>
>> Marek Vasut <marex@denx.de> wrote on Wed, 5 Feb 2020 10:41:05 +0100:
>>
>>> On 2/5/20 10:12 AM, Miquel Raynal wrote:
>>>> Hi Marek,
>>>>
>>>> Marek Vasut <marex@denx.de> wrote on Wed,  5 Feb 2020 08:08:34 +0100:
>>>>   
>>>>> This reverts commit d311e0c27b8fcc27f707f8cac48cd8bdc4155224, which
>>>>> completely breaks NAND access on Altera SoCFPGA (detected on ArriaV
>>>>> SoC).
>>>>>
>>>>> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
>>>>> hence the driver sets NAND_KEEP_TIMINGS flag. This did not happen before
>>>>> and is actually incorrect, as on SoCFPGA we do not want to retain timings
>>>>> from previous stage (the timings might be incorrect or outright invalid).
>>>>>
>>>>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
>>>>> Cc: Dinh Nguyen <dinguyen@kernel.org>
>>>>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>> Cc: Tim Sander <tim@krieglstein.org>
>>>>> To: linux-mtd <linux-mtd@lists.infradead.org>
>>>>> ---
>>>>>  drivers/mtd/nand/raw/denali.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
>>>>> index b6c463d02167..5fe3c62a756e 100644
>>>>> --- a/drivers/mtd/nand/raw/denali.c
>>>>> +++ b/drivers/mtd/nand/raw/denali.c
>>>>> @@ -1209,7 +1209,7 @@ int denali_chip_init(struct denali_controller *denali,
>>>>>  	}
>>>>>  
>>>>>  	/* clk rate info is needed for setup_data_interface */
>>>>> -	if (!denali->clk_rate || !denali->clk_x_rate)  
>>>>
>>>> I don't get it, if both clk_rate and clk_x_rate are set, the if
>>>> condition will not be entered, right?  
>>>
>>> Err, then it's the other way around and I need to keep the timings on
>>> socfpga ?
>>
>> Ok.
>>
>> Do you have a different compatible? Or a register to check? How do you
>> discriminate the different platforms by software? The quick and dirty
>> solution is to add a special case for your platform and specifically
>> use the NAND_KEEP_TIMINGS horror.
> 
> Sure, there's a socfpga compatible and at least two for uniphier.
> 
>> But I think using ->software_data_interface is the right solution. So
>> I would highly recommend fixing the implementation of this hook
>> for your platform and in this case the commit reverted is not the
>> culprit, the one introducing setup_data_interface is (for the Fixes:
>> tag).
> 
> I'll leave the details to Yamada-san.

Just got a confirmation that this fixes NAND behavior on SoCFPGA, so
this patch should go in in some form.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-11 10:04         ` Marek Vasut
@ 2020-02-11 16:07           ` Miquel Raynal
  2020-02-11 20:35             ` Marek Vasut
  0 siblings, 1 reply; 31+ messages in thread
From: Miquel Raynal @ 2020-02-11 16:07 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Masahiro Yamada, Boris Brezillon, linux-mtd, Tim Sander

Hi Marek, Masahiro,

Marek Vasut <marex@denx.de> wrote on Tue, 11 Feb 2020 11:04:10 +0100:

> On 2/5/20 11:08 AM, Marek Vasut wrote:
> > On 2/5/20 10:50 AM, Miquel Raynal wrote:  
> >> Hi Marek,
> >>
> >> Marek Vasut <marex@denx.de> wrote on Wed, 5 Feb 2020 10:41:05 +0100:
> >>  
> >>> On 2/5/20 10:12 AM, Miquel Raynal wrote:  
> >>>> Hi Marek,
> >>>>
> >>>> Marek Vasut <marex@denx.de> wrote on Wed,  5 Feb 2020 08:08:34 +0100:
> >>>>     
> >>>>> This reverts commit d311e0c27b8fcc27f707f8cac48cd8bdc4155224, which
> >>>>> completely breaks NAND access on Altera SoCFPGA (detected on ArriaV
> >>>>> SoC).
> >>>>>
> >>>>> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
> >>>>> hence the driver sets NAND_KEEP_TIMINGS flag. This did not happen before
> >>>>> and is actually incorrect, as on SoCFPGA we do not want to retain timings
> >>>>> from previous stage (the timings might be incorrect or outright invalid).
> >>>>>
> >>>>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> >>>>> Cc: Dinh Nguyen <dinguyen@kernel.org>
> >>>>> Cc: Masahiro Yamada <masahiroy@kernel.org>
> >>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> >>>>> Cc: Tim Sander <tim@krieglstein.org>
> >>>>> To: linux-mtd <linux-mtd@lists.infradead.org>
> >>>>> ---
> >>>>>  drivers/mtd/nand/raw/denali.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> >>>>> index b6c463d02167..5fe3c62a756e 100644
> >>>>> --- a/drivers/mtd/nand/raw/denali.c
> >>>>> +++ b/drivers/mtd/nand/raw/denali.c
> >>>>> @@ -1209,7 +1209,7 @@ int denali_chip_init(struct denali_controller *denali,
> >>>>>  	}
> >>>>>  
> >>>>>  	/* clk rate info is needed for setup_data_interface */
> >>>>> -	if (!denali->clk_rate || !denali->clk_x_rate)    
> >>>>
> >>>> I don't get it, if both clk_rate and clk_x_rate are set, the if
> >>>> condition will not be entered, right?    
> >>>
> >>> Err, then it's the other way around and I need to keep the timings on
> >>> socfpga ?  
> >>
> >> Ok.
> >>
> >> Do you have a different compatible? Or a register to check? How do you
> >> discriminate the different platforms by software? The quick and dirty
> >> solution is to add a special case for your platform and specifically
> >> use the NAND_KEEP_TIMINGS horror.  
> > 
> > Sure, there's a socfpga compatible and at least two for uniphier.
> >   
> >> But I think using ->software_data_interface is the right solution. So
> >> I would highly recommend fixing the implementation of this hook
> >> for your platform and in this case the commit reverted is not the
> >> culprit, the one introducing setup_data_interface is (for the Fixes:
> >> tag).  
> > 
> > I'll leave the details to Yamada-san.  
> 
> Just got a confirmation that this fixes NAND behavior on SoCFPGA, so
> this patch should go in in some form.

I'm sure it fixes it, but it is definitely not going in the right
direction!

The right thing to do is fixing ->setup_data_interface().

The bad thing to do if someone tells me that he will fix
->setup_data_interface() in a second time is to keep the
NAND_KEEP_TIMINGS flag only for a single compatible.


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-11 16:07           ` Miquel Raynal
@ 2020-02-11 20:35             ` Marek Vasut
  2020-02-12  9:00               ` Masahiro Yamada
  0 siblings, 1 reply; 31+ messages in thread
From: Marek Vasut @ 2020-02-11 20:35 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Dinh Nguyen, Masahiro Yamada, Boris Brezillon, linux-mtd, Tim Sander

On 2/11/20 5:07 PM, Miquel Raynal wrote:
> Hi Marek, Masahiro,
> 
> Marek Vasut <marex@denx.de> wrote on Tue, 11 Feb 2020 11:04:10 +0100:
> 
>> On 2/5/20 11:08 AM, Marek Vasut wrote:
>>> On 2/5/20 10:50 AM, Miquel Raynal wrote:  
>>>> Hi Marek,
>>>>
>>>> Marek Vasut <marex@denx.de> wrote on Wed, 5 Feb 2020 10:41:05 +0100:
>>>>  
>>>>> On 2/5/20 10:12 AM, Miquel Raynal wrote:  
>>>>>> Hi Marek,
>>>>>>
>>>>>> Marek Vasut <marex@denx.de> wrote on Wed,  5 Feb 2020 08:08:34 +0100:
>>>>>>     
>>>>>>> This reverts commit d311e0c27b8fcc27f707f8cac48cd8bdc4155224, which
>>>>>>> completely breaks NAND access on Altera SoCFPGA (detected on ArriaV
>>>>>>> SoC).
>>>>>>>
>>>>>>> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
>>>>>>> hence the driver sets NAND_KEEP_TIMINGS flag. This did not happen before
>>>>>>> and is actually incorrect, as on SoCFPGA we do not want to retain timings
>>>>>>> from previous stage (the timings might be incorrect or outright invalid).
>>>>>>>
>>>>>>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
>>>>>>> Cc: Dinh Nguyen <dinguyen@kernel.org>
>>>>>>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>>>>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>>>> Cc: Tim Sander <tim@krieglstein.org>
>>>>>>> To: linux-mtd <linux-mtd@lists.infradead.org>
>>>>>>> ---
>>>>>>>  drivers/mtd/nand/raw/denali.c | 2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
>>>>>>> index b6c463d02167..5fe3c62a756e 100644
>>>>>>> --- a/drivers/mtd/nand/raw/denali.c
>>>>>>> +++ b/drivers/mtd/nand/raw/denali.c
>>>>>>> @@ -1209,7 +1209,7 @@ int denali_chip_init(struct denali_controller *denali,
>>>>>>>  	}
>>>>>>>  
>>>>>>>  	/* clk rate info is needed for setup_data_interface */
>>>>>>> -	if (!denali->clk_rate || !denali->clk_x_rate)    
>>>>>>
>>>>>> I don't get it, if both clk_rate and clk_x_rate are set, the if
>>>>>> condition will not be entered, right?    
>>>>>
>>>>> Err, then it's the other way around and I need to keep the timings on
>>>>> socfpga ?  
>>>>
>>>> Ok.
>>>>
>>>> Do you have a different compatible? Or a register to check? How do you
>>>> discriminate the different platforms by software? The quick and dirty
>>>> solution is to add a special case for your platform and specifically
>>>> use the NAND_KEEP_TIMINGS horror.  
>>>
>>> Sure, there's a socfpga compatible and at least two for uniphier.
>>>   
>>>> But I think using ->software_data_interface is the right solution. So
>>>> I would highly recommend fixing the implementation of this hook
>>>> for your platform and in this case the commit reverted is not the
>>>> culprit, the one introducing setup_data_interface is (for the Fixes:
>>>> tag).  
>>>
>>> I'll leave the details to Yamada-san.  
>>
>> Just got a confirmation that this fixes NAND behavior on SoCFPGA, so
>> this patch should go in in some form.
> 
> I'm sure it fixes it, but it is definitely not going in the right
> direction!
> 
> The right thing to do is fixing ->setup_data_interface().
> 
> The bad thing to do if someone tells me that he will fix
> ->setup_data_interface() in a second time is to keep the
> NAND_KEEP_TIMINGS flag only for a single compatible.

OK, I'll leave that to Yamada-san. I don't really want to interfere with
his work on the Denali NAND driver.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-11 20:35             ` Marek Vasut
@ 2020-02-12  9:00               ` Masahiro Yamada
  2020-02-12  9:37                 ` Marek Vasut
  0 siblings, 1 reply; 31+ messages in thread
From: Masahiro Yamada @ 2020-02-12  9:00 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

Hi Marek,


On Wed, Feb 12, 2020 at 5:35 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/11/20 5:07 PM, Miquel Raynal wrote:
> > Hi Marek, Masahiro,
> >
> > Marek Vasut <marex@denx.de> wrote on Tue, 11 Feb 2020 11:04:10 +0100:
> >
> >> On 2/5/20 11:08 AM, Marek Vasut wrote:
> >>> On 2/5/20 10:50 AM, Miquel Raynal wrote:
> >>>> Hi Marek,
> >>>>
> >>>> Marek Vasut <marex@denx.de> wrote on Wed, 5 Feb 2020 10:41:05 +0100:
> >>>>
> >>>>> On 2/5/20 10:12 AM, Miquel Raynal wrote:
> >>>>>> Hi Marek,
> >>>>>>
> >>>>>> Marek Vasut <marex@denx.de> wrote on Wed,  5 Feb 2020 08:08:34 +0100:
> >>>>>>
> >>>>>>> This reverts commit d311e0c27b8fcc27f707f8cac48cd8bdc4155224, which
> >>>>>>> completely breaks NAND access on Altera SoCFPGA (detected on ArriaV
> >>>>>>> SoC).
> >>>>>>>
> >>>>>>> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
> >>>>>>> hence the driver sets NAND_KEEP_TIMINGS flag.


Interesting.
I have never seen such clock rates before.


For all known upstream platforms
(Altera SOCFPGA, Socionext UniPhier, Intel MRST),
the NAND controller core clock is 50 MHz,
the NAND bus clock is 200MHz.



What would happen if you hard-code:
denali->clk_rate = 50000000;
denali->clk_x_rate = 200000000;

like I had already suggested to Tim Sander:
https://lore.kernel.org/lkml/CAK7LNAQOCoJC0RzOhTEofHdR+zU5sQTxV-t4nERBExW1ddW5hw@mail.gmail.com/

Unfortunately, he did not want to do it, but
I am still interested in this experiment because
I suspect this might be a bug of drivers/clk/socfpga/.


4.19.10 kernel (, which Tim Sander agreed the timing was working fine)
was hard-coding them in order to deal with the immature SOCFPGA clock driver.

See this code:
https://elixir.bootlin.com/linux/v4.19.10/source/drivers/mtd/nand/raw/denali_dt.c#L162


Masahiro Yamada




> >>>>>>> and is actually incorrect, as on SoCFPGA we do not want to retain timings
> >>>>>>> from previous stage (the timings might be incorrect or outright invalid).
> >>>>>>>
> >>>>>>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> >>>>>>> Cc: Dinh Nguyen <dinguyen@kernel.org>
> >>>>>>> Cc: Masahiro Yamada <masahiroy@kernel.org>
> >>>>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> >>>>>>> Cc: Tim Sander <tim@krieglstein.org>
> >>>>>>> To: linux-mtd <linux-mtd@lists.infradead.org>
> >>>>>>> ---
> >>>>>>>  drivers/mtd/nand/raw/denali.c | 2 +-
> >>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> >>>>>>> index b6c463d02167..5fe3c62a756e 100644
> >>>>>>> --- a/drivers/mtd/nand/raw/denali.c
> >>>>>>> +++ b/drivers/mtd/nand/raw/denali.c
> >>>>>>> @@ -1209,7 +1209,7 @@ int denali_chip_init(struct denali_controller *denali,
> >>>>>>>         }
> >>>>>>>
> >>>>>>>         /* clk rate info is needed for setup_data_interface */
> >>>>>>> -       if (!denali->clk_rate || !denali->clk_x_rate)
> >>>>>>
> >>>>>> I don't get it, if both clk_rate and clk_x_rate are set, the if
> >>>>>> condition will not be entered, right?
> >>>>>
> >>>>> Err, then it's the other way around and I need to keep the timings on
> >>>>> socfpga ?
> >>>>
> >>>> Ok.
> >>>>
> >>>> Do you have a different compatible? Or a register to check? How do you
> >>>> discriminate the different platforms by software? The quick and dirty
> >>>> solution is to add a special case for your platform and specifically
> >>>> use the NAND_KEEP_TIMINGS horror.
> >>>
> >>> Sure, there's a socfpga compatible and at least two for uniphier.
> >>>
> >>>> But I think using ->software_data_interface is the right solution. So
> >>>> I would highly recommend fixing the implementation of this hook
> >>>> for your platform and in this case the commit reverted is not the
> >>>> culprit, the one introducing setup_data_interface is (for the Fixes:
> >>>> tag).
> >>>
> >>> I'll leave the details to Yamada-san.
> >>
> >> Just got a confirmation that this fixes NAND behavior on SoCFPGA, so
> >> this patch should go in in some form.
> >
> > I'm sure it fixes it, but it is definitely not going in the right
> > direction!
> >
> > The right thing to do is fixing ->setup_data_interface().
> >
> > The bad thing to do if someone tells me that he will fix
> > ->setup_data_interface() in a second time is to keep the
> > NAND_KEEP_TIMINGS flag only for a single compatible.
>
> OK, I'll leave that to Yamada-san. I don't really want to interfere with
> his work on the Denali NAND driver.



-- 
Best Regards
Masahiro Yamada

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-12  9:00               ` Masahiro Yamada
@ 2020-02-12  9:37                 ` Marek Vasut
  2020-02-12 16:56                   ` Masahiro Yamada
  0 siblings, 1 reply; 31+ messages in thread
From: Marek Vasut @ 2020-02-12  9:37 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

On 2/12/20 10:00 AM, Masahiro Yamada wrote:
> Hi Marek,

Hi,

> On Wed, Feb 12, 2020 at 5:35 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/11/20 5:07 PM, Miquel Raynal wrote:
>>> Hi Marek, Masahiro,
>>>
>>> Marek Vasut <marex@denx.de> wrote on Tue, 11 Feb 2020 11:04:10 +0100:
>>>
>>>> On 2/5/20 11:08 AM, Marek Vasut wrote:
>>>>> On 2/5/20 10:50 AM, Miquel Raynal wrote:
>>>>>> Hi Marek,
>>>>>>
>>>>>> Marek Vasut <marex@denx.de> wrote on Wed, 5 Feb 2020 10:41:05 +0100:
>>>>>>
>>>>>>> On 2/5/20 10:12 AM, Miquel Raynal wrote:
>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> Marek Vasut <marex@denx.de> wrote on Wed,  5 Feb 2020 08:08:34 +0100:
>>>>>>>>
>>>>>>>>> This reverts commit d311e0c27b8fcc27f707f8cac48cd8bdc4155224, which
>>>>>>>>> completely breaks NAND access on Altera SoCFPGA (detected on ArriaV
>>>>>>>>> SoC).
>>>>>>>>>
>>>>>>>>> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
>>>>>>>>> hence the driver sets NAND_KEEP_TIMINGS flag.
> 
> 
> Interesting.
> I have never seen such clock rates before.
> 
> 
> For all known upstream platforms
> (Altera SOCFPGA, Socionext UniPhier, Intel MRST),
> the NAND controller core clock is 50 MHz,
> the NAND bus clock is 200MHz.

You can configure whatever rate you want in the QSys HPS component.

> What would happen if you hard-code:
> denali->clk_rate = 50000000;
> denali->clk_x_rate = 200000000;

It will not work, because the IP would be using incorrect clock.

> like I had already suggested to Tim Sander:
> https://lore.kernel.org/lkml/CAK7LNAQOCoJC0RzOhTEofHdR+zU5sQTxV-t4nERBExW1ddW5hw@mail.gmail.com/
> 
> Unfortunately, he did not want to do it, but
> I am still interested in this experiment because
> I suspect this might be a bug of drivers/clk/socfpga/.

No, this is a feature of the platform, you can configure any clock you
want pretty much.

> 4.19.10 kernel (, which Tim Sander agreed the timing was working fine)
> was hard-coding them in order to deal with the immature SOCFPGA clock driver.

The 4.19 was working fine for Tim (and me as well), because it didn't
have this bug which this patch removes.

[...]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-12  9:37                 ` Marek Vasut
@ 2020-02-12 16:56                   ` Masahiro Yamada
  2020-02-12 17:13                     ` Masahiro Yamada
  2020-02-12 17:44                     ` Marek Vasut
  0 siblings, 2 replies; 31+ messages in thread
From: Masahiro Yamada @ 2020-02-12 16:56 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

Hi.


On Wed, Feb 12, 2020 at 10:41 PM Marek Vasut <marex@denx.de> wrote:
>
> On 2/12/20 10:00 AM, Masahiro Yamada wrote:
> > Hi Marek,
>
> Hi,
>
> > On Wed, Feb 12, 2020 at 5:35 AM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 2/11/20 5:07 PM, Miquel Raynal wrote:
> >>> Hi Marek, Masahiro,
> >>>
> >>> Marek Vasut <marex@denx.de> wrote on Tue, 11 Feb 2020 11:04:10 +0100:
> >>>
> >>>> On 2/5/20 11:08 AM, Marek Vasut wrote:
> >>>>> On 2/5/20 10:50 AM, Miquel Raynal wrote:
> >>>>>> Hi Marek,
> >>>>>>
> >>>>>> Marek Vasut <marex@denx.de> wrote on Wed, 5 Feb 2020 10:41:05 +0100:
> >>>>>>
> >>>>>>> On 2/5/20 10:12 AM, Miquel Raynal wrote:
> >>>>>>>> Hi Marek,
> >>>>>>>>
> >>>>>>>> Marek Vasut <marex@denx.de> wrote on Wed,  5 Feb 2020 08:08:34 +0100:
> >>>>>>>>
> >>>>>>>>> This reverts commit d311e0c27b8fcc27f707f8cac48cd8bdc4155224, which
> >>>>>>>>> completely breaks NAND access on Altera SoCFPGA (detected on ArriaV
> >>>>>>>>> SoC).
> >>>>>>>>>
> >>>>>>>>> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
> >>>>>>>>> hence the driver sets NAND_KEEP_TIMINGS flag.
> >
> >
> > Interesting.
> > I have never seen such clock rates before.
> >
> >
> > For all known upstream platforms
> > (Altera SOCFPGA, Socionext UniPhier, Intel MRST),
> > the NAND controller core clock is 50 MHz,
> > the NAND bus clock is 200MHz.
>
> You can configure whatever rate you want in the QSys HPS component.

OK.

The SOCFPGA maintainer, Dinh Nguyen, said this:
"From the clock controller, it provides a single 200MHz clock to the NAND
IP. Inside the NAND IP, there is a /4 for the clk. The 200MHz clock is
used for the clk_x and ecc_clk."


http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/592702.html



Maybe, you are using a brand-new,
different type of SOCFPGA?



> > What would happen if you hard-code:
> > denali->clk_rate = 50000000;
> > denali->clk_x_rate = 200000000;
>
> It will not work, because the IP would be using incorrect clock.

I wanted to see the past tense here instead of
future tense + subjunctive mood.

I wanted you to try it.



>
> > like I had already suggested to Tim Sander:
> > https://lore.kernel.org/lkml/CAK7LNAQOCoJC0RzOhTEofHdR+zU5sQTxV-t4nERBExW1ddW5hw@mail.gmail.com/
> >
> > Unfortunately, he did not want to do it, but
> > I am still interested in this experiment because
> > I suspect this might be a bug of drivers/clk/socfpga/.
>
> No, this is a feature of the platform, you can configure any clock you
> want pretty much.


OK, but if you agree the 4.19.10 is working,

denali->clk_rate = 50000000; denali->clk_x_rate = 200000000;

is worth trying.


>
> > 4.19.10 kernel (, which Tim Sander agreed the timing was working fine)
> > was hard-coding them in order to deal with the immature SOCFPGA clock driver.
>
> The 4.19 was working fine for Tim (and me as well), because it didn't
> have this bug which this patch removes.


d311e0c27b8fcc27f707f8ca did not exist in 4.19

But, 7a08dbaedd36 did not exist either in 4.19


$ git describe  7a08dbae
v4.20-rc2-34-g7a08dbaedd36
$ git describe  d311e0c
v5.0-rc2-3-gd311e0c27b8f


So, your patch is getting it back to
v4.20-rc2-34-g7a08dbaedd36
where the condition for ->setup_data_interface() call
is accidentally inverted for the Denali driver.



BTW, did you notice your code was doing the opposite
to your commit description?


Your commit description goes like this:

"On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
hence the driver sets NAND_KEEP_TIMINGS flag. This did not happen before
and is actually incorrect, as on SoCFPGA we do not want to retain timings
from previous stage (the timings might be incorrect or outright invalid)."


You clearly state denali->clk_rate and denali->clk_x_rate
are non-zero values.

If this patch were applied, we would end up with NAND_KEEP_TIMINGS set.
Then ->setup_data_interface() hook would be skipped.
So, the timings from previous stage would be retained.

Is this what you want to do?


One more thing, if your patch were applied,
we would get back to the situation
where the back-trace happens due to zero-division:


When denali->clk_x_rate is zero,
NAND_KEEP_TIMINGS would not be set with your patch.
So, ->setup_data_interface() would be called.

This would cause zero divion at line 781.
        t_x = DIV_ROUND_DOWN_ULL(1000000000000ULL, denali->clk_x_rate);



-- 
Best Regards
Masahiro Yamada

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-12 16:56                   ` Masahiro Yamada
@ 2020-02-12 17:13                     ` Masahiro Yamada
  2020-02-12 17:44                     ` Marek Vasut
  1 sibling, 0 replies; 31+ messages in thread
From: Masahiro Yamada @ 2020-02-12 17:13 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

On Thu, Feb 13, 2020 at 1:56 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Hi.
>
>
> On Wed, Feb 12, 2020 at 10:41 PM Marek Vasut <marex@denx.de> wrote:
> >
> > On 2/12/20 10:00 AM, Masahiro Yamada wrote:
> > > Hi Marek,
> >
> > Hi,
> >
> > > On Wed, Feb 12, 2020 at 5:35 AM Marek Vasut <marex@denx.de> wrote:
> > >>
> > >> On 2/11/20 5:07 PM, Miquel Raynal wrote:
> > >>> Hi Marek, Masahiro,
> > >>>
> > >>> Marek Vasut <marex@denx.de> wrote on Tue, 11 Feb 2020 11:04:10 +0100:
> > >>>
> > >>>> On 2/5/20 11:08 AM, Marek Vasut wrote:
> > >>>>> On 2/5/20 10:50 AM, Miquel Raynal wrote:
> > >>>>>> Hi Marek,
> > >>>>>>
> > >>>>>> Marek Vasut <marex@denx.de> wrote on Wed, 5 Feb 2020 10:41:05 +0100:
> > >>>>>>
> > >>>>>>> On 2/5/20 10:12 AM, Miquel Raynal wrote:
> > >>>>>>>> Hi Marek,
> > >>>>>>>>
> > >>>>>>>> Marek Vasut <marex@denx.de> wrote on Wed,  5 Feb 2020 08:08:34 +0100:
> > >>>>>>>>
> > >>>>>>>>> This reverts commit d311e0c27b8fcc27f707f8cac48cd8bdc4155224, which
> > >>>>>>>>> completely breaks NAND access on Altera SoCFPGA (detected on ArriaV
> > >>>>>>>>> SoC).
> > >>>>>>>>>
> > >>>>>>>>> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
> > >>>>>>>>> hence the driver sets NAND_KEEP_TIMINGS flag.
> > >
> > >
> > > Interesting.
> > > I have never seen such clock rates before.
> > >
> > >
> > > For all known upstream platforms
> > > (Altera SOCFPGA, Socionext UniPhier, Intel MRST),
> > > the NAND controller core clock is 50 MHz,
> > > the NAND bus clock is 200MHz.
> >
> > You can configure whatever rate you want in the QSys HPS component.
>
> OK.
>
> The SOCFPGA maintainer, Dinh Nguyen, said this:
> "From the clock controller, it provides a single 200MHz clock to the NAND
> IP. Inside the NAND IP, there is a /4 for the clk. The 200MHz clock is
> used for the clk_x and ecc_clk."
>
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/592702.html
>
>
>
> Maybe, you are using a brand-new,
> different type of SOCFPGA?
>
>
>
> > > What would happen if you hard-code:
> > > denali->clk_rate = 50000000;
> > > denali->clk_x_rate = 200000000;
> >
> > It will not work, because the IP would be using incorrect clock.
>
> I wanted to see the past tense here instead of
> future tense + subjunctive mood.
>
> I wanted you to try it.
>
>
>
> >
> > > like I had already suggested to Tim Sander:
> > > https://lore.kernel.org/lkml/CAK7LNAQOCoJC0RzOhTEofHdR+zU5sQTxV-t4nERBExW1ddW5hw@mail.gmail.com/
> > >
> > > Unfortunately, he did not want to do it, but
> > > I am still interested in this experiment because
> > > I suspect this might be a bug of drivers/clk/socfpga/.
> >
> > No, this is a feature of the platform, you can configure any clock you
> > want pretty much.
>
>
> OK, but if you agree the 4.19.10 is working,
>
> denali->clk_rate = 50000000; denali->clk_x_rate = 200000000;
>
> is worth trying.
>
>
> >
> > > 4.19.10 kernel (, which Tim Sander agreed the timing was working fine)
> > > was hard-coding them in order to deal with the immature SOCFPGA clock driver.
> >
> > The 4.19 was working fine for Tim (and me as well), because it didn't
> > have this bug which this patch removes.
>
>
> d311e0c27b8fcc27f707f8ca did not exist in 4.19
>
> But, 7a08dbaedd36 did not exist either in 4.19
>
>
> $ git describe  7a08dbae
> v4.20-rc2-34-g7a08dbaedd36
> $ git describe  d311e0c
> v5.0-rc2-3-gd311e0c27b8f
>
>
> So, your patch is getting it back to
> v4.20-rc2-34-g7a08dbaedd36
> where the condition for ->setup_data_interface() call
> is accidentally inverted for the Denali driver.
>
>
>
> BTW, did you notice your code was doing the opposite
> to your commit description?
>
>
> Your commit description goes like this:
>
> "On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
> hence the driver sets NAND_KEEP_TIMINGS flag. This did not happen before
> and is actually incorrect, as on SoCFPGA we do not want to retain timings
> from previous stage (the timings might be incorrect or outright invalid)."
>
>
> You clearly state denali->clk_rate and denali->clk_x_rate
> are non-zero values.
>
> If this patch were applied, we would end up with NAND_KEEP_TIMINGS set.
> Then ->setup_data_interface() hook would be skipped.
> So, the timings from previous stage would be retained.
>
> Is this what you want to do?
>
>
> One more thing, if your patch were applied,
> we would get back to the situation
> where the back-trace happens due to zero-division:
>
>
> When denali->clk_x_rate is zero,
> NAND_KEEP_TIMINGS would not be set with your patch.
> So, ->setup_data_interface() would be called.
>
> This would cause zero divion at line 781.
>         t_x = DIV_ROUND_DOWN_ULL(1000000000000ULL, denali->clk_x_rate);
>


I missed to read the whole of this thread.

As you discussed with Miquel and Boris,
you already understood the code and the description were opposite.






-- 
Best Regards
Masahiro Yamada

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-12 16:56                   ` Masahiro Yamada
  2020-02-12 17:13                     ` Masahiro Yamada
@ 2020-02-12 17:44                     ` Marek Vasut
  2020-02-17  8:33                       ` Masahiro Yamada
  1 sibling, 1 reply; 31+ messages in thread
From: Marek Vasut @ 2020-02-12 17:44 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

On 2/12/20 5:56 PM, Masahiro Yamada wrote:
> Hi.

Hi,

[...]

>>>>>>>>>>> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
>>>>>>>>>>> hence the driver sets NAND_KEEP_TIMINGS flag.
>>>
>>>
>>> Interesting.
>>> I have never seen such clock rates before.
>>>
>>>
>>> For all known upstream platforms
>>> (Altera SOCFPGA, Socionext UniPhier, Intel MRST),
>>> the NAND controller core clock is 50 MHz,
>>> the NAND bus clock is 200MHz.
>>
>> You can configure whatever rate you want in the QSys HPS component.
> 
> OK.
> 
> The SOCFPGA maintainer, Dinh Nguyen, said this:
> "From the clock controller, it provides a single 200MHz clock to the NAND
> IP. Inside the NAND IP, there is a /4 for the clk. The 200MHz clock is
> used for the clk_x and ecc_clk."

That sounds like some root clock. You can read the entire documentation
for the SoCFPGA CV/AV here:
http://www.altera.com/literature/hb/cyclone-v/cv_5v4.pdf
See section 3 , look for 'nand' there. Note that NAND can be supplied
from at least two different PLLs -- main and peripheral.

> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/592702.html
> 
> 
> 
> Maybe, you are using a brand-new,
> different type of SOCFPGA?

Cyclone V and Arria V , so no.

>>> What would happen if you hard-code:
>>> denali->clk_rate = 50000000;
>>> denali->clk_x_rate = 200000000;
>>
>> It will not work, because the IP would be using incorrect clock.
> 
> I wanted to see the past tense here instead of
> future tense + subjunctive mood.
> 
> I wanted you to try it.
> 
> 
> 
>>
>>> like I had already suggested to Tim Sander:
>>> https://lore.kernel.org/lkml/CAK7LNAQOCoJC0RzOhTEofHdR+zU5sQTxV-t4nERBExW1ddW5hw@mail.gmail.com/
>>>
>>> Unfortunately, he did not want to do it, but
>>> I am still interested in this experiment because
>>> I suspect this might be a bug of drivers/clk/socfpga/.
>>
>> No, this is a feature of the platform, you can configure any clock you
>> want pretty much.
> 
> 
> OK, but if you agree the 4.19.10 is working,
> 
> denali->clk_rate = 50000000; denali->clk_x_rate = 200000000;
> 
> is worth trying.

Why would misconfiguring the IP be worth trying ?

>>> 4.19.10 kernel (, which Tim Sander agreed the timing was working fine)
>>> was hard-coding them in order to deal with the immature SOCFPGA clock driver.
>>
>> The 4.19 was working fine for Tim (and me as well), because it didn't
>> have this bug which this patch removes.
> 
> 
> d311e0c27b8fcc27f707f8ca did not exist in 4.19
> 
> But, 7a08dbaedd36 did not exist either in 4.19
> 
> 
> $ git describe  7a08dbae
> v4.20-rc2-34-g7a08dbaedd36
> $ git describe  d311e0c
> v5.0-rc2-3-gd311e0c27b8f
> 
> 
> So, your patch is getting it back to
> v4.20-rc2-34-g7a08dbaedd36
> where the condition for ->setup_data_interface() call
> is accidentally inverted for the Denali driver.
> 
> 
> 
> BTW, did you notice your code was doing the opposite
> to your commit description?

I think Boris / Miquel mentioned that above already.

> Your commit description goes like this:
> 
> "On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
> hence the driver sets NAND_KEEP_TIMINGS flag. This did not happen before
> and is actually incorrect, as on SoCFPGA we do not want to retain timings
> from previous stage (the timings might be incorrect or outright invalid)."
> 
> 
> You clearly state denali->clk_rate and denali->clk_x_rate
> are non-zero values.

They are non-zero, 31.25 MHz and 125 MHz respectively.

> If this patch were applied, we would end up with NAND_KEEP_TIMINGS set.
> Then ->setup_data_interface() hook would be skipped.
> So, the timings from previous stage would be retained.
> 
> Is this what you want to do?

I don't know ? The calculated timings are apparently not working.

> One more thing, if your patch were applied,
> we would get back to the situation
> where the back-trace happens due to zero-division:
> 
> 
> When denali->clk_x_rate is zero,
> NAND_KEEP_TIMINGS would not be set with your patch.
> So, ->setup_data_interface() would be called.
> 
> This would cause zero divion at line 781.
>         t_x = DIV_ROUND_DOWN_ULL(1000000000000ULL, denali->clk_x_rate);

If the clock are non-zero, how would this be a division by zero ?

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-12 17:44                     ` Marek Vasut
@ 2020-02-17  8:33                       ` Masahiro Yamada
  2020-02-18  5:55                         ` Masahiro Yamada
  2020-02-19 18:27                         ` Marek Vasut
  0 siblings, 2 replies; 31+ messages in thread
From: Masahiro Yamada @ 2020-02-17  8:33 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

[-- Attachment #1: Type: text/plain, Size: 5483 bytes --]

Hi,



On Thu, Feb 13, 2020 at 2:45 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/12/20 5:56 PM, Masahiro Yamada wrote:
> > Hi.
>
> Hi,
>
> [...]
>
> >>>>>>>>>>> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
> >>>>>>>>>>> hence the driver sets NAND_KEEP_TIMINGS flag.
> >>>
> >>>
> >>> Interesting.
> >>> I have never seen such clock rates before.
> >>>
> >>>
> >>> For all known upstream platforms
> >>> (Altera SOCFPGA, Socionext UniPhier, Intel MRST),
> >>> the NAND controller core clock is 50 MHz,
> >>> the NAND bus clock is 200MHz.
> >>
> >> You can configure whatever rate you want in the QSys HPS component.
> >
> > OK.
> >
> > The SOCFPGA maintainer, Dinh Nguyen, said this:
> > "From the clock controller, it provides a single 200MHz clock to the NAND
> > IP. Inside the NAND IP, there is a /4 for the clk. The 200MHz clock is
> > used for the clk_x and ecc_clk."
>
> That sounds like some root clock. You can read the entire documentation
> for the SoCFPGA CV/AV here:
> http://www.altera.com/literature/hb/cyclone-v/cv_5v4.pdf
> See section 3 , look for 'nand' there. Note that NAND can be supplied
> from at least two different PLLs -- main and peripheral.
>
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/592702.html
> >
> >
> >
> > Maybe, you are using a brand-new,
> > different type of SOCFPGA?
>
> Cyclone V and Arria V , so no.
>
> >>> What would happen if you hard-code:
> >>> denali->clk_rate = 50000000;
> >>> denali->clk_x_rate = 200000000;
> >>
> >> It will not work, because the IP would be using incorrect clock.
> >
> > I wanted to see the past tense here instead of
> > future tense + subjunctive mood.
> >
> > I wanted you to try it.
> >
> >
> >
> >>
> >>> like I had already suggested to Tim Sander:
> >>> https://lore.kernel.org/lkml/CAK7LNAQOCoJC0RzOhTEofHdR+zU5sQTxV-t4nERBExW1ddW5hw@mail.gmail.com/
> >>>
> >>> Unfortunately, he did not want to do it, but
> >>> I am still interested in this experiment because
> >>> I suspect this might be a bug of drivers/clk/socfpga/.
> >>
> >> No, this is a feature of the platform, you can configure any clock you
> >> want pretty much.
> >
> >
> > OK, but if you agree the 4.19.10 is working,
> >
> > denali->clk_rate = 50000000; denali->clk_x_rate = 200000000;
> >
> > is worth trying.
>
> Why would misconfiguring the IP be worth trying ?



There is no change around the ->setup_data_interface() hook
after v4.19
The only difference I could think of is the clock frequency.

But, it is OK if you do not want to test it.

And you are confident.

So, let's suspect the ->setup_data_interface() hook.


If possible, can you provide the dump of
the attached debug code?




>
> >>> 4.19.10 kernel (, which Tim Sander agreed the timing was working fine)
> >>> was hard-coding them in order to deal with the immature SOCFPGA clock driver.
> >>
> >> The 4.19 was working fine for Tim (and me as well), because it didn't
> >> have this bug which this patch removes.
> >
> >
> > d311e0c27b8fcc27f707f8ca did not exist in 4.19
> >
> > But, 7a08dbaedd36 did not exist either in 4.19
> >
> >
> > $ git describe  7a08dbae
> > v4.20-rc2-34-g7a08dbaedd36
> > $ git describe  d311e0c
> > v5.0-rc2-3-gd311e0c27b8f
> >
> >
> > So, your patch is getting it back to
> > v4.20-rc2-34-g7a08dbaedd36
> > where the condition for ->setup_data_interface() call
> > is accidentally inverted for the Denali driver.
> >
> >
> >
> > BTW, did you notice your code was doing the opposite
> > to your commit description?
>
> I think Boris / Miquel mentioned that above already.
>
> > Your commit description goes like this:
> >
> > "On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
> > hence the driver sets NAND_KEEP_TIMINGS flag. This did not happen before
> > and is actually incorrect, as on SoCFPGA we do not want to retain timings
> > from previous stage (the timings might be incorrect or outright invalid)."
> >
> >
> > You clearly state denali->clk_rate and denali->clk_x_rate
> > are non-zero values.
>
> They are non-zero, 31.25 MHz and 125 MHz respectively.
>
> > If this patch were applied, we would end up with NAND_KEEP_TIMINGS set.
> > Then ->setup_data_interface() hook would be skipped.
> > So, the timings from previous stage would be retained.
> >
> > Is this what you want to do?
>
> I don't know ? The calculated timings are apparently not working.
>
> > One more thing, if your patch were applied,
> > we would get back to the situation
> > where the back-trace happens due to zero-division:
> >
> >
> > When denali->clk_x_rate is zero,
> > NAND_KEEP_TIMINGS would not be set with your patch.
> > So, ->setup_data_interface() would be called.
> >
> > This would cause zero divion at line 781.
> >         t_x = DIV_ROUND_DOWN_ULL(1000000000000ULL, denali->clk_x_rate);
>
> If the clock are non-zero, how would this be a division by zero ?


You have a false assumption "If the clock are non-zero...".

clk_get_rate() may return zero if the clock driver
is not ready to provide the frequency information.



The current code:
  If clk_rate or clk_x_rate is zero,
   do not call denali_setup_data_interface().
  If neither clk_rate nor clk_x is zero,
   call denali_setup_data_interface().


With your patch:
  If clk_rate or clk_x_rate is zero,
   call denali_setup_data_interface(),
   which causes zero division.
  If neither clk_rate nor clk_x is zero,
   do not call denali_setup_data_interface().




-- 
Best Regards
Masahiro Yamada

[-- Attachment #2: 0001-denali-dump-timing-parameters.patch --]
[-- Type: text/x-patch, Size: 4446 bytes --]

From 8734c5fd3f5917b765bd64b1d78d59bbbc22039e Mon Sep 17 00:00:00 2001
From: Masahiro Yamada <masahiroy@kernel.org>
Date: Mon, 17 Feb 2020 16:48:06 +0900
Subject: [PATCH] denali: dump timing parameters

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
 drivers/mtd/nand/raw/denali.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
index fafd0a0aa8e2..c332ca3cba94 100644
--- a/drivers/mtd/nand/raw/denali.c
+++ b/drivers/mtd/nand/raw/denali.c
@@ -794,6 +794,8 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	if (chipnr == NAND_DATA_IFACE_CHECK_ONLY)
 		return 0;
 
+	printk("Denali: clk_rate=%ld, clk_x_rate=%ld\n", denali->clk_rate, denali->clk_x_rate);
+
 	sel = &to_denali_chip(chip)->sels[chipnr];
 
 	/* tREA -> ACC_CLKS */
@@ -805,10 +807,16 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	tmp |= FIELD_PREP(ACC_CLKS__VALUE, acc_clks);
 	sel->acc_clks = tmp;
 
+	printk("Denali: tREA=%d\n", timings->tREA_max);
+	printk("Denali: acc_clks=%d\n", acc_clks);
+
 	/* tRWH -> RE_2_WE */
 	re_2_we = DIV_ROUND_UP(timings->tRHW_min, t_x);
 	re_2_we = min_t(int, re_2_we, RE_2_WE__VALUE);
 
+	printk("Denali: tRHW=%d\n", timings->tRHW_min);
+	printk("Denali: re_2_we=%d\n", re_2_we);
+
 	tmp = ioread32(denali->reg + RE_2_WE);
 	tmp &= ~RE_2_WE__VALUE;
 	tmp |= FIELD_PREP(RE_2_WE__VALUE, re_2_we);
@@ -818,6 +826,9 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	re_2_re = DIV_ROUND_UP(timings->tRHZ_max, t_x);
 	re_2_re = min_t(int, re_2_re, RE_2_RE__VALUE);
 
+	printk("Denali: tRHZ=%d\n", timings->tRHZ_max);
+	printk("Denali: re_2_re=%d\n", re_2_re);
+
 	tmp = ioread32(denali->reg + RE_2_RE);
 	tmp &= ~RE_2_RE__VALUE;
 	tmp |= FIELD_PREP(RE_2_RE__VALUE, re_2_re);
@@ -832,6 +843,10 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	we_2_re = DIV_ROUND_UP(max(timings->tCCS_min, timings->tWHR_min), t_x);
 	we_2_re = min_t(int, we_2_re, TWHR2_AND_WE_2_RE__WE_2_RE);
 
+	printk("Denali: tCCS=%d\n", timings->tCCS_min);
+	printk("Denali: tWHR=%d\n", timings->tWHR_min);
+	printk("Denali: we_2_re=%d\n", we_2_re);
+
 	tmp = ioread32(denali->reg + TWHR2_AND_WE_2_RE);
 	tmp &= ~TWHR2_AND_WE_2_RE__WE_2_RE;
 	tmp |= FIELD_PREP(TWHR2_AND_WE_2_RE__WE_2_RE, we_2_re);
@@ -847,6 +862,9 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	addr_2_data = DIV_ROUND_UP(timings->tADL_min, t_x);
 	addr_2_data = min_t(int, addr_2_data, addr_2_data_mask);
 
+	printk("Denali: tADL=%d\n", timings->tADL_min);
+	printk("Denali: addr_2_data=%d\n", addr_2_data);
+
 	tmp = ioread32(denali->reg + TCWAW_AND_ADDR_2_DATA);
 	tmp &= ~TCWAW_AND_ADDR_2_DATA__ADDR_2_DATA;
 	tmp |= FIELD_PREP(TCWAW_AND_ADDR_2_DATA__ADDR_2_DATA, addr_2_data);
@@ -857,6 +875,10 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 				  t_x);
 	rdwr_en_hi = min_t(int, rdwr_en_hi, RDWR_EN_HI_CNT__VALUE);
 
+	printk("Denali: tREH=%d\n", timings->tREH_min);
+	printk("Denali: tWH=%d\n", timings->tWH_min);
+	printk("Denali: rdwr_en_hi=%d\n", rdwr_en_hi);
+
 	tmp = ioread32(denali->reg + RDWR_EN_HI_CNT);
 	tmp &= ~RDWR_EN_HI_CNT__VALUE;
 	tmp |= FIELD_PREP(RDWR_EN_HI_CNT__VALUE, rdwr_en_hi);
@@ -870,6 +892,13 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	rdwr_en_lo = max(rdwr_en_lo, rdwr_en_lo_hi - rdwr_en_hi);
 	rdwr_en_lo = min_t(int, rdwr_en_lo, RDWR_EN_LO_CNT__VALUE);
 
+	printk("Denali: tRP=%d\n", timings->tRP_min);
+	printk("Denali: tWP=%d\n", timings->tWP_min);
+	printk("Denali: tRC=%d\n", timings->tRC_min);
+	printk("Denali: tWC=%d\n", timings->tWC_min);
+	printk("Denali: rdwr_en_lo_hi=%d\n", rdwr_en_lo_hi);
+	printk("Denali: rdwr_en_lo=%d\n", rdwr_en_lo);
+
 	tmp = ioread32(denali->reg + RDWR_EN_LO_CNT);
 	tmp &= ~RDWR_EN_LO_CNT__VALUE;
 	tmp |= FIELD_PREP(RDWR_EN_LO_CNT__VALUE, rdwr_en_lo);
@@ -881,6 +910,10 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 			0);
 	cs_setup = min_t(int, cs_setup, CS_SETUP_CNT__VALUE);
 
+	printk("Denali: tCS=%d\n", timings->tCS_min);
+	printk("Denali: tCEA=%d\n", timings->tCEA_max);
+	printk("Denali: cs_setup=%d\n", cs_setup);
+
 	tmp = ioread32(denali->reg + CS_SETUP_CNT);
 	tmp &= ~CS_SETUP_CNT__VALUE;
 	tmp |= FIELD_PREP(CS_SETUP_CNT__VALUE, cs_setup);
-- 
2.17.1


[-- Attachment #3: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-17  8:33                       ` Masahiro Yamada
@ 2020-02-18  5:55                         ` Masahiro Yamada
  2020-02-19 18:42                           ` Marek Vasut
  2020-02-19 18:27                         ` Marek Vasut
  1 sibling, 1 reply; 31+ messages in thread
From: Masahiro Yamada @ 2020-02-18  5:55 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

[-- Attachment #1: Type: text/plain, Size: 3169 bytes --]

Hi

On Mon, Feb 17, 2020 at 5:33 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Hi,
>
>
>
> On Thu, Feb 13, 2020 at 2:45 AM Marek Vasut <marex@denx.de> wrote:
> >
> > On 2/12/20 5:56 PM, Masahiro Yamada wrote:
> > > Hi.
> >
> > Hi,
> >
> > [...]
> >
> > >>>>>>>>>>> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
> > >>>>>>>>>>> hence the driver sets NAND_KEEP_TIMINGS flag.
> > >>>
> > >>>
> > >>> Interesting.
> > >>> I have never seen such clock rates before.
> > >>>
> > >>>
> > >>> For all known upstream platforms
> > >>> (Altera SOCFPGA, Socionext UniPhier, Intel MRST),
> > >>> the NAND controller core clock is 50 MHz,
> > >>> the NAND bus clock is 200MHz.
> > >>
> > >> You can configure whatever rate you want in the QSys HPS component.
> > >
> > > OK.
> > >
> > > The SOCFPGA maintainer, Dinh Nguyen, said this:
> > > "From the clock controller, it provides a single 200MHz clock to the NAND
> > > IP. Inside the NAND IP, there is a /4 for the clk. The 200MHz clock is
> > > used for the clk_x and ecc_clk."
> >
> > That sounds like some root clock. You can read the entire documentation
> > for the SoCFPGA CV/AV here:
> > http://www.altera.com/literature/hb/cyclone-v/cv_5v4.pdf
> > See section 3 , look for 'nand' there. Note that NAND can be supplied
> > from at least two different PLLs -- main and peripheral.
> >
> > > http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/592702.html
> > >
> > >
> > >
> > > Maybe, you are using a brand-new,
> > > different type of SOCFPGA?
> >
> > Cyclone V and Arria V , so no.
> >
> > >>> What would happen if you hard-code:
> > >>> denali->clk_rate = 50000000;
> > >>> denali->clk_x_rate = 200000000;
> > >>
> > >> It will not work, because the IP would be using incorrect clock.
> > >
> > > I wanted to see the past tense here instead of
> > > future tense + subjunctive mood.
> > >
> > > I wanted you to try it.
> > >
> > >
> > >
> > >>
> > >>> like I had already suggested to Tim Sander:
> > >>> https://lore.kernel.org/lkml/CAK7LNAQOCoJC0RzOhTEofHdR+zU5sQTxV-t4nERBExW1ddW5hw@mail.gmail.com/
> > >>>
> > >>> Unfortunately, he did not want to do it, but
> > >>> I am still interested in this experiment because
> > >>> I suspect this might be a bug of drivers/clk/socfpga/.
> > >>
> > >> No, this is a feature of the platform, you can configure any clock you
> > >> want pretty much.
> > >
> > >
> > > OK, but if you agree the 4.19.10 is working,
> > >
> > > denali->clk_rate = 50000000; denali->clk_x_rate = 200000000;
> > >
> > > is worth trying.
> >
> > Why would misconfiguring the IP be worth trying ?
>
>
>
> There is no change around the ->setup_data_interface() hook
> after v4.19
> The only difference I could think of is the clock frequency.
>
> But, it is OK if you do not want to test it.
>
> And you are confident.
>
> So, let's suspect the ->setup_data_interface() hook.
>
>
> If possible, can you provide the dump of
> the attached debug code?
>


I attached two experimental patches.

I cannot test them because
the mainline code works fine for my boards.

Does either of them improve something
on your settings?


-- 
Best Regards
Masahiro Yamada

[-- Attachment #2: 0001-denali-more-complicated-calculation-for-timings.patch --]
[-- Type: text/x-patch, Size: 4158 bytes --]

From 42c3dc3a3e79365b6fb467edbd49630e697fffc9 Mon Sep 17 00:00:00 2001
From: Masahiro Yamada <yamada.masahiro@socionext.com>
Date: Mon, 17 Feb 2020 21:16:33 +0900
Subject: [PATCH] denali: more complicated calculation for timings

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
 drivers/mtd/nand/raw/denali.c | 64 +++++++++++++++++++++++++++++------
 1 file changed, 53 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
index fafd0a0aa8e2..a16b2f1f50b5 100644
--- a/drivers/mtd/nand/raw/denali.c
+++ b/drivers/mtd/nand/raw/denali.c
@@ -796,15 +796,6 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 
 	sel = &to_denali_chip(chip)->sels[chipnr];
 
-	/* tREA -> ACC_CLKS */
-	acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x);
-	acc_clks = min_t(int, acc_clks, ACC_CLKS__VALUE);
-
-	tmp = ioread32(denali->reg + ACC_CLKS);
-	tmp &= ~ACC_CLKS__VALUE;
-	tmp |= FIELD_PREP(ACC_CLKS__VALUE, acc_clks);
-	sel->acc_clks = tmp;
-
 	/* tRWH -> RE_2_WE */
 	re_2_we = DIV_ROUND_UP(timings->tRHW_min, t_x);
 	re_2_we = min_t(int, re_2_we, RE_2_WE__VALUE);
@@ -862,14 +853,39 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	tmp |= FIELD_PREP(RDWR_EN_HI_CNT__VALUE, rdwr_en_hi);
 	sel->rdwr_en_hi_cnt = tmp;
 
-	/* tRP, tWP -> RDWR_EN_LO_CNT */
+	/*
+	 * tREA -> ACC_CLOCKS
+	 * tRP, tWP, tRHOH, tRC, tWC -> RDWR_EN_LO_CNT
+	 */
+
+	/*
+	 * Determine the minimum of acc_clks to meet the data setup timing.
+	 * (one additional clock cycle just in case)
+	 */
+	acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 1;
+
+	/* Determine the minimum of rdwr_en_lo_cnt from RE#/WE# pulse width */
 	rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min), t_x);
+
+	/* Extend rdwr_en_lo to meet the data hold timing */
+	rdwr_en_lo = max_t(int, rdwr_en_lo, acc_clks - timings->tRHOH_min / t_x);
+
+	/* Extend rdwr_en_lo to meet the requirement for RE#/WE# cycle time */
 	rdwr_en_lo_hi = DIV_ROUND_UP(max(timings->tRC_min, timings->tWC_min),
 				     t_x);
-	rdwr_en_lo_hi = max_t(int, rdwr_en_lo_hi, mult_x);
 	rdwr_en_lo = max(rdwr_en_lo, rdwr_en_lo_hi - rdwr_en_hi);
 	rdwr_en_lo = min_t(int, rdwr_en_lo, RDWR_EN_LO_CNT__VALUE);
 
+	/* Center the data latch timing for extra safety */
+	acc_clks = (acc_clks + rdwr_en_lo +
+		    DIV_ROUND_UP(timings->tRHOH_min, t_x)) / 2;
+	acc_clks = min_t(int, acc_clks, ACC_CLKS__VALUE);
+
+	tmp = ioread32(denali->reg + ACC_CLKS);
+	tmp &= ~ACC_CLKS__VALUE;
+	tmp |= FIELD_PREP(ACC_CLKS__VALUE, acc_clks);
+	sel->acc_clks = tmp;
+
 	tmp = ioread32(denali->reg + RDWR_EN_LO_CNT);
 	tmp &= ~RDWR_EN_LO_CNT__VALUE;
 	tmp |= FIELD_PREP(RDWR_EN_LO_CNT__VALUE, rdwr_en_lo);
@@ -886,6 +902,32 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	tmp |= FIELD_PREP(CS_SETUP_CNT__VALUE, cs_setup);
 	sel->cs_setup_cnt = tmp;
 
+	/* debug */
+	printk("Denali: clk_rate=%ld, clk_x_rate=%ld\n", denali->clk_rate, denali->clk_x_rate);
+	printk("Denali: tREA=%d\n", timings->tREA_max);
+	printk("Denali: tRHW=%d\n", timings->tRHW_min);
+	printk("Denali: tRHZ=%d\n", timings->tRHZ_max);
+	printk("Denali: tCCS=%d\n", timings->tCCS_min);
+	printk("Denali: tWHR=%d\n", timings->tWHR_min);
+	printk("Denali: tADL=%d\n", timings->tADL_min);
+	printk("Denali: tREH=%d\n", timings->tREH_min);
+	printk("Denali: tWH=%d\n", timings->tWH_min);
+	printk("Denali: tRP=%d\n", timings->tRP_min);
+	printk("Denali: tWP=%d\n", timings->tWP_min);
+	printk("Denali: tRC=%d\n", timings->tRC_min);
+	printk("Denali: tWC=%d\n", timings->tWC_min);
+	printk("Denali: tCS=%d\n", timings->tCS_min);
+	printk("Denali: tCEA=%d\n", timings->tCEA_max);
+	printk("Denali: acc_clks=%d\n", acc_clks);
+	printk("Denali: re_2_we=%d\n", re_2_we);
+	printk("Denali: re_2_re=%d\n", re_2_re);
+	printk("Denali: we_2_re=%d\n", we_2_re);
+	printk("Denali: addr_2_data=%d\n", addr_2_data);
+	printk("Denali: rdwr_en_hi=%d\n", rdwr_en_hi);
+	printk("Denali: rdwr_en_lo_hi=%d\n", rdwr_en_lo_hi);
+	printk("Denali: rdwr_en_lo=%d\n", rdwr_en_lo);
+	printk("Denali: cs_setup=%d\n", cs_setup);
+
 	return 0;
 }
 
-- 
2.17.1


[-- Attachment #3: 0001-denali-timing-hack-1.patch --]
[-- Type: text/x-patch, Size: 2422 bytes --]

From ff8eff942017a58e7f4de36cbb71b1de59a1dfbf Mon Sep 17 00:00:00 2001
From: Masahiro Yamada <yamada.masahiro@socionext.com>
Date: Mon, 17 Feb 2020 20:22:48 +0900
Subject: [PATCH] denali: timing hack 1

Increase setup time of data.
(but may not meet the hold time requirement)

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
 drivers/mtd/nand/raw/denali.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
index fafd0a0aa8e2..762fa8bc3e4c 100644
--- a/drivers/mtd/nand/raw/denali.c
+++ b/drivers/mtd/nand/raw/denali.c
@@ -798,7 +798,7 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 
 	/* tREA -> ACC_CLKS */
 	acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x);
-	acc_clks = min_t(int, acc_clks, ACC_CLKS__VALUE);
+	acc_clks = min_t(int, acc_clks, ACC_CLKS__VALUE) + 1;
 
 	tmp = ioread32(denali->reg + ACC_CLKS);
 	tmp &= ~ACC_CLKS__VALUE;
@@ -886,6 +886,32 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	tmp |= FIELD_PREP(CS_SETUP_CNT__VALUE, cs_setup);
 	sel->cs_setup_cnt = tmp;
 
+	/* debug */
+	printk("Denali: clk_rate=%ld, clk_x_rate=%ld\n", denali->clk_rate, denali->clk_x_rate);
+	printk("Denali: tREA=%d\n", timings->tREA_max);
+	printk("Denali: tRHW=%d\n", timings->tRHW_min);
+	printk("Denali: tRHZ=%d\n", timings->tRHZ_max);
+	printk("Denali: tCCS=%d\n", timings->tCCS_min);
+	printk("Denali: tWHR=%d\n", timings->tWHR_min);
+	printk("Denali: tADL=%d\n", timings->tADL_min);
+	printk("Denali: tREH=%d\n", timings->tREH_min);
+	printk("Denali: tWH=%d\n", timings->tWH_min);
+	printk("Denali: tRP=%d\n", timings->tRP_min);
+	printk("Denali: tWP=%d\n", timings->tWP_min);
+	printk("Denali: tRC=%d\n", timings->tRC_min);
+	printk("Denali: tWC=%d\n", timings->tWC_min);
+	printk("Denali: tCS=%d\n", timings->tCS_min);
+	printk("Denali: tCEA=%d\n", timings->tCEA_max);
+	printk("Denali: acc_clks=%d\n", acc_clks);
+	printk("Denali: re_2_we=%d\n", re_2_we);
+	printk("Denali: re_2_re=%d\n", re_2_re);
+	printk("Denali: we_2_re=%d\n", we_2_re);
+	printk("Denali: addr_2_data=%d\n", addr_2_data);
+	printk("Denali: rdwr_en_hi=%d\n", rdwr_en_hi);
+	printk("Denali: rdwr_en_lo_hi=%d\n", rdwr_en_lo_hi);
+	printk("Denali: rdwr_en_lo=%d\n", rdwr_en_lo);
+	printk("Denali: cs_setup=%d\n", cs_setup);
+
 	return 0;
 }
 
-- 
2.17.1


[-- Attachment #4: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-17  8:33                       ` Masahiro Yamada
  2020-02-18  5:55                         ` Masahiro Yamada
@ 2020-02-19 18:27                         ` Marek Vasut
  2020-02-25  0:38                           ` Masahiro Yamada
  1 sibling, 1 reply; 31+ messages in thread
From: Marek Vasut @ 2020-02-19 18:27 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

On 2/17/20 9:33 AM, Masahiro Yamada wrote:
> Hi,

Hi,

[...]

> If possible, can you provide the dump of
> the attached debug code?

Without this revert:

Denali: clk_rate=31250000, clk_x_rate=125000000
Denali: tREA=40000
Denali: acc_clks=5
Denali: tRHW=200000
Denali: re_2_we=25
Denali: tRHZ=200000
Denali: re_2_re=25
Denali: tCCS=500000000
Denali: tWHR=120000
Denali: we_2_re=63
Denali: tADL=400000
Denali: addr_2_data=50
Denali: tREH=30000
Denali: tWH=30000
Denali: rdwr_en_hi=4
Denali: tRP=50000
Denali: tWP=50000
Denali: tRC=100000
Denali: tWC=100000
Denali: rdwr_en_lo_hi=13
Denali: rdwr_en_lo=9
Denali: tCS=70000
Denali: tCEA=100000
Denali: cs_setup=8
Denali: clk_rate=31250000, clk_x_rate=125000000
Denali: tREA=16000
Denali: acc_clks=2
Denali: tRHW=100000
Denali: re_2_we=13
Denali: tRHZ=100000
Denali: re_2_re=13
Denali: tCCS=100000
Denali: tWHR=80000
Denali: we_2_re=13
Denali: tADL=400000
Denali: addr_2_data=50
Denali: tREH=7000
Denali: tWH=7000
Denali: rdwr_en_hi=1
Denali: tRP=10000
Denali: tWP=10000
Denali: tRC=20000
Denali: tWC=20000
Denali: rdwr_en_lo_hi=4
Denali: rdwr_en_lo=3
Denali: tCS=15000
Denali: tCEA=25000
Denali: cs_setup=2

With this revert, setup_data_interface() is not called, so there is no log.

[...]

>>> When denali->clk_x_rate is zero,
>>> NAND_KEEP_TIMINGS would not be set with your patch.
>>> So, ->setup_data_interface() would be called.
>>>
>>> This would cause zero divion at line 781.
>>>         t_x = DIV_ROUND_DOWN_ULL(1000000000000ULL, denali->clk_x_rate);
>>
>> If the clock are non-zero, how would this be a division by zero ?
> 
> 
> You have a false assumption "If the clock are non-zero...".
> 
> clk_get_rate() may return zero if the clock driver
> is not ready to provide the frequency information.
> 
> 
> 
> The current code:
>   If clk_rate or clk_x_rate is zero,
>    do not call denali_setup_data_interface().
>   If neither clk_rate nor clk_x is zero,
>    call denali_setup_data_interface().
> 
> 
> With your patch:
>   If clk_rate or clk_x_rate is zero,
>    call denali_setup_data_interface(),
>    which causes zero division.
>   If neither clk_rate nor clk_x is zero,
>    do not call denali_setup_data_interface().

OK, so it's just a miscommunication. In my case, neither of the clock
are zero. On SoCFPGA, I think clk_rate = clk_x_rate / 4, but I'm not
sure if that's always the case.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-18  5:55                         ` Masahiro Yamada
@ 2020-02-19 18:42                           ` Marek Vasut
  2020-02-25  0:41                             ` Masahiro Yamada
  0 siblings, 1 reply; 31+ messages in thread
From: Marek Vasut @ 2020-02-19 18:42 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

On 2/18/20 6:55 AM, Masahiro Yamada wrote:
> Hi

Hi,

[...]

>> There is no change around the ->setup_data_interface() hook
>> after v4.19
>> The only difference I could think of is the clock frequency.
>>
>> But, it is OK if you do not want to test it.
>>
>> And you are confident.
>>
>> So, let's suspect the ->setup_data_interface() hook.
>>
>>
>> If possible, can you provide the dump of
>> the attached debug code?
>>
> 
> 
> I attached two experimental patches.
> 
> I cannot test them because
> the mainline code works fine for my boards.
> 
> Does either of them improve something
> on your settings?

Considering that the NAND works if denali_setup_data_interface() is not
called, would it rather make sense to first read and print what's
programmed into the controller and then print what the code calculated
and intends to program into the controller ?

See attached patch, with which (without this revert) you get this:
denali->reg + TWHR2_AND_WE_2_RE = 0x00001414 -> 0x0000143f
denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
denali->reg + RE_2_WE = 0x00000014 -> 0x00000019
denali->reg + ACC_CLKS = 0x00000004 -> 0x00000005
denali->reg + RDWR_EN_LO_CNT = 0x00000002 -> 0x00000009
denali->reg + RDWR_EN_HI_CNT = 0x00000002 -> 0x00000004
denali->reg + CS_SETUP_CNT = 0x00000001 -> 0x00000008
denali->reg + RE_2_RE = 0x00000014 -> 0x00000019

[-- Attachment #2: 0001-denali-dump-timing-parameters.patch --]
[-- Type: text/x-patch, Size: 6264 bytes --]

>From a2a1300041979f183a5a85ddada63faa80b68983 Mon Sep 17 00:00:00 2001
From: Masahiro Yamada <masahiroy@kernel.org>
Date: Mon, 17 Feb 2020 16:48:06 +0900
Subject: [PATCH] denali: dump timing parameters

:'<,'>s@iowrite32(\([^,]\+\), \([^)]\+\));@pr_err("\2 = 0x%08x -> 0x%08x\\n", ioread32(\2), \1);\r\t&

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
 drivers/mtd/nand/raw/denali.c | 44 +++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
index b6c463d02167..4241b47d92a6 100644
--- a/drivers/mtd/nand/raw/denali.c
+++ b/drivers/mtd/nand/raw/denali.c
@@ -218,14 +218,21 @@ static void denali_select_target(struct nand_chip *chip, int cs)
 		return;
 
 	/* update timing registers unless NAND_KEEP_TIMINGS is set */
+	pr_err("denali->reg + TWHR2_AND_WE_2_RE = 0x%08x -> 0x%08x\n", ioread32(denali->reg + TWHR2_AND_WE_2_RE), sel->hwhr2_and_we_2_re);
 	iowrite32(sel->hwhr2_and_we_2_re, denali->reg + TWHR2_AND_WE_2_RE);
-	iowrite32(sel->tcwaw_and_addr_2_data,
-		  denali->reg + TCWAW_AND_ADDR_2_DATA);
+	pr_err("denali->reg + TCWAW_AND_ADDR_2_DATA = 0x%08x -> 0x%08x\n", ioread32(denali->reg + TCWAW_AND_ADDR_2_DATA), sel->tcwaw_and_addr_2_data);
+	iowrite32(sel->tcwaw_and_addr_2_data, denali->reg + TCWAW_AND_ADDR_2_DATA);
+	pr_err("denali->reg + RE_2_WE = 0x%08x -> 0x%08x\n", ioread32(denali->reg + RE_2_WE), sel->re_2_we);
 	iowrite32(sel->re_2_we, denali->reg + RE_2_WE);
+	pr_err("denali->reg + ACC_CLKS = 0x%08x -> 0x%08x\n", ioread32(denali->reg + ACC_CLKS), sel->acc_clks);
 	iowrite32(sel->acc_clks, denali->reg + ACC_CLKS);
+	pr_err("denali->reg + RDWR_EN_LO_CNT = 0x%08x -> 0x%08x\n", ioread32(denali->reg + RDWR_EN_LO_CNT), sel->rdwr_en_lo_cnt);
 	iowrite32(sel->rdwr_en_lo_cnt, denali->reg + RDWR_EN_LO_CNT);
+	pr_err("denali->reg + RDWR_EN_HI_CNT = 0x%08x -> 0x%08x\n", ioread32(denali->reg + RDWR_EN_HI_CNT), sel->rdwr_en_hi_cnt);
 	iowrite32(sel->rdwr_en_hi_cnt, denali->reg + RDWR_EN_HI_CNT);
+	pr_err("denali->reg + CS_SETUP_CNT = 0x%08x -> 0x%08x\n", ioread32(denali->reg + CS_SETUP_CNT), sel->cs_setup_cnt);
 	iowrite32(sel->cs_setup_cnt, denali->reg + CS_SETUP_CNT);
+	pr_err("denali->reg + RE_2_RE = 0x%08x -> 0x%08x\n", ioread32(denali->reg + RE_2_RE), sel->re_2_re);
 	iowrite32(sel->re_2_re, denali->reg + RE_2_RE);
 }
 
@@ -795,6 +802,8 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	if (chipnr == NAND_DATA_IFACE_CHECK_ONLY)
 		return 0;
 
+	printk("Denali: clk_rate=%ld, clk_x_rate=%ld\n", denali->clk_rate, denali->clk_x_rate);
+
 	sel = &to_denali_chip(chip)->sels[chipnr];
 
 	/* tREA -> ACC_CLKS */
@@ -806,10 +815,16 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	tmp |= FIELD_PREP(ACC_CLKS__VALUE, acc_clks);
 	sel->acc_clks = tmp;
 
+	printk("Denali: tREA=%d\n", timings->tREA_max);
+	printk("Denali: acc_clks=%d\n", acc_clks);
+
 	/* tRWH -> RE_2_WE */
 	re_2_we = DIV_ROUND_UP(timings->tRHW_min, t_x);
 	re_2_we = min_t(int, re_2_we, RE_2_WE__VALUE);
 
+	printk("Denali: tRHW=%d\n", timings->tRHW_min);
+	printk("Denali: re_2_we=%d\n", re_2_we);
+
 	tmp = ioread32(denali->reg + RE_2_WE);
 	tmp &= ~RE_2_WE__VALUE;
 	tmp |= FIELD_PREP(RE_2_WE__VALUE, re_2_we);
@@ -819,6 +834,9 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	re_2_re = DIV_ROUND_UP(timings->tRHZ_max, t_x);
 	re_2_re = min_t(int, re_2_re, RE_2_RE__VALUE);
 
+	printk("Denali: tRHZ=%d\n", timings->tRHZ_max);
+	printk("Denali: re_2_re=%d\n", re_2_re);
+
 	tmp = ioread32(denali->reg + RE_2_RE);
 	tmp &= ~RE_2_RE__VALUE;
 	tmp |= FIELD_PREP(RE_2_RE__VALUE, re_2_re);
@@ -833,6 +851,10 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	we_2_re = DIV_ROUND_UP(max(timings->tCCS_min, timings->tWHR_min), t_x);
 	we_2_re = min_t(int, we_2_re, TWHR2_AND_WE_2_RE__WE_2_RE);
 
+	printk("Denali: tCCS=%d\n", timings->tCCS_min);
+	printk("Denali: tWHR=%d\n", timings->tWHR_min);
+	printk("Denali: we_2_re=%d\n", we_2_re);
+
 	tmp = ioread32(denali->reg + TWHR2_AND_WE_2_RE);
 	tmp &= ~TWHR2_AND_WE_2_RE__WE_2_RE;
 	tmp |= FIELD_PREP(TWHR2_AND_WE_2_RE__WE_2_RE, we_2_re);
@@ -848,6 +870,9 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	addr_2_data = DIV_ROUND_UP(timings->tADL_min, t_x);
 	addr_2_data = min_t(int, addr_2_data, addr_2_data_mask);
 
+	printk("Denali: tADL=%d\n", timings->tADL_min);
+	printk("Denali: addr_2_data=%d\n", addr_2_data);
+
 	tmp = ioread32(denali->reg + TCWAW_AND_ADDR_2_DATA);
 	tmp &= ~TCWAW_AND_ADDR_2_DATA__ADDR_2_DATA;
 	tmp |= FIELD_PREP(TCWAW_AND_ADDR_2_DATA__ADDR_2_DATA, addr_2_data);
@@ -858,6 +883,10 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 				  t_x);
 	rdwr_en_hi = min_t(int, rdwr_en_hi, RDWR_EN_HI_CNT__VALUE);
 
+	printk("Denali: tREH=%d\n", timings->tREH_min);
+	printk("Denali: tWH=%d\n", timings->tWH_min);
+	printk("Denali: rdwr_en_hi=%d\n", rdwr_en_hi);
+
 	tmp = ioread32(denali->reg + RDWR_EN_HI_CNT);
 	tmp &= ~RDWR_EN_HI_CNT__VALUE;
 	tmp |= FIELD_PREP(RDWR_EN_HI_CNT__VALUE, rdwr_en_hi);
@@ -871,6 +900,13 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	rdwr_en_lo = max(rdwr_en_lo, rdwr_en_lo_hi - rdwr_en_hi);
 	rdwr_en_lo = min_t(int, rdwr_en_lo, RDWR_EN_LO_CNT__VALUE);
 
+	printk("Denali: tRP=%d\n", timings->tRP_min);
+	printk("Denali: tWP=%d\n", timings->tWP_min);
+	printk("Denali: tRC=%d\n", timings->tRC_min);
+	printk("Denali: tWC=%d\n", timings->tWC_min);
+	printk("Denali: rdwr_en_lo_hi=%d\n", rdwr_en_lo_hi);
+	printk("Denali: rdwr_en_lo=%d\n", rdwr_en_lo);
+
 	tmp = ioread32(denali->reg + RDWR_EN_LO_CNT);
 	tmp &= ~RDWR_EN_LO_CNT__VALUE;
 	tmp |= FIELD_PREP(RDWR_EN_LO_CNT__VALUE, rdwr_en_lo);
@@ -882,6 +918,10 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 			0);
 	cs_setup = min_t(int, cs_setup, CS_SETUP_CNT__VALUE);
 
+	printk("Denali: tCS=%d\n", timings->tCS_min);
+	printk("Denali: tCEA=%d\n", timings->tCEA_max);
+	printk("Denali: cs_setup=%d\n", cs_setup);
+
 	tmp = ioread32(denali->reg + CS_SETUP_CNT);
 	tmp &= ~CS_SETUP_CNT__VALUE;
 	tmp |= FIELD_PREP(CS_SETUP_CNT__VALUE, cs_setup);
-- 
2.24.1


[-- Attachment #3: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-19 18:27                         ` Marek Vasut
@ 2020-02-25  0:38                           ` Masahiro Yamada
  0 siblings, 0 replies; 31+ messages in thread
From: Masahiro Yamada @ 2020-02-25  0:38 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

Hi.

On Thu, Feb 20, 2020 at 3:44 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/17/20 9:33 AM, Masahiro Yamada wrote:
> > Hi,
>
> Hi,
>
> [...]
>
> > If possible, can you provide the dump of
> > the attached debug code?
>
> Without this revert:


Thanks for the dump.

denali_setup_data_interface() is called multiple times.

>
> Denali: clk_rate=31250000, clk_x_rate=125000000
> Denali: tREA=40000
> Denali: acc_clks=5
> Denali: tRHW=200000
> Denali: re_2_we=25
> Denali: tRHZ=200000
> Denali: re_2_re=25
> Denali: tCCS=500000000
> Denali: tWHR=120000
> Denali: we_2_re=63
> Denali: tADL=400000
> Denali: addr_2_data=50
> Denali: tREH=30000
> Denali: tWH=30000
> Denali: rdwr_en_hi=4
> Denali: tRP=50000
> Denali: tWP=50000
> Denali: tRC=100000
> Denali: tWC=100000
> Denali: rdwr_en_lo_hi=13
> Denali: rdwr_en_lo=9
> Denali: tCS=70000
> Denali: tCEA=100000
> Denali: cs_setup=8


This is the first call,
which I am not interested in.



> Denali: clk_rate=31250000, clk_x_rate=125000000
> Denali: tREA=16000
> Denali: acc_clks=2
> Denali: tRHW=100000
> Denali: re_2_we=13
> Denali: tRHZ=100000
> Denali: re_2_re=13
> Denali: tCCS=100000
> Denali: tWHR=80000
> Denali: we_2_re=13
> Denali: tADL=400000
> Denali: addr_2_data=50
> Denali: tREH=7000
> Denali: tWH=7000
> Denali: rdwr_en_hi=1
> Denali: tRP=10000
> Denali: tWP=10000
> Denali: tRC=20000
> Denali: tWC=20000
> Denali: rdwr_en_lo_hi=4
> Denali: rdwr_en_lo=3
> Denali: tCS=15000
> Denali: tCEA=25000
> Denali: cs_setup=2


This is the second call,
which sets up the final register values.

The parameter, acc_clks=2, is
the part I suspect the most.

(and that is why I attached two patches
to tweak acc_clks).



>
> With this revert, setup_data_interface() is not called, so there is no log.
>
> [...]
>
> >>> When denali->clk_x_rate is zero,
> >>> NAND_KEEP_TIMINGS would not be set with your patch.
> >>> So, ->setup_data_interface() would be called.
> >>>
> >>> This would cause zero divion at line 781.
> >>>         t_x = DIV_ROUND_DOWN_ULL(1000000000000ULL, denali->clk_x_rate);
> >>
> >> If the clock are non-zero, how would this be a division by zero ?
> >
> >
> > You have a false assumption "If the clock are non-zero...".
> >
> > clk_get_rate() may return zero if the clock driver
> > is not ready to provide the frequency information.
> >
> >
> >
> > The current code:
> >   If clk_rate or clk_x_rate is zero,
> >    do not call denali_setup_data_interface().
> >   If neither clk_rate nor clk_x is zero,
> >    call denali_setup_data_interface().
> >
> >
> > With your patch:
> >   If clk_rate or clk_x_rate is zero,
> >    call denali_setup_data_interface(),
> >    which causes zero division.
> >   If neither clk_rate nor clk_x is zero,
> >    do not call denali_setup_data_interface().
>
> OK, so it's just a miscommunication. In my case, neither of the clock
> are zero. On SoCFPGA, I think clk_rate = clk_x_rate / 4, but I'm not
> sure if that's always the case.


Drivers must be written in a generic manner
to take care of any possibility.

clk_get_rate() returns 0 if the clock driver
does not support ->recalc_rate() operation
or CONFIG_HAVE_CLK=n.



-- 
Best Regards
Masahiro Yamada

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-19 18:42                           ` Marek Vasut
@ 2020-02-25  0:41                             ` Masahiro Yamada
  2020-03-03 17:11                               ` Marek Vasut
  0 siblings, 1 reply; 31+ messages in thread
From: Masahiro Yamada @ 2020-02-25  0:41 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

Hi.

On Thu, Feb 20, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/18/20 6:55 AM, Masahiro Yamada wrote:
> > Hi
>
> Hi,
>
> [...]
>
> >> There is no change around the ->setup_data_interface() hook
> >> after v4.19
> >> The only difference I could think of is the clock frequency.
> >>
> >> But, it is OK if you do not want to test it.
> >>
> >> And you are confident.
> >>
> >> So, let's suspect the ->setup_data_interface() hook.
> >>
> >>
> >> If possible, can you provide the dump of
> >> the attached debug code?
> >>
> >
> >
> > I attached two experimental patches.
> >
> > I cannot test them because
> > the mainline code works fine for my boards.
> >
> > Does either of them improve something
> > on your settings?



I am still waiting for you to let me know
the result of my patches.



>
> Considering that the NAND works if denali_setup_data_interface() is not
> called, would it rather make sense to first read and print what's
> programmed into the controller and then print what the code calculated
> and intends to program into the controller ?

denali_select_target() is called every operation.
So, if you dumped this function for a working platform,
it might flood the printk buffer.

denali_setup_data_interface() is called just twice.
That's why I injected the debug code there.


>
> See attached patch, with which (without this revert) you get this:
> denali->reg + TWHR2_AND_WE_2_RE = 0x00001414 -> 0x0000143f
> denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
> denali->reg + RE_2_WE = 0x00000014 -> 0x00000019
> denali->reg + ACC_CLKS = 0x00000004 -> 0x00000005
> denali->reg + RDWR_EN_LO_CNT = 0x00000002 -> 0x00000009
> denali->reg + RDWR_EN_HI_CNT = 0x00000002 -> 0x00000004
> denali->reg + CS_SETUP_CNT = 0x00000001 -> 0x00000008
> denali->reg + RE_2_RE = 0x00000014 -> 0x00000019

OK, the left-hand side is probably the timing
set up by U-Boot.


-- 
Best Regards
Masahiro Yamada

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-02-25  0:41                             ` Masahiro Yamada
@ 2020-03-03 17:11                               ` Marek Vasut
  2020-03-09 10:27                                 ` Masahiro Yamada
  0 siblings, 1 reply; 31+ messages in thread
From: Marek Vasut @ 2020-03-03 17:11 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

On 2/25/20 1:41 AM, Masahiro Yamada wrote:
> Hi.

Hi,

> On Thu, Feb 20, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/18/20 6:55 AM, Masahiro Yamada wrote:
>>> Hi
>>
>> Hi,
>>
>> [...]
>>
>>>> There is no change around the ->setup_data_interface() hook
>>>> after v4.19
>>>> The only difference I could think of is the clock frequency.
>>>>
>>>> But, it is OK if you do not want to test it.
>>>>
>>>> And you are confident.
>>>>
>>>> So, let's suspect the ->setup_data_interface() hook.
>>>>
>>>>
>>>> If possible, can you provide the dump of
>>>> the attached debug code?
>>>>
>>>
>>>
>>> I attached two experimental patches.
>>>
>>> I cannot test them because
>>> the mainline code works fine for my boards.
>>>
>>> Does either of them improve something
>>> on your settings?
> 
> 
> 
> I am still waiting for you to let me know
> the result of my patches.

Neither patch works, sorry.

>> Considering that the NAND works if denali_setup_data_interface() is not
>> called, would it rather make sense to first read and print what's
>> programmed into the controller and then print what the code calculated
>> and intends to program into the controller ?
> 
> denali_select_target() is called every operation.
> So, if you dumped this function for a working platform,
> it might flood the printk buffer.
> 
> denali_setup_data_interface() is called just twice.
> That's why I injected the debug code there.
> 
> 
>>
>> See attached patch, with which (without this revert) you get this:
>> denali->reg + TWHR2_AND_WE_2_RE = 0x00001414 -> 0x0000143f
>> denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
>> denali->reg + RE_2_WE = 0x00000014 -> 0x00000019
>> denali->reg + ACC_CLKS = 0x00000004 -> 0x00000005
>> denali->reg + RDWR_EN_LO_CNT = 0x00000002 -> 0x00000009
>> denali->reg + RDWR_EN_HI_CNT = 0x00000002 -> 0x00000004
>> denali->reg + CS_SETUP_CNT = 0x00000001 -> 0x00000008
>> denali->reg + RE_2_RE = 0x00000014 -> 0x00000019
> 
> OK, the left-hand side is probably the timing
> set up by U-Boot.

Yep, the timings that work. So now, how do you get to those working
timings using the Linux driver ?

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-03-03 17:11                               ` Marek Vasut
@ 2020-03-09 10:27                                 ` Masahiro Yamada
  2020-03-11 12:52                                   ` Marek Vasut
  0 siblings, 1 reply; 31+ messages in thread
From: Masahiro Yamada @ 2020-03-09 10:27 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

Hi.

On Wed, Mar 4, 2020 at 2:13 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/25/20 1:41 AM, Masahiro Yamada wrote:
> > Hi.
>
> Hi,
>
> > On Thu, Feb 20, 2020 at 3:45 AM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 2/18/20 6:55 AM, Masahiro Yamada wrote:
> >>> Hi
> >>
> >> Hi,
> >>
> >> [...]
> >>
> >>>> There is no change around the ->setup_data_interface() hook
> >>>> after v4.19
> >>>> The only difference I could think of is the clock frequency.
> >>>>
> >>>> But, it is OK if you do not want to test it.
> >>>>
> >>>> And you are confident.
> >>>>
> >>>> So, let's suspect the ->setup_data_interface() hook.
> >>>>
> >>>>
> >>>> If possible, can you provide the dump of
> >>>> the attached debug code?
> >>>>
> >>>
> >>>
> >>> I attached two experimental patches.
> >>>
> >>> I cannot test them because
> >>> the mainline code works fine for my boards.
> >>>
> >>> Does either of them improve something
> >>> on your settings?
> >
> >
> >
> > I am still waiting for you to let me know
> > the result of my patches.
>
> Neither patch works, sorry.
>
> >> Considering that the NAND works if denali_setup_data_interface() is not
> >> called, would it rather make sense to first read and print what's
> >> programmed into the controller and then print what the code calculated
> >> and intends to program into the controller ?
> >
> > denali_select_target() is called every operation.
> > So, if you dumped this function for a working platform,
> > it might flood the printk buffer.
> >
> > denali_setup_data_interface() is called just twice.
> > That's why I injected the debug code there.
> >
> >
> >>
> >> See attached patch, with which (without this revert) you get this:
> >> denali->reg + TWHR2_AND_WE_2_RE = 0x00001414 -> 0x0000143f
> >> denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
> >> denali->reg + RE_2_WE = 0x00000014 -> 0x00000019
> >> denali->reg + ACC_CLKS = 0x00000004 -> 0x00000005
> >> denali->reg + RDWR_EN_LO_CNT = 0x00000002 -> 0x00000009
> >> denali->reg + RDWR_EN_HI_CNT = 0x00000002 -> 0x00000004
> >> denali->reg + CS_SETUP_CNT = 0x00000001 -> 0x00000008
> >> denali->reg + RE_2_RE = 0x00000014 -> 0x00000019
> >
> > OK, the left-hand side is probably the timing
> > set up by U-Boot.
>
> Yep, the timings that work. So now, how do you get to those working
> timings using the Linux driver ?


How about
0001-denali-more-complicated-calculation-for-timings.patch

+ following ?

diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
index b0482108a127..ea38aa42873e 100644
--- a/drivers/mtd/nand/raw/denali.c
+++ b/drivers/mtd/nand/raw/denali.c
@@ -860,9 +860,9 @@ static int denali_setup_data_interface(struct
nand_chip *chip, int chipnr,

        /*
         * Determine the minimum of acc_clks to meet the data setup timing.
-        * (one additional clock cycle just in case)
+        * (two additional clock cycles just in case)
         */
-       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 1;
+       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 2;

        /* Determine the minimum of rdwr_en_lo_cnt from RE#/WE# pulse width */
        rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min), t_x);


-- 
Best Regards
Masahiro Yamada

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-03-09 10:27                                 ` Masahiro Yamada
@ 2020-03-11 12:52                                   ` Marek Vasut
  2020-03-11 13:08                                     ` Miquel Raynal
  0 siblings, 1 reply; 31+ messages in thread
From: Marek Vasut @ 2020-03-11 12:52 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

[-- Attachment #1: Type: text/plain, Size: 3827 bytes --]

On 3/9/20 11:27 AM, Masahiro Yamada wrote:
> Hi.

Hi,

[...]

>>>> See attached patch, with which (without this revert) you get this:
>>>> denali->reg + TWHR2_AND_WE_2_RE = 0x00001414 -> 0x0000143f
>>>> denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
>>>> denali->reg + RE_2_WE = 0x00000014 -> 0x00000019
>>>> denali->reg + ACC_CLKS = 0x00000004 -> 0x00000005
>>>> denali->reg + RDWR_EN_LO_CNT = 0x00000002 -> 0x00000009
>>>> denali->reg + RDWR_EN_HI_CNT = 0x00000002 -> 0x00000004
>>>> denali->reg + CS_SETUP_CNT = 0x00000001 -> 0x00000008
>>>> denali->reg + RE_2_RE = 0x00000014 -> 0x00000019
>>>
>>> OK, the left-hand side is probably the timing
>>> set up by U-Boot.
>>
>> Yep, the timings that work. So now, how do you get to those working
>> timings using the Linux driver ?
> 
> 
> How about
> 0001-denali-more-complicated-calculation-for-timings.patch
> 
> + following ?
> 
> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> index b0482108a127..ea38aa42873e 100644
> --- a/drivers/mtd/nand/raw/denali.c
> +++ b/drivers/mtd/nand/raw/denali.c
> @@ -860,9 +860,9 @@ static int denali_setup_data_interface(struct
> nand_chip *chip, int chipnr,
> 
>         /*
>          * Determine the minimum of acc_clks to meet the data setup timing.
> -        * (one additional clock cycle just in case)
> +        * (two additional clock cycles just in case)
>          */
> -       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 1;
> +       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 2;
> 
>         /* Determine the minimum of rdwr_en_lo_cnt from RE#/WE# pulse width */
>         rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min), t_x);

Like the attached one ?

That seems to work, but -- the calculated timings differ from the ones
which are calculated by U-Boot and which were tested to work well.
That's not good, I would expect both timings to be identical:
 Denali: clk_rate=31250000, clk_x_rate=125000000
 Denali: tREA=40000
 Denali: tRHW=200000
 Denali: tRHZ=200000
 Denali: tCCS=500000000
 Denali: tWHR=120000
 Denali: tADL=400000
 Denali: tREH=30000
 Denali: tWH=30000
 Denali: tRP=50000
 Denali: tWP=50000
 Denali: tRC=100000
 Denali: tWC=100000
 Denali: tCS=70000
 Denali: tCEA=100000
 Denali: acc_clks=8
 Denali: re_2_we=25
 Denali: re_2_re=25
 Denali: we_2_re=63
 Denali: addr_2_data=50
 Denali: rdwr_en_hi=4
 Denali: rdwr_en_lo_hi=13
 Denali: rdwr_en_lo=9
 Denali: cs_setup=5

 denali->reg + TWHR2_AND_WE_2_RE = 0x00001414 -> 0x0000143f
 denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
 denali->reg + RE_2_WE = 0x00000014 -> 0x00000019
 denali->reg + ACC_CLKS = 0x00000004 -> 0x00000008
 denali->reg + RDWR_EN_LO_CNT = 0x00000002 -> 0x00000009
 denali->reg + RDWR_EN_HI_CNT = 0x00000002 -> 0x00000004
 denali->reg + CS_SETUP_CNT = 0x00000001 -> 0x00000005
 denali->reg + RE_2_RE = 0x00000014 -> 0x00000019

 denali->reg + TWHR2_AND_WE_2_RE = 0x0000143f -> 0x0000143f
 denali->reg + TCWAW_AND_ADDR_2_DATA = 0x00001432 -> 0x00001432
 denali->reg + RE_2_WE = 0x00000019 -> 0x00000019
 denali->reg + ACC_CLKS = 0x00000008 -> 0x00000008
 denali->reg + RDWR_EN_LO_CNT = 0x00000009 -> 0x00000009
 denali->reg + RDWR_EN_HI_CNT = 0x00000004 -> 0x00000004
 denali->reg + CS_SETUP_CNT = 0x00000005 -> 0x00000005
 denali->reg + RE_2_RE = 0x00000019 -> 0x00000019

 denali->reg + TWHR2_AND_WE_2_RE = 0x0000143f -> 0x0000143f
 denali->reg + TCWAW_AND_ADDR_2_DATA = 0x00001432 -> 0x00001432
 denali->reg + RE_2_WE = 0x00000019 -> 0x00000019
 denali->reg + ACC_CLKS = 0x00000008 -> 0x00000008
 denali->reg + RDWR_EN_LO_CNT = 0x00000009 -> 0x00000009
 denali->reg + RDWR_EN_HI_CNT = 0x00000004 -> 0x00000004
 denali->reg + CS_SETUP_CNT = 0x00000005 -> 0x00000005
 denali->reg + RE_2_RE = 0x00000019 -> 0x00000019
...

-- 
Best regards,
Marek Vasut

[-- Attachment #2: 0001-denali-more-complicated-calculation-for-timings.patch --]
[-- Type: text/x-patch, Size: 5855 bytes --]

>From 5fd2ee8e18de2490da347d0388481adb18a04408 Mon Sep 17 00:00:00 2001
From: Masahiro Yamada <yamada.masahiro@socionext.com>
Date: Mon, 17 Feb 2020 21:16:33 +0900
Subject: [PATCH] denali: more complicated calculation for timings

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
 drivers/mtd/nand/raw/denali.c | 75 +++++++++++++++++++++++++++++------
 1 file changed, 62 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
index 5fe3c62a756e..ef5ebb8394f3 100644
--- a/drivers/mtd/nand/raw/denali.c
+++ b/drivers/mtd/nand/raw/denali.c
@@ -218,14 +218,21 @@ static void denali_select_target(struct nand_chip *chip, int cs)
 		return;
 
 	/* update timing registers unless NAND_KEEP_TIMINGS is set */
+	pr_err("denali->reg + TWHR2_AND_WE_2_RE = 0x%08x -> 0x%08x\n", ioread32(denali->reg + TWHR2_AND_WE_2_RE), sel->hwhr2_and_we_2_re);
 	iowrite32(sel->hwhr2_and_we_2_re, denali->reg + TWHR2_AND_WE_2_RE);
-	iowrite32(sel->tcwaw_and_addr_2_data,
-		  denali->reg + TCWAW_AND_ADDR_2_DATA);
+	pr_err("denali->reg + TCWAW_AND_ADDR_2_DATA = 0x%08x -> 0x%08x\n", ioread32(denali->reg + TCWAW_AND_ADDR_2_DATA), sel->tcwaw_and_addr_2_data);
+	iowrite32(sel->tcwaw_and_addr_2_data, denali->reg + TCWAW_AND_ADDR_2_DATA);
+	pr_err("denali->reg + RE_2_WE = 0x%08x -> 0x%08x\n", ioread32(denali->reg + RE_2_WE), sel->re_2_we);
 	iowrite32(sel->re_2_we, denali->reg + RE_2_WE);
+	pr_err("denali->reg + ACC_CLKS = 0x%08x -> 0x%08x\n", ioread32(denali->reg + ACC_CLKS), sel->acc_clks);
 	iowrite32(sel->acc_clks, denali->reg + ACC_CLKS);
+	pr_err("denali->reg + RDWR_EN_LO_CNT = 0x%08x -> 0x%08x\n", ioread32(denali->reg + RDWR_EN_LO_CNT), sel->rdwr_en_lo_cnt);
 	iowrite32(sel->rdwr_en_lo_cnt, denali->reg + RDWR_EN_LO_CNT);
+	pr_err("denali->reg + RDWR_EN_HI_CNT = 0x%08x -> 0x%08x\n", ioread32(denali->reg + RDWR_EN_HI_CNT), sel->rdwr_en_hi_cnt);
 	iowrite32(sel->rdwr_en_hi_cnt, denali->reg + RDWR_EN_HI_CNT);
+	pr_err("denali->reg + CS_SETUP_CNT = 0x%08x -> 0x%08x\n", ioread32(denali->reg + CS_SETUP_CNT), sel->cs_setup_cnt);
 	iowrite32(sel->cs_setup_cnt, denali->reg + CS_SETUP_CNT);
+	pr_err("denali->reg + RE_2_RE = 0x%08x -> 0x%08x\n", ioread32(denali->reg + RE_2_RE), sel->re_2_re);
 	iowrite32(sel->re_2_re, denali->reg + RE_2_RE);
 }
 
@@ -797,15 +804,6 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 
 	sel = &to_denali_chip(chip)->sels[chipnr];
 
-	/* tREA -> ACC_CLKS */
-	acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x);
-	acc_clks = min_t(int, acc_clks, ACC_CLKS__VALUE);
-
-	tmp = ioread32(denali->reg + ACC_CLKS);
-	tmp &= ~ACC_CLKS__VALUE;
-	tmp |= FIELD_PREP(ACC_CLKS__VALUE, acc_clks);
-	sel->acc_clks = tmp;
-
 	/* tRWH -> RE_2_WE */
 	re_2_we = DIV_ROUND_UP(timings->tRHW_min, t_x);
 	re_2_we = min_t(int, re_2_we, RE_2_WE__VALUE);
@@ -863,14 +861,39 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	tmp |= FIELD_PREP(RDWR_EN_HI_CNT__VALUE, rdwr_en_hi);
 	sel->rdwr_en_hi_cnt = tmp;
 
-	/* tRP, tWP -> RDWR_EN_LO_CNT */
+	/*
+	 * tREA -> ACC_CLOCKS
+	 * tRP, tWP, tRHOH, tRC, tWC -> RDWR_EN_LO_CNT
+	 */
+
+	/*
+	 * Determine the minimum of acc_clks to meet the data setup timing.
+	 * (one additional clock cycle just in case)
+	 */
+	acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 2;
+
+	/* Determine the minimum of rdwr_en_lo_cnt from RE#/WE# pulse width */
 	rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min), t_x);
+
+	/* Extend rdwr_en_lo to meet the data hold timing */
+	rdwr_en_lo = max_t(int, rdwr_en_lo, acc_clks - timings->tRHOH_min / t_x);
+
+	/* Extend rdwr_en_lo to meet the requirement for RE#/WE# cycle time */
 	rdwr_en_lo_hi = DIV_ROUND_UP(max(timings->tRC_min, timings->tWC_min),
 				     t_x);
-	rdwr_en_lo_hi = max_t(int, rdwr_en_lo_hi, mult_x);
 	rdwr_en_lo = max(rdwr_en_lo, rdwr_en_lo_hi - rdwr_en_hi);
 	rdwr_en_lo = min_t(int, rdwr_en_lo, RDWR_EN_LO_CNT__VALUE);
 
+	/* Center the data latch timing for extra safety */
+	acc_clks = (acc_clks + rdwr_en_lo +
+		    DIV_ROUND_UP(timings->tRHOH_min, t_x)) / 2;
+	acc_clks = min_t(int, acc_clks, ACC_CLKS__VALUE);
+
+	tmp = ioread32(denali->reg + ACC_CLKS);
+	tmp &= ~ACC_CLKS__VALUE;
+	tmp |= FIELD_PREP(ACC_CLKS__VALUE, acc_clks);
+	sel->acc_clks = tmp;
+
 	tmp = ioread32(denali->reg + RDWR_EN_LO_CNT);
 	tmp &= ~RDWR_EN_LO_CNT__VALUE;
 	tmp |= FIELD_PREP(RDWR_EN_LO_CNT__VALUE, rdwr_en_lo);
@@ -887,6 +910,32 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	tmp |= FIELD_PREP(CS_SETUP_CNT__VALUE, cs_setup);
 	sel->cs_setup_cnt = tmp;
 
+	/* debug */
+	printk("Denali: clk_rate=%ld, clk_x_rate=%ld\n", denali->clk_rate, denali->clk_x_rate);
+	printk("Denali: tREA=%d\n", timings->tREA_max);
+	printk("Denali: tRHW=%d\n", timings->tRHW_min);
+	printk("Denali: tRHZ=%d\n", timings->tRHZ_max);
+	printk("Denali: tCCS=%d\n", timings->tCCS_min);
+	printk("Denali: tWHR=%d\n", timings->tWHR_min);
+	printk("Denali: tADL=%d\n", timings->tADL_min);
+	printk("Denali: tREH=%d\n", timings->tREH_min);
+	printk("Denali: tWH=%d\n", timings->tWH_min);
+	printk("Denali: tRP=%d\n", timings->tRP_min);
+	printk("Denali: tWP=%d\n", timings->tWP_min);
+	printk("Denali: tRC=%d\n", timings->tRC_min);
+	printk("Denali: tWC=%d\n", timings->tWC_min);
+	printk("Denali: tCS=%d\n", timings->tCS_min);
+	printk("Denali: tCEA=%d\n", timings->tCEA_max);
+	printk("Denali: acc_clks=%d\n", acc_clks);
+	printk("Denali: re_2_we=%d\n", re_2_we);
+	printk("Denali: re_2_re=%d\n", re_2_re);
+	printk("Denali: we_2_re=%d\n", we_2_re);
+	printk("Denali: addr_2_data=%d\n", addr_2_data);
+	printk("Denali: rdwr_en_hi=%d\n", rdwr_en_hi);
+	printk("Denali: rdwr_en_lo_hi=%d\n", rdwr_en_lo_hi);
+	printk("Denali: rdwr_en_lo=%d\n", rdwr_en_lo);
+	printk("Denali: cs_setup=%d\n", cs_setup);
+
 	return 0;
 }
 
-- 
2.25.0


[-- Attachment #3: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-03-11 12:52                                   ` Marek Vasut
@ 2020-03-11 13:08                                     ` Miquel Raynal
  2020-03-11 13:19                                       ` Marek Vasut
  0 siblings, 1 reply; 31+ messages in thread
From: Miquel Raynal @ 2020-03-11 13:08 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Masahiro Yamada, Boris Brezillon, linux-mtd, Tim Sander

Hi Marek,

Marek Vasut <marex@denx.de> wrote on Wed, 11 Mar 2020 13:52:30 +0100:

> On 3/9/20 11:27 AM, Masahiro Yamada wrote:
> > Hi.  
> 
> Hi,
> 
> [...]
> 
> >>>> See attached patch, with which (without this revert) you get this:
> >>>> denali->reg + TWHR2_AND_WE_2_RE = 0x00001414 -> 0x0000143f
> >>>> denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
> >>>> denali->reg + RE_2_WE = 0x00000014 -> 0x00000019
> >>>> denali->reg + ACC_CLKS = 0x00000004 -> 0x00000005
> >>>> denali->reg + RDWR_EN_LO_CNT = 0x00000002 -> 0x00000009
> >>>> denali->reg + RDWR_EN_HI_CNT = 0x00000002 -> 0x00000004
> >>>> denali->reg + CS_SETUP_CNT = 0x00000001 -> 0x00000008
> >>>> denali->reg + RE_2_RE = 0x00000014 -> 0x00000019  
> >>>
> >>> OK, the left-hand side is probably the timing
> >>> set up by U-Boot.  
> >>
> >> Yep, the timings that work. So now, how do you get to those working
> >> timings using the Linux driver ?  
> > 
> > 
> > How about
> > 0001-denali-more-complicated-calculation-for-timings.patch
> > 
> > + following ?
> > 
> > diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> > index b0482108a127..ea38aa42873e 100644
> > --- a/drivers/mtd/nand/raw/denali.c
> > +++ b/drivers/mtd/nand/raw/denali.c
> > @@ -860,9 +860,9 @@ static int denali_setup_data_interface(struct
> > nand_chip *chip, int chipnr,
> > 
> >         /*
> >          * Determine the minimum of acc_clks to meet the data setup timing.
> > -        * (one additional clock cycle just in case)
> > +        * (two additional clock cycles just in case)
> >          */
> > -       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 1;
> > +       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 2;
> > 
> >         /* Determine the minimum of rdwr_en_lo_cnt from RE#/WE# pulse width */
> >         rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min), t_x);  
> 
> Like the attached one ?
> 
> That seems to work, but -- the calculated timings differ from the ones
> which are calculated by U-Boot and which were tested to work well.
> That's not good, I would expect both timings to be identical:

There is no such "timings tested to work well". Timings represent
minimum and maximum values for certain operations on the NAND bus, you
can have two different values that will both work in the same
condition. And it is expected that Linux is more clever than U-Boot and
may optimize better the timings depending on the selected mode ([0-5])
(hence the different calls to ->setup_data_interface().

Run a stress test, if it passes, you should be good :)

Thanks,
Miquèl

>  Denali: clk_rate=31250000, clk_x_rate=125000000
>  Denali: tREA=40000
>  Denali: tRHW=200000
>  Denali: tRHZ=200000
>  Denali: tCCS=500000000
>  Denali: tWHR=120000
>  Denali: tADL=400000
>  Denali: tREH=30000
>  Denali: tWH=30000
>  Denali: tRP=50000
>  Denali: tWP=50000
>  Denali: tRC=100000
>  Denali: tWC=100000
>  Denali: tCS=70000
>  Denali: tCEA=100000
>  Denali: acc_clks=8
>  Denali: re_2_we=25
>  Denali: re_2_re=25
>  Denali: we_2_re=63
>  Denali: addr_2_data=50
>  Denali: rdwr_en_hi=4
>  Denali: rdwr_en_lo_hi=13
>  Denali: rdwr_en_lo=9
>  Denali: cs_setup=5
> 
>  denali->reg + TWHR2_AND_WE_2_RE = 0x00001414 -> 0x0000143f
>  denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
>  denali->reg + RE_2_WE = 0x00000014 -> 0x00000019
>  denali->reg + ACC_CLKS = 0x00000004 -> 0x00000008
>  denali->reg + RDWR_EN_LO_CNT = 0x00000002 -> 0x00000009
>  denali->reg + RDWR_EN_HI_CNT = 0x00000002 -> 0x00000004
>  denali->reg + CS_SETUP_CNT = 0x00000001 -> 0x00000005
>  denali->reg + RE_2_RE = 0x00000014 -> 0x00000019
> 
>  denali->reg + TWHR2_AND_WE_2_RE = 0x0000143f -> 0x0000143f
>  denali->reg + TCWAW_AND_ADDR_2_DATA = 0x00001432 -> 0x00001432
>  denali->reg + RE_2_WE = 0x00000019 -> 0x00000019
>  denali->reg + ACC_CLKS = 0x00000008 -> 0x00000008
>  denali->reg + RDWR_EN_LO_CNT = 0x00000009 -> 0x00000009
>  denali->reg + RDWR_EN_HI_CNT = 0x00000004 -> 0x00000004
>  denali->reg + CS_SETUP_CNT = 0x00000005 -> 0x00000005
>  denali->reg + RE_2_RE = 0x00000019 -> 0x00000019
> 
>  denali->reg + TWHR2_AND_WE_2_RE = 0x0000143f -> 0x0000143f
>  denali->reg + TCWAW_AND_ADDR_2_DATA = 0x00001432 -> 0x00001432
>  denali->reg + RE_2_WE = 0x00000019 -> 0x00000019
>  denali->reg + ACC_CLKS = 0x00000008 -> 0x00000008
>  denali->reg + RDWR_EN_LO_CNT = 0x00000009 -> 0x00000009
>  denali->reg + RDWR_EN_HI_CNT = 0x00000004 -> 0x00000004
>  denali->reg + CS_SETUP_CNT = 0x00000005 -> 0x00000005
>  denali->reg + RE_2_RE = 0x00000019 -> 0x00000019
> ...
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-03-11 13:08                                     ` Miquel Raynal
@ 2020-03-11 13:19                                       ` Marek Vasut
  2020-03-11 13:33                                         ` Miquel Raynal
  0 siblings, 1 reply; 31+ messages in thread
From: Marek Vasut @ 2020-03-11 13:19 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Dinh Nguyen, Masahiro Yamada, Boris Brezillon, linux-mtd, Tim Sander

On 3/11/20 2:08 PM, Miquel Raynal wrote:
> Hi Marek,

Hi Miquel,

> Marek Vasut <marex@denx.de> wrote on Wed, 11 Mar 2020 13:52:30 +0100:
> 
>> On 3/9/20 11:27 AM, Masahiro Yamada wrote:
>>> Hi.  
>>
>> Hi,
>>
>> [...]
>>
>>>>>> See attached patch, with which (without this revert) you get this:
>>>>>> denali->reg + TWHR2_AND_WE_2_RE = 0x00001414 -> 0x0000143f
>>>>>> denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
>>>>>> denali->reg + RE_2_WE = 0x00000014 -> 0x00000019
>>>>>> denali->reg + ACC_CLKS = 0x00000004 -> 0x00000005
>>>>>> denali->reg + RDWR_EN_LO_CNT = 0x00000002 -> 0x00000009
>>>>>> denali->reg + RDWR_EN_HI_CNT = 0x00000002 -> 0x00000004
>>>>>> denali->reg + CS_SETUP_CNT = 0x00000001 -> 0x00000008
>>>>>> denali->reg + RE_2_RE = 0x00000014 -> 0x00000019  
>>>>>
>>>>> OK, the left-hand side is probably the timing
>>>>> set up by U-Boot.  
>>>>
>>>> Yep, the timings that work. So now, how do you get to those working
>>>> timings using the Linux driver ?  
>>>
>>>
>>> How about
>>> 0001-denali-more-complicated-calculation-for-timings.patch
>>>
>>> + following ?
>>>
>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
>>> index b0482108a127..ea38aa42873e 100644
>>> --- a/drivers/mtd/nand/raw/denali.c
>>> +++ b/drivers/mtd/nand/raw/denali.c
>>> @@ -860,9 +860,9 @@ static int denali_setup_data_interface(struct
>>> nand_chip *chip, int chipnr,
>>>
>>>         /*
>>>          * Determine the minimum of acc_clks to meet the data setup timing.
>>> -        * (one additional clock cycle just in case)
>>> +        * (two additional clock cycles just in case)
>>>          */
>>> -       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 1;
>>> +       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 2;
>>>
>>>         /* Determine the minimum of rdwr_en_lo_cnt from RE#/WE# pulse width */
>>>         rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min), t_x);  
>>
>> Like the attached one ?
>>
>> That seems to work, but -- the calculated timings differ from the ones
>> which are calculated by U-Boot and which were tested to work well.
>> That's not good, I would expect both timings to be identical:
> 
> There is no such "timings tested to work well".

Hmmm, the board went through full temperature range testing in a chamber
with those timings and passed, and there are boards with those exact
timings deployed for years now with older kernel version, which work
too. So I would expect they are good and "timings tested to work well".

> Timings represent
> minimum and maximum values for certain operations on the NAND bus, you
> can have two different values that will both work in the same
> condition. And it is expected that Linux is more clever than U-Boot

Errr, why ?

> and
> may optimize better the timings depending on the selected mode ([0-5])
> (hence the different calls to ->setup_data_interface().

I would expect those two should produce identical timing parameters,
period, otherwise one or the other is wrong. Thus far, it was Linux that
produced non-working results.

> Run a stress test, if it passes, you should be good :)

Thank you for the hint, I think the stress test thus far could be
considered sufficient. I guess we can agree on that ?

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-03-11 13:19                                       ` Marek Vasut
@ 2020-03-11 13:33                                         ` Miquel Raynal
  2020-03-11 14:07                                           ` Marek Vasut
  0 siblings, 1 reply; 31+ messages in thread
From: Miquel Raynal @ 2020-03-11 13:33 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Masahiro Yamada, Boris Brezillon, linux-mtd, Tim Sander

Hi Marek,

Marek Vasut <marex@denx.de> wrote on Wed, 11 Mar 2020 14:19:17 +0100:

> On 3/11/20 2:08 PM, Miquel Raynal wrote:
> > Hi Marek,  
> 
> Hi Miquel,
> 
> > Marek Vasut <marex@denx.de> wrote on Wed, 11 Mar 2020 13:52:30 +0100:
> >   
> >> On 3/9/20 11:27 AM, Masahiro Yamada wrote:  
> >>> Hi.    
> >>
> >> Hi,
> >>
> >> [...]
> >>  
> >>>>>> See attached patch, with which (without this revert) you get this:
> >>>>>> denali->reg + TWHR2_AND_WE_2_RE = 0x00001414 -> 0x0000143f
> >>>>>> denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
> >>>>>> denali->reg + RE_2_WE = 0x00000014 -> 0x00000019
> >>>>>> denali->reg + ACC_CLKS = 0x00000004 -> 0x00000005
> >>>>>> denali->reg + RDWR_EN_LO_CNT = 0x00000002 -> 0x00000009
> >>>>>> denali->reg + RDWR_EN_HI_CNT = 0x00000002 -> 0x00000004
> >>>>>> denali->reg + CS_SETUP_CNT = 0x00000001 -> 0x00000008
> >>>>>> denali->reg + RE_2_RE = 0x00000014 -> 0x00000019    
> >>>>>
> >>>>> OK, the left-hand side is probably the timing
> >>>>> set up by U-Boot.    
> >>>>
> >>>> Yep, the timings that work. So now, how do you get to those working
> >>>> timings using the Linux driver ?    
> >>>
> >>>
> >>> How about
> >>> 0001-denali-more-complicated-calculation-for-timings.patch
> >>>
> >>> + following ?
> >>>
> >>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> >>> index b0482108a127..ea38aa42873e 100644
> >>> --- a/drivers/mtd/nand/raw/denali.c
> >>> +++ b/drivers/mtd/nand/raw/denali.c
> >>> @@ -860,9 +860,9 @@ static int denali_setup_data_interface(struct
> >>> nand_chip *chip, int chipnr,
> >>>
> >>>         /*
> >>>          * Determine the minimum of acc_clks to meet the data setup timing.
> >>> -        * (one additional clock cycle just in case)
> >>> +        * (two additional clock cycles just in case)
> >>>          */
> >>> -       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 1;
> >>> +       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 2;
> >>>
> >>>         /* Determine the minimum of rdwr_en_lo_cnt from RE#/WE# pulse width */
> >>>         rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min), t_x);    
> >>
> >> Like the attached one ?
> >>
> >> That seems to work, but -- the calculated timings differ from the ones
> >> which are calculated by U-Boot and which were tested to work well.
> >> That's not good, I would expect both timings to be identical:  
> > 
> > There is no such "timings tested to work well".  
> 
> Hmmm, the board went through full temperature range testing in a chamber
> with those timings and passed, and there are boards with those exact
> timings deployed for years now with older kernel version, which work
> too. So I would expect they are good and "timings tested to work well".
> 
> > Timings represent
> > minimum and maximum values for certain operations on the NAND bus, you
> > can have two different values that will both work in the same
> > condition. And it is expected that Linux is more clever than U-Boot  
> 
> Errr, why ?

Because sometimes people write simpler driver in U-Boot, or even
hardcoded values.

I checked the denali driver and indeed u-boot should not be much clever
than Linux. Are the differences significant? The code is so close, you
can probably check why you have differences. Also verify that the same
ONFI mode is used.

> 
> > and
> > may optimize better the timings depending on the selected mode ([0-5])
> > (hence the different calls to ->setup_data_interface().  
> 
> I would expect those two should produce identical timing parameters,
> period, otherwise one or the other is wrong. Thus far, it was Linux that
> produced non-working results.

Again, we define minimum and maximum delays. If the right thing is to
not wait more than 5us and you wait up to 6, it does not mean you
wrote "bad timings". 4us would be a bad timing though. It depends on
what you are looking at.

> 
> > Run a stress test, if it passes, you should be good :)  
> 
> Thank you for the hint, I think the stress test thus far could be
> considered sufficient. I guess we can agree on that ?

Oh yeah absolutely :)

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-03-11 13:33                                         ` Miquel Raynal
@ 2020-03-11 14:07                                           ` Marek Vasut
  2020-03-11 14:39                                             ` Miquel Raynal
  0 siblings, 1 reply; 31+ messages in thread
From: Marek Vasut @ 2020-03-11 14:07 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Dinh Nguyen, Masahiro Yamada, Boris Brezillon, linux-mtd, Tim Sander

On 3/11/20 2:33 PM, Miquel Raynal wrote:
> Hi Marek,

Hi,

[...]

>>>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
>>>>> index b0482108a127..ea38aa42873e 100644
>>>>> --- a/drivers/mtd/nand/raw/denali.c
>>>>> +++ b/drivers/mtd/nand/raw/denali.c
>>>>> @@ -860,9 +860,9 @@ static int denali_setup_data_interface(struct
>>>>> nand_chip *chip, int chipnr,
>>>>>
>>>>>         /*
>>>>>          * Determine the minimum of acc_clks to meet the data setup timing.
>>>>> -        * (one additional clock cycle just in case)
>>>>> +        * (two additional clock cycles just in case)
>>>>>          */
>>>>> -       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 1;
>>>>> +       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 2;
>>>>>
>>>>>         /* Determine the minimum of rdwr_en_lo_cnt from RE#/WE# pulse width */
>>>>>         rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min), t_x);    
>>>>
>>>> Like the attached one ?
>>>>
>>>> That seems to work, but -- the calculated timings differ from the ones
>>>> which are calculated by U-Boot and which were tested to work well.
>>>> That's not good, I would expect both timings to be identical:  
>>>
>>> There is no such "timings tested to work well".  
>>
>> Hmmm, the board went through full temperature range testing in a chamber
>> with those timings and passed, and there are boards with those exact
>> timings deployed for years now with older kernel version, which work
>> too. So I would expect they are good and "timings tested to work well".
>>
>>> Timings represent
>>> minimum and maximum values for certain operations on the NAND bus, you
>>> can have two different values that will both work in the same
>>> condition. And it is expected that Linux is more clever than U-Boot  
>>
>> Errr, why ?
> 
> Because sometimes people write simpler driver in U-Boot, or even
> hardcoded values.

I see, this is not the case with denali nand driver though.

> I checked the denali driver and indeed u-boot should not be much clever
> than Linux. Are the differences significant? The code is so close, you
> can probably check why you have differences. Also verify that the same
> ONFI mode is used.

It might've made sense to check those driver differences before making
such an statement ;-)

That said, I don't think either U-Boot or Linux uses the ONFI
information for this NAND, but I might be wrong.

>>> and
>>> may optimize better the timings depending on the selected mode ([0-5])
>>> (hence the different calls to ->setup_data_interface().  
>>
>> I would expect those two should produce identical timing parameters,
>> period, otherwise one or the other is wrong. Thus far, it was Linux that
>> produced non-working results.
> 
> Again, we define minimum and maximum delays. If the right thing is to
> not wait more than 5us and you wait up to 6, it does not mean you
> wrote "bad timings". 4us would be a bad timing though. It depends on
> what you are looking at.

I am look at for example

 denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432

Register was 0x143f before, now is 0x1432 , which is less.
I guess that would be the "bad timing" then ?

>>> Run a stress test, if it passes, you should be good :)  
>>
>> Thank you for the hint, I think the stress test thus far could be
>> considered sufficient. I guess we can agree on that ?
> 
> Oh yeah absolutely :)

Great :)

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-03-11 14:07                                           ` Marek Vasut
@ 2020-03-11 14:39                                             ` Miquel Raynal
  2020-03-14 14:48                                               ` Marek Vasut
  2020-03-16  4:36                                               ` Masahiro Yamada
  0 siblings, 2 replies; 31+ messages in thread
From: Miquel Raynal @ 2020-03-11 14:39 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Masahiro Yamada, Boris Brezillon, linux-mtd, Tim Sander

Hi Marek,

Marek Vasut <marex@denx.de> wrote on Wed, 11 Mar 2020 15:07:27 +0100:

> On 3/11/20 2:33 PM, Miquel Raynal wrote:
> > Hi Marek,  
> 
> Hi,
> 
> [...]
> 
> >>>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> >>>>> index b0482108a127..ea38aa42873e 100644
> >>>>> --- a/drivers/mtd/nand/raw/denali.c
> >>>>> +++ b/drivers/mtd/nand/raw/denali.c
> >>>>> @@ -860,9 +860,9 @@ static int denali_setup_data_interface(struct
> >>>>> nand_chip *chip, int chipnr,
> >>>>>
> >>>>>         /*
> >>>>>          * Determine the minimum of acc_clks to meet the data setup timing.
> >>>>> -        * (one additional clock cycle just in case)
> >>>>> +        * (two additional clock cycles just in case)
> >>>>>          */
> >>>>> -       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 1;
> >>>>> +       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 2;
> >>>>>
> >>>>>         /* Determine the minimum of rdwr_en_lo_cnt from RE#/WE# pulse width */
> >>>>>         rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min), t_x);      
> >>>>
> >>>> Like the attached one ?
> >>>>
> >>>> That seems to work, but -- the calculated timings differ from the ones
> >>>> which are calculated by U-Boot and which were tested to work well.
> >>>> That's not good, I would expect both timings to be identical:    
> >>>
> >>> There is no such "timings tested to work well".    
> >>
> >> Hmmm, the board went through full temperature range testing in a chamber
> >> with those timings and passed, and there are boards with those exact
> >> timings deployed for years now with older kernel version, which work
> >> too. So I would expect they are good and "timings tested to work well".
> >>  
> >>> Timings represent
> >>> minimum and maximum values for certain operations on the NAND bus, you
> >>> can have two different values that will both work in the same
> >>> condition. And it is expected that Linux is more clever than U-Boot    
> >>
> >> Errr, why ?  
> > 
> > Because sometimes people write simpler driver in U-Boot, or even
> > hardcoded values.  
> 
> I see, this is not the case with denali nand driver though.
> 
> > I checked the denali driver and indeed u-boot should not be much clever
> > than Linux. Are the differences significant? The code is so close, you
> > can probably check why you have differences. Also verify that the same
> > ONFI mode is used.  
> 
> It might've made sense to check those driver differences before making
> such an statement ;-)
> That said, I don't think either U-Boot or Linux uses the ONFI
> information for this NAND, but I might be wrong.

I don't know what is the exact device but most of the time, even for
non ONFI-compliant chips, the core starts talking at the lowest ONFI
speed (mode 0) and then negotiate with the NAND chip the actual timings
to use. This works if get/set_features is supported, otherwise you
might have a default mode somewhere. Is it the same in both cases? Does
the core tries to apply the same timings? Is the calculation the same?

These are pointers but I am sure you can figure all that out.

> >>> and
> >>> may optimize better the timings depending on the selected mode ([0-5])
> >>> (hence the different calls to ->setup_data_interface().    
> >>
> >> I would expect those two should produce identical timing parameters,
> >> period, otherwise one or the other is wrong. Thus far, it was Linux that
> >> produced non-working results.  
> > 
> > Again, we define minimum and maximum delays. If the right thing is to
> > not wait more than 5us and you wait up to 6, it does not mean you
> > wrote "bad timings". 4us would be a bad timing though. It depends on
> > what you are looking at.  
> 
> I am look at for example
> 
>  denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
> 
> Register was 0x143f before, now is 0x1432 , which is less.
> I guess that would be the "bad timing" then ?

Well, is it a minimum or a maximum ? How do you know U-Boot value is
straight on the edge? If you want to know if timings are valid, open
the part datasheet, do the math with a paper and compare. This is the
scientific way to declare timings valid or invalid.

> >>> Run a stress test, if it passes, you should be good :)    
> >>
> >> Thank you for the hint, I think the stress test thus far could be
> >> considered sufficient. I guess we can agree on that ?  
> > 
> > Oh yeah absolutely :)  

Just to be sure, we are talking about the new timings derived with
Masahiro's patch in Linux here, right?

Because "perfect timings" => "work in the oven" but let be clear on
the fact that "work in the oven" does not imply "perfect timings".


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-03-11 14:39                                             ` Miquel Raynal
@ 2020-03-14 14:48                                               ` Marek Vasut
  2020-03-17  9:27                                                 ` Masahiro Yamada
  2020-03-16  4:36                                               ` Masahiro Yamada
  1 sibling, 1 reply; 31+ messages in thread
From: Marek Vasut @ 2020-03-14 14:48 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Dinh Nguyen, Masahiro Yamada, Boris Brezillon, linux-mtd, Tim Sander

On 3/11/20 3:39 PM, Miquel Raynal wrote:
> Hi Marek,

Hello Miquel,

[...]

>>> I checked the denali driver and indeed u-boot should not be much clever
>>> than Linux. Are the differences significant? The code is so close, you
>>> can probably check why you have differences. Also verify that the same
>>> ONFI mode is used.  
>>
>> It might've made sense to check those driver differences before making
>> such an statement ;-)
>> That said, I don't think either U-Boot or Linux uses the ONFI
>> information for this NAND, but I might be wrong.
> 
> I don't know what is the exact device but most of the time, even for
> non ONFI-compliant chips, the core starts talking at the lowest ONFI
> speed (mode 0) and then negotiate with the NAND chip the actual timings
> to use. This works if get/set_features is supported, otherwise you
> might have a default mode somewhere. Is it the same in both cases? Does
> the core tries to apply the same timings? Is the calculation the same?
> 
> These are pointers but I am sure you can figure all that out.

The calculation is obviously not the same anymore, due to the recent
changes in the Linux driver, which seems to have broken it (in Linux).

>>>>> and
>>>>> may optimize better the timings depending on the selected mode ([0-5])
>>>>> (hence the different calls to ->setup_data_interface().    
>>>>
>>>> I would expect those two should produce identical timing parameters,
>>>> period, otherwise one or the other is wrong. Thus far, it was Linux that
>>>> produced non-working results.  
>>>
>>> Again, we define minimum and maximum delays. If the right thing is to
>>> not wait more than 5us and you wait up to 6, it does not mean you
>>> wrote "bad timings". 4us would be a bad timing though. It depends on
>>> what you are looking at.  
>>
>> I am look at for example
>>
>>  denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
>>
>> Register was 0x143f before, now is 0x1432 , which is less.
>> I guess that would be the "bad timing" then ?
> 
> Well, is it a minimum or a maximum ? How do you know U-Boot value is
> straight on the edge? If you want to know if timings are valid, open
> the part datasheet, do the math with a paper and compare. This is the
> scientific way to declare timings valid or invalid.

If the value were straight at the edge, I would expect this would
trigger errors over the years, when those values were used, or maybe it
would trigger an error in the thermal chamber tests ? If neither of that
happens, then the values are probably not at the edge enough to matter.

That said, timing calculations do not factor in only the datasheet
values, but also signal propagation delays and other details of the data
path on the PCB and in the SOC, so it's not as simple as you claim it
is. Moreover, while the NAND datasheet is available in public, the
Denali IP datasheet is not, so "do the math with a paper and compare" is
inapplicable here either way, sorry.

>>>>> Run a stress test, if it passes, you should be good :)    
>>>>
>>>> Thank you for the hint, I think the stress test thus far could be
>>>> considered sufficient. I guess we can agree on that ?  
>>>
>>> Oh yeah absolutely :)  
> 
> Just to be sure, we are talking about the new timings derived with
> Masahiro's patch in Linux here, right?

The timings which went through extensive testing are the original ones.

The ones coming out of Masahiro's patch at least do not trigger those
massive UBI errors, however they were tested only very lightly. And I
feel like adding +1/-1 somewhere into the patch is rather iffy, so I
would hope the Denali datasheet has something about this and why this is
needed.

> Because "perfect timings" => "work in the oven" but let be clear on
> the fact that "work in the oven" does not imply "perfect timings".

Let's be clear that I still prefer "practically working and possibly
imperfect" over "theoretically perfect and practically not working".

Also, correction, thermal chamber is not an owen, it does testing over
the entire temperature range of the device.

-- 
Best regards,
Marek Vasut

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-03-11 14:39                                             ` Miquel Raynal
  2020-03-14 14:48                                               ` Marek Vasut
@ 2020-03-16  4:36                                               ` Masahiro Yamada
  1 sibling, 0 replies; 31+ messages in thread
From: Masahiro Yamada @ 2020-03-16  4:36 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Marek Vasut, Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander

Hi.


On Wed, Mar 11, 2020 at 11:39 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Hi Marek,
>
> Marek Vasut <marex@denx.de> wrote on Wed, 11 Mar 2020 15:07:27 +0100:
>
> > On 3/11/20 2:33 PM, Miquel Raynal wrote:
> > > Hi Marek,
> >
> > Hi,
> >
> > [...]
> >
> > >>>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> > >>>>> index b0482108a127..ea38aa42873e 100644
> > >>>>> --- a/drivers/mtd/nand/raw/denali.c
> > >>>>> +++ b/drivers/mtd/nand/raw/denali.c
> > >>>>> @@ -860,9 +860,9 @@ static int denali_setup_data_interface(struct
> > >>>>> nand_chip *chip, int chipnr,
> > >>>>>
> > >>>>>         /*
> > >>>>>          * Determine the minimum of acc_clks to meet the data setup timing.
> > >>>>> -        * (one additional clock cycle just in case)
> > >>>>> +        * (two additional clock cycles just in case)
> > >>>>>          */
> > >>>>> -       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 1;
> > >>>>> +       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 2;
> > >>>>>
> > >>>>>         /* Determine the minimum of rdwr_en_lo_cnt from RE#/WE# pulse width */
> > >>>>>         rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min), t_x);
> > >>>>
> > >>>> Like the attached one ?
> > >>>>
> > >>>> That seems to work, but -- the calculated timings differ from the ones
> > >>>> which are calculated by U-Boot and which were tested to work well.
> > >>>> That's not good, I would expect both timings to be identical:
> > >>>
> > >>> There is no such "timings tested to work well".
> > >>
> > >> Hmmm, the board went through full temperature range testing in a chamber
> > >> with those timings and passed, and there are boards with those exact
> > >> timings deployed for years now with older kernel version, which work
> > >> too. So I would expect they are good and "timings tested to work well".
> > >>
> > >>> Timings represent
> > >>> minimum and maximum values for certain operations on the NAND bus, you
> > >>> can have two different values that will both work in the same
> > >>> condition. And it is expected that Linux is more clever than U-Boot
> > >>
> > >> Errr, why ?
> > >
> > > Because sometimes people write simpler driver in U-Boot, or even
> > > hardcoded values.
> >
> > I see, this is not the case with denali nand driver though.
> >
> > > I checked the denali driver and indeed u-boot should not be much clever
> > > than Linux. Are the differences significant? The code is so close, you
> > > can probably check why you have differences. Also verify that the same
> > > ONFI mode is used.
> >
> > It might've made sense to check those driver differences before making
> > such an statement ;-)
> > That said, I don't think either U-Boot or Linux uses the ONFI
> > information for this NAND, but I might be wrong.
>
> I don't know what is the exact device but most of the time, even for
> non ONFI-compliant chips, the core starts talking at the lowest ONFI
> speed (mode 0) and then negotiate with the NAND chip the actual timings
> to use. This works if get/set_features is supported, otherwise you
> might have a default mode somewhere. Is it the same in both cases? Does
> the core tries to apply the same timings? Is the calculation the same?
>
> These are pointers but I am sure you can figure all that out.
>
> > >>> and
> > >>> may optimize better the timings depending on the selected mode ([0-5])
> > >>> (hence the different calls to ->setup_data_interface().
> > >>
> > >> I would expect those two should produce identical timing parameters,
> > >> period, otherwise one or the other is wrong. Thus far, it was Linux that
> > >> produced non-working results.
> > >
> > > Again, we define minimum and maximum delays. If the right thing is to
> > > not wait more than 5us and you wait up to 6, it does not mean you
> > > wrote "bad timings". 4us would be a bad timing though. It depends on
> > > what you are looking at.
> >
> > I am look at for example
> >
> >  denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
> >
> > Register was 0x143f before, now is 0x1432 , which is less.
> > I guess that would be the "bad timing" then ?
>
> Well, is it a minimum or a maximum ? How do you know U-Boot value is
> straight on the edge? If you want to know if timings are valid, open
> the part datasheet, do the math with a paper and compare. This is the
> scientific way to declare timings valid or invalid.



In order not to waste time,
I'd like to agree with the Linux/U-Boot difference.


The timing requirement for a NAND chip provided by
drivers/mtd/nand/raw/nand_timing.c
are represented in pico-seconds
(same between in Linux and U-Boot in this regard)


The registers in the Denali IP specifies
"how many clock cycles should be inserted
between rising/falling edges of signals".

So, the register values depend on both
[1] the timing mode of the chip
   and
[2] clock frequencies at which the Denali IP is running


To calculate the clock cycles, denali_setup_data_interface()
needs two clocks (controller core clock, and the NAND bus clock).
But, we should not hard-code the clock frequency in the denali driver.
(The denali driver hard-coded the clock frequencies for the backward
compatibility reason, but our ultimate goal is to move the clock
info to the clock driver.)

In other words, we rely on the clock driver being correct.


denali_setup_data_interface() does the equivalent computation
between Linux and U-Boot.

It is obvious by diffing denali_setup_data_interface().

https://github.com/torvalds/linux/blob/v5.6-rc5/drivers/mtd/nand/raw/denali.c#L764
https://github.com/u-boot/u-boot/blob/v2020.04-rc3/drivers/mtd/nand/raw/denali.c#L917



The difference is the clock frequencies (and he knows it).


In this patch, Marek claims  denali->clk_rate = 31.25 MHz
denali->clk_x_rate = 125 MHz on Linux running on his board.


I am not sure whether commit 3b5015c4d834 (i.e. Linux 5.3)
is related to this or not, but the clock frequencies were
changed after Linux 4.19.


Marek also said that the SOCFPGA clock driver is not ready for U-Boot
as he posted this to U-Boot ML:
http://patchwork.ozlabs.org/patch/1220287/

Then, the clock frequencies fall back to the hard-coded default:
denali->clk_rate = 50 MHz  denali->clk_x_rate = 200 MHz
https://github.com/u-boot/u-boot/blob/v2020.04-rc3/drivers/mtd/nand/raw/denali_dt.c#L141


From this point, it is obvious that U-Boot values are not straight on the edge.
(U-Boot divides the timing values represented in pico-seconds
with the too small clock period.)


Thanks.



> > >>> Run a stress test, if it passes, you should be good :)
> > >>
> > >> Thank you for the hint, I think the stress test thus far could be
> > >> considered sufficient. I guess we can agree on that ?
> > >
> > > Oh yeah absolutely :)
>
> Just to be sure, we are talking about the new timings derived with
> Masahiro's patch in Linux here, right?
>
> Because "perfect timings" => "work in the oven" but let be clear on
> the fact that "work in the oven" does not imply "perfect timings".
>
>
> Thanks,
> Miquèl
--
Best Regards
Masahiro Yamada

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
  2020-03-14 14:48                                               ` Marek Vasut
@ 2020-03-17  9:27                                                 ` Masahiro Yamada
  0 siblings, 0 replies; 31+ messages in thread
From: Masahiro Yamada @ 2020-03-17  9:27 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Dinh Nguyen, Boris Brezillon, linux-mtd, Tim Sander, Miquel Raynal

Hi,


On Sat, Mar 14, 2020 at 11:49 PM Marek Vasut <marex@denx.de> wrote:
>
> On 3/11/20 3:39 PM, Miquel Raynal wrote:
> > Hi Marek,
>
> Hello Miquel,
>
> [...]
>
> >>> I checked the denali driver and indeed u-boot should not be much clever
> >>> than Linux. Are the differences significant? The code is so close, you
> >>> can probably check why you have differences. Also verify that the same
> >>> ONFI mode is used.
> >>
> >> It might've made sense to check those driver differences before making
> >> such an statement ;-)
> >> That said, I don't think either U-Boot or Linux uses the ONFI
> >> information for this NAND, but I might be wrong.
> >
> > I don't know what is the exact device but most of the time, even for
> > non ONFI-compliant chips, the core starts talking at the lowest ONFI
> > speed (mode 0) and then negotiate with the NAND chip the actual timings
> > to use. This works if get/set_features is supported, otherwise you
> > might have a default mode somewhere. Is it the same in both cases? Does
> > the core tries to apply the same timings? Is the calculation the same?
> >
> > These are pointers but I am sure you can figure all that out.
>
> The calculation is obviously not the same anymore, due to the recent
> changes in the Linux driver, which seems to have broken it (in Linux).
>
> >>>>> and
> >>>>> may optimize better the timings depending on the selected mode ([0-5])
> >>>>> (hence the different calls to ->setup_data_interface().
> >>>>
> >>>> I would expect those two should produce identical timing parameters,
> >>>> period, otherwise one or the other is wrong. Thus far, it was Linux that
> >>>> produced non-working results.
> >>>
> >>> Again, we define minimum and maximum delays. If the right thing is to
> >>> not wait more than 5us and you wait up to 6, it does not mean you
> >>> wrote "bad timings". 4us would be a bad timing though. It depends on
> >>> what you are looking at.
> >>
> >> I am look at for example
> >>
> >>  denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
> >>
> >> Register was 0x143f before, now is 0x1432 , which is less.
> >> I guess that would be the "bad timing" then ?
> >
> > Well, is it a minimum or a maximum ? How do you know U-Boot value is
> > straight on the edge? If you want to know if timings are valid, open
> > the part datasheet, do the math with a paper and compare. This is the
> > scientific way to declare timings valid or invalid.
>
> If the value were straight at the edge, I would expect this would
> trigger errors over the years, when those values were used, or maybe it
> would trigger an error in the thermal chamber tests ? If neither of that
> happens, then the values are probably not at the edge enough to matter.


This is a trade-off between the performance and the safety.

If you want to be very safe, you can use
the power-on-reset defaults.
The default register values are chosen to insert long enough
wait time to work with any devices.
Of course, it is slow, though.

Generally speaking, the datasheet spec contains the deviation,
so I think aligning with nand_timings.c will guarantee the
device will work over years.

So, in my understanding, ->setup_data_interface() hook
can simply encode struct nand_sdr_timings into the
controller registers.

One question I am still wondering is why this problem was
triggered only after Linux 4.19

As you said, the clock manager on SOCFPGA can change the
clock frequencies that drive the NAND controller.

In your setting,
denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz

But, maybe
denali->clk_rate = 50 MHz and denali->clk_x_rate = 200 MHz
on somebody else's. This exactly matches to the frequencies
the denali drivers originally used.
So, the register values are on the edge in this case.
But, nobody reported the problem before v4.19.





> That said, timing calculations do not factor in only the datasheet
> values, but also signal propagation delays and other details of the data
> path on the PCB and in the SOC, so it's not as simple as you claim it
> is.


I think you are right.
Most of parameters can be derived from the ONFI datasheet,
but the acc_clks register must contain the propagation delays.

This is why I was asking you to test the iffy patches
that increment the acc_clks by 1 or 2.



> Moreover, while the NAND datasheet is available in public, the
> Denali IP datasheet is not, so "do the math with a paper and compare" is
> inapplicable here either way, sorry.
>
> >>>>> Run a stress test, if it passes, you should be good :)
> >>>>
> >>>> Thank you for the hint, I think the stress test thus far could be
> >>>> considered sufficient. I guess we can agree on that ?
> >>>
> >>> Oh yeah absolutely :)
> >
> > Just to be sure, we are talking about the new timings derived with
> > Masahiro's patch in Linux here, right?
>
> The timings which went through extensive testing are the original ones.
>
> The ones coming out of Masahiro's patch at least do not trigger those
> massive UBI errors, however they were tested only very lightly. And I
> feel like adding +1/-1 somewhere into the patch is rather iffy, so I
> would hope the Denali datasheet has something about this and why this is
> needed.


This is iffy, but I have no idea to parameterize
the SoC/board-dependent delay.

In the posted patch, I used 'data_setup_on_host'
to avoid the magic number.




>
> > Because "perfect timings" => "work in the oven" but let be clear on
> > the fact that "work in the oven" does not imply "perfect timings".
>
> Let's be clear that I still prefer "practically working and possibly
> imperfect" over "theoretically perfect and practically not working".
>
> Also, correction, thermal chamber is not an owen, it does testing over
> the entire temperature range of the device.
>
> --
> Best regards,
> Marek Vasut



-- 
Best Regards
Masahiro Yamada

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, back to index

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-05  7:08 [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again" Marek Vasut
2020-02-05  9:12 ` Miquel Raynal
2020-02-05  9:41   ` Marek Vasut
2020-02-05  9:50     ` Miquel Raynal
2020-02-05 10:05       ` Boris Brezillon
2020-02-05 10:08       ` Marek Vasut
2020-02-11 10:04         ` Marek Vasut
2020-02-11 16:07           ` Miquel Raynal
2020-02-11 20:35             ` Marek Vasut
2020-02-12  9:00               ` Masahiro Yamada
2020-02-12  9:37                 ` Marek Vasut
2020-02-12 16:56                   ` Masahiro Yamada
2020-02-12 17:13                     ` Masahiro Yamada
2020-02-12 17:44                     ` Marek Vasut
2020-02-17  8:33                       ` Masahiro Yamada
2020-02-18  5:55                         ` Masahiro Yamada
2020-02-19 18:42                           ` Marek Vasut
2020-02-25  0:41                             ` Masahiro Yamada
2020-03-03 17:11                               ` Marek Vasut
2020-03-09 10:27                                 ` Masahiro Yamada
2020-03-11 12:52                                   ` Marek Vasut
2020-03-11 13:08                                     ` Miquel Raynal
2020-03-11 13:19                                       ` Marek Vasut
2020-03-11 13:33                                         ` Miquel Raynal
2020-03-11 14:07                                           ` Marek Vasut
2020-03-11 14:39                                             ` Miquel Raynal
2020-03-14 14:48                                               ` Marek Vasut
2020-03-17  9:27                                                 ` Masahiro Yamada
2020-03-16  4:36                                               ` Masahiro Yamada
2020-02-19 18:27                         ` Marek Vasut
2020-02-25  0:38                           ` Masahiro Yamada

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git