Message-ID: <CA+Oz1=a65HvfXHWjeSq4Ubq=5kzHp9pkLJVr77hvTYAGFHv0Mg@mail.gmail.com>
Date: Thu, 26 Jun 2025 12:32:14 +0530
From: Rahul Pathak <rpathak@...tanamicro.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Anup Patel <apatel@...tanamicro.com>, Michael Turquette <mturquette@...libre.com>, 
	Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jassi Brar <jassisinghbrar@...il.com>, Thomas Gleixner <tglx@...utronix.de>, 
	"Rafael J . Wysocki" <rafael@...nel.org>, Mika Westerberg <mika.westerberg@...ux.intel.com>, 
	Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>, 
	Uwe Kleine-König <ukleinek@...nel.org>, 
	Palmer Dabbelt <palmer@...belt.com>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Len Brown <lenb@...nel.org>, Sunil V L <sunilvl@...tanamicro.com>, 
	Leyfoon Tan <leyfoon.tan@...rfivetech.com>, Atish Patra <atish.patra@...ux.dev>, 
	Andrew Jones <ajones@...tanamicro.com>, Samuel Holland <samuel.holland@...ive.com>, 
	Anup Patel <anup@...infault.org>, linux-clk@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 09/23] clk: Add clock driver for the RISC-V RPMI clock
 service group

On Mon, Jun 23, 2025 at 2:36 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Wed, Jun 18, 2025 at 05:43:44PM +0530, Anup Patel wrote:
> > From: Rahul Pathak <rpathak@...tanamicro.com>
> >
> > The RPMI specification defines a clock service group which can be
> > accessed via SBI MPXY extension or dedicated S-mode RPMI transport.
> >
> > Add mailbox client based clock driver for the RISC-V RPMI clock
> > service group.
>
> ...
>
> ...
>
> > +enum rpmi_clk_config {
> > +     RPMI_CLK_DISABLE = 0,
> > +     RPMI_CLK_ENABLE = 1
>
> It's still unclear if this enum can be expanded in the future (and no, you may
> not answer this either). Hence, to reduce potential churn in the future, leave
> the trailing comma here.
>

Ok, will update

> > +};
>
> ...
>
> > +union rpmi_clk_rates {
> > +     u64 discrete[RPMI_CLK_DISCRETE_MAX_NUM_RATES];
> > +     struct {
> > +             u64 min;
> > +             u64 max;
> > +             u64 step;
> > +     } linear;
>
> Have you looked at the linear_range.h? Why can it not be (re-)used here?
>

I did the first time only when you commented. And i dont see any
benefit in that.
linear_range has slightly different way to access any value using `sel`.
Here this union represents how RPMI protocol represents the rates and
reusing linear_range will only introduce conversion to and fro.

> > +};
>
> ...
>
> > +static u32 rpmi_clk_get_num_clocks(struct rpmi_clk_context *context)
> > +{
> > +     struct rpmi_get_num_clocks_rx rx;
> > +     struct rpmi_mbox_message msg;
> > +     int ret;
> > +
> > +     rpmi_mbox_init_send_with_response(&msg, RPMI_CLK_SRV_GET_NUM_CLOCKS,
> > +                                       NULL, 0, &rx, sizeof(rx));
>
> ...here
>
> > +     ret = rpmi_mbox_send_message(context->chan, &msg);
> > +
>
> This blank line should be rather ^^^

Sure, I will update.

>
> > +     if (ret || rx.status)
> > +             return 0;
>
> Why rx.status can't be checked before calling to a sending message?
> Sounds like the rpmi_mbox_init_send_with_response() links rx to msg somehow.
> If this is the case, use msg here, otherwise move the check to be in the
> correct place.

Yes, the rpmi_mbox_init_send_with_response is a helper function which links
the rx to msg. It's a very simple function which only performs assignments.

Using msg instead of rx directly will require additional typecasting
which will only clutter
I can add a comment if that helps wherever the rpmi_mbox_init_send_with_response
is used.

>
> Seems the same question to the all similar checks in the code.
>
> > +     return le32_to_cpu(rx.num_clocks);
> > +}
>
> ...
>
> > +static int rpmi_clk_get_supported_rates(u32 clkid, struct rpmi_clk *rpmi_clk)
> > +{
> > +     struct rpmi_clk_context *context = rpmi_clk->context;
> > +     struct rpmi_clk_rate_discrete *rate_discrete;
> > +     struct rpmi_clk_rate_linear *rate_linear;
> > +     struct rpmi_get_supp_rates_rx *rx __free(kfree) = NULL;
> > +     struct rpmi_get_supp_rates_tx tx;
> > +     struct rpmi_mbox_message msg;
>
> > +     size_t clk_rate_idx = 0;
>
> This kind of assignments is hard to maintain and it's mistake prone in case
> some additional code is injected in the future that might reuse it.
>
I dont understand what is the problem with this assignment. If any
code added in the future reuse it then it has to make sure that
clk_rate_idx has the correct initial value before any further references.

> > +     int ret, rateidx, j;
> > +

...

> > +static void rpmi_clk_disable(struct clk_hw *hw)
> > +{
> > +     struct rpmi_clk *rpmi_clk = to_rpmi_clk(hw);
> > +     struct rpmi_clk_context *context = rpmi_clk->context;
> > +     struct rpmi_mbox_message msg;
> > +     struct rpmi_set_config_tx tx;
> > +     struct rpmi_set_config_rx rx;
> > +     int ret;
> > +
> > +     tx.config = cpu_to_le32(RPMI_CLK_DISABLE);
> > +     tx.clkid = cpu_to_le32(rpmi_clk->id);
> > +
> > +     rpmi_mbox_init_send_with_response(&msg, RPMI_CLK_SRV_SET_CONFIG,
> > +                                       &tx, sizeof(tx), &rx, sizeof(rx));
> > +     ret = rpmi_mbox_send_message(context->chan, &msg);
> > +     if (ret || rx.status)
> > +             pr_err("Failed to disable clk-%u\n", rpmi_clk->id);
>
> Close to useless message. You may improve it by splitting to two and printing
> rx.status in one and ret in the other with different text. Or drop it.

Sure, I think it's better to keep and split it into two as you suggest
instead of dropping.

>
> > +}
>
> > +static int rpmi_clk_probe(struct platform_device *pdev)
> > +{
> > +     int ret;
> > +     unsigned int num_clocks, i;
> > +     struct clk_hw_onecell_data *clk_data;
> > +     struct rpmi_clk_context *context;
> > +     struct rpmi_mbox_message msg;
> > +     struct clk_hw *hw_ptr;
> > +     struct device *dev = &pdev->dev;
> > +
> > +     context = devm_kzalloc(dev, sizeof(*context), GFP_KERNEL);
> > +     if (!context)
> > +             return -ENOMEM;
> > +     context->dev = dev;
> > +     platform_set_drvdata(pdev, context);
> > +
> > +     context->client.dev             = context->dev;
> > +     context->client.rx_callback     = NULL;
> > +     context->client.tx_block        = false;
> > +     context->client.knows_txdone    = true;
> > +     context->client.tx_tout         = 0;
> > +
> > +     context->chan = mbox_request_channel(&context->client, 0);
> > +     if (IS_ERR(context->chan))
> > +             return PTR_ERR(context->chan);
>
> Here is an incorrect order of the freeing resources. Besides that, wrapping the
> mbox_free_channel() into managed resources reduces this code by more
> than 10 LoCs! At bare minimum if will fix the bug,

Understood. So we can use devm_add_action_or_reset to link a release function
with the context->chan. Is this what you are suggesting? This will also make
the .remove callback redundant which can be removed.

...

> > +
> > +     rpmi_mbox_init_get_attribute(&msg, RPMI_MBOX_ATTR_MAX_MSG_DATA_SIZE);
> > +     ret = rpmi_mbox_send_message(context->chan, &msg);
> > +     if (ret) {
> > +             mbox_free_channel(context->chan);
> > +             return dev_err_probe(dev, ret, "Failed to get max message data size\n");
> > +     }
> > +
> > +     context->max_msg_data_size = msg.attr.value;
> > +     num_clocks = rpmi_clk_get_num_clocks(context);
> > +     if (!num_clocks) {
> > +             mbox_free_channel(context->chan);
> > +             return dev_err_probe(dev, -ENODEV, "No clocks found\n");
> > +     }
> > +
> > +     clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, num_clocks),
> > +                             GFP_KERNEL);
>
> (The above mention problem comes here after the successful allocation of
>  clk_data but failing of any further code.

Once the change mentioned in above comment will be done this will take
care of the rest of the exit scenarios.

>
> > +     if (!clk_data) {
> > +             mbox_free_channel(context->chan);
> > +             return dev_err_probe(dev, -ENOMEM, "No memory for clock data\n");
> > +     }
> > +     clk_data->num = num_clocks;
> > +
> > +     for (i = 0; i < clk_data->num; i++) {
> > +             hw_ptr = rpmi_clk_enumerate(context, i);
> > +             if (IS_ERR(hw_ptr)) {
> > +                     mbox_free_channel(context->chan);
> > +                     return dev_err_probe(dev, PTR_ERR(hw_ptr),
> > +                                          "failed to register clk-%d\n", i);
> > +             }
> > +             clk_data->hws[i] = hw_ptr;
> > +     }
> > +
> > +     ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
> > +     if (ret) {
> > +             mbox_free_channel(context->chan);
> > +             return dev_err_probe(dev, ret, "failed to register clock HW provider\n");
> > +     }
> > +
> > +     return 0;
> > +}
>
> ...
>
> > +static void rpmi_clk_remove(struct platform_device *pdev)
> > +{
> > +     struct rpmi_clk_context *context = platform_get_drvdata(pdev);
> > +
> > +     mbox_free_channel(context->chan);
> > +}
>
> This function will be gone. See above.

Agree

>
> --
> With Best Regards,
> Andy Shevchenko
>
>


-- 

Thanks
Rahul Pathak

Powered by blists - more mailing lists