[PATCH v8 0/5] Add support for the STM32F4 I2C

[PATCH v8 0/5] Add support for the STM32F4 I2C

[M'boumba Cedric Madianga]

This patchset adds support for the I2C controller embedded in STM32F4xx SoC.
It enables I2C transfer in interrupt mode with Standard-mode and Fast-mode bus
speed.

Changes since v7:
- Remove unneeded parenthesis in some macro definitions (Uwe)
- Fix some typo (s/KhzkHZ, s/PEC/POC) (Uwe)
- Fix alignment issues in some structures declaration (Uwe)
- Clarify comments to argue i2c_timing values chosen (Uwe)
- Raise an error if parent clk rate is out of scope during I2C hw config (Uwe)
- Use dev_dbg instead of dev_err message when I2C bus is busy  (Uwe)
- Add more comments about stuff done by stm32f4_i2c_handle_rx_btf() (Uwe)
- Simplify stm32f4_i2c_isr_error() routine implementation by removing possible
  status checking (Uwe)
- Rework stm32f4_i2c_isr_error() routine by removing the loop to check which status occured  (Uwe)
- Add open-drain property for SCL pins  (Uwe)
- Rework unneeded mul_ccr field from i2c_timing structure
- Remove min_ccr field from i2c_timing structure as default scl_period is chosen
  to have a correct minimal ccr value
- Execute hw_config once during probe
- Remove soft_reset after an I2C error as all errors are now handled and
  hw_config is done once during probe
- Generate STOP by software when Acknowledge failure occurs
- Set the max speed mode for I2C pins
- Add bias-disable property for I2C pins
- Use intrinsic limitation of APB bus to set I2C max input clk

M'boumba Cedric Madianga (5):
  dt-bindings: Document the STM32 I2C bindings
  i2c: Add STM32F4 I2C driver
  ARM: dts: stm32: Add I2C1 support for STM32F429 SoC
  ARM: dts: stm32: Add I2C1 support for STM32429 eval board
  ARM: configs: stm32: Add I2C support for STM32 defconfig

 .../devicetree/bindings/i2c/i2c-stm32.txt          |  33 +
 arch/arm/boot/dts/stm32429i-eval.dts               |   6 +
 arch/arm/boot/dts/stm32f429.dtsi                   |  23 +
 arch/arm/configs/stm32_defconfig                   |   3 +
 drivers/i2c/busses/Kconfig                         |  10 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-stm32f4.c                   | 866 +++++++++++++++++++++
 7 files changed, 942 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-stm32.txt
 create mode 100644 drivers/i2c/busses/i2c-stm32f4.c

-- 
1.9.1

[PATCH v8 1/5] dt-bindings: Document the STM32 I2C bindings

[M'boumba Cedric Madianga]

This patch adds documentation of device tree bindings for the STM32 I2C
controller.

Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/i2c/i2c-stm32.txt          | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-stm32.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-stm32.txt b/Documentation/devicetree/bindings/i2c/i2c-stm32.txt
new file mode 100644
index 0000000..78eaf7b
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-stm32.txt
@@ -0,0 +1,33 @@
+* I2C controller embedded in STMicroelectronics STM32 I2C platform
+
+Required properties :
+- compatible : Must be "st,stm32f4-i2c"
+- reg : Offset and length of the register set for the device
+- interrupts : Must contain the interrupt id for I2C event and then the
+  interrupt id for I2C error.
+- resets: Must contain the phandle to the reset controller.
+- clocks: Must contain the input clock of the I2C instance.
+- A pinctrl state named "default" must be defined to set pins in mode of
+  operation for I2C transfer
+- #address-cells = <1>;
+- #size-cells = <0>;
+
+Optional properties :
+- clock-frequency : Desired I2C bus clock frequency in Hz. If not specified,
+  the default 100 kHz frequency will be used. As only Normal and Fast modes
+  are supported, possible values are 100000 and 400000.
+
+Example :
+
+	i2c@40005400 {
+		compatible = "st,stm32f4-i2c";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0x40005400 0x400>;
+		interrupts = <31>,
+			     <32>;
+		resets = <&rcc 277>;
+		clocks = <&rcc 0 149>;
+		pinctrl-0 = <&i2c1_sda_pin>, <&i2c1_scl_pin>;
+		pinctrl-names = "default";
+	};
-- 
1.9.1

[PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

This patch adds support for the STM32F4 I2C controller.

Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
---
 drivers/i2c/busses/Kconfig       |  10 +
 drivers/i2c/busses/Makefile      |   1 +
 drivers/i2c/busses/i2c-stm32f4.c | 866 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 877 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-stm32f4.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 0cdc844..2719208 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -886,6 +886,16 @@ config I2C_ST
 	  This driver can also be built as module. If so, the module
 	  will be called i2c-st.
 
+config I2C_STM32F4
+	tristate "STMicroelectronics STM32F4 I2C support"
+	depends on ARCH_STM32 || COMPILE_TEST
+	help
+	  Enable this option to add support for STM32 I2C controller embedded
+	  in STM32F4 SoCs.
+
+	  This driver can also be built as module. If so, the module
+	  will be called i2c-stm32f4.
+
 config I2C_STU300
 	tristate "ST Microelectronics DDC I2C interface"
 	depends on MACH_U300
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 1c1bac8..a2c6ff5 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_I2C_SH_MOBILE)	+= i2c-sh_mobile.o
 obj-$(CONFIG_I2C_SIMTEC)	+= i2c-simtec.o
 obj-$(CONFIG_I2C_SIRF)		+= i2c-sirf.o
 obj-$(CONFIG_I2C_ST)		+= i2c-st.o
+obj-$(CONFIG_I2C_STM32F4)	+= i2c-stm32f4.o
 obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
 obj-$(CONFIG_I2C_SUN6I_P2WI)	+= i2c-sun6i-p2wi.o
 obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
diff --git a/drivers/i2c/busses/i2c-stm32f4.c b/drivers/i2c/busses/i2c-stm32f4.c
new file mode 100644
index 0000000..23e5757
--- /dev/null
+++ b/drivers/i2c/busses/i2c-stm32f4.c
@@ -0,0 +1,866 @@
+/*
+ * Driver for STMicroelectronics STM32 I2C controller
+ *
+ * This I2C controller is described in the STM32F429/439 Soc reference manual.
+ * Please see below a link to the documentation:
+ * http://www.st.com/resource/en/reference_manual/DM00031020.pdf
+ *
+ * Copyright (C) M'boumba Cedric Madianga 2016
+ * Author: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
+ *
+ * This driver is based on i2c-st.c
+ *
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#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/iopoll.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+
+/* STM32F4 I2C offset registers */
+#define STM32F4_I2C_CR1			0x00
+#define STM32F4_I2C_CR2			0x04
+#define STM32F4_I2C_DR			0x10
+#define STM32F4_I2C_SR1			0x14
+#define STM32F4_I2C_SR2			0x18
+#define STM32F4_I2C_CCR			0x1C
+#define STM32F4_I2C_TRISE		0x20
+#define STM32F4_I2C_FLTR		0x24
+
+/* STM32F4 I2C control 1*/
+#define STM32F4_I2C_CR1_POS		BIT(11)
+#define STM32F4_I2C_CR1_ACK		BIT(10)
+#define STM32F4_I2C_CR1_STOP		BIT(9)
+#define STM32F4_I2C_CR1_START		BIT(8)
+#define STM32F4_I2C_CR1_PE		BIT(0)
+
+/* STM32F4 I2C control 2 */
+#define STM32F4_I2C_CR2_FREQ_MASK	GENMASK(5, 0)
+#define STM32F4_I2C_CR2_FREQ(n)		((n) & STM32F4_I2C_CR2_FREQ_MASK)
+#define STM32F4_I2C_CR2_ITBUFEN		BIT(10)
+#define STM32F4_I2C_CR2_ITEVTEN		BIT(9)
+#define STM32F4_I2C_CR2_ITERREN		BIT(8)
+#define STM32F4_I2C_CR2_IRQ_MASK	(STM32F4_I2C_CR2_ITBUFEN | \
+					 STM32F4_I2C_CR2_ITEVTEN | \
+					 STM32F4_I2C_CR2_ITERREN)
+
+/* STM32F4 I2C Status 1 */
+#define STM32F4_I2C_SR1_AF		BIT(10)
+#define STM32F4_I2C_SR1_ARLO		BIT(9)
+#define STM32F4_I2C_SR1_BERR		BIT(8)
+#define STM32F4_I2C_SR1_TXE		BIT(7)
+#define STM32F4_I2C_SR1_RXNE		BIT(6)
+#define STM32F4_I2C_SR1_BTF		BIT(2)
+#define STM32F4_I2C_SR1_ADDR		BIT(1)
+#define STM32F4_I2C_SR1_SB		BIT(0)
+#define STM32F4_I2C_SR1_ITEVTEN_MASK	(STM32F4_I2C_SR1_BTF | \
+					 STM32F4_I2C_SR1_ADDR | \
+					 STM32F4_I2C_SR1_SB)
+#define STM32F4_I2C_SR1_ITBUFEN_MASK	(STM32F4_I2C_SR1_TXE | \
+					 STM32F4_I2C_SR1_RXNE)
+#define STM32F4_I2C_SR1_ITERREN_MASK	(STM32F4_I2C_SR1_AF | \
+					 STM32F4_I2C_SR1_ARLO | \
+					 STM32F4_I2C_SR1_BERR)
+
+/* STM32F4 I2C Status 2 */
+#define STM32F4_I2C_SR2_BUSY		BIT(1)
+
+/* STM32F4 I2C Control Clock */
+#define STM32F4_I2C_CCR_CCR_MASK	GENMASK(11, 0)
+#define STM32F4_I2C_CCR_CCR(n)		((n) & STM32F4_I2C_CCR_CCR_MASK)
+#define STM32F4_I2C_CCR_FS		BIT(15)
+#define STM32F4_I2C_CCR_DUTY		BIT(14)
+
+/* STM32F4 I2C Trise */
+#define STM32F4_I2C_TRISE_VALUE_MASK	GENMASK(5, 0)
+#define STM32F4_I2C_TRISE_VALUE(n)	((n) & STM32F4_I2C_TRISE_VALUE_MASK)
+
+/* STM32F4 I2C Filter */
+#define STM32F4_I2C_FLTR_DNF_MASK	GENMASK(3, 0)
+#define STM32F4_I2C_FLTR_DNF(n)		((n) & STM32F4_I2C_FLTR_DNF_MASK)
+#define STM32F4_I2C_FLTR_ANOFF		BIT(4)
+
+#define STM32F4_I2C_MIN_FREQ		2U
+#define STM32F4_I2C_MAX_FREQ		46U
+#define HZ_TO_MHZ			1000000
+
+enum stm32f4_i2c_speed {
+	STM32F4_I2C_SPEED_STANDARD, /* 100 kHz */
+	STM32F4_I2C_SPEED_FAST, /* 400 kHz */
+	STM32F4_I2C_SPEED_END,
+};
+
+/**
+ * struct stm32f4_i2c_timings - per-Mode tuning parameters
+ * @duty: Fast mode duty cycle
+ * @scl_period: SCL period in microsecond to reach 100kHz/400kHz SCL frequency
+ */
+struct stm32f4_i2c_timings {
+	u32 duty;
+	u32 scl_period;
+};
+
+/**
+ * struct stm32f4_i2c_msg - client specific data
+ * @addr: 8-bit slave addr, including r/w bit
+ * @count: number of bytes to be transferred
+ * @buf: data buffer
+ * @result: result of the transfer
+ * @stop: last I2C msg to be sent, i.e. STOP to be generated
+ */
+struct stm32f4_i2c_msg {
+	u8 addr;
+	u32 count;
+	u8 *buf;
+	int result;
+	bool stop;
+};
+
+/**
+ * struct stm32f4_i2c_dev - private data of the controller
+ * @adap: I2C adapter for this controller
+ * @dev: device for this controller
+ * @base: virtual memory area
+ * @complete: completion of I2C message
+ * @clk: hw i2c clock
+ * speed: I2C clock frequency of the controller. Standard or Fast only supported
+ * @msg: I2C transfer information
+ */
+struct stm32f4_i2c_dev {
+	struct i2c_adapter adap;
+	struct device *dev;
+	void __iomem *base;
+	struct completion complete;
+	struct clk *clk;
+	int speed;
+	struct stm32f4_i2c_msg msg;
+};
+
+/*
+ * In standard mode:
+ * SCL period = SCL high period = SCL low period = CCR * I2C parent clk period
+ *
+ * In fast mode:
+ * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
+ *		SCL low period  = 2  * CCR * I2C parent clk period
+ * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
+ *		SCL low period  = 16 * CCR * I2C parent clk period
+ * In order to reach 400 kHz with lower I2C parent clk frequencies we always set
+ * Duty = 1
+ *
+ * For both modes, we have CCR = SCL period * I2C parent clk frequency
+ * with scl_period = 5 microseconds in Standard mode and scl_period = 1
+ * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
+ * constraints defined by i2c bus specification
+ */
+static struct stm32f4_i2c_timings i2c_timings[] = {
+	[STM32F4_I2C_SPEED_STANDARD] = {
+		.duty			= 0,
+		.scl_period		= 5,
+	},
+	[STM32F4_I2C_SPEED_FAST] = {
+		.duty			= 1,
+		.scl_period		= 1,
+	},
+};
+
+static inline void stm32f4_i2c_set_bits(void __iomem *reg, u32 mask)
+{
+	writel_relaxed(readl_relaxed(reg) | mask, reg);
+}
+
+static inline void stm32f4_i2c_clr_bits(void __iomem *reg, u32 mask)
+{
+	writel_relaxed(readl_relaxed(reg) & ~mask, reg);
+}
+
+static void stm32f4_i2c_disable_irq(struct stm32f4_i2c_dev *i2c_dev)
+{
+	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
+
+	stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_IRQ_MASK);
+}
+
+static int stm32f4_i2c_set_periph_clk_freq(struct stm32f4_i2c_dev *i2c_dev)
+{
+	u32 clk_rate, cr2, freq;
+
+	/*
+	 * The minimum allowed frequency is 2MHz, the maximum APB frequency is
+	 * limited by an intrinsic value of 46 MHz
+	 */
+	clk_rate = clk_get_rate(i2c_dev->clk);
+	freq = DIV_ROUND_UP(clk_rate, HZ_TO_MHZ);
+	if (freq < STM32F4_I2C_MIN_FREQ || freq > STM32F4_I2C_MAX_FREQ) {
+		dev_err(i2c_dev->dev, "out of scope parent clk freq at %dMHz\n",
+			freq);
+		return -EINVAL;
+	}
+
+	cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
+	cr2 &= ~STM32F4_I2C_CR2_FREQ_MASK;
+	cr2 |= STM32F4_I2C_CR2_FREQ(freq);
+	writel_relaxed(cr2, i2c_dev->base + STM32F4_I2C_CR2);
+
+	return 0;
+}
+
+static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
+{
+	u32 trise, freq, cr2;
+
+	/*
+	 * These bits must be programmed with the maximum SCL rise time given in
+	 * the I2C bus specification, incremented by 1.
+	 *
+	 * In standard mode, the maximum allowed SCL rise time is 1000 ns.
+	 * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
+	 * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
+	 * programmed with 09h.(1000 ns / 125 ns = 8 + 1)
+	 * So, for I2C standard mode TRISE = FREQ[5:0] + 1
+	 *
+	 * In fast mode, the maximum allowed SCL rise time is 300 ns.
+	 * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
+	 * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
+	 * programmed with 03h.(300 ns / 125 ns = 2 + 1)
+	 * So, for I2C fast mode TRISE = FREQ[5:0] * 300 / 1000 + 1
+	 */
+
+	cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
+	freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
+
+	if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
+		trise = freq + 1;
+	else
+		trise = freq * 300 / 1000 + 1;
+
+	writel_relaxed(STM32F4_I2C_TRISE_VALUE(trise),
+		       i2c_dev->base + STM32F4_I2C_TRISE);
+}
+
+static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_timings *t = &i2c_timings[i2c_dev->speed];
+	u32 cr2, ccr, freq, val;
+
+	ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
+	ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
+		 STM32F4_I2C_CCR_CCR_MASK);
+
+	/*
+	 * Please see the comments above regarding i2c_timings[] declaration
+	 * to understand the below calculation
+	 */
+	cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
+	freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
+	val = freq * t->scl_period;
+	ccr |= STM32F4_I2C_CCR_CCR(val);
+
+	if (t->duty)
+		ccr |= STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY;
+
+	writel_relaxed(ccr, i2c_dev->base + STM32F4_I2C_CCR);
+}
+
+static void stm32f4_i2c_set_filter(struct stm32f4_i2c_dev *i2c_dev)
+{
+	u32 filter;
+
+	/* Enable analog noise filter and disable digital noise filter */
+	filter = readl_relaxed(i2c_dev->base + STM32F4_I2C_FLTR);
+	filter &= ~(STM32F4_I2C_FLTR_ANOFF | STM32F4_I2C_FLTR_DNF_MASK);
+	writel_relaxed(filter, i2c_dev->base + STM32F4_I2C_FLTR);
+}
+
+/**
+ * stm32f4_i2c_hw_config() - Prepare I2C block
+ * @i2c_dev: Controller's private data
+ */
+static int stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
+{
+	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
+	int ret = 0;
+
+	/* Disable I2C */
+	stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
+
+	ret = stm32f4_i2c_set_periph_clk_freq(i2c_dev);
+	if (ret)
+		return ret;
+
+	stm32f4_i2c_set_rise_time(i2c_dev);
+
+	stm32f4_i2c_set_speed_mode(i2c_dev);
+
+	stm32f4_i2c_set_filter(i2c_dev);
+
+	/* Enable I2C */
+	stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
+
+	return ret;
+}
+
+static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
+{
+	u32 status;
+	int ret;
+
+	ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
+					 status,
+					 !(status & STM32F4_I2C_SR2_BUSY),
+					 10, 1000);
+	if (ret) {
+		dev_dbg(i2c_dev->dev, "bus not free\n");
+		ret = -EBUSY;
+	}
+
+	return ret;
+}
+
+/**
+ * stm32f4_i2c_write_ byte() - Write a byte in the data register
+ * @i2c_dev: Controller's private data
+ * @byte: Data to write in the register
+ */
+static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
+{
+	writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
+}
+
+/**
+ * stm32f4_i2c_write_msg() - Fill the data register in write mode
+ * @i2c_dev: Controller's private data
+ *
+ * This function fills the data register with I2C transfer buffer
+ */
+static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+
+	stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
+	msg->count--;
+}
+
+static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	u32 rbuf;
+
+	rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
+	*msg->buf++ = rbuf & 0xff;
+	msg->count--;
+}
+
+static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
+
+	stm32f4_i2c_disable_irq(i2c_dev);
+
+	reg = i2c_dev->base + STM32F4_I2C_CR1;
+	if (msg->stop)
+		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
+	else
+		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
+
+	complete(&i2c_dev->complete);
+}
+
+/**
+ * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
+ * @i2c_dev: Controller's private data
+ */
+static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
+
+	if (msg->count) {
+		stm32f4_i2c_write_msg(i2c_dev);
+		if (!msg->count) {
+			/* Disable buffer interrupts for RXNE/TXE events */
+			stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
+		}
+	} else {
+		stm32f4_i2c_terminate_xfer(i2c_dev);
+	}
+}
+
+/**
+ * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
+ * @i2c_dev: Controller's private data
+ */
+static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
+
+	switch (msg->count) {
+	case 1:
+		stm32f4_i2c_disable_irq(i2c_dev);
+		stm32f4_i2c_read_msg(i2c_dev);
+		complete(&i2c_dev->complete);
+		break;
+	/*
+	 * For 2 or 3-byte reception, we do not have to read the data register
+	 * when RXNE occurs as we have to wait for byte transferred finished
+	 * event before reading data. So, here we just disable buffer
+	 * interrupt in order to avoid another system preemption due to RXNE
+	 * event
+	 */
+	case 2:
+	case 3:
+		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
+		break;
+	/* For N byte reception with N > 3 we directly read data register */
+	default:
+		stm32f4_i2c_read_msg(i2c_dev);
+	}
+}
+
+/**
+ * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
+ * in case of read
+ * @i2c_dev: Controller's private data
+ */
+static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	void __iomem *reg;
+	u32 mask;
+	int i;
+
+	switch (msg->count) {
+	case 2:
+		/*
+		 * In order to correctly send the Stop or Repeated Start
+		 * condition on the I2C bus, the STOP/START bit has to be set
+		 * before reading the last two bytes.
+		 * After that, we could read the last two bytes, disable
+		 * remaining interrupts and notify the end of xfer to the
+		 * client
+		 */
+		reg = i2c_dev->base + STM32F4_I2C_CR1;
+		if (msg->stop)
+			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
+		else
+			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
+
+		for (i = 2; i > 0; i--)
+			stm32f4_i2c_read_msg(i2c_dev);
+
+		reg = i2c_dev->base + STM32F4_I2C_CR2;
+		mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
+		stm32f4_i2c_clr_bits(reg, mask);
+
+		complete(&i2c_dev->complete);
+		break;
+	case 3:
+		/*
+		 * In order to correctly send the ACK on the I2C bus for the
+		 * last two bytes, we have to set ACK bit before reading the
+		 * third last data byte
+		 */
+		reg = i2c_dev->base + STM32F4_I2C_CR1;
+		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
+		stm32f4_i2c_read_msg(i2c_dev);
+		break;
+	default:
+		stm32f4_i2c_read_msg(i2c_dev);
+	}
+}
+
+/**
+ * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
+ * master receiver
+ * @i2c_dev: Controller's private data
+ */
+static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
+{
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	void __iomem *reg;
+
+	switch (msg->count) {
+	case 0:
+		stm32f4_i2c_terminate_xfer(i2c_dev);
+		/* Clear ADDR flag */
+		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
+		break;
+	case 1:
+		/*
+		 * Single byte reception:
+		 * Enable NACK, clear ADDR flag and generate STOP or RepSTART
+		 */
+		reg = i2c_dev->base + STM32F4_I2C_CR1;
+		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
+		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
+		if (msg->stop)
+			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
+		else
+			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
+		break;
+	case 2:
+		/*
+		 * 2-byte reception:
+		 * Enable NACK and set POS
+		 */
+		reg = i2c_dev->base + STM32F4_I2C_CR1;
+		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
+		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
+		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
+		break;
+
+	default:
+		/* N-byte reception: Enable ACK */
+		reg = i2c_dev->base + STM32F4_I2C_CR1;
+		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
+		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
+		break;
+	}
+}
+
+/**
+ * stm32f4_i2c_isr_event() - Interrupt routine for I2C bus event
+ * @irq: interrupt number
+ * @data: Controller's private data
+ */
+static irqreturn_t stm32f4_i2c_isr_event(int irq, void *data)
+{
+	struct stm32f4_i2c_dev *i2c_dev = data;
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	u32 possible_status = STM32F4_I2C_SR1_ITEVTEN_MASK;
+	void __iomem *reg;
+	u32 status, ien, event;
+
+	ien = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
+	ien &= STM32F4_I2C_CR2_IRQ_MASK;
+
+	/* Update possible_status if buffer interrupt is enabled */
+	if (ien & STM32F4_I2C_CR2_ITBUFEN)
+		possible_status |= STM32F4_I2C_SR1_ITBUFEN_MASK;
+
+	status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
+	event = status & possible_status;
+	if (!event) {
+		dev_dbg(i2c_dev->dev,
+			"spurious evt irq (status=0x%08x, ien=0x%08x)\n",
+			status, ien);
+		return IRQ_NONE;
+	}
+
+	if (event & STM32F4_I2C_SR1_SB)
+		stm32f4_i2c_write_byte(i2c_dev, msg->addr);
+
+	if (event & STM32F4_I2C_SR1_ADDR) {
+		if (msg->addr & I2C_M_RD)
+			stm32f4_i2c_handle_rx_addr(i2c_dev);
+		else
+			readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
+
+		/* Enable buffer interrupts for RXNE/TXE events */
+		reg = i2c_dev->base + STM32F4_I2C_CR2;
+		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
+	}
+
+	if ((event & STM32F4_I2C_SR1_TXE) && !(msg->addr & I2C_M_RD))
+		stm32f4_i2c_handle_write(i2c_dev);
+
+	if ((event & STM32F4_I2C_SR1_RXNE) && (msg->addr & I2C_M_RD))
+		stm32f4_i2c_handle_read(i2c_dev);
+
+	if (event & STM32F4_I2C_SR1_BTF) {
+		if (msg->addr & I2C_M_RD)
+			stm32f4_i2c_handle_rx_btf(i2c_dev);
+		else
+			stm32f4_i2c_handle_write(i2c_dev);
+	}
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * stm32f4_i2c_isr_error() - Interrupt routine for I2C bus error
+ * @irq: interrupt number
+ * @data: Controller's private data
+ */
+static irqreturn_t stm32f4_i2c_isr_error(int irq, void *data)
+{
+	struct stm32f4_i2c_dev *i2c_dev = data;
+	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
+	void __iomem *reg;
+	u32 status;
+
+	status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
+
+	/* Arbitration lost */
+	if (status & STM32F4_I2C_SR1_ARLO) {
+		reg = i2c_dev->base + STM32F4_I2C_SR1;
+		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_ARLO);
+		msg->result = -EAGAIN;
+	}
+
+	/*
+	 * Acknowledge failure:
+	 * In master transmitter mode a Stop must be generated by software
+	 */
+	if (status & STM32F4_I2C_SR1_AF) {
+		if (!(msg->addr & I2C_M_RD)) {
+			reg = i2c_dev->base + STM32F4_I2C_CR1;
+			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
+		}
+		reg = i2c_dev->base + STM32F4_I2C_SR1;
+		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_AF);
+		msg->result = -EIO;
+	}
+
+	/* Bus error */
+	if (status & STM32F4_I2C_SR1_BERR) {
+		reg = i2c_dev->base + STM32F4_I2C_SR1;
+		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_BERR);
+		msg->result = -EIO;
+	}
+
+	stm32f4_i2c_disable_irq(i2c_dev);
+	complete(&i2c_dev->complete);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * stm32f4_i2c_xfer_msg() - Transfer a single I2C message
+ * @i2c_dev: Controller's private data
+ * @msg: I2C message to transfer
+ * @is_first: first message of the sequence
+ * @is_last: last message of the sequence
+ */
+static int stm32f4_i2c_xfer_msg(struct stm32f4_i2c_dev *i2c_dev,
+				struct i2c_msg *msg, bool is_first,
+				bool is_last)
+{
+	struct stm32f4_i2c_msg *f4_msg = &i2c_dev->msg;
+	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
+	unsigned long timeout;
+	u32 mask;
+	int ret;
+
+	f4_msg->addr = i2c_8bit_addr_from_msg(msg);
+	f4_msg->buf = msg->buf;
+	f4_msg->count = msg->len;
+	f4_msg->result = 0;
+	f4_msg->stop = is_last;
+
+	reinit_completion(&i2c_dev->complete);
+
+	/* Enable events and errors interrupts */
+	mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
+	stm32f4_i2c_set_bits(i2c_dev->base + STM32F4_I2C_CR2, mask);
+
+	if (is_first) {
+		ret = stm32f4_i2c_wait_free_bus(i2c_dev);
+		if (ret)
+			return ret;
+
+		/* START generation */
+		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
+	}
+
+	timeout = wait_for_completion_timeout(&i2c_dev->complete,
+					      i2c_dev->adap.timeout);
+	ret = f4_msg->result;
+
+	/* Disable POS position Ack */
+	stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_POS);
+
+	if (!timeout)
+		ret = -ETIMEDOUT;
+
+	return ret;
+}
+
+/**
+ * stm32f4_i2c_xfer() - Transfer combined I2C message
+ * @i2c_adap: Adapter pointer to the controller
+ * @msgs: Pointer to data to be written.
+ * @num: Number of messages to be executed
+ */
+static int stm32f4_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[],
+			    int num)
+{
+	struct stm32f4_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
+	int ret, i;
+
+	ret = clk_enable(i2c_dev->clk);
+	if (ret) {
+		dev_err(i2c_dev->dev, "Failed to enable clock\n");
+		return ret;
+	}
+
+	for (i = 0; i < num && !ret; i++)
+		ret = stm32f4_i2c_xfer_msg(i2c_dev, &msgs[i], i == 0,
+					   i == num - 1);
+
+	clk_disable(i2c_dev->clk);
+
+	return (ret < 0) ? ret : num;
+}
+
+static u32 stm32f4_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm stm32f4_i2c_algo = {
+	.master_xfer = stm32f4_i2c_xfer,
+	.functionality = stm32f4_i2c_func,
+};
+
+static int stm32f4_i2c_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct stm32f4_i2c_dev *i2c_dev;
+	struct resource *res;
+	u32 irq_event, irq_error, clk_rate;
+	struct i2c_adapter *adap;
+	struct reset_control *rst;
+	int ret;
+
+	i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
+	if (!i2c_dev)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	i2c_dev->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(i2c_dev->base))
+		return PTR_ERR(i2c_dev->base);
+
+	irq_event = irq_of_parse_and_map(np, 0);
+	if (!irq_event) {
+		dev_err(&pdev->dev, "IRQ event missing or invalid\n");
+		return -EINVAL;
+	}
+
+	irq_error = irq_of_parse_and_map(np, 1);
+	if (!irq_error) {
+		dev_err(&pdev->dev, "IRQ error missing or invalid\n");
+		return -EINVAL;
+	}
+
+	i2c_dev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(i2c_dev->clk)) {
+		dev_err(&pdev->dev, "Error: Missing controller clock\n");
+		return PTR_ERR(i2c_dev->clk);
+	}
+	ret = clk_prepare_enable(i2c_dev->clk);
+	if (ret) {
+		dev_err(i2c_dev->dev, "Failed to prepare_enable clock\n");
+		return ret;
+	}
+
+	rst = devm_reset_control_get(&pdev->dev, NULL);
+	if (IS_ERR(rst)) {
+		dev_err(&pdev->dev, "Error: Missing controller reset\n");
+		ret = PTR_ERR(rst);
+		goto clk_free;
+	}
+	reset_control_assert(rst);
+	udelay(2);
+	reset_control_deassert(rst);
+
+	i2c_dev->speed = STM32F4_I2C_SPEED_STANDARD;
+	ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
+	if (!ret && clk_rate >= 40000)
+		i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
+
+	i2c_dev->dev = &pdev->dev;
+
+	ret = devm_request_irq(&pdev->dev, irq_event, stm32f4_i2c_isr_event, 0,
+			       pdev->name, i2c_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request irq event %i\n",
+			irq_event);
+		goto clk_free;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq_error, stm32f4_i2c_isr_error, 0,
+			       pdev->name, i2c_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request irq error %i\n",
+			irq_error);
+		goto clk_free;
+	}
+
+	ret = stm32f4_i2c_hw_config(i2c_dev);
+	if (ret)
+		goto clk_free;
+
+	adap = &i2c_dev->adap;
+	i2c_set_adapdata(adap, i2c_dev);
+	snprintf(adap->name, sizeof(adap->name), "STM32 I2C(%pa)", &res->start);
+	adap->owner = THIS_MODULE;
+	adap->timeout = 2 * HZ;
+	adap->retries = 0;
+	adap->algo = &stm32f4_i2c_algo;
+	adap->dev.parent = &pdev->dev;
+	adap->dev.of_node = pdev->dev.of_node;
+
+	init_completion(&i2c_dev->complete);
+
+	ret = i2c_add_adapter(adap);
+	if (ret)
+		goto clk_free;
+
+	platform_set_drvdata(pdev, i2c_dev);
+
+	clk_disable(i2c_dev->clk);
+
+	dev_info(i2c_dev->dev, "STM32F4 I2C driver registered\n");
+
+	return 0;
+
+clk_free:
+	clk_disable_unprepare(i2c_dev->clk);
+	return ret;
+}
+
+static int stm32f4_i2c_remove(struct platform_device *pdev)
+{
+	struct stm32f4_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c_dev->adap);
+
+	clk_unprepare(i2c_dev->clk);
+
+	return 0;
+}
+
+static const struct of_device_id stm32f4_i2c_match[] = {
+	{ .compatible = "st,stm32f4-i2c", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, stm32f4_i2c_match);
+
+static struct platform_driver stm32f4_i2c_driver = {
+	.driver = {
+		.name = "stm32f4-i2c",
+		.of_match_table = stm32f4_i2c_match,
+	},
+	.probe = stm32f4_i2c_probe,
+	.remove = stm32f4_i2c_remove,
+};
+
+module_platform_driver(stm32f4_i2c_driver);
+
+MODULE_AUTHOR("M'boumba Cedric Madianga <cedric.madianga@gmail.com>");
+MODULE_DESCRIPTION("STMicroelectronics STM32F4 I2C driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v8 3/5] ARM: dts: stm32: Add I2C1 support for STM32F429 SoC

[M'boumba Cedric Madianga]

This patch adds I2C1 support for STM32F429 SoC

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
---
 arch/arm/boot/dts/stm32f429.dtsi | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index e4dae0e..5b063e9 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -48,6 +48,7 @@
 #include "skeleton.dtsi"
 #include "armv7-m.dtsi"
 #include <dt-bindings/pinctrl/stm32f429-pinfunc.h>
+#include <dt-bindings/mfd/stm32f4-rcc.h>
 
 / {
 	clocks {
@@ -153,6 +154,18 @@
 			status = "disabled";
 		};
 
+		i2c1: i2c@40005400 {
+			compatible = "st,stm32f4-i2c";
+			reg = <0x40005400 0x400>;
+			interrupts = <31>,
+				     <32>;
+			resets = <&rcc STM32F4_APB1_RESET(I2C1)>;
+			clocks = <&rcc 0 STM32F4_APB1_CLOCK(I2C1)>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
 		usart7: serial@40007800 {
 			compatible = "st,stm32-usart", "st,stm32-uart";
 			reg = <0x40007800 0x400>;
@@ -355,6 +368,16 @@
 					slew-rate = <2>;
 				};
 			};
+
+			i2c1_pins_b: i2c1@0 {
+				pins {
+					pinmux = <STM32F429_PB9_FUNC_I2C1_SDA>,
+						 <STM32F429_PB6_FUNC_I2C1_SCL>;
+					bias-disable;
+					drive-open-drain;
+					slew-rate = <3>;
+				};
+			};
 		};
 
 		rcc: rcc@40023810 {
-- 
1.9.1

[PATCH v8 4/5] ARM: dts: stm32: Add I2C1 support for STM32429 eval board

[M'boumba Cedric Madianga]

This patch adds I2C1 instance support for STM32x9I-Eval board.

Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
---
 arch/arm/boot/dts/stm32429i-eval.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/stm32429i-eval.dts b/arch/arm/boot/dts/stm32429i-eval.dts
index 76f7206..c943539 100644
--- a/arch/arm/boot/dts/stm32429i-eval.dts
+++ b/arch/arm/boot/dts/stm32429i-eval.dts
@@ -146,3 +146,9 @@
 	pinctrl-names = "default";
 	status = "okay";
 };
+
+&i2c1 {
+	pinctrl-0 = <&i2c1_pins_b>;
+	pinctrl-names = "default";
+	status = "okay";
+};
-- 
1.9.1

[PATCH v8 5/5] ARM: configs: stm32: Add I2C support for STM32 defconfig

[M'boumba Cedric Madianga]

This patch adds I2C support for STM32 default configuration

Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
---
 arch/arm/configs/stm32_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/configs/stm32_defconfig b/arch/arm/configs/stm32_defconfig
index 5a72d69..323d2a3 100644
--- a/arch/arm/configs/stm32_defconfig
+++ b/arch/arm/configs/stm32_defconfig
@@ -47,6 +47,9 @@ CONFIG_SERIAL_NONSTANDARD=y
 CONFIG_SERIAL_STM32=y
 CONFIG_SERIAL_STM32_CONSOLE=y
 # CONFIG_HW_RANDOM is not set
+CONFIG_I2C=y
+CONFIG_I2C_CHARDEV=y
+CONFIG_I2C_STM32F4=y
 # CONFIG_HWMON is not set
 # CONFIG_USB_SUPPORT is not set
 CONFIG_NEW_LEDS=y
-- 
1.9.1

Re: [PATCH v8 0/5] Add support for the STM32F4 I2C

[Linus Walleij]

On Thu, Jan 5, 2017 at 10:07 AM, M'boumba Cedric Madianga
<cedric.madianga@gmail.com> wrote:

> This patchset adds support for the I2C controller embedded in STM32F4xx SoC.
> It enables I2C transfer in interrupt mode with Standard-mode and Fast-mode bus
> speed.

The whole series:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Looks perfect from my point of view now.

Yours,
Linus Walleij

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

Hello Cedric,

On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
> +/*
> + * In standard mode:
> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk period
> + *
> + * In fast mode:
> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
> + *		SCL low period  = 2  * CCR * I2C parent clk period
> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
> + *		SCL low period  = 16 * CCR * I2C parent clk period
s/  \*/ */ several times

> + * In order to reach 400 kHz with lower I2C parent clk frequencies we always set
> + * Duty = 1
> + *
> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
s/mode/Mode/

> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
> + * constraints defined by i2c bus specification

I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
somewhere?

> + */
> +static struct stm32f4_i2c_timings i2c_timings[] = {
> [...]
> +
> +/**
> + * stm32f4_i2c_hw_config() - Prepare I2C block
> + * @i2c_dev: Controller's private data
> + */
> +static int stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
> +	int ret = 0;
> +
> +	/* Disable I2C */
> +	stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
> +
> +	ret = stm32f4_i2c_set_periph_clk_freq(i2c_dev);
> +	if (ret)
> +		return ret;
> +
> +	stm32f4_i2c_set_rise_time(i2c_dev);
> +
> +	stm32f4_i2c_set_speed_mode(i2c_dev);
> +
> +	stm32f4_i2c_set_filter(i2c_dev);
> +
> +	/* Enable I2C */
> +	stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);

This function is called after a hw reset, so there should be no need to
use clr_bits and set_bits because the value read from hw should be
known.

> +	return ret;

return 0;

> +}
> +
> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	u32 status;
> +	int ret;
> +
> +	ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
> +					 status,
> +					 !(status & STM32F4_I2C_SR2_BUSY),
> +					 10, 1000);
> +	if (ret) {
> +		dev_dbg(i2c_dev->dev, "bus not free\n");
> +		ret = -EBUSY;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
> + * @i2c_dev: Controller's private data
> + * @byte: Data to write in the register
> + */
> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
> +{
> +	writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
> +}
> +
> +/**
> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
> + * @i2c_dev: Controller's private data
> + *
> + * This function fills the data register with I2C transfer buffer
> + */
> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> +
> +	stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
> +	msg->count--;
> +}
> +
> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> +	u32 rbuf;
> +
> +	rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
> +	*msg->buf++ = rbuf & 0xff;

This is unnecessary. buf has an 8 bit wide type so

	*msg->buf++ = rbuf;

has the same effect. (ISTR this is something I already pointed out
earlier?)

> +	msg->count--;
> +}
> +
> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> +	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> +	stm32f4_i2c_disable_irq(i2c_dev);
> +
> +	reg = i2c_dev->base + STM32F4_I2C_CR1;
> +	if (msg->stop)
> +		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> +	else
> +		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> +
> +	complete(&i2c_dev->complete);
> +}
> +
> +/**
> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> +	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> +	if (msg->count) {
> +		stm32f4_i2c_write_msg(i2c_dev);
> +		if (!msg->count) {
> +			/* Disable buffer interrupts for RXNE/TXE events */
> +			stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> +		}
> +	} else {
> +		stm32f4_i2c_terminate_xfer(i2c_dev);

Is stm32f4_i2c_terminate_xfer also called when arbitration is lost? If
yes, is it then right to set STM32F4_I2C_CR1_STOP or
STM32F4_I2C_CR1_START?

> +	}
> +}
> +
> +/**
> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> +	void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> +	switch (msg->count) {
> +	case 1:
> +		stm32f4_i2c_disable_irq(i2c_dev);
> +		stm32f4_i2c_read_msg(i2c_dev);
> +		complete(&i2c_dev->complete);
> +		break;
> +	/*
> +	 * For 2 or 3-byte reception, we do not have to read the data register
> +	 * when RXNE occurs as we have to wait for byte transferred finished

it's hard to understand because if you don't know the hardware the
meaning of RXNE is unknown.

> +	 * event before reading data. So, here we just disable buffer
> +	 * interrupt in order to avoid another system preemption due to RXNE
> +	 * event
> +	 */
> +	case 2:
> +	case 3:
> +		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> +		break;
> +	/* For N byte reception with N > 3 we directly read data register */
> +	default:
> +		stm32f4_i2c_read_msg(i2c_dev);
> +	}
> +}
> +
> +/**
> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
> + * in case of read
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
> +{

btf is a hw-related name. Maybe better use _done which is easier to
understand?

> +	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> +	void __iomem *reg;
> +	u32 mask;
> +	int i;
> +
> +	switch (msg->count) {
> +	case 2:
> +		/*
> +		 * In order to correctly send the Stop or Repeated Start
> +		 * condition on the I2C bus, the STOP/START bit has to be set
> +		 * before reading the last two bytes.
> +		 * After that, we could read the last two bytes, disable
> +		 * remaining interrupts and notify the end of xfer to the
> +		 * client

This is surprising. I didn't recheck the manual, but that looks very
uncomfortable. How does this work, when I only want to read a single
byte? Same problem for ACK below.

> +		 */
> +		reg = i2c_dev->base + STM32F4_I2C_CR1;
> +		if (msg->stop)
> +			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> +		else
> +			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> +
> +		for (i = 2; i > 0; i--)
> +			stm32f4_i2c_read_msg(i2c_dev);
> +
> +		reg = i2c_dev->base + STM32F4_I2C_CR2;
> +		mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
> +		stm32f4_i2c_clr_bits(reg, mask);
> +
> +		complete(&i2c_dev->complete);
> +		break;
> +	case 3:
> +		/*
> +		 * In order to correctly send the ACK on the I2C bus for the
> +		 * last two bytes, we have to set ACK bit before reading the
> +		 * third last data byte
> +		 */
> +		reg = i2c_dev->base + STM32F4_I2C_CR1;
> +		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> +		stm32f4_i2c_read_msg(i2c_dev);
> +		break;
> +	default:
> +		stm32f4_i2c_read_msg(i2c_dev);
> +	}
> +}
> +
> +/**
> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
> + * master receiver
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
> +{
> +	struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> +	void __iomem *reg;
> +
> +	switch (msg->count) {
> +	case 0:
> +		stm32f4_i2c_terminate_xfer(i2c_dev);
> +		/* Clear ADDR flag */
> +		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> +		break;
> +	case 1:
> +		/*
> +		 * Single byte reception:

This also happens for the last byte of a 5 byte transfer, right?

> +		 * Enable NACK, clear ADDR flag and generate STOP or RepSTART
> +		 */
> +		reg = i2c_dev->base + STM32F4_I2C_CR1;
> +		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> +		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> +		if (msg->stop)
> +			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> +		else
> +			stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> +		break;
> +	case 2:
> +		/*
> +		 * 2-byte reception:
> +		 * Enable NACK and set POS

What is POS?

> +		 */
> +		reg = i2c_dev->base + STM32F4_I2C_CR1;
> +		stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> +		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);

You could get rid of this, when caching the value of CR1. Would save two
register reads here. This doesn't work for all registers, but it should
be possible to apply for most of them, maybe enough to get rid of the
clr_bits and set_bits function.

> +		readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> +		break;
> +
> +	default:
> +		/* N-byte reception: Enable ACK */
> +		reg = i2c_dev->base + STM32F4_I2C_CR1;
> +		stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);

Do you need to set ACK for each byte transferred?

I stopp reviewing here because of -ENOTIME on my side but don't want to
delay discussion, so sent my comments up to here already now.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

Hi Uwe,

2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello Cedric,
>
> On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
>> +/*
>> + * In standard mode:
>> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk period
>> + *
>> + * In fast mode:
>> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
>> + *           SCL low period  = 2  * CCR * I2C parent clk period
>> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
>> + *           SCL low period  = 16 * CCR * I2C parent clk period
> s/  \*/ */ several times

Sorry but I don't see where is the issue as the style for multi-line
comments seems ok.
Could you please clarify that point if possible ? Thanks in advance

>
>> + * In order to reach 400 kHz with lower I2C parent clk frequencies we always set
>> + * Duty = 1
>> + *
>> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
>> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
> s/mode/Mode/

ok thanks

>
>> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
>> + * constraints defined by i2c bus specification
>
> I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
> of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
> somewhere?

As CCR = SCL_period * I2C parent clk frequency with minimal freq =
2Mhz and SCL_period = 1 we have:
CCR = 1 * 2Mhz = 2.
But to compute, scl_low and scl_high in Fast mode, we have to do the
following thing as Duty=1:
scl_high = 9 * CCR * I2C parent clk period
scl_low = 16 * CCR * I2C parent clk period
In our example:
scl_high = 9 * 2 * 0,0000005 = 0,000009 sec = 9 µs
scl_low = 16 * 2 * 0.0000005 = 0,000016 sec = 16 µs
So low + high = 27 µs > 2,5 µs

>
>> + */
>> +static struct stm32f4_i2c_timings i2c_timings[] = {
>> [...]
>> +
>> +/**
>> + * stm32f4_i2c_hw_config() - Prepare I2C block
>> + * @i2c_dev: Controller's private data
>> + */
>> +static int stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +     int ret = 0;
>> +
>> +     /* Disable I2C */
>> +     stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>> +
>> +     ret = stm32f4_i2c_set_periph_clk_freq(i2c_dev);
>> +     if (ret)
>> +             return ret;
>> +
>> +     stm32f4_i2c_set_rise_time(i2c_dev);
>> +
>> +     stm32f4_i2c_set_speed_mode(i2c_dev);
>> +
>> +     stm32f4_i2c_set_filter(i2c_dev);
>> +
>> +     /* Enable I2C */
>> +     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
>
> This function is called after a hw reset, so there should be no need to
> use clr_bits and set_bits because the value read from hw should be
> known.

ok thanks

>
>> +     return ret;
>
> return 0;

ok thanks

>
>> +}
>> +
>> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     u32 status;
>> +     int ret;
>> +
>> +     ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>> +                                      status,
>> +                                      !(status & STM32F4_I2C_SR2_BUSY),
>> +                                      10, 1000);
>> +     if (ret) {
>> +             dev_dbg(i2c_dev->dev, "bus not free\n");
>> +             ret = -EBUSY;
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
>> + * @i2c_dev: Controller's private data
>> + * @byte: Data to write in the register
>> + */
>> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
>> +{
>> +     writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
>> + * @i2c_dev: Controller's private data
>> + *
>> + * This function fills the data register with I2C transfer buffer
>> + */
>> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> +
>> +     stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
>> +     msg->count--;
>> +}
>> +
>> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> +     u32 rbuf;
>> +
>> +     rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
>> +     *msg->buf++ = rbuf & 0xff;
>
> This is unnecessary. buf has an 8 bit wide type so
>
>         *msg->buf++ = rbuf;
>
> has the same effect. (ISTR this is something I already pointed out
> earlier?)

Yes you are right.

>
>> +     msg->count--;
>> +}
>> +
>> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> +     stm32f4_i2c_disable_irq(i2c_dev);
>> +
>> +     reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +     if (msg->stop)
>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> +     else
>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> +
>> +     complete(&i2c_dev->complete);
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> +     if (msg->count) {
>> +             stm32f4_i2c_write_msg(i2c_dev);
>> +             if (!msg->count) {
>> +                     /* Disable buffer interrupts for RXNE/TXE events */
>> +                     stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> +             }
>> +     } else {
>> +             stm32f4_i2c_terminate_xfer(i2c_dev);
>
> Is stm32f4_i2c_terminate_xfer also called when arbitration is lost? If
> yes, is it then right to set STM32F4_I2C_CR1_STOP or
> STM32F4_I2C_CR1_START?

If arbitration is lost, stm32f4_i2c_terminate_xfer() is not called.
In that case, we return -EAGAIN and i2c-core will retry by calling
stm32f4_i2c_xfer()

>
>> +     }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> +     switch (msg->count) {
>> +     case 1:
>> +             stm32f4_i2c_disable_irq(i2c_dev);
>> +             stm32f4_i2c_read_msg(i2c_dev);
>> +             complete(&i2c_dev->complete);
>> +             break;
>> +     /*
>> +      * For 2 or 3-byte reception, we do not have to read the data register
>> +      * when RXNE occurs as we have to wait for byte transferred finished
>
> it's hard to understand because if you don't know the hardware the
> meaning of RXNE is unknown.

Ok I will replace RXNE by RX not empty in that comment

>
>> +      * event before reading data. So, here we just disable buffer
>> +      * interrupt in order to avoid another system preemption due to RXNE
>> +      * event
>> +      */
>> +     case 2:
>> +     case 3:
>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> +             break;
>> +     /* For N byte reception with N > 3 we directly read data register */
>> +     default:
>> +             stm32f4_i2c_read_msg(i2c_dev);
>> +     }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
>> + * in case of read
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>
> btf is a hw-related name. Maybe better use _done which is easier to
> understand?

OK

>
>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> +     void __iomem *reg;
>> +     u32 mask;
>> +     int i;
>> +
>> +     switch (msg->count) {
>> +     case 2:
>> +             /*
>> +              * In order to correctly send the Stop or Repeated Start
>> +              * condition on the I2C bus, the STOP/START bit has to be set
>> +              * before reading the last two bytes.
>> +              * After that, we could read the last two bytes, disable
>> +              * remaining interrupts and notify the end of xfer to the
>> +              * client
>
> This is surprising. I didn't recheck the manual, but that looks very
> uncomfortable.

I agree but this exactly the hardware way of working described in the
reference manual.

>How does this work, when I only want to read a single
> byte? Same problem for ACK below.

For a single reception, we enable NACK and STOP or Repeatead START
bits during address match.
The NACK and STOP/START pulses are sent as soon as the data is
received in the shift register.
Please note that in that case, we don't have to wait BTF event to read the data.
Data is read as soon as RXNE event occurs.

>
>> +              */
>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +             if (msg->stop)
>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> +             else
>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> +
>> +             for (i = 2; i > 0; i--)
>> +                     stm32f4_i2c_read_msg(i2c_dev);
>> +
>> +             reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +             mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
>> +             stm32f4_i2c_clr_bits(reg, mask);
>> +
>> +             complete(&i2c_dev->complete);
>> +             break;
>> +     case 3:
>> +             /*
>> +              * In order to correctly send the ACK on the I2C bus for the
>> +              * last two bytes, we have to set ACK bit before reading the
>> +              * third last data byte
>> +              */
>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> +             stm32f4_i2c_read_msg(i2c_dev);
>> +             break;
>> +     default:
>> +             stm32f4_i2c_read_msg(i2c_dev);
>> +     }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
>> + * master receiver
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> +     void __iomem *reg;
>> +
>> +     switch (msg->count) {
>> +     case 0:
>> +             stm32f4_i2c_terminate_xfer(i2c_dev);
>> +             /* Clear ADDR flag */
>> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> +             break;
>> +     case 1:
>> +             /*
>> +              * Single byte reception:
>
> This also happens for the last byte of a 5 byte transfer, right?

For a 5 byte transfer the behavior is different:
We have to read data from DR (data register)  as soon as the RXNE (RX
not empty) event occurs for data1, data2 and data3 (until N-2 data for
a more generic case)
The ACK is automatically sent as soon as the data is received in the
shift register as the I2C controller was configured to do that during
adress match phase.

For data3 (N-2 data), we wait for BTF (Byte Transfer finished) event
in order to set NACK before reading DR.
This event occurs when a new data has been received in shift register
(in our case data4 or N-1 data) but the prevoius data in DR (in our
case data3 or N-2 data) has not been read yet.
In that way, the NACK pulse will be correctly generated after the last
received data byte.

For data4 and data5, we wait for BTF event (data4 or N-1 data in DR
and data5 or N data in shift register), set STOP or repeated Start in
order to correctly sent the right pulse after the last received data
byte and run 2 consecutives read of DR.

>
>> +              * Enable NACK, clear ADDR flag and generate STOP or RepSTART
>> +              */
>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> +             if (msg->stop)
>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> +             else
>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> +             break;
>> +     case 2:
>> +             /*
>> +              * 2-byte reception:
>> +              * Enable NACK and set POS
>
> What is POS?
POS is used to define the position of the (N)ACK pulse
0: ACK is generated when the current is being received in the shift register
1: ACK is generated when the next byte which will be received in the
shift register (used for 2-byte reception)

>
>> +              */
>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>
> You could get rid of this, when caching the value of CR1. Would save two
> register reads here. This doesn't work for all registers, but it should
> be possible to apply for most of them, maybe enough to get rid of the
> clr_bits and set_bits function.
>
>> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> +             break;
>> +
>> +     default:
>> +             /* N-byte reception: Enable ACK */
>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
>
> Do you need to set ACK for each byte transferred?
I need to do that in order to be SMBus compatible and the ACK/NACK
seems to be used by default in Documentation/i2c/i2c-protocol file.

>
> I stopp reviewing here because of -ENOTIME on my side but don't want to
> delay discussion, so sent my comments up to here already now.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

>
>> +              */
>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>
> You could get rid of this, when caching the value of CR1. Would save two
> register reads here. This doesn't work for all registers, but it should
> be possible to apply for most of them, maybe enough to get rid of the
> clr_bits and set_bits function.

I agree at many places I could save registers read by not using
clr_bits and set_bits function when the registers in question has been
already read.
But it is not enough to get rid of the clr_bits and set_bits function.
For example when calling stm32f4_i2c_terminate_xfer(), the CR1
register is never read before so set_bits function is useful.
Another example, when stm32f4_i2c_handle_rx_done(), the CR1 register
is also never read before so clr_bits finction is again useful.

2017-01-11 14:58 GMT+01:00 M'boumba Cedric Madianga <cedric.madianga@gmail.com>:
> Hi Uwe,
>
> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> Hello Cedric,
>>
>> On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
>>> +/*
>>> + * In standard mode:
>>> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk period
>>> + *
>>> + * In fast mode:
>>> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
>>> + *           SCL low period  = 2  * CCR * I2C parent clk period
>>> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
>>> + *           SCL low period  = 16 * CCR * I2C parent clk period
>> s/  \*/ */ several times
>
> Sorry but I don't see where is the issue as the style for multi-line
> comments seems ok.
> Could you please clarify that point if possible ? Thanks in advance
>
>>
>>> + * In order to reach 400 kHz with lower I2C parent clk frequencies we always set
>>> + * Duty = 1
>>> + *
>>> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
>>> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
>> s/mode/Mode/
>
> ok thanks
>
>>
>>> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
>>> + * constraints defined by i2c bus specification
>>
>> I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
>> of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
>> somewhere?
>
> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
> 2Mhz and SCL_period = 1 we have:
> CCR = 1 * 2Mhz = 2.
> But to compute, scl_low and scl_high in Fast mode, we have to do the
> following thing as Duty=1:
> scl_high = 9 * CCR * I2C parent clk period
> scl_low = 16 * CCR * I2C parent clk period
> In our example:
> scl_high = 9 * 2 * 0,0000005 = 0,000009 sec = 9 µs
> scl_low = 16 * 2 * 0.0000005 = 0,000016 sec = 16 µs
> So low + high = 27 µs > 2,5 µs
>
>>
>>> + */
>>> +static struct stm32f4_i2c_timings i2c_timings[] = {
>>> [...]
>>> +
>>> +/**
>>> + * stm32f4_i2c_hw_config() - Prepare I2C block
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static int stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> +     int ret = 0;
>>> +
>>> +     /* Disable I2C */
>>> +     stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>>> +
>>> +     ret = stm32f4_i2c_set_periph_clk_freq(i2c_dev);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     stm32f4_i2c_set_rise_time(i2c_dev);
>>> +
>>> +     stm32f4_i2c_set_speed_mode(i2c_dev);
>>> +
>>> +     stm32f4_i2c_set_filter(i2c_dev);
>>> +
>>> +     /* Enable I2C */
>>> +     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
>>
>> This function is called after a hw reset, so there should be no need to
>> use clr_bits and set_bits because the value read from hw should be
>> known.
>
> ok thanks
>
>>
>>> +     return ret;
>>
>> return 0;
>
> ok thanks
>
>>
>>> +}
>>> +
>>> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     u32 status;
>>> +     int ret;
>>> +
>>> +     ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>>> +                                      status,
>>> +                                      !(status & STM32F4_I2C_SR2_BUSY),
>>> +                                      10, 1000);
>>> +     if (ret) {
>>> +             dev_dbg(i2c_dev->dev, "bus not free\n");
>>> +             ret = -EBUSY;
>>> +     }
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
>>> + * @i2c_dev: Controller's private data
>>> + * @byte: Data to write in the register
>>> + */
>>> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
>>> +{
>>> +     writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
>>> + * @i2c_dev: Controller's private data
>>> + *
>>> + * This function fills the data register with I2C transfer buffer
>>> + */
>>> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +
>>> +     stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
>>> +     msg->count--;
>>> +}
>>> +
>>> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +     u32 rbuf;
>>> +
>>> +     rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
>>> +     *msg->buf++ = rbuf & 0xff;
>>
>> This is unnecessary. buf has an 8 bit wide type so
>>
>>         *msg->buf++ = rbuf;
>>
>> has the same effect. (ISTR this is something I already pointed out
>> earlier?)
>
> Yes you are right.
>
>>
>>> +     msg->count--;
>>> +}
>>> +
>>> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>>> +
>>> +     stm32f4_i2c_disable_irq(i2c_dev);
>>> +
>>> +     reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> +     if (msg->stop)
>>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>>> +     else
>>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>>> +
>>> +     complete(&i2c_dev->complete);
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>>> +
>>> +     if (msg->count) {
>>> +             stm32f4_i2c_write_msg(i2c_dev);
>>> +             if (!msg->count) {
>>> +                     /* Disable buffer interrupts for RXNE/TXE events */
>>> +                     stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>>> +             }
>>> +     } else {
>>> +             stm32f4_i2c_terminate_xfer(i2c_dev);
>>
>> Is stm32f4_i2c_terminate_xfer also called when arbitration is lost? If
>> yes, is it then right to set STM32F4_I2C_CR1_STOP or
>> STM32F4_I2C_CR1_START?
>
> If arbitration is lost, stm32f4_i2c_terminate_xfer() is not called.
> In that case, we return -EAGAIN and i2c-core will retry by calling
> stm32f4_i2c_xfer()
>
>>
>>> +     }
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>>> +
>>> +     switch (msg->count) {
>>> +     case 1:
>>> +             stm32f4_i2c_disable_irq(i2c_dev);
>>> +             stm32f4_i2c_read_msg(i2c_dev);
>>> +             complete(&i2c_dev->complete);
>>> +             break;
>>> +     /*
>>> +      * For 2 or 3-byte reception, we do not have to read the data register
>>> +      * when RXNE occurs as we have to wait for byte transferred finished
>>
>> it's hard to understand because if you don't know the hardware the
>> meaning of RXNE is unknown.
>
> Ok I will replace RXNE by RX not empty in that comment
>
>>
>>> +      * event before reading data. So, here we just disable buffer
>>> +      * interrupt in order to avoid another system preemption due to RXNE
>>> +      * event
>>> +      */
>>> +     case 2:
>>> +     case 3:
>>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>>> +             break;
>>> +     /* For N byte reception with N > 3 we directly read data register */
>>> +     default:
>>> +             stm32f4_i2c_read_msg(i2c_dev);
>>> +     }
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
>>> + * in case of read
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>
>> btf is a hw-related name. Maybe better use _done which is easier to
>> understand?
>
> OK
>
>>
>>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +     void __iomem *reg;
>>> +     u32 mask;
>>> +     int i;
>>> +
>>> +     switch (msg->count) {
>>> +     case 2:
>>> +             /*
>>> +              * In order to correctly send the Stop or Repeated Start
>>> +              * condition on the I2C bus, the STOP/START bit has to be set
>>> +              * before reading the last two bytes.
>>> +              * After that, we could read the last two bytes, disable
>>> +              * remaining interrupts and notify the end of xfer to the
>>> +              * client
>>
>> This is surprising. I didn't recheck the manual, but that looks very
>> uncomfortable.
>
> I agree but this exactly the hardware way of working described in the
> reference manual.
>
>>How does this work, when I only want to read a single
>> byte? Same problem for ACK below.
>
> For a single reception, we enable NACK and STOP or Repeatead START
> bits during address match.
> The NACK and STOP/START pulses are sent as soon as the data is
> received in the shift register.
> Please note that in that case, we don't have to wait BTF event to read the data.
> Data is read as soon as RXNE event occurs.
>
>>
>>> +              */
>>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> +             if (msg->stop)
>>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>>> +             else
>>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>>> +
>>> +             for (i = 2; i > 0; i--)
>>> +                     stm32f4_i2c_read_msg(i2c_dev);
>>> +
>>> +             reg = i2c_dev->base + STM32F4_I2C_CR2;
>>> +             mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
>>> +             stm32f4_i2c_clr_bits(reg, mask);
>>> +
>>> +             complete(&i2c_dev->complete);
>>> +             break;
>>> +     case 3:
>>> +             /*
>>> +              * In order to correctly send the ACK on the I2C bus for the
>>> +              * last two bytes, we have to set ACK bit before reading the
>>> +              * third last data byte
>>> +              */
>>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>>> +             stm32f4_i2c_read_msg(i2c_dev);
>>> +             break;
>>> +     default:
>>> +             stm32f4_i2c_read_msg(i2c_dev);
>>> +     }
>>> +}
>>> +
>>> +/**
>>> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
>>> + * master receiver
>>> + * @i2c_dev: Controller's private data
>>> + */
>>> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
>>> +{
>>> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>>> +     void __iomem *reg;
>>> +
>>> +     switch (msg->count) {
>>> +     case 0:
>>> +             stm32f4_i2c_terminate_xfer(i2c_dev);
>>> +             /* Clear ADDR flag */
>>> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>>> +             break;
>>> +     case 1:
>>> +             /*
>>> +              * Single byte reception:
>>
>> This also happens for the last byte of a 5 byte transfer, right?
>
> For a 5 byte transfer the behavior is different:
> We have to read data from DR (data register)  as soon as the RXNE (RX
> not empty) event occurs for data1, data2 and data3 (until N-2 data for
> a more generic case)
> The ACK is automatically sent as soon as the data is received in the
> shift register as the I2C controller was configured to do that during
> adress match phase.
>
> For data3 (N-2 data), we wait for BTF (Byte Transfer finished) event
> in order to set NACK before reading DR.
> This event occurs when a new data has been received in shift register
> (in our case data4 or N-1 data) but the prevoius data in DR (in our
> case data3 or N-2 data) has not been read yet.
> In that way, the NACK pulse will be correctly generated after the last
> received data byte.
>
> For data4 and data5, we wait for BTF event (data4 or N-1 data in DR
> and data5 or N data in shift register), set STOP or repeated Start in
> order to correctly sent the right pulse after the last received data
> byte and run 2 consecutives read of DR.
>
>>
>>> +              * Enable NACK, clear ADDR flag and generate STOP or RepSTART
>>> +              */
>>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>>> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>>> +             if (msg->stop)
>>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>>> +             else
>>> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>>> +             break;
>>> +     case 2:
>>> +             /*
>>> +              * 2-byte reception:
>>> +              * Enable NACK and set POS
>>
>> What is POS?
> POS is used to define the position of the (N)ACK pulse
> 0: ACK is generated when the current is being received in the shift register
> 1: ACK is generated when the next byte which will be received in the
> shift register (used for 2-byte reception)
>
>>
>>> +              */
>>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>>
>> You could get rid of this, when caching the value of CR1. Would save two
>> register reads here. This doesn't work for all registers, but it should
>> be possible to apply for most of them, maybe enough to get rid of the
>> clr_bits and set_bits function.
>>
>>> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>>> +             break;
>>> +
>>> +     default:
>>> +             /* N-byte reception: Enable ACK */
>>> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>>> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
>>
>> Do you need to set ACK for each byte transferred?
> I need to do that in order to be SMBus compatible and the ACK/NACK
> seems to be used by default in Documentation/i2c/i2c-protocol file.
>
>>
>> I stopp reviewing here because of -ENOTIME on my side but don't want to
>> delay discussion, so sent my comments up to here already now.
>>
>> Best regards
>> Uwe
>>
>> --
>> Pengutronix e.K.                           | Uwe Kleine-König            |
>> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
> Hi Uwe,
> 
> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > Hello Cedric,
> >
> > On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
> >> +/*
> >> + * In standard mode:
> >> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk period
> >> + *
> >> + * In fast mode:
> >> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
                                         ^^
> >> + *           SCL low period  = 2  * CCR * I2C parent clk period
                                      ^^
> >> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
                                         ^^
> >> + *           SCL low period  = 16 * CCR * I2C parent clk period

> > s/  \*/ */ several times
> 
> Sorry but I don't see where is the issue as the style for multi-line
> comments seems ok.
> Could you please clarify that point if possible ? Thanks in advance

There are several places with double spaces before * marked above.

> >> + * In order to reach 400 kHz with lower I2C parent clk frequencies we always set
> >> + * Duty = 1
> >> + *
> >> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
> >> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
> > s/mode/Mode/
> 
> ok thanks
> 
> >
> >> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
> >> + * constraints defined by i2c bus specification
> >
> > I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
> > of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
> > somewhere?
> 
> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
> 2Mhz and SCL_period = 1 we have:
> CCR = 1 * 2Mhz = 2.
> But to compute, scl_low and scl_high in Fast mode, we have to do the
> following thing as Duty=1:
> scl_high = 9 * CCR * I2C parent clk period
> scl_low = 16 * CCR * I2C parent clk period
> In our example:
> scl_high = 9 * 2 * 0,0000005 = 0,000009 sec = 9 µs
> scl_low = 16 * 2 * 0.0000005 = 0,000016 sec = 16 µs
> So low + high = 27 µs > 2,5 µs

For me 9 µs + 16 µs is 25 µs, resulting in 40 kHz. That's why I wondered
if there is a factor 10 missing somewhere.

> >> + */
> >> +static struct stm32f4_i2c_timings i2c_timings[] = {
> >> [...]
> >> +
> >> +/**
> >> + * stm32f4_i2c_hw_config() - Prepare I2C block
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static int stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> +     int ret = 0;
> >> +
> >> +     /* Disable I2C */
> >> +     stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
> >> +
> >> +     ret = stm32f4_i2c_set_periph_clk_freq(i2c_dev);
> >> +     if (ret)
> >> +             return ret;
> >> +
> >> +     stm32f4_i2c_set_rise_time(i2c_dev);
> >> +
> >> +     stm32f4_i2c_set_speed_mode(i2c_dev);
> >> +
> >> +     stm32f4_i2c_set_filter(i2c_dev);
> >> +
> >> +     /* Enable I2C */
> >> +     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
> >
> > This function is called after a hw reset, so there should be no need to
> > use clr_bits and set_bits because the value read from hw should be
> > known.
> 
> ok thanks
> 
> >
> >> +     return ret;
> >
> > return 0;
> 
> ok thanks
> 
> >
> >> +}
> >> +
> >> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> +     u32 status;
> >> +     int ret;
> >> +
> >> +     ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
> >> +                                      status,
> >> +                                      !(status & STM32F4_I2C_SR2_BUSY),
> >> +                                      10, 1000);
> >> +     if (ret) {
> >> +             dev_dbg(i2c_dev->dev, "bus not free\n");
> >> +             ret = -EBUSY;
> >> +     }
> >> +
> >> +     return ret;
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
> >> + * @i2c_dev: Controller's private data
> >> + * @byte: Data to write in the register
> >> + */
> >> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
> >> +{
> >> +     writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
> >> + * @i2c_dev: Controller's private data
> >> + *
> >> + * This function fills the data register with I2C transfer buffer
> >> + */
> >> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> +
> >> +     stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
> >> +     msg->count--;
> >> +}
> >> +
> >> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> +     u32 rbuf;
> >> +
> >> +     rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
> >> +     *msg->buf++ = rbuf & 0xff;
> >
> > This is unnecessary. buf has an 8 bit wide type so
> >
> >         *msg->buf++ = rbuf;
> >
> > has the same effect. (ISTR this is something I already pointed out
> > earlier?)
> 
> Yes you are right.
> 
> >
> >> +     msg->count--;
> >> +}
> >> +
> >> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> >> +
> >> +     stm32f4_i2c_disable_irq(i2c_dev);
> >> +
> >> +     reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> +     if (msg->stop)
> >> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> >> +     else
> >> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> >> +
> >> +     complete(&i2c_dev->complete);
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> >> +
> >> +     if (msg->count) {
> >> +             stm32f4_i2c_write_msg(i2c_dev);
> >> +             if (!msg->count) {
> >> +                     /* Disable buffer interrupts for RXNE/TXE events */
> >> +                     stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> >> +             }
> >> +     } else {
> >> +             stm32f4_i2c_terminate_xfer(i2c_dev);
> >
> > Is stm32f4_i2c_terminate_xfer also called when arbitration is lost? If
> > yes, is it then right to set STM32F4_I2C_CR1_STOP or
> > STM32F4_I2C_CR1_START?
> 
> If arbitration is lost, stm32f4_i2c_terminate_xfer() is not called.
> In that case, we return -EAGAIN and i2c-core will retry by calling
> stm32f4_i2c_xfer()
> 
> >
> >> +     }
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> >> +
> >> +     switch (msg->count) {
> >> +     case 1:
> >> +             stm32f4_i2c_disable_irq(i2c_dev);
> >> +             stm32f4_i2c_read_msg(i2c_dev);
> >> +             complete(&i2c_dev->complete);
> >> +             break;
> >> +     /*
> >> +      * For 2 or 3-byte reception, we do not have to read the data register
> >> +      * when RXNE occurs as we have to wait for byte transferred finished
> >
> > it's hard to understand because if you don't know the hardware the
> > meaning of RXNE is unknown.
> 
> Ok I will replace RXNE by RX not empty in that comment
> 
> >
> >> +      * event before reading data. So, here we just disable buffer
> >> +      * interrupt in order to avoid another system preemption due to RXNE
> >> +      * event
> >> +      */
> >> +     case 2:
> >> +     case 3:
> >> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> >> +             break;
> >> +     /* For N byte reception with N > 3 we directly read data register */
> >> +     default:
> >> +             stm32f4_i2c_read_msg(i2c_dev);
> >> +     }
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
> >> + * in case of read
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >
> > btf is a hw-related name. Maybe better use _done which is easier to
> > understand?
> 
> OK
> 
> >
> >> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> +     void __iomem *reg;
> >> +     u32 mask;
> >> +     int i;
> >> +
> >> +     switch (msg->count) {
> >> +     case 2:
> >> +             /*
> >> +              * In order to correctly send the Stop or Repeated Start
> >> +              * condition on the I2C bus, the STOP/START bit has to be set
> >> +              * before reading the last two bytes.
> >> +              * After that, we could read the last two bytes, disable
> >> +              * remaining interrupts and notify the end of xfer to the
> >> +              * client
> >
> > This is surprising. I didn't recheck the manual, but that looks very
> > uncomfortable.
> 
> I agree but this exactly the hardware way of working described in the
> reference manual.

IMHO that's a hw bug. This makes it for example impossible to implement
SMBus block transfers (I think).

> > How does this work, when I only want to read a single
> > byte? Same problem for ACK below.
> 
> For a single reception, we enable NACK and STOP or Repeatead START
> bits during address match.
> The NACK and STOP/START pulses are sent as soon as the data is
> received in the shift register.
> Please note that in that case, we don't have to wait BTF event to read the data.
> Data is read as soon as RXNE event occurs.
> 
> >
> >> +              */
> >> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> +             if (msg->stop)
> >> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> >> +             else
> >> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> >> +
> >> +             for (i = 2; i > 0; i--)
> >> +                     stm32f4_i2c_read_msg(i2c_dev);
> >> +
> >> +             reg = i2c_dev->base + STM32F4_I2C_CR2;
> >> +             mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
> >> +             stm32f4_i2c_clr_bits(reg, mask);
> >> +
> >> +             complete(&i2c_dev->complete);
> >> +             break;
> >> +     case 3:
> >> +             /*
> >> +              * In order to correctly send the ACK on the I2C bus for the
> >> +              * last two bytes, we have to set ACK bit before reading the
> >> +              * third last data byte
> >> +              */
> >> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> >> +             stm32f4_i2c_read_msg(i2c_dev);
> >> +             break;
> >> +     default:
> >> +             stm32f4_i2c_read_msg(i2c_dev);
> >> +     }
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
> >> + * master receiver
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> +     void __iomem *reg;
> >> +
> >> +     switch (msg->count) {
> >> +     case 0:
> >> +             stm32f4_i2c_terminate_xfer(i2c_dev);
> >> +             /* Clear ADDR flag */
> >> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> >> +             break;
> >> +     case 1:
> >> +             /*
> >> +              * Single byte reception:
> >
> > This also happens for the last byte of a 5 byte transfer, right?
> 
> For a 5 byte transfer the behavior is different:
> We have to read data from DR (data register)  as soon as the RXNE (RX
> not empty) event occurs for data1, data2 and data3 (until N-2 data for
> a more generic case)
> The ACK is automatically sent as soon as the data is received in the
> shift register as the I2C controller was configured to do that during
> adress match phase.
> 
> For data3 (N-2 data), we wait for BTF (Byte Transfer finished) event
> in order to set NACK before reading DR.
> This event occurs when a new data has been received in shift register
> (in our case data4 or N-1 data) but the prevoius data in DR (in our
> case data3 or N-2 data) has not been read yet.
> In that way, the NACK pulse will be correctly generated after the last
> received data byte.
> 
> For data4 and data5, we wait for BTF event (data4 or N-1 data in DR
> and data5 or N data in shift register), set STOP or repeated Start in
> order to correctly sent the right pulse after the last received data
> byte and run 2 consecutives read of DR.

So "Single byte reception" above is wrong, as this case is also used for
longer transfers and should be updated accordingly.

> >> +              * Enable NACK, clear ADDR flag and generate STOP or RepSTART
> >> +              */
> >> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> >> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> >> +             if (msg->stop)
> >> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> >> +             else
> >> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> >> +             break;
> >> +     case 2:
> >> +             /*
> >> +              * 2-byte reception:
> >> +              * Enable NACK and set POS
> >
> > What is POS?
> POS is used to define the position of the (N)ACK pulse
> 0: ACK is generated when the current is being received in the shift register
> 1: ACK is generated when the next byte which will be received in the
> shift register (used for 2-byte reception)

Can you please put this into the comment. "POS" isn't much helpful
there.

> 
> >
> >> +              */
> >> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> >> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
> >
> > You could get rid of this, when caching the value of CR1. Would save two
> > register reads here. This doesn't work for all registers, but it should
> > be possible to apply for most of them, maybe enough to get rid of the
> > clr_bits and set_bits function.
> >
> >> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> >> +             break;
> >> +
> >> +     default:
> >> +             /* N-byte reception: Enable ACK */
> >> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
> >
> > Do you need to set ACK for each byte transferred?
> I need to do that in order to be SMBus compatible and the ACK/NACK
> seems to be used by default in Documentation/i2c/i2c-protocol file.

Yeah, protocol wise you need to ack each byte. I just wondered if you
need to set the hardware bit for each byte or if it is retained in
hardware until unset by a register write.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

Hello Cedric,

On Wed, Jan 11, 2017 at 03:20:41PM +0100, M'boumba Cedric Madianga wrote:
> >
> >> +              */
> >> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> >> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
> >
> > You could get rid of this, when caching the value of CR1. Would save two
> > register reads here. This doesn't work for all registers, but it should
> > be possible to apply for most of them, maybe enough to get rid of the
> > clr_bits and set_bits function.
> 
> I agree at many places I could save registers read by not using
> clr_bits and set_bits function when the registers in question has been
> already read.
> But it is not enough to get rid of the clr_bits and set_bits function.
> For example when calling stm32f4_i2c_terminate_xfer(), the CR1
> register is never read before so set_bits function is useful.

I didn't double check the manual, but I would expect that CR1 isn't
modified by hardware. So you can cache the result in the driver data
structure and do the necessary modifications with that one.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
>> Hi Uwe,
>>
>> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> > Hello Cedric,
>> >
>> > On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
>> >> +/*
>> >> + * In standard mode:
>> >> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk period
>> >> + *
>> >> + * In fast mode:
>> >> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
>                                          ^^
>> >> + *           SCL low period  = 2  * CCR * I2C parent clk period
>                                       ^^
>> >> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
>                                          ^^
>> >> + *           SCL low period  = 16 * CCR * I2C parent clk period
>
>> > s/  \*/ */ several times
>>
>> Sorry but I don't see where is the issue as the style for multi-line
>> comments seems ok.
>> Could you please clarify that point if possible ? Thanks in advance
>
> There are several places with double spaces before * marked above.

Ok I see thanks.

>
>> >> + * In order to reach 400 kHz with lower I2C parent clk frequencies we always set
>> >> + * Duty = 1
>> >> + *
>> >> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
>> >> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
>> > s/mode/Mode/
>>
>> ok thanks
>>
>> >
>> >> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
>> >> + * constraints defined by i2c bus specification
>> >
>> > I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
>> > of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
>> > somewhere?
>>
>> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
>> 2Mhz and SCL_period = 1 we have:
>> CCR = 1 * 2Mhz = 2.
>> But to compute, scl_low and scl_high in Fast mode, we have to do the
>> following thing as Duty=1:
>> scl_high = 9 * CCR * I2C parent clk period
>> scl_low = 16 * CCR * I2C parent clk period
>> In our example:
>> scl_high = 9 * 2 * 0,0000005 = 0,000009 sec = 9 µs
>> scl_low = 16 * 2 * 0.0000005 = 0,000016 sec = 16 µs
>> So low + high = 27 µs > 2,5 µs
>
> For me 9 µs + 16 µs is 25 µs, resulting in 40 kHz. That's why I wondered
> if there is a factor 10 missing somewhere.

Hum ok. I am going to double-check what is wrong because when I check
with the scope I always reach 400Khz for SCL.
I will let you know.
>
>> >> + */
>> >> +static struct stm32f4_i2c_timings i2c_timings[] = {
>> >> [...]
>> >> +
>> >> +/**
>> >> + * stm32f4_i2c_hw_config() - Prepare I2C block
>> >> + * @i2c_dev: Controller's private data
>> >> + */
>> >> +static int stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> +     int ret = 0;
>> >> +
>> >> +     /* Disable I2C */
>> >> +     stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>> >> +
>> >> +     ret = stm32f4_i2c_set_periph_clk_freq(i2c_dev);
>> >> +     if (ret)
>> >> +             return ret;
>> >> +
>> >> +     stm32f4_i2c_set_rise_time(i2c_dev);
>> >> +
>> >> +     stm32f4_i2c_set_speed_mode(i2c_dev);
>> >> +
>> >> +     stm32f4_i2c_set_filter(i2c_dev);
>> >> +
>> >> +     /* Enable I2C */
>> >> +     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
>> >
>> > This function is called after a hw reset, so there should be no need to
>> > use clr_bits and set_bits because the value read from hw should be
>> > known.
>>
>> ok thanks
>>
>> >
>> >> +     return ret;
>> >
>> > return 0;
>>
>> ok thanks
>>
>> >
>> >> +}
>> >> +
>> >> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> +     u32 status;
>> >> +     int ret;
>> >> +
>> >> +     ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>> >> +                                      status,
>> >> +                                      !(status & STM32F4_I2C_SR2_BUSY),
>> >> +                                      10, 1000);
>> >> +     if (ret) {
>> >> +             dev_dbg(i2c_dev->dev, "bus not free\n");
>> >> +             ret = -EBUSY;
>> >> +     }
>> >> +
>> >> +     return ret;
>> >> +}
>> >> +
>> >> +/**
>> >> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
>> >> + * @i2c_dev: Controller's private data
>> >> + * @byte: Data to write in the register
>> >> + */
>> >> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
>> >> +{
>> >> +     writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
>> >> +}
>> >> +
>> >> +/**
>> >> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
>> >> + * @i2c_dev: Controller's private data
>> >> + *
>> >> + * This function fills the data register with I2C transfer buffer
>> >> + */
>> >> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> >> +
>> >> +     stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
>> >> +     msg->count--;
>> >> +}
>> >> +
>> >> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> >> +     u32 rbuf;
>> >> +
>> >> +     rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
>> >> +     *msg->buf++ = rbuf & 0xff;
>> >
>> > This is unnecessary. buf has an 8 bit wide type so
>> >
>> >         *msg->buf++ = rbuf;
>> >
>> > has the same effect. (ISTR this is something I already pointed out
>> > earlier?)
>>
>> Yes you are right.
>>
>> >
>> >> +     msg->count--;
>> >> +}
>> >> +
>> >> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> >> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> >> +
>> >> +     stm32f4_i2c_disable_irq(i2c_dev);
>> >> +
>> >> +     reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> +     if (msg->stop)
>> >> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> >> +     else
>> >> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> >> +
>> >> +     complete(&i2c_dev->complete);
>> >> +}
>> >> +
>> >> +/**
>> >> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
>> >> + * @i2c_dev: Controller's private data
>> >> + */
>> >> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> >> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> >> +
>> >> +     if (msg->count) {
>> >> +             stm32f4_i2c_write_msg(i2c_dev);
>> >> +             if (!msg->count) {
>> >> +                     /* Disable buffer interrupts for RXNE/TXE events */
>> >> +                     stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> >> +             }
>> >> +     } else {
>> >> +             stm32f4_i2c_terminate_xfer(i2c_dev);
>> >
>> > Is stm32f4_i2c_terminate_xfer also called when arbitration is lost? If
>> > yes, is it then right to set STM32F4_I2C_CR1_STOP or
>> > STM32F4_I2C_CR1_START?
>>
>> If arbitration is lost, stm32f4_i2c_terminate_xfer() is not called.
>> In that case, we return -EAGAIN and i2c-core will retry by calling
>> stm32f4_i2c_xfer()
>>
>> >
>> >> +     }
>> >> +}
>> >> +
>> >> +/**
>> >> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
>> >> + * @i2c_dev: Controller's private data
>> >> + */
>> >> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> >> +     void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> >> +
>> >> +     switch (msg->count) {
>> >> +     case 1:
>> >> +             stm32f4_i2c_disable_irq(i2c_dev);
>> >> +             stm32f4_i2c_read_msg(i2c_dev);
>> >> +             complete(&i2c_dev->complete);
>> >> +             break;
>> >> +     /*
>> >> +      * For 2 or 3-byte reception, we do not have to read the data register
>> >> +      * when RXNE occurs as we have to wait for byte transferred finished
>> >
>> > it's hard to understand because if you don't know the hardware the
>> > meaning of RXNE is unknown.
>>
>> Ok I will replace RXNE by RX not empty in that comment
>>
>> >
>> >> +      * event before reading data. So, here we just disable buffer
>> >> +      * interrupt in order to avoid another system preemption due to RXNE
>> >> +      * event
>> >> +      */
>> >> +     case 2:
>> >> +     case 3:
>> >> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> >> +             break;
>> >> +     /* For N byte reception with N > 3 we directly read data register */
>> >> +     default:
>> >> +             stm32f4_i2c_read_msg(i2c_dev);
>> >> +     }
>> >> +}
>> >> +
>> >> +/**
>> >> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
>> >> + * in case of read
>> >> + * @i2c_dev: Controller's private data
>> >> + */
>> >> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >
>> > btf is a hw-related name. Maybe better use _done which is easier to
>> > understand?
>>
>> OK
>>
>> >
>> >> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> >> +     void __iomem *reg;
>> >> +     u32 mask;
>> >> +     int i;
>> >> +
>> >> +     switch (msg->count) {
>> >> +     case 2:
>> >> +             /*
>> >> +              * In order to correctly send the Stop or Repeated Start
>> >> +              * condition on the I2C bus, the STOP/START bit has to be set
>> >> +              * before reading the last two bytes.
>> >> +              * After that, we could read the last two bytes, disable
>> >> +              * remaining interrupts and notify the end of xfer to the
>> >> +              * client
>> >
>> > This is surprising. I didn't recheck the manual, but that looks very
>> > uncomfortable.
>>
>> I agree but this exactly the hardware way of working described in the
>> reference manual.
>
> IMHO that's a hw bug. This makes it for example impossible to implement
> SMBus block transfers (I think).

This is not correct.
Setting STOP/START bit does not mean the the pulse will be sent right now.
Here we have just to prepare the hardware for the 2 next pulse but the
STOP/START/ACK pulse will be generated at the right time as required
by I2C specification.
So SMBus block transfer will be possible.

>
>> > How does this work, when I only want to read a single
>> > byte? Same problem for ACK below.
>>
>> For a single reception, we enable NACK and STOP or Repeatead START
>> bits during address match.
>> The NACK and STOP/START pulses are sent as soon as the data is
>> received in the shift register.
>> Please note that in that case, we don't have to wait BTF event to read the data.
>> Data is read as soon as RXNE event occurs.
>>
>> >
>> >> +              */
>> >> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> +             if (msg->stop)
>> >> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> >> +             else
>> >> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> >> +
>> >> +             for (i = 2; i > 0; i--)
>> >> +                     stm32f4_i2c_read_msg(i2c_dev);
>> >> +
>> >> +             reg = i2c_dev->base + STM32F4_I2C_CR2;
>> >> +             mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
>> >> +             stm32f4_i2c_clr_bits(reg, mask);
>> >> +
>> >> +             complete(&i2c_dev->complete);
>> >> +             break;
>> >> +     case 3:
>> >> +             /*
>> >> +              * In order to correctly send the ACK on the I2C bus for the
>> >> +              * last two bytes, we have to set ACK bit before reading the
>> >> +              * third last data byte
>> >> +              */
>> >> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> >> +             stm32f4_i2c_read_msg(i2c_dev);
>> >> +             break;
>> >> +     default:
>> >> +             stm32f4_i2c_read_msg(i2c_dev);
>> >> +     }
>> >> +}
>> >> +
>> >> +/**
>> >> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
>> >> + * master receiver
>> >> + * @i2c_dev: Controller's private data
>> >> + */
>> >> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
>> >> +{
>> >> +     struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> >> +     void __iomem *reg;
>> >> +
>> >> +     switch (msg->count) {
>> >> +     case 0:
>> >> +             stm32f4_i2c_terminate_xfer(i2c_dev);
>> >> +             /* Clear ADDR flag */
>> >> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> >> +             break;
>> >> +     case 1:
>> >> +             /*
>> >> +              * Single byte reception:
>> >
>> > This also happens for the last byte of a 5 byte transfer, right?
>>
>> For a 5 byte transfer the behavior is different:
>> We have to read data from DR (data register)  as soon as the RXNE (RX
>> not empty) event occurs for data1, data2 and data3 (until N-2 data for
>> a more generic case)
>> The ACK is automatically sent as soon as the data is received in the
>> shift register as the I2C controller was configured to do that during
>> adress match phase.
>>
>> For data3 (N-2 data), we wait for BTF (Byte Transfer finished) event
>> in order to set NACK before reading DR.
>> This event occurs when a new data has been received in shift register
>> (in our case data4 or N-1 data) but the prevoius data in DR (in our
>> case data3 or N-2 data) has not been read yet.
>> In that way, the NACK pulse will be correctly generated after the last
>> received data byte.
>>
>> For data4 and data5, we wait for BTF event (data4 or N-1 data in DR
>> and data5 or N data in shift register), set STOP or repeated Start in
>> order to correctly sent the right pulse after the last received data
>> byte and run 2 consecutives read of DR.
>
> So "Single byte reception" above is wrong, as this case is also used for
> longer transfers and should be updated accordingly.

I don't think so.
stm32f4_i2c_handle_rx_addr() is called once during adress match phase.
It is used to configure the I2C controller according to the number of
data to be received as it has to be done in a different way according
to the number of data to received:
- single byte reception
- 2-byte reception
- N-byte reception
Then, as soon as, the controller is correctly configured, for each
byte to be received, we use stm32f4_i2c_handle_read() or
stm32f4_i2c_handle_rx_done().
stm32f4_i2c_handle_read() is used to read  data for a single byte
reception or until N-2 data for N-byte reception
stm32f4_i2c_handle_rx_done() is used to read data for a 2-byte
reception, or  data N-2, N-1 and N for a N-byte reception.
So, single-reception and longer transfer have been clearly managed in
a different way.

>
>> >> +              * Enable NACK, clear ADDR flag and generate STOP or RepSTART
>> >> +              */
>> >> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> >> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> >> +             if (msg->stop)
>> >> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> >> +             else
>> >> +                     stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> >> +             break;
>> >> +     case 2:
>> >> +             /*
>> >> +              * 2-byte reception:
>> >> +              * Enable NACK and set POS
>> >
>> > What is POS?
>> POS is used to define the position of the (N)ACK pulse
>> 0: ACK is generated when the current is being received in the shift register
>> 1: ACK is generated when the next byte which will be received in the
>> shift register (used for 2-byte reception)
>
> Can you please put this into the comment. "POS" isn't much helpful
> there.

Ok I will add a comment for that.

>
>>
>> >
>> >> +              */
>> >> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> >> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>> >
>> > You could get rid of this, when caching the value of CR1. Would save two
>> > register reads here. This doesn't work for all registers, but it should
>> > be possible to apply for most of them, maybe enough to get rid of the
>> > clr_bits and set_bits function.
>> >
>> >> +             readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> >> +             break;
>> >> +
>> >> +     default:
>> >> +             /* N-byte reception: Enable ACK */
>> >> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
>> >
>> > Do you need to set ACK for each byte transferred?
>> I need to do that in order to be SMBus compatible and the ACK/NACK
>> seems to be used by default in Documentation/i2c/i2c-protocol file.
>
> Yeah, protocol wise you need to ack each byte. I just wondered if you
> need to set the hardware bit for each byte or if it is retained in
> hardware until unset by a register write.

ACK bit is set in  stm32f4_i2c_handle_rx_addr().
As explained above, this function is called once during address match phase.
So, this bit is set only once just before receiving the first data byte.

>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

Hi Uwe,

2017-01-11 16:42 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello Cedric,
>
> On Wed, Jan 11, 2017 at 03:20:41PM +0100, M'boumba Cedric Madianga wrote:
>> >
>> >> +              */
>> >> +             reg = i2c_dev->base + STM32F4_I2C_CR1;
>> >> +             stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> >> +             stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>> >
>> > You could get rid of this, when caching the value of CR1. Would save two
>> > register reads here. This doesn't work for all registers, but it should
>> > be possible to apply for most of them, maybe enough to get rid of the
>> > clr_bits and set_bits function.
>>
>> I agree at many places I could save registers read by not using
>> clr_bits and set_bits function when the registers in question has been
>> already read.
>> But it is not enough to get rid of the clr_bits and set_bits function.
>> For example when calling stm32f4_i2c_terminate_xfer(), the CR1
>> register is never read before so set_bits function is useful.
>
> I didn't double check the manual, but I would expect that CR1 isn't
> modified by hardware. So you can cache the result in the driver data
> structure and do the necessary modifications with that one.

CR1 is modified by hardware to clear some bits set by software during
the communication.

>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

Hello Cedric,

On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> > This is surprising. I didn't recheck the manual, but that looks very
> >> > uncomfortable.
> >>
> >> I agree but this exactly the hardware way of working described in the
> >> reference manual.
> >
> > IMHO that's a hw bug. This makes it for example impossible to implement
> > SMBus block transfers (I think).
> 
> This is not correct.
> Setting STOP/START bit does not mean the the pulse will be sent right now.
> Here we have just to prepare the hardware for the 2 next pulse but the
> STOP/START/ACK pulse will be generated at the right time as required
> by I2C specification.
> So SMBus block transfer will be possible.

A block transfer consists of a byte that specifies the count of bytes
yet to come. So the device sends for example:

	0x01 0xab

So when you read the 1 in the first byte it's already too late to set
STOP to get it after the 2nd byte.

Not sure I got all the required details right, though.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

2017-01-12 13:03 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello Cedric,
>
> On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
>> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
>> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> >> > This is surprising. I didn't recheck the manual, but that looks very
>> >> > uncomfortable.
>> >>
>> >> I agree but this exactly the hardware way of working described in the
>> >> reference manual.
>> >
>> > IMHO that's a hw bug. This makes it for example impossible to implement
>> > SMBus block transfers (I think).
>>
>> This is not correct.
>> Setting STOP/START bit does not mean the the pulse will be sent right now.
>> Here we have just to prepare the hardware for the 2 next pulse but the
>> STOP/START/ACK pulse will be generated at the right time as required
>> by I2C specification.
>> So SMBus block transfer will be possible.
>
> A block transfer consists of a byte that specifies the count of bytes
> yet to come. So the device sends for example:
>
>         0x01 0xab
>
> So when you read the 1 in the first byte it's already too late to set
> STOP to get it after the 2nd byte.
>
> Not sure I got all the required details right, though.

Ok I understand your use case but I always think that the harware manages it.
If I take the above example, the I2C SMBus block read transaction will
be as below:
S Addr Wr [A] Comm [A]
           S Addr Rd [A] [Count] A [Data1] A [Data2] NA P

The first message is a single byte-transmission so there is no problem.

The second message is a N-byte reception with N = 3

When the I2C controller has finished to send the device address (S
Addr Rd), the ADDR flag is set and an interrupt is raised.
In the routine that handles ADDR event, we set ACK bit in order to
generate ACK pulse as soon as a data byte is received in the shift
register and then we clear the ADDR flag.
Please note that the SCL line is stretched low until ADDR flag is cleared.
So, as far I understand, the device could not sent any data as long as
the SCL line is stretched low. Right ?

Then, as soon as the SCL line is high, the device could send the first
data byte (Count).
When this byte is received in the shift register, an ACK is
automatically generated as defined during adress match phase and the
data byte is pushed in DR (data register).
Then, an interrupt is raised as RXNE (RX not empty) flag is set.
In the routine that handles RXNE event, as N=3, we just clear all
buffer interrupts in order to avoid another system preemption due to
RXNE event but we does not read the data in DR.

After receiving the ACK, the device could send the second data byte (Data1).
When this byte is received in the shift register, an ACK is
automatically generated.
As the first data byte has not been read yet in DR, the BTF (Byte
Transfer Finihed) flag is set and an interrupt is raised.
So, in that case, the SCL line is also streched low as long as the
data register has not been read.
In the routine that handle BTF event, we enable NACK in order to
generate this pulse as soon as the last data byte will be received and
then we read DR register ([Count])
At this moment, SCL line is released and the device could send the
last data byte.

After receiving the ACK, the device could send the third and last data
byte (Data2)
When this byte is received in the shift register, a NACK is
automatically generated as we enable it as explained above.
As the second data byte  (Data1) has not been read yet in DR, the BTF
flag is set again and an interrupt is raised.
The SCL line is stretched low and in that way we could set the STOP
bit to generate this pulse.
Then we run 2 consecutives read of DR to retrieve [Data1] and [Data2]
and to set SCL high.

So, thanks to SCL stretching, it seems that NA and P will be generated
at the right time.

Please let me know if it is not correct.

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

>>> > I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
>>> > of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
>>> > somewhere?
>>>
>>> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
>>> 2Mhz and SCL_period = 1 we have:
>>> CCR = 1 * 2Mhz = 2.
>>> But to compute, scl_low and scl_high in Fast mode, we have to do the
>>> following thing as Duty=1:
>>> scl_high = 9 * CCR * I2C parent clk period
>>> scl_low = 16 * CCR * I2C parent clk period
>>> In our example:
>>> scl_high = 9 * 2 * 0,0000005 = 0,000009 sec = 9 µs
>>> scl_low = 16 * 2 * 0.0000005 = 0,000016 sec = 16 µs
>>> So low + high = 27 µs > 2,5 µs
>>
>> For me 9 µs + 16 µs is 25 µs, resulting in 40 kHz. That's why I wondered
>> if there is a factor 10 missing somewhere.
>
> Hum ok. I am going to double-check what is wrong because when I check
> with the scope I always reach 400Khz for SCL.
> I will let you know.

There is one point I miss here that is described in the reference manual:
To reach the 400 kHz maximum I²C fast mode clock, the I2C parent rate
must be a multiple of 10 MHz.
So, contrary to what we said in a previous thread, 400 kHz could not
be reached with low frequencies.
In that way, we could compute CCR with duty = 0 by default.
So, I find another formula very close to the first one I pushed in the
first version:

In fast mode, we compute CCR with duty = 0:
t_scl_high = CCR * I2C parent clk period
t_scl_low = 2 *CCR * I2C parent clk period
So, CCR = I2C parent rate / 400 kHz / 3

For example with parent rate = 40 MHz:
CCR = 40000000 / 400000 / 3 = 33.333333333 = 33
t_scl_high = 33 * (1 / 2000000) = 825 ns > 600 ns
t_scl_low = 2 * 16 * (1 / 2000000) = 1650 ns > 1300 ns

It seems ok now.

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

On Thu, Jan 12, 2017 at 02:47:42PM +0100, M'boumba Cedric Madianga wrote:
> 2017-01-12 13:03 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > Hello Cedric,
> >
> > On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
> >> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
> >> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> >> > This is surprising. I didn't recheck the manual, but that looks very
> >> >> > uncomfortable.
> >> >>
> >> >> I agree but this exactly the hardware way of working described in the
> >> >> reference manual.
> >> >
> >> > IMHO that's a hw bug. This makes it for example impossible to implement
> >> > SMBus block transfers (I think).
> >>
> >> This is not correct.
> >> Setting STOP/START bit does not mean the the pulse will be sent right now.
> >> Here we have just to prepare the hardware for the 2 next pulse but the
> >> STOP/START/ACK pulse will be generated at the right time as required
> >> by I2C specification.
> >> So SMBus block transfer will be possible.
> >
> > A block transfer consists of a byte that specifies the count of bytes
> > yet to come. So the device sends for example:
> >
> >         0x01 0xab
> >
> > So when you read the 1 in the first byte it's already too late to set
> > STOP to get it after the 2nd byte.
> >
> > Not sure I got all the required details right, though.
> 
> Ok I understand your use case but I always think that the harware manages it.
> If I take the above example, the I2C SMBus block read transaction will
> be as below:
> S Addr Wr [A] Comm [A]
>            S Addr Rd [A] [Count] A [Data1] A [Data2] NA P
> 
> The first message is a single byte-transmission so there is no problem.
> 
> The second message is a N-byte reception with N = 3
> 
> When the I2C controller has finished to send the device address (S
> Addr Rd), the ADDR flag is set and an interrupt is raised.
> In the routine that handles ADDR event, we set ACK bit in order to
> generate ACK pulse as soon as a data byte is received in the shift
> register and then we clear the ADDR flag.
> Please note that the SCL line is stretched low until ADDR flag is cleared.
> So, as far I understand, the device could not sent any data as long as
> the SCL line is stretched low. Right ?
> 
> Then, as soon as the SCL line is high, the device could send the first
> data byte (Count).
> When this byte is received in the shift register, an ACK is
> automatically generated as defined during adress match phase and the
> data byte is pushed in DR (data register).
> Then, an interrupt is raised as RXNE (RX not empty) flag is set.
> In the routine that handles RXNE event, as N=3, we just clear all
> buffer interrupts in order to avoid another system preemption due to
> RXNE event but we does not read the data in DR.

In my example I want to receive a block of length 1, so only two bytes
are read, a 1 (the length) and the data byte (0xab in my example). I
think that as soon as you read the 1 it's already to late to schedule
the NA after the next byte?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

2017-01-12 18:49 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Thu, Jan 12, 2017 at 02:47:42PM +0100, M'boumba Cedric Madianga wrote:
>> 2017-01-12 13:03 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> > Hello Cedric,
>> >
>> > On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
>> >> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> >> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
>> >> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> >> >> > This is surprising. I didn't recheck the manual, but that looks very
>> >> >> > uncomfortable.
>> >> >>
>> >> >> I agree but this exactly the hardware way of working described in the
>> >> >> reference manual.
>> >> >
>> >> > IMHO that's a hw bug. This makes it for example impossible to implement
>> >> > SMBus block transfers (I think).
>> >>
>> >> This is not correct.
>> >> Setting STOP/START bit does not mean the the pulse will be sent right now.
>> >> Here we have just to prepare the hardware for the 2 next pulse but the
>> >> STOP/START/ACK pulse will be generated at the right time as required
>> >> by I2C specification.
>> >> So SMBus block transfer will be possible.
>> >
>> > A block transfer consists of a byte that specifies the count of bytes
>> > yet to come. So the device sends for example:
>> >
>> >         0x01 0xab
>> >
>> > So when you read the 1 in the first byte it's already too late to set
>> > STOP to get it after the 2nd byte.
>> >
>> > Not sure I got all the required details right, though.
>>
>> Ok I understand your use case but I always think that the harware manages it.
>> If I take the above example, the I2C SMBus block read transaction will
>> be as below:
>> S Addr Wr [A] Comm [A]
>>            S Addr Rd [A] [Count] A [Data1] A [Data2] NA P
>>
>> The first message is a single byte-transmission so there is no problem.
>>
>> The second message is a N-byte reception with N = 3
>>
>> When the I2C controller has finished to send the device address (S
>> Addr Rd), the ADDR flag is set and an interrupt is raised.
>> In the routine that handles ADDR event, we set ACK bit in order to
>> generate ACK pulse as soon as a data byte is received in the shift
>> register and then we clear the ADDR flag.
>> Please note that the SCL line is stretched low until ADDR flag is cleared.
>> So, as far I understand, the device could not sent any data as long as
>> the SCL line is stretched low. Right ?
>>
>> Then, as soon as the SCL line is high, the device could send the first
>> data byte (Count).
>> When this byte is received in the shift register, an ACK is
>> automatically generated as defined during adress match phase and the
>> data byte is pushed in DR (data register).
>> Then, an interrupt is raised as RXNE (RX not empty) flag is set.
>> In the routine that handles RXNE event, as N=3, we just clear all
>> buffer interrupts in order to avoid another system preemption due to
>> RXNE event but we does not read the data in DR.
>
> In my example I want to receive a block of length 1, so only two bytes
> are read, a 1 (the length) and the data byte (0xab in my example). I
> think that as soon as you read the 1 it's already to late to schedule
> the NA after the next byte?

Not really. This 2-byte reception is also correctly managed.
Indeed, in this case, when the controller has sent the device address,
the ADDR flag is set and an interrupt is raised.
So, as long as the ADDR flag is not cleared, the SCL line is stretched
low and the device could not send any data.
During this address match phase, for a 2-byte reception, we enable
NACK and set POS bit (ACK/NACK position).
As POS=1, the NACK will be sent for the next byte which will be
received in the shift register instead of the current one.
So in this example, the next byte will be the last one.
After that, we clear the ADDR flag and the device is allowed to send data.

When the first data is received in the shift register,  the RXNE flag
is set and an interrupt is raised.
As it is a 2-byte reception, we just clear all interrupts buffer to
avoid another preemption due to RXNE but we does not read DR.

Then, the second and last byte is received in the shift register.
The NACK is automatically sent by I2C controller as it was configured
to do that in the address match phase described above.
Moereover, as the first byte has not been read in DR, the BTF event
flag is set and an interrupt is raised.
Again, the SCL line is stretching low as long as data register has not
been read.
In the meantime, we set STOP bit to generate the pulse and we launch 2
consecutive read of DR to retrieve the 2 data bytes and release SCL
stretching.

In that way, NA and STOP are generated as expected even for a 2-byte reception.

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

On Thu, Jan 12, 2017 at 09:58:23PM +0100, M'boumba Cedric Madianga wrote:
> 2017-01-12 18:49 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > On Thu, Jan 12, 2017 at 02:47:42PM +0100, M'boumba Cedric Madianga wrote:
> >> 2017-01-12 13:03 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> > Hello Cedric,
> >> >
> >> > On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
> >> >> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> >> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
> >> >> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> >> >> > This is surprising. I didn't recheck the manual, but that looks very
> >> >> >> > uncomfortable.
> >> >> >>
> >> >> >> I agree but this exactly the hardware way of working described in the
> >> >> >> reference manual.
> >> >> >
> >> >> > IMHO that's a hw bug. This makes it for example impossible to implement
> >> >> > SMBus block transfers (I think).
> >> >>
> >> >> This is not correct.
> >> >> Setting STOP/START bit does not mean the the pulse will be sent right now.
> >> >> Here we have just to prepare the hardware for the 2 next pulse but the
> >> >> STOP/START/ACK pulse will be generated at the right time as required
> >> >> by I2C specification.
> >> >> So SMBus block transfer will be possible.
> >> >
> >> > A block transfer consists of a byte that specifies the count of bytes
> >> > yet to come. So the device sends for example:
> >> >
> >> >         0x01 0xab
> >> >
> >> > So when you read the 1 in the first byte it's already too late to set
> >> > STOP to get it after the 2nd byte.
> >> >
> >> > Not sure I got all the required details right, though.
> >>
> >> Ok I understand your use case but I always think that the harware manages it.
> >> If I take the above example, the I2C SMBus block read transaction will
> >> be as below:
> >> S Addr Wr [A] Comm [A]
> >>            S Addr Rd [A] [Count] A [Data1] A [Data2] NA P
> >>
> >> The first message is a single byte-transmission so there is no problem.
> >>
> >> The second message is a N-byte reception with N = 3
> >>
> >> When the I2C controller has finished to send the device address (S
> >> Addr Rd), the ADDR flag is set and an interrupt is raised.
> >> In the routine that handles ADDR event, we set ACK bit in order to
> >> generate ACK pulse as soon as a data byte is received in the shift
> >> register and then we clear the ADDR flag.
> >> Please note that the SCL line is stretched low until ADDR flag is cleared.
> >> So, as far I understand, the device could not sent any data as long as
> >> the SCL line is stretched low. Right ?
> >>
> >> Then, as soon as the SCL line is high, the device could send the first
> >> data byte (Count).
> >> When this byte is received in the shift register, an ACK is
> >> automatically generated as defined during adress match phase and the
> >> data byte is pushed in DR (data register).
> >> Then, an interrupt is raised as RXNE (RX not empty) flag is set.
> >> In the routine that handles RXNE event, as N=3, we just clear all
> >> buffer interrupts in order to avoid another system preemption due to
> >> RXNE event but we does not read the data in DR.
> >
> > In my example I want to receive a block of length 1, so only two bytes
> > are read, a 1 (the length) and the data byte (0xab in my example). I
> > think that as soon as you read the 1 it's already to late to schedule
> > the NA after the next byte?
> 
> Not really. This 2-byte reception is also correctly managed.
> Indeed, in this case, when the controller has sent the device address,
> the ADDR flag is set and an interrupt is raised.
> So, as long as the ADDR flag is not cleared, the SCL line is stretched
> low and the device could not send any data.
> During this address match phase, for a 2-byte reception, we enable
> NACK and set POS bit (ACK/NACK position).
> As POS=1, the NACK will be sent for the next byte which will be
> received in the shift register instead of the current one.
> So in this example, the next byte will be the last one.
> After that, we clear the ADDR flag and the device is allowed to send data.

I didn't follow, but if you are convinced it works that's good. I wonder
if it simplifies the driver if POS=1 is used and so ACK/NACK can be
setup later?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

2017-01-12 22:10 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Thu, Jan 12, 2017 at 09:58:23PM +0100, M'boumba Cedric Madianga wrote:
>> 2017-01-12 18:49 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> > On Thu, Jan 12, 2017 at 02:47:42PM +0100, M'boumba Cedric Madianga wrote:
>> >> 2017-01-12 13:03 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> >> > Hello Cedric,
>> >> >
>> >> > On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
>> >> >> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> >> >> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
>> >> >> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> >> >> >> > This is surprising. I didn't recheck the manual, but that looks very
>> >> >> >> > uncomfortable.
>> >> >> >>
>> >> >> >> I agree but this exactly the hardware way of working described in the
>> >> >> >> reference manual.
>> >> >> >
>> >> >> > IMHO that's a hw bug. This makes it for example impossible to implement
>> >> >> > SMBus block transfers (I think).
>> >> >>
>> >> >> This is not correct.
>> >> >> Setting STOP/START bit does not mean the the pulse will be sent right now.
>> >> >> Here we have just to prepare the hardware for the 2 next pulse but the
>> >> >> STOP/START/ACK pulse will be generated at the right time as required
>> >> >> by I2C specification.
>> >> >> So SMBus block transfer will be possible.
>> >> >
>> >> > A block transfer consists of a byte that specifies the count of bytes
>> >> > yet to come. So the device sends for example:
>> >> >
>> >> >         0x01 0xab
>> >> >
>> >> > So when you read the 1 in the first byte it's already too late to set
>> >> > STOP to get it after the 2nd byte.
>> >> >
>> >> > Not sure I got all the required details right, though.
>> >>
>> >> Ok I understand your use case but I always think that the harware manages it.
>> >> If I take the above example, the I2C SMBus block read transaction will
>> >> be as below:
>> >> S Addr Wr [A] Comm [A]
>> >>            S Addr Rd [A] [Count] A [Data1] A [Data2] NA P
>> >>
>> >> The first message is a single byte-transmission so there is no problem.
>> >>
>> >> The second message is a N-byte reception with N = 3
>> >>
>> >> When the I2C controller has finished to send the device address (S
>> >> Addr Rd), the ADDR flag is set and an interrupt is raised.
>> >> In the routine that handles ADDR event, we set ACK bit in order to
>> >> generate ACK pulse as soon as a data byte is received in the shift
>> >> register and then we clear the ADDR flag.
>> >> Please note that the SCL line is stretched low until ADDR flag is cleared.
>> >> So, as far I understand, the device could not sent any data as long as
>> >> the SCL line is stretched low. Right ?
>> >>
>> >> Then, as soon as the SCL line is high, the device could send the first
>> >> data byte (Count).
>> >> When this byte is received in the shift register, an ACK is
>> >> automatically generated as defined during adress match phase and the
>> >> data byte is pushed in DR (data register).
>> >> Then, an interrupt is raised as RXNE (RX not empty) flag is set.
>> >> In the routine that handles RXNE event, as N=3, we just clear all
>> >> buffer interrupts in order to avoid another system preemption due to
>> >> RXNE event but we does not read the data in DR.
>> >
>> > In my example I want to receive a block of length 1, so only two bytes
>> > are read, a 1 (the length) and the data byte (0xab in my example). I
>> > think that as soon as you read the 1 it's already to late to schedule
>> > the NA after the next byte?
>>
>> Not really. This 2-byte reception is also correctly managed.
>> Indeed, in this case, when the controller has sent the device address,
>> the ADDR flag is set and an interrupt is raised.
>> So, as long as the ADDR flag is not cleared, the SCL line is stretched
>> low and the device could not send any data.
>> During this address match phase, for a 2-byte reception, we enable
>> NACK and set POS bit (ACK/NACK position).
>> As POS=1, the NACK will be sent for the next byte which will be
>> received in the shift register instead of the current one.
>> So in this example, the next byte will be the last one.
>> After that, we clear the ADDR flag and the device is allowed to send data.
>
> I didn't follow, but if you are convinced it works that's good. I wonder
> if it simplifies the driver if POS=1 is used and so ACK/NACK can be
> setup later?

Please see below a quote from datasheet that clearly described how to handle
For 2-byte reception:
● Wait until ADDR = 1 (SCL stretched low until the ADDR flag is cleared)
● Set ACK low, set POS high
● Clear ADDR flag
● Wait until BTF = 1 (Data 1 in DR, Data2 in shift register, SCL
stretched low until a data1 is read)
● Set STOP high
● Read data 1 and 2

So we cannot set POS=1 and setup ACK/NACK later as you suggest.

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

Hello,

On Thu, Jan 12, 2017 at 10:28:20PM +0100, M'boumba Cedric Madianga wrote:
> Please see below a quote from datasheet that clearly described how to handle
> For 2-byte reception:
> ● Wait until ADDR = 1 (SCL stretched low until the ADDR flag is cleared)
> ● Set ACK low, set POS high
> ● Clear ADDR flag
> ● Wait until BTF = 1 (Data 1 in DR, Data2 in shift register, SCL
> stretched low until a data1 is read)
> ● Set STOP high
> ● Read data 1 and 2

The problem is that you only know that you have a 2 byte transfer after
you read the first byte (and it's a 1). (But note that this is
irrelevant for the patch as the driver doesn't claim to support this
kind of transfer.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 194 bytes --]


> (But note that this is irrelevant for the patch as the driver doesn't
> claim to support this kind of transfer.)

Yes, I wanted to mention that, too.

I'd think the series is good to go in?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

On Fri, Jan 13, 2017 at 09:29:03AM +0100, Wolfram Sang wrote:
> 
> > (But note that this is irrelevant for the patch as the driver doesn't
> > claim to support this kind of transfer.)
> 
> Yes, I wanted to mention that, too.
> 
> I'd think the series is good to go in?

AFAICT there are some unaddressed comments that Cedrics claimed to fix
before our discussion was dominated by block transfers.

Best regards
Uwe



-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

Ok so I am going to send the v9 asap.
Thanks

2017-01-13 9:45 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Fri, Jan 13, 2017 at 09:29:03AM +0100, Wolfram Sang wrote:
>>
>> > (But note that this is irrelevant for the patch as the driver doesn't
>> > claim to support this kind of transfer.)
>>
>> Yes, I wanted to mention that, too.
>>
>> I'd think the series is good to go in?
>
> AFAICT there are some unaddressed comments that Cedrics claimed to fix
> before our discussion was dominated by block transfers.
>
> Best regards
> Uwe
>
>
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |