Re: [PATCH v10 3/5] spmi: mediatek: Add support for MT6873/8192

Hi,

On Mon, 2021-08-09 at 10:55 -0700, Stephen Boyd wrote:

Quoting Hsin-Hsiung Wang (2021-08-02 23:34:19)

quoted

diff --git a/drivers/spmi/spmi-mtk-pmif.c b/drivers/spmi/spmi-mtk-pmif.c
new file mode 100644
index 000000000000..94c45d46ab0c
--- /dev/null
+++ b/drivers/spmi/spmi-mtk-pmif.c

@@ -0,0 +1,465 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2021 MediaTek Inc.
+
+#include <linux/clk.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of_device.h>

include platform_device.h for the platform device driver that this is.

Thanks for the commit, I will update it in the next patch.

quoted

+#include <linux/spmi.h>
+
+#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   10
+#define PMIF_TIMEOUT_US (10 * 1000)
+
+#define PMIF_CHAN_OFFSET 0x5
+
+#define PMIF_MAX_CLKS  3
+
+#define SPMI_OP_ST_BUSY 1
+
+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;
+};
+
+struct pmif {
+       void __iomem    *base;
+       void __iomem    *spmimst_base;
+       struct ch_reg   chan;
+       struct clk_bulk_data clks[PMIF_MAX_CLKS];
+       u32 nclks;

size_t? Surely 32-bits isn't important.

Thanks. I will use size_t in the next patch.

quoted

+       const struct pmif_data *data;
+};
+
+static const char * const pmif_clock_names[] = {
+       "pmif_sys_ck", "pmif_tmr_ck", "spmimst_clk_mux",
+};

[...]

quoted

+
+static bool pmif_is_fsm_vldclr(struct pmif *arb)
+{
+       u32 reg_rdata;
+
+       reg_rdata = pmif_readl(arb, arb->chan.ch_sta);

Newline here please.

Thanks, I will update it in the next patch.

quoted

+       return GET_SWINF(reg_rdata) == SWINF_WFVLDCLR;
+}
+
+static int pmif_arb_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
+{
+       struct pmif *arb = spmi_controller_get_drvdata(ctrl);
+       u32 rdata, cmd;
+       int ret;
+
+       /* Check the opcode */
+       if (opc < SPMI_CMD_RESET || opc > SPMI_CMD_WAKEUP)
+               return -EINVAL;
+
+       cmd = opc - SPMI_CMD_RESET;
+
+       mtk_spmi_writel(arb, (cmd << 0x4) | sid, SPMI_OP_ST_CTRL);
+       ret = readl_poll_timeout_atomic(arb->spmimst_base + arb->data->spmimst_regs[SPMI_OP_ST_STA],
+                                       rdata, (rdata & SPMI_OP_ST_BUSY) == SPMI_OP_ST_BUSY,
+                                       PMIF_DELAY_US, PMIF_TIMEOUT_US);
+       if (ret < 0)
+               dev_err(&ctrl->dev, "timeout, err = %d\n", ret);
+
+       return ret;
+}
+
+static int pmif_spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
+                             u16 addr, u8 *buf, size_t len)
+{
+       struct pmif *arb = spmi_controller_get_drvdata(ctrl);
+       struct ch_reg *inf_reg;
+       int ret;
+       u32 data, cmd;
+
+       /* Check for argument validation. */
+       if (sid & ~0xf) {
+               dev_err(&ctrl->dev, "exceed the max slv id\n");
+               return -EINVAL;
+       }
+
+       if (len > 4) {
+               dev_err(&ctrl->dev, "pmif supports 1..4 bytes per trans, but:%zu requested", len);

Missing newline

Thanks, I will update it in the next patch.

quoted

+               return -EINVAL;
+       }
+
+       if (opc >= 0x60 && opc <= 0x7f)
+               opc = PMIF_CMD_REG;
+       else if ((opc >= 0x20 && opc <= 0x2f) || (opc >= 0x38 && opc <= 0x3f))
+               opc = PMIF_CMD_EXT_REG_LONG;
+       else
+               return -EINVAL;
+
+       /* Wait for Software Interface FSM state to be IDLE. */
+       inf_reg = &arb->chan;
+       ret = readl_poll_timeout_atomic(arb->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))
+                       pmif_writel(arb, 1, inf_reg->ch_rdy);
+               dev_err(&ctrl->dev, "failed to wait for SWINF_IDLE\n");
+               goto out;
+       }
+
+       /* Send the command. */
+       cmd = (opc << 30) | (sid << 24) | ((len - 1) << 16) | addr;
+       pmif_writel(arb, 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_timeout_atomic(arb->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(&ctrl->dev, "failed to wait for SWINF_WFVLDCLR\n");
+               goto out;
+       }
+
+       data = pmif_readl(arb, inf_reg->rdata);
+       memcpy(buf, &data, len);
+       pmif_writel(arb, 1, inf_reg->ch_rdy);
+
+out:
+       if (ret < 0)
+               return ret;
+
+       return 0;
+}
+
+static int pmif_spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
+                              u16 addr, const u8 *buf, size_t len)
+{
+       struct pmif *arb = spmi_controller_get_drvdata(ctrl);
+       struct ch_reg *inf_reg;
+       int ret;
+       u32 data, cmd;
+
+       if (len > 4) {
+               dev_err(&ctrl->dev, "pmif supports 1..4 bytes per trans, but:%zu requested", len);

Missing newline

Thanks, I will update it in the next patch.

quoted

+               return -EINVAL;
+       }
+
+       /* Check the opcode */
+       if (opc >= 0x40 && opc <= 0x5F)
+               opc = PMIF_CMD_REG;
+       else if ((opc <= 0xF) || (opc >= 0x30 && opc <= 0x37))
+               opc = PMIF_CMD_EXT_REG_LONG;
+       else if (opc >= 0x80)
+               opc = PMIF_CMD_REG_0;
+       else
+               return -EINVAL;
+
+       /* Wait for Software Interface FSM state to be IDLE. */
+       inf_reg = &arb->chan;
+       ret = readl_poll_timeout_atomic(arb->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))
+                       pmif_writel(arb, 1, inf_reg->ch_rdy);
+               dev_err(&ctrl->dev, "failed to wait for SWINF_IDLE\n");
+               goto out;
+       }
+
+       /* Set the write data. */
+       memcpy(&data, buf, len);
+       pmif_writel(arb, data, inf_reg->wdata);
+
+       /* Send the command. */
+       cmd = (opc << 30) | BIT(29) | (sid << 24) | ((len - 1) << 16) | addr;
+       pmif_writel(arb, cmd, inf_reg->ch_send);
+
+out:
+       if (ret < 0)
+               return ret;
+
+       return 0;

Simplify to 

	out:
		return ret;

Thanks, I will update it in the next patch.

quoted

+}
+
+static const struct pmif_data mt6873_pmif_arb = {
+       .regs = mt6873_regs,
+       .spmimst_regs = mt6873_spmi_regs,
+       .soc_chan = 2,
+};
+
+static int mtk_spmi_probe(struct platform_device *pdev)
+{
+       struct pmif *arb;
+       struct spmi_controller *ctrl;
+       int err, i;
+       u32 chan_offset;
+
+       ctrl = spmi_controller_alloc(&pdev->dev, sizeof(*arb));
+       if (!ctrl)
+               return -ENOMEM;
+
+       arb = spmi_controller_get_drvdata(ctrl);
+       arb->data = of_device_get_match_data(&pdev->dev);

Use device_get_match_data() instead please.

Thanks, I will change to use device_get_match_data in the next patch.

quoted

+       if (!arb->data) {
+               err = -EINVAL;
+               dev_err(&pdev->dev, "Cannot get drv_data\n");
+               goto err_put_ctrl;
+       }
+
+       arb->base = devm_platform_ioremap_resource_byname(pdev, "pmif");
+       if (IS_ERR(arb->base)) {
+               err = PTR_ERR(arb->base);
+               dev_err(&pdev->dev, "pmif failed to get the remappped memory\n");

Please drop print as the API already prints errors for every problem.

Thanks, I will update it in the next patch.

quoted

+               goto err_put_ctrl;
+       }
+
+       arb->spmimst_base = devm_platform_ioremap_resource_byname(pdev, "spmimst");
+       if (IS_ERR(arb->spmimst_base)) {
+               err = PTR_ERR(arb->spmimst_base);
+               dev_err(&pdev->dev, "spmimst failed to get the remappped memory\n");

Please drop print as the API already prints errors for every problem.

Thanks, I will update it in the next patch.

quoted

+               goto err_put_ctrl;
+       }
+
+       arb->nclks = ARRAY_SIZE(pmif_clock_names);
+       if (arb->nclks > PMIF_MAX_CLKS) {
+               err = -EINVAL;
+               dev_err(&pdev->dev, "exceed the max clock numbers\n");

Do we really care? The dt schema should be checking this instead of the
driver.

Thanks, I will remove the check in the next patch.

quoted

+               goto err_put_ctrl;
+       }
+
+       for (i = 0; i < arb->nclks; i++)
+               arb->clks[i].id = pmif_clock_names[i];
+