Re: [PATCH 07/10] test: dm: add SCMI base protocol test
Hi,
On Thu, 13 Jul 2023 at 18:42, AKASHI Takahiro
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
> > 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
> > > > 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
> > > > > > 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
> > > > > > > > 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
> > > > > > > > > > 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
> > > > > > > > > > >
> > > > > > > > > > > ---
> > > > > > > > > > > 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
> > > > > > > > > > > #include
> > > > > > > > > > > #include
> > > > > > > > > > > +#include
> > > > > > > > > > > +#include
> > > > > > > > > > > +#include
> > > > > > > > > > > #include
> > > > > > > > > > > #include
> > > > > > > > > > > #include
> > > > > > > > > > > @@ -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",
> > > > > > > > > > > +
> > > > > > > > > > > _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, );
> > > > > > > > > >
> > > > > > > > > > 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
Re: [PATCH 07/10] test: dm: add SCMI base protocol test
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
> 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
> > > 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
> > > > > 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
> > > > > > > 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
> > > > > > > > > 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
> > > > > > > > > > ---
> > > > > > > > > > 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
> > > > > > > > > > #include
> > > > > > > > > > #include
> > > > > > > > > > +#include
> > > > > > > > > > +#include
> > > > > > > > > > +#include
> > > > > > > > > > #include
> > > > > > > > > > #include
> > > > > > > > > > #include
> > > > > > > > > > @@ -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",
> > > > > > > > > > + _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, );
> > > > > > > > >
> > > > > > > > > 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.
Re: [PATCH 07/10] test: dm: add SCMI base protocol test
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
> 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
> > > 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
> > > > > 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
> > > > > > > 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
> > > > > > > > ---
> > > > > > > > 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
> > > > > > > > #include
> > > > > > > > #include
> > > > > > > > +#include
> > > > > > > > +#include
> > > > > > > > +#include
> > > > > > > > #include
> > > > > > > > #include
> > > > > > > > #include
> > > > > > > > @@ -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",
> > > > > > > > + _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, );
> > > > > > >
> > > > > > > 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
Re: [PATCH 07/10] test: dm: add SCMI base protocol test
Hi,
On Sun, 9 Jul 2023 at 20:04, AKASHI Takahiro 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
> > 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
> > > > 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
> > > > > > 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
> > > > > > > ---
> > > > > > > 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
> > > > > > > #include
> > > > > > > #include
> > > > > > > +#include
> > > > > > > +#include
> > > > > > > +#include
> > > > > > > #include
> > > > > > > #include
> > > > > > > #include
> > > > > > > @@ -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",
> > > > > > > + _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, );
> > > > > >
> > > > > > 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,
Re: [PATCH 07/10] test: dm: add SCMI base protocol test
On Fri, Jul 07, 2023 at 11:35:49AM -0600, Simon Glass wrote:
> Hi,
>
> On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro
> 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
> > > 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
> > > > > 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
> > > > > > ---
> > > > > > 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
> > > > > > #include
> > > > > > #include
> > > > > > +#include
> > > > > > +#include
> > > > > > +#include
> > > > > > #include
> > > > > > #include
> > > > > > #include
> > > > > > @@ -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",
> > > > > > + _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, );
> > > > >
> > > > > 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
Re: [PATCH 07/10] test: dm: add SCMI base protocol test
Hi,
On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro 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
> > 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
> > > > 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
> > > > > ---
> > > > > 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
> > > > > #include
> > > > > #include
> > > > > +#include
> > > > > +#include
> > > > > +#include
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > @@ -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",
> > > > > + _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, );
> > > >
> > > > 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
Re: [PATCH 07/10] test: dm: add SCMI base protocol test
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
> 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
> > > 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
> > > > ---
> > > > 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
> > > > #include
> > > > #include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -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",
> > > > + _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, );
> > >
> > > 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
Re: [PATCH 07/10] test: dm: add SCMI base protocol test
Hi,
On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro 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
> > 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
> > > ---
> > > 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
> > > #include
> > > #include
> > > +#include
> > > +#include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -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",
> > > + _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, );
> >
> > 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
Re: [PATCH 07/10] test: dm: add SCMI base protocol test
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
> 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
> > ---
> > 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
> > #include
> > #include
> > +#include
> > +#include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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",
> > + _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, );
>
> 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, _agents, _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,
> > +);
> > + 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, _version);
> > + ut_assertok(ret);
> > + ut_asserteq(priv->impl_version, impl_version);
> > +
> > + /* discover agent (my self) */
> > + ret = (*ops->base_discover_agent)(base, 0x,
> > + _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, );
> > + 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 */
>
Re: [PATCH 07/10] test: dm: add SCMI base protocol test
Hi AKASHI,
On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
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
> ---
> 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
> #include
> #include
> +#include
> +#include
> +#include
> #include
> #include
> #include
> @@ -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",
> + _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, );
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, _agents, _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,
> +);
> + 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, _version);
> + ut_assertok(ret);
> + ut_asserteq(priv->impl_version, impl_version);
> +
> + /* discover agent (my self) */
> + ret = (*ops->base_discover_agent)(base, 0x,
> + _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, );
> + 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,
>
