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