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] 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

[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

[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

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

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

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

[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

[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

[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

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

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

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)

[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

[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

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

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