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

Message ID 20180110170743.27082-3-gregory.clement@free-electrons.com
State Superseded
Headers show
Series
  • i2c: mv64xxx: Fix clock resource for Armada 7K/8K
Related show

Commit Message

Gregory CLEMENT Jan. 10, 2018, 5:07 p.m.
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(-)

Comments

Thomas Petazzoni Jan. 10, 2018, 5:14 p.m. | #1
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
Andrew Lunn Jan. 10, 2018, 8:32 p.m. | #2
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
Gregory CLEMENT Jan. 11, 2018, 8:16 a.m. | #3
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

Patch

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@11000 {
@@ -42,3 +51,14 @@  For the Armada XP:
 		interrupts = <29>;
 		clock-frequency = <100000>;
 	};
+
+For the Armada 7040:
+
+	i2c@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;