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

[PATCH v2 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.

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 v2 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 v2 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..a835b724c738 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_clk" (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 v2 2/2] i2c: mv64xxx: Fix clock resource by adding an optional bus clock

[Thomas Petazzoni]

Hello,

On Wed, 10 Jan 2018 18:07:43 +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..a835b724c738 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_clk" (the latter is

Spurious ":" at beginning of line. In addition, the name is "axi" not
"axi_clk", at least according to your code.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Andrew Lunn]

On Wed, Jan 10, 2018 at 06:07:43PM +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..a835b724c738 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_clk" (the latter is
> +		     only for Armada 7K/8K).

Hi Gregory

Are these two clocks related?

Ethernet on Dove needs two clocks enabled. 

static const struct clk_gating_soc_desc dove_gating_desc[] __initconst = {
       { "usb0", NULL, 0, 0 },
       { "usb1", NULL, 1, 0 },
       { "ge",	 "gephy", 2, 0 },
       { "sata", NULL, 3, 0 },

ge has a parent clock gephy. When you enable ge, the common clock code
walks up the tree of clocks, so will also turn on gephy.

Does this child/parent relationship exist with these i2c clocks?

     Andrew

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

[Gregory CLEMENT]

Hi Andrew,
 
 On mer., janv. 10 2018, Andrew Lunn <andrew@lunn.ch> wrote:

> On Wed, Jan 10, 2018 at 06:07:43PM +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..a835b724c738 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_clk" (the latter is
>> +		     only for Armada 7K/8K).
>
> Hi Gregory
>
> Are these two clocks related?
>
> Ethernet on Dove needs two clocks enabled. 
>
> static const struct clk_gating_soc_desc dove_gating_desc[] __initconst = {
>        { "usb0", NULL, 0, 0 },
>        { "usb1", NULL, 1, 0 },
>        { "ge",	 "gephy", 2, 0 },
>        { "sata", NULL, 3, 0 },
>
> ge has a parent clock gephy. When you enable ge, the common clock code
> walks up the tree of clocks, so will also turn on gephy.
>
> Does this child/parent relationship exist with these i2c clocks?

The child/parent relationship was the wrong assumption we made when we
wrote the clock driver for Armada 7K/8K. But now with more
documentation, it turned out that these 2 clocks are independant but
both are needed for i2c on Armada 7K/8K.

Gregory


>
>      Andrew

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