Re: [PATCH v2 6/7] usb/ohci: Implement resume on connection status change

[BALATON Zoltan]

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.