[PATCH 2.6.22-rc2] libata: sata_sis fixes

[PATCH 2.6.22-rc2] libata: sata_sis fixes

[Uwe Koziolek]

The sata_sis driver supports SATA and PATA ports. The broken support
of both types in one controller is fixed.

the pata133 sis controllers does not support a drive present status
in the pci configspace like the older sis controllers, check removed.

Signed-off-by: Uwe Koziolek <uwe.koziolek@gmx.net>

--- a/drivers/ata/sata_sis.c	2007-05-22 11:05:38.000000000 +0200
+++ b/drivers/ata/sata_sis.c	2007-05-23 00:24:28.000000000 +0200
@@ -255,7 +255,7 @@
 {
 	static int printed_version;
 	struct ata_port_info pi = sis_port_info;
-	const struct ata_port_info *ppi[] = { &pi, NULL };
+	const struct ata_port_info *ppi[] = { &pi, &pi };
 	struct ata_host *host;
 	u32 genctl, val;
 	u8 pmr;
--- a/drivers/ata/pata_sis.c	2007-05-22 11:05:38.000000000 +0200
+++ b/drivers/ata/pata_sis.c	2007-05-23 23:10:33.000000000 +0200
@@ -154,6 +154,19 @@
 
 
 /**
+ *	sis_133_error_handler - Probe specified port on PATA host controller
+ *	@ap: Port to probe
+ *
+ *	LOCKING:
+ *	None (inherited from caller).
+ */
+
+static void sis_133_error_handler(struct ata_port *ap)
+{
+	ata_bmdma_drive_eh(ap, ata_std_prereset, ata_std_softreset, NULL, ata_std_postreset);
+}
+
+/**
  *	sis_error_handler - Probe specified port on PATA host controller
  *	@ap: Port to probe
  *
@@ -540,7 +553,7 @@
 
 	.freeze			= ata_bmdma_freeze,
 	.thaw			= ata_bmdma_thaw,
-	.error_handler		= sis_error_handler,
+	.error_handler		= sis_133_error_handler,
 	.post_internal_cmd	= ata_bmdma_post_internal_cmd,
 	.cable_detect		= sis_133_cable_detect,
 

Re: [PATCH 2.6.22-rc2] libata: sata_sis fixes

[Alan Cox]

> the pata133 sis controllers does not support a drive present status
> in the pci configspace like the older sis controllers, check removed.

Are you sure about this - I've no idea about the SATA/PATA combo but the
pure PATA 133Mhz (MuTOL 96x) chipsets seem to do so correctly.
 
> Signed-off-by: Uwe Koziolek <uwe.koziolek@gmx.net>

> +static void sis_133_error_handler(struct ata_port *ap)
> +{
> +	ata_bmdma_drive_eh(ap, ata_std_prereset, ata_std_softreset, NULL, ata_std_postreset);
> +}

NAK - if this is needed then you can simply set .error_handler to use the
default bmdma handler rather than adding the function

Alan

Re: [PATCH 2.6.22-rc2] libata: sata_sis fixes

[Jeff Garzik]

Uwe Koziolek wrote:
> The sata_sis driver supports SATA and PATA ports. The broken support
> of both types in one controller is fixed.
> 
> the pata133 sis controllers does not support a drive present status
> in the pci configspace like the older sis controllers, check removed.
> 
> Signed-off-by: Uwe Koziolek <uwe.koziolek@gmx.net>

ditto Alan's comments:  your implementation is identical to 
ata_bmdma_error_handler(), so you can just use that.

otherwise, seems OK...

	Jeff

[PATCH 2.6.22-rc2] libata: sata_sis fixes

[Uwe Koziolek]

The sata_sis driver supports SATA and PATA ports. The broken support
of both types in one controller is fixed.

the PATA-port of SiS180 controller does not support a drive present status
in the pci configspace like the other SiS PATA controllers, check skipped.

Signed-off-by: Uwe Koziolek <uwe.koziolek@gmx.net>

--- a/drivers/ata/sata_sis.c	2007-05-22 11:05:38.000000000 +0200
+++ b/drivers/ata/sata_sis.c	2007-05-23 00:24:28.000000000 +0200
@@ -255,7 +255,7 @@
 {
 	static int printed_version;
 	struct ata_port_info pi = sis_port_info;
-	const struct ata_port_info *ppi[] = { &pi, NULL };
+	const struct ata_port_info *ppi[] = { &pi, &pi };
 	struct ata_host *host;
 	u32 genctl, val;
 	u8 pmr;
--- a/drivers/ata/pata_sis.c	2007-05-22 11:05:38.000000000 +0200
+++ b/drivers/ata/pata_sis.c	2007-05-25 07:50:50.000000000 +0200
@@ -146,7 +146,8 @@
 
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
 
-	if (!pci_test_config_bits(pdev, &sis_enable_bits[ap->port_no]))
+	if ((pdev->device != 0x0180) && (pdev->device != 0x0181) &&
+	    !pci_test_config_bits(pdev, &sis_enable_bits[ap->port_no]))
 		return -ENOENT;
 
 	return ata_std_prereset(ap, deadline);

Re: [PATCH 2.6.22-rc2] libata: sata_sis fixes

[Jeff Garzik]

Uwe Koziolek wrote:
> --- a/drivers/ata/sata_sis.c	2007-05-22 11:05:38.000000000 +0200
> +++ b/drivers/ata/sata_sis.c	2007-05-23 00:24:28.000000000 +0200
> @@ -255,7 +255,7 @@
>  {
>  	static int printed_version;
>  	struct ata_port_info pi = sis_port_info;
> -	const struct ata_port_info *ppi[] = { &pi, NULL };
> +	const struct ata_port_info *ppi[] = { &pi, &pi };
>  	struct ata_host *host;
>  	u32 genctl, val;
>  	u8 pmr;

applied this part


> --- a/drivers/ata/pata_sis.c	2007-05-22 11:05:38.000000000 +0200
> +++ b/drivers/ata/pata_sis.c	2007-05-25 07:50:50.000000000 +0200
> @@ -146,7 +146,8 @@
>  
>  	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
>  
> -	if (!pci_test_config_bits(pdev, &sis_enable_bits[ap->port_no]))
> +	if ((pdev->device != 0x0180) && (pdev->device != 0x0181) &&
> +	    !pci_test_config_bits(pdev, &sis_enable_bits[ap->port_no]))
>  		return -ENOENT;
>  
>  	return ata_std_prereset(ap, deadline);

I think you misunderstood what Alan and I were saying.

If you remove the enable-bits check, then logically, all the function 
does is call ata_std_prereset.  Thus, your error handler only needs to 
the standard function for 0x180 and 0x181, ata_std_prereset() rather 
than sis_pre_reset().

Further, once your ata_bmdma_drive_eh() has been reduced entirely to 
calling standard functions, you need not use sis_error_handler() at all, 
because _that_ has been reduced to ata_bmdma_error_handler().

As a result, the following line

	.error_handler		= ata_bmdma_error_handler,

is functionally equivalent to your patch, but without custom code to 
produce that effect.

	Jeff

Re: [PATCH 2.6.22-rc2] libata: sata_sis fixes

[Alan Cox]

On Fri, 25 May 2007 09:48:52 +0200
Uwe Koziolek <uwe.koziolek@gmx.net> wrote:

> The sata_sis driver supports SATA and PATA ports. The broken support
> of both types in one controller is fixed.
> 
> the PATA-port of SiS180 controller does not support a drive present status
> in the pci configspace like the other SiS PATA controllers, check skipped.
> 
> Signed-off-by: Uwe Koziolek <uwe.koziolek@gmx.net>

Needs checking with SiS because they submitted code that uses those
enable bits and its been in drivers/ide for years with respect of the
MuTOL ATA133. No argument about the SATA one if you've checked the docs
and seen the bug.

> -	if (!pci_test_config_bits(pdev, &sis_enable_bits[ap->port_no]))
> +	if ((pdev->device != 0x0180) && (pdev->device != 0x0181) &&
> +	    !pci_test_config_bits(pdev, &sis_enable_bits[ap->port_no]))
>  		return -ENOENT;

Might look a lot nicer with less brackets, or even better pull the device
check out into a new static function (gcc will inline it all nicely
anyway) so you can just say

	if (sis_enables_supported(pdev) && !pci_test...

Re: [PATCH 2.6.22-rc2] libata: sata_sis fixes

[Uwe Koziolek]

>   
>> The sata_sis driver supports SATA and PATA ports. The broken support
>> of both types in one controller is fixed.
>>
>> the PATA-port of SiS180 controller does not support a drive present status
>> in the pci configspace like the other SiS PATA controllers, check skipped.
>>
>> Signed-off-by: Uwe Koziolek <uwe.koziolek@gmx.net>
>>     
>
> Needs checking with SiS because they submitted code that uses those
> enable bits and its been in drivers/ide for years with respect of the
> MuTOL ATA133. No argument about the SATA one if you've checked the docs
> and seen the bug.
>
>   
How should I proceed. I can submit both patches:
- exclude SiS180 from enable bit checking or
- exclude MuTol ATA133 from enable bit checking with different
error_handler.

Uwe

Re: [PATCH 2.6.22-rc2] libata: sata_sis fixes

[Alan Cox]

> How should I proceed. I can submit both patches:
> - exclude SiS180 from enable bit checking or
> - exclude MuTol ATA133 from enable bit checking with different
> error_handler.

Been away but - SiS180 wants excluding anyway according to all the docs,
the MuToL does appear to be working correctly with enable bits and we've
got no failure cases in old/new IDE so leave it alone (for now anyway)

Re: [PATCH 2.6.22-rc2] libata: sata_sis fixes

[Uwe Koziolek]

How should I proceed. I can submit both patches:
>> - exclude SiS180 from enable bit checking or
>> - exclude MuTol ATA133 from enable bit checking with different
>> error_handler.
>>     
>
> Been away but - SiS180 wants excluding anyway according to all the docs,
> the MuToL does appear to be working correctly with enable bits and we've
> got no failure cases in old/new IDE so leave it alone (for now anyway)
>   
I have checked some SiS 5513 config spaces with ATA133 for the enable
bits. In all cases Alans
code would work. But I dont know why the necesary bits are set. May be
the hardware is different
to the documentation or the BIOS is setting these bits for compatibility
issue.

The change of the error handler for SiS180 is really needed. The change
of the errorhandler for
Pre ATA133 chips is not reasonable.  The change of the  errorhandler 
for  SiS5513  ATA133
controller is not mandatory.

My suggestion: exclude only the SiS180 from the enable bits.

But i want to have an ok , or a vote for the other variant.

Uwe