With the current DLL sequence stability issues for data
transfer seen in HS400 and HS200 modes.
"mmc0: cqhci: error IRQ status: 0x00000000 cmd error -84
data error 0"
Rectify the DLL programming sequence as per latest hardware
programming guide and also incorporate support for HS200 and
HS400 DLL settings using the device tree.
Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
Signed-off-by: Jun Li <liju@codeaurora.org>
---
drivers/mmc/host/sdhci-msm.c | 372 +++++++++++++++++++++++++++++++++--
1 file changed, 353 insertions(+), 19 deletions(-)
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 2a5e588779fc..4ecb362f7f2a 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -28,6 +28,7 @@
#define CORE_VERSION_MAJOR_SHIFT 28
#define CORE_VERSION_MAJOR_MASK (0xf << CORE_VERSION_MAJOR_SHIFT)
#define CORE_VERSION_MINOR_MASK 0xff
+#define SDHCI_MSM_MIN_V_7FF 0x6e
#define CORE_MCI_GENERICS 0x70
#define SWITCHABLE_SIGNALING_VOLTAGE BIT(29)
@@ -118,7 +119,8 @@
#define CORE_PWRSAVE_DLL BIT(3)
#define DDR_CONFIG_POR_VAL 0x80040873
-
+#define DLL_CONFIG_3_POR_VAL 0x10
+#define TCXO_FREQ 19200000
#define INVALID_TUNING_PHASE -1
#define SDHCI_MSM_MIN_CLOCK 400000
@@ -256,6 +258,19 @@ struct sdhci_msm_variant_info {
const struct sdhci_msm_offset *offset;
};
+/*
+ * DLL registers which needs be programmed with HSR settings.
+ * Add any new register only at the end and don't change the
+ * sequence.
+ */
+struct sdhci_msm_dll {
+ u32 dll_config[2];
+ u32 dll_config_2[2];
+ u32 dll_config_3[2];
+ u32 dll_usr_ctl[2];
+ u32 ddr_config[2];
+};
+
struct sdhci_msm_host {
struct platform_device *pdev;
void __iomem *core_mem; /* MSM SDCC mapped address */
@@ -264,6 +279,7 @@ struct sdhci_msm_host {
struct clk *xo_clk; /* TCXO clk needed for FLL feature of cm_dll*/
/* core, iface, cal and sleep clocks */
struct clk_bulk_data bulk_clks[4];
+ struct sdhci_msm_dll dll;
#ifdef CONFIG_MMC_CRYPTO
struct qcom_ice *ice;
#endif
@@ -292,6 +308,17 @@ struct sdhci_msm_host {
u32 dll_config;
u32 ddr_config;
bool vqmmc_enabled;
+ bool artanis_dll;
+};
+
+enum dll_init_context {
+ DLL_INIT_NORMAL,
+ DLL_INIT_FROM_CX_COLLAPSE_EXIT,
+};
+
+enum mode {
+ HS400, // equivalent to SDR104 mode for DLL.
+ HS200, // equivalent to SDR50 mode for DLL.
};
static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
@@ -778,6 +805,210 @@ static int msm_init_cm_dll(struct sdhci_host *host)
return 0;
}
+static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
+{
+ return SDHCI_MSM_MIN_CLOCK;
+}
+
+static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
+{
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+ struct clk *core_clk = msm_host->bulk_clks[0].clk;
+ unsigned int sup_clk;
+
+ if (req_clk < sdhci_msm_get_min_clock(host))
+ return sdhci_msm_get_min_clock(host);
+
+ sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
+
+ if (host->clock != msm_host->clk_rate)
+ sup_clk = sup_clk / 2;
+
+ return sup_clk;
+}
+
+/* Initialize the DLL (Programmable Delay Line) */
+static int sdhci_msm_configure_dll(struct sdhci_host *host, enum dll_init_context
+ init_context, enum mode index)
+{
+ 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;
+ struct mmc_host *mmc = host->mmc;
+ u32 ddr_cfg_offset, core_vendor_spec, config;
+ void __iomem *ioaddr = host->ioaddr;
+ unsigned long flags, dll_clock;
+ int rc = 0, wait_cnt = 50;
+
+ dll_clock = sdhci_msm_get_clk_rate(host, host->clock);
+ spin_lock_irqsave(&host->lock, flags);
+
+ core_vendor_spec = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
+
+ /*
+ * Always disable PWRSAVE during the DLL power
+ * up regardless of its current setting.
+ */
+ core_vendor_spec &= ~CORE_CLK_PWRSAVE;
+ writel_relaxed(core_vendor_spec, ioaddr + msm_offset->core_vendor_spec);
+
+ if (msm_host->use_14lpp_dll_reset) {
+ /* Disable CK_OUT */
+ config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
+ config &= ~CORE_CK_OUT_EN;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ /* Disable the DLL clock */
+ config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
+ config |= CORE_DLL_CLOCK_DISABLE;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
+ }
+
+ /*
+ * Write 1 to DLL_RST bit of DLL_CONFIG register
+ * and Write 1 to DLL_PDN bit of DLL_CONFIG register.
+ */
+ config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
+ config |= (CORE_DLL_RST | CORE_DLL_PDN);
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ /*
+ * Configure DLL_CONFIG_3 and USER_CTRL
+ * (Only applicable for 7FF projects).
+ */
+ if (msm_host->core_minor >= SDHCI_MSM_MIN_V_7FF) {
+ writel_relaxed(msm_host->dll.dll_config_3[index],
+ ioaddr + msm_offset->core_dll_config_3);
+ writel_relaxed(msm_host->dll.dll_usr_ctl[index],
+ ioaddr + msm_offset->core_dll_usr_ctl);
+ }
+
+ /*
+ * Set DDR_CONFIG since step 7 is setting TEST_CTRL that can be skipped.
+ */
+ ddr_cfg_offset = msm_host->updated_ddr_cfg ? msm_offset->core_ddr_config
+ : msm_offset->core_ddr_config_old;
+
+ config = msm_host->dll.ddr_config[index];
+ writel_relaxed(config, ioaddr + ddr_cfg_offset);
+
+ /* Set DLL_CONFIG_2 */
+ if (msm_host->use_14lpp_dll_reset) {
+ u32 mclk_freq;
+ int cycle_cnt;
+
+ /*
+ * Only configure the mclk_freq in normal DLL init
+ * context. If the DLL init is coming from
+ * CX Collapse Exit context, the host->clock may be zero.
+ * The DLL_CONFIG_2 register has already been restored to
+ * proper value prior to getting here.
+ */
+ if (init_context == DLL_INIT_NORMAL) {
+ cycle_cnt = readl_relaxed(ioaddr +
+ msm_offset->core_dll_config_2)
+ & CORE_FLL_CYCLE_CNT ? 8 : 4;
+
+ mclk_freq = DIV_ROUND_CLOSEST_ULL(dll_clock * cycle_cnt, TCXO_FREQ);
+
+ if (dll_clock < 100000000) {
+ pr_err("%s: %s: Non standard clk freq =%u\n",
+ mmc_hostname(mmc), __func__, dll_clock);
+ rc = -EINVAL;
+ goto out;
+ }
+
+ config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
+ config = (config & ~(0xFF << 10)) | (mclk_freq << 10);
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
+ }
+ /* wait for 5us before enabling DLL clock */
+ udelay(5);
+ }
+
+ /*
+ * Update the lower two bytes of DLL_CONFIG only with
+ * HSR values. Since these are the static settings.
+ */
+ config = (readl_relaxed(ioaddr + msm_offset->core_dll_config));
+ config |= (msm_host->dll.dll_config[index] & 0xffff);
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ /* Wait for 52us */
+ spin_unlock_irqrestore(&host->lock, flags);
+ udelay(60);
+ spin_lock_irqsave(&host->lock, flags);
+
+ /*
+ * Write 0 to DLL_RST bit of DLL_CONFIG register
+ * and Write 0 to DLL_PDN bit of DLL_CONFIG register.
+ */
+ config &= ~CORE_DLL_RST;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ config &= ~CORE_DLL_PDN;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+ /* Write 1 to DLL_RST bit of DLL_CONFIG register */
+ config |= CORE_DLL_RST;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ /* Write 0 to DLL_RST bit of DLL_CONFIG register */
+ config &= ~CORE_DLL_RST;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ /* Set CORE_DLL_CLOCK_DISABLE to 0 */
+ if (msm_host->use_14lpp_dll_reset) {
+ config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
+ config &= ~CORE_DLL_CLOCK_DISABLE;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
+ }
+
+ /* Set DLL_EN bit to 1. */
+ config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
+ config |= CORE_DLL_EN;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ /*
+ * Wait for 8000 input clock. Here we calculate the
+ * delay from fixed clock freq 192MHz, which turns out 42us.
+ */
+ spin_unlock_irqrestore(&host->lock, flags);
+ udelay(50);
+ spin_lock_irqsave(&host->lock, flags);
+
+ /* Set CK_OUT_EN bit to 1. */
+ config |= CORE_CK_OUT_EN;
+ writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+ /*
+ * Wait until DLL_LOCK bit of DLL_STATUS register
+ * becomes '1'.
+ */
+ while (!(readl_relaxed(ioaddr + msm_offset->core_dll_status) &
+ CORE_DLL_LOCK)) {
+ /* max. wait for 50us sec for LOCK bit to be set */
+ if (--wait_cnt == 0) {
+ dev_err(mmc_dev(mmc), "%s: DLL failed to LOCK\n",
+ mmc_hostname(mmc));
+ rc = -ETIMEDOUT;
+ goto out;
+ }
+ /* wait for 1us before polling again */
+ udelay(1);
+ }
+
+out:
+ if (core_vendor_spec & CORE_CLK_PWRSAVE) {
+ /* Reenable PWRSAVE as needed */
+ config = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
+ config |= CORE_CLK_PWRSAVE;
+ writel_relaxed(config, ioaddr + msm_offset->core_vendor_spec);
+ }
+ spin_unlock_irqrestore(&host->lock, flags);
+ return rc;
+}
+
static void msm_hc_select_default(struct sdhci_host *host)
{
struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -900,14 +1131,35 @@ static void sdhci_msm_hc_select_mode(struct sdhci_host *host)
msm_hc_select_default(host);
}
+static int sdhci_msm_init_dll(struct sdhci_host *host, enum dll_init_context init_context)
+{
+ unsigned char timing = host->mmc->ios.timing;
+ int ret;
+
+ if (timing == MMC_TIMING_UHS_SDR104 || timing == MMC_TIMING_MMC_HS400)
+ ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS400);
+ else
+ ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS200);
+
+ return ret;
+}
+
+static int sdhci_msm_dll_config(struct sdhci_host *host, enum dll_init_context init_context)
+{
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+
+ return msm_host->artanis_dll ? sdhci_msm_init_dll(host, init_context) :
+ msm_init_cm_dll(host);
+}
+
static int sdhci_msm_cdclp533_calibration(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;
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__);
@@ -915,7 +1167,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
* Retuning in HS400 (DDR mode) will fail, just reset the
* tuning block and restore the saved tuning phase.
*/
- ret = msm_init_cm_dll(host);
+ ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
if (ret)
goto out;
@@ -1003,7 +1255,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
return ret;
}
-static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
+static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host, enum mode index)
{
struct mmc_host *mmc = host->mmc;
u32 dll_status, config, ddr_cfg_offset;
@@ -1014,7 +1266,6 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
sdhci_priv_msm_offset(host);
pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
-
/*
* Currently the core_ddr_config register defaults to desired
* configuration on reset. Currently reprogramming the power on
@@ -1026,7 +1277,11 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
ddr_cfg_offset = msm_offset->core_ddr_config;
else
ddr_cfg_offset = msm_offset->core_ddr_config_old;
- writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
+
+ if (msm_host->artanis_dll)
+ writel_relaxed(msm_host->dll.ddr_config[index], host->ioaddr + ddr_cfg_offset);
+ else
+ writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
if (mmc->ios.enhanced_strobe) {
config = readl_relaxed(host->ioaddr +
@@ -1083,11 +1338,10 @@ static int sdhci_msm_hs400_dll_calibration(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;
struct mmc_host *mmc = host->mmc;
- int ret;
u32 config;
- const struct sdhci_msm_offset *msm_offset =
- msm_host->offset;
+ int ret;
pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
@@ -1095,7 +1349,8 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
* Retuning in HS400 (DDR mode) will fail, just reset the
* tuning block and restore the saved tuning phase.
*/
- ret = msm_init_cm_dll(host);
+ ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
+
if (ret)
goto out;
@@ -1115,7 +1370,7 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
if (msm_host->use_cdclp533)
ret = sdhci_msm_cdclp533_calibration(host);
else
- ret = sdhci_msm_cm_dll_sdc4_calibration(host);
+ ret = sdhci_msm_cm_dll_sdc4_calibration(host, HS400);
out:
pr_debug("%s: %s: Exit, ret %d\n", mmc_hostname(host->mmc),
__func__, ret);
@@ -1154,7 +1409,8 @@ static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
return 0;
/* Reset the tuning block */
- ret = msm_init_cm_dll(host);
+ ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
+
if (ret)
return ret;
@@ -1223,7 +1479,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
retry:
/* First of all reset the tuning block */
- rc = msm_init_cm_dll(host);
+ rc = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
if (rc)
return rc;
@@ -1752,11 +2008,6 @@ static unsigned int sdhci_msm_get_max_clock(struct sdhci_host *host)
return clk_round_rate(core_clk, ULONG_MAX);
}
-static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
-{
- return SDHCI_MSM_MIN_CLOCK;
-}
-
/*
* __sdhci_msm_set_clock - sdhci_msm clock control.
*
@@ -2400,6 +2651,86 @@ static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
return ret;
}
+static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
+ u32 **bw_vecs, int *len, u32 size)
+{
+ struct device_node *np = dev->of_node;
+ u32 *arr = NULL;
+ int ret = 0;
+ size_t sz;
+
+ if (!np) {
+ ret = -ENODEV;
+ goto out;
+ }
+ if (!of_get_property(np, prop_name, len)) {
+ ret = -EINVAL;
+ goto out;
+ }
+ sz = *len = *len / sizeof(*arr);
+ if (sz <= 0 || (size > 0 && (sz > size))) {
+ dev_err(dev, "%s invalid size\n", prop_name);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
+ if (!arr) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ ret = of_property_read_u32_array(np, prop_name, arr, sz);
+ if (ret < 0) {
+ dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
+ goto out;
+ }
+ *bw_vecs = arr;
+out:
+ if (ret)
+ *len = 0;
+ return ret;
+}
+
+static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
+{
+ int dll_table_len, dll_reg_count;
+ u32 *dll_table = NULL;
+ u32 dll_values[10];
+ int ret = 0, i;
+
+ if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
+ &dll_table, &dll_table_len, 0))
+ goto skip_dll;
+
+ dll_reg_count = sizeof(struct sdhci_msm_dll) / sizeof(u32);
+
+ if (dll_table_len != dll_reg_count) {
+ dev_err(dev, "Number of HSR entries are not matching\n");
+ ret = -EINVAL;
+ goto skip_dll;
+ }
+
+ for (i = 0; i < 5; i++) {
+ dll_values[2 * i] = dll_table[i];
+ dll_values[2 * i + 1] = dll_table[i + 5];
+ }
+
+ for (i = 0; i < 10; i++)
+ dll_table[i] = dll_values[i];
+
+ memcpy(&msm_host->dll, dll_table, sizeof(struct sdhci_msm_dll));
+ msm_host->artanis_dll = true;
+
+skip_dll:
+ if (!dll_table) {
+ msm_host->artanis_dll = false;
+ dev_err(dev, "Failed to get dll hsr settings from dt\n");
+ }
+
+ return ret;
+}
+
static int sdhci_msm_probe(struct platform_device *pdev)
{
struct sdhci_host *host;
@@ -2446,6 +2777,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
+ if (sdhci_msm_dt_parse_dll_info(&pdev->dev, msm_host))
+ goto pltfm_free;
+
ret = sdhci_msm_gcc_reset(&pdev->dev, host);
if (ret)
goto pltfm_free;
--
2.17.1On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
> With the current DLL sequence stability issues for data
> transfer seen in HS400 and HS200 modes.
>
> "mmc0: cqhci: error IRQ status: 0x00000000 cmd error -84
> data error 0"
>
> Rectify the DLL programming sequence as per latest hardware
> programming guide and also incorporate support for HS200 and
> HS400 DLL settings using the device tree.
"foo also bar" usually points out that there should be two separate
commits.
>
> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
> Signed-off-by: Jun Li <liju@codeaurora.org>
This is very strange and incorrect.
> ---
> drivers/mmc/host/sdhci-msm.c | 372 +++++++++++++++++++++++++++++++++--
> 1 file changed, 353 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 2a5e588779fc..4ecb362f7f2a 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -28,6 +28,7 @@
> #define CORE_VERSION_MAJOR_SHIFT 28
> #define CORE_VERSION_MAJOR_MASK (0xf << CORE_VERSION_MAJOR_SHIFT)
> #define CORE_VERSION_MINOR_MASK 0xff
> +#define SDHCI_MSM_MIN_V_7FF 0x6e
>
> #define CORE_MCI_GENERICS 0x70
> #define SWITCHABLE_SIGNALING_VOLTAGE BIT(29)
> @@ -118,7 +119,8 @@
> #define CORE_PWRSAVE_DLL BIT(3)
>
> #define DDR_CONFIG_POR_VAL 0x80040873
> -
> +#define DLL_CONFIG_3_POR_VAL 0x10
> +#define TCXO_FREQ 19200000
What about the platforms where TCXO has different frequency?
>
> #define INVALID_TUNING_PHASE -1
> #define SDHCI_MSM_MIN_CLOCK 400000
> @@ -256,6 +258,19 @@ struct sdhci_msm_variant_info {
> const struct sdhci_msm_offset *offset;
> };
>
> +/*
> + * DLL registers which needs be programmed with HSR settings.
> + * Add any new register only at the end and don't change the
> + * sequence.
Why?
> + */
> +struct sdhci_msm_dll {
> + u32 dll_config[2];
> + u32 dll_config_2[2];
> + u32 dll_config_3[2];
> + u32 dll_usr_ctl[2];
> + u32 ddr_config[2];
> +};
> +
> struct sdhci_msm_host {
> struct platform_device *pdev;
> void __iomem *core_mem; /* MSM SDCC mapped address */
> @@ -264,6 +279,7 @@ struct sdhci_msm_host {
> struct clk *xo_clk; /* TCXO clk needed for FLL feature of cm_dll*/
> /* core, iface, cal and sleep clocks */
> struct clk_bulk_data bulk_clks[4];
> + struct sdhci_msm_dll dll;
> #ifdef CONFIG_MMC_CRYPTO
> struct qcom_ice *ice;
> #endif
> @@ -292,6 +308,17 @@ struct sdhci_msm_host {
> u32 dll_config;
> u32 ddr_config;
> bool vqmmc_enabled;
> + bool artanis_dll;
> +};
> +
> +enum dll_init_context {
> + DLL_INIT_NORMAL,
> + DLL_INIT_FROM_CX_COLLAPSE_EXIT,
> +};
> +
> +enum mode {
> + HS400, // equivalent to SDR104 mode for DLL.
> + HS200, // equivalent to SDR50 mode for DLL.
> };
>
> static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> @@ -778,6 +805,210 @@ static int msm_init_cm_dll(struct sdhci_host *host)
> return 0;
> }
>
> +static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
> +{
> + return SDHCI_MSM_MIN_CLOCK;
> +}
Why??? Why do you need a function to return a static value?
> +
> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
> + unsigned int sup_clk;
> +
> + if (req_clk < sdhci_msm_get_min_clock(host))
> + return sdhci_msm_get_min_clock(host);
> +
> + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
> +
> + if (host->clock != msm_host->clk_rate)
> + sup_clk = sup_clk / 2;
> +
> + return sup_clk;
Why?
> +}
> +
> +/* Initialize the DLL (Programmable Delay Line) */
> +static int sdhci_msm_configure_dll(struct sdhci_host *host, enum dll_init_context
> + init_context, enum mode index)
> +{
> + 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;
> + struct mmc_host *mmc = host->mmc;
> + u32 ddr_cfg_offset, core_vendor_spec, config;
> + void __iomem *ioaddr = host->ioaddr;
> + unsigned long flags, dll_clock;
> + int rc = 0, wait_cnt = 50;
> +
> + dll_clock = sdhci_msm_get_clk_rate(host, host->clock);
> + spin_lock_irqsave(&host->lock, flags);
> +
> + core_vendor_spec = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
> +
> + /*
> + * Always disable PWRSAVE during the DLL power
> + * up regardless of its current setting.
> + */
> + core_vendor_spec &= ~CORE_CLK_PWRSAVE;
> + writel_relaxed(core_vendor_spec, ioaddr + msm_offset->core_vendor_spec);
> +
> + if (msm_host->use_14lpp_dll_reset) {
> + /* Disable CK_OUT */
> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> + config &= ~CORE_CK_OUT_EN;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + /* Disable the DLL clock */
> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> + config |= CORE_DLL_CLOCK_DISABLE;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> + }
> +
> + /*
> + * Write 1 to DLL_RST bit of DLL_CONFIG register
> + * and Write 1 to DLL_PDN bit of DLL_CONFIG register.
> + */
> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> + config |= (CORE_DLL_RST | CORE_DLL_PDN);
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + /*
> + * Configure DLL_CONFIG_3 and USER_CTRL
> + * (Only applicable for 7FF projects).
> + */
> + if (msm_host->core_minor >= SDHCI_MSM_MIN_V_7FF) {
> + writel_relaxed(msm_host->dll.dll_config_3[index],
> + ioaddr + msm_offset->core_dll_config_3);
> + writel_relaxed(msm_host->dll.dll_usr_ctl[index],
> + ioaddr + msm_offset->core_dll_usr_ctl);
> + }
> +
> + /*
> + * Set DDR_CONFIG since step 7 is setting TEST_CTRL that can be skipped.
> + */
> + ddr_cfg_offset = msm_host->updated_ddr_cfg ? msm_offset->core_ddr_config
> + : msm_offset->core_ddr_config_old;
> +
> + config = msm_host->dll.ddr_config[index];
> + writel_relaxed(config, ioaddr + ddr_cfg_offset);
> +
> + /* Set DLL_CONFIG_2 */
> + if (msm_host->use_14lpp_dll_reset) {
> + u32 mclk_freq;
> + int cycle_cnt;
> +
> + /*
> + * Only configure the mclk_freq in normal DLL init
> + * context. If the DLL init is coming from
> + * CX Collapse Exit context, the host->clock may be zero.
> + * The DLL_CONFIG_2 register has already been restored to
> + * proper value prior to getting here.
> + */
> + if (init_context == DLL_INIT_NORMAL) {
> + cycle_cnt = readl_relaxed(ioaddr +
> + msm_offset->core_dll_config_2)
> + & CORE_FLL_CYCLE_CNT ? 8 : 4;
> +
> + mclk_freq = DIV_ROUND_CLOSEST_ULL(dll_clock * cycle_cnt, TCXO_FREQ);
> +
> + if (dll_clock < 100000000) {
> + pr_err("%s: %s: Non standard clk freq =%u\n",
> + mmc_hostname(mmc), __func__, dll_clock);
> + rc = -EINVAL;
> + goto out;
> + }
> +
> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> + config = (config & ~(0xFF << 10)) | (mclk_freq << 10);
GENMASK, FIELD_PREP?
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> + }
> + /* wait for 5us before enabling DLL clock */
> + udelay(5);
> + }
> +
> + /*
> + * Update the lower two bytes of DLL_CONFIG only with
> + * HSR values. Since these are the static settings.
> + */
> + config = (readl_relaxed(ioaddr + msm_offset->core_dll_config));
> + config |= (msm_host->dll.dll_config[index] & 0xffff);
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + /* Wait for 52us */
> + spin_unlock_irqrestore(&host->lock, flags);
> + udelay(60);
> + spin_lock_irqsave(&host->lock, flags);
> +
> + /*
> + * Write 0 to DLL_RST bit of DLL_CONFIG register
> + * and Write 0 to DLL_PDN bit of DLL_CONFIG register.
> + */
> + config &= ~CORE_DLL_RST;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + config &= ~CORE_DLL_PDN;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> + /* Write 1 to DLL_RST bit of DLL_CONFIG register */
> + config |= CORE_DLL_RST;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + /* Write 0 to DLL_RST bit of DLL_CONFIG register */
> + config &= ~CORE_DLL_RST;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + /* Set CORE_DLL_CLOCK_DISABLE to 0 */
> + if (msm_host->use_14lpp_dll_reset) {
> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> + config &= ~CORE_DLL_CLOCK_DISABLE;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> + }
> +
> + /* Set DLL_EN bit to 1. */
> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> + config |= CORE_DLL_EN;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + /*
> + * Wait for 8000 input clock. Here we calculate the
> + * delay from fixed clock freq 192MHz, which turns out 42us.
> + */
> + spin_unlock_irqrestore(&host->lock, flags);
> + udelay(50);
> + spin_lock_irqsave(&host->lock, flags);
> +
> + /* Set CK_OUT_EN bit to 1. */
> + config |= CORE_CK_OUT_EN;
> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> + /*
> + * Wait until DLL_LOCK bit of DLL_STATUS register
> + * becomes '1'.
> + */
> + while (!(readl_relaxed(ioaddr + msm_offset->core_dll_status) &
> + CORE_DLL_LOCK)) {
> + /* max. wait for 50us sec for LOCK bit to be set */
> + if (--wait_cnt == 0) {
> + dev_err(mmc_dev(mmc), "%s: DLL failed to LOCK\n",
> + mmc_hostname(mmc));
> + rc = -ETIMEDOUT;
> + goto out;
> + }
> + /* wait for 1us before polling again */
> + udelay(1);
> + }
> +
> +out:
> + if (core_vendor_spec & CORE_CLK_PWRSAVE) {
> + /* Reenable PWRSAVE as needed */
> + config = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
> + config |= CORE_CLK_PWRSAVE;
> + writel_relaxed(config, ioaddr + msm_offset->core_vendor_spec);
> + }
> + spin_unlock_irqrestore(&host->lock, flags);
> + return rc;
> +}
> +
> static void msm_hc_select_default(struct sdhci_host *host)
> {
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -900,14 +1131,35 @@ static void sdhci_msm_hc_select_mode(struct sdhci_host *host)
> msm_hc_select_default(host);
> }
>
> +static int sdhci_msm_init_dll(struct sdhci_host *host, enum dll_init_context init_context)
> +{
> + unsigned char timing = host->mmc->ios.timing;
> + int ret;
> +
> + if (timing == MMC_TIMING_UHS_SDR104 || timing == MMC_TIMING_MMC_HS400)
> + ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS400);
> + else
> + ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS200);
> +
> + return ret;
> +}
> +
> +static int sdhci_msm_dll_config(struct sdhci_host *host, enum dll_init_context init_context)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> + return msm_host->artanis_dll ? sdhci_msm_init_dll(host, init_context) :
> + msm_init_cm_dll(host);
> +}
> +
> static int sdhci_msm_cdclp533_calibration(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;
> 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__);
>
> @@ -915,7 +1167,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
> * Retuning in HS400 (DDR mode) will fail, just reset the
> * tuning block and restore the saved tuning phase.
> */
> - ret = msm_init_cm_dll(host);
> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> if (ret)
> goto out;
>
> @@ -1003,7 +1255,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
> return ret;
> }
>
> -static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
> +static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host, enum mode index)
> {
> struct mmc_host *mmc = host->mmc;
> u32 dll_status, config, ddr_cfg_offset;
> @@ -1014,7 +1266,6 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
> sdhci_priv_msm_offset(host);
>
> pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
> -
Unrelated, please drop.
> /*
> * Currently the core_ddr_config register defaults to desired
> * configuration on reset. Currently reprogramming the power on
> @@ -1026,7 +1277,11 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
> ddr_cfg_offset = msm_offset->core_ddr_config;
> else
> ddr_cfg_offset = msm_offset->core_ddr_config_old;
> - writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
> +
> + if (msm_host->artanis_dll)
> + writel_relaxed(msm_host->dll.ddr_config[index], host->ioaddr + ddr_cfg_offset);
> + else
> + writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
>
> if (mmc->ios.enhanced_strobe) {
> config = readl_relaxed(host->ioaddr +
> @@ -1083,11 +1338,10 @@ static int sdhci_msm_hs400_dll_calibration(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;
> struct mmc_host *mmc = host->mmc;
> - int ret;
> u32 config;
> - const struct sdhci_msm_offset *msm_offset =
> - msm_host->offset;
> + int ret;
>
> pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
>
> @@ -1095,7 +1349,8 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
> * Retuning in HS400 (DDR mode) will fail, just reset the
> * tuning block and restore the saved tuning phase.
> */
> - ret = msm_init_cm_dll(host);
> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> +
> if (ret)
> goto out;
>
> @@ -1115,7 +1370,7 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
> if (msm_host->use_cdclp533)
> ret = sdhci_msm_cdclp533_calibration(host);
> else
> - ret = sdhci_msm_cm_dll_sdc4_calibration(host);
> + ret = sdhci_msm_cm_dll_sdc4_calibration(host, HS400);
> out:
> pr_debug("%s: %s: Exit, ret %d\n", mmc_hostname(host->mmc),
> __func__, ret);
> @@ -1154,7 +1409,8 @@ static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
> return 0;
>
> /* Reset the tuning block */
> - ret = msm_init_cm_dll(host);
> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> +
> if (ret)
> return ret;
>
> @@ -1223,7 +1479,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>
> retry:
> /* First of all reset the tuning block */
> - rc = msm_init_cm_dll(host);
> + rc = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> if (rc)
> return rc;
>
> @@ -1752,11 +2008,6 @@ static unsigned int sdhci_msm_get_max_clock(struct sdhci_host *host)
> return clk_round_rate(core_clk, ULONG_MAX);
> }
>
> -static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
> -{
> - return SDHCI_MSM_MIN_CLOCK;
> -}
> -
> /*
> * __sdhci_msm_set_clock - sdhci_msm clock control.
> *
> @@ -2400,6 +2651,86 @@ static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
> return ret;
> }
>
> +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
> + u32 **bw_vecs, int *len, u32 size)
> +{
> + struct device_node *np = dev->of_node;
> + u32 *arr = NULL;
> + int ret = 0;
> + size_t sz;
> +
> + if (!np) {
> + ret = -ENODEV;
> + goto out;
> + }
> + if (!of_get_property(np, prop_name, len)) {
> + ret = -EINVAL;
> + goto out;
> + }
> + sz = *len = *len / sizeof(*arr);
You obviously skipped checkpatch run. Please don't do that.
> + if (sz <= 0 || (size > 0 && (sz > size))) {
> + dev_err(dev, "%s invalid size\n", prop_name);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
> + if (!arr) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + ret = of_property_read_u32_array(np, prop_name, arr, sz);
> + if (ret < 0) {
> + dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
> + goto out;
> + }
> + *bw_vecs = arr;
> +out:
> + if (ret)
> + *len = 0;
> + return ret;
> +}
> +
> +static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
> +{
> + int dll_table_len, dll_reg_count;
> + u32 *dll_table = NULL;
> + u32 dll_values[10];
> + int ret = 0, i;
> +
> + if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
> + &dll_table, &dll_table_len, 0))
> + goto skip_dll;
Missing update for the bindings.
> +
> + dll_reg_count = sizeof(struct sdhci_msm_dll) / sizeof(u32);
> +
> + if (dll_table_len != dll_reg_count) {
> + dev_err(dev, "Number of HSR entries are not matching\n");
> + ret = -EINVAL;
> + goto skip_dll;
> + }
> +
> + for (i = 0; i < 5; i++) {
Magic value 5, replace with ARRAY_SIZE
> + dll_values[2 * i] = dll_table[i];
> + dll_values[2 * i + 1] = dll_table[i + 5];
> + }
> +
> + for (i = 0; i < 10; i++)
> + dll_table[i] = dll_values[i];
So three memory copies to rearrange the order of values? That sounds
like a horrible solution.
> +
> + memcpy(&msm_host->dll, dll_table, sizeof(struct sdhci_msm_dll));
> + msm_host->artanis_dll = true;
> +
> +skip_dll:
> + if (!dll_table) {
> + msm_host->artanis_dll = false;
> + dev_err(dev, "Failed to get dll hsr settings from dt\n");
> + }
> +
> + return ret;
> +}
> +
> static int sdhci_msm_probe(struct platform_device *pdev)
> {
> struct sdhci_host *host;
> @@ -2446,6 +2777,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>
> msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
>
> + if (sdhci_msm_dt_parse_dll_info(&pdev->dev, msm_host))
> + goto pltfm_free;
> +
> ret = sdhci_msm_gcc_reset(&pdev->dev, host);
> if (ret)
> goto pltfm_free;
> --
> 2.17.1
>
--
With best wishes
Dmitry
On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>> With the current DLL sequence stability issues for data
>> transfer seen in HS400 and HS200 modes.
>>
>> "mmc0: cqhci: error IRQ status: 0x00000000 cmd error -84
>> data error 0"
>>
>> Rectify the DLL programming sequence as per latest hardware
>> programming guide and also incorporate support for HS200 and
>> HS400 DLL settings using the device tree.
>
> "foo also bar" usually points out that there should be two separate
> commits.
Thanks for review. I will split it into two patches.
>
>>
>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
>> Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
>> Signed-off-by: Jun Li <liju@codeaurora.org>
>
> This is very strange and incorrect.
Thanks for review. I will fix the format.
>
>> ---
>> drivers/mmc/host/sdhci-msm.c | 372 +++++++++++++++++++++++++++++++++--
>> 1 file changed, 353 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 2a5e588779fc..4ecb362f7f2a 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -28,6 +28,7 @@
>> #define CORE_VERSION_MAJOR_SHIFT 28
>> #define CORE_VERSION_MAJOR_MASK (0xf << CORE_VERSION_MAJOR_SHIFT)
>> #define CORE_VERSION_MINOR_MASK 0xff
>> +#define SDHCI_MSM_MIN_V_7FF 0x6e
>>
>> #define CORE_MCI_GENERICS 0x70
>> #define SWITCHABLE_SIGNALING_VOLTAGE BIT(29)
>> @@ -118,7 +119,8 @@
>> #define CORE_PWRSAVE_DLL BIT(3)
>>
>> #define DDR_CONFIG_POR_VAL 0x80040873
>> -
>> +#define DLL_CONFIG_3_POR_VAL 0x10
>> +#define TCXO_FREQ 19200000
>
> What about the platforms where TCXO has different frequency?
>
All emmc targets have 192 Mhz as TCXO freq.
>>
>> #define INVALID_TUNING_PHASE -1
>> #define SDHCI_MSM_MIN_CLOCK 400000
>> @@ -256,6 +258,19 @@ struct sdhci_msm_variant_info {
>> const struct sdhci_msm_offset *offset;
>> };
>>
>> +/*
>> + * DLL registers which needs be programmed with HSR settings.
>> + * Add any new register only at the end and don't change the
>> + * sequence.
>
> Why?
I will update the comment message in next patchset.
>
>> + */
>> +struct sdhci_msm_dll {
>> + u32 dll_config[2];
>> + u32 dll_config_2[2];
>> + u32 dll_config_3[2];
>> + u32 dll_usr_ctl[2];
>> + u32 ddr_config[2];
>> +};
>> +
>> struct sdhci_msm_host {
>> struct platform_device *pdev;
>> void __iomem *core_mem; /* MSM SDCC mapped address */
>> @@ -264,6 +279,7 @@ struct sdhci_msm_host {
>> struct clk *xo_clk; /* TCXO clk needed for FLL feature of cm_dll*/
>> /* core, iface, cal and sleep clocks */
>> struct clk_bulk_data bulk_clks[4];
>> + struct sdhci_msm_dll dll;
>> #ifdef CONFIG_MMC_CRYPTO
>> struct qcom_ice *ice;
>> #endif
>> @@ -292,6 +308,17 @@ struct sdhci_msm_host {
>> u32 dll_config;
>> u32 ddr_config;
>> bool vqmmc_enabled;
>> + bool artanis_dll;
>> +};
>> +
>> +enum dll_init_context {
>> + DLL_INIT_NORMAL,
>> + DLL_INIT_FROM_CX_COLLAPSE_EXIT,
>> +};
>> +
>> +enum mode {
>> + HS400, // equivalent to SDR104 mode for DLL.
>> + HS200, // equivalent to SDR50 mode for DLL.
>> };
>>
>> static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>> @@ -778,6 +805,210 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>> return 0;
>> }
>>
>> +static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
>> +{
>> + return SDHCI_MSM_MIN_CLOCK;
>> +}
>
> Why??? Why do you need a function to return a static value?
>
This is just rearrangement of the function. This function already exist,
moving here to avoid predeclaration.
>> +
>> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
>> + unsigned int sup_clk;
>> +
>> + if (req_clk < sdhci_msm_get_min_clock(host))
>> + return sdhci_msm_get_min_clock(host);
>> +
>> + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
>> +
>> + if (host->clock != msm_host->clk_rate)
>> + sup_clk = sup_clk / 2;
>> +
>> + return sup_clk;
>
> Why?
Sorry, I did not understand your question. Can you please explain in detail.
>
>> +}
>> +
>> +/* Initialize the DLL (Programmable Delay Line) */
>> +static int sdhci_msm_configure_dll(struct sdhci_host *host, enum dll_init_context
>> + init_context, enum mode index)
>> +{
>> + 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;
>> + struct mmc_host *mmc = host->mmc;
>> + u32 ddr_cfg_offset, core_vendor_spec, config;
>> + void __iomem *ioaddr = host->ioaddr;
>> + unsigned long flags, dll_clock;
>> + int rc = 0, wait_cnt = 50;
>> +
>> + dll_clock = sdhci_msm_get_clk_rate(host, host->clock);
>> + spin_lock_irqsave(&host->lock, flags);
>> +
>> + core_vendor_spec = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
>> +
>> + /*
>> + * Always disable PWRSAVE during the DLL power
>> + * up regardless of its current setting.
>> + */
>> + core_vendor_spec &= ~CORE_CLK_PWRSAVE;
>> + writel_relaxed(core_vendor_spec, ioaddr + msm_offset->core_vendor_spec);
>> +
>> + if (msm_host->use_14lpp_dll_reset) {
>> + /* Disable CK_OUT */
>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>> + config &= ~CORE_CK_OUT_EN;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + /* Disable the DLL clock */
>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>> + config |= CORE_DLL_CLOCK_DISABLE;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>> + }
>> +
>> + /*
>> + * Write 1 to DLL_RST bit of DLL_CONFIG register
>> + * and Write 1 to DLL_PDN bit of DLL_CONFIG register.
>> + */
>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>> + config |= (CORE_DLL_RST | CORE_DLL_PDN);
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + /*
>> + * Configure DLL_CONFIG_3 and USER_CTRL
>> + * (Only applicable for 7FF projects).
>> + */
>> + if (msm_host->core_minor >= SDHCI_MSM_MIN_V_7FF) {
>> + writel_relaxed(msm_host->dll.dll_config_3[index],
>> + ioaddr + msm_offset->core_dll_config_3);
>> + writel_relaxed(msm_host->dll.dll_usr_ctl[index],
>> + ioaddr + msm_offset->core_dll_usr_ctl);
>> + }
>> +
>> + /*
>> + * Set DDR_CONFIG since step 7 is setting TEST_CTRL that can be skipped.
>> + */
>> + ddr_cfg_offset = msm_host->updated_ddr_cfg ? msm_offset->core_ddr_config
>> + : msm_offset->core_ddr_config_old;
>> +
>> + config = msm_host->dll.ddr_config[index];
>> + writel_relaxed(config, ioaddr + ddr_cfg_offset);
>> +
>> + /* Set DLL_CONFIG_2 */
>> + if (msm_host->use_14lpp_dll_reset) {
>> + u32 mclk_freq;
>> + int cycle_cnt;
>> +
>> + /*
>> + * Only configure the mclk_freq in normal DLL init
>> + * context. If the DLL init is coming from
>> + * CX Collapse Exit context, the host->clock may be zero.
>> + * The DLL_CONFIG_2 register has already been restored to
>> + * proper value prior to getting here.
>> + */
>> + if (init_context == DLL_INIT_NORMAL) {
>> + cycle_cnt = readl_relaxed(ioaddr +
>> + msm_offset->core_dll_config_2)
>> + & CORE_FLL_CYCLE_CNT ? 8 : 4;
>> +
>> + mclk_freq = DIV_ROUND_CLOSEST_ULL(dll_clock * cycle_cnt, TCXO_FREQ);
>> +
>> + if (dll_clock < 100000000) {
>> + pr_err("%s: %s: Non standard clk freq =%u\n",
>> + mmc_hostname(mmc), __func__, dll_clock);
>> + rc = -EINVAL;
>> + goto out;
>> + }
>> +
>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>> + config = (config & ~(0xFF << 10)) | (mclk_freq << 10);
>
> GENMASK, FIELD_PREP?
Sure I will use the suggested macros.
>
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>> + }
>> + /* wait for 5us before enabling DLL clock */
>> + udelay(5);
>> + }
>> +
>> + /*
>> + * Update the lower two bytes of DLL_CONFIG only with
>> + * HSR values. Since these are the static settings.
>> + */
>> + config = (readl_relaxed(ioaddr + msm_offset->core_dll_config));
>> + config |= (msm_host->dll.dll_config[index] & 0xffff);
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + /* Wait for 52us */
>> + spin_unlock_irqrestore(&host->lock, flags);
>> + udelay(60);
>> + spin_lock_irqsave(&host->lock, flags);
>> +
>> + /*
>> + * Write 0 to DLL_RST bit of DLL_CONFIG register
>> + * and Write 0 to DLL_PDN bit of DLL_CONFIG register.
>> + */
>> + config &= ~CORE_DLL_RST;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + config &= ~CORE_DLL_PDN;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> + /* Write 1 to DLL_RST bit of DLL_CONFIG register */
>> + config |= CORE_DLL_RST;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + /* Write 0 to DLL_RST bit of DLL_CONFIG register */
>> + config &= ~CORE_DLL_RST;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + /* Set CORE_DLL_CLOCK_DISABLE to 0 */
>> + if (msm_host->use_14lpp_dll_reset) {
>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>> + config &= ~CORE_DLL_CLOCK_DISABLE;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>> + }
>> +
>> + /* Set DLL_EN bit to 1. */
>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>> + config |= CORE_DLL_EN;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + /*
>> + * Wait for 8000 input clock. Here we calculate the
>> + * delay from fixed clock freq 192MHz, which turns out 42us.
>> + */
>> + spin_unlock_irqrestore(&host->lock, flags);
>> + udelay(50);
>> + spin_lock_irqsave(&host->lock, flags);
>> +
>> + /* Set CK_OUT_EN bit to 1. */
>> + config |= CORE_CK_OUT_EN;
>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> + /*
>> + * Wait until DLL_LOCK bit of DLL_STATUS register
>> + * becomes '1'.
>> + */
>> + while (!(readl_relaxed(ioaddr + msm_offset->core_dll_status) &
>> + CORE_DLL_LOCK)) {
>> + /* max. wait for 50us sec for LOCK bit to be set */
>> + if (--wait_cnt == 0) {
>> + dev_err(mmc_dev(mmc), "%s: DLL failed to LOCK\n",
>> + mmc_hostname(mmc));
>> + rc = -ETIMEDOUT;
>> + goto out;
>> + }
>> + /* wait for 1us before polling again */
>> + udelay(1);
>> + }
>> +
>> +out:
>> + if (core_vendor_spec & CORE_CLK_PWRSAVE) {
>> + /* Reenable PWRSAVE as needed */
>> + config = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
>> + config |= CORE_CLK_PWRSAVE;
>> + writel_relaxed(config, ioaddr + msm_offset->core_vendor_spec);
>> + }
>> + spin_unlock_irqrestore(&host->lock, flags);
>> + return rc;
>> +}
>> +
>> static void msm_hc_select_default(struct sdhci_host *host)
>> {
>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -900,14 +1131,35 @@ static void sdhci_msm_hc_select_mode(struct sdhci_host *host)
>> msm_hc_select_default(host);
>> }
>>
>> +static int sdhci_msm_init_dll(struct sdhci_host *host, enum dll_init_context init_context)
>> +{
>> + unsigned char timing = host->mmc->ios.timing;
>> + int ret;
>> +
>> + if (timing == MMC_TIMING_UHS_SDR104 || timing == MMC_TIMING_MMC_HS400)
>> + ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS400);
>> + else
>> + ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS200);
>> +
>> + return ret;
>> +}
>> +
>> +static int sdhci_msm_dll_config(struct sdhci_host *host, enum dll_init_context init_context)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +
>> + return msm_host->artanis_dll ? sdhci_msm_init_dll(host, init_context) :
>> + msm_init_cm_dll(host);
>> +}
>> +
>> static int sdhci_msm_cdclp533_calibration(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;
>> 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__);
>>
>> @@ -915,7 +1167,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>> * Retuning in HS400 (DDR mode) will fail, just reset the
>> * tuning block and restore the saved tuning phase.
>> */
>> - ret = msm_init_cm_dll(host);
>> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>> if (ret)
>> goto out;
>>
>> @@ -1003,7 +1255,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>> return ret;
>> }
>>
>> -static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>> +static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host, enum mode index)
>> {
>> struct mmc_host *mmc = host->mmc;
>> u32 dll_status, config, ddr_cfg_offset;
>> @@ -1014,7 +1266,6 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>> sdhci_priv_msm_offset(host);
>>
>> pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
>> -
>
> Unrelated, please drop.
I will fix it in next patchset.
>
>> /*
>> * Currently the core_ddr_config register defaults to desired
>> * configuration on reset. Currently reprogramming the power on
>> @@ -1026,7 +1277,11 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>> ddr_cfg_offset = msm_offset->core_ddr_config;
>> else
>> ddr_cfg_offset = msm_offset->core_ddr_config_old;
>> - writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
>> +
>> + if (msm_host->artanis_dll)
>> + writel_relaxed(msm_host->dll.ddr_config[index], host->ioaddr + ddr_cfg_offset);
>> + else
>> + writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
>>
>> if (mmc->ios.enhanced_strobe) {
>> config = readl_relaxed(host->ioaddr +
>> @@ -1083,11 +1338,10 @@ static int sdhci_msm_hs400_dll_calibration(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;
>> struct mmc_host *mmc = host->mmc;
>> - int ret;
>> u32 config;
>> - const struct sdhci_msm_offset *msm_offset =
>> - msm_host->offset;
>> + int ret;
>>
>> pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
>>
>> @@ -1095,7 +1349,8 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>> * Retuning in HS400 (DDR mode) will fail, just reset the
>> * tuning block and restore the saved tuning phase.
>> */
>> - ret = msm_init_cm_dll(host);
>> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>> +
>> if (ret)
>> goto out;
>>
>> @@ -1115,7 +1370,7 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>> if (msm_host->use_cdclp533)
>> ret = sdhci_msm_cdclp533_calibration(host);
>> else
>> - ret = sdhci_msm_cm_dll_sdc4_calibration(host);
>> + ret = sdhci_msm_cm_dll_sdc4_calibration(host, HS400);
>> out:
>> pr_debug("%s: %s: Exit, ret %d\n", mmc_hostname(host->mmc),
>> __func__, ret);
>> @@ -1154,7 +1409,8 @@ static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
>> return 0;
>>
>> /* Reset the tuning block */
>> - ret = msm_init_cm_dll(host);
>> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>> +
>> if (ret)
>> return ret;
>>
>> @@ -1223,7 +1479,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>
>> retry:
>> /* First of all reset the tuning block */
>> - rc = msm_init_cm_dll(host);
>> + rc = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>> if (rc)
>> return rc;
>>
>> @@ -1752,11 +2008,6 @@ static unsigned int sdhci_msm_get_max_clock(struct sdhci_host *host)
>> return clk_round_rate(core_clk, ULONG_MAX);
>> }
>>
>> -static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
>> -{
>> - return SDHCI_MSM_MIN_CLOCK;
>> -}
>> -
>> /*
>> * __sdhci_msm_set_clock - sdhci_msm clock control.
>> *
>> @@ -2400,6 +2651,86 @@ static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
>> return ret;
>> }
>>
>> +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
>> + u32 **bw_vecs, int *len, u32 size)
>> +{
>> + struct device_node *np = dev->of_node;
>> + u32 *arr = NULL;
>> + int ret = 0;
>> + size_t sz;
>> +
>> + if (!np) {
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> + if (!of_get_property(np, prop_name, len)) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> + sz = *len = *len / sizeof(*arr);
>
> You obviously skipped checkpatch run. Please don't do that.
Before submitting the patchset I have already executed the checkpatch
please find the output.
$ ./scripts/checkpatch.pl patch/*
-----------------------------
patch/0000-cover-letter.patch
-----------------------------
total: 0 errors, 0 warnings, 0 lines checked
patch/0000-cover-letter.patch has no obvious style problems and is
ready for submission.
---------------------------------------------------------------------
patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch
---------------------------------------------------------------------
total: 0 errors, 0 warnings, 18 lines checked
patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch
has no obvious style problems and is ready for submission.
---------------------------------------------------------------------
patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch
---------------------------------------------------------------------
total: 0 errors, 0 warnings, 494 lines checked
patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch
has no obvious style problems and is ready for submission.
>
>> + if (sz <= 0 || (size > 0 && (sz > size))) {
>> + dev_err(dev, "%s invalid size\n", prop_name);
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
>> + if (!arr) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + ret = of_property_read_u32_array(np, prop_name, arr, sz);
>> + if (ret < 0) {
>> + dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
>> + goto out;
>> + }
>> + *bw_vecs = arr;
>> +out:
>> + if (ret)
>> + *len = 0;
>> + return ret;
>> +}
>> +
>> +static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
>> +{
>> + int dll_table_len, dll_reg_count;
>> + u32 *dll_table = NULL;
>> + u32 dll_values[10];
>> + int ret = 0, i;
>> +
>> + if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
>> + &dll_table, &dll_table_len, 0))
>> + goto skip_dll;
>
> Missing update for the bindings.
I will update in the next patchset.
>
>> +
>> + dll_reg_count = sizeof(struct sdhci_msm_dll) / sizeof(u32);
>> +
>> + if (dll_table_len != dll_reg_count) {
>> + dev_err(dev, "Number of HSR entries are not matching\n");
>> + ret = -EINVAL;
>> + goto skip_dll;
>> + }
>> +
>> + for (i = 0; i < 5; i++) {
>
> Magic value 5, replace with ARRAY_SIZE
I will fix in next patchset.
>
>> + dll_values[2 * i] = dll_table[i];
>> + dll_values[2 * i + 1] = dll_table[i + 5];
>> + }
>> +
>> + for (i = 0; i < 10; i++)
>> + dll_table[i] = dll_values[i];
>
> So three memory copies to rearrange the order of values? That sounds
> like a horrible solution.
I will fix in next patchset.
>
>> +
>> + memcpy(&msm_host->dll, dll_table, sizeof(struct sdhci_msm_dll));
>> + msm_host->artanis_dll = true;
>> +
>> +skip_dll:
>> + if (!dll_table) {
>> + msm_host->artanis_dll = false;
>> + dev_err(dev, "Failed to get dll hsr settings from dt\n");
>> + }
>> +
>> + return ret;
>> +}
>> +
>> static int sdhci_msm_probe(struct platform_device *pdev)
>> {
>> struct sdhci_host *host;
>> @@ -2446,6 +2777,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>
>> msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
>>
>> + if (sdhci_msm_dt_parse_dll_info(&pdev->dev, msm_host))
>> + goto pltfm_free;
>> +
>> ret = sdhci_msm_gcc_reset(&pdev->dev, host);
>> if (ret)
>> goto pltfm_free;
>> --
>> 2.17.1
>>
>
On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>
>
> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
> > On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
> > > With the current DLL sequence stability issues for data
> > > transfer seen in HS400 and HS200 modes.
> > >
> > > "mmc0: cqhci: error IRQ status: 0x00000000 cmd error -84
> > > data error 0"
> > >
> > > Rectify the DLL programming sequence as per latest hardware
> > > programming guide and also incorporate support for HS200 and
> > > HS400 DLL settings using the device tree.
> >
> > "foo also bar" usually points out that there should be two separate
> > commits.
>
> Thanks for review. I will split it into two patches.
>
> >
> > >
> > > Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> > > Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> > > Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
> > > Signed-off-by: Jun Li <liju@codeaurora.org>
> >
> > This is very strange and incorrect.
>
> Thanks for review. I will fix the format.
Well. If you write that you will fix the format, may I ask, how or what
do you plan to fix?
>
> >
> > > ---
> > > drivers/mmc/host/sdhci-msm.c | 372 +++++++++++++++++++++++++++++++++--
> > > 1 file changed, 353 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> > > index 2a5e588779fc..4ecb362f7f2a 100644
> > > --- a/drivers/mmc/host/sdhci-msm.c
> > > +++ b/drivers/mmc/host/sdhci-msm.c
> > > @@ -28,6 +28,7 @@
> > > #define CORE_VERSION_MAJOR_SHIFT 28
> > > #define CORE_VERSION_MAJOR_MASK (0xf << CORE_VERSION_MAJOR_SHIFT)
> > > #define CORE_VERSION_MINOR_MASK 0xff
> > > +#define SDHCI_MSM_MIN_V_7FF 0x6e
> > > #define CORE_MCI_GENERICS 0x70
> > > #define SWITCHABLE_SIGNALING_VOLTAGE BIT(29)
> > > @@ -118,7 +119,8 @@
> > > #define CORE_PWRSAVE_DLL BIT(3)
> > > #define DDR_CONFIG_POR_VAL 0x80040873
> > > -
> > > +#define DLL_CONFIG_3_POR_VAL 0x10
> > > +#define TCXO_FREQ 19200000
> >
> > What about the platforms where TCXO has different frequency?
> >
>
> All emmc targets have 192 Mhz as TCXO freq.
So, it's not a TCXO freq, but some other base freq?
>
> > > #define INVALID_TUNING_PHASE -1
> > > #define SDHCI_MSM_MIN_CLOCK 400000
> > > @@ -256,6 +258,19 @@ struct sdhci_msm_variant_info {
> > > const struct sdhci_msm_offset *offset;
> > > };
> > > +/*
> > > + * DLL registers which needs be programmed with HSR settings.
> > > + * Add any new register only at the end and don't change the
> > > + * sequence.
> >
> > Why?
>
> I will update the comment message in next patchset.
Well, you can respond to a question first. And once something is settled
you can get that to the commit message. It might save some round-trip
time.
>
> >
> > > + */
> > > +struct sdhci_msm_dll {
> > > + u32 dll_config[2];
> > > + u32 dll_config_2[2];
> > > + u32 dll_config_3[2];
> > > + u32 dll_usr_ctl[2];
> > > + u32 ddr_config[2];
> > > +};
> > > +
> > > struct sdhci_msm_host {
> > > struct platform_device *pdev;
> > > void __iomem *core_mem; /* MSM SDCC mapped address */
> > > @@ -264,6 +279,7 @@ struct sdhci_msm_host {
> > > struct clk *xo_clk; /* TCXO clk needed for FLL feature of cm_dll*/
> > > /* core, iface, cal and sleep clocks */
> > > struct clk_bulk_data bulk_clks[4];
> > > + struct sdhci_msm_dll dll;
> > > #ifdef CONFIG_MMC_CRYPTO
> > > struct qcom_ice *ice;
> > > #endif
> > > @@ -292,6 +308,17 @@ struct sdhci_msm_host {
> > > u32 dll_config;
> > > u32 ddr_config;
> > > bool vqmmc_enabled;
> > > + bool artanis_dll;
> > > +};
> > > +
> > > +enum dll_init_context {
> > > + DLL_INIT_NORMAL,
> > > + DLL_INIT_FROM_CX_COLLAPSE_EXIT,
> > > +};
> > > +
> > > +enum mode {
> > > + HS400, // equivalent to SDR104 mode for DLL.
> > > + HS200, // equivalent to SDR50 mode for DLL.
> > > };
> > > static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> > > @@ -778,6 +805,210 @@ static int msm_init_cm_dll(struct sdhci_host *host)
> > > return 0;
> > > }
> > > +static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
> > > +{
> > > + return SDHCI_MSM_MIN_CLOCK;
> > > +}
> >
> > Why??? Why do you need a function to return a static value?
> >
>
> This is just rearrangement of the function. This function already exist,
> moving here to avoid predeclaration.
Okay.
> > > +
> > > +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
> > > +{
> > > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > > + struct clk *core_clk = msm_host->bulk_clks[0].clk;
> > > + unsigned int sup_clk;
> > > +
> > > + if (req_clk < sdhci_msm_get_min_clock(host))
> > > + return sdhci_msm_get_min_clock(host);
> > > +
> > > + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
> > > +
> > > + if (host->clock != msm_host->clk_rate)
> > > + sup_clk = sup_clk / 2;
> > > +
> > > + return sup_clk;
> >
> > Why?
>
> Sorry, I did not understand your question. Can you please explain in detail.
Please explain the maths. You get the rate from the clock, then you
round it, but it is the rate that has just been returned, so there
should be no need to round it. And after that there a division by two
for some reason. So I've asked for an explanation for that code.
>
> >
> > > +}
> > > +
> > > +/* Initialize the DLL (Programmable Delay Line) */
> > > +static int sdhci_msm_configure_dll(struct sdhci_host *host, enum dll_init_context
> > > + init_context, enum mode index)
> > > +{
> > > + 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;
> > > + struct mmc_host *mmc = host->mmc;
> > > + u32 ddr_cfg_offset, core_vendor_spec, config;
> > > + void __iomem *ioaddr = host->ioaddr;
> > > + unsigned long flags, dll_clock;
> > > + int rc = 0, wait_cnt = 50;
> > > +
> > > + dll_clock = sdhci_msm_get_clk_rate(host, host->clock);
> > > + spin_lock_irqsave(&host->lock, flags);
> > > +
> > > + core_vendor_spec = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
> > > +
> > > + /*
> > > + * Always disable PWRSAVE during the DLL power
> > > + * up regardless of its current setting.
> > > + */
> > > + core_vendor_spec &= ~CORE_CLK_PWRSAVE;
> > > + writel_relaxed(core_vendor_spec, ioaddr + msm_offset->core_vendor_spec);
> > > +
> > > + if (msm_host->use_14lpp_dll_reset) {
> > > + /* Disable CK_OUT */
> > > + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> > > + config &= ~CORE_CK_OUT_EN;
> > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > +
> > > + /* Disable the DLL clock */
> > > + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> > > + config |= CORE_DLL_CLOCK_DISABLE;
> > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> > > + }
> > > +
> > > + /*
> > > + * Write 1 to DLL_RST bit of DLL_CONFIG register
> > > + * and Write 1 to DLL_PDN bit of DLL_CONFIG register.
> > > + */
> > > + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> > > + config |= (CORE_DLL_RST | CORE_DLL_PDN);
> > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > +
> > > + /*
> > > + * Configure DLL_CONFIG_3 and USER_CTRL
> > > + * (Only applicable for 7FF projects).
> > > + */
> > > + if (msm_host->core_minor >= SDHCI_MSM_MIN_V_7FF) {
> > > + writel_relaxed(msm_host->dll.dll_config_3[index],
> > > + ioaddr + msm_offset->core_dll_config_3);
> > > + writel_relaxed(msm_host->dll.dll_usr_ctl[index],
> > > + ioaddr + msm_offset->core_dll_usr_ctl);
> > > + }
> > > +
> > > + /*
> > > + * Set DDR_CONFIG since step 7 is setting TEST_CTRL that can be skipped.
> > > + */
> > > + ddr_cfg_offset = msm_host->updated_ddr_cfg ? msm_offset->core_ddr_config
> > > + : msm_offset->core_ddr_config_old;
> > > +
> > > + config = msm_host->dll.ddr_config[index];
> > > + writel_relaxed(config, ioaddr + ddr_cfg_offset);
> > > +
> > > + /* Set DLL_CONFIG_2 */
> > > + if (msm_host->use_14lpp_dll_reset) {
> > > + u32 mclk_freq;
> > > + int cycle_cnt;
> > > +
> > > + /*
> > > + * Only configure the mclk_freq in normal DLL init
> > > + * context. If the DLL init is coming from
> > > + * CX Collapse Exit context, the host->clock may be zero.
> > > + * The DLL_CONFIG_2 register has already been restored to
> > > + * proper value prior to getting here.
> > > + */
> > > + if (init_context == DLL_INIT_NORMAL) {
> > > + cycle_cnt = readl_relaxed(ioaddr +
> > > + msm_offset->core_dll_config_2)
> > > + & CORE_FLL_CYCLE_CNT ? 8 : 4;
> > > +
> > > + mclk_freq = DIV_ROUND_CLOSEST_ULL(dll_clock * cycle_cnt, TCXO_FREQ);
> > > +
> > > + if (dll_clock < 100000000) {
> > > + pr_err("%s: %s: Non standard clk freq =%u\n",
> > > + mmc_hostname(mmc), __func__, dll_clock);
> > > + rc = -EINVAL;
> > > + goto out;
> > > + }
> > > +
> > > + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> > > + config = (config & ~(0xFF << 10)) | (mclk_freq << 10);
> >
> > GENMASK, FIELD_PREP?
>
> Sure I will use the suggested macros.
>
> >
> > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> > > + }
> > > + /* wait for 5us before enabling DLL clock */
> > > + udelay(5);
> > > + }
> > > +
> > > + /*
> > > + * Update the lower two bytes of DLL_CONFIG only with
> > > + * HSR values. Since these are the static settings.
> > > + */
> > > + config = (readl_relaxed(ioaddr + msm_offset->core_dll_config));
> > > + config |= (msm_host->dll.dll_config[index] & 0xffff);
> > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > +
> > > + /* Wait for 52us */
> > > + spin_unlock_irqrestore(&host->lock, flags);
> > > + udelay(60);
> > > + spin_lock_irqsave(&host->lock, flags);
> > > +
> > > + /*
> > > + * Write 0 to DLL_RST bit of DLL_CONFIG register
> > > + * and Write 0 to DLL_PDN bit of DLL_CONFIG register.
> > > + */
> > > + config &= ~CORE_DLL_RST;
> > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > +
> > > + config &= ~CORE_DLL_PDN;
> > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > + /* Write 1 to DLL_RST bit of DLL_CONFIG register */
> > > + config |= CORE_DLL_RST;
> > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > +
> > > + /* Write 0 to DLL_RST bit of DLL_CONFIG register */
> > > + config &= ~CORE_DLL_RST;
> > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > +
> > > + /* Set CORE_DLL_CLOCK_DISABLE to 0 */
> > > + if (msm_host->use_14lpp_dll_reset) {
> > > + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> > > + config &= ~CORE_DLL_CLOCK_DISABLE;
> > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> > > + }
> > > +
> > > + /* Set DLL_EN bit to 1. */
> > > + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> > > + config |= CORE_DLL_EN;
> > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > +
> > > + /*
> > > + * Wait for 8000 input clock. Here we calculate the
> > > + * delay from fixed clock freq 192MHz, which turns out 42us.
> > > + */
> > > + spin_unlock_irqrestore(&host->lock, flags);
> > > + udelay(50);
> > > + spin_lock_irqsave(&host->lock, flags);
> > > +
> > > + /* Set CK_OUT_EN bit to 1. */
> > > + config |= CORE_CK_OUT_EN;
> > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > +
> > > + /*
> > > + * Wait until DLL_LOCK bit of DLL_STATUS register
> > > + * becomes '1'.
> > > + */
> > > + while (!(readl_relaxed(ioaddr + msm_offset->core_dll_status) &
> > > + CORE_DLL_LOCK)) {
> > > + /* max. wait for 50us sec for LOCK bit to be set */
> > > + if (--wait_cnt == 0) {
> > > + dev_err(mmc_dev(mmc), "%s: DLL failed to LOCK\n",
> > > + mmc_hostname(mmc));
> > > + rc = -ETIMEDOUT;
> > > + goto out;
> > > + }
> > > + /* wait for 1us before polling again */
> > > + udelay(1);
> > > + }
> > > +
> > > +out:
> > > + if (core_vendor_spec & CORE_CLK_PWRSAVE) {
> > > + /* Reenable PWRSAVE as needed */
> > > + config = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
> > > + config |= CORE_CLK_PWRSAVE;
> > > + writel_relaxed(config, ioaddr + msm_offset->core_vendor_spec);
> > > + }
> > > + spin_unlock_irqrestore(&host->lock, flags);
> > > + return rc;
> > > +}
> > > +
> > > static void msm_hc_select_default(struct sdhci_host *host)
> > > {
> > > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > @@ -900,14 +1131,35 @@ static void sdhci_msm_hc_select_mode(struct sdhci_host *host)
> > > msm_hc_select_default(host);
> > > }
> > > +static int sdhci_msm_init_dll(struct sdhci_host *host, enum dll_init_context init_context)
> > > +{
> > > + unsigned char timing = host->mmc->ios.timing;
> > > + int ret;
> > > +
> > > + if (timing == MMC_TIMING_UHS_SDR104 || timing == MMC_TIMING_MMC_HS400)
> > > + ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS400);
> > > + else
> > > + ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS200);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int sdhci_msm_dll_config(struct sdhci_host *host, enum dll_init_context init_context)
> > > +{
> > > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > > +
> > > + return msm_host->artanis_dll ? sdhci_msm_init_dll(host, init_context) :
> > > + msm_init_cm_dll(host);
> > > +}
> > > +
> > > static int sdhci_msm_cdclp533_calibration(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;
> > > 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__);
> > > @@ -915,7 +1167,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
> > > * Retuning in HS400 (DDR mode) will fail, just reset the
> > > * tuning block and restore the saved tuning phase.
> > > */
> > > - ret = msm_init_cm_dll(host);
> > > + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> > > if (ret)
> > > goto out;
> > > @@ -1003,7 +1255,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
> > > return ret;
> > > }
> > > -static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
> > > +static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host, enum mode index)
> > > {
> > > struct mmc_host *mmc = host->mmc;
> > > u32 dll_status, config, ddr_cfg_offset;
> > > @@ -1014,7 +1266,6 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
> > > sdhci_priv_msm_offset(host);
> > > pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
> > > -
> >
> > Unrelated, please drop.
>
> I will fix it in next patchset.
>
> >
> > > /*
> > > * Currently the core_ddr_config register defaults to desired
> > > * configuration on reset. Currently reprogramming the power on
> > > @@ -1026,7 +1277,11 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
> > > ddr_cfg_offset = msm_offset->core_ddr_config;
> > > else
> > > ddr_cfg_offset = msm_offset->core_ddr_config_old;
> > > - writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
> > > +
> > > + if (msm_host->artanis_dll)
> > > + writel_relaxed(msm_host->dll.ddr_config[index], host->ioaddr + ddr_cfg_offset);
> > > + else
> > > + writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
> > > if (mmc->ios.enhanced_strobe) {
> > > config = readl_relaxed(host->ioaddr +
> > > @@ -1083,11 +1338,10 @@ static int sdhci_msm_hs400_dll_calibration(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;
> > > struct mmc_host *mmc = host->mmc;
> > > - int ret;
> > > u32 config;
> > > - const struct sdhci_msm_offset *msm_offset =
> > > - msm_host->offset;
> > > + int ret;
> > > pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
> > > @@ -1095,7 +1349,8 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
> > > * Retuning in HS400 (DDR mode) will fail, just reset the
> > > * tuning block and restore the saved tuning phase.
> > > */
> > > - ret = msm_init_cm_dll(host);
> > > + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> > > +
> > > if (ret)
> > > goto out;
> > > @@ -1115,7 +1370,7 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
> > > if (msm_host->use_cdclp533)
> > > ret = sdhci_msm_cdclp533_calibration(host);
> > > else
> > > - ret = sdhci_msm_cm_dll_sdc4_calibration(host);
> > > + ret = sdhci_msm_cm_dll_sdc4_calibration(host, HS400);
> > > out:
> > > pr_debug("%s: %s: Exit, ret %d\n", mmc_hostname(host->mmc),
> > > __func__, ret);
> > > @@ -1154,7 +1409,8 @@ static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
> > > return 0;
> > > /* Reset the tuning block */
> > > - ret = msm_init_cm_dll(host);
> > > + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> > > +
> > > if (ret)
> > > return ret;
> > > @@ -1223,7 +1479,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
> > > retry:
> > > /* First of all reset the tuning block */
> > > - rc = msm_init_cm_dll(host);
> > > + rc = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> > > if (rc)
> > > return rc;
> > > @@ -1752,11 +2008,6 @@ static unsigned int sdhci_msm_get_max_clock(struct sdhci_host *host)
> > > return clk_round_rate(core_clk, ULONG_MAX);
> > > }
> > > -static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
> > > -{
> > > - return SDHCI_MSM_MIN_CLOCK;
> > > -}
> > > -
> > > /*
> > > * __sdhci_msm_set_clock - sdhci_msm clock control.
> > > *
> > > @@ -2400,6 +2651,86 @@ static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
> > > return ret;
> > > }
> > > +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
> > > + u32 **bw_vecs, int *len, u32 size)
> > > +{
> > > + struct device_node *np = dev->of_node;
> > > + u32 *arr = NULL;
> > > + int ret = 0;
> > > + size_t sz;
> > > +
> > > + if (!np) {
> > > + ret = -ENODEV;
> > > + goto out;
> > > + }
> > > + if (!of_get_property(np, prop_name, len)) {
> > > + ret = -EINVAL;
> > > + goto out;
> > > + }
> > > + sz = *len = *len / sizeof(*arr);
> >
> > You obviously skipped checkpatch run. Please don't do that.
>
> Before submitting the patchset I have already executed the checkpatch please
> find the output.
> $ ./scripts/checkpatch.pl patch/*
> -----------------------------
> patch/0000-cover-letter.patch
> -----------------------------
> total: 0 errors, 0 warnings, 0 lines checked
>
> patch/0000-cover-letter.patch has no obvious style problems and is ready
> for submission.
> ---------------------------------------------------------------------
> patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch
> ---------------------------------------------------------------------
> total: 0 errors, 0 warnings, 18 lines checked
>
> patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch has
> no obvious style problems and is ready for submission.
> ---------------------------------------------------------------------
> patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch
> ---------------------------------------------------------------------
> total: 0 errors, 0 warnings, 494 lines checked
>
> patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch has
> no obvious style problems and is ready for submission.
Strangely enogh checkpatch.pl doesn't warn about this line, although it
has the check for it:
if ($line =~ /^.\s*$Lval\s*=\s*$Lval\s*=(?!=)/) {
CHK("MULTIPLE_ASSIGNMENTS",
"multiple assignments should be avoided\n" . $herecurr);
}
Running checkpatch.pl --strict will give you more things to fix though.
And anyway, the API not so logical. You pass the size, then you return
the number of elements through the len pointer. Please pass and return
the same thing (e.g. pass the number of elements in the passed array,
return the number of elements retrieved from DT).
>
> >
> > > + if (sz <= 0 || (size > 0 && (sz > size))) {
> > > + dev_err(dev, "%s invalid size\n", prop_name);
> > > + ret = -EINVAL;
> > > + goto out;
> > > + }
> > > +
> > > + arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
> > > + if (!arr) {
> > > + ret = -ENOMEM;
> > > + goto out;
> > > + }
> > > +
> > > + ret = of_property_read_u32_array(np, prop_name, arr, sz);
> > > + if (ret < 0) {
> > > + dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
> > > + goto out;
> > > + }
> > > + *bw_vecs = arr;
> > > +out:
> > > + if (ret)
> > > + *len = 0;
> > > + return ret;
> > > +}
> > > +
> > > +static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
> > > +{
> > > + int dll_table_len, dll_reg_count;
> > > + u32 *dll_table = NULL;
> > > + u32 dll_values[10];
> > > + int ret = 0, i;
> > > +
> > > + if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
> > > + &dll_table, &dll_table_len, 0))
> > > + goto skip_dll;
> >
> > Missing update for the bindings.
>
> I will update in the next patchset.
Please update your internal upstreaming site: bindings changes MUST
always come before the corresponding driver changes. If it is already
documented there, you probably have a demerit for not following the
documented process.
>
> >
> > > +
> > > + dll_reg_count = sizeof(struct sdhci_msm_dll) / sizeof(u32);
> > > +
> > > + if (dll_table_len != dll_reg_count) {
> > > + dev_err(dev, "Number of HSR entries are not matching\n");
> > > + ret = -EINVAL;
> > > + goto skip_dll;
> > > + }
> > > +
> > > + for (i = 0; i < 5; i++) {
> >
> > Magic value 5, replace with ARRAY_SIZE
>
> I will fix in next patchset.
>
> >
> > > + dll_values[2 * i] = dll_table[i];
> > > + dll_values[2 * i + 1] = dll_table[i + 5];
> > > + }
> > > +
> > > + for (i = 0; i < 10; i++)
> > > + dll_table[i] = dll_values[i];
> >
> > So three memory copies to rearrange the order of values? That sounds
> > like a horrible solution.
>
> I will fix in next patchset.
>
> >
> > > +
> > > + memcpy(&msm_host->dll, dll_table, sizeof(struct sdhci_msm_dll));
> > > + msm_host->artanis_dll = true;
> > > +
> > > +skip_dll:
> > > + if (!dll_table) {
> > > + msm_host->artanis_dll = false;
> > > + dev_err(dev, "Failed to get dll hsr settings from dt\n");
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > static int sdhci_msm_probe(struct platform_device *pdev)
> > > {
> > > struct sdhci_host *host;
> > > @@ -2446,6 +2777,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
> > > msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
> > > + if (sdhci_msm_dt_parse_dll_info(&pdev->dev, msm_host))
> > > + goto pltfm_free;
> > > +
> > > ret = sdhci_msm_gcc_reset(&pdev->dev, host);
> > > if (ret)
> > > goto pltfm_free;
> > > --
> > > 2.17.1
> > >
> >
>
--
With best wishes
Dmitry
On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>
>>
>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>> With the current DLL sequence stability issues for data
>>>> transfer seen in HS400 and HS200 modes.
>>>>
>>>> "mmc0: cqhci: error IRQ status: 0x00000000 cmd error -84
>>>> data error 0"
>>>>
>>>> Rectify the DLL programming sequence as per latest hardware
>>>> programming guide and also incorporate support for HS200 and
>>>> HS400 DLL settings using the device tree.
>>>
>>> "foo also bar" usually points out that there should be two separate
>>> commits.
>>
>> Thanks for review. I will split it into two patches.
>>
>>>
>>>>
>>>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>>>> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
>>>> Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
>>>> Signed-off-by: Jun Li <liju@codeaurora.org>
>>>
>>> This is very strange and incorrect.
>>
>> Thanks for review. I will fix the format.
>
> Well. If you write that you will fix the format, may I ask, how or what
> do you plan to fix?
>
I will add Co-developed-by and Signed-off-by for co-authors and add
signed-off-by for author at the last.
>>
>>>
>>>> ---
>>>> drivers/mmc/host/sdhci-msm.c | 372 +++++++++++++++++++++++++++++++++--
>>>> 1 file changed, 353 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>>> index 2a5e588779fc..4ecb362f7f2a 100644
>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>> @@ -28,6 +28,7 @@
>>>> #define CORE_VERSION_MAJOR_SHIFT 28
>>>> #define CORE_VERSION_MAJOR_MASK (0xf << CORE_VERSION_MAJOR_SHIFT)
>>>> #define CORE_VERSION_MINOR_MASK 0xff
>>>> +#define SDHCI_MSM_MIN_V_7FF 0x6e
>>>> #define CORE_MCI_GENERICS 0x70
>>>> #define SWITCHABLE_SIGNALING_VOLTAGE BIT(29)
>>>> @@ -118,7 +119,8 @@
>>>> #define CORE_PWRSAVE_DLL BIT(3)
>>>> #define DDR_CONFIG_POR_VAL 0x80040873
>>>> -
>>>> +#define DLL_CONFIG_3_POR_VAL 0x10
>>>> +#define TCXO_FREQ 19200000
>>>
>>> What about the platforms where TCXO has different frequency?
>>>
>>
>> All emmc targets have 192 Mhz as TCXO freq.
>
> So, it's not a TCXO freq, but some other base freq?
>
It’s a TCXO frequency, this is as per hardware team recommendation.
>>
>>>> #define INVALID_TUNING_PHASE -1
>>>> #define SDHCI_MSM_MIN_CLOCK 400000
>>>> @@ -256,6 +258,19 @@ struct sdhci_msm_variant_info {
>>>> const struct sdhci_msm_offset *offset;
>>>> };
>>>> +/*
>>>> + * DLL registers which needs be programmed with HSR settings.
>>>> + * Add any new register only at the end and don't change the
>>>> + * sequence.
>>>
>>> Why?
>>
>> I will update the comment message in next patchset.
>
> Well, you can respond to a question first. And once something is settled
> you can get that to the commit message. It might save some round-trip
> time.
>
My intention for the comment is that as per Hardware Documents, as part
of DLL sequence DLL registers should be configured first. My above
comment is confusing, will remove it.
>>
>>>
>>>> + */
>>>> +struct sdhci_msm_dll {
>>>> + u32 dll_config[2];
>>>> + u32 dll_config_2[2];
>>>> + u32 dll_config_3[2];
>>>> + u32 dll_usr_ctl[2];
>>>> + u32 ddr_config[2];
>>>> +};
>>>> +
>>>> struct sdhci_msm_host {
>>>> struct platform_device *pdev;
>>>> void __iomem *core_mem; /* MSM SDCC mapped address */
>>>> @@ -264,6 +279,7 @@ struct sdhci_msm_host {
>>>> struct clk *xo_clk; /* TCXO clk needed for FLL feature of cm_dll*/
>>>> /* core, iface, cal and sleep clocks */
>>>> struct clk_bulk_data bulk_clks[4];
>>>> + struct sdhci_msm_dll dll;
>>>> #ifdef CONFIG_MMC_CRYPTO
>>>> struct qcom_ice *ice;
>>>> #endif
>>>> @@ -292,6 +308,17 @@ struct sdhci_msm_host {
>>>> u32 dll_config;
>>>> u32 ddr_config;
>>>> bool vqmmc_enabled;
>>>> + bool artanis_dll;
>>>> +};
>>>> +
>>>> +enum dll_init_context {
>>>> + DLL_INIT_NORMAL,
>>>> + DLL_INIT_FROM_CX_COLLAPSE_EXIT,
>>>> +};
>>>> +
>>>> +enum mode {
>>>> + HS400, // equivalent to SDR104 mode for DLL.
>>>> + HS200, // equivalent to SDR50 mode for DLL.
>>>> };
>>>> static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>>>> @@ -778,6 +805,210 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>>>> return 0;
>>>> }
>>>> +static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
>>>> +{
>>>> + return SDHCI_MSM_MIN_CLOCK;
>>>> +}
>>>
>>> Why??? Why do you need a function to return a static value?
>>>
>>
>> This is just rearrangement of the function. This function already exist,
>> moving here to avoid predeclaration.
>
> Okay.
>
>>>> +
>>>> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
>>>> +{
>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
>>>> + unsigned int sup_clk;
>>>> +
>>>> + if (req_clk < sdhci_msm_get_min_clock(host))
>>>> + return sdhci_msm_get_min_clock(host);
>>>> +
>>>> + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
>>>> +
>>>> + if (host->clock != msm_host->clk_rate)
>>>> + sup_clk = sup_clk / 2;
>>>> +
>>>> + return sup_clk;
>>>
>>> Why?
>>
>> Sorry, I did not understand your question. Can you please explain in detail.
>
> Please explain the maths. You get the rate from the clock, then you
> round it, but it is the rate that has just been returned, so there
> should be no need to round it. And after that there a division by two
> for some reason. So I've asked for an explanation for that code.
>
clk_round_rate is used in case of over clocking issue we can round it to
the usable frequency. Divide by 2 is used as for HS400 the tuning
happens in HS200 mode only so to update the frequency to 192 Mhz.
>>
>>>
>>>> +}
>>>> +
>>>> +/* Initialize the DLL (Programmable Delay Line) */
>>>> +static int sdhci_msm_configure_dll(struct sdhci_host *host, enum dll_init_context
>>>> + init_context, enum mode index)
>>>> +{
>>>> + 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;
>>>> + struct mmc_host *mmc = host->mmc;
>>>> + u32 ddr_cfg_offset, core_vendor_spec, config;
>>>> + void __iomem *ioaddr = host->ioaddr;
>>>> + unsigned long flags, dll_clock;
>>>> + int rc = 0, wait_cnt = 50;
>>>> +
>>>> + dll_clock = sdhci_msm_get_clk_rate(host, host->clock);
>>>> + spin_lock_irqsave(&host->lock, flags);
>>>> +
>>>> + core_vendor_spec = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
>>>> +
>>>> + /*
>>>> + * Always disable PWRSAVE during the DLL power
>>>> + * up regardless of its current setting.
>>>> + */
>>>> + core_vendor_spec &= ~CORE_CLK_PWRSAVE;
>>>> + writel_relaxed(core_vendor_spec, ioaddr + msm_offset->core_vendor_spec);
>>>> +
>>>> + if (msm_host->use_14lpp_dll_reset) {
>>>> + /* Disable CK_OUT */
>>>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>>>> + config &= ~CORE_CK_OUT_EN;
>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>> +
>>>> + /* Disable the DLL clock */
>>>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>>>> + config |= CORE_DLL_CLOCK_DISABLE;
>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>>>> + }
>>>> +
>>>> + /*
>>>> + * Write 1 to DLL_RST bit of DLL_CONFIG register
>>>> + * and Write 1 to DLL_PDN bit of DLL_CONFIG register.
>>>> + */
>>>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>>>> + config |= (CORE_DLL_RST | CORE_DLL_PDN);
>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>> +
>>>> + /*
>>>> + * Configure DLL_CONFIG_3 and USER_CTRL
>>>> + * (Only applicable for 7FF projects).
>>>> + */
>>>> + if (msm_host->core_minor >= SDHCI_MSM_MIN_V_7FF) {
>>>> + writel_relaxed(msm_host->dll.dll_config_3[index],
>>>> + ioaddr + msm_offset->core_dll_config_3);
>>>> + writel_relaxed(msm_host->dll.dll_usr_ctl[index],
>>>> + ioaddr + msm_offset->core_dll_usr_ctl);
>>>> + }
>>>> +
>>>> + /*
>>>> + * Set DDR_CONFIG since step 7 is setting TEST_CTRL that can be skipped.
>>>> + */
>>>> + ddr_cfg_offset = msm_host->updated_ddr_cfg ? msm_offset->core_ddr_config
>>>> + : msm_offset->core_ddr_config_old;
>>>> +
>>>> + config = msm_host->dll.ddr_config[index];
>>>> + writel_relaxed(config, ioaddr + ddr_cfg_offset);
>>>> +
>>>> + /* Set DLL_CONFIG_2 */
>>>> + if (msm_host->use_14lpp_dll_reset) {
>>>> + u32 mclk_freq;
>>>> + int cycle_cnt;
>>>> +
>>>> + /*
>>>> + * Only configure the mclk_freq in normal DLL init
>>>> + * context. If the DLL init is coming from
>>>> + * CX Collapse Exit context, the host->clock may be zero.
>>>> + * The DLL_CONFIG_2 register has already been restored to
>>>> + * proper value prior to getting here.
>>>> + */
>>>> + if (init_context == DLL_INIT_NORMAL) {
>>>> + cycle_cnt = readl_relaxed(ioaddr +
>>>> + msm_offset->core_dll_config_2)
>>>> + & CORE_FLL_CYCLE_CNT ? 8 : 4;
>>>> +
>>>> + mclk_freq = DIV_ROUND_CLOSEST_ULL(dll_clock * cycle_cnt, TCXO_FREQ);
>>>> +
>>>> + if (dll_clock < 100000000) {
>>>> + pr_err("%s: %s: Non standard clk freq =%u\n",
>>>> + mmc_hostname(mmc), __func__, dll_clock);
>>>> + rc = -EINVAL;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>>>> + config = (config & ~(0xFF << 10)) | (mclk_freq << 10);
>>>
>>> GENMASK, FIELD_PREP?
>>
>> Sure I will use the suggested macros.
>>
>>>
>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>>>> + }
>>>> + /* wait for 5us before enabling DLL clock */
>>>> + udelay(5);
>>>> + }
>>>> +
>>>> + /*
>>>> + * Update the lower two bytes of DLL_CONFIG only with
>>>> + * HSR values. Since these are the static settings.
>>>> + */
>>>> + config = (readl_relaxed(ioaddr + msm_offset->core_dll_config));
>>>> + config |= (msm_host->dll.dll_config[index] & 0xffff);
>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>> +
>>>> + /* Wait for 52us */
>>>> + spin_unlock_irqrestore(&host->lock, flags);
>>>> + udelay(60);
>>>> + spin_lock_irqsave(&host->lock, flags);
>>>> +
>>>> + /*
>>>> + * Write 0 to DLL_RST bit of DLL_CONFIG register
>>>> + * and Write 0 to DLL_PDN bit of DLL_CONFIG register.
>>>> + */
>>>> + config &= ~CORE_DLL_RST;
>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>> +
>>>> + config &= ~CORE_DLL_PDN;
>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>> + /* Write 1 to DLL_RST bit of DLL_CONFIG register */
>>>> + config |= CORE_DLL_RST;
>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>> +
>>>> + /* Write 0 to DLL_RST bit of DLL_CONFIG register */
>>>> + config &= ~CORE_DLL_RST;
>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>> +
>>>> + /* Set CORE_DLL_CLOCK_DISABLE to 0 */
>>>> + if (msm_host->use_14lpp_dll_reset) {
>>>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>>>> + config &= ~CORE_DLL_CLOCK_DISABLE;
>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>>>> + }
>>>> +
>>>> + /* Set DLL_EN bit to 1. */
>>>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>>>> + config |= CORE_DLL_EN;
>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>> +
>>>> + /*
>>>> + * Wait for 8000 input clock. Here we calculate the
>>>> + * delay from fixed clock freq 192MHz, which turns out 42us.
>>>> + */
>>>> + spin_unlock_irqrestore(&host->lock, flags);
>>>> + udelay(50);
>>>> + spin_lock_irqsave(&host->lock, flags);
>>>> +
>>>> + /* Set CK_OUT_EN bit to 1. */
>>>> + config |= CORE_CK_OUT_EN;
>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>> +
>>>> + /*
>>>> + * Wait until DLL_LOCK bit of DLL_STATUS register
>>>> + * becomes '1'.
>>>> + */
>>>> + while (!(readl_relaxed(ioaddr + msm_offset->core_dll_status) &
>>>> + CORE_DLL_LOCK)) {
>>>> + /* max. wait for 50us sec for LOCK bit to be set */
>>>> + if (--wait_cnt == 0) {
>>>> + dev_err(mmc_dev(mmc), "%s: DLL failed to LOCK\n",
>>>> + mmc_hostname(mmc));
>>>> + rc = -ETIMEDOUT;
>>>> + goto out;
>>>> + }
>>>> + /* wait for 1us before polling again */
>>>> + udelay(1);
>>>> + }
>>>> +
>>>> +out:
>>>> + if (core_vendor_spec & CORE_CLK_PWRSAVE) {
>>>> + /* Reenable PWRSAVE as needed */
>>>> + config = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
>>>> + config |= CORE_CLK_PWRSAVE;
>>>> + writel_relaxed(config, ioaddr + msm_offset->core_vendor_spec);
>>>> + }
>>>> + spin_unlock_irqrestore(&host->lock, flags);
>>>> + return rc;
>>>> +}
>>>> +
>>>> static void msm_hc_select_default(struct sdhci_host *host)
>>>> {
>>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> @@ -900,14 +1131,35 @@ static void sdhci_msm_hc_select_mode(struct sdhci_host *host)
>>>> msm_hc_select_default(host);
>>>> }
>>>> +static int sdhci_msm_init_dll(struct sdhci_host *host, enum dll_init_context init_context)
>>>> +{
>>>> + unsigned char timing = host->mmc->ios.timing;
>>>> + int ret;
>>>> +
>>>> + if (timing == MMC_TIMING_UHS_SDR104 || timing == MMC_TIMING_MMC_HS400)
>>>> + ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS400);
>>>> + else
>>>> + ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS200);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int sdhci_msm_dll_config(struct sdhci_host *host, enum dll_init_context init_context)
>>>> +{
>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>> +
>>>> + return msm_host->artanis_dll ? sdhci_msm_init_dll(host, init_context) :
>>>> + msm_init_cm_dll(host);
>>>> +}
>>>> +
>>>> static int sdhci_msm_cdclp533_calibration(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;
>>>> 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__);
>>>> @@ -915,7 +1167,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>>>> * Retuning in HS400 (DDR mode) will fail, just reset the
>>>> * tuning block and restore the saved tuning phase.
>>>> */
>>>> - ret = msm_init_cm_dll(host);
>>>> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>>>> if (ret)
>>>> goto out;
>>>> @@ -1003,7 +1255,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>>>> return ret;
>>>> }
>>>> -static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>>>> +static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host, enum mode index)
>>>> {
>>>> struct mmc_host *mmc = host->mmc;
>>>> u32 dll_status, config, ddr_cfg_offset;
>>>> @@ -1014,7 +1266,6 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>>>> sdhci_priv_msm_offset(host);
>>>> pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
>>>> -
>>>
>>> Unrelated, please drop.
>>
>> I will fix it in next patchset.
>>
>>>
>>>> /*
>>>> * Currently the core_ddr_config register defaults to desired
>>>> * configuration on reset. Currently reprogramming the power on
>>>> @@ -1026,7 +1277,11 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>>>> ddr_cfg_offset = msm_offset->core_ddr_config;
>>>> else
>>>> ddr_cfg_offset = msm_offset->core_ddr_config_old;
>>>> - writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
>>>> +
>>>> + if (msm_host->artanis_dll)
>>>> + writel_relaxed(msm_host->dll.ddr_config[index], host->ioaddr + ddr_cfg_offset);
>>>> + else
>>>> + writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
>>>> if (mmc->ios.enhanced_strobe) {
>>>> config = readl_relaxed(host->ioaddr +
>>>> @@ -1083,11 +1338,10 @@ static int sdhci_msm_hs400_dll_calibration(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;
>>>> struct mmc_host *mmc = host->mmc;
>>>> - int ret;
>>>> u32 config;
>>>> - const struct sdhci_msm_offset *msm_offset =
>>>> - msm_host->offset;
>>>> + int ret;
>>>> pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
>>>> @@ -1095,7 +1349,8 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>>>> * Retuning in HS400 (DDR mode) will fail, just reset the
>>>> * tuning block and restore the saved tuning phase.
>>>> */
>>>> - ret = msm_init_cm_dll(host);
>>>> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>>>> +
>>>> if (ret)
>>>> goto out;
>>>> @@ -1115,7 +1370,7 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>>>> if (msm_host->use_cdclp533)
>>>> ret = sdhci_msm_cdclp533_calibration(host);
>>>> else
>>>> - ret = sdhci_msm_cm_dll_sdc4_calibration(host);
>>>> + ret = sdhci_msm_cm_dll_sdc4_calibration(host, HS400);
>>>> out:
>>>> pr_debug("%s: %s: Exit, ret %d\n", mmc_hostname(host->mmc),
>>>> __func__, ret);
>>>> @@ -1154,7 +1409,8 @@ static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
>>>> return 0;
>>>> /* Reset the tuning block */
>>>> - ret = msm_init_cm_dll(host);
>>>> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>>>> +
>>>> if (ret)
>>>> return ret;
>>>> @@ -1223,7 +1479,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>>> retry:
>>>> /* First of all reset the tuning block */
>>>> - rc = msm_init_cm_dll(host);
>>>> + rc = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>>>> if (rc)
>>>> return rc;
>>>> @@ -1752,11 +2008,6 @@ static unsigned int sdhci_msm_get_max_clock(struct sdhci_host *host)
>>>> return clk_round_rate(core_clk, ULONG_MAX);
>>>> }
>>>> -static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
>>>> -{
>>>> - return SDHCI_MSM_MIN_CLOCK;
>>>> -}
>>>> -
>>>> /*
>>>> * __sdhci_msm_set_clock - sdhci_msm clock control.
>>>> *
>>>> @@ -2400,6 +2651,86 @@ static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
>>>> return ret;
>>>> }
>>>> +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
>>>> + u32 **bw_vecs, int *len, u32 size)
>>>> +{
>>>> + struct device_node *np = dev->of_node;
>>>> + u32 *arr = NULL;
>>>> + int ret = 0;
>>>> + size_t sz;
>>>> +
>>>> + if (!np) {
>>>> + ret = -ENODEV;
>>>> + goto out;
>>>> + }
>>>> + if (!of_get_property(np, prop_name, len)) {
>>>> + ret = -EINVAL;
>>>> + goto out;
>>>> + }
>>>> + sz = *len = *len / sizeof(*arr);
>>>
>>> You obviously skipped checkpatch run. Please don't do that.
>>
>> Before submitting the patchset I have already executed the checkpatch please
>> find the output.
>> $ ./scripts/checkpatch.pl patch/*
>> -----------------------------
>> patch/0000-cover-letter.patch
>> -----------------------------
>> total: 0 errors, 0 warnings, 0 lines checked
>>
>> patch/0000-cover-letter.patch has no obvious style problems and is ready
>> for submission.
>> ---------------------------------------------------------------------
>> patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch
>> ---------------------------------------------------------------------
>> total: 0 errors, 0 warnings, 18 lines checked
>>
>> patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch has
>> no obvious style problems and is ready for submission.
>> ---------------------------------------------------------------------
>> patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch
>> ---------------------------------------------------------------------
>> total: 0 errors, 0 warnings, 494 lines checked
>>
>> patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch has
>> no obvious style problems and is ready for submission.
>
> Strangely enogh checkpatch.pl doesn't warn about this line, although it
> has the check for it:
>
> if ($line =~ /^.\s*$Lval\s*=\s*$Lval\s*=(?!=)/) {
> CHK("MULTIPLE_ASSIGNMENTS",
> "multiple assignments should be avoided\n" . $herecurr);
> }
>
> Running checkpatch.pl --strict will give you more things to fix though.
>
Sorry, even with the --strict checkpatch does not throw error or
warning. But I will modify the line to make it simple assignment.
> And anyway, the API not so logical. You pass the size, then you return
> the number of elements through the len pointer. Please pass and return
> the same thing (e.g. pass the number of elements in the passed array,
> return the number of elements retrieved from DT).
>
Thank you for the comment, I will modify the API and remove size input
variable. we can use only data and length.
>>
>>>
>>>> + if (sz <= 0 || (size > 0 && (sz > size))) {
>>>> + dev_err(dev, "%s invalid size\n", prop_name);
>>>> + ret = -EINVAL;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
>>>> + if (!arr) {
>>>> + ret = -ENOMEM;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + ret = of_property_read_u32_array(np, prop_name, arr, sz);
>>>> + if (ret < 0) {
>>>> + dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
>>>> + goto out;
>>>> + }
>>>> + *bw_vecs = arr;
>>>> +out:
>>>> + if (ret)
>>>> + *len = 0;
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
>>>> +{
>>>> + int dll_table_len, dll_reg_count;
>>>> + u32 *dll_table = NULL;
>>>> + u32 dll_values[10];
>>>> + int ret = 0, i;
>>>> +
>>>> + if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
>>>> + &dll_table, &dll_table_len, 0))
>>>> + goto skip_dll;
>>>
>>> Missing update for the bindings.
>>
>> I will update in the next patchset.
>
> Please update your internal upstreaming site: bindings changes MUST
> always come before the corresponding driver changes. If it is already
> documented there, you probably have a demerit for not following the
> documented process.
>
Sure I will push DT binding change as a first patch in a new patch series.
>>
>>>
>>>> +
>>>> + dll_reg_count = sizeof(struct sdhci_msm_dll) / sizeof(u32);
>>>> +
>>>> + if (dll_table_len != dll_reg_count) {
>>>> + dev_err(dev, "Number of HSR entries are not matching\n");
>>>> + ret = -EINVAL;
>>>> + goto skip_dll;
>>>> + }
>>>> +
>>>> + for (i = 0; i < 5; i++) {
>>>
>>> Magic value 5, replace with ARRAY_SIZE
>>
>> I will fix in next patchset.
>>
>>>
>>>> + dll_values[2 * i] = dll_table[i];
>>>> + dll_values[2 * i + 1] = dll_table[i + 5];
>>>> + }
>>>> +
>>>> + for (i = 0; i < 10; i++)
>>>> + dll_table[i] = dll_values[i];
>>>
>>> So three memory copies to rearrange the order of values? That sounds
>>> like a horrible solution.
>>
>> I will fix in next patchset.
>>
>>>
>>>> +
>>>> + memcpy(&msm_host->dll, dll_table, sizeof(struct sdhci_msm_dll));
>>>> + msm_host->artanis_dll = true;
>>>> +
>>>> +skip_dll:
>>>> + if (!dll_table) {
>>>> + msm_host->artanis_dll = false;
>>>> + dev_err(dev, "Failed to get dll hsr settings from dt\n");
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> static int sdhci_msm_probe(struct platform_device *pdev)
>>>> {
>>>> struct sdhci_host *host;
>>>> @@ -2446,6 +2777,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>>> msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
>>>> + if (sdhci_msm_dt_parse_dll_info(&pdev->dev, msm_host))
>>>> + goto pltfm_free;
>>>> +
>>>> ret = sdhci_msm_gcc_reset(&pdev->dev, host);
>>>> if (ret)
>>>> goto pltfm_free;
>>>> --
>>>> 2.17.1
>>>>
>>>
>>
>
On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>
>
> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
> > On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
> > >
> > >
> > > On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
> > > > On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
> > > > > With the current DLL sequence stability issues for data
> > > > > transfer seen in HS400 and HS200 modes.
> > > > >
> > > > > "mmc0: cqhci: error IRQ status: 0x00000000 cmd error -84
> > > > > data error 0"
> > > > >
> > > > > Rectify the DLL programming sequence as per latest hardware
> > > > > programming guide and also incorporate support for HS200 and
> > > > > HS400 DLL settings using the device tree.
> > > >
> > > > "foo also bar" usually points out that there should be two separate
> > > > commits.
> > >
> > > Thanks for review. I will split it into two patches.
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> > > > > Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> > > > > Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
> > > > > Signed-off-by: Jun Li <liju@codeaurora.org>
> > > >
> > > > This is very strange and incorrect.
> > >
> > > Thanks for review. I will fix the format.
> >
> > Well. If you write that you will fix the format, may I ask, how or what
> > do you plan to fix?
> >
>
> I will add Co-developed-by and Signed-off-by for co-authors and add
> signed-off-by for author at the last.
>
> > >
> > > >
> > > > > ---
> > > > > drivers/mmc/host/sdhci-msm.c | 372 +++++++++++++++++++++++++++++++++--
> > > > > 1 file changed, 353 insertions(+), 19 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> > > > > index 2a5e588779fc..4ecb362f7f2a 100644
> > > > > --- a/drivers/mmc/host/sdhci-msm.c
> > > > > +++ b/drivers/mmc/host/sdhci-msm.c
> > > > > @@ -28,6 +28,7 @@
> > > > > #define CORE_VERSION_MAJOR_SHIFT 28
> > > > > #define CORE_VERSION_MAJOR_MASK (0xf << CORE_VERSION_MAJOR_SHIFT)
> > > > > #define CORE_VERSION_MINOR_MASK 0xff
> > > > > +#define SDHCI_MSM_MIN_V_7FF 0x6e
> > > > > #define CORE_MCI_GENERICS 0x70
> > > > > #define SWITCHABLE_SIGNALING_VOLTAGE BIT(29)
> > > > > @@ -118,7 +119,8 @@
> > > > > #define CORE_PWRSAVE_DLL BIT(3)
> > > > > #define DDR_CONFIG_POR_VAL 0x80040873
> > > > > -
> > > > > +#define DLL_CONFIG_3_POR_VAL 0x10
> > > > > +#define TCXO_FREQ 19200000
> > > >
> > > > What about the platforms where TCXO has different frequency?
> > > >
> > >
> > > All emmc targets have 192 Mhz as TCXO freq.
> >
> > So, it's not a TCXO freq, but some other base freq?
> >
>
> It’s a TCXO frequency, this is as per hardware team recommendation.
First of all, it's not 192 MHz. Second, is it an actual XO freq or some
other interim frequency? I'm asking since some platforms use higher
frequencies for the XO.
>
> > >
> > > > > #define INVALID_TUNING_PHASE -1
> > > > > #define SDHCI_MSM_MIN_CLOCK 400000
> > > > > @@ -256,6 +258,19 @@ struct sdhci_msm_variant_info {
> > > > > const struct sdhci_msm_offset *offset;
> > > > > };
> > > > > +/*
> > > > > + * DLL registers which needs be programmed with HSR settings.
> > > > > + * Add any new register only at the end and don't change the
> > > > > + * sequence.
> > > >
> > > > Why?
> > >
> > > I will update the comment message in next patchset.
> >
> > Well, you can respond to a question first. And once something is settled
> > you can get that to the commit message. It might save some round-trip
> > time.
> >
>
> My intention for the comment is that as per Hardware Documents, as part of
> DLL sequence DLL registers should be configured first. My above comment is
> confusing, will remove it.
>
> > >
> > > >
> > > > > + */
> > > > > +struct sdhci_msm_dll {
> > > > > + u32 dll_config[2];
> > > > > + u32 dll_config_2[2];
> > > > > + u32 dll_config_3[2];
> > > > > + u32 dll_usr_ctl[2];
> > > > > + u32 ddr_config[2];
> > > > > +};
> > > > > +
> > > > > struct sdhci_msm_host {
> > > > > struct platform_device *pdev;
> > > > > void __iomem *core_mem; /* MSM SDCC mapped address */
> > > > > @@ -264,6 +279,7 @@ struct sdhci_msm_host {
> > > > > struct clk *xo_clk; /* TCXO clk needed for FLL feature of cm_dll*/
> > > > > /* core, iface, cal and sleep clocks */
> > > > > struct clk_bulk_data bulk_clks[4];
> > > > > + struct sdhci_msm_dll dll;
> > > > > #ifdef CONFIG_MMC_CRYPTO
> > > > > struct qcom_ice *ice;
> > > > > #endif
> > > > > @@ -292,6 +308,17 @@ struct sdhci_msm_host {
> > > > > u32 dll_config;
> > > > > u32 ddr_config;
> > > > > bool vqmmc_enabled;
> > > > > + bool artanis_dll;
> > > > > +};
> > > > > +
> > > > > +enum dll_init_context {
> > > > > + DLL_INIT_NORMAL,
> > > > > + DLL_INIT_FROM_CX_COLLAPSE_EXIT,
> > > > > +};
> > > > > +
> > > > > +enum mode {
> > > > > + HS400, // equivalent to SDR104 mode for DLL.
> > > > > + HS200, // equivalent to SDR50 mode for DLL.
> > > > > };
> > > > > static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> > > > > @@ -778,6 +805,210 @@ static int msm_init_cm_dll(struct sdhci_host *host)
> > > > > return 0;
> > > > > }
> > > > > +static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
> > > > > +{
> > > > > + return SDHCI_MSM_MIN_CLOCK;
> > > > > +}
> > > >
> > > > Why??? Why do you need a function to return a static value?
> > > >
> > >
> > > This is just rearrangement of the function. This function already exist,
> > > moving here to avoid predeclaration.
> >
> > Okay.
> >
> > > > > +
> > > > > +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
> > > > > +{
> > > > > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > > > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > > > > + struct clk *core_clk = msm_host->bulk_clks[0].clk;
> > > > > + unsigned int sup_clk;
> > > > > +
> > > > > + if (req_clk < sdhci_msm_get_min_clock(host))
> > > > > + return sdhci_msm_get_min_clock(host);
> > > > > +
> > > > > + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
> > > > > +
> > > > > + if (host->clock != msm_host->clk_rate)
> > > > > + sup_clk = sup_clk / 2;
> > > > > +
> > > > > + return sup_clk;
> > > >
> > > > Why?
> > >
> > > Sorry, I did not understand your question. Can you please explain in detail.
> >
> > Please explain the maths. You get the rate from the clock, then you
> > round it, but it is the rate that has just been returned, so there
> > should be no need to round it. And after that there a division by two
> > for some reason. So I've asked for an explanation for that code.
> >
>
> clk_round_rate is used in case of over clocking issue we can round it to the
> usable frequency.
If it is a frequency _returned_ by the clock driver, why do you need to
round it? It sounds like that freq should be usable anyway.
> Divide by 2 is used as for HS400 the tuning happens in
> HS200 mode only so to update the frequency to 192 Mhz.
Again, is it really 192 MHz? Or 19.2 MHz?
Also if it is for HS400, then shouldn't /2 be limited to that mode?
>
> > >
> > > >
> > > > > +}
> > > > > +
> > > > > +/* Initialize the DLL (Programmable Delay Line) */
> > > > > +static int sdhci_msm_configure_dll(struct sdhci_host *host, enum dll_init_context
> > > > > + init_context, enum mode index)
> > > > > +{
> > > > > + 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;
> > > > > + struct mmc_host *mmc = host->mmc;
> > > > > + u32 ddr_cfg_offset, core_vendor_spec, config;
> > > > > + void __iomem *ioaddr = host->ioaddr;
> > > > > + unsigned long flags, dll_clock;
> > > > > + int rc = 0, wait_cnt = 50;
> > > > > +
> > > > > + dll_clock = sdhci_msm_get_clk_rate(host, host->clock);
> > > > > + spin_lock_irqsave(&host->lock, flags);
> > > > > +
> > > > > + core_vendor_spec = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
> > > > > +
> > > > > + /*
> > > > > + * Always disable PWRSAVE during the DLL power
> > > > > + * up regardless of its current setting.
> > > > > + */
> > > > > + core_vendor_spec &= ~CORE_CLK_PWRSAVE;
> > > > > + writel_relaxed(core_vendor_spec, ioaddr + msm_offset->core_vendor_spec);
> > > > > +
> > > > > + if (msm_host->use_14lpp_dll_reset) {
> > > > > + /* Disable CK_OUT */
> > > > > + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> > > > > + config &= ~CORE_CK_OUT_EN;
> > > > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > > > +
> > > > > + /* Disable the DLL clock */
> > > > > + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> > > > > + config |= CORE_DLL_CLOCK_DISABLE;
> > > > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> > > > > + }
> > > > > +
> > > > > + /*
> > > > > + * Write 1 to DLL_RST bit of DLL_CONFIG register
> > > > > + * and Write 1 to DLL_PDN bit of DLL_CONFIG register.
> > > > > + */
> > > > > + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> > > > > + config |= (CORE_DLL_RST | CORE_DLL_PDN);
> > > > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > > > +
> > > > > + /*
> > > > > + * Configure DLL_CONFIG_3 and USER_CTRL
> > > > > + * (Only applicable for 7FF projects).
> > > > > + */
> > > > > + if (msm_host->core_minor >= SDHCI_MSM_MIN_V_7FF) {
> > > > > + writel_relaxed(msm_host->dll.dll_config_3[index],
> > > > > + ioaddr + msm_offset->core_dll_config_3);
> > > > > + writel_relaxed(msm_host->dll.dll_usr_ctl[index],
> > > > > + ioaddr + msm_offset->core_dll_usr_ctl);
> > > > > + }
> > > > > +
> > > > > + /*
> > > > > + * Set DDR_CONFIG since step 7 is setting TEST_CTRL that can be skipped.
> > > > > + */
> > > > > + ddr_cfg_offset = msm_host->updated_ddr_cfg ? msm_offset->core_ddr_config
> > > > > + : msm_offset->core_ddr_config_old;
> > > > > +
> > > > > + config = msm_host->dll.ddr_config[index];
> > > > > + writel_relaxed(config, ioaddr + ddr_cfg_offset);
> > > > > +
> > > > > + /* Set DLL_CONFIG_2 */
> > > > > + if (msm_host->use_14lpp_dll_reset) {
> > > > > + u32 mclk_freq;
> > > > > + int cycle_cnt;
> > > > > +
> > > > > + /*
> > > > > + * Only configure the mclk_freq in normal DLL init
> > > > > + * context. If the DLL init is coming from
> > > > > + * CX Collapse Exit context, the host->clock may be zero.
> > > > > + * The DLL_CONFIG_2 register has already been restored to
> > > > > + * proper value prior to getting here.
> > > > > + */
> > > > > + if (init_context == DLL_INIT_NORMAL) {
> > > > > + cycle_cnt = readl_relaxed(ioaddr +
> > > > > + msm_offset->core_dll_config_2)
> > > > > + & CORE_FLL_CYCLE_CNT ? 8 : 4;
> > > > > +
> > > > > + mclk_freq = DIV_ROUND_CLOSEST_ULL(dll_clock * cycle_cnt, TCXO_FREQ);
> > > > > +
> > > > > + if (dll_clock < 100000000) {
> > > > > + pr_err("%s: %s: Non standard clk freq =%u\n",
> > > > > + mmc_hostname(mmc), __func__, dll_clock);
> > > > > + rc = -EINVAL;
> > > > > + goto out;
> > > > > + }
> > > > > +
> > > > > + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> > > > > + config = (config & ~(0xFF << 10)) | (mclk_freq << 10);
> > > >
> > > > GENMASK, FIELD_PREP?
> > >
> > > Sure I will use the suggested macros.
> > >
> > > >
> > > > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> > > > > + }
> > > > > + /* wait for 5us before enabling DLL clock */
> > > > > + udelay(5);
> > > > > + }
> > > > > +
> > > > > + /*
> > > > > + * Update the lower two bytes of DLL_CONFIG only with
> > > > > + * HSR values. Since these are the static settings.
> > > > > + */
> > > > > + config = (readl_relaxed(ioaddr + msm_offset->core_dll_config));
> > > > > + config |= (msm_host->dll.dll_config[index] & 0xffff);
> > > > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > > > +
> > > > > + /* Wait for 52us */
> > > > > + spin_unlock_irqrestore(&host->lock, flags);
> > > > > + udelay(60);
> > > > > + spin_lock_irqsave(&host->lock, flags);
> > > > > +
> > > > > + /*
> > > > > + * Write 0 to DLL_RST bit of DLL_CONFIG register
> > > > > + * and Write 0 to DLL_PDN bit of DLL_CONFIG register.
> > > > > + */
> > > > > + config &= ~CORE_DLL_RST;
> > > > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > > > +
> > > > > + config &= ~CORE_DLL_PDN;
> > > > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > > > + /* Write 1 to DLL_RST bit of DLL_CONFIG register */
> > > > > + config |= CORE_DLL_RST;
> > > > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > > > +
> > > > > + /* Write 0 to DLL_RST bit of DLL_CONFIG register */
> > > > > + config &= ~CORE_DLL_RST;
> > > > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > > > +
> > > > > + /* Set CORE_DLL_CLOCK_DISABLE to 0 */
> > > > > + if (msm_host->use_14lpp_dll_reset) {
> > > > > + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> > > > > + config &= ~CORE_DLL_CLOCK_DISABLE;
> > > > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> > > > > + }
> > > > > +
> > > > > + /* Set DLL_EN bit to 1. */
> > > > > + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> > > > > + config |= CORE_DLL_EN;
> > > > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > > > +
> > > > > + /*
> > > > > + * Wait for 8000 input clock. Here we calculate the
> > > > > + * delay from fixed clock freq 192MHz, which turns out 42us.
> > > > > + */
> > > > > + spin_unlock_irqrestore(&host->lock, flags);
> > > > > + udelay(50);
> > > > > + spin_lock_irqsave(&host->lock, flags);
> > > > > +
> > > > > + /* Set CK_OUT_EN bit to 1. */
> > > > > + config |= CORE_CK_OUT_EN;
> > > > > + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> > > > > +
> > > > > + /*
> > > > > + * Wait until DLL_LOCK bit of DLL_STATUS register
> > > > > + * becomes '1'.
> > > > > + */
> > > > > + while (!(readl_relaxed(ioaddr + msm_offset->core_dll_status) &
> > > > > + CORE_DLL_LOCK)) {
> > > > > + /* max. wait for 50us sec for LOCK bit to be set */
> > > > > + if (--wait_cnt == 0) {
> > > > > + dev_err(mmc_dev(mmc), "%s: DLL failed to LOCK\n",
> > > > > + mmc_hostname(mmc));
> > > > > + rc = -ETIMEDOUT;
> > > > > + goto out;
> > > > > + }
> > > > > + /* wait for 1us before polling again */
> > > > > + udelay(1);
> > > > > + }
> > > > > +
> > > > > +out:
> > > > > + if (core_vendor_spec & CORE_CLK_PWRSAVE) {
> > > > > + /* Reenable PWRSAVE as needed */
> > > > > + config = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
> > > > > + config |= CORE_CLK_PWRSAVE;
> > > > > + writel_relaxed(config, ioaddr + msm_offset->core_vendor_spec);
> > > > > + }
> > > > > + spin_unlock_irqrestore(&host->lock, flags);
> > > > > + return rc;
> > > > > +}
> > > > > +
> > > > > static void msm_hc_select_default(struct sdhci_host *host)
> > > > > {
> > > > > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > > > @@ -900,14 +1131,35 @@ static void sdhci_msm_hc_select_mode(struct sdhci_host *host)
> > > > > msm_hc_select_default(host);
> > > > > }
> > > > > +static int sdhci_msm_init_dll(struct sdhci_host *host, enum dll_init_context init_context)
> > > > > +{
> > > > > + unsigned char timing = host->mmc->ios.timing;
> > > > > + int ret;
> > > > > +
> > > > > + if (timing == MMC_TIMING_UHS_SDR104 || timing == MMC_TIMING_MMC_HS400)
> > > > > + ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS400);
> > > > > + else
> > > > > + ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS200);
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static int sdhci_msm_dll_config(struct sdhci_host *host, enum dll_init_context init_context)
> > > > > +{
> > > > > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > > > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > > > > +
> > > > > + return msm_host->artanis_dll ? sdhci_msm_init_dll(host, init_context) :
> > > > > + msm_init_cm_dll(host);
> > > > > +}
> > > > > +
> > > > > static int sdhci_msm_cdclp533_calibration(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;
> > > > > 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__);
> > > > > @@ -915,7 +1167,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
> > > > > * Retuning in HS400 (DDR mode) will fail, just reset the
> > > > > * tuning block and restore the saved tuning phase.
> > > > > */
> > > > > - ret = msm_init_cm_dll(host);
> > > > > + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> > > > > if (ret)
> > > > > goto out;
> > > > > @@ -1003,7 +1255,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
> > > > > return ret;
> > > > > }
> > > > > -static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
> > > > > +static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host, enum mode index)
> > > > > {
> > > > > struct mmc_host *mmc = host->mmc;
> > > > > u32 dll_status, config, ddr_cfg_offset;
> > > > > @@ -1014,7 +1266,6 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
> > > > > sdhci_priv_msm_offset(host);
> > > > > pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
> > > > > -
> > > >
> > > > Unrelated, please drop.
> > >
> > > I will fix it in next patchset.
> > >
> > > >
> > > > > /*
> > > > > * Currently the core_ddr_config register defaults to desired
> > > > > * configuration on reset. Currently reprogramming the power on
> > > > > @@ -1026,7 +1277,11 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
> > > > > ddr_cfg_offset = msm_offset->core_ddr_config;
> > > > > else
> > > > > ddr_cfg_offset = msm_offset->core_ddr_config_old;
> > > > > - writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
> > > > > +
> > > > > + if (msm_host->artanis_dll)
> > > > > + writel_relaxed(msm_host->dll.ddr_config[index], host->ioaddr + ddr_cfg_offset);
> > > > > + else
> > > > > + writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
> > > > > if (mmc->ios.enhanced_strobe) {
> > > > > config = readl_relaxed(host->ioaddr +
> > > > > @@ -1083,11 +1338,10 @@ static int sdhci_msm_hs400_dll_calibration(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;
> > > > > struct mmc_host *mmc = host->mmc;
> > > > > - int ret;
> > > > > u32 config;
> > > > > - const struct sdhci_msm_offset *msm_offset =
> > > > > - msm_host->offset;
> > > > > + int ret;
> > > > > pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
> > > > > @@ -1095,7 +1349,8 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
> > > > > * Retuning in HS400 (DDR mode) will fail, just reset the
> > > > > * tuning block and restore the saved tuning phase.
> > > > > */
> > > > > - ret = msm_init_cm_dll(host);
> > > > > + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> > > > > +
> > > > > if (ret)
> > > > > goto out;
> > > > > @@ -1115,7 +1370,7 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
> > > > > if (msm_host->use_cdclp533)
> > > > > ret = sdhci_msm_cdclp533_calibration(host);
> > > > > else
> > > > > - ret = sdhci_msm_cm_dll_sdc4_calibration(host);
> > > > > + ret = sdhci_msm_cm_dll_sdc4_calibration(host, HS400);
> > > > > out:
> > > > > pr_debug("%s: %s: Exit, ret %d\n", mmc_hostname(host->mmc),
> > > > > __func__, ret);
> > > > > @@ -1154,7 +1409,8 @@ static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
> > > > > return 0;
> > > > > /* Reset the tuning block */
> > > > > - ret = msm_init_cm_dll(host);
> > > > > + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> > > > > +
> > > > > if (ret)
> > > > > return ret;
> > > > > @@ -1223,7 +1479,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
> > > > > retry:
> > > > > /* First of all reset the tuning block */
> > > > > - rc = msm_init_cm_dll(host);
> > > > > + rc = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> > > > > if (rc)
> > > > > return rc;
> > > > > @@ -1752,11 +2008,6 @@ static unsigned int sdhci_msm_get_max_clock(struct sdhci_host *host)
> > > > > return clk_round_rate(core_clk, ULONG_MAX);
> > > > > }
> > > > > -static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
> > > > > -{
> > > > > - return SDHCI_MSM_MIN_CLOCK;
> > > > > -}
> > > > > -
> > > > > /*
> > > > > * __sdhci_msm_set_clock - sdhci_msm clock control.
> > > > > *
> > > > > @@ -2400,6 +2651,86 @@ static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
> > > > > return ret;
> > > > > }
> > > > > +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
> > > > > + u32 **bw_vecs, int *len, u32 size)
> > > > > +{
> > > > > + struct device_node *np = dev->of_node;
> > > > > + u32 *arr = NULL;
> > > > > + int ret = 0;
> > > > > + size_t sz;
> > > > > +
> > > > > + if (!np) {
> > > > > + ret = -ENODEV;
> > > > > + goto out;
> > > > > + }
> > > > > + if (!of_get_property(np, prop_name, len)) {
> > > > > + ret = -EINVAL;
> > > > > + goto out;
> > > > > + }
> > > > > + sz = *len = *len / sizeof(*arr);
> > > >
> > > > You obviously skipped checkpatch run. Please don't do that.
> > >
> > > Before submitting the patchset I have already executed the checkpatch please
> > > find the output.
> > > $ ./scripts/checkpatch.pl patch/*
> > > -----------------------------
> > > patch/0000-cover-letter.patch
> > > -----------------------------
> > > total: 0 errors, 0 warnings, 0 lines checked
> > >
> > > patch/0000-cover-letter.patch has no obvious style problems and is ready
> > > for submission.
> > > ---------------------------------------------------------------------
> > > patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch
> > > ---------------------------------------------------------------------
> > > total: 0 errors, 0 warnings, 18 lines checked
> > >
> > > patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch has
> > > no obvious style problems and is ready for submission.
> > > ---------------------------------------------------------------------
> > > patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch
> > > ---------------------------------------------------------------------
> > > total: 0 errors, 0 warnings, 494 lines checked
> > >
> > > patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch has
> > > no obvious style problems and is ready for submission.
> >
> > Strangely enogh checkpatch.pl doesn't warn about this line, although it
> > has the check for it:
> >
> > if ($line =~ /^.\s*$Lval\s*=\s*$Lval\s*=(?!=)/) {
> > CHK("MULTIPLE_ASSIGNMENTS",
> > "multiple assignments should be avoided\n" . $herecurr);
> > }
> >
> > Running checkpatch.pl --strict will give you more things to fix though.
> >
>
> Sorry, even with the --strict checkpatch does not throw error or warning.
> But I will modify the line to make it simple assignment.
And all other checkpatch issues too, please.
>
> > And anyway, the API not so logical. You pass the size, then you return
> > the number of elements through the len pointer. Please pass and return
> > the same thing (e.g. pass the number of elements in the passed array,
> > return the number of elements retrieved from DT).
> >
>
> Thank you for the comment, I will modify the API and remove size input
> variable. we can use only data and length.
>
>
> > >
> > > >
> > > > > + if (sz <= 0 || (size > 0 && (sz > size))) {
> > > > > + dev_err(dev, "%s invalid size\n", prop_name);
> > > > > + ret = -EINVAL;
> > > > > + goto out;
> > > > > + }
> > > > > +
> > > > > + arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
> > > > > + if (!arr) {
> > > > > + ret = -ENOMEM;
> > > > > + goto out;
> > > > > + }
> > > > > +
> > > > > + ret = of_property_read_u32_array(np, prop_name, arr, sz);
> > > > > + if (ret < 0) {
> > > > > + dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
> > > > > + goto out;
> > > > > + }
> > > > > + *bw_vecs = arr;
> > > > > +out:
> > > > > + if (ret)
> > > > > + *len = 0;
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
> > > > > +{
> > > > > + int dll_table_len, dll_reg_count;
> > > > > + u32 *dll_table = NULL;
> > > > > + u32 dll_values[10];
> > > > > + int ret = 0, i;
> > > > > +
> > > > > + if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
> > > > > + &dll_table, &dll_table_len, 0))
> > > > > + goto skip_dll;
> > > >
> > > > Missing update for the bindings.
> > >
> > > I will update in the next patchset.
> >
> > Please update your internal upstreaming site: bindings changes MUST
> > always come before the corresponding driver changes. If it is already
> > documented there, you probably have a demerit for not following the
> > documented process.
> >
>
> Sure I will push DT binding change as a first patch in a new patch series.
>
> > >
> > > >
> > > > > +
> > > > > + dll_reg_count = sizeof(struct sdhci_msm_dll) / sizeof(u32);
> > > > > +
> > > > > + if (dll_table_len != dll_reg_count) {
> > > > > + dev_err(dev, "Number of HSR entries are not matching\n");
> > > > > + ret = -EINVAL;
> > > > > + goto skip_dll;
> > > > > + }
> > > > > +
> > > > > + for (i = 0; i < 5; i++) {
> > > >
> > > > Magic value 5, replace with ARRAY_SIZE
> > >
> > > I will fix in next patchset.
> > >
> > > >
> > > > > + dll_values[2 * i] = dll_table[i];
> > > > > + dll_values[2 * i + 1] = dll_table[i + 5];
> > > > > + }
> > > > > +
> > > > > + for (i = 0; i < 10; i++)
> > > > > + dll_table[i] = dll_values[i];
> > > >
> > > > So three memory copies to rearrange the order of values? That sounds
> > > > like a horrible solution.
> > >
> > > I will fix in next patchset.
> > >
> > > >
> > > > > +
> > > > > + memcpy(&msm_host->dll, dll_table, sizeof(struct sdhci_msm_dll));
> > > > > + msm_host->artanis_dll = true;
> > > > > +
> > > > > +skip_dll:
> > > > > + if (!dll_table) {
> > > > > + msm_host->artanis_dll = false;
> > > > > + dev_err(dev, "Failed to get dll hsr settings from dt\n");
> > > > > + }
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > static int sdhci_msm_probe(struct platform_device *pdev)
> > > > > {
> > > > > struct sdhci_host *host;
> > > > > @@ -2446,6 +2777,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
> > > > > msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
> > > > > + if (sdhci_msm_dt_parse_dll_info(&pdev->dev, msm_host))
> > > > > + goto pltfm_free;
> > > > > +
> > > > > ret = sdhci_msm_gcc_reset(&pdev->dev, host);
> > > > > if (ret)
> > > > > goto pltfm_free;
> > > > > --
> > > > > 2.17.1
> > > > >
> > > >
> > >
> >
>
--
With best wishes
Dmitry
On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>
>>
>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>
>>>>
>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>>>> With the current DLL sequence stability issues for data
>>>>>> transfer seen in HS400 and HS200 modes.
>>>>>>
>>>>>> "mmc0: cqhci: error IRQ status: 0x00000000 cmd error -84
>>>>>> data error 0"
>>>>>>
>>>>>> Rectify the DLL programming sequence as per latest hardware
>>>>>> programming guide and also incorporate support for HS200 and
>>>>>> HS400 DLL settings using the device tree.
>>>>>
>>>>> "foo also bar" usually points out that there should be two separate
>>>>> commits.
>>>>
>>>> Thanks for review. I will split it into two patches.
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>>>>>> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
>>>>>> Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
>>>>>> Signed-off-by: Jun Li <liju@codeaurora.org>
>>>>>
>>>>> This is very strange and incorrect.
>>>>
>>>> Thanks for review. I will fix the format.
>>>
>>> Well. If you write that you will fix the format, may I ask, how or what
>>> do you plan to fix?
>>>
>>
>> I will add Co-developed-by and Signed-off-by for co-authors and add
>> signed-off-by for author at the last.
>>
>>>>
>>>>>
>>>>>> ---
>>>>>> drivers/mmc/host/sdhci-msm.c | 372 +++++++++++++++++++++++++++++++++--
>>>>>> 1 file changed, 353 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>>>>> index 2a5e588779fc..4ecb362f7f2a 100644
>>>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>>>> @@ -28,6 +28,7 @@
>>>>>> #define CORE_VERSION_MAJOR_SHIFT 28
>>>>>> #define CORE_VERSION_MAJOR_MASK (0xf << CORE_VERSION_MAJOR_SHIFT)
>>>>>> #define CORE_VERSION_MINOR_MASK 0xff
>>>>>> +#define SDHCI_MSM_MIN_V_7FF 0x6e
>>>>>> #define CORE_MCI_GENERICS 0x70
>>>>>> #define SWITCHABLE_SIGNALING_VOLTAGE BIT(29)
>>>>>> @@ -118,7 +119,8 @@
>>>>>> #define CORE_PWRSAVE_DLL BIT(3)
>>>>>> #define DDR_CONFIG_POR_VAL 0x80040873
>>>>>> -
>>>>>> +#define DLL_CONFIG_3_POR_VAL 0x10
>>>>>> +#define TCXO_FREQ 19200000
>>>>>
>>>>> What about the platforms where TCXO has different frequency?
>>>>>
>>>>
>>>> All emmc targets have 192 Mhz as TCXO freq.
>>>
>>> So, it's not a TCXO freq, but some other base freq?
>>>
>>
>> It’s a TCXO frequency, this is as per hardware team recommendation.
>
> First of all, it's not 192 MHz. Second, is it an actual XO freq or some
> other interim frequency? I'm asking since some platforms use higher
> frequencies for the XO.
>
Yes its 19.2 Mhz sorry for the confusion.
>>
>>>>
>>>>>> #define INVALID_TUNING_PHASE -1
>>>>>> #define SDHCI_MSM_MIN_CLOCK 400000
>>>>>> @@ -256,6 +258,19 @@ struct sdhci_msm_variant_info {
>>>>>> const struct sdhci_msm_offset *offset;
>>>>>> };
>>>>>> +/*
>>>>>> + * DLL registers which needs be programmed with HSR settings.
>>>>>> + * Add any new register only at the end and don't change the
>>>>>> + * sequence.
>>>>>
>>>>> Why?
>>>>
>>>> I will update the comment message in next patchset.
>>>
>>> Well, you can respond to a question first. And once something is settled
>>> you can get that to the commit message. It might save some round-trip
>>> time.
>>>
>>
>> My intention for the comment is that as per Hardware Documents, as part of
>> DLL sequence DLL registers should be configured first. My above comment is
>> confusing, will remove it.
>>
>>>>
>>>>>
>>>>>> + */
>>>>>> +struct sdhci_msm_dll {
>>>>>> + u32 dll_config[2];
>>>>>> + u32 dll_config_2[2];
>>>>>> + u32 dll_config_3[2];
>>>>>> + u32 dll_usr_ctl[2];
>>>>>> + u32 ddr_config[2];
>>>>>> +};
>>>>>> +
>>>>>> struct sdhci_msm_host {
>>>>>> struct platform_device *pdev;
>>>>>> void __iomem *core_mem; /* MSM SDCC mapped address */
>>>>>> @@ -264,6 +279,7 @@ struct sdhci_msm_host {
>>>>>> struct clk *xo_clk; /* TCXO clk needed for FLL feature of cm_dll*/
>>>>>> /* core, iface, cal and sleep clocks */
>>>>>> struct clk_bulk_data bulk_clks[4];
>>>>>> + struct sdhci_msm_dll dll;
>>>>>> #ifdef CONFIG_MMC_CRYPTO
>>>>>> struct qcom_ice *ice;
>>>>>> #endif
>>>>>> @@ -292,6 +308,17 @@ struct sdhci_msm_host {
>>>>>> u32 dll_config;
>>>>>> u32 ddr_config;
>>>>>> bool vqmmc_enabled;
>>>>>> + bool artanis_dll;
>>>>>> +};
>>>>>> +
>>>>>> +enum dll_init_context {
>>>>>> + DLL_INIT_NORMAL,
>>>>>> + DLL_INIT_FROM_CX_COLLAPSE_EXIT,
>>>>>> +};
>>>>>> +
>>>>>> +enum mode {
>>>>>> + HS400, // equivalent to SDR104 mode for DLL.
>>>>>> + HS200, // equivalent to SDR50 mode for DLL.
>>>>>> };
>>>>>> static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>>>>>> @@ -778,6 +805,210 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>>>>>> return 0;
>>>>>> }
>>>>>> +static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
>>>>>> +{
>>>>>> + return SDHCI_MSM_MIN_CLOCK;
>>>>>> +}
>>>>>
>>>>> Why??? Why do you need a function to return a static value?
>>>>>
>>>>
>>>> This is just rearrangement of the function. This function already exist,
>>>> moving here to avoid predeclaration.
>>>
>>> Okay.
>>>
>>>>>> +
>>>>>> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
>>>>>> +{
>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>>> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
>>>>>> + unsigned int sup_clk;
>>>>>> +
>>>>>> + if (req_clk < sdhci_msm_get_min_clock(host))
>>>>>> + return sdhci_msm_get_min_clock(host);
>>>>>> +
>>>>>> + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>> +
>>>>>> + if (host->clock != msm_host->clk_rate)
>>>>>> + sup_clk = sup_clk / 2;
>>>>>> +
>>>>>> + return sup_clk;
>>>>>
>>>>> Why?
>>>>
>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>
>>> Please explain the maths. You get the rate from the clock, then you
>>> round it, but it is the rate that has just been returned, so there
>>> should be no need to round it. And after that there a division by two
>>> for some reason. So I've asked for an explanation for that code.
>>>
>>
>> clk_round_rate is used in case of over clocking issue we can round it to the
>> usable frequency.
>
> If it is a frequency _returned_ by the clock driver, why do you need to
> round it? It sounds like that freq should be usable anyway.
>
I agree, rounding will be taken care by clock driver. Will remove in my
next patch.
>> Divide by 2 is used as for HS400 the tuning happens in
>> HS200 mode only so to update the frequency to 192 Mhz.
>
> Again, is it really 192 MHz? Or 19.2 MHz?
> Also if it is for HS400, then shouldn't /2 be limited to that mode?
>
Yes, It is 192 MHz.
As part of eMMC Init, driver will try to init with the best mode
supported by controller and device. In this case it is HS400 mode, But
as part of HS400 mode, we perform Tuning in HS200 mode only where we
need to configure half of the clock.
>>
>>>>
>>>>>
>>>>>> +}
>>>>>> +
>>>>>> +/* Initialize the DLL (Programmable Delay Line) */
>>>>>> +static int sdhci_msm_configure_dll(struct sdhci_host *host, enum dll_init_context
>>>>>> + init_context, enum mode index)
>>>>>> +{
>>>>>> + 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;
>>>>>> + struct mmc_host *mmc = host->mmc;
>>>>>> + u32 ddr_cfg_offset, core_vendor_spec, config;
>>>>>> + void __iomem *ioaddr = host->ioaddr;
>>>>>> + unsigned long flags, dll_clock;
>>>>>> + int rc = 0, wait_cnt = 50;
>>>>>> +
>>>>>> + dll_clock = sdhci_msm_get_clk_rate(host, host->clock);
>>>>>> + spin_lock_irqsave(&host->lock, flags);
>>>>>> +
>>>>>> + core_vendor_spec = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
>>>>>> +
>>>>>> + /*
>>>>>> + * Always disable PWRSAVE during the DLL power
>>>>>> + * up regardless of its current setting.
>>>>>> + */
>>>>>> + core_vendor_spec &= ~CORE_CLK_PWRSAVE;
>>>>>> + writel_relaxed(core_vendor_spec, ioaddr + msm_offset->core_vendor_spec);
>>>>>> +
>>>>>> + if (msm_host->use_14lpp_dll_reset) {
>>>>>> + /* Disable CK_OUT */
>>>>>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>>>>>> + config &= ~CORE_CK_OUT_EN;
>>>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>>>> +
>>>>>> + /* Disable the DLL clock */
>>>>>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>>>>>> + config |= CORE_DLL_CLOCK_DISABLE;
>>>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>>>>>> + }
>>>>>> +
>>>>>> + /*
>>>>>> + * Write 1 to DLL_RST bit of DLL_CONFIG register
>>>>>> + * and Write 1 to DLL_PDN bit of DLL_CONFIG register.
>>>>>> + */
>>>>>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>>>>>> + config |= (CORE_DLL_RST | CORE_DLL_PDN);
>>>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>>>> +
>>>>>> + /*
>>>>>> + * Configure DLL_CONFIG_3 and USER_CTRL
>>>>>> + * (Only applicable for 7FF projects).
>>>>>> + */
>>>>>> + if (msm_host->core_minor >= SDHCI_MSM_MIN_V_7FF) {
>>>>>> + writel_relaxed(msm_host->dll.dll_config_3[index],
>>>>>> + ioaddr + msm_offset->core_dll_config_3);
>>>>>> + writel_relaxed(msm_host->dll.dll_usr_ctl[index],
>>>>>> + ioaddr + msm_offset->core_dll_usr_ctl);
>>>>>> + }
>>>>>> +
>>>>>> + /*
>>>>>> + * Set DDR_CONFIG since step 7 is setting TEST_CTRL that can be skipped.
>>>>>> + */
>>>>>> + ddr_cfg_offset = msm_host->updated_ddr_cfg ? msm_offset->core_ddr_config
>>>>>> + : msm_offset->core_ddr_config_old;
>>>>>> +
>>>>>> + config = msm_host->dll.ddr_config[index];
>>>>>> + writel_relaxed(config, ioaddr + ddr_cfg_offset);
>>>>>> +
>>>>>> + /* Set DLL_CONFIG_2 */
>>>>>> + if (msm_host->use_14lpp_dll_reset) {
>>>>>> + u32 mclk_freq;
>>>>>> + int cycle_cnt;
>>>>>> +
>>>>>> + /*
>>>>>> + * Only configure the mclk_freq in normal DLL init
>>>>>> + * context. If the DLL init is coming from
>>>>>> + * CX Collapse Exit context, the host->clock may be zero.
>>>>>> + * The DLL_CONFIG_2 register has already been restored to
>>>>>> + * proper value prior to getting here.
>>>>>> + */
>>>>>> + if (init_context == DLL_INIT_NORMAL) {
>>>>>> + cycle_cnt = readl_relaxed(ioaddr +
>>>>>> + msm_offset->core_dll_config_2)
>>>>>> + & CORE_FLL_CYCLE_CNT ? 8 : 4;
>>>>>> +
>>>>>> + mclk_freq = DIV_ROUND_CLOSEST_ULL(dll_clock * cycle_cnt, TCXO_FREQ);
>>>>>> +
>>>>>> + if (dll_clock < 100000000) {
>>>>>> + pr_err("%s: %s: Non standard clk freq =%u\n",
>>>>>> + mmc_hostname(mmc), __func__, dll_clock);
>>>>>> + rc = -EINVAL;
>>>>>> + goto out;
>>>>>> + }
>>>>>> +
>>>>>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>>>>>> + config = (config & ~(0xFF << 10)) | (mclk_freq << 10);
>>>>>
>>>>> GENMASK, FIELD_PREP?
>>>>
>>>> Sure I will use the suggested macros.
>>>>
>>>>>
>>>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>>>>>> + }
>>>>>> + /* wait for 5us before enabling DLL clock */
>>>>>> + udelay(5);
>>>>>> + }
>>>>>> +
>>>>>> + /*
>>>>>> + * Update the lower two bytes of DLL_CONFIG only with
>>>>>> + * HSR values. Since these are the static settings.
>>>>>> + */
>>>>>> + config = (readl_relaxed(ioaddr + msm_offset->core_dll_config));
>>>>>> + config |= (msm_host->dll.dll_config[index] & 0xffff);
>>>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>>>> +
>>>>>> + /* Wait for 52us */
>>>>>> + spin_unlock_irqrestore(&host->lock, flags);
>>>>>> + udelay(60);
>>>>>> + spin_lock_irqsave(&host->lock, flags);
>>>>>> +
>>>>>> + /*
>>>>>> + * Write 0 to DLL_RST bit of DLL_CONFIG register
>>>>>> + * and Write 0 to DLL_PDN bit of DLL_CONFIG register.
>>>>>> + */
>>>>>> + config &= ~CORE_DLL_RST;
>>>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>>>> +
>>>>>> + config &= ~CORE_DLL_PDN;
>>>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>>>> + /* Write 1 to DLL_RST bit of DLL_CONFIG register */
>>>>>> + config |= CORE_DLL_RST;
>>>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>>>> +
>>>>>> + /* Write 0 to DLL_RST bit of DLL_CONFIG register */
>>>>>> + config &= ~CORE_DLL_RST;
>>>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>>>> +
>>>>>> + /* Set CORE_DLL_CLOCK_DISABLE to 0 */
>>>>>> + if (msm_host->use_14lpp_dll_reset) {
>>>>>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>>>>>> + config &= ~CORE_DLL_CLOCK_DISABLE;
>>>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>>>>>> + }
>>>>>> +
>>>>>> + /* Set DLL_EN bit to 1. */
>>>>>> + config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>>>>>> + config |= CORE_DLL_EN;
>>>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>>>> +
>>>>>> + /*
>>>>>> + * Wait for 8000 input clock. Here we calculate the
>>>>>> + * delay from fixed clock freq 192MHz, which turns out 42us.
>>>>>> + */
>>>>>> + spin_unlock_irqrestore(&host->lock, flags);
>>>>>> + udelay(50);
>>>>>> + spin_lock_irqsave(&host->lock, flags);
>>>>>> +
>>>>>> + /* Set CK_OUT_EN bit to 1. */
>>>>>> + config |= CORE_CK_OUT_EN;
>>>>>> + writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>>>>>> +
>>>>>> + /*
>>>>>> + * Wait until DLL_LOCK bit of DLL_STATUS register
>>>>>> + * becomes '1'.
>>>>>> + */
>>>>>> + while (!(readl_relaxed(ioaddr + msm_offset->core_dll_status) &
>>>>>> + CORE_DLL_LOCK)) {
>>>>>> + /* max. wait for 50us sec for LOCK bit to be set */
>>>>>> + if (--wait_cnt == 0) {
>>>>>> + dev_err(mmc_dev(mmc), "%s: DLL failed to LOCK\n",
>>>>>> + mmc_hostname(mmc));
>>>>>> + rc = -ETIMEDOUT;
>>>>>> + goto out;
>>>>>> + }
>>>>>> + /* wait for 1us before polling again */
>>>>>> + udelay(1);
>>>>>> + }
>>>>>> +
>>>>>> +out:
>>>>>> + if (core_vendor_spec & CORE_CLK_PWRSAVE) {
>>>>>> + /* Reenable PWRSAVE as needed */
>>>>>> + config = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
>>>>>> + config |= CORE_CLK_PWRSAVE;
>>>>>> + writel_relaxed(config, ioaddr + msm_offset->core_vendor_spec);
>>>>>> + }
>>>>>> + spin_unlock_irqrestore(&host->lock, flags);
>>>>>> + return rc;
>>>>>> +}
>>>>>> +
>>>>>> static void msm_hc_select_default(struct sdhci_host *host)
>>>>>> {
>>>>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>> @@ -900,14 +1131,35 @@ static void sdhci_msm_hc_select_mode(struct sdhci_host *host)
>>>>>> msm_hc_select_default(host);
>>>>>> }
>>>>>> +static int sdhci_msm_init_dll(struct sdhci_host *host, enum dll_init_context init_context)
>>>>>> +{
>>>>>> + unsigned char timing = host->mmc->ios.timing;
>>>>>> + int ret;
>>>>>> +
>>>>>> + if (timing == MMC_TIMING_UHS_SDR104 || timing == MMC_TIMING_MMC_HS400)
>>>>>> + ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS400);
>>>>>> + else
>>>>>> + ret = sdhci_msm_configure_dll(host, DLL_INIT_NORMAL, HS200);
>>>>>> +
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int sdhci_msm_dll_config(struct sdhci_host *host, enum dll_init_context init_context)
>>>>>> +{
>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>>> +
>>>>>> + return msm_host->artanis_dll ? sdhci_msm_init_dll(host, init_context) :
>>>>>> + msm_init_cm_dll(host);
>>>>>> +}
>>>>>> +
>>>>>> static int sdhci_msm_cdclp533_calibration(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;
>>>>>> 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__);
>>>>>> @@ -915,7 +1167,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>>>>>> * Retuning in HS400 (DDR mode) will fail, just reset the
>>>>>> * tuning block and restore the saved tuning phase.
>>>>>> */
>>>>>> - ret = msm_init_cm_dll(host);
>>>>>> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>>>>>> if (ret)
>>>>>> goto out;
>>>>>> @@ -1003,7 +1255,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>>>>>> return ret;
>>>>>> }
>>>>>> -static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>>>>>> +static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host, enum mode index)
>>>>>> {
>>>>>> struct mmc_host *mmc = host->mmc;
>>>>>> u32 dll_status, config, ddr_cfg_offset;
>>>>>> @@ -1014,7 +1266,6 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>>>>>> sdhci_priv_msm_offset(host);
>>>>>> pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
>>>>>> -
>>>>>
>>>>> Unrelated, please drop.
>>>>
>>>> I will fix it in next patchset.
>>>>
>>>>>
>>>>>> /*
>>>>>> * Currently the core_ddr_config register defaults to desired
>>>>>> * configuration on reset. Currently reprogramming the power on
>>>>>> @@ -1026,7 +1277,11 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>>>>>> ddr_cfg_offset = msm_offset->core_ddr_config;
>>>>>> else
>>>>>> ddr_cfg_offset = msm_offset->core_ddr_config_old;
>>>>>> - writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
>>>>>> +
>>>>>> + if (msm_host->artanis_dll)
>>>>>> + writel_relaxed(msm_host->dll.ddr_config[index], host->ioaddr + ddr_cfg_offset);
>>>>>> + else
>>>>>> + writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
>>>>>> if (mmc->ios.enhanced_strobe) {
>>>>>> config = readl_relaxed(host->ioaddr +
>>>>>> @@ -1083,11 +1338,10 @@ static int sdhci_msm_hs400_dll_calibration(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;
>>>>>> struct mmc_host *mmc = host->mmc;
>>>>>> - int ret;
>>>>>> u32 config;
>>>>>> - const struct sdhci_msm_offset *msm_offset =
>>>>>> - msm_host->offset;
>>>>>> + int ret;
>>>>>> pr_debug("%s: %s: Enter\n", mmc_hostname(host->mmc), __func__);
>>>>>> @@ -1095,7 +1349,8 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>>>>>> * Retuning in HS400 (DDR mode) will fail, just reset the
>>>>>> * tuning block and restore the saved tuning phase.
>>>>>> */
>>>>>> - ret = msm_init_cm_dll(host);
>>>>>> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>>>>>> +
>>>>>> if (ret)
>>>>>> goto out;
>>>>>> @@ -1115,7 +1370,7 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>>>>>> if (msm_host->use_cdclp533)
>>>>>> ret = sdhci_msm_cdclp533_calibration(host);
>>>>>> else
>>>>>> - ret = sdhci_msm_cm_dll_sdc4_calibration(host);
>>>>>> + ret = sdhci_msm_cm_dll_sdc4_calibration(host, HS400);
>>>>>> out:
>>>>>> pr_debug("%s: %s: Exit, ret %d\n", mmc_hostname(host->mmc),
>>>>>> __func__, ret);
>>>>>> @@ -1154,7 +1409,8 @@ static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
>>>>>> return 0;
>>>>>> /* Reset the tuning block */
>>>>>> - ret = msm_init_cm_dll(host);
>>>>>> + ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>>>>>> +
>>>>>> if (ret)
>>>>>> return ret;
>>>>>> @@ -1223,7 +1479,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>>>>> retry:
>>>>>> /* First of all reset the tuning block */
>>>>>> - rc = msm_init_cm_dll(host);
>>>>>> + rc = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>>>>>> if (rc)
>>>>>> return rc;
>>>>>> @@ -1752,11 +2008,6 @@ static unsigned int sdhci_msm_get_max_clock(struct sdhci_host *host)
>>>>>> return clk_round_rate(core_clk, ULONG_MAX);
>>>>>> }
>>>>>> -static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
>>>>>> -{
>>>>>> - return SDHCI_MSM_MIN_CLOCK;
>>>>>> -}
>>>>>> -
>>>>>> /*
>>>>>> * __sdhci_msm_set_clock - sdhci_msm clock control.
>>>>>> *
>>>>>> @@ -2400,6 +2651,86 @@ static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
>>>>>> return ret;
>>>>>> }
>>>>>> +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
>>>>>> + u32 **bw_vecs, int *len, u32 size)
>>>>>> +{
>>>>>> + struct device_node *np = dev->of_node;
>>>>>> + u32 *arr = NULL;
>>>>>> + int ret = 0;
>>>>>> + size_t sz;
>>>>>> +
>>>>>> + if (!np) {
>>>>>> + ret = -ENODEV;
>>>>>> + goto out;
>>>>>> + }
>>>>>> + if (!of_get_property(np, prop_name, len)) {
>>>>>> + ret = -EINVAL;
>>>>>> + goto out;
>>>>>> + }
>>>>>> + sz = *len = *len / sizeof(*arr);
>>>>>
>>>>> You obviously skipped checkpatch run. Please don't do that.
>>>>
>>>> Before submitting the patchset I have already executed the checkpatch please
>>>> find the output.
>>>> $ ./scripts/checkpatch.pl patch/*
>>>> -----------------------------
>>>> patch/0000-cover-letter.patch
>>>> -----------------------------
>>>> total: 0 errors, 0 warnings, 0 lines checked
>>>>
>>>> patch/0000-cover-letter.patch has no obvious style problems and is ready
>>>> for submission.
>>>> ---------------------------------------------------------------------
>>>> patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch
>>>> ---------------------------------------------------------------------
>>>> total: 0 errors, 0 warnings, 18 lines checked
>>>>
>>>> patch/0001-mmc-sdhci-msm-Add-core_major-minor-to-msm_host-struc.patch has
>>>> no obvious style problems and is ready for submission.
>>>> ---------------------------------------------------------------------
>>>> patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch
>>>> ---------------------------------------------------------------------
>>>> total: 0 errors, 0 warnings, 494 lines checked
>>>>
>>>> patch/0002-mmc-sdhci-msm-Rectify-DLL-programming-sequence-for-S.patch has
>>>> no obvious style problems and is ready for submission.
>>>
>>> Strangely enogh checkpatch.pl doesn't warn about this line, although it
>>> has the check for it:
>>>
>>> if ($line =~ /^.\s*$Lval\s*=\s*$Lval\s*=(?!=)/) {
>>> CHK("MULTIPLE_ASSIGNMENTS",
>>> "multiple assignments should be avoided\n" . $herecurr);
>>> }
>>>
>>> Running checkpatch.pl --strict will give you more things to fix though.
>>>
>>
>> Sorry, even with the --strict checkpatch does not throw error or warning.
>> But I will modify the line to make it simple assignment.
>
> And all other checkpatch issues too, please.
>
Sure resolved.
>>
>>> And anyway, the API not so logical. You pass the size, then you return
>>> the number of elements through the len pointer. Please pass and return
>>> the same thing (e.g. pass the number of elements in the passed array,
>>> return the number of elements retrieved from DT).
>>>
>>
>> Thank you for the comment, I will modify the API and remove size input
>> variable. we can use only data and length.
>>
>>
>>>>
>>>>>
>>>>>> + if (sz <= 0 || (size > 0 && (sz > size))) {
>>>>>> + dev_err(dev, "%s invalid size\n", prop_name);
>>>>>> + ret = -EINVAL;
>>>>>> + goto out;
>>>>>> + }
>>>>>> +
>>>>>> + arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
>>>>>> + if (!arr) {
>>>>>> + ret = -ENOMEM;
>>>>>> + goto out;
>>>>>> + }
>>>>>> +
>>>>>> + ret = of_property_read_u32_array(np, prop_name, arr, sz);
>>>>>> + if (ret < 0) {
>>>>>> + dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
>>>>>> + goto out;
>>>>>> + }
>>>>>> + *bw_vecs = arr;
>>>>>> +out:
>>>>>> + if (ret)
>>>>>> + *len = 0;
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
>>>>>> +{
>>>>>> + int dll_table_len, dll_reg_count;
>>>>>> + u32 *dll_table = NULL;
>>>>>> + u32 dll_values[10];
>>>>>> + int ret = 0, i;
>>>>>> +
>>>>>> + if (sdhci_msm_dt_get_array(dev, "qcom,dll-hsr-list",
>>>>>> + &dll_table, &dll_table_len, 0))
>>>>>> + goto skip_dll;
>>>>>
>>>>> Missing update for the bindings.
>>>>
>>>> I will update in the next patchset.
>>>
>>> Please update your internal upstreaming site: bindings changes MUST
>>> always come before the corresponding driver changes. If it is already
>>> documented there, you probably have a demerit for not following the
>>> documented process.
>>>
>>
>> Sure I will push DT binding change as a first patch in a new patch series.
>>
>>>>
>>>>>
>>>>>> +
>>>>>> + dll_reg_count = sizeof(struct sdhci_msm_dll) / sizeof(u32);
>>>>>> +
>>>>>> + if (dll_table_len != dll_reg_count) {
>>>>>> + dev_err(dev, "Number of HSR entries are not matching\n");
>>>>>> + ret = -EINVAL;
>>>>>> + goto skip_dll;
>>>>>> + }
>>>>>> +
>>>>>> + for (i = 0; i < 5; i++) {
>>>>>
>>>>> Magic value 5, replace with ARRAY_SIZE
>>>>
>>>> I will fix in next patchset.
>>>>
>>>>>
>>>>>> + dll_values[2 * i] = dll_table[i];
>>>>>> + dll_values[2 * i + 1] = dll_table[i + 5];
>>>>>> + }
>>>>>> +
>>>>>> + for (i = 0; i < 10; i++)
>>>>>> + dll_table[i] = dll_values[i];
>>>>>
>>>>> So three memory copies to rearrange the order of values? That sounds
>>>>> like a horrible solution.
>>>>
>>>> I will fix in next patchset.
>>>>
>>>>>
>>>>>> +
>>>>>> + memcpy(&msm_host->dll, dll_table, sizeof(struct sdhci_msm_dll));
>>>>>> + msm_host->artanis_dll = true;
>>>>>> +
>>>>>> +skip_dll:
>>>>>> + if (!dll_table) {
>>>>>> + msm_host->artanis_dll = false;
>>>>>> + dev_err(dev, "Failed to get dll hsr settings from dt\n");
>>>>>> + }
>>>>>> +
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> static int sdhci_msm_probe(struct platform_device *pdev)
>>>>>> {
>>>>>> struct sdhci_host *host;
>>>>>> @@ -2446,6 +2777,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>>>>> msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
>>>>>> + if (sdhci_msm_dt_parse_dll_info(&pdev->dev, msm_host))
>>>>>> + goto pltfm_free;
>>>>>> +
>>>>>> ret = sdhci_msm_gcc_reset(&pdev->dev, host);
>>>>>> if (ret)
>>>>>> goto pltfm_free;
>>>>>> --
>>>>>> 2.17.1
>>>>>>
>>>>>
>>>>
>>>
>>
>
On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
>
>
> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
> > On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
> > >
> > >
> > > On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
> > > > On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
> > > > >
> > > > >
> > > > > On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
> > > > > > On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
> > > > > > > +
> > > > > > > +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
> > > > > > > +{
> > > > > > > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > > > > > + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > > > > > > + struct clk *core_clk = msm_host->bulk_clks[0].clk;
> > > > > > > + unsigned int sup_clk;
> > > > > > > +
> > > > > > > + if (req_clk < sdhci_msm_get_min_clock(host))
> > > > > > > + return sdhci_msm_get_min_clock(host);
> > > > > > > +
> > > > > > > + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
> > > > > > > +
> > > > > > > + if (host->clock != msm_host->clk_rate)
> > > > > > > + sup_clk = sup_clk / 2;
> > > > > > > +
> > > > > > > + return sup_clk;
> > > > > >
> > > > > > Why?
> > > > >
> > > > > Sorry, I did not understand your question. Can you please explain in detail.
> > > >
> > > > Please explain the maths. You get the rate from the clock, then you
> > > > round it, but it is the rate that has just been returned, so there
> > > > should be no need to round it. And after that there a division by two
> > > > for some reason. So I've asked for an explanation for that code.
> > > >
> > >
> > > clk_round_rate is used in case of over clocking issue we can round it to the
> > > usable frequency.
> >
> > If it is a frequency _returned_ by the clock driver, why do you need to
> > round it? It sounds like that freq should be usable anyway.
> >
>
> I agree, rounding will be taken care by clock driver. Will remove in my next
> patch.
>
> > > Divide by 2 is used as for HS400 the tuning happens in
> > > HS200 mode only so to update the frequency to 192 Mhz.
> >
> > Again, is it really 192 MHz? Or 19.2 MHz?
> > Also if it is for HS400, then shouldn't /2 be limited to that mode?
> >
>
> Yes, It is 192 MHz.
Good, thanks for the confirmation.
> As part of eMMC Init, driver will try to init with the best mode supported
> by controller and device. In this case it is HS400 mode, But as part of
> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
> half of the clock.
This isn't an answer to the question. Let me rephrase it for you: if the
/2 is only used for HS400, why should it be attempted in all other
modes? Please limit the /2 just to HS400.
--
With best wishes
Dmitry
On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>>>>>> +
>>>>>>>> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
>>>>>>>> +{
>>>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>>>>> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
>>>>>>>> + unsigned int sup_clk;
>>>>>>>> +
>>>>>>>> + if (req_clk < sdhci_msm_get_min_clock(host))
>>>>>>>> + return sdhci_msm_get_min_clock(host);
>>>>>>>> +
>>>>>>>> + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>>>> +
>>>>>>>> + if (host->clock != msm_host->clk_rate)
>>>>>>>> + sup_clk = sup_clk / 2;
>>>>>>>> +
>>>>>>>> + return sup_clk;
>>>>>>> Why?
>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>>> Please explain the maths. You get the rate from the clock, then you
>>>>> round it, but it is the rate that has just been returned, so there
>>>>> should be no need to round it. And after that there a division by two
>>>>> for some reason. So I've asked for an explanation for that code.
>>>>>
>>>> clk_round_rate is used in case of over clocking issue we can round it to the
>>>> usable frequency.
>>> If it is a frequency _returned_ by the clock driver, why do you need to
>>> round it? It sounds like that freq should be usable anyway.
>>>
>> I agree, rounding will be taken care by clock driver. Will remove in my next
>> patch.
>>
>>>> Divide by 2 is used as for HS400 the tuning happens in
>>>> HS200 mode only so to update the frequency to 192 Mhz.
>>> Again, is it really 192 MHz? Or 19.2 MHz?
>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
>>>
>> Yes, It is 192 MHz.
> Good, thanks for the confirmation.
>
>> As part of eMMC Init, driver will try to init with the best mode supported
>> by controller and device. In this case it is HS400 mode, But as part of
>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
>> half of the clock.
> This isn't an answer to the question. Let me rephrase it for you: if the
> /2 is only used for HS400, why should it be attempted in all other
> modes? Please limit the /2 just to HS400.
Hi Dmitry,
like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
hs200 tuning and hs400 selection. In this case we must divide clk by 2
to get 192MHz and we find this as host->clock wont be equal to
msm_host->clk_rate. Now if we go for only HS200 mode supported card, there
the supported clock value would be 192Mhz itself and we need to pass
clk freq as 192MHz itself, hence division by 2 wont be needed, that is
achieved there as host->clock would be equal to msm_host->clk_rate. Hence
no other check is needed here.
sorry for it took time to update as I was gathering all this data.
since Sachin have already pushed patchset #3, and if this explanation
helps, let me know if we can continue on patchset #3.
Thanks,
Ram
On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
>
> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
> > On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
> >> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
> >>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
> >>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
> >>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
> >>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
> >>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
> >>>>>>>> +
> >>>>>>>> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
> >>>>>>>> +{
> >>>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >>>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> >>>>>>>> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
> >>>>>>>> + unsigned int sup_clk;
> >>>>>>>> +
> >>>>>>>> + if (req_clk < sdhci_msm_get_min_clock(host))
> >>>>>>>> + return sdhci_msm_get_min_clock(host);
> >>>>>>>> +
> >>>>>>>> + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
> >>>>>>>> +
> >>>>>>>> + if (host->clock != msm_host->clk_rate)
> >>>>>>>> + sup_clk = sup_clk / 2;
> >>>>>>>> +
> >>>>>>>> + return sup_clk;
> >>>>>>> Why?
> >>>>>> Sorry, I did not understand your question. Can you please explain in detail.
> >>>>> Please explain the maths. You get the rate from the clock, then you
> >>>>> round it, but it is the rate that has just been returned, so there
> >>>>> should be no need to round it. And after that there a division by two
> >>>>> for some reason. So I've asked for an explanation for that code.
> >>>>>
> >>>> clk_round_rate is used in case of over clocking issue we can round it to the
> >>>> usable frequency.
> >>> If it is a frequency _returned_ by the clock driver, why do you need to
> >>> round it? It sounds like that freq should be usable anyway.
> >>>
> >> I agree, rounding will be taken care by clock driver. Will remove in my next
> >> patch.
> >>
> >>>> Divide by 2 is used as for HS400 the tuning happens in
> >>>> HS200 mode only so to update the frequency to 192 Mhz.
> >>> Again, is it really 192 MHz? Or 19.2 MHz?
> >>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
> >>>
> >> Yes, It is 192 MHz.
> > Good, thanks for the confirmation.
> >
> >> As part of eMMC Init, driver will try to init with the best mode supported
> >> by controller and device. In this case it is HS400 mode, But as part of
> >> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
> >> half of the clock.
> > This isn't an answer to the question. Let me rephrase it for you: if the
> > /2 is only used for HS400, why should it be attempted in all other
> > modes? Please limit the /2 just to HS400.
>
> Hi Dmitry,
>
> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
> hs200 tuning and hs400 selection. In this case we must divide clk by 2
> to get 192MHz and we find this as host->clock wont be equal to
> msm_host->clk_rate.
What are host->clock and msm_host->clk_rate at this point?
What is the host->flags value? See sdhci_msm_hc_select_mode().
> Now if we go for only HS200 mode supported card, there
> the supported clock value would be 192Mhz itself and we need to pass
> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
> no other check is needed here.
Please think about the cause, not about the symptom. Clocks being
unequal is a result of some other checks being performed by the driver.
Please use those checks too.
>
> sorry for it took time to update as I was gathering all this data.
6 months? Well, that's a nice time to "gather all this data".
> since Sachin have already pushed patchset #3, and if this explanation
> helps, let me know if we can continue on patchset #3.
>
> Thanks,
> Ram
>
--
With best wishes
Dmitry
On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>>>>>>>> +
>>>>>>>>>> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
>>>>>>>>>> +{
>>>>>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>>>>>>> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
>>>>>>>>>> + unsigned int sup_clk;
>>>>>>>>>> +
>>>>>>>>>> + if (req_clk < sdhci_msm_get_min_clock(host))
>>>>>>>>>> + return sdhci_msm_get_min_clock(host);
>>>>>>>>>> +
>>>>>>>>>> + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>>>>>> +
>>>>>>>>>> + if (host->clock != msm_host->clk_rate)
>>>>>>>>>> + sup_clk = sup_clk / 2;
>>>>>>>>>> +
>>>>>>>>>> + return sup_clk;
>>>>>>>>> Why?
>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>>>>> Please explain the maths. You get the rate from the clock, then you
>>>>>>> round it, but it is the rate that has just been returned, so there
>>>>>>> should be no need to round it. And after that there a division by two
>>>>>>> for some reason. So I've asked for an explanation for that code.
>>>>>>>
>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
>>>>>> usable frequency.
>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
>>>>> round it? It sounds like that freq should be usable anyway.
>>>>>
>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
>>>> patch.
>>>>
>>>>>> Divide by 2 is used as for HS400 the tuning happens in
>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
>>>>>
>>>> Yes, It is 192 MHz.
>>> Good, thanks for the confirmation.
>>>
>>>> As part of eMMC Init, driver will try to init with the best mode supported
>>>> by controller and device. In this case it is HS400 mode, But as part of
>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
>>>> half of the clock.
>>> This isn't an answer to the question. Let me rephrase it for you: if the
>>> /2 is only used for HS400, why should it be attempted in all other
>>> modes? Please limit the /2 just to HS400.
>> Hi Dmitry,
>>
>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
>> to get 192MHz and we find this as host->clock wont be equal to
>> msm_host->clk_rate.
> What are host->clock and msm_host->clk_rate at this point?
> What is the host->flags value? See sdhci_msm_hc_select_mode().
There are 2 paths which are traced to this function when card initializes
in HS400 mode, please consider below call stack in 2 paths
sdhci_msm_configure_dll
sdhci_msm_dll_config
sdhci_msm_execute_tuning
mmc_execute_tuning
mmc_init_card
_mmc_resume
mmc_runtime_resume
with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
and host->flags as 0x90c6.
and
sdhci_msm_configure_dll
sdhci_msm_dll_config
sdhci_msm_set_uhs_signaling
sdhci_set_ios
mmc_set_clock
mmc_set_bus_speed
mmc_select_hs400
mmc_init_card
_mmc_resume
mmc_runtime_resume
with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
and host->flags as 0x90c6 which are same as 1st.
Now if card is initialized in HS200 mode only below is the call stack
sdhci_msm_configure_dll
sdhci_msm_dll_config
sdhci_msm_execute_tuning
mmc_execute_tuning
mmc_init_card
_mmc_resume
mmc_runtime_resume
with values of host->clock as 200000000 & msm_host-clk_rate as 200000000
and host->flags as 0x90c6.
now if you see the host->flags value, its same across the modes, and if
I am getting it right from the pointed out function
sdhci_msm_hc_select_mode(), your suggestion seems to be using the check
host->flags & SDHCI_HS400_TUNING which is bit[13], but in above dumped
host->flags SDHCI_HS400_TUNING bit is not set where we are using the /2.
and the reason is, this bit is getting cleared in sdhci_msm_execute_tuning()
before sdhci_msm_dll_config() call.
so this /2, is eventually called only for HS400 mode.
Thanks,
Ram
>
>> Now if we go for only HS200 mode supported card, there
>> the supported clock value would be 192Mhz itself and we need to pass
>> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
>> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
>> no other check is needed here.
> Please think about the cause, not about the symptom. Clocks being
> unequal is a result of some other checks being performed by the driver.
> Please use those checks too.
>
>> sorry for it took time to update as I was gathering all this data.
> 6 months? Well, that's a nice time to "gather all this data".
Took it up from sachin last month but still its a long gap.
Thanks for helping revive.
>
>> since Sachin have already pushed patchset #3, and if this explanation
>> helps, let me know if we can continue on patchset #3.
>>
>> Thanks,
>> Ram
>>
On 31/07/2025 14:46, Ram Prakash Gupta wrote:
>
> On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
>> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
>>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
>>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
>>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
>>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>>>>>>>>> +
>>>>>>>>>>> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
>>>>>>>>>>> +{
>>>>>>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>>>>>>>> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
>>>>>>>>>>> + unsigned int sup_clk;
>>>>>>>>>>> +
>>>>>>>>>>> + if (req_clk < sdhci_msm_get_min_clock(host))
>>>>>>>>>>> + return sdhci_msm_get_min_clock(host);
>>>>>>>>>>> +
>>>>>>>>>>> + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>>>>>>> +
>>>>>>>>>>> + if (host->clock != msm_host->clk_rate)
>>>>>>>>>>> + sup_clk = sup_clk / 2;
>>>>>>>>>>> +
>>>>>>>>>>> + return sup_clk;
>>>>>>>>>> Why?
>>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>>>>>> Please explain the maths. You get the rate from the clock, then you
>>>>>>>> round it, but it is the rate that has just been returned, so there
>>>>>>>> should be no need to round it. And after that there a division by two
>>>>>>>> for some reason. So I've asked for an explanation for that code.
>>>>>>>>
>>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
>>>>>>> usable frequency.
>>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
>>>>>> round it? It sounds like that freq should be usable anyway.
>>>>>>
>>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
>>>>> patch.
>>>>>
>>>>>>> Divide by 2 is used as for HS400 the tuning happens in
>>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
>>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
>>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
>>>>>>
>>>>> Yes, It is 192 MHz.
>>>> Good, thanks for the confirmation.
>>>>
>>>>> As part of eMMC Init, driver will try to init with the best mode supported
>>>>> by controller and device. In this case it is HS400 mode, But as part of
>>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
>>>>> half of the clock.
>>>> This isn't an answer to the question. Let me rephrase it for you: if the
>>>> /2 is only used for HS400, why should it be attempted in all other
>>>> modes? Please limit the /2 just to HS400.
>>> Hi Dmitry,
>>>
>>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
>>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
>>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
>>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
>>> to get 192MHz and we find this as host->clock wont be equal to
>>> msm_host->clk_rate.
>> What are host->clock and msm_host->clk_rate at this point?
>> What is the host->flags value? See sdhci_msm_hc_select_mode().
>
> There are 2 paths which are traced to this function when card initializes
> in HS400 mode, please consider below call stack in 2 paths
>
> sdhci_msm_configure_dll
> sdhci_msm_dll_config
> sdhci_msm_execute_tuning
> mmc_execute_tuning
> mmc_init_card
> _mmc_resume
> mmc_runtime_resume
>
> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
Please check the rates explicitly in the code rather than just checking
that they are not equal.
> and host->flags as 0x90c6.
>
> and
>
> sdhci_msm_configure_dll
> sdhci_msm_dll_config
> sdhci_msm_set_uhs_signaling
> sdhci_set_ios
> mmc_set_clock
> mmc_set_bus_speed
> mmc_select_hs400
> mmc_init_card
> _mmc_resume
> mmc_runtime_resume
>
> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
> and host->flags as 0x90c6 which are same as 1st.
>
> Now if card is initialized in HS200 mode only below is the call stack
>
> sdhci_msm_configure_dll
> sdhci_msm_dll_config
> sdhci_msm_execute_tuning
> mmc_execute_tuning
> mmc_init_card
> _mmc_resume
> mmc_runtime_resume
>
> with values of host->clock as 200000000 & msm_host-clk_rate as 200000000
> and host->flags as 0x90c6.
>
> now if you see the host->flags value, its same across the modes, and if
> I am getting it right from the pointed out function
> sdhci_msm_hc_select_mode(), your suggestion seems to be using the check
> host->flags & SDHCI_HS400_TUNING which is bit[13], but in above dumped
> host->flags SDHCI_HS400_TUNING bit is not set where we are using the /2.
>
> and the reason is, this bit is getting cleared in sdhci_msm_execute_tuning()
> before sdhci_msm_dll_config() call.
>
> so this /2, is eventually called only for HS400 mode.
>
> Thanks,
> Ram
>
>>
>>> Now if we go for only HS200 mode supported card, there
>>> the supported clock value would be 192Mhz itself and we need to pass
>>> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
>>> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
>>> no other check is needed here.
>> Please think about the cause, not about the symptom. Clocks being
>> unequal is a result of some other checks being performed by the driver.
>> Please use those checks too.
>>
>>> sorry for it took time to update as I was gathering all this data.
>> 6 months? Well, that's a nice time to "gather all this data".
>
> Took it up from sachin last month but still its a long gap.
> Thanks for helping revive.
>
>>
>>> since Sachin have already pushed patchset #3, and if this explanation
>>> helps, let me know if we can continue on patchset #3.
>>>
>>> Thanks,
>>> Ram
>>>
--
With best wishes
Dmitry
On 7/31/2025 7:49 PM, Dmitry Baryshkov wrote:
> On 31/07/2025 14:46, Ram Prakash Gupta wrote:
>>
>> On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
>>> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
>>>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
>>>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
>>>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
>>>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>>>>>>>>>> +
>>>>>>>>>>>> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
>>>>>>>>>>>> +{
>>>>>>>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>>>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>>>>>>>>> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
>>>>>>>>>>>> + unsigned int sup_clk;
>>>>>>>>>>>> +
>>>>>>>>>>>> + if (req_clk < sdhci_msm_get_min_clock(host))
>>>>>>>>>>>> + return sdhci_msm_get_min_clock(host);
>>>>>>>>>>>> +
>>>>>>>>>>>> + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>>>>>>>> +
>>>>>>>>>>>> + if (host->clock != msm_host->clk_rate)
>>>>>>>>>>>> + sup_clk = sup_clk / 2;
>>>>>>>>>>>> +
>>>>>>>>>>>> + return sup_clk;
>>>>>>>>>>> Why?
>>>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>>>>>>> Please explain the maths. You get the rate from the clock, then you
>>>>>>>>> round it, but it is the rate that has just been returned, so there
>>>>>>>>> should be no need to round it. And after that there a division by two
>>>>>>>>> for some reason. So I've asked for an explanation for that code.
>>>>>>>>>
>>>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
>>>>>>>> usable frequency.
>>>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
>>>>>>> round it? It sounds like that freq should be usable anyway.
>>>>>>>
>>>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
>>>>>> patch.
>>>>>>
>>>>>>>> Divide by 2 is used as for HS400 the tuning happens in
>>>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
>>>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
>>>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
>>>>>>>
>>>>>> Yes, It is 192 MHz.
>>>>> Good, thanks for the confirmation.
>>>>>
>>>>>> As part of eMMC Init, driver will try to init with the best mode supported
>>>>>> by controller and device. In this case it is HS400 mode, But as part of
>>>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
>>>>>> half of the clock.
>>>>> This isn't an answer to the question. Let me rephrase it for you: if the
>>>>> /2 is only used for HS400, why should it be attempted in all other
>>>>> modes? Please limit the /2 just to HS400.
>>>> Hi Dmitry,
>>>>
>>>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
>>>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
>>>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
>>>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
>>>> to get 192MHz and we find this as host->clock wont be equal to
>>>> msm_host->clk_rate.
>>> What are host->clock and msm_host->clk_rate at this point?
>>> What is the host->flags value? See sdhci_msm_hc_select_mode().
>>
>> There are 2 paths which are traced to this function when card initializes
>> in HS400 mode, please consider below call stack in 2 paths
>>
>> sdhci_msm_configure_dll
>> sdhci_msm_dll_config
>> sdhci_msm_execute_tuning
>> mmc_execute_tuning
>> mmc_init_card
>> _mmc_resume
>> mmc_runtime_resume
>>
>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
>
> Please check the rates explicitly in the code rather than just checking that they are not equal.
in function msm_get_clock_mult_for_bus_mode(), clk multiplier returns 2, with HS400
DDR52 and DDR50 modes which is called from sdhci_msm_set_clock() and
sdhci_msm_execute_tuning. And in sdhci_msm_execute_tuning(), we are calling
sdhci_msm_dll_config() when SDHCI_HS400_TUNING is set and this flag is cleared
immediately after return. And sdhci_msm_dll_config() is called after that.
Now when the card is supporting HS400 mode, then from mmc_hs200_tuning(),
sdhci_prepare_hs400_tuning is getting called, and there SDHCI_HS400_TUNING
flag is set, and clock set is multiplying the clk rate by 2 in below call stack
msm_set_clock_rate_for_bus_mode
sdhci_msm_execute_tuning
mmc_execute_tuning
mmc_init_card
so this clk rate is doubling only with HS400 mode selection and while setting up
dll in HS400 dll configuration path sup_clk need to divide by 2 as msm_host->clk_rate
is twice the host->clock as mentioned above.
>
>> and host->flags as 0x90c6.
>>
>> and
>>
>> sdhci_msm_configure_dll
>> sdhci_msm_dll_config
>> sdhci_msm_set_uhs_signaling
>> sdhci_set_ios
>> mmc_set_clock
>> mmc_set_bus_speed
>> mmc_select_hs400
>> mmc_init_card
>> _mmc_resume
>> mmc_runtime_resume
>>
>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
>> and host->flags as 0x90c6 which are same as 1st.
>>
>> Now if card is initialized in HS200 mode only below is the call stack
>>
>> sdhci_msm_configure_dll
>> sdhci_msm_dll_config
>> sdhci_msm_execute_tuning
>> mmc_execute_tuning
>> mmc_init_card
>> _mmc_resume
>> mmc_runtime_resume
>>
>> with values of host->clock as 200000000 & msm_host-clk_rate as 200000000
>> and host->flags as 0x90c6.
>>
>> now if you see the host->flags value, its same across the modes, and if
>> I am getting it right from the pointed out function
>> sdhci_msm_hc_select_mode(), your suggestion seems to be using the check
>> host->flags & SDHCI_HS400_TUNING which is bit[13], but in above dumped
>> host->flags SDHCI_HS400_TUNING bit is not set where we are using the /2.
>>
>> and the reason is, this bit is getting cleared in sdhci_msm_execute_tuning()
>> before sdhci_msm_dll_config() call.
>>
>> so this /2, is eventually called only for HS400 mode.
>>
>> Thanks,
>> Ram
>>
>>>
>>>> Now if we go for only HS200 mode supported card, there
>>>> the supported clock value would be 192Mhz itself and we need to pass
>>>> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
>>>> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
>>>> no other check is needed here.
>>> Please think about the cause, not about the symptom. Clocks being
>>> unequal is a result of some other checks being performed by the driver.
>>> Please use those checks too.
>>>
>>>> sorry for it took time to update as I was gathering all this data.
>>> 6 months? Well, that's a nice time to "gather all this data".
>>
>> Took it up from sachin last month but still its a long gap.
>> Thanks for helping revive.
>>
>>>
>>>> since Sachin have already pushed patchset #3, and if this explanation
>>>> helps, let me know if we can continue on patchset #3.
>>>>
>>>> Thanks,
>>>> Ram
>>>>
>
>
On Thu, Aug 07, 2025 at 01:28:28AM +0530, Ram Prakash Gupta wrote:
>
> On 7/31/2025 7:49 PM, Dmitry Baryshkov wrote:
> > On 31/07/2025 14:46, Ram Prakash Gupta wrote:
> >>
> >> On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
> >>> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
> >>>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
> >>>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
> >>>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
> >>>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
> >>>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
> >>>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
> >>>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
> >>>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
> >>>>>>>>>>>> +{
> >>>>>>>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >>>>>>>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> >>>>>>>>>>>> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
> >>>>>>>>>>>> + unsigned int sup_clk;
> >>>>>>>>>>>> +
> >>>>>>>>>>>> + if (req_clk < sdhci_msm_get_min_clock(host))
> >>>>>>>>>>>> + return sdhci_msm_get_min_clock(host);
> >>>>>>>>>>>> +
> >>>>>>>>>>>> + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
> >>>>>>>>>>>> +
> >>>>>>>>>>>> + if (host->clock != msm_host->clk_rate)
> >>>>>>>>>>>> + sup_clk = sup_clk / 2;
> >>>>>>>>>>>> +
> >>>>>>>>>>>> + return sup_clk;
> >>>>>>>>>>> Why?
> >>>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
> >>>>>>>>> Please explain the maths. You get the rate from the clock, then you
> >>>>>>>>> round it, but it is the rate that has just been returned, so there
> >>>>>>>>> should be no need to round it. And after that there a division by two
> >>>>>>>>> for some reason. So I've asked for an explanation for that code.
> >>>>>>>>>
> >>>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
> >>>>>>>> usable frequency.
> >>>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
> >>>>>>> round it? It sounds like that freq should be usable anyway.
> >>>>>>>
> >>>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
> >>>>>> patch.
> >>>>>>
> >>>>>>>> Divide by 2 is used as for HS400 the tuning happens in
> >>>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
> >>>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
> >>>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
> >>>>>>>
> >>>>>> Yes, It is 192 MHz.
> >>>>> Good, thanks for the confirmation.
> >>>>>
> >>>>>> As part of eMMC Init, driver will try to init with the best mode supported
> >>>>>> by controller and device. In this case it is HS400 mode, But as part of
> >>>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
> >>>>>> half of the clock.
> >>>>> This isn't an answer to the question. Let me rephrase it for you: if the
> >>>>> /2 is only used for HS400, why should it be attempted in all other
> >>>>> modes? Please limit the /2 just to HS400.
> >>>> Hi Dmitry,
> >>>>
> >>>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
> >>>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
> >>>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
> >>>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
> >>>> to get 192MHz and we find this as host->clock wont be equal to
> >>>> msm_host->clk_rate.
> >>> What are host->clock and msm_host->clk_rate at this point?
> >>> What is the host->flags value? See sdhci_msm_hc_select_mode().
> >>
> >> There are 2 paths which are traced to this function when card initializes
> >> in HS400 mode, please consider below call stack in 2 paths
> >>
> >> sdhci_msm_configure_dll
> >> sdhci_msm_dll_config
> >> sdhci_msm_execute_tuning
> >> mmc_execute_tuning
> >> mmc_init_card
> >> _mmc_resume
> >> mmc_runtime_resume
> >>
> >> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
> >
> > Please check the rates explicitly in the code rather than just checking that they are not equal.
>
> in function msm_get_clock_mult_for_bus_mode(), clk multiplier returns 2, with HS400
> DDR52 and DDR50 modes which is called from sdhci_msm_set_clock() and
> sdhci_msm_execute_tuning. And in sdhci_msm_execute_tuning(), we are calling
> sdhci_msm_dll_config() when SDHCI_HS400_TUNING is set and this flag is cleared
> immediately after return. And sdhci_msm_dll_config() is called after that.
>
> Now when the card is supporting HS400 mode, then from mmc_hs200_tuning(),
> sdhci_prepare_hs400_tuning is getting called, and there SDHCI_HS400_TUNING
> flag is set, and clock set is multiplying the clk rate by 2 in below call stack
>
> msm_set_clock_rate_for_bus_mode
> sdhci_msm_execute_tuning
> mmc_execute_tuning
> mmc_init_card
>
> so this clk rate is doubling only with HS400 mode selection and while setting up
> dll in HS400 dll configuration path sup_clk need to divide by 2 as msm_host->clk_rate
> is twice the host->clock as mentioned above.
I don't see how it's relevant. I'm asking you to check for the rate
values explicitly in the driver code rather than just checking that
rateA != rateB. You might error out if rateA != rateB and they are not
192 MHz / 384 MHz
>
>
> >
> >> and host->flags as 0x90c6.
> >>
> >> and
> >>
> >> sdhci_msm_configure_dll
> >> sdhci_msm_dll_config
> >> sdhci_msm_set_uhs_signaling
> >> sdhci_set_ios
> >> mmc_set_clock
> >> mmc_set_bus_speed
> >> mmc_select_hs400
> >> mmc_init_card
> >> _mmc_resume
> >> mmc_runtime_resume
> >>
> >> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
> >> and host->flags as 0x90c6 which are same as 1st.
> >>
> >> Now if card is initialized in HS200 mode only below is the call stack
> >>
> >> sdhci_msm_configure_dll
> >> sdhci_msm_dll_config
> >> sdhci_msm_execute_tuning
> >> mmc_execute_tuning
> >> mmc_init_card
> >> _mmc_resume
> >> mmc_runtime_resume
> >>
> >> with values of host->clock as 200000000 & msm_host-clk_rate as 200000000
> >> and host->flags as 0x90c6.
> >>
> >> now if you see the host->flags value, its same across the modes, and if
> >> I am getting it right from the pointed out function
> >> sdhci_msm_hc_select_mode(), your suggestion seems to be using the check
> >> host->flags & SDHCI_HS400_TUNING which is bit[13], but in above dumped
> >> host->flags SDHCI_HS400_TUNING bit is not set where we are using the /2.
> >>
> >> and the reason is, this bit is getting cleared in sdhci_msm_execute_tuning()
> >> before sdhci_msm_dll_config() call.
> >>
> >> so this /2, is eventually called only for HS400 mode.
> >>
> >> Thanks,
> >> Ram
> >>
> >>>
> >>>> Now if we go for only HS200 mode supported card, there
> >>>> the supported clock value would be 192Mhz itself and we need to pass
> >>>> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
> >>>> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
> >>>> no other check is needed here.
> >>> Please think about the cause, not about the symptom. Clocks being
> >>> unequal is a result of some other checks being performed by the driver.
> >>> Please use those checks too.
> >>>
> >>>> sorry for it took time to update as I was gathering all this data.
> >>> 6 months? Well, that's a nice time to "gather all this data".
> >>
> >> Took it up from sachin last month but still its a long gap.
> >> Thanks for helping revive.
> >>
> >>>
> >>>> since Sachin have already pushed patchset #3, and if this explanation
> >>>> helps, let me know if we can continue on patchset #3.
> >>>>
> >>>> Thanks,
> >>>> Ram
> >>>>
> >
> >
--
With best wishes
Dmitry
On 8/9/2025 3:10 PM, Dmitry Baryshkov wrote:
> On Thu, Aug 07, 2025 at 01:28:28AM +0530, Ram Prakash Gupta wrote:
>> On 7/31/2025 7:49 PM, Dmitry Baryshkov wrote:
>>> On 31/07/2025 14:46, Ram Prakash Gupta wrote:
>>>> On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
>>>>> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
>>>>>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
>>>>>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
>>>>>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>>>>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>>>>>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>>>>>>>>>>> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
>>>>>>>>>>>>>> + unsigned int sup_clk;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (req_clk < sdhci_msm_get_min_clock(host))
>>>>>>>>>>>>>> + return sdhci_msm_get_min_clock(host);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (host->clock != msm_host->clk_rate)
>>>>>>>>>>>>>> + sup_clk = sup_clk / 2;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + return sup_clk;
>>>>>>>>>>>>> Why?
>>>>>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>>>>>>>>> Please explain the maths. You get the rate from the clock, then you
>>>>>>>>>>> round it, but it is the rate that has just been returned, so there
>>>>>>>>>>> should be no need to round it. And after that there a division by two
>>>>>>>>>>> for some reason. So I've asked for an explanation for that code.
>>>>>>>>>>>
>>>>>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
>>>>>>>>>> usable frequency.
>>>>>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
>>>>>>>>> round it? It sounds like that freq should be usable anyway.
>>>>>>>>>
>>>>>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
>>>>>>>> patch.
>>>>>>>>
>>>>>>>>>> Divide by 2 is used as for HS400 the tuning happens in
>>>>>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
>>>>>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
>>>>>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
>>>>>>>>>
>>>>>>>> Yes, It is 192 MHz.
>>>>>>> Good, thanks for the confirmation.
>>>>>>>
>>>>>>>> As part of eMMC Init, driver will try to init with the best mode supported
>>>>>>>> by controller and device. In this case it is HS400 mode, But as part of
>>>>>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
>>>>>>>> half of the clock.
>>>>>>> This isn't an answer to the question. Let me rephrase it for you: if the
>>>>>>> /2 is only used for HS400, why should it be attempted in all other
>>>>>>> modes? Please limit the /2 just to HS400.
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
>>>>>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
>>>>>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
>>>>>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
>>>>>> to get 192MHz and we find this as host->clock wont be equal to
>>>>>> msm_host->clk_rate.
>>>>> What are host->clock and msm_host->clk_rate at this point?
>>>>> What is the host->flags value? See sdhci_msm_hc_select_mode().
>>>> There are 2 paths which are traced to this function when card initializes
>>>> in HS400 mode, please consider below call stack in 2 paths
>>>>
>>>> sdhci_msm_configure_dll
>>>> sdhci_msm_dll_config
>>>> sdhci_msm_execute_tuning
>>>> mmc_execute_tuning
>>>> mmc_init_card
>>>> _mmc_resume
>>>> mmc_runtime_resume
>>>>
>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
>>> Please check the rates explicitly in the code rather than just checking that they are not equal.
>> in function msm_get_clock_mult_for_bus_mode(), clk multiplier returns 2, with HS400
>> DDR52 and DDR50 modes which is called from sdhci_msm_set_clock() and
>> sdhci_msm_execute_tuning. And in sdhci_msm_execute_tuning(), we are calling
>> sdhci_msm_dll_config() when SDHCI_HS400_TUNING is set and this flag is cleared
>> immediately after return. And sdhci_msm_dll_config() is called after that.
>>
>> Now when the card is supporting HS400 mode, then from mmc_hs200_tuning(),
>> sdhci_prepare_hs400_tuning is getting called, and there SDHCI_HS400_TUNING
>> flag is set, and clock set is multiplying the clk rate by 2 in below call stack
>>
>> msm_set_clock_rate_for_bus_mode
>> sdhci_msm_execute_tuning
>> mmc_execute_tuning
>> mmc_init_card
>>
>> so this clk rate is doubling only with HS400 mode selection and while setting up
>> dll in HS400 dll configuration path sup_clk need to divide by 2 as msm_host->clk_rate
>> is twice the host->clock as mentioned above.
> I don't see how it's relevant. I'm asking you to check for the rate
> values explicitly in the driver code rather than just checking that
> rateA != rateB. You might error out if rateA != rateB and they are not
> 192 MHz / 384 MHz
ok I see, but I got one another alternate to this, I can try use similar checks used in
function msm_get_clock_mult_for_bus_mode(), ios.timing == MMC_TIMING_MMC_HS400
host->flags & SDHCI_HS400_TUNING, but for this I ll have to move check
host->flags &= ~SDHCI_HS400_TUNING in function sdhci_msm_execute_tuning () at the bottom
of this function, this will make code more readable and consistent to checks at other
places but I am testing it right now and will update.
If this doesn't work then I ll explicitly use the rate value in check.
Thanks,
Ram
>
>>
>>>> and host->flags as 0x90c6.
>>>>
>>>> and
>>>>
>>>> sdhci_msm_configure_dll
>>>> sdhci_msm_dll_config
>>>> sdhci_msm_set_uhs_signaling
>>>> sdhci_set_ios
>>>> mmc_set_clock
>>>> mmc_set_bus_speed
>>>> mmc_select_hs400
>>>> mmc_init_card
>>>> _mmc_resume
>>>> mmc_runtime_resume
>>>>
>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
>>>> and host->flags as 0x90c6 which are same as 1st.
>>>>
>>>> Now if card is initialized in HS200 mode only below is the call stack
>>>>
>>>> sdhci_msm_configure_dll
>>>> sdhci_msm_dll_config
>>>> sdhci_msm_execute_tuning
>>>> mmc_execute_tuning
>>>> mmc_init_card
>>>> _mmc_resume
>>>> mmc_runtime_resume
>>>>
>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 200000000
>>>> and host->flags as 0x90c6.
>>>>
>>>> now if you see the host->flags value, its same across the modes, and if
>>>> I am getting it right from the pointed out function
>>>> sdhci_msm_hc_select_mode(), your suggestion seems to be using the check
>>>> host->flags & SDHCI_HS400_TUNING which is bit[13], but in above dumped
>>>> host->flags SDHCI_HS400_TUNING bit is not set where we are using the /2.
>>>>
>>>> and the reason is, this bit is getting cleared in sdhci_msm_execute_tuning()
>>>> before sdhci_msm_dll_config() call.
>>>>
>>>> so this /2, is eventually called only for HS400 mode.
>>>>
>>>> Thanks,
>>>> Ram
>>>>
>>>>>> Now if we go for only HS200 mode supported card, there
>>>>>> the supported clock value would be 192Mhz itself and we need to pass
>>>>>> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
>>>>>> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
>>>>>> no other check is needed here.
>>>>> Please think about the cause, not about the symptom. Clocks being
>>>>> unequal is a result of some other checks being performed by the driver.
>>>>> Please use those checks too.
>>>>>
>>>>>> sorry for it took time to update as I was gathering all this data.
>>>>> 6 months? Well, that's a nice time to "gather all this data".
>>>> Took it up from sachin last month but still its a long gap.
>>>> Thanks for helping revive.
>>>>
>>>>>> since Sachin have already pushed patchset #3, and if this explanation
>>>>>> helps, let me know if we can continue on patchset #3.
>>>>>>
>>>>>> Thanks,
>>>>>> Ram
>>>>>>
>>>
On 8/21/2025 7:45 PM, Ram Prakash Gupta wrote:
> On 8/9/2025 3:10 PM, Dmitry Baryshkov wrote:
>> On Thu, Aug 07, 2025 at 01:28:28AM +0530, Ram Prakash Gupta wrote:
>>> On 7/31/2025 7:49 PM, Dmitry Baryshkov wrote:
>>>> On 31/07/2025 14:46, Ram Prakash Gupta wrote:
>>>>> On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
>>>>>> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
>>>>>>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
>>>>>>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>>>>>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>>>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>>>>>>>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>>>>>>>>>>>> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
>>>>>>>>>>>>>>> + unsigned int sup_clk;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (req_clk < sdhci_msm_get_min_clock(host))
>>>>>>>>>>>>>>> + return sdhci_msm_get_min_clock(host);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (host->clock != msm_host->clk_rate)
>>>>>>>>>>>>>>> + sup_clk = sup_clk / 2;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + return sup_clk;
>>>>>>>>>>>>>> Why?
>>>>>>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>>>>>>>>>> Please explain the maths. You get the rate from the clock, then you
>>>>>>>>>>>> round it, but it is the rate that has just been returned, so there
>>>>>>>>>>>> should be no need to round it. And after that there a division by two
>>>>>>>>>>>> for some reason. So I've asked for an explanation for that code.
>>>>>>>>>>>>
>>>>>>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
>>>>>>>>>>> usable frequency.
>>>>>>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
>>>>>>>>>> round it? It sounds like that freq should be usable anyway.
>>>>>>>>>>
>>>>>>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
>>>>>>>>> patch.
>>>>>>>>>
>>>>>>>>>>> Divide by 2 is used as for HS400 the tuning happens in
>>>>>>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
>>>>>>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
>>>>>>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
>>>>>>>>>>
>>>>>>>>> Yes, It is 192 MHz.
>>>>>>>> Good, thanks for the confirmation.
>>>>>>>>
>>>>>>>>> As part of eMMC Init, driver will try to init with the best mode supported
>>>>>>>>> by controller and device. In this case it is HS400 mode, But as part of
>>>>>>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
>>>>>>>>> half of the clock.
>>>>>>>> This isn't an answer to the question. Let me rephrase it for you: if the
>>>>>>>> /2 is only used for HS400, why should it be attempted in all other
>>>>>>>> modes? Please limit the /2 just to HS400.
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
>>>>>>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
>>>>>>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
>>>>>>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
>>>>>>> to get 192MHz and we find this as host->clock wont be equal to
>>>>>>> msm_host->clk_rate.
>>>>>> What are host->clock and msm_host->clk_rate at this point?
>>>>>> What is the host->flags value? See sdhci_msm_hc_select_mode().
>>>>> There are 2 paths which are traced to this function when card initializes
>>>>> in HS400 mode, please consider below call stack in 2 paths
>>>>>
>>>>> sdhci_msm_configure_dll
>>>>> sdhci_msm_dll_config
>>>>> sdhci_msm_execute_tuning
>>>>> mmc_execute_tuning
>>>>> mmc_init_card
>>>>> _mmc_resume
>>>>> mmc_runtime_resume
>>>>>
>>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
>>>> Please check the rates explicitly in the code rather than just checking that they are not equal.
>>> in function msm_get_clock_mult_for_bus_mode(), clk multiplier returns 2, with HS400
>>> DDR52 and DDR50 modes which is called from sdhci_msm_set_clock() and
>>> sdhci_msm_execute_tuning. And in sdhci_msm_execute_tuning(), we are calling
>>> sdhci_msm_dll_config() when SDHCI_HS400_TUNING is set and this flag is cleared
>>> immediately after return. And sdhci_msm_dll_config() is called after that.
>>>
>>> Now when the card is supporting HS400 mode, then from mmc_hs200_tuning(),
>>> sdhci_prepare_hs400_tuning is getting called, and there SDHCI_HS400_TUNING
>>> flag is set, and clock set is multiplying the clk rate by 2 in below call stack
>>>
>>> msm_set_clock_rate_for_bus_mode
>>> sdhci_msm_execute_tuning
>>> mmc_execute_tuning
>>> mmc_init_card
>>>
>>> so this clk rate is doubling only with HS400 mode selection and while setting up
>>> dll in HS400 dll configuration path sup_clk need to divide by 2 as msm_host->clk_rate
>>> is twice the host->clock as mentioned above.
>> I don't see how it's relevant. I'm asking you to check for the rate
>> values explicitly in the driver code rather than just checking that
>> rateA != rateB. You might error out if rateA != rateB and they are not
>> 192 MHz / 384 MHz
> ok I see, but I got one another alternate to this, I can try use similar checks used in
> function msm_get_clock_mult_for_bus_mode(), ios.timing == MMC_TIMING_MMC_HS400
> host->flags & SDHCI_HS400_TUNING, but for this I ll have to move check
> host->flags &= ~SDHCI_HS400_TUNING in function sdhci_msm_execute_tuning () at the bottom
> of this function, this will make code more readable and consistent to checks at other
> places but I am testing it right now and will update.
>
> If this doesn't work then I ll explicitly use the rate value in check.
>
> Thanks,
> Ram
Hi Dmitry,
adding checks for ios.timing == MMC_TIMING_MMC_HS400 && host->flags & SDHCI_HS400_TUNING
serves the same purpose for dividing the clk when mode is hs400 and hs400_tuning flag is
set, and clk value checks can be avoided now.
With this HS200, HS400, HS400ES modes of eMMC is tested.
so if this approach looks good to you, then I would like to proceed with next patchset.
Thanks,
Ram
>>>>> and host->flags as 0x90c6.
>>>>>
>>>>> and
>>>>>
>>>>> sdhci_msm_configure_dll
>>>>> sdhci_msm_dll_config
>>>>> sdhci_msm_set_uhs_signaling
>>>>> sdhci_set_ios
>>>>> mmc_set_clock
>>>>> mmc_set_bus_speed
>>>>> mmc_select_hs400
>>>>> mmc_init_card
>>>>> _mmc_resume
>>>>> mmc_runtime_resume
>>>>>
>>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
>>>>> and host->flags as 0x90c6 which are same as 1st.
>>>>>
>>>>> Now if card is initialized in HS200 mode only below is the call stack
>>>>>
>>>>> sdhci_msm_configure_dll
>>>>> sdhci_msm_dll_config
>>>>> sdhci_msm_execute_tuning
>>>>> mmc_execute_tuning
>>>>> mmc_init_card
>>>>> _mmc_resume
>>>>> mmc_runtime_resume
>>>>>
>>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 200000000
>>>>> and host->flags as 0x90c6.
>>>>>
>>>>> now if you see the host->flags value, its same across the modes, and if
>>>>> I am getting it right from the pointed out function
>>>>> sdhci_msm_hc_select_mode(), your suggestion seems to be using the check
>>>>> host->flags & SDHCI_HS400_TUNING which is bit[13], but in above dumped
>>>>> host->flags SDHCI_HS400_TUNING bit is not set where we are using the /2.
>>>>>
>>>>> and the reason is, this bit is getting cleared in sdhci_msm_execute_tuning()
>>>>> before sdhci_msm_dll_config() call.
>>>>>
>>>>> so this /2, is eventually called only for HS400 mode.
>>>>>
>>>>> Thanks,
>>>>> Ram
>>>>>
>>>>>>> Now if we go for only HS200 mode supported card, there
>>>>>>> the supported clock value would be 192Mhz itself and we need to pass
>>>>>>> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
>>>>>>> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
>>>>>>> no other check is needed here.
>>>>>> Please think about the cause, not about the symptom. Clocks being
>>>>>> unequal is a result of some other checks being performed by the driver.
>>>>>> Please use those checks too.
>>>>>>
>>>>>>> sorry for it took time to update as I was gathering all this data.
>>>>>> 6 months? Well, that's a nice time to "gather all this data".
>>>>> Took it up from sachin last month but still its a long gap.
>>>>> Thanks for helping revive.
>>>>>
>>>>>>> since Sachin have already pushed patchset #3, and if this explanation
>>>>>>> helps, let me know if we can continue on patchset #3.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Ram
>>>>>>>
On Thu, Aug 28, 2025 at 01:30:21PM +0530, Ram Prakash Gupta wrote:
>
> On 8/21/2025 7:45 PM, Ram Prakash Gupta wrote:
> > On 8/9/2025 3:10 PM, Dmitry Baryshkov wrote:
> >> On Thu, Aug 07, 2025 at 01:28:28AM +0530, Ram Prakash Gupta wrote:
> >>> On 7/31/2025 7:49 PM, Dmitry Baryshkov wrote:
> >>>> On 31/07/2025 14:46, Ram Prakash Gupta wrote:
> >>>>> On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
> >>>>>> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
> >>>>>>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
> >>>>>>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
> >>>>>>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
> >>>>>>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
> >>>>>>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
> >>>>>>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
> >>>>>>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
> >>>>>>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
> >>>>>>>>>>>>>>> +{
> >>>>>>>>>>>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >>>>>>>>>>>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> >>>>>>>>>>>>>>> + struct clk *core_clk = msm_host->bulk_clks[0].clk;
> >>>>>>>>>>>>>>> + unsigned int sup_clk;
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>> + if (req_clk < sdhci_msm_get_min_clock(host))
> >>>>>>>>>>>>>>> + return sdhci_msm_get_min_clock(host);
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>> + sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>> + if (host->clock != msm_host->clk_rate)
> >>>>>>>>>>>>>>> + sup_clk = sup_clk / 2;
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>> + return sup_clk;
> >>>>>>>>>>>>>> Why?
> >>>>>>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
> >>>>>>>>>>>> Please explain the maths. You get the rate from the clock, then you
> >>>>>>>>>>>> round it, but it is the rate that has just been returned, so there
> >>>>>>>>>>>> should be no need to round it. And after that there a division by two
> >>>>>>>>>>>> for some reason. So I've asked for an explanation for that code.
> >>>>>>>>>>>>
> >>>>>>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
> >>>>>>>>>>> usable frequency.
> >>>>>>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
> >>>>>>>>>> round it? It sounds like that freq should be usable anyway.
> >>>>>>>>>>
> >>>>>>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
> >>>>>>>>> patch.
> >>>>>>>>>
> >>>>>>>>>>> Divide by 2 is used as for HS400 the tuning happens in
> >>>>>>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
> >>>>>>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
> >>>>>>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
> >>>>>>>>>>
> >>>>>>>>> Yes, It is 192 MHz.
> >>>>>>>> Good, thanks for the confirmation.
> >>>>>>>>
> >>>>>>>>> As part of eMMC Init, driver will try to init with the best mode supported
> >>>>>>>>> by controller and device. In this case it is HS400 mode, But as part of
> >>>>>>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
> >>>>>>>>> half of the clock.
> >>>>>>>> This isn't an answer to the question. Let me rephrase it for you: if the
> >>>>>>>> /2 is only used for HS400, why should it be attempted in all other
> >>>>>>>> modes? Please limit the /2 just to HS400.
> >>>>>>> Hi Dmitry,
> >>>>>>>
> >>>>>>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
> >>>>>>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
> >>>>>>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
> >>>>>>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
> >>>>>>> to get 192MHz and we find this as host->clock wont be equal to
> >>>>>>> msm_host->clk_rate.
> >>>>>> What are host->clock and msm_host->clk_rate at this point?
> >>>>>> What is the host->flags value? See sdhci_msm_hc_select_mode().
> >>>>> There are 2 paths which are traced to this function when card initializes
> >>>>> in HS400 mode, please consider below call stack in 2 paths
> >>>>>
> >>>>> sdhci_msm_configure_dll
> >>>>> sdhci_msm_dll_config
> >>>>> sdhci_msm_execute_tuning
> >>>>> mmc_execute_tuning
> >>>>> mmc_init_card
> >>>>> _mmc_resume
> >>>>> mmc_runtime_resume
> >>>>>
> >>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
> >>>> Please check the rates explicitly in the code rather than just checking that they are not equal.
> >>> in function msm_get_clock_mult_for_bus_mode(), clk multiplier returns 2, with HS400
> >>> DDR52 and DDR50 modes which is called from sdhci_msm_set_clock() and
> >>> sdhci_msm_execute_tuning. And in sdhci_msm_execute_tuning(), we are calling
> >>> sdhci_msm_dll_config() when SDHCI_HS400_TUNING is set and this flag is cleared
> >>> immediately after return. And sdhci_msm_dll_config() is called after that.
> >>>
> >>> Now when the card is supporting HS400 mode, then from mmc_hs200_tuning(),
> >>> sdhci_prepare_hs400_tuning is getting called, and there SDHCI_HS400_TUNING
> >>> flag is set, and clock set is multiplying the clk rate by 2 in below call stack
> >>>
> >>> msm_set_clock_rate_for_bus_mode
> >>> sdhci_msm_execute_tuning
> >>> mmc_execute_tuning
> >>> mmc_init_card
> >>>
> >>> so this clk rate is doubling only with HS400 mode selection and while setting up
> >>> dll in HS400 dll configuration path sup_clk need to divide by 2 as msm_host->clk_rate
> >>> is twice the host->clock as mentioned above.
> >> I don't see how it's relevant. I'm asking you to check for the rate
> >> values explicitly in the driver code rather than just checking that
> >> rateA != rateB. You might error out if rateA != rateB and they are not
> >> 192 MHz / 384 MHz
> > ok I see, but I got one another alternate to this, I can try use similar checks used in
> > function msm_get_clock_mult_for_bus_mode(), ios.timing == MMC_TIMING_MMC_HS400
> > host->flags & SDHCI_HS400_TUNING, but for this I ll have to move check
> > host->flags &= ~SDHCI_HS400_TUNING in function sdhci_msm_execute_tuning () at the bottom
> > of this function, this will make code more readable and consistent to checks at other
> > places but I am testing it right now and will update.
> >
> > If this doesn't work then I ll explicitly use the rate value in check.
> >
> > Thanks,
> > Ram
>
> Hi Dmitry,
>
> adding checks for ios.timing == MMC_TIMING_MMC_HS400 && host->flags & SDHCI_HS400_TUNING
> serves the same purpose for dividing the clk when mode is hs400 and hs400_tuning flag is
> set, and clk value checks can be avoided now.
>
> With this HS200, HS400, HS400ES modes of eMMC is tested.
>
> so if this approach looks good to you, then I would like to proceed with next patchset.
LGTM.
--
With best wishes
Dmitry
© 2016 - 2025 Red Hat, Inc.