Hi Sascha:
Thanks for your comments.

Best Regards
Richard Zhu

-----Original Message-----
From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
Sent: Tuesday, March 15, 2011 5:22 PM
To: Zhu Richard-R65037
Cc: linux-arm-kernel at lists.infradead.org; jgarzik at pobox.com;
kernel at pengutronix.de; linux-ide at vger.kernel.org;
avorontsov at ru.mvista.com; eric at eukrea.com; eric.miao at linaro.org
Subject: Re: [RFC PATCH 1/2] AHCI Add the AHCI SATA feature on MX53
platforms

On Mon, Mar 14, 2011 at 05:55:43PM +0800, Richard Zhu wrote:

quoted

Signed-off-by: Richard Zhu <redacted>
---
 arch/arm/mach-mx5/clock-mx51-mx53.c             |   11 +++++
 arch/arm/mach-mx5/devices-imx53.h               |    4 ++
 arch/arm/plat-mxc/devices/Kconfig               |    4 ++
 arch/arm/plat-mxc/devices/Makefile              |    1 +
 arch/arm/plat-mxc/devices/platform-ahci-imx.c   |   55

+++++++++++++++++++++++

quoted

 arch/arm/plat-mxc/include/mach/ahci_sata.h      |   51

+++++++++++++++++++++

quoted

 arch/arm/plat-mxc/include/mach/devices-common.h |   10 ++++
 7 files changed, 136 insertions(+), 0 deletions(-)  create mode
100644 arch/arm/plat-mxc/devices/platform-ahci-imx.c
 create mode 100644 arch/arm/plat-mxc/include/mach/ahci_sata.h

diff --git a/arch/arm/mach-mx5/clock-mx51-mx53.c

b/arch/arm/mach-mx5/clock-mx51-mx53.c
index 652ace4..2b3d27c 100644

--- a/arch/arm/mach-mx5/clock-mx51-mx53.c
+++ b/arch/arm/mach-mx5/clock-mx51-mx53.c

@@ -1380,6 +1380,14 @@ static struct clk esdhc4_mx53_clk = {
    .secondary = &esdhc4_ipg_clk,
 };

+static struct clk sata_clk = {
+   .parent = &ipg_clk,
+   .enable = _clk_max_enable,
+   .enable_reg = MXC_CCM_CCGR4,
+   .enable_shift = MXC_CCM_CCGRx_CG1_OFFSET,
+   .disable = _clk_max_disable,
+};
+
 DEFINE_CLOCK(mipi_esc_clk, 0, MXC_CCM_CCGR4,

MXC_CCM_CCGRx_CG5_OFFSET, NULL, NULL, NULL, &pll2_sw_clk);
DEFINE_CLOCK(mipi_hsc2_clk, 0, MXC_CCM_CCGR4,
MXC_CCM_CCGRx_CG4_OFFSET, NULL, NULL, &mipi_esc_clk, &pll2_sw_clk);

DEFINE_CLOCK(mipi_hsc1_clk, 0, MXC_CCM_CCGR4, MXC_CCM_CCGRx_CG3_OFFSET,
NULL, NULL, &mipi_hsc2_clk, &pll2_sw_clk); @@ -1468,6 +1476,9 @@ static
struct clk_lookup mx53_lookups[] = {

quoted

    _REGISTER_CLOCK("imx53-cspi.0", NULL, cspi_clk)
    _REGISTER_CLOCK("imx2-wdt.0", NULL, dummy_clk)
    _REGISTER_CLOCK("imx2-wdt.1", NULL, dummy_clk)
+   _REGISTER_CLOCK(NULL, "imx_sata_clk", sata_clk)

help!

The NULL pointer here really should be "ahci" to associate this clock
with the device it is used with. Then think about the "imx_sata_clk"
string.
The information "imx" is not relevant. "sata" is redundant as it is
already associated with the ahci device. We are talking about a field in
clocks, so no need for "clk". This should read:

      _REGISTER_CLOCK("ahci", NULL, sata_clk)

Accepted.

quoted

+   _REGISTER_CLOCK(NULL, "usb_phy1", usb_phy1_clk)
+   _REGISTER_CLOCK(NULL, "ahb_clk", ahb_clk)

no _clk please.

Accepted.

quoted

 };

 static void clk_tree_init(void)

diff --git a/arch/arm/mach-mx5/devices-imx53.h

b/arch/arm/mach-mx5/devices-imx53.h
index 9251008..09ebb43 100644

--- a/arch/arm/mach-mx5/devices-imx53.h
+++ b/arch/arm/mach-mx5/devices-imx53.h

@@ -33,3 +33,7 @@ extern const struct imx_spi_imx_data

imx53_ecspi_data[] __initconst;  extern const struct imx_imx2_wdt_data

imx53_imx2_wdt_data[] __initconst;

quoted

 #define imx53_add_imx2_wdt(id, pdata)      \
    imx_add_imx2_wdt(&imx53_imx2_wdt_data[id])
+
+extern const struct imx_ahci_imx_data imx53_ahci_imx_data[]

__initconst;

quoted

+#define imx53_add_ahci_imx(id, pdata)   \
+   imx_add_ahci_imx(&imx53_ahci_imx_data[id], pdata)

diff --git a/arch/arm/plat-mxc/devices/Kconfig

b/arch/arm/plat-mxc/devices/Kconfig
index b9ab1d5..087595a 100644

--- a/arch/arm/plat-mxc/devices/Kconfig
+++ b/arch/arm/plat-mxc/devices/Kconfig

@@ -71,3 +71,7 @@ config IMX_HAVE_PLATFORM_SDHCI_ESDHC_IMX

 config IMX_HAVE_PLATFORM_SPI_IMX
    bool
+
+config IMX_HAVE_PLATFORM_SATA_AHCI
+   bool
+   default y if SOC_IMX53

diff --git a/arch/arm/plat-mxc/devices/Makefile

b/arch/arm/plat-mxc/devices/Makefile
index 75cd2ec..e0b7fa3 100644

--- a/arch/arm/plat-mxc/devices/Makefile
+++ b/arch/arm/plat-mxc/devices/Makefile

@@ -22,3 +22,4 @@ obj-$(CONFIG_IMX_HAVE_PLATFORM_MXC_RNGA) +=

platform-mxc_rnga.o
 obj-$(CONFIG_IMX_HAVE_PLATFORM_MXC_W1) += platform-mxc_w1.o
 obj-$(CONFIG_IMX_HAVE_PLATFORM_SDHCI_ESDHC_IMX) +=
platform-sdhci-esdhc-imx.o
 obj-$(CONFIG_IMX_HAVE_PLATFORM_SPI_IMX) +=  platform-spi_imx.o
+obj-$(CONFIG_IMX_HAVE_PLATFORM_SATA_AHCI) +=  platform-ahci-imx.o
+
+#ifdef CONFIG_SOC_IMX53
+const struct imx_ahci_imx_data imx53_ahci_imx_data __initconst =
+   imx_ahci_data_entry_single(MX53);
+#endif
+
+struct platform_device *__init imx_add_ahci_imx(
+           const struct imx_ahci_imx_data *data,
+           const struct ahci_platform_data *pdata) {
+   struct resource res[] = {
+           {
+                   .start = data->iobase,
+                   .end = data->iobase + SZ_4K - 1,
+                   .flags = IORESOURCE_MEM,
+           }, {
+                   .start = data->irq,
+                   .end = data->irq,
+                   .flags = IORESOURCE_IRQ,
+           },
+   };
+
+   return imx_add_platform_device_dmamask("ahci", 0 /* -1? */,

I think it doesn't matter much if you use o or -1 here, but please remove
the comment. btw if you keep 0 here you must use "ahci.0" in registering
the clock.

Accepted.

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