[PATCH v3 10/23] clk: Add clock driver for the RISC-V RPMI clock service group

[Rahul Pathak]

On Mon, May 12, 2025 at 12:38 PM Andy Shevchenko
<andriy.shevchenko at linux.intel.com> wrote:
>
> On Sun, May 11, 2025 at 07:09:26PM +0530, Anup Patel wrote:
> > From: Rahul Pathak <rpathak at ventanamicro.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.
>
> ...
>
> > +#include <linux/clk-provider.h>
> > +#include <linux/err.h>
> > +#include <linux/mailbox_client.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/mailbox/riscv-rpmi-message.h>
>
> Just to point out again that the above misses a lot of headers definitions
> and/or APIs this driver uses. Follow IWYU principle.

Sure

>
> ...
>
> > +#define GET_RATE_U64(hi_u32, lo_u32) ((u64)(hi_u32) << 32 | (lo_u32))
>
> Hmm... Perhaps add this kind of macro to wordpart.h ? IIRC not only this driver
> uses something like this.
>

Sure, it makes sense.

> ...
>
> > +enum rpmi_clk_type {
> > +     RPMI_CLK_DISCRETE = 0,
> > +     RPMI_CLK_LINEAR = 1,
>
> > +     RPMI_CLK_TYPE_MAX_IDX,
>
> No comma for the terminator. Please, clean all these cases.

Sure

>
> > +};
>
> ...
>
> > +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 ranges library we have in the kernel? Can you
> utilise it here?

Ok, i will check

>
> > +};
>
> ...
>
> > +struct rpmi_clk {
> > +     struct rpmi_clk_context *context;
> > +     u32 id;
> > +     u32 num_rates;
> > +     u32 transition_latency;
> > +     enum rpmi_clk_type type;
> > +     union rpmi_clk_rates *rates;
> > +     char name[RPMI_CLK_NAME_LEN];
> > +     struct clk_hw hw;
>
> Just a reminder to use `pahole` to check that your data layout is optimised for
> memory consumption.

Ok, i will check

>
> > +};
>
> ...
>
> > +struct rpmi_get_supp_rates_rx {
> > +     u32 status;
> > +     u32 flags;
> > +     u32 remaining;
> > +     u32 returned;
> > +     u32 rates[];
> > +};
>
> Is it ABI? (I mean if this is interface with some kind of FW)
> If so, Use proper endianess aware types. Same Q for all data
> types defined in this driver.

Sure.

It's the message format defined by the RISC-V RPMI spec.


>
> ...
>
> > +                     for (j = 0; j < rx->returned; j++) {
> > +                             if (rateidx >= (clk_rate_idx + rx->returned))
>
> Too many parentheses.

Ok, will remove

>
> > +                                     break;
> > +                             rpmi_clk->rates->discrete[rateidx++] =
> > +                                     GET_RATE_U64(rate_discrete[j].hi,
> > +                                                  rate_discrete[j].lo);
> > +                     }
> > +             }
>
> ...
>
> > +     devm_kfree(context->dev, rx);
>
> Why?! This is a red flag to point that here is misunderstanding or abuse of
> managed resources approach. Either use __Free() from cleanup.h or don't call
> devm_kfree(). The latter must have a very good justification to explain why.

Yeah, I think it's better to use kzalloc for this case and then free it.


>
> > +     return 0;
>
> (this is even not an error path, where it might have a little argument for)
>
> ...
>
> > +     /* Keep the requested rate if the clock format
> > +      * is of discrete type. Let the platform which
> > +      * is actually controlling the clock handle that.
> > +      */
>
> /*
>  * Use proper style for the multi-line comments. You can
>  * refer to this comment as an example.

Yes, I will correct it.

>  */
>
> ...
>
> > +out:
>
> Redundant label. Note, the labels are recommended to be named after the flow
> they will run if goto. This one can be named as out_literally_with_return_0,
> which makes it obvious how useless it is.

I will improve the flow.

>
> > +     return 0;
>
> ...
>
> > +     rates = devm_kzalloc(dev, sizeof(union rpmi_clk_rates), GFP_KERNEL);
>
> sizeof(*...)
>
> > +     if (!rates)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     rpmi_clk = devm_kzalloc(dev, sizeof(struct rpmi_clk), GFP_KERNEL);
>
> Ditto.
>
> > +     if (!rpmi_clk)
> > +             return ERR_PTR(-ENOMEM);
>
> ...
>
> > +     ret = rpmi_clk_get_supported_rates(clkid, rpmi_clk);
> > +     if (ret)
> > +             return dev_err_ptr_probe(dev, ret,
> > +                     "Get supported rates failed for clk-%u, %d\n", clkid, ret);
>
> Indentation issues. Repetitive ret in the message. Please, get familiar with
> the format dev_err_probe() uses.

Ok, I will correct.

>
> ...
>
> > +     int ret, num_clocks, i;
>
> Why is 'i' signed?

This and num_clocks can be unsigned, will change.
>
> ...
>
> > +     /* Allocate RPMI clock context */
> > +     context = devm_kzalloc(dev, sizeof(*context), GFP_KERNEL);
>
> Ha-ha, you have even inconsistent style in the same file! So, go through the
> whole series and make sure that the style used in each file is consistent.

Yeah, that's because there are multiple devs in this series.
I will improve that.

>
> > +     if (!context)
> > +             return -ENOMEM;
>
> ...
>
> > +     /* Validate RPMI specification version */
> > +     rpmi_mbox_init_get_attribute(&msg, RPMI_MBOX_ATTR_SPEC_VERSION);
> > +     ret = rpmi_mbox_send_message(context->chan, &msg);
> > +     if (ret) {
> > +             dev_err_probe(dev, ret, "Failed to get spec version\n");
> > +             goto fail_free_channel;
>
> This is simply wrong. You should not do goto before any devm_*() calls.
> The error path and ->remove(), if present) is broken. Fix it accordingly.
>
> Here should be
>
>                 return dev_err_probe(...);
>
> it's your homework to understand how to achieve that. Plenty of the examples in
> the kernel.
>

Sure

> > +     }
>
> ...
>
> > +enum rpmi_clock_service_id {
> > +     RPMI_CLK_SRV_ENABLE_NOTIFICATION = 0x01,
> > +     RPMI_CLK_SRV_GET_NUM_CLOCKS = 0x02,
> > +     RPMI_CLK_SRV_GET_ATTRIBUTES = 0x03,
> > +     RPMI_CLK_SRV_GET_SUPPORTED_RATES = 0x04,
> > +     RPMI_CLK_SRV_SET_CONFIG = 0x05,
> > +     RPMI_CLK_SRV_GET_CONFIG = 0x06,
> > +     RPMI_CLK_SRV_SET_RATE = 0x07,
> > +     RPMI_CLK_SRV_GET_RATE = 0x08,
>
> > +     RPMI_CLK_SRV_ID_MAX_COUNT,
>
> No comma in the terminator line.

Ok

>
> > +};
>
> --
> With Best Regards,
> Andy Shevchenko
>
>


Thanks

Rahul Pathak