Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox

From: Sudeep Holla
Date: Wed Jun 26 2019 - 13:03:04 EST

Next message: Joe Perches: "trivial script to update MAINTAINERS directory patterns"
Previous message: Catalin Marinas: "Re: [PATCH v3] arm64: rename dump_instr as dump_kernel_instr"
In reply to: Sudeep Holla: "Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Jun 26, 2019 at 01:31:15PM +0000, Peng Fan wrote:
>
> Hi All,
>
> > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> >
> > On Tue, Jun 25, 2019 at 2:30 AM Peng Fan <peng.fan@xxxxxxx> wrote:
> > >
> > > Hi Jassi
> > >
> > > > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > > >
> > > > On Mon, Jun 3, 2019 at 3:28 AM <peng.fan@xxxxxxx> wrote:
> > > > >
> > > > > From: Peng Fan <peng.fan@xxxxxxx>
> > > > >
> > > > > 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.
> > > > >
> > > > > Modified from Andre Przywara's v2 patch https://lore
> > > > > .kernel.org%2Fpatchwork%2Fpatch%2F812999%2F&data=02%7C0
> > 1%7
> > > > Cpeng.fa
> > > > >
> > > >
> > n%40nxp.com%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> > > > c6fa92cd
> > > > >
> > > >
> > 99c5c301635%7C0%7C0%7C636966462272457978&sdata=Hzgeu43m5
> > > > ZkeRMtL8Bx
> > > > > gUm3%2B6FBObib1OPHPlSccE%2B0%3D&reserved=0
> > > > >
> > > > > Cc: Andre Przywara <andre.przywara@xxxxxxx>
> > > > > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > > > > ---
> > > > >
> > > > > V2:
> > > > > Add interrupts notification support.
> > > > >
> > > > > drivers/mailbox/Kconfig | 7 ++
> > > > > drivers/mailbox/Makefile | 2 +
> > > > > drivers/mailbox/arm-smc-mailbox.c | 190
> > > > ++++++++++++++++++++++++++++++++
> > > > > include/linux/mailbox/arm-smc-mailbox.h | 10 ++
> > > > > 4 files changed, 209 insertions(+)
> > > > > create mode 100644 drivers/mailbox/arm-smc-mailbox.c create
> > mode
> > > > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > > > >
> > > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > > > 595542bfae85..c3bd0f1ddcd8 100644
> > > > > --- a/drivers/mailbox/Kconfig
> > > > > +++ b/drivers/mailbox/Kconfig
> > > > > @@ -15,6 +15,13 @@ config ARM_MHU
> > > > > The controller has 3 mailbox channels, the last of which can
> > be
> > > > > used in Secure mode only.
> > > > >
> > > > > +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.
> > > > > +
> > > > > config IMX_MBOX
> > > > > tristate "i.MX Mailbox"
> > > > > depends on ARCH_MXC || COMPILE_TEST diff --git
> > > > > a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> > > > > c22fad6f696b..93918a84c91b 100644
> > > > > --- a/drivers/mailbox/Makefile
> > > > > +++ b/drivers/mailbox/Makefile
> > > > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST) +=
> > mailbox-test.o
> > > > >
> > > > > obj-$(CONFIG_ARM_MHU) += arm_mhu.o
> > > > >
> > > > > +obj-$(CONFIG_ARM_SMC_MBOX) += arm-smc-mailbox.o
> > > > > +
> > > > > obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
> > > > >
> > > > > obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX) +=
> > > > armada-37xx-rwtm-mailbox.o
> > > > > diff --git a/drivers/mailbox/arm-smc-mailbox.c
> > > > > b/drivers/mailbox/arm-smc-mailbox.c
> > > > > new file mode 100644
> > > > > index 000000000000..fef6e38d8b98
> > > > > --- /dev/null
> > > > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > > > @@ -0,0 +1,190 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (C) 2016,2017 ARM Ltd.
> > > > > + * Copyright 2019 NXP
> > > > > + */
> > > > > +
> > > > > +#include <linux/arm-smccc.h>
> > > > > +#include <linux/device.h>
> > > > > +#include <linux/kernel.h>
> > > > > +#include <linux/interrupt.h>
> > > > > +#include <linux/mailbox_controller.h> #include
> > > > > +<linux/mailbox/arm-smc-mailbox.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +
> > > > > +#define ARM_SMC_MBOX_USE_HVC BIT(0)
> > > > > +#define ARM_SMC_MBOX_USB_IRQ BIT(1)
> > > > > +
> > > > IRQ bit is unused (and unnecessary IMO)
> > > >
> > > > > +struct arm_smc_chan_data {
> > > > > + u32 function_id;
> > > > > + u32 flags;
> > > > > + int irq;
> > > > > +};
> > > > > +
> > > > > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > > > > + struct arm_smc_chan_data *chan_data = link->con_priv;
> > > > > + struct arm_smccc_mbox_cmd *cmd = data;
> > > > > + struct arm_smccc_res res;
> > > > > + u32 function_id;
> > > > > +
> > > > > + if (chan_data->function_id != UINT_MAX)
> > > > > + function_id = chan_data->function_id;
> > > > > + else
> > > > > + function_id = cmd->a0;
> > > > > +
> > > > Not sure about chan_data->function_id. Why restrict from DT?
> > > > 'a0' is the function_id register, let the user pass func-id via the 'a0' like
> > other
> > > > values via 'a[1-7]'
> > >
> > > Missed to reply this comment.
> > >
> > > The firmware driver might not have func-id, such as SCMI/SCPI.
> > > So add an optional func-id to let smc mailbox driver could
> > > use smc SiP func id.
> > >
> > There is no end to conforming to protocols. Controller drivers should
> > be written having no particular client in mind.
>
> If the func-id needs be passed from user, then the chan_id suggested
> by Sudeep should also be passed from user, not in mailbox driver.
>

Why ? I understand SMC may have 1-1 mapping from DT to channel id, but
that may not be true for other mailbox controller. The client is provided
with mbox handle in DT and the mbox APIs are used to get the controller
handle. The client just uses the same and when it calls say send_data,
controller understands which channel the handle is mapped to and
client need not have remote idea on channel id. If by user you mean
DT yes but as described as it's indirect.

> Jassi, so from your point, arm_smc_send_data just send a0 - a6
> to firmware, right?
>
> Sudeep, Andre, Florian,
>
> What's your suggestion? SCMI not support, do you have
> plan to add smc transport in SCMI?
>

I *wish* we could abstract all the transport protocol behind the mailbox
APIs and SCMI just deals with message protocol as it is a message protocol
specification.

--
Regards,
Sudeep

Next message: Joe Perches: "trivial script to update MAINTAINERS directory patterns"
Previous message: Catalin Marinas: "Re: [PATCH v3] arm64: rename dump_instr as dump_kernel_instr"
In reply to: Sudeep Holla: "Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]