Skip to contentSkip to conversation

spi: cadence_qspi: Ensure spi_calibration is run when sclk change

PATCH Superseded Chin Liang See 3 participants 6 messages Sep 10, 2015 – Sep 30, 2015 1 message not in the archive SPI

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Chin Liang See

author
#0 lore raw

PATCH v3 1/5 spi: cadence_qspi: Ensure spi_calibration is run when sclk change

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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vikas Manocha <[email protected]>
Cc Jagannadh Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Acked-by Marek Vasut <[email protected]>
Reviewed-by Jagan Teki <[email protected]>

---
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(-)
drivers/spi/cadence_qspi.c +12 -8
index 34a0f46..0e7450c 100644
@@ -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 */
drivers/spi/cadence_qspi.h +1 -0
index 98e57aa..2912e36 100644
@@ -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

#1 lore raw

PATCH v3 1/5 spi: cadence_qspi: Ensure spi_calibration is run when sclk change

Quoted 35 lines · Chin Liang See

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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vikas Manocha <[email protected]>
Cc Jagannadh Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Acked-by Marek Vasut <[email protected]>
Reviewed-by Jagan Teki <[email protected]>

---
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(-)
drivers/spi/cadence_qspi.c +1 -0
index 34a0f46..0e7450c 100644
@@ -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)
 {

drivers/spi/cadence_qspi.c static int cadence_spi_write_speed(struct udevice *bus, · line 51

Quoted 10 lines
drivers/spi/cadence_qspi.c +3 -1
-	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.

Quoted 23 lines
drivers/spi/cadence_qspi.c +2 -2
 	/* 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 */

drivers/spi/cadence_qspi.c static int cadence_spi_set_speed(struct udevice *bus, uint hz) · line 134

Quoted 8 lines
drivers/spi/cadence_qspi.c +1 -3
 	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.

Quoted 24 lines
drivers/spi/cadence_qspi.c +4 -1
-		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 */
drivers/spi/cadence_qspi.h +1 -1
index 98e57aa..2912e36 100644
@@ -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

A message that is not in the archive (sent off-list or not yet received): 0016CF5815A1B142902817051AF62EB305F787ED@PG-ITEXCH02.altera.priv.altera.com

author
#3 lore raw

PATCH v3 1/5 spi: cadence_qspi: Ensure spi_calibration is run when sclk change

Hi,

Quoted 50 lines · Chin Liang See

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

Quoted 42 lines · Chin Liang See

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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vikas Manocha <[email protected]>
Cc Jagannadh Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Acked-by Marek Vasut <[email protected]>
Reviewed-by Jagan Teki <[email protected]>

---
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(-)
drivers/spi/cadence_qspi.c +1 -1
index 34a0f46..0e7450c 100644
@@ -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;
Quoted 6 lines

drivers/spi/cadence_qspi.c static int cadence_spi_write_speed(struct udevice *bus, uint hz) · line 50

Quoted 3 lines
drivers/spi/cadence_qspi.c +3 -0
+	/* 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

Quoted 59 lines
Quoted 24 lines
drivers/spi/cadence_qspi.c +2 -2
 	/* 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 */

drivers/spi/cadence_qspi.c static int cadence_spi_set_speed(struct udevice *bus, uint hz) · line 134

Quoted 8 lines
drivers/spi/cadence_qspi.c +1 -3
 	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.

Quoted 24 lines
drivers/spi/cadence_qspi.c +4 -1
-		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 */
drivers/spi/cadence_qspi.h +1 -1
index 98e57aa..2912e36 100644
@@ -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

#4 lore raw

PATCH v3 1/5 spi: cadence_qspi: Ensure spi_calibration is run when sclk change

Quoted 1 line · Chin Liang See

Hi,

Hi!

Quoted 69 lines · Chin Liang See
Quoted 56 lines

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

Quoted 48 lines · Chin Liang See

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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vikas Manocha <[email protected]>
Cc Jagannadh Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Acked-by Marek Vasut <[email protected]>
Reviewed-by Jagan Teki <[email protected]>

---
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(-)
drivers/spi/cadence_qspi.c +1 -1
index 34a0f46..0e7450c 100644
@@ -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;
Quoted 6 lines

drivers/spi/cadence_qspi.c static int cadence_spi_write_speed(struct udevice *bus, uint hz) · line 50

Quoted 3 lines
drivers/spi/cadence_qspi.c +3 -0
+	/* 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

Quoted 6 lines

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 ?

author
#5 lore raw

PATCH v3 1/5 spi: cadence_qspi: Ensure spi_calibration is run when sclk change

Hi Marek,

Quoted 74 lines · marex at denx.de
Quoted 1 line · Chin Liang See

Hi,

Hi!

Quoted 69 lines · Chin Liang See
Quoted 56 lines

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

Quoted 48 lines · Chin Liang See

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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vikas Manocha <[email protected]>
Cc Jagannadh Teki <[email protected]>
Cc Pavel Machek <[email protected]>
Acked-by Marek Vasut <[email protected]>
Reviewed-by Jagan Teki <[email protected]>

---
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(-)
drivers/spi/cadence_qspi.c +1 -1
index 34a0f46..0e7450c 100644
@@ -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;
Quoted 6 lines

drivers/spi/cadence_qspi.c static int cadence_spi_write_speed(struct udevice *bus, uint hz) · line 50

Quoted 3 lines
drivers/spi/cadence_qspi.c +3 -0
+	/* 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

Quoted 8 lines
Quoted 6 lines

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

Quoted 4 lines

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

#6 lore raw

PATCH v3 1/5 spi: cadence_qspi: Ensure spi_calibration is run when sclk change

In reply to Chin Liang See
Quoted 11 lines · Chin Liang See

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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Vikas Manocha <[email protected]>
Cc Jagannadh Teki <[email protected]>
Acked-by Pavel Machek <[email protected]>

drivers/spi/cadence_qspi.c static int cadence_spi_write_speed(struct udevice *bus, uint hz) · line 40

Quoted 8 lines
drivers/spi/cadence_qspi.c +1 -1
@@ -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

drivers/spi/cadence_qspi.c struct cadence_spi_priv { · line 42

Quoted 6 lines
drivers/spi/cadence_qspi.c +1 -0
@@ -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

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html