[PATCH 1/2] pata_sis: extract a sis_port_base() method

[PATCH 1/2] pata_sis: extract a sis_port_base() method

[Dan McGee]

This is similar to the existing sis_old_port_base() method. We do this
same calculation and logic in multiple places (with one more to come in
a future patch), so extracting it into a method makes sense.

Reviewed-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---
 drivers/ata/pata_sis.c |   46 ++++++++++++++++++++++++++++------------------
 1 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/ata/pata_sis.c b/drivers/ata/pata_sis.c
index 533f2ae..9374400 100644
--- a/drivers/ata/pata_sis.c
+++ b/drivers/ata/pata_sis.c
@@ -89,6 +89,29 @@ static int sis_old_port_base(struct ata_device *adev)
 }
 
 /**
+ *	sis_port_base		-	return PCI configuration base for dev
+ *	@adev: device
+ *
+ *	Returns the base of the PCI configuration registers for this port
+ *	number.
+ */
+
+static int sis_port_base(struct ata_device *adev)
+{
+	struct ata_port *ap = adev->link->ap;
+	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	int port = 0x40;
+	u32 reg54;
+
+	/* If bit 14 is set then the registers are mapped at 0x70 not 0x40 */
+	pci_read_config_dword(pdev, 0x54, &reg54);
+	if (reg54 & 0x40000000)
+		port = 0x70;
+
+	return  port + (8 * ap->port_no) +  (4 * adev->devno);
+}
+
+/**
  *	sis_133_cable_detect	-	check for 40/80 pin
  *	@ap: Port
  *	@deadline: deadline jiffies for the operation
@@ -266,9 +289,8 @@ static void sis_100_set_piomode (struct ata_port *ap, struct ata_device *adev)
 static void sis_133_set_piomode (struct ata_port *ap, struct ata_device *adev)
 {
 	struct pci_dev *pdev	= to_pci_dev(ap->host->dev);
-	int port = 0x40;
+	int port;
 	u32 t1;
-	u32 reg54;
 	int speed = adev->pio_mode - XFER_PIO_0;
 
 	const u32 timing133[] = {
@@ -288,12 +310,7 @@ static void sis_133_set_piomode (struct ata_port *ap, struct ata_device *adev)
 
 	sis_set_fifo(ap, adev);
 
-	/* If bit 14 is set then the registers are mapped at 0x70 not 0x40 */
-	pci_read_config_dword(pdev, 0x54, &reg54);
-	if (reg54 & 0x40000000)
-		port = 0x70;
-	port += 8 * ap->port_no +  4 * adev->devno;
-
+	port = sis_port_base(adev);
 	pci_read_config_dword(pdev, port, &t1);
 	t1 &= 0xC0C00FFF;	/* Mask out timing */
 
@@ -465,21 +482,14 @@ static void sis_133_early_set_dmamode (struct ata_port *ap, struct ata_device *a
 static void sis_133_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 port = 0x40;
+	int port;
 	u32 t1;
-	u32 reg54;
 
 	/* bits 4- cycle time 8 - cvs time */
 	static const u32 timing_u100[] = { 0x6B0, 0x470, 0x350, 0x140, 0x120, 0x110, 0x000 };
 	static const u32 timing_u133[] = { 0x9F0, 0x6A0, 0x470, 0x250, 0x230, 0x220, 0x210 };
 
-	/* If bit 14 is set then the registers are mapped at 0x70 not 0x40 */
-	pci_read_config_dword(pdev, 0x54, &reg54);
-	if (reg54 & 0x40000000)
-		port = 0x70;
-	port += (8 * ap->port_no) +  (4 * adev->devno);
-
+	port = sis_port_base(adev);
 	pci_read_config_dword(pdev, port, &t1);
 
 	if (adev->dma_mode < XFER_UDMA_0) {
@@ -487,7 +497,7 @@ static void sis_133_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 		/* FIXME: need data sheet to add MWDMA here. Also lacking on
 		   ide/pci driver */
 	} else {
-		speed = adev->dma_mode - XFER_UDMA_0;
+		int speed = adev->dma_mode - XFER_UDMA_0;
 		/* if & 8 no UDMA133 - need info for ... */
 		t1 &= ~0x00000FF0;
 		t1 |= 0x00000004;
-- 
1.7.6.1

[PATCH 2/2] pata_sis: add mode_filter method for certain sis5513 chipsets

[Dan McGee]

This mirrors a very old commit (3160d5416f39da9d9) to the old sis5513
IDE driver that prevents certain setups from working. UDMA6, aka
ATA/133, is not supported on some chipsets and we need to ensure this
mode is not chosen even if a connected drive supports it. Port this old
patch forward to the new PATA driver to ensure UDMA5 is the highest mode
used if that is what is supported.

Kernel bugzilla #41582.

Reviewed-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---
 drivers/ata/pata_sis.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/pata_sis.c b/drivers/ata/pata_sis.c
index 9374400..54c41c7 100644
--- a/drivers/ata/pata_sis.c
+++ b/drivers/ata/pata_sis.c
@@ -509,6 +509,27 @@ static void sis_133_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 	pci_write_config_dword(pdev, port, t1);
 }
 
+/**
+ *	sis_133_mode_filter - mode selection filter
+ *	@adev: ATA device
+ *
+ *	Block UDMA6 on devices that do not support it.
+ */
+
+static unsigned long sis_133_mode_filter(struct ata_device *adev, unsigned long mask)
+{
+	struct ata_port *ap = adev->link->ap;
+	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	int port = sis_port_base(adev);
+	u32 t1;
+
+	pci_read_config_dword(pdev, port, &t1);
+	/* if ATA133 is disabled, mask it out */
+	if (!(t1 & 0x08))
+		mask &= ~(0xE0 << ATA_SHIFT_UDMA);
+	return mask;
+}
+
 static struct scsi_host_template sis_sht = {
 	ATA_BMDMA_SHT(DRV_NAME),
 };
@@ -530,6 +551,7 @@ static struct ata_port_operations sis_133_ops = {
 	.set_piomode		= sis_133_set_piomode,
 	.set_dmamode		= sis_133_set_dmamode,
 	.cable_detect		= sis_133_cable_detect,
+	.mode_filter		= sis_133_mode_filter,
 };
 
 static struct ata_port_operations sis_133_early_ops = {
@@ -779,10 +801,13 @@ static int sis_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
 
 		switch(trueid) {
 		case 0x5518:	/* SIS 962/963 */
+			printk(KERN_INFO DRV_NAME " %s: SiS 962/963 MuTIOL IDE UDMA133 controller\n",
+				pci_name(pdev));
 			chipset = &sis133;
 			if ((idemisc & 0x40000000) == 0) {
 				pci_write_config_dword(pdev, 0x54, idemisc | 0x40000000);
-				printk(KERN_INFO "SIS5513: Switching to 5513 register mapping\n");
+				printk(KERN_INFO DRV_NAME " %s: Switching to 5513 register mapping\n",
+					pci_name(pdev));
 			}
 			break;
 		case 0x0180:	/* SIS 965/965L */
-- 
1.7.6.1

Re: [PATCH 2/2] pata_sis: add mode_filter method for certain sis5513 chipsets

[Jeff Garzik]

On 09/06/2011 03:21 PM, Dan McGee wrote:
> This mirrors a very old commit (3160d5416f39da9d9) to the old sis5513
> IDE driver that prevents certain setups from working. UDMA6, aka
> ATA/133, is not supported on some chipsets and we need to ensure this
> mode is not chosen even if a connected drive supports it. Port this old
> patch forward to the new PATA driver to ensure UDMA5 is the highest mode
> used if that is what is supported.
>
> Kernel bugzilla #41582.
>
> Reviewed-by: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com>
> Signed-off-by: Dan McGee<dpmcgee@gmail.com>
> ---
>   drivers/ata/pata_sis.c |   27 ++++++++++++++++++++++++++-
>   1 files changed, 26 insertions(+), 1 deletions(-)


Queued, thanks...

[PATCH] Ensure we actually allow UDMA/100

[Dan McGee]

We were limiting to UDMA/66 instead.
---
Jeff,

Can this get folded in? Just realized the bitmask I used from Alan's patch wasn't right as it didn't actually allow UDMA/100, it was setting the max at UDMA/66. This simple fixup has been boot tested.

-Dan

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

diff --git a/drivers/ata/pata_sis.c b/drivers/ata/pata_sis.c
index 54c41c7..810b7d6 100644
--- a/drivers/ata/pata_sis.c
+++ b/drivers/ata/pata_sis.c
@@ -526,7 +526,7 @@ static unsigned long sis_133_mode_filter(struct ata_device *adev, unsigned long
 	pci_read_config_dword(pdev, port, &t1);
 	/* if ATA133 is disabled, mask it out */
 	if (!(t1 & 0x08))
-		mask &= ~(0xE0 << ATA_SHIFT_UDMA);
+		mask &= ~(0xC0 << ATA_SHIFT_UDMA);
 	return mask;
 }
 
-- 
1.7.6.1

Re: [PATCH 1/2] pata_sis: extract a sis_port_base() method

[Dan McGee]

On Wed, Sep 7, 2011 at 4:58 AM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
> On 06-09-2011 23:21, Dan McGee wrote:
>
>> This is similar to the existing sis_old_port_base() method. We do this
>> same calculation and logic in multiple places (with one more to come in
>> a future patch), so extracting it into a method makes sense.
>
>> Reviewed-by: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com>
>> Signed-off-by: Dan McGee<dpmcgee@gmail.com>
>> ---
>>  drivers/ata/pata_sis.c |   46
>> ++++++++++++++++++++++++++++------------------
>>  1 files changed, 28 insertions(+), 18 deletions(-)
>
>> diff --git a/drivers/ata/pata_sis.c b/drivers/ata/pata_sis.c
>> index 533f2ae..9374400 100644
>> --- a/drivers/ata/pata_sis.c
>> +++ b/drivers/ata/pata_sis.c
>> @@ -89,6 +89,29 @@ static int sis_old_port_base(struct ata_device *adev)
>> +       /* If bit 14 is set then the registers are mapped at 0x70 not 0x40
>> */
>> +       pci_read_config_dword(pdev, 0x54,&reg54);
>> +       if (reg54 & 0x40000000)
>
>   This is bit 30, not bit 14.
You've snipped the rest of the patch where I simply cut this comment
from the three other places it was stated. I'm simply moving code and
text around here. I will update the apparently erroneous comment.

>
>> +               port = 0x70;
>> +
>> +       return  port + (8 * ap->port_no) +  (4 * adev->devno);
>
>   Why two spaces at some places?
It is from the odd style of the method right above it. This driver has
a lot of odd styling. I'll "fix" it.

>
> [...]
>>
>> @@ -465,21 +482,14 @@ static void sis_133_early_set_dmamode (struct
>> ata_port *ap, struct ata_device *a
>>  static void sis_133_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;
>
>   Unrelated change.
In context this change makes perfect sense- I'm cleaning up the rest
of the locals after extracting the method, and this one is out of
place and a totally dead assignment.

>
> [...]
>>
>> @@ -487,7 +497,7 @@ static void sis_133_set_dmamode (struct ata_port *ap,
>> struct ata_device *adev)
>>                /* FIXME: need data sheet to add MWDMA here. Also lacking
>> on
>>                   ide/pci driver */
>>        } else {
>> -               speed = adev->dma_mode - XFER_UDMA_0;
>> +               int speed = adev->dma_mode - XFER_UDMA_0;
>
>   Same here.
>
> WBR, Sergei