diff mbox series

usb: host: ohci-at91: suspend/resume ports after/before OHCI accesses

Message ID 20210609121027.70951-1-claudiu.beznea@microchip.com (mailing list archive)
State Superseded
Headers show
Series usb: host: ohci-at91: suspend/resume ports after/before OHCI accesses | expand

Commit Message

Claudiu Beznea June 9, 2021, 12:10 p.m. UTC
On SAMA7G5 suspending ports will cut the access to OHCI registers and
any subsequent access to them will lead to CPU being blocked trying to
access that memory. Same thing happens on resume: if OHCI memory is
accessed before resuming ports the CPU will block on that access. The
OCHI memory is accessed on suspend/resume though
ohci_suspend()/ohci_resume().

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---

The patch was tested on SAMA7G5, SAMA5D2 and SAM9X60.

Thank you,
Claudiu Beznea

 drivers/usb/host/ohci-at91.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Nicolas Ferre June 9, 2021, 12:40 p.m. UTC | #1
On 09/06/2021 at 14:10, Claudiu Beznea wrote:
> On SAMA7G5 suspending ports will cut the access to OHCI registers and
> any subsequent access to them will lead to CPU being blocked trying to
> access that memory. Same thing happens on resume: if OHCI memory is
> accessed before resuming ports the CPU will block on that access. The
> OCHI memory is accessed on suspend/resume though
> ohci_suspend()/ohci_resume().
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>

Claudiu,

Your patch look good to me.

In addition, I see ohci_at91_port_suspend() function also used in 
ohci_at91_hub_control(). It might suffer the same problem as I see 
accesses to ohci register, at first glance.

Can you please double check that we are not in such condition with calls 
to ohci_at91_hub_control()?

Best regards,
   Nicolas


> ---
> 
> The patch was tested on SAMA7G5, SAMA5D2 and SAM9X60.
> 
> Thank you,
> Claudiu Beznea
> 
>   drivers/usb/host/ohci-at91.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
> index b3a6a497dcb1..7c6202b05ff4 100644
> --- a/drivers/usb/host/ohci-at91.c
> +++ b/drivers/usb/host/ohci-at91.c
> @@ -666,8 +666,6 @@ ohci_hcd_at91_drv_suspend(struct device *dev)
>   	if (ohci_at91->wakeup)
>   		enable_irq_wake(hcd->irq);
>   
> -	ohci_at91_port_suspend(ohci_at91, 1);
> -
>   	ret = ohci_suspend(hcd, ohci_at91->wakeup);
>   	if (ret) {
>   		if (ohci_at91->wakeup)
> @@ -687,7 +685,10 @@ ohci_hcd_at91_drv_suspend(struct device *dev)
>   		/* flush the writes */
>   		(void) ohci_readl (ohci, &ohci->regs->control);
>   		msleep(1);
> +		ohci_at91_port_suspend(ohci_at91, 1);
>   		at91_stop_clock(ohci_at91);
> +	} else {
> +		ohci_at91_port_suspend(ohci_at91, 1);
>   	}
>   
>   	return ret;
> @@ -699,6 +700,8 @@ ohci_hcd_at91_drv_resume(struct device *dev)
>   	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>   	struct ohci_at91_priv *ohci_at91 = hcd_to_ohci_at91_priv(hcd);
>   
> +	ohci_at91_port_suspend(ohci_at91, 0);
> +
>   	if (ohci_at91->wakeup)
>   		disable_irq_wake(hcd->irq);
>   	else
> @@ -706,8 +709,6 @@ ohci_hcd_at91_drv_resume(struct device *dev)
>   
>   	ohci_resume(hcd, false);
>   
> -	ohci_at91_port_suspend(ohci_at91, 0);
> -
>   	return 0;
>   }
>   
>
Alan Stern June 9, 2021, 11:07 p.m. UTC | #2
On Wed, Jun 09, 2021 at 03:10:27PM +0300, Claudiu Beznea wrote:
> On SAMA7G5 suspending ports will cut the access to OHCI registers and
> any subsequent access to them will lead to CPU being blocked trying to
> access that memory. Same thing happens on resume: if OHCI memory is
> accessed before resuming ports the CPU will block on that access. The
> OCHI memory is accessed on suspend/resume though
> ohci_suspend()/ohci_resume().

That sounds very strange.  Suppose one of the ports is suspended, so access to the 
OHCI registers is blocked.  Then how can you resume the port?  Don't you have to 
access the OHCI registers in order to tell the controller to do the port resume?

What happens when there's more than one port, and one of them is suspended while 
the other one is still running?  How can you communicate with the active port if 
access to the OHCI registers is blocked?

Alan Stern
Claudiu Beznea June 23, 2021, 12:47 p.m. UTC | #3
On 10.06.2021 02:07, Alan Stern wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Jun 09, 2021 at 03:10:27PM +0300, Claudiu Beznea wrote:
>> On SAMA7G5 suspending ports will cut the access to OHCI registers and
>> any subsequent access to them will lead to CPU being blocked trying to
>> access that memory. Same thing happens on resume: if OHCI memory is
>> accessed before resuming ports the CPU will block on that access. The
>> OCHI memory is accessed on suspend/resume though
>> ohci_suspend()/ohci_resume().
> 
> That sounds very strange.

The clock scheme for OHCI and EHCI IPs on SAMA7G5 is as follows
(I hope it is readable):

                                            Main Xtal
                                               |
                                               +-------------+
                                               |             |
+---------------------------+                 \ /            |
|                 +------+  | 60MHz  +--------------------+  |
|                 |      |  |        |                    |------+
|                 | Port |<----------| UTMI Transceiver A |  |   |
|                 |      |  |        |                    |----+ |
|  USB 2.0 EHCI   |Router|  |        +--------------------+  | | |
| Host Controller |      |  | 60MHz  +--------------------+  | | |
|                 |      |<----------| UTMI Transceiver B |<-+ | |
|                 |      |  |        +--------------------+  | | |
|                 |      |  | 60MHz  +--------------------+  | | |
|                 |      |<----------| UTMI Transceiver C |<-+ | |
|                 |      |  |        +--------------------+    | |
|                 +------+  |                                  | |
|                           |                                  | |
+---------------------------+                                  | |
                                                               | |
+---------------------------+                                  | |
|                 +------+  |         UHP48M                   | |
|                 | Root |  |<---------------------------------+ |
|  USB 1.1 OHCI   | hub  |  |                                    |
| Host Controller | and  |  |         UHP12M                     |
|                 | host |  |<-----------------------------------+
|                 | SIE  |  |
|                 +------+  |
|                           |
+---------------------------+

Where UTMI transceiver A generates the 48MHz and 12MHz clocks for OHCI
full-speed operations.

The ports control is done through AT91_SFR_OHCIICR via
ohci_at91_port_suspend() function where. Setting 0 in AT91_SFR_OHCIICR
means suspend is controlled by EHCI-OHCI and 1 forces the port suspend.
Suspending the port A will cut the clocks for OHCI. I want to mention that
AT91_SFR_OHCIICR register is not in the same memory space of OHCI, EHCI IPs
and is clocked by other clocks.

> Suppose one of the ports is suspended, so access to the
> OHCI registers is blocked.  Then how can you resume the port? 

For run-time control (via ohci_at91_hub_control()), I agree with you that
the current implemented approach is not healthy (taking into account the
clock scheme above) and the fact that we do force the ports suspend on
ohci_at91_hub_control(). For suspend/resume it should be OK as the ports
are suspended at the end of any OHCI accesses (I don't know how the Linux
USB subsystem behaves so please do correct me if I'm wrong).

> Don't you have to
> access the OHCI registers in order to tell the controller to do the port resume?

On our implementation we control the port suspend/resume via
AT91_SFR_OHCIICR, a special kind of register, memory mapped at different
address (compared w/ OHCI, EHCI IPs), so clocked by other clocks.

> 
> What happens when there's more than one port, and one of them is suspended while
> the other one is still running?  How can you communicate with the active port if
> access to the OHCI registers is blocked?

For this kind of scenario (the run-time suspend of a port, not system
suspend/resume) a different mechanism should be implemented taking into
account the clock schema presented above.

Thank you,
Claudiu Beznea

> 
> Alan Stern
>
Alan Stern June 23, 2021, 1:59 p.m. UTC | #4
On Wed, Jun 23, 2021 at 12:47:56PM +0000, Claudiu.Beznea@microchip.com wrote:
> On 10.06.2021 02:07, Alan Stern wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Wed, Jun 09, 2021 at 03:10:27PM +0300, Claudiu Beznea wrote:
> >> On SAMA7G5 suspending ports will cut the access to OHCI registers and
> >> any subsequent access to them will lead to CPU being blocked trying to
> >> access that memory. Same thing happens on resume: if OHCI memory is
> >> accessed before resuming ports the CPU will block on that access. The
> >> OCHI memory is accessed on suspend/resume though
> >> ohci_suspend()/ohci_resume().
> > 
> > That sounds very strange.
> 
> The clock scheme for OHCI and EHCI IPs on SAMA7G5 is as follows
> (I hope it is readable):
> 
>                                             Main Xtal
>                                                |
>                                                +-------------+
>                                                |             |
> +---------------------------+                 \ /            |
> |                 +------+  | 60MHz  +--------------------+  |
> |                 |      |  |        |                    |------+
> |                 | Port |<----------| UTMI Transceiver A |  |   |
> |                 |      |  |        |                    |----+ |
> |  USB 2.0 EHCI   |Router|  |        +--------------------+  | | |
> | Host Controller |      |  | 60MHz  +--------------------+  | | |
> |                 |      |<----------| UTMI Transceiver B |<-+ | |
> |                 |      |  |        +--------------------+  | | |
> |                 |      |  | 60MHz  +--------------------+  | | |
> |                 |      |<----------| UTMI Transceiver C |<-+ | |
> |                 |      |  |        +--------------------+    | |
> |                 +------+  |                                  | |
> |                           |                                  | |
> +---------------------------+                                  | |
>                                                                | |
> +---------------------------+                                  | |
> |                 +------+  |         UHP48M                   | |
> |                 | Root |  |<---------------------------------+ |
> |  USB 1.1 OHCI   | hub  |  |                                    |
> | Host Controller | and  |  |         UHP12M                     |
> |                 | host |  |<-----------------------------------+
> |                 | SIE  |  |
> |                 +------+  |
> |                           |
> +---------------------------+
> 
> Where UTMI transceiver A generates the 48MHz and 12MHz clocks for OHCI
> full-speed operations.
> 
> The ports control is done through AT91_SFR_OHCIICR via
> ohci_at91_port_suspend() function where. Setting 0 in AT91_SFR_OHCIICR
> means suspend is controlled by EHCI-OHCI and 1 forces the port suspend.
> Suspending the port A will cut the clocks for OHCI. I want to mention that
> AT91_SFR_OHCIICR register is not in the same memory space of OHCI, EHCI IPs
> and is clocked by other clocks.
> 
> > Suppose one of the ports is suspended, so access to the
> > OHCI registers is blocked.  Then how can you resume the port? 
> 
> For run-time control (via ohci_at91_hub_control()), I agree with you that
> the current implemented approach is not healthy (taking into account the
> clock scheme above) and the fact that we do force the ports suspend on
> ohci_at91_hub_control(). For suspend/resume it should be OK as the ports
> are suspended at the end of any OHCI accesses (I don't know how the Linux
> USB subsystem behaves so please do correct me if I'm wrong).

(I haven't checked the details recently, so I'm not certain about 
this.)  In some -- perhaps all -- cases, we don't suspend the ports at 
all during system suspend.  We just rely on the USB devices 
automatically going into suspend when the root hub stops sending 
packets.

> > Don't you have to
> > access the OHCI registers in order to tell the controller to do the port resume?
> 
> On our implementation we control the port suspend/resume via
> AT91_SFR_OHCIICR, a special kind of register, memory mapped at different
> address (compared w/ OHCI, EHCI IPs), so clocked by other clocks.
> 
> > 
> > What happens when there's more than one port, and one of them is suspended while
> > the other one is still running?  How can you communicate with the active port if
> > access to the OHCI registers is blocked?
> 
> For this kind of scenario (the run-time suspend of a port, not system
> suspend/resume) a different mechanism should be implemented taking into
> account the clock schema presented above.

Okay, I see.  It seems like the driver will need some significant 
changes to handle runtime power management properly.

One thing you might consider changing: The name of the 
ohci_at91_port_suspend routine is misleading.  It doesn't really 
handle suspending the port; instead it handles the clocks that drive 
the entire OHCI controller.  Right?

Alan Stern
Claudiu Beznea June 23, 2021, 2:09 p.m. UTC | #5
On 23.06.2021 16:59, Alan Stern wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Jun 23, 2021 at 12:47:56PM +0000, Claudiu.Beznea@microchip.com wrote:
>> On 10.06.2021 02:07, Alan Stern wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Wed, Jun 09, 2021 at 03:10:27PM +0300, Claudiu Beznea wrote:
>>>> On SAMA7G5 suspending ports will cut the access to OHCI registers and
>>>> any subsequent access to them will lead to CPU being blocked trying to
>>>> access that memory. Same thing happens on resume: if OHCI memory is
>>>> accessed before resuming ports the CPU will block on that access. The
>>>> OCHI memory is accessed on suspend/resume though
>>>> ohci_suspend()/ohci_resume().
>>>
>>> That sounds very strange.
>>
>> The clock scheme for OHCI and EHCI IPs on SAMA7G5 is as follows
>> (I hope it is readable):
>>
>>                                             Main Xtal
>>                                                |
>>                                                +-------------+
>>                                                |             |
>> +---------------------------+                 \ /            |
>> |                 +------+  | 60MHz  +--------------------+  |
>> |                 |      |  |        |                    |------+
>> |                 | Port |<----------| UTMI Transceiver A |  |   |
>> |                 |      |  |        |                    |----+ |
>> |  USB 2.0 EHCI   |Router|  |        +--------------------+  | | |
>> | Host Controller |      |  | 60MHz  +--------------------+  | | |
>> |                 |      |<----------| UTMI Transceiver B |<-+ | |
>> |                 |      |  |        +--------------------+  | | |
>> |                 |      |  | 60MHz  +--------------------+  | | |
>> |                 |      |<----------| UTMI Transceiver C |<-+ | |
>> |                 |      |  |        +--------------------+    | |
>> |                 +------+  |                                  | |
>> |                           |                                  | |
>> +---------------------------+                                  | |
>>                                                                | |
>> +---------------------------+                                  | |
>> |                 +------+  |         UHP48M                   | |
>> |                 | Root |  |<---------------------------------+ |
>> |  USB 1.1 OHCI   | hub  |  |                                    |
>> | Host Controller | and  |  |         UHP12M                     |
>> |                 | host |  |<-----------------------------------+
>> |                 | SIE  |  |
>> |                 +------+  |
>> |                           |
>> +---------------------------+
>>
>> Where UTMI transceiver A generates the 48MHz and 12MHz clocks for OHCI
>> full-speed operations.
>>
>> The ports control is done through AT91_SFR_OHCIICR via
>> ohci_at91_port_suspend() function where. Setting 0 in AT91_SFR_OHCIICR
>> means suspend is controlled by EHCI-OHCI and 1 forces the port suspend.
>> Suspending the port A will cut the clocks for OHCI. I want to mention that
>> AT91_SFR_OHCIICR register is not in the same memory space of OHCI, EHCI IPs
>> and is clocked by other clocks.
>>
>>> Suppose one of the ports is suspended, so access to the
>>> OHCI registers is blocked.  Then how can you resume the port?
>>
>> For run-time control (via ohci_at91_hub_control()), I agree with you that
>> the current implemented approach is not healthy (taking into account the
>> clock scheme above) and the fact that we do force the ports suspend on
>> ohci_at91_hub_control(). For suspend/resume it should be OK as the ports
>> are suspended at the end of any OHCI accesses (I don't know how the Linux
>> USB subsystem behaves so please do correct me if I'm wrong).
> 
> (I haven't checked the details recently, so I'm not certain about
> this.)  In some -- perhaps all -- cases, we don't suspend the ports at
> all during system suspend.  We just rely on the USB devices
> automatically going into suspend when the root hub stops sending
> packets.
> 
>>> Don't you have to
>>> access the OHCI registers in order to tell the controller to do the port resume?
>>
>> On our implementation we control the port suspend/resume via
>> AT91_SFR_OHCIICR, a special kind of register, memory mapped at different
>> address (compared w/ OHCI, EHCI IPs), so clocked by other clocks.
>>
>>>
>>> What happens when there's more than one port, and one of them is suspended while
>>> the other one is still running?  How can you communicate with the active port if
>>> access to the OHCI registers is blocked?
>>
>> For this kind of scenario (the run-time suspend of a port, not system
>> suspend/resume) a different mechanism should be implemented taking into
>> account the clock schema presented above.
> 
> Okay, I see.  It seems like the driver will need some significant
> changes to handle runtime power management properly.
> 
> One thing you might consider changing: The name of the
> ohci_at91_port_suspend routine is misleading.  It doesn't really
> handle suspending the port; instead it handles the clocks that drive
> the entire OHCI controller.  Right?

It does both as far as I can tell at the moment.

> 
> Alan Stern
>
Alan Stern June 23, 2021, 2:19 p.m. UTC | #6
On Wed, Jun 23, 2021 at 02:09:16PM +0000, Claudiu.Beznea@microchip.com wrote:
> On 23.06.2021 16:59, Alan Stern wrote:
> > One thing you might consider changing: The name of the
> > ohci_at91_port_suspend routine is misleading.  It doesn't really
> > handle suspending the port; instead it handles the clocks that drive
> > the entire OHCI controller.  Right?
> 
> It does both as far as I can tell at the moment.

But the name suggests that it only handles suspending a port.  That's 
misleading.

And the way it is used in the SetPortFeature(USB_PORT_FEAT_SUSPEND) 
case in ohci_at91_hub_control is just plain wrong.  It won't merely 
suspend a single port; it will disable the entire OHCI controller.

Alan Stern
Claudiu Beznea June 23, 2021, 2:33 p.m. UTC | #7
On 23.06.2021 17:19, Alan Stern wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Jun 23, 2021 at 02:09:16PM +0000, Claudiu.Beznea@microchip.com wrote:
>> On 23.06.2021 16:59, Alan Stern wrote:
>>> One thing you might consider changing: The name of the
>>> ohci_at91_port_suspend routine is misleading.  It doesn't really
>>> handle suspending the port; instead it handles the clocks that drive
>>> the entire OHCI controller.  Right?
>>
>> It does both as far as I can tell at the moment.
> 
> But the name suggests that it only handles suspending a port.  That's
> misleading.
> 
> And the way it is used in the SetPortFeature(USB_PORT_FEAT_SUSPEND)
> case in ohci_at91_hub_control is just plain wrong.  It won't merely
> suspend a single port; it will disable the entire OHCI controller.

Agree with all the above!

> 
> Alan Stern
>
Alan Stern June 23, 2021, 4:41 p.m. UTC | #8
On Wed, Jun 23, 2021 at 02:33:14PM +0000, Claudiu.Beznea@microchip.com wrote:
> On 23.06.2021 17:19, Alan Stern wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Wed, Jun 23, 2021 at 02:09:16PM +0000, Claudiu.Beznea@microchip.com wrote:
> >> On 23.06.2021 16:59, Alan Stern wrote:
> >>> One thing you might consider changing: The name of the
> >>> ohci_at91_port_suspend routine is misleading.  It doesn't really
> >>> handle suspending the port; instead it handles the clocks that drive
> >>> the entire OHCI controller.  Right?
> >>
> >> It does both as far as I can tell at the moment.
> > 
> > But the name suggests that it only handles suspending a port.  That's
> > misleading.
> > 
> > And the way it is used in the SetPortFeature(USB_PORT_FEAT_SUSPEND)
> > case in ohci_at91_hub_control is just plain wrong.  It won't merely
> > suspend a single port; it will disable the entire OHCI controller.
> 
> Agree with all the above!

Are there any systems beside the SAMA7G5 and others you tested which 
might be affected by this patch?  Do they all work pretty much the 
same way?  (I want to make sure no others will be adversely affected 
by this change.)

Alan Stern
Claudiu Beznea June 24, 2021, 6:40 a.m. UTC | #9
On 23.06.2021 19:41, Alan Stern wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Jun 23, 2021 at 02:33:14PM +0000, Claudiu.Beznea@microchip.com wrote:
>> On 23.06.2021 17:19, Alan Stern wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Wed, Jun 23, 2021 at 02:09:16PM +0000, Claudiu.Beznea@microchip.com wrote:
>>>> On 23.06.2021 16:59, Alan Stern wrote:
>>>>> One thing you might consider changing: The name of the
>>>>> ohci_at91_port_suspend routine is misleading.  It doesn't really
>>>>> handle suspending the port; instead it handles the clocks that drive
>>>>> the entire OHCI controller.  Right?
>>>>
>>>> It does both as far as I can tell at the moment.
>>>
>>> But the name suggests that it only handles suspending a port.  That's
>>> misleading.
>>>
>>> And the way it is used in the SetPortFeature(USB_PORT_FEAT_SUSPEND)
>>> case in ohci_at91_hub_control is just plain wrong.  It won't merely
>>> suspend a single port; it will disable the entire OHCI controller.
>>
>> Agree with all the above!
> 
> Are there any systems beside the SAMA7G5 and others you tested which
> might be affected by this patch?  Do they all work pretty much the
> same way?  (I want to make sure no others will be adversely affected
> by this change.)

I tested it on SAMA7G5, SAMA5D2 and SAM9X60. I tested the suspend/resume
to/from mem. On SAMA5D2 and SAM9X60 there is no clock provided by
transceiver A to OHCI. I encountered no issues on tested systems. These IPs
are also present on SAMA5D3 and SAMA5D4 systems which I haven't tested as I
expect to behave as SAMA5D2 (as the clocking scheme is the same with
SAMA5D2). I can also try it on a SAMA5D3 (I don't have a SAMA5D4 with me at
the moment), tough, just to be sure nothing is broken there too.

Thank you,
Claudiu Beznea

> 
> Alan Stern
>
Claudiu Beznea June 24, 2021, 6:54 a.m. UTC | #10
On 24.06.2021 09:40, Claudiu Beznea - M18063 wrote:
> On 23.06.2021 19:41, Alan Stern wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On Wed, Jun 23, 2021 at 02:33:14PM +0000, Claudiu.Beznea@microchip.com wrote:
>>> On 23.06.2021 17:19, Alan Stern wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On Wed, Jun 23, 2021 at 02:09:16PM +0000, Claudiu.Beznea@microchip.com wrote:
>>>>> On 23.06.2021 16:59, Alan Stern wrote:
>>>>>> One thing you might consider changing: The name of the
>>>>>> ohci_at91_port_suspend routine is misleading.  It doesn't really
>>>>>> handle suspending the port; instead it handles the clocks that drive
>>>>>> the entire OHCI controller.  Right?
>>>>>
>>>>> It does both as far as I can tell at the moment.
>>>>
>>>> But the name suggests that it only handles suspending a port.  That's
>>>> misleading.
>>>>
>>>> And the way it is used in the SetPortFeature(USB_PORT_FEAT_SUSPEND)
>>>> case in ohci_at91_hub_control is just plain wrong.  It won't merely
>>>> suspend a single port; it will disable the entire OHCI controller.
>>>
>>> Agree with all the above!
>>
>> Are there any systems beside the SAMA7G5 and others you tested which
>> might be affected by this patch?  Do they all work pretty much the
>> same way?  (I want to make sure no others will be adversely affected
>> by this change.)
> 
> I tested it on SAMA7G5, SAMA5D2 and SAM9X60. I tested the suspend/resume
> to/from mem. 

To be sure it has been correctly understood: after suspend/resume cycle I
did USB tests like checking the USB device is recognized, writing data to a
USB mass storage device.

> On SAMA5D2 and SAM9X60 there is no clock provided by
> transceiver A to OHCI. I encountered no issues on tested systems. These IPs
> are also present on SAMA5D3 and SAMA5D4 systems which I haven't tested as I
> expect to behave as SAMA5D2 (as the clocking scheme is the same with
> SAMA5D2). I can also try it on a SAMA5D3 (I don't have a SAMA5D4 with me at
> the moment), tough, just to be sure nothing is broken there too.
> 
> Thank you,
> Claudiu Beznea
> 
>>
>> Alan Stern
>>
>
Alan Stern June 24, 2021, 1:23 p.m. UTC | #11
On Thu, Jun 24, 2021 at 06:40:25AM +0000, Claudiu.Beznea@microchip.com wrote:
> On 23.06.2021 19:41, Alan Stern wrote:
> > Are there any systems beside the SAMA7G5 and others you tested which
> > might be affected by this patch?  Do they all work pretty much the
> > same way?  (I want to make sure no others will be adversely affected
> > by this change.)
> 
> I tested it on SAMA7G5, SAMA5D2 and SAM9X60. I tested the suspend/resume
> to/from mem. On SAMA5D2 and SAM9X60 there is no clock provided by
> transceiver A to OHCI. I encountered no issues on tested systems. These IPs
> are also present on SAMA5D3 and SAMA5D4 systems which I haven't tested as I
> expect to behave as SAMA5D2 (as the clocking scheme is the same with
> SAMA5D2). I can also try it on a SAMA5D3 (I don't have a SAMA5D4 with me at
> the moment), tough, just to be sure nothing is broken there too.

That doesn't answer my question.  I asked if there were any systems 
which might be affected by your patch, and you listed a bunch of 
systems that _aren't_ affected (that is, they continue to work 
properly).

What systems might run into trouble with this patch?

Alan Stern
Claudiu Beznea June 30, 2021, 2:46 p.m. UTC | #12
On 24.06.2021 16:23, Alan Stern wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, Jun 24, 2021 at 06:40:25AM +0000, Claudiu.Beznea@microchip.com wrote:
>> On 23.06.2021 19:41, Alan Stern wrote:
>>> Are there any systems beside the SAMA7G5 and others you tested which
>>> might be affected by this patch?  Do they all work pretty much the
>>> same way?  (I want to make sure no others will be adversely affected
>>> by this change.)
>>
>> I tested it on SAMA7G5, SAMA5D2 and SAM9X60. I tested the suspend/resume
>> to/from mem. On SAMA5D2 and SAM9X60 there is no clock provided by
>> transceiver A to OHCI. I encountered no issues on tested systems. These IPs
>> are also present on SAMA5D3 and SAMA5D4 systems which I haven't tested as I
>> expect to behave as SAMA5D2 (as the clocking scheme is the same with
>> SAMA5D2). I can also try it on a SAMA5D3 (I don't have a SAMA5D4 with me at
>> the moment), tough, just to be sure nothing is broken there too.
> 
> That doesn't answer my question.  I asked if there were any systems
> which might be affected by your patch, and you listed a bunch of
> systems that _aren't_ affected (that is, they continue to work
> properly).

I wrongly understood the initial question.

> 
> What systems might run into trouble with this patch?

These are all I haven't tested and might be affected:
AT91RM9200,
SAM9260,
SAM9261,
SAM9263,
SAM9N12,
SAM9X35,
SAM9G45.

The last two (SAM9X35 and SAM9G45) have the same clocking scheme with
SAMA5D2 (which I tested). For the rest of them I cannot find the clocking
scheme in datasheet and don't have them to test (at least at the moment).

Thank you,
Claudiu Beznea

> 
> Alan Stern
>
Claudiu Beznea June 30, 2021, 2:47 p.m. UTC | #13
On 24.06.2021 16:23, Alan Stern wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, Jun 24, 2021 at 06:40:25AM +0000, Claudiu.Beznea@microchip.com wrote:
>> On 23.06.2021 19:41, Alan Stern wrote:
>>> Are there any systems beside the SAMA7G5 and others you tested which
>>> might be affected by this patch?  Do they all work pretty much the
>>> same way?  (I want to make sure no others will be adversely affected
>>> by this change.)
>>
>> I tested it on SAMA7G5, SAMA5D2 and SAM9X60. I tested the suspend/resume
>> to/from mem. On SAMA5D2 and SAM9X60 there is no clock provided by
>> transceiver A to OHCI. I encountered no issues on tested systems. These IPs
>> are also present on SAMA5D3 and SAMA5D4 systems which I haven't tested as I
>> expect to behave as SAMA5D2 (as the clocking scheme is the same with
>> SAMA5D2). I can also try it on a SAMA5D3 (I don't have a SAMA5D4 with me at
>> the moment), tough, just to be sure nothing is broken there too.
> 
> That doesn't answer my question.  I asked if there were any systems
> which might be affected by your patch, and you listed a bunch of
> systems that _aren't_ affected (that is, they continue to work
> properly).

I wrongly understood the initial question.

> 
> What systems might run into trouble with this patch?

These are all I haven't tested and might be affected:
AT91RM9200,
SAM9260,
SAM9261,
SAM9263,
SAM9N12,
SAM9X35,
SAM9G45.

The last two (SAM9X35 and SAM9G45) have the same clocking scheme with
SAMA5D2 (which I tested). For the rest of them I cannot find the clocking
scheme in datasheets and don't have them to test (at least at the moment).

Thank you,
Claudiu Beznea

> 
> Alan Stern
>
Alan Stern June 30, 2021, 6:21 p.m. UTC | #14
On Wed, Jun 30, 2021 at 02:46:47PM +0000, Claudiu.Beznea@microchip.com wrote:
> On 24.06.2021 16:23, Alan Stern wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Thu, Jun 24, 2021 at 06:40:25AM +0000, Claudiu.Beznea@microchip.com wrote:
> >> On 23.06.2021 19:41, Alan Stern wrote:
> >>> Are there any systems beside the SAMA7G5 and others you tested which
> >>> might be affected by this patch?  Do they all work pretty much the
> >>> same way?  (I want to make sure no others will be adversely affected
> >>> by this change.)
> >>
> >> I tested it on SAMA7G5, SAMA5D2 and SAM9X60. I tested the suspend/resume
> >> to/from mem. On SAMA5D2 and SAM9X60 there is no clock provided by
> >> transceiver A to OHCI. I encountered no issues on tested systems. These IPs
> >> are also present on SAMA5D3 and SAMA5D4 systems which I haven't tested as I
> >> expect to behave as SAMA5D2 (as the clocking scheme is the same with
> >> SAMA5D2). I can also try it on a SAMA5D3 (I don't have a SAMA5D4 with me at
> >> the moment), tough, just to be sure nothing is broken there too.
> > 
> > That doesn't answer my question.  I asked if there were any systems
> > which might be affected by your patch, and you listed a bunch of
> > systems that _aren't_ affected (that is, they continue to work
> > properly).
> 
> I wrongly understood the initial question.
> 
> > 
> > What systems might run into trouble with this patch?
> 
> These are all I haven't tested and might be affected:
> AT91RM9200,
> SAM9260,
> SAM9261,
> SAM9263,
> SAM9N12,
> SAM9X35,
> SAM9G45.
> 
> The last two (SAM9X35 and SAM9G45) have the same clocking scheme with
> SAMA5D2 (which I tested). For the rest of them I cannot find the clocking
> scheme in datasheet and don't have them to test (at least at the moment).

I see.  That seems reasonable; the others are probably the same as the 
ones you tested.

Did you ever answer the question that Nicolas raised back on June 9 in:

	https://marc.info/?l=linux-usb&m=162324242003349&w=2

Alan Stern
Claudiu Beznea July 1, 2021, 5:45 a.m. UTC | #15
On 30.06.2021 21:21, Alan Stern wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Jun 30, 2021 at 02:46:47PM +0000, Claudiu.Beznea@microchip.com wrote:
>> On 24.06.2021 16:23, Alan Stern wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Thu, Jun 24, 2021 at 06:40:25AM +0000, Claudiu.Beznea@microchip.com wrote:
>>>> On 23.06.2021 19:41, Alan Stern wrote:
>>>>> Are there any systems beside the SAMA7G5 and others you tested which
>>>>> might be affected by this patch?  Do they all work pretty much the
>>>>> same way?  (I want to make sure no others will be adversely affected
>>>>> by this change.)
>>>>
>>>> I tested it on SAMA7G5, SAMA5D2 and SAM9X60. I tested the suspend/resume
>>>> to/from mem. On SAMA5D2 and SAM9X60 there is no clock provided by
>>>> transceiver A to OHCI. I encountered no issues on tested systems. These IPs
>>>> are also present on SAMA5D3 and SAMA5D4 systems which I haven't tested as I
>>>> expect to behave as SAMA5D2 (as the clocking scheme is the same with
>>>> SAMA5D2). I can also try it on a SAMA5D3 (I don't have a SAMA5D4 with me at
>>>> the moment), tough, just to be sure nothing is broken there too.
>>>
>>> That doesn't answer my question.  I asked if there were any systems
>>> which might be affected by your patch, and you listed a bunch of
>>> systems that _aren't_ affected (that is, they continue to work
>>> properly).
>>
>> I wrongly understood the initial question.
>>
>>>
>>> What systems might run into trouble with this patch?
>>
>> These are all I haven't tested and might be affected:
>> AT91RM9200,
>> SAM9260,
>> SAM9261,
>> SAM9263,
>> SAM9N12,
>> SAM9X35,
>> SAM9G45.
>>
>> The last two (SAM9X35 and SAM9G45) have the same clocking scheme with
>> SAMA5D2 (which I tested). For the rest of them I cannot find the clocking
>> scheme in datasheet and don't have them to test (at least at the moment).
> 
> I see.  That seems reasonable; the others are probably the same as the
> ones you tested.
> 
> Did you ever answer the question that Nicolas raised back on June 9 in:
> 
>         https://marc.info/?l=linux-usb&m=162324242003349&w=2

Not directly. I replied previously in this thread "For run-time control
(via ohci_at91_hub_control()), I agree with you that
the current implemented approach is not healthy (taking into account the
clock scheme above) and the fact that we do force the ports suspend on
ohci_at91_hub_control()". Nicolas was referring to ohci_at91_port_suspend()
calls in ohci_at91_hub_control() so I agreed with him that work might need
to be done also for this function.

Thank you,
Claudiu Beznea


> 
> Alan Stern
>
Alan Stern July 1, 2021, 2:01 p.m. UTC | #16
On Thu, Jul 01, 2021 at 05:45:50AM +0000, Claudiu.Beznea@microchip.com wrote:
> On 30.06.2021 21:21, Alan Stern wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Wed, Jun 30, 2021 at 02:46:47PM +0000, Claudiu.Beznea@microchip.com wrote:
> >> On 24.06.2021 16:23, Alan Stern wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>
> >>> On Thu, Jun 24, 2021 at 06:40:25AM +0000, Claudiu.Beznea@microchip.com wrote:
> >>>> On 23.06.2021 19:41, Alan Stern wrote:
> >>>>> Are there any systems beside the SAMA7G5 and others you tested which
> >>>>> might be affected by this patch?  Do they all work pretty much the
> >>>>> same way?  (I want to make sure no others will be adversely affected
> >>>>> by this change.)
> >>>>
> >>>> I tested it on SAMA7G5, SAMA5D2 and SAM9X60. I tested the suspend/resume
> >>>> to/from mem. On SAMA5D2 and SAM9X60 there is no clock provided by
> >>>> transceiver A to OHCI. I encountered no issues on tested systems. These IPs
> >>>> are also present on SAMA5D3 and SAMA5D4 systems which I haven't tested as I
> >>>> expect to behave as SAMA5D2 (as the clocking scheme is the same with
> >>>> SAMA5D2). I can also try it on a SAMA5D3 (I don't have a SAMA5D4 with me at
> >>>> the moment), tough, just to be sure nothing is broken there too.
> >>>
> >>> That doesn't answer my question.  I asked if there were any systems
> >>> which might be affected by your patch, and you listed a bunch of
> >>> systems that _aren't_ affected (that is, they continue to work
> >>> properly).
> >>
> >> I wrongly understood the initial question.
> >>
> >>>
> >>> What systems might run into trouble with this patch?
> >>
> >> These are all I haven't tested and might be affected:
> >> AT91RM9200,
> >> SAM9260,
> >> SAM9261,
> >> SAM9263,
> >> SAM9N12,
> >> SAM9X35,
> >> SAM9G45.
> >>
> >> The last two (SAM9X35 and SAM9G45) have the same clocking scheme with
> >> SAMA5D2 (which I tested). For the rest of them I cannot find the clocking
> >> scheme in datasheet and don't have them to test (at least at the moment).
> > 
> > I see.  That seems reasonable; the others are probably the same as the
> > ones you tested.
> > 
> > Did you ever answer the question that Nicolas raised back on June 9 in:
> > 
> >         https://marc.info/?l=linux-usb&m=162324242003349&w=2
> 
> Not directly. I replied previously in this thread "For run-time control
> (via ohci_at91_hub_control()), I agree with you that
> the current implemented approach is not healthy (taking into account the
> clock scheme above) and the fact that we do force the ports suspend on
> ohci_at91_hub_control()". Nicolas was referring to ohci_at91_port_suspend()
> calls in ohci_at91_hub_control() so I agreed with him that work might need
> to be done also for this function.

All right.  I guess this is the best that can be done at this time.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Greg KH may not have the original patch still in his queue.  And in any 
case he'll probably ask you to resubmit it after the current merge 
window ends.

Alan Stern
Greg Kroah-Hartman July 21, 2021, 8:08 a.m. UTC | #17
On Wed, Jun 09, 2021 at 03:10:27PM +0300, Claudiu Beznea wrote:
> On SAMA7G5 suspending ports will cut the access to OHCI registers and
> any subsequent access to them will lead to CPU being blocked trying to
> access that memory. Same thing happens on resume: if OHCI memory is
> accessed before resuming ports the CPU will block on that access. The
> OCHI memory is accessed on suspend/resume though
> ohci_suspend()/ohci_resume().
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
> 
> The patch was tested on SAMA7G5, SAMA5D2 and SAM9X60.

This does not apply to 5.14-rc2, can you please rebase and resend?

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
index b3a6a497dcb1..7c6202b05ff4 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -666,8 +666,6 @@  ohci_hcd_at91_drv_suspend(struct device *dev)
 	if (ohci_at91->wakeup)
 		enable_irq_wake(hcd->irq);
 
-	ohci_at91_port_suspend(ohci_at91, 1);
-
 	ret = ohci_suspend(hcd, ohci_at91->wakeup);
 	if (ret) {
 		if (ohci_at91->wakeup)
@@ -687,7 +685,10 @@  ohci_hcd_at91_drv_suspend(struct device *dev)
 		/* flush the writes */
 		(void) ohci_readl (ohci, &ohci->regs->control);
 		msleep(1);
+		ohci_at91_port_suspend(ohci_at91, 1);
 		at91_stop_clock(ohci_at91);
+	} else {
+		ohci_at91_port_suspend(ohci_at91, 1);
 	}
 
 	return ret;
@@ -699,6 +700,8 @@  ohci_hcd_at91_drv_resume(struct device *dev)
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct ohci_at91_priv *ohci_at91 = hcd_to_ohci_at91_priv(hcd);
 
+	ohci_at91_port_suspend(ohci_at91, 0);
+
 	if (ohci_at91->wakeup)
 		disable_irq_wake(hcd->irq);
 	else
@@ -706,8 +709,6 @@  ohci_hcd_at91_drv_resume(struct device *dev)
 
 	ohci_resume(hcd, false);
 
-	ohci_at91_port_suspend(ohci_at91, 0);
-
 	return 0;
 }