Re: [PATCH v7 3/3] clk: tenstorrent: Add Atlantis clock controller driver

[Anirudh Srinivasan]

Hello Brian,

On Tue, Mar 3, 2026 at 4:32 PM Brian Masney <bmasney@xxxxxxxxxx> wrote:
>
> Hi Anirudh,
>
> Thanks for the patch. A few minor comments below with some minor
> nitpicks, additional places to use FIELD_GET(), plus some suggestions
> for additional regmap helpers to use.
>
> On Tue, Mar 03, 2026 at 11:36:09AM -0600, Anirudh Srinivasan wrote:
> > Add driver for clock controller in Tenstorrent Atlantis SoC. This version
> > of the driver covers clocks from RCPU subsystem.
> >
> > 5 types of clocks generated by this controller: PLLs (PLLs
> > with bypass functionality and an additional Gate clk at output), Shared
> > Gates (Multiple Gate clks that share an enable bit), standard Muxes,
> > Dividers and Gates. All clocks are implemented using custom clk ops and
> > use the regmap interface associated with the syscon. All clocks are derived
> > from a 24 Mhz oscillator.
> >
> > The reset controller is also setup as an auxiliary device of the clock
> > controller.
> >
> > Signed-off-by: Anirudh Srinivasan <asrinivasan@xxxxxxxxxxxxxxxxxxx>

> > +
> > +static u8 atlantis_clk_mux_get_parent(struct clk_hw *hw)
> > +{
> > +     struct atlantis_clk_mux *mux = hw_to_atlantis_clk_mux(hw);
> > +     u32 val;
> > +
> > +     regmap_read(mux->common.regmap, mux->config.reg_offset, &val);
> > +     val >>= mux->config.shift;
> > +     val &= (BIT(mux->config.width) - 1);
> > +
> > +     return val;
>
> FIELD_GET() ?

<copying another snippet from below>

> > +static unsigned long atlantis_clk_divider_recalc_rate(struct clk_hw *hw,
> > +                                                   unsigned long parent_rate)
> > +{
> > +     struct atlantis_clk_divider *divider = hw_to_atlantis_clk_divider(hw);
> > +     u32 val;
> > +
> > +     regmap_read(divider->common.regmap, divider->config.reg_offset, &val);
> > +
> > +     val >>= divider->config.shift;
> > +     val &= ((1 << (divider->config.width)) - 1);
>
> FIELD_GET() ?

Most uses of FIELD_GET I see have the shift and width for the mask be
static compile time constants, which isn't the case here. I don't
think it makes sense to use it here. You could manually compute the
mask and pass that to FIELD_GET, but you end up with more lines of
code at that point

>
> > +}
> > +
> > +static int atlantis_clk_mux_set_parent(struct clk_hw *hw, u8 index)
> > +{
> > +     struct atlantis_clk_mux *mux = hw_to_atlantis_clk_mux(hw);
> > +     u32 val = index;
> > +
> > +     return regmap_update_bits(mux->common.regmap, mux->config.reg_offset,
> > +                               (BIT(mux->config.width) - 1)
> > +                                       << mux->config.shift,
>
> This doesn't make the line that much longer, and is nicer to read:
>
>     (BIT(mux->config.width) - 1) << mux->config.shift,

Ack

> > +static void atlantis_clk_gate_endisable(struct clk_hw *hw, int enable)
>
> Should this return an int? This looks like we should use this return
> value below in atlantis_clk_gate_enable().
>
> > +{
> > +     struct atlantis_clk_gate *gate = hw_to_atlantis_clk_gate(hw);
> > +     u32 val;
> > +
> > +     if (enable)
> > +             val = gate->config.enable;
> > +     else
> > +             val = ~(gate->config.enable);
> > +
> > +     regmap_update_bits(gate->common.regmap, gate->config.reg_offset,
> > +                        gate->config.enable, val);
>
> This chunk could be simplified to use regmap_set_bits() and
> regmap_clear_bits().
>
> > +}
> > +
> > +static int atlantis_clk_gate_enable(struct clk_hw *hw)
> > +{
> > +     atlantis_clk_gate_endisable(hw, 1);
> > +
> > +     return 0;
>
> Follow up from above. Any reason why the return value of
> regmap_update_bits() in atlantis_clk_gate_endisable() is discarded?
>
> I know it's not used below in the disable().

For some reason, clk_gate_enable returns an int (I guess we care about
errors here), while clk_gate_disable doesn't return anything (but we
don't here?). I've updated them (and clk_get_endisable) based on your
suggestions so that the return code from regmap_{set|clear}_bits is
passed through.

> > +
> > +static int atlantis_clk_pll_is_enabled(struct clk_hw *hw)
> > +{
> > +     struct atlantis_clk_pll *pll = hw_to_atlantis_pll(hw);
> > +     u32 val, en_val, cg_val;
> > +
> > +     regmap_read(pll->common.regmap, pll->config.reg_offset, &val);
> > +     regmap_read(pll->common.regmap, pll->config.en_reg_offset, &en_val);
> > +     regmap_read(pll->common.regmap, pll->config.cg_reg_offset, &cg_val);
> > +
> > +     /* Check if PLL is powered on, locked, not bypassed and Gate clk is enabled */
> > +     return !!(en_val & PLL_CFG_EN_BIT) && !!(val & PLL_CFG_LOCK_BIT) &&
> > +            (!pll->config.cg_reg_enable || (cg_val & pll->config.cg_reg_enable)) &&
> > +            !(val & PLL_CFG_BYPASS_BIT);
>
> Could regmap_test_bits() make this a bit cleaner?
>
> > +}
> > +
> > +static int atlantis_clk_pll_enable(struct clk_hw *hw)
> > +{
> > +     struct atlantis_clk_pll *pll = hw_to_atlantis_pll(hw);
> > +     u32 val, en_val, cg_val;
> > +     int ret;
> > +
> > +     regmap_read(pll->common.regmap, pll->config.reg_offset, &val);
> > +     regmap_read(pll->common.regmap, pll->config.en_reg_offset, &en_val);
> > +     regmap_read(pll->common.regmap, pll->config.cg_reg_offset, &cg_val);
> > +
> > +     /* Check if PLL is already enabled, locked, not bypassed and Gate clk is enabled */
> > +     if ((en_val & PLL_CFG_EN_BIT) && (val & PLL_CFG_LOCK_BIT) &&
> > +         (!pll->config.cg_reg_enable || (cg_val & pll->config.cg_reg_enable)) &&
> > +         !(val & PLL_CFG_BYPASS_BIT)) {
>
> Same about regmap_test_bits() here.

These instances have it reading 3 different registers (unlike almost
all the other examples that just read one) and testing bits across
them. But I guess it should be possible to use test_bits here too. I
will update them.

>
> > +             return 0;
> > +     }
> > +
> > +     /* Step 1: Set bypass mode first */
> > +     regmap_update_bits(pll->common.regmap, pll->config.reg_offset,
> > +                        PLL_CFG_BYPASS_BIT, PLL_CFG_BYPASS_BIT);
> > +
> > +     /* Step 2: Enable PLL (clear then set power bit) */
> > +     regmap_update_bits(pll->common.regmap, pll->config.en_reg_offset,
> > +                        PLL_CFG_EN_BIT, 0);
> > +
> > +     regmap_update_bits(pll->common.regmap, pll->config.en_reg_offset,
> > +                        PLL_CFG_EN_BIT, PLL_CFG_EN_BIT);
> > +
> > +     /* Step 3: Wait for PLL lock */
> > +     ret = regmap_read_poll_timeout(pll->common.regmap,
> > +                                    pll->config.reg_offset, val,
> > +                                    val & PLL_CFG_LOCK_BIT, 10,
> > +                                    PLL_BYPASS_WAIT_US);
>
> Should the last two parameters be
> PLL_BYPASS_WAIT_US, PLL_LOCK_TIMEOUT_US instead of
> 10, PLL_BYPASS_WAIT_US?

Yes, thanks for catching this.

> > +
> > +     for (i = 0; i < data->num; i++) {
> > +             struct clk_hw *hw = data->hws[i];
> > +             struct atlantis_clk_common *common =
> > +                     hw_to_atlantis_clk_common(hw);
> > +             common->regmap = regmap;
> > +
> > +             ret = devm_clk_hw_register(dev, hw);
> > +
>
> Remove newline
>
> > +             if (ret)
> > +                     return ret;
> > +
> > +             clk_data->hws[common->clkid] = hw;
> > +     }
> > +
> > +     clk_data->num = num_clks;
> > +
> > +     ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
> > +
> > +     return ret;
>
> These 3 lines can be simplified to return devm_..();

Done

Thanks for your comments. I've incorporated  the changes you suggested
on using regmap helpers. I left some comments on the use of FIELD_GET.
Let me know if you have any other suggestions. I'll send an updated
version by the end of this week.

>
> Brian
>