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

[Marek Vasut]

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!

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 ?