[PATCH v3 0/9] mmc: sdhci-msm: Add clk-rates and DDR support

[PATCH v3 0/9] mmc: sdhci-msm: Add clk-rates and DDR support

[Ritesh Harjani]

Hi, 

This is v3 version of the patch series.

Changes from v2 -> v3 :-
1. Addded Patch 01 based on Bjorn comment[2] - 
   This fixes/unrolls the poor coding style of read/writes of
   registers from base sdhci-msm driver.

2. Fixed/unrolled poor style of reads/writes of registers in Patch 02,
   based on Bjorn comment[2]. Also changed name of flag from
   use_updated_dll_reset -> use_14lpp_dll_reset.

Changes from v1->v2 :-
1. Removed patch 06 & 08 from v1 patch series[1]
(which were introducing unnecessary quirks).
   Instead have implemented __sdhci_msm_set_clock version of
   sdhci_set_clock in sdhci_msm driver itself in patch 07 of
   this patch series.
2. Enabled extra quirk (SDHCI_QUIRK2_PRESET_VALUE_BROKEN) in
   patch 05 of this patch series. 

Description of patches :-
This patchset adds clk-rates & other required changes to
upstream sdhci-msm driver from codeaurora tree.
It has been tested on a db410c Dragonboard and msm8996 based
platform.

Patch 0002 - Adds updated dll sequence for newer controllers
which has minor_version >= 0x42. This is required for msm8996.

MSM controller HW recommendation is to use the base MCI clock
and directly control this MCI clock at GCC in order to
change the clk-rate. 
Patches 03-07 bring in required change for this to
sdhci-msm and DT of db410c.

MSM controller would require 2x clock rate from source
for DDR bus speed modes. Patch 08 adds this support.

Patch 09 - adds DDR support in DT for sdhc1 of msm8916.

[1]:- http://www.spinics.net/lists/linux-mmc/msg38467.html
[2]:- http://www.spinics.net/lists/linux-mmc/msg38578.html 


Ritesh Harjani (8):
  mmc: sdhci-msm: Change poor style writel/readl of registers
  mmc: sdhci-msm: add pltfm_data support to get clk-rates from DT
  arm64: dts: qcom: msm8916: Add clk-rates to sdhc1 & sdhc2
  mmc: sdhci-msm: Add get_min_clock() and get_max_clock() callback
  mmc: sdhci-msm: Enable few quirks
  mmc: sdhci-msm: Implement set_clock callback for sdhci-msm
  mmc: sdhci-msm: Add clock changes for DDR mode.
  arm64: dts: qcom: msm8916: Add ddr support to sdhc1

Venkat Gopalakrishnan (1):
  mmc: sdhci-msm: Update DLL reset sequence

 .../devicetree/bindings/mmc/sdhci-msm.txt          |   1 +
 arch/arm64/boot/dts/qcom/msm8916.dtsi              |   5 +
 drivers/mmc/host/sdhci-msm.c                       | 313 +++++++++++++++++++--
 3 files changed, 295 insertions(+), 24 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project.

[PATCH v3 1/9] mmc: sdhci-msm: Change poor style writel/readl of registers

[Ritesh Harjani]

This patch changes the poor style of writel/readl registers
into more readable format. Also to avoid mixed style format
of readl/writel in sdhci-msm driver.

Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 54 ++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 8ef44a2a..42f42aa 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -137,8 +137,9 @@ static int msm_config_cm_dll_phase(struct sdhci_host *host, u8 phase)
 	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
 
 	/* Set CK_OUT_EN bit of DLL_CONFIG register to 1. */
-	writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
-			| CORE_CK_OUT_EN), host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config |= CORE_CK_OUT_EN;
+	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
 
 	/* Wait until CK_OUT_EN bit of DLL_CONFIG register becomes '1' */
 	rc = msm_dll_poll_ck_out_en(host, 1);
@@ -305,6 +306,7 @@ static int msm_init_cm_dll(struct sdhci_host *host)
 	struct mmc_host *mmc = host->mmc;
 	int wait_cnt = 50;
 	unsigned long flags;
+	u32 config = 0;
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -313,33 +315,40 @@ static int msm_init_cm_dll(struct sdhci_host *host)
 	 * tuning is in progress. Keeping PWRSAVE ON may
 	 * turn off the clock.
 	 */
-	writel_relaxed((readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC)
-			& ~CORE_CLK_PWRSAVE), host->ioaddr + CORE_VENDOR_SPEC);
+	config = readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC);
+	config &= ~CORE_CLK_PWRSAVE;
+	writel_relaxed(config, host->ioaddr + CORE_VENDOR_SPEC);
 
 	/* Write 1 to DLL_RST bit of DLL_CONFIG register */
-	writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
-			| CORE_DLL_RST), host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config |= CORE_DLL_RST;
+	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
 
 	/* Write 1 to DLL_PDN bit of DLL_CONFIG register */
-	writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
-			| CORE_DLL_PDN), host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config |= CORE_DLL_PDN;
+	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
 	msm_cm_dll_set_freq(host);
 
 	/* Write 0 to DLL_RST bit of DLL_CONFIG register */
-	writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
-			& ~CORE_DLL_RST), host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config &= ~CORE_DLL_RST;
+	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
 
 	/* Write 0 to DLL_PDN bit of DLL_CONFIG register */
-	writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
-			& ~CORE_DLL_PDN), host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config &= ~CORE_DLL_PDN;
+	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
 
 	/* Set DLL_EN bit to 1. */
-	writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
-			| CORE_DLL_EN), host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config |= CORE_DLL_EN;
+	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
 
 	/* Set CK_OUT_EN bit to 1. */
-	writel_relaxed((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG)
-			| CORE_CK_OUT_EN), host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config |= CORE_CK_OUT_EN;
+	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
 
 	/* Wait until DLL_LOCK bit of DLL_STATUS register becomes '1' */
 	while (!(readl_relaxed(host->ioaddr + CORE_DLL_STATUS) &
@@ -536,7 +545,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	struct resource *core_memres;
 	int ret;
 	u16 host_version, core_minor;
-	u32 core_version, caps;
+	u32 core_version, config;
 	u8 core_major;
 
 	host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
@@ -605,8 +614,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	}
 
 	/* Reset the core and Enable SDHC mode */
-	writel_relaxed(readl_relaxed(msm_host->core_mem + CORE_POWER) |
-		       CORE_SW_RST, msm_host->core_mem + CORE_POWER);
+	config = readl_relaxed(msm_host->core_mem + CORE_POWER);
+	config |= CORE_SW_RST;
+	writel_relaxed(config, msm_host->core_mem + CORE_POWER);
 
 	/* SW reset can take upto 10HCLK + 15MCLK cycles. (min 40us) */
 	usleep_range(1000, 5000);
@@ -636,9 +646,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	 * controller versions and must be explicitly enabled.
 	 */
 	if (core_major >= 1 && core_minor != 0x11 && core_minor != 0x12) {
-		caps = readl_relaxed(host->ioaddr + SDHCI_CAPABILITIES);
-		caps |= SDHCI_CAN_VDD_300 | SDHCI_CAN_DO_8BIT;
-		writel_relaxed(caps, host->ioaddr +
+		config = readl_relaxed(host->ioaddr + SDHCI_CAPABILITIES);
+		config |= SDHCI_CAN_VDD_300 | SDHCI_CAN_DO_8BIT;
+		writel_relaxed(config, host->ioaddr +
 			       CORE_VENDOR_SPEC_CAPABILITIES0);
 	}
 
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project.

[PATCH v3 2/9] mmc: sdhci-msm: Update DLL reset sequence

[Ritesh Harjani]

From: Venkat Gopalakrishnan <venkatg@codeaurora.org>

SDCC core with minor version >= 0x42 introduced new 14lpp
DLL. This has additional requirements in the reset sequence
for DLL tuning. Make necessary changes as needed.

Without this patch we see below errors on such SDHC controllers
	sdhci_msm 7464900.sdhci: mmc0: DLL failed to LOCK
	mmc0: tuning execution failed: -110

Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 42f42aa..85ddaae 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -58,11 +58,17 @@
 #define CORE_DLL_CONFIG		0x100
 #define CORE_DLL_STATUS		0x108
 
+#define CORE_DLL_CONFIG_2	0x1b4
+#define CORE_FLL_CYCLE_CNT	BIT(18)
+#define CORE_DLL_CLOCK_DISABLE	BIT(21)
+
 #define CORE_VENDOR_SPEC	0x10c
 #define CORE_CLK_PWRSAVE	BIT(1)
 
 #define CORE_VENDOR_SPEC_CAPABILITIES0	0x11c
 
+#define TCXO_FREQ		19200000
+
 #define CDR_SELEXT_SHIFT	20
 #define CDR_SELEXT_MASK		(0xf << CDR_SELEXT_SHIFT)
 #define CMUX_SHIFT_PHASE_SHIFT	24
@@ -76,6 +82,7 @@ struct sdhci_msm_host {
 	struct clk *pclk;	/* SDHC peripheral bus clock */
 	struct clk *bus_clk;	/* SDHC bus voter clock */
 	struct mmc_host *mmc;
+	bool use_14lpp_dll_reset;
 };
 
 /* Platform specific tuning */
@@ -304,6 +311,8 @@ static inline void msm_cm_dll_set_freq(struct sdhci_host *host)
 static int msm_init_cm_dll(struct sdhci_host *host)
 {
 	struct mmc_host *mmc = host->mmc;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
 	int wait_cnt = 50;
 	unsigned long flags;
 	u32 config = 0;
@@ -319,6 +328,16 @@ static int msm_init_cm_dll(struct sdhci_host *host)
 	config &= ~CORE_CLK_PWRSAVE;
 	writel_relaxed(config, host->ioaddr + CORE_VENDOR_SPEC);
 
+	if (msm_host->use_14lpp_dll_reset) {
+		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+		config &= ~CORE_CK_OUT_EN;
+		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+
+		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
+		config |= CORE_DLL_CLOCK_DISABLE;
+		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
+	}
+
 	/* Write 1 to DLL_RST bit of DLL_CONFIG register */
 	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
 	config |= CORE_DLL_RST;
@@ -330,6 +349,24 @@ static int msm_init_cm_dll(struct sdhci_host *host)
 	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
 	msm_cm_dll_set_freq(host);
 
+	if (msm_host->use_14lpp_dll_reset) {
+		u32 mclk_freq = 0;
+
+		if ((readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2)
+					& CORE_FLL_CYCLE_CNT))
+			mclk_freq = (u32) ((host->clock / TCXO_FREQ) * 8);
+		else
+			mclk_freq = (u32) ((host->clock / TCXO_FREQ) * 4);
+
+		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
+		config &= ~(0xFF << 10);
+		config |= mclk_freq << 10;
+
+		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
+		/* wait for 5us before enabling DLL clock */
+		udelay(5);
+	}
+
 	/* Write 0 to DLL_RST bit of DLL_CONFIG register */
 	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
 	config &= ~CORE_DLL_RST;
@@ -340,6 +377,14 @@ static int msm_init_cm_dll(struct sdhci_host *host)
 	config &= ~CORE_DLL_PDN;
 	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
 
+	if (msm_host->use_14lpp_dll_reset) {
+		msm_cm_dll_set_freq(host);
+		/* Enable the DLL clock */
+		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
+		config &= ~CORE_DLL_CLOCK_DISABLE;
+		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
+	}
+
 	/* Set DLL_EN bit to 1. */
 	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
 	config |= CORE_DLL_EN;
@@ -641,6 +686,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	dev_dbg(&pdev->dev, "MCI Version: 0x%08x, major: 0x%04x, minor: 0x%02x\n",
 		core_version, core_major, core_minor);
 
+	if ((core_major == 1) && (core_minor >= 0x42))
+		msm_host->use_14lpp_dll_reset = true;
+
 	/*
 	 * Support for some capabilities is not advertised by newer
 	 * controller versions and must be explicitly enabled.
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project.

[PATCH v3 3/9] mmc: sdhci-msm: add pltfm_data support to get clk-rates from DT

[Ritesh Harjani]

This adds support for sdhc-msm controllers to get supported
clk-rates from DT. sdhci-msm would need it's own set_clock
ops to be implemented. For this, supported clk-rates needs
to be populated in sdhci_msm_pltfm_data.

Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
---
 .../devicetree/bindings/mmc/sdhci-msm.txt          |  1 +
 drivers/mmc/host/sdhci-msm.c                       | 71 ++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
index 485483a..6a83b38 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -17,6 +17,7 @@ Required properties:
 	"iface" - Main peripheral bus clock (PCLK/HCLK - AHB Bus clock) (required)
 	"core"	- SDC MMC clock (MCLK) (required)
 	"bus"	- SDCC bus voter clock (optional)
+- clk-rates: Array of supported GCC clock frequencies for sdhc, Units - Hz.
 
 Example:
 
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 85ddaae..2bf141b 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -74,6 +74,11 @@
 #define CMUX_SHIFT_PHASE_SHIFT	24
 #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
 
+struct sdhci_msm_pltfm_data {
+	u32 *clk_table;
+	size_t clk_table_sz;
+};
+
 struct sdhci_msm_host {
 	struct platform_device *pdev;
 	void __iomem *core_mem;	/* MSM SDCC mapped address */
@@ -83,6 +88,7 @@ struct sdhci_msm_host {
 	struct clk *bus_clk;	/* SDHC bus voter clock */
 	struct mmc_host *mmc;
 	bool use_14lpp_dll_reset;
+	struct sdhci_msm_pltfm_data *pdata;
 };
 
 /* Platform specific tuning */
@@ -582,6 +588,67 @@ static const struct sdhci_pltfm_data sdhci_msm_pdata = {
 	.ops = &sdhci_msm_ops,
 };
 
+static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
+				u32 **table, size_t *size)
+{
+	struct device_node *np = dev->of_node;
+	int count = 0;
+	u32 *arr = NULL;
+	int ret = 0;
+
+	count = of_property_count_elems_of_size(np, prop_name, sizeof(u32));
+	if (count < 0) {
+		dev_err(dev, "%s: Invalid dt property, err(%d)\n",
+				prop_name, count);
+		ret = count;
+		goto out;
+	}
+
+	arr = devm_kzalloc(dev, count * sizeof(*arr), GFP_KERNEL);
+	if (!arr) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = of_property_read_u32_array(np, prop_name, arr, count);
+	if (ret) {
+		dev_err(dev, "%s Invalid dt array property, err(%d)\n",
+				prop_name, ret);
+		goto out;
+	}
+	*table = arr;
+	*size = count;
+out:
+	return ret;
+}
+
+static struct sdhci_msm_pltfm_data *sdhci_msm_populate_pdata(struct device *dev,
+						struct sdhci_msm_host *msm_host)
+{
+	struct sdhci_msm_pltfm_data *pdata = NULL;
+	size_t table_sz = 0;
+	u32 *table = NULL;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		goto out;
+
+	if (sdhci_msm_dt_get_array(dev, "clk-rates", &table, &table_sz)) {
+		dev_err(dev, "failed in DT parsing for supported clk-rates\n");
+		goto out;
+	}
+	if (!table || !table_sz) {
+		dev_err(dev, "Invalid clock table\n");
+		goto out;
+	}
+	pdata->clk_table = table;
+	pdata->clk_table_sz = table_sz;
+
+	return pdata;
+out:
+	return NULL;
+}
+
 static int sdhci_msm_probe(struct platform_device *pdev)
 {
 	struct sdhci_host *host;
@@ -608,6 +675,10 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 
 	sdhci_get_of_property(pdev);
 
+	msm_host->pdata = sdhci_msm_populate_pdata(&pdev->dev, msm_host);
+	if (!msm_host->pdata)
+		goto pltfm_free;
+
 	/* Setup SDCC bus voter clock. */
 	msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
 	if (!IS_ERR(msm_host->bus_clk)) {
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project.

[PATCH v3 4/9] arm64: dts: qcom: msm8916: Add clk-rates to sdhc1 & sdhc2

[Ritesh Harjani]

Add supported msm8916 supported clk-rates for sdhc1 & sdhc2
in DT.

Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 9681200..5161740 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -388,6 +388,8 @@
 			clocks = <&gcc GCC_SDCC1_APPS_CLK>,
 				 <&gcc GCC_SDCC1_AHB_CLK>;
 			clock-names = "core", "iface";
+			clk-rates = <400000 25000000 50000000 100000000
+					177770000>;
 			bus-width = <8>;
 			non-removable;
 			status = "disabled";
@@ -403,6 +405,8 @@
 			clocks = <&gcc GCC_SDCC2_APPS_CLK>,
 				 <&gcc GCC_SDCC2_AHB_CLK>;
 			clock-names = "core", "iface";
+			clk-rates = <400000 25000000 50000000 100000000
+					200000000>;
 			bus-width = <4>;
 			status = "disabled";
 		};
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project.

[PATCH v3 5/9] mmc: sdhci-msm: Add get_min_clock() and get_max_clock() callback

[Ritesh Harjani]

This add get_min_clock() and get_max_clock() callback
for sdhci-msm. sdhci-msm min/max clocks may be different
hence implement these callbacks.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 2bf141b..4974079 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -565,6 +565,23 @@ static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static unsigned int sdhci_msm_get_max_clock(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	int count = msm_host->pdata->clk_table_sz;
+
+	return msm_host->pdata->clk_table[count - 1];
+}
+
+static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+
+	return msm_host->pdata->clk_table[0];
+}
+
 static const struct of_device_id sdhci_msm_dt_match[] = {
 	{ .compatible = "qcom,sdhci-msm-v4" },
 	{},
@@ -576,6 +593,8 @@ static const struct sdhci_ops sdhci_msm_ops = {
 	.platform_execute_tuning = sdhci_msm_execute_tuning,
 	.reset = sdhci_reset,
 	.set_clock = sdhci_set_clock,
+	.get_min_clock = sdhci_msm_get_min_clock,
+	.get_max_clock = sdhci_msm_get_max_clock,
 	.set_bus_width = sdhci_set_bus_width,
 	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
 	.voltage_switch = sdhci_msm_voltage_switch,
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project.

[PATCH v3 6/9] mmc: sdhci-msm: Enable few quirks

[Ritesh Harjani]

sdhc-msm controller needs this SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN
& SDHCI_QUIRK2_PRESET_VALUE_BROKEN to be set. Hence setting it.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 4974079..7c032c3 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -603,7 +603,9 @@ static const struct sdhci_ops sdhci_msm_ops = {
 static const struct sdhci_pltfm_data sdhci_msm_pdata = {
 	.quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |
 		  SDHCI_QUIRK_NO_CARD_NO_RESET |
-		  SDHCI_QUIRK_SINGLE_POWER_WRITE,
+		  SDHCI_QUIRK_SINGLE_POWER_WRITE |
+		  SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
 	.ops = &sdhci_msm_ops,
 };
 
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project.

[PATCH v3 7/9] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm

[Ritesh Harjani]

sdhci-msm controller may have different clk-rates for each
bus speed mode. Thus implement set_clock callback for
sdhci-msm driver.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 103 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 7c032c3..c0ad9c2 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -89,6 +89,7 @@ struct sdhci_msm_host {
 	struct mmc_host *mmc;
 	bool use_14lpp_dll_reset;
 	struct sdhci_msm_pltfm_data *pdata;
+	u32 clk_rate;
 };
 
 /* Platform specific tuning */
@@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
 	return msm_host->pdata->clk_table[0];
 }
 
+static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host,
+					u32 req_clk)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	int count = msm_host->pdata->clk_table_sz;
+	unsigned int sel_clk = -1;
+	int cnt;
+
+	if (req_clk < sdhci_msm_get_min_clock(host)) {
+		sel_clk = sdhci_msm_get_min_clock(host);
+		return sel_clk;
+	}
+
+	for (cnt = 0; cnt < count; cnt++) {
+		if (msm_host->pdata->clk_table[cnt] > req_clk) {
+			break;
+		} else if (msm_host->pdata->clk_table[cnt] == req_clk) {
+			sel_clk = msm_host->pdata->clk_table[cnt];
+			break;
+		} else {
+			sel_clk = msm_host->pdata->clk_table[cnt];
+		}
+	}
+	return sel_clk;
+}
+/**
+ * __sdhci_msm_set_clock - sdhci_msm clock control.
+ *
+ * Description:
+ * Implement MSM version of sdhci_set_clock.
+ * This is required since MSM controller does not
+ * use internal divider and instead directly control
+ * the GCC clock as per HW recommendation.
+ **/
+void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	u16 clk;
+	unsigned long timeout;
+
+	host->mmc->actual_clock = 0;
+
+	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
+
+	if (clock == 0)
+		return;
+
+	/*
+	 * MSM controller do not use clock divider.
+	 * Thus read SDHCI_CLOCK_CONTROL and only enable
+	 * clock with no divider value programmed.
+	 */
+	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+
+	clk |= SDHCI_CLOCK_INT_EN;
+	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+
+	/* Wait max 20 ms */
+	timeout = 20;
+	while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
+		& SDHCI_CLOCK_INT_STABLE)) {
+		if (timeout == 0) {
+			pr_err("%s: Internal clock never stabilised.\n",
+			       mmc_hostname(host->mmc));
+			return;
+		}
+		timeout--;
+		mdelay(1);
+	}
+
+	clk |= SDHCI_CLOCK_CARD_EN;
+	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+}
+
+static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	u32 msm_clock = 0;
+	int rc = 0;
+
+	if (!clock)
+		goto out;
+
+	if (clock != msm_host->clk_rate) {
+		msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
+		rc = clk_set_rate(msm_host->clk, msm_clock);
+		if (rc) {
+			pr_err("%s: failed to set clock at rate %u, requested clock rate %u\n",
+				mmc_hostname(host->mmc), msm_clock, clock);
+			goto out;
+		}
+		msm_host->clk_rate = clock;
+		pr_debug("%s: setting clock at rate %lu\n",
+			mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
+	}
+out:
+	__sdhci_msm_set_clock(host, clock);
+}
+
 static const struct of_device_id sdhci_msm_dt_match[] = {
 	{ .compatible = "qcom,sdhci-msm-v4" },
 	{},
@@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
 static const struct sdhci_ops sdhci_msm_ops = {
 	.platform_execute_tuning = sdhci_msm_execute_tuning,
 	.reset = sdhci_reset,
-	.set_clock = sdhci_set_clock,
+	.set_clock = sdhci_msm_set_clock,
 	.get_min_clock = sdhci_msm_get_min_clock,
 	.get_max_clock = sdhci_msm_get_max_clock,
 	.set_bus_width = sdhci_set_bus_width,
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project.

[PATCH v3 8/9] mmc: sdhci-msm: Add clock changes for DDR mode.

[Ritesh Harjani]

SDHC MSM controller need 2x clock for MCLK at GCC.
Hence make required changes to have 2x clock for
DDR timing modes.

Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index c0ad9c2..f0e6293 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -661,21 +661,35 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
-	u32 msm_clock = 0;
+	struct mmc_ios curr_ios = host->mmc->ios;
+	u32 msm_clock = clock, ddr_clock = 0;
 	int rc = 0;
 
 	if (!clock)
 		goto out;
 
-	if (clock != msm_host->clk_rate) {
-		msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
+	msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
+	if ((curr_ios.timing == MMC_TIMING_UHS_DDR50) ||
+		(curr_ios.timing == MMC_TIMING_MMC_DDR52) ||
+		(curr_ios.timing == MMC_TIMING_MMC_HS400)) {
+		/*
+		 * The SDHC requires internal clock frequency to be double the
+		 * actual clock that will be set for DDR mode. The controller
+		 * uses the faster clock(100/400MHz) for some of its parts and
+		 * send the actual required clock (50/200MHz) to the card.
+		 */
+		ddr_clock = clock * 2;
+		msm_clock = sdhci_msm_get_msm_clk_rate(host, ddr_clock);
+	}
+
+	if (msm_clock != msm_host->clk_rate) {
 		rc = clk_set_rate(msm_host->clk, msm_clock);
 		if (rc) {
 			pr_err("%s: failed to set clock at rate %u, requested clock rate %u\n",
 				mmc_hostname(host->mmc), msm_clock, clock);
 			goto out;
 		}
-		msm_host->clk_rate = clock;
+		msm_host->clk_rate = msm_clock;
 		pr_debug("%s: setting clock at rate %lu\n",
 			mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
 	}
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project.

[PATCH v3 9/9] arm64: dts: qcom: msm8916: Add ddr support to sdhc1

[Ritesh Harjani]

This adds mmc-ddr-1_8v support to DT for sdhc1 of msm8916.

Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 5161740..514c61e 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -390,6 +390,7 @@
 			clock-names = "core", "iface";
 			clk-rates = <400000 25000000 50000000 100000000
 					177770000>;
+			mmc-ddr-1_8v;
 			bus-width = <8>;
 			non-removable;
 			status = "disabled";
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
a Linux Foundation Collaborative Project.

Re: [PATCH v3 1/9] mmc: sdhci-msm: Change poor style writel/readl of registers

[Adrian Hunter]

On 19/08/16 07:36, Ritesh Harjani wrote:
> This patch changes the poor style of writel/readl registers
> into more readable format. Also to avoid mixed style format
> of readl/writel in sdhci-msm driver.
> 
> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Re: [PATCH v3 2/9] mmc: sdhci-msm: Update DLL reset sequence

[Adrian Hunter]

On 19/08/16 07:36, Ritesh Harjani wrote:
> From: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> 
> SDCC core with minor version >= 0x42 introduced new 14lpp
> DLL. This has additional requirements in the reset sequence
> for DLL tuning. Make necessary changes as needed.
> 
> Without this patch we see below errors on such SDHC controllers
> 	sdhci_msm 7464900.sdhci: mmc0: DLL failed to LOCK
> 	mmc0: tuning execution failed: -110
> 
> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Re: [PATCH v3 3/9] mmc: sdhci-msm: add pltfm_data support to get clk-rates from DT

[Adrian Hunter]

On 19/08/16 07:36, Ritesh Harjani wrote:
> This adds support for sdhc-msm controllers to get supported
> clk-rates from DT. sdhci-msm would need it's own set_clock
> ops to be implemented. For this, supported clk-rates needs
> to be populated in sdhci_msm_pltfm_data.
> 
> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
> ---
>  .../devicetree/bindings/mmc/sdhci-msm.txt          |  1 +
>  drivers/mmc/host/sdhci-msm.c                       | 71 ++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> index 485483a..6a83b38 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> @@ -17,6 +17,7 @@ Required properties:
>  	"iface" - Main peripheral bus clock (PCLK/HCLK - AHB Bus clock) (required)
>  	"core"	- SDC MMC clock (MCLK) (required)
>  	"bus"	- SDCC bus voter clock (optional)
> +- clk-rates: Array of supported GCC clock frequencies for sdhc, Units - Hz.
>  
>  Example:
>  
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 85ddaae..2bf141b 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -74,6 +74,11 @@
>  #define CMUX_SHIFT_PHASE_SHIFT	24
>  #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
>  
> +struct sdhci_msm_pltfm_data {
> +	u32 *clk_table;
> +	size_t clk_table_sz;

>From of_property_count_elems_of_size(), clk_table_sz is an 'int' and is used
as an 'int' in later patches.  So it can be an 'int' everywhere.

> +};
> +
>  struct sdhci_msm_host {
>  	struct platform_device *pdev;
>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
> @@ -83,6 +88,7 @@ struct sdhci_msm_host {
>  	struct clk *bus_clk;	/* SDHC bus voter clock */
>  	struct mmc_host *mmc;
>  	bool use_14lpp_dll_reset;
> +	struct sdhci_msm_pltfm_data *pdata;
>  };
>  
>  /* Platform specific tuning */
> @@ -582,6 +588,67 @@ static const struct sdhci_pltfm_data sdhci_msm_pdata = {
>  	.ops = &sdhci_msm_ops,
>  };
>  
> +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
> +				u32 **table, size_t *size)

Change 'size_t *size' to be 'int *size'

> +{
> +	struct device_node *np = dev->of_node;
> +	int count = 0;
> +	u32 *arr = NULL;
> +	int ret = 0;

Initialization of count and arr is not needed.

> +
> +	count = of_property_count_elems_of_size(np, prop_name, sizeof(u32));
> +	if (count < 0) {
> +		dev_err(dev, "%s: Invalid dt property, err(%d)\n",
> +				prop_name, count);
> +		ret = count;
> +		goto out;

Just return count

> +	}
> +
> +	arr = devm_kzalloc(dev, count * sizeof(*arr), GFP_KERNEL);

Better to use devm_kcalloc here

> +	if (!arr) {
> +		ret = -ENOMEM;
> +		goto out;

Just return -ENOMEM

> +	}
> +
> +	ret = of_property_read_u32_array(np, prop_name, arr, count);
> +	if (ret) {
> +		dev_err(dev, "%s Invalid dt array property, err(%d)\n",
> +				prop_name, ret);
> +		goto out;

Just return ret

> +	}
> +	*table = arr;
> +	*size = count;
> +out:
> +	return ret;

Then the label 'out:' is not needed, nor is the initialization of 'ret' and
here just return 0.

> +}
> +
> +static struct sdhci_msm_pltfm_data *sdhci_msm_populate_pdata(struct device *dev,
> +						struct sdhci_msm_host *msm_host)
> +{
> +	struct sdhci_msm_pltfm_data *pdata = NULL;
> +	size_t table_sz = 0;

table_sz is an 'int' elsewhere.  To keep it all the same type make it an
'int' here too.

> +	u32 *table = NULL;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		goto out;

Here and below just return NULL and then the label out: is not needed.

> +
> +	if (sdhci_msm_dt_get_array(dev, "clk-rates", &table, &table_sz)) {
> +		dev_err(dev, "failed in DT parsing for supported clk-rates\n");
> +		goto out;
> +	}
> +	if (!table || !table_sz) {
> +		dev_err(dev, "Invalid clock table\n");
> +		goto out;
> +	}
> +	pdata->clk_table = table;
> +	pdata->clk_table_sz = table_sz;
> +
> +	return pdata;
> +out:
> +	return NULL;
> +}
> +
>  static int sdhci_msm_probe(struct platform_device *pdev)
>  {
>  	struct sdhci_host *host;
> @@ -608,6 +675,10 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  
>  	sdhci_get_of_property(pdev);
>  
> +	msm_host->pdata = sdhci_msm_populate_pdata(&pdev->dev, msm_host);
> +	if (!msm_host->pdata)
> +		goto pltfm_free;
> +
>  	/* Setup SDCC bus voter clock. */
>  	msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
>  	if (!IS_ERR(msm_host->bus_clk)) {
> 

Re: [PATCH v3 5/9] mmc: sdhci-msm: Add get_min_clock() and get_max_clock() callback

[Adrian Hunter]

On 19/08/16 07:36, Ritesh Harjani wrote:
> This add get_min_clock() and get_max_clock() callback
> for sdhci-msm. sdhci-msm min/max clocks may be different
> hence implement these callbacks.
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Re: [PATCH v3 6/9] mmc: sdhci-msm: Enable few quirks

[Adrian Hunter]

On 19/08/16 07:36, Ritesh Harjani wrote:
> sdhc-msm controller needs this SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN
> & SDHCI_QUIRK2_PRESET_VALUE_BROKEN to be set. Hence setting it.
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Re: [PATCH v3 7/9] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm

[Adrian Hunter]

On 19/08/16 07:36, Ritesh Harjani wrote:
> sdhci-msm controller may have different clk-rates for each
> bus speed mode. Thus implement set_clock callback for
> sdhci-msm driver.
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 103 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 102 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 7c032c3..c0ad9c2 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -89,6 +89,7 @@ struct sdhci_msm_host {
>  	struct mmc_host *mmc;
>  	bool use_14lpp_dll_reset;
>  	struct sdhci_msm_pltfm_data *pdata;
> +	u32 clk_rate;
>  };
>  
>  /* Platform specific tuning */
> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
>  	return msm_host->pdata->clk_table[0];
>  }
>  
> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host,
> +					u32 req_clk)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	int count = msm_host->pdata->clk_table_sz;
> +	unsigned int sel_clk = -1;
> +	int cnt;
> +
> +	if (req_clk < sdhci_msm_get_min_clock(host)) {
> +		sel_clk = sdhci_msm_get_min_clock(host);
> +		return sel_clk;
> +	}
> +
> +	for (cnt = 0; cnt < count; cnt++) {
> +		if (msm_host->pdata->clk_table[cnt] > req_clk) {
> +			break;
> +		} else if (msm_host->pdata->clk_table[cnt] == req_clk) {
> +			sel_clk = msm_host->pdata->clk_table[cnt];
> +			break;
> +		} else {
> +			sel_clk = msm_host->pdata->clk_table[cnt];
> +		}
> +	}

'else' is not needed after 'break' but can't this be simpler e.g.

static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host *msm_host, u32 req_clk)
{
	int count = msm_host->pdata->clk_table_sz;
	unsigned int sel_clk = -1;

	while (count--) {
		sel_clk = msm_host->pdata->clk_table[count];
		if (req_clk >= sel_clk)
			return sel_clk;
	}

	return sel_clk;
}


> +	return sel_clk;
> +}

Blank line needed

> +/**
> + * __sdhci_msm_set_clock - sdhci_msm clock control.
> + *
> + * Description:
> + * Implement MSM version of sdhci_set_clock.
> + * This is required since MSM controller does not
> + * use internal divider and instead directly control
> + * the GCC clock as per HW recommendation.
> + **/
> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> +	u16 clk;
> +	unsigned long timeout;
> +
> +	host->mmc->actual_clock = 0;
> +
> +	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> +
> +	if (clock == 0)
> +		return;

Should set host->mmc->actual_clock to the actual rate somewhere.

> +
> +	/*
> +	 * MSM controller do not use clock divider.
> +	 * Thus read SDHCI_CLOCK_CONTROL and only enable
> +	 * clock with no divider value programmed.
> +	 */
> +	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +
> +	clk |= SDHCI_CLOCK_INT_EN;
> +	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +
> +	/* Wait max 20 ms */
> +	timeout = 20;
> +	while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
> +		& SDHCI_CLOCK_INT_STABLE)) {
> +		if (timeout == 0) {
> +			pr_err("%s: Internal clock never stabilised.\n",
> +			       mmc_hostname(host->mmc));
> +			return;
> +		}
> +		timeout--;
> +		mdelay(1);
> +	}
> +
> +	clk |= SDHCI_CLOCK_CARD_EN;
> +	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +}
> +
> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	u32 msm_clock = 0;
> +	int rc = 0;
> +
> +	if (!clock)
> +		goto out;
> +
> +	if (clock != msm_host->clk_rate) {
> +		msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
> +		rc = clk_set_rate(msm_host->clk, msm_clock);
> +		if (rc) {
> +			pr_err("%s: failed to set clock at rate %u, requested clock rate %u\n",
> +				mmc_hostname(host->mmc), msm_clock, clock);
> +			goto out;
> +		}
> +		msm_host->clk_rate = clock;
> +		pr_debug("%s: setting clock at rate %lu\n",
> +			mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
> +	}
> +out:
> +	__sdhci_msm_set_clock(host, clock);
> +}
> +
>  static const struct of_device_id sdhci_msm_dt_match[] = {
>  	{ .compatible = "qcom,sdhci-msm-v4" },
>  	{},
> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>  static const struct sdhci_ops sdhci_msm_ops = {
>  	.platform_execute_tuning = sdhci_msm_execute_tuning,
>  	.reset = sdhci_reset,
> -	.set_clock = sdhci_set_clock,
> +	.set_clock = sdhci_msm_set_clock,
>  	.get_min_clock = sdhci_msm_get_min_clock,
>  	.get_max_clock = sdhci_msm_get_max_clock,
>  	.set_bus_width = sdhci_set_bus_width,
> 

Re: [PATCH v3 8/9] mmc: sdhci-msm: Add clock changes for DDR mode.

[Adrian Hunter]

On 19/08/16 07:36, Ritesh Harjani wrote:
> SDHC MSM controller need 2x clock for MCLK at GCC.
> Hence make required changes to have 2x clock for
> DDR timing modes.
> 
> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index c0ad9c2..f0e6293 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -661,21 +661,35 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> -	u32 msm_clock = 0;
> +	struct mmc_ios curr_ios = host->mmc->ios;
> +	u32 msm_clock = clock, ddr_clock = 0;

Don't need to initialize msm_clock or ddr_clock

>  	int rc = 0;
>  
>  	if (!clock)
>  		goto out;
>  
> -	if (clock != msm_host->clk_rate) {
> -		msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
> +	msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
> +	if ((curr_ios.timing == MMC_TIMING_UHS_DDR50) ||
> +		(curr_ios.timing == MMC_TIMING_MMC_DDR52) ||
> +		(curr_ios.timing == MMC_TIMING_MMC_HS400)) {
> +		/*
> +		 * The SDHC requires internal clock frequency to be double the
> +		 * actual clock that will be set for DDR mode. The controller
> +		 * uses the faster clock(100/400MHz) for some of its parts and
> +		 * send the actual required clock (50/200MHz) to the card.
> +		 */
> +		ddr_clock = clock * 2;
> +		msm_clock = sdhci_msm_get_msm_clk_rate(host, ddr_clock);
> +	}
> +
> +	if (msm_clock != msm_host->clk_rate) {
>  		rc = clk_set_rate(msm_host->clk, msm_clock);
>  		if (rc) {
>  			pr_err("%s: failed to set clock at rate %u, requested clock rate %u\n",
>  				mmc_hostname(host->mmc), msm_clock, clock);
>  			goto out;
>  		}
> -		msm_host->clk_rate = clock;
> +		msm_host->clk_rate = msm_clock;
>  		pr_debug("%s: setting clock at rate %lu\n",
>  			mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
>  	}
> 

Re: [PATCH v3 8/9] mmc: sdhci-msm: Add clock changes for DDR mode.

[Ritesh Harjani]

Hi Adrian,

Thanks for the review.

On 8/19/2016 6:34 PM, Adrian Hunter wrote:
> On 19/08/16 07:36, Ritesh Harjani wrote:
>> SDHC MSM controller need 2x clock for MCLK at GCC.
>> Hence make required changes to have 2x clock for
>> DDR timing modes.
>>
>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>> ---
>>  drivers/mmc/host/sdhci-msm.c | 22 ++++++++++++++++++----
>>  1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index c0ad9c2..f0e6293 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -661,21 +661,35 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>  {
>>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>  	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> -	u32 msm_clock = 0;
>> +	struct mmc_ios curr_ios = host->mmc->ios;
>> +	u32 msm_clock = clock, ddr_clock = 0;
>
> Don't need to initialize msm_clock or ddr_clock
Sure. Will do that.


>
>>  	int rc = 0;
>>
>>  	if (!clock)
>>  		goto out;
>>
>> -	if (clock != msm_host->clk_rate) {
>> -		msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
>> +	msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
>> +	if ((curr_ios.timing == MMC_TIMING_UHS_DDR50) ||
>> +		(curr_ios.timing == MMC_TIMING_MMC_DDR52) ||
>> +		(curr_ios.timing == MMC_TIMING_MMC_HS400)) {
>> +		/*
>> +		 * The SDHC requires internal clock frequency to be double the
>> +		 * actual clock that will be set for DDR mode. The controller
>> +		 * uses the faster clock(100/400MHz) for some of its parts and
>> +		 * send the actual required clock (50/200MHz) to the card.
>> +		 */
>> +		ddr_clock = clock * 2;
>> +		msm_clock = sdhci_msm_get_msm_clk_rate(host, ddr_clock);
>> +	}
>> +
>> +	if (msm_clock != msm_host->clk_rate) {
>>  		rc = clk_set_rate(msm_host->clk, msm_clock);
>>  		if (rc) {
>>  			pr_err("%s: failed to set clock at rate %u, requested clock rate %u\n",
>>  				mmc_hostname(host->mmc), msm_clock, clock);
>>  			goto out;
>>  		}
>> -		msm_host->clk_rate = clock;
>> +		msm_host->clk_rate = msm_clock;
>>  		pr_debug("%s: setting clock at rate %lu\n",
>>  			mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
>>  	}
>>
>

Re: [PATCH v3 7/9] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm

[Ritesh Harjani]

Hi,


On 8/19/2016 6:34 PM, Adrian Hunter wrote:
> On 19/08/16 07:36, Ritesh Harjani wrote:
>> sdhci-msm controller may have different clk-rates for each
>> bus speed mode. Thus implement set_clock callback for
>> sdhci-msm driver.
>>
>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>> ---
>>  drivers/mmc/host/sdhci-msm.c | 103 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 102 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 7c032c3..c0ad9c2 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -89,6 +89,7 @@ struct sdhci_msm_host {
>>  	struct mmc_host *mmc;
>>  	bool use_14lpp_dll_reset;
>>  	struct sdhci_msm_pltfm_data *pdata;
>> +	u32 clk_rate;
>>  };
>>
>>  /* Platform specific tuning */
>> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
>>  	return msm_host->pdata->clk_table[0];
>>  }
>>
>> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host,
>> +					u32 req_clk)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +	int count = msm_host->pdata->clk_table_sz;
>> +	unsigned int sel_clk = -1;
>> +	int cnt;
>> +
>> +	if (req_clk < sdhci_msm_get_min_clock(host)) {
>> +		sel_clk = sdhci_msm_get_min_clock(host);
>> +		return sel_clk;
>> +	}
>> +
>> +	for (cnt = 0; cnt < count; cnt++) {
>> +		if (msm_host->pdata->clk_table[cnt] > req_clk) {
>> +			break;
>> +		} else if (msm_host->pdata->clk_table[cnt] == req_clk) {
>> +			sel_clk = msm_host->pdata->clk_table[cnt];
>> +			break;
>> +		} else {
>> +			sel_clk = msm_host->pdata->clk_table[cnt];
>> +		}
>> +	}
>
> 'else' is not needed after 'break' but can't this be simpler e.g.
>
> static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host *msm_host, u32 req_clk)
> {
> 	int count = msm_host->pdata->clk_table_sz;
> 	unsigned int sel_clk = -1;
>
> 	while (count--) {
> 		sel_clk = msm_host->pdata->clk_table[count];
> 		if (req_clk >= sel_clk)
> 			return sel_clk;
> 	}
>
> 	return sel_clk;
> }

Ok, sure I will check and get back on this.


>
>
>> +	return sel_clk;
>> +}
>
> Blank line needed
Ok done.

>
>> +/**
>> + * __sdhci_msm_set_clock - sdhci_msm clock control.
>> + *
>> + * Description:
>> + * Implement MSM version of sdhci_set_clock.
>> + * This is required since MSM controller does not
>> + * use internal divider and instead directly control
>> + * the GCC clock as per HW recommendation.
>> + **/
>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>> +{
>> +	u16 clk;
>> +	unsigned long timeout;
>> +
>> +	host->mmc->actual_clock = 0;
>> +
>> +	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>> +
>> +	if (clock == 0)
>> +		return;
>
> Should set host->mmc->actual_clock to the actual rate somewhere.
Since MSM controller does not uses divider then there is no need of 
having actual clock since it should be same as host->clock.

That's why I had kept it 0 intentionally. But I will add a comment
here then.

>
>> +
>> +	/*
>> +	 * MSM controller do not use clock divider.
>> +	 * Thus read SDHCI_CLOCK_CONTROL and only enable
>> +	 * clock with no divider value programmed.
>> +	 */
>> +	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>> +
>> +	clk |= SDHCI_CLOCK_INT_EN;
>> +	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>> +
>> +	/* Wait max 20 ms */
>> +	timeout = 20;
>> +	while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>> +		& SDHCI_CLOCK_INT_STABLE)) {
>> +		if (timeout == 0) {
>> +			pr_err("%s: Internal clock never stabilised.\n",
>> +			       mmc_hostname(host->mmc));
>> +			return;
>> +		}
>> +		timeout--;
>> +		mdelay(1);
>> +	}
>> +
>> +	clk |= SDHCI_CLOCK_CARD_EN;
>> +	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>> +}
>> +
>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +	u32 msm_clock = 0;
>> +	int rc = 0;
>> +
>> +	if (!clock)
>> +		goto out;
>> +
>> +	if (clock != msm_host->clk_rate) {
>> +		msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
>> +		rc = clk_set_rate(msm_host->clk, msm_clock);
>> +		if (rc) {
>> +			pr_err("%s: failed to set clock at rate %u, requested clock rate %u\n",
>> +				mmc_hostname(host->mmc), msm_clock, clock);
>> +			goto out;
>> +		}
>> +		msm_host->clk_rate = clock;
>> +		pr_debug("%s: setting clock at rate %lu\n",
>> +			mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
>> +	}
>> +out:
>> +	__sdhci_msm_set_clock(host, clock);
>> +}
>> +
>>  static const struct of_device_id sdhci_msm_dt_match[] = {
>>  	{ .compatible = "qcom,sdhci-msm-v4" },
>>  	{},
>> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>>  static const struct sdhci_ops sdhci_msm_ops = {
>>  	.platform_execute_tuning = sdhci_msm_execute_tuning,
>>  	.reset = sdhci_reset,
>> -	.set_clock = sdhci_set_clock,
>> +	.set_clock = sdhci_msm_set_clock,
>>  	.get_min_clock = sdhci_msm_get_min_clock,
>>  	.get_max_clock = sdhci_msm_get_max_clock,
>>  	.set_bus_width = sdhci_set_bus_width,
>>
>

Re: [PATCH v3 3/9] mmc: sdhci-msm: add pltfm_data support to get clk-rates from DT

[Ritesh Harjani]

Hi Adrian,

Thanks for the review.
Will address the comments below.

Regards
Ritesh


On 8/19/2016 6:33 PM, Adrian Hunter wrote:
> On 19/08/16 07:36, Ritesh Harjani wrote:
>> This adds support for sdhc-msm controllers to get supported
>> clk-rates from DT. sdhci-msm would need it's own set_clock
>> ops to be implemented. For this, supported clk-rates needs
>> to be populated in sdhci_msm_pltfm_data.
>>
>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>> ---
>>  .../devicetree/bindings/mmc/sdhci-msm.txt          |  1 +
>>  drivers/mmc/host/sdhci-msm.c                       | 71 ++++++++++++++++++++++
>>  2 files changed, 72 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> index 485483a..6a83b38 100644
>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> @@ -17,6 +17,7 @@ Required properties:
>>  	"iface" - Main peripheral bus clock (PCLK/HCLK - AHB Bus clock) (required)
>>  	"core"	- SDC MMC clock (MCLK) (required)
>>  	"bus"	- SDCC bus voter clock (optional)
>> +- clk-rates: Array of supported GCC clock frequencies for sdhc, Units - Hz.
>>
>>  Example:
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 85ddaae..2bf141b 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -74,6 +74,11 @@
>>  #define CMUX_SHIFT_PHASE_SHIFT	24
>>  #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
>>
>> +struct sdhci_msm_pltfm_data {
>> +	u32 *clk_table;
>> +	size_t clk_table_sz;
>
> From of_property_count_elems_of_size(), clk_table_sz is an 'int' and is used
> as an 'int' in later patches.  So it can be an 'int' everywhere.
Thanks for noticing, will change it to int.

>
>> +};
>> +
>>  struct sdhci_msm_host {
>>  	struct platform_device *pdev;
>>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
>> @@ -83,6 +88,7 @@ struct sdhci_msm_host {
>>  	struct clk *bus_clk;	/* SDHC bus voter clock */
>>  	struct mmc_host *mmc;
>>  	bool use_14lpp_dll_reset;
>> +	struct sdhci_msm_pltfm_data *pdata;
>>  };
>>
>>  /* Platform specific tuning */
>> @@ -582,6 +588,67 @@ static const struct sdhci_pltfm_data sdhci_msm_pdata = {
>>  	.ops = &sdhci_msm_ops,
>>  };
>>
>> +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
>> +				u32 **table, size_t *size)
>
> Change 'size_t *size' to be 'int *size'
Sure.

>
>> +{
>> +	struct device_node *np = dev->of_node;
>> +	int count = 0;
>> +	u32 *arr = NULL;
>> +	int ret = 0;
>
> Initialization of count and arr is not needed.
Alright.

>
>> +
>> +	count = of_property_count_elems_of_size(np, prop_name, sizeof(u32));
>> +	if (count < 0) {
>> +		dev_err(dev, "%s: Invalid dt property, err(%d)\n",
>> +				prop_name, count);
>> +		ret = count;
>> +		goto out;
>
> Just return count
Ok.

>
>> +	}
>> +
>> +	arr = devm_kzalloc(dev, count * sizeof(*arr), GFP_KERNEL);
>
> Better to use devm_kcalloc here
Ok.

>
>> +	if (!arr) {
>> +		ret = -ENOMEM;
>> +		goto out;
>
> Just return -ENOMEM
Ok.

>
>> +	}
>> +
>> +	ret = of_property_read_u32_array(np, prop_name, arr, count);
>> +	if (ret) {
>> +		dev_err(dev, "%s Invalid dt array property, err(%d)\n",
>> +				prop_name, ret);
>> +		goto out;
>
> Just return ret
Ok.

>
>> +	}
>> +	*table = arr;
>> +	*size = count;
>> +out:
>> +	return ret;
>
> Then the label 'out:' is not needed, nor is the initialization of 'ret' and
> here just return 0.

Ok.

>
>> +}
>> +
>> +static struct sdhci_msm_pltfm_data *sdhci_msm_populate_pdata(struct device *dev,
>> +						struct sdhci_msm_host *msm_host)
>> +{
>> +	struct sdhci_msm_pltfm_data *pdata = NULL;
>> +	size_t table_sz = 0;
>
> table_sz is an 'int' elsewhere.  To keep it all the same type make it an
> 'int' here too.
Ok.


>
>> +	u32 *table = NULL;
>> +
>> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +	if (!pdata)
>> +		goto out;
>
> Here and below just return NULL and then the label out: is not needed.
Alright.

>
>> +
>> +	if (sdhci_msm_dt_get_array(dev, "clk-rates", &table, &table_sz)) {
>> +		dev_err(dev, "failed in DT parsing for supported clk-rates\n");
>> +		goto out;
>> +	}
>> +	if (!table || !table_sz) {
>> +		dev_err(dev, "Invalid clock table\n");
>> +		goto out;
>> +	}
>> +	pdata->clk_table = table;
>> +	pdata->clk_table_sz = table_sz;
>> +
>> +	return pdata;
>> +out:
>> +	return NULL;
>> +}
>> +
>>  static int sdhci_msm_probe(struct platform_device *pdev)
>>  {
>>  	struct sdhci_host *host;
>> @@ -608,6 +675,10 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>
>>  	sdhci_get_of_property(pdev);
>>
>> +	msm_host->pdata = sdhci_msm_populate_pdata(&pdev->dev, msm_host);
>> +	if (!msm_host->pdata)
>> +		goto pltfm_free;
>> +
>>  	/* Setup SDCC bus voter clock. */
>>  	msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
>>  	if (!IS_ERR(msm_host->bus_clk)) {
>>
>

Re: [PATCH v3 7/9] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm

[Adrian Hunter]

On 19/08/16 16:31, Ritesh Harjani wrote:
> Hi,
> 
> 
> On 8/19/2016 6:34 PM, Adrian Hunter wrote:
>> On 19/08/16 07:36, Ritesh Harjani wrote:
>>> sdhci-msm controller may have different clk-rates for each
>>> bus speed mode. Thus implement set_clock callback for
>>> sdhci-msm driver.
>>>
>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>>> ---
>>>  drivers/mmc/host/sdhci-msm.c | 103
>>> ++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 102 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>> index 7c032c3..c0ad9c2 100644
>>> --- a/drivers/mmc/host/sdhci-msm.c
>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>> @@ -89,6 +89,7 @@ struct sdhci_msm_host {
>>>      struct mmc_host *mmc;
>>>      bool use_14lpp_dll_reset;
>>>      struct sdhci_msm_pltfm_data *pdata;
>>> +    u32 clk_rate;
>>>  };
>>>
>>>  /* Platform specific tuning */
>>> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct
>>> sdhci_host *host)
>>>      return msm_host->pdata->clk_table[0];
>>>  }
>>>
>>> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host,
>>> +                    u32 req_clk)
>>> +{
>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> +    struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>> +    int count = msm_host->pdata->clk_table_sz;
>>> +    unsigned int sel_clk = -1;
>>> +    int cnt;
>>> +
>>> +    if (req_clk < sdhci_msm_get_min_clock(host)) {
>>> +        sel_clk = sdhci_msm_get_min_clock(host);
>>> +        return sel_clk;
>>> +    }
>>> +
>>> +    for (cnt = 0; cnt < count; cnt++) {
>>> +        if (msm_host->pdata->clk_table[cnt] > req_clk) {
>>> +            break;
>>> +        } else if (msm_host->pdata->clk_table[cnt] == req_clk) {
>>> +            sel_clk = msm_host->pdata->clk_table[cnt];
>>> +            break;
>>> +        } else {
>>> +            sel_clk = msm_host->pdata->clk_table[cnt];
>>> +        }
>>> +    }
>>
>> 'else' is not needed after 'break' but can't this be simpler e.g.
>>
>> static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host
>> *msm_host, u32 req_clk)
>> {
>>     int count = msm_host->pdata->clk_table_sz;
>>     unsigned int sel_clk = -1;
>>
>>     while (count--) {
>>         sel_clk = msm_host->pdata->clk_table[count];
>>         if (req_clk >= sel_clk)
>>             return sel_clk;
>>     }
>>
>>     return sel_clk;
>> }
> 
> Ok, sure I will check and get back on this.
> 
> 
>>
>>
>>> +    return sel_clk;
>>> +}
>>
>> Blank line needed
> Ok done.
> 
>>
>>> +/**
>>> + * __sdhci_msm_set_clock - sdhci_msm clock control.
>>> + *
>>> + * Description:
>>> + * Implement MSM version of sdhci_set_clock.
>>> + * This is required since MSM controller does not
>>> + * use internal divider and instead directly control
>>> + * the GCC clock as per HW recommendation.
>>> + **/
>>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>> +{
>>> +    u16 clk;
>>> +    unsigned long timeout;
>>> +
>>> +    host->mmc->actual_clock = 0;
>>> +
>>> +    sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>>> +
>>> +    if (clock == 0)
>>> +        return;
>>
>> Should set host->mmc->actual_clock to the actual rate somewhere.
> Since MSM controller does not uses divider then there is no need of having
> actual clock since it should be same as host->clock.

Ok, so sdhci_msm_get_msm_clk_rate() is not the actual clock rate?

> 
> That's why I had kept it 0 intentionally. But I will add a comment
> here then.
> 
>>
>>> +
>>> +    /*
>>> +     * MSM controller do not use clock divider.
>>> +     * Thus read SDHCI_CLOCK_CONTROL and only enable
>>> +     * clock with no divider value programmed.
>>> +     */
>>> +    clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>> +
>>> +    clk |= SDHCI_CLOCK_INT_EN;
>>> +    sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>> +
>>> +    /* Wait max 20 ms */
>>> +    timeout = 20;
>>> +    while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>>> +        & SDHCI_CLOCK_INT_STABLE)) {
>>> +        if (timeout == 0) {
>>> +            pr_err("%s: Internal clock never stabilised.\n",
>>> +                   mmc_hostname(host->mmc));
>>> +            return;
>>> +        }
>>> +        timeout--;
>>> +        mdelay(1);
>>> +    }
>>> +
>>> +    clk |= SDHCI_CLOCK_CARD_EN;
>>> +    sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>> +}
>>> +
>>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int
>>> clock)
>>> +{
>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> +    struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>> +    u32 msm_clock = 0;
>>> +    int rc = 0;
>>> +
>>> +    if (!clock)
>>> +        goto out;
>>> +
>>> +    if (clock != msm_host->clk_rate) {
>>> +        msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
>>> +        rc = clk_set_rate(msm_host->clk, msm_clock);
>>> +        if (rc) {
>>> +            pr_err("%s: failed to set clock at rate %u, requested clock
>>> rate %u\n",
>>> +                mmc_hostname(host->mmc), msm_clock, clock);
>>> +            goto out;
>>> +        }
>>> +        msm_host->clk_rate = clock;
>>> +        pr_debug("%s: setting clock at rate %lu\n",
>>> +            mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
>>> +    }
>>> +out:
>>> +    __sdhci_msm_set_clock(host, clock);
>>> +}
>>> +
>>>  static const struct of_device_id sdhci_msm_dt_match[] = {
>>>      { .compatible = "qcom,sdhci-msm-v4" },
>>>      {},
>>> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>>>  static const struct sdhci_ops sdhci_msm_ops = {
>>>      .platform_execute_tuning = sdhci_msm_execute_tuning,
>>>      .reset = sdhci_reset,
>>> -    .set_clock = sdhci_set_clock,
>>> +    .set_clock = sdhci_msm_set_clock,
>>>      .get_min_clock = sdhci_msm_get_min_clock,
>>>      .get_max_clock = sdhci_msm_get_max_clock,
>>>      .set_bus_width = sdhci_set_bus_width,
>>>
>>
> 

Re: [PATCH v3 7/9] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm

[Ritesh Harjani]

Hi Adrian,


On 8/22/2016 11:50 AM, Adrian Hunter wrote:
> On 19/08/16 16:31, Ritesh Harjani wrote:
>> Hi,
>>
>>
>> On 8/19/2016 6:34 PM, Adrian Hunter wrote:
>>> On 19/08/16 07:36, Ritesh Harjani wrote:
>>>> sdhci-msm controller may have different clk-rates for each
>>>> bus speed mode. Thus implement set_clock callback for
>>>> sdhci-msm driver.
>>>>
>>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>>>> ---
>>>>  drivers/mmc/host/sdhci-msm.c | 103
>>>> ++++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 102 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>>> index 7c032c3..c0ad9c2 100644
>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>> @@ -89,6 +89,7 @@ struct sdhci_msm_host {
>>>>      struct mmc_host *mmc;
>>>>      bool use_14lpp_dll_reset;
>>>>      struct sdhci_msm_pltfm_data *pdata;
>>>> +    u32 clk_rate;
>>>>  };
>>>>
>>>>  /* Platform specific tuning */
>>>> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct
>>>> sdhci_host *host)
>>>>      return msm_host->pdata->clk_table[0];
>>>>  }
>>>>
>>>> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host,
>>>> +                    u32 req_clk)
>>>> +{
>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +    struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>> +    int count = msm_host->pdata->clk_table_sz;
>>>> +    unsigned int sel_clk = -1;
>>>> +    int cnt;
>>>> +
>>>> +    if (req_clk < sdhci_msm_get_min_clock(host)) {
>>>> +        sel_clk = sdhci_msm_get_min_clock(host);
>>>> +        return sel_clk;
>>>> +    }
>>>> +
>>>> +    for (cnt = 0; cnt < count; cnt++) {
>>>> +        if (msm_host->pdata->clk_table[cnt] > req_clk) {
>>>> +            break;
>>>> +        } else if (msm_host->pdata->clk_table[cnt] == req_clk) {
>>>> +            sel_clk = msm_host->pdata->clk_table[cnt];
>>>> +            break;
>>>> +        } else {
>>>> +            sel_clk = msm_host->pdata->clk_table[cnt];
>>>> +        }
>>>> +    }
>>>
>>> 'else' is not needed after 'break' but can't this be simpler e.g.
>>>
>>> static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host
>>> *msm_host, u32 req_clk)
>>> {
>>>     int count = msm_host->pdata->clk_table_sz;
>>>     unsigned int sel_clk = -1;
>>>
>>>     while (count--) {
>>>         sel_clk = msm_host->pdata->clk_table[count];
>>>         if (req_clk >= sel_clk)
>>>             return sel_clk;
>>>     }
>>>
>>>     return sel_clk;
>>> }
>>
>> Ok, sure I will check and get back on this.
>>
>>
>>>
>>>
>>>> +    return sel_clk;
>>>> +}
>>>
>>> Blank line needed
>> Ok done.
>>
>>>
>>>> +/**
>>>> + * __sdhci_msm_set_clock - sdhci_msm clock control.
>>>> + *
>>>> + * Description:
>>>> + * Implement MSM version of sdhci_set_clock.
>>>> + * This is required since MSM controller does not
>>>> + * use internal divider and instead directly control
>>>> + * the GCC clock as per HW recommendation.
>>>> + **/
>>>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>>> +{
>>>> +    u16 clk;
>>>> +    unsigned long timeout;
>>>> +
>>>> +    host->mmc->actual_clock = 0;
>>>> +
>>>> +    sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>>>> +
>>>> +    if (clock == 0)
>>>> +        return;
>>>
>>> Should set host->mmc->actual_clock to the actual rate somewhere.
>> Since MSM controller does not uses divider then there is no need of having
>> actual clock since it should be same as host->clock.
>
> Ok, so sdhci_msm_get_msm_clk_rate() is not the actual clock rate?
So I was assuming we need host->mmc->actual_clock rate if host->clock is 
not the actual rate.
Ok, so I will update actual_clock to host->clock itself.

pls, let me know if any concerns.
>
>>
>> That's why I had kept it 0 intentionally. But I will add a comment
>> here then.
>>
>>>
>>>> +
>>>> +    /*
>>>> +     * MSM controller do not use clock divider.
>>>> +     * Thus read SDHCI_CLOCK_CONTROL and only enable
>>>> +     * clock with no divider value programmed.
>>>> +     */
>>>> +    clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>>> +
>>>> +    clk |= SDHCI_CLOCK_INT_EN;
>>>> +    sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>> +
>>>> +    /* Wait max 20 ms */
>>>> +    timeout = 20;
>>>> +    while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>>>> +        & SDHCI_CLOCK_INT_STABLE)) {
>>>> +        if (timeout == 0) {
>>>> +            pr_err("%s: Internal clock never stabilised.\n",
>>>> +                   mmc_hostname(host->mmc));
>>>> +            return;
>>>> +        }
>>>> +        timeout--;
>>>> +        mdelay(1);
>>>> +    }
>>>> +
>>>> +    clk |= SDHCI_CLOCK_CARD_EN;
>>>> +    sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>> +}
>>>> +
>>>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int
>>>> clock)
>>>> +{
>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> +    struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>> +    u32 msm_clock = 0;
>>>> +    int rc = 0;
>>>> +
>>>> +    if (!clock)
>>>> +        goto out;
>>>> +
>>>> +    if (clock != msm_host->clk_rate) {
>>>> +        msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
>>>> +        rc = clk_set_rate(msm_host->clk, msm_clock);
>>>> +        if (rc) {
>>>> +            pr_err("%s: failed to set clock at rate %u, requested clock
>>>> rate %u\n",
>>>> +                mmc_hostname(host->mmc), msm_clock, clock);
>>>> +            goto out;
>>>> +        }
>>>> +        msm_host->clk_rate = clock;
>>>> +        pr_debug("%s: setting clock at rate %lu\n",
>>>> +            mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
>>>> +    }
>>>> +out:
>>>> +    __sdhci_msm_set_clock(host, clock);
>>>> +}
>>>> +
>>>>  static const struct of_device_id sdhci_msm_dt_match[] = {
>>>>      { .compatible = "qcom,sdhci-msm-v4" },
>>>>      {},
>>>> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>>>>  static const struct sdhci_ops sdhci_msm_ops = {
>>>>      .platform_execute_tuning = sdhci_msm_execute_tuning,
>>>>      .reset = sdhci_reset,
>>>> -    .set_clock = sdhci_set_clock,
>>>> +    .set_clock = sdhci_msm_set_clock,
>>>>      .get_min_clock = sdhci_msm_get_min_clock,
>>>>      .get_max_clock = sdhci_msm_get_max_clock,
>>>>      .set_bus_width = sdhci_set_bus_width,
>>>>
>>>
>>
>

Re: [PATCH v3 7/9] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm

[Adrian Hunter]

On 22/08/16 12:07, Ritesh Harjani wrote:
> Hi Adrian,
> 
> 
> On 8/22/2016 11:50 AM, Adrian Hunter wrote:
>> On 19/08/16 16:31, Ritesh Harjani wrote:
>>> Hi,
>>>
>>>
>>> On 8/19/2016 6:34 PM, Adrian Hunter wrote:
>>>> On 19/08/16 07:36, Ritesh Harjani wrote:
>>>>> sdhci-msm controller may have different clk-rates for each
>>>>> bus speed mode. Thus implement set_clock callback for
>>>>> sdhci-msm driver.
>>>>>
>>>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>>>>> ---
>>>>>  drivers/mmc/host/sdhci-msm.c | 103
>>>>> ++++++++++++++++++++++++++++++++++++++++++-
>>>>>  1 file changed, 102 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>>>> index 7c032c3..c0ad9c2 100644
>>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>>> @@ -89,6 +89,7 @@ struct sdhci_msm_host {
>>>>>      struct mmc_host *mmc;
>>>>>      bool use_14lpp_dll_reset;
>>>>>      struct sdhci_msm_pltfm_data *pdata;
>>>>> +    u32 clk_rate;
>>>>>  };
>>>>>
>>>>>  /* Platform specific tuning */
>>>>> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct
>>>>> sdhci_host *host)
>>>>>      return msm_host->pdata->clk_table[0];
>>>>>  }
>>>>>
>>>>> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host,
>>>>> +                    u32 req_clk)
>>>>> +{
>>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>> +    struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>> +    int count = msm_host->pdata->clk_table_sz;
>>>>> +    unsigned int sel_clk = -1;
>>>>> +    int cnt;
>>>>> +
>>>>> +    if (req_clk < sdhci_msm_get_min_clock(host)) {
>>>>> +        sel_clk = sdhci_msm_get_min_clock(host);
>>>>> +        return sel_clk;
>>>>> +    }
>>>>> +
>>>>> +    for (cnt = 0; cnt < count; cnt++) {
>>>>> +        if (msm_host->pdata->clk_table[cnt] > req_clk) {
>>>>> +            break;
>>>>> +        } else if (msm_host->pdata->clk_table[cnt] == req_clk) {
>>>>> +            sel_clk = msm_host->pdata->clk_table[cnt];
>>>>> +            break;
>>>>> +        } else {
>>>>> +            sel_clk = msm_host->pdata->clk_table[cnt];
>>>>> +        }
>>>>> +    }
>>>>
>>>> 'else' is not needed after 'break' but can't this be simpler e.g.
>>>>
>>>> static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host
>>>> *msm_host, u32 req_clk)
>>>> {
>>>>     int count = msm_host->pdata->clk_table_sz;
>>>>     unsigned int sel_clk = -1;
>>>>
>>>>     while (count--) {
>>>>         sel_clk = msm_host->pdata->clk_table[count];
>>>>         if (req_clk >= sel_clk)
>>>>             return sel_clk;
>>>>     }
>>>>
>>>>     return sel_clk;
>>>> }
>>>
>>> Ok, sure I will check and get back on this.
>>>
>>>
>>>>
>>>>
>>>>> +    return sel_clk;
>>>>> +}
>>>>
>>>> Blank line needed
>>> Ok done.
>>>
>>>>
>>>>> +/**
>>>>> + * __sdhci_msm_set_clock - sdhci_msm clock control.
>>>>> + *
>>>>> + * Description:
>>>>> + * Implement MSM version of sdhci_set_clock.
>>>>> + * This is required since MSM controller does not
>>>>> + * use internal divider and instead directly control
>>>>> + * the GCC clock as per HW recommendation.
>>>>> + **/
>>>>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>>>> +{
>>>>> +    u16 clk;
>>>>> +    unsigned long timeout;
>>>>> +
>>>>> +    host->mmc->actual_clock = 0;
>>>>> +
>>>>> +    sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>>>>> +
>>>>> +    if (clock == 0)
>>>>> +        return;
>>>>
>>>> Should set host->mmc->actual_clock to the actual rate somewhere.
>>> Since MSM controller does not uses divider then there is no need of having
>>> actual clock since it should be same as host->clock.
>>
>> Ok, so sdhci_msm_get_msm_clk_rate() is not the actual clock rate?
> So I was assuming we need host->mmc->actual_clock rate if host->clock is not
> the actual rate.
> Ok, so I will update actual_clock to host->clock itself.
> 
> pls, let me know if any concerns.

I am confused.  Isn't clk_get_rate(msm_host->clk) the actual clock rate?

>>
>>>
>>> That's why I had kept it 0 intentionally. But I will add a comment
>>> here then.
>>>
>>>>
>>>>> +
>>>>> +    /*
>>>>> +     * MSM controller do not use clock divider.
>>>>> +     * Thus read SDHCI_CLOCK_CONTROL and only enable
>>>>> +     * clock with no divider value programmed.
>>>>> +     */
>>>>> +    clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>>>> +
>>>>> +    clk |= SDHCI_CLOCK_INT_EN;
>>>>> +    sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>>> +
>>>>> +    /* Wait max 20 ms */
>>>>> +    timeout = 20;
>>>>> +    while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>>>>> +        & SDHCI_CLOCK_INT_STABLE)) {
>>>>> +        if (timeout == 0) {
>>>>> +            pr_err("%s: Internal clock never stabilised.\n",
>>>>> +                   mmc_hostname(host->mmc));
>>>>> +            return;
>>>>> +        }
>>>>> +        timeout--;
>>>>> +        mdelay(1);
>>>>> +    }
>>>>> +
>>>>> +    clk |= SDHCI_CLOCK_CARD_EN;
>>>>> +    sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>>> +}
>>>>> +
>>>>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int
>>>>> clock)
>>>>> +{
>>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>> +    struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>> +    u32 msm_clock = 0;
>>>>> +    int rc = 0;
>>>>> +
>>>>> +    if (!clock)
>>>>> +        goto out;
>>>>> +
>>>>> +    if (clock != msm_host->clk_rate) {
>>>>> +        msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
>>>>> +        rc = clk_set_rate(msm_host->clk, msm_clock);
>>>>> +        if (rc) {
>>>>> +            pr_err("%s: failed to set clock at rate %u, requested clock
>>>>> rate %u\n",
>>>>> +                mmc_hostname(host->mmc), msm_clock, clock);
>>>>> +            goto out;
>>>>> +        }
>>>>> +        msm_host->clk_rate = clock;
>>>>> +        pr_debug("%s: setting clock at rate %lu\n",
>>>>> +            mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
>>>>> +    }
>>>>> +out:
>>>>> +    __sdhci_msm_set_clock(host, clock);
>>>>> +}
>>>>> +
>>>>>  static const struct of_device_id sdhci_msm_dt_match[] = {
>>>>>      { .compatible = "qcom,sdhci-msm-v4" },
>>>>>      {},
>>>>> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>>>>>  static const struct sdhci_ops sdhci_msm_ops = {
>>>>>      .platform_execute_tuning = sdhci_msm_execute_tuning,
>>>>>      .reset = sdhci_reset,
>>>>> -    .set_clock = sdhci_set_clock,
>>>>> +    .set_clock = sdhci_msm_set_clock,
>>>>>      .get_min_clock = sdhci_msm_get_min_clock,
>>>>>      .get_max_clock = sdhci_msm_get_max_clock,
>>>>>      .set_bus_width = sdhci_set_bus_width,
>>>>>
>>>>
>>>
>>
> 

Re: [PATCH v3 7/9] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm

[Ritesh Harjani]

Hi Adrian,


On 8/22/2016 2:59 PM, Adrian Hunter wrote:
> On 22/08/16 12:07, Ritesh Harjani wrote:
>> Hi Adrian,
>>
>>
>> On 8/22/2016 11:50 AM, Adrian Hunter wrote:
>>> On 19/08/16 16:31, Ritesh Harjani wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 8/19/2016 6:34 PM, Adrian Hunter wrote:
>>>>> On 19/08/16 07:36, Ritesh Harjani wrote:
>>>>>> sdhci-msm controller may have different clk-rates for each
>>>>>> bus speed mode. Thus implement set_clock callback for
>>>>>> sdhci-msm driver.
>>>>>>
>>>>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>>>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>>>>>> ---
>>>>>>  drivers/mmc/host/sdhci-msm.c | 103
>>>>>> ++++++++++++++++++++++++++++++++++++++++++-
>>>>>>  1 file changed, 102 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>>>>> index 7c032c3..c0ad9c2 100644
>>>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>>>> @@ -89,6 +89,7 @@ struct sdhci_msm_host {
>>>>>>      struct mmc_host *mmc;
>>>>>>      bool use_14lpp_dll_reset;
>>>>>>      struct sdhci_msm_pltfm_data *pdata;
>>>>>> +    u32 clk_rate;
>>>>>>  };
>>>>>>
>>>>>>  /* Platform specific tuning */
>>>>>> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct
>>>>>> sdhci_host *host)
>>>>>>      return msm_host->pdata->clk_table[0];
>>>>>>  }
>>>>>>
>>>>>> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host,
>>>>>> +                    u32 req_clk)
>>>>>> +{
>>>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>> +    struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>>> +    int count = msm_host->pdata->clk_table_sz;
>>>>>> +    unsigned int sel_clk = -1;
>>>>>> +    int cnt;
>>>>>> +
>>>>>> +    if (req_clk < sdhci_msm_get_min_clock(host)) {
>>>>>> +        sel_clk = sdhci_msm_get_min_clock(host);
>>>>>> +        return sel_clk;
>>>>>> +    }
>>>>>> +
>>>>>> +    for (cnt = 0; cnt < count; cnt++) {
>>>>>> +        if (msm_host->pdata->clk_table[cnt] > req_clk) {
>>>>>> +            break;
>>>>>> +        } else if (msm_host->pdata->clk_table[cnt] == req_clk) {
>>>>>> +            sel_clk = msm_host->pdata->clk_table[cnt];
>>>>>> +            break;
>>>>>> +        } else {
>>>>>> +            sel_clk = msm_host->pdata->clk_table[cnt];
>>>>>> +        }
>>>>>> +    }
>>>>>
>>>>> 'else' is not needed after 'break' but can't this be simpler e.g.
>>>>>
>>>>> static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host
>>>>> *msm_host, u32 req_clk)
>>>>> {
>>>>>     int count = msm_host->pdata->clk_table_sz;
>>>>>     unsigned int sel_clk = -1;
>>>>>
>>>>>     while (count--) {
>>>>>         sel_clk = msm_host->pdata->clk_table[count];
>>>>>         if (req_clk >= sel_clk)
>>>>>             return sel_clk;
>>>>>     }
>>>>>
>>>>>     return sel_clk;
>>>>> }
>>>>
>>>> Ok, sure I will check and get back on this.
>>>>
>>>>
>>>>>
>>>>>
>>>>>> +    return sel_clk;
>>>>>> +}
>>>>>
>>>>> Blank line needed
>>>> Ok done.
>>>>
>>>>>
>>>>>> +/**
>>>>>> + * __sdhci_msm_set_clock - sdhci_msm clock control.
>>>>>> + *
>>>>>> + * Description:
>>>>>> + * Implement MSM version of sdhci_set_clock.
>>>>>> + * This is required since MSM controller does not
>>>>>> + * use internal divider and instead directly control
>>>>>> + * the GCC clock as per HW recommendation.
>>>>>> + **/
>>>>>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>>>>> +{
>>>>>> +    u16 clk;
>>>>>> +    unsigned long timeout;
>>>>>> +
>>>>>> +    host->mmc->actual_clock = 0;
>>>>>> +
>>>>>> +    sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>>>>>> +
>>>>>> +    if (clock == 0)
>>>>>> +        return;
>>>>>
>>>>> Should set host->mmc->actual_clock to the actual rate somewhere.
>>>> Since MSM controller does not uses divider then there is no need of having
>>>> actual clock since it should be same as host->clock.
>>>
>>> Ok, so sdhci_msm_get_msm_clk_rate() is not the actual clock rate?
>> So I was assuming we need host->mmc->actual_clock rate if host->clock is not
>> the actual rate.
>> Ok, so I will update actual_clock to host->clock itself.
>>
>> pls, let me know if any concerns.
>
> I am confused.  Isn't clk_get_rate(msm_host->clk) the actual clock rate?

Actually, msm controller have few quirks around clocks itself and
I was thinking of not using actual_clock to avoid adding any extra 
quirks for msm.

If you see if actual_clock is 0 and if 
SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK is set, timeout_clk take host->clock 
for timeout calculation.

That's one reason I did not want to update the host->actual_clock. 
(otherwise timeout calc will go wrong for DDR modes for msm, where clock 
from GCC is made double of host->clock and internally divided by 
controller on it's own. In this case actual_clock would be shown as 2x 
of host->clock).
Do you think that keeping actual_clock to 0 should be fine in this case?


I have not yet worked over timeout calculation patches yet, since it is 
of lower priority in my list. After few other areas (mostly HS400), I 
will take over clock timeout fixes to be up-streamed.

>
>>>
>>>>
>>>> That's why I had kept it 0 intentionally. But I will add a comment
>>>> here then.
>>>>
>>>>>
>>>>>> +
>>>>>> +    /*
>>>>>> +     * MSM controller do not use clock divider.
>>>>>> +     * Thus read SDHCI_CLOCK_CONTROL and only enable
>>>>>> +     * clock with no divider value programmed.
>>>>>> +     */
>>>>>> +    clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>>>>> +
>>>>>> +    clk |= SDHCI_CLOCK_INT_EN;
>>>>>> +    sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>>>> +
>>>>>> +    /* Wait max 20 ms */
>>>>>> +    timeout = 20;
>>>>>> +    while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>>>>>> +        & SDHCI_CLOCK_INT_STABLE)) {
>>>>>> +        if (timeout == 0) {
>>>>>> +            pr_err("%s: Internal clock never stabilised.\n",
>>>>>> +                   mmc_hostname(host->mmc));
>>>>>> +            return;
>>>>>> +        }
>>>>>> +        timeout--;
>>>>>> +        mdelay(1);
>>>>>> +    }
>>>>>> +
>>>>>> +    clk |= SDHCI_CLOCK_CARD_EN;
>>>>>> +    sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>>>> +}
>>>>>> +
>>>>>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int
>>>>>> clock)
>>>>>> +{
>>>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>> +    struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>>> +    u32 msm_clock = 0;
>>>>>> +    int rc = 0;
>>>>>> +
>>>>>> +    if (!clock)
>>>>>> +        goto out;
>>>>>> +
>>>>>> +    if (clock != msm_host->clk_rate) {
>>>>>> +        msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
>>>>>> +        rc = clk_set_rate(msm_host->clk, msm_clock);
>>>>>> +        if (rc) {
>>>>>> +            pr_err("%s: failed to set clock at rate %u, requested clock
>>>>>> rate %u\n",
>>>>>> +                mmc_hostname(host->mmc), msm_clock, clock);
>>>>>> +            goto out;
>>>>>> +        }
>>>>>> +        msm_host->clk_rate = clock;
>>>>>> +        pr_debug("%s: setting clock at rate %lu\n",
>>>>>> +            mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
>>>>>> +    }
>>>>>> +out:
>>>>>> +    __sdhci_msm_set_clock(host, clock);
>>>>>> +}
>>>>>> +
>>>>>>  static const struct of_device_id sdhci_msm_dt_match[] = {
>>>>>>      { .compatible = "qcom,sdhci-msm-v4" },
>>>>>>      {},
>>>>>> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>>>>>>  static const struct sdhci_ops sdhci_msm_ops = {
>>>>>>      .platform_execute_tuning = sdhci_msm_execute_tuning,
>>>>>>      .reset = sdhci_reset,
>>>>>> -    .set_clock = sdhci_set_clock,
>>>>>> +    .set_clock = sdhci_msm_set_clock,
>>>>>>      .get_min_clock = sdhci_msm_get_min_clock,
>>>>>>      .get_max_clock = sdhci_msm_get_max_clock,
>>>>>>      .set_bus_width = sdhci_set_bus_width,
>>>>>>
>>>>>
>>>>
>>>
>>
>

Re: [PATCH v3 3/9] mmc: sdhci-msm: add pltfm_data support to get clk-rates from DT

[Bjorn Andersson]

On Thu 18 Aug 21:36 PDT 2016, Ritesh Harjani wrote:

> This adds support for sdhc-msm controllers to get supported
> clk-rates from DT. sdhci-msm would need it's own set_clock
> ops to be implemented. For this, supported clk-rates needs
> to be populated in sdhci_msm_pltfm_data.
> 
> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
> ---
>  .../devicetree/bindings/mmc/sdhci-msm.txt          |  1 +
>  drivers/mmc/host/sdhci-msm.c                       | 71 ++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> index 485483a..6a83b38 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> @@ -17,6 +17,7 @@ Required properties:
>  	"iface" - Main peripheral bus clock (PCLK/HCLK - AHB Bus clock) (required)
>  	"core"	- SDC MMC clock (MCLK) (required)
>  	"bus"	- SDCC bus voter clock (optional)
> +- clk-rates: Array of supported GCC clock frequencies for sdhc, Units - Hz.
>  
>  Example:
>  
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 85ddaae..2bf141b 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -74,6 +74,11 @@
>  #define CMUX_SHIFT_PHASE_SHIFT	24
>  #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
>  
> +struct sdhci_msm_pltfm_data {
> +	u32 *clk_table;
> +	size_t clk_table_sz;
> +};

Rather than calling this "platform data", just call it
sdhci_msm_freq_table and make it:

struct sdhci_msm_freq_table {
	size_t num_freqs;
	u32 freqs[];
};

> +
>  struct sdhci_msm_host {
>  	struct platform_device *pdev;
>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
> @@ -83,6 +88,7 @@ struct sdhci_msm_host {
>  	struct clk *bus_clk;	/* SDHC bus voter clock */
>  	struct mmc_host *mmc;
>  	bool use_14lpp_dll_reset;
> +	struct sdhci_msm_pltfm_data *pdata;
>  };
>  
>  /* Platform specific tuning */
> @@ -582,6 +588,67 @@ static const struct sdhci_pltfm_data sdhci_msm_pdata = {
>  	.ops = &sdhci_msm_ops,
>  };
>  
> +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
> +				u32 **table, size_t *size)
> +{
> +	struct device_node *np = dev->of_node;
> +	int count = 0;
> +	u32 *arr = NULL;
> +	int ret = 0;
> +
> +	count = of_property_count_elems_of_size(np, prop_name, sizeof(u32));
> +	if (count < 0) {
> +		dev_err(dev, "%s: Invalid dt property, err(%d)\n",
> +				prop_name, count);
> +		ret = count;
> +		goto out;
> +	}
> +
> +	arr = devm_kzalloc(dev, count * sizeof(*arr), GFP_KERNEL);
> +	if (!arr) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = of_property_read_u32_array(np, prop_name, arr, count);
> +	if (ret) {
> +		dev_err(dev, "%s Invalid dt array property, err(%d)\n",
> +				prop_name, ret);
> +		goto out;
> +	}
> +	*table = arr;
> +	*size = count;
> +out:
> +	return ret;
> +}
> +
> +static struct sdhci_msm_pltfm_data *sdhci_msm_populate_pdata(struct device *dev,
> +						struct sdhci_msm_host *msm_host)
> +{
> +	struct sdhci_msm_pltfm_data *pdata = NULL;
> +	size_t table_sz = 0;
> +	u32 *table = NULL;
> +

Count of_property_count_elems_of_size() here and then allocate
sizeof(*tbl) + count * sizeof(tbl->freqs[0]) and fill that in, no need
for the two allocations.

> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		goto out;

No need for a goto for early errors, just return as you haven't touch
any state yet.

> +
> +	if (sdhci_msm_dt_get_array(dev, "clk-rates", &table, &table_sz)) {
> +		dev_err(dev, "failed in DT parsing for supported clk-rates\n");
> +		goto out;
> +	}
> +	if (!table || !table_sz) {

A general comment; don't be too defensive in your code. Neither table
nor table_sz should be able to be zero if the above condition fails...

> +		dev_err(dev, "Invalid clock table\n");
> +		goto out;
> +	}
> +	pdata->clk_table = table;
> +	pdata->clk_table_sz = table_sz;
> +
> +	return pdata;
> +out:
> +	return NULL;
> +}
> +
>  static int sdhci_msm_probe(struct platform_device *pdev)
>  {
>  	struct sdhci_host *host;
> @@ -608,6 +675,10 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  
>  	sdhci_get_of_property(pdev);
>  
> +	msm_host->pdata = sdhci_msm_populate_pdata(&pdev->dev, msm_host);
> +	if (!msm_host->pdata)
> +		goto pltfm_free;

Adding this as a requirement breaks existing platforms/dtbs, you may
force it for 8996 if you can detect that, but you should not change it
for existing platforms.

> +
>  	/* Setup SDCC bus voter clock. */
>  	msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
>  	if (!IS_ERR(msm_host->bus_clk)) {

Regards,
Bjorn

Re: [PATCH v3 3/9] mmc: sdhci-msm: add pltfm_data support to get clk-rates from DT

[Ritesh Harjani]

Hi Bjorn,


On 8/23/2016 10:01 AM, Bjorn Andersson wrote:
> On Thu 18 Aug 21:36 PDT 2016, Ritesh Harjani wrote:
>
>> This adds support for sdhc-msm controllers to get supported
>> clk-rates from DT. sdhci-msm would need it's own set_clock
>> ops to be implemented. For this, supported clk-rates needs
>> to be populated in sdhci_msm_pltfm_data.
>>
>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>> ---
>>  .../devicetree/bindings/mmc/sdhci-msm.txt          |  1 +
>>  drivers/mmc/host/sdhci-msm.c                       | 71 ++++++++++++++++++++++
>>  2 files changed, 72 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> index 485483a..6a83b38 100644
>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> @@ -17,6 +17,7 @@ Required properties:
>>  	"iface" - Main peripheral bus clock (PCLK/HCLK - AHB Bus clock) (required)
>>  	"core"	- SDC MMC clock (MCLK) (required)
>>  	"bus"	- SDCC bus voter clock (optional)
>> +- clk-rates: Array of supported GCC clock frequencies for sdhc, Units - Hz.
>>
>>  Example:
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 85ddaae..2bf141b 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -74,6 +74,11 @@
>>  #define CMUX_SHIFT_PHASE_SHIFT	24
>>  #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
>>
>> +struct sdhci_msm_pltfm_data {
>> +	u32 *clk_table;
>> +	size_t clk_table_sz;
>> +};
>
> Rather than calling this "platform data", just call it
> sdhci_msm_freq_table and make it:
Going ahead this sdhci_msm_pltfm_data will be needed to store
other stuff as well, hence it will be preferable to have it as 
pltfm_data only.


>
> struct sdhci_msm_freq_table {
> 	size_t num_freqs;
> 	u32 freqs[];
> };
>
>> +
>>  struct sdhci_msm_host {
>>  	struct platform_device *pdev;
>>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
>> @@ -83,6 +88,7 @@ struct sdhci_msm_host {
>>  	struct clk *bus_clk;	/* SDHC bus voter clock */
>>  	struct mmc_host *mmc;
>>  	bool use_14lpp_dll_reset;
>> +	struct sdhci_msm_pltfm_data *pdata;
>>  };
>>
>>  /* Platform specific tuning */
>> @@ -582,6 +588,67 @@ static const struct sdhci_pltfm_data sdhci_msm_pdata = {
>>  	.ops = &sdhci_msm_ops,
>>  };
>>
>> +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
>> +				u32 **table, size_t *size)
>> +{
>> +	struct device_node *np = dev->of_node;
>> +	int count = 0;
>> +	u32 *arr = NULL;
>> +	int ret = 0;
>> +
>> +	count = of_property_count_elems_of_size(np, prop_name, sizeof(u32));
>> +	if (count < 0) {
>> +		dev_err(dev, "%s: Invalid dt property, err(%d)\n",
>> +				prop_name, count);
>> +		ret = count;
>> +		goto out;
>> +	}
>> +
>> +	arr = devm_kzalloc(dev, count * sizeof(*arr), GFP_KERNEL);
>> +	if (!arr) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	ret = of_property_read_u32_array(np, prop_name, arr, count);
>> +	if (ret) {
>> +		dev_err(dev, "%s Invalid dt array property, err(%d)\n",
>> +				prop_name, ret);
>> +		goto out;
>> +	}
>> +	*table = arr;
>> +	*size = count;
>> +out:
>> +	return ret;
>> +}
>> +
>> +static struct sdhci_msm_pltfm_data *sdhci_msm_populate_pdata(struct device *dev,
>> +						struct sdhci_msm_host *msm_host)
>> +{
>> +	struct sdhci_msm_pltfm_data *pdata = NULL;
>> +	size_t table_sz = 0;
>> +	u32 *table = NULL;
>> +
>
> Count of_property_count_elems_of_size() here and then allocate
> sizeof(*tbl) + count * sizeof(tbl->freqs[0]) and fill that in, no need
> for the two allocations.

As mentioned above, would need this pltfm_data for other stuff when full 
sdhci-msm driver will be up-streamed.

>
>> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +	if (!pdata)
>> +		goto out;
>
> No need for a goto for early errors, just return as you haven't touch
> any state yet.
>
>> +
>> +	if (sdhci_msm_dt_get_array(dev, "clk-rates", &table, &table_sz)) {
>> +		dev_err(dev, "failed in DT parsing for supported clk-rates\n");
>> +		goto out;
>> +	}
>> +	if (!table || !table_sz) {
>
> A general comment; don't be too defensive in your code. Neither table
> nor table_sz should be able to be zero if the above condition fails...
>
>> +		dev_err(dev, "Invalid clock table\n");
>> +		goto out;
>> +	}
>> +	pdata->clk_table = table;
>> +	pdata->clk_table_sz = table_sz;
>> +
>> +	return pdata;
>> +out:
>> +	return NULL;
>> +}
>> +
>>  static int sdhci_msm_probe(struct platform_device *pdev)
>>  {
>>  	struct sdhci_host *host;
>> @@ -608,6 +675,10 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>
>>  	sdhci_get_of_property(pdev);
>>
>> +	msm_host->pdata = sdhci_msm_populate_pdata(&pdev->dev, msm_host);
>> +	if (!msm_host->pdata)
>> +		goto pltfm_free;
>
> Adding this as a requirement breaks existing platforms/dtbs, you may
> force it for 8996 if you can detect that, but you should not change it
> for existing platforms.
Ok, good point and thanks for catching it.
Actually I checked all arch/arm64 dts files and could only see 
8916.dtsi. But I think there would be changes required for arch/arm dts 
files as well.

In that case I will add clk entries to other boards as well.
I will check and see if I can get any of this board to test it on as well.

>
>> +
>>  	/* Setup SDCC bus voter clock. */
>>  	msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
>>  	if (!IS_ERR(msm_host->bus_clk)) {
>
> Regards,
> Bjorn
>

Re: [PATCH v3 7/9] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm

[Adrian Hunter]

On 22/08/16 15:56, Ritesh Harjani wrote:
> Hi Adrian,
> 
> 
> On 8/22/2016 2:59 PM, Adrian Hunter wrote:
>> On 22/08/16 12:07, Ritesh Harjani wrote:
>>> Hi Adrian,
>>>
>>>
>>> On 8/22/2016 11:50 AM, Adrian Hunter wrote:
>>>> On 19/08/16 16:31, Ritesh Harjani wrote:
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 8/19/2016 6:34 PM, Adrian Hunter wrote:
>>>>>> On 19/08/16 07:36, Ritesh Harjani wrote:
>>>>>>> sdhci-msm controller may have different clk-rates for each
>>>>>>> bus speed mode. Thus implement set_clock callback for
>>>>>>> sdhci-msm driver.
>>>>>>>
>>>>>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>>>>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>>>>>>> ---
>>>>>>>  drivers/mmc/host/sdhci-msm.c | 103
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++-
>>>>>>>  1 file changed, 102 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>>>>>> index 7c032c3..c0ad9c2 100644
>>>>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>>>>> @@ -89,6 +89,7 @@ struct sdhci_msm_host {
>>>>>>>      struct mmc_host *mmc;
>>>>>>>      bool use_14lpp_dll_reset;
>>>>>>>      struct sdhci_msm_pltfm_data *pdata;
>>>>>>> +    u32 clk_rate;
>>>>>>>  };
>>>>>>>
>>>>>>>  /* Platform specific tuning */
>>>>>>> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct
>>>>>>> sdhci_host *host)
>>>>>>>      return msm_host->pdata->clk_table[0];
>>>>>>>  }
>>>>>>>
>>>>>>> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host,
>>>>>>> +                    u32 req_clk)
>>>>>>> +{
>>>>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>>> +    struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>>>> +    int count = msm_host->pdata->clk_table_sz;
>>>>>>> +    unsigned int sel_clk = -1;
>>>>>>> +    int cnt;
>>>>>>> +
>>>>>>> +    if (req_clk < sdhci_msm_get_min_clock(host)) {
>>>>>>> +        sel_clk = sdhci_msm_get_min_clock(host);
>>>>>>> +        return sel_clk;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    for (cnt = 0; cnt < count; cnt++) {
>>>>>>> +        if (msm_host->pdata->clk_table[cnt] > req_clk) {
>>>>>>> +            break;
>>>>>>> +        } else if (msm_host->pdata->clk_table[cnt] == req_clk) {
>>>>>>> +            sel_clk = msm_host->pdata->clk_table[cnt];
>>>>>>> +            break;
>>>>>>> +        } else {
>>>>>>> +            sel_clk = msm_host->pdata->clk_table[cnt];
>>>>>>> +        }
>>>>>>> +    }
>>>>>>
>>>>>> 'else' is not needed after 'break' but can't this be simpler e.g.
>>>>>>
>>>>>> static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host
>>>>>> *msm_host, u32 req_clk)
>>>>>> {
>>>>>>     int count = msm_host->pdata->clk_table_sz;
>>>>>>     unsigned int sel_clk = -1;
>>>>>>
>>>>>>     while (count--) {
>>>>>>         sel_clk = msm_host->pdata->clk_table[count];
>>>>>>         if (req_clk >= sel_clk)
>>>>>>             return sel_clk;
>>>>>>     }
>>>>>>
>>>>>>     return sel_clk;
>>>>>> }
>>>>>
>>>>> Ok, sure I will check and get back on this.
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> +    return sel_clk;
>>>>>>> +}
>>>>>>
>>>>>> Blank line needed
>>>>> Ok done.
>>>>>
>>>>>>
>>>>>>> +/**
>>>>>>> + * __sdhci_msm_set_clock - sdhci_msm clock control.
>>>>>>> + *
>>>>>>> + * Description:
>>>>>>> + * Implement MSM version of sdhci_set_clock.
>>>>>>> + * This is required since MSM controller does not
>>>>>>> + * use internal divider and instead directly control
>>>>>>> + * the GCC clock as per HW recommendation.
>>>>>>> + **/
>>>>>>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>>>>>> +{
>>>>>>> +    u16 clk;
>>>>>>> +    unsigned long timeout;
>>>>>>> +
>>>>>>> +    host->mmc->actual_clock = 0;
>>>>>>> +
>>>>>>> +    sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>>>>>>> +
>>>>>>> +    if (clock == 0)
>>>>>>> +        return;
>>>>>>
>>>>>> Should set host->mmc->actual_clock to the actual rate somewhere.
>>>>> Since MSM controller does not uses divider then there is no need of having
>>>>> actual clock since it should be same as host->clock.
>>>>
>>>> Ok, so sdhci_msm_get_msm_clk_rate() is not the actual clock rate?
>>> So I was assuming we need host->mmc->actual_clock rate if host->clock is not
>>> the actual rate.
>>> Ok, so I will update actual_clock to host->clock itself.
>>>
>>> pls, let me know if any concerns.
>>
>> I am confused.  Isn't clk_get_rate(msm_host->clk) the actual clock rate?
> 
> Actually, msm controller have few quirks around clocks itself and
> I was thinking of not using actual_clock to avoid adding any extra quirks
> for msm.
> 
> If you see if actual_clock is 0 and if SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK
> is set, timeout_clk take host->clock for timeout calculation.
> 
> That's one reason I did not want to update the host->actual_clock.
> (otherwise timeout calc will go wrong for DDR modes for msm, where clock
> from GCC is made double of host->clock and internally divided by controller
> on it's own. In this case actual_clock would be shown as 2x of host->clock).
> Do you think that keeping actual_clock to 0 should be fine in this case?

Yes, but please add a comment in the code.

> 
> 
> I have not yet worked over timeout calculation patches yet, since it is of
> lower priority in my list. After few other areas (mostly HS400), I will take
> over clock timeout fixes to be up-streamed.
> 
>>
>>>>
>>>>>
>>>>> That's why I had kept it 0 intentionally. But I will add a comment
>>>>> here then.
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * MSM controller do not use clock divider.
>>>>>>> +     * Thus read SDHCI_CLOCK_CONTROL and only enable
>>>>>>> +     * clock with no divider value programmed.
>>>>>>> +     */
>>>>>>> +    clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>>>>>> +
>>>>>>> +    clk |= SDHCI_CLOCK_INT_EN;
>>>>>>> +    sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>>>>> +
>>>>>>> +    /* Wait max 20 ms */
>>>>>>> +    timeout = 20;
>>>>>>> +    while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>>>>>>> +        & SDHCI_CLOCK_INT_STABLE)) {
>>>>>>> +        if (timeout == 0) {
>>>>>>> +            pr_err("%s: Internal clock never stabilised.\n",
>>>>>>> +                   mmc_hostname(host->mmc));
>>>>>>> +            return;
>>>>>>> +        }
>>>>>>> +        timeout--;
>>>>>>> +        mdelay(1);
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    clk |= SDHCI_CLOCK_CARD_EN;
>>>>>>> +    sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int
>>>>>>> clock)
>>>>>>> +{
>>>>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>>> +    struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>>>> +    u32 msm_clock = 0;
>>>>>>> +    int rc = 0;
>>>>>>> +
>>>>>>> +    if (!clock)
>>>>>>> +        goto out;
>>>>>>> +
>>>>>>> +    if (clock != msm_host->clk_rate) {
>>>>>>> +        msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
>>>>>>> +        rc = clk_set_rate(msm_host->clk, msm_clock);
>>>>>>> +        if (rc) {
>>>>>>> +            pr_err("%s: failed to set clock at rate %u, requested clock
>>>>>>> rate %u\n",
>>>>>>> +                mmc_hostname(host->mmc), msm_clock, clock);
>>>>>>> +            goto out;
>>>>>>> +        }
>>>>>>> +        msm_host->clk_rate = clock;
>>>>>>> +        pr_debug("%s: setting clock at rate %lu\n",
>>>>>>> +            mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
>>>>>>> +    }
>>>>>>> +out:
>>>>>>> +    __sdhci_msm_set_clock(host, clock);
>>>>>>> +}
>>>>>>> +
>>>>>>>  static const struct of_device_id sdhci_msm_dt_match[] = {
>>>>>>>      { .compatible = "qcom,sdhci-msm-v4" },
>>>>>>>      {},
>>>>>>> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>>>>>>>  static const struct sdhci_ops sdhci_msm_ops = {
>>>>>>>      .platform_execute_tuning = sdhci_msm_execute_tuning,
>>>>>>>      .reset = sdhci_reset,
>>>>>>> -    .set_clock = sdhci_set_clock,
>>>>>>> +    .set_clock = sdhci_msm_set_clock,
>>>>>>>      .get_min_clock = sdhci_msm_get_min_clock,
>>>>>>>      .get_max_clock = sdhci_msm_get_max_clock,
>>>>>>>      .set_bus_width = sdhci_set_bus_width,
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 

Re: [PATCH v3 7/9] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm

[Ritesh Harjani]

Hi,

On 8/23/2016 6:47 PM, Adrian Hunter wrote:
> On 22/08/16 15:56, Ritesh Harjani wrote:
>> Hi Adrian,
>>
>>
>> On 8/22/2016 2:59 PM, Adrian Hunter wrote:
>>> On 22/08/16 12:07, Ritesh Harjani wrote:
>>>> Hi Adrian,
>>>>
>>>>
>>>> On 8/22/2016 11:50 AM, Adrian Hunter wrote:
>>>>> On 19/08/16 16:31, Ritesh Harjani wrote:
>>>>>> Hi,
>>>>>>
>>>>>>
>>>>>> On 8/19/2016 6:34 PM, Adrian Hunter wrote:
>>>>>>> On 19/08/16 07:36, Ritesh Harjani wrote:
>>>>>>>> sdhci-msm controller may have different clk-rates for each
>>>>>>>> bus speed mode. Thus implement set_clock callback for
>>>>>>>> sdhci-msm driver.
>>>>>>>>
>>>>>>>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>>>>>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>>>>>>>> ---
>>>>>>>>  drivers/mmc/host/sdhci-msm.c | 103
>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++-
>>>>>>>>  1 file changed, 102 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>>>>>>> index 7c032c3..c0ad9c2 100644
>>>>>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>>>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>>>>>> @@ -89,6 +89,7 @@ struct sdhci_msm_host {
>>>>>>>>      struct mmc_host *mmc;
>>>>>>>>      bool use_14lpp_dll_reset;
>>>>>>>>      struct sdhci_msm_pltfm_data *pdata;
>>>>>>>> +    u32 clk_rate;
>>>>>>>>  };
>>>>>>>>
>>>>>>>>  /* Platform specific tuning */
>>>>>>>> @@ -582,6 +583,106 @@ static unsigned int sdhci_msm_get_min_clock(struct
>>>>>>>> sdhci_host *host)
>>>>>>>>      return msm_host->pdata->clk_table[0];
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_host *host,
>>>>>>>> +                    u32 req_clk)
>>>>>>>> +{
>>>>>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>>>> +    struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>>>>> +    int count = msm_host->pdata->clk_table_sz;
>>>>>>>> +    unsigned int sel_clk = -1;
>>>>>>>> +    int cnt;
>>>>>>>> +
>>>>>>>> +    if (req_clk < sdhci_msm_get_min_clock(host)) {
>>>>>>>> +        sel_clk = sdhci_msm_get_min_clock(host);
>>>>>>>> +        return sel_clk;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    for (cnt = 0; cnt < count; cnt++) {
>>>>>>>> +        if (msm_host->pdata->clk_table[cnt] > req_clk) {
>>>>>>>> +            break;
>>>>>>>> +        } else if (msm_host->pdata->clk_table[cnt] == req_clk) {
>>>>>>>> +            sel_clk = msm_host->pdata->clk_table[cnt];
>>>>>>>> +            break;
>>>>>>>> +        } else {
>>>>>>>> +            sel_clk = msm_host->pdata->clk_table[cnt];
>>>>>>>> +        }
>>>>>>>> +    }
>>>>>>>
>>>>>>> 'else' is not needed after 'break' but can't this be simpler e.g.
>>>>>>>
>>>>>>> static unsigned int sdhci_msm_get_msm_clk_rate(struct sdhci_msm_host
>>>>>>> *msm_host, u32 req_clk)
>>>>>>> {
>>>>>>>     int count = msm_host->pdata->clk_table_sz;
>>>>>>>     unsigned int sel_clk = -1;
>>>>>>>
>>>>>>>     while (count--) {
>>>>>>>         sel_clk = msm_host->pdata->clk_table[count];
>>>>>>>         if (req_clk >= sel_clk)
>>>>>>>             return sel_clk;
>>>>>>>     }
>>>>>>>
>>>>>>>     return sel_clk;
>>>>>>> }
>>>>>>
>>>>>> Ok, sure I will check and get back on this.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> +    return sel_clk;
>>>>>>>> +}
>>>>>>>
>>>>>>> Blank line needed
>>>>>> Ok done.
>>>>>>
>>>>>>>
>>>>>>>> +/**
>>>>>>>> + * __sdhci_msm_set_clock - sdhci_msm clock control.
>>>>>>>> + *
>>>>>>>> + * Description:
>>>>>>>> + * Implement MSM version of sdhci_set_clock.
>>>>>>>> + * This is required since MSM controller does not
>>>>>>>> + * use internal divider and instead directly control
>>>>>>>> + * the GCC clock as per HW recommendation.
>>>>>>>> + **/
>>>>>>>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>>>>>>> +{
>>>>>>>> +    u16 clk;
>>>>>>>> +    unsigned long timeout;
>>>>>>>> +
>>>>>>>> +    host->mmc->actual_clock = 0;
>>>>>>>> +
>>>>>>>> +    sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>>>>>>>> +
>>>>>>>> +    if (clock == 0)
>>>>>>>> +        return;
>>>>>>>
>>>>>>> Should set host->mmc->actual_clock to the actual rate somewhere.
>>>>>> Since MSM controller does not uses divider then there is no need of having
>>>>>> actual clock since it should be same as host->clock.
>>>>>
>>>>> Ok, so sdhci_msm_get_msm_clk_rate() is not the actual clock rate?
>>>> So I was assuming we need host->mmc->actual_clock rate if host->clock is not
>>>> the actual rate.
>>>> Ok, so I will update actual_clock to host->clock itself.
>>>>
>>>> pls, let me know if any concerns.
>>>
>>> I am confused.  Isn't clk_get_rate(msm_host->clk) the actual clock rate?
>>
>> Actually, msm controller have few quirks around clocks itself and
>> I was thinking of not using actual_clock to avoid adding any extra quirks
>> for msm.
>>
>> If you see if actual_clock is 0 and if SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK
>> is set, timeout_clk take host->clock for timeout calculation.
>>
>> That's one reason I did not want to update the host->actual_clock.
>> (otherwise timeout calc will go wrong for DDR modes for msm, where clock
>> from GCC is made double of host->clock and internally divided by controller
>> on it's own. In this case actual_clock would be shown as 2x of host->clock).
>> Do you think that keeping actual_clock to 0 should be fine in this case?
>
> Yes, but please add a comment in the code.

Sure thanks. I will address all comments in v4 spin.

>
>>
>>
>> I have not yet worked over timeout calculation patches yet, since it is of
>> lower priority in my list. After few other areas (mostly HS400), I will take
>> over clock timeout fixes to be up-streamed.
>>
>>>
>>>>>
>>>>>>
>>>>>> That's why I had kept it 0 intentionally. But I will add a comment
>>>>>> here then.
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +    /*
>>>>>>>> +     * MSM controller do not use clock divider.
>>>>>>>> +     * Thus read SDHCI_CLOCK_CONTROL and only enable
>>>>>>>> +     * clock with no divider value programmed.
>>>>>>>> +     */
>>>>>>>> +    clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>>>>>>> +
>>>>>>>> +    clk |= SDHCI_CLOCK_INT_EN;
>>>>>>>> +    sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>>>>>> +
>>>>>>>> +    /* Wait max 20 ms */
>>>>>>>> +    timeout = 20;
>>>>>>>> +    while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>>>>>>>> +        & SDHCI_CLOCK_INT_STABLE)) {
>>>>>>>> +        if (timeout == 0) {
>>>>>>>> +            pr_err("%s: Internal clock never stabilised.\n",
>>>>>>>> +                   mmc_hostname(host->mmc));
>>>>>>>> +            return;
>>>>>>>> +        }
>>>>>>>> +        timeout--;
>>>>>>>> +        mdelay(1);
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    clk |= SDHCI_CLOCK_CARD_EN;
>>>>>>>> +    sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int
>>>>>>>> clock)
>>>>>>>> +{
>>>>>>>> +    struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>>>> +    struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>>>>> +    u32 msm_clock = 0;
>>>>>>>> +    int rc = 0;
>>>>>>>> +
>>>>>>>> +    if (!clock)
>>>>>>>> +        goto out;
>>>>>>>> +
>>>>>>>> +    if (clock != msm_host->clk_rate) {
>>>>>>>> +        msm_clock = sdhci_msm_get_msm_clk_rate(host, clock);
>>>>>>>> +        rc = clk_set_rate(msm_host->clk, msm_clock);
>>>>>>>> +        if (rc) {
>>>>>>>> +            pr_err("%s: failed to set clock at rate %u, requested clock
>>>>>>>> rate %u\n",
>>>>>>>> +                mmc_hostname(host->mmc), msm_clock, clock);
>>>>>>>> +            goto out;
>>>>>>>> +        }
>>>>>>>> +        msm_host->clk_rate = clock;
>>>>>>>> +        pr_debug("%s: setting clock at rate %lu\n",
>>>>>>>> +            mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
>>>>>>>> +    }
>>>>>>>> +out:
>>>>>>>> +    __sdhci_msm_set_clock(host, clock);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  static const struct of_device_id sdhci_msm_dt_match[] = {
>>>>>>>>      { .compatible = "qcom,sdhci-msm-v4" },
>>>>>>>>      {},
>>>>>>>> @@ -592,7 +693,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>>>>>>>>  static const struct sdhci_ops sdhci_msm_ops = {
>>>>>>>>      .platform_execute_tuning = sdhci_msm_execute_tuning,
>>>>>>>>      .reset = sdhci_reset,
>>>>>>>> -    .set_clock = sdhci_set_clock,
>>>>>>>> +    .set_clock = sdhci_msm_set_clock,
>>>>>>>>      .get_min_clock = sdhci_msm_get_min_clock,
>>>>>>>>      .get_max_clock = sdhci_msm_get_max_clock,
>>>>>>>>      .set_bus_width = sdhci_set_bus_width,
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v3 3/9] mmc: sdhci-msm: add pltfm_data support to get clk-rates from DT

[Bjorn Andersson]

On Mon 22 Aug 23:35 PDT 2016, Ritesh Harjani wrote:

> Hi Bjorn,
> 
> 
> On 8/23/2016 10:01 AM, Bjorn Andersson wrote:
> >On Thu 18 Aug 21:36 PDT 2016, Ritesh Harjani wrote:
> >
> >>This adds support for sdhc-msm controllers to get supported
> >>clk-rates from DT. sdhci-msm would need it's own set_clock
> >>ops to be implemented. For this, supported clk-rates needs
> >>to be populated in sdhci_msm_pltfm_data.
> >>
> >>Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
> >>---
> >> .../devicetree/bindings/mmc/sdhci-msm.txt          |  1 +
> >> drivers/mmc/host/sdhci-msm.c                       | 71 ++++++++++++++++++++++
> >> 2 files changed, 72 insertions(+)
> >>
> >>diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> >>index 485483a..6a83b38 100644
> >>--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> >>+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> >>@@ -17,6 +17,7 @@ Required properties:
> >> 	"iface" - Main peripheral bus clock (PCLK/HCLK - AHB Bus clock) (required)
> >> 	"core"	- SDC MMC clock (MCLK) (required)
> >> 	"bus"	- SDCC bus voter clock (optional)
> >>+- clk-rates: Array of supported GCC clock frequencies for sdhc, Units - Hz.
> >>
> >> Example:
> >>
> >>diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> >>index 85ddaae..2bf141b 100644
> >>--- a/drivers/mmc/host/sdhci-msm.c
> >>+++ b/drivers/mmc/host/sdhci-msm.c
> >>@@ -74,6 +74,11 @@
> >> #define CMUX_SHIFT_PHASE_SHIFT	24
> >> #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
> >>
> >>+struct sdhci_msm_pltfm_data {
> >>+	u32 *clk_table;
> >>+	size_t clk_table_sz;
> >>+};
> >
> >Rather than calling this "platform data", just call it
> >sdhci_msm_freq_table and make it:
> Going ahead this sdhci_msm_pltfm_data will be needed to store
> other stuff as well, hence it will be preferable to have it as pltfm_data
> only.
> 

Ok, that's fine then.

[..]
> >
> >Adding this as a requirement breaks existing platforms/dtbs, you may
> >force it for 8996 if you can detect that, but you should not change it
> >for existing platforms.
> Ok, good point and thanks for catching it.
> Actually I checked all arch/arm64 dts files and could only see 8916.dtsi.
> But I think there would be changes required for arch/arm dts files as well.
> 
> In that case I will add clk entries to other boards as well.
> I will check and see if I can get any of this board to test it on as well.
> 

In the upstream kernel you should be compatible with older DTBs, so
while it's good that you're adding this to the arm dts files, the code
should continue to function without this property - e.g. by falling back
to default values or skipping the new functionality.

(Unless there's a really really good reason for breaking this
compatibility)

Regards,
Bjorn

Re: [PATCH v3 3/9] mmc: sdhci-msm: add pltfm_data support to get clk-rates from DT

[Ritesh Harjani]

Hi Bjorn,

On 8/24/2016 10:26 PM, Bjorn Andersson wrote:
> On Mon 22 Aug 23:35 PDT 2016, Ritesh Harjani wrote:
>
>> Hi Bjorn,
>>
>>
>> On 8/23/2016 10:01 AM, Bjorn Andersson wrote:
>>> On Thu 18 Aug 21:36 PDT 2016, Ritesh Harjani wrote:
>>>
>>>> This adds support for sdhc-msm controllers to get supported
>>>> clk-rates from DT. sdhci-msm would need it's own set_clock
>>>> ops to be implemented. For this, supported clk-rates needs
>>>> to be populated in sdhci_msm_pltfm_data.
>>>>
>>>> Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
>>>> ---
>>>> .../devicetree/bindings/mmc/sdhci-msm.txt          |  1 +
>>>> drivers/mmc/host/sdhci-msm.c                       | 71 ++++++++++++++++++++++
>>>> 2 files changed, 72 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>>>> index 485483a..6a83b38 100644
>>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>>>> @@ -17,6 +17,7 @@ Required properties:
>>>> 	"iface" - Main peripheral bus clock (PCLK/HCLK - AHB Bus clock) (required)
>>>> 	"core"	- SDC MMC clock (MCLK) (required)
>>>> 	"bus"	- SDCC bus voter clock (optional)
>>>> +- clk-rates: Array of supported GCC clock frequencies for sdhc, Units - Hz.
>>>>
>>>> Example:
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>>> index 85ddaae..2bf141b 100644
>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>> @@ -74,6 +74,11 @@
>>>> #define CMUX_SHIFT_PHASE_SHIFT	24
>>>> #define CMUX_SHIFT_PHASE_MASK	(7 << CMUX_SHIFT_PHASE_SHIFT)
>>>>
>>>> +struct sdhci_msm_pltfm_data {
>>>> +	u32 *clk_table;
>>>> +	size_t clk_table_sz;
>>>> +};
>>>
>>> Rather than calling this "platform data", just call it
>>> sdhci_msm_freq_table and make it:
>> Going ahead this sdhci_msm_pltfm_data will be needed to store
>> other stuff as well, hence it will be preferable to have it as pltfm_data
>> only.
>>
>
> Ok, that's fine then.
>
> [..]
>>>
>>> Adding this as a requirement breaks existing platforms/dtbs, you may
>>> force it for 8996 if you can detect that, but you should not change it
>>> for existing platforms.
>> Ok, good point and thanks for catching it.
>> Actually I checked all arch/arm64 dts files and could only see 8916.dtsi.
>> But I think there would be changes required for arch/arm dts files as well.
>>
>> In that case I will add clk entries to other boards as well.
>> I will check and see if I can get any of this board to test it on as well.
>>
>
> In the upstream kernel you should be compatible with older DTBs, so
> while it's good that you're adding this to the arm dts files, the code
> should continue to function without this property - e.g. by falling back
> to default values or skipping the new functionality.

sdhci-msm driver in upstream supports only basic functionality as of
now. There is a lot more to be added to upstream driver (like, HW 
recommendations, bus speed modes, bus-voting, scaling feature, pm_qos
and a lot more).

This patch addresses few clock related changes which is as per HW 
recommendation only. Going ahead there will be more changes which will 
be coming in which may have a dependency with clk-rates.
Also, we added clk-rates into all sdhc DT nodes of MSM platforms in 
upstream.

But I understand your concern that even without this property the basic 
msm driver support should work.
Let me spin version(v5) of this patch series after addressing your concern.

Do you think it will be ok to follow below approach -
1. Let clk-rates be *required properties* in DT file (since this is as 
per HW recommendation).
2. Print a warning if clk-rates property is not mentioned in DT.
3. Still continue to support basic msm driver even without this property.

Please let me know if you have anything else to be addressed as well?

>
> (Unless there's a really really good reason for breaking this
> compatibility)
>
> Regards,
> Bjorn
>


Regards
Ritesh