Patchwork [2/5] pata_sis: Implement MWDMA for the UDMA 133 capable chips

login
register
mail settings
Submitter Alan Cox
Date Nov. 17, 2009, 2:51 p.m.
Message ID <20091117145111.15430.67106.stgit@localhost.localdomain>
Download mbox | patch
Permalink /patch/38642/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Alan Cox - Nov. 17, 2009, 2:51 p.m.
Bartlomiej pointed out that while this got fixed in the old driver whoever
did it didn't port it across.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/ata/pata_sis.c |   91 ++++++++++++++++++++++++++++++++++++------------
 1 files changed, 69 insertions(+), 22 deletions(-)



--
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
Bartlomiej Zolnierkiewicz - Nov. 17, 2009, 5:27 p.m.
On Tuesday 17 November 2009 15:51:17 Alan Cox wrote:
> Bartlomiej pointed out that while this got fixed in the old driver whoever
> did it didn't port it across.

Please add ',' before 'whoever' and change 'the old driver' to 'sis5513'
so it is at least passable as 'information manipulation' on what I really
said instead of just being a 'straight lie'..  Thanks!

> +/**
> + *	mwdma_clip_to_pio		-	clip MWDMA mode
> + *	@adev: device
> + *
> + *	As the SiS shared MWDMA and PIO timings we must program the equivalent
> + *	PIO timing for the MWDMA mode but we must not program one higher than
> + *	the permitted PIO timing of the device.
> + */
> +
> +static int mwdma_clip_to_pio(struct ata_device *adev)
> +{
> +	const int mwdma_to_pio[3] = {
> +		XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
> +	};
> +	return min(mwdma_to_pio[adev->dma_mode - XFER_MW_DMA_0],
> +				adev->pio_mode - XFER_PIO_0);
> +}

This wants to be in the generic libata code.
Alan Cox - Nov. 17, 2009, 5:38 p.m.
> > +static int mwdma_clip_to_pio(struct ata_device *adev)
> > +{
> > +	const int mwdma_to_pio[3] = {
> > +		XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
> > +	};
> > +	return min(mwdma_to_pio[adev->dma_mode - XFER_MW_DMA_0],
> > +				adev->pio_mode - XFER_PIO_0);
> > +}
> 
> This wants to be in the generic libata code.

I'm not convinced because for the majority of drivers the libata timing
interface handles it. SiS needs it just because it does things by
precomputed tables. It's a one off interface.

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
Bartlomiej Zolnierkiewicz - Nov. 17, 2009, 5:57 p.m.
On Tuesday 17 November 2009 18:38:23 Alan Cox wrote:
> > > +static int mwdma_clip_to_pio(struct ata_device *adev)
> > > +{
> > > +	const int mwdma_to_pio[3] = {
> > > +		XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
> > > +	};
> > > +	return min(mwdma_to_pio[adev->dma_mode - XFER_MW_DMA_0],
> > > +				adev->pio_mode - XFER_PIO_0);
> > > +}
> > 
> > This wants to be in the generic libata code.
> 
> I'm not convinced because for the majority of drivers the libata timing
> interface handles it. SiS needs it just because it does things by
> precomputed tables. It's a one off interface.

Controllers based on *Intel* PIIX are in the disagreement with the above
paragraph and having generic helper to do conversion (without a clipping)
would bring us a little step closer to killing a needless code duplication
currently present in their ->set_piomode and ->set_dmamode methods.

[ In case somebody wonders: no, 'old' drivers don't have such duplication
  and 'whoever did it didn't port it across' cause said 'whoever'-s were not
  into the development of the square wheels.... ]
Alan Cox - Nov. 17, 2009, 6:21 p.m.
> > I'm not convinced because for the majority of drivers the libata timing
> > interface handles it. SiS needs it just because it does things by
> > precomputed tables. It's a one off interface.
> 
> Controllers based on *Intel* PIIX are in the disagreement with the above

No the PIIX is quite different. You use the matching PIO timing (which is
a short lookup and shorter code than even a helper function call). You do
not use the clipping instead you set bit 3 to ensure that PIO cycles
occur at low speed but MWDMA runs at the right speed. That is usually a
win over picking a lower mode as the PIIX can do ATAPI DMA happily.

So the only thing you can "share" is what would be a 4 byte table.

> paragraph and having generic helper to do conversion (without a clipping)
> would bring us a little step closer to killing a needless code duplication
> currently present in their ->set_piomode and ->set_dmamode methods.
> 
> [ In case somebody wonders: no, 'old' drivers don't have such duplication
>   and 'whoever did it didn't port it across' cause said 'whoever'-s were not
>   into the development of the square wheels.... ]

There is no duplication in the old drivers because they don't implement
the needed check and clipping/bit setting that I can see ;)

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
Bartlomiej Zolnierkiewicz - Nov. 17, 2009, 7:33 p.m.
On Tuesday 17 November 2009 19:21:31 Alan Cox wrote:
> > > I'm not convinced because for the majority of drivers the libata timing
> > > interface handles it. SiS needs it just because it does things by
> > > precomputed tables. It's a one off interface.
> > 
> > Controllers based on *Intel* PIIX are in the disagreement with the above
> 
> No the PIIX is quite different. You use the matching PIO timing (which is
> a short lookup and shorter code than even a helper function call). You do
> not use the clipping instead you set bit 3 to ensure that PIO cycles
> occur at low speed but MWDMA runs at the right speed. That is usually a
> win over picking a lower mode as the PIIX can do ATAPI DMA happily.

Thank you for the detailed explanation, I certainly would still be
scratching my head over it if it wasn't for your great help.

> So the only thing you can "share" is what would be a 4 byte table.

You are of course right, I must have been confused by the old driver.

> > paragraph and having generic helper to do conversion (without a clipping)
> > would bring us a little step closer to killing a needless code duplication
> > currently present in their ->set_piomode and ->set_dmamode methods.
> > 
> > [ In case somebody wonders: no, 'old' drivers don't have such duplication
> >   and 'whoever did it didn't port it across' cause said 'whoever'-s were not
> >   into the development of the square wheels.... ]
> 
> There is no duplication in the old drivers because they don't implement
> the needed check and clipping/bit setting that I can see ;)

Seems like the whoever maintained the old driver didn't port across
this critical bugfix. :(

Patch

diff --git a/drivers/ata/pata_sis.c b/drivers/ata/pata_sis.c
index 488e77b..d70ecec 100644
--- a/drivers/ata/pata_sis.c
+++ b/drivers/ata/pata_sis.c
@@ -252,24 +252,25 @@  static void sis_100_set_piomode (struct ata_port *ap, struct ata_device *adev)
 }
 
 /**
- *	sis_133_set_piomode - Initialize host controller PATA PIO timings
+ *	sis_133_do_piomode - Initialize host controller PATA PIO/DMA timings
  *	@ap: Port whose timings we are configuring
  *	@adev: Device we are configuring for.
  *
  *	Set PIO mode for device, in host controller PCI config space. This
- *	function handles PIO set up for the later ATA133 devices.
+ *	function handles PIO set up for the later ATA133 devices. The same
+ *	timings are used for MWDMA.
  *
  *	LOCKING:
  *	None (inherited from caller).
  */
 
-static void sis_133_set_piomode (struct ata_port *ap, struct ata_device *adev)
+static void sis_133_do_piomode(struct ata_port *ap, struct ata_device *adev,
+					int speed)
 {
 	struct pci_dev *pdev	= to_pci_dev(ap->host->dev);
 	int port = 0x40;
 	u32 t1;
 	u32 reg54;
-	int speed = adev->pio_mode - XFER_PIO_0;
 
 	const u32 timing133[] = {
 		0x28269000,	/* Recovery << 24 | Act << 16 | Ini << 12 */
@@ -305,6 +306,42 @@  static void sis_133_set_piomode (struct ata_port *ap, struct ata_device *adev)
 }
 
 /**
+ *	sis_133_set_piomode - Initialize host controller PATA PIO timings
+ *	@ap: Port whose timings we are configuring
+ *	@adev: Device we are configuring for.
+ *
+ *	Set PIO mode for device, in host controller PCI config space. This
+ *	function handles PIO set up for the later ATA133 devices.
+ *
+ *	LOCKING:
+ *	None (inherited from caller).
+ */
+
+static void sis_133_set_piomode (struct ata_port *ap, struct ata_device *adev)
+{
+
+	sis_133_do_piomode(ap, adev, adev->pio_mode - XFER_PIO_0);
+}
+
+/**
+ *	mwdma_clip_to_pio		-	clip MWDMA mode
+ *	@adev: device
+ *
+ *	As the SiS shared MWDMA and PIO timings we must program the equivalent
+ *	PIO timing for the MWDMA mode but we must not program one higher than
+ *	the permitted PIO timing of the device.
+ */
+
+static int mwdma_clip_to_pio(struct ata_device *adev)
+{
+	const int mwdma_to_pio[3] = {
+		XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
+	};
+	return min(mwdma_to_pio[adev->dma_mode - XFER_MW_DMA_0],
+				adev->pio_mode - XFER_PIO_0);
+}
+
+/**
  *	sis_old_set_dmamode - Initialize host controller PATA DMA timings
  *	@ap: Port whose timings we are configuring
  *	@adev: Device to program
@@ -332,6 +369,7 @@  static void sis_old_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 	if (adev->dma_mode < XFER_UDMA_0) {
 		/* bits 3-0 hold recovery timing bits 8-10 active timing and
 		   the higher bits are dependant on the device */
+		speed = mwdma_clip_to_pio(adev);
 		timing &= ~0x870F;
 		timing |= mwdma_bits[speed];
 	} else {
@@ -372,6 +410,7 @@  static void sis_66_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 	if (adev->dma_mode < XFER_UDMA_0) {
 		/* bits 3-0 hold recovery timing bits 8-10 active timing and
 		   the higher bits are dependant on the device, bit 15 udma */
+		speed = mwdma_clip_to_pio(adev);
 		timing &= ~0x870F;
 		timing |= mwdma_bits[speed];
 	} else {
@@ -389,7 +428,7 @@  static void sis_66_set_dmamode (struct ata_port *ap, struct ata_device *adev)
  *	@adev: Device to program
  *
  *	Set UDMA/MWDMA mode for device, in host controller PCI config space.
- *	Handles UDMA66 and early UDMA100 devices.
+ *	Handles later UDMA100 devices.
  *
  *	LOCKING:
  *	None (inherited from caller).
@@ -400,21 +439,25 @@  static void sis_100_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 	struct pci_dev *pdev	= to_pci_dev(ap->host->dev);
 	int speed = adev->dma_mode - XFER_MW_DMA_0;
 	int drive_pci = sis_old_port_base(adev);
-	u8 timing;
+	u16 timing;
 
-	const u8 udma_bits[]  = { 0x8B, 0x87, 0x85, 0x83, 0x82, 0x81};
+	const u16 udma_bits[]  = {
+		0x8B00, 0x8700, 0x8500, 0x8300, 0x8200, 0x8100};
+	const u8 mwdma_bits[] = { 0x08, 0x32, 0x31 };
 
-	pci_read_config_byte(pdev, drive_pci + 1, &timing);
+	pci_read_config_word(pdev, drive_pci, &timing);
 
 	if (adev->dma_mode < XFER_UDMA_0) {
-		/* NOT SUPPORTED YET: NEED DATA SHEET. DITTO IN OLD DRIVER */
+		speed = mwdma_clip_to_pio(adev);
+		timing &= ~0x80FF;
+		timing |= mwdma_bits[speed];
 	} else {
 		/* Bit 7 is UDMA on/off, bit 0-3 are cycle time */
 		speed = adev->dma_mode - XFER_UDMA_0;
-		timing &= ~0x8F;
+		timing &= ~0x8F00;
 		timing |= udma_bits[speed];
 	}
-	pci_write_config_byte(pdev, drive_pci + 1, timing);
+	pci_write_config_word(pdev, drive_pci, timing);
 }
 
 /**
@@ -434,21 +477,26 @@  static void sis_133_early_set_dmamode (struct ata_port *ap, struct ata_device *a
 	struct pci_dev *pdev	= to_pci_dev(ap->host->dev);
 	int speed = adev->dma_mode - XFER_MW_DMA_0;
 	int drive_pci = sis_old_port_base(adev);
-	u8 timing;
-	/* Low 4 bits are timing */
-	static const u8 udma_bits[]  = { 0x8F, 0x8A, 0x87, 0x85, 0x83, 0x82, 0x81};
+	u16 timing;
+	/* Bits 15-12  are timing */
+	static const u16 udma_bits[]  = {
+		0x8F00, 0x8A00, 0x8700, 0x8500, 0x8300, 0x8200, 0x8100
+	};
+	static const u8 mwdma_bits[] = { 0x08, 0x32, 0x31 };
 
-	pci_read_config_byte(pdev, drive_pci + 1, &timing);
+	pci_read_config_word(pdev, drive_pci, &timing);
 
 	if (adev->dma_mode < XFER_UDMA_0) {
-		/* NOT SUPPORTED YET: NEED DATA SHEET. DITTO IN OLD DRIVER */
+		speed = mwdma_clip_to_pio(adev);
+		timing &= ~0x80FF;
+		timing = mwdma_bits[speed];
 	} else {
 		/* Bit 7 is UDMA on/off, bit 0-3 are cycle time */
 		speed = adev->dma_mode - XFER_UDMA_0;
-		timing &= ~0x8F;
+		timing &= ~0x8F00;
 		timing |= udma_bits[speed];
 	}
-	pci_write_config_byte(pdev, drive_pci + 1, timing);
+	pci_write_config_word(pdev, drive_pci, timing);
 }
 
 /**
@@ -479,13 +527,12 @@  static void sis_133_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 	if (reg54 & 0x40000000)
 		port = 0x70;
 	port += (8 * ap->port_no) +  (4 * adev->devno);
-
 	pci_read_config_dword(pdev, port, &t1);
 
 	if (adev->dma_mode < XFER_UDMA_0) {
-		t1 &= ~0x00000004;
-		/* FIXME: need data sheet to add MWDMA here. Also lacking on
-		   ide/pci driver */
+		speed = mwdma_clip_to_pio(adev);
+		sis_133_do_piomode(ap, adev, speed);
+		t1 &= ~4;	/* UDMA off */
 	} else {
 		speed = adev->dma_mode - XFER_UDMA_0;
 		/* if & 8 no UDMA133 - need info for ... */