| Message ID | 20200919192235.43872-3-james.quinlan@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mailbox: Add Broadcom STB mailbox driver | expand |
Hi Jim, On 9/19/20 12:22 PM, Jim Quinlan wrote: > --- > drivers/mailbox/Kconfig | 12 +++ > drivers/mailbox/Makefile | 2 + > drivers/mailbox/brcmstb-mailbox.c | 173 ++++++++++++++++++++++++++++++ > 3 files changed, 187 insertions(+) > create mode 100644 drivers/mailbox/brcmstb-mailbox.c > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig > index 05b1009e2820..96faf1139cb9 100644 > --- a/drivers/mailbox/Kconfig > +++ b/drivers/mailbox/Kconfig > @@ -254,4 +254,16 @@ config QCOM_IPCC > acts as an interrupt controller for receiving interrupts from clients. > Say Y here if you want to build this driver. > > +config BRCMSTB_MBOX > + tristate "Broadcom STB Mailbox" > + depends on ARM64 || ARM > + depends on ARM_SCMI_PROTOCOL && ARCH_BRCMSTB > + default ARM_SCMI_PROTOCOL && ARCH_BRCMSTB > + help "help" is still indented with spaces instead of one tab. You probably should wait a bit to see if there are other comments on this patch before doing a v4. > + Mailbox implementation of the Broadcom STB for the sole purposes > + of SCMI communication. This is used by the SCMI drivers to > + communicate with FW that runs in EL3. This mailbox only implements > + the agent-to-platform channgel of SCMI but may be augmented in > + the future to add the platform-to-agent channel. > + > endif > diff --git a/drivers/mailbox/brcmstb-mailbox.c b/drivers/mailbox/brcmstb-mailbox.c > new file mode 100644 > index 000000000000..27d87938b08c > --- /dev/null > +++ b/drivers/mailbox/brcmstb-mailbox.c > @@ -0,0 +1,173 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2017, Broadcom */ 2017 is a little surprising. > +#if defined(CONFIG_ARM64) || defined(CONFIG_ARM) > +static int announce_msg(unsigned int mbox_num, unsigned int ch) > +{ > + struct arm_smccc_res res; > + > + if (ch >= NUM_CHAN) > + return -EIO; > + arm_smccc_smc(BRCM_FID(ch), mbox_num, 0, 0, 0, 0, 0, 0, &res); > + if (res.a0) > + return -EIO; > + return 0; > +} > +#else > +#error Func announce_msg() not defined for the current ARCH > +#endif This is handled by the Kconfig depends on clause. Why is it repeated here? thanks.
On Sat, Sep 19, 2020 at 3:32 PM Randy Dunlap <rdunlap@infradead.org> wrote: > > Hi Jim, > > On 9/19/20 12:22 PM, Jim Quinlan wrote: > > --- > > drivers/mailbox/Kconfig | 12 +++ > > drivers/mailbox/Makefile | 2 + > > drivers/mailbox/brcmstb-mailbox.c | 173 ++++++++++++++++++++++++++++++ > > 3 files changed, 187 insertions(+) > > create mode 100644 drivers/mailbox/brcmstb-mailbox.c > > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig > > index 05b1009e2820..96faf1139cb9 100644 > > --- a/drivers/mailbox/Kconfig > > +++ b/drivers/mailbox/Kconfig > > @@ -254,4 +254,16 @@ config QCOM_IPCC > > acts as an interrupt controller for receiving interrupts from clients. > > Say Y here if you want to build this driver. > > > > +config BRCMSTB_MBOX > > + tristate "Broadcom STB Mailbox" > > + depends on ARM64 || ARM > > + depends on ARM_SCMI_PROTOCOL && ARCH_BRCMSTB > > + default ARM_SCMI_PROTOCOL && ARCH_BRCMSTB > > + help > > "help" is still indented with spaces instead of one tab. > You probably should wait a bit to see if there are other comments on this patch > before doing a v4. Hi Randy, Will fix, then wait... > > > + Mailbox implementation of the Broadcom STB for the sole purposes > > + of SCMI communication. This is used by the SCMI drivers to > > + communicate with FW that runs in EL3. This mailbox only implements > > + the agent-to-platform channgel of SCMI but may be augmented in > > + the future to add the platform-to-agent channel. > > + > > endif > > diff --git a/drivers/mailbox/brcmstb-mailbox.c b/drivers/mailbox/brcmstb-mailbox.c > > new file mode 100644 > > index 000000000000..27d87938b08c > > --- /dev/null > > +++ b/drivers/mailbox/brcmstb-mailbox.c > > @@ -0,0 +1,173 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* Copyright (c) 2017, Broadcom */ > > 2017 is a little surprising. Will fix. > > > > +#if defined(CONFIG_ARM64) || defined(CONFIG_ARM) > > +static int announce_msg(unsigned int mbox_num, unsigned int ch) > > +{ > > + struct arm_smccc_res res; > > + > > + if (ch >= NUM_CHAN) > > + return -EIO; > > + arm_smccc_smc(BRCM_FID(ch), mbox_num, 0, 0, 0, 0, 0, 0, &res); > > + if (res.a0) > > + return -EIO; > > + return 0; > > +} > > +#else > > +#error Func announce_msg() not defined for the current ARCH > > +#endif > > This is handled by the Kconfig depends on clause. > Why is it repeated here? Will fix. Thanks, Jim > > > > thanks. > -- > ~Randy >
On Sat, Sep 19, 2020 at 03:22:30PM -0400, Jim Quinlan wrote: > only implements the agent-to-platform channel; In that case any reason why you can't reuse the existing smc transport for SCMI. It was added recently in case you haven't checked the latest kernel version(v5.8 or above). Check out for drivers/firmware/arm_scmi/smc.c IIUC rather vaguely Florian was cc-ed on those patches. > we may implement the platform-to-agent channel in the future. This is not yet support with that transport, it is hard to generalise as different vendors have their own solutions there. > An unusual aspect of this driver is how the completion of an SCMI message > is indicated. An SCMI message is initiated with an ARM SMC call, but the > return of this call does not indicate the execution or completion of the > message. Rather, the message's completion is signaled by an interrupt. > So are these not fast SMC/HVC calls then ? If so we may need some changes to that driver. I just rejected multiple message support as we had assumed fast smc/hvc.
On 10/9/2020 9:38 AM, Sudeep Holla wrote: > On Sat, Sep 19, 2020 at 03:22:30PM -0400, Jim Quinlan wrote: >> only implements the agent-to-platform channel; > > In that case any reason why you can't reuse the existing smc transport > for SCMI. It was added recently in case you haven't checked the latest > kernel version(v5.8 or above). Check out for drivers/firmware/arm_scmi/smc.c > IIUC rather vaguely Florian was cc-ed on those patches. Our firmware completes all commands by posting an interrupt and we already have platforms in the field with this mailbox driver. I was hoping we could make use of the SMC transport, but the current firmware implementation completes commands by raising an interrupt. The reason why we did that was that we could support synchronous and asynchronous calls the same way. > >> we may implement the platform-to-agent channel in the future. > > This is not yet support with that transport, it is hard to generalise > as different vendors have their own solutions there. > >> An unusual aspect of this driver is how the completion of an SCMI message >> is indicated. An SCMI message is initiated with an ARM SMC call, but the >> return of this call does not indicate the execution or completion of the >> message. Rather, the message's completion is signaled by an interrupt. >> > > So are these not fast SMC/HVC calls then ? If so we may need some changes > to that driver. I just rejected multiple message support as we had assumed > fast smc/hvc. >
On Fri, Oct 9, 2020 at 12:38 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Sat, Sep 19, 2020 at 03:22:30PM -0400, Jim Quinlan wrote: > > only implements the agent-to-platform channel; > > In that case any reason why you can't reuse the existing smc transport > for SCMI. It was added recently in case you haven't checked the latest > kernel version(v5.8 or above). Check out for drivers/firmware/arm_scmi/smc.c > IIUC rather vaguely Florian was cc-ed on those patches. Hi Sudeep, Sorry for the delay. As Florian mentioned, we tried to use what you've submitted but could not because in our system a return does not indicate the completion of the SCMI operation. It is indicated by an interrupt. There are a number of reasons for this and some are out of our control. > > > > we may implement the platform-to-agent channel in the future. > > This is not yet support with that transport, it is hard to generalise > as different vendors have their own solutions there. > > > An unusual aspect of this driver is how the completion of an SCMI message > > is indicated. An SCMI message is initiated with an ARM SMC call, but the > > return of this call does not indicate the execution or completion of the > > message. Rather, the message's completion is signaled by an interrupt. > > > > So are these not fast SMC/HVC calls then ? If so we may need some changes > to that driver. I just rejected multiple message support as we had assumed > fast smc/hvc. Yes, we are using fast SMC calls. We don't have multiple message support either. The disconnect we have with the smc/hvc transport commit si this: smc_send_message(...) { /* ... */ + arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res); + scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem)); In our code the second line is not here but in the interrupt handler. I don't see any way you can easily change/augment the smc/hvc transport to accommodate us. Regards, Jim > > > -- > Regards, > Sudeep
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index 05b1009e2820..96faf1139cb9 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -254,4 +254,16 @@ config QCOM_IPCC acts as an interrupt controller for receiving interrupts from clients. Say Y here if you want to build this driver. +config BRCMSTB_MBOX + tristate "Broadcom STB Mailbox" + depends on ARM64 || ARM + depends on ARM_SCMI_PROTOCOL && ARCH_BRCMSTB + default ARM_SCMI_PROTOCOL && ARCH_BRCMSTB + help + Mailbox implementation of the Broadcom STB for the sole purposes + of SCMI communication. This is used by the SCMI drivers to + communicate with FW that runs in EL3. This mailbox only implements + the agent-to-platform channgel of SCMI but may be augmented in + the future to add the platform-to-agent channel. + endif diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index 60d224b723a1..a74efd02eece 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -54,3 +54,5 @@ obj-$(CONFIG_SUN6I_MSGBOX) += sun6i-msgbox.o obj-$(CONFIG_SPRD_MBOX) += sprd-mailbox.o obj-$(CONFIG_QCOM_IPCC) += qcom-ipcc.o + +obj-$(CONFIG_BRCMSTB_MBOX) += brcmstb-mailbox.o diff --git a/drivers/mailbox/brcmstb-mailbox.c b/drivers/mailbox/brcmstb-mailbox.c new file mode 100644 index 000000000000..27d87938b08c --- /dev/null +++ b/drivers/mailbox/brcmstb-mailbox.c @@ -0,0 +1,173 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2017, Broadcom */ + +#include <linux/kernel.h> +#include <linux/interrupt.h> +#include <linux/mailbox_controller.h> +#include <linux/mailbox_client.h> +#include <linux/module.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_irq.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#if defined(CONFIG_ARM64) || defined(CONFIG_ARM) +#include <linux/arm-smccc.h> +#endif + +#define BRCM_SCMI_SMC_OEM_FUNC 0x400 +#define BRCM_SCMI_MBOX_NUM 0 +#define BRCM_FID(ch) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ + IS_ENABLED(CONFIG_ARM64), \ + ARM_SMCCC_OWNER_OEM, \ + BRCM_SCMI_SMC_OEM_FUNC + (ch)) +enum { + A2P_CHAN = 0, + NUM_CHAN +}; + +struct chan_priv { + unsigned int mbox_num; + unsigned int ch; +}; + +struct brcm_mbox { + struct mbox_controller controller; + int irqs[NUM_CHAN]; +}; + +static struct mbox_chan *brcm_mbox_of_xlate(struct mbox_controller *controller, + const struct of_phandle_args *sp) +{ + unsigned int ch = sp->args[0]; + struct brcm_mbox *mbox + = container_of(controller, struct brcm_mbox, controller); + + if (!mbox || ch >= NUM_CHAN) + return ERR_PTR(-ENOENT); + + return &mbox->controller.chans[ch]; +} + +#if defined(CONFIG_ARM64) || defined(CONFIG_ARM) +static int announce_msg(unsigned int mbox_num, unsigned int ch) +{ + struct arm_smccc_res res; + + if (ch >= NUM_CHAN) + return -EIO; + arm_smccc_smc(BRCM_FID(ch), mbox_num, 0, 0, 0, 0, 0, 0, &res); + if (res.a0) + return -EIO; + return 0; +} +#else +#error Func announce_msg() not defined for the current ARCH +#endif + +static int brcm_mbox_send_data(struct mbox_chan *chan, void *data) +{ + struct chan_priv *priv = chan->con_priv; + + return announce_msg(priv->mbox_num, priv->ch); +} + +static int brcm_mbox_startup(struct mbox_chan *chan) +{ + return 0; +} + +static const struct mbox_chan_ops brcm_mbox_ops = { + .send_data = brcm_mbox_send_data, + .startup = brcm_mbox_startup, +}; + +static irqreturn_t brcm_a2p_isr(int irq, void *data) +{ + struct mbox_chan *chan = data; + + mbox_chan_received_data(chan, NULL); + return IRQ_HANDLED; +} + +static int brcm_mbox_probe(struct platform_device *pdev) +{ + struct brcm_mbox *mbox; + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + struct chan_priv *chan_priv; + int ret; + + if (!np) + return -EINVAL; + + mbox = devm_kzalloc(&pdev->dev, sizeof(*mbox), GFP_KERNEL); + if (!mbox) + return -ENOMEM; + + /* Allocate channels */ + mbox->controller.chans = devm_kzalloc( + &pdev->dev, NUM_CHAN * sizeof(struct mbox_chan), GFP_KERNEL); + if (!mbox->controller.chans) + return -ENOMEM; + chan_priv = devm_kzalloc( + &pdev->dev, NUM_CHAN * sizeof(struct chan_priv), GFP_KERNEL); + if (!chan_priv) + return -ENOMEM; + + mbox->irqs[A2P_CHAN] = platform_get_irq(pdev, 0); + ret = devm_request_irq(&pdev->dev, mbox->irqs[A2P_CHAN], brcm_a2p_isr, + IRQF_NO_SUSPEND, "brcm: SCMI a2p intr", + &mbox->controller.chans[A2P_CHAN]); + if (ret) { + dev_err(&pdev->dev, "failed to setup SCMI a2p isr\n"); + return ret; + } + chan_priv[A2P_CHAN].mbox_num = BRCM_SCMI_MBOX_NUM; + chan_priv[A2P_CHAN].ch = A2P_CHAN; + mbox->controller.chans[A2P_CHAN].con_priv = &chan_priv[A2P_CHAN]; + mbox->controller.num_chans++; + mbox->controller.dev = &pdev->dev; + mbox->controller.ops = &brcm_mbox_ops; + mbox->controller.of_xlate = brcm_mbox_of_xlate; + ret = mbox_controller_register(&mbox->controller); + if (ret) { + dev_err(dev, "failed to register BrcmSTB mbox\n"); + return ret; + } + + platform_set_drvdata(pdev, mbox); + return 0; +} + +static int brcm_mbox_remove(struct platform_device *pdev) +{ + struct brcm_mbox *mbox = platform_get_drvdata(pdev); + + mbox_controller_unregister(&mbox->controller); + + return 0; +} + +static const struct of_device_id brcm_mbox_of_match[] = { + { .compatible = "brcm,brcmstb-mbox", }, + {} +}; +MODULE_DEVICE_TABLE(of, brcm_mbox_of_match); + +static struct platform_driver brcm_mbox_driver = { + .probe = brcm_mbox_probe, + .remove = brcm_mbox_remove, + .driver = { + .name = "brcm_mbox", + .of_match_table = brcm_mbox_of_match, + }, +}; + +module_platform_driver(brcm_mbox_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Broadcom STB SCMI Mailbox driver"); +MODULE_AUTHOR("Broadcom");