On Mon, Oct 26, 2026 at 02:54:15PM -0400, Charles Perry wrote:
> On Mon, Oct 26, 2026 at 12:49:39PM +0530, Subrahmanya Lingappa wrote:
> > Charles,
> >
> > On Wed, Oct 14, 2026 at 7:47 PM Charles Perry
> > <[email protected]> wrote:
> > >
> > > Create a new transport layer that sends the messages to a simulated RPMI
> > > firmware.
> > >
> > > For now, this only contains a simulation of the clock service group
> > > which is implemented using code from librpmi [1] and adapted.
> > >
> > > Also add a test in test/dm that makes use of the RPMI clock driver.
> > >
> > > [1]:
> > > https://github.com/riscv-software-src/librpmi/blob/3f3c4ca47339a8795432e260f17e1fc5091c9a3a/lib/rpmi_service_group_clock.c
> > >
> > > Signed-off-by: Charles Perry <[email protected]>
> > > ---
> > > Changes in v4: None
> > > Changes in v3: None
> > >
> > > Changes in v2:
> > > - Reorder headers
> > > ---
> > > MAINTAINERS | 1 +
> > > arch/sandbox/dts/test.dts | 12 +
> > > configs/sandbox_defconfig | 2 +
> > > drivers/firmware/rpmi/Kconfig | 3 +-
> > > drivers/firmware/rpmi/Makefile | 1 +
> > > drivers/firmware/rpmi/rpmi-sandbox-clock.c | 446
> > > +++++++++++++++++++++++++++++
> > > drivers/firmware/rpmi/rpmi-sandbox.c | 156 ++++++++++
> > > drivers/firmware/rpmi/rpmi-sandbox.h | 47 +++
> > > test/dm/Makefile | 1 +
> > > test/dm/rpmi.c | 48 ++++
> > > 10 files changed, 716 insertions(+), 1 deletion(-)
> > >
...
> > > +
> > > +static int dm_test_rpmi_clock(struct unit_test_state *uts)
> > > +{
> > > + struct udevice *rpmi_dev, *clk_dev;
> > > + struct clk clk0, clk1, clk2, clk3;
> > > +
> > > + ut_assertok(uclass_get_device_by_name(UCLASS_RPMI, "rpmi",
> > > &rpmi_dev));
> > > +
> > > + ut_assertok(
> > > + uclass_get_device_by_name(UCLASS_CLK, "rpmi-clock",
> > > &clk_dev));
> > > +
> > > + // Grab clock 0 and retrieve the rate
> > > + ut_assertok(clk_get_by_index(clk_dev, 0, &clk0));
> > > + ut_asserteq_64(20000000, clk_get_rate(&clk0));
> > > +
> > > + // Change to 1GHz
> > > + ut_asserteq(1000000000, clk_set_rate(&clk0, 1000000000));
> >
> > The test does not exercise the supported-rates path above. Please add a
> > regression test that sends `GET_SUPPORTED_RATES` through the sandbox
> > transport
> > for both a valid clock ID and an out-of-range ID. The current
> > get-rate/set-rate
> > test passes without invoking this service. Enable/disable coverage can be
> > added separately; shared-memory and MPXY command tests belong with their
> > respective transport/command harnesses.
>
> The RPMI clock driver doesn't even use the GET_SUPPORTED_RATES service as
> of now. I'll add the ->round_rate() callback for completeness so that we
> can test this service.
>
I changed my mind.
I won't implement ->round_rate() because clk_round_rate() has very few
users and because it's not even possible to implement correctly.
"include/clk.h" states that clk_round_rate() should round as
clk_set_state() would round but without touching the clock. This isn't
possible to infer just by the info of GET_SUPPORTED_RATES.
For reference, the Linux driver [1] skips rounding entirely for discrete
clocks and rounds up linear clocks.
I think the responsible thing to do here is "keep it simple" and get rid of
the GET_SUPPORTED_RATES sandbox mode code.
[1]: https://elixir.bootlin.com/linux/v7.3.1/source/drivers/clk/clk-rpmi.c#L328
--
Thanks,
Charles
