SF: Cadence QSPI driver fixes and clean up

PATCH Superseded Phil Edworthy 4 participants 33 messages Nov 25, 2016 – Nov 29, 2016 SPI

lore

Share & tools

lore

Phil Edworthy

author

2016-11-25 14:38 UTC #0 lore raw

v2 cover SF: Cadence QSPI driver fixes and clean up

This series has fixes, patches to clean the code up, and add support for specifying the sampling edge.

Changed in v2:
spi: cadence_qspi: Fix baud rate calculation
spi: cadence_qspi: Fix CS timings (was "Fix CQSPI_CAL_DELAY calculation")
spi: cadence_qspi: Support specifying the sample edge used

Added in v2:
spi: cadence_qspi: Better debug information on the SPI clock rate

Phil Edworthy (8):
spi: cadence_qspi: Fix clearing of pol/pha bits
spi: cadence_qspi: Fix baud rate calculation
spi: cadence_qspi: Better debug information on the SPI clock rate
spi: cadence_qspi: Use #define for bits instead of bit shifts
spi: cadence_qspi: Clean up the #define names
spi: cadence_qspi: Remove returns from end of void functions
spi: cadence_qspi: Fix CS timings
spi: cadence_qspi: Support specifying the sample edge used

 doc/device-tree-bindings/spi/spi-cadence.txt |   2 +
 drivers/spi/cadence_qspi.c                   |  13 +-
 drivers/spi/cadence_qspi.h                   |   3 +-
 drivers/spi/cadence_qspi_apb.c               | 191 +++++++++++++--------------
 4 files changed, 104 insertions(+), 105 deletions(-)

Signature · 1 line

2.7.4

Phil Edworthy

author

2016-11-25 14:38 UTC #1 lore raw

PATCH v2 1/8 spi: cadence_qspi: Fix clearing of pol/pha bits

Or'ing together bit positions is clearly wrong.

Signed-off-by Phil Edworthy <[email protected]>

---
 drivers/spi/cadence_qspi_apb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/spi/cadence_qspi_apb.c +2 -2

index e285d3c..2403e71 100644
@@ -311,8 +311,8 @@ void cadence_qspi_apb_set_clk_mode(void *reg_base,
 
 	cadence_qspi_apb_controller_disable(reg_base);
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
-	reg &= ~(1 <<
-		(CQSPI_REG_CONFIG_CLK_POL_LSB | CQSPI_REG_CONFIG_CLK_PHA_LSB));
+	reg &= ~(1 << CQSPI_REG_CONFIG_CLK_POL_LSB);
+	reg &= ~(1 << CQSPI_REG_CONFIG_CLK_PHA_LSB);
 
 	reg |= ((clk_pol & 0x1) << CQSPI_REG_CONFIG_CLK_POL_LSB);
 	reg |= ((clk_pha & 0x1) << CQSPI_REG_CONFIG_CLK_PHA_LSB);

Signature · 1 line

2.7.4

Marek Vasut

2016-11-25 14:56 UTC #2 lore raw

PATCH v2 1/8 spi: cadence_qspi: Fix clearing of pol/pha bits

Quoted 12 lines · Phil Edworthy

Or'ing together bit positions is clearly wrong.
Signed-off-by Phil Edworthy <[email protected]>
---
 drivers/spi/cadence_qspi_apb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/spi/cadence_qspi_apb.c +0 -0
index e285d3c..2403e71 100644

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_set_clk_mode(void *reg_base, · line 315

Quoted 7 lines

drivers/spi/cadence_qspi_apb.c +2 -2

@@ -311,8 +311,8 @@ void cadence_qspi_apb_set_clk_mode(void *reg_base,
 
 	cadence_qspi_apb_controller_disable(reg_base);
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
-	reg &= ~(1 <<
-		(CQSPI_REG_CONFIG_CLK_POL_LSB | CQSPI_REG_CONFIG_CLK_PHA_LSB));
+	reg &= ~(1 << CQSPI_REG_CONFIG_CLK_POL_LSB);
+	reg &= ~(1 << CQSPI_REG_CONFIG_CLK_PHA_LSB);

Oh, nice.

Acked-by Marek Vasut <[email protected]>

Quoted 3 lines

drivers/spi/cadence_qspi_apb.c +0 -0

 	reg |= ((clk_pol & 0x1) << CQSPI_REG_CONFIG_CLK_POL_LSB);
 	reg |= ((clk_pha & 0x1) << CQSPI_REG_CONFIG_CLK_PHA_LSB);

Signature · 2 lines

Best regards,
Marek Vasut

Jagan Teki

2016-11-25 15:29 UTC #3 lore raw

PATCH v2 1/8 spi: cadence_qspi: Fix clearing of pol/pha bits

In reply to Phil Edworthy

Quoted 12 lines · Phil Edworthy

Or'ing together bit positions is clearly wrong.
Signed-off-by Phil Edworthy <[email protected]>
---
 drivers/spi/cadence_qspi_apb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/spi/cadence_qspi_apb.c +0 -0
index e285d3c..2403e71 100644

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_set_clk_mode(void *reg_base, · line 315

Quoted 7 lines

drivers/spi/cadence_qspi_apb.c +2 -2

@@ -311,8 +311,8 @@ void cadence_qspi_apb_set_clk_mode(void *reg_base,
 
        cadence_qspi_apb_controller_disable(reg_base);
        reg = readl(reg_base + CQSPI_REG_CONFIG);
-       reg &= ~(1 <<
-               (CQSPI_REG_CONFIG_CLK_POL_LSB | CQSPI_REG_CONFIG_CLK_PHA_LSB));
+       reg &= ~(1 << CQSPI_REG_CONFIG_CLK_POL_LSB);
+       reg &= ~(1 << CQSPI_REG_CONFIG_CLK_PHA_LSB);

OK, but see below

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_set_clk_mode(void *reg_base, · line 318

Quoted 3 lines

drivers/spi/cadence_qspi_apb.c +0 -0

        reg |= ((clk_pol & 0x1) << CQSPI_REG_CONFIG_CLK_POL_LSB);
        reg |= ((clk_pha & 0x1) << CQSPI_REG_CONFIG_CLK_PHA_LSB);

This existing code look not easy to understand for me with doing & and << operations with numeric 0x1. what about this

Say for example POL and PHA on cadence has with bit 1 and 2
CQSPI_REG_CONFIG_CLK_POL_LSB BIT(1)
CQSPI_REG_CONFIG_CLK_PHA_LSB BIT(2)

reg &= ~(CQSPI_REG_CONFIG_CLK_PHA_LSB | CQSPI_REG_CONFIG_CLK_POL_LSB);

if (mode & SPI_CPHA)

     reg |= CQSPI_REG_CONFIG_CLK_PHA_LSB;
if (mode & SPI_CPOL)
     reg |= CQSPI_REG_CONFIG_CLK_POL_LSB;

thanks!

Signature · 4 lines

Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

Phil Edworthy

author

2016-11-25 15:34 UTC #4 lore raw

PATCH v2 1/8 spi: cadence_qspi: Fix clearing of pol/pha bits

Hi Jagan,

Quoted 36 lines · On 25 November 2016 15:29 Jagan Teki wrote:

Quoted 12 lines · Phil Edworthy
Or'ing together bit positions is clearly wrong.
Signed-off-by Phil Edworthy <[email protected]>
---
 drivers/spi/cadence_qspi_apb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/spi/cadence_qspi_apb.c +0 -0
index e285d3c..2403e71 100644
drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_set_clk_mode(void *reg_base, · line 315
Quoted 5 lines
drivers/spi/cadence_qspi_apb.c +0 -2
@@ -311,8 +311,8 @@ void cadence_qspi_apb_set_clk_mode(void *reg_base,
 
        cadence_qspi_apb_controller_disable(reg_base);
        reg = readl(reg_base + CQSPI_REG_CONFIG);
-       reg &= ~(1 <<
-               (CQSPI_REG_CONFIG_CLK_POL_LSB |
CQSPI_REG_CONFIG_CLK_PHA_LSB));
drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_set_clk_mode(void *reg_base, · line 315
Quoted 2 lines
drivers/spi/cadence_qspi_apb.c +2 -0
+       reg &= ~(1 << CQSPI_REG_CONFIG_CLK_POL_LSB);
+       reg &= ~(1 << CQSPI_REG_CONFIG_CLK_PHA_LSB);
OK, but see below
drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_set_clk_mode(void *reg_base, · line 318
Quoted 3 lines
drivers/spi/cadence_qspi_apb.c +0 -0
        reg |= ((clk_pol & 0x1) << CQSPI_REG_CONFIG_CLK_POL_LSB);
        reg |= ((clk_pha & 0x1) << CQSPI_REG_CONFIG_CLK_PHA_LSB);
This existing code look not easy to understand for me with doing & and << operations with numeric 0x1. what about this
Say for example POL and PHA on cadence has with bit 1 and 2
CQSPI_REG_CONFIG_CLK_POL_LSB BIT(1)
CQSPI_REG_CONFIG_CLK_PHA_LSB BIT(2)

Quoted 7 lines

reg &= ~(CQSPI_REG_CONFIG_CLK_PHA_LSB |
CQSPI_REG_CONFIG_CLK_POL_LSB);
if (mode & SPI_CPHA)
     reg |= CQSPI_REG_CONFIG_CLK_PHA_LSB;
if (mode & SPI_CPOL)
     reg |= CQSPI_REG_CONFIG_CLK_POL_LSB;

I completely agree. This is addressed in patch 4 along with the other code that defines shifts.

Quoted 6 lines

thanks!
--
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

Thanks
Phil

Phil Edworthy

author

2016-11-25 14:38 UTC #5 lore raw

PATCH v2 2/8 spi: cadence_qspi: Fix baud rate calculation

In reply to Phil Edworthy

With the existing code, when the requested SPI clock rate is near to the lowest that can be achieved by the hardware (max divider of the ref clock is 32), the generated clock rate is wrong. For example, with a 50MHz ref clock, when asked for anything less than a 1.5MHz SPI clock, the code sets up the divider to generate 25MHz.

This change fixes the calculation.

Signed-off-by Phil Edworthy <[email protected]>

---
v2:
- Use the DIV_ROUND_UP macro

---
 drivers/spi/cadence_qspi_apb.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

drivers/spi/cadence_qspi_apb.c +7 ~~-16~~

index 2403e71..b9e0df7 100644
@@ -273,22 +273,13 @@ void cadence_qspi_apb_config_baudrate_div(void *reg_base,
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
 	reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK << CQSPI_REG_CONFIG_BAUD_LSB);
 
-	div = ref_clk_hz / sclk_hz;
-
-	if (div > 32)
-		div = 32;
-
-	/* Check if even number. */
-	if ((div & 1)) {
-		div = (div / 2);
-	} else {
-		if (ref_clk_hz % sclk_hz)
-			/* ensure generated SCLK doesn't exceed user
-			specified sclk_hz */
-			div = (div / 2);
-		else
-			div = (div / 2) - 1;
-	}
+	/*
+	 * The baud_div field in the config reg is 4 bits, and the ref clock is
+	 * divided by 2 * (baud_div + 1). Round up the divider to ensure the
+	 * SPI clock rate is less than or equal to the requested clock rate.
+	 */
+	div = DIV_ROUND_UP(ref_clk_hz, sclk_hz);
+	div = DIV_ROUND_UP(div, 2) - 1;
 
 	debug("%s: ref_clk %dHz sclk %dHz Div 0x%x\n", __func__,
 	      ref_clk_hz, sclk_hz, div);

Signature · 1 line

2.7.4

Marek Vasut

2016-11-25 14:57 UTC #6 lore raw

PATCH v2 2/8 spi: cadence_qspi: Fix baud rate calculation

Quoted 38 lines · Phil Edworthy

With the existing code, when the requested SPI clock rate is near to the lowest that can be achieved by the hardware (max divider of the ref clock is 32), the generated clock rate is wrong. For example, with a 50MHz ref clock, when asked for anything less than a 1.5MHz SPI clock, the code sets up the divider to generate 25MHz.
This change fixes the calculation.
Signed-off-by Phil Edworthy <[email protected]>
---
v2:
- Use the DIV_ROUND_UP macro
---
 drivers/spi/cadence_qspi_apb.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)
drivers/spi/cadence_qspi_apb.c +0 ~~-13~~
index 2403e71..b9e0df7 100644
@@ -273,22 +273,13 @@ void cadence_qspi_apb_config_baudrate_div(void *reg_base,
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
 	reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK << CQSPI_REG_CONFIG_BAUD_LSB);
 
-	div = ref_clk_hz / sclk_hz;
-
-	if (div > 32)
-		div = 32;
-
-	/* Check if even number. */
-	if ((div & 1)) {
-		div = (div / 2);
-	} else {
-		if (ref_clk_hz % sclk_hz)
-			/* ensure generated SCLK doesn't exceed user
-			specified sclk_hz */
-			div = (div / 2);

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void *reg_base, · line 282

Quoted 10 lines

drivers/spi/cadence_qspi_apb.c +7 -3

-		else
-			div = (div / 2) - 1;
-	}
+	/*
+	 * The baud_div field in the config reg is 4 bits, and the ref clock is
+	 * divided by 2 * (baud_div + 1). Round up the divider to ensure the
+	 * SPI clock rate is less than or equal to the requested clock rate.
+	 */
+	div = DIV_ROUND_UP(ref_clk_hz, sclk_hz);
+	div = DIV_ROUND_UP(div, 2) - 1;

Same as you did for u-boot, right ?

Acked-by Marek Vasut <[email protected]>

Quoted 3 lines

drivers/spi/cadence_qspi_apb.c +0 -0

 	debug("%s: ref_clk %dHz sclk %dHz Div 0x%x\n", __func__,
 	      ref_clk_hz, sclk_hz, div);

Signature · 2 lines

Best regards,
Marek Vasut

Phil Edworthy

author

2016-11-25 15:19 UTC #7 lore raw

PATCH v2 2/8 spi: cadence_qspi: Fix baud rate calculation

Hi Marek,

Quoted 45 lines · On 25 November 2016 14:58 Marek Vasut wrote:

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void
Quoted 22 lines · Phil Edworthy
With the existing code, when the requested SPI clock rate is near to the lowest that can be achieved by the hardware (max divider of the ref clock is 32), the generated clock rate is wrong. For example, with a 50MHz ref clock, when asked for anything less than a 1.5MHz SPI clock, the code sets up the divider to generate 25MHz.
This change fixes the calculation.
Signed-off-by Phil Edworthy <[email protected]>
---
v2:
- Use the DIV_ROUND_UP macro
---
 drivers/spi/cadence_qspi_apb.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)
drivers/spi/cadence_qspi_apb.c +0 -0
index 2403e71..b9e0df7 100644
@@ -273,22 +273,13 @@ void cadence_qspi_apb_config_baudrate_div(void
*reg_base,
drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void · line 274
Quoted 2 lines
drivers/spi/cadence_qspi_apb.c +0 -0
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
 	reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK <<
CQSPI_REG_CONFIG_BAUD_LSB);
Quoted 18 lines
drivers/spi/cadence_qspi_apb.c +1 ~~-16~~
-	div = ref_clk_hz / sclk_hz;
-
-	if (div > 32)
-		div = 32;
-
-	/* Check if even number. */
-	if ((div & 1)) {
-		div = (div / 2);
-	} else {
-		if (ref_clk_hz % sclk_hz)
-			/* ensure generated SCLK doesn't exceed user
-			specified sclk_hz */
-			div = (div / 2);
-		else
-			div = (div / 2) - 1;
-	}
+	/*

Quoted 8 lines

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void *reg_base, · line 282
Quoted 6 lines
drivers/spi/cadence_qspi_apb.c +6 -0
+	 * The baud_div field in the config reg is 4 bits, and the ref clock is
+	 * divided by 2 * (baud_div + 1). Round up the divider to ensure the
+	 * SPI clock rate is less than or equal to the requested clock rate.
+	 */
+	div = DIV_ROUND_UP(ref_clk_hz, sclk_hz);
+	div = DIV_ROUND_UP(div, 2) - 1;
Same as you did for u-boot, right ?

Eh? This is u-boot :)

Quoted 10 lines

Acked-by Marek Vasut <[email protected]>
drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void *reg_base,
Quoted 3 lines
drivers/spi/cadence_qspi_apb.c +0 -0
 	debug("%s: ref_clk %dHz sclk %dHz Div 0x%x\n", __func__,
 	      ref_clk_hz, sclk_hz, div);
 
--
Best regards,
Marek Vasut

Thanks
Phil

Marek Vasut

2016-11-25 15:53 UTC #8 lore raw

PATCH v2 2/8 spi: cadence_qspi: Fix baud rate calculation

Quoted 56 lines · Phil Edworthy

Hi Marek,
Quoted 45 lines · On 25 November 2016 14:58 Marek Vasut wrote:
drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void
Quoted 22 lines · Phil Edworthy
With the existing code, when the requested SPI clock rate is near to the lowest that can be achieved by the hardware (max divider of the ref clock is 32), the generated clock rate is wrong. For example, with a 50MHz ref clock, when asked for anything less than a 1.5MHz SPI clock, the code sets up the divider to generate 25MHz.
This change fixes the calculation.
Signed-off-by Phil Edworthy <[email protected]>
---
v2:
- Use the DIV_ROUND_UP macro
---
 drivers/spi/cadence_qspi_apb.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)
drivers/spi/cadence_qspi_apb.c +0 -0
index 2403e71..b9e0df7 100644
@@ -273,22 +273,13 @@ void cadence_qspi_apb_config_baudrate_div(void
*reg_base,
drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void · line 274
Quoted 2 lines
drivers/spi/cadence_qspi_apb.c +0 -0
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
 	reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK <<
CQSPI_REG_CONFIG_BAUD_LSB);
Quoted 18 lines
drivers/spi/cadence_qspi_apb.c +1 ~~-16~~
-	div = ref_clk_hz / sclk_hz;
-
-	if (div > 32)
-		div = 32;
-
-	/* Check if even number. */
-	if ((div & 1)) {
-		div = (div / 2);
-	} else {
-		if (ref_clk_hz % sclk_hz)
-			/* ensure generated SCLK doesn't exceed user
-			specified sclk_hz */
-			div = (div / 2);
-		else
-			div = (div / 2) - 1;
-	}
+	/*
Quoted 8 lines
drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void *reg_base, · line 282
Quoted 6 lines
drivers/spi/cadence_qspi_apb.c +6 -0
+	 * The baud_div field in the config reg is 4 bits, and the ref clock is
+	 * divided by 2 * (baud_div + 1). Round up the divider to ensure the
+	 * SPI clock rate is less than or equal to the requested clock rate.
+	 */
+	div = DIV_ROUND_UP(ref_clk_hz, sclk_hz);
+	div = DIV_ROUND_UP(div, 2) - 1;
Same as you did for u-boot, right ?

Quoted 1 line

Eh? This is u-boot :)

Yeah, I realized that too when I saw the later patches. I thought this was for the CQSPI in Linux at right.

Signature · 2 lines

Best regards,
Marek Vasut

Jagan Teki

2016-11-25 16:07 UTC #9 lore raw

PATCH v2 2/8 spi: cadence_qspi: Fix baud rate calculation

Quoted 57 lines · Marek Vasut

Quoted 56 lines · Phil Edworthy
Hi Marek,
Quoted 53 lines · On 25 November 2016 14:58 Marek Vasut wrote:
drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void
Quoted 22 lines · Phil Edworthy
With the existing code, when the requested SPI clock rate is near to the lowest that can be achieved by the hardware (max divider of the ref clock is 32), the generated clock rate is wrong. For example, with a 50MHz ref clock, when asked for anything less than a 1.5MHz SPI clock, the code sets up the divider to generate 25MHz.
This change fixes the calculation.
Signed-off-by Phil Edworthy <[email protected]>
---
v2:
- Use the DIV_ROUND_UP macro
---
 drivers/spi/cadence_qspi_apb.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)
drivers/spi/cadence_qspi_apb.c +0 -0
index 2403e71..b9e0df7 100644
@@ -273,22 +273,13 @@ void cadence_qspi_apb_config_baudrate_div(void
*reg_base,
drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void · line 274
Quoted 2 lines
drivers/spi/cadence_qspi_apb.c +0 -0
    reg = readl(reg_base + CQSPI_REG_CONFIG);
    reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK <<
CQSPI_REG_CONFIG_BAUD_LSB);
Quoted 14 lines
drivers/spi/cadence_qspi_apb.c +0 ~~-13~~
-   div = ref_clk_hz / sclk_hz;
-
-   if (div > 32)
-           div = 32;
-
-   /* Check if even number. */
-   if ((div & 1)) {
-           div = (div / 2);
-   } else {
-           if (ref_clk_hz % sclk_hz)
-                   /* ensure generated SCLK doesn't exceed user
-                   specified sclk_hz */
-                   div = (div / 2);
drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void *reg_base, · line 282
Quoted 10 lines
drivers/spi/cadence_qspi_apb.c +7 -3
-           else
-                   div = (div / 2) - 1;
-   }
+   /*
+    * The baud_div field in the config reg is 4 bits, and the ref clock is
+    * divided by 2 * (baud_div + 1). Round up the divider to ensure the
+    * SPI clock rate is less than or equal to the requested clock rate.
+    */
+   div = DIV_ROUND_UP(ref_clk_hz, sclk_hz);
+   div = DIV_ROUND_UP(div, 2) - 1;
Same as you did for u-boot, right ?

Quoted 4 lines

Quoted 1 line
Eh? This is u-boot :)
Yeah, I realized that too when I saw the later patches. I thought this was for the CQSPI in Linux at right.

Yes, and look good as well.

thanks!

Signature · 4 lines

Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

Jagan Teki

2016-11-25 15:41 UTC #10 lore raw

PATCH v2 2/8 spi: cadence_qspi: Fix baud rate calculation

In reply to Phil Edworthy

Quoted 38 lines · Phil Edworthy

With the existing code, when the requested SPI clock rate is near to the lowest that can be achieved by the hardware (max divider of the ref clock is 32), the generated clock rate is wrong. For example, with a 50MHz ref clock, when asked for anything less than a 1.5MHz SPI clock, the code sets up the divider to generate 25MHz.
This change fixes the calculation.
Signed-off-by Phil Edworthy <[email protected]>
---
v2:
- Use the DIV_ROUND_UP macro
---
 drivers/spi/cadence_qspi_apb.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)
drivers/spi/cadence_qspi_apb.c +0 ~~-13~~
index 2403e71..b9e0df7 100644
@@ -273,22 +273,13 @@ void cadence_qspi_apb_config_baudrate_div(void *reg_base,
        reg = readl(reg_base + CQSPI_REG_CONFIG);
        reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK << CQSPI_REG_CONFIG_BAUD_LSB);
 
-       div = ref_clk_hz / sclk_hz;
-
-       if (div > 32)
-               div = 32;
-
-       /* Check if even number. */
-       if ((div & 1)) {
-               div = (div / 2);
-       } else {
-               if (ref_clk_hz % sclk_hz)
-                       /* ensure generated SCLK doesn't exceed user
-                       specified sclk_hz */
-                       div = (div / 2);

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void *reg_base, · line 282

Quoted 10 lines

drivers/spi/cadence_qspi_apb.c +7 -3

-               else
-                       div = (div / 2) - 1;
-       }
+       /*
+        * The baud_div field in the config reg is 4 bits, and the ref clock is
+        * divided by 2 * (baud_div + 1). Round up the divider to ensure the
+        * SPI clock rate is less than or equal to the requested clock rate.
+        */
+       div = DIV_ROUND_UP(ref_clk_hz, sclk_hz);
+       div = DIV_ROUND_UP(div, 2) - 1;

What about div = (ref / (sclk * 2)) -1 that means
div = DIV_ROUND_UP(ref_clk_hz, sclk_hz * 2) -1;

thanks!

Signature · 4 lines

Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

Phil Edworthy

author

2016-11-25 16:05 UTC #11 lore raw

PATCH v2 2/8 spi: cadence_qspi: Fix baud rate calculation

Hi Jagan,

Quoted 47 lines · On 25 November 2016 15:42 Jagan Teki wrote:

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void
Quoted 22 lines · Phil Edworthy
With the existing code, when the requested SPI clock rate is near to the lowest that can be achieved by the hardware (max divider of the ref clock is 32), the generated clock rate is wrong. For example, with a 50MHz ref clock, when asked for anything less than a 1.5MHz SPI clock, the code sets up the divider to generate 25MHz.
This change fixes the calculation.
Signed-off-by Phil Edworthy <[email protected]>
---
v2:
- Use the DIV_ROUND_UP macro
---
 drivers/spi/cadence_qspi_apb.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)
drivers/spi/cadence_qspi_apb.c +0 -0
index 2403e71..b9e0df7 100644
@@ -273,22 +273,13 @@ void cadence_qspi_apb_config_baudrate_div(void
*reg_base,
drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void · line 274
Quoted 2 lines
drivers/spi/cadence_qspi_apb.c +0 -0
        reg = readl(reg_base + CQSPI_REG_CONFIG);
        reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK <<
CQSPI_REG_CONFIG_BAUD_LSB);
Quoted 19 lines
drivers/spi/cadence_qspi_apb.c +2 ~~-16~~
-       div = ref_clk_hz / sclk_hz;
-
-       if (div > 32)
-               div = 32;
-
-       /* Check if even number. */
-       if ((div & 1)) {
-               div = (div / 2);
-       } else {
-               if (ref_clk_hz % sclk_hz)
-                       /* ensure generated SCLK doesn't exceed user
-                       specified sclk_hz */
-                       div = (div / 2);
-               else
-                       div = (div / 2) - 1;
-       }
+       /*
+        * The baud_div field in the config reg is 4 bits, and the ref clock is

Quoted 8 lines

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void *reg_base, · line 282
Quoted 5 lines
drivers/spi/cadence_qspi_apb.c +5 -0
+        * divided by 2 * (baud_div + 1). Round up the divider to ensure the
+        * SPI clock rate is less than or equal to the requested clock rate.
+        */
+       div = DIV_ROUND_UP(ref_clk_hz, sclk_hz);
+       div = DIV_ROUND_UP(div, 2) - 1;
What about div = (ref / (sclk * 2)) -1 that means
div = DIV_ROUND_UP(ref_clk_hz, sclk_hz * 2) -1;

Yes, that is also the same result. I'll change the code.

Quoted 6 lines

thanks!
--
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

Thanks
Phil

Phil Edworthy

author

2016-11-25 14:38 UTC #12 lore raw

PATCH v2 3/8 spi: cadence_qspi: Better debug information on the SPI clock rate

In reply to Phil Edworthy

Show what the output clock rate actually is.

Signed-off-by Phil Edworthy <[email protected]>

---
 drivers/spi/cadence_qspi_apb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/spi/cadence_qspi_apb.c +3 -3

index b9e0df7..3ae4b5a 100644
@@ -281,13 +281,13 @@ void cadence_qspi_apb_config_baudrate_div(void *reg_base,
 	div = DIV_ROUND_UP(ref_clk_hz, sclk_hz);
 	div = DIV_ROUND_UP(div, 2) - 1;
 
-	debug("%s: ref_clk %dHz sclk %dHz Div 0x%x\n", __func__,
-	      ref_clk_hz, sclk_hz, div);
-
 	/* ensure the baud rate doesn't exceed the max value */
 	if (div > CQSPI_REG_CONFIG_BAUD_MASK)
 		div = CQSPI_REG_CONFIG_BAUD_MASK;
 
+	debug("%s: ref_clk %dHz sclk %dHz Div 0x%x, actual %dHz\n", __func__,
+	      ref_clk_hz, sclk_hz, div, ref_clk_hz / (2 * (div + 1)));
+
 	reg |= (div << CQSPI_REG_CONFIG_BAUD_LSB);
 	writel(reg, reg_base + CQSPI_REG_CONFIG);

Signature · 1 line

2.7.4

Marek Vasut

2016-11-25 14:58 UTC #13 lore raw

PATCH v2 3/8 spi: cadence_qspi: Better debug information on the SPI clock rate

Quoted 14 lines · Phil Edworthy

Show what the output clock rate actually is.

Signed-off-by Phil Edworthy <[email protected]>

---
 drivers/spi/cadence_qspi_apb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/spi/cadence_qspi_apb.c +0 -0

index b9e0df7..3ae4b5a 100644
@@ -281,13 +281,13 @@ void cadence_qspi_apb_config_baudrate_div(void *reg_base,
 	div = DIV_ROUND_UP(ref_clk_hz, sclk_hz);
 	div = DIV_ROUND_UP(div, 2) - 1;

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_config_baudrate_div(void *reg_base, · line 289

Quoted 10 lines

drivers/spi/cadence_qspi_apb.c +2 -3

 
-	debug("%s: ref_clk %dHz sclk %dHz Div 0x%x\n", __func__,
-	      ref_clk_hz, sclk_hz, div);
-
 	/* ensure the baud rate doesn't exceed the max value */
 	if (div > CQSPI_REG_CONFIG_BAUD_MASK)
 		div = CQSPI_REG_CONFIG_BAUD_MASK;
 
+	debug("%s: ref_clk %dHz sclk %dHz Div 0x%x, actual %dHz\n", __func__,
+	      ref_clk_hz, sclk_hz, div, ref_clk_hz / (2 * (div + 1)));

Fine by me:

Acked-by Marek Vasut <[email protected]>

Quoted 4 lines

drivers/spi/cadence_qspi_apb.c +0 -0

 	reg |= (div << CQSPI_REG_CONFIG_BAUD_LSB);
 	writel(reg, reg_base + CQSPI_REG_CONFIG);

Signature · 2 lines

Best regards,
Marek Vasut

Phil Edworthy

author

2016-11-25 14:38 UTC #14 lore raw

PATCH v2 4/8 spi: cadence_qspi: Use #define for bits instead of bit shifts

In reply to Phil Edworthy

Most of the code already uses #defines for the bit value, rather than the shift required to get the value. This changes the remaining code over.

Whislt at it, fix the names of the "Rd Data Capture" register defs.

Signed-off-by Phil Edworthy <[email protected]>

---
 drivers/spi/cadence_qspi_apb.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

drivers/spi/cadence_qspi_apb.c +19 ~~-18~~

index 3ae4b5a..cd46a15 100644
@@ -57,9 +57,9 @@
  * Controller's configuration and status register (offset from QSPI_BASE)
  ****************************************************************************/
 #define	CQSPI_REG_CONFIG			0x00
-#define	CQSPI_REG_CONFIG_CLK_POL_LSB		1
-#define	CQSPI_REG_CONFIG_CLK_PHA_LSB		2
 #define	CQSPI_REG_CONFIG_ENABLE_MASK		BIT(0)
+#define	CQSPI_REG_CONFIG_CLK_POL		BIT(1)
+#define	CQSPI_REG_CONFIG_CLK_PHA		BIT(2)
 #define	CQSPI_REG_CONFIG_DIRECT_MASK		BIT(7)
 #define	CQSPI_REG_CONFIG_DECODE_MASK		BIT(9)
 #define	CQSPI_REG_CONFIG_XIP_IMM_MASK		BIT(18)

@@ -94,10 +94,10 @@
 #define	CQSPI_REG_DELAY_TSD2D_MASK		0xFF
 #define	CQSPI_REG_DELAY_TSHSL_MASK		0xFF
 
-#define	CQSPI_READLCAPTURE			0x10
-#define	CQSPI_READLCAPTURE_BYPASS_LSB		0
-#define	CQSPI_READLCAPTURE_DELAY_LSB		1
-#define	CQSPI_READLCAPTURE_DELAY_MASK		0xF
+#define	CQSPI_REG_RD_DATA_CAPTURE		0x10
+#define	CQSPI_REG_RD_DATA_CAPTURE_BYPASS	BIT(0)
+#define	CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB	1
+#define	CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK	0xF
 
 #define	CQSPI_REG_SIZE				0x14
 #define	CQSPI_REG_SIZE_ADDRESS_LSB		0

@@ -244,20 +244,20 @@ void cadence_qspi_apb_readdata_capture(void *reg_base,
 	unsigned int reg;
 	cadence_qspi_apb_controller_disable(reg_base);
 
-	reg = readl(reg_base + CQSPI_READLCAPTURE);
+	reg = readl(reg_base + CQSPI_REG_RD_DATA_CAPTURE);
 
 	if (bypass)
-		reg |= (1 << CQSPI_READLCAPTURE_BYPASS_LSB);
+		reg |= CQSPI_REG_RD_DATA_CAPTURE_BYPASS;
 	else
-		reg &= ~(1 << CQSPI_READLCAPTURE_BYPASS_LSB);
+		reg &= ~CQSPI_REG_RD_DATA_CAPTURE_BYPASS;
 
-	reg &= ~(CQSPI_READLCAPTURE_DELAY_MASK
-		<< CQSPI_READLCAPTURE_DELAY_LSB);
+	reg &= ~(CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK
+		<< CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB);
 
-	reg |= ((delay & CQSPI_READLCAPTURE_DELAY_MASK)
-		<< CQSPI_READLCAPTURE_DELAY_LSB);
+	reg |= ((delay & CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK)
+		<< CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB);
 
-	writel(reg, reg_base + CQSPI_READLCAPTURE);
+	writel(reg, reg_base + CQSPI_REG_RD_DATA_CAPTURE);
 
 	cadence_qspi_apb_controller_enable(reg_base);
 	return;

@@ -302,11 +302,12 @@ void cadence_qspi_apb_set_clk_mode(void *reg_base,
 
 	cadence_qspi_apb_controller_disable(reg_base);
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
-	reg &= ~(1 << CQSPI_REG_CONFIG_CLK_POL_LSB);
-	reg &= ~(1 << CQSPI_REG_CONFIG_CLK_PHA_LSB);
+	reg &= ~(CQSPI_REG_CONFIG_CLK_POL | CQSPI_REG_CONFIG_CLK_PHA);
 
-	reg |= ((clk_pol & 0x1) << CQSPI_REG_CONFIG_CLK_POL_LSB);
-	reg |= ((clk_pha & 0x1) << CQSPI_REG_CONFIG_CLK_PHA_LSB);
+	if (clk_pol)
+		reg |= CQSPI_REG_CONFIG_CLK_POL;
+	if (clk_pha)
+		reg |= CQSPI_REG_CONFIG_CLK_PHA;
 
 	writel(reg, reg_base + CQSPI_REG_CONFIG);

Signature · 1 line

2.7.4

Marek Vasut

2016-11-25 14:59 UTC #15 lore raw

PATCH v2 4/8 spi: cadence_qspi: Use #define for bits instead of bit shifts

Quoted 55 lines · Phil Edworthy

Most of the code already uses #defines for the bit value, rather than the shift required to get the value. This changes the remaining code over.

Whislt at it, fix the names of the "Rd Data Capture" register defs.

Signed-off-by Phil Edworthy <[email protected]>

---
 drivers/spi/cadence_qspi_apb.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

drivers/spi/cadence_qspi_apb.c +9 -9

index 3ae4b5a..cd46a15 100644
@@ -57,9 +57,9 @@
  * Controller's configuration and status register (offset from QSPI_BASE)
  ****************************************************************************/
 #define	CQSPI_REG_CONFIG			0x00
-#define	CQSPI_REG_CONFIG_CLK_POL_LSB		1
-#define	CQSPI_REG_CONFIG_CLK_PHA_LSB		2
 #define	CQSPI_REG_CONFIG_ENABLE_MASK		BIT(0)
+#define	CQSPI_REG_CONFIG_CLK_POL		BIT(1)
+#define	CQSPI_REG_CONFIG_CLK_PHA		BIT(2)
 #define	CQSPI_REG_CONFIG_DIRECT_MASK		BIT(7)
 #define	CQSPI_REG_CONFIG_DECODE_MASK		BIT(9)
 #define	CQSPI_REG_CONFIG_XIP_IMM_MASK		BIT(18)

@@ -94,10 +94,10 @@
 #define	CQSPI_REG_DELAY_TSD2D_MASK		0xFF
 #define	CQSPI_REG_DELAY_TSHSL_MASK		0xFF
 
-#define	CQSPI_READLCAPTURE			0x10
-#define	CQSPI_READLCAPTURE_BYPASS_LSB		0
-#define	CQSPI_READLCAPTURE_DELAY_LSB		1
-#define	CQSPI_READLCAPTURE_DELAY_MASK		0xF
+#define	CQSPI_REG_RD_DATA_CAPTURE		0x10
+#define	CQSPI_REG_RD_DATA_CAPTURE_BYPASS	BIT(0)
+#define	CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB	1
+#define	CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK	0xF
 
 #define	CQSPI_REG_SIZE				0x14
 #define	CQSPI_REG_SIZE_ADDRESS_LSB		0

@@ -244,20 +244,20 @@ void cadence_qspi_apb_readdata_capture(void *reg_base,
 	unsigned int reg;
 	cadence_qspi_apb_controller_disable(reg_base);
 
-	reg = readl(reg_base + CQSPI_READLCAPTURE);
+	reg = readl(reg_base + CQSPI_REG_RD_DATA_CAPTURE);
 
 	if (bypass)
-		reg |= (1 << CQSPI_READLCAPTURE_BYPASS_LSB);
+		reg |= CQSPI_REG_RD_DATA_CAPTURE_BYPASS;
 	else
-		reg &= ~(1 << CQSPI_READLCAPTURE_BYPASS_LSB);
+		reg &= ~CQSPI_REG_RD_DATA_CAPTURE_BYPASS;

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_readdata_capture(void *reg_base, · line 258

Quoted 10 lines

drivers/spi/cadence_qspi_apb.c +4 -4

 
-	reg &= ~(CQSPI_READLCAPTURE_DELAY_MASK
-		<< CQSPI_READLCAPTURE_DELAY_LSB);
+	reg &= ~(CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK
+		<< CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB);
 
-	reg |= ((delay & CQSPI_READLCAPTURE_DELAY_MASK)
-		<< CQSPI_READLCAPTURE_DELAY_LSB);
+	reg |= ((delay & CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK)
+		<< CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB);

You can also drop the () around the whole expression here.

Quoted 10 lines

drivers/spi/cadence_qspi_apb.c +1 -1

-	writel(reg, reg_base + CQSPI_READLCAPTURE);
+	writel(reg, reg_base + CQSPI_REG_RD_DATA_CAPTURE);
 
 	cadence_qspi_apb_controller_enable(reg_base);
 	return;

@@ -302,11 +302,12 @@ void cadence_qspi_apb_set_clk_mode(void *reg_base,
 
 	cadence_qspi_apb_controller_disable(reg_base);
 	reg = readl(reg_base + CQSPI_REG_CONFIG);

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_set_clk_mode(void *reg_base, · line 310

Quoted 10 lines

drivers/spi/cadence_qspi_apb.c +5 -4

-	reg &= ~(1 << CQSPI_REG_CONFIG_CLK_POL_LSB);
-	reg &= ~(1 << CQSPI_REG_CONFIG_CLK_PHA_LSB);
+	reg &= ~(CQSPI_REG_CONFIG_CLK_POL | CQSPI_REG_CONFIG_CLK_PHA);
 
-	reg |= ((clk_pol & 0x1) << CQSPI_REG_CONFIG_CLK_POL_LSB);
-	reg |= ((clk_pha & 0x1) << CQSPI_REG_CONFIG_CLK_PHA_LSB);
+	if (clk_pol)
+		reg |= CQSPI_REG_CONFIG_CLK_POL;
+	if (clk_pha)
+		reg |= CQSPI_REG_CONFIG_CLK_PHA;

Except the minor nit above,

Acked-by Marek Vasut <[email protected]>

Quoted 3 lines

drivers/spi/cadence_qspi_apb.c +0 -0
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 
 

Signature · 2 lines

Best regards,
Marek Vasut

Phil Edworthy

author

2016-11-25 15:22 UTC #16 lore raw

PATCH v2 4/8 spi: cadence_qspi: Use #define for bits instead of bit shifts

Hi Marek,

Quoted 65 lines · On 25 November 2016 15:00 Marek Vasut wrote:

Quoted 16 lines · Phil Edworthy

Most of the code already uses #defines for the bit value, rather than the shift required to get the value. This changes the remaining code over.
Whislt at it, fix the names of the "Rd Data Capture" register defs.
Signed-off-by Phil Edworthy <[email protected]>
---
 drivers/spi/cadence_qspi_apb.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)
drivers/spi/cadence_qspi_apb.c +0 -0
index 3ae4b5a..cd46a15 100644

drivers/spi/cadence_qspi_apb.c line 58

Quoted 2 lines

drivers/spi/cadence_qspi_apb.c +0 -0

@@ -57,9 +57,9 @@
  * Controller's configuration and status register (offset from QSPI_BASE)

**************************************************************
**************/

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_readdata_capture(void

Quoted 25 lines

drivers/spi/cadence_qspi_apb.c +6 -6

 #define	CQSPI_REG_CONFIG			0x00
-#define	CQSPI_REG_CONFIG_CLK_POL_LSB		1
-#define	CQSPI_REG_CONFIG_CLK_PHA_LSB		2
 #define	CQSPI_REG_CONFIG_ENABLE_MASK		BIT(0)
+#define	CQSPI_REG_CONFIG_CLK_POL		BIT(1)
+#define	CQSPI_REG_CONFIG_CLK_PHA		BIT(2)
 #define	CQSPI_REG_CONFIG_DIRECT_MASK		BIT(7)
 #define	CQSPI_REG_CONFIG_DECODE_MASK		BIT(9)
 #define	CQSPI_REG_CONFIG_XIP_IMM_MASK		BIT(18)

@@ -94,10 +94,10 @@
 #define	CQSPI_REG_DELAY_TSD2D_MASK		0xFF
 #define	CQSPI_REG_DELAY_TSHSL_MASK		0xFF
 
-#define	CQSPI_READLCAPTURE			0x10
-#define	CQSPI_READLCAPTURE_BYPASS_LSB		0
-#define	CQSPI_READLCAPTURE_DELAY_LSB		1
-#define	CQSPI_READLCAPTURE_DELAY_MASK		0xF
+#define	CQSPI_REG_RD_DATA_CAPTURE		0x10
+#define	CQSPI_REG_RD_DATA_CAPTURE_BYPASS	BIT(0)
+#define	CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB	1
+#define	CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK	0xF
 
 #define	CQSPI_REG_SIZE				0x14
 #define	CQSPI_REG_SIZE_ADDRESS_LSB		0

@@ -244,20 +244,20 @@ void cadence_qspi_apb_readdata_capture(void

*reg_base,

Quoted 18 lines

drivers/spi/cadence_qspi_apb.c +5 -5

 	unsigned int reg;
 	cadence_qspi_apb_controller_disable(reg_base);
 
-	reg = readl(reg_base + CQSPI_READLCAPTURE);
+	reg = readl(reg_base + CQSPI_REG_RD_DATA_CAPTURE);
 
 	if (bypass)
-		reg |= (1 << CQSPI_READLCAPTURE_BYPASS_LSB);
+		reg |= CQSPI_REG_RD_DATA_CAPTURE_BYPASS;
 	else
-		reg &= ~(1 << CQSPI_READLCAPTURE_BYPASS_LSB);
+		reg &= ~CQSPI_REG_RD_DATA_CAPTURE_BYPASS;
 
-	reg &= ~(CQSPI_READLCAPTURE_DELAY_MASK
-		<< CQSPI_READLCAPTURE_DELAY_LSB);
+	reg &= ~(CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK
+		<< CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB);

Quoted 6 lines

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_readdata_capture(void *reg_base, · line 258
Quoted 4 lines
drivers/spi/cadence_qspi_apb.c +2 -2
-	reg |= ((delay & CQSPI_READLCAPTURE_DELAY_MASK)
-		<< CQSPI_READLCAPTURE_DELAY_LSB);
+	reg |= ((delay & CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK)
+		<< CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB);
You can also drop the () around the whole expression here.

Ok, will do.

Quoted 27 lines

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_set_clk_mode(void
Quoted 7 lines
drivers/spi/cadence_qspi_apb.c +1 -1
-	writel(reg, reg_base + CQSPI_READLCAPTURE);
+	writel(reg, reg_base + CQSPI_REG_RD_DATA_CAPTURE);
 
 	cadence_qspi_apb_controller_enable(reg_base);
 	return;
@@ -302,11 +302,12 @@ void cadence_qspi_apb_set_clk_mode(void
*reg_base,
drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_set_clk_mode(void *reg_base,
Quoted 6 lines
drivers/spi/cadence_qspi_apb.c +1 -2
 	cadence_qspi_apb_controller_disable(reg_base);
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
-	reg &= ~(1 << CQSPI_REG_CONFIG_CLK_POL_LSB);
-	reg &= ~(1 << CQSPI_REG_CONFIG_CLK_PHA_LSB);
+	reg &= ~(CQSPI_REG_CONFIG_CLK_POL |
CQSPI_REG_CONFIG_CLK_PHA);
drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_set_clk_mode(void *reg_base, · line 310
Quoted 7 lines
drivers/spi/cadence_qspi_apb.c +4 -2
-	reg |= ((clk_pol & 0x1) << CQSPI_REG_CONFIG_CLK_POL_LSB);
-	reg |= ((clk_pha & 0x1) << CQSPI_REG_CONFIG_CLK_PHA_LSB);
+	if (clk_pol)
+		reg |= CQSPI_REG_CONFIG_CLK_POL;
+	if (clk_pha)
+		reg |= CQSPI_REG_CONFIG_CLK_PHA;
Except the minor nit above,
Acked-by Marek Vasut <[email protected]>

Quoted 8 lines

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_set_clk_mode(void *reg_base,
Quoted 3 lines
drivers/spi/cadence_qspi_apb.c +0 -0
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 
 
--
Best regards,
Marek Vasut

Thanks
Phil

Jagan Teki

2016-11-25 16:06 UTC #17 lore raw

PATCH v2 4/8 spi: cadence_qspi: Use #define for bits instead of bit shifts

In reply to Phil Edworthy

Quoted 75 lines · Phil Edworthy

Most of the code already uses #defines for the bit value, rather than the shift required to get the value. This changes the remaining code over.

Whislt at it, fix the names of the "Rd Data Capture" register defs.

Signed-off-by Phil Edworthy <[email protected]>

---
 drivers/spi/cadence_qspi_apb.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

drivers/spi/cadence_qspi_apb.c +14 ~~-14~~

index 3ae4b5a..cd46a15 100644
@@ -57,9 +57,9 @@
  * Controller's configuration and status register (offset from QSPI_BASE)
  ****************************************************************************/
 #define        CQSPI_REG_CONFIG                        0x00
-#define        CQSPI_REG_CONFIG_CLK_POL_LSB            1
-#define        CQSPI_REG_CONFIG_CLK_PHA_LSB            2
 #define        CQSPI_REG_CONFIG_ENABLE_MASK            BIT(0)
+#define        CQSPI_REG_CONFIG_CLK_POL                BIT(1)
+#define        CQSPI_REG_CONFIG_CLK_PHA                BIT(2)
 #define        CQSPI_REG_CONFIG_DIRECT_MASK            BIT(7)
 #define        CQSPI_REG_CONFIG_DECODE_MASK            BIT(9)
 #define        CQSPI_REG_CONFIG_XIP_IMM_MASK           BIT(18)

@@ -94,10 +94,10 @@
 #define        CQSPI_REG_DELAY_TSD2D_MASK              0xFF
 #define        CQSPI_REG_DELAY_TSHSL_MASK              0xFF
 
-#define        CQSPI_READLCAPTURE                      0x10
-#define        CQSPI_READLCAPTURE_BYPASS_LSB           0
-#define        CQSPI_READLCAPTURE_DELAY_LSB            1
-#define        CQSPI_READLCAPTURE_DELAY_MASK           0xF
+#define        CQSPI_REG_RD_DATA_CAPTURE               0x10
+#define        CQSPI_REG_RD_DATA_CAPTURE_BYPASS        BIT(0)
+#define        CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB     1
+#define        CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK    0xF
 
 #define        CQSPI_REG_SIZE                          0x14
 #define        CQSPI_REG_SIZE_ADDRESS_LSB              0

@@ -244,20 +244,20 @@ void cadence_qspi_apb_readdata_capture(void *reg_base,
        unsigned int reg;
        cadence_qspi_apb_controller_disable(reg_base);
 
-       reg = readl(reg_base + CQSPI_READLCAPTURE);
+       reg = readl(reg_base + CQSPI_REG_RD_DATA_CAPTURE);
 
        if (bypass)
-               reg |= (1 << CQSPI_READLCAPTURE_BYPASS_LSB);
+               reg |= CQSPI_REG_RD_DATA_CAPTURE_BYPASS;
        else
-               reg &= ~(1 << CQSPI_READLCAPTURE_BYPASS_LSB);
+               reg &= ~CQSPI_REG_RD_DATA_CAPTURE_BYPASS;
 
-       reg &= ~(CQSPI_READLCAPTURE_DELAY_MASK
-               << CQSPI_READLCAPTURE_DELAY_LSB);
+       reg &= ~(CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK
+               << CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB);
 
-       reg |= ((delay & CQSPI_READLCAPTURE_DELAY_MASK)
-               << CQSPI_READLCAPTURE_DELAY_LSB);
+       reg |= ((delay & CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK)
+               << CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB);
 
-       writel(reg, reg_base + CQSPI_READLCAPTURE);
+       writel(reg, reg_base + CQSPI_REG_RD_DATA_CAPTURE);
 
        cadence_qspi_apb_controller_enable(reg_base);
        return;

@@ -302,11 +302,12 @@ void cadence_qspi_apb_set_clk_mode(void *reg_base,
 
        cadence_qspi_apb_controller_disable(reg_base);
        reg = readl(reg_base + CQSPI_REG_CONFIG);

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_set_clk_mode(void *reg_base, · line 310

Quoted 10 lines

drivers/spi/cadence_qspi_apb.c +5 -4

-       reg &= ~(1 << CQSPI_REG_CONFIG_CLK_POL_LSB);
-       reg &= ~(1 << CQSPI_REG_CONFIG_CLK_PHA_LSB);
+       reg &= ~(CQSPI_REG_CONFIG_CLK_POL | CQSPI_REG_CONFIG_CLK_PHA);
 
-       reg |= ((clk_pol & 0x1) << CQSPI_REG_CONFIG_CLK_POL_LSB);
-       reg |= ((clk_pha & 0x1) << CQSPI_REG_CONFIG_CLK_PHA_LSB);
+       if (clk_pol)
+               reg |= CQSPI_REG_CONFIG_CLK_POL;
+       if (clk_pha)
+               reg |= CQSPI_REG_CONFIG_CLK_PHA;

Take the mode directly from cadence_spi_set_mode and do mode checking here, this defiantly reduce one func arg and local variables from cadence_spi_set_mode

thanks!

Signature · 4 lines

Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

Phil Edworthy

author

2016-11-25 14:38 UTC #18 lore raw

PATCH v2 5/8 spi: cadence_qspi: Clean up the #define names

In reply to Phil Edworthy

A lot of the #defines are for single bits in a register, where the name has _MASK on the end. Since this can be used for both a mask and the value, remove _MASK from them.

Whilst doing so, also remove the unnecessary brackets around the constants.

Signed-off-by Phil Edworthy <[email protected]>

---
 drivers/spi/cadence_qspi_apb.c | 86 +++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 43 deletions(-)

drivers/spi/cadence_qspi_apb.c +43 ~~-43~~

index cd46a15..e7d8320 100644
@@ -32,37 +32,37 @@
 #include <spi.h>
 #include "cadence_qspi.h"
 
-#define CQSPI_REG_POLL_US			(1) /* 1us */
-#define CQSPI_REG_RETRY				(10000)
-#define CQSPI_POLL_IDLE_RETRY			(3)
+#define CQSPI_REG_POLL_US			1 /* 1us */
+#define CQSPI_REG_RETRY				10000
+#define CQSPI_POLL_IDLE_RETRY			3
 
-#define CQSPI_FIFO_WIDTH			(4)
+#define CQSPI_FIFO_WIDTH			4
 
-#define CQSPI_REG_SRAM_THRESHOLD_WORDS		(50)
+#define CQSPI_REG_SRAM_THRESHOLD_WORDS		50
 
 /* Transfer mode */
-#define CQSPI_INST_TYPE_SINGLE			(0)
-#define CQSPI_INST_TYPE_DUAL			(1)
-#define CQSPI_INST_TYPE_QUAD			(2)
+#define CQSPI_INST_TYPE_SINGLE			0
+#define CQSPI_INST_TYPE_DUAL			1
+#define CQSPI_INST_TYPE_QUAD			2
 
-#define CQSPI_STIG_DATA_LEN_MAX			(8)
-
-#define CQSPI_DUMMY_CLKS_PER_BYTE		(8)
-#define CQSPI_DUMMY_BYTES_MAX			(4)
+#define CQSPI_STIG_DATA_LEN_MAX			8
 
+#define CQSPI_DUMMY_CLKS_PER_BYTE		8
+#define CQSPI_DUMMY_BYTES_MAX			4
 
 #define CQSPI_REG_SRAM_FILL_THRESHOLD	\
 	((CQSPI_REG_SRAM_SIZE_WORD / 2) * CQSPI_FIFO_WIDTH)
+
 /****************************************************************************
  * Controller's configuration and status register (offset from QSPI_BASE)
  ****************************************************************************/
 #define	CQSPI_REG_CONFIG			0x00
-#define	CQSPI_REG_CONFIG_ENABLE_MASK		BIT(0)
+#define	CQSPI_REG_CONFIG_ENABLE			BIT(0)
 #define	CQSPI_REG_CONFIG_CLK_POL		BIT(1)
 #define	CQSPI_REG_CONFIG_CLK_PHA		BIT(2)
-#define	CQSPI_REG_CONFIG_DIRECT_MASK		BIT(7)
-#define	CQSPI_REG_CONFIG_DECODE_MASK		BIT(9)
-#define	CQSPI_REG_CONFIG_XIP_IMM_MASK		BIT(18)
+#define	CQSPI_REG_CONFIG_DIRECT			BIT(7)
+#define	CQSPI_REG_CONFIG_DECODE			BIT(9)
+#define	CQSPI_REG_CONFIG_XIP_IMM		BIT(18)
 #define	CQSPI_REG_CONFIG_CHIPSELECT_LSB		10
 #define	CQSPI_REG_CONFIG_BAUD_LSB		19
 #define	CQSPI_REG_CONFIG_IDLE_LSB		31

@@ -123,18 +123,18 @@
 #define	CQSPI_REG_IRQMASK			0x44
 
 #define	CQSPI_REG_INDIRECTRD			0x60
-#define	CQSPI_REG_INDIRECTRD_START_MASK		BIT(0)
-#define	CQSPI_REG_INDIRECTRD_CANCEL_MASK	BIT(1)
-#define	CQSPI_REG_INDIRECTRD_INPROGRESS_MASK	BIT(2)
-#define	CQSPI_REG_INDIRECTRD_DONE_MASK		BIT(5)
+#define	CQSPI_REG_INDIRECTRD_START		BIT(0)
+#define	CQSPI_REG_INDIRECTRD_CANCEL		BIT(1)
+#define	CQSPI_REG_INDIRECTRD_INPROGRESS		BIT(2)
+#define	CQSPI_REG_INDIRECTRD_DONE		BIT(5)
 
 #define	CQSPI_REG_INDIRECTRDWATERMARK		0x64
 #define	CQSPI_REG_INDIRECTRDSTARTADDR		0x68
 #define	CQSPI_REG_INDIRECTRDBYTES		0x6C
 
 #define	CQSPI_REG_CMDCTRL			0x90
-#define	CQSPI_REG_CMDCTRL_EXECUTE_MASK		BIT(0)
-#define	CQSPI_REG_CMDCTRL_INPROGRESS_MASK	BIT(1)
+#define	CQSPI_REG_CMDCTRL_EXECUTE		BIT(0)
+#define	CQSPI_REG_CMDCTRL_INPROGRESS		BIT(1)
 #define	CQSPI_REG_CMDCTRL_DUMMY_LSB		7
 #define	CQSPI_REG_CMDCTRL_WR_BYTES_LSB		12
 #define	CQSPI_REG_CMDCTRL_WR_EN_LSB		15

@@ -150,10 +150,10 @@
 #define	CQSPI_REG_CMDCTRL_OPCODE_MASK		0xFF
 
 #define	CQSPI_REG_INDIRECTWR			0x70
-#define	CQSPI_REG_INDIRECTWR_START_MASK		BIT(0)
-#define	CQSPI_REG_INDIRECTWR_CANCEL_MASK	BIT(1)
-#define	CQSPI_REG_INDIRECTWR_INPROGRESS_MASK	BIT(2)
-#define	CQSPI_REG_INDIRECTWR_DONE_MASK		BIT(5)
+#define	CQSPI_REG_INDIRECTWR_START		BIT(0)
+#define	CQSPI_REG_INDIRECTWR_CANCEL		BIT(1)
+#define	CQSPI_REG_INDIRECTWR_INPROGRESS		BIT(2)
+#define	CQSPI_REG_INDIRECTWR_DONE		BIT(5)
 
 #define	CQSPI_REG_INDIRECTWRWATERMARK		0x74
 #define	CQSPI_REG_INDIRECTWRSTARTADDR		0x78

@@ -197,7 +197,7 @@ void cadence_qspi_apb_controller_enable(void *reg_base)
 {
 	unsigned int reg;
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
-	reg |= CQSPI_REG_CONFIG_ENABLE_MASK;
+	reg |= CQSPI_REG_CONFIG_ENABLE;
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 	return;
 }

@@ -206,7 +206,7 @@ void cadence_qspi_apb_controller_disable(void *reg_base)
 {
 	unsigned int reg;
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
-	reg &= ~CQSPI_REG_CONFIG_ENABLE_MASK;
+	reg &= ~CQSPI_REG_CONFIG_ENABLE;
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 	return;
 }

@@ -328,9 +328,9 @@ void cadence_qspi_apb_chipselect(void *reg_base,
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
 	/* docoder */
 	if (decoder_enable) {
-		reg |= CQSPI_REG_CONFIG_DECODE_MASK;
+		reg |= CQSPI_REG_CONFIG_DECODE;
 	} else {
-		reg &= ~CQSPI_REG_CONFIG_DECODE_MASK;
+		reg &= ~CQSPI_REG_CONFIG_DECODE;
 		/* Convert CS if without decoder.
 		 * CS0 to 4b'1110
 		 * CS1 to 4b'1101

@@ -424,12 +424,12 @@ static int cadence_qspi_apb_exec_flash_cmd(void *reg_base,
 	/* Write the CMDCTRL without start execution. */
 	writel(reg, reg_base + CQSPI_REG_CMDCTRL);
 	/* Start execute */
-	reg |= CQSPI_REG_CMDCTRL_EXECUTE_MASK;
+	reg |= CQSPI_REG_CMDCTRL_EXECUTE;
 	writel(reg, reg_base + CQSPI_REG_CMDCTRL);
 
 	while (retry--) {
 		reg = readl(reg_base + CQSPI_REG_CMDCTRL);
-		if ((reg & CQSPI_REG_CMDCTRL_INPROGRESS_MASK) == 0)
+		if ((reg & CQSPI_REG_CMDCTRL_INPROGRESS) == 0)
 			break;
 		udelay(1);
 	}

@@ -647,7 +647,7 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat,
 	writel(n_rx, plat->regbase + CQSPI_REG_INDIRECTRDBYTES);
 
 	/* Start the indirect read transfer */
-	writel(CQSPI_REG_INDIRECTRD_START_MASK,
+	writel(CQSPI_REG_INDIRECTRD_START,
 	       plat->regbase + CQSPI_REG_INDIRECTRD);
 
 	while (remaining > 0) {

@@ -676,21 +676,21 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat,
 
 	/* Check indirect done status */
 	ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_INDIRECTRD,
-			   CQSPI_REG_INDIRECTRD_DONE_MASK, 1, 10, 0);
+			   CQSPI_REG_INDIRECTRD_DONE, 1, 10, 0);
 	if (ret) {
 		printf("Indirect read completion error (%i)\n", ret);
 		goto failrd;
 	}
 
 	/* Clear indirect completion status */
-	writel(CQSPI_REG_INDIRECTRD_DONE_MASK,
+	writel(CQSPI_REG_INDIRECTRD_DONE,
 	       plat->regbase + CQSPI_REG_INDIRECTRD);
 
 	return 0;
 
 failrd:
 	/* Cancel the indirect read */
-	writel(CQSPI_REG_INDIRECTRD_CANCEL_MASK,
+	writel(CQSPI_REG_INDIRECTRD_CANCEL,
 	       plat->regbase + CQSPI_REG_INDIRECTRD);
 	return ret;
 }

@@ -738,7 +738,7 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
 	writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
 
 	/* Start the indirect write transfer */
-	writel(CQSPI_REG_INDIRECTWR_START_MASK,
+	writel(CQSPI_REG_INDIRECTWR_START,
 	       plat->regbase + CQSPI_REG_INDIRECTWR);
 
 	while (remaining > 0) {

@@ -763,20 +763,20 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
 
 	/* Check indirect done status */
 	ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_INDIRECTWR,
-			   CQSPI_REG_INDIRECTWR_DONE_MASK, 1, 10, 0);
+			   CQSPI_REG_INDIRECTWR_DONE, 1, 10, 0);
 	if (ret) {
 		printf("Indirect write completion error (%i)\n", ret);
 		goto failwr;
 	}
 
 	/* Clear indirect completion status */
-	writel(CQSPI_REG_INDIRECTWR_DONE_MASK,
+	writel(CQSPI_REG_INDIRECTWR_DONE,
 	       plat->regbase + CQSPI_REG_INDIRECTWR);
 	return 0;
 
 failwr:
 	/* Cancel the indirect write */
-	writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
+	writel(CQSPI_REG_INDIRECTWR_CANCEL,
 	       plat->regbase + CQSPI_REG_INDIRECTWR);
 	return ret;
 }

@@ -787,9 +787,9 @@ void cadence_qspi_apb_enter_xip(void *reg_base, char xip_dummy)
 
 	/* enter XiP mode immediately and enable direct mode */
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
-	reg |= CQSPI_REG_CONFIG_ENABLE_MASK;
-	reg |= CQSPI_REG_CONFIG_DIRECT_MASK;
-	reg |= CQSPI_REG_CONFIG_XIP_IMM_MASK;
+	reg |= CQSPI_REG_CONFIG_ENABLE;
+	reg |= CQSPI_REG_CONFIG_DIRECT;
+	reg |= CQSPI_REG_CONFIG_XIP_IMM;
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 
 	/* keep the XiP mode */

Signature · 1 line

2.7.4

Marek Vasut

2016-11-25 15:01 UTC #19 lore raw

PATCH v2 5/8 spi: cadence_qspi: Clean up the #define names

Quoted 8 lines · Phil Edworthy

A lot of the #defines are for single bits in a register, where the name has _MASK on the end. Since this can be used for both a mask and the value, remove _MASK from them.
Whilst doing so, also remove the unnecessary brackets around the constants.
Signed-off-by Phil Edworthy <[email protected]>

I guess this is done in automated way ?

Anyway, thanks !

Acked-by Marek Vasut <[email protected]>

Quoted 228 lines

---
 drivers/spi/cadence_qspi_apb.c | 86 +++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 43 deletions(-)

drivers/spi/cadence_qspi_apb.c +43 ~~-43~~

index cd46a15..e7d8320 100644
@@ -32,37 +32,37 @@
 #include <spi.h>
 #include "cadence_qspi.h"
 
-#define CQSPI_REG_POLL_US			(1) /* 1us */
-#define CQSPI_REG_RETRY				(10000)
-#define CQSPI_POLL_IDLE_RETRY			(3)
+#define CQSPI_REG_POLL_US			1 /* 1us */
+#define CQSPI_REG_RETRY				10000
+#define CQSPI_POLL_IDLE_RETRY			3
 
-#define CQSPI_FIFO_WIDTH			(4)
+#define CQSPI_FIFO_WIDTH			4
 
-#define CQSPI_REG_SRAM_THRESHOLD_WORDS		(50)
+#define CQSPI_REG_SRAM_THRESHOLD_WORDS		50
 
 /* Transfer mode */
-#define CQSPI_INST_TYPE_SINGLE			(0)
-#define CQSPI_INST_TYPE_DUAL			(1)
-#define CQSPI_INST_TYPE_QUAD			(2)
+#define CQSPI_INST_TYPE_SINGLE			0
+#define CQSPI_INST_TYPE_DUAL			1
+#define CQSPI_INST_TYPE_QUAD			2
 
-#define CQSPI_STIG_DATA_LEN_MAX			(8)
-
-#define CQSPI_DUMMY_CLKS_PER_BYTE		(8)
-#define CQSPI_DUMMY_BYTES_MAX			(4)
+#define CQSPI_STIG_DATA_LEN_MAX			8
 
+#define CQSPI_DUMMY_CLKS_PER_BYTE		8
+#define CQSPI_DUMMY_BYTES_MAX			4
 
 #define CQSPI_REG_SRAM_FILL_THRESHOLD	\
 	((CQSPI_REG_SRAM_SIZE_WORD / 2) * CQSPI_FIFO_WIDTH)
+
 /****************************************************************************
  * Controller's configuration and status register (offset from QSPI_BASE)
  ****************************************************************************/
 #define	CQSPI_REG_CONFIG			0x00
-#define	CQSPI_REG_CONFIG_ENABLE_MASK		BIT(0)
+#define	CQSPI_REG_CONFIG_ENABLE			BIT(0)
 #define	CQSPI_REG_CONFIG_CLK_POL		BIT(1)
 #define	CQSPI_REG_CONFIG_CLK_PHA		BIT(2)
-#define	CQSPI_REG_CONFIG_DIRECT_MASK		BIT(7)
-#define	CQSPI_REG_CONFIG_DECODE_MASK		BIT(9)
-#define	CQSPI_REG_CONFIG_XIP_IMM_MASK		BIT(18)
+#define	CQSPI_REG_CONFIG_DIRECT			BIT(7)
+#define	CQSPI_REG_CONFIG_DECODE			BIT(9)
+#define	CQSPI_REG_CONFIG_XIP_IMM		BIT(18)
 #define	CQSPI_REG_CONFIG_CHIPSELECT_LSB		10
 #define	CQSPI_REG_CONFIG_BAUD_LSB		19
 #define	CQSPI_REG_CONFIG_IDLE_LSB		31

@@ -123,18 +123,18 @@
 #define	CQSPI_REG_IRQMASK			0x44
 
 #define	CQSPI_REG_INDIRECTRD			0x60
-#define	CQSPI_REG_INDIRECTRD_START_MASK		BIT(0)
-#define	CQSPI_REG_INDIRECTRD_CANCEL_MASK	BIT(1)
-#define	CQSPI_REG_INDIRECTRD_INPROGRESS_MASK	BIT(2)
-#define	CQSPI_REG_INDIRECTRD_DONE_MASK		BIT(5)
+#define	CQSPI_REG_INDIRECTRD_START		BIT(0)
+#define	CQSPI_REG_INDIRECTRD_CANCEL		BIT(1)
+#define	CQSPI_REG_INDIRECTRD_INPROGRESS		BIT(2)
+#define	CQSPI_REG_INDIRECTRD_DONE		BIT(5)
 
 #define	CQSPI_REG_INDIRECTRDWATERMARK		0x64
 #define	CQSPI_REG_INDIRECTRDSTARTADDR		0x68
 #define	CQSPI_REG_INDIRECTRDBYTES		0x6C
 
 #define	CQSPI_REG_CMDCTRL			0x90
-#define	CQSPI_REG_CMDCTRL_EXECUTE_MASK		BIT(0)
-#define	CQSPI_REG_CMDCTRL_INPROGRESS_MASK	BIT(1)
+#define	CQSPI_REG_CMDCTRL_EXECUTE		BIT(0)
+#define	CQSPI_REG_CMDCTRL_INPROGRESS		BIT(1)
 #define	CQSPI_REG_CMDCTRL_DUMMY_LSB		7
 #define	CQSPI_REG_CMDCTRL_WR_BYTES_LSB		12
 #define	CQSPI_REG_CMDCTRL_WR_EN_LSB		15

@@ -150,10 +150,10 @@
 #define	CQSPI_REG_CMDCTRL_OPCODE_MASK		0xFF
 
 #define	CQSPI_REG_INDIRECTWR			0x70
-#define	CQSPI_REG_INDIRECTWR_START_MASK		BIT(0)
-#define	CQSPI_REG_INDIRECTWR_CANCEL_MASK	BIT(1)
-#define	CQSPI_REG_INDIRECTWR_INPROGRESS_MASK	BIT(2)
-#define	CQSPI_REG_INDIRECTWR_DONE_MASK		BIT(5)
+#define	CQSPI_REG_INDIRECTWR_START		BIT(0)
+#define	CQSPI_REG_INDIRECTWR_CANCEL		BIT(1)
+#define	CQSPI_REG_INDIRECTWR_INPROGRESS		BIT(2)
+#define	CQSPI_REG_INDIRECTWR_DONE		BIT(5)
 
 #define	CQSPI_REG_INDIRECTWRWATERMARK		0x74
 #define	CQSPI_REG_INDIRECTWRSTARTADDR		0x78

@@ -197,7 +197,7 @@ void cadence_qspi_apb_controller_enable(void *reg_base)
 {
 	unsigned int reg;
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
-	reg |= CQSPI_REG_CONFIG_ENABLE_MASK;
+	reg |= CQSPI_REG_CONFIG_ENABLE;
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 	return;
 }

@@ -206,7 +206,7 @@ void cadence_qspi_apb_controller_disable(void *reg_base)
 {
 	unsigned int reg;
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
-	reg &= ~CQSPI_REG_CONFIG_ENABLE_MASK;
+	reg &= ~CQSPI_REG_CONFIG_ENABLE;
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 	return;
 }

@@ -328,9 +328,9 @@ void cadence_qspi_apb_chipselect(void *reg_base,
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
 	/* docoder */
 	if (decoder_enable) {
-		reg |= CQSPI_REG_CONFIG_DECODE_MASK;
+		reg |= CQSPI_REG_CONFIG_DECODE;
 	} else {
-		reg &= ~CQSPI_REG_CONFIG_DECODE_MASK;
+		reg &= ~CQSPI_REG_CONFIG_DECODE;
 		/* Convert CS if without decoder.
 		 * CS0 to 4b'1110
 		 * CS1 to 4b'1101

@@ -424,12 +424,12 @@ static int cadence_qspi_apb_exec_flash_cmd(void *reg_base,
 	/* Write the CMDCTRL without start execution. */
 	writel(reg, reg_base + CQSPI_REG_CMDCTRL);
 	/* Start execute */
-	reg |= CQSPI_REG_CMDCTRL_EXECUTE_MASK;
+	reg |= CQSPI_REG_CMDCTRL_EXECUTE;
 	writel(reg, reg_base + CQSPI_REG_CMDCTRL);
 
 	while (retry--) {
 		reg = readl(reg_base + CQSPI_REG_CMDCTRL);
-		if ((reg & CQSPI_REG_CMDCTRL_INPROGRESS_MASK) == 0)
+		if ((reg & CQSPI_REG_CMDCTRL_INPROGRESS) == 0)
 			break;
 		udelay(1);
 	}

@@ -647,7 +647,7 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat,
 	writel(n_rx, plat->regbase + CQSPI_REG_INDIRECTRDBYTES);
 
 	/* Start the indirect read transfer */
-	writel(CQSPI_REG_INDIRECTRD_START_MASK,
+	writel(CQSPI_REG_INDIRECTRD_START,
 	       plat->regbase + CQSPI_REG_INDIRECTRD);
 
 	while (remaining > 0) {

@@ -676,21 +676,21 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat,
 
 	/* Check indirect done status */
 	ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_INDIRECTRD,
-			   CQSPI_REG_INDIRECTRD_DONE_MASK, 1, 10, 0);
+			   CQSPI_REG_INDIRECTRD_DONE, 1, 10, 0);
 	if (ret) {
 		printf("Indirect read completion error (%i)\n", ret);
 		goto failrd;
 	}
 
 	/* Clear indirect completion status */
-	writel(CQSPI_REG_INDIRECTRD_DONE_MASK,
+	writel(CQSPI_REG_INDIRECTRD_DONE,
 	       plat->regbase + CQSPI_REG_INDIRECTRD);
 
 	return 0;
 
 failrd:
 	/* Cancel the indirect read */
-	writel(CQSPI_REG_INDIRECTRD_CANCEL_MASK,
+	writel(CQSPI_REG_INDIRECTRD_CANCEL,
 	       plat->regbase + CQSPI_REG_INDIRECTRD);
 	return ret;
 }

@@ -738,7 +738,7 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
 	writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
 
 	/* Start the indirect write transfer */
-	writel(CQSPI_REG_INDIRECTWR_START_MASK,
+	writel(CQSPI_REG_INDIRECTWR_START,
 	       plat->regbase + CQSPI_REG_INDIRECTWR);
 
 	while (remaining > 0) {

@@ -763,20 +763,20 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
 
 	/* Check indirect done status */
 	ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_INDIRECTWR,
-			   CQSPI_REG_INDIRECTWR_DONE_MASK, 1, 10, 0);
+			   CQSPI_REG_INDIRECTWR_DONE, 1, 10, 0);
 	if (ret) {
 		printf("Indirect write completion error (%i)\n", ret);
 		goto failwr;
 	}
 
 	/* Clear indirect completion status */
-	writel(CQSPI_REG_INDIRECTWR_DONE_MASK,
+	writel(CQSPI_REG_INDIRECTWR_DONE,
 	       plat->regbase + CQSPI_REG_INDIRECTWR);
 	return 0;
 
 failwr:
 	/* Cancel the indirect write */
-	writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
+	writel(CQSPI_REG_INDIRECTWR_CANCEL,
 	       plat->regbase + CQSPI_REG_INDIRECTWR);
 	return ret;
 }

@@ -787,9 +787,9 @@ void cadence_qspi_apb_enter_xip(void *reg_base, char xip_dummy)
 
 	/* enter XiP mode immediately and enable direct mode */
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
-	reg |= CQSPI_REG_CONFIG_ENABLE_MASK;
-	reg |= CQSPI_REG_CONFIG_DIRECT_MASK;
-	reg |= CQSPI_REG_CONFIG_XIP_IMM_MASK;
+	reg |= CQSPI_REG_CONFIG_ENABLE;
+	reg |= CQSPI_REG_CONFIG_DIRECT;
+	reg |= CQSPI_REG_CONFIG_XIP_IMM;
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 
 	/* keep the XiP mode */

Signature · 2 lines

Best regards,
Marek Vasut

Phil Edworthy

author

2016-11-25 14:38 UTC #20 lore raw

PATCH v2 6/8 spi: cadence_qspi: Remove returns from end of void functions

In reply to Phil Edworthy

Signed-off-by Phil Edworthy <[email protected]>

---
 drivers/spi/cadence_qspi_apb.c | 8 --------
 1 file changed, 8 deletions(-)

drivers/spi/cadence_qspi_apb.c +0 -8

index e7d8320..1cd636a 100644
@@ -199,7 +199,6 @@ void cadence_qspi_apb_controller_enable(void *reg_base)
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
 	reg |= CQSPI_REG_CONFIG_ENABLE;
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
-	return;
 }
 
 void cadence_qspi_apb_controller_disable(void *reg_base)

@@ -208,7 +207,6 @@ void cadence_qspi_apb_controller_disable(void *reg_base)
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
 	reg &= ~CQSPI_REG_CONFIG_ENABLE;
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
-	return;
 }
 
 /* Return 1 if idle, otherwise return 0 (busy). */

@@ -260,7 +258,6 @@ void cadence_qspi_apb_readdata_capture(void *reg_base,
 	writel(reg, reg_base + CQSPI_REG_RD_DATA_CAPTURE);
 
 	cadence_qspi_apb_controller_enable(reg_base);
-	return;
 }
 
 void cadence_qspi_apb_config_baudrate_div(void *reg_base,

@@ -292,7 +289,6 @@ void cadence_qspi_apb_config_baudrate_div(void *reg_base,
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 
 	cadence_qspi_apb_controller_enable(reg_base);
-	return;
 }
 
 void cadence_qspi_apb_set_clk_mode(void *reg_base,

@@ -312,7 +308,6 @@ void cadence_qspi_apb_set_clk_mode(void *reg_base,
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 
 	cadence_qspi_apb_controller_enable(reg_base);
-	return;
 }
 
 void cadence_qspi_apb_chipselect(void *reg_base,

@@ -347,7 +342,6 @@ void cadence_qspi_apb_chipselect(void *reg_base,
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 
 	cadence_qspi_apb_controller_enable(reg_base);
-	return;
 }
 
 void cadence_qspi_apb_delay(void *reg_base,

@@ -385,7 +379,6 @@ void cadence_qspi_apb_delay(void *reg_base,
 	writel(reg, reg_base + CQSPI_REG_DELAY);
 
 	cadence_qspi_apb_controller_enable(reg_base);
-	return;
 }
 
 void cadence_qspi_apb_controller_init(struct cadence_spi_platdata *plat)

@@ -413,7 +406,6 @@ void cadence_qspi_apb_controller_init(struct cadence_spi_platdata *plat)
 	writel(0, plat->regbase + CQSPI_REG_IRQMASK);
 
 	cadence_qspi_apb_controller_enable(plat->regbase);
-	return;
 }
 
 static int cadence_qspi_apb_exec_flash_cmd(void *reg_base,

Signature · 1 line

2.7.4

Marek Vasut

2016-11-25 15:01 UTC #21 lore raw

PATCH v2 6/8 spi: cadence_qspi: Remove returns from end of void functions

Quoted 1 line · Phil Edworthy

Signed-off-by Phil Edworthy <[email protected]>

Nice find :)

Acked-by Marek Vasut <[email protected]>

Quoted 73 lines

---
 drivers/spi/cadence_qspi_apb.c | 8 --------
 1 file changed, 8 deletions(-)

drivers/spi/cadence_qspi_apb.c +0 -8

index e7d8320..1cd636a 100644
@@ -199,7 +199,6 @@ void cadence_qspi_apb_controller_enable(void *reg_base)
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
 	reg |= CQSPI_REG_CONFIG_ENABLE;
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
-	return;
 }
 
 void cadence_qspi_apb_controller_disable(void *reg_base)

@@ -208,7 +207,6 @@ void cadence_qspi_apb_controller_disable(void *reg_base)
 	reg = readl(reg_base + CQSPI_REG_CONFIG);
 	reg &= ~CQSPI_REG_CONFIG_ENABLE;
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
-	return;
 }
 
 /* Return 1 if idle, otherwise return 0 (busy). */

@@ -260,7 +258,6 @@ void cadence_qspi_apb_readdata_capture(void *reg_base,
 	writel(reg, reg_base + CQSPI_REG_RD_DATA_CAPTURE);
 
 	cadence_qspi_apb_controller_enable(reg_base);
-	return;
 }
 
 void cadence_qspi_apb_config_baudrate_div(void *reg_base,

@@ -292,7 +289,6 @@ void cadence_qspi_apb_config_baudrate_div(void *reg_base,
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 
 	cadence_qspi_apb_controller_enable(reg_base);
-	return;
 }
 
 void cadence_qspi_apb_set_clk_mode(void *reg_base,

@@ -312,7 +308,6 @@ void cadence_qspi_apb_set_clk_mode(void *reg_base,
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 
 	cadence_qspi_apb_controller_enable(reg_base);
-	return;
 }
 
 void cadence_qspi_apb_chipselect(void *reg_base,

@@ -347,7 +342,6 @@ void cadence_qspi_apb_chipselect(void *reg_base,
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 
 	cadence_qspi_apb_controller_enable(reg_base);
-	return;
 }
 
 void cadence_qspi_apb_delay(void *reg_base,

@@ -385,7 +379,6 @@ void cadence_qspi_apb_delay(void *reg_base,
 	writel(reg, reg_base + CQSPI_REG_DELAY);
 
 	cadence_qspi_apb_controller_enable(reg_base);
-	return;
 }
 
 void cadence_qspi_apb_controller_init(struct cadence_spi_platdata *plat)

@@ -413,7 +406,6 @@ void cadence_qspi_apb_controller_init(struct cadence_spi_platdata *plat)
 	writel(0, plat->regbase + CQSPI_REG_IRQMASK);
 
 	cadence_qspi_apb_controller_enable(plat->regbase);
-	return;
 }
 
 static int cadence_qspi_apb_exec_flash_cmd(void *reg_base,

Signature · 2 lines

Best regards,
Marek Vasut

Phil Edworthy

author

2016-11-25 14:38 UTC #22 lore raw

PATCH v2 7/8 spi: cadence_qspi: Fix CS timings

In reply to Phil Edworthy

The Cadence QSPI controller has specified overheads for the various CS times that are in addition to those programmed in to the Device Delay register. The overheads are different for the delays.

In addition, the existing code does not handle the case when the delay is less than a SCLK period.

This change accurately calculates the additional delays in Ref clocks.

Signed-off-by Phil Edworthy <[email protected]>

---
v2:
Was "spi: cadence_qspi: Fix CQSPI_CAL_DELAY calculation" Note only did the existing code not cope with the delay less than an SCLK period, but the calculation didn't round properly, and didn't take into account the delays that the QSPI Controller adds to those programmed into the Device Delay reg.

---
 drivers/spi/cadence_qspi_apb.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

drivers/spi/cadence_qspi_apb.c +12 ~~-11~~

index 1cd636a..56ad952 100644
@@ -169,9 +169,6 @@
 	((readl(base + CQSPI_REG_CONFIG) >>		\
 		CQSPI_REG_CONFIG_IDLE_LSB) & 0x1)
 
-#define CQSPI_CAL_DELAY(tdelay_ns, tref_ns, tsclk_ns)		\
-	((((tdelay_ns) - (tsclk_ns)) / (tref_ns)))
-
 #define CQSPI_GET_RD_SRAM_LEVEL(reg_base)			\
 	(((readl(reg_base + CQSPI_REG_SDRAMLEVEL)) >>	\
 	CQSPI_REG_SDRAMLEVEL_RD_LSB) & CQSPI_REG_SDRAMLEVEL_RD_MASK)

@@ -357,16 +354,20 @@ void cadence_qspi_apb_delay(void *reg_base,
 	cadence_qspi_apb_controller_disable(reg_base);
 
 	/* Convert to ns. */
-	ref_clk_ns = (1000000000) / ref_clk;
+	ref_clk_ns = DIV_ROUND_UP(1000000000, ref_clk);
 
 	/* Convert to ns. */
-	sclk_ns = (1000000000) / sclk_hz;
-
-	/* Plus 1 to round up 1 clock cycle. */
-	tshsl = CQSPI_CAL_DELAY(tshsl_ns, ref_clk_ns, sclk_ns) + 1;
-	tchsh = CQSPI_CAL_DELAY(tchsh_ns, ref_clk_ns, sclk_ns) + 1;
-	tslch = CQSPI_CAL_DELAY(tslch_ns, ref_clk_ns, sclk_ns) + 1;
-	tsd2d = CQSPI_CAL_DELAY(tsd2d_ns, ref_clk_ns, sclk_ns) + 1;
+	sclk_ns = DIV_ROUND_UP(1000000000, sclk_hz);
+
+	/* The controller adds additional delay to that programmed in the reg */
+	if (tshsl_ns >= sclk_ns + ref_clk_ns)
+		tshsl_ns -= sclk_ns + ref_clk_ns;
+	if (tchsh_ns >= sclk_ns + 3 * ref_clk_ns)
+		tchsh_ns -= sclk_ns + 3 * ref_clk_ns;
+	tshsl = DIV_ROUND_UP(tshsl_ns, ref_clk_ns);
+	tchsh = DIV_ROUND_UP(tchsh_ns, ref_clk_ns);
+	tslch = DIV_ROUND_UP(tslch_ns, ref_clk_ns);
+	tsd2d = DIV_ROUND_UP(tsd2d_ns, ref_clk_ns);
 
 	reg = ((tshsl & CQSPI_REG_DELAY_TSHSL_MASK)
 			<< CQSPI_REG_DELAY_TSHSL_LSB);

Signature · 1 line

2.7.4

Marek Vasut

2016-11-25 15:04 UTC #23 lore raw

PATCH v2 7/8 spi: cadence_qspi: Fix CS timings

Quoted 10 lines · Phil Edworthy

The Cadence QSPI controller has specified overheads for the various CS times that are in addition to those programmed in to the Device Delay register. The overheads are different for the delays.
In addition, the existing code does not handle the case when the delay is less than a SCLK period.
This change accurately calculates the additional delays in Ref clocks.
Signed-off-by Phil Edworthy <[email protected]>

Looks fine to me, but Dinh/Chin, can you check whether this doesn't break SoCFPGA ?

Quoted 55 lines

---
 drivers/spi/cadence_qspi_apb.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

drivers/spi/cadence_qspi_apb.c +12 ~~-11~~

index 1cd636a..56ad952 100644
@@ -169,9 +169,6 @@
 	((readl(base + CQSPI_REG_CONFIG) >>		\
 		CQSPI_REG_CONFIG_IDLE_LSB) & 0x1)
 
-#define CQSPI_CAL_DELAY(tdelay_ns, tref_ns, tsclk_ns)		\
-	((((tdelay_ns) - (tsclk_ns)) / (tref_ns)))
-
 #define CQSPI_GET_RD_SRAM_LEVEL(reg_base)			\
 	(((readl(reg_base + CQSPI_REG_SDRAMLEVEL)) >>	\
 	CQSPI_REG_SDRAMLEVEL_RD_LSB) & CQSPI_REG_SDRAMLEVEL_RD_MASK)

@@ -357,16 +354,20 @@ void cadence_qspi_apb_delay(void *reg_base,
 	cadence_qspi_apb_controller_disable(reg_base);
 
 	/* Convert to ns. */
-	ref_clk_ns = (1000000000) / ref_clk;
+	ref_clk_ns = DIV_ROUND_UP(1000000000, ref_clk);
 
 	/* Convert to ns. */
-	sclk_ns = (1000000000) / sclk_hz;
-
-	/* Plus 1 to round up 1 clock cycle. */
-	tshsl = CQSPI_CAL_DELAY(tshsl_ns, ref_clk_ns, sclk_ns) + 1;
-	tchsh = CQSPI_CAL_DELAY(tchsh_ns, ref_clk_ns, sclk_ns) + 1;
-	tslch = CQSPI_CAL_DELAY(tslch_ns, ref_clk_ns, sclk_ns) + 1;
-	tsd2d = CQSPI_CAL_DELAY(tsd2d_ns, ref_clk_ns, sclk_ns) + 1;
+	sclk_ns = DIV_ROUND_UP(1000000000, sclk_hz);
+
+	/* The controller adds additional delay to that programmed in the reg */
+	if (tshsl_ns >= sclk_ns + ref_clk_ns)
+		tshsl_ns -= sclk_ns + ref_clk_ns;
+	if (tchsh_ns >= sclk_ns + 3 * ref_clk_ns)
+		tchsh_ns -= sclk_ns + 3 * ref_clk_ns;
+	tshsl = DIV_ROUND_UP(tshsl_ns, ref_clk_ns);
+	tchsh = DIV_ROUND_UP(tchsh_ns, ref_clk_ns);
+	tslch = DIV_ROUND_UP(tslch_ns, ref_clk_ns);
+	tsd2d = DIV_ROUND_UP(tsd2d_ns, ref_clk_ns);
 
 	reg = ((tshsl & CQSPI_REG_DELAY_TSHSL_MASK)
 			<< CQSPI_REG_DELAY_TSHSL_LSB);

Signature · 2 lines

Best regards,
Marek Vasut

See

2016-11-28 12:48 UTC #24 lore raw

PATCH v2 7/8 spi: cadence_qspi: Fix CS timings

In reply to Phil Edworthy

Hi Phil,

Quoted 58 lines · Phil Edworthy

The Cadence QSPI controller has specified overheads for the various CS
times that are in addition to those programmed in to the Device Delay register. The overheads are different for the delays.
In addition, the existing code does not handle the case when the delay
is less than a SCLK period.
This change accurately calculates the additional delays in Ref clocks.
Signed-off-by Phil Edworthy <[email protected]>
---
v2:
?Was "spi: cadence_qspi: Fix CQSPI_CAL_DELAY calculation" ?Note only did the existing code not cope with the delay less than ?an SCLK period, but the calculation didn't round properly, and ?didn't take into account the delays that the QSPI Controller adds ?to those programmed into the Device Delay reg.
---
?drivers/spi/cadence_qspi_apb.c | 23 ++++++++++++----------- ?1 file changed, 12 insertions(+), 11 deletions(-)
drivers/spi/cadence_qspi_apb.c +0 -3
index 1cd636a..56ad952 100644
@@ -169,9 +169,6 @@
 ????????((readl(base + CQSPI_REG_CONFIG) >>?????????????\
 ????????????????CQSPI_REG_CONFIG_IDLE_LSB) & 0x1)
 
-#define CQSPI_CAL_DELAY(tdelay_ns, tref_ns, tsclk_ns)??????????\
-???????((((tdelay_ns) - (tsclk_ns)) / (tref_ns)))
-
 ?#define CQSPI_GET_RD_SRAM_LEVEL(reg_base)??????????????????????\
 ????????(((readl(reg_base + CQSPI_REG_SDRAMLEVEL)) >>???\
 ????????CQSPI_REG_SDRAMLEVEL_RD_LSB) & CQSPI_REG_SDRAMLEVEL_RD_MASK)
@@ -357,16 +354,20 @@ void cadence_qspi_apb_delay(void *reg_base,
 ????????cadence_qspi_apb_controller_disable(reg_base);
 
????????/* Convert to ns. */ -???????ref_clk_ns = (1000000000) / ref_clk; +???????ref_clk_ns = DIV_ROUND_UP(1000000000, ref_clk);
????????/* Convert to ns. */
-???????sclk_ns = (1000000000) / sclk_hz;
drivers/spi/cadence_qspi_apb.c +3 -6
-
-???????/* Plus 1 to round up 1 clock cycle. */
-???????tshsl = CQSPI_CAL_DELAY(tshsl_ns, ref_clk_ns, sclk_ns) + 1;
-???????tchsh = CQSPI_CAL_DELAY(tchsh_ns, ref_clk_ns, sclk_ns) + 1;
-???????tslch = CQSPI_CAL_DELAY(tslch_ns, ref_clk_ns, sclk_ns) + 1;
-???????tsd2d = CQSPI_CAL_DELAY(tsd2d_ns, ref_clk_ns, sclk_ns) + 1;
+???????sclk_ns = DIV_ROUND_UP(1000000000, sclk_hz);
+
+???????/* The controller adds additional delay to that programmed in

Quoted 5 lines

the reg */
+???????if (tshsl_ns >= sclk_ns + ref_clk_ns) +???????????????tshsl_ns -= sclk_ns + ref_clk_ns; +???????if (tchsh_ns >= sclk_ns + 3 * ref_clk_ns) +???????????????tchsh_ns -= sclk_ns + 3 * ref_clk_ns;

Any reason we need this?
The DIV_ROUND_UP or previous + 1 in algo will ensure its more than a SCLK period.

Thanks
Chin Liang

Quoted 22 lines

+???????tshsl = DIV_ROUND_UP(tshsl_ns, ref_clk_ns); +???????tchsh = DIV_ROUND_UP(tchsh_ns, ref_clk_ns); +???????tslch = DIV_ROUND_UP(tslch_ns, ref_clk_ns); +???????tsd2d = DIV_ROUND_UP(tsd2d_ns, ref_clk_ns);
????????reg = ((tshsl & CQSPI_REG_DELAY_TSHSL_MASK) ????????????????????????<< CQSPI_REG_DELAY_TSHSL_LSB); --
2.7.4
________________________________
Confidentiality Notice.
This message may contain information that is confidential or otherwise protected from disclosure. If you are not the intended recipient, you are hereby notified that any use, disclosure, dissemination, distribution, or copying of this message, or any attachments, is strictly prohibited. If you have received this message in error, please advise the sender by reply e-mail, and delete the message and any attachments. Thank you.

Phil Edworthy

author

2016-11-29 10:13 UTC #25 lore raw

PATCH v2 7/8 spi: cadence_qspi: Fix CS timings

Hi Chin Liang,

Quoted 60 lines · Chin Liang

Quoted 39 lines · Phil Edworthy
The Cadence QSPI controller has specified overheads for the various CS
times that are in addition to those programmed in to the Device Delay register. The overheads are different for the delays.
In addition, the existing code does not handle the case when the delay
is less than a SCLK period.
This change accurately calculates the additional delays in Ref clocks.
Signed-off-by Phil Edworthy <[email protected]>
---
v2:
?Was "spi: cadence_qspi: Fix CQSPI_CAL_DELAY calculation" ?Note only did the existing code not cope with the delay less than ?an SCLK period, but the calculation didn't round properly, and ?didn't take into account the delays that the QSPI Controller adds ?to those programmed into the Device Delay reg.
---
?drivers/spi/cadence_qspi_apb.c | 23 ++++++++++++----------- ?1 file changed, 12 insertions(+), 11 deletions(-)
drivers/spi/cadence_qspi_apb.c +0 -3
index 1cd636a..56ad952 100644
@@ -169,9 +169,6 @@
 ????????((readl(base + CQSPI_REG_CONFIG) >>?????????????\
 ????????????????CQSPI_REG_CONFIG_IDLE_LSB) & 0x1)
 
-#define CQSPI_CAL_DELAY(tdelay_ns, tref_ns, tsclk_ns)??????????\
-???????((((tdelay_ns) - (tsclk_ns)) / (tref_ns)))
-
 ?#define CQSPI_GET_RD_SRAM_LEVEL(reg_base)??????????????????????\
 ????????(((readl(reg_base + CQSPI_REG_SDRAMLEVEL)) >>???\
Quoted 1 line
????????CQSPI_REG_SDRAMLEVEL_RD_LSB) &
CQSPI_REG_SDRAMLEVEL_RD_MASK)
Quoted 18 lines
drivers/spi/cadence_qspi_apb.c +0 -0
@@ -357,16 +354,20 @@ void cadence_qspi_apb_delay(void *reg_base,
 ????????cadence_qspi_apb_controller_disable(reg_base);
 
????????/* Convert to ns. */ -???????ref_clk_ns = (1000000000) / ref_clk; +???????ref_clk_ns = DIV_ROUND_UP(1000000000, ref_clk);
????????/* Convert to ns. */
-???????sclk_ns = (1000000000) / sclk_hz;
drivers/spi/cadence_qspi_apb.c +3 -6
-
-???????/* Plus 1 to round up 1 clock cycle. */
-???????tshsl = CQSPI_CAL_DELAY(tshsl_ns, ref_clk_ns, sclk_ns) + 1;
-???????tchsh = CQSPI_CAL_DELAY(tchsh_ns, ref_clk_ns, sclk_ns) + 1;
-???????tslch = CQSPI_CAL_DELAY(tslch_ns, ref_clk_ns, sclk_ns) + 1;
-???????tsd2d = CQSPI_CAL_DELAY(tsd2d_ns, ref_clk_ns, sclk_ns) + 1;
+???????sclk_ns = DIV_ROUND_UP(1000000000, sclk_hz);
+
+???????/* The controller adds additional delay to that programmed in

Quoted 8 lines

Quoted 5 lines
the reg */
+???????if (tshsl_ns >= sclk_ns + ref_clk_ns) +???????????????tshsl_ns -= sclk_ns + ref_clk_ns; +???????if (tchsh_ns >= sclk_ns + 3 * ref_clk_ns) +???????????????tchsh_ns -= sclk_ns + 3 * ref_clk_ns;
Any reason we need this?
The DIV_ROUND_UP or previous + 1 in algo will ensure its more than a SCLK period.

In general, all of these CS timing should be optimal. I measured differences in throughput with the sub-optimal setting. Admittedly, the difference is small but still we should set it correctly.

Quoted 14 lines

Thanks
Chin Liang
Quoted 11 lines
+???????tshsl = DIV_ROUND_UP(tshsl_ns, ref_clk_ns); +???????tchsh = DIV_ROUND_UP(tchsh_ns, ref_clk_ns); +???????tslch = DIV_ROUND_UP(tslch_ns, ref_clk_ns); +???????tsd2d = DIV_ROUND_UP(tsd2d_ns, ref_clk_ns);
????????reg = ((tshsl & CQSPI_REG_DELAY_TSHSL_MASK) ????????????????????????<< CQSPI_REG_DELAY_TSHSL_LSB); --
2.7.4

Thanks
Phil

Phil Edworthy

author

2016-11-25 14:38 UTC #26 lore raw

PATCH v2 8/8 spi: cadence_qspi: Support specifying the sample edge used

In reply to Phil Edworthy

Whilst at it, move the code to read the "sram-size" property into the other code that reads properties from the node, rather than the SF subnode.

Also change the code to use a bool for the bypass arg.

Signed-off-by Phil Edworthy <[email protected]>

---
v2:
Change name of DT prop and provide details of what it does. Whilst at it, move the code to read the "sram-size" property into the other code that reads properties from the node, rather than the SF subnode.

Also change the code to use a bool for the bypass arg.

---
 doc/device-tree-bindings/spi/spi-cadence.txt |  2 ++
 drivers/spi/cadence_qspi.c                   | 13 +++++++++----
 drivers/spi/cadence_qspi.h                   |  3 ++-
 drivers/spi/cadence_qspi_apb.c               |  8 +++++++-
 4 files changed, 20 insertions(+), 6 deletions(-)

doc/device-tree-bindings/spi/spi-cadence.txt +2 -0

index c1e2233..94c800b 100644
@@ -26,3 +26,5 @@ connected flash properties
 			  select (n_ss_out).
 - tslch-ns		: Delay in master reference clocks between setting
 			  n_ss_out low and first bit transfer
+- sample-edge-rising	: Data outputs from flash memory will be sampled on the
+			  rising edge. Default is falling edge.

drivers/spi/cadence_qspi.c +9 -4

index 1051afb..16fb18e 100644
@@ -39,8 +39,10 @@ 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, uint hz)
 {
+	struct cadence_spi_platdata *plat = bus->platdata;
 	struct cadence_spi_priv *priv = dev_get_priv(bus);
 	void *base = priv->regbase;
+	bool edge = plat->sample_edge_rising;
 	u8 opcode_rdid = 0x9F;
 	unsigned int idcode = 0, temp = 0;
 	int err = 0, i, range_lo = -1, range_hi = -1;

@@ -49,7 +51,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
 	cadence_spi_write_speed(bus, 1000000);
 
 	/* configure the read data capture delay register to 0 */
-	cadence_qspi_apb_readdata_capture(base, 1, 0);
+	cadence_qspi_apb_readdata_capture(base, true, edge, 0);
 
 	/* Enable QSPI */
 	cadence_qspi_apb_controller_enable(base);

@@ -69,7 +71,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
 		cadence_qspi_apb_controller_disable(base);
 
 		/* reconfigure the read data capture delay register */
-		cadence_qspi_apb_readdata_capture(base, 1, i);
+		cadence_qspi_apb_readdata_capture(base, true, edge, i);
 
 		/* Enable back QSPI */
 		cadence_qspi_apb_controller_enable(base);

@@ -105,7 +107,8 @@ static int spi_calibration(struct udevice *bus, uint hz)
 	cadence_qspi_apb_controller_disable(base);
 
 	/* configure the final value for read data capture delay register */
-	cadence_qspi_apb_readdata_capture(base, 1, (range_hi + range_lo) / 2);
+	cadence_qspi_apb_readdata_capture(base, true, edge,
+		(range_hi + range_lo) / 2);
 	debug("SF: Read data capture delay calibrated to %i (%i - %i)\n",
 	      (range_hi + range_lo) / 2, range_lo, range_hi);

@@ -298,6 +301,7 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
 
 	plat->regbase = (void *)data[0];
 	plat->ahbbase = (void *)data[2];
+	plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
 
 	/* All other paramters are embedded in the child node */
 	subnode = fdt_first_subnode(blob, node);

@@ -317,7 +321,8 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
 	plat->tsd2d_ns = fdtdec_get_int(blob, subnode, "tsd2d-ns", 255);
 	plat->tchsh_ns = fdtdec_get_int(blob, subnode, "tchsh-ns", 20);
 	plat->tslch_ns = fdtdec_get_int(blob, subnode, "tslch-ns", 20);
-	plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
+	plat->sample_edge_rising = fdtdec_get_bool(blob, subnode,
+		"sample-edge-rising");
 
 	debug("%s: regbase=%p ahbbase=%p max-frequency=%d page-size=%d\n",
 	      __func__, plat->regbase, plat->ahbbase, plat->max_hz,

drivers/spi/cadence_qspi.h +2 -1

index a849f7b..ad1dbae 100644
@@ -26,6 +26,7 @@ struct cadence_spi_platdata {
 	u32		tchsh_ns;
 	u32		tslch_ns;
 	u32		sram_size;
+	bool		sample_edge_rising;
 };
 
 struct cadence_spi_priv {

@@ -73,6 +74,6 @@ void cadence_qspi_apb_delay(void *reg_base,
 	unsigned int tchsh_ns, unsigned int tslch_ns);
 void cadence_qspi_apb_enter_xip(void *reg_base, char xip_dummy);
 void cadence_qspi_apb_readdata_capture(void *reg_base,
-	unsigned int bypass, unsigned int delay);
+	bool bypass, bool edge, unsigned int delay);
 
 #endif /* __CADENCE_QSPI_H__ */

drivers/spi/cadence_qspi_apb.c +7 -1

index 56ad952..e43973c 100644
@@ -98,6 +98,7 @@
 #define	CQSPI_REG_RD_DATA_CAPTURE_BYPASS	BIT(0)
 #define	CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB	1
 #define	CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK	0xF
+#define	CQSPI_REG_RD_DATA_CAPTURE_EDGE		BIT(5)
 
 #define	CQSPI_REG_SIZE				0x14
 #define	CQSPI_REG_SIZE_ADDRESS_LSB		0

@@ -234,7 +235,7 @@ static unsigned int cadence_qspi_wait_idle(void *reg_base)
 }
 
 void cadence_qspi_apb_readdata_capture(void *reg_base,
-				unsigned int bypass, unsigned int delay)
+	bool bypass, bool edge, unsigned int delay)
 {
 	unsigned int reg;
 	cadence_qspi_apb_controller_disable(reg_base);

@@ -246,6 +247,11 @@ void cadence_qspi_apb_readdata_capture(void *reg_base,
 	else
 		reg &= ~CQSPI_REG_RD_DATA_CAPTURE_BYPASS;
 
+	if (edge)
+		reg |= CQSPI_REG_RD_DATA_CAPTURE_EDGE;
+	else
+		reg &= ~CQSPI_REG_RD_DATA_CAPTURE_EDGE;
+
 	reg &= ~(CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK
 		<< CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB);

Signature · 1 line

2.7.4

Marek Vasut

2016-11-25 15:05 UTC #27 lore raw

PATCH v2 8/8 spi: cadence_qspi: Support specifying the sample edge used

Quoted 3 lines · Phil Edworthy

Whilst at it, move the code to read the "sram-size" property into the other code that reads properties from the node, rather than the SF subnode.

The commit message does not match the subject, but I think this needs splitting into two patches.

Quoted 144 lines

Also change the code to use a bool for the bypass arg.

Signed-off-by Phil Edworthy <[email protected]>

Also change the code to use a bool for the bypass arg.

---
 doc/device-tree-bindings/spi/spi-cadence.txt |  2 ++
 drivers/spi/cadence_qspi.c                   | 13 +++++++++----
 drivers/spi/cadence_qspi.h                   |  3 ++-
 drivers/spi/cadence_qspi_apb.c               |  8 +++++++-
 4 files changed, 20 insertions(+), 6 deletions(-)

doc/device-tree-bindings/spi/spi-cadence.txt +2 -0

index c1e2233..94c800b 100644
@@ -26,3 +26,5 @@ connected flash properties
 			  select (n_ss_out).
 - tslch-ns		: Delay in master reference clocks between setting
 			  n_ss_out low and first bit transfer
+- sample-edge-rising	: Data outputs from flash memory will be sampled on the
+			  rising edge. Default is falling edge.

drivers/spi/cadence_qspi.c +9 -4

index 1051afb..16fb18e 100644
@@ -39,8 +39,10 @@ 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, uint hz)
 {
+	struct cadence_spi_platdata *plat = bus->platdata;
 	struct cadence_spi_priv *priv = dev_get_priv(bus);
 	void *base = priv->regbase;
+	bool edge = plat->sample_edge_rising;
 	u8 opcode_rdid = 0x9F;
 	unsigned int idcode = 0, temp = 0;
 	int err = 0, i, range_lo = -1, range_hi = -1;

@@ -49,7 +51,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
 	cadence_spi_write_speed(bus, 1000000);
 
 	/* configure the read data capture delay register to 0 */
-	cadence_qspi_apb_readdata_capture(base, 1, 0);
+	cadence_qspi_apb_readdata_capture(base, true, edge, 0);
 
 	/* Enable QSPI */
 	cadence_qspi_apb_controller_enable(base);

@@ -69,7 +71,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
 		cadence_qspi_apb_controller_disable(base);
 
 		/* reconfigure the read data capture delay register */
-		cadence_qspi_apb_readdata_capture(base, 1, i);
+		cadence_qspi_apb_readdata_capture(base, true, edge, i);
 
 		/* Enable back QSPI */
 		cadence_qspi_apb_controller_enable(base);

@@ -105,7 +107,8 @@ static int spi_calibration(struct udevice *bus, uint hz)
 	cadence_qspi_apb_controller_disable(base);
 
 	/* configure the final value for read data capture delay register */
-	cadence_qspi_apb_readdata_capture(base, 1, (range_hi + range_lo) / 2);
+	cadence_qspi_apb_readdata_capture(base, true, edge,
+		(range_hi + range_lo) / 2);
 	debug("SF: Read data capture delay calibrated to %i (%i - %i)\n",
 	      (range_hi + range_lo) / 2, range_lo, range_hi);

@@ -298,6 +301,7 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
 
 	plat->regbase = (void *)data[0];
 	plat->ahbbase = (void *)data[2];
+	plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
 
 	/* All other paramters are embedded in the child node */
 	subnode = fdt_first_subnode(blob, node);

@@ -317,7 +321,8 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
 	plat->tsd2d_ns = fdtdec_get_int(blob, subnode, "tsd2d-ns", 255);
 	plat->tchsh_ns = fdtdec_get_int(blob, subnode, "tchsh-ns", 20);
 	plat->tslch_ns = fdtdec_get_int(blob, subnode, "tslch-ns", 20);
-	plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
+	plat->sample_edge_rising = fdtdec_get_bool(blob, subnode,
+		"sample-edge-rising");
 
 	debug("%s: regbase=%p ahbbase=%p max-frequency=%d page-size=%d\n",
 	      __func__, plat->regbase, plat->ahbbase, plat->max_hz,

drivers/spi/cadence_qspi.h +2 -1

index a849f7b..ad1dbae 100644
@@ -26,6 +26,7 @@ struct cadence_spi_platdata {
 	u32		tchsh_ns;
 	u32		tslch_ns;
 	u32		sram_size;
+	bool		sample_edge_rising;
 };
 
 struct cadence_spi_priv {

@@ -73,6 +74,6 @@ void cadence_qspi_apb_delay(void *reg_base,
 	unsigned int tchsh_ns, unsigned int tslch_ns);
 void cadence_qspi_apb_enter_xip(void *reg_base, char xip_dummy);
 void cadence_qspi_apb_readdata_capture(void *reg_base,
-	unsigned int bypass, unsigned int delay);
+	bool bypass, bool edge, unsigned int delay);
 
 #endif /* __CADENCE_QSPI_H__ */

drivers/spi/cadence_qspi_apb.c +7 -1

index 56ad952..e43973c 100644
@@ -98,6 +98,7 @@
 #define	CQSPI_REG_RD_DATA_CAPTURE_BYPASS	BIT(0)
 #define	CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB	1
 #define	CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK	0xF
+#define	CQSPI_REG_RD_DATA_CAPTURE_EDGE		BIT(5)
 
 #define	CQSPI_REG_SIZE				0x14
 #define	CQSPI_REG_SIZE_ADDRESS_LSB		0

@@ -234,7 +235,7 @@ static unsigned int cadence_qspi_wait_idle(void *reg_base)
 }
 
 void cadence_qspi_apb_readdata_capture(void *reg_base,
-				unsigned int bypass, unsigned int delay)
+	bool bypass, bool edge, unsigned int delay)
 {
 	unsigned int reg;
 	cadence_qspi_apb_controller_disable(reg_base);

@@ -246,6 +247,11 @@ void cadence_qspi_apb_readdata_capture(void *reg_base,
 	else
 		reg &= ~CQSPI_REG_RD_DATA_CAPTURE_BYPASS;
 
+	if (edge)
+		reg |= CQSPI_REG_RD_DATA_CAPTURE_EDGE;
+	else
+		reg &= ~CQSPI_REG_RD_DATA_CAPTURE_EDGE;
+
 	reg &= ~(CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK
 		<< CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB);

Signature · 2 lines

Best regards,
Marek Vasut

Phil Edworthy

author

2016-11-25 15:24 UTC #28 lore raw

PATCH v2 8/8 spi: cadence_qspi: Support specifying the sample edge used

Hi Marek,

Quoted 7 lines · On 25 November 2016 15:06 Marek Vasut wrote:

Quoted 3 lines · Phil Edworthy
Whilst at it, move the code to read the "sram-size" property into the other code that reads properties from the node, rather than the SF subnode.
The commit message does not match the subject, but I think this needs splitting into two patches.

Ah, right the subject was covering it.
I'll split the patch in two.

Quoted 143 lines

doc/device-tree-

Quoted 20 lines

Also change the code to use a bool for the bypass arg.
Signed-off-by Phil Edworthy <[email protected]>
---
v2:
Change name of DT prop and provide details of what it does. Whilst at it, move the code to read the "sram-size" property into the other code that reads properties from the node, rather than the SF subnode.
Also change the code to use a bool for the bypass arg.
---
 doc/device-tree-bindings/spi/spi-cadence.txt |  2 ++
 drivers/spi/cadence_qspi.c                   | 13 +++++++++----
 drivers/spi/cadence_qspi.h                   |  3 ++-
 drivers/spi/cadence_qspi_apb.c               |  8 +++++++-
 4 files changed, 20 insertions(+), 6 deletions(-)
doc/device-tree- renamed from doc/device-tree-bindings/spi/spi-cadence.txt +0 -0

bindings/spi/spi-cadence.txt

doc/device-tree-bindings/spi/spi-cadence.txt connected flash properties · line 29

Quoted 8 lines

index c1e2233..94c800b 100644

doc/device-tree-bindings/spi/spi-cadence.txt +1 -0

@@ -26,3 +26,5 @@ connected flash properties
 			  select (n_ss_out).
 - tslch-ns		: Delay in master reference clocks between setting
 			  n_ss_out low and first bit transfer
+- sample-edge-rising	: Data outputs from flash memory will be sampled

on the

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

Quoted 6 lines

doc/device-tree-bindings/spi/spi-cadence.txt +1 -0
+			  rising edge. Default is falling edge.
drivers/spi/cadence_qspi.c +0 -0
index 1051afb..16fb18e 100644
@@ -39,8 +39,10 @@ static int cadence_spi_write_speed(struct udevice *bus,

uint hz)

drivers/spi/cadence_qspi.c static int cadence_spi_ofdata_to_platdata(struct

Quoted 39 lines

drivers/spi/cadence_qspi.c +6 -3

 /* Calibration sequence to determine the read data capture delay register */
 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;
+	bool edge = plat->sample_edge_rising;
 	u8 opcode_rdid = 0x9F;
 	unsigned int idcode = 0, temp = 0;
 	int err = 0, i, range_lo = -1, range_hi = -1;

@@ -49,7 +51,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
 	cadence_spi_write_speed(bus, 1000000);
 
 	/* configure the read data capture delay register to 0 */
-	cadence_qspi_apb_readdata_capture(base, 1, 0);
+	cadence_qspi_apb_readdata_capture(base, true, edge, 0);
 
 	/* Enable QSPI */
 	cadence_qspi_apb_controller_enable(base);

@@ -69,7 +71,7 @@ static int spi_calibration(struct udevice *bus, uint hz)
 		cadence_qspi_apb_controller_disable(base);
 
 		/* reconfigure the read data capture delay register */
-		cadence_qspi_apb_readdata_capture(base, 1, i);
+		cadence_qspi_apb_readdata_capture(base, true, edge, i);
 
 		/* Enable back QSPI */
 		cadence_qspi_apb_controller_enable(base);

@@ -105,7 +107,8 @@ static int spi_calibration(struct udevice *bus, uint hz)
 	cadence_qspi_apb_controller_disable(base);
 
 	/* configure the final value for read data capture delay register */
-	cadence_qspi_apb_readdata_capture(base, 1, (range_hi + range_lo) / 2);
+	cadence_qspi_apb_readdata_capture(base, true, edge,
+		(range_hi + range_lo) / 2);
 	debug("SF: Read data capture delay calibrated to %i (%i - %i)\n",
 	      (range_hi + range_lo) / 2, range_lo, range_hi);

@@ -298,6 +301,7 @@ static int cadence_spi_ofdata_to_platdata(struct

udevice *bus)

drivers/spi/cadence_qspi.c static int cadence_spi_ofdata_to_platdata(struct

Quoted 8 lines

drivers/spi/cadence_qspi.c +1 -0

 	plat->regbase = (void *)data[0];
 	plat->ahbbase = (void *)data[2];
+	plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
 
 	/* All other paramters are embedded in the child node */
 	subnode = fdt_first_subnode(blob, node);

@@ -317,7 +321,8 @@ static int cadence_spi_ofdata_to_platdata(struct

udevice *bus)

drivers/spi/cadence_qspi.c static int cadence_spi_ofdata_to_platdata(struct udevice *bus)

Quoted 8 lines

drivers/spi/cadence_qspi.c +2 -1

 	plat->tsd2d_ns = fdtdec_get_int(blob, subnode, "tsd2d-ns", 255);
 	plat->tchsh_ns = fdtdec_get_int(blob, subnode, "tchsh-ns", 20);
 	plat->tslch_ns = fdtdec_get_int(blob, subnode, "tslch-ns", 20);
-	plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
+	plat->sample_edge_rising = fdtdec_get_bool(blob, subnode,
+		"sample-edge-rising");
 
 	debug("%s: regbase=%p ahbbase=%p max-frequency=%d page-

size=%d\n",

drivers/spi/cadence_qspi_apb.c static unsigned int cadence_qspi_wait_idle(void

Quoted 34 lines

drivers/spi/cadence_qspi.c +0 -0

 	      __func__, plat->regbase, plat->ahbbase, plat->max_hz,

drivers/spi/cadence_qspi.h +2 -1

index a849f7b..ad1dbae 100644
@@ -26,6 +26,7 @@ struct cadence_spi_platdata {
 	u32		tchsh_ns;
 	u32		tslch_ns;
 	u32		sram_size;
+	bool		sample_edge_rising;
 };
 
 struct cadence_spi_priv {

@@ -73,6 +74,6 @@ void cadence_qspi_apb_delay(void *reg_base,
 	unsigned int tchsh_ns, unsigned int tslch_ns);
 void cadence_qspi_apb_enter_xip(void *reg_base, char xip_dummy);
 void cadence_qspi_apb_readdata_capture(void *reg_base,
-	unsigned int bypass, unsigned int delay);
+	bool bypass, bool edge, unsigned int delay);
 
 #endif /* __CADENCE_QSPI_H__ */

drivers/spi/cadence_qspi_apb.c +1 -0

index 56ad952..e43973c 100644
@@ -98,6 +98,7 @@
 #define	CQSPI_REG_RD_DATA_CAPTURE_BYPASS	BIT(0)
 #define	CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB	1
 #define	CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK	0xF
+#define	CQSPI_REG_RD_DATA_CAPTURE_EDGE		BIT(5)
 
 #define	CQSPI_REG_SIZE				0x14
 #define	CQSPI_REG_SIZE_ADDRESS_LSB		0

@@ -234,7 +235,7 @@ static unsigned int cadence_qspi_wait_idle(void

*reg_base)

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_readdata_capture(void

Quoted 9 lines

drivers/spi/cadence_qspi_apb.c +1 -1

 }
 
 void cadence_qspi_apb_readdata_capture(void *reg_base,
-				unsigned int bypass, unsigned int delay)
+	bool bypass, bool edge, unsigned int delay)
 {
 	unsigned int reg;
 	cadence_qspi_apb_controller_disable(reg_base);

@@ -246,6 +247,11 @@ void cadence_qspi_apb_readdata_capture(void

*reg_base,

Quoted 3 lines

drivers/spi/cadence_qspi_apb.c +0 -0
 	else
 		reg &= ~CQSPI_REG_RD_DATA_CAPTURE_BYPASS;
 

Quoted 14 lines

drivers/spi/cadence_qspi_apb.c void cadence_qspi_apb_readdata_capture(void *reg_base, · line 257
Quoted 9 lines
drivers/spi/cadence_qspi_apb.c +5 -0
+	if (edge)
+		reg |= CQSPI_REG_RD_DATA_CAPTURE_EDGE;
+	else
+		reg &= ~CQSPI_REG_RD_DATA_CAPTURE_EDGE;
+
 	reg &= ~(CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK
 		<< CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB);
 
--
Best regards,
Marek Vasut

Thanks
Phil

See

2016-11-28 13:37 UTC #29 lore raw

PATCH v2 8/8 spi: cadence_qspi: Support specifying the sample edge used

In reply to Phil Edworthy

Hi Phil,

Quoted 23 lines · Phil Edworthy

Whilst at it, move the code to read the "sram-size" property into the other code that reads properties from the node, rather than the SF subnode.
Also change the code to use a bool for the bypass arg.
Signed-off-by Phil Edworthy <[email protected]>
---
v2:
?Change name of DT prop and provide details of what it does. ?Whilst at it, move the code to read the "sram-size" property ?into the other code that reads properties from the node, rather ?than the SF subnode.
?Also change the code to use a bool for the bypass arg.
---
?doc/device-tree-bindings/spi/spi-cadence.txt |??2 ++
?drivers/spi/cadence_qspi.c???????????????????| 13 +++++++++----
?drivers/spi/cadence_qspi.h???????????????????|??3 ++-
?drivers/spi/cadence_qspi_apb.c???????????????|??8 +++++++-
?4 files changed, 20 insertions(+), 6 deletions(-)

[..]

Quoted 10 lines

drivers/spi/cadence_qspi_apb.c +0 -0
index 56ad952..e43973c 100644
@@ -98,6 +98,7 @@
 ?#define????????CQSPI_REG_RD_DATA_CAPTURE_BYPASS????????BIT(0)
?#define????????CQSPI_REG_RD_DATA_CAPTURE_DELAY_LSB?????1 ?#define????????CQSPI_REG_RD_DATA_CAPTURE_DELAY_MASK????0xF +#define????????CQSPI_REG_RD_DATA_CAPTURE_EDGE??????????BIT(5)

Actually we don't have this edge bit at SOCFPGA.
But no harm as its unused bit at SOCFPGA today

Thanks
Chin Liang

Jagan Teki

2016-11-28 08:07 UTC #30 lore raw

v2 cover SF: Cadence QSPI driver fixes and clean up

In reply to Phil Edworthy

Quoted 8 lines · Phil Edworthy

This series has fixes, patches to clean the code up, and add support for specifying the sampling edge.
Changed in v2:
spi: cadence_qspi: Fix baud rate calculation
spi: cadence_qspi: Fix CS timings (was "Fix CQSPI_CAL_DELAY calculation")
spi: cadence_qspi: Support specifying the sample edge used

Quoted 6 lines

Added in v2:
spi: cadence_qspi: Better debug information on the SPI clock rate
Phil Edworthy (8):
spi: cadence_qspi: Fix clearing of pol/pha bits
spi: cadence_qspi: Fix baud rate calculation

Please fix the comment for this patch.

Quoted 2 lines

spi: cadence_qspi: Better debug information on the SPI clock rate
spi: cadence_qspi: Use #define for bits instead of bit shifts

And this one,

Quoted 4 lines

spi: cadence_qspi: Clean up the #define names
spi: cadence_qspi: Remove returns from end of void functions
spi: cadence_qspi: Fix CS timings
spi: cadence_qspi: Support specifying the sample edge used

All look OK, expect above two.

thanks!

Signature · 4 lines

Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

Marek Vasut

2016-11-28 12:50 UTC #31 lore raw

v2 cover SF: Cadence QSPI driver fixes and clean up

Quoted 10 lines · Jagan Teki

Quoted 8 lines · Phil Edworthy
This series has fixes, patches to clean the code up, and add support for specifying the sampling edge.
Changed in v2:
spi: cadence_qspi: Fix baud rate calculation
spi: cadence_qspi: Fix CS timings (was "Fix CQSPI_CAL_DELAY calculation")
spi: cadence_qspi: Support specifying the sample edge used

Quoted 8 lines

Quoted 6 lines
Added in v2:
spi: cadence_qspi: Better debug information on the SPI clock rate
Phil Edworthy (8):
spi: cadence_qspi: Fix clearing of pol/pha bits
spi: cadence_qspi: Fix baud rate calculation
Please fix the comment for this patch.

Fix how ? It seems perfectly fine to me, so explain.

Quoted 4 lines

Quoted 2 lines
spi: cadence_qspi: Better debug information on the SPI clock rate
spi: cadence_qspi: Use #define for bits instead of bit shifts
And this one,

DTTO, seems perfectly fine.

Quoted 9 lines

Quoted 4 lines
spi: cadence_qspi: Clean up the #define names
spi: cadence_qspi: Remove returns from end of void functions
spi: cadence_qspi: Fix CS timings
spi: cadence_qspi: Support specifying the sample edge used
All look OK, expect above two.
thanks!

Signature · 2 lines

Best regards,
Marek Vasut

Jagan Teki

2016-11-28 13:32 UTC #32 lore raw

v2 cover SF: Cadence QSPI driver fixes and clean up

Quoted 20 lines · Marek Vasut

Quoted 10 lines · Jagan Teki
Quoted 8 lines · Phil Edworthy
This series has fixes, patches to clean the code up, and add support for specifying the sampling edge.
Changed in v2:
spi: cadence_qspi: Fix baud rate calculation
spi: cadence_qspi: Fix CS timings (was "Fix CQSPI_CAL_DELAY calculation")
spi: cadence_qspi: Support specifying the sample edge used
Quoted 8 lines
Quoted 6 lines
Added in v2:
spi: cadence_qspi: Better debug information on the SPI clock rate
Phil Edworthy (8):
spi: cadence_qspi: Fix clearing of pol/pha bits
spi: cadence_qspi: Fix baud rate calculation
Please fix the comment for this patch.

Quoted 8 lines

Fix how ? It seems perfectly fine to me, so explain.
Quoted 4 lines
Quoted 2 lines
spi: cadence_qspi: Better debug information on the SPI clock rate
spi: cadence_qspi: Use #define for bits instead of bit shifts
And this one,
DTTO, seems perfectly fine.

See my comments on these patch threads.

thanks!

Signature · 4 lines

Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.