Re: [PATCH] mmc: sdhci-of-arasan: Use standard mmc_clk_phase_map infrastructure

Sai Krishna Potthuri
2026-03-23T09:27:33Z

> -----Original Message-----
> From: Simek, Michal <michal.simek@amd.com>
> Sent: Thursday, March 19, 2026 3:39 PM
> To: Shawn Lin <shawn.lin@rock-chips.com>; Ulf Hansson
> <ulf.hansson@linaro.org>; Potthuri, Sai Krishna
> <sai.krishna.potthuri@amd.com>
> Cc: linux-mmc@vger.kernel.org; Adrian Hunter <adrian.hunter@intel.com>;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] mmc: sdhci-of-arasan: Use standard
> mmc_clk_phase_map infrastructure
>
> +Sai,
>
> On 3/17/26 03:17, Shawn Lin wrote:
> > Convert the Arasan SDHCI driver to use the mainline standard
> > mmc_clk_phase_map infrastructure instead of custom clk_phase_in/out
> > arrays as well as arasan_dt_read_clk_phase().
> >
> > The phase values for ZynqMP, Versal, and Versal-NET platforms are
> > still initialized from the predefined tables, but now follow the
> > standard phase_map format with valid flag.
> >
> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> > ---
> >
> > drivers/mmc/host/sdhci-of-arasan.c | 79 +++++++++-----------------------------
> > 1 file changed, 18 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-arasan.c
> > b/drivers/mmc/host/sdhci-of-arasan.c
> > index caf9723..b97d27c 100644
> > --- a/drivers/mmc/host/sdhci-of-arasan.c
> > +++ b/drivers/mmc/host/sdhci-of-arasan.c
> > @@ -152,8 +152,7 @@ struct sdhci_arasan_clk_ops {
> > * @sdcardclk: Pointer to normal 'struct clock' for
> sdcardclk_hw.
> > * @sampleclk_hw: Struct for the clock we might provide to a PHY.
> > * @sampleclk: Pointer to normal 'struct clock' for
> sampleclk_hw.
> > - * @clk_phase_in: Array of Input Clock Phase Delays for all speed modes
> > - * @clk_phase_out: Array of Output Clock Phase Delays for all speed
> modes
> > + * @phase_map: Struct for mmc_clk_phase_map provided.
> > * @set_clk_delays: Function pointer for setting Clock Delays
> > * @clk_of_data: Platform specific runtime clock data storage pointer
> > */
> > @@ -162,8 +161,7 @@ struct sdhci_arasan_clk_data {
> > struct clk *sdcardclk;
> > struct clk_hw sampleclk_hw;
> > struct clk *sampleclk;
> > - int clk_phase_in[MMC_TIMING_MMC_HS400 + 1];
> > - int clk_phase_out[MMC_TIMING_MMC_HS400 + 1];
> > + struct mmc_clk_phase_map phase_map;
> > void (*set_clk_delays)(struct sdhci_host *host);
> > void *clk_of_data;
> > };
> > @@ -1248,37 +1246,13 @@ static void sdhci_arasan_set_clk_delays(struct
> sdhci_host *host)
> > struct sdhci_arasan_data *sdhci_arasan =
> sdhci_pltfm_priv(pltfm_host);
> > struct sdhci_arasan_clk_data *clk_data = &sdhci_arasan->clk_data;
> >
> > - clk_set_phase(clk_data->sampleclk,
> > - clk_data->clk_phase_in[host->timing]);
> > - clk_set_phase(clk_data->sdcardclk,
> > - clk_data->clk_phase_out[host->timing]);
> > -}
> > -
> > -static void arasan_dt_read_clk_phase(struct device *dev,
> > - struct sdhci_arasan_clk_data *clk_data,
> > - unsigned int timing, const char *prop)
> > -{
> > - struct device_node *np = dev->of_node;
> > -
> > - u32 clk_phase[2] = {0};
> > - int ret;
> > -
> > - /*
> > - * Read Tap Delay values from DT, if the DT does not contain the
> > - * Tap Values then use the pre-defined values.
> > - */
> > - ret = of_property_read_variable_u32_array(np, prop, &clk_phase[0],
> > - 2, 0);
> > - if (ret < 0) {
> > - dev_dbg(dev, "Using predefined clock phase for %s = %d
> %d\n",
> > - prop, clk_data->clk_phase_in[timing],
> > - clk_data->clk_phase_out[timing]);
> > + if (!clk_data->phase_map.phase[host->timing].valid)
> > return;

This check breaks platforms relying on default phase values. When DT
properties are absent, mmc_of_parse_timing_phase() sets valid=false,
which causes sdhci_arasan_set_clk_delays() to skip phase configuration.

Regards
Sai Krishna

> > - }
> >
> > - /* The values read are Input and Output Clock Delays in order */
> > - clk_data->clk_phase_in[timing] = clk_phase[0];
> > - clk_data->clk_phase_out[timing] = clk_phase[1];
> > + clk_set_phase(clk_data->sampleclk,
> > + clk_data->phase_map.phase[host->timing].in_deg);
> > + clk_set_phase(clk_data->sdcardclk,
> > + clk_data->phase_map.phase[host->timing].out_deg);
> > }
> >
> > /**
> > @@ -1315,8 +1289,9 @@ static void arasan_dt_parse_clk_phases(struct
> device *dev,
> > }
> >
> > for (i = 0; i <= MMC_TIMING_MMC_HS400; i++) {
> > - clk_data->clk_phase_in[i] = zynqmp_iclk_phase[i];
> > - clk_data->clk_phase_out[i] = zynqmp_oclk_phase[i];
> > + clk_data->phase_map.phase[i].in_deg =
> zynqmp_iclk_phase[i];
> > + clk_data->phase_map.phase[i].out_deg =
> zynqmp_oclk_phase[i];
> > + clk_data->phase_map.phase[i].valid = true;
> > }
> > }
> >
> > @@ -1327,8 +1302,9 @@ static void arasan_dt_parse_clk_phases(struct
> device *dev,
> > VERSAL_OCLK_PHASE;
> >
> > for (i = 0; i <= MMC_TIMING_MMC_HS400; i++) {
> > - clk_data->clk_phase_in[i] = versal_iclk_phase[i];
> > - clk_data->clk_phase_out[i] = versal_oclk_phase[i];
> > + clk_data->phase_map.phase[i].in_deg =
> versal_iclk_phase[i];
> > + clk_data->phase_map.phase[i].out_deg =
> versal_oclk_phase[i];
> > + clk_data->phase_map.phase[i].valid = true;
> > }
> > }
> > if (of_device_is_compatible(dev->of_node, "xlnx,versal-net-emmc"))
> > { @@ -1338,32 +1314,13 @@ static void arasan_dt_parse_clk_phases(struct
> device *dev,
> > VERSAL_NET_EMMC_OCLK_PHASE;
> >
> > for (i = 0; i <= MMC_TIMING_MMC_HS400; i++) {
> > - clk_data->clk_phase_in[i] = versal_net_iclk_phase[i];
> > - clk_data->clk_phase_out[i] =
> versal_net_oclk_phase[i];
> > + clk_data->phase_map.phase[i].in_deg =
> versal_net_iclk_phase[i];
> > + clk_data->phase_map.phase[i].out_deg =
> versal_net_oclk_phase[i];
> > + clk_data->phase_map.phase[i].valid = true;
> > }
> > }
> > - arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_LEGACY,
> > - "clk-phase-legacy");
> > - arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_MMC_HS,
> > - "clk-phase-mmc-hs");
> > - arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_SD_HS,
> > - "clk-phase-sd-hs");
> > - arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR12,
> > - "clk-phase-uhs-sdr12");
> > - arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR25,
> > - "clk-phase-uhs-sdr25");
> > - arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR50,
> > - "clk-phase-uhs-sdr50");
> > - arasan_dt_read_clk_phase(dev, clk_data,
> MMC_TIMING_UHS_SDR104,
> > - "clk-phase-uhs-sdr104");
> > - arasan_dt_read_clk_phase(dev, clk_data,
> MMC_TIMING_UHS_DDR50,
> > - "clk-phase-uhs-ddr50");
> > - arasan_dt_read_clk_phase(dev, clk_data,
> MMC_TIMING_MMC_DDR52,
> > - "clk-phase-mmc-ddr52");
> > - arasan_dt_read_clk_phase(dev, clk_data,
> MMC_TIMING_MMC_HS200,
> > - "clk-phase-mmc-hs200");
> > - arasan_dt_read_clk_phase(dev, clk_data,
> MMC_TIMING_MMC_HS400,
> > - "clk-phase-mmc-hs400");
> > +
> > + mmc_of_parse_clk_phase(dev, &clk_data->phase_map);
> > }
> >
> > static const struct sdhci_pltfm_data sdhci_arasan_pdata = {
>
> please take a look at this patch.
> I was talking to him yesterday and he needs to finish two things before he has
> time to look at it. That's why please give him some time.
>
> Thanks,
> Michal