RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

[Yang, Wenyou]

Hi Alan,

Sorry for late answer.

> -----Original Message-----
> From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx]
> Sent: 2016年5月13日 2:11
> To: Yang, Wenyou <Wenyou.Yang@xxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Ferre, Nicolas
> <Nicolas.FERRE@xxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending
> 
> On Thu, 12 May 2016, Wenyou Yang wrote:
> 
> > In order to get lower consumption, as a workaround, suspend the USB
> > PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI Interrupt
> > Configuration Register while OHCI USB suspending.
> 
> What does this mean?  What does suspending a port do?  Is it the same as a
> normal USB port suspend?

The usb controller from Synopsis does not managed correctly the suspend mode for the EHCI. 
There is no way to have the VDDUTMII (USB device and host UTMI interface) suspend without any device connected to it. 

That's why we added this specific control to fix this issue. Namely, by setting some bits of one of the special function registers to fix this issue outside the usb controller. 

And the suspend mode works in OHCI mode.

It is not same as a normal USB port suspend.

> 
> If it is the same, why doesn't the USB_PORT_FEAT_SUSPEND subcase of the
> SetPortFeature case in ohci_hub_control() already take care of this?
> 
> > This suspend operation must be done before stopping the USB clock,
> > resume after the USB clock enabled.
> >
> > Signed-off-by: Wenyou Yang <wenyou.yang@xxxxxxxxx>
> > ---
> 
> > @@ -132,6 +135,17 @@ static void at91_stop_hc(struct platform_device
> > *pdev)
> >
> >
> > /*--------------------------------------------------------------------
> > -----*/
> >
> > +struct regmap *at91_dt_syscon_sfr(void) {
> > +	struct regmap *regmap;
> > +
> > +	regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> > +	if (IS_ERR(regmap))
> > +		return NULL;
> 
> If you get an error, the regmap pointer is set to NULL...
> 
> > @@ -197,6 +211,8 @@ static int usb_hcd_at91_probe(const struct hc_driver
> *driver,
> >  		goto err;
> >  	}
> >
> > +	ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
> 
> With no other error checking...
> 
> > +
> >  	board = hcd->self.controller->platform_data;
> >  	ohci = hcd_to_ohci(hcd);
> >  	ohci->num_ports = board->ports;
> 
> > +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable) {
> > +	u32 regval;
> > +	int ret;
> > +
> > +	if (IS_ERR(regmap))
> > +		return PTR_ERR(regmap);
> > +
> > +	ret = regmap_read(regmap, SFR_OHCIICR, &regval);
> 
> And now what happens if regmap is NULL?  Hint: It won't be pretty...
> 
> Alan Stern


Best Regards,
Wenyou Yang