updates to Vitesse SATA driver

Signed-off-by Dave Jiang ([email protected])

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.

Signature · 9 lines

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

drivers/scsi/sata_vsc.c +27 -1

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

Quoted 10 lines · Dave

Signed-off-by Dave Jiang ([email protected] )

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?

Quoted 8 lines

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.

Quoted 57 lines

drivers/scsi/sata_vsc.c 2004-09-17 15:43:12.688212928 -0700 renamed from drivers/scsi/sata_vsc.c 2004-08-14 03:55:33.000000000 -0700 +27 -1

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

drivers/scsi/sata_vsc.c 2004-09-17 15:43:12.688212928 -0700 line 368

Quoted 10 lines

drivers/scsi/sata_vsc.c 2004-09-17 15:43:12.688212928 -0700 renamed from drivers/scsi/sata_vsc.c 2004-08-14 03:55:33.000000000 -0700 +10 -0

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

drivers/scsi/sata_vsc.c 2004-09-17 15:43:12.688212928 -0700 line 371

Quoted 3 lines

drivers/scsi/sata_vsc.c 2004-09-17 15:43:12.688212928 -0700 renamed from drivers/scsi/sata_vsc.c 2004-08-14 03:55:33.000000000 -0700 +0 -0

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

Quoted 10 lines · Jeremy Higdon

Quoted 8 lines · Dave

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.

Quoted 3 lines

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.

Quoted 7 lines

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?

Quoted 15 lines · Dave

Quoted 10 lines · Jeremy Higdon

Quoted 8 lines · Dave

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.

Quoted 3 lines

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.

Quoted 15 lines

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?

Quoted 32 lines · Dave

Quoted 15 lines · Dave

Quoted 10 lines · Jeremy Higdon

Quoted 8 lines · Dave

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.

Quoted 3 lines

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.

Quoted 15 lines

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.

Quoted 9 lines

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

So where did the discussion on this patch land?

Quoted 1 line · Jeff Garzik

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

Quoted 3 lines · Dave

Quoted 1 line · Jeff Garzik

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.

Quoted 9 lines

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.

Quoted 4 lines

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?

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

Quoted 7 lines

Quoted 3 lines · Jeff Garzik

Quoted 2 lines

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?

Quoted 8 lines

Quoted 4 lines

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 maybe you or Dave could be convinced to roll that into a separate patch, since we have consensus on that item?

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.

Quoted 3 lines · Jeff Garzik

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.