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@xxxxxxx>
>
> (...)
>
> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
