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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Olga,

+ Michal (please add him in Cc of your next iteration)

okitain@gmail.com wrote on Tue, 30 Nov 2021 23:08:20 +0300:

> Hi Miquel,
> 
> On 30.11.2021 10:20, Miquel Raynal wrote:
> > Hi Olga,
> > 
> > okitain@gmail.com wrote on Mon, 29 Nov 2021 21:06:05 +0300:
> >   
> >> 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.  
> > 
> > I believe I made a serious mistake, re-reading the code it feels like
> > I'm changing the system's clock (which basically changes nothing in our
> > case) instead of changing the NAND bus clock.
> >   
> >> 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.
> >>  
> > 
> > Please create a two-patch series:
> > 1- Setting the right clock in the current code base (inverting bus_clk
> > and controller_clk where relevant, setting one to 100MHz and letting
> > the other as it is)
> > 2- Changing the default NV-DDR rate based on tCK (below patch).
> > 
> > Do you have the necessary hardware for testing?  
> 
> I'm sorry to say - I do not. The SoC this problem was initially noticed on can't run latest Linux,
> and even if it did I have no way of acquiring an NV-DDR-capable flash.
> 
> Since Bootlin merged in NV-DDR support into the kernel, is it possible for you to test 
> the next iteration of this patch series on NV-DDR hardware as well?
> Say, by purposefully preventing NV-DDR mode 5 from being chosen in anfc_setup_interface()?

I don't have the hardware anymore.

Please send a v2 with the necessary changes, then we will ask Naga (or
somebody else from the same team) with access to the board to test it.

> >>>> 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
> >>>     
> > 
> > 
> > Thanks,
> > Miquèl
> >   
> 
> Thanks,
> Olga.


Thanks,
Miquèl