| 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 |
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; > } > >
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
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 >
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
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 >
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
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 >
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
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 >
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 >> >
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
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 >
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 >
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
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 >
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
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 --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; }
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(-)