i2c: mv64xxx: Fix clock resource for Armada 7K/8K
Subsystems (3)
v2 cover i2c: mv64xxx: Fix clock resource for Armada 7K/8K
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(-)
Signature · 1 line
2.15.1
PATCH v2 1/2 i2c: mv64xxx: Remove useless test before clk_disable_unprepare
The 2 functions called from clk_disable_unprepare() already check that the clock pointer is valid: no need to test it before calling it.
--- drivers/i2c/busses/i2c-mv64xxx.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/i2c/busses/i2c-mv64xxx.c +2 -6
@@ -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 ; }
Signature · 1 line
2.15.1
PATCH v2 2/2 i2c: mv64xxx: Fix clock resource by adding an optional bus clock
In reply to Gregory CLEMENTOn 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.
--- .../devicetree/bindings/i2c/i2c-mv64xxx.txt | 20 ++++++++++++++++++++ drivers/i2c/busses/i2c-mv64xxx.c | 12 +++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +20 -0
@@ -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>; + };
drivers/i2c/busses/i2c-mv64xxx.c +11 -1
@@ -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 ;
Signature · 1 line
2.15.1
PATCH v2 2/2 i2c: mv64xxx: Fix clock resource by adding an optional bus clock
Hello,
Quoted 17 lines · 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 <[email protected]>--- .../devicetree/bindings/i2c/i2c-mv64xxx.txt | 20 ++++++++++++++++++++ drivers/i2c/busses/i2c-mv64xxx.c | 12 +++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-)Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +0
-0
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt default frequency is 100kHz · line 34
Quoted 10 lines
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +7
-0@@ -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
Signature · 3 lines
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
In reply to Gregory CLEMENTQuoted 18 lines · 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 <[email protected]>--- .../devicetree/bindings/i2c/i2c-mv64xxx.txt | 20 ++++++++++++++++++++ drivers/i2c/busses/i2c-mv64xxx.c | 12 +++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-)Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +0
-0@@ -25,6 +25,15 @@ default frequency is 100kHz whenever you're using the "allwinner,sun6i-a31-i2c"
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt default frequency is 100kHz · line 35
Quoted 10 lines
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +8
-0compatible. + - 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
Hi Andrew,
Quoted 42 lines · Andrew Lunn
Quoted 18 lines · 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 <[email protected]>--- .../devicetree/bindings/i2c/i2c-mv64xxx.txt | 20 ++++++++++++++++++++ drivers/i2c/busses/i2c-mv64xxx.c | 12 +++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-)Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +0
-0@@ -25,6 +25,15 @@ default frequency is 100kHz whenever you're using the "allwinner,sun6i-a31-i2c"Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt default frequency is 100kHz · line 35
Quoted 10 lines
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +8
-0compatible. + - 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 },
Quoted 4 lines
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
Quoted 2 lines
Andrew
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com