Re: [PATCH v2 1/4] firmware: add new driver for SCMI firmwares
Hi Etienne, On Sun, 23 Aug 2020 at 11:07, Etienne Carriere wrote: > > Hello Simon, > > > > This change introduces SCMI agent driver in U-Boot in the firmware > > > U-class. > > > (...) > > > Changes in v2: > > > (...) > > > > > > Note: review comments on defining a uclass and sandbox for SCMI > > > transport drivers are NOT addressed in this v2. Main issue is that > > > there is no driver/device defined for SCMI transport layer as well as > > > and no defined compatible ID in the SCMI DT bindings documentation. > > > > I'd still like to see this. You can define an API with a header file. > > It is certainly easier if the DT binding can cover the transport type > > with a separate subnode. > > The bindings are already defined for scmi (ref is the Linux kernel > source tree) and there is no sub-node currently defined for the > scmi driver transport configuration. It's done through the > compatible property and dedicated optional properties. > I think changing the scmi DT binding is a bit out of the scope > of my patch series :) Fair enough. The bindings are pretty linux-specific of course, since that is the only user. In general it seems hard to change them for U-Boot although I haven't tried in years. > > > But that doesn't stop you creating a uclass > > for the transport. It will also allow you to create a sandbox impl so > > you can add a test for this code. > > Ok, thanks, I understand. > > > > > Also the two interfaces should really be in separate files rather than > > using #ifdefs, I think. > > I'll send a v3 with the implementation over several source files and > the requested uclass/sandbox. > I think I'll create sub-directory drivers/firmware/scmi/ for the source files. I think that's a good idea. > > Thanks again for the feedback on this v2. You're welcome. Regards, Simon
Re: [PATCH v2 1/4] firmware: add new driver for SCMI firmwares
Hello Simon, > > This change introduces SCMI agent driver in U-Boot in the firmware > > U-class. > > (...) > > Changes in v2: > > (...) > > > > Note: review comments on defining a uclass and sandbox for SCMI > > transport drivers are NOT addressed in this v2. Main issue is that > > there is no driver/device defined for SCMI transport layer as well as > > and no defined compatible ID in the SCMI DT bindings documentation. > > I'd still like to see this. You can define an API with a header file. > It is certainly easier if the DT binding can cover the transport type > with a separate subnode. The bindings are already defined for scmi (ref is the Linux kernel source tree) and there is no sub-node currently defined for the scmi driver transport configuration. It's done through the compatible property and dedicated optional properties. I think changing the scmi DT binding is a bit out of the scope of my patch series :) > But that doesn't stop you creating a uclass > for the transport. It will also allow you to create a sandbox impl so > you can add a test for this code. Ok, thanks, I understand. > > Also the two interfaces should really be in separate files rather than > using #ifdefs, I think. I'll send a v3 with the implementation over several source files and the requested uclass/sandbox. I think I'll create sub-directory drivers/firmware/scmi/ for the source files. Thanks again for the feedback on this v2. Regards, Etienne > > > --- > > drivers/firmware/Kconfig | 20 ++ > > drivers/firmware/Makefile | 1 + > > drivers/firmware/scmi.c | 490 ++ > > include/scmi.h| 82 +++ > > 4 files changed, 593 insertions(+) > > create mode 100644 drivers/firmware/scmi.c > > create mode 100644 include/scmi.h > > > > Regards, > Simon
Re: [PATCH v2 1/4] firmware: add new driver for SCMI firmwares
Hi Etienne, On Tue, 18 Aug 2020 at 09:44, Etienne Carriere wrote: > > This change introduces SCMI agent driver in U-Boot in the firmware > U-class. > > SCMI agent driver is designed for platforms that embed a SCMI server in > a firmware hosted for example by a companion co-processor or the secure > world of the executing processor. > > SCMI protocols allow an SCMI agent to discover and access external > resources as clock, reset controllers and many more. SCMI agent and > server communicate following the SCMI specification [1]. SCMI agent > complies with the DT bindings defined in the Linux kernel source tree > regarding SCMI agent description since v5.8-rc1. > > These bindings describe 2 supported message transport layer: using > mailbox uclass devices or using Arm SMC invocation instruction. Both > use a piece or shared memory for message data exchange. > > In the current state, the SCMI agent driver does not bind to any SCMI > protocol to a U-Boot device driver. Former changes will implement > dedicated driver (i.e. an SCMI clock driver or an SCMI reset controller > driver) and add bind supported SCMI protocols in scmi_agent_bind(). > > Links: [1] > https://developer.arm.com/architectures/system-architectures/software-standards/scmi > Signed-off-by: Etienne Carriere > Cc: Simon Glass > Cc: Peng Fan > Cc: Sudeep Holla > --- > > Changes in v2: > - Fix CONFIG_SCMI_FIRMWARE description with explicit SCMI reference. > - Move struct, enum and macro definitions at source file top and > add inline comment description for the structures and local functions. > - Replace rc with ret as return value local variable label. > - Use explicit return 0 on successful return paths. > - Replace EINVAL with more accurate error numbers. > - Use dev_read_u32() instead of ofnode_read_u32(dev_ofnode(), ...). > - Use memcpy_toio()/memcpy_fromio() when copying message payload > to/from IO memory. > - Embed mailbox transport resources upon CONFIG_DM_MAILBOX and > SMCCC transport resources upon CONFIG_ARM_SMCCC. > > Note: review comments on defining a uclass and sandbox for SCMI > transport drivers are NOT addressed in this v2. Main issue is that > there is no driver/device defined for SCMI transport layer as well as > and no defined compatible ID in the SCMI DT bindings documentation. I'd still like to see this. You can define an API with a header file. It is certainly easier if the DT binding can cover the transport type with a separate subnode. But that doesn't stop you creating a uclass for the transport. It will also allow you to create a sandbox impl so you can add a test for this code. Also the two interfaces should really be in separate files rather than using #ifdefs, I think. > --- > drivers/firmware/Kconfig | 20 ++ > drivers/firmware/Makefile | 1 + > drivers/firmware/scmi.c | 490 ++ > include/scmi.h| 82 +++ > 4 files changed, 593 insertions(+) > create mode 100644 drivers/firmware/scmi.c > create mode 100644 include/scmi.h > Regards, Simon
Re: [PATCH v2 1/4] firmware: add new driver for SCMI firmwares
Hello Sudeep, On Tue, 18 Aug 2020 at 17:44, Etienne Carriere wrote: > > This change introduces SCMI agent driver in U-Boot in the firmware > U-class. > (...) > --- a/drivers/firmware/Kconfig > +++ b/drivers/firmware/Kconfig > @@ -1,6 +1,26 @@ > config FIRMWARE > bool "Enable Firmware driver support" > > +config SCMI_FIRMWARE > + bool "Enable SCMI support" > + select FIRMWARE > + select OF_TRANSLATE > + depends on DM_MAILBOX || ARM_SMCCC > + help > + System Control and Management Interface (SCMI) is a communication > + protocol that defines standard interfaces for power, performance > + and system management. The SCMI specification is available at > + > https://developer.arm.com/architectures/system-architectures/software-standards/scmi You suggested https://developer.arm.com/documentation/den0056/latest. It is shorter and points straight to the spec document whereas this link is used above points to more generic info about SCMI among which one can find the spec doc. Maybe I should change to the link you suggested. > + > + An SCMI agent communicates with a related SCMI server firmware > + located in another sub-system, as a companion micro controller > + or a companion host in the CPU system. > + > + Communications between agent (client) and the SCMI server are > + based on message exchange. Messages can be exchange over tranport Typo here: s/tranport/transport/ > + channels as a mailbox device or an Arm SMCCC service with some > + piece of identified shared memory. > + > config SPL_FIRMWARE > bool "Enable Firmware driver support in SPL" > depends on FIRMWARE
