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

[Anirudh Srinivasan]

Hi Brian,

On Wed, Mar 4, 2026 at 4:21 PM Brian Masney <bmasney at redhat.com> wrote:
>
> Hi Anirudh,
>
> On Wed, Mar 04, 2026 at 11:53:38AM -0600, Anirudh Srinivasan wrote:
> > Sorry for the follow up, but wanted to run something by you.
> >
> > On Wed, Mar 4, 2026 at 10:40 AM Anirudh Srinivasan <
> > asrinivasan at oss.tenstorrent.com> wrote:
> > >
> > > Hello Brian,
> > >
> > > On Tue, Mar 3, 2026 at 4:32 PM Brian Masney <bmasney at redhat.com> 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.
> > > >
> >
> >
> > > > > +
> > > > > +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.
> >
> > This ends up becoming like this.
> >
> > static int atlantis_clk_pll_is_enabled(struct clk_hw *hw)
> >
> > {
> >
> >         struct atlantis_clk_pll *pll = hw_to_atlantis_pll(hw);
> >
> >
> >
> >
> >         /* Check if PLL is powered on, locked, not bypassed and Gate clk is
> > enabled */
> >         return regmap_test_bits(pll->common.regmap, pll->config.reg_offset,
> > PLL_CFG_LOCK_BIT) &&
> >
> >                 regmap_test_bits(pll->common.regmap,
> > pll->config.en_reg_offset, PLL_CFG_EN_BIT) &&
> >
> >                 regmap_test_bits(pll->common.regmap,
> > pll->config.cg_reg_offset, pll->config.cg_reg_enable) &&
> >
> >                 !regmap_test_bits(pll->common.regmap,
> > pll->config.reg_offset, PLL_CFG_BYPASS_BIT);
> >
> > }
> >
> > We can't use a single call of regmap_test_bits to the
> > pll->config.reg_offset register cause we need to check if PLL_CFG_LOCK_BIT
> > is set and PLL_CFG_BYPASS_BIT is unset. We end up needing to make 2 reads
> > to that register.
> >
> > Any thoughts on whether I should still be using regmap_test_bits here?
>
> If it's not looking good in practice once you implement it, then just
> fall back to the older behavior. It was just a suggestion.

Understood.

>
> I haven't used this yet, however there are also the regmap_field_xxx()
> helpers that are also available. drivers/clk/mstar/clk-msc313-mpll.c
> uses these helpers. I don't know if it would help to simplify your code.

Doesn't look like it'll help simplify things to me, so I'll keep things as is.

>
> Brian
>