pata_platform: add 8 bit data io support
RFC PATCH 1/1 pata_platform: add 8 bit data io support
To avoid adding another rare used ata_port member, new bit is added to ata_port->flags.
Originally, I hacked pata_platform to make it 8bit only to support 8bit data wired CF card. This patch is more generic.
With this patch, __pata_platform_probe() interface is changed, and pata_of_platform is broken, so a small patch is needed.
--- drivers/ata/pata_platform.c | 63 ++++++++++++++++++++++++++++++++++++++++-- include/linux/ata.h | 8 +++++ include/linux/ata_platform.h | 4 ++ include/linux/libata.h | 1 + 4 files changed, 73 insertions(+), 3 deletions(-)
drivers/ata/pata_platform.c +60 -3
@@ -50,9 +50,62 @@ static struct scsi_host_template pata_platform_sht = { ATA_PIO_SHT(DRV_NAME), }; +static void pata_platform_postreset (struct ata_link *link,unsigned int *classes) +{ +struct ata_port *ap = link->ap; +struct ata_device *dev; +u8 select =ATA_DEVICE_OBS ; + +/* Call default callback first */ +ata_sff_postreset (link, classes); + +if (!(ap->flags &ATA_FLAG_8BIT_DATA )) +return ; + +/* Set 8-bit mode. We know we can do that */ +ata_link_for_each_dev (dev, link) { +if (dev->devno) + select |=ATA_DEV1 ; + +iowrite8 (SETFEATURES_8BIT_ON , ap->ioaddr.feature_addr); +iowrite8 (select, ap->ioaddr.device_addr); +iowrite8 (ATA_CMD_SET_FEATURES , ap->ioaddr.command_addr); + } +} + +static unsigned int pata_platform_data_xfer (struct ata_device *dev, +unsigned char *buf,unsigned int buflen,int rw) +{ +struct ata_port *ap = dev->link->ap; + +if (!(ap->flags &ATA_FLAG_8BIT_DATA )) +return ata_sff_data_xfer (dev, buf, buflen, rw); + +if (rw ==READ ) +ioread8_rep (ap->ioaddr.data_addr, buf, buflen); +else +iowrite8_rep (ap->ioaddr.data_addr, buf, buflen); + +return buflen; +} + +static unsigned int pata_platform_data_xfer_noirq (struct ata_device *dev, +unsigned char *buf,unsigned int buflen,int rw) +{ +unsigned long flags; +unsigned int consumed; + +local_irq_save (flags); + consumed =pata_platform_data_xfer (dev, buf, buflen, rw); +local_irq_restore (flags); + +return consumed; +} +static struct ata_port_operations pata_platform_port_ops = { .inherits = &ata_sff_port_ops, - .sff_data_xfer = ata_sff_data_xfer_noirq, + .postreset = pata_platform_postreset, + .sff_data_xfer = pata_platform_data_xfer_noirq, .cable_detect = ata_cable_unknown, .set_mode = pata_platform_set_mode, .port_start =ATA_OP_NULL ,
@@ -106,7 +159,8 @@ int __devinit __pata_platform_probe(struct device *dev,struct resource *ctl_res, struct resource *irq_res, unsigned int ioport_shift, - int __pio_mask) + int __pio_mask, + unsigned int data_width) { struct ata_host *host;struct ata_port *ap;
@@ -140,6 +194,9 @@ int __devinit __pata_platform_probe(struct device *dev, ap->pio_mask = __pio_mask; ap->flags |=ATA_FLAG_SLAVE_POSS ; +if (data_width ==ATA_DATA_WIDTH_8BIT ) + ap->flags |=ATA_FLAG_8BIT_DATA ; +/* * Use polling mode if there's no IRQ */
@@ -242,7 +299,7 @@ static int __devinit pata_platform_probe(struct platform_device *pdev)return __pata_platform_probe (&pdev->dev, io_res, ctl_res, irq_res, pp_info ? pp_info->ioport_shift :0 , - pio_mask); + pio_mask, pp_info->data_width); } static int __devexit pata_platform_remove(struct platform_device *pdev)
include/linux/ata.h +8 -0
@@ -45,6 +45,11 @@ enum {ATA_MAX_SECTORS_LBA48 =65535 ,/* TODO: 65536? */ ATA_MAX_SECTORS_TAPE =65535 , +ATA_DATA_WIDTH_8BIT =1 , +ATA_DATA_WIDTH_16BIT =2 , +ATA_DATA_WIDTH_DEFAULT =2 , +ATA_DATA_WIDTH_32BIT =4 , +ATA_ID_WORDS =256 ,ATA_ID_CONFIG =0 , ATA_ID_CYLS = 1,
@@ -280,6 +285,9 @@ enum {XFER_PIO_0 =0x08 ,XFER_PIO_SLOW =0x00 , +SETFEATURES_8BIT_ON =0x01 ,/* Enable 8 bit data transfers */ +SETFEATURES_8BIT_OFF =0x81 ,/* Disable 8 bit data transfers */ +SETFEATURES_WC_ON =0x02 ,/* Enable write cache */ SETFEATURES_WC_OFF = 0x82, /* Disable write cache */
include/linux/ata_platform.h +4 -0
@@ -13,6 +13,10 @@ struct pata_platform_info {* IRQ flags when call request_irq() */ unsigned int irq_flags; +/* +* Data I/O width (in byte) +*/ +unsigned int data_width; }; extern int __devinit __pata_platform_probe(struct device *dev,
include/linux/libata.h +1 -0
@@ -187,6 +187,7 @@ enum {ATA_FLAG_PIO_POLLING = (1 <<9 ),/* use polling PIO if LLD * doesn't handle PIO interrupts */ ATA_FLAG_NCQ = (1 <<10 ),/* host supports NCQ */ +ATA_FLAG_8BIT_DATA = (1 <<11 ),/* Host using 8 bit data */ ATA_FLAG_DEBUGMSG = (1 <<13 ),ATA_FLAG_IGN_SIMPLEX = (1 <<15 ),/* ignore SIMPLEX */ ATA_FLAG_NO_IORDY = (1 << 16), /* controller lacks iordy */
Signature · 1 line
1.5.6.5
Quoted 11 lines · Wang Jian
To avoid adding another rare used
ata_portmember, new bit is added toata_port->flags.Originally, I hacked
pata_platformto make it 8bit only to support 8bit data wired CF card. This patch is more generic.With this patch,
__pata_platform_probe()interface is changed, andpata_of_platformis broken, so a small patch is needed.Signed-off-by Wang Jian <[email protected]>---
A couple of things. First I would personally prefer (but I'm not the libata maintainer so it's up to Jeff ...) if you had a separate patch that adds the 8-bit support to libata core first, and then a patch that modifies pata_platform.
Then, in order to avoid breaking bisection, I would like you to fixup pata_of_platform in the same patch that modifies __pata_platform_probe so there is no breakage in between patches.
Now, regarding the patch itself, if the core grows a 8-bit flag, then I strongly suspect the core should also grow the 8-bit xfer function rather than having it hidden in pata_platform.
Cheers,
Ben.
Quoted 10 lines · Benjamin Herrenschmidt
Quoted 9 lines · Wang Jian
To avoid adding another rare used
ata_portmember, new bit is added toata_port->flags.Originally, I hacked
pata_platformto make it 8bit only to support 8bit data wired CF card. This patch is more generic.With this patch,
__pata_platform_probe()interface is changed, andpata_of_platformis broken, so a small patch is needed.
Quoted 7 lines
Quoted 2 lines
Signed-off-by Wang Jian <[email protected]>---
A couple of things. First I would personally prefer (but I'm not the libata maintainer so it's up to Jeff ...) if you had a separate patch that adds the 8-bit support to libata core first, and then a patch that modifies
pata_platform.
I will do that if my 8-bit mode patch is done right technically.
Quoted 4 lines
Then, in order to avoid breaking bisection, I would like you to fixup
pata_of_platformin the same patch that modifies__pata_platform_probeso there is no breakage in between patches.
Yes.
Quoted 4 lines
Now, regarding the patch itself, if the core grows a 8-bit flag, then I strongly suspect the core should also grow the 8-bit xfer function rather than having it hidden in
pata_platform.
This is the main reason I send a single RFC patch. Where to add 8-bit mode should be decided first.
Because 8-bit mode is mostly used for embedded devices, my opinion is 8-bit mode in pata_platform is enough.
However, look at pata_platform_data_xfer() I added, the code can be merged into ata_sff_data_xfer() of libata-sff.c easily. Moving the code there is trivial if necessary.
Another problem should be addressed: using flags v.s. using data_width member. I add a bit to indicate 8 bit mode, but this seems to be a problem for future 32 bit I/O support in libata.
Hello,
Quoted 10 lines · Wang Jian
drivers/ata/pata_platform.c +10
-0+static void pata_platform_postreset (struct ata_link *link,unsigned int *classes) +{ +struct ata_port *ap = link->ap; +struct ata_device *dev; +u8 select =ATA_DEVICE_OBS ; + +/* Call default callback first */ +ata_sff_postreset (link, classes); + +if (!(ap->flags &ATA_FLAG_8BIT_DATA ))
drivers/ata/pata_platform.c static struct scsi_host_template pata_platform_sht = { · line 72
Quoted 10 lines
drivers/ata/pata_platform.c +10
-0+return ; + +/* Set 8-bit mode. We know we can do that */ +ata_link_for_each_dev (dev, link) { +if (dev->devno) + select |=ATA_DEV1 ; + +iowrite8 (SETFEATURES_8BIT_ON , ap->ioaddr.feature_addr); +iowrite8 (select, ap->ioaddr.device_addr); +iowrite8 (ATA_CMD_SET_FEATURES , ap->ioaddr.command_addr);
Aieee... Please don't do this. I think it best belongs to ata_dev_configure() or ->dev_config() if you wanna put it in low level driver.
Quoted 11 lines
drivers/ata/pata_platform.c +2
-1@@ -106,7 +159,8 @@ int __devinit __pata_platform_probe(struct device *dev,struct resource *ctl_res, struct resource *irq_res, unsigned int ioport_shift, - int __pio_mask) + int __pio_mask, + unsigned int data_width) { struct ata_host *host;struct ata_port *ap;
drivers/ata/pata_platform.c int __devinit __pata_platform_probe(struct device *dev, · line 198
Quoted 5 lines
drivers/ata/pata_platform.c +2
-0@@ -140,6 +194,9 @@ int __devinit __pata_platform_probe(struct device *dev, ap->pio_mask = __pio_mask; ap->flags |=ATA_FLAG_SLAVE_POSS ; +if (data_width ==ATA_DATA_WIDTH_8BIT ) + ap->flags |=ATA_FLAG_8BIT_DATA ;
It's strange to define ATA_DATA_WIDTH_* constants in ata.h and only use it in ata_platform.
Overall, I think the bulk of the 8bit PIO implementation should go into the libata core layer and transfer width should be property of struct ata_device - probably right above or below pio/dma_mode and xfer_mode/shift fields.
Thanks.
Signature · 1 line
tejun
Quoted 19 lines · Tejun Heo
Hello,
Quoted 16 lines · Wang Jian
drivers/ata/pata_platform.c +16
-0+static void pata_platform_postreset (struct ata_link *link,unsigned int *classes) +{ +struct ata_port *ap = link->ap; +struct ata_device *dev; +u8 select =ATA_DEVICE_OBS ; + +/* Call default callback first */ +ata_sff_postreset (link, classes); + +if (!(ap->flags &ATA_FLAG_8BIT_DATA )) +return ; + +/* Set 8-bit mode. We know we can do that */ +ata_link_for_each_dev (dev, link) { +if (dev->devno) + select |=ATA_DEV1 ;
Quoted 9 lines
drivers/ata/pata_platform.c static struct scsi_host_template pata_platform_sht = { · line 72
Quoted 4 lines
drivers/ata/pata_platform.c +4
-0+ +iowrite8 (SETFEATURES_8BIT_ON , ap->ioaddr.feature_addr); +iowrite8 (select, ap->ioaddr.device_addr); +iowrite8 (ATA_CMD_SET_FEATURES , ap->ioaddr.command_addr);Aieee... Please don't do this. I think it best belongs to
ata_dev_configure()or ->dev_config() if you wanna put it in low level driver.
Good.
I remember the spec states that this setfeature command should be issued every time reset is issued. This is just a quick and safe hack.
I will look into libata deeper and figure out how to do it better per your suggestion.
Quoted 14 lines
Quoted 14 lines
drivers/ata/pata_platform.c +2
-1@@ -106,7 +159,8 @@ int __devinit __pata_platform_probe(struct device *dev,struct resource *ctl_res, struct resource *irq_res, unsigned int ioport_shift, - int __pio_mask) + int __pio_mask, + unsigned int data_width) { struct ata_host *host;struct ata_port *ap;@@ -140,6 +194,9 @@ int __devinit __pata_platform_probe(struct device *dev, ap->pio_mask = __pio_mask; ap->flags |=ATA_FLAG_SLAVE_POSS ;
Quoted 5 lines
drivers/ata/pata_platform.c int __devinit __pata_platform_probe(struct device *dev, · line 198
Quoted 2 lines
drivers/ata/pata_platform.c +2
-0+if (data_width ==ATA_DATA_WIDTH_8BIT ) + ap->flags |=ATA_FLAG_8BIT_DATA ;It's strange to define ATA_DATA_WIDTH_* constants in
ata.hand only use it inata_platform.
I have expressed in another reply that the best place the code belongs to should be decided first. The usage of flags looks ugly too :)
Quoted 6 lines
Overall, I think the bulk of the 8bit PIO implementation should go into the libata core layer and transfer width should be property of struct
ata_device- probably right above or below pio/dma_mode and xfer_mode/shift fields.
Yes, I agree it'd better go into libata core layer. But for transfer width, I think it is not belongs to ata_device. It's about how ata controller wired for data line. (In my case, it is how CF card wired). Am I wrong?
Best regards
Quoted 31 lines · Wang Jian
Quoted 20 lines · Tejun Heo
Hello,
Quoted 17 lines · Wang Jian
+static void
pata_platform_postreset(struct ata_link *link, unsigned int *classes)drivers/ata/pata_platform.c +15
-0+{ +struct ata_port *ap = link->ap; +struct ata_device *dev; +u8 select =ATA_DEVICE_OBS ; + +/* Call default callback first */ +ata_sff_postreset (link, classes); + +if (!(ap->flags &ATA_FLAG_8BIT_DATA )) +return ; + +/* Set 8-bit mode. We know we can do that */ +ata_link_for_each_dev (dev, link) { +if (dev->devno) + select |=ATA_DEV1 ;Quoted 9 lines
drivers/ata/pata_platform.c static struct scsi_host_template pata_platform_sht = { · line 72
Quoted 4 lines
drivers/ata/pata_platform.c +4
-0+ +iowrite8 (SETFEATURES_8BIT_ON , ap->ioaddr.feature_addr); +iowrite8 (select, ap->ioaddr.device_addr); +iowrite8 (ATA_CMD_SET_FEATURES , ap->ioaddr.command_addr);Aieee... Please don't do this. I think it best belongs to
ata_dev_configure()or ->dev_config() if you wanna put it in low level driver.
Quoted 7 lines
Good.
I remember the spec states that this setfeature command should be issued every time reset is issued. This is just a quick and safe hack.
I will look into libata deeper and figure out how to do it better per your suggestion.
Yeap, ata_dev_configure() exactly is the place you're looking for. It's reponsible for reprogramming the device after it has been reset.
Quoted 8 lines
Quoted 5 lines
drivers/ata/pata_platform.c int __devinit __pata_platform_probe(struct device *dev, · line 198
Quoted 2 lines
drivers/ata/pata_platform.c +2
-0+if (data_width ==ATA_DATA_WIDTH_8BIT ) + ap->flags |=ATA_FLAG_8BIT_DATA ;It's strange to define ATA_DATA_WIDTH_* constants in
ata.hand only use it inata_platform.I have expressed in another reply that the best place the code belongs to should be decided first. The usage of flags looks ugly too :)
:-)
Quoted 10 lines
Quoted 5 lines
Overall, I think the bulk of the 8bit PIO implementation should go into the libata core layer and transfer width should be property of struct
ata_device- probably right above or below pio/dma_mode and xfer_mode/shift fields.Yes, I agree it'd better go into libata core layer. But for transfer width, I think it is not belongs to
ata_device. It's about how ata controller wired for data line. (In my case, it is how CF card wired). Am I wrong?
Well, yes, it primarily depends on the controller but ISTR cases where PIO issues resolved by turning off 32bit PIO (dunno why that is, some timing issue?) and for things like that to work, the setting should be per-device. I'm not too sure about this. Cc'ing Alan. He should know much better than I do.
Alan, where does 8/16/32-bit PIO transfer belong? Host, port or device?
Thanks.
Signature · 1 line
tejun
Quoted 16 lines · On Mon, 13 Oct 2008 16:17:02 +0900 Tejun Heo <[email protected]> wrote:
Hello,
drivers/ata/pata_platform.c static struct scsi_host_template pata_platform_sht = { · line 65
Quoted 13 lines · Wang Jian
drivers/ata/pata_platform.c +13
-0+static void pata_platform_postreset (struct ata_link *link,unsigned int *classes) +{ +struct ata_port *ap = link->ap; +struct ata_device *dev; +u8 select =ATA_DEVICE_OBS ; + +/* Call default callback first */ +ata_sff_postreset (link, classes); + +if (!(ap->flags &ATA_FLAG_8BIT_DATA )) +return ; + +/* Set 8-bit mode. We know we can do that */
Not really - the 8bit transfer command is CFA specific not ATA or ATAPI, in addition it is not needed with most hardware that is running in 8bit modes.