Skip to contentSkip to conversation

spi: altera: Use struct-based register access

PATCH New Marek Vasut 3 participants 16 messages Oct 19, 2014 – Oct 21, 2014 SPI

lore
Share & tools
lore

Marek Vasut

author
#0 lore raw

PATCH 1/7 spi: altera: Use struct-based register access

Zap the offset-based register access and use the struct-based one as this is the preferred method.

No functional change, but there are some line-over-80 problems in the driver, which will be addressed later.

Signed-off-by Marek Vasut <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Rini <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Jagannadha Sutradharudu Teki <[email protected]>
---
 drivers/spi/altera_spi.c | 49 ++++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 24 deletions(-)
drivers/spi/altera_spi.c +25 -24
index 5accbb5..13191f3 100644
@@ -12,11 +12,14 @@
 #include <malloc.h>
 #include <spi.h>
 
-#define ALTERA_SPI_RXDATA	0
-#define ALTERA_SPI_TXDATA	4
-#define ALTERA_SPI_STATUS	8
-#define ALTERA_SPI_CONTROL	12
-#define ALTERA_SPI_SLAVE_SEL	20
+struct altera_spi_regs {
+	u32	rxdata;
+	u32	txdata;
+	u32	status;
+	u32	control;
+	u32	_reserved;
+	u32	slave_sel;
+};
 
 #define ALTERA_SPI_STATUS_ROE_MSK	(0x8)
 #define ALTERA_SPI_STATUS_TOE_MSK	(0x10)

@@ -39,8 +42,8 @@
 static ulong altera_spi_base_list[] = CONFIG_SYS_ALTERA_SPI_LIST;
 
 struct altera_spi_slave {
-	struct spi_slave slave;
-	ulong base;
+	struct spi_slave	slave;
+	struct altera_spi_regs	*regs;
 };
 #define to_altera_spi_slave(s) container_of(s, struct altera_spi_slave, slave)
 

@@ -54,16 +57,16 @@ __attribute__((weak))
 void spi_cs_activate(struct spi_slave *slave)
 {
 	struct altera_spi_slave *altspi = to_altera_spi_slave(slave);
-	writel(1 << slave->cs, altspi->base + ALTERA_SPI_SLAVE_SEL);
-	writel(ALTERA_SPI_CONTROL_SSO_MSK, altspi->base + ALTERA_SPI_CONTROL);
+	writel(1 << slave->cs, &altspi->regs->slave_sel);
+	writel(ALTERA_SPI_CONTROL_SSO_MSK, &altspi->regs->control);
 }
 
 __attribute__((weak))
 void spi_cs_deactivate(struct spi_slave *slave)
 {
 	struct altera_spi_slave *altspi = to_altera_spi_slave(slave);
-	writel(0, altspi->base + ALTERA_SPI_CONTROL);
-	writel(0, altspi->base + ALTERA_SPI_SLAVE_SEL);
+	writel(0, &altspi->regs->control);
+	writel(0, &altspi->regs->slave_sel);
 }
 
 void spi_init(void)

@@ -87,9 +90,9 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
 	if (!altspi)
 		return NULL;
 
-	altspi->base = altera_spi_base_list[bus];
-	debug("%s: bus:%i cs:%i base:%lx\n", __func__,
-		bus, cs, altspi->base);
+	altspi->regs = (struct altera_spi_regs *)altera_spi_base_list[bus];
+	debug("%s: bus:%i cs:%i base:%p\n", __func__,
+		bus, cs, altspi->regs);
 
 	return &altspi->slave;
 }

@@ -105,8 +108,8 @@ int spi_claim_bus(struct spi_slave *slave)
 	struct altera_spi_slave *altspi = to_altera_spi_slave(slave);
 
 	debug("%s: bus:%i cs:%i\n", __func__, slave->bus, slave->cs);
-	writel(0, altspi->base + ALTERA_SPI_CONTROL);
-	writel(0, altspi->base + ALTERA_SPI_SLAVE_SEL);
+	writel(0, &altspi->regs->control);
+	writel(0, &altspi->regs->slave_sel);
 	return 0;
 }
 

@@ -115,7 +118,7 @@ void spi_release_bus(struct spi_slave *slave)
 	struct altera_spi_slave *altspi = to_altera_spi_slave(slave);
 
 	debug("%s: bus:%i cs:%i\n", __func__, slave->bus, slave->cs);
-	writel(0, altspi->base + ALTERA_SPI_SLAVE_SEL);
+	writel(0, &altspi->regs->slave_sel);
 }
 
 #ifndef CONFIG_ALTERA_SPI_IDLE_VAL

@@ -142,20 +145,18 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 	}
 
 	/* empty read buffer */
-	if (readl(altspi->base + ALTERA_SPI_STATUS) &
-	    ALTERA_SPI_STATUS_RRDY_MSK)
-		readl(altspi->base + ALTERA_SPI_RXDATA);
+	if (readl(&altspi->regs->status) & ALTERA_SPI_STATUS_RRDY_MSK)
+		readl(&altspi->regs->rxdata);
 	if (flags & SPI_XFER_BEGIN)
 		spi_cs_activate(slave);
 
 	while (bytes--) {
 		uchar d = txp ? *txp++ : CONFIG_ALTERA_SPI_IDLE_VAL;
 		debug("%s: tx:%x ", __func__, d);
-		writel(d, altspi->base + ALTERA_SPI_TXDATA);
-		while (!(readl(altspi->base + ALTERA_SPI_STATUS) &
-			 ALTERA_SPI_STATUS_RRDY_MSK))
+		writel(d, &altspi->regs->txdata);
+		while (!(readl(&altspi->regs->status) & ALTERA_SPI_STATUS_RRDY_MSK))
 			;
-		d = readl(altspi->base + ALTERA_SPI_RXDATA);
+		d = readl(&altspi->regs->rxdata);
 		if (rxp)
 			*rxp++ = d;
 		debug("rx:%x\n", d);
Signature · 1 line
2.1.1
author
#1 lore raw

PATCH 2/7 spi: altera: Clean up bit definitions

Clean up the definitions of bits in the Altera SPI driver, there is no need to put braces around numbers afterall. No functional change.

Signed-off-by Marek Vasut <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Rini <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Jagannadha Sutradharudu Teki <[email protected]>
---
 drivers/spi/altera_spi.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
drivers/spi/altera_spi.c +13 -13
index 13191f3..21f90fc 100644
@@ -21,19 +21,19 @@ struct altera_spi_regs {
 	u32	slave_sel;
 };
 
-#define ALTERA_SPI_STATUS_ROE_MSK	(0x8)
-#define ALTERA_SPI_STATUS_TOE_MSK	(0x10)
-#define ALTERA_SPI_STATUS_TMT_MSK	(0x20)
-#define ALTERA_SPI_STATUS_TRDY_MSK	(0x40)
-#define ALTERA_SPI_STATUS_RRDY_MSK	(0x80)
-#define ALTERA_SPI_STATUS_E_MSK	(0x100)
-
-#define ALTERA_SPI_CONTROL_IROE_MSK	(0x8)
-#define ALTERA_SPI_CONTROL_ITOE_MSK	(0x10)
-#define ALTERA_SPI_CONTROL_ITRDY_MSK	(0x40)
-#define ALTERA_SPI_CONTROL_IRRDY_MSK	(0x80)
-#define ALTERA_SPI_CONTROL_IE_MSK	(0x100)
-#define ALTERA_SPI_CONTROL_SSO_MSK	(0x400)
+#define ALTERA_SPI_STATUS_ROE_MSK	(1 << 3)
+#define ALTERA_SPI_STATUS_TOE_MSK	(1 << 4)
+#define ALTERA_SPI_STATUS_TMT_MSK	(1 << 5)
+#define ALTERA_SPI_STATUS_TRDY_MSK	(1 << 6)
+#define ALTERA_SPI_STATUS_RRDY_MSK	(1 << 7)
+#define ALTERA_SPI_STATUS_E_MSK		(1 << 8)
+
+#define ALTERA_SPI_CONTROL_IROE_MSK	(1 << 3)
+#define ALTERA_SPI_CONTROL_ITOE_MSK	(1 << 4)
+#define ALTERA_SPI_CONTROL_ITRDY_MSK	(1 << 6)
+#define ALTERA_SPI_CONTROL_IRRDY_MSK	(1 << 7)
+#define ALTERA_SPI_CONTROL_IE_MSK	(1 << 8)
+#define ALTERA_SPI_CONTROL_SSO_MSK	(1 << 10)
 
 #ifndef CONFIG_SYS_ALTERA_SPI_LIST
 #define CONFIG_SYS_ALTERA_SPI_LIST { CONFIG_SYS_SPI_BASE }
Signature · 1 line
2.1.1
author
#2 lore raw

PATCH 3/7 spi: altera: Clean up most checkpatch issues

In reply to Marek Vasut

This patch just zaps most of the checkpatch cries present in the driver. There is one more left, which will be addressed separately. There is no functional change.

This patch also adds a bunch of newlines all around the place, this is to make the code much more readable.

Signed-off-by Marek Vasut <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Rini <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Jagannadha Sutradharudu Teki <[email protected]>
---
 drivers/spi/altera_spi.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)
drivers/spi/altera_spi.c +13 -10
index 21f90fc..373ce30 100644
@@ -47,22 +47,19 @@ struct altera_spi_slave {
 };
 #define to_altera_spi_slave(s) container_of(s, struct altera_spi_slave, slave)
 
-__attribute__((weak))
-int spi_cs_is_valid(unsigned int bus, unsigned int cs)
+__weak int spi_cs_is_valid(unsigned int bus, unsigned int cs)
 {
 	return bus < ARRAY_SIZE(altera_spi_base_list) && cs < 32;
 }
 
-__attribute__((weak))
-void spi_cs_activate(struct spi_slave *slave)
+__weak void spi_cs_activate(struct spi_slave *slave)
 {
 	struct altera_spi_slave *altspi = to_altera_spi_slave(slave);
 	writel(1 << slave->cs, &altspi->regs->slave_sel);
 	writel(ALTERA_SPI_CONTROL_SSO_MSK, &altspi->regs->control);
 }
 
-__attribute__((weak))
-void spi_cs_deactivate(struct spi_slave *slave)
+__weak void spi_cs_deactivate(struct spi_slave *slave)
 {
 	struct altera_spi_slave *altspi = to_altera_spi_slave(slave);
 	writel(0, &altspi->regs->control);

@@ -91,8 +88,7 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
 		return NULL;
 
 	altspi->regs = (struct altera_spi_regs *)altera_spi_base_list[bus];
-	debug("%s: bus:%i cs:%i base:%p\n", __func__,
-		bus, cs, altspi->regs);
+	debug("%s: bus:%i cs:%i base:%p\n", __func__, bus, cs, altspi->regs);
 
 	return &altspi->slave;
 }

@@ -135,7 +131,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 	uchar *rxp = din;
 
 	debug("%s: bus:%i cs:%i bitlen:%i bytes:%i flags:%lx\n", __func__,
-		slave->bus, slave->cs, bitlen, bytes, flags);
+	      slave->bus, slave->cs, bitlen, bytes, flags);
+
 	if (bitlen == 0)
 		goto done;
 

@@ -147,21 +144,27 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 	/* empty read buffer */
 	if (readl(&altspi->regs->status) & ALTERA_SPI_STATUS_RRDY_MSK)
 		readl(&altspi->regs->rxdata);
+
 	if (flags & SPI_XFER_BEGIN)
 		spi_cs_activate(slave);
 
 	while (bytes--) {
 		uchar d = txp ? *txp++ : CONFIG_ALTERA_SPI_IDLE_VAL;
+
 		debug("%s: tx:%x ", __func__, d);
 		writel(d, &altspi->regs->txdata);
+
 		while (!(readl(&altspi->regs->status) & ALTERA_SPI_STATUS_RRDY_MSK))
 			;
+
 		d = readl(&altspi->regs->rxdata);
 		if (rxp)
 			*rxp++ = d;
+
 		debug("rx:%x\n", d);
 	}
- done:
+
+done:
 	if (flags & SPI_XFER_END)
 		spi_cs_deactivate(slave);
Signature · 1 line
2.1.1
author
#3 lore raw

PATCH 4/7 spi: altera: Zap endless loop

In reply to Marek Vasut

The driver contained an endless loop when waiting for TX completion, this is a bad idea since if the hardware fails, the loop might spin forever. Add timeout and handle it.

Signed-off-by Marek Vasut <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Rini <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Jagannadha Sutradharudu Teki <[email protected]>
---
 drivers/spi/altera_spi.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
drivers/spi/altera_spi.c +12 -2
index 373ce30..ee65ec2 100644
@@ -129,6 +129,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 	uint bytes = bitlen / 8;
 	const uchar *txp = dout;
 	uchar *rxp = din;
+	int timeout = 10000;
+	uint32_t reg;
 
 	debug("%s: bus:%i cs:%i bitlen:%i bytes:%i flags:%lx\n", __func__,
 	      slave->bus, slave->cs, bitlen, bytes, flags);

@@ -154,8 +156,16 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 		debug("%s: tx:%x ", __func__, d);
 		writel(d, &altspi->regs->txdata);
 
-		while (!(readl(&altspi->regs->status) & ALTERA_SPI_STATUS_RRDY_MSK))
-			;
+		while (--timeout) {
+			reg = readl(&altspi->regs->status);
+			if (reg & ALTERA_SPI_STATUS_RRDY_MSK)
+				break;
+		}
+
+		if (!timeout) {
+			printf("%s: Transmission timed out!\n", __func__);
+			goto done;
+		}
 
 		d = readl(&altspi->regs->rxdata);
 		if (rxp)
Signature · 1 line
2.1.1
#4 lore raw

PATCH 4/7 spi: altera: Zap endless loop

Quoted 21 lines · Marek Vasut

The driver contained an endless loop when waiting for TX completion, this is a bad idea since if the hardware fails, the loop might spin forever. Add timeout and handle it.

Signed-off-by Marek Vasut <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Rini <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Jagannadha Sutradharudu Teki <[email protected]>
---
 drivers/spi/altera_spi.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
drivers/spi/altera_spi.c +0 -0
index 373ce30..ee65ec2 100644

drivers/spi/altera_spi.c int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, · line 132

Quoted 4 lines
drivers/spi/altera_spi.c +1 -0
@@ -129,6 +129,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
        uint bytes = bitlen / 8;
        const uchar *txp = dout;
        uchar *rxp = din;
+       int timeout = 10000;

This could be macro definable.

Quoted 10 lines
drivers/spi/altera_spi.c +1 -2
+       uint32_t reg;
 
        debug("%s: bus:%i cs:%i bitlen:%i bytes:%i flags:%lx\n", __func__,
              slave->bus, slave->cs, bitlen, bytes, flags);

@@ -154,8 +156,16 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
                debug("%s: tx:%x ", __func__, d);
                writel(d, &altspi->regs->txdata);
 
-               while (!(readl(&altspi->regs->status) & ALTERA_SPI_STATUS_RRDY_MSK))
-                       ;

drivers/spi/altera_spi.c int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, · line 168

Quoted 10 lines
drivers/spi/altera_spi.c +10 -0
+               while (--timeout) {
+                       reg = readl(&altspi->regs->status);
+                       if (reg & ALTERA_SPI_STATUS_RRDY_MSK)
+                               break;
+               }
+
+               if (!timeout) {
+                       printf("%s: Transmission timed out!\n", __func__);
+                       goto done;
+               }

It's better to use tx status check with the help of get_timer() instead of normal while loop.
Pls- use the same, we have some drivers who does the same.

Quoted 6 lines
drivers/spi/altera_spi.c +0 -1
                d = readl(&altspi->regs->rxdata);
                if (rxp)
--

2.1.1

thanks!

Signature · 1 line
Jagan.
author
#5 lore raw

PATCH 5/7 spi: altera: Clean up the use of variable d

In reply to Marek Vasut

The variable d is used in rather questionable way. Rework the code a bit so it's clearer what it does. Also, rename the variable from d to data to make it's name less mysterious. Finally, change it's data type to uint32_t , since it's accessed as a 32bit number.

Signed-off-by Marek Vasut <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Rini <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Jagannadha Sutradharudu Teki <[email protected]>
---
 drivers/spi/altera_spi.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)
drivers/spi/altera_spi.c +13 -10
index ee65ec2..3065e96 100644
@@ -126,11 +126,11 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 {
 	struct altera_spi_slave *altspi = to_altera_spi_slave(slave);
 	/* assume spi core configured to do 8 bit transfers */
-	uint bytes = bitlen / 8;
-	const uchar *txp = dout;
-	uchar *rxp = din;
+	unsigned int bytes = bitlen / 8;
+	const unsigned char *txp = dout;
+	unsigned char *rxp = din;
 	int timeout = 10000;
-	uint32_t reg;
+	uint32_t reg, data;
 
 	debug("%s: bus:%i cs:%i bitlen:%i bytes:%i flags:%lx\n", __func__,
 	      slave->bus, slave->cs, bitlen, bytes, flags);

@@ -151,10 +151,13 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 		spi_cs_activate(slave);
 
 	while (bytes--) {
-		uchar d = txp ? *txp++ : CONFIG_ALTERA_SPI_IDLE_VAL;
+		if (txp)
+			data = *txp++;
+		else
+			data = CONFIG_ALTERA_SPI_IDLE_VAL;
 
-		debug("%s: tx:%x ", __func__, d);
-		writel(d, &altspi->regs->txdata);
+		debug("%s: tx:%x ", __func__, data);
+		writel(data, &altspi->regs->txdata);
 
 		while (--timeout) {
 			reg = readl(&altspi->regs->status);

@@ -167,11 +170,11 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 			goto done;
 		}
 
-		d = readl(&altspi->regs->rxdata);
+		data = readl(&altspi->regs->rxdata);
 		if (rxp)
-			*rxp++ = d;
+			*rxp++ = data & 0xff;
 
-		debug("rx:%x\n", d);
+		debug("rx:%x\n", data);
 	}
 
 done:
Signature · 1 line
2.1.1
author
#6 lore raw

PATCH 6/7 spi: altera: Add short note about EPCS/EPCQx1

In reply to Marek Vasut

Add short documentation-alike note on how to use the Altera SPI driver with the EPCS/EPCQx1 FPGA IP block on SoCFPGA Cyclone V.

Signed-off-by Marek Vasut <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Rini <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Jagannadha Sutradharudu Teki <[email protected]>
---
 drivers/spi/altera_spi.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/spi/altera_spi.c +8 -0
index 3065e96..0566e4f 100644
@@ -4,6 +4,14 @@
  * based on bfin_spi.c
  * Copyright (c) 2005-2008 Analog Devices Inc.
  * Copyright (C) 2010 Thomas Chou <[email protected]>
+ * Copyright (C) 2014 Marek Vasut <[email protected]>
+ *
+ * SoCFPGA EPCS/EPCQx1 mini howto:
+ * - Instantiate EPCS/EPCQx1 Serial flash controller in QSys and rebuild
+ * - The controller base address is the "Base" in QSys + 0x400
+ * - Set MSEL[4:0]=10010 (AS Standard)
+ * - Load the bitstream into FPGA, enable bridges
+ * - Only then will the driver work
  *
  * SPDX-License-Identifier:	GPL-2.0+
  */
Signature · 1 line
2.1.1
#7 lore raw

PATCH 6/7 spi: altera: Add short note about EPCS/EPCQx1

Quoted 20 lines · Marek Vasut

Add short documentation-alike note on how to use the Altera SPI driver with the EPCS/EPCQx1 FPGA IP block on SoCFPGA Cyclone V.

Signed-off-by Marek Vasut <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Rini <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Jagannadha Sutradharudu Teki <[email protected]>
---
 drivers/spi/altera_spi.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/spi/altera_spi.c +0 -0
index 3065e96..0566e4f 100644

drivers/spi/altera_spi.c line 7

Quoted 4 lines
drivers/spi/altera_spi.c +1 -0
@@ -4,6 +4,14 @@
  * based on bfin_spi.c
  * Copyright (c) 2005-2008 Analog Devices Inc.
  * Copyright (C) 2010 Thomas Chou <[email protected]>
+ * Copyright (C) 2014 Marek Vasut <[email protected]>

Looks not good to me - with few changes.

drivers/spi/altera_spi.c line 14

Quoted 7 lines
drivers/spi/altera_spi.c +7 -0
+ *
+ * SoCFPGA EPCS/EPCQx1 mini howto:
+ * - Instantiate EPCS/EPCQx1 Serial flash controller in QSys and rebuild
+ * - The controller base address is the "Base" in QSys + 0x400
+ * - Set MSEL[4:0]=10010 (AS Standard)
+ * - Load the bitstream into FPGA, enable bridges
+ * - Only then will the driver work

Instead of here,
Pls- try to test any hardware with this written sequence with the obtained logs copy
that info on doc/SPI we're maintaining the test in this format.

Testing the written sequence with logs are more authentic than listing on a driver file.

Quoted 6 lines
drivers/spi/altera_spi.c +0 -1
  *
  * SPDX-License-Identifier:    GPL-2.0+
  */
--

2.1.1

thanks!

Signature · 1 line
Jagan.
author
#8 lore raw

PATCH 6/7 spi: altera: Add short note about EPCS/EPCQx1

On Monday, October 20, 2014 at 05:10:17 PM, Jagan Teki wrote:

[...]

Quoted 16 lines
Quoted 5 lines

* based on bfin_spi.c
* Copyright (c) 2005-2008 Analog Devices Inc.
* Copyright (C) 2010 Thomas Chou <[email protected]>

+ * Copyright (C) 2014 Marek Vasut <[email protected]>

Looks not good to me - with few changes.

Quoted 7 lines

+ *
+ * SoCFPGA EPCS/EPCQx1 mini howto:
+ * - Instantiate EPCS/EPCQx1 Serial flash controller in QSys and rebuild
+ * - The controller base address is the "Base" in QSys + 0x400
+ * - Set MSEL[4:0]=10010 (AS Standard)
+ * - Load the bitstream into FPGA, enable bridges
+ * - Only then will the driver work

Quoted 7 lines

Instead of here,
Pls- try to test any hardware with this written sequence with the obtained logs copy
that info on doc/SPI we're maintaining the test in this format.

Testing the written sequence with logs are more authentic than listing on a driver file.

Excuse me, but I don't quite understand what you're asking of me, sorry.

Best regards,
Marek Vasut

#9 lore raw

PATCH 6/7 spi: altera: Add short note about EPCS/EPCQx1

Quoted 25 lines · Marek Vasut

On Monday, October 20, 2014 at 05:10:17 PM, Jagan Teki wrote:

[...]

Quoted 21 lines
Quoted 5 lines

* based on bfin_spi.c
* Copyright (c) 2005-2008 Analog Devices Inc.
* Copyright (C) 2010 Thomas Chou <[email protected]>

+ * Copyright (C) 2014 Marek Vasut <[email protected]>

Looks not good to me - with few changes.

Quoted 7 lines

+ *
+ * SoCFPGA EPCS/EPCQx1 mini howto:
+ * - Instantiate EPCS/EPCQx1 Serial flash controller in QSys and rebuild
+ * - The controller base address is the "Base" in QSys + 0x400
+ * - Set MSEL[4:0]=10010 (AS Standard)
+ * - Load the bitstream into FPGA, enable bridges
+ * - Only then will the driver work

Instead of here,
Pls- try to test any hardware with this written sequence with the obtained logs copy
that info on doc/SPI we're maintaining the test in this format.

Quoted 4 lines
Quoted 2 lines

Testing the written sequence with logs are more authentic than listing on a driver file.

Excuse me, but I don't quite understand what you're asking of me, sorry.

OK, the steps you written is looks like a usage of driver on specific module, my point
here was instead of placing these into driver file try to verify the same steps on hardware
with the help of simple spi tests and place the same file on doc/SPI/ .

It's a well showed prof that this driver is working with this kind of steps.

thanks!

Signature · 1 line
Jagan.
author
#10 lore raw

PATCH 7/7 spi: altera: Move the config options to the top

In reply to Marek Vasut

Just move the configuration options scattered all over the driver to the top of the source file. No functional change.

Signed-off-by Marek Vasut <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Rini <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Jagannadha Sutradharudu Teki <[email protected]>
---
 drivers/spi/altera_spi.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/spi/altera_spi.c +8 -8
index 0566e4f..f3a6fc9 100644
@@ -20,6 +20,14 @@
 #include <malloc.h>
 #include <spi.h>
 
+#ifndef CONFIG_ALTERA_SPI_IDLE_VAL
+#define CONFIG_ALTERA_SPI_IDLE_VAL 0xff
+#endif
+
+#ifndef CONFIG_SYS_ALTERA_SPI_LIST
+#define CONFIG_SYS_ALTERA_SPI_LIST { CONFIG_SYS_SPI_BASE }
+#endif
+
 struct altera_spi_regs {
 	u32	rxdata;
 	u32	txdata;

@@ -43,10 +51,6 @@ struct altera_spi_regs {
 #define ALTERA_SPI_CONTROL_IE_MSK	(1 << 8)
 #define ALTERA_SPI_CONTROL_SSO_MSK	(1 << 10)
 
-#ifndef CONFIG_SYS_ALTERA_SPI_LIST
-#define CONFIG_SYS_ALTERA_SPI_LIST { CONFIG_SYS_SPI_BASE }
-#endif
-
 static ulong altera_spi_base_list[] = CONFIG_SYS_ALTERA_SPI_LIST;
 
 struct altera_spi_slave {

@@ -125,10 +129,6 @@ void spi_release_bus(struct spi_slave *slave)
 	writel(0, &altspi->regs->slave_sel);
 }
 
-#ifndef CONFIG_ALTERA_SPI_IDLE_VAL
-# define CONFIG_ALTERA_SPI_IDLE_VAL 0xff
-#endif
-
 int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 	     void *din, unsigned long flags)
 {
Signature · 1 line
2.1.1
#11 lore raw

PATCH 1/7 spi: altera: Use struct-based register access

In reply to Marek Vasut
Quoted 5 lines · Marek Vasut

Zap the offset-based register access and use the struct-based one as this is the preferred method.

No functional change, but there are some line-over-80 problems in the driver, which will be addressed later.

For the whole series,

Acked-by Pavel Machek <[email protected]>
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#12 lore raw

PATCH 1/7 spi: altera: Use struct-based register access

In reply to Marek Vasut
Quoted 29 lines · Marek Vasut

Zap the offset-based register access and use the struct-based one as this is the preferred method.

No functional change, but there are some line-over-80 problems in the driver, which will be addressed later.

Signed-off-by Marek Vasut <[email protected]>
Cc Chin Liang See <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Tom Rini <[email protected]>
Cc Wolfgang Denk <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Jagannadha Sutradharudu Teki <[email protected]>
---
 drivers/spi/altera_spi.c | 49 ++++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 24 deletions(-)
drivers/spi/altera_spi.c +0 -3
index 5accbb5..13191f3 100644
@@ -12,11 +12,14 @@
 #include <malloc.h>
 #include <spi.h>
 
-#define ALTERA_SPI_RXDATA      0
-#define ALTERA_SPI_TXDATA      4
-#define ALTERA_SPI_STATUS      8

drivers/spi/altera_spi.c line 22

Quoted 10 lines
drivers/spi/altera_spi.c +8 -2
-#define ALTERA_SPI_CONTROL     12
-#define ALTERA_SPI_SLAVE_SEL   20
+struct altera_spi_regs {
+       u32     rxdata;
+       u32     txdata;
+       u32     status;
+       u32     control;
+       u32     _reserved;
+       u32     slave_sel;
+};

Can you place this structure definition below of all macro defines, i don't think the
next level patches does that, does they?

Quoted 95 lines
drivers/spi/altera_spi.c +17 -20
 #define ALTERA_SPI_STATUS_ROE_MSK      (0x8)
 #define ALTERA_SPI_STATUS_TOE_MSK      (0x10)

@@ -39,8 +42,8 @@
 static ulong altera_spi_base_list[] = CONFIG_SYS_ALTERA_SPI_LIST;
 
 struct altera_spi_slave {
-       struct spi_slave slave;
-       ulong base;
+       struct spi_slave        slave;
+       struct altera_spi_regs  *regs;
 };
 #define to_altera_spi_slave(s) container_of(s, struct altera_spi_slave, slave)
 

@@ -54,16 +57,16 @@ __attribute__((weak))
 void spi_cs_activate(struct spi_slave *slave)
 {
        struct altera_spi_slave *altspi = to_altera_spi_slave(slave);
-       writel(1 << slave->cs, altspi->base + ALTERA_SPI_SLAVE_SEL);
-       writel(ALTERA_SPI_CONTROL_SSO_MSK, altspi->base + ALTERA_SPI_CONTROL);
+       writel(1 << slave->cs, &altspi->regs->slave_sel);
+       writel(ALTERA_SPI_CONTROL_SSO_MSK, &altspi->regs->control);
 }
 
 __attribute__((weak))
 void spi_cs_deactivate(struct spi_slave *slave)
 {
        struct altera_spi_slave *altspi = to_altera_spi_slave(slave);
-       writel(0, altspi->base + ALTERA_SPI_CONTROL);
-       writel(0, altspi->base + ALTERA_SPI_SLAVE_SEL);
+       writel(0, &altspi->regs->control);
+       writel(0, &altspi->regs->slave_sel);
 }
 
 void spi_init(void)

@@ -87,9 +90,9 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
        if (!altspi)
                return NULL;
 
-       altspi->base = altera_spi_base_list[bus];
-       debug("%s: bus:%i cs:%i base:%lx\n", __func__,
-               bus, cs, altspi->base);
+       altspi->regs = (struct altera_spi_regs *)altera_spi_base_list[bus];
+       debug("%s: bus:%i cs:%i base:%p\n", __func__,
+               bus, cs, altspi->regs);
 
        return &altspi->slave;
 }

@@ -105,8 +108,8 @@ int spi_claim_bus(struct spi_slave *slave)
        struct altera_spi_slave *altspi = to_altera_spi_slave(slave);
 
        debug("%s: bus:%i cs:%i\n", __func__, slave->bus, slave->cs);
-       writel(0, altspi->base + ALTERA_SPI_CONTROL);
-       writel(0, altspi->base + ALTERA_SPI_SLAVE_SEL);
+       writel(0, &altspi->regs->control);
+       writel(0, &altspi->regs->slave_sel);
        return 0;
 }
 

@@ -115,7 +118,7 @@ void spi_release_bus(struct spi_slave *slave)
        struct altera_spi_slave *altspi = to_altera_spi_slave(slave);
 
        debug("%s: bus:%i cs:%i\n", __func__, slave->bus, slave->cs);
-       writel(0, altspi->base + ALTERA_SPI_SLAVE_SEL);
+       writel(0, &altspi->regs->slave_sel);
 }
 
 #ifndef CONFIG_ALTERA_SPI_IDLE_VAL

@@ -142,20 +145,18 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
        }
 
        /* empty read buffer */
-       if (readl(altspi->base + ALTERA_SPI_STATUS) &
-           ALTERA_SPI_STATUS_RRDY_MSK)
-               readl(altspi->base + ALTERA_SPI_RXDATA);
+       if (readl(&altspi->regs->status) & ALTERA_SPI_STATUS_RRDY_MSK)
+               readl(&altspi->regs->rxdata);
        if (flags & SPI_XFER_BEGIN)
                spi_cs_activate(slave);
 
        while (bytes--) {
                uchar d = txp ? *txp++ : CONFIG_ALTERA_SPI_IDLE_VAL;
                debug("%s: tx:%x ", __func__, d);
-               writel(d, altspi->base + ALTERA_SPI_TXDATA);
-               while (!(readl(altspi->base + ALTERA_SPI_STATUS) &
-                        ALTERA_SPI_STATUS_RRDY_MSK))
+               writel(d, &altspi->regs->txdata);
+               while (!(readl(&altspi->regs->status) & ALTERA_SPI_STATUS_RRDY_MSK))
                        ;
-               d = readl(altspi->base + ALTERA_SPI_RXDATA);
+               d = readl(&altspi->regs->rxdata);
                if (rxp)
                        *rxp++ = d;
                debug("rx:%x\n", d);
--
Quoted 2 lines

2.1.1

thanks!

Signature · 1 line
Jagan.
author
#13 lore raw

PATCH 1/7 spi: altera: Use struct-based register access

On Monday, October 20, 2014 at 04:53:15 PM, Jagan Teki wrote:

[...]

Quoted 9 lines
Quoted 9 lines
drivers/spi/altera_spi.c +4 -5
-#define ALTERA_SPI_RXDATA      0
-#define ALTERA_SPI_TXDATA      4
-#define ALTERA_SPI_STATUS      8
-#define ALTERA_SPI_CONTROL     12
-#define ALTERA_SPI_SLAVE_SEL   20
+struct altera_spi_regs {
+       u32     rxdata;
+       u32     txdata;
+       u32     status;
Quoted 8 lines

drivers/spi/altera_spi.c line 22

Quoted 4 lines
drivers/spi/altera_spi.c +4 -0
+       u32     control;
+       u32     _reserved;
+       u32     slave_sel;
+};

Can you place this structure definition below of all macro defines, i don't think the
next level patches does that, does they?

Does it make sense to you, to first define the bits in registers and then the register layout ? It does not make sense to me, so I would prefer to keep it like it is.

[...]

Best regards,
Marek Vasut

#14 lore raw

PATCH 1/7 spi: altera: Use struct-based register access

Quoted 22 lines · Marek Vasut

On Monday, October 20, 2014 at 04:53:15 PM, Jagan Teki wrote:

[...]

Quoted 9 lines
Quoted 9 lines
drivers/spi/altera_spi.c +4 -5
-#define ALTERA_SPI_RXDATA      0
-#define ALTERA_SPI_TXDATA      4
-#define ALTERA_SPI_STATUS      8
-#define ALTERA_SPI_CONTROL     12
-#define ALTERA_SPI_SLAVE_SEL   20
+struct altera_spi_regs {
+       u32     rxdata;
+       u32     txdata;
+       u32     status;
Quoted 8 lines

drivers/spi/altera_spi.c line 22

Quoted 4 lines
drivers/spi/altera_spi.c +4 -0
+       u32     control;
+       u32     _reserved;
+       u32     slave_sel;
+};

Can you place this structure definition below of all macro defines, i don't think the
next level patches does that, does they?

Quoted 3 lines

Does it make sense to you, to first define the bits in registers and then the register layout ? It does not make sense to me, so I would prefer to keep it like it is.

You're correct the way you replaced, usually the driver code will go like this

-- >includes

--> macros definitions

--> global or structure definitions

--> driver function calls.

We follow this [1] to make the driver more readable.

[1] http://patchwork.ozlabs.org/patch/265683/

thanks!

Signature · 1 line
Jagan.
author
#15 lore raw

PATCH 1/7 spi: altera: Use struct-based register access

Quoted 34 lines · Jagan Teki
Quoted 22 lines · Marek Vasut

On Monday, October 20, 2014 at 04:53:15 PM, Jagan Teki wrote:

[...]

Quoted 9 lines
Quoted 9 lines
drivers/spi/altera_spi.c +4 -5
-#define ALTERA_SPI_RXDATA      0
-#define ALTERA_SPI_TXDATA      4
-#define ALTERA_SPI_STATUS      8
-#define ALTERA_SPI_CONTROL     12
-#define ALTERA_SPI_SLAVE_SEL   20
+struct altera_spi_regs {
+       u32     rxdata;
+       u32     txdata;
+       u32     status;
Quoted 8 lines

drivers/spi/altera_spi.c line 22

Quoted 4 lines
drivers/spi/altera_spi.c +4 -0
+       u32     control;
+       u32     _reserved;
+       u32     slave_sel;
+};

Can you place this structure definition below of all macro defines, i don't think the
next level patches does that, does they?

Quoted 3 lines

Does it make sense to you, to first define the bits in registers and then the register layout ? It does not make sense to me, so I would prefer to keep it like it is.

You're correct the way you replaced, usually the driver code will go like this

-- >includes

--> macros definitions

Quoted 7 lines

--> global or structure definitions

--> driver function calls.

We follow this [1] to make the driver more readable.

[1] http://patchwork.ozlabs.org/patch/265683/

I'm not sure what I am supposed to follow in the link above. Is it fine to assume that this patch does not need any change then ?

Best regards,
Marek Vasut