RE: [PATCH 1/2 v1] pata_via.c: support VX855 and future chips whose IDE controller use 0x0571.

RE: [PATCH 1/2 v1] pata_via.c: support VX855 and future chips whose IDE controller use 0x0571.

[JosephChan]

> 
> > Also all the second port stuff is unneeded: Just do
> > 
> > 	if (has_one_port)
> > 		ppi[1] = &ata_dummy_port_info;
> > 
> > and only the first port will be probed.
> > 
> 
> We will check it out. Thanks. :)

How about this one? Thanks.

--- a/drivers/ata/pata_via.c	2008-12-25 07:26:37.000000000 +0800
+++ b/drivers/ata/pata_via.c	2009-01-17 03:34:55.000000000 +0800
@@ -63,6 +63,7 @@
 
 #define DRV_NAME "pata_via"
 #define DRV_VERSION "0.3.3"
+#define SINGLE	1 /* To identify single channel controllers */
 
 /*
  *	The following comes directly from Vojtech Pavlik's ide/pci/via82cxxx
@@ -97,6 +98,8 @@
 	u8 rev_max;
 	u16 flags;
 } via_isa_bridges[] = {
+	{ "vtxxxx",	PCI_DEVICE_ID_VIA_ANON,     0x00, 0x2f, VIA_UDMA_133 |
+	VIA_BAD_AST },
 	{ "vx800",	PCI_DEVICE_ID_VIA_VX800,    0x00, 0x2f, VIA_UDMA_133 |
 	VIA_BAD_AST | VIA_SATA_PATA },
 	{ "vt8237s",	PCI_DEVICE_ID_VIA_8237S,    0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST },
@@ -460,6 +463,7 @@
 	static int printed_version;
 	u8 enable;
 	u32 timing;
+	int single_port = (int) id->driver_data;
 	int rc;
 
 	if (!printed_version++)
@@ -469,6 +473,9 @@
 	if (rc)
 		return rc;
 
+	if (single_port)
+		ppi[1] = &ata_dummy_port_info;
+	
 	/* To find out how the IDE will behave and what features we
 	   actually have to look at the bridge not the IDE controller */
 	for (config = via_isa_bridges; config->id; config++)
@@ -483,10 +490,10 @@
 		}
 
 	if (!config->id) {
-		printk(KERN_WARNING "via: Unknown VIA SouthBridge, disabling.\n");
-		return -ENODEV;
-	}
-	pci_dev_put(isa);
+		printk(KERN_WARNING "via: Unknown VIA SouthBridge.\n");
+		config = via_isa_bridges;
+	} else
+		pci_dev_put(isa);
 
 	if (!(config->flags & VIA_NO_ENABLES)) {
 		/* 0x40 low bits indicate enabled channels */
@@ -587,6 +594,7 @@
 	{ PCI_VDEVICE(VIA, 0x1571), },
 	{ PCI_VDEVICE(VIA, 0x3164), },
 	{ PCI_VDEVICE(VIA, 0x5324), },
+	{ PCI_VDEVICE(VIA, 0xC409), SINGLE },
 
 	{ },
 };

Re: [PATCH 1/2 v1] pata_via.c: support VX855 and future chips whose IDE controller use 0x0571.

[Tejun Heo]

Hello, Joseph.

JosephChan@via.com.tw wrote:
> How about this one? Thanks.

Yeap, I like this one much better but I think this can be made a bit
prettier.

> --- a/drivers/ata/pata_via.c	2008-12-25 07:26:37.000000000 +0800
> +++ b/drivers/ata/pata_via.c	2009-01-17 03:34:55.000000000 +0800
> @@ -63,6 +63,7 @@
>  
>  #define DRV_NAME "pata_via"
>  #define DRV_VERSION "0.3.3"
> +#define SINGLE	1 /* To identify single channel controllers */

It's customary to prefix identifiers with something which is somewhat
unique, say, VIA_IDFLAG_SINGLE.  And if it's gonna be flags, the libata way
of doing it is...

enum {
     VIA_IDFLAG_SINGLE		= (1 << 0),
     VIA_IDFLAG_WHATEVER_NEXT	= (1 << 1),
};

and so on...

> @@ -460,6 +463,7 @@
>  	static int printed_version;
>  	u8 enable;
>  	u32 timing;
> +	int single_port = (int) id->driver_data;

How about unsigned long flags = id->driver_data; here

> +	if (single_port)
> +		ppi[1] = &ata_dummy_port_info;
> +	

and flags & VIA_IDFLAG_SINGLE here?

>  	if (!config->id) {
> -		printk(KERN_WARNING "via: Unknown VIA SouthBridge, disabling.\n");
> -		return -ENODEV;
> -	}
> -	pci_dev_put(isa);
> +		printk(KERN_WARNING "via: Unknown VIA SouthBridge.\n");
> +		config = via_isa_bridges;
> +	} else
> +		pci_dev_put(isa);

Also, you can just bypass whole southbridge thing.  Add another flag,
say, VIA_IDFLAG_IGN_SOUTH or something and just skip the whole thing
if the flags is set.

Thanks.

-- 
tejun

RE: [PATCH 1/2 v1] pata_via.c: support VX855 and future chips whose IDE controller use 0x0571.

[JosephChan]

> 
> Also, you can just bypass whole southbridge thing.  Add 
> another flag, say, VIA_IDFLAG_IGN_SOUTH or something and just 
> skip the whole thing if the flags is set.
> 

This is our next goal for this. :)

According to your suggestions, how about this one?

--- a/include/linux/pci_ids.h	2009-01-16 03:17:35.000000000 +0800
+++ b/include/linux/pci_ids.h	2009-01-20 02:21:54.000000000 +0800
@@ -1357,6 +1357,7 @@
 #define PCI_DEVICE_ID_VIA_8783_0	0x3208
 #define PCI_DEVICE_ID_VIA_8237		0x3227
 #define PCI_DEVICE_ID_VIA_8251		0x3287
+#define PCI_DEVICE_ID_VIA_8261		0x3402
 #define PCI_DEVICE_ID_VIA_8237A		0x3337
 #define PCI_DEVICE_ID_VIA_8237S		0x3372
 #define PCI_DEVICE_ID_VIA_SATA_EIDE	0x5324
@@ -1366,10 +1367,13 @@
 #define PCI_DEVICE_ID_VIA_CX700		0x8324
 #define PCI_DEVICE_ID_VIA_CX700_IDE	0x0581
 #define PCI_DEVICE_ID_VIA_VX800		0x8353
+#define PCI_DEVICE_ID_VIA_VX855		0x8409
 #define PCI_DEVICE_ID_VIA_8371_1	0x8391
 #define PCI_DEVICE_ID_VIA_82C598_1	0x8598
 #define PCI_DEVICE_ID_VIA_838X_1	0xB188
 #define PCI_DEVICE_ID_VIA_83_87XX_1	0xB198
+#define PCI_DEVICE_ID_VIA_C409_IDE	0XC409
+#define PCI_DEVICE_ID_VIA_ANON		0xFFFF
 
 #define PCI_VENDOR_ID_SIEMENS           0x110A
 #define PCI_DEVICE_ID_SIEMENS_DSCC4     0x2102
--- a/drivers/ata/pata_via.c	2008-12-25 07:26:37.000000000 +0800
+++ b/drivers/ata/pata_via.c	2009-01-20 02:59:05.000000000 +0800
@@ -86,6 +86,10 @@
 	VIA_SATA_PATA	= 0x800, /* SATA/PATA combined configuration */
 };
 
+enum {
+	VIA_IDFLAG_SINGLE = (1 << 0), /* single channel controller) */
+};
+
 /*
  * VIA SouthBridge chips.
  */
@@ -97,8 +101,14 @@
 	u8 rev_max;
 	u16 flags;
 } via_isa_bridges[] = {
+	{ "vtxxxx",	PCI_DEVICE_ID_VIA_ANON,    0x00, 0x2f, VIA_UDMA_133 |
+	VIA_BAD_AST },
+	{ "vx855",	PCI_DEVICE_ID_VIA_VX855,    0x00, 0x2f, VIA_UDMA_133 |
+	VIA_BAD_AST | VIA_SATA_PATA },
 	{ "vx800",	PCI_DEVICE_ID_VIA_VX800,    0x00, 0x2f, VIA_UDMA_133 |
 	VIA_BAD_AST | VIA_SATA_PATA },
+	{ "vt8261",	PCI_DEVICE_ID_VIA_8261,     0x00, 0x2f, VIA_UDMA_133 |
+	VIA_BAD_AST },
 	{ "vt8237s",	PCI_DEVICE_ID_VIA_8237S,    0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST },
 	{ "vt8251",	PCI_DEVICE_ID_VIA_8251,     0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST },
 	{ "cx700",	PCI_DEVICE_ID_VIA_CX700,    0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST | VIA_SATA_PATA },
@@ -460,6 +470,7 @@
 	static int printed_version;
 	u8 enable;
 	u32 timing;
+	unsigned long flags = id->driver_data;
 	int rc;
 
 	if (!printed_version++)
@@ -469,6 +480,9 @@
 	if (rc)
 		return rc;
 
+	if (flags & VIA_IDFLAG_SINGLE)
+		ppi[1] = &ata_dummy_port_info;
+	
 	/* To find out how the IDE will behave and what features we
 	   actually have to look at the bridge not the IDE controller */
 	for (config = via_isa_bridges; config->id; config++)
@@ -483,10 +497,10 @@
 		}
 
 	if (!config->id) {
-		printk(KERN_WARNING "via: Unknown VIA SouthBridge, disabling.\n");
-		return -ENODEV;
-	}
-	pci_dev_put(isa);
+		printk(KERN_WARNING "via: Unknown VIA SouthBridge.\n");
+		config = via_isa_bridges;
+	} else
+		pci_dev_put(isa);
 
 	if (!(config->flags & VIA_NO_ENABLES)) {
 		/* 0x40 low bits indicate enabled channels */
@@ -587,6 +601,7 @@
 	{ PCI_VDEVICE(VIA, 0x1571), },
 	{ PCI_VDEVICE(VIA, 0x3164), },
 	{ PCI_VDEVICE(VIA, 0x5324), },
+	{ PCI_VDEVICE(VIA, 0xC409), VIA_IDFLAG_SINGLE },
 
 	{ },
 };

Re: [PATCH 1/2 v1] pata_via.c: support VX855 and future chips whose IDE controller use 0x0571.

[Tejun Heo]

Hello, Joseph.

JosephChan@via.com.tw wrote:
>> Also, you can just bypass whole southbridge thing.  Add 
>> another flag, say, VIA_IDFLAG_IGN_SOUTH or something and just 
>> skip the whole thing if the flags is set.
>>
> 
> This is our next goal for this. :)

:-)

> @@ -97,8 +101,14 @@
>  	u8 rev_max;
>  	u16 flags;
>  } via_isa_bridges[] = {
> +	{ "vtxxxx",	PCI_DEVICE_ID_VIA_ANON,    0x00, 0x2f, VIA_UDMA_133 |
> +	VIA_BAD_AST },

More usual line breaking / indenting style would be...

	{ "vtxxxx",	PCI_DEVICE_ID_VIA_ANON,    0x00, 0x2f,
	  VIA_UDMA_133 | VIA_BAD_AST },

> @@ -460,6 +470,7 @@
>  	static int printed_version;
>  	u8 enable;
>  	u32 timing;
> +	unsigned long flags = id->driver_data;
>  	int rc;
>  
>  	if (!printed_version++)
> @@ -469,6 +480,9 @@
>  	if (rc)
>  		return rc;
>  
> +	if (flags & VIA_IDFLAG_SINGLE)
> +		ppi[1] = &ata_dummy_port_info;
> +	

Yay.

> @@ -483,10 +497,10 @@
>  		}
>  
>  	if (!config->id) {
> -		printk(KERN_WARNING "via: Unknown VIA SouthBridge, disabling.\n");
> -		return -ENODEV;
> -	}
> -	pci_dev_put(isa);
> +		printk(KERN_WARNING "via: Unknown VIA SouthBridge.\n");
> +		config = via_isa_bridges;

Heh.. a bit confusing.  Can you please put the wildcard entry at the
end of the list and make the loop condition

 for (config = via_isa_bridges; config->id != PCI_DEVICE_ID_VIA_ANON; config++)

That way, you won't need any special handling and just use "UNKNOWN"
as the name of the wildcard entry.

Thanks.

-- 
tejun

RE: [PATCH 1/2 v1] pata_via.c: support VX855 and future chips whose IDE controller use 0x0571.

[JosephChan]

>  for (config = via_isa_bridges; config->id != 
> PCI_DEVICE_ID_VIA_ANON; config++)
> 
> That way, you won't need any special handling and just use "UNKNOWN"
> as the name of the wildcard entry.
> 

How about this? :) 
I also found that if I modified to the following line.
for (config = via_isa_bridges; config->id != PCI_DEVICE_ID_VIA_ANON; config++)
I need to move the PCI_DEVICE_ID_VIA_ANON to the end of via_isa_bridges.
Or the for loop will exit at 1st check.


--- a/drivers/ata/pata_via.c	2009-01-23 03:25:50.000000000 +0800
+++ b/drivers/ata/pata_via.c	2009-01-23 03:25:11.000000000 +0800
@@ -86,6 +86,10 @@
 	VIA_SATA_PATA	= 0x800, /* SATA/PATA combined configuration */
 };
 
+enum {
+	VIA_IDFLAG_SINGLE = (1 << 0), /* single channel controller) */
+};
+
 /*
  * VIA SouthBridge chips.
  */
@@ -97,8 +101,12 @@
 	u8 rev_max;
 	u16 flags;
 } via_isa_bridges[] = {
+	{ "vx855",	PCI_DEVICE_ID_VIA_VX855,    0x00, 0x2f, 
+	  VIA_UDMA_133 | VIA_BAD_AST | VIA_SATA_PATA }, 
 	{ "vx800",	PCI_DEVICE_ID_VIA_VX800,    0x00, 0x2f, VIA_UDMA_133 |
 	VIA_BAD_AST | VIA_SATA_PATA },
+	{ "vt8261",	PCI_DEVICE_ID_VIA_8261,     0x00, 0x2f,
+	  VIA_UDMA_133 | VIA_BAD_AST },
 	{ "vt8237s",	PCI_DEVICE_ID_VIA_8237S,    0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST },
 	{ "vt8251",	PCI_DEVICE_ID_VIA_8251,     0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST },
 	{ "cx700",	PCI_DEVICE_ID_VIA_CX700,    0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST | VIA_SATA_PATA },
@@ -122,6 +130,8 @@
 	{ "vt82c586",	PCI_DEVICE_ID_VIA_82C586_0, 0x00, 0x0f, VIA_UDMA_NONE | VIA_SET_FIFO },
 	{ "vt82c576",	PCI_DEVICE_ID_VIA_82C576,   0x00, 0x2f, VIA_UDMA_NONE | VIA_SET_FIFO | VIA_NO_UNMASK },
 	{ "vt82c576",	PCI_DEVICE_ID_VIA_82C576,   0x00, 0x2f, VIA_UDMA_NONE | VIA_SET_FIFO | VIA_NO_UNMASK | VIA_BAD_ID },
+	{ "vtxxxx",	PCI_DEVICE_ID_VIA_ANON,    0x00, 0x2f,
+	  VIA_UDMA_133 | VIA_BAD_AST },
 	{ NULL }
 };
 
@@ -460,6 +470,7 @@
 	static int printed_version;
 	u8 enable;
 	u32 timing;
+	unsigned long flags = id->driver_data;
 	int rc;
 
 	if (!printed_version++)
@@ -469,9 +480,13 @@
 	if (rc)
 		return rc;
 
+	if (flags & VIA_IDFLAG_SINGLE)
+		ppi[1] = &ata_dummy_port_info;
+	
 	/* To find out how the IDE will behave and what features we
 	   actually have to look at the bridge not the IDE controller */
-	for (config = via_isa_bridges; config->id; config++)
+	for (config = via_isa_bridges; config->id != PCI_DEVICE_ID_VIA_ANON;
+	     config++)
 		if ((isa = pci_get_device(PCI_VENDOR_ID_VIA +
 			!!(config->flags & VIA_BAD_ID),
 			config->id, NULL))) {
@@ -482,10 +497,6 @@
 			pci_dev_put(isa);
 		}
 
-	if (!config->id) {
-		printk(KERN_WARNING "via: Unknown VIA SouthBridge, disabling.\n");
-		return -ENODEV;
-	}
 	pci_dev_put(isa);
 
 	if (!(config->flags & VIA_NO_ENABLES)) {
@@ -587,6 +598,7 @@
 	{ PCI_VDEVICE(VIA, 0x1571), },
 	{ PCI_VDEVICE(VIA, 0x3164), },
 	{ PCI_VDEVICE(VIA, 0x5324), },
+	{ PCI_VDEVICE(VIA, 0xC409), VIA_IDFLAG_SINGLE },
 
 	{ },
 };

Re: [PATCH 1/2 v1] pata_via.c: support VX855 and future chips whose IDE controller use 0x0571.

[Tejun Heo]

JosephChan@via.com.tw wrote:
>>  for (config = via_isa_bridges; config->id != 
>> PCI_DEVICE_ID_VIA_ANON; config++)
>>
>> That way, you won't need any special handling and just use "UNKNOWN"
>> as the name of the wildcard entry.
>>
> 
> How about this? :) 
> I also found that if I modified to the following line.
> for (config = via_isa_bridges; config->id != PCI_DEVICE_ID_VIA_ANON; config++)
> I need to move the PCI_DEVICE_ID_VIA_ANON to the end of via_isa_bridges.
> Or the for loop will exit at 1st check.

Looks good to me.  :-)

Thanks.

-- 
tejun

RE: [PATCH 1/2 v1] pata_via.c: support VX855 and future chips whose IDE controller use 0x0571.

[JosephChan]

> > config++) I need to move the PCI_DEVICE_ID_VIA_ANON to the 
> end of via_isa_bridges.
> > Or the for loop will exit at 1st check.
> 
> Looks good to me.  :-)
> 

Thanks, I will resummit it later.
Happy Chinese New Year.

Re: [PATCH 1/2 v1] pata_via.c: support VX855 and future chips whose IDE controller use 0x0571.

[Tejun Heo]

JosephChan@via.com.tw wrote:
> Thanks, I will resummit it later.
> Happy Chinese New Year.

Heh... we call it lunar New Year here as we aren't exactly Chinese.
Anyways, happy new year.  :-)

-- 
tejun

[PATCH 1/2 v1] pata_via.c: support VX855 and future chips whose IDE controller use 0x0571.

[JosephChan]

This patch supports VX855 and future chips whose IDE controller use 0x0571.


Signed-off-by: Joseph Chan <josephchan@via.com.tw>

--- a/include/linux/pci_ids.h	2009-01-09 23:28:18.000000000 +0800
+++ b/include/linux/pci_ids.h	2009-01-10 00:35:15.000000000 +0800
@@ -1370,6 +1370,7 @@
 #define PCI_DEVICE_ID_VIA_82C598_1	0x8598
 #define PCI_DEVICE_ID_VIA_838X_1	0xB188
 #define PCI_DEVICE_ID_VIA_83_87XX_1	0xB198
+#define PCI_DEVICE_ID_VIA_ANON		0xFFFF
 
 #define PCI_VENDOR_ID_SIEMENS           0x110A
 #define PCI_DEVICE_ID_SIEMENS_DSCC4     0x2102
--- a/drivers/ata/pata_via.c	2008-12-25 07:26:37.000000000 +0800
+++ a/drivers/ata/pata_via.c	2009-01-16 02:36:58.000000000 +0800
@@ -97,6 +97,8 @@
 	u8 rev_max;
 	u16 flags;
 } via_isa_bridges[] = {
+	{ "vtxxxx",	PCI_DEVICE_ID_VIA_ANON,     0x00, 0x2f, VIA_UDMA_133 |
+	VIA_BAD_AST },
 	{ "vx800",	PCI_DEVICE_ID_VIA_VX800,    0x00, 0x2f, VIA_UDMA_133 |
 	VIA_BAD_AST | VIA_SATA_PATA },
 	{ "vt8237s",	PCI_DEVICE_ID_VIA_8237S,    0x00, 0x2f, VIA_UDMA_133 | VIA_BAD_AST },
@@ -176,6 +178,16 @@
 	if ((config->flags & VIA_SATA_PATA) && ap->port_no == 0)
 		return ATA_CBL_SATA;
 
+	if (pdev->device == 0xC409) {
+		if (ap->port_no == 0) {
+			pci_read_config_dword(pdev, 0x52, &ata66);
+			return (ata66 & 0x10) ? ATA_CBL_PATA80 : ATA_CBL_PATA40;
+		} else if (ap->port_no == 1) {
+			DPRINTK("C409 only has one pata channel\n");
+			return ATA_CBL_PATA_UNK;
+		}
+	}
+
 	/* Early chips are 40 wire */
 	if ((config->flags & VIA_UDMA) < VIA_UDMA_66)
 		return ATA_CBL_PATA40;
@@ -483,10 +495,10 @@
 		}
 
 	if (!config->id) {
-		printk(KERN_WARNING "via: Unknown VIA SouthBridge, disabling.\n");
-		return -ENODEV;
-	}
-	pci_dev_put(isa);
+		printk(KERN_WARNING "via: Unknown VIA SouthBridge.\n");
+		config = via_isa_bridges;
+	} else
+		pci_dev_put(isa);
 
 	if (!(config->flags & VIA_NO_ENABLES)) {
 		/* 0x40 low bits indicate enabled channels */
@@ -587,6 +599,7 @@
 	{ PCI_VDEVICE(VIA, 0x1571), },
 	{ PCI_VDEVICE(VIA, 0x3164), },
 	{ PCI_VDEVICE(VIA, 0x5324), },
+	{ PCI_VDEVICE(VIA, 0xC409), },
 
 	{ },
 };

Re: [PATCH 1/2 v1] pata_via.c: support VX855 and future chips whose IDE controller use 0x0571.

[Alan Cox]

> This patch supports VX855 and future chips whose IDE controller use 0x0571.

The patch adds entries for 0xFFFF (is that really right) as a bridge and
0xC409 - I see nothing for 0x0571.

Also all the second port stuff is unneeded: Just do

	if (has_one_port)
		ppi[1] = &ata_dummy_port_info;

and only the first port will be probed.

RE: [PATCH 1/2 v1] pata_via.c: support VX855 and future chips whose IDE controller use 0x0571.

[JosephChan]

> 
> The patch adds entries for 0xFFFF (is that really right) as a 
> bridge and
> 0xC409 - I see nothing for 0x0571.

PCI_DEVICE_ID_VIA_ANON is used to stand for Unknown South Bridge for VX855
and future chips, since the pata_via has a southbridge check, which is redundant. 
If we may VIA has a new SB with new bridge ID which in not in the check list, whose 
IDE controller is 0x0571. Linux default pata_via can not drive IDE controller of 402, since
the southbridge check in pata_via can not pass. 
So we add PCI_DEVICE_ID_VIA_ANON to stand for our future southbridge,
and this will help to pass southbridge check.
Is it possible to do this way? And can it be acceptable in kernel policy?

> Also all the second port stuff is unneeded: Just do
> 
> 	if (has_one_port)
> 		ppi[1] = &ata_dummy_port_info;
> 
> and only the first port will be probed.
> 

We will check it out. Thanks. :)

Re: [PATCH 1/2 v1] pata_via.c: support VX855 and future chips whose IDE controller use 0x0571.

[Sergei Shtylyov]

Hello.

JosephChan@via.com.tw wrote:

>> The patch adds entries for 0xFFFF (is that really right) as a 
>> bridge and
>> 0xC409 - I see nothing for 0x0571.
>>     
>
> PCI_DEVICE_ID_VIA_ANON is used to stand for Unknown South Bridge for VX855
> and future chips, since the pata_via has a southbridge check, which is redundant.

   If it was redundant, it wouldn't have been there...

> If we may VIA has a new SB with new bridge ID which in not in the check list, whose 
> IDE controller is 0x0571. Linux default pata_via can not drive IDE controller of 402, since
> the southbridge check in pata_via can not pass. 
>   
> So we add PCI_DEVICE_ID_VIA_ANON to stand for our future southbridge,
> and this will help to pass southbridge check.
>   

   Why not just add the new and future SB device IDs to the list?

> Is it possible to do this way? And can it be acceptable in kernel policy?
>   

   I doubt it...

MBR, Sergei