[PATCH 2/8] mmc: sdhci: host: add new f_sdh30

[PATCH 2/8] mmc: sdhci: host: add new f_sdh30

[Mollie Wu]

This patch adds new host controller driver for
Fujitsu SDHCI controller f_sdh30.

Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof <olof@lixom.net>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Tetsuya Takinishi <t.takinishi@jp.fujitsu.com>
Signed-off-by: Mollie Wu <mollie.wu@linaro.org>
---
 .../devicetree/bindings/mmc/sdhci-fujitsu.txt      |  36 ++
 drivers/mmc/host/Kconfig                           |   7 +
 drivers/mmc/host/Makefile                          |   1 +
 drivers/mmc/host/sdhci_f_sdh30.c                   | 380 +++++++++++++++++++++
 4 files changed, 424 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
 create mode 100644 drivers/mmc/host/sdhci_f_sdh30.c

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
new file mode 100644
index 0000000..34f20c9
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
@@ -0,0 +1,36 @@
+* Fujitsu SDHCI controller
+
+This file documents differences between the core properties in mmc.txt
+and the properties used by the sdhci_f_sdh30 driver.
+
+Required properties:
+- compatible: "fujitsu,f-sdh30"
+- voltage-ranges : This is a list of pairs. In each pair, two cells
+  are required. First cell specifies minimum slot voltage (mV), second
+  cell specifies maximum slot voltage (mV). In case of supported voltage
+  range is discontinuous, several ranges could be specified as a list.
+
+Optional properties:
+- pwr-mux-gpios: This is one optional gpio for controlling a power mux
+  which switches between two power supplies. 3.3V is selected when gpio
+  is high, and 1.8V is selected when gpio is low. This voltage is used
+  for signal level.
+- clocks: Must contain an entry for each entry in clock-names. It is a
+  list of phandles and clock-specifier pairs.
+  See ../clocks/clock-bindings.txt for details.
+- clock-names: Should contain the following two entries:
+	"sd_sd4clk" - clock primarily used for tuning process
+	"sd_bclk"   - base clock for sdhci controller
+
+Example:
+
+	sdhci1: sdio@36600000 {
+		compatible = "fujitsu,f-sdh30";
+		reg = <0 0x36600000 0x1000>;
+		interrupts = <0 172 0x4>,
+			     <0 173 0x4>;
+		voltage-ranges = <1800 1800>, <3300 3300>;
+		pwr-mux-gpios = <&gpio0 7 0>;
+		clocks = <&clk_hdmi_2_0>, <&clk_hdmi_3_0>;
+		clock-names = "sd_sd4clk", "sd_bclk";
+	};
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index a565254..1d5975c 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -279,6 +279,13 @@ config MMC_SDHCI_BCM2835
 	  This selects the BCM2835 SD/MMC controller. If you have a BCM2835
 	  platform with SD or MMC devices, say Y or M here.
 
+config MMC_SDHCI_F_SDH30
+	tristate "SDHCI support for Fujitsu Semiconductor F_SDH30"
+	depends on MMC_SDHCI && ARCH_MB86S7X
+	help
+	  This selects the Secure Digital Host Controller Interface (SDHCI)
+	  Needed by some Fujitsu SoC for MMC / SD / SDIO support.
+	  If you have a controller with this interface, say Y or M here.
 	  If unsure, say N.
 
 config MMC_MOXART
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 7f81ddf..a4c89e5 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_MMC_SDHCI_PXAV3)	+= sdhci-pxav3.o
 obj-$(CONFIG_MMC_SDHCI_PXAV2)	+= sdhci-pxav2.o
 obj-$(CONFIG_MMC_SDHCI_S3C)	+= sdhci-s3c.o
 obj-$(CONFIG_MMC_SDHCI_SIRF)   	+= sdhci-sirf.o
+obj-$(CONFIG_MMC_SDHCI_F_SDH30)	+= sdhci_f_sdh30.o
 obj-$(CONFIG_MMC_SDHCI_SPEAR)	+= sdhci-spear.o
 obj-$(CONFIG_MMC_WBSD)		+= wbsd.o
 obj-$(CONFIG_MMC_AU1X)		+= au1xmmc.o
diff --git a/drivers/mmc/host/sdhci_f_sdh30.c b/drivers/mmc/host/sdhci_f_sdh30.c
new file mode 100644
index 0000000..8d23f2d
--- /dev/null
+++ b/drivers/mmc/host/sdhci_f_sdh30.c
@@ -0,0 +1,380 @@
+/*
+ * linux/drivers/mmc/host/sdhci_f_sdh30.c
+ *
+ * Copyright (C) 2013 - 2014 Fujitsu Semiconductor, Ltd
+ *              Vincent Yang <vincent.yang@tw.fujitsu.com>
+ * Copyright (C) 2014 Linaro Ltd  Andy Green <andy.green@linaro.org>
+ *
+ * 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, version 2 of the License.
+ */
+
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/mmc/sd.h>
+#include <linux/mmc/host.h>
+#include <linux/mmc/card.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/suspend.h>
+
+#include "sdhci.h"
+#include "sdhci-pltfm.h"
+#include "../core/core.h"
+
+#define DRIVER_NAME "f_sdh30"
+
+/* F_SDH30 extended Controller registers */
+#define F_SDH30_AHB_CONFIG		0x100
+#define  F_SDH30_AHB_BIGED		0x00000040
+#define  F_SDH30_BUSLOCK_DMA		0x00000020
+#define  F_SDH30_BUSLOCK_EN		0x00000010
+#define  F_SDH30_SIN			0x00000008
+#define  F_SDH30_AHB_INCR_16		0x00000004
+#define  F_SDH30_AHB_INCR_8		0x00000002
+#define  F_SDH30_AHB_INCR_4		0x00000001
+
+#define F_SDH30_TUNING_SETTING		0x108
+#define  F_SDH30_CMD_CHK_DIS		0x00010000
+
+#define F_SDH30_IO_CONTROL2		0x114
+#define  F_SDH30_CRES_O_DN		0x00080000
+#define  F_SDH30_MSEL_O_1_8		0x00040000
+
+#define F_SDH30_ESD_CONTROL		0x124
+#define  F_SDH30_EMMC_RST		0x00000002
+#define  F_SDH30_EMMC_HS200		0x01000000
+
+#define F_SDH30_CMD_DAT_DELAY		0x200
+
+#define F_SDH30_MIN_CLOCK		400000
+
+struct f_sdhost_priv {
+	struct clk *clk_sd4;
+	struct clk *clk_b;
+	int gpio_select_1v8;
+	u32 vendor_hs200;
+	struct device *dev;
+};
+
+void sdhci_f_sdh30_soft_voltage_switch(struct sdhci_host *host)
+{
+	struct f_sdhost_priv *priv = sdhci_priv(host);
+	u32 ctrl = 0;
+
+	usleep_range(2500, 3000);
+	ctrl = sdhci_readl(host, F_SDH30_IO_CONTROL2);
+	ctrl |= F_SDH30_CRES_O_DN;
+	sdhci_writel(host, ctrl, F_SDH30_IO_CONTROL2);
+	ctrl |= F_SDH30_MSEL_O_1_8;
+	sdhci_writel(host, ctrl, F_SDH30_IO_CONTROL2);
+
+	if (gpio_is_valid(priv->gpio_select_1v8)) {
+		dev_info(priv->dev, "%s: setting gpio\n", __func__);
+		gpio_direction_output(priv->gpio_select_1v8, 0);
+	}
+
+	ctrl &= ~F_SDH30_CRES_O_DN;
+	sdhci_writel(host, ctrl, F_SDH30_IO_CONTROL2);
+	usleep_range(2500, 3000);
+
+	if (priv->vendor_hs200) {
+		dev_info(priv->dev, "%s: setting hs200\n", __func__);
+		ctrl = sdhci_readl(host, F_SDH30_ESD_CONTROL);
+		ctrl |= priv->vendor_hs200;
+		sdhci_writel(host, ctrl, F_SDH30_ESD_CONTROL);
+	}
+
+	ctrl = sdhci_readl(host, F_SDH30_TUNING_SETTING);
+	ctrl |= F_SDH30_CMD_CHK_DIS;
+	sdhci_writel(host, ctrl, F_SDH30_TUNING_SETTING);
+}
+
+unsigned int sdhci_f_sdh30_get_min_clock(struct sdhci_host *host)
+{
+	return F_SDH30_MIN_CLOCK;
+}
+
+void sdhci_f_sdh30_reset(struct sdhci_host *host, u8 mask)
+{
+	struct f_sdhost_priv *priv = sdhci_priv(host);
+
+	if (gpio_is_valid(priv->gpio_select_1v8))
+		gpio_direction_output(priv->gpio_select_1v8, 1);
+
+	if (sdhci_readw(host, SDHCI_CLOCK_CONTROL) == 0) {
+		sdhci_writew(host, 0xBC01, SDHCI_CLOCK_CONTROL);
+		mmiowb();
+	}
+
+	sdhci_reset(host, mask);
+}
+
+static const struct sdhci_ops sdhci_f_sdh30_ops = {
+	.voltage_switch = sdhci_f_sdh30_soft_voltage_switch,
+	.get_min_clock = sdhci_f_sdh30_get_min_clock,
+	.reset = sdhci_f_sdh30_reset,
+	.set_clock = sdhci_set_clock,
+	.set_bus_width = sdhci_set_bus_width,
+	.set_uhs_signaling = sdhci_set_uhs_signaling,
+};
+
+static int sdhci_f_sdh30_probe(struct platform_device *pdev)
+{
+	struct sdhci_host *host;
+	struct device *dev = &pdev->dev;
+	int irq, ctrl = 0, ret = 0;
+	struct f_sdhost_priv *priv;
+	u32 reg = 0, bus_width;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "%s: no irq specified\n", __func__);
+		ret = irq;
+		goto err;
+	}
+
+	host = sdhci_alloc_host(dev, sizeof(struct sdhci_host) +
+						sizeof(struct f_sdhost_priv));
+	if (IS_ERR(host)) {
+		dev_err(dev, "%s: host allocate error\n", __func__);
+		ret = -ENOMEM;
+		goto err;
+	}
+	priv = sdhci_priv(host);
+	priv->dev = dev;
+
+	host->quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC |
+		       SDHCI_QUIRK_INVERTED_WRITE_PROTECT;
+	host->quirks2 = SDHCI_QUIRK2_SUPPORT_SINGLE |
+			SDHCI_QUIRK2_VOLTAGE_SWITCH |
+			SDHCI_QUIRK2_TUNING_WORK_AROUND;
+
+	if (!of_property_read_u32(pdev->dev.of_node, "bus-width", &bus_width)) {
+		switch (bus_width) {
+		case 8:
+			dev_info(dev, "Applying 8 bit bus width\n");
+			host->mmc->caps |= MMC_CAP_8_BIT_DATA;
+			break;
+		case 4:
+			dev_info(dev, "Applying 4 bit bus width\n");
+			host->mmc->caps |= MMC_CAP_4_BIT_DATA;
+			break;
+		case 1:
+		default:
+			dev_err(dev, "Invalid bus width: %u\n", bus_width);
+			break;
+		}
+	}
+
+	ret = mmc_of_parse_voltage(pdev->dev.of_node, &host->ocr_mask);
+	if (ret) {
+		dev_err(dev, "%s: parse voltage error\n", __func__);
+		goto err_voltage;
+	}
+
+	host->hw_name = DRIVER_NAME;
+	host->ops = &sdhci_f_sdh30_ops;
+	host->irq = irq;
+
+	host->ioaddr = of_iomap(pdev->dev.of_node, 0);
+	if (!host->ioaddr) {
+		dev_err(dev, "%s: failed to remap registers\n", __func__);
+		ret = -ENXIO;
+		goto err_remap;
+	}
+
+	priv->clk_sd4 = clk_get(&pdev->dev, "sd_sd4clk");
+	if (!IS_ERR(priv->clk_sd4)) {
+		ret = clk_prepare_enable(priv->clk_sd4);
+		if (ret < 0) {
+			dev_err(dev, "Failed to enable sd4 clock: %d\n", ret);
+			goto err_clk1;
+		}
+	}
+	priv->clk_b = clk_get(&pdev->dev, "sd_bclk");
+	if (!IS_ERR(priv->clk_b)) {
+		ret = clk_prepare_enable(priv->clk_b);
+		if (ret < 0) {
+			dev_err(dev, "Failed to enable clk_b clock: %d\n", ret);
+			goto err_clk2;
+		}
+	}
+
+	/* optional */
+	priv->gpio_select_1v8 = of_get_named_gpio(pdev->dev.of_node,
+						  "pwr-mux-gpios", 0);
+	if (gpio_is_valid(priv->gpio_select_1v8)) {
+		ret = gpio_request(priv->gpio_select_1v8, "sdhci");
+		if (unlikely(ret)) {
+			dev_err(dev, " gpio %d request failed ",
+				priv->gpio_select_1v8);
+			goto err_gpio;
+		}
+		/* initially 3.3V */
+		ret = gpio_direction_output(priv->gpio_select_1v8, 1);
+		if (unlikely(ret)) {
+			dev_err(dev, "failed to set phy_enable gpio\n");
+			goto err_gpio;
+		}
+	}
+
+	platform_set_drvdata(pdev, host);
+
+#ifdef CONFIG_PM_RUNTIME
+	pm_runtime_enable(&pdev->dev);
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0)
+		dev_err(dev, "Failed to pm_runtime_get_sync: %d\n", ret);
+#endif
+
+	ret = sdhci_add_host(host);
+	if (ret) {
+		dev_err(dev, "%s: host add error\n", __func__);
+		goto err_add_host;
+	}
+
+	/* init vendor specific regs */
+	ctrl = sdhci_readw(host, F_SDH30_AHB_CONFIG);
+	ctrl |= F_SDH30_SIN | F_SDH30_AHB_INCR_16 | F_SDH30_AHB_INCR_8 |
+		F_SDH30_AHB_INCR_4;
+	ctrl &= ~(F_SDH30_AHB_BIGED | F_SDH30_BUSLOCK_EN);
+	sdhci_writew(host, ctrl, F_SDH30_AHB_CONFIG);
+	mmiowb();
+
+	reg = sdhci_readl(host, F_SDH30_ESD_CONTROL);
+	sdhci_writel(host, reg & ~F_SDH30_EMMC_RST, F_SDH30_ESD_CONTROL);
+	msleep(20);
+	sdhci_writel(host, reg | F_SDH30_EMMC_RST, F_SDH30_ESD_CONTROL);
+	mmiowb();
+
+	reg = sdhci_readl(host, SDHCI_CAPABILITIES);
+	if (reg & SDHCI_CAN_DO_8BIT)
+		priv->vendor_hs200 = F_SDH30_EMMC_HS200;
+	else
+		priv->vendor_hs200 = 0;
+
+	return 0;
+
+err_add_host:
+	if (gpio_is_valid(priv->gpio_select_1v8)) {
+		gpio_direction_output(priv->gpio_select_1v8, 1);
+		gpio_free(priv->gpio_select_1v8);
+	}
+err_gpio:
+	clk_put(priv->clk_b);
+err_clk2:
+	clk_put(priv->clk_sd4);
+err_clk1:
+	iounmap(host->ioaddr);
+err_remap:
+err_voltage:
+	sdhci_free_host(host);
+err:
+	return ret;
+}
+
+static int sdhci_f_sdh30_remove(struct platform_device *pdev)
+{
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+	struct f_sdhost_priv *priv = sdhci_priv(host);
+	struct resource *iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	sdhci_remove_host(host, readl(host->ioaddr + SDHCI_INT_STATUS) ==
+			  0xffffffff);
+	iounmap(host->ioaddr);
+	release_mem_region(iomem->start, resource_size(iomem));
+
+	clk_disable_unprepare(priv->clk_sd4);
+	clk_disable_unprepare(priv->clk_b);
+
+	clk_put(priv->clk_b);
+	clk_put(priv->clk_sd4);
+
+	if (gpio_is_valid(priv->gpio_select_1v8)) {
+		gpio_direction_output(priv->gpio_select_1v8, 1);
+		gpio_free(priv->gpio_select_1v8);
+	}
+
+	sdhci_free_host(host);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int sdhci_f_sdh30_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+
+	return sdhci_suspend_host(host);
+}
+
+static int sdhci_f_sdh30_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+
+	return sdhci_resume_host(host);
+}
+#endif
+
+#ifdef CONFIG_PM_RUNTIME
+static int sdhci_f_sdh30_runtime_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+
+	return sdhci_runtime_suspend_host(host);
+}
+
+static int sdhci_f_sdh30_runtime_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+
+	return sdhci_runtime_resume_host(host);
+}
+#endif
+
+#ifdef CONFIG_PM
+static const struct dev_pm_ops sdhci_f_sdh30_pmops = {
+	SET_SYSTEM_SLEEP_PM_OPS(sdhci_f_sdh30_suspend, sdhci_f_sdh30_resume)
+	SET_RUNTIME_PM_OPS(sdhci_f_sdh30_runtime_suspend,
+			   sdhci_f_sdh30_runtime_resume, NULL)
+};
+
+#define SDHCI_F_SDH30_PMOPS (&sdhci_f_sdh30_pmops)
+
+#else
+#define SDHCI_F_SDH30_PMOPS NULL
+#endif
+
+static const struct of_device_id f_sdh30_dt_ids[] = {
+	{ .compatible = "fujitsu,f-sdh30" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, f_sdh30_dt_ids);
+
+static struct platform_driver sdhci_f_sdh30_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table = f_sdh30_dt_ids,
+#ifdef CONFIG_PM_SLEEP
+		.pm	= SDHCI_F_SDH30_PMOPS,
+#endif
+	},
+	.probe	= sdhci_f_sdh30_probe,
+	.remove	= sdhci_f_sdh30_remove,
+};
+
+module_platform_driver(sdhci_f_sdh30_driver);
+
+MODULE_DESCRIPTION("F_SDH30 SD Card Controller driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("FUJITSU SEMICONDUCTOR LTD.");
+MODULE_ALIAS("platform: " DRIVER_NAME);
-- 
1.8.1.2

[PATCH 3/8] mmc: core: add manual resume capability

[Mollie Wu]

This patch adds manual resume for some embedded platforms with rootfs
stored in SD card. It references CONFIG_MMC_BLOCK_DEFERRED_RESUME in
kernel 3.10. It lets host controller driver to manually handle resume
by itself.

[symptom]
This issue is found on mb86s7x platforms with rootfs stored in SD card.
It failed to resume form STR suspend mode because SD card cannot be ready
in time. It take longer time (e.g., 600ms) to be ready for access.
The error log looks like below:

root@localhost:~# echo mem > /sys/power/state
[   30.441974] SUSPEND

SCB Firmware : Category 01 Version 02.03 Rev. 00_
    Config   : (no configuration)
root@localhost:~# [   30.702976] Buffer I/O error on device mmcblk1p2, logical block 31349
[   30.709678] Buffer I/O error on device mmcblk1p2, logical block 168073
[   30.716220] Buffer I/O error on device mmcblk1p2, logical block 168074
[   30.722759] Buffer I/O error on device mmcblk1p2, logical block 168075
[   30.729456] Buffer I/O error on device mmcblk1p2, logical block 31349
[   30.735916] Buffer I/O error on device mmcblk1p2, logical block 31350
[   30.742370] Buffer I/O error on device mmcblk1p2, logical block 31351
[   30.749025] Buffer I/O error on device mmcblk1p2, logical block 168075
[   30.755657] Buffer I/O error on device mmcblk1p2, logical block 31351
[   30.763130] Aborting journal on device mmcblk1p2-8.
[   30.768060] JBD2: Error -5 detected when updating journal superblock for mmcblk1p2-8.
[   30.776085] EXT4-fs error (device mmcblk1p2): ext4_journal_check_start:56: Detected aborted journal
[   30.785259] EXT4-fs (mmcblk1p2): Remounting filesystem read-only
[   31.370716] EXT4-fs error (device mmcblk1p2): ext4_find_entry:1309: inode #2490369: comm udevd: reading directory lblock 0
[   31.382485] EXT4-fs error (device mmcblk1p2): ext4_find_entry:1309: inode #1048577: comm udevd: reading directory lblock 0

[analysis]
In system resume path, mmc_sd_resume() is failed with error code -123
because at that time SD card is still not ready on mb86s7x platforms.

[solution]
In order to not blocking system resume path, this patch just sets a flag
MMC_BUSRESUME_MANUAL_RESUME when this error happened, and then host controller
driver can understand it by this flag. Then host controller driver have to
resume SD card manually and asynchronously.

Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof <olof@lixom.net>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Tetsuya Takinishi <t.takinishi@jp.fujitsu.com>
Signed-off-by: Mollie Wu <mollie.wu@linaro.org>
---
 drivers/mmc/core/core.c          |  4 ++
 drivers/mmc/core/sd.c            |  4 ++
 drivers/mmc/host/sdhci_f_sdh30.c | 89 ++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h         | 14 +++++++
 4 files changed, 111 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 764af63..51fce49 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2648,6 +2648,10 @@ int mmc_pm_notify(struct notifier_block *notify_block,
 	case PM_POST_RESTORE:
 
 		spin_lock_irqsave(&host->lock, flags);
+		if (mmc_bus_manual_resume(host)) {
+			spin_unlock_irqrestore(&host->lock, flags);
+			break;
+		}
 		host->rescan_disable = 0;
 		spin_unlock_irqrestore(&host->lock, flags);
 		_mmc_detect_change(host, 0, false);
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 0c44510..859390d 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1133,6 +1133,10 @@ static int mmc_sd_resume(struct mmc_host *host)
 
 	if (!(host->caps & MMC_CAP_RUNTIME_RESUME)) {
 		err = _mmc_sd_resume(host);
+		if ((host->caps2 & MMC_CAP2_MANUAL_RESUME) && err)
+			mmc_set_bus_resume_policy(host, 1);
+		else
+			mmc_set_bus_resume_policy(host, 0);
 		pm_runtime_set_active(&host->card->dev);
 		pm_runtime_mark_last_busy(&host->card->dev);
 	}
diff --git a/drivers/mmc/host/sdhci_f_sdh30.c b/drivers/mmc/host/sdhci_f_sdh30.c
index 8d23f2d..32864ba 100644
--- a/drivers/mmc/host/sdhci_f_sdh30.c
+++ b/drivers/mmc/host/sdhci_f_sdh30.c
@@ -29,6 +29,12 @@
 #include "../core/core.h"
 
 #define DRIVER_NAME "f_sdh30"
+#define RESUME_WAIT_COUNT	100
+#define RESUME_WAIT_TIME	50
+#define RESUME_WAIT_JIFFIES	msecs_to_jiffies(RESUME_DETECT_TIME)
+#define RESUME_DETECT_COUNT	16
+#define RESUME_DETECT_TIME	50
+#define RESUME_DETECT_JIFFIES	msecs_to_jiffies(RESUME_DETECT_TIME)
 
 /* F_SDH30 extended Controller registers */
 #define F_SDH30_AHB_CONFIG		0x100
@@ -61,8 +67,59 @@ struct f_sdhost_priv {
 	int gpio_select_1v8;
 	u32 vendor_hs200;
 	struct device *dev;
+	unsigned int quirks;	/* Deviations from spec. */
+
+/* retry to detect mmc device when resume */
+#define F_SDH30_QUIRK_RESUME_DETECT_RETRY		(1<<0)
+
+	struct workqueue_struct *resume_detect_wq;
+	struct delayed_work resume_detect_work;
+	unsigned int		resume_detect_count;
+	unsigned int		resume_wait_count;
 };
 
+static void sdhci_f_sdh30_resume_detect_work_func(struct work_struct *work)
+{
+	struct f_sdhost_priv *priv = container_of(work, struct f_sdhost_priv,
+		resume_detect_work.work);
+	struct sdhci_host *host = dev_get_drvdata(priv->dev);
+	int err = 0;
+
+	if (mmc_bus_manual_resume(host->mmc)) {
+		pm_runtime_disable(&host->mmc->card->dev);
+		mmc_card_set_suspended(host->mmc->card);
+		err = host->mmc->bus_ops->resume(host->mmc);
+		if (priv->resume_detect_count-- && err)
+			queue_delayed_work(priv->resume_detect_wq,
+					   &priv->resume_detect_work,
+				RESUME_DETECT_JIFFIES);
+		else
+			pr_debug("%s: resume detection done (count:%d, wait:%d, err:%d)\n",
+				 mmc_hostname(host->mmc),
+				 priv->resume_detect_count,
+				 priv->resume_wait_count, err);
+	} else {
+		if (priv->resume_wait_count--)
+			queue_delayed_work(priv->resume_detect_wq,
+					   &priv->resume_detect_work,
+				RESUME_WAIT_JIFFIES);
+		else
+			pr_debug("%s: resume done\n", mmc_hostname(host->mmc));
+	}
+}
+
+static void sdhci_f_sdh30_resume_detect(struct mmc_host *mmc,
+					int detect, int wait)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	struct f_sdhost_priv *priv = sdhci_priv(host);
+
+	priv->resume_detect_count = detect;
+	priv->resume_wait_count = wait;
+	queue_delayed_work(priv->resume_detect_wq,
+			   &priv->resume_detect_work, 0);
+}
+
 void sdhci_f_sdh30_soft_voltage_switch(struct sdhci_host *host)
 {
 	struct f_sdhost_priv *priv = sdhci_priv(host);
@@ -173,6 +230,12 @@ static int sdhci_f_sdh30_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (of_find_property(pdev->dev.of_node, "resume-detect-retry", NULL)) {
+		dev_info(dev, "Applying resume detect retry quirk\n");
+		priv->quirks |= F_SDH30_QUIRK_RESUME_DETECT_RETRY;
+		host->mmc->caps2 |= MMC_CAP2_MANUAL_RESUME;
+	}
+
 	ret = mmc_of_parse_voltage(pdev->dev.of_node, &host->ocr_mask);
 	if (ret) {
 		dev_err(dev, "%s: parse voltage error\n", __func__);
@@ -225,6 +288,18 @@ static int sdhci_f_sdh30_probe(struct platform_device *pdev)
 		}
 	}
 
+	/* Init workqueue */
+	if (priv->quirks & F_SDH30_QUIRK_RESUME_DETECT_RETRY) {
+		priv->resume_detect_wq = create_workqueue("sdhci_f_sdh30");
+		if (priv->resume_detect_wq == NULL) {
+			ret = -ENOMEM;
+			dev_err(dev, "Failed to create resume detection workqueue\n");
+			goto err_init_wq;
+		}
+		INIT_DELAYED_WORK(&priv->resume_detect_work,
+				  sdhci_f_sdh30_resume_detect_work_func);
+	}
+
 	platform_set_drvdata(pdev, host);
 
 #ifdef CONFIG_PM_RUNTIME
@@ -263,6 +338,9 @@ static int sdhci_f_sdh30_probe(struct platform_device *pdev)
 	return 0;
 
 err_add_host:
+	if (priv->quirks & F_SDH30_QUIRK_RESUME_DETECT_RETRY)
+		destroy_workqueue(priv->resume_detect_wq);
+err_init_wq:
 	if (gpio_is_valid(priv->gpio_select_1v8)) {
 		gpio_direction_output(priv->gpio_select_1v8, 1);
 		gpio_free(priv->gpio_select_1v8);
@@ -302,6 +380,9 @@ static int sdhci_f_sdh30_remove(struct platform_device *pdev)
 		gpio_free(priv->gpio_select_1v8);
 	}
 
+	if (priv->quirks & F_SDH30_QUIRK_RESUME_DETECT_RETRY)
+		destroy_workqueue(priv->resume_detect_wq);
+
 	sdhci_free_host(host);
 	platform_set_drvdata(pdev, NULL);
 
@@ -319,7 +400,15 @@ static int sdhci_f_sdh30_suspend(struct device *dev)
 static int sdhci_f_sdh30_resume(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct f_sdhost_priv *priv = sdhci_priv(host);
 
+	if (priv->quirks & F_SDH30_QUIRK_RESUME_DETECT_RETRY) {
+		pr_debug("%s: start resume detect\n",
+			 mmc_hostname(host->mmc));
+		sdhci_f_sdh30_resume_detect(host->mmc,
+					    RESUME_DETECT_COUNT,
+					    RESUME_WAIT_COUNT);
+	}
 	return sdhci_resume_host(host);
 }
 #endif
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 7960424..55221dd 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -283,6 +283,7 @@ struct mmc_host {
 #define MMC_CAP2_HS400		(MMC_CAP2_HS400_1_8V | \
 				 MMC_CAP2_HS400_1_2V)
 #define MMC_CAP2_SDIO_IRQ_NOTHREAD (1 << 17)
+#define MMC_CAP2_MANUAL_RESUME     (1 << 18)	/* Resume manually when error */
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 
@@ -338,6 +339,9 @@ struct mmc_host {
 	const struct mmc_bus_ops *bus_ops;	/* current bus driver */
 	unsigned int		bus_refs;	/* reference counter */
 
+	unsigned int		bus_resume_flags;
+#define MMC_BUSRESUME_MANUAL_RESUME	(1 << 0)
+
 	unsigned int		sdio_irqs;
 	struct task_struct	*sdio_irq_thread;
 	bool			sdio_irq_pending;
@@ -384,6 +388,16 @@ static inline void *mmc_priv(struct mmc_host *host)
 #define mmc_dev(x)	((x)->parent)
 #define mmc_classdev(x)	(&(x)->class_dev)
 #define mmc_hostname(x)	(dev_name(&(x)->class_dev))
+#define mmc_bus_manual_resume(host) ((host)->bus_resume_flags &		\
+	MMC_BUSRESUME_MANUAL_RESUME)
+
+static inline void mmc_set_bus_resume_policy(struct mmc_host *host, int manual)
+{
+	if (manual)
+		host->bus_resume_flags |= MMC_BUSRESUME_MANUAL_RESUME;
+	else
+		host->bus_resume_flags &= ~MMC_BUSRESUME_MANUAL_RESUME;
+}
 
 int mmc_power_save_host(struct mmc_host *host);
 int mmc_power_restore_host(struct mmc_host *host);
-- 
1.8.1.2

Re: [PATCH 2/8] mmc: sdhci: host: add new f_sdh30

[Arnd Bergmann]

On Sunday 13 July 2014 14:29:31 Mollie Wu wrote:
> +Required properties:
> +- compatible: "fujitsu,f-sdh30"
> +- voltage-ranges : This is a list of pairs. In each pair, two cells
> +  are required. First cell specifies minimum slot voltage (mV), second
> +  cell specifies maximum slot voltage (mV). In case of supported voltage
> +  range is discontinuous, several ranges could be specified as a list.
> +
> +Optional properties:
> +- pwr-mux-gpios: This is one optional gpio for controlling a power mux
> +  which switches between two power supplies. 3.3V is selected when gpio
> +  is high, and 1.8V is selected when gpio is low. This voltage is used
> +  for signal level.

It sounds like what you really want here is a reference to a gpio-regulator
node and, and to put the details of the voltage switching in there.

> diff --git a/drivers/mmc/host/sdhci_f_sdh30.c b/drivers/mmc/host/sdhci_f_sdh30.c
> new file mode 100644
> index 0000000..8d23f2d
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci_f_sdh30.c
> @@ -0,0 +1,380 @@
> +/*
> + * linux/drivers/mmc/host/sdhci_f_sdh30.c
> + *
> + * Copyright (C) 2013 - 2014 Fujitsu Semiconductor, Ltd
> + *              Vincent Yang <vincent.yang@tw.fujitsu.com>
> + * Copyright (C) 2014 Linaro Ltd  Andy Green <andy.green@linaro.org>
> + *
> + * 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, version 2 of the License.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/mmc/sd.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/card.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/suspend.h>
> +
> +#include "sdhci.h"
> +#include "sdhci-pltfm.h"
> +#include "../core/core.h"

Should this be <linux/mmc/core.h>? A device driver should not be looking
at the drivers/mmc/core/core.h.

> +#define DRIVER_NAME "f_sdh30"

This macro doesn't seem to serve any purpose, it would be easier to
read if you open-code this.

> +
> +void sdhci_f_sdh30_soft_voltage_switch(struct sdhci_host *host)
> +{
> +	struct f_sdhost_priv *priv = sdhci_priv(host);
> +	u32 ctrl = 0;
> +
> +	usleep_range(2500, 3000);
> +	ctrl = sdhci_readl(host, F_SDH30_IO_CONTROL2);
> +	ctrl |= F_SDH30_CRES_O_DN;
> +	sdhci_writel(host, ctrl, F_SDH30_IO_CONTROL2);
> +	ctrl |= F_SDH30_MSEL_O_1_8;
> +	sdhci_writel(host, ctrl, F_SDH30_IO_CONTROL2);
> +
> +	if (gpio_is_valid(priv->gpio_select_1v8)) {
> +		dev_info(priv->dev, "%s: setting gpio\n", __func__);
> +		gpio_direction_output(priv->gpio_select_1v8, 0);
> +	}
> +
> +	ctrl &= ~F_SDH30_CRES_O_DN;
> +	sdhci_writel(host, ctrl, F_SDH30_IO_CONTROL2);
> +	usleep_range(2500, 3000);
> +
> +	if (priv->vendor_hs200) {
> +		dev_info(priv->dev, "%s: setting hs200\n", __func__);
> +		ctrl = sdhci_readl(host, F_SDH30_ESD_CONTROL);
> +		ctrl |= priv->vendor_hs200;
> +		sdhci_writel(host, ctrl, F_SDH30_ESD_CONTROL);
> +	}
> +
> +	ctrl = sdhci_readl(host, F_SDH30_TUNING_SETTING);
> +	ctrl |= F_SDH30_CMD_CHK_DIS;
> +	sdhci_writel(host, ctrl, F_SDH30_TUNING_SETTING);
> +}

I think this should really use the regulator API.

If the regular is defined properly, this will work without any extra code.

> +unsigned int sdhci_f_sdh30_get_min_clock(struct sdhci_host *host)
> +{
> +	return F_SDH30_MIN_CLOCK;
> +}
> +
> +void sdhci_f_sdh30_reset(struct sdhci_host *host, u8 mask)
> +{
> +	struct f_sdhost_priv *priv = sdhci_priv(host);
> +
> +	if (gpio_is_valid(priv->gpio_select_1v8))
> +		gpio_direction_output(priv->gpio_select_1v8, 1);
> +
> +	if (sdhci_readw(host, SDHCI_CLOCK_CONTROL) == 0) {
> +		sdhci_writew(host, 0xBC01, SDHCI_CLOCK_CONTROL);
> +		mmiowb();
> +	}

Can you explain the mmiowb call here?

> +
> +	if (!of_property_read_u32(pdev->dev.of_node, "bus-width", &bus_width)) {
> +		switch (bus_width) {
> +		case 8:
> +			dev_info(dev, "Applying 8 bit bus width\n");
> +			host->mmc->caps |= MMC_CAP_8_BIT_DATA;
> +			break;
> +		case 4:
> +			dev_info(dev, "Applying 4 bit bus width\n");
> +			host->mmc->caps |= MMC_CAP_4_BIT_DATA;
> +			break;
> +		case 1:
> +		default:
> +			dev_err(dev, "Invalid bus width: %u\n", bus_width);
> +			break;
> +		}
> +	}

This should probably be done in generic sdhci code somewhere. How about
adding it to sdhci_get_of_property instead_
> +	priv->clk_sd4 = clk_get(&pdev->dev, "sd_sd4clk");
> +	if (!IS_ERR(priv->clk_sd4)) {
> +		ret = clk_prepare_enable(priv->clk_sd4);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to enable sd4 clock: %d\n", ret);
> +			goto err_clk1;
> +		}
> +	}
> +	priv->clk_b = clk_get(&pdev->dev, "sd_bclk");
> +	if (!IS_ERR(priv->clk_b)) {
> +		ret = clk_prepare_enable(priv->clk_b);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to enable clk_b clock: %d\n", ret);
> +			goto err_clk2;
> +		}
> +	}

Please pick clock names that match what some of the other drivers use.

Ideally some of that should also move to common code.

	Arnd

RE: [PATCH 2/8] mmc: sdhci: host: add new f_sdh30

[Vincent.Yang]

>-----Original Message-----
>From: Arnd Bergmann [mailto:arnd@arndb.de]
>On Sunday 13 July 2014 14:29:31 Mollie Wu wrote:
>> +Required properties:
>> +- compatible: "fujitsu,f-sdh30"
>> +- voltage-ranges : This is a list of pairs. In each pair, two cells
>> +  are required. First cell specifies minimum slot voltage (mV), second
>> +  cell specifies maximum slot voltage (mV). In case of supported voltage
>> +  range is discontinuous, several ranges could be specified as a list.
>> +
>> +Optional properties:
>> +- pwr-mux-gpios: This is one optional gpio for controlling a power mux
>> +  which switches between two power supplies. 3.3V is selected when gpio
>> +  is high, and 1.8V is selected when gpio is low. This voltage is used
>> +  for signal level.
>
>It sounds like what you really want here is a reference to a gpio-regulator
>node and, and to put the details of the voltage switching in there.

Hi Arnd,
Yes, I'll use a gpio-regulator for it in next version.

>
>> diff --git a/drivers/mmc/host/sdhci_f_sdh30.c
>b/drivers/mmc/host/sdhci_f_sdh30.c
>> new file mode 100644
>> index 0000000..8d23f2d
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci_f_sdh30.c
>> @@ -0,0 +1,380 @@
>> +/*
>> + * linux/drivers/mmc/host/sdhci_f_sdh30.c
>> + *
>> + * Copyright (C) 2013 - 2014 Fujitsu Semiconductor, Ltd
>> + *              Vincent Yang <vincent.yang@tw.fujitsu.com>
>> + * Copyright (C) 2014 Linaro Ltd  Andy Green <andy.green@linaro.org>
>> + *
>> + * 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, version 2 of the License.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>> +#include <linux/mmc/sd.h>
>> +#include <linux/mmc/host.h>
>> +#include <linux/mmc/card.h>
>> +#include <linux/gpio.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/suspend.h>
>> +
>> +#include "sdhci.h"
>> +#include "sdhci-pltfm.h"
>> +#include "../core/core.h"
>
>Should this be <linux/mmc/core.h>? A device driver should not be looking
>at the drivers/mmc/core/core.h.

Yes, I'll remove it.

>
>> +#define DRIVER_NAME "f_sdh30"
>
>This macro doesn't seem to serve any purpose, it would be easier to
>read if you open-code this.

Yes, I'll do it.

>
>> +
>> +void sdhci_f_sdh30_soft_voltage_switch(struct sdhci_host *host)
>> +{
>> +     struct f_sdhost_priv *priv = sdhci_priv(host);
>> +     u32 ctrl = 0;
>> +
>> +     usleep_range(2500, 3000);
>> +     ctrl = sdhci_readl(host, F_SDH30_IO_CONTROL2);
>> +     ctrl |= F_SDH30_CRES_O_DN;
>> +     sdhci_writel(host, ctrl, F_SDH30_IO_CONTROL2);
>> +     ctrl |= F_SDH30_MSEL_O_1_8;
>> +     sdhci_writel(host, ctrl, F_SDH30_IO_CONTROL2);
>> +
>> +     if (gpio_is_valid(priv->gpio_select_1v8)) {
>> +             dev_info(priv->dev, "%s: setting gpio\n", __func__);
>> +             gpio_direction_output(priv->gpio_select_1v8, 0);
>> +     }
>> +
>> +     ctrl &= ~F_SDH30_CRES_O_DN;
>> +     sdhci_writel(host, ctrl, F_SDH30_IO_CONTROL2);
>> +     usleep_range(2500, 3000);
>> +
>> +     if (priv->vendor_hs200) {
>> +             dev_info(priv->dev, "%s: setting hs200\n", __func__);
>> +             ctrl = sdhci_readl(host, F_SDH30_ESD_CONTROL);
>> +             ctrl |= priv->vendor_hs200;
>> +             sdhci_writel(host, ctrl, F_SDH30_ESD_CONTROL);
>> +     }
>> +
>> +     ctrl = sdhci_readl(host, F_SDH30_TUNING_SETTING);
>> +     ctrl |= F_SDH30_CMD_CHK_DIS;
>> +     sdhci_writel(host, ctrl, F_SDH30_TUNING_SETTING);
>> +}
>
>I think this should really use the regulator API.
>
>If the regular is defined properly, this will work without any extra code.

Yes, I'll use a gpio-regulator for it.

>
>> +unsigned int sdhci_f_sdh30_get_min_clock(struct sdhci_host *host)
>> +{
>> +     return F_SDH30_MIN_CLOCK;
>> +}
>> +
>> +void sdhci_f_sdh30_reset(struct sdhci_host *host, u8 mask)
>> +{
>> +     struct f_sdhost_priv *priv = sdhci_priv(host);
>> +
>> +     if (gpio_is_valid(priv->gpio_select_1v8))
>> +             gpio_direction_output(priv->gpio_select_1v8, 1);
>> +
>> +     if (sdhci_readw(host, SDHCI_CLOCK_CONTROL) == 0) {
>> +             sdhci_writew(host, 0xBC01, SDHCI_CLOCK_CONTROL);
>> +             mmiowb();
>> +     }
>
>Can you explain the mmiowb call here?

This came from the original 3.0 based driver. It's trying to be a
write memory barrier. It wants to ensure the clock control change
actually happened before the following code.
I'll change it to a regular wmb in next version.

>
>> +
>> +     if (!of_property_read_u32(pdev->dev.of_node, "bus-width", &bus_width))
>{
>> +             switch (bus_width) {
>> +             case 8:
>> +                     dev_info(dev, "Applying 8 bit bus width\n");
>> +                     host->mmc->caps |= MMC_CAP_8_BIT_DATA;
>> +                     break;
>> +             case 4:
>> +                     dev_info(dev, "Applying 4 bit bus width\n");
>> +                     host->mmc->caps |= MMC_CAP_4_BIT_DATA;
>> +                     break;
>> +             case 1:
>> +             default:
>> +                     dev_err(dev, "Invalid bus width: %u\n", bus_width);
>> +                     break;
>> +             }
>> +     }
>
>This should probably be done in generic sdhci code somewhere. How about
>adding it to sdhci_get_of_property instead_

I should use generic mmc_of_parse for it. I'll update it in next version.

>> +     priv->clk_sd4 = clk_get(&pdev->dev, "sd_sd4clk");
>> +     if (!IS_ERR(priv->clk_sd4)) {
>> +             ret = clk_prepare_enable(priv->clk_sd4);
>> +             if (ret < 0) {
>> +                     dev_err(dev, "Failed to enable sd4 clock: %d\n", ret);
>> +                     goto err_clk1;
>> +             }
>> +     }
>> +     priv->clk_b = clk_get(&pdev->dev, "sd_bclk");
>> +     if (!IS_ERR(priv->clk_b)) {
>> +             ret = clk_prepare_enable(priv->clk_b);
>> +             if (ret < 0) {
>> +                     dev_err(dev, "Failed to enable clk_b clock: %d\n", ret);
>> +                     goto err_clk2;
>> +             }
>> +     }
>
>Please pick clock names that match what some of the other drivers use.

I'll use "iface" and "core" for clock names because they match what used
in sdhci-msm driver.

Thanks a lot for your review!

Best regards,
Vincent Yang

>
>Ideally some of that should also move to common code.
>
>        Arnd

Re: [PATCH 2/8] mmc: sdhci: host: add new f_sdh30

[Arnd Bergmann]

On Wednesday 16 July 2014 17:35:41 Vincent. Yang wrote:

> >
> >> +unsigned int sdhci_f_sdh30_get_min_clock(struct sdhci_host *host)
> >> +{
> >> +     return F_SDH30_MIN_CLOCK;
> >> +}
> >> +
> >> +void sdhci_f_sdh30_reset(struct sdhci_host *host, u8 mask)
> >> +{
> >> +     struct f_sdhost_priv *priv = sdhci_priv(host);
> >> +
> >> +     if (gpio_is_valid(priv->gpio_select_1v8))
> >> +             gpio_direction_output(priv->gpio_select_1v8, 1);
> >> +
> >> +     if (sdhci_readw(host, SDHCI_CLOCK_CONTROL) == 0) {
> >> +             sdhci_writew(host, 0xBC01, SDHCI_CLOCK_CONTROL);
> >> +             mmiowb();
> >> +     }
> >
> >Can you explain the mmiowb call here?
> 
> This came from the original 3.0 based driver. It's trying to be a
> write memory barrier. It wants to ensure the clock control change
> actually happened before the following code.
> I'll change it to a regular wmb in next version.

Note that with 'readw'/writew', you shouldn't need any extra barriers.
If sdhci_writew() is readw_relaxed(), wmb() is the correct barrier.

> >> +
> >> +     if (!of_property_read_u32(pdev->dev.of_node, "bus-width", &bus_width))
> >{
> >> +             switch (bus_width) {
> >> +             case 8:
> >> +                     dev_info(dev, "Applying 8 bit bus width\n");
> >> +                     host->mmc->caps |= MMC_CAP_8_BIT_DATA;
> >> +                     break;
> >> +             case 4:
> >> +                     dev_info(dev, "Applying 4 bit bus width\n");
> >> +                     host->mmc->caps |= MMC_CAP_4_BIT_DATA;
> >> +                     break;
> >> +             case 1:
> >> +             default:
> >> +                     dev_err(dev, "Invalid bus width: %u\n", bus_width);
> >> +                     break;
> >> +             }
> >> +     }
> >
> >This should probably be done in generic sdhci code somewhere. How about
> >adding it to sdhci_get_of_property instead_
> 
> I should use generic mmc_of_parse for it. I'll update it in next version.

Ah right, I thought we had this in common code already but couldn't find
it. Using mmc_of_parse() is definitely the correct solution here.

> >> +     priv->clk_sd4 = clk_get(&pdev->dev, "sd_sd4clk");
> >> +     if (!IS_ERR(priv->clk_sd4)) {
> >> +             ret = clk_prepare_enable(priv->clk_sd4);
> >> +             if (ret < 0) {
> >> +                     dev_err(dev, "Failed to enable sd4 clock: %d\n", ret);
> >> +                     goto err_clk1;
> >> +             }
> >> +     }
> >> +     priv->clk_b = clk_get(&pdev->dev, "sd_bclk");
> >> +     if (!IS_ERR(priv->clk_b)) {
> >> +             ret = clk_prepare_enable(priv->clk_b);
> >> +             if (ret < 0) {
> >> +                     dev_err(dev, "Failed to enable clk_b clock: %d\n", ret);
> >> +                     goto err_clk2;
> >> +             }
> >> +     }
> >
> >Please pick clock names that match what some of the other drivers use.
> 
> I'll use "iface" and "core" for clock names because they match what used
> in sdhci-msm driver.

Ok, makes sense.

	Arnd

RE: [PATCH 2/8] mmc: sdhci: host: add new f_sdh30

[Vincent.Yang]

>-----Original Message-----
>From: Arnd Bergmann [mailto:arnd@arndb.de]
>On Wednesday 16 July 2014 17:35:41 Vincent. Yang wrote:
>
>> >
>> >> +unsigned int sdhci_f_sdh30_get_min_clock(struct sdhci_host *host)
>> >> +{
>> >> +     return F_SDH30_MIN_CLOCK;
>> >> +}
>> >> +
>> >> +void sdhci_f_sdh30_reset(struct sdhci_host *host, u8 mask)
>> >> +{
>> >> +     struct f_sdhost_priv *priv = sdhci_priv(host);
>> >> +
>> >> +     if (gpio_is_valid(priv->gpio_select_1v8))
>> >> +             gpio_direction_output(priv->gpio_select_1v8, 1);
>> >> +
>> >> +     if (sdhci_readw(host, SDHCI_CLOCK_CONTROL) == 0) {
>> >> +             sdhci_writew(host, 0xBC01, SDHCI_CLOCK_CONTROL);
>> >> +             mmiowb();
>> >> +     }
>> >
>> >Can you explain the mmiowb call here?
>>
>> This came from the original 3.0 based driver. It's trying to be a
>> write memory barrier. It wants to ensure the clock control change
>> actually happened before the following code.
>> I'll change it to a regular wmb in next version.
>
>Note that with 'readw'/writew', you shouldn't need any extra barriers.
>If sdhci_writew() is readw_relaxed(), wmb() is the correct barrier.

Hi Arnd,
The sdhci_writew() is using writew(), so I shouldn't need any extra
barriers. I'll remove them in next version.
Thanks a lot for your review!


Best regards,
Vincent Yang

>
>> >> +
>> >> +     if (!of_property_read_u32(pdev->dev.of_node, "bus-width",
>&bus_width))
>> >{
>> >> +             switch (bus_width) {
>> >> +             case 8:
>> >> +                     dev_info(dev, "Applying 8 bit bus width\n");
>> >> +                     host->mmc->caps |= MMC_CAP_8_BIT_DATA;
>> >> +                     break;
>> >> +             case 4:
>> >> +                     dev_info(dev, "Applying 4 bit bus width\n");
>> >> +                     host->mmc->caps |= MMC_CAP_4_BIT_DATA;
>> >> +                     break;
>> >> +             case 1:
>> >> +             default:
>> >> +                     dev_err(dev, "Invalid bus width: %u\n", bus_width);
>> >> +                     break;
>> >> +             }
>> >> +     }
>> >
>> >This should probably be done in generic sdhci code somewhere. How about
>> >adding it to sdhci_get_of_property instead_
>>
>> I should use generic mmc_of_parse for it. I'll update it in next version.
>
>Ah right, I thought we had this in common code already but couldn't find
>it. Using mmc_of_parse() is definitely the correct solution here.
>
>> >> +     priv->clk_sd4 = clk_get(&pdev->dev, "sd_sd4clk");
>> >> +     if (!IS_ERR(priv->clk_sd4)) {
>> >> +             ret = clk_prepare_enable(priv->clk_sd4);
>> >> +             if (ret < 0) {
>> >> +                     dev_err(dev, "Failed to enable sd4 clock: %d\n", ret);
>> >> +                     goto err_clk1;
>> >> +             }
>> >> +     }
>> >> +     priv->clk_b = clk_get(&pdev->dev, "sd_bclk");
>> >> +     if (!IS_ERR(priv->clk_b)) {
>> >> +             ret = clk_prepare_enable(priv->clk_b);
>> >> +             if (ret < 0) {
>> >> +                     dev_err(dev, "Failed to enable clk_b clock: %d\n", ret);
>> >> +                     goto err_clk2;
>> >> +             }
>> >> +     }
>> >
>> >Please pick clock names that match what some of the other drivers use.
>>
>> I'll use "iface" and "core" for clock names because they match what used
>> in sdhci-msm driver.
>
>Ok, makes sense.
>
>        Arnd