| Message ID | 1308557202-1895-1-git-send-email-yuanlmm@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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
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.
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
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
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.
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
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.
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) {
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(-)