Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1694365 > unrolled thread

Re: [PATCH 1/8] mailbox: introduce ARM SMC based mailbox

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/8] mailbox: introduce ARM SMC based mailbox André Przywara <andre.przywara@arm.com> - 2017-07-24 01:30 +0200
    Re: [PATCH 1/8] mailbox: introduce ARM SMC based mailbox Jassi Brar <jassisinghbrar@gmail.com> - 2017-07-24 19:30 +0200
      Re: [PATCH 1/8] mailbox: introduce ARM SMC based mailbox Sudeep Holla <sudeep.holla@arm.com> - 2017-07-24 19:50 +0200
        Re: [PATCH 1/8] mailbox: introduce ARM SMC based mailbox Jassi Brar <jassisinghbrar@gmail.com> - 2017-07-24 20:10 +0200

#1694365 — 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@arm.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@arm.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@arm.com>");
>> +MODULE_DESCRIPTION("Generic ARM smc mailbox driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.9.0
>>

[toc] | [next] | [standalone]

#1694914

On Mon, Jul 24, 2017 at 4:50 AM, André Przywara <andre.przywara@arm.com> wrote:
> On 02/07/17 06:55, Jassi Brar wrote:
>
>>> +       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 meant instead of calling mbox_chan_received_data(), simply update
the value at 'data' with res.a0

Technically the firmware does not "send" us a message. It only updates
the structure we share with it. So maybe we could reflect that by
updating the data pointer the client driver asked to send.
Also it is optional for clients to provide the rx_callback(). By
calling mbox_chan_received_data() you mandate clients provide that
callback.

Nothing serious, just that looking closely, updating 'data' seems a
better option.

> 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.
>
I am afraid, SCPI driver is not the golden example for client drivers
to follow. It is supposed to work only with MHU, and then, it is
likely to break if some other protocol is running parallel to it.

Thanks

[toc] | [prev] | [next] | [standalone]

#1694946

On 24/07/17 18:20, Jassi Brar wrote:
> On Mon, Jul 24, 2017 at 4:50 AM, André Przywara <andre.przywara@arm.com> wrote:
>> On 02/07/17 06:55, Jassi Brar wrote:
>>
>>>> +       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 meant instead of calling mbox_chan_received_data(), simply update
> the value at 'data' with res.a0
> 
> Technically the firmware does not "send" us a message. It only updates
> the structure we share with it. So maybe we could reflect that by
> updating the data pointer the client driver asked to send.
> Also it is optional for clients to provide the rx_callback(). By
> calling mbox_chan_received_data() you mandate clients provide that
> callback.
> 
> Nothing serious, just that looking closely, updating 'data' seems a
> better option.
> 
>> 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.
>>
> I am afraid, SCPI driver is not the golden example for client drivers
> to follow. It is supposed to work only with MHU, and then, it is
> likely to break if some other protocol is running parallel to it.
> 

Not sure why do you say it works only with ARM MHU ? AmLogic uses it
with their mailbox driver. However they followed an interim version of
the SCPI spec which is termed "legacy" in the driver.
-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1694970

On Mon, Jul 24, 2017 at 11:08 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On 24/07/17 18:20, Jassi Brar wrote:

>>
>>> 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.
>>>
>> I am afraid, SCPI driver is not the golden example for client drivers
>> to follow. It is supposed to work only with MHU, and then, it is
>> likely to break if some other protocol is running parallel to it.
>>
>
> Not sure why do you say it works only with ARM MHU ? AmLogic uses it
> with their mailbox driver. However they followed an interim version of
> the SCPI spec which is termed "legacy" in the driver.
>
Screw coding... Just tell me what stuff do you smoke? Must be really good!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web