Re: [PATCH v6 07/10] clk: realtek: Add support for MMC-tuned PLL clocks

[Yu-Chun Lin <eleanor.lin@realtek.com>]

Hi Brian,

Sorry for the late reply.

> Hi Yu-Chun and Cheng-Yu,
>
> On Thu, Apr 02, 2026 at 03:39:54PM +0800, Yu-Chun Lin wrote:
> > From: Cheng-Yu Lee <cylee12@realtek.com>
> > 
> > Add clk_pll_mmc_ops for enable/disable, prepare, rate control, and status
> > operations on MMC PLL clocks.
> > 
> > Also add clk_pll_mmc_phase_ops to support phase get/set operations.
> > 
> > Signed-off-by: Cheng-Yu Lee <cylee12@realtek.com>
> > Co-developed-by: Jyan Chou <jyanchou@realtek.com>
> > Signed-off-by: Jyan Chou <jyanchou@realtek.com>
> > Co-developed-by: Yu-Chun Lin <eleanor.lin@realtek.com>
> > Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>
> > ---
> > Changes in v6:
> > - Add the headers used in c file to follow the "Include What You Use" principle.
> > - Move to_clk_pll_mmc() from clk-pll.h to clk-pll-mmc.c to limit its scope.
> > - Change offset type from int to unsigned int.
> > ---
> >  MAINTAINERS                       |   8 +
> >  drivers/clk/realtek/Kconfig       |   3 +
> >  drivers/clk/realtek/Makefile      |   2 +
> >  drivers/clk/realtek/clk-pll-mmc.c | 410 ++++++++++++++++++++++++++++++
> >  drivers/clk/realtek/clk-pll.h     |  13 +
> >  5 files changed, 436 insertions(+)
> >  create mode 100644 drivers/clk/realtek/clk-pll-mmc.c
> > 

(snip)

> > +
> > +static inline int get_phrt0(struct clk_pll_mmc *clkm, u32 *val)
> > +{
> > +	u32 reg;
> > +	int ret;
> > +
> > +	ret = regmap_read(clkm->clkr.regmap, clkm->pll_ofs + PLL_EMMC1_OFFSET, &reg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	*val = (reg >> PLL_PHRT0_SHIFT) & PLL_PHRT0_MASK;
>
> Sashiko reports the following:
> https://sashiko.dev/#/patchset/20260402073957.2742459-1-eleanor.lin%40realtek.com
> 
>    With PLL_PHRT0_SHIFT defined as 1 and PLL_PHRT0_MASK as BIT(1) (0x02), shifting
>    right by 1 moves the target bit 1 to position 0, but masking with 0x02 checks
>    position 1 of the shifted value.
>    
>    Will this cause clk_pll_mmc_is_enabled() to always evaluate to false since it
>    expects val == 0x1?
>

Thank you for catching this critical bug! You're absolutely right.
The issue is that I incorrectly used BIT() for the mask values
I will correct them, like PLL_PHRT0_MASK from BIT(1) to 0x1.

> > +	return 0;
> > +}
> > +
> > +static inline int set_phrt0(struct clk_pll_mmc *clkm, u32 val)
> > +{
> > +	return regmap_update_bits(clkm->clkr.regmap, clkm->pll_ofs + PLL_EMMC1_OFFSET,
> > +				  PLL_PHRT0_MASK, val << PLL_PHRT0_SHIFT);
> > +}
> > +
> > +static inline int get_phsel(struct clk_pll_mmc *clkm, int id, u32 *val)
> > +{
> > +	int ret;
> > +	u32 raw_val;
> > +	u32 sft = id ? 8 : 3;
>
> Put variables in reverse Christmas tree order.
>

Ack.

(snip)

> > +
> > +static int clk_pll_mmc_phase_set_phase(struct clk_hw *hw, int degrees)
> > +{
> > +	struct clk_hw *hwp = clk_hw_get_parent(hw);
> > +	struct clk_pll_mmc *clkm;
> > +	int phase_id;
> > +	int ret;
> > +	u32 val;
> > +
> > +	if (!hwp)
> > +		return -ENOENT;
> > +
> > +	clkm = to_clk_pll_mmc(hwp);
> > +	phase_id = (hw - &clkm->phase0_hw) ? 1 : 0;
> 
> Are you checking to see if these two pointers are the same? If so, what
> do you think about this instead?
>
>    hw == &clkm->phase0_hw
>
>
> Does you mean phase_id = (hw == &clkm->phase0_hw) ? 0 : 1; ?
>

Yes, I will revise it according to your suggestion.

> > +	val = DIV_ROUND_CLOSEST(degrees * 100, PHASE_SCALE_FACTOR);
> > +	ret = set_phsel(clkm, phase_id, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	usleep_range(10, 20);
> > +	return 0;
> > +}
> > +

(snip)

> > +
> > +static unsigned long clk_pll_mmc_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> > +{
> > +	struct clk_pll_mmc *clkm = to_clk_pll_mmc(hw);
> > +	u32 val, ext_f;
> > +	int ret;
> > +
> > +	ret = get_ssc_div_n(clkm, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = get_ssc_div_ext_f(clkm, &ext_f);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return parent_rate / 4 * (val + 2) + (parent_rate / 4 * ext_f) / 8192;
> > +}
> > +
> > +static int clk_pll_mmc_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> > +{
>
> Should there be a check for a parent rate of zero before the division is
> done?
>

Ack, I will do it.

> > +	u32 val = DIV_ROUND_CLOSEST(req->rate * 4, req->best_parent_rate);
> > +
> > +	req->rate = req->best_parent_rate * val / 4;
> > +	return 0;
> > +}
> > +
> > +static int clk_pll_mmc_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate)
> > +{
> > +	struct clk_pll_mmc *clkm = to_clk_pll_mmc(hw);
> > +	u32 val = PLL_MMC_SSC_DIV_N_VAL;
> > +	int ret;
> > +
> > +	ret = regmap_update_bits(clkm->clkr.regmap,
> > +				 clkm->ssc_dig_ofs + PLL_SSC_DIG_EMMC1_OFFSET,
> > +				 PLL_FLAG_INITAL_EMMC_MASK, 0x0 << PLL_FLAG_INITAL_EMMC_SHIFT);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = set_ssc_div_n(clkm, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = set_ssc_div_ext_f(clkm, 1517);
> > +	if (ret)
> > +		return ret;
> > +
> > +	switch (val) {
> > +	case 31 ... 46:
> > +		ret |= set_pi_ibselh(clkm, 3);
> > +		ret |= set_sscpll_rs(clkm, 3);
> > +		ret |= set_sscpll_icp(clkm, 2);
> > +		break;
> > +
> > +	case 20 ... 30:
> > +		ret |= set_pi_ibselh(clkm, 2);
> > +		ret |= set_sscpll_rs(clkm, 3);
> > +		ret |= set_sscpll_icp(clkm, 1);
> > +		break;
> > +
> > +	case 10 ... 19:
> > +		ret |= set_pi_ibselh(clkm, 1);
> > +		ret |= set_sscpll_rs(clkm, 2);
> > +		ret |= set_sscpll_icp(clkm, 1);
> > +		break;
> > +
> > +	case 5 ... 9:
> > +		ret |= set_pi_ibselh(clkm, 0);
> > +		ret |= set_sscpll_rs(clkm, 2);
> > +		ret |= set_sscpll_icp(clkm, 0);
> > +		break;
> > +	}
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(clkm->clkr.regmap,
> > +				 clkm->ssc_dig_ofs + PLL_SSC_DIG_EMMC3_OFFSET,
> > +				 PLL_NCODE_SSC_EMMC_MASK,
> > +				 27 << PLL_NCODE_SSC_EMMC_SHIFT);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(clkm->clkr.regmap,
> > +				 clkm->ssc_dig_ofs + PLL_SSC_DIG_EMMC3_OFFSET,
> > +				 PLL_FCODE_SSC_EMMC_MASK, 321);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(clkm->clkr.regmap,
> > +				 clkm->ssc_dig_ofs + PLL_SSC_DIG_EMMC4_OFFSET,
> > +				 PLL_GRAN_EST_EM_MC_MASK, 5985);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(clkm->clkr.regmap,
> > +				 clkm->ssc_dig_ofs + PLL_SSC_DIG_EMMC1_OFFSET,
> > +				 PLL_EN_SSC_EMMC_MASK, 0x1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(clkm->clkr.regmap,
> > +				 clkm->ssc_dig_ofs + PLL_SSC_DIG_EMMC1_OFFSET,
> > +				 PLL_EN_SSC_EMMC_MASK, 0x0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(clkm->clkr.regmap,
> > +				 clkm->ssc_dig_ofs + PLL_SSC_DIG_EMMC1_OFFSET,
> > +				 PLL_FLAG_INITAL_EMMC_MASK,
> > +				 0x1 << PLL_FLAG_INITAL_EMMC_SHIFT);
>
> It looks like the rate and parent rate are not used in this function.
> Will this always end up with the same rate when everything is
> successful?
>
> Brian

Despite receiving various rate requests (26MHz, 52MHz, 200MHz), this function
consistently returns 0x1b (represents the 27MHz) because it reflects the input
reference clock frequency to the SSCPLL, not the PLL output frequency.

However, the emmc host controller handles frequency division internally to
achieve the requested eMMC frequency.

Best Regards,
Yu-Chun