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

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

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

Hi,

This is v4 of the patch series to enable SD/MMC on the SOCFPGA platform.

V4 differences from V3:

* Re-use the SOCFPGA clock driver for controlling the SDR settings of the CIU
clock. By using the .prepare member of the common clk_ops structure, the settings
of the SD/MMC CIU can be done through the common clock API in the SD/MMC driver.

* Re-use exising binding that is on the Rockchip. The "rockchip,rk2928-dw-mshc"
binding that will also work for SOCFPGA.

* Remove dw_mmm-socfpga.c that is platform specific to SOCFPGA as the only
require platform specific code is already done with "rockchip,rk2928-dw-mshc".

* Add error checking for getting "samsung,dw-mshc-sdr-timing".

* Add support for SD/MMC to socfpga_defconfig.

* Add SD/MMC dts entries for socfpga_arria5.

Thanks,

Dinh Nguyen (4):
  arm: dts: Add support for SD/MMC on SOCFPGA
  clk: socfpga: Add a hook for SD/MMC driver to control CIU clock
    settings
  mmc: dw_mmc-socfpga: Remove the SOCFPGA specific platform for dw_mmc
  ARM: socfpga_defconfig: enable SD/MMC support

 arch/arm/boot/dts/socfpga.dtsi          |   11 +++
 arch/arm/boot/dts/socfpga_arria5.dtsi   |   12 +++
 arch/arm/boot/dts/socfpga_cyclone5.dtsi |   12 +++
 arch/arm/boot/dts/socfpga_vt.dts        |   12 +++
 arch/arm/configs/socfpga_defconfig      |    2 +
 drivers/clk/socfpga/clk.c               |   22 +++++
 drivers/mmc/host/Kconfig                |    8 --
 drivers/mmc/host/dw_mmc-socfpga.c       |  138 -------------------------------
 8 files changed, 71 insertions(+), 146 deletions(-)
 delete mode 100644 drivers/mmc/host/dw_mmc-socfpga.c

-- 
1.7.9.5

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

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

Re-use the "rockchip,rk2928-dw-mshc" binding that will support SD/MMC on
Altera's SOCFPGA platform.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
---
v3: Re-use "rockchip,rk2928-dw-mshc" binding
v3: none
v2: none
---
 arch/arm/boot/dts/socfpga.dtsi          |   11 +++++++++++
 arch/arm/boot/dts/socfpga_arria5.dtsi   |   12 ++++++++++++
 arch/arm/boot/dts/socfpga_cyclone5.dtsi |   12 ++++++++++++
 arch/arm/boot/dts/socfpga_vt.dts        |   12 ++++++++++++
 4 files changed, 47 insertions(+)

diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index f936476..3d9f01b 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 = "rockchip,rk2928-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@fffec600 {
 			compatible = "arm,cortex-a9-twd-timer";
diff --git a/arch/arm/boot/dts/socfpga_arria5.dtsi b/arch/arm/boot/dts/socfpga_arria5.dtsi
index a85b404..0feb15f 100644
--- a/arch/arm/boot/dts/socfpga_arria5.dtsi
+++ b/arch/arm/boot/dts/socfpga_arria5.dtsi
@@ -27,6 +27,18 @@
 			};
 		};
 
+		dwmmc0@ff704000 {
+			num-slots = <1>;
+			supports-highspeed;
+			broken-cd;
+			samsung,dw-mshc-sdr-timing = <3 0>;
+
+			slot@0 {
+				reg = <0>;
+				bus-width = <4>;
+			};
+		};
+
 		serial0@ffc02000 {
 			clock-frequency = <100000000>;
 		};
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

[PATCHv4 2/4] clk: socfpga: Add a hook for SD/MMC driver to control CIU clock settings

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

Populate the .prepare function in the clk-ops for the "sdmmc_clk" that represents
the "ciu" clock for the SD/MMC driver. The prepare function will handle setting
the correct clock-phase for the CIU clock of the SD/MMC IP.

Re-use the "rockchip,rk2928-dw-mshc" binding as it is already defined and
appropriate for the SOCFPGA platform as well.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
---
 drivers/clk/socfpga/clk.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/clk/socfpga/clk.c b/drivers/clk/socfpga/clk.c
index 60cb2f5..01baf20 100644
--- a/drivers/clk/socfpga/clk.c
+++ b/drivers/clk/socfpga/clk.c
@@ -55,7 +55,13 @@
 #define div_mask(width)	((1 << (width)) - 1)
 #define streq(a, b) (strcmp((a), (b)) == 0)
 
+#define SYSMGR_SDMMCGRP_CTRL_OFFSET	0x108
+/* SDMMC Group for System Manager defines */
+#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)          \
+	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
+
 extern void __iomem *clk_mgr_base_addr;
+extern void __iomem *sys_manager_base_addr;
 
 struct socfpga_clk {
 	struct clk_gate hw;
@@ -68,6 +74,22 @@ struct socfpga_clk {
 };
 #define to_socfpga_clk(p) container_of(p, struct socfpga_clk, hw.hw)
 
+static int sdmmc_ciuclk_prepare(struct clk_hw *hwclk)
+{
+	struct device_node *np;
+	u32 timing[2];
+	u32 hs_timing;
+
+	np = of_find_compatible_node(NULL, NULL, "rockchip,rk2928-dw-mshc");
+	if (of_property_read_u32_array(np, "samsung,dw-mshc-sdr-timing", timing, 2)) {
+		pr_err("SDMMC: cannot find samsung,dw-mshc-sdr-timing!\n");
+		return -ENODATA;
+	}
+	hs_timing = SYSMGR_SDMMC_CTRL_SET(timing[1], timing[0]);
+	writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET);
+	return 0;
+}
+
 static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
 					 unsigned long parent_rate)
 {
@@ -274,6 +296,9 @@ static void __init socfpga_gate_clk_init(struct device_node *node,
 		socfpga_clk->hw.reg = clk_mgr_base_addr + clk_gate[0];
 		socfpga_clk->hw.bit_idx = clk_gate[1];
 
+		if (streq(clk_name, "sdmmc_clk"))
+			gateclk_ops.prepare = sdmmc_ciuclk_prepare;
+
 		gateclk_ops.enable = clk_gate_ops.enable;
 		gateclk_ops.disable = clk_gate_ops.disable;
 	}
-- 
1.7.9.5

[PATCHv4 3/4] mmc: dw_mmc-socfpga: Remove the SOCFPGA specific platform for dw_mmc

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

It turns now that the only really platform specific code that is needed for
SOCFPGA is using the SDMMC_CMD_USE_HOLD_REG in the prepare_command function.
Since the Rockchip already has this functionality, re-use the code that is
already in dw_mmc-pltfm.c.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
---
 drivers/mmc/host/Kconfig          |    8 ---
 drivers/mmc/host/dw_mmc-socfpga.c |  138 -------------------------------------
 2 files changed, 146 deletions(-)
 delete mode 100644 drivers/mmc/host/dw_mmc-socfpga.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 7fc5099..6737a4f 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -567,14 +567,6 @@ config MMC_DW_EXYNOS
 	  Synopsys DesignWare Memory Card Interface driver. Select this option
 	  for platforms based on Exynos4 and Exynos5 SoC's.
 
-config MMC_DW_SOCFPGA
-	tristate "SOCFPGA specific extensions for Synopsys DW Memory Card Interface"
-	depends on MMC_DW && MFD_SYSCON
-	select MMC_DW_PLTFM
-	help
-	  This selects support for Altera SoCFPGA specific extensions to the
-	  Synopsys DesignWare Memory Card Interface driver.
-
 config MMC_DW_PCI
 	tristate "Synopsys Designware MCI support on PCI bus"
 	depends on MMC_DW && PCI
diff --git a/drivers/mmc/host/dw_mmc-socfpga.c b/drivers/mmc/host/dw_mmc-socfpga.c
deleted file mode 100644
index 3e8e53a..0000000
--- a/drivers/mmc/host/dw_mmc-socfpga.c
+++ /dev/null
@@ -1,138 +0,0 @@
-/*
- * Altera SoCFPGA Specific Extensions for Synopsys DW Multimedia Card Interface
- * driver
- *
- *  Copyright (C) 2012, Samsung Electronics Co., Ltd.
- *  Copyright (C) 2013 Altera Corporation
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * 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);
-	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;
-}
-
-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[] = {
-	{ .compatible = "altr,socfpga-dw-mshc",
-			.data = &socfpga_drv_data, },
-	{},
-};
-MODULE_DEVICE_TABLE(of, dw_mci_socfpga_match);
-
-static int dw_mci_socfpga_probe(struct platform_device *pdev)
-{
-	const struct dw_mci_drv_data *drv_data;
-	const struct of_device_id *match;
-
-	match = of_match_node(dw_mci_socfpga_match, pdev->dev.of_node);
-	drv_data = match->data;
-	return dw_mci_pltfm_register(pdev, drv_data);
-}
-
-static struct platform_driver dw_mci_socfpga_pltfm_driver = {
-	.probe		= dw_mci_socfpga_probe,
-	.remove		= __exit_p(dw_mci_pltfm_remove),
-	.driver		= {
-		.name		= "dwmmc_socfpga",
-		.of_match_table	= dw_mci_socfpga_match,
-		.pm		= &dw_mci_pltfm_pmops,
-	},
-};
-
-module_platform_driver(dw_mci_socfpga_pltfm_driver);
-
-MODULE_DESCRIPTION("Altera SOCFPGA Specific DW-MSHC Driver Extension");
-MODULE_LICENSE("GPL v2");
-MODULE_ALIAS("platform:dwmmc-socfpga");
-- 
1.7.9.5

[PATCHv4 4/4] ARM: socfpga_defconfig: enable SD/MMC support

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

Enables the dw_mmc driver for SOCFPGA.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
---
 arch/arm/configs/socfpga_defconfig |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/socfpga_defconfig b/arch/arm/configs/socfpga_defconfig
index 4e1ce21..8fff96ba 100644
--- a/arch/arm/configs/socfpga_defconfig
+++ b/arch/arm/configs/socfpga_defconfig
@@ -82,3 +82,5 @@ CONFIG_DEBUG_INFO=y
 CONFIG_ENABLE_DEFAULT_TRACERS=y
 CONFIG_DEBUG_USER=y
 CONFIG_XZ_DEC=y
+CONFIG_MMC=y
+CONFIG_MMC_DW=y
-- 
1.7.9.5

Re: [PATCHv4 2/4] clk: socfpga: Add a hook for SD/MMC driver to control CIU clock settings

[Arnd Bergmann]

On Thursday 05 December 2013, dinguyen@altera.com wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> Populate the .prepare function in the clk-ops for the "sdmmc_clk" that represents
> the "ciu" clock for the SD/MMC driver. The prepare function will handle setting
> the correct clock-phase for the CIU clock of the SD/MMC IP.
> 
> Re-use the "rockchip,rk2928-dw-mshc" binding as it is already defined and
> appropriate for the SOCFPGA platform as well.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>

I think with this series we're getting closer to where things should be, but
there are still a number of the same problems. Maybe I have misunderstood
a few things here in how the clocks fit together, but here is what still
looks wrong about this patch:

> diff --git a/drivers/clk/socfpga/clk.c b/drivers/clk/socfpga/clk.c
> index 60cb2f5..01baf20 100644
> --- a/drivers/clk/socfpga/clk.c
> +++ b/drivers/clk/socfpga/clk.c
> @@ -55,7 +55,13 @@
>  #define div_mask(width)	((1 << (width)) - 1)
>  #define streq(a, b) (strcmp((a), (b)) == 0)
>  
> +#define SYSMGR_SDMMCGRP_CTRL_OFFSET	0x108
> +/* SDMMC Group for System Manager defines */
> +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)          \
> +	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
> +
>  extern void __iomem *clk_mgr_base_addr;
> +extern void __iomem *sys_manager_base_addr;

As mentioned in yesterday's comments, you should not have the 'extern'
declaration for sys_manager_base_addr here. In fact, the existing
clk_mgr_base_addr declaration is just as wrong.

> +static int sdmmc_ciuclk_prepare(struct clk_hw *hwclk)
> +{
> +	struct device_node *np;
> +	u32 timing[2];
> +	u32 hs_timing;
> +
> +	np = of_find_compatible_node(NULL, NULL, "rockchip,rk2928-dw-mshc");
> +	if (of_property_read_u32_array(np, "samsung,dw-mshc-sdr-timing", timing, 2)) {
> +		pr_err("SDMMC: cannot find samsung,dw-mshc-sdr-timing!\n");
> +		return -ENODATA;
> +	}
> +	hs_timing = SYSMGR_SDMMC_CTRL_SET(timing[1], timing[0]);
> +	writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET);
> +	return 0;
> +}
> +

And in now way should the clock provider code look into the DT properties of the
clock consumer. From what I can tell, the dw-mshc code already interprets the
"samsung,dw-mshc-sdr-timing" property and uses the data to pass the correct
clock rate using 'clk_set_rate()'. The clock code should only use the data passed
in the argument to that function to set up the registers and not need to know
at all who is setting it.

I am a little confused though what the SYSMGR_SDMMCGRP_CTRL_OFFSET register actually
does. It looks like this is just a clock divider, which should be represented as
a separate clock node (as you had in v3) and compute the correct factor from the
requested clock rate and the parent clock rate.

	Arnd

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

[Arnd Bergmann]

On Thursday 05 December 2013, dinguyen@altera.com wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> Re-use the "rockchip,rk2928-dw-mshc" binding that will support SD/MMC on
> Altera's SOCFPGA platform.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
> ---
> v3: Re-use "rockchip,rk2928-dw-mshc" binding
> v3: none
> v2: none
> ---
>  arch/arm/boot/dts/socfpga.dtsi          |   11 +++++++++++
>  arch/arm/boot/dts/socfpga_arria5.dtsi   |   12 ++++++++++++
>  arch/arm/boot/dts/socfpga_cyclone5.dtsi |   12 ++++++++++++
>  arch/arm/boot/dts/socfpga_vt.dts        |   12 ++++++++++++
>  4 files changed, 47 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index f936476..3d9f01b 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 = "rockchip,rk2928-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";
> +		};


I think it's great that you can reuse the existing driver, but I'd recommend
using a generic compatible string here in addition to one that identifies
your specific implementation. You can list "rockchip,rk2928-dw-mshc" as
well, but that is probably not necessary.

What I'd expect to see here is either

	compatible = "altr,socfpga-dw-mshc", "rockchip,rk2928-dw-mshc", "snps,dw-mshc";

or 

	compatible = "altr,socfpga-dw-mshc", "rockchip,rk2928-dw-mshc", "snps,dw-mshc";


It's probably not too late to generalize the SDMMC_CMD_USE_HOLD_REG
handling, which is currently the only difference between the generic
"snps,dw-mshc" and the newer "rockchip,rk2928-dw-mshc" variant.

It's quite likely that all implementations should actually set
SDMMC_CMD_USE_HOLD_REG (if both rockchips and altera set it) and you
don't need to have any distinction in the dw_mmc-pltfm.c driver at all.

If it's actually needed on some but not others, you could add a binary
DT property to tell the driver about it rather than keying it off of
the compatible string. If it's needed only on older (or only on newer)
versions of the dw-mshc design, it could be encoded as a version in the
string, such as "snps,dw-mshc-1.234".

	Arnd

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

[Dinh Nguyen]

On Thu, 2013-12-05 at 22:08 +0100, Arnd Bergmann wrote:
> On Thursday 05 December 2013, dinguyen@altera.com wrote:
> > From: Dinh Nguyen <dinguyen@altera.com>
> > 
> > Re-use the "rockchip,rk2928-dw-mshc" binding that will support SD/MMC on
> > Altera's SOCFPGA platform.
> > 
> > Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
> > ---
> > v3: Re-use "rockchip,rk2928-dw-mshc" binding
> > v3: none
> > v2: none
> > ---
> >  arch/arm/boot/dts/socfpga.dtsi          |   11 +++++++++++
> >  arch/arm/boot/dts/socfpga_arria5.dtsi   |   12 ++++++++++++
> >  arch/arm/boot/dts/socfpga_cyclone5.dtsi |   12 ++++++++++++
> >  arch/arm/boot/dts/socfpga_vt.dts        |   12 ++++++++++++
> >  4 files changed, 47 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> > index f936476..3d9f01b 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 = "rockchip,rk2928-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";
> > +		};
> 
> 
> I think it's great that you can reuse the existing driver, but I'd recommend
> using a generic compatible string here in addition to one that identifies
> your specific implementation. You can list "rockchip,rk2928-dw-mshc" as
> well, but that is probably not necessary.
> 
> What I'd expect to see here is either
> 
> 	compatible = "altr,socfpga-dw-mshc", "rockchip,rk2928-dw-mshc", "snps,dw-mshc";
> 
> or 
> 
> 	compatible = "altr,socfpga-dw-mshc", "rockchip,rk2928-dw-mshc", "snps,dw-mshc";
> 
> 
> It's probably not too late to generalize the SDMMC_CMD_USE_HOLD_REG
> handling, which is currently the only difference between the generic
> "snps,dw-mshc" and the newer "rockchip,rk2928-dw-mshc" variant.

Ok, I will try to generalize the driver.

> 
> It's quite likely that all implementations should actually set
> SDMMC_CMD_USE_HOLD_REG (if both rockchips and altera set it) and you
> don't need to have any distinction in the dw_mmc-pltfm.c driver at all.
> 
> If it's actually needed on some but not others, you could add a binary
> DT property to tell the driver about it rather than keying it off of
> the compatible string. If it's needed only on older (or only on newer)
> versions of the dw-mshc design, it could be encoded as a version in the
> string, such as "snps,dw-mshc-1.234".

Perhaps Seungwon and Chris might have an opinion on this:

>From the Synopsys databook for this IP, using the SDMMC_CMD_USE_HOLD_REG
is not recommended for SDR104, SDR50 and DDR50 speed modes. For other
speeds, SDR12, and SDR25, it would be necessary to use
SDMMC_CMD_USE_HOLD_REG.

I am referencing v2.50a of the Synopsys DesignWare Cores Mobile Storage
Host Databook.

So I think a binary DT property should suffice?

Thanks,
Dinh
> 
> 	Arnd
> 

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

[Arnd Bergmann]

On Thursday 05 December 2013, Dinh Nguyen wrote:
> Perhaps Seungwon and Chris might have an opinion on this:
> 
> From the Synopsys databook for this IP, using the SDMMC_CMD_USE_HOLD_REG
> is not recommended for SDR104, SDR50 and DDR50 speed modes. For other
> speeds, SDR12, and SDR25, it would be necessary to use
> SDMMC_CMD_USE_HOLD_REG.
> 
> I am referencing v2.50a of the Synopsys DesignWare Cores Mobile Storage
> Host Databook.
> 
> So I think a binary DT property should suffice?

Well, if it depends on the interface speed, it is something the driver should
actually be able to figure out itself, since it knows the speed that is used.

That way, you don't even need a property or per-platform compatible string.

	Arnd

Re: [PATCHv4 2/4] clk: socfpga: Add a hook for SD/MMC driver to control CIU clock settings

[Dinh Nguyen]

On 12/5/13 2:57 PM, Arnd Bergmann wrote:
> On Thursday 05 December 2013, dinguyen@altera.com wrote:
>> From: Dinh Nguyen <dinguyen@altera.com>
>>
>> Populate the .prepare function in the clk-ops for the "sdmmc_clk" that represents
>> the "ciu" clock for the SD/MMC driver. The prepare function will handle setting
>> the correct clock-phase for the CIU clock of the SD/MMC IP.
>>
>> Re-use the "rockchip,rk2928-dw-mshc" binding as it is already defined and
>> appropriate for the SOCFPGA platform as well.
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
> I think with this series we're getting closer to where things should be, but
> there are still a number of the same problems. Maybe I have misunderstood
> a few things here in how the clocks fit together, but here is what still
> looks wrong about this patch:
>
>> diff --git a/drivers/clk/socfpga/clk.c b/drivers/clk/socfpga/clk.c
>> index 60cb2f5..01baf20 100644
>> --- a/drivers/clk/socfpga/clk.c
>> +++ b/drivers/clk/socfpga/clk.c
>> @@ -55,7 +55,13 @@
>>  #define div_mask(width)	((1 << (width)) - 1)
>>  #define streq(a, b) (strcmp((a), (b)) == 0)
>>  
>> +#define SYSMGR_SDMMCGRP_CTRL_OFFSET	0x108
>> +/* SDMMC Group for System Manager defines */
>> +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)          \
>> +	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
>> +
>>  extern void __iomem *clk_mgr_base_addr;
>> +extern void __iomem *sys_manager_base_addr;
> As mentioned in yesterday's comments, you should not have the 'extern'
> declaration for sys_manager_base_addr here. In fact, the existing
> clk_mgr_base_addr declaration is just as wrong.
Yes, will fix..
>
>> +static int sdmmc_ciuclk_prepare(struct clk_hw *hwclk)
>> +{
>> +	struct device_node *np;
>> +	u32 timing[2];
>> +	u32 hs_timing;
>> +
>> +	np = of_find_compatible_node(NULL, NULL, "rockchip,rk2928-dw-mshc");
>> +	if (of_property_read_u32_array(np, "samsung,dw-mshc-sdr-timing", timing, 2)) {
>> +		pr_err("SDMMC: cannot find samsung,dw-mshc-sdr-timing!\n");
>> +		return -ENODATA;
>> +	}
>> +	hs_timing = SYSMGR_SDMMC_CTRL_SET(timing[1], timing[0]);
>> +	writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET);
>> +	return 0;
>> +}
>> +
> And in now way should the clock provider code look into the DT properties of the
> clock consumer. From what I can tell, the dw-mshc code already interprets the
> "samsung,dw-mshc-sdr-timing" property and uses the data to pass the correct
> clock rate using 'clk_set_rate()'. The clock code should only use the data passed
> in the argument to that function to set up the registers and not need to know
> at all who is setting it.
You're right, the dw-mshc code interprets "samsung,dw-mshc-sdr-timing",
but it is
currently doing it in the platform specific code(dw_mmc-exynos and
dw_mmc-socfpga).
I was thinking I can remove the platform specific for SOCFPGA. But
perhaps now that
patch "mmc: dw_mmc: Make the use of the hold reg generic" is
progressing, and
that patch reads "samsung,dw-mshc-sdr-timing" in the generic dw_mmc
code, I can now
pass it through the clk_set_rate call.

My initial thought to read "samsung,dw-mshc-sdr-timing" in the socfpga clock
driver's was that the SOCFPGA's sdmmc_clk only role in life is to feed
the sdmmc block. So
reading the phase shift bits that adjust this clock does seem weird, but
a straightforward way
of doing it. I stuck it in the function in .prepare function because the
setting of the clock's phase
shift also requires that CIU clock be turned off while the bits are
toggled, so the prepare function
seems to work perfectly. So I think you're only concern is that I should
figure out a way to pass
the values of "samsung,dw-mshc-sdr-timing" from the dw_mmc driver to the
clock driver, correct?

Yes, I can read the "samsung,dw-mshc-sdr-timing" in the dw_mmc code and
pass it to the clock
driver using clk_set_rate(), but this method seems more cumbersome, as
this clk_set_rate does not
have anything to do with setting the rate, but only the phase shift
settings of the clock.
>
> I am a little confused though what the SYSMGR_SDMMCGRP_CTRL_OFFSET register actually
> does. It looks like this is just a clock divider, which should be represented as
> a separate clock node (as you had in v3) and compute the correct factor from the
> requested clock rate and the parent clock rate.
The SYSMGR_SDMMCGRP_CTRL_OFFSET is just putting the 2 values of
"samsung,dw-mshc-sdr-timing"
into a a single u32 value to be written to the register.
"samsung,dw-mshc-sdr-timing" has 2 values that
controls the phase shift of 2 clocks, the drive clock and the sample clock.

Thanks,
Dinh
>
> 	Arnd

Re: [PATCHv4 2/4] clk: socfpga: Add a hook for SD/MMC driver to control CIU clock settings

[Arnd Bergmann]

On Tuesday 10 December 2013, Dinh Nguyen wrote:
> On 12/5/13 2:57 PM, Arnd Bergmann wrote:

> > And in now way should the clock provider code look into the DT properties of the
> > clock consumer. From what I can tell, the dw-mshc code already interprets the
> > "samsung,dw-mshc-sdr-timing" property and uses the data to pass the correct
> > clock rate using 'clk_set_rate()'. The clock code should only use the data passed
> > in the argument to that function to set up the registers and not need to know
> > at all who is setting it.
> You're right, the dw-mshc code interprets "samsung,dw-mshc-sdr-timing",
> but it is
> currently doing it in the platform specific code(dw_mmc-exynos and
> dw_mmc-socfpga).
> I was thinking I can remove the platform specific for SOCFPGA. But
> perhaps now that
> patch "mmc: dw_mmc: Make the use of the hold reg generic" is
> progressing, and
> that patch reads "samsung,dw-mshc-sdr-timing" in the generic dw_mmc
> code, I can now
> pass it through the clk_set_rate call.

Ok, good.

> My initial thought to read "samsung,dw-mshc-sdr-timing" in the socfpga clock
> driver's was that the SOCFPGA's sdmmc_clk only role in life is to feed
> the sdmmc block. So
> reading the phase shift bits that adjust this clock does seem weird, but
> a straightforward way
> of doing it. I stuck it in the function in .prepare function because the
> setting of the clock's phase
> shift also requires that CIU clock be turned off while the bits are
> toggled, so the prepare function
> seems to work perfectly. So I think you're only concern is that I should
> figure out a way to pass
> the values of "samsung,dw-mshc-sdr-timing" from the dw_mmc driver to the
> clock driver, correct?
> 
> Yes, I can read the "samsung,dw-mshc-sdr-timing" in the dw_mmc code and
> pass it to the clock
> driver using clk_set_rate(), but this method seems more cumbersome, as
> this clk_set_rate does not
> have anything to do with setting the rate, but only the phase shift
> settings of the clock.

I see, that does indeed sound a bit strange and is probably not the
best API. I'm not that familiar with what is actually going on at the
hardware level. Can you explain in a little more detail what the phase
setting is about? What are the possible values here and what units
are being used?

> > I am a little confused though what the SYSMGR_SDMMCGRP_CTRL_OFFSET register actually
> > does. It looks like this is just a clock divider, which should be represented as
> > a separate clock node (as you had in v3) and compute the correct factor from the
> > requested clock rate and the parent clock rate.
> The SYSMGR_SDMMCGRP_CTRL_OFFSET is just putting the 2 values of
> "samsung,dw-mshc-sdr-timing"
> into a a single u32 value to be written to the register.
> "samsung,dw-mshc-sdr-timing" has 2 values that
> controls the phase shift of 2 clocks, the drive clock and the sample clock.

Do other clocks ever have the same requirement? Would it make sense to have
a generic clk_set_phase interface for this?

If not, could the values alternatively (i.e. departing from the samsung
method) be encoded in the clock specifier? That would come down to setting
#clock-cells = <2> for this clock and using a complex clock in the mshc
node to point at it using

	<&phandle $phase1 $phase2>
	Arnd

	Arnd

Re: [PATCHv4 2/4] clk: socfpga: Add a hook for SD/MMC driver to control CIU clock settings

[Dinh Nguyen]

On 12/10/13 12:15 PM, Arnd Bergmann wrote:
> On Tuesday 10 December 2013, Dinh Nguyen wrote:
>> On 12/5/13 2:57 PM, Arnd Bergmann wrote:
>>> And in now way should the clock provider code look into the DT properties of the
>>> clock consumer. From what I can tell, the dw-mshc code already interprets the
>>> "samsung,dw-mshc-sdr-timing" property and uses the data to pass the correct
>>> clock rate using 'clk_set_rate()'. The clock code should only use the data passed
>>> in the argument to that function to set up the registers and not need to know
>>> at all who is setting it.
>> You're right, the dw-mshc code interprets "samsung,dw-mshc-sdr-timing",
>> but it is
>> currently doing it in the platform specific code(dw_mmc-exynos and
>> dw_mmc-socfpga).
>> I was thinking I can remove the platform specific for SOCFPGA. But
>> perhaps now that
>> patch "mmc: dw_mmc: Make the use of the hold reg generic" is
>> progressing, and
>> that patch reads "samsung,dw-mshc-sdr-timing" in the generic dw_mmc
>> code, I can now
>> pass it through the clk_set_rate call.
> Ok, good.
>
>> My initial thought to read "samsung,dw-mshc-sdr-timing" in the socfpga clock
>> driver's was that the SOCFPGA's sdmmc_clk only role in life is to feed
>> the sdmmc block. So
>> reading the phase shift bits that adjust this clock does seem weird, but
>> a straightforward way
>> of doing it. I stuck it in the function in .prepare function because the
>> setting of the clock's phase
>> shift also requires that CIU clock be turned off while the bits are
>> toggled, so the prepare function
>> seems to work perfectly. So I think you're only concern is that I should
>> figure out a way to pass
>> the values of "samsung,dw-mshc-sdr-timing" from the dw_mmc driver to the
>> clock driver, correct?
>>
>> Yes, I can read the "samsung,dw-mshc-sdr-timing" in the dw_mmc code and
>> pass it to the clock
>> driver using clk_set_rate(), but this method seems more cumbersome, as
>> this clk_set_rate does not
>> have anything to do with setting the rate, but only the phase shift
>> settings of the clock.
> I see, that does indeed sound a bit strange and is probably not the
> best API. I'm not that familiar with what is actually going on at the
> hardware level. Can you explain in a little more detail what the phase
> setting is about? What are the possible values here and what units
> are being used?
The phase shift settings for this clock is a way for the IP block to
meet the delay and hold
timing requirements for the different data modes of the SD. For example
by shifting the input
clock that is feeding the CIU by 90 degrees in SDR25 mode, the minimum
hold/delay time of 2ns
is achieved.

The values can be:
0  -> 0 degrees
0x1 -> 45 degrees
0x2 -> 90 degrees
0x3 -> 135 degrees
0x4 -> 180 degrees
0x5 -> 225 degrees
0x6 -> 270 degrees
0x7 -> 315 degrees
>
>>> I am a little confused though what the SYSMGR_SDMMCGRP_CTRL_OFFSET register actually
>>> does. It looks like this is just a clock divider, which should be represented as
>>> a separate clock node (as you had in v3) and compute the correct factor from the
>>> requested clock rate and the parent clock rate.
>> The SYSMGR_SDMMCGRP_CTRL_OFFSET is just putting the 2 values of
>> "samsung,dw-mshc-sdr-timing"
>> into a a single u32 value to be written to the register.
>> "samsung,dw-mshc-sdr-timing" has 2 values that
>> controls the phase shift of 2 clocks, the drive clock and the sample clock.
> Do other clocks ever have the same requirement? Would it make sense to have
> a generic clk_set_phase interface for this?
I am not aware of any other clocks that need these phase adjustments.
> If not, could the values alternatively (i.e. departing from the samsung
> method) be encoded in the clock specifier? That would come down to setting
> #clock-cells = <2> for this clock and using a complex clock in the mshc
> node to point at it using
>
> 	<&phandle $phase1 $phase2>
Hmm..this could also be done. I just sent out a V6 that uses the
clk_set_rate API. I believe
that there can be other platforms that can use this approach.

Thanks,
Dinh
> 	Arnd
>
> 	Arnd