| Message ID | 20081011180014.GA4908@debian (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Sun, 2008-10-12 at 02:00 +0800, Wang Jian wrote: > 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. > > Signed-off-by: Wang Jian <lark@linux.net.cn> > --- 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.
Hello, Wang Jian wrote: > +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); 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. > @@ -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; 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.
Tejun Heo wrote: > Hello, > > Wang Jian wrote: >> +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); > > 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. >> @@ -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; > > It's strange to define ATA_DATA_WIDTH_* constants in ata.h and only > use it in ata_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 :) > > 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
Wang Jian wrote: > Tejun Heo wrote: >> Hello, >> >> Wang Jian wrote: >>> +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); >> >> 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. Yeap, ata_dev_configure() exactly is the place you're looking for. It's reponsible for reprogramming the device after it has been reset. >>> + 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. > > 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 :) :-) >> 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.
On Mon, 13 Oct 2008 16:17:02 +0900 Tejun Heo <htejun@gmail.com> wrote: > Hello, > > Wang Jian wrote: > > +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.
diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c index 8f65ad6..d2276ad 100644 --- a/drivers/ata/pata_platform.c +++ b/drivers/ata/pata_platform.c @@ -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) diff --git a/include/linux/ata.h b/include/linux/ata.h index be00973..4ce26df 100644 --- a/include/linux/ata.h +++ b/include/linux/ata.h @@ -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 */ diff --git a/include/linux/ata_platform.h b/include/linux/ata_platform.h index 9a26c83..434fe49 100644 --- a/include/linux/ata_platform.h +++ b/include/linux/ata_platform.h @@ -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, diff --git a/include/linux/libata.h b/include/linux/libata.h index 947cf84..156b7d3 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -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 */
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. Signed-off-by: Wang Jian <lark@linux.net.cn> --- 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(-)