[V1,3/3] mmc: host: Register changes for sdcc V5

Message ID 1526552938-21292-4-git-send-email-vviswana@codeaurora.org
State Changes Requested
Headers show
Series
  • Changes for SDCC5 version
Related show

Commit Message

Vijay Viswanath May 17, 2018, 10:28 a.m.
From: Sayali Lokhande <sayalil@codeaurora.org>

For SDCC version 5.0.0 and higher, new compatible string
"qcom,sdhci-msm-v5" is added.

Based on the msm variant, pick the relevant variant data and
use it for register read/write to msm specific registers.

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                       | 344 +++++++++++++--------
 2 files changed, 222 insertions(+), 127 deletions(-)

Comments

Evan Green May 22, 2018, 6:12 p.m. | #1
Hi Vijay. Thanks for this patch.

On Thu, May 17, 2018 at 3:30 AM Vijay Viswanath <vviswana@codeaurora.org>
wrote:

> From: Sayali Lokhande <sayalil@codeaurora.org>

> For SDCC version 5.0.0 and higher, new compatible string
> "qcom,sdhci-msm-v5" is added.

> Based on the msm variant, pick the relevant variant data and
> use it for register read/write to msm specific registers.

> 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                       | 344
+++++++++++++--------
>   2 files changed, 222 insertions(+), 127 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 bb2bb59..408e6b2 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -33,16 +33,11 @@
>   #define CORE_MCI_GENERICS              0x70
>   #define SWITCHABLE_SIGNALING_VOLTAGE   BIT(29)

> -#define CORE_HC_MODE           0x78

Remove CORE_MCI_VERSION as well.

>   #define HC_MODE_EN             0x1
>   #define CORE_POWER             0x0
>   #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 +58,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 +102,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
> @@ -380,10 +368,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;

I notice this pattern is pasted all over the place in order to get to the
offsets. Maybe a macro or inlined function would be cleaner to get you to
directly to the sdhci_msm_offset struct from sdhci_host, rather than this
blob of paste soup everywhere. In some places you do seem to use the
intermediate locals, so those cases wouldn't need to use the new helper.


>          /* 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) {
> @@ -393,8 +385,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;
> @@ -410,16 +402,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);
> @@ -430,24 +426,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);

Nit: host->ioaddr + msm_offset->core_dll_config might benefit from having
its own local, since you use it so much in this function. Same goes for
where I've noted below...

>          goto out;

>   err_out:
> @@ -573,6 +569,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)
> @@ -592,10 +592,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) */
> @@ -607,6 +607,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);

> @@ -615,34 +617,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),
> @@ -651,40 +662,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);


...here. A local for host->ioaddr + msm_offset->core_dll_config would save
you a lot of split lines.

> @@ -1272,12 +1327,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),
> +                msm_host->var_ops->msm_readl_relaxed(host,

There's a weird extra space here.

> +                       msm_offset->core_pwrctl_status),
> +               msm_host->var_ops->msm_readl_relaxed(host,
> +                       msm_offset->core_pwrctl_mask),
> +               msm_host->var_ops->msm_readl_relaxed(host,
> +                       msm_offset->core_pwrctl_ctl));

I think the idea of function pointers is fine, but overall the use of them
everywhere sure is ugly. It makes it really hard to actually see what's
happening. I wonder if things might look a lot cleaner with a helper
function here. Then instead of:

msm_host->var_ops->msm_readl_relaxed(host, msm_offset->core_pwrctl_ctl);

You could have

msm_core_read(host, msm_offset->core_pwrctl_ctl);

> @@ -1553,7 +1619,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);

Remove the second space before the readl_relaxed.

> @@ -1710,32 +1793,40 @@ 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 */
> +               msm_host->var_ops->msm_writel_relaxed(HC_MODE_EN, host,
> +                               msm_offset->core_hc_mode);
> +               config = msm_host->var_ops->msm_readl_relaxed(host,
> +                               msm_offset->core_hc_mode);
> +               config |= FF_CLK_SW_RST_DIS;
> +               msm_host->var_ops->msm_writel_relaxed(config, host,
> +                               msm_offset->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 =  msm_host->var_ops->msm_readl_relaxed(host,
> +               msm_offset->core_mci_version);

Another double space after the =. Perhaps this was a find/replace error?
Look out for more of these that I missed.

-Evan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring May 22, 2018, 7:45 p.m. | #2
On Thu, May 17, 2018 at 03:58:58PM +0530, Vijay Viswanath wrote:
> From: Sayali Lokhande <sayalil@codeaurora.org>
> 
> For SDCC version 5.0.0 and higher, new compatible string
> "qcom,sdhci-msm-v5" is added.
> 
> Based on the msm variant, pick the relevant variant data and
> use it for register read/write to msm specific registers.
> 
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  .../devicetree/bindings/mmc/sdhci-msm.txt          |   5 +-

Please split binding patches.

>  drivers/mmc/host/sdhci-msm.c                       | 344 +++++++++++++--------
>  2 files changed, 222 insertions(+), 127 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".

Format with 1 per line.

> +		 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)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vijay Viswanath May 24, 2018, 1 p.m. | #3
On 5/22/2018 11:42 PM, Evan Green wrote:
> Hi Vijay. Thanks for this patch.
> 
> On Thu, May 17, 2018 at 3:30 AM Vijay Viswanath <vviswana@codeaurora.org>
> wrote:
> 
>> From: Sayali Lokhande <sayalil@codeaurora.org>
> 
>> For SDCC version 5.0.0 and higher, new compatible string
>> "qcom,sdhci-msm-v5" is added.
> 
>> Based on the msm variant, pick the relevant variant data and
>> use it for register read/write to msm specific registers.
> 
>> 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                       | 344
> +++++++++++++--------
>>    2 files changed, 222 insertions(+), 127 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 bb2bb59..408e6b2 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -33,16 +33,11 @@
>>    #define CORE_MCI_GENERICS              0x70
>>    #define SWITCHABLE_SIGNALING_VOLTAGE   BIT(29)
> 
>> -#define CORE_HC_MODE           0x78
> 
> Remove CORE_MCI_VERSION as well.
> 

Missed it. Will remove

>>    #define HC_MODE_EN             0x1
>>    #define CORE_POWER             0x0
>>    #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 +58,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 +102,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
>> @@ -380,10 +368,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;
> 
> I notice this pattern is pasted all over the place in order to get to the
> offsets. Maybe a macro or inlined function would be cleaner to get you to
> directly to the sdhci_msm_offset struct from sdhci_host, rather than this
> blob of paste soup everywhere. In some places you do seem to use the
> intermediate locals, so those cases wouldn't need to use the new helper.
> 
> 
>>           /* 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) {
>> @@ -393,8 +385,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;
>> @@ -410,16 +402,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);
>> @@ -430,24 +426,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);
> 
> Nit: host->ioaddr + msm_offset->core_dll_config might benefit from having
> its own local, since you use it so much in this function. Same goes for
> where I've noted below...
> 

core_dll_config is very much used. But having a local for it feels like 
a bad idea. As different versions come up, the most used register may 
change. So it would be better to stick to a consistent approach to 
accessing every register.

>>           goto out;
> 
>>    err_out:
>> @@ -573,6 +569,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)
>> @@ -592,10 +592,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) */
>> @@ -607,6 +607,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);
> 
>> @@ -615,34 +617,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),
>> @@ -651,40 +662,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);
> 
> 
> ...here. A local for host->ioaddr + msm_offset->core_dll_config would save
> you a lot of split lines.
> 
>> @@ -1272,12 +1327,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),
>> +                msm_host->var_ops->msm_readl_relaxed(host,
> 
> There's a weird extra space here.
> 
>> +                       msm_offset->core_pwrctl_status),
>> +               msm_host->var_ops->msm_readl_relaxed(host,
>> +                       msm_offset->core_pwrctl_mask),
>> +               msm_host->var_ops->msm_readl_relaxed(host,
>> +                       msm_offset->core_pwrctl_ctl));
> 
> I think the idea of function pointers is fine, but overall the use of them
> everywhere sure is ugly. It makes it really hard to actually see what's
> happening. I wonder if things might look a lot cleaner with a helper
> function here. Then instead of:
> 
> msm_host->var_ops->msm_readl_relaxed(host, msm_offset->core_pwrctl_ctl);
> 
> You could have
> 
> msm_core_read(host, msm_offset->core_pwrctl_ctl);
> 

if we use a helper function, then we will have to pass msm_host into it 
as well. Otherwise there would be the hassle of deriving msm_host 
address from sdhci_host.

How about using a MACRO here instead for readability ?

>> @@ -1553,7 +1619,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);
> 
> Remove the second space before the readl_relaxed.
> 
>> @@ -1710,32 +1793,40 @@ 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 */
>> +               msm_host->var_ops->msm_writel_relaxed(HC_MODE_EN, host,
>> +                               msm_offset->core_hc_mode);
>> +               config = msm_host->var_ops->msm_readl_relaxed(host,
>> +                               msm_offset->core_hc_mode);
>> +               config |= FF_CLK_SW_RST_DIS;
>> +               msm_host->var_ops->msm_writel_relaxed(config, host,
>> +                               msm_offset->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 =  msm_host->var_ops->msm_readl_relaxed(host,
>> +               msm_offset->core_mci_version);
> 
> Another double space after the =. Perhaps this was a find/replace error?
> Look out for more of these that I missed.
> 
> -Evan
> 

Thanks for pointing these out. Will check for such issues in all 3 patches.

Thanks,
Vijay
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Evan Green May 25, 2018, 8:46 p.m. | #4
On Thu, May 24, 2018 at 6:01 AM Vijay Viswanath <vviswana@codeaurora.org>
wrote:



> On 5/22/2018 11:42 PM, Evan Green wrote:
> > Hi Vijay. Thanks for this patch.
> >
> > On Thu, May 17, 2018 at 3:30 AM Vijay Viswanath <vviswana@codeaurora.org

> > wrote:
> >
> >> From: Sayali Lokhande <sayalil@codeaurora.org>
> >
...
> > Nit: host->ioaddr + msm_offset->core_dll_config might benefit from
having
> > its own local, since you use it so much in this function. Same goes for
> > where I've noted below...
> >

> core_dll_config is very much used. But having a local for it feels like
> a bad idea. As different versions come up, the most used register may
> change. So it would be better to stick to a consistent approach to
> accessing every register.


I generally optimize for readability, rather than find/replace-ability. In
my opinion, it's distracting to see that expression copy/pasted so many
times in the same function. But ultimately this is a style preference, so
if you decide not to do it, I'll live.

> >
> >> +                       msm_offset->core_pwrctl_status),
> >> +               msm_host->var_ops->msm_readl_relaxed(host,
> >> +                       msm_offset->core_pwrctl_mask),
> >> +               msm_host->var_ops->msm_readl_relaxed(host,
> >> +                       msm_offset->core_pwrctl_ctl));
> >
> > I think the idea of function pointers is fine, but overall the use of
them
> > everywhere sure is ugly. It makes it really hard to actually see what's
> > happening. I wonder if things might look a lot cleaner with a helper
> > function here. Then instead of:
> >
> > msm_host->var_ops->msm_readl_relaxed(host, msm_offset->core_pwrctl_ctl);
> >
> > You could have
> >
> > msm_core_read(host, msm_offset->core_pwrctl_ctl);
> >

> if we use a helper function, then we will have to pass msm_host into it
> as well. Otherwise there would be the hassle of deriving msm_host
> address from sdhci_host.

> How about using a MACRO here instead for readability ?


The deriving part in the helper would likely get inlined and shared by the
compiler among all call-sites within a function. But yes, a macro would
work too.

Thanks Vijay,
Evan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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 bb2bb59..408e6b2 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -33,16 +33,11 @@ 
 #define CORE_MCI_GENERICS		0x70
 #define SWITCHABLE_SIGNALING_VOLTAGE	BIT(29)
 
-#define CORE_HC_MODE		0x78
 #define HC_MODE_EN		0x1
 #define CORE_POWER		0x0
 #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 +58,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 +102,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
@@ -380,10 +368,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) {
@@ -393,8 +385,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;
@@ -410,16 +402,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);
@@ -430,24 +426,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:
@@ -573,6 +569,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)
@@ -592,10 +592,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) */
@@ -607,6 +607,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);
 
@@ -615,34 +617,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),
@@ -651,40 +662,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) {
@@ -705,19 +728,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
@@ -726,10 +751,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
@@ -745,32 +770,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 |
@@ -822,6 +851,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__);
 
@@ -838,13 +869,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;
@@ -854,9 +885,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 */
 
@@ -908,9 +939,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);
@@ -922,32 +953,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",
@@ -955,9 +994,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
@@ -977,6 +1016,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__);
 
@@ -994,9 +1035,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)
@@ -1126,6 +1169,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 */
@@ -1166,13 +1211,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
@@ -1214,7 +1263,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,
@@ -1223,8 +1274,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 =  msm_host->var_ops->msm_readl_relaxed(host,
+					msm_offset->core_generics);
 	if ((req_type & REQ_IO_HIGH || req_type & REQ_IO_LOW) &&
 	    !(val & SWITCHABLE_SIGNALING_VOLTAGE)) {
 		return;
@@ -1272,12 +1327,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),
+		 msm_host->var_ops->msm_readl_relaxed(host,
+			msm_offset->core_pwrctl_status),
+		msm_host->var_ops->msm_readl_relaxed(host,
+			msm_offset->core_pwrctl_mask),
+		msm_host->var_ops->msm_readl_relaxed(host,
+			msm_offset->core_pwrctl_ctl));
 }
 
 static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
@@ -1288,11 +1348,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 = msm_host->var_ops->msm_readl_relaxed(host,
+			msm_offset->core_pwrctl_status);
 	irq_status &= INT_MASK;
 
-	writel_relaxed(irq_status, msm_host->core_mem + CORE_PWRCTL_CLEAR);
+	msm_host->var_ops->msm_writel_relaxed(irq_status, host,
+			msm_offset->core_pwrctl_clear);
 
 	/*
 	 * There is a rare HW scenario where the first clear pulse could be
@@ -1301,8 +1364,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 & msm_host->var_ops->msm_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);
@@ -1310,8 +1373,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);
+		msm_host->var_ops->msm_writel_relaxed(irq_status, host,
+			msm_offset->core_pwrctl_clear);
 		retry--;
 		udelay(10);
 	}
@@ -1342,7 +1405,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);
+	msm_host->var_ops->msm_writel_relaxed(irq_ack, host,
+			msm_offset->core_pwrctl_ctl);
 
 	/*
 	 * If we don't have info regarding the voltage levels supported by
@@ -1361,7 +1425,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) &&
@@ -1372,8 +1437,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)
@@ -1534,6 +1599,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))
@@ -1553,7 +1619,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))
@@ -1561,7 +1628,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);
@@ -1594,7 +1662,8 @@  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-v4", .data = &sdhci_msm_mci_var},
+	{.compatible = "qcom,sdhci-msm-v5", .data = &sdhci_msm_v5_var},
 	{},
 };
 
@@ -1631,6 +1700,8 @@  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;
+	const struct sdhci_msm_variant_info *var_info;
 
 	host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
 	if (IS_ERR(host))
@@ -1646,6 +1717,18 @@  static int sdhci_msm_probe(struct platform_device *pdev)
 	if (ret)
 		goto pltfm_free;
 
+	/*
+	 * Based on the compatible string, load the required msm host info from
+	 * the data associated with the version info.
+	 */
+	var_info = of_device_get_match_data(&pdev->dev);
+
+	msm_host->mci_removed = var_info->mci_removed;
+	msm_host->var_ops = var_info->var_ops;
+	msm_host->offset = var_info->offset;
+
+	msm_offset = msm_host->offset;
+
 	sdhci_get_of_property(pdev);
 
 	msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
@@ -1710,32 +1793,40 @@  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 */
+		msm_host->var_ops->msm_writel_relaxed(HC_MODE_EN, host,
+				msm_offset->core_hc_mode);
+		config = msm_host->var_ops->msm_readl_relaxed(host,
+				msm_offset->core_hc_mode);
+		config |= FF_CLK_SW_RST_DIS;
+		msm_host->var_ops->msm_writel_relaxed(config, host,
+				msm_offset->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 =  msm_host->var_ops->msm_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;
@@ -1760,7 +1851,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);
 	}
 
 	/*
@@ -1789,7 +1880,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);
+	msm_host->var_ops->msm_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,