| Message ID | 1405617213-27360-1-git-send-email-lee.jones@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi, On Thu, Jul 17, 2014 at 06:13:33PM +0100, Lee Jones wrote: > This patch provides mechanism for subordinate devices to check > whether the DWC3 core probed successfully or otherwise. Useful > if PHYs are required to configure controllers, but aren't yet > available. The DWC3 core driver will defer probe if PHYs are > unavailable, however subordinate DWC3 drivers currently do not > have any visibility or means to check status - until now. what's a subordinate DWC3 driver ? > Another way to do this would be to *_phy_get*(), but if every > driver did this it would create a high level of code > duplication. > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/usb/dwc3/core.c | 12 ++++++++++++ > drivers/usb/dwc3/core.h | 1 + > 2 files changed, 13 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index eb69eb9..171ca52 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -47,6 +47,14 @@ > > /* -------------------------------------------------------------------------- */ > > +static bool is_enabled = false; > + > +int dwc3_is_enabled(void) > +{ > + return is_enabled; > +} > +EXPORT_SYMBOL(dwc3_is_enabled); no, no, no, no. Let me try that again, hello no! You _do_ realise there are systems with more than one dwc3 instance, right ? And this is the most fragile possible way of doing this. You never explained what's a dwc3 subordinate driver, you don't show any example of how this would be used and why/where does the PHY need to poke into DWC3. Why isn't probe defer enough for you ? Which platform are you working on ? what is the problem that you're trying to solve ? From this patch, all I can is NAK this patch with no mercy, sorry.
On Thu, 17 Jul 2014, Felipe Balbi wrote: > Hi, > > On Thu, Jul 17, 2014 at 06:13:33PM +0100, Lee Jones wrote: > > This patch provides mechanism for subordinate devices to check > > whether the DWC3 core probed successfully or otherwise. Useful > > if PHYs are required to configure controllers, but aren't yet > > available. The DWC3 core driver will defer probe if PHYs are > > unavailable, however subordinate DWC3 drivers currently do not > > have any visibility or means to check status - until now. > > what's a subordinate DWC3 driver ? > > > Another way to do this would be to *_phy_get*(), but if every > > driver did this it would create a high level of code > > duplication. > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > drivers/usb/dwc3/core.c | 12 ++++++++++++ > > drivers/usb/dwc3/core.h | 1 + > > 2 files changed, 13 insertions(+) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index eb69eb9..171ca52 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -47,6 +47,14 @@ > > > > /* -------------------------------------------------------------------------- */ > > > > +static bool is_enabled = false; > > + > > +int dwc3_is_enabled(void) > > +{ > > + return is_enabled; > > +} > > +EXPORT_SYMBOL(dwc3_is_enabled); > > no, no, no, no. Let me try that again, hello no! You _do_ realise there > are systems with more than one dwc3 instance, right ? And this is the > most fragile possible way of doing this. > > You never explained what's a dwc3 subordinate driver, you don't show any > example of how this would be used and why/where does the PHY need to > poke into DWC3. Why isn't probe defer enough for you ? Which platform > are you working on ? what is the problem that you're trying to solve ? > > From this patch, all I can is NAK this patch with no mercy, sorry. That's okay, I knew this was going to happen hence the RFC status of the patch. In the DT case, I describe 'subordinate devices' as are drivers which register the DWC3 core using of_platform_populate(), so, for now: drivers/usb/dwc3/dwc3-exynos.c drivers/usb/dwc3/dwc3-keystone.c drivers/usb/dwc3/dwc3-omap.c We're attempting to use the same process; however, at the moment we are suffering with a 'boot order' issue. If the PHYs aren't up and we attempt to configure through the glue-layer our board locks up. Presumably waiting for a read to return, forever. Whist the core does the correct thing i.e. -EPROBE_DEFER, we (dwc3-st.c) have no way of checking the return status of dwc3_probe(). As mentioned in the commit message, another way of ensuring the PHYs are available is to request them, but this would mean an awful lot of code duplication. In your opinion, what's the best way to handle this?
Hi, On Fri, Jul 18, 2014 at 08:11:15AM +0100, Lee Jones wrote: <snip> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > index eb69eb9..171ca52 100644 > > > --- a/drivers/usb/dwc3/core.c > > > +++ b/drivers/usb/dwc3/core.c > > > @@ -47,6 +47,14 @@ > > > > > > /* -------------------------------------------------------------------------- */ > > > > > > +static bool is_enabled = false; > > > + > > > +int dwc3_is_enabled(void) > > > +{ > > > + return is_enabled; > > > +} > > > +EXPORT_SYMBOL(dwc3_is_enabled); > > > > no, no, no, no. Let me try that again, hello no! You _do_ realise there > > are systems with more than one dwc3 instance, right ? And this is the > > most fragile possible way of doing this. > > > > You never explained what's a dwc3 subordinate driver, you don't show any > > example of how this would be used and why/where does the PHY need to > > poke into DWC3. Why isn't probe defer enough for you ? Which platform > > are you working on ? what is the problem that you're trying to solve ? > > > > From this patch, all I can is NAK this patch with no mercy, sorry. > > That's okay, I knew this was going to happen hence the RFC status of > the patch. In the DT case, I describe 'subordinate devices' as are > drivers which register the DWC3 core using of_platform_populate(), > so, for now: > > drivers/usb/dwc3/dwc3-exynos.c > drivers/usb/dwc3/dwc3-keystone.c > drivers/usb/dwc3/dwc3-omap.c > > We're attempting to use the same process; however, at the moment we are > suffering with a 'boot order' issue. If the PHYs aren't up and we > attempt to configure through the glue-layer our board locks up. what are you configuring through the glue-layer ? Which glue-layer is causing that ? > Presumably waiting for a read to return, forever. Whist the core does > the correct thing i.e. -EPROBE_DEFER, we (dwc3-st.c) have no way of ah! finally, the glue layer. > checking the return status of dwc3_probe(). As mentioned in the yeah, because glue layers are not supposed to know. There should be no coupling what so ever between glue layer and core driver, other than the fact that glue layer is the one which triggers platform_device creation through of_platform_population(). But the glue layer has (or should have) no interest in exactly when the core driver finishes probing. > commit message, another way of ensuring the PHYs are available is to > request them, but this would mean an awful lot of code duplication. > > In your opinion, what's the best way to handle this? How can I know ? You still haven't fully explained what you need. All you said was that you're trying to "configure through the glue-layer". Care to further explain what the problem really is ? I'm assuming below is what you're concerned about which I had to go dig in the archives because there was no reference to that patch anywhere here. > +static void st_dwc3_init(struct st_dwc3 *dwc3_data) > +{ > + u32 reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL); > + > + reg |= aux_clk_en(1) | ext_cfg_reset_n(1) | xhci_revision(1); so you have auxiliary clock, an external config reset, what's this xhci_revision ? > + reg &= ~sw_pipew_reset_n(1); another reset > + st_dwc3_writel(dwc3_data->glue_base, USB2_CLKRST_CTRL, reg); looks like it should be split between a CCF and reset drivers. Or maybe a single driver which does both. Do you have a clock/reset control for all IPs ? That might be a good way to hide stuff, driver would simply call clk_get()/clk_prepare_enable() and reset_assert()/deassert() when necessary (sure, this doesn't solve the 'when has that guy probe' but you still haven't explained why you need it). > + reg = st_dwc3_readl(dwc3_data->glue_base, USB2_VBUS_MNGMNT_SEL1); > + reg |= SEL_OVERRIDE_VBUSVALID(1) | SEL_OVERRIDE_POWERPRESENT(1) | > + SEL_OVERRIDE_BVALID(1); this is not correct. You don't know if VBUS is really valid at this time. We have used a gpio which gets pull high/low depending on the state of VBUS/ID. > + st_dwc3_writel(dwc3_data->glue_base, USB2_VBUS_MNGMNT_SEL1, reg); > + udelay(100); > + > + reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL); > + reg |= sw_pipew_reset_n(1); > + st_dwc3_writel(dwc3_data->glue_base, USB2_CLKRST_CTRL, reg); let me ask you something else. Isn't the DWC3_GUSB3PIPECTL_PHYSOFTRST bit functional for you guys ? This sw_pipe2_reset_n looks suspicious. ps: I read that the board hangs, but why ? Have you checked with IP folks ? Maybe that's a silicon bug they're going to fix and we can hack things for now with an errata ID/revision check ?
Hi Felipe, Sorry for the delay in replying. I've been trying to get to the root cause of this problem so I could reply which took longer than I had hoped. The problem manifested itself as a hang on register read/write access if dwc3-st probed before the usb3 phy. Even though dwc3 core would bail and return -EPROBE_DEFER that is not propogated up through of_platform_populate. <snip> > > yeah, because glue layers are not supposed to know. There should be no > coupling what so ever between glue layer and core driver, other than the > fact that glue layer is the one which triggers platform_device creation > through of_platform_population(). But the glue layer has (or should > have) no interest in exactly when the core driver finishes probing. Thanks for this clue :-) As it got me debugging why there was this dependency between the usb3 phy IP and the ST glue register wrapper around the dwc3 usb core. The reason for the depedency / hang is that there is a shared reset signal for the dwc3 core, glue registers and usb3 phy. This reset signal was only being managed in the USB3 phy driver, which is why if dwc3-st or dwc3 did any register access it would cause a hang. So the solution is in addition to taking the devm_reset_control for the powerdown signal, in V3 of the dwc3-st glue layer, it also gets the softreset signal, and deasserts this before any register accesses. This is now working properly without any init ordering hacks etc. > > > commit message, another way of ensuring the PHYs are available is to > > request them, but this would mean an awful lot of code duplication. > > > > In your opinion, what's the best way to handle this? > > How can I know ? You still haven't fully explained what you need. All > you said was that you're trying to "configure through the glue-layer". We can forget about this now. Having dwc3-st take a reference on the usb3 phys was just another method I was experimenting with to find out whether the usb3 PHY had probed or not. > > Care to further explain what the problem really is ? I'm assuming below > is what you're concerned about which I had to go dig in the archives > because there was no reference to that patch anywhere here. Hopefully I have now above, and the proposed solution. > > > +static void st_dwc3_init(struct st_dwc3 *dwc3_data) > > +{ > > + u32 reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL); > > + > > + reg |= aux_clk_en(1) | ext_cfg_reset_n(1) | xhci_revision(1); > > so you have auxiliary clock, an external config reset, what's this > xhci_revision ? xhci_revision is an input signal to the dwc3 core, if it is asserted then the host controller compiles with the xHCI revision 1.0 spec, if not it complies with xHCI revision 0.96 spec. This input signal to dwc3 core is exposed in the CLKRST_CTRL glue register wrapped around the controller by ST. Looking through the docs, it was present until 2.40a, then removed as an input signal to the core from 2.50a onwards. To make this clearer I have also added a comment above the xhci_revision macro in V3 of the dwc3-st patches explaining the bitfield and what it does. > looks like it should be split between a CCF and reset drivers. Or maybe > a single driver which does both. Do you have a clock/reset control for > all IPs ? Yes most IPs which have reset or powerdown signals are already controlled by a driver in drivers/reset/sti. These reset and powerdown signals are all exposed in the sysconfig registers of the SoC. Indeed it was a shared reset signal which wasn't being properly managed and causing the hang. However the reset signal and clock gate here is controlling a small piece of wrapper IP called pipew which sits between the dwc3 core and usb3 phy. I believe this pipew protocol wrapper hardware is designed internally by ST, and has some special contriants which is why these reset signals are being exposed here in the glue logic (see below). > That might be a good way to hide stuff, driver would simply > call clk_get()/clk_prepare_enable() and reset_assert()/deassert() when > necessary (sure, this doesn't solve the 'when has that guy probe' but > you still haven't explained why you need it). > > > + reg = st_dwc3_readl(dwc3_data->glue_base, USB2_VBUS_MNGMNT_SEL1); > > + reg |= SEL_OVERRIDE_VBUSVALID(1) | SEL_OVERRIDE_POWERPRESENT(1) | > > + SEL_OVERRIDE_BVALID(1); > > this is not correct. You don't know if VBUS is really valid at this > time. We have used a gpio which gets pull high/low depending on the > state of VBUS/ID. This isn't stating that VBUS is valid, it is configuring a mux to select where the vbus / bvalid / powerpresent signals will be selected from. I have added a better comment in V3 which hopefully makes the function of VBUS_MNGMNT_SEL register clearer. > > > + st_dwc3_writel(dwc3_data->glue_base, USB2_VBUS_MNGMNT_SEL1, reg); > > + udelay(100); > > + > > + reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL); > > + reg |= sw_pipew_reset_n(1); > > + st_dwc3_writel(dwc3_data->glue_base, USB2_CLKRST_CTRL, reg); > > let me ask you something else. Isn't the DWC3_GUSB3PIPECTL_PHYSOFTRST > bit functional for you guys ? This sw_pipe2_reset_n looks suspicious. Your right to be suspicious ;-) Due to a constriant on the pipew hardware, they have provided two extra software controlled resets ext_cfg_reset and sw_pipew_reset in the CLKRST_CTRL glue reg. These two software controlled resets are ANDED with the nominal cfg_reset_n and pipe_reset_n resets to the pipew hardware. So yes DWC3_GUSB3PIPECTL_PHYSOFTRST is functional, but it will only actually issue a reset to pipew and then onto MiPHY if sw_pipew_reset_n is also set in the glue. The same goes with the global bus_reset_n signal which is subsystem wide reset, it will only bepropogated to pipew if ext_cfg_reset is also set in CLKRST_CTRL. Hopefully that makes things clearer and I've answered everything. I intend to send a V3 shortly. kind regards, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 22, 2014 at 02:55:34PM +0100, Peter Griffin wrote: > Hi Felipe, > > Sorry for the delay in replying. I've been trying to get to the root cause > of this problem so I could reply which took longer than I had hoped. > > The problem manifested itself as a hang on register read/write access if dwc3-st > probed before the usb3 phy. Even though dwc3 core would bail and return > -EPROBE_DEFER that is not propogated up through of_platform_populate. > > <snip> > > > > yeah, because glue layers are not supposed to know. There should be no > > coupling what so ever between glue layer and core driver, other than the > > fact that glue layer is the one which triggers platform_device creation > > through of_platform_population(). But the glue layer has (or should > > have) no interest in exactly when the core driver finishes probing. > > Thanks for this clue :-) As it got me debugging why there was this dependency > between the usb3 phy IP and the ST glue register wrapper around the dwc3 usb core. > > The reason for the depedency / hang is that there is a shared reset signal > for the dwc3 core, glue registers and usb3 phy. This reset signal was only > being managed in the USB3 phy driver, which is why if dwc3-st or dwc3 did > any register access it would cause a hang. > > So the solution is in addition to taking the devm_reset_control for the powerdown > signal, in V3 of the dwc3-st glue layer, it also gets the softreset signal, > and deasserts this before any register accesses. > > This is now working properly without any init ordering hacks etc. AWESOME! :-) Thanks for finding that out, it really helps us keep dwc3 clean without platform-specific hacks ;-) > > > commit message, another way of ensuring the PHYs are available is to > > > request them, but this would mean an awful lot of code duplication. > > > > > > In your opinion, what's the best way to handle this? > > > > How can I know ? You still haven't fully explained what you need. All > > you said was that you're trying to "configure through the glue-layer". > > We can forget about this now. Having dwc3-st take a reference on the usb3 phys was > just another method I was experimenting with to find out whether the usb3 > PHY had probed or not. > > > > > Care to further explain what the problem really is ? I'm assuming below > > is what you're concerned about which I had to go dig in the archives > > because there was no reference to that patch anywhere here. > > Hopefully I have now above, and the proposed solution. > > > > > > +static void st_dwc3_init(struct st_dwc3 *dwc3_data) > > > +{ > > > + u32 reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL); > > > + > > > + reg |= aux_clk_en(1) | ext_cfg_reset_n(1) | xhci_revision(1); > > > > so you have auxiliary clock, an external config reset, what's this > > xhci_revision ? > > xhci_revision is an input signal to the dwc3 core, if it is asserted > then the host controller compiles with the xHCI revision 1.0 spec, if > not it complies with xHCI revision 0.96 spec. This input signal to > dwc3 core is exposed in the CLKRST_CTRL glue register wrapped around > the controller by ST. I wonder why would HW folks give SW access to that, though. Oh well, I've seen weirder things ;-) > Looking through the docs, it was present until 2.40a, then removed as > an input signal to the core from 2.50a onwards. > > To make this clearer I have also added a comment above the > xhci_revision macro in V3 of the dwc3-st patches explaining the > bitfield and what it does. thanks > > looks like it should be split between a CCF and reset drivers. Or maybe > > a single driver which does both. Do you have a clock/reset control for > > all IPs ? > > Yes most IPs which have reset or powerdown signals are already > controlled by a driver in drivers/reset/sti. These reset and powerdown > signals are all exposed in the sysconfig registers of the SoC. Indeed > it was a shared reset signal which wasn't being properly managed and > causing the hang. > > However the reset signal and clock gate here is controlling a small > piece of wrapper IP called pipew which sits between the dwc3 core and > usb3 phy. I believe this pipew protocol wrapper hardware is designed > internally by ST, and has some special contriants which is why these > reset signals are being exposed here in the glue logic (see below). > > > That might be a good way to hide stuff, driver would simply > > call clk_get()/clk_prepare_enable() and reset_assert()/deassert() when > > necessary (sure, this doesn't solve the 'when has that guy probe' but > > you still haven't explained why you need it). > > > > > + reg = st_dwc3_readl(dwc3_data->glue_base, USB2_VBUS_MNGMNT_SEL1); > > > + reg |= SEL_OVERRIDE_VBUSVALID(1) | SEL_OVERRIDE_POWERPRESENT(1) | > > > + SEL_OVERRIDE_BVALID(1); > > > > this is not correct. You don't know if VBUS is really valid at this > > time. We have used a gpio which gets pull high/low depending on the > > state of VBUS/ID. > > This isn't stating that VBUS is valid, it is configuring a mux to > select where the vbus / bvalid / powerpresent signals will be selected > from. /me now notices the "SEL_" prefix :-) > I have added a better comment in V3 which hopefully makes the function > of VBUS_MNGMNT_SEL register clearer. thanks > > > + st_dwc3_writel(dwc3_data->glue_base, USB2_VBUS_MNGMNT_SEL1, reg); > > > + udelay(100); > > > + > > > + reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL); > > > + reg |= sw_pipew_reset_n(1); > > > + st_dwc3_writel(dwc3_data->glue_base, USB2_CLKRST_CTRL, reg); > > > > let me ask you something else. Isn't the DWC3_GUSB3PIPECTL_PHYSOFTRST > > bit functional for you guys ? This sw_pipe2_reset_n looks suspicious. > > Your right to be suspicious ;-) > > Due to a constriant on the pipew hardware, they have provided two > extra software controlled resets ext_cfg_reset and sw_pipew_reset in > the CLKRST_CTRL glue reg. > > These two software controlled resets are ANDED with the nominal > cfg_reset_n and pipe_reset_n resets to the pipew hardware. > > So yes DWC3_GUSB3PIPECTL_PHYSOFTRST is functional, but it will only > actually issue a reset to pipew and then onto MiPHY if > sw_pipew_reset_n is also set in the glue. The same goes with the > global bus_reset_n signal which is subsystem wide reset, it will only > bepropogated to pipew if ext_cfg_reset is also set in CLKRST_CTRL. oh man, what a mess :-) > Hopefully that makes things clearer and I've answered everything. I > intend to send a V3 shortly. sure, thanks
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index eb69eb9..171ca52 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -47,6 +47,14 @@ /* -------------------------------------------------------------------------- */ +static bool is_enabled = false; + +int dwc3_is_enabled(void) +{ + return is_enabled; +} +EXPORT_SYMBOL(dwc3_is_enabled); + void dwc3_set_mode(struct dwc3 *dwc, u32 mode) { u32 reg; @@ -757,6 +765,8 @@ static int dwc3_probe(struct platform_device *pdev) pm_runtime_allow(dev); + is_enabled = true; + return 0; err3: @@ -786,6 +796,8 @@ static int dwc3_remove(struct platform_device *pdev) { struct dwc3 *dwc = platform_get_drvdata(pdev); + is_enabled = false; + usb_phy_set_suspend(dwc->usb2_phy, 1); usb_phy_set_suspend(dwc->usb3_phy, 1); phy_power_off(dwc->usb2_generic_phy); diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 57332e3..94dee86 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -906,6 +906,7 @@ struct dwc3_gadget_ep_cmd_params { /* prototypes */ void dwc3_set_mode(struct dwc3 *dwc, u32 mode); int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc); +int dwc3_is_enabled(void); #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE) int dwc3_host_init(struct dwc3 *dwc);
This patch provides mechanism for subordinate devices to check whether the DWC3 core probed successfully or otherwise. Useful if PHYs are required to configure controllers, but aren't yet available. The DWC3 core driver will defer probe if PHYs are unavailable, however subordinate DWC3 drivers currently do not have any visibility or means to check status - until now. Another way to do this would be to *_phy_get*(), but if every driver did this it would create a high level of code duplication. Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/usb/dwc3/core.c | 12 ++++++++++++ drivers/usb/dwc3/core.h | 1 + 2 files changed, 13 insertions(+)