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.
> 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 ?
> "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()
[...]
>
> The SCMI server is likely not to return a errno compliant value.
>
> SMCCC specification states that unsupported function IDs should return signed
> extended -1. I suggest to change the return above with:
>
> return res.a0 == ~0 ? -EINVAL : 0;
>
I need to check that.
--
Regards,
Sudeep
