Re: [RFC PATCH] usb: dwc3: core: allow vendor drivers to check probe status

[Felipe Balbi]

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

-- 
balbi

Attachment: signature.asc
Description: Digital signature