[6/6] usb/ohci: Implement resume on connection status change

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

On Tue, 31 Jan 2023 at 19:39, BALATON Zoltan <balaton@eik.bme.hu> 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 c5bec4e4d7..7f98ab8924 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -1401,6 +1401,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.
> @@ -1417,7 +1429,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;
>      }
> @@ -1820,11 +1835,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 */

I think we should retain this comment in the factored-out function.

> -        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.

Otherwise I'll leave this one for Gerd to review since it's
USB spec stuff.

thanks
-- PMM

On Tue, 7 Feb 2023, Peter Maydell wrote:
> On Tue, 31 Jan 2023 at 19:39, BALATON Zoltan <balaton@eik.bme.hu> 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 c5bec4e4d7..7f98ab8924 100644
>> --- a/hw/usb/hcd-ohci.c
>> +++ b/hw/usb/hcd-ohci.c
>> @@ -1401,6 +1401,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.
>> @@ -1417,7 +1429,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;
>>      }
>> @@ -1820,11 +1835,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 */
>
> I think we should retain this comment in the factored-out function.

I did retain it. It's just above the function header.

I'll do the check for the reg names array and also add Philippe's patch 
for typo fix unless Gerd wants some more changes so I wait a bit more for 
comments.

Regards,
BALATON Zoltan

On Tue, 7 Feb 2023 at 17:41, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Tue, 7 Feb 2023, Peter Maydell wrote:
> > I think we should retain this comment in the factored-out function.
>
> I did retain it. It's just above the function header.

Oops, sorry, was scanning the patch too quickly.

thanks
-- PMM