Subject: [PATCH 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.

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

Index: b/drivers/ata/ata_piix.c
===================================================================
--- a/drivers/ata/ata_piix.c
+++ 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;


Subject: Re: [PATCH 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.
- Get the reverse logic correct this time.

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

Index: b/drivers/ata/ata_piix.c
===================================================================
--- a/drivers/ata/ata_piix.c
+++ 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;

2011-02-08 17:13:42

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH 21/20] ata_piix: add new PIIX_FLAG_* flags

Hello.

Bartlomiej Zolnierkiewicz wrote:

> 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]>
[...]

> Index: b/drivers/ata/ata_piix.c
> ===================================================================
> --- a/drivers/ata/ata_piix.c
> +++ 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 */

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

WBR, Sergei

Subject: Re: [PATCH 21/20] ata_piix: add new PIIX_FLAG_* flags

Hi,

On Tue, Feb 8, 2011 at 6:12 PM, Sergei Shtylyov <[email protected]> wrote:
> Hello.
>
> Bartlomiej Zolnierkiewicz wrote:
>
>> 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]>
>
> [...]
>
>> Index: b/drivers/ata/ata_piix.c
>> ===================================================================
>> --- a/drivers/ata/ata_piix.c
>> +++ 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 */
>
> ? 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

2011-02-08 18:31:50

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH 21/20] ata_piix: add new PIIX_FLAG_* flags

Bartlomiej Zolnierkiewicz wrote:

>>> 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]>
>> [...]

>>> Index: b/drivers/ata/ata_piix.c
>>> ===================================================================
>>> --- a/drivers/ata/ata_piix.c
>>> +++ 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 */

>> 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..

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

> Thanks,
> Bartlomiej

WBR, Sergei

Subject: Re: [PATCH 21/20] ata_piix: add new PIIX_FLAG_* flags

Sergei Shtylyov wrote:

> Bartlomiej Zolnierkiewicz wrote:
>
> >>> 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]>
> >> [...]
>
> >>> Index: b/drivers/ata/ata_piix.c
> >>> ===================================================================
> >>> --- a/drivers/ata/ata_piix.c
> >>> +++ 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 */
>
> >> 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..
>
> 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(-)

Index: b/drivers/ata/ata_piix.c
===================================================================
--- a/drivers/ata/ata_piix.c
+++ 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;

2011-02-11 13:05:28

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH 21/20] ata_piix: add new PIIX_FLAG_* flags

Hello.

On 08-02-2011 21:18, Bartlomiej Zolnierkiewicz wrote:

>>> 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]>

>> [...]

>>> Index: b/drivers/ata/ata_piix.c
>>> ===================================================================
>>> --- a/drivers/ata/ata_piix.c
>>> +++ 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 */

>> 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

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. :-)

> Thanks,
> Bartlomiej

WBR, Sergei