[PATCH] ahci_platform: Provide for vendor specific init

[PATCH] ahci_platform: Provide for vendor specific init

[Jassi Brar]

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(-)

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;
-- 
1.6.2.5

Re: [PATCH] ahci_platform: Provide for vendor specific init

[Jeff Garzik]

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

Re: [PATCH] ahci_platform: Provide for vendor specific init

[Jassi Brar]

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.

Re: [PATCH] ahci_platform: Provide for vendor specific init

[Anton Vorontsov]

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,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] ahci_platform: Provide for vendor specific init

[Anton Vorontsov]

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!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] ahci_platform: Provide for vendor specific init

[Jassi Brar]

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.