Re: [PATCH V5 2/2] firmware: arm_scmi: add smc/hvc transport

[Sudeep Holla]

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