Message-ID: <22d23c1d-ea8d-9829-4624-6305cd83ad4a@arm.com>
Date:   Mon, 24 Jul 2017 00:20:08 +0100
From:   André Przywara <andre.przywara@....com>
To:     Jassi Brar <jassisinghbrar@...il.com>
Cc:     Sudeep Holla <sudeep.holla@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-sunxi@...glegroups.com,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        Chen-Yu Tsai <wens@...e.org>, Icenowy Zheng <icenowy@...c.xyz>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Devicetree List <devicetree@...r.kernel.org>
Subject: Re: [PATCH 1/8] mailbox: introduce ARM SMC based mailbox

On 02/07/17 06:55, Jassi Brar wrote:

Hi Jassi,

thank you very much for having a look!

> On Fri, Jun 30, 2017 at 3:26 PM, Andre Przywara <andre.przywara@....com> wrote:
>> This mailbox driver implements a mailbox which signals transmitted data
>> via an ARM smc (secure monitor call) instruction. The mailbox receiver
>> is implemented in firmware and can synchronously return data when it
>> returns execution to the non-secure world again.
>> An asynchronous receive path is not implemented.
>> This allows the usage of a mailbox to trigger firmware actions on SoCs
>> which either don't have a separate management processor or on which such
>> a core is not available. A user of this mailbox could be the SCP
>> interface.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@....com>
>> ---
>>  drivers/mailbox/Kconfig           |   8 ++
>>  drivers/mailbox/Makefile          |   2 +
>>  drivers/mailbox/arm-smc-mailbox.c | 172 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 182 insertions(+)
>>  create mode 100644 drivers/mailbox/arm-smc-mailbox.c
>>
>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>> index c5731e5..5664b7f 100644
>> --- a/drivers/mailbox/Kconfig
>> +++ b/drivers/mailbox/Kconfig
>> @@ -170,4 +170,12 @@ config BCM_FLEXRM_MBOX
>>           Mailbox implementation of the Broadcom FlexRM ring manager,
>>           which provides access to various offload engines on Broadcom
>>           SoCs. Say Y here if you want to use the Broadcom FlexRM.
>> +
>> +config ARM_SMC_MBOX
>> +       tristate "Generic ARM smc mailbox"
>> +       depends on OF && HAVE_ARM_SMCCC
>> +       help
>> +         Generic mailbox driver which uses ARM smc calls to call into
>> +         firmware for triggering mailboxes.
>> +
>>  endif
>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>> index d54e412..8ec6869 100644
>> --- a/drivers/mailbox/Makefile
>> +++ b/drivers/mailbox/Makefile
>> @@ -35,3 +35,5 @@ obj-$(CONFIG_BCM_FLEXRM_MBOX) += bcm-flexrm-mailbox.o
>>  obj-$(CONFIG_QCOM_APCS_IPC)    += qcom-apcs-ipc-mailbox.o
>>
>>  obj-$(CONFIG_TEGRA_HSP_MBOX)   += tegra-hsp.o
>> +
>> +obj-$(CONFIG_ARM_SMC_MBOX)     += arm-smc-mailbox.o
>> diff --git a/drivers/mailbox/arm-smc-mailbox.c b/drivers/mailbox/arm-smc-mailbox.c
>> new file mode 100644
>> index 0000000..578aed2
>> --- /dev/null
>> +++ b/drivers/mailbox/arm-smc-mailbox.c
>> @@ -0,0 +1,172 @@
>> +/*
>> + *  Copyright (C) 2016,2017 ARM Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This device provides a mechanism for emulating a mailbox by using
>> + * smc calls, allowing a "mailbox" consumer to sit in firmware running
>> + * on the same core.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mailbox_controller.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/arm-smccc.h>
>> +
> Please have relook at what headers are really needed.

Good point, indeed many of them are not needed.

>> +#define ARM_SMC_MBOX_SMC               (0 << 0)
>> +#define ARM_SMC_MBOX_HVC               (1 << 0)
>> +#define ARM_SMC_MBOX_METHOD_MASK       (1 << 0)
>> +
> Maybe have only
>    #define ARM_SMC_MBOX_HVC    BIT(0)
> 
>> +struct arm_smc_chan_data {
>> +       u32 function_id;
>> +       u32 flags;
>> +};
>> +
>> +static int arm_smc_send_data(struct mbox_chan *link, void *data)
>> +{
>> +       struct arm_smc_chan_data *chan_data = link->con_priv;
>> +       u32 function_id = chan_data->function_id;
>> +       struct arm_smccc_res res;
>> +       u32 msg = *(u32 *)data;
>> +
>> +       if ((chan_data->flags & ARM_SMC_MBOX_METHOD_MASK) == ARM_SMC_MBOX_SMC)
>> +               arm_smccc_smc(function_id, msg, 0, 0, 0, 0, 0, 0, &res);
>> +       else
>> +               arm_smccc_hvc(function_id, msg, 0, 0, 0, 0, 0, 0, &res);
>> +
>        if (chan_data->flags & ARM_SMC_MBOX_HVC)
>                arm_smccc_hvc(function_id, msg, 0, 0, 0, 0, 0, 0, &res);
>        else
>                arm_smccc_smc(function_id, msg, 0, 0, 0, 0, 0, 0, &res);
> 
> is simpler.

Yeah, makes sense, I changed that.

>> +       mbox_chan_received_data(link, (void *)res.a0);
>> +
> Or you can update the 'data' with value from 'a0' ?

Mmh, I am a bit puzzled by this. Why would this be needed or useful? I
see that the SCPI firmware driver (as the user of the mailbox API) is
expecting the return value from a0 as returned above, translating the
firmware error codes into Linux' ones.
What am I missing here?

>> +       return 0;
>> +}
>> +
>> +static int arm_smc_startup(struct mbox_chan *link)
>> +{
>> +       return 0;
>> +}
>> +
>> +static void arm_smc_shutdown(struct mbox_chan *link)
>> +{
>> +}
>> +
> startup and shutdown can be omitted now.

Ah, thanks for the heads up, removed them.

>> +/* This mailbox is synchronous, so we are always done. */
>> +static bool arm_smc_last_tx_done(struct mbox_chan *link)
>> +{
>> +       return true;
>> +}
>> +
>> +static const struct mbox_chan_ops arm_smc_mbox_chan_ops = {
>> +       .send_data      = arm_smc_send_data,
>> +       .startup        = arm_smc_startup,
>> +       .shutdown       = arm_smc_shutdown,
>> +       .last_tx_done   = arm_smc_last_tx_done
>> +};
>> +
>> +static int arm_smc_mbox_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct mbox_controller *mbox;
>> +       struct arm_smc_chan_data *chan_data;
>> +       const char *method;
>> +       bool use_hvc = false;
>> +       int ret = 0, i;
>> +
> No need to initialise 'ret'

Indeed!

Thanks for the comments!

Cheers,
Andre.

>> +       ret = of_property_count_elems_of_size(dev->of_node, "arm,smc-func-ids",
>> +                                             sizeof(u32));
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       if (!of_property_read_string(dev->of_node, "method", &method)) {
>> +               if (!strcmp("hvc", method)) {
>> +                       use_hvc = true;
>> +               } else if (!strcmp("smc", method)) {
>> +                       use_hvc = false;
>> +               } else {
>> +                       dev_warn(dev, "invalid \"method\" property: %s\n",
>> +                                method);
>> +
>> +                       return -EINVAL;
>> +               }
>> +       }
>> +
>> +       mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
>> +       if (!mbox)
>> +               return -ENOMEM;
>> +
>> +       mbox->num_chans = ret;
>> +       mbox->chans = devm_kcalloc(dev, mbox->num_chans, sizeof(*mbox->chans),
>> +                                  GFP_KERNEL);
>> +       if (!mbox->chans)
>> +               return -ENOMEM;
>> +
>> +       chan_data = devm_kcalloc(dev, mbox->num_chans, sizeof(*chan_data),
>> +                                GFP_KERNEL);
>> +       if (!chan_data)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < mbox->num_chans; i++) {
>> +               u32 function_id;
>> +
>> +               ret = of_property_read_u32_index(dev->of_node,
>> +                                                "arm,smc-func-ids", i,
>> +                                                &function_id);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               chan_data[i].function_id = function_id;
>> +               if (use_hvc)
>> +                       chan_data[i].flags |= ARM_SMC_MBOX_HVC;
>> +               mbox->chans[i].con_priv = &chan_data[i];
>> +       }
>> +
>> +       mbox->txdone_poll = true;
>> +       mbox->txdone_irq = false;
>> +       mbox->txpoll_period = 1;
>> +       mbox->ops = &arm_smc_mbox_chan_ops;
>> +       mbox->dev = dev;
>> +
>> +       ret = mbox_controller_register(mbox);
>> +       if (ret)
>> +               return ret;
>> +
>> +       platform_set_drvdata(pdev, mbox);
>> +       dev_info(dev, "ARM SMC mailbox enabled with %d chan%s.\n",
>> +                mbox->num_chans, mbox->num_chans == 1 ? "" : "s");
>> +
>> +       return ret;
>> +}
>> +
>> +static int arm_smc_mbox_remove(struct platform_device *pdev)
>> +{
>> +       struct mbox_controller *mbox = platform_get_drvdata(pdev);
>> +
>> +       mbox_controller_unregister(mbox);
>> +       return 0;
>> +}
>> +
>> +static const struct of_device_id arm_smc_mbox_of_match[] = {
>> +       { .compatible = "arm,smc-mbox", },
>> +       {},
>> +};
>> +MODULE_DEVICE_TABLE(of, arm_smc_mbox_of_match);
>> +
>> +static struct platform_driver arm_smc_mbox_driver = {
>> +       .driver = {
>> +               .name = "arm-smc-mbox",
>> +               .of_match_table = arm_smc_mbox_of_match,
>> +       },
>> +       .probe          = arm_smc_mbox_probe,
>> +       .remove         = arm_smc_mbox_remove,
>> +};
>> +module_platform_driver(arm_smc_mbox_driver);
>> +
>> +MODULE_AUTHOR("Andre Przywara <andre.przywara@....com>");
>> +MODULE_DESCRIPTION("Generic ARM smc mailbox driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.9.0
>>

Powered by blists - more mailing lists