Re: [PATCH linux-2.6.13-rc3] SATA: rewritten sil24 driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Tejun; I'm the guy at Google working on SATA drivers (port multipliers right now). As soon as I can (next week perhaps, I'll start looking at the driver you wrote. From what I can see, it looks quite good.



+
+static u8 sil24_check_status(struct ata_port *ap)
+{
+	return ATA_DRDY;
+}
+
+static u8 sil24_check_err(struct ata_port *ap)
+{
+	return 0;
+}

For these two functions, we need to grab the values for these registers from the D2H Register FIS. These should not be constant values, they are used in probing.


 Sadly I don't know where the values are.  The original driver seems
to assume that taskfile registers are overlayed with PORT_PRB, but
they are not.  Values are bogus there.  Again, in need of hardware
docs here.

The original driver is broken. Taskfile registers have to be read back from the returned FIS block after a command completion.

Hmmmm, perhaps libata should provide an ata_fis_to_tf() function that examines a FIS block, confirms that it's a device-to-host type FIS, and read the taskfile registers back out.




 The original rewritten sil24 driver against NCQ helpers had simple
status register emulation using normal/error completion interrupt.  I
don't know why I stripped that while converting the driver over
vanilla libata (sorry).  I'll restore it.  It's not good, but it
correctly indicates error on error.

It's still better than what we have.



+static void sil24_phy_reset(struct ata_port *ap)
+{
+	__sata_phy_reset(ap);
+	/*
+	 * No ATAPI yet.  Just unconditionally indicate ATA device.
+	 * If ATAPI device is attached, it will fail ATA_CMD_ID_ATA
+	 * and libata core will ignore the device.
+	 */
+	if (!(ap->flags & ATA_FLAG_PORT_DISABLED))
+		ap->device[0].class = ATA_DEV_ATA;
+}

We need to use the standard probing code to figure this out. ata_dev_classify(), etc.



 Again, the problem here is that I don't know how to access the
signature values after reset.

Again, you would need to fetch them from the returned FIS structure. Here's a code fragment derived from SiI's issue_soft_reset() function:

	struct Port_Registers *port_base = yadayada;
	struct sil_port_priv *pp = ap->private_data;
	struct Port_Registers *PR;	/* in memory */
	dma_addr_t paddr = pp->pc_dma; /* physical address base */

	PR = (struct Port_Registers *) (&pp->pc->mregs);
	port_base = yadayada;
	slot = 0;
	slotp = &PR->Slot[slot];
	memset(&slotp->Prb, 0, sizeof(slotp->Prb));
	slotp->Prb.Control = 0x80;		/* soft reset */
	slotp->Prb.FisType == 0;
	writel(paddr, &port_base->CmdActivate[slot].s.ActiveLow);
	if (!sil_wait_for_completion(port_base)) {
		/* timeout or error */
		return ATA_DEV_NONE;
	} else {
		/* Examine slot for taskfile registers */
		slotp = port_base->Slot[slot];
		if (slotp->Prb.FisType != 0x34 &&
		    slotp->Prb.FisType != 0x5F) {
			/* WTF?  Wrong FIS Type */
			return ATA_DEV_NONE;
		}
		if (slotp->Prb.CylLow == 0 &&
		    slotp->Prb.CylHigh == 0)
			return ATA_DEV_ATA;
		if (slotp->Prb.CylLow == 0x14 &&
		    slotp->Prb.CylHigh == 0xEB)
			return ATA_DEV_ATAPI;
		if (slotp->Prb.CylLow == 0x69 &&
		    slotp->Prb.CylHigh == 0x96)
			return ATA_DEV_PORT_MULTIPLIER;
		printk(KERN_WARN "unknown ATA device signature %x.%x\n",
			slotp->Prb.CylLow, slotp->Prb.CylHigh);
		return ATA_DEV_NONE;
	}
	


+static void sil24_irq_clear(struct ata_port *ap)
+{
+	/* unused */
+}

we need to fill this in.

I think this will work (adapted from sil_interrupt():

static void sil_irq_clear(struct ata_port *ap)
{
        struct sil_port_priv *pp = ap->private_data;
        struct Port_Registers *port_base = pp->pregs;
	unsigned long port_int;

	port_int  = readl((void *)&port_base->IntStatus);
	writel(port_int, &port_base->IntStatus);
}

I'm assuming that this entry point is expected to clear all interrupts, no?



+	/* Max ~100ms */
+	for (cnt = 0; cnt < 1000; cnt++) {
+		udelay(100);
+		tmp = readl(port + PORT_CTRL_STAT);
+		if (!(tmp & PORT_CS_DEV_RST))
+			break;
+	}

Use mdelay.  We should be able to sleep here?

Either way, we want to avoid long delays like this.

Does mdelay() sleep?  I thought it just called udelay().

At any rate, I think 100ms is only the worst-case delay.

Is it safe to call msleep() at boot time?




+	/* GPIO off */
+	writel(0, host_base + HOST_FLASH_CMD);
+
+	/* Mask interrupts during initialization */
+	writel(0, host_base + HOST_CTRL);

add a readl() to flush this write out to the PCI bus ("PCI posting")



 Sure.  And, out of curiosity, isn't sync unnecessary unless we're
gonna perform some kind of timed waiting following it?  We don't have
any timing requirement after above interrupt masking, do we?


I think we're ok here; the code reads PORT_CTRL_STAT a few lines down; that will flush the write. I don't think there's a race condition involved.





OK, a couple of questions of my own:

Any documentation on NCQ helpers or other related kernel code?

What's the proper way to implement very long delays. I want to implement staggered disk spinup in my port multiplier code. Would mdelay(5000) be terribly anti-social? I'm guessing yes, but then, how do I ensure that no process tries to access the disk until I've spun it up?

Port multiplier spec says that the GSCR[2] register indicates how many ports the port multiplier supports. On my 5-port device, this register reads back as six. Are they counting the control port, or is this device defective?

What causes a disk to spin up? Is it the COMRESET? Soft reset? In either case, it sounds like I need to spin up a drive just to detect it. Not optimal.

	-ed falk
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]
  Powered by Linux