[PATCH v3 6/8] firmware: scmi: sandbox test for SCMI clocks

[Simon Glass]

Hi Etienne,

On Wed, 9 Sep 2020 at 03:58, Etienne Carriere
<etienne.carriere at linaro.org> wrote:
>
> On Tue, 8 Sep 2020 at 17:21, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Etienne,
> >
> > On Mon, 7 Sep 2020 at 08:50, Etienne Carriere
> > <etienne.carriere at linaro.org> wrote:
> > >
> > > Add tests for SCMI clocks. A test device driver sandbox-scmi_devices.c
> > > is used to get clock resources, allowing further clock manipulation.
> > >
> > > Change sandbox-smci_agent to emulate 3 clocks exposed through 2 agents.
> > > Add DM test scmi_clocks to test these 3 clocks.
> > > Update DM test sandbox_scmi_agent with load/remove test sequences
> > > factorized by {load|remove}_sandbox_scmi_test_devices() helper functions.
> > >
> > > Signed-off-by: Etienne Carriere <etienne.carriere at linaro.org>
> > > Cc: Simon Glass <sjg at chromium.org>
> > > Cc: Peng Fan <peng.fan at nxp.com>
> > > Cc: Sudeep Holla <sudeep.holla at arm.com>
> > > ---
> > >
> > > Changes in v3:
> > > - New commit in the series, addresses review comments on test support.
> > >   ut_dm_scmi_clocks test SCMI are found and behave as expected for the
> > >   implemented clk uclass methods.
> > > ---
> > >  arch/sandbox/dts/test.dts                    |  15 ++
> > >  arch/sandbox/include/asm/scmi_test.h         |  37 ++++
> > >  configs/sandbox_defconfig                    |   1 +
> > >  drivers/firmware/scmi/Makefile               |   2 +-
> > >  drivers/firmware/scmi/sandbox-scmi_agent.c   | 169 ++++++++++++++++++-
> > >  drivers/firmware/scmi/sandbox-scmi_devices.c |  86 ++++++++++
> > >  test/dm/scmi.c                               | 139 ++++++++++++++-
> > >  7 files changed, 438 insertions(+), 11 deletions(-)
> > >  create mode 100644 drivers/firmware/scmi/sandbox-scmi_devices.c
> >
> > Reviewed-by: Simon Glass <sjg at chromium.org>
> >
> > Nits below
> >
> > [..]

> > > + * memory allocation to ease sharing with test sequence implementation.
> > > + */
> > > +static struct clk sandbox_scmi_clk_device[SCMI_TEST_DEVICES_CLK_COUNT];
> > > +static struct sandbox_scmi_devices sandbox_scmi_devhld;
> >
> > This should really be in a struct, I think, pointed to by
> > dev_get_priv() on this device. I do try to avoid BSS with driver
> > model, although it is not a hard rule with test code.
>
> I used a static structure here to ease sharing context reference with
> the test sequence implementation.

Is that in a different file?

> Context reference returned by sandbox_scmi_devices_ctx() is always
> reliable for the sequence.
> (not possibly dereference in which case the test may segfault).
>
> I can go this way if you prefer no BSS in drivers (Note this is a
> sandbox driver). I'll update the test accordingly.

If you have a reason to use BSS for a sandbox driver that is OK. I
don't quite understand the problem you are solving though. If private
data is allocated too late, you can allocate the info as platdata
which happens when the device is bound, and then use
dev_get_platdata().

But ultimately it is up to you, as this is a test driver. It just
helps to avoid people copying a pattern to their own driver.

[..]

Regards,
Simon