[PATCH v3 0/2] i2c: mv64xxx: Fix clock resource for Armada 7K/8K

[PATCH v3 0/2] i2c: mv64xxx: Fix clock resource for Armada 7K/8K

[Gregory CLEMENT]

Hi,

This short series fixes the way the clocks are used for the mv64xxx
controller embedded in the Marvell Armada 7K/8K SoCs. On these SoCs a
second one is needed in order to clock the registers. It was not
noticed until now because we relied on the bootloader and also because
the clock driver was wrong.

Thanks to this fix, it would be possible to fix the clock driver
without introducing a regression.

The first patch is just a small cleanup found when I wrote the main
patch.

Thanks,

Gregory

Changelog:
v1 -> v2:

 - Really add the binding documentation in the second patch, noticed
   by Thomas Petazzoni.

v2 -> v3

 - Fix typo in binding documentation reported by Thomas	Petazzoni
 - Use correct name for the axi clock, reported by Thomas Petazzoni

Gregory CLEMENT (2):
  i2c: mv64xxx: Remove useless test before clk_disable_unprepare
  i2c: mv64xxx: Fix clock resource by adding an optional bus clock

 .../devicetree/bindings/i2c/i2c-mv64xxx.txt          | 20 ++++++++++++++++++++
 drivers/i2c/busses/i2c-mv64xxx.c                     | 20 +++++++++++++-------
 2 files changed, 33 insertions(+), 7 deletions(-)

-- 
2.15.1

[PATCH v3 1/2] i2c: mv64xxx: Remove useless test before clk_disable_unprepare

[Gregory CLEMENT]

The 2 functions called from clk_disable_unprepare() already check that
the clock pointer is valid: no need to test it before calling it.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index a832c45276a4..f69066266faa 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -950,9 +950,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 exit_reset:
 	reset_control_assert(drv_data->rstc);
 exit_clk:
-	/* Not all platforms have a clk */
-	if (!IS_ERR(drv_data->clk))
-		clk_disable_unprepare(drv_data->clk);
+	clk_disable_unprepare(drv_data->clk);
 
 	return rc;
 }
@@ -965,9 +963,7 @@ mv64xxx_i2c_remove(struct platform_device *dev)
 	i2c_del_adapter(&drv_data->adapter);
 	free_irq(drv_data->irq, drv_data);
 	reset_control_assert(drv_data->rstc);
-	/* Not all platforms have a clk */
-	if (!IS_ERR(drv_data->clk))
-		clk_disable_unprepare(drv_data->clk);
+	clk_disable_unprepare(drv_data->clk);
 
 	return 0;
 }
-- 
2.15.1

[PATCH v3 2/2] i2c: mv64xxx: Fix clock resource by adding an optional bus clock

[Gregory CLEMENT]

On Armada 7K/8K we need to explicitly enable the bus clock. The bus clock
is optional because not all the SoCs need them but at least for Armada
7K/8K it is actually mandatory.

The binding documentation is updating accordingly.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 .../devicetree/bindings/i2c/i2c-mv64xxx.txt          | 20 ++++++++++++++++++++
 drivers/i2c/busses/i2c-mv64xxx.c                     | 12 +++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index 5c30026921ae..3d76bb19492f 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -25,6 +25,15 @@ default frequency is 100kHz
                      whenever you're using the "allwinner,sun6i-a31-i2c"
                      compatible.
 
+ - clocks:	   : pointers to the reference clocks for this device, the
+		     first one is the one used for the clock on the i2c bus,
+		     the second one is the clock used for the functional part
+		     of the controller
+
+ - clock-names	   : names of used clocks, mandatory if the second clock is
+		     used, the name must be "core", and "axi" (the latter is
+		     only for Armada 7K/8K).
+
 Examples:
 
 	i2c at 11000 {
@@ -42,3 +51,14 @@ For the Armada XP:
 		interrupts = <29>;
 		clock-frequency = <100000>;
 	};
+
+For the Armada 7040:
+
+	i2c at 701000 {
+		compatible = "marvell,mv78230-i2c";
+		reg = <0x701000 0x20>;
+		interrupts = <29>;
+		clock-frequency = <100000>;
+		clock-names = "core", "axi";
+		clocks = <&core_clock>, <&axi_clock>;
+	};
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index f69066266faa..cce37d8ecf41 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -135,6 +135,7 @@ struct mv64xxx_i2c_data {
 	u32			freq_m;
 	u32			freq_n;
 	struct clk              *clk;
+	struct clk              *axi_clk;
 	wait_queue_head_t	waitq;
 	spinlock_t		lock;
 	struct i2c_msg		*msg;
@@ -894,13 +895,20 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 	init_waitqueue_head(&drv_data->waitq);
 	spin_lock_init(&drv_data->lock);
 
-	/* Not all platforms have a clk */
+	/* Not all platforms have clocks */
 	drv_data->clk = devm_clk_get(&pd->dev, NULL);
 	if (IS_ERR(drv_data->clk) && PTR_ERR(drv_data->clk) == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 	if (!IS_ERR(drv_data->clk))
 		clk_prepare_enable(drv_data->clk);
 
+	drv_data->axi_clk = devm_clk_get(&pd->dev, "axi");
+	if (IS_ERR(drv_data->axi_clk) &&
+	    PTR_ERR(drv_data->axi_clk) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+	if (!IS_ERR(drv_data->axi_clk))
+		clk_prepare_enable(drv_data->axi_clk);
+
 	drv_data->irq = platform_get_irq(pd, 0);
 
 	if (pdata) {
@@ -950,6 +958,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 exit_reset:
 	reset_control_assert(drv_data->rstc);
 exit_clk:
+	clk_disable_unprepare(drv_data->axi_clk);
 	clk_disable_unprepare(drv_data->clk);
 
 	return rc;
@@ -963,6 +972,7 @@ mv64xxx_i2c_remove(struct platform_device *dev)
 	i2c_del_adapter(&drv_data->adapter);
 	free_irq(drv_data->irq, drv_data);
 	reset_control_assert(drv_data->rstc);
+	clk_disable_unprepare(drv_data->axi_clk);
 	clk_disable_unprepare(drv_data->clk);
 
 	return 0;
-- 
2.15.1

[PATCH v3 2/2] i2c: mv64xxx: Fix clock resource by adding an optional bus clock

[Maxime Ripard]

Hi,

On Fri, Jan 12, 2018 at 11:39:56AM +0100, Gregory CLEMENT wrote:
> On Armada 7K/8K we need to explicitly enable the bus clock. The bus clock
> is optional because not all the SoCs need them but at least for Armada
> 7K/8K it is actually mandatory.
> 
> The binding documentation is updating accordingly.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  .../devicetree/bindings/i2c/i2c-mv64xxx.txt          | 20 ++++++++++++++++++++
>  drivers/i2c/busses/i2c-mv64xxx.c                     | 12 +++++++++++-
>  2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> index 5c30026921ae..3d76bb19492f 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> @@ -25,6 +25,15 @@ default frequency is 100kHz
>                       whenever you're using the "allwinner,sun6i-a31-i2c"
>                       compatible.
>  
> + - clocks:	   : pointers to the reference clocks for this device, the
> +		     first one is the one used for the clock on the i2c bus,
> +		     the second one is the clock used for the functional part
> +		     of the controller

This documentation is confusing, as usually the functional clock is
the clock driving the bus, as opposed to the interface clock clocking
the bus interface.

> + - clock-names	   : names of used clocks, mandatory if the second clock is
> +		     used, the name must be "core", and "axi" (the latter is
> +		     only for Armada 7K/8K).
> +

Are you sure the i2c controller is on the AXI bus? It seems more
likely to be on an APB bus.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180112/4f2142f6/attachment.sig>

[PATCH v3 2/2] i2c: mv64xxx: Fix clock resource by adding an optional bus clock

[Gregory CLEMENT]

Hi Maxime,
 
 On ven., janv. 12 2018, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Hi,
>
> On Fri, Jan 12, 2018 at 11:39:56AM +0100, Gregory CLEMENT wrote:
>> On Armada 7K/8K we need to explicitly enable the bus clock. The bus clock
>> is optional because not all the SoCs need them but at least for Armada
>> 7K/8K it is actually mandatory.
>> 
>> The binding documentation is updating accordingly.
>> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>  .../devicetree/bindings/i2c/i2c-mv64xxx.txt          | 20 ++++++++++++++++++++
>>  drivers/i2c/busses/i2c-mv64xxx.c                     | 12 +++++++++++-
>>  2 files changed, 31 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> index 5c30026921ae..3d76bb19492f 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> @@ -25,6 +25,15 @@ default frequency is 100kHz
>>                       whenever you're using the "allwinner,sun6i-a31-i2c"
>>                       compatible.
>>  
>> + - clocks:	   : pointers to the reference clocks for this device, the
>> +		     first one is the one used for the clock on the i2c bus,
>> +		     the second one is the clock used for the functional part
>> +		     of the controller
>
> This documentation is confusing, as usually the functional clock is
> the clock driving the bus, as opposed to the interface clock clocking
> the bus interface.

I don't find it less confusing, for my point of view the interface of a
bus is the lines interfacing the bus with the devices on this bus. But I
don't mind modifying it, as I clearly state that the first clock is the
one used to generate the clock of the i2c bus, so we can change the
name.

>
>> + - clock-names	   : names of used clocks, mandatory if the second clock is
>> +		     used, the name must be "core", and "axi" (the latter is
>> +		     only for Armada 7K/8K).
>> +
>
> Are you sure the i2c controller is on the AXI bus? It seems more
> likely to be on an APB bus.

No I am not sure :)
Actually I don't have this information, so lets call it "reg" for
register clock.

Gregory

>
> Maxime
>
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v3 2/2] i2c: mv64xxx: Fix clock resource by adding an optional bus clock

[Wolfram Sang]

> > Are you sure the i2c controller is on the AXI bus? It seems more
> > likely to be on an APB bus.
> 
> No I am not sure :)
> Actually I don't have this information, so lets call it "reg" for
> register clock.

So, I assume a V4 is needed for this series.

BTW would be someone from FreeElectrons be up to maintaining this
driver?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180115/50eaf8f1/attachment.sig>