[PATCH] fix pata-rb532-cf

[PATCH] fix pata-rb532-cf

[Phil Sutter]

Hi,

I applied the latest comments to my set of changes for pata-rb532-cf, i.e.:
* add missing Acket-by statements
* switch order of the last two patches
* have some cosmetics for the 4-byte-blocks patch

The rb532_gpio_set_i{level,stat} symbols aren't upstream yet, though
Ralf Baechle has the according patch in his linux-mips tree (branch
master), so they should be available soon.

Greetings, Phil

---
After applying the following changes I could verify functionality by
mounting a filesystem on the cfdisk and reading/writing files in it.

The set_irq_type() function must be wrong, as there is no set_type()
function defined for the rb532 IRQ chip. But as the used IRQ actually is
being triggered by a GPIO, setting it's interrupt level should be the
right alternative. Also to clear a GPIO triggered IRQ, the source has to
be cleared. This is being done at the end of rb532_pata_irq_handler.

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

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index f8b3ffc..7b11f40 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -31,6 +31,7 @@
 #include <scsi/scsi_host.h>
 
 #include <asm/gpio.h>
+#include <asm/mach-rc32434/gpio.h>
 
 #define DRV_NAME	"pata-rb532-cf"
 #define DRV_VERSION	"0.1.0"
@@ -39,7 +40,8 @@
 #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
 
@@ -62,7 +64,7 @@ static inline void rb532_pata_finish_io(struct ata_port *ap)
 	ata_sff_dma_pause(ap);
 	ndelay(RB500_CF_IO_DELAY);
 
-	set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
+	rb532_gpio_set_ilevel(1, info->gpio_line);
 }
 
 static void rb532_pata_exec_command(struct ata_port *ap,
@@ -109,13 +111,15 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 	struct rb532_cf_info *info = ah->private_data;
 
 	if (gpio_get_value(info->gpio_line)) {
-		set_irq_type(info->irq, IRQ_TYPE_LEVEL_LOW);
+		rb532_gpio_set_ilevel(0, info->gpio_line);
 		if (!info->frozen)
 			ata_sff_interrupt(info->irq, dev_instance);
 	} else {
-		set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
+		rb532_gpio_set_ilevel(1, info->gpio_line);
 	}
 
+	rb532_gpio_set_istat(0, info->gpio_line);
+
 	return IRQ_HANDLED;
 }
 
@@ -146,13 +150,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.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 7b11f40..576fc99 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 |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index 576fc99..4479b04 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -43,7 +43,8 @@
 #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;
@@ -79,18 +80,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)
@@ -158,7 +155,7 @@ static void rb532_pata_setup_ports(struct ata_host *ah)
 
 	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;
 }
 
-- 
1.5.6.4

Re: [PATCH] fix pata-rb532-cf

[Sergei Shtylyov]

Hello.

Phil Sutter wrote:

> I applied the latest comments to my set of changes for pata-rb532-cf, i.e.:
> * add missing Acket-by statements
> * switch order of the last two patches
> * have some cosmetics for the 4-byte-blocks patch
>
> The rb532_gpio_set_i{level,stat} symbols aren't upstream yet, though
> Ralf Baechle has the according patch in his linux-mips tree (branch
> master), so they should be available soon.
>
> Greetings, Phil
>
> ---
> After applying the following changes I could verify functionality by
> mounting a filesystem on the cfdisk and reading/writing files in it.
>
> The set_irq_type() function must be wrong, as there is no set_type()
> function defined for the rb532 IRQ chip. But as the used IRQ actually is
> being triggered by a GPIO, setting it's interrupt level should be the
> right alternative. Also to clear a GPIO triggered IRQ, the source has to
> be cleared. This is being done at the end of rb532_pata_irq_handler.
>
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
> Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Acked-by: Florian Fainelli <florian@openwrt.org>
>   

   You mixed up the order: your comments about the recent patch changes 
should be below --- and the patch description/signoff/ACKs above it.

> diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
> index f8b3ffc..7b11f40 100644
> --- a/drivers/ata/pata_rb532_cf.c
> +++ b/drivers/ata/pata_rb532_cf.c
> @@ -31,6 +31,7 @@
>  #include <scsi/scsi_host.h>
>  
>  #include <asm/gpio.h>
> +#include <asm/mach-rc32434/gpio.h>
>  
>  #define DRV_NAME	"pata-rb532-cf"
>  #define DRV_VERSION	"0.1.0"
> @@ -39,7 +40,8 @@
>  #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
>   

   I think you need to split off the RB500_CF_REG_* changes as they're 
unrelated to IRQ trickery and can be applied right now.

> @@ -62,7 +64,7 @@ static inline void rb532_pata_finish_io(struct ata_port *ap)
>  	ata_sff_dma_pause(ap);
>  	ndelay(RB500_CF_IO_DELAY);
>  
> -	set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
> +	rb532_gpio_set_ilevel(1, info->gpio_line);
>  }
>  
>  static void rb532_pata_exec_command(struct ata_port *ap,
> @@ -109,13 +111,15 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
>  	struct rb532_cf_info *info = ah->private_data;
>  
>  	if (gpio_get_value(info->gpio_line)) {
> -		set_irq_type(info->irq, IRQ_TYPE_LEVEL_LOW);
> +		rb532_gpio_set_ilevel(0, info->gpio_line);
>  		if (!info->frozen)
>  			ata_sff_interrupt(info->irq, dev_instance);
>  	} else {
> -		set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
> +		rb532_gpio_set_ilevel(1, info->gpio_line);
>   

   I'm still perplexed with why this IRQ sensitivity trick is needed...

MBR, Sergei

[PATCH] fix pata-rb532-cf

[Phil Sutter]

After applying the following changes I could verify functionality by
mounting a filesystem on the cfdisk and reading/writing files in it.

The symbols rb532_gpio_set_ilevel and rb532_gpio_set_istat are not yet
available in a vanilla kernel, an appropriate patch has already been
sent to the linux-mips mailinglist.

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

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index f8b3ffc..a8b3c3a 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -39,9 +39,14 @@
 #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_CTRL	0x080E
 #define RB500_CF_REG_DATA	0x0C00
+#define RB500_CF_REG_ERR	0x080D
+
+/* exported in arch/mips/rb532/gpio.c */
+extern void rb532_gpio_set_ilevel(int bit, unsigned gpio);
+extern void rb532_gpio_set_istat(int bit, unsigned gpio);
 
 struct rb532_cf_info {
 	void __iomem	*iobase;
@@ -62,21 +67,23 @@ static inline void rb532_pata_finish_io(struct ata_port *ap)
 	ata_sff_dma_pause(ap);
 	ndelay(RB500_CF_IO_DELAY);
 
-	set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
+	rb532_gpio_set_ilevel(1, info->gpio_line);
 }
 
 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);
 }
 
-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;
+	unsigned int ret = buflen;
 
 	if (write_data) {
 		for (; buflen > 0; buflen--, buf++)
@@ -87,6 +94,8 @@ static void rb532_pata_data_xfer(struct ata_device *adev, unsigned char *buf,
 	}
 
 	rb532_pata_finish_io(adev->link->ap);
+
+	return ret;
 }
 
 static void rb532_pata_freeze(struct ata_port *ap)
@@ -109,13 +118,16 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 	struct rb532_cf_info *info = ah->private_data;
 
 	if (gpio_get_value(info->gpio_line)) {
-		set_irq_type(info->irq, IRQ_TYPE_LEVEL_LOW);
+		rb532_gpio_set_ilevel(0, info->gpio_line);
 		if (!info->frozen)
 			ata_sff_interrupt(info->irq, dev_instance);
 	} else {
-		set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
+		rb532_gpio_set_ilevel(1, info->gpio_line);
 	}
 
+	rb532_gpio_set_istat(0, info->gpio_line);
+
+
 	return IRQ_HANDLED;
 }
 
@@ -146,12 +158,13 @@ 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.error_addr	= info->iobase + RB500_CF_REG_ERR;
 	ap->ioaddr.data_addr	= info->iobase + RB500_CF_REG_DATA;
 }
 
-- 
1.5.6.4

Re: [PATCH] fix pata-rb532-cf

[Florian Fainelli]

Hi Phil,

Le Thursday 30 October 2008 22:47:31 Phil Sutter, vous avez écrit :
> After applying the following changes I could verify functionality by
> mounting a filesystem on the cfdisk and reading/writing files in it.
>
> The symbols rb532_gpio_set_ilevel and rb532_gpio_set_istat are not yet
> available in a vanilla kernel, an appropriate patch has already been
> sent to the linux-mips mailinglist.

Thanks a lot for fixing this.

>
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>

Acked-by: Florian Fainelli <florian@openwrt.org>
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------

Re: [PATCH] fix pata-rb532-cf

[Sergei Shtylyov]

Hello.

Phil Sutter wrote:

> After applying the following changes I could verify functionality by
> mounting a filesystem on the cfdisk and reading/writing files in it.
>   

   Some of the changes look unneeded and some just strange...

> The symbols rb532_gpio_set_ilevel and rb532_gpio_set_istat are not yet
> available in a vanilla kernel, an appropriate patch has already been
> sent to the linux-mips mailinglist.
>
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
> ---
>  drivers/ata/pata_rb532_cf.c |   25 +++++++++++++++++++------
>  1 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
> index f8b3ffc..a8b3c3a 100644
> --- a/drivers/ata/pata_rb532_cf.c
> +++ b/drivers/ata/pata_rb532_cf.c
> @@ -39,9 +39,14 @@
>  #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_CTRL	0x080E
>  #define RB500_CF_REG_DATA	0x0C00
> +#define RB500_CF_REG_ERR	0x080D
>   

   Hm...

> +/* exported in arch/mips/rb532/gpio.c */
> +extern void rb532_gpio_set_ilevel(int bit, unsigned gpio);
> +extern void rb532_gpio_set_istat(int bit, unsigned gpio);
>   

   Those should better be in some header file...

> @@ -62,21 +67,23 @@ static inline void rb532_pata_finish_io(struct ata_port *ap)
>  static void rb532_pata_exec_command(struct ata_port *ap,
>  				const struct ata_taskfile *tf)
>  {
> +
>   

   Totally unneeded change.

>  	writeb(tf->command, ap->ioaddr.command_addr);
>  	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;
> +	unsigned int ret = buflen;
>  
>  	if (write_data) {
>  		for (; buflen > 0; buflen--, buf++)
> @@ -87,6 +94,8 @@ static void rb532_pata_data_xfer(struct ata_device *adev, unsigned char *buf,
>  	}
>  
>  	rb532_pata_finish_io(adev->link->ap);
> +
> +	return ret;
>   

   Totally unneeded variable.

> @@ -109,13 +118,16 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
>  	struct rb532_cf_info *info = ah->private_data;
>  
>  	if (gpio_get_value(info->gpio_line)) {
> -		set_irq_type(info->irq, IRQ_TYPE_LEVEL_LOW);
> +		rb532_gpio_set_ilevel(0, info->gpio_line);
>  		if (!info->frozen)
>  			ata_sff_interrupt(info->irq, dev_instance);
>  	} else {
> -		set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
> +		rb532_gpio_set_ilevel(1, info->gpio_line);
>  	}
>  
> +	rb532_gpio_set_istat(0, info->gpio_line);
> +
> +
>   

  One line too many...

> @@ -146,12 +158,13 @@ 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.error_addr	= info->iobase + RB500_CF_REG_ERR;
>   

   Wait, how's that?! The error register belongs to the 8-register 
command block, not to the control block.

MBR, Sergei

Re: [PATCH] fix pata-rb532-cf

[Phil Sutter]

Hi,

On Fri, Oct 31, 2008 at 02:20:54AM +0300, Sergei Shtylyov wrote:
>   Some of the changes look unneeded and some just strange...

you're completely right, my apologies for that.

> >-#define RB500_CF_REG_CMD	0x0800
> >+#define RB500_CF_REG_BASE	0x0800
> > #define RB500_CF_REG_CTRL	0x080E
> > #define RB500_CF_REG_DATA	0x0C00
> >+#define RB500_CF_REG_ERR	0x080D
> >  
> 
>   Hm...

The original driver (see my patch) accesses 0x80D to read error codes,
so I assumed this is correct. Indeed, the driver works also with the
standard setting. Is there a way I can clearly verify which is the right
offset?

> >-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;
> >+	unsigned int ret = buflen;
> > 
> > 	if (write_data) {
> > 		for (; buflen > 0; buflen--, buf++)
> >@@ -87,6 +94,8 @@ static void rb532_pata_data_xfer(struct ata_device 
> >*adev, unsigned char *buf,
> > 	}
> > 
> > 	rb532_pata_finish_io(adev->link->ap);
> >+
> >+	return ret;
> >  
> 
>   Totally unneeded variable.

I wanted to preserve the behaviour of ata_sff_data_xfer(), i.e.
returning the number of bytes written. The variable exists, because
buflen is being decremented in the loop and therefore zero afterwards.
Returning a static value also should be no alternative, as it could lead
to unexpected behaviour on the caller side. So, do I miss something?

Greetings, Phil

Re: [PATCH] fix pata-rb532-cf

[Sergei Shtylyov]

Hello.

Phil Sutter wrote:

>>> -#define RB500_CF_REG_CMD	0x0800
>>> +#define RB500_CF_REG_BASE	0x0800
>>> #define RB500_CF_REG_CTRL	0x080E
>>> #define RB500_CF_REG_DATA	0x0C00
>>> +#define RB500_CF_REG_ERR	0x080D
>>>  
>>>       
>>   Hm...
>>     
>
> The original driver (see my patch) accesses 0x80D to read error codes,
> so I assumed this is correct. Indeed, the driver works also with the
> standard setting. Is there a way I can clearly verify which is the right
> offset?
>   

   I assume you don't have the documentation? I wodner why they needed 
to duplicate (or remap) this register...

>>> -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;
>>> +	unsigned int ret = buflen;
>>>
>>> 	if (write_data) {
>>> 		for (; buflen > 0; buflen--, buf++)
>>> @@ -87,6 +94,8 @@ static void rb532_pata_data_xfer(struct ata_device 
>>> *adev, unsigned char *buf,
>>> 	}
>>>
>>> 	rb532_pata_finish_io(adev->link->ap);
>>> +
>>> +	return ret;
>>>  
>>>       
>>   Totally unneeded variable.
>>     
>
> I wanted to preserve the behaviour of ata_sff_data_xfer(), i.e.
> returning the number of bytes written. The variable exists, because
> buflen is being decremented in the loop and therefore zero afterwards.
>   

   Oops, sorry -- I've managed to miss that decrement... :-<

> Returning a static value also should be no alternative, as it could lead
> to unexpected behaviour on the caller side. So, do I miss something?
>   

   No. The alternatives would be to use local variable as a loop counter 
or, better yet, using readsb()/writesb() instead of the loops...
Wait! The original driver used 32-bit I/O to this register, not 8-bit -- 
so it looks like you have artificially slowed it down... :-/

MBR, Sergei

Re: [PATCH] fix pata-rb532-cf

[Sergei Shtylyov]

Hello, I wrote:

>>>> -#define RB500_CF_REG_CMD    0x0800
>>>> +#define RB500_CF_REG_BASE    0x0800
>>>> #define RB500_CF_REG_CTRL    0x080E
>>>> #define RB500_CF_REG_DATA    0x0C00

   This is actually not the IDE data register itself but some data 
buffer register and supporting 32-bit I/O (and presumably implementing 
read prefetch)...

>>>> -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;
>>>> +    unsigned int ret = buflen;
>>>>
>>>>     if (write_data) {
>>>>         for (; buflen > 0; buflen--, buf++)
>>>> @@ -87,6 +94,8 @@ static void rb532_pata_data_xfer(struct 
>>>> ata_device *adev, unsigned char *buf,
>>>>     }
>>>>
>>>>     rb532_pata_finish_io(adev->link->ap);
>>>> +
>>>> +    return ret;
>>>>  
>>>>       
>>>   Totally unneeded variable.
>>>     
>>
>> I wanted to preserve the behaviour of ata_sff_data_xfer(), i.e.
>> returning the number of bytes written. The variable exists, because
>> buflen is being decremented in the loop and therefore zero afterwards.
>>   
>
>   Oops, sorry -- I've managed to miss that decrement... :-<
>
>> Returning a static value also should be no alternative, as it could lead
>> to unexpected behaviour on the caller side. So, do I miss something?
>>   
>
>   No. The alternatives would be to use local variable as a loop 
> counter or, better yet, using readsb()/writesb() instead of the loops...
> Wait! The original driver used 32-bit I/O to this register, not 8-bit 
> -- so it looks like you have artificially slowed it down... :-/

   Sorry again -- looks like it was Florian who was the author of the 
original driver...

MBR, Sergei

Re: [PATCH] fix pata-rb532-cf

[Phil Sutter]

Hi,

On Fri, Oct 31, 2008 at 01:38:27PM +0300, Sergei Shtylyov wrote:
> >The original driver (see my patch) accesses 0x80D to read error codes,
> >so I assumed this is correct. Indeed, the driver works also with the
> >standard setting. Is there a way I can clearly verify which is the right
> >offset?
> >  
> 
>   I assume you don't have the documentation? I wodner why they needed 
> to duplicate (or remap) this register...

Sadly not. All I have is the specification of the SoC by IDT, but the
CompactFlash slot was added by Mikrotik, so it's not mentioned in there.
The routerboard user's manual Mikrotik has published doesn't contain
much more information about it than "it's there".

>   No. The alternatives would be to use local variable as a loop counter 
> or, better yet, using readsb()/writesb() instead of the loops...
> Wait! The original driver used 32-bit I/O to this register, not 8-bit -- 
> so it looks like you have artificially slowed it down... :-/

Ok, so this should be clear now. I changed the code to use readl() and
writel() (the reads*() and writes*() versions sadly aren't useable as
they increment the target memory pointer) which worked fine.

When it comes to the register offsets I'd say in dubio pro reo and keep
the custom error address. Changing it afterwards to the standard should
also be much easier than fiddling out what the custom offset was again.

I'll reply to this mail with an updated patch addressing all discussed
problems.

Greetings and many thanks, Phil

[PATCH] fix pata-rb532-cf

[Phil Sutter]

After applying the following changes I could verify functionality by
mounting a filesystem on the cfdisk and reading/writing files in it.

The symbols rb532_gpio_set_ilevel and rb532_gpio_set_istat are not yet
available in a vanilla kernel, an appropriate patch has already been
sent to the linux-mips mailinglist.

Also change rb532_pata_data_xfer() so it reads and writes 4-byte blocks,
like the original driver did. Rename the offset definition of the
buffered data register for clearness.

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

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index f8b3ffc..bdf413e 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -31,6 +31,7 @@
 #include <scsi/scsi_host.h>
 
 #include <asm/gpio.h>
+#include <asm/mach-rc32434/gpio.h>
 
 #define DRV_NAME	"pata-rb532-cf"
 #define DRV_VERSION	"0.1.0"
@@ -39,9 +40,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_CTRL	0x080E
-#define RB500_CF_REG_DATA	0x0C00
+/* 32bit buffered data register offset */
+#define RB500_CF_REG_DBUF32	0x0C00
+#define RB500_CF_REG_ERR	0x080D
 
 struct rb532_cf_info {
 	void __iomem	*iobase;
@@ -62,7 +65,7 @@ static inline void rb532_pata_finish_io(struct ata_port *ap)
 	ata_sff_dma_pause(ap);
 	ndelay(RB500_CF_IO_DELAY);
 
-	set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
+	rb532_gpio_set_ilevel(1, info->gpio_line);
 }
 
 static void rb532_pata_exec_command(struct ata_port *ap,
@@ -72,21 +75,26 @@ 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)
 {
+	int i;
 	struct ata_port *ap = adev->link->ap;
 	void __iomem *ioaddr = ap->ioaddr.data_addr;
 
+	BUG_ON(buflen % sizeof(u32));
+
 	if (write_data) {
-		for (; buflen > 0; buflen--, buf++)
-			writeb(*buf, ioaddr);
+		for(i = 0; i < buflen / sizeof(u32); i++)
+			writel(((u32 *)buf)[i], ioaddr);
 	} else {
-		for (; buflen > 0; buflen--, buf++)
-			*buf = readb(ioaddr);
+		for(i = 0; i < buflen / sizeof(u32); i++)
+			((u32 *)buf)[i] = readl(ioaddr);
 	}
 
 	rb532_pata_finish_io(adev->link->ap);
+
+	return buflen;
 }
 
 static void rb532_pata_freeze(struct ata_port *ap)
@@ -109,13 +117,15 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 	struct rb532_cf_info *info = ah->private_data;
 
 	if (gpio_get_value(info->gpio_line)) {
-		set_irq_type(info->irq, IRQ_TYPE_LEVEL_LOW);
+		rb532_gpio_set_ilevel(0, info->gpio_line);
 		if (!info->frozen)
 			ata_sff_interrupt(info->irq, dev_instance);
 	} else {
-		set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
+		rb532_gpio_set_ilevel(1, info->gpio_line);
 	}
 
+	rb532_gpio_set_istat(0, info->gpio_line);
+
 	return IRQ_HANDLED;
 }
 
@@ -146,13 +156,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.error_addr	= info->iobase + RB500_CF_REG_ERR;
+	ap->ioaddr.data_addr	= info->iobase + RB500_CF_REG_DBUF32;
 }
 
 static __devinit int rb532_pata_driver_probe(struct platform_device *pdev)
-- 
1.5.6.4

Re: [PATCH] fix pata-rb532-cf

[Sergei Shtylyov]

Hello.

Phil Sutter wrote:

> After applying the following changes I could verify functionality by
> mounting a filesystem on the cfdisk and reading/writing files in it.
>
> The symbols rb532_gpio_set_ilevel and rb532_gpio_set_istat are not yet
> available in a vanilla kernel, an appropriate patch has already been
> sent to the linux-mips mailinglist.
>
> Also change rb532_pata_data_xfer() so it reads and writes 4-byte blocks,
> like the original driver did. Rename the offset definition of the
> buffered data register for clearness.
>   


   Looks ike I'll have to NAK this part...

> diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
> index f8b3ffc..bdf413e 100644
> --- a/drivers/ata/pata_rb532_cf.c
> +++ b/drivers/ata/pata_rb532_cf.c
>   
[...]
> @@ -39,9 +40,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_CTRL	0x080E
> -#define RB500_CF_REG_DATA	0x0C00
> +/* 32bit buffered data register offset */
> +#define RB500_CF_REG_DBUF32	0x0C00
> +#define RB500_CF_REG_ERR	0x080D
>   

   Wouldn't hurt to have the macros in the ascending address order...

> @@ -72,21 +75,26 @@ 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)
>  {
> +	int i;
>  	struct ata_port *ap = adev->link->ap;
>  	void __iomem *ioaddr = ap->ioaddr.data_addr;
>  
> +	BUG_ON(buflen % sizeof(u32));
> +
>  	if (write_data) {
> -		for (; buflen > 0; buflen--, buf++)
> -			writeb(*buf, ioaddr);
> +		for(i = 0; i < buflen / sizeof(u32); i++)
> +			writel(((u32 *)buf)[i], ioaddr);
>  	} else {
> -		for (; buflen > 0; buflen--, buf++)
> -			*buf = readb(ioaddr);
> +		for(i = 0; i < buflen / sizeof(u32); i++)
> +			((u32 *)buf)[i] = readl(ioaddr);
>  	}

   So, I didn't get what was wrong with using readsl() and writesl()?
   Besides, using readl() and witel() this way would be wrong on BE mode 
since the data is expected to be stored to memory in the LE order.

MBR, Sergei

Re: [PATCH] fix pata-rb532-cf

[Sergei Shtylyov]

Hello, I wrote:

>> After applying the following changes I could verify functionality by
>> mounting a filesystem on the cfdisk and reading/writing files in it.
>>
>> The symbols rb532_gpio_set_ilevel and rb532_gpio_set_istat are not yet
>> available in a vanilla kernel, an appropriate patch has already been
>> sent to the linux-mips mailinglist.
>>
>> Also change rb532_pata_data_xfer() so it reads and writes 4-byte blocks,
>> like the original driver did. Rename the offset definition of the
>> buffered data register for clearness.
>>   
>
>   Looks ike I'll have to NAK this part...

   I'd advise to break up the patch since you're fixing 2 unrelated 
things now...

>> index f8b3ffc..bdf413e 100644
>> --- a/drivers/ata/pata_rb532_cf.c
>> +++ b/drivers/ata/pata_rb532_cf.c
>>   
> diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
> [...]
>> @@ -39,9 +40,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_CTRL    0x080E
>> -#define RB500_CF_REG_DATA    0x0C00
>> +/* 32bit buffered data register offset */
>> +#define RB500_CF_REG_DBUF32    0x0C00
>> +#define RB500_CF_REG_ERR    0x080D
>>   
>
>   Wouldn't hurt to have the macros in the ascending address order...
>
>> @@ -72,21 +75,26 @@ 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)
>>  {
>> +    int i;

  I'd have made this the last variable in the declartion block...

>>      struct ata_port *ap = adev->link->ap;
>>      void __iomem *ioaddr = ap->ioaddr.data_addr;
>>  
>> +    BUG_ON(buflen % sizeof(u32));

   Well, if there's no chance for an ATAPI device to be connected...

>> +
>>      if (write_data) {
>> -        for (; buflen > 0; buflen--, buf++)
>> -            writeb(*buf, ioaddr);
>> +        for(i = 0; i < buflen / sizeof(u32); i++)
>> +            writel(((u32 *)buf)[i], ioaddr);
>>      } else {
>> -        for (; buflen > 0; buflen--, buf++)
>> -            *buf = readb(ioaddr);
>> +        for(i = 0; i < buflen / sizeof(u32); i++)
>> +            ((u32 *)buf)[i] = readl(ioaddr);
>>      }
>
>   So, I didn't get what was wrong with using readsl() and writesl()?
>   Besides, using readl() and witel() this way would be wrong on BE 
> mode since the data is expected to be stored to memory in the LE order.
>

   Hm, I'seeing that RB532 only supports LE it seems -- though I wodner 
why it also selects CONFIG_SWAP_IO_SPACE which shouldn't affect anything 
in LE mode...
Anyway, using readl()/writel() this way is wrong...

MBR, Sergei

Re: [PATCH] fix pata-rb532-cf

[Phil Sutter]

Hi,

On Sat, Nov 01, 2008 at 07:26:34PM +0300, Sergei Shtylyov wrote:
>   So, I didn't get what was wrong with using readsl() and writesl()?
>   Besides, using readl() and witel() this way would be wrong on BE mode 
> since the data is expected to be stored to memory in the LE order.

Sorry, my fault. When having a glance at the macro definition, I messed
up the parameter's meanings and assumed the target address would be
incremented, which is normally the case when writing regular memory, not
to IO addresses.

I changed (and successfully tested) the code, patch follows.

Thanks again, Phil

[PATCH] fix pata-rb532-cf

[Phil Sutter]

After applying the following changes I could verify functionality by
mounting a filesystem on the cfdisk and reading/writing files in it.

The symbols rb532_gpio_set_ilevel and rb532_gpio_set_istat are not yet
available in a vanilla kernel, an appropriate patch has already been
sent to the linux-mips mailinglist.

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

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index f8b3ffc..7b11f40 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -31,6 +31,7 @@
 #include <scsi/scsi_host.h>
 
 #include <asm/gpio.h>
+#include <asm/mach-rc32434/gpio.h>
 
 #define DRV_NAME	"pata-rb532-cf"
 #define DRV_VERSION	"0.1.0"
@@ -39,7 +40,8 @@
 #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
 
@@ -62,7 +64,7 @@ static inline void rb532_pata_finish_io(struct ata_port *ap)
 	ata_sff_dma_pause(ap);
 	ndelay(RB500_CF_IO_DELAY);
 
-	set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
+	rb532_gpio_set_ilevel(1, info->gpio_line);
 }
 
 static void rb532_pata_exec_command(struct ata_port *ap,
@@ -109,13 +111,15 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 	struct rb532_cf_info *info = ah->private_data;
 
 	if (gpio_get_value(info->gpio_line)) {
-		set_irq_type(info->irq, IRQ_TYPE_LEVEL_LOW);
+		rb532_gpio_set_ilevel(0, info->gpio_line);
 		if (!info->frozen)
 			ata_sff_interrupt(info->irq, dev_instance);
 	} else {
-		set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
+		rb532_gpio_set_ilevel(1, info->gpio_line);
 	}
 
+	rb532_gpio_set_istat(0, info->gpio_line);
+
 	return IRQ_HANDLED;
 }
 
@@ -146,13 +150,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.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] fix pata-rb532-cf

[Sergei Shtylyov]

Hello.

Phil Sutter wrote:

> After applying the following changes I could verify functionality by
> mounting a filesystem on the cfdisk and reading/writing files in it.
>
> The symbols rb532_gpio_set_ilevel and rb532_gpio_set_istat are not yet
> available in a vanilla kernel, an appropriate patch has already been
> sent to the linux-mips mailinglist.
>
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
[...]
> @@ -62,7 +64,7 @@ static inline void rb532_pata_finish_io(struct ata_port *ap)
>  	ata_sff_dma_pause(ap);
>  	ndelay(RB500_CF_IO_DELAY);
>  
> -	set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
> +	rb532_gpio_set_ilevel(1, info->gpio_line);
>  }
>  
>  static void rb532_pata_exec_command(struct ata_port *ap,
> @@ -109,13 +111,15 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
>  	struct rb532_cf_info *info = ah->private_data;
>  
>  	if (gpio_get_value(info->gpio_line)) {
> -		set_irq_type(info->irq, IRQ_TYPE_LEVEL_LOW);
> +		rb532_gpio_set_ilevel(0, info->gpio_line);
>  		if (!info->frozen)
>  			ata_sff_interrupt(info->irq, dev_instance);
>  	} else {
> -		set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
> +		rb532_gpio_set_ilevel(1, info->gpio_line);
>   

   I wonder why is this IRQ level switching magic is necessary... I'd 
think that the interrupt trigger level should be programmed only once, 
at init time. Ah, you're masking them on/off that way...

MBR, Sergei

Re: [PATCH] fix pata-rb532-cf

[Phil Sutter]

Hi,

On Mon, Nov 03, 2008 at 01:04:28AM +0300, Sergei Shtylyov wrote:
> >After applying the following changes I could verify functionality by
> >mounting a filesystem on the cfdisk and reading/writing files in it.
> >
> >The symbols rb532_gpio_set_ilevel and rb532_gpio_set_istat are not yet
> >available in a vanilla kernel, an appropriate patch has already been
> >sent to the linux-mips mailinglist.
> >
> >Signed-off-by: Phil Sutter <n0-1@freewrt.org>
> [...]
> >@@ -62,7 +64,7 @@ static inline void rb532_pata_finish_io(struct ata_port 
> >*ap)
> > 	ata_sff_dma_pause(ap);
> > 	ndelay(RB500_CF_IO_DELAY);
> > 
> >-	set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
> >+	rb532_gpio_set_ilevel(1, info->gpio_line);
> > }
> > 
> > static void rb532_pata_exec_command(struct ata_port *ap,
> >@@ -109,13 +111,15 @@ static irqreturn_t rb532_pata_irq_handler(int irq, 
> >void *dev_instance)
> > 	struct rb532_cf_info *info = ah->private_data;
> > 
> > 	if (gpio_get_value(info->gpio_line)) {
> >-		set_irq_type(info->irq, IRQ_TYPE_LEVEL_LOW);
> >+		rb532_gpio_set_ilevel(0, info->gpio_line);
> > 		if (!info->frozen)
> > 			ata_sff_interrupt(info->irq, dev_instance);
> > 	} else {
> >-		set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
> >+		rb532_gpio_set_ilevel(1, info->gpio_line);
> >  
> 
>   I wonder why is this IRQ level switching magic is necessary... I'd 
> think that the interrupt trigger level should be programmed only once, 
> at init time. Ah, you're masking them on/off that way...

Well, that's not really clear to me. But likely the interrupt level register
turns the interrupt source functionality of the GPIOs on and off, as you
say. As a little exerpt of what I've gone through the last days:

| GPIO Interrupt Level. When the value of a GPIO pin matches the value of
| the corresponding bit in this field, the corresponding bit is set in the
| GPIOISTAT field is set.

It's maybe because of my english, but I can only guess what is meant
here.

I'm at least sure that set_irq_type() is wrong. The lack of a set_type()
function should make them expand to no ops. IIRC the idea for this
change came by looking at the callers of prepare_cf_irq() in the
standalone driver, combined with a lot of trial and error. :)

Greetings, Phil

Re: [PATCH] fix pata-rb532-cf

[Sergei Shtylyov]

Hello.

Phil Sutter wrote:

>>   No. The alternatives would be to use local variable as a loop counter 
>> or, better yet, using readsb()/writesb() instead of the loops...
>> Wait! The original driver used 32-bit I/O to this register, not 8-bit -- 
>> so it looks like you have artificially slowed it down... :-/
>>     
>
> Ok, so this should be clear now. I changed the code to use readl() and
> writel() (the reads*() and writes*() versions sadly aren't useable as
> they increment the target memory pointer) which worked fine.
>   

   So what? This is perfectly valid. Doesn't the current code increment it?

MBR, Sergei

[PATCH] fix pata-rb532-cf

[Phil Sutter]

The following patch series is a complete updated set of changes I did to
the pata-rb532-cf driver. In ordered detail: fix functionality, restore
the behaviour of the original driver when doing transfers and eliminate
compiler warnings.

Greetings, Phil

---

After applying the following changes I could verify functionality by
mounting a filesystem on the cfdisk and reading/writing files in it.

The set_irq_type() function must be wrong, as there is no set_type()
function defined for the rb532 IRQ chip. But as the used IRQ actually is
being triggered by a GPIO, setting it's interrupt level should be the
right alternative. Also to clear a GPIO triggered IRQ, the source has to
be cleared. This is being done at the end of rb532_pata_irq_handler.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
Acked-by: Florian Fainelli <florian@openwrt.org>
---
 drivers/ata/pata_rb532_cf.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index f8b3ffc..7b11f40 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -31,6 +31,7 @@
 #include <scsi/scsi_host.h>
 
 #include <asm/gpio.h>
+#include <asm/mach-rc32434/gpio.h>
 
 #define DRV_NAME	"pata-rb532-cf"
 #define DRV_VERSION	"0.1.0"
@@ -39,7 +40,8 @@
 #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
 
@@ -62,7 +64,7 @@ static inline void rb532_pata_finish_io(struct ata_port *ap)
 	ata_sff_dma_pause(ap);
 	ndelay(RB500_CF_IO_DELAY);
 
-	set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
+	rb532_gpio_set_ilevel(1, info->gpio_line);
 }
 
 static void rb532_pata_exec_command(struct ata_port *ap,
@@ -109,13 +111,15 @@ static irqreturn_t rb532_pata_irq_handler(int irq, void *dev_instance)
 	struct rb532_cf_info *info = ah->private_data;
 
 	if (gpio_get_value(info->gpio_line)) {
-		set_irq_type(info->irq, IRQ_TYPE_LEVEL_LOW);
+		rb532_gpio_set_ilevel(0, info->gpio_line);
 		if (!info->frozen)
 			ata_sff_interrupt(info->irq, dev_instance);
 	} else {
-		set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
+		rb532_gpio_set_ilevel(1, info->gpio_line);
 	}
 
+	rb532_gpio_set_istat(0, info->gpio_line);
+
 	return IRQ_HANDLED;
 }
 
@@ -146,13 +150,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.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] fix pata-rb532-cf

[Bartlomiej Zolnierkiewicz]

On Wednesday 12 November 2008, Phil Sutter wrote:
> The following patch series is a complete updated set of changes I did to
> the pata-rb532-cf driver. In ordered detail: fix functionality, restore
> the behaviour of the original driver when doing transfers and eliminate
> compiler warnings.

Looks good to me and you may add

	Acked-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

from me but libata patches should go via Jeff (added to cc:).

Thanks,
Bart

Re: [PATCH] fix pata-rb532-cf

[Jeff Garzik]

Phil Sutter wrote:
> The following patch series is a complete updated set of changes I did to
> the pata-rb532-cf driver. In ordered detail: fix functionality, restore
> the behaviour of the original driver when doing transfers and eliminate
> compiler warnings.
> 
> Greetings, Phil
> 
> ---
> 
> After applying the following changes I could verify functionality by
> mounting a filesystem on the cfdisk and reading/writing files in it.
> 
> The set_irq_type() function must be wrong, as there is no set_type()
> function defined for the rb532 IRQ chip. But as the used IRQ actually is
> being triggered by a GPIO, setting it's interrupt level should be the
> right alternative. Also to clear a GPIO triggered IRQ, the source has to
> be cleared. This is being done at the end of rb532_pata_irq_handler.
> 
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
> Acked-by: Florian Fainelli <florian@openwrt.org>
> ---
>  drivers/ata/pata_rb532_cf.c |   15 ++++++++++-----
>  1 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
> index f8b3ffc..7b11f40 100644
> --- a/drivers/ata/pata_rb532_cf.c
> +++ b/drivers/ata/pata_rb532_cf.c
> @@ -31,6 +31,7 @@
>  #include <scsi/scsi_host.h>
>  
>  #include <asm/gpio.h>
> +#include <asm/mach-rc32434/gpio.h>
>  
>  #define DRV_NAME	"pata-rb532-cf"
>  #define DRV_VERSION	"0.1.0"
> @@ -39,7 +40,8 @@
>  #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
>  
> @@ -62,7 +64,7 @@ static inline void rb532_pata_finish_io(struct ata_port *ap)
>  	ata_sff_dma_pause(ap);
>  	ndelay(RB500_CF_IO_DELAY);
>  
> -	set_irq_type(info->irq, IRQ_TYPE_LEVEL_HIGH);
> +	rb532_gpio_set_ilevel(1, info->gpio_line);

I'll need to wait for this function to appear upstream... it's not in 
2.6.28-rc at least

Re: [PATCH] fix pata-rb532-cf

[Florian Fainelli]

Hello Jeff,

Le Saturday 15 November 2008 00:53:44 Jeff Garzik, vous avez écrit :
> I'll need to wait for this function to appear upstream... it's not in
> 2.6.28-rc at least

I just checked that you now got this function in your libata-dev tree, this 
patch still applies, can you merge it ? Thanks a lot.
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------

Re: [PATCH] fix pata-rb532-cf

[Phil Sutter]

Hi,

On Sun, Jan 11, 2009 at 05:13:28PM +0100, Florian Fainelli wrote:
> Hello Jeff,
> 
> Le Saturday 15 November 2008 00:53:44 Jeff Garzik, vous avez écrit :
> > I'll need to wait for this function to appear upstream... it's not in
> > 2.6.28-rc at least
> 
> I just checked that you now got this function in your libata-dev tree, this 
> patch still applies, can you merge it ? Thanks a lot.

No, please not! In fact these changes are obsolete and useless, see my
later mail to the linux-ide list.
(Message-ID: 20081128193322.D103C386DBBE@mail.ifyouseekate.net)

Accessing the IRQ this generic way is fine, we just needed code for the
GPIO mapped IRQs to be handled correctly.

Greetings, Phil

Re: [PATCH] fix pata-rb532-cf

[Florian Fainelli]

Hi Phil

Le Sunday 11 January 2009 21:47:07 Phil Sutter, vous avez écrit :
> No, please not! In fact these changes are obsolete and useless, see my
> later mail to the linux-ide list.
> (Message-ID: 20081128193322.D103C386DBBE@mail.ifyouseekate.net)
>
> Accessing the IRQ this generic way is fine, we just needed code for the
> GPIO mapped IRQs to be handled correctly.

Right, I now remember. Is the pata_rb532_cf in Jeff's tree missing any fix 
from you ?
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------

Re: [PATCH] fix pata-rb532-cf

[Phil Sutter]

Hi,

On Mon, Jan 12, 2009 at 03:32:34PM +0100, Florian Fainelli wrote:
> Right, I now remember. Is the pata_rb532_cf in Jeff's tree missing any fix 
> from you ?

AFAIK this IO optimisation patch is still waiting to be accepted
(Message-ID <20081128194607.93677386B1A7@mail.ifyouseekate.net>). I
think you also have it in OpenWrt trunk. Maybe some performance
measurement with/without it could help to clarify if it's useful or not.

Greetings, Phil