Skip to contentSkip to conversation

Qualcomm Universal Peripheral (QUP) I2C controller

PATCH Superseded Bjorn Andersson 9 participants 15 messages Jan 17, 2014 – Jan 30, 2014

Subsystems (3)

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

v3 cover Qualcomm Universal Peripheral (QUP) I2C controller

Continuing on Ivans i2c-qup series.

Changes from v2:
- Removed unused variables and includes
- Corrected read logic in irq handler
- Made the polling loop in qup_i2c_poll_state() less arbitrary
- Only building suspend/resume if CONFIG_PM_SLEEP

Changes from v1:
- Cleaned up device tree binding example.
- Refrased device tree bindings.
- Following changes in the i2c framework.
- Use the core clock to calculate divider for the bus clock, instead of explicitly setting it.
- Remove explicit pinctrl settting.
- Split/renamed qup_i2c_enable(bool) into enable/disable functions.
- Return value was overwritten on error in write_one/read_one.
- Initialize the i2c core every time, so that we actually can execute more than 1 transmission per xfer.

Ivan T. Ivanov (2):
i2c: qup: Add device tree bindings information
i2c: New bus driver for the QUP I2C controller

 .../devicetree/bindings/i2c/qcom,i2c-qup.txt       |  41 +
 drivers/i2c/busses/Kconfig                         |  10 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-qup.c                       | 894 +++++++++++++++++++++
 4 files changed, 946 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt
 create mode 100644 drivers/i2c/busses/i2c-qup.c
Signature · 1 line
1.8.2.2
#1 lore raw

PATCH v3 1/2 i2c: qup: Add device tree bindings information

From: "Ivan T. Ivanov" <[email protected]>

The Qualcomm Universal Peripherial (QUP) wraps I2C mini-core and provide input and output FIFO's for it. I2C controller can operate as master with supported bus speeds of 100Kbps and 400Kbps.

[bjorn: reformulated part of binding description and cleaned up example]
---
 .../devicetree/bindings/i2c/qcom,i2c-qup.txt       | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt
Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt new file +41 -0
new file mode 100644
index 0000000..a99711b
@@ -0,0 +1,41 @@
+Qualcomm Universal Peripheral (QUP) I2C controller
+
+Required properties:
+ - compatible: Should be "qcom,i2c-qup".
+ - reg: Should contain QUP register address and length.
+ - interrupts: Should contain I2C interrupt.
+
+ - clocks: Should contain the core clock and the AHB clock.
+ - clock-names: Should be "core" for the core clock and "iface" for the
+                AHB clock.
+
+ - #address-cells: Should be <1> Address cells for i2c device address
+ - #size-cells: Should be <0> as i2c addresses have no size component
+
+Optional properties:
+ - clock-frequency: Should specify the desired i2c bus clock frequency in Hz,
+                    default is 100kHz if omitted.
+
+Child nodes should conform to i2c bus binding.
+
+Example:
+
+ i2c2: i2c@f9924000 {
+ 	compatible = "qcom,i2c-qup";
+ 	reg = <0xf9924000 0x1000>;
+ 	interrupts = <0 96 0>;
+
+ 	clocks = <&gcc_blsp1_qup2_i2c_apps_clk>, <&gcc_blsp1_ahb_clk>;
+ 	clock-names = "core", "iface";
+
+ 	clock-frequency = <355000>;
+
+ 	#address-cells = <1>;
+ 	#size-cells = <0>;
+
+ 	dummy@60 {
+ 		compatible = "dummy";
+ 		reg = <0x60>;
+ 	};
+ };
+
Signature · 1 line
1.8.2.2
#2 lore raw

Re: [PATCH v3 1/2] i2c: qup: Add device tree bindings information

Quoted 10 lines · Bjorn Andersson

From: "Ivan T. Ivanov" <[email protected]>

The Qualcomm Universal Peripherial (QUP) wraps I2C mini-core and provide input and output FIFO's for it. I2C controller can operate as master with supported bus speeds of 100Kbps and 400Kbps.

[bjorn: reformulated part of binding description and cleaned up example]

---

Patch history?

Quoted 10 lines
 .../devicetree/bindings/i2c/qcom,i2c-qup.txt       | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt
Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt new file +0 -0
new file mode 100644
index 0000000..a99711b

Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt line 4

Quoted 4 lines
Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt new file +4 -0
@@ -0,0 +1,41 @@
+Qualcomm Universal Peripheral (QUP) I2C controller
+
+Required properties:
+ - compatible: Should be "qcom,i2c-qup".

Seems a bit generic. All versions of the IP are exactly the same? "qcom,<chip>-i2c-qup" would be better.

Quoted 44 lines
Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt new file +37 -1
+ - reg: Should contain QUP register address and length.
+ - interrupts: Should contain I2C interrupt.
+
+ - clocks: Should contain the core clock and the AHB clock.
+ - clock-names: Should be "core" for the core clock and "iface" for the
+                AHB clock.
+
+ - #address-cells: Should be <1> Address cells for i2c device address
+ - #size-cells: Should be <0> as i2c addresses have no size component
+
+Optional properties:
+ - clock-frequency: Should specify the desired i2c bus clock frequency in Hz,
+                    default is 100kHz if omitted.
+
+Child nodes should conform to i2c bus binding.
+
+Example:
+
+ i2c2: i2c at f9924000 {
+       compatible = "qcom,i2c-qup";
+       reg = <0xf9924000 0x1000>;
+       interrupts = <0 96 0>;
+
+       clocks = <&gcc_blsp1_qup2_i2c_apps_clk>, <&gcc_blsp1_ahb_clk>;
+       clock-names = "core", "iface";
+
+       clock-frequency = <355000>;
+
+       #address-cells = <1>;
+       #size-cells = <0>;
+
+       dummy at 60 {
+               compatible = "dummy";
+               reg = <0x60>;
+       };
+ };
+
--

1.8.2.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw

PATCH v3 1/2 i2c: qup: Add device tree bindings information

Quoted 7 lines · Rob Herring
Quoted 4 lines · Bjorn Andersson
 .../devicetree/bindings/i2c/qcom,i2c-qup.txt       | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt
Quoted 14 lines

Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt line 4

Quoted 10 lines
Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt new file +4 -0
new file mode 100644
index 0000000..a99711b
@@ -0,0 +1,41 @@
+Qualcomm Universal Peripheral (QUP) I2C controller
+
+Required properties:
+ - compatible: Should be "qcom,i2c-qup".

Seems a bit generic. All versions of the IP are exactly the same? "qcom,<chip>-i2c-qup" would be better.

There are different versions of the IP in different SoCs. The versions for platforms supported upstream are:

	1.1.1 (MSM8660, MSM8960)
	2.2.1 (MSM8974v2)

so how about one of "qcom,i2c-qup-v1.1.1" or "qcom,i2c-qup-v2.2.1" should be specified as more specific compatible strings in addition to "qcom,i2c-qup"?

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
#4 lore raw

PATCH v3 1/2 i2c: qup: Add device tree bindings information

Quoted 26 lines · Stephen Boyd
Quoted 7 lines · Rob Herring
Quoted 4 lines · Bjorn Andersson
 .../devicetree/bindings/i2c/qcom,i2c-qup.txt       | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt
Quoted 14 lines

Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt line 4

Quoted 10 lines
Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt new file +4 -0
new file mode 100644
index 0000000..a99711b
@@ -0,0 +1,41 @@
+Qualcomm Universal Peripheral (QUP) I2C controller
+
+Required properties:
+ - compatible: Should be "qcom,i2c-qup".

Seems a bit generic. All versions of the IP are exactly the same? "qcom,<chip>-i2c-qup" would be better.

There are different versions of the IP in different SoCs. The versions for platforms supported upstream are:

Quoted 7 lines
	1.1.1 (MSM8660, MSM8960)
	2.2.1 (MSM8974v2)

so how about one of "qcom,i2c-qup-v1.1.1" or "qcom,i2c-qup-v2.2.1" should be specified as more specific compatible strings in addition to "qcom,i2c-qup"?

This definitely needs two compatible tags. The v1.1.1 supports the FIFO, block, and data mover modes. The v2.2.1 supports FIFO, block, and BAM DMA.

The BAM patches are currently in review and ideally BAM support can be added to the I2C. We'll need a way to differentiate so BAM can be used.

Otherwise, the register interfaces are interchangeable except for BAM specific fields in the registers.

Quoted 4 lines

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signature · 3 lines
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
#5 lore raw

PATCH v3 1/2 i2c: qup: Add device tree bindings information

In reply to Rob Herring

Rob,

Quoted 19 lines · Rob Herring
Quoted 10 lines · Bjorn Andersson

From: "Ivan T. Ivanov" <[email protected]>

The Qualcomm Universal Peripherial (QUP) wraps I2C mini-core and provide input and output FIFO's for it. I2C controller can operate as master with supported bus speeds of 100Kbps and 400Kbps.

[bjorn: reformulated part of binding description and cleaned up example]

---

Patch history?

Quoted 4 lines

.../devicetree/bindings/i2c/qcom,i2c-qup.txt | 41 ++++++++++++++++++++++

1 file changed, 41 insertions(+)

create mode 100644 Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt

Quoted 13 lines

Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt line 4

Quoted 10 lines
Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt new file +4 -0
new file mode 100644
index 0000000..a99711b
@@ -0,0 +1,41 @@
+Qualcomm Universal Peripheral (QUP) I2C controller
+
+Required properties:
+ - compatible: Should be "qcom,i2c-qup".

Seems a bit generic. All versions of the IP are exactly the same? "qcom,<chip>-i2c-qup" would be better.

The QUP IP block is the same across chips. Its possible there could be different versions of the IP block but its pretty stable now.

Quoted 41 lines
Quoted 41 lines
Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt new file +37 -1
+ - reg: Should contain QUP register address and length.
+ - interrupts: Should contain I2C interrupt.
+
+ - clocks: Should contain the core clock and the AHB clock.
+ - clock-names: Should be "core" for the core clock and "iface" for the
+                AHB clock.
+
+ - #address-cells: Should be <1> Address cells for i2c device address
+ - #size-cells: Should be <0> as i2c addresses have no size component
+
+Optional properties:
+ - clock-frequency: Should specify the desired i2c bus clock frequency in Hz,
+                    default is 100kHz if omitted.
+
+Child nodes should conform to i2c bus binding.
+
+Example:
+
+ i2c2: i2c at f9924000 {
+       compatible = "qcom,i2c-qup";
+       reg = <0xf9924000 0x1000>;
+       interrupts = <0 96 0>;
+
+       clocks = <&gcc_blsp1_qup2_i2c_apps_clk>, <&gcc_blsp1_ahb_clk>;
+       clock-names = "core", "iface";
+
+       clock-frequency = <355000>;
+
+       #address-cells = <1>;
+       #size-cells = <0>;
+
+       dummy at 60 {
+               compatible = "dummy";
+               reg = <0x60>;
+       };
+ };
+
--

1.8.2.2

Quoted 9 lines
Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

#6 lore raw

PATCH v3 1/2 i2c: qup: Add device tree bindings information

Quoted 1 line · Matthew Locke

The QUP IP block is the same across chips. Its possible there could be different versions of the IP block but its pretty stable now.

Do you know the version numbers of the IP block? That would be the ideal case. We just fall back to using the SoC name in most cases because that's all that is publically known.

Arnd

#7 lore raw

PATCH v3 1/2 i2c: qup: Add device tree bindings information

In reply to Matthew Locke
Quoted 5 lines · Matthew Locke

Rob,

Quoted 1 line · Rob Herring

On Fri, Jan 17, 2014 at 5:03 PM, Bjorn Andersson

[...]

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

+Required properties:
+ - compatible: Should be "qcom,i2c-qup".

Seems a bit generic. All versions of the IP are exactly the same? "qcom,<chip>-i2c-qup" would be better.

The QUP IP block is the same across chips. Its possible there could be different versions of the IP block but its pretty stable now.

I have not been able to find any differences in this block between 8960 and 8074 and I have successfully tested the driver on both platforms.

So the reason for making it more specific would be to handle any future crazyness. Not sure what to do about this though, I have never heard about the version numbers Stephen gave us and I can't find them in the documentation available to us, so I wouldn't know which of the two to select...

Regards,
Bjorn

#8 lore raw

PATCH v3 2/2 i2c: New bus driver for the QUP I2C controller

In reply to Bjorn Andersson

From: "Ivan T. Ivanov" <[email protected]>

This bus driver supports the QUP i2c hardware controller in the Qualcomm MSM SOCs. The Qualcomm Universal Peripheral Engine (QUP) is a general purpose data path engine with input/output FIFOs and an embedded i2c
mini-core. The driver supports FIFO mode (for low bandwidth applications)
and block mode (interrupt generated for each block-size data transfer). The driver currently does not support DMA transfers.

Shamelessly based on codeaurora version of the driver.

[bjorn: updated to reflect i2c framework changes
splited up qup_i2c_enable() in enable/disable don't overwrite ret value on error in xfer functions initilize core for each transfer
remove explicit pinctrl selection
use existing clock instead of setting new core clock]

---
 drivers/i2c/busses/Kconfig   |  10 +
 drivers/i2c/busses/Makefile  |   1 +
 drivers/i2c/busses/i2c-qup.c | 894 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 905 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-qup.c
drivers/i2c/busses/Kconfig +10 -0
index 3b26129..4eaded0 100644
@@ -648,6 +648,16 @@ config I2C_PXA_SLAVE
 	  is necessary for systems where the PXA may be a target on the
 	  I2C bus.
 
+config I2C_QUP
+	tristate "Qualcomm QUP based I2C controller"
+	depends on ARCH_MSM
+	help
+	  If you say yes to this option, support will be included for the
+	  built-in I2C interface on the MSM family processors.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-qup.
+
 config HAVE_S3C2410_I2C
 	bool
 	help
drivers/i2c/busses/Makefile +1 -0
index c73eb0e..c93f593 100644
@@ -63,6 +63,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_QUP)		+= i2c-qup.o
 obj-$(CONFIG_I2C_S3C2410)	+= i2c-s3c2410.o
 obj-$(CONFIG_I2C_S6000)		+= i2c-s6000.o
 obj-$(CONFIG_I2C_SH7760)	+= i2c-sh7760.o
drivers/i2c/busses/i2c-qup.c new file +894 -0

894 lines not on this page · raw message

Signature · 1 line
1.8.2.2
#9 lore raw

Re: [PATCH v3 2/2] i2c: New bus driver for the QUP I2C controller

Quoted 123 lines · Bjorn Andersson
drivers/i2c/busses/i2c-qup.c new file +117 -0
new file mode 100644
index 0000000..2e0020e
@@ -0,0 +1,894 @@
+/* Copyright (c) 2009-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+/* QUP Registers */
+#define QUP_CONFIG		0x000
+#define QUP_STATE		0x004
+#define QUP_IO_MODE		0x008
+#define QUP_SW_RESET		0x00c
+#define QUP_OPERATIONAL		0x018
+#define QUP_ERROR_FLAGS		0x01c
+#define QUP_ERROR_FLAGS_EN	0x020
+#define QUP_HW_VERSION		0x030
+#define QUP_MX_OUTPUT_CNT	0x100
+#define QUP_OUT_FIFO_BASE	0x110
+#define QUP_MX_WRITE_CNT	0x150
+#define QUP_MX_INPUT_CNT	0x200
+#define QUP_MX_READ_CNT		0x208
+#define QUP_IN_FIFO_BASE	0x218
+#define QUP_I2C_CLK_CTL		0x400
+#define QUP_I2C_STATUS		0x404
+
+/* QUP States and reset values */
+#define QUP_RESET_STATE		0
+#define QUP_RUN_STATE		1
+#define QUP_PAUSE_STATE		3
+#define QUP_STATE_MASK		3
+
+#define QUP_STATE_VALID		BIT(2)
+#define QUP_I2C_MAST_GEN	BIT(4)
+
+#define QUP_OPERATIONAL_RESET	0x000ff0
+#define QUP_I2C_STATUS_RESET	0xfffffc
+
+/* QUP OPERATIONAL FLAGS */
+#define QUP_OUT_SVC_FLAG	BIT(8)
+#define QUP_IN_SVC_FLAG		BIT(9)
+#define QUP_MX_INPUT_DONE	BIT(11)
+
+/* I2C mini core related values */
+#define I2C_MINI_CORE		(2 << 8)
+#define I2C_N_VAL		15
+/* Most significant word offset in FIFO port */
+#define QUP_MSW_SHIFT		(I2C_N_VAL + 1)
+#define QUP_CLOCK_AUTO_GATE	BIT(13)
+
+/* Packing/Unpacking words in FIFOs, and IO modes */
+#define QUP_UNPACK_EN		BIT(14)
+#define QUP_PACK_EN		BIT(15)
+#define QUP_OUTPUT_BLK_MODE	BIT(10)
+#define QUP_INPUT_BLK_MODE	BIT(12)
+
+#define QUP_REPACK_EN		(QUP_UNPACK_EN | QUP_PACK_EN)
+
+#define QUP_OUTPUT_BLOCK_SIZE(x)(((x) & (0x03 << 0)) >> 0)
+#define QUP_OUTPUT_FIFO_SIZE(x)	(((x) & (0x07 << 2)) >> 2)
+#define QUP_INPUT_BLOCK_SIZE(x)	(((x) & (0x03 << 5)) >> 5)
+#define QUP_INPUT_FIFO_SIZE(x)	(((x) & (0x07 << 7)) >> 7)
+
+/* QUP tags */
+#define QUP_OUT_NOP		(0 << 8)
+#define QUP_OUT_START		(1 << 8)
+#define QUP_OUT_DATA		(2 << 8)
+#define QUP_OUT_STOP		(3 << 8)
+#define QUP_OUT_REC		(4 << 8)
+#define QUP_IN_DATA		(5 << 8)
+#define QUP_IN_STOP		(6 << 8)
+#define QUP_IN_NACK		(7 << 8)
+
+/* Status, Error flags */
+#define I2C_STATUS_WR_BUFFER_FULL	BIT(0)
+#define I2C_STATUS_BUS_ACTIVE		BIT(8)
+#define I2C_STATUS_BUS_MASTER		BIT(9)
+#define I2C_STATUS_ERROR_MASK		0x38000fc
+#define QUP_I2C_NACK_FLAG		BIT(3)
+#define QUP_IN_NOT_EMPTY		BIT(5)
+#define QUP_STATUS_ERROR_FLAGS		0x7c
+
+/* Master bus_err clock states */
+#define I2C_CLK_RESET_BUSIDLE_STATE	0
+#define I2C_CLK_FORCED_LOW_STATE	5
+
+#define QUP_MAX_CLK_STATE_RETRIES	300
+#define QUP_MAX_QUP_STATE_RETRIES	100
+#define I2C_STATUS_CLK_STATE		13
+#define QUP_OUT_FIFO_NOT_EMPTY		0x10
+#define QUP_READ_LIMIT			256
+
+struct qup_i2c_dev {
+	struct device		*dev;
+	void __iomem		*base;
+	int			irq;
+	struct clk		*clk;
+	struct clk		*pclk;
+	struct i2c_adapter	adap;
+
+	int			clk_ctl;

drivers/i2c/busses/i2c-qup.c line 127

Quoted 10 lines
drivers/i2c/busses/i2c-qup.c new file +10 -0
+	int			one_bit_t;
+	int			out_fifo_sz;
+	int			in_fifo_sz;
+	int			out_blk_sz;
+	int			in_blk_sz;
+	unsigned long		xfer_time;
+	unsigned long		wait_idle;
+
+	struct i2c_msg		*msg;
+	/* Current posion in user message buffer */

s/posion/position/

Quoted 14 lines
drivers/i2c/busses/i2c-qup.c new file +14 -0
+	int			pos;
+	/* Keep number of bytes left to be transmitted */
+	int			cnt;
+	/* I2C protocol errors */
+	u32			bus_err;
+	/* QUP core errors */
+	u32			qup_err;
+	/*
+	 * Maximum bytes that could be send (per iteration). Could be
+	 * equal of fifo size or block size (in block mode)
+	 */
+	int			chunk_sz;
+	struct completion	xfer;
+};

[...]

Quoted 20 lines
drivers/i2c/busses/i2c-qup.c new file +20 -0
+
+static int
+qup_i2c_poll_state(struct qup_i2c_dev *qup, u32 req_state, bool only_valid)
+{
+	int retries = 0;
+	u32 state;
+
+	do {
+		state = readl(qup->base + QUP_STATE);
+
+		/*
+		 * If only valid bit needs to be checked, requested state is
+		 * 'don't care'
+		 */
+		if (state & QUP_STATE_VALID) {
+			if (only_valid)
+				return 0;
+			if ((req_state & QUP_I2C_MAST_GEN)
+			    && (state & QUP_I2C_MAST_GEN))
+				return 0;

drivers/i2c/busses/i2c-qup.c

Quoted 10 lines
drivers/i2c/busses/i2c-qup.c new file +10 -0
+			if ((state & QUP_STATE_MASK) == req_state)
+				return 0;
+		}
+
+		udelay(1);
+	} while (retries++ != QUP_MAX_QUP_STATE_RETRIES);
+
+	return -ETIMEDOUT;
+}
+

This is what I was suggesting. Don't know if it's any clearer, but it saves us a whopping 3 lines!

---8<----

 drivers/i2c/busses/i2c-qup.c | 43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)
drivers/i2c/busses/i2c-qup.c +20 -23
index 2e0020e829ae..431de13f6281 100644
@@ -47,7 +47,7 @@
 #define QUP_STATE_MASK		3
 
 #define QUP_STATE_VALID		BIT(2)
-#define QUP_I2C_MAST_GEN	BIT(4)
+#define QUP_I2C_MAST_GEN	(QUP_STATE_VALID | BIT(4))
 
 #define QUP_OPERATIONAL_RESET	0x000ff0
 #define QUP_I2C_STATUS_RESET	0xfffffc
@@ -190,43 +190,40 @@ done:
 	return IRQ_HANDLED;
 }
 
-static int
-qup_i2c_poll_state(struct qup_i2c_dev *qup, u32 req_state, bool only_valid)
+static int __qup_i2c_poll_state(struct qup_i2c_dev *qup, u32 mask, u32 value)
 {
 	int retries = 0;
 	u32 state;
 
 	do {
 		state = readl(qup->base + QUP_STATE);
-
-		/*
-		 * If only valid bit needs to be checked, requested state is
-		 * 'don't care'
-		 */
-		if (state & QUP_STATE_VALID) {
-			if (only_valid)
-				return 0;
-			if ((req_state & QUP_I2C_MAST_GEN)
-			    && (state & QUP_I2C_MAST_GEN))
-				return 0;
-			if ((state & QUP_STATE_MASK) == req_state)
-				return 0;
-		}
-
+		if ((state & mask) == value)
+			return 0;
 		udelay(1);
 	} while (retries++ != QUP_MAX_QUP_STATE_RETRIES);
 
 	return -ETIMEDOUT;
 }
 
+static int qup_i2c_poll_state_bit(struct qup_i2c_dev *qup, u32 mask)
+{
+	return __qup_i2c_poll_state(qup, mask, mask);
+}
+
+static int qup_i2c_poll_state(struct qup_i2c_dev *qup, u32 state)
+{
+	return __qup_i2c_poll_state(qup, QUP_STATE_VALID | QUP_STATE_MASK,
+					 QUP_STATE_VALID | state);
+}
+
 static int qup_i2c_change_state(struct qup_i2c_dev *qup, u32 state)
 {
-	if (qup_i2c_poll_state(qup, 0, true) != 0)
+	if (qup_i2c_poll_state_bit(qup, QUP_STATE_VALID) != 0)
 		return -EIO;
 
 	writel(state, qup->base + QUP_STATE);
 
-	if (qup_i2c_poll_state(qup, state, false) != 0)
+	if (qup_i2c_poll_state(qup, state) != 0)
 		return -EIO;
 	return 0;
 }
@@ -616,7 +613,7 @@ qup_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		goto out;
 
 	writel(1, qup->base + QUP_SW_RESET);
-	ret = qup_i2c_poll_state(qup, QUP_RESET_STATE, false);
+	ret = qup_i2c_poll_state(qup, QUP_RESET_STATE);
 	if (ret) {
 		dev_err(qup->dev, "cannot goto reset state\n");
 		goto out;
@@ -633,7 +630,7 @@ qup_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		writel(0, qup->base + QUP_I2C_CLK_CTL);
 		writel(QUP_I2C_STATUS_RESET, qup->base + QUP_I2C_STATUS);
 
-		if (qup_i2c_poll_state(qup, QUP_I2C_MAST_GEN, false) != 0) {
+		if (qup_i2c_poll_state_bit(qup, QUP_I2C_MAST_GEN) != 0) {
 			ret = -EIO;
 			goto out;
 		}
@@ -730,7 +727,7 @@ static int qup_i2c_probe(struct platform_device *pdev)
 	 * so we reset the core before registering for interrupts.
 	 */
 	writel(1, qup->base + QUP_SW_RESET);
-	ret = qup_i2c_poll_state(qup, 0, true);
+	ret = qup_i2c_poll_state(qup, QUP_STATE_VALID);
 	if (ret)
 		goto fail;
 
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
#10 lore raw

Re: [PATCH v3 2/2] i2c: New bus driver for the QUP I2C controller

In reply to Bjorn Andersson
Quoted 20 lines · Bjorn Andersson

From: "Ivan T. Ivanov" <[email protected]>

This bus driver supports the QUP i2c hardware controller in the Qualcomm MSM SOCs. The Qualcomm Universal Peripheral Engine (QUP) is a general purpose data path engine with input/output FIFOs and an embedded i2c
mini-core. The driver supports FIFO mode (for low bandwidth applications)
and block mode (interrupt generated for each block-size data transfer). The driver currently does not support DMA transfers.

Shamelessly based on codeaurora version of the driver.

[bjorn: updated to reflect i2c framework changes
splited up qup_i2c_enable() in enable/disable don't overwrite ret value on error in xfer functions initilize core for each transfer
remove explicit pinctrl selection
use existing clock instead of setting new core clock]

---

<snip>

Quoted 6 lines
drivers/i2c/busses/i2c-qup.c new file +6 -0
+
+	io_mode = readl(qup->base + QUP_IO_MODE);
+
+	size = QUP_OUTPUT_BLOCK_SIZE(io_mode);
+	if (size)
+		qup->out_blk_sz = size * 16;

drivers/i2c/busses/i2c-qup.c line 768

Quoted 10 lines
drivers/i2c/busses/i2c-qup.c new file +10 -0
+	else
+		qup->out_blk_sz = 16;
+
+	size = QUP_INPUT_BLOCK_SIZE(io_mode);
+	if (size)
+		qup->in_blk_sz = size * 16;
+	else
+		qup->in_blk_sz = 16;
+
+	qup->xfer_time = msecs_to_jiffies(qup->out_fifo_sz);

qup->xfer_time should be set after you calculate qup->out_fifo_sz below.

Quoted 127 lines
drivers/i2c/busses/i2c-qup.c new file +126 -0
+
+	/*
+	 * The block/fifo size w.r.t. 'actual data' is 1/2 due to 'tag'
+	 * associated with each byte written/received
+	 */
+	qup->out_blk_sz /= 2;
+	qup->in_blk_sz /= 2;
+
+	size = QUP_OUTPUT_FIFO_SIZE(io_mode);
+	qup->out_fifo_sz = qup->out_blk_sz * (2 << size);
+
+	size = QUP_INPUT_FIFO_SIZE(io_mode);
+	qup->in_fifo_sz = qup->in_blk_sz * (2 << size);
+
+	/*
+	 * Wait for FIFO number of bytes to be absolutely sure
+	 * that I2C write state machine is not idle. Each byte
+	 * takes 9 clock cycles. (8 bits + 1 ack)
+	 */
+	qup->wait_idle = qup->one_bit_t * 9;
+	qup->wait_idle *= qup->out_fifo_sz;
+
+	dev_info(qup->dev, "IN:block:%d, fifo:%d, OUT:block:%d, fifo:%d\n",
+		qup->in_blk_sz, qup->in_fifo_sz,
+		qup->out_blk_sz, qup->out_fifo_sz);
+
+	i2c_set_adapdata(&qup->adap, qup);
+	qup->adap.algo = &qup_i2c_algo;
+	qup->adap.nr = pdev->id;
+	qup->adap.dev.parent = qup->dev;
+	qup->adap.dev.of_node = pdev->dev.of_node;
+	strlcpy(qup->adap.name, "QUP I2C adapter", sizeof(qup->adap.name));
+
+	ret = i2c_add_numbered_adapter(&qup->adap);
+	if (!ret) {
+		pm_runtime_set_autosuspend_delay(qup->dev, MSEC_PER_SEC);
+		pm_runtime_use_autosuspend(qup->dev);
+		pm_runtime_enable(qup->dev);
+		return 0;
+	}
+fail:
+	qup_i2c_disable_clocks(qup);
+	return ret;
+}
+
+static int qup_i2c_remove(struct platform_device *pdev)
+{
+	struct qup_i2c_dev *qup = platform_get_drvdata(pdev);
+
+	disable_irq(qup->irq);
+	qup_i2c_disable_clocks(qup);
+	i2c_del_adapter(&qup->adap);
+	pm_runtime_disable(qup->dev);
+	pm_runtime_set_suspended(qup->dev);
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int qup_i2c_pm_suspend_runtime(struct device *device)
+{
+	struct qup_i2c_dev *qup = dev_get_drvdata(device);
+
+	dev_dbg(device, "pm_runtime: suspending...\n");
+	qup_i2c_disable_clocks(qup);
+	return 0;
+}
+
+static int qup_i2c_pm_resume_runtime(struct device *device)
+{
+	struct qup_i2c_dev *qup = dev_get_drvdata(device);
+
+	dev_dbg(device, "pm_runtime: resuming...\n");
+	qup_i2c_enable_clocks(qup);
+	return 0;
+}
+#endif
+
+#ifdef CONFIG_PM_SLEEP
+static int qup_i2c_suspend(struct device *device)
+{
+	dev_dbg(device, "system suspend");
+	qup_i2c_pm_suspend_runtime(device);
+	return 0;
+}
+
+static int qup_i2c_resume(struct device *device)
+{
+	dev_dbg(device, "system resume");
+	qup_i2c_pm_resume_runtime(device);
+	pm_runtime_mark_last_busy(device);
+	pm_request_autosuspend(device);
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops qup_i2c_qup_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(
+		qup_i2c_suspend,
+		qup_i2c_resume)
+	SET_RUNTIME_PM_OPS(
+		qup_i2c_pm_suspend_runtime,
+		qup_i2c_pm_resume_runtime,
+		NULL)
+};
+
+static const struct of_device_id qup_i2c_dt_match[] = {
+	{.compatible = "qcom,i2c-qup"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, qup_i2c_dt_match);
+
+static struct platform_driver qup_i2c_driver = {
+	.probe  = qup_i2c_probe,
+	.remove = qup_i2c_remove,
+	.driver = {
+		.name = "i2c_qup",
+		.owner = THIS_MODULE,
+		.pm = &qup_i2c_qup_pm_ops,
+		.of_match_table = qup_i2c_dt_match,
+	},
+};
+
+module_platform_driver(qup_i2c_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:i2c_qup");
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
#11 lore raw
In reply to Bjorn Andersson

Hi Bjorn,

Quoted 2 lines · Bjorn Andersson

Continuing on Ivans i2c-qup series.

Do you plan to send v4 of this driver? I would like to address the remaining errors and suggestions and send a new version.

Regards,
Ivan

#12 lore raw

v3 cover Qualcomm Universal Peripheral (QUP) I2C controller

Quoted 10 lines · Ivan T. Ivanov

Hi Bjorn,

Quoted 2 lines · Bjorn Andersson

Continuing on Ivans i2c-qup series.

Do you plan to send v4 of this driver? I would like to address the remaining errors and suggestions and send a new version.

Hi Ivan,

Yes I'm planning to send out a new revision of the patch set.

I've incorporated fixes from the review comments here and my colleague concluded through some testing that block read did not work, so we've fixed that as well.

What have been holding me from submitting a new patchset is the 3 functions that does polling of state and status updates;
* qup_i2c_poll_state() reads the state register up to 1000 times, hoping we reach the expected state, will delay 100uS and then continue with 1000 more retries.
According to the data sheet a state transition is supposed to take
up to 2 bus cycles. Only time I can see that this would take longer time are all error states, but the data sheet is not very clear regarding this.

* qup_i2c_wait_idle() reads the status register up to 1000 times, hoping the fifo gets drained and the bus go idle, if that fails it sleeps for the time we expect it to take to drain a full fifo and then loops another 1000 times. This waits for the fifo to have drained and the bus to go idle. On a read we get to this state if we issue the write and then hit the error state, so we would reset the entire block. On write we will only wait for the buffer not to be full before returning.

* qup_i2c_wait_clock_ready() waits up to 300 bus-clocks for the i2c bus to go idle or forced low, I don't know why it retries 300 times. This is called at the end of a write, possibly to wait for the fifo to drain.

All three loops are in line with how it's been in codeaurora since the beginning of time, but I at least need to figure out some good names for those "magic numbers".

Regards,
Bjorn

#13 lore raw

v3 cover Qualcomm Universal Peripheral (QUP) I2C controller

Hi Bjorn,

Quoted 11 lines · Bjorn Andersson
Quoted 10 lines · Ivan T. Ivanov

Hi Bjorn,

Quoted 2 lines · Bjorn Andersson

Continuing on Ivans i2c-qup series.

Do you plan to send v4 of this driver? I would like to address the remaining errors and suggestions and send a new version.

Quoted 7 lines

Hi Ivan,

Yes I'm planning to send out a new revision of the patch set.

I've incorporated fixes from the review comments here and my colleague concluded through some testing that block read did not work, so we've fixed that as well.

Busted. I have not test it.

Quoted 26 lines

What have been holding me from submitting a new patchset is the 3 functions that does polling of state and status updates;
* qup_i2c_poll_state() reads the state register up to 1000 times, hoping we reach the expected state, will delay 100uS and then continue with 1000 more retries.
According to the data sheet a state transition is supposed to take
up to 2 bus cycles. Only time I can see that this would take longer time are all error states, but the data sheet is not very clear regarding this.

* qup_i2c_wait_idle() reads the status register up to 1000 times, hoping the fifo gets drained and the bus go idle, if that fails it sleeps for the time we expect it to take to drain a full fifo and then loops another 1000 times. This waits for the fifo to have drained and the bus to go idle. On a read we get to this state if we issue the write and then hit the error state, so we would reset the entire block. On write we will only wait for the buffer not to be full before returning.

* qup_i2c_wait_clock_ready() waits up to 300 bus-clocks for the i2c bus to go idle or forced low, I don't know why it retries 300 times. This is called at the end of a write, possibly to wait for the fifo to drain.

Quoted 3 lines

All three loops are in line with how it's been in codeaurora since the beginning of time, but I at least need to figure out some good names for those "magic numbers".

Sure. I have keep them this way, just because I don't have information for internal trickery of the block.

Thanks,
Ivan

Quoted 3 lines

Regards,
Bjorn

#14 lore raw

v3 cover Qualcomm Universal Peripheral (QUP) I2C controller

Quoted 17 lines · Ivan T. Ivanov

Hi Bjorn,

Quoted 13 lines · Bjorn Andersson
Quoted 10 lines · Ivan T. Ivanov

Hi Bjorn,

Quoted 2 lines · Bjorn Andersson

Continuing on Ivans i2c-qup series.

Do you plan to send v4 of this driver? I would like to address the remaining errors and suggestions and send a new version.

Hi Ivan,

Quoted 7 lines
Quoted 5 lines

Yes I'm planning to send out a new revision of the patch set.

I've incorporated fixes from the review comments here and my colleague concluded through some testing that block read did not work, so we've fixed that as well.

Busted. I have not test it.

No worries, I'm glad you did the major cleanup from codeaurora!

Quoted 27 lines
Quoted 27 lines

What have been holding me from submitting a new patchset is the 3 functions that does polling of state and status updates;
* qup_i2c_poll_state() reads the state register up to 1000 times, hoping we reach the expected state, will delay 100uS and then continue with 1000 more retries.
According to the data sheet a state transition is supposed to take
up to 2 bus cycles. Only time I can see that this would take longer time are all error states, but the data sheet is not very clear regarding this.

* qup_i2c_wait_idle() reads the status register up to 1000 times, hoping the fifo gets drained and the bus go idle, if that fails it sleeps for the time we expect it to take to drain a full fifo and then loops another 1000 times. This waits for the fifo to have drained and the bus to go idle. On a read we get to this state if we issue the write and then hit the error state, so we would reset the entire block. On write we will only wait for the buffer not to be full before returning.

* qup_i2c_wait_clock_ready() waits up to 300 bus-clocks for the i2c bus to go idle or forced low, I don't know why it retries 300 times. This is called at the end of a write, possibly to wait for the fifo to drain.

Quoted 7 lines
Quoted 3 lines

All three loops are in line with how it's been in codeaurora since the beginning of time, but I at least need to figure out some good names for those "magic numbers".

Sure. I have keep them this way, just because I don't have information for internal trickery of the block.

I'll continue to talk to the Qualcomm guys to see if we can figure out anything regarding the expected timings of those operations and I'll send out a new version when we have something saner.

Regards,
Bjorn