Re: [RFC] usb: dwc3: core: set force_gen1 bit in USB31 devices if max speed is SS

[Thinh Nguyen]

On Sat, May 13, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 5/13/2023 12:45 AM, Krishna Kurapati PSSNV wrote:
> > Hi Thinh,
> > 
> > On 5/13/2023 12:16 AM, Thinh Nguyen wrote:
> > > On Fri, May 12, 2023, Krishna Kurapati wrote:
> > > > Currently for dwc3_usb31 devices, if maximum_speed is limited to
> > > 
> > > We usually call the controller dwc_usb3, dwc_usb31, or dwc_usb32.
> > > 
> > > > super-speed in DT, then device mode is limited to SS, but host mode
> > > > still works in SSP.
> > > > 
> > > > The documentation for max-speed property is as follows:
> > > > 
> > > > "Tells USB controllers we want to work up to a certain speed.
> > > > Incase  this isn't passed via DT, USB controllers should default to
> > > > their maximum HW capability."
> > > > 
> > > > It doesn't specify that the property is only for device mode.
> > > 
> > > Since this isn't really a fix, can we rephrase the lines below
> > > 
> > > > Fix this by forcing controller supported max speed to Gen1 by
> > > > setting LLUCTL.Force_Gen1 bit if controller is DWC3_USB31 and
> > > > max speed is mentioned as SS in DT.
> > > 
> > > As follow:
> > > There are cases where we need to limit the host's maximum speed to
> > > SuperSpeed only. Use this property for host mode to contrain host's
> > > speed to SuperSpeed.
> > > 
> > > 
> > Sure, will rephrase it accordingly. Thanks for the suggestion.
> > > > 
> > > > Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
> > > > ---
> > > > Discussion regarding the same at:
> > > > https://urldefense.com/v3/__https://lore.kernel.org/all/e465c69c-3a9d-cbdb-d44e-96b99cfa1a92@xxxxxxxxxxx/__;!!A4F2R9G_pg!YiQpjZIJAw-yu6gEwbKqb5nusjnKQ9dQJrulx39lQP-7JMhcNA2xd8uLJoZ_HE8SuG4Rm2uvhJTSdQ2k0fJVAxU2RWYHHg$
> > > > 
> > > >   drivers/usb/dwc3/core.c | 13 +++++++++++++
> > > >   drivers/usb/dwc3/core.h |  4 ++++
> > > >   2 files changed, 17 insertions(+)
> > > > 
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index 0beaab932e7d..989dc76ecbca 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -116,6 +116,18 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
> > > >       dwc->current_dr_role = mode;
> > > >   }
> > > > +static void dwc3_configure_host_speed(struct dwc3 *dwc)
> > > > +{
> > > > +    u32 reg;
> > > > +
> > > > +    if (DWC3_IP_IS(DWC31) &&
> > > > +       (dwc->maximum_speed == USB_SPEED_SUPER)) {
> > > > +        reg = dwc3_readl(dwc->regs, DWC3_LLUCTL);
> > > > +        reg |= DWC3_LLUCTL_FORCE_GEN1;
> > > > +        dwc3_writel(dwc->regs, DWC3_LLUCTL, reg);
> > > > +    }
> > > > +}
> > > > +
> > > >   static void __dwc3_set_mode(struct work_struct *work)
> > > >   {
> > > >       struct dwc3 *dwc = work_to_dwc(work);
> > > > @@ -194,6 +206,7 @@ static void __dwc3_set_mode(struct
> > > > work_struct *work)
> > > >       switch (desired_dr_role) {
> > > >       case DWC3_GCTL_PRTCAP_HOST:
> > > > +        dwc3_configure_host_speed(dwc);
> > >  > The LLUCTL doesn't change until there's a Vcc reset. Let's just>
> > initialize it once during dwc3_core_init() if the GHWPARAM indicates the
> > > controller is DRD or host only.
> > > 
> > 
> > I thought GCTL Core soft reset might clear this bit. That is why I
> > placed it here. For device mode gadget.c takes care of limiting speed.
> > So wanted to do this setting only for host mode, before we invoke host
> > init.
> > 
> > Thanks for letting know that only VCC reset affects this. Will move this
> > check to core init.
> > 
> > Regards,
> > Krishna,
> > 
> 
> In fact, The following snippet from programming guide is why I thought GCTL
> reset can affect this register: ( the below is from usb3 prog guide, I
> assume this would be applicable for usb31 as well)

No. Don't use the same programming guide for different controller
version. The version you're using (1.90a) deprecated GCTL.CoreSoftReset.

> 
> Fields for Register: GCTL
> Core Soft Reset (CoreSoftReset)
> ■ 1'b0 - No soft reset
> ■ 1'b1 - Soft reset to controller
> 
>  Clears the interrupts and all the CSRs except the following
> registers:
> ■ GCTL
> ■ GUCTL
> ■ GSTS
> ■ GSNPSID
> ■ GGPIO
> ■ GUID
> ■ GUSB2PHYCFGn registers
> ■ GUSB3PIPECTLn registers
> ■ DCFG
> ■ DCTL
> ■ DEVTEN
> ■ DSTS
> 
> If so, don't we need to do this setting after every GCTL core soft reset ?
> 

Don't look at that list. It's incomplete. Depending on what programming
guide version you have, usually the newer versions document these info
under "Additional Register Information"

IMHO, it's arguably cleaner to place it in dwc3_core_init(), but it's
fine to place it in host init also. If you feel it's better to place it
in host init, it's reasonable also.

Thanks,
Thinh