[PATCHv3 0/4] socfpga: Enable SD/MMC support

[PATCHv3 0/4] socfpga: Enable SD/MMC support

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

Hi,

This is v3 of the patch series to enable SD/MMC on the SOCFPGA platform. The
SD/MMC IP is the Synopsys DW SD/MMC driver. However, the settings for the
clock-phase of the CIU resides in a register that is located outside of SD/MMC
IP, in a register block called the system manager.

Per Arnd's suggestion, I implemented a clock driver for the system manager
block. The SD/MMC driver can now use the common clock API to set the correct
settings for the SD/MMC clock phase.

Patch 1/4: clk: socfpga: Add a clock driver for SOCFPGA's system manager
This patch adds a clk-sysmgr driver that can be use by a common clock API
to set system manager register bits needed by the SD/MMC driver. The SD/MMC
driver can simply call a common clock API to set the required clock phase
settings for the SD/MMC CIU.

Patch 2/4: arm: dts: Add a system manager compatible property
This patch adds a DTS compatible entry for the new clk-sysmgr driver.

Patch 3/4: mmc: dw_mmc-socfpga: Clean up SOCFPGA platform specific functionality
This patch cleans up dw_mmc-socpfga.c file from defines that are outside of
the SD/MMC IP. It makes the common clock API call to set the SD/MMC clock
phase settings in the system manager.

Patch 4/4: arm: dts: Add support for SD/MMC on SOCFPGA
This patch adds the necessary DTS bindings for the SOCFPGA specific extensions
to the base Synopsys DW SD/MMC driver.

Thanks,

Dinh Nguyen (4):
  clk: socfpga: Add a clock driver for SOCFPGA's system manager
  arm: dts: Add a system manager compatible property
  mmc: dw_mmc-socfpga: Clean up SOCFPGA platform specific
    funcationality
  arm: dts: Add support for SD/MMC on SOCFPGA

 .../bindings/arm/altera/socfpga-system.txt         |   10 +++
 .../devicetree/bindings/mmc/socfpga-dw-mshc.txt    |   38 ++++++++
 arch/arm/boot/dts/socfpga.dtsi                     |   25 +++++-
 arch/arm/boot/dts/socfpga_cyclone5.dtsi            |   12 +++
 arch/arm/boot/dts/socfpga_vt.dts                   |   12 +++
 drivers/clk/socfpga/Makefile                       |    2 +-
 drivers/clk/socfpga/clk-sysmgr.c                   |   91 ++++++++++++++++++++
 drivers/mmc/host/dw_mmc-socfpga.c                  |   78 +++--------------
 8 files changed, 196 insertions(+), 72 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
 create mode 100644 drivers/clk/socfpga/clk-sysmgr.c

-- 
1.7.9.5

[PATCHv3 1/4] clk: socfpga: Add a clock driver for SOCFPGA's system manager

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

The system manager is an IP block on the SOCFPGA platform. The system manager
contains registers that control other IPs on the platform. One of the IPs that
the system manager has control over is the SD/MMC, by way of controlling the
clock phase on the SD/MMC Card Interface Unit.

This patch adds a clock driver that the SD/MMC driver can use by calling
the common clock API in order to set the appropriate register in the
system manager by way of a syscon driver.

This clock driver can also be re-used for other IPs that need to poke the system
manager.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
---
v3: Not use the syscon driver because as of 3.13-rc1, the syscon driver
is loaded after the clock driver.

v2: Use the syscon driver
---
 drivers/clk/socfpga/Makefile     |    2 +-
 drivers/clk/socfpga/clk-sysmgr.c |   88 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/socfpga/clk-sysmgr.c

diff --git a/drivers/clk/socfpga/Makefile b/drivers/clk/socfpga/Makefile
index 0303c0b..cfceabc 100644
--- a/drivers/clk/socfpga/Makefile
+++ b/drivers/clk/socfpga/Makefile
@@ -1 +1 @@
-obj-y += clk.o
+obj-y += clk.o clk-sysmgr.o
diff --git a/drivers/clk/socfpga/clk-sysmgr.c b/drivers/clk/socfpga/clk-sysmgr.c
new file mode 100644
index 0000000..7538b22
--- /dev/null
+++ b/drivers/clk/socfpga/clk-sysmgr.c
@@ -0,0 +1,88 @@
+/*
+ * Copyright 2013 Altera Corporation <www.altera.com>
+ *
+ * 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/slab.h>
+#include <linux/clk-provider.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+
+/* SDMMC Group for System Manager defines */
+#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)          \
+	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
+
+extern void __iomem *sys_manager_base_addr;
+
+struct socfpga_sysmgr {
+	struct clk_hw	hw;
+	u32     reg;
+};
+#define to_sysmgr_clk(p) container_of(p, struct socfpga_sysmgr, hw)
+
+static int sysmgr_set_dwmmc_drvsel_smpsel(struct clk_hw *hwclk)
+{
+	struct device_node *np;
+	struct socfpga_sysmgr *socfpga_sysmgr = to_sysmgr_clk(hwclk);
+	u32 timing[2];
+	u32 hs_timing;
+
+	np = of_find_compatible_node(NULL, NULL, "altr,socfpga-dw-mshc");
+	of_property_read_u32_array(np, "samsung,dw-mshc-sdr-timing", timing, 2);
+	hs_timing = SYSMGR_SDMMC_CTRL_SET(timing[1], timing[0]);
+	writel(hs_timing, sys_manager_base_addr + socfpga_sysmgr->reg);
+	return 0;
+}
+
+static const struct clk_ops clk_sysmgr_sdmmc_ops = {
+	.enable = sysmgr_set_dwmmc_drvsel_smpsel,
+};
+
+static void __init socfpga_sysmgr_init(struct device_node *node, const struct clk_ops *ops)
+{
+	u32 reg;
+	struct clk *clk;
+	struct socfpga_sysmgr *socfpga_sysmgr;
+	const char *clk_name = node->name;
+	struct clk_init_data init;
+	int rc;
+
+	socfpga_sysmgr = kzalloc(sizeof(*socfpga_sysmgr), GFP_KERNEL);
+	if (WARN_ON(!socfpga_sysmgr))
+		return;
+
+	rc = of_property_read_u32(node, "reg", &reg);
+	if (WARN_ON(rc))
+		return;
+
+	socfpga_sysmgr->reg = reg;
+
+	init.name = clk_name;
+	init.ops = ops;
+	init.flags = 0;
+	init.num_parents = 0;
+
+	socfpga_sysmgr->hw.init = &init;
+	clk = clk_register(NULL, &socfpga_sysmgr->hw);
+	if (WARN_ON(IS_ERR(clk))) {
+		kfree(socfpga_sysmgr);
+		return;
+	}
+	rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	if (WARN_ON(rc))
+		return;
+}
+
+static void __init sysmgr_init(struct device_node *node)
+{
+	socfpga_sysmgr_init(node, &clk_sysmgr_sdmmc_ops);
+}
+CLK_OF_DECLARE(sysmgr, "altr,sysmgr-sdmmc-sdr", sysmgr_init);
-- 
1.7.9.5

[PATCHv3 2/4] arm: dts: Add a system manager compatible property

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

The "altr,sysmgr-sdmmc-sdr" compatible property is used for the SOCFPGA
clk-sysmgr driver. This property represents the register inside the
system manager that controls the clock phase of the SD/MMC driver.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
---
v3: Cannot use the syscon driver along with the clock because as of v3.13-rc1,
the syscon driver is loaded after the clocks.
v2: Add syscon
---
 .../bindings/arm/altera/socfpga-system.txt         |   10 ++++++++++
 arch/arm/boot/dts/socfpga.dtsi                     |   14 +++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-system.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-system.txt
index f4d04a0..7a6c7ed 100644
--- a/Documentation/devicetree/bindings/arm/altera/socfpga-system.txt
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-system.txt
@@ -5,9 +5,19 @@ Required properties:
 - reg : Should contain 1 register ranges(address and length)
 - cpu1-start-addr : CPU1 start address in hex.
 
+Optional properties:
+- compatible = "altr,sysmgr-sdmmc-sdr". This compatible property is used
+to represent the clock phase settings for the SD/MMC IP.
+
 Example:
 	 sysmgr@ffd08000 {
 		compatible = "altr,sys-mgr";
 		reg = <0xffd08000 0x1000>;
 		cpu1-start-addr = <0xffd080c4>;
+
+		sysmgr_sdr_mmc: sysmgr_sdr_mmc {
+			#clock-cells = <0>;
+			compatible = "altr,sysmgr-sdmmc-sdr";
+			reg = <0x108 1>;
+		};
 	};
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index f936476..a6a13b3 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -522,9 +522,17 @@
 			reg = <0xffd05000 0x1000>;
 		};
 
-		sysmgr@ffd08000 {
-				compatible = "altr,sys-mgr";
-				reg = <0xffd08000 0x4000>;
+		sysmgr: sysmgr@ffd08000 {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			compatible = "altr,sys-mgr";
+			reg = <0xffd08000 0x4000>;
+
+			sysmgr_sdr_mmc: sysmgr_sdr_mmc {
+				#clock-cells = <0>;
+				compatible = "altr,sysmgr-sdmmc-sdr";
+				reg = <0x108 1>;
 			};
+		};
 	};
 };
-- 
1.7.9.5

[PATCHv3 3/4] mmc: dw_mmc-socfpga: Clean up SOCFPGA platform specific funcationality

[dinguyen]

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. We can use the normal clock API to set the SDR
settings.

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>
---
v3: none
v2: none
---
 drivers/mmc/host/dw_mmc-socfpga.c |   78 +++++--------------------------------
 1 file changed, 10 insertions(+), 68 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-socfpga.c b/drivers/mmc/host/dw_mmc-socfpga.c
index 3e8e53a..d790cc0 100644
--- a/drivers/mmc/host/dw_mmc-socfpga.c
+++ b/drivers/mmc/host/dw_mmc-socfpga.c
@@ -13,95 +13,37 @@
  * Taken from dw_mmc-exynos.c
  */
 #include <linux/clk.h>
-#include <linux/mfd/syscon.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/dw_mmc.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <linux/regmap.h>
 
 #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))
-
-/* 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 */
-};
-
-static int dw_mci_socfpga_priv_init(struct dw_mci *host)
-{
-	return 0;
-}
-
 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);
-	clk_prepare_enable(host->ciu_clk);
-
-	host->bus_hz /= (priv->ciu_div + 1);
+	struct clk *sysmgr_clk = devm_clk_get(host->dev, "sysmgr-sdr-mmc");
+
+	if (IS_ERR(sysmgr_clk))
+		dev_err(host->dev, "sysmgr-sdr-mmc not available\n");
+	else {
+		clk_disable_unprepare(host->ciu_clk);
+		clk_prepare_enable(sysmgr_clk);
+		clk_prepare_enable(host->ciu_clk);
+	}
 	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;
-}
-
-static int dw_mci_socfpga_parse_dt(struct dw_mci *host)
-{
-	struct dw_mci_socfpga_priv_data *priv;
-	struct device_node *np = host->dev->of_node;
-	u32 timing[2];
-	u32 div = 0;
-	int ret;
-
-	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv) {
-		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);
-	}
-
-	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);
-	if (ret)
-		return ret;
-
-	priv->hs_timing = SYSMGR_SDMMC_CTRL_SET(timing[0], timing[1]);
-	host->priv = priv;
-	return 0;
+	*cmdr |= SDMMC_CMD_USE_HOLD_REG;
 }
 
 static const struct dw_mci_drv_data socfpga_drv_data = {
-	.init			= dw_mci_socfpga_priv_init,
 	.setup_clock		= dw_mci_socfpga_setup_clock,
 	.prepare_command	= dw_mci_socfpga_prepare_command,
-	.parse_dt		= dw_mci_socfpga_parse_dt,
 };
 
 static const struct of_device_id dw_mci_socfpga_match[] = {
-- 
1.7.9.5

[PATCHv3 4/4] arm: dts: Add support for SD/MMC on SOCFPGA

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

Add new bindings that support SD/MMC on Altera's SOCFPGA platform.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
---
v3: none
v2: none
---
 .../devicetree/bindings/mmc/socfpga-dw-mshc.txt    |   38 ++++++++++++++++++++
 arch/arm/boot/dts/socfpga.dtsi                     |   11 ++++++
 arch/arm/boot/dts/socfpga_cyclone5.dtsi            |   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..c408e74
--- /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@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>, <&sysmgr_sdr_mmc>;
+		clock-names = "biu", "ciu", "sysmgr-sdr-mmc";
+		num-slots = <1>;
+		supports-highspeed;
+		broken-cd;
+		samsung,dw-mshc-sdr-timing = <3 0>;
+
+		slot@0 {
+			reg = <0>;
+			bus-width = <4>;
+		};
+	};
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index a6a13b3..5023e25 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -469,6 +469,17 @@
 			cache-level = <2>;
 		};
 
+		mmc: dwmmc0@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>, <&sysmgr_sdr_mmc>;
+			clock-names = "biu", "ciu", "sysmgr-sdr-mmc";
+		};
+
 		/* Local timer */
 		timer@fffec600 {
 			compatible = "arm,cortex-a9-twd-timer";
diff --git a/arch/arm/boot/dts/socfpga_cyclone5.dtsi b/arch/arm/boot/dts/socfpga_cyclone5.dtsi
index a8716f6..4ef4fa4 100644
--- a/arch/arm/boot/dts/socfpga_cyclone5.dtsi
+++ b/arch/arm/boot/dts/socfpga_cyclone5.dtsi
@@ -28,6 +28,18 @@
 			};
 		};
 
+		dwmmc0@ff704000 {
+			num-slots = <1>;
+			supports-highspeed;
+			broken-cd;
+			samsung,dw-mshc-sdr-timing = <3 0>;
+
+			slot@0 {
+				reg = <0>;
+				bus-width = <4>;
+			};
+		};
+
 		ethernet@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@ff704000 {
+			num-slots = <1>;
+			supports-highspeed;
+			broken-cd;
+			samsung,dw-mshc-sdr-timing = <3 0>;
+
+			slot@0 {
+				reg = <0>;
+				bus-width = <4>;
+			};
+		};
+
 		ethernet@ff700000 {
 			phy-mode = "gmii";
 			status = "okay";
-- 
1.7.9.5

Re: [PATCHv3 1/4] clk: socfpga: Add a clock driver for SOCFPGA's system manager

[Arnd Bergmann]

On Wednesday 04 December 2013, dinguyen@altera.com wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> The system manager is an IP block on the SOCFPGA platform. The system manager
> contains registers that control other IPs on the platform. One of the IPs that
> the system manager has control over is the SD/MMC, by way of controlling the
> clock phase on the SD/MMC Card Interface Unit.
> 
> This patch adds a clock driver that the SD/MMC driver can use by calling
> the common clock API in order to set the appropriate register in the
> system manager by way of a syscon driver.
> 
> This clock driver can also be re-used for other IPs that need to poke the system
> manager.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>

I think this still gets things wrong on multiple levels.

> ---
> v3: Not use the syscon driver because as of 3.13-rc1, the syscon driver
> is loaded after the clock driver.
> 
> v2: Use the syscon driver

Can't you reference the syscon driver just from the set_rate function?
When you do that, you won't need to care if it's available until the clock
is actually used by the sd card driver.

> +
> +#include <linux/slab.h>
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +
> +/* SDMMC Group for System Manager defines */
> +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)          \
> +	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
> +
> +extern void __iomem *sys_manager_base_addr;

You definitely cannot put "extern" variable declarations into driver files.
This is always a bug.

> +static int sysmgr_set_dwmmc_drvsel_smpsel(struct clk_hw *hwclk)
> +{
> +	struct device_node *np;
> +	struct socfpga_sysmgr *socfpga_sysmgr = to_sysmgr_clk(hwclk);
> +	u32 timing[2];
> +	u32 hs_timing;
> +
> +	np = of_find_compatible_node(NULL, NULL, "altr,socfpga-dw-mshc");
> +	of_property_read_u32_array(np, "samsung,dw-mshc-sdr-timing", timing, 2);
> +	hs_timing = SYSMGR_SDMMC_CTRL_SET(timing[1], timing[0]);
> +	writel(hs_timing, sys_manager_base_addr + socfpga_sysmgr->reg);
> +	return 0;
> +}

The clock driver has absolutely no business looking into the
"samsung,dw-mshc-sdr-timing" property, that is a layering violation.
The SD card driver should pass the frequency to the clock driver
instead.

> +static void __init socfpga_sysmgr_init(struct device_node *node, const struct clk_ops *ops)
> +{
> +	u32 reg;
> +	struct clk *clk;
> +	struct socfpga_sysmgr *socfpga_sysmgr;
> +	const char *clk_name = node->name;
> +	struct clk_init_data init;
> +	int rc;
> +
> +	socfpga_sysmgr = kzalloc(sizeof(*socfpga_sysmgr), GFP_KERNEL);
> +	if (WARN_ON(!socfpga_sysmgr))
> +		return;
> +
> +	rc = of_property_read_u32(node, "reg", &reg);
> +	if (WARN_ON(rc))
> +		return;

This feels wrong: drivers should not manually interpret the standard "reg"
property. I'll let Mike comment on this, but I think what you want instead
is to use an index into the sysmgr in the clock reference. This assumes
that sysmgr contains a set of identical clock register blocks that can
be indexed in a simple way.

> +	socfpga_sysmgr->reg = reg;
> +
> +	init.name = clk_name;
> +	init.ops = ops;
> +	init.flags = 0;
> +	init.num_parents = 0;
> +
> +	socfpga_sysmgr->hw.init = &init;
> +	clk = clk_register(NULL, &socfpga_sysmgr->hw);
> +	if (WARN_ON(IS_ERR(clk))) {
> +		kfree(socfpga_sysmgr);
> +		return;
> +	}
> +	rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +	if (WARN_ON(rc))
> +		return;
> +}
> +
> +static void __init sysmgr_init(struct device_node *node)
> +{
> +	socfpga_sysmgr_init(node, &clk_sysmgr_sdmmc_ops);
> +}
> +CLK_OF_DECLARE(sysmgr, "altr,sysmgr-sdmmc-sdr", sysmgr_init);

Along the same lines: the "altr,sysmgr-sdmmc-sdr" string doesn't make
sense here, it should be a name given to the clock register layout
of the sysmgr, which is presumably identical for a number of clocks,
and cannot contain "sdmmc-sdr" in the name.

	Arnd

Re: [PATCHv3 4/4] arm: dts: Add support for SD/MMC on SOCFPGA

[Arnd Bergmann]

On Wednesday 04 December 2013, dinguyen@altera.com wrote:

> +
> +* 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@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>, <&sysmgr_sdr_mmc>;
> +		clock-names = "biu", "ciu", "sysmgr-sdr-mmc";

You add a "sysmgr-sdr-mmc" clock here without documenting it. I think what you
actually mean here is

> +		clocks = <&l4_mp_clk>, <&sysmgr_sdr_mmc>;
> +		clock-names = "biu", "ciu";

i.e. the <&sysmgr_sdr_mmc> clock is actually your "ciu". If I understand your
code correctly, the dw-mshc has exactly two clock inputs, biu and ciu, and
you use sysmgr to provide ciu. The driver code already contains logic to
set the rate of the ciu clock, and you just need to hook into that.

	Arnd

Re: [PATCHv3 2/4] arm: dts: Add a system manager compatible property

[Mark Rutland]

On Wed, Dec 04, 2013 at 10:52:54PM +0000, dinguyen@altera.com wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> The "altr,sysmgr-sdmmc-sdr" compatible property is used for the SOCFPGA
> clk-sysmgr driver. This property represents the register inside the
> system manager that controls the clock phase of the SD/MMC driver.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
> ---
> v3: Cannot use the syscon driver along with the clock because as of v3.13-rc1,
> the syscon driver is loaded after the clocks.
> v2: Add syscon
> ---
>  .../bindings/arm/altera/socfpga-system.txt         |   10 ++++++++++
>  arch/arm/boot/dts/socfpga.dtsi                     |   14 +++++++++++---
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-system.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-system.txt
> index f4d04a0..7a6c7ed 100644
> --- a/Documentation/devicetree/bindings/arm/altera/socfpga-system.txt
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-system.txt
> @@ -5,9 +5,19 @@ Required properties:
>  - reg : Should contain 1 register ranges(address and length)
>  - cpu1-start-addr : CPU1 start address in hex.
>  
> +Optional properties:
> +- compatible = "altr,sysmgr-sdmmc-sdr". This compatible property is used
> +to represent the clock phase settings for the SD/MMC IP.
> +

This makes no sense with the example below. This is _not_ an optional
property of the sysmgr node, this is a poor description of a child node.

>  Example:
>  	 sysmgr@ffd08000 {
>  		compatible = "altr,sys-mgr";
>  		reg = <0xffd08000 0x1000>;
>  		cpu1-start-addr = <0xffd080c4>;
> +
> +		sysmgr_sdr_mmc: sysmgr_sdr_mmc {
> +			#clock-cells = <0>;
> +			compatible = "altr,sysmgr-sdmmc-sdr";
> +			reg = <0x108 1>;

What's this reg?

Is # clock-cells required?

Neither were described in the binding.

Mark.

Re: [PATCHv3 3/4] mmc: dw_mmc-socfpga: Clean up SOCFPGA platform specific funcationality

[Mark Rutland]

On Wed, Dec 04, 2013 at 10:52:55PM +0000, dinguyen@altera.com wrote:
> 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. We can use the normal clock API to set the SDR
> settings.
> 
> 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.

If this property isn't necessary, please mark it as deprecated in the
documentation.

[...]

> +	if (IS_ERR(sysmgr_clk))
> +		dev_err(host->dev, "sysmgr-sdr-mmc not available\n");
> +	else {
> +		clk_disable_unprepare(host->ciu_clk);
> +		clk_prepare_enable(sysmgr_clk);
> +		clk_prepare_enable(host->ciu_clk);
> +	}
>  	return 0;
>  }

This looks a little odd. Should you not return an error if you don't
have the requisite clocks?

[...]

> -	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);
> -	}

Is this property deprecated?

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

Deprecated too?

Thanks,
Mark.

Re: [PATCHv3 4/4] arm: dts: Add support for SD/MMC on SOCFPGA

[Dinh Nguyen]

On Thu, 2013-12-05 at 04:07 +0100, Arnd Bergmann wrote:
> On Wednesday 04 December 2013, dinguyen@altera.com wrote:
> 
> > +
> > +* 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@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>, <&sysmgr_sdr_mmc>;
> > +		clock-names = "biu", "ciu", "sysmgr-sdr-mmc";
> 
> You add a "sysmgr-sdr-mmc" clock here without documenting it. I think what you
> actually mean here is
> 
> > +		clocks = <&l4_mp_clk>, <&sysmgr_sdr_mmc>;
> > +		clock-names = "biu", "ciu";
> 
> i.e. the <&sysmgr_sdr_mmc> clock is actually your "ciu". If I understand your
> code correctly, the dw-mshc has exactly two clock inputs, biu and ciu, and
> you use sysmgr to provide ciu. The driver code already contains logic to
> set the rate of the ciu clock, and you just need to hook into that.

Ah yes! This is fantastic. I can definitely just re-use the ciu-clk
hook.

Thanks!

Dinh
> 
> 	Arnd
> 

Re: [PATCHv3 3/4] mmc: dw_mmc-socfpga: Clean up SOCFPGA platform specific funcationality

[Dinh Nguyen]

On Thu, 2013-12-05 at 11:47 +0000, Mark Rutland wrote:
> On Wed, Dec 04, 2013 at 10:52:55PM +0000, dinguyen@altera.com wrote:
> > 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. We can use the normal clock API to set the SDR
> > settings.
> > 
> > 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.
> 
> If this property isn't necessary, please mark it as deprecated in the
> documentation.

I would deprecate it. If only there was documentation for it.

> 
> [...]
> 
> > +	if (IS_ERR(sysmgr_clk))
> > +		dev_err(host->dev, "sysmgr-sdr-mmc not available\n");
> > +	else {
> > +		clk_disable_unprepare(host->ciu_clk);
> > +		clk_prepare_enable(sysmgr_clk);
> > +		clk_prepare_enable(host->ciu_clk);
> > +	}
> >  	return 0;
> >  }
> 
> This looks a little odd. Should you not return an error if you don't
> have the requisite clocks?
> 
> [...]
> 
> > -	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);
> > -	}
> 
> Is this property deprecated?

"altr,sys-mgr" is used in other places, so no deprecated is necessary.

> 
> > -
> > -	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);
> 
> Deprecated too?

Once again "altr,dw-mshc-sdr-timing" was not documented. But either way,
v4 of this patch will not be introducing any new bindings. Thanks to
Arnd's suggestion, I found a way to hook into the existing socfpga clock
driver.

Thanks alot for your review!

Dinh

> 
> Thanks,
> Mark.
>