| Message ID | 1398350916-885-1-git-send-email-ch.naveen@samsung.com |
|---|---|
| State | Rejected |
| Headers | show |
On Thu, Apr 24, 2014 at 08:18:36PM +0530, Naveen Krishna Chatradhi wrote: > This patch moves initialization code to subsys_initcall() to ensure > that the i2c bus is available early so the regulators can be quickly > probed and available for other devices on their probe() call. > Such solution has been proposed by Mark Brown to fix the problem of > the regulators not beeing available on the peripheral device probe(): > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011971.html What specifically is this needed for? We *should* be able to use deferred probe for most things, but I know that not all subsystems are able to yet.
On 04/24/2014 09:55 PM, Mark Brown wrote: > On Thu, Apr 24, 2014 at 08:18:36PM +0530, Naveen Krishna Chatradhi wrote: >> This patch moves initialization code to subsys_initcall() to ensure >> that the i2c bus is available early so the regulators can be quickly >> probed and available for other devices on their probe() call. > >> Such solution has been proposed by Mark Brown to fix the problem of >> the regulators not beeing available on the peripheral device probe(): >> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011971.html > > What specifically is this needed for? We *should* be able to use > deferred probe for most things, but I know that not all subsystems are > able to yet. > DRM-Exynos is one such sub-system right now that doesn't handle deferred probe well. That is one of the reasons for this patch.
Hello Mark, On 24 April 2014 21:55, Mark Brown <broonie@kernel.org> wrote: > On Thu, Apr 24, 2014 at 08:18:36PM +0530, Naveen Krishna Chatradhi wrote: >> This patch moves initialization code to subsys_initcall() to ensure >> that the i2c bus is available early so the regulators can be quickly >> probed and available for other devices on their probe() call. > >> Such solution has been proposed by Mark Brown to fix the problem of >> the regulators not beeing available on the peripheral device probe(): >> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011971.html > > What specifically is this needed for? We *should* be able to use > deferred probe for most things, but I know that not all subsystems are > able to yet. DRM related drivers like DP, FIMD, HDMI, Mixer wants to be probed ASAP during the boot. The real problem comes when, one of these drivers do a regulator_get(). If the physical supply is not enabled/hookedup the regulator_get() call assumes that physical supply is present and returns a "dummy_regulator" (But, not an error). Because of which, Display and several other devices fails to work. I2C, I2C_TUNNEL, SPI and DMA drivers are required as subsys_initcall() for similar reason.
On Fri, May 09, 2014 at 05:50:00PM +0530, Naveen Krishna Ch wrote: > On 24 April 2014 21:55, Mark Brown <broonie@kernel.org> wrote: > >> Such solution has been proposed by Mark Brown to fix the problem of > >> the regulators not beeing available on the peripheral device probe(): > >> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011971.html > > What specifically is this needed for? We *should* be able to use > > deferred probe for most things, but I know that not all subsystems are > > able to yet. > DRM related drivers like DP, FIMD, HDMI, Mixer wants to be probed ASAP > during the boot. > The real problem comes when, one of these drivers do a regulator_get(). > If the physical supply is not enabled/hookedup the regulator_get() call > assumes that physical supply is present and returns a > "dummy_regulator" (But, not an error). > Because of which, Display and several other devices fails to work. These drivers are buggy, if they geniunely expect and handle a missing supply then they should be using regulator_get_optional() to request the regulator and even if they don't the use of subsys_initcall() is not going to fix anything here - if a dummy regulator is going to be returned the time things are probed won't make a difference. > I2C, I2C_TUNNEL, SPI and DMA drivers are required as subsys_initcall() > for similar reason. What makes you say this? Typically those drivers don't use regulators at all.
Hello Mark, On 9 May 2014 19:21, Mark Brown <broonie@kernel.org> wrote: > On Fri, May 09, 2014 at 05:50:00PM +0530, Naveen Krishna Ch wrote: >> On 24 April 2014 21:55, Mark Brown <broonie@kernel.org> wrote: > >> >> Such solution has been proposed by Mark Brown to fix the problem of >> >> the regulators not beeing available on the peripheral device probe(): >> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011971.html > >> > What specifically is this needed for? We *should* be able to use >> > deferred probe for most things, but I know that not all subsystems are >> > able to yet. > >> DRM related drivers like DP, FIMD, HDMI, Mixer wants to be probed ASAP >> during the boot. >> The real problem comes when, one of these drivers do a regulator_get(). > >> If the physical supply is not enabled/hookedup the regulator_get() call >> assumes that physical supply is present and returns a >> "dummy_regulator" (But, not an error). > >> Because of which, Display and several other devices fails to work. > > These drivers are buggy, if they geniunely expect and handle a missing > supply then they should be using regulator_get_optional() to request the > regulator and even if they don't the use of subsys_initcall() is not > going to fix anything here - if a dummy regulator is going to be > returned the time things are probed won't make a difference. We have a situation where.a PMIC is sitting under I2C_TUNNEL with I2C, SPI and SPI uses DMA. PMIC --- I2C_TUNNEL ---- I2C/SPI ---- DMA This PMIC is setting some FETS required for Backlight, LCD and G3D modules. If all the I2C, SPI, DMA, I2C_TUNNEL, DRM based LCD are all mod_probes() DRM drivers are probing a head of the PMIC probe. Which is causing the display drivers to get "dummy_regulators" instead of the real supplies. We couldn't implement -PROBE_DEFER because, the probes are not failing. Should we should check if the return value of regulator_get() is a "dummy_regulator" and then defer the probe /. > >> I2C, I2C_TUNNEL, SPI and DMA drivers are required as subsys_initcall() >> for similar reason. > > What makes you say this? Typically those drivers don't use regulators > at all. I mean I2C, I2C_TUNNEL, SPI and DMA drivers have be to subsys_initcall() to get the PMIC up intime. so, that the system would use the real supplies. This looks like a board specific solution. I guess, this is quite frequent with the buses like I2C, SPI. Kindly, suggest a nicer way. Thanks
On Fri, May 09, 2014 at 08:12:47PM +0530, Naveen Krishna Ch wrote: > On 9 May 2014 19:21, Mark Brown <broonie@kernel.org> wrote: > > On Fri, May 09, 2014 at 05:50:00PM +0530, Naveen Krishna Ch wrote: > >> DRM related drivers like DP, FIMD, HDMI, Mixer wants to be probed ASAP > >> during the boot. > >> The real problem comes when, one of these drivers do a regulator_get(). > >> If the physical supply is not enabled/hookedup the regulator_get() call > >> assumes that physical supply is present and returns a > >> "dummy_regulator" (But, not an error). > >> Because of which, Display and several other devices fails to work. > > These drivers are buggy, if they geniunely expect and handle a missing > > supply then they should be using regulator_get_optional() to request the > > regulator and even if they don't the use of subsys_initcall() is not > > going to fix anything here - if a dummy regulator is going to be > > returned the time things are probed won't make a difference. ... > If all the I2C, SPI, DMA, I2C_TUNNEL, DRM based LCD are all mod_probes() > DRM drivers are probing a head of the PMIC probe. Which is causing the > display drivers to get "dummy_regulators" instead of the real supplies. No, it really won't - I have no idea what you are doing but it's not mainline. If you are getting dummy regulators then you don't have a supply present at all and probe ordering isn't going to make a blind bit of difference. Please provide a specific technical description of the problem you are seeing in mainline.
On Fri, May 09, 2014 at 03:54:25PM +0100, Mark Brown wrote: > On Fri, May 09, 2014 at 08:12:47PM +0530, Naveen Krishna Ch wrote: > > On 9 May 2014 19:21, Mark Brown <broonie@kernel.org> wrote: > > > On Fri, May 09, 2014 at 05:50:00PM +0530, Naveen Krishna Ch wrote: > > > >> DRM related drivers like DP, FIMD, HDMI, Mixer wants to be probed ASAP > > >> during the boot. > > >> The real problem comes when, one of these drivers do a regulator_get(). > > > >> If the physical supply is not enabled/hookedup the regulator_get() call > > >> assumes that physical supply is present and returns a > > >> "dummy_regulator" (But, not an error). > > > >> Because of which, Display and several other devices fails to work. > > > > These drivers are buggy, if they geniunely expect and handle a missing > > > supply then they should be using regulator_get_optional() to request the > > > regulator and even if they don't the use of subsys_initcall() is not > > > going to fix anything here - if a dummy regulator is going to be > > > returned the time things are probed won't make a difference. > > ... > > > If all the I2C, SPI, DMA, I2C_TUNNEL, DRM based LCD are all mod_probes() > > DRM drivers are probing a head of the PMIC probe. Which is causing the > > display drivers to get "dummy_regulators" instead of the real supplies. > > No, it really won't - I have no idea what you are doing but it's not > mainline. If you are getting dummy regulators then you don't have a > supply present at all and probe ordering isn't going to make a blind > bit of difference. > > Please provide a specific technical description of the problem you are > seeing in mainline. +1 Unless we have a clear understanding that this is the only acceptable solution in mainline, this is really an out-of-tree patch.
Hello Wolfram, On 21 May 2014 15:55, Wolfram Sang <wsa@the-dreams.de> wrote: > On Fri, May 09, 2014 at 03:54:25PM +0100, Mark Brown wrote: >> On Fri, May 09, 2014 at 08:12:47PM +0530, Naveen Krishna Ch wrote: >> > On 9 May 2014 19:21, Mark Brown <broonie@kernel.org> wrote: >> > > On Fri, May 09, 2014 at 05:50:00PM +0530, Naveen Krishna Ch wrote: >> >> > >> DRM related drivers like DP, FIMD, HDMI, Mixer wants to be probed ASAP >> > >> during the boot. >> > >> The real problem comes when, one of these drivers do a regulator_get(). >> >> > >> If the physical supply is not enabled/hookedup the regulator_get() call >> > >> assumes that physical supply is present and returns a >> > >> "dummy_regulator" (But, not an error). >> >> > >> Because of which, Display and several other devices fails to work. >> >> > > These drivers are buggy, if they geniunely expect and handle a missing >> > > supply then they should be using regulator_get_optional() to request the >> > > regulator and even if they don't the use of subsys_initcall() is not >> > > going to fix anything here - if a dummy regulator is going to be >> > > returned the time things are probed won't make a difference. >> >> ... >> >> > If all the I2C, SPI, DMA, I2C_TUNNEL, DRM based LCD are all mod_probes() >> > DRM drivers are probing a head of the PMIC probe. Which is causing the >> > display drivers to get "dummy_regulators" instead of the real supplies. >> >> No, it really won't - I have no idea what you are doing but it's not >> mainline. If you are getting dummy regulators then you don't have a >> supply present at all and probe ordering isn't going to make a blind >> bit of difference. >> >> Please provide a specific technical description of the problem you are >> seeing in mainline. > > +1 Unless we have a clear understanding that this is the only acceptable > solution in mainline, this is really an out-of-tree patch. Few of the DRM based devices need regulators during their probe and DRM subsystem wants to bring up LCD as fast as it could to give a better user experience. Exynos DRM (few others for that matter) does platform_driver_register() of devices in a sequence and finally binds them all together into one blob or /dev/dri-0 device. Which makes it hard to implement -EPROBE_DEFER. Even if -EPROBE_DEFER is implemented in DRM, it adds lot of overhead during the boot up. I've proposed to make exynos_drm_init() as late_initcall() instead of making I2C, SPI, DMA and I2C-arbitrator and I2C-TUNNEL drivers as subsys_initcall() http://www.spinics.net/lists/linux-samsung-soc/msg30858.html Kindly, suggest a workable approach for all the subsystems. >
> Kindly, suggest a workable approach for all the subsystems.
Keep this patch out-of-tree? I know that probe ordering causes problems,
and that it needs major efforts. Yet, I understood that adding more and
more subsys_initcall to mainline is not going to help the process unless
essential. Using subsys_initcall has issues, too.
diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c index 00af0a0..20e3077 100644 --- a/drivers/i2c/busses/i2c-exynos5.c +++ b/drivers/i2c/busses/i2c-exynos5.c @@ -762,8 +762,18 @@ static struct platform_driver exynos5_i2c_driver = { }, }; -module_platform_driver(exynos5_i2c_driver); +static int __init i2c_adap_exynos5_init(void) +{ + return platform_driver_register(&exynos5_i2c_driver); +} +subsys_initcall(i2c_adap_exynos5_init); + +static void __exit i2c_adap_exynos5_exit(void) +{ + platform_driver_unregister(&exynos5_i2c_driver); +} +module_exit(i2c_adap_exynos5_exit); MODULE_DESCRIPTION("Exynos5 HS-I2C Bus driver"); MODULE_AUTHOR("Naveen Krishna Chatradhi, <ch.naveen@samsung.com>"); MODULE_AUTHOR("Taekgyun Ko, <taeggyun.ko@samsung.com>");
This patch moves initialization code to subsys_initcall() to ensure that the i2c bus is available early so the regulators can be quickly probed and available for other devices on their probe() call. Such solution has been proposed by Mark Brown to fix the problem of the regulators not beeing available on the peripheral device probe(): http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011971.html Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com> --- drivers/i2c/busses/i2c-exynos5.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)