diff mbox

[v3,1/5] i2c: mv64xxx: Add reset deassert call

Message ID 1393950521-4173-2-git-send-email-maxime.ripard@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maxime Ripard March 4, 2014, 4:28 p.m. UTC 
The Allwinner A31 SoC using that IP has a reset controller maintaining
it reset unless told otherwise.

Add some optional reset support to the driver.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 .../devicetree/bindings/i2c/i2c-mv64xxx.txt         |  1 +
 drivers/i2c/busses/Kconfig                          |  1 +
 drivers/i2c/busses/i2c-mv64xxx.c                    | 21 +++++++++++++++++++--
 3 files changed, 21 insertions(+), 2 deletions(-)

Comments

Wolfram Sang March 5, 2014, 4:31 p.m. UTC | #1 
On Tue, Mar 04, 2014 at 05:28:37PM +0100, Maxime Ripard wrote:
> The Allwinner A31 SoC using that IP has a reset controller maintaining
> it reset unless told otherwise.
> 
> Add some optional reset support to the driver.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Applied to for-next, thanks!
Russell King - ARM Linux March 7, 2014, 9:52 a.m. UTC | #2 
On Tue, Mar 04, 2014 at 05:28:37PM +0100, Maxime Ripard wrote:
> The Allwinner A31 SoC using that IP has a reset controller maintaining
> it reset unless told otherwise.
> 
> Add some optional reset support to the driver.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

This appears to be causing some build errors in Olof's next builder
for many of the ARM platforms which make use of this:

drivers/i2c/busses/i2c-mv64xxx.c:924: undefined reference to `reset_control_assert'
drivers/i2c/busses/i2c-mv64xxx.c:904: undefined reference to `reset_control_assert'
drivers/i2c/busses/i2c-mv64xxx.c:771: undefined reference to `devm_reset_control_get'
drivers/i2c/busses/i2c-mv64xxx.c:778: undefined reference to `reset_control_deassert'
Maxime Ripard March 7, 2014, 10:07 a.m. UTC | #3 
Hi Russell,

On Fri, Mar 07, 2014 at 09:52:23AM +0000, Russell King - ARM Linux wrote:
> On Tue, Mar 04, 2014 at 05:28:37PM +0100, Maxime Ripard wrote:
> > The Allwinner A31 SoC using that IP has a reset controller maintaining
> > it reset unless told otherwise.
> > 
> > Add some optional reset support to the driver.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> 
> This appears to be causing some build errors in Olof's next builder
> for many of the ARM platforms which make use of this:
> 
> drivers/i2c/busses/i2c-mv64xxx.c:924: undefined reference to `reset_control_assert'
> drivers/i2c/busses/i2c-mv64xxx.c:904: undefined reference to `reset_control_assert'
> drivers/i2c/busses/i2c-mv64xxx.c:771: undefined reference to `devm_reset_control_get'
> drivers/i2c/busses/i2c-mv64xxx.c:778: undefined reference to `reset_control_deassert'

The reset framework doesn't define its functions when its not
selected, and somehow I think it was not here. What's odd is that
there is an explicit select on RESET_CONTROLLER in the Kconfig. Maybe
it's the circular dependency issue that has been reported that cause
this and Wolfram sent a patch for: http://patchwork.ozlabs.org/patch/327573/

Maxime
Russell King - ARM Linux March 7, 2014, 10:34 a.m. UTC | #4 
On Fri, Mar 07, 2014 at 11:07:51AM +0100, Maxime Ripard wrote:
> Hi Russell,
> 
> On Fri, Mar 07, 2014 at 09:52:23AM +0000, Russell King - ARM Linux wrote:
> > On Tue, Mar 04, 2014 at 05:28:37PM +0100, Maxime Ripard wrote:
> > > The Allwinner A31 SoC using that IP has a reset controller maintaining
> > > it reset unless told otherwise.
> > > 
> > > Add some optional reset support to the driver.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > > Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > 
> > This appears to be causing some build errors in Olof's next builder
> > for many of the ARM platforms which make use of this:
> > 
> > drivers/i2c/busses/i2c-mv64xxx.c:924: undefined reference to `reset_control_assert'
> > drivers/i2c/busses/i2c-mv64xxx.c:904: undefined reference to `reset_control_assert'
> > drivers/i2c/busses/i2c-mv64xxx.c:771: undefined reference to `devm_reset_control_get'
> > drivers/i2c/busses/i2c-mv64xxx.c:778: undefined reference to `reset_control_deassert'
> 
> The reset framework doesn't define its functions when its not
> selected, and somehow I think it was not here. What's odd is that
> there is an explicit select on RESET_CONTROLLER in the Kconfig. Maybe
> it's the circular dependency issue that has been reported that cause
> this and Wolfram sent a patch for: http://patchwork.ozlabs.org/patch/327573/

If that patch has been taken, then yes, it will have caused the above -
because now we have Dove and Kirkwood platforms trying to build this
driver without RESET_CONTROLLER being set.

The problem with depending on RESET_CONTROLLER is that then these
platforms end up without their I2C controller - because there's nothing
which enables RESET_CONTROLLER in their configuration.

Since RESET_CONTROLLER is not required for those platforms, it really
should be optional - and I think the real fix is for the reset controller
support to provide stub functions.
Wolfram Sang March 7, 2014, 10:37 a.m. UTC | #5 
> Since RESET_CONTROLLER is not required for those platforms, it really
> should be optional - and I think the real fix is for the reset controller
> support to provide stub functions.

I agree; and I assumed it already does. Will resend a patch posted
earlier [1] to fix the issue. And still debugging why my build-tests
didn't get it :(

[1] https://lkml.org/lkml/2013/8/9/442
Chen-Yu Tsai March 7, 2014, 11:11 a.m. UTC | #6 
On Fri, Mar 7, 2014 at 6:34 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Mar 07, 2014 at 11:07:51AM +0100, Maxime Ripard wrote:
>> Hi Russell,
>>
>> On Fri, Mar 07, 2014 at 09:52:23AM +0000, Russell King - ARM Linux wrote:
>> > On Tue, Mar 04, 2014 at 05:28:37PM +0100, Maxime Ripard wrote:
>> > > The Allwinner A31 SoC using that IP has a reset controller maintaining
>> > > it reset unless told otherwise.
>> > >
>> > > Add some optional reset support to the driver.
>> > >
>> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> > > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> > > Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> >
>> > This appears to be causing some build errors in Olof's next builder
>> > for many of the ARM platforms which make use of this:
>> >
>> > drivers/i2c/busses/i2c-mv64xxx.c:924: undefined reference to `reset_control_assert'
>> > drivers/i2c/busses/i2c-mv64xxx.c:904: undefined reference to `reset_control_assert'
>> > drivers/i2c/busses/i2c-mv64xxx.c:771: undefined reference to `devm_reset_control_get'
>> > drivers/i2c/busses/i2c-mv64xxx.c:778: undefined reference to `reset_control_deassert'
>>
>> The reset framework doesn't define its functions when its not
>> selected, and somehow I think it was not here. What's odd is that
>> there is an explicit select on RESET_CONTROLLER in the Kconfig. Maybe
>> it's the circular dependency issue that has been reported that cause
>> this and Wolfram sent a patch for: http://patchwork.ozlabs.org/patch/327573/
>
> If that patch has been taken, then yes, it will have caused the above -
> because now we have Dove and Kirkwood platforms trying to build this
> driver without RESET_CONTROLLER being set.
>
> The problem with depending on RESET_CONTROLLER is that then these
> platforms end up without their I2C controller - because there's nothing
> which enables RESET_CONTROLLER in their configuration.
>
> Since RESET_CONTROLLER is not required for those platforms, it really
> should be optional - and I think the real fix is for the reset controller
> support to provide stub functions.

Philipp Zabel suggested that adding a _optional variant that provides stubs
and doesn't depend on RESET_CONTROLLER is probably better. This keeps the
compile time checks for drivers requiring it.

See: https://lkml.org/lkml/2014/1/10/220

I ended up dropping my patch though.


Cheers,
ChenYu
Wolfram Sang March 7, 2014, 11:18 a.m. UTC | #7 
> > Since RESET_CONTROLLER is not required for those platforms, it really
> > should be optional - and I think the real fix is for the reset controller
> > support to provide stub functions.
> 
> Philipp Zabel suggested that adding a _optional variant that provides stubs
> and doesn't depend on RESET_CONTROLLER is probably better. This keeps the
> compile time checks for drivers requiring it.
> 
> See: https://lkml.org/lkml/2014/1/10/220
> 
> I ended up dropping my patch though.

Thanks for the pointer. Well, looks like I need to revert the offending
i2c patches then until this issue is fixed? We can't have
RESET_CONTROLLER (circular dependency) and we can't skip it (build
failures).
Maxime Ripard March 7, 2014, 1:44 p.m. UTC | #8 
Hi,

On Fri, Mar 07, 2014 at 12:18:58PM +0100, Wolfram Sang wrote:
> 
> > > Since RESET_CONTROLLER is not required for those platforms, it really
> > > should be optional - and I think the real fix is for the reset controller
> > > support to provide stub functions.
> > 
> > Philipp Zabel suggested that adding a _optional variant that provides stubs
> > and doesn't depend on RESET_CONTROLLER is probably better. This keeps the
> > compile time checks for drivers requiring it.
> > 
> > See: https://lkml.org/lkml/2014/1/10/220
> > 
> > I ended up dropping my patch though.
> 
> Thanks for the pointer. Well, looks like I need to revert the offending
> i2c patches then until this issue is fixed? We can't have
> RESET_CONTROLLER (circular dependency) and we can't skip it (build
> failures).
> 

I just sent a fix in reply to your mail that should fix the issue
without having to revert the patches.

Thanks!
Maxime
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index 582b465..21062bc 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -16,6 +16,7 @@  Optional properties :
 
  - clock-frequency : Desired I2C bus clock frequency in Hz. If not set the
 default frequency is 100kHz
+ - resets          : phandle to the parent reset controller
 
 Examples:
 
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index f5ed031..70bcad9 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -528,6 +528,7 @@  config I2C_MPC
 config I2C_MV64XXX
 	tristate "Marvell mv64xxx I2C Controller"
 	depends on (MV64X60 || PLAT_ORION || ARCH_SUNXI)
+	select RESET_CONTROLLER
 	help
 	  If you say yes to this option, support will be included for the
 	  built-in I2C interface on the Marvell 64xxx line of host bridges.
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index d52d849..1bb69b6 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -17,6 +17,7 @@ 
 #include <linux/interrupt.h>
 #include <linux/mv643xx_i2c.h>
 #include <linux/platform_device.h>
+#include <linux/reset.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -148,6 +149,7 @@  struct mv64xxx_i2c_data {
 	bool			offload_enabled;
 /* 5us delay in order to avoid repeated start timing violation */
 	bool			errata_delay;
+	struct reset_control	*rstc;
 };
 
 static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -759,6 +761,16 @@  mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 	}
 	drv_data->irq = irq_of_parse_and_map(np, 0);
 
+	drv_data->rstc = devm_reset_control_get(dev, NULL);
+	if (IS_ERR(drv_data->rstc)) {
+		if (PTR_ERR(drv_data->rstc) == -EPROBE_DEFER) {
+			rc = -EPROBE_DEFER;
+			goto out;
+		}
+	} else {
+		reset_control_deassert(drv_data->rstc);
+	}
+
 	/* Its not yet defined how timeouts will be specified in device tree.
 	 * So hard code the value to 1 second.
 	 */
@@ -845,7 +857,7 @@  mv64xxx_i2c_probe(struct platform_device *pd)
 	}
 	if (drv_data->irq < 0) {
 		rc = -ENXIO;
-		goto exit_clk;
+		goto exit_reset;
 	}
 
 	drv_data->adapter.dev.parent = &pd->dev;
@@ -865,7 +877,7 @@  mv64xxx_i2c_probe(struct platform_device *pd)
 		dev_err(&drv_data->adapter.dev,
 			"mv64xxx: Can't register intr handler irq%d: %d\n",
 			drv_data->irq, rc);
-		goto exit_clk;
+		goto exit_reset;
 	} else if ((rc = i2c_add_numbered_adapter(&drv_data->adapter)) != 0) {
 		dev_err(&drv_data->adapter.dev,
 			"mv64xxx: Can't add i2c adapter, rc: %d\n", -rc);
@@ -876,6 +888,9 @@  mv64xxx_i2c_probe(struct platform_device *pd)
 
 exit_free_irq:
 	free_irq(drv_data->irq, drv_data);
+exit_reset:
+	if (pd->dev.of_node && !IS_ERR(drv_data->rstc))
+		reset_control_assert(drv_data->rstc);
 exit_clk:
 #if defined(CONFIG_HAVE_CLK)
 	/* Not all platforms have a clk */
@@ -894,6 +909,8 @@  mv64xxx_i2c_remove(struct platform_device *dev)
 
 	i2c_del_adapter(&drv_data->adapter);
 	free_irq(drv_data->irq, drv_data);
+	if (dev->dev.of_node && !IS_ERR(drv_data->rstc))
+		reset_control_assert(drv_data->rstc);
 #if defined(CONFIG_HAVE_CLK)
 	/* Not all platforms have a clk */
 	if (!IS_ERR(drv_data->clk)) {