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 <avroma...@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 <avroma...@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 > > > > > <avroma...@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 <avroma...@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. > > 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. > > 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
