[PATCH] mtd: dc21285.c: handle nw_gpio_lock correctly

[PATCH] mtd: dc21285.c: handle nw_gpio_lock correctly

[Christian Dietrich]

nw_gpio_lock is a raw_spinlock_t, therefore raw_spin_lock_irqsave should be
used. it doesn't make a difference, while rlock is the first element of
spinlock_t.

Also the check for machine_is_netwinder() is a double check of
CONFIG_ARCH_NETWINDER.

Signed-off-by: Christian Dietrich <christian.dietrich@informatik.uni-erlangen.de>
---
 drivers/mtd/maps/dc21285.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/maps/dc21285.c b/drivers/mtd/maps/dc21285.c
index 080f060..38fbf23 100644
--- a/drivers/mtd/maps/dc21285.c
+++ b/drivers/mtd/maps/dc21285.c
@@ -38,9 +38,9 @@ static void nw_en_write(void)
 	 * we want to write a bit pattern XXX1 to Xilinx to enable
 	 * the write gate, which will be open for about the next 2ms.
 	 */
-	spin_lock_irqsave(&nw_gpio_lock, flags);
+	raw_spin_lock_irqsave(&nw_gpio_lock, flags);
 	nw_cpld_modify(CPLD_FLASH_WR_ENABLE, CPLD_FLASH_WR_ENABLE);
-	spin_unlock_irqrestore(&nw_gpio_lock, flags);
+	raw_spin_unlock_irqrestore(&nw_gpio_lock, flags);
 
 	/*
 	 * let the ISA bus to catch on...
@@ -79,8 +79,7 @@ static void dc21285_copy_from(struct map_info *map, void *to, unsigned long from
 
 static void dc21285_write8(struct map_info *map, const map_word d, unsigned long adr)
 {
-	if (machine_is_netwinder())
-		nw_en_write();
+	nw_en_write();
 	*CSR_ROMWRITEREG = adr & 3;
 	adr &= ~3;
 	*(uint8_t*)(map->virt + adr) = d.x[0];
@@ -88,8 +87,7 @@ static void dc21285_write8(struct map_info *map, const map_word d, unsigned long
 
 static void dc21285_write16(struct map_info *map, const map_word d, unsigned long adr)
 {
-	if (machine_is_netwinder())
-		nw_en_write();
+	nw_en_write();
 	*CSR_ROMWRITEREG = adr & 3;
 	adr &= ~3;
 	*(uint16_t*)(map->virt + adr) = d.x[0];
@@ -97,8 +95,7 @@ static void dc21285_write16(struct map_info *map, const map_word d, unsigned lon
 
 static void dc21285_write32(struct map_info *map, const map_word d, unsigned long adr)
 {
-	if (machine_is_netwinder())
-		nw_en_write();
+	nw_en_write();
 	*(uint32_t*)(map->virt + adr) = d.x[0];
 }
 
-- 
1.7.5.4

Re: [PATCH] mtd: dc21285.c: handle nw_gpio_lock correctly

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1662 bytes --]

On Fri, 2012-05-25 at 10:28 +0200, Christian Dietrich wrote:
> nw_gpio_lock is a raw_spinlock_t, therefore raw_spin_lock_irqsave should be
> used. it doesn't make a difference, while rlock is the first element of
> spinlock_t.
> 
> Also the check for machine_is_netwinder() is a double check of
> CONFIG_ARCH_NETWINDER.
> 
> Signed-off-by: Christian Dietrich <christian.dietrich@informatik.uni-erlangen.de>

I do not understand this commit message. Please, split your patch on 2
patches and put a better to each one. Thanks!

> ---
>  drivers/mtd/maps/dc21285.c |   13 +++++--------
>  1 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/maps/dc21285.c b/drivers/mtd/maps/dc21285.c
> index 080f060..38fbf23 100644
> --- a/drivers/mtd/maps/dc21285.c
> +++ b/drivers/mtd/maps/dc21285.c
> @@ -38,9 +38,9 @@ static void nw_en_write(void)
>  	 * we want to write a bit pattern XXX1 to Xilinx to enable
>  	 * the write gate, which will be open for about the next 2ms.
>  	 */
> -	spin_lock_irqsave(&nw_gpio_lock, flags);
> +	raw_spin_lock_irqsave(&nw_gpio_lock, flags);

Please, make this to be a separate patch.

And surely there are many other places in the kernel which need this
conversion?

>  static void dc21285_write8(struct map_info *map, const map_word d, unsigned long adr)
>  {
> -	if (machine_is_netwinder())
> -		nw_en_write();
> +	nw_en_write();
>  	*CSR_ROMWRITEREG = adr & 3;

I do not understand why this "if" statement is not needed? Why it was
there in the first place and why you remove it? Please, describe this in
the commit message.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] netwinder: nw_gpio_lock is a raw_spinlock_t

[Christian Dietrich]

Since nw_gpio_lock is a raw_spinlock_t it should be used with the
raw_spinlock_* functions and not the spinlock_* variants. Functionally
this is equivalent at the moment, because the raw_spinlock_t is the
first field of spinlock_t, and therefore &nw_gpio_lock ==
&(nw_gpio_lock->rlock). But when other spinlock_t functions use other
field they read and write random memory.

Signed-off-by: Christian Dietrich <christian.dietrich@informatik.uni-erlangen.de>
---
 drivers/char/ds1620.c      |    8 ++++----
 drivers/char/nwflash.c     |    4 ++--
 drivers/mtd/maps/dc21285.c |    4 ++--
 sound/oss/waveartist.c     |    4 ++--
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/char/ds1620.c b/drivers/char/ds1620.c
index aab9605..7d86139 100644
--- a/drivers/char/ds1620.c
+++ b/drivers/char/ds1620.c
@@ -74,21 +74,21 @@ static inline void netwinder_ds1620_reset(void)
 
 static inline void netwinder_lock(unsigned long *flags)
 {
-	spin_lock_irqsave(&nw_gpio_lock, *flags);
+	raw_spin_lock_irqsave(&nw_gpio_lock, *flags);
 }
 
 static inline void netwinder_unlock(unsigned long *flags)
 {
-	spin_unlock_irqrestore(&nw_gpio_lock, *flags);
+	raw_spin_unlock_irqrestore(&nw_gpio_lock, *flags);
 }
 
 static inline void netwinder_set_fan(int i)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&nw_gpio_lock, flags);
+	netwinder_lock(&flags);
 	nw_gpio_modify_op(GPIO_FAN, i ? GPIO_FAN : 0);
-	spin_unlock_irqrestore(&nw_gpio_lock, flags);
+	netwinder_unlock(&flags);
 }
 
 static inline int netwinder_get_fan(void)
diff --git a/drivers/char/nwflash.c b/drivers/char/nwflash.c
index d45c334..04e2a94 100644
--- a/drivers/char/nwflash.c
+++ b/drivers/char/nwflash.c
@@ -617,9 +617,9 @@ static void kick_open(void)
 	 * we want to write a bit pattern XXX1 to Xilinx to enable
 	 * the write gate, which will be open for about the next 2ms.
 	 */
-	spin_lock_irqsave(&nw_gpio_lock, flags);
+	raw_spin_lock_irqsave(&nw_gpio_lock, flags);
 	nw_cpld_modify(CPLD_FLASH_WR_ENABLE, CPLD_FLASH_WR_ENABLE);
-	spin_unlock_irqrestore(&nw_gpio_lock, flags);
+	raw_spin_unlock_irqrestore(&nw_gpio_lock, flags);
 
 	/*
 	 * let the ISA bus to catch on...
diff --git a/drivers/mtd/maps/dc21285.c b/drivers/mtd/maps/dc21285.c
index 080f060..86598a1 100644
--- a/drivers/mtd/maps/dc21285.c
+++ b/drivers/mtd/maps/dc21285.c
@@ -38,9 +38,9 @@ static void nw_en_write(void)
 	 * we want to write a bit pattern XXX1 to Xilinx to enable
 	 * the write gate, which will be open for about the next 2ms.
 	 */
-	spin_lock_irqsave(&nw_gpio_lock, flags);
+	raw_spin_lock_irqsave(&nw_gpio_lock, flags);
 	nw_cpld_modify(CPLD_FLASH_WR_ENABLE, CPLD_FLASH_WR_ENABLE);
-	spin_unlock_irqrestore(&nw_gpio_lock, flags);
+	raw_spin_unlock_irqrestore(&nw_gpio_lock, flags);
 
 	/*
 	 * let the ISA bus to catch on...
diff --git a/sound/oss/waveartist.c b/sound/oss/waveartist.c
index 24c430f..672af8b 100644
--- a/sound/oss/waveartist.c
+++ b/sound/oss/waveartist.c
@@ -1482,9 +1482,9 @@ vnc_mute_spkr(wavnc_info *devc)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&nw_gpio_lock, flags);
+	raw_spin_lock_irqsave(&nw_gpio_lock, flags);
 	nw_cpld_modify(CPLD_UNMUTE, devc->spkr_mute_state ? 0 : CPLD_UNMUTE);
-	spin_unlock_irqrestore(&nw_gpio_lock, flags);
+	raw_spin_unlock_irqrestore(&nw_gpio_lock, flags);
 }
 
 static void
-- 
1.7.5.4

Re: [PATCH] netwinder: nw_gpio_lock is a raw_spinlock_t

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On Tue, 2012-05-29 at 12:06 +0200, Christian Dietrich wrote:
> Since nw_gpio_lock is a raw_spinlock_t it should be used with the
> raw_spinlock_* functions and not the spinlock_* variants. Functionally
> this is equivalent at the moment, because the raw_spinlock_t is the
> first field of spinlock_t, and therefore &nw_gpio_lock ==
> &(nw_gpio_lock->rlock). But when other spinlock_t functions use other
> field they read and write random memory.

Hm, why are we exposing a raw spinlock to drivers? 

Should we export a helper function (or macro, I suppose) which does the
appropriate locking *and* the GPIO operation?

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] netwinder: nw_gpio_lock is a raw_spinlock_t

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]

On Tue, 2012-05-29 at 11:11 +0100, David Woodhouse wrote:
> On Tue, 2012-05-29 at 12:06 +0200, Christian Dietrich wrote:
> > Since nw_gpio_lock is a raw_spinlock_t it should be used with the
> > raw_spinlock_* functions and not the spinlock_* variants. Functionally
> > this is equivalent at the moment, because the raw_spinlock_t is the
> > first field of spinlock_t, and therefore &nw_gpio_lock ==
> > &(nw_gpio_lock->rlock). But when other spinlock_t functions use other
> > field they read and write random memory.
> 
> Hm, why are we exposing a raw spinlock to drivers? 
> 
> Should we export a helper function (or macro, I suppose) which does the
> appropriate locking *and* the GPIO operation?

AFAIR, raw spinlock is the one that will not be turned into a
"preemptable" spinlock in the RT tree. E.g., this is needed when dealing
with interrupts. And what if not drivers should use them?

But this commit message did not explain still (although I requested)
_why_ it needs a raw spinlock, which problems it solves?

This URL may be useful for Christian:

http://who-t.blogspot.com/2009/12/on-commit-messages.html

I like that blog-post.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] netwinder: encapsulate CPLD hardware locking and access

[Christian Dietrich]

In order to not use the raw_spinlock_t nw_gpio_lock all over the
kernel, the locking of the GPIO ports is done by using the
nw_gpio_{un,}lock functions.

The write-gate is only open for about 2ms after it was
enabled. Therefore it is necessary to disable the preemption during a
series of writes to prevent the kernel from preempting the writing
task. The nw_cpld_enable_write function opens the write gate and
disables preemption, and the nw_cpld_disable_write function enables
the preemption again, the write gate closes automatically.

Signed-off-by: Christian Dietrich <christian.dietrich@informatik.uni-erlangen.de>
---
 arch/arm/mach-footbridge/include/mach/hardware.h |   19 ++++-
 arch/arm/mach-footbridge/netwinder-hw.c          |   53 +++++++++++--
 arch/arm/mach-footbridge/netwinder-leds.c        |    4 +-
 drivers/char/ds1620.c                            |   21 ++----
 drivers/char/nwflash.c                           |   95 ++++++++++++++-------
 drivers/mtd/maps/dc21285.c                       |   46 ++++-------
 sound/oss/waveartist.c                           |    4 +-
 7 files changed, 153 insertions(+), 89 deletions(-)

David Woodhouse <dwmw2@infradead.org> writes:

> Hm, why are we exposing a raw spinlock to drivers? 
>
> Should we export a helper function (or macro, I suppose) which does the
> appropriate locking *and* the GPIO operation?

The locking shouldn't be put into every modification operation, because
sometimes operations are supposed to be done in the same critical
region. The DS1620 driver is such an example.

> If you were to make these functions public by shifting them into
> arch/arm/mach-footbridge/include/mach/hardware.h that would be a lot
> nicer, and other places wouldn't have to touch the raw spinlock
> directly.

I put locking primitives into the mentioned header file. I didn't make
the nw_gpio_lock module static, since i don't know it there is any third
party code touching this gpio lock, and i didn't want to break the API.

> Also... while we're thinking about preemption and netwinder, note that
> the write enable is valid for only 2ms or so. So all the functions in
> dc21285.c that you just touched should probably *also* be disabling
> preemption when they're run on a netwinder, to ensure that that time
> doesn't expire before they actually get to run.

I hope my draft of the preemption disabling is not to much crap. I would
appreciate some comments. And please be aware, that i don't own this
hardware, and haven't tested it on real hardware therefore.

diff --git a/arch/arm/mach-footbridge/include/mach/hardware.h b/arch/arm/mach-footbridge/include/mach/hardware.h
index e3d6cca..642d5c4 100644
--- a/arch/arm/mach-footbridge/include/mach/hardware.h
+++ b/arch/arm/mach-footbridge/include/mach/hardware.h
@@ -93,11 +93,28 @@
 #define CPLD_FLASH_WR_ENABLE	1
 
 #ifndef __ASSEMBLY__
+#ifdef CONFIG_ARCH_NETWINDER
 extern raw_spinlock_t nw_gpio_lock;
 extern void nw_gpio_modify_op(unsigned int mask, unsigned int set);
 extern void nw_gpio_modify_io(unsigned int mask, unsigned int in);
 extern unsigned int nw_gpio_read(void);
 extern void nw_cpld_modify(unsigned int mask, unsigned int set);
-#endif
+extern void nw_cpld_enable_write(void);
+extern void nw_cpld_disable_write(void);
+
+static inline void nw_cpld_lock(unsigned long *flags)
+{
+	raw_spin_lock_irqsave(&nw_gpio_lock, *flags);
+}
+
+static inline void nw_cpld_unlock(unsigned long *flags)
+{
+	raw_spin_unlock_irqrestore(&nw_gpio_lock, *flags);
+}
 
+#else
+#define nw_cpld_enable_write() do {} while(0)
+#define nw_cpld_disable_write() do {} while(0)
+#endif
+#endif
 #endif
diff --git a/arch/arm/mach-footbridge/netwinder-hw.c b/arch/arm/mach-footbridge/netwinder-hw.c
index cac9f67..380f763 100644
--- a/arch/arm/mach-footbridge/netwinder-hw.c
+++ b/arch/arm/mach-footbridge/netwinder-hw.c
@@ -328,9 +328,9 @@ static inline void wb977_init_gpio(void)
 	/*
 	 * Set Group1/Group2 outputs
 	 */
-	raw_spin_lock_irqsave(&nw_gpio_lock, flags);
+	nw_cpld_lock(&flags);
 	nw_gpio_modify_op(-1, GPIO_RED_LED | GPIO_FAN);
-	raw_spin_unlock_irqrestore(&nw_gpio_lock, flags);
+	nw_cpld_unlock(&flags);
 }
 
 /*
@@ -387,13 +387,54 @@ void nw_cpld_modify(unsigned int mask, unsigned int set)
 }
 EXPORT_SYMBOL(nw_cpld_modify);
 
+/*
+ * This is really ugly, but it seams to be the only
+ * realiable way to do it, as the cpld state machine
+ * is unpredictible. So we have a 25us penalty per
+ * write access.
+ */
+void nw_cpld_enable_write(void)
+{
+	unsigned long flags;
+
+	/*
+	 * we want to write a bit pattern XXX1 to Xilinx to enable
+	 * the write gate, which will be open for about the next 2ms.
+	 * Because the write gate is just opened for 2ms, it is
+	 * necessary to prevent the kernel from preempting this
+	 * task.
+	 */
+	nw_cpld_lock(&flags);
+
+	/* Increment the preempt counter after we got the lock */
+	preempt_disable();
+
+	nw_cpld_modify(CPLD_FLASH_WR_ENABLE, CPLD_FLASH_WR_ENABLE);
+	nw_cpld_unlock(&flags);
+
+	/*
+	 * let the ISA bus to catch on...
+	 */
+	udelay(25);
+}
+
+void nw_cpld_disable_write(void)
+{
+	/* Since we disabled preemption in nw_cpld_enable_write, we
+	 * have to enable it again after the critical write section. The
+	 * hardware will close the write gate automatically after 2ms.
+	 */
+	preempt_enable();
+}
+
+
 static void __init cpld_init(void)
 {
 	unsigned long flags;
 
-	raw_spin_lock_irqsave(&nw_gpio_lock, flags);
+	nw_cpld_lock(&flags);
 	nw_cpld_modify(-1, CPLD_UNMUTE | CPLD_7111_DISABLE);
-	raw_spin_unlock_irqrestore(&nw_gpio_lock, flags);
+	nw_cpld_unlock(&flags);
 }
 
 static unsigned char rwa_unlock[] __initdata =
@@ -617,9 +658,9 @@ static int __init nw_hw_init(void)
 		cpld_init();
 		rwa010_init();
 
-		raw_spin_lock_irqsave(&nw_gpio_lock, flags);
+		nw_cpld_lock(&flags);
 		nw_gpio_modify_op(GPIO_RED_LED|GPIO_GREEN_LED, DEFAULT_LEDS);
-		raw_spin_unlock_irqrestore(&nw_gpio_lock, flags);
+		nw_cpld_unlock(&flags);
 	}
 	return 0;
 }
diff --git a/arch/arm/mach-footbridge/netwinder-leds.c b/arch/arm/mach-footbridge/netwinder-leds.c
index 5a2bd89..61e6282 100644
--- a/arch/arm/mach-footbridge/netwinder-leds.c
+++ b/arch/arm/mach-footbridge/netwinder-leds.c
@@ -119,9 +119,9 @@ static void netwinder_leds_event(led_event_t evt)
 	raw_spin_unlock_irqrestore(&leds_lock, flags);
 
 	if  (led_state & LED_STATE_ENABLED) {
-		raw_spin_lock_irqsave(&nw_gpio_lock, flags);
+		nw_cpld_lock(&flags);
 		nw_gpio_modify_op(GPIO_RED_LED | GPIO_GREEN_LED, hw_led_state);
-		raw_spin_unlock_irqrestore(&nw_gpio_lock, flags);
+		nw_cpld_unlock(&flags);
 	}
 }
 
diff --git a/drivers/char/ds1620.c b/drivers/char/ds1620.c
index aab9605..d302ea5 100644
--- a/drivers/char/ds1620.c
+++ b/drivers/char/ds1620.c
@@ -72,23 +72,14 @@ static inline void netwinder_ds1620_reset(void)
 	nw_cpld_modify(CPLD_DS_ENABLE, CPLD_DS_ENABLE);
 }
 
-static inline void netwinder_lock(unsigned long *flags)
-{
-	spin_lock_irqsave(&nw_gpio_lock, *flags);
-}
-
-static inline void netwinder_unlock(unsigned long *flags)
-{
-	spin_unlock_irqrestore(&nw_gpio_lock, *flags);
-}
 
 static inline void netwinder_set_fan(int i)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&nw_gpio_lock, flags);
+	nw_cpld_lock(&flags);
 	nw_gpio_modify_op(GPIO_FAN, i ? GPIO_FAN : 0);
-	spin_unlock_irqrestore(&nw_gpio_lock, flags);
+	nw_cpld_unlock(&flags);
 }
 
 static inline int netwinder_get_fan(void)
@@ -145,7 +136,7 @@ static void ds1620_out(int cmd, int bits, int value)
 {
 	unsigned long flags;
 
-	netwinder_lock(&flags);
+	nw_cpld_lock(&flags);
 	netwinder_ds1620_set_clk(1);
 	netwinder_ds1620_set_data_dir(0);
 	netwinder_ds1620_reset();
@@ -159,7 +150,7 @@ static void ds1620_out(int cmd, int bits, int value)
 	udelay(1);
 
 	netwinder_ds1620_reset();
-	netwinder_unlock(&flags);
+	nw_cpld_unlock(&flags);
 
 	msleep(20);
 }
@@ -169,7 +160,7 @@ static unsigned int ds1620_in(int cmd, int bits)
 	unsigned long flags;
 	unsigned int value;
 
-	netwinder_lock(&flags);
+	nw_cpld_lock(&flags);
 	netwinder_ds1620_set_clk(1);
 	netwinder_ds1620_set_data_dir(0);
 	netwinder_ds1620_reset();
@@ -182,7 +173,7 @@ static unsigned int ds1620_in(int cmd, int bits)
 	value = ds1620_recv_bits(bits);
 
 	netwinder_ds1620_reset();
-	netwinder_unlock(&flags);
+	nw_cpld_unlock(&flags);
 
 	return value;
 }
diff --git a/drivers/char/nwflash.c b/drivers/char/nwflash.c
index d45c334..f007eec 100644
--- a/drivers/char/nwflash.c
+++ b/drivers/char/nwflash.c
@@ -40,7 +40,6 @@
 #define	NWFLASH_VERSION "6.4"
 
 static DEFINE_MUTEX(flash_mutex);
-static void kick_open(void);
 static int get_flash_id(void);
 static int erase_block(int nBlock);
 static int write_block(unsigned long p, const char __user *buf, int count);
@@ -65,7 +64,7 @@ static int get_flash_id(void)
 	/*
 	 * try to get flash chip ID
 	 */
-	kick_open();
+	nw_cpld_enable_write();
 	c2 = inb(0x80);
 	*(volatile unsigned char *) (FLASH_BASE + 0x8000) = 0x90;
 	udelay(15);
@@ -90,6 +89,8 @@ static int get_flash_id(void)
 	if (c2 == KFLASH_ID4)
 		gbFlashSize = KFLASH_SIZE4;
 
+	nw_cpld_disable_write();
+
 	return c2;
 }
 
@@ -167,7 +168,7 @@ static ssize_t flash_write(struct file *file, const char __user *buf,
 
 	if (count > gbFlashSize - p)
 		count = gbFlashSize - p;
-			
+
 	if (!access_ok(VERIFY_READ, buf, count))
 		return -EFAULT;
 
@@ -348,7 +349,12 @@ static int erase_block(int nBlock)
 	 */
 	c1 = *(volatile unsigned char *) (FLASH_BASE + 0x8000);
 
-	kick_open();
+	/*
+	 * Enable the write gate and disable preemption. The write
+	 * gate is open for 2ms.
+	 */
+	nw_cpld_enable_write();
+
 	/*
 	 * reset status if old errors
 	 */
@@ -364,7 +370,6 @@ static int erase_block(int nBlock)
 	 */
 	c1 = *pWritePtr;
 
-	kick_open();
 	/*
 	 * erase
 	 */
@@ -376,6 +381,11 @@ static int erase_block(int nBlock)
 	*(volatile unsigned char *) pWritePtr = 0xD0;
 
 	/*
+	 * Reenable preemption again, before we go to sleep
+	 */
+	nw_cpld_disable_write();
+
+	/*
 	 * wait 10 ms
 	 */
 	msleep(10);
@@ -395,9 +405,14 @@ static int erase_block(int nBlock)
 	}
 
 	/*
+	 * Enable the write gate and disable preemption. The write
+	 * gate is open for 2ms.
+	 */
+	nw_cpld_enable_write();
+
+	/*
 	 * set flash for normal read access
 	 */
-	kick_open();
 //      *(volatile unsigned char*)(FLASH_BASE+0x8000) = 0xFF;
 	*(volatile unsigned char *) pWritePtr = 0xFF;	//back to normal operation
 
@@ -415,6 +430,11 @@ static int erase_block(int nBlock)
 	}
 
 	/*
+	 * Reenable preemption again, before we go to sleep
+	 */
+	nw_cpld_disable_write();
+
+	/*
 	 * just to make sure - verify if erased OK...
 	 */
 	msleep(10);
@@ -480,9 +500,10 @@ static int write_block(unsigned long p, const char __user *buf, int count)
 		c1 = *(volatile unsigned char *) (FLASH_BASE + 0x8000);
 
 		/*
-		 * kick open the write gate
+		 * Enable the write gate and disable preemption. The write
+		 * gate is open for 2ms.
 		 */
-		kick_open();
+		nw_cpld_enable_write();
 
 		/*
 		 * program footbridge to the correct offset...0..3
@@ -504,8 +525,12 @@ static int write_block(unsigned long p, const char __user *buf, int count)
 		 */
 		*(volatile unsigned char *) (FLASH_BASE + 0x10000) = 0x70;
 
-		c1 = 0;
+		/*
+		 * Reenable preemption again, before we go to sleep
+		 */
+		nw_cpld_disable_write();
 
+		c1 = 0;
 		/*
 		 * wait up to 1 sec for this byte
 		 */
@@ -521,35 +546,55 @@ static int write_block(unsigned long p, const char __user *buf, int count)
 		 * if timeout getting status
 		 */
 		if (time_after_eq(jiffies, timeout1)) {
-			kick_open();
+			/*
+			 * Enable the write gate and disable preemption. The write
+			 * gate is open for 2ms.
+			 */
+			nw_cpld_enable_write();
+
 			/*
 			 * reset err
 			 */
 			*(volatile unsigned char *) (FLASH_BASE + 0x8000) = 0x50;
 
+			/*
+			 * Reenable preemption again, before we go to sleep
+			 */
+			nw_cpld_disable_write();
+
+
 			goto WriteRetry;
 		}
 		/*
 		 * switch on read access, as a default flash operation mode
 		 */
-		kick_open();
+		/*
+		 * Enable the write gate and disable preemption. The write
+		 * gate is open for 2ms.
+		 */
+		nw_cpld_enable_write();
+
 		/*
 		 * read access
 		 */
 		*(volatile unsigned char *) (FLASH_BASE + 0x8000) = 0xFF;
 
 		/*
-		 * if hardware reports an error writing, and not timeout - 
+		 * if hardware reports an error writing, and not timeout -
 		 * reset the chip and retry
 		 */
 		if (c1 & 0x10) {
-			kick_open();
 			/*
 			 * reset err
 			 */
 			*(volatile unsigned char *) (FLASH_BASE + 0x8000) = 0x50;
 
 			/*
+			 * Reenable preemption again, before we go to sleep
+			 */
+			nw_cpld_disable_write();
+
+			/*
 			 * before timeout?
 			 */
 			if (time_before(jiffies, timeout)) {
@@ -580,6 +625,11 @@ static int write_block(unsigned long p, const char __user *buf, int count)
 				return -2;
 
 			}
+		} else {
+			/*
+			 * Reenable preemption again, before we go to sleep
+			 */
+			nw_cpld_disable_write();
 		}
 	}
 
@@ -608,25 +658,6 @@ static int write_block(unsigned long p, const char __user *buf, int count)
 	return count;
 }
 
-
-static void kick_open(void)
-{
-	unsigned long flags;
-
-	/*
-	 * we want to write a bit pattern XXX1 to Xilinx to enable
-	 * the write gate, which will be open for about the next 2ms.
-	 */
-	spin_lock_irqsave(&nw_gpio_lock, flags);
-	nw_cpld_modify(CPLD_FLASH_WR_ENABLE, CPLD_FLASH_WR_ENABLE);
-	spin_unlock_irqrestore(&nw_gpio_lock, flags);
-
-	/*
-	 * let the ISA bus to catch on...
-	 */
-	udelay(25);
-}
-
 static const struct file_operations flash_fops =
 {
 	.owner		= THIS_MODULE,
diff --git a/drivers/mtd/maps/dc21285.c b/drivers/mtd/maps/dc21285.c
index 080f060..9414ceb 100644
--- a/drivers/mtd/maps/dc21285.c
+++ b/drivers/mtd/maps/dc21285.c
@@ -23,34 +23,6 @@
 
 static struct mtd_info *dc21285_mtd;
 
-#ifdef CONFIG_ARCH_NETWINDER
-/*
- * This is really ugly, but it seams to be the only
- * realiable way to do it, as the cpld state machine
- * is unpredictible. So we have a 25us penalty per
- * write access.
- */
-static void nw_en_write(void)
-{
-	unsigned long flags;
-
-	/*
-	 * we want to write a bit pattern XXX1 to Xilinx to enable
-	 * the write gate, which will be open for about the next 2ms.
-	 */
-	spin_lock_irqsave(&nw_gpio_lock, flags);
-	nw_cpld_modify(CPLD_FLASH_WR_ENABLE, CPLD_FLASH_WR_ENABLE);
-	spin_unlock_irqrestore(&nw_gpio_lock, flags);
-
-	/*
-	 * let the ISA bus to catch on...
-	 */
-	udelay(25);
-}
-#else
-#define nw_en_write() do { } while (0)
-#endif
-
 static map_word dc21285_read8(struct map_info *map, unsigned long ofs)
 {
 	map_word val;
@@ -80,26 +52,38 @@ static void dc21285_copy_from(struct map_info *map, void *to, unsigned long from
 static void dc21285_write8(struct map_info *map, const map_word d, unsigned long adr)
 {
 	if (machine_is_netwinder())
-		nw_en_write();
+		nw_cpld_enable_write();
+
 	*CSR_ROMWRITEREG = adr & 3;
 	adr &= ~3;
 	*(uint8_t*)(map->virt + adr) = d.x[0];
+
+	if (machine_is_netwinder())
+		nw_cpld_disable_write();
 }
 
 static void dc21285_write16(struct map_info *map, const map_word d, unsigned long adr)
 {
 	if (machine_is_netwinder())
-		nw_en_write();
+		nw_cpld_enable_write();
+
 	*CSR_ROMWRITEREG = adr & 3;
 	adr &= ~3;
 	*(uint16_t*)(map->virt + adr) = d.x[0];
+
+	if (machine_is_netwinder())
+		nw_cpld_disable_write();
 }
 
 static void dc21285_write32(struct map_info *map, const map_word d, unsigned long adr)
 {
 	if (machine_is_netwinder())
-		nw_en_write();
+		nw_cpld_enable_write();
+
 	*(uint32_t*)(map->virt + adr) = d.x[0];
+
+	if (machine_is_netwinder())
+		nw_cpld_disable_write();
 }
 
 static void dc21285_copy_to_32(struct map_info *map, unsigned long to, const void *from, ssize_t len)
diff --git a/sound/oss/waveartist.c b/sound/oss/waveartist.c
index 24c430f..efb6438 100644
--- a/sound/oss/waveartist.c
+++ b/sound/oss/waveartist.c
@@ -1482,9 +1482,9 @@ vnc_mute_spkr(wavnc_info *devc)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&nw_gpio_lock, flags);
+	nw_cpld_lock(&flags);
 	nw_cpld_modify(CPLD_UNMUTE, devc->spkr_mute_state ? 0 : CPLD_UNMUTE);
-	spin_unlock_irqrestore(&nw_gpio_lock, flags);
+	nw_cpld_unlock(&flags);
 }
 
 static void
-- 
1.7.5.4

Re: [PATCH] netwinder: encapsulate CPLD hardware locking and access

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 338 bytes --]

On Thu, 2012-05-31 at 12:15 +0200, Christian Dietrich wrote:
> I hope my draft of the preemption disabling is not to much crap. I would
> appreciate some comments. And please be aware, that i don't own this
> hardware, and haven't tested it on real hardware therefore. 

Looks relatively sane at first glance; thanks.

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] netwinder: nw_gpio_lock is a raw_spinlock_t

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

On Tue, 2012-05-29 at 12:06 +0200, Christian Dietrich wrote:
> --- a/drivers/char/ds1620.c
> +++ b/drivers/char/ds1620.c
> @@ -74,21 +74,21 @@ static inline void netwinder_ds1620_reset(void)
>  
>  static inline void netwinder_lock(unsigned long *flags)
>  {
> -       spin_lock_irqsave(&nw_gpio_lock, *flags);
> +       raw_spin_lock_irqsave(&nw_gpio_lock, *flags);
>  }
>  
>  static inline void netwinder_unlock(unsigned long *flags)
>  {
> -       spin_unlock_irqrestore(&nw_gpio_lock, *flags);
> +       raw_spin_unlock_irqrestore(&nw_gpio_lock, *flags);
>  } 

If you were to make these functions public by shifting them into
arch/arm/mach-footbridge/include/mach/hardware.h that would be a lot
nicer, and other places wouldn't have to touch the raw spinlock
directly.

Also... while we're thinking about preemption and netwinder, note that
the write enable is valid for only 2ms or so. So all the functions in
dc21285.c that you just touched should probably *also* be disabling
preemption when they're run on a netwinder, to ensure that that time
doesn't expire before they actually get to run.

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

[PATCH] mtd: dc21285.c: remove double check of CONFIG_ARCH_NETWINDER

[Christian Dietrich]

When CONFIG_ARCH_NETWINDER is unset nw_en_write is a NOP. But
machine_is_netwinder() also checks for
CONFIG_ARCH_NETWINDER. Therefore in the !netwinder case the
preprocessed code is:

if (0)
   do {} while(0);

Signed-off-by: Christian Dietrich <christian.dietrich@informatik.uni-erlangen.de>
---
 drivers/mtd/maps/dc21285.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/maps/dc21285.c b/drivers/mtd/maps/dc21285.c
index 86598a1..38fbf23 100644
--- a/drivers/mtd/maps/dc21285.c
+++ b/drivers/mtd/maps/dc21285.c
@@ -79,8 +79,7 @@ static void dc21285_copy_from(struct map_info *map, void *to, unsigned long from
 
 static void dc21285_write8(struct map_info *map, const map_word d, unsigned long adr)
 {
-	if (machine_is_netwinder())
-		nw_en_write();
+	nw_en_write();
 	*CSR_ROMWRITEREG = adr & 3;
 	adr &= ~3;
 	*(uint8_t*)(map->virt + adr) = d.x[0];
@@ -88,8 +87,7 @@ static void dc21285_write8(struct map_info *map, const map_word d, unsigned long
 
 static void dc21285_write16(struct map_info *map, const map_word d, unsigned long adr)
 {
-	if (machine_is_netwinder())
-		nw_en_write();
+	nw_en_write();
 	*CSR_ROMWRITEREG = adr & 3;
 	adr &= ~3;
 	*(uint16_t*)(map->virt + adr) = d.x[0];
@@ -97,8 +95,7 @@ static void dc21285_write16(struct map_info *map, const map_word d, unsigned lon
 
 static void dc21285_write32(struct map_info *map, const map_word d, unsigned long adr)
 {
-	if (machine_is_netwinder())
-		nw_en_write();
+	nw_en_write();
 	*(uint32_t*)(map->virt + adr) = d.x[0];
 }
 
-- 
1.7.5.4

Re: [PATCH] mtd: dc21285.c: remove double check of CONFIG_ARCH_NETWINDER

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

On Tue, 2012-05-29 at 12:06 +0200, Christian Dietrich wrote:
> When CONFIG_ARCH_NETWINDER is unset nw_en_write is a NOP. But
> machine_is_netwinder() also checks for
> CONFIG_ARCH_NETWINDER. Therefore in the !netwinder case the
> preprocessed code is:
> 
> if (0)
>    do {} while(0); 

It's not a double check. It's a compile time check for "do we even need
to build this code at all?", and a separate run time check for "do we
need to run this code now?".

Think about the case where CONFIG_ARCH_NETWINDER is set, but you aren't
actually running this kernel binary on a netwinder *today*.

Yes, we might not support multi-platform kernels on ARM yet, but we are
slowly getting there. This kind of change just makes that harder.

And even if the machine_is_netwinder() "function" is *currently* a macro
which is hard-coded to return one or zero, that just means that your
change achieves nothing in the compiler output. It'll be silently
optimised away, or not, as appropriate.

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]