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

[Etienne Carriere]

On Wed, 9 Sep 2020 at 16:35, Simon Glass <sjg at chromium.org> wrote:
>
> 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?

I made the helper function sandbox_scmi_devices_ctx() exported by
sandbox-scmi_devices.c that references probed devices.
The function is called from test/dm/scmi.c to run the test (actions &
verifications).


>
> > 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.

Ok.
I'll try to keep it simple.

Regards,
etienne

>
> [..]
>
> Regards,
> Simon