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

["Yu-Chun Lin [林祐君]" <eleanor.lin@realtek.com>]

Hi Jerome,

> > +
> > +static int rtk_clk_regmap_pll_mmc_prepare(struct clk_hw *hw) {
> > +     struct rtk_clk_regmap_pll_mmc *clkm = to_clk_pll_mmc(hw);
> > +
> > +     return set_pow_ctl(clkm, 7);
> > +}
> > +
> > +static void rtk_clk_regmap_pll_mmc_unprepare(struct clk_hw *hw) {
> > +     struct rtk_clk_regmap_pll_mmc *clkm = to_clk_pll_mmc(hw);
> > +
> > +     set_pow_ctl(clkm, 0);
> > +}
> > +
> > +static int rtk_clk_regmap_pll_mmc_is_prepared(struct clk_hw *hw) {
> > +     struct rtk_clk_regmap_pll_mmc *clkm = to_clk_pll_mmc(hw);
> > +     u32 val;
> > +     int ret;
> > +
> > +     ret = get_pow_ctl(clkm, &val);
> > +     if (ret)
> > +             return 1;
> > +
> > +     return val != 0x0;
> > +}
> > +
> > +static int rtk_clk_regmap_pll_mmc_enable(struct clk_hw *hw) {
> > +     struct rtk_clk_regmap_pll_mmc *clkm = to_clk_pll_mmc(hw);
> > +     int ret;
> > +
> > +     ret = set_phrt0(clkm, 1);
> > +     if (ret)
> > +             return ret;
> 
> looks fairly close to the regular gate as it stand.
> should your _pow be in there instead ?
> 

Will move the power-on/off operations into the .enable/.disable callbacks.

[...]

> > +
> > +static unsigned long rtk_clk_regmap_pll_mmc_recalc_rate(struct clk_hw
> *hw,
> > +                                                     unsigned
> long
> > +parent_rate) {
> > +     struct rtk_clk_regmap_pll_mmc *clkm = to_clk_pll_mmc(hw);
> > +     u32 val, ext_f;
> > +     u64 rate, base;
> > +     int ret;
> > +
> > +     ret = get_ssc_div_n(clkm, &val);
> > +     if (ret)
> > +             return 0;
> > +
> > +     ret = get_ssc_div_ext_f(clkm, &ext_f);
> > +     if (ret)
> > +             return 0;
> > +
> > +     base = parent_rate / 4;
> 
> So you've got a pre-divider
> 
> > +     rate = base * (val + 2);
> 
> Then an integer divider
> 
> > +     rate += div_u64(base * ext_f, 8192);
> 
> and finally the fractional part
> 
> > +
> > +     return rate;
> > +}
> > +
> > +static int rtk_clk_regmap_pll_mmc_determine_rate(struct clk_hw *hw,
> > +struct clk_rate_request *req) {
> > +     u32 val = RTK_PLL_MMC_SSC_DIV_N_VAL + 2;
> > +     u64 tmp;
> > +
> > +     if (!req->best_parent_rate)
> > +             return -EINVAL;
> > +
> > +     req->rate = req->best_parent_rate / 4 * val;
> 
> Here you seem to support only a single integer part setting
> 
> > +     tmp = (u64)(req->best_parent_rate / 4) * 1517;
> > +     req->rate += tmp >> 13;
> 
> So nothing says that here that the math won't overflow
> 
> Could you explain the 1517 here ? I suppose it is linked to the 8192 above ?
> 

The full formula is:
parent_rate / 4 * (val + 2) + (parent_rate / 4 * ext_f) / 8192;

To avoid hardcoding magic numbers,
We will introduce a rate table ('struct clk_pll_mmc_rate')

For example:
static const struct clk_pll_mmc_rate rtd1625_pll_emmc_rate_tbl[] = {
	{ .rate = 196999969, .ssc_div_n = 27,
	  .ssc_div_ext_f = 1517, .ncode = 27, .fcode = 321, .gran_est = 5985 },

The logic in rtk_clk_regmap_pll_mmc_determine_rate() will be updated to find
the closest match from this table, similar to the
'rtk_clk_regmap_pll_determine_rate' approach.

Additionally, in 'rtk_clk_regmap_pll_mmc_recalc_rate()', we will first
attempt to find an exact match in the rate table by comparing the full set
of register values. This aligns with our approach of relying on
characterized operating points, ensuring we return the exact validated
rate without integer truncation errors.

If no exact match is found, it will fall back to a helper function that
calculates the rate using the mathematical formula, casting variables to
u64 to prevent overflow.

> > +
> > +     return 0;
> > +}
> > +
> > +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).
> 
> Maybe you should fix the mmc driver then and properly request 27MHz if this
> what you expect of this particular PLL. Just taking any request and setting
> something else regardless is not OK.
>

With the introduction of the rate table (rtd1625_pll_emmc_rate_tbl) mentioned
above, the actual calculated output rate (e.g., 196999969) for the shared
27 MHz reference settings will be properly defined.

The driver will properly respect the requested rate, pick the corresponding
configuration from the rate table, and apply the exact register settings.

[...]

> > +}
> > +
> > +const struct clk_ops rtk_clk_pll_mmc_ops = {
> > +     .prepare          = rtk_clk_regmap_pll_mmc_prepare,
> > +     .unprepare        = rtk_clk_regmap_pll_mmc_unprepare,
> > +     .is_prepared      = rtk_clk_regmap_pll_mmc_is_prepared,
> 
> prepare is meant for slow (as in sleeping) operation.
> I've seen no sleeping operation in your implementation.
> If the regmap is slow, then there is problem because your using the same one
> in enable/disable() which run under a spin lock
> 

Agreed, as the above talk about. will remove these three callback implementaiton.
merge them to enable, disable and is_enable().

[...]

> > +struct rtk_clk_regmap_pll_mmc {
> > +     struct rtk_clk_regmap clkr;
> > +     unsigned int pll_ofs;
> > +     unsigned int ssc_dig_ofs;
> > +     struct clk_hw phase0_hw;
> > +     struct clk_hw phase1_hw;
> 
> your phases and pll use completly different ops. Why do you to mix the 2
> here ?
> 

Will create 'struct rtk_clk_pll_mmc_phase' for the phase clocks, separating
them entirely from the main 'struct rtk_clk_regmap_pll_mmc'.

Best Regards,
Yu-Chun

> --
> Jerome