[RESEND PATCHv2 1/3] arm: socfpga: Set the SDMMC clock phase in system manager

[RESEND PATCHv2 1/3] arm: socfpga: Set the SDMMC clock phase in system manager

[dinguyen at altera.com]

From: Dinh Nguyen <dinguyen@altera.com>

Add functionality in the System Manager to set the SDR settings for the
SD/MMC IP.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
Cc: Pavel Machek <pavel@denx.de>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Olof Johansson <olof@lixom.net>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Chris Ball <cjb@laptop.org>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Seungwon Jeon <tgih.jun@samsung.com>
Cc: devicetree at vger.kernel.org
Cc: linux-mmc at vger.kernel.org
CC: linux-arm-kernel at lists.infradead.org

---
v2:
- Have socfpga_sysmgr_set_dwmmc_drvsel_smpsel() take in the SDR settings
directly so that it does not have to walk the DTB.
---
 arch/arm/mach-socfpga/Makefile     |    2 +-
 arch/arm/mach-socfpga/core.h       |    6 ++++++
 arch/arm/mach-socfpga/system_mgr.c |   28 ++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-socfpga/system_mgr.c

diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile
index 6dd7a93..e4ff8b9 100644
--- a/arch/arm/mach-socfpga/Makefile
+++ b/arch/arm/mach-socfpga/Makefile
@@ -2,5 +2,5 @@
 # Makefile for the linux kernel.
 #
 
-obj-y					:= socfpga.o
+obj-y					:= socfpga.o system_mgr.o
 obj-$(CONFIG_SMP)	+= headsmp.o platsmp.o
diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h
index 572b8f7..b05fa6a 100644
--- a/arch/arm/mach-socfpga/core.h
+++ b/arch/arm/mach-socfpga/core.h
@@ -44,4 +44,10 @@ extern unsigned long cpu1start_addr;
 
 #define SOCFPGA_SCU_VIRT_BASE   0xfffec000
 
+/* SDMMC Group for System Manager defines */
+#define SYSMGR_SDMMCGRP_CTRL_OFFSET             0x108
+#define DRV_CLK_PHASE_SHIFT_SEL_MASK    0x7
+#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)          \
+	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
+
 #endif
diff --git a/arch/arm/mach-socfpga/system_mgr.c b/arch/arm/mach-socfpga/system_mgr.c
new file mode 100644
index 0000000..9fdce1d
--- /dev/null
+++ b/arch/arm/mach-socfpga/system_mgr.c
@@ -0,0 +1,28 @@
+/*
+ * Copyright Altera Corporation (C) 2013. All Rights Reserved.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/of_platform.h>
+#include <asm/mach/map.h>
+
+#include "core.h"
+
+void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel)
+{
+	u32 hs_timing;
+
+	hs_timing = SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel);
+	writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET);
+}
+EXPORT_SYMBOL(socfpga_sysmgr_set_dwmmc_drvsel_smpsel);
-- 
1.7.9.5

[RESEND PATCHv2 2/3] mmc: dw_mmc-socfpga: Clean up SOCFPGA platform specific functionality

[dinguyen at altera.com]

From: Dinh Nguyen <dinguyen@altera.com>

The SDR timing registers for the SD/MMC IP block for SOCFPGA is located
in the system manager. This system manager IP block is located outside of
the SD IP block itself. Therefore, the function to set the SDR timing
register should be in the platform specific code so that the SD driver can
be autonomous of any future System Manager changes.

Also, there is no need for "altr,dw-mshc-ciu-div" as the driver can get
the value of the CIU clock from the common clock API.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
Cc: Pavel Machek <pavel@denx.de>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Olof Johansson <olof@lixom.net>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Chris Ball <cjb@laptop.org>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Seungwon Jeon <tgih.jun@samsung.com>
Cc: devicetree at vger.kernel.org
Cc: linux-mmc at vger.kernel.org
CC: linux-arm-kernel at lists.infradead.org
---
v2:
- Let the mmc driver walk the DTB to get the SDR values
---
 drivers/mmc/host/dw_mmc-socfpga.c |   38 +++++++++----------------------------
 1 file changed, 9 insertions(+), 29 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-socfpga.c b/drivers/mmc/host/dw_mmc-socfpga.c
index 14b5961..3be2e2a 100644
--- a/drivers/mmc/host/dw_mmc-socfpga.c
+++ b/drivers/mmc/host/dw_mmc-socfpga.c
@@ -24,16 +24,12 @@
 #include "dw_mmc.h"
 #include "dw_mmc-pltfm.h"
 
-#define SYSMGR_SDMMCGRP_CTRL_OFFSET		0x108
-#define DRV_CLK_PHASE_SHIFT_SEL_MASK	0x7
-#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)          \
-	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
+extern void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel);
 
 /* SOCFPGA implementation specific driver private data */
 struct dw_mci_socfpga_priv_data {
-	u8	ciu_div; /* card interface unit divisor */
-	u32	hs_timing; /* bitmask for CIU clock phase shift */
-	struct regmap   *sysreg; /* regmap for system manager register */
+	u32     drvsel; /*Phase shift for the Drive clock, or tx mode.*/
+	u32	smplsel; /*Phase shift for the Sample clock, or rx mode.*/
 };
 
 static int dw_mci_socfpga_priv_init(struct dw_mci *host)
@@ -45,14 +41,7 @@ static int dw_mci_socfpga_priv_init(struct dw_mci *host)
 		dev_err(host->dev, "mem alloc failed for private data\n");
 		return -ENOMEM;
 	}
-
-	priv->sysreg = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
-	if (IS_ERR(priv->sysreg)) {
-		dev_err(host->dev, "regmap for altr,sys-mgr lookup failed.\n");
-		return PTR_ERR(priv->sysreg);
-	}
 	host->priv = priv;
-
 	return 0;
 }
 
@@ -61,20 +50,15 @@ static int dw_mci_socfpga_setup_clock(struct dw_mci *host)
 	struct dw_mci_socfpga_priv_data *priv = host->priv;
 
 	clk_disable_unprepare(host->ciu_clk);
-	regmap_write(priv->sysreg, SYSMGR_SDMMCGRP_CTRL_OFFSET,
-		priv->hs_timing);
+	socfpga_sysmgr_set_dwmmc_drvsel_smpsel(priv->drvsel, priv->smplsel);
 	clk_prepare_enable(host->ciu_clk);
 
-	host->bus_hz /= (priv->ciu_div + 1);
 	return 0;
 }
 
 static void dw_mci_socfpga_prepare_command(struct dw_mci *host, u32 *cmdr)
 {
-	struct dw_mci_socfpga_priv_data *priv = host->priv;
-
-	if (priv->hs_timing & DRV_CLK_PHASE_SHIFT_SEL_MASK)
-		*cmdr |= SDMMC_CMD_USE_HOLD_REG;
+	*cmdr |= SDMMC_CMD_USE_HOLD_REG;
 }
 
 static int dw_mci_socfpga_parse_dt(struct dw_mci *host)
@@ -82,20 +66,16 @@ static int dw_mci_socfpga_parse_dt(struct dw_mci *host)
 	struct dw_mci_socfpga_priv_data *priv = host->priv;
 	struct device_node *np = host->dev->of_node;
 	u32 timing[2];
-	u32 div = 0;
 	int ret;
 
-	ret = of_property_read_u32(np, "altr,dw-mshc-ciu-div", &div);
-	if (ret)
-		dev_info(host->dev, "No dw-mshc-ciu-div specified, assuming 1");
-	priv->ciu_div = div;
-
 	ret = of_property_read_u32_array(np,
-			"altr,dw-mshc-sdr-timing", timing, 2);
+		"samsung,dw-mshc-sdr-timing", timing, 2);
 	if (ret)
 		return ret;
 
-	priv->hs_timing = SYSMGR_SDMMC_CTRL_SET(timing[0], timing[1]);
+	priv->drvsel = timing[0];
+	priv->smplsel = timing[1];
+
 	return 0;
 }
 
-- 
1.7.9.5

[RESEND PATCHv2 3/3] arm: dts: socfpga: Add support for SD/MMC

[dinguyen at altera.com]

From: Dinh Nguyen <dinguyen@altera.com>

Add bindings for SD/MMC for SOCFPGA.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
Reviewed-by: Pavel Machek <pavel@denx.de>
Cc: Pavel Machek <pavel@denx.de>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Olof Johansson <olof@lixom.net>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Chris Ball <cjb@laptop.org>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Seungwon Jeon <tgih.jun@samsung.com>
Cc: devicetree at vger.kernel.org
Cc: linux-mmc at vger.kernel.org
CC: linux-arm-kernel at lists.infradead.org
---
v2:
- Fix documentation s/binding/property and s/Synopsis/Synopsys
---
 .../devicetree/bindings/mmc/socfpga-dw-mshc.txt    |   38 ++++++++++++++++++++
 arch/arm/boot/dts/socfpga.dtsi                     |   11 ++++++
 arch/arm/boot/dts/socfpga_cyclone5.dts             |   12 +++++++
 arch/arm/boot/dts/socfpga_vt.dts                   |   12 +++++++
 4 files changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt

diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
new file mode 100644
index 0000000..531336a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
@@ -0,0 +1,38 @@
+* Altera SOCFPGA specific extensions to the Synopsys Designware Mobile
+  Storage Host Controller
+
+The Synopsys designware mobile storage host controller is used to interface
+a SoC with storage medium such as eMMC or SD/MMC cards. This file documents
+differences between the core Synopsys dw mshc controller properties described
+by synopsys-dw-mshc.txt and the properties used by the SOCFPGA specific
+extensions to the Synopsys Designware Mobile Storage Host Controller.
+
+Required Properties:
+
+* compatible: should be
+        - "altr,socfpga-dw-mshc": for controllers with Altera SOCFPGA
+          specific extensions.
+
+* samsung,dw-mshc-sdr-timing: See exynos-dw-mshc.txt for more information about
+	this property.
+
+Example:
+        dwmmc0 at ff704000 {
+                compatible = "altr,socfpga-dw-mshc", "snps,dw-mshc";
+                reg = <0xff704000 0x1000>;
+                interrupts = <0 139 4>;
+                fifo-depth = <0x400>;
+                #address-cells = <1>;
+                #size-cells = <0>;
+                clocks = <&l4_mp_clk>, <&sdmmc_clk>;
+                clock-names = "biu", "ciu";
+                num-slots = <1>;
+                supports-highspeed;
+                broken-cd;
+                samsung,dw-mshc-sdr-timing = <3 0>;
+
+                slot at 0 {
+                        reg = <0>;
+                        bus-width = <4>;
+                };
+        };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index e273fa9..1ee6079 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -468,6 +468,17 @@
 			cache-level = <2>;
 		};
 
+		mmc: dwmmc0 at ff704000 {
+			compatible = "altr,socfpga-dw-mshc";
+			reg = <0xff704000 0x1000>;
+			interrupts = <0 139 4>;
+			fifo-depth = <0x400>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&l4_mp_clk>, <&sdmmc_clk>;
+			clock-names = "biu", "ciu";
+		};
+
 		/* Local timer */
 		timer at fffec600 {
 			compatible = "arm,cortex-a9-twd-timer";
diff --git a/arch/arm/boot/dts/socfpga_cyclone5.dts b/arch/arm/boot/dts/socfpga_cyclone5.dts
index 973999d..bfed066 100644
--- a/arch/arm/boot/dts/socfpga_cyclone5.dts
+++ b/arch/arm/boot/dts/socfpga_cyclone5.dts
@@ -48,6 +48,18 @@
 			};
 		};
 
+		dwmmc0 at ff704000 {
+			num-slots = <1>;
+			supports-highspeed;
+			broken-cd;
+			samsung,dw-mshc-sdr-timing = <3 0>;
+
+			slot at 0 {
+				reg = <0>;
+				bus-width = <4>;
+			};
+		};
+
 		ethernet at ff702000 {
 			phy-mode = "rgmii";
 			phy-addr = <0xffffffff>; /* probe for phy addr */
diff --git a/arch/arm/boot/dts/socfpga_vt.dts b/arch/arm/boot/dts/socfpga_vt.dts
index d1ec0ca..25b2653 100644
--- a/arch/arm/boot/dts/socfpga_vt.dts
+++ b/arch/arm/boot/dts/socfpga_vt.dts
@@ -41,6 +41,18 @@
 			};
 		};
 
+		dwmmc0 at ff704000 {
+			num-slots = <1>;
+			supports-highspeed;
+			broken-cd;
+			samsung,dw-mshc-sdr-timing = <3 0>;
+
+			slot at 0 {
+				reg = <0>;
+				bus-width = <4>;
+			};
+		};
+
 		ethernet at ff700000 {
 			phy-mode = "gmii";
 			status = "okay";
-- 
1.7.9.5

[RESEND PATCHv2 1/3] arm: socfpga: Set the SDMMC clock phase in system manager

[Jaehoon Chung]

Hi Dinh,

On 10/15/2013 04:47 AM, dinguyen at altera.com wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> Add functionality in the System Manager to set the SDR settings for the
> SD/MMC IP.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
> Cc: Pavel Machek <pavel@denx.de>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Olof Johansson <olof@lixom.net>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Chris Ball <cjb@laptop.org>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Seungwon Jeon <tgih.jun@samsung.com>
> Cc: devicetree at vger.kernel.org
> Cc: linux-mmc at vger.kernel.org
> CC: linux-arm-kernel at lists.infradead.org
> 
> ---
> v2:
> - Have socfpga_sysmgr_set_dwmmc_drvsel_smpsel() take in the SDR settings
> directly so that it does not have to walk the DTB.
> ---
>  arch/arm/mach-socfpga/Makefile     |    2 +-
>  arch/arm/mach-socfpga/core.h       |    6 ++++++
>  arch/arm/mach-socfpga/system_mgr.c |   28 ++++++++++++++++++++++++++++
>  3 files changed, 35 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/mach-socfpga/system_mgr.c
> 
> diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile
> index 6dd7a93..e4ff8b9 100644
> --- a/arch/arm/mach-socfpga/Makefile
> +++ b/arch/arm/mach-socfpga/Makefile
> @@ -2,5 +2,5 @@
>  # Makefile for the linux kernel.
>  #
>  
> -obj-y					:= socfpga.o
> +obj-y					:= socfpga.o system_mgr.o
>  obj-$(CONFIG_SMP)	+= headsmp.o platsmp.o
> diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h
> index 572b8f7..b05fa6a 100644
> --- a/arch/arm/mach-socfpga/core.h
> +++ b/arch/arm/mach-socfpga/core.h
> @@ -44,4 +44,10 @@ extern unsigned long cpu1start_addr;
>  
>  #define SOCFPGA_SCU_VIRT_BASE   0xfffec000
>  
> +/* SDMMC Group for System Manager defines */
> +#define SYSMGR_SDMMCGRP_CTRL_OFFSET             0x108
> +#define DRV_CLK_PHASE_SHIFT_SEL_MASK    0x7
I didn't see where this define is used. When do this use?

Best Regards,
Jaehoon Chung

> +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)          \
> +	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
> +
>  #endif
> diff --git a/arch/arm/mach-socfpga/system_mgr.c b/arch/arm/mach-socfpga/system_mgr.c
> new file mode 100644
> index 0000000..9fdce1d
> --- /dev/null
> +++ b/arch/arm/mach-socfpga/system_mgr.c
> @@ -0,0 +1,28 @@
> +/*
> + * Copyright Altera Corporation (C) 2013. All Rights Reserved.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/of_platform.h>
> +#include <asm/mach/map.h>
> +
> +#include "core.h"
> +
> +void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel)
> +{
> +	u32 hs_timing;
> +
> +	hs_timing = SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel);
> +	writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET);
> +}
> +EXPORT_SYMBOL(socfpga_sysmgr_set_dwmmc_drvsel_smpsel);
> 

[RESEND PATCHv2 1/3] arm: socfpga: Set the SDMMC clock phase in system manager

[Dinh Nguyen]

Hi Jaehoo,

On 10/15/13 1:51 AM, Jaehoon Chung wrote:
> Hi Dinh,
>
> On 10/15/2013 04:47 AM, dinguyen at altera.com wrote:
>> From: Dinh Nguyen <dinguyen@altera.com>
>>
>> Add functionality in the System Manager to set the SDR settings for the
>> SD/MMC IP.
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
>> Cc: Pavel Machek <pavel@denx.de>
>> CC: Arnd Bergmann <arnd@arndb.de>
>> CC: Olof Johansson <olof@lixom.net>
>> Cc: Rob Herring <rob.herring@calxeda.com>
>> Cc: Pawel Moll <pawel.moll@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Stephen Warren <swarren@wwwdotorg.org>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Chris Ball <cjb@laptop.org>
>> Cc: Jaehoon Chung <jh80.chung@samsung.com>
>> Cc: Seungwon Jeon <tgih.jun@samsung.com>
>> Cc: devicetree at vger.kernel.org
>> Cc: linux-mmc at vger.kernel.org
>> CC: linux-arm-kernel at lists.infradead.org
>>
>> ---
>> v2:
>> - Have socfpga_sysmgr_set_dwmmc_drvsel_smpsel() take in the SDR settings
>> directly so that it does not have to walk the DTB.
>> ---
>>   arch/arm/mach-socfpga/Makefile     |    2 +-
>>   arch/arm/mach-socfpga/core.h       |    6 ++++++
>>   arch/arm/mach-socfpga/system_mgr.c |   28 ++++++++++++++++++++++++++++
>>   3 files changed, 35 insertions(+), 1 deletion(-)
>>   create mode 100644 arch/arm/mach-socfpga/system_mgr.c
>>
>> diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile
>> index 6dd7a93..e4ff8b9 100644
>> --- a/arch/arm/mach-socfpga/Makefile
>> +++ b/arch/arm/mach-socfpga/Makefile
>> @@ -2,5 +2,5 @@
>>   # Makefile for the linux kernel.
>>   #
>>   
>> -obj-y					:= socfpga.o
>> +obj-y					:= socfpga.o system_mgr.o
>>   obj-$(CONFIG_SMP)	+= headsmp.o platsmp.o
>> diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h
>> index 572b8f7..b05fa6a 100644
>> --- a/arch/arm/mach-socfpga/core.h
>> +++ b/arch/arm/mach-socfpga/core.h
>> @@ -44,4 +44,10 @@ extern unsigned long cpu1start_addr;
>>   
>>   #define SOCFPGA_SCU_VIRT_BASE   0xfffec000
>>   
>> +/* SDMMC Group for System Manager defines */
>> +#define SYSMGR_SDMMCGRP_CTRL_OFFSET             0x108
>> +#define DRV_CLK_PHASE_SHIFT_SEL_MASK    0x7
> I didn't see where this define is used. When do this use?
It's not used at all. I will remove.

Thanks for the review.
Dinh
>
> Best Regards,
> Jaehoon Chung
>
>> +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)          \
>> +	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
>> +
>>   #endif
>> diff --git a/arch/arm/mach-socfpga/system_mgr.c b/arch/arm/mach-socfpga/system_mgr.c
>> new file mode 100644
>> index 0000000..9fdce1d
>> --- /dev/null
>> +++ b/arch/arm/mach-socfpga/system_mgr.c
>> @@ -0,0 +1,28 @@
>> +/*
>> + * Copyright Altera Corporation (C) 2013. All Rights Reserved.
>> + *
>> + * 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.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +#include <linux/of_platform.h>
>> +#include <asm/mach/map.h>
>> +
>> +#include "core.h"
>> +
>> +void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel)
>> +{
>> +	u32 hs_timing;
>> +
>> +	hs_timing = SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel);
>> +	writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET);
>> +}
>> +EXPORT_SYMBOL(socfpga_sysmgr_set_dwmmc_drvsel_smpsel);
>>

[RESEND PATCHv2 1/3] arm: socfpga: Set the SDMMC clock phase in system manager

[Arnd Bergmann]

On Monday 14 October 2013, dinguyen at altera.com wrote:
> +void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel)
> +{
> +       u32 hs_timing;
> +
> +       hs_timing = SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel);
> +       writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET);
> +}
> +EXPORT_SYMBOL(socfpga_sysmgr_set_dwmmc_drvsel_smpsel);

This looks like the wrong approach. What are you trying to do? If you want to
set a clock, please use the clk API.

	Arnd

[RESEND PATCHv2 1/3] arm: socfpga: Set the SDMMC clock phase in system manager

[Dinh Nguyen]

Hi Arnd,

On 10/15/13 7:50 AM, Arnd Bergmann wrote:
> On Monday 14 October 2013, dinguyen at altera.com wrote:
>> +void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel)
>> +{
>> +       u32 hs_timing;
>> +
>> +       hs_timing = SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel);
>> +       writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET);
>> +}
>> +EXPORT_SYMBOL(socfpga_sysmgr_set_dwmmc_drvsel_smpsel);
> This looks like the wrong approach. What are you trying to do? If you want to
> set a clock, please use the clk API.
I can't use the clk API because this function is setting up a clock 
phase bit for the SD/MMC
clock that is used to clock the card, not the IP. This register is 
located outside the SD/MMC
and the clock manager.

Just to refresh your memory on this topic:
I tried to use the syscon approach that you suggested:

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/168470.html
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/170423.html

But this approach was rejected by Stephen Warren because we wanted to 
the SD driver to be automonous
of registers outside its IP:

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/194014.html

So I went with the approach of exposing a platform API so that the 
SD/MMC platform specific
code can call it.

The system manager has a plethora of registers that controls other IPs 
on the SOC, so I kinda thought
syscon was the way to go with this. A driver for this IP did not make 
sense to me.

Please advise if you know of another approach?

Thanks,
Dinh






>
> 	Arnd

[RESEND PATCHv2 1/3] arm: socfpga: Set the SDMMC clock phase in system manager

[Arnd Bergmann]

On Tuesday 15 October 2013, Dinh Nguyen wrote:
> Hi Arnd,
> 
> On 10/15/13 7:50 AM, Arnd Bergmann wrote:
> > On Monday 14 October 2013, dinguyen at altera.com wrote:
> >> +void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel)
> >> +{
> >> +       u32 hs_timing;
> >> +
> >> +       hs_timing = SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel);
> >> +       writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET);
> >> +}
> >> +EXPORT_SYMBOL(socfpga_sysmgr_set_dwmmc_drvsel_smpsel);
> > This looks like the wrong approach. What are you trying to do? If you want to
> > set a clock, please use the clk API.
> I can't use the clk API because this function is setting up a clock 
> phase bit for the SD/MMC
> clock that is used to clock the card, not the IP. This register is 
> located outside the SD/MMC
> and the clock manager.
> 
> Just to refresh your memory on this topic:
> I tried to use the syscon approach that you suggested:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/168470.html
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/170423.html

Ah, thanks. I knew this problem had come up before, I just didn't remember
it was for socfpga.

> But this approach was rejected by Stephen Warren because we wanted to 
> the SD driver to be automonous
> of registers outside its IP:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/194014.html
> 
> So I went with the approach of exposing a platform API so that the 
> SD/MMC platform specific
> code can call it.
> 
> The system manager has a plethora of registers that controls other IPs 
> on the SOC, so I kinda thought
> syscon was the way to go with this. A driver for this IP did not make 
> sense to me.
> 
> Please advise if you know of another approach?

I don't remember the details of what we have gone through before, but
I think this should still work:

1 Create a "syscon" backend driver to control your "system manager", which
  lets other drivers hook into it without calling a private API.
2 Create a trivial clock driver that is independent of your existing
  clock driver and independent of the other drivers using the system
  manager, by using syscon as the low-level interface.
3 Make the sdmmc driver use the normal clock API and link its clock to the
  driver from step 2 in the device tree.

Is this what you have tried before?

	Arnd

[RESEND PATCHv2 1/3] arm: socfpga: Set the SDMMC clock phase in system manager

[Dinh Nguyen]

Hi Arnd,

On 10/15/13 2:01 PM, Arnd Bergmann wrote:
> On Tuesday 15 October 2013, Dinh Nguyen wrote:
>> Hi Arnd,
>>
>> On 10/15/13 7:50 AM, Arnd Bergmann wrote:
>>> On Monday 14 October 2013, dinguyen at altera.com wrote:
>>>> +void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel)
>>>> +{
>>>> +       u32 hs_timing;
>>>> +
>>>> +       hs_timing = SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel);
>>>> +       writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET);
>>>> +}
>>>> +EXPORT_SYMBOL(socfpga_sysmgr_set_dwmmc_drvsel_smpsel);
>>> This looks like the wrong approach. What are you trying to do? If you want to
>>> set a clock, please use the clk API.
>> I can't use the clk API because this function is setting up a clock
>> phase bit for the SD/MMC
>> clock that is used to clock the card, not the IP. This register is
>> located outside the SD/MMC
>> and the clock manager.
>>
>> Just to refresh your memory on this topic:
>> I tried to use the syscon approach that you suggested:
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/168470.html
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/170423.html
> Ah, thanks. I knew this problem had come up before, I just didn't remember
> it was for socfpga.
>
>> But this approach was rejected by Stephen Warren because we wanted to
>> the SD driver to be automonous
>> of registers outside its IP:
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/194014.html
>>
>> So I went with the approach of exposing a platform API so that the
>> SD/MMC platform specific
>> code can call it.
>>
>> The system manager has a plethora of registers that controls other IPs
>> on the SOC, so I kinda thought
>> syscon was the way to go with this. A driver for this IP did not make
>> sense to me.
>>
>> Please advise if you know of another approach?
> I don't remember the details of what we have gone through before, but
> I think this should still work:
>
> 1 Create a "syscon" backend driver to control your "system manager", which
>    lets other drivers hook into it without calling a private API.
Yes, if you look at drivers/mmc/host/dw_mmc-socfpga.c that is in the 
mainline,
it is hooking into the "system manager" through "syscon". Is this what you
mean here?

The problem is because of the SYSMGR_SDMMCGRP_CTRL_OFFSET define
in this file. This means the SD/MMC driver needs information that is 
outside of its IP.

Dinh
> 2 Create a trivial clock driver that is independent of your existing
>    clock driver and independent of the other drivers using the system
>    manager, by using syscon as the low-level interface.
> 3 Make the sdmmc driver use the normal clock API and link its clock to the
>    driver from step 2 in the device tree.
>
> Is this what you have tried before?
>
> 	Arnd

[RESEND PATCHv2 1/3] arm: socfpga: Set the SDMMC clock phase in system manager

[Arnd Bergmann]

On Tuesday 15 October 2013, Dinh Nguyen wrote:
> >
> > 1 Create a "syscon" backend driver to control your "system manager", which
> >    lets other drivers hook into it without calling a private API.
> Yes, if you look at drivers/mmc/host/dw_mmc-socfpga.c that is in the 
> mainline,
> it is hooking into the "system manager" through "syscon". Is this what you
> mean here?

No, because you directly hook into the syscon driver, rather than using
a clock driver as the middle-man, see steps 2 and 3 below.
 
> The problem is because of the SYSMGR_SDMMCGRP_CTRL_OFFSET define
> in this file. This means the SD/MMC driver needs information that is 
> outside of its IP.

Yes, this is not ideal because you don't really want that information
in the sd/mmc driver. That driver should only know about the fact
that it talks to a clock controller, not how it's implemented.

	Arnd

> > 2 Create a trivial clock driver that is independent of your existing
> >    clock driver and independent of the other drivers using the system
> >    manager, by using syscon as the low-level interface.
> > 3 Make the sdmmc driver use the normal clock API and link its clock to the
> >    driver from step 2 in the device tree.
> >
> > Is this what you have tried before?

[RESEND PATCHv2 1/3] arm: socfpga: Set the SDMMC clock phase in system manager

[Dinh Nguyen]

On 10/15/13 2:47 PM, Arnd Bergmann wrote:
> On Tuesday 15 October 2013, Dinh Nguyen wrote:
>>> 1 Create a "syscon" backend driver to control your "system manager", which
>>>     lets other drivers hook into it without calling a private API.
>> Yes, if you look at drivers/mmc/host/dw_mmc-socfpga.c that is in the
>> mainline,
>> it is hooking into the "system manager" through "syscon". Is this what you
>> mean here?
> No, because you directly hook into the syscon driver, rather than using
> a clock driver as the middle-man, see steps 2 and 3 below.
>   
Ok, now I understand.
>> The problem is because of the SYSMGR_SDMMCGRP_CTRL_OFFSET define
>> in this file. This means the SD/MMC driver needs information that is
>> outside of its IP.
> Yes, this is not ideal because you don't really want that information
> in the sd/mmc driver. That driver should only know about the fact
> that it talks to a clock controller, not how it's implemented.
>
> 	Arnd
>
>>> 2 Create a trivial clock driver that is independent of your existing
>>>     clock driver and independent of the other drivers using the system
>>>     manager, by using syscon as the low-level interface.
>>> 3 Make the sdmmc driver use the normal clock API and link its clock to the
>>>     driver from step 2 in the device tree.
This makes sense for the SD/MMC driver, but do you think I can use the 
same approach for
other drivers that this system manager touches? i.e. The ethernet IP's 
PHY setting is controlled
by a register that is in the system manager as well.

I have submitted this patch for enabling ethernet. It is making use of 
the driver platform specific
driver calls to touch the system manager.

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-September/200173.html

Just wondering if that is right approach for the ethernet driver? If 
not, then I think I may have
to come up with a generic system manager driver so that it can be used 
for other IPs.

Thanks,
Dinh

>>>
>>> Is this what you have tried before?

[RESEND PATCHv2 1/3] arm: socfpga: Set the SDMMC clock phase in system manager

[Arnd Bergmann]

On Tuesday 15 October 2013, Dinh Nguyen wrote:
> This makes sense for the SD/MMC driver, but do you think I can use the 
> same approach for
> other drivers that this system manager touches?

It might not be as straightforward for every driver, but something like
this should work at least for some of the more interesting ones. For the
rest, you can have a system manager driver that exports functions.

> i.e. The ethernet IP's 
> PHY setting is controlled
> by a register that is in the system manager as well.
>
> I have submitted this patch for enabling ethernet. It is making use of 
> the driver platform specific
> driver calls to touch the system manager.
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-September/200173.html

I definitely don't want to see auxdata getting used for stuff like this.

In case of the ethernet driver, I suspect an SoC specific PHY driver would
be the right approach.
 
> Just wondering if that is right approach for the ethernet driver? If 
> not, then I think I may have
> to come up with a generic system manager driver so that it can be used 
> for other IPs.

The general way of handling this should be to see if there is a subsystem
available for handling a particular feature. In case of clk and phy, we
have those subsystems, so it's a matter of writing an appropriate driver
that communicates with the high-level drivers using that.

I don't know what other features are provided by the system manager that
might need a different solution, but if you can't come up with a nice
solution, you can either have an soc specific portion in the device driver
(as you suggested for sdmmc first), or you can have that platform specific
mfd driver exporting some functions.
If the functionality is actually something common but doesn't have a
subsystem in Linux yet, the "right" approach might also be to introduce
a new subsystem, which I realize can be a lot of work.

	Arnd