Re: Patch(2.4.5): Fix PCMCIA ATA/IDE freeze (w/ PCI add-in cards)V3

Re: Patch(2.4.5): Fix PCMCIA ATA/IDE freeze (w/ PCI add-in cards)V3

[Andries.Brouwer]

    Andre Hedrick wrote:

    > That is a legacy bit from ATA-2 but it is one of those things you cannot
    > get rid of :-(

    in ANSI X3.279-1996, "AT Attachment Interface with Extensions (ATA-2)",
    Approved September 11, 1996 , control register bit 3-7 are reserved.

    However ANSI X3.221-1994, "AT Attachment Interface for Disk Drives",
    Approved May 12, 1994, bit3 is "1" and bits 4-7 are "x".
    No further explanation.

    How far back must we go, to get the sense ?

    >   struct {
    >           unsigned bit0           : 1;
    >           unsigned nIEN           : 1;    /* device INTRQ to host */
    >           unsigned SRST           : 1;    /* host soft reset bit */
    >           unsigned bit3           : 1;    /* ATA-2 thingy */
    >           unsigned reserved456    : 3;
    >           unsigned HOB            : 1;    /* 48-bit address ordering */
    >   } control_t;
    > 
    > once I add-in the real def of bit3 then I will not
    > need to look it up again.

bit3: 0: drive has 1-8 heads
      1: drive has more than 8 heads

(From old MFM/RLL times. In ATA-1 bit3 is set to 1.
See also
	http://www.win.tue.nl/~aeb/linux/hdtypes/hdtypes-2.html
.)

Andries

Re: Patch(2.4.5): Fix PCMCIA ATA/IDE freeze (w/ PCI add-in cards)V3

[Andre Hedrick]

I should have known that you (instructor of the current and previous
maintainer) would have the answer off the top ;-)
Therefore by your description it mys be set always but I guess it is a
DGD; however, I do want to know that it is now.

Oh, and now that Big Drive Technology has been annouced I should finally
send you the code for 48-bit, sorry about the delay.

Cheers,

Andre Hedrick
ASL Kernel Development
Linux ATA Development
-----------------------------------------------------------------------------
ASL, Inc.                                     Toll free: 1-877-ASL-3535
1757 Houret Court                             Fax: 1-408-941-2071
Milpitas, CA 95035                            Web: www.aslab.com

On Fri, 29 Jun 2001 Andries.Brouwer@cwi.nl wrote:

>     Andre Hedrick wrote:
> 
>     > That is a legacy bit from ATA-2 but it is one of those things you cannot
>     > get rid of :-(
> 
>     in ANSI X3.279-1996, "AT Attachment Interface with Extensions (ATA-2)",
>     Approved September 11, 1996 , control register bit 3-7 are reserved.
> 
>     However ANSI X3.221-1994, "AT Attachment Interface for Disk Drives",
>     Approved May 12, 1994, bit3 is "1" and bits 4-7 are "x".
>     No further explanation.
> 
>     How far back must we go, to get the sense ?
> 
>     >   struct {
>     >           unsigned bit0           : 1;
>     >           unsigned nIEN           : 1;    /* device INTRQ to host */
>     >           unsigned SRST           : 1;    /* host soft reset bit */
>     >           unsigned bit3           : 1;    /* ATA-2 thingy */
>     >           unsigned reserved456    : 3;
>     >           unsigned HOB            : 1;    /* 48-bit address ordering */
>     >   } control_t;
>     > 
>     > once I add-in the real def of bit3 then I will not
>     > need to look it up again.
> 
> bit3: 0: drive has 1-8 heads
>       1: drive has more than 8 heads
> 
> (From old MFM/RLL times. In ATA-1 bit3 is set to 1.
> See also
> 	http://www.win.tue.nl/~aeb/linux/hdtypes/hdtypes-2.html
> .)
> 
> Andries
> 

Re: Patch(2.4.5): Fix PCMCIA ATA/IDE freeze (w/ PCI add-in cards)

[Andre Hedrick]

Gunther,

It fixes a BUG in CFA, but what will it do to the other stuff?
Parse it exclusive to CFA and there is not an issue.

Also look closely....

No all ./arch have a control register doing this randomly without know the
rest of the driver will kill more than it fixes.

static int try_to_identify (ide_drive_t *drive, byte cmd)
{
        int rc;
        ide_ioreg_t hd_status;
        unsigned long timeout;
        unsigned long irqs = 0;
        byte s, a;

        if (IDE_CONTROL_REG) {
<snip>
        } else {
                ide_delay_50ms();
                hd_status = IDE_STATUS_REG;
        }
<snip>
}

It will not be accepted until it correctly address and handles all HOSTs
correctly, period.

Andre Hedrick
ASL Kernel Development
Linux ATA Development
-----------------------------------------------------------------------------
ASL, Inc.                                     Toll free: 1-877-ASL-3535
1757 Houret Court                             Fax: 1-408-941-2071
Milpitas, CA 95035                            Web: www.aslab.com

On Wed, 27 Jun 2001, Gunther Mayer wrote:

> Andre Hedrick wrote:
> > 
> > PARANIOA.
> 
> This is not a valid reason.
> 
> This clearly fixes a bug in linux. Note: the irq disable
> is local to ide-cs. Are you paranoid enough to believe
> enabling the irq by writing globally to the control register that
> existed since ATA will have ill effects? 
> 
> You claim the relevant PCMCIA ATA behaviour is not ATA(>3?) compliant,
> however you didn`t yet give any facts to support this !
> 
> You claim this locks the driver, again no facts.
> 
> 
> > 
> > Remember that ATAPI is generally screwed beyond reality, so adjusting the
> > probe code in general (global) is a bad thing.
> ...
> > On Wed, 27 Jun 2001, Alan Cox wrote:
> > 
> > > > obsoleting ATA-2 did their attention at CFA become alarmed.  I agree that
> > > > there needs to be a fix, but not at the price of locking the rest of the
> > > > driver.  Since we now the identity of the device prior to assigned the
> > > > interrupt we can handle the execption, but you do not go around blanket
> > > > wacking the control register of all devices.
> 
> The proposed patch is very simple (as per Linus' liking). When considering to
> install an earlier (and  global) irq handler I believe you can see
> this will impose a much greater risk !
> 
> > >
> > > I dont see why it locks up the driver ?
> 

Re: Patch(2.4.5): Fix PCMCIA ATA/IDE freeze (w/ PCI add-in cards) V3

[Gunther Mayer]

Andre Hedrick wrote:
> 
> It fixes a BUG in CFA, but what will it do to the other stuff?
> Parse it exclusive to CFA and there is not an issue.
...
> Not all ./arch have a control register doing this randomly without know the
> rest of the driver will kill more than it fixes.
> 

Thanks for pointing out this implementation bug. Although I fixed another problem
in ide-cs, where ctl_base could eventually be 0.

I would rather not add a special hwif->is_pcmcia flag, as
the control register (if it exists) is well defined
(bit2=softreset bit1=nIEN, others reserved; however there is
 a hardcoded value of 0x08 somewhere in the ide code?).

-
Gunther



--- linux245.orig/drivers/ide/ide-cs.c  Fri Feb  9 20:40:02 2001
+++ linux/drivers/ide/ide-cs.c  Thu Jun 28 18:04:27 2001
@@ -42,6 +42,7 @@
 #include <linux/ioport.h>
 #include <linux/hdreg.h>
 #include <linux/major.h>
+#include <linux/ide.h>
 
 #include <asm/io.h>
 #include <asm/system.h>
@@ -223,6 +224,15 @@
 #define CFG_CHECK(fn, args...) \
 if (CardServices(fn, args) != 0) goto next_entry
 
+int idecs_register (int arg1, int arg2, int irq)
+{
+        hw_regs_t hw;
+        ide_init_hwif_ports(&hw, (ide_ioreg_t) arg1, (ide_ioreg_t) arg2, NULL);
+        hw.irq = irq;
+        hw.chipset = ide_pci; // this enables IRQ sharing w/ PCI irqs
+        return ide_register_hw(&hw, NULL);
+}
+
 void ide_config(dev_link_t *link)
 {
     client_handle_t handle = link->handle;
@@ -326,10 +336,12 @@
 
     /* retry registration in case device is still spinning up */
     for (i = 0; i < 10; i++) {
-       hd = ide_register(io_base, ctl_base, link->irq.AssignedIRQ);
+       if(ctl_base) outb(0x02, ctl_base); // Set nIEN = disable device interrupts
+       hd = idecs_register(io_base, ctl_base, link->irq.AssignedIRQ);
        if (hd >= 0) break;
        if (link->io.NumPorts1 == 0x20) {
-           hd = ide_register(io_base+0x10, ctl_base+0x10,
+           if(ctl_base) outb(0x02, ctl_base+0x10);
+           hd = idecs_register(io_base+0x10, ctl_base+0x10,
                              link->irq.AssignedIRQ);
            if (hd >= 0) {
                io_base += 0x10; ctl_base += 0x10;
--- linux245.orig/drivers/ide/ide-probe.c       Sun Mar 18 18:25:02 2001
+++ linux/drivers/ide/ide-probe.c       Thu Jun 28 18:43:43 2001
@@ -685,6 +685,9 @@
 #else /* !CONFIG_IDEPCI_SHARE_IRQ */
                int sa = (hwif->chipset == ide_pci) ? SA_INTERRUPT|SA_SHIRQ : SA_INTERRUPT;
 #endif /* CONFIG_IDEPCI_SHARE_IRQ */
+
+               if(hwif->io_ports[IDE_CONTROL_OFFSET])
+                       OUT_BYTE(0x00, hwif->io_ports[IDE_CONTROL_OFFSET]); // clear nIEN == enable irqs
                if (ide_request_irq(hwif->irq, &ide_intr, sa, hwif->name, hwgroup)) {
                        if (!match)
                                kfree(hwgroup);
--- linux245.orig/drivers/ide/ide.c     Wed May  2 01:05:00 2001
+++ linux/drivers/ide/ide.c     Thu Jun 28 18:04:42 2001
@@ -2181,6 +2181,7 @@
        memcpy(hwif->io_ports, hwif->hw.io_ports, sizeof(hwif->hw.io_ports));
        hwif->irq = hw->irq;
        hwif->noprobe = 0;
+       hwif->chipset = hw->chipset;
 
        if (!initializing) {
                ide_probe_module();
--- linux245.orig/include/linux/ide.h   Sat May 26 03:02:42 2001
+++ linux/include/linux/ide.h   Thu Jun 28 18:18:05 2001
@@ -226,6 +226,19 @@
 #endif
 
 /*
+ * hwif_chipset_t is used to keep track of the specific hardware
+ * chipset used by each IDE interface, if known.
+ */
+typedef enum {  ide_unknown,    ide_generic,    ide_pci,
+                ide_cmd640,     ide_dtc2278,    ide_ali14xx,
+                ide_qd6580,     ide_umc8672,    ide_ht6560b,
+                ide_pdc4030,    ide_rz1000,     ide_trm290,
+                ide_cmd646,     ide_cy82c693,   ide_4drives,
+                ide_pmac
+} hwif_chipset_t;
+
+
+/*
  * Structure to hold all information about the location of this port
  */
 typedef struct hw_regs_s {
@@ -234,6 +247,7 @@
        int             dma;                    /* our dma entry */
        ide_ack_intr_t  *ack_intr;              /* acknowledge interrupt */
        void            *priv;                  /* interface specific data */
+       hwif_chipset_t  chipset;
 } hw_regs_t;
 
 /*
@@ -396,17 +410,6 @@
 typedef void (ide_maskproc_t) (ide_drive_t *, int);
 typedef void (ide_rw_proc_t) (ide_drive_t *, ide_dma_action_t);
 
-/*
- * hwif_chipset_t is used to keep track of the specific hardware
- * chipset used by each IDE interface, if known.
- */
-typedef enum { ide_unknown,    ide_generic,    ide_pci,
-               ide_cmd640,     ide_dtc2278,    ide_ali14xx,
-               ide_qd6580,     ide_umc8672,    ide_ht6560b,
-               ide_pdc4030,    ide_rz1000,     ide_trm290,
-               ide_cmd646,     ide_cy82c693,   ide_4drives,
-               ide_pmac
-} hwif_chipset_t;
 
 #ifdef CONFIG_BLK_DEV_IDEPCI
 typedef struct ide_pci_devid_s {

Re: Patch(2.4.5): Fix PCMCIA ATA/IDE freeze (w/ PCI add-in cards) V3

[Andre Hedrick]

That is a legacy bit from ATA-2 but it is one of those things you can not
get rid of :-( even thou things are obsoleted, they are not retired.
This means that you have to go back into the past to see how it was used,
silly!  I hope you agree to that point.

This is the drive->ctrl register pointer.

outp(drive->ctl|0x02, IDE_CONTROL_REG);

typedef union {
        unsigned all                    : 8;    /* all of the bits together */
        struct {
                unsigned bit0           : 1;
                unsigned nIEN           : 1;    /* device INTRQ to host */
                unsigned SRST           : 1;    /* host soft reset bit */
                unsigned bit3           : 1;    /* ATA-2 thingy */
                unsigned reserved456    : 3;
                unsigned HOB            : 1;    /* 48-bit address ordering */
        } b;
} control_t;

This is a new struct that is to be added for 48-bit addressing and it will
reflect drive->ctl soon.  I have not decided how to use it best or at all,
but it has meaning and once I add-in the real def of bit3 then I will not
need to look it up again.

Cheers,

Andre Hedrick
ASL Kernel Development
Linux ATA Development
-----------------------------------------------------------------------------
ASL, Inc.                                     Toll free: 1-877-ASL-3535
1757 Houret Court                             Fax: 1-408-941-2071
Milpitas, CA 95035                            Web: www.aslab.com

On Thu, 28 Jun 2001, Gunther Mayer wrote:

> Andre Hedrick wrote:
> > 
> > It fixes a BUG in CFA, but what will it do to the other stuff?
> > Parse it exclusive to CFA and there is not an issue.
> ...
> > Not all ./arch have a control register doing this randomly without know the
> > rest of the driver will kill more than it fixes.
> > 
> 
> Thanks for pointing out this implementation bug. Although I fixed another problem
> in ide-cs, where ctl_base could eventually be 0.
> 
> I would rather not add a special hwif->is_pcmcia flag, as
> the control register (if it exists) is well defined
> (bit2=softreset bit1=nIEN, others reserved; however there is
>  a hardcoded value of 0x08 somewhere in the ide code?).
> 
> -
> Gunther
> 
> 
> 
> --- linux245.orig/drivers/ide/ide-cs.c  Fri Feb  9 20:40:02 2001
> +++ linux/drivers/ide/ide-cs.c  Thu Jun 28 18:04:27 2001
> @@ -42,6 +42,7 @@
>  #include <linux/ioport.h>
>  #include <linux/hdreg.h>
>  #include <linux/major.h>
> +#include <linux/ide.h>
>  
>  #include <asm/io.h>
>  #include <asm/system.h>
> @@ -223,6 +224,15 @@
>  #define CFG_CHECK(fn, args...) \
>  if (CardServices(fn, args) != 0) goto next_entry
>  
> +int idecs_register (int arg1, int arg2, int irq)
> +{
> +        hw_regs_t hw;
> +        ide_init_hwif_ports(&hw, (ide_ioreg_t) arg1, (ide_ioreg_t) arg2, NULL);
> +        hw.irq = irq;
> +        hw.chipset = ide_pci; // this enables IRQ sharing w/ PCI irqs
> +        return ide_register_hw(&hw, NULL);
> +}
> +
>  void ide_config(dev_link_t *link)
>  {
>      client_handle_t handle = link->handle;
> @@ -326,10 +336,12 @@
>  
>      /* retry registration in case device is still spinning up */
>      for (i = 0; i < 10; i++) {
> -       hd = ide_register(io_base, ctl_base, link->irq.AssignedIRQ);
> +       if(ctl_base) outb(0x02, ctl_base); // Set nIEN = disable device interrupts
> +       hd = idecs_register(io_base, ctl_base, link->irq.AssignedIRQ);
>         if (hd >= 0) break;
>         if (link->io.NumPorts1 == 0x20) {
> -           hd = ide_register(io_base+0x10, ctl_base+0x10,
> +           if(ctl_base) outb(0x02, ctl_base+0x10);
> +           hd = idecs_register(io_base+0x10, ctl_base+0x10,
>                               link->irq.AssignedIRQ);
>             if (hd >= 0) {
>                 io_base += 0x10; ctl_base += 0x10;
> --- linux245.orig/drivers/ide/ide-probe.c       Sun Mar 18 18:25:02 2001
> +++ linux/drivers/ide/ide-probe.c       Thu Jun 28 18:43:43 2001
> @@ -685,6 +685,9 @@
>  #else /* !CONFIG_IDEPCI_SHARE_IRQ */
>                 int sa = (hwif->chipset == ide_pci) ? SA_INTERRUPT|SA_SHIRQ : SA_INTERRUPT;
>  #endif /* CONFIG_IDEPCI_SHARE_IRQ */
> +
> +               if(hwif->io_ports[IDE_CONTROL_OFFSET])
> +                       OUT_BYTE(0x00, hwif->io_ports[IDE_CONTROL_OFFSET]); // clear nIEN == enable irqs
>                 if (ide_request_irq(hwif->irq, &ide_intr, sa, hwif->name, hwgroup)) {
>                         if (!match)
>                                 kfree(hwgroup);
> --- linux245.orig/drivers/ide/ide.c     Wed May  2 01:05:00 2001
> +++ linux/drivers/ide/ide.c     Thu Jun 28 18:04:42 2001
> @@ -2181,6 +2181,7 @@
>         memcpy(hwif->io_ports, hwif->hw.io_ports, sizeof(hwif->hw.io_ports));
>         hwif->irq = hw->irq;
>         hwif->noprobe = 0;
> +       hwif->chipset = hw->chipset;
>  
>         if (!initializing) {
>                 ide_probe_module();
> --- linux245.orig/include/linux/ide.h   Sat May 26 03:02:42 2001
> +++ linux/include/linux/ide.h   Thu Jun 28 18:18:05 2001
> @@ -226,6 +226,19 @@
>  #endif
>  
>  /*
> + * hwif_chipset_t is used to keep track of the specific hardware
> + * chipset used by each IDE interface, if known.
> + */
> +typedef enum {  ide_unknown,    ide_generic,    ide_pci,
> +                ide_cmd640,     ide_dtc2278,    ide_ali14xx,
> +                ide_qd6580,     ide_umc8672,    ide_ht6560b,
> +                ide_pdc4030,    ide_rz1000,     ide_trm290,
> +                ide_cmd646,     ide_cy82c693,   ide_4drives,
> +                ide_pmac
> +} hwif_chipset_t;
> +
> +
> +/*
>   * Structure to hold all information about the location of this port
>   */
>  typedef struct hw_regs_s {
> @@ -234,6 +247,7 @@
>         int             dma;                    /* our dma entry */
>         ide_ack_intr_t  *ack_intr;              /* acknowledge interrupt */
>         void            *priv;                  /* interface specific data */
> +       hwif_chipset_t  chipset;
>  } hw_regs_t;
>  
>  /*
> @@ -396,17 +410,6 @@
>  typedef void (ide_maskproc_t) (ide_drive_t *, int);
>  typedef void (ide_rw_proc_t) (ide_drive_t *, ide_dma_action_t);
>  
> -/*
> - * hwif_chipset_t is used to keep track of the specific hardware
> - * chipset used by each IDE interface, if known.
> - */
> -typedef enum { ide_unknown,    ide_generic,    ide_pci,
> -               ide_cmd640,     ide_dtc2278,    ide_ali14xx,
> -               ide_qd6580,     ide_umc8672,    ide_ht6560b,
> -               ide_pdc4030,    ide_rz1000,     ide_trm290,
> -               ide_cmd646,     ide_cy82c693,   ide_4drives,
> -               ide_pmac
> -} hwif_chipset_t;
>  
>  #ifdef CONFIG_BLK_DEV_IDEPCI
>  typedef struct ide_pci_devid_s {
> 

Re: Patch(2.4.5): Fix PCMCIA ATA/IDE freeze (w/ PCI add-in cards)V3

[Gunther Mayer]

Andre Hedrick wrote:
> 
> That is a legacy bit from ATA-2 but it is one of those things you can not
> get rid of :-( even thou things are obsoleted, they are not retired.
> This means that you have to go back into the past to see how it was used,
> silly!  I hope you agree to that point.

No,
in ANSI X3.279-1996, "AT Attachment Interface with Extensions (ATA-2)",
Approved September 11, 1996 , control register bit 3-7 are reserved.

However ANSI X3.221-1994, "AT Attachment Interface for Disk Drives",
Approved May 12, 1994, bit3 is "1" and bits 4-7 are "x". No further explanation.

How far back must we go, to get the sense ?

> 
> This is the drive->ctrl register pointer.
> 
> outp(drive->ctl|0x02, IDE_CONTROL_REG);
> 
> typedef union {
>         unsigned all                    : 8;    /* all of the bits together */
>         struct {
>                 unsigned bit0           : 1;
>                 unsigned nIEN           : 1;    /* device INTRQ to host */
>                 unsigned SRST           : 1;    /* host soft reset bit */
>                 unsigned bit3           : 1;    /* ATA-2 thingy */
>                 unsigned reserved456    : 3;
>                 unsigned HOB            : 1;    /* 48-bit address ordering */
>         } b;
> } control_t;
> 
> This is a new struct that is to be added for 48-bit addressing and it will
> reflect drive->ctl soon.  I have not decided how to use it best or at all,
> but it has meaning and once I add-in the real def of bit3 then I will not
> need to look it up again.

Re: Patch(2.4.5): Fix PCMCIA ATA/IDE freeze (w/ PCI add-in cards)V3

[Andre Hedrick]

Okay my bad it is ATA-1 but even that does not explain the bit.
only that section 7.2.6 top of page 14 (index numbers) defines it to be
set to 1 with out a reason.

This this is a pre-ATA thing back in IDE.

If you really want to know the answer I can go dig it up, but later.

Cheers,

Andre Hedrick
ASL Kernel Development
Linux ATA Development
-----------------------------------------------------------------------------
ASL, Inc.                                     Toll free: 1-877-ASL-3535
1757 Houret Court                             Fax: 1-408-941-2071
Milpitas, CA 95035                            Web: www.aslab.com

On Fri, 29 Jun 2001, Gunther Mayer wrote:

> Andre Hedrick wrote:
> > 
> > That is a legacy bit from ATA-2 but it is one of those things you can not
> > get rid of :-( even thou things are obsoleted, they are not retired.
> > This means that you have to go back into the past to see how it was used,
> > silly!  I hope you agree to that point.
> 
> No,
> in ANSI X3.279-1996, "AT Attachment Interface with Extensions (ATA-2)",
> Approved September 11, 1996 , control register bit 3-7 are reserved.
> 
> However ANSI X3.221-1994, "AT Attachment Interface for Disk Drives",
> Approved May 12, 1994, bit3 is "1" and bits 4-7 are "x". No further explanation.
> 
> How far back must we go, to get the sense ?
> 
> > 
> > This is the drive->ctrl register pointer.
> > 
> > outp(drive->ctl|0x02, IDE_CONTROL_REG);
> > 
> > typedef union {
> >         unsigned all                    : 8;    /* all of the bits together */
> >         struct {
> >                 unsigned bit0           : 1;
> >                 unsigned nIEN           : 1;    /* device INTRQ to host */
> >                 unsigned SRST           : 1;    /* host soft reset bit */
> >                 unsigned bit3           : 1;    /* ATA-2 thingy */
> >                 unsigned reserved456    : 3;
> >                 unsigned HOB            : 1;    /* 48-bit address ordering */
> >         } b;
> > } control_t;
> > 
> > This is a new struct that is to be added for 48-bit addressing and it will
> > reflect drive->ctl soon.  I have not decided how to use it best or at all,
> > but it has meaning and once I add-in the real def of bit3 then I will not
> > need to look it up again.
>