ARM: Add spi controller driver support for NUC900
Subsystems (3)
PATCH 1/1 ARM: Add spi controller driver support for NUC900
Dear David,
Add winbond/nuvoton NUC900 spi controller driver support, on my evaluation board,there is a winbond w25x16 spi flash, so I test my spi controller driver with m25p80.c.
--- arch/arm/mach-w90x900/include/mach/nuc900_spi.h | 35 ++ drivers/spi/Kconfig | 7 + drivers/spi/Makefile | 1 + drivers/spi/spi_nuc900.c | 458 +++++++++++++++++++++++ 4 files changed, 501 insertions(+), 0 deletions(-) create mode 100644 arch/arm/mach-w90x900/include/mach/nuc900_spi.h create mode 100644 drivers/spi/spi_nuc900.c
arch/arm/mach-w90x900/include/mach/nuc900_spi.h new file +35 -0
@@ -0,0 +1,35 @@ +/* +* arch/arm/mach-w90x900/include/mach/nuc900_spi.h +* +* Copyright (c) 2009 Nuvoton technology corporation. +* +* Wan ZongShun <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation;version 2 of the License. +* +*/ + +#ifndef __ASM_ARCH_SPI_H +#define __ASM_ARCH_SPI_H + +extern void mfp_set_groupg (struct device *dev); + +struct w90p910_spi_info { +unsigned int num_cs; +unsigned int lsb; +unsigned int txneg; +unsigned int rxneg; +unsigned int divider; +unsigned int sleep; +unsigned int txnum; +unsigned int txbitlen; +int bus_num; +}; + +struct w90p910_spi_chip { +unsigned char bits_per_word; +}; + +#endif /* __ASM_ARCH_SPI_H */
drivers/spi/Kconfig +7 -0
@@ -244,6 +244,13 @@ config SPI_XILINX See the "OPB Serial Peripheral Interface (SPI) (v1.00e)" Product Specification document (DS464) for hardware details. +config SPI_NUC900 + tristate "Nuvoton NUC900 series SPI" + depends on ARCH_W90X900 && EXPERIMENTAL + select SPI_BITBANG + help + SPI driver for Nuvoton NUC900 series ARM SoCs + # # Add new SPI master controllers in alphabetical order above this line #
drivers/spi/Makefile +1 -0
@@ -33,6 +33,7 @@ obj-$(CONFIG_SPI_TXX9) += spi_txx9.o obj-$(CONFIG_SPI_XILINX) += xilinx_spi.o obj-$(CONFIG_SPI_SH_SCI) += spi_sh_sci.o obj-$(CONFIG_SPI_STMP3XXX) += spi_stmp.o +obj-$(CONFIG_SPI_NUC900) += spi_nuc900.o # ... add above this line ... # SPI protocol drivers (device/link on bus)
drivers/spi/spi_nuc900.c new file +458 -0
458 lines not on this page · raw message
Signature · 1 line
1.5.6.3
Quoted 9 lines · On Tue, 17 Nov 2009 14:48:40 +0800 Wan ZongShun <[email protected]> wrote:
Dear David,
Add winbond/nuvoton NUC900 spi controller driver support, on my evaluation board,there is a winbond w25x16 spi flash, so I test my spi controller driver with
m25p80.c....
drivers/spi/spi_nuc900.c line 71
Quoted 6 lines
drivers/spi/spi_nuc900.c new file +6
-0+static inline struct w90p910_spi *to_hw (struct spi_device *sdev) +{ +return spi_master_get_devdata (sdev->master); +} + +static void w90p910_slave_seclect (struct spi_device *spi,unsigned int ssr)
I think you meant "select" here?
Quoted 19 lines
drivers/spi/spi_nuc900.c new file +19
-0+{ +struct w90p910_spi *hw =to_hw (spi); +unsigned int val; +unsigned int cs = spi->mode &SPI_CS_HIGH ?1 :0 ; +unsigned int cpol = spi->mode &SPI_CPOL ?1 :0 ; + + val =__raw_readl (hw->regs +USI_SSR ); + +if (!cs) + val &= ~SELECTLEV ; +else + val |=SELECTLEV ; + +if (!ssr) + val &= ~SELECTSLAVE ; +else + val |=SELECTSLAVE ; + +__raw_writel (val, hw->regs +USI_SSR );
drivers/spi/spi_nuc900.c line 100
Quoted 10 lines
drivers/spi/spi_nuc900.c new file +10
-0+ + val =__raw_readl (hw->regs +USI_CNT ); + +if (!cpol) + val &= ~SELECTPOL ; +else + val |=SELECTPOL ; + +__raw_writel (val, hw->regs +USI_CNT ); +}
That's a read-modify-write operation. What locking prevents two threads of control from altering the USI_SSR and USI_CNT registers at the same time, resulting in an indeterminate setting?
Quoted 41 lines
drivers/spi/spi_nuc900.c new file +41
-0+static void w90p910_spi_chipsel (struct spi_device *spi,int value) +{ +switch (value) { +case BITBANG_CS_INACTIVE : +w90p910_slave_seclect (spi,0 ); +break ; + +case BITBANG_CS_ACTIVE : +w90p910_slave_seclect (spi,1 ); +break ; + } +} + +static void w90p910_spi_setup_txnum (struct w90p910_spi *hw, +unsigned int txnum) +{ +unsigned int val; + + val =__raw_readl (hw->regs +USI_CNT ); + +if (!txnum) + val &= ~TXNUM ; +else + val |= txnum <<0x08 ; + +__raw_writel (val, hw->regs +USI_CNT ); + +} + +static void w90p910_spi_setup_txbitlen (struct w90p910_spi *hw, +unsigned int txbitlen) +{ +unsigned int val; + + val =__raw_readl (hw->regs +USI_CNT ); + + val |= (txbitlen <<0x03 ); + +__raw_writel (val, hw->regs +USI_CNT ); +} +
drivers/spi/spi_nuc900.c line 152
Quoted 10 lines
drivers/spi/spi_nuc900.c new file +10
-0+static void w90p910_spi_gobusy (struct w90p910_spi *hw) +{ +unsigned int val; + + val =__raw_readl (hw->regs +USI_CNT ); + + val |=GOBUSY ; + +__raw_writel (val, hw->regs +USI_CNT ); +}
ditto, ditto, ditto.
Quoted 25 lines
drivers/spi/spi_nuc900.c new file +25
-0+static int w90p910_spi_setupxfer (struct spi_device *spi, +struct spi_transfer *t) +{ +return 0 ; +} + +static int w90p910_spi_setup (struct spi_device *spi) +{ +return 0 ; +} + +static inline unsigned int hw_txbyte (struct w90p910_spi *hw,int count) +{ +return hw->tx ? hw->tx[count] :0 ; +} + +static int w90p910_spi_txrx (struct spi_device *spi,struct spi_transfer *t) +{ +struct w90p910_spi *hw =to_hw (spi); + + hw->tx = t->tx_buf; + hw->rx = t->rx_buf; + hw->len = t->len; + hw->count =0 ; +
drivers/spi/spi_nuc900.c line 188
Quoted 10 lines
drivers/spi/spi_nuc900.c new file +10
-0+init_completion (&hw->done); + +__raw_writel (hw_txbyte (hw,0x0 ), hw->regs +USI_TX0 ); + +w90p910_spi_gobusy (hw); + +wait_for_completion (&hw->done); + +return hw->count; +}
The init_completion() should be unneeded? The structure was initialised at setup time and will be left in a reusable state after a complete()/wait_for_completion(). Reinitialising the structure all the time like this adds risk that it will be scribbled on while in use.
drivers/spi/spi_nuc900.c line 421
Quoted 13 lines
...
drivers/spi/spi_nuc900.c new file +10
-0+static int __devexit w90p910_spi_remove(struct platform_device *dev) +{ +struct w90p910_spi *hw =platform_get_drvdata (dev); + +platform_set_drvdata (dev,NULL ); + +spi_unregister_master (hw->master); + +clk_disable (hw->clk); +clk_put (hw->clk);
As far as I can tell, a hardware interrupt could still be pending, or be under service while the above code is executing?
If so, I expect bad things will happen?
Quoted 13 lines
drivers/spi/spi_nuc900.c new file +10
-0+free_irq (hw->irq, hw); +iounmap (hw->regs); + +release_resource (hw->ioarea); +kfree (hw->ioarea); + +spi_master_put (hw->master); +return 0 ; +} +
Dear Andrew,
Thanks a lot for your help, and I have a question below.
2009/11/19 Andrew Morton <[email protected]>:
Quoted 167 lines
Quoted 9 lines · On Tue, 17 Nov 2009 14:48:40 +0800 Wan ZongShun <[email protected]> wrote:
Dear David,
Add winbond/nuvoton NUC900 spi controller driver support, on my evaluation board,there is a winbond w25x16 spi flash, so I test my spi controller driver with
m25p80.c....
drivers/spi/spi_nuc900.c line 71
Quoted 6 lines
drivers/spi/spi_nuc900.c new file +6
-0+static inline struct w90p910_spi *to_hw (struct spi_device *sdev) +{ + return spi_master_get_devdata(sdev->master); +} + +static void w90p910_slave_seclect (struct spi_device *spi,unsigned int ssr)I think you meant "select" here?
Quoted 19 lines
drivers/spi/spi_nuc900.c new file +19
-0+{ + struct w90p910_spi *hw = to_hw(spi); + unsigned int val; + unsigned int cs = spi->mode & SPI_CS_HIGH ? 1 : 0; + unsigned int cpol = spi->mode & SPI_CPOL ? 1 : 0; + + val = __raw_readl(hw->regs + USI_SSR); + + if (!cs) + val &= ~SELECTLEV; + else + val |= SELECTLEV; + + if (!ssr) + val &= ~SELECTSLAVE; + else + val |= SELECTSLAVE; + + __raw_writel(val, hw->regs + USI_SSR);drivers/spi/spi_nuc900.c line 100
Quoted 10 lines
drivers/spi/spi_nuc900.c new file +10
-0+ + val = __raw_readl(hw->regs + USI_CNT); + + if (!cpol) + val &= ~SELECTPOL; + else + val |= SELECTPOL; + + __raw_writel(val, hw->regs + USI_CNT); +}That's a read-modify-write operation. What locking prevents two threads of control from altering the
USI_SSRandUSI_CNTregisters at the same time, resulting in an indeterminate setting?Quoted 41 lines
drivers/spi/spi_nuc900.c new file +41
-0+static void w90p910_spi_chipsel (struct spi_device *spi,int value) +{ + switch (value) { + case BITBANG_CS_INACTIVE: + w90p910_slave_seclect(spi, 0); + break; + + case BITBANG_CS_ACTIVE: + w90p910_slave_seclect(spi, 1); + break; + } +} + +static void w90p910_spi_setup_txnum (struct w90p910_spi *hw, + unsigned int txnum) +{ + unsigned int val; + + val = __raw_readl(hw->regs + USI_CNT); + + if (!txnum) + val &= ~TXNUM; + else + val |= txnum << 0x08; + + __raw_writel(val, hw->regs + USI_CNT); + +} + +static void w90p910_spi_setup_txbitlen (struct w90p910_spi *hw, + unsigned int txbitlen) +{ + unsigned int val; + + val = __raw_readl(hw->regs + USI_CNT); + + val |= (txbitlen << 0x03); + + __raw_writel(val, hw->regs + USI_CNT); +} +drivers/spi/spi_nuc900.c line 152
Quoted 10 lines
drivers/spi/spi_nuc900.c new file +10
-0+static void w90p910_spi_gobusy (struct w90p910_spi *hw) +{ + unsigned int val; + + val = __raw_readl(hw->regs + USI_CNT); + + val |= GOBUSY; + + __raw_writel(val, hw->regs + USI_CNT); +}ditto, ditto, ditto.
Quoted 25 lines
drivers/spi/spi_nuc900.c new file +25
-0+static int w90p910_spi_setupxfer (struct spi_device *spi, + struct spi_transfer *t) +{ + return 0; +} + +static int w90p910_spi_setup (struct spi_device *spi) +{ + return 0; +} + +static inline unsigned int hw_txbyte (struct w90p910_spi *hw,int count) +{ + return hw->tx ? hw->tx[count] : 0; +} + +static int w90p910_spi_txrx (struct spi_device *spi,struct spi_transfer *t) +{ + struct w90p910_spi *hw = to_hw(spi); + + hw->tx = t->tx_buf; + hw->rx = t->rx_buf; + hw->len = t->len; + hw->count = 0; +drivers/spi/spi_nuc900.c line 188
Quoted 10 lines
drivers/spi/spi_nuc900.c new file +10
-0+ init_completion(&hw->done); + + __raw_writel(hw_txbyte(hw, 0x0), hw->regs + USI_TX0); + + w90p910_spi_gobusy(hw); + + wait_for_completion(&hw->done); + + return hw->count; +}The
init_completion()should be unneeded? The structure was initialised at setup time and will be left in a reusable state after a complete()/wait_for_completion(). Reinitialising the structure all the time like this adds risk that it will be scribbled on while in use.drivers/spi/spi_nuc900.c line 421
Quoted 13 lines
...
drivers/spi/spi_nuc900.c new file +10
-0+static int __devexit w90p910_spi_remove(struct platform_device *dev) +{ + struct w90p910_spi *hw = platform_get_drvdata(dev); + + platform_set_drvdata(dev, NULL); + + spi_unregister_master(hw->master); + + clk_disable(hw->clk); + clk_put(hw->clk);As far as I can tell, a hardware interrupt could still be pending, or be under service while the above code is executing?
Quoted 1 line
If so, I expect bad things will happen?
Do you mean that I should put this 'free_irq()' in the front of w90p910_spi_remove？
such as:
"free_irq(hw->irq, hw);
platform_set_drvdata(dev, NULL);
spi_unregister_master(hw->master);
clk_disable(hw->clk);clk_put(hw->clk);
"
Quoted 15 lines
Quoted 13 lines
+
free_irq(hw->irq, hw);
+iounmap(hw->regs);drivers/spi/spi_nuc900.c new file +8
-0+ + release_resource(hw->ioarea); + kfree(hw->ioarea); + + spi_master_put(hw->master); + return 0; +} +
Signature · 3 lines
linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 23 lines · On Thu, 19 Nov 2009 14:23:49 +0800 Wan ZongShun <[email protected]> wrote:
Quoted 14 lines
drivers/spi/spi_nuc900.c
Quoted 10 lines
drivers/spi/spi_nuc900.c new file +10
-0+static int __devexit w90p910_spi_remove(struct platform_device *dev) +{ + __ __ struct w90p910_spi *hw = platform_get_drvdata(dev); + + __ __ platform_set_drvdata(dev, NULL); + + __ __ spi_unregister_master(hw->master); + + __ __ clk_disable(hw->clk); + __ __ clk_put(hw->clk);As far as I can tell, a hardware interrupt could still be pending, or be under service while the above code is executing?
Quoted 1 line
If so, I expect bad things will happen?
Do you mean that I should put this '
free_irq()' in the front ofw90p910_spi_remove___such as:
"free_irq(hw->irq, hw);
Quoted 6 lines
platform_set_drvdata(dev, NULL);
spi_unregister_master(hw->master);
clk_disable(hw->clk);clk_put(hw->clk);
I don't know, because I don't know what operation the hardware needs to stop it from generating interrupts. Perhaps that's clk_disable()?
Once you've stopped the source of interrupts then the code should wait for the IRQ handler to complete if it's running on another CPU. Yes, free_irq() does that.
It's only after the clk_disable() and the free_irq() that you can guarantee that no interrupt handler will run and attempt to access the device and its associated data structures.
2009/11/19 Andrew Morton <[email protected]>:
Quoted 27 lines · On Thu, 19 Nov 2009 14:23:49 +0800 Wan ZongShun <[email protected]> wrote:
Quoted 26 lines
Quoted 15 lines
drivers/spi/spi_nuc900.c
Quoted 11 lines
drivers/spi/spi_nuc900.c new file +10
-0+static int __devexit w90p910_spi_remove(struct platform_device *dev) +{ + __ __ struct w90p910_spi *hw = platform_get_drvdata(dev); + + __ __ platform_set_drvdata(dev, NULL); + + __ __ spi_unregister_master(hw->master); + + __ __ clk_disable(hw->clk); + __ __ clk_put(hw->clk);As far as I can tell, a hardware interrupt could still be pending, or be under service while the above code is executing?
Quoted 1 line
If so, I expect bad things will happen?
Do you mean that I should put this '
free_irq()' in the front ofw90p910_spi_remove___such as:
"free_irq(hw->irq, hw);
platform_set_drvdata(dev, NULL);
Quoted 7 lines
Quoted 4 lines
spi_unregister_master(hw->master);
clk_disable(hw->clk);clk_put(hw->clk);I don't know, because I don't know what operation the hardware needs to stop it from generating interrupts. Perhaps that's
clk_disable()?
The interrupt will be not occur as long as I run clk_disable().
Quoted 3 lines
Once you've stopped the source of interrupts then the code should wait for the IRQ handler to complete if it's running on another CPU. Yes,
free_irq()does that.
So, regarding my system of single CPU, maybe I need put this 'clk_disable()' in the front of function of w90p910_spi_remove().
right?
Quoted 5 lines
It's only after the
clk_disable()and thefree_irq()that you can guarantee that no interrupt handler will run and attempt to access the device and its associated data structures.
Signature · 3 lines
linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 6 lines · Wan ZongShun
2009/11/19 Andrew Morton <[email protected]>:
Quoted 2 lines
I don't know, because I don't know what operation the hardware needs to stop it from generating interrupts. ?Perhaps that's
clk_disable()?The interrupt will be not occur as long as I run
clk_disable().
Quoted 8 lines
Quoted 3 lines
Once you've stopped the source of interrupts then the code should wait for the IRQ handler to complete if it's running on another CPU. ?Yes,
free_irq()does that.So, regarding my system of single CPU, maybe I need put this '
clk_disable()' in the front of function ofw90p910_spi_remove().right?
Depending on the hardware, that's not the right answer. If turning off the clock also causes register accesses to the device to abort, it is a very dangerous thing to do.
It can also be dangerous if the clock is used to synchronise the interrupt output - it can lead to the output being permanently asserted if the clock is turned off with it asserted.
Normally devices have an "interrupt enable" register. You should disable all interrupts from the device using this register after unregistering the driver from the (SPI) subsystem.
2009/11/19 Russell King - ARM Linux <[email protected]>:
Quoted 20 lines · Wan ZongShun
Quoted 6 lines
2009/11/19 Andrew Morton <[email protected]>:
Quoted 2 lines
I don't know, because I don't know what operation the hardware needs to stop it from generating interrupts. Perhaps that's
clk_disable()?The interrupt will be not occur as long as I run
clk_disable().Quoted 8 lines
Quoted 3 lines
Once you've stopped the source of interrupts then the code should wait for the IRQ handler to complete if it's running on another CPU. Yes,
free_irq()does that.So, regarding my system of single CPU, maybe I need put this '
clk_disable()' in the front of function ofw90p910_spi_remove().right?
Depending on the hardware, that's not the right answer. If turning off the clock also causes register accesses to the device to abort, it is a very dangerous thing to do.
Quoted 7 lines
It can also be dangerous if the clock is used to synchronise the interrupt output - it can lead to the output being permanently asserted if the clock is turned off with it asserted.
Normally devices have an "interrupt enable" register. You should disable all interrupts from the device using this register after unregistering the driver from the (SPI) subsystem.
sir, so I need implement a API to disable spi device interrupt by "interrupt enable" register?
but I did not find the operation in other ARM platform when they close your spi driver.
or something I missed?
Quoted 1 line
Signature · 3 lines
linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 29 lines · Wan ZongShun
2009/11/19 Russell King - ARM Linux <[email protected]>:
Quoted 20 lines · Wan ZongShun
Quoted 6 lines
2009/11/19 Andrew Morton <[email protected]>:
Quoted 2 lines
I don't know, because I don't know what operation the hardware needs to stop it from generating interrupts. ?Perhaps that's
clk_disable()?The interrupt will be not occur as long as I run
clk_disable().Quoted 8 lines
Quoted 3 lines
Once you've stopped the source of interrupts then the code should wait for the IRQ handler to complete if it's running on another CPU. ?Yes,
free_irq()does that.So, regarding my system of single CPU, maybe I need put this '
clk_disable()' in the front of function ofw90p910_spi_remove().right?
Depending on the hardware, that's not the right answer. ?If turning off the clock also causes register accesses to the device to abort, it is a very dangerous thing to do.
Quoted 7 lines
It can also be dangerous if the clock is used to synchronise the interrupt output - it can lead to the output being permanently asserted if the clock is turned off with it asserted.
Normally devices have an "interrupt enable" register. ?You should disable all interrupts from the device using this register after unregistering the driver from the (SPI) subsystem.
Quoted 6 lines
sir, so I need implement a API to disable spi device interrupt by "interrupt enable" register?
but I did not find the operation in other ARM platform when they close your spi driver.or something I missed?
No, I think you need to reverse the effects of w90p910_enable_int() by clearing the ENINT bit. I'm making the assumption that doing so prevents interrupts being generated by the device.