[U-Boot] [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 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);
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 }
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);
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)
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!
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:
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+ */
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!
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
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!
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) {
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
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!
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
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!
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.
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
participants (3)
-
Jagan Teki -
Marek Vasut -
Pavel Machek