[PATCH v2 0/2] i2c: i2c-ocores: clock improvements

[PATCH v2 0/2] i2c: i2c-ocores: clock improvements

[Max Filippov]

Peter, Wolfram,

this series fixes clock-frequency attribute usage by i2c-ocores driver and
adds optional common clock framework support.

Changes v1->v2:
- add cleanup patch for clock-frequency attribute that implements options
  2 and 3, as was suggested here: https://lkml.org/lkml/2015/1/22/527
- make CCF support optional

Max Filippov (2):
  i2c: i2c-ocores: fix clock-frequency binding usage
  i2c: i2c-ocores: add common clock support

 .../devicetree/bindings/i2c/i2c-ocores.txt         | 42 +++++++++-
 drivers/i2c/busses/i2c-ocores.c                    | 95 +++++++++++++++++++---
 2 files changed, 122 insertions(+), 15 deletions(-)

-- 
1.8.1.4

[PATCH v2 1/2] i2c: i2c-ocores: fix clock-frequency binding usage

[Max Filippov]

clock-frequency property is meant to control the bus frequency for i2c bus
drivers, but it was incorrectly used to specify i2c controller input clock
frequency.
Introduce new attribute, opencores,ip-clock-frequency, that specifies i2c
controller clock frequency and make clock-frequency attribute compatible
with other i2c drivers. Maintain backwards compatibility in case
opencores,ip-clock-frequency attribute is missing.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
Changes v1->v2:
- new patch

 .../devicetree/bindings/i2c/i2c-ocores.txt         | 18 ++++++-
 drivers/i2c/busses/i2c-ocores.c                    | 55 +++++++++++++++++-----
 2 files changed, 58 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
index 1637c29..5bef3ad 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
@@ -4,15 +4,29 @@ Required properties:
 - compatible      : "opencores,i2c-ocores" or "aeroflexgaisler,i2cmst"
 - reg             : bus address start and address range size of device
 - interrupts      : interrupt number
-- clock-frequency : frequency of bus clock in Hz
+- opencores,ip-clock-frequency: frequency of the controller clock in Hz;
+                    see the note below
 - #address-cells  : should be <1>
 - #size-cells     : should be <0>
 
 Optional properties:
+- clock-frequency : frequency of bus clock in Hz; see the note below.
+                    Defaults to 100 KHz when the property is not specified
 - reg-shift       : device register offsets are shifted by this value
 - reg-io-width    : io register width in bytes (1, 2 or 4)
 - regstep         : deprecated, use reg-shift above
 
+Note
+clock-frequency property is meant to control the bus frequency for i2c bus
+drivers, but it was incorrectly used to specify i2c controller input clock
+frequency. So the following rules are set to fix this situation:
+- if clock-frequency is present and opencores,ip-clock-frequency is not,
+  then clock-frequency specifies i2c controller clock frequency. This is
+  to keep backwards compatibility with setups using old DTB. i2c bus
+  frequency is fixed at 100 KHz.
+- if opencores,ip-clock-frequency is present it specifies i2c controller
+  clock frequency. clock-frequency property specifies i2c bus frequency.
+
 Example:
 
 	i2c0: ocores@a0000000 {
@@ -21,7 +35,7 @@ Example:
 		compatible = "opencores,i2c-ocores";
 		reg = <0xa0000000 0x8>;
 		interrupts = <10>;
-		clock-frequency = <20000000>;
+		opencores,ip-clock-frequency = <20000000>;
 
 		reg-shift = <0>;	/* 8 bit registers */
 		reg-io-width = <1>;	/* 8 bit read/write */
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 2a4fe0b..bff99e3 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -35,7 +35,8 @@ struct ocores_i2c {
 	int pos;
 	int nmsgs;
 	int state; /* see STATE_ */
-	int clock_khz;
+	int ip_clock_khz;
+	int bus_clock_khz;
 	void (*setreg)(struct ocores_i2c *i2c, int reg, u8 value);
 	u8 (*getreg)(struct ocores_i2c *i2c, int reg);
 };
@@ -215,21 +216,34 @@ static int ocores_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 		return -ETIMEDOUT;
 }
 
-static void ocores_init(struct ocores_i2c *i2c)
+static int ocores_init(struct device *dev, struct ocores_i2c *i2c)
 {
 	int prescale;
+	int diff;
 	u8 ctrl = oc_getreg(i2c, OCI2C_CONTROL);
 
 	/* make sure the device is disabled */
 	oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
 
-	prescale = (i2c->clock_khz / (5*100)) - 1;
+	prescale = (i2c->ip_clock_khz / (5 * i2c->bus_clock_khz)) - 1;
+	prescale = clamp(prescale, 0, 0xffff);
+
+	diff = i2c->ip_clock_khz / (5 * (prescale + 1)) - i2c->bus_clock_khz;
+	if (abs(diff) > i2c->bus_clock_khz / 10) {
+		dev_err(dev,
+			"Unsupported clock settings: core: %d KHz, bus: %d KHz\n",
+			i2c->ip_clock_khz, i2c->bus_clock_khz);
+		return -EINVAL;
+	}
+
 	oc_setreg(i2c, OCI2C_PRELOW, prescale & 0xff);
 	oc_setreg(i2c, OCI2C_PREHIGH, prescale >> 8);
 
 	/* Init the device */
 	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
 	oc_setreg(i2c, OCI2C_CONTROL, ctrl | OCI2C_CTRL_IEN | OCI2C_CTRL_EN);
+
+	return 0;
 }
 
 
@@ -304,6 +318,8 @@ static int ocores_i2c_of_probe(struct platform_device *pdev,
 	struct device_node *np = pdev->dev.of_node;
 	const struct of_device_id *match;
 	u32 val;
+	u32 clock_frequency;
+	bool clock_frequency_present;
 
 	if (of_property_read_u32(np, "reg-shift", &i2c->reg_shift)) {
 		/* no 'reg-shift', check for deprecated 'regstep' */
@@ -319,12 +335,24 @@ static int ocores_i2c_of_probe(struct platform_device *pdev,
 		}
 	}
 
-	if (of_property_read_u32(np, "clock-frequency", &val)) {
-		dev_err(&pdev->dev,
-			"Missing required parameter 'clock-frequency'\n");
-		return -ENODEV;
+	clock_frequency_present = !of_property_read_u32(np, "clock-frequency",
+							&clock_frequency);
+	i2c->bus_clock_khz = 100;
+
+	if (of_property_read_u32(np, "opencores,ip-clock-frequency", &val)) {
+		if (!clock_frequency_present) {
+			dev_err(&pdev->dev,
+				"Missing required parameter 'opencores,ip-clock-frequency'\n");
+			return -ENODEV;
+		}
+		i2c->ip_clock_khz = clock_frequency / 1000;
+		dev_warn(&pdev->dev,
+			 "Deprecated usage of the 'clock-frequency' property, please update to 'opencores,ip-clock-frequency'\n");
+	} else {
+		i2c->ip_clock_khz = val / 1000;
+		if (clock_frequency_present)
+			i2c->bus_clock_khz = clock_frequency / 1000;
 	}
-	i2c->clock_khz = val / 1000;
 
 	of_property_read_u32(pdev->dev.of_node, "reg-io-width",
 				&i2c->reg_io_width);
@@ -368,7 +396,8 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 	if (pdata) {
 		i2c->reg_shift = pdata->reg_shift;
 		i2c->reg_io_width = pdata->reg_io_width;
-		i2c->clock_khz = pdata->clock_khz;
+		i2c->ip_clock_khz = pdata->clock_khz;
+		i2c->bus_clock_khz = 100;
 	} else {
 		ret = ocores_i2c_of_probe(pdev, i2c);
 		if (ret)
@@ -402,7 +431,9 @@ static int ocores_i2c_probe(struct platform_device *pdev)
 		}
 	}
 
-	ocores_init(i2c);
+	ret = ocores_init(&pdev->dev, i2c);
+	if (ret)
+		return ret;
 
 	init_waitqueue_head(&i2c->wait);
 	ret = devm_request_irq(&pdev->dev, irq, ocores_isr, 0,
@@ -465,9 +496,7 @@ static int ocores_i2c_resume(struct device *dev)
 {
 	struct ocores_i2c *i2c = dev_get_drvdata(dev);
 
-	ocores_init(i2c);
-
-	return 0;
+	return ocores_init(dev, i2c);
 }
 
 static SIMPLE_DEV_PM_OPS(ocores_i2c_pm, ocores_i2c_suspend, ocores_i2c_resume);
-- 
1.8.1.4

Re: [PATCH v2 1/2] i2c: i2c-ocores: fix clock-frequency binding usage

[Wolfram Sang]

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

On Mon, Feb 02, 2015 at 06:28:12PM +0300, Max Filippov wrote:
> clock-frequency property is meant to control the bus frequency for i2c bus
> drivers, but it was incorrectly used to specify i2c controller input clock
> frequency.
> Introduce new attribute, opencores,ip-clock-frequency, that specifies i2c
> controller clock frequency and make clock-frequency attribute compatible
> with other i2c drivers. Maintain backwards compatibility in case
> opencores,ip-clock-frequency attribute is missing.
> 
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>

Exactly what I had in mind. Thank you for making the effort to fix this
properly!

Applied to for-next!


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

Re: [PATCH v2 1/2] i2c: i2c-ocores: fix clock-frequency binding usage

[Max Filippov]

On Thu, Feb 5, 2015 at 10:11 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> Exactly what I had in mind. Thank you for making the effort to fix this
> properly!
>
> Applied to for-next!

Wolfram, is there any chance this series makes it into 3.20?
(I need to update xtfpga platform, and the update depends on whether
these changes are there or not).

-- 
Thanks.
-- Max

Re: [PATCH v2 1/2] i2c: i2c-ocores: fix clock-frequency binding usage

[Wolfram Sang]

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

On Thu, Feb 05, 2015 at 10:46:26PM +0300, Max Filippov wrote:
> On Thu, Feb 5, 2015 at 10:11 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > Exactly what I had in mind. Thank you for making the effort to fix this
> > properly!
> >
> > Applied to for-next!
> 
> Wolfram, is there any chance this series makes it into 3.20?
> (I need to update xtfpga platform, and the update depends on whether
> these changes are there or not).

Yes, it is planned for 3.20.


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

[PATCH v2 2/2] i2c: i2c-ocores: add common clock support

[Max Filippov]

Allow bus clock specification as a common clock handle. This makes this
controller easier to use in a setup based on common clock framework.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
Changes v1->v2:
- fix binding documentation;
- make CCF support optional.

 .../devicetree/bindings/i2c/i2c-ocores.txt         | 32 +++++++++++++---
 drivers/i2c/busses/i2c-ocores.c                    | 44 +++++++++++++++++++++-
 2 files changed, 70 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
index 5bef3ad..17bef9a 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
@@ -4,8 +4,10 @@ Required properties:
 - compatible      : "opencores,i2c-ocores" or "aeroflexgaisler,i2cmst"
 - reg             : bus address start and address range size of device
 - interrupts      : interrupt number
+- clocks          : handle to the controller clock; see the note below.
+                    Mutually exclusive with opencores,ip-clock-frequency
 - opencores,ip-clock-frequency: frequency of the controller clock in Hz;
-                    see the note below
+                    see the note below. Mutually exclusive with clocks
 - #address-cells  : should be <1>
 - #size-cells     : should be <0>
 
@@ -20,14 +22,16 @@ Note
 clock-frequency property is meant to control the bus frequency for i2c bus
 drivers, but it was incorrectly used to specify i2c controller input clock
 frequency. So the following rules are set to fix this situation:
-- if clock-frequency is present and opencores,ip-clock-frequency is not,
-  then clock-frequency specifies i2c controller clock frequency. This is
-  to keep backwards compatibility with setups using old DTB. i2c bus
+- if clock-frequency is present and neither opencores,ip-clock-frequency nor
+  clocks are, then clock-frequency specifies i2c controller clock frequency.
+  This is to keep backwards compatibility with setups using old DTB. i2c bus
   frequency is fixed at 100 KHz.
+- if clocks is present it specifies i2c controller clock. clock-frequency
+  property specifies i2c bus frequency.
 - if opencores,ip-clock-frequency is present it specifies i2c controller
   clock frequency. clock-frequency property specifies i2c bus frequency.
 
-Example:
+Examples:
 
 	i2c0: ocores@a0000000 {
 		#address-cells = <1>;
@@ -45,3 +49,21 @@ Example:
 			reg = <0x60>;
 		};
 	};
+or
+	i2c0: ocores@a0000000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "opencores,i2c-ocores";
+		reg = <0xa0000000 0x8>;
+		interrupts = <10>;
+		clocks = <&osc>;
+		clock-frequency = <400000>; /* i2c bus frequency 400 KHz */
+
+		reg-shift = <0>;	/* 8 bit registers */
+		reg-io-width = <1>;	/* 8 bit read/write */
+
+		dummy@60 {
+			compatible = "dummy";
+			reg = <0x60>;
+		};
+	};
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index bff99e3..f9ec14c 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -12,6 +12,7 @@
  * kind, whether express or implied.
  */
 
+#include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -35,6 +36,9 @@ struct ocores_i2c {
 	int pos;
 	int nmsgs;
 	int state; /* see STATE_ */
+#ifdef CONFIG_HAVE_CLK
+	struct clk *clk;
+#endif
 	int ip_clock_khz;
 	int bus_clock_khz;
 	void (*setreg)(struct ocores_i2c *i2c, int reg, u8 value);
@@ -339,7 +343,24 @@ static int ocores_i2c_of_probe(struct platform_device *pdev,
 							&clock_frequency);
 	i2c->bus_clock_khz = 100;
 
-	if (of_property_read_u32(np, "opencores,ip-clock-frequency", &val)) {
+#ifdef CONFIG_HAVE_CLK
+	i2c->clk = devm_clk_get(&pdev->dev, NULL);
+
+	if (!IS_ERR(i2c->clk)) {
+		int ret = clk_prepare_enable(i2c->clk);
+
+		if (ret) {
+			dev_err(&pdev->dev,
+				"clk_prepare_enable failed: %d\n", ret);
+			return ret;
+		}
+		i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000;
+		if (clock_frequency_present)
+			i2c->bus_clock_khz = clock_frequency / 1000;
+	} else
+#endif
+		if (of_property_read_u32(np, "opencores,ip-clock-frequency",
+					 &val)) {
 		if (!clock_frequency_present) {
 			dev_err(&pdev->dev,
 				"Missing required parameter 'opencores,ip-clock-frequency'\n");
@@ -477,6 +498,11 @@ static int ocores_i2c_remove(struct platform_device *pdev)
 	/* remove adapter & data */
 	i2c_del_adapter(&i2c->adap);
 
+#ifdef CONFIG_HAVE_CLK
+	if (!IS_ERR(i2c->clk))
+		clk_disable_unprepare(i2c->clk);
+#endif
+
 	return 0;
 }
 
@@ -489,6 +515,10 @@ static int ocores_i2c_suspend(struct device *dev)
 	/* make sure the device is disabled */
 	oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~(OCI2C_CTRL_EN|OCI2C_CTRL_IEN));
 
+#ifdef CONFIG_HAVE_CLK
+	if (!IS_ERR(i2c->clk))
+		clk_disable_unprepare(i2c->clk);
+#endif
 	return 0;
 }
 
@@ -496,6 +526,18 @@ static int ocores_i2c_resume(struct device *dev)
 {
 	struct ocores_i2c *i2c = dev_get_drvdata(dev);
 
+#ifdef CONFIG_HAVE_CLK
+	if (!IS_ERR(i2c->clk)) {
+		int ret = clk_prepare_enable(i2c->clk);
+
+		if (ret) {
+			dev_err(&pdev->dev,
+				"clk_prepare_enable failed: %d\n", ret);
+			return ret;
+		}
+		i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000;
+	}
+#endif
 	return ocores_init(dev, i2c);
 }
 
-- 
1.8.1.4

Re: [PATCH v2 2/2] i2c: i2c-ocores: add common clock support

[Wolfram Sang]

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

> -	if (of_property_read_u32(np, "opencores,ip-clock-frequency", &val)) {
> +#ifdef CONFIG_HAVE_CLK
> +	i2c->clk = devm_clk_get(&pdev->dev, NULL);
> +
> +	if (!IS_ERR(i2c->clk)) {
> +		int ret = clk_prepare_enable(i2c->clk);
> +
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"clk_prepare_enable failed: %d\n", ret);
> +			return ret;
> +		}
> +		i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000;
> +		if (clock_frequency_present)
> +			i2c->bus_clock_khz = clock_frequency / 1000;
> +	} else
> +#endif
> +		if (of_property_read_u32(np, "opencores,ip-clock-frequency",
> +					 &val)) {

This is not very readable (wrong indentation) and fragile. I'd suggest
to leave out #ifdeffery, the compiler will remove the code for !HAVE_CLK
because of the empty dummy functions[1]. Then, you can write properly
formatted code again.

[1] Well, it should if there wasn't a bug in that: https://lkml.org/lkml/2015/2/5/544

> +#ifdef CONFIG_HAVE_CLK
> +	if (!IS_ERR(i2c->clk)) {
> +		int ret = clk_prepare_enable(i2c->clk);
> +
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"clk_prepare_enable failed: %d\n", ret);
> +			return ret;
> +		}
> +		i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000;
> +	}
> +#endif

Ahem, there is a build error in here :(


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

Re: [PATCH v2 2/2] i2c: i2c-ocores: add common clock support

[Max Filippov]

On Thu, Feb 5, 2015 at 10:15 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
> > -     if (of_property_read_u32(np, "opencores,ip-clock-frequency", &val)) {
> > +#ifdef CONFIG_HAVE_CLK
> > +     i2c->clk = devm_clk_get(&pdev->dev, NULL);
> > +
> > +     if (!IS_ERR(i2c->clk)) {
> > +             int ret = clk_prepare_enable(i2c->clk);
> > +
> > +             if (ret) {
> > +                     dev_err(&pdev->dev,
> > +                             "clk_prepare_enable failed: %d\n", ret);
> > +                     return ret;
> > +             }
> > +             i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000;
> > +             if (clock_frequency_present)
> > +                     i2c->bus_clock_khz = clock_frequency / 1000;
> > +     } else
> > +#endif
> > +             if (of_property_read_u32(np, "opencores,ip-clock-frequency",
> > +                                      &val)) {
>
> This is not very readable (wrong indentation) and fragile. I'd suggest
> to leave out #ifdeffery, the compiler will remove the code for !HAVE_CLK
> because of the empty dummy functions[1]. Then, you can write properly
> formatted code again.
>
> [1] Well, it should if there wasn't a bug in that: https://lkml.org/lkml/2015/2/5/544

Ok, will fix.

> > +#ifdef CONFIG_HAVE_CLK
> > +     if (!IS_ERR(i2c->clk)) {
> > +             int ret = clk_prepare_enable(i2c->clk);
> > +
> > +             if (ret) {
> > +                     dev_err(&pdev->dev,
> > +                             "clk_prepare_enable failed: %d\n", ret);
> > +                     return ret;
> > +             }
> > +             i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000;
> > +     }
> > +#endif
>
> Ahem, there is a build error in here :(

Ooops, looks like I don't have CONFIG_PM_SLEEP in my test config.
Will fix. Sorry about that.

-- 
Thanks.
-- Max