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

[Yu-Chun Lin]

Hi Brian,

> Hi Yu-Chun,
>
> I should have finished going through Sashiko while manually reviewing
> your patches.
>
> On Thu, Apr 02, 2026 at 03:39:54PM +0800, Yu-Chun Lin wrote:
>> From: Cheng-Yu Lee <cylee12@xxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxx>
> > Co-developed-by: Jyan Chou <jyanchou@xxxxxxxxxxx>
> > Signed-off-by: Jyan Chou <jyanchou@xxxxxxxxxxx>
> > Co-developed-by: Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx>
> > Signed-off-by: Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx>
> > ---
> > +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);
> 
> Sashiko reports:
> https://sashiko.dev/#/patchset/20260402073957.2742459-1-eleanor.lin%40realtek.com
> 
>     Is it intended to use bitwise OR to accumulate these return values? Because
>     these hardware operations return standard negative error codes on failure,
>     performing a bitwise OR on multiple negative integers will merge their bit
>     patterns and create a corrupted error code.
> 

Will return immediately upon the first error.

> > +		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);
> 
> Sashiko reports:
> https://sashiko.dev/#/patchset/20260402073957.2742459-1-eleanor.lin%40realtek.com
> 
>     Are the mask and shift values mismatched here? PLL_FLAG_INITAL_EMMC_MASK is
>     defined as BIT(1) (0x02), but PLL_FLAG_INITAL_EMMC_SHIFT is 8.
> 
>     When regmap_update_bits() applies the 0x02 mask to a value shifted by 8,
>     won't it evaluate to 0 and fail to set the intended initialization flag?
> 
> Brian

You're right, will fix it.

Yu-Chun.