LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH RFC 0/4] Changes for SDCC5 version
@ 2018-05-01 10:39 Vijay Viswanath
  2018-05-01 10:39 ` [PATCH RFC 1/4] mmc: host: Register changes for sdcc V5 Vijay Viswanath
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Vijay Viswanath @ 2018-05-01 10:39 UTC (permalink / raw)
  To: adrian.hunter, ulf.hansson
  Cc: linux-mmc, linux-kernel, shawn.lin, linux-arm-msm, georgi.djakov,
	asutoshd, stummala, venkatg, jeremymc, vviswana, bjorn.andersson,
	riteshh, vbadigan, dianders, sayalil

With SDCC5, the MCI register space got removed and the offset/order of
several registers have changed. Based on SDCC version used and the register,
we need to pick the base address and offset.
Also power irq is a signal from controller to SW that it is ready for
voltage switch. So added support to register voltage regulators in the
msm driver and use them. With this core layer will not have to take care of
voltage regulators. Chips which are currently using core layer regulator APIs
can continue to do so, while newer chips can utilize power irq for voltage
switch.

Depends on patch series:
	[PATCH V5 0/2] mmc: sdhci-msm: Configuring IO_PAD support for sdhci-msm
	https://lkml.org/lkml/2018/4/20/370

Asutosh Das (1):
  mmc: sdhci-msm: Add and use voltage regulator related APIs

Sahitya Tummala (2):
  host: sdhci: fix current caps when there is no host->vmmc
  host: sdhci-msm: implement get_current_limit() host op

Sayali Lokhande (1):
  mmc: host: Register changes for sdcc V5

 .../devicetree/bindings/mmc/sdhci-msm.txt          |   32 +-
 drivers/mmc/host/sdhci-msm.c                       | 1027 +++++++++++++++++---
 drivers/mmc/host/sdhci.c                           |   11 +-
 drivers/mmc/host/sdhci.h                           |    1 +
 4 files changed, 912 insertions(+), 159 deletions(-)

-- 
 Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH RFC 1/4] mmc: host: Register changes for sdcc V5
  2018-05-01 10:39 [PATCH RFC 0/4] Changes for SDCC5 version Vijay Viswanath
@ 2018-05-01 10:39 ` Vijay Viswanath
  2018-05-02  8:28   ` Ulf Hansson
  2018-05-01 10:39 ` [PATCH RFC 2/4] mmc: sdhci-msm: Add and use voltage regulator related APIs Vijay Viswanath
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Vijay Viswanath @ 2018-05-01 10:39 UTC (permalink / raw)
  To: adrian.hunter, ulf.hansson
  Cc: linux-mmc, linux-kernel, shawn.lin, linux-arm-msm, georgi.djakov,
	asutoshd, stummala, venkatg, jeremymc, vviswana, bjorn.andersson,
	riteshh, vbadigan, dianders, sayalil

From: Sayali Lokhande <sayalil@codeaurora.org>

For SDCC version 5.0.0, MCI registers are removed from SDCC
interface and some registers are moved to HC. This change is
to support MCI register removal for msmfalcon. New compatible
string "qcom,sdhci-msm-v5" is added for msmfalcon to support
this change.

Change-Id: I0febfd9bb2222436a8eff20c20107dd4180c9781
Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 .../devicetree/bindings/mmc/sdhci-msm.txt          |   5 +-
 drivers/mmc/host/sdhci-msm.c                       | 485 +++++++++++++++------
 2 files changed, 365 insertions(+), 125 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
index bfdcdc4..c2b7b2b 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -4,7 +4,10 @@ This file documents differences between the core properties in mmc.txt
 and the properties used by the sdhci-msm driver.
 
 Required properties:
-- compatible: Should contain "qcom,sdhci-msm-v4".
+- compatible: Should contain "qcom,sdhci-msm-v4" or "qcom,sdhci-msm-v5".
+		 For SDCC version 5.0.0, MCI registers are removed from SDCC
+		 interface and some registers are moved to HC. New compatible
+		 string is added to support this change - "qcom,sdhci-msm-v5".
 - reg: Base address and length of the register in the following order:
 	- Host controller register map (required)
 	- SD Core register map (required)
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index bb11916..d4d432b 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -39,10 +39,6 @@
 #define CORE_SW_RST		BIT(7)
 #define FF_CLK_SW_RST_DIS	BIT(13)
 
-#define CORE_PWRCTL_STATUS	0xdc
-#define CORE_PWRCTL_MASK	0xe0
-#define CORE_PWRCTL_CLEAR	0xe4
-#define CORE_PWRCTL_CTL		0xe8
 #define CORE_PWRCTL_BUS_OFF	BIT(0)
 #define CORE_PWRCTL_BUS_ON	BIT(1)
 #define CORE_PWRCTL_IO_LOW	BIT(2)
@@ -63,17 +59,13 @@
 #define CORE_CDR_EXT_EN		BIT(19)
 #define CORE_DLL_PDN		BIT(29)
 #define CORE_DLL_RST		BIT(30)
-#define CORE_DLL_CONFIG		0x100
 #define CORE_CMD_DAT_TRACK_SEL	BIT(0)
-#define CORE_DLL_STATUS		0x108
 
-#define CORE_DLL_CONFIG_2	0x1b4
 #define CORE_DDR_CAL_EN		BIT(0)
 #define CORE_FLL_CYCLE_CNT	BIT(18)
 #define CORE_DLL_CLOCK_DISABLE	BIT(21)
 
-#define CORE_VENDOR_SPEC	0x10c
-#define CORE_VENDOR_SPEC_POR_VAL	0xa1c
+#define CORE_VENDOR_SPEC_POR_VAL 0xa1c
 #define CORE_CLK_PWRSAVE	BIT(1)
 #define CORE_HC_MCLK_SEL_DFLT	(2 << 8)
 #define CORE_HC_MCLK_SEL_HS400	(3 << 8)
@@ -111,17 +103,14 @@
 #define CORE_CDC_SWITCH_BYPASS_OFF	BIT(0)
 #define CORE_CDC_SWITCH_RC_EN		BIT(1)
 
-#define CORE_DDR_200_CFG		0x184
 #define CORE_CDC_T4_DLY_SEL		BIT(0)
 #define CORE_CMDIN_RCLK_EN		BIT(1)
 #define CORE_START_CDC_TRAFFIC		BIT(6)
-#define CORE_VENDOR_SPEC3	0x1b0
+
 #define CORE_PWRSAVE_DLL	BIT(3)
 
-#define CORE_DDR_CONFIG		0x1b8
 #define DDR_CONFIG_POR_VAL	0x80040853
 
-#define CORE_VENDOR_SPEC_CAPABILITIES0	0x11c
 
 #define INVALID_TUNING_PHASE	-1
 #define SDHCI_MSM_MIN_CLOCK	400000
@@ -137,6 +126,93 @@
 /* Timeout value to avoid infinite waiting for pwr_irq */
 #define MSM_PWR_IRQ_TIMEOUT_MS 5000
 
+struct sdhci_msm_offset {
+	u32 core_mci_data_cnt;
+	u32 core_mci_status;
+	u32 core_mci_fifo_cnt;
+	u32 core_mci_version;
+	u32 core_generics;
+	u32 core_testbus_config;
+	u32 core_testbus_sel2_bit;
+	u32 core_testbus_ena;
+	u32 core_testbus_sel2;
+	u32 core_pwrctl_status;
+	u32 core_pwrctl_mask;
+	u32 core_pwrctl_clear;
+	u32 core_pwrctl_ctl;
+	u32 core_sdcc_debug_reg;
+	u32 core_dll_config;
+	u32 core_dll_status;
+	u32 core_vendor_spec;
+	u32 core_vendor_spec_adma_err_addr0;
+	u32 core_vendor_spec_adma_err_addr1;
+	u32 core_vendor_spec_func2;
+	u32 core_vendor_spec_capabilities0;
+	u32 core_ddr_200_cfg;
+	u32 core_vendor_spec3;
+	u32 core_dll_config_2;
+	u32 core_ddr_config;
+	u32 core_ddr_config_2;
+};
+
+struct sdhci_msm_offset sdhci_msm_offset_mci_removed = {
+	.core_mci_data_cnt = 0x35c,
+	.core_mci_status = 0x324,
+	.core_mci_fifo_cnt = 0x308,
+	.core_mci_version = 0x318,
+	.core_generics = 0x320,
+	.core_testbus_config = 0x32c,
+	.core_testbus_sel2_bit = 3,
+	.core_testbus_ena = (1 << 31),
+	.core_testbus_sel2 = (1 << 3),
+	.core_pwrctl_status = 0x240,
+	.core_pwrctl_mask = 0x244,
+	.core_pwrctl_clear = 0x248,
+	.core_pwrctl_ctl = 0x24c,
+	.core_sdcc_debug_reg = 0x358,
+	.core_dll_config = 0x200,
+	.core_dll_status = 0x208,
+	.core_vendor_spec = 0x20c,
+	.core_vendor_spec_adma_err_addr0 = 0x214,
+	.core_vendor_spec_adma_err_addr1 = 0x218,
+	.core_vendor_spec_func2 = 0x210,
+	.core_vendor_spec_capabilities0 = 0x21c,
+	.core_ddr_200_cfg = 0x224,
+	.core_vendor_spec3 = 0x250,
+	.core_dll_config_2 = 0x254,
+	.core_ddr_config = 0x258,
+	.core_ddr_config_2 = 0x25c,
+};
+
+struct sdhci_msm_offset sdhci_msm_offset_mci_present = {
+	.core_mci_data_cnt = 0x30,
+	.core_mci_status = 0x34,
+	.core_mci_fifo_cnt = 0x44,
+	.core_mci_version = 0x050,
+	.core_generics = 0x70,
+	.core_testbus_config = 0x0CC,
+	.core_testbus_sel2_bit = 4,
+	.core_testbus_ena = (1 << 3),
+	.core_testbus_sel2 = (1 << 4),
+	.core_pwrctl_status = 0xDC,
+	.core_pwrctl_mask = 0xE0,
+	.core_pwrctl_clear = 0xE4,
+	.core_pwrctl_ctl = 0xE8,
+	.core_sdcc_debug_reg = 0x124,
+	.core_dll_config = 0x100,
+	.core_dll_status = 0x108,
+	.core_vendor_spec = 0x10C,
+	.core_vendor_spec_adma_err_addr0 = 0x114,
+	.core_vendor_spec_adma_err_addr1 = 0x118,
+	.core_vendor_spec_func2 = 0x110,
+	.core_vendor_spec_capabilities0 = 0x11C,
+	.core_ddr_200_cfg = 0x184,
+	.core_vendor_spec3 = 0x1B0,
+	.core_dll_config_2 = 0x1B4,
+	.core_ddr_config = 0x1B8,
+	.core_ddr_config_2 = 0x1BC,
+};
+
 struct sdhci_msm_host {
 	struct platform_device *pdev;
 	void __iomem *core_mem;	/* MSM SDCC mapped address */
@@ -156,8 +232,72 @@ struct sdhci_msm_host {
 	wait_queue_head_t pwr_irq_wait;
 	bool pwr_irq_flag;
 	u32 caps_0;
+	bool mci_removed;
+	const struct sdhci_msm_offset *offset;
 };
 
+/*
+ * APIs to write to vendor specific registers which were there in the core_mem
+ * region before MCI was removed.
+ */
+u8 sdhci_msm_vendor_readb_relaxed(struct sdhci_host *host, u32 offset)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	void __iomem *base_addr;
+
+	if (msm_host->mci_removed)
+		base_addr = host->ioaddr;
+	else
+		base_addr = msm_host->core_mem;
+
+	return readb_relaxed(base_addr + offset);
+}
+
+u32 sdhci_msm_vendor_readl_relaxed(struct sdhci_host *host, u32 offset)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	void __iomem *base_addr;
+
+	if (msm_host->mci_removed)
+		base_addr = host->ioaddr;
+	else
+		base_addr = msm_host->core_mem;
+
+	return readl_relaxed(base_addr + offset);
+}
+
+void sdhci_msm_vendor_writeb_relaxed(u8 val, struct sdhci_host *host,
+		u32 offset)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	void __iomem *base_addr;
+
+	if (msm_host->mci_removed)
+		base_addr = host->ioaddr;
+	else
+		base_addr = msm_host->core_mem;
+
+	writeb_relaxed(val, base_addr + offset);
+}
+
+void sdhci_msm_vendor_writel_relaxed(u32 val, struct sdhci_host *host,
+		u32 offset)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	void __iomem *base_addr;
+
+	if (msm_host->mci_removed)
+		base_addr = host->ioaddr;
+	else
+		base_addr = msm_host->core_mem;
+
+	writel_relaxed(val, base_addr + offset);
+}
+
 static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
 						    unsigned int clock)
 {
@@ -205,10 +345,14 @@ static inline int msm_dll_poll_ck_out_en(struct sdhci_host *host, u8 poll)
 	u32 wait_cnt = 50;
 	u8 ck_out_en;
 	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);
+	const struct sdhci_msm_offset *msm_offset =
+					msm_host->offset;
 
 	/* Poll for CK_OUT_EN bit.  max. poll time = 50us */
-	ck_out_en = !!(readl_relaxed(host->ioaddr + CORE_DLL_CONFIG) &
-			CORE_CK_OUT_EN);
+	ck_out_en = !!(readl_relaxed(host->ioaddr +
+			msm_offset->core_dll_config) & CORE_CK_OUT_EN);
 
 	while (ck_out_en != poll) {
 		if (--wait_cnt == 0) {
@@ -218,8 +362,8 @@ static inline int msm_dll_poll_ck_out_en(struct sdhci_host *host, u8 poll)
 		}
 		udelay(1);
 
-		ck_out_en = !!(readl_relaxed(host->ioaddr + CORE_DLL_CONFIG) &
-				CORE_CK_OUT_EN);
+		ck_out_en = !!(readl_relaxed(host->ioaddr +
+			msm_offset->core_dll_config) & CORE_CK_OUT_EN);
 	}
 
 	return 0;
@@ -235,16 +379,20 @@ static int msm_config_cm_dll_phase(struct sdhci_host *host, u8 phase)
 	unsigned long flags;
 	u32 config;
 	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);
+	const struct sdhci_msm_offset *msm_offset =
+					msm_host->offset;
 
 	if (phase > 0xf)
 		return -EINVAL;
 
 	spin_lock_irqsave(&host->lock, flags);
 
-	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_dll_config);
 	config &= ~(CORE_CDR_EN | CORE_CK_OUT_EN);
 	config |= (CORE_CDR_EXT_EN | CORE_DLL_EN);
-	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_dll_config);
 
 	/* Wait until CK_OUT_EN bit of DLL_CONFIG register becomes '0' */
 	rc = msm_dll_poll_ck_out_en(host, 0);
@@ -255,24 +403,24 @@ static int msm_config_cm_dll_phase(struct sdhci_host *host, u8 phase)
 	 * Write the selected DLL clock output phase (0 ... 15)
 	 * to CDR_SELEXT bit field of DLL_CONFIG register.
 	 */
-	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_dll_config);
 	config &= ~CDR_SELEXT_MASK;
 	config |= grey_coded_phase_table[phase] << CDR_SELEXT_SHIFT;
-	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_dll_config);
 
-	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_dll_config);
 	config |= CORE_CK_OUT_EN;
-	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_dll_config);
 
 	/* Wait until CK_OUT_EN bit of DLL_CONFIG register becomes '1' */
 	rc = msm_dll_poll_ck_out_en(host, 1);
 	if (rc)
 		goto err_out;
 
-	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_dll_config);
 	config |= CORE_CDR_EN;
 	config &= ~CORE_CDR_EXT_EN;
-	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_dll_config);
 	goto out;
 
 err_out:
@@ -398,6 +546,10 @@ static int msm_find_most_appropriate_phase(struct sdhci_host *host,
 static inline void msm_cm_dll_set_freq(struct sdhci_host *host)
 {
 	u32 mclk_freq = 0, config;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	const struct sdhci_msm_offset *msm_offset =
+					msm_host->offset;
 
 	/* Program the MCLK value to MCLK_FREQ bit field */
 	if (host->clock <= 112000000)
@@ -417,10 +569,10 @@ static inline void msm_cm_dll_set_freq(struct sdhci_host *host)
 	else if (host->clock <= 200000000)
 		mclk_freq = 7;
 
-	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_dll_config);
 	config &= ~CMUX_SHIFT_PHASE_MASK;
 	config |= mclk_freq << CMUX_SHIFT_PHASE_SHIFT;
-	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_dll_config);
 }
 
 /* Initialize the DLL (Programmable Delay Line) */
@@ -432,6 +584,8 @@ static int msm_init_cm_dll(struct sdhci_host *host)
 	int wait_cnt = 50;
 	unsigned long flags;
 	u32 config;
+	const struct sdhci_msm_offset *msm_offset =
+					msm_host->offset;
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -440,34 +594,43 @@ static int msm_init_cm_dll(struct sdhci_host *host)
 	 * tuning is in progress. Keeping PWRSAVE ON may
 	 * turn off the clock.
 	 */
-	config = readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_vendor_spec);
 	config &= ~CORE_CLK_PWRSAVE;
-	writel_relaxed(config, host->ioaddr + CORE_VENDOR_SPEC);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_vendor_spec);
 
 	if (msm_host->use_14lpp_dll_reset) {
-		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+		config = readl_relaxed(host->ioaddr +
+				msm_offset->core_dll_config);
 		config &= ~CORE_CK_OUT_EN;
-		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+		writel_relaxed(config, host->ioaddr +
+				msm_offset->core_dll_config);
 
-		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
+		config = readl_relaxed(host->ioaddr +
+				msm_offset->core_dll_config_2);
 		config |= CORE_DLL_CLOCK_DISABLE;
-		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
+		writel_relaxed(config, host->ioaddr +
+				msm_offset->core_dll_config_2);
 	}
 
-	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr +
+			msm_offset->core_dll_config);
 	config |= CORE_DLL_RST;
-	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+	writel_relaxed(config, host->ioaddr +
+			msm_offset->core_dll_config);
 
-	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr +
+			msm_offset->core_dll_config);
 	config |= CORE_DLL_PDN;
-	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+	writel_relaxed(config, host->ioaddr +
+			msm_offset->core_dll_config);
 	msm_cm_dll_set_freq(host);
 
 	if (msm_host->use_14lpp_dll_reset &&
 	    !IS_ERR_OR_NULL(msm_host->xo_clk)) {
 		u32 mclk_freq = 0;
 
-		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
+		config = readl_relaxed(host->ioaddr +
+				msm_offset->core_dll_config_2);
 		config &= CORE_FLL_CYCLE_CNT;
 		if (config)
 			mclk_freq = DIV_ROUND_CLOSEST_ULL((host->clock * 8),
@@ -476,40 +639,52 @@ static int msm_init_cm_dll(struct sdhci_host *host)
 			mclk_freq = DIV_ROUND_CLOSEST_ULL((host->clock * 4),
 					clk_get_rate(msm_host->xo_clk));
 
-		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
+		config = readl_relaxed(host->ioaddr +
+				msm_offset->core_dll_config_2);
 		config &= ~(0xFF << 10);
 		config |= mclk_freq << 10;
 
-		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
+		writel_relaxed(config, host->ioaddr +
+				msm_offset->core_dll_config_2);
 		/* wait for 5us before enabling DLL clock */
 		udelay(5);
 	}
 
-	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr +
+			msm_offset->core_dll_config);
 	config &= ~CORE_DLL_RST;
-	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+	writel_relaxed(config, host->ioaddr +
+			msm_offset->core_dll_config);
 
-	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr +
+			msm_offset->core_dll_config);
 	config &= ~CORE_DLL_PDN;
-	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+	writel_relaxed(config, host->ioaddr +
+			msm_offset->core_dll_config);
 
 	if (msm_host->use_14lpp_dll_reset) {
 		msm_cm_dll_set_freq(host);
-		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
+		config = readl_relaxed(host->ioaddr +
+				msm_offset->core_dll_config_2);
 		config &= ~CORE_DLL_CLOCK_DISABLE;
-		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
+		writel_relaxed(config, host->ioaddr +
+				msm_offset->core_dll_config_2);
 	}
 
-	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr +
+			msm_offset->core_dll_config);
 	config |= CORE_DLL_EN;
-	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+	writel_relaxed(config, host->ioaddr +
+			msm_offset->core_dll_config);
 
-	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr +
+			msm_offset->core_dll_config);
 	config |= CORE_CK_OUT_EN;
-	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+	writel_relaxed(config, host->ioaddr +
+			msm_offset->core_dll_config);
 
 	/* Wait until DLL_LOCK bit of DLL_STATUS register becomes '1' */
-	while (!(readl_relaxed(host->ioaddr + CORE_DLL_STATUS) &
+	while (!(readl_relaxed(host->ioaddr + msm_offset->core_dll_status) &
 		 CORE_DLL_LOCK)) {
 		/* max. wait for 50us sec for LOCK bit to be set */
 		if (--wait_cnt == 0) {
@@ -530,19 +705,21 @@ static void msm_hc_select_default(struct sdhci_host *host)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
 	u32 config;
+	const struct sdhci_msm_offset *msm_offset =
+					msm_host->offset;
 
 	if (!msm_host->use_cdclp533) {
 		config = readl_relaxed(host->ioaddr +
-				CORE_VENDOR_SPEC3);
+				msm_offset->core_vendor_spec3);
 		config &= ~CORE_PWRSAVE_DLL;
 		writel_relaxed(config, host->ioaddr +
-				CORE_VENDOR_SPEC3);
+				msm_offset->core_vendor_spec3);
 	}
 
-	config = readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_vendor_spec);
 	config &= ~CORE_HC_MCLK_SEL_MASK;
 	config |= CORE_HC_MCLK_SEL_DFLT;
-	writel_relaxed(config, host->ioaddr + CORE_VENDOR_SPEC);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_vendor_spec);
 
 	/*
 	 * Disable HC_SELECT_IN to be able to use the UHS mode select
@@ -551,10 +728,10 @@ static void msm_hc_select_default(struct sdhci_host *host)
 	 * Write 0 to HC_SELECT_IN and HC_SELECT_IN_EN field
 	 * in VENDOR_SPEC_FUNC
 	 */
-	config = readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_vendor_spec);
 	config &= ~CORE_HC_SELECT_IN_EN;
 	config &= ~CORE_HC_SELECT_IN_MASK;
-	writel_relaxed(config, host->ioaddr + CORE_VENDOR_SPEC);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_vendor_spec);
 
 	/*
 	 * Make sure above writes impacting free running MCLK are completed
@@ -570,32 +747,36 @@ static void msm_hc_select_hs400(struct sdhci_host *host)
 	struct mmc_ios ios = host->mmc->ios;
 	u32 config, dll_lock;
 	int rc;
+	const struct sdhci_msm_offset *msm_offset =
+					msm_host->offset;
 
 	/* Select the divided clock (free running MCLK/2) */
-	config = readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_vendor_spec);
 	config &= ~CORE_HC_MCLK_SEL_MASK;
 	config |= CORE_HC_MCLK_SEL_HS400;
 
-	writel_relaxed(config, host->ioaddr + CORE_VENDOR_SPEC);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_vendor_spec);
 	/*
 	 * Select HS400 mode using the HC_SELECT_IN from VENDOR SPEC
 	 * register
 	 */
 	if ((msm_host->tuning_done || ios.enhanced_strobe) &&
 	    !msm_host->calibration_done) {
-		config = readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC);
+		config = readl_relaxed(host->ioaddr +
+				msm_offset->core_vendor_spec);
 		config |= CORE_HC_SELECT_IN_HS400;
 		config |= CORE_HC_SELECT_IN_EN;
-		writel_relaxed(config, host->ioaddr + CORE_VENDOR_SPEC);
+		writel_relaxed(config, host->ioaddr +
+				msm_offset->core_vendor_spec);
 	}
 	if (!msm_host->clk_rate && !msm_host->use_cdclp533) {
 		/*
 		 * Poll on DLL_LOCK or DDR_DLL_LOCK bits in
-		 * CORE_DLL_STATUS to be set.  This should get set
+		 * core_dll_status to be set.  This should get set
 		 * within 15 us at 200 MHz.
 		 */
 		rc = readl_relaxed_poll_timeout(host->ioaddr +
-						CORE_DLL_STATUS,
+						msm_offset->core_dll_status,
 						dll_lock,
 						(dll_lock &
 						(CORE_DLL_LOCK |
@@ -647,6 +828,8 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
 	u32 config, calib_done;
 	int ret;
+	const struct sdhci_msm_offset *msm_offset =
+					msm_host->offset;
 
 	pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
 
@@ -663,13 +846,13 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
 	if (ret)
 		goto out;
 
-	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_dll_config);
 	config |= CORE_CMD_DAT_TRACK_SEL;
-	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_dll_config);
 
-	config = readl_relaxed(host->ioaddr + CORE_DDR_200_CFG);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_ddr_200_cfg);
 	config &= ~CORE_CDC_T4_DLY_SEL;
-	writel_relaxed(config, host->ioaddr + CORE_DDR_200_CFG);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_ddr_200_cfg);
 
 	config = readl_relaxed(host->ioaddr + CORE_CSR_CDC_GEN_CFG);
 	config &= ~CORE_CDC_SWITCH_BYPASS_OFF;
@@ -679,9 +862,9 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
 	config |= CORE_CDC_SWITCH_RC_EN;
 	writel_relaxed(config, host->ioaddr + CORE_CSR_CDC_GEN_CFG);
 
-	config = readl_relaxed(host->ioaddr + CORE_DDR_200_CFG);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_ddr_200_cfg);
 	config &= ~CORE_START_CDC_TRAFFIC;
-	writel_relaxed(config, host->ioaddr + CORE_DDR_200_CFG);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_ddr_200_cfg);
 
 	/* Perform CDC Register Initialization Sequence */
 
@@ -733,9 +916,9 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
 		goto out;
 	}
 
-	config = readl_relaxed(host->ioaddr + CORE_DDR_200_CFG);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_ddr_200_cfg);
 	config |= CORE_START_CDC_TRAFFIC;
-	writel_relaxed(config, host->ioaddr + CORE_DDR_200_CFG);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_ddr_200_cfg);
 out:
 	pr_debug("%s: %s: Exit, ret %d\n", mmc_hostname(host->mmc),
 		 __func__, ret);
@@ -747,32 +930,40 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
 	struct mmc_host *mmc = host->mmc;
 	u32 dll_status, config;
 	int ret;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	const struct sdhci_msm_offset *msm_offset =
+					msm_host->offset;
 
 	pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
 
 	/*
-	 * Currently the CORE_DDR_CONFIG register defaults to desired
+	 * Currently the core_ddr_config register defaults to desired
 	 * configuration on reset. Currently reprogramming the power on
 	 * reset (POR) value in case it might have been modified by
 	 * bootloaders. In the future, if this changes, then the desired
 	 * values will need to be programmed appropriately.
 	 */
-	writel_relaxed(DDR_CONFIG_POR_VAL, host->ioaddr + CORE_DDR_CONFIG);
+	writel_relaxed(DDR_CONFIG_POR_VAL, host->ioaddr +
+			msm_offset->core_ddr_config);
 
 	if (mmc->ios.enhanced_strobe) {
-		config = readl_relaxed(host->ioaddr + CORE_DDR_200_CFG);
+		config = readl_relaxed(host->ioaddr +
+				msm_offset->core_ddr_200_cfg);
 		config |= CORE_CMDIN_RCLK_EN;
-		writel_relaxed(config, host->ioaddr + CORE_DDR_200_CFG);
+		writel_relaxed(config, host->ioaddr +
+				msm_offset->core_ddr_200_cfg);
 	}
 
-	config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_dll_config_2);
 	config |= CORE_DDR_CAL_EN;
-	writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_dll_config_2);
 
-	ret = readl_relaxed_poll_timeout(host->ioaddr + CORE_DLL_STATUS,
-					 dll_status,
-					 (dll_status & CORE_DDR_DLL_LOCK),
-					 10, 1000);
+	ret = readl_relaxed_poll_timeout(host->ioaddr +
+					msm_offset->core_dll_status,
+					dll_status,
+					(dll_status & CORE_DDR_DLL_LOCK),
+					10, 1000);
 
 	if (ret == -ETIMEDOUT) {
 		pr_err("%s: %s: CM_DLL_SDC4 calibration was not completed\n",
@@ -780,9 +971,9 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
 		goto out;
 	}
 
-	config = readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC3);
+	config = readl_relaxed(host->ioaddr + msm_offset->core_vendor_spec3);
 	config |= CORE_PWRSAVE_DLL;
-	writel_relaxed(config, host->ioaddr + CORE_VENDOR_SPEC3);
+	writel_relaxed(config, host->ioaddr + msm_offset->core_vendor_spec3);
 
 	/*
 	 * Drain writebuffer to ensure above DLL calibration
@@ -802,6 +993,8 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
 	struct mmc_host *mmc = host->mmc;
 	int ret;
 	u32 config;
+	const struct sdhci_msm_offset *msm_offset =
+					msm_host->offset;
 
 	pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
 
@@ -819,9 +1012,11 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
 					      msm_host->saved_tuning_phase);
 		if (ret)
 			goto out;
-		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+		config = readl_relaxed(host->ioaddr +
+				msm_offset->core_dll_config);
 		config |= CORE_CMD_DAT_TRACK_SEL;
-		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+		writel_relaxed(config, host->ioaddr +
+				msm_offset->core_dll_config);
 	}
 
 	if (msm_host->use_cdclp533)
@@ -951,6 +1146,8 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
 	u16 ctrl_2;
 	u32 config;
+	const struct sdhci_msm_offset *msm_offset =
+					msm_host->offset;
 
 	ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
 	/* Select Bus Speed Mode for host */
@@ -991,13 +1188,17 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
 		 * DLL is not required for clock <= 100MHz
 		 * Thus, make sure DLL it is disabled when not required
 		 */
-		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+		config = readl_relaxed(host->ioaddr +
+				msm_offset->core_dll_config);
 		config |= CORE_DLL_RST;
-		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+		writel_relaxed(config, host->ioaddr +
+				msm_offset->core_dll_config);
 
-		config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
+		config = readl_relaxed(host->ioaddr +
+				msm_offset->core_dll_config);
 		config |= CORE_DLL_PDN;
-		writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
+		writel_relaxed(config, host->ioaddr +
+				msm_offset->core_dll_config);
 
 		/*
 		 * The DLL needs to be restored and CDCLP533 recalibrated
@@ -1039,7 +1240,9 @@ static void sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type)
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
 	bool done = false;
-	u32 val;
+	u32 val = SWITCHABLE_SIGNALING_VOLTAGE;
+	const struct sdhci_msm_offset *msm_offset =
+					msm_host->offset;
 
 	pr_debug("%s: %s: request %d curr_pwr_state %x curr_io_level %x\n",
 			mmc_hostname(host->mmc), __func__, req_type,
@@ -1048,8 +1251,12 @@ static void sdhci_msm_check_power_status(struct sdhci_host *host, u32 req_type)
 	/*
 	 * The power interrupt will not be generated for signal voltage
 	 * switches if SWITCHABLE_SIGNALING_VOLTAGE in MCI_GENERICS is not set.
+	 * Since sdhci-msm-v5, this bit has been removed and SW must consider
+	 * it as always set.
 	 */
-	val = readl(msm_host->core_mem + CORE_MCI_GENERICS);
+	if (!msm_host->mci_removed)
+		val =  sdhci_msm_vendor_readl_relaxed(host,
+					msm_offset->core_generics);
 	if ((req_type & REQ_IO_HIGH || req_type & REQ_IO_LOW) &&
 	    !(val & SWITCHABLE_SIGNALING_VOLTAGE)) {
 		return;
@@ -1097,12 +1304,17 @@ static void sdhci_msm_dump_pwr_ctrl_regs(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	const struct sdhci_msm_offset *msm_offset =
+					msm_host->offset;
 
 	pr_err("%s: PWRCTL_STATUS: 0x%08x | PWRCTL_MASK: 0x%08x | PWRCTL_CTL: 0x%08x\n",
-			mmc_hostname(host->mmc),
-			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS),
-			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_MASK),
-			readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL));
+		mmc_hostname(host->mmc),
+		sdhci_msm_vendor_readl_relaxed(host,
+			msm_offset->core_pwrctl_status),
+		sdhci_msm_vendor_readl_relaxed(host,
+			msm_offset->core_pwrctl_mask),
+		sdhci_msm_vendor_readl_relaxed(host,
+			msm_offset->core_pwrctl_ctl));
 }
 
 static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
@@ -1113,11 +1325,14 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 	int retry = 10;
 	u32 pwr_state = 0, io_level = 0;
 	u32 config;
+	const struct sdhci_msm_offset *msm_offset = msm_host->offset;
 
-	irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
+	irq_status = sdhci_msm_vendor_readl_relaxed(host,
+			msm_offset->core_pwrctl_status);
 	irq_status &= INT_MASK;
 
-	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
+	sdhci_msm_vendor_writel_relaxed(irq_status, host,
+			msm_offset->core_pwrctl_clear);
 
 	/*
 	 * There is a rare HW scenario where the first clear pulse could be
@@ -1126,8 +1341,8 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 	 * sure status register is cleared. Otherwise, this will result in
 	 * a spurious power IRQ resulting in system instability.
 	 */
-	while (irq_status & readl_relaxed(msm_host->core_mem +
-				CORE_PWRCTL_STATUS)) {
+	while (irq_status & sdhci_msm_vendor_readl_relaxed(host,
+		msm_offset->core_pwrctl_status)) {
 		if (retry == 0) {
 			pr_err("%s: Timedout clearing (0x%x) pwrctl status register\n",
 					mmc_hostname(host->mmc), irq_status);
@@ -1135,8 +1350,8 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 			WARN_ON(1);
 			break;
 		}
-		writel_relaxed(irq_status,
-				msm_host->core_mem + CORE_PWRCTL_CLEAR);
+		sdhci_msm_vendor_writel_relaxed(irq_status, host,
+			msm_offset->core_pwrctl_clear);
 		retry--;
 		udelay(10);
 	}
@@ -1167,7 +1382,8 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 	 * report back if it succeded or not to this register. The voltage
 	 * switches are handled by the sdhci core, so just report success.
 	 */
-	writel_relaxed(irq_ack, msm_host->core_mem + CORE_PWRCTL_CTL);
+	sdhci_msm_vendor_writel_relaxed(irq_ack, host,
+			msm_offset->core_pwrctl_ctl);
 
 	/*
 	 * If we don't have info regarding the voltage levels supported by
@@ -1186,7 +1402,8 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 		 * controllers with only 1.8V, we will set the IO PAD bit
 		 * without waiting for a REQ_IO_LOW.
 		 */
-		config = readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC);
+		config =  readl_relaxed(host->ioaddr +
+				msm_offset->core_vendor_spec);
 		new_config = config;
 
 		if ((io_level & REQ_IO_HIGH) &&
@@ -1197,8 +1414,8 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 			new_config |= CORE_IO_PAD_PWR_SWITCH;
 
 		if (config ^ new_config)
-			writel_relaxed(new_config,
-					host->ioaddr + CORE_VENDOR_SPEC);
+			writel_relaxed(new_config, host->ioaddr +
+					msm_offset->core_vendor_spec);
 	}
 
 	if (pwr_state)
@@ -1359,6 +1576,7 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
 	struct regulator *supply = mmc->supply.vqmmc;
 	u32 caps = 0, config;
 	struct sdhci_host *host = mmc_priv(mmc);
+	const struct sdhci_msm_offset *msm_offset = msm_host->offset;
 
 	if (!IS_ERR(mmc->supply.vqmmc)) {
 		if (regulator_is_supported_voltage(supply, 1700000, 1950000))
@@ -1378,7 +1596,8 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
 		 */
 		u32 io_level = msm_host->curr_io_level;
 
-		config = readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC);
+		config =  readl_relaxed(host->ioaddr +
+				msm_offset->core_vendor_spec);
 		config |= CORE_IO_PAD_PWR_SWITCH_EN;
 
 		if ((io_level & REQ_IO_HIGH) && (caps &	CORE_3_0V_SUPPORT))
@@ -1386,7 +1605,8 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
 		else if ((io_level & REQ_IO_LOW) || (caps & CORE_1_8V_SUPPORT))
 			config |= CORE_IO_PAD_PWR_SWITCH;
 
-		writel_relaxed(config, host->ioaddr + CORE_VENDOR_SPEC);
+		writel_relaxed(config,
+				host->ioaddr + msm_offset->core_vendor_spec);
 	}
 	msm_host->caps_0 |= caps;
 	pr_debug("%s: supported caps: 0x%08x\n", mmc_hostname(mmc), caps);
@@ -1394,6 +1614,7 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
 
 static const struct of_device_id sdhci_msm_dt_match[] = {
 	{ .compatible = "qcom,sdhci-msm-v4" },
+	{.compatible = "qcom,sdhci-msm-v5"},
 	{},
 };
 
@@ -1430,6 +1651,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	u16 host_version, core_minor;
 	u32 core_version, config;
 	u8 core_major;
+	const struct sdhci_msm_offset *msm_offset;
 
 	host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
 	if (IS_ERR(host))
@@ -1445,6 +1667,15 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	if (ret)
 		goto pltfm_free;
 
+	if (of_find_compatible_node(NULL, NULL, "qcom,sdhci-msm-v5")) {
+		msm_host->mci_removed = true;
+		msm_host->offset = &sdhci_msm_offset_mci_removed;
+	} else {
+		msm_host->mci_removed = false;
+		msm_host->offset = &sdhci_msm_offset_mci_present;
+	}
+	msm_offset = msm_host->offset;
+
 	sdhci_get_of_property(pdev);
 
 	msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
@@ -1509,32 +1740,37 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		dev_warn(&pdev->dev, "TCXO clk not present (%d)\n", ret);
 	}
 
-	core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	msm_host->core_mem = devm_ioremap_resource(&pdev->dev, core_memres);
+	if (!msm_host->mci_removed) {
+		core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
+				core_memres);
 
-	if (IS_ERR(msm_host->core_mem)) {
-		dev_err(&pdev->dev, "Failed to remap registers\n");
-		ret = PTR_ERR(msm_host->core_mem);
-		goto clk_disable;
+		if (IS_ERR(msm_host->core_mem)) {
+			dev_err(&pdev->dev, "Failed to remap registers\n");
+			ret = PTR_ERR(msm_host->core_mem);
+			goto clk_disable;
+		}
 	}
 
 	/* Reset the vendor spec register to power on reset state */
 	writel_relaxed(CORE_VENDOR_SPEC_POR_VAL,
-		       host->ioaddr + CORE_VENDOR_SPEC);
-
-	/* Set HC_MODE_EN bit in HC_MODE register */
-	writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE));
-
-	config = readl_relaxed(msm_host->core_mem + CORE_HC_MODE);
-	config |= FF_CLK_SW_RST_DIS;
-	writel_relaxed(config, msm_host->core_mem + CORE_HC_MODE);
+			host->ioaddr + msm_offset->core_vendor_spec);
+
+	if (!msm_host->mci_removed) {
+		/* Set HC_MODE_EN bit in HC_MODE register */
+		writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE));
+		config = readl_relaxed(msm_host->core_mem + CORE_HC_MODE);
+		config |= FF_CLK_SW_RST_DIS;
+		writel_relaxed(config, msm_host->core_mem + CORE_HC_MODE);
+	}
 
 	host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
 	dev_dbg(&pdev->dev, "Host Version: 0x%x Vendor Version 0x%x\n",
 		host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
 			       SDHCI_VENDOR_VER_SHIFT));
 
-	core_version = readl_relaxed(msm_host->core_mem + CORE_MCI_VERSION);
+	core_version =  sdhci_msm_vendor_readl_relaxed(host,
+		msm_offset->core_mci_version);
 	core_major = (core_version & CORE_VERSION_MAJOR_MASK) >>
 		      CORE_VERSION_MAJOR_SHIFT;
 	core_minor = core_version & CORE_VERSION_MINOR_MASK;
@@ -1559,7 +1795,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		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);
+				msm_offset->core_vendor_spec_capabilities0);
 	}
 
 	/*
@@ -1588,7 +1824,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 
 	sdhci_msm_init_pwr_irq_wait(msm_host);
 	/* Enable pwr irq interrupts */
-	writel_relaxed(INT_MASK, msm_host->core_mem + CORE_PWRCTL_MASK);
+	sdhci_msm_vendor_writel_relaxed(INT_MASK, host,
+		msm_offset->core_pwrctl_mask);
 
 	ret = devm_request_threaded_irq(&pdev->dev, msm_host->pwr_irq, NULL,
 					sdhci_msm_pwr_irq, IRQF_ONESHOT,
-- 
 Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH RFC 2/4] mmc: sdhci-msm: Add and use voltage regulator related APIs
  2018-05-01 10:39 [PATCH RFC 0/4] Changes for SDCC5 version Vijay Viswanath
  2018-05-01 10:39 ` [PATCH RFC 1/4] mmc: host: Register changes for sdcc V5 Vijay Viswanath
@ 2018-05-01 10:39 ` Vijay Viswanath
  2018-05-02  8:49   ` Ulf Hansson
  2018-05-01 10:39 ` [PATCH RFC 3/4] host: sdhci: fix current caps when there is no host->vmmc Vijay Viswanath
  2018-05-01 10:39 ` [PATCH RFC 4/4] host: sdhci-msm: implement get_current_limit() host op Vijay Viswanath
  3 siblings, 1 reply; 9+ messages in thread
From: Vijay Viswanath @ 2018-05-01 10:39 UTC (permalink / raw)
  To: adrian.hunter, ulf.hansson
  Cc: linux-mmc, linux-kernel, shawn.lin, linux-arm-msm, georgi.djakov,
	asutoshd, stummala, venkatg, jeremymc, vviswana, bjorn.andersson,
	riteshh, vbadigan, dianders, sayalil, Subhash Jadavani

From: Asutosh Das <asutoshd@codeaurora.org>

Some platforms require that the voltage switching happen only after
the register write occurs and controller is ready for the switch. When
the controller is ready, it will inform through power irq.

Add voltage regulator APIs and use them during power irq to switch
voltage instead of relying on core layer voltage switching.

Change-Id: Iaa98686e71a5bfe0092c68e9ffa563b060c5ac60
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 .../devicetree/bindings/mmc/sdhci-msm.txt          |  27 +-
 drivers/mmc/host/sdhci-msm.c                       | 537 +++++++++++++++++++--
 2 files changed, 529 insertions(+), 35 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
index c2b7b2b..c454046 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -23,6 +23,22 @@ Required properties:
 	"xo"	- TCXO clock (optional)
 	"cal"	- reference clock for RCLK delay calibration (optional)
 	"sleep"	- sleep clock for RCLK delay calibration (optional)
+- <supply-name>-supply: phandle to the regulator device tree node if voltage
+		regulators needs to be handled from within sdhci-msm layer.
+		Supported "supply-name" are "vdd" and "vdd-io".
+
+Optional Properties:
+	- qcom,<supply>-always-on - specifies whether supply should be kept
+		"on" always.
+	- qcom,<supply>-lpm_sup - specifies whether supply can be kept in low
+		power mode (lpm).
+	- qcom,<supply>-voltage_level - specifies voltage levels for supply.
+		Should be specified in pairs (min, max), units uV.
+	- qcom,<supply>-current_level - specifies load levels for supply in lpm
+		or high power mode (hpm). Should be specified in
+		pairs (lpm, hpm), units uA.
+
+
 
 Example:
 
@@ -33,8 +49,15 @@ Example:
 		bus-width = <8>;
 		non-removable;
 
-		vmmc-supply = <&pm8941_l20>;
-		vqmmc-supply = <&pm8941_s3>;
+		vdd-supply = <&pm8941_l20>;
+		qcom,vdd-voltage-level = <2950000 2950000>;
+		qcom,vdd-current-level = <9000 800000>;
+
+		vdd-io-supply = <&pm8941_s3>;
+		qcom,vdd-io-always-on;
+		qcom,vdd-io-lpm-sup;
+		qcom,vdd-io-voltage-level = <1800000 2950000>;
+		qcom,vdd-io-current-level = <6 22000>;
 
 		pinctrl-names = "default";
 		pinctrl-0 = <&sdc1_clk &sdc1_cmd &sdc1_data>;
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index d4d432b..0e0f12d 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -213,6 +213,33 @@ struct sdhci_msm_offset sdhci_msm_offset_mci_present = {
 	.core_ddr_config_2 = 0x1BC,
 };
 
+/* This structure keeps information per regulator */
+struct sdhci_msm_reg_data {
+	/* voltage regulator handle */
+	struct regulator *reg;
+	/* regulator name */
+	const char *name;
+	/* voltage level to be set */
+	u32 low_vol_level;
+	u32 high_vol_level;
+	/* Load values for low power and high power mode */
+	u32 lpm_uA;
+	u32 hpm_uA;
+	/* is this regulator enabled? */
+	bool is_enabled;
+	/* is this regulator needs to be always on? */
+	bool is_always_on;
+	/* is low power mode setting required for this regulator? */
+	bool lpm_sup;
+	bool set_voltage_sup;
+};
+
+struct sdhci_msm_pltfm_data {
+	/* Change-Id: Ide3a658ad51a3c3d4a05c47c0e8f013f647c9516 */
+	struct sdhci_msm_reg_data *vdd_data;
+	struct sdhci_msm_reg_data *vdd_io_data;
+};
+
 struct sdhci_msm_host {
 	struct platform_device *pdev;
 	void __iomem *core_mem;	/* MSM SDCC mapped address */
@@ -234,6 +261,8 @@ struct sdhci_msm_host {
 	u32 caps_0;
 	bool mci_removed;
 	const struct sdhci_msm_offset *offset;
+	bool pltfm_init_done;
+	struct sdhci_msm_pltfm_data pdata;
 };
 
 /*
@@ -298,6 +327,336 @@ void sdhci_msm_vendor_writel_relaxed(u32 val, struct sdhci_host *host,
 	writel_relaxed(val, base_addr + offset);
 }
 
+enum vdd_io_level {
+	/* set vdd_io_data->low_vol_level */
+	VDD_IO_LOW,
+	/* set vdd_io_data->high_vol_level */
+	VDD_IO_HIGH,
+	/*
+	 * set whatever there in voltage_level (third argument) of
+	 * sdhci_msm_set_vdd_io_vol() function.
+	 */
+	VDD_IO_SET_LEVEL,
+};
+
+#define MAX_PROP_SIZE 32
+static int sdhci_msm_dt_parse_vreg_info(struct device *dev,
+		struct sdhci_msm_reg_data **vreg_data, const char *vreg_name)
+{
+	int len, ret = 0;
+	const __be32 *prop;
+	char prop_name[MAX_PROP_SIZE];
+	struct sdhci_msm_reg_data *vreg;
+	struct device_node *np = dev->of_node;
+
+	snprintf(prop_name, MAX_PROP_SIZE, "%s-supply", vreg_name);
+	if (!of_parse_phandle(np, prop_name, 0)) {
+		dev_warn(dev, "No internal vreg data found for %s\n",
+				vreg_name);
+		ret = -EINVAL;
+		return ret;
+	}
+	vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
+	if (!vreg) {
+		ret = -ENOMEM;
+		return ret;
+	}
+	vreg->name = vreg_name;
+	snprintf(prop_name, MAX_PROP_SIZE,
+			"qcom,%s-always-on", vreg_name);
+	if (of_get_property(np, prop_name, NULL))
+		vreg->is_always_on = true;
+
+	snprintf(prop_name, MAX_PROP_SIZE,
+			"qcom,%s-lpm-sup", vreg_name);
+	if (of_get_property(np, prop_name, NULL))
+		vreg->lpm_sup = true;
+	snprintf(prop_name, MAX_PROP_SIZE,
+			"qcom,%s-voltage-level", vreg_name);
+	prop = of_get_property(np, prop_name, &len);
+	if (!prop || (len != (2 * sizeof(__be32)))) {
+		dev_warn(dev, "%s %s property\n",
+				prop ? "invalid format" : "no", prop_name);
+	} else {
+		vreg->low_vol_level = be32_to_cpup(&prop[0]);
+		vreg->high_vol_level = be32_to_cpup(&prop[1]);
+	}
+	snprintf(prop_name, MAX_PROP_SIZE,
+			"qcom,%s-current-level", vreg_name);
+	prop = of_get_property(np, prop_name, &len);
+	if (!prop || (len != (2 * sizeof(__be32)))) {
+		dev_warn(dev, "%s %s property\n",
+				prop ? "invalid format" : "no", prop_name);
+	} else {
+		vreg->lpm_uA = be32_to_cpup(&prop[0]);
+		vreg->hpm_uA = be32_to_cpup(&prop[1]);
+	}
+	*vreg_data = vreg;
+	dev_dbg(dev, "%s: %s %s vol=[%d %d]uV, curr=[%d %d]uA\n",
+			vreg->name, vreg->is_always_on ? "always_on," : "",
+			vreg->lpm_sup ? "lpm_sup," : "", vreg->low_vol_level,
+			vreg->high_vol_level, vreg->lpm_uA, vreg->hpm_uA);
+
+	return ret;
+}
+
+/* Regulator utility functions */
+static int sdhci_msm_vreg_init_reg(struct device *dev,
+					struct sdhci_msm_reg_data *vreg)
+{
+	int ret = 0;
+
+	/* check if regulator is already initialized? */
+	if (vreg->reg)
+		goto out;
+
+	/* Get the regulator handle */
+	vreg->reg = devm_regulator_get(dev, vreg->name);
+	if (IS_ERR(vreg->reg)) {
+		ret = PTR_ERR(vreg->reg);
+		pr_err("%s: devm_regulator_get(%s) failed. ret=%d\n",
+			__func__, vreg->name, ret);
+		goto out;
+	}
+
+	if (regulator_count_voltages(vreg->reg) > 0) {
+		vreg->set_voltage_sup = true;
+		/* sanity check */
+		if (!vreg->high_vol_level || !vreg->hpm_uA) {
+			pr_err("%s: %s invalid constraints specified high_vol_level: %d hpm_uA: %d\n",
+			       __func__, vreg->name, vreg->high_vol_level,
+			       vreg->hpm_uA);
+			ret = -EINVAL;
+		}
+	}
+
+out:
+	return ret;
+}
+
+static void sdhci_msm_vreg_deinit_reg(struct sdhci_msm_reg_data *vreg)
+{
+	if (vreg->reg)
+		devm_regulator_put(vreg->reg);
+}
+
+static int sdhci_msm_vreg_set_optimum_mode(struct sdhci_msm_reg_data
+						  *vreg, int uA_load)
+{
+	int ret = 0;
+
+	/*
+	 * regulators that do not support regulator_set_voltage also
+	 * do not support regulator_set_optimum_mode
+	 */
+	if (vreg->set_voltage_sup) {
+		ret = regulator_set_load(vreg->reg, uA_load);
+		if (ret < 0)
+			pr_err("%s: regulator_set_load(reg=%s,uA_load=%d) failed. ret=%d\n",
+			       __func__, vreg->name, uA_load, ret);
+		else
+			/*
+			 * regulator_set_load() can return non zero
+			 * value even for success case.
+			 */
+			ret = 0;
+	}
+	return ret;
+}
+
+static int sdhci_msm_vreg_set_voltage(struct sdhci_msm_reg_data *vreg,
+					int min_uV, int max_uV)
+{
+	int ret = 0;
+
+	if (vreg && vreg->set_voltage_sup) {
+		ret = regulator_set_voltage(vreg->reg, min_uV, max_uV);
+		if (ret) {
+			pr_err("%s: regulator_set_voltage(%s)failed. min_uV=%d,max_uV=%d,ret=%d\n",
+			       __func__, vreg->name, min_uV, max_uV, ret);
+		}
+	}
+
+	return ret;
+}
+
+static int sdhci_msm_vreg_enable(struct sdhci_msm_reg_data *vreg)
+{
+	int ret = 0;
+
+	/* Put regulator in HPM (high power mode) */
+	ret = sdhci_msm_vreg_set_optimum_mode(vreg, vreg->hpm_uA);
+	if (ret < 0)
+		return ret;
+
+	if (!vreg->is_enabled) {
+		/* Set voltage level */
+		ret = sdhci_msm_vreg_set_voltage(vreg, vreg->high_vol_level,
+						vreg->high_vol_level);
+		if (ret)
+			return ret;
+	}
+	ret = regulator_enable(vreg->reg);
+	if (ret) {
+		pr_err("%s: regulator_enable(%s) failed. ret=%d\n",
+				__func__, vreg->name, ret);
+		return ret;
+	}
+	vreg->is_enabled = true;
+	return ret;
+}
+
+static int sdhci_msm_vreg_disable(struct sdhci_msm_reg_data *vreg)
+{
+	int ret = 0;
+
+	/* Never disable regulator marked as always_on */
+	if (vreg->is_enabled && !vreg->is_always_on) {
+		ret = regulator_disable(vreg->reg);
+		if (ret) {
+			pr_err("%s: regulator_disable(%s) failed. ret=%d\n",
+				__func__, vreg->name, ret);
+			goto out;
+		}
+		vreg->is_enabled = false;
+
+		ret = sdhci_msm_vreg_set_optimum_mode(vreg, 0);
+		if (ret < 0)
+			goto out;
+
+		/* Set min. voltage level to 0 */
+		ret = sdhci_msm_vreg_set_voltage(vreg, 0, vreg->high_vol_level);
+		if (ret)
+			goto out;
+	} else if (vreg->is_enabled && vreg->is_always_on) {
+		if (vreg->lpm_sup) {
+			/* Put always_on regulator in LPM (low power mode) */
+			ret = sdhci_msm_vreg_set_optimum_mode(vreg,
+							      vreg->lpm_uA);
+			if (ret < 0)
+				goto out;
+		}
+	}
+out:
+	return ret;
+}
+
+static int sdhci_msm_setup_vreg(struct sdhci_msm_pltfm_data *pdata,
+			bool enable, bool is_init)
+{
+	int ret = 0, i;
+	struct sdhci_msm_reg_data *vreg_table[2];
+
+	vreg_table[0] = pdata->vdd_data;
+	vreg_table[1] = pdata->vdd_io_data;
+
+	for (i = 0; i < ARRAY_SIZE(vreg_table); i++) {
+		if (vreg_table[i]) {
+			if (enable)
+				ret = sdhci_msm_vreg_enable(vreg_table[i]);
+			else
+				ret = sdhci_msm_vreg_disable(vreg_table[i]);
+			if (ret)
+				goto out;
+		}
+	}
+out:
+	return ret;
+}
+
+/* This init function should be called only once for each SDHC slot */
+static int sdhci_msm_vreg_init(struct device *dev,
+				struct sdhci_msm_pltfm_data *pdata,
+				bool is_init)
+{
+	int ret = 0;
+	struct sdhci_msm_reg_data *curr_vdd_reg, *curr_vdd_io_reg;
+
+
+	curr_vdd_reg = pdata->vdd_data;
+	curr_vdd_io_reg = pdata->vdd_io_data;
+
+	if (!is_init)
+		/* Deregister all regulators from regulator framework */
+		goto vdd_io_reg_deinit;
+
+	/*
+	 * Get the regulator handle from voltage regulator framework
+	 * and then try to set the voltage level for the regulator
+	 */
+	if (curr_vdd_reg) {
+		ret = sdhci_msm_vreg_init_reg(dev, curr_vdd_reg);
+		if (ret)
+			goto out;
+	}
+	if (curr_vdd_io_reg) {
+		ret = sdhci_msm_vreg_init_reg(dev, curr_vdd_io_reg);
+		if (ret)
+			goto vdd_reg_deinit;
+	}
+
+	goto out;
+
+vdd_io_reg_deinit:
+	if (curr_vdd_io_reg)
+		sdhci_msm_vreg_deinit_reg(curr_vdd_io_reg);
+vdd_reg_deinit:
+	if (curr_vdd_reg)
+		sdhci_msm_vreg_deinit_reg(curr_vdd_reg);
+out:
+	if (ret)
+		dev_err(dev, "vreg reset failed (%d)\n", ret);
+	return ret;
+}
+
+static int sdhci_msm_set_vdd_io_vol(struct sdhci_msm_pltfm_data *pdata,
+			enum vdd_io_level level,
+			unsigned int voltage_level)
+{
+	int ret = 0;
+	int set_level;
+	struct sdhci_msm_reg_data *vdd_io_reg;
+
+	vdd_io_reg = pdata->vdd_io_data;
+	if (vdd_io_reg && vdd_io_reg->is_enabled) {
+		switch (level) {
+		case VDD_IO_LOW:
+			set_level = vdd_io_reg->low_vol_level;
+			break;
+		case VDD_IO_HIGH:
+			set_level = vdd_io_reg->high_vol_level;
+			break;
+		case VDD_IO_SET_LEVEL:
+			set_level = voltage_level;
+			break;
+		default:
+			pr_err("%s: invalid argument level = %d",
+					__func__, level);
+			ret = -EINVAL;
+			return ret;
+		}
+		ret = sdhci_msm_vreg_set_voltage(vdd_io_reg, set_level,
+				set_level);
+	}
+	return ret;
+}
+
+/* Parse platform data */
+static void sdhci_msm_populate_pdata(struct device *dev,
+		struct sdhci_msm_pltfm_data *pdata)
+{
+	int ret = 0;
+
+	ret = sdhci_msm_dt_parse_vreg_info(dev, &pdata->vdd_data, "vdd");
+	if (ret)
+		dev_warn(dev, "failed parsing vdd data (%d)\n", ret);
+
+	ret = sdhci_msm_dt_parse_vreg_info(dev, &pdata->vdd_io_data, "vdd-io");
+	if (ret)
+		dev_warn(dev, "failed parsing vdd-io data (%d)\n", ret);
+
+}
+
 static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
 						    unsigned int clock)
 {
@@ -1326,6 +1685,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 	u32 pwr_state = 0, io_level = 0;
 	u32 config;
 	const struct sdhci_msm_offset *msm_offset = msm_host->offset;
+	int ret = 0;
 
 	irq_status = sdhci_msm_vendor_readl_relaxed(host,
 			msm_offset->core_pwrctl_status);
@@ -1358,21 +1718,54 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 
 	/* Handle BUS ON/OFF*/
 	if (irq_status & CORE_PWRCTL_BUS_ON) {
+		ret = sdhci_msm_setup_vreg(&msm_host->pdata, true, false);
+		if (!ret) {
+			ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata,
+					VDD_IO_HIGH, 0);
+			if (ret)
+				pr_err("%s: error setting vdd io in BUS_ON: %d\n",
+						mmc_hostname(host->mmc), ret);
+		} else {
+			pr_err("%s: error setting up vreg ON : %d\n",
+					mmc_hostname(host->mmc), ret);
+		}
 		pwr_state = REQ_BUS_ON;
 		io_level = REQ_IO_HIGH;
 		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
 	}
 	if (irq_status & CORE_PWRCTL_BUS_OFF) {
+		if (msm_host->pltfm_init_done)
+			ret = sdhci_msm_setup_vreg(&msm_host->pdata,
+					false, false);
+		if (!ret) {
+			ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata,
+					VDD_IO_LOW, 0);
+			if (ret)
+				pr_err("%s: error setting vdd io in BUS_OFF: %d\n",
+						mmc_hostname(host->mmc), ret);
+		} else {
+			pr_err("%s: error setting up vreg OFF: %d\n",
+					mmc_hostname(host->mmc), ret);
+		}
 		pwr_state = REQ_BUS_OFF;
 		io_level = REQ_IO_LOW;
 		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
 	}
 	/* Handle IO LOW/HIGH */
 	if (irq_status & CORE_PWRCTL_IO_LOW) {
+		ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata, VDD_IO_LOW, 0);
+		if (ret)
+			pr_err("%s: error setting up vdd io low: %d\n",
+					mmc_hostname(host->mmc), ret);
 		io_level = REQ_IO_LOW;
 		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
 	}
 	if (irq_status & CORE_PWRCTL_IO_HIGH) {
+		ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata,
+				VDD_IO_HIGH, 0);
+		if (ret)
+			pr_err("%s: error setting up vdd io high: %d\n",
+					mmc_hostname(host->mmc), ret);
 		io_level = REQ_IO_HIGH;
 		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
 	}
@@ -1380,7 +1773,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 	/*
 	 * The driver has to acknowledge the interrupt, switch voltages and
 	 * report back if it succeded or not to this register. The voltage
-	 * switches are handled by the sdhci core, so just report success.
+	 * switches may be handled by the sdhci core, so just report success.
 	 */
 	sdhci_msm_vendor_writel_relaxed(irq_ack, host,
 			msm_offset->core_pwrctl_ctl);
@@ -1612,6 +2005,74 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
 	pr_debug("%s: supported caps: 0x%08x\n", mmc_hostname(mmc), caps);
 }
 
+static void sdhci_msm_set_default_hw_caps(struct device *dev,
+		struct sdhci_msm_host *msm_host,
+		struct sdhci_host *host)
+{
+	u32 version, config;
+	u16 minor;
+	u8 major;
+	const struct sdhci_msm_offset *msm_offset = msm_host->offset;
+	struct sdhci_msm_reg_data *vdd_io_reg = msm_host->pdata.vdd_io_data;
+	u32 caps = 0;
+
+	version =  sdhci_msm_vendor_readl_relaxed(host,
+			msm_offset->core_mci_version);
+	major = (version & CORE_VERSION_MAJOR_MASK) >>
+		CORE_VERSION_MAJOR_SHIFT;
+	minor = version & CORE_VERSION_MINOR_MASK;
+	dev_dbg(dev, "MCI Version: 0x%08x, major: 0x%04x, minor: 0x%02x\n",
+			version, major, minor);
+
+	/*
+	 * If voltage regulators are controlled by msm host layer and
+	 * IO voltage regulator can support 1.8V, we need to
+	 * update the capabilities here before sdhci host is added.
+	 */
+	if (vdd_io_reg && (vdd_io_reg->low_vol_level < 1950000))
+		caps |= CORE_1_8V_SUPPORT;
+	if (vdd_io_reg && (vdd_io_reg->low_vol_level > 2700000))
+		caps |= CORE_3_0V_SUPPORT;
+	if (caps) {
+		msm_host->caps_0 |= caps;
+		config =  readl_relaxed(host->ioaddr +
+				msm_offset->core_vendor_spec);
+		config |= CORE_IO_PAD_PWR_SWITCH_EN;
+		writel_relaxed(config,
+				host->ioaddr + msm_offset->core_vendor_spec);
+	}
+
+	caps = readl_relaxed(host->ioaddr + SDHCI_CAPABILITIES);
+
+	if (major == 1 && minor >= 0x42)
+		msm_host->use_14lpp_dll_reset = true;
+	/*
+	 * SDCC 5 controller with major version 1, minor version 0x34 and later
+	 * with HS 400 mode support will use CM DLL instead of CDC LP 533 DLL.
+	 */
+	if (major == 1 && minor < 0x34)
+		msm_host->use_cdclp533 = true;
+
+	/*
+	 * Support for some capabilities is not advertised by newer
+	 * controller versions and must be explicitly enabled.
+	 */
+	if (major >= 1 && minor != 0x11 && minor != 0x12) {
+
+		vdd_io_reg = msm_host->pdata.vdd_io_data;
+		caps |= SDHCI_CAN_VDD_300 | SDHCI_CAN_DO_8BIT;
+		if (msm_host->caps_0 & CORE_1_8V_SUPPORT)
+			caps |= CORE_1_8V_SUPPORT;
+	}
+	msm_host->caps_0 |= caps;
+	writel_relaxed(msm_host->caps_0, host->ioaddr +
+			msm_offset->core_vendor_spec_capabilities0);
+
+	/* Set more host capabilities */
+	msm_host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
+	msm_host->mmc->caps2 |= MMC_CAP2_BOOTPART_NOACC;
+}
+
 static const struct of_device_id sdhci_msm_dt_match[] = {
 	{ .compatible = "qcom,sdhci-msm-v4" },
 	{.compatible = "qcom,sdhci-msm-v5"},
@@ -1648,9 +2109,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	struct resource *core_memres;
 	struct clk *clk;
 	int ret;
-	u16 host_version, core_minor;
-	u32 core_version, config;
-	u8 core_major;
+	u16 host_version;
+	u32 config;
 	const struct sdhci_msm_offset *msm_offset;
 
 	host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
@@ -1678,6 +2138,29 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 
 	sdhci_get_of_property(pdev);
 
+	sdhci_msm_populate_pdata(&pdev->dev, &msm_host->pdata);
+	/*
+	 * If voltage regulators are controlled internally, set the caps2
+	 * based on regulator capability
+	 */
+	if (msm_host->pdata.vdd_io_data) {
+		struct sdhci_msm_reg_data *vdd_io = msm_host->pdata.vdd_io_data;
+		u32 caps = 0;
+
+		if (vdd_io->high_vol_level > 2700000)
+			caps |= CORE_3_0V_SUPPORT;
+		if (vdd_io->low_vol_level < 1950000)
+			caps |= CORE_1_8V_SUPPORT;
+		if (caps) {
+			msm_host->caps_0 |= caps;
+			config =  readl_relaxed(host->ioaddr +
+				msm_offset->core_vendor_spec);
+			config |= CORE_IO_PAD_PWR_SWITCH_EN;
+			writel_relaxed(config,
+				host->ioaddr + msm_offset->core_vendor_spec);
+		}
+	}
+
 	msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
 
 	/* Setup SDCC bus voter clock. */
@@ -1740,6 +2223,13 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		dev_warn(&pdev->dev, "TCXO clk not present (%d)\n", ret);
 	}
 
+	/* Setup regulators if they are controlled internally */
+	ret = sdhci_msm_vreg_init(&pdev->dev, &msm_host->pdata, true);
+	if (ret) {
+		dev_err(&pdev->dev, "Regulator setup failed (%d)\n", ret);
+		goto clk_disable;
+	}
+
 	if (!msm_host->mci_removed) {
 		core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 		msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
@@ -1748,7 +2238,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		if (IS_ERR(msm_host->core_mem)) {
 			dev_err(&pdev->dev, "Failed to remap registers\n");
 			ret = PTR_ERR(msm_host->core_mem);
-			goto clk_disable;
+			goto vreg_deinit;
 		}
 	}
 
@@ -1769,34 +2259,11 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
 			       SDHCI_VENDOR_VER_SHIFT));
 
-	core_version =  sdhci_msm_vendor_readl_relaxed(host,
-		msm_offset->core_mci_version);
-	core_major = (core_version & CORE_VERSION_MAJOR_MASK) >>
-		      CORE_VERSION_MAJOR_SHIFT;
-	core_minor = core_version & CORE_VERSION_MINOR_MASK;
-	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;
-
-	/*
-	 * SDCC 5 controller with major version 1, minor version 0x34 and later
-	 * with HS 400 mode support will use CM DLL instead of CDC LP 533 DLL.
-	 */
-	if (core_major == 1 && core_minor < 0x34)
-		msm_host->use_cdclp533 = true;
-
 	/*
-	 * Support for some capabilities is not advertised by newer
-	 * controller versions and must be explicitly enabled.
+	 * Based on controller version and voltage regulator support,
+	 * enable some properties.
 	 */
-	if (core_major >= 1 && core_minor != 0x11 && core_minor != 0x12) {
-		config = readl_relaxed(host->ioaddr + SDHCI_CAPABILITIES);
-		config |= SDHCI_CAN_VDD_300 | SDHCI_CAN_DO_8BIT;
-		writel_relaxed(config, host->ioaddr +
-				msm_offset->core_vendor_spec_capabilities0);
-	}
+	sdhci_msm_set_default_hw_caps(&pdev->dev, msm_host, host);
 
 	/*
 	 * Power on reset state may trigger power irq if previous status of
@@ -1819,7 +2286,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Get pwr_irq failed (%d)\n",
 			msm_host->pwr_irq);
 		ret = msm_host->pwr_irq;
-		goto clk_disable;
+		goto vreg_deinit;
 	}
 
 	sdhci_msm_init_pwr_irq_wait(msm_host);
@@ -1832,7 +2299,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 					dev_name(&pdev->dev), host);
 	if (ret) {
 		dev_err(&pdev->dev, "Request IRQ failed (%d)\n", ret);
-		goto clk_disable;
+		goto vreg_deinit;
 	}
 
 	pm_runtime_get_noresume(&pdev->dev);
@@ -1857,6 +2324,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_set_suspended(&pdev->dev);
 	pm_runtime_put_noidle(&pdev->dev);
+vreg_deinit:
+	sdhci_msm_vreg_init(&pdev->dev, &msm_host->pdata, false);
 clk_disable:
 	clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks),
 				   msm_host->bulk_clks);
@@ -1882,6 +2351,8 @@ static int sdhci_msm_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_put_noidle(&pdev->dev);
 
+	sdhci_msm_vreg_init(&pdev->dev, &msm_host->pdata, false);
+
 	clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks),
 				   msm_host->bulk_clks);
 	if (!IS_ERR(msm_host->bus_clk))
-- 
 Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH RFC 3/4] host: sdhci: fix current caps when there is no host->vmmc
  2018-05-01 10:39 [PATCH RFC 0/4] Changes for SDCC5 version Vijay Viswanath
  2018-05-01 10:39 ` [PATCH RFC 1/4] mmc: host: Register changes for sdcc V5 Vijay Viswanath
  2018-05-01 10:39 ` [PATCH RFC 2/4] mmc: sdhci-msm: Add and use voltage regulator related APIs Vijay Viswanath
@ 2018-05-01 10:39 ` Vijay Viswanath
  2018-05-01 10:39 ` [PATCH RFC 4/4] host: sdhci-msm: implement get_current_limit() host op Vijay Viswanath
  3 siblings, 0 replies; 9+ messages in thread
From: Vijay Viswanath @ 2018-05-01 10:39 UTC (permalink / raw)
  To: adrian.hunter, ulf.hansson
  Cc: linux-mmc, linux-kernel, shawn.lin, linux-arm-msm, georgi.djakov,
	asutoshd, stummala, venkatg, jeremymc, vviswana, bjorn.andersson,
	riteshh, vbadigan, dianders, sayalil

From: Sahitya Tummala <stummala@codeaurora.org>

When the regulators are not managed by SDHCI host driver (i.e.,
when host->vmmc and host->vmmcq are absent), get the regulator
current capabilities through a new host op get_current_limit().

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
[vviswana@codeaurora.org: fixed trivial merge conflicts]
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>

Change-Id: I3370a2411beec1f03cc5f102bf95cd816c60351e
---
 drivers/mmc/host/sdhci.c | 11 ++++++++---
 drivers/mmc/host/sdhci.h |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0d5fcca..edc2ccd 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3570,10 +3570,15 @@ int sdhci_setup_host(struct sdhci_host *host)
 	 * value.
 	 */
 	max_current_caps = sdhci_readl(host, SDHCI_MAX_CURRENT);
-	if (!max_current_caps && !IS_ERR(mmc->supply.vmmc)) {
-		int curr = regulator_get_current_limit(mmc->supply.vmmc);
-		if (curr > 0) {
+	if (!max_current_caps) {
+		u32 curr = 0;
+
+		if (!IS_ERR(mmc->supply.vmmc))
+			curr = regulator_get_current_limit(mmc->supply.vmmc);
+		else if (host->ops->get_current_limit)
+			curr = host->ops->get_current_limit(host);
 
+		if (curr > 0) {
 			/* convert to SDHCI_MAX_CURRENT format */
 			curr = curr/1000;  /* convert to mA */
 			curr = curr/SDHCI_MAX_CURRENT_MULTIPLIER;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 54bc444..a01af78 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -584,6 +584,7 @@ struct sdhci_ops {
 	void    (*adma_workaround)(struct sdhci_host *host, u32 intmask);
 	void    (*card_event)(struct sdhci_host *host);
 	void	(*voltage_switch)(struct sdhci_host *host);
+	unsigned int	(*get_current_limit)(struct sdhci_host *host);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
-- 
 Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH RFC 4/4] host: sdhci-msm: implement get_current_limit() host op
  2018-05-01 10:39 [PATCH RFC 0/4] Changes for SDCC5 version Vijay Viswanath
                   ` (2 preceding siblings ...)
  2018-05-01 10:39 ` [PATCH RFC 3/4] host: sdhci: fix current caps when there is no host->vmmc Vijay Viswanath
@ 2018-05-01 10:39 ` Vijay Viswanath
  3 siblings, 0 replies; 9+ messages in thread
From: Vijay Viswanath @ 2018-05-01 10:39 UTC (permalink / raw)
  To: adrian.hunter, ulf.hansson
  Cc: linux-mmc, linux-kernel, shawn.lin, linux-arm-msm, georgi.djakov,
	asutoshd, stummala, venkatg, jeremymc, vviswana, bjorn.andersson,
	riteshh, vbadigan, dianders, sayalil

From: Sahitya Tummala <stummala@codeaurora.org>

This is needed to get the current capabilities of vdd
regulator that is not managed by SDHCI driver.

Change-Id: I927c14b9890f1d672fe8a3e89d0b334f43463b36
Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 0e0f12d..083b4a5 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -2079,6 +2079,18 @@ static void sdhci_msm_set_default_hw_caps(struct device *dev,
 	{},
 };
 
+static unsigned int sdhci_msm_get_current_limit(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	struct sdhci_msm_reg_data *vdd_data = msm_host->pdata.vdd_data;
+	u32 max_curr = 0;
+
+	if (vdd_data)
+		max_curr = vdd_data->hpm_uA;
+	return max_curr;
+}
+
 MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
 
 static const struct sdhci_ops sdhci_msm_ops = {
@@ -2090,6 +2102,7 @@ static void sdhci_msm_set_default_hw_caps(struct device *dev,
 	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
 	.write_w = sdhci_msm_writew,
 	.write_b = sdhci_msm_writeb,
+	.get_current_limit = sdhci_msm_get_current_limit,
 };
 
 static const struct sdhci_pltfm_data sdhci_msm_pdata = {
-- 
 Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 1/4] mmc: host: Register changes for sdcc V5
  2018-05-01 10:39 ` [PATCH RFC 1/4] mmc: host: Register changes for sdcc V5 Vijay Viswanath
@ 2018-05-02  8:28   ` Ulf Hansson
  2018-05-03  9:37     ` Vijay Viswanath
  0 siblings, 1 reply; 9+ messages in thread
From: Ulf Hansson @ 2018-05-02  8:28 UTC (permalink / raw)
  To: Vijay Viswanath
  Cc: Adrian Hunter, linux-mmc, Linux Kernel Mailing List, Shawn Lin,
	linux-arm-msm, Georgi Djakov, Asutosh Das, Sahitya Tummala,
	Venkat Gopalakrishnan, Jeremy McNicoll, Bjorn Andersson,
	Harjani Ritesh, vbadigan, Doug Anderson, sayalil

On 1 May 2018 at 12:39, Vijay Viswanath <vviswana@codeaurora.org> wrote:
> From: Sayali Lokhande <sayalil@codeaurora.org>
>
> For SDCC version 5.0.0, MCI registers are removed from SDCC
> interface and some registers are moved to HC. This change is
> to support MCI register removal for msmfalcon. New compatible
> string "qcom,sdhci-msm-v5" is added for msmfalcon to support
> this change.

To simplify review, I would recommend to split this up in a few more
pieces, a few re-factoring while the final change adds the
qcom,sdhci-msm-v5 support.

>
> Change-Id: I0febfd9bb2222436a8eff20c20107dd4180c9781
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  .../devicetree/bindings/mmc/sdhci-msm.txt          |   5 +-

Please move DT changes into separate patches and make sure to sent it
to DT maintainers as well.

>  drivers/mmc/host/sdhci-msm.c                       | 485 +++++++++++++++------
>  2 files changed, 365 insertions(+), 125 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> index bfdcdc4..c2b7b2b 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> @@ -4,7 +4,10 @@ This file documents differences between the core properties in mmc.txt
>  and the properties used by the sdhci-msm driver.
>
>  Required properties:
> -- compatible: Should contain "qcom,sdhci-msm-v4".
> +- compatible: Should contain "qcom,sdhci-msm-v4" or "qcom,sdhci-msm-v5".
> +                For SDCC version 5.0.0, MCI registers are removed from SDCC
> +                interface and some registers are moved to HC. New compatible
> +                string is added to support this change - "qcom,sdhci-msm-v5".
>  - reg: Base address and length of the register in the following order:
>         - Host controller register map (required)
>         - SD Core register map (required)
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index bb11916..d4d432b 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c

[...]

>
> +struct sdhci_msm_offset {

I think a better idea is to create a struct sdhci_msm_variant and make
it contain all the variant specific data. So, not only the offsets,
but other variant data as well.

More comments below.

> +       u32 core_mci_data_cnt;
> +       u32 core_mci_status;
> +       u32 core_mci_fifo_cnt;
> +       u32 core_mci_version;
> +       u32 core_generics;
> +       u32 core_testbus_config;
> +       u32 core_testbus_sel2_bit;
> +       u32 core_testbus_ena;
> +       u32 core_testbus_sel2;
> +       u32 core_pwrctl_status;
> +       u32 core_pwrctl_mask;
> +       u32 core_pwrctl_clear;
> +       u32 core_pwrctl_ctl;
> +       u32 core_sdcc_debug_reg;
> +       u32 core_dll_config;
> +       u32 core_dll_status;
> +       u32 core_vendor_spec;
> +       u32 core_vendor_spec_adma_err_addr0;
> +       u32 core_vendor_spec_adma_err_addr1;
> +       u32 core_vendor_spec_func2;
> +       u32 core_vendor_spec_capabilities0;
> +       u32 core_ddr_200_cfg;
> +       u32 core_vendor_spec3;
> +       u32 core_dll_config_2;
> +       u32 core_ddr_config;
> +       u32 core_ddr_config_2;
> +};
> +
> +struct sdhci_msm_offset sdhci_msm_offset_mci_removed = {
> +       .core_mci_data_cnt = 0x35c,
> +       .core_mci_status = 0x324,
> +       .core_mci_fifo_cnt = 0x308,
> +       .core_mci_version = 0x318,
> +       .core_generics = 0x320,
> +       .core_testbus_config = 0x32c,
> +       .core_testbus_sel2_bit = 3,
> +       .core_testbus_ena = (1 << 31),
> +       .core_testbus_sel2 = (1 << 3),
> +       .core_pwrctl_status = 0x240,
> +       .core_pwrctl_mask = 0x244,
> +       .core_pwrctl_clear = 0x248,
> +       .core_pwrctl_ctl = 0x24c,
> +       .core_sdcc_debug_reg = 0x358,
> +       .core_dll_config = 0x200,
> +       .core_dll_status = 0x208,
> +       .core_vendor_spec = 0x20c,
> +       .core_vendor_spec_adma_err_addr0 = 0x214,
> +       .core_vendor_spec_adma_err_addr1 = 0x218,
> +       .core_vendor_spec_func2 = 0x210,
> +       .core_vendor_spec_capabilities0 = 0x21c,
> +       .core_ddr_200_cfg = 0x224,
> +       .core_vendor_spec3 = 0x250,
> +       .core_dll_config_2 = 0x254,
> +       .core_ddr_config = 0x258,
> +       .core_ddr_config_2 = 0x25c,
> +};
> +
> +struct sdhci_msm_offset sdhci_msm_offset_mci_present = {
> +       .core_mci_data_cnt = 0x30,
> +       .core_mci_status = 0x34,
> +       .core_mci_fifo_cnt = 0x44,
> +       .core_mci_version = 0x050,
> +       .core_generics = 0x70,
> +       .core_testbus_config = 0x0CC,
> +       .core_testbus_sel2_bit = 4,
> +       .core_testbus_ena = (1 << 3),
> +       .core_testbus_sel2 = (1 << 4),
> +       .core_pwrctl_status = 0xDC,
> +       .core_pwrctl_mask = 0xE0,
> +       .core_pwrctl_clear = 0xE4,
> +       .core_pwrctl_ctl = 0xE8,
> +       .core_sdcc_debug_reg = 0x124,
> +       .core_dll_config = 0x100,
> +       .core_dll_status = 0x108,
> +       .core_vendor_spec = 0x10C,
> +       .core_vendor_spec_adma_err_addr0 = 0x114,
> +       .core_vendor_spec_adma_err_addr1 = 0x118,
> +       .core_vendor_spec_func2 = 0x110,
> +       .core_vendor_spec_capabilities0 = 0x11C,
> +       .core_ddr_200_cfg = 0x184,
> +       .core_vendor_spec3 = 0x1B0,
> +       .core_dll_config_2 = 0x1B4,
> +       .core_ddr_config = 0x1B8,
> +       .core_ddr_config_2 = 0x1BC,
> +};
> +
>  struct sdhci_msm_host {
>         struct platform_device *pdev;
>         void __iomem *core_mem; /* MSM SDCC mapped address */
> @@ -156,8 +232,72 @@ struct sdhci_msm_host {
>         wait_queue_head_t pwr_irq_wait;
>         bool pwr_irq_flag;
>         u32 caps_0;
> +       bool mci_removed;
> +       const struct sdhci_msm_offset *offset;
>  };
>
> +/*
> + * APIs to write to vendor specific registers which were there in the core_mem
> + * region before MCI was removed.
> + */
> +u8 sdhci_msm_vendor_readb_relaxed(struct sdhci_host *host, u32 offset)
> +{
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +       void __iomem *base_addr;
> +
> +       if (msm_host->mci_removed)
> +               base_addr = host->ioaddr;

I would rather use a separate set of functions, which you assign at ->probe().

That may speed things up a little bit, as you don't need to check for
"mci_removed", each time in these read/write functions.

> +       else
> +               base_addr = msm_host->core_mem;
> +
> +       return readb_relaxed(base_addr + offset);
> +}

[...]

>  static const struct of_device_id sdhci_msm_dt_match[] = {
>         { .compatible = "qcom,sdhci-msm-v4" },
> +       {.compatible = "qcom,sdhci-msm-v5"},
>         {},
>  };
>
> @@ -1430,6 +1651,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>         u16 host_version, core_minor;
>         u32 core_version, config;
>         u8 core_major;
> +       const struct sdhci_msm_offset *msm_offset;
>
>         host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
>         if (IS_ERR(host))
> @@ -1445,6 +1667,15 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>         if (ret)
>                 goto pltfm_free;
>
> +       if (of_find_compatible_node(NULL, NULL, "qcom,sdhci-msm-v5")) {
> +               msm_host->mci_removed = true;
> +               msm_host->offset = &sdhci_msm_offset_mci_removed;

I would suggest to extend your array of "struct of_device_id" to use
the .data member to store all the variant specific data (such as the
offsets for example).

Then you can use of_device_get_match_data(), to get the variant data.

> +       } else {
> +               msm_host->mci_removed = false;
> +               msm_host->offset = &sdhci_msm_offset_mci_present;
> +       }
> +       msm_offset = msm_host->offset;
> +

[...]

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 2/4] mmc: sdhci-msm: Add and use voltage regulator related APIs
  2018-05-01 10:39 ` [PATCH RFC 2/4] mmc: sdhci-msm: Add and use voltage regulator related APIs Vijay Viswanath
@ 2018-05-02  8:49   ` Ulf Hansson
  2018-05-03  9:39     ` Vijay Viswanath
  0 siblings, 1 reply; 9+ messages in thread
From: Ulf Hansson @ 2018-05-02  8:49 UTC (permalink / raw)
  To: Vijay Viswanath
  Cc: Adrian Hunter, linux-mmc, Linux Kernel Mailing List, Shawn Lin,
	linux-arm-msm, Georgi Djakov, Asutosh Das, Sahitya Tummala,
	Venkat Gopalakrishnan, Jeremy McNicoll, Bjorn Andersson,
	Harjani Ritesh, vbadigan, Doug Anderson, sayalil,
	Subhash Jadavani

On 1 May 2018 at 12:39, Vijay Viswanath <vviswana@codeaurora.org> wrote:
> From: Asutosh Das <asutoshd@codeaurora.org>
>
> Some platforms require that the voltage switching happen only after
> the register write occurs and controller is ready for the switch. When
> the controller is ready, it will inform through power irq.
>
> Add voltage regulator APIs and use them during power irq to switch
> voltage instead of relying on core layer voltage switching.

This is way to simplified. 529 lines of new code deserves some more
explanations.

>
> Change-Id: Iaa98686e71a5bfe0092c68e9ffa563b060c5ac60

Remove this.

> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  .../devicetree/bindings/mmc/sdhci-msm.txt          |  27 +-

Split DT changes and add DT maintainers.

>  drivers/mmc/host/sdhci-msm.c                       | 537 +++++++++++++++++++--
>  2 files changed, 529 insertions(+), 35 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> index c2b7b2b..c454046 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> @@ -23,6 +23,22 @@ Required properties:
>         "xo"    - TCXO clock (optional)
>         "cal"   - reference clock for RCLK delay calibration (optional)
>         "sleep" - sleep clock for RCLK delay calibration (optional)
> +- <supply-name>-supply: phandle to the regulator device tree node if voltage
> +               regulators needs to be handled from within sdhci-msm layer.
> +               Supported "supply-name" are "vdd" and "vdd-io".
> +
> +Optional Properties:
> +       - qcom,<supply>-always-on - specifies whether supply should be kept
> +               "on" always.
> +       - qcom,<supply>-lpm_sup - specifies whether supply can be kept in low
> +               power mode (lpm).
> +       - qcom,<supply>-voltage_level - specifies voltage levels for supply.
> +               Should be specified in pairs (min, max), units uV.
> +       - qcom,<supply>-current_level - specifies load levels for supply in lpm
> +               or high power mode (hpm). Should be specified in
> +               pairs (lpm, hpm), units uA.
> +
> +

This looks really weird.

What's so special here that requires you to have your own specific
regulator bindings?

>
>  Example:
>
> @@ -33,8 +49,15 @@ Example:
>                 bus-width = <8>;
>                 non-removable;
>
> -               vmmc-supply = <&pm8941_l20>;
> -               vqmmc-supply = <&pm8941_s3>;
> +               vdd-supply = <&pm8941_l20>;
> +               qcom,vdd-voltage-level = <2950000 2950000>;
> +               qcom,vdd-current-level = <9000 800000>;
> +
> +               vdd-io-supply = <&pm8941_s3>;
> +               qcom,vdd-io-always-on;
> +               qcom,vdd-io-lpm-sup;
> +               qcom,vdd-io-voltage-level = <1800000 2950000>;
> +               qcom,vdd-io-current-level = <6 22000>;
>
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&sdc1_clk &sdc1_cmd &sdc1_data>;
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index d4d432b..0e0f12d 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -213,6 +213,33 @@ struct sdhci_msm_offset sdhci_msm_offset_mci_present = {
>         .core_ddr_config_2 = 0x1BC,
>  };
>
> +/* This structure keeps information per regulator */
> +struct sdhci_msm_reg_data {
> +       /* voltage regulator handle */
> +       struct regulator *reg;
> +       /* regulator name */
> +       const char *name;
> +       /* voltage level to be set */
> +       u32 low_vol_level;
> +       u32 high_vol_level;
> +       /* Load values for low power and high power mode */
> +       u32 lpm_uA;
> +       u32 hpm_uA;
> +       /* is this regulator enabled? */
> +       bool is_enabled;
> +       /* is this regulator needs to be always on? */
> +       bool is_always_on;
> +       /* is low power mode setting required for this regulator? */
> +       bool lpm_sup;
> +       bool set_voltage_sup;
> +};
> +
> +struct sdhci_msm_pltfm_data {
> +       /* Change-Id: Ide3a658ad51a3c3d4a05c47c0e8f013f647c9516 */
> +       struct sdhci_msm_reg_data *vdd_data;
> +       struct sdhci_msm_reg_data *vdd_io_data;
> +};
> +
>  struct sdhci_msm_host {
>         struct platform_device *pdev;
>         void __iomem *core_mem; /* MSM SDCC mapped address */
> @@ -234,6 +261,8 @@ struct sdhci_msm_host {
>         u32 caps_0;
>         bool mci_removed;
>         const struct sdhci_msm_offset *offset;
> +       bool pltfm_init_done;
> +       struct sdhci_msm_pltfm_data pdata;
>  };
>
>  /*
> @@ -298,6 +327,336 @@ void sdhci_msm_vendor_writel_relaxed(u32 val, struct sdhci_host *host,
>         writel_relaxed(val, base_addr + offset);
>  }
>
> +enum vdd_io_level {
> +       /* set vdd_io_data->low_vol_level */
> +       VDD_IO_LOW,
> +       /* set vdd_io_data->high_vol_level */
> +       VDD_IO_HIGH,
> +       /*
> +        * set whatever there in voltage_level (third argument) of
> +        * sdhci_msm_set_vdd_io_vol() function.
> +        */
> +       VDD_IO_SET_LEVEL,
> +};
> +
> +#define MAX_PROP_SIZE 32
> +static int sdhci_msm_dt_parse_vreg_info(struct device *dev,
> +               struct sdhci_msm_reg_data **vreg_data, const char *vreg_name)
> +{
> +       int len, ret = 0;
> +       const __be32 *prop;
> +       char prop_name[MAX_PROP_SIZE];
> +       struct sdhci_msm_reg_data *vreg;
> +       struct device_node *np = dev->of_node;
> +
> +       snprintf(prop_name, MAX_PROP_SIZE, "%s-supply", vreg_name);
> +       if (!of_parse_phandle(np, prop_name, 0)) {
> +               dev_warn(dev, "No internal vreg data found for %s\n",
> +                               vreg_name);
> +               ret = -EINVAL;
> +               return ret;
> +       }
> +       vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
> +       if (!vreg) {
> +               ret = -ENOMEM;
> +               return ret;
> +       }
> +       vreg->name = vreg_name;
> +       snprintf(prop_name, MAX_PROP_SIZE,
> +                       "qcom,%s-always-on", vreg_name);
> +       if (of_get_property(np, prop_name, NULL))
> +               vreg->is_always_on = true;
> +
> +       snprintf(prop_name, MAX_PROP_SIZE,
> +                       "qcom,%s-lpm-sup", vreg_name);
> +       if (of_get_property(np, prop_name, NULL))
> +               vreg->lpm_sup = true;
> +       snprintf(prop_name, MAX_PROP_SIZE,
> +                       "qcom,%s-voltage-level", vreg_name);
> +       prop = of_get_property(np, prop_name, &len);
> +       if (!prop || (len != (2 * sizeof(__be32)))) {
> +               dev_warn(dev, "%s %s property\n",
> +                               prop ? "invalid format" : "no", prop_name);
> +       } else {
> +               vreg->low_vol_level = be32_to_cpup(&prop[0]);
> +               vreg->high_vol_level = be32_to_cpup(&prop[1]);
> +       }
> +       snprintf(prop_name, MAX_PROP_SIZE,
> +                       "qcom,%s-current-level", vreg_name);
> +       prop = of_get_property(np, prop_name, &len);
> +       if (!prop || (len != (2 * sizeof(__be32)))) {
> +               dev_warn(dev, "%s %s property\n",
> +                               prop ? "invalid format" : "no", prop_name);
> +       } else {
> +               vreg->lpm_uA = be32_to_cpup(&prop[0]);
> +               vreg->hpm_uA = be32_to_cpup(&prop[1]);
> +       }
> +       *vreg_data = vreg;
> +       dev_dbg(dev, "%s: %s %s vol=[%d %d]uV, curr=[%d %d]uA\n",
> +                       vreg->name, vreg->is_always_on ? "always_on," : "",
> +                       vreg->lpm_sup ? "lpm_sup," : "", vreg->low_vol_level,
> +                       vreg->high_vol_level, vreg->lpm_uA, vreg->hpm_uA);

So again, I don't get why this isn't being implemented as a regular
regulator and why is this code in the mmc driver?

> +
> +       return ret;
> +}
> +
> +/* Regulator utility functions */
> +static int sdhci_msm_vreg_init_reg(struct device *dev,
> +                                       struct sdhci_msm_reg_data *vreg)
> +{
> +       int ret = 0;
> +
> +       /* check if regulator is already initialized? */
> +       if (vreg->reg)
> +               goto out;
> +
> +       /* Get the regulator handle */
> +       vreg->reg = devm_regulator_get(dev, vreg->name);
> +       if (IS_ERR(vreg->reg)) {
> +               ret = PTR_ERR(vreg->reg);
> +               pr_err("%s: devm_regulator_get(%s) failed. ret=%d\n",
> +                       __func__, vreg->name, ret);
> +               goto out;
> +       }
> +
> +       if (regulator_count_voltages(vreg->reg) > 0) {
> +               vreg->set_voltage_sup = true;
> +               /* sanity check */
> +               if (!vreg->high_vol_level || !vreg->hpm_uA) {
> +                       pr_err("%s: %s invalid constraints specified high_vol_level: %d hpm_uA: %d\n",
> +                              __func__, vreg->name, vreg->high_vol_level,
> +                              vreg->hpm_uA);
> +                       ret = -EINVAL;
> +               }
> +       }
> +
> +out:
> +       return ret;
> +}
> +
> +static void sdhci_msm_vreg_deinit_reg(struct sdhci_msm_reg_data *vreg)
> +{
> +       if (vreg->reg)
> +               devm_regulator_put(vreg->reg);
> +}
> +
> +static int sdhci_msm_vreg_set_optimum_mode(struct sdhci_msm_reg_data
> +                                                 *vreg, int uA_load)
> +{
> +       int ret = 0;
> +
> +       /*
> +        * regulators that do not support regulator_set_voltage also
> +        * do not support regulator_set_optimum_mode
> +        */

If that's the case, the regulator core should return proper error
codes and you should need to have these kind of checks here. Right?

Or at least there should be a helper function telling what you can do
with your regulator, instead of encoding it at in the mmc driver.

> +       if (vreg->set_voltage_sup) {
> +               ret = regulator_set_load(vreg->reg, uA_load);
> +               if (ret < 0)
> +                       pr_err("%s: regulator_set_load(reg=%s,uA_load=%d) failed. ret=%d\n",
> +                              __func__, vreg->name, uA_load, ret);
> +               else
> +                       /*
> +                        * regulator_set_load() can return non zero
> +                        * value even for success case.
> +                        */
> +                       ret = 0;
> +       }
> +       return ret;
> +}
> +
> +static int sdhci_msm_vreg_set_voltage(struct sdhci_msm_reg_data *vreg,
> +                                       int min_uV, int max_uV)
> +{
> +       int ret = 0;
> +
> +       if (vreg && vreg->set_voltage_sup) {

Ditto.

> +               ret = regulator_set_voltage(vreg->reg, min_uV, max_uV);
> +               if (ret) {
> +                       pr_err("%s: regulator_set_voltage(%s)failed. min_uV=%d,max_uV=%d,ret=%d\n",
> +                              __func__, vreg->name, min_uV, max_uV, ret);
> +               }
> +       }
> +
> +       return ret;
> +}
> +
> +static int sdhci_msm_vreg_enable(struct sdhci_msm_reg_data *vreg)
> +{
> +       int ret = 0;
> +
> +       /* Put regulator in HPM (high power mode) */
> +       ret = sdhci_msm_vreg_set_optimum_mode(vreg, vreg->hpm_uA);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (!vreg->is_enabled) {
> +               /* Set voltage level */
> +               ret = sdhci_msm_vreg_set_voltage(vreg, vreg->high_vol_level,
> +                                               vreg->high_vol_level);
> +               if (ret)
> +                       return ret;
> +       }
> +       ret = regulator_enable(vreg->reg);
> +       if (ret) {
> +               pr_err("%s: regulator_enable(%s) failed. ret=%d\n",
> +                               __func__, vreg->name, ret);
> +               return ret;
> +       }
> +       vreg->is_enabled = true;
> +       return ret;

Why don't you use the mmc_regulator_*() APIs? It seems like lots of
open coding here.

> +}
> +
> +static int sdhci_msm_vreg_disable(struct sdhci_msm_reg_data *vreg)
> +{
> +       int ret = 0;
> +
> +       /* Never disable regulator marked as always_on */
> +       if (vreg->is_enabled && !vreg->is_always_on) {
> +               ret = regulator_disable(vreg->reg);
> +               if (ret) {
> +                       pr_err("%s: regulator_disable(%s) failed. ret=%d\n",
> +                               __func__, vreg->name, ret);
> +                       goto out;
> +               }
> +               vreg->is_enabled = false;
> +
> +               ret = sdhci_msm_vreg_set_optimum_mode(vreg, 0);
> +               if (ret < 0)
> +                       goto out;
> +
> +               /* Set min. voltage level to 0 */
> +               ret = sdhci_msm_vreg_set_voltage(vreg, 0, vreg->high_vol_level);
> +               if (ret)
> +                       goto out;
> +       } else if (vreg->is_enabled && vreg->is_always_on) {
> +               if (vreg->lpm_sup) {
> +                       /* Put always_on regulator in LPM (low power mode) */
> +                       ret = sdhci_msm_vreg_set_optimum_mode(vreg,
> +                                                             vreg->lpm_uA);
> +                       if (ret < 0)
> +                               goto out;
> +               }
> +       }
> +out:
> +       return ret;
> +}

Ditto.

[...]

> +
> +/* Parse platform data */
> +static void sdhci_msm_populate_pdata(struct device *dev,
> +               struct sdhci_msm_pltfm_data *pdata)
> +{
> +       int ret = 0;
> +
> +       ret = sdhci_msm_dt_parse_vreg_info(dev, &pdata->vdd_data, "vdd");
> +       if (ret)
> +               dev_warn(dev, "failed parsing vdd data (%d)\n", ret);
> +
> +       ret = sdhci_msm_dt_parse_vreg_info(dev, &pdata->vdd_io_data, "vdd-io");
> +       if (ret)
> +               dev_warn(dev, "failed parsing vdd-io data (%d)\n", ret);
> +
> +}
> +
>  static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
>                                                     unsigned int clock)
>  {
> @@ -1326,6 +1685,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>         u32 pwr_state = 0, io_level = 0;
>         u32 config;
>         const struct sdhci_msm_offset *msm_offset = msm_host->offset;
> +       int ret = 0;
>
>         irq_status = sdhci_msm_vendor_readl_relaxed(host,
>                         msm_offset->core_pwrctl_status);
> @@ -1358,21 +1718,54 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>
>         /* Handle BUS ON/OFF*/
>         if (irq_status & CORE_PWRCTL_BUS_ON) {
> +               ret = sdhci_msm_setup_vreg(&msm_host->pdata, true, false);
> +               if (!ret) {
> +                       ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata,
> +                                       VDD_IO_HIGH, 0);
> +                       if (ret)
> +                               pr_err("%s: error setting vdd io in BUS_ON: %d\n",
> +                                               mmc_hostname(host->mmc), ret);
> +               } else {
> +                       pr_err("%s: error setting up vreg ON : %d\n",
> +                                       mmc_hostname(host->mmc), ret);
> +               }
>                 pwr_state = REQ_BUS_ON;
>                 io_level = REQ_IO_HIGH;
>                 irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>         }
>         if (irq_status & CORE_PWRCTL_BUS_OFF) {
> +               if (msm_host->pltfm_init_done)
> +                       ret = sdhci_msm_setup_vreg(&msm_host->pdata,
> +                                       false, false);
> +               if (!ret) {
> +                       ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata,
> +                                       VDD_IO_LOW, 0);
> +                       if (ret)
> +                               pr_err("%s: error setting vdd io in BUS_OFF: %d\n",
> +                                               mmc_hostname(host->mmc), ret);
> +               } else {
> +                       pr_err("%s: error setting up vreg OFF: %d\n",
> +                                       mmc_hostname(host->mmc), ret);
> +               }
>                 pwr_state = REQ_BUS_OFF;
>                 io_level = REQ_IO_LOW;
>                 irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>         }
>         /* Handle IO LOW/HIGH */
>         if (irq_status & CORE_PWRCTL_IO_LOW) {
> +               ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata, VDD_IO_LOW, 0);
> +               if (ret)
> +                       pr_err("%s: error setting up vdd io low: %d\n",
> +                                       mmc_hostname(host->mmc), ret);
>                 io_level = REQ_IO_LOW;
>                 irq_ack |= CORE_PWRCTL_IO_SUCCESS;
>         }
>         if (irq_status & CORE_PWRCTL_IO_HIGH) {
> +               ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata,
> +                               VDD_IO_HIGH, 0);
> +               if (ret)
> +                       pr_err("%s: error setting up vdd io high: %d\n",
> +                                       mmc_hostname(host->mmc), ret);
>                 io_level = REQ_IO_HIGH;
>                 irq_ack |= CORE_PWRCTL_IO_SUCCESS;
>         }
> @@ -1380,7 +1773,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>         /*
>          * The driver has to acknowledge the interrupt, switch voltages and
>          * report back if it succeded or not to this register. The voltage
> -        * switches are handled by the sdhci core, so just report success.
> +        * switches may be handled by the sdhci core, so just report success.
>          */
>         sdhci_msm_vendor_writel_relaxed(irq_ack, host,
>                         msm_offset->core_pwrctl_ctl);
> @@ -1612,6 +2005,74 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>         pr_debug("%s: supported caps: 0x%08x\n", mmc_hostname(mmc), caps);
>  }
>
> +static void sdhci_msm_set_default_hw_caps(struct device *dev,
> +               struct sdhci_msm_host *msm_host,
> +               struct sdhci_host *host)
> +{
> +       u32 version, config;
> +       u16 minor;
> +       u8 major;
> +       const struct sdhci_msm_offset *msm_offset = msm_host->offset;
> +       struct sdhci_msm_reg_data *vdd_io_reg = msm_host->pdata.vdd_io_data;
> +       u32 caps = 0;
> +
> +       version =  sdhci_msm_vendor_readl_relaxed(host,
> +                       msm_offset->core_mci_version);
> +       major = (version & CORE_VERSION_MAJOR_MASK) >>
> +               CORE_VERSION_MAJOR_SHIFT;
> +       minor = version & CORE_VERSION_MINOR_MASK;
> +       dev_dbg(dev, "MCI Version: 0x%08x, major: 0x%04x, minor: 0x%02x\n",
> +                       version, major, minor);
> +
> +       /*
> +        * If voltage regulators are controlled by msm host layer and
> +        * IO voltage regulator can support 1.8V, we need to
> +        * update the capabilities here before sdhci host is added.
> +        */
> +       if (vdd_io_reg && (vdd_io_reg->low_vol_level < 1950000))
> +               caps |= CORE_1_8V_SUPPORT;
> +       if (vdd_io_reg && (vdd_io_reg->low_vol_level > 2700000))
> +               caps |= CORE_3_0V_SUPPORT;
> +       if (caps) {
> +               msm_host->caps_0 |= caps;
> +               config =  readl_relaxed(host->ioaddr +
> +                               msm_offset->core_vendor_spec);
> +               config |= CORE_IO_PAD_PWR_SWITCH_EN;
> +               writel_relaxed(config,
> +                               host->ioaddr + msm_offset->core_vendor_spec);
> +       }
> +
> +       caps = readl_relaxed(host->ioaddr + SDHCI_CAPABILITIES);
> +
> +       if (major == 1 && minor >= 0x42)
> +               msm_host->use_14lpp_dll_reset = true;
> +       /*
> +        * SDCC 5 controller with major version 1, minor version 0x34 and later
> +        * with HS 400 mode support will use CM DLL instead of CDC LP 533 DLL.
> +        */
> +       if (major == 1 && minor < 0x34)
> +               msm_host->use_cdclp533 = true;
> +
> +       /*
> +        * Support for some capabilities is not advertised by newer
> +        * controller versions and must be explicitly enabled.
> +        */
> +       if (major >= 1 && minor != 0x11 && minor != 0x12) {
> +
> +               vdd_io_reg = msm_host->pdata.vdd_io_data;
> +               caps |= SDHCI_CAN_VDD_300 | SDHCI_CAN_DO_8BIT;
> +               if (msm_host->caps_0 & CORE_1_8V_SUPPORT)
> +                       caps |= CORE_1_8V_SUPPORT;
> +       }
> +       msm_host->caps_0 |= caps;
> +       writel_relaxed(msm_host->caps_0, host->ioaddr +
> +                       msm_offset->core_vendor_spec_capabilities0);
> +
> +       /* Set more host capabilities */
> +       msm_host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
> +       msm_host->mmc->caps2 |= MMC_CAP2_BOOTPART_NOACC;

Is really all above only related to regulators, as according to the
change log? Probably not.

Reaching this point in the review, it certainly seems like the change
needs to be split up in quite a few smaller pieces. Can you please do
that!?

[...]

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 1/4] mmc: host: Register changes for sdcc V5
  2018-05-02  8:28   ` Ulf Hansson
@ 2018-05-03  9:37     ` Vijay Viswanath
  0 siblings, 0 replies; 9+ messages in thread
From: Vijay Viswanath @ 2018-05-03  9:37 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Adrian Hunter, linux-mmc, Linux Kernel Mailing List, Shawn Lin,
	linux-arm-msm, Georgi Djakov, Asutosh Das, Sahitya Tummala,
	Venkat Gopalakrishnan, Jeremy McNicoll, Bjorn Andersson,
	Harjani Ritesh, vbadigan, Doug Anderson, sayalil



On 5/2/2018 1:58 PM, Ulf Hansson wrote:
> On 1 May 2018 at 12:39, Vijay Viswanath <vviswana@codeaurora.org> wrote:
>> From: Sayali Lokhande <sayalil@codeaurora.org>
>>
>> For SDCC version 5.0.0, MCI registers are removed from SDCC
>> interface and some registers are moved to HC. This change is
>> to support MCI register removal for msmfalcon. New compatible
>> string "qcom,sdhci-msm-v5" is added for msmfalcon to support
>> this change.
> 
> To simplify review, I would recommend to split this up in a few more
> pieces, a few re-factoring while the final change adds the
> qcom,sdhci-msm-v5 support.
> 

Will do

>>
>> Change-Id: I0febfd9bb2222436a8eff20c20107dd4180c9781
>> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   .../devicetree/bindings/mmc/sdhci-msm.txt          |   5 +-
> 
> Please move DT changes into separate patches and make sure to sent it
> to DT maintainers as well.
> 
>>   drivers/mmc/host/sdhci-msm.c                       | 485 +++++++++++++++------
>>   2 files changed, 365 insertions(+), 125 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> index bfdcdc4..c2b7b2b 100644
>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> @@ -4,7 +4,10 @@ This file documents differences between the core properties in mmc.txt
>>   and the properties used by the sdhci-msm driver.
>>
>>   Required properties:
>> -- compatible: Should contain "qcom,sdhci-msm-v4".
>> +- compatible: Should contain "qcom,sdhci-msm-v4" or "qcom,sdhci-msm-v5".
>> +                For SDCC version 5.0.0, MCI registers are removed from SDCC
>> +                interface and some registers are moved to HC. New compatible
>> +                string is added to support this change - "qcom,sdhci-msm-v5".
>>   - reg: Base address and length of the register in the following order:
>>          - Host controller register map (required)
>>          - SD Core register map (required)
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index bb11916..d4d432b 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
> 
> [...]
> 
>>
>> +struct sdhci_msm_offset {
> 
> I think a better idea is to create a struct sdhci_msm_variant and make
> it contain all the variant specific data. So, not only the offsets,
> but other variant data as well.
> 
> More comments below.
> 
>> +       u32 core_mci_data_cnt;
>> +       u32 core_mci_status;
>> +       u32 core_mci_fifo_cnt;
>> +       u32 core_mci_version;
>> +       u32 core_generics;
>> +       u32 core_testbus_config;
>> +       u32 core_testbus_sel2_bit;
>> +       u32 core_testbus_ena;
>> +       u32 core_testbus_sel2;
>> +       u32 core_pwrctl_status;
>> +       u32 core_pwrctl_mask;
>> +       u32 core_pwrctl_clear;
>> +       u32 core_pwrctl_ctl;
>> +       u32 core_sdcc_debug_reg;
>> +       u32 core_dll_config;
>> +       u32 core_dll_status;
>> +       u32 core_vendor_spec;
>> +       u32 core_vendor_spec_adma_err_addr0;
>> +       u32 core_vendor_spec_adma_err_addr1;
>> +       u32 core_vendor_spec_func2;
>> +       u32 core_vendor_spec_capabilities0;
>> +       u32 core_ddr_200_cfg;
>> +       u32 core_vendor_spec3;
>> +       u32 core_dll_config_2;
>> +       u32 core_ddr_config;
>> +       u32 core_ddr_config_2;
>> +};
>> +
>> +struct sdhci_msm_offset sdhci_msm_offset_mci_removed = {
>> +       .core_mci_data_cnt = 0x35c,
>> +       .core_mci_status = 0x324,
>> +       .core_mci_fifo_cnt = 0x308,
>> +       .core_mci_version = 0x318,
>> +       .core_generics = 0x320,
>> +       .core_testbus_config = 0x32c,
>> +       .core_testbus_sel2_bit = 3,
>> +       .core_testbus_ena = (1 << 31),
>> +       .core_testbus_sel2 = (1 << 3),
>> +       .core_pwrctl_status = 0x240,
>> +       .core_pwrctl_mask = 0x244,
>> +       .core_pwrctl_clear = 0x248,
>> +       .core_pwrctl_ctl = 0x24c,
>> +       .core_sdcc_debug_reg = 0x358,
>> +       .core_dll_config = 0x200,
>> +       .core_dll_status = 0x208,
>> +       .core_vendor_spec = 0x20c,
>> +       .core_vendor_spec_adma_err_addr0 = 0x214,
>> +       .core_vendor_spec_adma_err_addr1 = 0x218,
>> +       .core_vendor_spec_func2 = 0x210,
>> +       .core_vendor_spec_capabilities0 = 0x21c,
>> +       .core_ddr_200_cfg = 0x224,
>> +       .core_vendor_spec3 = 0x250,
>> +       .core_dll_config_2 = 0x254,
>> +       .core_ddr_config = 0x258,
>> +       .core_ddr_config_2 = 0x25c,
>> +};
>> +
>> +struct sdhci_msm_offset sdhci_msm_offset_mci_present = {
>> +       .core_mci_data_cnt = 0x30,
>> +       .core_mci_status = 0x34,
>> +       .core_mci_fifo_cnt = 0x44,
>> +       .core_mci_version = 0x050,
>> +       .core_generics = 0x70,
>> +       .core_testbus_config = 0x0CC,
>> +       .core_testbus_sel2_bit = 4,
>> +       .core_testbus_ena = (1 << 3),
>> +       .core_testbus_sel2 = (1 << 4),
>> +       .core_pwrctl_status = 0xDC,
>> +       .core_pwrctl_mask = 0xE0,
>> +       .core_pwrctl_clear = 0xE4,
>> +       .core_pwrctl_ctl = 0xE8,
>> +       .core_sdcc_debug_reg = 0x124,
>> +       .core_dll_config = 0x100,
>> +       .core_dll_status = 0x108,
>> +       .core_vendor_spec = 0x10C,
>> +       .core_vendor_spec_adma_err_addr0 = 0x114,
>> +       .core_vendor_spec_adma_err_addr1 = 0x118,
>> +       .core_vendor_spec_func2 = 0x110,
>> +       .core_vendor_spec_capabilities0 = 0x11C,
>> +       .core_ddr_200_cfg = 0x184,
>> +       .core_vendor_spec3 = 0x1B0,
>> +       .core_dll_config_2 = 0x1B4,
>> +       .core_ddr_config = 0x1B8,
>> +       .core_ddr_config_2 = 0x1BC,
>> +};
>> +
>>   struct sdhci_msm_host {
>>          struct platform_device *pdev;
>>          void __iomem *core_mem; /* MSM SDCC mapped address */
>> @@ -156,8 +232,72 @@ struct sdhci_msm_host {
>>          wait_queue_head_t pwr_irq_wait;
>>          bool pwr_irq_flag;
>>          u32 caps_0;
>> +       bool mci_removed;
>> +       const struct sdhci_msm_offset *offset;
>>   };
>>
>> +/*
>> + * APIs to write to vendor specific registers which were there in the core_mem
>> + * region before MCI was removed.
>> + */
>> +u8 sdhci_msm_vendor_readb_relaxed(struct sdhci_host *host, u32 offset)
>> +{
>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +       struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +       void __iomem *base_addr;
>> +
>> +       if (msm_host->mci_removed)
>> +               base_addr = host->ioaddr;
> 
> I would rather use a separate set of functions, which you assign at ->probe().
> 
> That may speed things up a little bit, as you don't need to check for
> "mci_removed", each time in these read/write functions.
> 
>> +       else
>> +               base_addr = msm_host->core_mem;
>> +
>> +       return readb_relaxed(base_addr + offset);
>> +}
> 
> [...]
> 
>>   static const struct of_device_id sdhci_msm_dt_match[] = {
>>          { .compatible = "qcom,sdhci-msm-v4" },
>> +       {.compatible = "qcom,sdhci-msm-v5"},
>>          {},
>>   };
>>
>> @@ -1430,6 +1651,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>          u16 host_version, core_minor;
>>          u32 core_version, config;
>>          u8 core_major;
>> +       const struct sdhci_msm_offset *msm_offset;
>>
>>          host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
>>          if (IS_ERR(host))
>> @@ -1445,6 +1667,15 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>          if (ret)
>>                  goto pltfm_free;
>>
>> +       if (of_find_compatible_node(NULL, NULL, "qcom,sdhci-msm-v5")) {
>> +               msm_host->mci_removed = true;
>> +               msm_host->offset = &sdhci_msm_offset_mci_removed;
> 
> I would suggest to extend your array of "struct of_device_id" to use
> the .data member to store all the variant specific data (such as the
> offsets for example).
> 
> Then you can use of_device_get_match_data(), to get the variant data.
> 
>> +       } else {
>> +               msm_host->mci_removed = false;
>> +               msm_host->offset = &sdhci_msm_offset_mci_present;
>> +       }
>> +       msm_offset = msm_host->offset;
>> +
> 
> [...]
> 
> Kind regards
> Uffe
> --
> 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
> 

I will explore the approach you suggested and will get back to you.

Thanks,
Vijay

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 2/4] mmc: sdhci-msm: Add and use voltage regulator related APIs
  2018-05-02  8:49   ` Ulf Hansson
@ 2018-05-03  9:39     ` Vijay Viswanath
  0 siblings, 0 replies; 9+ messages in thread
From: Vijay Viswanath @ 2018-05-03  9:39 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Adrian Hunter, linux-mmc, Linux Kernel Mailing List, Shawn Lin,
	linux-arm-msm, Georgi Djakov, Asutosh Das, Sahitya Tummala,
	Venkat Gopalakrishnan, Jeremy McNicoll, Bjorn Andersson,
	Harjani Ritesh, vbadigan, Doug Anderson, sayalil,
	Subhash Jadavani



On 5/2/2018 2:19 PM, Ulf Hansson wrote:
> On 1 May 2018 at 12:39, Vijay Viswanath <vviswana@codeaurora.org> wrote:
>> From: Asutosh Das <asutoshd@codeaurora.org>
>>
>> Some platforms require that the voltage switching happen only after
>> the register write occurs and controller is ready for the switch. When
>> the controller is ready, it will inform through power irq.
>>
>> Add voltage regulator APIs and use them during power irq to switch
>> voltage instead of relying on core layer voltage switching.
> 
> This is way to simplified. 529 lines of new code deserves some more
> explanations.
> 
>>
>> Change-Id: Iaa98686e71a5bfe0092c68e9ffa563b060c5ac60
> 
> Remove this.
> 
>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   .../devicetree/bindings/mmc/sdhci-msm.txt          |  27 +-
> 
> Split DT changes and add DT maintainers.
> 

Will do

>>   drivers/mmc/host/sdhci-msm.c                       | 537 +++++++++++++++++++--
>>   2 files changed, 529 insertions(+), 35 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> index c2b7b2b..c454046 100644
>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> @@ -23,6 +23,22 @@ Required properties:
>>          "xo"    - TCXO clock (optional)
>>          "cal"   - reference clock for RCLK delay calibration (optional)
>>          "sleep" - sleep clock for RCLK delay calibration (optional)
>> +- <supply-name>-supply: phandle to the regulator device tree node if voltage
>> +               regulators needs to be handled from within sdhci-msm layer.
>> +               Supported "supply-name" are "vdd" and "vdd-io".
>> +
>> +Optional Properties:
>> +       - qcom,<supply>-always-on - specifies whether supply should be kept
>> +               "on" always.
>> +       - qcom,<supply>-lpm_sup - specifies whether supply can be kept in low
>> +               power mode (lpm).
>> +       - qcom,<supply>-voltage_level - specifies voltage levels for supply.
>> +               Should be specified in pairs (min, max), units uV.
>> +       - qcom,<supply>-current_level - specifies load levels for supply in lpm
>> +               or high power mode (hpm). Should be specified in
>> +               pairs (lpm, hpm), units uA.
>> +
>> +
> 
> This looks really weird.
> 
> What's so special here that requires you to have your own specific
> regulator bindings?
> 
>>
>>   Example:
>>
>> @@ -33,8 +49,15 @@ Example:
>>                  bus-width = <8>;
>>                  non-removable;
>>
>> -               vmmc-supply = <&pm8941_l20>;
>> -               vqmmc-supply = <&pm8941_s3>;
>> +               vdd-supply = <&pm8941_l20>;
>> +               qcom,vdd-voltage-level = <2950000 2950000>;
>> +               qcom,vdd-current-level = <9000 800000>;
>> +
>> +               vdd-io-supply = <&pm8941_s3>;
>> +               qcom,vdd-io-always-on;
>> +               qcom,vdd-io-lpm-sup;
>> +               qcom,vdd-io-voltage-level = <1800000 2950000>;
>> +               qcom,vdd-io-current-level = <6 22000>;
>>
>>                  pinctrl-names = "default";
>>                  pinctrl-0 = <&sdc1_clk &sdc1_cmd &sdc1_data>;
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index d4d432b..0e0f12d 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -213,6 +213,33 @@ struct sdhci_msm_offset sdhci_msm_offset_mci_present = {
>>          .core_ddr_config_2 = 0x1BC,
>>   };
>>
>> +/* This structure keeps information per regulator */
>> +struct sdhci_msm_reg_data {
>> +       /* voltage regulator handle */
>> +       struct regulator *reg;
>> +       /* regulator name */
>> +       const char *name;
>> +       /* voltage level to be set */
>> +       u32 low_vol_level;
>> +       u32 high_vol_level;
>> +       /* Load values for low power and high power mode */
>> +       u32 lpm_uA;
>> +       u32 hpm_uA;
>> +       /* is this regulator enabled? */
>> +       bool is_enabled;
>> +       /* is this regulator needs to be always on? */
>> +       bool is_always_on;
>> +       /* is low power mode setting required for this regulator? */
>> +       bool lpm_sup;
>> +       bool set_voltage_sup;
>> +};
>> +
>> +struct sdhci_msm_pltfm_data {
>> +       /* Change-Id: Ide3a658ad51a3c3d4a05c47c0e8f013f647c9516 */
>> +       struct sdhci_msm_reg_data *vdd_data;
>> +       struct sdhci_msm_reg_data *vdd_io_data;
>> +};
>> +
>>   struct sdhci_msm_host {
>>          struct platform_device *pdev;
>>          void __iomem *core_mem; /* MSM SDCC mapped address */
>> @@ -234,6 +261,8 @@ struct sdhci_msm_host {
>>          u32 caps_0;
>>          bool mci_removed;
>>          const struct sdhci_msm_offset *offset;
>> +       bool pltfm_init_done;
>> +       struct sdhci_msm_pltfm_data pdata;
>>   };
>>
>>   /*
>> @@ -298,6 +327,336 @@ void sdhci_msm_vendor_writel_relaxed(u32 val, struct sdhci_host *host,
>>          writel_relaxed(val, base_addr + offset);
>>   }
>>
>> +enum vdd_io_level {
>> +       /* set vdd_io_data->low_vol_level */
>> +       VDD_IO_LOW,
>> +       /* set vdd_io_data->high_vol_level */
>> +       VDD_IO_HIGH,
>> +       /*
>> +        * set whatever there in voltage_level (third argument) of
>> +        * sdhci_msm_set_vdd_io_vol() function.
>> +        */
>> +       VDD_IO_SET_LEVEL,
>> +};
>> +
>> +#define MAX_PROP_SIZE 32
>> +static int sdhci_msm_dt_parse_vreg_info(struct device *dev,
>> +               struct sdhci_msm_reg_data **vreg_data, const char *vreg_name)
>> +{
>> +       int len, ret = 0;
>> +       const __be32 *prop;
>> +       char prop_name[MAX_PROP_SIZE];
>> +       struct sdhci_msm_reg_data *vreg;
>> +       struct device_node *np = dev->of_node;
>> +
>> +       snprintf(prop_name, MAX_PROP_SIZE, "%s-supply", vreg_name);
>> +       if (!of_parse_phandle(np, prop_name, 0)) {
>> +               dev_warn(dev, "No internal vreg data found for %s\n",
>> +                               vreg_name);
>> +               ret = -EINVAL;
>> +               return ret;
>> +       }
>> +       vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
>> +       if (!vreg) {
>> +               ret = -ENOMEM;
>> +               return ret;
>> +       }
>> +       vreg->name = vreg_name;
>> +       snprintf(prop_name, MAX_PROP_SIZE,
>> +                       "qcom,%s-always-on", vreg_name);
>> +       if (of_get_property(np, prop_name, NULL))
>> +               vreg->is_always_on = true;
>> +
>> +       snprintf(prop_name, MAX_PROP_SIZE,
>> +                       "qcom,%s-lpm-sup", vreg_name);
>> +       if (of_get_property(np, prop_name, NULL))
>> +               vreg->lpm_sup = true;
>> +       snprintf(prop_name, MAX_PROP_SIZE,
>> +                       "qcom,%s-voltage-level", vreg_name);
>> +       prop = of_get_property(np, prop_name, &len);
>> +       if (!prop || (len != (2 * sizeof(__be32)))) {
>> +               dev_warn(dev, "%s %s property\n",
>> +                               prop ? "invalid format" : "no", prop_name);
>> +       } else {
>> +               vreg->low_vol_level = be32_to_cpup(&prop[0]);
>> +               vreg->high_vol_level = be32_to_cpup(&prop[1]);
>> +       }
>> +       snprintf(prop_name, MAX_PROP_SIZE,
>> +                       "qcom,%s-current-level", vreg_name);
>> +       prop = of_get_property(np, prop_name, &len);
>> +       if (!prop || (len != (2 * sizeof(__be32)))) {
>> +               dev_warn(dev, "%s %s property\n",
>> +                               prop ? "invalid format" : "no", prop_name);
>> +       } else {
>> +               vreg->lpm_uA = be32_to_cpup(&prop[0]);
>> +               vreg->hpm_uA = be32_to_cpup(&prop[1]);
>> +       }
>> +       *vreg_data = vreg;
>> +       dev_dbg(dev, "%s: %s %s vol=[%d %d]uV, curr=[%d %d]uA\n",
>> +                       vreg->name, vreg->is_always_on ? "always_on," : "",
>> +                       vreg->lpm_sup ? "lpm_sup," : "", vreg->low_vol_level,
>> +                       vreg->high_vol_level, vreg->lpm_uA, vreg->hpm_uA);
> 
> So again, I don't get why this isn't being implemented as a regular
> regulator and why is this code in the mmc driver?
> 
>> +
>> +       return ret;
>> +}
>> +
>> +/* Regulator utility functions */
>> +static int sdhci_msm_vreg_init_reg(struct device *dev,
>> +                                       struct sdhci_msm_reg_data *vreg)
>> +{
>> +       int ret = 0;
>> +
>> +       /* check if regulator is already initialized? */
>> +       if (vreg->reg)
>> +               goto out;
>> +
>> +       /* Get the regulator handle */
>> +       vreg->reg = devm_regulator_get(dev, vreg->name);
>> +       if (IS_ERR(vreg->reg)) {
>> +               ret = PTR_ERR(vreg->reg);
>> +               pr_err("%s: devm_regulator_get(%s) failed. ret=%d\n",
>> +                       __func__, vreg->name, ret);
>> +               goto out;
>> +       }
>> +
>> +       if (regulator_count_voltages(vreg->reg) > 0) {
>> +               vreg->set_voltage_sup = true;
>> +               /* sanity check */
>> +               if (!vreg->high_vol_level || !vreg->hpm_uA) {
>> +                       pr_err("%s: %s invalid constraints specified high_vol_level: %d hpm_uA: %d\n",
>> +                              __func__, vreg->name, vreg->high_vol_level,
>> +                              vreg->hpm_uA);
>> +                       ret = -EINVAL;
>> +               }
>> +       }
>> +
>> +out:
>> +       return ret;
>> +}
>> +
>> +static void sdhci_msm_vreg_deinit_reg(struct sdhci_msm_reg_data *vreg)
>> +{
>> +       if (vreg->reg)
>> +               devm_regulator_put(vreg->reg);
>> +}
>> +
>> +static int sdhci_msm_vreg_set_optimum_mode(struct sdhci_msm_reg_data
>> +                                                 *vreg, int uA_load)
>> +{
>> +       int ret = 0;
>> +
>> +       /*
>> +        * regulators that do not support regulator_set_voltage also
>> +        * do not support regulator_set_optimum_mode
>> +        */
> 
> If that's the case, the regulator core should return proper error
> codes and you should need to have these kind of checks here. Right?
> 
> Or at least there should be a helper function telling what you can do
> with your regulator, instead of encoding it at in the mmc driver.
> 
>> +       if (vreg->set_voltage_sup) {
>> +               ret = regulator_set_load(vreg->reg, uA_load);
>> +               if (ret < 0)
>> +                       pr_err("%s: regulator_set_load(reg=%s,uA_load=%d) failed. ret=%d\n",
>> +                              __func__, vreg->name, uA_load, ret);
>> +               else
>> +                       /*
>> +                        * regulator_set_load() can return non zero
>> +                        * value even for success case.
>> +                        */
>> +                       ret = 0;
>> +       }
>> +       return ret;
>> +}
>> +
>> +static int sdhci_msm_vreg_set_voltage(struct sdhci_msm_reg_data *vreg,
>> +                                       int min_uV, int max_uV)
>> +{
>> +       int ret = 0;
>> +
>> +       if (vreg && vreg->set_voltage_sup) {
> 
> Ditto.
> 
>> +               ret = regulator_set_voltage(vreg->reg, min_uV, max_uV);
>> +               if (ret) {
>> +                       pr_err("%s: regulator_set_voltage(%s)failed. min_uV=%d,max_uV=%d,ret=%d\n",
>> +                              __func__, vreg->name, min_uV, max_uV, ret);
>> +               }
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static int sdhci_msm_vreg_enable(struct sdhci_msm_reg_data *vreg)
>> +{
>> +       int ret = 0;
>> +
>> +       /* Put regulator in HPM (high power mode) */
>> +       ret = sdhci_msm_vreg_set_optimum_mode(vreg, vreg->hpm_uA);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       if (!vreg->is_enabled) {
>> +               /* Set voltage level */
>> +               ret = sdhci_msm_vreg_set_voltage(vreg, vreg->high_vol_level,
>> +                                               vreg->high_vol_level);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +       ret = regulator_enable(vreg->reg);
>> +       if (ret) {
>> +               pr_err("%s: regulator_enable(%s) failed. ret=%d\n",
>> +                               __func__, vreg->name, ret);
>> +               return ret;
>> +       }
>> +       vreg->is_enabled = true;
>> +       return ret;
> 
> Why don't you use the mmc_regulator_*() APIs? It seems like lots of
> open coding here.
> 
>> +}
>> +
>> +static int sdhci_msm_vreg_disable(struct sdhci_msm_reg_data *vreg)
>> +{
>> +       int ret = 0;
>> +
>> +       /* Never disable regulator marked as always_on */
>> +       if (vreg->is_enabled && !vreg->is_always_on) {
>> +               ret = regulator_disable(vreg->reg);
>> +               if (ret) {
>> +                       pr_err("%s: regulator_disable(%s) failed. ret=%d\n",
>> +                               __func__, vreg->name, ret);
>> +                       goto out;
>> +               }
>> +               vreg->is_enabled = false;
>> +
>> +               ret = sdhci_msm_vreg_set_optimum_mode(vreg, 0);
>> +               if (ret < 0)
>> +                       goto out;
>> +
>> +               /* Set min. voltage level to 0 */
>> +               ret = sdhci_msm_vreg_set_voltage(vreg, 0, vreg->high_vol_level);
>> +               if (ret)
>> +                       goto out;
>> +       } else if (vreg->is_enabled && vreg->is_always_on) {
>> +               if (vreg->lpm_sup) {
>> +                       /* Put always_on regulator in LPM (low power mode) */
>> +                       ret = sdhci_msm_vreg_set_optimum_mode(vreg,
>> +                                                             vreg->lpm_uA);
>> +                       if (ret < 0)
>> +                               goto out;
>> +               }
>> +       }
>> +out:
>> +       return ret;
>> +}
> 
> Ditto.
> 
> [...]
> 
>> +
>> +/* Parse platform data */
>> +static void sdhci_msm_populate_pdata(struct device *dev,
>> +               struct sdhci_msm_pltfm_data *pdata)
>> +{
>> +       int ret = 0;
>> +
>> +       ret = sdhci_msm_dt_parse_vreg_info(dev, &pdata->vdd_data, "vdd");
>> +       if (ret)
>> +               dev_warn(dev, "failed parsing vdd data (%d)\n", ret);
>> +
>> +       ret = sdhci_msm_dt_parse_vreg_info(dev, &pdata->vdd_io_data, "vdd-io");
>> +       if (ret)
>> +               dev_warn(dev, "failed parsing vdd-io data (%d)\n", ret);
>> +
>> +}
>> +
>>   static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
>>                                                      unsigned int clock)
>>   {
>> @@ -1326,6 +1685,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>>          u32 pwr_state = 0, io_level = 0;
>>          u32 config;
>>          const struct sdhci_msm_offset *msm_offset = msm_host->offset;
>> +       int ret = 0;
>>
>>          irq_status = sdhci_msm_vendor_readl_relaxed(host,
>>                          msm_offset->core_pwrctl_status);
>> @@ -1358,21 +1718,54 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>>
>>          /* Handle BUS ON/OFF*/
>>          if (irq_status & CORE_PWRCTL_BUS_ON) {
>> +               ret = sdhci_msm_setup_vreg(&msm_host->pdata, true, false);
>> +               if (!ret) {
>> +                       ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata,
>> +                                       VDD_IO_HIGH, 0);
>> +                       if (ret)
>> +                               pr_err("%s: error setting vdd io in BUS_ON: %d\n",
>> +                                               mmc_hostname(host->mmc), ret);
>> +               } else {
>> +                       pr_err("%s: error setting up vreg ON : %d\n",
>> +                                       mmc_hostname(host->mmc), ret);
>> +               }
>>                  pwr_state = REQ_BUS_ON;
>>                  io_level = REQ_IO_HIGH;
>>                  irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>>          }
>>          if (irq_status & CORE_PWRCTL_BUS_OFF) {
>> +               if (msm_host->pltfm_init_done)
>> +                       ret = sdhci_msm_setup_vreg(&msm_host->pdata,
>> +                                       false, false);
>> +               if (!ret) {
>> +                       ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata,
>> +                                       VDD_IO_LOW, 0);
>> +                       if (ret)
>> +                               pr_err("%s: error setting vdd io in BUS_OFF: %d\n",
>> +                                               mmc_hostname(host->mmc), ret);
>> +               } else {
>> +                       pr_err("%s: error setting up vreg OFF: %d\n",
>> +                                       mmc_hostname(host->mmc), ret);
>> +               }
>>                  pwr_state = REQ_BUS_OFF;
>>                  io_level = REQ_IO_LOW;
>>                  irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>>          }
>>          /* Handle IO LOW/HIGH */
>>          if (irq_status & CORE_PWRCTL_IO_LOW) {
>> +               ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata, VDD_IO_LOW, 0);
>> +               if (ret)
>> +                       pr_err("%s: error setting up vdd io low: %d\n",
>> +                                       mmc_hostname(host->mmc), ret);
>>                  io_level = REQ_IO_LOW;
>>                  irq_ack |= CORE_PWRCTL_IO_SUCCESS;
>>          }
>>          if (irq_status & CORE_PWRCTL_IO_HIGH) {
>> +               ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata,
>> +                               VDD_IO_HIGH, 0);
>> +               if (ret)
>> +                       pr_err("%s: error setting up vdd io high: %d\n",
>> +                                       mmc_hostname(host->mmc), ret);
>>                  io_level = REQ_IO_HIGH;
>>                  irq_ack |= CORE_PWRCTL_IO_SUCCESS;
>>          }
>> @@ -1380,7 +1773,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>>          /*
>>           * The driver has to acknowledge the interrupt, switch voltages and
>>           * report back if it succeded or not to this register. The voltage
>> -        * switches are handled by the sdhci core, so just report success.
>> +        * switches may be handled by the sdhci core, so just report success.
>>           */
>>          sdhci_msm_vendor_writel_relaxed(irq_ack, host,
>>                          msm_offset->core_pwrctl_ctl);
>> @@ -1612,6 +2005,74 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>>          pr_debug("%s: supported caps: 0x%08x\n", mmc_hostname(mmc), caps);
>>   }
>>
>> +static void sdhci_msm_set_default_hw_caps(struct device *dev,
>> +               struct sdhci_msm_host *msm_host,
>> +               struct sdhci_host *host)
>> +{
>> +       u32 version, config;
>> +       u16 minor;
>> +       u8 major;
>> +       const struct sdhci_msm_offset *msm_offset = msm_host->offset;
>> +       struct sdhci_msm_reg_data *vdd_io_reg = msm_host->pdata.vdd_io_data;
>> +       u32 caps = 0;
>> +
>> +       version =  sdhci_msm_vendor_readl_relaxed(host,
>> +                       msm_offset->core_mci_version);
>> +       major = (version & CORE_VERSION_MAJOR_MASK) >>
>> +               CORE_VERSION_MAJOR_SHIFT;
>> +       minor = version & CORE_VERSION_MINOR_MASK;
>> +       dev_dbg(dev, "MCI Version: 0x%08x, major: 0x%04x, minor: 0x%02x\n",
>> +                       version, major, minor);
>> +
>> +       /*
>> +        * If voltage regulators are controlled by msm host layer and
>> +        * IO voltage regulator can support 1.8V, we need to
>> +        * update the capabilities here before sdhci host is added.
>> +        */
>> +       if (vdd_io_reg && (vdd_io_reg->low_vol_level < 1950000))
>> +               caps |= CORE_1_8V_SUPPORT;
>> +       if (vdd_io_reg && (vdd_io_reg->low_vol_level > 2700000))
>> +               caps |= CORE_3_0V_SUPPORT;
>> +       if (caps) {
>> +               msm_host->caps_0 |= caps;
>> +               config =  readl_relaxed(host->ioaddr +
>> +                               msm_offset->core_vendor_spec);
>> +               config |= CORE_IO_PAD_PWR_SWITCH_EN;
>> +               writel_relaxed(config,
>> +                               host->ioaddr + msm_offset->core_vendor_spec);
>> +       }
>> +
>> +       caps = readl_relaxed(host->ioaddr + SDHCI_CAPABILITIES);
>> +
>> +       if (major == 1 && minor >= 0x42)
>> +               msm_host->use_14lpp_dll_reset = true;
>> +       /*
>> +        * SDCC 5 controller with major version 1, minor version 0x34 and later
>> +        * with HS 400 mode support will use CM DLL instead of CDC LP 533 DLL.
>> +        */
>> +       if (major == 1 && minor < 0x34)
>> +               msm_host->use_cdclp533 = true;
>> +
>> +       /*
>> +        * Support for some capabilities is not advertised by newer
>> +        * controller versions and must be explicitly enabled.
>> +        */
>> +       if (major >= 1 && minor != 0x11 && minor != 0x12) {
>> +
>> +               vdd_io_reg = msm_host->pdata.vdd_io_data;
>> +               caps |= SDHCI_CAN_VDD_300 | SDHCI_CAN_DO_8BIT;
>> +               if (msm_host->caps_0 & CORE_1_8V_SUPPORT)
>> +                       caps |= CORE_1_8V_SUPPORT;
>> +       }
>> +       msm_host->caps_0 |= caps;
>> +       writel_relaxed(msm_host->caps_0, host->ioaddr +
>> +                       msm_offset->core_vendor_spec_capabilities0);
>> +
>> +       /* Set more host capabilities */
>> +       msm_host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
>> +       msm_host->mmc->caps2 |= MMC_CAP2_BOOTPART_NOACC;
> 
> Is really all above only related to regulators, as according to the
> change log? Probably not.
> 
> Reaching this point in the review, it certainly seems like the change
> needs to be split up in quite a few smaller pieces. Can you please do
> that!?
> 
> [...]
> 
> Kind regards
> Uffe
> 

I am checking this. Will get back to you.

Thanks,
Vijay

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-05-03  9:39 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-01 10:39 [PATCH RFC 0/4] Changes for SDCC5 version Vijay Viswanath
2018-05-01 10:39 ` [PATCH RFC 1/4] mmc: host: Register changes for sdcc V5 Vijay Viswanath
2018-05-02  8:28   ` Ulf Hansson
2018-05-03  9:37     ` Vijay Viswanath
2018-05-01 10:39 ` [PATCH RFC 2/4] mmc: sdhci-msm: Add and use voltage regulator related APIs Vijay Viswanath
2018-05-02  8:49   ` Ulf Hansson
2018-05-03  9:39     ` Vijay Viswanath
2018-05-01 10:39 ` [PATCH RFC 3/4] host: sdhci: fix current caps when there is no host->vmmc Vijay Viswanath
2018-05-01 10:39 ` [PATCH RFC 4/4] host: sdhci-msm: implement get_current_limit() host op Vijay Viswanath

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).