[PATCH 0/5] Add support for MSCC Ocelot i2c

[PATCH 0/5] Add support for MSCC Ocelot i2c

[Alexandre Belloni]

Hi,

Because the designware IP was not able to the the SDA hold time, MSCC has
its own implementation. Add support for it and then add i2c on ocelot
boards.

I would expect patches 1 to 3 to go through the i2c tree and 4-5 through
the mips tree once patch 3 has been reviewed by the DT maintainers.

Alexandre Belloni (5):
  i2c: designware: factorize setting SDA hold time
  i2c: designware: allow IP specific sda_hold_time
  i2c: designware: add MSCC Ocelot support
  mips: dts: mscc: Add i2c on ocelot
  mips: dts: mscc: enable i2c on ocelot_pcb123

 .../bindings/i2c/i2c-designware.txt           |  5 ++-
 arch/mips/boot/dts/mscc/ocelot.dtsi           | 19 +++++++++++
 arch/mips/boot/dts/mscc/ocelot_pcb123.dts     |  5 +++
 drivers/i2c/busses/i2c-designware-common.c    | 33 +++++++++++++++++++
 drivers/i2c/busses/i2c-designware-core.h      |  3 ++
 drivers/i2c/busses/i2c-designware-master.c    | 22 +------------
 drivers/i2c/busses/i2c-designware-platdrv.c   | 20 +++++++++++
 drivers/i2c/busses/i2c-designware-slave.c     | 22 +------------
 8 files changed, 86 insertions(+), 43 deletions(-)

-- 
2.18.0

[PATCH 1/5] i2c: designware: factorize setting SDA hold time

[Alexandre Belloni]

Factorize setting the SDA hold time in a new function
i2c_dw_set_sda_hold_time() that is used for both master and slave mode.

This conveniently pulls the fix for the spurious warning from commit
7a20e707aae2 ("i2c: designware: suppress unneeded SDA hold time warnings")
in slave mode.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/i2c/busses/i2c-designware-common.c | 27 ++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-core.h   |  1 +
 drivers/i2c/busses/i2c-designware-master.c | 22 +-----------------
 drivers/i2c/busses/i2c-designware-slave.c  | 22 +-----------------
 4 files changed, 30 insertions(+), 42 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 48914dfc8ce8..9afc3e075b33 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -293,5 +293,32 @@ u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev)
 }
 EXPORT_SYMBOL_GPL(i2c_dw_read_comp_param);
 
+void i2c_dw_set_sda_hold_time(struct dw_i2c_dev *dev)
+{
+	u32 reg;
+
+	/* Configure SDA Hold Time if required. */
+	reg = dw_readl(dev, DW_IC_COMP_VERSION);
+	if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
+		if (!dev->sda_hold_time) {
+			/* Keep previous hold time setting if no one set it. */
+			dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD);
+		}
+		/*
+		 * Workaround for avoiding TX arbitration lost in case I2C
+		 * slave pulls SDA down "too quickly" after falling egde of
+		 * SCL by enabling non-zero SDA RX hold. Specification says it
+		 * extends incoming SDA low to high transition while SCL is
+		 * high but it apprears to help also above issue.
+		 */
+		if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK))
+			dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT;
+		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
+	} else if (dev->sda_hold_time) {
+		dev_warn(dev->dev,
+			 "Hardware too old to adjust SDA hold time.\n");
+	}
+}
+
 MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter core");
 MODULE_LICENSE("GPL");
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index d690e648bc01..bc43fb9ac1cf 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -306,6 +306,7 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev);
 u32 i2c_dw_func(struct i2c_adapter *adap);
 void i2c_dw_disable(struct dw_i2c_dev *dev);
 void i2c_dw_disable_int(struct dw_i2c_dev *dev);
+void i2c_dw_set_sda_hold_time(struct dw_i2c_dev *dev);
 
 static inline void __i2c_dw_enable(struct dw_i2c_dev *dev)
 {
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 27436a937492..826329f8e30b 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -146,27 +146,7 @@ static int i2c_dw_init_master(struct dw_i2c_dev *dev)
 		}
 	}
 
-	/* Configure SDA Hold Time if required */
-	reg = dw_readl(dev, DW_IC_COMP_VERSION);
-	if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
-		if (!dev->sda_hold_time) {
-			/* Keep previous hold time setting if no one set it */
-			dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD);
-		}
-		/*
-		 * Workaround for avoiding TX arbitration lost in case I2C
-		 * slave pulls SDA down "too quickly" after falling egde of
-		 * SCL by enabling non-zero SDA RX hold. Specification says it
-		 * extends incoming SDA low to high transition while SCL is
-		 * high but it apprears to help also above issue.
-		 */
-		if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK))
-			dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT;
-		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
-	} else if (dev->sda_hold_time) {
-		dev_warn(dev->dev,
-			"Hardware too old to adjust SDA hold time.\n");
-	}
+	i2c_dw_set_sda_hold_time(dev);
 
 	i2c_dw_configure_fifo_master(dev);
 	i2c_dw_release_lock(dev);
diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
index 8ce2cd368477..150e5edb064c 100644
--- a/drivers/i2c/busses/i2c-designware-slave.c
+++ b/drivers/i2c/busses/i2c-designware-slave.c
@@ -77,27 +77,7 @@ static int i2c_dw_init_slave(struct dw_i2c_dev *dev)
 	/* Disable the adapter. */
 	__i2c_dw_disable(dev);
 
-	/* Configure SDA Hold Time if required. */
-	reg = dw_readl(dev, DW_IC_COMP_VERSION);
-	if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
-		if (!dev->sda_hold_time) {
-			/* Keep previous hold time setting if no one set it. */
-			dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD);
-		}
-		/*
-		 * Workaround for avoiding TX arbitration lost in case I2C
-		 * slave pulls SDA down "too quickly" after falling egde of
-		 * SCL by enabling non-zero SDA RX hold. Specification says it
-		 * extends incoming SDA low to high transition while SCL is
-		 * high but it apprears to help also above issue.
-		 */
-		if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK))
-			dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT;
-		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
-	} else {
-		dev_warn(dev->dev,
-			 "Hardware too old to adjust SDA hold time.\n");
-	}
+	i2c_dw_set_sda_hold_time(dev);
 
 	i2c_dw_configure_fifo_slave(dev);
 	i2c_dw_release_lock(dev);
-- 
2.18.0

[PATCH 2/5] i2c: designware: allow IP specific sda_hold_time

[Alexandre Belloni]

Because some old designware IPs were not supporting setting an SDA hold
time, vendors developed their own solution. Add a way for the final driver
to provide its own SDA hold time handling.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/i2c/busses/i2c-designware-common.c | 6 ++++++
 drivers/i2c/busses/i2c-designware-core.h   | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 9afc3e075b33..545b69d6be3c 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -297,6 +297,12 @@ void i2c_dw_set_sda_hold_time(struct dw_i2c_dev *dev)
 {
 	u32 reg;
 
+	if (dev->set_sda_hold_time) {
+		dev->set_sda_hold_time(dev);
+
+		return;
+	}
+
 	/* Configure SDA Hold Time if required. */
 	reg = dw_readl(dev, DW_IC_COMP_VERSION);
 	if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index bc43fb9ac1cf..b2778b6d8aca 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -283,6 +283,7 @@ struct dw_i2c_dev {
 	void			(*disable)(struct dw_i2c_dev *dev);
 	void			(*disable_int)(struct dw_i2c_dev *dev);
 	int			(*init)(struct dw_i2c_dev *dev);
+	int			(*set_sda_hold_time)(struct dw_i2c_dev *dev);
 	int			mode;
 	struct i2c_bus_recovery_info rinfo;
 };
-- 
2.18.0

[PATCH 3/5] i2c: designware: add MSCC Ocelot support

[Alexandre Belloni]

The Microsemi Ocelot I2C controller is a designware IP. It also has a
second set of registers to allow tweaking SDA hold time and spike
filtering.

Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 .../bindings/i2c/i2c-designware.txt           |  5 ++++-
 drivers/i2c/busses/i2c-designware-core.h      |  1 +
 drivers/i2c/busses/i2c-designware-platdrv.c   | 20 +++++++++++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index fbb0a6d8b964..7af4176da4af 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -2,7 +2,7 @@
 
 Required properties :
 
- - compatible : should be "snps,designware-i2c"
+ - compatible : should be "snps,designware-i2c" or "mscc,ocelot-i2c"
  - reg : Offset and length of the register set for the device
  - interrupts : <IRQ> where IRQ is the interrupt number.
 
@@ -11,6 +11,9 @@ Recommended properties :
  - clock-frequency : desired I2C bus clock frequency in Hz.
 
 Optional properties :
+ - reg : for "mscc,ocelot-i2c", a second register set to configure the SDA hold
+   time, named ICPU_CFG:TWI_DELAY in the datasheet.
+
  - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
    This option is only supported in hardware blocks version 1.11a or newer.
 
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index b2778b6d8aca..37f63d084c1d 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -237,6 +237,7 @@
 struct dw_i2c_dev {
 	struct device		*dev;
 	void __iomem		*base;
+	void __iomem		*base_ext;
 	struct completion	cmd_complete;
 	struct clk		*clk;
 	struct reset_control	*rst;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 5660daf6c92e..1a476a6e3551 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -204,6 +204,18 @@ static void i2c_dw_configure_slave(struct dw_i2c_dev *dev)
 	dev->mode = DW_IC_SLAVE;
 }
 
+#define MSCC_ICPU_CFG_TWI_DELAY		0x0
+#define MSCC_ICPU_CFG_TWI_DELAY_ENABLE	BIT(0)
+#define MSCC_ICPU_CFG_TWI_SPIKE_FILTER	0x4
+
+static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev)
+{
+	writel((dev->sda_hold_time << 1) | MSCC_ICPU_CFG_TWI_DELAY_ENABLE,
+	       dev->base_ext + MSCC_ICPU_CFG_TWI_DELAY);
+
+	return 0;
+}
+
 static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id)
 {
 	u32 param, tx_fifo_depth, rx_fifo_depth;
@@ -342,6 +354,13 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 				1000000);
 	}
 
+	if (of_device_is_compatible(pdev->dev.of_node, "mscc,ocelot-i2c")) {
+		mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		dev->base_ext = devm_ioremap_resource(&pdev->dev, mem);
+		if (!IS_ERR(dev->base_ext))
+			dev->set_sda_hold_time = mscc_twi_set_sda_hold_time;
+	}
+
 	dw_i2c_set_fifo_size(dev, pdev->id);
 
 	adap = &dev->adapter;
@@ -410,6 +429,7 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
 #ifdef CONFIG_OF
 static const struct of_device_id dw_i2c_of_match[] = {
 	{ .compatible = "snps,designware-i2c", },
+	{ .compatible = "mscc,ocelot-i2c", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
-- 
2.18.0

[PATCH 4/5] mips: dts: mscc: Add i2c on ocelot

[Alexandre Belloni]

Ocelot has an i2c controller, add it. There is only one possible pinmux
configuration so add it as well.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 arch/mips/boot/dts/mscc/ocelot.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/mips/boot/dts/mscc/ocelot.dtsi b/arch/mips/boot/dts/mscc/ocelot.dtsi
index 4f33dbc67348..8b6398e09bb6 100644
--- a/arch/mips/boot/dts/mscc/ocelot.dtsi
+++ b/arch/mips/boot/dts/mscc/ocelot.dtsi
@@ -78,6 +78,20 @@
 			status = "disabled";
 		};
 
+		i2c: i2c@100400 {
+			compatible = "mscc,ocelot-i2c", "snps,designware-i2c";
+			pinctrl-0 = <&i2c_pins>;
+			pinctrl-names = "default";
+			reg = <0x100400 0x100>, <0x198 0x8>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			interrupts = <8>;
+			i2c-sda-hold-time-ns = <300>;
+			clocks = <&ahb_clk>;
+
+			status = "disabled";
+		};
+
 		uart2: serial@100800 {
 			pinctrl-0 = <&uart2_pins>;
 			pinctrl-names = "default";
@@ -178,6 +192,11 @@
 				pins = "GPIO_12", "GPIO_13";
 				function = "uart2";
 			};
+
+			i2c_pins: i2c-pins {
+				pins = "GPIO_16", "GPIO_17";
+				function = "twi";
+			};
 		};
 
 		mdio0: mdio@107009c {
-- 
2.18.0

[PATCH 5/5] mips: dts: mscc: enable i2c on ocelot_pcb123

[Alexandre Belloni]

Enable the i2c controller on ocelot PCB123. While there are no i2c devices
on the board itself, it can be used to control the SFP transceivers.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 arch/mips/boot/dts/mscc/ocelot_pcb123.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/mips/boot/dts/mscc/ocelot_pcb123.dts b/arch/mips/boot/dts/mscc/ocelot_pcb123.dts
index 4ccd65379059..d933f1cd8a60 100644
--- a/arch/mips/boot/dts/mscc/ocelot_pcb123.dts
+++ b/arch/mips/boot/dts/mscc/ocelot_pcb123.dts
@@ -26,6 +26,11 @@
 	status = "okay";
 };
 
+&i2c {
+	clock-frequency = <100000>;
+	status = "okay";
+};
+
 &mdio0 {
 	status = "okay";
 };
-- 
2.18.0

Re: [PATCH 1/5] i2c: designware: factorize setting SDA hold time

[Andy Shevchenko]

On Tue, 2018-07-17 at 13:48 +0200, Alexandre Belloni wrote:
> Factorize setting the SDA hold time in a new function
> i2c_dw_set_sda_hold_time() that is used for both master and slave
> mode.
> 
> This conveniently pulls the fix for the spurious warning from commit
> 7a20e707aae2 ("i2c: designware: suppress unneeded SDA hold time
> warnings")
> in slave mode.

Isn't this a duplication of 

commit 1080ee7e28e1cea86310739e5dd4612868768aed
Author: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Date:   Tue Jun 19 14:23:22 2018 +0300

    i2c: designware: Move SDA hold time configuration to common code

?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/5] i2c: designware: add MSCC Ocelot support

[Andy Shevchenko]

On Tue, 2018-07-17 at 13:48 +0200, Alexandre Belloni wrote:
> The Microsemi Ocelot I2C controller is a designware IP. It also has a
> second set of registers to allow tweaking SDA hold time and spike
> filtering.

Can you elaborate a bit?

Are they platform specific? Are they shadow registers? Are they
something else? Datasheet link / excerpt would be also good to read.
 
>  Optional properties :
> + - reg : for "mscc,ocelot-i2c", a second register set to configure
> the SDA hold
> +   time, named ICPU_CFG:TWI_DELAY in the datasheet.
> +

Hmm... Is this registers unique to the SoC in question? Is address of
them fixed or may be configured on RTL level?

If former is right, why do we need a separate property?

>  
> +#define MSCC_ICPU_CFG_TWI_DELAY		0x0
> +#define MSCC_ICPU_CFG_TWI_DELAY_ENABLE	BIT(0)
> +#define MSCC_ICPU_CFG_TWI_SPIKE_FILTER	0x4
> +
> +static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev)
> +{
> +	writel((dev->sda_hold_time << 1) |
> MSCC_ICPU_CFG_TWI_DELAY_ENABLE,
> +	       dev->base_ext + MSCC_ICPU_CFG_TWI_DELAY);
> +
> +	return 0;
> +}

Hmm... And does how this make native DesignWare IP's registers obsolete?


> +	if (of_device_is_compatible(pdev->dev.of_node, "mscc,ocelot-
> i2c"))

Can't you just ask for this unconditionally? Why not?
(It seems I might have known why not, but can we use named resource
instead in case this is not so SoC specific)


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 0/5] Add support for MSCC Ocelot i2c

[Andy Shevchenko]

On Tue, 2018-07-17 at 13:48 +0200, Alexandre Belloni wrote:
> Hi,
> 
> Because the designware IP was not able to the the SDA hold time, MSCC
> has
> its own implementation. Add support for it and then add i2c on ocelot
> boards.
> 
> I would expect patches 1 to 3 to go through the i2c tree and 4-5
> through
> the mips tree once patch 3 has been reviewed by the DT maintainers.

Without reading datasheet it all feels like a wrong place to put.

But maybe that SoC (SoC family?) has some update to DesignWare IP.
Btw, what the version of DW IP it's using? 2.00a? More older, more
recent?


> Alexandre Belloni (5):
>   i2c: designware: factorize setting SDA hold time
>   i2c: designware: allow IP specific sda_hold_time
>   i2c: designware: add MSCC Ocelot support
>   mips: dts: mscc: Add i2c on ocelot
>   mips: dts: mscc: enable i2c on ocelot_pcb123
> 
>  .../bindings/i2c/i2c-designware.txt           |  5 ++-
>  arch/mips/boot/dts/mscc/ocelot.dtsi           | 19 +++++++++++
>  arch/mips/boot/dts/mscc/ocelot_pcb123.dts     |  5 +++
>  drivers/i2c/busses/i2c-designware-common.c    | 33
> +++++++++++++++++++
>  drivers/i2c/busses/i2c-designware-core.h      |  3 ++
>  drivers/i2c/busses/i2c-designware-master.c    | 22 +------------
>  drivers/i2c/busses/i2c-designware-platdrv.c   | 20 +++++++++++
>  drivers/i2c/busses/i2c-designware-slave.c     | 22 +------------
>  8 files changed, 86 insertions(+), 43 deletions(-)
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/5] i2c: designware: factorize setting SDA hold time

[Alexandre Belloni]

On 17/07/2018 15:11:50+0300, Andy Shevchenko wrote:
> On Tue, 2018-07-17 at 13:48 +0200, Alexandre Belloni wrote:
> > Factorize setting the SDA hold time in a new function
> > i2c_dw_set_sda_hold_time() that is used for both master and slave
> > mode.
> > 
> > This conveniently pulls the fix for the spurious warning from commit
> > 7a20e707aae2 ("i2c: designware: suppress unneeded SDA hold time
> > warnings")
> > in slave mode.
> 
> Isn't this a duplication of 
> 
> commit 1080ee7e28e1cea86310739e5dd4612868768aed
> Author: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Date:   Tue Jun 19 14:23:22 2018 +0300
> 
>     i2c: designware: Move SDA hold time configuration to common code
> 
> ?
> 

Indeed, I was on 4.18-rc1, not -next.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 3/5] i2c: designware: add MSCC Ocelot support

[Alexandre Belloni]

On 17/07/2018 15:19:08+0300, Andy Shevchenko wrote:
> On Tue, 2018-07-17 at 13:48 +0200, Alexandre Belloni wrote:
> > The Microsemi Ocelot I2C controller is a designware IP. It also has a
> > second set of registers to allow tweaking SDA hold time and spike
> > filtering.
> 
> Can you elaborate a bit?
> 
> Are they platform specific? Are they shadow registers? Are they
> something else? Datasheet link / excerpt would be also good to read.
>  
> >  Optional properties :
> > + - reg : for "mscc,ocelot-i2c", a second register set to configure
> > the SDA hold
> > +   time, named ICPU_CFG:TWI_DELAY in the datasheet.
> > +
> 
> Hmm... Is this registers unique to the SoC in question? Is address of
> them fixed or may be configured on RTL level?
> 
> If former is right, why do we need a separate property?
> 

Those are registers from the SoC, their position varies depending on
the SoC.

Even if the position was fixed, I'm pretty sure another register set is
needed. It is not a new property.

> >  
> > +#define MSCC_ICPU_CFG_TWI_DELAY		0x0
> > +#define MSCC_ICPU_CFG_TWI_DELAY_ENABLE	BIT(0)
> > +#define MSCC_ICPU_CFG_TWI_SPIKE_FILTER	0x4
> > +
> > +static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev)
> > +{
> > +	writel((dev->sda_hold_time << 1) |
> > MSCC_ICPU_CFG_TWI_DELAY_ENABLE,
> > +	       dev->base_ext + MSCC_ICPU_CFG_TWI_DELAY);
> > +
> > +	return 0;
> > +}
> 
> Hmm... And does how this make native DesignWare IP's registers obsolete?
> 

DW_IC_SDA_HOLD doesn't exist in this version of the IP. It is replaced
by this SoC specific register.

> 
> > +	if (of_device_is_compatible(pdev->dev.of_node, "mscc,ocelot-
> > i2c"))
> 
> Can't you just ask for this unconditionally? Why not?
> (It seems I might have known why not, but can we use named resource
> instead in case this is not so SoC specific)
> 

It is SoC specific.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/5] Add support for MSCC Ocelot i2c

[Alexandre Belloni]

On 17/07/2018 15:21:20+0300, Andy Shevchenko wrote:
> On Tue, 2018-07-17 at 13:48 +0200, Alexandre Belloni wrote:
> > Hi,
> > 
> > Because the designware IP was not able to the the SDA hold time, MSCC
> > has
> > its own implementation. Add support for it and then add i2c on ocelot
> > boards.
> > 
> > I would expect patches 1 to 3 to go through the i2c tree and 4-5
> > through
> > the mips tree once patch 3 has been reviewed by the DT maintainers.
> 
> Without reading datasheet it all feels like a wrong place to put.
> 
> But maybe that SoC (SoC family?) has some update to DesignWare IP.
> Btw, what the version of DW IP it's using? 2.00a? More older, more
> recent?
> 

COMP_VERSION reads 0x3131302A so it is before DW_IC_SDA_HOLD_MIN_VERS.

> 
> > Alexandre Belloni (5):
> >   i2c: designware: factorize setting SDA hold time
> >   i2c: designware: allow IP specific sda_hold_time
> >   i2c: designware: add MSCC Ocelot support
> >   mips: dts: mscc: Add i2c on ocelot
> >   mips: dts: mscc: enable i2c on ocelot_pcb123
> > 
> >  .../bindings/i2c/i2c-designware.txt           |  5 ++-
> >  arch/mips/boot/dts/mscc/ocelot.dtsi           | 19 +++++++++++
> >  arch/mips/boot/dts/mscc/ocelot_pcb123.dts     |  5 +++
> >  drivers/i2c/busses/i2c-designware-common.c    | 33
> > +++++++++++++++++++
> >  drivers/i2c/busses/i2c-designware-core.h      |  3 ++
> >  drivers/i2c/busses/i2c-designware-master.c    | 22 +------------
> >  drivers/i2c/busses/i2c-designware-platdrv.c   | 20 +++++++++++
> >  drivers/i2c/busses/i2c-designware-slave.c     | 22 +------------
> >  8 files changed, 86 insertions(+), 43 deletions(-)
> > 
> 
> -- 
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/5] i2c: designware: allow IP specific sda_hold_time

[Andy Shevchenko]

On Tue, 2018-07-17 at 13:48 +0200, Alexandre Belloni wrote:
> Because some old designware IPs were not supporting setting an SDA
> hold
> time, vendors developed their own solution. Add a way for the final
> driver
> to provide its own SDA hold time handling.
> 

Thanks for information you provided. See my comment below.
After addressing it, 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  drivers/i2c/busses/i2c-designware-common.c | 6 ++++++
>  drivers/i2c/busses/i2c-designware-core.h   | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-common.c
> b/drivers/i2c/busses/i2c-designware-common.c
> index 9afc3e075b33..545b69d6be3c 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -297,6 +297,12 @@ void i2c_dw_set_sda_hold_time(struct dw_i2c_dev
> *dev)
>  {
>  	u32 reg;
> 

>  
> +	if (dev->set_sda_hold_time) {
> +		dev->set_sda_hold_time(dev);
> +
> +		return;
> +	}

I would rather inject this, for now (*), into if-else-if ladder, i.e.

if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
 ...
} else if (dev->set_sda_hold_time) { // <<<
 ...
} else if (dev->sda_hold_time) {
 ...

(*) Since your IP is of v1.10a I don't believe any new version of IP
would shadow existing DW IP registers, thus version check is okay to
have first in my opinion.

> +
>  	/* Configure SDA Hold Time if required. */
>  	reg = dw_readl(dev, DW_IC_COMP_VERSION);
>  	if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
> diff --git a/drivers/i2c/busses/i2c-designware-core.h
> b/drivers/i2c/busses/i2c-designware-core.h
> index bc43fb9ac1cf..b2778b6d8aca 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -283,6 +283,7 @@ struct dw_i2c_dev {
>  	void			(*disable)(struct dw_i2c_dev
> *dev);
>  	void			(*disable_int)(struct dw_i2c_dev
> *dev);
>  	int			(*init)(struct dw_i2c_dev *dev);
> +	int			(*set_sda_hold_time)(struct
> dw_i2c_dev *dev);
>  	int			mode;
>  	struct i2c_bus_recovery_info rinfo;
>  };

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/5] i2c: designware: add MSCC Ocelot support

[Andy Shevchenko]

On Tue, 2018-07-17 at 13:48 +0200, Alexandre Belloni wrote:
> The Microsemi Ocelot I2C controller is a designware IP. It also has a
> second set of registers to allow tweaking SDA hold time and spike
> filtering.

Thanks for information you provided. See my comments below.

>  struct dw_i2c_dev {
>  	struct device		*dev;
>  	void __iomem		*base;
> +	void __iomem		*base_ext;

Maybe simple "ext"? Up to you.
 
> +#define MSCC_ICPU_CFG_TWI_DELAY		0x0
> +#define MSCC_ICPU_CFG_TWI_DELAY_ENABLE	BIT(0)
> +#define MSCC_ICPU_CFG_TWI_SPIKE_FILTER	0x4
> +
> +static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev)
> +{
> +	writel((dev->sda_hold_time << 1) |
> MSCC_ICPU_CFG_TWI_DELAY_ENABLE,
> +	       dev->base_ext + MSCC_ICPU_CFG_TWI_DELAY);
> +
> +	return 0;
> +}

(1)

>  
> +	if (of_device_is_compatible(pdev->dev.of_node, "mscc,ocelot-
> i2c")) {
> +		mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +		dev->base_ext = devm_ioremap_resource(&pdev->dev,
> mem);
> +		if (!IS_ERR(dev->base_ext))
> +			dev->set_sda_hold_time =
> mscc_twi_set_sda_hold_time;
> +	}

(2)

>  static const struct of_device_id dw_i2c_of_match[] = {
>  	{ .compatible = "snps,designware-i2c", },
> +	{ .compatible = "mscc,ocelot-i2c", },
>  	{},
>  };

(3)


I would rather place them in analogue how we do for ACPI, i.e.

--- 8< --- 8< ---

...
#define MODEL_MSCC_OCELOT  0x00000200
#define MODEL_MASK         0x00000f00
...

#ifdef CONFIG_OF
-->(1)
int dw_i2c_of_configure(pdev)
{
...
-->(2) (perhaps we don't care if it goes without condition, or move it
below in the corresponding case?
...

 switch(dev->flags & MODEL_MASK) {
  case MODEL_MSCC_OCELOT:
   ...
   break;
  default:
   break;
 }

 return 0;
}

-->(3)
...
{ .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
...

#else
static inline int dw_i2c_of_configure(pdev) { return -ENODEV; }
#endif

...

->probe():

...

/* REPLACE THIS in dw_i2c_acpi_configure() by below in ->probe():
 * id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev-
>dev);
 * if (id && id->driver_data)
 *   dev->flags |= (u32)id->driver_data;
 */

  dev->flags |= (u32)device_get_match_data(&pdev->dev);

  if (&pdev->dev.of_node)
    dw_i2c_of_configure(pdev);

...

--- 8< --- 8< ---

What do you think?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/5] i2c: designware: add MSCC Ocelot support

[Andy Shevchenko]

On Tue, 2018-07-17 at 18:16 +0300, Andy Shevchenko wrote:
> On Tue, 2018-07-17 at 13:48 +0200, Alexandre Belloni wrote:
> > The Microsemi Ocelot I2C controller is a designware IP. It also has
> > a
> > second set of registers to allow tweaking SDA hold time and spike
> > filtering.

> What do you think?

You can also split it to 2-3 patches, like:
- move to device_get_match_data()
- move OF device table above in the code (no func changes)
- add support for MSCC


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/5] i2c: designware: add MSCC Ocelot support

[Rob Herring]

On Tue, Jul 17, 2018 at 01:48:35PM +0200, Alexandre Belloni wrote:
> The Microsemi Ocelot I2C controller is a designware IP. It also has a
> second set of registers to allow tweaking SDA hold time and spike
> filtering.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  .../bindings/i2c/i2c-designware.txt           |  5 ++++-
>  drivers/i2c/busses/i2c-designware-core.h      |  1 +
>  drivers/i2c/busses/i2c-designware-platdrv.c   | 20 +++++++++++++++++++
>  3 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> index fbb0a6d8b964..7af4176da4af 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -2,7 +2,7 @@
>  
>  Required properties :
>  
> - - compatible : should be "snps,designware-i2c"
> + - compatible : should be "snps,designware-i2c" or "mscc,ocelot-i2c"

Sounds like the registers are optional (or could be initialized by 
firmware), so shouldn't 'snps,designware-i2c' be a fallback compatible?

>   - reg : Offset and length of the register set for the device
>   - interrupts : <IRQ> where IRQ is the interrupt number.
>  
> @@ -11,6 +11,9 @@ Recommended properties :
>   - clock-frequency : desired I2C bus clock frequency in Hz.
>  
>  Optional properties :
> + - reg : for "mscc,ocelot-i2c", a second register set to configure the SDA hold
> +   time, named ICPU_CFG:TWI_DELAY in the datasheet.
> +
>   - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
>     This option is only supported in hardware blocks version 1.11a or newer.

Perhaps this needs an update too? It sounds like Microsemi fixed this 
problem on their own before version 1.11a of the IP block.

Rob