| Message ID | 35c4d4ccf2f73e6a87cdbd28fb6a1b33de72ed74.1676916640.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | OHCI changes | expand |
On 20/2/23 19:19, BALATON Zoltan wrote: > If certain bit is set remote wake up should change state from > suspended to resume and generate interrupt. There was a todo comment > for this, implement that by moving existing resume logic to a function > and call that. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > hw/usb/hcd-ohci.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c > index bad8db7b1d..88bd42b14a 100644 > --- a/hw/usb/hcd-ohci.c > +++ b/hw/usb/hcd-ohci.c > @@ -1410,6 +1410,18 @@ static void ohci_set_hub_status(OHCIState *ohci, uint32_t val) > } > } > > +/* This is the one state transition the controller can do by itself */ > +static int ohci_resume(OHCIState *s) Preferably returning bool. > +{ > + if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) { > + trace_usb_ohci_remote_wakeup(s->name); > + s->ctl &= ~OHCI_CTL_HCFS; > + s->ctl |= OHCI_USB_RESUME; > + return 1; > + } > + return 0; > +} > + > /* > * Sets a flag in a port status reg but only set it if the port is connected. > * If not set ConnectStatusChange flag. If flag is enabled return 1. > @@ -1426,7 +1438,10 @@ static int ohci_port_set_if_connected(OHCIState *ohci, int i, uint32_t val) > if (!(ohci->rhport[i].ctrl & OHCI_PORT_CCS)) { > ohci->rhport[i].ctrl |= OHCI_PORT_CSC; // ConnectStatusChange > if (ohci->rhstatus & OHCI_RHS_DRWE) { // DeviceRemoteWakeupEnable: ConnectStatusChange is a remote wakeup event. > - /* TODO: CSC is a wakeup event */ > + /* CSC is a wakeup event */ > + if (ohci_resume(ohci)) { > + ohci_set_interrupt(ohci, OHCI_INTR_RD); Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Gerd, if you Ack I can queue this. > + } > } > return 0; > } > @@ -1828,11 +1843,7 @@ static void ohci_wakeup(USBPort *port1) > intr = OHCI_INTR_RHSC; > } > /* Note that the controller can be suspended even if this port is not */ > - if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) { > - trace_usb_ohci_remote_wakeup(s->name); > - /* This is the one state transition the controller can do by itself */ > - s->ctl &= ~OHCI_CTL_HCFS; > - s->ctl |= OHCI_USB_RESUME; > + if (ohci_resume(s)) { > /* > * In suspend mode only ResumeDetected is possible, not RHSC: > * see the OHCI spec 5.1.2.3.
> > - /* TODO: CSC is a wakeup event */ > > + /* CSC is a wakeup event */ > > + if (ohci_resume(ohci)) { > > + ohci_set_interrupt(ohci, OHCI_INTR_RD); > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > Gerd, if you Ack I can queue this. Looks good to me (whole series). Acked-by: Gerd Hoffmann <kraxel@redhat.com> thanks, Gerd
On Tue, 28 Feb 2023, Philippe Mathieu-Daudé wrote: > On 20/2/23 19:19, BALATON Zoltan wrote: >> If certain bit is set remote wake up should change state from >> suspended to resume and generate interrupt. There was a todo comment >> for this, implement that by moving existing resume logic to a function >> and call that. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/usb/hcd-ohci.c | 23 +++++++++++++++++------ >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c >> index bad8db7b1d..88bd42b14a 100644 >> --- a/hw/usb/hcd-ohci.c >> +++ b/hw/usb/hcd-ohci.c >> @@ -1410,6 +1410,18 @@ static void ohci_set_hub_status(OHCIState *ohci, >> uint32_t val) >> } >> } >> +/* This is the one state transition the controller can do by itself */ >> +static int ohci_resume(OHCIState *s) > > Preferably returning bool. I can change that on rebase. I just followed other exising functions in this file for consistency which return int (although some use 1 and others use -1 besides 0). >> +{ >> + if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) { >> + trace_usb_ohci_remote_wakeup(s->name); >> + s->ctl &= ~OHCI_CTL_HCFS; >> + s->ctl |= OHCI_USB_RESUME; >> + return 1; >> + } >> + return 0; >> +} >> + >> /* >> * Sets a flag in a port status reg but only set it if the port is >> connected. >> * If not set ConnectStatusChange flag. If flag is enabled return 1. >> @@ -1426,7 +1438,10 @@ static int ohci_port_set_if_connected(OHCIState >> *ohci, int i, uint32_t val) >> if (!(ohci->rhport[i].ctrl & OHCI_PORT_CCS)) { >> ohci->rhport[i].ctrl |= OHCI_PORT_CSC; > > // ConnectStatusChange > >> if (ohci->rhstatus & OHCI_RHS_DRWE) { > > // DeviceRemoteWakeupEnable: ConnectStatusChange is a remote wakeup event. Not clear if you want any change here or the comments are just explanation in this email. >> - /* TODO: CSC is a wakeup event */ >> + /* CSC is a wakeup event */ >> + if (ohci_resume(ohci)) { >> + ohci_set_interrupt(ohci, OHCI_INTR_RD); > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Thanks for the review. You put a lot of work in QEMU and we appreciate very much that you're also doing the job of other maintainers. Regards, BALATON Zoltan > Gerd, if you Ack I can queue this. > >> + } >> } >> return 0; >> } >> @@ -1828,11 +1843,7 @@ static void ohci_wakeup(USBPort *port1) >> intr = OHCI_INTR_RHSC; >> } >> /* Note that the controller can be suspended even if this port is not >> */ >> - if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) { >> - trace_usb_ohci_remote_wakeup(s->name); >> - /* This is the one state transition the controller can do by >> itself */ >> - s->ctl &= ~OHCI_CTL_HCFS; >> - s->ctl |= OHCI_USB_RESUME; >> + if (ohci_resume(s)) { >> /* >> * In suspend mode only ResumeDetected is possible, not RHSC: >> * see the OHCI spec 5.1.2.3. > >
On 28/2/23 15:55, BALATON Zoltan wrote: > On Tue, 28 Feb 2023, Philippe Mathieu-Daudé wrote: >> On 20/2/23 19:19, BALATON Zoltan wrote: >>> If certain bit is set remote wake up should change state from >>> suspended to resume and generate interrupt. There was a todo comment >>> for this, implement that by moving existing resume logic to a function >>> and call that. >>> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> --- >>> hw/usb/hcd-ohci.c | 23 +++++++++++++++++------ >>> 1 file changed, 17 insertions(+), 6 deletions(-) >>> >>> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c >>> index bad8db7b1d..88bd42b14a 100644 >>> --- a/hw/usb/hcd-ohci.c >>> +++ b/hw/usb/hcd-ohci.c >>> @@ -1410,6 +1410,18 @@ static void ohci_set_hub_status(OHCIState >>> *ohci, uint32_t val) >>> } >>> } >>> +/* This is the one state transition the controller can do by >>> itself */ >>> +static int ohci_resume(OHCIState *s) >> >> Preferably returning bool. > > I can change that on rebase. I just followed other exising functions in > this file for consistency which return int (although some use 1 and > others use -1 besides 0). I'll squash that myself. >>> +{ >>> + if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) { >>> + trace_usb_ohci_remote_wakeup(s->name); >>> + s->ctl &= ~OHCI_CTL_HCFS; >>> + s->ctl |= OHCI_USB_RESUME; >>> + return 1; >>> + } >>> + return 0; >>> +} >>> + >>> /* >>> * Sets a flag in a port status reg but only set it if the port is >>> connected. >>> * If not set ConnectStatusChange flag. If flag is enabled return 1. >>> @@ -1426,7 +1438,10 @@ static int >>> ohci_port_set_if_connected(OHCIState *ohci, int i, uint32_t val) >>> if (!(ohci->rhport[i].ctrl & OHCI_PORT_CCS)) { >>> ohci->rhport[i].ctrl |= OHCI_PORT_CSC; >> >> // ConnectStatusChange >> >>> if (ohci->rhstatus & OHCI_RHS_DRWE) { >> >> // DeviceRemoteWakeupEnable: ConnectStatusChange is a remote wakeup >> event. > > Not clear if you want any change here or the comments are just > explanation in this email. I was just taking notes while reviewing ;) Our OHCI definitions aren't very verbose. >>> - /* TODO: CSC is a wakeup event */ >>> + /* CSC is a wakeup event */ >>> + if (ohci_resume(ohci)) { >>> + ohci_set_interrupt(ohci, OHCI_INTR_RD); >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > Thanks for the review. You put a lot of work in QEMU and we appreciate > very much that you're also doing the job of other maintainers. No problem. I'm queuing this patch for my next PR (hopefully much less patches, and before the freeze).
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c index bad8db7b1d..88bd42b14a 100644 --- a/hw/usb/hcd-ohci.c +++ b/hw/usb/hcd-ohci.c @@ -1410,6 +1410,18 @@ static void ohci_set_hub_status(OHCIState *ohci, uint32_t val) } } +/* This is the one state transition the controller can do by itself */ +static int ohci_resume(OHCIState *s) +{ + if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) { + trace_usb_ohci_remote_wakeup(s->name); + s->ctl &= ~OHCI_CTL_HCFS; + s->ctl |= OHCI_USB_RESUME; + return 1; + } + return 0; +} + /* * Sets a flag in a port status reg but only set it if the port is connected. * If not set ConnectStatusChange flag. If flag is enabled return 1. @@ -1426,7 +1438,10 @@ static int ohci_port_set_if_connected(OHCIState *ohci, int i, uint32_t val) if (!(ohci->rhport[i].ctrl & OHCI_PORT_CCS)) { ohci->rhport[i].ctrl |= OHCI_PORT_CSC; if (ohci->rhstatus & OHCI_RHS_DRWE) { - /* TODO: CSC is a wakeup event */ + /* CSC is a wakeup event */ + if (ohci_resume(ohci)) { + ohci_set_interrupt(ohci, OHCI_INTR_RD); + } } return 0; } @@ -1828,11 +1843,7 @@ static void ohci_wakeup(USBPort *port1) intr = OHCI_INTR_RHSC; } /* Note that the controller can be suspended even if this port is not */ - if ((s->ctl & OHCI_CTL_HCFS) == OHCI_USB_SUSPEND) { - trace_usb_ohci_remote_wakeup(s->name); - /* This is the one state transition the controller can do by itself */ - s->ctl &= ~OHCI_CTL_HCFS; - s->ctl |= OHCI_USB_RESUME; + if (ohci_resume(s)) { /* * In suspend mode only ResumeDetected is possible, not RHSC: * see the OHCI spec 5.1.2.3.
If certain bit is set remote wake up should change state from suspended to resume and generate interrupt. There was a todo comment for this, implement that by moving existing resume logic to a function and call that. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/usb/hcd-ohci.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)