mbox series

[V5,0/2] firmware: arm_scmi: add smc/hvc transports support

Message ID 1583673879-20714-1-git-send-email-peng.fan@nxp.com
Headers show
Series firmware: arm_scmi: add smc/hvc transports support | expand

Message

Peng Fan March 8, 2020, 1:24 p.m. UTC 
From: Peng Fan <peng.fan@nxp.com>

V5:
 Add Rob's R-b tag for patch 1
 Add mutex for patch 2

V4:
 Drop prot_id in scmi_chan_info, since not used for now.

V3:
 Add back arm,scmi-smc compatible string
 Change smc-id to arm,smc-id
 Directly use arm_smccc_1_1_invoke
 Add prot_id in scmi_chan_info for per protocol shmem usage.

V2:
 patch 1/2: only add smc-id property
 patch 2/2: Parse smc/hvc from psci node
	    Use prot_id as 2nd arg when issue smc/hvc
	    Differentiate tranports using mboxes or smc-id property
https://lore.kernel.org/patchwork/cover/1193435/

This is to add smc/hvc transports support, based on Viresh's v6.
SCMI firmware could be implemented in EL3, S-EL1, NS-EL2 or other
A core exception level. Then smc/hvc could be used. And for vendor
specific firmware, a wrapper layer could added in EL3, S-EL1,
NS-EL2 and etc to translate SCMI calls to vendor specific firmware calls.

A new compatible string arm,scmi-smc is added. arm,scmi is still for
mailbox transports.

Per smc/hvc, only Tx supported.

Peng Fan (2):
  dt-bindings: arm: arm,scmi: add smc/hvc transport
  firmware: arm_scmi: add smc/hvc transport

 Documentation/devicetree/bindings/arm/arm,scmi.txt |   3 +-
 drivers/firmware/arm_scmi/Makefile                 |   2 +-
 drivers/firmware/arm_scmi/common.h                 |   1 +
 drivers/firmware/arm_scmi/driver.c                 |   1 +
 drivers/firmware/arm_scmi/smc.c                    | 152 +++++++++++++++++++++
 5 files changed, 157 insertions(+), 2 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/smc.c

Comments

Peng Fan March 20, 2020, 8:27 a.m. UTC | #1 
Hi Sudeep,

> Subject: [PATCH V5 0/2] firmware: arm_scmi: add smc/hvc transports support

Are you fine with this patchset? Or You expect multi channel support?

We have some features depending on this for long time, so hope smc transports
could be finalized sooner.

Thanks,
Peng.

> 
> From: Peng Fan <peng.fan@nxp.com>
> 
> V5:
>  Add Rob's R-b tag for patch 1
>  Add mutex for patch 2
> 
> V4:
>  Drop prot_id in scmi_chan_info, since not used for now.
> 
> V3:
>  Add back arm,scmi-smc compatible string  Change smc-id to arm,smc-id
> Directly use arm_smccc_1_1_invoke  Add prot_id in scmi_chan_info for per
> protocol shmem usage.
> 
> V2:
>  patch 1/2: only add smc-id property
>  patch 2/2: Parse smc/hvc from psci node
> 	    Use prot_id as 2nd arg when issue smc/hvc
> 	    Differentiate tranports using mboxes or smc-id property
> https://lore.kernel.org/patchwork/cover/1193435/
> 
> This is to add smc/hvc transports support, based on Viresh's v6.
> SCMI firmware could be implemented in EL3, S-EL1, NS-EL2 or other A core
> exception level. Then smc/hvc could be used. And for vendor specific
> firmware, a wrapper layer could added in EL3, S-EL1,
> NS-EL2 and etc to translate SCMI calls to vendor specific firmware calls.
> 
> A new compatible string arm,scmi-smc is added. arm,scmi is still for mailbox
> transports.
> 
> Per smc/hvc, only Tx supported.
> 
> Peng Fan (2):
>   dt-bindings: arm: arm,scmi: add smc/hvc transport
>   firmware: arm_scmi: add smc/hvc transport
> 
>  Documentation/devicetree/bindings/arm/arm,scmi.txt |   3 +-
>  drivers/firmware/arm_scmi/Makefile                 |   2 +-
>  drivers/firmware/arm_scmi/common.h                 |   1 +
>  drivers/firmware/arm_scmi/driver.c                 |   1 +
>  drivers/firmware/arm_scmi/smc.c                    | 152
> +++++++++++++++++++++
>  5 files changed, 157 insertions(+), 2 deletions(-)  create mode 100644
> drivers/firmware/arm_scmi/smc.c
> 
> --
> 2.16.4
Sudeep Holla March 20, 2020, 8:37 a.m. UTC | #2 
On Fri, Mar 20, 2020 at 08:27:47AM +0000, Peng Fan wrote:
> Hi Sudeep,
>
> > Subject: [PATCH V5 0/2] firmware: arm_scmi: add smc/hvc transports support
>
> Are you fine with this patchset? Or You expect multi channel support?
>
> We have some features depending on this for long time, so hope smc transports
> could be finalized sooner.
>

I have applied these patches as is, yet to push it out. I am waiting for
my v5.7 scmi PR to reach arm-soc next before I update my scmi branch. I will
let you know replying to this thread once that happens.

--
Regards,
Sudeep
Peng Fan March 20, 2020, 8:40 a.m. UTC | #3 
> Subject: Re: [PATCH V5 0/2] firmware: arm_scmi: add smc/hvc transports
> support
> 
> On Fri, Mar 20, 2020 at 08:27:47AM +0000, Peng Fan wrote:
> > Hi Sudeep,
> >
> > > Subject: [PATCH V5 0/2] firmware: arm_scmi: add smc/hvc transports
> > > support
> >
> > Are you fine with this patchset? Or You expect multi channel support?
> >
> > We have some features depending on this for long time, so hope smc
> > transports could be finalized sooner.
> >
> 
> I have applied these patches as is, yet to push it out. I am waiting for my v5.7
> scmi PR to reach arm-soc next before I update my scmi branch. I will let you
> know replying to this thread once that happens.

Appreciate.

Thanks,
Peng.

> 
> --
> Regards,
> Sudeep
Sudeep Holla March 27, 2020, 4:32 p.m. UTC | #4 
On Fri, Mar 20, 2020 at 08:27:47AM +0000, Peng Fan wrote:
> Hi Sudeep,
>
> > Subject: [PATCH V5 0/2] firmware: arm_scmi: add smc/hvc transports support
>
> Are you fine with this patchset? Or You expect multi channel support?
>

I applied these patches[1]. I also looked at multi channel support and
I think it should be simple. I have made changes and will post soon.
I would like you to review and if possible test. I don't want to break
the existing single channel, so please do test in your setup for single
channel itself.

--
Regards,
Sudeep

[1] git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git for-next/scmi
Etienne Carriere April 15, 2020, 10:58 a.m. UTC | #5 
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.

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().

"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.



Regards,
Etienne


> From: Peng Fan <peng.fan@nxp.commm>
> 
> Take arm,smc-id as the 1st arg, leave the other args as zero for now.
> There is no Rx, only Tx because of smc/hvc not support Rx.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>
> (...)
>
> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> new file mode 100644
> index 000000000000..336168e40f49
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Message SMC/HVC
> + * Transport driver
> + *
> + * Copyright 2020 NXP
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +
> +#include "common.h"
> +
> +/**
> + * struct scmi_smc - Structure representing a SCMI smc transport
> + *
> + * @cinfo: SCMI channel info
> + * @shmem: Transmit/Receive shared memory area
> + * @func_id: smc/hvc call function id
> + */
> +
> +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;

With function proto type defined:

  typedef void (scmi_arm_smccc_invoke_fn)(unsigned long, struct arm_smccc_res *);

And materials to set the invocation hanlder:

/* Simple wrapper functions to be able to use a function pointer */
static void _smccc_smc(unsigned long func_id, struct arm_smccc_res *res)
{
	arm_smccc_smc(func_id, 0, 0, 0, 0, 0, 0, 0, res);
}

static void _smccc_hvc(unsigned long func_id, struct arm_smccc_res *res)
{
        arm_smccc_hvc(func_id, 0, 0, 0, 0, 0, 0, 0, res);
}

static void _smccc_1_1(unsigned long func_id, struct arm_smccc_res *res)
{
	arm_smccc_1_1_invoke(func_id, 0, 0, 0, 0, 0, 0, 0, res);
}

static scmi_arm_smccc_invoke_fn *get_invoke_function(struct device *dev)
{
        const char *method;

        if (device_property_read_string(dev, "method", &method))
		return _smccc_1_1;

        if (!strcmp("hvc", method))
                return _smccc_hvc;

        if (!strcmp("smc", method))
                return _smccc_smc;

        dev_err(dev, "Invalid \"method\" property: %s\n", method);
        return ERR_PTR(-EINVAL);
}
 
> +
> +static DEFINE_MUTEX(smc_mutex);
> +
> +static bool smc_chan_available(struct device *dev, int idx)
> +{
> +	return true;
> +}
> +
> +static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> +			  bool tx)
> +{
> +	struct device *cdev = cinfo->dev;
> +	struct scmi_smc *scmi_info;
> +	resource_size_t size;
> +	struct resource res;
> +	struct device_node *np;
> +	u32 func_id;
> +	int ret;
> +
> +	if (!tx)
> +		return -ENODEV;
> +
> +	scmi_info = devm_kzalloc(dev, sizeof(*scmi_info), GFP_KERNEL);
> +	if (!scmi_info)
> +		return -ENOMEM;
> +
> +	np = of_parse_phandle(cdev->of_node, "shmem", 0> );
> +	if (!np)
> +		np = of_parse_phandle(dev->of_node, "shmem", 0);
> +	ret = of_address_to_resource(np, 0, &res);
> +	of_node_put(np);
> +	if (ret) {
> +		dev_err(cdev, "failed to get SCMI Tx shared memory\n");
> +		return ret;
> +	}
> +
> +	size = resource_size(&res);
> +	scmi_info->shmem = devm_ioremap(dev, res.start, size);
> +	if (!scmi_info->shmem) {
> +		dev_err(dev, "failed to ioremap SCMI Tx shared memory\n");
> +		return -EADDRNOTAVAIL;
> +	}
> +
> +	ret = of_property_read_u32(dev->of_node, "arm,smc-id", &func_id);
> +	if (ret < 0)
> +		return ret;

Here to get the handler for the invocation method:

	scmi_info->invoke_fn = get_invoke_function(dev);
	if (IS_ERR(scmi_info->invoke_fn))
		return PTR_ERR(scmi_info->invoke_fn);
 
> +
> +	scmi_info->func_id = func_id;
> +	scmi_info->cinfo = cinfo;
> +	cinfo->transport_info = scmi_info;
> +
> +	return 0;
> +}
> +
> +static int smc_chan_free(int id, void *p, void *data)
> +{
> +	struct scmi_chan_info *cinfo = p;
> +	struct scmi_smc *scmi_info = cinfo->transport_info;
> +
> +	cinfo->transport_info = NULL;
> +	scmi_info->cinfo = NULL;
> +
> +	scmi_free_channel(cinfo, data, id);
> +
> +	return 0;
> +}
> +
> +static int smc_send_message(struct scmi_chan_info *cinfo,
> +			    struct scmi_xfer *xfer)
> +{
> +	struct scmi_smc *scmi_info = cinfo->transport_info;
> +	struct arm_smccc_res res;
> +
> +	mutex_lock(&smc_mutex);
> +
> +	shmem_tx_prepare(scmi_info->shmem, xfer);
> +
> +	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);

Last, here would rahter call the registered handler instead:

	scmi_info->invoke_fn(scmi_info->func_id, &res);


> +	scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
> +
> +	mutex_unlock(&smc_mutex);
> +
> +	return res.a0;


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;


Regards,
Etienne

> +}
> +
> +static void smc_mark_txdone(struct scmi_chan_info *cinfo, int ret)
> +{
> +}
> +
> +static void smc_fetch_response(struct scmi_chan_info *cinfo,
> +			       struct scmi_xfer *xfer)
> +{
> +	struct scmi_smc *scmi_info = cinfo->transport_info;
> +
> +	shmem_fetch_response(scmi_info->shmem, xfer);
> +}
> +
> +static bool
> +smc_poll_done(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer)
> +{
> +	struct scmi_smc *scmi_info = cinfo->transport_info;
> +
> +	return shmem_poll_done(scmi_info->shmem, xfer);
> +}
> +
> +static struct scmi_transport_ops scmi_smc_ops = {
> +	.chan_available = smc_chan_available,
> +	.chan_setup = smc_chan_setup,
> +	.chan_free = smc_chan_free,
> +	.send_message = smc_send_message,
> +	.mark_txdone = smc_mark_txdone,
> +	.fetch_response = smc_fetch_response,
> +	.poll_done = smc_poll_done,
> +};
> +
> +const struct scmi_desc scmi_smc_desc = {
> +	.ops = &scmi_smc_ops,
> +	.max_rx_timeout_ms = 30,
> +	.max_msg = 1,
> +	.max_msg_size = 128,
> +};
> -- 
> 2.16.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Sudeep Holla April 15, 2020, 1:16 p.m. UTC | #6 
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
Etienne Carriere April 15, 2020, 2:15 p.m. UTC | #7 
Hello Sudeep,

On Wed, 15 Apr 2020 at 15:16, Sudeep Holla <sudeep.holla@arm.com> 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