| Message ID | 1361093083-22940-9-git-send-email-peter.chen@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Feb 17, 2013 at 05:24:42PM +0800, Peter Chen wrote: > If the probe fails, the ci13xxx_add_device will not return error, > (bus_probe_device doesn't has return value) > therefore, the platform layer can't know whether core's probe > is successful or not, if platform layer goes on using core's struct > which is initialized at core's probe, the error will occur. > > This error is showed when I only compile gadget, the host-only > controller reports "no supported roles", and fails probe, but imx > platform code doesn't know it, and goes on using core's private data. > > Signed-off-by: Peter Chen <peter.chen@freescale.com> this just tells you that platform code shouldn't be using the driver directly. passing probe_retval via platform_data is an abomination, fix the real problem instead, whatever it is.
On Tue, Feb 26, 2013 at 11:42:34AM +0200, Felipe Balbi wrote: > On Sun, Feb 17, 2013 at 05:24:42PM +0800, Peter Chen wrote: > > If the probe fails, the ci13xxx_add_device will not return error, > > (bus_probe_device doesn't has return value) > > therefore, the platform layer can't know whether core's probe > > is successful or not, if platform layer goes on using core's struct > > which is initialized at core's probe, the error will occur. > > > > This error is showed when I only compile gadget, the host-only > > controller reports "no supported roles", and fails probe, but imx > > platform code doesn't know it, and goes on using core's private data. > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > this just tells you that platform code shouldn't be using the driver > directly. passing probe_retval via platform_data is an abomination, fix > the real problem instead, whatever it is. So you suggest the platform glue layer should not use core driver's data directly, eg, for your dwc3, the platform glue layer should not use struct dwc3 *dwc directly? At dwc3-exynos.c, it has code "exynos->dwc3 = dwc3;", so the exynos platform code may will use struct dwc3 in future. Besides, if the dwc3 core driver's probe fails, the exynos platform code will not know it, the usb clk will be on on the usb can't be used. If you suggest like above, we may need to add lots of notify function at core driver as there are many platform specific things, eg, special init/ shutdown, suspend/resume, board layer gpio setting for vbus control (used at id switch), probe fail, etc.
Hi, On Wed, Feb 27, 2013 at 10:22:03AM +0800, Peter Chen wrote: > On Tue, Feb 26, 2013 at 11:42:34AM +0200, Felipe Balbi wrote: > > On Sun, Feb 17, 2013 at 05:24:42PM +0800, Peter Chen wrote: > > > If the probe fails, the ci13xxx_add_device will not return error, > > > (bus_probe_device doesn't has return value) > > > therefore, the platform layer can't know whether core's probe > > > is successful or not, if platform layer goes on using core's struct > > > which is initialized at core's probe, the error will occur. > > > > > > This error is showed when I only compile gadget, the host-only > > > controller reports "no supported roles", and fails probe, but imx > > > platform code doesn't know it, and goes on using core's private data. > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > > > this just tells you that platform code shouldn't be using the driver > > directly. passing probe_retval via platform_data is an abomination, fix > > the real problem instead, whatever it is. > > So you suggest the platform glue layer should not use core driver's data > directly, eg, for your dwc3, the platform glue layer should not use > struct dwc3 *dwc directly? yes, and it doesn't. Ever. > At dwc3-exynos.c, it has code "exynos->dwc3 = dwc3;", so the exynos > platform code may will use struct dwc3 in future. Besides, if the dwc3 nonsense. That's a structure platform_device which was created by the glue, it has nothing to do with struct dwc3. struct platform_device belongs to the glue, but there's an easy way to prevent that by using device_for_each_child() on your ->remove() method. > core driver's probe fails, the exynos platform code will not know it, > the usb clk will be on on the usb can't be used. so ? If the clock belongs to the glue, then the glue should enable it in order to have access to its registers; if the clock belongs to the dwc3 core, then the glue is buggy; if the clock is shared between glue and dwc3 core, then that's another bug, since nobody added clk handling code to dwc3. > If you suggest like above, we may need to add lots of notify function at wrong. There's no need for any notification at all. A driver failing to probe is just normal life. DWC3 is releasing all resources it allocated, but the glue still needs the clock, then that's just the way it is. There are many error messages which will tell the user that e.g. dwc3 failed to probe and user will just try again. If clocks are left enabled, that's a bug in either core driver or glue layer which needs fixing. > core driver as there are many platform specific things, eg, special init/ > shutdown, suspend/resume, board layer gpio setting for vbus control (used gpio handling should be done at board-file, that's a bug. You need to add a fixed regulator which is toggled by a GPIO.
On Wed, Feb 27, 2013 at 02:12:38PM +0200, Felipe Balbi wrote: > Hi, > > On Wed, Feb 27, 2013 at 10:22:03AM +0800, Peter Chen wrote: > > On Tue, Feb 26, 2013 at 11:42:34AM +0200, Felipe Balbi wrote: > > > On Sun, Feb 17, 2013 at 05:24:42PM +0800, Peter Chen wrote: > > > > If the probe fails, the ci13xxx_add_device will not return error, > > > > (bus_probe_device doesn't has return value) > > > > therefore, the platform layer can't know whether core's probe > > > > is successful or not, if platform layer goes on using core's struct > > > > which is initialized at core's probe, the error will occur. > > > > > > > > This error is showed when I only compile gadget, the host-only > > > > controller reports "no supported roles", and fails probe, but imx > > > > platform code doesn't know it, and goes on using core's private data. > > > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > > > > > this just tells you that platform code shouldn't be using the driver > > > directly. passing probe_retval via platform_data is an abomination, fix > > > the real problem instead, whatever it is. > > > > So you suggest the platform glue layer should not use core driver's data > > directly, eg, for your dwc3, the platform glue layer should not use > > struct dwc3 *dwc directly? > > yes, and it doesn't. Ever. > > > At dwc3-exynos.c, it has code "exynos->dwc3 = dwc3;", so the exynos > > platform code may will use struct dwc3 in future. Besides, if the dwc3 > > nonsense. That's a structure platform_device which was created by the > glue, it has nothing to do with struct dwc3. struct platform_device > belongs to the glue, but there's an easy way to prevent that by using > device_for_each_child() on your ->remove() method. Sorry, I just thought it may use platform_get_drvdata(exynos->dwc3) to get core data if exynos has special suspend/resume routine, and need to visit dwc3 register. > > wrong. There's no need for any notification at all. A driver failing to > probe is just normal life. DWC3 is releasing all resources it allocated, > but the glue still needs the clock, then that's just the way it is. Here, we just talk the controller core clk, if the core fails to probe, any meaningful for platform layer still alive? > > There are many error messages which will tell the user that e.g. dwc3 > failed to probe and user will just try again. If clocks are left > enabled, that's a bug in either core driver or glue layer which needs > fixing. > If the dwc3 core fails to probe, but controller core clk is still on, is it a valid case? > > core driver as there are many platform specific things, eg, special init/ > > shutdown, suspend/resume, board layer gpio setting for vbus control (used > > gpio handling should be done at board-file, that's a bug. You need to > add a fixed regulator which is toggled by a GPIO. I think I need to move vbus regulator from platform code to core code as we have struct otg at core data, and vbus operation is common operation. > > -- > balbi
Hi, On Thu, Feb 28, 2013 at 11:11:20AM +0800, Peter Chen wrote: > On Wed, Feb 27, 2013 at 02:12:38PM +0200, Felipe Balbi wrote: > > Hi, > > > > On Wed, Feb 27, 2013 at 10:22:03AM +0800, Peter Chen wrote: > > > On Tue, Feb 26, 2013 at 11:42:34AM +0200, Felipe Balbi wrote: > > > > On Sun, Feb 17, 2013 at 05:24:42PM +0800, Peter Chen wrote: > > > > > If the probe fails, the ci13xxx_add_device will not return error, > > > > > (bus_probe_device doesn't has return value) > > > > > therefore, the platform layer can't know whether core's probe > > > > > is successful or not, if platform layer goes on using core's struct > > > > > which is initialized at core's probe, the error will occur. > > > > > > > > > > This error is showed when I only compile gadget, the host-only > > > > > controller reports "no supported roles", and fails probe, but imx > > > > > platform code doesn't know it, and goes on using core's private data. > > > > > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > > > > > > > this just tells you that platform code shouldn't be using the driver > > > > directly. passing probe_retval via platform_data is an abomination, fix > > > > the real problem instead, whatever it is. > > > > > > So you suggest the platform glue layer should not use core driver's data > > > directly, eg, for your dwc3, the platform glue layer should not use > > > struct dwc3 *dwc directly? > > > > yes, and it doesn't. Ever. > > > > > At dwc3-exynos.c, it has code "exynos->dwc3 = dwc3;", so the exynos > > > platform code may will use struct dwc3 in future. Besides, if the dwc3 > > > > nonsense. That's a structure platform_device which was created by the > > glue, it has nothing to do with struct dwc3. struct platform_device > > belongs to the glue, but there's an easy way to prevent that by using > > device_for_each_child() on your ->remove() method. > > Sorry, I just thought it may use platform_get_drvdata(exynos->dwc3) to > get core data if exynos has special suspend/resume routine, and need > to visit dwc3 register. that would be utterly wrong and has caused many issues before (see MUSB). Glue shouldn't know anything about the core IP. > > There are many error messages which will tell the user that e.g. dwc3 > > failed to probe and user will just try again. If clocks are left > > enabled, that's a bug in either core driver or glue layer which needs > > fixing. > > > > If the dwc3 core fails to probe, but controller core clk is still on, is it > a valid case? of course not, but then again, core clk shouldn't be handled by glue layer. You need to figure out who owns the clock, if it feeds DWC3 why would you clk_get() and clk_prepare_enable() from glue ? Makes no sense. > > > core driver as there are many platform specific things, eg, special init/ > > > shutdown, suspend/resume, board layer gpio setting for vbus control (used > > > > gpio handling should be done at board-file, that's a bug. You need to I mean "shouldn't" > > add a fixed regulator which is toggled by a GPIO. > > I think I need to move vbus regulator from platform code to core code as we have > struct otg at core data, and vbus operation is common operation. right
On Thu, Feb 28, 2013 at 09:26:17AM +0200, Felipe Balbi wrote: > Hi, > > On Thu, Feb 28, 2013 at 11:11:20AM +0800, Peter Chen wrote: > > On Wed, Feb 27, 2013 at 02:12:38PM +0200, Felipe Balbi wrote: > > > Hi, > > > > > > On Wed, Feb 27, 2013 at 10:22:03AM +0800, Peter Chen wrote: > > > > On Tue, Feb 26, 2013 at 11:42:34AM +0200, Felipe Balbi wrote: > > > > > On Sun, Feb 17, 2013 at 05:24:42PM +0800, Peter Chen wrote: > > > > > > If the probe fails, the ci13xxx_add_device will not return error, > > > > > > (bus_probe_device doesn't has return value) > > > > > > therefore, the platform layer can't know whether core's probe > > > > > > is successful or not, if platform layer goes on using core's struct > > > > > > which is initialized at core's probe, the error will occur. > > > > > > > > > > > > This error is showed when I only compile gadget, the host-only > > > > > > controller reports "no supported roles", and fails probe, but imx > > > > > > platform code doesn't know it, and goes on using core's private data. > > > > > > > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > > > > > > > > > this just tells you that platform code shouldn't be using the driver > > > > > directly. passing probe_retval via platform_data is an abomination, fix > > > > > the real problem instead, whatever it is. > > > > > > > > So you suggest the platform glue layer should not use core driver's data > > > > directly, eg, for your dwc3, the platform glue layer should not use > > > > struct dwc3 *dwc directly? > > > > > > yes, and it doesn't. Ever. > > > > > > > If the dwc3 core fails to probe, but controller core clk is still on, is it > > a valid case? > > of course not, but then again, core clk shouldn't be handled by glue > layer. You need to figure out who owns the clock, if it feeds DWC3 why > would you clk_get() and clk_prepare_enable() from glue ? Makes no sense. > Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it try to access register at probe, unless platform layer open the clock, how can the core visit the core register.
Hi, On Thu, Feb 28, 2013 at 04:31:44PM +0800, Peter Chen wrote: > > > > > > > If the probe fails, the ci13xxx_add_device will not return error, > > > > > > > (bus_probe_device doesn't has return value) > > > > > > > therefore, the platform layer can't know whether core's probe > > > > > > > is successful or not, if platform layer goes on using core's struct > > > > > > > which is initialized at core's probe, the error will occur. > > > > > > > > > > > > > > This error is showed when I only compile gadget, the host-only > > > > > > > controller reports "no supported roles", and fails probe, but imx > > > > > > > platform code doesn't know it, and goes on using core's private data. > > > > > > > > > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > > > > > > > > > > > this just tells you that platform code shouldn't be using the driver > > > > > > directly. passing probe_retval via platform_data is an abomination, fix > > > > > > the real problem instead, whatever it is. > > > > > > > > > > So you suggest the platform glue layer should not use core driver's data > > > > > directly, eg, for your dwc3, the platform glue layer should not use > > > > > struct dwc3 *dwc directly? > > > > > > > > yes, and it doesn't. Ever. > > > > > > > > > > If the dwc3 core fails to probe, but controller core clk is still on, is it > > > a valid case? > > > > of course not, but then again, core clk shouldn't be handled by glue > > layer. You need to figure out who owns the clock, if it feeds DWC3 why > > would you clk_get() and clk_prepare_enable() from glue ? Makes no sense. > > > > Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it > try to access register at probe, unless platform layer open the clock, how > can the core visit the core register. Is it really this difficult to figure out ? Fair enough, below are all the details: To understand the reason why dwc3/core.c doesn't know about struct clk, you need to consider where the driver was originally written; it was written on an OMAP platform (actually first on a virtual model OMAP - somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then ARM-based FPGA prototype, then OMAP5, none of which needed explicit clock control, see below). OMAP's PM is written in such a way that a pm_runtime_get() will enable the device the all clocks necessary to be usable. Since OMAP would never need to use clocks directly and I would never be able to test that code, I decided not to add it. Now, if dwc3-exynos needs it, the sane thing to do would be add struct clk knowledge to dwc3/core.c but make it optional. If there are no clocks available, don't bail out. Just because dwc3/core.c doesn't know about clocks, it doesn't mean it's correct to hack it into the glue layer if that doesn't need the clock. Now that we know that's a bug, who's going to send me tested patches to teach dwc3/core.c about struct clk in a way that doesn't break PCIe, nor OMAP5 ? cheers
Felipe Balbi <balbi@ti.com> writes: > Hi, > > On Thu, Feb 28, 2013 at 04:31:44PM +0800, Peter Chen wrote: >> > > > > > > If the probe fails, the ci13xxx_add_device will not return error, >> > > > > > > (bus_probe_device doesn't has return value) >> > > > > > > therefore, the platform layer can't know whether core's probe >> > > > > > > is successful or not, if platform layer goes on using core's struct >> > > > > > > which is initialized at core's probe, the error will occur. >> > > > > > > >> > > > > > > This error is showed when I only compile gadget, the host-only >> > > > > > > controller reports "no supported roles", and fails probe, but imx >> > > > > > > platform code doesn't know it, and goes on using core's private data. >> > > > > > > >> > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> >> > > > > > >> > > > > > this just tells you that platform code shouldn't be using the driver >> > > > > > directly. passing probe_retval via platform_data is an abomination, fix >> > > > > > the real problem instead, whatever it is. >> > > > > >> > > > > So you suggest the platform glue layer should not use core driver's data >> > > > > directly, eg, for your dwc3, the platform glue layer should not use >> > > > > struct dwc3 *dwc directly? >> > > > >> > > > yes, and it doesn't. Ever. >> > > > >> > > >> > > If the dwc3 core fails to probe, but controller core clk is still on, is it >> > > a valid case? >> > >> > of course not, but then again, core clk shouldn't be handled by glue >> > layer. You need to figure out who owns the clock, if it feeds DWC3 why >> > would you clk_get() and clk_prepare_enable() from glue ? Makes no sense. >> > >> >> Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it >> try to access register at probe, unless platform layer open the clock, how >> can the core visit the core register. > > Is it really this difficult to figure out ? Fair enough, below are all > the details: > > To understand the reason why dwc3/core.c doesn't know about struct clk, > you need to consider where the driver was originally written; it was > written on an OMAP platform (actually first on a virtual model OMAP - > somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then > ARM-based FPGA prototype, then OMAP5, none of which needed explicit > clock control, see below). > > OMAP's PM is written in such a way that a pm_runtime_get() will enable > the device the all clocks necessary to be usable. Since OMAP would never > need to use clocks directly and I would never be able to test that code, > I decided not to add it. > > Now, if dwc3-exynos needs it, the sane thing to do would be add struct > clk knowledge to dwc3/core.c but make it optional. If there are no > clocks available, don't bail out. I'm not too familiar with the multitudes of platforms out there, but my simple question is: why can't we have pm runtime take care of enabling/disabling the clocks so that we don't have to do it in drivers? Seems obvious that a platform/SoC/board should know about it's clock tree structure, so why doesn't the platform code then take care of all the dirty details? It seems totally unreasonable and messy to add notion of clocks to drivers just because some platforms can't get their PM right. > Just because dwc3/core.c doesn't know about clocks, it doesn't mean it's > correct to hack it into the glue layer if that doesn't need the clock. > > Now that we know that's a bug, who's going to send me tested patches to > teach dwc3/core.c about struct clk in a way that doesn't break PCIe, nor > OMAP5 ? So are you sure that's what you want? Regards, -- Alex
On Thu, Feb 28, 2013 at 11:32:09AM +0200, Alexander Shishkin wrote: > Felipe Balbi <balbi@ti.com> writes: > > > Hi, > > > > On Thu, Feb 28, 2013 at 04:31:44PM +0800, Peter Chen wrote: > >> > > > > > > If the probe fails, the ci13xxx_add_device will not return error, > >> > > > > > > (bus_probe_device doesn't has return value) > >> > > > > > > therefore, the platform layer can't know whether core's probe > >> > > > > > > is successful or not, if platform layer goes on using core's struct > >> > > > > > > which is initialized at core's probe, the error will occur. > >> > > > > > > > >> > > > > > > This error is showed when I only compile gadget, the host-only > >> > > > > > > controller reports "no supported roles", and fails probe, but imx > >> > > > > > > platform code doesn't know it, and goes on using core's private data. > >> > > > > > > > >> > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > >> > > > > > > >> > > > > > this just tells you that platform code shouldn't be using the driver > >> > > > > > directly. passing probe_retval via platform_data is an abomination, fix > >> > > > > > the real problem instead, whatever it is. > >> > > > > > >> > > > > So you suggest the platform glue layer should not use core driver's data > >> > > > > directly, eg, for your dwc3, the platform glue layer should not use > >> > > > > struct dwc3 *dwc directly? > >> > > > > >> > > > yes, and it doesn't. Ever. > >> > > > > >> > > > >> > > If the dwc3 core fails to probe, but controller core clk is still on, is it > >> > > a valid case? > >> > > >> > of course not, but then again, core clk shouldn't be handled by glue > >> > layer. You need to figure out who owns the clock, if it feeds DWC3 why > >> > would you clk_get() and clk_prepare_enable() from glue ? Makes no sense. > >> > > >> > >> Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it > >> try to access register at probe, unless platform layer open the clock, how > >> can the core visit the core register. > > > > Is it really this difficult to figure out ? Fair enough, below are all > > the details: > > > > To understand the reason why dwc3/core.c doesn't know about struct clk, > > you need to consider where the driver was originally written; it was > > written on an OMAP platform (actually first on a virtual model OMAP - > > somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then > > ARM-based FPGA prototype, then OMAP5, none of which needed explicit > > clock control, see below). > > > > OMAP's PM is written in such a way that a pm_runtime_get() will enable > > the device the all clocks necessary to be usable. Since OMAP would never > > need to use clocks directly and I would never be able to test that code, > > I decided not to add it. > > > > Now, if dwc3-exynos needs it, the sane thing to do would be add struct > > clk knowledge to dwc3/core.c but make it optional. If there are no > > clocks available, don't bail out. > > I'm not too familiar with the multitudes of platforms out there, but my > simple question is: why can't we have pm runtime take care of > enabling/disabling the clocks so that we don't have to do it in drivers? > Seems obvious that a platform/SoC/board should know about it's clock > tree structure, so why doesn't the platform code then take care of all > the dirty details? I agree, clock stuffs should be handled at platform layer. For this corner case (core probe fails), if all of us agree with clock needs to be closed, we may need add some special handling. For runtime pm enabled, it is easy. we can set runtime pm active at fail path, as platform is parent of core, it will call platform's runtime suspend to do low power handling. For runtime pm disabled, we may had to add some ugly things, like notify core probe fail, and platform layer needs to handle this failed notify. > > It seems totally unreasonable and messy to add notion of clocks to > drivers just because some platforms can't get their PM right. > > > Just because dwc3/core.c doesn't know about clocks, it doesn't mean it's > > correct to hack it into the glue layer if that doesn't need the clock. > > > > Now that we know that's a bug, who's going to send me tested patches to > > teach dwc3/core.c about struct clk in a way that doesn't break PCIe, nor > > OMAP5 ? > > So are you sure that's what you want? > > Regards, > -- > Alex >
Hi, On Thu, Feb 28, 2013 at 11:32:09AM +0200, Alexander Shishkin wrote: > >> > > > > > > If the probe fails, the ci13xxx_add_device will not return error, > >> > > > > > > (bus_probe_device doesn't has return value) > >> > > > > > > therefore, the platform layer can't know whether core's probe > >> > > > > > > is successful or not, if platform layer goes on using core's struct > >> > > > > > > which is initialized at core's probe, the error will occur. > >> > > > > > > > >> > > > > > > This error is showed when I only compile gadget, the host-only > >> > > > > > > controller reports "no supported roles", and fails probe, but imx > >> > > > > > > platform code doesn't know it, and goes on using core's private data. > >> > > > > > > > >> > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > >> > > > > > > >> > > > > > this just tells you that platform code shouldn't be using the driver > >> > > > > > directly. passing probe_retval via platform_data is an abomination, fix > >> > > > > > the real problem instead, whatever it is. > >> > > > > > >> > > > > So you suggest the platform glue layer should not use core driver's data > >> > > > > directly, eg, for your dwc3, the platform glue layer should not use > >> > > > > struct dwc3 *dwc directly? > >> > > > > >> > > > yes, and it doesn't. Ever. > >> > > > > >> > > > >> > > If the dwc3 core fails to probe, but controller core clk is still on, is it > >> > > a valid case? > >> > > >> > of course not, but then again, core clk shouldn't be handled by glue > >> > layer. You need to figure out who owns the clock, if it feeds DWC3 why > >> > would you clk_get() and clk_prepare_enable() from glue ? Makes no sense. > >> > > >> > >> Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it > >> try to access register at probe, unless platform layer open the clock, how > >> can the core visit the core register. > > > > Is it really this difficult to figure out ? Fair enough, below are all > > the details: > > > > To understand the reason why dwc3/core.c doesn't know about struct clk, > > you need to consider where the driver was originally written; it was > > written on an OMAP platform (actually first on a virtual model OMAP - > > somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then > > ARM-based FPGA prototype, then OMAP5, none of which needed explicit > > clock control, see below). > > > > OMAP's PM is written in such a way that a pm_runtime_get() will enable > > the device the all clocks necessary to be usable. Since OMAP would never > > need to use clocks directly and I would never be able to test that code, > > I decided not to add it. > > > > Now, if dwc3-exynos needs it, the sane thing to do would be add struct > > clk knowledge to dwc3/core.c but make it optional. If there are no > > clocks available, don't bail out. > > I'm not too familiar with the multitudes of platforms out there, but my > simple question is: why can't we have pm runtime take care of > enabling/disabling the clocks so that we don't have to do it in drivers? that's what OMAP does. > Seems obvious that a platform/SoC/board should know about it's clock > tree structure, so why doesn't the platform code then take care of all > the dirty details? it might seem that way, but it's not that obvious ;-) Some platforms have a single clock, some will split interface and functional clocks, in some cases, you have extra optional clocks which might be needed during certain use cases or to implement erratas at locations that only driver knows. It's a tradeoff, of course. > It seems totally unreasonable and messy to add notion of clocks to > drivers just because some platforms can't get their PM right. it's not that simple ;-) > > Just because dwc3/core.c doesn't know about clocks, it doesn't mean it's > > correct to hack it into the glue layer if that doesn't need the clock. > > > > Now that we know that's a bug, who's going to send me tested patches to > > teach dwc3/core.c about struct clk in a way that doesn't break PCIe, nor > > OMAP5 ? > > So are you sure that's what you want? Well, how quickly can Exynos be changed to handle clocks without driver's knowledge ? Also, I'm a lot more 'at ease' when I see the driver explicitly handling all of its resources. The whole "let's hide XYZ from driver because driver authors never get things right" always causes problems. Specially in the ARM land where there's no standardization at all. When you think solely about x86 platforms, where you have ACPI properly standardized and anyone wanting to build an x86 processor needs to implement ACPI, it's a lot easier :-) I'm sure the new "get default pinctrl state before driver probe()" will cause issues down the road (think silicon erratas, shared balls on the BGA packaging). So, if they can hide clock handling from driver easily, good, let's do just that. Otherwise, meanwhile we need to cope with the extra power consumption that error condition would cause.
Hi, On Thu, Feb 28, 2013 at 06:06:55PM +0800, Peter Chen wrote: > > >> > > > > > > If the probe fails, the ci13xxx_add_device will not return error, > > >> > > > > > > (bus_probe_device doesn't has return value) > > >> > > > > > > therefore, the platform layer can't know whether core's probe > > >> > > > > > > is successful or not, if platform layer goes on using core's struct > > >> > > > > > > which is initialized at core's probe, the error will occur. > > >> > > > > > > > > >> > > > > > > This error is showed when I only compile gadget, the host-only > > >> > > > > > > controller reports "no supported roles", and fails probe, but imx > > >> > > > > > > platform code doesn't know it, and goes on using core's private data. > > >> > > > > > > > > >> > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > >> > > > > > > > >> > > > > > this just tells you that platform code shouldn't be using the driver > > >> > > > > > directly. passing probe_retval via platform_data is an abomination, fix > > >> > > > > > the real problem instead, whatever it is. > > >> > > > > > > >> > > > > So you suggest the platform glue layer should not use core driver's data > > >> > > > > directly, eg, for your dwc3, the platform glue layer should not use > > >> > > > > struct dwc3 *dwc directly? > > >> > > > > > >> > > > yes, and it doesn't. Ever. > > >> > > > > > >> > > > > >> > > If the dwc3 core fails to probe, but controller core clk is still on, is it > > >> > > a valid case? > > >> > > > >> > of course not, but then again, core clk shouldn't be handled by glue > > >> > layer. You need to figure out who owns the clock, if it feeds DWC3 why > > >> > would you clk_get() and clk_prepare_enable() from glue ? Makes no sense. > > >> > > > >> > > >> Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it > > >> try to access register at probe, unless platform layer open the clock, how > > >> can the core visit the core register. > > > > > > Is it really this difficult to figure out ? Fair enough, below are all > > > the details: > > > > > > To understand the reason why dwc3/core.c doesn't know about struct clk, > > > you need to consider where the driver was originally written; it was > > > written on an OMAP platform (actually first on a virtual model OMAP - > > > somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then > > > ARM-based FPGA prototype, then OMAP5, none of which needed explicit > > > clock control, see below). > > > > > > OMAP's PM is written in such a way that a pm_runtime_get() will enable > > > the device the all clocks necessary to be usable. Since OMAP would never > > > need to use clocks directly and I would never be able to test that code, > > > I decided not to add it. > > > > > > Now, if dwc3-exynos needs it, the sane thing to do would be add struct > > > clk knowledge to dwc3/core.c but make it optional. If there are no > > > clocks available, don't bail out. > > > > I'm not too familiar with the multitudes of platforms out there, but my > > simple question is: why can't we have pm runtime take care of > > enabling/disabling the clocks so that we don't have to do it in drivers? > > Seems obvious that a platform/SoC/board should know about it's clock > > tree structure, so why doesn't the platform code then take care of all > > the dirty details? > > I agree, clock stuffs should be handled at platform layer. > For this corner case (core probe fails), if all of us > agree with clock needs to be closed, we may need add some > special handling. > For runtime pm enabled, it is easy. we can set runtime pm active at > fail path, as platform is parent of core, it will call > platform's runtime suspend to do low power handling. if core probe fails, we should still call pm_runtime_put_sync() and pm_runtime_disable() and that should be enough to trigger your ->pm_domain->runtime_suspend() which can be used to turn off unnecessary clocks. > For runtime pm disabled, we may had to add some ugly things, like > notify core probe fail, and platform layer needs to handle this failed > notify. Please stop talking about about "notify core probe fail" that will never happen. Not today, not ever. Forget that idea.
Felipe Balbi <balbi@ti.com> writes: > Hi, > > On Thu, Feb 28, 2013 at 11:32:09AM +0200, Alexander Shishkin wrote: >> >> > > > > > > If the probe fails, the ci13xxx_add_device will not return error, >> >> > > > > > > (bus_probe_device doesn't has return value) >> >> > > > > > > therefore, the platform layer can't know whether core's probe >> >> > > > > > > is successful or not, if platform layer goes on using core's struct >> >> > > > > > > which is initialized at core's probe, the error will occur. >> >> > > > > > > >> >> > > > > > > This error is showed when I only compile gadget, the host-only >> >> > > > > > > controller reports "no supported roles", and fails probe, but imx >> >> > > > > > > platform code doesn't know it, and goes on using core's private data. >> >> > > > > > > >> >> > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> >> >> > > > > > >> >> > > > > > this just tells you that platform code shouldn't be using the driver >> >> > > > > > directly. passing probe_retval via platform_data is an abomination, fix >> >> > > > > > the real problem instead, whatever it is. >> >> > > > > >> >> > > > > So you suggest the platform glue layer should not use core driver's data >> >> > > > > directly, eg, for your dwc3, the platform glue layer should not use >> >> > > > > struct dwc3 *dwc directly? >> >> > > > >> >> > > > yes, and it doesn't. Ever. >> >> > > > >> >> > > >> >> > > If the dwc3 core fails to probe, but controller core clk is still on, is it >> >> > > a valid case? >> >> > >> >> > of course not, but then again, core clk shouldn't be handled by glue >> >> > layer. You need to figure out who owns the clock, if it feeds DWC3 why >> >> > would you clk_get() and clk_prepare_enable() from glue ? Makes no sense. >> >> > >> >> >> >> Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it >> >> try to access register at probe, unless platform layer open the clock, how >> >> can the core visit the core register. >> > >> > Is it really this difficult to figure out ? Fair enough, below are all >> > the details: >> > >> > To understand the reason why dwc3/core.c doesn't know about struct clk, >> > you need to consider where the driver was originally written; it was >> > written on an OMAP platform (actually first on a virtual model OMAP - >> > somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then >> > ARM-based FPGA prototype, then OMAP5, none of which needed explicit >> > clock control, see below). >> > >> > OMAP's PM is written in such a way that a pm_runtime_get() will enable >> > the device the all clocks necessary to be usable. Since OMAP would never >> > need to use clocks directly and I would never be able to test that code, >> > I decided not to add it. >> > >> > Now, if dwc3-exynos needs it, the sane thing to do would be add struct >> > clk knowledge to dwc3/core.c but make it optional. If there are no >> > clocks available, don't bail out. >> >> I'm not too familiar with the multitudes of platforms out there, but my >> simple question is: why can't we have pm runtime take care of >> enabling/disabling the clocks so that we don't have to do it in drivers? > > that's what OMAP does. > >> Seems obvious that a platform/SoC/board should know about it's clock >> tree structure, so why doesn't the platform code then take care of all >> the dirty details? > > it might seem that way, but it's not that obvious ;-) Some platforms > have a single clock, some will split interface and functional clocks, in > some cases, you have extra optional clocks which might be needed during > certain use cases or to implement erratas at locations that only driver > knows. Then drivers could use platform fixup callbacks. Curious how many drivers out there do proper handling of interface vs functional clocks. Something tells me that the common pattern will be "enable all clocks with lots of line of boilerplate copied from that other driver" in probe() and "disable all" in remove(). > It's a tradeoff, of course. > >> It seems totally unreasonable and messy to add notion of clocks to >> drivers just because some platforms can't get their PM right. > > it's not that simple ;-) > >> > Just because dwc3/core.c doesn't know about clocks, it doesn't mean it's >> > correct to hack it into the glue layer if that doesn't need the clock. >> > >> > Now that we know that's a bug, who's going to send me tested patches to >> > teach dwc3/core.c about struct clk in a way that doesn't break PCIe, nor >> > OMAP5 ? >> >> So are you sure that's what you want? > > Well, how quickly can Exynos be changed to handle clocks without > driver's knowledge ? Motivation for the platforms to change should come from the general direction of linux kernel maintainers, in order for the change to happen. :) But yes, for the time being, you're right, we'll probably have to just cope with it. > Also, I'm a lot more 'at ease' when I see the driver explicitly handling > all of its resources. The whole "let's hide XYZ from driver because > driver authors never get things right" always causes problems. Specially > in the ARM land where there's no standardization at all. I think it's going to be like that as long as developers are working in "hack it and ship it and let somebody else figure out APIs" routine. I understand that some drivers will always need to call into clock framework directly, but other should have an option not to if their operation doesn't depend on it. Regards, -- Alex
On Thu, Feb 28, 2013 at 12:45:59PM +0200, Felipe Balbi wrote: > Hi, > > On Thu, Feb 28, 2013 at 06:06:55PM +0800, Peter Chen wrote: > > > For runtime pm disabled, we may had to add some ugly things, like > > notify core probe fail, and platform layer needs to handle this failed > > notify. > > Please stop talking about about "notify core probe fail" that will never > happen. Not today, not ever. Forget that idea. Of course, it is ugly way, I just thought if it will affect power, eg dvfs, bus freq if the usb clock is on. Maybe the user will find USB core init fails case causes runtime power rise, and fix the core init fail bug. > > -- > balbi
On Fri, Mar 01, 2013 at 09:45:51AM +0800, Peter Chen wrote: > On Thu, Feb 28, 2013 at 12:45:59PM +0200, Felipe Balbi wrote: > > Hi, > > > > On Thu, Feb 28, 2013 at 06:06:55PM +0800, Peter Chen wrote: > > > > > For runtime pm disabled, we may had to add some ugly things, like > > > notify core probe fail, and platform layer needs to handle this failed > > > notify. > > > > Please stop talking about about "notify core probe fail" that will never > > happen. Not today, not ever. Forget that idea. > > Of course, it is ugly way, I just thought if it will affect power, eg dvfs, > bus freq if the usb clock is on. Maybe the user will find USB core init fails > case causes runtime power rise, and fix the core init fail bug. right, we need to fix the bug, but hiding it under some obscure way of passing return values back to glue layer isn't correct. We should fix the real issue, always, instead of hacking around buggy code.
Hi, On Thu, Feb 28, 2013 at 01:55:30PM +0200, Alexander Shishkin wrote: > >> >> > > > > > > If the probe fails, the ci13xxx_add_device will not return error, > >> >> > > > > > > (bus_probe_device doesn't has return value) > >> >> > > > > > > therefore, the platform layer can't know whether core's probe > >> >> > > > > > > is successful or not, if platform layer goes on using core's struct > >> >> > > > > > > which is initialized at core's probe, the error will occur. > >> >> > > > > > > > >> >> > > > > > > This error is showed when I only compile gadget, the host-only > >> >> > > > > > > controller reports "no supported roles", and fails probe, but imx > >> >> > > > > > > platform code doesn't know it, and goes on using core's private data. > >> >> > > > > > > > >> >> > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > >> >> > > > > > > >> >> > > > > > this just tells you that platform code shouldn't be using the driver > >> >> > > > > > directly. passing probe_retval via platform_data is an abomination, fix > >> >> > > > > > the real problem instead, whatever it is. > >> >> > > > > > >> >> > > > > So you suggest the platform glue layer should not use core driver's data > >> >> > > > > directly, eg, for your dwc3, the platform glue layer should not use > >> >> > > > > struct dwc3 *dwc directly? > >> >> > > > > >> >> > > > yes, and it doesn't. Ever. > >> >> > > > > >> >> > > > >> >> > > If the dwc3 core fails to probe, but controller core clk is still on, is it > >> >> > > a valid case? > >> >> > > >> >> > of course not, but then again, core clk shouldn't be handled by glue > >> >> > layer. You need to figure out who owns the clock, if it feeds DWC3 why > >> >> > would you clk_get() and clk_prepare_enable() from glue ? Makes no sense. > >> >> > > >> >> > >> >> Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it > >> >> try to access register at probe, unless platform layer open the clock, how > >> >> can the core visit the core register. > >> > > >> > Is it really this difficult to figure out ? Fair enough, below are all > >> > the details: > >> > > >> > To understand the reason why dwc3/core.c doesn't know about struct clk, > >> > you need to consider where the driver was originally written; it was > >> > written on an OMAP platform (actually first on a virtual model OMAP - > >> > somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then > >> > ARM-based FPGA prototype, then OMAP5, none of which needed explicit > >> > clock control, see below). > >> > > >> > OMAP's PM is written in such a way that a pm_runtime_get() will enable > >> > the device the all clocks necessary to be usable. Since OMAP would never > >> > need to use clocks directly and I would never be able to test that code, > >> > I decided not to add it. > >> > > >> > Now, if dwc3-exynos needs it, the sane thing to do would be add struct > >> > clk knowledge to dwc3/core.c but make it optional. If there are no > >> > clocks available, don't bail out. > >> > >> I'm not too familiar with the multitudes of platforms out there, but my > >> simple question is: why can't we have pm runtime take care of > >> enabling/disabling the clocks so that we don't have to do it in drivers? > > > > that's what OMAP does. > > > >> Seems obvious that a platform/SoC/board should know about it's clock > >> tree structure, so why doesn't the platform code then take care of all > >> the dirty details? > > > > it might seem that way, but it's not that obvious ;-) Some platforms > > have a single clock, some will split interface and functional clocks, in > > some cases, you have extra optional clocks which might be needed during > > certain use cases or to implement erratas at locations that only driver > > knows. > > Then drivers could use platform fixup callbacks. Curious how many ugh, I just puked in my mouth a little bit... > drivers out there do proper handling of interface vs functional > clocks. Something tells me that the common pattern will be "enable all that, in itself, is no argument against explicitly handling clocks. Bugs are bugs and will always exist, instead of hiding the problem, we should fix them ;-) > clocks with lots of line of boilerplate copied from that other driver" > in probe() and "disable all" in remove(). for a first iteration, you're likely right. When we get to a point where driver is really stable we can start trying to optimize runtime power consumption by fiddling with clocks. If we're not going to access a device's register, why would you keep interface clock running ? That's where e.g. runtime_pm fails to give us a good solution. There's no (easy) way to tell pm_domain code that it can gate interface clock but not functional clock. > > Well, how quickly can Exynos be changed to handle clocks without > > driver's knowledge ? > > Motivation for the platforms to change should come from the general > direction of linux kernel maintainers, in order for the change to > happen. :) > > But yes, for the time being, you're right, we'll probably have to just > cope with it. exactly. Just because XYZ wants clocks to be handled all via pm_domain, it doesn't mean we can switch to that overnight. On top of that there are situations such as the one describe above. > > Also, I'm a lot more 'at ease' when I see the driver explicitly handling > > all of its resources. The whole "let's hide XYZ from driver because > > driver authors never get things right" always causes problems. Specially > > in the ARM land where there's no standardization at all. > > I think it's going to be like that as long as developers are working in > "hack it and ship it and let somebody else figure out APIs" routine. I that's why we need capable maintainers. Sometimes things will fall through the cracks, sure, but in most cases we should be blocking "hack and ship" during code review ;-) > understand that some drivers will always need to call into clock > framework directly, but other should have an option not to if their > operation doesn't depend on it. right, so that's why I asked to add struct clk knowledge to dwc3 but make it optional: OMAP doesn't need it, PCIe doesn't need it; only exynos (currently) needs it. cheers
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 6cc6069..45fa227 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -689,6 +689,7 @@ static int ci_hdrc_probe(struct platform_device *pdev) /* Defer some operations */ queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(200)); + ci->platdata->probe_retval = ret; return ret; free_irq: @@ -704,6 +705,7 @@ rm_wq: flush_workqueue(ci->wq); destroy_workqueue(ci->wq); + ci->platdata->probe_retval = ret; return ret; } diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h index d543e4a..8f61e97 100644 --- a/include/linux/usb/chipidea.h +++ b/include/linux/usb/chipidea.h @@ -23,6 +23,7 @@ struct ci13xxx_platform_data { #define CI13XXX_CONTROLLER_RESET_EVENT 0 #define CI13XXX_CONTROLLER_STOPPED_EVENT 1 void (*notify_event) (struct ci13xxx *ci, unsigned event); + int probe_retval; /* tell platform layer ci core probe's result */ }; /* Default offset of capability registers */
If the probe fails, the ci13xxx_add_device will not return error, (bus_probe_device doesn't has return value) therefore, the platform layer can't know whether core's probe is successful or not, if platform layer goes on using core's struct which is initialized at core's probe, the error will occur. This error is showed when I only compile gadget, the host-only controller reports "no supported roles", and fails probe, but imx platform code doesn't know it, and goes on using core's private data. Signed-off-by: Peter Chen <peter.chen@freescale.com> --- drivers/usb/chipidea/core.c | 2 ++ include/linux/usb/chipidea.h | 1 + 2 files changed, 3 insertions(+), 0 deletions(-)