| Message ID | 1277441858-26993-1-git-send-email-jassisinghbrar@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On 06/25/2010 12:57 AM, Jassi Brar wrote: > From: Jassi Brar<jassi.brar@samsung.com> > > Some AHCI implementations may use Vendor Specific HBA[A0h, FFh] > and/or Port[70h, 7Fh] registers to 'prepare' for initialization. > For that, the platform needs memory mapped address of AHCI registers. > > This patch adds the 'mmio' argument and reorders the call to > platform init function. > > Signed-off-by: Jassi Brar<jassi.brar@samsung.com> > --- > drivers/ata/ahci_platform.c | 23 +++++++++++++---------- > include/linux/ahci_platform.h | 2 +- > 2 files changed, 14 insertions(+), 11 deletions(-) It is also possible that platforms may need the init to prep the MMIO area for use? Anton, does this patch work for you, or introduce such a problem? Thanks, Jeff -- 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
On Fri, Jun 25, 2010 at 4:26 PM, Jeff Garzik <jgarzik@pobox.com> wrote: > On 06/25/2010 12:57 AM, Jassi Brar wrote: >> >> From: Jassi Brar<jassi.brar@samsung.com> >> >> Some AHCI implementations may use Vendor Specific HBA[A0h, FFh] >> and/or Port[70h, 7Fh] registers to 'prepare' for initialization. >> For that, the platform needs memory mapped address of AHCI registers. >> >> This patch adds the 'mmio' argument and reorders the call to >> platform init function. >> >> Signed-off-by: Jassi Brar<jassi.brar@samsung.com> >> --- >> drivers/ata/ahci_platform.c | 23 +++++++++++++---------- >> include/linux/ahci_platform.h | 2 +- >> 2 files changed, 14 insertions(+), 11 deletions(-) > > It is also possible that platforms may need the init to prep the MMIO area > for use? Anton, does this patch work for you, or introduce such a problem? IMO platforms that need such prep(setup iommu ?) have the option to do it in earlier machine init code, but platforms that need to access vendor specific regs have no way other than to duplicate ioremap. Btw, I see no impact on CNS3XXX. Thanks. -- 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
On Fri, Jun 25, 2010 at 03:26:58AM -0400, Jeff Garzik wrote: > On 06/25/2010 12:57 AM, Jassi Brar wrote: > >From: Jassi Brar<jassi.brar@samsung.com> > > > >Some AHCI implementations may use Vendor Specific HBA[A0h, FFh] > >and/or Port[70h, 7Fh] registers to 'prepare' for initialization. > >For that, the platform needs memory mapped address of AHCI registers. > > > >This patch adds the 'mmio' argument and reorders the call to > >platform init function. > > > >Signed-off-by: Jassi Brar<jassi.brar@samsung.com> > >--- > > drivers/ata/ahci_platform.c | 23 +++++++++++++---------- > > include/linux/ahci_platform.h | 2 +- > > 2 files changed, 14 insertions(+), 11 deletions(-) > > It is also possible that platforms may need the init to prep the > MMIO area for use? Anton, does this patch work for you, or > introduce such a problem? Currently it doesn't introduce such a problem, but you're right, platforms might need to, for example, turn on the clocks before the MMIO area becomes accessible (otherwise machine check exception might arrive). There are two options: introduce a ->post_init() hook, or just make sure that ahci_platform driver won't start accessing the mmio region between ioremap() and pdata->init() calls. I think a simple comment in the code will suffice, so no need for the additional hook. And if we ever need some hook before ioremap(), we can name it pre_init(). Thanks,
On Fri, Jun 25, 2010 at 01:57:38PM +0900, Jassi Brar wrote: > From: Jassi Brar <jassi.brar@samsung.com> > > Some AHCI implementations may use Vendor Specific HBA[A0h, FFh] > and/or Port[70h, 7Fh] registers to 'prepare' for initialization. > For that, the platform needs memory mapped address of AHCI registers. > > This patch adds the 'mmio' argument and reorders the call to > platform init function. > > Signed-off-by: Jassi Brar <jassi.brar@samsung.com> Thanks for the patch. Looks good, but see some comments below. (Plus some comments on Jeff's reply) > --- > drivers/ata/ahci_platform.c | 23 +++++++++++++---------- > include/linux/ahci_platform.h | 2 +- > 2 files changed, 14 insertions(+), 11 deletions(-) > > diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c > index 5e11b16..4403941 100644 > --- a/drivers/ata/ahci_platform.c > +++ b/drivers/ata/ahci_platform.c [...] > @@ -74,8 +68,17 @@ static int __init ahci_probe(struct platform_device *pdev) > hpriv->mmio = devm_ioremap(dev, mem->start, resource_size(mem)); > if (!hpriv->mmio) { > dev_err(dev, "can't map %pR\n", mem); > - rc = -ENOMEM; > - goto err0; > + devm_kfree(dev, hpriv); You don't need to explicitly free devm_* resources. > + return -ENOMEM; > + } > + > + if (pdata && pdata->init) { > + rc = pdata->init(dev, hpriv->mmio); > + if (rc) { > + devm_iounmap(dev, hpriv->mmio); > + devm_kfree(dev, hpriv); Ditto, no need for iounmap and kfree. > + return rc; > + } > } > > ahci_save_initial_config(dev, hpriv, > diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h > index f7dd576..f7ffc4c 100644 > --- a/include/linux/ahci_platform.h > +++ b/include/linux/ahci_platform.h > @@ -19,7 +19,7 @@ struct device; > struct ata_port_info; > > struct ahci_platform_data { > - int (*init)(struct device *dev); > + int (*init)(struct device *dev, void __iomem *addr); Please include linux/compiler.h for __iomem. > void (*exit)(struct device *dev); > const struct ata_port_info *ata_port_info; > unsigned int force_port_map; Thanks!
On Fri, Jun 25, 2010 at 4:49 PM, Anton Vorontsov <cbouatmailru@gmail.com> wrote: > On Fri, Jun 25, 2010 at 01:57:38PM +0900, Jassi Brar wrote: >> From: Jassi Brar <jassi.brar@samsung.com> >> >> Some AHCI implementations may use Vendor Specific HBA[A0h, FFh] >> and/or Port[70h, 7Fh] registers to 'prepare' for initialization. >> For that, the platform needs memory mapped address of AHCI registers. >> >> This patch adds the 'mmio' argument and reorders the call to >> platform init function. >> >> Signed-off-by: Jassi Brar <jassi.brar@samsung.com> > > Thanks for the patch. Looks good, but see some comments below. > (Plus some comments on Jeff's reply) Ok, will resend. Thanks. -- 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
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c index 5e11b16..4403941 100644 --- a/drivers/ata/ahci_platform.c +++ b/drivers/ata/ahci_platform.c @@ -54,19 +54,13 @@ static int __init ahci_probe(struct platform_device *pdev) return -EINVAL; } - if (pdata && pdata->init) { - rc = pdata->init(dev); - if (rc) - return rc; - } - if (pdata && pdata->ata_port_info) pi = *pdata->ata_port_info; hpriv = devm_kzalloc(dev, sizeof(*hpriv), GFP_KERNEL); if (!hpriv) { - rc = -ENOMEM; - goto err0; + dev_err(dev, "can't alloc ahci_host_priv\n"); + return -ENOMEM; } hpriv->flags |= (unsigned long)pi.private_data; @@ -74,8 +68,17 @@ static int __init ahci_probe(struct platform_device *pdev) hpriv->mmio = devm_ioremap(dev, mem->start, resource_size(mem)); if (!hpriv->mmio) { dev_err(dev, "can't map %pR\n", mem); - rc = -ENOMEM; - goto err0; + devm_kfree(dev, hpriv); + return -ENOMEM; + } + + if (pdata && pdata->init) { + rc = pdata->init(dev, hpriv->mmio); + if (rc) { + devm_iounmap(dev, hpriv->mmio); + devm_kfree(dev, hpriv); + return rc; + } } ahci_save_initial_config(dev, hpriv, diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h index f7dd576..f7ffc4c 100644 --- a/include/linux/ahci_platform.h +++ b/include/linux/ahci_platform.h @@ -19,7 +19,7 @@ struct device; struct ata_port_info; struct ahci_platform_data { - int (*init)(struct device *dev); + int (*init)(struct device *dev, void __iomem *addr); void (*exit)(struct device *dev); const struct ata_port_info *ata_port_info; unsigned int force_port_map;