* [PATCH v4 0/2] i2c: Add support for Qcom CCI I2C controller
@ 2018-08-20 6:39 Vinod Koul
2018-08-20 6:39 ` [PATCH v4 1/2] dt-bindings: i2c: Add binding for Qualcomm " Vinod Koul
2018-08-20 6:39 ` [PATCH v4 2/2] i2c: Add Qualcomm CCI I2C driver Vinod Koul
0 siblings, 2 replies; 16+ messages in thread
From: Vinod Koul @ 2018-08-20 6:39 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c
Cc: Bjorn Andersson, linux-arm-msm, Rob Herring, devicetree,
Todor Tomov, Vinod Koul
Hi,
This series adds support for Qualcomm Camera Control Interface (CCI) I2C
controller. This block contains one/two I2C masters and registers these
adapters based on the controller probed on.
Changes in v4:
- Add pm support
- Fix based on Bjorn feedback
Changes in v3:
- Update DT binding per RobH comments
- Add the subnode for bus instances
- Move all compataible data to driver data
- add num_master data and use that to register and use masters
Changes in v2:
- Add support for two adapters
Todor Tomov (2):
dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
i2c: Add Qualcomm CCI I2C driver
.../devicetree/bindings/i2c/i2c-qcom-cci.txt | 83 ++
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-qcom-cci.c | 879 +++++++++++++++++++++
4 files changed, 973 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
create mode 100644 drivers/i2c/busses/i2c-qcom-cci.c
--
2.14.4
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
2018-08-20 6:39 [PATCH v4 0/2] i2c: Add support for Qcom CCI I2C controller Vinod Koul
@ 2018-08-20 6:39 ` Vinod Koul
2018-08-20 18:18 ` Rob Herring
2018-08-20 6:39 ` [PATCH v4 2/2] i2c: Add Qualcomm CCI I2C driver Vinod Koul
1 sibling, 1 reply; 16+ messages in thread
From: Vinod Koul @ 2018-08-20 6:39 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c
Cc: Bjorn Andersson, linux-arm-msm, Rob Herring, devicetree,
Todor Tomov, Vinod Koul
From: Todor Tomov <todor.tomov@linaro.org>
Add DT binding document for Qualcomm Camera Control Interface (CCI)
I2C controller.
Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
.../devicetree/bindings/i2c/i2c-qcom-cci.txt | 83 ++++++++++++++++++++++
1 file changed, 83 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
new file mode 100644
index 000000000000..b7f4240ce5c8
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
@@ -0,0 +1,83 @@
+Qualcomm Camera Control Interface (CCI) I2C controller
+
+PROPERTIES:
+
+- compatible:
+ Usage: required
+ Value type: <string>
+ Definition: must be one of:
+ "qcom,msm-8916-cci"
+ "qcom,msm-8996-cci"
+
+- reg
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: base address CCI I2C controller and length of memory
+ mapped region.
+
+- interrupts:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: specifies the CCI I2C interrupt. The format of the
+ specifier is defined by the binding document describing
+ the node's interrupt parent.
+
+- clocks:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: a list of phandle, should contain an entry for each
+ entries in clock-names.
+
+- clock-names
+ Usage: required
+ Value type: <string>
+ Definition: a list of clock names, must include these entries:
+ "mmss_mmagic_ahb" - on "qcom,msm-8996-cci" only;
+ "camss_top_ahb";
+ "cci_ahb";
+ "cci";
+ "camss_ahb";
+
+- power-domains
+ Usage: required for "qcom,msm-8996-cci"
+ Value type: <prop-encoded-array>
+ Definition:
+
+SUBNODES:
+
+The CCI provides I2C masters for one or two i2c busses, described as
+subdevices named "i2c-bus0" and "i2c-bus1".
+
+PROPERTIES:
+
+- clock-frequency:
+ Usage: optional
+ Value type: <u32>
+ Definition: Desired I2C bus clock frequency in Hz, defaults to 100
+ kHz if omitted.
+
+Example:
+
+ cci@a0c000 {
+ compatible = "qcom,msm-8996-cci";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0xa0c000 0x1000>;
+ interrupts = <GIC_SPI 295 IRQ_TYPE_EDGE_RISING>;
+ clocks = <&mmcc MMSS_MMAGIC_AHB_CLK>,
+ <&mmcc CAMSS_TOP_AHB_CLK>,
+ <&mmcc CAMSS_CCI_AHB_CLK>,
+ <&mmcc CAMSS_CCI_CLK>,
+ <&mmcc CAMSS_AHB_CLK>;
+ clock-names = "mmss_mmagic_ahb",
+ "camss_top_ahb",
+ "cci_ahb",
+ "cci",
+ "camss_ahb";
+ i2c-bus0 {
+ clock-frequency = <400000>;
+ };
+ i2c-bus1 {
+ clock-frequency = <400000>;
+ };
+ };
--
2.14.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] i2c: Add Qualcomm CCI I2C driver
2018-08-20 6:39 [PATCH v4 0/2] i2c: Add support for Qcom CCI I2C controller Vinod Koul
2018-08-20 6:39 ` [PATCH v4 1/2] dt-bindings: i2c: Add binding for Qualcomm " Vinod Koul
@ 2018-08-20 6:39 ` Vinod Koul
2018-08-24 7:30 ` Stephen Boyd
1 sibling, 1 reply; 16+ messages in thread
From: Vinod Koul @ 2018-08-20 6:39 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c
Cc: Bjorn Andersson, linux-arm-msm, Rob Herring, devicetree,
Todor Tomov, Vinod Koul
From: Todor Tomov <todor.tomov@linaro.org>
This commit adds I2C bus support for the Camera Control Interface
(CCI) I2C controller found on the Qualcomm SoC processors. This I2C
controller supports two masters and they are registered to the core.
CCI versions supported in the driver are MSM8916 and MSM8996.
Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-qcom-cci.c | 879 ++++++++++++++++++++++++++++++++++++++
3 files changed, 890 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-qcom-cci.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 4f8df2ec87b1..033958bcdd4b 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -828,6 +828,16 @@ config I2C_PXA_SLAVE
is necessary for systems where the PXA may be a target on the
I2C bus.
+config I2C_QCOM_CCI
+ tristate "Qualcomm Camera Control Interface"
+ depends on ARCH_QCOM
+ help
+ If you say yes to this option, support will be included for the
+ built-in camera control interface on the Qualcomm SoCs.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-qcom-cci.
+
config I2C_QUP
tristate "Qualcomm QUP based I2C controller"
depends on ARCH_QCOM
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 5a869144a0c5..d6e48ec3372a 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -83,6 +83,7 @@ obj-$(CONFIG_I2C_PNX) += i2c-pnx.o
obj-$(CONFIG_I2C_PUV3) += i2c-puv3.o
obj-$(CONFIG_I2C_PXA) += i2c-pxa.o
obj-$(CONFIG_I2C_PXA_PCI) += i2c-pxa-pci.o
+obj-$(CONFIG_I2C_QCOM_CCI) += i2c-qcom-cci.o
obj-$(CONFIG_I2C_QUP) += i2c-qup.o
obj-$(CONFIG_I2C_RIIC) += i2c-riic.o
obj-$(CONFIG_I2C_RK3X) += i2c-rk3x.o
diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
new file mode 100644
index 000000000000..6fd6cecc0ed5
--- /dev/null
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -0,0 +1,879 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
+// Copyright (c) 2017-18 Linaro Limited.
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+#define CCI_HW_VERSION 0x0
+#define CCI_RESET_CMD 0x004
+#define CCI_RESET_CMD_MASK 0x0f73f3f7
+#define CCI_RESET_CMD_M0_MASK 0x000003f1
+#define CCI_RESET_CMD_M1_MASK 0x0003f001
+#define CCI_QUEUE_START 0x008
+#define CCI_HALT_REQ 0x034
+#define CCI_HALT_REQ_I2C_M0_Q0Q1 BIT(0)
+#define CCI_HALT_REQ_I2C_M1_Q0Q1 BIT(1)
+
+#define CCI_I2C_Mm_SCL_CTL(m) (0x100 + 0x100 * (m))
+#define CCI_I2C_Mm_SDA_CTL_0(m) (0x104 + 0x100 * (m))
+#define CCI_I2C_Mm_SDA_CTL_1(m) (0x108 + 0x100 * (m))
+#define CCI_I2C_Mm_SDA_CTL_2(m) (0x10c + 0x100 * (m))
+#define CCI_I2C_Mm_MISC_CTL(m) (0x110 + 0x100 * (m))
+
+#define CCI_I2C_Mm_READ_DATA(m) (0x118 + 0x100 * (m))
+#define CCI_I2C_Mm_READ_BUF_LEVEL(m) (0x11c + 0x100 * (m))
+#define CCI_I2C_Mm_Qn_EXEC_WORD_CNT(m, n) (0x300 + 0x200 * (m) + 0x100 * (n))
+#define CCI_I2C_Mm_Qn_CUR_WORD_CNT(m, n) (0x304 + 0x200 * (m) + 0x100 * (n))
+#define CCI_I2C_Mm_Qn_CUR_CMD(m, n) (0x308 + 0x200 * (m) + 0x100 * (n))
+#define CCI_I2C_Mm_Qn_REPORT_STATUS(m, n) (0x30c + 0x200 * (m) + 0x100 * (n))
+#define CCI_I2C_Mm_Qn_LOAD_DATA(m, n) (0x310 + 0x200 * (m) + 0x100 * (n))
+
+#define CCI_IRQ_GLOBAL_CLEAR_CMD 0xc00
+#define CCI_IRQ_MASK_0 0xc04
+#define CCI_IRQ_MASK_0_I2C_M0_RD_DONE BIT(0)
+#define CCI_IRQ_MASK_0_I2C_M0_Q0_REPORT BIT(4)
+#define CCI_IRQ_MASK_0_I2C_M0_Q1_REPORT BIT(8)
+#define CCI_IRQ_MASK_0_I2C_M1_RD_DONE BIT(12)
+#define CCI_IRQ_MASK_0_I2C_M1_Q0_REPORT BIT(16)
+#define CCI_IRQ_MASK_0_I2C_M1_Q1_REPORT BIT(20)
+#define CCI_IRQ_MASK_0_RST_DONE_ACK BIT(24)
+#define CCI_IRQ_MASK_0_I2C_M0_Q0Q1_HALT_ACK BIT(25)
+#define CCI_IRQ_MASK_0_I2C_M1_Q0Q1_HALT_ACK BIT(26)
+#define CCI_IRQ_MASK_0_I2C_M0_ERROR 0x18000ee6
+#define CCI_IRQ_MASK_0_I2C_M1_ERROR 0x60ee6000
+#define CCI_IRQ_CLEAR_0 0xc08
+#define CCI_IRQ_STATUS_0 0xc0c
+#define CCI_IRQ_STATUS_0_I2C_M0_RD_DONE BIT(0)
+#define CCI_IRQ_STATUS_0_I2C_M0_Q0_REPORT BIT(4)
+#define CCI_IRQ_STATUS_0_I2C_M0_Q1_REPORT BIT(8)
+#define CCI_IRQ_STATUS_0_I2C_M1_RD_DONE BIT(12)
+#define CCI_IRQ_STATUS_0_I2C_M1_Q0_REPORT BIT(16)
+#define CCI_IRQ_STATUS_0_I2C_M1_Q1_REPORT BIT(20)
+#define CCI_IRQ_STATUS_0_RST_DONE_ACK BIT(24)
+#define CCI_IRQ_STATUS_0_I2C_M0_Q0Q1_HALT_ACK BIT(25)
+#define CCI_IRQ_STATUS_0_I2C_M1_Q0Q1_HALT_ACK BIT(26)
+#define CCI_IRQ_STATUS_0_I2C_M0_ERROR 0x18000ee6
+#define CCI_IRQ_STATUS_0_I2C_M1_ERROR 0x60ee6000
+
+#define CCI_TIMEOUT (msecs_to_jiffies(100))
+#define NUM_MASTERS 2
+#define NUM_QUEUES 2
+
+/* Max number of resources + 1 for a NULL terminator */
+#define CCI_RES_MAX 6
+
+enum cci_i2c_cmd {
+ CCI_I2C_SET_PARAM = 1,
+ CCI_I2C_WAIT,
+ CCI_I2C_WAIT_SYNC,
+ CCI_I2C_WAIT_GPIO_EVENT,
+ CCI_I2C_TRIG_I2C_EVENT,
+ CCI_I2C_LOCK,
+ CCI_I2C_UNLOCK,
+ CCI_I2C_REPORT,
+ CCI_I2C_WRITE,
+ CCI_I2C_READ,
+ CCI_I2C_WRITE_DISABLE_P,
+ CCI_I2C_READ_DISABLE_P,
+};
+
+#define CCI_I2C_REPORT_IRQ_EN BIT(8)
+
+enum {
+ I2C_MODE_STANDARD,
+ I2C_MODE_FAST,
+ I2C_MODE_FAST_PLUS,
+};
+
+enum cci_i2c_queue_t {
+ QUEUE_0,
+ QUEUE_1
+};
+
+struct cci_res {
+ char *clock[CCI_RES_MAX];
+ u32 clock_rate[CCI_RES_MAX];
+};
+
+struct hw_params {
+ u16 thigh;
+ u16 tlow;
+ u16 tsu_sto;
+ u16 tsu_sta;
+ u16 thd_dat;
+ u16 thd_sta;
+ u16 tbuf;
+ u8 scl_stretch_en;
+ u16 trdhld;
+ u16 tsp;
+};
+
+struct cci;
+
+struct cci_master {
+ struct i2c_adapter adap;
+ u16 master;
+ u8 mode;
+ int status;
+ bool complete_pending;
+ struct completion irq_complete;
+ struct cci *cci;
+};
+
+struct cci_data {
+ unsigned int num_masters;
+ struct i2c_adapter_quirks quirks;
+ u16 queue_size[NUM_QUEUES];
+ struct cci_res res;
+ struct hw_params params[3];
+};
+
+struct cci {
+ struct device *dev;
+ void __iomem *base;
+ unsigned int irq;
+ const struct cci_data *data;
+ struct clk_bulk_data *clock;
+ int nclocks;
+ struct cci_master master[NUM_MASTERS];
+};
+
+/**
+ * cci_clock_set_rate() - Set clock frequency rates
+ * @nclocks: Number of clocks
+ * @clock: Clock array
+ * @clock_freq: Clock frequency rate array
+ * @dev: Device
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static int cci_clock_set_rate(int nclocks, struct clk_bulk_data *clock,
+ const u32 *clock_freq, struct device *dev)
+{
+ int i, ret;
+ long rate;
+
+ for (i = 0; i < nclocks; i++) {
+ if (clock_freq[i]) {
+ rate = clk_round_rate(clock[i].clk, clock_freq[i]);
+ if (rate < 0) {
+ dev_err(dev, "clk round rate failed: %ld\n",
+ rate);
+ return rate;
+ }
+
+ ret = clk_set_rate(clock[i].clk, clock_freq[i]);
+ if (ret < 0) {
+ dev_err(dev, "clk set rate failed: %d\n", ret);
+ return ret;
+ }
+ }
+ }
+
+ return 0;
+}
+
+static irqreturn_t cci_isr(int irq, void *dev)
+{
+ struct cci *cci = dev;
+ u32 val, reset = 0;
+
+ val = readl(cci->base + CCI_IRQ_STATUS_0);
+ writel(val, cci->base + CCI_IRQ_CLEAR_0);
+ writel(0x1, cci->base + CCI_IRQ_GLOBAL_CLEAR_CMD);
+
+ if (val & CCI_IRQ_STATUS_0_RST_DONE_ACK) {
+ if (cci->master[0].complete_pending) {
+ cci->master[0].complete_pending = false;
+ complete(&cci->master[0].irq_complete);
+ }
+
+ if (cci->master[1].complete_pending) {
+ cci->master[1].complete_pending = false;
+ complete(&cci->master[1].irq_complete);
+ }
+ }
+
+ if (val & CCI_IRQ_STATUS_0_I2C_M0_RD_DONE ||
+ val & CCI_IRQ_STATUS_0_I2C_M0_Q0_REPORT ||
+ val & CCI_IRQ_STATUS_0_I2C_M0_Q1_REPORT) {
+ cci->master[0].status = 0;
+ complete(&cci->master[0].irq_complete);
+ }
+
+ if (val & CCI_IRQ_STATUS_0_I2C_M1_RD_DONE ||
+ val & CCI_IRQ_STATUS_0_I2C_M1_Q0_REPORT ||
+ val & CCI_IRQ_STATUS_0_I2C_M1_Q1_REPORT) {
+ cci->master[1].status = 0;
+ complete(&cci->master[1].irq_complete);
+ }
+
+ if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M0_Q0Q1_HALT_ACK)) {
+ cci->master[0].complete_pending = true;
+ reset = CCI_RESET_CMD_M0_MASK;
+ }
+
+ if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M1_Q0Q1_HALT_ACK)) {
+ cci->master[1].complete_pending = true;
+ reset = CCI_RESET_CMD_M1_MASK;
+ }
+
+ if (unlikely(reset))
+ writel(reset, cci->base + CCI_RESET_CMD);
+
+ if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M0_ERROR)) {
+ dev_err_ratelimited(cci->dev, "Master 0 error 0x%08x\n", val);
+ cci->master[0].status = -EIO;
+ writel(CCI_HALT_REQ_I2C_M0_Q0Q1, cci->base + CCI_HALT_REQ);
+ }
+
+ if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M1_ERROR)) {
+ dev_err_ratelimited(cci->dev, "Master 1 error 0x%08x\n", val);
+ cci->master[1].status = -EIO;
+ writel(CCI_HALT_REQ_I2C_M1_Q0Q1, cci->base + CCI_HALT_REQ);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int cci_halt(struct cci *cci, u8 master_num)
+{
+ struct cci_master *master;
+ unsigned long time;
+ u32 val;
+
+ switch (master_num) {
+ case 0:
+ val = CCI_HALT_REQ_I2C_M0_Q0Q1;
+ master = &cci->master[0];
+ break;
+
+ case 1:
+ val = CCI_HALT_REQ_I2C_M1_Q0Q1;
+ master = &cci->master[1];
+ break;
+
+ default:
+ dev_err(cci->dev, "Invalid master:%d\n", master_num);
+ return -EINVAL;
+ }
+
+ master->complete_pending = true;
+ writel(val, cci->base + CCI_HALT_REQ);
+
+ time = wait_for_completion_timeout(&master->irq_complete, CCI_TIMEOUT);
+ if (!time) {
+ dev_err(cci->dev, "CCI halt timeout\n");
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
+static int cci_reset(struct cci *cci)
+{
+ unsigned long time;
+
+ /*
+ * we reset the whole controller (CCI_RESET_CMD_MASK),here and for
+ * simplicity use master[0].xxx for waiting on it
+ */
+ cci->master[0].complete_pending = true;
+ writel(CCI_RESET_CMD_MASK, cci->base + CCI_RESET_CMD);
+
+ time = wait_for_completion_timeout
+ (&cci->master[0].irq_complete, CCI_TIMEOUT);
+ if (!time) {
+ dev_err(cci->dev, "CCI reset timeout\n");
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
+static int cci_init(struct cci *cci, const struct hw_params *hw)
+{
+ u32 val = CCI_IRQ_MASK_0_I2C_M0_RD_DONE |
+ CCI_IRQ_MASK_0_I2C_M0_Q0_REPORT |
+ CCI_IRQ_MASK_0_I2C_M0_Q1_REPORT |
+ CCI_IRQ_MASK_0_I2C_M1_RD_DONE |
+ CCI_IRQ_MASK_0_I2C_M1_Q0_REPORT |
+ CCI_IRQ_MASK_0_I2C_M1_Q1_REPORT |
+ CCI_IRQ_MASK_0_RST_DONE_ACK |
+ CCI_IRQ_MASK_0_I2C_M0_Q0Q1_HALT_ACK |
+ CCI_IRQ_MASK_0_I2C_M1_Q0Q1_HALT_ACK |
+ CCI_IRQ_MASK_0_I2C_M0_ERROR |
+ CCI_IRQ_MASK_0_I2C_M1_ERROR;
+ int i;
+
+ writel(val, cci->base + CCI_IRQ_MASK_0);
+
+ for (i = 0; i < cci->data->num_masters; i++) {
+ val = hw->thigh << 16 | hw->tlow;
+ writel(val, cci->base + CCI_I2C_Mm_SCL_CTL(i));
+
+ val = hw->tsu_sto << 16 | hw->tsu_sta;
+ writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_0(i));
+
+ val = hw->thd_dat << 16 | hw->thd_sta;
+ writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_1(i));
+
+ val = hw->tbuf;
+ writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_2(i));
+
+ val = hw->scl_stretch_en << 8 | hw->trdhld << 4 | hw->tsp;
+ writel(val, cci->base + CCI_I2C_Mm_MISC_CTL(i));
+ }
+
+ return 0;
+}
+
+static int cci_run_queue(struct cci *cci, u8 master, u8 queue)
+{
+ unsigned long time;
+ u32 val;
+ int ret;
+
+ val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
+ writel(val, cci->base + CCI_I2C_Mm_Qn_EXEC_WORD_CNT(master, queue));
+
+ val = BIT(master * 2 + queue);
+ writel(val, cci->base + CCI_QUEUE_START);
+
+ time = wait_for_completion_timeout
+ (&cci->master[master].irq_complete, CCI_TIMEOUT);
+ if (!time) {
+ dev_err(cci->dev, "master %d queue %d timeout\n",
+ master, queue);
+
+ cci_halt(cci, master);
+
+ return -ETIMEDOUT;
+ }
+
+ ret = cci->master[master].status;
+ if (ret < 0)
+ dev_err(cci->dev, "master %d queue %d error %d\n",
+ master, queue, ret);
+
+ return ret;
+}
+
+static int cci_validate_queue(struct cci *cci, u8 master, u8 queue)
+{
+ u32 val;
+
+ val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
+ if (val == cci->data->queue_size[queue])
+ return -EINVAL;
+
+ if (!val)
+ return val;
+
+ val = CCI_I2C_REPORT | CCI_I2C_REPORT_IRQ_EN;
+ writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+
+ return cci_run_queue(cci, master, queue);
+}
+
+static int cci_i2c_read(struct cci *cci, u16 master,
+ u16 addr, u8 *buf, u16 len)
+{
+ u32 val, words_read, words_exp;
+ u8 queue = QUEUE_1;
+ int i, index = 0, ret;
+ bool first = false;
+
+ /*
+ * Call validate queue to make sure queue is empty before starting.
+ * This is to avoid overflow / underflow of queue.
+ */
+ ret = cci_validate_queue(cci, master, queue);
+ if (ret < 0)
+ return ret;
+
+ val = CCI_I2C_SET_PARAM | (addr & 0x7f) << 4;
+ writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+
+ val = CCI_I2C_READ | len << 4;
+ writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+
+ ret = cci_run_queue(cci, master, queue);
+ if (ret < 0)
+ return ret;
+
+ words_read = readl(cci->base + CCI_I2C_Mm_READ_BUF_LEVEL(master));
+ words_exp = len / 4 + 1;
+ if (words_read != words_exp) {
+ dev_err(cci->dev, "words read = %d, words expected = %d\n",
+ words_read, words_exp);
+ return -EIO;
+ }
+
+ do {
+ val = readl(cci->base + CCI_I2C_Mm_READ_DATA(master));
+
+ for (i = 0; i < 4 && index < len; i++) {
+ if (first) {
+ first = false;
+ continue;
+ }
+ buf[index++] = (val >> (i * 8)) & 0xff;
+ }
+ } while (--words_read);
+
+ return 0;
+}
+
+static int cci_i2c_write(struct cci *cci, u16 master,
+ u16 addr, u8 *buf, u16 len)
+{
+ u8 queue = QUEUE_0;
+ u8 load[12] = { 0 };
+ int i = 0, j, ret;
+ u32 val;
+
+ /*
+ * Call validate queue to make sure queue is empty before starting.
+ * This is to avoid overflow / underflow of queue.
+ */
+ ret = cci_validate_queue(cci, master, queue);
+ if (ret < 0)
+ return ret;
+
+ val = CCI_I2C_SET_PARAM | (addr & 0x7f) << 4;
+ writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+
+ load[i++] = CCI_I2C_WRITE | len << 4;
+
+ for (j = 0; j < len; j++)
+ load[i++] = buf[j];
+
+ for (j = 0; j < i; j += 4) {
+ val = load[j];
+ val |= load[j + 1] << 8;
+ val |= load[j + 2] << 16;
+ val |= load[j + 3] << 24;
+ writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+ }
+
+ val = CCI_I2C_REPORT | CCI_I2C_REPORT_IRQ_EN;
+ writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+
+ return cci_run_queue(cci, master, queue);
+}
+
+static int cci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+{
+ struct cci_master *cci_master = i2c_get_adapdata(adap);
+ struct cci *cci = cci_master->cci;
+ int i, ret;
+
+ ret = pm_runtime_get_sync(cci->dev);
+ if (ret < 0)
+ goto err;
+
+ for (i = 0; i < num; i++) {
+ if (msgs[i].flags & I2C_M_RD)
+ ret = cci_i2c_read(cci, cci_master->master,
+ msgs[i].addr, msgs[i].buf,
+ msgs[i].len);
+ else
+ ret = cci_i2c_write(cci, msgs[i].addr,
+ cci_master->master,
+ msgs[i].buf, msgs[i].len);
+
+ if (ret < 0) {
+ dev_err(cci->dev, "cci i2c xfer error %d", ret);
+ break;
+ }
+ }
+
+ if (!ret)
+ ret = num;
+
+err:
+ pm_runtime_mark_last_busy(cci->dev);
+ pm_runtime_put_autosuspend(cci->dev);
+
+ return ret;
+}
+
+static u32 cci_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C;
+}
+
+static const struct i2c_algorithm cci_algo = {
+ .master_xfer = cci_xfer,
+ .functionality = cci_func,
+};
+
+static int cci_enable_clocks(struct cci *cci)
+{
+ int ret;
+
+ ret = clk_bulk_prepare_enable(cci->nclocks, cci->clock);
+ if (ret < 0)
+ dev_err(cci->dev, "Bulk clock prepare failed: %d\n", ret);
+
+ return ret;
+}
+
+static void cci_disable_clocks(struct cci *cci)
+{
+ clk_bulk_disable_unprepare(cci->nclocks, cci->clock);
+}
+
+#ifdef CONFIG_PM
+static int cci_suspend_runtime(struct device *dev)
+{
+ struct cci *cci = dev_get_drvdata(dev);
+
+ dev_dbg(dev, "Supend invoked\n");
+ cci_disable_clocks(cci);
+ return 0;
+}
+
+static int cci_resume_runtime(struct device *dev)
+{
+ struct cci *cci = dev_get_drvdata(dev);
+
+ dev_dbg(dev, "Resume invoked\n");
+ return cci_enable_clocks(cci);
+}
+#endif
+
+#ifdef CONFIG_PM_SLEEP
+static int cci_suspend(struct device *dev)
+{
+ if (!pm_runtime_suspended(dev))
+ return cci_suspend_runtime(dev);
+
+ return 0;
+}
+
+static int cci_resume(struct device *dev)
+{
+ cci_resume_runtime(dev);
+ pm_runtime_mark_last_busy(dev);
+ pm_request_autosuspend(dev);
+
+ return 0;
+}
+#endif
+
+static const struct dev_pm_ops qcom_cci_pm = {
+ SET_SYSTEM_SLEEP_PM_OPS(cci_suspend, cci_resume)
+ SET_RUNTIME_PM_OPS(cci_suspend_runtime, cci_resume_runtime, NULL)
+};
+
+static int cci_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *of_node;
+ struct resource *r;
+ struct cci *cci;
+ char name[16];
+ int ret = 0, i;
+ u32 val;
+
+ cci = devm_kzalloc(dev, sizeof(*cci), GFP_KERNEL);
+ if (!cci)
+ return -ENOMEM;
+
+ cci->dev = dev;
+ platform_set_drvdata(pdev, cci);
+ cci->data = device_get_match_data(&pdev->dev);
+ if (!cci->data) {
+ dev_err(&pdev->dev, "Driver data is null, abort\n");
+ return -EIO;
+ }
+
+ for (i = 0; i < cci->data->num_masters; i++) {
+ cci->master[i].adap.quirks = &cci->data->quirks;
+ cci->master[i].adap.algo = &cci_algo;
+ cci->master[i].adap.dev.parent = cci->dev;
+ cci->master[i].master = i;
+ cci->master[i].cci = cci;
+ i2c_set_adapdata(&cci->master[i].adap, cci);
+ snprintf(cci->master[i].adap.name,
+ sizeof(cci->master[i].adap.name),
+ "Qualcomm Camera Control Interface: %d", i);
+
+ /* find the child node for i2c-bus as we are on cci node */
+ snprintf(name, sizeof(name), "i2c-bus%d", i);
+ of_node = of_get_child_by_name(dev->of_node, name);
+ if (!of_node) {
+ dev_err(dev, "couldn't find i2cbus child node\n");
+ return -EINVAL;
+ }
+ cci->master[i].adap.dev.of_node = of_node;
+
+ cci->master[i].mode = I2C_MODE_STANDARD;
+ ret = of_property_read_u32(of_node, "clock-frequency", &val);
+ if (!ret) {
+ if (val == 400000)
+ cci->master[i].mode = I2C_MODE_FAST;
+ else if (val == 1000000)
+ cci->master[i].mode = I2C_MODE_FAST_PLUS;
+ }
+
+ init_completion(&cci->master[i].irq_complete);
+ }
+
+ /* Memory */
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ cci->base = devm_ioremap_resource(dev, r);
+ if (IS_ERR(cci->base)) {
+ dev_err(dev, "could not map memory\n");
+ return PTR_ERR(cci->base);
+ }
+
+ /* Interrupt */
+
+ ret = platform_get_irq(pdev, 0);
+ if (ret < 0) {
+ dev_err(dev, "missing IRQ: %d\n", ret);
+ return ret;
+ }
+ cci->irq = ret;
+
+ ret = devm_request_irq(dev, cci->irq, cci_isr,
+ IRQF_TRIGGER_RISING, dev_name(dev), cci);
+ if (ret < 0) {
+ dev_err(dev, "request_irq failed, ret: %d\n", ret);
+ return ret;
+ }
+
+ disable_irq(cci->irq);
+
+ /* Clocks */
+
+ cci->nclocks = 0;
+ while (cci->data->res.clock[cci->nclocks])
+ cci->nclocks++;
+
+ cci->clock = devm_kcalloc(dev, cci->nclocks,
+ sizeof(*cci->clock), GFP_KERNEL);
+ if (!cci->clock)
+ return -ENOMEM;
+
+ for (i = 0; i < cci->nclocks; i++)
+ cci->clock[i].id = cci->data->res.clock[i];
+
+ ret = devm_clk_bulk_get(dev, cci->nclocks, cci->clock);
+ if (ret < 0)
+ return ret;
+
+ ret = cci_clock_set_rate(cci->nclocks, cci->clock,
+ cci->data->res.clock_rate, dev);
+ if (ret < 0)
+ return ret;
+
+ ret = cci_enable_clocks(cci);
+ if (ret < 0)
+ return ret;
+
+ pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
+ pm_runtime_use_autosuspend(dev);
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+
+ val = readl_relaxed(cci->base + CCI_HW_VERSION);
+ dev_dbg(dev, "%s: CCI HW version = 0x%08x", __func__, val);
+
+ enable_irq(cci->irq);
+
+ ret = cci_reset(cci);
+ if (ret < 0)
+ goto error;
+
+ for (i = 0; i < cci->data->num_masters; i++) {
+ ret = cci_init(cci, &cci->data->params[cci->master[i].mode]);
+ if (ret < 0)
+ goto pm_error;
+
+ ret = i2c_add_adapter(&cci->master[i].adap);
+ if (ret < 0)
+ goto error_i2c;
+ }
+
+ return 0;
+
+error_i2c:
+ for (; i >= 0; i--)
+ i2c_del_adapter(&cci->master[i].adap);
+pm_error:
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+error:
+ disable_irq(cci->irq);
+ cci_disable_clocks(cci);
+
+ return ret;
+}
+
+static int cci_remove(struct platform_device *pdev)
+{
+ struct cci *cci = platform_get_drvdata(pdev);
+ int i;
+
+ for (i = 0; i < cci->data->num_masters; i++) {
+ i2c_del_adapter(&cci->master[i].adap);
+ cci_halt(cci, i);
+ }
+
+ disable_irq(cci->irq);
+ pm_runtime_disable(&pdev->dev);
+ pm_runtime_set_suspended(&pdev->dev);
+
+ return 0;
+}
+
+static const struct cci_data cci_8916_data = {
+ .num_masters = 1,
+ .queue_size = { 64, 16 },
+ .quirks = {
+ .max_write_len = 10,
+ .max_read_len = 12,
+ },
+ .res = {
+ .clock = {
+ "camss_top_ahb",
+ "cci_ahb",
+ "camss_ahb",
+ "cci"
+ },
+ .clock_rate = {
+ 0,
+ 80000000,
+ 0,
+ 19200000
+ },
+ },
+ .params = {
+ {
+ /* I2C_MODE_STANDARD */
+ .thigh = 78,
+ .tlow = 114,
+ .tsu_sto = 28,
+ .tsu_sta = 28,
+ .thd_dat = 10,
+ .thd_sta = 77,
+ .tbuf = 118,
+ .scl_stretch_en = 0,
+ .trdhld = 6,
+ .tsp = 1
+ },
+ {
+ /* I2C_MODE_FAST */
+ .thigh = 20,
+ .tlow = 28,
+ .tsu_sto = 21,
+ .tsu_sta = 21,
+ .thd_dat = 13,
+ .thd_sta = 18,
+ .tbuf = 32,
+ .scl_stretch_en = 0,
+ .trdhld = 6,
+ .tsp = 3
+ },
+ },
+};
+
+static const struct cci_data cci_8996_data = {
+ .num_masters = 2,
+ .queue_size = { 64, 16 },
+ .quirks = {
+ .max_write_len = 11,
+ .max_read_len = 12,
+ },
+ .res = {
+ .clock = {
+ "camss_top_ahb",
+ "cci_ahb",
+ "camss_ahb",
+ "cci"
+ },
+ .clock_rate = {
+ 0,
+ 0,
+ 0,
+ 37500000
+ },
+ },
+ .params = {
+ {
+ /* I2C_MODE_STANDARD */
+ .thigh = 201,
+ .tlow = 174,
+ .tsu_sto = 204,
+ .tsu_sta = 231,
+ .thd_dat = 22,
+ .thd_sta = 162,
+ .tbuf = 227,
+ .scl_stretch_en = 0,
+ .trdhld = 6,
+ .tsp = 3
+ },
+ {
+ /* I2C_MODE_FAST */
+ .thigh = 38,
+ .tlow = 56,
+ .tsu_sto = 40,
+ .tsu_sta = 40,
+ .thd_dat = 22,
+ .thd_sta = 35,
+ .tbuf = 62,
+ .scl_stretch_en = 0,
+ .trdhld = 6,
+ .tsp = 3
+ },
+ {
+ /* I2C_MODE_FAST_PLUS */
+ .thigh = 16,
+ .tlow = 22,
+ .tsu_sto = 17,
+ .tsu_sta = 18,
+ .thd_dat = 16,
+ .thd_sta = 15,
+ .tbuf = 24,
+ .scl_stretch_en = 0,
+ .trdhld = 3,
+ .tsp = 3
+ }
+ },
+};
+
+static const struct of_device_id cci_dt_match[] = {
+ { .compatible = "qcom,msm-8916-cci", .data = &cci_8916_data},
+ { .compatible = "qcom,msm-8996-cci", .data = &cci_8996_data},
+ {}
+};
+MODULE_DEVICE_TABLE(of, cci_dt_match);
+
+static struct platform_driver qcom_cci_driver = {
+ .probe = cci_probe,
+ .remove = cci_remove,
+ .driver = {
+ .name = "i2c-qcom-cci",
+ .of_match_table = cci_dt_match,
+ .pm = &qcom_cci_pm,
+ },
+};
+
+module_platform_driver(qcom_cci_driver);
+
+MODULE_DESCRIPTION("Qualcomm Camera Control Interface driver");
+MODULE_AUTHOR("Todor Tomov <todor.tomov@linaro.org>");
+MODULE_LICENSE("GPL v2");
--
2.14.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
2018-08-20 6:39 ` [PATCH v4 1/2] dt-bindings: i2c: Add binding for Qualcomm " Vinod Koul
@ 2018-08-20 18:18 ` Rob Herring
2018-08-21 9:28 ` Vinod
0 siblings, 1 reply; 16+ messages in thread
From: Rob Herring @ 2018-08-20 18:18 UTC (permalink / raw)
To: Vinod Koul
Cc: Wolfram Sang, linux-i2c, Bjorn Andersson, linux-arm-msm,
devicetree, Todor Tomov
On Mon, Aug 20, 2018 at 12:09:52PM +0530, Vinod Koul wrote:
> From: Todor Tomov <todor.tomov@linaro.org>
>
> Add DT binding document for Qualcomm Camera Control Interface (CCI)
> I2C controller.
>
> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> .../devicetree/bindings/i2c/i2c-qcom-cci.txt | 83 ++++++++++++++++++++++
> 1 file changed, 83 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> new file mode 100644
> index 000000000000..b7f4240ce5c8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> @@ -0,0 +1,83 @@
> +Qualcomm Camera Control Interface (CCI) I2C controller
> +
> +PROPERTIES:
> +
> +- compatible:
> + Usage: required
> + Value type: <string>
> + Definition: must be one of:
> + "qcom,msm-8916-cci"
> + "qcom,msm-8996-cci"
I think everywhere else is 'msm8916' and 'msm8996'.
> +
> +- reg
> + Usage: required
> + Value type: <prop-encoded-array>
> + Definition: base address CCI I2C controller and length of memory
> + mapped region.
> +
> +- interrupts:
> + Usage: required
> + Value type: <prop-encoded-array>
> + Definition: specifies the CCI I2C interrupt. The format of the
> + specifier is defined by the binding document describing
> + the node's interrupt parent.
> +
> +- clocks:
> + Usage: required
> + Value type: <prop-encoded-array>
> + Definition: a list of phandle, should contain an entry for each
> + entries in clock-names.
> +
> +- clock-names
> + Usage: required
> + Value type: <string>
> + Definition: a list of clock names, must include these entries:
> + "mmss_mmagic_ahb" - on "qcom,msm-8996-cci" only;
> + "camss_top_ahb";
> + "cci_ahb";
> + "cci";
> + "camss_ahb";
> +
> +- power-domains
> + Usage: required for "qcom,msm-8996-cci"
> + Value type: <prop-encoded-array>
> + Definition:
> +
> +SUBNODES:
> +
> +The CCI provides I2C masters for one or two i2c busses, described as
> +subdevices named "i2c-bus0" and "i2c-bus1".
Use a unit-address and reg property with 0 and 1 here.
With those fixed,
Reviewed-by: Rob Herring <robh@kernel.org>
> +
> +PROPERTIES:
> +
> +- clock-frequency:
> + Usage: optional
> + Value type: <u32>
> + Definition: Desired I2C bus clock frequency in Hz, defaults to 100
> + kHz if omitted.
> +
> +Example:
> +
> + cci@a0c000 {
> + compatible = "qcom,msm-8996-cci";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0xa0c000 0x1000>;
> + interrupts = <GIC_SPI 295 IRQ_TYPE_EDGE_RISING>;
> + clocks = <&mmcc MMSS_MMAGIC_AHB_CLK>,
> + <&mmcc CAMSS_TOP_AHB_CLK>,
> + <&mmcc CAMSS_CCI_AHB_CLK>,
> + <&mmcc CAMSS_CCI_CLK>,
> + <&mmcc CAMSS_AHB_CLK>;
> + clock-names = "mmss_mmagic_ahb",
> + "camss_top_ahb",
> + "cci_ahb",
> + "cci",
> + "camss_ahb";
> + i2c-bus0 {
> + clock-frequency = <400000>;
> + };
> + i2c-bus1 {
> + clock-frequency = <400000>;
> + };
> + };
> --
> 2.14.4
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
2018-08-20 18:18 ` Rob Herring
@ 2018-08-21 9:28 ` Vinod
2018-08-21 13:11 ` Rob Herring
0 siblings, 1 reply; 16+ messages in thread
From: Vinod @ 2018-08-21 9:28 UTC (permalink / raw)
To: Rob Herring
Cc: Wolfram Sang, linux-i2c, Bjorn Andersson, linux-arm-msm,
devicetree, Todor Tomov
On 20-08-18, 13:18, Rob Herring wrote:
> On Mon, Aug 20, 2018 at 12:09:52PM +0530, Vinod Koul wrote:
> > +PROPERTIES:
> > +
> > +- compatible:
> > + Usage: required
> > + Value type: <string>
> > + Definition: must be one of:
> > + "qcom,msm-8916-cci"
> > + "qcom,msm-8996-cci"
>
> I think everywhere else is 'msm8916' and 'msm8996'.
Quick grep told me that is the case, so will update.
> > +SUBNODES:
> > +
> > +The CCI provides I2C masters for one or two i2c busses, described as
> > +subdevices named "i2c-bus0" and "i2c-bus1".
>
> Use a unit-address and reg property with 0 and 1 here.
Am not sure I understood that properly, still learning DT nuisances,
care to elaborate a bit please.
> With those fixed,
>
> Reviewed-by: Rob Herring <robh@kernel.org>
Great, thanks for the review.
--
~Vinod
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
2018-08-21 9:28 ` Vinod
@ 2018-08-21 13:11 ` Rob Herring
2018-08-21 13:12 ` Rob Herring
0 siblings, 1 reply; 16+ messages in thread
From: Rob Herring @ 2018-08-21 13:11 UTC (permalink / raw)
To: Vinod
Cc: Wolfram Sang, Linux I2C, Bjorn Andersson, linux-arm-msm,
devicetree, Todor Tomov
On Tue, Aug 21, 2018 at 4:28 AM Vinod <vkoul@kernel.org> wrote:
>
> On 20-08-18, 13:18, Rob Herring wrote:
> > On Mon, Aug 20, 2018 at 12:09:52PM +0530, Vinod Koul wrote:
>
> > > +PROPERTIES:
> > > +
> > > +- compatible:
> > > + Usage: required
> > > + Value type: <string>
> > > + Definition: must be one of:
> > > + "qcom,msm-8916-cci"
> > > + "qcom,msm-8996-cci"
> >
> > I think everywhere else is 'msm8916' and 'msm8996'.
>
> Quick grep told me that is the case, so will update.
>
> > > +SUBNODES:
> > > +
> > > +The CCI provides I2C masters for one or two i2c busses, described as
> > > +subdevices named "i2c-bus0" and "i2c-bus1".
> >
> > Use a unit-address and reg property with 0 and 1 here.
>
> Am not sure I understood that properly, still learning DT nuisances,
> care to elaborate a bit please.
Node names are supposed to be standard (there's a list in the DT spec)
and i2c-bus is for cases where the controller is not the bus parent.
So you just need it to look like this:
i2c-bus@0 {
reg = <0>;
...
};
i2c-bus@1 {
reg = <1>;
...
};
It's similar to how i2c muxes are done where you have multiple
downstream i2c buses. Following this will enable some i2c bus checks
in dtc (current master, not kernel copy yet) as node names are the
only way
Rob
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
2018-08-21 13:11 ` Rob Herring
@ 2018-08-21 13:12 ` Rob Herring
2018-08-21 16:00 ` Vinod
0 siblings, 1 reply; 16+ messages in thread
From: Rob Herring @ 2018-08-21 13:12 UTC (permalink / raw)
To: Vinod
Cc: Wolfram Sang, Linux I2C, Bjorn Andersson, linux-arm-msm,
devicetree, Todor Tomov
Hit send too soon...
On Tue, Aug 21, 2018 at 8:11 AM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Aug 21, 2018 at 4:28 AM Vinod <vkoul@kernel.org> wrote:
> >
> > On 20-08-18, 13:18, Rob Herring wrote:
> > > On Mon, Aug 20, 2018 at 12:09:52PM +0530, Vinod Koul wrote:
> >
> > > > +PROPERTIES:
> > > > +
> > > > +- compatible:
> > > > + Usage: required
> > > > + Value type: <string>
> > > > + Definition: must be one of:
> > > > + "qcom,msm-8916-cci"
> > > > + "qcom,msm-8996-cci"
> > >
> > > I think everywhere else is 'msm8916' and 'msm8996'.
> >
> > Quick grep told me that is the case, so will update.
> >
> > > > +SUBNODES:
> > > > +
> > > > +The CCI provides I2C masters for one or two i2c busses, described as
> > > > +subdevices named "i2c-bus0" and "i2c-bus1".
> > >
> > > Use a unit-address and reg property with 0 and 1 here.
> >
> > Am not sure I understood that properly, still learning DT nuisances,
> > care to elaborate a bit please.
>
> Node names are supposed to be standard (there's a list in the DT spec)
> and i2c-bus is for cases where the controller is not the bus parent.
> So you just need it to look like this:
>
> i2c-bus@0 {
> reg = <0>;
> ...
> };
>
> i2c-bus@1 {
> reg = <1>;
> ...
> };
>
> It's similar to how i2c muxes are done where you have multiple
> downstream i2c buses. Following this will enable some i2c bus checks
> in dtc (current master, not kernel copy yet) as node names are the
> only way
...we can match i2c buses in a generic way.
Rob
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
2018-08-21 13:12 ` Rob Herring
@ 2018-08-21 16:00 ` Vinod
0 siblings, 0 replies; 16+ messages in thread
From: Vinod @ 2018-08-21 16:00 UTC (permalink / raw)
To: Rob Herring
Cc: Wolfram Sang, Linux I2C, Bjorn Andersson, linux-arm-msm,
devicetree, Todor Tomov
On 21-08-18, 08:12, Rob Herring wrote:
> Hit send too soon...
>
> On Tue, Aug 21, 2018 at 8:11 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Aug 21, 2018 at 4:28 AM Vinod <vkoul@kernel.org> wrote:
> > >
> > > On 20-08-18, 13:18, Rob Herring wrote:
> > > > On Mon, Aug 20, 2018 at 12:09:52PM +0530, Vinod Koul wrote:
> > >
> > > > > +PROPERTIES:
> > > > > +
> > > > > +- compatible:
> > > > > + Usage: required
> > > > > + Value type: <string>
> > > > > + Definition: must be one of:
> > > > > + "qcom,msm-8916-cci"
> > > > > + "qcom,msm-8996-cci"
> > > >
> > > > I think everywhere else is 'msm8916' and 'msm8996'.
> > >
> > > Quick grep told me that is the case, so will update.
> > >
> > > > > +SUBNODES:
> > > > > +
> > > > > +The CCI provides I2C masters for one or two i2c busses, described as
> > > > > +subdevices named "i2c-bus0" and "i2c-bus1".
> > > >
> > > > Use a unit-address and reg property with 0 and 1 here.
> > >
> > > Am not sure I understood that properly, still learning DT nuisances,
> > > care to elaborate a bit please.
> >
> > Node names are supposed to be standard (there's a list in the DT spec)
> > and i2c-bus is for cases where the controller is not the bus parent.
> > So you just need it to look like this:
> >
> > i2c-bus@0 {
> > reg = <0>;
> > ...
> > };
> >
> > i2c-bus@1 {
> > reg = <1>;
> > ...
> > };
> >
> > It's similar to how i2c muxes are done where you have multiple
> > downstream i2c buses. Following this will enable some i2c bus checks
> > in dtc (current master, not kernel copy yet) as node names are the
> > only way
>
> ...we can match i2c buses in a generic way.
I tried i2c-bus@0, but wasn't able to do it properly, let me try again
with this approach. It does sound great to me, will update..
--
~Vinod
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/2] i2c: Add Qualcomm CCI I2C driver
2018-08-20 6:39 ` [PATCH v4 2/2] i2c: Add Qualcomm CCI I2C driver Vinod Koul
@ 2018-08-24 7:30 ` Stephen Boyd
2018-08-24 10:16 ` Vinod
0 siblings, 1 reply; 16+ messages in thread
From: Stephen Boyd @ 2018-08-24 7:30 UTC (permalink / raw)
To: Vinod Koul, Wolfram Sang, linux-i2c
Cc: Bjorn Andersson, linux-arm-msm, Rob Herring, devicetree, Todor Tomov
Quoting Vinod Koul (2018-08-19 23:39:53)
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 4f8df2ec87b1..033958bcdd4b 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -828,6 +828,16 @@ config I2C_PXA_SLAVE
> is necessary for systems where the PXA may be a target on the
> I2C bus.
>
> +config I2C_QCOM_CCI
> + tristate "Qualcomm Camera Control Interface"
> + depends on ARCH_QCOM
Or COMPILE_TEST?
> + help
> + If you say yes to this option, support will be included for the
> + built-in camera control interface on the Qualcomm SoCs.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-qcom-cci.
> +
> config I2C_QUP
> tristate "Qualcomm QUP based I2C controller"
> depends on ARCH_QCOM
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> new file mode 100644
> index 000000000000..6fd6cecc0ed5
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -0,0 +1,879 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
> +// Copyright (c) 2017-18 Linaro Limited.
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/i2c.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#define CCI_HW_VERSION 0x0
> +#define CCI_RESET_CMD 0x004
> +#define CCI_RESET_CMD_MASK 0x0f73f3f7
> +#define CCI_RESET_CMD_M0_MASK 0x000003f1
> +#define CCI_RESET_CMD_M1_MASK 0x0003f001
> +#define CCI_QUEUE_START 0x008
> +#define CCI_HALT_REQ 0x034
> +#define CCI_HALT_REQ_I2C_M0_Q0Q1 BIT(0)
> +#define CCI_HALT_REQ_I2C_M1_Q0Q1 BIT(1)
> +
> +#define CCI_I2C_Mm_SCL_CTL(m) (0x100 + 0x100 * (m))
> +#define CCI_I2C_Mm_SDA_CTL_0(m) (0x104 + 0x100 * (m))
> +#define CCI_I2C_Mm_SDA_CTL_1(m) (0x108 + 0x100 * (m))
> +#define CCI_I2C_Mm_SDA_CTL_2(m) (0x10c + 0x100 * (m))
> +#define CCI_I2C_Mm_MISC_CTL(m) (0x110 + 0x100 * (m))
> +
> +#define CCI_I2C_Mm_READ_DATA(m) (0x118 + 0x100 * (m))
> +#define CCI_I2C_Mm_READ_BUF_LEVEL(m) (0x11c + 0x100 * (m))
> +#define CCI_I2C_Mm_Qn_EXEC_WORD_CNT(m, n) (0x300 + 0x200 * (m) + 0x100 * (n))
> +#define CCI_I2C_Mm_Qn_CUR_WORD_CNT(m, n) (0x304 + 0x200 * (m) + 0x100 * (n))
> +#define CCI_I2C_Mm_Qn_CUR_CMD(m, n) (0x308 + 0x200 * (m) + 0x100 * (n))
> +#define CCI_I2C_Mm_Qn_REPORT_STATUS(m, n) (0x30c + 0x200 * (m) + 0x100 * (n))
> +#define CCI_I2C_Mm_Qn_LOAD_DATA(m, n) (0x310 + 0x200 * (m) + 0x100 * (n))
> +
> +#define CCI_IRQ_GLOBAL_CLEAR_CMD 0xc00
> +#define CCI_IRQ_MASK_0 0xc04
> +#define CCI_IRQ_MASK_0_I2C_M0_RD_DONE BIT(0)
> +#define CCI_IRQ_MASK_0_I2C_M0_Q0_REPORT BIT(4)
> +#define CCI_IRQ_MASK_0_I2C_M0_Q1_REPORT BIT(8)
> +#define CCI_IRQ_MASK_0_I2C_M1_RD_DONE BIT(12)
> +#define CCI_IRQ_MASK_0_I2C_M1_Q0_REPORT BIT(16)
> +#define CCI_IRQ_MASK_0_I2C_M1_Q1_REPORT BIT(20)
> +#define CCI_IRQ_MASK_0_RST_DONE_ACK BIT(24)
> +#define CCI_IRQ_MASK_0_I2C_M0_Q0Q1_HALT_ACK BIT(25)
> +#define CCI_IRQ_MASK_0_I2C_M1_Q0Q1_HALT_ACK BIT(26)
> +#define CCI_IRQ_MASK_0_I2C_M0_ERROR 0x18000ee6
> +#define CCI_IRQ_MASK_0_I2C_M1_ERROR 0x60ee6000
> +#define CCI_IRQ_CLEAR_0 0xc08
> +#define CCI_IRQ_STATUS_0 0xc0c
> +#define CCI_IRQ_STATUS_0_I2C_M0_RD_DONE BIT(0)
> +#define CCI_IRQ_STATUS_0_I2C_M0_Q0_REPORT BIT(4)
> +#define CCI_IRQ_STATUS_0_I2C_M0_Q1_REPORT BIT(8)
> +#define CCI_IRQ_STATUS_0_I2C_M1_RD_DONE BIT(12)
> +#define CCI_IRQ_STATUS_0_I2C_M1_Q0_REPORT BIT(16)
> +#define CCI_IRQ_STATUS_0_I2C_M1_Q1_REPORT BIT(20)
> +#define CCI_IRQ_STATUS_0_RST_DONE_ACK BIT(24)
> +#define CCI_IRQ_STATUS_0_I2C_M0_Q0Q1_HALT_ACK BIT(25)
> +#define CCI_IRQ_STATUS_0_I2C_M1_Q0Q1_HALT_ACK BIT(26)
> +#define CCI_IRQ_STATUS_0_I2C_M0_ERROR 0x18000ee6
> +#define CCI_IRQ_STATUS_0_I2C_M1_ERROR 0x60ee6000
> +
> +#define CCI_TIMEOUT (msecs_to_jiffies(100))
> +#define NUM_MASTERS 2
> +#define NUM_QUEUES 2
> +
> +/* Max number of resources + 1 for a NULL terminator */
> +#define CCI_RES_MAX 6
> +
> +enum cci_i2c_cmd {
Make these #defines so we can easily see what the value is instead of
having to count. Plus, the enum isn't helping anyone figure out that a
command is being used because the enum isn't used anywhere in the code.
> + CCI_I2C_SET_PARAM = 1,
> + CCI_I2C_WAIT,
> + CCI_I2C_WAIT_SYNC,
> + CCI_I2C_WAIT_GPIO_EVENT,
> + CCI_I2C_TRIG_I2C_EVENT,
> + CCI_I2C_LOCK,
> + CCI_I2C_UNLOCK,
> + CCI_I2C_REPORT,
> + CCI_I2C_WRITE,
> + CCI_I2C_READ,
> + CCI_I2C_WRITE_DISABLE_P,
> + CCI_I2C_READ_DISABLE_P,
> +};
> +
> +#define CCI_I2C_REPORT_IRQ_EN BIT(8)
> +
> +enum {
> + I2C_MODE_STANDARD,
> + I2C_MODE_FAST,
> + I2C_MODE_FAST_PLUS,
> +};
> +
> +enum cci_i2c_queue_t {
> + QUEUE_0,
> + QUEUE_1
> +};
> +
> +struct cci_res {
> + char *clock[CCI_RES_MAX];
> + u32 clock_rate[CCI_RES_MAX];
> +};
> +
> +struct hw_params {
> + u16 thigh;
> + u16 tlow;
> + u16 tsu_sto;
> + u16 tsu_sta;
> + u16 thd_dat;
> + u16 thd_sta;
> + u16 tbuf;
> + u8 scl_stretch_en;
> + u16 trdhld;
> + u16 tsp;
Care to spell these out fully? Or at least add kernel documentation so
we know what they actually mean?
> +};
> +
> +struct cci;
> +
> +struct cci_master {
> + struct i2c_adapter adap;
> + u16 master;
> + u8 mode;
> + int status;
> + bool complete_pending;
> + struct completion irq_complete;
Having a bool and a completion is very odd and probably racy. Can you
get rid of the bool and just complete things when they need completing
and wait for them when they need waiting?
> + struct cci *cci;
> +};
> +
> +struct cci_data {
> + unsigned int num_masters;
> + struct i2c_adapter_quirks quirks;
> + u16 queue_size[NUM_QUEUES];
> + struct cci_res res;
> + struct hw_params params[3];
> +};
> +
> +struct cci {
> + struct device *dev;
> + void __iomem *base;
> + unsigned int irq;
> + const struct cci_data *data;
> + struct clk_bulk_data *clock;
> + int nclocks;
> + struct cci_master master[NUM_MASTERS];
> +};
> +
> +/**
> + * cci_clock_set_rate() - Set clock frequency rates
> + * @nclocks: Number of clocks
> + * @clock: Clock array
> + * @clock_freq: Clock frequency rate array
> + * @dev: Device
> + *
> + * Return 0 on success or a negative error code otherwise
> + */
> +static int cci_clock_set_rate(int nclocks, struct clk_bulk_data *clock,
> + const u32 *clock_freq, struct device *dev)
> +{
> + int i, ret;
> + long rate;
> +
> + for (i = 0; i < nclocks; i++) {
> + if (clock_freq[i]) {
> + rate = clk_round_rate(clock[i].clk, clock_freq[i]);
> + if (rate < 0) {
> + dev_err(dev, "clk round rate failed: %ld\n",
> + rate);
> + return rate;
> + }
> +
> + ret = clk_set_rate(clock[i].clk, clock_freq[i]);
> + if (ret < 0) {
> + dev_err(dev, "clk set rate failed: %d\n", ret);
> + return ret;
> + }
> + }
> + }
Hopefully this whole function can go away.
> +
> + return 0;
> +}
> +
> +static irqreturn_t cci_isr(int irq, void *dev)
> +{
> + struct cci *cci = dev;
> + u32 val, reset = 0;
> +
> + val = readl(cci->base + CCI_IRQ_STATUS_0);
> + writel(val, cci->base + CCI_IRQ_CLEAR_0);
> + writel(0x1, cci->base + CCI_IRQ_GLOBAL_CLEAR_CMD);
> +
> + if (val & CCI_IRQ_STATUS_0_RST_DONE_ACK) {
> + if (cci->master[0].complete_pending) {
> + cci->master[0].complete_pending = false;
> + complete(&cci->master[0].irq_complete);
> + }
> +
> + if (cci->master[1].complete_pending) {
> + cci->master[1].complete_pending = false;
> + complete(&cci->master[1].irq_complete);
> + }
> + }
> +
> + if (val & CCI_IRQ_STATUS_0_I2C_M0_RD_DONE ||
> + val & CCI_IRQ_STATUS_0_I2C_M0_Q0_REPORT ||
> + val & CCI_IRQ_STATUS_0_I2C_M0_Q1_REPORT) {
> + cci->master[0].status = 0;
> + complete(&cci->master[0].irq_complete);
> + }
> +
> + if (val & CCI_IRQ_STATUS_0_I2C_M1_RD_DONE ||
> + val & CCI_IRQ_STATUS_0_I2C_M1_Q0_REPORT ||
> + val & CCI_IRQ_STATUS_0_I2C_M1_Q1_REPORT) {
> + cci->master[1].status = 0;
> + complete(&cci->master[1].irq_complete);
> + }
> +
> + if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M0_Q0Q1_HALT_ACK)) {
> + cci->master[0].complete_pending = true;
> + reset = CCI_RESET_CMD_M0_MASK;
> + }
> +
> + if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M1_Q0Q1_HALT_ACK)) {
> + cci->master[1].complete_pending = true;
> + reset = CCI_RESET_CMD_M1_MASK;
> + }
> +
> + if (unlikely(reset))
> + writel(reset, cci->base + CCI_RESET_CMD);
> +
> + if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M0_ERROR)) {
> + dev_err_ratelimited(cci->dev, "Master 0 error 0x%08x\n", val);
> + cci->master[0].status = -EIO;
> + writel(CCI_HALT_REQ_I2C_M0_Q0Q1, cci->base + CCI_HALT_REQ);
> + }
> +
> + if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M1_ERROR)) {
> + dev_err_ratelimited(cci->dev, "Master 1 error 0x%08x\n", val);
> + cci->master[1].status = -EIO;
> + writel(CCI_HALT_REQ_I2C_M1_Q0Q1, cci->base + CCI_HALT_REQ);
> + }
> +
> + return IRQ_HANDLED;
What if none of the bits above were set? We should return IRQ_NONE in
that case so we can detect spurious interrupts.
> +}
> +
> +static int cci_halt(struct cci *cci, u8 master_num)
> +{
> + struct cci_master *master;
> + unsigned long time;
> + u32 val;
> +
> + switch (master_num) {
> + case 0:
> + val = CCI_HALT_REQ_I2C_M0_Q0Q1;
> + master = &cci->master[0];
> + break;
> +
> + case 1:
> + val = CCI_HALT_REQ_I2C_M1_Q0Q1;
> + master = &cci->master[1];
> + break;
> +
> + default:
> + dev_err(cci->dev, "Invalid master:%d\n", master_num);
> + return -EINVAL;
> + }
Why it this a case statement? It could be
if (master_num > 1) {
dev_err(...);
return -EINVAL;
}
val = BIT(master_num);
master = &cci->master[master_num];
> +
> + master->complete_pending = true;
> + writel(val, cci->base + CCI_HALT_REQ);
> +
> + time = wait_for_completion_timeout(&master->irq_complete, CCI_TIMEOUT);
> + if (!time) {
Why not just
if (!wait_for_completion_timeout(...)
? Applies throughout this patch.
> + dev_err(cci->dev, "CCI halt timeout\n");
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +
> +static int cci_reset(struct cci *cci)
> +{
> + unsigned long time;
> +
> + /*
> + * we reset the whole controller (CCI_RESET_CMD_MASK),here and for
Add a space after that close parenthesis.
> + * simplicity use master[0].xxx for waiting on it
> + */
> + cci->master[0].complete_pending = true;
> + writel(CCI_RESET_CMD_MASK, cci->base + CCI_RESET_CMD);
> +
> + time = wait_for_completion_timeout
> + (&cci->master[0].irq_complete, CCI_TIMEOUT);
> + if (!time) {
> + dev_err(cci->dev, "CCI reset timeout\n");
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +
> +static int cci_init(struct cci *cci, const struct hw_params *hw)
> +{
> + u32 val = CCI_IRQ_MASK_0_I2C_M0_RD_DONE |
> + CCI_IRQ_MASK_0_I2C_M0_Q0_REPORT |
> + CCI_IRQ_MASK_0_I2C_M0_Q1_REPORT |
> + CCI_IRQ_MASK_0_I2C_M1_RD_DONE |
> + CCI_IRQ_MASK_0_I2C_M1_Q0_REPORT |
> + CCI_IRQ_MASK_0_I2C_M1_Q1_REPORT |
> + CCI_IRQ_MASK_0_RST_DONE_ACK |
> + CCI_IRQ_MASK_0_I2C_M0_Q0Q1_HALT_ACK |
> + CCI_IRQ_MASK_0_I2C_M1_Q0Q1_HALT_ACK |
> + CCI_IRQ_MASK_0_I2C_M0_ERROR |
> + CCI_IRQ_MASK_0_I2C_M1_ERROR;
> + int i;
> +
> + writel(val, cci->base + CCI_IRQ_MASK_0);
> +
> + for (i = 0; i < cci->data->num_masters; i++) {
> + val = hw->thigh << 16 | hw->tlow;
> + writel(val, cci->base + CCI_I2C_Mm_SCL_CTL(i));
> +
> + val = hw->tsu_sto << 16 | hw->tsu_sta;
> + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_0(i));
> +
> + val = hw->thd_dat << 16 | hw->thd_sta;
> + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_1(i));
> +
> + val = hw->tbuf;
> + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_2(i));
> +
> + val = hw->scl_stretch_en << 8 | hw->trdhld << 4 | hw->tsp;
> + writel(val, cci->base + CCI_I2C_Mm_MISC_CTL(i));
> + }
> +
> + return 0;
> +}
> +
> +static int cci_run_queue(struct cci *cci, u8 master, u8 queue)
> +{
> + unsigned long time;
> + u32 val;
> + int ret;
> +
> + val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
> + writel(val, cci->base + CCI_I2C_Mm_Qn_EXEC_WORD_CNT(master, queue));
> +
> + val = BIT(master * 2 + queue);
> + writel(val, cci->base + CCI_QUEUE_START);
> +
> + time = wait_for_completion_timeout
> + (&cci->master[master].irq_complete, CCI_TIMEOUT);
This line is really long. Make a local variable?
> + if (!time) {
> + dev_err(cci->dev, "master %d queue %d timeout\n",
> + master, queue);
> +
> + cci_halt(cci, master);
> +
> + return -ETIMEDOUT;
> + }
> +
> + ret = cci->master[master].status;
> + if (ret < 0)
> + dev_err(cci->dev, "master %d queue %d error %d\n",
> + master, queue, ret);
> +
> + return ret;
> +}
> +
> +static int cci_validate_queue(struct cci *cci, u8 master, u8 queue)
> +{
> + u32 val;
> +
> + val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
> + if (val == cci->data->queue_size[queue])
> + return -EINVAL;
> +
> + if (!val)
> + return val;
return 0?
> +
> + val = CCI_I2C_REPORT | CCI_I2C_REPORT_IRQ_EN;
> + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> +
> + return cci_run_queue(cci, master, queue);
> +}
> +
> +static int cci_i2c_read(struct cci *cci, u16 master,
> + u16 addr, u8 *buf, u16 len)
> +{
> + u32 val, words_read, words_exp;
> + u8 queue = QUEUE_1;
> + int i, index = 0, ret;
> + bool first = false;
> +
> + /*
> + * Call validate queue to make sure queue is empty before starting.
> + * This is to avoid overflow / underflow of queue.
> + */
> + ret = cci_validate_queue(cci, master, queue);
> + if (ret < 0)
> + return ret;
> +
> + val = CCI_I2C_SET_PARAM | (addr & 0x7f) << 4;
> + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> +
> + val = CCI_I2C_READ | len << 4;
> + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> +
> + ret = cci_run_queue(cci, master, queue);
> + if (ret < 0)
> + return ret;
> +
> + words_read = readl(cci->base + CCI_I2C_Mm_READ_BUF_LEVEL(master));
> + words_exp = len / 4 + 1;
> + if (words_read != words_exp) {
> + dev_err(cci->dev, "words read = %d, words expected = %d\n",
> + words_read, words_exp);
> + return -EIO;
> + }
> +
> + do {
> + val = readl(cci->base + CCI_I2C_Mm_READ_DATA(master));
> +
> + for (i = 0; i < 4 && index < len; i++) {
> + if (first) {
> + first = false;
> + continue;
> + }
> + buf[index++] = (val >> (i * 8)) & 0xff;
Is this some sort of readsl() but where the first byte is ignored?
> + }
> + } while (--words_read);
> +
> + return 0;
> +}
> +
> +static int cci_i2c_write(struct cci *cci, u16 master,
> + u16 addr, u8 *buf, u16 len)
> +{
> + u8 queue = QUEUE_0;
> + u8 load[12] = { 0 };
> + int i = 0, j, ret;
> + u32 val;
> +
> + /*
> + * Call validate queue to make sure queue is empty before starting.
> + * This is to avoid overflow / underflow of queue.
> + */
> + ret = cci_validate_queue(cci, master, queue);
> + if (ret < 0)
> + return ret;
> +
> + val = CCI_I2C_SET_PARAM | (addr & 0x7f) << 4;
> + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> +
> + load[i++] = CCI_I2C_WRITE | len << 4;
Can 'len' really be 16 bits? Because this assignment truncates that very
quickly.
> +
> + for (j = 0; j < len; j++)
Well I guess len can be at most '11', so maybe the type should be u8
instead?
> + load[i++] = buf[j];
> +
> + for (j = 0; j < i; j += 4) {
> + val = load[j];
> + val |= load[j + 1] << 8;
> + val |= load[j + 2] << 16;
> + val |= load[j + 3] << 24;
> + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
Use writesl() instead?
> + }
> +
> + val = CCI_I2C_REPORT | CCI_I2C_REPORT_IRQ_EN;
> + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> +
> + return cci_run_queue(cci, master, queue);
> +}
[...]
> +
> +static int cci_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *of_node;
> + struct resource *r;
> + struct cci *cci;
> + char name[16];
> + int ret = 0, i;
> + u32 val;
> +
> + cci = devm_kzalloc(dev, sizeof(*cci), GFP_KERNEL);
> + if (!cci)
> + return -ENOMEM;
> +
> + cci->dev = dev;
> + platform_set_drvdata(pdev, cci);
> + cci->data = device_get_match_data(&pdev->dev);
> + if (!cci->data) {
> + dev_err(&pdev->dev, "Driver data is null, abort\n");
This would never happen though.
> + return -EIO;
> + }
> +
> + for (i = 0; i < cci->data->num_masters; i++) {
> + cci->master[i].adap.quirks = &cci->data->quirks;
> + cci->master[i].adap.algo = &cci_algo;
> + cci->master[i].adap.dev.parent = cci->dev;
> + cci->master[i].master = i;
> + cci->master[i].cci = cci;
> + i2c_set_adapdata(&cci->master[i].adap, cci);
> + snprintf(cci->master[i].adap.name,
> + sizeof(cci->master[i].adap.name),
> + "Qualcomm Camera Control Interface: %d", i);
> +
> + /* find the child node for i2c-bus as we are on cci node */
> + snprintf(name, sizeof(name), "i2c-bus%d", i);
> + of_node = of_get_child_by_name(dev->of_node, name);
> + if (!of_node) {
> + dev_err(dev, "couldn't find i2cbus child node\n");
> + return -EINVAL;
> + }
> + cci->master[i].adap.dev.of_node = of_node;
> +
> + cci->master[i].mode = I2C_MODE_STANDARD;
> + ret = of_property_read_u32(of_node, "clock-frequency", &val);
> + if (!ret) {
> + if (val == 400000)
> + cci->master[i].mode = I2C_MODE_FAST;
> + else if (val == 1000000)
> + cci->master[i].mode = I2C_MODE_FAST_PLUS;
> + }
> +
> + init_completion(&cci->master[i].irq_complete);
> + }
> +
> + /* Memory */
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + cci->base = devm_ioremap_resource(dev, r);
> + if (IS_ERR(cci->base)) {
> + dev_err(dev, "could not map memory\n");
devm_ioremap_resource() already prints an error. Drop this.
> + return PTR_ERR(cci->base);
> + }
> +
> + /* Interrupt */
> +
> + ret = platform_get_irq(pdev, 0);
> + if (ret < 0) {
> + dev_err(dev, "missing IRQ: %d\n", ret);
> + return ret;
> + }
> + cci->irq = ret;
> +
> + ret = devm_request_irq(dev, cci->irq, cci_isr,
> + IRQF_TRIGGER_RISING, dev_name(dev), cci);
> + if (ret < 0) {
> + dev_err(dev, "request_irq failed, ret: %d\n", ret);
> + return ret;
> + }
> +
> + disable_irq(cci->irq);
Why? Is the irq always triggering or something?
> +
> + /* Clocks */
> +
> + cci->nclocks = 0;
> + while (cci->data->res.clock[cci->nclocks])
> + cci->nclocks++;
> +
> + cci->clock = devm_kcalloc(dev, cci->nclocks,
> + sizeof(*cci->clock), GFP_KERNEL);
> + if (!cci->clock)
> + return -ENOMEM;
> +
> + for (i = 0; i < cci->nclocks; i++)
> + cci->clock[i].id = cci->data->res.clock[i];
> +
> + ret = devm_clk_bulk_get(dev, cci->nclocks, cci->clock);
> + if (ret < 0)
> + return ret;
> +
> + ret = cci_clock_set_rate(cci->nclocks, cci->clock,
> + cci->data->res.clock_rate, dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = cci_enable_clocks(cci);
> + if (ret < 0)
> + return ret;
> +
> + pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> + pm_runtime_use_autosuspend(dev);
> + pm_runtime_set_active(dev);
> + pm_runtime_enable(dev);
> +
> + val = readl_relaxed(cci->base + CCI_HW_VERSION);
> + dev_dbg(dev, "%s: CCI HW version = 0x%08x", __func__, val);
> +
> + enable_irq(cci->irq);
> +
> + ret = cci_reset(cci);
> + if (ret < 0)
> + goto error;
> +
> + for (i = 0; i < cci->data->num_masters; i++) {
> + ret = cci_init(cci, &cci->data->params[cci->master[i].mode]);
> + if (ret < 0)
> + goto pm_error;
> +
> + ret = i2c_add_adapter(&cci->master[i].adap);
> + if (ret < 0)
> + goto error_i2c;
> + }
> +
> + return 0;
> +
> +error_i2c:
> + for (; i >= 0; i--)
> + i2c_del_adapter(&cci->master[i].adap);
> +pm_error:
> + pm_runtime_disable(dev);
> + pm_runtime_set_suspended(dev);
> +error:
> + disable_irq(cci->irq);
> + cci_disable_clocks(cci);
> +
> + return ret;
> +}
> +
> +static int cci_remove(struct platform_device *pdev)
> +{
> + struct cci *cci = platform_get_drvdata(pdev);
> + int i;
> +
> + for (i = 0; i < cci->data->num_masters; i++) {
> + i2c_del_adapter(&cci->master[i].adap);
> + cci_halt(cci, i);
> + }
> +
> + disable_irq(cci->irq);
> + pm_runtime_disable(&pdev->dev);
> + pm_runtime_set_suspended(&pdev->dev);
> +
> + return 0;
> +}
> +
> +static const struct cci_data cci_8916_data = {
> + .num_masters = 1,
> + .queue_size = { 64, 16 },
> + .quirks = {
> + .max_write_len = 10,
> + .max_read_len = 12,
> + },
> + .res = {
> + .clock = {
> + "camss_top_ahb",
> + "cci_ahb",
> + "camss_ahb",
> + "cci"
I guess this is another design where you just want to "get all the clks"
and not care about what they are?
> + },
> + .clock_rate = {
> + 0,
> + 80000000,
> + 0,
> + 19200000
> + },
> + },
> + .params = {
> + {
> + /* I2C_MODE_STANDARD */
> + .thigh = 78,
> + .tlow = 114,
> + .tsu_sto = 28,
> + .tsu_sta = 28,
> + .thd_dat = 10,
> + .thd_sta = 77,
> + .tbuf = 118,
> + .scl_stretch_en = 0,
> + .trdhld = 6,
> + .tsp = 1
> + },
> + {
> + /* I2C_MODE_FAST */
> + .thigh = 20,
> + .tlow = 28,
> + .tsu_sto = 21,
> + .tsu_sta = 21,
> + .thd_dat = 13,
> + .thd_sta = 18,
> + .tbuf = 32,
> + .scl_stretch_en = 0,
> + .trdhld = 6,
> + .tsp = 3
> + },
> + },
> +};
> +
> +static const struct cci_data cci_8996_data = {
> + .num_masters = 2,
> + .queue_size = { 64, 16 },
> + .quirks = {
> + .max_write_len = 11,
> + .max_read_len = 12,
> + },
> + .res = {
> + .clock = {
> + "camss_top_ahb",
> + "cci_ahb",
> + "camss_ahb",
> + "cci"
> + },
> + .clock_rate = {
> + 0,
> + 0,
> + 0,
> + 37500000
Use assigned clock rates from DT instead?
> + },
> + },
> + .params = {
> + {
> + /* I2C_MODE_STANDARD */
> + .thigh = 201,
> + .tlow = 174,
> + .tsu_sto = 204,
> + .tsu_sta = 231,
> + .thd_dat = 22,
> + .thd_sta = 162,
> + .tbuf = 227,
> + .scl_stretch_en = 0,
> + .trdhld = 6,
> + .tsp = 3
> + },
> + {
> + /* I2C_MODE_FAST */
> + .thigh = 38,
> + .tlow = 56,
> + .tsu_sto = 40,
> + .tsu_sta = 40,
> + .thd_dat = 22,
> + .thd_sta = 35,
> + .tbuf = 62,
> + .scl_stretch_en = 0,
> + .trdhld = 6,
> + .tsp = 3
> + },
> + {
> + /* I2C_MODE_FAST_PLUS */
Could be written like [I2C_MODE_FAST_PLUS] = { ... } and then drop the
comments and positional magic.
> + .thigh = 16,
> + .tlow = 22,
> + .tsu_sto = 17,
> + .tsu_sta = 18,
> + .thd_dat = 16,
> + .thd_sta = 15,
> + .tbuf = 24,
> + .scl_stretch_en = 0,
> + .trdhld = 3,
> + .tsp = 3
> + }
> + },
> +};
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/2] i2c: Add Qualcomm CCI I2C driver
2018-08-24 7:30 ` Stephen Boyd
@ 2018-08-24 10:16 ` Vinod
2018-08-27 19:19 ` Stephen Boyd
0 siblings, 1 reply; 16+ messages in thread
From: Vinod @ 2018-08-24 10:16 UTC (permalink / raw)
To: Stephen Boyd
Cc: Wolfram Sang, linux-i2c, Bjorn Andersson, linux-arm-msm,
Rob Herring, devicetree, Todor Tomov
On 24-08-18, 00:30, Stephen Boyd wrote:
> Quoting Vinod Koul (2018-08-19 23:39:53)
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 4f8df2ec87b1..033958bcdd4b 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -828,6 +828,16 @@ config I2C_PXA_SLAVE
> > is necessary for systems where the PXA may be a target on the
> > I2C bus.
> >
> > +config I2C_QCOM_CCI
> > + tristate "Qualcomm Camera Control Interface"
> > + depends on ARCH_QCOM
>
> Or COMPILE_TEST?
Sure
>
> > + help
> > + If you say yes to this option, support will be included for the
> > + built-in camera control interface on the Qualcomm SoCs.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called i2c-qcom-cci.
> > +
> > config I2C_QUP
> > tristate "Qualcomm QUP based I2C controller"
> > depends on ARCH_QCOM
> > diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> > new file mode 100644
> > index 000000000000..6fd6cecc0ed5
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> > @@ -0,0 +1,879 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
> > +// Copyright (c) 2017-18 Linaro Limited.
> > +
> > +#include <linux/clk.h>
> > +#include <linux/completion.h>
> > +#include <linux/i2c.h>
> > +#include <linux/io.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +
> > +#define CCI_HW_VERSION 0x0
> > +#define CCI_RESET_CMD 0x004
> > +#define CCI_RESET_CMD_MASK 0x0f73f3f7
> > +#define CCI_RESET_CMD_M0_MASK 0x000003f1
> > +#define CCI_RESET_CMD_M1_MASK 0x0003f001
> > +#define CCI_QUEUE_START 0x008
> > +#define CCI_HALT_REQ 0x034
> > +#define CCI_HALT_REQ_I2C_M0_Q0Q1 BIT(0)
> > +#define CCI_HALT_REQ_I2C_M1_Q0Q1 BIT(1)
> > +
> > +#define CCI_I2C_Mm_SCL_CTL(m) (0x100 + 0x100 * (m))
> > +#define CCI_I2C_Mm_SDA_CTL_0(m) (0x104 + 0x100 * (m))
> > +#define CCI_I2C_Mm_SDA_CTL_1(m) (0x108 + 0x100 * (m))
> > +#define CCI_I2C_Mm_SDA_CTL_2(m) (0x10c + 0x100 * (m))
> > +#define CCI_I2C_Mm_MISC_CTL(m) (0x110 + 0x100 * (m))
> > +
> > +#define CCI_I2C_Mm_READ_DATA(m) (0x118 + 0x100 * (m))
> > +#define CCI_I2C_Mm_READ_BUF_LEVEL(m) (0x11c + 0x100 * (m))
> > +#define CCI_I2C_Mm_Qn_EXEC_WORD_CNT(m, n) (0x300 + 0x200 * (m) + 0x100 * (n))
> > +#define CCI_I2C_Mm_Qn_CUR_WORD_CNT(m, n) (0x304 + 0x200 * (m) + 0x100 * (n))
> > +#define CCI_I2C_Mm_Qn_CUR_CMD(m, n) (0x308 + 0x200 * (m) + 0x100 * (n))
> > +#define CCI_I2C_Mm_Qn_REPORT_STATUS(m, n) (0x30c + 0x200 * (m) + 0x100 * (n))
> > +#define CCI_I2C_Mm_Qn_LOAD_DATA(m, n) (0x310 + 0x200 * (m) + 0x100 * (n))
> > +
> > +#define CCI_IRQ_GLOBAL_CLEAR_CMD 0xc00
> > +#define CCI_IRQ_MASK_0 0xc04
> > +#define CCI_IRQ_MASK_0_I2C_M0_RD_DONE BIT(0)
> > +#define CCI_IRQ_MASK_0_I2C_M0_Q0_REPORT BIT(4)
> > +#define CCI_IRQ_MASK_0_I2C_M0_Q1_REPORT BIT(8)
> > +#define CCI_IRQ_MASK_0_I2C_M1_RD_DONE BIT(12)
> > +#define CCI_IRQ_MASK_0_I2C_M1_Q0_REPORT BIT(16)
> > +#define CCI_IRQ_MASK_0_I2C_M1_Q1_REPORT BIT(20)
> > +#define CCI_IRQ_MASK_0_RST_DONE_ACK BIT(24)
> > +#define CCI_IRQ_MASK_0_I2C_M0_Q0Q1_HALT_ACK BIT(25)
> > +#define CCI_IRQ_MASK_0_I2C_M1_Q0Q1_HALT_ACK BIT(26)
> > +#define CCI_IRQ_MASK_0_I2C_M0_ERROR 0x18000ee6
> > +#define CCI_IRQ_MASK_0_I2C_M1_ERROR 0x60ee6000
> > +#define CCI_IRQ_CLEAR_0 0xc08
> > +#define CCI_IRQ_STATUS_0 0xc0c
> > +#define CCI_IRQ_STATUS_0_I2C_M0_RD_DONE BIT(0)
> > +#define CCI_IRQ_STATUS_0_I2C_M0_Q0_REPORT BIT(4)
> > +#define CCI_IRQ_STATUS_0_I2C_M0_Q1_REPORT BIT(8)
> > +#define CCI_IRQ_STATUS_0_I2C_M1_RD_DONE BIT(12)
> > +#define CCI_IRQ_STATUS_0_I2C_M1_Q0_REPORT BIT(16)
> > +#define CCI_IRQ_STATUS_0_I2C_M1_Q1_REPORT BIT(20)
> > +#define CCI_IRQ_STATUS_0_RST_DONE_ACK BIT(24)
> > +#define CCI_IRQ_STATUS_0_I2C_M0_Q0Q1_HALT_ACK BIT(25)
> > +#define CCI_IRQ_STATUS_0_I2C_M1_Q0Q1_HALT_ACK BIT(26)
> > +#define CCI_IRQ_STATUS_0_I2C_M0_ERROR 0x18000ee6
> > +#define CCI_IRQ_STATUS_0_I2C_M1_ERROR 0x60ee6000
> > +
> > +#define CCI_TIMEOUT (msecs_to_jiffies(100))
> > +#define NUM_MASTERS 2
> > +#define NUM_QUEUES 2
> > +
> > +/* Max number of resources + 1 for a NULL terminator */
> > +#define CCI_RES_MAX 6
> > +
> > +enum cci_i2c_cmd {
>
> Make these #defines so we can easily see what the value is instead of
> having to count. Plus, the enum isn't helping anyone figure out that a
> command is being used because the enum isn't used anywhere in the code.
OK, will remove the unused ones and define used ones
> > + CCI_I2C_SET_PARAM = 1,
> > + CCI_I2C_WAIT,
> > + CCI_I2C_WAIT_SYNC,
> > + CCI_I2C_WAIT_GPIO_EVENT,
> > + CCI_I2C_TRIG_I2C_EVENT,
> > + CCI_I2C_LOCK,
> > + CCI_I2C_UNLOCK,
> > + CCI_I2C_REPORT,
> > + CCI_I2C_WRITE,
> > + CCI_I2C_READ,
> > + CCI_I2C_WRITE_DISABLE_P,
> > + CCI_I2C_READ_DISABLE_P,
> > +};
> > +
> > +#define CCI_I2C_REPORT_IRQ_EN BIT(8)
> > +
> > +enum {
> > + I2C_MODE_STANDARD,
> > + I2C_MODE_FAST,
> > + I2C_MODE_FAST_PLUS,
> > +};
> > +
> > +enum cci_i2c_queue_t {
> > + QUEUE_0,
> > + QUEUE_1
> > +};
> > +
> > +struct cci_res {
> > + char *clock[CCI_RES_MAX];
> > + u32 clock_rate[CCI_RES_MAX];
> > +};
> > +
> > +struct hw_params {
> > + u16 thigh;
> > + u16 tlow;
> > + u16 tsu_sto;
> > + u16 tsu_sta;
> > + u16 thd_dat;
> > + u16 thd_sta;
> > + u16 tbuf;
> > + u8 scl_stretch_en;
> > + u16 trdhld;
> > + u16 tsp;
>
> Care to spell these out fully? Or at least add kernel documentation so
> we know what they actually mean?
Sure will add, they are the parameters used for i2c bus configuration.
>
> > +};
> > +
> > +struct cci;
> > +
> > +struct cci_master {
> > + struct i2c_adapter adap;
> > + u16 master;
> > + u8 mode;
> > + int status;
> > + bool complete_pending;
> > + struct completion irq_complete;
>
> Having a bool and a completion is very odd and probably racy. Can you
> get rid of the bool and just complete things when they need completing
> and wait for them when they need waiting?
sounds sensible to me, will fix.
>
> > + struct cci *cci;
> > +};
> > +
> > +struct cci_data {
> > + unsigned int num_masters;
> > + struct i2c_adapter_quirks quirks;
> > + u16 queue_size[NUM_QUEUES];
> > + struct cci_res res;
> > + struct hw_params params[3];
> > +};
> > +
> > +struct cci {
> > + struct device *dev;
> > + void __iomem *base;
> > + unsigned int irq;
> > + const struct cci_data *data;
> > + struct clk_bulk_data *clock;
> > + int nclocks;
> > + struct cci_master master[NUM_MASTERS];
> > +};
> > +
> > +/**
> > + * cci_clock_set_rate() - Set clock frequency rates
> > + * @nclocks: Number of clocks
> > + * @clock: Clock array
> > + * @clock_freq: Clock frequency rate array
> > + * @dev: Device
> > + *
> > + * Return 0 on success or a negative error code otherwise
> > + */
> > +static int cci_clock_set_rate(int nclocks, struct clk_bulk_data *clock,
> > + const u32 *clock_freq, struct device *dev)
> > +{
> > + int i, ret;
> > + long rate;
> > +
> > + for (i = 0; i < nclocks; i++) {
> > + if (clock_freq[i]) {
> > + rate = clk_round_rate(clock[i].clk, clock_freq[i]);
> > + if (rate < 0) {
> > + dev_err(dev, "clk round rate failed: %ld\n",
> > + rate);
> > + return rate;
> > + }
> > +
> > + ret = clk_set_rate(clock[i].clk, clock_freq[i]);
> > + if (ret < 0) {
> > + dev_err(dev, "clk set rate failed: %d\n", ret);
> > + return ret;
> > + }
> > + }
> > + }
>
> Hopefully this whole function can go away.
okay will revisit this
> > +
> > + return 0;
> > +}
> > +
> > +static irqreturn_t cci_isr(int irq, void *dev)
> > +{
> > + struct cci *cci = dev;
> > + u32 val, reset = 0;
> > +
> > + val = readl(cci->base + CCI_IRQ_STATUS_0);
> > + writel(val, cci->base + CCI_IRQ_CLEAR_0);
> > + writel(0x1, cci->base + CCI_IRQ_GLOBAL_CLEAR_CMD);
> > +
> > + if (val & CCI_IRQ_STATUS_0_RST_DONE_ACK) {
> > + if (cci->master[0].complete_pending) {
> > + cci->master[0].complete_pending = false;
> > + complete(&cci->master[0].irq_complete);
> > + }
> > +
> > + if (cci->master[1].complete_pending) {
> > + cci->master[1].complete_pending = false;
> > + complete(&cci->master[1].irq_complete);
> > + }
> > + }
> > +
> > + if (val & CCI_IRQ_STATUS_0_I2C_M0_RD_DONE ||
> > + val & CCI_IRQ_STATUS_0_I2C_M0_Q0_REPORT ||
> > + val & CCI_IRQ_STATUS_0_I2C_M0_Q1_REPORT) {
> > + cci->master[0].status = 0;
> > + complete(&cci->master[0].irq_complete);
> > + }
> > +
> > + if (val & CCI_IRQ_STATUS_0_I2C_M1_RD_DONE ||
> > + val & CCI_IRQ_STATUS_0_I2C_M1_Q0_REPORT ||
> > + val & CCI_IRQ_STATUS_0_I2C_M1_Q1_REPORT) {
> > + cci->master[1].status = 0;
> > + complete(&cci->master[1].irq_complete);
> > + }
> > +
> > + if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M0_Q0Q1_HALT_ACK)) {
> > + cci->master[0].complete_pending = true;
> > + reset = CCI_RESET_CMD_M0_MASK;
> > + }
> > +
> > + if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M1_Q0Q1_HALT_ACK)) {
> > + cci->master[1].complete_pending = true;
> > + reset = CCI_RESET_CMD_M1_MASK;
> > + }
> > +
> > + if (unlikely(reset))
> > + writel(reset, cci->base + CCI_RESET_CMD);
> > +
> > + if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M0_ERROR)) {
> > + dev_err_ratelimited(cci->dev, "Master 0 error 0x%08x\n", val);
> > + cci->master[0].status = -EIO;
> > + writel(CCI_HALT_REQ_I2C_M0_Q0Q1, cci->base + CCI_HALT_REQ);
> > + }
> > +
> > + if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M1_ERROR)) {
> > + dev_err_ratelimited(cci->dev, "Master 1 error 0x%08x\n", val);
> > + cci->master[1].status = -EIO;
> > + writel(CCI_HALT_REQ_I2C_M1_Q0Q1, cci->base + CCI_HALT_REQ);
> > + }
> > +
> > + return IRQ_HANDLED;
>
> What if none of the bits above were set? We should return IRQ_NONE in
> that case so we can detect spurious interrupts.
yes will do
>
> > +}
> > +
> > +static int cci_halt(struct cci *cci, u8 master_num)
> > +{
> > + struct cci_master *master;
> > + unsigned long time;
> > + u32 val;
> > +
> > + switch (master_num) {
> > + case 0:
> > + val = CCI_HALT_REQ_I2C_M0_Q0Q1;
> > + master = &cci->master[0];
> > + break;
> > +
> > + case 1:
> > + val = CCI_HALT_REQ_I2C_M1_Q0Q1;
> > + master = &cci->master[1];
> > + break;
> > +
> > + default:
> > + dev_err(cci->dev, "Invalid master:%d\n", master_num);
> > + return -EINVAL;
> > + }
>
> Why it this a case statement? It could be
>
> if (master_num > 1) {
> dev_err(...);
> return -EINVAL;
> }
>
> val = BIT(master_num);
> master = &cci->master[master_num];
yes that can be the alternate implementation.
>
> > +
> > + master->complete_pending = true;
> > + writel(val, cci->base + CCI_HALT_REQ);
> > +
> > + time = wait_for_completion_timeout(&master->irq_complete, CCI_TIMEOUT);
> > + if (!time) {
>
> Why not just
>
> if (!wait_for_completion_timeout(...)
>
> ? Applies throughout this patch.
>
> > + dev_err(cci->dev, "CCI halt timeout\n");
> > + return -ETIMEDOUT;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int cci_reset(struct cci *cci)
> > +{
> > + unsigned long time;
> > +
> > + /*
> > + * we reset the whole controller (CCI_RESET_CMD_MASK),here and for
>
> Add a space after that close parenthesis.
oops missed that
>
> > + * simplicity use master[0].xxx for waiting on it
> > + */
> > + cci->master[0].complete_pending = true;
> > + writel(CCI_RESET_CMD_MASK, cci->base + CCI_RESET_CMD);
> > +
> > + time = wait_for_completion_timeout
> > + (&cci->master[0].irq_complete, CCI_TIMEOUT);
> > + if (!time) {
> > + dev_err(cci->dev, "CCI reset timeout\n");
> > + return -ETIMEDOUT;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int cci_init(struct cci *cci, const struct hw_params *hw)
> > +{
> > + u32 val = CCI_IRQ_MASK_0_I2C_M0_RD_DONE |
> > + CCI_IRQ_MASK_0_I2C_M0_Q0_REPORT |
> > + CCI_IRQ_MASK_0_I2C_M0_Q1_REPORT |
> > + CCI_IRQ_MASK_0_I2C_M1_RD_DONE |
> > + CCI_IRQ_MASK_0_I2C_M1_Q0_REPORT |
> > + CCI_IRQ_MASK_0_I2C_M1_Q1_REPORT |
> > + CCI_IRQ_MASK_0_RST_DONE_ACK |
> > + CCI_IRQ_MASK_0_I2C_M0_Q0Q1_HALT_ACK |
> > + CCI_IRQ_MASK_0_I2C_M1_Q0Q1_HALT_ACK |
> > + CCI_IRQ_MASK_0_I2C_M0_ERROR |
> > + CCI_IRQ_MASK_0_I2C_M1_ERROR;
> > + int i;
> > +
> > + writel(val, cci->base + CCI_IRQ_MASK_0);
> > +
> > + for (i = 0; i < cci->data->num_masters; i++) {
> > + val = hw->thigh << 16 | hw->tlow;
> > + writel(val, cci->base + CCI_I2C_Mm_SCL_CTL(i));
> > +
> > + val = hw->tsu_sto << 16 | hw->tsu_sta;
> > + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_0(i));
> > +
> > + val = hw->thd_dat << 16 | hw->thd_sta;
> > + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_1(i));
> > +
> > + val = hw->tbuf;
> > + writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_2(i));
> > +
> > + val = hw->scl_stretch_en << 8 | hw->trdhld << 4 | hw->tsp;
> > + writel(val, cci->base + CCI_I2C_Mm_MISC_CTL(i));
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int cci_run_queue(struct cci *cci, u8 master, u8 queue)
> > +{
> > + unsigned long time;
> > + u32 val;
> > + int ret;
> > +
> > + val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
> > + writel(val, cci->base + CCI_I2C_Mm_Qn_EXEC_WORD_CNT(master, queue));
> > +
> > + val = BIT(master * 2 + queue);
> > + writel(val, cci->base + CCI_QUEUE_START);
> > +
> > + time = wait_for_completion_timeout
> > + (&cci->master[master].irq_complete, CCI_TIMEOUT);
>
> This line is really long. Make a local variable?
yeah sure
>
> > + if (!time) {
> > + dev_err(cci->dev, "master %d queue %d timeout\n",
> > + master, queue);
> > +
> > + cci_halt(cci, master);
> > +
> > + return -ETIMEDOUT;
> > + }
> > +
> > + ret = cci->master[master].status;
> > + if (ret < 0)
> > + dev_err(cci->dev, "master %d queue %d error %d\n",
> > + master, queue, ret);
> > +
> > + return ret;
> > +}
> > +
> > +static int cci_validate_queue(struct cci *cci, u8 master, u8 queue)
> > +{
> > + u32 val;
> > +
> > + val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
> > + if (val == cci->data->queue_size[queue])
> > + return -EINVAL;
> > +
> > + if (!val)
> > + return val;
>
> return 0?
do you want to make it explicit? both seem to me to do the same thing
> > + val = CCI_I2C_REPORT | CCI_I2C_REPORT_IRQ_EN;
> > + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> > +
> > + return cci_run_queue(cci, master, queue);
> > +}
> > +
> > +static int cci_i2c_read(struct cci *cci, u16 master,
> > + u16 addr, u8 *buf, u16 len)
> > +{
> > + u32 val, words_read, words_exp;
> > + u8 queue = QUEUE_1;
> > + int i, index = 0, ret;
> > + bool first = false;
> > +
> > + /*
> > + * Call validate queue to make sure queue is empty before starting.
> > + * This is to avoid overflow / underflow of queue.
> > + */
> > + ret = cci_validate_queue(cci, master, queue);
> > + if (ret < 0)
> > + return ret;
> > +
> > + val = CCI_I2C_SET_PARAM | (addr & 0x7f) << 4;
> > + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> > +
> > + val = CCI_I2C_READ | len << 4;
> > + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> > +
> > + ret = cci_run_queue(cci, master, queue);
> > + if (ret < 0)
> > + return ret;
> > +
> > + words_read = readl(cci->base + CCI_I2C_Mm_READ_BUF_LEVEL(master));
> > + words_exp = len / 4 + 1;
> > + if (words_read != words_exp) {
> > + dev_err(cci->dev, "words read = %d, words expected = %d\n",
> > + words_read, words_exp);
> > + return -EIO;
> > + }
> > +
> > + do {
> > + val = readl(cci->base + CCI_I2C_Mm_READ_DATA(master));
> > +
> > + for (i = 0; i < 4 && index < len; i++) {
> > + if (first) {
> > + first = false;
> > + continue;
> > + }
> > + buf[index++] = (val >> (i * 8)) & 0xff;
>
> Is this some sort of readsl() but where the first byte is ignored?
Let me check with Todor and get back
>
> > + }
> > + } while (--words_read);
> > +
> > + return 0;
> > +}
> > +
> > +static int cci_i2c_write(struct cci *cci, u16 master,
> > + u16 addr, u8 *buf, u16 len)
> > +{
> > + u8 queue = QUEUE_0;
> > + u8 load[12] = { 0 };
> > + int i = 0, j, ret;
> > + u32 val;
> > +
> > + /*
> > + * Call validate queue to make sure queue is empty before starting.
> > + * This is to avoid overflow / underflow of queue.
> > + */
> > + ret = cci_validate_queue(cci, master, queue);
> > + if (ret < 0)
> > + return ret;
> > +
> > + val = CCI_I2C_SET_PARAM | (addr & 0x7f) << 4;
> > + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> > +
> > + load[i++] = CCI_I2C_WRITE | len << 4;
>
> Can 'len' really be 16 bits? Because this assignment truncates that very
> quickly.
Yes, this is passed from i2c_msg->len which is 16 bytes. But here it
doesn't support so it is truncated here
>
> > +
> > + for (j = 0; j < len; j++)
>
> Well I guess len can be at most '11', so maybe the type should be u8
> instead?
I can use a local variable and cast to it, but am not sure that helps!
> > + load[i++] = buf[j];
> > +
> > + for (j = 0; j < i; j += 4) {
> > + val = load[j];
> > + val |= load[j + 1] << 8;
> > + val |= load[j + 2] << 16;
> > + val |= load[j + 3] << 24;
> > + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
>
> Use writesl() instead?
hmm should be possible. Will try and update.
> > +static int cci_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *of_node;
> > + struct resource *r;
> > + struct cci *cci;
> > + char name[16];
> > + int ret = 0, i;
> > + u32 val;
> > +
> > + cci = devm_kzalloc(dev, sizeof(*cci), GFP_KERNEL);
> > + if (!cci)
> > + return -ENOMEM;
> > +
> > + cci->dev = dev;
> > + platform_set_drvdata(pdev, cci);
> > + cci->data = device_get_match_data(&pdev->dev);
> > + if (!cci->data) {
> > + dev_err(&pdev->dev, "Driver data is null, abort\n");
>
> This would never happen though.
not currently, but when someone adds an entry and misses driver data :)
> > + return -EIO;
> > + }
> > +
> > + for (i = 0; i < cci->data->num_masters; i++) {
> > + cci->master[i].adap.quirks = &cci->data->quirks;
> > + cci->master[i].adap.algo = &cci_algo;
> > + cci->master[i].adap.dev.parent = cci->dev;
> > + cci->master[i].master = i;
> > + cci->master[i].cci = cci;
> > + i2c_set_adapdata(&cci->master[i].adap, cci);
> > + snprintf(cci->master[i].adap.name,
> > + sizeof(cci->master[i].adap.name),
> > + "Qualcomm Camera Control Interface: %d", i);
> > +
> > + /* find the child node for i2c-bus as we are on cci node */
> > + snprintf(name, sizeof(name), "i2c-bus%d", i);
> > + of_node = of_get_child_by_name(dev->of_node, name);
> > + if (!of_node) {
> > + dev_err(dev, "couldn't find i2cbus child node\n");
> > + return -EINVAL;
> > + }
> > + cci->master[i].adap.dev.of_node = of_node;
> > +
> > + cci->master[i].mode = I2C_MODE_STANDARD;
> > + ret = of_property_read_u32(of_node, "clock-frequency", &val);
> > + if (!ret) {
> > + if (val == 400000)
> > + cci->master[i].mode = I2C_MODE_FAST;
> > + else if (val == 1000000)
> > + cci->master[i].mode = I2C_MODE_FAST_PLUS;
> > + }
> > +
> > + init_completion(&cci->master[i].irq_complete);
> > + }
> > +
> > + /* Memory */
> > +
> > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + cci->base = devm_ioremap_resource(dev, r);
> > + if (IS_ERR(cci->base)) {
> > + dev_err(dev, "could not map memory\n");
>
> devm_ioremap_resource() already prints an error. Drop this.
sure
> > + return PTR_ERR(cci->base);
> > + }
> > +
> > + /* Interrupt */
> > +
> > + ret = platform_get_irq(pdev, 0);
> > + if (ret < 0) {
> > + dev_err(dev, "missing IRQ: %d\n", ret);
> > + return ret;
> > + }
> > + cci->irq = ret;
> > +
> > + ret = devm_request_irq(dev, cci->irq, cci_isr,
> > + IRQF_TRIGGER_RISING, dev_name(dev), cci);
> > + if (ret < 0) {
> > + dev_err(dev, "request_irq failed, ret: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + disable_irq(cci->irq);
>
> Why? Is the irq always triggering or something?
I supposed Todor didn't want to enable IRQ until everything is set.
I could move this block before adding i2c adapter.
> > +static const struct cci_data cci_8916_data = {
> > + .num_masters = 1,
> > + .queue_size = { 64, 16 },
> > + .quirks = {
> > + .max_write_len = 10,
> > + .max_read_len = 12,
> > + },
> > + .res = {
> > + .clock = {
> > + "camss_top_ahb",
> > + "cci_ahb",
> > + "camss_ahb",
> > + "cci"
>
> I guess this is another design where you just want to "get all the clks"
> and not care about what they are?
Yes that is how this seems to be
> > +static const struct cci_data cci_8996_data = {
> > + .num_masters = 2,
> > + .queue_size = { 64, 16 },
> > + .quirks = {
> > + .max_write_len = 11,
> > + .max_read_len = 12,
> > + },
> > + .res = {
> > + .clock = {
> > + "camss_top_ahb",
> > + "cci_ahb",
> > + "camss_ahb",
> > + "cci"
> > + },
> > + .clock_rate = {
> > + 0,
> > + 0,
> > + 0,
> > + 37500000
>
> Use assigned clock rates from DT instead?
Yes should be possible.
> > + },
> > + },
> > + .params = {
> > + {
> > + /* I2C_MODE_STANDARD */
> > + .thigh = 201,
> > + .tlow = 174,
> > + .tsu_sto = 204,
> > + .tsu_sta = 231,
> > + .thd_dat = 22,
> > + .thd_sta = 162,
> > + .tbuf = 227,
> > + .scl_stretch_en = 0,
> > + .trdhld = 6,
> > + .tsp = 3
> > + },
> > + {
> > + /* I2C_MODE_FAST */
> > + .thigh = 38,
> > + .tlow = 56,
> > + .tsu_sto = 40,
> > + .tsu_sta = 40,
> > + .thd_dat = 22,
> > + .thd_sta = 35,
> > + .tbuf = 62,
> > + .scl_stretch_en = 0,
> > + .trdhld = 6,
> > + .tsp = 3
> > + },
> > + {
> > + /* I2C_MODE_FAST_PLUS */
>
> Could be written like [I2C_MODE_FAST_PLUS] = { ... } and then drop the
> comments and positional magic.
Agreed
--
~Vinod
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/2] i2c: Add Qualcomm CCI I2C driver
2018-08-24 10:16 ` Vinod
@ 2018-08-27 19:19 ` Stephen Boyd
2018-08-28 3:38 ` Vinod
0 siblings, 1 reply; 16+ messages in thread
From: Stephen Boyd @ 2018-08-27 19:19 UTC (permalink / raw)
To: Vinod
Cc: Wolfram Sang, linux-i2c, Bjorn Andersson, linux-arm-msm,
Rob Herring, devicetree, Todor Tomov
Quoting Vinod (2018-08-24 03:16:35)
> On 24-08-18, 00:30, Stephen Boyd wrote:
> > Quoting Vinod Koul (2018-08-19 23:39:53)
> >
> > > + if (!time) {
> > > + dev_err(cci->dev, "master %d queue %d timeout\n",
> > > + master, queue);
> > > +
> > > + cci_halt(cci, master);
> > > +
> > > + return -ETIMEDOUT;
> > > + }
> > > +
> > > + ret = cci->master[master].status;
> > > + if (ret < 0)
> > > + dev_err(cci->dev, "master %d queue %d error %d\n",
> > > + master, queue, ret);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int cci_validate_queue(struct cci *cci, u8 master, u8 queue)
> > > +{
> > > + u32 val;
> > > +
> > > + val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
> > > + if (val == cci->data->queue_size[queue])
> > > + return -EINVAL;
> > > +
> > > + if (!val)
> > > + return val;
> >
> > return 0?
>
> do you want to make it explicit? both seem to me to do the same thing
Yes, please.
> >
> > > + }
> > > + } while (--words_read);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int cci_i2c_write(struct cci *cci, u16 master,
> > > + u16 addr, u8 *buf, u16 len)
> > > +{
> > > + u8 queue = QUEUE_0;
> > > + u8 load[12] = { 0 };
> > > + int i = 0, j, ret;
> > > + u32 val;
> > > +
> > > + /*
> > > + * Call validate queue to make sure queue is empty before starting.
> > > + * This is to avoid overflow / underflow of queue.
> > > + */
> > > + ret = cci_validate_queue(cci, master, queue);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + val = CCI_I2C_SET_PARAM | (addr & 0x7f) << 4;
> > > + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> > > +
> > > + load[i++] = CCI_I2C_WRITE | len << 4;
> >
> > Can 'len' really be 16 bits? Because this assignment truncates that very
> > quickly.
>
> Yes, this is passed from i2c_msg->len which is 16 bytes. But here it
> doesn't support so it is truncated here
>
> >
> > > +
> > > + for (j = 0; j < len; j++)
> >
> > Well I guess len can be at most '11', so maybe the type should be u8
> > instead?
>
> I can use a local variable and cast to it, but am not sure that helps!
Maybe just a comment indicating that max len is 11?
>
> > > + return PTR_ERR(cci->base);
> > > + }
> > > +
> > > + /* Interrupt */
> > > +
> > > + ret = platform_get_irq(pdev, 0);
> > > + if (ret < 0) {
> > > + dev_err(dev, "missing IRQ: %d\n", ret);
> > > + return ret;
> > > + }
> > > + cci->irq = ret;
> > > +
> > > + ret = devm_request_irq(dev, cci->irq, cci_isr,
> > > + IRQF_TRIGGER_RISING, dev_name(dev), cci);
> > > + if (ret < 0) {
> > > + dev_err(dev, "request_irq failed, ret: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + disable_irq(cci->irq);
> >
> > Why? Is the irq always triggering or something?
>
> I supposed Todor didn't want to enable IRQ until everything is set.
> I could move this block before adding i2c adapter.
But does it actually matter? The interrupt is "enabled" from request to
this function call so it's still possible to trigger. I'd just remove
the irq disabling unless it actually matters.
>
> > > +static const struct cci_data cci_8916_data = {
> > > + .num_masters = 1,
> > > + .queue_size = { 64, 16 },
> > > + .quirks = {
> > > + .max_write_len = 10,
> > > + .max_read_len = 12,
> > > + },
> > > + .res = {
> > > + .clock = {
> > > + "camss_top_ahb",
> > > + "cci_ahb",
> > > + "camss_ahb",
> > > + "cci"
> >
> > I guess this is another design where you just want to "get all the clks"
> > and not care about what they are?
>
> Yes that is how this seems to be
Ok. I'm going to merge the "get all the clks" API soon. Maybe you can
use that here.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/2] i2c: Add Qualcomm CCI I2C driver
2018-08-27 19:19 ` Stephen Boyd
@ 2018-08-28 3:38 ` Vinod
2018-09-20 19:39 ` Ricardo Ribalda Delgado
0 siblings, 1 reply; 16+ messages in thread
From: Vinod @ 2018-08-28 3:38 UTC (permalink / raw)
To: Stephen Boyd
Cc: Wolfram Sang, linux-i2c, Bjorn Andersson, linux-arm-msm,
Rob Herring, devicetree, Todor Tomov
On 27-08-18, 12:19, Stephen Boyd wrote:
> Quoting Vinod (2018-08-24 03:16:35)
> > On 24-08-18, 00:30, Stephen Boyd wrote:
> > > Quoting Vinod Koul (2018-08-19 23:39:53)
> > > > +static int cci_i2c_write(struct cci *cci, u16 master,
> > > > + u16 addr, u8 *buf, u16 len)
> > > > +{
> > > > + u8 queue = QUEUE_0;
> > > > + u8 load[12] = { 0 };
> > > > + int i = 0, j, ret;
> > > > + u32 val;
> > > > +
> > > > + /*
> > > > + * Call validate queue to make sure queue is empty before starting.
> > > > + * This is to avoid overflow / underflow of queue.
> > > > + */
> > > > + ret = cci_validate_queue(cci, master, queue);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + val = CCI_I2C_SET_PARAM | (addr & 0x7f) << 4;
> > > > + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> > > > +
> > > > + load[i++] = CCI_I2C_WRITE | len << 4;
> > >
> > > Can 'len' really be 16 bits? Because this assignment truncates that very
> > > quickly.
> >
> > Yes, this is passed from i2c_msg->len which is 16 bytes. But here it
> > doesn't support so it is truncated here
> >
> > >
> > > > +
> > > > + for (j = 0; j < len; j++)
> > >
> > > Well I guess len can be at most '11', so maybe the type should be u8
> > > instead?
> >
> > I can use a local variable and cast to it, but am not sure that helps!
>
> Maybe just a comment indicating that max len is 11?
Sure, will add
> > > > + return PTR_ERR(cci->base);
> > > > + }
> > > > +
> > > > + /* Interrupt */
> > > > +
> > > > + ret = platform_get_irq(pdev, 0);
> > > > + if (ret < 0) {
> > > > + dev_err(dev, "missing IRQ: %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > + cci->irq = ret;
> > > > +
> > > > + ret = devm_request_irq(dev, cci->irq, cci_isr,
> > > > + IRQF_TRIGGER_RISING, dev_name(dev), cci);
> > > > + if (ret < 0) {
> > > > + dev_err(dev, "request_irq failed, ret: %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + disable_irq(cci->irq);
> > >
> > > Why? Is the irq always triggering or something?
> >
> > I supposed Todor didn't want to enable IRQ until everything is set.
> > I could move this block before adding i2c adapter.
>
> But does it actually matter? The interrupt is "enabled" from request to
> this function call so it's still possible to trigger. I'd just remove
> the irq disabling unless it actually matters.
Agreed, will do that
> > > > +static const struct cci_data cci_8916_data = {
> > > > + .num_masters = 1,
> > > > + .queue_size = { 64, 16 },
> > > > + .quirks = {
> > > > + .max_write_len = 10,
> > > > + .max_read_len = 12,
> > > > + },
> > > > + .res = {
> > > > + .clock = {
> > > > + "camss_top_ahb",
> > > > + "cci_ahb",
> > > > + "camss_ahb",
> > > > + "cci"
> > >
> > > I guess this is another design where you just want to "get all the clks"
> > > and not care about what they are?
> >
> > Yes that is how this seems to be
>
> Ok. I'm going to merge the "get all the clks" API soon. Maybe you can
> use that here.
Sure, once merged I will send a patch for this driver, which I guess
would be after next merge window..
--
~Vinod
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/2] i2c: Add Qualcomm CCI I2C driver
2018-08-28 3:38 ` Vinod
@ 2018-09-20 19:39 ` Ricardo Ribalda Delgado
2018-09-21 15:36 ` Vinod
0 siblings, 1 reply; 16+ messages in thread
From: Ricardo Ribalda Delgado @ 2018-09-20 19:39 UTC (permalink / raw)
To: Vinod Koul
Cc: swboyd, wsa+renesas, linux-i2c, bjorn.andersson, linux-arm-msm,
Rob Herring, devicetree, Todor Tomov
Hi
Sorry to enter the discussion this late.
I have been using this driver with release/qcomlt-4.14 on a db820c and
if I try to use the i2c bus before camss is running I get a Timeout
error and the bus is unusable after that. I have sent a patch that
fixes this issue, in case you want to merge it/add it before this
driver goes upstream.
Is there a way to use the i2c bus without using the camera?
Cheers
On Tue, Aug 28, 2018 at 5:39 AM Vinod <vkoul@kernel.org> wrote:
>
> On 27-08-18, 12:19, Stephen Boyd wrote:
> > Quoting Vinod (2018-08-24 03:16:35)
> > > On 24-08-18, 00:30, Stephen Boyd wrote:
> > > > Quoting Vinod Koul (2018-08-19 23:39:53)
>
> > > > > +static int cci_i2c_write(struct cci *cci, u16 master,
> > > > > + u16 addr, u8 *buf, u16 len)
> > > > > +{
> > > > > + u8 queue = QUEUE_0;
> > > > > + u8 load[12] = { 0 };
> > > > > + int i = 0, j, ret;
> > > > > + u32 val;
> > > > > +
> > > > > + /*
> > > > > + * Call validate queue to make sure queue is empty before starting.
> > > > > + * This is to avoid overflow / underflow of queue.
> > > > > + */
> > > > > + ret = cci_validate_queue(cci, master, queue);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > +
> > > > > + val = CCI_I2C_SET_PARAM | (addr & 0x7f) << 4;
> > > > > + writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> > > > > +
> > > > > + load[i++] = CCI_I2C_WRITE | len << 4;
> > > >
> > > > Can 'len' really be 16 bits? Because this assignment truncates that very
> > > > quickly.
> > >
> > > Yes, this is passed from i2c_msg->len which is 16 bytes. But here it
> > > doesn't support so it is truncated here
> > >
> > > >
> > > > > +
> > > > > + for (j = 0; j < len; j++)
> > > >
> > > > Well I guess len can be at most '11', so maybe the type should be u8
> > > > instead?
> > >
> > > I can use a local variable and cast to it, but am not sure that helps!
> >
> > Maybe just a comment indicating that max len is 11?
>
> Sure, will add
>
> > > > > + return PTR_ERR(cci->base);
> > > > > + }
> > > > > +
> > > > > + /* Interrupt */
> > > > > +
> > > > > + ret = platform_get_irq(pdev, 0);
> > > > > + if (ret < 0) {
> > > > > + dev_err(dev, "missing IRQ: %d\n", ret);
> > > > > + return ret;
> > > > > + }
> > > > > + cci->irq = ret;
> > > > > +
> > > > > + ret = devm_request_irq(dev, cci->irq, cci_isr,
> > > > > + IRQF_TRIGGER_RISING, dev_name(dev), cci);
> > > > > + if (ret < 0) {
> > > > > + dev_err(dev, "request_irq failed, ret: %d\n", ret);
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + disable_irq(cci->irq);
> > > >
> > > > Why? Is the irq always triggering or something?
> > >
> > > I supposed Todor didn't want to enable IRQ until everything is set.
> > > I could move this block before adding i2c adapter.
> >
> > But does it actually matter? The interrupt is "enabled" from request to
> > this function call so it's still possible to trigger. I'd just remove
> > the irq disabling unless it actually matters.
>
> Agreed, will do that
>
> > > > > +static const struct cci_data cci_8916_data = {
> > > > > + .num_masters = 1,
> > > > > + .queue_size = { 64, 16 },
> > > > > + .quirks = {
> > > > > + .max_write_len = 10,
> > > > > + .max_read_len = 12,
> > > > > + },
> > > > > + .res = {
> > > > > + .clock = {
> > > > > + "camss_top_ahb",
> > > > > + "cci_ahb",
> > > > > + "camss_ahb",
> > > > > + "cci"
> > > >
> > > > I guess this is another design where you just want to "get all the clks"
> > > > and not care about what they are?
> > >
> > > Yes that is how this seems to be
> >
> > Ok. I'm going to merge the "get all the clks" API soon. Maybe you can
> > use that here.
>
> Sure, once merged I will send a patch for this driver, which I guess
> would be after next merge window..
>
> --
> ~Vinod
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/2] i2c: Add Qualcomm CCI I2C driver
2018-09-20 19:39 ` Ricardo Ribalda Delgado
@ 2018-09-21 15:36 ` Vinod
2018-09-21 15:43 ` Todor Tomov
0 siblings, 1 reply; 16+ messages in thread
From: Vinod @ 2018-09-21 15:36 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: swboyd, wsa+renesas, linux-i2c, bjorn.andersson, linux-arm-msm,
Rob Herring, devicetree, Todor Tomov
Hi Ricardo,
On 20-09-18, 21:39, Ricardo Ribalda Delgado wrote:
> Sorry to enter the discussion this late.
>
> I have been using this driver with release/qcomlt-4.14 on a db820c and
> if I try to use the i2c bus before camss is running I get a Timeout
> error and the bus is unusable after that. I have sent a patch that
> fixes this issue, in case you want to merge it/add it before this
> driver goes upstream.
Thanks for this, I will test and port the fix to upstream and send it
along.
> Is there a way to use the i2c bus without using the camera?
This is Camera specific bus but it is an i2c bus, so theoretically it
should be possible to do so, Todor do you know if that is doable/done?
--
~Vinod
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/2] i2c: Add Qualcomm CCI I2C driver
2018-09-21 15:36 ` Vinod
@ 2018-09-21 15:43 ` Todor Tomov
2018-09-22 22:02 ` Ricardo Ribalda Delgado
0 siblings, 1 reply; 16+ messages in thread
From: Todor Tomov @ 2018-09-21 15:43 UTC (permalink / raw)
To: Vinod, Ricardo Ribalda Delgado
Cc: swboyd, wsa+renesas, linux-i2c, bjorn.andersson, linux-arm-msm,
Rob Herring, devicetree
Hi Vinod,
On 21.09.2018 08:36, Vinod wrote:
> Hi Ricardo,
>
> On 20-09-18, 21:39, Ricardo Ribalda Delgado wrote:
>> Sorry to enter the discussion this late.
>>
>> I have been using this driver with release/qcomlt-4.14 on a db820c and
>> if I try to use the i2c bus before camss is running I get a Timeout
>> error and the bus is unusable after that. I have sent a patch that
>> fixes this issue, in case you want to merge it/add it before this
>> driver goes upstream.
>
> Thanks for this, I will test and port the fix to upstream and send it
> along.
>
>> Is there a way to use the i2c bus without using the camera?
>
> This is Camera specific bus but it is an i2c bus, so theoretically it
> should be possible to do so, Todor do you know if that is doable/done?
I'm not aware of such experiments but yes, if you manage to connect
another i2c hardware it should be possible to control it.
--
Best regards,
Todor Tomov
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/2] i2c: Add Qualcomm CCI I2C driver
2018-09-21 15:43 ` Todor Tomov
@ 2018-09-22 22:02 ` Ricardo Ribalda Delgado
0 siblings, 0 replies; 16+ messages in thread
From: Ricardo Ribalda Delgado @ 2018-09-22 22:02 UTC (permalink / raw)
To: Todor Tomov
Cc: Vinod Koul, swboyd, wsa+renesas, linux-i2c, bjorn.andersson,
linux-arm-msm, Rob Herring, devicetree
Hi Todor, Hi Vinod
On Fri, Sep 21, 2018 at 5:43 PM Todor Tomov <todor.tomov@linaro.org> wrote:
>
> Hi Vinod,
>
> On 21.09.2018 08:36, Vinod wrote:
> > Hi Ricardo,
> >
> > On 20-09-18, 21:39, Ricardo Ribalda Delgado wrote:
> >> Sorry to enter the discussion this late.
> >>
> >> I have been using this driver with release/qcomlt-4.14 on a db820c and
> >> if I try to use the i2c bus before camss is running I get a Timeout
> >> error and the bus is unusable after that. I have sent a patch that
> >> fixes this issue, in case you want to merge it/add it before this
> >> driver goes upstream.
> >
> > Thanks for this, I will test and port the fix to upstream and send it
> > along.
> >
> >> Is there a way to use the i2c bus without using the camera?
> >
> > This is Camera specific bus but it is an i2c bus, so theoretically it
> > should be possible to do so, Todor do you know if that is doable/done?
>
> I'm not aware of such experiments but yes, if you manage to connect
> another i2c hardware it should be possible to control it.
I can access other i2c devices, but only when I am streaming images.
>
> --
> Best regards,
> Todor Tomov
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-09-22 22:02 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-20 6:39 [PATCH v4 0/2] i2c: Add support for Qcom CCI I2C controller Vinod Koul
2018-08-20 6:39 ` [PATCH v4 1/2] dt-bindings: i2c: Add binding for Qualcomm " Vinod Koul
2018-08-20 18:18 ` Rob Herring
2018-08-21 9:28 ` Vinod
2018-08-21 13:11 ` Rob Herring
2018-08-21 13:12 ` Rob Herring
2018-08-21 16:00 ` Vinod
2018-08-20 6:39 ` [PATCH v4 2/2] i2c: Add Qualcomm CCI I2C driver Vinod Koul
2018-08-24 7:30 ` Stephen Boyd
2018-08-24 10:16 ` Vinod
2018-08-27 19:19 ` Stephen Boyd
2018-08-28 3:38 ` Vinod
2018-09-20 19:39 ` Ricardo Ribalda Delgado
2018-09-21 15:36 ` Vinod
2018-09-21 15:43 ` Todor Tomov
2018-09-22 22:02 ` Ricardo Ribalda Delgado
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.