Patchwork [v4] ahci: move ahci_sb600_softreset to libahci.c and rename it

login
register
mail settings
Submitter Yuan-Hsin Chen
Date June 20, 2011, 8:06 a.m.
Message ID <1308557202-1895-1-git-send-email-yuanlmm@gmail.com>
Download mbox | patch
Permalink /patch/101056/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Yuan-Hsin Chen - June 20, 2011, 8:06 a.m.
From: Yuan-Hsin Chen <yhchen@faraday-tech.com>

ahci_sb600_softreset was in ahci.c. This function is used
to fix soft reset failure and renames as ahci_pmp_retry_srst_softreset
in libahci.c.
---
 drivers/ata/ahci.c    |   54 +---------------------------------------------
 drivers/ata/ahci.h    |    1 +
 drivers/ata/libahci.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 53 deletions(-)
Sergei Shtylyov - June 20, 2011, 8:15 a.m.
Hello.

On 20-06-2011 12:06, Yuan-Hsin Chen wrote:

> From: Yuan-Hsin Chen<yhchen@faraday-tech.com>

> ahci_sb600_softreset was in ahci.c. This function is used
> to fix soft reset failure and renames as ahci_pmp_retry_srst_softreset
> in libahci.c.

> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 71afe03..2de36b6 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -79,8 +79,6 @@ enum board_ids {
>   };
>
>   static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
> -static int ahci_sb600_softreset(struct ata_link *link, unsigned int *class,
> -			  unsigned long deadline);
>   static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
>   				 unsigned long deadline);
>   static int ahci_p5wdh_hardreset(struct ata_link *link, unsigned int *class,
> @@ -106,8 +104,7 @@ static struct ata_port_operations ahci_p5wdh_ops = {
>
>   static struct ata_port_operations ahci_sb600_ops = {
>   	.inherits		=&ahci_ops,
> -	.softreset		= ahci_sb600_softreset,
> -	.pmp_softreset		= ahci_sb600_softreset,
> +	.softreset		= ahci_pmp_retry_srst_softreset,

    I have to ask you again: have you tried to compile this?

> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index d38c40f..0fd5a30 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -82,6 +82,8 @@ static void ahci_pmp_attach(struct ata_port *ap);
>   static void ahci_pmp_detach(struct ata_port *ap);
>   static int ahci_softreset(struct ata_link *link, unsigned int *class,
>   			  unsigned long deadline);
> +static int ahci_pmp_retry_srst_softreset(struct ata_link *link, unsigned int *class,
> +			  unsigned long deadline);

    How come this is static if you reference it outside this module?

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - June 20, 2011, 8:19 a.m.
Hello, Yuan.

Yeap, almost there, just few very minor things. :)

On Mon, Jun 20, 2011 at 04:06:41PM +0800, Yuan-Hsin Chen wrote:
>  static struct ata_port_operations ahci_sb600_ops = {
>  	.inherits		= &ahci_ops,
> -	.softreset		= ahci_sb600_softreset,
> -	.pmp_softreset		= ahci_sb600_softreset,
> +	.softreset		= ahci_pmp_retry_srst_softreset,
>  };

Why doesn't sb600 just use ahci_pmp_retry_srst_ops?

> @@ -312,6 +312,7 @@ extern struct device_attribute *ahci_sdev_attrs[];
>  	.sdev_attrs		= ahci_sdev_attrs
>  
>  extern struct ata_port_operations ahci_ops;
> +extern struct ata_port_operations ahci_pmp_retry_srst_ops;

Heh, I know I suggested that name but that is one ugly name.  If
anyone has any better idea, please come forward.

> @@ -82,6 +82,8 @@ static void ahci_pmp_attach(struct ata_port *ap);
>  static void ahci_pmp_detach(struct ata_port *ap);
>  static int ahci_softreset(struct ata_link *link, unsigned int *class,
>  			  unsigned long deadline);
> +static int ahci_pmp_retry_srst_softreset(struct ata_link *link, unsigned int *class,
> +			  unsigned long deadline);

srst equals softreset, so ahci_pmp_retry_softreset() should do.

> +struct ata_port_operations ahci_pmp_retry_srst_ops = {
> +	.inherits		= &ahci_ops,
> +	.softreset		= ahci_pmp_retry_srst_softreset,
> +};
> +EXPORT_SYMBOL_GPL(ahci_pmp_retry_srst_ops);
> +
>  struct ata_port_operations ahci_ops = {
>  	.inherits		= &sata_pmp_port_ops,

Maybe it's better to place ahci_pmp_retry_srst_ops after ahci_ops?

> +static int ahci_pmp_check_ready(struct ata_link *link)

It would be better if the name reflects that it's not for specific
workaround.

Thank you.
Yuan-Hsin Chen - June 20, 2011, 8:47 a.m.
Hello.
I'm really sorry for ignoring your previous e-mail.
I'll fix it in the next patch. Thanks for reviewing my patch.

Yuan-Hsin

On Mon, Jun 20, 2011 at 4:15 PM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
> On 20-06-2011 12:06, Yuan-Hsin Chen wrote:
>
>> From: Yuan-Hsin Chen<yhchen@faraday-tech.com>
>
>> ahci_sb600_softreset was in ahci.c. This function is used
>> to fix soft reset failure and renames as ahci_pmp_retry_srst_softreset
>> in libahci.c.
>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 71afe03..2de36b6 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -79,8 +79,6 @@ enum board_ids {
>>  };
>>
>>  static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
>> *ent);
>> -static int ahci_sb600_softreset(struct ata_link *link, unsigned int
>> *class,
>> -                         unsigned long deadline);
>>  static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int
>> *class,
>>                                 unsigned long deadline);
>>  static int ahci_p5wdh_hardreset(struct ata_link *link, unsigned int
>> *class,
>> @@ -106,8 +104,7 @@ static struct ata_port_operations ahci_p5wdh_ops = {
>>
>>  static struct ata_port_operations ahci_sb600_ops = {
>>        .inherits               =&ahci_ops,
>> -       .softreset              = ahci_sb600_softreset,
>> -       .pmp_softreset          = ahci_sb600_softreset,
>> +       .softreset              = ahci_pmp_retry_srst_softreset,
>
>   I have to ask you again: have you tried to compile this?
>
>> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
>> index d38c40f..0fd5a30 100644
>> --- a/drivers/ata/libahci.c
>> +++ b/drivers/ata/libahci.c
>> @@ -82,6 +82,8 @@ static void ahci_pmp_attach(struct ata_port *ap);
>>  static void ahci_pmp_detach(struct ata_port *ap);
>>  static int ahci_softreset(struct ata_link *link, unsigned int *class,
>>                          unsigned long deadline);
>> +static int ahci_pmp_retry_srst_softreset(struct ata_link *link, unsigned
>> int *class,
>> +                         unsigned long deadline);
>
>   How come this is static if you reference it outside this module?
>
> WBR, Sergei
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yuan-Hsin Chen - June 20, 2011, 10:06 a.m.
Hello.

On Mon, Jun 20, 2011 at 4:19 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Yuan.
>
> Yeap, almost there, just few very minor things. :)
>
> On Mon, Jun 20, 2011 at 04:06:41PM +0800, Yuan-Hsin Chen wrote:
>>  static struct ata_port_operations ahci_sb600_ops = {
>>       .inherits               = &ahci_ops,
>> -     .softreset              = ahci_sb600_softreset,
>> -     .pmp_softreset          = ahci_sb600_softreset,
>> +     .softreset              = ahci_pmp_retry_srst_softreset,
>>  };
>
> Why doesn't sb600 just use ahci_pmp_retry_srst_ops?
>
>> @@ -312,6 +312,7 @@ extern struct device_attribute *ahci_sdev_attrs[];
>>       .sdev_attrs             = ahci_sdev_attrs
>>
>>  extern struct ata_port_operations ahci_ops;
>> +extern struct ata_port_operations ahci_pmp_retry_srst_ops;
>
> Heh, I know I suggested that name but that is one ugly name.  If
> anyone has any better idea, please come forward.

How about ahci_pmp_workaround_softreset or ahci_pmp_ipms_set_softreset?
Also applys to ops, ahci_pmp_workaround_ops or ahci_pmp_ipms_set_ops.

>
>> @@ -82,6 +82,8 @@ static void ahci_pmp_attach(struct ata_port *ap);
>>  static void ahci_pmp_detach(struct ata_port *ap);
>>  static int ahci_softreset(struct ata_link *link, unsigned int *class,
>>                         unsigned long deadline);
>> +static int ahci_pmp_retry_srst_softreset(struct ata_link *link, unsigned int *class,
>> +                       unsigned long deadline);
>
> srst equals softreset, so ahci_pmp_retry_softreset() should do.
>
>> +struct ata_port_operations ahci_pmp_retry_srst_ops = {
>> +     .inherits               = &ahci_ops,
>> +     .softreset              = ahci_pmp_retry_srst_softreset,
>> +};
>> +EXPORT_SYMBOL_GPL(ahci_pmp_retry_srst_ops);
>> +
>>  struct ata_port_operations ahci_ops = {
>>       .inherits               = &sata_pmp_port_ops,
>
> Maybe it's better to place ahci_pmp_retry_srst_ops after ahci_ops?
>
>> +static int ahci_pmp_check_ready(struct ata_link *link)
>
> It would be better if the name reflects that it's not for specific
> workaround.
>
> Thank you.
>
> --
> tejun
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - June 20, 2011, 12:47 p.m.
Hello,

On Mon, Jun 20, 2011 at 06:06:52PM +0800, Yuan-Hsin Chen wrote:
> > Heh, I know I suggested that name but that is one ugly name.  If
> > anyone has any better idea, please come forward.
> 
> How about ahci_pmp_workaround_softreset or ahci_pmp_ipms_set_softreset?
> Also applys to ops, ahci_pmp_workaround_ops or ahci_pmp_ipms_set_ops.

Heh, maybe.  ahci_pmp_workaround_softreset is a bit generic but at
least doesn't completely stupid.  I don't know. :)

Thanks.
Yuan-Hsin Chen - June 21, 2011, 3:24 a.m.
Hello, Tejun.

On Mon, Jun 20, 2011 at 4:19 PM, Tejun Heo <tj@kernel.org> wrote:
>> +static int ahci_pmp_check_ready(struct ata_link *link)
>
> It would be better if the name reflects that it's not for specific
> workaround.

Do you mean it would be better to name it for specific workaround?
How about ahci_bad_pmp_check_ready? Because this function is
to avoid checking TFDATA if BAD PMP is found.

Thanks.

Yuan-Hsin
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - June 21, 2011, 7:17 a.m.
Hello,

On Tue, Jun 21, 2011 at 5:24 AM, Yuan-Hsin Chen <yuanlmm@gmail.com> wrote:
>> It would be better if the name reflects that it's not for specific
>> workaround.
>
> Do you mean it would be better to name it for specific workaround?

Yes, I meant that. An extra 'not' there.

> How about ahci_bad_pmp_check_ready? Because this function is
> to avoid checking TFDATA if BAD PMP is found.

It's minor anyway and I don't think it matters all that much as long
as we make the actual reset and ops names not too confusing.

Thank you.

Patch

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 71afe03..2de36b6 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -79,8 +79,6 @@  enum board_ids {
 };
 
 static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
-static int ahci_sb600_softreset(struct ata_link *link, unsigned int *class,
-			  unsigned long deadline);
 static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
 				 unsigned long deadline);
 static int ahci_p5wdh_hardreset(struct ata_link *link, unsigned int *class,
@@ -106,8 +104,7 @@  static struct ata_port_operations ahci_p5wdh_ops = {
 
 static struct ata_port_operations ahci_sb600_ops = {
 	.inherits		= &ahci_ops,
-	.softreset		= ahci_sb600_softreset,
-	.pmp_softreset		= ahci_sb600_softreset,
+	.softreset		= ahci_pmp_retry_srst_softreset,
 };
 
 #define AHCI_HFLAGS(flags)	.private_data	= (void *)(flags)
@@ -502,55 +499,6 @@  static void ahci_pci_init_controller(struct ata_host *host)
 	ahci_init_controller(host);
 }
 
-static int ahci_sb600_check_ready(struct ata_link *link)
-{
-	void __iomem *port_mmio = ahci_port_base(link->ap);
-	u8 status = readl(port_mmio + PORT_TFDATA) & 0xFF;
-	u32 irq_status = readl(port_mmio + PORT_IRQ_STAT);
-
-	/*
-	 * There is no need to check TFDATA if BAD PMP is found due to HW bug,
-	 * which can save timeout delay.
-	 */
-	if (irq_status & PORT_IRQ_BAD_PMP)
-		return -EIO;
-
-	return ata_check_ready(status);
-}
-
-static int ahci_sb600_softreset(struct ata_link *link, unsigned int *class,
-				unsigned long deadline)
-{
-	struct ata_port *ap = link->ap;
-	void __iomem *port_mmio = ahci_port_base(ap);
-	int pmp = sata_srst_pmp(link);
-	int rc;
-	u32 irq_sts;
-
-	DPRINTK("ENTER\n");
-
-	rc = ahci_do_softreset(link, class, pmp, deadline,
-			       ahci_sb600_check_ready);
-
-	/*
-	 * Soft reset fails on some ATI chips with IPMS set when PMP
-	 * is enabled but SATA HDD/ODD is connected to SATA port,
-	 * do soft reset again to port 0.
-	 */
-	if (rc == -EIO) {
-		irq_sts = readl(port_mmio + PORT_IRQ_STAT);
-		if (irq_sts & PORT_IRQ_BAD_PMP) {
-			ata_link_printk(link, KERN_WARNING,
-					"applying SB600 PMP SRST workaround "
-					"and retrying\n");
-			rc = ahci_do_softreset(link, class, 0, deadline,
-					       ahci_check_ready);
-		}
-	}
-
-	return rc;
-}
-
 static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
 				 unsigned long deadline)
 {
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 12c5282..b175000 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -312,6 +312,7 @@  extern struct device_attribute *ahci_sdev_attrs[];
 	.sdev_attrs		= ahci_sdev_attrs
 
 extern struct ata_port_operations ahci_ops;
+extern struct ata_port_operations ahci_pmp_retry_srst_ops;
 
 void ahci_fill_cmd_slot(struct ahci_port_priv *pp, unsigned int tag,
 			u32 opts);
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index d38c40f..0fd5a30 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -82,6 +82,8 @@  static void ahci_pmp_attach(struct ata_port *ap);
 static void ahci_pmp_detach(struct ata_port *ap);
 static int ahci_softreset(struct ata_link *link, unsigned int *class,
 			  unsigned long deadline);
+static int ahci_pmp_retry_srst_softreset(struct ata_link *link, unsigned int *class,
+			  unsigned long deadline);
 static int ahci_hardreset(struct ata_link *link, unsigned int *class,
 			  unsigned long deadline);
 static void ahci_postreset(struct ata_link *link, unsigned int *class);
@@ -141,6 +143,12 @@  struct device_attribute *ahci_sdev_attrs[] = {
 };
 EXPORT_SYMBOL_GPL(ahci_sdev_attrs);
 
+struct ata_port_operations ahci_pmp_retry_srst_ops = {
+	.inherits		= &ahci_ops,
+	.softreset		= ahci_pmp_retry_srst_softreset,
+};
+EXPORT_SYMBOL_GPL(ahci_pmp_retry_srst_ops);
+
 struct ata_port_operations ahci_ops = {
 	.inherits		= &sata_pmp_port_ops,
 
@@ -1329,6 +1337,55 @@  static int ahci_softreset(struct ata_link *link, unsigned int *class,
 }
 EXPORT_SYMBOL_GPL(ahci_do_softreset);
 
+static int ahci_pmp_check_ready(struct ata_link *link)
+{
+	void __iomem *port_mmio = ahci_port_base(link->ap);
+	u8 status = readl(port_mmio + PORT_TFDATA) & 0xFF;
+	u32 irq_status = readl(port_mmio + PORT_IRQ_STAT);
+
+	/*
+	 * There is no need to check TFDATA if BAD PMP is found due to HW bug,
+	 * which can save timeout delay.
+	 */
+	if (irq_status & PORT_IRQ_BAD_PMP)
+		return -EIO;
+
+	return ata_check_ready(status);
+}
+
+static int ahci_pmp_retry_srst_softreset(struct ata_link *link, unsigned int *class,
+				unsigned long deadline)
+{
+	struct ata_port *ap = link->ap;
+	void __iomem *port_mmio = ahci_port_base(ap);
+	int pmp = sata_srst_pmp(link);
+	int rc;
+	u32 irq_sts;
+
+	DPRINTK("ENTER\n");
+
+	rc = ahci_do_softreset(link, class, pmp, deadline,
+			       ahci_pmp_check_ready);
+
+	/*
+	 * Soft reset fails with IPMS set when PMP is enabled but
+	 * SATA HDD/ODD is connected to SATA port, do soft reset
+	 * again to port 0.
+	 */
+	if (rc == -EIO) {
+		irq_sts = readl(port_mmio + PORT_IRQ_STAT);
+		if (irq_sts & PORT_IRQ_BAD_PMP) {
+			ata_link_printk(link, KERN_WARNING,
+					"applying PMP SRST workaround "
+					"and retrying\n");
+			rc = ahci_do_softreset(link, class, 0, deadline,
+					       ahci_check_ready);
+		}
+	}
+
+	return rc;
+}
+
 static int ahci_hardreset(struct ata_link *link, unsigned int *class,
 			  unsigned long deadline)
 {