| Message ID | 20110208122409.19110.4233.sendpatchset@linux-mhg7.site |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 08 Feb 2011 13:24:09 +0100 Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote: > >From 8acc9371550ce5f98da56f6888ae4b898096ed2c Mon Sep 17 00:00:00 2001 > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > Date: Tue, 8 Feb 2011 12:39:25 +0100 > Subject: [PATCH 05/20] pata_efar: always program master_data before slave_data > > We may need to set SITRE before programming slave_data. Do you have a documentation cite for this or is it random fiddling with a driver you can't test ? -- 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 Tue, Feb 8, 2011 at 2:07 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: > On Tue, 08 Feb 2011 13:24:09 +0100 > Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote: > >> >From 8acc9371550ce5f98da56f6888ae4b898096ed2c Mon Sep 17 00:00:00 2001 >> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> >> Date: Tue, 8 Feb 2011 12:39:25 +0100 >> Subject: [PATCH 05/20] pata_efar: always program master_data before slave_data >> >> We may need to set SITRE before programming slave_data. > > Do you have a documentation cite for this or is it random fiddling with a > driver you can't test ? It is good to enable SITRE register before programming it and all Intel controllers (from which EFAR chipset is derived) are programmed this way (per ata_piix.c). -- 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
> >> We may need to set SITRE before programming slave_data. > > > > Do you have a documentation cite for this or is it random fiddling with a > > driver you can't test ? > > It is good to enable SITRE register before programming it and all > Intel controllers That sounds like someone quoting religion. Documentation cite please. -- 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 Tue, Feb 8, 2011 at 2:25 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: >> >> We may need to set SITRE before programming slave_data. >> > >> > Do you have a documentation cite for this or is it random fiddling with a >> > driver you can't test ? >> >> It is good to enable SITRE register before programming it and all >> Intel controllers > > That sounds like someone quoting religion. Documentation cite please. s/religion/common sense/ -- 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. On 08-02-2011 16:25, Alan Cox wrote: >>>> We may need to set SITRE before programming slave_data. >>> Do you have a documentation cite for this or is it random fiddling with a >>> driver you can't test ? >> It is good to enable SITRE register before programming it and all >> Intel controllers > That sounds like someone quoting religion. Documentation cite please. SLC90E66 datasheet only says that SIDETIM register has no effect without SITRE bit set. 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
> > That sounds like someone quoting religion. Documentation cite please. > > s/religion/common sense/ Anyone who programs ATA controllers on the basis of common sense rather than documentation, errata sheets and actually testing rather than speculating is naïve. In the case of ATA more so than most hardware. Stil waiting a documentation cite for your otherwise gratuitious and untested change -- 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 Tue, Feb 8, 2011 at 2:39 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: >> > That sounds like someone quoting religion. Documentation cite please. >> >> s/religion/common sense/ > > Anyone who programs ATA controllers on the basis of common sense rather > than documentation, errata sheets and actually testing rather than > speculating is naïve. In the case of ATA more so than most hardware. Fully agreed. > Stil waiting a documentation cite for your otherwise gratuitious and > untested change Alan, I'm not doing real-time consulting with you. The code is provided in the best effort manner and it most likely will be underrated (like all ATA/IDE work is) anyway so I do not really care that much if it will be merged ever. Once it will be merged it may only mean more unpaid support time for me since some maintainers are not doing any real maintenance work these days.. Please at least respect some of my time spent on all this burdensome silly little driver differences comparisons and read the whole patch set before making comments on individual changes (which you certainly haven't done given timing of your review mails and complexity of changes).. -- 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
> respect some of my time spent on all this burdensome silly little > driver differences comparisons and read the whole patch set before > making comments on individual changes (which you certainly haven't > done given timing of your review mails and complexity of changes).. I was hoping you'd improved but apparently not. Any untested change is dangerous. An untested change that merges drivers together simply means you can break lots of stuff for no gain at all. If these were all PCI card devices it might make some sense but given they are all motherboard chipsets putting them into one driver merely increases memory use as well. As far as stuff like unsigned int has_sitre = (dev->vendor != 0x8086 || dev->device != 0x1230); and the even worse mess you generate with the added patch all the other PIIX code does this by flags, and if you had a HAS_SITRE (or NO_SITRE) flag in the device data it would be obvious to anyone reading stuff how it all fitted together. 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
On Tue, Feb 8, 2011 at 3:12 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: >> respect some of my time spent on all this burdensome silly little >> driver differences comparisons and read the whole patch set before >> making comments on individual changes (which you certainly haven't >> done given timing of your review mails and complexity of changes).. > > I was hoping you'd improved but apparently not. I'm not the person who doesn't change.. > Any untested change is dangerous. An untested change that merges drivers > together simply means you can break lots of stuff for no gain at all. This is why patches were posted to mailing list with a request for a real hardware testing: "All testing was done using QEMU's PIIX3 controller emulation so any testing with real EFAR, IT8213, old PIIX, RDC and Radisys R82600 PATA controllers would be really appreciated.." instead of request for a merge. It was all there in initial mail. Besides decreased complexity,1400 LOC gone and 5 drivers removed cannot be really called "no gain at all". > If these were all PCI card devices it might make some sense but given > they are all motherboard chipsets putting them into one driver merely > increases memory use as well. We can improve situation here by doing generic ATA or SCSI changes instead, not worth keeping separate drivers when you make similar effect but more maintainable and generic changes. > As far as stuff like > > unsigned int has_sitre = (dev->vendor != 0x8086 || > dev->device != 0x1230); > > and the even worse mess you generate with the added patch all the other > PIIX code does this by flags, and if you had a HAS_SITRE (or NO_SITRE) > flag in the device data it would be obvious to anyone reading stuff how > it all fitted together. Now that's what is a real review work. It will be changed in the next revision, thanks! -- Bartlomiej -- 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. On 08-02-2011 15:24, Bartlomiej Zolnierkiewicz wrote: [...] > We may need to set SITRE before programming slave_data. > This makes pata_efar match the behavior of IDE's slc90e66 host driver > and also of libata's ata_piix one. > Signed-off-by: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com> [...] > diff --git a/drivers/ata/pata_efar.c b/drivers/ata/pata_efar.c > index 1e2ff7d..7f564d7 100644 > --- a/drivers/ata/pata_efar.c > +++ b/drivers/ata/pata_efar.c > @@ -74,10 +74,12 @@ static void efar_set_timings(struct ata_port *ap, struct ata_device *adev, > u8 pio, bool use_mwdma) > { > struct pci_dev *dev = to_pci_dev(ap->host->dev); > + unsigned int is_slave = (adev->devno != 0); What's the point of this variable? To save one pointer dereference? :-) > unsigned long flags; > u8 master_port = ap->port_no ? 0x42 : 0x40; > u16 master_data; > u8 udma_enable; > + u8 slave_data; > int control = 0; > > /* > @@ -110,14 +112,13 @@ static void efar_set_timings(struct ata_port *ap, struct ata_device *adev, > pci_read_config_word(dev, master_port,&master_data); > > /* Set PPE, IE, and TIME as appropriate */ > - if (adev->devno == 0) { > + if (is_slave == 0) { !is_slave appeals more to me. :-) > @@ -126,12 +127,14 @@ static void efar_set_timings(struct ata_port *ap, struct ata_device *adev, > pci_read_config_byte(dev, 0x44,&slave_data); > slave_data&= ap->port_no ? 0x0F : 0xF0; > slave_data |= ((timings[pio][0]<< 2) | timings[pio][1])<< shift; > - pci_write_config_byte(dev, 0x44, slave_data); > } > > master_data |= 0x4000; /* Ensure SITRE is set */ > pci_write_config_word(dev, master_port, master_data); > > + if (is_slave) > + pci_write_config_byte(dev, 0x44, slave_data); > + > pci_read_config_byte(dev, 0x48,&udma_enable); > udma_enable&= ~(1<< (2 * ap->port_no + adev->devno)); > pci_write_config_byte(dev, 0x48, udma_enable); 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
Hi, On Thu, Feb 10, 2011 at 3:23 PM, Sergei Shtylyov <sshtylyov@mvista.com> wrote: > Hello. > > On 08-02-2011 15:24, Bartlomiej Zolnierkiewicz wrote: > [...] > >> We may need to set SITRE before programming slave_data. > >> This makes pata_efar match the behavior of IDE's slc90e66 host driver >> and also of libata's ata_piix one. > >> Signed-off-by: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com> > > [...] > >> diff --git a/drivers/ata/pata_efar.c b/drivers/ata/pata_efar.c >> index 1e2ff7d..7f564d7 100644 >> --- a/drivers/ata/pata_efar.c >> +++ b/drivers/ata/pata_efar.c >> @@ -74,10 +74,12 @@ static void efar_set_timings(struct ata_port *ap, >> struct ata_device *adev, >> u8 pio, bool use_mwdma) >> { >> struct pci_dev *dev = to_pci_dev(ap->host->dev); >> + unsigned int is_slave = (adev->devno != 0); > > What's the point of this variable? To save one pointer dereference? :-) Make code more similar to ata_piix.c and thus easier for comparisons through 'diff -ub'. In reality it doesn't matter now that much as pata_efar (same for pata_oldpiix) vanishes completely at the end of the patch series.. :-) Thanks, Bartlomiej -- 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. Bartlomiej Zolnierkiewicz wrote: >> On 08-02-2011 15:24, Bartlomiej Zolnierkiewicz wrote: >> [...] >>> We may need to set SITRE before programming slave_data. >>> This makes pata_efar match the behavior of IDE's slc90e66 host driver >>> and also of libata's ata_piix one. >>> Signed-off-by: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com> >> [...] >>> diff --git a/drivers/ata/pata_efar.c b/drivers/ata/pata_efar.c >>> index 1e2ff7d..7f564d7 100644 >>> --- a/drivers/ata/pata_efar.c >>> +++ b/drivers/ata/pata_efar.c >>> @@ -74,10 +74,12 @@ static void efar_set_timings(struct ata_port *ap, >>> struct ata_device *adev, >>> u8 pio, bool use_mwdma) >>> { >>> struct pci_dev *dev = to_pci_dev(ap->host->dev); >>> + unsigned int is_slave = (adev->devno != 0); >> What's the point of this variable? To save one pointer dereference? :-) > Make code more similar to ata_piix.c and thus easier for comparisons > through 'diff -ub'. > In reality it doesn't matter now that much as pata_efar (same for > pata_oldpiix) vanishes completely at the end of the patch series.. :-) Yeah, I understood that. Just don't have time to review the full series yet. > Thanks, > Bartlomiej 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
Hi, On Tue, Feb 8, 2011 at 2:38 PM, Sergei Shtylyov <sshtylyov@mvista.com> wrote: > Hello. > > On 08-02-2011 16:25, Alan Cox wrote: > >>>>> We may need to set SITRE before programming slave_data. > >>>> Do you have a documentation cite for this or is it random fiddling with >>>> a >>>> driver you can't test ? > >>> It is good to enable SITRE register before programming it and all >>> Intel controllers > >> That sounds like someone quoting religion. Documentation cite please. > > SLC90E66 datasheet only says that SIDETIM register has no effect without > SITRE bit set. Alan, is this explanation sufficient to ACK this patch? (Thanks Sergei for digging it out.) Jeff, would it be possible to queue patches #01-15 for 2.6.39 if there are no further concerns with them (thus leaving the merging of PIIX-like drivers for later)? They got additional testing on ICH4 and they look mostly safe & straight-forward compared to #16-21. Thanks, Bartlomiej -- 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
> > SLC90E66 datasheet only says that SIDETIM register has no effect without > > SITRE bit set. Which means the current setup is fine yes ? > Alan, is this explanation sufficient to ACK this patch? (Thanks > Sergei for digging it out.) > > Jeff, would it be possible to queue patches #01-15 for 2.6.39 Have they been tested on the relevant hardware yet - I don't believe the changes should be made for the untested stuff and you indicated you agreed with that. The tidyups for the easily tested cases (PIIX4 etc) look sensible -- 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 Sat, Feb 19, 2011 at 5:48 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: >> > SLC90E66 datasheet only says that SIDETIM register has no effect without >> > SITRE bit set. > > Which means the current setup is fine yes ? That's the problem, the current setup is buggy in slave-only setups: slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << shift; pci_write_config_byte(dev, 0x44, slave_data); } idetm_data |= 0x4000; /* Ensure SITRE is set */ pci_write_config_word(dev, idetm_port, idetm_data); idetm_port needs to be programmed _before_ slave_data to "ensure SITRE is set". IOW We currently depend on the BIOS here to set SITRE bit for us.. >> Alan, is this explanation sufficient to ACK this patch? (Thanks >> Sergei for digging it out.) >> >> Jeff, would it be possible to queue patches #01-15 for 2.6.39 > > Have they been tested on the relevant hardware yet - I don't believe the > changes should be made for the untested stuff and you indicated you > agreed with that. Unless they are obvious or risk is higher than benefit (bugfixes based on code review). I don't think that ATA code has became recently special in this regard compared to the rest of the kernel. > The tidyups for the easily tested cases (PIIX4 etc) look sensible Please take look at the changes #01-15 then: ata_piix: SITRE handling fix ata_piix: unify code for programming PIO and MWDMA timings pata_efar: fix register naming used in efar_set_piomode() pata_efar: unify code for programming PIO and MWDMA timings pata_efar: always program master_data before slave_data pata_it8213: fix register naming used in it8213_set_piomode() pata_it8213: unify code for programming PIO and MWDMA timings pata_it8213: add UDMA100 and UDMA133 support pata_oldpiix: unify code for programming PIO and MWDMA timings pata_radisys: unify code for programming PIO and MWDMA timings pata_rdc: unify code for programming PIO and MWDMA timings pata_rdc: parallel scanning needs an extra locking pata_rdc: add Power Management support pata_oldpiix: add locking for parallel scanning pata_oldpiix: enable parallel scan Most patches has been posted months ago in the past as the part of atang tree. They don't contain risky stuff (it was intentionally pushed at the end of the patch queue). We have here obvious register naming unification, PIO/DMA folding (it was tested with ata_piix and only later ported to other PIIX-like drivers) and: * para_rdc: locking bug-fix and Power Management support * pata_it8213: IDE -> libata regression fix (UDMA100/133 support based on vendor / old IDE driver) * pata_oldpiix: locking bug-fix (ACK-ed by you in the past) and parallel scanning support I see a lot of magnitude more risky stuff going elsewhere in the kernel, including ATA itself and if there are any concerns left those patches (#01-15) I'll be happy to address them. Thanks, Bartlomiej -- 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
> idetm_data |= 0x4000; /* Ensure SITRE is set */ > pci_write_config_word(dev, idetm_port, idetm_data); > > idetm_port needs to be programmed _before_ slave_data to "ensure SITRE is set". That isn't what the documentation seems to say. It says it has no effect unless SITRE is set not that you can't program the registers. > Unless they are obvious or risk is higher than benefit (bugfixes based > on code review). I don't think that ATA code has became recently > special in this regard compared to the rest of the kernel. They aren't special - random unneeded code changes that haven't been tested shouldn't be going into the code unless there is a big gain. For obscure ancient devices there isn't a gain. > ata_piix: unify code for programming PIO and MWDMA timings Which as I said makes sense > pata_efar: fix register naming used in efar_set_piomode() > pata_efar: unify code for programming PIO and MWDMA timings > pata_efar: always program master_data before slave_data All untested > pata_it8213: fix register naming used in it8213_set_piomode() > pata_it8213: unify code for programming PIO and MWDMA timings > pata_it8213: add UDMA100 and UDMA133 support All untested > pata_oldpiix: unify code for programming PIO and MWDMA timings Untested > pata_radisys: unify code for programming PIO and MWDMA timings Untested > pata_rdc: unify code for programming PIO and MWDMA timings > pata_rdc: parallel scanning needs an extra locking > pata_rdc: add Power Management support All untested but the locking is a clear bug fix and I think should go in > pata_oldpiix: add locking for parallel scanning > pata_oldpiix: enable parallel scan This is an ancient device on some old pentium class boxes, it's not worth adding this stuff really is it ? Well maybe putting the locking in or at least comments that it will be needed ? > Most patches has been posted months ago in the past as the part of > atang tree. So - where are the test reports > * pata_it8213: IDE -> libata regression fix (UDMA100/133 support based > on vendor / old IDE driver) I didn't see it tested in the old IDE driver either. > * pata_oldpiix: locking bug-fix (ACK-ed by you in the past) and > parallel scanning support I'm happy with the locking fix, I don't see the point in the parallel scan - and that wants testing because I don't know how the hardware will react. Most controllers were not designed for parallel scan and its a path windows will never have exercised therefore may well never have been tested out. In the PIIX4+ cases Jeff insisted we chased this down with the hardware folks in Intel to be sure it was ok. I'm not sure there is anyone who remembers original PIIX however. > I see a lot of magnitude more risky stuff going elsewhere in the > kernel, including ATA itself And being tested. efar/it8213/radisys are going to be tricky to find testers for as they are rare chips but the RDC is found in some of the embedded CPU/ATA/Net/etc in a device embedded x86 devices. 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
On 02/19/2011 04:25 AM, Bartlomiej Zolnierkiewicz wrote: > Jeff, would it be possible to queue patches #01-15 for 2.6.39 if there > are no further concerns with them (thus leaving the merging of > PIIX-like drivers for later)? They got additional testing on ICH4 and > they look mostly safe& straight-forward compared to #16-21. This seems to directly contradict what you wrote earlier in the thread, This is why patches were posted to mailing list with a request for a real hardware testing: "All testing was done using QEMU's PIIX3 controller emulation so any testing with real EFAR, IT8213, old PIIX, RDC and Radisys R82600 PATA controllers would be really appreciated.." instead of request for a merge. It was all there in initial mail. and I do not really care that much if it will be merged ever Regardless of this self-contradictory attitude, I do want useful patches and many of these patches seem useful. So I will continue watching the Bart/Alan/Sergei threads play out, and then look at merging the result. In the midst of all the arguing, productive work / forward progress is occurring, so the end result should be positive. It would be nice if we could get at least an "it works" test for the older hardware, since those are the changes /least/ likely to be tested by queueing to linux-next. Jeff -- 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
Hi, On Mon, Feb 21, 2011 at 9:06 PM, Jeff Garzik <jgarzik@pobox.com> wrote: > On 02/19/2011 04:25 AM, Bartlomiej Zolnierkiewicz wrote: >> >> Jeff, would it be possible to queue patches #01-15 for 2.6.39 if there >> are no further concerns with them (thus leaving the merging of >> PIIX-like drivers for later)? They got additional testing on ICH4 and >> they look mostly safe& straight-forward compared to #16-21. > > This seems to directly contradict what you wrote earlier in the thread, > > This is why patches were posted to mailing list with a request > for a real hardware testing: > > "All testing was done using QEMU's PIIX3 controller emulation > so any testing with real EFAR, IT8213, old PIIX, RDC and > Radisys R82600 PATA controllers would be really appreciated.." > > instead of request for a merge. It was all there in initial > mail. > > and > > I do not really care that much if it will be merged ever > > Regardless of this self-contradictory attitude, I do want useful patches and > many of these patches seem useful. Nothing self-contradictory there. :) First quote is about patches #01-15 only, not whole patchset (#01-20) like the second one, and I still don't care _that_ much personally if it gets merged since it is all unpaid & voluntary work. > So I will continue watching the Bart/Alan/Sergei threads play out, and then > look at merging the result. In the midst of all the arguing, productive > work / forward progress is occurring, so the end result should be positive. > > It would be nice if we could get at least an "it works" test for the older > hardware, since those are the changes /least/ likely to be tested by > queueing to linux-next. I was thinking about re-doing ata_piix part in a way that we could merge it now by adding support for older PIIX-alikes to ata_piix and making it enabled only if "all_piixalikes" module parameter is specified. This way older drivers would be left untouched for now and we can easily get in-tree testing for a new code. Does it sound as a viable alternative? Thanks, Bartlomiej -- 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 was thinking about re-doing ata_piix part in a way that we could > merge it now by adding support for older PIIX-alikes to ata_piix and > making it enabled only if "all_piixalikes" module parameter is > specified. This way older drivers would be left untouched for now > and we can easily get in-tree testing for a new code. Does it sound > as a viable alternative? That is the bit to me that makes the least sense. Each of the devices in question is found only in the chipset so they can never be in combination on a board (except multiple PIIX4 which is already covered) Merging them makes it more likely stuff breaks and increases memory usage - its a lose/lose situation. 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
On Tue, Feb 22, 2011 at 12:14 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: >> I was thinking about re-doing ata_piix part in a way that we could >> merge it now by adding support for older PIIX-alikes to ata_piix and >> making it enabled only if "all_piixalikes" module parameter is >> specified. This way older drivers would be left untouched for now >> and we can easily get in-tree testing for a new code. Does it sound >> as a viable alternative? > > That is the bit to me that makes the least sense. Each of the devices in > question is found only in the chipset so they can never be in combination > on a board (except multiple PIIX4 which is already covered) > > Merging them makes it more likely stuff breaks and increases memory usage > - its a lose/lose situation. Each device is very similar to other one, so keeping 6 drivers for the (almost) same stuff makes a little sense from the long-term maintainability POV, It is a question of different trade-offs since on the other side of equation we have 5 drivers less, 1400 LOCs less, no risk of losing bugfixes and features (which is true already w/ pata_rdc lacking locking fixes + Power Management and pata_oldpiix lacking parallel scanning feature). As for increased memory usage -- we are talking here only about 10-20k more. If it really is a problem maybe ata_piix can be redesigned into ata_generic-style manner so with the help of existing config options we can keep code size / memory usage on a existing level. Thanks, Bartlomiej -- 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 Wed, Feb 23, 2011 at 9:45 AM, Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote: > As for increased memory usage -- we are talking here only about 10-20k > more. If it really is a problem maybe ata_piix can be redesigned into > ata_generic-style manner so with the help of existing config options > we can keep code size / memory usage on a existing level. s/ata_generic/pata_legacy/ of course -- what I have in mind is making Intel specific code depended on ATA_PIIX and adding new config option ATA_PIIXALIKE for generic code which would be selected by existing PATA_[EFAR,IT8213, OLDPIIX,RADISYS,RDC] options. Thanks, Bartlomiej -- 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 the other side of equation we have 5 drivers less, 1400 LOCs less, > no risk of losing bugfixes and features (which is true already w/ > pata_rdc lacking locking fixes + Power Management and pata_oldpiix > lacking parallel scanning feature). The parallel scanning feature is somewhat irrelevant on an original PIIX, if the hardware even supports it which is somewhat of an unknown. If you have a minor boot time performance concern you'd buy a real computer instead. And the other stuff you actually create a problem rather than solving one - the difficulting testing the rare chips makes it harder to fix ata_piix bugs and do full testing. 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
diff --git a/drivers/ata/pata_efar.c b/drivers/ata/pata_efar.c index 1e2ff7d..7f564d7 100644 --- a/drivers/ata/pata_efar.c +++ b/drivers/ata/pata_efar.c @@ -74,10 +74,12 @@ static void efar_set_timings(struct ata_port *ap, struct ata_device *adev, u8 pio, bool use_mwdma) { struct pci_dev *dev = to_pci_dev(ap->host->dev); + unsigned int is_slave = (adev->devno != 0); unsigned long flags; u8 master_port = ap->port_no ? 0x42 : 0x40; u16 master_data; u8 udma_enable; + u8 slave_data; int control = 0; /* @@ -110,14 +112,13 @@ static void efar_set_timings(struct ata_port *ap, struct ata_device *adev, pci_read_config_word(dev, master_port, &master_data); /* Set PPE, IE, and TIME as appropriate */ - if (adev->devno == 0) { + if (is_slave == 0) { master_data &= 0xCCF0; master_data |= control; master_data |= (timings[pio][0] << 12) | (timings[pio][1] << 8); } else { int shift = 4 * ap->port_no; - u8 slave_data; master_data &= 0xFF0F; master_data |= (control << 4); @@ -126,12 +127,14 @@ static void efar_set_timings(struct ata_port *ap, struct ata_device *adev, pci_read_config_byte(dev, 0x44, &slave_data); slave_data &= ap->port_no ? 0x0F : 0xF0; slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << shift; - pci_write_config_byte(dev, 0x44, slave_data); } master_data |= 0x4000; /* Ensure SITRE is set */ pci_write_config_word(dev, master_port, master_data); + if (is_slave) + pci_write_config_byte(dev, 0x44, slave_data); + pci_read_config_byte(dev, 0x48, &udma_enable); udma_enable &= ~(1 << (2 * ap->port_no + adev->devno)); pci_write_config_byte(dev, 0x48, udma_enable);