Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
diff mbox series

Message ID 20200205070834.3087104-1-marex@denx.de
State New
Headers show
Series
  • Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
Related show

Commit Message

Marek Vasut Feb. 5, 2020, 7:08 a.m. UTC
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(-)

Comments

Miquel Raynal Feb. 5, 2020, 9:12 a.m. UTC | #1
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
Marek Vasut Feb. 5, 2020, 9:41 a.m. UTC | #2
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;

[...]
Miquel Raynal Feb. 5, 2020, 9:50 a.m. UTC | #3
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
Boris Brezillon Feb. 5, 2020, 10:05 a.m. UTC | #4
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.
Marek Vasut Feb. 5, 2020, 10:08 a.m. UTC | #5
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.
Marek Vasut Feb. 11, 2020, 10:04 a.m. UTC | #6
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.
Miquel Raynal Feb. 11, 2020, 4:07 p.m. UTC | #7
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
Marek Vasut Feb. 11, 2020, 8:35 p.m. UTC | #8
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.
Masahiro Yamada Feb. 12, 2020, 9 a.m. UTC | #9
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.
Marek Vasut Feb. 12, 2020, 9:37 a.m. UTC | #10
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.

[...]
Masahiro Yamada Feb. 12, 2020, 4:56 p.m. UTC | #11
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);
Masahiro Yamada Feb. 12, 2020, 5:13 p.m. UTC | #12
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.
Marek Vasut Feb. 12, 2020, 5:44 p.m. UTC | #13
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 ?
Masahiro Yamada Feb. 17, 2020, 8:33 a.m. UTC | #14
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().
Masahiro Yamada Feb. 18, 2020, 5:55 a.m. UTC | #15
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?
Marek Vasut Feb. 19, 2020, 6:27 p.m. UTC | #16
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.
Marek Vasut Feb. 19, 2020, 6:42 p.m. UTC | #17
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
Masahiro Yamada Feb. 25, 2020, 12:38 a.m. UTC | #18
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.
Masahiro Yamada Feb. 25, 2020, 12:41 a.m. UTC | #19
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.

Patch
diff mbox series

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;