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

Alexey Romanov AVRomanov at sberdevices.ru
Tue Aug 8 10:14:16 CEST 2023 

Hi Neil,

I would like to know your opinion before I change the patches.

On Sat, Jul 15, 2023 at 05:40:53PM -0600, Simon Glass wrote:
> Hi,
> 
> On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro
> <takahiro.akashi at linaro.org> wrote:
> >
> > On Tue, Jul 11, 2023 at 01:13:29PM -0600, Simon Glass wrote:
> > > +AKASHI Takahiro
> >
> > Me?
> 
> Yes, I'm asking for your help to try to clean this stuff up.
> 
> >
> > > 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.
> >
> > Do you mean a "sandbox_scmi_devices" in drivers/firmware/scmi/sandbox-scmi_devices.c?
> > If so, it has nothing to do with the discussion here.
> >
> > It is a kind of pseudo device for dm ut, which is subject to manage
> > with the faked scmi server in sandbox (sandbox-scmi_agent.c).
> > It is connected, for instance, to clock lines or vol regulators.
> > (see sandbox's test.dts.)
> >
> > I believe that similar usages can be seen across drivers/, for instance,
> > drivers/clk/clk_sandbox_test.c.
> >
> > Do you see anything wrong with it?
> 
> At this point I'm just confused. Perhaps MISC and FIRMWARE should not
> be used as much? It seems that FIRMWARE doesn't really mean anything
> at present?
> 
> >
> > > 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?
> >
> > SMCCC stands for SMC Calling Convention, where SMC (or Secure Monitor Call)
> > instruction escalates the cpu's EL (Execution Level) to EL3 (secure).
> > This interface is clearly defined by Arm, and many high level services are
> > defined and implemented on top of that.
> > PSCI and FF-A are ones among those users, and even vendor specific functions
> > may be allowed.
> >
> > From the viewpoint of implementation, arm_smccc_smc() (and smc_call()) is
> > just a C helper function around one SMC assembly. I don't think we need to
> > have UCLASS_SMC as it is basically a communication method.
> >
> > Nevertheless, I agree with Simon in the point that the SMC related code
> > can be a bit reworked since there are duplicated code.
> > Especially, the followings are essentially the same:
> >   -- arch/arm/cpu/armv8/fwcall.c with smc_call()
> >        psci_system_reset()
> >        psci_system_off()
> >   -- drivers/firmware/psci.c with arm_smccc_smc()
> >        psci_sys_reset()
> >        psci_sys_poweroff()
> 
> OK, what that is a start.
> 
> >
> >
> > > > It seemed to me that UCLASS_FIRMWARE is a SoC(arch)-specific driver
> > > > that can work with any interface.
> >
> > IMO, UCLASS_FIRMWARE is quite misleading as all the drivers belonging to
> > this class do not have any common operations.
> 
> Well misc.h does actually have operations, but yes, people misuse it.
> Perhaps we should invent something else for these people?
> 
> For FIRMWARE, what should it mean? It needs to have a firmware.h file
> with some information.
> 
> > (And UCLASS_MISC as well.)
> >
> > -Takahiro Akashi
> >
> > > 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
> 
> Regards,
> Simon

-- 
Thank you,
Alexey

More information about the U-Boot mailing list