diff mbox

[OpenWrt-Devel,2/8] ramips: remove rt2880 spi lock and clean bit operation

Message ID 1444313768-23970-2-git-send-email-igvtee@gmail.com
State Accepted
Headers show

Commit Message

Mingyu Li Oct. 8, 2015, 2:16 p.m. UTC
Signed-off-by: Michael Lee <igvtee@gmail.com>
---
 ...0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch | 23 +++++-----------------
 1 file changed, 5 insertions(+), 18 deletions(-)

Comments

John Crispin Oct. 26, 2015, 2:26 p.m. UTC | #1
Hi Michael,

while merging the series i stumbled across this patch. to me this looks
wrong.

rather than removing the locking i think we should add more to
rt2880_spi_read and rt2880_spi_write.

i'll merge the other 7 patches just now if they apply without 2/8


	John

On 08/10/2015 16:16, Michael Lee wrote:
> Signed-off-by: Michael Lee <igvtee@gmail.com>
> ---
>  ...0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch | 23 +++++-----------------
>  1 file changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch b/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch
> index ca04a17..d6a462c 100644
> --- a/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch
> +++ b/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch
> @@ -41,7 +41,7 @@ Acked-by: John Crispin <blogic@openwrt.org>
>   spi-s3c24xx-hw-$(CONFIG_SPI_S3C24XX_FIQ) += spi-s3c24xx-fiq.o
>  --- /dev/null
>  +++ b/drivers/spi/spi-rt2880.c
> -@@ -0,0 +1,493 @@
> +@@ -0,0 +1,480 @@
>  +/*
>  + * spi-rt2880.c -- Ralink RT288x/RT305x SPI controller driver
>  + *
> @@ -174,7 +174,6 @@ Acked-by: John Crispin <blogic@openwrt.org>
>  +	unsigned int		sys_freq;
>  +	unsigned int		speed;
>  +	struct clk		*clk;
> -+	spinlock_t		lock;
>  +};
>  +
>  +static inline struct rt2880_spi *spidev_to_rt2880_spi(struct spi_device *spi)
> @@ -187,7 +186,8 @@ Acked-by: John Crispin <blogic@openwrt.org>
>  +	return ioread32(rs->base + reg);
>  +}
>  +
> -+static inline void rt2880_spi_write(struct rt2880_spi *rs, u32 reg, u32 val)
> ++static inline void rt2880_spi_write(struct rt2880_spi *rs, u32 reg,
> ++		const u32 val)
>  +{
>  +	iowrite32(val, rs->base + reg);
>  +}
> @@ -195,27 +195,15 @@ Acked-by: John Crispin <blogic@openwrt.org>
>  +static inline void rt2880_spi_setbits(struct rt2880_spi *rs, u32 reg, u32 mask)
>  +{
>  +	void __iomem *addr = rs->base + reg;
> -+	unsigned long flags;
> -+	u32 val;
>  +
> -+	spin_lock_irqsave(&rs->lock, flags);
> -+	val = ioread32(addr);
> -+	val |= mask;
> -+	iowrite32(val, addr);
> -+	spin_unlock_irqrestore(&rs->lock, flags);
> ++	iowrite32((ioread32(addr) | mask), addr);
>  +}
>  +
>  +static inline void rt2880_spi_clrbits(struct rt2880_spi *rs, u32 reg, u32 mask)
>  +{
>  +	void __iomem *addr = rs->base + reg;
> -+	unsigned long flags;
> -+	u32 val;
>  +
> -+	spin_lock_irqsave(&rs->lock, flags);
> -+	val = ioread32(addr);
> -+	val &= ~mask;
> -+	iowrite32(val, addr);
> -+	spin_unlock_irqrestore(&rs->lock, flags);
> ++	iowrite32((ioread32(addr) & ~mask), addr);
>  +}
>  +
>  +static int rt2880_spi_baudrate_set(struct spi_device *spi, unsigned int speed)
> @@ -488,7 +476,6 @@ Acked-by: John Crispin <blogic@openwrt.org>
>  +	rs->master = master;
>  +	rs->sys_freq = clk_get_rate(rs->clk);
>  +	dev_dbg(&pdev->dev, "sys_freq: %u\n", rs->sys_freq);
> -+	spin_lock_irqsave(&rs->lock, flags);
>  +
>  +	device_reset(&pdev->dev);
>  +
>
Mingyu Li Oct. 26, 2015, 4:09 p.m. UTC | #2
Hi John.

i check the code which functions need read/write register.
3 functions (prepare_message, set_cs, transfer_one) are used
at same function __spi_pump_messages at spi.c.

the __spi_pump_message is protected by spi_master->queue_lock
this make sure spi device operation is serialized.

the spi bus is protected by spi_master->bus_lock_spinlock. you can
found it at spi_async function. it make sure only one spi device work
on the same spi bus.
so the 3 functions don't have lock problem.

about the setup function. it maybe be called by user space to setup
a new spi device. but it will not affect other spi device. because it just
disable the new spi device by useing cs pin.

so i think it is save to remove the lock protect.

the most important is the lock on probe function must be wrong.
because it only call spin_lock_irqsave. but not call spin_unlock_irqrestore.

2015-10-26 22:26 GMT+08:00 John Crispin <blogic@openwrt.org>:

> Hi Michael,
>
> while merging the series i stumbled across this patch. to me this looks
> wrong.
>
> rather than removing the locking i think we should add more to
> rt2880_spi_read and rt2880_spi_write.
>
> i'll merge the other 7 patches just now if they apply without 2/8
>
>
>         John
>
> On 08/10/2015 16:16, Michael Lee wrote:
> > Signed-off-by: Michael Lee <igvtee@gmail.com>
> > ---
> >  ...0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch | 23
> +++++-----------------
> >  1 file changed, 5 insertions(+), 18 deletions(-)
> >
> > diff --git
> a/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch
> b/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch
> > index ca04a17..d6a462c 100644
> > ---
> a/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch
> > +++
> b/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch
> > @@ -41,7 +41,7 @@ Acked-by: John Crispin <blogic@openwrt.org>
> >   spi-s3c24xx-hw-$(CONFIG_SPI_S3C24XX_FIQ) += spi-s3c24xx-fiq.o
> >  --- /dev/null
> >  +++ b/drivers/spi/spi-rt2880.c
> > -@@ -0,0 +1,493 @@
> > +@@ -0,0 +1,480 @@
> >  +/*
> >  + * spi-rt2880.c -- Ralink RT288x/RT305x SPI controller driver
> >  + *
> > @@ -174,7 +174,6 @@ Acked-by: John Crispin <blogic@openwrt.org>
> >  +    unsigned int            sys_freq;
> >  +    unsigned int            speed;
> >  +    struct clk              *clk;
> > -+    spinlock_t              lock;
> >  +};
> >  +
> >  +static inline struct rt2880_spi *spidev_to_rt2880_spi(struct
> spi_device *spi)
> > @@ -187,7 +186,8 @@ Acked-by: John Crispin <blogic@openwrt.org>
> >  +    return ioread32(rs->base + reg);
> >  +}
> >  +
> > -+static inline void rt2880_spi_write(struct rt2880_spi *rs, u32 reg,
> u32 val)
> > ++static inline void rt2880_spi_write(struct rt2880_spi *rs, u32 reg,
> > ++            const u32 val)
> >  +{
> >  +    iowrite32(val, rs->base + reg);
> >  +}
> > @@ -195,27 +195,15 @@ Acked-by: John Crispin <blogic@openwrt.org>
> >  +static inline void rt2880_spi_setbits(struct rt2880_spi *rs, u32 reg,
> u32 mask)
> >  +{
> >  +    void __iomem *addr = rs->base + reg;
> > -+    unsigned long flags;
> > -+    u32 val;
> >  +
> > -+    spin_lock_irqsave(&rs->lock, flags);
> > -+    val = ioread32(addr);
> > -+    val |= mask;
> > -+    iowrite32(val, addr);
> > -+    spin_unlock_irqrestore(&rs->lock, flags);
> > ++    iowrite32((ioread32(addr) | mask), addr);
> >  +}
> >  +
> >  +static inline void rt2880_spi_clrbits(struct rt2880_spi *rs, u32 reg,
> u32 mask)
> >  +{
> >  +    void __iomem *addr = rs->base + reg;
> > -+    unsigned long flags;
> > -+    u32 val;
> >  +
> > -+    spin_lock_irqsave(&rs->lock, flags);
> > -+    val = ioread32(addr);
> > -+    val &= ~mask;
> > -+    iowrite32(val, addr);
> > -+    spin_unlock_irqrestore(&rs->lock, flags);
> > ++    iowrite32((ioread32(addr) & ~mask), addr);
> >  +}
> >  +
> >  +static int rt2880_spi_baudrate_set(struct spi_device *spi, unsigned
> int speed)
> > @@ -488,7 +476,6 @@ Acked-by: John Crispin <blogic@openwrt.org>
> >  +    rs->master = master;
> >  +    rs->sys_freq = clk_get_rate(rs->clk);
> >  +    dev_dbg(&pdev->dev, "sys_freq: %u\n", rs->sys_freq);
> > -+    spin_lock_irqsave(&rs->lock, flags);
> >  +
> >  +    device_reset(&pdev->dev);
> >  +
> >
>
diff mbox

Patch

diff --git a/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch b/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch
index ca04a17..d6a462c 100644
--- a/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch
+++ b/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch
@@ -41,7 +41,7 @@  Acked-by: John Crispin <blogic@openwrt.org>
  spi-s3c24xx-hw-$(CONFIG_SPI_S3C24XX_FIQ) += spi-s3c24xx-fiq.o
 --- /dev/null
 +++ b/drivers/spi/spi-rt2880.c
-@@ -0,0 +1,493 @@
+@@ -0,0 +1,480 @@
 +/*
 + * spi-rt2880.c -- Ralink RT288x/RT305x SPI controller driver
 + *
@@ -174,7 +174,6 @@  Acked-by: John Crispin <blogic@openwrt.org>
 +	unsigned int		sys_freq;
 +	unsigned int		speed;
 +	struct clk		*clk;
-+	spinlock_t		lock;
 +};
 +
 +static inline struct rt2880_spi *spidev_to_rt2880_spi(struct spi_device *spi)
@@ -187,7 +186,8 @@  Acked-by: John Crispin <blogic@openwrt.org>
 +	return ioread32(rs->base + reg);
 +}
 +
-+static inline void rt2880_spi_write(struct rt2880_spi *rs, u32 reg, u32 val)
++static inline void rt2880_spi_write(struct rt2880_spi *rs, u32 reg,
++		const u32 val)
 +{
 +	iowrite32(val, rs->base + reg);
 +}
@@ -195,27 +195,15 @@  Acked-by: John Crispin <blogic@openwrt.org>
 +static inline void rt2880_spi_setbits(struct rt2880_spi *rs, u32 reg, u32 mask)
 +{
 +	void __iomem *addr = rs->base + reg;
-+	unsigned long flags;
-+	u32 val;
 +
-+	spin_lock_irqsave(&rs->lock, flags);
-+	val = ioread32(addr);
-+	val |= mask;
-+	iowrite32(val, addr);
-+	spin_unlock_irqrestore(&rs->lock, flags);
++	iowrite32((ioread32(addr) | mask), addr);
 +}
 +
 +static inline void rt2880_spi_clrbits(struct rt2880_spi *rs, u32 reg, u32 mask)
 +{
 +	void __iomem *addr = rs->base + reg;
-+	unsigned long flags;
-+	u32 val;
 +
-+	spin_lock_irqsave(&rs->lock, flags);
-+	val = ioread32(addr);
-+	val &= ~mask;
-+	iowrite32(val, addr);
-+	spin_unlock_irqrestore(&rs->lock, flags);
++	iowrite32((ioread32(addr) & ~mask), addr);
 +}
 +
 +static int rt2880_spi_baudrate_set(struct spi_device *spi, unsigned int speed)
@@ -488,7 +476,6 @@  Acked-by: John Crispin <blogic@openwrt.org>
 +	rs->master = master;
 +	rs->sys_freq = clk_get_rate(rs->clk);
 +	dev_dbg(&pdev->dev, "sys_freq: %u\n", rs->sys_freq);
-+	spin_lock_irqsave(&rs->lock, flags);
 +
 +	device_reset(&pdev->dev);
 +