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

[Anirudh Srinivasan]

Hello Brian

On Tue, Feb 17, 2026 at 10:14 AM Brian Masney <bmasney@xxxxxxxxxx> wrote:
>
> Hi Anirudh,
>
> On Mon, Feb 16, 2026 at 04:16:34PM -0600, Anirudh Srinivasan wrote:
> > Add driver for clock controller in Tenstorrent Atlantis SoC. This version
> > of the driver coves clocks from RCPU syscon.
>
> ...covers clocks..

Thank you for your comments. I will address the typos and add the
static modifier for the different variables that you suggested.

> > +
> > +struct atlantis_clk_gate_shared_config {
> > +     u32 reg_offset;
> > +     u32 enable;
> > +     unsigned int *share_count;
>
> Why is this a pointer? Could this just be a plain unsigned int since
> all occurrences of this are dereferenced?

We have a group of gate clocks that have a single enable bit shared
among them (instead of individual enable bits for each clock). We need
to keep track of the number of clocks within a group that have
requested an enable, and only unset the bit if all the clocks are
disabled. share_count is used to keep track of this. It gets updated
by each clock. Hence it's a pointer (and the mutexes around access to
it).

> > +static int atlantis_clk_gate_is_enabled(struct clk_hw *hw)
> > +{
> > +     struct atlantis_clk_gate *gate = hw_to_atlantis_clk_gate(hw);
> > +     u32 val;
> > +
> > +     regmap_read(gate->common.regmap, gate->config.reg_offset, &val);
> > +
> > +     val &= gate->config.enable;
> > +
> > +     return val ? 1 : 0;
>
> What do you think about this instead?
>
>     return !!val;

Ack

> > +static int atlantis_clk_gate_shared_enable(struct clk_hw *hw)
> > +{
> > +     struct atlantis_clk_gate_shared *gate =
> > +             hw_to_atlantis_clk_gate_shared(hw);
> > +     bool need_enable;
> > +     u32 reg;
> > +
> > +     scoped_guard(spinlock_irqsave, gate->config.refcount_lock)
> > +     {
> > +             need_enable = (*gate->config.share_count)++ == 0;
> > +             if (need_enable) {
> > +                     regmap_read(gate->common.regmap,
> > +                                 gate->config.reg_offset, &reg);
> > +                     reg |= gate->config.enable;
> > +                     regmap_write(gate->common.regmap,
> > +                                  gate->config.reg_offset, reg);
> > +             }
> > +     }
> > +
> > +     if (need_enable) {
> > +             regmap_read(gate->common.regmap, gate->config.reg_offset, &reg);
> > +
> > +             if (!(reg & gate->config.enable)) {
> > +                     pr_warn("%s: gate enable %d failed to enable\n",
> > +                             clk_hw_get_name(hw), gate->config.enable);
> > +                     return -EIO;
> > +             }
> > +     }
>
> Should this check be done within the scoped_guard?

The lock is used only for access to *gate->config.share_count. Since
we aren't reading that here, it isn't put inside the lock.

> > +static int atlantis_prcm_clocks_register(struct device *dev,
> > +                                      struct regmap *regmap,
> > +                                      const struct atlantis_prcm_data *data)
> > +{
> > +     struct clk_hw_onecell_data *clk_data;
> > +     int i, ret;
> > +     size_t num_clks = data->num;
> > +
> > +     clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, data->num),
> > +                             GFP_KERNEL);
> > +     if (!clk_data)
> > +             return -ENOMEM;
> > +
> > +     for (i = 0; i < data->num; i++) {
> > +             struct clk_hw *hw = data->hws[i];
> > +             const char *name = hw->init->name;
> > +             struct atlantis_clk_common *common =
> > +                     hw_to_atlantis_clk_common(hw);
>
> You can join these two lines into one and you'll be at 83 characters.
> checkpatch.pl now allows up to 100.

>
> > +             common->regmap = regmap;
> > +
> > +             ret = devm_clk_hw_register(dev, hw);
> > +
> > +             if (ret) {
> > +                     dev_err(dev, "Cannot register clock %d - %s\n", i,
> > +                             name);
>
> This will be at 81 characters if the lines are joined.

I used clang-format to do the formatting here and it seems to have
picked a line length of 80? I can change this

>
> However, bigger question is if this message should be dropped entirely? If
> this condition occurs, an error is logged here, and a second message will be
> logged in atlantis_prcm_probe() below.
>
> > +                     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);
> > +     if (ret)
> > +             dev_err(dev, "failed to add clock hardware provider (%d)\n",
> > +                     ret);
>
> Should this message also be dropped as well?

So you're suggesting that we just print a single error message instead
of multiple. I can change it to be like that.

>
> Brian
>