Message ID | 1393950521-4173-2-git-send-email-maxime.ripard@free-electrons.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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!
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'
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
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.
> 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
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
> > 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).
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 --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)) {