Date:	Thu, 4 Aug 2016 11:02:26 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Wenyou Yang <wenyou.yang@...el.com>
cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
	<linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-usb@...r.kernel.org>
Subject: Re: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB
 suspend

On Thu, 4 Aug 2016, Wenyou Yang wrote:

> The usb controller does not managed correctly the suspend mode for
> the ehci. In echi mode, there is no way to have utmi_suspend_o_n[1]
> suspend without any device connected to it. This is why this specific
> control is added to fix this issue. The suspend mode works in ohci
> mode.

Why are you talking about EHCI mode?  This patch is only for the 
ohci-at91 driver.

> This specific control is by setting the SUSPEND_A/B/C fields of
> SFR_OHCIICR(OHCI Interrupt Configuration Register) in the SFR
> while OHCI USB suspend.
> 
> This setting operation must be done before the USB clock disabled,
> clear them after the USB clock enabled.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@...el.com>
> Reviewed-by: Alexandre Belloni <alexandre.belloni@...e-electrons.com>
> Acked-by: Nicolas Ferre <nicolas.ferre@...el.com>

I don't know if this is any better than before!  See the comments 
below.

> ---
> 
> Changes in v5:
>  - Use the USB_PORT_FEAT_SUSPEND subcase of the SetPortFeature case
>    to take care it.
>  - Update the commit log.
> 
> Changes in v4:
>  - To check whether the SFR node with "atmel,sama5d2-sfr" compatible
>    is present or not to decide if this feature is applied or not
>    when USB OHCI suspend/resume, instead of new compatible.
>  - Drop the compatible "atmel,sama5d2-ohci".
>  - Drop [PATCH 2/2] ARM: at91/dt: sama5d2: Use new compatible for
>    ohci node.
>  - Drop include/soc/at91/at91_sfr.h, move the macro definitions to
>    atmel-sfr.h which already exists.
>  - Change the defines to align the exists.
> 
> Changes in v3:
>  - Change the compatible description for more precise.
> 
> Changes in v2:
>  - Add compatible to support forcibly suspend the ports.
>  - Add soc/at91/at91_sfr.h to accommodate the defines.
>  - Add error checking for .sfr_regmap.
>  - Remove unnecessary regmap_read() statement.


> @@ -282,6 +301,28 @@ static int ohci_at91_hub_status_data(struct usb_hcd *hcd, char *buf)
>  	return length;
>  }
>  
> +static int ohci_at91_port_ctrl(struct regmap *regmap, u16 port, u8 set)
> +{
> +	u32 regval;
> +	int ret;
> +
> +	if (!regmap)
> +		return 0;
> +
> +	ret = regmap_read(regmap, AT91_SFR_OHCIICR, &regval);
> +	if (ret)
> +		return ret;
> +
> +	if (set)
> +		regval |= AT91_OHCIICR_SUSPEND(port);
> +	else
> +		regval &= ~AT91_OHCIICR_SUSPEND(port);

In the earlier versions of this patch, you did not use the port number.  
Why has this changed?

How many ports does this controller have?

> +
> +	regmap_write(regmap, AT91_SFR_OHCIICR, regval);
> +
> +	return 0;
> +}
> +
>  /*
>   * Look at the control requests to the root hub and see if we need to override.
>   */
> @@ -289,6 +330,7 @@ static int ohci_at91_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  				 u16 wIndex, char *buf, u16 wLength)
>  {
>  	struct at91_usbh_data *pdata = dev_get_platdata(hcd->self.controller);
> +	struct ohci_at91_priv *ohci_at91 = hcd_to_ohci_at91_priv(hcd);
>  	struct usb_hub_descriptor *desc;
>  	int ret = -EINVAL;
>  	u32 *data = (u32 *)buf;
> @@ -301,7 +343,8 @@ static int ohci_at91_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  
>  	switch (typeReq) {
>  	case SetPortFeature:
> -		if (wValue == USB_PORT_FEAT_POWER) {
> +		switch (wValue) {
> +		case USB_PORT_FEAT_POWER:
>  			dev_dbg(hcd->self.controller, "SetPortFeat: POWER\n");
>  			if (valid_port(wIndex)) {
>  				ohci_at91_usb_set_power(pdata, wIndex, 1);
> @@ -309,6 +352,11 @@ static int ohci_at91_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  			}
>  
>  			goto out;
> +
> +		case USB_PORT_FEAT_SUSPEND:
> +			dev_dbg(hcd->self.controller, "SetPortFeat: SUSPEND\n");
> +			ohci_at91_port_ctrl(ohci_at91->sfr_regmap, wIndex, 1);
> +			break;
>  		}
>  		break;
>  
> @@ -342,6 +390,12 @@ static int ohci_at91_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  				ohci_at91_usb_set_power(pdata, wIndex, 0);
>  				return 0;
>  			}
> +			break;
> +
> +		case USB_PORT_FEAT_SUSPEND:
> +			dev_dbg(hcd->self.controller, "ClearPortFeature: SUSPEND\n");
> +			ohci_at91_port_ctrl(ohci_at91->sfr_regmap, wIndex, 0);
> +			break;
>  		}
>  		break;
>  	}

Note that after all this, the code goes ahead to call 
ohci_bub_control().

> @@ -587,7 +641,8 @@ ohci_hcd_at91_drv_suspend(struct device *dev)
>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>  	struct ohci_hcd	*ohci = hcd_to_ohci(hcd);
>  	struct ohci_at91_priv *ohci_at91 = hcd_to_ohci_at91_priv(hcd);
> -	int		ret;
> +	u16 i;
> +	int ret;
>  
>  	/*
>  	 * Disable wakeup if we are going to sleep with slow clock mode
> @@ -599,6 +654,11 @@ ohci_hcd_at91_drv_suspend(struct device *dev)
>  	if (ohci_at91->wakeup)
>  		enable_irq_wake(hcd->irq);
>  
> +	for (i = 0; i < ohci->num_ports; i++) {
> +		ohci_at91_hub_control(hcd, SetPortFeature,
> +				      USB_PORT_FEAT_SUSPEND, (i + 1), NULL, 0);
> +	}
> +
>  	ret = ohci_suspend(hcd, ohci_at91->wakeup);

Do you really want to call ohci_hub_control() for ports that don't have 
a device attached?

>  	if (ret) {
>  		if (ohci_at91->wakeup)
> @@ -630,7 +690,9 @@ static int __maybe_unused
>  ohci_hcd_at91_drv_resume(struct device *dev)
>  {
>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
> +	struct ohci_hcd	*ohci = hcd_to_ohci(hcd);
>  	struct ohci_at91_priv *ohci_at91 = hcd_to_ohci_at91_priv(hcd);
> +	u16 i;
>  
>  	if (ohci_at91->wakeup)
>  		disable_irq_wake(hcd->irq);
> @@ -638,6 +700,12 @@ ohci_hcd_at91_drv_resume(struct device *dev)
>  	at91_start_clock(ohci_at91);
>  
>  	ohci_resume(hcd, false);
> +
> +	for (i = 0; i < ohci->num_ports; i++) {
> +		ohci_at91_hub_control(hcd, ClearPortFeature,
> +				      USB_PORT_FEAT_SUSPEND, i + 1, NULL, 0);
> +	}

Have you thought about how this will interact with runtime PM?

If you intend to do this only for system suspend and not for runtime 
suspend, why not set all the suspend bits for all the ports in the 
OHCIICR register at once, in a single write, like you were doing 
before?

Alan Stern

Powered by blists - more mailing lists