| Message ID | 20230706112020.24528-2-avromanov@sberdevices.ru |
|---|---|
| State | New |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | Introduce Meson SM driver | expand |
Hi Alexey, On Thu, 6 Jul 2023 at 14:16, Alexey Romanov <avromanov@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 doesn'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@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? Regards, SImon
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@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@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. > Regards, > SImon
Hi Alexey, On Fri, 7 Jul 2023 at 09:43, Alexey Romanov <AVRomanov@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@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@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? Also there is a pending series on FFA - is that related? It uses smc from what I can tell. Regards, Simon
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@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@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@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. 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 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. > > Regards, > Simon
Hi Alexey, On Mon, 10 Jul 2023 at 02:34, Alexey Romanov <AVRomanov@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@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@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@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. 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. 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
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@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@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@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@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
+AKASHI Takahiro Hi Alexey, On Tue, 11 Jul 2023 at 04:25, Alexey Romanov <AVRomanov@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@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@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@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@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
On Tue, Jul 11, 2023 at 01:13:29PM -0600, Simon Glass wrote: > +AKASHI Takahiro Me? > Hi Alexey, > > On Tue, 11 Jul 2023 at 04:25, Alexey Romanov <AVRomanov@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@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@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@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@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? > 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() > > 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. (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
Hi, On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro <takahiro.akashi@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@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@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@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@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@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
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@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@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@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@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@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@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
Hi, On 16/07/2023 01:40, Simon Glass wrote: > Hi, > > On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro > <takahiro.akashi@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. The thread is long and hard to answer directly, but as AKASHI said there's no point to add a SMC class since it's only the message passing instruction, and there's no point using remoteproc since the firmware runs on a separate secure state of the same CPU. And I don't see how we can actually define a finite set of ops because none of the secure firmware interfaces has even similar functions. So a new UCLASS for each firmware interface should be added, not sure this is scalable or required since those firmwares are mainly SoC or vendor specific, except the PSCI or other ARM specific interfaces of course. So I think UCLASS_FIRMWARE is good enough since it avoids using UCLASS_MISC, but it should be probably documented somewhere that the ops are implementation defined. Neil > >> >>> Hi Alexey, >>> >>> On Tue, 11 Jul 2023 at 04:25, Alexey Romanov <AVRomanov@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@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@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@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@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
Hi Neil, On Mon, 21 Aug 2023 at 03:16, neil.armstrong@linaro.org <neil.armstrong@linaro.org> wrote: > > Hi, > > On 16/07/2023 01:40, Simon Glass wrote: > > Hi, > > > > On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro > > <takahiro.akashi@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. > > The thread is long and hard to answer directly, but as AKASHI > said there's no point to add a SMC class since it's only the message > passing instruction, and there's no point using remoteproc since the > firmware runs on a separate secure state of the same CPU. > > And I don't see how we can actually define a finite set of ops because > none of the secure firmware interfaces has even similar functions. > > So a new UCLASS for each firmware interface should be added, not sure > this is scalable or required since those firmwares are mainly SoC or > vendor specific, except the PSCI or other ARM specific interfaces of course. > > So I think UCLASS_FIRMWARE is good enough since it avoids using UCLASS_MISC, > but it should be probably documented somewhere that the ops are implementation > defined. Yes it needs docs...but what exactly is the 'firmware' uclass? I assumed it was for loading firmware into a device, but it seems that it is something else? Perhaps we should have a UCLASS_SVC (supervisor call) or something like that, rather than continuing with firmware? [..] Regards, Simon
On 21/08/2023 21:11, Simon Glass wrote: > Hi Neil, > > On Mon, 21 Aug 2023 at 03:16, neil.armstrong@linaro.org > <neil.armstrong@linaro.org> wrote: >> >> Hi, >> >> On 16/07/2023 01:40, Simon Glass wrote: >>> Hi, >>> >>> On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro >>> <takahiro.akashi@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. >> >> The thread is long and hard to answer directly, but as AKASHI >> said there's no point to add a SMC class since it's only the message >> passing instruction, and there's no point using remoteproc since the >> firmware runs on a separate secure state of the same CPU. >> >> And I don't see how we can actually define a finite set of ops because >> none of the secure firmware interfaces has even similar functions. >> >> So a new UCLASS for each firmware interface should be added, not sure >> this is scalable or required since those firmwares are mainly SoC or >> vendor specific, except the PSCI or other ARM specific interfaces of course. >> >> So I think UCLASS_FIRMWARE is good enough since it avoids using UCLASS_MISC, >> but it should be probably documented somewhere that the ops are implementation >> defined. > > Yes it needs docs...but what exactly is the 'firmware' uclass? I > assumed it was for loading firmware into a device, but it seems that > it is something else? Nop, it's based on the same "firmware" naming as Linux, which is an interface with a system control firmware like PSCI, SCPI... not to interact with loadable co-processors. Systems do have multiple interfaces implemented like PSCI, SCPI, OPTEE and other vendor specific ones like Alexey is changing, all via the same instruction call. > > Perhaps we should have a UCLASS_SVC (supervisor call) or something > like that, rather than continuing with firmware? I have no opinion on that, I don't think the call type is significant here. Neil > > [..] > > Regards, > Simon
Hi, On Tue, Aug 22, 2023 at 10:24:23AM +0200, neil.armstrong@linaro.org wrote: > On 21/08/2023 21:11, Simon Glass wrote: > > Hi Neil, > > > > On Mon, 21 Aug 2023 at 03:16, neil.armstrong@linaro.org > > <neil.armstrong@linaro.org> wrote: > > > > > > Hi, > > > > > > On 16/07/2023 01:40, Simon Glass wrote: > > > > Hi, > > > > > > > > On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro > > > > <takahiro.akashi@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. > > > > > > The thread is long and hard to answer directly, but as AKASHI > > > said there's no point to add a SMC class since it's only the message > > > passing instruction, and there's no point using remoteproc since the > > > firmware runs on a separate secure state of the same CPU. > > > > > > And I don't see how we can actually define a finite set of ops because > > > none of the secure firmware interfaces has even similar functions. > > > > > > So a new UCLASS for each firmware interface should be added, not sure > > > this is scalable or required since those firmwares are mainly SoC or > > > vendor specific, except the PSCI or other ARM specific interfaces of course. > > > > > > So I think UCLASS_FIRMWARE is good enough since it avoids using UCLASS_MISC, > > > but it should be probably documented somewhere that the ops are implementation > > > defined. > > > > Yes it needs docs...but what exactly is the 'firmware' uclass? I > > assumed it was for loading firmware into a device, but it seems that > > it is something else? > > Nop, it's based on the same "firmware" naming as Linux, which is an interface > with a system control firmware like PSCI, SCPI... not to interact with loadable > co-processors. > > Systems do have multiple interfaces implemented like PSCI, SCPI, OPTEE and other > vendor specific ones like Alexey is changing, all via the same instruction call. > > > > > Perhaps we should have a UCLASS_SVC (supervisor call) or something > > like that, rather than continuing with firmware? You propose to create UCLASS with an interface consisting of functions of something like: ->smc_call(), ->hvc_call()? In this case, it seems ARM specific. Or UCLASS with only one callback, different for different archs, which will call the hypervisor or something like that. In my understanding, this add-on are redundant. I still think UCLASS firmware is the best fit for my Secure Monitor implementation at the moment. > > I have no opinion on that, I don't think the call type is significant here. > > Neil > > > > > [..] > > > > Regards, > > Simon >
Hi Alexey, On Tue, 22 Aug 2023 at 06:59, Alexey Romanov <avromanov@salutedevices.com> wrote: > > Hi, > > On Tue, Aug 22, 2023 at 10:24:23AM +0200, neil.armstrong@linaro.org wrote: > > On 21/08/2023 21:11, Simon Glass wrote: > > > Hi Neil, > > > > > > On Mon, 21 Aug 2023 at 03:16, neil.armstrong@linaro.org > > > <neil.armstrong@linaro.org> wrote: > > > > > > > > Hi, > > > > > > > > On 16/07/2023 01:40, Simon Glass wrote: > > > > > Hi, > > > > > > > > > > On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro > > > > > <takahiro.akashi@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. > > > > > > > > The thread is long and hard to answer directly, but as AKASHI > > > > said there's no point to add a SMC class since it's only the message > > > > passing instruction, and there's no point using remoteproc since the > > > > firmware runs on a separate secure state of the same CPU. > > > > > > > > And I don't see how we can actually define a finite set of ops because > > > > none of the secure firmware interfaces has even similar functions. > > > > > > > > So a new UCLASS for each firmware interface should be added, not sure > > > > this is scalable or required since those firmwares are mainly SoC or > > > > vendor specific, except the PSCI or other ARM specific interfaces of course. > > > > > > > > So I think UCLASS_FIRMWARE is good enough since it avoids using UCLASS_MISC, > > > > but it should be probably documented somewhere that the ops are implementation > > > > defined. > > > > > > Yes it needs docs...but what exactly is the 'firmware' uclass? I > > > assumed it was for loading firmware into a device, but it seems that > > > it is something else? > > > > Nop, it's based on the same "firmware" naming as Linux, which is an interface > > with a system control firmware like PSCI, SCPI... not to interact with loadable > > co-processors. > > > > Systems do have multiple interfaces implemented like PSCI, SCPI, OPTEE and other > > vendor specific ones like Alexey is changing, all via the same instruction call. > > > > > > > > Perhaps we should have a UCLASS_SVC (supervisor call) or something > > > like that, rather than continuing with firmware? > > You propose to create UCLASS with an interface consisting of functions > of something like: ->smc_call(), ->hvc_call()? In this case, it seems > ARM specific. Yes, but we have a lot of arch-specific interfaces. > > Or UCLASS with only one callback, different for different archs, which > will call the hypervisor or something like that. In my understanding, > this add-on are redundant. OK. > > I still think UCLASS firmware is the best fit for my Secure Monitor > implementation at the moment. How about you create a UCLASS_MESON_SM then? I don't really like the idea of a uclass with no standard API. One goal is to help people understand things and can't see that helping. > > > > > I have no opinion on that, I don't think the call type is significant here. > > > > Neil > > > > > > > > [..] > > > > > > Regards, > > > Simon > > > > -- > Thank you, > Alexey Regards, Simon
Hi Simon, On Tue, Aug 22, 2023 at 12:56:32PM -0600, Simon Glass wrote: > Hi Alexey, > > On Tue, 22 Aug 2023 at 06:59, Alexey Romanov > <avromanov@salutedevices.com> wrote: > > > > Hi, > > > > On Tue, Aug 22, 2023 at 10:24:23AM +0200, neil.armstrong@linaro.org wrote: > > > On 21/08/2023 21:11, Simon Glass wrote: > > > > Hi Neil, > > > > > > > > On Mon, 21 Aug 2023 at 03:16, neil.armstrong@linaro.org > > > > <neil.armstrong@linaro.org> wrote: > > > > > > > > > > Hi, > > > > > > > > > > On 16/07/2023 01:40, Simon Glass wrote: > > > > > > Hi, > > > > > > > > > > > > On Thu, 13 Jul 2023 at 23:30, AKASHI Takahiro > > > > > > <takahiro.akashi@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. > > > > > > > > > > The thread is long and hard to answer directly, but as AKASHI > > > > > said there's no point to add a SMC class since it's only the message > > > > > passing instruction, and there's no point using remoteproc since the > > > > > firmware runs on a separate secure state of the same CPU. > > > > > > > > > > And I don't see how we can actually define a finite set of ops because > > > > > none of the secure firmware interfaces has even similar functions. > > > > > > > > > > So a new UCLASS for each firmware interface should be added, not sure > > > > > this is scalable or required since those firmwares are mainly SoC or > > > > > vendor specific, except the PSCI or other ARM specific interfaces of course. > > > > > > > > > > So I think UCLASS_FIRMWARE is good enough since it avoids using UCLASS_MISC, > > > > > but it should be probably documented somewhere that the ops are implementation > > > > > defined. > > > > > > > > Yes it needs docs...but what exactly is the 'firmware' uclass? I > > > > assumed it was for loading firmware into a device, but it seems that > > > > it is something else? > > > > > > Nop, it's based on the same "firmware" naming as Linux, which is an interface > > > with a system control firmware like PSCI, SCPI... not to interact with loadable > > > co-processors. > > > > > > Systems do have multiple interfaces implemented like PSCI, SCPI, OPTEE and other > > > vendor specific ones like Alexey is changing, all via the same instruction call. > > > > > > > > > > > Perhaps we should have a UCLASS_SVC (supervisor call) or something > > > > like that, rather than continuing with firmware? > > > > You propose to create UCLASS with an interface consisting of functions > > of something like: ->smc_call(), ->hvc_call()? In this case, it seems > > ARM specific. > > Yes, but we have a lot of arch-specific interfaces. > > > > > Or UCLASS with only one callback, different for different archs, which > > will call the hypervisor or something like that. In my understanding, > > this add-on are redundant. > > OK. > > > > > I still think UCLASS firmware is the best fit for my Secure Monitor > > implementation at the moment. > > How about you create a UCLASS_MESON_SM then? > > I don't really like the idea of a uclass with no standard API. One > goal is to help people understand things and can't see that helping. OK. Will do it in v2. > > > > > > > > > I have no opinion on that, I don't think the call type is significant here. > > > > > > Neil > > > > > > > > > > > [..] > > > > > > > > Regards, > > > > Simon > > > > > > > -- > > Thank you, > > Alexey > > Regards, > Simon
diff --git a/arch/arm/mach-meson/Kconfig b/arch/arm/mach-meson/Kconfig index 669ca09a00a..b8746d27f63 100644 --- a/arch/arm/mach-meson/Kconfig +++ b/arch/arm/mach-meson/Kconfig @@ -11,6 +11,7 @@ config MESON64_COMMON select PWRSEQ select MMC_PWRSEQ select BOARD_LATE_INIT + select MESON_FIRMWARE imply CMD_DM config MESON_GX diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index eae1c8ddc9f..17b70fdea6d 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -37,6 +37,15 @@ config ZYNQMP_FIRMWARE Say yes to enable ZynqMP firmware interface driver. If in doubt, say N. +config MESON_FIRMWARE + bool "Meson Firmware interface" + select FIRMWARE + help + This option enables Meson firmware interface, + which is used by different drivers to communicate + with the firmware for various platform management + services. + config ARM_SMCCC_FEATURES bool "Arm SMCCC features discovery" depends on ARM_PSCI_FW @@ -45,4 +54,5 @@ config ARM_SMCCC_FEATURES the PSCI driver is always probed and binds dirvers registered to the Arm SMCCC services if any and reported as supported by the SMCCC firmware. +source "drivers/firmware/meson/Kconfig" source "drivers/firmware/scmi/Kconfig" diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile index 7ce83d72bd3..a6300be27ad 100644 --- a/drivers/firmware/Makefile +++ b/drivers/firmware/Makefile @@ -4,3 +4,4 @@ obj-$(CONFIG_TI_SCI_PROTOCOL) += ti_sci.o obj-$(CONFIG_SANDBOX) += firmware-sandbox.o obj-$(CONFIG_ZYNQMP_FIRMWARE) += firmware-zynqmp.o obj-$(CONFIG_SCMI_FIRMWARE) += scmi/ +obj-$(CONFIG_MESON_FIRMWARE) += meson/ diff --git a/drivers/firmware/meson/Kconfig b/drivers/firmware/meson/Kconfig new file mode 100644 index 00000000000..0fd4f3251e1 --- /dev/null +++ b/drivers/firmware/meson/Kconfig @@ -0,0 +1,6 @@ +config MESON_SM + bool "Amlogic Secure Monitor driver" + depends on ARCH_MESON + default y + help + Say y here to enable the Amlogic secure monitor driver. diff --git a/drivers/firmware/meson/Makefile b/drivers/firmware/meson/Makefile new file mode 100644 index 00000000000..b5d26f150b0 --- /dev/null +++ b/drivers/firmware/meson/Makefile @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0-only + +obj-$(CONFIG_MESON_SM) += meson_sm.o diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c new file mode 100644 index 00000000000..28eacb89810 --- /dev/null +++ b/drivers/firmware/meson/meson_sm.c @@ -0,0 +1,217 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2023 SberDevices, Inc. + * + * Author: Alexey Romanov <avromanov@sberdevices.ru> + */ + +#include <dm.h> +#include <common.h> +#include <stdlib.h> +#include <regmap.h> +#include <syscon.h> +#include <asm/ptrace.h> +#include <asm/system.h> +#include <linux/err.h> +#include <linux/sizes.h> +#include <linux/bitfield.h> +#include <meson/sm_handle.h> + +struct meson_sm_cmd { + u32 smc_id; +}; + +#define SET_CMD(index, id) \ + [index] = { \ + .smc_id = id, \ + } + +struct meson_sm_data { + u32 cmd_get_shmem_in; + u32 cmd_get_shmem_out; + unsigned int shmem_size; + struct meson_sm_cmd cmd[]; +}; + +struct meson_sm_priv { + void *sm_shmem_in; + void *sm_shmem_out; + struct meson_sm_handle handle; + const struct meson_sm_data *data; +}; + +static unsigned long __meson_sm_call(u32 cmd, u32 arg0, u32 arg1, u32 arg2, + u32 arg3, u32 arg4) +{ + struct pt_regs r; + + r.regs[0] = cmd; + r.regs[1] = arg0; + r.regs[2] = arg1; + r.regs[3] = arg2; + r.regs[4] = arg3; + r.regs[5] = arg4; + + smc_call(&r); + + return r.regs[0]; +}; + +static u32 meson_sm_get_cmd(const struct meson_sm_data *data, + u32 cmd_index) +{ + struct meson_sm_cmd cmd; + + if (cmd_index >= MESON_SMC_CMD_COUNT) + return 0; + + cmd = data->cmd[cmd_index]; + return cmd.smc_id; +} + +static int meson_sm_call(struct udevice *dev, u32 cmd_index, u32 *retval, + u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4) +{ + struct meson_sm_priv *priv = dev_get_priv(dev); + u32 cmd, ret; + + cmd = meson_sm_get_cmd(priv->data, cmd_index); + if (!cmd) + return -ENOENT; + + ret = __meson_sm_call(cmd, arg0, arg1, arg2, arg3, arg4); + if (retval) + *retval = ret; + + return 0; +} + +static int meson_sm_call_read(struct udevice *dev, void *buffer, size_t size, + u32 cmd_index, u32 offset, u32 cnt) +{ + struct meson_sm_priv *priv = dev_get_priv(dev); + u32 nbytes; + int ret; + + if (!buffer || size > priv->data->shmem_size) + return -EINVAL; + + ret = meson_sm_call(dev, cmd_index, &nbytes, offset, cnt, 0, 0, 0); + if (ret) + return ret; + + if (nbytes > size) + return -ENOBUFS; + + /* In some cases (for example GET_CHIP_ID command), + * SMC doesn't return the number of bytes read, even + * though the bytes were actually read into sm_shmem_out. + * So this check is needed. + */ + ret = nbytes; + if (!nbytes) + nbytes = size; + + memcpy(buffer, priv->sm_shmem_out, nbytes); + + return ret; +} + +static int meson_sm_call_write(struct udevice *dev, void *buffer, size_t size, + u32 cmd_index, u32 offset, u32 cnt) +{ + struct meson_sm_priv *priv = dev_get_priv(dev); + u32 nbytes; + int ret; + + if (!buffer || size > priv->data->shmem_size) + return -EINVAL; + + memcpy(priv->sm_shmem_in, buffer, size); + + ret = meson_sm_call(dev, cmd_index, &nbytes, offset, cnt, 0, 0, 0); + if (ret) + return ret; + + if (!nbytes) + return -EIO; + + return nbytes; +} + +struct meson_sm_handle *meson_sm_get_handle(struct udevice *dev) +{ + struct meson_sm_priv *priv; + struct meson_sm_handle *handle; + + priv = dev_get_priv(dev); + if (!priv) + return ERR_PTR(-EINVAL); + + handle = &priv->handle; + + return handle; +} + +static const struct meson_sm_ops sm_ops = { + .sm_call = meson_sm_call, + .sm_call_read = meson_sm_call_read, + .sm_call_write = meson_sm_call_write, +}; + +static int meson_sm_probe(struct udevice *dev) +{ + struct meson_sm_priv *priv = dev_get_priv(dev); + + priv->handle.ops = sm_ops; + priv->data = (struct meson_sm_data *)dev_get_driver_data(dev); + if (!priv->data) + return -EINVAL; + + priv->sm_shmem_in = + (void *)__meson_sm_call(priv->data->cmd_get_shmem_in, 0, 0, 0, 0, 0); + + if (!priv->sm_shmem_in) + return -ENOMEM; + + priv->sm_shmem_out = + (void *)__meson_sm_call(priv->data->cmd_get_shmem_out, 0, 0, 0, 0, 0); + + if (!priv->sm_shmem_out) + return -ENOMEM; + + pr_debug("meson sm driver probed\n" + "shmem_in addr: 0x%p, shmem_out addr: 0x%p\n", + priv->sm_shmem_in, + priv->sm_shmem_out); + + return 0; +} + +static const struct meson_sm_data meson_sm_gxbb_data = { + .cmd_get_shmem_in = 0x82000020, + .cmd_get_shmem_out = 0x82000021, + .shmem_size = SZ_4K, + .cmd = { + SET_CMD(MESON_SMC_CMD_EFUSE_READ, 0x82000030), + SET_CMD(MESON_SMC_CMD_EFUSE_WRITE, 0x82000031), + SET_CMD(MESON_SMC_CMD_CHIP_ID_GET, 0x82000044), + SET_CMD(MESON_SMC_CMD_PWRDM_SET, 0x82000093), + }, +}; + +static const struct udevice_id meson_sm_ids[] = { + { + .compatible = "amlogic,meson-gxbb-sm", + .data = (ulong)&meson_sm_gxbb_data, + }, + { } +}; + +U_BOOT_DRIVER(meson_sm) = { + .name = "meson_sm", + .id = UCLASS_FIRMWARE, + .of_match = meson_sm_ids, + .probe = meson_sm_probe, + .priv_auto = sizeof(struct meson_sm_priv), +}; diff --git a/include/meson/sm_handle.h b/include/meson/sm_handle.h new file mode 100644 index 00000000000..a3c69b77bd6 --- /dev/null +++ b/include/meson/sm_handle.h @@ -0,0 +1,38 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Copyright (c) 2023 SberDevices, Inc. + * + * Author: Alexey Romanov <avromanov@sberdevices.ru> + */ + +#ifndef _MESON_SM_H_ +#define _MESON_SM_H_ + +#include <dm/device.h> + +enum meson_smc_cmd { + MESON_SMC_CMD_EFUSE_READ, + MESON_SMC_CMD_EFUSE_WRITE, + MESON_SMC_CMD_CHIP_ID_GET, + MESON_SMC_CMD_PWRDM_SET, + MESON_SMC_CMD_COUNT, +}; + +struct meson_sm_ops { + int (*sm_call)(struct udevice *dev, u32 cmd, u32 *ret, + u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4); + + int (*sm_call_read)(struct udevice *dev, void *buffer, + size_t size, u32 cmd, u32 offset, u32 cnt); + + int (*sm_call_write)(struct udevice *dev, void *buffer, + size_t size, u32 cmd, u32 offset, u32 cnt); +}; + +struct meson_sm_handle { + struct meson_sm_ops ops; +}; + +struct meson_sm_handle *meson_sm_get_handle(struct udevice *dev); + +#endif /* _MESON_SM_H_ */
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 doesn'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@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