ata_piix: add new PIIX_FLAG_* flags

PATCH Bartlomiej Zolnierkiewicz 2 participants 7 messages Feb 8, 2011 – Feb 11, 2011 libata

lore

Share & tools

lore

Bartlomiej Zolnierkiewicz

author

2011-02-08 15:53 UTC #0 lore raw

Turn open-coded checks in piix_set_timings() into PIIX_FLAG_* flags per suggestion from Alan Cox.

Suggested-by Alan Cox <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
On top of https://lkml.org/lkml/2011/2/8/97 patchset.

 drivers/ata/ata_piix.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

drivers/ata/ata_piix.c +7 -6

Index: b/drivers/ata/ata_piix.c
===================================================================
@@ -133,6 +133,8 @@ enum {
 
 	PIIX_FLAG_CHECKINTR	= (1 << 28), /* make sure PCI INTx enabled */
 	PIIX_FLAG_SIDPR		= (1 << 29), /* SATA idx/data pair regs */
+	PIIX_FLAG_NO_SITRE	= (1 << 30), /* no SITRE register */
+	PIIX_FLAG_RADISYS	= (1 << 31), /* host is Radisys R82600 */
 
 	PIIX_PATA_FLAGS		= ATA_FLAG_SLAVE_POSS,
 	PIIX_SATA_FLAGS		= ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR,

@@ -698,7 +700,7 @@ static struct ata_port_info piix_port_in
 
 	[oldpiix_pata] =
 	{
-		.flags		= PIIX_PATA_FLAGS,
+		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_NO_SITRE,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA12_ONLY,
 		.port_ops	= &oldpiix_pata_ops,

@@ -706,7 +708,8 @@ static struct ata_port_info piix_port_in
 
 	[radisys_pata] =
 	{
-		.flags		= PIIX_PATA_FLAGS,
+		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_NO_SITRE |
+				  PIIX_FLAG_RADISYS,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA12_ONLY,
 		.udma_mask	= ATA_UDMA24_ONLY,

@@ -859,10 +862,8 @@ static void piix_set_timings(struct ata_
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
 	unsigned long flags;
 	unsigned int is_slave	= (adev->devno != 0);
-	unsigned int is_radisys	= (dev->vendor == PCI_VENDOR_ID_RADISYS &&
-				   dev->device == 0x8201);
-	unsigned int has_sitre	= (dev->vendor != PCI_VENDOR_ID_INTEL ||
-				   dev->device != 0x1230) && !is_radisys;
+	unsigned int is_radisys	= (ap->flags & PIIX_FLAG_RADISYS) ? 1 : 0;
+	unsigned int has_sitre	= (ap->flags & PIIX_FLAG_NO_SITRE) ? 1 : 0;
 	unsigned int master_port= ap->port_no ? 0x42 : 0x40;
 	unsigned int slave_port	= 0x44;
 	u16 master_data;

Bartlomiej Zolnierkiewicz

author

2011-02-08 16:03 UTC #1 lore raw

Turn open-coded checks in piix_set_timings() into PIIX_FLAG_* flags per suggestion from Alan Cox.

Suggested-by Alan Cox <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
- On top of https://lkml.org/lkml/2011/2/8/97 patchset.
- Get the reverse logic correct this time.

 drivers/ata/ata_piix.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

drivers/ata/ata_piix.c +7 -6

Index: b/drivers/ata/ata_piix.c
===================================================================
@@ -133,6 +133,8 @@ enum {
 
 	PIIX_FLAG_CHECKINTR	= (1 << 28), /* make sure PCI INTx enabled */
 	PIIX_FLAG_SIDPR		= (1 << 29), /* SATA idx/data pair regs */
+	PIIX_FLAG_NO_SITRE	= (1 << 30), /* no SITRE register */
+	PIIX_FLAG_RADISYS	= (1 << 31), /* host is Radisys R82600 */
 
 	PIIX_PATA_FLAGS		= ATA_FLAG_SLAVE_POSS,
 	PIIX_SATA_FLAGS		= ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR,

@@ -698,7 +700,7 @@ static struct ata_port_info piix_port_in
 
 	[oldpiix_pata] =
 	{
-		.flags		= PIIX_PATA_FLAGS,
+		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_NO_SITRE,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA12_ONLY,
 		.port_ops	= &oldpiix_pata_ops,

@@ -706,7 +708,8 @@ static struct ata_port_info piix_port_in
 
 	[radisys_pata] =
 	{
-		.flags		= PIIX_PATA_FLAGS,
+		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_NO_SITRE |
+				  PIIX_FLAG_RADISYS,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA12_ONLY,
 		.udma_mask	= ATA_UDMA24_ONLY,

@@ -859,10 +862,8 @@ static void piix_set_timings(struct ata_
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
 	unsigned long flags;
 	unsigned int is_slave	= (adev->devno != 0);
-	unsigned int is_radisys	= (dev->vendor == PCI_VENDOR_ID_RADISYS &&
-				   dev->device == 0x8201);
-	unsigned int has_sitre	= (dev->vendor != PCI_VENDOR_ID_INTEL ||
-				   dev->device != 0x1230) && !is_radisys;
+	unsigned int is_radisys	= (ap->flags & PIIX_FLAG_RADISYS) ? 1 : 0;
+	unsigned int has_sitre	= (ap->flags & PIIX_FLAG_NO_SITRE) ? 0 : 1;
 	unsigned int master_port= ap->port_no ? 0x42 : 0x40;
 	unsigned int slave_port	= 0x44;
 	u16 master_data;

Sergei Shtylyov

2011-02-08 17:13 UTC #2 lore raw

In reply to Bartlomiej Zolnierkiewicz

Hello.

Quoted 5 lines · Bartlomiej Zolnierkiewicz

Turn open-coded checks in piix_set_timings() into PIIX_FLAG_* flags per suggestion from Alan Cox.
Suggested-by Alan Cox <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

[...]

drivers/ata/ata_piix.c enum { · line 136

Quoted 9 lines

Index: b/drivers/ata/ata_piix.c
===================================================================
drivers/ata/ata_piix.c +1 -0
@@ -133,6 +133,8 @@ enum {
 
 	PIIX_FLAG_CHECKINTR	= (1 << 28), /* make sure PCI INTx enabled */
 	PIIX_FLAG_SIDPR		= (1 << 29), /* SATA idx/data pair regs */
+	PIIX_FLAG_NO_SITRE	= (1 << 30), /* no SITRE register */

The register in question is called SIDETIM, SITRE is a bit that enables its use.

WBR, Sergei

Bartlomiej Zolnierkiewicz

author

2011-02-08 18:18 UTC #3 lore raw

Hi,

Quoted 12 lines · Sergei Shtylyov

Hello.
Quoted 5 lines · Bartlomiej Zolnierkiewicz
Turn open-coded checks in piix_set_timings() into PIIX_FLAG_* flags per suggestion from Alan Cox.
Suggested-by Alan Cox <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
[...]

Quoted 12 lines

Quoted 9 lines
Index: b/drivers/ata/ata_piix.c
===================================================================
drivers/ata/ata_piix.c +0 -0
@@ -133,6 +133,8 @@ enum {
PIIX_FLAG_CHECKINTR = (1 << 28), /* make sure PCI INTx enabled */
PIIX_FLAG_SIDPR = (1 << 29), /* SATA idx/data pair regs */
+ PIIX_FLAG_NO_SITRE = (1 << 30), /* no SITRE register */
The register in question is called SIDETIM, SITRE is a bit that enables its use.

ICH4-M databook that I have at hand (Intel IDE PRM seems to be gone from Intel's website, though I'm sure I have a backup _somewhere_) it is called SLV_IDETIM so lets just stick with the current naming for now..

Thanks,
Bartlomiej

Sergei Shtylyov

2011-02-08 18:31 UTC #4 lore raw

Quoted 19 lines · Bartlomiej Zolnierkiewicz

Quoted 6 lines
Quoted 4 lines
Turn open-coded checks in piix_set_timings() into PIIX_FLAG_* flags per suggestion from Alan Cox.
Suggested-by Alan Cox <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
[...]
Quoted 12 lines
drivers/ata/ata_piix.c enum { · line 136
Quoted 9 lines
Index: b/drivers/ata/ata_piix.c
===================================================================
drivers/ata/ata_piix.c +1 -0
@@ -133,6 +133,8 @@ enum {
       PIIX_FLAG_CHECKINTR     = (1 << 28), /* make sure PCI INTx enabled
 */
       PIIX_FLAG_SIDPR         = (1 << 29), /* SATA idx/data pair regs */
+       PIIX_FLAG_NO_SITRE      = (1 << 30), /* no SITRE register */
The register in question is called SIDETIM, SITRE is a bit that enables
its use.

Quoted 4 lines

ICH4-M databook that I have at hand (Intel IDE PRM seems to be gone from Intel's website, though I'm sure I have a backup _somewhere_) it is called SLV_IDETIM so lets just stick with the current naming for now..

Why not just say that SITRE is a bit, not register? :-)

Quoted 2 lines

Thanks,
Bartlomiej

WBR, Sergei

Bartlomiej Zolnierkiewicz

author

2011-02-09 09:11 UTC #5 lore raw

Quoted 21 lines · Sergei Shtylyov

Quoted 18 lines · Bartlomiej Zolnierkiewicz
Quoted 18 lines
Quoted 4 lines
Turn open-coded checks in piix_set_timings() into PIIX_FLAG_* flags per suggestion from Alan Cox.
Suggested-by Alan Cox <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
[...]
drivers/ata/ata_piix.c enum { · line 136
Quoted 9 lines
Index: b/drivers/ata/ata_piix.c
===================================================================
drivers/ata/ata_piix.c +1 -0
@@ -133,6 +133,8 @@ enum {
       PIIX_FLAG_CHECKINTR     = (1 << 28), /* make sure PCI INTx enabled
 */
       PIIX_FLAG_SIDPR         = (1 << 29), /* SATA idx/data pair regs */
+       PIIX_FLAG_NO_SITRE      = (1 << 30), /* no SITRE register */
The register in question is called SIDETIM, SITRE is a bit that enables
its use.

Quoted 6 lines

Quoted 4 lines
ICH4-M databook that I have at hand (Intel IDE PRM seems to be gone from Intel's website, though I'm sure I have a backup _somewhere_) it is called SLV_IDETIM so lets just stick with the current naming for now..
Why not just say that SITRE is a bit, not register? :-)

Good point.. :-)

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH v2 21/20] ata_piix: add new PIIX_FLAG_* flags

Turn open-coded checks in piix_set_timings() into PIIX_FLAG_* flags per suggestion from Alan Cox.

Suggested-by Alan Cox <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
On top of https://lkml.org/lkml/2011/2/8/97 patchset.

v2: s/SITRE register/SITRE bit/ per Sergei's comment

 drivers/ata/ata_piix.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

drivers/ata/ata_piix.c +7 -6

Index: b/drivers/ata/ata_piix.c
===================================================================
@@ -133,6 +133,8 @@ enum {
 
 	PIIX_FLAG_CHECKINTR	= (1 << 28), /* make sure PCI INTx enabled */
 	PIIX_FLAG_SIDPR		= (1 << 29), /* SATA idx/data pair regs */
+	PIIX_FLAG_NO_SITRE	= (1 << 30), /* no SITRE bit */
+	PIIX_FLAG_RADISYS	= (1 << 31), /* host is Radisys R82600 */
 
 	PIIX_PATA_FLAGS		= ATA_FLAG_SLAVE_POSS,
 	PIIX_SATA_FLAGS		= ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR,

@@ -698,7 +700,7 @@ static struct ata_port_info piix_port_in
 
 	[oldpiix_pata] =
 	{
-		.flags		= PIIX_PATA_FLAGS,
+		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_NO_SITRE,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA12_ONLY,
 		.port_ops	= &oldpiix_pata_ops,

@@ -706,7 +708,8 @@ static struct ata_port_info piix_port_in
 
 	[radisys_pata] =
 	{
-		.flags		= PIIX_PATA_FLAGS,
+		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_NO_SITRE |
+				  PIIX_FLAG_RADISYS,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA12_ONLY,
 		.udma_mask	= ATA_UDMA24_ONLY,

@@ -859,10 +862,8 @@ static void piix_set_timings(struct ata_
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
 	unsigned long flags;
 	unsigned int is_slave	= (adev->devno != 0);
-	unsigned int is_radisys	= (dev->vendor == PCI_VENDOR_ID_RADISYS &&
-				   dev->device == 0x8201);
-	unsigned int has_sitre	= (dev->vendor != PCI_VENDOR_ID_INTEL ||
-				   dev->device != 0x1230) && !is_radisys;
+	unsigned int is_radisys	= (ap->flags & PIIX_FLAG_RADISYS) ? 1 : 0;
+	unsigned int has_sitre	= (ap->flags & PIIX_FLAG_NO_SITRE) ? 0 : 1;
 	unsigned int master_port= ap->port_no ? 0x42 : 0x40;
 	unsigned int slave_port	= 0x44;
 	u16 master_data;

Sergei Shtylyov

2011-02-11 13:05 UTC #6 lore raw

In reply to Bartlomiej Zolnierkiewicz

Hello.

Quoted 21 lines · Bartlomiej Zolnierkiewicz

Quoted 8 lines
Quoted 5 lines
Turn open-coded checks in piix_set_timings() into PIIX_FLAG_* flags per suggestion from Alan Cox.
Suggested-by Alan Cox<[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz<[email protected]>
[...]
Quoted 12 lines
drivers/ata/ata_piix.c enum { · line 136
Quoted 9 lines
Index: b/drivers/ata/ata_piix.c
===================================================================
drivers/ata/ata_piix.c +1 -0
@@ -133,6 +133,8 @@ enum {
        PIIX_FLAG_CHECKINTR     = (1<<  28), /* make sure PCI INTx enabled
 */
        PIIX_FLAG_SIDPR         = (1<<  29), /* SATA idx/data pair regs */
+       PIIX_FLAG_NO_SITRE      = (1<<  30), /* no SITRE register */
The register in question is called SIDETIM, SITRE is a bit that enables
its use.

Quoted 2 lines

ICH4-M databook that I have at hand (Intel IDE PRM seems to be gone from Intel's website, though I'm sure I have a backup _somewhere_) it

If you mean ICH IDE Controller PRM (29860004.pdf), it indeed calls the
register *SITR* (yet it has SIDETIM mentioned in the table 37 :-). Intel seems
not very consistent in its documentation. :-)

Quoted 2 lines

Thanks,
Bartlomiej

WBR, Sergei