[PATCH] i2c-ocores: add common clock support

[PATCH] 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>
---
 .../devicetree/bindings/i2c/i2c-ocores.txt         |  4 ++-
 drivers/i2c/busses/i2c-ocores.c                    | 38 ++++++++++++++++++++--
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
index 1637c29..4b4c5a3 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-ocores.txt
@@ -4,7 +4,9 @@ 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
+- clk             : handle to bus clock (mutually exclusive with
+                    clock-frequency)
+- clock-frequency : frequency of bus clock in Hz (mutually exclusive with clk)
 - #address-cells  : should be <1>
 - #size-cells     : should be <0>
 
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 7249b5b..f97c5c8 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>
@@ -36,6 +37,7 @@ struct ocores_i2c {
 	int nmsgs;
 	int state; /* see STATE_ */
 	int clock_khz;
+	struct clk *clk;
 	void (*setreg)(struct ocores_i2c *i2c, int reg, u8 value);
 	u8 (*getreg)(struct ocores_i2c *i2c, int reg);
 };
@@ -320,9 +322,23 @@ 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;
+		struct clk *clk = devm_clk_get(&pdev->dev, NULL);
+
+		if (!IS_ERR(clk)) {
+			int ret = clk_prepare_enable(clk);
+
+			if (ret) {
+				dev_err(&pdev->dev,
+					"clk_prepare_enable failed: %d\n", ret);
+				return ret;
+			}
+			i2c->clk = clk;
+			val = clk_get_rate(clk);
+		} else {
+			dev_err(&pdev->dev,
+				"Missing required parameter 'clock-frequency'\n");
+			return -ENODEV;
+		}
 	}
 	i2c->clock_khz = val / 1000;
 
@@ -446,6 +462,9 @@ static int ocores_i2c_remove(struct platform_device *pdev)
 	/* remove adapter & data */
 	i2c_del_adapter(&i2c->adap);
 
+	if (i2c->clk)
+		clk_disable_unprepare(i2c->clk);
+
 	return 0;
 }
 
@@ -458,6 +477,9 @@ 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));
 
+	if (i2c->clk)
+		clk_disable_unprepare(i2c->clk);
+
 	return 0;
 }
 
@@ -465,6 +487,16 @@ static int ocores_i2c_resume(struct device *dev)
 {
 	struct ocores_i2c *i2c = dev_get_drvdata(dev);
 
+	if (i2c->clk) {
+		int ret = clk_prepare_enable(i2c->clk);
+
+		if (ret) {
+			dev_err(&pdev->dev,
+				"clk_prepare_enable failed: %d\n", ret);
+			return ret;
+		}
+	}
+
 	ocores_init(i2c);
 
 	return 0;
-- 
1.8.1.4

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

[Peter Korsgaard]

>>>>> "Max" == Max Filippov <jcmvbkbc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

 > 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Looks sensible to me - Thanks.

Acked-by: Peter Korsgaard <peter-+2lRwdCCLRT2eFz/2MeuCQ@public.gmane.org>

-- 
Bye, Peter Korsgaard

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

[Wolfram Sang]

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

> @@ -320,9 +322,23 @@ 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;
> +		struct clk *clk = devm_clk_get(&pdev->dev, NULL);
> +
> +		if (!IS_ERR(clk)) {
> +			int ret = clk_prepare_enable(clk);
> +
> +			if (ret) {
> +				dev_err(&pdev->dev,
> +					"clk_prepare_enable failed: %d\n", ret);
> +				return ret;
> +			}
> +			i2c->clk = clk;
> +			val = clk_get_rate(clk);
> +		} else {
> +			dev_err(&pdev->dev,
> +				"Missing required parameter 'clock-frequency'\n");
> +			return -ENODEV;
> +		}

Either NAK or I don't understand the logic here :) If a dts does NOT
have the bus-speed set by 'clock-frequency', then we take the value of
the clock assigned to this platform_device?

The usual thing to do when 'clock-frequency' is not set is to default to
100kHz.


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

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

[Max Filippov]

On Thu, Jan 22, 2015 at 5:45 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>> @@ -320,9 +322,23 @@ 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;
>> +             struct clk *clk = devm_clk_get(&pdev->dev, NULL);
>> +
>> +             if (!IS_ERR(clk)) {
>> +                     int ret = clk_prepare_enable(clk);
>> +
>> +                     if (ret) {
>> +                             dev_err(&pdev->dev,
>> +                                     "clk_prepare_enable failed: %d\n", ret);
>> +                             return ret;
>> +                     }
>> +                     i2c->clk = clk;
>> +                     val = clk_get_rate(clk);
>> +             } else {
>> +                     dev_err(&pdev->dev,
>> +                             "Missing required parameter 'clock-frequency'\n");
>> +                     return -ENODEV;
>> +             }
>
> Either NAK or I don't understand the logic here :) If a dts does NOT
> have the bus-speed set by 'clock-frequency', then we take the value of
> the clock assigned to this platform_device?
>
> The usual thing to do when 'clock-frequency' is not set is to default to
> 100kHz.

The clock here is not the i2c bus clock, but the clock input of the
controller. The function ocores_init initializes the prescaler register of
the controller so that the bus clock equals 100kHz (internal clock
runs at 500kHz):

static void ocores_init(struct ocores_i2c *i2c)
{
        int prescale;
...
        prescale = (i2c->clock_khz / (5*100)) - 1;
        oc_setreg(i2c, OCI2C_PRELOW, prescale & 0xff);
        oc_setreg(i2c, OCI2C_PREHIGH, prescale >> 8);
...

Then yes, when there's no 'clock-frequency' we take the value of the
clock assigned to this platform_device, which in turn comes from the
'clk' property.

-- 
Thanks.
-- Max

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

[Wolfram Sang]

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


> The clock here is not the i2c bus clock, but the clock input of the

Yes, what I would expect from a clk-property :)

> controller. The function ocores_init initializes the prescaler register of
> the controller so that the bus clock equals 100kHz (internal clock
> runs at 500kHz):

'clock-frequency' usually describes the I2C bus speed. So, for ocores,
it describes speed of the clock for the controller? That would be
ouch...


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

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

[Peter Korsgaard]

>>>>> "Wolfram" == Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> writes:

 >> The clock here is not the i2c bus clock, but the clock input of the

 > Yes, what I would expect from a clk-property :)

 >> controller. The function ocores_init initializes the prescaler register of
 >> the controller so that the bus clock equals 100kHz (internal clock
 >> runs at 500kHz):

 > 'clock-frequency' usually describes the I2C bus speed. So, for ocores,
 > it describes speed of the clock for the controller? That would be
 > ouch...

Indeed :/

Looking back in the history, the device tree patch originally used a
custom "clock_khz" property until some guy told him to use
clock-frequency ;)

https://lists.ozlabs.org/pipermail/devicetree-discuss/2010-November/003650.html

As far as I can see I wasn't CC'ed on that patch.

-- 
Bye, Peter Korsgaard

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

[Wolfram Sang]

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

On Thu, Jan 22, 2015 at 07:28:21PM +0100, Peter Korsgaard wrote:
> >>>>> "Wolfram" == Wolfram Sang <wsa@the-dreams.de> writes:
> 
>  >> The clock here is not the i2c bus clock, but the clock input of the
> 
>  > Yes, what I would expect from a clk-property :)
> 
>  >> controller. The function ocores_init initializes the prescaler register of
>  >> the controller so that the bus clock equals 100kHz (internal clock
>  >> runs at 500kHz):
> 
>  > 'clock-frequency' usually describes the I2C bus speed. So, for ocores,
>  > it describes speed of the clock for the controller? That would be
>  > ouch...
> 
> Indeed :/

Oh well, then let's fix this...

> Looking back in the history, the device tree patch originally used a
> custom "clock_khz" property until some guy told him to use
> clock-frequency ;)

I have sympathy for that guy. Very uncommon to specify IP core clocks
specifically in a dts ;)

My suggestion is:

1) if there is a clk node:
	- we get the clock rate via clock framework
	- "clock-frequency" is describing the bus speed as usual (Note
	  that parsing here can be as simple as checking for 100kHz only.
	  Although a seperate patch could probably easily add support for
	  other bus speeds to)

2?) a new binding is present to specify the IP clock speed:
	- is this needed? is somebody using the driver without CCF?
	- if so, the new binding is parsed and evaluated
	- I couldn't find an existing binding to specify a clock speed.
	  Please have a look, too. Otherwise we need to introduce sth
	  like "opencores,ip-clock-khz" probably.
	- "clock-frequency" is describing the bus speed as usual

3) only "clock-frequency" is present:
	- we keep the current behaviour to be backwards compatible.
	- driver should emit a warning to convert to new style
	- must be marked deprecated everywhere

The documentation should be updated accordingly.

Thoughts?


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

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

[Max Filippov]

On Thu, Jan 22, 2015 at 9:57 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> My suggestion is:
>
> 1) if there is a clk node:
>         - we get the clock rate via clock framework
>         - "clock-frequency" is describing the bus speed as usual (Note
>           that parsing here can be as simple as checking for 100kHz only.
>           Although a seperate patch could probably easily add support for
>           other bus speeds to)
>
> 2?) a new binding is present to specify the IP clock speed:
>         - is this needed? is somebody using the driver without CCF?
>         - if so, the new binding is parsed and evaluated
>         - I couldn't find an existing binding to specify a clock speed.
>           Please have a look, too. Otherwise we need to introduce sth
>           like "opencores,ip-clock-khz" probably.
>         - "clock-frequency" is describing the bus speed as usual
>
> 3) only "clock-frequency" is present:
>         - we keep the current behaviour to be backwards compatible.
>         - driver should emit a warning to convert to new style
>         - must be marked deprecated everywhere
>
> The documentation should be updated accordingly.
>
> Thoughts?

I can update my patch to do (1) and (3), leaving (2) to whoever may
need that.

-- 
Thanks.
-- Max

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

[Wolfram Sang]

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

On Thu, Jan 22, 2015 at 10:15:40PM +0300, Max Filippov wrote:
> On Thu, Jan 22, 2015 at 9:57 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> > My suggestion is:
> >
> > 1) if there is a clk node:
> >         - we get the clock rate via clock framework
> >         - "clock-frequency" is describing the bus speed as usual (Note
> >           that parsing here can be as simple as checking for 100kHz only.
> >           Although a seperate patch could probably easily add support for
> >           other bus speeds to)
> >
> > 2?) a new binding is present to specify the IP clock speed:
> >         - is this needed? is somebody using the driver without CCF?
> >         - if so, the new binding is parsed and evaluated
> >         - I couldn't find an existing binding to specify a clock speed.
> >           Please have a look, too. Otherwise we need to introduce sth
> >           like "opencores,ip-clock-khz" probably.
> >         - "clock-frequency" is describing the bus speed as usual
> >
> > 3) only "clock-frequency" is present:
> >         - we keep the current behaviour to be backwards compatible.
> >         - driver should emit a warning to convert to new style
> >         - must be marked deprecated everywhere
> >
> > The documentation should be updated accordingly.
> >
> > Thoughts?
> 
> I can update my patch to do (1) and (3), leaving (2) to whoever may
> need that.

Please implement (2) as well. Otherwise we would have documented
ambiguity of "clock-frequency" which is bad. It shouldn't be much code.


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

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

[Max Filippov]

On Thu, Jan 22, 2015 at 10:26 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> On Thu, Jan 22, 2015 at 10:15:40PM +0300, Max Filippov wrote:
>> On Thu, Jan 22, 2015 at 9:57 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>> > My suggestion is:
>> >
>> > 1) if there is a clk node:
>> >         - we get the clock rate via clock framework
>> >         - "clock-frequency" is describing the bus speed as usual (Note
>> >           that parsing here can be as simple as checking for 100kHz only.
>> >           Although a seperate patch could probably easily add support for
>> >           other bus speeds to)
>> >
>> > 2?) a new binding is present to specify the IP clock speed:
>> >         - is this needed? is somebody using the driver without CCF?
>> >         - if so, the new binding is parsed and evaluated
>> >         - I couldn't find an existing binding to specify a clock speed.
>> >           Please have a look, too. Otherwise we need to introduce sth
>> >           like "opencores,ip-clock-khz" probably.
>> >         - "clock-frequency" is describing the bus speed as usual
>> >
>> > 3) only "clock-frequency" is present:
>> >         - we keep the current behaviour to be backwards compatible.
>> >         - driver should emit a warning to convert to new style
>> >         - must be marked deprecated everywhere
>> >
>> > The documentation should be updated accordingly.
>> >
>> > Thoughts?
>>
>> I can update my patch to do (1) and (3), leaving (2) to whoever may
>> need that.
>
> Please implement (2) as well. Otherwise we would have documented
> ambiguity of "clock-frequency" which is bad. It shouldn't be much code.

There will be ambiguity if we want to maintain backwards compatibility,
so we're documenting it anyway. And since we're going to maintain it,
those who don't use CCF will still have option (3). Is there a point in
introducing (2) which currently does not exist and thus has no users?

-- 
Thanks.
-- Max

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

[Wolfram Sang]

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

On Thu, Jan 22, 2015 at 11:53:51PM +0300, Max Filippov wrote:
> On Thu, Jan 22, 2015 at 10:26 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> > On Thu, Jan 22, 2015 at 10:15:40PM +0300, Max Filippov wrote:
> >> On Thu, Jan 22, 2015 at 9:57 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> >> > My suggestion is:
> >> >
> >> > 1) if there is a clk node:
> >> >         - we get the clock rate via clock framework
> >> >         - "clock-frequency" is describing the bus speed as usual (Note
> >> >           that parsing here can be as simple as checking for 100kHz only.
> >> >           Although a seperate patch could probably easily add support for
> >> >           other bus speeds to)
> >> >
> >> > 2?) a new binding is present to specify the IP clock speed:
> >> >         - is this needed? is somebody using the driver without CCF?
> >> >         - if so, the new binding is parsed and evaluated
> >> >         - I couldn't find an existing binding to specify a clock speed.
> >> >           Please have a look, too. Otherwise we need to introduce sth
> >> >           like "opencores,ip-clock-khz" probably.
> >> >         - "clock-frequency" is describing the bus speed as usual
> >> >
> >> > 3) only "clock-frequency" is present:
> >> >         - we keep the current behaviour to be backwards compatible.
> >> >         - driver should emit a warning to convert to new style
> >> >         - must be marked deprecated everywhere
> >> >
> >> > The documentation should be updated accordingly.
> >> >
> >> > Thoughts?
> >>
> >> I can update my patch to do (1) and (3), leaving (2) to whoever may
> >> need that.
> >
> > Please implement (2) as well. Otherwise we would have documented
> > ambiguity of "clock-frequency" which is bad. It shouldn't be much code.
> 
> There will be ambiguity if we want to maintain backwards compatibility,
> so we're documenting it anyway. And since we're going to maintain it,
> those who don't use CCF will still have option (3). Is there a point in
> introducing (2) which currently does not exist and thus has no users?

Yes, because 3) is marked as deprecated "DON'T USE". If you don't
implement 2), then we force people to actually use the ambiguity!


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

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

[Max Filippov]

On Thu, Jan 22, 2015 at 9:57 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> 2?) a new binding is present to specify the IP clock speed:
>         - is this needed? is somebody using the driver without CCF?
>         - if so, the new binding is parsed and evaluated
>         - I couldn't find an existing binding to specify a clock speed.
>           Please have a look, too. Otherwise we need to introduce sth
>           like "opencores,ip-clock-khz" probably.
>         - "clock-frequency" is describing the bus speed as usual

ePAPR document says that clock-frequency 'specifies the frequency
of a clock in Hz', not identifying which clock. Outside i2c drivers
subtree it is commonly used to specify input clock frequency of IP
blocks, even for bus controllers, e.g. SPI.

-- 
Thanks.
-- Max

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

[Wolfram Sang]

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

On Thu, Jan 22, 2015 at 11:36:25PM +0300, Max Filippov wrote:
> On Thu, Jan 22, 2015 at 9:57 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > 2?) a new binding is present to specify the IP clock speed:
> >         - is this needed? is somebody using the driver without CCF?
> >         - if so, the new binding is parsed and evaluated
> >         - I couldn't find an existing binding to specify a clock speed.
> >           Please have a look, too. Otherwise we need to introduce sth
> >           like "opencores,ip-clock-khz" probably.
> >         - "clock-frequency" is describing the bus speed as usual
> 
> ePAPR document says that clock-frequency 'specifies the frequency
> of a clock in Hz', not identifying which clock. Outside i2c drivers
> subtree it is commonly used to specify input clock frequency of IP
> blocks, even for bus controllers, e.g. SPI.

I know; it was established in I2C this way before I took over.
I don't think it is a good idea to change that.


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

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

[Peter Korsgaard]

>>>>> "Wolfram" == Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> writes:

 >> @@ -320,9 +322,23 @@ 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;
 >> +		struct clk *clk = devm_clk_get(&pdev->dev, NULL);
 >> +
 >> +		if (!IS_ERR(clk)) {
 >> +			int ret = clk_prepare_enable(clk);
 >> +
 >> +			if (ret) {
 >> +				dev_err(&pdev->dev,
 >> +					"clk_prepare_enable failed: %d\n", ret);
 >> +				return ret;
 >> +			}
 >> +			i2c->clk = clk;
 >> +			val = clk_get_rate(clk);
 >> +		} else {
 >> +			dev_err(&pdev->dev,
 >> +				"Missing required parameter 'clock-frequency'\n");
 >> +			return -ENODEV;
 >> +		}

 > Either NAK or I don't understand the logic here :) If a dts does NOT
 > have the bus-speed set by 'clock-frequency', then we take the value of
 > the clock assigned to this platform_device?

 > The usual thing to do when 'clock-frequency' is not set is to default to
 > 100kHz.

The confusion comes from the fact that the device tree bindings uses
clock-frequency for the clock frequency of the IP core and NOT for the
i2c bus frequency. This dates back to when the bindings where added
(049bb69d82e5f7f356) :/

The driver is currently hardcoded to use a 100KHz i2c bus clock (see
ocoores_init()).

-- 
Bye, Peter Korsgaard