RE: [PATCH v15 05/11] clk: realtek: Add support for phase locked loops (PLLs)

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

Hi Jerome,

> > +static inline struct rtk_clk_regmap_pll *to_rtk_clk_regmap_pll(struct
> > +clk_hw *hw) {
> > +     struct rtk_clk_regmap *clkr = to_rtk_clk_regmap(hw);
> > +
> > +     return container_of(clkr, struct rtk_clk_regmap_pll, clkr); }
> > +
> > +static int wait_freq_ready(struct rtk_clk_regmap_pll *clkp) {
> > +     u32 pollval;
> > +
> > +     /* reg == 0 means not configured.
> > +      * Register offset 0 is never a valid address on Realtek SoCs.
> 
> Fix the comment style please
> 

Ack.

> > +      */
> > +     if (!clkp->freq_ready_reg)
> > +             return 0;
> 
> Do you need to protect against your own driver providing bad offset ?
> 

Will change the comment to

/*
 * Optional register: return success if not provided.
 */

> > +
> > +     return regmap_read_poll_timeout_atomic(clkp->clkr.regmap,
> clkp->freq_ready_reg, pollval,
> > +             (pollval & clkp->freq_ready_mask) ==
> > +clkp->freq_ready_val, 1, TIMEOUT); }
> > +
> > +static bool is_power_on(struct rtk_clk_regmap_pll *clkp) {
> > +     u32 val;
> > +
> > +     /* reg == 0 means not configured (assume always on).
> > +      * Register offset 0 is never a valid address on Realtek SoCs.
> > +      */
> 
> You've already said this. It would be more helpful to tell us that PLL that does
> not provider a power reg is considered to be always powered, if that is the case
> 

Will change the comment to 

/*
 * If the PLL does not provide a power register, it is considered
 * to be always powered.
 */


> > +     if (!clkp->power_reg)
> > +             return true;
> > +
> > +     if (regmap_read(clkp->clkr.regmap, clkp->power_reg, &val))
> > +             return false;
> > +
> > +     return (val & clkp->power_mask) == clkp->power_val_on; }
> > +
> > +static void rtk_clk_regmap_pll_disable(struct clk_hw *hw) {
> > +     struct rtk_clk_regmap_pll *clkp = to_rtk_clk_regmap_pll(hw);
> > +     unsigned long flags;
> > +
> > +     if (!clkp->seq_power_off)
> > +             return;
> > +
> > +     spin_lock_irqsave(&clkp->lock, flags);
> 
> What do you protect with this ?
> regmap does provide some locking already and CCF too. Do you expect
> something to poke the registers you are writing ?
> 

This spinlock was added based on Brian's feedback [1] to prevent a race
condition between '.set_rate()' and '.enable()'/'.disable()'.

Since our '.set_rate()' involves a multi-step register sequence and a
polling loop, an interleaved '.enable()' or '.disable()' operation could
disrupt the hardware state. Thus, we need this shared spinlock to serialize
them.

[1] https://lore.kernel.org/lkml/ac_SX1UJRqiBH2iM@redhat.com/


> > +
> > +     regmap_multi_reg_write(clkp->clkr.regmap, clkp->seq_power_off,
> > +                            clkp->num_seq_power_off);
> > +
> > +     spin_unlock_irqrestore(&clkp->lock, flags); }
> > +
> > +static int rtk_clk_regmap_pll_is_enabled(struct clk_hw *hw) {
> > +     struct rtk_clk_regmap_pll *clkp = to_rtk_clk_regmap_pll(hw);
> > +     unsigned long flags;
> > +     int ret;
> > +
> > +     spin_lock_irqsave(&clkp->lock, flags);
> > +
> > +     ret = is_power_on(clkp);
> > +
> > +     spin_unlock_irqrestore(&clkp->lock, flags);
> > +
> > +     return ret;
> > +}
> > +
> > +static int rtk_clk_regmap_pll_determine_rate(struct clk_hw *hw,
> > +                                          struct clk_rate_request
> > +*req) {
> > +     struct rtk_clk_regmap_pll *clkp = to_rtk_clk_regmap_pll(hw);
> > +     const struct rtk_freq_table *ftblv = NULL;
> > +
> > +     ftblv = ftbl_find_by_rate(clkp->freq_tbl, req->rate);
> 
> Doesn't this completly disregard the parent rate ?
> 

Yes, the lookup works on the programmed code, not on the parent rate.
In our hardware, these PLLs are hardwired to a single 27MHz crystal
("osc27m") without any mux.
Therefore, the "parent rate" is a fixed property of the hardware rather
than dynamically selectable source, which is why the code operates on
predefined values.

> I understand that SoC vendor may expect a single frequency as input but it it is
> pretty fragile to do so.
> 

Agreed. will add an explicit check against the expected parent rate. This
ensures that if a board declares a different oscillator, it will fail safely
and explicitly, rather than silently programming unvalidated values.

> A PLL is usually some sort of (fractional) multiplier.
> It is way better to provide the table of multiplier you support, along with their
> sequence and properly doing the calculation here.
> 
> > +
> > +     if (ftblv && ftblv->rate >= req->min_rate) {
> > +             req->rate = ftblv->rate;
> > +             return 0;
> > +     }
> > +
> > +     /* floor result is below min_rate; find the smallest rate >= min_rate
> */
> > +     ftblv = ftbl_find_ceil_by_rate(clkp->freq_tbl, req->min_rate);
> > +     if (!ftblv || ftblv->rate > req->max_rate)
> > +             return -EINVAL;
> 
> This is not how determine_rate() works. You just round to the closest rate the
> PLL supports. You are free to round as you wish (bellow or closest)
> 
> Just test the different multipliers you support and return what is best compared
> to the request. There is no reason to return EINVAL for this.
> 

Will drop the complex floor-then-ceil search and do a plain closest match
within the '[min_rate, max_rate]' boundaries.

Regarding the multiplier table:
We prefer to keep this as a predefined list rather than deriving a new point
from an arbitrary rate. This is because these rows in 'freq_tbl[]' are the
exact operating points where the surrounding sequences (power, SSC, ready
flow) were validated.

We hope this design choice makes sense to you.

Therefore, the updated logic will look like this:

static int rtk_clk_regmap_pll_determine_rate(struct clk_hw *hw,
                                             struct clk_rate_request *req)
{
        struct rtk_clk_regmap_pll *clkp = to_rtk_clk_regmap_pll(hw);
        const struct rtk_freq_table *ftblv = NULL;

        if (clkp->expected_parent_rate &&
            req->best_parent_rate != clkp->expected_parent_rate)
                return -EINVAL;

        ftblv = ftbl_find_closest_by_rate(clkp->freq_tbl, req->rate,
                                          req->min_rate, req->max_rate);
        if (!ftblv)
                return -EINVAL;

        req->rate = ftblv->rate;

        return 0;
}

> > +
> > +     req->rate = ftblv->rate;
> > +
> > +     return 0;
> > +}
> > +

[...]

> > +EXPORT_SYMBOL_NS_GPL(rtk_clk_pll_ops, "CLK_REALTEK");
> > +
> > +const struct clk_ops rtk_clk_pll_ro_ops = {
> > +     .recalc_rate = rtk_clk_regmap_pll_recalc_rate,
> 
> is_enabled ?

Ack. Will add.

Best Regards,
Yu-Chun
> 
> --
> Jerome