RE: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB suspend
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.y...@microchip.com [mailto:wenyou.y...@microchip.com]
> Sent: 2016年8月5日 11:46
> To: st...@rowland.harvard.edu; wenyou.y...@atmel.com
> Cc: gre...@linuxfoundation.org; nicolas.fe...@atmel.com;
> alexandre.bell...@free-electrons.com; linux-ker...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-usb@vger.kernel.org
> Subject: RE: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB
> suspend
>
> Hi Alan,
>
> > -Original Message-
> > From: Alan Stern [mailto:st...@rowland.harvard.edu]
> > Sent: 2016年8月4日 23:02
> > To: Wenyou Yang
> > Cc: Greg Kroah-Hartman ; Nicolas Ferre
> > ; Alexandre Belloni > electrons.com>; linux-ker...@vger.kernel.org; linux-arm-
> > ker...@lists.infradead.org; linux-usb@vger.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.
>
> 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
> > > Reviewed-by: Alexandre Belloni
> > >
> > > Acked-by: Nicolas Ferre
> >
> > 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, ®val);
> > > + 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
RE: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB suspend
Hi Alan,
> -Original Message-
> From: Alan Stern [mailto:st...@rowland.harvard.edu]
> Sent: 2016年8月4日 23:02
> To: Wenyou Yang
> Cc: Greg Kroah-Hartman ; Nicolas Ferre
> ; Alexandre Belloni electrons.com>; linux-ker...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-usb@vger.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.
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
> > Reviewed-by: Alexandre Belloni
> > Acked-by: Nicolas Ferre
>
> 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, ®val);
> > + 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_
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
> Reviewed-by: Alexandre Belloni
> Acked-by: Nicolas Ferre
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, ®val);
> + 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;
> + u1
