[v2 PATCH 0/2] mmc: dw_mmc: add dw_mmc-k3

[v2 PATCH 0/2] mmc: dw_mmc: add dw_mmc-k3

[Zhangfei Gao]

v2:
Follow Jaehoon's suggestion
Use slot-gpio.c handle cd pin
Move table out to dts
other suggestion

Zhangfei Gao (2):
  mmc: dw_mmc: add dw_mci_of_get_cd_gpio to handle cd pin
  mmc: dw_mmc: add dw_mmc-k3 for k3 platform

 .../devicetree/bindings/mmc/k3-dw-mshc.txt         |   77 +++++
 drivers/mmc/host/Kconfig                           |   10 +
 drivers/mmc/host/Makefile                          |    1 +
 drivers/mmc/host/dw_mmc-k3.c                       |  298 ++++++++++++++++++++
 drivers/mmc/host/dw_mmc.c                          |   34 +++
 5 files changed, 420 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
 create mode 100644 drivers/mmc/host/dw_mmc-k3.c

-- 
1.7.9.5

[PATCH 1/2] mmc: dw_mmc: add dw_mci_of_get_cd_gpio to handle cd pin

[Zhangfei Gao]

Add function dw_mci_of_get_cd_gpio to check "cd-gpios" from dts.
mmc_gpio_request_cd and mmc_gpio_get_cd are used to handle cd pin

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
CC: Jaehoon Chung <jh80.chung@samsung.com>
---
 drivers/mmc/host/dw_mmc.c |   34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 018f365..b8bb9a5 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -35,6 +35,7 @@
 #include <linux/workqueue.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
+#include <linux/mmc/slot-gpio.h>
 
 #include "dw_mmc.h"
 
@@ -872,12 +873,15 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	int present;
 	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci_board *brd = slot->host->pdata;
+	int gpio_cd = !mmc_gpio_get_cd(mmc);
 
 	/* Use platform get_cd function, else try onboard card detect */
 	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)
 		present = 1;
 	else if (brd->get_cd)
 		present = !brd->get_cd(slot->id);
+	else if (!IS_ERR_VALUE(gpio_cd))
+		present = !!gpio_cd;
 	else
 		present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
 			== 0 ? 1 : 0;
@@ -1876,6 +1880,30 @@ static int dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
 
 	return gpio;
 }
+
+/* find the cd gpio for a given slot; or -1 if none specified */
+static int dw_mci_of_get_cd_gpio(struct device *dev, u8 slot,
+					struct mmc_host *mmc)
+{
+	struct device_node *np = dw_mci_of_find_slot_node(dev, slot);
+	int gpio;
+
+	if (!np)
+		return -EINVAL;
+
+	gpio = of_get_named_gpio(np, "cd-gpios", 0);
+
+	/* Having a missing entry is valid; return silently */
+	if (!gpio_is_valid(gpio))
+		return -EINVAL;
+
+	if (mmc_gpio_request_cd(mmc, gpio, 0)) {
+		dev_warn(dev, "gpio [%d] request failed\n", gpio);
+		return -EINVAL;
+	}
+
+	return gpio;
+}
 #else /* CONFIG_OF */
 static int dw_mci_of_get_slot_quirks(struct device *dev, u8 slot)
 {
@@ -1893,6 +1921,11 @@ static int dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
 {
 	return -EINVAL;
 }
+static int dw_mci_of_get_cd_gpio(struct device *dev, u8 slot,
+					struct mmc_host *mmc)
+{
+	return -EINVAL;
+}
 #endif /* CONFIG_OF */
 
 static int dw_mci_init_slot(struct dw_mci *host, unsigned int id)
@@ -1996,6 +2029,7 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id)
 		clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
 
 	slot->wp_gpio = dw_mci_of_get_wp_gpio(host->dev, slot->id);
+	dw_mci_of_get_cd_gpio(host->dev, slot->id, mmc);
 
 	ret = mmc_add_host(mmc);
 	if (ret)
-- 
1.7.9.5

[PATCH 2/2] mmc: dw_mmc: add dw_mmc-k3 for k3 platform

[Zhangfei Gao]

Add dw_mmc-k3.c for k3v2, support sd/emmc

Signed-off-by: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Tested-by: Zhigang Wang <brooke.wangzhigang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/mmc/k3-dw-mshc.txt         |   77 +++++
 drivers/mmc/host/Kconfig                           |   10 +
 drivers/mmc/host/Makefile                          |    1 +
 drivers/mmc/host/dw_mmc-k3.c                       |  298 ++++++++++++++++++++
 4 files changed, 386 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
 create mode 100644 drivers/mmc/host/dw_mmc-k3.c

diff --git a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
new file mode 100644
index 0000000..0de8c27
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
@@ -0,0 +1,77 @@
+* Hisilicon specific extensions to the Synopsys Designware Mobile
+  Storage Host Controller
+
+Read synopsis-dw-mshc.txt for more details
+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 synopsis-dw-mshc.txt and the properties used by the Hisilicon specific
+extensions to the Synopsys Designware Mobile Storage Host Controller.
+
+Required Properties:
+
+* compatible: should be
+	- "hisilicon,hi4511-dw-mshc": for controllers with hi4511
+	  specific extentions.
+* vmmc-supply: should be vmmc used in dwmmc
+* fifo-depth: should be provided if register can not provide correct value
+* clken-reg: should be clock enable register and offset
+* drv-sel-reg: should be driver delay select register, offset and bits
+* sam-sel-reg: should be sample delay select register, offset and bits
+* div-reg: should be divider register, offset and bits
+* tune-table: should be array of clock tune mmc controller
+
+Example:
+
+  The MSHC controller node can be split into two portions, SoC specific and
+  board specific portions as listed below.
+
+	dwmmc_0: dwmmc0@fcd03000 {
+		compatible = "hisilicon,hi4511-dw-mshc";
+		reg = <0xfcd03000 0x1000>;
+		interrupts = <0 16 4>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&clk_sd>, <&clk_ddrc_per>;
+		clock-names = "ciu", "biu";
+		clken-reg = <0x1f8 0>;
+		drv-sel-reg = <0x1f8 4 4>;
+		sam-sel-reg = <0x1f8 8 4>;
+		div-reg = <0x1f8 1 3>;
+		tune-table =
+		<180000000 6 6 13 13 25000000>,
+		<0 0 0 0 0 0>,
+		<360000000 6 4 2 0 50000000>,
+		<180000000 6 4 13 13 25000000>,
+		<360000000 6 4 2 0 50000000>,
+		<720000000 6 1 9 4 100000000>,
+		<0 0 0 0 0 0>,
+		<360000000 7 1 3 0 50000000>;
+	};
+	dwmmc0@fcd03000 {
+		num-slots = <1>;
+		vmmc-supply = <&ldo12>;
+		fifo-depth = <0x100>;
+		supports-highspeed;
+		pinctrl-names = "default";
+		pinctrl-0 = <&sd_pmx_pins &sd_cfg_func1 &sd_cfg_func2>;
+		slot@0 {
+			reg = <0>;
+			bus-width = <4>;
+			disable-wp;
+			cd-gpios = <&gpio10 3 0>;
+		};
+	};
+
+PCTRL:
+
+Required Properties:
+* compatible: should be
+	- "hisilicon,pctrl": Peripheral control
+
+Example:
+
+	pctrl: pctrl@fca09000 {
+		compatible = "hisilicon,pctrl";
+		reg = <0xfca09000 0x1000>;
+	};
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 7fc5099..45aaa2d 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -575,6 +575,16 @@ config MMC_DW_SOCFPGA
 	  This selects support for Altera SoCFPGA specific extensions to the
 	  Synopsys DesignWare Memory Card Interface driver.
 
+config MMC_DW_K3
+	tristate "K3 specific extensions for Synopsys DW Memory Card Interface"
+	depends on MMC_DW
+	select MMC_DW_PLTFM
+	select MMC_DW_IDMAC
+	help
+	  This selects support for Hisilicon K3 SoC specific extensions to the
+	  Synopsys DesignWare Memory Card Interface driver. Select this option
+	  for platforms based on Hisilicon K3 SoC's.
+
 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 c41d0c3..64f5f8d 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -43,6 +43,7 @@ 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_K3)		+= dw_mmc-k3.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-k3.c b/drivers/mmc/host/dw_mmc-k3.c
new file mode 100644
index 0000000..f186b84
--- /dev/null
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -0,0 +1,298 @@
+/*
+ * Copyright (c) 2013 Linaro Ltd.
+ * Copyright (c) 2013 Hisilicon Limited.
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/mmc/host.h>
+#include <linux/mmc/dw_mmc.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_address.h>
+
+#include "dw_mmc.h"
+#include "dw_mmc-pltfm.h"
+
+enum dw_mci_k3_type {
+	DW_MCI_TYPE_HI4511,
+};
+
+static struct dw_mci_k3_compatible {
+	char				*compatible;
+	enum dw_mci_k3_type		type;
+} k3_compat[] = {
+	{
+		.compatible	= "hisilicon,hi4511-dw-mshc",
+		.type		= DW_MCI_TYPE_HI4511,
+	},
+};
+
+#define TABLE_WIDTH 6
+#define TABLE_HEIGHT 8
+struct dw_mci_k3_priv_data {
+	enum dw_mci_k3_type	type;
+	int			old_timing;
+	u32			table[TABLE_WIDTH * TABLE_HEIGHT];
+	u32			clken_reg;
+	u32			clken_bit;
+	u32			sam_sel_reg;
+	u32			sam_sel_off;
+	u32			sam_sel_bits;
+	u32			drv_sel_reg;
+	u32			drv_sel_off;
+	u32			drv_sel_bits;
+	u32			div_reg;
+	u32			div_off;
+	u32			div_bits;
+};
+
+static void __iomem *pctrl;
+static DEFINE_SPINLOCK(mmc_tuning_lock);
+
+static u32 dw_mci_k3_delay(u32 val, u32 para, u32 off, u32 len)
+{
+	u32 i;
+
+	if (para >= 0) {
+		for (i = 0; i < len; i++) {
+			if (para % 2)
+				val |= 1<<(off + i);
+			else
+				val &= ~(1<<(off + i));
+			para = para >> 1;
+		}
+	}
+	return val;
+}
+
+static void dw_mci_k3_set_timing(struct dw_mci_k3_priv_data *priv,
+					u32 sam, u32 drv, u32 div)
+{
+	u32 val;
+	unsigned long flags;
+
+	if (!pctrl)
+		return;
+
+	spin_lock_irqsave(&mmc_tuning_lock, flags);
+
+	/* disable clock */
+	val = readl(pctrl + priv->clken_reg);
+	val &= ~(1 << priv->clken_bit);
+	writel(val, pctrl + priv->clken_reg);
+
+	val = readl(pctrl + priv->sam_sel_reg);
+	val = dw_mci_k3_delay(val, sam, priv->sam_sel_off, priv->sam_sel_bits);
+	writel(val, pctrl + priv->sam_sel_reg);
+
+	val = readl(pctrl + priv->drv_sel_reg);
+	val = dw_mci_k3_delay(val, drv, priv->drv_sel_off, priv->drv_sel_bits);
+	writel(val, pctrl + priv->drv_sel_reg);
+
+	val = readl(pctrl + priv->div_reg);
+	val = dw_mci_k3_delay(val, drv, priv->div_off, priv->div_bits);
+	writel(val, pctrl + priv->div_reg);
+
+	/* enable clock */
+	val = readl(pctrl + priv->clken_reg);
+	val |= 1 << priv->clken_bit;
+	writel(val, pctrl + priv->clken_reg);
+
+	spin_unlock_irqrestore(&mmc_tuning_lock, flags);
+}
+
+static void dw_mci_k3_tun(struct dw_mci *host, int timing)
+{
+	struct dw_mci_k3_priv_data *priv = host->priv;
+	struct device_node *np = host->dev->of_node;
+	int ret;
+	u32 *table = &priv->table[TABLE_WIDTH * timing];
+
+	if (priv->old_timing == timing)
+		return;
+
+	ret = clk_set_rate(host->ciu_clk, table[0]);
+	if (ret) {
+		dev_err(host->dev, "clk_set_rate failed\n");
+		return;
+	}
+	dw_mci_k3_set_timing(priv, (table[3] + table[4]) / 2,
+			table[2], table[1]);
+	host->bus_hz = table[5];
+	priv->old_timing = timing;
+}
+
+static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios)
+{
+	struct dw_mci_k3_priv_data *priv = host->priv;
+
+	if (priv->type == DW_MCI_TYPE_HI4511)
+		dw_mci_k3_tun(host, ios->timing);
+}
+
+static int dw_mci_k3_priv_init(struct dw_mci *host)
+{
+	struct dw_mci_k3_priv_data *priv;
+	int i, 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->old_timing = -1;
+	host->priv = priv;
+
+	for (i = 0; i < ARRAY_SIZE(k3_compat); i++) {
+		if (of_device_is_compatible(host->dev->of_node,
+					k3_compat[i].compatible))
+			priv->type = k3_compat[i].type;
+	}
+
+	if (priv->type == DW_MCI_TYPE_HI4511) {
+		ret = of_property_read_u32_array(host->dev->of_node,
+			"tune-table", priv->table, TABLE_WIDTH * TABLE_HEIGHT);
+		if (ret)
+			return -EINVAL;
+
+		if (!pctrl) {
+			struct device_node *node;
+
+			node = of_find_compatible_node(NULL, NULL,
+						"hisilicon,pctrl");
+			pctrl = of_iomap(node, 0);
+		}
+	}
+
+	return 0;
+}
+
+static int dw_mci_k3_setup_clock(struct dw_mci *host)
+{
+	struct dw_mci_k3_priv_data *priv = host->priv;
+
+	if (priv->type == DW_MCI_TYPE_HI4511)
+		dw_mci_k3_tun(host, MMC_TIMING_LEGACY);
+
+	return 0;
+}
+
+static int dw_mci_k3_parse_dt(struct dw_mci *host)
+{
+	struct dw_mci_k3_priv_data *priv = host->priv;
+	struct device_node *np = host->dev->of_node;
+	u32 data[3];
+	int ret;
+
+	if (priv->type == DW_MCI_TYPE_HI4511) {
+		ret = of_property_read_u32_array(np,
+				"clken-reg", data, 2);
+		if (!ret) {
+			priv->clken_reg = data[0];
+			priv->clken_bit = data[1];
+		}
+
+		ret = of_property_read_u32_array(np,
+				"drv-sel-reg", data, 3);
+		if (!ret) {
+			priv->drv_sel_reg = data[0];
+			priv->drv_sel_off = data[1];
+			priv->drv_sel_bits = data[2];
+		}
+
+		ret = of_property_read_u32_array(np,
+				"sam-sel-reg", data, 3);
+		if (!ret) {
+			priv->sam_sel_reg = data[0];
+			priv->sam_sel_off = data[1];
+			priv->sam_sel_bits = data[2];
+		}
+
+		ret = of_property_read_u32_array(np,
+				"div-reg", data, 3);
+		if (!ret) {
+			priv->div_reg = data[0];
+			priv->div_off = data[1];
+			priv->div_bits = data[2];
+		}
+	}
+
+	return 0;
+}
+
+static unsigned long k3_dwmmc_caps[4] = {
+	MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED,
+	MMC_CAP_8_BIT_DATA | MMC_CAP_MMC_HIGHSPEED,
+	0,
+	0,
+};
+
+static const struct dw_mci_drv_data k3_drv_data = {
+	.caps			= k3_dwmmc_caps,
+	.init			= dw_mci_k3_priv_init,
+	.set_ios		= dw_mci_k3_set_ios,
+	.setup_clock		= dw_mci_k3_setup_clock,
+	.parse_dt		= dw_mci_k3_parse_dt,
+};
+
+static const struct of_device_id dw_mci_k3_match[] = {
+	{ .compatible = "hisilicon,hi4511-dw-mshc",
+			.data = &k3_drv_data, },
+	{},
+};
+MODULE_DEVICE_TABLE(of, dw_mci_k3_match);
+
+static int dw_mci_k3_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_k3_match, pdev->dev.of_node);
+	drv_data = match->data;
+
+	return dw_mci_pltfm_register(pdev, drv_data);
+}
+
+static int dw_mci_k3_suspend(struct device *dev)
+{
+	struct dw_mci *host = dev_get_drvdata(dev);
+
+	return dw_mci_suspend(host);
+}
+
+static int dw_mci_k3_resume(struct device *dev)
+{
+	struct dw_mci *host = dev_get_drvdata(dev);
+	struct dw_mci_k3_priv_data *priv = host->priv;
+
+	if (priv->type == DW_MCI_TYPE_HI4511) {
+		priv->old_timing = -1;
+		dw_mci_k3_tun(host, MMC_TIMING_LEGACY);
+	}
+
+	return dw_mci_resume(host);
+}
+
+SIMPLE_DEV_PM_OPS(dw_mci_k3_pmops, dw_mci_k3_suspend, dw_mci_k3_resume);
+
+static struct platform_driver dw_mci_k3_pltfm_driver = {
+	.probe		= dw_mci_k3_probe,
+	.remove		= dw_mci_pltfm_remove,
+	.driver		= {
+		.name		= "dwmmc_k3",
+		.of_match_table	= dw_mci_k3_match,
+		.pm		= &dw_mci_k3_pmops,
+	},
+};
+
+module_platform_driver(dw_mci_k3_pltfm_driver);
+
+MODULE_DESCRIPTION("K3 Specific DW-MSHC Driver Extension");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] mmc: dw_mmc: add dw_mmc-k3 for k3 platform

[Zhangfei Gao]

update: fix typo

Add dw_mmc-k3.c for k3v2, support sd/emmc

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Tested-by: Zhigang Wang <brooke.wangzhigang@huawei.com>
---
 .../devicetree/bindings/mmc/k3-dw-mshc.txt         |   77 +++++
 drivers/mmc/host/Kconfig                           |   10 +
 drivers/mmc/host/Makefile                          |    1 +
 drivers/mmc/host/dw_mmc-k3.c                       |  297 ++++++++++++++++++++
 4 files changed, 385 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
 create mode 100644 drivers/mmc/host/dw_mmc-k3.c

diff --git a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
new file mode 100644
index 0000000..0de8c27
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
@@ -0,0 +1,77 @@
+* Hisilicon specific extensions to the Synopsys Designware Mobile
+  Storage Host Controller
+
+Read synopsis-dw-mshc.txt for more details
+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 synopsis-dw-mshc.txt and the properties used by the Hisilicon specific
+extensions to the Synopsys Designware Mobile Storage Host Controller.
+
+Required Properties:
+
+* compatible: should be
+	- "hisilicon,hi4511-dw-mshc": for controllers with hi4511
+	  specific extentions.
+* vmmc-supply: should be vmmc used in dwmmc
+* fifo-depth: should be provided if register can not provide correct value
+* clken-reg: should be clock enable register and offset
+* drv-sel-reg: should be driver delay select register, offset and bits
+* sam-sel-reg: should be sample delay select register, offset and bits
+* div-reg: should be divider register, offset and bits
+* tune-table: should be array of clock tune mmc controller
+
+Example:
+
+  The MSHC controller node can be split into two portions, SoC specific and
+  board specific portions as listed below.
+
+	dwmmc_0: dwmmc0@fcd03000 {
+		compatible = "hisilicon,hi4511-dw-mshc";
+		reg = <0xfcd03000 0x1000>;
+		interrupts = <0 16 4>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&clk_sd>, <&clk_ddrc_per>;
+		clock-names = "ciu", "biu";
+		clken-reg = <0x1f8 0>;
+		drv-sel-reg = <0x1f8 4 4>;
+		sam-sel-reg = <0x1f8 8 4>;
+		div-reg = <0x1f8 1 3>;
+		tune-table =
+		<180000000 6 6 13 13 25000000>,
+		<0 0 0 0 0 0>,
+		<360000000 6 4 2 0 50000000>,
+		<180000000 6 4 13 13 25000000>,
+		<360000000 6 4 2 0 50000000>,
+		<720000000 6 1 9 4 100000000>,
+		<0 0 0 0 0 0>,
+		<360000000 7 1 3 0 50000000>;
+	};
+	dwmmc0@fcd03000 {
+		num-slots = <1>;
+		vmmc-supply = <&ldo12>;
+		fifo-depth = <0x100>;
+		supports-highspeed;
+		pinctrl-names = "default";
+		pinctrl-0 = <&sd_pmx_pins &sd_cfg_func1 &sd_cfg_func2>;
+		slot@0 {
+			reg = <0>;
+			bus-width = <4>;
+			disable-wp;
+			cd-gpios = <&gpio10 3 0>;
+		};
+	};
+
+PCTRL:
+
+Required Properties:
+* compatible: should be
+	- "hisilicon,pctrl": Peripheral control
+
+Example:
+
+	pctrl: pctrl@fca09000 {
+		compatible = "hisilicon,pctrl";
+		reg = <0xfca09000 0x1000>;
+	};
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 7fc5099..45aaa2d 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -575,6 +575,16 @@ config MMC_DW_SOCFPGA
 	  This selects support for Altera SoCFPGA specific extensions to the
 	  Synopsys DesignWare Memory Card Interface driver.
 
+config MMC_DW_K3
+	tristate "K3 specific extensions for Synopsys DW Memory Card Interface"
+	depends on MMC_DW
+	select MMC_DW_PLTFM
+	select MMC_DW_IDMAC
+	help
+	  This selects support for Hisilicon K3 SoC specific extensions to the
+	  Synopsys DesignWare Memory Card Interface driver. Select this option
+	  for platforms based on Hisilicon K3 SoC's.
+
 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 c41d0c3..64f5f8d 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -43,6 +43,7 @@ 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_K3)		+= dw_mmc-k3.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-k3.c b/drivers/mmc/host/dw_mmc-k3.c
new file mode 100644
index 0000000..0ff75328
--- /dev/null
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -0,0 +1,297 @@
+/*
+ * Copyright (c) 2013 Linaro Ltd.
+ * Copyright (c) 2013 Hisilicon Limited.
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/mmc/host.h>
+#include <linux/mmc/dw_mmc.h>
+#include <linux/of_address.h>
+
+#include "dw_mmc.h"
+#include "dw_mmc-pltfm.h"
+
+enum dw_mci_k3_type {
+	DW_MCI_TYPE_HI4511,
+};
+
+static struct dw_mci_k3_compatible {
+	char				*compatible;
+	enum dw_mci_k3_type		type;
+} k3_compat[] = {
+	{
+		.compatible	= "hisilicon,hi4511-dw-mshc",
+		.type		= DW_MCI_TYPE_HI4511,
+	},
+};
+
+#define TABLE_WIDTH 6
+#define TABLE_HEIGHT 8
+struct dw_mci_k3_priv_data {
+	enum dw_mci_k3_type	type;
+	int			old_timing;
+	u32			table[TABLE_WIDTH * TABLE_HEIGHT];
+	u32			clken_reg;
+	u32			clken_bit;
+	u32			sam_reg;
+	u32			sam_off;
+	u32			sam_bits;
+	u32			drv_reg;
+	u32			drv_off;
+	u32			drv_bits;
+	u32			div_reg;
+	u32			div_off;
+	u32			div_bits;
+};
+
+static void __iomem *pctrl;
+static DEFINE_SPINLOCK(mmc_tuning_lock);
+
+static u32 dw_mci_k3_delay(u32 val, u32 para, u32 off, u32 len)
+{
+	u32 i;
+
+	if (para >= 0) {
+		for (i = 0; i < len; i++) {
+			if (para % 2)
+				val |= 1<<(off + i);
+			else
+				val &= ~(1<<(off + i));
+			para = para >> 1;
+		}
+	}
+	return val;
+}
+
+static void dw_mci_k3_set_timing(struct dw_mci_k3_priv_data *priv,
+					u32 sam, u32 drv, u32 div)
+{
+	u32 val;
+	unsigned long flags;
+
+	if (!pctrl || !priv->clken_reg || !priv->sam_reg
+		|| !priv->drv_reg || !priv->div_reg)
+		return;
+
+	spin_lock_irqsave(&mmc_tuning_lock, flags);
+
+	val = readl(pctrl + priv->clken_reg);
+	val &= ~(1 << priv->clken_bit);
+	writel(val, pctrl + priv->clken_reg);
+
+	val = readl(pctrl + priv->sam_reg);
+	val = dw_mci_k3_delay(val, sam, priv->sam_off, priv->sam_bits);
+	writel(val, pctrl + priv->sam_reg);
+
+	val = readl(pctrl + priv->drv_reg);
+	val = dw_mci_k3_delay(val, drv, priv->drv_off, priv->drv_bits);
+	writel(val, pctrl + priv->drv_reg);
+
+	val = readl(pctrl + priv->div_reg);
+	val = dw_mci_k3_delay(val, div, priv->div_off, priv->div_bits);
+	writel(val, pctrl + priv->div_reg);
+
+	val = readl(pctrl + priv->clken_reg);
+	val |= 1 << priv->clken_bit;
+	writel(val, pctrl + priv->clken_reg);
+
+	spin_unlock_irqrestore(&mmc_tuning_lock, flags);
+}
+
+static void dw_mci_k3_tun(struct dw_mci *host, int timing)
+{
+	struct dw_mci_k3_priv_data *priv = host->priv;
+	int ret;
+	u32 *table = &priv->table[TABLE_WIDTH * timing];
+
+	if (priv->old_timing == timing)
+		return;
+
+	ret = clk_set_rate(host->ciu_clk, table[0]);
+	if (ret) {
+		dev_err(host->dev, "clk_set_rate failed\n");
+		return;
+	}
+	dw_mci_k3_set_timing(priv, (table[3] + table[4]) / 2,
+			table[2], table[1]);
+	host->bus_hz = table[5];
+	priv->old_timing = timing;
+}
+
+static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios)
+{
+	struct dw_mci_k3_priv_data *priv = host->priv;
+
+	if (priv->type == DW_MCI_TYPE_HI4511)
+		dw_mci_k3_tun(host, ios->timing);
+}
+
+static int dw_mci_k3_priv_init(struct dw_mci *host)
+{
+	struct dw_mci_k3_priv_data *priv;
+	int i, 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->old_timing = -1;
+	host->priv = priv;
+
+	for (i = 0; i < ARRAY_SIZE(k3_compat); i++) {
+		if (of_device_is_compatible(host->dev->of_node,
+					k3_compat[i].compatible))
+			priv->type = k3_compat[i].type;
+	}
+
+	if (priv->type == DW_MCI_TYPE_HI4511) {
+		ret = of_property_read_u32_array(host->dev->of_node,
+			"tune-table", priv->table, TABLE_WIDTH * TABLE_HEIGHT);
+		if (ret) {
+			dev_err(host->dev, "not found tune-table\n");
+			return -EINVAL;
+		}
+
+		if (!pctrl) {
+			struct device_node *node;
+
+			node = of_find_compatible_node(NULL, NULL,
+						"hisilicon,pctrl");
+			pctrl = of_iomap(node, 0);
+		}
+	}
+
+	return 0;
+}
+
+static int dw_mci_k3_setup_clock(struct dw_mci *host)
+{
+	struct dw_mci_k3_priv_data *priv = host->priv;
+
+	if (priv->type == DW_MCI_TYPE_HI4511)
+		dw_mci_k3_tun(host, MMC_TIMING_LEGACY);
+
+	return 0;
+}
+
+static int dw_mci_k3_parse_dt(struct dw_mci *host)
+{
+	struct dw_mci_k3_priv_data *priv = host->priv;
+	struct device_node *np = host->dev->of_node;
+	u32 data[3];
+	int ret;
+
+	if (priv->type == DW_MCI_TYPE_HI4511) {
+		ret = of_property_read_u32_array(np,
+				"clken-reg", data, 2);
+		if (!ret) {
+			priv->clken_reg = data[0];
+			priv->clken_bit = data[1];
+		}
+
+		ret = of_property_read_u32_array(np,
+				"drv-sel-reg", data, 3);
+		if (!ret) {
+			priv->drv_reg = data[0];
+			priv->drv_off = data[1];
+			priv->drv_bits = data[2];
+		}
+
+		ret = of_property_read_u32_array(np,
+				"sam-sel-reg", data, 3);
+		if (!ret) {
+			priv->sam_reg = data[0];
+			priv->sam_off = data[1];
+			priv->sam_bits = data[2];
+		}
+
+		ret = of_property_read_u32_array(np,
+				"div-reg", data, 3);
+		if (!ret) {
+			priv->div_reg = data[0];
+			priv->div_off = data[1];
+			priv->div_bits = data[2];
+		}
+	}
+
+	return 0;
+}
+
+static unsigned long k3_dwmmc_caps[4] = {
+	MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED,
+	MMC_CAP_8_BIT_DATA | MMC_CAP_MMC_HIGHSPEED,
+	0,
+	0,
+};
+
+static const struct dw_mci_drv_data k3_drv_data = {
+	.caps			= k3_dwmmc_caps,
+	.init			= dw_mci_k3_priv_init,
+	.set_ios		= dw_mci_k3_set_ios,
+	.setup_clock		= dw_mci_k3_setup_clock,
+	.parse_dt		= dw_mci_k3_parse_dt,
+};
+
+static const struct of_device_id dw_mci_k3_match[] = {
+	{ .compatible = "hisilicon,hi4511-dw-mshc",
+			.data = &k3_drv_data, },
+	{},
+};
+MODULE_DEVICE_TABLE(of, dw_mci_k3_match);
+
+static int dw_mci_k3_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_k3_match, pdev->dev.of_node);
+	drv_data = match->data;
+
+	return dw_mci_pltfm_register(pdev, drv_data);
+}
+
+static int dw_mci_k3_suspend(struct device *dev)
+{
+	struct dw_mci *host = dev_get_drvdata(dev);
+
+	return dw_mci_suspend(host);
+}
+
+static int dw_mci_k3_resume(struct device *dev)
+{
+	struct dw_mci *host = dev_get_drvdata(dev);
+	struct dw_mci_k3_priv_data *priv = host->priv;
+
+	if (priv->type == DW_MCI_TYPE_HI4511) {
+		priv->old_timing = -1;
+		dw_mci_k3_tun(host, MMC_TIMING_LEGACY);
+	}
+
+	return dw_mci_resume(host);
+}
+
+SIMPLE_DEV_PM_OPS(dw_mci_k3_pmops, dw_mci_k3_suspend, dw_mci_k3_resume);
+
+static struct platform_driver dw_mci_k3_pltfm_driver = {
+	.probe		= dw_mci_k3_probe,
+	.remove		= dw_mci_pltfm_remove,
+	.driver		= {
+		.name		= "dwmmc_k3",
+		.of_match_table	= dw_mci_k3_match,
+		.pm		= &dw_mci_k3_pmops,
+	},
+};
+
+module_platform_driver(dw_mci_k3_pltfm_driver);
+
+MODULE_DESCRIPTION("K3 Specific DW-MSHC Driver Extension");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:dwmmc-k3");
-- 
1.7.9.5

Re: [PATCH 2/2] mmc: dw_mmc: add dw_mmc-k3 for k3 platform

[Chris Ball]

Hi Zhangfei, just small changes,

On Wed, Oct 23 2013, Zhangfei Gao wrote:
> +* tune-table: should be array of clock tune mmc controller

This sounds like it should be "tuning-table", and shouldn't it also
have a prefix?

> +			node = of_find_compatible_node(NULL, NULL,
> +						"hisilicon,pctrl");

No need to use two lines here.

> +		ret = of_property_read_u32_array(np,
> +				"clken-reg", data, 2);

Or here, same with the other of_property_read_u32_array() calls.

> +MODULE_DESCRIPTION("K3 Specific DW-MSHC Driver Extension");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:dwmmc-k3");

Can you add a MODULE_AUTHOR too?

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>

Re: [PATCH 2/2] mmc: dw_mmc: add dw_mmc-k3 for k3 platform

[Kumar Gala]

On Oct 23, 2013, at 8:03 AM, Zhangfei Gao wrote:

> update: fix typo
> 
> Add dw_mmc-k3.c for k3v2, support sd/emmc
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Tested-by: Zhigang Wang <brooke.wangzhigang@huawei.com>
> ---
> .../devicetree/bindings/mmc/k3-dw-mshc.txt         |   77 +++++
> drivers/mmc/host/Kconfig                           |   10 +
> drivers/mmc/host/Makefile                          |    1 +
> drivers/mmc/host/dw_mmc-k3.c                       |  297 ++++++++++++++++++++
> 4 files changed, 385 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
> create mode 100644 drivers/mmc/host/dw_mmc-k3.c
> 
> diff --git a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
> new file mode 100644
> index 0000000..0de8c27
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
> @@ -0,0 +1,77 @@
> +* Hisilicon specific extensions to the Synopsys Designware Mobile
> +  Storage Host Controller
> +
> +Read synopsis-dw-mshc.txt for more details
> +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 synopsis-dw-mshc.txt and the properties used by the Hisilicon specific
> +extensions to the Synopsys Designware Mobile Storage Host Controller.
> +
> +Required Properties:
> +
> +* compatible: should be
> +	- "hisilicon,hi4511-dw-mshc": for controllers with hi4511
> +	  specific extentions.
> +* vmmc-supply: should be vmmc used in dwmmc
> +* fifo-depth: should be provided if register can not provide correct value
> +* clken-reg: should be clock enable register and offset
> +* drv-sel-reg: should be driver delay select register, offset and bits
> +* sam-sel-reg: should be sample delay select register, offset and bits
> +* div-reg: should be divider register, offset and bits
> +* tune-table: should be array of clock tune mmc controller
> +

1. These should have vendor prefixes
2. why do we need the *-reg properties
3. for the *-reg properties its not clear what bits means?
4. tune-table is not well described, what does 'clock tune' mean, why an array?


> +Example:
> +
> +  The MSHC controller node can be split into two portions, SoC specific and
> +  board specific portions as listed below.
> +

Does dtc merge this all into a single node?  Would probably be good to have comments
in the example stating both that the merge into one node, and which is board and which
is soc.

> +	dwmmc_0: dwmmc0@fcd03000 {
> +		compatible = "hisilicon,hi4511-dw-mshc";
> +		reg = <0xfcd03000 0x1000>;
> +		interrupts = <0 16 4>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		clocks = <&clk_sd>, <&clk_ddrc_per>;
> +		clock-names = "ciu", "biu";
> +		clken-reg = <0x1f8 0>;
> +		drv-sel-reg = <0x1f8 4 4>;
> +		sam-sel-reg = <0x1f8 8 4>;
> +		div-reg = <0x1f8 1 3>;
> +		tune-table =
> +		<180000000 6 6 13 13 25000000>,
> +		<0 0 0 0 0 0>,
> +		<360000000 6 4 2 0 50000000>,
> +		<180000000 6 4 13 13 25000000>,
> +		<360000000 6 4 2 0 50000000>,
> +		<720000000 6 1 9 4 100000000>,
> +		<0 0 0 0 0 0>,
> +		<360000000 7 1 3 0 50000000>;
> +	};
> +	dwmmc0@fcd03000 {
> +		num-slots = <1>;
> +		vmmc-supply = <&ldo12>;
> +		fifo-depth = <0x100>;
> +		supports-highspeed;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&sd_pmx_pins &sd_cfg_func1 &sd_cfg_func2>;
> +		slot@0 {
> +			reg = <0>;
> +			bus-width = <4>;
> +			disable-wp;
> +			cd-gpios = <&gpio10 3 0>;
> +		};
> +	};
> +
> +PCTRL:
> +

should have a description

> +Required Properties:
> +* compatible: should be
> +	- "hisilicon,pctrl": Peripheral control
> +

Reg is not described as a required property.

> +Example:
> +
> +	pctrl: pctrl@fca09000 {
> +		compatible = "hisilicon,pctrl";
> +		reg = <0xfca09000 0x1000>;
> +	};

This should be in its own binding document, and not part of the mmc binding

- k
> 

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 2/2] mmc: dw_mmc: add dw_mmc-k3 for k3 platform

[zhangfei gao]

Dear Kumar

Thanks for the careful review.

On Sun, Oct 27, 2013 at 11:29 PM, Kumar Gala <galak@codeaurora.org> wrote:
>
> On Oct 23, 2013, at 8:03 AM, Zhangfei Gao wrote:
>

>> +Required Properties:
>> +
>> +* compatible: should be
>> +     - "hisilicon,hi4511-dw-mshc": for controllers with hi4511
>> +       specific extentions.
>> +* vmmc-supply: should be vmmc used in dwmmc
>> +* fifo-depth: should be provided if register can not provide correct value
>> +* clken-reg: should be clock enable register and offset
>> +* drv-sel-reg: should be driver delay select register, offset and bits
>> +* sam-sel-reg: should be sample delay select register, offset and bits
>> +* div-reg: should be divider register, offset and bits
>> +* tune-table: should be array of clock tune mmc controller
>> +
>
> 1. These should have vendor prefixes
However Olof and Mark once suggested not using prefix if vector is
internally used.

Example before:
> + - #hisilicon,clkdiv-table-cells : the number of parameters after phandle in
> +   hisilicon,clkdiv-table property.
> + - hisilicon,clkdiv-table : list of value that are used to configure clock

Olof:
For a binding that is your own, you don't need to prefix the properties.
Prefixes are mostly used when adding new vendor-specific attributes to a common
binding.

Mark:
People seem to be *very* keen on adding them for all bindings for some
reason.

> 2. why do we need the *-reg properties
> 3. for the *-reg properties its not clear what bits means?

The silicon has some limitaiton to tune the clock timing according to
working mode,
including driver delay, sample delay, and divider.
If not set even the host register may not read out.
The register in PCTRL described below,
And one register may contains sereral usage,
for example
mmc0: 0x1f8: bit 0 is clock en/dis, bit 1 ~ bit 3 controls div, bit
4~7 controls drv-sel, bit 8 ~11 controls sam-sel
mmc1: 0x1f8: bit 12 ~~
register, offset and bits stands for 0x1f8, start bit and how many bits.
Is this fine?


> 4. tune-table is not well described, what does 'clock tune' mean, why an array?
Will change to  "tuning-table"
It is values used for computing these div, drv, sample register,
Also contains system clock need to be set, and output clock output
after PCTRL control.

>
>
>> +Example:
>> +
>> +  The MSHC controller node can be split into two portions, SoC specific and
>> +  board specific portions as listed below.
>> +
>
> Does dtc merge this all into a single node?  Would probably be good to have comments
> in the example stating both that the merge into one node, and which is board and which
> is soc.
OK, will add comments to distinguish dtsi for soc and dts for board.

>
>> +     dwmmc_0: dwmmc0@fcd03000 {
>> +             compatible = "hisilicon,hi4511-dw-mshc";
>> +             reg = <0xfcd03000 0x1000>;
>> +             interrupts = <0 16 4>;
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>> +             clocks = <&clk_sd>, <&clk_ddrc_per>;
>> +             clock-names = "ciu", "biu";
>> +             clken-reg = <0x1f8 0>;
>> +             drv-sel-reg = <0x1f8 4 4>;
>> +             sam-sel-reg = <0x1f8 8 4>;
>> +             div-reg = <0x1f8 1 3>;
>> +             tune-table =
>> +             <180000000 6 6 13 13 25000000>,
>> +             <0 0 0 0 0 0>,
>> +             <360000000 6 4 2 0 50000000>,
>> +             <180000000 6 4 13 13 25000000>,
>> +             <360000000 6 4 2 0 50000000>,
>> +             <720000000 6 1 9 4 100000000>,
>> +             <0 0 0 0 0 0>,
>> +             <360000000 7 1 3 0 50000000>;
>> +     };
>> +     dwmmc0@fcd03000 {
>> +             num-slots = <1>;
>> +             vmmc-supply = <&ldo12>;
>> +             fifo-depth = <0x100>;
>> +             supports-highspeed;
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&sd_pmx_pins &sd_cfg_func1 &sd_cfg_func2>;
>> +             slot@0 {
>> +                     reg = <0>;
>> +                     bus-width = <4>;
>> +                     disable-wp;
>> +                     cd-gpios = <&gpio10 3 0>;
>> +             };
>> +     };
>> +
>> +PCTRL:
>> +
>
> should have a description
OK.

>
>> +Required Properties:
>> +* compatible: should be
>> +     - "hisilicon,pctrl": Peripheral control
>> +
>
> Reg is not described as a required property.
OK.

>
>> +Example:
>> +
>> +     pctrl: pctrl@fca09000 {
>> +             compatible = "hisilicon,pctrl";
>> +             reg = <0xfca09000 0x1000>;
>> +     };
>
> This should be in its own binding document, and not part of the mmc binding
bindings/arm/hisilicon/hisilicon.txt still not been merged in another
patch, where
we want to push such desc as well as other system binding.
Is it OK adding this node desc twice in two files, since mmc need such node.
or move this desc after hisilicon.txt get merged for saving conflict?

Thanks

RE: [PATCH 2/2] mmc: dw_mmc: add dw_mmc-k3 for k3 platform

[Seungwon Jeon]

Hi Zhangfei,

On Wed, Oct 23, 2013, Zhangfei Gao wrote:
> update: fix typo
> 
> Add dw_mmc-k3.c for k3v2, support sd/emmc
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Tested-by: Zhigang Wang <brooke.wangzhigang@huawei.com>
> ---
>  .../devicetree/bindings/mmc/k3-dw-mshc.txt         |   77 +++++
>  drivers/mmc/host/Kconfig                           |   10 +
>  drivers/mmc/host/Makefile                          |    1 +
>  drivers/mmc/host/dw_mmc-k3.c                       |  297 ++++++++++++++++++++
>  4 files changed, 385 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
>  create mode 100644 drivers/mmc/host/dw_mmc-k3.c
> 
> diff --git a/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
> b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
> new file mode 100644
> index 0000000..0de8c27
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/k3-dw-mshc.txt
> @@ -0,0 +1,77 @@
> +* Hisilicon specific extensions to the Synopsys Designware Mobile
> +  Storage Host Controller
> +
> +Read synopsis-dw-mshc.txt for more details
> +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 synopsis-dw-mshc.txt and the properties used by the Hisilicon specific
> +extensions to the Synopsys Designware Mobile Storage Host Controller.
> +
> +Required Properties:
> +
> +* compatible: should be
> +	- "hisilicon,hi4511-dw-mshc": for controllers with hi4511
> +	  specific extentions.
> +* vmmc-supply: should be vmmc used in dwmmc
> +* fifo-depth: should be provided if register can not provide correct value
> +* clken-reg: should be clock enable register and offset
> +* drv-sel-reg: should be driver delay select register, offset and bits
> +* sam-sel-reg: should be sample delay select register, offset and bits
> +* div-reg: should be divider register, offset and bits
> +* tune-table: should be array of clock tune mmc controller
> +
> +Example:
> +
> +  The MSHC controller node can be split into two portions, SoC specific and
> +  board specific portions as listed below.
> +
> +	dwmmc_0: dwmmc0@fcd03000 {
> +		compatible = "hisilicon,hi4511-dw-mshc";
> +		reg = <0xfcd03000 0x1000>;
> +		interrupts = <0 16 4>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		clocks = <&clk_sd>, <&clk_ddrc_per>;
> +		clock-names = "ciu", "biu";
> +		clken-reg = <0x1f8 0>;
> +		drv-sel-reg = <0x1f8 4 4>;
> +		sam-sel-reg = <0x1f8 8 4>;
> +		div-reg = <0x1f8 1 3>;
> +		tune-table =
> +		<180000000 6 6 13 13 25000000>,
> +		<0 0 0 0 0 0>,
> +		<360000000 6 4 2 0 50000000>,
> +		<180000000 6 4 13 13 25000000>,
> +		<360000000 6 4 2 0 50000000>,
> +		<720000000 6 1 9 4 100000000>,
> +		<0 0 0 0 0 0>,
> +		<360000000 7 1 3 0 50000000>;
> +	};
> +	dwmmc0@fcd03000 {
> +		num-slots = <1>;
> +		vmmc-supply = <&ldo12>;
> +		fifo-depth = <0x100>;
> +		supports-highspeed;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&sd_pmx_pins &sd_cfg_func1 &sd_cfg_func2>;
> +		slot@0 {
> +			reg = <0>;
> +			bus-width = <4>;
> +			disable-wp;
> +			cd-gpios = <&gpio10 3 0>;
> +		};
> +	};
> +
> +PCTRL:
> +
> +Required Properties:
> +* compatible: should be
> +	- "hisilicon,pctrl": Peripheral control
> +
> +Example:
> +
> +	pctrl: pctrl@fca09000 {
> +		compatible = "hisilicon,pctrl";
> +		reg = <0xfca09000 0x1000>;
> +	};
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 7fc5099..45aaa2d 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -575,6 +575,16 @@ config MMC_DW_SOCFPGA
>  	  This selects support for Altera SoCFPGA specific extensions to the
>  	  Synopsys DesignWare Memory Card Interface driver.
> 
> +config MMC_DW_K3
> +	tristate "K3 specific extensions for Synopsys DW Memory Card Interface"
> +	depends on MMC_DW
> +	select MMC_DW_PLTFM
> +	select MMC_DW_IDMAC
> +	help
> +	  This selects support for Hisilicon K3 SoC specific extensions to the
> +	  Synopsys DesignWare Memory Card Interface driver. Select this option
> +	  for platforms based on Hisilicon K3 SoC's.
> +
>  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 c41d0c3..64f5f8d 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -43,6 +43,7 @@ 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_K3)		+= dw_mmc-k3.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-k3.c b/drivers/mmc/host/dw_mmc-k3.c
> new file mode 100644
> index 0000000..0ff75328
> --- /dev/null
> +++ b/drivers/mmc/host/dw_mmc-k3.c
> @@ -0,0 +1,297 @@
> +/*
> + * Copyright (c) 2013 Linaro Ltd.
> + * Copyright (c) 2013 Hisilicon Limited.
> + *
> + * 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.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/dw_mmc.h>
> +#include <linux/of_address.h>
> +
> +#include "dw_mmc.h"
> +#include "dw_mmc-pltfm.h"
> +
> +enum dw_mci_k3_type {
> +	DW_MCI_TYPE_HI4511,
> +};
> +
> +static struct dw_mci_k3_compatible {
> +	char				*compatible;
> +	enum dw_mci_k3_type		type;
> +} k3_compat[] = {
> +	{
> +		.compatible	= "hisilicon,hi4511-dw-mshc",
> +		.type		= DW_MCI_TYPE_HI4511,
> +	},
> +};
> +
> +#define TABLE_WIDTH 6
> +#define TABLE_HEIGHT 8
> +struct dw_mci_k3_priv_data {
> +	enum dw_mci_k3_type	type;
> +	int			old_timing;
> +	u32			table[TABLE_WIDTH * TABLE_HEIGHT];
> +	u32			clken_reg;
> +	u32			clken_bit;
> +	u32			sam_reg;
> +	u32			sam_off;
> +	u32			sam_bits;
> +	u32			drv_reg;
> +	u32			drv_off;
> +	u32			drv_bits;
> +	u32			div_reg;
> +	u32			div_off;
> +	u32			div_bits;
> +};
> +
> +static void __iomem *pctrl;
> +static DEFINE_SPINLOCK(mmc_tuning_lock);
> +
> +static u32 dw_mci_k3_delay(u32 val, u32 para, u32 off, u32 len)
> +{
> +	u32 i;
> +
> +	if (para >= 0) {
> +		for (i = 0; i < len; i++) {
> +			if (para % 2)
> +				val |= 1<<(off + i);
> +			else
> +				val &= ~(1<<(off + i));
> +			para = para >> 1;
> +		}
> +	}
> +	return val;
> +}
> +
> +static void dw_mci_k3_set_timing(struct dw_mci_k3_priv_data *priv,
> +					u32 sam, u32 drv, u32 div)
> +{
> +	u32 val;
> +	unsigned long flags;
> +
> +	if (!pctrl || !priv->clken_reg || !priv->sam_reg
> +		|| !priv->drv_reg || !priv->div_reg)
> +		return;
> +
> +	spin_lock_irqsave(&mmc_tuning_lock, flags);
> +
> +	val = readl(pctrl + priv->clken_reg);
> +	val &= ~(1 << priv->clken_bit);
> +	writel(val, pctrl + priv->clken_reg);
> +
> +	val = readl(pctrl + priv->sam_reg);
> +	val = dw_mci_k3_delay(val, sam, priv->sam_off, priv->sam_bits);
> +	writel(val, pctrl + priv->sam_reg);
> +
> +	val = readl(pctrl + priv->drv_reg);
> +	val = dw_mci_k3_delay(val, drv, priv->drv_off, priv->drv_bits);
> +	writel(val, pctrl + priv->drv_reg);
> +
> +	val = readl(pctrl + priv->div_reg);
> +	val = dw_mci_k3_delay(val, div, priv->div_off, priv->div_bits);
> +	writel(val, pctrl + priv->div_reg);
> +
> +	val = readl(pctrl + priv->clken_reg);
> +	val |= 1 << priv->clken_bit;
> +	writel(val, pctrl + priv->clken_reg);
> +
> +	spin_unlock_irqrestore(&mmc_tuning_lock, flags);
> +}
> +
> +static void dw_mci_k3_tun(struct dw_mci *host, int timing)
> +{
> +	struct dw_mci_k3_priv_data *priv = host->priv;
> +	int ret;
> +	u32 *table = &priv->table[TABLE_WIDTH * timing];
> +
> +	if (priv->old_timing == timing)
> +		return;
> +
> +	ret = clk_set_rate(host->ciu_clk, table[0]);
> +	if (ret) {
> +		dev_err(host->dev, "clk_set_rate failed\n");
> +		return;
> +	}
> +	dw_mci_k3_set_timing(priv, (table[3] + table[4]) / 2,
> +			table[2], table[1]);
> +	host->bus_hz = table[5];
> +	priv->old_timing = timing;
> +}
> +
> +static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> +{
> +	struct dw_mci_k3_priv_data *priv = host->priv;
> +
> +	if (priv->type == DW_MCI_TYPE_HI4511)
There are several checking controller type.
But now DW_MCI_TYPE_HI4511 is just introduced alone.
It seems like unnecessary.

> +		dw_mci_k3_tun(host, ios->timing);
> +}
> +
> +static int dw_mci_k3_priv_init(struct dw_mci *host)
> +{
> +	struct dw_mci_k3_priv_data *priv;
> +	int i, 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->old_timing = -1;
> +	host->priv = priv;
> +
> +	for (i = 0; i < ARRAY_SIZE(k3_compat); i++) {
> +		if (of_device_is_compatible(host->dev->of_node,
> +					k3_compat[i].compatible))
> +			priv->type = k3_compat[i].type;
> +	}
> +
> +	if (priv->type == DW_MCI_TYPE_HI4511) {
> +		ret = of_property_read_u32_array(host->dev->of_node,
> +			"tune-table", priv->table, TABLE_WIDTH * TABLE_HEIGHT);
> +		if (ret) {
> +			dev_err(host->dev, "not found tune-table\n");
> +			return -EINVAL;
> +		}
> +
> +		if (!pctrl) {
> +			struct device_node *node;
> +
> +			node = of_find_compatible_node(NULL, NULL,
> +						"hisilicon,pctrl");
> +			pctrl = of_iomap(node, 0);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int dw_mci_k3_setup_clock(struct dw_mci *host)
> +{
> +	struct dw_mci_k3_priv_data *priv = host->priv;
> +
> +	if (priv->type == DW_MCI_TYPE_HI4511)
> +		dw_mci_k3_tun(host, MMC_TIMING_LEGACY);
> +
> +	return 0;
> +}
> +
> +static int dw_mci_k3_parse_dt(struct dw_mci *host)
> +{
> +	struct dw_mci_k3_priv_data *priv = host->priv;
> +	struct device_node *np = host->dev->of_node;
> +	u32 data[3];
> +	int ret;
> +
> +	if (priv->type == DW_MCI_TYPE_HI4511) {
Didn't you see Kernel panic here?
host->priv is not allocated yet, it's a invalid pointer dereference.
dw_mci_k3_parse_dt() is called prior to dw_mci_k3_priv_init().
You can check dw_mci_probe() sequence.

> +		ret = of_property_read_u32_array(np,
> +				"clken-reg", data, 2);
> +		if (!ret) {
> +			priv->clken_reg = data[0];
> +			priv->clken_bit = data[1];
> +		}
> +
> +		ret = of_property_read_u32_array(np,
> +				"drv-sel-reg", data, 3);
> +		if (!ret) {
> +			priv->drv_reg = data[0];
> +			priv->drv_off = data[1];
> +			priv->drv_bits = data[2];
> +		}
> +
> +		ret = of_property_read_u32_array(np,
> +				"sam-sel-reg", data, 3);
> +		if (!ret) {
> +			priv->sam_reg = data[0];
> +			priv->sam_off = data[1];
> +			priv->sam_bits = data[2];
> +		}
> +
> +		ret = of_property_read_u32_array(np,
> +				"div-reg", data, 3);
> +		if (!ret) {
> +			priv->div_reg = data[0];
> +			priv->div_off = data[1];
> +			priv->div_bits = data[2];
> +		}
Should these register information be got from dt?
It could be define in source code instead.

Thanks,
Seungwon Jeon
> +	}
> +
> +	return 0;
> +}
> +
> +static unsigned long k3_dwmmc_caps[4] = {
> +	MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED,
> +	MMC_CAP_8_BIT_DATA | MMC_CAP_MMC_HIGHSPEED,
> +	0,
> +	0,
> +};
> +
> +static const struct dw_mci_drv_data k3_drv_data = {
> +	.caps			= k3_dwmmc_caps,
> +	.init			= dw_mci_k3_priv_init,
> +	.set_ios		= dw_mci_k3_set_ios,
> +	.setup_clock		= dw_mci_k3_setup_clock,
> +	.parse_dt		= dw_mci_k3_parse_dt,
> +};
> +
> +static const struct of_device_id dw_mci_k3_match[] = {
> +	{ .compatible = "hisilicon,hi4511-dw-mshc",
> +			.data = &k3_drv_data, },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, dw_mci_k3_match);
> +
> +static int dw_mci_k3_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_k3_match, pdev->dev.of_node);
> +	drv_data = match->data;
> +
> +	return dw_mci_pltfm_register(pdev, drv_data);
> +}
> +
> +static int dw_mci_k3_suspend(struct device *dev)
> +{
> +	struct dw_mci *host = dev_get_drvdata(dev);
> +
> +	return dw_mci_suspend(host);
> +}
> +
> +static int dw_mci_k3_resume(struct device *dev)
> +{
> +	struct dw_mci *host = dev_get_drvdata(dev);
> +	struct dw_mci_k3_priv_data *priv = host->priv;
> +
> +	if (priv->type == DW_MCI_TYPE_HI4511) {
> +		priv->old_timing = -1;
> +		dw_mci_k3_tun(host, MMC_TIMING_LEGACY);
> +	}
> +
> +	return dw_mci_resume(host);
> +}
> +
> +SIMPLE_DEV_PM_OPS(dw_mci_k3_pmops, dw_mci_k3_suspend, dw_mci_k3_resume);
> +
> +static struct platform_driver dw_mci_k3_pltfm_driver = {
> +	.probe		= dw_mci_k3_probe,
> +	.remove		= dw_mci_pltfm_remove,
> +	.driver		= {
> +		.name		= "dwmmc_k3",
> +		.of_match_table	= dw_mci_k3_match,
> +		.pm		= &dw_mci_k3_pmops,
> +	},
> +};
> +
> +module_platform_driver(dw_mci_k3_pltfm_driver);
> +
> +MODULE_DESCRIPTION("K3 Specific DW-MSHC Driver Extension");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:dwmmc-k3");
> --
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mmc: dw_mmc: add dw_mmc-k3 for k3 platform

[zhangfei gao]

Dear Seungwon

Thanks for giving suggestion.

On Thu, Oct 31, 2013 at 11:24 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> Hi Zhangfei,

>> +static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>> +{
>> +     struct dw_mci_k3_priv_data *priv = host->priv;
>> +
>> +     if (priv->type == DW_MCI_TYPE_HI4511)
> There are several checking controller type.
> But now DW_MCI_TYPE_HI4511 is just introduced alone.
> It seems like unnecessary.

Yes, currently only K3V2 is added, and k3v3 code will be added soon.
Do you think type is more suitable to add later?

>> +static int dw_mci_k3_parse_dt(struct dw_mci *host)
>> +{
>> +     struct dw_mci_k3_priv_data *priv = host->priv;
>> +     struct device_node *np = host->dev->of_node;
>> +     u32 data[3];
>> +     int ret;
>> +
>> +     if (priv->type == DW_MCI_TYPE_HI4511) {
> Didn't you see Kernel panic here?
> host->priv is not allocated yet, it's a invalid pointer dereference.
> dw_mci_k3_parse_dt() is called prior to dw_mci_k3_priv_init().
> You can check dw_mci_probe() sequence.

It is no problem here
dw_mci_pltfm_register -> drv_data->init(host) -> dw_mci_probe -> dw_mci_parse_dt

>
>> +             ret = of_property_read_u32_array(np,
>> +                             "clken-reg", data, 2);
>> +             if (!ret) {
>> +                     priv->clken_reg = data[0];
>> +                     priv->clken_bit = data[1];
>> +             }
>> +
>> +             ret = of_property_read_u32_array(np,
>> +                             "drv-sel-reg", data, 3);
>> +             if (!ret) {
>> +                     priv->drv_reg = data[0];
>> +                     priv->drv_off = data[1];
>> +                     priv->drv_bits = data[2];
>> +             }
>> +
>> +             ret = of_property_read_u32_array(np,
>> +                             "sam-sel-reg", data, 3);
>> +             if (!ret) {
>> +                     priv->sam_reg = data[0];
>> +                     priv->sam_off = data[1];
>> +                     priv->sam_bits = data[2];
>> +             }
>> +
>> +             ret = of_property_read_u32_array(np,
>> +                             "div-reg", data, 3);
>> +             if (!ret) {
>> +                     priv->div_reg = data[0];
>> +                     priv->div_off = data[1];
>> +                     priv->div_bits = data[2];
>> +             }
> Should these register information be got from dt?
> It could be define in source code instead.

There are many register from pctrl node instead of mmc controller.
If set in code, we may read id and then switch id, set register, start
bit, bits number,
which is no rules for different controller, using defiine is not helpful.
for example:
 switch (idx) {
        case 0:
                clken_reg = 0x1F8;
                clken_bit = 0;
                drv_sel_reg = 0x1F8;
                drv_sel_bit = 4;
                sam_sel_reg = 0x1F8;
                sam_sel_bit = 8;
                div_reg = 0x1F8;
                div_bit = 1;
                break;
        case 1:
                clken_reg = 0x1F8;
                clken_bit = 12;
                drv_sel_reg = 0x1F8;
                drv_sel_bit = 16;
                sam_sel_reg = 0x1F8;
                sam_sel_bit = 20;
                div_reg = 0x1F8;
                div_bit = 13;
                break;
        case 2:
                clken_reg = 0x1F8;
                clken_bit = 24;
                drv_sel_reg = 0x1F8;
                drv_sel_bit = 28;
                sam_sel_reg = 0x1FC;
                sam_sel_bit = 0;
                div_reg = 0x1F8;
                div_bit = 25;
                break;

So define register offset, start bit and bits number in dts make it simplier.
Is this acceptable?

Thanks.

RE: [PATCH 2/2] mmc: dw_mmc: add dw_mmc-k3 for k3 platform

[Seungwon Jeon]

On Fri, November 01, 2013, zhangfei gao wrote:
> Dear Seungwon
> 
> Thanks for giving suggestion.
> 
> On Thu, Oct 31, 2013 at 11:24 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> > Hi Zhangfei,
> 
> >> +static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> >> +{
> >> +     struct dw_mci_k3_priv_data *priv = host->priv;
> >> +
> >> +     if (priv->type == DW_MCI_TYPE_HI4511)
> > There are several checking controller type.
> > But now DW_MCI_TYPE_HI4511 is just introduced alone.
> > It seems like unnecessary.
> 
> Yes, currently only K3V2 is added, and k3v3 code will be added soon.
> Do you think type is more suitable to add later?
Yes, it would be better.

> 
> >> +static int dw_mci_k3_parse_dt(struct dw_mci *host)
> >> +{
> >> +     struct dw_mci_k3_priv_data *priv = host->priv;
> >> +     struct device_node *np = host->dev->of_node;
> >> +     u32 data[3];
> >> +     int ret;
> >> +
> >> +     if (priv->type == DW_MCI_TYPE_HI4511) {
> > Didn't you see Kernel panic here?
> > host->priv is not allocated yet, it's a invalid pointer dereference.
> > dw_mci_k3_parse_dt() is called prior to dw_mci_k3_priv_init().
> > You can check dw_mci_probe() sequence.
> 
> It is no problem here
> dw_mci_pltfm_register -> drv_data->init(host) -> dw_mci_probe -> dw_mci_parse_dt
I guess your base is not latest.
Can you check cjb's tree?

> 
> >
> >> +             ret = of_property_read_u32_array(np,
> >> +                             "clken-reg", data, 2);
> >> +             if (!ret) {
> >> +                     priv->clken_reg = data[0];
> >> +                     priv->clken_bit = data[1];
> >> +             }
> >> +
> >> +             ret = of_property_read_u32_array(np,
> >> +                             "drv-sel-reg", data, 3);
> >> +             if (!ret) {
> >> +                     priv->drv_reg = data[0];
> >> +                     priv->drv_off = data[1];
> >> +                     priv->drv_bits = data[2];
> >> +             }
> >> +
> >> +             ret = of_property_read_u32_array(np,
> >> +                             "sam-sel-reg", data, 3);
> >> +             if (!ret) {
> >> +                     priv->sam_reg = data[0];
> >> +                     priv->sam_off = data[1];
> >> +                     priv->sam_bits = data[2];
> >> +             }
> >> +
> >> +             ret = of_property_read_u32_array(np,
> >> +                             "div-reg", data, 3);
> >> +             if (!ret) {
> >> +                     priv->div_reg = data[0];
> >> +                     priv->div_off = data[1];
> >> +                     priv->div_bits = data[2];
> >> +             }
> > Should these register information be got from dt?
> > It could be define in source code instead.
> 
> There are many register from pctrl node instead of mmc controller.
> If set in code, we may read id and then switch id, set register, start
> bit, bits number,
> which is no rules for different controller, using defiine is not helpful.
> for example:
>  switch (idx) {
>         case 0:
>                 clken_reg = 0x1F8;
>                 clken_bit = 0;
>                 drv_sel_reg = 0x1F8;
>                 drv_sel_bit = 4;
>                 sam_sel_reg = 0x1F8;
>                 sam_sel_bit = 8;
>                 div_reg = 0x1F8;
>                 div_bit = 1;
>                 break;
>         case 1:
>                 clken_reg = 0x1F8;
>                 clken_bit = 12;
>                 drv_sel_reg = 0x1F8;
>                 drv_sel_bit = 16;
>                 sam_sel_reg = 0x1F8;
>                 sam_sel_bit = 20;
>                 div_reg = 0x1F8;
>                 div_bit = 13;
>                 break;
>         case 2:
>                 clken_reg = 0x1F8;
>                 clken_bit = 24;
>                 drv_sel_reg = 0x1F8;
>                 drv_sel_bit = 28;
>                 sam_sel_reg = 0x1FC;
>                 sam_sel_bit = 0;
>                 div_reg = 0x1F8;
>                 div_bit = 25;
>                 break;
> 
> So define register offset, start bit and bits number in dts make it simplier.
> Is this acceptable?
It's up to you. I think there will be better ways.
For example, some table for variable register can be used for each controller type.
Additionally, if you intend to use dt, error handling is needed.
dw_mci_k3_parse_dt always returns success although of_property_read_u32_array is failed.

Thanks,
Seungwon Jeon

Re: [PATCH 2/2] mmc: dw_mmc: add dw_mmc-k3 for k3 platform

[zhangfei gao]

On Fri, Nov 1, 2013 at 1:21 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> On Fri, November 01, 2013, zhangfei gao wrote:
>> Dear Seungwon
>>
>> Thanks for giving suggestion.
>>
>> On Thu, Oct 31, 2013 at 11:24 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
>> > Hi Zhangfei,
>>
>> >> +static void dw_mci_k3_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>> >> +{
>> >> +     struct dw_mci_k3_priv_data *priv = host->priv;
>> >> +
>> >> +     if (priv->type == DW_MCI_TYPE_HI4511)
>> > There are several checking controller type.
>> > But now DW_MCI_TYPE_HI4511 is just introduced alone.
>> > It seems like unnecessary.
>>
>> Yes, currently only K3V2 is added, and k3v3 code will be added soon.
>> Do you think type is more suitable to add later?
> Yes, it would be better.
OK, got it.

>
>>
>> >> +static int dw_mci_k3_parse_dt(struct dw_mci *host)
>> >> +{
>> >> +     struct dw_mci_k3_priv_data *priv = host->priv;
>> >> +     struct device_node *np = host->dev->of_node;
>> >> +     u32 data[3];
>> >> +     int ret;
>> >> +
>> >> +     if (priv->type == DW_MCI_TYPE_HI4511) {
>> > Didn't you see Kernel panic here?
>> > host->priv is not allocated yet, it's a invalid pointer dereference.
>> > dw_mci_k3_parse_dt() is called prior to dw_mci_k3_priv_init().
>> > You can check dw_mci_probe() sequence.
>>
>> It is no problem here
>> dw_mci_pltfm_register -> drv_data->init(host) -> dw_mci_probe -> dw_mci_parse_dt
> I guess your base is not latest.
> Can you check cjb's tree?

Yes, thanks for the info.
It is changed in 3.12-rc2
While what we use is 3.12-rc1, will rebase.

>
>>
>> >
>> >> +             ret = of_property_read_u32_array(np,
>> >> +                             "clken-reg", data, 2);
>> >> +             if (!ret) {
>> >> +                     priv->clken_reg = data[0];
>> >> +                     priv->clken_bit = data[1];
>> >> +             }
>> >> +
>> >> +             ret = of_property_read_u32_array(np,
>> >> +                             "drv-sel-reg", data, 3);
>> >> +             if (!ret) {
>> >> +                     priv->drv_reg = data[0];
>> >> +                     priv->drv_off = data[1];
>> >> +                     priv->drv_bits = data[2];
>> >> +             }
>> >> +
>> >> +             ret = of_property_read_u32_array(np,
>> >> +                             "sam-sel-reg", data, 3);
>> >> +             if (!ret) {
>> >> +                     priv->sam_reg = data[0];
>> >> +                     priv->sam_off = data[1];
>> >> +                     priv->sam_bits = data[2];
>> >> +             }
>> >> +
>> >> +             ret = of_property_read_u32_array(np,
>> >> +                             "div-reg", data, 3);
>> >> +             if (!ret) {
>> >> +                     priv->div_reg = data[0];
>> >> +                     priv->div_off = data[1];
>> >> +                     priv->div_bits = data[2];
>> >> +             }
>> > Should these register information be got from dt?
>> > It could be define in source code instead.
>>
>> There are many register from pctrl node instead of mmc controller.
>> If set in code, we may read id and then switch id, set register, start
>> bit, bits number,
>> which is no rules for different controller, using defiine is not helpful.
>> for example:
>>  switch (idx) {
>>         case 0:
>>                 clken_reg = 0x1F8;
>>                 clken_bit = 0;
>>                 drv_sel_reg = 0x1F8;
>>                 drv_sel_bit = 4;
>>                 sam_sel_reg = 0x1F8;
>>                 sam_sel_bit = 8;
>>                 div_reg = 0x1F8;
>>                 div_bit = 1;
>>                 break;
>>         case 1:
>>                 clken_reg = 0x1F8;
>>                 clken_bit = 12;
>>                 drv_sel_reg = 0x1F8;
>>                 drv_sel_bit = 16;
>>                 sam_sel_reg = 0x1F8;
>>                 sam_sel_bit = 20;
>>                 div_reg = 0x1F8;
>>                 div_bit = 13;
>>                 break;
>>         case 2:
>>                 clken_reg = 0x1F8;
>>                 clken_bit = 24;
>>                 drv_sel_reg = 0x1F8;
>>                 drv_sel_bit = 28;
>>                 sam_sel_reg = 0x1FC;
>>                 sam_sel_bit = 0;
>>                 div_reg = 0x1F8;
>>                 div_bit = 25;
>>                 break;
>>
>> So define register offset, start bit and bits number in dts make it simplier.
>> Is this acceptable?
> It's up to you. I think there will be better ways.
> For example, some table for variable register can be used for each controller type.
> Additionally, if you intend to use dt, error handling is needed.
> dw_mci_k3_parse_dt always returns success although of_property_read_u32_array is failed.

The reason is they are optional property.
SD would fail if not setting the property, while emmc does not matter.
So set for optional, dw_mci_k3_set_timing will check whether they exists.

Thanks