| Message ID | 1441861272-2393-1-git-send-email-clsee@altera.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
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@altera.com> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com> > Cc: Dinh Nguyen <dinh.linux@gmail.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Stefan Roese <sr@denx.de> > Cc: Vikas Manocha <vikas.manocha@st.com> > Cc: Jagannadh Teki <jteki@openedev.com> > Cc: Pavel Machek <pavel@denx.de> > Acked-by: Marek Vasut <marex@denx.de> > Reviewed-by: Jagan Teki <jteki@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. > /* start with slowest clock (1 MHz) */ > cadence_spi_write_speed(bus, 1000000); > > @@ -64,7 +67,7 @@ static int spi_calibration(struct udevice *bus) > } > > /* use back the intended clock and find low range */ > - cadence_spi_write_speed(bus, plat->max_hz); > + cadence_spi_write_speed(bus, hz); > for (i = 0; i < CQSPI_READ_CAPTURE_MAX_DELAY; i++) { > /* Disable QSPI */ > cadence_qspi_apb_controller_disable(base); > @@ -111,7 +114,7 @@ static int spi_calibration(struct udevice *bus) > (range_hi + range_lo) / 2, range_lo, range_hi); > > /* just to ensure we do once only when speed or chip select change */ > - priv->qspi_calibrated_hz = plat->max_hz; > + priv->qspi_calibrated_hz = hz; > priv->qspi_calibrated_cs = spi_chip_select(bus); > > return 0; > @@ -126,14 +129,15 @@ static int cadence_spi_set_speed(struct udevice *bus, > uint hz) /* Disable QSPI */ > cadence_qspi_apb_controller_disable(priv->regbase); > > - cadence_spi_write_speed(bus, hz); > - > /* Calibration required for different SCLK speed or chip select */ > - if (priv->qspi_calibrated_hz != plat->max_hz || > + if (priv->previous_hz != hz || > priv->qspi_calibrated_cs != spi_chip_select(bus)) { This check here is what I'm referring to. > - err = spi_calibration(bus); > + err = spi_calibration(bus, hz); > if (err) > return err; > + > + /* prevent calibration run when same as previous request */ > + priv->previous_hz = hz; > } > > /* Enable QSPI */ > diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h > index 98e57aa..2912e36 100644 > --- a/drivers/spi/cadence_qspi.h > +++ b/drivers/spi/cadence_qspi.h > @@ -38,6 +38,7 @@ struct cadence_spi_priv { > int qspi_is_init; > unsigned int qspi_calibrated_hz; > unsigned int qspi_calibrated_cs; > + unsigned int previous_hz; > }; > > /* Functions call declaration */ > -- > 1.7.7.4
On Thu 2015-09-10 00:01:12, 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@altera.com> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com> > Cc: Dinh Nguyen <dinh.linux@gmail.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Stefan Roese <sr@denx.de> > Cc: Vikas Manocha <vikas.manocha@st.com> > Cc: Jagannadh Teki <jteki@openedev.com> Acked-by: Pavel Machek <pavel@denx.de> > --- 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) I'd do "unsigned int" here, for consistency with > @@ -38,6 +38,7 @@ struct cadence_spi_priv { > int qspi_is_init; > unsigned int qspi_calibrated_hz; > unsigned int qspi_calibrated_cs; > + unsigned int previous_hz; > }; this, but that's nitpicking. Thanks, Pavel
Hi, On Wed, 2015-09-23 at 11:13 +0800, Chin Liang See wrote: > > From: marex@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@altera.com> > > Cc: Dinh Nguyen <dinguyen@opensource.altera.com> > > Cc: Dinh Nguyen <dinh.linux@gmail.com> > > Cc: Marek Vasut <marex@denx.de> > > Cc: Stefan Roese <sr@denx.de> > > Cc: Vikas Manocha <vikas.manocha@st.com> > > Cc: Jagannadh Teki <jteki@openedev.com> > > Cc: Pavel Machek <pavel@denx.de> > > Acked-by: Marek Vasut <marex@denx.de> > > Reviewed-by: Jagan Teki <jteki@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. Thanks Chin Liang > > > /* start with slowest clock (1 MHz) */ > > cadence_spi_write_speed(bus, 1000000); > > > > @@ -64,7 +67,7 @@ static int spi_calibration(struct udevice *bus) > > } > > > > /* use back the intended clock and find low range */ > > - cadence_spi_write_speed(bus, plat->max_hz); > > + cadence_spi_write_speed(bus, hz); > > for (i = 0; i < CQSPI_READ_CAPTURE_MAX_DELAY; i++) { > > /* Disable QSPI */ > > cadence_qspi_apb_controller_disable(base); > > @@ -111,7 +114,7 @@ static int spi_calibration(struct udevice *bus) > > (range_hi + range_lo) / 2, range_lo, range_hi); > > > > /* just to ensure we do once only when speed or chip select change */ > > - priv->qspi_calibrated_hz = plat->max_hz; > > + priv->qspi_calibrated_hz = hz; > > priv->qspi_calibrated_cs = spi_chip_select(bus); > > > > return 0; > > @@ -126,14 +129,15 @@ static int cadence_spi_set_speed(struct udevice *bus, > > uint hz) /* Disable QSPI */ > > cadence_qspi_apb_controller_disable(priv->regbase); > > > > - cadence_spi_write_speed(bus, hz); > > - > > /* Calibration required for different SCLK speed or chip select */ > > - if (priv->qspi_calibrated_hz != plat->max_hz || > > + if (priv->previous_hz != hz || > > priv->qspi_calibrated_cs != spi_chip_select(bus)) { > > This check here is what I'm referring to. > > > - err = spi_calibration(bus); > > + err = spi_calibration(bus, hz); > > if (err) > > return err; > > + > > + /* prevent calibration run when same as previous request */ > > + priv->previous_hz = hz; > > } > > > > /* Enable QSPI */ > > diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h > > index 98e57aa..2912e36 100644 > > --- a/drivers/spi/cadence_qspi.h > > +++ b/drivers/spi/cadence_qspi.h > > @@ -38,6 +38,7 @@ struct cadence_spi_priv { > > int qspi_is_init; > > unsigned int qspi_calibrated_hz; > > unsigned int qspi_calibrated_cs; > > + unsigned int previous_hz; > > }; > > > > /* Functions call declaration */ > > -- > > 1.7.7.4
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@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@altera.com> > > > Cc: Dinh Nguyen <dinguyen@opensource.altera.com> > > > Cc: Dinh Nguyen <dinh.linux@gmail.com> > > > Cc: Marek Vasut <marex@denx.de> > > > Cc: Stefan Roese <sr@denx.de> > > > Cc: Vikas Manocha <vikas.manocha@st.com> > > > Cc: Jagannadh Teki <jteki@openedev.com> > > > Cc: Pavel Machek <pavel@denx.de> > > > Acked-by: Marek Vasut <marex@denx.de> > > > Reviewed-by: Jagan Teki <jteki@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 ?
Hi Marek, On Thu, 2015-09-24 at 09:16 +0200, marex@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@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@altera.com> > > > > Cc: Dinh Nguyen <dinguyen@opensource.altera.com> > > > > Cc: Dinh Nguyen <dinh.linux@gmail.com> > > > > Cc: Marek Vasut <marex@denx.de> > > > > Cc: Stefan Roese <sr@denx.de> > > > > Cc: Vikas Manocha <vikas.manocha@st.com> > > > > Cc: Jagannadh Teki <jteki@openedev.com> > > > > Cc: Pavel Machek <pavel@denx.de> > > > > Acked-by: Marek Vasut <marex@denx.de> > > > > Reviewed-by: Jagan Teki <jteki@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
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; + /* start with slowest clock (1 MHz) */ cadence_spi_write_speed(bus, 1000000); @@ -64,7 +67,7 @@ static int spi_calibration(struct udevice *bus) } /* use back the intended clock and find low range */ - cadence_spi_write_speed(bus, plat->max_hz); + cadence_spi_write_speed(bus, hz); for (i = 0; i < CQSPI_READ_CAPTURE_MAX_DELAY; i++) { /* Disable QSPI */ cadence_qspi_apb_controller_disable(base); @@ -111,7 +114,7 @@ static int spi_calibration(struct udevice *bus) (range_hi + range_lo) / 2, range_lo, range_hi); /* just to ensure we do once only when speed or chip select change */ - priv->qspi_calibrated_hz = plat->max_hz; + priv->qspi_calibrated_hz = hz; priv->qspi_calibrated_cs = spi_chip_select(bus); return 0; @@ -126,14 +129,15 @@ static int cadence_spi_set_speed(struct udevice *bus, uint hz) /* Disable QSPI */ cadence_qspi_apb_controller_disable(priv->regbase); - cadence_spi_write_speed(bus, hz); - /* Calibration required for different SCLK speed or chip select */ - if (priv->qspi_calibrated_hz != plat->max_hz || + if (priv->previous_hz != hz || priv->qspi_calibrated_cs != spi_chip_select(bus)) { - err = spi_calibration(bus); + err = spi_calibration(bus, hz); if (err) return err; + + /* prevent calibration run when same as previous request */ + priv->previous_hz = hz; } /* Enable QSPI */ diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h index 98e57aa..2912e36 100644 --- a/drivers/spi/cadence_qspi.h +++ b/drivers/spi/cadence_qspi.h @@ -38,6 +38,7 @@ struct cadence_spi_priv { int qspi_is_init; unsigned int qspi_calibrated_hz; unsigned int qspi_calibrated_cs; + unsigned int previous_hz; }; /* Functions call declaration */