[PATCH] updates to Vitesse SATA driver

[PATCH] updates to Vitesse SATA driver

[Dave]

[-- Attachment #1: Type: text/plain, Size: 1417 bytes --]

Signed-off-by: Dave Jiang (dave.jiang@gmail.com)
Patch attached, diffed against 2.6.8.1.

I was trying to get the Vitesse SATA driver running on XScale platform
and encountered the problem during identify device where when
vsc_intr_mask_update() was called the irq mask register never seem to
change. It seems that on certain XScale platforms doing a writeb() to
a 32bit register (instead of writel()) doesn't seem to do anything.  I
updated the code to do writel() instead so that other platforms
besides IA32 would work.

After getting that working I noticed that the last_ctl value gets
changed back by the LIBATA core and the Vitesse SATA driver depends on
the previous value in order to unmask the interrupt. So it seems that
there's no need to set/unset the interrupt masks in the first place if
we set the CTL register with ATA_NIEN directly, which I believe is
probably the way LIBATA intended the driver to do. After doing that
everything seems to be working great. So I suppose
vsc_intr_mask_update() is no longer needed probably.

I also added some code to enable the per port LEDs on the HBA during
initialization. Perviously all activities goes to port 0 LED.

-- 
-= Dave =-

Software Engineer - Advanced Development Engineering Team 
Storage Component Division - Intel Corp. 
----
The views expressed in this email are
mine alone and do not necessarily 
reflect the views of my employer
(Intel Corp.).

[-- Attachment #2: patch-sata --]
[-- Type: application/octet-stream, Size: 1732 bytes --]

--- linux-2.6.8.1-iop3/drivers/scsi/sata_vsc.c	2004-08-14 03:55:33.000000000 -0700
+++ dj_bktree-2.6.8.1/drivers/scsi/sata_vsc.c	2004-09-17 15:43:12.688212928 -0700
@@ -80,6 +80,7 @@
 
 static void vsc_intr_mask_update(struct ata_port *ap, u8 ctl)
 {
+#if 0
 	unsigned long mask_addr;
 	u8 mask;
 
@@ -91,6 +92,26 @@
 	else
 		mask &= 0x7F;
 	writeb(mask, mask_addr);
+#endif 
+    u32 mask = 0;
+    u32 regval; 
+
+    regval = readl(ap->host_set->mmio_base + VSC_SATA_INT_MASK_OFFSET);
+    
+
+    mask = (0x80 << (8 * ap->port_no));
+    
+    if(ctl & ATA_NIEN)
+    {
+        mask = ~mask;
+        mask = regval & mask;
+    }
+    else
+    {
+        mask = regval | mask;
+    }
+
+    writel(mask, ap->host_set->mmio_base + VSC_SATA_INT_MASK_OFFSET);
 }
 
 
@@ -105,8 +126,10 @@
 	 * However, if ATA_NIEN is changed, then we need to change the interrupt register.
 	 */
 	if ((tf->ctl & ATA_NIEN) != (ap->last_ctl & ATA_NIEN)) {
+        writeb(tf->ctl, ioaddr->ctl_addr);
 		ap->last_ctl = tf->ctl;
-		vsc_intr_mask_update(ap, tf->ctl & ATA_NIEN);
+        ata_wait_idle(ap);
+//      vsc_intr_mask_update(ap, tf->ctl & ATA_NIEN);
 	}
 	if (is_addr && (tf->flags & ATA_TFLAG_LBA48)) {
 		writew(tf->feature | (((u16)tf->hob_feature) << 8), ioaddr->feature_addr);
@@ -330,6 +353,19 @@
 
 	pci_set_master(pdev);
 
+    /* set per port LEDs */
+    {
+        unsigned long mask; 
+        unsigned int regval;
+
+        set_bit(28, &mask);
+        mask = ~mask;
+        
+        pci_read_config_dword(pdev, 0x98, &regval);
+        regval = regval & mask;
+        pci_write_config_dword(pdev, 0x98, regval);
+    }
+
 	/* FIXME: check ata_device_add return value */
 	ata_device_add(probe_ent);
 	kfree(probe_ent);

Re: [PATCH] updates to Vitesse SATA driver

[Jeremy Higdon]

On Tue, Sep 21, 2004 at 02:16:46PM -0700, Dave wrote:
> Signed-off-by: Dave Jiang (dave.jiang@gmail.com)
> Patch attached, diffed against 2.6.8.1.
> 
> I was trying to get the Vitesse SATA driver running on XScale platform
> and encountered the problem during identify device where when
> vsc_intr_mask_update() was called the irq mask register never seem to
> change. It seems that on certain XScale platforms doing a writeb() to
> a 32bit register (instead of writel()) doesn't seem to do anything.  I
> updated the code to do writel() instead so that other platforms
> besides IA32 would work.

It actually does work already on IA64.
Why would a writeb not work?  All it does is set the appropriate byte
selects.  The platform doesn't care what the register size is, and the
target (PCI chip) seems to be able to figure it out, so there must be
something else wrong.

What is an XScale, btw?

> After getting that working I noticed that the last_ctl value gets
> changed back by the LIBATA core and the Vitesse SATA driver depends on
> the previous value in order to unmask the interrupt. So it seems that
> there's no need to set/unset the interrupt masks in the first place if
> we set the CTL register with ATA_NIEN directly, which I believe is
> probably the way LIBATA intended the driver to do. After doing that
> everything seems to be working great. So I suppose
> vsc_intr_mask_update() is no longer needed probably.

The chip specification says not to set it that way.  Are you using
the chip in DPA mode or PCI-IDE mode?  The driver assumes the former.  I
don't know why anyone would use the chip in PCI-IDE mode.

> I also added some code to enable the per port LEDs on the HBA during
> initialization. Perviously all activities goes to port 0 LED.
> 
> -- 
> -= Dave =-
> 
> Software Engineer - Advanced Development Engineering Team 
> Storage Component Division - Intel Corp. 
> ----
> The views expressed in this email are
> mine alone and do not necessarily 
> reflect the views of my employer
> (Intel Corp.).

> --- linux-2.6.8.1-iop3/drivers/scsi/sata_vsc.c	2004-08-14 03:55:33.000000000 -0700
> +++ dj_bktree-2.6.8.1/drivers/scsi/sata_vsc.c	2004-09-17 15:43:12.688212928 -0700
> @@ -80,6 +80,7 @@
>  
>  static void vsc_intr_mask_update(struct ata_port *ap, u8 ctl)
>  {
> +#if 0
>  	unsigned long mask_addr;
>  	u8 mask;
>  
> @@ -91,6 +92,26 @@
>  	else
>  		mask &= 0x7F;
>  	writeb(mask, mask_addr);
> +#endif 
> +    u32 mask = 0;
> +    u32 regval; 
> +
> +    regval = readl(ap->host_set->mmio_base + VSC_SATA_INT_MASK_OFFSET);
> +    
> +
> +    mask = (0x80 << (8 * ap->port_no));
> +    
> +    if(ctl & ATA_NIEN)
> +    {
> +        mask = ~mask;
> +        mask = regval & mask;
> +    }
> +    else
> +    {
> +        mask = regval | mask;
> +    }
> +
> +    writel(mask, ap->host_set->mmio_base + VSC_SATA_INT_MASK_OFFSET);
>  }
>  
>  
> @@ -105,8 +126,10 @@
>  	 * However, if ATA_NIEN is changed, then we need to change the interrupt register.
>  	 */
>  	if ((tf->ctl & ATA_NIEN) != (ap->last_ctl & ATA_NIEN)) {
> +        writeb(tf->ctl, ioaddr->ctl_addr);
>  		ap->last_ctl = tf->ctl;
> -		vsc_intr_mask_update(ap, tf->ctl & ATA_NIEN);
> +        ata_wait_idle(ap);
> +//      vsc_intr_mask_update(ap, tf->ctl & ATA_NIEN);
>  	}
>  	if (is_addr && (tf->flags & ATA_TFLAG_LBA48)) {
>  		writew(tf->feature | (((u16)tf->hob_feature) << 8), ioaddr->feature_addr);
> @@ -330,6 +353,19 @@
>  
>  	pci_set_master(pdev);
>  
> +    /* set per port LEDs */
> +    {
> +        unsigned long mask; 
> +        unsigned int regval;
> +
> +        set_bit(28, &mask);
> +        mask = ~mask;
> +        
> +        pci_read_config_dword(pdev, 0x98, &regval);
> +        regval = regval & mask;
> +        pci_write_config_dword(pdev, 0x98, regval);
> +    }
> +

That's a somewhat wordy way of doing it.  You could just do:

	pci_read_config_dword(pdev, 0x98, &regval);
	regval = regval & ~(1 << 28)
	pci_write_config_dword(pdev, 0x98, regval);

and since the driver is DPA only, we can just do:

	pci_write_config_dword(pdev, 0x98, 0);

If anyone ever adds PCI-IDE support, then we're probably before
the point that we're trying to select channel 0/1, so I think that's
still safe.

>  	/* FIXME: check ata_device_add return value */
>  	ata_device_add(probe_ent);
>  	kfree(probe_ent);

Also, jgarzik is the maintainer of this driver.  I'll cc him.

jeremy

Re: [PATCH] updates to Vitesse SATA driver

[Dave]

On Tue, 21 Sep 2004 19:06:14 -0700, Jeremy Higdon <jeremy@sgi.com> wrote:
> On Tue, Sep 21, 2004 at 02:16:46PM -0700, Dave wrote:
> > Signed-off-by: Dave Jiang (dave.jiang@gmail.com )
> > Patch attached, diffed against 2.6.8.1 .
> >
> > I was trying to get the Vitesse SATA driver running on XScale platform
> > and encountered the problem during identify device where when
> > vsc_intr_mask_update() was called the irq mask register never seem to
> > change. It seems that on certain XScale platforms doing a writeb() to
> > a 32bit register (instead of writel()) doesn't seem to do anything.  I
> > updated the code to do writel() instead so that other platforms
> > besides IA32 would work.
> 
> It actually does work already on IA64.
> Why would a writeb not work?  All it does is set the appropriate byte
> selects.  The platform doesn't care what the register size is, and the
> target (PCI chip) seems to be able to figure it out, so there must be
> something else wrong.
> 
> What is an XScale, btw?

XScale is a CPU branched off from the ARM family by Intel. Mostly you
will see them in PDAs. However, there are other XScale platforms that
are used as network processors or I/O processors.  I probably need to
do some PCI-X sniffing but all I know currently is that after the
writeb the mask register remains unchanged. However if I do a writel
it changes. So currently if I load the driver as it is, I get
infinitely interrupt calls because the irq isn't masked and the irq
handler doesn't know what to do with the interrupt. Since this isn't
really a fast path routine anyhow, wouldn't it be better to make all
platforms happy?
 
> > After getting that working I noticed that the last_ctl value gets
> > changed back by the LIBATA core and the Vitesse SATA driver depends on
> > the previous value in order to unmask the interrupt. So it seems that
> > there's no need to set/unset the interrupt masks in the first place if
> > we set the CTL register with ATA_NIEN directly, which I believe is
> > probably the way LIBATA intended the driver to do. After doing that
> > everything seems to be working great. So I suppose
> > vsc_intr_mask_update() is no longer needed probably.
> 
> The chip specification says not to set it that way.  Are you using
> the chip in DPA mode or PCI-IDE mode?  The driver assumes the former.  I
> don't know why anyone would use the chip in PCI-IDE mode.

Hmmm...I did not see it in the spec. Can you provide the pointer to
the spec please? Thanks! I am using it in DPA mode. If you are in
PCI-IDE mode, all registers would reside in different BARs and the
card wouldn't work anyways with that driver. I copied the section from
sata_svw.c. And it seems to work just fine. I don't see how the
previous method would be working. In vsc_sata_tf_load a compare is
done between the tf->ctl and the ap->last_ctl. However, if I do it
with the current method, I continue to see ap->last_ctl with ATA_NIEN
cleared even though the previous value was set. Therefore the driver
never unmask the IDE interrupt and thus hang.  What happened was, host
1, dev 0 was probed and was okay. Then it attempted to probe host 1,
dev 1, which of course does not exist, so at exit, ata_irq_on() was
called. With that the last_ctl was reset with ATA_NIEN cleared.
Therefore when the driver does the compare of ctl with last_ctl, it
sees no difference, and therefore interrupt mask never changed.
 
> 
> > --- linux-2.6.8.1-iop3/drivers/scsi/sata_vsc.c        2004-08-14 03:55:33.000000000 -0700
> > +++ dj_bktree-2.6.8.1/drivers/scsi/sata_vsc.c 2004-09-17 15:43:12.688212928 -0700
> > @@ -80,6 +80,7 @@
> >
> >  static void vsc_intr_mask_update(struct ata_port *ap, u8 ctl)
> >  {
> > +#if 0
> >       unsigned long mask_addr;
> >       u8 mask;
> >
> > @@ -91,6 +92,26 @@
> >       else
> >               mask &= 0x7F;
> >       writeb(mask, mask_addr);
> > +#endif
> > +    u32 mask = 0;
> > +    u32 regval;
> > +
> > +    regval = readl(ap->host_set->mmio_base + VSC_SATA_INT_MASK_OFFSET);
> > +
> > +
> > +    mask = (0x80 << (8 * ap->port_no));
> > +
> > +    if(ctl & ATA_NIEN)
> > +    {
> > +        mask = ~mask;
> > +        mask = regval & mask;
> > +    }
> > +    else
> > +    {
> > +        mask = regval | mask;
> > +    }
> > +
> > +    writel(mask, ap->host_set->mmio_base + VSC_SATA_INT_MASK_OFFSET);
> >  }
> >
> >
> > @@ -105,8 +126,10 @@
> >        * However, if ATA_NIEN is changed, then we need to change the interrupt register.
> >        */
> >       if ((tf->ctl & ATA_NIEN) != (ap->last_ctl & ATA_NIEN)) {
> > +        writeb(tf->ctl, ioaddr->ctl_addr);
> >               ap->last_ctl = tf->ctl;
> > -             vsc_intr_mask_update(ap, tf->ctl & ATA_NIEN);
> > +        ata_wait_idle(ap);
> > +//      vsc_intr_mask_update(ap, tf->ctl & ATA_NIEN);
> >       }
> >       if (is_addr && (tf->flags & ATA_TFLAG_LBA48)) {
> >               writew(tf->feature | (((u16)tf->hob_feature) << 8), ioaddr->feature_addr);
> > @@ -330,6 +353,19 @@
> >
> >       pci_set_master(pdev);
> >
> > +    /* set per port LEDs */
> > +    {
> > +        unsigned long mask;
> > +        unsigned int regval;
> > +
> > +        set_bit(28, &mask);
> > +        mask = ~mask;
> > +
> > +        pci_read_config_dword(pdev, 0x98, &regval);
> > +        regval = regval & mask;
> > +        pci_write_config_dword(pdev, 0x98, regval);
> > +    }
> > +
> 
> That's a somewhat wordy way of doing it.  You could just do:
> 
>         pci_read_config_dword(pdev, 0x98, &regval);
>         regval = regval & ~(1 << 28)
>         pci_write_config_dword(pdev, 0x98, regval);
> 
> and since the driver is DPA only, we can just do:
> 
>         pci_write_config_dword(pdev, 0x98, 0);
> 
> If anyone ever adds PCI-IDE support, then we're probably before
> the point that we're trying to select channel 0/1, so I think that's
> still safe.
> 
> >       /* FIXME: check ata_device_add return value */
> >       ata_device_add(probe_ent);
> >       kfree(probe_ent);
> 
> Also, jgarzik is the maintainer of this driver.  I'll cc him.
> 
> jeremy
>

Re: [PATCH] updates to Vitesse SATA driver

[Dave]

On Wed, 22 Sep 2004 08:59:06 -0700, Dave <dave.jiang@gmail.com> wrote:
> On Tue, 21 Sep 2004 19:06:14 -0700, Jeremy Higdon <jeremy@sgi.com > wrote:
> > On Tue, Sep 21, 2004 at 02:16:46PM -0700, Dave wrote:
> > > After getting that working I noticed that the last_ctl value gets
> > > changed back by the LIBATA core and the Vitesse SATA driver depends on
> > > the previous value in order to unmask the interrupt. So it seems that
> > > there's no need to set/unset the interrupt masks in the first place if
> > > we set the CTL register with ATA_NIEN directly, which I believe is
> > > probably the way LIBATA intended the driver to do. After doing that
> > > everything seems to be working great. So I suppose
> > > vsc_intr_mask_update() is no longer needed probably.
> >
> > The chip specification says not to set it that way.  Are you using
> > the chip in DPA mode or PCI-IDE mode?  The driver assumes the former.  I
> > don't know why anyone would use the chip in PCI-IDE mode.
> 
> Hmmm...I did not see it in the spec. Can you provide the pointer to
> the spec please? Thanks! I am using it in DPA mode. If you are in
> PCI-IDE mode, all registers would reside in different BARs and the
> card wouldn't work anyways with that driver. I copied the section from
> sata_svw.c. And it seems to work just fine. I don't see how the
> previous method would be working. In vsc_sata_tf_load a compare is
> done between the tf->ctl and the ap->last_ctl. However, if I do it
> with the current method, I continue to see ap->last_ctl with ATA_NIEN
> cleared even though the previous value was set. Therefore the driver
> never unmask the IDE interrupt and thus hang.  What happened was, host
> 1, dev 0 was probed and was okay. Then it attempted to probe host 1,
> dev 1, which of course does not exist, so at exit, ata_irq_on() was
> called. With that the last_ctl was reset with ATA_NIEN cleared.
> Therefore when the driver does the compare of ctl with last_ctl, it
> sees no difference, and therefore interrupt mask never changed.

I see what you mean by the spec regarding the ATA_NIEN bit. ATA_NIEN
is reserved bit in DPA mode. I guess it still works. But to do the
things the right way now this becomes a problem since as I mentioned
above, libata-core can change the last_ctl value without the driver's
knowledge. So really we cannot always depend on last_ctl to be the
last_ctl the driver has set. I'm not exactly sure yet why the driver
works on IA in the current state. Everything tells me it shouldn't....
Unless there's something that I'm suppose to configure to force it not
probe device 1 since in SATA there's never device 1?

Re: [PATCH] updates to Vitesse SATA driver

[Jeremy Higdon]

On Wed, Sep 22, 2004 at 10:09:03AM -0700, Dave wrote:
> On Wed, 22 Sep 2004 08:59:06 -0700, Dave <dave.jiang@gmail.com> wrote:
> > On Tue, 21 Sep 2004 19:06:14 -0700, Jeremy Higdon <jeremy@sgi.com > wrote:
> > > On Tue, Sep 21, 2004 at 02:16:46PM -0700, Dave wrote:
> > > > After getting that working I noticed that the last_ctl value gets
> > > > changed back by the LIBATA core and the Vitesse SATA driver depends on
> > > > the previous value in order to unmask the interrupt. So it seems that
> > > > there's no need to set/unset the interrupt masks in the first place if
> > > > we set the CTL register with ATA_NIEN directly, which I believe is
> > > > probably the way LIBATA intended the driver to do. After doing that
> > > > everything seems to be working great. So I suppose
> > > > vsc_intr_mask_update() is no longer needed probably.
> > >
> > > The chip specification says not to set it that way.  Are you using
> > > the chip in DPA mode or PCI-IDE mode?  The driver assumes the former.  I
> > > don't know why anyone would use the chip in PCI-IDE mode.
> > 
> > Hmmm...I did not see it in the spec. Can you provide the pointer to
> > the spec please? Thanks! I am using it in DPA mode. If you are in
> > PCI-IDE mode, all registers would reside in different BARs and the
> > card wouldn't work anyways with that driver. I copied the section from
> > sata_svw.c. And it seems to work just fine. I don't see how the
> > previous method would be working. In vsc_sata_tf_load a compare is
> > done between the tf->ctl and the ap->last_ctl. However, if I do it
> > with the current method, I continue to see ap->last_ctl with ATA_NIEN
> > cleared even though the previous value was set. Therefore the driver
> > never unmask the IDE interrupt and thus hang.  What happened was, host
> > 1, dev 0 was probed and was okay. Then it attempted to probe host 1,
> > dev 1, which of course does not exist, so at exit, ata_irq_on() was
> > called. With that the last_ctl was reset with ATA_NIEN cleared.
> > Therefore when the driver does the compare of ctl with last_ctl, it
> > sees no difference, and therefore interrupt mask never changed.
> 
> I see what you mean by the spec regarding the ATA_NIEN bit. ATA_NIEN
> is reserved bit in DPA mode. I guess it still works. But to do the
> things the right way now this becomes a problem since as I mentioned
> above, libata-core can change the last_ctl value without the driver's
> knowledge. So really we cannot always depend on last_ctl to be the
> last_ctl the driver has set. I'm not exactly sure yet why the driver
> works on IA in the current state. Everything tells me it shouldn't....
> Unless there's something that I'm suppose to configure to force it not
> probe device 1 since in SATA there's never device 1?

I can tell you that it works fine on our Altix (IA64 Numa) boxes with
1 - 4 drives attached.  I don't recall exactly how we tell libata that
there is only one device per port -- there must be a parameter somewhere :-)
I do remember that we had to explicitly tell the IDE layer about that,
for the IDE version of the driver in lk2.4.

jeremy

Re: [PATCH] updates to Vitesse SATA driver

[Dave]

On Wed, 22 Sep 2004 13:13:28 -0700, Jeremy Higdon <jeremy@sgi.com> wrote:
> 
> 
> On Wed, Sep 22, 2004 at 10:09:03AM -0700, Dave wrote:
> > On Wed, 22 Sep 2004 08:59:06 -0700, Dave <dave.jiang@gmail.com > wrote:
> > > On Tue, 21 Sep 2004 19:06:14 -0700, Jeremy Higdon <jeremy@sgi.com > wrote:
> > > > On Tue, Sep 21, 2004 at 02:16:46PM -0700, Dave wrote:
> > > > > After getting that working I noticed that the last_ctl value gets
> > > > > changed back by the LIBATA core and the Vitesse SATA driver depends on
> > > > > the previous value in order to unmask the interrupt. So it seems that
> > > > > there's no need to set/unset the interrupt masks in the first place if
> > > > > we set the CTL register with ATA_NIEN directly, which I believe is
> > > > > probably the way LIBATA intended the driver to do. After doing that
> > > > > everything seems to be working great. So I suppose
> > > > > vsc_intr_mask_update() is no longer needed probably.
> > > >
> > > > The chip specification says not to set it that way.  Are you using
> > > > the chip in DPA mode or PCI-IDE mode?  The driver assumes the former.  I
> > > > don't know why anyone would use the chip in PCI-IDE mode.
> > >
> > > Hmmm...I did not see it in the spec. Can you provide the pointer to
> > > the spec please? Thanks! I am using it in DPA mode. If you are in
> > > PCI-IDE mode, all registers would reside in different BARs and the
> > > card wouldn't work anyways with that driver. I copied the section from
> > > sata_svw.c. And it seems to work just fine. I don't see how the
> > > previous method would be working. In vsc_sata_tf_load a compare is
> > > done between the tf->ctl and the ap->last_ctl. However, if I do it
> > > with the current method, I continue to see ap->last_ctl with ATA_NIEN
> > > cleared even though the previous value was set. Therefore the driver
> > > never unmask the IDE interrupt and thus hang.  What happened was, host
> > > 1, dev 0 was probed and was okay. Then it attempted to probe host 1,
> > > dev 1, which of course does not exist, so at exit, ata_irq_on() was
> > > called. With that the last_ctl was reset with ATA_NIEN cleared.
> > > Therefore when the driver does the compare of ctl with last_ctl, it
> > > sees no difference, and therefore interrupt mask never changed.
> >
> > I see what you mean by the spec regarding the ATA_NIEN bit. ATA_NIEN
> > is reserved bit in DPA mode. I guess it still works. But to do the
> > things the right way now this becomes a problem since as I mentioned
> > above, libata-core can change the last_ctl value without the driver's
> > knowledge. So really we cannot always depend on last_ctl to be the
> > last_ctl the driver has set. I'm not exactly sure yet why the driver
> > works on IA in the current state. Everything tells me it shouldn't....
> > Unless there's something that I'm suppose to configure to force it not
> > probe device 1 since in SATA there's never device 1?
> 
> I can tell you that it works fine on our Altix (IA64 Numa) boxes with
> 1 - 4 drives attached.  I don't recall exactly how we tell libata that
> there is only one device per port -- there must be a parameter somewhere :-)
> I do remember that we had to explicitly tell the IDE layer about that,
> for the IDE version of the driver in lk2.4.
> 
> jeremy
> 

Jeff, 
would it make sense in this case to provide a callback from the
libata-core such that when libata calls the routine to clear/mask the
interrupt, the driver can also do any fixups that it needs? I would
think this would help those HBAs that do not support ATA_NIEN as such
of a case here. True we can perhaps somehow force the driver to not
scan the second device, but what happens when SATA port expanders come
out? Will that be an issue then? (Not that I've thought much about
that....).

-- 
-= Dave =-

Re: [PATCH] updates to Vitesse SATA driver

[Jeremy Higdon]

On Wed, Sep 22, 2004 at 08:59:06AM -0700, Dave wrote:
> > It actually does work already on IA64.
> > Why would a writeb not work?  All it does is set the appropriate byte
> > selects.  The platform doesn't care what the register size is, and the
> > target (PCI chip) seems to be able to figure it out, so there must be
> > something else wrong.
> > 
> > What is an XScale, btw?
> 
> XScale is a CPU branched off from the ARM family by Intel. Mostly you
> will see them in PDAs. However, there are other XScale platforms that
> are used as network processors or I/O processors.  I probably need to
> do some PCI-X sniffing but all I know currently is that after the
> writeb the mask register remains unchanged. However if I do a writel
> it changes. So currently if I load the driver as it is, I get
> infinitely interrupt calls because the irq isn't masked and the irq
> handler doesn't know what to do with the interrupt. Since this isn't
> really a fast path routine anyhow, wouldn't it be better to make all
> platforms happy?

The problem you have is that if you use writel, I think you'll be
subject to race conditions, unless you single thread updates of
the mask register among the different ports.  I don't know that they
are today (Jeff would), because different ports usually have separate
registers.

jeremy

Re: [PATCH] updates to Vitesse SATA driver

[Jeff Garzik]

So where did the discussion on this patch land?

Re: [PATCH] updates to Vitesse SATA driver

[Dave]

On Wed, 29 Sep 2004 18:19:40 -0400, Jeff Garzik <jgarzik@pobox.com> wrote:
> So where did the discussion on this patch land?
> 
> 

Jeff,

I believe waiting for you to comment on whether to put in a hook for
controllers that do not support ATA_NIEN so it can mask/unmask IRQ
with a fixup.  Even though the spec for this particular SATA
controller says the bit is reserved, it seems to work just fine. From
my understanding, the registers are actually on the drives, and the
ones we write to the HBAs are just shadow registers right? I suppose
either we can use the "undocumented" reserved bit for ATA_NIEN, or
provide some sort of special hook in lib_ata core to clear the
interrupt bit....

Obviosly if it is working on IA32 or IA64, this code must be only
scanning device 0 per port and not device 1. Not sure why it is only
scanning 1 device on IA platforms, but both on XScale. Either way we
have a corner case here that should be addressed....


-- 
-= Dave =-

Software Engineer - Advanced Development Engineering Team 
Storage Component Division - Intel Corp. 
mailto://dave.jiang @ intel
----
The views expressed in this email are
mine alone and do not necessarily 
reflect the views of my employer
(Intel Corp.).

Re: [PATCH] updates to Vitesse SATA driver

[Jeremy Higdon]

On Wed, Sep 29, 2004 at 03:32:34PM -0700, Dave wrote:
> On Wed, 29 Sep 2004 18:19:40 -0400, Jeff Garzik <jgarzik@pobox.com> wrote:
> > So where did the discussion on this patch land?

The only part that made sense to me was the LED patch, which should
just be a single line of code.

> Jeff,
> 
> I believe waiting for you to comment on whether to put in a hook for
> controllers that do not support ATA_NIEN so it can mask/unmask IRQ
> with a fixup.  Even though the spec for this particular SATA
> controller says the bit is reserved, it seems to work just fine. From
> my understanding, the registers are actually on the drives, and the
> ones we write to the HBAs are just shadow registers right? I suppose
> either we can use the "undocumented" reserved bit for ATA_NIEN, or
> provide some sort of special hook in lib_ata core to clear the
> interrupt bit....

I don't think we need this.  In vsc_sata_tf_load(), we see if the
ATA_NIEN bit has changed and call vsc_intr_mask_update() as
necessary.

> Obviosly if it is working on IA32 or IA64, this code must be only
> scanning device 0 per port and not device 1. Not sure why it is only
> scanning 1 device on IA platforms, but both on XScale. Either way we
> have a corner case here that should be addressed....

Jeff, I don't remember how libata knows how many devices a port
can have.  Which field is it?

jeremy

Re: [PATCH] updates to Vitesse SATA driver

[Jeff Garzik]

Jeremy Higdon wrote:
> On Wed, Sep 29, 2004 at 03:32:34PM -0700, Dave wrote:
> 
>>On Wed, 29 Sep 2004 18:19:40 -0400, Jeff Garzik <jgarzik@pobox.com> wrote:
>>
>>>So where did the discussion on this patch land?
> 
> 
> The only part that made sense to me was the LED patch, which should
> just be a single line of code.

So maybe you or Dave could be convinced to roll that into a separate 
patch, since we have consensus on that item?



>>I believe waiting for you to comment on whether to put in a hook for
>>controllers that do not support ATA_NIEN so it can mask/unmask IRQ
>>with a fixup.  Even though the spec for this particular SATA
>>controller says the bit is reserved, it seems to work just fine. From
>>my understanding, the registers are actually on the drives, and the
>>ones we write to the HBAs are just shadow registers right? I suppose
>>either we can use the "undocumented" reserved bit for ATA_NIEN, or
>>provide some sort of special hook in lib_ata core to clear the
>>interrupt bit....
> 
> 
> I don't think we need this.  In vsc_sata_tf_load(), we see if the
> ATA_NIEN bit has changed and call vsc_intr_mask_update() as
> necessary.
> 
> 
>>Obviosly if it is working on IA32 or IA64, this code must be only
>>scanning device 0 per port and not device 1. Not sure why it is only
>>scanning 1 device on IA platforms, but both on XScale. Either way we
>>have a corner case here that should be addressed....
> 
> 
> Jeff, I don't remember how libata knows how many devices a port
> can have.  Which field is it?

By default it only scans device 0, since SATA is a point-to-point 
connection where there is no possibility for more than one device :)

However, some hardware chooses to emulate master/slave mode.  Use of 
this, particularly on Vitesse/Intel, is highly discouraged in favor of 
DPA mode.  I would prefer to not support master/slave mode at all.

Technically speaking, one sets the ATA_FLAG_SLAVE_POSS flag to tell the 
libata core to scan for device 1.

If we are talking about Port Multipliers, those aren't supported at all yet.

	Jeff

Re: [PATCH] updates to Vitesse SATA driver

[Jeremy Higdon]

On Wed, Sep 29, 2004 at 11:28:19PM -0400, Jeff Garzik wrote:
> >>On Wed, 29 Sep 2004 18:19:40 -0400, Jeff Garzik <jgarzik@pobox.com> wrote:
> >>
> >>>So where did the discussion on this patch land?
> >
> >
> >The only part that made sense to me was the LED patch, which should
> >just be a single line of code.
> 
> So maybe you or Dave could be convinced to roll that into a separate 
> patch, since we have consensus on that item?

Since I'm just as happy without it, I'll let Dave do it  :-)

> >>Obviosly if it is working on IA32 or IA64, this code must be only
> >>scanning device 0 per port and not device 1. Not sure why it is only
> >>scanning 1 device on IA platforms, but both on XScale. Either way we
> >>have a corner case here that should be addressed....
> >
> >
> >Jeff, I don't remember how libata knows how many devices a port
> >can have.  Which field is it?
> 
> By default it only scans device 0, since SATA is a point-to-point 
> connection where there is no possibility for more than one device :)
> 
> However, some hardware chooses to emulate master/slave mode.  Use of 
> this, particularly on Vitesse/Intel, is highly discouraged in favor of 
> DPA mode.  I would prefer to not support master/slave mode at all.
> 
> Technically speaking, one sets the ATA_FLAG_SLAVE_POSS flag to tell the 
> libata core to scan for device 1.
> 
> If we are talking about Port Multipliers, those aren't supported at all yet.
> 
> 	Jeff

So there's something amiss with the XScale, then, it appears.  They
also were having trouble with byte writes to the interrupt mask
register.  That also seems to be a problem unique to that platform.

jeremy

[PATCH 2.6.9-rc2] per-port LED control for sata_vsc

[Jeremy Higdon]

On Wed, Sep 29, 2004 at 11:28:19PM -0400, Jeff Garzik wrote:
> 
> So maybe you or Dave could be convinced to roll that into a separate 
> patch, since we have consensus on that item?

I changed my mind.  Here's the patch.  I decided that I wanted a
big comment about this.

There may be a small offset to the latest version of this driver,
Jeff.  If it's a problem, let me know and I'll regenerate.


signed-off-by: Jeremy Higdon <jeremy@sgi.com>

===== drivers/scsi/sata_vsc.c 1.19 vs edited =====
--- 1.19/drivers/scsi/sata_vsc.c	2004-09-15 23:45:15 -07:00
+++ edited/drivers/scsi/sata_vsc.c	2004-09-29 21:12:40 -07:00
@@ -333,6 +333,14 @@
 
 	pci_set_master(pdev);
 
+	/* 
+	 * Config offset 0x98 is "Extended Control and Status Register 0"
+	 * Default value is (1 << 28).  All bits except bit 28 are reserved in
+	 * DPA mode.  If bit 28 is set, LED 0 reflects all ports' activity.
+	 * If bit 28 is clear, each port has its own LED.
+	 */
+	pci_write_config_dword(pdev, 0x98, 0);
+
 	/* FIXME: check ata_device_add return value */
 	ata_device_add(probe_ent);
 	kfree(probe_ent);

Re: [PATCH] updates to Vitesse SATA driver

[Dave]

On Wed, 29 Sep 2004 23:28:19 -0400, Jeff Garzik <jgarzik@pobox.com> wrote:

> By default it only scans device 0, since SATA is a point-to-point
> connection where there is no possibility for more than one device :)
> 
> However, some hardware chooses to emulate master/slave mode.  Use of
> this, particularly on Vitesse/Intel, is highly discouraged in favor of
> DPA mode.  I would prefer to not support master/slave mode at all.

I am using the DPA mode. There is no way to even have the controller
work in legacy mode with the sata driver. Although the generic IDE
driver can be tweaked to use that. And yes I agree, no point to
support the master/slave mode. It was done for backward compatibility
for whatever reason....
 
> Technically speaking, one sets the ATA_FLAG_SLAVE_POSS flag to tell the
> libata core to scan for device 1.
> 

For some odd reason we are scanning device 1 on XScale IOP platform
with the exact same driver code. I'll have to investigate some more
and see why it's doing that.

-- 
-= Dave =-

Software Engineer - Advanced Development Engineering Team 
Storage Component Division - Intel Corp. 
mailto://dave.jiang @ intel
----
The views expressed in this email are
mine alone and do not necessarily 
reflect the views of my employer
(Intel Corp.).

Re: [PATCH] updates to Vitesse SATA driver

[Dave]

On Thu, 30 Sep 2004 09:17:09 -0700, Dave <dave.jiang@gmail.com> wrote:
> On Wed, 29 Sep 2004 23:28:19 -0400, Jeff Garzik <jgarzik@pobox.com > wrote:
> 
> > Technically speaking, one sets the ATA_FLAG_SLAVE_POSS flag to tell the
> > libata core to scan for device 1.
> >
> 
> For some odd reason we are scanning device 1 on XScale IOP platform
> with the exact same driver code. I'll have to investigate some more
> and see why it's doing that.
> 

Okay, I must be not seeing something obvious. The ATA_FLAG_SLAVE_POSS
flag is not set. But I only see it being checked during reset. When
ata_bus_probe is called, it probes for ATA_MAX_DEVICES, which is both
dev 0 and dev 1. Thus dev 1 fails of course, and the core changes the
ctl without the sata driver knowing and causes interrupt mask to
remain set.....

-- 
-= Dave =-

Software Engineer - Advanced Development Engineering Team 
Storage Component Division - Intel Corp. 
mailto://dave.jiang @ intel
----
The views expressed in this email are
mine alone and do not necessarily 
reflect the views of my employer
(Intel Corp.).

Re: [PATCH] updates to Vitesse SATA driver

[Jeff Garzik]

Dave wrote:
> I believe waiting for you to comment on whether to put in a hook for
> controllers that do not support ATA_NIEN so it can mask/unmask IRQ
> with a fixup.  Even though the spec for this particular SATA
> controller says the bit is reserved, it seems to work just fine. From
> my understanding, the registers are actually on the drives, and the
> ones we write to the HBAs are just shadow registers right? I suppose
> either we can use the "undocumented" reserved bit for ATA_NIEN, or
> provide some sort of special hook in lib_ata core to clear the
> interrupt bit....


Close.

The shadow registers are an entity that exists solely on the host 
controller.  The entire register block is converted internally by the 
silicon to a SATA Host-to-Device Register FIS, and the FIS is what is 
sent to the SATA device.

As you can see in the SATA specification (www.serialata.org), each SATA 
FIS contains an interrupt bit.  The decision about when and how to set 
this interrupt bit is highly controller-specific.  Sometimes it is 
mapped to nIEN in the Device Control register.  Sometimes it is 
completely ignored (as with AHCI), and the controller sends interrupts 
based on various bits set in an interrupt-status register.

In any case, it is still not clear what you are trying to achieve :(

In what way is sata_vsc's current ATA_NIEN handling code deficient?

	Jeff