| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a3c3280478bf86eb97a422782ce60ec4eaa35224.camel@mediatek.com>
Date: Tue, 29 Oct 2024 08:27:37 +0000
From: Karl Li (李智嘉) <Karl.Li@...iatek.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
"wenst@...omium.org" <wenst@...omium.org>
CC: Chungying Lu (呂忠穎) <Chungying.Lu@...iatek.com>,
Project_Global_Chrome_Upstream_Group
<Project_Global_Chrome_Upstream_Group@...iatek.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, "robh@...nel.org"
<robh@...nel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
"jassisinghbrar@...il.com" <jassisinghbrar@...il.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "conor+dt@...nel.org"
<conor+dt@...nel.org>, "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>
Subject: Re: [PATCH 3/3] mailbox: mediatek: Add mtk-apu-mailbox driver
On Mon, 2024-10-28 at 14:16 +0800, Chen-Yu Tsai wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> On Thu, Oct 24, 2024 at 7:13 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@...labora.com> wrote:
> >
> > Il 24/10/24 11:25, Karl.Li ha scritto:
> > > From: Karl Li <karl.li@...iatek.com>
> > >
> > > Add mtk-apu-mailbox driver to support the communication with
> > > APU remote microprocessor.
> > >
> > > Also, the mailbox hardware contains extra spare (scratch)
> > > registers
> > > that other hardware blocks use to communicate through.
> > > Expose these with custom mtk_apu_mbox_(read|write)() functions.
> > >
> > > Signed-off-by: Karl Li <karl.li@...iatek.com>
> > > ---
> > > drivers/mailbox/Kconfig | 9 +
> > > drivers/mailbox/Makefile | 2 +
> > > drivers/mailbox/mtk-apu-mailbox.c | 222
> > > ++++++++++++++++++++++++
> > > include/linux/mailbox/mtk-apu-mailbox.h | 20 +++
> > > 4 files changed, 253 insertions(+)
> > > create mode 100644 drivers/mailbox/mtk-apu-mailbox.c
> > > create mode 100644 include/linux/mailbox/mtk-apu-mailbox.h
> > >
> > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > > index 6fb995778636..2338e08a110a 100644
> > > --- a/drivers/mailbox/Kconfig
> > > +++ b/drivers/mailbox/Kconfig
> > > @@ -240,6 +240,15 @@ config MTK_ADSP_MBOX
> > > between processors with ADSP. It will place the
> > > message to share
> > > buffer and will access the ipc control.
> > >
> > > +config MTK_APU_MBOX
> > > + tristate "MediaTek APU Mailbox Support"
> > > + depends on ARCH_MEDIATEK || COMPILE_TEST
> > > + help
> > > + Say yes here to add support for the MediaTek APU Mailbox
> > > + driver. The mailbox implementation provides access from
> > > the
> > > + application processor to the MediaTek AI Processing Unit.
> > > + If unsure say N.
> > > +
> > > config MTK_CMDQ_MBOX
> > > tristate "MediaTek CMDQ Mailbox Support"
> > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > > index 3c3c27d54c13..6b6dcc78d644 100644
> > > --- a/drivers/mailbox/Makefile
> > > +++ b/drivers/mailbox/Makefile
> > > @@ -53,6 +53,8 @@ obj-$(CONFIG_STM32_IPCC) += stm32-ipcc.o
> > >
> > > obj-$(CONFIG_MTK_ADSP_MBOX) += mtk-adsp-mailbox.o
> > >
> > > +obj-$(CONFIG_MTK_APU_MBOX) += mtk-apu-mailbox.o
> > > +
> > > obj-$(CONFIG_MTK_CMDQ_MBOX) += mtk-cmdq-mailbox.o
> > >
> > > obj-$(CONFIG_ZYNQMP_IPI_MBOX) += zynqmp-ipi-mailbox.o
> > > diff --git a/drivers/mailbox/mtk-apu-mailbox.c
> > > b/drivers/mailbox/mtk-apu-mailbox.c
> > > new file mode 100644
> > > index 000000000000..b347ebd34ef7
> > > --- /dev/null
> > > +++ b/drivers/mailbox/mtk-apu-mailbox.c
> > > @@ -0,0 +1,222 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2024 MediaTek Inc.
> > > + */
> > > +
> > > +#include <asm/io.h>
> > > +#include <linux/bits.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/mailbox_controller.h>
> > > +#include <linux/mailbox/mtk-apu-mailbox.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#define INBOX (0x0)
> > > +#define OUTBOX (0x20)
> > > +#define INBOX_IRQ (0xc0)
> > > +#define OUTBOX_IRQ (0xc4)
> > > +#define INBOX_IRQ_MASK (0xd0)
> > > +
> > > +#define SPARE_OFF_START (0x40)
> > > +#define SPARE_OFF_END (0xB0)
> > > +
> > > +struct mtk_apu_mailbox {
> > > + struct device *dev;
> > > + void __iomem *regs;
> > > + struct mbox_controller controller;
> >
> > struct mbox_controller mbox;
> >
> > ...it's shorter and consistent with at least other MTK mailbox
> > drivers.
> >
> > > + u32 msgs[MSG_MBOX_SLOTS];
> >
> > Just reuse struct mtk_apu_mailbox_msg instead.....
> >
> > > +};
> > > +
> > > +struct mtk_apu_mailbox *g_mbox;
> >
> > That global struct must disappear - and if you use the mailbox API
> > correctly
> > it's even simple.
> >
> > Also, you want something like....
> >
> > static inline struct mtk_apu_mailbox *get_mtk_apu_mailbox(struct
> > mbox_controller *mbox)
> > {
> > return container_of(mbox, struct mtk_apu_mailbox, mbox);
> > }
> >
> > > +
> > > +static irqreturn_t mtk_apu_mailbox_irq_top_half(int irq, void
> > > *dev_id)
> > > +{
> > static irqreturn_t mtk_apu_mailbox_irq(int irq, void *data)
> > {
> > struct mbox_chan *chan = data;
> > struct mtk_apu_mailbox = get_mtk_apu_mailbox(chan->mbox);
> >
> > > + struct mtk_apu_mailbox *mbox = dev_id;
> > > + struct mbox_chan *link = &mbox->controller.chans[0];
> > > + int i;
> > > +
> > > + for (i = 0; i < MSG_MBOX_SLOTS; i++)
> > > + mbox->msgs[i] = readl(mbox->regs + OUTBOX + i *
> > > sizeof(u32));
> > > +
> > > + mbox_chan_received_data(link, &mbox->msgs);
> > > +
> > > + return IRQ_WAKE_THREAD;
> > > +}
> > > +
> > > +static irqreturn_t mtk_apu_mailbox_irq_btm_half(int irq, void
> > > *dev_id)
> >
> > ....mtk_apu_mailbox_irq_thread(...)
> >
> > > +{
> > > + struct mtk_apu_mailbox *mbox = dev_id;
> > > + struct mbox_chan *link = &mbox->controller.chans[0];
> > > +
> > > + mbox_chan_received_data_bh(link, &mbox->msgs);
> >
> > I don't think that you really need this _bh variant, looks more
> > like you wanted
> > to have two callbacks instead of one.
> >
> > You can instead have one callback and vary functionality based
> > based on reading
> > a variable to decide what to actually do inside. Not a big deal.
>
> The problem is that they need something with different semantics.
> mbox_chan_received_data() is atomic only.
Yes, as Chen-Yu said, we want to have another callback which can run
under non-atomic semantic.
Even though we change the function based in the callback function of
mbox_chan_received_data(), it is still non-atomic for the bottom-half
handler.
>
> > > + writel(readl(mbox->regs + OUTBOX_IRQ), mbox->regs +
> > > OUTBOX_IRQ);
> > > +
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int mtk_apu_mailbox_send_data(struct mbox_chan *chan,
> > > void *data)
> > > +{
> > > + struct mtk_apu_mailbox *mbox = container_of(chan->mbox,
> > > + struct
> > > mtk_apu_mailbox,
> > > + controller);
> > > + struct mtk_apu_mailbox_msg *msg = data;
> > > + int i;
> > > +
> > > + if (msg->send_cnt <= 0 || msg->send_cnt > MSG_MBOX_SLOTS) {
> > > + dev_err(mbox->dev, "%s: invalid send_cnt %d\n",
> > > __func__, msg->send_cnt);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /*
> > > + * Mask lowest "send_cnt-1" interrupts bits, so the
> > > interrupt on the other side
> > > + * triggers only after the last data slot is written
> > > (sent).
> > > + */
> > > + writel(GENMASK(msg->send_cnt - 2, 0), mbox->regs +
> > > INBOX_IRQ_MASK);
> > > + for (i = 0; i < msg->send_cnt; i++)
> > > + writel(msg->data[i], mbox->regs + INBOX + i *
> > > sizeof(u32));
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static bool mtk_apu_mailbox_last_tx_done(struct mbox_chan *chan)
> > > +{
> > > + struct mtk_apu_mailbox *mbox = container_of(chan->mbox,
> > > + struct
> > > mtk_apu_mailbox,
> > > + controller);
> > > +
> > > + return readl(mbox->regs + INBOX_IRQ) == 0;
> > > +}
> > > +
> > > +static const struct mbox_chan_ops mtk_apu_mailbox_ops = {
> > > + .send_data = mtk_apu_mailbox_send_data,
> > > + .last_tx_done = mtk_apu_mailbox_last_tx_done,
> > > +};
> > > +
> > > +/**
> > > + * mtk_apu_mbox_write - Write value to specifice mtk_apu_mbox
> > > spare register.
> > > + * @val: Value to be written.
> > > + * @offset: Offset of the spare register.
> > > + *
> > > + * Return: 0 if successful
> > > + * negative value if error happened
> > > + */
> > > +int mtk_apu_mbox_write(u32 val, u32 offset)
> > > +{
> > > + if (!g_mbox) {
> > > + pr_err("mtk apu mbox was not initialized, stop
> > > writing register\n");
> > > + return -ENODEV;
> > > + }
> > > +
> > > + if (offset < SPARE_OFF_START || offset >= SPARE_OFF_END) {
> > > + dev_err(g_mbox->dev, "Invalid offset %d for mtk apu
> > > mbox spare register\n", offset);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + writel(val, g_mbox->regs + offset);
> >
> > There's something odd in what you're doing here, why would you ever
> > need
> > a function that performs a writel just like that? What's the
> > purpose?
> >
> > What are you writing to the spare registers?
> > For which reason?
>
> I'll leave the explaining to Karl, but based on internal reviews for
> the
> previous generation, it looked like passing values to/from the MCU.
>
The main reason we want to access the APU mailbox spare registers is to
ensure that we can configure the necessary settings before the APU
firmware becomes fully operational.
At the early stage, the communication pathways between the APU and the
Linux Kernel aren't yet available, so these spare registers are needed
for passing the initial configuration data.
> > I think you can avoid (read this as: you *have to* avoid) having
> > such a
> > function around.
>
> Again, during the previous round of internal reviews, I had thought
> about modeling these as extra mbox channels. I may have even asked
> about this on IRC.
>
> The problem is that it doesn't really have mbox semantics. They are
> just shared registers with no send/receive notification. So at the
> very least, there's nothing that will trigger a reception. I suppose
> we could make the .peek_data op trigger RX, but that's a really
> convoluted way to read just a register.
>
> The other option would be to have a syscon / custom exported regmap?
>
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_NS(mtk_apu_mbox_write, MTK_APU_MAILBOX);
> > > +
> > > +/**
> > > + * mtk_apu_mbox_read - Read value to specifice mtk_apu_mbox
> > > spare register.
> > > + * @offset: Offset of the spare register.
> > > + * @val: Pointer to store read value.
> > > + *
> > > + * Return: 0 if successful
> > > + * negative value if error happened
> > > + */
> > > +int mtk_apu_mbox_read(u32 offset, u32 *val)
> > > +{
> > > + if (!g_mbox) {
> > > + pr_err("mtk apu mbox was not initialized, stop
> > > reading register\n");
> > > + return -ENODEV;
> > > + }
> > > +
> > > + if (offset < SPARE_OFF_START || offset >= SPARE_OFF_END) {
> > > + dev_err(g_mbox->dev, "Invalid offset %d for mtk apu
> > > mbox spare register\n", offset);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + *val = readl(g_mbox->regs + offset);
> > > +
> >
> > Same goes for this one.
> >
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_NS(mtk_apu_mbox_read, MTK_APU_MAILBOX);
> > > +
> > > +static int mtk_apu_mailbox_probe(struct platform_device *pdev)
> > > +{
> > > + struct device *dev = &pdev->dev;
> > > + struct mtk_apu_mailbox *mbox;
> > > + int irq = -1, ret = 0;
> > > +
> > > + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > > + if (!mbox)
> > > + return -ENOMEM;
> > > +
> > > + mbox->dev = dev;
> > > + platform_set_drvdata(pdev, mbox);
> > > +
> >
> > Please move the platform_get_irq call here or anyway before
> > registering the
> > mbox controller: in case anything goes wrong, devm won't have to
> > unregister
> > the mbox afterwards because it never got registered in the first
> > place.
>
> To clarify, you mean _just_ platform_get_irq() and not request_irq as
> well.
>
> > > + mbox->regs = devm_platform_ioremap_resource(pdev, 0);
> > > + if (IS_ERR(mbox->regs))
> > > + return PTR_ERR(mbox->regs);
> > > +
> > > + mbox->controller.txdone_irq = false;
> > > + mbox->controller.txdone_poll = true;
> > > + mbox->controller.txpoll_period = 1;
> > > + mbox->controller.ops = &mtk_apu_mailbox_ops;
> > > + mbox->controller.dev = dev;
> > > + /*
> > > + * Here we only register 1 mbox channel.
> > > + * The remaining channels are used by other modules.
> >
> > What other modules? I don't really see any - so please at least
> > explain that in the
> > commit description.
Sorry for any confusion caused by the above comment. To clarify, the
comment was specific to the MT8188 platform, which is the legacy
platform compared to MT8196.
In the context of MT8188, the APU mailbox has multiple in/out boxes,
and Linux only utilizes in/out box 0, while the others are reserved for
different VMs.
However, the APU mailbox hardware design in MT8196 differs from that of
MT8188, and in MT8196, Linux has full access to the APU mailbox.
Given that this patch is primarily for the MT8196 platform, we will
remove the above comment in the next version of the patch.
Thanks for your asking.
Karl
> >
> > > + */
> > > + mbox->controller.num_chans = 1;
> > > + mbox->controller.chans = devm_kcalloc(dev, mbox-
> > > >controller.num_chans,
> > > + sizeof(*mbox-
> > > >controller.chans),
> > > + GFP_KERNEL);
> > > + if (!mbox->controller.chans)
> > > + return -ENOMEM;
> > > +
> > > + ret = devm_mbox_controller_register(dev, &mbox-
> > > >controller);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + irq = platform_get_irq(pdev, 0);
> > > + if (irq < 0)
> > > + return irq;
> > > +
> > > + ret = devm_request_threaded_irq(dev, irq,
> > > mtk_apu_mailbox_irq_top_half,
> > > +
> > > mtk_apu_mailbox_irq_btm_half, IRQF_ONESHOT,
> > > + dev_name(dev), mbox);
> >
> > pass mbox->chans to the isr
> >
> > > + if (ret)
> > > + return dev_err_probe(dev, ret, "Failed to request
> > > IRQ\n");
> > > +
> > > + g_mbox = mbox;
> > > +
> > > + dev_dbg(dev, "registered mtk apu mailbox\n");
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void mtk_apu_mailbox_remove(struct platform_device *pdev)
> > > +{
> > > + g_mbox = NULL;
> > > +}
> > > +
> > > +static const struct of_device_id mtk_apu_mailbox_of_match[] = {
> > > + { .compatible = "mediatek,mt8188-apu-mailbox" },
> > > + { .compatible = "mediatek,mt8196-apu-mailbox" },
> >
> > Just mediatek,mt8188-apu-mailbox is fine; you can allow
> > mt8196==mt8188 in the
> > binding instead.
> >
> > > + {}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mtk_apu_mailbox_of_match);
> > > +
> > > +static struct platform_driver mtk_apu_mailbox_driver = {
> > > + .probe = mtk_apu_mailbox_probe,
> > > + .remove = mtk_apu_mailbox_remove,
> >
> > You don't need this remove callback, since g_mbox has to disappear
> > :-)
> >
> > > + .driver = {
> > > + .name = "mtk-apu-mailbox",
> > > + .of_match_table = mtk_apu_mailbox_of_match,
> > > + },
> > > +};
> > > +
> > > +module_platform_driver(mtk_apu_mailbox_driver);
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_DESCRIPTION("MediaTek APU Mailbox Driver");
> > > diff --git a/include/linux/mailbox/mtk-apu-mailbox.h
> > > b/include/linux/mailbox/mtk-apu-mailbox.h
> > > new file mode 100644
> > > index 000000000000..d1457d16ce9b
> > > --- /dev/null
> > > +++ b/include/linux/mailbox/mtk-apu-mailbox.h
> > > @@ -0,0 +1,20 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright (c) 2024 MediaTek Inc.
> > > + *
> > > + */
> > > +
> > > +#ifndef __MTK_APU_MAILBOX_H__
> > > +#define __MTK_APU_MAILBOX_H__
> > > +
> > > +#define MSG_MBOX_SLOTS (8)
> > > +
> > > +struct mtk_apu_mailbox_msg {
> > > + int send_cnt;
> >
> > u8 data_cnt;
> >
> > > + u32 data[MSG_MBOX_SLOTS];
> >
> > With hardcoded slots, what happens when we get a new chip in the
> > future that
> > supports more slots?
>
> Seems like we can make it a flexible array member? But the problem
> then
> becomes how does the client know what the maximum length is. Or maybe
> it should already know given it's tied to a particular platform.
>
> In any case it becomes:
>
> struct mtk_apu_mailbox_msg {
> u8 data_size;
> u8 data[] __counted_by(data_size);
> };
>
> This can't be allocated on the stack if `data_size` isn't a compile
> time constant though; but again it shouldn't be a problem given the
> message size is tied to the client & its platform and should be
> constant anyway.
>
> The controller should just error out if the message is larger than
> what it can atomically send.
>
>
> ChenYu
>
> > Please think about this now and make the implementation flexible
> > before that
> > happens because, at a later time, it'll be harder.
> >
> > Regards,
> > Angelo
> >
> > > +};
> > > +
> > > +int mtk_apu_mbox_write(u32 val, u32 offset);
> > > +int mtk_apu_mbox_read(u32 offset, u32 *val);
> > > +
> > > +#endif /* __MTK_APU_MAILBOX_H__ */
> >
> >
Powered by blists - more mailing lists