RE: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB suspend

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Alan,

As you saw, I think the version 4 is better than this, can we take the version 4?


Best Regards,
Wenyou Yang

> -----Original Message-----
> From: Wenyou.Yang@xxxxxxxxxxxxx [mailto:Wenyou.Yang@xxxxxxxxxxxxx]
> Sent: 2016年8月5日 11:46
> To: stern@xxxxxxxxxxxxxxxxxxx; wenyou.yang@xxxxxxxxx
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; nicolas.ferre@xxxxxxxxx;
> alexandre.belloni@xxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB
> suspend
> 
> Hi Alan,
> 
> > -----Original Message-----
> > From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx]
> > Sent: 2016年8月4日 23:02
> > To: Wenyou Yang <wenyou.yang@xxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Nicolas Ferre
> > <nicolas.ferre@xxxxxxxxx>; Alexandre Belloni <alexandre.belloni@free-
> > electrons.com>; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
> > kernel@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx
> > 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.
> 
> Actually I described the issue according to the documents from our IP, and this
> specific control is recommended to do in ohci mode.
> 
> >
> > > 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@xxxxxxxxx>
> > > Reviewed-by: Alexandre Belloni
> > > <alexandre.belloni@xxxxxxxxxxxxxxxxxx>
> > > Acked-by: Nicolas Ferre <nicolas.ferre@xxxxxxxxx>
> >
> > I don't know if this is any better than before!  See the comments below.
> 
> Yes, I think so.
> 
> What else advice?
> 
> >
> > > ---
> > >
> > > 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?
> 
> This function is called by ohci_at91_hub_control(), which is invoked on basis of
> port number.
> 
> So, I think it is more reasonable of adding the port argument.
> 
> >
> > How many ports does this controller have?
> 
> This controller has three ports.
> 
> >
> > > +
> > > +	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().
> 
> Yes,
> The ohci_bub_control() will change the port_status[port] register after this
> operation.
> 
> >
> > > @@ -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?
> 
> Yes. I need to do this operation 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?
> 
> Yes, this is only for system suspend.
> 
> As I mentioned above, here calls ohci_at91_hub_control(), which is based on port
> number.
> 
> >
> > Alan Stern
> 
> 
> Best Regards,
> Wenyou Yang
?韬{.n?????%??檩??w?{.n???{炳???骅w*jg????????G??⒏⒎?:+v????????????"??????



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux