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

Hi Vijay. Thanks for this patch.

On Thu, May 17, 2018 at 3:30 AM Vijay Viswanath [off-list ref]
wrote:

From: Sayali Lokhande <redacted>

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 <redacted>
Signed-off-by: Vijay Viswanath <redacted>
---
  .../devicetree/bindings/mmc/sdhci-msm.txt          |   5 +-
  drivers/mmc/host/sdhci-msm.c                       | 344

+++++++++++++--------

  2 files changed, 222 insertions(+), 127 deletions(-)

b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt

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".

-#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_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_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

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);

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);
         }

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);

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;

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;

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);

  }

         spin_lock_irqsave(&host->lock, flags);

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),

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.

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);

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.

*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