| Message ID | 20110208122346.19110.8441.sendpatchset@linux-mhg7.site |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On 08.02.2011 15:23, Bartlomiej Zolnierkiewicz wrote: > From ca6c1fbc7d853829524b87a3bcbbb1f67796281c Mon Sep 17 00:00:00 2001 > From: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com> > Date: Tue, 8 Feb 2011 12:39:24 +0100 > Subject: [PATCH 02/20] ata_piix: unify code for programming PIO and MWDMA timings > Besides making things noticably simpler it results in ~2% decrease in > the driver LOC count and also ~2% decrease in the driver binary size > (as measured on x86-32). > Fix piix_set_piomode() documentation while at it. > Signed-off-by: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com> [...] > diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c > index 0f4856d..c954f91 100644 > --- a/drivers/ata/ata_piix.c > +++ b/drivers/ata/ata_piix.c > @@ -696,22 +696,11 @@ static int piix_pata_prereset(struct ata_link *link, unsigned long deadline) > > static DEFINE_SPINLOCK(piix_lock); > > -/** > - * piix_set_piomode - Initialize host controller PATA PIO timings > - * @ap: Port whose timings we are configuring > - * @adev: um > - * > - * Set PIO mode for device, in host controller PCI config space. > - * > - * LOCKING: > - * None (inherited from caller). > - */ > - > -static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev) > +static void piix_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 long flags; > - unsigned int pio = adev->pio_mode - XFER_PIO_0; > unsigned int is_slave = (adev->devno != 0); > unsigned int master_port= ap->port_no ? 0x42 : 0x40; > unsigned int slave_port = 0x44; > @@ -732,14 +721,18 @@ static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev) > { 2, 1 }, > { 2, 3 }, }; > > - if (pio >= 2) > + if (pio >= 2 || use_mwdma) The 'use_mwdma' check is actually superfluous... > control |= 1; /* TIME1 enable */ > - if (ata_pio_need_iordy(adev)) > + if (ata_pio_need_iordy(adev) || use_mwdma) > control |= 2; /* IE enable */ Why IORDY is enabled for MWDMA has always been beyond me... I understand that the stupid Intel docs are to be blamed here. > - > /* Intel specifies that the PPE functionality is for disk only */ > if (adev->class == ATA_DEV_ATA) > control |= 4; /* PPE enable */ > + /* If the drive MWDMA is faster than it can do PIO then > + we must force PIO into PIO0 */ It's not the preferred multi-line comment style: /* * bla * bla */ > + if (use_mwdma && adev->pio_mode < (XFER_PIO_0 + pio)) Parens not needed around +. > @@ -803,31 +812,20 @@ static void do_pata_set_dmamode(struct ata_port *ap, struct ata_device *adev, in > { > struct pci_dev *dev = to_pci_dev(ap->host->dev); > unsigned long flags; > - u8 master_port = ap->port_no ? 0x42 : 0x40; > - u16 master_data; > u8 speed = adev->dma_mode; > int devid = adev->devno + 2 * ap->port_no; > u8 udma_enable = 0; > > - static const /* ISP RTC */ > - u8 timings[][2] = { { 0, 0 }, > - { 0, 0 }, > - { 1, 0 }, > - { 2, 1 }, > - { 2, 3 }, }; > - > - spin_lock_irqsave(&piix_lock, flags); > - > - pci_read_config_word(dev, master_port, &master_data); > - if (ap->udma_mask) > - pci_read_config_byte(dev, 0x48, &udma_enable); I don't think you should have removed that... > @@ -860,56 +858,20 @@ static void do_pata_set_dmamode(struct ata_port *ap, struct ata_device *adev, in > performance (WR_PingPong_En) */ > pci_write_config_word(dev, 0x54, ideconf); > } > + > + pci_write_config_byte(dev, 0x48, udma_enable); > + > + spin_unlock_irqrestore(&piix_lock, flags); > } else { [...] > - if (ap->udma_mask) > - udma_enable &= ~(1 << devid); This does not seem correct -- you must disable UDMA when programming MWDMA. > - > - pci_write_config_word(dev, master_port, master_data); > + piix_set_timings(ap, adev, pio, 1); > } > - /* Don't scribble on 0x48 if the controller does not support UDMA */ > - if (ap->udma_mask) > - pci_write_config_byte(dev, 0x48, udma_enable); > - > - spin_unlock_irqrestore(&piix_lock, flags); > } > > /** 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
> > + if (ata_pio_need_iordy(adev) || use_mwdma) > > control |= 2; /* IE enable */ > > Why IORDY is enabled for MWDMA has always been beyond me... I understand > that the stupid Intel docs are to be blamed here. I fail to see whats stupid about the docs ? The same timing register set is used for MWDMA and PIO cycles in MWDMA tuned modes (eg ATAPI). Thus the controller needs to be in an MWDMA mode whose timings are compatible with the PIO timing and we want IORDY in use for the PIO transfer parts. 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. On 20-02-2011 21:59, Alan Cox wrote: >>> + if (ata_pio_need_iordy(adev) || use_mwdma) >>> control |= 2; /* IE enable */ >> Why IORDY is enabled for MWDMA has always been beyond me... I understand >> that the stupid Intel docs are to be blamed here. > I fail to see whats stupid about the docs ? Association of IORDY with DMA mode is wrong. > The same timing register set is used for MWDMA and PIO cycles in MWDMA > tuned modes (eg ATAPI). Thus the controller needs to be in an MWDMA mode > whose timings are compatible with the PIO timing and we want IORDY in use > for the PIO transfer parts. Yeah, especially if we also set the bit which only enables fast timing for DMA. ;-) PIO mode is setup by different code, and it takes care of the IORDY setting according to the PIO rules (and it gets called). DMA mode setup should just ignore the IORDY setting as in all other sane drivers. > 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
> PIO mode is setup by different code, and it takes care of the IORDY > setting according to the PIO rules (and it gets called). DMA mode setup should > just ignore the IORDY setting as in all other sane drivers. Well it can't ignore it - but if you mean just keep the bit as is then that sounds sensible, have to see what the docs say happens if you ever set MWDMA without IORDY. -- 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 21-02-2011 0:07, Alan Cox wrote: >> PIO mode is setup by different code, and it takes care of the IORDY >> setting according to the PIO rules (and it gets called). DMA mode setup should >> just ignore the IORDY setting as in all other sane drivers. > Well it can't ignore it - but if you mean just keep the bit as is then Yes, I mean this. > that sounds sensible, have to see what the docs say happens if you ever > set MWDMA without IORDY. Don't think they say anything on this matter but I can't imagine that IORDY matters for DMA. 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. On 21-02-2011 14:38, Sergei Shtylyov wrote: >>> PIO mode is setup by different code, and it takes care of the IORDY >>> setting according to the PIO rules (and it gets called). DMA mode setup should >>> just ignore the IORDY setting as in all other sane drivers. >> Well it can't ignore it - but if you mean just keep the bit as is then > Yes, I mean this. >> that sounds sensible, have to see what the docs say happens if you ever >> set MWDMA without IORDY. > Don't think they say anything on this matter but I can't imagine that IORDY > matters for DMA. What it says is that IORDY enable bit has no effect when the fast timing bit is cleared. That's from ICH IDE PRM (29860004.pdf). 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 Mon, 21 Feb 2011 14:38:37 +0300 Sergei Shtylyov <sshtylyov@mvista.com> wrote: > Hello. > > On 21-02-2011 0:07, Alan Cox wrote: > > >> PIO mode is setup by different code, and it takes care of the IORDY > >> setting according to the PIO rules (and it gets called). DMA mode setup should > >> just ignore the IORDY setting as in all other sane drivers. > > > Well it can't ignore it - but if you mean just keep the bit as is then > > Yes, I mean this. > > > that sounds sensible, have to see what the docs say happens if you ever > > set MWDMA without IORDY. > > Don't think they say anything on this matter but I can't imagine that > IORDY matters for DMA. Sure - but it does matter for an ATAPI transaction with then has a DMA phase because those timings will be used for the command transfer which is PIO. 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. On 21-02-2011 14:58, Alan Cox wrote: >>>> PIO mode is setup by different code, and it takes care of the IORDY >>>> setting according to the PIO rules (and it gets called). DMA mode setup should >>>> just ignore the IORDY setting as in all other sane drivers. >>> Well it can't ignore it - but if you mean just keep the bit as is then >> Yes, I mean this. >>> that sounds sensible, have to see what the docs say happens if you ever >>> set MWDMA without IORDY. >> Don't think they say anything on this matter but I can't imagine that >> IORDY matters for DMA. ... unless the IE bit means something special (and undocumented) for DMA, if Intel so insists on setting it for every DMA mode. > Sure - but it does matter for an ATAPI transaction with then has a DMA > phase because those timings will be used for the command transfer which > is PIO. So what? We've already setup PIO timings before the DMA ones. Why force the IE bit? > 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
> > Don't think they say anything on this matter but I can't imagine that IORDY > > matters for DMA. > > What it says is that IORDY enable bit has no effect when the fast timing > bit is cleared. That's from ICH IDE PRM (29860004.pdf). Ok thats the detail I missed - cool. -- 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/ata_piix.c b/drivers/ata/ata_piix.c index 0f4856d..c954f91 100644 --- a/drivers/ata/ata_piix.c +++ b/drivers/ata/ata_piix.c @@ -696,22 +696,11 @@ static int piix_pata_prereset(struct ata_link *link, unsigned long deadline) static DEFINE_SPINLOCK(piix_lock); -/** - * piix_set_piomode - Initialize host controller PATA PIO timings - * @ap: Port whose timings we are configuring - * @adev: um - * - * Set PIO mode for device, in host controller PCI config space. - * - * LOCKING: - * None (inherited from caller). - */ - -static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev) +static void piix_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 long flags; - unsigned int pio = adev->pio_mode - XFER_PIO_0; unsigned int is_slave = (adev->devno != 0); unsigned int master_port= ap->port_no ? 0x42 : 0x40; unsigned int slave_port = 0x44; @@ -732,14 +721,18 @@ static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev) { 2, 1 }, { 2, 3 }, }; - if (pio >= 2) + if (pio >= 2 || use_mwdma) control |= 1; /* TIME1 enable */ - if (ata_pio_need_iordy(adev)) + if (ata_pio_need_iordy(adev) || use_mwdma) control |= 2; /* IE enable */ - /* Intel specifies that the PPE functionality is for disk only */ if (adev->class == ATA_DEV_ATA) control |= 4; /* PPE enable */ + /* If the drive MWDMA is faster than it can do PIO then + we must force PIO into PIO0 */ + if (use_mwdma && adev->pio_mode < (XFER_PIO_0 + pio)) + /* Enable DMA timing only */ + control |= 8; /* PIO cycles in PIO0 */ spin_lock_irqsave(&piix_lock, flags); @@ -788,6 +781,22 @@ static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev) } /** + * piix_set_piomode - Initialize host controller PATA PIO timings + * @ap: Port whose timings we are configuring + * @adev: Drive in question + * + * Set PIO mode for device, in host controller PCI config space. + * + * LOCKING: + * None (inherited from caller). + */ + +static void piix_set_piomode(struct ata_port *ap, struct ata_device *adev) +{ + piix_set_timings(ap, adev, adev->pio_mode - XFER_PIO_0, 0); +} + +/** * do_pata_set_dmamode - Initialize host controller PATA PIO timings * @ap: Port whose timings we are configuring * @adev: Drive in question @@ -803,31 +812,20 @@ static void do_pata_set_dmamode(struct ata_port *ap, struct ata_device *adev, in { struct pci_dev *dev = to_pci_dev(ap->host->dev); unsigned long flags; - u8 master_port = ap->port_no ? 0x42 : 0x40; - u16 master_data; u8 speed = adev->dma_mode; int devid = adev->devno + 2 * ap->port_no; u8 udma_enable = 0; - static const /* ISP RTC */ - u8 timings[][2] = { { 0, 0 }, - { 0, 0 }, - { 1, 0 }, - { 2, 1 }, - { 2, 3 }, }; - - spin_lock_irqsave(&piix_lock, flags); - - pci_read_config_word(dev, master_port, &master_data); - if (ap->udma_mask) - pci_read_config_byte(dev, 0x48, &udma_enable); - if (speed >= XFER_UDMA_0) { - unsigned int udma = adev->dma_mode - XFER_UDMA_0; + unsigned int udma = speed - XFER_UDMA_0; u16 udma_timing; u16 ideconf; int u_clock, u_speed; + spin_lock_irqsave(&piix_lock, flags); + + pci_read_config_byte(dev, 0x48, &udma_enable); + /* * UDMA is handled by a combination of clock switching and * selection of dividers @@ -860,56 +858,20 @@ static void do_pata_set_dmamode(struct ata_port *ap, struct ata_device *adev, in performance (WR_PingPong_En) */ pci_write_config_word(dev, 0x54, ideconf); } + + pci_write_config_byte(dev, 0x48, udma_enable); + + spin_unlock_irqrestore(&piix_lock, flags); } else { - /* - * MWDMA is driven by the PIO timings. We must also enable - * IORDY unconditionally along with TIME1. PPE has already - * been set when the PIO timing was set. - */ - unsigned int mwdma = adev->dma_mode - XFER_MW_DMA_0; - unsigned int control; - u8 slave_data; + /* MWDMA is driven by the PIO timings. */ + unsigned int mwdma = speed - XFER_MW_DMA_0; const unsigned int needed_pio[3] = { XFER_PIO_0, XFER_PIO_3, XFER_PIO_4 }; int pio = needed_pio[mwdma] - XFER_PIO_0; - control = 3; /* IORDY|TIME1 */ - - /* If the drive MWDMA is faster than it can do PIO then - we must force PIO into PIO0 */ - - if (adev->pio_mode < needed_pio[mwdma]) - /* Enable DMA timing only */ - control |= 8; /* PIO cycles in PIO0 */ - - if (adev->devno) { /* Slave */ - master_data &= 0xFF4F; /* Mask out IORDY|TIME1|DMAONLY */ - master_data |= control << 4; - pci_read_config_byte(dev, 0x44, &slave_data); - slave_data &= (ap->port_no ? 0x0f : 0xf0); - /* Load the matching timing */ - slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << (ap->port_no ? 4 : 0); - pci_write_config_byte(dev, 0x44, slave_data); - } else { /* Master */ - master_data &= 0xCCF4; /* Mask out IORDY|TIME1|DMAONLY - and master timing bits */ - master_data |= control; - master_data |= - (timings[pio][0] << 12) | - (timings[pio][1] << 8); - } - - if (ap->udma_mask) - udma_enable &= ~(1 << devid); - - pci_write_config_word(dev, master_port, master_data); + piix_set_timings(ap, adev, pio, 1); } - /* Don't scribble on 0x48 if the controller does not support UDMA */ - if (ap->udma_mask) - pci_write_config_byte(dev, 0x48, udma_enable); - - spin_unlock_irqrestore(&piix_lock, flags); } /**