[PATCH RFC] pata_platform: add 8 bit data io support

[PATCH RFC] pata_platform: add 8 bit data io support

[Wang Jian]

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(-)

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 */
-- 
1.5.6.5

Re: [PATCH RFC] pata_platform: add 8 bit data io support

[Benjamin Herrenschmidt]

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.

Re: [PATCH RFC] pata_platform: add 8 bit data io support

[Wang Jian]

Benjamin Herrenschmidt 写道:
> 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.

I will do that if my 8-bit mode patch is done right technically.

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

Yes.

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

Re: [PATCH RFC] pata_platform: add 8 bit data io support

[Tejun Heo]

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

Re: [PATCH RFC] pata_platform: add 8 bit data io support

[Wang Jian]

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

Re: [PATCH RFC] pata_platform: add 8 bit data io support

[Tejun Heo]

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.

-- 
tejun

Re: [PATCH RFC] pata_platform: add 8 bit data io support

[Alan Cox]

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.