RE: [PATCH v16 6/9] clk: realtek: Add support for MMC-tuned PLL clocks

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

Hi Brian,

> > +
> > +static const struct rtk_clk_pll_mmc_rate *
> > +rtk_clk_pll_mmc_find_closest_rate(const struct rtk_clk_pll_mmc_rate *tbl,
> u32 num,
> > +                               unsigned long rate, unsigned long
> min_rate,
> > +                               unsigned long max_rate) {
> > +     const struct rtk_clk_pll_mmc_rate *best = NULL;
> > +     unsigned long best_delta = ~0UL;
> > +
> > +     for (unsigned int i = 0; i < num; i++) {
> > +             const struct rtk_clk_pll_mmc_rate *entry = &tbl[i];
> > +             unsigned long delta;
> > +
> > +             if (entry->rate < min_rate || entry->rate > max_rate)
> > +                     continue;
> > +
> > +             delta = entry->rate >= rate ? entry->rate - rate : rate -
> entry->rate;
> > +             if (delta < best_delta) {
> > +                     best = entry;
> > +                     best_delta = delta;
> > +             }
> > +     }
> > +
> > +     return best;
> > +}
> 
> This duplicates the functionality in ftbl_find_closest_by_rate() from patch 3.
> 

I considered sharing the logic, but the two tables have different entry
layouts.

Unifying them would need a macro or (void *) (both lose type checking), or
embedding rtk_freq_table with container_of() (a larger rework for little
gain). I prefer keeping the two type-safe functions. Is that acceptable?

> > +
> > +static int rtk_clk_regmap_pll_mmc_determine_rate(struct clk_hw *hw,
> > +                                              struct
> clk_rate_request
> > +*req) {
> > +     struct rtk_clk_regmap_pll_mmc *clkm = to_clk_pll_mmc(hw);
> > +     const struct rtk_clk_pll_mmc_rate *entry;
> > +
> > +     if (!clkm->rate_tbl || !clkm->num_rate_tbl ||
> > +         req->min_rate > req->max_rate)
> 
> clk_core_check_boundaries() already checks the min/max rate.
> 

Ack. Will drop.

> > +             return -EINVAL;
> > +
> > +     entry = rtk_clk_pll_mmc_find_closest_rate(clkm->rate_tbl,
> > +
> clkm->num_rate_tbl, req->rate,
> > +                                               req->min_rate,
> req->max_rate);
> > +     if (!entry)
> > +             return -EINVAL;
> > +
> > +     req->rate = entry->rate;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct rtk_clk_pll_mmc_div_cfg clk_pll_emmc_default_div_cfg[]
> = {
> > +     { 31, 46, 3, 3, 2 },
> > +     { 20, 30, 2, 3, 1 },
> > +     { 10, 19, 1, 2, 1 },
> > +     {  5,  9, 0, 2, 0 },
> > +};
> > +
> > +static const struct rtk_clk_pll_mmc_div_cfg *
> > +clk_pll_emmc_find_div_cfg(struct rtk_clk_regmap_pll_mmc *clkm, u32
> > +div) {
> > +     const struct rtk_clk_pll_mmc_div_cfg *tbl = clkm->div_cfg;
> > +     u8 num = clkm->num_div_cfg;
> 
> This is a u32 in struct rtk_clk_regmap_pll_mmc.

Yes, will fix.

[...]

> > +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);
> > +     const struct rtk_clk_pll_mmc_rate *entry =
> rtk_clk_pll_mmc_find_rate(clkm, rate);
> > +     int ret;
> > +
> > +     if (!entry)
> > +             return -EINVAL;
> > +
> > +     ret = regmap_update_bits(clkm->clkr.regmap,
> > +                              clkm->ssc_dig_ofs +
> RTK_PLL_SSC_DIG_EMMC0_OFFSET,
> > +                              RTK_PLL_FLAG_INITIAL_EMMC_MASK,
> > +
> FIELD_PREP(RTK_PLL_FLAG_INITIAL_EMMC_MASK, 0));
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = clk_pll_emmc_set_ssc_div_n(clkm, entry->ssc_div_n);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = clk_pll_emmc_set_ssc_div_ext_f(clkm, entry->ssc_div_ext_f);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = clk_pll_emmc_program_div_cfg(clkm, entry->ssc_div_n);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_update_bits(clkm->clkr.regmap,
> > +                              clkm->ssc_dig_ofs +
> RTK_PLL_SSC_DIG_EMMC3_OFFSET,
> > +                              RTK_PLL_NCODE_SSC_EMMC_MASK,
> > +
> FIELD_PREP(RTK_PLL_NCODE_SSC_EMMC_MASK, entry->ncode));
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_update_bits(clkm->clkr.regmap,
> > +                              clkm->ssc_dig_ofs +
> RTK_PLL_SSC_DIG_EMMC3_OFFSET,
> > +                              RTK_PLL_FCODE_SSC_EMMC_MASK,
> > +
> FIELD_PREP(RTK_PLL_FCODE_SSC_EMMC_MASK, entry->fcode));
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_update_bits(clkm->clkr.regmap,
> > +                              clkm->ssc_dig_ofs +
> RTK_PLL_SSC_DIG_EMMC4_OFFSET,
> > +                              RTK_PLL_GRAN_EST_EM_MC_MASK,
> > +
> FIELD_PREP(RTK_PLL_GRAN_EST_EM_MC_MASK, entry->gran_est));
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_update_bits(clkm->clkr.regmap,
> > +                              clkm->ssc_dig_ofs +
> RTK_PLL_SSC_DIG_EMMC0_OFFSET,
> > +                              RTK_PLL_SSC_DIG_EMMC0_EN_SSC,
> > +
> FIELD_PREP(RTK_PLL_SSC_DIG_EMMC0_EN_SSC, 1));
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_update_bits(clkm->clkr.regmap,
> > +                              clkm->ssc_dig_ofs +
> RTK_PLL_SSC_DIG_EMMC0_OFFSET,
> > +                              RTK_PLL_EN_SSC_EMMC_MASK,
> > +
> FIELD_PREP(RTK_PLL_EN_SSC_EMMC_MASK, 1));
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_update_bits(clkm->clkr.regmap,
> > +                              clkm->ssc_dig_ofs +
> RTK_PLL_SSC_DIG_EMMC0_OFFSET,
> > +                              RTK_PLL_EN_SSC_EMMC_MASK,
> > +
> FIELD_PREP(RTK_PLL_EN_SSC_EMMC_MASK,
> > + 0));
> 
> EN_SSC_EMMC is set and then immediately cleared. If this is intentional then
> put a comment here to be sure it's intentional.
> 

Sorry for the confusion, the macro name was wrong:
RTK_PLL_EN_SSC_EMMC_MASK should be RTK_PLL_SSC_DIG_EMMC0_OC_EN.

OC_EN is a trigger bit (enables over-clock or under-clock adjustment),
not the SSC enable. It needs to be set and then cleared to trigger
the adjustment.

> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_update_bits(clkm->clkr.regmap,
> > +                              clkm->ssc_dig_ofs +
> RTK_PLL_SSC_DIG_EMMC0_OFFSET,
> > +                              RTK_PLL_FLAG_INITIAL_EMMC_MASK,
> > +
> FIELD_PREP(RTK_PLL_FLAG_INITIAL_EMMC_MASK, 1));
> > +     if (ret)
> > +             return ret;
> > +
> > +     usleep_range(10, 20);
> > +
> > +     return 0;
> > +}
> > +

[...]

> > +
> > +struct rtk_clk_regmap_pll_mmc {
> > +     struct rtk_clk_regmap clkr;
> > +     unsigned int pll_ofs;
> > +     unsigned int ssc_dig_ofs;
> > +
> > +     /*
> > +      * Optional per-SoC tuning table. When NULL, the driver's default
> table
> > +      * is used and num_div_cfg is ignored;
> 
> Replace ; at end with .

Ack.

Best Regards,
Yu-Chun