[PATCH v3 0/4] mmc: sdhci: adding support for a new Fujitsu sdhci IP

[PATCH v3 0/4] mmc: sdhci: adding support for a new Fujitsu sdhci IP

[Vincent Yang]

Hello,

  Fujitsu have an sdhci IP which is implemented in a SoC we're 
adding to mainline, the most recent series for that was sent 
here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/314615.html

  We welcome any comment and advice about how to make any
improvements or better align them with upstream.

Changes since v2:
* Removed voltage-ranges because it is no longer a required property.
* Removed unnecessary error message.
* Used the devm_* managed functions and updated the error handling
accordingly.
* Removed #ifdef CONFIG_PM_RUNTIME and also updated some other #ifdef.
* Moved vendor specfic stuff prior to sdhci_add_host().
* Handled runtime PM on module removal.

Changes since v1:
* Thanks to Uffe, removed ARCH_MB86S7X dependency and separated
mmc patches to this patchset.
* Node name changed from "fujitsu,mb86s70-sdh30" to
"fujitsu,mb86s70-sdhci-3.0".

Thanks.

Vincent Yang (4):
  mmc: sdhci: add a voltage switch callback function
  mmc: sdhci: add a quirk for tuning work around
  mmc: sdhci: add a quirk for single block transactions
  mmc: sdhci: host: add new f_sdh30

 .../devicetree/bindings/mmc/sdhci-fujitsu.txt      |  30 ++
 drivers/mmc/host/Kconfig                           |   8 +
 drivers/mmc/host/Makefile                          |   1 +
 drivers/mmc/host/sdhci.c                           |  14 +-
 drivers/mmc/host/sdhci.h                           |   1 +
 drivers/mmc/host/sdhci_f_sdh30.c                   | 306 +++++++++++++++++++++
 include/linux/mmc/sdhci.h                          |   4 +
 7 files changed, 361 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
 create mode 100644 drivers/mmc/host/sdhci_f_sdh30.c

-- 
1.9.0

[PATCH v3 0/4] mmc: sdhci: adding support for a new Fujitsu sdhci IP

[Vincent Yang]

Hello,

  Fujitsu have an sdhci IP which is implemented in a SoC we're 
adding to mainline, the most recent series for that was sent 
here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/314615.html

  We welcome any comment and advice about how to make any
improvements or better align them with upstream.

Changes since v2:
* Removed voltage-ranges because it is no longer a required property.
* Removed unnecessary error message.
* Used the devm_* managed functions and updated the error handling
accordingly.
* Removed #ifdef CONFIG_PM_RUNTIME and also updated some other #ifdef.
* Moved vendor specfic stuff prior to sdhci_add_host().
* Handled runtime PM on module removal.

Changes since v1:
* Thanks to Uffe, removed ARCH_MB86S7X dependency and separated
mmc patches to this patchset.
* Node name changed from "fujitsu,mb86s70-sdh30" to
"fujitsu,mb86s70-sdhci-3.0".

Thanks.

Vincent Yang (4):
  mmc: sdhci: add a voltage switch callback function
  mmc: sdhci: add a quirk for tuning work around
  mmc: sdhci: add a quirk for single block transactions
  mmc: sdhci: host: add new f_sdh30

 .../devicetree/bindings/mmc/sdhci-fujitsu.txt      |  30 ++
 drivers/mmc/host/Kconfig                           |   8 +
 drivers/mmc/host/Makefile                          |   1 +
 drivers/mmc/host/sdhci.c                           |  14 +-
 drivers/mmc/host/sdhci.h                           |   1 +
 drivers/mmc/host/sdhci_f_sdh30.c                   | 306 +++++++++++++++++++++
 include/linux/mmc/sdhci.h                          |   4 +
 7 files changed, 361 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
 create mode 100644 drivers/mmc/host/sdhci_f_sdh30.c

-- 
1.9.0

[PATCH v3 1/4] mmc: sdhci: add a voltage switch callback function

[Vincent Yang]

This patch adds a callback function to do
controller-specific actions when switching voltages.
It is a preparation and will be used by Fujitsu
SDHCI controller f_sdh30 driver.

Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
---
 drivers/mmc/host/sdhci.c | 4 ++++
 drivers/mmc/host/sdhci.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index cbb245b..cd1f311 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1827,6 +1827,10 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		ctrl |= SDHCI_CTRL_VDD_180;
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
+		/* Some controller need to do more when switching */
+		if (host->ops->voltage_switch)
+			host->ops->voltage_switch(host);
+
 		/* 1.8V regulator output should be stable within 5 ms */
 		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
 		if (ctrl & SDHCI_CTRL_VDD_180)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 41a2c34..0315e18 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -339,6 +339,7 @@ struct sdhci_ops {
 	void    (*adma_workaround)(struct sdhci_host *host, u32 intmask);
 	void	(*platform_init)(struct sdhci_host *host);
 	void    (*card_event)(struct sdhci_host *host);
+	void	(*voltage_switch)(struct sdhci_host *host);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
-- 
1.9.0

[PATCH v3 1/4] mmc: sdhci: add a voltage switch callback function

[Vincent Yang]

This patch adds a callback function to do
controller-specific actions when switching voltages.
It is a preparation and will be used by Fujitsu
SDHCI controller f_sdh30 driver.

Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
---
 drivers/mmc/host/sdhci.c | 4 ++++
 drivers/mmc/host/sdhci.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index cbb245b..cd1f311 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1827,6 +1827,10 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
 		ctrl |= SDHCI_CTRL_VDD_180;
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
+		/* Some controller need to do more when switching */
+		if (host->ops->voltage_switch)
+			host->ops->voltage_switch(host);
+
 		/* 1.8V regulator output should be stable within 5 ms */
 		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
 		if (ctrl & SDHCI_CTRL_VDD_180)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 41a2c34..0315e18 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -339,6 +339,7 @@ struct sdhci_ops {
 	void    (*adma_workaround)(struct sdhci_host *host, u32 intmask);
 	void	(*platform_init)(struct sdhci_host *host);
 	void    (*card_event)(struct sdhci_host *host);
+	void	(*voltage_switch)(struct sdhci_host *host);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
-- 
1.9.0

[PATCH v3 2/4] mmc: sdhci: add a quirk for tuning work around

[Vincent Yang]

This patch defines a quirk for tuning work
around for some sdhci host controller. It sets
both SDHCI_CTRL_EXEC_TUNING and SDHCI_CTRL_TUNED_CLK
for tuning.
It is a preparation and will be used by Fujitsu
SDHCI controller f_sdh30 driver.

Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
---
 drivers/mmc/host/sdhci.c  | 2 ++
 include/linux/mmc/sdhci.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index cd1f311..b319c10 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1929,6 +1929,8 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 
 	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
 	ctrl |= SDHCI_CTRL_EXEC_TUNING;
+	if (host->quirks2 & SDHCI_QUIRK2_TUNING_WORK_AROUND)
+		ctrl |= SDHCI_CTRL_TUNED_CLK;
 	sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
 	/*
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 375af80..dd2ba4b 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -106,6 +106,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD	(1<<10)
 /* Capability register bit-63 indicates HS400 support */
 #define SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400		(1<<11)
+/* forced tuned clock */
+#define SDHCI_QUIRK2_TUNING_WORK_AROUND			(1<<12)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
1.9.0

[PATCH v3 2/4] mmc: sdhci: add a quirk for tuning work around

[Vincent Yang]

This patch defines a quirk for tuning work
around for some sdhci host controller. It sets
both SDHCI_CTRL_EXEC_TUNING and SDHCI_CTRL_TUNED_CLK
for tuning.
It is a preparation and will be used by Fujitsu
SDHCI controller f_sdh30 driver.

Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
---
 drivers/mmc/host/sdhci.c  | 2 ++
 include/linux/mmc/sdhci.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index cd1f311..b319c10 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1929,6 +1929,8 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 
 	ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
 	ctrl |= SDHCI_CTRL_EXEC_TUNING;
+	if (host->quirks2 & SDHCI_QUIRK2_TUNING_WORK_AROUND)
+		ctrl |= SDHCI_CTRL_TUNED_CLK;
 	sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
 	/*
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 375af80..dd2ba4b 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -106,6 +106,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD	(1<<10)
 /* Capability register bit-63 indicates HS400 support */
 #define SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400		(1<<11)
+/* forced tuned clock */
+#define SDHCI_QUIRK2_TUNING_WORK_AROUND			(1<<12)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
1.9.0

[PATCH v3 3/4] mmc: sdhci: add a quirk for single block transactions

[Vincent Yang]

This patch defines a quirk to disable the block count
for single block transactions.
It is a preparation and will be used by Fujitsu
SDHCI controller f_sdh30 driver.

Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
---
 drivers/mmc/host/sdhci.c  | 8 +++++---
 include/linux/mmc/sdhci.h | 2 ++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index b319c10..5581b16 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -911,7 +911,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 static void sdhci_set_transfer_mode(struct sdhci_host *host,
 	struct mmc_command *cmd)
 {
-	u16 mode;
+	u16 mode = 0;
 	struct mmc_data *data = cmd->data;
 
 	if (data == NULL) {
@@ -929,9 +929,11 @@ static void sdhci_set_transfer_mode(struct sdhci_host *host,
 
 	WARN_ON(!host->data);
 
-	mode = SDHCI_TRNS_BLK_CNT_EN;
+	if (!(host->quirks2 & SDHCI_QUIRK2_SUPPORT_SINGLE))
+		mode = SDHCI_TRNS_BLK_CNT_EN;
+
 	if (mmc_op_multi(cmd->opcode) || data->blocks > 1) {
-		mode |= SDHCI_TRNS_MULTI;
+		mode = SDHCI_TRNS_BLK_CNT_EN | SDHCI_TRNS_MULTI;
 		/*
 		 * If we are sending CMD23, CMD12 never gets sent
 		 * on successful completion (so no Auto-CMD12).
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index dd2ba4b..2e048f4 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -108,6 +108,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400		(1<<11)
 /* forced tuned clock */
 #define SDHCI_QUIRK2_TUNING_WORK_AROUND			(1<<12)
+/* disable the block count for single block transactions */
+#define SDHCI_QUIRK2_SUPPORT_SINGLE			(1<<13)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
1.9.0

[PATCH v3 3/4] mmc: sdhci: add a quirk for single block transactions

[Vincent Yang]

This patch defines a quirk to disable the block count
for single block transactions.
It is a preparation and will be used by Fujitsu
SDHCI controller f_sdh30 driver.

Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
---
 drivers/mmc/host/sdhci.c  | 8 +++++---
 include/linux/mmc/sdhci.h | 2 ++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index b319c10..5581b16 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -911,7 +911,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 static void sdhci_set_transfer_mode(struct sdhci_host *host,
 	struct mmc_command *cmd)
 {
-	u16 mode;
+	u16 mode = 0;
 	struct mmc_data *data = cmd->data;
 
 	if (data == NULL) {
@@ -929,9 +929,11 @@ static void sdhci_set_transfer_mode(struct sdhci_host *host,
 
 	WARN_ON(!host->data);
 
-	mode = SDHCI_TRNS_BLK_CNT_EN;
+	if (!(host->quirks2 & SDHCI_QUIRK2_SUPPORT_SINGLE))
+		mode = SDHCI_TRNS_BLK_CNT_EN;
+
 	if (mmc_op_multi(cmd->opcode) || data->blocks > 1) {
-		mode |= SDHCI_TRNS_MULTI;
+		mode = SDHCI_TRNS_BLK_CNT_EN | SDHCI_TRNS_MULTI;
 		/*
 		 * If we are sending CMD23, CMD12 never gets sent
 		 * on successful completion (so no Auto-CMD12).
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index dd2ba4b..2e048f4 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -108,6 +108,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400		(1<<11)
 /* forced tuned clock */
 #define SDHCI_QUIRK2_TUNING_WORK_AROUND			(1<<12)
+/* disable the block count for single block transactions */
+#define SDHCI_QUIRK2_SUPPORT_SINGLE			(1<<13)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
1.9.0

[PATCH v3 4/4] mmc: sdhci: host: add new f_sdh30

[Vincent Yang]

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

Signed-off-by: Andy Green <andy.green@linaro.org>
Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
Signed-off-by: Tetsuya Takinishi <t.takinishi@jp.fujitsu.com>
---
 .../devicetree/bindings/mmc/sdhci-fujitsu.txt      |  30 ++
 drivers/mmc/host/Kconfig                           |   8 +
 drivers/mmc/host/Makefile                          |   1 +
 drivers/mmc/host/sdhci_f_sdh30.c                   | 306 +++++++++++++++++++++
 4 files changed, 345 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..d77df7b
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
@@ -0,0 +1,30 @@
+* 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,mb86s70-sdhci-3.0"
+
+Optional properties:
+- vqmmc-supply: phandle to the regulator device tree node, mentioned
+  as the VCCQ/VDD_IO supply in the eMMC/SD specs.
+- 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:
+	"iface" - clock used for sdhci interface
+	"core"  - core clock for sdhci controller
+
+Example:
+
+	sdhci1: mmc@36600000 {
+		compatible = "fujitsu,mb86s70-sdhci-3.0";
+		reg = <0 0x36600000 0x1000>;
+		interrupts = <0 172 0x4>,
+			     <0 173 0x4>;
+		bus-width = <4>;
+		vqmmc-supply = <&vccq_sdhci1>;
+		clocks = <&clock 2 2 0>, <&clock 2 3 0>;
+		clock-names = "iface", "core";
+	};
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 2d6fbdd..288dcc3 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -290,6 +290,14 @@ 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_PLTFM
+	depends on OF
+	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 f7b0a77..6a7cfe0 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -16,6 +16,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..2dc16e7
--- /dev/null
+++ b/drivers/mmc/host/sdhci_f_sdh30.c
@@ -0,0 +1,306 @@
+/*
+ * linux/drivers/mmc/host/sdhci_f_sdh30.c
+ *
+ * Copyright (C) 2013 - 2015 Fujitsu Semiconductor, Ltd
+ *              Vincent Yang <vincent.yang@tw.fujitsu.com>
+ * Copyright (C) 2015 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"
+
+/* 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_iface;
+	struct clk *clk;
+	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);
+
+	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)
+{
+	if (sdhci_readw(host, SDHCI_CLOCK_CONTROL) == 0)
+		sdhci_writew(host, 0xBC01, SDHCI_CLOCK_CONTROL);
+
+	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;
+	struct resource *res;
+	int irq, ctrl = 0, ret = 0;
+	struct f_sdhost_priv *priv;
+	u32 reg = 0;
+
+	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)) {
+		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_TUNING_WORK_AROUND;
+
+	ret = mmc_of_parse(host->mmc);
+	if (ret)
+		goto err_of_parse;
+
+	platform_set_drvdata(pdev, host);
+
+	sdhci_get_of_property(pdev);
+	host->hw_name = "f_sdh30";
+	host->ops = &sdhci_f_sdh30_ops;
+	host->irq = irq;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	host->ioaddr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(host->ioaddr)) {
+		dev_err(dev, "%s: failed to remap registers\n", __func__);
+		ret = -ENXIO;
+		goto err_of_parse;
+	}
+
+	priv->clk_iface = devm_clk_get(&pdev->dev, "iface");
+	if (!IS_ERR(priv->clk_iface)) {
+		ret = clk_prepare_enable(priv->clk_iface);
+		if (ret < 0) {
+			dev_err(dev, "Failed to enable iface clock: %d\n", ret);
+			goto err_of_parse;
+		}
+	}
+	priv->clk = devm_clk_get(&pdev->dev, "core");
+	if (!IS_ERR(priv->clk)) {
+		ret = clk_prepare_enable(priv->clk);
+		if (ret < 0) {
+			dev_err(dev, "Failed to enable core clock: %d\n", ret);
+			goto err_clk;
+		}
+	}
+
+	pm_runtime_set_active(&pdev->dev);
+	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);
+
+	/* 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);
+
+	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);
+
+	reg = sdhci_readl(host, SDHCI_CAPABILITIES);
+	if (reg & SDHCI_CAN_DO_8BIT)
+		priv->vendor_hs200 = F_SDH30_EMMC_HS200;
+	else
+		priv->vendor_hs200 = 0;
+
+	ret = sdhci_add_host(host);
+	if (ret) {
+		dev_err(dev, "%s: host add error\n", __func__);
+		goto err_add_host;
+	}
+
+	return 0;
+
+err_add_host:
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+	clk_disable_unprepare(priv->clk);
+err_clk:
+	clk_disable_unprepare(priv->clk_iface);
+err_of_parse:
+	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);
+
+	pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+
+	sdhci_remove_host(host, readl(host->ioaddr + SDHCI_INT_STATUS) ==
+			  0xffffffff);
+	release_mem_region(iomem->start, resource_size(iomem));
+
+	clk_disable_unprepare(priv->clk_iface);
+	clk_disable_unprepare(priv->clk);
+
+	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
+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
+
+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)
+};
+
+static const struct of_device_id f_sdh30_dt_ids[] = {
+	{ .compatible = "fujitsu,mb86s70-sdhci-3.0" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, f_sdh30_dt_ids);
+
+static struct platform_driver sdhci_f_sdh30_driver = {
+	.driver = {
+		.name = "f_sdh30",
+		.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:f_sdh30");
-- 
1.9.0

[PATCH v3 4/4] mmc: sdhci: host: add new f_sdh30

[Vincent Yang]

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

Signed-off-by: Andy Green <andy.green@linaro.org>
Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
Signed-off-by: Tetsuya Takinishi <t.takinishi@jp.fujitsu.com>
---
 .../devicetree/bindings/mmc/sdhci-fujitsu.txt      |  30 ++
 drivers/mmc/host/Kconfig                           |   8 +
 drivers/mmc/host/Makefile                          |   1 +
 drivers/mmc/host/sdhci_f_sdh30.c                   | 306 +++++++++++++++++++++
 4 files changed, 345 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..d77df7b
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
@@ -0,0 +1,30 @@
+* 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,mb86s70-sdhci-3.0"
+
+Optional properties:
+- vqmmc-supply: phandle to the regulator device tree node, mentioned
+  as the VCCQ/VDD_IO supply in the eMMC/SD specs.
+- 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:
+	"iface" - clock used for sdhci interface
+	"core"  - core clock for sdhci controller
+
+Example:
+
+	sdhci1: mmc at 36600000 {
+		compatible = "fujitsu,mb86s70-sdhci-3.0";
+		reg = <0 0x36600000 0x1000>;
+		interrupts = <0 172 0x4>,
+			     <0 173 0x4>;
+		bus-width = <4>;
+		vqmmc-supply = <&vccq_sdhci1>;
+		clocks = <&clock 2 2 0>, <&clock 2 3 0>;
+		clock-names = "iface", "core";
+	};
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 2d6fbdd..288dcc3 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -290,6 +290,14 @@ 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_PLTFM
+	depends on OF
+	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 f7b0a77..6a7cfe0 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -16,6 +16,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..2dc16e7
--- /dev/null
+++ b/drivers/mmc/host/sdhci_f_sdh30.c
@@ -0,0 +1,306 @@
+/*
+ * linux/drivers/mmc/host/sdhci_f_sdh30.c
+ *
+ * Copyright (C) 2013 - 2015 Fujitsu Semiconductor, Ltd
+ *              Vincent Yang <vincent.yang@tw.fujitsu.com>
+ * Copyright (C) 2015 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"
+
+/* 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_iface;
+	struct clk *clk;
+	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);
+
+	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)
+{
+	if (sdhci_readw(host, SDHCI_CLOCK_CONTROL) == 0)
+		sdhci_writew(host, 0xBC01, SDHCI_CLOCK_CONTROL);
+
+	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;
+	struct resource *res;
+	int irq, ctrl = 0, ret = 0;
+	struct f_sdhost_priv *priv;
+	u32 reg = 0;
+
+	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)) {
+		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_TUNING_WORK_AROUND;
+
+	ret = mmc_of_parse(host->mmc);
+	if (ret)
+		goto err_of_parse;
+
+	platform_set_drvdata(pdev, host);
+
+	sdhci_get_of_property(pdev);
+	host->hw_name = "f_sdh30";
+	host->ops = &sdhci_f_sdh30_ops;
+	host->irq = irq;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	host->ioaddr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(host->ioaddr)) {
+		dev_err(dev, "%s: failed to remap registers\n", __func__);
+		ret = -ENXIO;
+		goto err_of_parse;
+	}
+
+	priv->clk_iface = devm_clk_get(&pdev->dev, "iface");
+	if (!IS_ERR(priv->clk_iface)) {
+		ret = clk_prepare_enable(priv->clk_iface);
+		if (ret < 0) {
+			dev_err(dev, "Failed to enable iface clock: %d\n", ret);
+			goto err_of_parse;
+		}
+	}
+	priv->clk = devm_clk_get(&pdev->dev, "core");
+	if (!IS_ERR(priv->clk)) {
+		ret = clk_prepare_enable(priv->clk);
+		if (ret < 0) {
+			dev_err(dev, "Failed to enable core clock: %d\n", ret);
+			goto err_clk;
+		}
+	}
+
+	pm_runtime_set_active(&pdev->dev);
+	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);
+
+	/* 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);
+
+	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);
+
+	reg = sdhci_readl(host, SDHCI_CAPABILITIES);
+	if (reg & SDHCI_CAN_DO_8BIT)
+		priv->vendor_hs200 = F_SDH30_EMMC_HS200;
+	else
+		priv->vendor_hs200 = 0;
+
+	ret = sdhci_add_host(host);
+	if (ret) {
+		dev_err(dev, "%s: host add error\n", __func__);
+		goto err_add_host;
+	}
+
+	return 0;
+
+err_add_host:
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+	clk_disable_unprepare(priv->clk);
+err_clk:
+	clk_disable_unprepare(priv->clk_iface);
+err_of_parse:
+	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);
+
+	pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+
+	sdhci_remove_host(host, readl(host->ioaddr + SDHCI_INT_STATUS) ==
+			  0xffffffff);
+	release_mem_region(iomem->start, resource_size(iomem));
+
+	clk_disable_unprepare(priv->clk_iface);
+	clk_disable_unprepare(priv->clk);
+
+	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
+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
+
+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)
+};
+
+static const struct of_device_id f_sdh30_dt_ids[] = {
+	{ .compatible = "fujitsu,mb86s70-sdhci-3.0" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, f_sdh30_dt_ids);
+
+static struct platform_driver sdhci_f_sdh30_driver = {
+	.driver = {
+		.name = "f_sdh30",
+		.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:f_sdh30");
-- 
1.9.0

Re: [PATCH v3 4/4] mmc: sdhci: host: add new f_sdh30

[Ulf Hansson]

On 9 January 2015 at 12:41, Vincent Yang <vincent.yang.fujitsu@gmail.com> wrote:
> This patch adds new host controller driver for
> Fujitsu SDHCI controller f_sdh30.
>
> Signed-off-by: Andy Green <andy.green@linaro.org>
> Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
> Signed-off-by: Tetsuya Takinishi <t.takinishi@jp.fujitsu.com>
> ---
>  .../devicetree/bindings/mmc/sdhci-fujitsu.txt      |  30 ++
>  drivers/mmc/host/Kconfig                           |   8 +
>  drivers/mmc/host/Makefile                          |   1 +
>  drivers/mmc/host/sdhci_f_sdh30.c                   | 306 +++++++++++++++++++++
>  4 files changed, 345 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..d77df7b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
> @@ -0,0 +1,30 @@
> +* 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,mb86s70-sdhci-3.0"
> +
> +Optional properties:
> +- vqmmc-supply: phandle to the regulator device tree node, mentioned
> +  as the VCCQ/VDD_IO supply in the eMMC/SD specs.
> +- 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:
> +       "iface" - clock used for sdhci interface
> +       "core"  - core clock for sdhci controller
> +
> +Example:
> +
> +       sdhci1: mmc@36600000 {
> +               compatible = "fujitsu,mb86s70-sdhci-3.0";
> +               reg = <0 0x36600000 0x1000>;
> +               interrupts = <0 172 0x4>,
> +                            <0 173 0x4>;
> +               bus-width = <4>;
> +               vqmmc-supply = <&vccq_sdhci1>;
> +               clocks = <&clock 2 2 0>, <&clock 2 3 0>;
> +               clock-names = "iface", "core";
> +       };
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 2d6fbdd..288dcc3 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -290,6 +290,14 @@ 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_PLTFM
> +       depends on OF
> +       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 f7b0a77..6a7cfe0 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -16,6 +16,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..2dc16e7
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci_f_sdh30.c
> @@ -0,0 +1,306 @@
> +/*
> + * linux/drivers/mmc/host/sdhci_f_sdh30.c
> + *
> + * Copyright (C) 2013 - 2015 Fujitsu Semiconductor, Ltd
> + *              Vincent Yang <vincent.yang@tw.fujitsu.com>
> + * Copyright (C) 2015 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>

You can likely remove some of these includes which you don't use. Like
sd.h, suspend.h, etc...

Also, I don't find clk.h

>
> +#include "sdhci.h"
> +#include "sdhci-pltfm.h"
> +
> +/* 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_iface;
> +       struct clk *clk;
> +       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);
> +
> +       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)
> +{
> +       if (sdhci_readw(host, SDHCI_CLOCK_CONTROL) == 0)
> +               sdhci_writew(host, 0xBC01, SDHCI_CLOCK_CONTROL);
> +
> +       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;
> +       struct resource *res;
> +       int irq, ctrl = 0, ret = 0;
> +       struct f_sdhost_priv *priv;
> +       u32 reg = 0;
> +
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq < 0) {
> +               dev_err(dev, "%s: no irq specified\n", __func__);
> +               ret = irq;
> +               goto err;

The goto is a superfluous, just do: return irq;

> +       }
> +
> +       host = sdhci_alloc_host(dev, sizeof(struct sdhci_host) +
> +                                               sizeof(struct f_sdhost_priv));
> +       if (IS_ERR(host)) {
> +               ret = -ENOMEM;
> +               goto err;

Replace by:
return PTR_ERR(host);

> +       }
> +       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_TUNING_WORK_AROUND;
> +
> +       ret = mmc_of_parse(host->mmc);
> +       if (ret)
> +               goto err_of_parse;
> +
> +       platform_set_drvdata(pdev, host);
> +
> +       sdhci_get_of_property(pdev);
> +       host->hw_name = "f_sdh30";
> +       host->ops = &sdhci_f_sdh30_ops;
> +       host->irq = irq;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       host->ioaddr = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(host->ioaddr)) {
> +               dev_err(dev, "%s: failed to remap registers\n", __func__);

The error print is not needed, since it's already done by
devm_ioremap_resource().

> +               ret = -ENXIO;

Don't overwrite the error, instead do: ret = PTR_ERR(host);

> +               goto err_of_parse;
> +       }
> +
> +       priv->clk_iface = devm_clk_get(&pdev->dev, "iface");

Is this clock really optional?

> +       if (!IS_ERR(priv->clk_iface)) {
> +               ret = clk_prepare_enable(priv->clk_iface);
> +               if (ret < 0) {
> +                       dev_err(dev, "Failed to enable iface clock: %d\n", ret);
> +                       goto err_of_parse;
> +               }
> +       }
> +       priv->clk = devm_clk_get(&pdev->dev, "core");

Is this clock really optional?

> +       if (!IS_ERR(priv->clk)) {
> +               ret = clk_prepare_enable(priv->clk);
> +               if (ret < 0) {
> +                       dev_err(dev, "Failed to enable core clock: %d\n", ret);
> +                       goto err_clk;
> +               }
> +       }
> +
> +       pm_runtime_set_active(&pdev->dev);
> +       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);

As I stated earlier I think this is a strange behaviour of how to
implement runtime PM support. Could you elaborate one more time why
this actually is needed?

Moreover, if you insist of keeping the device runtime PM resumed
forever, you should replace the pm_runtime_get_sync() with a call to
pm_runtime_get_noresume() prior you invoke pm_runtime_set_active().

> +
> +       /* 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);
> +
> +       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);
> +
> +       reg = sdhci_readl(host, SDHCI_CAPABILITIES);
> +       if (reg & SDHCI_CAN_DO_8BIT)
> +               priv->vendor_hs200 = F_SDH30_EMMC_HS200;
> +       else
> +               priv->vendor_hs200 = 0;
> +
> +       ret = sdhci_add_host(host);
> +       if (ret) {
> +               dev_err(dev, "%s: host add error\n", __func__);

The printing is already taken care of by sdhci_add_host() no need to
do it here as well.

> +               goto err_add_host;
> +       }
> +
> +       return 0;
> +
> +err_add_host:
> +       pm_runtime_disable(&pdev->dev);
> +       pm_runtime_put_noidle(&pdev->dev);
> +       clk_disable_unprepare(priv->clk);
> +err_clk:
> +       clk_disable_unprepare(priv->clk_iface);

According to upper code, both clk_iface and clk are optional, thus you
need to check (!IS_ERR(clk*)) before gating them.

> +err_of_parse:
> +       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);
> +
> +       pm_runtime_get_sync(&pdev->dev);

If you keep the device runtime PM resumed forever, done by your
->probe() routine you don't need the above pm_runtime_get_sync()

> +       pm_runtime_disable(&pdev->dev);
> +       pm_runtime_put_noidle(&pdev->dev);
> +
> +       sdhci_remove_host(host, readl(host->ioaddr + SDHCI_INT_STATUS) ==
> +                         0xffffffff);
> +       release_mem_region(iomem->start, resource_size(iomem));

No need for this, you are using the devm_* managed functions.

> +
> +       clk_disable_unprepare(priv->clk_iface);
> +       clk_disable_unprepare(priv->clk);

According to probe the clocks are optional!?

> +
> +       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
> +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
> +
> +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)
> +};
> +
> +static const struct of_device_id f_sdh30_dt_ids[] = {
> +       { .compatible = "fujitsu,mb86s70-sdhci-3.0" },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, f_sdh30_dt_ids);
> +
> +static struct platform_driver sdhci_f_sdh30_driver = {
> +       .driver = {
> +               .name = "f_sdh30",
> +               .of_match_table = f_sdh30_dt_ids,
> +#ifdef CONFIG_PM_SLEEP

You can remove this ifdef.

> +               .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:f_sdh30");
> --
> 1.9.0
>

Kind regards
Uffe

[PATCH v3 4/4] mmc: sdhci: host: add new f_sdh30

[Ulf Hansson]

On 9 January 2015 at 12:41, Vincent Yang <vincent.yang.fujitsu@gmail.com> wrote:
> This patch adds new host controller driver for
> Fujitsu SDHCI controller f_sdh30.
>
> Signed-off-by: Andy Green <andy.green@linaro.org>
> Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
> Signed-off-by: Tetsuya Takinishi <t.takinishi@jp.fujitsu.com>
> ---
>  .../devicetree/bindings/mmc/sdhci-fujitsu.txt      |  30 ++
>  drivers/mmc/host/Kconfig                           |   8 +
>  drivers/mmc/host/Makefile                          |   1 +
>  drivers/mmc/host/sdhci_f_sdh30.c                   | 306 +++++++++++++++++++++
>  4 files changed, 345 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..d77df7b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
> @@ -0,0 +1,30 @@
> +* 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,mb86s70-sdhci-3.0"
> +
> +Optional properties:
> +- vqmmc-supply: phandle to the regulator device tree node, mentioned
> +  as the VCCQ/VDD_IO supply in the eMMC/SD specs.
> +- 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:
> +       "iface" - clock used for sdhci interface
> +       "core"  - core clock for sdhci controller
> +
> +Example:
> +
> +       sdhci1: mmc at 36600000 {
> +               compatible = "fujitsu,mb86s70-sdhci-3.0";
> +               reg = <0 0x36600000 0x1000>;
> +               interrupts = <0 172 0x4>,
> +                            <0 173 0x4>;
> +               bus-width = <4>;
> +               vqmmc-supply = <&vccq_sdhci1>;
> +               clocks = <&clock 2 2 0>, <&clock 2 3 0>;
> +               clock-names = "iface", "core";
> +       };
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 2d6fbdd..288dcc3 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -290,6 +290,14 @@ 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_PLTFM
> +       depends on OF
> +       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 f7b0a77..6a7cfe0 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -16,6 +16,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..2dc16e7
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci_f_sdh30.c
> @@ -0,0 +1,306 @@
> +/*
> + * linux/drivers/mmc/host/sdhci_f_sdh30.c
> + *
> + * Copyright (C) 2013 - 2015 Fujitsu Semiconductor, Ltd
> + *              Vincent Yang <vincent.yang@tw.fujitsu.com>
> + * Copyright (C) 2015 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>

You can likely remove some of these includes which you don't use. Like
sd.h, suspend.h, etc...

Also, I don't find clk.h

>
> +#include "sdhci.h"
> +#include "sdhci-pltfm.h"
> +
> +/* 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_iface;
> +       struct clk *clk;
> +       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);
> +
> +       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)
> +{
> +       if (sdhci_readw(host, SDHCI_CLOCK_CONTROL) == 0)
> +               sdhci_writew(host, 0xBC01, SDHCI_CLOCK_CONTROL);
> +
> +       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;
> +       struct resource *res;
> +       int irq, ctrl = 0, ret = 0;
> +       struct f_sdhost_priv *priv;
> +       u32 reg = 0;
> +
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq < 0) {
> +               dev_err(dev, "%s: no irq specified\n", __func__);
> +               ret = irq;
> +               goto err;

The goto is a superfluous, just do: return irq;

> +       }
> +
> +       host = sdhci_alloc_host(dev, sizeof(struct sdhci_host) +
> +                                               sizeof(struct f_sdhost_priv));
> +       if (IS_ERR(host)) {
> +               ret = -ENOMEM;
> +               goto err;

Replace by:
return PTR_ERR(host);

> +       }
> +       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_TUNING_WORK_AROUND;
> +
> +       ret = mmc_of_parse(host->mmc);
> +       if (ret)
> +               goto err_of_parse;
> +
> +       platform_set_drvdata(pdev, host);
> +
> +       sdhci_get_of_property(pdev);
> +       host->hw_name = "f_sdh30";
> +       host->ops = &sdhci_f_sdh30_ops;
> +       host->irq = irq;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       host->ioaddr = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(host->ioaddr)) {
> +               dev_err(dev, "%s: failed to remap registers\n", __func__);

The error print is not needed, since it's already done by
devm_ioremap_resource().

> +               ret = -ENXIO;

Don't overwrite the error, instead do: ret = PTR_ERR(host);

> +               goto err_of_parse;
> +       }
> +
> +       priv->clk_iface = devm_clk_get(&pdev->dev, "iface");

Is this clock really optional?

> +       if (!IS_ERR(priv->clk_iface)) {
> +               ret = clk_prepare_enable(priv->clk_iface);
> +               if (ret < 0) {
> +                       dev_err(dev, "Failed to enable iface clock: %d\n", ret);
> +                       goto err_of_parse;
> +               }
> +       }
> +       priv->clk = devm_clk_get(&pdev->dev, "core");

Is this clock really optional?

> +       if (!IS_ERR(priv->clk)) {
> +               ret = clk_prepare_enable(priv->clk);
> +               if (ret < 0) {
> +                       dev_err(dev, "Failed to enable core clock: %d\n", ret);
> +                       goto err_clk;
> +               }
> +       }
> +
> +       pm_runtime_set_active(&pdev->dev);
> +       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);

As I stated earlier I think this is a strange behaviour of how to
implement runtime PM support. Could you elaborate one more time why
this actually is needed?

Moreover, if you insist of keeping the device runtime PM resumed
forever, you should replace the pm_runtime_get_sync() with a call to
pm_runtime_get_noresume() prior you invoke pm_runtime_set_active().

> +
> +       /* 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);
> +
> +       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);
> +
> +       reg = sdhci_readl(host, SDHCI_CAPABILITIES);
> +       if (reg & SDHCI_CAN_DO_8BIT)
> +               priv->vendor_hs200 = F_SDH30_EMMC_HS200;
> +       else
> +               priv->vendor_hs200 = 0;
> +
> +       ret = sdhci_add_host(host);
> +       if (ret) {
> +               dev_err(dev, "%s: host add error\n", __func__);

The printing is already taken care of by sdhci_add_host() no need to
do it here as well.

> +               goto err_add_host;
> +       }
> +
> +       return 0;
> +
> +err_add_host:
> +       pm_runtime_disable(&pdev->dev);
> +       pm_runtime_put_noidle(&pdev->dev);
> +       clk_disable_unprepare(priv->clk);
> +err_clk:
> +       clk_disable_unprepare(priv->clk_iface);

According to upper code, both clk_iface and clk are optional, thus you
need to check (!IS_ERR(clk*)) before gating them.

> +err_of_parse:
> +       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);
> +
> +       pm_runtime_get_sync(&pdev->dev);

If you keep the device runtime PM resumed forever, done by your
->probe() routine you don't need the above pm_runtime_get_sync()

> +       pm_runtime_disable(&pdev->dev);
> +       pm_runtime_put_noidle(&pdev->dev);
> +
> +       sdhci_remove_host(host, readl(host->ioaddr + SDHCI_INT_STATUS) ==
> +                         0xffffffff);
> +       release_mem_region(iomem->start, resource_size(iomem));

No need for this, you are using the devm_* managed functions.

> +
> +       clk_disable_unprepare(priv->clk_iface);
> +       clk_disable_unprepare(priv->clk);

According to probe the clocks are optional!?

> +
> +       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
> +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
> +
> +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)
> +};
> +
> +static const struct of_device_id f_sdh30_dt_ids[] = {
> +       { .compatible = "fujitsu,mb86s70-sdhci-3.0" },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, f_sdh30_dt_ids);
> +
> +static struct platform_driver sdhci_f_sdh30_driver = {
> +       .driver = {
> +               .name = "f_sdh30",
> +               .of_match_table = f_sdh30_dt_ids,
> +#ifdef CONFIG_PM_SLEEP

You can remove this ifdef.

> +               .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:f_sdh30");
> --
> 1.9.0
>

Kind regards
Uffe

Re: [PATCH v3 4/4] mmc: sdhci: host: add new f_sdh30

[Vincent Yang]

2015-01-13 20:19 GMT+08:00 Ulf Hansson <ulf.hansson@linaro.org>:
> On 9 January 2015 at 12:41, Vincent Yang <vincent.yang.fujitsu@gmail.com> wrote:
>> This patch adds new host controller driver for
>> Fujitsu SDHCI controller f_sdh30.
>>
>> Signed-off-by: Andy Green <andy.green@linaro.org>
>> Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
>> Signed-off-by: Tetsuya Takinishi <t.takinishi@jp.fujitsu.com>
>> ---
>>  .../devicetree/bindings/mmc/sdhci-fujitsu.txt      |  30 ++
>>  drivers/mmc/host/Kconfig                           |   8 +
>>  drivers/mmc/host/Makefile                          |   1 +
>>  drivers/mmc/host/sdhci_f_sdh30.c                   | 306 +++++++++++++++++++++
>>  4 files changed, 345 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..d77df7b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
>> @@ -0,0 +1,30 @@
>> +* 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,mb86s70-sdhci-3.0"
>> +
>> +Optional properties:
>> +- vqmmc-supply: phandle to the regulator device tree node, mentioned
>> +  as the VCCQ/VDD_IO supply in the eMMC/SD specs.
>> +- 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:
>> +       "iface" - clock used for sdhci interface
>> +       "core"  - core clock for sdhci controller
>> +
>> +Example:
>> +
>> +       sdhci1: mmc@36600000 {
>> +               compatible = "fujitsu,mb86s70-sdhci-3.0";
>> +               reg = <0 0x36600000 0x1000>;
>> +               interrupts = <0 172 0x4>,
>> +                            <0 173 0x4>;
>> +               bus-width = <4>;
>> +               vqmmc-supply = <&vccq_sdhci1>;
>> +               clocks = <&clock 2 2 0>, <&clock 2 3 0>;
>> +               clock-names = "iface", "core";
>> +       };
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 2d6fbdd..288dcc3 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -290,6 +290,14 @@ 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_PLTFM
>> +       depends on OF
>> +       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 f7b0a77..6a7cfe0 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -16,6 +16,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..2dc16e7
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci_f_sdh30.c
>> @@ -0,0 +1,306 @@
>> +/*
>> + * linux/drivers/mmc/host/sdhci_f_sdh30.c
>> + *
>> + * Copyright (C) 2013 - 2015 Fujitsu Semiconductor, Ltd
>> + *              Vincent Yang <vincent.yang@tw.fujitsu.com>
>> + * Copyright (C) 2015 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>
>
> You can likely remove some of these includes which you don't use. Like
> sd.h, suspend.h, etc...
>
> Also, I don't find clk.h

Yes, I'll remove unnecessary ones and add clk.h

>
>>
>> +#include "sdhci.h"
>> +#include "sdhci-pltfm.h"
>> +
>> +/* 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_iface;
>> +       struct clk *clk;
>> +       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);
>> +
>> +       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)
>> +{
>> +       if (sdhci_readw(host, SDHCI_CLOCK_CONTROL) == 0)
>> +               sdhci_writew(host, 0xBC01, SDHCI_CLOCK_CONTROL);
>> +
>> +       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;
>> +       struct resource *res;
>> +       int irq, ctrl = 0, ret = 0;
>> +       struct f_sdhost_priv *priv;
>> +       u32 reg = 0;
>> +
>> +       irq = platform_get_irq(pdev, 0);
>> +       if (irq < 0) {
>> +               dev_err(dev, "%s: no irq specified\n", __func__);
>> +               ret = irq;
>> +               goto err;
>
> The goto is a superfluous, just do: return irq;

Yes, I'll do it.

>
>> +       }
>> +
>> +       host = sdhci_alloc_host(dev, sizeof(struct sdhci_host) +
>> +                                               sizeof(struct f_sdhost_priv));
>> +       if (IS_ERR(host)) {
>> +               ret = -ENOMEM;
>> +               goto err;
>
> Replace by:
> return PTR_ERR(host);

Yes, I'll do it.

>
>> +       }
>> +       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_TUNING_WORK_AROUND;
>> +
>> +       ret = mmc_of_parse(host->mmc);
>> +       if (ret)
>> +               goto err_of_parse;
>> +
>> +       platform_set_drvdata(pdev, host);
>> +
>> +       sdhci_get_of_property(pdev);
>> +       host->hw_name = "f_sdh30";
>> +       host->ops = &sdhci_f_sdh30_ops;
>> +       host->irq = irq;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       host->ioaddr = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(host->ioaddr)) {
>> +               dev_err(dev, "%s: failed to remap registers\n", __func__);
>
> The error print is not needed, since it's already done by
> devm_ioremap_resource().

Yes, I'll remove it.

>
>> +               ret = -ENXIO;
>
> Don't overwrite the error, instead do: ret = PTR_ERR(host);

Yes, I'll do it.

>
>> +               goto err_of_parse;
>> +       }
>> +
>> +       priv->clk_iface = devm_clk_get(&pdev->dev, "iface");
>
> Is this clock really optional?

I'm sorry about this mistake.
I double checked it and found it should be required instead of optional.
I'll also update device tree binding document accordingly.

>
>> +       if (!IS_ERR(priv->clk_iface)) {
>> +               ret = clk_prepare_enable(priv->clk_iface);
>> +               if (ret < 0) {
>> +                       dev_err(dev, "Failed to enable iface clock: %d\n", ret);
>> +                       goto err_of_parse;
>> +               }
>> +       }
>> +       priv->clk = devm_clk_get(&pdev->dev, "core");
>
> Is this clock really optional?

I'm sorry about this mistake.
It should also be required instead of optional.

>
>> +       if (!IS_ERR(priv->clk)) {
>> +               ret = clk_prepare_enable(priv->clk);
>> +               if (ret < 0) {
>> +                       dev_err(dev, "Failed to enable core clock: %d\n", ret);
>> +                       goto err_clk;
>> +               }
>> +       }
>> +
>> +       pm_runtime_set_active(&pdev->dev);
>> +       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);
>
> As I stated earlier I think this is a strange behaviour of how to
> implement runtime PM support. Could you elaborate one more time why
> this actually is needed?

Thanks for pointing out this.
We studied again and realized that this runtime PM support was only there for
powerdomain management, but we have not yet upstreamed the powerdomain
support. Thus we would like to remove it in next version.

>
> Moreover, if you insist of keeping the device runtime PM resumed
> forever, you should replace the pm_runtime_get_sync() with a call to
> pm_runtime_get_noresume() prior you invoke pm_runtime_set_active().

We will remove runtime PM support in next version.

>
>> +
>> +       /* 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);
>> +
>> +       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);
>> +
>> +       reg = sdhci_readl(host, SDHCI_CAPABILITIES);
>> +       if (reg & SDHCI_CAN_DO_8BIT)
>> +               priv->vendor_hs200 = F_SDH30_EMMC_HS200;
>> +       else
>> +               priv->vendor_hs200 = 0;
>> +
>> +       ret = sdhci_add_host(host);
>> +       if (ret) {
>> +               dev_err(dev, "%s: host add error\n", __func__);
>
> The printing is already taken care of by sdhci_add_host() no need to
> do it here as well.

Yes, I'll remove it.

>
>> +               goto err_add_host;
>> +       }
>> +
>> +       return 0;
>> +
>> +err_add_host:
>> +       pm_runtime_disable(&pdev->dev);
>> +       pm_runtime_put_noidle(&pdev->dev);
>> +       clk_disable_unprepare(priv->clk);
>> +err_clk:
>> +       clk_disable_unprepare(priv->clk_iface);
>
> According to upper code, both clk_iface and clk are optional, thus you
> need to check (!IS_ERR(clk*)) before gating them.

I'm sorry both clk_iface and clk should be required.
Thus I think I could skip this check.

>
>> +err_of_parse:
>> +       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);
>> +
>> +       pm_runtime_get_sync(&pdev->dev);
>
> If you keep the device runtime PM resumed forever, done by your
> ->probe() routine you don't need the above pm_runtime_get_sync()

Thanks for pointing out this.
I'll remove runtime PM support in next version.
All pm_runtime_*() functions will be removed.

>
>> +       pm_runtime_disable(&pdev->dev);
>> +       pm_runtime_put_noidle(&pdev->dev);
>> +
>> +       sdhci_remove_host(host, readl(host->ioaddr + SDHCI_INT_STATUS) ==
>> +                         0xffffffff);
>> +       release_mem_region(iomem->start, resource_size(iomem));
>
> No need for this, you are using the devm_* managed functions.

Yes, I'll remove it.

>
>> +
>> +       clk_disable_unprepare(priv->clk_iface);
>> +       clk_disable_unprepare(priv->clk);
>
> According to probe the clocks are optional!?

Sorry they should be required and I'll fix it in next version.

>
>> +
>> +       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
>> +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
>> +
>> +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)
>> +};
>> +
>> +static const struct of_device_id f_sdh30_dt_ids[] = {
>> +       { .compatible = "fujitsu,mb86s70-sdhci-3.0" },
>> +       { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, f_sdh30_dt_ids);
>> +
>> +static struct platform_driver sdhci_f_sdh30_driver = {
>> +       .driver = {
>> +               .name = "f_sdh30",
>> +               .of_match_table = f_sdh30_dt_ids,
>> +#ifdef CONFIG_PM_SLEEP
>
> You can remove this ifdef.

Yes, I'll remove it.
Thanks a lot for your review and comments!

Kind regards
Vincent

>
>> +               .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:f_sdh30");
>> --
>> 1.9.0
>>
>
> Kind regards
> Uffe

[PATCH v3 4/4] mmc: sdhci: host: add new f_sdh30

[Vincent Yang]

2015-01-13 20:19 GMT+08:00 Ulf Hansson <ulf.hansson@linaro.org>:
> On 9 January 2015 at 12:41, Vincent Yang <vincent.yang.fujitsu@gmail.com> wrote:
>> This patch adds new host controller driver for
>> Fujitsu SDHCI controller f_sdh30.
>>
>> Signed-off-by: Andy Green <andy.green@linaro.org>
>> Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
>> Signed-off-by: Tetsuya Takinishi <t.takinishi@jp.fujitsu.com>
>> ---
>>  .../devicetree/bindings/mmc/sdhci-fujitsu.txt      |  30 ++
>>  drivers/mmc/host/Kconfig                           |   8 +
>>  drivers/mmc/host/Makefile                          |   1 +
>>  drivers/mmc/host/sdhci_f_sdh30.c                   | 306 +++++++++++++++++++++
>>  4 files changed, 345 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..d77df7b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
>> @@ -0,0 +1,30 @@
>> +* 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,mb86s70-sdhci-3.0"
>> +
>> +Optional properties:
>> +- vqmmc-supply: phandle to the regulator device tree node, mentioned
>> +  as the VCCQ/VDD_IO supply in the eMMC/SD specs.
>> +- 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:
>> +       "iface" - clock used for sdhci interface
>> +       "core"  - core clock for sdhci controller
>> +
>> +Example:
>> +
>> +       sdhci1: mmc at 36600000 {
>> +               compatible = "fujitsu,mb86s70-sdhci-3.0";
>> +               reg = <0 0x36600000 0x1000>;
>> +               interrupts = <0 172 0x4>,
>> +                            <0 173 0x4>;
>> +               bus-width = <4>;
>> +               vqmmc-supply = <&vccq_sdhci1>;
>> +               clocks = <&clock 2 2 0>, <&clock 2 3 0>;
>> +               clock-names = "iface", "core";
>> +       };
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 2d6fbdd..288dcc3 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -290,6 +290,14 @@ 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_PLTFM
>> +       depends on OF
>> +       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 f7b0a77..6a7cfe0 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -16,6 +16,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..2dc16e7
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci_f_sdh30.c
>> @@ -0,0 +1,306 @@
>> +/*
>> + * linux/drivers/mmc/host/sdhci_f_sdh30.c
>> + *
>> + * Copyright (C) 2013 - 2015 Fujitsu Semiconductor, Ltd
>> + *              Vincent Yang <vincent.yang@tw.fujitsu.com>
>> + * Copyright (C) 2015 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>
>
> You can likely remove some of these includes which you don't use. Like
> sd.h, suspend.h, etc...
>
> Also, I don't find clk.h

Yes, I'll remove unnecessary ones and add clk.h

>
>>
>> +#include "sdhci.h"
>> +#include "sdhci-pltfm.h"
>> +
>> +/* 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_iface;
>> +       struct clk *clk;
>> +       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);
>> +
>> +       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)
>> +{
>> +       if (sdhci_readw(host, SDHCI_CLOCK_CONTROL) == 0)
>> +               sdhci_writew(host, 0xBC01, SDHCI_CLOCK_CONTROL);
>> +
>> +       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;
>> +       struct resource *res;
>> +       int irq, ctrl = 0, ret = 0;
>> +       struct f_sdhost_priv *priv;
>> +       u32 reg = 0;
>> +
>> +       irq = platform_get_irq(pdev, 0);
>> +       if (irq < 0) {
>> +               dev_err(dev, "%s: no irq specified\n", __func__);
>> +               ret = irq;
>> +               goto err;
>
> The goto is a superfluous, just do: return irq;

Yes, I'll do it.

>
>> +       }
>> +
>> +       host = sdhci_alloc_host(dev, sizeof(struct sdhci_host) +
>> +                                               sizeof(struct f_sdhost_priv));
>> +       if (IS_ERR(host)) {
>> +               ret = -ENOMEM;
>> +               goto err;
>
> Replace by:
> return PTR_ERR(host);

Yes, I'll do it.

>
>> +       }
>> +       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_TUNING_WORK_AROUND;
>> +
>> +       ret = mmc_of_parse(host->mmc);
>> +       if (ret)
>> +               goto err_of_parse;
>> +
>> +       platform_set_drvdata(pdev, host);
>> +
>> +       sdhci_get_of_property(pdev);
>> +       host->hw_name = "f_sdh30";
>> +       host->ops = &sdhci_f_sdh30_ops;
>> +       host->irq = irq;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       host->ioaddr = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(host->ioaddr)) {
>> +               dev_err(dev, "%s: failed to remap registers\n", __func__);
>
> The error print is not needed, since it's already done by
> devm_ioremap_resource().

Yes, I'll remove it.

>
>> +               ret = -ENXIO;
>
> Don't overwrite the error, instead do: ret = PTR_ERR(host);

Yes, I'll do it.

>
>> +               goto err_of_parse;
>> +       }
>> +
>> +       priv->clk_iface = devm_clk_get(&pdev->dev, "iface");
>
> Is this clock really optional?

I'm sorry about this mistake.
I double checked it and found it should be required instead of optional.
I'll also update device tree binding document accordingly.

>
>> +       if (!IS_ERR(priv->clk_iface)) {
>> +               ret = clk_prepare_enable(priv->clk_iface);
>> +               if (ret < 0) {
>> +                       dev_err(dev, "Failed to enable iface clock: %d\n", ret);
>> +                       goto err_of_parse;
>> +               }
>> +       }
>> +       priv->clk = devm_clk_get(&pdev->dev, "core");
>
> Is this clock really optional?

I'm sorry about this mistake.
It should also be required instead of optional.

>
>> +       if (!IS_ERR(priv->clk)) {
>> +               ret = clk_prepare_enable(priv->clk);
>> +               if (ret < 0) {
>> +                       dev_err(dev, "Failed to enable core clock: %d\n", ret);
>> +                       goto err_clk;
>> +               }
>> +       }
>> +
>> +       pm_runtime_set_active(&pdev->dev);
>> +       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);
>
> As I stated earlier I think this is a strange behaviour of how to
> implement runtime PM support. Could you elaborate one more time why
> this actually is needed?

Thanks for pointing out this.
We studied again and realized that this runtime PM support was only there for
powerdomain management, but we have not yet upstreamed the powerdomain
support. Thus we would like to remove it in next version.

>
> Moreover, if you insist of keeping the device runtime PM resumed
> forever, you should replace the pm_runtime_get_sync() with a call to
> pm_runtime_get_noresume() prior you invoke pm_runtime_set_active().

We will remove runtime PM support in next version.

>
>> +
>> +       /* 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);
>> +
>> +       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);
>> +
>> +       reg = sdhci_readl(host, SDHCI_CAPABILITIES);
>> +       if (reg & SDHCI_CAN_DO_8BIT)
>> +               priv->vendor_hs200 = F_SDH30_EMMC_HS200;
>> +       else
>> +               priv->vendor_hs200 = 0;
>> +
>> +       ret = sdhci_add_host(host);
>> +       if (ret) {
>> +               dev_err(dev, "%s: host add error\n", __func__);
>
> The printing is already taken care of by sdhci_add_host() no need to
> do it here as well.

Yes, I'll remove it.

>
>> +               goto err_add_host;
>> +       }
>> +
>> +       return 0;
>> +
>> +err_add_host:
>> +       pm_runtime_disable(&pdev->dev);
>> +       pm_runtime_put_noidle(&pdev->dev);
>> +       clk_disable_unprepare(priv->clk);
>> +err_clk:
>> +       clk_disable_unprepare(priv->clk_iface);
>
> According to upper code, both clk_iface and clk are optional, thus you
> need to check (!IS_ERR(clk*)) before gating them.

I'm sorry both clk_iface and clk should be required.
Thus I think I could skip this check.

>
>> +err_of_parse:
>> +       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);
>> +
>> +       pm_runtime_get_sync(&pdev->dev);
>
> If you keep the device runtime PM resumed forever, done by your
> ->probe() routine you don't need the above pm_runtime_get_sync()

Thanks for pointing out this.
I'll remove runtime PM support in next version.
All pm_runtime_*() functions will be removed.

>
>> +       pm_runtime_disable(&pdev->dev);
>> +       pm_runtime_put_noidle(&pdev->dev);
>> +
>> +       sdhci_remove_host(host, readl(host->ioaddr + SDHCI_INT_STATUS) ==
>> +                         0xffffffff);
>> +       release_mem_region(iomem->start, resource_size(iomem));
>
> No need for this, you are using the devm_* managed functions.

Yes, I'll remove it.

>
>> +
>> +       clk_disable_unprepare(priv->clk_iface);
>> +       clk_disable_unprepare(priv->clk);
>
> According to probe the clocks are optional!?

Sorry they should be required and I'll fix it in next version.

>
>> +
>> +       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
>> +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
>> +
>> +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)
>> +};
>> +
>> +static const struct of_device_id f_sdh30_dt_ids[] = {
>> +       { .compatible = "fujitsu,mb86s70-sdhci-3.0" },
>> +       { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, f_sdh30_dt_ids);
>> +
>> +static struct platform_driver sdhci_f_sdh30_driver = {
>> +       .driver = {
>> +               .name = "f_sdh30",
>> +               .of_match_table = f_sdh30_dt_ids,
>> +#ifdef CONFIG_PM_SLEEP
>
> You can remove this ifdef.

Yes, I'll remove it.
Thanks a lot for your review and comments!

Kind regards
Vincent

>
>> +               .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:f_sdh30");
>> --
>> 1.9.0
>>
>
> Kind regards
> Uffe

Re: [PATCH v3 4/4] mmc: sdhci: host: add new f_sdh30

[Ulf Hansson]

[...]

>>> +       pm_runtime_set_active(&pdev->dev);
>>> +       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);
>>
>> As I stated earlier I think this is a strange behaviour of how to
>> implement runtime PM support. Could you elaborate one more time why
>> this actually is needed?
>
> Thanks for pointing out this.
> We studied again and realized that this runtime PM support was only there for
> powerdomain management, but we have not yet upstreamed the powerdomain
> support. Thus we would like to remove it in next version.

I am fine with you removing the runtime PM support in the next
version. I also hope you to upstream the power domain support later
on, that would be nice.

A small note:
>From a driver perspective, you shall be able to implement runtime PM
support even if you haven't upstreamed the power domain support yet.
But, let's then deal with that as a separate patch later on.

Kind regards
Uffe

[PATCH v3 4/4] mmc: sdhci: host: add new f_sdh30

[Ulf Hansson]

[...]

>>> +       pm_runtime_set_active(&pdev->dev);
>>> +       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);
>>
>> As I stated earlier I think this is a strange behaviour of how to
>> implement runtime PM support. Could you elaborate one more time why
>> this actually is needed?
>
> Thanks for pointing out this.
> We studied again and realized that this runtime PM support was only there for
> powerdomain management, but we have not yet upstreamed the powerdomain
> support. Thus we would like to remove it in next version.

I am fine with you removing the runtime PM support in the next
version. I also hope you to upstream the power domain support later
on, that would be nice.

A small note:
>From a driver perspective, you shall be able to implement runtime PM
support even if you haven't upstreamed the power domain support yet.
But, let's then deal with that as a separate patch later on.

Kind regards
Uffe

Re: [PATCH v3 4/4] mmc: sdhci: host: add new f_sdh30

[Vincent Yang]

2015-01-16 16:36 GMT+08:00 Ulf Hansson <ulf.hansson@linaro.org>:
> [...]
>
>>>> +       pm_runtime_set_active(&pdev->dev);
>>>> +       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);
>>>
>>> As I stated earlier I think this is a strange behaviour of how to
>>> implement runtime PM support. Could you elaborate one more time why
>>> this actually is needed?
>>
>> Thanks for pointing out this.
>> We studied again and realized that this runtime PM support was only there for
>> powerdomain management, but we have not yet upstreamed the powerdomain
>> support. Thus we would like to remove it in next version.
>
> I am fine with you removing the runtime PM support in the next
> version. I also hope you to upstream the power domain support later
> on, that would be nice.
>
> A small note:
> From a driver perspective, you shall be able to implement runtime PM
> support even if you haven't upstreamed the power domain support yet.
> But, let's then deal with that as a separate patch later on.

Yes, I got it.
Thanks a lot for your review and comments!

Kind regards
Vincent

>
> Kind regards
> Uffe

[PATCH v3 4/4] mmc: sdhci: host: add new f_sdh30

[Vincent Yang]

2015-01-16 16:36 GMT+08:00 Ulf Hansson <ulf.hansson@linaro.org>:
> [...]
>
>>>> +       pm_runtime_set_active(&pdev->dev);
>>>> +       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);
>>>
>>> As I stated earlier I think this is a strange behaviour of how to
>>> implement runtime PM support. Could you elaborate one more time why
>>> this actually is needed?
>>
>> Thanks for pointing out this.
>> We studied again and realized that this runtime PM support was only there for
>> powerdomain management, but we have not yet upstreamed the powerdomain
>> support. Thus we would like to remove it in next version.
>
> I am fine with you removing the runtime PM support in the next
> version. I also hope you to upstream the power domain support later
> on, that would be nice.
>
> A small note:
> From a driver perspective, you shall be able to implement runtime PM
> support even if you haven't upstreamed the power domain support yet.
> But, let's then deal with that as a separate patch later on.

Yes, I got it.
Thanks a lot for your review and comments!

Kind regards
Vincent

>
> Kind regards
> Uffe