[PATCH 3/5] sil24: use SRST for phy_reset

[PATCH 3/5] sil24: use SRST for phy_reset

[Tejun Heo]

There seems to be no way to obtain device signature from sil24 after
SATA phy reset and SRST is needed anyway for later port multiplier
suppport.  This patch converts sil24_phy_reset to use SRST instaed.

Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work/drivers/scsi/sata_sil24.c
===================================================================
--- work.orig/drivers/scsi/sata_sil24.c	2005-11-16 16:58:01.000000000 +0900
+++ work/drivers/scsi/sata_sil24.c	2005-11-16 17:02:56.000000000 +0900
@@ -333,7 +333,7 @@ static struct ata_port_info sil24_port_i
 	{
 		.sht		= &sil24_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
+				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
 				  ATA_FLAG_PIO_DMA | SIL24_NPORTS2FLAG(4),
 		.pio_mask	= 0x1f,			/* pio0-4 */
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
@@ -344,7 +344,7 @@ static struct ata_port_info sil24_port_i
 	{
 		.sht		= &sil24_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
+				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
 				  ATA_FLAG_PIO_DMA | SIL24_NPORTS2FLAG(2),
 		.pio_mask	= 0x1f,			/* pio0-4 */
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
@@ -355,7 +355,7 @@ static struct ata_port_info sil24_port_i
 	{
 		.sht		= &sil24_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
+				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
 				  ATA_FLAG_PIO_DMA | SIL24_NPORTS2FLAG(1),
 		.pio_mask	= 0x1f,			/* pio0-4 */
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
@@ -415,16 +415,69 @@ static void sil24_tf_read(struct ata_por
 	*tf = pp->tf;
 }
 
+static int sil24_issue_SRST(struct ata_port *ap)
+{
+	void __iomem *port = (void __iomem *)ap->ioaddr.cmd_addr;
+	struct sil24_port_priv *pp = ap->private_data;
+	struct sil24_prb *prb = &pp->cmd_block[0].prb;
+	dma_addr_t paddr = pp->cmd_block_dma;
+	u32 irq_enable, irq_stat;
+	int cnt;
+
+	/* temporarily turn off IRQs during SRST */
+	irq_enable = readl(port + PORT_IRQ_ENABLE_SET);
+	writel(irq_enable, port + PORT_IRQ_ENABLE_CLR);
+
+	/* delay 10 ms */
+	udelay(10000);
+
+	prb->ctrl = PRB_CTRL_SRST;
+	prb->fis[1] = 0; /* no PM yet */
+
+	writel((u32)paddr, port + PORT_CMD_ACTIVATE);
+
+	for (cnt = 0; cnt < 100; cnt++) {
+		irq_stat = readl(port + PORT_IRQ_STAT);
+		writel(irq_stat, port + PORT_IRQ_STAT);		/* clear irq */
+
+		irq_stat >>= PORT_IRQ_RAW_SHIFT;
+		if (irq_stat & (PORT_IRQ_COMPLETE | PORT_IRQ_ERROR))
+			break;
+
+		udelay(1000);
+	}
+
+	/* restore IRQs */
+	writel(irq_enable, port + PORT_IRQ_ENABLE_SET);
+
+	if (!(irq_stat & PORT_IRQ_COMPLETE))
+		return -1;
+
+	/* update TF */
+	sil24_update_tf(ap);
+	return 0;
+}
+
 static void sil24_phy_reset(struct ata_port *ap)
 {
+	struct sil24_port_priv *pp = ap->private_data;
+
 	__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;
+	if (ap->flags & ATA_FLAG_PORT_DISABLED)
+		return;
+
+	if (sil24_issue_SRST(ap) < 0) {
+		printk(KERN_ERR DRV_NAME
+		       " ata%u: SRST failed, disabling port\n", ap->id);
+		ap->ops->port_disable(ap);
+		return;
+	}
+
+	ap->device->class = ata_dev_classify(&pp->tf);
+
+	/* No ATAPI yet */
+	if (ap->device->class == ATA_DEV_ATAPI)
+		ap->ops->port_disable(ap);
 }
 
 static inline void sil24_fill_sg(struct ata_queued_cmd *qc,

Re: [PATCH 3/5] sil24: use SRST for phy_reset

[Jeff Garzik]

Tejun Heo wrote:
> There seems to be no way to obtain device signature from sil24 after
> SATA phy reset and SRST is needed anyway for later port multiplier
> suppport.  This patch converts sil24_phy_reset to use SRST instaed.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> Index: work/drivers/scsi/sata_sil24.c
> ===================================================================
> --- work.orig/drivers/scsi/sata_sil24.c	2005-11-16 16:58:01.000000000 +0900
> +++ work/drivers/scsi/sata_sil24.c	2005-11-16 17:02:56.000000000 +0900
> @@ -333,7 +333,7 @@ static struct ata_port_info sil24_port_i
>  	{
>  		.sht		= &sil24_sht,
>  		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
> -				  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
> +				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
>  				  ATA_FLAG_PIO_DMA | SIL24_NPORTS2FLAG(4),
>  		.pio_mask	= 0x1f,			/* pio0-4 */
>  		.mwdma_mask	= 0x07,			/* mwdma0-2 */
> @@ -344,7 +344,7 @@ static struct ata_port_info sil24_port_i
>  	{
>  		.sht		= &sil24_sht,
>  		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
> -				  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
> +				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
>  				  ATA_FLAG_PIO_DMA | SIL24_NPORTS2FLAG(2),
>  		.pio_mask	= 0x1f,			/* pio0-4 */
>  		.mwdma_mask	= 0x07,			/* mwdma0-2 */
> @@ -355,7 +355,7 @@ static struct ata_port_info sil24_port_i
>  	{
>  		.sht		= &sil24_sht,
>  		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
> -				  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
> +				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
>  				  ATA_FLAG_PIO_DMA | SIL24_NPORTS2FLAG(1),
>  		.pio_mask	= 0x1f,			/* pio0-4 */
>  		.mwdma_mask	= 0x07,			/* mwdma0-2 */

This is not only OK, it is preferred, since 3124 port reset issues 
COMRESET for us.


> @@ -415,16 +415,69 @@ static void sil24_tf_read(struct ata_por
>  	*tf = pp->tf;
>  }
>  
> +static int sil24_issue_SRST(struct ata_port *ap)
> +{
> +	void __iomem *port = (void __iomem *)ap->ioaddr.cmd_addr;
> +	struct sil24_port_priv *pp = ap->private_data;
> +	struct sil24_prb *prb = &pp->cmd_block[0].prb;
> +	dma_addr_t paddr = pp->cmd_block_dma;
> +	u32 irq_enable, irq_stat;
> +	int cnt;
> +
> +	/* temporarily turn off IRQs during SRST */
> +	irq_enable = readl(port + PORT_IRQ_ENABLE_SET);
> +	writel(irq_enable, port + PORT_IRQ_ENABLE_CLR);
> +
> +	/* delay 10 ms */
> +	udelay(10000);

NAK:
* long udelay (always NAK'd), as they chew processor and lock out other 
things, for no reason.
* passing 1000 or more to udelay() means you should use mdelay() instead
* in this case, msleep() works just fine
* I don't see why this delay is needed at all?


> +	prb->ctrl = PRB_CTRL_SRST;
> +	prb->fis[1] = 0; /* no PM yet */
> +
> +	writel((u32)paddr, port + PORT_CMD_ACTIVATE);
> +
> +	for (cnt = 0; cnt < 100; cnt++) {
> +		irq_stat = readl(port + PORT_IRQ_STAT);
> +		writel(irq_stat, port + PORT_IRQ_STAT);		/* clear irq */
> +
> +		irq_stat >>= PORT_IRQ_RAW_SHIFT;
> +		if (irq_stat & (PORT_IRQ_COMPLETE | PORT_IRQ_ERROR))
> +			break;
> +
> +		udelay(1000);

NAK:
* long udelay (always NAK'd)
* passing 1000 or more to udelay() means you should use mdelay() instead
* in this case, msleep() works just fine

Re: [PATCH 3/5] sil24: use SRST for phy_reset

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> There seems to be no way to obtain device signature from sil24 after
>> SATA phy reset and SRST is needed anyway for later port multiplier
>> suppport.  This patch converts sil24_phy_reset to use SRST instaed.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>> Index: work/drivers/scsi/sata_sil24.c
>> ===================================================================
>> --- work.orig/drivers/scsi/sata_sil24.c    2005-11-16 
>> 16:58:01.000000000 +0900
>> +++ work/drivers/scsi/sata_sil24.c    2005-11-16 17:02:56.000000000 +0900
>> @@ -333,7 +333,7 @@ static struct ata_port_info sil24_port_i
>>      {
>>          .sht        = &sil24_sht,
>>          .host_flags    = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
>> -                  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
>> +                  ATA_FLAG_SRST | ATA_FLAG_MMIO |
>>                    ATA_FLAG_PIO_DMA | SIL24_NPORTS2FLAG(4),
>>          .pio_mask    = 0x1f,            /* pio0-4 */
>>          .mwdma_mask    = 0x07,            /* mwdma0-2 */
>> @@ -344,7 +344,7 @@ static struct ata_port_info sil24_port_i
>>      {
>>          .sht        = &sil24_sht,
>>          .host_flags    = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
>> -                  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
>> +                  ATA_FLAG_SRST | ATA_FLAG_MMIO |
>>                    ATA_FLAG_PIO_DMA | SIL24_NPORTS2FLAG(2),
>>          .pio_mask    = 0x1f,            /* pio0-4 */
>>          .mwdma_mask    = 0x07,            /* mwdma0-2 */
>> @@ -355,7 +355,7 @@ static struct ata_port_info sil24_port_i
>>      {
>>          .sht        = &sil24_sht,
>>          .host_flags    = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
>> -                  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
>> +                  ATA_FLAG_SRST | ATA_FLAG_MMIO |
>>                    ATA_FLAG_PIO_DMA | SIL24_NPORTS2FLAG(1),
>>          .pio_mask    = 0x1f,            /* pio0-4 */
>>          .mwdma_mask    = 0x07,            /* mwdma0-2 */
> 
> 
> This is not only OK, it is preferred, since 3124 port reset issues 
> COMRESET for us.
> 
> 
>> @@ -415,16 +415,69 @@ static void sil24_tf_read(struct ata_por
>>      *tf = pp->tf;
>>  }
>>  
>> +static int sil24_issue_SRST(struct ata_port *ap)
>> +{
>> +    void __iomem *port = (void __iomem *)ap->ioaddr.cmd_addr;
>> +    struct sil24_port_priv *pp = ap->private_data;
>> +    struct sil24_prb *prb = &pp->cmd_block[0].prb;
>> +    dma_addr_t paddr = pp->cmd_block_dma;
>> +    u32 irq_enable, irq_stat;
>> +    int cnt;
>> +
>> +    /* temporarily turn off IRQs during SRST */
>> +    irq_enable = readl(port + PORT_IRQ_ENABLE_SET);
>> +    writel(irq_enable, port + PORT_IRQ_ENABLE_CLR);
>> +
>> +    /* delay 10 ms */
>> +    udelay(10000);
> 
> 
> NAK:
> * long udelay (always NAK'd), as they chew processor and lock out other 
> things, for no reason.
> * passing 1000 or more to udelay() means you should use mdelay() instead
> * in this case, msleep() works just fine
> * I don't see why this delay is needed at all?
> 

The delay is from the original driver and I wasn't really sure whether 
we're allowed to sleep in phy_reset handlers.  IIRC, it didn't use to. 
I'll convert it to msleep.

> 
>> +    prb->ctrl = PRB_CTRL_SRST;
>> +    prb->fis[1] = 0; /* no PM yet */
>> +
>> +    writel((u32)paddr, port + PORT_CMD_ACTIVATE);
>> +
>> +    for (cnt = 0; cnt < 100; cnt++) {
>> +        irq_stat = readl(port + PORT_IRQ_STAT);
>> +        writel(irq_stat, port + PORT_IRQ_STAT);        /* clear irq */
>> +
>> +        irq_stat >>= PORT_IRQ_RAW_SHIFT;
>> +        if (irq_stat & (PORT_IRQ_COMPLETE | PORT_IRQ_ERROR))
>> +            break;
>> +
>> +        udelay(1000);
> 
> 
> NAK:
> * long udelay (always NAK'd)
> * passing 1000 or more to udelay() means you should use mdelay() instead
> * in this case, msleep() works just fine
> 

ditto.

-- 
tejun

Re: [PATCH 3/5] sil24: use SRST for phy_reset

[Mark Lord]

Tejun Heo wrote:
> There seems to be no way to obtain device signature from sil24 after
> SATA phy reset and SRST is needed anyway for later port multiplier
> suppport.  This patch converts sil24_phy_reset to use SRST instaed.

Mmm.. I doubt that sil24 is the only SATA controller with
this issue.  The sata_qstor device, for example, requires PHY reset
*followed* by SRST if one wants to reliably detect a port multiplier.

Maybe the chipset drivers should simply do PHY / SRST as they're told,
and have libata-core ensure PHY + SRST before doing PM detection?

Cheers

Re: [PATCH 3/5] sil24: use SRST for phy_reset

[Jeff Garzik]

Mark Lord wrote:
> Tejun Heo wrote:
> 
>> There seems to be no way to obtain device signature from sil24 after
>> SATA phy reset and SRST is needed anyway for later port multiplier
>> suppport.  This patch converts sil24_phy_reset to use SRST instaed.
> 
> 
> Mmm.. I doubt that sil24 is the only SATA controller with
> this issue.  The sata_qstor device, for example, requires PHY reset
> *followed* by SRST if one wants to reliably detect a port multiplier.
> 
> Maybe the chipset drivers should simply do PHY / SRST as they're told,
> and have libata-core ensure PHY + SRST before doing PM detection?

Well, there is a hardware-dependent component, and a PM-dependent component.

Hardware:  some controller hardware cannot reliably provide the 
signature from the post-COMRESET D2H Register FIS to the OS driver, so 
an SRST is required in any case.

PM:  The SATA PM docs clearly state in two places, that SRST should be 
used to obtain the port multiplier signature, and from there proceed 
with probing.

Thus, if we take the docs at face value, it sounds like sata reset 
should look like

	take iface active (write 0x300 to scontrol)
	poll sstatus for device detection
	clear serror
	issue srst
	get signature
	do useful stuff with signature

That's also a bit nicer to the hardware too, for two reasons:
* COMRESET in general is a hard reset
* often, advanced host controllers issue COMRESET anyway, so we're 
likely doing it twice

Patches welcome ;-)  I am a bit nervous about making this change 
globally, since some early phys may need a COMRESET kick (0x301 to 
SControl)...  so maybe we could phase this in on the advanced 
controllers first (libata's API permits this), and then roll it out to 
the dumber controllers if nothing breaks.

	Jeff