Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On 07/18/2013 09:23 PM, Shawn Guo wrote: On Thu, Jul 18, 2013 at 11:45:29AM -0700, Olof Johansson wrote: ... You're spending an awful lot of energy arguing over this, but nobody has actually shown data of how much deferral is happening -- and that it's a real problem. Until someone does so there's no reason to change it from the default module init level at all, I'd say. On imx6qdl-sabreauto board, there are 3 MAX7310 units as IO expanders to output 3 x 8 = 24 GPIOs. 18 of them are used for resets, enables and pin steering for various peripherals on the board. BACKLITE_ON SAT_SHUTDN_B CPU_PER_RST_B MAIN_PER_RST_B IPOD_RST_B MLB_RST_B SSI_STEERING GPS_RST_B GPS_PWREN VIDEO_ADC_PWRDN_B ENET_CAN1_STEER EIMD30_BTUART3_STEER CAN_STBY CAN_EN USB_H1_PWR USB_OTG_PWR_ON SAT_RST_B NAND_BT_WIFI_STEER Most of these GPIOs need to set up properly at client device driver's probe stage - module_init() time. So if we have all these service providers resets/enables/steering API registered at module_init() too, the probes of all these client device drivers stand a good chance to be deferred. That sounds like it /could/ well be an issue. Do you have all that actually hooked up through device tree so you can demonstrate the amount of deferred probing that /actually/ happens though? In order to push the probe order in the right direction, I would personally prefer to see some kind of explicit hinting or outright representation of probe order in the DT. If we fiddle with initcall levels to do this, (a) it won't always work; what may be optimal for one board/SoC may not be optimal for another, and with multi-platform kernels, we have to take a broader view, (b) the only beneficiary is Linux; if we add information to DT for this, every OS could benefit in the same way. My view is that when one HW object depends on another (e.g. it's reset by some other module, sends an interrupt to another module, is affected by a pin controller module, etc.), that really is a HW issue, and hence is appropriate to represent in device tree. In fact, we already do represent the dependencies in device tree; e.g. a foo-gpios/foo-reset/... property already indicates this. The issue is that there's no standard naming/structure across all types of DT binding (GPIO, reset, ...), and hence the only way to parse these dependencies is to do exactly what deferred probe is doing; try probing each driver, which then encompasses all device-specific and subsystem-specific naming and DT structure in its actions, and if it fails, then defer probe. If we had some explicit information re: dependencies in the DT, in an entirely standard format, perhaps the device core or similar could parse this and not even attempt probe until the relevant devices had completed probe. If the information turned out to be bogus (e.g. loops, missing drivers), we could always fall back to pure deferred probe later in the boot sequence. Or perhaps we should revisit whether DT node order should be defined as having a guaranteed influence on probe order. We could enforce that the source order has no influence, but then run a post-processing tool that finds all the dependencies from the phandles in the DT, and then re-sorts the DTB to get the probing order right? That would probably require every single driver to be module_initcall, or to ignore driver registration order and only probe devices in the order they appear in DT. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
Hi, On Thu, Jul 18, 2013 at 4:25 AM, Pavel Machek pa...@denx.de wrote: It sound to me like keeping ammount of -EPROBE_DEFER to minimum is still preferred. Hand-crafting initcall level ordering of various drivers and subsystem is probably an even greater evil though. We've done it in the past, but now that we have deferred probe we have the option of not having to do it every time, such as this. You're spending an awful lot of energy arguing over this, but nobody has actually shown data of how much deferral is happening -- and that it's a real problem. Until someone does so there's no reason to change it from the default module init level at all, I'd say. -Olof ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On Wed, Jul 17, 2013 at 9:57 AM, Stephen Warren swar...@wwwdotorg.org wrote: On 07/16/2013 09:02 PM, Shawn Guo wrote: On Tue, Jul 16, 2013 at 09:45:43AM -0600, Stephen Warren wrote: Registering the driver earlier won't cause any bugs. However, it's not the correct approach. Deferred probe /is/ the approach for assuring correct dependencies between drivers. It works and should be used. There are not enough initcall levels to play games using initcalls and solve all the issues, and the ordering requirements vary board-to-board. Deferred probe at runtime handles this without having to manually place all the drivers into specific initcall levels, and dynamically adjusts to board differences, since it all happens automatically at run-time. I do not quite follow the argument here. I agree with you that deferred probe is the approach to solve dependencies. But it does not necessarily mean that initcall can not be used to help it save some nasty or nested deferring. Deferred probe and initcalls are not two mutually exclusive mechanisms but two which can help each other. My understanding is that deferred probe was implemented specifically to avoid having to, or allowing, the use of initcall levels to determine probe order. Correct. Futzing around with initcalls may optimize particular use cases, but it isn't complete. I've been pushing for all drivers to use module_initcall() unless there is a very strong reason to do otherwise. Premature optimization (without measuring time consumed) is not a strong justification. g. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On Thu, Jul 18, 2013 at 4:25 AM, Pavel Machek pa...@denx.de wrote: Hi! I do not quite follow the argument here. I agree with you that deferred probe is the approach to solve dependencies. But it does not necessarily mean that initcall can not be used to help it save some nasty or nested deferring. Deferred probe and initcalls are not two mutually exclusive mechanisms but two which can help each other. My understanding is that deferred probe was implemented specifically to avoid having to, or allowing, the use of initcall levels to determine probe order. However, if someone closely associated with the implementation of deferred probe (e.g. Grant, or a device core maintainer) is willing to step up and say I'm wrong, I'll drop my objection. AFAIR, defered probing is known to be a hack by its authors. We should still try to get initcall levels right... I don't /think/ it was the concept of deferred probe that was considered hacky, but perhaps just the first proof-of-concept implementation, and any hackiness was presumably solved before the feature was merged. Well... http://lwn.net/Articles/485194/ From: Grant Likely grant.lik...@secretlab.ca To: linux-ker...@vger.kernel.org Subject:[PATCH] drivercore: Add driver probe deferral mechanism Date: Mon, 5 Mar 2012 08:47:41 -0700 Message-ID: 1330962461-9061-1-git-send-email-grant.lik...@secretlab.ca ... I know that not everybody is happy with this approach, but I've yet to see a better alternative. However, it is *really easy* to find all the users of deferred probe since any user must return -EPROBE_DEFER explicitly. If/when a better approach is found, all the users will be easy to find and modify. ... It sound to me like keeping ammount of -EPROBE_DEFER to minimum is still preferred. That wasn't the point I was trying to make in the above thread. My point was that -EPROBE_DEFER is the best option that we currently have to get rid of the initcall ordering madness. I'm all ears if someone comes up with a simple and inexpensive alternative that can take into account dependencies between devices. g. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On Thu, Jul 18, 2013 at 11:45:29AM -0700, Olof Johansson wrote: Hi, On Thu, Jul 18, 2013 at 4:25 AM, Pavel Machek pa...@denx.de wrote: It sound to me like keeping ammount of -EPROBE_DEFER to minimum is still preferred. Hand-crafting initcall level ordering of various drivers and subsystem is probably an even greater evil though. We've done it in the past, but now that we have deferred probe we have the option of not having to do it every time, such as this. You're spending an awful lot of energy arguing over this, but nobody has actually shown data of how much deferral is happening -- and that it's a real problem. Until someone does so there's no reason to change it from the default module init level at all, I'd say. On imx6qdl-sabreauto board, there are 3 MAX7310 units as IO expanders to output 3 x 8 = 24 GPIOs. 18 of them are used for resets, enables and pin steering for various peripherals on the board. BACKLITE_ON SAT_SHUTDN_B CPU_PER_RST_B MAIN_PER_RST_B IPOD_RST_B MLB_RST_B SSI_STEERING GPS_RST_B GPS_PWREN VIDEO_ADC_PWRDN_B ENET_CAN1_STEER EIMD30_BTUART3_STEER CAN_STBY CAN_EN USB_H1_PWR USB_OTG_PWR_ON SAT_RST_B NAND_BT_WIFI_STEER Most of these GPIOs need to set up properly at client device driver's probe stage - module_init() time. So if we have all these service providers resets/enables/steering API registered at module_init() too, the probes of all these client device drivers stand a good chance to be deferred. Shawn ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
Am Dienstag, den 16.07.2013, 09:47 -0600 schrieb Stephen Warren: On 07/16/2013 12:51 AM, Shawn Guo wrote: On Tue, Jul 16, 2013 at 09:50:42AM +0800, Shawn Guo wrote: Hi Philipp, On Thu, May 30, 2013 at 11:09:00AM +0200, Philipp Zabel wrote: This driver implements a reset controller device that toggle a gpio connected to a reset pin of a peripheral IC. The delay between assertion and de-assertion of the reset signal can be configured via device tree. Signed-off-by: Philipp Zabel p.za...@pengutronix.de Reviewed-by: Stephen Warren swar...@nvidia.com I see this patch is very useful, as GPIOs are widely used to reset components/devices on board. But I do not find the patch in v3.11-rc1. What's your plan about it? Also, I'm wondering if we should register the driver a little bit early. Please see the following patch. If it makes sense to you, I can send the patch to you, or you can just quash it into yours. And here is another change request. diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c - gpio_set_value(drvdata-gpio, value); + if (gpio_cansleep(drvdata-gpio)) + gpio_set_value_cansleep(drvdata-gpio, value); + else + gpio_set_value(drvdata-gpio, value); That's not right. Calling gpio_set_value() v.s. gpio_set_value_cansleep() should be based on the properties of the calling context, not the GPIO being controlled. In other words, if it's permissible to call gpio_set_value_cansleep() at this point in the code, simply always call that, and remove the conditional logic. In which case I'd say let's just call the _cansleep variant here unconditionally and declare that reset_control_assert/deassert() may sleep, just as reset_control_reset() has to anyway. regards Philipp ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On 07/16/2013 09:02 PM, Shawn Guo wrote: On Tue, Jul 16, 2013 at 09:45:43AM -0600, Stephen Warren wrote: Registering the driver earlier won't cause any bugs. However, it's not the correct approach. Deferred probe /is/ the approach for assuring correct dependencies between drivers. It works and should be used. There are not enough initcall levels to play games using initcalls and solve all the issues, and the ordering requirements vary board-to-board. Deferred probe at runtime handles this without having to manually place all the drivers into specific initcall levels, and dynamically adjusts to board differences, since it all happens automatically at run-time. I do not quite follow the argument here. I agree with you that deferred probe is the approach to solve dependencies. But it does not necessarily mean that initcall can not be used to help it save some nasty or nested deferring. Deferred probe and initcalls are not two mutually exclusive mechanisms but two which can help each other. My understanding is that deferred probe was implemented specifically to avoid having to, or allowing, the use of initcall levels to determine probe order. However, if someone closely associated with the implementation of deferred probe (e.g. Grant, or a device core maintainer) is willing to step up and say I'm wrong, I'll drop my objection. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On 07/17/2013 03:38 PM, Pavel Machek wrote: On Wed 2013-07-17 10:57:08, Stephen Warren wrote: On 07/16/2013 09:02 PM, Shawn Guo wrote: On Tue, Jul 16, 2013 at 09:45:43AM -0600, Stephen Warren wrote: Registering the driver earlier won't cause any bugs. However, it's not the correct approach. Deferred probe /is/ the approach for assuring correct dependencies between drivers. It works and should be used. There are not enough initcall levels to play games using initcalls and solve all the issues, and the ordering requirements vary board-to-board. Deferred probe at runtime handles this without having to manually place all the drivers into specific initcall levels, and dynamically adjusts to board differences, since it all happens automatically at run-time. I do not quite follow the argument here. I agree with you that deferred probe is the approach to solve dependencies. But it does not necessarily mean that initcall can not be used to help it save some nasty or nested deferring. Deferred probe and initcalls are not two mutually exclusive mechanisms but two which can help each other. My understanding is that deferred probe was implemented specifically to avoid having to, or allowing, the use of initcall levels to determine probe order. However, if someone closely associated with the implementation of deferred probe (e.g. Grant, or a device core maintainer) is willing to step up and say I'm wrong, I'll drop my objection. AFAIR, defered probing is known to be a hack by its authors. We should still try to get initcall levels right... I don't /think/ it was the concept of deferred probe that was considered hacky, but perhaps just the first proof-of-concept implementation, and any hackiness was presumably solved before the feature was merged. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On Tue, Jul 16, 2013 at 09:50:42AM +0800, Shawn Guo wrote: Hi Philipp, On Thu, May 30, 2013 at 11:09:00AM +0200, Philipp Zabel wrote: This driver implements a reset controller device that toggle a gpio connected to a reset pin of a peripheral IC. The delay between assertion and de-assertion of the reset signal can be configured via device tree. Signed-off-by: Philipp Zabel p.za...@pengutronix.de Reviewed-by: Stephen Warren swar...@nvidia.com I see this patch is very useful, as GPIOs are widely used to reset components/devices on board. But I do not find the patch in v3.11-rc1. What's your plan about it? Also, I'm wondering if we should register the driver a little bit early. Please see the following patch. If it makes sense to you, I can send the patch to you, or you can just quash it into yours. And here is another change request. Shawn 8--- From 822a0c4fb7c99b371844ba7e957affcaa8cb1cfe Mon Sep 17 00:00:00 2001 From: Shawn Guo shawn@linaro.org Date: Sun, 14 Jul 2013 20:28:05 +0800 Subject: [PATCH] ENGR00269945: reset: handle cansleep case in gpio-reset Some gpio reset may be backed by a gpio that can sleep, e.g. pca953x gpio output. For such gpio, gpio_set_value_cansleep() should be called. Otherwise, the WARN_ON(chip-can_sleep) in gpiod_set_value() will be hit. Add a gpio_cansleep() check to handle cansleep gpio. Signed-off-by: Shawn Guo shawn@linaro.org --- drivers/reset/gpio-reset.c |5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c index 5d2515a..4f372f9 100644 --- a/drivers/reset/gpio-reset.c +++ b/drivers/reset/gpio-reset.c @@ -32,7 +32,10 @@ static void gpio_reset_set(struct reset_controller_dev *rcdev, int asserted) if (drvdata-active_low) value = !value; - gpio_set_value(drvdata-gpio, value); + if (gpio_cansleep(drvdata-gpio)) + gpio_set_value_cansleep(drvdata-gpio, value); + else + gpio_set_value(drvdata-gpio, value); } static int gpio_reset(struct reset_controller_dev *rcdev, unsigned long id) -- 1.7.9.5 ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
Hi Shawn, Am Dienstag, den 16.07.2013, 12:10 +0800 schrieb Shawn Guo: On Mon, Jul 15, 2013 at 09:35:52PM -0600, Stephen Warren wrote: It's a little bit late to register gpio-reset driver at module_init time, because gpio-reset provides reset control via gpio for other devices which are mostly probed at module_init time too. And it becomes even worse, when the gpio comes from IO expander on I2C bus, e.g. pca953x. In that case, gpio-reset needs to be ready before I2C bus driver which is generally ready at subsys_initcall time. Let's register gpio-reset driver in arch_initcall() to have it ready early enough. There's no need for the reset driver to be registered before its users; the users of the reset GPIO will simply have their probe deferred until the reset controller is available, and then everything will work out just fine. The defer probe mechanism is not used here, because a reset controller driver should be reasonably registered early than other devices. More importantly, defer probe doe not help in some nasty cases, e.g. the gpio-pca953x device itself needs a reset from gpio-reset driver to start working. That should work fine with deferred probe. I should probably rework the commit log. But I do not see a problem to register gpio-reset driver a little bit earlier. On the other hand, if we reply on deferred probe, many drivers probe could be deferred. For example, on my system, the gpio-pca953x on I2C bus works as a GPIO controller and provides resets to many board level components. Deferring probe of gpio-pca953x on I2C bus means every single probe of all these components gets deferred. IMO, this situation should be reasonably avoided. so you want to have gpio-reset probed at arch_initcall time and you have gpio-pca953x probed at subsys_initcall time. Won't then all gpio-reset devices that use gpios on pca953x to reset other peripherals need to be deferred? regards Philipp ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
Am Dienstag, den 16.07.2013, 14:51 +0800 schrieb Shawn Guo: On Tue, Jul 16, 2013 at 09:50:42AM +0800, Shawn Guo wrote: Hi Philipp, On Thu, May 30, 2013 at 11:09:00AM +0200, Philipp Zabel wrote: This driver implements a reset controller device that toggle a gpio connected to a reset pin of a peripheral IC. The delay between assertion and de-assertion of the reset signal can be configured via device tree. Signed-off-by: Philipp Zabel p.za...@pengutronix.de Reviewed-by: Stephen Warren swar...@nvidia.com I see this patch is very useful, as GPIOs are widely used to reset components/devices on board. But I do not find the patch in v3.11-rc1. What's your plan about it? Also, I'm wondering if we should register the driver a little bit early. Please see the following patch. If it makes sense to you, I can send the patch to you, or you can just quash it into yours. And here is another change request. Looks good to me. I can squash it into the original patch and resend if you like. regards Philipp Shawn 8--- From 822a0c4fb7c99b371844ba7e957affcaa8cb1cfe Mon Sep 17 00:00:00 2001 From: Shawn Guo shawn@linaro.org Date: Sun, 14 Jul 2013 20:28:05 +0800 Subject: [PATCH] ENGR00269945: reset: handle cansleep case in gpio-reset Some gpio reset may be backed by a gpio that can sleep, e.g. pca953x gpio output. For such gpio, gpio_set_value_cansleep() should be called. Otherwise, the WARN_ON(chip-can_sleep) in gpiod_set_value() will be hit. Add a gpio_cansleep() check to handle cansleep gpio. Signed-off-by: Shawn Guo shawn@linaro.org --- drivers/reset/gpio-reset.c |5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c index 5d2515a..4f372f9 100644 --- a/drivers/reset/gpio-reset.c +++ b/drivers/reset/gpio-reset.c @@ -32,7 +32,10 @@ static void gpio_reset_set(struct reset_controller_dev *rcdev, int asserted) if (drvdata-active_low) value = !value; - gpio_set_value(drvdata-gpio, value); + if (gpio_cansleep(drvdata-gpio)) + gpio_set_value_cansleep(drvdata-gpio, value); + else + gpio_set_value(drvdata-gpio, value); } static int gpio_reset(struct reset_controller_dev *rcdev, unsigned long id) ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
Hi Stephen, Am Montag, den 15.07.2013, 21:35 -0600 schrieb Stephen Warren: On 07/15/2013 07:50 PM, Shawn Guo wrote: Hi Philipp, On Thu, May 30, 2013 at 11:09:00AM +0200, Philipp Zabel wrote: This driver implements a reset controller device that toggle a gpio connected to a reset pin of a peripheral IC. The delay between assertion and de-assertion of the reset signal can be configured via device tree. Signed-off-by: Philipp Zabel p.za...@pengutronix.de Reviewed-by: Stephen Warren swar...@nvidia.com I see this patch is very useful, as GPIOs are widely used to reset components/devices on board. But I do not find the patch in v3.11-rc1. What's your plan about it? Also, I'm wondering if we should register the driver a little bit early. Please see the following patch. If it makes sense to you, I can send the patch to you, or you can just quash it into yours. Shawn ---8 From 2f8ce9e5d6525b98f3828b707458e83fabb39d50 Mon Sep 17 00:00:00 2001 From: Shawn Guo shawn@linaro.org Date: Sun, 14 Jul 2013 20:41:00 +0800 Subject: [PATCH] ENGR00269945: reset: register gpio-reset driver in arch_initcall It's a little bit late to register gpio-reset driver at module_init time, because gpio-reset provides reset control via gpio for other devices which are mostly probed at module_init time too. And it becomes even worse, when the gpio comes from IO expander on I2C bus, e.g. pca953x. In that case, gpio-reset needs to be ready before I2C bus driver which is generally ready at subsys_initcall time. Let's register gpio-reset driver in arch_initcall() to have it ready early enough. There's no need for the reset driver to be registered before its users; the users of the reset GPIO will simply have their probe deferred until the reset controller is available, and then everything will work out just fine. The defer probe mechanism is not used here, because a reset controller driver should be reasonably registered early than other devices. More importantly, defer probe doe not help in some nasty cases, e.g. the gpio-pca953x device itself needs a reset from gpio-reset driver to start working. That should work fine with deferred probe. Deferred probing is fine, but it'd be nice to keep the probe deferral loops to a minimum where possible and/or reasonable. regards Philipp ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On Tue, Jul 16, 2013 at 11:51:19AM +0200, Philipp Zabel wrote: Looks good to me. I can squash it into the original patch and resend if you like. Yes, please. Thanks. Shawn ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On Tue, Jul 16, 2013 at 11:49:26AM +0200, Philipp Zabel wrote: so you want to have gpio-reset probed at arch_initcall time and you have gpio-pca953x probed at subsys_initcall time. Won't then all gpio-reset devices that use gpios on pca953x to reset other peripherals need to be deferred? Yes, they will be deferred, but they will be probed and ready at subsys_initcall time when gpio-pca953x probes. This is still good since most of component device drivers probes at module_init time. Shawn ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On 07/15/2013 10:10 PM, Shawn Guo wrote: On Mon, Jul 15, 2013 at 09:35:52PM -0600, Stephen Warren wrote: It's a little bit late to register gpio-reset driver at module_init time, because gpio-reset provides reset control via gpio for other devices which are mostly probed at module_init time too. And it becomes even worse, when the gpio comes from IO expander on I2C bus, e.g. pca953x. In that case, gpio-reset needs to be ready before I2C bus driver which is generally ready at subsys_initcall time. Let's register gpio-reset driver in arch_initcall() to have it ready early enough. There's no need for the reset driver to be registered before its users; the users of the reset GPIO will simply have their probe deferred until the reset controller is available, and then everything will work out just fine. The defer probe mechanism is not used here, because a reset controller driver should be reasonably registered early than other devices. More importantly, defer probe doe not help in some nasty cases, e.g. the gpio-pca953x device itself needs a reset from gpio-reset driver to start working. That should work fine with deferred probe. I should probably rework the commit log. But I do not see a problem to register gpio-reset driver a little bit earlier. Registering the driver earlier won't cause any bugs. However, it's not the correct approach. Deferred probe /is/ the approach for assuring correct dependencies between drivers. It works and should be used. There are not enough initcall levels to play games using initcalls and solve all the issues, and the ordering requirements vary board-to-board. Deferred probe at runtime handles this without having to manually place all the drivers into specific initcall levels, and dynamically adjusts to board differences, since it all happens automatically at run-time. Consider a SoC that has: * An external PMIC, which the CPU has to communicate with to enable power to all SoC components outside the CPU and a single I2C instance dedicated to communicating with the PMIC. * An on-SoC reset controller (for on-SoC) modules that resets other on-SoC I2C controllers * An on-SoC I2C controller which communicates with a GPIO expander * One of the GPIOs on that expander is the reset GPIO for some other device connected by I2C What initcall levels are you going to use for all the drivers (PM-related I2C, PMIC, on-SoC reset controller, secondary I2C controller, GPIO expander IC, GPIO-reset device, the final device affected by the GPIO reset controller). Now wonder whether that same order is going to be suitable for every single other board. That's quite unlikely... On the other hand, if we reply on deferred probe, many drivers probe could be deferred. For example, on my system, the gpio-pca953x on I2C bus works as a GPIO controller and provides resets to many board level components. Deferring probe of gpio-pca953x on I2C bus means every single probe of all these components gets deferred. IMO, this situation should be reasonably avoided. Yes, deferred probe will happen. If we want to solve that, I believe we need a more generic solution specifically targeted at that, rather that playing games with initcalls. (It'd be nice if each DT node declared all its resources in a way the DT core could parse them and determine the dependency graph itself, without having to write code in all drivers to extract that information from DT. This would need a standardized resource representation in DT, and as such would mean throwing out all the bindings and starting again... Perhaps some parallel set of properties could be invented to hint at the order, but still fall back to deferred probe where that information was missing or inaccurate.) ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On 07/16/2013 12:51 AM, Shawn Guo wrote: On Tue, Jul 16, 2013 at 09:50:42AM +0800, Shawn Guo wrote: Hi Philipp, On Thu, May 30, 2013 at 11:09:00AM +0200, Philipp Zabel wrote: This driver implements a reset controller device that toggle a gpio connected to a reset pin of a peripheral IC. The delay between assertion and de-assertion of the reset signal can be configured via device tree. Signed-off-by: Philipp Zabel p.za...@pengutronix.de Reviewed-by: Stephen Warren swar...@nvidia.com I see this patch is very useful, as GPIOs are widely used to reset components/devices on board. But I do not find the patch in v3.11-rc1. What's your plan about it? Also, I'm wondering if we should register the driver a little bit early. Please see the following patch. If it makes sense to you, I can send the patch to you, or you can just quash it into yours. And here is another change request. diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c - gpio_set_value(drvdata-gpio, value); + if (gpio_cansleep(drvdata-gpio)) + gpio_set_value_cansleep(drvdata-gpio, value); + else + gpio_set_value(drvdata-gpio, value); That's not right. Calling gpio_set_value() v.s. gpio_set_value_cansleep() should be based on the properties of the calling context, not the GPIO being controlled. In other words, if it's permissible to call gpio_set_value_cansleep() at this point in the code, simply always call that, and remove the conditional logic. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On 07/16/2013 03:59 AM, Philipp Zabel wrote: ... Deferred probing is fine, but it'd be nice to keep the probe deferral loops to a minimum where possible and/or reasonable. I agree, but manually selecting initcalls for drivers is not the solution. See my other email. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On Tue, Jul 16, 2013 at 09:45:43AM -0600, Stephen Warren wrote: Registering the driver earlier won't cause any bugs. However, it's not the correct approach. Deferred probe /is/ the approach for assuring correct dependencies between drivers. It works and should be used. There are not enough initcall levels to play games using initcalls and solve all the issues, and the ordering requirements vary board-to-board. Deferred probe at runtime handles this without having to manually place all the drivers into specific initcall levels, and dynamically adjusts to board differences, since it all happens automatically at run-time. I do not quite follow the argument here. I agree with you that deferred probe is the approach to solve dependencies. But it does not necessarily mean that initcall can not be used to help it save some nasty or nested deferring. Deferred probe and initcalls are not two mutually exclusive mechanisms but two which can help each other. Shawn ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On Tue, Jul 16, 2013 at 09:47:02AM -0600, Stephen Warren wrote: diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c - gpio_set_value(drvdata-gpio, value); + if (gpio_cansleep(drvdata-gpio)) + gpio_set_value_cansleep(drvdata-gpio, value); + else + gpio_set_value(drvdata-gpio, value); That's not right. Calling gpio_set_value() v.s. gpio_set_value_cansleep() should be based on the properties of the calling context, not the GPIO being controlled. In other words, if it's permissible to call gpio_set_value_cansleep() at this point in the code, simply always call that, and remove the conditional logic. Ah, yes. I was confused by the API gpio_cansleep(). Which API we should call depends on the calling context. And the context should come from the gpio-reset client device driver. IOW, we have no idea what the context is in gpio-reset driver. To start with something simple, we may want to define the gpio-reset as a sleepable interface and always call gpio_set_value_cansleep() in there. Shawn ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On 07/15/2013 07:50 PM, Shawn Guo wrote: Hi Philipp, On Thu, May 30, 2013 at 11:09:00AM +0200, Philipp Zabel wrote: This driver implements a reset controller device that toggle a gpio connected to a reset pin of a peripheral IC. The delay between assertion and de-assertion of the reset signal can be configured via device tree. Signed-off-by: Philipp Zabel p.za...@pengutronix.de Reviewed-by: Stephen Warren swar...@nvidia.com I see this patch is very useful, as GPIOs are widely used to reset components/devices on board. But I do not find the patch in v3.11-rc1. What's your plan about it? Also, I'm wondering if we should register the driver a little bit early. Please see the following patch. If it makes sense to you, I can send the patch to you, or you can just quash it into yours. Shawn ---8 From 2f8ce9e5d6525b98f3828b707458e83fabb39d50 Mon Sep 17 00:00:00 2001 From: Shawn Guo shawn@linaro.org Date: Sun, 14 Jul 2013 20:41:00 +0800 Subject: [PATCH] ENGR00269945: reset: register gpio-reset driver in arch_initcall It's a little bit late to register gpio-reset driver at module_init time, because gpio-reset provides reset control via gpio for other devices which are mostly probed at module_init time too. And it becomes even worse, when the gpio comes from IO expander on I2C bus, e.g. pca953x. In that case, gpio-reset needs to be ready before I2C bus driver which is generally ready at subsys_initcall time. Let's register gpio-reset driver in arch_initcall() to have it ready early enough. There's no need for the reset driver to be registered before its users; the users of the reset GPIO will simply have their probe deferred until the reset controller is available, and then everything will work out just fine. The defer probe mechanism is not used here, because a reset controller driver should be reasonably registered early than other devices. More importantly, defer probe doe not help in some nasty cases, e.g. the gpio-pca953x device itself needs a reset from gpio-reset driver to start working. That should work fine with deferred probe. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins
On Mon, Jul 15, 2013 at 09:35:52PM -0600, Stephen Warren wrote: It's a little bit late to register gpio-reset driver at module_init time, because gpio-reset provides reset control via gpio for other devices which are mostly probed at module_init time too. And it becomes even worse, when the gpio comes from IO expander on I2C bus, e.g. pca953x. In that case, gpio-reset needs to be ready before I2C bus driver which is generally ready at subsys_initcall time. Let's register gpio-reset driver in arch_initcall() to have it ready early enough. There's no need for the reset driver to be registered before its users; the users of the reset GPIO will simply have their probe deferred until the reset controller is available, and then everything will work out just fine. The defer probe mechanism is not used here, because a reset controller driver should be reasonably registered early than other devices. More importantly, defer probe doe not help in some nasty cases, e.g. the gpio-pca953x device itself needs a reset from gpio-reset driver to start working. That should work fine with deferred probe. I should probably rework the commit log. But I do not see a problem to register gpio-reset driver a little bit earlier. On the other hand, if we reply on deferred probe, many drivers probe could be deferred. For example, on my system, the gpio-pca953x on I2C bus works as a GPIO controller and provides resets to many board level components. Deferring probe of gpio-pca953x on I2C bus means every single probe of all these components gets deferred. IMO, this situation should be reasonably avoided. Shawn ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
