| Message ID | 1273382493-5859-1-git-send-email-graeme.russ@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
[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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 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.. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 ! -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(-)