[U-Boot] [PATCH v3 1/5] spi: cadence_qspi: Ensure spi_calibration is run when sclk change

[Chin Liang See]

Hi Marek,

On Thu, 2015-09-24 at 09:16 +0200, marex at denx.de wrote:
> On Wednesday, September 23, 2015 at 05:21:51 AM, Chin Liang See wrote:
> > Hi,
> 
> Hi!
> 
> > On Wed, 2015-09-23 at 11:13 +0800, Chin Liang See wrote:
> > > From: marex at denx.de
> > > Sent: Thursday, September 10, 2015 7:52 PM
> > > To: Chin Liang See
> > > Cc: ZY - u-boot; Dinh Nguyen; Dinh Nguyen; ZY - sr; Vikas Manocha;
> > > Jagannadh Teki; ZY - pavel Subject: Re: [PATCH v3 1/5] spi:
> > > cadence_qspi: Ensure spi_calibration is run when sclk change
> > > 
> > > On Thursday, September 10, 2015 at 07:01:12 AM, Chin Liang See wrote:
> > > > Ensuring spi_calibration is run when there is a change of sclk
> > > > frequency. This will ensure the qspi flash access works for high
> > > > sclk frequency
> > > > 
> > > > Signed-off-by: Chin Liang See <clsee at altera.com>
> > > > Cc: Dinh Nguyen <dinguyen at opensource.altera.com>
> > > > Cc: Dinh Nguyen <dinh.linux at gmail.com>
> > > > Cc: Marek Vasut <marex at denx.de>
> > > > Cc: Stefan Roese <sr at denx.de>
> > > > Cc: Vikas Manocha <vikas.manocha at st.com>
> > > > Cc: Jagannadh Teki <jteki at openedev.com>
> > > > Cc: Pavel Machek <pavel at denx.de>
> > > > Acked-by: Marek Vasut <marex at denx.de>
> > > > Reviewed-by: Jagan Teki <jteki at openedev.com>
> > > > ---
> > > > Changes for v2
> > > > - remove frequency set before calibration
> > > > - introducing previous_hz to store requested frequency
> > > > - prevent calibration run when request same frequency
> > > > ---
> > > > 
> > > >  drivers/spi/cadence_qspi.c |   20 ++++++++++++--------
> > > >  drivers/spi/cadence_qspi.h |    1 +
> > > >  2 files changed, 13 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> > > > index 34a0f46..0e7450c 100644
> > > > --- a/drivers/spi/cadence_qspi.c
> > > > +++ b/drivers/spi/cadence_qspi.c
> > > > @@ -37,15 +37,18 @@ static int cadence_spi_write_speed(struct udevice
> > > > *bus, uint hz) }
> > > > 
> > > >  /* Calibration sequence to determine the read data capture delay
> > > >  register
> > > > 
> > > > */ -static int spi_calibration(struct udevice *bus)
> > > > +static int spi_calibration(struct udevice *bus, uint hz)
> > > > 
> > > >  {
> > > > 
> > > > -	struct cadence_spi_platdata *plat = bus->platdata;
> > > > 
> > > >  	struct cadence_spi_priv *priv = dev_get_priv(bus);
> > > >  	void *base = priv->regbase;
> > > >  	u8 opcode_rdid = 0x9F;
> > > >  	unsigned int idcode = 0, temp = 0;
> > > >  	int err = 0, i, range_lo = -1, range_hi = -1;
> > > > 
> > > > +	/* if calibrated frequency same as reqeusted, skip it */
> > > > +	if (priv->qspi_calibrated_hz == hz)
> > > > +		return 0;
> > > 
> > > Do you need this check ? I think you're doing the same check in
> > > cadence_spi_set_speed() down below here, so this is redundant.
> > 
> > Oh this is checked against calibrated value. It works together with the
> > check of previous_hz. This is to cater the incident where the calibrated
> > frequency = max frequency when requested frequency exceed the max
> > frequency. Some examples as below
> > 
> > Request 100MHz, Max 50MHz,  run, calibrated 50MHz, previous 100MHz
> > Request 100MHz, Max 50MHz, skip, calibrated 50MHz, previous 100MHz
> > Request  90MHz, Max 50MHz, skip, calibrated 50MHz, previous  90MHz
> > 
> > The check here will skip calibration for third incident.
> > Hope that explains.
> 
> Ah, I see, that makes sense now, thanks!

Cool

> 
> Wouldn't it thus make more sense to place this check into the
> cadence_spi_set_speed() function instead , so that the spi_calibrate()
> function only really does calibration , no matter what ?

Sound right to me :)
Let me update this into v4.

Thanks
Chin Liang