Re: [PATCH v6 08/14] firmware: scmi: add a version check against base protocol

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Hi Etienne,

Thank you again for your review.

On Wed, Oct 11, 2023 at 03:44:36PM +0000, Etienne CARRIERE - foss wrote:
> > From: U-Boot <u-boot-bounces@lists.denx.de> on behalf of AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Sent: Wednesday, October 11, 2023 12:07 PM
> > 
> > In SCMI base protocol version 2 (0x20000), new interfaces,
> > BASE_SET_DEVICE_PERMISSIONS/BASE_SET_PROTOCOL_PERMISSIONS/
> > BASE_RESET_AGENT_CONFIGURATION, were added. Moreover, the api of
> > BASE_DISCOVER_AGENT was changed to support self-agent discovery.
> > 
> > So the driver expects SCMI firmware support version 2 of base protocol.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> > v6
> > * new commit
> > ---
> >  drivers/firmware/scmi/base.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/firmware/scmi/base.c b/drivers/firmware/scmi/base.c
> > index ee84e261945a..1d41a8a98fc6 100644
> > --- a/drivers/firmware/scmi/base.c
> > +++ b/drivers/firmware/scmi/base.c
> > @@ -481,6 +481,7 @@ static int scmi_base_reset_agent_configuration_int(struct udevice *dev,
> >   */
> >  static int scmi_base_probe(struct udevice *dev)
> >  {
> > +       u32 version;
> >         int ret;
> > 
> >         ret = devm_scmi_of_get_channel(dev);
> > @@ -488,6 +489,13 @@ static int scmi_base_probe(struct udevice *dev)
> >                 dev_err(dev, "get_channel failed\n");
> >                 return ret;
> >         }
> > +       ret = scmi_base_protocol_version_int(dev, &version);
> > +       if (ret) {
> > +               dev_err(dev, "getting protocol version failed\n");
> > +               return ret;
> > +       }
> > +       if (version < SCMI_BASE_PROTOCOL_VERSION)
> > +               return -EINVAL;
> 
> LGTM. The open source SCMI server implementations I'm aware of (scp-firmware, tf-a and optee-os) all report SCMI Base protocol version v2.0.

Yeah, I also confirmed this.

> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
> 
> That said, maybe a more flexible implementation would support both version v1.0 (0x10000) and v2.0

SCMI v2.0 (not the base protocol, but the specification itself)
introduced base protocol v2.0 and the spec was published in 2019.
Along with the status of the existing SCMI server implementation (as you
mentioned above), I doubt any chance that there comes up any new SCMI with
the base protocol v1.0 in the future.

> but disable permission commands for v1.0 compliant servers. Maybe in a later change, if there is a need for.

Yes, we can make a tweak later. But anyhow "permission" commands are
categorized as optional even in v2.0 (i.e. the firmware may still return
SCMI_NOT_SUPPORTED) and U-Boot has no framework to utilize them for now.

BTW, I will not add a version check against all the existing protocols
(you implemented) as they utilize only the interfaces defined in each one's v1.0.

Thanks,
-Takahiro Akashi

> I fear using such strict minima protocol version values for other SCMI procotols make U-Boot client too restrictive.
> 
> BR,
> Etienne
> 
> > 
> >         return ret;
> >  }
> > --
> > 2.34.1
> > 
> >