[PATCH] fix pata-rb532-cf

[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

[PATCH] read and 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
* use writesl() and readsl() which implicitly iterate over the data

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

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index 7b11f40..0ae377f 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;
@@ -80,15 +81,13 @@ static void rb532_pata_data_xfer(struct ata_device *adev, unsigned char *buf,
 	struct ata_port *ap = adev->link->ap;
 	void __iomem *ioaddr = ap->ioaddr.data_addr;
 
-	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 buflen;
 }
 
 static void rb532_pata_freeze(struct ata_port *ap)
@@ -156,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

[PATCH] read and write data in 4-byte blocks

[Phil Sutter]

I forgot to include the one line changing rb532_pata_data_xfer()'s signature,
sorry for that. So here's the correct version.

* rename the offset definition to avoid abiguity with the standard ATA
  IO address
* read and write four bytes at once
* use writesl() and readsl() which implicitly iterate over the data
* fix the signature of rb532_pata_data_xfer() to mach the function
  pointer definition and return the number of bytes consumed

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

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index 7b11f40..e265577 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;
@@ -74,21 +75,19 @@ 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;
 
-	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 buflen;
 }
 
 static void rb532_pata_freeze(struct ata_port *ap)
@@ -156,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] read and write data in 4-byte blocks

[Sergei Shtylyov]

Hello.

Phil Sutter wrote:

> I forgot to include the one line changing rb532_pata_data_xfer()'s signature,
> sorry for that. So here's the correct version.
>
> * rename the offset definition to avoid abiguity with the standard ATA
>   IO address
> * read and write four bytes at once
> * use writesl() and readsl() which implicitly iterate over the data
> * fix the signature of rb532_pata_data_xfer() to mach the function
>   

   s/mach/match/

>   pointer definition and return the number of bytes consumed
>
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
>   

Acked-by: Sergei Shtyltov <sshtylyov@ru.mvista.com>

MBR, Sergei

[PATCH] pata-rb532-cf: read and 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
* use writesl() and readsl() which implicitly iterate over the data
* fix the signature of rb532_pata_data_xfer() to match the function
  pointer definition and return the number of bytes consumed

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
Acked-by: Sergei Shtyltov <sshtylyov@ru.mvista.com>
---
 drivers/ata/pata_rb532_cf.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index 7b11f40..a220992 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;
@@ -74,21 +75,19 @@ 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,
-				unsigned int buflen, int write_data)
+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) {
-		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 buflen;
 }
 
 static void rb532_pata_freeze(struct ata_port *ap)
@@ -156,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] pata-rb532-cf: read and write data in 4-byte blocks

[Bartlomiej Zolnierkiewicz]

On Monday 03 November 2008, Phil Sutter wrote:
> * rename the offset definition to avoid abiguity with the standard ATA
>   IO address
> * read and write four bytes at once
> * use writesl() and readsl() which implicitly iterate over the data
> * fix the signature of rb532_pata_data_xfer() to match the function
>   pointer definition and return the number of bytes 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>

Jeff, please apply.

> ---
>  drivers/ata/pata_rb532_cf.c |   21 ++++++++++-----------
>  1 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
> index 7b11f40..a220992 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;
> @@ -74,21 +75,19 @@ 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,
> -				unsigned int buflen, int write_data)
> +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) {
> -		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 buflen;
>  }
>  
>  static void rb532_pata_freeze(struct ata_port *ap)
> @@ -156,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;
>  }

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

[Jeff Garzik]

Phil Sutter wrote:
> * rename the offset definition to avoid abiguity with the standard ATA
>   IO address
> * read and write four bytes at once
> * use writesl() and readsl() which implicitly iterate over the data
> * fix the signature of rb532_pata_data_xfer() to match the function
>   pointer definition and return the number of bytes consumed
> 
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
> Acked-by: Sergei Shtyltov <sshtylyov@ru.mvista.com>
> ---
>  drivers/ata/pata_rb532_cf.c |   21 ++++++++++-----------
>  1 files changed, 10 insertions(+), 11 deletions(-)

None of this description says _why_ you wish to do this.  Also, maybe I 
missed the response, what about ATAPI?

	Jeff

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

[Phil Sutter]

Hi,

On Tue, Nov 04, 2008 at 01:01:25AM -0500, Jeff Garzik wrote:
> Phil Sutter wrote:
> >* rename the offset definition to avoid abiguity with the standard ATA
> >  IO address
> >* read and write four bytes at once
> >* use writesl() and readsl() which implicitly iterate over the data
> >* fix the signature of rb532_pata_data_xfer() to match the function
> >  pointer definition and return the number of bytes consumed
> >
> >Signed-off-by: Phil Sutter <n0-1@freewrt.org>
> >Acked-by: Sergei Shtyltov <sshtylyov@ru.mvista.com>
> >---
> > drivers/ata/pata_rb532_cf.c |   21 ++++++++++-----------
> > 1 files changed, 10 insertions(+), 11 deletions(-)
> 
> None of this description says _why_ you wish to do this.  Also, maybe I 
> missed the response, what about ATAPI?

Well, Sergei Shtylyov pointed out that the original driver does transfer
data at 4-byte blocks, so I changed it accordingly and tested it.
Hopefully this will provide a performance gain, though I did not do any
performance tests and therefore can't tell if that's really the case.

I had interpreted Sergei's response to my BUG_ON() statement for it to
be unnecessary, which may be wrong. I don't know if it's possible to
connect ATAPI devices to the on board CompactFlash slot, maybe you can
give me some hints on this. OTOH I see that the sector size is hard
coded in the original driver to 0x200, so I guess either ATAPI is not
possible or at least not with the original driver, right?

Greetings, Phil

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

[Sergei Shtylyov]

Hello.

Phil Sutter wrote:

>>> * rename the offset definition to avoid abiguity with the standard ATA
>>>  IO address
>>> * read and write four bytes at once
>>> * use writesl() and readsl() which implicitly iterate over the data
>>> * fix the signature of rb532_pata_data_xfer() to match the function
>>>  pointer definition and return the number of bytes consumed
>>>
>>> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
>>> Acked-by: Sergei Shtyltov <sshtylyov@ru.mvista.com>
>>> ---
>>> drivers/ata/pata_rb532_cf.c |   21 ++++++++++-----------
>>> 1 files changed, 10 insertions(+), 11 deletions(-)
>>>       
>> None of this description says _why_ you wish to do this.  Also, maybe I 
>> missed the response, what about ATAPI?
>>     
>
> Well, Sergei Shtylyov pointed out that the original driver does transfer
> data at 4-byte blocks, so I changed it accordingly and tested it.
> Hopefully this will provide a performance gain, though I did not do any
> performance tests and therefore can't tell if that's really the case.
>   

   Might've been worth to put this into separate patch, apart from the 
fix for the function prototype.

> I had interpreted Sergei's response to my BUG_ON() statement for it to
> be unnecessary, which may be wrong.

   You misintepreted my response. :-)

> I don't know if it's possible to
> connect ATAPI devices to the on board CompactFlash slot, maybe you can
>   

   I don't think so.

> give me some hints on this. OTOH I see that the sector size is hard
> coded in the original driver to 0x200, so I guess either ATAPI is not
> possible or at least not with the original driver, right?
>   

   If ATAPI was indeed possible, BUG() wouldn't have been an option.
   I've just remembered about the READ/WRITE LONG commands which might 
cause byte count to not be divisible by 4 (although by default ECC is 
4-byte entity and CF 1.4 says that this is the ony option).

MBR, Sergei

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

[Phil Sutter]

Hi,

On Wed, Nov 05, 2008 at 02:54:44PM +0300, Sergei Shtylyov wrote:
> Phil Sutter wrote:
> 
> >Well, Sergei Shtylyov pointed out that the original driver does transfer
> >data at 4-byte blocks, so I changed it accordingly and tested it.
> >Hopefully this will provide a performance gain, though I did not do any
> >performance tests and therefore can't tell if that's really the case.
> >  
> 
>   Might've been worth to put this into separate patch, apart from the 
> fix for the function prototype.

Ok, I will do that.

> >I had interpreted Sergei's response to my BUG_ON() statement for it to
> >be unnecessary, which may be wrong.
> 
>   You misintepreted my response. :-)

Oh well, if only communication was as easy as programming C. :)

> >I don't know if it's possible to
> >connect ATAPI devices to the on board CompactFlash slot, maybe you can
> 
>   I don't think so.

What about those devices for PDAs like GPS mice or UMTS modems? The
platform for dangerous half knowledge talks about "CF I/O". Is this
something completely different?

> >give me some hints on this. OTOH I see that the sector size is hard
> >coded in the original driver to 0x200, so I guess either ATAPI is not
> >possible or at least not with the original driver, right?
> 
>   If ATAPI was indeed possible, BUG() wouldn't have been an option.
>   I've just remembered about the READ/WRITE LONG commands which might 
> cause byte count to not be divisible by 4 (although by default ECC is 
> 4-byte entity and CF 1.4 says that this is the ony option).

Sure, I added it because it's an assumption made on the algorithm to
function correctly and I wasn't sure if that's the case.

So what's the final solution to this? Transfer byte-wise if buflen is
not a multiple of four (I doubt this would provide full ATAPI
compatibility, though I have no idea) or maybe transfer (buflen % 4)
bytes less and return something different than buflen?

I sadly don't have any hardware to test with other than a CF-disk, which
of course is an ATA device.

Thanks, Phil

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

[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.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
Acked-by: Sergei Shtyltov <sshtylyov@ru.mvista.com>
---
 drivers/ata/pata_rb532_cf.c |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index 7b11f40..b919012 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;
@@ -74,19 +75,16 @@ 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,
-				unsigned int buflen, int write_data)
+static void 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) {
-		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);
 }
@@ -156,7 +154,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

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

[Phil Sutter]

Per definition, this function should return the number of bytes
consumed. Also take care of the unlikely case when buflen is not a
multiple of four; while transferring, the division cuts the remaining
bytes, so alter the return value accordingly.

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

diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
index b919012..95a0d66 100644
--- a/drivers/ata/pata_rb532_cf.c
+++ b/drivers/ata/pata_rb532_cf.c
@@ -75,7 +75,7 @@ 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,
+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;
@@ -87,6 +87,7 @@ static void rb532_pata_data_xfer(struct ata_device *adev,
 		readsl(ioaddr, buf, buflen / sizeof(u32));
 
 	rb532_pata_finish_io(adev->link->ap);
+	return buflen - (buflen % sizeof(u32));
 }
 
 static void rb532_pata_freeze(struct ata_port *ap)
-- 
1.5.6.4

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

[Florian Fainelli]

Hi Phil,

(CC'in Jeff as he is the libata maintainer).

Le Wednesday 12 November 2008 01:14:09 Phil Sutter, vous avez écrit :
> Per definition, this function should return the number of bytes
> consumed. Also take care of the unlikely case when buflen is not a
> multiple of four; while transferring, the division cuts the remaining
> bytes, so alter the return value accordingly.
>
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
Acked-by: Florian Fainelli <florian@openwrt.org>

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

[Sergei Shtylyov]

Hello.

Phil Sutter wrote:

> Per definition, this function should return the number of bytes
> consumed. Also take care of the unlikely case when buflen is not a
> multiple of four; while transferring, the division cuts the remaining
> bytes, so alter the return value accordingly.
>
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
>   

Acked-by: Sergei Shtyltov <sshtylyov@ru.mvista.com>

   Except it should've been the first patch in the series.

> diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
> index b919012..95a0d66 100644
> --- a/drivers/ata/pata_rb532_cf.c
> +++ b/drivers/ata/pata_rb532_cf.c
> @@ -75,7 +75,7 @@ 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,
> +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;
> @@ -87,6 +87,7 @@ static void rb532_pata_data_xfer(struct ata_device *adev,
>  		readsl(ioaddr, buf, buflen / sizeof(u32));
>  
>  	rb532_pata_finish_io(adev->link->ap);
> +	return buflen - (buflen % sizeof(u32));
>   

   Unneeded parens -- multiplication/division/modulo has natural 
priority over addition/subtraction.

WBR, Sergei

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

[Florian Fainelli]

Hi Phil,

(CC'in Jeff as he is the libata maintainer).

Le Wednesday 12 November 2008 01:13:41 Phil Sutter, vous avez écrit :
> * 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.
>
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
> Acked-by: Sergei Shtyltov <sshtylyov@ru.mvista.com>

Acked-by: Florian Fainelli <florian@openwrt.org>

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

[Sergei Shtylyov]

Hello.

Phil Sutter wrote:

> * 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.
>
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
> Acked-by: Sergei Shtyltov <sshtylyov@ru.mvista.com>
>   

   The rb532_pata_data_xfer() prototype fix should've been preceding 
this one, as it was dealing with a real issue, and this one only 
enhances the driver performance.

> diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
> index 7b11f40..b919012 100644
> --- a/drivers/ata/pata_rb532_cf.c
> +++ b/drivers/ata/pata_rb532_cf.c
>   
[...]
> @@ -74,19 +75,16 @@ 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,
> -				unsigned int buflen, int write_data)
> +static void rb532_pata_data_xfer(struct ata_device *adev,
> +		unsigned char *buf, unsigned int buflen, int write_data)
>   

   Please avoid doing the "collateral damage" to the code like this. The 
formatting was pretty good before.

WBR, Sergei

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

[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: 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: 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