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

[Brian Masney <bmasney@redhat.com>]

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@oss.tenstorrent.com> wrote:
> >
> > Hello Brian,
> >
> > On Tue, Mar 3, 2026 at 4:32 PM Brian Masney <bmasney@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.

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.

Brian