Re: MIPS: RB532: Provide functions for gpio configuration

Re: MIPS: RB532: Provide functions for gpio configuration

[Phil Sutter]

Hi!

I fullquote here because of the lists and people added to Cc.

On Thu, Nov 20, 2008 at 08:02:15PM -0800, David Brownell wrote:
> I just noticed:
> 
> > commit 2e373952cc893207a8b47a5e68c2f5155f912449
> > Author: Phil Sutter <n0-1@freewrt.org>
> > Date:   Sat Nov 1 15:13:21 2008 +0100
> >
> >     MIPS: RB532: Provide functions for gpio configuration
> >
> >     As gpiolib doesn't support pin multiplexing, it provides no way to
> >     access the GPIOFUNC register. Also there is no support for setting
> >     interrupt status and level. These functions provide access to them and
> >     are needed by the CompactFlash driver.
> >
> >     Signed-off-by: Phil Sutter <n0-1@freewrt.org>
> >     Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> 
> The conventional way to do most of that is through the irq_chip
> associated with that block of IRQ-capable GPIOs.
> 
> 
> So ...
> 
> > -       /* Set the interrupt status and level for the CF pin */
> > -       rb532_gpio_set_int_level(&rb532_gpio_chip->chip, CF_GPIO_NUM, 1);
> > -       rb532_gpio_set_int_status(&rb532_gpio_chip->chip, CF_GPIO_NUM, 0);
> > +       /* configure CF_GPIO_NUM as CFRDY IRQ source */
> > +       rb532_gpio_set_func(0, CF_GPIO_NUM);
> 
> ... the pinmux would indeed be a SOC-specific mechanism, kicked in
> only for boards that use that GPIO in that way, but ...
> 
> 
> > +       rb532_gpio_direction_input(&rb532_gpio_chip->chip, CF_GPIO_NUM);
> 
> ... normal gpio_request() + gpio_direction_input() would set the pin up, and ...
> 
> 
> > +       rb532_gpio_set_ilevel(1, CF_GPIO_NUM);
> > +       rb532_gpio_set_istat(0, CF_GPIO_NUM);
> 
> 	status = request_irq(gpio_to_irq(CF_GPIO_NUM), ... )
> 
> with appropriate IRQF_TRIGGER_* flags should solve that problem.
> Or even just set_irq_type().  (At least for one of the two
> registers updated there ... I can't guess what "istat" would be.)
> 
> Just FYI at this point.  Maybe you have a reason not to fit into
> the genirq framework.
> 
> - Dave

Thanks for your hints, Dave. I already had a patch flying around adding
a set_type() function to the irq_chip (merely a dummy to shut up
warnings in the boot log). After extending it to cover the needs of the
mapped GPIO pins and some other work, I could drop that CompactFlash
initialisation code in gpio.c completely.

As a side effect, many of my changes to pata-rb532-cf got unnecessary
which I consider a very good sign.

I'll reply to this mail with the relevant patches for each list and
maintainer. 

Greetings, Phil

[PATCH] MIPS: rb532: add set_type() function to IRQ struct

[Phil Sutter]

Interrupt Group 4 mapps the GPIO pins enabled as interrupt sources;
add defines to make this clear when addressing them later in code.

The mapped GPIOs support triggering on either level high or low. To
achieve this, the set_type() function calls rb532_gpio_set_ilevel() for
interrupts of the above mentioned group.

As there is no way to alter the triggering characteristics of the other
interrupts, accept level triggering on status high only. (This is just a
guess; but as the system boots fine and interrupt-driven devices (e.g.
serial console) work with no implications, it seems to be right.)

To clear a GPIO mapped IRQ, the source has to be cleared (i.e., the
interrupt status bit of the corresponding GPIO pin). This is done inside
rb532_disable_irq().

After applying these changes I could undo most of my former "fixes" to
pata-rb532-cf. Particularly all interrupt handling can be done
generically via set_irq_type() as it was before.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/include/asm/mach-rc32434/irq.h |    3 +++
 arch/mips/rb532/irq.c                    |   26 ++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/arch/mips/include/asm/mach-rc32434/irq.h b/arch/mips/include/asm/mach-rc32434/irq.h
index 56738d8..023a5b1 100644
--- a/arch/mips/include/asm/mach-rc32434/irq.h
+++ b/arch/mips/include/asm/mach-rc32434/irq.h
@@ -30,4 +30,7 @@
 #define ETH0_RX_OVR_IRQ   	(GROUP3_IRQ_BASE + 9)
 #define ETH0_TX_UND_IRQ   	(GROUP3_IRQ_BASE + 10)
 
+#define GPIO_MAPPED_IRQ_BASE	GROUP4_IRQ_BASE
+#define GPIO_MAPPED_IRQ_GROUP	4
+
 #endif  /* __ASM_RC32434_IRQ_H */
diff --git a/arch/mips/rb532/irq.c b/arch/mips/rb532/irq.c
index 549b46d..e265af5 100644
--- a/arch/mips/rb532/irq.c
+++ b/arch/mips/rb532/irq.c
@@ -46,6 +46,7 @@
 #include <asm/system.h>
 
 #include <asm/mach-rc32434/irq.h>
+#include <asm/mach-rc32434/gpio.h>
 
 struct intr_group {
 	u32 mask;	/* mask of valid bits in pending/mask registers */
@@ -150,6 +151,9 @@ static void rb532_disable_irq(unsigned int irq_nr)
 		mask |= intr_bit;
 		WRITE_MASK(addr, mask);
 
+		if (group == GPIO_MAPPED_IRQ_GROUP)
+			rb532_gpio_set_istat(0, irq_nr - GPIO_MAPPED_IRQ_BASE);
+
 		/*
 		 * if there are no more interrupts enabled in this
 		 * group, disable corresponding IP
@@ -165,12 +169,34 @@ static void rb532_mask_and_ack_irq(unsigned int irq_nr)
 	ack_local_irq(group_to_ip(irq_to_group(irq_nr)));
 }
 
+static int rb532_set_type(unsigned int irq_nr, unsigned type)
+{
+	int gpio = irq_nr - GPIO_MAPPED_IRQ_BASE;
+	int group = irq_to_group(irq_nr);
+
+	if (group != GPIO_MAPPED_IRQ_GROUP)
+		return (type == IRQ_TYPE_LEVEL_HIGH) ? 0 : -EINVAL;
+
+	switch (type) {
+		case IRQ_TYPE_LEVEL_HIGH:
+			rb532_gpio_set_ilevel(1, gpio);
+			break;
+		case IRQ_TYPE_LEVEL_LOW:
+			rb532_gpio_set_ilevel(0, gpio);
+			break;
+		default:
+			return -EINVAL;
+	}
+	return 0;
+}
+
 static struct irq_chip rc32434_irq_type = {
 	.name		= "RB532",
 	.ack		= rb532_disable_irq,
 	.mask		= rb532_disable_irq,
 	.mask_ack	= rb532_mask_and_ack_irq,
 	.unmask		= rb532_enable_irq,
+	.set_type	= rb532_set_type,
 };
 
 void __init arch_init_irq(void)
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: add set_type() function to IRQ struct

[Ralf Baechle]

On Fri, Nov 28, 2008 at 08:45:10PM +0100, Phil Sutter wrote:

> +	switch (type) {
> +		case IRQ_TYPE_LEVEL_HIGH:
> +			rb532_gpio_set_ilevel(1, gpio);
> +			break;
> +		case IRQ_TYPE_LEVEL_LOW:
> +			rb532_gpio_set_ilevel(0, gpio);
> +			break;
> +		default:
> +			return -EINVAL;
> +	}

Linux coding style - the case and default labels should have the same
indentation level as the switch statement.  I fixed that.

Applied, thanks.

  Ralf

[PATCH] MIPS: rb532: auto disable GPIO alternate function

[Phil Sutter]

When a driver calls gpio_set_direction_{input,output}(), it obviously
doesn't want the alternate function for that pin to be active (as the
direction would not matter in that case). This patch ensures alternate
function is disabled when the direction is being changed.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/rb532/gpio.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index e35cb75..f5b15a1 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -169,8 +169,8 @@ static int rb532_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 
 	gpch = container_of(chip, struct rb532_gpio_chip, chip);
 
-	if (rb532_get_bit(offset, gpch->regbase + GPIOFUNC))
-		return 1;	/* alternate function, GPIOCFG is ignored */
+	/* disable alternate function in case it's set */
+	rb532_set_bit(0, offset, gpch->regbase + GPIOFUNC);
 
 	rb532_set_bit(0, offset, gpch->regbase + GPIOCFG);
 	return 0;
@@ -186,8 +186,8 @@ static int rb532_gpio_direction_output(struct gpio_chip *chip,
 
 	gpch = container_of(chip, struct rb532_gpio_chip, chip);
 
-	if (rb532_get_bit(offset, gpch->regbase + GPIOFUNC))
-		return 1;	/* alternate function, GPIOCFG is ignored */
+	/* disable alternate function in case it's set */
+	rb532_set_bit(0, offset, gpch->regbase + GPIOFUNC);
 
 	/* set the initial output value */
 	rb532_set_bit(value, offset, gpch->regbase + GPIOD);
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: auto disable GPIO alternate function

[Ralf Baechle]

On Fri, Nov 28, 2008 at 08:46:09PM +0100, Phil Sutter wrote:

> When a driver calls gpio_set_direction_{input,output}(), it obviously
> doesn't want the alternate function for that pin to be active (as the
> direction would not matter in that case). This patch ensures alternate
> function is disabled when the direction is being changed.

Applied.  Thanks,

  Ralf

[PATCH] MIPS: rb532: remove useless CF GPIO initialisation

[Phil Sutter]

As the pata-rb532-cf driver calls gpio_set_direction_input(), the calls
to rb532_gpio_set_func() and rb532_gpio_direction_input() are not needed
since the alternate function is automatically being disabled when
changing the GPIO pin direction.
The later two calls to rb532_gpio_set_{ilevel,istat}() are implicitly
being done by the IRQ initialisation of pata-rb532-cf.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/rb532/gpio.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index f5b15a1..b195f79 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -259,12 +259,6 @@ int __init rb532_gpio_init(void)
 		return -ENXIO;
 	}
 
-	/* configure CF_GPIO_NUM as CFRDY IRQ source */
-	rb532_gpio_set_func(0, CF_GPIO_NUM);
-	rb532_gpio_direction_input(&rb532_gpio_chip->chip, CF_GPIO_NUM);
-	rb532_gpio_set_ilevel(1, CF_GPIO_NUM);
-	rb532_gpio_set_istat(0, CF_GPIO_NUM);
-
 	return 0;
 }
 arch_initcall(rb532_gpio_init);
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: remove useless CF GPIO initialisation

[Ralf Baechle]

On Fri, Nov 28, 2008 at 08:46:22PM +0100, Phil Sutter wrote:

> As the pata-rb532-cf driver calls gpio_set_direction_input(), the calls
> to rb532_gpio_set_func() and rb532_gpio_direction_input() are not needed
> since the alternate function is automatically being disabled when
> changing the GPIO pin direction.
> The later two calls to rb532_gpio_set_{ilevel,istat}() are implicitly
> being done by the IRQ initialisation of pata-rb532-cf.
> 
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>

Applied.  Thanks!

  Ralf

[PATCH] MIPS: rb532: remove unused rb532_gpio_set_func()

[Phil Sutter]

Since disabling of the alternate function of a GPIO pin is being done
implicitly when changing it's direction, the above mentioned function is
not being called anymore and can be removed.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/rb532/gpio.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index b195f79..d75eb19 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -228,14 +228,6 @@ void rb532_gpio_set_istat(int bit, unsigned gpio)
 }
 EXPORT_SYMBOL(rb532_gpio_set_istat);
 
-/*
- * Configure GPIO alternate function
- */
-static void rb532_gpio_set_func(int bit, unsigned gpio)
-{
-       rb532_set_bit(bit, gpio, rb532_gpio_chip->regbase + GPIOFUNC);
-}
-
 int __init rb532_gpio_init(void)
 {
 	struct resource *r;
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: remove unused rb532_gpio_set_func()

[Phil Sutter]

Hi,

On Fri, Nov 28, 2008 at 08:46:29PM +0100, Phil Sutter wrote:
> Since disabling of the alternate function of a GPIO pin is being done
> implicitly when changing it's direction, the above mentioned function is
> not being called anymore and can be removed.

Please do not apply this patch. In fact, the function in question should
be exported as kernel symbol, as else there is no possibility for
drivers to turn on alternate function for a GPIO pin. This is of
particular use when trying to read the S1 button state, as the
corresponding GPIO pin is multiplexed with UART0 input.

Greetings, Phil

[PATCH] pata-rb532-cf: fix and rename register definitions

[Phil Sutter]

The original standalone driver uses a custom address for the error
register. Use it in pata-rb532-cf, too.

Rename two register definitions:
- The address offset 0x0800 in fact is the ATA base, not ATA command
  address.
- The offset 0x0C00 is not a regular ATA data address, but a buffered one
  allowing 4-byte IO.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 drivers/ata/pata_rb532_cf.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index f8b3ffc..392116c 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -39,9 +39,11 @@
 #define RB500_CF_MAXPORTS	1
 #define RB500_CF_IO_DELAY	400
 
-#define RB500_CF_REG_CMD	0x0800
+#define RB500_CF_REG_BASE	0x0800
+#define RB500_CF_REG_ERR	0x080D
 #define RB500_CF_REG_CTRL	0x080E
-#define RB500_CF_REG_DATA	0x0C00
+/* 32bit buffered data register offset */
+#define RB500_CF_REG_DBUF32	0x0C00
 
 struct rb532_cf_info {
 	void __iomem	*iobase;
@@ -146,13 +148,14 @@ static void rb532_pata_setup_ports(struct ata_host *ah)
 	ap->pio_mask	= 0x1f; /* PIO4 */
 	ap->flags	= ATA_FLAG_NO_LEGACY | ATA_FLAG_MMIO;
 
-	ap->ioaddr.cmd_addr	= info->iobase + RB500_CF_REG_CMD;
+	ap->ioaddr.cmd_addr	= info->iobase + RB500_CF_REG_BASE;
 	ap->ioaddr.ctl_addr	= info->iobase + RB500_CF_REG_CTRL;
 	ap->ioaddr.altstatus_addr = info->iobase + RB500_CF_REG_CTRL;
 
 	ata_sff_std_ports(&ap->ioaddr);
 
-	ap->ioaddr.data_addr	= info->iobase + RB500_CF_REG_DATA;
+	ap->ioaddr.data_addr	= info->iobase + RB500_CF_REG_DBUF32;
+	ap->ioaddr.error_addr	= info->iobase + RB500_CF_REG_ERR;
 }
 
 static __devinit int rb532_pata_driver_probe(struct platform_device *pdev)
-- 
1.5.6.4

Re: [PATCH] pata-rb532-cf: fix and rename register definitions

[Jeff Garzik]

Phil Sutter wrote:
> The original standalone driver uses a custom address for the error
> register. Use it in pata-rb532-cf, too.
> 
> Rename two register definitions:
> - The address offset 0x0800 in fact is the ATA base, not ATA command
>   address.
> - The offset 0x0C00 is not a regular ATA data address, but a buffered one
>   allowing 4-byte IO.
> 
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
> ---
>  drivers/ata/pata_rb532_cf.c |   11 +++++++----

applied

[PATCH] pata-rb532-cf: fix signature of the xfer function

[Phil Sutter]

Per definition, this function should return the number of bytes
consumed. As the original parameter "buflen" is being decremented inside
the read/write loop, save it in "retlen" at the beginning.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
Acked-by: Sergei Shtyltov <sshtylyov@ru.mvista.com>
Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Acked-by: Florian Fainelli <florian@openwrt.org>
---
 drivers/ata/pata_rb532_cf.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index 392116c..c2e6fb9 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -74,11 +74,12 @@ static void rb532_pata_exec_command(struct ata_port *ap,
 	rb532_pata_finish_io(ap);
 }
 
-static void rb532_pata_data_xfer(struct ata_device *adev, unsigned char *buf,
+static unsigned int rb532_pata_data_xfer(struct ata_device *adev, unsigned char *buf,
 				unsigned int buflen, int write_data)
 {
 	struct ata_port *ap = adev->link->ap;
 	void __iomem *ioaddr = ap->ioaddr.data_addr;
+	int retlen = buflen;
 
 	if (write_data) {
 		for (; buflen > 0; buflen--, buf++)
@@ -89,6 +90,7 @@ static void rb532_pata_data_xfer(struct ata_device *adev, unsigned char *buf,
 	}
 
 	rb532_pata_finish_io(adev->link->ap);
+	return retlen;
 }
 
 static void rb532_pata_freeze(struct ata_port *ap)
-- 
1.5.6.4

Re: [PATCH] pata-rb532-cf: fix signature of the xfer function

[Jeff Garzik]

Phil Sutter wrote:
> Per definition, this function should return the number of bytes
> consumed. As the original parameter "buflen" is being decremented inside
> the read/write loop, save it in "retlen" at the beginning.
> 
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
> Acked-by: Sergei Shtyltov <sshtylyov@ru.mvista.com>
> Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Acked-by: Florian Fainelli <florian@openwrt.org>
> ---
>  drivers/ata/pata_rb532_cf.c |    4 +++-

applied

[PATCH] pata-rb532-cf: read/write data in 4-byte blocks

[Phil Sutter]

* rename the offset definition to avoid abiguity with the standard ATA
  IO address
* read and write four bytes at once like the original driver does
* use writesl() and readsl() which implicitly iterate over the data

This patch assumes buflen to be a multiple of four, which is true for
ATA devices. ATAPI support is not known, though unlikely, as the
original driver always transfers 512 Bytes at once. In doubt, do the
right thing and return the number of bytes actually consumed.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
Acked-by: Sergei Shtyltov <sshtylyov@ru.mvista.com>
Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Acked-by: Florian Fainelli <florian@openwrt.org>
---
 drivers/ata/pata_rb532_cf.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index c2e6fb9..362af11 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -79,18 +79,14 @@ static unsigned int rb532_pata_data_xfer(struct ata_device *adev, unsigned char
 {
 	struct ata_port *ap = adev->link->ap;
 	void __iomem *ioaddr = ap->ioaddr.data_addr;
-	int retlen = buflen;
 
-	if (write_data) {
-		for (; buflen > 0; buflen--, buf++)
-			writeb(*buf, ioaddr);
-	} else {
-		for (; buflen > 0; buflen--, buf++)
-			*buf = readb(ioaddr);
-	}
+	if (write_data)
+		writesl(ioaddr, buf, buflen / sizeof(u32));
+	else
+		readsl(ioaddr, buf, buflen / sizeof(u32));
 
 	rb532_pata_finish_io(adev->link->ap);
-	return retlen;
+	return buflen - buflen % sizeof(u32);
 }
 
 static void rb532_pata_freeze(struct ata_port *ap)
-- 
1.5.6.4

review of pata-rb532-cf

[Phil Sutter]

Hi,

I did some review of pata-rb532-cf in order to simplify the code a bit
in which I think have succeeded.

After finding the first patch by accident, patches two and three were
quite obvious. Patch four also solves the discussed changes to
rb532_pata_data_xfer() as it drops it completely, replacing it by
a standard libata function. Patch 5 is rather experimental and hopefully
triggers a discussion about the changes it introduces.

Greetings, Phil

Re: review of pata-rb532-cf

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday 20 January 2009, Phil Sutter wrote:
> 
> Hi,
> 
> I did some review of pata-rb532-cf in order to simplify the code a bit
> in which I think have succeeded.
> 
> After finding the first patch by accident, patches two and three were
> quite obvious. Patch four also solves the discussed changes to
> rb532_pata_data_xfer() as it drops it completely, replacing it by
> a standard libata function. Patch 5 is rather experimental and hopefully
> triggers a discussion about the changes it introduces.

FWIW patches 1-4 look OK to me.

Thanks,
Bart

[PATCH 1/5] pata-rb532-cf: remove set_irq_type from finish_io

[Phil Sutter]

The driver has been tested without the call to set_irq_type at this
point and occurs to work fine, so it should be safe to remove it.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 drivers/ata/pata_rb532_cf.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index 362af11..cd2eecd 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -63,8 +63,6 @@ static inline void rb532_pata_finish_io(struct ata_port *ap)
 	   ata_sff_sync might be sufficient. */
 	ata_sff_dma_pause(ap);
 	ndelay(RB500_CF_IO_DELAY);
-
-	set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
 }
 
 static void rb532_pata_exec_command(struct ata_port *ap,
-- 
1.5.6.4

Re: [PATCH 1/5] pata-rb532-cf: remove set_irq_type from finish_io

[Jeff Garzik]

Phil Sutter wrote:
> The driver has been tested without the call to set_irq_type at this
> point and occurs to work fine, so it should be safe to remove it.
> 
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
> ---
>  drivers/ata/pata_rb532_cf.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
> index 362af11..cd2eecd 100644
> --- a/drivers/ata/pata_rb532_cf.c
> +++ b/drivers/ata/pata_rb532_cf.c
> @@ -63,8 +63,6 @@ static inline void rb532_pata_finish_io(struct ata_port *ap)
>  	   ata_sff_sync might be sufficient. */
>  	ata_sff_dma_pause(ap);
>  	ndelay(RB500_CF_IO_DELAY);
> -
> -	set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);

applied

[PATCH 2/5] pata-rb532-cf: replace rb532_pata_finish_io()

[Phil Sutter]

Since the delay used internally is just the same as ata_sff_pause()
uses, rb532_pata_finish_io() does exactly the same as ata_sff_pause()
and thus can be replaced by the later one.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 drivers/ata/pata_rb532_cf.c |   15 ++-------------
 1 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index cd2eecd..9305eee 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -54,22 +54,11 @@ struct rb532_cf_info {
 
 /* ------------------------------------------------------------------------ */
 
-static inline void rb532_pata_finish_io(struct ata_port *ap)
-{
-	struct ata_host *ah = ap->host;
-	struct rb532_cf_info *info = ah->private_data;
-
-	/* FIXME: Keep previous delay. If this is merely a fence then
-	   ata_sff_sync might be sufficient. */
-	ata_sff_dma_pause(ap);
-	ndelay(RB500_CF_IO_DELAY);
-}
-
 static void rb532_pata_exec_command(struct ata_port *ap,
 				const struct ata_taskfile *tf)
 {
 	writeb(tf->command, ap->ioaddr.command_addr);
-	rb532_pata_finish_io(ap);
+	ata_sff_pause(ap);
 }
 
 static unsigned int rb532_pata_data_xfer(struct ata_device *adev, unsigned char *buf,
@@ -83,7 +72,7 @@ static unsigned int rb532_pata_data_xfer(struct ata_device *adev, unsigned char
 	else
 		readsl(ioaddr, buf, buflen / sizeof(u32));
 
-	rb532_pata_finish_io(adev->link->ap);
+	ata_sff_pause(ap);
 	return buflen - buflen % sizeof(u32);
 }
 
-- 
1.5.6.4

Re: [PATCH 2/5] pata-rb532-cf: replace rb532_pata_finish_io()

[Jeff Garzik]

Phil Sutter wrote:
> Since the delay used internally is just the same as ata_sff_pause()
> uses, rb532_pata_finish_io() does exactly the same as ata_sff_pause()
> and thus can be replaced by the later one.
> 
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
> ---
>  drivers/ata/pata_rb532_cf.c |   15 ++-------------
>  1 files changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
> index cd2eecd..9305eee 100644
> --- a/drivers/ata/pata_rb532_cf.c
> +++ b/drivers/ata/pata_rb532_cf.c
> @@ -54,22 +54,11 @@ struct rb532_cf_info {
>  
>  /* ------------------------------------------------------------------------ */
>  
> -static inline void rb532_pata_finish_io(struct ata_port *ap)
> -{
> -	struct ata_host *ah = ap->host;
> -	struct rb532_cf_info *info = ah->private_data;
> -
> -	/* FIXME: Keep previous delay. If this is merely a fence then
> -	   ata_sff_sync might be sufficient. */
> -	ata_sff_dma_pause(ap);
> -	ndelay(RB500_CF_IO_DELAY);
> -}

ACK patches 2-5, but they don't apply to upstream (plus patch #1)

Re: [PATCH 2/5] pata-rb532-cf: replace rb532_pata_finish_io()

[Phil Sutter]

Hi,

On Tue, Jan 27, 2009 at 02:14:00AM -0500, Jeff Garzik wrote:
> ACK patches 2-5, but they don't apply to upstream (plus patch #1)

the problem was the original patch regarding 32bit IO, which I forgot to
remove prior to creating the discussed changes. I'll reply to this email
with patches 2-5, which should now cleanly apply to current ide-2.6.

Greetings, Phil

[PATCH 2/5] pata-rb532-cf: replace rb532_pata_finish_io()

[Phil Sutter]

Since the delay used internally is just the same as ata_sff_pause()
uses, rb532_pata_finish_io() does exactly the same as ata_sff_pause()
and thus can be replaced by the later one.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 drivers/ata/pata_rb532_cf.c |   15 ++-------------
 1 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index ebfcda2..6fe660b 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -54,22 +54,11 @@ struct rb532_cf_info {
 
 /* ------------------------------------------------------------------------ */
 
-static inline void rb532_pata_finish_io(struct ata_port *ap)
-{
-	struct ata_host *ah = ap->host;
-	struct rb532_cf_info *info = ah->private_data;
-
-	/* FIXME: Keep previous delay. If this is merely a fence then
-	   ata_sff_sync might be sufficient. */
-	ata_sff_dma_pause(ap);
-	ndelay(RB500_CF_IO_DELAY);
-}
-
 static void rb532_pata_exec_command(struct ata_port *ap,
 				const struct ata_taskfile *tf)
 {
 	writeb(tf->command, ap->ioaddr.command_addr);
-	rb532_pata_finish_io(ap);
+	ata_sff_pause(ap);
 }
 
 static unsigned int rb532_pata_data_xfer(struct ata_device *adev, unsigned char *buf,
@@ -87,7 +76,7 @@ static unsigned int rb532_pata_data_xfer(struct ata_device *adev, unsigned char
 			*buf = readb(ioaddr);
 	}
 
-	rb532_pata_finish_io(adev->link->ap);
+	ata_sff_pause(ap);
 	return retlen;
 }
 
-- 
1.5.6.4

Re: [PATCH 2/5] pata-rb532-cf: replace rb532_pata_finish_io()

[Jeff Garzik]

Phil Sutter wrote:
> Since the delay used internally is just the same as ata_sff_pause()
> uses, rb532_pata_finish_io() does exactly the same as ata_sff_pause()
> and thus can be replaced by the later one.
> 
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>

applied 2-5

[PATCH 3/5] pata-rb532-cf: use ata_sff_exec_command()

[Phil Sutter]

The only difference between rb532_pata_exec_command() and
ata_sff_exec_command() is added debugging output, so it can be dropped
and the standard op used instead.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 drivers/ata/pata_rb532_cf.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index 6fe660b..9d61ce5 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -54,13 +54,6 @@ struct rb532_cf_info {
 
 /* ------------------------------------------------------------------------ */
 
-static void rb532_pata_exec_command(struct ata_port *ap,
-				const struct ata_taskfile *tf)
-{
-	writeb(tf->command, ap->ioaddr.command_addr);
-	ata_sff_pause(ap);
-}
-
 static unsigned int rb532_pata_data_xfer(struct ata_device *adev, unsigned char *buf,
 				unsigned int buflen, int write_data)
 {
@@ -112,7 +105,6 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 
 static struct ata_port_operations rb532_pata_port_ops = {
 	.inherits		= &ata_sff_port_ops,
-	.sff_exec_command	= rb532_pata_exec_command,
 	.sff_data_xfer		= rb532_pata_data_xfer,
 	.freeze			= rb532_pata_freeze,
 	.thaw			= rb532_pata_thaw,
-- 
1.5.6.4

[PATCH 4/5] pata-rb532-cf: use ata_sff_data_xfer32()

[Phil Sutter]

The biggest difference between rb532_pata_data_xfer() and
ata_sff_data_xfer32() is the call to ata_sff_pause() at the end of
rb532_pata_data_xfer() which I suppose to be unnecessary since it works
without. I've also tested using ata_sff_data_xfer() as replacement, but
since we know that the driver supports 32bit IO, using the optimised
version should be safe.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 drivers/ata/pata_rb532_cf.c |   21 +--------------------
 1 files changed, 1 insertions(+), 20 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index 9d61ce5..9fb91e4 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -54,25 +54,6 @@ struct rb532_cf_info {
 
 /* ------------------------------------------------------------------------ */
 
-static unsigned int rb532_pata_data_xfer(struct ata_device *adev, unsigned char *buf,
-				unsigned int buflen, int write_data)
-{
-	struct ata_port *ap = adev->link->ap;
-	void __iomem *ioaddr = ap->ioaddr.data_addr;
-	int retlen = buflen;
-
-	if (write_data) {
-		for (; buflen > 0; buflen--, buf++)
-			writeb(*buf, ioaddr);
-	} else {
-		for (; buflen > 0; buflen--, buf++)
-			*buf = readb(ioaddr);
-	}
-
-	ata_sff_pause(ap);
-	return retlen;
-}
-
 static void rb532_pata_freeze(struct ata_port *ap)
 {
 	struct rb532_cf_info *info = ap->host->private_data;
@@ -105,7 +86,7 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 
 static struct ata_port_operations rb532_pata_port_ops = {
 	.inherits		= &ata_sff_port_ops,
-	.sff_data_xfer		= rb532_pata_data_xfer,
+	.sff_data_xfer		= ata_sff_data_xfer32,
 	.freeze			= rb532_pata_freeze,
 	.thaw			= rb532_pata_thaw,
 };
-- 
1.5.6.4

[PATCH 5/5] pata-rb532-cf: drop custom freeze and thaw

[Phil Sutter]

I'm not quite sure what freezing and thawing is used for. Tests showed
that the port is being frozen at initialisation state and thawed right
afterwards, then the functions were not called anymore. Dropping the
complete custom code for handling the frozen state seems to work at
least for a standard use case including mounting a partition, copying
some files in it (in parallel) and finally removing them and unmounting
the partition.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 drivers/ata/pata_rb532_cf.c |   20 +-------------------
 1 files changed, 1 insertions(+), 19 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index 9fb91e4..fbfee1b 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -48,26 +48,11 @@
 struct rb532_cf_info {
 	void __iomem	*iobase;
 	unsigned int	gpio_line;
-	int		frozen;
 	unsigned int	irq;
 };
 
 /* ------------------------------------------------------------------------ */
 
-static void rb532_pata_freeze(struct ata_port *ap)
-{
-	struct rb532_cf_info *info = ap->host->private_data;
-
-	info->frozen = 1;
-}
-
-static void rb532_pata_thaw(struct ata_port *ap)
-{
-	struct rb532_cf_info *info = ap->host->private_data;
-
-	info->frozen = 0;
-}
-
 static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 {
 	struct ata_host *ah = dev_instance;
@@ -75,8 +60,7 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 
 	if (gpio_get_value(info->gpio_line)) {
 		set_irq_type(info->irq, IRQ_TYPE_LEVEL_LOW);
-		if (!info->frozen)
-			ata_sff_interrupt(info->irq, dev_instance);
+		ata_sff_interrupt(info->irq, dev_instance);
 	} else {
 		set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
 	}
@@ -87,8 +71,6 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 static struct ata_port_operations rb532_pata_port_ops = {
 	.inherits		= &ata_sff_port_ops,
 	.sff_data_xfer		= ata_sff_data_xfer32,
-	.freeze			= rb532_pata_freeze,
-	.thaw			= rb532_pata_thaw,
 };
 
 /* ------------------------------------------------------------------------ */
-- 
1.5.6.4

[PATCH 3/5] pata-rb532-cf: use ata_sff_exec_command()

[Phil Sutter]

The only difference between rb532_pata_exec_command() and
ata_sff_exec_command() is added debugging output, so it can be dropped
and the standard op used instead.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 drivers/ata/pata_rb532_cf.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index 9305eee..11a4411 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -54,13 +54,6 @@ struct rb532_cf_info {
 
 /* ------------------------------------------------------------------------ */
 
-static void rb532_pata_exec_command(struct ata_port *ap,
-				const struct ata_taskfile *tf)
-{
-	writeb(tf->command, ap->ioaddr.command_addr);
-	ata_sff_pause(ap);
-}
-
 static unsigned int rb532_pata_data_xfer(struct ata_device *adev, unsigned char *buf,
 				unsigned int buflen, int write_data)
 {
@@ -108,7 +101,6 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 
 static struct ata_port_operations rb532_pata_port_ops = {
 	.inherits		= &ata_sff_port_ops,
-	.sff_exec_command	= rb532_pata_exec_command,
 	.sff_data_xfer		= rb532_pata_data_xfer,
 	.freeze			= rb532_pata_freeze,
 	.thaw			= rb532_pata_thaw,
-- 
1.5.6.4

[PATCH 4/5] pata-rb532-cf: use ata_sff_data_xfer32()

[Phil Sutter]

The biggest difference between rb532_pata_data_xfer() and
ata_sff_data_xfer32() is the call to ata_sff_pause() at the end of
rb532_pata_data_xfer() which I suppose to be unnecessary since it works
without. I've also tested using ata_sff_data_xfer() as replacement, but
since we know that the driver supports 32bit IO, using the optimised
version should be safe.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 drivers/ata/pata_rb532_cf.c |   17 +----------------
 1 files changed, 1 insertions(+), 16 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index 11a4411..9fb91e4 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -54,21 +54,6 @@ struct rb532_cf_info {
 
 /* ------------------------------------------------------------------------ */
 
-static unsigned int rb532_pata_data_xfer(struct ata_device *adev, unsigned char *buf,
-				unsigned int buflen, int write_data)
-{
-	struct ata_port *ap = adev->link->ap;
-	void __iomem *ioaddr = ap->ioaddr.data_addr;
-
-	if (write_data)
-		writesl(ioaddr, buf, buflen / sizeof(u32));
-	else
-		readsl(ioaddr, buf, buflen / sizeof(u32));
-
-	ata_sff_pause(ap);
-	return buflen - buflen % sizeof(u32);
-}
-
 static void rb532_pata_freeze(struct ata_port *ap)
 {
 	struct rb532_cf_info *info = ap->host->private_data;
@@ -101,7 +86,7 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 
 static struct ata_port_operations rb532_pata_port_ops = {
 	.inherits		= &ata_sff_port_ops,
-	.sff_data_xfer		= rb532_pata_data_xfer,
+	.sff_data_xfer		= ata_sff_data_xfer32,
 	.freeze			= rb532_pata_freeze,
 	.thaw			= rb532_pata_thaw,
 };
-- 
1.5.6.4

[PATCH 5/5] pata-rb532-cf: drop custom freeze and thaw

[Phil Sutter]

I'm not quite sure what freezing and thawing is used for. Tests showed
that the port is being frozen at initialisation state and thawed right
afterwards, then the functions were not called anymore. Dropping the
complete custom code for handling the frozen state seems to work at
least for a standard use case including mounting a partition, copying
some files in it (in parallel) and finally removing them and unmounting
the partition.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 drivers/ata/pata_rb532_cf.c |   20 +-------------------
 1 files changed, 1 insertions(+), 19 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index 9fb91e4..fbfee1b 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -48,26 +48,11 @@
 struct rb532_cf_info {
 	void __iomem	*iobase;
 	unsigned int	gpio_line;
-	int		frozen;
 	unsigned int	irq;
 };
 
 /* ------------------------------------------------------------------------ */
 
-static void rb532_pata_freeze(struct ata_port *ap)
-{
-	struct rb532_cf_info *info = ap->host->private_data;
-
-	info->frozen = 1;
-}
-
-static void rb532_pata_thaw(struct ata_port *ap)
-{
-	struct rb532_cf_info *info = ap->host->private_data;
-
-	info->frozen = 0;
-}
-
 static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 {
 	struct ata_host *ah = dev_instance;
@@ -75,8 +60,7 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 
 	if (gpio_get_value(info->gpio_line)) {
 		set_irq_type(info->irq, IRQ_TYPE_LEVEL_LOW);
-		if (!info->frozen)
-			ata_sff_interrupt(info->irq, dev_instance);
+		ata_sff_interrupt(info->irq, dev_instance);
 	} else {
 		set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
 	}
@@ -87,8 +71,6 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 static struct ata_port_operations rb532_pata_port_ops = {
 	.inherits		= &ata_sff_port_ops,
 	.sff_data_xfer		= ata_sff_data_xfer32,
-	.freeze			= rb532_pata_freeze,
-	.thaw			= rb532_pata_thaw,
 };
 
 /* ------------------------------------------------------------------------ */
-- 
1.5.6.4

[PATCH] rb532: update headers

[Phil Sutter]

Remove the {set,get}_434_reg() prototypes, as the functions have been
removed. Also move the prototypes for {get,set}_latch_u5() to the
correct place.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/include/asm/mach-rc32434/gpio.h |    4 ----
 arch/mips/include/asm/mach-rc32434/rb.h   |    3 +++
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/mips/include/asm/mach-rc32434/gpio.h b/arch/mips/include/asm/mach-rc32434/gpio.h
index b5cf645..ca92c81 100644
--- a/arch/mips/include/asm/mach-rc32434/gpio.h
+++ b/arch/mips/include/asm/mach-rc32434/gpio.h
@@ -80,10 +80,6 @@ struct rb532_gpio_reg {
 /* Compact Flash GPIO pin */
 #define CF_GPIO_NUM		13
 
-extern void set_434_reg(unsigned reg_offs, unsigned bit, unsigned len, unsigned val);
-extern unsigned get_434_reg(unsigned reg_offs);
-extern void set_latch_u5(unsigned char or_mask, unsigned char nand_mask);
-extern unsigned char get_latch_u5(void);
 extern void rb532_gpio_set_ilevel(int bit, unsigned gpio);
 extern void rb532_gpio_set_istat(int bit, unsigned gpio);
 
diff --git a/arch/mips/include/asm/mach-rc32434/rb.h b/arch/mips/include/asm/mach-rc32434/rb.h
index f25a849..6dc5f8d 100644
--- a/arch/mips/include/asm/mach-rc32434/rb.h
+++ b/arch/mips/include/asm/mach-rc32434/rb.h
@@ -83,4 +83,7 @@ struct mpmc_device {
 	void __iomem 	*base;
 };
 
+extern void set_latch_u5(unsigned char or_mask, unsigned char nand_mask);
+extern unsigned char get_latch_u5(void);
+
 #endif  /* __ASM_RC32434_RB_H */
-- 
1.5.6.4

Re: [PATCH] rb532: update headers

[Ralf Baechle]

On Thu, Jan 22, 2009 at 07:28:50PM +0100, Phil Sutter wrote:

Applied.

  Ralf

[PATCH] MIPS: rb532: export rb532_gpio_set_func()

[Phil Sutter]

This kernel symbol provides a way for drivers to switch on alternate
function for a certain GPIO pin. Turning it off is done implicitly when
changing the GPIO direction, as that would be fixed when using the given
pin als alternate function.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/include/asm/mach-rc32434/gpio.h |    1 +
 arch/mips/rb532/gpio.c                    |    5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/mach-rc32434/gpio.h b/arch/mips/include/asm/mach-rc32434/gpio.h
index ca92c81..3cb50d1 100644
--- a/arch/mips/include/asm/mach-rc32434/gpio.h
+++ b/arch/mips/include/asm/mach-rc32434/gpio.h
@@ -82,5 +82,6 @@ struct rb532_gpio_reg {
 
 extern void rb532_gpio_set_ilevel(int bit, unsigned gpio);
 extern void rb532_gpio_set_istat(int bit, unsigned gpio);
+extern void rb532_gpio_set_func(unsigned gpio);
 
 #endif /* _RC32434_GPIO_H_ */
diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index f338681..37de05d 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -174,10 +174,11 @@ EXPORT_SYMBOL(rb532_gpio_set_istat);
 /*
  * Configure GPIO alternate function
  */
-static void rb532_gpio_set_func(int bit, unsigned gpio)
+void rb532_gpio_set_func(unsigned gpio)
 {
-       rb532_set_bit(bit, gpio, rb532_gpio_chip->regbase + GPIOFUNC);
+       rb532_set_bit(1, gpio, rb532_gpio_chip->regbase + GPIOFUNC);
 }
+EXPORT_SYMBOL(rb532_gpio_set_func);
 
 int __init rb532_gpio_init(void)
 {
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: export rb532_gpio_set_func()

[Ralf Baechle]

On Thu, Jan 22, 2009 at 07:32:43PM +0100, Phil Sutter wrote:

Applied.

  Ralf