| Message ID | 20230628004841.21774-8-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: scmi: add SCMI base protocol support | expand |
Hi AKASHI, On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Added is a new unit test for SCMI base protocol, which will exercise all > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS. > $ ut dm scmi_base > It is assumed that test.dtb is used as sandbox's device tree. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 112 insertions(+) > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c > index 881be3171b7c..563017bb63e0 100644 > --- a/test/dm/scmi.c > +++ b/test/dm/scmi.c > @@ -16,6 +16,9 @@ > #include <clk.h> > #include <dm.h> > #include <reset.h> > +#include <scmi_agent.h> > +#include <scmi_agent-uclass.h> > +#include <scmi_protocols.h> > #include <asm/scmi_test.h> > #include <dm/device-internal.h> > #include <dm/test.h> > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts) > } > DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT); > > +static int dm_test_scmi_base(struct unit_test_state *uts) > +{ > + struct udevice *agent_dev, *base; > + struct scmi_agent_priv *priv; > + const struct scmi_base_ops *ops; > + u32 version, num_agents, num_protocols, impl_version; > + u32 attributes, agent_id; > + char vendor[SCMI_BASE_NAME_LENGTH_MAX], > + agent_name[SCMI_BASE_NAME_LENGTH_MAX]; > + u8 *protocols; > + int ret; > + > + /* preparation */ > + ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi", > + &agent_dev)); > + ut_assertnonnull(agent_dev); > + ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev)); > + ut_assertnonnull(base = scmi_get_protocol(agent_dev, > + SCMI_PROTOCOL_ID_BASE)); > + ut_assertnonnull(ops = dev_get_driver_ops(base)); > + > + /* version */ > + ret = (*ops->protocol_version)(base, &version); Can you add uclass helpers to call each of the methods? That is how it is commonly done. You should not be calling ops->xxx directly here. > + ut_assertok(ret); > + ut_asserteq(priv->version, version); > + > + /* protocol attributes */ > + ret = (*ops->protocol_attrs)(base, &num_agents, &num_protocols); > + ut_assertok(ret); > + ut_asserteq(priv->num_agents, num_agents); > + ut_asserteq(priv->num_protocols, num_protocols); > + > + /* discover vendor */ > + ret = (*ops->base_discover_vendor)(base, vendor); > + ut_assertok(ret); > + ut_asserteq_str(priv->vendor, vendor); > + > + /* message attributes */ > + ret = (*ops->protocol_message_attrs)(base, > + SCMI_BASE_DISCOVER_SUB_VENDOR, > + &attributes); > + ut_assertok(ret); > + ut_assertok(attributes); > + > + /* discover sub vendor */ > + ret = (*ops->base_discover_sub_vendor)(base, vendor); > + ut_assertok(ret); > + ut_asserteq_str(priv->sub_vendor, vendor); > + > + /* impl version */ > + ret = (*ops->base_discover_impl_version)(base, &impl_version); > + ut_assertok(ret); > + ut_asserteq(priv->impl_version, impl_version); > + > + /* discover agent (my self) */ > + ret = (*ops->base_discover_agent)(base, 0xffffffff, > + &agent_id, agent_name); > + ut_assertok(ret); > + ut_asserteq(priv->agent_id, agent_id); > + ut_asserteq_str(priv->agent_name, agent_name); > + > + /* discover protocols */ > + ret = (*ops->base_discover_list_protocols)(base, &protocols); > + ut_asserteq(num_protocols, ret); > + ut_asserteq_mem(priv->protocols, protocols, sizeof(u8) * num_protocols); > + free(protocols); > + > + /* > + * NOTE: Sandbox SCMI driver handles device-0 only. It supports setting > + * access and protocol permissions, but doesn't allow unsetting them nor > + * resetting the configurations. > + */ > + /* set device permissions */ > + ret = (*ops->base_set_device_permissions)(base, agent_id, 0, > + SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS); > + ut_assertok(ret); /* SCMI_SUCCESS */ > + ret = (*ops->base_set_device_permissions)(base, agent_id, 1, > + SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS); > + ut_asserteq(-ENOENT, ret); /* SCMI_NOT_FOUND */ > + ret = (*ops->base_set_device_permissions)(base, agent_id, 0, 0); > + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ > + > + /* set protocol permissions */ > + ret = (*ops->base_set_protocol_permissions)(base, agent_id, 0, > + SCMI_PROTOCOL_ID_CLOCK, > + SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS); > + ut_assertok(ret); /* SCMI_SUCCESS */ > + ret = (*ops->base_set_protocol_permissions)(base, agent_id, 1, > + SCMI_PROTOCOL_ID_CLOCK, > + SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS); > + ut_asserteq(-ENOENT, ret); /* SCMI_NOT_FOUND */ > + ret = (*ops->base_set_protocol_permissions)(base, agent_id, 0, > + SCMI_PROTOCOL_ID_CLOCK, 0); > + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ > + > + /* reset agent configuration */ > + ret = (*ops->base_reset_agent_configuration)(base, agent_id, 0); > + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ > + ret = (*ops->base_reset_agent_configuration)(base, agent_id, > + SCMI_BASE_RESET_ALL_ACCESS_PERMISSIONS); > + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ > + ret = (*ops->base_reset_agent_configuration)(base, agent_id, 0); > + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ > + > + return 0; > +} > + > +DM_TEST(dm_test_scmi_base, UT_TESTF_SCAN_FDT); > + > static int dm_test_scmi_clocks(struct unit_test_state *uts) > { > struct sandbox_scmi_agent *agent; > -- > 2.41.0 > Regards, Simon
On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote: > Hi AKASHI, > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > Added is a new unit test for SCMI base protocol, which will exercise all > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS. > > $ ut dm scmi_base > > It is assumed that test.dtb is used as sandbox's device tree. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 112 insertions(+) > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c > > index 881be3171b7c..563017bb63e0 100644 > > --- a/test/dm/scmi.c > > +++ b/test/dm/scmi.c > > @@ -16,6 +16,9 @@ > > #include <clk.h> > > #include <dm.h> > > #include <reset.h> > > +#include <scmi_agent.h> > > +#include <scmi_agent-uclass.h> > > +#include <scmi_protocols.h> > > #include <asm/scmi_test.h> > > #include <dm/device-internal.h> > > #include <dm/test.h> > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts) > > } > > DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT); > > > > +static int dm_test_scmi_base(struct unit_test_state *uts) > > +{ > > + struct udevice *agent_dev, *base; > > + struct scmi_agent_priv *priv; > > + const struct scmi_base_ops *ops; > > + u32 version, num_agents, num_protocols, impl_version; > > + u32 attributes, agent_id; > > + char vendor[SCMI_BASE_NAME_LENGTH_MAX], > > + agent_name[SCMI_BASE_NAME_LENGTH_MAX]; > > + u8 *protocols; > > + int ret; > > + > > + /* preparation */ > > + ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi", > > + &agent_dev)); > > + ut_assertnonnull(agent_dev); > > + ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev)); > > + ut_assertnonnull(base = scmi_get_protocol(agent_dev, > > + SCMI_PROTOCOL_ID_BASE)); > > + ut_assertnonnull(ops = dev_get_driver_ops(base)); > > + > > + /* version */ > > + ret = (*ops->protocol_version)(base, &version); > > Can you add uclass helpers to call each of the methods? That is how it > is commonly done. You should not be calling ops->xxx directly here. Yes, I will add inline functions instead. -Takahiro Akashi > > + ut_assertok(ret); > > + ut_asserteq(priv->version, version); > > + > > + /* protocol attributes */ > > + ret = (*ops->protocol_attrs)(base, &num_agents, &num_protocols); > > + ut_assertok(ret); > > + ut_asserteq(priv->num_agents, num_agents); > > + ut_asserteq(priv->num_protocols, num_protocols); > > + > > + /* discover vendor */ > > + ret = (*ops->base_discover_vendor)(base, vendor); > > + ut_assertok(ret); > > + ut_asserteq_str(priv->vendor, vendor); > > + > > + /* message attributes */ > > + ret = (*ops->protocol_message_attrs)(base, > > + SCMI_BASE_DISCOVER_SUB_VENDOR, > > + &attributes); > > + ut_assertok(ret); > > + ut_assertok(attributes); > > + > > + /* discover sub vendor */ > > + ret = (*ops->base_discover_sub_vendor)(base, vendor); > > + ut_assertok(ret); > > + ut_asserteq_str(priv->sub_vendor, vendor); > > + > > + /* impl version */ > > + ret = (*ops->base_discover_impl_version)(base, &impl_version); > > + ut_assertok(ret); > > + ut_asserteq(priv->impl_version, impl_version); > > + > > + /* discover agent (my self) */ > > + ret = (*ops->base_discover_agent)(base, 0xffffffff, > > + &agent_id, agent_name); > > + ut_assertok(ret); > > + ut_asserteq(priv->agent_id, agent_id); > > + ut_asserteq_str(priv->agent_name, agent_name); > > + > > + /* discover protocols */ > > + ret = (*ops->base_discover_list_protocols)(base, &protocols); > > + ut_asserteq(num_protocols, ret); > > + ut_asserteq_mem(priv->protocols, protocols, sizeof(u8) * num_protocols); > > + free(protocols); > > + > > + /* > > + * NOTE: Sandbox SCMI driver handles device-0 only. It supports setting > > + * access and protocol permissions, but doesn't allow unsetting them nor > > + * resetting the configurations. > > + */ > > + /* set device permissions */ > > + ret = (*ops->base_set_device_permissions)(base, agent_id, 0, > > + SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS); > > + ut_assertok(ret); /* SCMI_SUCCESS */ > > + ret = (*ops->base_set_device_permissions)(base, agent_id, 1, > > + SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS); > > + ut_asserteq(-ENOENT, ret); /* SCMI_NOT_FOUND */ > > + ret = (*ops->base_set_device_permissions)(base, agent_id, 0, 0); > > + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ > > + > > + /* set protocol permissions */ > > + ret = (*ops->base_set_protocol_permissions)(base, agent_id, 0, > > + SCMI_PROTOCOL_ID_CLOCK, > > + SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS); > > + ut_assertok(ret); /* SCMI_SUCCESS */ > > + ret = (*ops->base_set_protocol_permissions)(base, agent_id, 1, > > + SCMI_PROTOCOL_ID_CLOCK, > > + SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS); > > + ut_asserteq(-ENOENT, ret); /* SCMI_NOT_FOUND */ > > + ret = (*ops->base_set_protocol_permissions)(base, agent_id, 0, > > + SCMI_PROTOCOL_ID_CLOCK, 0); > > + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ > > + > > + /* reset agent configuration */ > > + ret = (*ops->base_reset_agent_configuration)(base, agent_id, 0); > > + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ > > + ret = (*ops->base_reset_agent_configuration)(base, agent_id, > > + SCMI_BASE_RESET_ALL_ACCESS_PERMISSIONS); > > + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ > > + ret = (*ops->base_reset_agent_configuration)(base, agent_id, 0); > > + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ > > + > > + return 0; > > +} > > + > > +DM_TEST(dm_test_scmi_base, UT_TESTF_SCAN_FDT); > > + > > static int dm_test_scmi_clocks(struct unit_test_state *uts) > > { > > struct sandbox_scmi_agent *agent; > > -- > > 2.41.0 > > > > Regards, > Simon
Hi, On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote: > > Hi AKASHI, > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > Added is a new unit test for SCMI base protocol, which will exercise all > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS. > > > $ ut dm scmi_base > > > It is assumed that test.dtb is used as sandbox's device tree. > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > --- > > > test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 112 insertions(+) > > > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c > > > index 881be3171b7c..563017bb63e0 100644 > > > --- a/test/dm/scmi.c > > > +++ b/test/dm/scmi.c > > > @@ -16,6 +16,9 @@ > > > #include <clk.h> > > > #include <dm.h> > > > #include <reset.h> > > > +#include <scmi_agent.h> > > > +#include <scmi_agent-uclass.h> > > > +#include <scmi_protocols.h> > > > #include <asm/scmi_test.h> > > > #include <dm/device-internal.h> > > > #include <dm/test.h> > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts) > > > } > > > DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT); > > > > > > +static int dm_test_scmi_base(struct unit_test_state *uts) > > > +{ > > > + struct udevice *agent_dev, *base; > > > + struct scmi_agent_priv *priv; > > > + const struct scmi_base_ops *ops; > > > + u32 version, num_agents, num_protocols, impl_version; > > > + u32 attributes, agent_id; > > > + char vendor[SCMI_BASE_NAME_LENGTH_MAX], > > > + agent_name[SCMI_BASE_NAME_LENGTH_MAX]; > > > + u8 *protocols; > > > + int ret; > > > + > > > + /* preparation */ > > > + ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi", > > > + &agent_dev)); > > > + ut_assertnonnull(agent_dev); > > > + ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev)); > > > + ut_assertnonnull(base = scmi_get_protocol(agent_dev, > > > + SCMI_PROTOCOL_ID_BASE)); > > > + ut_assertnonnull(ops = dev_get_driver_ops(base)); > > > + > > > + /* version */ > > > + ret = (*ops->protocol_version)(base, &version); > > > > Can you add uclass helpers to call each of the methods? That is how it > > is commonly done. You should not be calling ops->xxx directly here. > > Yes, I will add inline functions instead. I don't mean inline...see all the other uclasses which define a function which is implemented in the uclass. It is confusing when one uclass does something different. People might copy this style and then the code base diverges. Did you not notice this when looking around the source tree? Regards, Simon
Hi Simon, On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote: > Hi, > > On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote: > > > Hi AKASHI, > > > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > Added is a new unit test for SCMI base protocol, which will exercise all > > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS. > > > > $ ut dm scmi_base > > > > It is assumed that test.dtb is used as sandbox's device tree. > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > --- > > > > test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 112 insertions(+) > > > > > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c > > > > index 881be3171b7c..563017bb63e0 100644 > > > > --- a/test/dm/scmi.c > > > > +++ b/test/dm/scmi.c > > > > @@ -16,6 +16,9 @@ > > > > #include <clk.h> > > > > #include <dm.h> > > > > #include <reset.h> > > > > +#include <scmi_agent.h> > > > > +#include <scmi_agent-uclass.h> > > > > +#include <scmi_protocols.h> > > > > #include <asm/scmi_test.h> > > > > #include <dm/device-internal.h> > > > > #include <dm/test.h> > > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts) > > > > } > > > > DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT); > > > > > > > > +static int dm_test_scmi_base(struct unit_test_state *uts) > > > > +{ > > > > + struct udevice *agent_dev, *base; > > > > + struct scmi_agent_priv *priv; > > > > + const struct scmi_base_ops *ops; > > > > + u32 version, num_agents, num_protocols, impl_version; > > > > + u32 attributes, agent_id; > > > > + char vendor[SCMI_BASE_NAME_LENGTH_MAX], > > > > + agent_name[SCMI_BASE_NAME_LENGTH_MAX]; > > > > + u8 *protocols; > > > > + int ret; > > > > + > > > > + /* preparation */ > > > > + ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi", > > > > + &agent_dev)); > > > > + ut_assertnonnull(agent_dev); > > > > + ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev)); > > > > + ut_assertnonnull(base = scmi_get_protocol(agent_dev, > > > > + SCMI_PROTOCOL_ID_BASE)); > > > > + ut_assertnonnull(ops = dev_get_driver_ops(base)); > > > > + > > > > + /* version */ > > > > + ret = (*ops->protocol_version)(base, &version); > > > > > > Can you add uclass helpers to call each of the methods? That is how it > > > is commonly done. You should not be calling ops->xxx directly here. > > > > Yes, I will add inline functions instead. > > I don't mean inline...see all the other uclasses which define a Okay, I will *real* functions. > function which is implemented in the uclass. It is confusing when one > uclass does something different. People might copy this style and then > the code base diverges. Did you not notice this when looking around > the source tree? But one concern came up in my mind. Contrary to ordinary "device controllers", there exists only a single implementation of driver for each of "udevice"'s associated with SCMI protocols including the base protocol. So if I follow your suggestion, the code (base.c) might look like: === static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) { ... } struct scmi_base_ops scmi_base_ops = { .base_discover_vendor = __scmi_base_discover_vendor, } int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) { struct scmi_base_ops *ops; ops = scmi_base_dev_ops(dev); return ops->base_discover_vendor(dev, vendor); } === We will have to have similar definitions for every operation in ops. It looks quite weird to me as there are always pairs of functions, like __scmi_base_discover_vendor() and scmi_base_discover_vendor(). We can avoid this redundant code easily by eliminating "ops" abstraction. But as far as I remember, you insist that every driver that complies to U-Boot driver model should have a "ops". What do you make of this? Thanks -Takahiro Akashi > Regards, > Simon
Hi, On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Hi Simon, > > On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote: > > Hi, > > > > On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote: > > > > Hi AKASHI, > > > > > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > Added is a new unit test for SCMI base protocol, which will exercise all > > > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS. > > > > > $ ut dm scmi_base > > > > > It is assumed that test.dtb is used as sandbox's device tree. > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > --- > > > > > test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 112 insertions(+) > > > > > > > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c > > > > > index 881be3171b7c..563017bb63e0 100644 > > > > > --- a/test/dm/scmi.c > > > > > +++ b/test/dm/scmi.c > > > > > @@ -16,6 +16,9 @@ > > > > > #include <clk.h> > > > > > #include <dm.h> > > > > > #include <reset.h> > > > > > +#include <scmi_agent.h> > > > > > +#include <scmi_agent-uclass.h> > > > > > +#include <scmi_protocols.h> > > > > > #include <asm/scmi_test.h> > > > > > #include <dm/device-internal.h> > > > > > #include <dm/test.h> > > > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts) > > > > > } > > > > > DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT); > > > > > > > > > > +static int dm_test_scmi_base(struct unit_test_state *uts) > > > > > +{ > > > > > + struct udevice *agent_dev, *base; > > > > > + struct scmi_agent_priv *priv; > > > > > + const struct scmi_base_ops *ops; > > > > > + u32 version, num_agents, num_protocols, impl_version; > > > > > + u32 attributes, agent_id; > > > > > + char vendor[SCMI_BASE_NAME_LENGTH_MAX], > > > > > + agent_name[SCMI_BASE_NAME_LENGTH_MAX]; > > > > > + u8 *protocols; > > > > > + int ret; > > > > > + > > > > > + /* preparation */ > > > > > + ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi", > > > > > + &agent_dev)); > > > > > + ut_assertnonnull(agent_dev); > > > > > + ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev)); > > > > > + ut_assertnonnull(base = scmi_get_protocol(agent_dev, > > > > > + SCMI_PROTOCOL_ID_BASE)); > > > > > + ut_assertnonnull(ops = dev_get_driver_ops(base)); > > > > > + > > > > > + /* version */ > > > > > + ret = (*ops->protocol_version)(base, &version); > > > > > > > > Can you add uclass helpers to call each of the methods? That is how it > > > > is commonly done. You should not be calling ops->xxx directly here. > > > > > > Yes, I will add inline functions instead. > > > > I don't mean inline...see all the other uclasses which define a > > Okay, I will *real* functions. > > > function which is implemented in the uclass. It is confusing when one > > uclass does something different. People might copy this style and then > > the code base diverges. Did you not notice this when looking around > > the source tree? > > But one concern came up in my mind. > Contrary to ordinary "device controllers", there exists only a single > implementation of driver for each of "udevice"'s associated with SCMI > protocols including the base protocol. > > So if I follow your suggestion, the code (base.c) might look like: > === > static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) > { > ... > } > > struct scmi_base_ops scmi_base_ops = { > > .base_discover_vendor = __scmi_base_discover_vendor, > > } > > int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) > { > struct scmi_base_ops *ops; > > ops = scmi_base_dev_ops(dev); > > return ops->base_discover_vendor(dev, vendor); > } > === > > We will have to have similar definitions for every operation in ops. > It looks quite weird to me as there are always pairs of functions, > like __scmi_base_discover_vendor() and scmi_base_discover_vendor(). Yes I understand that you only have one driver at present. Is there not a sandbox driver? > > We can avoid this redundant code easily by eliminating "ops" abstraction. > But as far as I remember, you insist that every driver that complies > to U-Boot driver model should have a "ops". > > What do you make of this? Well there are some exceptions, but yes that is the idea. Operations should be in a 'ops' struct and documented and implemented in a consistent way. Regards, Simon
On Fri, Jul 07, 2023 at 11:35:49AM -0600, Simon Glass wrote: > Hi, > > On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > Hi Simon, > > > > On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote: > > > Hi, > > > > > > On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > > > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote: > > > > > Hi AKASHI, > > > > > > > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > Added is a new unit test for SCMI base protocol, which will exercise all > > > > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS. > > > > > > $ ut dm scmi_base > > > > > > It is assumed that test.dtb is used as sandbox's device tree. > > > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > --- > > > > > > test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > 1 file changed, 112 insertions(+) > > > > > > > > > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c > > > > > > index 881be3171b7c..563017bb63e0 100644 > > > > > > --- a/test/dm/scmi.c > > > > > > +++ b/test/dm/scmi.c > > > > > > @@ -16,6 +16,9 @@ > > > > > > #include <clk.h> > > > > > > #include <dm.h> > > > > > > #include <reset.h> > > > > > > +#include <scmi_agent.h> > > > > > > +#include <scmi_agent-uclass.h> > > > > > > +#include <scmi_protocols.h> > > > > > > #include <asm/scmi_test.h> > > > > > > #include <dm/device-internal.h> > > > > > > #include <dm/test.h> > > > > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts) > > > > > > } > > > > > > DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT); > > > > > > > > > > > > +static int dm_test_scmi_base(struct unit_test_state *uts) > > > > > > +{ > > > > > > + struct udevice *agent_dev, *base; > > > > > > + struct scmi_agent_priv *priv; > > > > > > + const struct scmi_base_ops *ops; > > > > > > + u32 version, num_agents, num_protocols, impl_version; > > > > > > + u32 attributes, agent_id; > > > > > > + char vendor[SCMI_BASE_NAME_LENGTH_MAX], > > > > > > + agent_name[SCMI_BASE_NAME_LENGTH_MAX]; > > > > > > + u8 *protocols; > > > > > > + int ret; > > > > > > + > > > > > > + /* preparation */ > > > > > > + ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi", > > > > > > + &agent_dev)); > > > > > > + ut_assertnonnull(agent_dev); > > > > > > + ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev)); > > > > > > + ut_assertnonnull(base = scmi_get_protocol(agent_dev, > > > > > > + SCMI_PROTOCOL_ID_BASE)); > > > > > > + ut_assertnonnull(ops = dev_get_driver_ops(base)); > > > > > > + > > > > > > + /* version */ > > > > > > + ret = (*ops->protocol_version)(base, &version); > > > > > > > > > > Can you add uclass helpers to call each of the methods? That is how it > > > > > is commonly done. You should not be calling ops->xxx directly here. > > > > > > > > Yes, I will add inline functions instead. > > > > > > I don't mean inline...see all the other uclasses which define a > > > > Okay, I will *real* functions. > > > > > function which is implemented in the uclass. It is confusing when one > > > uclass does something different. People might copy this style and then > > > the code base diverges. Did you not notice this when looking around > > > the source tree? > > > > But one concern came up in my mind. > > Contrary to ordinary "device controllers", there exists only a single > > implementation of driver for each of "udevice"'s associated with SCMI > > protocols including the base protocol. > > > > So if I follow your suggestion, the code (base.c) might look like: > > === > > static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) > > { > > ... > > } > > > > struct scmi_base_ops scmi_base_ops = { > > > > .base_discover_vendor = __scmi_base_discover_vendor, > > > > } > > > > int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) > > { > > struct scmi_base_ops *ops; > > > > ops = scmi_base_dev_ops(dev); > > > > return ops->base_discover_vendor(dev, vendor); > > } > > === > > > > We will have to have similar definitions for every operation in ops. > > It looks quite weird to me as there are always pairs of functions, > > like __scmi_base_discover_vendor() and scmi_base_discover_vendor(). > > Yes I understand that you only have one driver at present. Is there > not a sandbox driver? No. Please remember that SCMI protocol drivers on U-Boot are nothing but stubs that makes a call to SCMI servers, supporting common communication channel interfaces for different transports (either OP-TEE, SMCCC or mailbox). Sandbox driver, if is properly named, is also implemented as a sort of transport layer, where a invocation is replaced with a function call which mimicks one of specific commands in SCMI protocol on behalf of a real SCMI server. In this sense, there will exist only a single driver under the current form of framework forever. > > > > > We can avoid this redundant code easily by eliminating "ops" abstraction. > > But as far as I remember, you insist that every driver that complies > > to U-Boot driver model should have a "ops". > > > > What do you make of this? > > Well there are some exceptions, but yes that is the idea. Operations > should be in a 'ops' struct and documented and implemented in a > consistent way. Is it your choice that I should keep "ops" structure in this specific implementation? -Takahiro Akashi > Regards, > Simon
Hi, On Sun, 9 Jul 2023 at 20:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > On Fri, Jul 07, 2023 at 11:35:49AM -0600, Simon Glass wrote: > > Hi, > > > > On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote: > > > > Hi, > > > > > > > > On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote: > > > > > > Hi AKASHI, > > > > > > > > > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro > > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > > > Added is a new unit test for SCMI base protocol, which will exercise all > > > > > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS. > > > > > > > $ ut dm scmi_base > > > > > > > It is assumed that test.dtb is used as sandbox's device tree. > > > > > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > > --- > > > > > > > test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > 1 file changed, 112 insertions(+) > > > > > > > > > > > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c > > > > > > > index 881be3171b7c..563017bb63e0 100644 > > > > > > > --- a/test/dm/scmi.c > > > > > > > +++ b/test/dm/scmi.c > > > > > > > @@ -16,6 +16,9 @@ > > > > > > > #include <clk.h> > > > > > > > #include <dm.h> > > > > > > > #include <reset.h> > > > > > > > +#include <scmi_agent.h> > > > > > > > +#include <scmi_agent-uclass.h> > > > > > > > +#include <scmi_protocols.h> > > > > > > > #include <asm/scmi_test.h> > > > > > > > #include <dm/device-internal.h> > > > > > > > #include <dm/test.h> > > > > > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts) > > > > > > > } > > > > > > > DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT); > > > > > > > > > > > > > > +static int dm_test_scmi_base(struct unit_test_state *uts) > > > > > > > +{ > > > > > > > + struct udevice *agent_dev, *base; > > > > > > > + struct scmi_agent_priv *priv; > > > > > > > + const struct scmi_base_ops *ops; > > > > > > > + u32 version, num_agents, num_protocols, impl_version; > > > > > > > + u32 attributes, agent_id; > > > > > > > + char vendor[SCMI_BASE_NAME_LENGTH_MAX], > > > > > > > + agent_name[SCMI_BASE_NAME_LENGTH_MAX]; > > > > > > > + u8 *protocols; > > > > > > > + int ret; > > > > > > > + > > > > > > > + /* preparation */ > > > > > > > + ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi", > > > > > > > + &agent_dev)); > > > > > > > + ut_assertnonnull(agent_dev); > > > > > > > + ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev)); > > > > > > > + ut_assertnonnull(base = scmi_get_protocol(agent_dev, > > > > > > > + SCMI_PROTOCOL_ID_BASE)); > > > > > > > + ut_assertnonnull(ops = dev_get_driver_ops(base)); > > > > > > > + > > > > > > > + /* version */ > > > > > > > + ret = (*ops->protocol_version)(base, &version); > > > > > > > > > > > > Can you add uclass helpers to call each of the methods? That is how it > > > > > > is commonly done. You should not be calling ops->xxx directly here. > > > > > > > > > > Yes, I will add inline functions instead. > > > > > > > > I don't mean inline...see all the other uclasses which define a > > > > > > Okay, I will *real* functions. > > > > > > > function which is implemented in the uclass. It is confusing when one > > > > uclass does something different. People might copy this style and then > > > > the code base diverges. Did you not notice this when looking around > > > > the source tree? > > > > > > But one concern came up in my mind. > > > Contrary to ordinary "device controllers", there exists only a single > > > implementation of driver for each of "udevice"'s associated with SCMI > > > protocols including the base protocol. > > > > > > So if I follow your suggestion, the code (base.c) might look like: > > > === > > > static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) > > > { > > > ... > > > } > > > > > > struct scmi_base_ops scmi_base_ops = { > > > > > > .base_discover_vendor = __scmi_base_discover_vendor, > > > > > > } > > > > > > int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) > > > { > > > struct scmi_base_ops *ops; > > > > > > ops = scmi_base_dev_ops(dev); > > > > > > return ops->base_discover_vendor(dev, vendor); > > > } > > > === > > > > > > We will have to have similar definitions for every operation in ops. > > > It looks quite weird to me as there are always pairs of functions, > > > like __scmi_base_discover_vendor() and scmi_base_discover_vendor(). > > > > Yes I understand that you only have one driver at present. Is there > > not a sandbox driver? > > No. > Please remember that SCMI protocol drivers on U-Boot are nothing but > stubs that makes a call to SCMI servers, supporting common communication > channel interfaces for different transports (either OP-TEE, SMCCC or mailbox). > > Sandbox driver, if is properly named, is also implemented as a sort of > transport layer, where a invocation is replaced with a function call which > mimicks one of specific commands in SCMI protocol on behalf of a real SCMI server. > > In this sense, there will exist only a single driver under the current > form of framework forever. OK, so driver model is used for the transport but not the top-level driver? I see. > > > > > > > > > We can avoid this redundant code easily by eliminating "ops" abstraction. > > > But as far as I remember, you insist that every driver that complies > > > to U-Boot driver model should have a "ops". > > > > > > What do you make of this? > > > > Well there are some exceptions, but yes that is the idea. Operations > > should be in a 'ops' struct and documented and implemented in a > > consistent way. > > Is it your choice that I should keep "ops" structure in this specific > implementation? I can't actually find this patch on patchwork. But yes, you do need a function for each ops call. They should be used in the tests, which should not directly call functions using ops->xxx() Regards, Simon
Hi Simon, On Mon, Jul 10, 2023 at 01:45:58PM -0600, Simon Glass wrote: > Hi, > > On Sun, 9 Jul 2023 at 20:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > On Fri, Jul 07, 2023 at 11:35:49AM -0600, Simon Glass wrote: > > > Hi, > > > > > > On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > > > Hi Simon, > > > > > > > > On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote: > > > > > Hi, > > > > > > > > > > On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote: > > > > > > > Hi AKASHI, > > > > > > > > > > > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro > > > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > > > > > Added is a new unit test for SCMI base protocol, which will exercise all > > > > > > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS. > > > > > > > > $ ut dm scmi_base > > > > > > > > It is assumed that test.dtb is used as sandbox's device tree. > > > > > > > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > > > --- > > > > > > > > test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > > 1 file changed, 112 insertions(+) > > > > > > > > > > > > > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c > > > > > > > > index 881be3171b7c..563017bb63e0 100644 > > > > > > > > --- a/test/dm/scmi.c > > > > > > > > +++ b/test/dm/scmi.c > > > > > > > > @@ -16,6 +16,9 @@ > > > > > > > > #include <clk.h> > > > > > > > > #include <dm.h> > > > > > > > > #include <reset.h> > > > > > > > > +#include <scmi_agent.h> > > > > > > > > +#include <scmi_agent-uclass.h> > > > > > > > > +#include <scmi_protocols.h> > > > > > > > > #include <asm/scmi_test.h> > > > > > > > > #include <dm/device-internal.h> > > > > > > > > #include <dm/test.h> > > > > > > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts) > > > > > > > > } > > > > > > > > DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT); > > > > > > > > > > > > > > > > +static int dm_test_scmi_base(struct unit_test_state *uts) > > > > > > > > +{ > > > > > > > > + struct udevice *agent_dev, *base; > > > > > > > > + struct scmi_agent_priv *priv; > > > > > > > > + const struct scmi_base_ops *ops; > > > > > > > > + u32 version, num_agents, num_protocols, impl_version; > > > > > > > > + u32 attributes, agent_id; > > > > > > > > + char vendor[SCMI_BASE_NAME_LENGTH_MAX], > > > > > > > > + agent_name[SCMI_BASE_NAME_LENGTH_MAX]; > > > > > > > > + u8 *protocols; > > > > > > > > + int ret; > > > > > > > > + > > > > > > > > + /* preparation */ > > > > > > > > + ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi", > > > > > > > > + &agent_dev)); > > > > > > > > + ut_assertnonnull(agent_dev); > > > > > > > > + ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev)); > > > > > > > > + ut_assertnonnull(base = scmi_get_protocol(agent_dev, > > > > > > > > + SCMI_PROTOCOL_ID_BASE)); > > > > > > > > + ut_assertnonnull(ops = dev_get_driver_ops(base)); > > > > > > > > + > > > > > > > > + /* version */ > > > > > > > > + ret = (*ops->protocol_version)(base, &version); > > > > > > > > > > > > > > Can you add uclass helpers to call each of the methods? That is how it > > > > > > > is commonly done. You should not be calling ops->xxx directly here. > > > > > > > > > > > > Yes, I will add inline functions instead. > > > > > > > > > > I don't mean inline...see all the other uclasses which define a > > > > > > > > Okay, I will *real* functions. > > > > > > > > > function which is implemented in the uclass. It is confusing when one > > > > > uclass does something different. People might copy this style and then > > > > > the code base diverges. Did you not notice this when looking around > > > > > the source tree? > > > > > > > > But one concern came up in my mind. > > > > Contrary to ordinary "device controllers", there exists only a single > > > > implementation of driver for each of "udevice"'s associated with SCMI > > > > protocols including the base protocol. > > > > > > > > So if I follow your suggestion, the code (base.c) might look like: > > > > === > > > > static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) > > > > { > > > > ... > > > > } > > > > > > > > struct scmi_base_ops scmi_base_ops = { > > > > > > > > .base_discover_vendor = __scmi_base_discover_vendor, > > > > > > > > } > > > > > > > > int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) > > > > { > > > > struct scmi_base_ops *ops; > > > > > > > > ops = scmi_base_dev_ops(dev); > > > > > > > > return ops->base_discover_vendor(dev, vendor); > > > > } > > > > === > > > > > > > > We will have to have similar definitions for every operation in ops. > > > > It looks quite weird to me as there are always pairs of functions, > > > > like __scmi_base_discover_vendor() and scmi_base_discover_vendor(). > > > > > > Yes I understand that you only have one driver at present. Is there > > > not a sandbox driver? > > > > No. > > Please remember that SCMI protocol drivers on U-Boot are nothing but > > stubs that makes a call to SCMI servers, supporting common communication > > channel interfaces for different transports (either OP-TEE, SMCCC or mailbox). > > > > Sandbox driver, if is properly named, is also implemented as a sort of > > transport layer, where a invocation is replaced with a function call which > > mimicks one of specific commands in SCMI protocol on behalf of a real SCMI server. > > > > In this sense, there will exist only a single driver under the current > > form of framework forever. > > OK, so driver model is used for the transport but not the top-level > driver? I see. I'm not sure if you fully understand. Yes, transports, or interchangeably named as a channel, are modeled as U-Boot devices as you see in drivers/firmware/scmi/*_agent.c (Their names, *_agent, are misleading in my opinion as their functionality is purely a transport method to SCMI server. An agent means, in SCMI jargon, a user or client of SCMI server.) On top of that, each SCMI protocol, the base protocol in my patch set, is also modeled as a U-Boot device. You can see another example, say, in drivers/firmware/clk/clk_scmi.c. Since there is no corresponding uclass for the base protocol, I create a new one (UCLASS_SCMI_BASE) even though it may not be seen an concrete device object. > > > > > > > > > > > > > We can avoid this redundant code easily by eliminating "ops" abstraction. > > > > But as far as I remember, you insist that every driver that complies > > > > to U-Boot driver model should have a "ops". > > > > > > > > What do you make of this? > > > > > > Well there are some exceptions, but yes that is the idea. Operations > > > should be in a 'ops' struct and documented and implemented in a > > > consistent way. > > > > Is it your choice that I should keep "ops" structure in this specific > > implementation? > > I can't actually find this patch on patchwork. Indeed (why?), but you have already seen it. https://lists.denx.de/pipermail/u-boot/2023-June/521288.html > But yes, you do need a function for each ops call. They should be used > in the tests, which should not directly call functions using > ops->xxx() Even though there is no practical benefit to do so. Right? -Takahiro Akashi > > Regards, > Simon
Hi Simon, On Tue, Jul 11, 2023 at 12:41:58PM -0600, Simon Glass wrote: > Hi Takahiro, > > On Mon, 10 Jul 2023 at 19:02, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > Hi Simon, > > > > On Mon, Jul 10, 2023 at 01:45:58PM -0600, Simon Glass wrote: > > > Hi, > > > > > > On Sun, 9 Jul 2023 at 20:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > > > On Fri, Jul 07, 2023 at 11:35:49AM -0600, Simon Glass wrote: > > > > > Hi, > > > > > > > > > > On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote: > > > > > > > Hi, > > > > > > > > > > > > > > On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > > > > > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote: > > > > > > > > > Hi AKASHI, > > > > > > > > > > > > > > > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro > > > > > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > > > > > > > > > Added is a new unit test for SCMI base protocol, which will exercise all > > > > > > > > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS. > > > > > > > > > > $ ut dm scmi_base > > > > > > > > > > It is assumed that test.dtb is used as sandbox's device tree. > > > > > > > > > > > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > > > > > --- > > > > > > > > > > test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > > > > 1 file changed, 112 insertions(+) > > > > > > > > > > > > > > > > > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c > > > > > > > > > > index 881be3171b7c..563017bb63e0 100644 > > > > > > > > > > --- a/test/dm/scmi.c > > > > > > > > > > +++ b/test/dm/scmi.c > > > > > > > > > > @@ -16,6 +16,9 @@ > > > > > > > > > > #include <clk.h> > > > > > > > > > > #include <dm.h> > > > > > > > > > > #include <reset.h> > > > > > > > > > > +#include <scmi_agent.h> > > > > > > > > > > +#include <scmi_agent-uclass.h> > > > > > > > > > > +#include <scmi_protocols.h> > > > > > > > > > > #include <asm/scmi_test.h> > > > > > > > > > > #include <dm/device-internal.h> > > > > > > > > > > #include <dm/test.h> > > > > > > > > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts) > > > > > > > > > > } > > > > > > > > > > DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT); > > > > > > > > > > > > > > > > > > > > +static int dm_test_scmi_base(struct unit_test_state *uts) > > > > > > > > > > +{ > > > > > > > > > > + struct udevice *agent_dev, *base; > > > > > > > > > > + struct scmi_agent_priv *priv; > > > > > > > > > > + const struct scmi_base_ops *ops; > > > > > > > > > > + u32 version, num_agents, num_protocols, impl_version; > > > > > > > > > > + u32 attributes, agent_id; > > > > > > > > > > + char vendor[SCMI_BASE_NAME_LENGTH_MAX], > > > > > > > > > > + agent_name[SCMI_BASE_NAME_LENGTH_MAX]; > > > > > > > > > > + u8 *protocols; > > > > > > > > > > + int ret; > > > > > > > > > > + > > > > > > > > > > + /* preparation */ > > > > > > > > > > + ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi", > > > > > > > > > > + &agent_dev)); > > > > > > > > > > + ut_assertnonnull(agent_dev); > > > > > > > > > > + ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev)); > > > > > > > > > > + ut_assertnonnull(base = scmi_get_protocol(agent_dev, > > > > > > > > > > + SCMI_PROTOCOL_ID_BASE)); > > > > > > > > > > + ut_assertnonnull(ops = dev_get_driver_ops(base)); > > > > > > > > > > + > > > > > > > > > > + /* version */ > > > > > > > > > > + ret = (*ops->protocol_version)(base, &version); > > > > > > > > > > > > > > > > > > Can you add uclass helpers to call each of the methods? That is how it > > > > > > > > > is commonly done. You should not be calling ops->xxx directly here. > > > > > > > > > > > > > > > > Yes, I will add inline functions instead. > > > > > > > > > > > > > > I don't mean inline...see all the other uclasses which define a > > > > > > > > > > > > Okay, I will *real* functions. > > > > > > > > > > > > > function which is implemented in the uclass. It is confusing when one > > > > > > > uclass does something different. People might copy this style and then > > > > > > > the code base diverges. Did you not notice this when looking around > > > > > > > the source tree? > > > > > > > > > > > > But one concern came up in my mind. > > > > > > Contrary to ordinary "device controllers", there exists only a single > > > > > > implementation of driver for each of "udevice"'s associated with SCMI > > > > > > protocols including the base protocol. > > > > > > > > > > > > So if I follow your suggestion, the code (base.c) might look like: > > > > > > === > > > > > > static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) > > > > > > { > > > > > > ... > > > > > > } > > > > > > > > > > > > struct scmi_base_ops scmi_base_ops = { > > > > > > > > > > > > .base_discover_vendor = __scmi_base_discover_vendor, > > > > > > > > > > > > } > > > > > > > > > > > > int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) > > > > > > { > > > > > > struct scmi_base_ops *ops; > > > > > > > > > > > > ops = scmi_base_dev_ops(dev); > > > > > > > > > > > > return ops->base_discover_vendor(dev, vendor); > > > > > > } > > > > > > === > > > > > > > > > > > > We will have to have similar definitions for every operation in ops. > > > > > > It looks quite weird to me as there are always pairs of functions, > > > > > > like __scmi_base_discover_vendor() and scmi_base_discover_vendor(). > > > > > > > > > > Yes I understand that you only have one driver at present. Is there > > > > > not a sandbox driver? > > > > > > > > No. > > > > Please remember that SCMI protocol drivers on U-Boot are nothing but > > > > stubs that makes a call to SCMI servers, supporting common communication > > > > channel interfaces for different transports (either OP-TEE, SMCCC or mailbox). > > > > > > > > Sandbox driver, if is properly named, is also implemented as a sort of > > > > transport layer, where a invocation is replaced with a function call which > > > > mimicks one of specific commands in SCMI protocol on behalf of a real SCMI server. > > > > > > > > In this sense, there will exist only a single driver under the current > > > > form of framework forever. > > > > > > OK, so driver model is used for the transport but not the top-level > > > driver? I see. > > > > I'm not sure if you fully understand. > > Yes, transports, or interchangeably named as a channel, are modeled > > as U-Boot devices as you see in drivers/firmware/scmi/*_agent.c > > (Their names, *_agent, are misleading in my opinion as their functionality > > is purely a transport method to SCMI server. An agent means, in SCMI jargon, > > a user or client of SCMI server.) > > > > On top of that, each SCMI protocol, the base protocol in my patch set, > > is also modeled as a U-Boot device. > > You can see another example, say, in drivers/firmware/clk/clk_scmi.c. > > > > Since there is no corresponding uclass for the base protocol, I create > > a new one (UCLASS_SCMI_BASE) even though it may not be seen an concrete > > device object. > > It doesn't need to be 'concrete', whatever that means. A uclass is a > model of hardware or something else. We have lots of drivers that > don't deal with a hardware device. Yes, I know. > > > > > > > > > > > > > > > > > > > > > > > We can avoid this redundant code easily by eliminating "ops" abstraction. > > > > > > But as far as I remember, you insist that every driver that complies > > > > > > to U-Boot driver model should have a "ops". > > > > > > > > > > > > What do you make of this? > > > > > > > > > > Well there are some exceptions, but yes that is the idea. Operations > > > > > should be in a 'ops' struct and documented and implemented in a > > > > > consistent way. > > > > > > > > Is it your choice that I should keep "ops" structure in this specific > > > > implementation? > > > > > > I can't actually find this patch on patchwork. > > > > Indeed (why?), but you have already seen it. > > https://lists.denx.de/pipermail/u-boot/2023-June/521288.html > > I mean patchwork. Yes, I know. But even a bunch of patches listed in patchwork are left "new" state forever. > > > > > But yes, you do need a function for each ops call. They should be used > > > in the tests, which should not directly call functions using > > > ops->xxx() > > > > Even though there is no practical benefit to do so. Right? > > I don't have a useful answer to that question. If you want to > contribute to U-Boot, please follow its conventions. It is just > frustrating for people doing code review, with limited time. I simply wanted to double-check the rule since you mention, in another thread, that there are some exceptions. > While xxx may be unique in U-Boot, I very much doubt it always is, so > using 'ops' is not helping people looking through the code, nor is it > obvious what is happening without looking up 'ops'. The helper > functions should be used in all cases. > > The thing is, this convention has been in place since the start of > driver model, so I wonder how you have missed it? Well, probably I was confused with many uses of function pointers for boot and runtime services in UEFI code. That said, after rethinking, I decided to remove udevice-related code from my patch here, i.e. there will be no U-Boot device for the base protocol as it is a protocol not a concrete device. Its sole purpose is to get *meta* information about SCMI server and the services, then manipulate them. The analogy is that HTTP or FTP is a protocol over an ethernet device to access a remote application (server), but is not a device object in any sense. > Please also fix the other uclasses, as I see for example that > devm_scmi_process_msg() calls ops->process_msg() when it should have > an exported function in that file. I'm not sure that this is the case. devm_scmi_process_mesg() is just a wrapper, as the name suggests, to ops->process_msg() like other uclass driver operations, say blk_read/blk_write(). The only difference, if any, "ops" doesn't derive from the device itself, but it parent (or grand^* parent). If you really want, I will fix it (but in a separate patch). -Takahiro Akashi > Every uclass should have its > operations in a struct, clearly documented, as well as helper > functions to call each operation. > > Regards, > Simon
Hi, On Thu, 13 Jul 2023 at 18:42, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Hi Simon, > > On Tue, Jul 11, 2023 at 12:41:58PM -0600, Simon Glass wrote: > > Hi Takahiro, > > > > On Mon, 10 Jul 2023 at 19:02, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > On Mon, Jul 10, 2023 at 01:45:58PM -0600, Simon Glass wrote: > > > > Hi, > > > > > > > > On Sun, 9 Jul 2023 at 20:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > On Fri, Jul 07, 2023 at 11:35:49AM -0600, Simon Glass wrote: > > > > > > Hi, > > > > > > > > > > > > On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > > > > > > > On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote: > > > > > > > > > > Hi AKASHI, > > > > > > > > > > > > > > > > > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro > > > > > > > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > > > > > > > > > > > > > Added is a new unit test for SCMI base protocol, which will exercise all > > > > > > > > > > > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS. > > > > > > > > > > > $ ut dm scmi_base > > > > > > > > > > > It is assumed that test.dtb is used as sandbox's device tree. > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > > > > > > --- > > > > > > > > > > > test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > > > > > 1 file changed, 112 insertions(+) > > > > > > > > > > > > > > > > > > > > > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c > > > > > > > > > > > index 881be3171b7c..563017bb63e0 100644 > > > > > > > > > > > --- a/test/dm/scmi.c > > > > > > > > > > > +++ b/test/dm/scmi.c > > > > > > > > > > > @@ -16,6 +16,9 @@ > > > > > > > > > > > #include <clk.h> > > > > > > > > > > > #include <dm.h> > > > > > > > > > > > #include <reset.h> > > > > > > > > > > > +#include <scmi_agent.h> > > > > > > > > > > > +#include <scmi_agent-uclass.h> > > > > > > > > > > > +#include <scmi_protocols.h> > > > > > > > > > > > #include <asm/scmi_test.h> > > > > > > > > > > > #include <dm/device-internal.h> > > > > > > > > > > > #include <dm/test.h> > > > > > > > > > > > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts) > > > > > > > > > > > } > > > > > > > > > > > DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT); > > > > > > > > > > > > > > > > > > > > > > +static int dm_test_scmi_base(struct unit_test_state *uts) > > > > > > > > > > > +{ > > > > > > > > > > > + struct udevice *agent_dev, *base; > > > > > > > > > > > + struct scmi_agent_priv *priv; > > > > > > > > > > > + const struct scmi_base_ops *ops; > > > > > > > > > > > + u32 version, num_agents, num_protocols, impl_version; > > > > > > > > > > > + u32 attributes, agent_id; > > > > > > > > > > > + char vendor[SCMI_BASE_NAME_LENGTH_MAX], > > > > > > > > > > > + agent_name[SCMI_BASE_NAME_LENGTH_MAX]; > > > > > > > > > > > + u8 *protocols; > > > > > > > > > > > + int ret; > > > > > > > > > > > + > > > > > > > > > > > + /* preparation */ > > > > > > > > > > > + ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi", > > > > > > > > > > > + &agent_dev)); > > > > > > > > > > > + ut_assertnonnull(agent_dev); > > > > > > > > > > > + ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev)); > > > > > > > > > > > + ut_assertnonnull(base = scmi_get_protocol(agent_dev, > > > > > > > > > > > + SCMI_PROTOCOL_ID_BASE)); > > > > > > > > > > > + ut_assertnonnull(ops = dev_get_driver_ops(base)); > > > > > > > > > > > + > > > > > > > > > > > + /* version */ > > > > > > > > > > > + ret = (*ops->protocol_version)(base, &version); > > > > > > > > > > > > > > > > > > > > Can you add uclass helpers to call each of the methods? That is how it > > > > > > > > > > is commonly done. You should not be calling ops->xxx directly here. > > > > > > > > > > > > > > > > > > Yes, I will add inline functions instead. > > > > > > > > > > > > > > > > I don't mean inline...see all the other uclasses which define a > > > > > > > > > > > > > > Okay, I will *real* functions. > > > > > > > > > > > > > > > function which is implemented in the uclass. It is confusing when one > > > > > > > > uclass does something different. People might copy this style and then > > > > > > > > the code base diverges. Did you not notice this when looking around > > > > > > > > the source tree? > > > > > > > > > > > > > > But one concern came up in my mind. > > > > > > > Contrary to ordinary "device controllers", there exists only a single > > > > > > > implementation of driver for each of "udevice"'s associated with SCMI > > > > > > > protocols including the base protocol. > > > > > > > > > > > > > > So if I follow your suggestion, the code (base.c) might look like: > > > > > > > === > > > > > > > static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) > > > > > > > { > > > > > > > ... > > > > > > > } > > > > > > > > > > > > > > struct scmi_base_ops scmi_base_ops = { > > > > > > > > > > > > > > .base_discover_vendor = __scmi_base_discover_vendor, > > > > > > > > > > > > > > } > > > > > > > > > > > > > > int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) > > > > > > > { > > > > > > > struct scmi_base_ops *ops; > > > > > > > > > > > > > > ops = scmi_base_dev_ops(dev); > > > > > > > > > > > > > > return ops->base_discover_vendor(dev, vendor); > > > > > > > } > > > > > > > === > > > > > > > > > > > > > > We will have to have similar definitions for every operation in ops. > > > > > > > It looks quite weird to me as there are always pairs of functions, > > > > > > > like __scmi_base_discover_vendor() and scmi_base_discover_vendor(). > > > > > > > > > > > > Yes I understand that you only have one driver at present. Is there > > > > > > not a sandbox driver? > > > > > > > > > > No. > > > > > Please remember that SCMI protocol drivers on U-Boot are nothing but > > > > > stubs that makes a call to SCMI servers, supporting common communication > > > > > channel interfaces for different transports (either OP-TEE, SMCCC or mailbox). > > > > > > > > > > Sandbox driver, if is properly named, is also implemented as a sort of > > > > > transport layer, where a invocation is replaced with a function call which > > > > > mimicks one of specific commands in SCMI protocol on behalf of a real SCMI server. > > > > > > > > > > In this sense, there will exist only a single driver under the current > > > > > form of framework forever. > > > > > > > > OK, so driver model is used for the transport but not the top-level > > > > driver? I see. > > > > > > I'm not sure if you fully understand. > > > Yes, transports, or interchangeably named as a channel, are modeled > > > as U-Boot devices as you see in drivers/firmware/scmi/*_agent.c > > > (Their names, *_agent, are misleading in my opinion as their functionality > > > is purely a transport method to SCMI server. An agent means, in SCMI jargon, > > > a user or client of SCMI server.) > > > > > > On top of that, each SCMI protocol, the base protocol in my patch set, > > > is also modeled as a U-Boot device. > > > You can see another example, say, in drivers/firmware/clk/clk_scmi.c. > > > > > > Since there is no corresponding uclass for the base protocol, I create > > > a new one (UCLASS_SCMI_BASE) even though it may not be seen an concrete > > > device object. > > > > It doesn't need to be 'concrete', whatever that means. A uclass is a > > model of hardware or something else. We have lots of drivers that > > don't deal with a hardware device. > > Yes, I know. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > We can avoid this redundant code easily by eliminating "ops" abstraction. > > > > > > > But as far as I remember, you insist that every driver that complies > > > > > > > to U-Boot driver model should have a "ops". > > > > > > > > > > > > > > What do you make of this? > > > > > > > > > > > > Well there are some exceptions, but yes that is the idea. Operations > > > > > > should be in a 'ops' struct and documented and implemented in a > > > > > > consistent way. > > > > > > > > > > Is it your choice that I should keep "ops" structure in this specific > > > > > implementation? > > > > > > > > I can't actually find this patch on patchwork. > > > > > > Indeed (why?), but you have already seen it. > > > https://lists.denx.de/pipermail/u-boot/2023-June/521288.html > > > > I mean patchwork. > > Yes, I know. > But even a bunch of patches listed in patchwork are left "new" state forever. > > > > > > > > But yes, you do need a function for each ops call. They should be used > > > > in the tests, which should not directly call functions using > > > > ops->xxx() > > > > > > Even though there is no practical benefit to do so. Right? > > > > I don't have a useful answer to that question. If you want to > > contribute to U-Boot, please follow its conventions. It is just > > frustrating for people doing code review, with limited time. > > I simply wanted to double-check the rule since you mention, > in another thread, that there are some exceptions. > > > While xxx may be unique in U-Boot, I very much doubt it always is, so > > using 'ops' is not helping people looking through the code, nor is it > > obvious what is happening without looking up 'ops'. The helper > > functions should be used in all cases. > > > > The thing is, this convention has been in place since the start of > > driver model, so I wonder how you have missed it? > > Well, probably I was confused with many uses of function pointers > for boot and runtime services in UEFI code. Perhaps, but you mustn't add to the confusion with more counter-examples. > > That said, after rethinking, I decided to remove udevice-related > code from my patch here, i.e. there will be no U-Boot device > for the base protocol as it is a protocol not a concrete device. > Its sole purpose is to get *meta* information about SCMI server > and the services, then manipulate them. > > The analogy is that HTTP or FTP is a protocol over an ethernet > device to access a remote application (server), but is not a > device object in any sense. Please don't. I don't know what you are referring to with a concrete device, but it isn't relevant to driver model. It has lots of virtual things in it. > > > Please also fix the other uclasses, as I see for example that > > devm_scmi_process_msg() calls ops->process_msg() when it should have > > an exported function in that file. > > I'm not sure that this is the case. devm_scmi_process_mesg() > is just a wrapper, as the name suggests, to ops->process_msg() > like other uclass driver operations, say blk_read/blk_write(). > The only difference, if any, "ops" doesn't derive from the device > itself, but it parent (or grand^* parent). > > If you really want, I will fix it (but in a separate patch). Then call the function on the parent device if you like. See how this works in the pmic subsystem, for example. > > -Takahiro Akashi > > > > Every uclass should have its > > operations in a struct, clearly documented, as well as helper > > functions to call each operation. > > > > Regards, > > Simon Regards, Simon
diff --git a/test/dm/scmi.c b/test/dm/scmi.c index 881be3171b7c..563017bb63e0 100644 --- a/test/dm/scmi.c +++ b/test/dm/scmi.c @@ -16,6 +16,9 @@ #include <clk.h> #include <dm.h> #include <reset.h> +#include <scmi_agent.h> +#include <scmi_agent-uclass.h> +#include <scmi_protocols.h> #include <asm/scmi_test.h> #include <dm/device-internal.h> #include <dm/test.h> @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts) } DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT); +static int dm_test_scmi_base(struct unit_test_state *uts) +{ + struct udevice *agent_dev, *base; + struct scmi_agent_priv *priv; + const struct scmi_base_ops *ops; + u32 version, num_agents, num_protocols, impl_version; + u32 attributes, agent_id; + char vendor[SCMI_BASE_NAME_LENGTH_MAX], + agent_name[SCMI_BASE_NAME_LENGTH_MAX]; + u8 *protocols; + int ret; + + /* preparation */ + ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi", + &agent_dev)); + ut_assertnonnull(agent_dev); + ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev)); + ut_assertnonnull(base = scmi_get_protocol(agent_dev, + SCMI_PROTOCOL_ID_BASE)); + ut_assertnonnull(ops = dev_get_driver_ops(base)); + + /* version */ + ret = (*ops->protocol_version)(base, &version); + ut_assertok(ret); + ut_asserteq(priv->version, version); + + /* protocol attributes */ + ret = (*ops->protocol_attrs)(base, &num_agents, &num_protocols); + ut_assertok(ret); + ut_asserteq(priv->num_agents, num_agents); + ut_asserteq(priv->num_protocols, num_protocols); + + /* discover vendor */ + ret = (*ops->base_discover_vendor)(base, vendor); + ut_assertok(ret); + ut_asserteq_str(priv->vendor, vendor); + + /* message attributes */ + ret = (*ops->protocol_message_attrs)(base, + SCMI_BASE_DISCOVER_SUB_VENDOR, + &attributes); + ut_assertok(ret); + ut_assertok(attributes); + + /* discover sub vendor */ + ret = (*ops->base_discover_sub_vendor)(base, vendor); + ut_assertok(ret); + ut_asserteq_str(priv->sub_vendor, vendor); + + /* impl version */ + ret = (*ops->base_discover_impl_version)(base, &impl_version); + ut_assertok(ret); + ut_asserteq(priv->impl_version, impl_version); + + /* discover agent (my self) */ + ret = (*ops->base_discover_agent)(base, 0xffffffff, + &agent_id, agent_name); + ut_assertok(ret); + ut_asserteq(priv->agent_id, agent_id); + ut_asserteq_str(priv->agent_name, agent_name); + + /* discover protocols */ + ret = (*ops->base_discover_list_protocols)(base, &protocols); + ut_asserteq(num_protocols, ret); + ut_asserteq_mem(priv->protocols, protocols, sizeof(u8) * num_protocols); + free(protocols); + + /* + * NOTE: Sandbox SCMI driver handles device-0 only. It supports setting + * access and protocol permissions, but doesn't allow unsetting them nor + * resetting the configurations. + */ + /* set device permissions */ + ret = (*ops->base_set_device_permissions)(base, agent_id, 0, + SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS); + ut_assertok(ret); /* SCMI_SUCCESS */ + ret = (*ops->base_set_device_permissions)(base, agent_id, 1, + SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS); + ut_asserteq(-ENOENT, ret); /* SCMI_NOT_FOUND */ + ret = (*ops->base_set_device_permissions)(base, agent_id, 0, 0); + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ + + /* set protocol permissions */ + ret = (*ops->base_set_protocol_permissions)(base, agent_id, 0, + SCMI_PROTOCOL_ID_CLOCK, + SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS); + ut_assertok(ret); /* SCMI_SUCCESS */ + ret = (*ops->base_set_protocol_permissions)(base, agent_id, 1, + SCMI_PROTOCOL_ID_CLOCK, + SCMI_BASE_SET_PROTOCOL_PERMISSIONS_ACCESS); + ut_asserteq(-ENOENT, ret); /* SCMI_NOT_FOUND */ + ret = (*ops->base_set_protocol_permissions)(base, agent_id, 0, + SCMI_PROTOCOL_ID_CLOCK, 0); + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ + + /* reset agent configuration */ + ret = (*ops->base_reset_agent_configuration)(base, agent_id, 0); + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ + ret = (*ops->base_reset_agent_configuration)(base, agent_id, + SCMI_BASE_RESET_ALL_ACCESS_PERMISSIONS); + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ + ret = (*ops->base_reset_agent_configuration)(base, agent_id, 0); + ut_asserteq(-EACCES, ret); /* SCMI_DENIED */ + + return 0; +} + +DM_TEST(dm_test_scmi_base, UT_TESTF_SCAN_FDT); + static int dm_test_scmi_clocks(struct unit_test_state *uts) { struct sandbox_scmi_agent *agent;
Added is a new unit test for SCMI base protocol, which will exercise all the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS. $ ut dm scmi_base It is assumed that test.dtb is used as sandbox's device tree. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+)