Re: [PATCH] mtd: rawnand: arasan: Fix clock rate in NV-DDR

* Re: [PATCH] mtd: rawnand: arasan: Fix clock rate in NV-DDR
       [not found] ` <20211129095559.01aa63a6@xps13>
@ 2021-11-29 18:06   ` Olga Kitaina
  2021-11-30  7:20     ` Miquel Raynal
  0 siblings, 1 reply; 6+ messages in thread
From: Olga Kitaina @ 2021-11-29 18:06 UTC (permalink / raw)
  To: Miquel Raynal; +Cc: linux-mtd, nagasure, richard, vigneshr, linux-kernel

Hi Miquel,

On 29.11.2021 11:55, Miquel Raynal wrote:
> Hi Olga,
> 
> Please add all the MTD maintainers in copy, as requested by
> get_maintainers.pl.
> 
> okitain@gmail.com wrote on Sat, 27 Nov 2021 21:07:58
> +0300:
> 
>> According to the Arasan NAND controller spec,
>> the flash clock rate for SDR must be <= 100 MHz,
>> while for NV-DDR it must be the same as the rate
>> of the CLK line for the mode.
> 
> I completely missed that, where did you get the information?

The "Data Interface Transitions" chapter of the spec contains timings for flash clock setup in NV-DDR
and NV-DDR2 modes. The "time period" of those clocks is equal to tCK in NV-DDR and tRC in NV-DDR2.

The same chapter should have information about necessary steps to switch from NV-DDR to SDR,
which includes setting the flash clock to 100 MHz.

Just to make sure i'm not shooting myself in the foot: am I changing the right clock?
The documentation points out that we have to change flash_clk, which i thought was
nfc->controller_clk and set up by anand->clk, but it seems like it might actually be nfc->bus_clk.

In that case, does setting nfc->controller_clk to 100 MHz by default make sense?
There isn't a hard limit on what the system clock might be (beyond a specific SoC),
but there are timing requirements for the flash clock, and so setting a specific 
system clock frequency seems unnecessary for most devices.

>> The driver previously always set 100 MHz for NV-DDR, which
>> would result in incorrect behavior for NV-DDR modes 0-4.
>>
>> The appropriate clock rate can be calculated
>> from the NV-DDR timing parameters as 1/tCK, or for rates
>> measured in picoseconds, 10^12 / nand_nvddr_timings->tCK_min.
>>
> 
> You need a couple of Fixes + Cc: stable tags here, otherwise the
> patch looks good to me.
> 

Will include in the next iteration of the patch, thank you.

>> Signed-off-by: Olga Kitaina <okitain@gmail.com>
>> ---
>>  drivers/mtd/nand/raw/arasan-nand-controller.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/raw/arasan-nand-controller.c b/drivers/mtd/nand/raw/arasan-nand-controller.c
>> index 53bd10738418..ed4ee9942441 100644
>> --- a/drivers/mtd/nand/raw/arasan-nand-controller.c
>> +++ b/drivers/mtd/nand/raw/arasan-nand-controller.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/gpio/consumer.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/iopoll.h>
>> +#include <linux/math64.h>
>>  #include <linux/module.h>
>>  #include <linux/mtd/mtd.h>
>>  #include <linux/mtd/partitions.h>
>> @@ -1043,7 +1044,12 @@ static int anfc_setup_interface(struct nand_chip *chip, int target,
>>  				 DQS_BUFF_SEL_OUT(dqs_mode);
>>  	}
>>  
>> -	anand->clk = ANFC_XLNX_SDR_DFLT_CORE_CLK;
>> +	if (nand_interface_is_sdr)
>> +		anand->clk = ANFC_XLNX_SDR_DFLT_CORE_CLK;
>> +	else
>> +		/* ONFI timings are defined in picoseconds */
>> +		anand->clk = div_u64((u64)NSEC_PER_SEC * 1000,
>> +				     conf->timings.nvddr.tCK_min);
>>  
>>  	/*
>>  	 * Due to a hardware bug in the ZynqMP SoC, SDR timing modes 0-1 work
>>
>> base-commit: f53d4c109a666bf1a4883b45d546fba079258717
> 
> 
> Thanks,
> Miquèl
> 

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