On 17/10/2024 18:36, Tudor Ambarus wrote:
> ACPM (Alive Clock and Power Manager) is a firmware that operates on the
> APM (Active Power Management) module that handles overall power management
> activities. ACPM and masters regard each other as independent
> hardware component and communicate with each other using mailbox messages
> and shared memory.
>
> The mailbox channels are initialized based on the configuration data
> found at a specific offset into the shared memory (mmio-sram). The
> configuration data consists of channel id, message and queue lengths,
> pointers to the RX and TX queues which are also part of the SRAM, and
> whether RX works by polling or interrupts. All known clients of this
> driver are using polling channels, thus the driver implements for now
> just polling mode.
>
> Add support for the exynos acpm core driver. Helper drivers will follow.
> These will construct the mailbox messages in the format expected by the
> firmware.
I skimmed through the driver and I do not understand why this is
firmware. You are implementing a mailbox provider/controller.
I did not perform full review yet, just skimmed over the code. I will
take a look a bit later.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> ---
> drivers/firmware/Kconfig | 1 +
> drivers/firmware/Makefile | 1 +
> drivers/firmware/samsung/Kconfig | 11 +
> drivers/firmware/samsung/Makefile | 3 +
> drivers/firmware/samsung/exynos-acpm.c | 703 ++++++++++++++++++++
Please add directory to the Samsung Exynos SoC maintainer entry. I also
encourage adding separate entry for the driver where you would be listed
as maintainer.
There is no firmware tree, so this will be going via Samsung SoC.
> include/linux/mailbox/exynos-acpm-message.h | 21 +
> 6 files changed, 740 insertions(+)
> create mode 100644 drivers/firmware/samsung/Kconfig
> create mode 100644 drivers/firmware/samsung/Makefile
> create mode 100644 drivers/firmware/samsung/exynos-acpm.c
> create mode 100644 include/linux/mailbox/exynos-acpm-message.h
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 71d8b26c4103..24edb956831b 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -267,6 +267,7 @@ source "drivers/firmware/meson/Kconfig"
> source "drivers/firmware/microchip/Kconfig"
> source "drivers/firmware/psci/Kconfig"
> source "drivers/firmware/qcom/Kconfig"
> +source "drivers/firmware/samsung/Kconfig"
> source "drivers/firmware/smccc/Kconfig"
> source "drivers/firmware/tegra/Kconfig"
> source "drivers/firmware/xilinx/Kconfig"
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 7a8d486e718f..91efcc868a05 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -33,6 +33,7 @@ obj-y += efi/
> obj-y += imx/
> obj-y += psci/
> obj-y += qcom/
> +obj-y += samsung/
> obj-y += smccc/
> obj-y += tegra/
> obj-y += xilinx/
> diff --git a/drivers/firmware/samsung/Kconfig b/drivers/firmware/samsung/Kconfig
> new file mode 100644
> index 000000000000..f908773c1441
> --- /dev/null
> +++ b/drivers/firmware/samsung/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config EXYNOS_ACPM
> + tristate "Exynos ACPM (Alive Clock and Power Manager) driver support"
depends ARCH_EXYNOS || COMPILE_TEST
Please also send a arm64 defconfig change making it a module.
> + select MAILBOX
> + help
> + ACPM is a firmware that operates on the APM (Active Power Management)
> + module that handles overall power management activities. ACPM and
> + masters regard each other as independent hardware component and
> + communicate with each other using mailbox messages and shared memory.
> + This module provides the means to communicate with the ACPM firmware.
> diff --git a/drivers/firmware/samsung/Makefile b/drivers/firmware/samsung/Makefile
> new file mode 100644
> index 000000000000..35ff3076bbea
> --- /dev/null
> +++ b/drivers/firmware/samsung/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_EXYNOS_ACPM) += exynos-acpm.o
> diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
> new file mode 100644
> index 000000000000..c3ad4dc7a9e0
> --- /dev/null
> +++ b/drivers/firmware/samsung/exynos-acpm.c
> @@ -0,0 +1,703 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2020 Samsung Electronics Co., Ltd.
> + * Copyright 2020 Google LLC.
> + * Copyright 2024 Linaro Ltd.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>
> +#include <linux/bits.h>
> +#include <linux/container_of.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/mailbox/exynos-acpm-message.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define EXYNOS_ACPM_MCUCTRL 0x0 /* Mailbox Control Register */
> +#define EXYNOS_ACPM_INTCR0 0x24 /* Interrupt Clear Register 0 */
> +#define EXYNOS_ACPM_INTMR0 0x28 /* Interrupt Mask Register 0 */
> +#define EXYNOS_ACPM_INTSR0 0x2c /* Interrupt Status Register 0 */
> +#define EXYNOS_ACPM_INTMSR0 0x30 /* Interrupt Mask Status Register 0 */
> +#define EXYNOS_ACPM_INTGR1 0x40 /* Interrupt Generation Register 1 */
> +#define EXYNOS_ACPM_INTMR1 0x48 /* Interrupt Mask Register 1 */
> +#define EXYNOS_ACPM_INTSR1 0x4c /* Interrupt Status Register 1 */
> +#define EXYNOS_ACPM_INTMSR1 0x50 /* Interrupt Mask Status Register 1 */
> +
> +#define EXYNOS_ACPM_INTMR0_MASK GENMASK(15, 0)
> +#define EXYNOS_ACPM_PROTOCOL_SEQNUM GENMASK(21, 16)
> +
> +/* The unit of counter is 20 us. 5000 * 20 = 100 ms */
> +#define EXYNOS_ACPM_POLL_TIMEOUT 5000
> +#define EXYNOS_ACPM_TX_TIMEOUT_US 500000
> +
> +/**
> + * struct exynos_acpm_shmem - mailbox shared memory configuration information.
> + * @reserved: reserved for future use.
> + * @chans: offset to array of struct exynos_acpm_shmem_chan.
> + * @reserved1: reserved for future use.
> + * @num_chans: number of channels.
> + */
> +struct exynos_acpm_shmem {
> + u32 reserved[2];
> + u32 chans;
> + u32 reserved1[3];
> + u32 num_chans;
> +};
> +
> +/**
> + * struct exynos_acpm_shmem_chan - descriptor of a shared memory channel.
> + *
> + * @id: channel ID.
> + * @reserved: reserved for future use.
> + * @rx_rear: rear pointer of RX queue.
> + * @rx_front: front pointer of RX queue.
> + * @rx_base: base address of RX queue.
> + * @reserved1: reserved for future use.
> + * @tx_rear: rear pointer of TX queue.
> + * @tx_front: front pointer of TX queue.
> + * @tx_base: base address of TX queue.
> + * @qlen: queue length. Applies to both TX/RX queues.
> + * @mlen: message length. Applies to both TX/RX queues.
> + * @reserved2: reserved for future use.
> + * @polling: true when the channel works on polling.
> + */
> +struct exynos_acpm_shmem_chan {
> + u32 id;
> + u32 reserved[3];
> + u32 rx_rear;
> + u32 rx_front;
> + u32 rx_base;
> + u32 reserved1[3];
> + u32 tx_rear;
> + u32 tx_front;
> + u32 tx_base;
> + u32 qlen;
> + u32 mlen;
> + u32 reserved2[2];
> + u32 polling;
Why are you storing addresses as u32? Shouldn't these be __iomem*?
I also cannot find any piece of code setting several of above, e.g. tx_base
> +};
> +
> +/**
> + * struct exynos_acpm_queue - exynos acpm queue.
> + *
> + * @rear: rear address of the queue.
> + * @front: front address of the queue.
> + * @base: base address of the queue.
> + */
> +struct exynos_acpm_queue {
> + void __iomem *rear;
> + void __iomem *front;
> + void __iomem *base;
> +};
> +
> +/**
> + * struct exynos_acpm_rx_data - RX queue data.
> + *
> + * @cmd: pointer to where the data shall be saved.
> + * @response: true if the client expects the RX data.
> + */
> +struct exynos_acpm_rx_data {
> + u32 *cmd;
> + bool response;
> +};
> +
> +#define EXYNOS_ACPM_SEQNUM_MAX 64
> +
...
> + if (IS_ERR(work_data))
> + return PTR_ERR(work_data);
> +
> + spin_lock_irqsave(&chan->tx_lock, flags);
> +
> + tx_front = readl_relaxed(chan->tx.front);
> + idx = (tx_front + 1) % chan->qlen;
> +
> + ret = exynos_acpm_wait_for_queue_slots(mbox_chan, idx);
> + if (ret)
> + goto exit;
> +
> + exynos_acpm_prepare_request(mbox_chan, req);
> +
> + /* Write TX command. */
> + __iowrite32_copy(chan->tx.base + chan->mlen * tx_front, tx->cmd,
> + req->txlen / 4);
> +
> + /* Advance TX front. */
> + writel_relaxed(idx, chan->tx.front);
> +
> + /* Flush SRAM posted writes. */
> + readl_relaxed(chan->tx.front);
> +
How does this flush work? Maybe you just miss here barries (s/relaxed//)?
> + /* Generate ACPM interrupt. */
> + writel_relaxed(BIT(chan->id), exynos_acpm->regs + EXYNOS_ACPM_INTGR1);
> +
> + /* Flush mailbox controller posted writes. */
> + readl_relaxed(exynos_acpm->regs + EXYNOS_ACPM_MCUCTRL);
> +
> + spin_unlock_irqrestore(&chan->tx_lock, flags);
> +
> + queue_work(exynos_acpm->wq, &work_data->work);
> +
> + return -EINPROGRESS;
> +exit:
> + spin_unlock_irqrestore(&chan->tx_lock, flags);
> + kfree(work_data);
> + return ret;
> +}
> +
> +static int exynos_acpm_chan_startup(struct mbox_chan *mbox_chan)
> +{
> + struct exynos_acpm_chan *chan = mbox_chan->con_priv;
> +
> + if (!chan->polling) {
> + dev_err(mbox_chan->mbox->dev, "IRQs not supported.\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static const struct mbox_chan_ops exynos_acpm_chan_ops = {
> + .send_request = exynos_acpm_send_request,
> + .startup = exynos_acpm_chan_startup,
> +};
> +
> +static void __iomem *exynos_acpm_get_iomem_addr(void __iomem *base,
> + void __iomem *addr)
> +{
> + u32 offset;
> +
> + offset = readl_relaxed(addr);
> + return base + offset;
> +}
> +
> +static void exynos_acpm_rx_queue_init(struct exynos_acpm *exynos_acpm,
> + struct exynos_acpm_shmem_chan *shmem_chan,
> + struct exynos_acpm_queue *rx)
> +{
> + void __iomem *base = exynos_acpm->sram_base;
> +
> + rx->base = exynos_acpm_get_iomem_addr(base, &shmem_chan->tx_base);
> + rx->front = exynos_acpm_get_iomem_addr(base, &shmem_chan->tx_front);
> + rx->rear = exynos_acpm_get_iomem_addr(base, &shmem_chan->tx_rear);
> +}
> +
> +static void exynos_acpm_tx_queue_init(struct exynos_acpm *exynos_acpm,
> + struct exynos_acpm_shmem_chan *shmem_chan,
> + struct exynos_acpm_queue *tx)
> +{
> + void __iomem *base = exynos_acpm->sram_base;
> +
> + tx->base = exynos_acpm_get_iomem_addr(base, &shmem_chan->rx_base);
> + tx->front = exynos_acpm_get_iomem_addr(base, &shmem_chan->rx_front);
> + tx->rear = exynos_acpm_get_iomem_addr(base, &shmem_chan->rx_rear);
> +}
> +
> +static int exynos_acpm_alloc_cmds(struct exynos_acpm *exynos_acpm,
> + struct exynos_acpm_chan *chan)
> +{
> + struct device *dev = exynos_acpm->dev;
> + struct exynos_acpm_rx_data *rx_data;
> + unsigned int mlen = chan->mlen;
> + int i;
> +
> + for (i = 0; i < EXYNOS_ACPM_SEQNUM_MAX; i++) {
> + rx_data = &chan->rx_data[i];
> + rx_data->cmd = devm_kcalloc(dev, mlen, sizeof(*(rx_data->cmd)),
> + GFP_KERNEL);
> + if (!rx_data->cmd)
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static int exynos_acpm_chans_init(struct exynos_acpm *exynos_acpm)
> +{
> + struct exynos_acpm_shmem_chan *shmem_chans, *shmem_chan;
> + struct exynos_acpm_shmem *shmem = exynos_acpm->shmem;
> + struct mbox_chan *mbox_chan, *mbox_chans;
> + struct exynos_acpm_chan *chan, *chans;
> + struct device *dev = exynos_acpm->dev;
> + int i, ret;
> +
> + exynos_acpm->num_chans = readl_relaxed(&shmem->num_chans);
> +
> + mbox_chans = devm_kcalloc(dev, exynos_acpm->num_chans,
> + sizeof(*mbox_chans), GFP_KERNEL);
> + if (!mbox_chans)
> + return -ENOMEM;
> +
> + chans = devm_kcalloc(dev, exynos_acpm->num_chans, sizeof(*chans),
> + GFP_KERNEL);
> + if (!chans)
> + return -ENOMEM;
> +
> + shmem_chans = exynos_acpm_get_iomem_addr(exynos_acpm->sram_base,
> + &shmem->chans);
> +
> + for (i = 0; i < exynos_acpm->num_chans; i++) {
> + shmem_chan = &shmem_chans[i];
> + mbox_chan = &mbox_chans[i];
> + chan = &chans[i];
> +
> + /* Set parameters for the mailbox channel. */
> + mbox_chan->con_priv = chan;
> + mbox_chan->mbox = exynos_acpm->mbox;
> +
> + /* Set parameters for the ACPM channel. */
> + chan->mlen = readl_relaxed(&shmem_chan->mlen);
> +
> + ret = exynos_acpm_alloc_cmds(exynos_acpm, chan);
> + if (ret)
> + return ret;
> +
> + chan->polling = readl_relaxed(&shmem_chan->polling);
> + chan->id = readl_relaxed(&shmem_chan->id);
> + chan->qlen = readl_relaxed(&shmem_chan->qlen);
> +
> + exynos_acpm_rx_queue_init(exynos_acpm, shmem_chan, &chan->rx);
> + exynos_acpm_tx_queue_init(exynos_acpm, shmem_chan, &chan->tx);
> +
> + mutex_init(&chan->rx_lock);
> + spin_lock_init(&chan->tx_lock);
> +
> + dev_vdbg(dev, "ID = %d poll = %d, mlen = %d, qlen = %d\n",
> + chan->id, chan->polling, chan->mlen, chan->qlen);
> + }
> +
> + /* Save pointers to the ACPM and mailbox channels. */
> + exynos_acpm->mbox->chans = mbox_chans;
> + exynos_acpm->chans = chans;
> +
> + return 0;
> +}
> +
> +static const struct of_device_id exynos_acpm_match[] = {
> + { .compatible = "google,gs101-acpm" },
Where are the bindings?
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, exynos_acpm_match);
> +
> +static int exynos_acpm_probe(struct platform_device *pdev)
> +{
> + struct device_node *node = pdev->dev.of_node;
> + struct device *dev = &pdev->dev;
> + struct exynos_acpm *exynos_acpm;
> + struct mbox_controller *mbox;
> + struct device_node *shmem;
> + resource_size_t size;
> + struct resource res;
> + const __be32 *prop;
> + int ret;
> +
> + exynos_acpm = devm_kzalloc(dev, sizeof(*exynos_acpm), GFP_KERNEL);
> + if (!exynos_acpm)
> + return -ENOMEM;
> +
> + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> + if (!mbox)
> + return -ENOMEM;
> +
> + exynos_acpm->regs = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(exynos_acpm->regs))
> + return PTR_ERR(exynos_acpm->regs);
> +
> + shmem = of_parse_phandle(node, "shmem", 0);
> + ret = of_address_to_resource(shmem, 0, &res);
> + of_node_put(shmem);
> + if (ret) {
> + dev_err(dev, "Failed to get shared memory.\n");
> + return ret;
> + }
> +
> + size = resource_size(&res);
> + exynos_acpm->sram_base = devm_ioremap(dev, res.start, size);
> + if (!exynos_acpm->sram_base) {
> + dev_err(dev, "Failed to ioremap shared memory.\n");
> + return -ENOMEM;
> + }
> +
> + prop = of_get_property(node, "initdata-base", NULL);
> + if (!prop) {
> + dev_err(dev, "Parsing initdata_base failed.\n");
> + return -EINVAL;
> + }
> +
> + exynos_acpm->pclk = devm_clk_get(dev, "pclk");
devm_clk_get_enabled
> + if (IS_ERR(exynos_acpm->pclk)) {
> + dev_err(dev, "Missing peripheral clock.\n");
return dev_err_probe()
> + return PTR_ERR(exynos_acpm->pclk);
> + }
> +
> + ret = clk_prepare_enable(exynos_acpm->pclk);
> + if (ret) {
> + dev_err(dev, "Failed to enable the peripheral clock.\n");
> + return ret;
> + }
> +
> + exynos_acpm->wq = alloc_workqueue("exynos-acpm-wq", 0, 0);
> + if (!exynos_acpm->wq)
> + return -ENOMEM;
> +
> + exynos_acpm->dev = dev;
> + exynos_acpm->mbox = mbox;
> + exynos_acpm->shmem = exynos_acpm->sram_base + be32_to_cpup(prop);
> +
> + ret = exynos_acpm_chans_init(exynos_acpm);
> + if (ret)
> + return ret;
> +
> + mbox->num_chans = exynos_acpm->num_chans;
> + mbox->dev = dev;
> + mbox->ops = &exynos_acpm_chan_ops;
> +
> + platform_set_drvdata(pdev, exynos_acpm);
> +
> + /* Mask out all interrupts. We support just polling channels for now. */
> + writel_relaxed(EXYNOS_ACPM_INTMR0_MASK,
> + exynos_acpm->regs + EXYNOS_ACPM_INTMR0);
> +
> + ret = devm_mbox_controller_register(dev, mbox);
> + if (ret)
> + dev_err(dev, "Failed to register mbox_controller(%d).\n", ret);
> +
> + return ret;
> +}
> +
> +static void exynos_acpm_remove(struct platform_device *pdev)
> +{
> + struct exynos_acpm *exynos_acpm = platform_get_drvdata(pdev);
> +
> + flush_workqueue(exynos_acpm->wq);
> + destroy_workqueue(exynos_acpm->wq);
> + clk_disable_unprepare(exynos_acpm->pclk);
> +}
> +
> +static struct platform_driver exynos_acpm_driver = {
> + .probe = exynos_acpm_probe,
> + .remove = exynos_acpm_remove,
> + .driver = {
> + .name = "exynos-acpm",
> + .owner = THIS_MODULE,
Drop
> + .of_match_table = exynos_acpm_match,
> + },
> +};
> +module_platform_driver(exynos_acpm_driver);
Best regards,
Krzysztof
