[PATCH v4 0/3] ahci: enable ahci sata support on imx6q

[PATCH v4 0/3] ahci: enable ahci sata support on imx6q

[Richard Zhu]

V4: add imx6q specific ahci sata support
  Thanks for the review comments provided by Sascha, and Alexander.
  - Use the private data and keep a pointer to the PHY clock.
  - Don't use the global platform device variable, because that
  it would make the driver broken for mutiple instances.
  - Don't do the "writel" with assignment.
  - Other minor changes, such as print the error code when printing
  error message, use a u32 type to store readl results, and so on.

v3: http://www.spinics.net/lists/linux-ide/msg45688.html
  - Keep arch/arm and ahci_platform driver clean.
  - Add the sata_imx standalone driver contained all the
  specific setup
  - Add the release function, support the loadable module
  driver.
  - Tested on imx6q sd board.

v2: http://www.spinics.net/lists/linux-ide/msg45666.html 
 - Setup standalone imx ahci sata driver, because of
 the misalignments of the bits definition of the HBA register.
 - Replace the node by the label in the board dts.
 - 

v1: http://www.spinics.net/lists/linux-ide/msg45581.html
 - add imx6q specific ahci sata support to arch/arm/mach-imx/mach-imx6q.c

These patches is based on imx/dt branch of
"http://git.linaro.org/git-ro/people/shawnguo/linux-2.6.git"

[v4 1/3] ARM: dtsi: enable ahci sata on imx6q platforms
[v4 2/3] ARM: imx6q: update the sata bits definitions of gpr13
[v4 3/3] sata: imx: add ahci sata support on imx platforms

arch/arm/boot/dts/imx6q-sabreauto.dts       |    4 +
arch/arm/boot/dts/imx6q-sabrelite.dts       |    4 +
arch/arm/boot/dts/imx6q-sabresd.dts         |    4 +
arch/arm/boot/dts/imx6q.dtsi                |    9 +
drivers/ata/Kconfig                         |    9 +
drivers/ata/Makefile                        |    1 +
drivers/ata/sata_imx.c                      |  237 +++++++++++++++++++++++++++
include/linux/mfd/syscon/imx6q-iomuxc-gpr.h |  121 ++++++++++----
8 files changed, 352 insertions(+), 37 deletions(-)

[v4 3/3] sata: imx: add ahci sata support on imx platforms

[Sascha Hauer]

On Wed, Jul 10, 2013 at 04:35:55PM +0800, Richard Zhu wrote:
> From: Richard Zhu <r65037@freescale.com>
> 
> imx6q contains one Synopsys AHCI SATA controller,
> But it can't shares ahci_platform driver with other
> controllers.
> Because there are some misalignments of the generic
> AHCI controller.
> The bits definitions of the HBA registers, the Vendor
> Specific registers, and the AHCI PHY clock.
>  - CAP_SSS(bit20) of the HOST_CAP is writable, default
>  value is '0', should be configured to be '1'
>  - bit0 (only one AHCI SATA port on imx6q) of the
>  HOST_PORTS_IMPL should be set to be '1'.(default 0)
>  - One Vendor Specific register HOST_TIMER1MS(offset:0xe0)
>  should be configured regarding to the frequency of AHB
>  bus clock.
>  - Configurations of the AHCI PHY clock, and the signal
>  parameters of the GPR13
> 
> Setup its own ahci sata driver, contained the imx6q specific
> initialized codes, and re-use the generic ahci_platform drier,
> 
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  drivers/ata/Kconfig    |    9 ++
>  drivers/ata/Makefile   |    1 +
>  drivers/ata/sata_imx.c |  237 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 247 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/ata/sata_imx.c
> 

[...]

> +
> +/* imx6q ahci module initialization. */
> +static int imx6q_sata_init(struct device *dev, void __iomem *mmio)
> +{
> +	int ret = 0;
> +	unsigned int rc;
> +	struct regmap *gpr;
> +	struct clk *ahb_clk;
> +	struct imx_ahci_priv *imxpriv = (struct imx_ahci_priv *)dev->p;

Just because a pointer points to valid memory doesn't mean that it's
free to use for your purposes. If you look closer maybe you realize that
the imxpriv pointer you use here is another one than the one you
allocate below in your probe function.

I give up here. Maybe I'll write a driver for this when I find time for
it.

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 |

[v4 3/3] sata: imx: add ahci sata support on imx platforms

[Zhu Richard-R65037]

Hi Sascha:
Sorry to make a mess on the usage of the private data.
The private data of "imx_ahci_pdev" platform device would be used to store ata_host point.
I shouldn't set imxpriv to the private data of "imx_ahci_pdev", so do the wrong assignment of
The private pointer of device.

Best Regards
Richard Zhu


-----Original Message-----
From: Sascha Hauer [mailto:s.hauer at pengutronix.de] 
Sent: Thursday, July 11, 2013 5:04 AM
To: Richard Zhu
Cc: shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org; jgarzik at pobox.com; tj at kernel.org; rob.herring at calxeda.com; linux-ide at vger.kernel.org; Zhu Richard-R65037
Subject: Re: [v4 3/3] sata: imx: add ahci sata support on imx platforms

On Wed, Jul 10, 2013 at 04:35:55PM +0800, Richard Zhu wrote:
> From: Richard Zhu <r65037@freescale.com>
> 
> imx6q contains one Synopsys AHCI SATA controller, But it can't shares 
> ahci_platform driver with other controllers.
> Because there are some misalignments of the generic AHCI controller.
> The bits definitions of the HBA registers, the Vendor Specific 
> registers, and the AHCI PHY clock.
>  - CAP_SSS(bit20) of the HOST_CAP is writable, default  value is '0', 
> should be configured to be '1'
>  - bit0 (only one AHCI SATA port on imx6q) of the  HOST_PORTS_IMPL 
> should be set to be '1'.(default 0)
>  - One Vendor Specific register HOST_TIMER1MS(offset:0xe0)  should be 
> configured regarding to the frequency of AHB  bus clock.
>  - Configurations of the AHCI PHY clock, and the signal  parameters of 
> the GPR13
> 
> Setup its own ahci sata driver, contained the imx6q specific 
> initialized codes, and re-use the generic ahci_platform drier,
> 
> Signed-off-by: Richard Zhu <r65037@freescale.com>
> ---
>  drivers/ata/Kconfig    |    9 ++
>  drivers/ata/Makefile   |    1 +
>  drivers/ata/sata_imx.c |  237 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 247 insertions(+), 0 deletions(-)  create mode 
> 100644 drivers/ata/sata_imx.c
> 

[...]

> +
> +/* imx6q ahci module initialization. */ static int 
> +imx6q_sata_init(struct device *dev, void __iomem *mmio) {
> +	int ret = 0;
> +	unsigned int rc;
> +	struct regmap *gpr;
> +	struct clk *ahb_clk;
> +	struct imx_ahci_priv *imxpriv = (struct imx_ahci_priv *)dev->p;

Just because a pointer points to valid memory doesn't mean that it's free to use for your purposes. If you look closer maybe you realize that the imxpriv pointer you use here is another one than the one you allocate below in your probe function.

I give up here. Maybe I'll write a driver for this when I find time for it.

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 |

[v4 3/3] sata: imx: add ahci sata support on imx platforms

[Sascha Hauer]

Hi Richard,

On Thu, Jul 11, 2013 at 07:50:50AM +0000, Zhu Richard-R65037 wrote:
> Hi Sascha:
> Sorry to make a mess on the usage of the private data.
> The private data of "imx_ahci_pdev" platform device would be used to store ata_host point.
> I shouldn't set imxpriv to the private data of "imx_ahci_pdev", so do the wrong assignment of
> The private pointer of device.

Please allocate your private data along with the platform_device in the
probe function, then in the init callback you can use container_of() to
get a pointer to the data.
Also I would recommend you to request the necessary resources (clocks,
regmap) in the probe function and not in the init callback.

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 |

[v4 3/3] sata: imx: add ahci sata support on imx platforms

[Zhu Richard-R65037]

Hi Sascha:
Thanks for your comments.
Yes, it is.
That's is what I done in the V5 patch-set. The patch-set is almost done, would send out a moment later.

Best Regards
Richard Zhu


-----Original Message-----
From: Sascha Hauer [mailto:s.hauer at pengutronix.de] 
Sent: Thursday, July 11, 2013 3:56 PM
To: Zhu Richard-R65037
Cc: Richard Zhu; shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org; jgarzik at pobox.com; tj at kernel.org; rob.herring at calxeda.com; linux-ide at vger.kernel.org
Subject: Re: [v4 3/3] sata: imx: add ahci sata support on imx platforms

Hi Richard,

On Thu, Jul 11, 2013 at 07:50:50AM +0000, Zhu Richard-R65037 wrote:
> Hi Sascha:
> Sorry to make a mess on the usage of the private data.
> The private data of "imx_ahci_pdev" platform device would be used to store ata_host point.
> I shouldn't set imxpriv to the private data of "imx_ahci_pdev", so do 
> the wrong assignment of The private pointer of device.

Please allocate your private data along with the platform_device in the probe function, then in the init callback you can use container_of() to get a pointer to the data.
Also I would recommend you to request the necessary resources (clocks,
regmap) in the probe function and not in the init callback.

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 |