[PATCH] Add hook for custom xfer function in PATA Platform driver

[PATCH] Add hook for custom xfer function in PATA Platform driver

[Graeme Russ]

Allows the existing 16-bit data transfer function used by the PATA Platform
driver to be overriden by a vendor specific fuction. This is useful if,
for example, the PATA device is only capable of 8-bit data transfers

Signed-off-by: Graeme Russ <graeme.russ@gmail.com>
---
 drivers/ata/pata_platform.c  |    7 +++++++
 include/linux/ata_platform.h |    5 +++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index 3f6ebc6..dc516b4 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -238,6 +238,13 @@ static int __devinit pata_platform_probe(struct platform_device *pdev)
 	if (irq_res)
 		irq_res->flags = pp_info ? pp_info->irq_flags : 0;
 
+	/*
+	 * Assign custom data transfer function (if defined)
+	 */
+	if (pp_info)
+		if (unlikely(pp_info->data_xfer))
+			pata_platform_port_ops.sff_data_xfer = pp_info->data_xfer;
+
 	return __pata_platform_probe(&pdev->dev, io_res, ctl_res, irq_res,
 				     pp_info ? pp_info->ioport_shift : 0,
 				     pio_mask);
diff --git a/include/linux/ata_platform.h b/include/linux/ata_platform.h
index 9a26c83..0913633 100644
--- a/include/linux/ata_platform.h
+++ b/include/linux/ata_platform.h
@@ -13,6 +13,11 @@ struct pata_platform_info {
 	 * IRQ flags when call request_irq()
 	 */
 	unsigned int irq_flags;
+	/*
+	 * Custom data transfer function
+	 */
+	unsigned int (*data_xfer)(struct ata_device *dev,
+			unsigned char *buf, unsigned int buflen, int rw);
 };
 
 extern int __devinit __pata_platform_probe(struct device *dev,
-- 
1.7.1.rc1.12.ga601

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Graeme Russ]

[Added linux-ide back onto distribution list]

Sergei Shtylyov wrote:
> Hello.
> 
> Graeme Russ wrote:
> 
>> +    /*
>> +     * Assign custom data transfer function (if defined)
>> +     */
>> +    if (pp_info)
>> +        if (unlikely(pp_info->data_xfer))
>>   
> 
>   Could be folded into a signle *if* statement.

I thought so - I didn't exactly know what the linux coding standards are in
regards to this shortcut though.

> 
>   You should have also taught the symmetric ide-platfrom driver the same
> trick. However, IDE core's data transfer methods has a different prototype.

I did think about the other drivers (OF Platform etc) but I have no way of
testing them.

> I suggest to rather add a new flag, indicating 8-bit data I/O, and have
> the alternate sff_data_xfer() method defined inside the driver.
> 

Well actually it goes beyond simply implementing 8-bit data transfers - On
my particular board, the PATA devices (2x CF slots) are on a General
Purpose Bus with a few other peripherals. The timing requirements for the
bus when accessing the CF cards are very strict (and very slow compared to
other devices on the bus). By overriding the data transfer function I can
arbitrate access to the bus and switch the bus timings based on the
peripheral being accessed. This cannot be done be a generic data transfer
function.

Regards,

Graeme

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Sergei Shtylyov]

Hello.

Graeme Russ wrote:

> [Added linux-ide back onto distribution list]
>   

   Right, I didn't intend to exclude it -- probably didn't press all the 
keys at once for the reply-to-all function.

[...]
>
>>   You should have also taught the symmetric ide-platfrom driver the same
>> trick. However, IDE core's data transfer methods has a different prototype.
>>     
>
> I did think about the other drivers (OF Platform etc) but I have no way of
> testing them.
>   

   pata_of_platform is not easily extensible in this way. It gets all 
the information about the device from the device tree -- and you can't 
encode all your complications there, unless you invent a new OF device.

>> I suggest to rather add a new flag, indicating 8-bit data I/O, and have
>> the alternate sff_data_xfer() method defined inside the driver.
>>
>>     
>
> Well actually it goes beyond simply implementing 8-bit data transfers - On
> my particular board, the PATA devices (2x CF slots) are on a General
> Purpose Bus with a few other peripherals. The timing requirements for the
> bus when accessing the CF cards are very strict (and very slow compared to
> other devices on the bus). By overriding the data transfer function I can
> arbitrate access to the bus and switch the bus timings based on the
> peripheral being accessed. This cannot be done be a generic data transfer
> function.
>   

   I disagree with your approach of overriding the libata methods at the 
board level, so I suggest to write a new driver. This is too complicated 
stuff for poor old pata_platform. :-)

> Regards,
>
> Graeme

MBR, Sergei

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Graeme Russ]

Sergei Shtylyov wrote:
> Hello.
> 
> Graeme Russ wrote:
> 
>> [Added linux-ide back onto distribution list]
>>   
> 
>   Right, I didn't intend to exclude it -- probably didn't press all the
> keys at once for the reply-to-all function.
> 
> [...]
>>
>>>   You should have also taught the symmetric ide-platfrom driver the same
>>> trick. However, IDE core's data transfer methods has a different
>>> prototype.
>>>     
>>
>> I did think about the other drivers (OF Platform etc) but I have no
>> way of
>> testing them.
>>   
> 
>   pata_of_platform is not easily extensible in this way. It gets all the
> information about the device from the device tree -- and you can't
> encode all your complications there, unless you invent a new OF device.
> 
>>> I suggest to rather add a new flag, indicating 8-bit data I/O, and have
>>> the alternate sff_data_xfer() method defined inside the driver.

The vast majority of implementations are 16-bit (no one has complained
about the lack of 8-bit support to date). I don't think the majority of
users should be carrying around the extra code for a tiny minority. Yes, it
could be wrapped around an #ifdef but then things start to get ugly (why
not just ditch the flag and #ifdef the 8-bit transfers entirely, hack
Kconfig etc) eeewwwww....

>> other devices on the bus). By overriding the data transfer function I can
>> arbitrate access to the bus and switch the bus timings based on the
>> peripheral being accessed. This cannot be done be a generic data transfer
>> function.
>>   
> 
>   I disagree with your approach of overriding the libata methods at the
> board level, so I suggest to write a new driver. This is too complicated
> stuff for poor old pata_platform. :-)

My custom function to date looks like:

unsigned int ata_enet_data_xfer(struct ata_device *dev, unsigned char *buf,
			       unsigned int buflen, int rw)
{
	struct ata_port *ap = dev->link->ap;
	void __iomem *data_addr = ap->ioaddr.data_addr;

	set_gp_bus_slow();
	/* Transfer bytes */
	if (rw == READ)
		ioread8_rep(data_addr, buf, buflen);
	else
		iowrite8_rep(data_addr, buf, buflen);

	set_gp_bus_fast();
	return buflen;
}

set_gp_bus_slow() and set_gp_bus_fast() (at the moment) simply set a few
config registers to set the GP bus timing (no arbitration yet, but these
functions will also handle that using a mutex). I don't see the point in
re-writing the entire PATA Platform driver when the existing driver appears
to be perfectly capable with a very minor extension hook.

Regards,

Graeme

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Sergei Shtylyov]

Hello.

Graeme Russ wrote:

> Sergei Shtylyov wrote:
>   
>> Hello.
>>
>> Graeme Russ wrote:
>>
>>     
>>> [Added linux-ide back onto distribution list]
>>>   
>>>       
>>   Right, I didn't intend to exclude it -- probably didn't press all the
>> keys at once for the reply-to-all function.
>>
>> [...]
>>     
>>>>   You should have also taught the symmetric ide-platfrom driver the same
>>>> trick. However, IDE core's data transfer methods has a different
>>>> prototype.
>>>>     
>>>>         
>>> I did think about the other drivers (OF Platform etc) but I have no
>>> way of
>>> testing them.
>>>   
>>>       
>>   pata_of_platform is not easily extensible in this way. It gets all the
>> information about the device from the device tree -- and you can't
>> encode all your complications there, unless you invent a new OF device.
>>
>>     
>>>> I suggest to rather add a new flag, indicating 8-bit data I/O, and have
>>>> the alternate sff_data_xfer() method defined inside the driver.
>>>>         
>
> The vast majority of implementations are 16-bit (no one has complained
> about the lack of 8-bit support to date). I don't think the majority of
> users should be carrying around the extra code for a tiny minority. Yes, it
> could be wrapped around an #ifdef but then things start to get ugly (why
> not just ditch the flag and #ifdef the 8-bit transfers entirely, hack
> Kconfig etc) eeewwwww....
>   

   I didn't propose any of this. Anyway, this is not an option anymore 
now that we know enough about your hardware.

>   
>>> other devices on the bus). By overriding the data transfer function I can
>>> arbitrate access to the bus and switch the bus timings based on the
>>> peripheral being accessed. This cannot be done be a generic data transfer
>>> function.
>>>   
>>>       
>>   I disagree with your approach of overriding the libata methods at the
>> board level, so I suggest to write a new driver. This is too complicated
>> stuff for poor old pata_platform. :-)
>>     
>
> My custom function to date looks like:
>
> unsigned int ata_enet_data_xfer(struct ata_device *dev, unsigned char *buf,
> 			       unsigned int buflen, int rw)
> {
> 	struct ata_port *ap = dev->link->ap;
> 	void __iomem *data_addr = ap->ioaddr.data_addr;
>
> 	set_gp_bus_slow();
> 	/* Transfer bytes */
> 	if (rw == READ)
> 		ioread8_rep(data_addr, buf, buflen);
> 	else
> 		iowrite8_rep(data_addr, buf, buflen);
>
> 	set_gp_bus_fast();
> 	return buflen;
> }
>
> set_gp_bus_slow() and set_gp_bus_fast() (at the moment) simply set a few
> config registers to set the GP bus timing (no arbitration yet, but these
> functions will also handle that using a mutex). I don't see the point in
> re-writing the entire PATA Platform driver when the existing driver appears
> to be perfectly capable with a very minor extension hook.
>   

   As I said, we *can't* implement the driver methods at the board 
level. Especially if they involve messing with timings -- that's the 
point where the ATA driver stops being generic, like pata_platform, and 
there arises a need for the dedicated driver. Also, your patch would 
bring in disparity with the ide-platfrom driver (which should be 
interchangeable with pata_platfrom). For me, the need of a separate 
driver is clear now, so I'll remain opposed to your patch. Of course, 
the maintainer (Jeff Garzik) will decide but if I could veto this patch 
I would.

> Regards,
>
> Graeme

WBR, Sergei

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Sergei Shtylyov]

Hello.

Sergei Shtylyov wrote:

>>>> other devices on the bus). By overriding the data transfer function 
>>>> I can
>>>> arbitrate access to the bus and switch the bus timings based on the
>>>> peripheral being accessed. This cannot be done be a generic data 
>>>> transfer
>>>> function.
>>>>         
>>>   I disagree with your approach of overriding the libata methods at the
>>> board level, so I suggest to write a new driver. This is too 
>>> complicated
>>> stuff for poor old pata_platform. :-)
>>>     
>>
>> My custom function to date looks like:
>>
>> unsigned int ata_enet_data_xfer(struct ata_device *dev, unsigned char 
>> *buf,
>>                    unsigned int buflen, int rw)
>> {
>>     struct ata_port *ap = dev->link->ap;
>>     void __iomem *data_addr = ap->ioaddr.data_addr;
>>
>>     set_gp_bus_slow();
>>     /* Transfer bytes */
>>     if (rw == READ)
>>         ioread8_rep(data_addr, buf, buflen);
>>     else
>>         iowrite8_rep(data_addr, buf, buflen);
>>
>>     set_gp_bus_fast();
>>     return buflen;
>> }
>>
>> set_gp_bus_slow() and set_gp_bus_fast() (at the moment) simply set a few
>> config registers to set the GP bus timing (no arbitration yet, but these

   BTW, do you know that there are different ATA PIO transfer modes with 
different timings?

>> functions will also handle that using a mutex). I don't see the point in
>> re-writing the entire PATA Platform driver when the existing driver 
>> appears
>> to be perfectly capable with a very minor extension hook.
>>   
>
>   As I said, we *can't* implement the driver methods at the board 
> level. Especially if they involve messing with timings -- that's the 
> point where the ATA driver stops being generic, like pata_platform, 
> and there arises a need for the dedicated driver. Also, your patch 
> would bring in disparity with the ide-platfrom driver (which should be 
> interchangeable with pata_platfrom). For me, the need of a separate 
> driver is clear now, so I'll remain opposed to your patch. Of course, 
> the maintainer (Jeff Garzik) will decide but if I could veto this 
> patch I would.

   At the very least, you should remove 'struct ata_device *' parameter 
from your hook, so that it could be called from the IDE driver also. 
Without this change, the patch will remain totally unacceptable.

>> Graeme
> Regards,

WBR, Sergei

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Robert Hancock]

On 05/10/2010 03:51 AM, Sergei Shtylyov wrote:
> Hello.
>
> Graeme Russ wrote:
>
>> Sergei Shtylyov wrote:
>>> Hello.
>>>
>>> Graeme Russ wrote:
>>>
>>>> [Added linux-ide back onto distribution list]
>>> Right, I didn't intend to exclude it -- probably didn't press all the
>>> keys at once for the reply-to-all function.
>>>
>>> [...]
>>>>> You should have also taught the symmetric ide-platfrom driver the same
>>>>> trick. However, IDE core's data transfer methods has a different
>>>>> prototype.

The IDE subsystem is deprecated and in maintenance mode, it shouldn't be 
growing support for new hardware, which I assume this is.

>>>> I did think about the other drivers (OF Platform etc) but I have no
>>>> way of
>>>> testing them.
>>> pata_of_platform is not easily extensible in this way. It gets all the
>>> information about the device from the device tree -- and you can't
>>> encode all your complications there, unless you invent a new OF device.
>>>
>>>>> I suggest to rather add a new flag, indicating 8-bit data I/O, and
>>>>> have
>>>>> the alternate sff_data_xfer() method defined inside the driver.
>>
>> The vast majority of implementations are 16-bit (no one has complained
>> about the lack of 8-bit support to date). I don't think the majority of
>> users should be carrying around the extra code for a tiny minority.
>> Yes, it
>> could be wrapped around an #ifdef but then things start to get ugly (why
>> not just ditch the flag and #ifdef the 8-bit transfers entirely, hack
>> Kconfig etc) eeewwwww....
>
> I didn't propose any of this. Anyway, this is not an option anymore now
> that we know enough about your hardware.
>
>>>> other devices on the bus). By overriding the data transfer function
>>>> I can
>>>> arbitrate access to the bus and switch the bus timings based on the
>>>> peripheral being accessed. This cannot be done be a generic data
>>>> transfer
>>>> function.
>>> I disagree with your approach of overriding the libata methods at the
>>> board level, so I suggest to write a new driver. This is too complicated
>>> stuff for poor old pata_platform. :-)
>>
>> My custom function to date looks like:
>>
>> unsigned int ata_enet_data_xfer(struct ata_device *dev, unsigned char
>> *buf,
>> unsigned int buflen, int rw)
>> {
>> struct ata_port *ap = dev->link->ap;
>> void __iomem *data_addr = ap->ioaddr.data_addr;
>>
>> set_gp_bus_slow();
>> /* Transfer bytes */
>> if (rw == READ)
>> ioread8_rep(data_addr, buf, buflen);
>> else
>> iowrite8_rep(data_addr, buf, buflen);
>>
>> set_gp_bus_fast();
>> return buflen;
>> }
>>
>> set_gp_bus_slow() and set_gp_bus_fast() (at the moment) simply set a few
>> config registers to set the GP bus timing (no arbitration yet, but these
>> functions will also handle that using a mutex). I don't see the point in
>> re-writing the entire PATA Platform driver when the existing driver
>> appears
>> to be perfectly capable with a very minor extension hook.
>
> As I said, we *can't* implement the driver methods at the board level.
> Especially if they involve messing with timings -- that's the point
> where the ATA driver stops being generic, like pata_platform, and there
> arises a need for the dedicated driver. Also, your patch would bring in
> disparity with the ide-platfrom driver (which should be interchangeable
> with pata_platfrom). For me, the need of a separate driver is clear now,
> so I'll remain opposed to your patch. Of course, the maintainer (Jeff
> Garzik) will decide but if I could veto this patch I would.

I think there's a case to be made for doing some refactoring to allow 
splitting the code related to this hardware into a different file or 
something. However, creating an entirely different driver when the only 
thing different from pata_platform is that function seems excessive.

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Sergei Shtylyov]

Hello.

Robert Hancock wrote:
> On 05/10/2010 03:51 AM, Sergei Shtylyov wrote:
>> Hello.
>>
>> Graeme Russ wrote:
>>
>>> Sergei Shtylyov wrote:
>>>> Hello.
>>>>
>>>> Graeme Russ wrote:
>>>>
>>>>> [Added linux-ide back onto distribution list]
>>>> Right, I didn't intend to exclude it -- probably didn't press all the
>>>> keys at once for the reply-to-all function.
>>>>
>>>> [...]
>>>>>> You should have also taught the symmetric ide-platfrom driver the 
>>>>>> same
>>>>>> trick. However, IDE core's data transfer methods has a different
>>>>>> prototype.
>
> The IDE subsystem is deprecated and in maintenance mode,

   I know, I know. :-)

> it shouldn't be growing support for new hardware, which I assume this is.

   This is not a new hardware as it's going to use an existing driver. 
Also, despite maintenance mode, there was a patch accepted recently 
restoring feature parity between ide-platfrom and pata_platfrom.

>>>>> I did think about the other drivers (OF Platform etc) but I have 
>>>>> no way of testing them.
>>>> pata_of_platform is not easily extensible in this way. It gets all the
>>>> information about the device from the device tree -- and you can't
>>>> encode all your complications there, unless you invent a new OF device.

   Besides, Graeme, for what arch is your hardware? If it's PowerPC, you 
should be using pata_of_platform -- but as I said you can't really do it.

>>>>>> I suggest to rather add a new flag, indicating 8-bit data I/O, and
>>>>>> have
>>>>>> the alternate sff_data_xfer() method defined inside the driver.
>>>
>>> The vast majority of implementations are 16-bit (no one has complained
>>> about the lack of 8-bit support to date). I don't think the majority of
>>> users should be carrying around the extra code for a tiny minority.
>>> Yes, it
>>> could be wrapped around an #ifdef but then things start to get ugly 
>>> (why
>>> not just ditch the flag and #ifdef the 8-bit transfers entirely, hack
>>> Kconfig etc) eeewwwww....
>>
>> I didn't propose any of this. Anyway, this is not an option anymore now
>> that we know enough about your hardware.
>>
>>>>> other devices on the bus). By overriding the data transfer function
>>>>> I can
>>>>> arbitrate access to the bus and switch the bus timings based on the
>>>>> peripheral being accessed. This cannot be done be a generic data
>>>>> transfer
>>>>> function.
>>>> I disagree with your approach of overriding the libata methods at the
>>>> board level, so I suggest to write a new driver. This is too 
>>>> complicated
>>>> stuff for poor old pata_platform. :-)
>>>
>>> My custom function to date looks like:
>>>
>>> unsigned int ata_enet_data_xfer(struct ata_device *dev, unsigned char
>>> *buf,
>>> unsigned int buflen, int rw)
>>> {
>>> struct ata_port *ap = dev->link->ap;
>>> void __iomem *data_addr = ap->ioaddr.data_addr;
>>>
>>> set_gp_bus_slow();
>>> /* Transfer bytes */
>>> if (rw == READ)
>>> ioread8_rep(data_addr, buf, buflen);
>>> else
>>> iowrite8_rep(data_addr, buf, buflen);
>>>
>>> set_gp_bus_fast();
>>> return buflen;
>>> }
>>>
>>> set_gp_bus_slow() and set_gp_bus_fast() (at the moment) simply set a 
>>> few
>>> config registers to set the GP bus timing (no arbitration yet, but 
>>> these
>>> functions will also handle that using a mutex). I don't see the 
>>> point in
>>> re-writing the entire PATA Platform driver when the existing driver
>>> appears
>>> to be perfectly capable with a very minor extension hook.
>>
>> As I said, we *can't* implement the driver methods at the board level.
>> Especially if they involve messing with timings -- that's the point
>> where the ATA driver stops being generic, like pata_platform, and there
>> arises a need for the dedicated driver. Also, your patch would bring in
>> disparity with the ide-platfrom driver (which should be interchangeable
>> with pata_platfrom). For me, the need of a separate driver is clear now,
>> so I'll remain opposed to your patch. Of course, the maintainer (Jeff
>> Garzik) will decide but if I could veto this patch I would.
>
> I think there's a case to be made for doing some refactoring to allow 
> splitting the code related to this hardware into a different file or 
> something. However, creating an entirely different driver when the 
> only thing different from pata_platform is that function seems excessive.

    You propose something like pata_of_platform (riding on top of 
pata_platform)? Anyway, I suspect that we have fully programmable bus 
hardware here, which should allow for PIO mode setting, and hence woud 
really need a whole new driver...

WBR, Sergei

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Graeme Russ]

On 13/05/10 06:58, Sergei Shtylyov wrote:
> Hello.
> 
> Robert Hancock wrote:
>> On 05/10/2010 03:51 AM, Sergei Shtylyov wrote:
>>> Hello.
>>>
>>> Graeme Russ wrote:
>>>
>>>> Sergei Shtylyov wrote:
>>>>> Hello.
>>>>>
>>>>> Graeme Russ wrote:
>>>>>
>>>>>> [Added linux-ide back onto distribution list]
>>>>> Right, I didn't intend to exclude it -- probably didn't press all the
>>>>> keys at once for the reply-to-all function.
>>>>>
>>>>> [...]
>>>>>>> You should have also taught the symmetric ide-platfrom driver the
>>>>>>> same
>>>>>>> trick. However, IDE core's data transfer methods has a different
>>>>>>> prototype.
>>
>> The IDE subsystem is deprecated and in maintenance mode,
> 
>   I know, I know. :-)
> 
>> it shouldn't be growing support for new hardware, which I assume this is.
> 
>   This is not a new hardware as it's going to use an existing driver.

Correct - it is a direct attachment of a CF card onto an 8-bit data bus

> Also, despite maintenance mode, there was a patch accepted recently
> restoring feature parity between ide-platfrom and pata_platfrom.
> 

>>>>>> I did think about the other drivers (OF Platform etc) but I have
>>>>>> no way of testing them.
>>>>> pata_of_platform is not easily extensible in this way. It gets all the
>>>>> information about the device from the device tree -- and you can't
>>>>> encode all your complications there, unless you invent a new OF
>>>>> device.
> 
>   Besides, Graeme, for what arch is your hardware? If it's PowerPC, you
> should be using pata_of_platform -- but as I said you can't really do it.
> 

This is for an embedded x86 solutions (AMD SC520). Note: By embedded I
really do mean embedded, not mini-PC or the like (i.e. no BIOS)

[snip]

>> I think there's a case to be made for doing some refactoring to allow
>> splitting the code related to this hardware into a different file or
>> something. However, creating an entirely different driver when the
>> only thing different from pata_platform is that function seems excessive.
> 

I agree - It is a pretty poor driver architecture that requires an entirely
new driver for the sake of <10% change in functionality. I would agree that
if a driver has override hooks for over 50% of its functions and you are
using all of those hooks then perhaps a dedicated driver is the way to go,
but adding a 8-bit versus 16-bit access hook is probably the most trivial
and fundamental hook you could add to a device driver (even more
fundamental than the existing byte stuffing hook IMHO)

>    You propose something like pata_of_platform (riding on top of
> pata_platform)? Anyway, I suspect that we have fully programmable bus
> hardware here, which should allow for PIO mode setting, and hence woud
> really need a whole new driver...

Yes, the bus timing is fully programmable - It allows the insertion of an
integer number of 33MHz bus cycles into various stages of the bus access.

I don't really care for changing bus timings wrt PIO mode supported by the
CF card. It is not a really high speed CPU (486 DX4100 equivalent) so
supporting high speed data transfers without DMA is a bit pointless. The
only timing switch I care for is 'Accessing CF / Not Accessing CF' which
has a timing difference of 5 or so bus cycles.

And truthfully, I don't even think I need to mutex the bus - The other
devices can use the bus using the slower timings anyway so it does not
really matter if a context switch occurs during the CF read/write in order
to access another device

Regards,

Graeme

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Sergei Shtylyov]

Hello.

Graeme Russ wrote:

>>>>>>> [Added linux-ide back onto distribution list]
>>>>>>>               
>>>>>> Right, I didn't intend to exclude it -- probably didn't press all the
>>>>>> keys at once for the reply-to-all function.
>>>>>>
>>>>>> [...]
>>>>>>             
>>>>>>>> You should have also taught the symmetric ide-platfrom driver the
>>>>>>>> same
>>>>>>>> trick. However, IDE core's data transfer methods has a different
>>>>>>>> prototype.
>>>>>>>>                 
>>> The IDE subsystem is deprecated and in maintenance mode,
>>>       
>>   I know, I know. :-)
>>
>>     
>>> it shouldn't be growing support for new hardware, which I assume this is.
>>>       
>>   This is not a new hardware as it's going to use an existing driver.
>>     

   If it ends up using it. I tend to think about it as a truly new 
hardware needing a separate driver, so IDE support would really be out 
of question.

> Correct - it is a direct attachment of a CF card onto an 8-bit data bus
>   

   And we have several *separate* drivers for the alike CF cases.

>> Also, despite maintenance mode, there was a patch accepted recently
>> restoring feature parity between ide-platfrom and pata_platfrom.
>>
>>     
>
>   
>>>>>>> I did think about the other drivers (OF Platform etc) but I have
>>>>>>> no way of testing them.
>>>>>>>               
>>>>>> pata_of_platform is not easily extensible in this way. It gets all the
>>>>>> information about the device from the device tree -- and you can't
>>>>>> encode all your complications there, unless you invent a new OF
>>>>>> device.
>>>>>>             
>>   Besides, Graeme, for what arch is your hardware? If it's PowerPC, you
>> should be using pata_of_platform -- but as I said you can't really do it.
>>
>>     
>
> This is for an embedded x86 solutions (AMD SC520). Note: By embedded I
> really do mean embedded, not mini-PC or the like (i.e. no BIOS)
>   

   Ah, I saw your initial request on u-boot maling list which mentioned 
OF platfrom driver and assumed you might be on a PowerPC machine...

> [snip]
>
>   
>>> I think there's a case to be made for doing some refactoring to allow
>>> splitting the code related to this hardware into a different file or
>>> something. However, creating an entirely different driver when the
>>> only thing different from pata_platform is that function seems excessive.
>>>       
>
> I agree - It is a pretty poor driver architecture that requires an entirely
> new driver for the sake of <10% change in functionality. I would agree that
>   

   I tend to disagree on the percentage with you.

> if a driver has override hooks for over 50% of its functions and you are
> using all of those hooks then perhaps a dedicated driver is the way to go,
> but adding a 8-bit versus 16-bit access hook is probably the most trivial
> and fundamental hook you could add to a device driver (even more
> fundamental than the existing byte stuffing hook IMHO)
>   

   What byte stuffing do you mean?

>>    You propose something like pata_of_platform (riding on top of
>> pata_platform)? Anyway, I suspect that we have fully programmable bus
>> hardware here, which should allow for PIO mode setting, and hence woud
>> really need a whole new driver...
>>     
>
> Yes, the bus timing is fully programmable - It allows the insertion of an
> integer number of 33MHz bus cycles into various stages of the bus access.
>
> I don't really care for changing bus timings wrt PIO mode supported by the
>   

   Let's not allow laziness to be our guide in making fundamental 
decisions. ;-)

> CF card. It is not a really high speed CPU (486 DX4100 equivalent) so
> supporting high speed data transfers without DMA is a bit pointless. The
>   

   People wrote a lot of VLB drivers in order to get high PIO speeds on 
i486. And PIO4 gives *significant* speed increase over PIO0. Although I 
suspect libata's PIO speeds are still painfully low compared to IDE core 
-- need to re-check this with the recent kernel. So, chosing libata over 
IDE might not be so obvious advantage as it seems...

> only timing switch I care for is 'Accessing CF / Not Accessing CF' which
> has a timing difference of 5 or so bus cycles.
>   

   There could be *negative* difference with high PIO modes -- 
PIO4-to-PIO0 cycle ratio is 5 (600:120).

> And truthfully, I don't even think I need to mutex the bus - The other
> devices can use the bus using the slower timings anyway so it does not
> really matter if a context switch occurs during the CF read/write in order
> to access another device
>   

   You'd better be careful anyway.

> Regards,
>
> Graeme
>   

WBR, Sergei

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Graeme Russ]

On Fri, May 14, 2010 at 7:02 AM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
> Graeme Russ wrote:
>

[snip]

>>>> it shouldn't be growing support for new hardware, which I assume this
>>>> is.
>>>>
>>>
>>>  This is not a new hardware as it's going to use an existing driver.

That is a really daft sentance on my behalf - I should have said that the
hardware design is not conceptually new (i.e. directly wiring a CF card
onto an 8-bit bus is nothing new) and therefore should be able to use an
existing driver

>
>  If it ends up using it. I tend to think about it as a truly new hardware
> needing a separate driver, so IDE support would really be out of question.
>
>> Correct - it is a direct attachment of a CF card onto an 8-bit data bus
>>
>
>  And we have several *separate* drivers for the alike CF cases.

Can you elaborate - I thought the PATA Platform driver was designed to
unify the very discontguous approaches that have previously been taken
in the embedded designs. As such, the need for these various drivers
should diminished.

[snip]

>>>
>>>  Besides, Graeme, for what arch is your hardware? If it's PowerPC, you
>>> should be using pata_of_platform -- but as I said you can't really do it.
>>>
>>>
>>
>> This is for an embedded x86 solutions (AMD SC520). Note: By embedded I
>> really do mean embedded, not mini-PC or the like (i.e. no BIOS)
>>
>
>  Ah, I saw your initial request on u-boot maling list which mentioned OF
> platfrom driver and assumed you might be on a PowerPC machine...

Pardon my ignorance but when I asked that question I did not realise that
'OF' was 'Open Firmware' and, ergo, PPC

[snip]

>> if a driver has override hooks for over 50% of its functions and you are
>> using all of those hooks then perhaps a dedicated driver is the way to go,
>> but adding a 8-bit versus 16-bit access hook is probably the most trivial
>> and fundamental hook you could add to a device driver (even more
>> fundamental than the existing byte stuffing hook IMHO)
>>
>
>  What byte stuffing do you mean?

Sorry, I meant the 'Port Shifting' (I/O port shift, for platforms with ports
that are constantly spaced and need larger than the 1-byte spacing used by
ata_std_ports())

So yes, in theory, your original idea of adding an '8-bit flag' would be
more in keeping with Port Shift (i.e. tell the driver to use a specifically
non-standard function which *is* implemented in the driver core). But this
goes back to my prior arguments - why pollute the driver core with non-
standard functions when a simple hook which is ignored if NULL will do.
Now if we then find that many different developers are using a particular
hook to do the same thing, that 'thing' becomes 'common' and gets merged
into the driver core later.

>
>>>   You propose something like pata_of_platform (riding on top of
>>> pata_platform)? Anyway, I suspect that we have fully programmable bus
>>> hardware here, which should allow for PIO mode setting, and hence woud
>>> really need a whole new driver...
>>>
>>
>> Yes, the bus timing is fully programmable - It allows the insertion of an
>> integer number of 33MHz bus cycles into various stages of the bus access.
>>
>> I don't really care for changing bus timings wrt PIO mode supported by the
>>
>
>  Let's not allow laziness to be our guide in making fundamental decisions.
> ;-)

True, and well very well said. This is only a hobby platform for me - it is
not likely to leave my bench but I would still like to contribute something
for the greater good.

>
>> CF card. It is not a really high speed CPU (486 DX4100 equivalent) so
>> supporting high speed data transfers without DMA is a bit pointless. The
>>
>
>  People wrote a lot of VLB drivers in order to get high PIO speeds on i486.
> And PIO4 gives *significant* speed increase over PIO0. Although I suspect
> libata's PIO speeds are still painfully low compared to IDE core -- need to
> re-check this with the recent kernel. So, chosing libata over IDE might not
> be so obvious advantage as it seems...

You will have tp pardon my ignorance - Until a few weeks ago I knew nothing
about CF / IDE / ATA / PIO etc. I do not fully understand how the various
PIO modes are implemented.

>
>> only timing switch I care for is 'Accessing CF / Not Accessing CF' which
>> has a timing difference of 5 or so bus cycles.
>>
>
>  There could be *negative* difference with high PIO modes -- PIO4-to-PIO0
> cycle ratio is 5 (600:120).
>

Sorry, I do not understand.

>> And truthfully, I don't even think I need to mutex the bus - The other
>> devices can use the bus using the slower timings anyway so it does not
>> really matter if a context switch occurs during the CF read/write in order
>> to access another device
>>
>
>  You'd better be careful anyway.

Yes, I would prefer to do the right thing anyway

Regards,

Graeme

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Sergei Shtylyov]

Hello.

Graeme Russ wrote:

    Sorry for the belated reply, I've been somewhat busy.

>>  If it ends up using it. I tend to think about it as a truly new hardware
>> needing a separate driver, so IDE support would really be out of question.

>>> Correct - it is a direct attachment of a CF card onto an 8-bit data bus

>>  And we have several *separate* drivers for the alike CF cases.

> Can you elaborate - I thought the PATA Platform driver was designed to
> unify the very discontguous approaches that have previously been taken
> in the embedded designs. As such, the need for these various drivers
> should diminished.

    Look at pata_ixp4xx_cf as an example. Part of what it does is 
override the sff_data_xfer() method to temporarily switch the bus from 
8-bit to 16-bit mode (i.e. *almost* the same thing as you're going to do 
in your override). Other CF drivers (like pata_octeon_cf) have DMA 
support though...

> [snip]

>>>>  Besides, Graeme, for what arch is your hardware? If it's PowerPC, you
>>>> should be using pata_of_platform -- but as I said you can't really do it.

>>> This is for an embedded x86 solutions (AMD SC520). Note: By embedded I
>>> really do mean embedded, not mini-PC or the like (i.e. no BIOS)

>>  Ah, I saw your initial request on u-boot maling list which mentioned OF
>> platfrom driver and assumed you might be on a PowerPC machine...

> Pardon my ignorance but when I asked that question I did not realise that
> 'OF' was 'Open Firmware' and, ergo, PPC

    Oh, there are ongoing efforts to port the most used concept of OF, 
i.e. device tree to ARM and MIPS... Hence, pata_of_platform would be 
able to run on these archs as well...

> [snip]

>>> if a driver has override hooks for over 50% of its functions and you are
>>> using all of those hooks then perhaps a dedicated driver is the way to go,
>>> but adding a 8-bit versus 16-bit access hook is probably the most trivial
>>> and fundamental hook you could add to a device driver (even more
>>> fundamental than the existing byte stuffing hook IMHO)

>>  What byte stuffing do you mean?

> Sorry, I meant the 'Port Shifting' (I/O port shift, for platforms with ports
> that are constantly spaced and need larger than the 1-byte spacing used by
> ata_std_ports())

> So yes, in theory, your original idea of adding an '8-bit flag' would be
> more in keeping with Port Shift (i.e. tell the driver to use a specifically
> non-standard function which *is* implemented in the driver core). But this
> goes back to my prior arguments - why pollute the driver core with non-
> standard functions

    8-bit I/O is pretty standard, just rarely used method of transfer in 
the ATA world. The PCMCIA ATA driver, pata_pcmcia implements it, for 
example.

> when a simple hook which is ignored if NULL will do.
> Now if we then find that many different developers are using a particular
> hook to do the same thing, that 'thing' becomes 'common' and gets merged
> into the driver core later.

>>> CF card. It is not a really high speed CPU (486 DX4100 equivalent) so
>>> supporting high speed data transfers without DMA is a bit pointless. The

>>  People wrote a lot of VLB drivers in order to get high PIO speeds on i486.
>> And PIO4 gives *significant* speed increase over PIO0. Although I suspect
>> libata's PIO speeds are still painfully low compared to IDE core -- need to

    I still really need to measure this another time...

>> re-check this with the recent kernel. So, chosing libata over IDE might not
>> be so obvious advantage as it seems...

> You will have tp pardon my ignorance - Until a few weeks ago I knew nothing
> about CF / IDE / ATA / PIO etc. I do not fully understand how the various
> PIO modes are implemented.

    The most important thing is that the higher is the PIO mode is, the 
lower is the transfer cycle time, and hence the transfer speed.

>>> only timing switch I care for is 'Accessing CF / Not Accessing CF' which
>>> has a timing difference of 5 or so bus cycles.

>>  There could be *negative* difference with high PIO modes -- PIO4-to-PIO0
>> cycle ratio is 5 (600:120).

> Sorry, I do not understand.

    PIO mode 0 (PIO0), the most compatible mode that should be 
implemented by default, has 600 ns transfer cycle; the highest standard 
PIO mode 4 (PIO4) has 120 ns cycle, so is (theoretically) 5 times faster 
than PIO0.

> Regards,

> Graeme

MBR, Sergei

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Graeme Russ]

On 27/05/10 00:05, Sergei Shtylyov wrote:
> Hello.
> 
> Graeme Russ wrote:
> 
>    Sorry for the belated reply, I've been somewhat busy.

Ditto - I really have not had much time to think more on this one.

To briefly sum up all the arguments, I see three options:

1) Write a new driver based on the PATA Platform driver specifically for my
needs
2) Add the 'custom data xfer hook' to both the PATA and IDE Platform
drivers and implement the 8-bit data xfer functions outside the driver
(board specific code)
3) Add an '8-bit xfer' flag to the PATA and IDE Platform drivers and add
the 8-bit data xfer in the driver

I see #1 as the lowest (zero) impact on current kernel code but #2 is by
far the most flexible and appears to follow driver design philosophy. #3 is
very restrictive and opens up a Pandora's Box of driver code hacks to
support each and every esoteric implementation.

If the consensus is on #2, I'll respin a new patch for both the PATA and
IDE Platform drivers

Regards,

Graeme

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Alan Cox]

>    As I said, we *can't* implement the driver methods at the board 
> level. Especially if they involve messing with timings -- that's the 
> point where the ATA driver stops being generic, like pata_platform, and 
> there arises a need for the dedicated driver. Also, your patch would 
> bring in disparity with the ide-platfrom driver (which should be 
> interchangeable with pata_platfrom). For me, the need of a separate 
> driver is clear now, so I'll remain opposed to your patch. Of course, 
> the maintainer (Jeff Garzik) will decide but if I could veto this patch 
> I would.

I don't think ide-platform matters in this case. You can certainly add
the same support to the old ide_platform driver, but the old code can't
be allowed to block progress with newer stuff. It's also trivial to add

	if (we have an xfer method) {
		printk("Oh poo");
		return -ENODEV;
	}

to the IDE one so it simply refuses to bind to more featureful
implementatins.

I also don't see a problem with the transfer function needing to do
arbitration, byte stuffing and other magic - that's quite common on
embedded weirdnesses.

The point it ceases to make sense is where you need to do mode setting.
If the GPIO frobbing is managing the platform bus requirements then it
makes sense as a platform function. If it's going to grow into full ATA
set xfer mode support it probably turns into a new driver at some point.

Either way it makes sense to support overriding the data_xfer operation,
and we've done something analogous for years with 8250 serial and it has
been a *huge* success and saved us having about thirty similar
platform serial drivers.

Alan

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>>    As I said, we *can't* implement the driver methods at the board 
>> level. Especially if they involve messing with timings -- that's the 
>> point where the ATA driver stops being generic, like pata_platform, and 
>> there arises a need for the dedicated driver. Also, your patch would 
>> bring in disparity with the ide-platfrom driver (which should be 
>> interchangeable with pata_platfrom). For me, the need of a separate 
>> driver is clear now, so I'll remain opposed to your patch. Of course, 
>> the maintainer (Jeff Garzik) will decide but if I could veto this patch 
>> I would.
>>     
>
> I don't think ide-platform matters in this case.

   You've just supported a patch restoring feature parity between 
pata_platform and ide-platfrom WRT IRQ flags. ;-)

> You can certainly add
> the same support to the old ide_platform driver, but the old code can't
> be allowed to block progress with newer stuff. It's also trivial to add
>
> 	if (we have an xfer method) {
> 		printk("Oh poo");
> 		return -ENODEV;
> 	}
>
> to the IDE one so it simply refuses to bind to more featureful
> implementatins.
>   

   Yes, but this needs to be *done* too, preferrably by the author of 
the original patch and simultaneously with it. We can't accept a single 
patch that only touches pata_platform.

> I also don't see a problem with the transfer function needing to do
> arbitration, byte stuffing and other magic - that's quite common on
> embedded weirdnesses.
>   

   Agreed.

> The point it ceases to make sense is where you need to do mode setting.
> If the GPIO frobbing is managing the platform bus requirements then it
> makes sense as a platform function. If it's going to grow into full ATA
> set xfer mode support it probably turns into a new driver at some point.
>   

   In this case I do suspect we have fully programmable bus timings, and 
hence should be able to do mode setting. Partly because of that, I'd 
still like to see this case handled with a separate driver.

> Either way it makes sense to support overriding the data_xfer operation,
> and we've done something analogous for years with 8250 serial and it has
> been a *huge* success and saved us having about thirty similar
> platform serial drivers.
>   

   Well, with 8250 we still don't allow for overriding things at the 
board level, via the platform data callbacks.

> Alan
>   

WBR, Sergei

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Alan Cox]

> > I don't think ide-platform matters in this case.
> 
>    You've just supported a patch restoring feature parity between 
> pata_platform and ide-platfrom WRT IRQ flags. ;-)

Because it was trivial to do.

>    Yes, but this needs to be *done* too, preferrably by the author of 
> the original patch and simultaneously with it. We can't accept a single 
> patch that only touches pata_platform.

Sure - or support it in both. I've no problem with both supporting that
function, just with legacy code blocking progress.

>    Well, with 8250 we still don't allow for overriding things at the 
> board level, via the platform data callbacks.

Oh yes we do. Platform specific drivers can directly override the access
operators and they do some truely horrible platform specific stuff in the
overridden operators.

And the great thing - nobody else has to know what the board vendors have
been on..

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Graeme Russ]

On 13/05/10 09:13, Alan Cox wrote:
>>> I don't think ide-platform matters in this case.
>>
>>    You've just supported a patch restoring feature parity between 
>> pata_platform and ide-platfrom WRT IRQ flags. ;-)
> 
> Because it was trivial to do.
> 
>>    Yes, but this needs to be *done* too, preferrably by the author of 
>> the original patch and simultaneously with it. We can't accept a single 
>> patch that only touches pata_platform.

I'm happy to expand the patch, but why would we implement such a thing in a
legacy driver. Odds are that nobody would ever use it. If I had developed
my code prior to pata_platform and used the old IDE driver instead I would
of patched it then (and it may have been included in pata_platform). If
anyone requires this hook from today on, odds on it is for a new system and
the old IDE driver will not even be considered for the task. Besides, by
only implementing these features in the latest (supported) drivers, it will
encourage people to move away from the legacy drivers which is a Good Thing(tm)

> 
> Sure - or support it in both. I've no problem with both supporting that
> function, just with legacy code blocking progress.
> 
>>    Well, with 8250 we still don't allow for overriding things at the 
>> board level, via the platform data callbacks.
> 
> Oh yes we do. Platform specific drivers can directly override the access
> operators and they do some truely horrible platform specific stuff in the
> overridden operators.
> 
> And the great thing - nobody else has to know what the board vendors have
> been on..
> 

Exactly - This is good driver design. Implement the fundamentals for the
majority of use-cases and provide overrides to allow individual
implementations which can do anything weird and wonderful without polluting
the code space of everyone else.

Regards,

Graeme

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Sergei Shtylyov]

Hello.

Graeme Russ wrote:

>>>> I don't think ide-platform matters in this case.

>>>    You've just supported a patch restoring feature parity between 
>>> pata_platform and ide-platfrom WRT IRQ flags. ;-)

>> Because it was trivial to do.

>>>    Yes, but this needs to be *done* too, preferrably by the author of 
>>> the original patch and simultaneously with it. We can't accept a single 
>>> patch that only touches pata_platform.

> I'm happy to expand the patch,  but why would we implement such a thing in a
> legacy driver.

    Because the drivers are intended to be interchangeable.

> Odds are that nobody would ever use it.

    As I said a choice of libata over IDE might not be so obvious 
advantage as some people tend to think. The ide-platfrom is still used 
in embedded, given that it's actually quite young driver.

> If I had developed
> my code prior to pata_platform and used the old IDE driver instead I would
> of

    Can't parse "would of" -- did you mean "would have"? :-)

> patched it then (and it may have been included in pata_platform).

    You'll be surprised to know that pata_platfrom actually *predates* 
ide-platfrom. ;-)

> If anyone requires this hook from today on, odds on it is for a new system and
> the old IDE driver will not even be considered for the task.

    Odds can be odd enough -- try measuring/comparing the performance 
you'll get with libata and IDE drivers for example.

> Besides, by
> only implementing these features in the latest (supported) drivers, it will
> encourage people to move away from the legacy drivers which is a Good Thing(tm)

    As Alan said, you should at least fail gracefully in the 
ide-platfrom, and not leave it ignorant of your change.

>> Sure - or support it in both. I've no problem with both supporting that
>> function, just with legacy code blocking progress.

>>>    Well, with 8250 we still don't allow for overriding things at the 
>>> board level, via the platform data callbacks.

>> Oh yes we do. Platform specific drivers can directly override the access
>> operators and they do some truely horrible platform specific stuff in the
>> overridden operators.

    That's something new to me. Looking at <linux/serial_8250.h>, you're 
right. But this got added only in January, due to Cavium.

>> And the great thing - nobody else has to know what the board vendors have
>> been on..

    Well, mainly SoC vendors...

> Exactly - This is good driver design. Implement the fundamentals for the
> majority of use-cases and provide overrides to allow individual
> implementations which can do anything weird and wonderful without polluting
> the code space of everyone else.

    You should have looked at 8250.c first -- there's still lot of 
pollution left because old accessors were all left in place, including 
the unfortunate Alchemy UART's ones. :-)

> Regards,

> Graeme

WBR, Sergei

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Alan Cox]

>     You should have looked at 8250.c first -- there's still lot of 
> pollution left because old accessors were all left in place, including 
> the unfortunate Alchemy UART's ones. :-)

That is one very good example of why I want to see this sorted properly
the first time around with the ide/ata case !

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Graeme Russ]

On Fri, May 14, 2010 at 7:22 AM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
> Graeme Russ wrote:
>
>>>>> I don't think ide-platform matters in this case.
>
>>>>   You've just supported a patch restoring feature parity between
>>>> pata_platform and ide-platfrom WRT IRQ flags. ;-)
>
>>> Because it was trivial to do.
>
>>>>   Yes, but this needs to be *done* too, preferrably by the author of the
>>>> original patch and simultaneously with it. We can't accept a single patch
>>>> that only touches pata_platform.
>
>> I'm happy to expand the patch,  but why would we implement such a thing in
>> a
>> legacy driver.
>
>   Because the drivers are intended to be interchangeable.

I thought the whole purpose of marking a driver 'legacy' was to no longer
enhance its features and encourage people to migrate away from it so that
one day it can be removed entirely.

I also think I have a massive understanding gap with the idea of
interchangeability between IDE and ATA (please excuse me for that) - I
thought that the pata_platform driver is designed to be 'hard-wired' for
embedded systems (i.e. the board developer explicitly sets up address
ranges etc used for initialization in the board init function). I didn't
think interchangeability with another driver platform was a goal.

>
>> Odds are that nobody would ever use it.
>
>   As I said a choice of libata over IDE might not be so obvious advantage as
> some people tend to think. The ide-platfrom is still used in embedded, given
> that it's actually quite young driver.
>
>> If I had developed
>> my code prior to pata_platform and used the old IDE driver instead I would
>> of
>
>   Can't parse "would of" -- did you mean "would have"? :-)

Yes, (Aussie English) ;-)

>
>> patched it then (and it may have been included in pata_platform).
>
>   You'll be surprised to know that pata_platfrom actually *predates*
> ide-platfrom. ;-)
>

Yes, I am

>> If anyone requires this hook from today on, odds on it is for a new system
>> and
>> the old IDE driver will not even be considered for the task.
>
>   Odds can be odd enough -- try measuring/comparing the performance you'll
> get with libata and IDE drivers for example.
>
>> Besides, by
>> only implementing these features in the latest (supported) drivers, it
>> will
>> encourage people to move away from the legacy drivers which is a Good
>> Thing(tm)
>
>   As Alan said, you should at least fail gracefully in the ide-platfrom, and
> not leave it ignorant of your change.
>
>>> Sure - or support it in both. I've no problem with both supporting that
>>> function, just with legacy code blocking progress.
>
>>>>   Well, with 8250 we still don't allow for overriding things at the
>>>> board level, via the platform data callbacks.
>
>>> Oh yes we do. Platform specific drivers can directly override the access
>>> operators and they do some truely horrible platform specific stuff in the
>>> overridden operators.
>
>   That's something new to me. Looking at <linux/serial_8250.h>, you're
> right. But this got added only in January, due to Cavium.
>
>>> And the great thing - nobody else has to know what the board vendors have
>>> been on..
>
>   Well, mainly SoC vendors...
>
>> Exactly - This is good driver design. Implement the fundamentals for the
>> majority of use-cases and provide overrides to allow individual
>> implementations which can do anything weird and wonderful without
>> polluting
>> the code space of everyone else.
>
>   You should have looked at 8250.c first -- there's still lot of pollution
> left because old accessors were all left in place, including the unfortunate
> Alchemy UART's ones. :-)

I've had a quick look at 8250 and, yes, the serial_in / serial_out overrides
are exactly what I have proposed for PATA Platform. I do not see how this
could be considered bad driver design IMHO. As I said before, provide
baseline functionality for the majority (80%), designed-in extensibility
for the minority (15%) and force the last 5% into writing their own driver
as a lesson to them for poor hardware design decisions ;)


Regards,

Graeme

Re: [PATCH] Add hook for custom xfer function in PATA Platform driver

[Sergei Shtylyov]

Graeme Russ wrote:

>>>>>> I don't think ide-platform matters in this case.
>>>>>   You've just supported a patch restoring feature parity between
>>>>> pata_platform and ide-platfrom WRT IRQ flags. ;-)
>>>> Because it was trivial to do.
>>>>>   Yes, but this needs to be *done* too, preferrably by the author of the
>>>>> original patch and simultaneously with it. We can't accept a single patch
>>>>> that only touches pata_platform.
>>> I'm happy to expand the patch,  but why would we implement such a thing in
>>> a
>>> legacy driver.

>>   Because the drivers are intended to be interchangeable.

> I thought the whole purpose of marking a driver 'legacy' was to no longer
> enhance its features and encourage people to migrate away from it so that
> one day it can be removed entirely.

> I also think I have a massive understanding gap with the idea of
> interchangeability between IDE and ATA (please excuse me for that) - I
> thought that the pata_platform driver is designed to be 'hard-wired' for
> embedded systems (i.e. the board developer explicitly sets up address
> ranges etc used for initialization in the board init function). I didn't
> think interchangeability with another driver platform was a goal.

    The 'ide_platform' driver handles exactly the same platform device 
that 'pata_platform' does, so that the drivers can be swapped over 
without changing anything in the code. They are quite coupled together, 
so a change in one driver should ensue a parallel change in the other 
one; or at least, we should explicitly mark the new 'pata_platform' 
driver feature as unsupported in the 'ide_platform' driver -- one way or 
another, the latter driver needs to be aware of it.

>>> Odds are that nobody would ever use it.
>>   As I said a choice of libata over IDE might not be so obvious advantage as
>> some people tend to think. The ide-platfrom is still used in embedded, given
>> that it's actually quite young driver.

>>> If I had developed
>>> my code prior to pata_platform and used the old IDE driver instead I would
>>> of
>>   Can't parse "would of" -- did you mean "would have"? :-)

> Yes, (Aussie English) ;-)

    Strange, I've also encountered that with non-Australian speakers. 
Must be some form of English slang...

>>> patched it then (and it may have been included in pata_platform).

>>   You'll be surprised to know that pata_platfrom actually *predates*
>> ide-platfrom. ;-)

> Yes, I am

    Now you should see that there were a need in the IDE driver still, 
despite the libata one already existed. So MontaVista had to write it 
(although some form of IDE platform driver had existed for some time out 
of tree -- it was called something like ide-cf, and was written exectly 
to support various kinds of CF encountered on the embedded boards).

>>>> Sure - or support it in both. I've no problem with both supporting that
>>>> function, just with legacy code blocking progress.
>>>>>   Well, with 8250 we still don't allow for overriding things at the
>>>>> board level, via the platform data callbacks.
>>>> Oh yes we do. Platform specific drivers can directly override the access
>>>> operators and they do some truely horrible platform specific stuff in the
>>>> overridden operators.

>>   That's something new to me. Looking at <linux/serial_8250.h>, you're
>> right. But this got added only in January, due to Cavium.

>>>> And the great thing - nobody else has to know what the board vendors have
>>>> been on..

>>   Well, mainly SoC vendors...

>>> Exactly - This is good driver design. Implement the fundamentals for the
>>> majority of use-cases and provide overrides to allow individual
>>> implementations which can do anything weird and wonderful without
>>> polluting
>>> the code space of everyone else.

>>   You should have looked at 8250.c first -- there's still lot of pollution
>> left because old accessors were all left in place, including the unfortunate
>> Alchemy UART's ones. :-)

> I've had a quick look at 8250 and, yes, the serial_in / serial_out overrides
> are exactly what I have proposed for PATA Platform. I do not see how this
> could be considered bad driver design IMHO.

    As I've said, the platform data overrides were a recent addition, 
before that the driver was "polluted" by e.g. the Alchemy UART accessors 
(which were result of merging support for this "not very compatible" 
UART from a separate driver -- which just blindly copied most of 
8250.c). This stuff still pollutes 8250.c, although it should be hidden 
by the overrides now...

> As I said before, provide
> baseline functionality for the majority (80%), designed-in extensibility
> for the minority (15%) and force the last 5% into writing their own driver
> as a lesson to them for poor hardware design decisions ;)

    It's only not clear where to draw a line between those 15% and 5%... :-)

> Regards,

> Graeme

MBR, Sergei