On Tue, 8 Sep 2020 at 17:21, Simon Glass <s...@chromium.org> wrote:
>
> Hi Etienne,
>
> On Mon, 7 Sep 2020 at 08:50, Etienne Carriere
> <etienne.carri...@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.carri...@linaro.org>
> > Cc: Simon Glass <s...@chromium.org>
> > Cc: Peng Fan <peng....@nxp.com>
> > Cc: Sudeep Holla <sudeep.ho...@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 <s...@chromium.org>
>
> Nits below
>
> [..]
>
> > diff --git a/drivers/firmware/scmi/sandbox-scmi_devices.c
> > b/drivers/firmware/scmi/sandbox-scmi_devices.c
> > new file mode 100644
> > index 0000000000..2ce8e664df
> > --- /dev/null
> > +++ b/drivers/firmware/scmi/sandbox-scmi_devices.c
> > @@ -0,0 +1,86 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020, Linaro Limited
> > + */
> > +
> > +#include <common.h>
> > +#include <clk.h>
> > +#include <dm.h>
> > +#include <malloc.h>
> > +#include <asm/io.h>
> > +#include <asm/scmi_test.h>
> > +#include <dm/device_compat.h>
> > +
> > +/*
> > + * Simulate to some extend a SCMI exhange.
>
> extend, exchange
acked.
>
> > + * This drivers gets SCMI resources and offers API function to the
> > + * SCMI test sequence manipulate the resources.
> > + */
> > +
> > +#define SCMI_TEST_DEVICES_CLK_COUNT 3
> > +
> > +/*
> > + * These tables store de handles used in the various uclasses device
> > function
>
> s/de/the/ ?
acked.
>
> > + * that are instancied when probed through the SCMI agent. Use a static
>
> spelling
acked, thanks.
>
> > + * 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.
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.
etienne
>
> > +
> > +struct sandbox_scmi_devices *sandbox_scmi_devices_ctx(void)
> > +{
> > + return &sandbox_scmi_devhld;
> > +}
> > +
> > +static void dereference_device_handles(struct sandbox_scmi_devices
> > *devices)
> > +{
> > + devices->clk = NULL;
> > + devices->clk_count = 0;
> > +}
> > +
> > +static int sandbox_scmi_devices_remove(struct udevice *dev)
> > +{
> > + struct sandbox_scmi_devices *devices = sandbox_scmi_devices_ctx();
> > +
> > + dereference_device_handles(devices);
> > +
> > + return 0;
> > +}
> > +
> > +static int sandbox_scmi_devices_probe(struct udevice *dev)
> > +{
> > + struct sandbox_scmi_devices *devices = sandbox_scmi_devices_ctx();
> > + int rc;
> > + size_t n;
> > +
> > + devices->clk = sandbox_scmi_clk_device;
> > + devices->clk_count = SCMI_TEST_DEVICES_CLK_COUNT;
> > +
> > + for (n = 0; n < SCMI_TEST_DEVICES_CLK_COUNT; n++) {
> > + rc = clk_get_by_index(dev, n, devices->clk + n);
> > + if (rc) {
> > + dev_err(dev, "%s: Failed on clk %zu\n", __func__,
> > n);
> > + goto err_clk;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +err_clk:
> > + dereference_device_handles(devices);
> > +
> > + return rc;
> > +}
> > +
> > +static const struct udevice_id sandbox_scmi_devices_ids[] = {
> > + { .compatible = "sandbox,scmi-devices" },
> > + { }
> > +};
> > +
> > +U_BOOT_DRIVER(sandbox_scmi_devices) = {
> > + .name = "sandbox-scmi_devices",
> > + .id = UCLASS_MISC,
> > + .of_match = sandbox_scmi_devices_ids,
> > + .remove = sandbox_scmi_devices_remove,
> > + .probe = sandbox_scmi_devices_probe,
> > +};
> [..]
>
> Regards,
> Simon
