public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access
@ 2014-10-19 18:43 Marek Vasut
  2014-10-19 18:43 ` [U-Boot] [PATCH 2/7] spi: altera: Clean up bit definitions Marek Vasut
                   ` (7 more replies)
  0 siblings, 8 replies; 18+ messages in thread
From: Marek Vasut @ 2014-10-19 18:43 UTC (permalink / raw)
  To: u-boot

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 <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@altera.com>
Cc: Albert Aribaud <albert.u.boot@aribaud.net>
Cc: Tom Rini <trini@ti.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Pavel Machek <pavel@denx.de>
Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
---
 drivers/spi/altera_spi.c | 49 ++++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/drivers/spi/altera_spi.c b/drivers/spi/altera_spi.c
index 5accbb5..13191f3 100644
--- a/drivers/spi/altera_spi.c
+++ b/drivers/spi/altera_spi.c
@@ -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);
-- 
2.1.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/7] spi: altera: Clean up bit definitions
  2014-10-19 18:43 [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access Marek Vasut
@ 2014-10-19 18:43 ` Marek Vasut
  2014-10-19 18:43 ` [U-Boot] [PATCH 3/7] spi: altera: Clean up most checkpatch issues Marek Vasut
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2014-10-19 18:43 UTC (permalink / raw)
  To: u-boot

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 <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@altera.com>
Cc: Albert Aribaud <albert.u.boot@aribaud.net>
Cc: Tom Rini <trini@ti.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Pavel Machek <pavel@denx.de>
Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
---
 drivers/spi/altera_spi.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/altera_spi.c b/drivers/spi/altera_spi.c
index 13191f3..21f90fc 100644
--- a/drivers/spi/altera_spi.c
+++ b/drivers/spi/altera_spi.c
@@ -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 }
-- 
2.1.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 3/7] spi: altera: Clean up most checkpatch issues
  2014-10-19 18:43 [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access Marek Vasut
  2014-10-19 18:43 ` [U-Boot] [PATCH 2/7] spi: altera: Clean up bit definitions Marek Vasut
@ 2014-10-19 18:43 ` Marek Vasut
  2014-10-19 18:43 ` [U-Boot] [PATCH 4/7] spi: altera: Zap endless loop Marek Vasut
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2014-10-19 18:43 UTC (permalink / raw)
  To: u-boot

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 <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@altera.com>
Cc: Albert Aribaud <albert.u.boot@aribaud.net>
Cc: Tom Rini <trini@ti.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Pavel Machek <pavel@denx.de>
Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
---
 drivers/spi/altera_spi.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/altera_spi.c b/drivers/spi/altera_spi.c
index 21f90fc..373ce30 100644
--- a/drivers/spi/altera_spi.c
+++ b/drivers/spi/altera_spi.c
@@ -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);
 
-- 
2.1.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 4/7] spi: altera: Zap endless loop
  2014-10-19 18:43 [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access Marek Vasut
  2014-10-19 18:43 ` [U-Boot] [PATCH 2/7] spi: altera: Clean up bit definitions Marek Vasut
  2014-10-19 18:43 ` [U-Boot] [PATCH 3/7] spi: altera: Clean up most checkpatch issues Marek Vasut
@ 2014-10-19 18:43 ` Marek Vasut
  2014-10-20 15:03   ` Jagan Teki
  2014-10-19 18:43 ` [U-Boot] [PATCH 5/7] spi: altera: Clean up the use of variable d Marek Vasut
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Marek Vasut @ 2014-10-19 18:43 UTC (permalink / raw)
  To: u-boot

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 <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@altera.com>
Cc: Albert Aribaud <albert.u.boot@aribaud.net>
Cc: Tom Rini <trini@ti.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Pavel Machek <pavel@denx.de>
Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
---
 drivers/spi/altera_spi.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/altera_spi.c b/drivers/spi/altera_spi.c
index 373ce30..ee65ec2 100644
--- a/drivers/spi/altera_spi.c
+++ b/drivers/spi/altera_spi.c
@@ -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)
-- 
2.1.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 5/7] spi: altera: Clean up the use of variable d
  2014-10-19 18:43 [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access Marek Vasut
                   ` (2 preceding siblings ...)
  2014-10-19 18:43 ` [U-Boot] [PATCH 4/7] spi: altera: Zap endless loop Marek Vasut
@ 2014-10-19 18:43 ` Marek Vasut
  2014-10-19 18:43 ` [U-Boot] [PATCH 6/7] spi: altera: Add short note about EPCS/EPCQx1 Marek Vasut
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2014-10-19 18:43 UTC (permalink / raw)
  To: u-boot

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 <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@altera.com>
Cc: Albert Aribaud <albert.u.boot@aribaud.net>
Cc: Tom Rini <trini@ti.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Pavel Machek <pavel@denx.de>
Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
---
 drivers/spi/altera_spi.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/altera_spi.c b/drivers/spi/altera_spi.c
index ee65ec2..3065e96 100644
--- a/drivers/spi/altera_spi.c
+++ b/drivers/spi/altera_spi.c
@@ -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:
-- 
2.1.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 6/7] spi: altera: Add short note about EPCS/EPCQx1
  2014-10-19 18:43 [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access Marek Vasut
                   ` (3 preceding siblings ...)
  2014-10-19 18:43 ` [U-Boot] [PATCH 5/7] spi: altera: Clean up the use of variable d Marek Vasut
@ 2014-10-19 18:43 ` Marek Vasut
  2014-10-20 15:10   ` Jagan Teki
  2014-10-19 18:43 ` [U-Boot] [PATCH 7/7] spi: altera: Move the config options to the top Marek Vasut
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Marek Vasut @ 2014-10-19 18:43 UTC (permalink / raw)
  To: u-boot

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 <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@altera.com>
Cc: Albert Aribaud <albert.u.boot@aribaud.net>
Cc: Tom Rini <trini@ti.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Pavel Machek <pavel@denx.de>
Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
---
 drivers/spi/altera_spi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/spi/altera_spi.c b/drivers/spi/altera_spi.c
index 3065e96..0566e4f 100644
--- a/drivers/spi/altera_spi.c
+++ b/drivers/spi/altera_spi.c
@@ -4,6 +4,14 @@
  * based on bfin_spi.c
  * Copyright (c) 2005-2008 Analog Devices Inc.
  * Copyright (C) 2010 Thomas Chou <thomas@wytron.com.tw>
+ * Copyright (C) 2014 Marek Vasut <marex@denx.de>
+ *
+ * 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+
  */
-- 
2.1.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 7/7] spi: altera: Move the config options to the top
  2014-10-19 18:43 [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access Marek Vasut
                   ` (4 preceding siblings ...)
  2014-10-19 18:43 ` [U-Boot] [PATCH 6/7] spi: altera: Add short note about EPCS/EPCQx1 Marek Vasut
@ 2014-10-19 18:43 ` Marek Vasut
  2014-10-20  7:36 ` [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access Pavel Machek
  2014-10-20 14:53 ` Jagan Teki
  7 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2014-10-19 18:43 UTC (permalink / raw)
  To: u-boot

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 <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@altera.com>
Cc: Albert Aribaud <albert.u.boot@aribaud.net>
Cc: Tom Rini <trini@ti.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Pavel Machek <pavel@denx.de>
Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
---
 drivers/spi/altera_spi.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/altera_spi.c b/drivers/spi/altera_spi.c
index 0566e4f..f3a6fc9 100644
--- a/drivers/spi/altera_spi.c
+++ b/drivers/spi/altera_spi.c
@@ -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)
 {
-- 
2.1.1

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access
  2014-10-19 18:43 [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access Marek Vasut
                   ` (5 preceding siblings ...)
  2014-10-19 18:43 ` [U-Boot] [PATCH 7/7] spi: altera: Move the config options to the top Marek Vasut
@ 2014-10-20  7:36 ` Pavel Machek
  2014-10-20 14:53 ` Jagan Teki
  7 siblings, 0 replies; 18+ messages in thread
From: Pavel Machek @ 2014-10-20  7:36 UTC (permalink / raw)
  To: u-boot

On Sun 2014-10-19 20:43:33, Marek Vasut wrote:
> 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 <pavel@denx.de>

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access
  2014-10-19 18:43 [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access Marek Vasut
                   ` (6 preceding siblings ...)
  2014-10-20  7:36 ` [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access Pavel Machek
@ 2014-10-20 14:53 ` Jagan Teki
  2014-10-20 15:10   ` Marek Vasut
  7 siblings, 1 reply; 18+ messages in thread
From: Jagan Teki @ 2014-10-20 14:53 UTC (permalink / raw)
  To: u-boot

On 20 October 2014 00:13, Marek Vasut <marex@denx.de> wrote:
> 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 <marex@denx.de>
> Cc: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@altera.com>
> Cc: Albert Aribaud <albert.u.boot@aribaud.net>
> Cc: Tom Rini <trini@ti.com>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
> ---
>  drivers/spi/altera_spi.c | 49 ++++++++++++++++++++++++------------------------
>  1 file changed, 25 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/spi/altera_spi.c b/drivers/spi/altera_spi.c
> index 5accbb5..13191f3 100644
> --- a/drivers/spi/altera_spi.c
> +++ b/drivers/spi/altera_spi.c
> @@ -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;
> +};

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

>
>  #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);
> --
> 2.1.1
>

thanks!
-- 
Jagan.

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 4/7] spi: altera: Zap endless loop
  2014-10-19 18:43 ` [U-Boot] [PATCH 4/7] spi: altera: Zap endless loop Marek Vasut
@ 2014-10-20 15:03   ` Jagan Teki
  0 siblings, 0 replies; 18+ messages in thread
From: Jagan Teki @ 2014-10-20 15:03 UTC (permalink / raw)
  To: u-boot

On 20 October 2014 00:13, Marek Vasut <marex@denx.de> wrote:
> 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 <marex@denx.de>
> Cc: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@altera.com>
> Cc: Albert Aribaud <albert.u.boot@aribaud.net>
> Cc: Tom Rini <trini@ti.com>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
> ---
>  drivers/spi/altera_spi.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/altera_spi.c b/drivers/spi/altera_spi.c
> index 373ce30..ee65ec2 100644
> --- a/drivers/spi/altera_spi.c
> +++ b/drivers/spi/altera_spi.c
> @@ -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.

> +       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;
> +               }

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.

>
>                 d = readl(&altspi->regs->rxdata);
>                 if (rxp)
> --
> 2.1.1
>

thanks!
-- 
Jagan.

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 6/7] spi: altera: Add short note about EPCS/EPCQx1
  2014-10-19 18:43 ` [U-Boot] [PATCH 6/7] spi: altera: Add short note about EPCS/EPCQx1 Marek Vasut
@ 2014-10-20 15:10   ` Jagan Teki
  2014-10-20 15:13     ` Marek Vasut
  0 siblings, 1 reply; 18+ messages in thread
From: Jagan Teki @ 2014-10-20 15:10 UTC (permalink / raw)
  To: u-boot

On 20 October 2014 00:13, Marek Vasut <marex@denx.de> wrote:
> 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 <marex@denx.de>
> Cc: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@altera.com>
> Cc: Albert Aribaud <albert.u.boot@aribaud.net>
> Cc: Tom Rini <trini@ti.com>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
> ---
>  drivers/spi/altera_spi.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/spi/altera_spi.c b/drivers/spi/altera_spi.c
> index 3065e96..0566e4f 100644
> --- a/drivers/spi/altera_spi.c
> +++ b/drivers/spi/altera_spi.c
> @@ -4,6 +4,14 @@
>   * based on bfin_spi.c
>   * Copyright (c) 2005-2008 Analog Devices Inc.
>   * Copyright (C) 2010 Thomas Chou <thomas@wytron.com.tw>
> + * Copyright (C) 2014 Marek Vasut <marex@denx.de>

Looks not good to me - with few changes.

> + *
> + * 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.

>   *
>   * SPDX-License-Identifier:    GPL-2.0+
>   */
> --
> 2.1.1
>

thanks!
-- 
Jagan.

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access
  2014-10-20 14:53 ` Jagan Teki
@ 2014-10-20 15:10   ` Marek Vasut
  2014-10-20 17:19     ` Jagan Teki
  0 siblings, 1 reply; 18+ messages in thread
From: Marek Vasut @ 2014-10-20 15:10 UTC (permalink / raw)
  To: u-boot

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

[...]

> > -#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;
> > +};
> 
> 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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 6/7] spi: altera: Add short note about EPCS/EPCQx1
  2014-10-20 15:10   ` Jagan Teki
@ 2014-10-20 15:13     ` Marek Vasut
  2014-10-20 17:27       ` Jagan Teki
  0 siblings, 1 reply; 18+ messages in thread
From: Marek Vasut @ 2014-10-20 15:13 UTC (permalink / raw)
  To: u-boot

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

[...]

> >   * based on bfin_spi.c
> >   * Copyright (c) 2005-2008 Analog Devices Inc.
> >   * Copyright (C) 2010 Thomas Chou <thomas@wytron.com.tw>
> > 
> > + * Copyright (C) 2014 Marek Vasut <marex@denx.de>
> 
> Looks not good to me - with few changes.
> 
> > + *
> > + * 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.

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

Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access
  2014-10-20 15:10   ` Marek Vasut
@ 2014-10-20 17:19     ` Jagan Teki
  2014-10-21 22:15       ` Marek Vasut
  0 siblings, 1 reply; 18+ messages in thread
From: Jagan Teki @ 2014-10-20 17:19 UTC (permalink / raw)
  To: u-boot

On 20 October 2014 20:40, Marek Vasut <marex@denx.de> wrote:
> On Monday, October 20, 2014 at 04:53:15 PM, Jagan Teki wrote:
>
> [...]
>
>> > -#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;
>> > +};
>>
>> 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.

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!
-- 
Jagan.

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 6/7] spi: altera: Add short note about EPCS/EPCQx1
  2014-10-20 15:13     ` Marek Vasut
@ 2014-10-20 17:27       ` Jagan Teki
  0 siblings, 0 replies; 18+ messages in thread
From: Jagan Teki @ 2014-10-20 17:27 UTC (permalink / raw)
  To: u-boot

On 20 October 2014 20:43, Marek Vasut <marex@denx.de> wrote:
> On Monday, October 20, 2014 at 05:10:17 PM, Jagan Teki wrote:
>
> [...]
>
>> >   * based on bfin_spi.c
>> >   * Copyright (c) 2005-2008 Analog Devices Inc.
>> >   * Copyright (C) 2010 Thomas Chou <thomas@wytron.com.tw>
>> >
>> > + * Copyright (C) 2014 Marek Vasut <marex@denx.de>
>>
>> Looks not good to me - with few changes.
>>
>> > + *
>> > + * 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.
>
> 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!
-- 
Jagan.

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access
  2014-10-20 17:19     ` Jagan Teki
@ 2014-10-21 22:15       ` Marek Vasut
  0 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2014-10-21 22:15 UTC (permalink / raw)
  To: u-boot

On Monday, October 20, 2014 at 07:19:33 PM, Jagan Teki wrote:
> On 20 October 2014 20:40, Marek Vasut <marex@denx.de> wrote:
> > On Monday, October 20, 2014 at 04:53:15 PM, Jagan Teki wrote:
> > 
> > [...]
> > 
> >> > -#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;
> >> > +};
> >> 
> >> 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.
> 
> 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/

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access
@ 2014-10-22 19:55 Marek Vasut
  2014-10-23 14:45 ` Pavel Machek
  0 siblings, 1 reply; 18+ messages in thread
From: Marek Vasut @ 2014-10-22 19:55 UTC (permalink / raw)
  To: u-boot

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 <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@altera.com>
Cc: Albert Aribaud <albert.u.boot@aribaud.net>
Cc: Pavel Machek <pavel@denx.de>
Cc: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
---
 drivers/spi/altera_spi.c | 49 ++++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/drivers/spi/altera_spi.c b/drivers/spi/altera_spi.c
index 5accbb5..13191f3 100644
--- a/drivers/spi/altera_spi.c
+++ b/drivers/spi/altera_spi.c
@@ -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);
-- 
2.0.0

^ permalink raw reply related	[flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access
  2014-10-22 19:55 Marek Vasut
@ 2014-10-23 14:45 ` Pavel Machek
  0 siblings, 0 replies; 18+ messages in thread
From: Pavel Machek @ 2014-10-23 14:45 UTC (permalink / raw)
  To: u-boot

On Wed 2014-10-22 21:55:58, Marek Vasut wrote:
> 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 1-7:

Acked-by: Pavel Machek <pavel@denx.de>

									Pavel

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

^ permalink raw reply	[flat|nested] 18+ messages in thread
end of thread, other threads:[~2014-10-23 14:45 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-19 18:43 [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access Marek Vasut
2014-10-19 18:43 ` [U-Boot] [PATCH 2/7] spi: altera: Clean up bit definitions Marek Vasut
2014-10-19 18:43 ` [U-Boot] [PATCH 3/7] spi: altera: Clean up most checkpatch issues Marek Vasut
2014-10-19 18:43 ` [U-Boot] [PATCH 4/7] spi: altera: Zap endless loop Marek Vasut
2014-10-20 15:03   ` Jagan Teki
2014-10-19 18:43 ` [U-Boot] [PATCH 5/7] spi: altera: Clean up the use of variable d Marek Vasut
2014-10-19 18:43 ` [U-Boot] [PATCH 6/7] spi: altera: Add short note about EPCS/EPCQx1 Marek Vasut
2014-10-20 15:10   ` Jagan Teki
2014-10-20 15:13     ` Marek Vasut
2014-10-20 17:27       ` Jagan Teki
2014-10-19 18:43 ` [U-Boot] [PATCH 7/7] spi: altera: Move the config options to the top Marek Vasut
2014-10-20  7:36 ` [U-Boot] [PATCH 1/7] spi: altera: Use struct-based register access Pavel Machek
2014-10-20 14:53 ` Jagan Teki
2014-10-20 15:10   ` Marek Vasut
2014-10-20 17:19     ` Jagan Teki
2014-10-21 22:15       ` Marek Vasut
  -- strict thread matches above, loose matches on Subject: below --
2014-10-22 19:55 Marek Vasut
2014-10-23 14:45 ` Pavel Machek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox