[PATCH v5 0/4] Add I2C support to ST SoCs

[PATCH v5 0/4] Add I2C support to ST SoCs

[Maxime COQUELIN]

The goal of this series is to add I2C support to ST SoCs.
The DT definition is added for STiH415 and STiH416 SoCs on
B2000 and B2020 boards.

The series has been tested working on STiH416-B2020 board.
It applies on top of v3.12-rc5.

Changes since v4:
  - Fixed IRQ flags to level high triggering as reported by Stephen Gallimore
  - Revert back to generic DT properties for Anti-glitch filtering. Their DT
    binding documentation will be updated when i2c DT binding docuementation
    will be created (In Wolfram's ToDo list)
  - Rebased on v3.12-rc5

Changes since v3:
  - Switch back to vendor specific DT properties regarding the anti-glitch
    filter configuration, as this IP is the only one having such a filter
  - Add DT binding documentation regarding pinctrl-related properties
  - Move recommended properties to optional properties in DT binding
    documentation
  - Rebased on v3.12-rc4

Changes since v2:
  - Create generic DT property for Anti-glitch filtering configuration
  - Sort i2c-st entries in Makefile and Kconfig
  - Various cosmetics changes
  - Return IRQ_NONE in case of spurious interrupt
  - Use INIT_COMPLETION instead of init_completion in st_i2c_xfer_msg
  - Simplify st_i2c_xfer as proposed by Wolfram
  - Fix PM ops issues
  - Change author name to myself and fix license to GPL v2
  - Include .h file content into the .c file

Changes since v1:
  - Anti-glitch filtering simplified (Only HW Anti-glitch is kept)
  - Update I2C timings to comply with I2C specification
  - Handle spurious interrupts properly
  - DT binding updates taking into account Stephen's comments
  - Use "clock-names" property to comply with GCF
  - DT cosmetic changes reported by Lee and Gabriel 

Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>

Maxime Coquelin (4):
  i2c: busses: i2c-st: Add ST I2C controller
  ARM: STi: Supply I2C configuration to STiH416 SoC
  ARM: STi: Supply I2C configuration to STiH415 SoC
  ARM: STi: Add I2C config to B2000 and B2020 boards

 Documentation/devicetree/bindings/i2c/i2c-st.txt |   38 +
 arch/arm/boot/dts/stih415-pinctrl.dtsi           |   36 +
 arch/arm/boot/dts/stih415.dtsi                   |   53 ++
 arch/arm/boot/dts/stih416-pinctrl.dtsi           |   35 +
 arch/arm/boot/dts/stih416.dtsi                   |   53 ++
 arch/arm/boot/dts/stih41x-b2000.dtsi             |    9 +
 arch/arm/boot/dts/stih41x-b2020.dtsi             |   22 +
 drivers/i2c/busses/Kconfig                       |   10 +
 drivers/i2c/busses/Makefile                      |    1 +
 drivers/i2c/busses/i2c-st.c                      |  867 ++++++++++++++++++++++
 10 files changed, 1124 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-st.txt
 create mode 100644 drivers/i2c/busses/i2c-st.c

-- 
1.7.9.5

[PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Maxime COQUELIN]

This patch adds support to SSC (Synchronous Serial Controller)
I2C driver. This IP also supports SPI protocol, but this is not
the aim of this driver.

This IP is embedded in all ST SoCs for Set-top box platorms, and
supports I2C Standard and Fast modes.

Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
---
 Documentation/devicetree/bindings/i2c/i2c-st.txt |   38 +
 drivers/i2c/busses/Kconfig                       |   10 +
 drivers/i2c/busses/Makefile                      |    1 +
 drivers/i2c/busses/i2c-st.c                      |  867 ++++++++++++++++++++++
 4 files changed, 916 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-st.txt
 create mode 100644 drivers/i2c/busses/i2c-st.c

diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt
new file mode 100644
index 0000000..8b2fd0b
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-st.txt
@@ -0,0 +1,38 @@
+ST SSC binding, for I2C mode operation
+
+Required properties :
+- compatible : Must be "st,comms-i2c"
+- reg : Offset and length of the register set for the device
+- interrupts : the interrupt specifier
+- clock-names: Must contain "ssc".
+- clocks: Must contain an entry for each name in clock-names. See the common
+  clock bindings.
+- A pinctrl state named "default" must be defined, using the bindings in
+  ../pinctrl/pinctrl-binding.txt
+
+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.
+- i2c-min-scl-pulse-width-us : The minimum valid SCL pulse width that is
+  allowed through the deglitch circuit. In units of us.
+- i2c-min-sda-pulse-width-us : The minimum valid SDA pulse width that is
+  allowed through the deglitch circuit. In units of us.
+- A pinctrl state named "sleep" could be defined, using the bindings in
+  ../pinctrl/pinctrl-binding.txt
+
+
+Example :
+
+i2c0: i2c@fed40000 {
+	compatible	= "st,comms-ssc-i2c";
+	reg		= <0xfed40000 0x110>;
+	interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
+	clocks		= <&CLK_S_ICN_REG_0>;
+	clock-names	= "ssc";
+	clock-frequency = <400000>;
+	pinctrl-names	= "default";
+	pinctrl-0	= <&pinctrl_i2c0_default>;
+	i2c-min-scl-pulse-width-us = <0>;
+	i2c-min-sda-pulse-width-us = <5>;
+};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index cdcbd83..67f85f9 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -695,6 +695,16 @@ config I2C_SIRF
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-sirf.
 
+config I2C_ST
+	tristate "STMicroelectronics SSC I2C support"
+	depends on ARCH_STI
+	help
+	  Enable this option to add support for STMicroelectronics SoCs
+	  hardware SSC (Synchronous Serial Controller) as an I2C controller.
+
+	  This driver can also be built as module. If so, the module
+	  will be called i2c-st.
+
 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 d00997f..a0f52e4 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_I2C_SH7760)	+= i2c-sh7760.o
 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_STU300)	+= i2c-stu300.o
 obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
 obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
diff --git a/drivers/i2c/busses/i2c-st.c b/drivers/i2c/busses/i2c-st.c
new file mode 100644
index 0000000..a22e0b2
--- /dev/null
+++ b/drivers/i2c/busses/i2c-st.c
@@ -0,0 +1,867 @@
+/*
+ * Copyright (C) 2013 STMicroelectronics
+ *
+ * I2C master mode controller driver, used in STMicroelectronics devices.
+ *
+ * Author: Maxime Coquelin <maxime.coquelin@st.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2, as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/i2c.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+/* SSC registers */
+#define SSC_BRG				0x000
+#define SSC_TBUF			0x004
+#define SSC_RBUF			0x008
+#define SSC_CTL				0x00C
+#define SSC_IEN				0x010
+#define SSC_STA				0x014
+#define SSC_I2C				0x018
+#define SSC_SLAD			0x01C
+#define SSC_REP_START_HOLD		0x020
+#define SSC_START_HOLD			0x024
+#define SSC_REP_START_SETUP		0x028
+#define SSC_DATA_SETUP			0x02C
+#define SSC_STOP_SETUP			0x030
+#define SSC_BUS_FREE			0x034
+#define SSC_TX_FSTAT			0x038
+#define SSC_RX_FSTAT			0x03C
+#define SSC_PRE_SCALER_BRG		0x040
+#define SSC_CLR				0x080
+#define SSC_NOISE_SUPP_WIDTH		0x100
+#define SSC_PRSCALER			0x104
+#define SSC_NOISE_SUPP_WIDTH_DATAOUT	0x108
+#define SSC_PRSCALER_DATAOUT		0x10c
+
+/* SSC Control */
+#define SSC_CTL_DATA_WIDTH_9		0x8
+#define SSC_CTL_DATA_WIDTH_MSK		0xf
+#define SSC_CTL_BM			0xf
+#define SSC_CTL_HB			BIT(4)
+#define SSC_CTL_PH			BIT(5)
+#define SSC_CTL_PO			BIT(6)
+#define SSC_CTL_SR			BIT(7)
+#define SSC_CTL_MS			BIT(8)
+#define SSC_CTL_EN			BIT(9)
+#define SSC_CTL_LPB			BIT(10)
+#define SSC_CTL_EN_TX_FIFO		BIT(11)
+#define SSC_CTL_EN_RX_FIFO		BIT(12)
+#define SSC_CTL_EN_CLST_RX		BIT(13)
+
+/* SSC Interrupt Enable */
+#define SSC_IEN_RIEN			BIT(0)
+#define SSC_IEN_TIEN			BIT(1)
+#define SSC_IEN_TEEN			BIT(2)
+#define SSC_IEN_REEN			BIT(3)
+#define SSC_IEN_PEEN			BIT(4)
+#define SSC_IEN_AASEN			BIT(6)
+#define SSC_IEN_STOPEN			BIT(7)
+#define SSC_IEN_ARBLEN			BIT(8)
+#define SSC_IEN_NACKEN			BIT(10)
+#define SSC_IEN_REPSTRTEN		BIT(11)
+#define SSC_IEN_TX_FIFO_HALF		BIT(12)
+#define SSC_IEN_RX_FIFO_HALF_FULL	BIT(14)
+
+/* SSC Status */
+#define SSC_STA_RIR			BIT(0)
+#define SSC_STA_TIR			BIT(1)
+#define SSC_STA_TE			BIT(2)
+#define SSC_STA_RE			BIT(3)
+#define SSC_STA_PE			BIT(4)
+#define SSC_STA_CLST			BIT(5)
+#define SSC_STA_AAS			BIT(6)
+#define SSC_STA_STOP			BIT(7)
+#define SSC_STA_ARBL			BIT(8)
+#define SSC_STA_BUSY			BIT(9)
+#define SSC_STA_NACK			BIT(10)
+#define SSC_STA_REPSTRT			BIT(11)
+#define SSC_STA_TX_FIFO_HALF		BIT(12)
+#define SSC_STA_TX_FIFO_FULL		BIT(13)
+#define SSC_STA_RX_FIFO_HALF		BIT(14)
+
+/* SSC I2C Control */
+#define SSC_I2C_I2CM			BIT(0)
+#define SSC_I2C_STRTG			BIT(1)
+#define SSC_I2C_STOPG			BIT(2)
+#define SSC_I2C_ACKG			BIT(3)
+#define SSC_I2C_AD10			BIT(4)
+#define SSC_I2C_TXENB			BIT(5)
+#define SSC_I2C_REPSTRTG		BIT(11)
+#define SSC_I2C_SLAVE_DISABLE		BIT(12)
+
+/* SSC Tx FIFO Status */
+#define SSC_TX_FSTAT_STATUS		0x07
+
+/* SSC Rx FIFO Status */
+#define SSC_RX_FSTAT_STATUS		0x07
+
+/* SSC Clear bit operation */
+#define SSC_CLR_SSCAAS			BIT(6)
+#define SSC_CLR_SSCSTOP			BIT(7)
+#define SSC_CLR_SSCARBL			BIT(8)
+#define SSC_CLR_NACK			BIT(10)
+#define SSC_CLR_REPSTRT			BIT(11)
+
+/* SSC Clock Prescaler */
+#define SSC_PRSC_VALUE			0x0f
+
+
+#define SSC_TXFIFO_SIZE			0x8
+#define SSC_RXFIFO_SIZE			0x8
+
+enum st_i2c_mode {
+	I2C_MODE_STANDARD,
+	I2C_MODE_FAST,
+	I2C_MODE_END,
+};
+
+/**
+ * struct st_i2c_timings - per-Mode tuning parameters
+ * @rate: I2C bus rate
+ * @rep_start_hold: I2C repeated start hold time requirement
+ * @rep_start_setup: I2C repeated start set up time requirement
+ * @start_hold: I2C start hold time requirement
+ * @data_setup_time: I2C data set up time requirement
+ * @stop_setup_time: I2C stop set up time requirement
+ * @bus_free_time: I2C bus free time requirement
+ * @sda_pulse_min_limit: I2C SDA pulse mini width limit
+ */
+struct st_i2c_timings {
+	u32 rate;
+	u32 rep_start_hold;
+	u32 rep_start_setup;
+	u32 start_hold;
+	u32 data_setup_time;
+	u32 stop_setup_time;
+	u32 bus_free_time;
+	u32 sda_pulse_min_limit;
+};
+
+/**
+ * struct st_i2c_deglitch - Anti-glitch filter configuration
+ * @scl_min_width_us: SCL line minimum pulse width in ns
+ * @sda_min_width_us: SDA line minimum pulse width in ns
+ */
+struct st_i2c_deglitch {
+	u32	scl_min_width_us;
+	u32	sda_min_width_us;
+};
+
+/**
+ * struct st_i2c_client - client specific data
+ * @addr: 8-bit slave addr, including r/w bit
+ * @count: number of bytes to be transfered
+ * @xfered: number of bytes already transferred
+ * @buf: data buffer
+ * @result: result of the transfer
+ * @stop: last I2C msg to be sent, i.e. STOP to be generated
+ */
+struct st_i2c_client {
+	u8	addr;
+	u32	count;
+	u32	xfered;
+	u8	*buf;
+	int	result;
+	bool	stop;
+};
+
+/**
+ * struct st_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
+ * @irq: interrupt line for th controller
+ * @clk: hw ssc block clock
+ * @mode: I2C mode of the controller. Standard or Fast only supported
+ * @deglitch: Anti-glitch filters parameters
+ * @client: I2C transfert information
+ * @busy: I2C transfer on-going
+ */
+struct st_i2c_dev {
+	struct i2c_adapter	adap;
+	struct device		*dev;
+	void __iomem		*base;
+	struct completion	complete;
+	int			irq;
+	struct clk		*clk;
+	int			mode;
+	struct st_i2c_deglitch	deglitch;
+	struct st_i2c_client	client;
+	bool			busy;
+};
+
+static inline void st_i2c_set_bits(void __iomem *reg, u32 mask)
+{
+	writel(readl(reg) | mask, reg);
+}
+
+static inline void st_i2c_clr_bits(void __iomem *reg, u32 mask)
+{
+	writel(readl(reg) & ~mask, reg);
+}
+
+/* From I2C Specifications v0.5 */
+static struct st_i2c_timings i2c_timings[] = {
+	[I2C_MODE_STANDARD] = {
+		.rate			= 100000,
+		.rep_start_hold		= 4000,
+		.rep_start_setup	= 4700,
+		.start_hold		= 4000,
+		.data_setup_time	= 250,
+		.stop_setup_time	= 4000,
+		.bus_free_time		= 4700,
+	},
+	[I2C_MODE_FAST] = {
+		.rate			= 400000,
+		.rep_start_hold		= 600,
+		.rep_start_setup	= 600,
+		.start_hold		= 600,
+		.data_setup_time	= 100,
+		.stop_setup_time	= 600,
+		.bus_free_time		= 1300,
+	},
+};
+
+static void st_i2c_flush_rx_fifo(struct st_i2c_dev *i2c_dev)
+{
+	int count, i;
+
+	/*
+	 * Counter only counts up to 7 but fifo size is 8...
+	 * When fifo is full, counter is 0 and RIR bit of status register is
+	 * set
+	 */
+	if (readl(i2c_dev->base + SSC_STA) & SSC_STA_RIR)
+		count = SSC_RXFIFO_SIZE;
+	else
+		count = readl(i2c_dev->base + SSC_RX_FSTAT) &
+			SSC_RX_FSTAT_STATUS;
+
+	for (i = 0; i < count; i++)
+		readl(i2c_dev->base + SSC_RBUF);
+}
+
+static void st_i2c_soft_reset(struct st_i2c_dev *i2c_dev)
+{
+	/*
+	 * FIFO needs to be emptied before reseting the IP,
+	 * else the controller raises a BUSY error.
+	 */
+	st_i2c_flush_rx_fifo(i2c_dev);
+
+	st_i2c_set_bits(i2c_dev->base + SSC_CTL, SSC_CTL_SR);
+	st_i2c_clr_bits(i2c_dev->base + SSC_CTL, SSC_CTL_SR);
+}
+
+/**
+ * st_i2c_hw_config() - Prepare SSC block, calculate and apply tuning timings
+ * @i2c_dev: Controller's private data
+ */
+static void st_i2c_hw_config(struct st_i2c_dev *i2c_dev)
+{
+	unsigned long rate;
+	u32 val, ns_per_clk;
+	struct st_i2c_timings *t = &i2c_timings[i2c_dev->mode];
+
+	st_i2c_soft_reset(i2c_dev);
+
+	val = SSC_CLR_REPSTRT | SSC_CLR_NACK | SSC_CLR_SSCARBL |
+		SSC_CLR_SSCAAS | SSC_CLR_SSCSTOP;
+	writel(val, i2c_dev->base + SSC_CLR);
+
+	/* SSC Control register setup */
+	val = SSC_CTL_PO | SSC_CTL_PH | SSC_CTL_HB | SSC_CTL_DATA_WIDTH_9;
+	writel(val, i2c_dev->base + SSC_CTL);
+
+	rate = clk_get_rate(i2c_dev->clk);
+	ns_per_clk = 1000000000 / rate;
+
+	/* Baudrate */
+	val = rate / (2 * t->rate);
+	writel(val, i2c_dev->base + SSC_BRG);
+
+	/* Pre-scaler baudrate */
+	writel(1, i2c_dev->base + SSC_PRE_SCALER_BRG);
+
+	/* Enable I2C mode */
+	writel(SSC_I2C_I2CM, i2c_dev->base + SSC_I2C);
+
+	/* Repeated start hold time */
+	val = t->rep_start_hold / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_REP_START_HOLD);
+
+	/* Repeated start set up time */
+	val = t->rep_start_setup / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_REP_START_SETUP);
+
+	/* Start hold time */
+	val = t->start_hold / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_START_HOLD);
+
+	/* Data set up time */
+	val = t->data_setup_time / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_DATA_SETUP);
+
+	/* Stop set up time */
+	val = t->stop_setup_time / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_STOP_SETUP);
+
+	/* Bus free time */
+	val = t->bus_free_time / ns_per_clk;
+	writel(val, i2c_dev->base + SSC_BUS_FREE);
+
+	/* Prescalers set up */
+	val = rate / 10000000;
+	writel(val, i2c_dev->base + SSC_PRSCALER);
+	writel(val, i2c_dev->base + SSC_PRSCALER_DATAOUT);
+
+	/* Noise suppression witdh */
+	val = i2c_dev->deglitch.scl_min_width_us * rate / 100000000;
+	writel(val, i2c_dev->base + SSC_NOISE_SUPP_WIDTH);
+
+	/* Noise suppression max output data delay width */
+	val = i2c_dev->deglitch.sda_min_width_us * rate / 100000000;
+	writel(val, i2c_dev->base + SSC_NOISE_SUPP_WIDTH_DATAOUT);
+}
+
+static int st_i2c_wait_free_bus(struct st_i2c_dev *i2c_dev)
+{
+	u32 sta;
+	int i;
+
+	for (i = 0; i < 10; i++) {
+		sta = readl(i2c_dev->base + SSC_STA);
+		if (!(sta & SSC_STA_BUSY))
+			return 0;
+
+		usleep_range(2000, 4000);
+	}
+
+	dev_err(i2c_dev->dev, "bus not free (status = 0x%08x)\n", sta);
+
+	return -EBUSY;
+}
+
+/**
+ * st_i2c_write_tx_fifo() - Write a byte in the Tx FIFO
+ * @i2c_dev: Controller's private data
+ * @byte: Data to write in the Tx FIFO
+ */
+static inline void st_i2c_write_tx_fifo(struct st_i2c_dev *i2c_dev, u8 byte)
+{
+	u16 tbuf = byte << 1;
+
+	writel(tbuf | 1, i2c_dev->base + SSC_TBUF);
+}
+
+/**
+ * st_i2c_wr_fill_tx_fifo() - Fill the Tx FIFO in write mode
+ * @i2c_dev: Controller's private data
+ *
+ * This functions fills the Tx FIFO with I2C transfert buffer when
+ * in write mode.
+ */
+static void st_i2c_wr_fill_tx_fifo(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 tx_fstat, sta;
+	int i;
+
+	sta = readl(i2c_dev->base + SSC_STA);
+	if (sta & SSC_STA_TX_FIFO_FULL)
+		return;
+
+	tx_fstat = readl(i2c_dev->base + SSC_TX_FSTAT) & SSC_TX_FSTAT_STATUS;
+
+	if (c->count < (SSC_TXFIFO_SIZE - tx_fstat))
+		i = c->count;
+	else
+		i = SSC_TXFIFO_SIZE - tx_fstat;
+
+	for (; i > 0; i--, c->count--, c->buf++)
+		st_i2c_write_tx_fifo(i2c_dev, *c->buf);
+}
+
+/**
+ * st_i2c_rd_fill_tx_fifo() - Fill the Tx FIFO in read mode
+ * @i2c_dev: Controller's private data
+ *
+ * This functions fills the Tx FIFO with fixed pattern when
+ * in read mode to trigger clock.
+ */
+static void st_i2c_rd_fill_tx_fifo(struct st_i2c_dev *i2c_dev, int max)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 tx_fstat, sta;
+	int i;
+
+	sta = readl(i2c_dev->base + SSC_STA);
+	if (sta & SSC_STA_TX_FIFO_FULL)
+		return;
+
+	tx_fstat = readl(i2c_dev->base + SSC_TX_FSTAT) & SSC_TX_FSTAT_STATUS;
+
+	if (max < (SSC_TXFIFO_SIZE - tx_fstat))
+		i = max;
+	else
+		i = SSC_TXFIFO_SIZE - tx_fstat;
+
+	for (; i > 0; i--, c->xfered++)
+		st_i2c_write_tx_fifo(i2c_dev, 0xff);
+}
+
+static void st_i2c_read_rx_fifo(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 i, sta;
+	u16 rbuf;
+
+	sta = readl(i2c_dev->base + SSC_STA);
+	if (sta & SSC_STA_RIR) {
+		i = SSC_RXFIFO_SIZE;
+	} else {
+		i = readl(i2c_dev->base + SSC_RX_FSTAT);
+		i &= SSC_RX_FSTAT_STATUS;
+	}
+
+	for (; (i > 0) && (c->count > 0); i--, c->count--) {
+		rbuf = readl(i2c_dev->base + SSC_RBUF) >> 1;
+		*c->buf++ = (u8)rbuf & 0xff;
+	}
+
+	if (i) {
+		dev_err(i2c_dev->dev, "Unexpected %d bytes in rx fifo\n", i);
+		st_i2c_flush_rx_fifo(i2c_dev);
+	}
+}
+
+/**
+ * st_i2c_terminate_xfer() - Send either STOP or REPSTART condition
+ * @i2c_dev: Controller's private data
+ */
+static void st_i2c_terminate_xfer(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+
+	st_i2c_clr_bits(i2c_dev->base + SSC_IEN, SSC_IEN_TEEN);
+	st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STRTG);
+
+	if (c->stop) {
+		st_i2c_set_bits(i2c_dev->base + SSC_IEN, SSC_IEN_STOPEN);
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
+	} else {
+		st_i2c_set_bits(i2c_dev->base + SSC_IEN, SSC_IEN_REPSTRTEN);
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_REPSTRTG);
+	}
+}
+
+/**
+ * st_i2c_handle_write() - Handle FIFO empty interrupt in case of write
+ * @i2c_dev: Controller's private data
+ */
+static void st_i2c_handle_write(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+
+	st_i2c_flush_rx_fifo(i2c_dev);
+
+	if (!c->count)
+		/* End of xfer, send stop or repstart */
+		st_i2c_terminate_xfer(i2c_dev);
+	else
+		st_i2c_wr_fill_tx_fifo(i2c_dev);
+}
+
+/**
+ * st_i2c_handle_write() - Handle FIFO enmpty interrupt in case of read
+ * @i2c_dev: Controller's private data
+ */
+static void st_i2c_handle_read(struct st_i2c_dev *i2c_dev)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 ien;
+
+	/* Trash the address read back */
+	if (!c->xfered) {
+		readl(i2c_dev->base + SSC_RBUF);
+		st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_TXENB);
+	} else
+		st_i2c_read_rx_fifo(i2c_dev);
+
+	if (!c->count) {
+		/* End of xfer, send stop or repstart */
+		st_i2c_terminate_xfer(i2c_dev);
+	} else if (c->count == 1) {
+		/* Penultimate byte to xfer, disable ACK gen. */
+		st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_ACKG);
+
+		/* Last received byte is to be handled by NACK interrupt */
+		ien = SSC_IEN_NACKEN | SSC_IEN_ARBLEN;
+		writel(ien, i2c_dev->base + SSC_IEN);
+
+		st_i2c_rd_fill_tx_fifo(i2c_dev, c->count);
+	} else
+		st_i2c_rd_fill_tx_fifo(i2c_dev, c->count - 1);
+}
+
+/**
+ * st_i2c_isr() - Interrupt routine
+ * @irq: interrupt number
+ * @data: Controller's private data
+ */
+static irqreturn_t st_i2c_isr(int irq, void *data)
+{
+	struct st_i2c_dev *i2c_dev = data;
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 sta, ien;
+	int it;
+
+	ien = readl(i2c_dev->base + SSC_IEN);
+	sta = readl(i2c_dev->base + SSC_STA);
+
+	/* Use __fls() to check error bits first */
+	it = __fls(sta & ien);
+	if (it < 0) {
+		dev_dbg(i2c_dev->dev, "spurious it (sta=0x%04x, ien=0x%04x)\n",
+				sta, ien);
+		return IRQ_NONE;
+	}
+
+	switch (1 << it) {
+	case SSC_STA_TE:
+		if (c->addr & I2C_M_RD)
+			st_i2c_handle_read(i2c_dev);
+		else
+			st_i2c_handle_write(i2c_dev);
+		break;
+
+	case SSC_STA_STOP:
+	case SSC_STA_REPSTRT:
+		writel(0, i2c_dev->base + SSC_IEN);
+		complete(&i2c_dev->complete);
+		break;
+
+	case SSC_STA_NACK:
+		writel(SSC_CLR_NACK, i2c_dev->base + SSC_CLR);
+
+		/* Last received byte handled by NACK interrupt */
+		if ((c->addr & I2C_M_RD) && (c->count == 1) && (c->xfered)) {
+			st_i2c_handle_read(i2c_dev);
+			break;
+		}
+
+		it = SSC_IEN_STOPEN | SSC_IEN_ARBLEN;
+		writel(it, i2c_dev->base + SSC_IEN);
+
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
+		c->result = -EIO;
+		break;
+
+	case SSC_STA_ARBL:
+		writel(SSC_CLR_SSCARBL, i2c_dev->base + SSC_CLR);
+
+		it = SSC_IEN_STOPEN | SSC_IEN_ARBLEN;
+		writel(it, i2c_dev->base + SSC_IEN);
+
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
+		c->result = -EIO;
+		break;
+
+	default:
+		dev_err(i2c_dev->dev,
+				"it %d unhandled (sta=0x%04x)\n", it, sta);
+	}
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * st_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 st_i2c_xfer_msg(struct st_i2c_dev *i2c_dev, struct i2c_msg *msg,
+			    bool is_first, bool is_last)
+{
+	struct st_i2c_client *c = &i2c_dev->client;
+	u32 ctl, i2c, it;
+	unsigned long timeout;
+	int ret;
+
+	c->addr		= (u8)(msg->addr << 1);
+	c->addr		|= (msg->flags & I2C_M_RD);
+	c->buf		= msg->buf;
+	c->count	= msg->len;
+	c->xfered	= 0;
+	c->result	= 0;
+	c->stop		= is_last;
+
+	INIT_COMPLETION(i2c_dev->complete);
+
+	ctl = SSC_CTL_EN | SSC_CTL_MS |	SSC_CTL_EN_RX_FIFO | SSC_CTL_EN_TX_FIFO;
+	st_i2c_set_bits(i2c_dev->base + SSC_CTL, ctl);
+
+	i2c = SSC_I2C_TXENB;
+	if (c->addr & I2C_M_RD)
+		i2c |= SSC_I2C_ACKG;
+	st_i2c_set_bits(i2c_dev->base + SSC_I2C, i2c);
+
+	/* Write slave address */
+	st_i2c_write_tx_fifo(i2c_dev, c->addr);
+
+	/* Pre-fill Tx fifo with data in case of write */
+	if (!(c->addr & I2C_M_RD))
+		st_i2c_wr_fill_tx_fifo(i2c_dev);
+
+	it = SSC_IEN_NACKEN | SSC_IEN_TEEN | SSC_IEN_ARBLEN;
+	writel(it, i2c_dev->base + SSC_IEN);
+
+	if (is_first) {
+		ret = st_i2c_wait_free_bus(i2c_dev);
+		if (ret)
+			return ret;
+
+		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STRTG);
+	}
+
+	timeout = wait_for_completion_timeout(&i2c_dev->complete,
+			i2c_dev->adap.timeout);
+	ret = c->result;
+
+	if (!timeout) {
+		dev_err(i2c_dev->dev, "Write to slave 0x%x timed out\n",
+				c->addr);
+		ret = -ETIMEDOUT;
+	}
+
+	i2c = SSC_I2C_STOPG | SSC_I2C_REPSTRTG;
+	st_i2c_clr_bits(i2c_dev->base + SSC_I2C, i2c);
+
+	writel(SSC_CLR_SSCSTOP | SSC_CLR_REPSTRT, i2c_dev->base + SSC_CLR);
+
+	return ret;
+}
+
+/**
+ * st_i2c_xfer() - Transfer a single 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 st_i2c_xfer(struct i2c_adapter *i2c_adap,
+			struct i2c_msg msgs[], int num)
+{
+	struct st_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
+	int ret, i;
+
+	i2c_dev->busy = true;
+
+	ret = clk_prepare_enable(i2c_dev->clk);
+	if (ret) {
+		dev_err(i2c_dev->dev, "Failed to prepare_enable clock\n");
+		return ret;
+	}
+
+	pinctrl_pm_select_default_state(i2c_dev->dev);
+
+	st_i2c_hw_config(i2c_dev);
+
+	for (i = 0; (i < num) && !ret; i++)
+		ret = st_i2c_xfer_msg(i2c_dev, &msgs[i], i == 0, i == num - 1);
+
+	pinctrl_pm_select_idle_state(i2c_dev->dev);
+
+	clk_disable_unprepare(i2c_dev->clk);
+
+	i2c_dev->busy = false;
+
+	return (ret < 0) ? ret : i;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int st_i2c_suspend(struct device *dev)
+{
+	struct platform_device *pdev =
+		container_of(dev, struct platform_device, dev);
+	struct st_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	if (i2c_dev->busy)
+		return -EBUSY;
+
+	pinctrl_pm_select_sleep_state(dev);
+
+	return 0;
+}
+
+static int st_i2c_resume(struct device *dev)
+{
+	pinctrl_pm_select_default_state(dev);
+	/* Go in idle state if available */
+	pinctrl_pm_select_idle_state(dev);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(st_i2c_pm, st_i2c_suspend, st_i2c_resume);
+#define ST_I2C_PM	(&st_i2c_pm)
+#else
+#define ST_I2C_PM	NULL
+#endif
+
+static u32 st_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm st_i2c_algo = {
+	.master_xfer = st_i2c_xfer,
+	.functionality = st_i2c_func,
+};
+
+static int st_i2c_of_get_deglitch(struct device_node *np,
+		struct st_i2c_dev *i2c_dev)
+{
+	int ret;
+
+	ret = of_property_read_u32(np, "i2c-min-scl-pulse-width-us",
+			&i2c_dev->deglitch.scl_min_width_us);
+	if ((ret == -ENODATA) || (ret == -EOVERFLOW)) {
+		dev_err(i2c_dev->dev, "i2c-min-scl-pulse-width-us invalid\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "i2c-min-sda-pulse-width-us",
+			&i2c_dev->deglitch.sda_min_width_us);
+	if ((ret == -ENODATA) || (ret == -EOVERFLOW)) {
+		dev_err(i2c_dev->dev, "i2c-min-sda-pulse-width-us invalid\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int st_i2c_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct st_i2c_dev *i2c_dev;
+	struct resource *res;
+	u32 clk_rate;
+	struct i2c_adapter *adap;
+	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);
+
+	i2c_dev->irq = irq_of_parse_and_map(np, 0);
+	if (!i2c_dev->irq) {
+		dev_err(&pdev->dev, "IRQ missing or invalid\n");
+		return -EINVAL;
+	}
+
+	i2c_dev->clk = of_clk_get_by_name(np, "ssc");
+	if (IS_ERR(i2c_dev->clk)) {
+		dev_err(&pdev->dev, "Unable to request clock\n");
+		return PTR_ERR(i2c_dev->clk);
+	}
+
+	i2c_dev->mode = I2C_MODE_STANDARD;
+	ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
+	if ((!ret) && (clk_rate == 400000))
+		i2c_dev->mode = I2C_MODE_FAST;
+
+	i2c_dev->dev = &pdev->dev;
+
+	ret = devm_request_irq(&pdev->dev, i2c_dev->irq, st_i2c_isr, 0,
+			pdev->name, i2c_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
+		return ret;
+	}
+
+	pinctrl_pm_select_default_state(i2c_dev->dev);
+	/* In case idle state available, select it */
+	pinctrl_pm_select_idle_state(i2c_dev->dev);
+
+	ret = st_i2c_of_get_deglitch(np, i2c_dev);
+	if (ret)
+		return ret;
+
+	adap = &i2c_dev->adap;
+	i2c_set_adapdata(adap, i2c_dev);
+	snprintf(adap->name, sizeof(adap->name), "ST I2C(0x%08x)", res->start);
+	adap->owner = THIS_MODULE;
+	adap->timeout = 2 * HZ;
+	adap->retries = 0;
+	adap->class = I2C_CLASS_HWMON | I2C_CLASS_DDC | I2C_CLASS_SPD;
+	adap->algo = &st_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) {
+		dev_err(&pdev->dev, "Failed to add adapter\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, i2c_dev);
+
+	dev_info(i2c_dev->dev, "%s initialized\n", adap->name);
+
+	return 0;
+}
+
+static int st_i2c_remove(struct platform_device *pdev)
+{
+	struct st_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c_dev->adap);
+
+	return 0;
+}
+
+static struct of_device_id st_i2c_match[] = {
+	{ .compatible = "st,comms-ssc-i2c", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, st_i2c_match);
+
+static struct platform_driver st_i2c_driver = {
+	.driver = {
+		.name = "st-i2c",
+		.owner = THIS_MODULE,
+		.of_match_table = st_i2c_match,
+		.pm = ST_I2C_PM,
+	},
+	.probe = st_i2c_probe,
+	.remove = st_i2c_remove,
+};
+
+module_platform_driver(st_i2c_driver);
+
+MODULE_AUTHOR("Maxime Coquelin <maxime.coquelin@st.com>");
+MODULE_DESCRIPTION("STMicroelectronics I2C driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Jean-Christophe PLAGNIOL-VILLARD]

> +/**
> + * struct st_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
> + * @irq: interrupt line for th controller
> + * @clk: hw ssc block clock
> + * @mode: I2C mode of the controller. Standard or Fast only supported
> + * @deglitch: Anti-glitch filters parameters
> + * @client: I2C transfert information
> + * @busy: I2C transfer on-going
> + */
> +struct st_i2c_dev {
> +	struct i2c_adapter	adap;
> +	struct device		*dev;
> +	void __iomem		*base;
> +	struct completion	complete;
> +	int			irq;
> +	struct clk		*clk;
> +	int			mode;
> +	struct st_i2c_deglitch	deglitch;
> +	struct st_i2c_client	client;
> +	bool			busy;
> +};
> +
> +static inline void st_i2c_set_bits(void __iomem *reg, u32 mask)
> +{
> +	writel(readl(reg) | mask, reg);
> +}
> +
> +static inline void st_i2c_clr_bits(void __iomem *reg, u32 mask)
> +{
> +	writel(readl(reg) & ~mask, reg);
use set_bit api and use relaxed version
> +}
> +
> +/* From I2C Specifications v0.5 */
> +static struct st_i2c_timings i2c_timings[] = {
> +	[I2C_MODE_STANDARD] = {
> +		.rate			= 100000,
> +		.rep_start_hold		= 4000,
> +		.rep_start_setup	= 4700,
> +		.start_hold		= 4000,
> +		.data_setup_time	= 250,
> +		.stop_setup_time	= 4000,
> +		.bus_free_time		= 4700,
> +	},
> +	[I2C_MODE_FAST] = {
> +		.rate			= 400000,
> +		.rep_start_hold		= 600,
> +		.rep_start_setup	= 600,
> +		.start_hold		= 600,
> +		.data_setup_time	= 100,
> +		.stop_setup_time	= 600,
> +		.bus_free_time		= 1300,
> +	},
> +};

so how do you plane to support other rate that 100k and 400k?
> +
> +static void st_i2c_flush_rx_fifo(struct st_i2c_dev *i2c_dev)
> +{
> +	int count, i;
> +
> +	/*
> +	 * Counter only counts up to 7 but fifo size is 8...
> +	 * When fifo is full, counter is 0 and RIR bit of status register is
> +	 * set
> +	 */
> +	if (readl(i2c_dev->base + SSC_STA) & SSC_STA_RIR)
> +		count = SSC_RXFIFO_SIZE;
> +	else
> +		count = readl(i2c_dev->base + SSC_RX_FSTAT) &
> +			SSC_RX_FSTAT_STATUS;
> +
> +	for (i = 0; i < count; i++)
> +		readl(i2c_dev->base + SSC_RBUF);
use readsl

use relaxed version as much as possible

> +}
> +
> +static void st_i2c_soft_reset(struct st_i2c_dev *i2c_dev)
> +{
> +	/*
> +	 * FIFO needs to be emptied before reseting the IP,
> +	 * else the controller raises a BUSY error.
> +	 */
> +	st_i2c_flush_rx_fifo(i2c_dev);
> +
> +	st_i2c_set_bits(i2c_dev->base + SSC_CTL, SSC_CTL_SR);
> +	st_i2c_clr_bits(i2c_dev->base + SSC_CTL, SSC_CTL_SR);
> +}
> +
> +/**
> + * st_i2c_hw_config() - Prepare SSC block, calculate and apply tuning timings
> + * @i2c_dev: Controller's private data
> + */
> +static void st_i2c_hw_config(struct st_i2c_dev *i2c_dev)
> +{
> +	unsigned long rate;
> +	u32 val, ns_per_clk;
> +	struct st_i2c_timings *t = &i2c_timings[i2c_dev->mode];
> +
> +	st_i2c_soft_reset(i2c_dev);
> +
> +	val = SSC_CLR_REPSTRT | SSC_CLR_NACK | SSC_CLR_SSCARBL |
> +		SSC_CLR_SSCAAS | SSC_CLR_SSCSTOP;
> +	writel(val, i2c_dev->base + SSC_CLR);
> +
> +	/* SSC Control register setup */
> +	val = SSC_CTL_PO | SSC_CTL_PH | SSC_CTL_HB | SSC_CTL_DATA_WIDTH_9;
> +	writel(val, i2c_dev->base + SSC_CTL);
> +
> +	rate = clk_get_rate(i2c_dev->clk);
> +	ns_per_clk = 1000000000 / rate;
> +
> +	/* Baudrate */
> +	val = rate / (2 * t->rate);
> +	writel(val, i2c_dev->base + SSC_BRG);
> +
> +	/* Pre-scaler baudrate */
> +	writel(1, i2c_dev->base + SSC_PRE_SCALER_BRG);
> +
> +	/* Enable I2C mode */
> +	writel(SSC_I2C_I2CM, i2c_dev->base + SSC_I2C);
> +
> +	/* Repeated start hold time */
> +	val = t->rep_start_hold / ns_per_clk;
> +	writel(val, i2c_dev->base + SSC_REP_START_HOLD);
> +
> +	/* Repeated start set up time */
> +	val = t->rep_start_setup / ns_per_clk;
> +	writel(val, i2c_dev->base + SSC_REP_START_SETUP);
> +
> +	/* Start hold time */
> +	val = t->start_hold / ns_per_clk;
> +	writel(val, i2c_dev->base + SSC_START_HOLD);
> +
> +	/* Data set up time */
> +	val = t->data_setup_time / ns_per_clk;
> +	writel(val, i2c_dev->base + SSC_DATA_SETUP);
> +
> +	/* Stop set up time */
> +	val = t->stop_setup_time / ns_per_clk;
> +	writel(val, i2c_dev->base + SSC_STOP_SETUP);
> +
> +	/* Bus free time */
> +	val = t->bus_free_time / ns_per_clk;
> +	writel(val, i2c_dev->base + SSC_BUS_FREE);
> +
> +	/* Prescalers set up */
> +	val = rate / 10000000;
> +	writel(val, i2c_dev->base + SSC_PRSCALER);
> +	writel(val, i2c_dev->base + SSC_PRSCALER_DATAOUT);
> +
> +	/* Noise suppression witdh */
> +	val = i2c_dev->deglitch.scl_min_width_us * rate / 100000000;
> +	writel(val, i2c_dev->base + SSC_NOISE_SUPP_WIDTH);
> +
> +	/* Noise suppression max output data delay width */
> +	val = i2c_dev->deglitch.sda_min_width_us * rate / 100000000;
> +	writel(val, i2c_dev->base + SSC_NOISE_SUPP_WIDTH_DATAOUT);
> +}
> +
> +static int st_i2c_wait_free_bus(struct st_i2c_dev *i2c_dev)
> +{
> +	u32 sta;
> +	int i;
> +
> +	for (i = 0; i < 10; i++) {
> +		sta = readl(i2c_dev->base + SSC_STA);
> +		if (!(sta & SSC_STA_BUSY))
> +			return 0;
> +
> +		usleep_range(2000, 4000);
can not use interrupt?
> +	}
> +
> +	dev_err(i2c_dev->dev, "bus not free (status = 0x%08x)\n", sta);
> +
> +	return -EBUSY;
> +}
> +
> +/**
> + * st_i2c_write_tx_fifo() - Write a byte in the Tx FIFO
> + * @i2c_dev: Controller's private data
> + * @byte: Data to write in the Tx FIFO
> + */
> +static inline void st_i2c_write_tx_fifo(struct st_i2c_dev *i2c_dev, u8 byte)
> +{
> +	u16 tbuf = byte << 1;
> +
> +	writel(tbuf | 1, i2c_dev->base + SSC_TBUF);
> +}
> +
> +/**
> + * st_i2c_wr_fill_tx_fifo() - Fill the Tx FIFO in write mode
> + * @i2c_dev: Controller's private data
> + *
> + * This functions fills the Tx FIFO with I2C transfert buffer when
> + * in write mode.
> + */
> +static void st_i2c_wr_fill_tx_fifo(struct st_i2c_dev *i2c_dev)
> +{
> +	struct st_i2c_client *c = &i2c_dev->client;
> +	u32 tx_fstat, sta;
> +	int i;
> +
> +	sta = readl(i2c_dev->base + SSC_STA);
> +	if (sta & SSC_STA_TX_FIFO_FULL)
> +		return;
> +
> +	tx_fstat = readl(i2c_dev->base + SSC_TX_FSTAT) & SSC_TX_FSTAT_STATUS;
> +
> +	if (c->count < (SSC_TXFIFO_SIZE - tx_fstat))
> +		i = c->count;
> +	else
> +		i = SSC_TXFIFO_SIZE - tx_fstat;
> +
> +	for (; i > 0; i--, c->count--, c->buf++)
> +		st_i2c_write_tx_fifo(i2c_dev, *c->buf);
> +}
> +
> +/**
> + * st_i2c_rd_fill_tx_fifo() - Fill the Tx FIFO in read mode
> + * @i2c_dev: Controller's private data
> + *
> + * This functions fills the Tx FIFO with fixed pattern when
> + * in read mode to trigger clock.
> + */
> +static void st_i2c_rd_fill_tx_fifo(struct st_i2c_dev *i2c_dev, int max)
> +{
> +	struct st_i2c_client *c = &i2c_dev->client;
> +	u32 tx_fstat, sta;
> +	int i;
> +
> +	sta = readl(i2c_dev->base + SSC_STA);
> +	if (sta & SSC_STA_TX_FIFO_FULL)
> +		return;
> +
> +	tx_fstat = readl(i2c_dev->base + SSC_TX_FSTAT) & SSC_TX_FSTAT_STATUS;
> +
> +	if (max < (SSC_TXFIFO_SIZE - tx_fstat))
> +		i = max;
> +	else
> +		i = SSC_TXFIFO_SIZE - tx_fstat;
> +
> +	for (; i > 0; i--, c->xfered++)
> +		st_i2c_write_tx_fifo(i2c_dev, 0xff);
> +}
> +
> +static void st_i2c_read_rx_fifo(struct st_i2c_dev *i2c_dev)
> +{
> +	struct st_i2c_client *c = &i2c_dev->client;
> +	u32 i, sta;
> +	u16 rbuf;
> +
> +	sta = readl(i2c_dev->base + SSC_STA);
> +	if (sta & SSC_STA_RIR) {
> +		i = SSC_RXFIFO_SIZE;
> +	} else {
> +		i = readl(i2c_dev->base + SSC_RX_FSTAT);
> +		i &= SSC_RX_FSTAT_STATUS;
> +	}
> +
> +	for (; (i > 0) && (c->count > 0); i--, c->count--) {
> +		rbuf = readl(i2c_dev->base + SSC_RBUF) >> 1;
> +		*c->buf++ = (u8)rbuf & 0xff;
> +	}
> +
> +	if (i) {
> +		dev_err(i2c_dev->dev, "Unexpected %d bytes in rx fifo\n", i);
> +		st_i2c_flush_rx_fifo(i2c_dev);
> +	}
> +}
> +
> +/**
> + * st_i2c_terminate_xfer() - Send either STOP or REPSTART condition
> + * @i2c_dev: Controller's private data
> + */
> +static void st_i2c_terminate_xfer(struct st_i2c_dev *i2c_dev)
> +{
> +	struct st_i2c_client *c = &i2c_dev->client;
> +
> +	st_i2c_clr_bits(i2c_dev->base + SSC_IEN, SSC_IEN_TEEN);
> +	st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STRTG);
> +
> +	if (c->stop) {
> +		st_i2c_set_bits(i2c_dev->base + SSC_IEN, SSC_IEN_STOPEN);
> +		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
> +	} else {
> +		st_i2c_set_bits(i2c_dev->base + SSC_IEN, SSC_IEN_REPSTRTEN);
> +		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_REPSTRTG);
> +	}
> +}
> +
> +/**
> + * st_i2c_handle_write() - Handle FIFO empty interrupt in case of write
> + * @i2c_dev: Controller's private data
> + */
> +static void st_i2c_handle_write(struct st_i2c_dev *i2c_dev)
> +{
> +	struct st_i2c_client *c = &i2c_dev->client;
> +
> +	st_i2c_flush_rx_fifo(i2c_dev);
> +
> +	if (!c->count)
> +		/* End of xfer, send stop or repstart */
> +		st_i2c_terminate_xfer(i2c_dev);
> +	else
> +		st_i2c_wr_fill_tx_fifo(i2c_dev);
> +}
> +
> +/**
> + * st_i2c_handle_write() - Handle FIFO enmpty interrupt in case of read
> + * @i2c_dev: Controller's private data
> + */
> +static void st_i2c_handle_read(struct st_i2c_dev *i2c_dev)
> +{
> +	struct st_i2c_client *c = &i2c_dev->client;
> +	u32 ien;
> +
> +	/* Trash the address read back */
> +	if (!c->xfered) {
> +		readl(i2c_dev->base + SSC_RBUF);
> +		st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_TXENB);
> +	} else
> +		st_i2c_read_rx_fifo(i2c_dev);
> +
> +	if (!c->count) {
> +		/* End of xfer, send stop or repstart */
> +		st_i2c_terminate_xfer(i2c_dev);
> +	} else if (c->count == 1) {
> +		/* Penultimate byte to xfer, disable ACK gen. */
> +		st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_ACKG);
> +
> +		/* Last received byte is to be handled by NACK interrupt */
> +		ien = SSC_IEN_NACKEN | SSC_IEN_ARBLEN;
> +		writel(ien, i2c_dev->base + SSC_IEN);
> +
> +		st_i2c_rd_fill_tx_fifo(i2c_dev, c->count);
> +	} else
> +		st_i2c_rd_fill_tx_fifo(i2c_dev, c->count - 1);
> +}
> +
> +/**
> + * st_i2c_isr() - Interrupt routine
> + * @irq: interrupt number
> + * @data: Controller's private data
> + */
> +static irqreturn_t st_i2c_isr(int irq, void *data)
> +{
> +	struct st_i2c_dev *i2c_dev = data;
> +	struct st_i2c_client *c = &i2c_dev->client;
> +	u32 sta, ien;
> +	int it;
> +
> +	ien = readl(i2c_dev->base + SSC_IEN);
> +	sta = readl(i2c_dev->base + SSC_STA);
> +
> +	/* Use __fls() to check error bits first */
> +	it = __fls(sta & ien);
> +	if (it < 0) {
> +		dev_dbg(i2c_dev->dev, "spurious it (sta=0x%04x, ien=0x%04x)\n",
> +				sta, ien);
> +		return IRQ_NONE;
> +	}
> +
> +	switch (1 << it) {
> +	case SSC_STA_TE:
> +		if (c->addr & I2C_M_RD)
> +			st_i2c_handle_read(i2c_dev);
> +		else
> +			st_i2c_handle_write(i2c_dev);
> +		break;
> +
> +	case SSC_STA_STOP:
> +	case SSC_STA_REPSTRT:
> +		writel(0, i2c_dev->base + SSC_IEN);
> +		complete(&i2c_dev->complete);
> +		break;
> +
> +	case SSC_STA_NACK:
> +		writel(SSC_CLR_NACK, i2c_dev->base + SSC_CLR);
> +
> +		/* Last received byte handled by NACK interrupt */
> +		if ((c->addr & I2C_M_RD) && (c->count == 1) && (c->xfered)) {
> +			st_i2c_handle_read(i2c_dev);
> +			break;
> +		}
> +
> +		it = SSC_IEN_STOPEN | SSC_IEN_ARBLEN;
> +		writel(it, i2c_dev->base + SSC_IEN);
> +
> +		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
> +		c->result = -EIO;
> +		break;
> +
> +	case SSC_STA_ARBL:
> +		writel(SSC_CLR_SSCARBL, i2c_dev->base + SSC_CLR);
> +
> +		it = SSC_IEN_STOPEN | SSC_IEN_ARBLEN;
> +		writel(it, i2c_dev->base + SSC_IEN);
> +
> +		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STOPG);
> +		c->result = -EIO;
> +		break;
> +
> +	default:
> +		dev_err(i2c_dev->dev,
> +				"it %d unhandled (sta=0x%04x)\n", it, sta);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/**
> + * st_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 st_i2c_xfer_msg(struct st_i2c_dev *i2c_dev, struct i2c_msg *msg,
> +			    bool is_first, bool is_last)
> +{
> +	struct st_i2c_client *c = &i2c_dev->client;
> +	u32 ctl, i2c, it;
> +	unsigned long timeout;
> +	int ret;
> +
> +	c->addr		= (u8)(msg->addr << 1);
> +	c->addr		|= (msg->flags & I2C_M_RD);
> +	c->buf		= msg->buf;
> +	c->count	= msg->len;
> +	c->xfered	= 0;
> +	c->result	= 0;
> +	c->stop		= is_last;
> +
> +	INIT_COMPLETION(i2c_dev->complete);
> +
> +	ctl = SSC_CTL_EN | SSC_CTL_MS |	SSC_CTL_EN_RX_FIFO | SSC_CTL_EN_TX_FIFO;
> +	st_i2c_set_bits(i2c_dev->base + SSC_CTL, ctl);
> +
> +	i2c = SSC_I2C_TXENB;
> +	if (c->addr & I2C_M_RD)
> +		i2c |= SSC_I2C_ACKG;
> +	st_i2c_set_bits(i2c_dev->base + SSC_I2C, i2c);
> +
> +	/* Write slave address */
> +	st_i2c_write_tx_fifo(i2c_dev, c->addr);
> +
> +	/* Pre-fill Tx fifo with data in case of write */
> +	if (!(c->addr & I2C_M_RD))
> +		st_i2c_wr_fill_tx_fifo(i2c_dev);
> +
> +	it = SSC_IEN_NACKEN | SSC_IEN_TEEN | SSC_IEN_ARBLEN;
> +	writel(it, i2c_dev->base + SSC_IEN);
> +
> +	if (is_first) {
> +		ret = st_i2c_wait_free_bus(i2c_dev);
> +		if (ret)
> +			return ret;
> +
> +		st_i2c_set_bits(i2c_dev->base + SSC_I2C, SSC_I2C_STRTG);
> +	}
> +
> +	timeout = wait_for_completion_timeout(&i2c_dev->complete,
> +			i2c_dev->adap.timeout);
> +	ret = c->result;
> +
> +	if (!timeout) {
> +		dev_err(i2c_dev->dev, "Write to slave 0x%x timed out\n",
> +				c->addr);
> +		ret = -ETIMEDOUT;
> +	}
> +
> +	i2c = SSC_I2C_STOPG | SSC_I2C_REPSTRTG;
> +	st_i2c_clr_bits(i2c_dev->base + SSC_I2C, i2c);
> +
> +	writel(SSC_CLR_SSCSTOP | SSC_CLR_REPSTRT, i2c_dev->base + SSC_CLR);
> +
> +	return ret;
> +}
> +
> +/**
> + * st_i2c_xfer() - Transfer a single 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 st_i2c_xfer(struct i2c_adapter *i2c_adap,
> +			struct i2c_msg msgs[], int num)
> +{
> +	struct st_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
> +	int ret, i;
> +
> +	i2c_dev->busy = true;
> +
> +	ret = clk_prepare_enable(i2c_dev->clk);
> +	if (ret) {
> +		dev_err(i2c_dev->dev, "Failed to prepare_enable clock\n");
> +		return ret;
> +	}
> +
> +	pinctrl_pm_select_default_state(i2c_dev->dev);
> +
> +	st_i2c_hw_config(i2c_dev);
> +
> +	for (i = 0; (i < num) && !ret; i++)
> +		ret = st_i2c_xfer_msg(i2c_dev, &msgs[i], i == 0, i == num - 1);
> +
> +	pinctrl_pm_select_idle_state(i2c_dev->dev);
> +
> +	clk_disable_unprepare(i2c_dev->clk);
> +
> +	i2c_dev->busy = false;
> +
> +	return (ret < 0) ? ret : i;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int st_i2c_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev =
> +		container_of(dev, struct platform_device, dev);
> +	struct st_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
> +
> +	if (i2c_dev->busy)
> +		return -EBUSY;
> +
> +	pinctrl_pm_select_sleep_state(dev);
> +
> +	return 0;
> +}
> +
> +static int st_i2c_resume(struct device *dev)
> +{
> +	pinctrl_pm_select_default_state(dev);
> +	/* Go in idle state if available */
> +	pinctrl_pm_select_idle_state(dev);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(st_i2c_pm, st_i2c_suspend, st_i2c_resume);
> +#define ST_I2C_PM	(&st_i2c_pm)
> +#else
> +#define ST_I2C_PM	NULL
> +#endif
> +
> +static u32 st_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static struct i2c_algorithm st_i2c_algo = {
> +	.master_xfer = st_i2c_xfer,
> +	.functionality = st_i2c_func,
> +};
> +
> +static int st_i2c_of_get_deglitch(struct device_node *np,
> +		struct st_i2c_dev *i2c_dev)
> +{
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "i2c-min-scl-pulse-width-us",
> +			&i2c_dev->deglitch.scl_min_width_us);
> +	if ((ret == -ENODATA) || (ret == -EOVERFLOW)) {
> +		dev_err(i2c_dev->dev, "i2c-min-scl-pulse-width-us invalid\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(np, "i2c-min-sda-pulse-width-us",
> +			&i2c_dev->deglitch.sda_min_width_us);
> +	if ((ret == -ENODATA) || (ret == -EOVERFLOW)) {
> +		dev_err(i2c_dev->dev, "i2c-min-sda-pulse-width-us invalid\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int st_i2c_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct st_i2c_dev *i2c_dev;
> +	struct resource *res;
> +	u32 clk_rate;
> +	struct i2c_adapter *adap;
> +	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);
> +
> +	i2c_dev->irq = irq_of_parse_and_map(np, 0);
> +	if (!i2c_dev->irq) {
> +		dev_err(&pdev->dev, "IRQ missing or invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	i2c_dev->clk = of_clk_get_by_name(np, "ssc");
> +	if (IS_ERR(i2c_dev->clk)) {
> +		dev_err(&pdev->dev, "Unable to request clock\n");
> +		return PTR_ERR(i2c_dev->clk);
> +	}
> +
> +	i2c_dev->mode = I2C_MODE_STANDARD;
> +	ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
> +	if ((!ret) && (clk_rate == 400000))
> +		i2c_dev->mode = I2C_MODE_FAST;
> +
> +	i2c_dev->dev = &pdev->dev;
> +
> +	ret = devm_request_irq(&pdev->dev, i2c_dev->irq, st_i2c_isr, 0,
> +			pdev->name, i2c_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
> +		return ret;
> +	}
> +
> +	pinctrl_pm_select_default_state(i2c_dev->dev);
> +	/* In case idle state available, select it */
> +	pinctrl_pm_select_idle_state(i2c_dev->dev);
> +
> +	ret = st_i2c_of_get_deglitch(np, i2c_dev);
> +	if (ret)
> +		return ret;
> +
> +	adap = &i2c_dev->adap;
> +	i2c_set_adapdata(adap, i2c_dev);
> +	snprintf(adap->name, sizeof(adap->name), "ST I2C(0x%08x)", res->start);
> +	adap->owner = THIS_MODULE;
> +	adap->timeout = 2 * HZ;
> +	adap->retries = 0;
> +	adap->class = I2C_CLASS_HWMON | I2C_CLASS_DDC | I2C_CLASS_SPD;
> +	adap->algo = &st_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) {
> +		dev_err(&pdev->dev, "Failed to add adapter\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, i2c_dev);
> +
> +	dev_info(i2c_dev->dev, "%s initialized\n", adap->name);
> +
> +	return 0;
> +}
> +
> +static int st_i2c_remove(struct platform_device *pdev)
> +{
> +	struct st_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&i2c_dev->adap);
> +
> +	return 0;
> +}
> +
> +static struct of_device_id st_i2c_match[] = {
> +	{ .compatible = "st,comms-ssc-i2c", },
the rules is to put the first soc that use the ip in the compatible
as st,sti7100-scc-i2c

Best Regards,
J.

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Maxime COQUELIN]

Hi Jean-Christophe,

On 10/16/2013 05:14 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:


...
>> +
>> +static inline void st_i2c_set_bits(void __iomem *reg, u32 mask)
>> +{
>> +     writel(readl(reg) | mask, reg);
>> +}
>> +
>> +static inline void st_i2c_clr_bits(void __iomem *reg, u32 mask)
>> +{
>> +     writel(readl(reg) & ~mask, reg);
> use set_bit api and use relaxed version
Using the set_bit api here does not match with the purpose of these 
functions.
We want to be able to set/clear multiple bits, and AFAICS the set_bit 
api does not
provide this possibility.

I took example on i2c-nomadik for these functions.

>> +}
>> +
>> +/* From I2C Specifications v0.5 */
>> +static struct st_i2c_timings i2c_timings[] = {
>> +     [I2C_MODE_STANDARD] = {
>> +             .rate                   = 100000,
>> +             .rep_start_hold         = 4000,
>> +             .rep_start_setup        = 4700,
>> +             .start_hold             = 4000,
>> +             .data_setup_time        = 250,
>> +             .stop_setup_time        = 4000,
>> +             .bus_free_time          = 4700,
>> +     },
>> +     [I2C_MODE_FAST] = {
>> +             .rate                   = 400000,
>> +             .rep_start_hold         = 600,
>> +             .rep_start_setup        = 600,
>> +             .start_hold             = 600,
>> +             .data_setup_time        = 100,
>> +             .stop_setup_time        = 600,
>> +             .bus_free_time          = 1300,
>> +     },
>> +};
> so how do you plane to support other rate that 100k and 400k?
The SSC IP only supports Standard and Fast modes.
There are no plans to support faster modes.
>> +
>> +static void st_i2c_flush_rx_fifo(struct st_i2c_dev *i2c_dev)
>> +{
>> +     int count, i;
>> +
>> +     /*
>> +      * Counter only counts up to 7 but fifo size is 8...
>> +      * When fifo is full, counter is 0 and RIR bit of status register is
>> +      * set
>> +      */
>> +     if (readl(i2c_dev->base + SSC_STA) & SSC_STA_RIR)
>> +             count = SSC_RXFIFO_SIZE;
>> +     else
>> +             count = readl(i2c_dev->base + SSC_RX_FSTAT) &
>> +                     SSC_RX_FSTAT_STATUS;
>> +
>> +     for (i = 0; i < count; i++)
>> +             readl(i2c_dev->base + SSC_RBUF);
> use readsl
Since the read content is flushed, I prefer keeping it as it is instead 
of allocating
a buffer of the FIFO's size.
>
> use relaxed version as much as possible
I was not comfortable with the different possibilities (_raw_readl, 
readl_relaxed, readl...).
I found this interresting discussion: 
_http://comments.gmane.org/gmane.linux.ports.arm.kernel/117626
 From what I understood, you are right, I should be able to use 
readl_relaxed everywhere.

Maybe I should perform a readl on the interrupt mask register before 
returning from the interrupt handler,
in order to ensure that the write to the IEN register is effective 
before the IRQ for the device is re-enabled at GIC level.
Maybe this could avoid the few spurious interrupts I face sometimes, I 
will have a try.

>> +}
>> +
...
>> +
>> +static int st_i2c_wait_free_bus(struct st_i2c_dev *i2c_dev)
>> +{
>> +     u32 sta;
>> +     int i;
>> +
>> +     for (i = 0; i < 10; i++) {
>> +             sta = readl(i2c_dev->base + SSC_STA);
>> +             if (!(sta & SSC_STA_BUSY))
>> +                     return 0;
>> +
>> +             usleep_range(2000, 4000);
> can not use interrupt?
Sadly, no.
There is no interrupt linked to the busy bit.
>> +     }
>> +
...
>> +
>> +static struct of_device_id st_i2c_match[] = {
>> +     { .compatible = "st,comms-ssc-i2c", },
> the rules is to put the first soc that use the ip in the compatible
> as st,sti7100-scc-i2c
Ok. There are no plans to upstream the SH4 platforms, it will only 
remains in stlinux.com.
Maybe I can set the first ARM platform (STiH415)?
That would give st,stih415-ssc-i2c.

Thanks for the review,
Maxime

>
> Best Regards,
> J.

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[srinivas kandagatla]

On 17/10/13 08:27, Maxime COQUELIN wrote:
> ...
>>> >> +
>>> >> +static struct of_device_id st_i2c_match[] = {
>>> >> +     { .compatible = "st,comms-ssc-i2c", },
>> > the rules is to put the first soc that use the ip in the compatible
>> > as st,sti7100-scc-i2c
> Ok. There are no plans to upstream the SH4 platforms, it will only 
> remains in stlinux.com.
> Maybe I can set the first ARM platform (STiH415)?
> That would give st,stih415-ssc-i2c.
NAK, for st,stih415-ssc-i2c naming.

IMO, this makes sense when the same IP integration done on different SOC
changes slightly/very differently.

But in this case the "comms" IP remains unchanged across all the SOCs.

I would still prefer "st,comms-ssc-i2c", allowing a single device driver
to match against several SoCs. ST "comms" IP it is integrated across all
the STi series of SoCs, so we don't want to add new entry in compatible
for every new SOC.


Thanks,
srini

> 
> Thanks for the review,
> Maxime
> 
>> >

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:33 Thu 17 Oct     , srinivas kandagatla wrote:
> On 17/10/13 08:27, Maxime COQUELIN wrote:
> > ...
> >>> >> +
> >>> >> +static struct of_device_id st_i2c_match[] = {
> >>> >> +     { .compatible = "st,comms-ssc-i2c", },
> >> > the rules is to put the first soc that use the ip in the compatible
> >> > as st,sti7100-scc-i2c
> > Ok. There are no plans to upstream the SH4 platforms, it will only 
> > remains in stlinux.com.
> > Maybe I can set the first ARM platform (STiH415)?
> > That would give st,stih415-ssc-i2c.
> NAK, for st,stih415-ssc-i2c naming.
> 
> IMO, this makes sense when the same IP integration done on different SOC
> changes slightly/very differently.
> 
> But in this case the "comms" IP remains unchanged across all the SOCs.
> 
> I would still prefer "st,comms-ssc-i2c", allowing a single device driver
> to match against several SoCs. ST "comms" IP it is integrated across all
> the STi series of SoCs, so we don't want to add new entry in compatible
> for every new SOC.

you never need this you always the first SoC that's all

see other bindings on at91 as example sorry NACK

Best Regards,
J.
> 
> 
> Thanks,
> srini
> 
> > 
> > Thanks for the review,
> > Maxime
> > 
> >> >
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[srinivas kandagatla]

On 17/10/13 15:19, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 10:33 Thu 17 Oct     , srinivas kandagatla wrote:
>> On 17/10/13 08:27, Maxime COQUELIN wrote:
>>> ...
>>>>>>> +
>>>>>>> +static struct of_device_id st_i2c_match[] = {
>>>>>>> +     { .compatible = "st,comms-ssc-i2c", },
>>>>> the rules is to put the first soc that use the ip in the compatible
>>>>> as st,sti7100-scc-i2c
>>> Ok. There are no plans to upstream the SH4 platforms, it will only 
>>> remains in stlinux.com.
>>> Maybe I can set the first ARM platform (STiH415)?
>>> That would give st,stih415-ssc-i2c.
>> NAK, for st,stih415-ssc-i2c naming.
>>
>> IMO, this makes sense when the same IP integration done on different SOC
>> changes slightly/very differently.
>>
>> But in this case the "comms" IP remains unchanged across all the SOCs.
>>
>> I would still prefer "st,comms-ssc-i2c", allowing a single device driver
>> to match against several SoCs. ST "comms" IP it is integrated across all
>> the STi series of SoCs, so we don't want to add new entry in compatible
>> for every new SOC.
> 
> you never need this you always the first SoC that's all

Sorry to ask this but, Where is this requirement coming from?
I have not spotted any thing as such in ePAPR specs.


All the spec says is.
===
The compatible property value consists of one or more strings that
define the specific programming model for the device. This list of
strings should be used by a client program for device driver selection.
The property value consists of a concatenated list of null terminated
strings, *from most specific to most general.* They allow a device to
express its compatibility with a family of similar devices, potentially
allowing a single device driver to match against several devices.
The recommended format is “manufacturer,model”, where manufacturer is a
string describing the name of the manufacturer (such as an OUI), and
model specifies the model number.

Example:
compatible = “fsl,mpc8641-uart”, “ns16550";
In this example, an operating system would first try to locate a device
driver that supported fsl,mpc8641-uart. If a driver was not found, it
would then try to locate a driver that supported the more general
ns16550 device type.
===

The more general compatible string in this case is "st,comms-ssc-i2c",
rather than the first soc name.
How can a first SOC name be more general?

As this driver is not very specific to StiH415, it is generic driver for
ST comms-ssc-i2c block.

Thanks,
srini




> 
> see other bindings on at91 as example sorry NACK
> 
> Best Regards,
> J.
>>
>>
>> Thanks,
>> srini
>>
>>>
>>> Thanks for the review,
>>> Maxime
>>>
>>>>>
>>
> 
> 

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Lucas Stach]

Am Donnerstag, den 17.10.2013, 15:30 +0100 schrieb srinivas kandagatla:
[...]
> Sorry to ask this but, Where is this requirement coming from?
> I have not spotted any thing as such in ePAPR specs.
> 
> 
> All the spec says is.
> ===
> The compatible property value consists of one or more strings that
> define the specific programming model for the device. This list of
> strings should be used by a client program for device driver selection.
> The property value consists of a concatenated list of null terminated
> strings, *from most specific to most general.* They allow a device to
> express its compatibility with a family of similar devices, potentially
> allowing a single device driver to match against several devices.
> The recommended format is “manufacturer,model”, where manufacturer is a
> string describing the name of the manufacturer (such as an OUI), and
> model specifies the model number.
> 
> Example:
> compatible = “fsl,mpc8641-uart”, “ns16550";
> In this example, an operating system would first try to locate a device
> driver that supported fsl,mpc8641-uart. If a driver was not found, it
> would then try to locate a driver that supported the more general
> ns16550 device type.
> ===
> 
> The more general compatible string in this case is "st,comms-ssc-i2c",
> rather than the first soc name.
> How can a first SOC name be more general?
> 
> As this driver is not very specific to StiH415, it is generic driver for
> ST comms-ssc-i2c block.
> 

You just can't know if someone in the future decides to subtly change
the register layout or make some other incompatible change to the
comms-ssc-i2c block.

So as a general rule of thumb you take the first SoC where a particular
IP block appeared as the compatible string, so you can just pick the
name of the SoC where the incompatible change was made as the next
string and not need to invent generic and unspecific comms-ssc-i2c-v2
compatibles.

Regards,
Lucas
-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[srinivas kandagatla]

On 17/10/13 15:49, Lucas Stach wrote:
> Am Donnerstag, den 17.10.2013, 15:30 +0100 schrieb srinivas kandagatla:
> [...]
>> Sorry to ask this but, Where is this requirement coming from?
>> I have not spotted any thing as such in ePAPR specs.
>>
>>
>> All the spec says is.
>> ===
>> The compatible property value consists of one or more strings that
>> define the specific programming model for the device. This list of
>> strings should be used by a client program for device driver selection.
>> The property value consists of a concatenated list of null terminated
>> strings, *from most specific to most general.* They allow a device to
>> express its compatibility with a family of similar devices, potentially
>> allowing a single device driver to match against several devices.
>> The recommended format is “manufacturer,model”, where manufacturer is a
>> string describing the name of the manufacturer (such as an OUI), and
>> model specifies the model number.
>>
>> Example:
>> compatible = “fsl,mpc8641-uart”, “ns16550";
>> In this example, an operating system would first try to locate a device
>> driver that supported fsl,mpc8641-uart. If a driver was not found, it
>> would then try to locate a driver that supported the more general
>> ns16550 device type.
>> ===
>>
>> The more general compatible string in this case is "st,comms-ssc-i2c",
>> rather than the first soc name.
>> How can a first SOC name be more general?
>>
>> As this driver is not very specific to StiH415, it is generic driver for
>> ST comms-ssc-i2c block.
>>
> 
> You just can't know if someone in the future decides to subtly change
> the register layout or make some other incompatible change to the
> comms-ssc-i2c block.
> 
This is not the case for comms-ssc-i2c block, This IP is kind of reused
from past 10+ years(I think!!). Am not predicting the future here, but I
am making a informed guess from past experience that this IP would not
change in future.

Am still not happy with the idea of using first SoC for the compatible
for following reasons:

1> Generic IPs can be integrated into various vendor SoCs. For example
synopsis IP can be integrated by ST parts and other non-ST parts. What
would be the first SoC name in this case?

2> Looking at example like "arm,pl310-cache", "arm,l220-cache"... or any
other generic ips, why are these IPs not encoding the first SoC name in
there compatible string? I think the answer is generic IP.

3> IMHO, the idea of first SoC might solve the problem you described,
but why would some one know about the first SoC when this was available.
In this case this IP was available may be 10+ years back on an ST40
platform. Having such old SoC names in compatible strings in the device
trees for a modern chip looks bit confusing.

4> ST generic drivers which are in kernel still use st,<IP> name, so I
would like this consistency across all the st drivers (at least the ones
which are going to be used by mach-sti platforms).

5> ePAPR spec clearly says that compatible string should contain "most
specific to most general" names. In this case using more generic name
makes more sense than having a specific name because its generic IP.
Allowing a single device driver to match against several devices.

6> IMHO, the compatible string should be "vendor,<IP-name>-<IP-version>"
rather than first SoC.


Thanks,
srini

> So as a general rule of thumb you take the first SoC where a particular
> IP block appeared as the compatible string, so you can just pick the
> name of the SoC where the incompatible change was made as the next
> string and not need to invent generic and unspecific comms-ssc-i2c-v2
> compatibles.
> 
> Regards,
> Lucas
> 

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Maxime Coquelin]

On 10/18/2013 10:22 AM, Srinivas KANDAGATLA wrote:
> On 17/10/13 15:49, Lucas Stach wrote:
>> Am Donnerstag, den 17.10.2013, 15:30 +0100 schrieb srinivas kandagatla:
>> [...]
>>> Sorry to ask this but, Where is this requirement coming from?
>>> I have not spotted any thing as such in ePAPR specs.
>>>
>>>
>>> All the spec says is.
>>> ===
>>> The compatible property value consists of one or more strings that
>>> define the specific programming model for the device. This list of
>>> strings should be used by a client program for device driver selection.
>>> The property value consists of a concatenated list of null terminated
>>> strings, *from most specific to most general.* They allow a device to
>>> express its compatibility with a family of similar devices, potentially
>>> allowing a single device driver to match against several devices.
>>> The recommended format is “manufacturer,model”, where manufacturer is a
>>> string describing the name of the manufacturer (such as an OUI), and
>>> model specifies the model number.
>>>
>>> Example:
>>> compatible = “fsl,mpc8641-uart”, “ns16550";
>>> In this example, an operating system would first try to locate a device
>>> driver that supported fsl,mpc8641-uart. If a driver was not found, it
>>> would then try to locate a driver that supported the more general
>>> ns16550 device type.
>>> ===
>>>
>>> The more general compatible string in this case is "st,comms-ssc-i2c",
>>> rather than the first soc name.
>>> How can a first SOC name be more general?
>>>
>>> As this driver is not very specific to StiH415, it is generic driver for
>>> ST comms-ssc-i2c block.
>>>
>> You just can't know if someone in the future decides to subtly change
>> the register layout or make some other incompatible change to the
>> comms-ssc-i2c block.
>>
> This is not the case for comms-ssc-i2c block, This IP is kind of reused
> from past 10+ years(I think!!). Am not predicting the future here, but I
> am making a informed guess from past experience that this IP would not
> change in future.
Having discussed with HW design team in charge of this IP,
I also bet that this IP won't change in the future.

>
> Am still not happy with the idea of using first SoC for the compatible
> for following reasons:
>
> 1> Generic IPs can be integrated into various vendor SoCs. For example
> synopsis IP can be integrated by ST parts and other non-ST parts. What
> would be the first SoC name in this case?
>
> 2> Looking at example like "arm,pl310-cache", "arm,l220-cache"... or any
> other generic ips, why are these IPs not encoding the first SoC name in
> there compatible string? I think the answer is generic IP.
>
> 3> IMHO, the idea of first SoC might solve the problem you described,
> but why would some one know about the first SoC when this was available.
> In this case this IP was available may be 10+ years back on an ST40
> platform. Having such old SoC names in compatible strings in the device
> trees for a modern chip looks bit confusing.
>
> 4> ST generic drivers which are in kernel still use st,<IP> name, so I
> would like this consistency across all the st drivers (at least the ones
> which are going to be used by mach-sti platforms).
>
> 5> ePAPR spec clearly says that compatible string should contain "most
> specific to most general" names. In this case using more generic name
> makes more sense than having a specific name because its generic IP.
> Allowing a single device driver to match against several devices.
>
> 6> IMHO, the compatible string should be "vendor,<IP-name>-<IP-version>"
> rather than first SoC.
I agree.
In this case, we add support to revision 4 of SSC IP.

Is "st,comms-ssc-v4" okay?


Thanks,
Maxime

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

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[srinivas kandagatla]

On 28/10/13 15:02, Maxime Coquelin wrote:
>>
>> 6> IMHO, the compatible string should be "vendor,<IP-name>-<IP-version>"
>> rather than first SoC.
> I agree.
> In this case, we add support to revision 4 of SSC IP.
Its not the revision its name of the new IP which is SSC4. However  this
driver is also compatible with old SSC IP, so It would be nice to have
something like two compatible strings "st,comms-ssc-i2c" and
"st,comms-ssc4-i2c"

Thanks,
srini
> 
> Is "st,comms-ssc-v4" okay?

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Lee Jones]

On Thu, 17 Oct 2013, Jean-Christophe PLAGNIOL-VILLARD wrote:

> On 10:33 Thu 17 Oct     , srinivas kandagatla wrote:
> > On 17/10/13 08:27, Maxime COQUELIN wrote:
> > > ...
> > >>> >> +
> > >>> >> +static struct of_device_id st_i2c_match[] = {
> > >>> >> +     { .compatible = "st,comms-ssc-i2c", },
> > >> > the rules is to put the first soc that use the ip in the compatible
> > >> > as st,sti7100-scc-i2c
> > > Ok. There are no plans to upstream the SH4 platforms, it will only 
> > > remains in stlinux.com.
> > > Maybe I can set the first ARM platform (STiH415)?
> > > That would give st,stih415-ssc-i2c.
> > NAK, for st,stih415-ssc-i2c naming.
> > 
> > IMO, this makes sense when the same IP integration done on different SOC
> > changes slightly/very differently.
> > 
> > But in this case the "comms" IP remains unchanged across all the SOCs.
> > 
> > I would still prefer "st,comms-ssc-i2c", allowing a single device driver
> > to match against several SoCs. ST "comms" IP it is integrated across all
> > the STi series of SoCs, so we don't want to add new entry in compatible
> > for every new SOC.
> 
> you never need this you always the first SoC that's all
> 
> see other bindings on at91 as example sorry NACK

I'm guessing that using the first SoC is an I2C'isum.

Guys, if you don't want to be too specific, just make it as generic as
possible whilest still using the SoC as a POR: st,stih41x-ssc-i2c will
do for now, as it covers all current bases.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:53 Thu 17 Oct     , Lee Jones wrote:
> On Thu, 17 Oct 2013, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> > On 10:33 Thu 17 Oct     , srinivas kandagatla wrote:
> > > On 17/10/13 08:27, Maxime COQUELIN wrote:
> > > > ...
> > > >>> >> +
> > > >>> >> +static struct of_device_id st_i2c_match[] = {
> > > >>> >> +     { .compatible = "st,comms-ssc-i2c", },
> > > >> > the rules is to put the first soc that use the ip in the compatible
> > > >> > as st,sti7100-scc-i2c
> > > > Ok. There are no plans to upstream the SH4 platforms, it will only 
> > > > remains in stlinux.com.
> > > > Maybe I can set the first ARM platform (STiH415)?
> > > > That would give st,stih415-ssc-i2c.
> > > NAK, for st,stih415-ssc-i2c naming.
> > > 
> > > IMO, this makes sense when the same IP integration done on different SOC
> > > changes slightly/very differently.
> > > 
> > > But in this case the "comms" IP remains unchanged across all the SOCs.
> > > 
> > > I would still prefer "st,comms-ssc-i2c", allowing a single device driver
> > > to match against several SoCs. ST "comms" IP it is integrated across all
> > > the STi series of SoCs, so we don't want to add new entry in compatible
> > > for every new SOC.
> > 
> > you never need this you always the first SoC that's all
> > 
> > see other bindings on at91 as example sorry NACK
> 
> I'm guessing that using the first SoC is an I2C'isum.
> 
> Guys, if you don't want to be too specific, just make it as generic as
> possible whilest still using the SoC as a POR: st,stih41x-ssc-i2c will
> do for now, as it covers all current bases.

except this is wrong the IP is used on much older SoC than the recent ARM ones

and the DT so please respect the rule first SoC used on

Best Regards,
J.
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

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

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:27 Thu 17 Oct     , Maxime COQUELIN wrote:
> Hi Jean-Christophe,
> 
> On 10/16/2013 05:14 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> 
> ...
> >> +
> >> +static inline void st_i2c_set_bits(void __iomem *reg, u32 mask)
> >> +{
> >> +     writel(readl(reg) | mask, reg);
> >> +}
> >> +
> >> +static inline void st_i2c_clr_bits(void __iomem *reg, u32 mask)
> >> +{
> >> +     writel(readl(reg) & ~mask, reg);
> > use set_bit api and use relaxed version
> Using the set_bit api here does not match with the purpose of these 
> functions.
> We want to be able to set/clear multiple bits, and AFAICS the set_bit 
> api does not
> provide this possibility.
> 
> I took example on i2c-nomadik for these functions.
> 

so factorize the code not copy and paste
> >> +}
> >> +
> >> +/* From I2C Specifications v0.5 */
> >> +static struct st_i2c_timings i2c_timings[] = {
> >> +     [I2C_MODE_STANDARD] = {
> >> +             .rate                   = 100000,
> >> +             .rep_start_hold         = 4000,
> >> +             .rep_start_setup        = 4700,
> >> +             .start_hold             = 4000,
> >> +             .data_setup_time        = 250,
> >> +             .stop_setup_time        = 4000,
> >> +             .bus_free_time          = 4700,
> >> +     },
> >> +     [I2C_MODE_FAST] = {
> >> +             .rate                   = 400000,
> >> +             .rep_start_hold         = 600,
> >> +             .rep_start_setup        = 600,
> >> +             .start_hold             = 600,
> >> +             .data_setup_time        = 100,
> >> +             .stop_setup_time        = 600,
> >> +             .bus_free_time          = 1300,
> >> +     },
> >> +};
> > so how do you plane to support other rate that 100k and 400k?
> The SSC IP only supports Standard and Fast modes.
> There are no plans to support faster modes.
> >> +
> >> +static void st_i2c_flush_rx_fifo(struct st_i2c_dev *i2c_dev)
> >> +{
> >> +     int count, i;
> >> +
> >> +     /*
> >> +      * Counter only counts up to 7 but fifo size is 8...
> >> +      * When fifo is full, counter is 0 and RIR bit of status register is
> >> +      * set
> >> +      */
> >> +     if (readl(i2c_dev->base + SSC_STA) & SSC_STA_RIR)
> >> +             count = SSC_RXFIFO_SIZE;
> >> +     else
> >> +             count = readl(i2c_dev->base + SSC_RX_FSTAT) &
> >> +                     SSC_RX_FSTAT_STATUS;
> >> +
> >> +     for (i = 0; i < count; i++)
> >> +             readl(i2c_dev->base + SSC_RBUF);
> > use readsl
> Since the read content is flushed, I prefer keeping it as it is instead 
> of allocating
> a buffer of the FIFO's size.

keep point is to speedup the bus
> >
> > use relaxed version as much as possible
> I was not comfortable with the different possibilities (_raw_readl, 
> readl_relaxed, readl...).
> I found this interresting discussion: 
> _http://comments.gmane.org/gmane.linux.ports.arm.kernel/117626
>  From what I understood, you are right, I should be able to use 
> readl_relaxed everywhere.
> 
> Maybe I should perform a readl on the interrupt mask register before 
> returning from the interrupt handler,
> in order to ensure that the write to the IEN register is effective 
> before the IRQ for the device is re-enabled at GIC level.
> Maybe this could avoid the few spurious interrupts I face sometimes, I 
> will have a try.
ok
> 
> >> +}
> >> +
> ...
> >> +
> >> +static int st_i2c_wait_free_bus(struct st_i2c_dev *i2c_dev)
> >> +{
> >> +     u32 sta;
> >> +     int i;
> >> +
> >> +     for (i = 0; i < 10; i++) {
> >> +             sta = readl(i2c_dev->base + SSC_STA);
> >> +             if (!(sta & SSC_STA_BUSY))
> >> +                     return 0;
> >> +
> >> +             usleep_range(2000, 4000);
> > can not use interrupt?
> Sadly, no.
> There is no interrupt linked to the busy bit.
> >> +     }
> >> +
> ...
> >> +
> >> +static struct of_device_id st_i2c_match[] = {
> >> +     { .compatible = "st,comms-ssc-i2c", },
> > the rules is to put the first soc that use the ip in the compatible
> > as st,sti7100-scc-i2c
> Ok. There are no plans to upstream the SH4 platforms, it will only 
> remains in stlinux.com.
> Maybe I can set the first ARM platform (STiH415)?
> That would give st,stih415-ssc-i2c.

no you need to put the first soc even not mainline dt is not relaated to linux
mainline of not we describe hw

IIRC it was even present before sh4, on st200 IIRC need to check my old
datasheet

Best Regards,
J.
> 
> Thanks for the review,
> Maxime
> 
> >
> > Best Regards,
> > J.

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Maxime COQUELIN]

On 10/17/2013 04:16 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 09:27 Thu 17 Oct     , Maxime COQUELIN wrote:
>> Hi Jean-Christophe,
>>
>> On 10/16/2013 05:14 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>
>>
>> ...
>>>> +
>>>> +static inline void st_i2c_set_bits(void __iomem *reg, u32 mask)
>>>> +{
>>>> +     writel(readl(reg) | mask, reg);
>>>> +}
>>>> +
>>>> +static inline void st_i2c_clr_bits(void __iomem *reg, u32 mask)
>>>> +{
>>>> +     writel(readl(reg) & ~mask, reg);
>>> use set_bit api and use relaxed version
>> Using the set_bit api here does not match with the purpose of these
>> functions.
>> We want to be able to set/clear multiple bits, and AFAICS the set_bit
>> api does not
>> provide this possibility.
>>
>> I took example on i2c-nomadik for these functions.
>>
> so factorize the code not copy and paste
I won't create a new API for this.
If this is blocking for you, then I will just remove this functions.

>>>> +}
>>>> +
>>>> +/* From I2C Specifications v0.5 */
>>>> +static struct st_i2c_timings i2c_timings[] = {
>>>> use readsl
>> Since the read content is flushed, I prefer keeping it as it is instead
>> of allocating
>> a buffer of the FIFO's size.
> keep point is to speedup the bus
I meant I will use readl_relaxed, not readl.

>>> use relaxed version as much as possible
>> I was not comfortable with the different possibilities (_raw_readl,
>> readl_relaxed, readl...).
>> I found this interresting discussion:
>> _http://comments.gmane.org/gmane.linux.ports.arm.kernel/117626
>>   From what I understood, you are right, I should be able to use
>> readl_relaxed everywhere.
>>
>> Maybe I should perform a readl on the interrupt mask register before
>> returning from the interrupt handler,
>> in order to ensure that the write to the IEN register is effective
>> before the IRQ for the device is re-enabled at GIC level.
>> Maybe this could avoid the few spurious interrupts I face sometimes, I
>> will have a try.
> ok
I failed to reproduce the spurious interrupt without the patch, so I 
can't say
whether performing a readl at this stage helps.
>>>> +}

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Kumar Gala]

On Oct 14, 2013, at 7:46 AM, Maxime COQUELIN wrote:

> This patch adds support to SSC (Synchronous Serial Controller)
> I2C driver. This IP also supports SPI protocol, but this is not
> the aim of this driver.
> 
> This IP is embedded in all ST SoCs for Set-top box platorms, and
> supports I2C Standard and Fast modes.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
> ---
> Documentation/devicetree/bindings/i2c/i2c-st.txt |   38 +
> drivers/i2c/busses/Kconfig                       |   10 +
> drivers/i2c/busses/Makefile                      |    1 +
> drivers/i2c/busses/i2c-st.c                      |  867 ++++++++++++++++++++++
> 4 files changed, 916 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-st.txt
> create mode 100644 drivers/i2c/busses/i2c-st.c
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt
> new file mode 100644
> index 0000000..8b2fd0b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-st.txt
> @@ -0,0 +1,38 @@
> +ST SSC binding, for I2C mode operation
> +
> +Required properties :
> +- compatible : Must be "st,comms-i2c"
> +- reg : Offset and length of the register set for the device
> +- interrupts : the interrupt specifier
> +- clock-names: Must contain "ssc".
> +- clocks: Must contain an entry for each name in clock-names. See the common
> +  clock bindings.
> +- A pinctrl state named "default" must be defined, using the bindings in
> +  ../pinctrl/pinctrl-binding.txt
> +
> +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.
> +- i2c-min-scl-pulse-width-us : The minimum valid SCL pulse width that is
> +  allowed through the deglitch circuit. In units of us.
> +- i2c-min-sda-pulse-width-us : The minimum valid SDA pulse width that is
> +  allowed through the deglitch circuit. In units of us.

i2c-min... should be vendor prefixed, st,i2c-min...

> +- A pinctrl state named "sleep" could be defined, using the bindings in
> +  ../pinctrl/pinctrl-binding.txt

I don't see any reference to "sleep" in pinctrl-binding.txt

> +
> +
> +Example :
> +
> +i2c0: i2c@fed40000 {
> +	compatible	= "st,comms-ssc-i2c";
> +	reg		= <0xfed40000 0x110>;
> +	interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
> +	clocks		= <&CLK_S_ICN_REG_0>;
> +	clock-names	= "ssc";
> +	clock-frequency = <400000>;
> +	pinctrl-names	= "default";
> +	pinctrl-0	= <&pinctrl_i2c0_default>;
> +	i2c-min-scl-pulse-width-us = <0>;
> +	i2c-min-sda-pulse-width-us = <5>;
> +};

- k

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

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Maxime Coquelin]

On 10/28/2013 08:25 PM, Kumar Gala wrote:
> On Oct 14, 2013, at 7:46 AM, Maxime COQUELIN wrote:
>
>> This patch adds support to SSC (Synchronous Serial Controller)
>> I2C driver. This IP also supports SPI protocol, but this is not
>> the aim of this driver.
>>
>> This IP is embedded in all ST SoCs for Set-top box platorms, and
>> supports I2C Standard and Fast modes.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
>> ---
>> Documentation/devicetree/bindings/i2c/i2c-st.txt |   38 +
>> drivers/i2c/busses/Kconfig                       |   10 +
>> drivers/i2c/busses/Makefile                      |    1 +
>> drivers/i2c/busses/i2c-st.c                      |  867 ++++++++++++++++++++++
>> 4 files changed, 916 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-st.txt
>> create mode 100644 drivers/i2c/busses/i2c-st.c
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt
>> new file mode 100644
>> index 0000000..8b2fd0b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-st.txt
>> @@ -0,0 +1,38 @@
>> +ST SSC binding, for I2C mode operation
>> +
>> +Required properties :
>> +- compatible : Must be "st,comms-i2c"
>> +- reg : Offset and length of the register set for the device
>> +- interrupts : the interrupt specifier
>> +- clock-names: Must contain "ssc".
>> +- clocks: Must contain an entry for each name in clock-names. See the common
>> +  clock bindings.
>> +- A pinctrl state named "default" must be defined, using the bindings in
>> +  ../pinctrl/pinctrl-binding.txt
>> +
>> +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.
>> +- i2c-min-scl-pulse-width-us : The minimum valid SCL pulse width that is
>> +  allowed through the deglitch circuit. In units of us.
>> +- i2c-min-sda-pulse-width-us : The minimum valid SDA pulse width that is
>> +  allowed through the deglitch circuit. In units of us.
> i2c-min... should be vendor prefixed, st,i2c-min...
This was already discussed in earlier revisions of the patches.
Initially this was prefixed with "st,", but Wolfram asked to put these 
properties as generic.
As explained in the cover letter, there are no I2C DT binding 
documentation for now, but it is in Wolfram's ToDo list.
As soon as Wolfram has created the documentation, I will create a patch 
to document

i2c-min-scl-pulse-width-us and i2c-min-sda-pulse-width-us there.


>
>> +- A pinctrl state named "sleep" could be defined, using the bindings in
>> +  ../pinctrl/pinctrl-binding.txt
> I don't see any reference to "sleep" in pinctrl-binding.txt
Right, I will correct that.

Thanks for the review,
Maxime
>
> - k
>

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Kumar Gala]

On Oct 29, 2013, at 8:19 AM, Maxime Coquelin wrote:

> 
> On 10/28/2013 08:25 PM, Kumar Gala wrote:
>> On Oct 14, 2013, at 7:46 AM, Maxime COQUELIN wrote:
>> 
>>> This patch adds support to SSC (Synchronous Serial Controller)
>>> I2C driver. This IP also supports SPI protocol, but this is not
>>> the aim of this driver.
>>> 
>>> This IP is embedded in all ST SoCs for Set-top box platorms, and
>>> supports I2C Standard and Fast modes.
>>> 
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin-qxv4g6HH51o@public.gmane.org>
>>> ---
>>> Documentation/devicetree/bindings/i2c/i2c-st.txt |   38 +
>>> drivers/i2c/busses/Kconfig                       |   10 +
>>> drivers/i2c/busses/Makefile                      |    1 +
>>> drivers/i2c/busses/i2c-st.c                      |  867 ++++++++++++++++++++++
>>> 4 files changed, 916 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-st.txt
>>> create mode 100644 drivers/i2c/busses/i2c-st.c
>>> 
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt
>>> new file mode 100644
>>> index 0000000..8b2fd0b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-st.txt
>>> @@ -0,0 +1,38 @@
>>> +ST SSC binding, for I2C mode operation
>>> +
>>> +Required properties :
>>> +- compatible : Must be "st,comms-i2c"
>>> +- reg : Offset and length of the register set for the device
>>> +- interrupts : the interrupt specifier
>>> +- clock-names: Must contain "ssc".
>>> +- clocks: Must contain an entry for each name in clock-names. See the common
>>> +  clock bindings.
>>> +- A pinctrl state named "default" must be defined, using the bindings in
>>> +  ../pinctrl/pinctrl-binding.txt
>>> +
>>> +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.
>>> +- i2c-min-scl-pulse-width-us : The minimum valid SCL pulse width that is
>>> +  allowed through the deglitch circuit. In units of us.
>>> +- i2c-min-sda-pulse-width-us : The minimum valid SDA pulse width that is
>>> +  allowed through the deglitch circuit. In units of us.
>> i2c-min... should be vendor prefixed, st,i2c-min...
> This was already discussed in earlier revisions of the patches.
> Initially this was prefixed with "st,", but Wolfram asked to put these properties as generic.
> As explained in the cover letter, there are no I2C DT binding documentation for now, but it is in Wolfram's ToDo list.
> As soon as Wolfram has created the documentation, I will create a patch to document
> 
> i2c-min-scl-pulse-width-us and i2c-min-sda-pulse-width-us there.

Missed that, would be good to maybe comment about that in the commit message.. Or is there some reason to not just put them into a i2c/i2c-bus.txt right now?

>>> 
>>> +- A pinctrl state named "sleep" could be defined, using the bindings in
>>> +  ../pinctrl/pinctrl-binding.txt
>> I don't see any reference to "sleep" in pinctrl-binding.txt
> Right, I will correct that.
> 
> Thanks for the review,
> Maxime

- k

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

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Wolfram Sang]

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

Hi,

On Mon, Oct 14, 2013 at 02:46:49PM +0200, Maxime COQUELIN wrote:
> This patch adds support to SSC (Synchronous Serial Controller)
> I2C driver. This IP also supports SPI protocol, but this is not
> the aim of this driver.
> 
> This IP is embedded in all ST SoCs for Set-top box platorms, and
> supports I2C Standard and Fast modes.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-st.txt |   38 +
>  drivers/i2c/busses/Kconfig                       |   10 +
>  drivers/i2c/busses/Makefile                      |    1 +
>  drivers/i2c/busses/i2c-st.c                      |  867 ++++++++++++++++++++++
>  4 files changed, 916 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-st.txt
>  create mode 100644 drivers/i2c/busses/i2c-st.c
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt
> new file mode 100644
> index 0000000..8b2fd0b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-st.txt
> @@ -0,0 +1,38 @@
> +ST SSC binding, for I2C mode operation
> +
> +Required properties :
> +- compatible : Must be "st,comms-i2c"

I personally don't care about naming here. Has there been a solution
now?

> +- reg : Offset and length of the register set for the device
> +- interrupts : the interrupt specifier
> +- clock-names: Must contain "ssc".
> +- clocks: Must contain an entry for each name in clock-names. See the common
> +  clock bindings.
> +- A pinctrl state named "default" must be defined, using the bindings in
> +  ../pinctrl/pinctrl-binding.txt
> +
> +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.
> +- i2c-min-scl-pulse-width-us : The minimum valid SCL pulse width that is
> +  allowed through the deglitch circuit. In units of us.
> +- i2c-min-sda-pulse-width-us : The minimum valid SDA pulse width that is
> +  allowed through the deglitch circuit. In units of us.

Okay, so we had lots of dt bindings discussions at kernel summit. Since
we don't have unstable bindings yet, I have been convinced that it is
okay two have one or two vendor specific bindings before introducing a
generic one. That will create a little bit of cruft, but increases
chances that the generic bindings are proper. So, really sorry for going
back and forth, but it was important for the process. Vendor binding is
it now. Period.

...

> +/**
> + * struct st_i2c_deglitch - Anti-glitch filter configuration
> + * @scl_min_width_us: SCL line minimum pulse width in ns
> + * @sda_min_width_us: SDA line minimum pulse width in ns
> + */
> +struct st_i2c_deglitch {
> +	u32	scl_min_width_us;
> +	u32	sda_min_width_us;
> +};

Minor: Why a seperate struct?

> +/**
> + * st_i2c_handle_write() - Handle FIFO enmpty interrupt in case of read
> + * @i2c_dev: Controller's private data
> + */
> +static void st_i2c_handle_read(struct st_i2c_dev *i2c_dev)
> +{
> +	struct st_i2c_client *c = &i2c_dev->client;
> +	u32 ien;
> +
> +	/* Trash the address read back */
> +	if (!c->xfered) {
> +		readl(i2c_dev->base + SSC_RBUF);
> +		st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_TXENB);
> +	} else
> +		st_i2c_read_rx_fifo(i2c_dev);

Braces around else branch.

> +
> +	if (!c->count) {
> +		/* End of xfer, send stop or repstart */
> +		st_i2c_terminate_xfer(i2c_dev);
> +	} else if (c->count == 1) {
> +		/* Penultimate byte to xfer, disable ACK gen. */
> +		st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_ACKG);
> +
> +		/* Last received byte is to be handled by NACK interrupt */
> +		ien = SSC_IEN_NACKEN | SSC_IEN_ARBLEN;
> +		writel(ien, i2c_dev->base + SSC_IEN);
> +
> +		st_i2c_rd_fill_tx_fifo(i2c_dev, c->count);
> +	} else
> +		st_i2c_rd_fill_tx_fifo(i2c_dev, c->count - 1);

Braces around else branch.

> +}
> +static int st_i2c_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct st_i2c_dev *i2c_dev;
> +	struct resource *res;
> +	u32 clk_rate;
> +	struct i2c_adapter *adap;
> +	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);
> +
> +	i2c_dev->irq = irq_of_parse_and_map(np, 0);
> +	if (!i2c_dev->irq) {
> +		dev_err(&pdev->dev, "IRQ missing or invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	i2c_dev->clk = of_clk_get_by_name(np, "ssc");
> +	if (IS_ERR(i2c_dev->clk)) {
> +		dev_err(&pdev->dev, "Unable to request clock\n");
> +		return PTR_ERR(i2c_dev->clk);
> +	}
> +
> +	i2c_dev->mode = I2C_MODE_STANDARD;
> +	ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
> +	if ((!ret) && (clk_rate == 400000))
> +		i2c_dev->mode = I2C_MODE_FAST;
> +
> +	i2c_dev->dev = &pdev->dev;
> +
> +	ret = devm_request_irq(&pdev->dev, i2c_dev->irq, st_i2c_isr, 0,
> +			pdev->name, i2c_dev);

Suggestion: Maybe threaded irq since you do fifo handling in the isr?

> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
> +		return ret;
> +	}
> +
> +	pinctrl_pm_select_default_state(i2c_dev->dev);
> +	/* In case idle state available, select it */
> +	pinctrl_pm_select_idle_state(i2c_dev->dev);
> +
> +	ret = st_i2c_of_get_deglitch(np, i2c_dev);
> +	if (ret)
> +		return ret;
> +
> +	adap = &i2c_dev->adap;
> +	i2c_set_adapdata(adap, i2c_dev);
> +	snprintf(adap->name, sizeof(adap->name), "ST I2C(0x%08x)", res->start);

resource_size_t can also be 64 bit.

> +	adap->owner = THIS_MODULE;
> +	adap->timeout = 2 * HZ;
> +	adap->retries = 0;
> +	adap->class = I2C_CLASS_HWMON | I2C_CLASS_DDC | I2C_CLASS_SPD;

This question is still open:
Why do you need class based instantiation. It will most likely cost
boot-time and you have devicetree means for doing instantiation.

> +	adap->algo = &st_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) {
> +		dev_err(&pdev->dev, "Failed to add adapter\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, i2c_dev);
> +
> +	dev_info(i2c_dev->dev, "%s initialized\n", adap->name);
> +
> +	return 0;
> +}
> +

Rest looks good! We are mostly ready to go.

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

[Maxime Coquelin]

Hi Wolfram,

On 11/01/2013 12:16 PM, Wolfram Sang wrote:
> Hi,
>
...
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-st.txt b/Documentation/devicetree/bindings/i2c/i2c-st.txt
>> new file mode 100644
>> index 0000000..8b2fd0b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-st.txt
>> @@ -0,0 +1,38 @@
>> +ST SSC binding, for I2C mode operation
>> +
>> +Required properties :
>> +- compatible : Must be "st,comms-i2c"
>
> I personally don't care about naming here. Has there been a solution
> now?

Srini proposes to add 2 compatible strings, as the IP is compatible with 
two SSC IPs:
"st,comms-ssc-i2c"
"st,comms-ssc4-i2c"

>> +
>> +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.
>> +- i2c-min-scl-pulse-width-us : The minimum valid SCL pulse width that is
>> +  allowed through the deglitch circuit. In units of us.
>> +- i2c-min-sda-pulse-width-us : The minimum valid SDA pulse width that is
>> +  allowed through the deglitch circuit. In units of us.
>
> Okay, so we had lots of dt bindings discussions at kernel summit. Since
> we don't have unstable bindings yet, I have been convinced that it is
> okay two have one or two vendor specific bindings before introducing a
> generic one. That will create a little bit of cruft, but increases
> chances that the generic bindings are proper. So, really sorry for going
> back and forth, but it was important for the process. Vendor binding is
> it now. Period.
>
> ...
No problem wolfram! Thanks for clarifying this thing.

>
>> +/**
>> + * struct st_i2c_deglitch - Anti-glitch filter configuration
>> + * @scl_min_width_us: SCL line minimum pulse width in ns
>> + * @sda_min_width_us: SDA line minimum pulse width in ns
>> + */
>> +struct st_i2c_deglitch {
>> +	u32	scl_min_width_us;
>> +	u32	sda_min_width_us;
>> +};
>
> Minor: Why a seperate struct?
This not needed, I will move its fields into st_i2c_dev struct.

>
>> +/**
>> + * st_i2c_handle_write() - Handle FIFO enmpty interrupt in case of read
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void st_i2c_handle_read(struct st_i2c_dev *i2c_dev)
>> +{
>> +	struct st_i2c_client *c = &i2c_dev->client;
>> +	u32 ien;
>> +
>> +	/* Trash the address read back */
>> +	if (!c->xfered) {
>> +		readl(i2c_dev->base + SSC_RBUF);
>> +		st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_TXENB);
>> +	} else
>> +		st_i2c_read_rx_fifo(i2c_dev);
>
> Braces around else branch.
Okay
>
>> +
>> +	if (!c->count) {
>> +		/* End of xfer, send stop or repstart */
>> +		st_i2c_terminate_xfer(i2c_dev);
>> +	} else if (c->count == 1) {
>> +		/* Penultimate byte to xfer, disable ACK gen. */
>> +		st_i2c_clr_bits(i2c_dev->base + SSC_I2C, SSC_I2C_ACKG);
>> +
>> +		/* Last received byte is to be handled by NACK interrupt */
>> +		ien = SSC_IEN_NACKEN | SSC_IEN_ARBLEN;
>> +		writel(ien, i2c_dev->base + SSC_IEN);
>> +
>> +		st_i2c_rd_fill_tx_fifo(i2c_dev, c->count);
>> +	} else
>> +		st_i2c_rd_fill_tx_fifo(i2c_dev, c->count - 1);
>
> Braces around else branch.
Ditto
>
>> +}
>> +static int st_i2c_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	struct st_i2c_dev *i2c_dev;
>> +	struct resource *res;
>> +	u32 clk_rate;
>> +	struct i2c_adapter *adap;
>> +	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);
>> +
>> +	i2c_dev->irq = irq_of_parse_and_map(np, 0);
>> +	if (!i2c_dev->irq) {
>> +		dev_err(&pdev->dev, "IRQ missing or invalid\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	i2c_dev->clk = of_clk_get_by_name(np, "ssc");
>> +	if (IS_ERR(i2c_dev->clk)) {
>> +		dev_err(&pdev->dev, "Unable to request clock\n");
>> +		return PTR_ERR(i2c_dev->clk);
>> +	}
>> +
>> +	i2c_dev->mode = I2C_MODE_STANDARD;
>> +	ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
>> +	if ((!ret) && (clk_rate == 400000))
>> +		i2c_dev->mode = I2C_MODE_FAST;
>> +
>> +	i2c_dev->dev = &pdev->dev;
>> +
>> +	ret = devm_request_irq(&pdev->dev, i2c_dev->irq, st_i2c_isr, 0,
>> +			pdev->name, i2c_dev);
>
> Suggestion: Maybe threaded irq since you do fifo handling in the isr?
That's a good point, I will switch to threaded irq.

>
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
>> +		return ret;
>> +	}
>> +
>> +	pinctrl_pm_select_default_state(i2c_dev->dev);
>> +	/* In case idle state available, select it */
>> +	pinctrl_pm_select_idle_state(i2c_dev->dev);
>> +
>> +	ret = st_i2c_of_get_deglitch(np, i2c_dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	adap = &i2c_dev->adap;
>> +	i2c_set_adapdata(adap, i2c_dev);
>> +	snprintf(adap->name, sizeof(adap->name), "ST I2C(0x%08x)", res->start);
>
> resource_size_t can also be 64 bit.
Okay, I will change to 0x%x
>
>> +	adap->owner = THIS_MODULE;
>> +	adap->timeout = 2 * HZ;
>> +	adap->retries = 0;
>> +	adap->class = I2C_CLASS_HWMON | I2C_CLASS_DDC | I2C_CLASS_SPD;
>
> This question is still open:
> Why do you need class based instantiation. It will most likely cost
> boot-time and you have devicetree means for doing instantiation.
Sorry, I missed to take your remark into account last time...
This is indeed useless and adds a cost at boot time, it will be removed 
in next series.

>
>> +	adap->algo = &st_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) {
>> +		dev_err(&pdev->dev, "Failed to add adapter\n");
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, i2c_dev);
>> +
>> +	dev_info(i2c_dev->dev, "%s initialized\n", adap->name);
>> +
>> +	return 0;
>> +}
>> +
>
> Rest looks good! We are mostly ready to go.
Perfect! :)
I will try to send a new series this week.

Thanks,
Maxime
>
> Thanks,
>
>     Wolfram
>

[PATCH v5 3/4] ARM: STi: Supply I2C configuration to STiH415 SoC

[Maxime COQUELIN]

This patch supplies I2C configuration to STiH415 SoC.

Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
---
 arch/arm/boot/dts/stih415-pinctrl.dtsi |   36 ++++++++++++++++++++++
 arch/arm/boot/dts/stih415.dtsi         |   53 ++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/arch/arm/boot/dts/stih415-pinctrl.dtsi b/arch/arm/boot/dts/stih415-pinctrl.dtsi
index 1d322b2..e56449d 100644
--- a/arch/arm/boot/dts/stih415-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stih415-pinctrl.dtsi
@@ -86,6 +86,24 @@
 					};
 				};
 			};
+
+			sbc_i2c0 {
+				pinctrl_sbc_i2c0_default: sbc_i2c0-default {
+					st,pins {
+						sda = <&PIO4 6 ALT1 BIDIR>;
+						scl = <&PIO4 5 ALT1 BIDIR>;
+					};
+				};
+			};
+
+			sbc_i2c1 {
+				pinctrl_sbc_i2c1_default: sbc_i2c1-default {
+					st,pins {
+						sda = <&PIO3 2 ALT2 BIDIR>;
+						scl = <&PIO3 1 ALT2 BIDIR>;
+					};
+				};
+			};
 		};
 
 		pin-controller-front {
@@ -143,6 +161,24 @@
 				reg		= <0x7000 0x100>;
 				st,bank-name	= "PIO12";
 			};
+
+			i2c0 {
+				pinctrl_i2c0_default: i2c0-default {
+					st,pins {
+						sda = <&PIO9 3 ALT1 BIDIR>;
+						scl = <&PIO9 2 ALT1 BIDIR>;
+					};
+				};
+			};
+
+			i2c1 {
+				pinctrl_i2c1_default: i2c1-default {
+					st,pins {
+						sda = <&PIO12 1 ALT1 BIDIR>;
+						scl = <&PIO12 0 ALT1 BIDIR>;
+					};
+				};
+			};
 		};
 
 		pin-controller-rear {
diff --git a/arch/arm/boot/dts/stih415.dtsi b/arch/arm/boot/dts/stih415.dtsi
index 74ab8de..a85fbf1 100644
--- a/arch/arm/boot/dts/stih415.dtsi
+++ b/arch/arm/boot/dts/stih415.dtsi
@@ -9,6 +9,7 @@
 #include "stih41x.dtsi"
 #include "stih415-clock.dtsi"
 #include "stih415-pinctrl.dtsi"
+#include <dt-bindings/interrupt-controller/arm-gic.h>
 / {
 
 	L2: cache-controller {
@@ -83,5 +84,57 @@
 			pinctrl-names 	= "default";
 			pinctrl-0	= <&pinctrl_sbc_serial1>;
 		};
+
+		i2c@fed40000 {
+			compatible	= "st,comms-ssc-i2c";
+			reg		= <0xfed40000 0x110>;
+			interrupts	=  <GIC_SPI 187 IRQ_TYPE_LEVEL_HIGH>;
+			clocks		= <&CLKS_ICN_REG_0>;
+			clock-names	= "ssc";
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_i2c0_default>;
+
+			status		= "disabled";
+		};
+
+		i2c@fed41000 {
+			compatible	= "st,comms-ssc-i2c";
+			reg		= <0xfed41000 0x110>;
+			interrupts	=  <GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>;
+			clocks		= <&CLKS_ICN_REG_0>;
+			clock-names	= "ssc";
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_i2c1_default>;
+
+			status		= "disabled";
+		};
+
+		i2c@fe540000 {
+			compatible	= "st,comms-ssc-i2c";
+			reg		= <0xfe540000 0x110>;
+			interrupts	=  <GIC_SPI 206 IRQ_TYPE_LEVEL_HIGH>;
+			clocks		= <&CLK_SYSIN>;
+			clock-names	= "ssc";
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_sbc_i2c0_default>;
+
+			status		= "disabled";
+		};
+
+		i2c@fe541000 {
+			compatible	= "st,comms-ssc-i2c";
+			reg		= <0xfe541000 0x110>;
+			interrupts	=  <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>;
+			clocks		= <&CLK_SYSIN>;
+			clock-names	= "ssc";
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_sbc_i2c1_default>;
+
+			status		= "disabled";
+		};
 	};
 };
-- 
1.7.9.5

[PATCH v5 2/4] ARM: STi: Supply I2C configuration to STiH416 SoC

[Maxime COQUELIN]

This patch supplies I2C configuration to STiH416 SoC.

Signed-off-by: Maxime Coquelin <maxime.coquelin-qxv4g6HH51o@public.gmane.org>
---
 arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 +++++++++++++++++++++
 arch/arm/boot/dts/stih416.dtsi         |   53 ++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+)

diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
index 0f246c9..b29ff4b 100644
--- a/arch/arm/boot/dts/stih416-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stih416-pinctrl.dtsi
@@ -97,6 +97,24 @@
 					};
 				};
 			};
+
+			sbc_i2c0 {
+				pinctrl_sbc_i2c0_default: sbc_i2c0-default {
+					st,pins {
+						sda = <&PIO4 6 ALT1 BIDIR>;
+						scl = <&PIO4 5 ALT1 BIDIR>;
+					};
+				};
+			};
+
+			sbc_i2c1 {
+				pinctrl_sbc_i2c1_default: sbc_i2c1-default {
+					st,pins {
+						sda = <&PIO3 2 ALT2 BIDIR>;
+						scl = <&PIO3 1 ALT2 BIDIR>;
+					};
+				};
+			};
 		};
 
 		pin-controller-front {
@@ -175,6 +193,23 @@
 				};
 			};
 
+			i2c0 {
+				pinctrl_i2c0_default: i2c0-default {
+					st,pins {
+						sda = <&PIO9 3 ALT1 BIDIR>;
+						scl = <&PIO9 2 ALT1 BIDIR>;
+					};
+				};
+			};
+
+			i2c1 {
+				pinctrl_i2c1_default: i2c1-default {
+					st,pins {
+						sda = <&PIO12 1 ALT1 BIDIR>;
+						scl = <&PIO12 0 ALT1 BIDIR>;
+					};
+				};
+			};
 		};
 
 		pin-controller-rear {
diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index 1a0326e..04afd1e 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -9,6 +9,7 @@
 #include "stih41x.dtsi"
 #include "stih416-clock.dtsi"
 #include "stih416-pinctrl.dtsi"
+#include <dt-bindings/interrupt-controller/arm-gic.h>
 / {
 	L2: cache-controller {
 		compatible = "arm,pl310-cache";
@@ -92,5 +93,57 @@
 			pinctrl-0 	= <&pinctrl_sbc_serial1>;
 			clocks          = <&CLK_SYSIN>;
 		};
+
+		i2c@fed40000 {
+			compatible	= "st,comms-ssc-i2c";
+			reg		= <0xfed40000 0x110>;
+			interrupts	=  <GIC_SPI 187 IRQ_TYPE_LEVEL_HIGH>;
+			clocks		= <&CLK_S_ICN_REG_0>;
+			clock-names	= "ssc";
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_i2c0_default>;
+
+			status		= "disabled";
+		};
+
+		i2c@fed41000 {
+			compatible	= "st,comms-ssc-i2c";
+			reg		= <0xfed41000 0x110>;
+			interrupts	=  <GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>;
+			clocks		= <&CLK_S_ICN_REG_0>;
+			clock-names	= "ssc";
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_i2c1_default>;
+
+			status		= "disabled";
+		};
+
+		i2c@fe540000 {
+			compatible	= "st,comms-ssc-i2c";
+			reg		= <0xfe540000 0x110>;
+			interrupts	=  <GIC_SPI 206 IRQ_TYPE_LEVEL_HIGH>;
+			clocks		= <&CLK_SYSIN>;
+			clock-names	= "ssc";
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_sbc_i2c0_default>;
+
+			status		= "disabled";
+		};
+
+		i2c@fe541000 {
+			compatible	= "st,comms-ssc-i2c";
+			reg		= <0xfe541000 0x110>;
+			interrupts	=  <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>;
+			clocks		= <&CLK_SYSIN>;
+			clock-names	= "ssc";
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_sbc_i2c1_default>;
+
+			status		= "disabled";
+		};
 	};
 };
-- 
1.7.9.5

[PATCH v5 4/4] ARM: STi: Add I2C config to B2000 and B2020 boards

[Maxime COQUELIN]

This patch supplies I2C configuration to B2000 and B2020
based on either STiH415 or STiH416 SoCs.

Signed-off-by: Maxime Coquelin <maxime.coquelin-qxv4g6HH51o@public.gmane.org>
---
 arch/arm/boot/dts/stih41x-b2000.dtsi |    9 +++++++++
 arch/arm/boot/dts/stih41x-b2020.dtsi |   22 ++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/arch/arm/boot/dts/stih41x-b2000.dtsi b/arch/arm/boot/dts/stih41x-b2000.dtsi
index 8e694d2..1e6aa92 100644
--- a/arch/arm/boot/dts/stih41x-b2000.dtsi
+++ b/arch/arm/boot/dts/stih41x-b2000.dtsi
@@ -37,5 +37,14 @@
 			};
 		};
 
+		/* HDMI Tx I2C */
+		i2c@fed41000 {
+			/* HDMI V1.3a supports Standard mode only */
+			clock-frequency = <100000>;
+			i2c-min-scl-pulse-width-us = <0>;
+			i2c-min-sda-pulse-width-us = <5>;
+
+			status = "okay";
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/stih41x-b2020.dtsi b/arch/arm/boot/dts/stih41x-b2020.dtsi
index 133e181..0ef0a69 100644
--- a/arch/arm/boot/dts/stih41x-b2020.dtsi
+++ b/arch/arm/boot/dts/stih41x-b2020.dtsi
@@ -38,5 +38,27 @@
 				default-state = "off";
 			};
 		};
+
+		i2c@fed40000 {
+			status = "okay";
+		};
+
+		/* HDMI Tx I2C */
+		i2c@fed41000 {
+			/* HDMI V1.3a supports Standard mode only */
+			clock-frequency = <100000>;
+			i2c-min-scl-pulse-width-us = <0>;
+			i2c-min-sda-pulse-width-us = <5>;
+
+			status = "okay";
+		};
+
+		i2c@fe540000 {
+			status = "okay";
+		};
+
+		i2c@fe541000 {
+			status = "okay";
+		};
 	};
 };
-- 
1.7.9.5

Re: [PATCH v5 0/4] Add I2C support to ST SoCs

[Maxime COQUELIN]

Hi Wolfram,

On 10/14/2013 02:46 PM, Maxime COQUELIN wrote:
> The goal of this series is to add I2C support to ST SoCs.
> The DT definition is added for STiH415 and STiH416 SoCs on
> B2000 and B2020 boards.
>
> The series has been tested working on STiH416-B2020 board.
> It applies on top of v3.12-rc5.
>
> Changes since v4:
>    - Fixed IRQ flags to level high triggering as reported by Stephen Gallimore
>    - Revert back to generic DT properties for Anti-glitch filtering. Their DT
>      binding documentation will be updated when i2c DT binding docuementation
>      will be created (In Wolfram's ToDo list)
Do you agree with this approach?

Did you had time to review the driver?

Regards,
Maxime

> ...
>
> Maxime Coquelin (4):
>    i2c: busses: i2c-st: Add ST I2C controller
>    ARM: STi: Supply I2C configuration to STiH416 SoC
>    ARM: STi: Supply I2C configuration to STiH415 SoC
>    ARM: STi: Add I2C config to B2000 and B2020 boards
>
>   Documentation/devicetree/bindings/i2c/i2c-st.txt |   38 +
>   arch/arm/boot/dts/stih415-pinctrl.dtsi           |   36 +
>   arch/arm/boot/dts/stih415.dtsi                   |   53 ++
>   arch/arm/boot/dts/stih416-pinctrl.dtsi           |   35 +
>   arch/arm/boot/dts/stih416.dtsi                   |   53 ++
>   arch/arm/boot/dts/stih41x-b2000.dtsi             |    9 +
>   arch/arm/boot/dts/stih41x-b2020.dtsi             |   22 +
>   drivers/i2c/busses/Kconfig                       |   10 +
>   drivers/i2c/busses/Makefile                      |    1 +
>   drivers/i2c/busses/i2c-st.c                      |  867 ++++++++++++++++++++++
>   10 files changed, 1124 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/i2c/i2c-st.txt
>   create mode 100644 drivers/i2c/busses/i2c-st.c
>