Re: [PATCH v3 04/10] spmi: Add MSM PMIC Arbiter SPMI controller
On Wed, Oct 30, 2013 at 11:05:36AM -0700, Stephen Boyd wrote:
> On 10/28, Josh Cartwright wrote:
> > +
> > +/**
> > + * pa_write_data: write 1..4 bytes from buf to pmic-arb's register
> > + * @bc byte-count -1. range: 0..3
> > + * @reg register's address
> > + * @buf buffer to write. length must be bc+1
>
> Missing colon between variable and description.
I'll clean the documentation up a bit and push it all to the
implementation (as suggested in a previous review).
[..]
> > + if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
> > + dev_err(&ctrl->dev,
> > + "pmic-arb supports 1..%d bytes per trans, but:%d
> > requested",
>
> Nitpick: Please replace the colon between but and %d with a space.
>
> > + PMIC_ARB_MAX_TRANS_BYTES, bc+1);
>
> Space around that '+' please.
Sure.
> > + return -EINVAL;
> > + }
> > + dev_dbg(&ctrl->dev,
> > + "op:0x%x sid:%d bc:%d addr:0x%x\n", opc, sid, bc, addr);
> > +
> > + /* Check the opcode */
> > + if (opc >= 0x60 && opc <= 0x7F)
> > + opc = PMIC_ARB_OP_READ;
> > + else if (opc >= 0x20 && opc <= 0x2F)
> > + opc = PMIC_ARB_OP_EXT_READ;
> > + else if (opc >= 0x38 && opc <= 0x3F)
> > + opc = PMIC_ARB_OP_EXT_READL;
> > + else
> > + return -EINVAL;
> > +
> > + cmd = (opc << 27) | ((sid & 0xf) << 20) | (addr << 4) | (bc & 0x7);
> > +
> > + spin_lock_irqsave(&pmic_arb->lock, flags);
> > + pmic_arb_base_write(pmic_arb, PMIC_ARB_CMD(pmic_arb->channel), cmd);
> > + rc = pmic_arb_wait_for_done(ctrl);
> > + if (rc)
> > + goto done;
> > +
> > + /* Read from FIFO, note 'bc' is actually number of bytes minus 1 */
> > + pa_read_data(pmic_arb, buf, PMIC_ARB_RDATA0(pmic_arb->channel)
> > + , min_t(u8, bc, 3));
>
> Nitpick: Weird comma starting a line here.
Okay.
[..]
> > +static int __exit spmi_pmic_arb_remove(struct platform_device *pdev)
>
> __exit shouldn't be here. We want this function in modules.
>
> > +{
> > + struct spmi_controller *ctrl = platform_get_drvdata(pdev);
> > + spmi_controller_remove(ctrl);
> > + return 0;
> > +}
> > +
> > +static struct of_device_id spmi_pmic_arb_match_table[] = {
> > + { .compatible = "qcom,spmi-pmic-arb", },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, spmi_pmic_arb_match_table);
> > +
> > +static struct platform_driver spmi_pmic_arb_driver = {
> > + .probe = spmi_pmic_arb_probe,
> > + .remove = __exit_p(spmi_pmic_arb_remove),
>
> Please drop this __exit_p() usage as well.
>
> > + .driver = {
> > + .name = "spmi_pmic_arb",
> > + .owner = THIS_MODULE,
> > + .of_match_table = spmi_pmic_arb_match_table,
> > + },
> > +};
> > +module_platform_driver(spmi_pmic_arb_driver);
>
> MODULE_LICENSE()
> MODULE_ALIAS()
Yep, will re-add. Not sure why I dropped these...
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 04/10] spmi: Add MSM PMIC Arbiter SPMI controller
On 10/28, Josh Cartwright wrote:
> +
> +/**
> + * pa_write_data: write 1..4 bytes from buf to pmic-arb's register
> + * @bc byte-count -1. range: 0..3
> + * @reg register's address
> + * @buf buffer to write. length must be bc+1
Missing colon between variable and description.
> + */
> +static void
> +pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32 reg, u8 bc)
> +{
> + u32 data = 0;
> + memcpy(&data, buf, (bc & 3) + 1);
> + pmic_arb_base_write(dev, reg, data);
> +}
> +
[...]
> +static int pmic_arb_read_cmd(struct spmi_controller *ctrl,
> + u8 opc, u8 sid, u16 addr, u8 bc, u8 *buf)
> +{
> + struct spmi_pmic_arb_dev *pmic_arb = spmi_controller_get_drvdata(ctrl);
> + unsigned long flags;
> + u32 cmd;
> + int rc;
> +
> + if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
> + dev_err(&ctrl->dev,
> + "pmic-arb supports 1..%d bytes per trans, but:%d
> requested",
Nitpick: Please replace the colon between but and %d with a space.
> + PMIC_ARB_MAX_TRANS_BYTES, bc+1);
Space around that '+' please.
> + return -EINVAL;
> + }
> + dev_dbg(&ctrl->dev,
> + "op:0x%x sid:%d bc:%d addr:0x%x\n", opc, sid, bc, addr);
> +
> + /* Check the opcode */
> + if (opc >= 0x60 && opc <= 0x7F)
> + opc = PMIC_ARB_OP_READ;
> + else if (opc >= 0x20 && opc <= 0x2F)
> + opc = PMIC_ARB_OP_EXT_READ;
> + else if (opc >= 0x38 && opc <= 0x3F)
> + opc = PMIC_ARB_OP_EXT_READL;
> + else
> + return -EINVAL;
> +
> + cmd = (opc << 27) | ((sid & 0xf) << 20) | (addr << 4) | (bc & 0x7);
> +
> + spin_lock_irqsave(&pmic_arb->lock, flags);
> + pmic_arb_base_write(pmic_arb, PMIC_ARB_CMD(pmic_arb->channel), cmd);
> + rc = pmic_arb_wait_for_done(ctrl);
> + if (rc)
> + goto done;
> +
> + /* Read from FIFO, note 'bc' is actually number of bytes minus 1 */
> + pa_read_data(pmic_arb, buf, PMIC_ARB_RDATA0(pmic_arb->channel)
> + , min_t(u8, bc, 3));
Nitpick: Weird comma starting a line here.
> +
> + if (bc > 3)
> + pa_read_data(pmic_arb, buf + 4,
> + PMIC_ARB_RDATA1(pmic_arb->channel), bc - 4);
> +
> +done:
> + spin_unlock_irqrestore(&pmic_arb->lock, flags);
> + return rc;
> +}
> +
[...]
> +static int spmi_pmic_arb_probe(struct platform_device *pdev)
> +{
> + struct spmi_pmic_arb_dev *pa;
> + struct spmi_controller *ctrl;
> + struct resource *res;
> + int err, i;
> +
> + ctrl = spmi_controller_alloc(&pdev->dev, sizeof(*pa));
> + if (!ctrl)
> + return -ENOMEM;
> +
> + pa = spmi_controller_get_drvdata(ctrl);
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
> + pa->base = devm_ioremap_resource(&ctrl->dev, res);
> + if (IS_ERR(pa->base)) {
> + err = PTR_ERR(pa->base);
> + goto err_put_ctrl;
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr");
> + pa->intr = devm_ioremap_resource(&ctrl->dev, res);
> + if (IS_ERR(pa->intr)) {
> + err = PTR_ERR(pa->intr);
> + goto err_put_ctrl;
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cnfg");
> + pa->cnfg = devm_ioremap_resource(&ctrl->dev, res);
> + if (IS_ERR(pa->cnfg)) {
> + err = PTR_ERR(pa->cnfg);
> + goto err_put_ctrl;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
> + pa->mapping_table[i] = readl_relaxed(
> + pa->cnfg + SPMI_MAPPING_TABLE_REG(i));
> +
> + platform_set_drvdata(pdev, ctrl);
> + spin_lock_init(&pa->lock);
> +
> + pa->channel = 0;
> + pa->max_apid = 0;
> + pa->min_apid = PMIC_ARB_MAX_PERIPHS - 1;
That looks backwards. Is this right?
> +
> + ctrl->cmd = pmic_arb_cmd;
> + ctrl->read_cmd = pmic_arb_read_cmd;
> + ctrl->write_cmd = pmic_arb_write_cmd;
> +
> + err = spmi_controller_add(ctrl);
> + if (err)
> + goto err_put_ctrl;
> +
> + dev_dbg(&ctrl->dev, "PMIC Arb Version 0x%x\n",
> + pmic_arb_base_read(pa, PMIC_ARB_VERSION));
> +
> + return 0;
> +
> +err_put_ctrl:
> + spmi_controller_put(ctrl);
> + return err;
> +}
> +
> +static int __exit spmi_pmic_arb_remove(struct platform_device *pdev)
__exit shouldn't be here. We want this function in modules.
> +{
> + struct spmi_controller *ctrl = platform_get_drvdata(pdev);
> + spmi_controller_remove(ctrl);
> + return 0;
> +}
> +
> +static struct of_device_id spmi_pmic_arb_match_table[] = {
> + { .compatible = "qcom,spmi-pmic-arb", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, spmi_pmic_arb_match_table);
> +
> +static struct platform_driver spmi_pmic_arb_driver = {
> + .probe = spmi_pmic_arb_probe,
> + .
[PATCH v3 04/10] spmi: Add MSM PMIC Arbiter SPMI controller
From: Kenneth Heitke
Qualcomm's PMIC Arbiter SPMI controller functions as a bus master and
is used to communication with one or more PMIC (slave) devices on the
SPMI bus. The PMIC Arbiter is actually a hardware wrapper around the
SPMI controller that provides concurrent and autonomous PMIC access
to various entities that need to communicate with the PMIC.
The SPMI controller hardware handles all of the SPMI bus activity (bus
arbitration, sequence start condition, transmission of frames, etc).
This software driver uses the PMIC Arbiter register interface to
initiate command sequences on the SPMI bus. The status register is
read to determine when the command sequence has completed and whether
or not it completed successfully.
Signed-off-by: Kenneth Heitke
Signed-off-by: Josh Cartwright
---
drivers/spmi/Kconfig | 15 ++
drivers/spmi/Makefile| 2 +
drivers/spmi/spmi-pmic-arb.c | 403 +++
3 files changed, 420 insertions(+)
create mode 100644 drivers/spmi/spmi-pmic-arb.c
diff --git a/drivers/spmi/Kconfig b/drivers/spmi/Kconfig
index a03835f..056891d 100644
--- a/drivers/spmi/Kconfig
+++ b/drivers/spmi/Kconfig
@@ -7,3 +7,18 @@ menuconfig SPMI
SPMI (System Power Management Interface) is a two-wire
serial interface between baseband and application processors
and Power Management Integrated Circuits (PMIC).
+
+if SPMI
+
+config SPMI_MSM_PMIC_ARB
+ tristate "Qualcomm MSM SPMI Controller (PMIC Arbiter)"
+ depends on ARCH_MSM
+ help
+ If you say yes to this option, support will be included for the
+ built-in SPMI PMIC Arbiter interface on Qualcomm MSM family
+ processors.
+
+ This is required for communicating with Qualcomm PMICs and
+ other devices that have the SPMI interface.
+
+endif
diff --git a/drivers/spmi/Makefile b/drivers/spmi/Makefile
index 1de1acd..fc75104 100644
--- a/drivers/spmi/Makefile
+++ b/drivers/spmi/Makefile
@@ -2,3 +2,5 @@
# Makefile for kernel SPMI framework.
#
obj-$(CONFIG_SPMI) += spmi.o
+
+obj-$(CONFIG_SPMI_MSM_PMIC_ARB)+= spmi-pmic-arb.o
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
new file mode 100644
index 000..92e1416
--- /dev/null
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -0,0 +1,403 @@
+/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* PMIC Arbiter configuration registers */
+#define PMIC_ARB_VERSION 0x
+#define PMIC_ARB_INT_EN0x0004
+
+/* PMIC Arbiter channel registers */
+#define PMIC_ARB_CMD(N)(0x0800 + (0x80 * (N)))
+#define PMIC_ARB_CONFIG(N) (0x0804 + (0x80 * (N)))
+#define PMIC_ARB_STATUS(N) (0x0808 + (0x80 * (N)))
+#define PMIC_ARB_WDATA0(N) (0x0810 + (0x80 * (N)))
+#define PMIC_ARB_WDATA1(N) (0x0814 + (0x80 * (N)))
+#define PMIC_ARB_RDATA0(N) (0x0818 + (0x80 * (N)))
+#define PMIC_ARB_RDATA1(N) (0x081C + (0x80 * (N)))
+
+/* Interrupt Controller */
+#define SPMI_PIC_OWNER_ACC_STATUS(M, N)(0x + ((32 * (M)) + (4 *
(N
+#define SPMI_PIC_ACC_ENABLE(N) (0x0200 + (4 * (N)))
+#define SPMI_PIC_IRQ_STATUS(N) (0x0600 + (4 * (N)))
+#define SPMI_PIC_IRQ_CLEAR(N) (0x0A00 + (4 * (N)))
+
+/* Mapping Table */
+#define SPMI_MAPPING_TABLE_REG(N) (0x0B00 + (4 * (N)))
+#define SPMI_MAPPING_BIT_INDEX(X) (((X) >> 18) & 0xF)
+#define SPMI_MAPPING_BIT_IS_0_FLAG(X) (((X) >> 17) & 0x1)
+#define SPMI_MAPPING_BIT_IS_0_RESULT(X)(((X) >> 9) & 0xFF)
+#define SPMI_MAPPING_BIT_IS_1_FLAG(X) (((X) >> 8) & 0x1)
+#define SPMI_MAPPING_BIT_IS_1_RESULT(X)(((X) >> 0) & 0xFF)
+
+#define SPMI_MAPPING_TABLE_LEN 255
+#define SPMI_MAPPING_TABLE_TREE_DEPTH 16 /* Maximum of 16-bits */
+
+/* Ownership Table */
+#define SPMI_OWNERSHIP_TABLE_REG(N)(0x0700 + (4 * (N)))
+#define SPMI_OWNERSHIP_PERIPH2OWNER(X) ((X) & 0x7)
+
+/* Channel Status fields */
+enum pmic_arb_chnl_status {
+ PMIC_ARB_STATUS_DONE= (1 << 0),
+ PMIC_ARB_STATUS_FAILURE = (1 << 1),
+ PMIC_ARB_STATUS_DENIED = (1 << 2),
+ PMIC_ARB_STATUS_DROPPED = (1 << 3),
+};
+
+/* Command register fields */
+#define PMIC_ARB_CMD_MAX_BYTE_COUNT8
+
+/* Command Opcodes */
+enum pmic_arb_cmd_op_code {
+ PMIC_ARB_OP_EXT
