[PATCH v2 0/5] Add intial support to DW MMC host on ZTE SoC

[PATCH v2 0/5] Add intial support to DW MMC host on ZTE SoC

[Jun Nie]

Add intial support to DW MMC host on ZTE SoC. It include platform
specific wrapper driver and workarounds for fifo quirk.

Patches are prepared based on latest dw mmc runtime change:
   https://github.com/jh80chung/dw-mmc.git for-ulf

Changes vs version 1:
  - Change fifo-addr-override to fifo-addr and remove its workaround tag in comments.
  - Remove ZX DW MMC driver reset cap in driver, which can be added in dt nodes.

Jun Nie (5):
  mmc: dt-bindings: add ZTE MMC bindings
  mmc: zx: Initial support for ZX mmc controller
  Documentation: synopsys-dw-mshc: add binding for fifo quirks
  mmc: dw: Add fifo address property
  mmc: dw: Add fifo watermark quirk

 .../devicetree/bindings/mmc/synopsys-dw-mshc.txt   |  13 ++
 .../devicetree/bindings/mmc/zx-dw-mshc.txt         |  34 +++
 drivers/mmc/host/Kconfig                           |   9 +
 drivers/mmc/host/Makefile                          |   1 +
 drivers/mmc/host/dw_mmc-zx.c                       | 230 +++++++++++++++++++++
 drivers/mmc/host/dw_mmc-zx.h                       |  23 +++
 drivers/mmc/host/dw_mmc.c                          |  16 +-
 include/linux/mmc/dw_mmc.h                         |   4 +
 8 files changed, 328 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/zx-dw-mshc.txt
 create mode 100644 drivers/mmc/host/dw_mmc-zx.c
 create mode 100644 drivers/mmc/host/dw_mmc-zx.h

-- 
1.9.1

[PATCH v2 1/5] mmc: dt-bindings: add ZTE MMC bindings

[Jun Nie]

Document the device-tree binding of ZTE MMC host

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 .../devicetree/bindings/mmc/zx-dw-mshc.txt         | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/zx-dw-mshc.txt

diff --git a/Documentation/devicetree/bindings/mmc/zx-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/zx-dw-mshc.txt
new file mode 100644
index 0000000..94cc6e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/zx-dw-mshc.txt
@@ -0,0 +1,34 @@
+* ZTE specific extensions to the Synopsys Designware Mobile Storage
+  Host Controller
+
+The Synopsys designware mobile storage host controller is used to interface
+a SoC with storage medium such as eMMC or SD/MMC cards. This file documents
+differences between the core Synopsys dw mshc controller properties described
+by synopsys-dw-mshc.txt and the properties used by the ZTE specific
+extensions to the Synopsys Designware Mobile Storage Host Controller.
+
+Required Properties:
+
+* compatible: should be
+	- "zx,dw-mshc": for ZX SoCs
+
+Example:
+
+	mmc1: mmc@1110000 {
+		compatible = "zte,dw-mshc";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0x01110000 0x1000>;
+		interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
+		fifo-depth = <32>;
+		fifo-addr = <0x200>;
+		fifo-watermark-quirk;
+		bus-width = <4>;
+		clock-frequency = <50000000>;
+		clocks = <&topcrm SD0_AHB>, <&topcrm SD0_WCLK>;
+		clock-names = "biu", "ciu";
+		num-slots = <1>;
+		max-frequency = <50000000>;
+		cap-sdio-irq;
+		cap-sd-highspeed;
+		status = "disabled";
-- 
1.9.1

[PATCH v2 2/5] mmc: zx: Initial support for ZX mmc controller

[Jun Nie]

This platform driver adds initial support for the DW host controller
found on ZTE SoCs.

It has been tested on ZX296718 EVB board currently. More support on
timing tuning will be added when hardware is available.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 drivers/mmc/host/Kconfig     |   9 ++
 drivers/mmc/host/Makefile    |   1 +
 drivers/mmc/host/dw_mmc-zx.c | 230 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/dw_mmc-zx.h |  23 +++++
 4 files changed, 263 insertions(+)
 create mode 100644 drivers/mmc/host/dw_mmc-zx.c
 create mode 100644 drivers/mmc/host/dw_mmc-zx.h

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 5274f50..2b3202c 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -662,6 +662,15 @@ config MMC_DW_ROCKCHIP
 	  Synopsys DesignWare Memory Card Interface driver. Select this option
 	  for platforms based on RK3066, RK3188 and RK3288 SoC's.
 
+config MMC_DW_ZX
+	tristate "ZTE specific extensions for Synopsys DW Memory Card Interface"
+	depends on MMC_DW
+	select MMC_DW_PLTFM
+	help
+	  This selects support for ZTE SoC specific extensions to the
+	  Synopsys DesignWare Memory Card Interface driver. Select this option
+	  for platforms based on ZX296718 SoC's.
+
 config MMC_SH_MMCIF
 	tristate "SuperH Internal MMCIF support"
 	depends on HAS_DMA
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index e2bdaaf..9766143 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_MMC_DW_EXYNOS)	+= dw_mmc-exynos.o
 obj-$(CONFIG_MMC_DW_K3)		+= dw_mmc-k3.o
 obj-$(CONFIG_MMC_DW_PCI)	+= dw_mmc-pci.o
 obj-$(CONFIG_MMC_DW_ROCKCHIP)	+= dw_mmc-rockchip.o
+obj-$(CONFIG_MMC_DW_ZX)		+= dw_mmc-zx.o
 obj-$(CONFIG_MMC_SH_MMCIF)	+= sh_mmcif.o
 obj-$(CONFIG_MMC_JZ4740)	+= jz4740_mmc.o
 obj-$(CONFIG_MMC_VUB300)	+= vub300.o
diff --git a/drivers/mmc/host/dw_mmc-zx.c b/drivers/mmc/host/dw_mmc-zx.c
new file mode 100644
index 0000000..0404f8e
--- /dev/null
+++ b/drivers/mmc/host/dw_mmc-zx.c
@@ -0,0 +1,230 @@
+/*
+ * ZX Specific Extensions for Synopsys DW Multimedia Card Interface driver
+ *
+ * Copyright (C) 2016, Linaro Ltd.
+ * Copyright (C) 2016, ZTE Corp.
+ *
+ * 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/clk.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mmc/dw_mmc.h>
+#include <linux/mmc/host.h>
+#include <linux/mmc/mmc.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include "dw_mmc.h"
+#include "dw_mmc-pltfm.h"
+#include "dw_mmc-zx.h"
+
+#define ZX_DLL_LOCKED BIT(2)
+
+struct dw_mci_zx_priv_data {
+	struct regmap	*sysc_base;
+};
+
+static int dw_mci_zx_emmc_set_delay(struct dw_mci *host, unsigned int delay,
+				    unsigned int clk_flag)
+{
+	struct dw_mci_zx_priv_data *priv = host->priv;
+	struct regmap *sysc_base = priv->sysc_base;
+	unsigned int clksel;
+	unsigned int loop = 1000;
+	int ret;
+
+	ret = regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
+			   PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4));
+	if (ret)
+		return ret;
+
+	ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG1, &clksel);
+	if (ret)
+		return ret;
+
+	if (clk_flag) {
+		clksel &= ~(CLK_SAMP_DELAY(0x7F));
+		clksel |= (delay << 8);
+	} else {
+		clksel &= ~(READ_DQS_DELAY(0x7F));
+		clksel |= delay;
+	}
+
+	regmap_write(sysc_base, LB_AON_EMMC_CFG_REG1, clksel);
+	regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
+		     PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4) |
+		     DLL_REG_SET);
+
+	do {
+		ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG2, &clksel);
+		if (ret)
+			return ret;
+
+	} while (--loop && !(clksel & ZX_DLL_LOCKED));
+
+	if (!loop) {
+		dev_err(host->dev, "Error: %s dll lock fail\n", __func__);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int dw_mci_zx_emmc_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
+{
+	struct dw_mci *host = slot->host;
+	struct mmc_host *mmc = slot->mmc;
+	int len, start = 0, end = 0, delay, best = 0;
+	int ret = 0;
+
+	for (delay = 1 ; delay < 128; delay++) {
+		ret = dw_mci_zx_emmc_set_delay(host, delay, 1);
+		if (ret)
+			return ret;
+
+		if (mmc_send_tuning(mmc, opcode, NULL)) {
+			if (start >= 0) {
+				end = delay - 1;
+				/* check and update longest good range */
+				if ((end - start) > len) {
+					best = (start + end) >> 1;
+					len = end - start;
+				}
+			}
+			start = -1;
+			end = 0;
+			continue;
+		}
+		if (start < 0)
+			start = delay;
+	}
+
+	if (start >= 0) {
+		end = delay - 1;
+		if ((end - start) > len) {
+			best = (start + end) >> 1;
+			len = end - start;
+		}
+	}
+	if (best < 0)
+		return -EIO;
+
+	dev_info(host->dev, "%s best range: start %d end %d\n", __func__,
+		 start, end);
+	dw_mci_zx_emmc_set_delay(host, best, 1);
+	return 0;
+}
+
+static int dw_mci_zx_prepare_hs400_tuning(struct dw_mci *host,
+					  struct mmc_ios *ios)
+{
+	int ret;
+
+	/* config phase shift 90 */
+	ret = dw_mci_zx_emmc_set_delay(host, 32, 0);
+	if (ret < 0)
+		return -EIO;
+
+	return 0;
+}
+
+static int dw_mci_zx_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
+{
+	struct dw_mci *host = slot->host;
+
+	if (host->verid == 0x290a) /* emmc */
+		return dw_mci_zx_emmc_execute_tuning(slot, opcode);
+
+	return 0;
+}
+
+static int dw_mci_zx_parse_dt(struct dw_mci *host)
+{
+	struct device_node *np = host->dev->of_node;
+	struct device_node *node;
+	struct dw_mci_zx_priv_data *priv;
+	struct regmap *sysc_base;
+	int ret;
+
+	node = of_parse_phandle(np, "zte,aon-syscon", 0);
+	if (node) {
+		sysc_base = syscon_node_to_regmap(node);
+		of_node_put(node);
+
+		if (IS_ERR(sysc_base)) {
+			ret = PTR_ERR(sysc_base);
+			if (ret != -EPROBE_DEFER)
+				dev_err(host->dev, "Can't get syscon: %d\n",
+					ret);
+			return ret;
+		}
+	}
+
+	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	priv->sysc_base = sysc_base;
+	host->priv = priv;
+
+	return 0;
+}
+
+static unsigned long zx_dwmmc_caps[3] = {
+	MMC_CAP_CMD23,
+	MMC_CAP_CMD23,
+	MMC_CAP_CMD23,
+};
+
+static const struct dw_mci_drv_data zx_drv_data = {
+	.caps			= zx_dwmmc_caps,
+	.execute_tuning		= dw_mci_zx_execute_tuning,
+	.prepare_hs400_tuning	= dw_mci_zx_prepare_hs400_tuning,
+	.parse_dt               = dw_mci_zx_parse_dt,
+};
+
+static const struct of_device_id dw_mci_zx_match[] = {
+	{ .compatible = "zte,dw-mshc", .data = &zx_drv_data},
+};
+MODULE_DEVICE_TABLE(of, dw_mci_zx_match);
+
+static int dw_mci_zx_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_zx_match, pdev->dev.of_node);
+	drv_data = match->data;
+
+	return dw_mci_pltfm_register(pdev, drv_data);
+}
+
+static const struct dev_pm_ops dw_mci_zx_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
+			   dw_mci_runtime_resume,
+			   NULL)
+};
+
+static struct platform_driver dw_mci_zx_pltfm_driver = {
+	.probe		= dw_mci_zx_probe,
+	.remove		= dw_mci_pltfm_remove,
+	.driver		= {
+		.name		= "dwmmc_zx",
+		.of_match_table	= dw_mci_zx_match,
+		.pm		= &dw_mci_zx_dev_pm_ops,
+	},
+};
+
+module_platform_driver(dw_mci_zx_pltfm_driver);
+
+MODULE_DESCRIPTION("ZTE emmc/sd driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/mmc/host/dw_mmc-zx.h b/drivers/mmc/host/dw_mmc-zx.h
new file mode 100644
index 0000000..b1aac52
--- /dev/null
+++ b/drivers/mmc/host/dw_mmc-zx.h
@@ -0,0 +1,23 @@
+#ifndef _DW_MMC_ZX_H_
+#define _DW_MMC_ZX_H_
+
+/* dll reg offset*/
+#define LB_AON_EMMC_CFG_REG0  0x1B0
+#define LB_AON_EMMC_CFG_REG1  0x1B4
+#define LB_AON_EMMC_CFG_REG2  0x1B8
+
+/* LB_AON_EMMC_CFG_REG0 register defines */
+#define PARA_DLL_START_POINT(x)	(((x) & 0xFF) << 0)
+#define DLL_REG_SET		BIT(8)
+#define PARA_DLL_LOCK_NUM(x)	(((x) & 7) << 16)
+#define PARA_PHASE_DET_SEL(x)	(((x) & 7) << 20)
+#define PARA_DLL_BYPASS_MODE	BIT(23)
+#define PARA_HALF_CLK_MODE	BIT(24)
+
+/* LB_AON_EMMC_CFG_REG1 register defines */
+#define READ_DQS_DELAY(x)	(((x) & 0x7F) << 0)
+#define READ_DQS_BYPASS_MODE	BIT(7)
+#define CLK_SAMP_DELAY(x)	(((x) & 0x7F) << 8)
+#define CLK_SAMP_BYPASS_MODE	BIT(15)
+
+#endif /* _DW_MMC_ZX_H_ */
-- 
1.9.1

Re: [PATCH v2 2/5] mmc: zx: Initial support for ZX mmc controller

[Jaehoon Chung]

On 10/28/2016 11:37 AM, Jun Nie wrote:
> This platform driver adds initial support for the DW host controller
> found on ZTE SoCs.
> 
> It has been tested on ZX296718 EVB board currently. More support on
> timing tuning will be added when hardware is available.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  drivers/mmc/host/Kconfig     |   9 ++
>  drivers/mmc/host/Makefile    |   1 +
>  drivers/mmc/host/dw_mmc-zx.c | 230 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/mmc/host/dw_mmc-zx.h |  23 +++++
>  4 files changed, 263 insertions(+)
>  create mode 100644 drivers/mmc/host/dw_mmc-zx.c
>  create mode 100644 drivers/mmc/host/dw_mmc-zx.h
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 5274f50..2b3202c 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -662,6 +662,15 @@ config MMC_DW_ROCKCHIP
>  	  Synopsys DesignWare Memory Card Interface driver. Select this option
>  	  for platforms based on RK3066, RK3188 and RK3288 SoC's.
>  
> +config MMC_DW_ZX
> +	tristate "ZTE specific extensions for Synopsys DW Memory Card Interface"
> +	depends on MMC_DW

I guess MMC_DW_ZX depends on your SoC config, doesn't?

> +	select MMC_DW_PLTFM
> +	help
> +	  This selects support for ZTE SoC specific extensions to the
> +	  Synopsys DesignWare Memory Card Interface driver. Select this option
> +	  for platforms based on ZX296718 SoC's.
> +
>  config MMC_SH_MMCIF
>  	tristate "SuperH Internal MMCIF support"
>  	depends on HAS_DMA
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index e2bdaaf..9766143 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_MMC_DW_EXYNOS)	+= dw_mmc-exynos.o
>  obj-$(CONFIG_MMC_DW_K3)		+= dw_mmc-k3.o
>  obj-$(CONFIG_MMC_DW_PCI)	+= dw_mmc-pci.o
>  obj-$(CONFIG_MMC_DW_ROCKCHIP)	+= dw_mmc-rockchip.o
> +obj-$(CONFIG_MMC_DW_ZX)		+= dw_mmc-zx.o
>  obj-$(CONFIG_MMC_SH_MMCIF)	+= sh_mmcif.o
>  obj-$(CONFIG_MMC_JZ4740)	+= jz4740_mmc.o
>  obj-$(CONFIG_MMC_VUB300)	+= vub300.o
> diff --git a/drivers/mmc/host/dw_mmc-zx.c b/drivers/mmc/host/dw_mmc-zx.c
> new file mode 100644
> index 0000000..0404f8e
> --- /dev/null
> +++ b/drivers/mmc/host/dw_mmc-zx.c
> @@ -0,0 +1,230 @@
> +/*
> + * ZX Specific Extensions for Synopsys DW Multimedia Card Interface driver
> + *
> + * Copyright (C) 2016, Linaro Ltd.
> + * Copyright (C) 2016, ZTE Corp.
> + *
> + * 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/clk.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mmc/dw_mmc.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#include "dw_mmc.h"
> +#include "dw_mmc-pltfm.h"
> +#include "dw_mmc-zx.h"
> +
> +#define ZX_DLL_LOCKED BIT(2)

Some DLL rigsters and bits are defined in dw_mmc-zx.h.
why defined ZX_DLL_LOCKED at here.

You can choose that all defines locates to dw_mmc-zx.c or dw_mmc-zx.h

> +
> +struct dw_mci_zx_priv_data {
> +	struct regmap	*sysc_base;
> +};
> +
> +static int dw_mci_zx_emmc_set_delay(struct dw_mci *host, unsigned int delay,
> +				    unsigned int clk_flag)

Why do you use "unsigned int" as clk_flag? It's just use one of 0 and 1.
And 0 and 1 are what means? On/Off?

> +{
> +	struct dw_mci_zx_priv_data *priv = host->priv;
> +	struct regmap *sysc_base = priv->sysc_base;
> +	unsigned int clksel;
> +	unsigned int loop = 1000;
> +	int ret;
> +

priv->sysc_base doesn't never NULL?

> +	ret = regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
> +			   PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4));

Could you add the comment for controlling this regs?
I'm not sure because i didn't have ZX TRM..but PARA_DLL_LOCK_NUM should be locked?

It doesn't affect to other bit?
I think you can use the regmap_update_bits instead of regmap_write.

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG1, &clksel);
> +	if (ret)
> +		return ret;
> +
> +	if (clk_flag) {
> +		clksel &= ~(CLK_SAMP_DELAY(0x7F));

It's meaningless..CLK_SAMP_DELAY used only at here.
CLK_SAMP_DELAY ((x) & 0x7F << 0)

It's just CLK_SAMP_DELAY_MASK.?

#define CLK_SAMP_DELAY_MASK (0x7F << 0)
clksel &= ~CLK_SAMP_DELAY_MASK;


> +		clksel |= (delay << 8);

Use the CLK_SAMP_DELAY_SHIFT instead of 8.

> +	} else {
> +		clksel &= ~(READ_DQS_DELAY(0x7F));

Ditto.
And i think it also can be changed to regmap_update_bits.

> +		clksel |= delay;
> +	}
> +
> +	regmap_write(sysc_base, LB_AON_EMMC_CFG_REG1, clksel);
> +	regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
> +		     PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4) |
> +		     DLL_REG_SET);

regmap_update_bits?

> +
> +	do {
> +		ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG2, &clksel);
> +		if (ret)
> +			return ret;
> +
> +	} while (--loop && !(clksel & ZX_DLL_LOCKED));
> +
> +	if (!loop) {
> +		dev_err(host->dev, "Error: %s dll lock fail\n", __func__);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dw_mci_zx_emmc_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
> +{
> +	struct dw_mci *host = slot->host;
> +	struct mmc_host *mmc = slot->mmc;
> +	int len, start = 0, end = 0, delay, best = 0;
> +	int ret = 0;
> +
> +	for (delay = 1 ; delay < 128; delay++) {
> +		ret = dw_mci_zx_emmc_set_delay(host, delay, 1);
> +		if (ret)
> +			return ret;

When it's failed, just returned.
Doesn't need to try with next delay value?

> +
> +		if (mmc_send_tuning(mmc, opcode, NULL)) {
> +			if (start >= 0) {
> +				end = delay - 1;
> +				/* check and update longest good range */
> +				if ((end - start) > len) {
> +					best = (start + end) >> 1;
> +					len = end - start;
> +				}
> +			}
> +			start = -1;
> +			end = 0;
> +			continue;
> +		}
> +		if (start < 0)
> +			start = delay;
> +	}
> +
> +	if (start >= 0) {
> +		end = delay - 1;
> +		if ((end - start) > len) {
> +			best = (start + end) >> 1;
> +			len = end - start;
> +		}
> +	}
> +	if (best < 0)
> +		return -EIO;
> +
> +	dev_info(host->dev, "%s best range: start %d end %d\n", __func__,
> +		 start, end);
> +	dw_mci_zx_emmc_set_delay(host, best, 1);
> +	return 0;
> +}
> +
> +static int dw_mci_zx_prepare_hs400_tuning(struct dw_mci *host,
> +					  struct mmc_ios *ios)
> +{
> +	int ret;
> +
> +	/* config phase shift 90 */
> +	ret = dw_mci_zx_emmc_set_delay(host, 32, 0);

It's always fixed to 32? What means 32?

> +	if (ret < 0)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int dw_mci_zx_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
> +{
> +	struct dw_mci *host = slot->host;
> +
> +	if (host->verid == 0x290a) /* emmc */
> +		return dw_mci_zx_emmc_execute_tuning(slot, opcode);

I didn't know why you check host->verid is 2.90a..
Is there any reason?

> +
> +	return 0;
> +}
> +
> +static int dw_mci_zx_parse_dt(struct dw_mci *host)
> +{
> +	struct device_node *np = host->dev->of_node;
> +	struct device_node *node;
> +	struct dw_mci_zx_priv_data *priv;
> +	struct regmap *sysc_base;
> +	int ret;
> +
> +	node = of_parse_phandle(np, "zte,aon-syscon", 0);
> +	if (node) {
> +		sysc_base = syscon_node_to_regmap(node);
> +		of_node_put(node);

Use the syscon_regmap_lookup_by_phandle(). It's same behavior.

> +
> +		if (IS_ERR(sysc_base)) {
> +			ret = PTR_ERR(sysc_base);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(host->dev, "Can't get syscon: %d\n",
> +					ret);
> +			return ret;
> +		}
> +	}
> +
> +	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	priv->sysc_base = sysc_base;

Is there no case that sysc_base is NULL?

> +	host->priv = priv;
> +
> +	return 0;
> +}
> +
> +static unsigned long zx_dwmmc_caps[3] = {
> +	MMC_CAP_CMD23,
> +	MMC_CAP_CMD23,
> +	MMC_CAP_CMD23,
> +};
> +
> +static const struct dw_mci_drv_data zx_drv_data = {
> +	.caps			= zx_dwmmc_caps,
> +	.execute_tuning		= dw_mci_zx_execute_tuning,
> +	.prepare_hs400_tuning	= dw_mci_zx_prepare_hs400_tuning,
> +	.parse_dt               = dw_mci_zx_parse_dt,
> +};
> +
> +static const struct of_device_id dw_mci_zx_match[] = {
> +	{ .compatible = "zte,dw-mshc", .data = &zx_drv_data},
> +};
> +MODULE_DEVICE_TABLE(of, dw_mci_zx_match);
> +
> +static int dw_mci_zx_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_zx_match, pdev->dev.of_node);
> +	drv_data = match->data;
> +
> +	return dw_mci_pltfm_register(pdev, drv_data);
> +}
> +
> +static const struct dev_pm_ops dw_mci_zx_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				pm_runtime_force_resume)
> +	SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
> +			   dw_mci_runtime_resume,
> +			   NULL)
> +};
> +
> +static struct platform_driver dw_mci_zx_pltfm_driver = {
> +	.probe		= dw_mci_zx_probe,
> +	.remove		= dw_mci_pltfm_remove,
> +	.driver		= {
> +		.name		= "dwmmc_zx",
> +		.of_match_table	= dw_mci_zx_match,
> +		.pm		= &dw_mci_zx_dev_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(dw_mci_zx_pltfm_driver);
> +
> +MODULE_DESCRIPTION("ZTE emmc/sd driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mmc/host/dw_mmc-zx.h b/drivers/mmc/host/dw_mmc-zx.h
> new file mode 100644
> index 0000000..b1aac52
> --- /dev/null
> +++ b/drivers/mmc/host/dw_mmc-zx.h
> @@ -0,0 +1,23 @@
> +#ifndef _DW_MMC_ZX_H_
> +#define _DW_MMC_ZX_H_
> +
> +/* dll reg offset*/
> +#define LB_AON_EMMC_CFG_REG0  0x1B0
> +#define LB_AON_EMMC_CFG_REG1  0x1B4
> +#define LB_AON_EMMC_CFG_REG2  0x1B8
> +
> +/* LB_AON_EMMC_CFG_REG0 register defines */
> +#define PARA_DLL_START_POINT(x)	(((x) & 0xFF) << 0)
> +#define DLL_REG_SET		BIT(8)
> +#define PARA_DLL_LOCK_NUM(x)	(((x) & 7) << 16)
> +#define PARA_PHASE_DET_SEL(x)	(((x) & 7) << 20)
> +#define PARA_DLL_BYPASS_MODE	BIT(23)
> +#define PARA_HALF_CLK_MODE	BIT(24)

PAR_PHASE_DET_SEL/PARA_DLL_BYPASS_MODE_BIT/PARA_HALF_CLK_MODE are never used anywhere.

> +
> +/* LB_AON_EMMC_CFG_REG1 register defines */
> +#define READ_DQS_DELAY(x)	(((x) & 0x7F) << 0)
> +#define READ_DQS_BYPASS_MODE	BIT(7)
> +#define CLK_SAMP_DELAY(x)	(((x) & 0x7F) << 8)
> +#define CLK_SAMP_BYPASS_MODE	BIT(15)

Also READ_DQS_BYPASS_MODe/CLK_SAMP_BYBASS_MODE didnt used anywhere.


Hmm. These are not dwmmc host controller's register.
So If you needs to add these defines..I think you needs to add dessriptions in more detail.

At least..Which board's DLL reg offset. 

Best Regards,
Jaehoon Chung

> +
> +#endif /* _DW_MMC_ZX_H_ */
> 

Re: [PATCH v2 2/5] mmc: zx: Initial support for ZX mmc controller

[Jun Nie]

2016-10-28 13:16 GMT+08:00 Jaehoon Chung <jh80.chung@samsung.com>:
> On 10/28/2016 11:37 AM, Jun Nie wrote:
>> This platform driver adds initial support for the DW host controller
>> found on ZTE SoCs.
>>
>> It has been tested on ZX296718 EVB board currently. More support on
>> timing tuning will be added when hardware is available.
>>
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> ---
>>  drivers/mmc/host/Kconfig     |   9 ++
>>  drivers/mmc/host/Makefile    |   1 +
>>  drivers/mmc/host/dw_mmc-zx.c | 230 +++++++++++++++++++++++++++++++++++++++++++
>>  drivers/mmc/host/dw_mmc-zx.h |  23 +++++
>>  4 files changed, 263 insertions(+)
>>  create mode 100644 drivers/mmc/host/dw_mmc-zx.c
>>  create mode 100644 drivers/mmc/host/dw_mmc-zx.h
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 5274f50..2b3202c 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -662,6 +662,15 @@ config MMC_DW_ROCKCHIP
>>         Synopsys DesignWare Memory Card Interface driver. Select this option
>>         for platforms based on RK3066, RK3188 and RK3288 SoC's.
>>
>> +config MMC_DW_ZX
>> +     tristate "ZTE specific extensions for Synopsys DW Memory Card Interface"
>> +     depends on MMC_DW
>
> I guess MMC_DW_ZX depends on your SoC config, doesn't?

Right, will add dependency.

>
>> +     select MMC_DW_PLTFM
>> +     help
>> +       This selects support for ZTE SoC specific extensions to the
>> +       Synopsys DesignWare Memory Card Interface driver. Select this option
>> +       for platforms based on ZX296718 SoC's.
>> +
>>  config MMC_SH_MMCIF
>>       tristate "SuperH Internal MMCIF support"
>>       depends on HAS_DMA
>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>> index e2bdaaf..9766143 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -48,6 +48,7 @@ obj-$(CONFIG_MMC_DW_EXYNOS) += dw_mmc-exynos.o
>>  obj-$(CONFIG_MMC_DW_K3)              += dw_mmc-k3.o
>>  obj-$(CONFIG_MMC_DW_PCI)     += dw_mmc-pci.o
>>  obj-$(CONFIG_MMC_DW_ROCKCHIP)        += dw_mmc-rockchip.o
>> +obj-$(CONFIG_MMC_DW_ZX)              += dw_mmc-zx.o
>>  obj-$(CONFIG_MMC_SH_MMCIF)   += sh_mmcif.o
>>  obj-$(CONFIG_MMC_JZ4740)     += jz4740_mmc.o
>>  obj-$(CONFIG_MMC_VUB300)     += vub300.o
>> diff --git a/drivers/mmc/host/dw_mmc-zx.c b/drivers/mmc/host/dw_mmc-zx.c
>> new file mode 100644
>> index 0000000..0404f8e
>> --- /dev/null
>> +++ b/drivers/mmc/host/dw_mmc-zx.c
>> @@ -0,0 +1,230 @@
>> +/*
>> + * ZX Specific Extensions for Synopsys DW Multimedia Card Interface driver
>> + *
>> + * Copyright (C) 2016, Linaro Ltd.
>> + * Copyright (C) 2016, ZTE Corp.
>> + *
>> + * 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/clk.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/mmc/dw_mmc.h>
>> +#include <linux/mmc/host.h>
>> +#include <linux/mmc/mmc.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +
>> +#include "dw_mmc.h"
>> +#include "dw_mmc-pltfm.h"
>> +#include "dw_mmc-zx.h"
>> +
>> +#define ZX_DLL_LOCKED BIT(2)
>
> Some DLL rigsters and bits are defined in dw_mmc-zx.h.
> why defined ZX_DLL_LOCKED at here.
>
> You can choose that all defines locates to dw_mmc-zx.c or dw_mmc-zx.h
>
Will move together.

>> +
>> +struct dw_mci_zx_priv_data {
>> +     struct regmap   *sysc_base;
>> +};
>> +
>> +static int dw_mci_zx_emmc_set_delay(struct dw_mci *host, unsigned int delay,
>> +                                 unsigned int clk_flag)
>
> Why do you use "unsigned int" as clk_flag? It's just use one of 0 and 1.
> And 0 and 1 are what means? On/Off?
>
Yes,  enumeration shall looks better here. It is a flag for different
delay type. Will change to enumeration.

>> +{
>> +     struct dw_mci_zx_priv_data *priv = host->priv;
>> +     struct regmap *sysc_base = priv->sysc_base;
>> +     unsigned int clksel;
>> +     unsigned int loop = 1000;
>> +     int ret;
>> +
>
> priv->sysc_base doesn't never NULL?

For this SoC, it is never NULL if dts is correct. Adding a check is
better anyway.
>
>> +     ret = regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
>> +                        PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4));
>
> Could you add the comment for controlling this regs?
> I'm not sure because i didn't have ZX TRM..but PARA_DLL_LOCK_NUM should be locked?
>
> It doesn't affect to other bit?
> I think you can use the regmap_update_bits instead of regmap_write.

It does not affect other bit as all bits are write as desired. But
your suggestion make it clearer.

>
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG1, &clksel);
>> +     if (ret)
>> +             return ret;
>> +
>> +     if (clk_flag) {
>> +             clksel &= ~(CLK_SAMP_DELAY(0x7F));
>
> It's meaningless..CLK_SAMP_DELAY used only at here.
> CLK_SAMP_DELAY ((x) & 0x7F << 0)
>
> It's just CLK_SAMP_DELAY_MASK.?
>
> #define CLK_SAMP_DELAY_MASK (0x7F << 0)
> clksel &= ~CLK_SAMP_DELAY_MASK;
>
Right, clearer.

>
>> +             clksel |= (delay << 8);
>
> Use the CLK_SAMP_DELAY_SHIFT instead of 8.
>
>> +     } else {
>> +             clksel &= ~(READ_DQS_DELAY(0x7F));
>
> Ditto.
> And i think it also can be changed to regmap_update_bits.
>
>> +             clksel |= delay;
>> +     }
>> +
>> +     regmap_write(sysc_base, LB_AON_EMMC_CFG_REG1, clksel);
>> +     regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
>> +                  PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4) |
>> +                  DLL_REG_SET);
>
> regmap_update_bits?

Will do.

>
>> +
>> +     do {
>> +             ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG2, &clksel);
>> +             if (ret)
>> +                     return ret;
>> +
>> +     } while (--loop && !(clksel & ZX_DLL_LOCKED));
>> +
>> +     if (!loop) {
>> +             dev_err(host->dev, "Error: %s dll lock fail\n", __func__);
>> +             return -EIO;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int dw_mci_zx_emmc_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
>> +{
>> +     struct dw_mci *host = slot->host;
>> +     struct mmc_host *mmc = slot->mmc;
>> +     int len, start = 0, end = 0, delay, best = 0;
>> +     int ret = 0;
>> +
>> +     for (delay = 1 ; delay < 128; delay++) {
>> +             ret = dw_mci_zx_emmc_set_delay(host, delay, 1);
>> +             if (ret)
>> +                     return ret;
>
> When it's failed, just returned.
> Doesn't need to try with next delay value?

Yes, more retry make robust.

>
>> +
>> +             if (mmc_send_tuning(mmc, opcode, NULL)) {
>> +                     if (start >= 0) {
>> +                             end = delay - 1;
>> +                             /* check and update longest good range */
>> +                             if ((end - start) > len) {
>> +                                     best = (start + end) >> 1;
>> +                                     len = end - start;
>> +                             }
>> +                     }
>> +                     start = -1;
>> +                     end = 0;
>> +                     continue;
>> +             }
>> +             if (start < 0)
>> +                     start = delay;
>> +     }
>> +
>> +     if (start >= 0) {
>> +             end = delay - 1;
>> +             if ((end - start) > len) {
>> +                     best = (start + end) >> 1;
>> +                     len = end - start;
>> +             }
>> +     }
>> +     if (best < 0)
>> +             return -EIO;
>> +
>> +     dev_info(host->dev, "%s best range: start %d end %d\n", __func__,
>> +              start, end);
>> +     dw_mci_zx_emmc_set_delay(host, best, 1);
>> +     return 0;
>> +}
>> +
>> +static int dw_mci_zx_prepare_hs400_tuning(struct dw_mci *host,
>> +                                       struct mmc_ios *ios)
>> +{
>> +     int ret;
>> +
>> +     /* config phase shift 90 */
>> +     ret = dw_mci_zx_emmc_set_delay(host, 32, 0);
>
> It's always fixed to 32? What means 32?

It means 90 degree shift for value 32. This configuration comes from
ZTE engineer as I do not have hardware for this tuning. Let's just
keep it with adding more comments till tuning is needed.

>
>> +     if (ret < 0)
>> +             return -EIO;
>> +
>> +     return 0;
>> +}
>> +
>> +static int dw_mci_zx_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
>> +{
>> +     struct dw_mci *host = slot->host;
>> +
>> +     if (host->verid == 0x290a) /* emmc */
>> +             return dw_mci_zx_emmc_execute_tuning(slot, opcode);
>
> I didn't know why you check host->verid is 2.90a..
> Is there any reason?
There are two version DW MMC IP on this SoC and different
configuration is needed for them. I do not have hardware for tuning
version 210A timing and will be added later.

>
>> +
>> +     return 0;
>> +}
>> +
>> +static int dw_mci_zx_parse_dt(struct dw_mci *host)
>> +{
>> +     struct device_node *np = host->dev->of_node;
>> +     struct device_node *node;
>> +     struct dw_mci_zx_priv_data *priv;
>> +     struct regmap *sysc_base;
>> +     int ret;
>> +
>> +     node = of_parse_phandle(np, "zte,aon-syscon", 0);
>> +     if (node) {
>> +             sysc_base = syscon_node_to_regmap(node);
>> +             of_node_put(node);
>
> Use the syscon_regmap_lookup_by_phandle(). It's same behavior.
Will do.
>
>> +
>> +             if (IS_ERR(sysc_base)) {
>> +                     ret = PTR_ERR(sysc_base);
>> +                     if (ret != -EPROBE_DEFER)
>> +                             dev_err(host->dev, "Can't get syscon: %d\n",
>> +                                     ret);
>> +                     return ret;
>> +             }
>> +     }
>> +
>> +     priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
>> +     if (!priv)
>> +             return -ENOMEM;
>> +     priv->sysc_base = sysc_base;
>
> Is there no case that sysc_base is NULL?

sysc_base is needed only for eMMC. So it is NULL for SD/MMC cases, and
we can save memory for SD/MMC cases here :)

>
>> +     host->priv = priv;
>> +
>> +     return 0;
>> +}
>> +
>> +static unsigned long zx_dwmmc_caps[3] = {
>> +     MMC_CAP_CMD23,
>> +     MMC_CAP_CMD23,
>> +     MMC_CAP_CMD23,
>> +};
>> +
>> +static const struct dw_mci_drv_data zx_drv_data = {
>> +     .caps                   = zx_dwmmc_caps,
>> +     .execute_tuning         = dw_mci_zx_execute_tuning,
>> +     .prepare_hs400_tuning   = dw_mci_zx_prepare_hs400_tuning,
>> +     .parse_dt               = dw_mci_zx_parse_dt,
>> +};
>> +
>> +static const struct of_device_id dw_mci_zx_match[] = {
>> +     { .compatible = "zte,dw-mshc", .data = &zx_drv_data},
>> +};
>> +MODULE_DEVICE_TABLE(of, dw_mci_zx_match);
>> +
>> +static int dw_mci_zx_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_zx_match, pdev->dev.of_node);
>> +     drv_data = match->data;
>> +
>> +     return dw_mci_pltfm_register(pdev, drv_data);
>> +}
>> +
>> +static const struct dev_pm_ops dw_mci_zx_dev_pm_ops = {
>> +     SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> +                             pm_runtime_force_resume)
>> +     SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
>> +                        dw_mci_runtime_resume,
>> +                        NULL)
>> +};
>> +
>> +static struct platform_driver dw_mci_zx_pltfm_driver = {
>> +     .probe          = dw_mci_zx_probe,
>> +     .remove         = dw_mci_pltfm_remove,
>> +     .driver         = {
>> +             .name           = "dwmmc_zx",
>> +             .of_match_table = dw_mci_zx_match,
>> +             .pm             = &dw_mci_zx_dev_pm_ops,
>> +     },
>> +};
>> +
>> +module_platform_driver(dw_mci_zx_pltfm_driver);
>> +
>> +MODULE_DESCRIPTION("ZTE emmc/sd driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/mmc/host/dw_mmc-zx.h b/drivers/mmc/host/dw_mmc-zx.h
>> new file mode 100644
>> index 0000000..b1aac52
>> --- /dev/null
>> +++ b/drivers/mmc/host/dw_mmc-zx.h
>> @@ -0,0 +1,23 @@
>> +#ifndef _DW_MMC_ZX_H_
>> +#define _DW_MMC_ZX_H_
>> +
>> +/* dll reg offset*/
>> +#define LB_AON_EMMC_CFG_REG0  0x1B0
>> +#define LB_AON_EMMC_CFG_REG1  0x1B4
>> +#define LB_AON_EMMC_CFG_REG2  0x1B8
>> +
>> +/* LB_AON_EMMC_CFG_REG0 register defines */
>> +#define PARA_DLL_START_POINT(x)      (((x) & 0xFF) << 0)
>> +#define DLL_REG_SET          BIT(8)
>> +#define PARA_DLL_LOCK_NUM(x) (((x) & 7) << 16)
>> +#define PARA_PHASE_DET_SEL(x)        (((x) & 7) << 20)
>> +#define PARA_DLL_BYPASS_MODE BIT(23)
>> +#define PARA_HALF_CLK_MODE   BIT(24)
>
> PAR_PHASE_DET_SEL/PARA_DLL_BYPASS_MODE_BIT/PARA_HALF_CLK_MODE are never used anywhere.
>
>> +
>> +/* LB_AON_EMMC_CFG_REG1 register defines */
>> +#define READ_DQS_DELAY(x)    (((x) & 0x7F) << 0)
>> +#define READ_DQS_BYPASS_MODE BIT(7)
>> +#define CLK_SAMP_DELAY(x)    (((x) & 0x7F) << 8)
>> +#define CLK_SAMP_BYPASS_MODE BIT(15)
>
> Also READ_DQS_BYPASS_MODe/CLK_SAMP_BYBASS_MODE didnt used anywhere.
>
>
> Hmm. These are not dwmmc host controller's register.
> So If you needs to add these defines..I think you needs to add dessriptions in more detail.
>
> At least..Which board's DLL reg offset.
>
> Best Regards,
> Jaehoon Chung
>
>> +
>> +#endif /* _DW_MMC_ZX_H_ */
>>
>

Re: [PATCH v2 2/5] mmc: zx: Initial support for ZX mmc controller

[Jaehoon Chung]

On 10/31/2016 05:47 PM, Jun Nie wrote:
> 2016-10-28 13:16 GMT+08:00 Jaehoon Chung <jh80.chung@samsung.com>:
>> On 10/28/2016 11:37 AM, Jun Nie wrote:
>>> This platform driver adds initial support for the DW host controller
>>> found on ZTE SoCs.
>>>
>>> It has been tested on ZX296718 EVB board currently. More support on
>>> timing tuning will be added when hardware is available.
>>>
>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>> ---
>>>  drivers/mmc/host/Kconfig     |   9 ++
>>>  drivers/mmc/host/Makefile    |   1 +
>>>  drivers/mmc/host/dw_mmc-zx.c | 230 +++++++++++++++++++++++++++++++++++++++++++
>>>  drivers/mmc/host/dw_mmc-zx.h |  23 +++++
>>>  4 files changed, 263 insertions(+)
>>>  create mode 100644 drivers/mmc/host/dw_mmc-zx.c
>>>  create mode 100644 drivers/mmc/host/dw_mmc-zx.h
>>>
>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>> index 5274f50..2b3202c 100644
>>> --- a/drivers/mmc/host/Kconfig
>>> +++ b/drivers/mmc/host/Kconfig
>>> @@ -662,6 +662,15 @@ config MMC_DW_ROCKCHIP
>>>         Synopsys DesignWare Memory Card Interface driver. Select this option
>>>         for platforms based on RK3066, RK3188 and RK3288 SoC's.
>>>
>>> +config MMC_DW_ZX
>>> +     tristate "ZTE specific extensions for Synopsys DW Memory Card Interface"
>>> +     depends on MMC_DW
>>
>> I guess MMC_DW_ZX depends on your SoC config, doesn't?
> 
> Right, will add dependency.
> 
>>
>>> +     select MMC_DW_PLTFM
>>> +     help
>>> +       This selects support for ZTE SoC specific extensions to the
>>> +       Synopsys DesignWare Memory Card Interface driver. Select this option
>>> +       for platforms based on ZX296718 SoC's.
>>> +
>>>  config MMC_SH_MMCIF
>>>       tristate "SuperH Internal MMCIF support"
>>>       depends on HAS_DMA
>>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>>> index e2bdaaf..9766143 100644
>>> --- a/drivers/mmc/host/Makefile
>>> +++ b/drivers/mmc/host/Makefile
>>> @@ -48,6 +48,7 @@ obj-$(CONFIG_MMC_DW_EXYNOS) += dw_mmc-exynos.o
>>>  obj-$(CONFIG_MMC_DW_K3)              += dw_mmc-k3.o
>>>  obj-$(CONFIG_MMC_DW_PCI)     += dw_mmc-pci.o
>>>  obj-$(CONFIG_MMC_DW_ROCKCHIP)        += dw_mmc-rockchip.o
>>> +obj-$(CONFIG_MMC_DW_ZX)              += dw_mmc-zx.o
>>>  obj-$(CONFIG_MMC_SH_MMCIF)   += sh_mmcif.o
>>>  obj-$(CONFIG_MMC_JZ4740)     += jz4740_mmc.o
>>>  obj-$(CONFIG_MMC_VUB300)     += vub300.o
>>> diff --git a/drivers/mmc/host/dw_mmc-zx.c b/drivers/mmc/host/dw_mmc-zx.c
>>> new file mode 100644
>>> index 0000000..0404f8e
>>> --- /dev/null
>>> +++ b/drivers/mmc/host/dw_mmc-zx.c
>>> @@ -0,0 +1,230 @@
>>> +/*
>>> + * ZX Specific Extensions for Synopsys DW Multimedia Card Interface driver
>>> + *
>>> + * Copyright (C) 2016, Linaro Ltd.
>>> + * Copyright (C) 2016, ZTE Corp.
>>> + *
>>> + * 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/clk.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/mmc/dw_mmc.h>
>>> +#include <linux/mmc/host.h>
>>> +#include <linux/mmc/mmc.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/slab.h>
>>> +
>>> +#include "dw_mmc.h"
>>> +#include "dw_mmc-pltfm.h"
>>> +#include "dw_mmc-zx.h"
>>> +
>>> +#define ZX_DLL_LOCKED BIT(2)
>>
>> Some DLL rigsters and bits are defined in dw_mmc-zx.h.
>> why defined ZX_DLL_LOCKED at here.
>>
>> You can choose that all defines locates to dw_mmc-zx.c or dw_mmc-zx.h
>>
> Will move together.
> 
>>> +
>>> +struct dw_mci_zx_priv_data {
>>> +     struct regmap   *sysc_base;
>>> +};
>>> +
>>> +static int dw_mci_zx_emmc_set_delay(struct dw_mci *host, unsigned int delay,
>>> +                                 unsigned int clk_flag)
>>
>> Why do you use "unsigned int" as clk_flag? It's just use one of 0 and 1.
>> And 0 and 1 are what means? On/Off?
>>
> Yes,  enumeration shall looks better here. It is a flag for different
> delay type. Will change to enumeration.
> 
>>> +{
>>> +     struct dw_mci_zx_priv_data *priv = host->priv;
>>> +     struct regmap *sysc_base = priv->sysc_base;
>>> +     unsigned int clksel;
>>> +     unsigned int loop = 1000;
>>> +     int ret;
>>> +
>>
>> priv->sysc_base doesn't never NULL?
> 
> For this SoC, it is never NULL if dts is correct. Adding a check is
> better anyway.
>>
>>> +     ret = regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
>>> +                        PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4));
>>
>> Could you add the comment for controlling this regs?
>> I'm not sure because i didn't have ZX TRM..but PARA_DLL_LOCK_NUM should be locked?
>>
>> It doesn't affect to other bit?
>> I think you can use the regmap_update_bits instead of regmap_write.
> 
> It does not affect other bit as all bits are write as desired. But
> your suggestion make it clearer.
> 
>>
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG1, &clksel);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     if (clk_flag) {
>>> +             clksel &= ~(CLK_SAMP_DELAY(0x7F));
>>
>> It's meaningless..CLK_SAMP_DELAY used only at here.
>> CLK_SAMP_DELAY ((x) & 0x7F << 0)
>>
>> It's just CLK_SAMP_DELAY_MASK.?
>>
>> #define CLK_SAMP_DELAY_MASK (0x7F << 0)
>> clksel &= ~CLK_SAMP_DELAY_MASK;
>>
> Right, clearer.
> 
>>
>>> +             clksel |= (delay << 8);
>>
>> Use the CLK_SAMP_DELAY_SHIFT instead of 8.
>>
>>> +     } else {
>>> +             clksel &= ~(READ_DQS_DELAY(0x7F));
>>
>> Ditto.
>> And i think it also can be changed to regmap_update_bits.
>>
>>> +             clksel |= delay;
>>> +     }
>>> +
>>> +     regmap_write(sysc_base, LB_AON_EMMC_CFG_REG1, clksel);
>>> +     regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
>>> +                  PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4) |
>>> +                  DLL_REG_SET);
>>
>> regmap_update_bits?
> 
> Will do.
> 
>>
>>> +
>>> +     do {
>>> +             ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG2, &clksel);
>>> +             if (ret)
>>> +                     return ret;
>>> +
>>> +     } while (--loop && !(clksel & ZX_DLL_LOCKED));
>>> +
>>> +     if (!loop) {
>>> +             dev_err(host->dev, "Error: %s dll lock fail\n", __func__);
>>> +             return -EIO;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int dw_mci_zx_emmc_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
>>> +{
>>> +     struct dw_mci *host = slot->host;
>>> +     struct mmc_host *mmc = slot->mmc;
>>> +     int len, start = 0, end = 0, delay, best = 0;
>>> +     int ret = 0;
>>> +
>>> +     for (delay = 1 ; delay < 128; delay++) {
>>> +             ret = dw_mci_zx_emmc_set_delay(host, delay, 1);
>>> +             if (ret)
>>> +                     return ret;
>>
>> When it's failed, just returned.
>> Doesn't need to try with next delay value?
> 
> Yes, more retry make robust.
> 
>>
>>> +
>>> +             if (mmc_send_tuning(mmc, opcode, NULL)) {
>>> +                     if (start >= 0) {
>>> +                             end = delay - 1;
>>> +                             /* check and update longest good range */
>>> +                             if ((end - start) > len) {
>>> +                                     best = (start + end) >> 1;
>>> +                                     len = end - start;
>>> +                             }
>>> +                     }
>>> +                     start = -1;
>>> +                     end = 0;
>>> +                     continue;
>>> +             }
>>> +             if (start < 0)
>>> +                     start = delay;
>>> +     }
>>> +
>>> +     if (start >= 0) {
>>> +             end = delay - 1;
>>> +             if ((end - start) > len) {
>>> +                     best = (start + end) >> 1;
>>> +                     len = end - start;
>>> +             }
>>> +     }
>>> +     if (best < 0)
>>> +             return -EIO;
>>> +
>>> +     dev_info(host->dev, "%s best range: start %d end %d\n", __func__,
>>> +              start, end);
>>> +     dw_mci_zx_emmc_set_delay(host, best, 1);
>>> +     return 0;
>>> +}
>>> +
>>> +static int dw_mci_zx_prepare_hs400_tuning(struct dw_mci *host,
>>> +                                       struct mmc_ios *ios)
>>> +{
>>> +     int ret;
>>> +
>>> +     /* config phase shift 90 */
>>> +     ret = dw_mci_zx_emmc_set_delay(host, 32, 0);
>>
>> It's always fixed to 32? What means 32?
> 
> It means 90 degree shift for value 32. This configuration comes from
> ZTE engineer as I do not have hardware for this tuning. Let's just
> keep it with adding more comments till tuning is needed.
> 
>>
>>> +     if (ret < 0)
>>> +             return -EIO;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int dw_mci_zx_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
>>> +{
>>> +     struct dw_mci *host = slot->host;
>>> +
>>> +     if (host->verid == 0x290a) /* emmc */
>>> +             return dw_mci_zx_emmc_execute_tuning(slot, opcode);
>>
>> I didn't know why you check host->verid is 2.90a..
>> Is there any reason?
> There are two version DW MMC IP on this SoC and different
> configuration is needed for them. I do not have hardware for tuning
> version 210A timing and will be added later.

you means there are two IP version with same SoC?

Best Regards,
Jaehoon Chung

> 
>>
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int dw_mci_zx_parse_dt(struct dw_mci *host)
>>> +{
>>> +     struct device_node *np = host->dev->of_node;
>>> +     struct device_node *node;
>>> +     struct dw_mci_zx_priv_data *priv;
>>> +     struct regmap *sysc_base;
>>> +     int ret;
>>> +
>>> +     node = of_parse_phandle(np, "zte,aon-syscon", 0);
>>> +     if (node) {
>>> +             sysc_base = syscon_node_to_regmap(node);
>>> +             of_node_put(node);
>>
>> Use the syscon_regmap_lookup_by_phandle(). It's same behavior.
> Will do.
>>
>>> +
>>> +             if (IS_ERR(sysc_base)) {
>>> +                     ret = PTR_ERR(sysc_base);
>>> +                     if (ret != -EPROBE_DEFER)
>>> +                             dev_err(host->dev, "Can't get syscon: %d\n",
>>> +                                     ret);
>>> +                     return ret;
>>> +             }
>>> +     }
>>> +
>>> +     priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
>>> +     if (!priv)
>>> +             return -ENOMEM;
>>> +     priv->sysc_base = sysc_base;
>>
>> Is there no case that sysc_base is NULL?
> 
> sysc_base is needed only for eMMC. So it is NULL for SD/MMC cases, and
> we can save memory for SD/MMC cases here :)
> 
>>
>>> +     host->priv = priv;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static unsigned long zx_dwmmc_caps[3] = {
>>> +     MMC_CAP_CMD23,
>>> +     MMC_CAP_CMD23,
>>> +     MMC_CAP_CMD23,
>>> +};
>>> +
>>> +static const struct dw_mci_drv_data zx_drv_data = {
>>> +     .caps                   = zx_dwmmc_caps,
>>> +     .execute_tuning         = dw_mci_zx_execute_tuning,
>>> +     .prepare_hs400_tuning   = dw_mci_zx_prepare_hs400_tuning,
>>> +     .parse_dt               = dw_mci_zx_parse_dt,
>>> +};
>>> +
>>> +static const struct of_device_id dw_mci_zx_match[] = {
>>> +     { .compatible = "zte,dw-mshc", .data = &zx_drv_data},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, dw_mci_zx_match);
>>> +
>>> +static int dw_mci_zx_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_zx_match, pdev->dev.of_node);
>>> +     drv_data = match->data;
>>> +
>>> +     return dw_mci_pltfm_register(pdev, drv_data);
>>> +}
>>> +
>>> +static const struct dev_pm_ops dw_mci_zx_dev_pm_ops = {
>>> +     SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>> +                             pm_runtime_force_resume)
>>> +     SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
>>> +                        dw_mci_runtime_resume,
>>> +                        NULL)
>>> +};
>>> +
>>> +static struct platform_driver dw_mci_zx_pltfm_driver = {
>>> +     .probe          = dw_mci_zx_probe,
>>> +     .remove         = dw_mci_pltfm_remove,
>>> +     .driver         = {
>>> +             .name           = "dwmmc_zx",
>>> +             .of_match_table = dw_mci_zx_match,
>>> +             .pm             = &dw_mci_zx_dev_pm_ops,
>>> +     },
>>> +};
>>> +
>>> +module_platform_driver(dw_mci_zx_pltfm_driver);
>>> +
>>> +MODULE_DESCRIPTION("ZTE emmc/sd driver");
>>> +MODULE_LICENSE("GPL v2");
>>> diff --git a/drivers/mmc/host/dw_mmc-zx.h b/drivers/mmc/host/dw_mmc-zx.h
>>> new file mode 100644
>>> index 0000000..b1aac52
>>> --- /dev/null
>>> +++ b/drivers/mmc/host/dw_mmc-zx.h
>>> @@ -0,0 +1,23 @@
>>> +#ifndef _DW_MMC_ZX_H_
>>> +#define _DW_MMC_ZX_H_
>>> +
>>> +/* dll reg offset*/
>>> +#define LB_AON_EMMC_CFG_REG0  0x1B0
>>> +#define LB_AON_EMMC_CFG_REG1  0x1B4
>>> +#define LB_AON_EMMC_CFG_REG2  0x1B8
>>> +
>>> +/* LB_AON_EMMC_CFG_REG0 register defines */
>>> +#define PARA_DLL_START_POINT(x)      (((x) & 0xFF) << 0)
>>> +#define DLL_REG_SET          BIT(8)
>>> +#define PARA_DLL_LOCK_NUM(x) (((x) & 7) << 16)
>>> +#define PARA_PHASE_DET_SEL(x)        (((x) & 7) << 20)
>>> +#define PARA_DLL_BYPASS_MODE BIT(23)
>>> +#define PARA_HALF_CLK_MODE   BIT(24)
>>
>> PAR_PHASE_DET_SEL/PARA_DLL_BYPASS_MODE_BIT/PARA_HALF_CLK_MODE are never used anywhere.
>>
>>> +
>>> +/* LB_AON_EMMC_CFG_REG1 register defines */
>>> +#define READ_DQS_DELAY(x)    (((x) & 0x7F) << 0)
>>> +#define READ_DQS_BYPASS_MODE BIT(7)
>>> +#define CLK_SAMP_DELAY(x)    (((x) & 0x7F) << 8)
>>> +#define CLK_SAMP_BYPASS_MODE BIT(15)
>>
>> Also READ_DQS_BYPASS_MODe/CLK_SAMP_BYBASS_MODE didnt used anywhere.
>>
>>
>> Hmm. These are not dwmmc host controller's register.
>> So If you needs to add these defines..I think you needs to add dessriptions in more detail.
>>
>> At least..Which board's DLL reg offset.
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>> +
>>> +#endif /* _DW_MMC_ZX_H_ */
>>>
>>
> 
> 
> 

Re: [PATCH v2 2/5] mmc: zx: Initial support for ZX mmc controller

[Jun Nie]

2016-10-31 17:40 GMT+08:00 Jaehoon Chung <jh80.chung@samsung.com>:
> On 10/31/2016 05:47 PM, Jun Nie wrote:
>> 2016-10-28 13:16 GMT+08:00 Jaehoon Chung <jh80.chung@samsung.com>:
>>> On 10/28/2016 11:37 AM, Jun Nie wrote:
>>>> This platform driver adds initial support for the DW host controller
>>>> found on ZTE SoCs.
>>>>
>>>> It has been tested on ZX296718 EVB board currently. More support on
>>>> timing tuning will be added when hardware is available.
>>>>
>>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>>> ---
>>>>  drivers/mmc/host/Kconfig     |   9 ++
>>>>  drivers/mmc/host/Makefile    |   1 +
>>>>  drivers/mmc/host/dw_mmc-zx.c | 230 +++++++++++++++++++++++++++++++++++++++++++
>>>>  drivers/mmc/host/dw_mmc-zx.h |  23 +++++
>>>>  4 files changed, 263 insertions(+)
>>>>  create mode 100644 drivers/mmc/host/dw_mmc-zx.c
>>>>  create mode 100644 drivers/mmc/host/dw_mmc-zx.h
>>>>
>>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>>> index 5274f50..2b3202c 100644
>>>> --- a/drivers/mmc/host/Kconfig
>>>> +++ b/drivers/mmc/host/Kconfig
>>>> @@ -662,6 +662,15 @@ config MMC_DW_ROCKCHIP
>>>>         Synopsys DesignWare Memory Card Interface driver. Select this option
>>>>         for platforms based on RK3066, RK3188 and RK3288 SoC's.
>>>>
>>>> +config MMC_DW_ZX
>>>> +     tristate "ZTE specific extensions for Synopsys DW Memory Card Interface"
>>>> +     depends on MMC_DW
>>>
>>> I guess MMC_DW_ZX depends on your SoC config, doesn't?
>>
>> Right, will add dependency.
>>
>>>
>>>> +     select MMC_DW_PLTFM
>>>> +     help
>>>> +       This selects support for ZTE SoC specific extensions to the
>>>> +       Synopsys DesignWare Memory Card Interface driver. Select this option
>>>> +       for platforms based on ZX296718 SoC's.
>>>> +
>>>>  config MMC_SH_MMCIF
>>>>       tristate "SuperH Internal MMCIF support"
>>>>       depends on HAS_DMA
>>>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>>>> index e2bdaaf..9766143 100644
>>>> --- a/drivers/mmc/host/Makefile
>>>> +++ b/drivers/mmc/host/Makefile
>>>> @@ -48,6 +48,7 @@ obj-$(CONFIG_MMC_DW_EXYNOS) += dw_mmc-exynos.o
>>>>  obj-$(CONFIG_MMC_DW_K3)              += dw_mmc-k3.o
>>>>  obj-$(CONFIG_MMC_DW_PCI)     += dw_mmc-pci.o
>>>>  obj-$(CONFIG_MMC_DW_ROCKCHIP)        += dw_mmc-rockchip.o
>>>> +obj-$(CONFIG_MMC_DW_ZX)              += dw_mmc-zx.o
>>>>  obj-$(CONFIG_MMC_SH_MMCIF)   += sh_mmcif.o
>>>>  obj-$(CONFIG_MMC_JZ4740)     += jz4740_mmc.o
>>>>  obj-$(CONFIG_MMC_VUB300)     += vub300.o
>>>> diff --git a/drivers/mmc/host/dw_mmc-zx.c b/drivers/mmc/host/dw_mmc-zx.c
>>>> new file mode 100644
>>>> index 0000000..0404f8e
>>>> --- /dev/null
>>>> +++ b/drivers/mmc/host/dw_mmc-zx.c
>>>> @@ -0,0 +1,230 @@
>>>> +/*
>>>> + * ZX Specific Extensions for Synopsys DW Multimedia Card Interface driver
>>>> + *
>>>> + * Copyright (C) 2016, Linaro Ltd.
>>>> + * Copyright (C) 2016, ZTE Corp.
>>>> + *
>>>> + * 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/clk.h>
>>>> +#include <linux/mfd/syscon.h>
>>>> +#include <linux/mmc/dw_mmc.h>
>>>> +#include <linux/mmc/host.h>
>>>> +#include <linux/mmc/mmc.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/pm_runtime.h>
>>>> +#include <linux/regmap.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +#include "dw_mmc.h"
>>>> +#include "dw_mmc-pltfm.h"
>>>> +#include "dw_mmc-zx.h"
>>>> +
>>>> +#define ZX_DLL_LOCKED BIT(2)
>>>
>>> Some DLL rigsters and bits are defined in dw_mmc-zx.h.
>>> why defined ZX_DLL_LOCKED at here.
>>>
>>> You can choose that all defines locates to dw_mmc-zx.c or dw_mmc-zx.h
>>>
>> Will move together.
>>
>>>> +
>>>> +struct dw_mci_zx_priv_data {
>>>> +     struct regmap   *sysc_base;
>>>> +};
>>>> +
>>>> +static int dw_mci_zx_emmc_set_delay(struct dw_mci *host, unsigned int delay,
>>>> +                                 unsigned int clk_flag)
>>>
>>> Why do you use "unsigned int" as clk_flag? It's just use one of 0 and 1.
>>> And 0 and 1 are what means? On/Off?
>>>
>> Yes,  enumeration shall looks better here. It is a flag for different
>> delay type. Will change to enumeration.
>>
>>>> +{
>>>> +     struct dw_mci_zx_priv_data *priv = host->priv;
>>>> +     struct regmap *sysc_base = priv->sysc_base;
>>>> +     unsigned int clksel;
>>>> +     unsigned int loop = 1000;
>>>> +     int ret;
>>>> +
>>>
>>> priv->sysc_base doesn't never NULL?
>>
>> For this SoC, it is never NULL if dts is correct. Adding a check is
>> better anyway.
>>>
>>>> +     ret = regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
>>>> +                        PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4));
>>>
>>> Could you add the comment for controlling this regs?
>>> I'm not sure because i didn't have ZX TRM..but PARA_DLL_LOCK_NUM should be locked?
>>>
>>> It doesn't affect to other bit?
>>> I think you can use the regmap_update_bits instead of regmap_write.
>>
>> It does not affect other bit as all bits are write as desired. But
>> your suggestion make it clearer.
>>
>>>
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG1, &clksel);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     if (clk_flag) {
>>>> +             clksel &= ~(CLK_SAMP_DELAY(0x7F));
>>>
>>> It's meaningless..CLK_SAMP_DELAY used only at here.
>>> CLK_SAMP_DELAY ((x) & 0x7F << 0)
>>>
>>> It's just CLK_SAMP_DELAY_MASK.?
>>>
>>> #define CLK_SAMP_DELAY_MASK (0x7F << 0)
>>> clksel &= ~CLK_SAMP_DELAY_MASK;
>>>
>> Right, clearer.
>>
>>>
>>>> +             clksel |= (delay << 8);
>>>
>>> Use the CLK_SAMP_DELAY_SHIFT instead of 8.
>>>
>>>> +     } else {
>>>> +             clksel &= ~(READ_DQS_DELAY(0x7F));
>>>
>>> Ditto.
>>> And i think it also can be changed to regmap_update_bits.
>>>
>>>> +             clksel |= delay;
>>>> +     }
>>>> +
>>>> +     regmap_write(sysc_base, LB_AON_EMMC_CFG_REG1, clksel);
>>>> +     regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
>>>> +                  PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4) |
>>>> +                  DLL_REG_SET);
>>>
>>> regmap_update_bits?
>>
>> Will do.
>>
>>>
>>>> +
>>>> +     do {
>>>> +             ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG2, &clksel);
>>>> +             if (ret)
>>>> +                     return ret;
>>>> +
>>>> +     } while (--loop && !(clksel & ZX_DLL_LOCKED));
>>>> +
>>>> +     if (!loop) {
>>>> +             dev_err(host->dev, "Error: %s dll lock fail\n", __func__);
>>>> +             return -EIO;
>>>> +     }
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int dw_mci_zx_emmc_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
>>>> +{
>>>> +     struct dw_mci *host = slot->host;
>>>> +     struct mmc_host *mmc = slot->mmc;
>>>> +     int len, start = 0, end = 0, delay, best = 0;
>>>> +     int ret = 0;
>>>> +
>>>> +     for (delay = 1 ; delay < 128; delay++) {
>>>> +             ret = dw_mci_zx_emmc_set_delay(host, delay, 1);
>>>> +             if (ret)
>>>> +                     return ret;
>>>
>>> When it's failed, just returned.
>>> Doesn't need to try with next delay value?
>>
>> Yes, more retry make robust.
>>
>>>
>>>> +
>>>> +             if (mmc_send_tuning(mmc, opcode, NULL)) {
>>>> +                     if (start >= 0) {
>>>> +                             end = delay - 1;
>>>> +                             /* check and update longest good range */
>>>> +                             if ((end - start) > len) {
>>>> +                                     best = (start + end) >> 1;
>>>> +                                     len = end - start;
>>>> +                             }
>>>> +                     }
>>>> +                     start = -1;
>>>> +                     end = 0;
>>>> +                     continue;
>>>> +             }
>>>> +             if (start < 0)
>>>> +                     start = delay;
>>>> +     }
>>>> +
>>>> +     if (start >= 0) {
>>>> +             end = delay - 1;
>>>> +             if ((end - start) > len) {
>>>> +                     best = (start + end) >> 1;
>>>> +                     len = end - start;
>>>> +             }
>>>> +     }
>>>> +     if (best < 0)
>>>> +             return -EIO;
>>>> +
>>>> +     dev_info(host->dev, "%s best range: start %d end %d\n", __func__,
>>>> +              start, end);
>>>> +     dw_mci_zx_emmc_set_delay(host, best, 1);
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int dw_mci_zx_prepare_hs400_tuning(struct dw_mci *host,
>>>> +                                       struct mmc_ios *ios)
>>>> +{
>>>> +     int ret;
>>>> +
>>>> +     /* config phase shift 90 */
>>>> +     ret = dw_mci_zx_emmc_set_delay(host, 32, 0);
>>>
>>> It's always fixed to 32? What means 32?
>>
>> It means 90 degree shift for value 32. This configuration comes from
>> ZTE engineer as I do not have hardware for this tuning. Let's just
>> keep it with adding more comments till tuning is needed.
>>
>>>
>>>> +     if (ret < 0)
>>>> +             return -EIO;
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int dw_mci_zx_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
>>>> +{
>>>> +     struct dw_mci *host = slot->host;
>>>> +
>>>> +     if (host->verid == 0x290a) /* emmc */
>>>> +             return dw_mci_zx_emmc_execute_tuning(slot, opcode);
>>>
>>> I didn't know why you check host->verid is 2.90a..
>>> Is there any reason?
>> There are two version DW MMC IP on this SoC and different
>> configuration is needed for them. I do not have hardware for tuning
>> version 210A timing and will be added later.
>
> you means there are two IP version with same SoC?
>
> Best Regards,
> Jaehoon Chung

Right, two version IP exist on the same SoC.

>
>>
>>>
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int dw_mci_zx_parse_dt(struct dw_mci *host)
>>>> +{
>>>> +     struct device_node *np = host->dev->of_node;
>>>> +     struct device_node *node;
>>>> +     struct dw_mci_zx_priv_data *priv;
>>>> +     struct regmap *sysc_base;
>>>> +     int ret;
>>>> +
>>>> +     node = of_parse_phandle(np, "zte,aon-syscon", 0);
>>>> +     if (node) {
>>>> +             sysc_base = syscon_node_to_regmap(node);
>>>> +             of_node_put(node);
>>>
>>> Use the syscon_regmap_lookup_by_phandle(). It's same behavior.
>> Will do.
>>>
>>>> +
>>>> +             if (IS_ERR(sysc_base)) {
>>>> +                     ret = PTR_ERR(sysc_base);
>>>> +                     if (ret != -EPROBE_DEFER)
>>>> +                             dev_err(host->dev, "Can't get syscon: %d\n",
>>>> +                                     ret);
>>>> +                     return ret;
>>>> +             }
>>>> +     }
>>>> +
>>>> +     priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
>>>> +     if (!priv)
>>>> +             return -ENOMEM;
>>>> +     priv->sysc_base = sysc_base;
>>>
>>> Is there no case that sysc_base is NULL?
>>
>> sysc_base is needed only for eMMC. So it is NULL for SD/MMC cases, and
>> we can save memory for SD/MMC cases here :)
>>
>>>
>>>> +     host->priv = priv;
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static unsigned long zx_dwmmc_caps[3] = {
>>>> +     MMC_CAP_CMD23,
>>>> +     MMC_CAP_CMD23,
>>>> +     MMC_CAP_CMD23,
>>>> +};
>>>> +
>>>> +static const struct dw_mci_drv_data zx_drv_data = {
>>>> +     .caps                   = zx_dwmmc_caps,
>>>> +     .execute_tuning         = dw_mci_zx_execute_tuning,
>>>> +     .prepare_hs400_tuning   = dw_mci_zx_prepare_hs400_tuning,
>>>> +     .parse_dt               = dw_mci_zx_parse_dt,
>>>> +};
>>>> +
>>>> +static const struct of_device_id dw_mci_zx_match[] = {
>>>> +     { .compatible = "zte,dw-mshc", .data = &zx_drv_data},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, dw_mci_zx_match);
>>>> +
>>>> +static int dw_mci_zx_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_zx_match, pdev->dev.of_node);
>>>> +     drv_data = match->data;
>>>> +
>>>> +     return dw_mci_pltfm_register(pdev, drv_data);
>>>> +}
>>>> +
>>>> +static const struct dev_pm_ops dw_mci_zx_dev_pm_ops = {
>>>> +     SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>>> +                             pm_runtime_force_resume)
>>>> +     SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
>>>> +                        dw_mci_runtime_resume,
>>>> +                        NULL)
>>>> +};
>>>> +
>>>> +static struct platform_driver dw_mci_zx_pltfm_driver = {
>>>> +     .probe          = dw_mci_zx_probe,
>>>> +     .remove         = dw_mci_pltfm_remove,
>>>> +     .driver         = {
>>>> +             .name           = "dwmmc_zx",
>>>> +             .of_match_table = dw_mci_zx_match,
>>>> +             .pm             = &dw_mci_zx_dev_pm_ops,
>>>> +     },
>>>> +};
>>>> +
>>>> +module_platform_driver(dw_mci_zx_pltfm_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("ZTE emmc/sd driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> diff --git a/drivers/mmc/host/dw_mmc-zx.h b/drivers/mmc/host/dw_mmc-zx.h
>>>> new file mode 100644
>>>> index 0000000..b1aac52
>>>> --- /dev/null
>>>> +++ b/drivers/mmc/host/dw_mmc-zx.h
>>>> @@ -0,0 +1,23 @@
>>>> +#ifndef _DW_MMC_ZX_H_
>>>> +#define _DW_MMC_ZX_H_
>>>> +
>>>> +/* dll reg offset*/
>>>> +#define LB_AON_EMMC_CFG_REG0  0x1B0
>>>> +#define LB_AON_EMMC_CFG_REG1  0x1B4
>>>> +#define LB_AON_EMMC_CFG_REG2  0x1B8
>>>> +
>>>> +/* LB_AON_EMMC_CFG_REG0 register defines */
>>>> +#define PARA_DLL_START_POINT(x)      (((x) & 0xFF) << 0)
>>>> +#define DLL_REG_SET          BIT(8)
>>>> +#define PARA_DLL_LOCK_NUM(x) (((x) & 7) << 16)
>>>> +#define PARA_PHASE_DET_SEL(x)        (((x) & 7) << 20)
>>>> +#define PARA_DLL_BYPASS_MODE BIT(23)
>>>> +#define PARA_HALF_CLK_MODE   BIT(24)
>>>
>>> PAR_PHASE_DET_SEL/PARA_DLL_BYPASS_MODE_BIT/PARA_HALF_CLK_MODE are never used anywhere.
>>>
>>>> +
>>>> +/* LB_AON_EMMC_CFG_REG1 register defines */
>>>> +#define READ_DQS_DELAY(x)    (((x) & 0x7F) << 0)
>>>> +#define READ_DQS_BYPASS_MODE BIT(7)
>>>> +#define CLK_SAMP_DELAY(x)    (((x) & 0x7F) << 8)
>>>> +#define CLK_SAMP_BYPASS_MODE BIT(15)
>>>
>>> Also READ_DQS_BYPASS_MODe/CLK_SAMP_BYBASS_MODE didnt used anywhere.
>>>
>>>
>>> Hmm. These are not dwmmc host controller's register.
>>> So If you needs to add these defines..I think you needs to add dessriptions in more detail.
>>>
>>> At least..Which board's DLL reg offset.
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>> +
>>>> +#endif /* _DW_MMC_ZX_H_ */
>>>>
>>>
>>
>>
>>
>

Re: [PATCH v2 2/5] mmc: zx: Initial support for ZX mmc controller

[Jaehoon Chung]

On 11/01/2016 10:16 AM, Jun Nie wrote:
> 2016-10-31 17:40 GMT+08:00 Jaehoon Chung <jh80.chung@samsung.com>:
>> On 10/31/2016 05:47 PM, Jun Nie wrote:
>>> 2016-10-28 13:16 GMT+08:00 Jaehoon Chung <jh80.chung@samsung.com>:
>>>> On 10/28/2016 11:37 AM, Jun Nie wrote:
>>>>> This platform driver adds initial support for the DW host controller
>>>>> found on ZTE SoCs.
>>>>>
>>>>> It has been tested on ZX296718 EVB board currently. More support on
>>>>> timing tuning will be added when hardware is available.
>>>>>
>>>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>>>> ---
>>>>>  drivers/mmc/host/Kconfig     |   9 ++
>>>>>  drivers/mmc/host/Makefile    |   1 +
>>>>>  drivers/mmc/host/dw_mmc-zx.c | 230 +++++++++++++++++++++++++++++++++++++++++++
>>>>>  drivers/mmc/host/dw_mmc-zx.h |  23 +++++
>>>>>  4 files changed, 263 insertions(+)
>>>>>  create mode 100644 drivers/mmc/host/dw_mmc-zx.c
>>>>>  create mode 100644 drivers/mmc/host/dw_mmc-zx.h
>>>>>
>>>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>>>> index 5274f50..2b3202c 100644
>>>>> --- a/drivers/mmc/host/Kconfig
>>>>> +++ b/drivers/mmc/host/Kconfig
>>>>> @@ -662,6 +662,15 @@ config MMC_DW_ROCKCHIP
>>>>>         Synopsys DesignWare Memory Card Interface driver. Select this option
>>>>>         for platforms based on RK3066, RK3188 and RK3288 SoC's.
>>>>>
>>>>> +config MMC_DW_ZX
>>>>> +     tristate "ZTE specific extensions for Synopsys DW Memory Card Interface"
>>>>> +     depends on MMC_DW
>>>>
>>>> I guess MMC_DW_ZX depends on your SoC config, doesn't?
>>>
>>> Right, will add dependency.
>>>
>>>>
>>>>> +     select MMC_DW_PLTFM
>>>>> +     help
>>>>> +       This selects support for ZTE SoC specific extensions to the
>>>>> +       Synopsys DesignWare Memory Card Interface driver. Select this option
>>>>> +       for platforms based on ZX296718 SoC's.
>>>>> +
>>>>>  config MMC_SH_MMCIF
>>>>>       tristate "SuperH Internal MMCIF support"
>>>>>       depends on HAS_DMA
>>>>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>>>>> index e2bdaaf..9766143 100644
>>>>> --- a/drivers/mmc/host/Makefile
>>>>> +++ b/drivers/mmc/host/Makefile
>>>>> @@ -48,6 +48,7 @@ obj-$(CONFIG_MMC_DW_EXYNOS) += dw_mmc-exynos.o
>>>>>  obj-$(CONFIG_MMC_DW_K3)              += dw_mmc-k3.o
>>>>>  obj-$(CONFIG_MMC_DW_PCI)     += dw_mmc-pci.o
>>>>>  obj-$(CONFIG_MMC_DW_ROCKCHIP)        += dw_mmc-rockchip.o
>>>>> +obj-$(CONFIG_MMC_DW_ZX)              += dw_mmc-zx.o
>>>>>  obj-$(CONFIG_MMC_SH_MMCIF)   += sh_mmcif.o
>>>>>  obj-$(CONFIG_MMC_JZ4740)     += jz4740_mmc.o
>>>>>  obj-$(CONFIG_MMC_VUB300)     += vub300.o
>>>>> diff --git a/drivers/mmc/host/dw_mmc-zx.c b/drivers/mmc/host/dw_mmc-zx.c
>>>>> new file mode 100644
>>>>> index 0000000..0404f8e
>>>>> --- /dev/null
>>>>> +++ b/drivers/mmc/host/dw_mmc-zx.c
>>>>> @@ -0,0 +1,230 @@
>>>>> +/*
>>>>> + * ZX Specific Extensions for Synopsys DW Multimedia Card Interface driver
>>>>> + *
>>>>> + * Copyright (C) 2016, Linaro Ltd.
>>>>> + * Copyright (C) 2016, ZTE Corp.
>>>>> + *
>>>>> + * 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/clk.h>
>>>>> +#include <linux/mfd/syscon.h>
>>>>> +#include <linux/mmc/dw_mmc.h>
>>>>> +#include <linux/mmc/host.h>
>>>>> +#include <linux/mmc/mmc.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +#include <linux/pm_runtime.h>
>>>>> +#include <linux/regmap.h>
>>>>> +#include <linux/slab.h>
>>>>> +
>>>>> +#include "dw_mmc.h"
>>>>> +#include "dw_mmc-pltfm.h"
>>>>> +#include "dw_mmc-zx.h"
>>>>> +
>>>>> +#define ZX_DLL_LOCKED BIT(2)
>>>>
>>>> Some DLL rigsters and bits are defined in dw_mmc-zx.h.
>>>> why defined ZX_DLL_LOCKED at here.
>>>>
>>>> You can choose that all defines locates to dw_mmc-zx.c or dw_mmc-zx.h
>>>>
>>> Will move together.
>>>
>>>>> +
>>>>> +struct dw_mci_zx_priv_data {
>>>>> +     struct regmap   *sysc_base;
>>>>> +};
>>>>> +
>>>>> +static int dw_mci_zx_emmc_set_delay(struct dw_mci *host, unsigned int delay,
>>>>> +                                 unsigned int clk_flag)
>>>>
>>>> Why do you use "unsigned int" as clk_flag? It's just use one of 0 and 1.
>>>> And 0 and 1 are what means? On/Off?
>>>>
>>> Yes,  enumeration shall looks better here. It is a flag for different
>>> delay type. Will change to enumeration.
>>>
>>>>> +{
>>>>> +     struct dw_mci_zx_priv_data *priv = host->priv;
>>>>> +     struct regmap *sysc_base = priv->sysc_base;
>>>>> +     unsigned int clksel;
>>>>> +     unsigned int loop = 1000;
>>>>> +     int ret;
>>>>> +
>>>>
>>>> priv->sysc_base doesn't never NULL?
>>>
>>> For this SoC, it is never NULL if dts is correct. Adding a check is
>>> better anyway.
>>>>
>>>>> +     ret = regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
>>>>> +                        PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4));
>>>>
>>>> Could you add the comment for controlling this regs?
>>>> I'm not sure because i didn't have ZX TRM..but PARA_DLL_LOCK_NUM should be locked?
>>>>
>>>> It doesn't affect to other bit?
>>>> I think you can use the regmap_update_bits instead of regmap_write.
>>>
>>> It does not affect other bit as all bits are write as desired. But
>>> your suggestion make it clearer.
>>>
>>>>
>>>>> +     if (ret)
>>>>> +             return ret;
>>>>> +
>>>>> +     ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG1, &clksel);
>>>>> +     if (ret)
>>>>> +             return ret;
>>>>> +
>>>>> +     if (clk_flag) {
>>>>> +             clksel &= ~(CLK_SAMP_DELAY(0x7F));
>>>>
>>>> It's meaningless..CLK_SAMP_DELAY used only at here.
>>>> CLK_SAMP_DELAY ((x) & 0x7F << 0)
>>>>
>>>> It's just CLK_SAMP_DELAY_MASK.?
>>>>
>>>> #define CLK_SAMP_DELAY_MASK (0x7F << 0)
>>>> clksel &= ~CLK_SAMP_DELAY_MASK;
>>>>
>>> Right, clearer.
>>>
>>>>
>>>>> +             clksel |= (delay << 8);
>>>>
>>>> Use the CLK_SAMP_DELAY_SHIFT instead of 8.
>>>>
>>>>> +     } else {
>>>>> +             clksel &= ~(READ_DQS_DELAY(0x7F));
>>>>
>>>> Ditto.
>>>> And i think it also can be changed to regmap_update_bits.
>>>>
>>>>> +             clksel |= delay;
>>>>> +     }
>>>>> +
>>>>> +     regmap_write(sysc_base, LB_AON_EMMC_CFG_REG1, clksel);
>>>>> +     regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
>>>>> +                  PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4) |
>>>>> +                  DLL_REG_SET);
>>>>
>>>> regmap_update_bits?
>>>
>>> Will do.
>>>
>>>>
>>>>> +
>>>>> +     do {
>>>>> +             ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG2, &clksel);
>>>>> +             if (ret)
>>>>> +                     return ret;
>>>>> +
>>>>> +     } while (--loop && !(clksel & ZX_DLL_LOCKED));
>>>>> +
>>>>> +     if (!loop) {
>>>>> +             dev_err(host->dev, "Error: %s dll lock fail\n", __func__);
>>>>> +             return -EIO;
>>>>> +     }
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int dw_mci_zx_emmc_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
>>>>> +{
>>>>> +     struct dw_mci *host = slot->host;
>>>>> +     struct mmc_host *mmc = slot->mmc;
>>>>> +     int len, start = 0, end = 0, delay, best = 0;
>>>>> +     int ret = 0;
>>>>> +
>>>>> +     for (delay = 1 ; delay < 128; delay++) {
>>>>> +             ret = dw_mci_zx_emmc_set_delay(host, delay, 1);
>>>>> +             if (ret)
>>>>> +                     return ret;
>>>>
>>>> When it's failed, just returned.
>>>> Doesn't need to try with next delay value?
>>>
>>> Yes, more retry make robust.
>>>
>>>>
>>>>> +
>>>>> +             if (mmc_send_tuning(mmc, opcode, NULL)) {
>>>>> +                     if (start >= 0) {
>>>>> +                             end = delay - 1;
>>>>> +                             /* check and update longest good range */
>>>>> +                             if ((end - start) > len) {
>>>>> +                                     best = (start + end) >> 1;
>>>>> +                                     len = end - start;
>>>>> +                             }
>>>>> +                     }
>>>>> +                     start = -1;
>>>>> +                     end = 0;
>>>>> +                     continue;
>>>>> +             }
>>>>> +             if (start < 0)
>>>>> +                     start = delay;
>>>>> +     }
>>>>> +
>>>>> +     if (start >= 0) {
>>>>> +             end = delay - 1;
>>>>> +             if ((end - start) > len) {
>>>>> +                     best = (start + end) >> 1;
>>>>> +                     len = end - start;
>>>>> +             }
>>>>> +     }
>>>>> +     if (best < 0)
>>>>> +             return -EIO;
>>>>> +
>>>>> +     dev_info(host->dev, "%s best range: start %d end %d\n", __func__,
>>>>> +              start, end);
>>>>> +     dw_mci_zx_emmc_set_delay(host, best, 1);
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int dw_mci_zx_prepare_hs400_tuning(struct dw_mci *host,
>>>>> +                                       struct mmc_ios *ios)
>>>>> +{
>>>>> +     int ret;
>>>>> +
>>>>> +     /* config phase shift 90 */
>>>>> +     ret = dw_mci_zx_emmc_set_delay(host, 32, 0);
>>>>
>>>> It's always fixed to 32? What means 32?
>>>
>>> It means 90 degree shift for value 32. This configuration comes from
>>> ZTE engineer as I do not have hardware for this tuning. Let's just
>>> keep it with adding more comments till tuning is needed.
>>>
>>>>
>>>>> +     if (ret < 0)
>>>>> +             return -EIO;
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int dw_mci_zx_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
>>>>> +{
>>>>> +     struct dw_mci *host = slot->host;
>>>>> +
>>>>> +     if (host->verid == 0x290a) /* emmc */
>>>>> +             return dw_mci_zx_emmc_execute_tuning(slot, opcode);
>>>>
>>>> I didn't know why you check host->verid is 2.90a..
>>>> Is there any reason?
>>> There are two version DW MMC IP on this SoC and different
>>> configuration is needed for them. I do not have hardware for tuning
>>> version 210A timing and will be added later.
>>
>> you means there are two IP version with same SoC?
>>
>> Best Regards,
>> Jaehoon Chung
> 
> Right, two version IP exist on the same SoC.

It's strange. :) The latest and older versions are existed on same SoC.
Then i recommend that add the comment for this status.
Why you put this condition, and which case is used..
Not just "/* emmc */" :)

One more thing..dw_mci_zx_emmc_set_delay() and dw_mci_zx_emmc_set_delay() are for only emmc?
other cards don't use this function?

just can remove "_emmc_"?

Best Regards,
Jaehoon Chung

> 
>>
>>>
>>>>
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int dw_mci_zx_parse_dt(struct dw_mci *host)
>>>>> +{
>>>>> +     struct device_node *np = host->dev->of_node;
>>>>> +     struct device_node *node;
>>>>> +     struct dw_mci_zx_priv_data *priv;
>>>>> +     struct regmap *sysc_base;
>>>>> +     int ret;
>>>>> +
>>>>> +     node = of_parse_phandle(np, "zte,aon-syscon", 0);
>>>>> +     if (node) {
>>>>> +             sysc_base = syscon_node_to_regmap(node);
>>>>> +             of_node_put(node);
>>>>
>>>> Use the syscon_regmap_lookup_by_phandle(). It's same behavior.
>>> Will do.
>>>>
>>>>> +
>>>>> +             if (IS_ERR(sysc_base)) {
>>>>> +                     ret = PTR_ERR(sysc_base);
>>>>> +                     if (ret != -EPROBE_DEFER)
>>>>> +                             dev_err(host->dev, "Can't get syscon: %d\n",
>>>>> +                                     ret);
>>>>> +                     return ret;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>> +     priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
>>>>> +     if (!priv)
>>>>> +             return -ENOMEM;
>>>>> +     priv->sysc_base = sysc_base;
>>>>
>>>> Is there no case that sysc_base is NULL?
>>>
>>> sysc_base is needed only for eMMC. So it is NULL for SD/MMC cases, and
>>> we can save memory for SD/MMC cases here :)
>>>
>>>>
>>>>> +     host->priv = priv;
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static unsigned long zx_dwmmc_caps[3] = {
>>>>> +     MMC_CAP_CMD23,
>>>>> +     MMC_CAP_CMD23,
>>>>> +     MMC_CAP_CMD23,
>>>>> +};
>>>>> +
>>>>> +static const struct dw_mci_drv_data zx_drv_data = {
>>>>> +     .caps                   = zx_dwmmc_caps,
>>>>> +     .execute_tuning         = dw_mci_zx_execute_tuning,
>>>>> +     .prepare_hs400_tuning   = dw_mci_zx_prepare_hs400_tuning,
>>>>> +     .parse_dt               = dw_mci_zx_parse_dt,
>>>>> +};
>>>>> +
>>>>> +static const struct of_device_id dw_mci_zx_match[] = {
>>>>> +     { .compatible = "zte,dw-mshc", .data = &zx_drv_data},
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, dw_mci_zx_match);
>>>>> +
>>>>> +static int dw_mci_zx_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_zx_match, pdev->dev.of_node);
>>>>> +     drv_data = match->data;
>>>>> +
>>>>> +     return dw_mci_pltfm_register(pdev, drv_data);
>>>>> +}
>>>>> +
>>>>> +static const struct dev_pm_ops dw_mci_zx_dev_pm_ops = {
>>>>> +     SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>>>> +                             pm_runtime_force_resume)
>>>>> +     SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
>>>>> +                        dw_mci_runtime_resume,
>>>>> +                        NULL)
>>>>> +};
>>>>> +
>>>>> +static struct platform_driver dw_mci_zx_pltfm_driver = {
>>>>> +     .probe          = dw_mci_zx_probe,
>>>>> +     .remove         = dw_mci_pltfm_remove,
>>>>> +     .driver         = {
>>>>> +             .name           = "dwmmc_zx",
>>>>> +             .of_match_table = dw_mci_zx_match,
>>>>> +             .pm             = &dw_mci_zx_dev_pm_ops,
>>>>> +     },
>>>>> +};
>>>>> +
>>>>> +module_platform_driver(dw_mci_zx_pltfm_driver);
>>>>> +
>>>>> +MODULE_DESCRIPTION("ZTE emmc/sd driver");
>>>>> +MODULE_LICENSE("GPL v2");
>>>>> diff --git a/drivers/mmc/host/dw_mmc-zx.h b/drivers/mmc/host/dw_mmc-zx.h
>>>>> new file mode 100644
>>>>> index 0000000..b1aac52
>>>>> --- /dev/null
>>>>> +++ b/drivers/mmc/host/dw_mmc-zx.h
>>>>> @@ -0,0 +1,23 @@
>>>>> +#ifndef _DW_MMC_ZX_H_
>>>>> +#define _DW_MMC_ZX_H_
>>>>> +
>>>>> +/* dll reg offset*/
>>>>> +#define LB_AON_EMMC_CFG_REG0  0x1B0
>>>>> +#define LB_AON_EMMC_CFG_REG1  0x1B4
>>>>> +#define LB_AON_EMMC_CFG_REG2  0x1B8
>>>>> +
>>>>> +/* LB_AON_EMMC_CFG_REG0 register defines */
>>>>> +#define PARA_DLL_START_POINT(x)      (((x) & 0xFF) << 0)
>>>>> +#define DLL_REG_SET          BIT(8)
>>>>> +#define PARA_DLL_LOCK_NUM(x) (((x) & 7) << 16)
>>>>> +#define PARA_PHASE_DET_SEL(x)        (((x) & 7) << 20)
>>>>> +#define PARA_DLL_BYPASS_MODE BIT(23)
>>>>> +#define PARA_HALF_CLK_MODE   BIT(24)
>>>>
>>>> PAR_PHASE_DET_SEL/PARA_DLL_BYPASS_MODE_BIT/PARA_HALF_CLK_MODE are never used anywhere.
>>>>
>>>>> +
>>>>> +/* LB_AON_EMMC_CFG_REG1 register defines */
>>>>> +#define READ_DQS_DELAY(x)    (((x) & 0x7F) << 0)
>>>>> +#define READ_DQS_BYPASS_MODE BIT(7)
>>>>> +#define CLK_SAMP_DELAY(x)    (((x) & 0x7F) << 8)
>>>>> +#define CLK_SAMP_BYPASS_MODE BIT(15)
>>>>
>>>> Also READ_DQS_BYPASS_MODe/CLK_SAMP_BYBASS_MODE didnt used anywhere.
>>>>
>>>>
>>>> Hmm. These are not dwmmc host controller's register.
>>>> So If you needs to add these defines..I think you needs to add dessriptions in more detail.
>>>>
>>>> At least..Which board's DLL reg offset.
>>>>
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>>> +
>>>>> +#endif /* _DW_MMC_ZX_H_ */
>>>>>
>>>>
>>>
>>>
>>>
>>
> 
> 
> 

Re: [PATCH v2 2/5] mmc: zx: Initial support for ZX mmc controller

[Jun Nie]

2016-11-01 15:25 GMT+08:00 Jaehoon Chung <jh80.chung@samsung.com>:
> On 11/01/2016 10:16 AM, Jun Nie wrote:
>> 2016-10-31 17:40 GMT+08:00 Jaehoon Chung <jh80.chung@samsung.com>:
>>> On 10/31/2016 05:47 PM, Jun Nie wrote:
>>>> 2016-10-28 13:16 GMT+08:00 Jaehoon Chung <jh80.chung@samsung.com>:
>>>>> On 10/28/2016 11:37 AM, Jun Nie wrote:
>>>>>> This platform driver adds initial support for the DW host controller
>>>>>> found on ZTE SoCs.
>>>>>>
>>>>>> It has been tested on ZX296718 EVB board currently. More support on
>>>>>> timing tuning will be added when hardware is available.
>>>>>>
>>>>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>>>>> ---
>>>>>>  drivers/mmc/host/Kconfig     |   9 ++
>>>>>>  drivers/mmc/host/Makefile    |   1 +
>>>>>>  drivers/mmc/host/dw_mmc-zx.c | 230 +++++++++++++++++++++++++++++++++++++++++++
>>>>>>  drivers/mmc/host/dw_mmc-zx.h |  23 +++++
>>>>>>  4 files changed, 263 insertions(+)
>>>>>>  create mode 100644 drivers/mmc/host/dw_mmc-zx.c
>>>>>>  create mode 100644 drivers/mmc/host/dw_mmc-zx.h
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>>>>> index 5274f50..2b3202c 100644
>>>>>> --- a/drivers/mmc/host/Kconfig
>>>>>> +++ b/drivers/mmc/host/Kconfig
>>>>>> @@ -662,6 +662,15 @@ config MMC_DW_ROCKCHIP
>>>>>>         Synopsys DesignWare Memory Card Interface driver. Select this option
>>>>>>         for platforms based on RK3066, RK3188 and RK3288 SoC's.
>>>>>>
>>>>>> +config MMC_DW_ZX
>>>>>> +     tristate "ZTE specific extensions for Synopsys DW Memory Card Interface"
>>>>>> +     depends on MMC_DW
>>>>>
>>>>> I guess MMC_DW_ZX depends on your SoC config, doesn't?
>>>>
>>>> Right, will add dependency.
>>>>
>>>>>
>>>>>> +     select MMC_DW_PLTFM
>>>>>> +     help
>>>>>> +       This selects support for ZTE SoC specific extensions to the
>>>>>> +       Synopsys DesignWare Memory Card Interface driver. Select this option
>>>>>> +       for platforms based on ZX296718 SoC's.
>>>>>> +
>>>>>>  config MMC_SH_MMCIF
>>>>>>       tristate "SuperH Internal MMCIF support"
>>>>>>       depends on HAS_DMA
>>>>>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>>>>>> index e2bdaaf..9766143 100644
>>>>>> --- a/drivers/mmc/host/Makefile
>>>>>> +++ b/drivers/mmc/host/Makefile
>>>>>> @@ -48,6 +48,7 @@ obj-$(CONFIG_MMC_DW_EXYNOS) += dw_mmc-exynos.o
>>>>>>  obj-$(CONFIG_MMC_DW_K3)              += dw_mmc-k3.o
>>>>>>  obj-$(CONFIG_MMC_DW_PCI)     += dw_mmc-pci.o
>>>>>>  obj-$(CONFIG_MMC_DW_ROCKCHIP)        += dw_mmc-rockchip.o
>>>>>> +obj-$(CONFIG_MMC_DW_ZX)              += dw_mmc-zx.o
>>>>>>  obj-$(CONFIG_MMC_SH_MMCIF)   += sh_mmcif.o
>>>>>>  obj-$(CONFIG_MMC_JZ4740)     += jz4740_mmc.o
>>>>>>  obj-$(CONFIG_MMC_VUB300)     += vub300.o
>>>>>> diff --git a/drivers/mmc/host/dw_mmc-zx.c b/drivers/mmc/host/dw_mmc-zx.c
>>>>>> new file mode 100644
>>>>>> index 0000000..0404f8e
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/mmc/host/dw_mmc-zx.c
>>>>>> @@ -0,0 +1,230 @@
>>>>>> +/*
>>>>>> + * ZX Specific Extensions for Synopsys DW Multimedia Card Interface driver
>>>>>> + *
>>>>>> + * Copyright (C) 2016, Linaro Ltd.
>>>>>> + * Copyright (C) 2016, ZTE Corp.
>>>>>> + *
>>>>>> + * 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/clk.h>
>>>>>> +#include <linux/mfd/syscon.h>
>>>>>> +#include <linux/mmc/dw_mmc.h>
>>>>>> +#include <linux/mmc/host.h>
>>>>>> +#include <linux/mmc/mmc.h>
>>>>>> +#include <linux/module.h>
>>>>>> +#include <linux/of.h>
>>>>>> +#include <linux/platform_device.h>
>>>>>> +#include <linux/pm_runtime.h>
>>>>>> +#include <linux/regmap.h>
>>>>>> +#include <linux/slab.h>
>>>>>> +
>>>>>> +#include "dw_mmc.h"
>>>>>> +#include "dw_mmc-pltfm.h"
>>>>>> +#include "dw_mmc-zx.h"
>>>>>> +
>>>>>> +#define ZX_DLL_LOCKED BIT(2)
>>>>>
>>>>> Some DLL rigsters and bits are defined in dw_mmc-zx.h.
>>>>> why defined ZX_DLL_LOCKED at here.
>>>>>
>>>>> You can choose that all defines locates to dw_mmc-zx.c or dw_mmc-zx.h
>>>>>
>>>> Will move together.
>>>>
>>>>>> +
>>>>>> +struct dw_mci_zx_priv_data {
>>>>>> +     struct regmap   *sysc_base;
>>>>>> +};
>>>>>> +
>>>>>> +static int dw_mci_zx_emmc_set_delay(struct dw_mci *host, unsigned int delay,
>>>>>> +                                 unsigned int clk_flag)
>>>>>
>>>>> Why do you use "unsigned int" as clk_flag? It's just use one of 0 and 1.
>>>>> And 0 and 1 are what means? On/Off?
>>>>>
>>>> Yes,  enumeration shall looks better here. It is a flag for different
>>>> delay type. Will change to enumeration.
>>>>
>>>>>> +{
>>>>>> +     struct dw_mci_zx_priv_data *priv = host->priv;
>>>>>> +     struct regmap *sysc_base = priv->sysc_base;
>>>>>> +     unsigned int clksel;
>>>>>> +     unsigned int loop = 1000;
>>>>>> +     int ret;
>>>>>> +
>>>>>
>>>>> priv->sysc_base doesn't never NULL?
>>>>
>>>> For this SoC, it is never NULL if dts is correct. Adding a check is
>>>> better anyway.
>>>>>
>>>>>> +     ret = regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
>>>>>> +                        PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4));
>>>>>
>>>>> Could you add the comment for controlling this regs?
>>>>> I'm not sure because i didn't have ZX TRM..but PARA_DLL_LOCK_NUM should be locked?
>>>>>
>>>>> It doesn't affect to other bit?
>>>>> I think you can use the regmap_update_bits instead of regmap_write.
>>>>
>>>> It does not affect other bit as all bits are write as desired. But
>>>> your suggestion make it clearer.
>>>>
>>>>>
>>>>>> +     if (ret)
>>>>>> +             return ret;
>>>>>> +
>>>>>> +     ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG1, &clksel);
>>>>>> +     if (ret)
>>>>>> +             return ret;
>>>>>> +
>>>>>> +     if (clk_flag) {
>>>>>> +             clksel &= ~(CLK_SAMP_DELAY(0x7F));
>>>>>
>>>>> It's meaningless..CLK_SAMP_DELAY used only at here.
>>>>> CLK_SAMP_DELAY ((x) & 0x7F << 0)
>>>>>
>>>>> It's just CLK_SAMP_DELAY_MASK.?
>>>>>
>>>>> #define CLK_SAMP_DELAY_MASK (0x7F << 0)
>>>>> clksel &= ~CLK_SAMP_DELAY_MASK;
>>>>>
>>>> Right, clearer.
>>>>
>>>>>
>>>>>> +             clksel |= (delay << 8);
>>>>>
>>>>> Use the CLK_SAMP_DELAY_SHIFT instead of 8.
>>>>>
>>>>>> +     } else {
>>>>>> +             clksel &= ~(READ_DQS_DELAY(0x7F));
>>>>>
>>>>> Ditto.
>>>>> And i think it also can be changed to regmap_update_bits.
>>>>>
>>>>>> +             clksel |= delay;
>>>>>> +     }
>>>>>> +
>>>>>> +     regmap_write(sysc_base, LB_AON_EMMC_CFG_REG1, clksel);
>>>>>> +     regmap_write(sysc_base, LB_AON_EMMC_CFG_REG0,
>>>>>> +                  PARA_DLL_START_POINT(4) | PARA_DLL_LOCK_NUM(4) |
>>>>>> +                  DLL_REG_SET);
>>>>>
>>>>> regmap_update_bits?
>>>>
>>>> Will do.
>>>>
>>>>>
>>>>>> +
>>>>>> +     do {
>>>>>> +             ret = regmap_read(sysc_base, LB_AON_EMMC_CFG_REG2, &clksel);
>>>>>> +             if (ret)
>>>>>> +                     return ret;
>>>>>> +
>>>>>> +     } while (--loop && !(clksel & ZX_DLL_LOCKED));
>>>>>> +
>>>>>> +     if (!loop) {
>>>>>> +             dev_err(host->dev, "Error: %s dll lock fail\n", __func__);
>>>>>> +             return -EIO;
>>>>>> +     }
>>>>>> +
>>>>>> +     return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int dw_mci_zx_emmc_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
>>>>>> +{
>>>>>> +     struct dw_mci *host = slot->host;
>>>>>> +     struct mmc_host *mmc = slot->mmc;
>>>>>> +     int len, start = 0, end = 0, delay, best = 0;
>>>>>> +     int ret = 0;
>>>>>> +
>>>>>> +     for (delay = 1 ; delay < 128; delay++) {
>>>>>> +             ret = dw_mci_zx_emmc_set_delay(host, delay, 1);
>>>>>> +             if (ret)
>>>>>> +                     return ret;
>>>>>
>>>>> When it's failed, just returned.
>>>>> Doesn't need to try with next delay value?
>>>>
>>>> Yes, more retry make robust.
>>>>
>>>>>
>>>>>> +
>>>>>> +             if (mmc_send_tuning(mmc, opcode, NULL)) {
>>>>>> +                     if (start >= 0) {
>>>>>> +                             end = delay - 1;
>>>>>> +                             /* check and update longest good range */
>>>>>> +                             if ((end - start) > len) {
>>>>>> +                                     best = (start + end) >> 1;
>>>>>> +                                     len = end - start;
>>>>>> +                             }
>>>>>> +                     }
>>>>>> +                     start = -1;
>>>>>> +                     end = 0;
>>>>>> +                     continue;
>>>>>> +             }
>>>>>> +             if (start < 0)
>>>>>> +                     start = delay;
>>>>>> +     }
>>>>>> +
>>>>>> +     if (start >= 0) {
>>>>>> +             end = delay - 1;
>>>>>> +             if ((end - start) > len) {
>>>>>> +                     best = (start + end) >> 1;
>>>>>> +                     len = end - start;
>>>>>> +             }
>>>>>> +     }
>>>>>> +     if (best < 0)
>>>>>> +             return -EIO;
>>>>>> +
>>>>>> +     dev_info(host->dev, "%s best range: start %d end %d\n", __func__,
>>>>>> +              start, end);
>>>>>> +     dw_mci_zx_emmc_set_delay(host, best, 1);
>>>>>> +     return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int dw_mci_zx_prepare_hs400_tuning(struct dw_mci *host,
>>>>>> +                                       struct mmc_ios *ios)
>>>>>> +{
>>>>>> +     int ret;
>>>>>> +
>>>>>> +     /* config phase shift 90 */
>>>>>> +     ret = dw_mci_zx_emmc_set_delay(host, 32, 0);
>>>>>
>>>>> It's always fixed to 32? What means 32?
>>>>
>>>> It means 90 degree shift for value 32. This configuration comes from
>>>> ZTE engineer as I do not have hardware for this tuning. Let's just
>>>> keep it with adding more comments till tuning is needed.
>>>>
>>>>>
>>>>>> +     if (ret < 0)
>>>>>> +             return -EIO;
>>>>>> +
>>>>>> +     return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int dw_mci_zx_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
>>>>>> +{
>>>>>> +     struct dw_mci *host = slot->host;
>>>>>> +
>>>>>> +     if (host->verid == 0x290a) /* emmc */
>>>>>> +             return dw_mci_zx_emmc_execute_tuning(slot, opcode);
>>>>>
>>>>> I didn't know why you check host->verid is 2.90a..
>>>>> Is there any reason?
>>>> There are two version DW MMC IP on this SoC and different
>>>> configuration is needed for them. I do not have hardware for tuning
>>>> version 210A timing and will be added later.
>>>
>>> you means there are two IP version with same SoC?
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>
>> Right, two version IP exist on the same SoC.
>
> It's strange. :) The latest and older versions are existed on same SoC.
> Then i recommend that add the comment for this status.
> Why you put this condition, and which case is used..
> Not just "/* emmc */" :)
>
> One more thing..dw_mci_zx_emmc_set_delay() and dw_mci_zx_emmc_set_delay() are for only emmc?
> other cards don't use this function?
>
> just can remove "_emmc_"?
>
> Best Regards,
> Jaehoon Chung

Yes, 290A version for eMMC and 210A version for SD/SDIO. Only 290A
version timing tuning use this function and tuning registers is SoC
specific. So *_emmc_* name is dedicated for 290A version here. We need
a name anyway, *_290A_* can be a candidate, but not as direct as
*_emmc_* for this SoCs.

>
>>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +     return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int dw_mci_zx_parse_dt(struct dw_mci *host)
>>>>>> +{
>>>>>> +     struct device_node *np = host->dev->of_node;
>>>>>> +     struct device_node *node;
>>>>>> +     struct dw_mci_zx_priv_data *priv;
>>>>>> +     struct regmap *sysc_base;
>>>>>> +     int ret;
>>>>>> +
>>>>>> +     node = of_parse_phandle(np, "zte,aon-syscon", 0);
>>>>>> +     if (node) {
>>>>>> +             sysc_base = syscon_node_to_regmap(node);
>>>>>> +             of_node_put(node);
>>>>>
>>>>> Use the syscon_regmap_lookup_by_phandle(). It's same behavior.
>>>> Will do.
>>>>>
>>>>>> +
>>>>>> +             if (IS_ERR(sysc_base)) {
>>>>>> +                     ret = PTR_ERR(sysc_base);
>>>>>> +                     if (ret != -EPROBE_DEFER)
>>>>>> +                             dev_err(host->dev, "Can't get syscon: %d\n",
>>>>>> +                                     ret);
>>>>>> +                     return ret;
>>>>>> +             }
>>>>>> +     }
>>>>>> +
>>>>>> +     priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
>>>>>> +     if (!priv)
>>>>>> +             return -ENOMEM;
>>>>>> +     priv->sysc_base = sysc_base;
>>>>>
>>>>> Is there no case that sysc_base is NULL?
>>>>
>>>> sysc_base is needed only for eMMC. So it is NULL for SD/MMC cases, and
>>>> we can save memory for SD/MMC cases here :)
>>>>
>>>>>
>>>>>> +     host->priv = priv;
>>>>>> +
>>>>>> +     return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static unsigned long zx_dwmmc_caps[3] = {
>>>>>> +     MMC_CAP_CMD23,
>>>>>> +     MMC_CAP_CMD23,
>>>>>> +     MMC_CAP_CMD23,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct dw_mci_drv_data zx_drv_data = {
>>>>>> +     .caps                   = zx_dwmmc_caps,
>>>>>> +     .execute_tuning         = dw_mci_zx_execute_tuning,
>>>>>> +     .prepare_hs400_tuning   = dw_mci_zx_prepare_hs400_tuning,
>>>>>> +     .parse_dt               = dw_mci_zx_parse_dt,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct of_device_id dw_mci_zx_match[] = {
>>>>>> +     { .compatible = "zte,dw-mshc", .data = &zx_drv_data},
>>>>>> +};
>>>>>> +MODULE_DEVICE_TABLE(of, dw_mci_zx_match);
>>>>>> +
>>>>>> +static int dw_mci_zx_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_zx_match, pdev->dev.of_node);
>>>>>> +     drv_data = match->data;
>>>>>> +
>>>>>> +     return dw_mci_pltfm_register(pdev, drv_data);
>>>>>> +}
>>>>>> +
>>>>>> +static const struct dev_pm_ops dw_mci_zx_dev_pm_ops = {
>>>>>> +     SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>>>>> +                             pm_runtime_force_resume)
>>>>>> +     SET_RUNTIME_PM_OPS(dw_mci_runtime_suspend,
>>>>>> +                        dw_mci_runtime_resume,
>>>>>> +                        NULL)
>>>>>> +};
>>>>>> +
>>>>>> +static struct platform_driver dw_mci_zx_pltfm_driver = {
>>>>>> +     .probe          = dw_mci_zx_probe,
>>>>>> +     .remove         = dw_mci_pltfm_remove,
>>>>>> +     .driver         = {
>>>>>> +             .name           = "dwmmc_zx",
>>>>>> +             .of_match_table = dw_mci_zx_match,
>>>>>> +             .pm             = &dw_mci_zx_dev_pm_ops,
>>>>>> +     },
>>>>>> +};
>>>>>> +
>>>>>> +module_platform_driver(dw_mci_zx_pltfm_driver);
>>>>>> +
>>>>>> +MODULE_DESCRIPTION("ZTE emmc/sd driver");
>>>>>> +MODULE_LICENSE("GPL v2");
>>>>>> diff --git a/drivers/mmc/host/dw_mmc-zx.h b/drivers/mmc/host/dw_mmc-zx.h
>>>>>> new file mode 100644
>>>>>> index 0000000..b1aac52
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/mmc/host/dw_mmc-zx.h
>>>>>> @@ -0,0 +1,23 @@
>>>>>> +#ifndef _DW_MMC_ZX_H_
>>>>>> +#define _DW_MMC_ZX_H_
>>>>>> +
>>>>>> +/* dll reg offset*/
>>>>>> +#define LB_AON_EMMC_CFG_REG0  0x1B0
>>>>>> +#define LB_AON_EMMC_CFG_REG1  0x1B4
>>>>>> +#define LB_AON_EMMC_CFG_REG2  0x1B8
>>>>>> +
>>>>>> +/* LB_AON_EMMC_CFG_REG0 register defines */
>>>>>> +#define PARA_DLL_START_POINT(x)      (((x) & 0xFF) << 0)
>>>>>> +#define DLL_REG_SET          BIT(8)
>>>>>> +#define PARA_DLL_LOCK_NUM(x) (((x) & 7) << 16)
>>>>>> +#define PARA_PHASE_DET_SEL(x)        (((x) & 7) << 20)
>>>>>> +#define PARA_DLL_BYPASS_MODE BIT(23)
>>>>>> +#define PARA_HALF_CLK_MODE   BIT(24)
>>>>>
>>>>> PAR_PHASE_DET_SEL/PARA_DLL_BYPASS_MODE_BIT/PARA_HALF_CLK_MODE are never used anywhere.
>>>>>
>>>>>> +
>>>>>> +/* LB_AON_EMMC_CFG_REG1 register defines */
>>>>>> +#define READ_DQS_DELAY(x)    (((x) & 0x7F) << 0)
>>>>>> +#define READ_DQS_BYPASS_MODE BIT(7)
>>>>>> +#define CLK_SAMP_DELAY(x)    (((x) & 0x7F) << 8)
>>>>>> +#define CLK_SAMP_BYPASS_MODE BIT(15)
>>>>>
>>>>> Also READ_DQS_BYPASS_MODe/CLK_SAMP_BYBASS_MODE didnt used anywhere.
>>>>>
>>>>>
>>>>> Hmm. These are not dwmmc host controller's register.
>>>>> So If you needs to add these defines..I think you needs to add dessriptions in more detail.
>>>>>
>>>>> At least..Which board's DLL reg offset.
>>>>>
>>>>> Best Regards,
>>>>> Jaehoon Chung
>>>>>
>>>>>> +
>>>>>> +#endif /* _DW_MMC_ZX_H_ */
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>

Re: [PATCH v2 2/5] mmc: zx: Initial support for ZX mmc controller

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2321 bytes --]

Hi Jun,

[auto build test ERROR on ulf.hansson-mmc/next]
[also build test ERROR on v4.9-rc2 next-20161028]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Jun-Nie/Add-intial-support-to-DW-MMC-host-on-ZTE-SoC/20161028-104045
base:   https://git.linaro.org/people/ulf.hansson/mmc next
config: cris-allmodconfig (attached as .config)
compiler: cris-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=cris 

All errors (new ones prefixed by >>):

   drivers/mmc/host/dw_mmc-zx: struct of_device_id is 196 bytes.  The last of 1 is:
   0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x7a 0x74 0x65 0x2c 0x64 0x77 0x2d 0x6d 0x73 0x68 0x63 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x
 00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
>> FATAL: drivers/mmc/host/dw_mmc-zx: struct of_device_id is not terminated with a NULL entry!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39944 bytes --]

[PATCH v2 3/5] Documentation: synopsys-dw-mshc: add binding for fifo quirks

[Jun Nie]

Add fifo-addr property and fifo-watermark-quirk property to
synopsys-dw-mshc bindings. It is intended to provide more
dt interface to support SoCs specific configuration.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
index 4e00e85..c8182b4 100644
--- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
+++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
@@ -76,6 +76,17 @@ Optional properties:
 
 * broken-cd: as documented in mmc core bindings.
 
+* fifo-addr: Override fifo address with value provided by DT. The default FIFO reg
+  offset is assumed as 0x100 (version < 0x240A) and 0x200(version >= 0x240A) by
+  driver. If the controller does not follow this rule, please use this property
+  to set fifo address in device tree.
+
+* fifo-watermark-quirk: Data done irq is expected if data length is less than
+  watermark in PIO mode. But fifo watermark is requested to be aligned with data
+  length in some SoC so that TX/RX irq can be generated with data done irq. Add the
+  watermark quirk to mark this requirement and force fifo watermark setting
+  accordingly.
+
 * vmmc-supply: The phandle to the regulator to use for vmmc.  If this is
   specified we'll defer probe until we can find this regulator.
 
@@ -103,6 +114,8 @@ board specific portions as listed below.
 		interrupts = <0 75 0>;
 		#address-cells = <1>;
 		#size-cells = <0>;
+		fifo-addr = <0x200>;
+		fifo-watermark-quirk;
 	};
 
 [board specific internal DMA resources]
-- 
1.9.1

[PATCH v2 4/5] mmc: dw: Add fifo address property

[Jun Nie]

The FIFO address may break default address assumption of 0x100
(version < 0x240A) and 0x200(version >= 0x240A) in current driver.
The new property is introduced to override fifo address via DT
node information.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 drivers/mmc/host/dw_mmc.c  | 5 +++++
 include/linux/mmc/dw_mmc.h | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 1c9ee36..24ae05b6 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2955,6 +2955,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
 
 	of_property_read_u32(np, "card-detect-delay", &pdata->detect_delay_ms);
 
+	of_property_read_u32(np, "fifo-addr", &host->fifo_addr_override);
+
 	if (!of_property_read_u32(np, "clock-frequency", &clock_frequency))
 		pdata->bus_hz = clock_frequency;
 
@@ -3163,6 +3165,9 @@ int dw_mci_probe(struct dw_mci *host)
 	else
 		host->fifo_reg = host->regs + DATA_240A_OFFSET;
 
+	if (host->fifo_addr_override)
+		host->fifo_reg = host->regs + host->fifo_addr_override;
+
 	tasklet_init(&host->tasklet, dw_mci_tasklet_func, (unsigned long)host);
 	ret = devm_request_irq(host->dev, host->irq, dw_mci_interrupt,
 			       host->irq_flags, "dw-mci", host);
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index f5af2bd..4866ef5 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -107,6 +107,7 @@ struct dw_mci_dma_slave {
  * @ciu_clk: Pointer to card interface unit clock instance.
  * @slot: Slots sharing this MMC controller.
  * @fifo_depth: depth of FIFO.
+ * @fifo_addr_override: override fifo reg offset with this value.
  * @data_shift: log2 of FIFO item size.
  * @part_buf_start: Start index in part_buf.
  * @part_buf_count: Bytes of partial data in part_buf.
@@ -154,6 +155,7 @@ struct dw_mci {
 	spinlock_t		irq_lock;
 	void __iomem		*regs;
 	void __iomem		*fifo_reg;
+	u32			fifo_addr_override;
 
 	struct scatterlist	*sg;
 	struct sg_mapping_iter	sg_miter;
-- 
1.9.1

Re: [PATCH v2 4/5] mmc: dw: Add fifo address property

[Jaehoon Chung]

On 10/28/2016 11:37 AM, Jun Nie wrote:
> The FIFO address may break default address assumption of 0x100
> (version < 0x240A) and 0x200(version >= 0x240A) in current driver.
> The new property is introduced to override fifo address via DT
> node information.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  drivers/mmc/host/dw_mmc.c  | 5 +++++
>  include/linux/mmc/dw_mmc.h | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 1c9ee36..24ae05b6 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2955,6 +2955,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>  
>  	of_property_read_u32(np, "card-detect-delay", &pdata->detect_delay_ms);
>  
> +	of_property_read_u32(np, "fifo-addr", &host->fifo_addr_override);
> +
>  	if (!of_property_read_u32(np, "clock-frequency", &clock_frequency))
>  		pdata->bus_hz = clock_frequency;
>  
> @@ -3163,6 +3165,9 @@ int dw_mci_probe(struct dw_mci *host)
>  	else
>  		host->fifo_reg = host->regs + DATA_240A_OFFSET;
>  
> +	if (host->fifo_addr_override)
> +		host->fifo_reg = host->regs + host->fifo_addr_override;
> +

Check condition the sequentially.

if (host->fifo_addr_override) {
...
} else if (host->verid < DW_MMC_240A) {
..
} else {
..
}

how about?

>  	tasklet_init(&host->tasklet, dw_mci_tasklet_func, (unsigned long)host);
>  	ret = devm_request_irq(host->dev, host->irq, dw_mci_interrupt,
>  			       host->irq_flags, "dw-mci", host);
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index f5af2bd..4866ef5 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -107,6 +107,7 @@ struct dw_mci_dma_slave {
>   * @ciu_clk: Pointer to card interface unit clock instance.
>   * @slot: Slots sharing this MMC controller.
>   * @fifo_depth: depth of FIFO.
> + * @fifo_addr_override: override fifo reg offset with this value.

DATA addr is more correct. it's related with DATA register.

Best Regards,
Jaehoon Chung

>   * @data_shift: log2 of FIFO item size.
>   * @part_buf_start: Start index in part_buf.
>   * @part_buf_count: Bytes of partial data in part_buf.
> @@ -154,6 +155,7 @@ struct dw_mci {
>  	spinlock_t		irq_lock;
>  	void __iomem		*regs;
>  	void __iomem		*fifo_reg;
> +	u32			fifo_addr_override;
>  
>  	struct scatterlist	*sg;
>  	struct sg_mapping_iter	sg_miter;
> 

Re: [PATCH v2 4/5] mmc: dw: Add fifo address property

[Jun Nie]

2016-10-28 13:24 GMT+08:00 Jaehoon Chung <jh80.chung@samsung.com>:
> On 10/28/2016 11:37 AM, Jun Nie wrote:
>> The FIFO address may break default address assumption of 0x100
>> (version < 0x240A) and 0x200(version >= 0x240A) in current driver.
>> The new property is introduced to override fifo address via DT
>> node information.
>>
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> ---
>>  drivers/mmc/host/dw_mmc.c  | 5 +++++
>>  include/linux/mmc/dw_mmc.h | 2 ++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 1c9ee36..24ae05b6 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -2955,6 +2955,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>>
>>       of_property_read_u32(np, "card-detect-delay", &pdata->detect_delay_ms);
>>
>> +     of_property_read_u32(np, "fifo-addr", &host->fifo_addr_override);
>> +
>>       if (!of_property_read_u32(np, "clock-frequency", &clock_frequency))
>>               pdata->bus_hz = clock_frequency;
>>
>> @@ -3163,6 +3165,9 @@ int dw_mci_probe(struct dw_mci *host)
>>       else
>>               host->fifo_reg = host->regs + DATA_240A_OFFSET;
>>
>> +     if (host->fifo_addr_override)
>> +             host->fifo_reg = host->regs + host->fifo_addr_override;
>> +
>
> Check condition the sequentially.
>
> if (host->fifo_addr_override) {
> ...
> } else if (host->verid < DW_MMC_240A) {
> ..
> } else {
> ..
> }
>
> how about?

Okay, will change to this.

>
>>       tasklet_init(&host->tasklet, dw_mci_tasklet_func, (unsigned long)host);
>>       ret = devm_request_irq(host->dev, host->irq, dw_mci_interrupt,
>>                              host->irq_flags, "dw-mci", host);
>> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
>> index f5af2bd..4866ef5 100644
>> --- a/include/linux/mmc/dw_mmc.h
>> +++ b/include/linux/mmc/dw_mmc.h
>> @@ -107,6 +107,7 @@ struct dw_mci_dma_slave {
>>   * @ciu_clk: Pointer to card interface unit clock instance.
>>   * @slot: Slots sharing this MMC controller.
>>   * @fifo_depth: depth of FIFO.
>> + * @fifo_addr_override: override fifo reg offset with this value.
>
> DATA addr is more correct. it's related with DATA register.

You name it, will change to data_addr_override.
>
> Best Regards,
> Jaehoon Chung
>
>>   * @data_shift: log2 of FIFO item size.
>>   * @part_buf_start: Start index in part_buf.
>>   * @part_buf_count: Bytes of partial data in part_buf.
>> @@ -154,6 +155,7 @@ struct dw_mci {
>>       spinlock_t              irq_lock;
>>       void __iomem            *regs;
>>       void __iomem            *fifo_reg;
>> +     u32                     fifo_addr_override;
>>
>>       struct scatterlist      *sg;
>>       struct sg_mapping_iter  sg_miter;
>>
>

Re: [PATCH v2 4/5] mmc: dw: Add fifo address property

[Jaehoon Chung]

On 10/31/2016 05:50 PM, Jun Nie wrote:
> 2016-10-28 13:24 GMT+08:00 Jaehoon Chung <jh80.chung@samsung.com>:
>> On 10/28/2016 11:37 AM, Jun Nie wrote:
>>> The FIFO address may break default address assumption of 0x100
>>> (version < 0x240A) and 0x200(version >= 0x240A) in current driver.
>>> The new property is introduced to override fifo address via DT
>>> node information.
>>>
>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>> ---
>>>  drivers/mmc/host/dw_mmc.c  | 5 +++++
>>>  include/linux/mmc/dw_mmc.h | 2 ++
>>>  2 files changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 1c9ee36..24ae05b6 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -2955,6 +2955,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>>>
>>>       of_property_read_u32(np, "card-detect-delay", &pdata->detect_delay_ms);
>>>
>>> +     of_property_read_u32(np, "fifo-addr", &host->fifo_addr_override);
>>> +
>>>       if (!of_property_read_u32(np, "clock-frequency", &clock_frequency))
>>>               pdata->bus_hz = clock_frequency;
>>>
>>> @@ -3163,6 +3165,9 @@ int dw_mci_probe(struct dw_mci *host)
>>>       else
>>>               host->fifo_reg = host->regs + DATA_240A_OFFSET;
>>>
>>> +     if (host->fifo_addr_override)
>>> +             host->fifo_reg = host->regs + host->fifo_addr_override;
>>> +
>>
>> Check condition the sequentially.
>>
>> if (host->fifo_addr_override) {
>> ...
>> } else if (host->verid < DW_MMC_240A) {
>> ..
>> } else {
>> ..
>> }
>>
>> how about?
> 
> Okay, will change to this.
> 
>>
>>>       tasklet_init(&host->tasklet, dw_mci_tasklet_func, (unsigned long)host);
>>>       ret = devm_request_irq(host->dev, host->irq, dw_mci_interrupt,
>>>                              host->irq_flags, "dw-mci", host);
>>> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
>>> index f5af2bd..4866ef5 100644
>>> --- a/include/linux/mmc/dw_mmc.h
>>> +++ b/include/linux/mmc/dw_mmc.h
>>> @@ -107,6 +107,7 @@ struct dw_mci_dma_slave {
>>>   * @ciu_clk: Pointer to card interface unit clock instance.
>>>   * @slot: Slots sharing this MMC controller.
>>>   * @fifo_depth: depth of FIFO.
>>> + * @fifo_addr_override: override fifo reg offset with this value.
>>
>> DATA addr is more correct. it's related with DATA register.
> 
> You name it, will change to data_addr_override.

Yes, data_addr or data_addr_override? It's more clear than fifo.

Best Regards,
Jaehoon Chung


>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>   * @data_shift: log2 of FIFO item size.
>>>   * @part_buf_start: Start index in part_buf.
>>>   * @part_buf_count: Bytes of partial data in part_buf.
>>> @@ -154,6 +155,7 @@ struct dw_mci {
>>>       spinlock_t              irq_lock;
>>>       void __iomem            *regs;
>>>       void __iomem            *fifo_reg;
>>> +     u32                     fifo_addr_override;
>>>
>>>       struct scatterlist      *sg;
>>>       struct sg_mapping_iter  sg_miter;
>>>
>>
> 
> 
> 

[PATCH v2 5/5] mmc: dw: Add fifo watermark quirk

[Jun Nie]

Data done irq is expected if data length is less than
watermark in PIO mode. But fifo watermark is requested
to be aligned with data length in some SoC so that TX/RX
irq can be generated with data done irq. Add the
watermark quirk to mark this requirement and force
fifo watermark setting accordingly.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 drivers/mmc/host/dw_mmc.c  | 11 +++++++++--
 include/linux/mmc/dw_mmc.h |  2 ++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 24ae05b6..e0f49cc 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1111,11 +1111,15 @@ static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data)
 		mci_writel(host, CTRL, temp);
 
 		/*
-		 * Use the initial fifoth_val for PIO mode.
+		 * Use the initial fifoth_val for PIO mode. If wm_quirk
+		 * is set, we set watermark same as data size.
 		 * If next issued data may be transfered by DMA mode,
 		 * prev_blksz should be invalidated.
 		 */
-		mci_writel(host, FIFOTH, host->fifoth_val);
+		if (host->wm_quirk)
+			dw_mci_adjust_fifoth(host, data);
+		else
+			mci_writel(host, FIFOTH, host->fifoth_val);
 		host->prev_blksz = 0;
 	} else {
 		/*
@@ -2957,6 +2961,9 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
 
 	of_property_read_u32(np, "fifo-addr", &host->fifo_addr_override);
 
+	if (of_get_property(np, "fifo-watermark-quirk", NULL))
+		host->wm_quirk = true;
+
 	if (!of_property_read_u32(np, "clock-frequency", &clock_frequency))
 		pdata->bus_hz = clock_frequency;
 
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 4866ef5..2ccfd9c 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -108,6 +108,7 @@ struct dw_mci_dma_slave {
  * @slot: Slots sharing this MMC controller.
  * @fifo_depth: depth of FIFO.
  * @fifo_addr_override: override fifo reg offset with this value.
+ * @wm_quirk: force fifo watermark equal with data length in PIO mode.
  * @data_shift: log2 of FIFO item size.
  * @part_buf_start: Start index in part_buf.
  * @part_buf_count: Bytes of partial data in part_buf.
@@ -156,6 +157,7 @@ struct dw_mci {
 	void __iomem		*regs;
 	void __iomem		*fifo_reg;
 	u32			fifo_addr_override;
+	u32			wm_quirk;
 
 	struct scatterlist	*sg;
 	struct sg_mapping_iter	sg_miter;
-- 
1.9.1

Re: [PATCH v2 5/5] mmc: dw: Add fifo watermark quirk

[Jaehoon Chung]

On 10/28/2016 11:37 AM, Jun Nie wrote:
> Data done irq is expected if data length is less than
> watermark in PIO mode. But fifo watermark is requested
> to be aligned with data length in some SoC so that TX/RX
> irq can be generated with data done irq. Add the
> watermark quirk to mark this requirement and force
> fifo watermark setting accordingly.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  drivers/mmc/host/dw_mmc.c  | 11 +++++++++--
>  include/linux/mmc/dw_mmc.h |  2 ++
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 24ae05b6..e0f49cc 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1111,11 +1111,15 @@ static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data)
>  		mci_writel(host, CTRL, temp);
>  
>  		/*
> -		 * Use the initial fifoth_val for PIO mode.
> +		 * Use the initial fifoth_val for PIO mode. If wm_quirk
> +		 * is set, we set watermark same as data size.
>  		 * If next issued data may be transfered by DMA mode,
>  		 * prev_blksz should be invalidated.
>  		 */
> -		mci_writel(host, FIFOTH, host->fifoth_val);
> +		if (host->wm_quirk)
> +			dw_mci_adjust_fifoth(host, data);
> +		else
> +			mci_writel(host, FIFOTH, host->fifoth_val);
>  		host->prev_blksz = 0;
>  	} else {
>  		/*
> @@ -2957,6 +2961,9 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>  
>  	of_property_read_u32(np, "fifo-addr", &host->fifo_addr_override);
>  
> +	if (of_get_property(np, "fifo-watermark-quirk", NULL))
> +		host->wm_quirk = true;
> +
>  	if (!of_property_read_u32(np, "clock-frequency", &clock_frequency))
>  		pdata->bus_hz = clock_frequency;
>  
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index 4866ef5..2ccfd9c 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -108,6 +108,7 @@ struct dw_mci_dma_slave {
>   * @slot: Slots sharing this MMC controller.
>   * @fifo_depth: depth of FIFO.
>   * @fifo_addr_override: override fifo reg offset with this value.
> + * @wm_quirk: force fifo watermark equal with data length in PIO mode.

quirk...hmm..It might be just my preference..
quirk looks like workaround...so how about changing other name?

>   * @data_shift: log2 of FIFO item size.
>   * @part_buf_start: Start index in part_buf.
>   * @part_buf_count: Bytes of partial data in part_buf.
> @@ -156,6 +157,7 @@ struct dw_mci {
>  	void __iomem		*regs;
>  	void __iomem		*fifo_reg;
>  	u32			fifo_addr_override;
> +	u32			wm_quirk;

For True or false, u32?

>  
>  	struct scatterlist	*sg;
>  	struct sg_mapping_iter	sg_miter;
> 

Re: [PATCH v2 5/5] mmc: dw: Add fifo watermark quirk

[Jun Nie]

2016-10-28 13:30 GMT+08:00 Jaehoon Chung <jh80.chung@samsung.com>:
> On 10/28/2016 11:37 AM, Jun Nie wrote:
>> Data done irq is expected if data length is less than
>> watermark in PIO mode. But fifo watermark is requested
>> to be aligned with data length in some SoC so that TX/RX
>> irq can be generated with data done irq. Add the
>> watermark quirk to mark this requirement and force
>> fifo watermark setting accordingly.
>>
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> ---
>>  drivers/mmc/host/dw_mmc.c  | 11 +++++++++--
>>  include/linux/mmc/dw_mmc.h |  2 ++
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 24ae05b6..e0f49cc 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1111,11 +1111,15 @@ static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data)
>>               mci_writel(host, CTRL, temp);
>>
>>               /*
>> -              * Use the initial fifoth_val for PIO mode.
>> +              * Use the initial fifoth_val for PIO mode. If wm_quirk
>> +              * is set, we set watermark same as data size.
>>                * If next issued data may be transfered by DMA mode,
>>                * prev_blksz should be invalidated.
>>                */
>> -             mci_writel(host, FIFOTH, host->fifoth_val);
>> +             if (host->wm_quirk)
>> +                     dw_mci_adjust_fifoth(host, data);
>> +             else
>> +                     mci_writel(host, FIFOTH, host->fifoth_val);
>>               host->prev_blksz = 0;
>>       } else {
>>               /*
>> @@ -2957,6 +2961,9 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>>
>>       of_property_read_u32(np, "fifo-addr", &host->fifo_addr_override);
>>
>> +     if (of_get_property(np, "fifo-watermark-quirk", NULL))
>> +             host->wm_quirk = true;
>> +
>>       if (!of_property_read_u32(np, "clock-frequency", &clock_frequency))
>>               pdata->bus_hz = clock_frequency;
>>
>> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
>> index 4866ef5..2ccfd9c 100644
>> --- a/include/linux/mmc/dw_mmc.h
>> +++ b/include/linux/mmc/dw_mmc.h
>> @@ -108,6 +108,7 @@ struct dw_mci_dma_slave {
>>   * @slot: Slots sharing this MMC controller.
>>   * @fifo_depth: depth of FIFO.
>>   * @fifo_addr_override: override fifo reg offset with this value.
>> + * @wm_quirk: force fifo watermark equal with data length in PIO mode.
>
> quirk...hmm..It might be just my preference..
> quirk looks like workaround...so how about changing other name?

How about fifo_wm_aligned ?

>
>>   * @data_shift: log2 of FIFO item size.
>>   * @part_buf_start: Start index in part_buf.
>>   * @part_buf_count: Bytes of partial data in part_buf.
>> @@ -156,6 +157,7 @@ struct dw_mci {
>>       void __iomem            *regs;
>>       void __iomem            *fifo_reg;
>>       u32                     fifo_addr_override;
>> +     u32                     wm_quirk;
>
> For True or false, u32?
True means alignment is needed. Will add  this to comments.
>
>>
>>       struct scatterlist      *sg;
>>       struct sg_mapping_iter  sg_miter;
>>
>

Re: [PATCH v2 5/5] mmc: dw: Add fifo watermark quirk

[Jaehoon Chung]

On 10/31/2016 05:48 PM, Jun Nie wrote:
> 2016-10-28 13:30 GMT+08:00 Jaehoon Chung <jh80.chung@samsung.com>:
>> On 10/28/2016 11:37 AM, Jun Nie wrote:
>>> Data done irq is expected if data length is less than
>>> watermark in PIO mode. But fifo watermark is requested
>>> to be aligned with data length in some SoC so that TX/RX
>>> irq can be generated with data done irq. Add the
>>> watermark quirk to mark this requirement and force
>>> fifo watermark setting accordingly.
>>>
>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>> ---
>>>  drivers/mmc/host/dw_mmc.c  | 11 +++++++++--
>>>  include/linux/mmc/dw_mmc.h |  2 ++
>>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 24ae05b6..e0f49cc 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -1111,11 +1111,15 @@ static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data)
>>>               mci_writel(host, CTRL, temp);
>>>
>>>               /*
>>> -              * Use the initial fifoth_val for PIO mode.
>>> +              * Use the initial fifoth_val for PIO mode. If wm_quirk
>>> +              * is set, we set watermark same as data size.
>>>                * If next issued data may be transfered by DMA mode,
>>>                * prev_blksz should be invalidated.
>>>                */
>>> -             mci_writel(host, FIFOTH, host->fifoth_val);
>>> +             if (host->wm_quirk)
>>> +                     dw_mci_adjust_fifoth(host, data);
>>> +             else
>>> +                     mci_writel(host, FIFOTH, host->fifoth_val);
>>>               host->prev_blksz = 0;
>>>       } else {
>>>               /*
>>> @@ -2957,6 +2961,9 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>>>
>>>       of_property_read_u32(np, "fifo-addr", &host->fifo_addr_override);
>>>
>>> +     if (of_get_property(np, "fifo-watermark-quirk", NULL))
>>> +             host->wm_quirk = true;
>>> +
>>>       if (!of_property_read_u32(np, "clock-frequency", &clock_frequency))
>>>               pdata->bus_hz = clock_frequency;
>>>
>>> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
>>> index 4866ef5..2ccfd9c 100644
>>> --- a/include/linux/mmc/dw_mmc.h
>>> +++ b/include/linux/mmc/dw_mmc.h
>>> @@ -108,6 +108,7 @@ struct dw_mci_dma_slave {
>>>   * @slot: Slots sharing this MMC controller.
>>>   * @fifo_depth: depth of FIFO.
>>>   * @fifo_addr_override: override fifo reg offset with this value.
>>> + * @wm_quirk: force fifo watermark equal with data length in PIO mode.
>>
>> quirk...hmm..It might be just my preference..
>> quirk looks like workaround...so how about changing other name?
> 
> How about fifo_wm_aligned ?

If it's not quirk or workaround, whatever. :)

Best Regards,
Jaehoon Chung

> 
>>
>>>   * @data_shift: log2 of FIFO item size.
>>>   * @part_buf_start: Start index in part_buf.
>>>   * @part_buf_count: Bytes of partial data in part_buf.
>>> @@ -156,6 +157,7 @@ struct dw_mci {
>>>       void __iomem            *regs;
>>>       void __iomem            *fifo_reg;
>>>       u32                     fifo_addr_override;
>>> +     u32                     wm_quirk;
>>
>> For True or false, u32?
> True means alignment is needed. Will add  this to comments.
>>
>>>
>>>       struct scatterlist      *sg;
>>>       struct sg_mapping_iter  sg_miter;
>>>
>>
> --
> 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
> 
> 
>