Hello Sudeep, On Wed, 15 Apr 2020 at 15:16, Sudeep Holla <sudeep.holla@xxxxxxx> wrote: > > On Wed, Apr 15, 2020 at 12:58:58PM +0200, Etienne Carriere wrote: > > Hello Peng, > > > > I have 2 comments on this change. The main is about using > > arm_smccc_1_1_invoke(). Below some details and I added comments > > inside you patch. The second of on SMC return value, see my > > comment in your patch below. > > > > About arm_smccc_1_1_invoke(), this functon currently relies on PSCI > > driver to define a conduit method but SCMI agent driver does not > > mandate CONFIG_PSCI to be enable. > > > > Yes this was discussed and it is done so deliberately. I have added the > build dependency when I merged the patch. There's no dependency on > CONFIG_PSCI. Ok, I understand your point. Yet it seems to me there is still a dependency on CONFIG_ARM_PSCI_FW and also a dependency on a PSCI node in the DT. However, I must admit I don't know yet a platform that enables CONFIG_ARM_SCMI_PROTOCOL but not CONFIG_ARM_PSCI_FW, hence so far, so good. About dependencies, it think the dependency on MAILBOX in firmware/Kconfig should be updated: config ARM_SCMI_PROTOCOL bool "ARM System Control and Management Interface (SCMI) Message Protocol" depends on ARM || ARM64 || COMPILE_TEST - depends on MAILBOX + depends on MAILBOX | HAVE_ARM_SMCCC help > > > Could you add an optional "method" property for "arm,scmi-smc" for platforms > > willing to not rely on PSCI Linux driver? If no property "method" is > > defined in the FDT, invocation relies on arm_smccc_1_1_invoke(). > > > > Nope, we don't want mixture here. Why is the system not using PSCI/SMCCC ? Ok, as I staed above, I don't know any platform that enables CONFIG_ARM_SCMI_PROTOCOL but not CONFIG_ARM_PSCI_FW. > > > "method" naming mimics what is done in the OP-TEE driver (drivers/tee/optee/). > > Here is a proposal for the documenting property "method" in > > Documentation/arm,scmi.txt: > > > > - method : "smc" or "hvc" > > Optional property defining the conduit method for to be used > > for invoking the SCMI server in secure world. > > "smc" states instruction SMC #0 is used whereas "hvc" states > > instruction HVC #0 is used. > > > > > > It was rejected, you can try your luck with OPTEE :) > We will just use the system conduit here with SCMI for SMC/HVC transport. > Details in previous version of the patch. > > [...] > > > > +struct scmi_smc { > > > + struct scmi_chan_info *cinfo; > > > + struct scmi_shared_mem __iomem *shmem; > > > + u32 func_id; > > > +}; > > > > Add here a field for the secure world invocation function handler: > > > > scmi_arm_smccc_invoke_fn *invoke_fn; > > > > As stated not needed if we use arm_smccc_1_1_invoke() > > [...] > Regards, Etienne