[RFC v3 00/13] ahci: add sunxi driver and cleanup imx driver

[RFC v3 00/13] ahci: add sunxi driver and cleanup imx driver

[Hans de Goede]

Hi All,

Here is v3 of my patchset for adding ahci-sunxi support. It has grown quite
a bit since I've been going for a more generic approach this time and I've
also cleaned up the ahci-imx driver to use the same generic approach.

History:

v1, by Olliver Schinagl:
This was using the approach of having a platform device which probe method
creates a new child platform device which gets driven by ahci_platform.c,
as done by ahci_imx.c .

v2, by Hans de Goede:
Stand-alone platform driver based on Olliver's work

v3, by Hans de Goede:
patch-series, with 4 different parts
a) Make ahci_platform.c more generic, handle more then 1 clk, target pwr
   regulator
b) New ahci-sunxi code only populating ahci_platform_data, passed to
   ahci_platform.c to of_device_id matching.
c) Refactor ahci-imx code to work the same as the new ahci-sunxi code, this
   is the reason why v3 is an RFC, I'm waiting for the wandboard I ordered to
   arrive so that I can actually test this.
d) dts bindings for the sunxi ahci parts

Parts a-c are intended for merging through the ata tree, the dts bindings
will be merged to Maxime Ripard's sunxi-dts tree.

I hope people like the new approach for dealing with of drivers which need
to provide ahci_platform_data. An alternate approach would be to export
ahci_probe (renamed to ahci_platform_probe) from ahci_platform.c, then
ahci_sunxi.c could have a probe function like this:

int ahci_sunxi_probe(struct platform_device *pdev) {
    pdev->dev.platform_data = &ahci_sunxi_pdata;
    return ahci_platform_probe(pdev);
}

And then each of ahci_platform, ahci_sunxi and ahci_imx could be its own
module (if build as module) rather then all of them being build into one
module.

Regards,

Hans

[RFC v3 01/13] libahci: Allow drivers to override start_engine

[Hans de Goede]

From: Oliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>

Allwinner A10 and A20 ARM SoCs have an AHCI sata controller which needs a
special register to be poked before starting the DMA engine.

This register gets reset on an ahci_stop_engine call, so there is no other
place then ahci_start_engine where this poking can be done.

This commit allows drivers to override ahci_start_engine behavior for use by
the Allwinner AHCI driver (and potentially other drivers in the future).

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/ata/ahci.h    | 2 ++
 drivers/ata/libahci.c | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 2289efd..8f60f33 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -323,6 +323,8 @@ struct ahci_host_priv {
 	u32			em_msg_type;	/* EM message type */
 	struct clk		*clk;		/* Only for platforms supporting clk */
 	void			*plat_data;	/* Other platform data */
+	/* Optional ahci_start_engine override */
+	void			(*start_engine)(struct ata_port *ap);
 };
 
 extern int ahci_ignore_sss;
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index c482f8c..f6eabbc 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -568,8 +568,14 @@ static int ahci_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val)
 void ahci_start_engine(struct ata_port *ap)
 {
 	void __iomem *port_mmio = ahci_port_base(ap);
+	struct ahci_host_priv *hpriv = ap->host->private_data;
 	u32 tmp;
 
+	if (hpriv->start_engine) {
+		hpriv->start_engine(ap);
+		return;
+	}
+
 	/* start DMA */
 	tmp = readl(port_mmio + PORT_CMD);
 	tmp |= PORT_CMD_START;
-- 
1.8.4.2

Re: [RFC v3 01/13] libahci: Allow drivers to override start_engine

[Tejun Heo]

Hello, Hans.

On Sun, Jan 19, 2014 at 12:48:43AM +0100, Hans de Goede wrote:
>  void ahci_start_engine(struct ata_port *ap)
>  {
>  	void __iomem *port_mmio = ahci_port_base(ap);
> +	struct ahci_host_priv *hpriv = ap->host->private_data;
>  	u32 tmp;
>  
> +	if (hpriv->start_engine) {
> +		hpriv->start_engine(ap);
> +		return;
> +	}
> +

I'd much prefer if the driver always calls hpriv->start_engine() which
is initialized to the default callback during common init and then
overridden if necessary.

Thanks.

-- 
tejun

Re: [RFC v3 01/13] libahci: Allow drivers to override start_engine

[Hans de Goede]

Hi,

On 01/19/2014 05:46 AM, Tejun Heo wrote:
> Hello, Hans.
>
> On Sun, Jan 19, 2014 at 12:48:43AM +0100, Hans de Goede wrote:
>>   void ahci_start_engine(struct ata_port *ap)
>>   {
>>   	void __iomem *port_mmio = ahci_port_base(ap);
>> +	struct ahci_host_priv *hpriv = ap->host->private_data;
>>   	u32 tmp;
>>
>> +	if (hpriv->start_engine) {
>> +		hpriv->start_engine(ap);
>> +		return;
>> +	}
>> +
>
> I'd much prefer if the driver always calls hpriv->start_engine() which
> is initialized to the default callback during common init and then
> overridden if necessary.

This is a bit trouble some because ahci_start_engine is also exported,
so the patch would also need to fix all callers (and probably un-export
it). I can do this in the next revision in the patch-set if you want me
to, just explaining why I choose to do this the way I did.

Regards,

Hans

[RFC v3 02/13] sata-highbank: Remove unnecessary ahci_platform.h include

[Hans de Goede]

The sata-highbank driver is a complete standalone sata driver, which does
not use ahci_platform.c / ahci_platform_data in any way.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/ata/sata_highbank.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index ea3b3dc..870b11e 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -29,7 +29,6 @@
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/libata.h>
-#include <linux/ahci_platform.h>
 #include <linux/interrupt.h>
 #include <linux/delay.h>
 #include <linux/export.h>
-- 
1.8.4.2

Re: [RFC v3 02/13] sata-highbank: Remove unnecessary ahci_platform.h include

[Tejun Heo]

On Sun, Jan 19, 2014 at 12:48:44AM +0100, Hans de Goede wrote:
> The sata-highbank driver is a complete standalone sata driver, which does
> not use ahci_platform.c / ahci_platform_data in any way.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Applied to libata/for-3.14.

Thanks.

-- 
tejun

[RFC v3 03/13] ahci-platform: Fix clk enable/disable unbalance on suspend/resume

[Hans de Goede]

For devices where ahci_platform_data provides suspend() there is an unbalance
in clk enable/disable calls. The suspend path does not disable the clk, but
the resume path enables it. This commit fixes it by always disabling the clk
in the suspend path independent of there being an ahci_platform_data suspend
method.

On all drivers currently using a suspend method, the suspend method always
succeeds, so change its return type to void, to avoid having the introduce
somewhat complex error handling paths.

The disabling of the clock on suspend is a functional change, to ensure this
is ok I've audited all drivers using ahci_platform_data:

arch/arm/mach-davinci/devices-da8xx.c: Does not use a suspend method
arch/arm/mach-spear/spear1340.c: Does not use the clock framework
drivers/ata/ahci_imx.c: Does have suspend and resume pdata methods, these
disable / enable another clock, so likely it is ok to disable / enable the
clock at of-node index 0 as well, I've ordered a wandboard to be able to
test these changes.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-spear/spear1340.c | 6 ++----
 drivers/ata/ahci_imx.c          | 4 +---
 drivers/ata/ahci_platform.c     | 2 +-
 include/linux/ahci_platform.h   | 2 +-
 4 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-spear/spear1340.c b/arch/arm/mach-spear/spear1340.c
index 3fb6834..c4cbbd2 100644
--- a/arch/arm/mach-spear/spear1340.c
+++ b/arch/arm/mach-spear/spear1340.c
@@ -107,14 +107,12 @@ void sata_miphy_exit(struct device *dev)
 	msleep(20);
 }
 
-int sata_suspend(struct device *dev)
+void sata_suspend(struct device *dev)
 {
 	if (dev->power.power_state.event == PM_EVENT_FREEZE)
-		return 0;
+		return;
 
 	sata_miphy_exit(dev);
-
-	return 0;
 }
 
 int sata_resume(struct device *dev)
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index 3e23e99..30568d3 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -171,7 +171,7 @@ static void imx6q_sata_exit(struct device *dev)
 	clk_disable_unprepare(imxpriv->sata_ref_clk);
 }
 
-static int imx_ahci_suspend(struct device *dev)
+static void imx_ahci_suspend(struct device *dev)
 {
 	struct imx_ahci_priv *imxpriv =  dev_get_drvdata(dev->parent);
 
@@ -185,8 +185,6 @@ static int imx_ahci_suspend(struct device *dev)
 				!IMX6Q_GPR13_SATA_MPLL_CLK_EN);
 		clk_disable_unprepare(imxpriv->sata_ref_clk);
 	}
-
-	return 0;
 }
 
 static int imx_ahci_resume(struct device *dev)
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 4b231ba..dc1ef73 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -275,7 +275,7 @@ static int ahci_suspend(struct device *dev)
 		return rc;
 
 	if (pdata && pdata->suspend)
-		return pdata->suspend(dev);
+		pdata->suspend(dev);
 
 	if (!IS_ERR(hpriv->clk))
 		clk_disable_unprepare(hpriv->clk);
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 73a2500..a641cb6 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -23,7 +23,7 @@ struct ata_port_info;
 struct ahci_platform_data {
 	int (*init)(struct device *dev, void __iomem *addr);
 	void (*exit)(struct device *dev);
-	int (*suspend)(struct device *dev);
+	void (*suspend)(struct device *dev);
 	int (*resume)(struct device *dev);
 	const struct ata_port_info *ata_port_info;
 	unsigned int force_port_map;
-- 
1.8.4.2

Re: [RFC v3 03/13] ahci-platform: Fix clk enable/disable unbalance on suspend/resume

[Tejun Heo]

On Sun, Jan 19, 2014 at 12:48:45AM +0100, Hans de Goede wrote:
> For devices where ahci_platform_data provides suspend() there is an unbalance
> in clk enable/disable calls. The suspend path does not disable the clk, but
> the resume path enables it. This commit fixes it by always disabling the clk
> in the suspend path independent of there being an ahci_platform_data suspend
> method.
> 
> On all drivers currently using a suspend method, the suspend method always
> succeeds, so change its return type to void, to avoid having the introduce
> somewhat complex error handling paths.
> 
> The disabling of the clock on suspend is a functional change, to ensure this
> is ok I've audited all drivers using ahci_platform_data:
> 
> arch/arm/mach-davinci/devices-da8xx.c: Does not use a suspend method
> arch/arm/mach-spear/spear1340.c: Does not use the clock framework
> drivers/ata/ahci_imx.c: Does have suspend and resume pdata methods, these
> disable / enable another clock, so likely it is ok to disable / enable the
> clock at of-node index 0 as well, I've ordered a wandboard to be able to
> test these changes.

This isn't your fault but similarly to the previous patch, I'd much
prefer if drivers which need custom ops just override the whole
operation and are allowed to use the default platform from their
custom implementations as they see fit.  Allowing partial overrides
seems like an efficient thing to do at the beginning but if you
continue to stack them, you eventually end up with giant pile of
methods where figuring out which code paths are actually executed
takes quite a bit of effort.  I'd really like to avoid that.

Thanks.

-- 
tejun

Re: [RFC v3 03/13] ahci-platform: Fix clk enable/disable unbalance on suspend/resume

[Hans de Goede]

Hi,

On 01/19/2014 12:14 PM, Tejun Heo wrote:
> On Sun, Jan 19, 2014 at 12:48:45AM +0100, Hans de Goede wrote:
>> For devices where ahci_platform_data provides suspend() there is an unbalance
>> in clk enable/disable calls. The suspend path does not disable the clk, but
>> the resume path enables it. This commit fixes it by always disabling the clk
>> in the suspend path independent of there being an ahci_platform_data suspend
>> method.
>>
>> On all drivers currently using a suspend method, the suspend method always
>> succeeds, so change its return type to void, to avoid having the introduce
>> somewhat complex error handling paths.
>>
>> The disabling of the clock on suspend is a functional change, to ensure this
>> is ok I've audited all drivers using ahci_platform_data:
>>
>> arch/arm/mach-davinci/devices-da8xx.c: Does not use a suspend method
>> arch/arm/mach-spear/spear1340.c: Does not use the clock framework
>> drivers/ata/ahci_imx.c: Does have suspend and resume pdata methods, these
>> disable / enable another clock, so likely it is ok to disable / enable the
>> clock at of-node index 0 as well, I've ordered a wandboard to be able to
>> test these changes.
>
> This isn't your fault but similarly to the previous patch, I'd much
> prefer if drivers which need custom ops just override the whole
> operation and are allowed to use the default platform from their
> custom implementations as they see fit.  Allowing partial overrides
> seems like an efficient thing to do at the beginning but if you
> continue to stack them, you eventually end up with giant pile of
> methods where figuring out which code paths are actually executed
> takes quite a bit of effort.  I'd really like to avoid that.

I disagree, if we were to follow this reasoning then the init and exit
overrides would have to logically also be all or nothing propositions,
currently ahci_probe and ahci_stop do all the clks, regulator and sata-core
setup / teardown needed with the init / exit pdata callbacks doing any
implementation specific register frobbing needed.

As I see it either doing clks, regulator and sata-core things in a common
place makes sense, and then it goes for suspend and resume too, or we
opt for always following the complete override model, and which point
it becomes more sensible to just do a separate platform driver per
ahci implementation.

After this patch, suspend / resume exactly follow init / stop in that
the ahci_platform.c bits take care of the common stuff, while calling
into a platform_data callback for implementation specific setup.

Also if you look at both the imx and sunxi implementations doing things
this way works well in practice.

Regards,

Hans

Re: [RFC v3 03/13] ahci-platform: Fix clk enable/disable unbalance on suspend/resume

[Tejun Heo]

Hey,

On Sun, Jan 19, 2014 at 07:47:06PM +0100, Hans de Goede wrote:
> As I see it either doing clks, regulator and sata-core things in a common
> place makes sense, and then it goes for suspend and resume too, or we
> opt for always following the complete override model, and which point
> it becomes more sensible to just do a separate platform driver per
> ahci implementation.

It makes sense in light of those specific cases, but there are gonna
be cases where the placement of the callback is slightly wrong and we
end up with ->XXX_ops_pre() and then ->XXX_ops_post() and so on.
Please make the whole op overridable and then export the default op
and use it as library.

Thanks.

-- 
tejun

Re: [RFC v3 03/13] ahci-platform: Fix clk enable/disable unbalance on suspend/resume

[Hans de Goede]

Hi,

On 01/19/2014 08:15 PM, Tejun Heo wrote:
> Hey,
>
> On Sun, Jan 19, 2014 at 07:47:06PM +0100, Hans de Goede wrote:
>> As I see it either doing clks, regulator and sata-core things in a common
>> place makes sense, and then it goes for suspend and resume too, or we
>> opt for always following the complete override model, and which point
>> it becomes more sensible to just do a separate platform driver per
>> ahci implementation.
>
> It makes sense in light of those specific cases, but there are gonna
> be cases where the placement of the callback is slightly wrong and we
> end up with ->XXX_ops_pre() and then ->XXX_ops_post() and so on.
> Please make the whole op overridable and then export the default op
> and use it as library.

If we were to put a generic implementation in ahci_platform.c and export
it for use from overrides to avoid copy and pasting common bits everywhere,
then we still have the ordering problem you are talking about. How do you
envision all of this fitting together, I can imagine ie a
ahci_platform_resume_controller which has the bits of what is currently
ahci_resume stating at:

"if (dev->power.power_state.event == PM_EVENT_SUSPEND) {"

And having ahci_resume in ahci_platform.c still doing the clk and power enabling
before calling into ahci_platform_resume, and drivers overriding the resume method
need to do their own clk + regulator + whatever setup
before calling into ahci_platform_resume_controller ?

Also how do you see overriding the entire op, does that mean that pdata->suspend
will be deprecated (we will need to keep it around for now to avoid breaking
existing drivers using it), and all of:

ahci_probe
ahci_suspend
ahci_resume

Will get exported from ahci_platform.c and drivers needing to override any of them
will provide their own platform_driver struct, pointing either to their overrides,
or for driver methods they don't need to override to the exported function from
ahci_platform.c ?

This would also work nicely for the of_device_id data stuff, since if drivers
have their own platform_driver struct these bits could just go inside the
driver file instead of being in #ifdef CONFIG_FOO in ahci_platform.c

Regards,

Hans

Re: [RFC v3 03/13] ahci-platform: Fix clk enable/disable unbalance on suspend/resume

[Tejun Heo]

Hey,

On Sun, Jan 19, 2014 at 08:52:21PM +0100, Hans de Goede wrote:
> And having ahci_resume in ahci_platform.c still doing the clk and power enabling
> before calling into ahci_platform_resume, and drivers overriding the resume method
> need to do their own clk + regulator + whatever setup
> before calling into ahci_platform_resume_controller ?

If the use cases are significant enough, split the base function to
two parts?  The thing which gets really messy down the road is when
common code and callbacks intertwine arbitrarily.  A driver wants an
early exit after overriding a subpart, so if that callback exists,
it's an early exit.  The next driver wants to override something at
slightly different point but still want to proceed with the rest of
the common code, so it adds a new callback which doesn't do early
exit, and so on.  Pretty soon, when you're looking at a driver, it
becomes really difficult what it actually does.  We *HAD* this problem
over and over again with ide and it was a nightmare.

Providing larger, logical overriding points while providing common lib
routines makes understanding what a given driver does a lot easier.
Also, it forces people to think about how the overall API looks and
whether a split of an existing library which require giving the splits
sensible names and semantics is justifiable or if the case at hand is
an one-off thing which can be better served by just open coding it.

> Will get exported from ahci_platform.c and drivers needing to override any of them
> will provide their own platform_driver struct, pointing either to their overrides,
> or for driver methods they don't need to override to the exported function from
> ahci_platform.c ?

Yeah, makes sense to me.

Thanks.

-- 
tejun

[RFC v3 04/13] ahci-platform: Undo pdata->resume on resume failure

[Hans de Goede]

When the ahci_resume fails the error handling code tries to undo all changes
made, but it was not undoing the results of pdata->resume.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/ata/ahci_platform.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index dc1ef73..41720cb 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -307,7 +307,7 @@ static int ahci_resume(struct device *dev)
 	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
 		rc = ahci_reset_controller(host);
 		if (rc)
-			goto disable_unprepare_clk;
+			goto pdata_suspend;
 
 		ahci_init_controller(host);
 	}
@@ -316,6 +316,9 @@ static int ahci_resume(struct device *dev)
 
 	return 0;
 
+pdata_suspend:
+	if (pdata && pdata->suspend)
+		pdata->suspend(dev);
 disable_unprepare_clk:
 	if (!IS_ERR(hpriv->clk))
 		clk_disable_unprepare(hpriv->clk);
-- 
1.8.4.2

Re: [RFC v3 04/13] ahci-platform: Undo pdata->resume on resume failure

[Tejun Heo]

Hello,

On Sun, Jan 19, 2014 at 12:48:46AM +0100, Hans de Goede wrote:
> When the ahci_resume fails the error handling code tries to undo all changes
> made, but it was not undoing the results of pdata->resume.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/ata/ahci_platform.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index dc1ef73..41720cb 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
> @@ -307,7 +307,7 @@ static int ahci_resume(struct device *dev)
>  	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
>  		rc = ahci_reset_controller(host);
>  		if (rc)
> -			goto disable_unprepare_clk;
> +			goto pdata_suspend;
>  
>  		ahci_init_controller(host);
>  	}
> @@ -316,6 +316,9 @@ static int ahci_resume(struct device *dev)
>  
>  	return 0;
>  
> +pdata_suspend:
> +	if (pdata && pdata->suspend)
> +		pdata->suspend(dev);
>  disable_unprepare_clk:
>  	if (!IS_ERR(hpriv->clk))
>  		clk_disable_unprepare(hpriv->clk);

Hmmmm... resume isn't an operation you can revert without side-effect
when the whole system is waking up from sleep.  e.g. think about what
should happen the driver is removed and loaded again - it should be
able to reinitialized the device, which is unlikely to work if the
device is suspended at the platform level.  If resume fails, the right
state to be in is "failed with as much as resumed" instead of
"suspended".

Thanks.

-- 
tejun

Re: [RFC v3 04/13] ahci-platform: Undo pdata->resume on resume failure

[Hans de Goede]

Hi,

On 01/19/2014 12:27 PM, Tejun Heo wrote:
> Hello,
>
> On Sun, Jan 19, 2014 at 12:48:46AM +0100, Hans de Goede wrote:
>> When the ahci_resume fails the error handling code tries to undo all changes
>> made, but it was not undoing the results of pdata->resume.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>   drivers/ata/ahci_platform.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
>> index dc1ef73..41720cb 100644
>> --- a/drivers/ata/ahci_platform.c
>> +++ b/drivers/ata/ahci_platform.c
>> @@ -307,7 +307,7 @@ static int ahci_resume(struct device *dev)
>>   	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
>>   		rc = ahci_reset_controller(host);
>>   		if (rc)
>> -			goto disable_unprepare_clk;
>> +			goto pdata_suspend;
>>
>>   		ahci_init_controller(host);
>>   	}
>> @@ -316,6 +316,9 @@ static int ahci_resume(struct device *dev)
>>
>>   	return 0;
>>
>> +pdata_suspend:
>> +	if (pdata && pdata->suspend)
>> +		pdata->suspend(dev);
>>   disable_unprepare_clk:
>>   	if (!IS_ERR(hpriv->clk))
>>   		clk_disable_unprepare(hpriv->clk);
>
> Hmmmm... resume isn't an operation you can revert without side-effect
> when the whole system is waking up from sleep.  e.g. think about what
> should happen the driver is removed and loaded again - it should be
> able to reinitialized the device, which is unlikely to work if the
> device is suspended at the platform level.  If resume fails, the right
> state to be in is "failed with as much as resumed" instead of
> "suspended".

That sounds like your advocating for just returning from resume on the
first error without undoing any of the previous steps, have I gotten that
right?

That sounds as sensible as any other approach on resume errors
(there are IMHO no good answers), if that is what you mean, shall I do a
patch in the next versions of my patch-set doing that ?

Regards,

Hans

Re: [RFC v3 04/13] ahci-platform: Undo pdata->resume on resume failure

[Tejun Heo]

Hello,

On Sun, Jan 19, 2014 at 07:40:21PM +0100, Hans de Goede wrote:
> That sounds like your advocating for just returning from resume on the
> first error without undoing any of the previous steps, have I gotten that
> right?

Yeah, or just ignore reset failure and proceed.

> That sounds as sensible as any other approach on resume errors
> (there are IMHO no good answers), if that is what you mean, shall I do a

Suspending back is the wrong answer tho.

> patch in the next versions of my patch-set doing that ?

Isn't just dropping this patch enough tho?

Thanks.

-- 
tejun

Re: [RFC v3 04/13] ahci-platform: Undo pdata->resume on resume failure

[Hans de Goede]

Hi,

On 01/19/2014 08:13 PM, Tejun Heo wrote:
> Hello,
>
> On Sun, Jan 19, 2014 at 07:40:21PM +0100, Hans de Goede wrote:
>> That sounds like your advocating for just returning from resume on the
>> first error without undoing any of the previous steps, have I gotten that
>> right?
>
> Yeah, or just ignore reset failure and proceed.

That seems like a bad idea IMHO, if reset failed something is seriously
amiss and just continuing as nothing happened seems unhelpful.

>> That sounds as sensible as any other approach on resume errors
>> (there are IMHO no good answers), if that is what you mean, shall I do a
>
> Suspending back is the wrong answer tho.
>
>> patch in the next versions of my patch-set doing that ?
>
> Isn't just dropping this patch enough tho?

Well the current error handling still re-disables the clks on resume failure,
if you want to proceed with resume as far as possible, rather then return to
a suspended state it seems sensible to just leave the clocks on as well.

Also disabling the clocks on resume failure, followed by a rmmod will cause
a WARN_ON to trigger in the clock-framework when ahci_host_stop tries to
disable the clks for a second time.

Regards,

Hans

Re: [RFC v3 04/13] ahci-platform: Undo pdata->resume on resume failure

[Tejun Heo]

On Sun, Jan 19, 2014 at 08:34:51PM +0100, Hans de Goede wrote:
> Well the current error handling still re-disables the clks on resume failure,
> if you want to proceed with resume as far as possible, rather then return to

Let's put it as "put it in a state where it can be reinitialized
afterwards".

Thanks.

-- 
tejun

Re: [RFC v3 04/13] ahci-platform: Undo pdata->resume on resume failure

[Hans de Goede]

Hi,

On 01/19/2014 08:42 PM, Tejun Heo wrote:
> On Sun, Jan 19, 2014 at 08:34:51PM +0100, Hans de Goede wrote:
>> Well the current error handling still re-disables the clks on resume failure,
>> if you want to proceed with resume as far as possible, rather then return to
>
> Let's put it as "put it in a state where it can be reinitialized
> afterwards".

Ok, I'll drop the patch then.

Regards,

Hans

[RFC v3 05/13] ahci-platform: Pass ahci_host_priv ptr to ahci_platform_data init method

[Hans de Goede]

Some ahci_platform_data->init methods need access to the ahci_host_priv data.

Note:

When calling ahci_platform_data->init the ata_host has not been allocated yet,
so access to ahci_host_priv through the dev argument is not possible.

The hpriv->mmio argument may seem redundant, but it is useful for drivers
which live outside of drivers/ata and thus don't know the contents of the
ahci_platform_data data type.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-davinci/devices-da8xx.c | 3 ++-
 arch/arm/mach-spear/spear1340.c       | 3 ++-
 drivers/ata/ahci_imx.c                | 3 ++-
 drivers/ata/ahci_platform.c           | 2 +-
 include/linux/ahci_platform.h         | 4 +++-
 5 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
index 78829c5..81d0110 100644
--- a/arch/arm/mach-davinci/devices-da8xx.c
+++ b/arch/arm/mach-davinci/devices-da8xx.c
@@ -1061,7 +1061,8 @@ static unsigned long da850_sata_xtal[] = {
 	KHZ_TO_HZ(60000),
 };
 
-static int da850_sata_init(struct device *dev, void __iomem *addr)
+static int da850_sata_init(struct device *dev, struct ahci_host_priv *hpriv,
+			   void __iomem *addr)
 {
 	int i, ret;
 	unsigned int val;
diff --git a/arch/arm/mach-spear/spear1340.c b/arch/arm/mach-spear/spear1340.c
index c4cbbd2..9e4f70b 100644
--- a/arch/arm/mach-spear/spear1340.c
+++ b/arch/arm/mach-spear/spear1340.c
@@ -77,7 +77,8 @@
 			SPEAR1340_MIPHY_PLL_RATIO_TOP(25))
 
 /* SATA device registration */
-static int sata_miphy_init(struct device *dev, void __iomem *addr)
+static int sata_miphy_init(struct device *dev, struct ahci_host_priv *hpriv,
+			   void __iomem *addr)
 {
 	writel(SPEAR1340_SATA_CFG_VAL, SPEAR1340_PCIE_SATA_CFG);
 	writel(SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA_25M_CRYSTAL_CLK,
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index 30568d3..0051f29 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -91,7 +91,8 @@ static const struct ata_port_info ahci_imx_port_info = {
 	.port_ops	= &ahci_imx_ops,
 };
 
-static int imx6q_sata_init(struct device *dev, void __iomem *mmio)
+static int imx6q_sata_init(struct device *dev, struct ahci_host_priv *hpriv,
+			   void __iomem *mmio)
 {
 	int ret = 0;
 	unsigned int reg_val;
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 41720cb..33ac7a4 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -149,7 +149,7 @@ static int ahci_probe(struct platform_device *pdev)
 	 * returned successfully.
 	 */
 	if (pdata && pdata->init) {
-		rc = pdata->init(dev, hpriv->mmio);
+		rc = pdata->init(dev, hpriv, hpriv->mmio);
 		if (rc)
 			goto disable_unprepare_clk;
 	}
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index a641cb6..796dfb4 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -19,9 +19,11 @@
 
 struct device;
 struct ata_port_info;
+struct ahci_host_priv;
 
 struct ahci_platform_data {
-	int (*init)(struct device *dev, void __iomem *addr);
+	int (*init)(struct device *dev, struct ahci_host_priv *hpriv,
+		    void __iomem *addr);
 	void (*exit)(struct device *dev);
 	void (*suspend)(struct device *dev);
 	int (*resume)(struct device *dev);
-- 
1.8.4.2

Re: [RFC v3 05/13] ahci-platform: Pass ahci_host_priv ptr to ahci_platform_data init method

[Tejun Heo]

On Sun, Jan 19, 2014 at 12:48:47AM +0100, Hans de Goede wrote:
> Some ahci_platform_data->init methods need access to the ahci_host_priv data.
> 
> Note:
> 
> When calling ahci_platform_data->init the ata_host has not been allocated yet,
> so access to ahci_host_priv through the dev argument is not possible.
> 
> The hpriv->mmio argument may seem redundant, but it is useful for drivers
> which live outside of drivers/ata and thus don't know the contents of the
> ahci_platform_data data type.

Wouldn't the right thing to do be moving them under drivers/ata?

Thanks.

-- 
tejun

Re: [RFC v3 05/13] ahci-platform: Pass ahci_host_priv ptr to ahci_platform_data init method

[Hans de Goede]

Hi,

On 01/19/2014 12:30 PM, Tejun Heo wrote:
> On Sun, Jan 19, 2014 at 12:48:47AM +0100, Hans de Goede wrote:
>> Some ahci_platform_data->init methods need access to the ahci_host_priv data.
>>
>> Note:
>>
>> When calling ahci_platform_data->init the ata_host has not been allocated yet,
>> so access to ahci_host_priv through the dev argument is not possible.
>>
>> The hpriv->mmio argument may seem redundant, but it is useful for drivers
>> which live outside of drivers/ata and thus don't know the contents of the
>> ahci_platform_data data type.
>
> Wouldn't the right thing to do be moving them under drivers/ata?

Yes, but they are part of arch/arm/mach-foo, so moving them is non trivial, and
I don't want to go make these kinda changes without hardware to test. Please keep
in mind that I'm partially cleaning up other peoples mess here (the imx bits
specifically). I'm willing to do that to some extend (*). But buying an imx6q board
and fixing that is about as far as I'm willing to go.

Regards,

Hans


*) Keep in mind that this is a hobby project done in my spare time, with all
hardware bought from personal budget.

Re: [RFC v3 05/13] ahci-platform: Pass ahci_host_priv ptr to ahci_platform_data init method

[Tejun Heo]

On Sun, Jan 19, 2014 at 07:51:48PM +0100, Hans de Goede wrote:
> Yes, but they are part of arch/arm/mach-foo, so moving them is non trivial, and
> I don't want to go make these kinda changes without hardware to test. Please keep
> in mind that I'm partially cleaning up other peoples mess here (the imx bits
> specifically). I'm willing to do that to some extend (*). But buying an imx6q board
> and fixing that is about as far as I'm willing to go.

If they can't be moved, the right thing to do is moving the header out
to include/linux so that they can be included from those directories.
Let's please not contort the API to fit the organizational issues.

Thanks.

-- 
tejun

Re: [RFC v3 05/13] ahci-platform: Pass ahci_host_priv ptr to ahci_platform_data init method

[Hans de Goede]

Hi,

On 01/19/2014 08:17 PM, Tejun Heo wrote:
> On Sun, Jan 19, 2014 at 07:51:48PM +0100, Hans de Goede wrote:
>> Yes, but they are part of arch/arm/mach-foo, so moving them is non trivial, and
>> I don't want to go make these kinda changes without hardware to test. Please keep
>> in mind that I'm partially cleaning up other peoples mess here (the imx bits
>> specifically). I'm willing to do that to some extend (*). But buying an imx6q board
>> and fixing that is about as far as I'm willing to go.
>
> If they can't be moved, the right thing to do is moving the header out
> to include/linux so that they can be included from those directories.
> Let's please not contort the API to fit the organizational issues.

Ok, asking the obvious here I guess, but you would accept a patch moving drivers/ata/ahci.h
to include/linux as part of this patch-set ?

Regards,

Hans

Re: [RFC v3 05/13] ahci-platform: Pass ahci_host_priv ptr to ahci_platform_data init method

[Tejun Heo]

On Sun, Jan 19, 2014 at 08:56:09PM +0100, Hans de Goede wrote:
> Ok, asking the obvious here I guess, but you would accept a patch moving drivers/ata/ahci.h
> to include/linux as part of this patch-set ?

Wouldn't creating a minimal file which only contains the necessary
bits make more sense?

Thanks.

-- 
tejun

[RFC v3 06/13] ahci-platform: Add support for devices with more then 1 clock

[Hans de Goede]

The allwinner-sun4i AHCI controller needs 2 clocks to be enabled and the
imx AHCI controller needs 3 clocks to be enabled.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/ata/ahci-platform.txt      |  1 +
 drivers/ata/ahci.h                                 |  3 +-
 drivers/ata/ahci_platform.c                        | 95 +++++++++++++++-------
 3 files changed, 67 insertions(+), 32 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 89de156..3ced07d 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -10,6 +10,7 @@ Required properties:
 
 Optional properties:
 - dma-coherent      : Present if dma operations are coherent
+- clocks            : a list of phandle + clock specifier pairs
 
 Example:
         sata@ffe08000 {
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 8f60f33..7950b3a 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -52,6 +52,7 @@
 enum {
 	AHCI_MAX_PORTS		= 32,
 	AHCI_MAX_SG		= 168, /* hardware max is 64K */
+	AHCI_MAX_CLKS		= 3,
 	AHCI_DMA_BOUNDARY	= 0xffffffff,
 	AHCI_MAX_CMDS		= 32,
 	AHCI_CMD_SZ		= 32,
@@ -321,7 +322,7 @@ struct ahci_host_priv {
 	u32 			em_loc; /* enclosure management location */
 	u32			em_buf_sz;	/* EM buffer size in byte */
 	u32			em_msg_type;	/* EM message type */
-	struct clk		*clk;		/* Only for platforms supporting clk */
+	struct clk		*clks[AHCI_MAX_CLKS]; /* Optional */
 	void			*plat_data;	/* Other platform data */
 	/* Optional ahci_start_engine override */
 	void			(*start_engine)(struct ata_port *ap);
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 33ac7a4..75a3d47 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -87,6 +87,42 @@ static struct scsi_host_template ahci_platform_sht = {
 	AHCI_SHT("ahci_platform"),
 };
 
+static int ahci_enable_clks(struct device *dev, struct ahci_host_priv *hpriv)
+{
+	int c, rc;
+
+	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) {
+		rc = clk_prepare_enable(hpriv->clks[c]);
+		if (rc) {
+			dev_err(dev, "clock prepare enable failed");
+			goto disable_unprepare_clk;
+		}
+	}
+	return 0;
+
+disable_unprepare_clk:
+	while (--c >= 0)
+		clk_disable_unprepare(hpriv->clks[c]);
+	return rc;
+}
+
+static void ahci_disable_clks(struct ahci_host_priv *hpriv)
+{
+	int c;
+
+	for (c = AHCI_MAX_CLKS - 1; c >= 0; c--)
+		if (hpriv->clks[c])
+			clk_disable_unprepare(hpriv->clks[c]);
+}
+
+static void ahci_put_clks(struct ahci_host_priv *hpriv)
+{
+	int c;
+
+	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++)
+		clk_put(hpriv->clks[c]);
+}
+
 static int ahci_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -97,10 +133,8 @@ static int ahci_probe(struct platform_device *pdev)
 	struct ahci_host_priv *hpriv;
 	struct ata_host *host;
 	struct resource *mem;
-	int irq;
-	int n_ports;
-	int i;
-	int rc;
+	struct clk *clk;
+	int i, irq, max_clk, n_ports, rc;
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!mem) {
@@ -131,17 +165,26 @@ static int ahci_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	hpriv->clk = clk_get(dev, NULL);
-	if (IS_ERR(hpriv->clk)) {
-		dev_err(dev, "can't get clock\n");
-	} else {
-		rc = clk_prepare_enable(hpriv->clk);
-		if (rc) {
-			dev_err(dev, "clock prepare enable failed");
-			goto free_clk;
+	max_clk = dev->of_node ? AHCI_MAX_CLKS : 1;
+	for (i = 0; i < max_clk; i++) {
+		if (i == 0)
+			clk = clk_get(dev, NULL); /* For old platform init */
+		else
+			clk = of_clk_get(dev->of_node, i);
+
+		if (IS_ERR(clk)) {
+			rc = PTR_ERR(clk);
+			if (rc == -EPROBE_DEFER)
+				goto free_clk;
+			break;
 		}
+		hpriv->clks[i] = clk;
 	}
 
+	rc = ahci_enable_clks(dev, hpriv);
+	if (rc)
+		goto free_clk;
+
 	/*
 	 * Some platforms might need to prepare for mmio region access,
 	 * which could be done in the following init call. So, the mmio
@@ -222,11 +265,9 @@ pdata_exit:
 	if (pdata && pdata->exit)
 		pdata->exit(dev);
 disable_unprepare_clk:
-	if (!IS_ERR(hpriv->clk))
-		clk_disable_unprepare(hpriv->clk);
+	ahci_disable_clks(hpriv);
 free_clk:
-	if (!IS_ERR(hpriv->clk))
-		clk_put(hpriv->clk);
+	ahci_put_clks(hpriv);
 	return rc;
 }
 
@@ -239,10 +280,8 @@ static void ahci_host_stop(struct ata_host *host)
 	if (pdata && pdata->exit)
 		pdata->exit(dev);
 
-	if (!IS_ERR(hpriv->clk)) {
-		clk_disable_unprepare(hpriv->clk);
-		clk_put(hpriv->clk);
-	}
+	ahci_disable_clks(hpriv);
+	ahci_put_clks(hpriv);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -277,8 +316,7 @@ static int ahci_suspend(struct device *dev)
 	if (pdata && pdata->suspend)
 		pdata->suspend(dev);
 
-	if (!IS_ERR(hpriv->clk))
-		clk_disable_unprepare(hpriv->clk);
+	ahci_disable_clks(hpriv);
 
 	return 0;
 }
@@ -290,13 +328,9 @@ static int ahci_resume(struct device *dev)
 	struct ahci_host_priv *hpriv = host->private_data;
 	int rc;
 
-	if (!IS_ERR(hpriv->clk)) {
-		rc = clk_prepare_enable(hpriv->clk);
-		if (rc) {
-			dev_err(dev, "clock prepare enable failed");
-			return rc;
-		}
-	}
+	rc = ahci_enable_clks(dev, hpriv);
+	if (rc)
+		return rc;
 
 	if (pdata && pdata->resume) {
 		rc = pdata->resume(dev);
@@ -320,8 +354,7 @@ pdata_suspend:
 	if (pdata && pdata->suspend)
 		pdata->suspend(dev);
 disable_unprepare_clk:
-	if (!IS_ERR(hpriv->clk))
-		clk_disable_unprepare(hpriv->clk);
+	ahci_disable_clks(hpriv);
 
 	return rc;
 }
-- 
1.8.4.2

Re: [RFC v3 06/13] ahci-platform: Add support for devices with more then 1 clock

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 12:48:48AM +0100, Hans de Goede wrote:
> +static int ahci_enable_clks(struct device *dev, struct ahci_host_priv *hpriv)
> +{
> +	int c, rc;
> +
> +	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) {

	for (c = 0; c < AHCI_MAX_CLKS && !IS_ERR(hpriv->clks[c]); c++) {

> +		rc = clk_prepare_enable(hpriv->clks[c]);
> +		if (rc) {
> +			dev_err(dev, "clock prepare enable failed");
> +			goto disable_unprepare_clk;
> +		}
> +	}
> +	return 0;
> +
> +disable_unprepare_clk:
> +	while (--c >= 0)
> +		clk_disable_unprepare(hpriv->clks[c]);
> +	return rc;
> +}
> +
> +static void ahci_disable_clks(struct ahci_host_priv *hpriv)
> +{
> +	int c;
> +
> +	for (c = AHCI_MAX_CLKS - 1; c >= 0; c--)
> +		if (hpriv->clks[c])

		if (!IS_ERR(hpriv->clks[c]))

> +			clk_disable_unprepare(hpriv->clks[c]);
> +}
> +
> +static void ahci_put_clks(struct ahci_host_priv *hpriv)
> +{
> +	int c;
> +
> +	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++)

	for (c = 0; c < AHCI_MAX_CLKS && !IS_ERR(hpriv->clks[c]); c++)

> +		clk_put(hpriv->clks[c]);
> +}

Better still for this one, consider using devm_clk_get() - in which case
the above is even more important to get right.  We really should have
a devm_of_clk_get() too.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [RFC v3 06/13] ahci-platform: Add support for devices with more then 1 clock

[Hans de Goede]

Hi,

On 01/19/2014 01:38 PM, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 12:48:48AM +0100, Hans de Goede wrote:
>> +static int ahci_enable_clks(struct device *dev, struct ahci_host_priv *hpriv)
>> +{
>> +	int c, rc;
>> +
>> +	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) {
>
> 	for (c = 0; c < AHCI_MAX_CLKS && !IS_ERR(hpriv->clks[c]); c++) {

That won't work, hpriv->clks == NULL for clks entries which are not used,
and before we get into a discussion about leaving any PTR_ERR returns
from clk_get in-place. I've had similar discussions when doing similar
changes to ohci-platform.c and ehci-platform.c and there the conclusion
was that "if (clk)" is just much more nice to read then "if (!IS_ERR(clk))",
I would like to avoid having the same discussion again.

More-over all clk_foo() methods check for and will safely handle clk == NULL,
and will crash and burn with clk == IS_ERR(clk).

I've chosen to still explicitly check for clk == NULL as that makes it much more
clear when reading the code that clk maybe NULL.


>> +		rc = clk_prepare_enable(hpriv->clks[c]);
>> +		if (rc) {
>> +			dev_err(dev, "clock prepare enable failed");
>> +			goto disable_unprepare_clk;
>> +		}
>> +	}
>> +	return 0;
>> +
>> +disable_unprepare_clk:
>> +	while (--c >= 0)
>> +		clk_disable_unprepare(hpriv->clks[c]);
>> +	return rc;
>> +}
>> +
>> +static void ahci_disable_clks(struct ahci_host_priv *hpriv)
>> +{
>> +	int c;
>> +
>> +	for (c = AHCI_MAX_CLKS - 1; c >= 0; c--)
>> +		if (hpriv->clks[c])
>
> 		if (!IS_ERR(hpriv->clks[c]))
>

Idem.

>> +			clk_disable_unprepare(hpriv->clks[c]);
>> +}
>> +
>> +static void ahci_put_clks(struct ahci_host_priv *hpriv)
>> +{
>> +	int c;
>> +
>> +	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++)
>
> 	for (c = 0; c < AHCI_MAX_CLKS && !IS_ERR(hpriv->clks[c]); c++)
>

Idem.

>> +		clk_put(hpriv->clks[c]);
>> +}
>
> Better still for this one, consider using devm_clk_get() - in which case
> the above is even more important to get right.

The above depends on how errors are handled when calling clk_get (or variants),
which in the case of this patch is such that hpriv->clks[i] == NULL when not
present.

> We really should have a devm_of_clk_get() too.

Agreed, but that seems something for another patch-set, this one is big
enough as is.

Regards,

Hans

[RFC v3 07/13] ahci-platform: Add support for an optional regulator for sata-target power

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/ata/ahci-platform.txt      |  1 +
 drivers/ata/ahci.h                                 |  2 ++
 drivers/ata/ahci_platform.c                        | 39 +++++++++++++++++++---
 3 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 3ced07d..1ac807f 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -11,6 +11,7 @@ Required properties:
 Optional properties:
 - dma-coherent      : Present if dma operations are coherent
 - clocks            : a list of phandle + clock specifier pairs
+- target-supply     : regulator for SATA target power
 
 Example:
         sata@ffe08000 {
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 7950b3a..c3e0c49 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -36,6 +36,7 @@
 #define _AHCI_H
 
 #include <linux/clk.h>
+#include <linux/regulator/consumer.h>
 #include <linux/libata.h>
 
 /* Enclosure Management Control */
@@ -323,6 +324,7 @@ struct ahci_host_priv {
 	u32			em_buf_sz;	/* EM buffer size in byte */
 	u32			em_msg_type;	/* EM message type */
 	struct clk		*clks[AHCI_MAX_CLKS]; /* Optional */
+	struct regulator	*target_pwr;	/* Optional */
 	void			*plat_data;	/* Other platform data */
 	/* Optional ahci_start_engine override */
 	void			(*start_engine)(struct ata_port *ap);
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 75a3d47..3bc2dab 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -181,6 +181,13 @@ static int ahci_probe(struct platform_device *pdev)
 		hpriv->clks[i] = clk;
 	}
 
+	hpriv->target_pwr = devm_regulator_get_optional(dev, "target");
+	if (IS_ERR(hpriv->target_pwr)) {
+		if (PTR_ERR(hpriv->target_pwr) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		hpriv->target_pwr = NULL;
+	}
+
 	rc = ahci_enable_clks(dev, hpriv);
 	if (rc)
 		goto free_clk;
@@ -197,6 +204,12 @@ static int ahci_probe(struct platform_device *pdev)
 			goto disable_unprepare_clk;
 	}
 
+	if (hpriv->target_pwr) {
+		rc = regulator_enable(hpriv->target_pwr);
+		if (rc)
+			goto pdata_exit;
+	}
+
 	ahci_save_initial_config(dev, hpriv,
 		pdata ? pdata->force_port_map : 0,
 		pdata ? pdata->mask_port_map  : 0);
@@ -220,7 +233,7 @@ static int ahci_probe(struct platform_device *pdev)
 	host = ata_host_alloc_pinfo(dev, ppi, n_ports);
 	if (!host) {
 		rc = -ENOMEM;
-		goto pdata_exit;
+		goto disable_regulator;
 	}
 
 	host->private_data = hpriv;
@@ -250,7 +263,7 @@ static int ahci_probe(struct platform_device *pdev)
 
 	rc = ahci_reset_controller(host);
 	if (rc)
-		goto pdata_exit;
+		goto disable_regulator;
 
 	ahci_init_controller(host);
 	ahci_print_info(host, "platform");
@@ -258,9 +271,12 @@ static int ahci_probe(struct platform_device *pdev)
 	rc = ata_host_activate(host, irq, ahci_interrupt, IRQF_SHARED,
 			       &ahci_platform_sht);
 	if (rc)
-		goto pdata_exit;
+		goto disable_regulator;
 
 	return 0;
+disable_regulator:
+	if (hpriv->target_pwr)
+		regulator_disable(hpriv->target_pwr);
 pdata_exit:
 	if (pdata && pdata->exit)
 		pdata->exit(dev);
@@ -277,6 +293,9 @@ static void ahci_host_stop(struct ata_host *host)
 	struct ahci_platform_data *pdata = dev_get_platdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
 
+	if (hpriv->target_pwr)
+		regulator_disable(hpriv->target_pwr);
+
 	if (pdata && pdata->exit)
 		pdata->exit(dev);
 
@@ -313,6 +332,9 @@ static int ahci_suspend(struct device *dev)
 	if (rc)
 		return rc;
 
+	if (hpriv->target_pwr)
+		regulator_disable(hpriv->target_pwr);
+
 	if (pdata && pdata->suspend)
 		pdata->suspend(dev);
 
@@ -338,10 +360,16 @@ static int ahci_resume(struct device *dev)
 			goto disable_unprepare_clk;
 	}
 
+	if (hpriv->target_pwr) {
+		rc = regulator_enable(hpriv->target_pwr);
+		if (rc)
+			goto pdata_suspend;
+	}
+
 	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
 		rc = ahci_reset_controller(host);
 		if (rc)
-			goto pdata_suspend;
+			goto disable_regulator;
 
 		ahci_init_controller(host);
 	}
@@ -350,6 +378,9 @@ static int ahci_resume(struct device *dev)
 
 	return 0;
 
+disable_regulator:
+	if (hpriv->target_pwr)
+		regulator_disable(hpriv->target_pwr);
 pdata_suspend:
 	if (pdata && pdata->suspend)
 		pdata->suspend(dev);
-- 
1.8.4.2

[RFC v3 08/13] ahci-platform: Allow specifying platform_data through of_device_id

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/ata/ahci_platform.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 3bc2dab..0676d72 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -20,6 +20,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/device.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/libata.h>
 #include <linux/ahci_platform.h>
@@ -87,6 +88,30 @@ static struct scsi_host_template ahci_platform_sht = {
 	AHCI_SHT("ahci_platform"),
 };
 
+static const struct of_device_id ahci_of_match[] = {
+	{ .compatible = "snps,spear-ahci", },
+	{ .compatible = "snps,exynos5440-ahci", },
+	{ .compatible = "ibm,476gtr-ahci", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ahci_of_match);
+
+static const struct ahci_platform_data *ahci_get_pdata(struct device *dev)
+{
+	struct ahci_platform_data *pdata;
+	const struct of_device_id *of_id;
+
+	pdata = dev_get_platdata(dev);
+	if (pdata)
+		return pdata;
+
+	of_id = of_match_device(ahci_of_match, dev);
+	if (of_id)
+		return of_id->data;
+
+	return NULL;
+}
+
 static int ahci_enable_clks(struct device *dev, struct ahci_host_priv *hpriv)
 {
 	int c, rc;
@@ -126,7 +151,7 @@ static void ahci_put_clks(struct ahci_host_priv *hpriv)
 static int ahci_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct ahci_platform_data *pdata = dev_get_platdata(dev);
+	const struct ahci_platform_data *pdata = ahci_get_pdata(dev);
 	const struct platform_device_id *id = platform_get_device_id(pdev);
 	struct ata_port_info pi = ahci_port_info[id ? id->driver_data : 0];
 	const struct ata_port_info *ppi[] = { &pi, NULL };
@@ -290,7 +315,7 @@ free_clk:
 static void ahci_host_stop(struct ata_host *host)
 {
 	struct device *dev = host->dev;
-	struct ahci_platform_data *pdata = dev_get_platdata(dev);
+	const struct ahci_platform_data *pdata = ahci_get_pdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
 
 	if (hpriv->target_pwr)
@@ -306,7 +331,7 @@ static void ahci_host_stop(struct ata_host *host)
 #ifdef CONFIG_PM_SLEEP
 static int ahci_suspend(struct device *dev)
 {
-	struct ahci_platform_data *pdata = dev_get_platdata(dev);
+	const struct ahci_platform_data *pdata = ahci_get_pdata(dev);
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
 	void __iomem *mmio = hpriv->mmio;
@@ -345,7 +370,7 @@ static int ahci_suspend(struct device *dev)
 
 static int ahci_resume(struct device *dev)
 {
-	struct ahci_platform_data *pdata = dev_get_platdata(dev);
+	const struct ahci_platform_data *pdata = ahci_get_pdata(dev);
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
 	int rc;
@@ -393,14 +418,6 @@ disable_unprepare_clk:
 
 static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_suspend, ahci_resume);
 
-static const struct of_device_id ahci_of_match[] = {
-	{ .compatible = "snps,spear-ahci", },
-	{ .compatible = "snps,exynos5440-ahci", },
-	{ .compatible = "ibm,476gtr-ahci", },
-	{},
-};
-MODULE_DEVICE_TABLE(of, ahci_of_match);
-
 static struct platform_driver ahci_driver = {
 	.probe = ahci_probe,
 	.remove = ata_platform_remove_one,
-- 
1.8.4.2

Re: [RFC v3 08/13] ahci-platform: Allow specifying platform_data through of_device_id

[Tejun Heo]

On Sun, Jan 19, 2014 at 12:48:50AM +0100, Hans de Goede wrote:
> @@ -126,7 +151,7 @@ static void ahci_put_clks(struct ahci_host_priv *hpriv)
>  static int ahci_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct ahci_platform_data *pdata = dev_get_platdata(dev);
> +	const struct ahci_platform_data *pdata = ahci_get_pdata(dev);

Let's please not add const.  For data types which are known to be
terminal, const more or less works, I suppose but for anything even
mildly complicated it ends up being more of a headache.  C just
doesn't have enough language support to make const actually useful.
e.g. now if somebody wants to add an accessor to ahci_platform_data()
which is applicable to both readers and writers, we either need two
separate accessors for const and !const paths or have to cast away
const.

Thanks.

-- 
tejun

Re: [RFC v3 08/13] ahci-platform: Allow specifying platform_data through of_device_id

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 06:38:38AM -0500, Tejun Heo wrote:
> On Sun, Jan 19, 2014 at 12:48:50AM +0100, Hans de Goede wrote:
> > @@ -126,7 +151,7 @@ static void ahci_put_clks(struct ahci_host_priv *hpriv)
> >  static int ahci_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > -	struct ahci_platform_data *pdata = dev_get_platdata(dev);
> > +	const struct ahci_platform_data *pdata = ahci_get_pdata(dev);
> 
> Let's please not add const.  For data types which are known to be
> terminal, const more or less works, I suppose but for anything even
> mildly complicated it ends up being more of a headache.  C just
> doesn't have enough language support to make const actually useful.
> e.g. now if somebody wants to add an accessor to ahci_platform_data()
> which is applicable to both readers and writers, we either need two
> separate accessors for const and !const paths or have to cast away
> const.

I don't see anything wrong with this.  Platform data should _never_ be
written to, because doing so will change the driver behaviour between
bindings.  It really should be read-only to driver code.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [RFC v3 08/13] ahci-platform: Allow specifying platform_data through of_device_id

[Tejun Heo]

On Sun, Jan 19, 2014 at 12:30:55PM +0000, Russell King - ARM Linux wrote:
> I don't see anything wrong with this.  Platform data should _never_ be
> written to, because doing so will change the driver behaviour between
> bindings.  It really should be read-only to driver code.

Hmmm, problems usually arise when const is used to distinguish ro and
rw users.  When it's simple, it seems okay but later on it often ends
up requiring dropping const in almost arbitrary places or forced casts
somewhere random.  Over time, it makes const annotations in the kernel
sparse and inconsistent.  For anything non-trivial, I think it's best
to ignore it.

That said, if the object is actually immutable once initialized, it
shouldn't cause any trouble.  That probably is one of the few proper
use case for const on complex data types.

Thanks.

-- 
tejun

Re: [RFC v3 08/13] ahci-platform: Allow specifying platform_data through of_device_id

[Hans de Goede]

Hi,

On 01/19/2014 12:38 PM, Tejun Heo wrote:
> On Sun, Jan 19, 2014 at 12:48:50AM +0100, Hans de Goede wrote:
>> @@ -126,7 +151,7 @@ static void ahci_put_clks(struct ahci_host_priv *hpriv)
>>   static int ahci_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> -	struct ahci_platform_data *pdata = dev_get_platdata(dev);
>> +	const struct ahci_platform_data *pdata = ahci_get_pdata(dev);
>
> Let's please not add const.  For data types which are known to be
> terminal, const more or less works, I suppose but for anything even
> mildly complicated it ends up being more of a headache.  C just
> doesn't have enough language support to make const actually useful.
> e.g. now if somebody wants to add an accessor to ahci_platform_data()
> which is applicable to both readers and writers, we either need two
> separate accessors for const and !const paths or have to cast away
> const.

The problem is that:

1) of_match_device returns const, so without the const the code would need
to cast that const away somewhere
2) of_match_device is right to return const because
2a) the data can actually be const
2b) even if not const, it is shared between multiple instances of the same
device-type and thus should never be written too

So as Russell already said, the use of const is correct here, and the best
thing to do is to simply keep it.

Regards,

Hans

Re: [RFC v3 08/13] ahci-platform: Allow specifying platform_data through of_device_id

[Tejun Heo]

On Sun, Jan 19, 2014 at 07:56:53PM +0100, Hans de Goede wrote:
> 1) of_match_device returns const, so without the const the code would need
> to cast that const away somewhere
> 2) of_match_device is right to return const because
> 2a) the data can actually be const
> 2b) even if not const, it is shared between multiple instances of the same
> device-type and thus should never be written too

Sure, if it has already gone that way, keeping it on is probably the
easiest way.

> So as Russell already said, the use of const is correct here, and the best
> thing to do is to simply keep it.

It's not about whether this specific annotation is correct or not.
It's that C as a language doesn't have good enough const support to
have proper const annotations and tends to lead to practical problems
often making the trade-off unclear in complex cases.

Anyways, as written above, if it's propagation of existing ones and
doesn't have mixed ro/rw usages, keeping it on probably is the eaiest.

Thanks.

-- 
tejun

Re: [RFC v3 08/13] ahci-platform: Allow specifying platform_data through of_device_id

[Sascha Hauer]

On Sun, Jan 19, 2014 at 12:48:50AM +0100, Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/ata/ahci_platform.c | 41 +++++++++++++++++++++++++++++------------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index 3bc2dab..0676d72 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
> @@ -20,6 +20,7 @@
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/device.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/libata.h>
>  #include <linux/ahci_platform.h>
> @@ -87,6 +88,30 @@ static struct scsi_host_template ahci_platform_sht = {
>  	AHCI_SHT("ahci_platform"),
>  };
>  
> +static const struct of_device_id ahci_of_match[] = {
> +	{ .compatible = "snps,spear-ahci", },
> +	{ .compatible = "snps,exynos5440-ahci", },
> +	{ .compatible = "ibm,476gtr-ahci", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ahci_of_match);
> +
> +static const struct ahci_platform_data *ahci_get_pdata(struct device *dev)
> +{
> +	struct ahci_platform_data *pdata;
> +	const struct of_device_id *of_id;
> +
> +	pdata = dev_get_platdata(dev);
> +	if (pdata)
> +		return pdata;
> +
> +	of_id = of_match_device(ahci_of_match, dev);
> +	if (of_id)
> +		return of_id->data;

I don't think it's a good idea to force of_id->data to be of type struct
struct ahci_platform_data *. With this we don't have a place to store
SoC specific data anymore.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC v3 08/13] ahci-platform: Allow specifying platform_data through of_device_id

[Hans de Goede]

Hi,

On 01/20/2014 09:24 AM, Sascha Hauer wrote:
> On Sun, Jan 19, 2014 at 12:48:50AM +0100, Hans de Goede wrote:
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>   drivers/ata/ahci_platform.c | 41 +++++++++++++++++++++++++++++------------
>>   1 file changed, 29 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
>> index 3bc2dab..0676d72 100644
>> --- a/drivers/ata/ahci_platform.c
>> +++ b/drivers/ata/ahci_platform.c
>> @@ -20,6 +20,7 @@
>>   #include <linux/init.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/device.h>
>> +#include <linux/of_device.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/libata.h>
>>   #include <linux/ahci_platform.h>
>> @@ -87,6 +88,30 @@ static struct scsi_host_template ahci_platform_sht = {
>>   	AHCI_SHT("ahci_platform"),
>>   };
>>
>> +static const struct of_device_id ahci_of_match[] = {
>> +	{ .compatible = "snps,spear-ahci", },
>> +	{ .compatible = "snps,exynos5440-ahci", },
>> +	{ .compatible = "ibm,476gtr-ahci", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ahci_of_match);
>> +
>> +static const struct ahci_platform_data *ahci_get_pdata(struct device *dev)
>> +{
>> +	struct ahci_platform_data *pdata;
>> +	const struct of_device_id *of_id;
>> +
>> +	pdata = dev_get_platdata(dev);
>> +	if (pdata)
>> +		return pdata;
>> +
>> +	of_id = of_match_device(ahci_of_match, dev);
>> +	if (of_id)
>> +		return of_id->data;
>
> I don't think it's a good idea to force of_id->data to be of type struct
> struct ahci_platform_data *. With this we don't have a place to store
> SoC specific data anymore.

?? ahci_platform_data *is* soc specific data, it allows various soc
specific overrides.

Regards,

Hans

Re: [RFC v3 08/13] ahci-platform: Allow specifying platform_data through of_device_id

[Sascha Hauer]

On Mon, Jan 20, 2014 at 09:35:06AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 01/20/2014 09:24 AM, Sascha Hauer wrote:
> >>+
> >>+static const struct ahci_platform_data *ahci_get_pdata(struct device *dev)
> >>+{
> >>+	struct ahci_platform_data *pdata;
> >>+	const struct of_device_id *of_id;
> >>+
> >>+	pdata = dev_get_platdata(dev);
> >>+	if (pdata)
> >>+		return pdata;
> >>+
> >>+	of_id = of_match_device(ahci_of_match, dev);
> >>+	if (of_id)
> >>+		return of_id->data;
> >
> >I don't think it's a good idea to force of_id->data to be of type struct
> >struct ahci_platform_data *. With this we don't have a place to store
> >SoC specific data anymore.
> 
> ?? ahci_platform_data *is* soc specific data, it allows various soc
> specific overrides.

I know, but it might not be enough for encding the slight differences
between i.MX53 and i.MX6. So you say then we would need to different
instances of struct ahci_platform_data, one for i.MX53 and one for
i.MX6. Ok, that works.

Overall I must say that I'm not really happy with giving up control over
the probe function and putting ahci_platform as a midlayer between the
SoC and the ahci lib. Just my 2 cents, if I'm the only one feel free to
ignore me, but maybe there are others that have the same feeling.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC v3 08/13] ahci-platform: Allow specifying platform_data through of_device_id

[Hans de Goede]

Hi,

On 01/20/2014 10:09 AM, Sascha Hauer wrote:
> On Mon, Jan 20, 2014 at 09:35:06AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 01/20/2014 09:24 AM, Sascha Hauer wrote:
>>>> +
>>>> +static const struct ahci_platform_data *ahci_get_pdata(struct device *dev)
>>>> +{
>>>> +	struct ahci_platform_data *pdata;
>>>> +	const struct of_device_id *of_id;
>>>> +
>>>> +	pdata = dev_get_platdata(dev);
>>>> +	if (pdata)
>>>> +		return pdata;
>>>> +
>>>> +	of_id = of_match_device(ahci_of_match, dev);
>>>> +	if (of_id)
>>>> +		return of_id->data;
>>>
>>> I don't think it's a good idea to force of_id->data to be of type struct
>>> struct ahci_platform_data *. With this we don't have a place to store
>>> SoC specific data anymore.
>>
>> ?? ahci_platform_data *is* soc specific data, it allows various soc
>> specific overrides.
>
> I know, but it might not be enough for encding the slight differences
> between i.MX53 and i.MX6. So you say then we would need to different
> instances of struct ahci_platform_data, one for i.MX53 and one for
> i.MX6. Ok, that works.
>
> Overall I must say that I'm not really happy with giving up control over
> the probe function and putting ahci_platform as a midlayer between the
> SoC and the ahci lib. Just my 2 cents, if I'm the only one feel free to
> ignore me, but maybe there are others that have the same feeling.

I'm currently working on a slightly different implementation of a more
generic ahci_platform.c where ahci_platform.c exports some standard platform
related functionality as library functions.

Drivers which need to override some of ahci_platform.c's behavior because of
non standard hw, will then export their own struct platform_driver and can
call into the exported functions for standard stuff to avoid code duplication
where appropriate, while still having 100% freedom to do things in a custom
way where necessary.

I hope to post a PATCH RFC v4 with these changes later today, which you will
hopefully like better. Input on v4, even just a "yep better" remark would be
much appreciated.

Regards,

Hans

Re: [RFC v3 08/13] ahci-platform: Allow specifying platform_data through of_device_id

[Sascha Hauer]

On Mon, Jan 20, 2014 at 10:17:24AM +0100, Hans de Goede wrote:
> Hi,
> 
> I'm currently working on a slightly different implementation of a more
> generic ahci_platform.c where ahci_platform.c exports some standard platform
> related functionality as library functions.
> 
> Drivers which need to override some of ahci_platform.c's behavior because of
> non standard hw, will then export their own struct platform_driver and can
> call into the exported functions for standard stuff to avoid code duplication
> where appropriate, while still having 100% freedom to do things in a custom
> way where necessary.

Nice, thanks.

> 
> I hope to post a PATCH RFC v4 with these changes later today, which you will
> hopefully like better. Input on v4, even just a "yep better" remark would be
> much appreciated.

Yes, I will give feedback on v4.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC v3 09/13] ARM: sunxi: Add support for Allwinner SUNXi SoCs sata to ahci_platform

[Hans de Goede]

From: Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>

This patch adds support for the ahci sata controler found on Allwinner A10
and A20 SoCs to the ahci_platform driver.

Orignally written by Olliver Schinagl using the approach of having a platform
device which probe method creates a new child platform device which gets
driven by ahci_platform.c, as done by ahci_imx.c .

Refactored by Hans de Goede to add most of the non sunxi specific functionality
to ahci_platform.c and use a platform_data pointer from of_device_id for the
sunxi specific bits.

Signed-off-by: Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/ata/ahci-platform.txt      |  18 +-
 drivers/ata/Kconfig                                |   9 +
 drivers/ata/Makefile                               |   4 +-
 drivers/ata/ahci_platform.c                        |   4 +
 drivers/ata/ahci_sunxi.c                           | 182 +++++++++++++++++++++
 include/linux/ahci_platform.h                      |   2 +
 6 files changed, 215 insertions(+), 4 deletions(-)
 create mode 100644 drivers/ata/ahci_sunxi.c

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 1ac807f..f036e786 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -4,7 +4,9 @@ SATA nodes are defined to describe on-chip Serial ATA controllers.
 Each SATA controller should have its own node.
 
 Required properties:
-- compatible        : compatible list, contains "snps,spear-ahci"
+- compatible        : compatible list, one of "snps,spear-ahci",
+                      "snps,exynos5440-ahci", "ibm,476gtr-ahci", or
+                      "allwinner,sun4i-a10-ahci"
 - interrupts        : <interrupt mapping for SATA IRQ>
 - reg               : <registers mapping>
 
@@ -13,10 +15,20 @@ Optional properties:
 - clocks            : a list of phandle + clock specifier pairs
 - target-supply     : regulator for SATA target power
 
-Example:
+allwinner,sun4i-a10-ahci required properties:
+- clocks            : index 0 must point to the sata_ref clk, 1 to the ahb clk
+
+Examples:
         sata@ffe08000 {
 		compatible = "snps,spear-ahci";
 		reg = <0xffe08000 0x1000>;
 		interrupts = <115>;
-
         };
+
+	sata: ahci@01c18000 {
+		compatible = "allwinner,sun4i-a10-ahci";
+		reg = <0x01c18000 0x1000>;
+		interrupts = <56>;
+		clocks = <&pll6 0>, <&ahb_gates 25>;
+		target-supply = <&reg_ahci_5v>;
+	};
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 4e73772..5f6c11a 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -106,6 +106,15 @@ config AHCI_IMX
 
 	  If unsure, say N.
 
+config AHCI_SUNXI
+	bool "Allwinner sunxi AHCI SATA support"
+	depends on ARCH_SUNXI && SATA_AHCI_PLATFORM
+	help
+	  This option enables support for the Allwinner sunxi SoC's
+	  onboard AHCI SATA in the ahci_platform driver.
+
+	  If unsure, say N.
+
 config SATA_FSL
 	tristate "Freescale 3.0Gbps SATA support"
 	depends on FSL_SOC
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 46518c6..4d8778c 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -4,7 +4,7 @@ obj-$(CONFIG_ATA)		+= libata.o
 # non-SFF interface
 obj-$(CONFIG_SATA_AHCI)		+= ahci.o libahci.o
 obj-$(CONFIG_SATA_ACARD_AHCI)	+= acard-ahci.o libahci.o
-obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o
+obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_plat.o libahci.o
 obj-$(CONFIG_SATA_FSL)		+= sata_fsl.o
 obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
 obj-$(CONFIG_SATA_SIL24)	+= sata_sil24.o
@@ -12,6 +12,8 @@ obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
 obj-$(CONFIG_SATA_HIGHBANK)	+= sata_highbank.o libahci.o
 obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o
 
+ahci_plat-objs := ahci_platform.o ahci_sunxi.o
+
 # SFF w/ custom DMA
 obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o
 obj-$(CONFIG_PATA_ARASAN_CF)	+= pata_arasan_cf.o
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 0676d72..324d066 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -92,6 +92,10 @@ static const struct of_device_id ahci_of_match[] = {
 	{ .compatible = "snps,spear-ahci", },
 	{ .compatible = "snps,exynos5440-ahci", },
 	{ .compatible = "ibm,476gtr-ahci", },
+#ifdef CONFIG_AHCI_SUNXI
+	{ .compatible = "allwinner,sun4i-a10-ahci",
+	  .data = &ahci_sunxi_pdata, },
+#endif
 	{},
 };
 MODULE_DEVICE_TABLE(of, ahci_of_match);
diff --git a/drivers/ata/ahci_sunxi.c b/drivers/ata/ahci_sunxi.c
new file mode 100644
index 0000000..cf357bf
--- /dev/null
+++ b/drivers/ata/ahci_sunxi.c
@@ -0,0 +1,182 @@
+/*
+ * Allwinner sunxi AHCI SATA platform driver
+ * Copyright 2013 Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
+ * Copyright 2014 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
+ *
+ * based on the AHCI SATA platform driver by Jeff Garzik and Anton Vorontsov
+ * Based on code from Allwinner Technology Co., Ltd. <www.allwinnertech.com>,
+ * Daniel Wang <danielwang-0TFLnhJekD6UEPyfVivIlAC/G2K4zDHf@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/ahci_platform.h>
+#include <linux/clk.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include "ahci.h"
+
+#define AHCI_BISTAFR 0x00a0
+#define AHCI_BISTCR 0x00a4
+#define AHCI_BISTFCTR 0x00a8
+#define AHCI_BISTSR 0x00ac
+#define AHCI_BISTDECR 0x00b0
+#define AHCI_DIAGNR0 0x00b4
+#define AHCI_DIAGNR1 0x00b8
+#define AHCI_OOBR 0x00bc
+#define AHCI_PHYCS0R 0x00c0
+#define AHCI_PHYCS1R 0x00c4
+#define AHCI_PHYCS2R 0x00c8
+#define AHCI_TIMER1MS 0x00e0
+#define AHCI_GPARAM1R 0x00e8
+#define AHCI_GPARAM2R 0x00ec
+#define AHCI_PPARAMR 0x00f0
+#define AHCI_TESTR 0x00f4
+#define AHCI_VERSIONR 0x00f8
+#define AHCI_IDR 0x00fc
+#define AHCI_RWCR 0x00fc
+#define AHCI_P0DMACR 0x0170
+#define AHCI_P0PHYCR 0x0178
+#define AHCI_P0PHYSR 0x017c
+
+#ifdef CONFIG_AHCI_SUNXI
+
+static void sunxi_clrbits(void __iomem *reg, u32 clr_val)
+{
+	u32 reg_val;
+
+	reg_val = readl(reg);
+	reg_val &= ~(clr_val);
+	writel(reg_val, reg);
+}
+
+static void sunxi_setbits(void __iomem *reg, u32 set_val)
+{
+	u32 reg_val;
+
+	reg_val = readl(reg);
+	reg_val |= set_val;
+	writel(reg_val, reg);
+}
+
+static void sunxi_clrsetbits(void __iomem *reg, u32 clr_val, u32 set_val)
+{
+	u32 reg_val;
+
+	reg_val = readl(reg);
+	reg_val &= ~(clr_val);
+	reg_val |= set_val;
+	writel(reg_val, reg);
+}
+
+static u32 sunxi_getbits(void __iomem *reg, u8 mask, u8 shift)
+{
+	return (readl(reg) >> shift) & mask;
+}
+
+static int ahci_sunxi_phy_init(struct device *dev, void __iomem *reg_base)
+{
+	u32 reg_val;
+	int timeout;
+
+	/* This magic is from the original code */
+	writel(0, reg_base + AHCI_RWCR);
+	mdelay(5);
+
+	sunxi_setbits(reg_base + AHCI_PHYCS1R, BIT(19));
+	sunxi_clrsetbits(reg_base + AHCI_PHYCS0R,
+			 (0x7 << 24),
+			 (0x5 << 24) | BIT(23) | BIT(18));
+	sunxi_clrsetbits(reg_base + AHCI_PHYCS1R,
+			 (0x3 << 16) | (0x1f << 8) | (0x3 << 6),
+			 (0x2 << 16) | (0x6 << 8) | (0x2 << 6));
+	sunxi_setbits(reg_base + AHCI_PHYCS1R, BIT(28) | BIT(15));
+	sunxi_clrbits(reg_base + AHCI_PHYCS1R, BIT(19));
+	sunxi_clrsetbits(reg_base + AHCI_PHYCS0R,
+			 (0x7 << 20), (0x3 << 20));
+	sunxi_clrsetbits(reg_base + AHCI_PHYCS2R,
+			 (0x1f << 5), (0x19 << 5));
+	mdelay(5);
+
+	sunxi_setbits(reg_base + AHCI_PHYCS0R, (0x1 << 19));
+
+	timeout = 0x100000;
+	do {
+		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS0R, 0x7, 28);
+	} while (--timeout && (reg_val != 0x2));
+	if (!timeout) {
+		dev_err(dev, "PHY power up failed.\n");
+		return -EIO;
+	}
+
+	sunxi_setbits(reg_base + AHCI_PHYCS2R, (0x1 << 24));
+
+	timeout = 0x100000;
+	do {
+		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS2R, 0x1, 24);
+	} while (--timeout && reg_val);
+	if (!timeout) {
+		dev_err(dev, "PHY calibration failed.\n");
+		return -EIO;
+	}
+	mdelay(15);
+
+	writel(0x7, reg_base + AHCI_RWCR);
+
+	return 0;
+}
+
+static void ahci_sunxi_start_engine(struct ata_port *ap)
+{
+	void __iomem *port_mmio = ahci_port_base(ap);
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+
+	/* Setup DMA before DMA start */
+	sunxi_clrsetbits(hpriv->mmio + AHCI_P0DMACR, 0x0000ff00, 0x00004400);
+
+	/* Start DMA */
+	sunxi_setbits(port_mmio + PORT_CMD, PORT_CMD_START);
+}
+
+static int ahci_sunxi_init(struct device *dev, struct ahci_host_priv *hpriv,
+			   void __iomem *reg_base)
+{
+	hpriv->start_engine = ahci_sunxi_start_engine;
+	return ahci_sunxi_phy_init(dev, reg_base);
+}
+
+int ahci_sunxi_resume(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+
+	return ahci_sunxi_phy_init(dev, hpriv->mmio);
+}
+
+static const struct ata_port_info ahci_sunxi_port_info = {
+	AHCI_HFLAGS(AHCI_HFLAG_32BIT_ONLY | AHCI_HFLAG_NO_MSI |
+			  AHCI_HFLAG_NO_PMP | AHCI_HFLAG_YES_NCQ),
+	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NCQ,
+	.pio_mask	= ATA_PIO4,
+	.udma_mask	= ATA_UDMA6,
+	.port_ops	= &ahci_platform_ops,
+};
+
+struct ahci_platform_data ahci_sunxi_pdata = {
+	.init = ahci_sunxi_init,
+	.resume = ahci_sunxi_resume,
+	.ata_port_info = &ahci_sunxi_port_info,
+};
+
+#endif
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 796dfb4..4e6cbe0 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -32,4 +32,6 @@ struct ahci_platform_data {
 	unsigned int mask_port_map;
 };
 
+extern struct ahci_platform_data ahci_sunxi_pdata;
+
 #endif /* _AHCI_PLATFORM_H */
-- 
1.8.4.2

Re: [RFC v3 09/13] ARM: sunxi: Add support for Allwinner SUNXi SoCs sata to ahci_platform

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 12:48:51AM +0100, Hans de Goede wrote:
> +	timeout = 0x100000;
> +	do {
> +		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS0R, 0x7, 28);
> +	} while (--timeout && (reg_val != 0x2));
> +	if (!timeout) {
> +		dev_err(dev, "PHY power up failed.\n");
> +		return -EIO;
> +	}

This is not a good way to detect failure - there's several things wrong
here.

First, how long does sunxi_getbits() take?  What does that depend on?
Therefore, how long does it take to time out?

Secondly, what if the success condition becomes true at the same time that
a timeout occurs?

So:

	timeout = some_us_value;
	do {
		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS0R, 0x7, 28);
		if (reg_val == 2)
			break;

		timeout--;
		if (timeout == 0) {
			dev_err(dev, "PHY power up failed.\n");
			return -EIO;
		}

		udelay(1);
	} while (1);

is far more predictable.  Same goes for the other loop in this function.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [RFC v3 09/13] ARM: sunxi: Add support for Allwinner SUNXi SoCs sata to ahci_platform

[Hans de Goede]

Hi,

On 01/19/2014 01:22 PM, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 12:48:51AM +0100, Hans de Goede wrote:
>> +	timeout = 0x100000;
>> +	do {
>> +		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS0R, 0x7, 28);
>> +	} while (--timeout && (reg_val != 0x2));
>> +	if (!timeout) {
>> +		dev_err(dev, "PHY power up failed.\n");
>> +		return -EIO;
>> +	}
>
> This is not a good way to detect failure - there's several things wrong
> here.
>
> First, how long does sunxi_getbits() take?  What does that depend on?
> Therefore, how long does it take to time out?

You're interpreting the timeout in the above code as an actual timeout, but
that is not what it is, it is a means to avoid looping forever if something
is seriously amiss. The only time I've ever seen the timeout trigger is when
I forgot to enable some clks iirc.

I can rename the variable from timeout to max_tries to make this more clear.

> Secondly, what if the success condition becomes true at the same time that
> a timeout occurs?

We should never get anywhere near timeout becoming 0, so if both happen at
the same time, then something is pretty seriously broken and the returning of
an error as the code does now is the right thing to do.

Regards,

Hans

Re: [RFC v3 09/13] ARM: sunxi: Add support for Allwinner SUNXi SoCs sata to ahci_platform

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 08:07:46PM +0100, Hans de Goede wrote:
> Hi,
>
> On 01/19/2014 01:22 PM, Russell King - ARM Linux wrote:
>> On Sun, Jan 19, 2014 at 12:48:51AM +0100, Hans de Goede wrote:
>>> +	timeout = 0x100000;
>>> +	do {
>>> +		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS0R, 0x7, 28);
>>> +	} while (--timeout && (reg_val != 0x2));
>>> +	if (!timeout) {
>>> +		dev_err(dev, "PHY power up failed.\n");
>>> +		return -EIO;
>>> +	}
>>
>> This is not a good way to detect failure - there's several things wrong
>> here.
>>
>> First, how long does sunxi_getbits() take?  What does that depend on?
>> Therefore, how long does it take to time out?
>
> You're interpreting the timeout in the above code as an actual timeout, but
> that is not what it is, it is a means to avoid looping forever if something
> is seriously amiss. The only time I've ever seen the timeout trigger is when
> I forgot to enable some clks iirc.
>
> I can rename the variable from timeout to max_tries to make this more clear.
>
>> Secondly, what if the success condition becomes true at the same time that
>> a timeout occurs?
>
> We should never get anywhere near timeout becoming 0, so if both happen at
> the same time, then something is pretty seriously broken and the returning of
> an error as the code does now is the right thing to do.

Yes... and if we look back in history, there's been lots of stuff just
like this where the loop has had to have the number of iterations
increased as CPUs have become faster and compilers become better?

So... my question stands: but let me put it a different way in two parts:

1. What is the maximum expected time for the success condition to be
   satisfied?
2. How long does it actually take for the loop to time out in existing
   CPUs/compilers?

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [RFC v3 09/13] ARM: sunxi: Add support for Allwinner SUNXi SoCs sata to ahci_platform

[Hans de Goede]

Hi,

On 01/19/2014 08:56 PM, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 08:07:46PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 01/19/2014 01:22 PM, Russell King - ARM Linux wrote:
>>> On Sun, Jan 19, 2014 at 12:48:51AM +0100, Hans de Goede wrote:
>>>> +	timeout = 0x100000;
>>>> +	do {
>>>> +		reg_val = sunxi_getbits(reg_base + AHCI_PHYCS0R, 0x7, 28);
>>>> +	} while (--timeout && (reg_val != 0x2));
>>>> +	if (!timeout) {
>>>> +		dev_err(dev, "PHY power up failed.\n");
>>>> +		return -EIO;
>>>> +	}
>>>
>>> This is not a good way to detect failure - there's several things wrong
>>> here.
>>>
>>> First, how long does sunxi_getbits() take?  What does that depend on?
>>> Therefore, how long does it take to time out?
>>
>> You're interpreting the timeout in the above code as an actual timeout, but
>> that is not what it is, it is a means to avoid looping forever if something
>> is seriously amiss. The only time I've ever seen the timeout trigger is when
>> I forgot to enable some clks iirc.
>>
>> I can rename the variable from timeout to max_tries to make this more clear.
>>
>>> Secondly, what if the success condition becomes true at the same time that
>>> a timeout occurs?
>>
>> We should never get anywhere near timeout becoming 0, so if both happen at
>> the same time, then something is pretty seriously broken and the returning of
>> an error as the code does now is the right thing to do.
>
> Yes... and if we look back in history, there's been lots of stuff just
> like this where the loop has had to have the number of iterations
> increased as CPUs have become faster and compilers become better?
>
> So... my question stands: but let me put it a different way in two parts:
>
> 1. What is the maximum expected time for the success condition to be
>     satisfied?

TBH, I don't have a clue this code comes from the android driver (never an
excuse, I know) and we don't have any documentation other then the android
driver.

> 2. How long does it actually take for the loop to time out in existing
>     CPUs/compilers?

I don't know either. I understand where you're coming from, and I believe
that the best way to solve this is to take your suggested implementation, start
with a way too high timeout, and add a debug print to see how much time is left
after a successfull phy_init, then do a couple of test runs to get a ballpark
figure for how long we need to wait, multiply that by 5 to be on the safe side,
and use that.

Does that sound like a plan ?

Regards,

Hans

[RFC v3 10/13] ahci_imx: Adjust for ahci_platform managing the clocks

[Hans de Goede]

ahci_platform manages all 3 clocks now, so enabling / disabling them from
ahci_imx just results in the sata_ref clock getting enabled / disabled twice.

Note untested, I've ordered a wandboard to be able to test these changes.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/ata/ahci-platform.txt      |  8 ++-
 drivers/ata/ahci_imx.c                             | 82 +++++++++-------------
 2 files changed, 40 insertions(+), 50 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index f036e786..ee3a127 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -5,8 +5,8 @@ Each SATA controller should have its own node.
 
 Required properties:
 - compatible        : compatible list, one of "snps,spear-ahci",
-                      "snps,exynos5440-ahci", "ibm,476gtr-ahci", or
-                      "allwinner,sun4i-a10-ahci"
+                      "snps,exynos5440-ahci", "ibm,476gtr-ahci",
+                      "allwinner,sun4i-a10-ahci" or "fsl,imx6q-ahci"
 - interrupts        : <interrupt mapping for SATA IRQ>
 - reg               : <registers mapping>
 
@@ -18,6 +18,10 @@ Optional properties:
 allwinner,sun4i-a10-ahci required properties:
 - clocks            : index 0 must point to the sata_ref clk, 1 to the ahb clk
 
+fsl,imx6q-ahci required properties:
+- clocks            : index 0 must point to the sataf clk, 1 to the sata_ref
+		      clk and 2 to the ahb clk
+
 Examples:
         sata@ffe08000 {
 		compatible = "snps,spear-ahci";
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index 0051f29..8eb24a3 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -34,12 +34,15 @@ enum {
 	HOST_TIMER1MS = 0xe0,			/* Timer 1-ms */
 };
 
+enum {
+	CLK_SATA,
+	CLK_SATA_REF,
+	CLK_AHB
+};
+
 struct imx_ahci_priv {
 	struct platform_device *ahci_pdev;
-	struct clk *sata_ref_clk;
-	struct clk *ahb_clk;
 	struct regmap *gpr;
-	bool no_device;
 	bool first_time;
 };
 
@@ -55,6 +58,7 @@ static void ahci_imx_error_handler(struct ata_port *ap)
 	struct ahci_host_priv *hpriv = host->private_data;
 	void __iomem *mmio = hpriv->mmio;
 	struct imx_ahci_priv *imxpriv = dev_get_drvdata(ap->dev->parent);
+	int i;
 
 	ahci_error_handler(ap);
 
@@ -75,8 +79,13 @@ static void ahci_imx_error_handler(struct ata_port *ap)
 	regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
 			IMX6Q_GPR13_SATA_MPLL_CLK_EN,
 			!IMX6Q_GPR13_SATA_MPLL_CLK_EN);
-	clk_disable_unprepare(imxpriv->sata_ref_clk);
-	imxpriv->no_device = true;
+
+	for (i = CLK_AHB; i >= 0; i--) {
+		clk_disable_unprepare(hpriv->clks[i]);
+		/* Stop ahci_platform.c from trying to use the clks */
+		clk_put(hpriv->clks[i]);
+		hpriv->clks[i] = NULL;
+	}
 }
 
 static struct ata_port_operations ahci_imx_ops = {
@@ -94,7 +103,6 @@ static const struct ata_port_info ahci_imx_port_info = {
 static int imx6q_sata_init(struct device *dev, struct ahci_host_priv *hpriv,
 			   void __iomem *mmio)
 {
-	int ret = 0;
 	unsigned int reg_val;
 	struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent);
 
@@ -105,12 +113,6 @@ static int imx6q_sata_init(struct device *dev, struct ahci_host_priv *hpriv,
 		return PTR_ERR(imxpriv->gpr);
 	}
 
-	ret = clk_prepare_enable(imxpriv->sata_ref_clk);
-	if (ret < 0) {
-		dev_err(dev, "prepare-enable sata_ref clock err:%d\n", ret);
-		return ret;
-	}
-
 	/*
 	 * set PHY Paremeters, two steps to configure the GPR13,
 	 * one write for rest of parameters, mask of first write
@@ -157,7 +159,11 @@ static int imx6q_sata_init(struct device *dev, struct ahci_host_priv *hpriv,
 		writel(reg_val, mmio + HOST_PORTS_IMPL);
 	}
 
-	reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000;
+	if (!hpriv->clks[CLK_AHB]) {
+		dev_err(dev, "no ahb clk, need sata, sata_ref and ahb clks\n");
+		return -ENOENT;
+	}
+	reg_val = clk_get_rate(hpriv->clks[CLK_AHB]) / 1000;
 	writel(reg_val, mmio + HOST_TIMER1MS);
 
 	return 0;
@@ -165,41 +171,36 @@ static int imx6q_sata_init(struct device *dev, struct ahci_host_priv *hpriv,
 
 static void imx6q_sata_exit(struct device *dev)
 {
-	struct imx_ahci_priv *imxpriv =  dev_get_drvdata(dev->parent);
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent);
 
-	regmap_update_bits(imxpriv->gpr, 0x34, IMX6Q_GPR13_SATA_MPLL_CLK_EN,
+	if (hpriv->clks[CLK_SATA])
+		regmap_update_bits(imxpriv->gpr, 0x34,
+			IMX6Q_GPR13_SATA_MPLL_CLK_EN,
 			!IMX6Q_GPR13_SATA_MPLL_CLK_EN);
-	clk_disable_unprepare(imxpriv->sata_ref_clk);
 }
 
 static void imx_ahci_suspend(struct device *dev)
 {
-	struct imx_ahci_priv *imxpriv =  dev_get_drvdata(dev->parent);
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent);
 
-	/*
-	 * If no_device is set, The CLKs had been gated off in the
-	 * initialization so don't do it again here.
-	 */
-	if (!imxpriv->no_device) {
+	/* Check the CLKs have not been gated off in the initialization. */
+	if (hpriv->clks[CLK_SATA])
 		regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
 				IMX6Q_GPR13_SATA_MPLL_CLK_EN,
 				!IMX6Q_GPR13_SATA_MPLL_CLK_EN);
-		clk_disable_unprepare(imxpriv->sata_ref_clk);
-	}
 }
 
 static int imx_ahci_resume(struct device *dev)
 {
-	struct imx_ahci_priv *imxpriv =  dev_get_drvdata(dev->parent);
-	int ret;
-
-	if (!imxpriv->no_device) {
-		ret = clk_prepare_enable(imxpriv->sata_ref_clk);
-		if (ret < 0) {
-			dev_err(dev, "pre-enable sata_ref clock err:%d\n", ret);
-			return ret;
-		}
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent);
 
+	if (hpriv->clks[CLK_SATA]) {
 		regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
 				IMX6Q_GPR13_SATA_MPLL_CLK_EN,
 				IMX6Q_GPR13_SATA_MPLL_CLK_EN);
@@ -247,22 +248,7 @@ static int imx_ahci_probe(struct platform_device *pdev)
 	ahci_dev = &ahci_pdev->dev;
 	ahci_dev->parent = dev;
 
-	imxpriv->no_device = false;
 	imxpriv->first_time = true;
-	imxpriv->ahb_clk = devm_clk_get(dev, "ahb");
-	if (IS_ERR(imxpriv->ahb_clk)) {
-		dev_err(dev, "can't get ahb clock.\n");
-		ret = PTR_ERR(imxpriv->ahb_clk);
-		goto err_out;
-	}
-
-	imxpriv->sata_ref_clk = devm_clk_get(dev, "sata_ref");
-	if (IS_ERR(imxpriv->sata_ref_clk)) {
-		dev_err(dev, "can't get sata_ref clock.\n");
-		ret = PTR_ERR(imxpriv->sata_ref_clk);
-		goto err_out;
-	}
-
 	imxpriv->ahci_pdev = ahci_pdev;
 	platform_set_drvdata(pdev, imxpriv);
 
-- 
1.8.4.2

Re: [RFC v3 10/13] ahci_imx: Adjust for ahci_platform managing the clocks

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 12:48:52AM +0100, Hans de Goede wrote:
> +enum {
> +	CLK_SATA,
> +	CLK_SATA_REF,
> +	CLK_AHB
> +};

Err, so we now rely on the order that these clocks are specified in DT
rather than their name properties to provide the correct clock... that
sounds particularly fragile to me.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [RFC v3 10/13] ahci_imx: Adjust for ahci_platform managing the clocks

[Hans de Goede]

Hi,

On 01/19/2014 01:41 PM, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 12:48:52AM +0100, Hans de Goede wrote:
>> +enum {
>> +	CLK_SATA,
>> +	CLK_SATA_REF,
>> +	CLK_AHB
>> +};
>
> Err, so we now rely on the order that these clocks are specified in DT
> rather than their name properties to provide the correct clock... that
> sounds particularly fragile to me.

Both in the ohci- / ehci-platform case, where the idea of purely addressing
clocks by index comes from, as well as in the ahci-platform case, people
have been asking me to make things more generic, so as to avoid having
a gazillion almost but not quite the same ehci-foo platform drivers.

This has already happened with ohci-foo.c drivers, and the hope is that
with the new generalized ohci-plaform.c many of the existing ohci-foo
drivers can go away over time.

The downside of this generalized approach is that we cannot use clock-names
since those tend to be implementation specific.

In the specific case of the ahci-imx driver this means that certain clocks
must be at a specific index, since it needs to know which one is the AHB
clock, as documented in the bindings documentation. I don't see how mandating
certain indices is different and/or more fragile then mandating certain names
in the bindings document. I agree that is is slightly less clear to someone
reading the dts, but that is the price we have to pay for this desire for
things to be generic.

Regards,

Hans

Re: [RFC v3 10/13] ahci_imx: Adjust for ahci_platform managing the clocks

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 08:30:37PM +0100, Hans de Goede wrote:
> Hi,
>
> On 01/19/2014 01:41 PM, Russell King - ARM Linux wrote:
>> On Sun, Jan 19, 2014 at 12:48:52AM +0100, Hans de Goede wrote:
>>> +enum {
>>> +	CLK_SATA,
>>> +	CLK_SATA_REF,
>>> +	CLK_AHB
>>> +};
>>
>> Err, so we now rely on the order that these clocks are specified in DT
>> rather than their name properties to provide the correct clock... that
>> sounds particularly fragile to me.
>
> Both in the ohci- / ehci-platform case, where the idea of purely addressing
> clocks by index comes from, as well as in the ahci-platform case, people
> have been asking me to make things more generic, so as to avoid having
> a gazillion almost but not quite the same ehci-foo platform drivers.
>
> This has already happened with ohci-foo.c drivers, and the hope is that
> with the new generalized ohci-plaform.c many of the existing ohci-foo
> drivers can go away over time.
>
> The downside of this generalized approach is that we cannot use clock-names
> since those tend to be implementation specific.
>
> In the specific case of the ahci-imx driver this means that certain clocks
> must be at a specific index, since it needs to know which one is the AHB
> clock, as documented in the bindings documentation. I don't see how mandating
> certain indices is different and/or more fragile then mandating certain names
> in the bindings document. I agree that is is slightly less clear to someone
> reading the dts, but that is the price we have to pay for this desire for
> things to be generic.

So what happens if we have the same IP block appearing, but the SATA_REF
or SATA clock isn't present - how do we still provide an AHB clock (for
argument sake)?

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [RFC v3 10/13] ahci_imx: Adjust for ahci_platform managing the clocks

[Hans de Goede]

Hi,

On 01/19/2014 08:32 PM, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 08:30:37PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 01/19/2014 01:41 PM, Russell King - ARM Linux wrote:
>>> On Sun, Jan 19, 2014 at 12:48:52AM +0100, Hans de Goede wrote:
>>>> +enum {
>>>> +	CLK_SATA,
>>>> +	CLK_SATA_REF,
>>>> +	CLK_AHB
>>>> +};
>>>
>>> Err, so we now rely on the order that these clocks are specified in DT
>>> rather than their name properties to provide the correct clock... that
>>> sounds particularly fragile to me.
>>
>> Both in the ohci- / ehci-platform case, where the idea of purely addressing
>> clocks by index comes from, as well as in the ahci-platform case, people
>> have been asking me to make things more generic, so as to avoid having
>> a gazillion almost but not quite the same ehci-foo platform drivers.
>>
>> This has already happened with ohci-foo.c drivers, and the hope is that
>> with the new generalized ohci-plaform.c many of the existing ohci-foo
>> drivers can go away over time.
>>
>> The downside of this generalized approach is that we cannot use clock-names
>> since those tend to be implementation specific.
>>
>> In the specific case of the ahci-imx driver this means that certain clocks
>> must be at a specific index, since it needs to know which one is the AHB
>> clock, as documented in the bindings documentation. I don't see how mandating
>> certain indices is different and/or more fragile then mandating certain names
>> in the bindings document. I agree that is is slightly less clear to someone
>> reading the dts, but that is the price we have to pay for this desire for
>> things to be generic.
>
> So what happens if we have the same IP block appearing, but the SATA_REF
> or SATA clock isn't present - how do we still provide an AHB clock (for
> argument sake)?

Then it will not use the same compatible string, since then clearly it is not
compatible with "fsl,imx6q-ahci"

And we can document different indices for the new compatible string (and adjust
the code to match).

Regards,

Hans

[RFC v3 11/13] ahci-imx: Don't create a nested platform device from probe

[Hans de Goede]

Instead only provide platform_data through of_device_id, like the ahci-sunxi
driver does.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/ata/Kconfig           |   4 +-
 drivers/ata/Makefile          |   3 +-
 drivers/ata/ahci_imx.c        | 118 ++++++------------------------------------
 drivers/ata/ahci_platform.c   |   3 ++
 include/linux/ahci_platform.h |   1 +
 5 files changed, 22 insertions(+), 107 deletions(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 5f6c11a..05523ad 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -98,11 +98,11 @@ config SATA_AHCI_PLATFORM
 	  If unsure, say N.
 
 config AHCI_IMX
-	tristate "Freescale i.MX AHCI SATA support"
+	bool "Freescale i.MX AHCI SATA support"
 	depends on SATA_AHCI_PLATFORM && MFD_SYSCON
 	help
 	  This option enables support for the Freescale i.MX SoC's
-	  onboard AHCI SATA.
+	  onboard AHCI SATA in the ahci_platform driver.
 
 	  If unsure, say N.
 
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 4d8778c..99f4715 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -10,9 +10,8 @@ obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
 obj-$(CONFIG_SATA_SIL24)	+= sata_sil24.o
 obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
 obj-$(CONFIG_SATA_HIGHBANK)	+= sata_highbank.o libahci.o
-obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o
 
-ahci_plat-objs := ahci_platform.o ahci_sunxi.o
+ahci_plat-objs := ahci_platform.o ahci_imx.o ahci_sunxi.o
 
 # SFF w/ custom DMA
 obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index 8eb24a3..6e0178f 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -28,6 +28,8 @@
 #include <linux/libata.h>
 #include "ahci.h"
 
+#ifdef CONFIG_AHCI_IMX
+
 enum {
 	PORT_PHY_CTL = 0x178,			/* Port0 PHY Control */
 	PORT_PHY_CTL_PDDQ_LOC = 0x100000,	/* PORT_PHY_CTL bits */
@@ -57,7 +59,7 @@ static void ahci_imx_error_handler(struct ata_port *ap)
 	struct ata_host *host = dev_get_drvdata(ap->dev);
 	struct ahci_host_priv *hpriv = host->private_data;
 	void __iomem *mmio = hpriv->mmio;
-	struct imx_ahci_priv *imxpriv = dev_get_drvdata(ap->dev->parent);
+	struct imx_ahci_priv *imxpriv = hpriv->plat_data;
 	int i;
 
 	ahci_error_handler(ap);
@@ -104,8 +106,13 @@ static int imx6q_sata_init(struct device *dev, struct ahci_host_priv *hpriv,
 			   void __iomem *mmio)
 {
 	unsigned int reg_val;
-	struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent);
+	struct imx_ahci_priv *imxpriv;
+
+	imxpriv = devm_kzalloc(dev, sizeof(*imxpriv), GFP_KERNEL);
+	if (!imxpriv)
+		return -ENOMEM;
 
+	hpriv->plat_data = imxpriv;
 	imxpriv->gpr =
 		syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
 	if (IS_ERR(imxpriv->gpr)) {
@@ -173,7 +180,7 @@ static void imx6q_sata_exit(struct device *dev)
 {
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
-	struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent);
+	struct imx_ahci_priv *imxpriv = hpriv->plat_data;
 
 	if (hpriv->clks[CLK_SATA])
 		regmap_update_bits(imxpriv->gpr, 0x34,
@@ -185,7 +192,7 @@ static void imx_ahci_suspend(struct device *dev)
 {
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
-	struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent);
+	struct imx_ahci_priv *imxpriv = hpriv->plat_data;
 
 	/* Check the CLKs have not been gated off in the initialization. */
 	if (hpriv->clks[CLK_SATA])
@@ -198,7 +205,7 @@ static int imx_ahci_resume(struct device *dev)
 {
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
-	struct imx_ahci_priv *imxpriv = dev_get_drvdata(dev->parent);
+	struct imx_ahci_priv *imxpriv = hpriv->plat_data;
 
 	if (hpriv->clks[CLK_SATA]) {
 		regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
@@ -210,7 +217,7 @@ static int imx_ahci_resume(struct device *dev)
 	return 0;
 }
 
-static struct ahci_platform_data imx6q_sata_pdata = {
+struct ahci_platform_data imx6q_sata_pdata = {
 	.init = imx6q_sata_init,
 	.exit = imx6q_sata_exit,
 	.ata_port_info = &ahci_imx_port_info,
@@ -218,102 +225,7 @@ static struct ahci_platform_data imx6q_sata_pdata = {
 	.resume = imx_ahci_resume,
 };
 
-static const struct of_device_id imx_ahci_of_match[] = {
-	{ .compatible = "fsl,imx6q-ahci", .data = &imx6q_sata_pdata},
-	{},
-};
-MODULE_DEVICE_TABLE(of, imx_ahci_of_match);
-
-static int imx_ahci_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct resource *mem, *irq, res[2];
-	const struct of_device_id *of_id;
-	const struct ahci_platform_data *pdata = NULL;
-	struct imx_ahci_priv *imxpriv;
-	struct device *ahci_dev;
-	struct platform_device *ahci_pdev;
-	int ret;
-
-	imxpriv = devm_kzalloc(dev, sizeof(*imxpriv), GFP_KERNEL);
-	if (!imxpriv) {
-		dev_err(dev, "can't alloc ahci_host_priv\n");
-		return -ENOMEM;
-	}
-
-	ahci_pdev = platform_device_alloc("ahci", -1);
-	if (!ahci_pdev)
-		return -ENODEV;
-
-	ahci_dev = &ahci_pdev->dev;
-	ahci_dev->parent = dev;
-
-	imxpriv->first_time = true;
-	imxpriv->ahci_pdev = ahci_pdev;
-	platform_set_drvdata(pdev, imxpriv);
-
-	of_id = of_match_device(imx_ahci_of_match, dev);
-	if (of_id) {
-		pdata = of_id->data;
-	} else {
-		ret = -EINVAL;
-		goto err_out;
-	}
-
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!mem || !irq) {
-		dev_err(dev, "no mmio/irq resource\n");
-		ret = -ENOMEM;
-		goto err_out;
-	}
-
-	res[0] = *mem;
-	res[1] = *irq;
-
-	ahci_dev->coherent_dma_mask = DMA_BIT_MASK(32);
-	ahci_dev->dma_mask = &ahci_dev->coherent_dma_mask;
-	ahci_dev->of_node = dev->of_node;
-
-	ret = platform_device_add_resources(ahci_pdev, res, 2);
-	if (ret)
-		goto err_out;
-
-	ret = platform_device_add_data(ahci_pdev, pdata, sizeof(*pdata));
-	if (ret)
-		goto err_out;
-
-	ret = platform_device_add(ahci_pdev);
-	if (ret) {
-err_out:
-		platform_device_put(ahci_pdev);
-		return ret;
-	}
-
-	return 0;
-}
-
-static int imx_ahci_remove(struct platform_device *pdev)
-{
-	struct imx_ahci_priv *imxpriv = platform_get_drvdata(pdev);
-	struct platform_device *ahci_pdev = imxpriv->ahci_pdev;
-
-	platform_device_unregister(ahci_pdev);
-	return 0;
-}
-
-static struct platform_driver imx_ahci_driver = {
-	.probe = imx_ahci_probe,
-	.remove = imx_ahci_remove,
-	.driver = {
-		.name = "ahci-imx",
-		.owner = THIS_MODULE,
-		.of_match_table = imx_ahci_of_match,
-	},
-};
-module_platform_driver(imx_ahci_driver);
-
-MODULE_DESCRIPTION("Freescale i.MX AHCI SATA platform driver");
 MODULE_AUTHOR("Richard Zhu <Hong-Xing.Zhu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>");
-MODULE_LICENSE("GPL");
 MODULE_ALIAS("ahci:imx");
+
+#endif
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 324d066..9022922 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -96,6 +96,9 @@ static const struct of_device_id ahci_of_match[] = {
 	{ .compatible = "allwinner,sun4i-a10-ahci",
 	  .data = &ahci_sunxi_pdata, },
 #endif
+#ifdef CONFIG_AHCI_IMX
+	{ .compatible = "fsl,imx6q-ahci", .data = &imx6q_sata_pdata, },
+#endif
 	{},
 };
 MODULE_DEVICE_TABLE(of, ahci_of_match);
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 4e6cbe0..19623ad 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -33,5 +33,6 @@ struct ahci_platform_data {
 };
 
 extern struct ahci_platform_data ahci_sunxi_pdata;
+extern struct ahci_platform_data imx6q_sata_pdata;
 
 #endif /* _AHCI_PLATFORM_H */
-- 
1.8.4.2

Re: [RFC v3 11/13] ahci-imx: Don't create a nested platform device from probe

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 12:48:53AM +0100, Hans de Goede wrote:
> Instead only provide platform_data through of_device_id, like the ahci-sunxi
> driver does.

It looks like the whole of ahci_imx.c (apart from the inclues) is
enclosed in a big ifdef.  You can avoid that by using this in the
Makefile:

> @@ -10,9 +10,8 @@ obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
>  obj-$(CONFIG_SATA_SIL24)	+= sata_sil24.o
>  obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
>  obj-$(CONFIG_SATA_HIGHBANK)	+= sata_highbank.o libahci.o
> -obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o
>  
> -ahci_plat-objs := ahci_platform.o ahci_sunxi.o
> +ahci_plat-objs := ahci_platform.o ahci_imx.o ahci_sunxi.o

ahci_plat-y := ahci_platform.o ahci_sunxi.o
ahci_plat-$(CONFIG_AHCI_IMX) += ahci_imx.o

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [RFC v3 11/13] ahci-imx: Don't create a nested platform device from probe

[Hans de Goede]

Hi,

On 01/19/2014 01:25 PM, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 12:48:53AM +0100, Hans de Goede wrote:
>> Instead only provide platform_data through of_device_id, like the ahci-sunxi
>> driver does.
>
> It looks like the whole of ahci_imx.c (apart from the inclues) is
> enclosed in a big ifdef.  You can avoid that by using this in the
> Makefile:
>
>> @@ -10,9 +10,8 @@ obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
>>   obj-$(CONFIG_SATA_SIL24)	+= sata_sil24.o
>>   obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
>>   obj-$(CONFIG_SATA_HIGHBANK)	+= sata_highbank.o libahci.o
>> -obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o
>>
>> -ahci_plat-objs := ahci_platform.o ahci_sunxi.o
>> +ahci_plat-objs := ahci_platform.o ahci_imx.o ahci_sunxi.o
>
> ahci_plat-y := ahci_platform.o ahci_sunxi.o
> ahci_plat-$(CONFIG_AHCI_IMX) += ahci_imx.o
>

Ah, good idea, the same goes for the sunxi bits. I'll fix this in the next
revision of the patch-set.

Regards,

Hans

[RFC v3 12/13] ARM: sun4i: dts: Add ahci / sata support

[Hans de Goede]

From: Oliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>

This patch adds sunxi sata support to A10 boards that have such a connector.
Some boards also feature a regulator via a GPIO and support for this is also
added.

Signed-off-by: Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/sun4i-a10-a1000.dts      |  4 ++++
 arch/arm/boot/dts/sun4i-a10-cubieboard.dts |  6 +++++
 arch/arm/boot/dts/sun4i-a10.dtsi           |  8 +++++++
 arch/arm/boot/dts/sunxi-ahci-reg.dtsi      | 38 ++++++++++++++++++++++++++++++
 4 files changed, 56 insertions(+)
 create mode 100644 arch/arm/boot/dts/sunxi-ahci-reg.dtsi

diff --git a/arch/arm/boot/dts/sun4i-a10-a1000.dts b/arch/arm/boot/dts/sun4i-a10-a1000.dts
index aef8207..fd6d512 100644
--- a/arch/arm/boot/dts/sun4i-a10-a1000.dts
+++ b/arch/arm/boot/dts/sun4i-a10-a1000.dts
@@ -48,6 +48,10 @@
 			status = "okay";
 		};
 
+		sata: ahci@01c18000 {
+			status = "okay";
+		};
+
 		pinctrl@01c20800 {
 			mmc0_cd_pin_a1000: mmc0_cd_pin@0 {
 				allwinner,pins = "PH1";
diff --git a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
index f50fb2b..d23aaa8 100644
--- a/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
+++ b/arch/arm/boot/dts/sun4i-a10-cubieboard.dts
@@ -12,6 +12,7 @@
 
 /dts-v1/;
 /include/ "sun4i-a10.dtsi"
+/include/ "sunxi-ahci-reg.dtsi"
 
 / {
 	model = "Cubietech Cubieboard";
@@ -51,6 +52,11 @@
 			status = "okay";
 		};
 
+		sata: ahci@01c18000 {
+			target-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl@01c20800 {
 			mmc0_cd_pin_cubieboard: mmc0_cd_pin@0 {
 				allwinner,pins = "PH1";
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index 4736dd2..09517c3 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -331,6 +331,14 @@
 			status = "disabled";
 		};
 
+		sata: ahci@01c18000 {
+			compatible = "allwinner,sun4i-a10-ahci";
+			reg = <0x01c18000 0x1000>;
+			interrupts = <56>;
+			clocks = <&pll6 0>, <&ahb_gates 25>;
+			status = "disabled";
+		};
+
 		intc: interrupt-controller@01c20400 {
 			compatible = "allwinner,sun4i-ic";
 			reg = <0x01c20400 0x400>;
diff --git a/arch/arm/boot/dts/sunxi-ahci-reg.dtsi b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi
new file mode 100644
index 0000000..955b197
--- /dev/null
+++ b/arch/arm/boot/dts/sunxi-ahci-reg.dtsi
@@ -0,0 +1,38 @@
+/*
+ * sunxi boards sata target power supply common code
+ *
+ * Copyright 2014 - Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/ {
+	soc@01c00000 {
+		ahci_pwr_pin_a: ahci_pwr_pin@0 {
+			allwinner,pins = "PB8";
+			allwinner,function = "gpio_out";
+			allwinner,drive = <0>;
+			allwinner,pull = <0>;
+		};
+	};
+
+	regulators {
+		compatible = "simple-bus";
+		pinctrl-names = "default";
+
+		reg_ahci_5v: ahci-5v {
+			compatible = "regulator-fixed";
+			regulator-name = "ahci-5v";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			pinctrl-0 = <&ahci_pwr_pin_a>;
+			gpio = <&pio 1 8 0>;
+			enable-active-high;
+		};
+	};
+};
-- 
1.8.4.2

[RFC v3 13/13] ARM: sun7i: dts: Add ahci / sata support

[Hans de Goede]

This patch adds sunxi sata support to A20 boards that have such a connector.
Some boards also feature a regulator via a GPIO and support for this is also
added.

Signed-off-by: Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts     |  6 ++++++
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts      | 20 ++++++++++++++++++++
 arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts |  6 ++++++
 arch/arm/boot/dts/sun7i-a20.dtsi                |  8 ++++++++
 4 files changed, 40 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
index 48777cd..785fac0 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
@@ -13,6 +13,7 @@
 
 /dts-v1/;
 /include/ "sun7i-a20.dtsi"
+/include/ "sunxi-ahci-reg.dtsi"
 
 / {
 	model = "Cubietech Cubieboard2";
@@ -28,6 +29,11 @@
 			status = "okay";
 		};
 
+		sata: ahci@01c18000 {
+			target-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl@01c20800 {
 			mmc0_cd_pin_cubieboard2: mmc0_cd_pin@0 {
 				allwinner,pins = "PH1";
diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
index 2684f27..68e0809 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
@@ -13,6 +13,7 @@
 
 /dts-v1/;
 /include/ "sun7i-a20.dtsi"
+/include/ "sunxi-ahci-reg.dtsi"
 
 / {
 	model = "Cubietech Cubietruck";
@@ -28,6 +29,11 @@
 			status = "okay";
 		};
 
+		sata: ahci@01c18000 {
+			target-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl@01c20800 {
 			mmc0_cd_pin_cubietruck: mmc0_cd_pin@0 {
 				allwinner,pins = "PH1";
@@ -36,6 +42,13 @@
 				allwinner,pull = <0>;
 			};
 
+			ahci_pwr_pin_cubietruck: ahci_pwr_pin@1 {
+				allwinner,pins = "PH12";
+				allwinner,function = "gpio_out";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
 			led_pins_cubietruck: led_pins@0 {
 				allwinner,pins = "PH7", "PH11", "PH20", "PH21";
 				allwinner,function = "gpio_out";
@@ -84,4 +97,11 @@
 			gpios = <&pio 7 7 0>;
 		};
 	};
+
+	regulators {
+		reg_ahci_5v: ahci-5v {
+			pinctrl-0 = <&ahci_pwr_pin_cubietruck>;
+			gpio = <&pio 7 12 0>;
+		};
+	};
 };
diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
index bf6f6c8..8a289c4 100644
--- a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
+++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
@@ -13,6 +13,7 @@
 
 /dts-v1/;
 /include/ "sun7i-a20.dtsi"
+/include/ "sunxi-ahci-reg.dtsi"
 
 / {
 	model = "Olimex A20-Olinuxino Micro";
@@ -37,6 +38,11 @@
 			status = "okay";
 		};
 
+		sata: ahci@01c18000 {
+			target-supply = <&reg_ahci_5v>;
+			status = "okay";
+		};
+
 		pinctrl@01c20800 {
 			mmc0_cd_pin_olinuxinom: mmc0_cd_pin@0 {
 				allwinner,pins = "PH1";
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index c9c123a..fc1be33 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -347,6 +347,14 @@
 			status = "disabled";
 		};
 
+		sata: ahci@01c18000 {
+			compatible = "allwinner,sun4i-a10-ahci";
+			reg = <0x01c18000 0x1000>;
+			interrupts = <0 56 1>;
+			clocks = <&pll6 0>, <&ahb_gates 25>;
+			status = "disabled";
+		};
+
 		pio: pinctrl@01c20800 {
 			compatible = "allwinner,sun7i-a20-pinctrl";
 			reg = <0x01c20800 0x400>;
-- 
1.8.4.2

Re: [RFC v3 00/13] ahci: add sunxi driver and cleanup imx driver

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 12:48:42AM +0100, Hans de Goede wrote:
> Hi All,
> 
> Here is v3 of my patchset for adding ahci-sunxi support. It has grown quite
> a bit since I've been going for a more generic approach this time and I've
> also cleaned up the ahci-imx driver to use the same generic approach.

If you want to avoid getting caught in dwmw2's moderation queue, then
please use "[PATCH RFC" and not just "[RFC" for threaded patches.  The
important bit is the "[PATCH" bit.  Thanks.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".