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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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
  0 siblings, 0 replies; 20+ 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] 20+ messages in thread

end of thread, back to index

Thread overview: 20+ 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-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