[PATCH v1 1/2] drivers: firmware: introduce Meson Secure Monitor driver

[Simon Glass]

+AKASHI Takahiro

Hi Alexey,

On Tue, 11 Jul 2023 at 04:25, Alexey Romanov <AVRomanov at sberdevices.ru> wrote:
>
> Hi Simon,
>
> On Mon, Jul 10, 2023 at 01:45:53PM -0600, Simon Glass wrote:
> > Hi Alexey,
> >
> > On Mon, 10 Jul 2023 at 02:34, Alexey Romanov <AVRomanov at sberdevices.ru> wrote:
> > >
> > >
> > > Hello!
> > >
> > > On Fri, Jul 07, 2023 at 11:35:47AM -0600, Simon Glass wrote:
> > > > Hi Alexey,
> > > >
> > > > On Fri, 7 Jul 2023 at 09:43, Alexey Romanov <AVRomanov at sberdevices.ru> wrote:
> > > > >
> > > > > Hello, Simon!
> > > > >
> > > > > On Thu, Jul 06, 2023 at 09:58:02AM -0600, Simon Glass wrote:
> > > > > > Hi Alexey,
> > > > > >
> > > > > > On Thu, 6 Jul 2023 at 14:16, Alexey Romanov <avromanov at sberdevices.ru> wrote:
> > > > > > >
> > > > > > > At the moment, only smc API is a set of functions in
> > > > > > > arch/arm/mach-meson/sm.c. This approach is hard to configure
> > > > > > > and also doesni't contain any generic API for calling smc.
> > > > > > >
> > > > > > > This patch add Meson SM driver with generic API (struct meson_sm_ops):
> > > > > > >
> > > > > > > - sm_call()
> > > > > > > - sm_call_write()
> > > > > > > - sm_call_read()
> > > > > > >
> > > > > > > A typical driver usage example is shown here:
> > > > > > >
> > > > > > > 1. uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
> > > > > > > 2. handle = meson_sm_get_handle(dev);
> > > > > > > 3. handle->ops.sm_call(dev, cmd, ...);
> > > > > > >
> > > > > > > Signed-off-by: Alexey Romanov <avromanov at sberdevices.ru>
> > > > > > > ---
> > > > > > >  arch/arm/mach-meson/Kconfig       |   1 +
> > > > > > >  drivers/firmware/Kconfig          |  10 ++
> > > > > > >  drivers/firmware/Makefile         |   1 +
> > > > > > >  drivers/firmware/meson/Kconfig    |   6 +
> > > > > > >  drivers/firmware/meson/Makefile   |   3 +
> > > > > > >  drivers/firmware/meson/meson_sm.c | 217 ++++++++++++++++++++++++++++++
> > > > > > >  include/meson/sm_handle.h         |  38 ++++++
> > > > > > >  7 files changed, 276 insertions(+)
> > > > > > >  create mode 100644 drivers/firmware/meson/Kconfig
> > > > > > >  create mode 100644 drivers/firmware/meson/Makefile
> > > > > > >  create mode 100644 drivers/firmware/meson/meson_sm.c
> > > > > > >  create mode 100644 include/meson/sm_handle.h
> > > > > >
> > > > > > Please can you use the remoteproc uclass for this and add a proper driver?
> > > > > >
> > > > >
> > > > > I don't see it architecturally well. Can you explain please?
> > > > >
> > > > > This driver is just ARM SMC fw interface. There seems to be nothing to
> > > > > do here for remoteproc uclass.
> > > >
> > > > Well you seem to be implementing a remote CPU interface, which is what
> > > > remoteproc is for. How does Linux do this?
> > >
> > > The main idea of the patchset is to abstract the smc calls (which run on
> > > the same CPU) and make a request to the firmware that runs on a higher
> > > EL. UCLASS_REMOTEPROC may give the impression that we are
> > > interacting with another CPU, although this is not the case.
> > >
> > > Also, UCLASS_REMOTEPROC requires two mandatory interfaces: load() and
> > > start(), and I don't even know how to apply my current changes to them.
> >
> > You can return -ENOSYS if not implemented.
> >
> > >
> > > My implementation is very close to the Linux implementation, they
> > > also use the firmware driver without remoteproc:
> > >
> > > https://elixir.bootlin.com/linux/latest/source/drivers/firmware/meson/meson_sm.c
> >
> > Yes that seems like it doesn't use any common infra. I don't think it
> > is a great model for U-Boot though.
> >
> > >
> > > I spoke to Neil on IRC and he said UCLASS_FIRMWARE for such driver is
> > > OK.
> > >
> > > >
> > > > Also there is a pending series on FFA - is that related? It uses smc
> > > > from what I can tell.
> > >
> > > Not entirely clear, my changes don't seem to be related to this
> > > patchset.
> >
> > So perhaps this needs a new UCLASS_SMC? I see various other SMC calls
> > in U-Boot but no one has taken the initiative to think about this in
> > terms of driver model.
>
> What will be the feature of this uclass? If it's just us adding a new
> uclass with a different name... Don't know if that makes amy sense?
> Then the difference between UCLASS_SMC and UCLASS_FIRMWARE will be only
> in the name.

Honestly I'm not sure, but I question why you are putting all this on
the reviewer. Can't you look around the code base and figure out a
sensible approach and defend it in your patch?

I've just reviewed the SCMI stuff which use UCLASS_MISC in one case.
I'm not sure if this is related to firmware or not, or whether what
you are doing relates.

>
> >
> > It might just need one operation, to make a call, passing a regs
> > struct, perhaps? The uclass would presumably be ARM-specific.
> >
> > I really don't think this is UCLASS_FIRMWARE. That seems to be for
> > loading firmware. It doesn't even have a firmware.h header.
>
> I see what drivers/firmware/psci.c is UCLASS_FIRMWARE and also use
> smc_call() function. Or firmware-zynqmo.c. In this case, we then have to
> convert them to UCLASS_SMC in the same way?
>
> It seemed to me that UCLASS_FIRMWARE is a SoC(arch)-specific driver
> that can work with any interface.

OK, well please take a look at all this and see what you think is
best. If UCLASS_FIRMWARE is correct, then please create a proper
firmware.h file and define what the uclass operations are. You can't
just randomly create your own private operations and ignore the rest
of the code base.

Driver model is intended to bring order to the chaos that used to
exist with drivers in U-Boot. Please see [1] for an introduction.

Please try to dig in and understand how things should be done. It is
important for the health of the project.

>
> >
> > Also instead of:
> >
> > 1. uclass_get_device_by_driver(UCLASS_FIRMWARE, "secure-monitor", &dev);
> >
> > use:
> >
> > ret = uclass_first_device_err(UCLASS_SMC, &dev)
> >
> > using device tree to find the device.
> >
> > Regards,
> > Simon
>
> --
> Thank you,
> Alexey

[1] https://elinux.org/Boot_Loaders#Order_at_Last:_The_New_U-Boot_Driver_Model_Architecture_.5BELCE_2015.5D