[PATCH 5/5] mmc: dw_mmc: Add support DW SD/MMC driver on SOCFPGA

[PATCH 5/5] mmc: dw_mmc: Add support DW SD/MMC driver on SOCFPGA

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

Add platform specific functionality for the DW SD/MMC driver for
SoCFPGA. Move SDMMC_CMD_USE_HOLD_REG to dw_mmc.h so other platforms
can use this define.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
CC: Seungwon Jeon <tgih.jun@samsung.com>
CC: Jaehoon Chung <jh80.chung@samsung.com>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Olof Johansson <olof@lixom.net>
CC: Pavel Machek <pavel@denx.de>
CC: linux-mmc@vger.kernel.org
---
 drivers/mmc/host/Kconfig          |    8 +++
 drivers/mmc/host/Makefile         |    1 +
 drivers/mmc/host/dw_mmc-exynos.c  |    2 -
 drivers/mmc/host/dw_mmc-socfpga.c |  141 +++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/dw_mmc.h         |    1 +
 5 files changed, 151 insertions(+), 2 deletions(-)
 create mode 100644 drivers/mmc/host/dw_mmc-socfpga.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 9ab8f8d..1be2289 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -556,6 +556,14 @@ 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
+	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/Makefile b/drivers/mmc/host/Makefile
index cd32280..67718c1 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_SDH_BFIN)		+= bfin_sdh.o
 obj-$(CONFIG_MMC_DW)		+= dw_mmc.o
 obj-$(CONFIG_MMC_DW_PLTFM)	+= dw_mmc-pltfm.o
 obj-$(CONFIG_MMC_DW_EXYNOS)	+= dw_mmc-exynos.o
+obj-$(CONFIG_MMC_DW_SOCFPGA)	+= dw_mmc-socfpga.o
 obj-$(CONFIG_MMC_DW_PCI)	+= dw_mmc-pci.o
 obj-$(CONFIG_MMC_SH_MMCIF)	+= sh_mmcif.o
 obj-$(CONFIG_MMC_JZ4740)	+= jz4740_mmc.o
diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
index f013e7e..866edef 100644
--- a/drivers/mmc/host/dw_mmc-exynos.c
+++ b/drivers/mmc/host/dw_mmc-exynos.c
@@ -31,8 +31,6 @@
 					SDMMC_CLKSEL_CCLK_DRIVE(y) |	\
 					SDMMC_CLKSEL_CCLK_DIVIDER(z))
 
-#define SDMMC_CMD_USE_HOLD_REG		BIT(29)
-
 #define EXYNOS4210_FIXED_CIU_CLK_DIV	2
 #define EXYNOS4412_FIXED_CIU_CLK_DIV	4
 
diff --git a/drivers/mmc/host/dw_mmc-socfpga.c b/drivers/mmc/host/dw_mmc-socfpga.c
new file mode 100644
index 0000000..7eb3163
--- /dev/null
+++ b/drivers/mmc/host/dw_mmc-socfpga.c
@@ -0,0 +1,141 @@
+/*
+ * 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/mmc/host.h>
+#include <linux/mmc/dw_mmc.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.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)		\
+	((((drvsel) << 0) & 0x7) | (((smplsel) << 3) & 0x38))
+
+extern void __iomem *sys_manager_base_addr;
+
+/* SOCFPGA implementation specific driver private data */
+struct dw_mci_socfpga_priv_data {
+	u8	ciu_div;
+	u32	hs_timing;
+};
+
+static int dw_mci_socfpga_priv_init(struct dw_mci *host)
+{
+	struct dw_mci_socfpga_priv_data *priv;
+	struct device *dev = host->dev;
+	int pwr_en;
+
+	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		dev_err(host->dev, "mem alloc failed for private data\n");
+		return -ENOMEM;
+	}
+
+	host->priv = priv;
+
+	if (of_property_read_u32(dev->of_node, "pwr-en", &pwr_en)) {
+		dev_info(dev, "couldn't determine pwr-en, assuming pwr-en = 0\n");
+		pwr_en = 0;
+	}
+
+	/* Set PWREN bit */
+	mci_writel(host, PWREN, pwr_en);
+
+	return 0;
+}
+
+static int dw_mci_socfpga_setup_clock(struct dw_mci *host)
+{
+	struct dw_mci_socfpga_priv_data *priv = host->priv;
+
+	clk_disable(host->ciu_clk);
+	writel(priv->hs_timing, sys_manager_base_addr +
+		SYSMGR_SDMMCGRP_CTRL_OFFSET);
+	clk_enable(host->ciu_clk);
+
+	host->bus_hz /= priv->ciu_div;
+	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 = host->priv;
+	struct device_node *np = host->dev->of_node;
+	u32 timing[2];
+	u32 div = 0;
+	int ret;
+
+	of_property_read_u32(np, "altr,dw-mshc-ciu-div", &div);
+	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]);
+	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);
+
+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	= of_match_ptr(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");
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 0b74189..3700cb2 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -111,6 +111,7 @@
 #define SDMMC_INT_ERROR			0xbfc2
 /* Command register defines */
 #define SDMMC_CMD_START			BIT(31)
+#define SDMMC_CMD_USE_HOLD_REG	BIT(29)
 #define SDMMC_CMD_CCS_EXP		BIT(23)
 #define SDMMC_CMD_CEATA_RD		BIT(22)
 #define SDMMC_CMD_UPD_CLK		BIT(21)
-- 
1.7.9.5

Re: [PATCH 5/5] mmc: dw_mmc: Add support DW SD/MMC driver on SOCFPGA

[Jaehoon Chung]

On 05/15/2013 07:52 AM, dinguyen@altera.com wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> Add platform specific functionality for the DW SD/MMC driver for
> SoCFPGA. Move SDMMC_CMD_USE_HOLD_REG to dw_mmc.h so other platforms
> can use this define.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
> CC: Seungwon Jeon <tgih.jun@samsung.com>
> CC: Jaehoon Chung <jh80.chung@samsung.com>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Olof Johansson <olof@lixom.net>
> CC: Pavel Machek <pavel@denx.de>
> CC: linux-mmc@vger.kernel.org
> ---
>  drivers/mmc/host/Kconfig          |    8 +++
>  drivers/mmc/host/Makefile         |    1 +
>  drivers/mmc/host/dw_mmc-exynos.c  |    2 -
>  drivers/mmc/host/dw_mmc-socfpga.c |  141 +++++++++++++++++++++++++++++++++++++
>  drivers/mmc/host/dw_mmc.h         |    1 +
>  5 files changed, 151 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/mmc/host/dw_mmc-socfpga.c
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 9ab8f8d..1be2289 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -556,6 +556,14 @@ 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
SOCFPGA? I think good that should be changed more generic.
> +	tristate "SOCFPGA specific extensions for Synopsys DW Memory Card Interface"
> +	depends on MMC_DW
> +	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/Makefile b/drivers/mmc/host/Makefile
> index cd32280..67718c1 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -42,6 +42,7 @@ obj-$(CONFIG_SDH_BFIN)		+= bfin_sdh.o
>  obj-$(CONFIG_MMC_DW)		+= dw_mmc.o
>  obj-$(CONFIG_MMC_DW_PLTFM)	+= dw_mmc-pltfm.o
>  obj-$(CONFIG_MMC_DW_EXYNOS)	+= dw_mmc-exynos.o
> +obj-$(CONFIG_MMC_DW_SOCFPGA)	+= dw_mmc-socfpga.o
>  obj-$(CONFIG_MMC_DW_PCI)	+= dw_mmc-pci.o
>  obj-$(CONFIG_MMC_SH_MMCIF)	+= sh_mmcif.o
>  obj-$(CONFIG_MMC_JZ4740)	+= jz4740_mmc.o
> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
> index f013e7e..866edef 100644
> --- a/drivers/mmc/host/dw_mmc-exynos.c
> +++ b/drivers/mmc/host/dw_mmc-exynos.c
> @@ -31,8 +31,6 @@
>  					SDMMC_CLKSEL_CCLK_DRIVE(y) |	\
>  					SDMMC_CLKSEL_CCLK_DIVIDER(z))
>  
> -#define SDMMC_CMD_USE_HOLD_REG		BIT(29)
> -
>  #define EXYNOS4210_FIXED_CIU_CLK_DIV	2
>  #define EXYNOS4412_FIXED_CIU_CLK_DIV	4
>  
> diff --git a/drivers/mmc/host/dw_mmc-socfpga.c b/drivers/mmc/host/dw_mmc-socfpga.c
> new file mode 100644
> index 0000000..7eb3163
> --- /dev/null
> +++ b/drivers/mmc/host/dw_mmc-socfpga.c
> @@ -0,0 +1,141 @@
> +/*
> + * 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/mmc/host.h>
> +#include <linux/mmc/dw_mmc.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#include "dw_mmc.h"
> +#include "dw_mmc-pltfm.h"
> +
> +#define SYSMGR_SDMMCGRP_CTRL_OFFSET		0x108
Is it vendor specific register offset?
> +#define DRV_CLK_PHASE_SHIFT_SEL_MASK	0x7
> +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)		\
> +	((((drvsel) << 0) & 0x7) | (((smplsel) << 3) & 0x38))
Change more readable. ((drvsel & 0x7) << 0 | (smplsel & 0x7) << 3)?
> +
> +extern void __iomem *sys_manager_base_addr;
> +
> +/* SOCFPGA implementation specific driver private data */
> +struct dw_mci_socfpga_priv_data {
> +	u8	ciu_div;
> +	u32	hs_timing;
> +};
> +
> +static int dw_mci_socfpga_priv_init(struct dw_mci *host)
> +{
> +	struct dw_mci_socfpga_priv_data *priv;
> +	struct device *dev = host->dev;
> +	int pwr_en;
> +
> +	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		dev_err(host->dev, "mem alloc failed for private data\n");
> +		return -ENOMEM;
> +	}
> +
> +	host->priv = priv;
> +
> +	if (of_property_read_u32(dev->of_node, "pwr-en", &pwr_en)) {
> +		dev_info(dev, "couldn't determine pwr-en, assuming pwr-en = 0\n");
> +		pwr_en = 0;
> +	}
> +
> +	/* Set PWREN bit */
> +	mci_writel(host, PWREN, pwr_en);
What is this setting? In dw-mmc.c, PWREN register should be set.
> +
> +	return 0;
> +}
> +
> +static int dw_mci_socfpga_setup_clock(struct dw_mci *host)
> +{
> +	struct dw_mci_socfpga_priv_data *priv = host->priv;
> +
> +	clk_disable(host->ciu_clk);
> +	writel(priv->hs_timing, sys_manager_base_addr +
> +		SYSMGR_SDMMCGRP_CTRL_OFFSET);
> +	clk_enable(host->ciu_clk);
Why do you use the clk_disable/enable at this point?
> +
> +	host->bus_hz /= priv->ciu_div;
Didn't consider the "divide by zero"?
> +	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 = host->priv;
> +	struct device_node *np = host->dev->of_node;
> +	u32 timing[2];
> +	u32 div = 0;
> +	int ret;
> +
> +	of_property_read_u32(np, "altr,dw-mshc-ciu-div", &div);
> +	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]);
> +	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);
> +
> +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	= of_match_ptr(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");
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 0b74189..3700cb2 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -111,6 +111,7 @@
>  #define SDMMC_INT_ERROR			0xbfc2
>  /* Command register defines */
>  #define SDMMC_CMD_START			BIT(31)
> +#define SDMMC_CMD_USE_HOLD_REG	BIT(29)
>  #define SDMMC_CMD_CCS_EXP		BIT(23)
>  #define SDMMC_CMD_CEATA_RD		BIT(22)
>  #define SDMMC_CMD_UPD_CLK		BIT(21)
> 

Re: [PATCH 5/5] mmc: dw_mmc: Add support DW SD/MMC driver on SOCFPGA

[Arnd Bergmann]

On Wednesday 15 May 2013, dinguyen@altera.com wrote:
> +
> +#define SYSMGR_SDMMCGRP_CTRL_OFFSET		0x108
> +#define DRV_CLK_PHASE_SHIFT_SEL_MASK	0x7
> +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)		\
> +	((((drvsel) << 0) & 0x7) | (((smplsel) << 3) & 0x38))
> +
> +extern void __iomem *sys_manager_base_addr;

This is not acceptable, you cannot just reference external symbols
from one driver in another, without a proper interface.

Please explain what the functionality is that you need here, then
we can help you find the proper interface. My guess is that you
need either the functionality provided by drivers/reset/
or drivers/mfd/syscon.c.

> +       if (of_property_read_u32(dev->of_node, "pwr-en", &pwr_en)) {
> +               dev_info(dev, "couldn't determine pwr-en, assuming pwr-en = 0\n");
> +               pwr_en = 0;
> +       }
> +
> +       /* Set PWREN bit */
> +       mci_writel(host, PWREN, pwr_en);

If you add new properties, you have to document them in
Documentations/devicetree/bindings/*.

What is the requirement for this property? Is there no way to 
automatically power the card on/off using the normal
dw_mci_set_ios function?

The rest of this patch looks ok to me.

	Arnd

Re: [PATCH 5/5] mmc: dw_mmc: Add support DW SD/MMC driver on SOCFPGA

[Dinh Nguyen]

Hi,

On 05/14/2013 11:28 PM, Jaehoon Chung wrote:
> On 05/15/2013 07:52 AM, dinguyen@altera.com wrote:
>> From: Dinh Nguyen <dinguyen@altera.com>
>>
>> Add platform specific functionality for the DW SD/MMC driver for
>> SoCFPGA. Move SDMMC_CMD_USE_HOLD_REG to dw_mmc.h so other platforms
>> can use this define.
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
>> CC: Seungwon Jeon <tgih.jun@samsung.com>
>> CC: Jaehoon Chung <jh80.chung@samsung.com>
>> CC: Arnd Bergmann <arnd@arndb.de>
>> CC: Olof Johansson <olof@lixom.net>
>> CC: Pavel Machek <pavel@denx.de>
>> CC: linux-mmc@vger.kernel.org
>> ---
>>  drivers/mmc/host/Kconfig          |    8 +++
>>  drivers/mmc/host/Makefile         |    1 +
>>  drivers/mmc/host/dw_mmc-exynos.c  |    2 -
>>  drivers/mmc/host/dw_mmc-socfpga.c |  141 +++++++++++++++++++++++++++++++++++++
>>  drivers/mmc/host/dw_mmc.h         |    1 +
>>  5 files changed, 151 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/mmc/host/dw_mmc-socfpga.c
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 9ab8f8d..1be2289 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -556,6 +556,14 @@ 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
> SOCFPGA? I think good that should be changed more generic.

Not sure what you mean? SOCFPGA is the name of new ARM SOCs from Altera.

>> +	tristate "SOCFPGA specific extensions for Synopsys DW Memory Card Interface"
>> +	depends on MMC_DW
>> +	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/Makefile b/drivers/mmc/host/Makefile
>> index cd32280..67718c1 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -42,6 +42,7 @@ obj-$(CONFIG_SDH_BFIN)		+= bfin_sdh.o
>>  obj-$(CONFIG_MMC_DW)		+= dw_mmc.o
>>  obj-$(CONFIG_MMC_DW_PLTFM)	+= dw_mmc-pltfm.o
>>  obj-$(CONFIG_MMC_DW_EXYNOS)	+= dw_mmc-exynos.o
>> +obj-$(CONFIG_MMC_DW_SOCFPGA)	+= dw_mmc-socfpga.o
>>  obj-$(CONFIG_MMC_DW_PCI)	+= dw_mmc-pci.o
>>  obj-$(CONFIG_MMC_SH_MMCIF)	+= sh_mmcif.o
>>  obj-$(CONFIG_MMC_JZ4740)	+= jz4740_mmc.o
>> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
>> index f013e7e..866edef 100644
>> --- a/drivers/mmc/host/dw_mmc-exynos.c
>> +++ b/drivers/mmc/host/dw_mmc-exynos.c
>> @@ -31,8 +31,6 @@
>>  					SDMMC_CLKSEL_CCLK_DRIVE(y) |	\
>>  					SDMMC_CLKSEL_CCLK_DIVIDER(z))
>>  
>> -#define SDMMC_CMD_USE_HOLD_REG		BIT(29)
>> -
>>  #define EXYNOS4210_FIXED_CIU_CLK_DIV	2
>>  #define EXYNOS4412_FIXED_CIU_CLK_DIV	4
>>  
>> diff --git a/drivers/mmc/host/dw_mmc-socfpga.c b/drivers/mmc/host/dw_mmc-socfpga.c
>> new file mode 100644
>> index 0000000..7eb3163
>> --- /dev/null
>> +++ b/drivers/mmc/host/dw_mmc-socfpga.c
>> @@ -0,0 +1,141 @@
>> +/*
>> + * 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/mmc/host.h>
>> +#include <linux/mmc/dw_mmc.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include "dw_mmc.h"
>> +#include "dw_mmc-pltfm.h"
>> +
>> +#define SYSMGR_SDMMCGRP_CTRL_OFFSET		0x108
> Is it vendor specific register offset?
>> +#define DRV_CLK_PHASE_SHIFT_SEL_MASK	0x7
>> +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)		\
>> +	((((drvsel) << 0) & 0x7) | (((smplsel) << 3) & 0x38))
> Change more readable. ((drvsel & 0x7) << 0 | (smplsel & 0x7) << 3)?
>> +
>> +extern void __iomem *sys_manager_base_addr;
>> +
>> +/* SOCFPGA implementation specific driver private data */
>> +struct dw_mci_socfpga_priv_data {
>> +	u8	ciu_div;
>> +	u32	hs_timing;
>> +};
>> +
>> +static int dw_mci_socfpga_priv_init(struct dw_mci *host)
>> +{
>> +	struct dw_mci_socfpga_priv_data *priv;
>> +	struct device *dev = host->dev;
>> +	int pwr_en;
>> +
>> +	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv) {
>> +		dev_err(host->dev, "mem alloc failed for private data\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	host->priv = priv;
>> +
>> +	if (of_property_read_u32(dev->of_node, "pwr-en", &pwr_en)) {
>> +		dev_info(dev, "couldn't determine pwr-en, assuming pwr-en = 0\n");
>> +		pwr_en = 0;
>> +	}
>> +
>> +	/* Set PWREN bit */
>> +	mci_writel(host, PWREN, pwr_en);
> What is this setting? In dw-mmc.c, PWREN register should be set.

Ah..it was there before 3.9...I will adjust accordingly.

>> +
>> +	return 0;
>> +}
>> +
>> +static int dw_mci_socfpga_setup_clock(struct dw_mci *host)
>> +{
>> +	struct dw_mci_socfpga_priv_data *priv = host->priv;
>> +
>> +	clk_disable(host->ciu_clk);
>> +	writel(priv->hs_timing, sys_manager_base_addr +
>> +		SYSMGR_SDMMCGRP_CTRL_OFFSET);
>> +	clk_enable(host->ciu_clk);
> Why do you use the clk_disable/enable at this point?

Our implementation of the controller requires us to disable the clock
when we adjust the timing parameter.

>> +
>> +	host->bus_hz /= priv->ciu_div;
> Didn't consider the "divide by zero"?

Will fix..

Thanks alot for the review.

Dinh
>> +	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 = host->priv;
>> +	struct device_node *np = host->dev->of_node;
>> +	u32 timing[2];
>> +	u32 div = 0;
>> +	int ret;
>> +
>> +	of_property_read_u32(np, "altr,dw-mshc-ciu-div", &div);
>> +	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]);
>> +	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);
>> +
>> +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	= of_match_ptr(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");
>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>> index 0b74189..3700cb2 100644
>> --- a/drivers/mmc/host/dw_mmc.h
>> +++ b/drivers/mmc/host/dw_mmc.h
>> @@ -111,6 +111,7 @@
>>  #define SDMMC_INT_ERROR			0xbfc2
>>  /* Command register defines */
>>  #define SDMMC_CMD_START			BIT(31)
>> +#define SDMMC_CMD_USE_HOLD_REG	BIT(29)
>>  #define SDMMC_CMD_CCS_EXP		BIT(23)
>>  #define SDMMC_CMD_CEATA_RD		BIT(22)
>>  #define SDMMC_CMD_UPD_CLK		BIT(21)
>>
> 

Re: [PATCH 5/5] mmc: dw_mmc: Add support DW SD/MMC driver on SOCFPGA

[Dinh Nguyen]

Hi Arnd,

Thanks for the review.

On 05/15/2013 08:25 AM, Arnd Bergmann wrote:
> On Wednesday 15 May 2013, dinguyen@altera.com wrote:
>> +
>> +#define SYSMGR_SDMMCGRP_CTRL_OFFSET		0x108
>> +#define DRV_CLK_PHASE_SHIFT_SEL_MASK	0x7
>> +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)		\
>> +	((((drvsel) << 0) & 0x7) | (((smplsel) << 3) & 0x38))
>> +
>> +extern void __iomem *sys_manager_base_addr;
> 
> This is not acceptable, you cannot just reference external symbols
> from one driver in another, without a proper interface.
> 
> Please explain what the functionality is that you need here, then
> we can help you find the proper interface. My guess is that you
> need either the functionality provided by drivers/reset/
> or drivers/mfd/syscon.c.

Our implementation has the timing controls for the SD/MMC controller in
another custom IP block(system manager). sys_manager_base_addr was
mapped in mach-socfpga/socfpga.c. I saw the same approach with
drivers/clk(clk_mgr_base_addr), so I thought it would be ok with this
driver. Please advise on another way to do this...

> 
>> +       if (of_property_read_u32(dev->of_node, "pwr-en", &pwr_en)) {
>> +               dev_info(dev, "couldn't determine pwr-en, assuming pwr-en = 0\n");
>> +               pwr_en = 0;
>> +       }
>> +
>> +       /* Set PWREN bit */
>> +       mci_writel(host, PWREN, pwr_en);
> 
> If you add new properties, you have to document them in
> Documentations/devicetree/bindings/*.
> 
> What is the requirement for this property? Is there no way to 
> automatically power the card on/off using the normal
> dw_mci_set_ios function?

Apologies...it wasn't there in 3.8. I will fix...

Thanks,
Dinh
> 
> The rest of this patch looks ok to me.
> 
> 	Arnd
> 

Re: [PATCH 5/5] mmc: dw_mmc: Add support DW SD/MMC driver on SOCFPGA

[Arnd Bergmann]

On Wednesday 15 May 2013 11:40:12 Dinh Nguyen wrote:
> On 05/15/2013 08:25 AM, Arnd Bergmann wrote:
> > On Wednesday 15 May 2013, dinguyen@altera.com wrote:
> >> +
> >> +#define SYSMGR_SDMMCGRP_CTRL_OFFSET         0x108
> >> +#define DRV_CLK_PHASE_SHIFT_SEL_MASK        0x7
> >> +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)              \
> >> +    ((((drvsel) << 0) & 0x7) | (((smplsel) << 3) & 0x38))
> >> +
> >> +extern void __iomem *sys_manager_base_addr;
> > 
> > This is not acceptable, you cannot just reference external symbols
> > from one driver in another, without a proper interface.
> > 
> > Please explain what the functionality is that you need here, then
> > we can help you find the proper interface. My guess is that you
> > need either the functionality provided by drivers/reset/
> > or drivers/mfd/syscon.c.
> 
> Our implementation has the timing controls for the SD/MMC controller in
> another custom IP block(system manager). sys_manager_base_addr was
> mapped in mach-socfpga/socfpga.c. I saw the same approach with
> drivers/clk(clk_mgr_base_addr), so I thought it would be ok with this
> driver. Please advise on another way to do this...

The clock code is tied more more closely to the platform code, so I
was turning a blind eye on that one, under the assumption that it
was only used there.

	Arnd

Re: [PATCH 5/5] mmc: dw_mmc: Add support DW SD/MMC driver on SOCFPGA

[Dinh Nguyen]

Hi Arnd,

On 05/15/2013 12:11 PM, Arnd Bergmann wrote:
> On Wednesday 15 May 2013 11:40:12 Dinh Nguyen wrote:
>> On 05/15/2013 08:25 AM, Arnd Bergmann wrote:
>>> On Wednesday 15 May 2013, dinguyen@altera.com wrote:
>>>> +
>>>> +#define SYSMGR_SDMMCGRP_CTRL_OFFSET         0x108
>>>> +#define DRV_CLK_PHASE_SHIFT_SEL_MASK        0x7
>>>> +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)              \
>>>> +    ((((drvsel) << 0) & 0x7) | (((smplsel) << 3) & 0x38))
>>>> +
>>>> +extern void __iomem *sys_manager_base_addr;
>>>
>>> This is not acceptable, you cannot just reference external symbols
>>> from one driver in another, without a proper interface.
>>>
>>> Please explain what the functionality is that you need here, then
>>> we can help you find the proper interface. My guess is that you
>>> need either the functionality provided by drivers/reset/
>>> or drivers/mfd/syscon.c.
>>
>> Our implementation has the timing controls for the SD/MMC controller in
>> another custom IP block(system manager). sys_manager_base_addr was
>> mapped in mach-socfpga/socfpga.c. I saw the same approach with
>> drivers/clk(clk_mgr_base_addr), so I thought it would be ok with this
>> driver. Please advise on another way to do this...
> 
> The clock code is tied more more closely to the platform code, so I
> was turning a blind eye on that one, under the assumption that it
> was only used there.

I think I can use the syscon interface for this. Thanks for the pointer.
I kinda thought that these dw_mmc-<platform> is platform specific
enough, but I will go the syscon route.

Dinh
> 
> 	Arnd
>