RE: [PATCH v11 08/11] clk: realtek: Add support for MMC-tuned PLL clocks

Hi Brian,
 
> Hi Yu-Chun,
>
> On Tue, Jul 28, 2026 at 10:28:03PM +0800, Yu-Chun Lin wrote:
> > From: Cheng-Yu Lee <[email protected]>
 
(...)
 
> > +static int rtk_clk_regmap_pll_mmc_phase_get_phase(struct clk_hw *hw)
> > +{
> > +     struct clk_hw *hwp;
> > +     struct rtk_clk_regmap_pll_mmc *clkm;
> > +     int phase_id;
> > +     int ret;
> > +     u32 val;
>
> Put in reverse Christmas tree order. You can combine the two int rows.
>
 
Ack.
 
> > +
> > +     hwp = clk_hw_get_parent(hw);
> > +     if (!hwp)
> > +             return -ENOENT;
> > +
> > +     clkm = to_clk_pll_mmc(hwp);
> > +     phase_id = (hw == &clkm->phase0_hw) ? 0 : 1;
> > +     ret = get_phsel(clkm, phase_id, &val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     val = DIV_ROUND_CLOSEST(val * CYCLE_DEGREES, PHASE_STEPS);
> > +
> > +     return val;
> > +}
> > +
> > +const struct clk_ops rtk_clk_pll_mmc_phase_ops = {
> > +     .set_phase = rtk_clk_regmap_pll_mmc_phase_set_phase,
> > +     .get_phase = rtk_clk_regmap_pll_mmc_phase_get_phase,
> > +};
> > +EXPORT_SYMBOL_NS_GPL(rtk_clk_pll_mmc_phase_ops, "REALTEK_CLK");
>
> Just looking at existing clk drivers, there's 5 that use
> EXPORT_SYMBOL_NS_GPL(). 3 of them use CLK_FOO, so let's just make this
> CLK_REALTEK for consistency with the other drivers. I like this because it's
> framework/clk subsystem. I'm not going to call this out on the other patches.
>
 
Ack. Will rename to CLK_REALTEK.
 
> > +
 
(...)
 
> > +static int rtk_clk_regmap_pll_mmc_set_rate(struct clk_hw *hw, unsigned
> long rate,
> > +                                        unsigned long parent_rate)
> {
> > +     struct rtk_clk_regmap_pll_mmc *clkm = to_clk_pll_mmc(hw);
> > +     u32 val = RTK_PLL_MMC_SSC_DIV_N_VAL;
> > +     int ret;
> > +
> > +     /*
> > +      * The 'rate' and 'parent_rate' are intentionally unused here.
> > +      *
> > +      * Despite receiving various rate requests (e.g., 26MHz, 52MHz,
> 200MHz),
> > +      * this function consistently configures the hardware for 27MHz
> (0x1b).
> > +      * This is because these settings reflect the input reference clock
> > +      * frequency to the SSCPLL, not the final PLL output frequency.
> > +      *
> > +      * The actual frequency division to achieve the requested eMMC rate
> > +      * is handled internally by the downstream eMMC host controller.
> > +      */
> > +
> > +     ret = regmap_update_bits(clkm->clkr.regmap,
> > +                              clkm->ssc_dig_ofs +
> RTK_PLL_SSC_DIG_EMMC1_OFFSET,
> > +                              RTK_PLL_FLAG_INITAL_EMMC_MASK,
> > +                              0x0 <<
> RTK_PLL_FLAG_INITAL_EMMC_SHIFT);
>
> determine_rate() above implies that this PLL can do multiple frequencies.
> There will be a mismatch between what the clk core thinks the frequency is
> compared to what's actually programmed in the hardware. Should
> determine_rate above also return the fixed rate as well?
>
> Brian
 
You're right. This PLL is actually configured to operate at a fixed
frequency for the MMC.
 
Will update determine_rate() like this:
 
    val = RTK_PLL_MMC_SSC_DIV_N_VAL + 2;
    req->rate = req->best_parent_rate / 4 * val +
                (req->best_parent_rate / 4 * 1517) / 8192;
 
Best Regards,
Yu-Chun