On 26-09-24 18:02:49, Arnaud Ferraris wrote:
> This driver (ported from the Linux kernel) implements 1-byte wide read
> and write operations for the SPMI-PMIF bus found on MediaTek SoCs. It
> currently supports MT8195 and MT8196.
>
> Signed-off-by: Arnaud Ferraris <[email protected]>
> ---
> drivers/spmi/Kconfig | 7 +
> drivers/spmi/Makefile | 1 +
> drivers/spmi/spmi-mtk-pmif.c | 462
> +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 470 insertions(+)
>
> diff --git a/drivers/spmi/Kconfig b/drivers/spmi/Kconfig
> index f8972371150..cab82e83c24 100644
> --- a/drivers/spmi/Kconfig
> +++ b/drivers/spmi/Kconfig
> @@ -27,6 +27,13 @@ config SPL_SPMI_MSM
> Support SPMI bus implementation found on Qualcomm Snapdragon SoCs
> in SPL.
>
> +config SPMI_MTK_PMIF
> + bool "Support MediaTek SPMI-PMIF bus"
> + depends on SPMI
> + help
> + Support SPMI bus implementation found on MediaTek SoCs. This
> + driver currently support the MT8195 and MT8196 SoCs.
> +
> config SPMI_SANDBOX
> bool "Support for Sandbox SPMI bus"
> depends on SPMI
> diff --git a/drivers/spmi/Makefile b/drivers/spmi/Makefile
> index 8cb93c820e0..016a8e6b734 100644
> --- a/drivers/spmi/Makefile
> +++ b/drivers/spmi/Makefile
> @@ -4,4 +4,5 @@
>
> obj-$(CONFIG_$(PHASE_)SPMI) += spmi-uclass.o
> obj-$(CONFIG_$(PHASE_)SPMI_MSM) += spmi-msm.o
> +obj-$(CONFIG_SPMI_MTK_PMIF) += spmi-mtk-pmif.o
> obj-$(CONFIG_SPMI_SANDBOX) += spmi-sandbox.o
> diff --git a/drivers/spmi/spmi-mtk-pmif.c b/drivers/spmi/spmi-mtk-pmif.c
> new file mode 100644
> index 00000000000..0bc8309b447
> --- /dev/null
> +++ b/drivers/spmi/spmi-mtk-pmif.c
> @@ -0,0 +1,462 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2021 MediaTek Inc.
> + * Copyright (c) 2025-2026 Collabora Ltd
> + * Authors: AngeloGioacchino Del Regno
> <[email protected]>
> + * Arnaud Ferraris <[email protected]>
> + */
> +
> +#include <clk.h>
> +#include <dm.h>
> +#include <malloc.h>
> +#include <regmap.h>
> +#include <syscon.h>
> +#include <asm/io.h>
> +#include <asm/processor.h>
> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/iopoll.h>
> +#include <linux/time.h>
> +#include <dm/device_compat.h>
> +#include <dm/device-internal.h>
> +#include <dm/devres.h>
> +#include <inttypes.h>
> +#include <spmi/spmi.h>
Do you really need all of these?
> +
> +#define SWINF_IDLE 0x00
> +#define SWINF_WFVLDCLR 0x06
> +
> +#define GET_SWINF(x) (((x) >> 1) & 0x7)
> +
> +#define PMIF_CMD_REG_0 0
> +#define PMIF_CMD_REG 1
> +#define PMIF_CMD_EXT_REG 2
> +#define PMIF_CMD_EXT_REG_LONG 3
> +
> +#define PMIF_DELAY_US 2
> +#define PMIF_TIMEOUT_US (10 * 1000)
> +
> +#define PMIF_CHAN_OFFSET 0x5
> +
> +#define PMIF_MAX_BUSES 2
> +#define PMIF_MAX_CLKS 3
> +
> +#define SPMI_OP_ST_BUSY 1
PMIF_MAX_CLKS and SPMI_OP_ST_BUSY are unused. Same for PMIF_CMD_REG_0,
PMIF_CMD_REG and PMIF_CMD_EXT_REG above. Please drop the unused defines.
> +
> +struct ch_reg {
> + u32 ch_sta;
> + u32 wdata;
> + u32 rdata;
> + u32 ch_send;
> + u32 ch_rdy;
> +};
> +
> +struct pmif_data {
> + const u32 *regs;
> + const u32 *spmimst_regs;
> + u32 soc_chan;
> + u8 spmi_ver;
> + u32 num_spmi_buses;
> +};
> +
> +struct pmif_bus {
> + void __iomem *base;
> + void __iomem *spmimst_base;
> + struct pmif *arb;
> + struct clk_bulk clks;
> +};
> +
> +struct pmif {
> + struct pmif_bus bus[PMIF_MAX_BUSES];
> + struct ch_reg chan;
> + const struct pmif_data *data;
> +};
The bus array isn't used, since the bus instances are allocated separately
in bind(). spmi_ver and spmimst_regs also don't seem to have any users.
Can you drop the unused fields and the unused SPMI register table?
> +
> +enum pmif_regs {
> + PMIF_INIT_DONE,
> + PMIF_INF_EN,
> + PMIF_ARB_EN,
> + PMIF_CMDISSUE_EN,
> + PMIF_TIMER_CTRL,
> + PMIF_SPI_MODE_CTRL,
> + PMIF_IRQ_EVENT_EN_0,
> + PMIF_IRQ_FLAG_0,
> + PMIF_IRQ_CLR_0,
> + PMIF_IRQ_EVENT_EN_1,
> + PMIF_IRQ_FLAG_1,
> + PMIF_IRQ_CLR_1,
> + PMIF_IRQ_EVENT_EN_2,
> + PMIF_IRQ_FLAG_2,
> + PMIF_IRQ_CLR_2,
> + PMIF_IRQ_EVENT_EN_3,
> + PMIF_IRQ_FLAG_3,
> + PMIF_IRQ_CLR_3,
> + PMIF_IRQ_EVENT_EN_4,
> + PMIF_IRQ_FLAG_4,
> + PMIF_IRQ_CLR_4,
> + PMIF_WDT_EVENT_EN_0,
> + PMIF_WDT_FLAG_0,
> + PMIF_WDT_EVENT_EN_1,
> + PMIF_WDT_FLAG_1,
> + PMIF_SWINF_0_STA,
> + PMIF_SWINF_0_WDATA_31_0,
> + PMIF_SWINF_0_RDATA_31_0,
> + PMIF_SWINF_0_ACC,
> + PMIF_SWINF_0_VLD_CLR,
> + PMIF_SWINF_1_STA,
> + PMIF_SWINF_1_WDATA_31_0,
> + PMIF_SWINF_1_RDATA_31_0,
> + PMIF_SWINF_1_ACC,
> + PMIF_SWINF_1_VLD_CLR,
> + PMIF_SWINF_2_STA,
> + PMIF_SWINF_2_WDATA_31_0,
> + PMIF_SWINF_2_RDATA_31_0,
> + PMIF_SWINF_2_ACC,
> + PMIF_SWINF_2_VLD_CLR,
> + PMIF_SWINF_3_STA,
> + PMIF_SWINF_3_WDATA_31_0,
> + PMIF_SWINF_3_RDATA_31_0,
> + PMIF_SWINF_3_ACC,
> + PMIF_SWINF_3_VLD_CLR,
> +};
> +
> +static const u32 mt8195_regs[] = {
> + [PMIF_INIT_DONE] = 0x0000,
> + [PMIF_INF_EN] = 0x0024,
> + [PMIF_ARB_EN] = 0x0150,
> + [PMIF_CMDISSUE_EN] = 0x03B8,
> + [PMIF_TIMER_CTRL] = 0x03E4,
> + [PMIF_SPI_MODE_CTRL] = 0x0408,
> + [PMIF_IRQ_EVENT_EN_0] = 0x0420,
> + [PMIF_IRQ_FLAG_0] = 0x0428,
> + [PMIF_IRQ_CLR_0] = 0x042C,
> + [PMIF_IRQ_EVENT_EN_1] = 0x0430,
> + [PMIF_IRQ_FLAG_1] = 0x0438,
> + [PMIF_IRQ_CLR_1] = 0x043C,
> + [PMIF_IRQ_EVENT_EN_2] = 0x0440,
> + [PMIF_IRQ_FLAG_2] = 0x0448,
> + [PMIF_IRQ_CLR_2] = 0x044C,
> + [PMIF_IRQ_EVENT_EN_3] = 0x0450,
> + [PMIF_IRQ_FLAG_3] = 0x0458,
> + [PMIF_IRQ_CLR_3] = 0x045C,
> + [PMIF_IRQ_EVENT_EN_4] = 0x0460,
> + [PMIF_IRQ_FLAG_4] = 0x0468,
> + [PMIF_IRQ_CLR_4] = 0x046C,
> + [PMIF_WDT_EVENT_EN_0] = 0x0474,
> + [PMIF_WDT_FLAG_0] = 0x0478,
> + [PMIF_WDT_EVENT_EN_1] = 0x047C,
> + [PMIF_WDT_FLAG_1] = 0x0480,
> + [PMIF_SWINF_0_ACC] = 0x0800,
> + [PMIF_SWINF_0_WDATA_31_0] = 0x0804,
> + [PMIF_SWINF_0_RDATA_31_0] = 0x0814,
> + [PMIF_SWINF_0_VLD_CLR] = 0x0824,
> + [PMIF_SWINF_0_STA] = 0x0828,
> + [PMIF_SWINF_1_ACC] = 0x0840,
> + [PMIF_SWINF_1_WDATA_31_0] = 0x0844,
> + [PMIF_SWINF_1_RDATA_31_0] = 0x0854,
> + [PMIF_SWINF_1_VLD_CLR] = 0x0864,
> + [PMIF_SWINF_1_STA] = 0x0868,
> + [PMIF_SWINF_2_ACC] = 0x0880,
> + [PMIF_SWINF_2_WDATA_31_0] = 0x0884,
> + [PMIF_SWINF_2_RDATA_31_0] = 0x0894,
> + [PMIF_SWINF_2_VLD_CLR] = 0x08A4,
> + [PMIF_SWINF_2_STA] = 0x08A8,
> + [PMIF_SWINF_3_ACC] = 0x08C0,
> + [PMIF_SWINF_3_WDATA_31_0] = 0x08C4,
> + [PMIF_SWINF_3_RDATA_31_0] = 0x08D4,
> + [PMIF_SWINF_3_VLD_CLR] = 0x08E4,
> + [PMIF_SWINF_3_STA] = 0x08E8,
> +};
> +
> +enum spmi_regs {
> + SPMI_OP_ST_CTRL,
> + SPMI_GRP_ID_EN,
> + SPMI_OP_ST_STA,
> + SPMI_MST_SAMPL,
> + SPMI_MST_REQ_EN,
> + SPMI_REC_CTRL,
> + SPMI_REC0,
> + SPMI_REC1,
> + SPMI_REC2,
> + SPMI_REC3,
> + SPMI_REC4,
> + SPMI_MST_DBG,
> +
> + /* MT8195 spmi regs */
> + SPMI_MST_RCS_CTRL,
> + SPMI_SLV_3_0_EINT,
> + SPMI_SLV_7_4_EINT,
> + SPMI_SLV_B_8_EINT,
> + SPMI_SLV_F_C_EINT,
> + SPMI_REC_CMD_DEC,
> + SPMI_DEC_DBG,
> +};
> +
> +static const u32 mt8195_spmi_regs[] = {
> + [SPMI_OP_ST_CTRL] = 0x0000,
> + [SPMI_GRP_ID_EN] = 0x0004,
> + [SPMI_OP_ST_STA] = 0x0008,
> + [SPMI_MST_SAMPL] = 0x000C,
> + [SPMI_MST_REQ_EN] = 0x0010,
> + [SPMI_MST_RCS_CTRL] = 0x0014,
> + [SPMI_SLV_3_0_EINT] = 0x0020,
> + [SPMI_SLV_7_4_EINT] = 0x0024,
> + [SPMI_SLV_B_8_EINT] = 0x0028,
> + [SPMI_SLV_F_C_EINT] = 0x002C,
> + [SPMI_REC_CTRL] = 0x0040,
> + [SPMI_REC0] = 0x0044,
> + [SPMI_REC1] = 0x0048,
> + [SPMI_REC2] = 0x004C,
> + [SPMI_REC3] = 0x0050,
> + [SPMI_REC4] = 0x0054,
> + [SPMI_REC_CMD_DEC] = 0x005C,
> + [SPMI_DEC_DBG] = 0x00F8,
> + [SPMI_MST_DBG] = 0x00FC,
> +};
> +
> +static u32 pmif_readl(struct pmif *arb, struct pmif_bus *pbus, enum
> pmif_regs reg)
> +{
> + return readl(pbus->base + arb->data->regs[reg]);
> +}
> +
> +static void pmif_writel(struct pmif *arb, struct pmif_bus *pbus,
> + u32 val, enum pmif_regs reg)
> +{
> + writel(val, pbus->base + arb->data->regs[reg]);
> +}
> +
> +static bool pmif_is_fsm_vldclr(struct pmif *arb, struct pmif_bus *pbus)
> +{
> + u32 reg_rdata;
> +
> + reg_rdata = pmif_readl(arb, pbus, arb->chan.ch_sta);
> +
> + return GET_SWINF(reg_rdata) == SWINF_WFVLDCLR;
> +}
> +
> +static int pmif_spmi_read(struct udevice *dev, int sid, int pid, int reg)
> +{
> + struct pmif_bus *pbus = dev_get_priv(dev);
> + struct pmif *arb = pbus->arb;
> + struct ch_reg *inf_reg;
> + int ret, data;
> + u32 cmd;
> +
> + /* Check for argument validation. */
> + if (sid & ~0xf) {
> + dev_err(dev, "exceed the max slv id\n");
> + return -EINVAL;
> + }
> +
> + /* Wait for Software Interface FSM state to be IDLE. */
> + inf_reg = &arb->chan;
> + ret = readl_poll_sleep_timeout(pbus->base +
> arb->data->regs[inf_reg->ch_sta],
> + data, GET_SWINF(data) == SWINF_IDLE,
> + PMIF_DELAY_US, PMIF_TIMEOUT_US);
> + if (ret < 0) {
> + /* set channel ready if the data has transferred */
> + if (pmif_is_fsm_vldclr(arb, pbus))
> + pmif_writel(arb, pbus, 1, inf_reg->ch_rdy);
> + dev_err(dev, "failed to wait for SWINF_IDLE\n");
> + return ret;
> + }
> +
> + /* Send the command. */
> + cmd = (PMIF_CMD_EXT_REG_LONG << 30) | (sid << 24) | reg;
> + pmif_writel(arb, pbus, cmd, inf_reg->ch_send);
> +
> + /*
> + * Wait for Software Interface FSM state to be WFVLDCLR,
> + * read the data and clear the valid flag.
> + */
> + ret = readl_poll_sleep_timeout(pbus->base +
> arb->data->regs[inf_reg->ch_sta],
> + data, GET_SWINF(data) == SWINF_WFVLDCLR,
> + PMIF_DELAY_US, PMIF_TIMEOUT_US);
> + if (ret < 0) {
> + dev_err(dev, "failed to wait for SWINF_WFVLDCLR\n");
> + return ret;
> + }
> +
> + data = (int)pmif_readl(arb, pbus, inf_reg->rdata);
Are the upper bits guaranteed to be zero for a single-byte read? Otherwise,
this can return a value outside 0..255, or even a negative value which the
caller will interpret as an error. I'd mask the result with 0xff here.
> + pmif_writel(arb, pbus, 1, inf_reg->ch_rdy);
> +
> + return data;
> +}
> +
> +static int pmif_spmi_write(struct udevice *dev, int sid, int pid, int reg,
> u8 val)
> +{
> + struct pmif_bus *pbus = dev_get_priv(dev);
> + struct pmif *arb = pbus->arb;
> + struct ch_reg *inf_reg;
> + int ret;
> + u32 data, cmd;
> +
> + /* Check for argument validation. */
> + if (unlikely(sid & ~0xf)) {
> + dev_err(dev, "exceed the max slv id\n");
> + return -EINVAL;
> + }
> +
> + /* Wait for Software Interface FSM state to be IDLE. */
> + inf_reg = &arb->chan;
> + ret = readl_poll_sleep_timeout(pbus->base +
> arb->data->regs[inf_reg->ch_sta],
> + data, GET_SWINF(data) == SWINF_IDLE,
> + PMIF_DELAY_US, PMIF_TIMEOUT_US);
> + if (ret < 0) {
> + /* set channel ready if the data has transferred */
> + if (pmif_is_fsm_vldclr(arb, pbus))
> + pmif_writel(arb, pbus, 1, inf_reg->ch_rdy);
> + dev_err(dev, "failed to wait for SWINF_IDLE\n");
> + return ret;
> + }
> +
> + pmif_writel(arb, pbus, (u32)val, inf_reg->wdata);
> +
> + /* Send the command. */
> + cmd = (PMIF_CMD_EXT_REG_LONG << 30) | BIT(29) | (sid << 24) | reg;
> + pmif_writel(arb, pbus, cmd, inf_reg->ch_send);
> +
> + return 0;
> +}
> +
> +static const struct pmif_data mt8195_pmif_arb = {
> + .regs = mt8195_regs,
> + .spmimst_regs = mt8195_spmi_regs,
> + .soc_chan = 2,
> +};
> +
> +static const struct pmif_data mt8196_pmif_arb = {
> + .regs = mt8195_regs,
> + .spmimst_regs = mt8195_spmi_regs,
> + .soc_chan = 2,
> + .spmi_ver = 2,
> + .num_spmi_buses = 2,
> +};
> +
> +static int mtk_spmi_probe(struct udevice *dev)
> +{
> + struct pmif_bus *pbus = dev_get_priv(dev);
> + int ret;
> +
> + if (!pbus) {
> + dev_info(dev, "Not probing a non-bus spmi device\n");
> + return 0;
> + }
> +
> + pbus->base = devfdt_remap_addr_name(dev, "pmif");
> + if (IS_ERR(pbus->base))
> + return PTR_ERR(pbus->base);
> +
> + pbus->spmimst_base = devfdt_remap_addr_name(dev, "spmimst");
> + if (IS_ERR(pbus->spmimst_base))
> + return PTR_ERR(pbus->spmimst_base);
devfdt_remap_addr_name() returns NULL when the address cannot be resolved,
not an ERR_PTR(). These checks won't catch that, so probe can succeed with
an invalid MMIO base. Please check for NULL and return an error instead.
Also, spmimst_base is never used after this. Do we need to map it at all?
With the IS_ERR()/PTR_ERR() calls gone, linux/err.h can go too.
> +
> + ret = clk_get_bulk(dev, &pbus->clks);
> + if (ret < 0) {
> + dev_err(dev, "Failed to get clocks\n");
> + return ret;
> + }
> +
> + ret = clk_enable_bulk(&pbus->clks);
> + if (ret) {
> + dev_err(dev, "Failed to enable clocks\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct udevice_id mtk_spmi_match_table[] = {
> + {
> + .compatible = "mediatek,mt8195-spmi",
> + .data = (unsigned long)&mt8195_pmif_arb,
> + }, {
> + .compatible = "mediatek,mt8196-spmi",
> + .data = (unsigned long)&mt8196_pmif_arb,
> + }, {
> + /* sentinel */
> + },
> +};
> +
> +static int mtk_spmi_bind(struct udevice *dev)
> +{
> + struct pmif_data *drv_data = (struct pmif_data
> *)dev_get_driver_data(dev);
> + ofnode child, node = dev_ofnode(dev);
> + int ret = 0;
> +
> + if (!drv_data) {
> + dev_err(dev, "Cannot get drv_data\n");
> + return -EINVAL;
> + }
> +
> + if (ofnode_name_eq(node, "spmi")) {
> + struct pmif_bus *bus = devm_kzalloc(dev, sizeof(struct
> pmif_bus), GFP_KERNEL);
> +
> + if (!bus)
> + return -ENOMEM;
> + bus->arb = dev_get_plat(dev->parent);
> + dev_set_priv(dev, bus);
> + } else {
> + struct pmif *arb;
> + u32 chan_offset;
> +
> + arb = devm_kzalloc(dev, sizeof(struct pmif), GFP_KERNEL);
> + if (!arb)
> + return -ENOMEM;
> + dev_set_plat(dev, arb);
> +
> + arb->data = drv_data;
> + chan_offset = PMIF_CHAN_OFFSET * arb->data->soc_chan;
> + arb->chan.ch_sta = PMIF_SWINF_0_STA + chan_offset;
> + arb->chan.wdata = PMIF_SWINF_0_WDATA_31_0 + chan_offset;
> + arb->chan.rdata = PMIF_SWINF_0_RDATA_31_0 + chan_offset;
> + arb->chan.ch_send = PMIF_SWINF_0_ACC + chan_offset;
> + arb->chan.ch_rdy = PMIF_SWINF_0_VLD_CLR + chan_offset;
> +
> + if (arb->data->num_spmi_buses) {
> + ofnode_for_each_subnode(child, node) {
> + if (!ofnode_name_eq(child, "spmi"))
> + continue;
> +
> + /* Bind the subnode to this driver */
> + ret = device_bind_with_driver_data(dev,
> dev->driver,
> +
> ofnode_get_name(child),
> +
> dev->driver_data,
> + child,
> + NULL);
> + if (ret == -ENODEV)
> + printf("Driver '%s' refuses to bind\n",
> + dev->driver->name);
> +
> + if (ret)
> + printf("Error binding driver '%s':
> %d\n",
> + dev->driver->name, ret);
Please propagate the bind error instead of just printing it. As it stands,
we continue and return success even if a child bus failed to bind.
