Re: [cdi-devel] [PATCH 2/5] ehci: Implement interrupt transfers

[Kevin Wolf <kevin@xxxxxxxxxx>]

On Tue, Dec 29, 2015 at 01:00:51AM +0100, Max Reitz wrote:
> Signed-off-by: Max Reitz <max@xxxxxxxxxx>
> ---
>  ehci/ehci.c | 265 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  ehci/ehci.h |  20 +++++
>  ehci/main.c |  11 +--
>  3 files changed, 278 insertions(+), 18 deletions(-)
> 
> diff --git a/ehci/ehci.c b/ehci/ehci.c
> index addec00..4b548ae 100644
> --- a/ehci/ehci.c
> +++ b/ehci/ehci.c
> @@ -40,6 +40,13 @@
>  
>  static void bios_handoff(struct cdi_pci_device *pci_dev, struct ehci *hc);
>  static void irq_handler(struct cdi_device *dev);
> +static void init_periodic_pseudo_qhs(struct ehci *hc);
> +
> +
> +static inline bool is_power_of_two(unsigned value)
> +{
> +    return !(value & (value - 1));
> +}
>  
>  
>  struct cdi_device *ehci_init_device(struct cdi_bus_data *bus_data)
> @@ -111,6 +118,18 @@ struct cdi_device *ehci_init_device(struct cdi_bus_data *bus_data)
>      }
>      hc->regs->periodiclistbase = hc->periodic_list_mem->paddr.items[0].start;
>  
> +    hc->periodic_qhs = calloc(1024, sizeof(*hc->periodic_qhs));
> +    // 10 = |0..10| = |ld(1)..ld(1024)|

I gues you mean "// 11 = ..."?

> +    hc->periodic_pseudo_qhs_mem =
> +        cdi_mem_alloc(11 * sizeof(hc->periodic_pseudo_qhs[0]),
> +                      6
> +                      | CDI_MEM_PHYS_CONTIGUOUS
> +                      | CDI_MEM_DMA_4G
> +                      | CDI_MEM_NOINIT);
> +    hc->periodic_pseudo_qhs = hc->periodic_pseudo_qhs_mem->vaddr;
> +
> +    init_periodic_pseudo_qhs(hc);
> +
>      // Create anchor
>      struct cdi_mem_area *anchor_qh;
>      anchor_qh = cdi_mem_alloc(sizeof(ehci_fat_qh_t), 6

> @@ -323,7 +442,28 @@ static void enter_async_qh(struct ehci *hc, ehci_fat_qh_t *qh)
>  }
>  
>  
> -static void retire_qh(struct ehci *hc, ehci_fat_qh_t *qh)
> +static void enter_interrupt_qh(struct ehci *hc, ehci_fat_qh_t *qh,
> +                               int frame_interval_shift)
> +{
> +    qh->next = hc->periodic_functional_qhs;

This value...

> +    hc->periodic_functional_qhs = qh;
> +
> +    ehci_fat_qh_t *pseudo_qh = &hc->periodic_pseudo_qhs[frame_interval_shift];
> +
> +    qh->next = pseudo_qh->next;

...is immediately overwritten here.

As I don't understand yet what functional and pseudo QHs are and the
header doesn't tell anything about it, I can't say how bad this is, but
we seem to throw away all of the QHs in hc->periodic_functional_qhs and
replace them by qh.

My guess is that the first assignment should be qh->next_periodic.

> +    qh->previous = pseudo_qh;
> +
> +    pseudo_qh->next = qh;
> +    if (qh->next) {
> +        qh->next->previous = qh;
> +    }
> +
> +    qh->qh.next_qh = pseudo_qh->qh.next_qh;
> +    pseudo_qh->qh.next_qh = qh->paddr | EHCI_QH;
> +}
> +
> +
> +static void retire_async_qh(struct ehci *hc, ehci_fat_qh_t *qh)
>  {
>      qh->previous->qh.next_qh = qh->qh.next_qh;
>  
> @@ -336,11 +476,29 @@ static void retire_qh(struct ehci *hc, ehci_fat_qh_t *qh)
>  }
>  
>  
> -static void retire_first_qtd(ehci_fat_qh_t *qh)
> +static void retire_periodic_qh(struct ehci *hc, ehci_fat_qh_t *qh)
>  {
> -    ehci_fat_qtd_t *qtd = qh->first_qtd;
> +    qh->previous->qh.next_qh = qh->qh.next_qh;
>  
> -    assert(qtd);
> +    qh->previous->next = qh->next;
> +    if (qh->next) {
> +        qh->next->previous = qh->previous;
> +    }
> +
> +    ehci_fat_qh_t **func_periodic_ptr = &hc->periodic_functional_qhs;
> +    while (*func_periodic_ptr && *func_periodic_ptr != qh) {
> +        func_periodic_ptr = &(*func_periodic_ptr)->next_periodic;
> +    }
> +    assert(*func_periodic_ptr == qh);
> +    *func_periodic_ptr = qh->next_periodic;
> +
> +    cdi_list_push(hc->retired_qhs, qh);

I think this will only be processed when the next async QH is retired.
Is this good enough?

> +}
> +
> +
> +static void qtd_write_back(ehci_fat_qtd_t *qtd)
> +{
> +    ehci_fat_qh_t *qh = qtd->qh;
>  
>      uint32_t err_mask = (qh->qh.ep_state[0] & EHCI_QHES0_EPS_MASK)
>                           == EHCI_QHES0_EPS_HS

> diff --git a/ehci/ehci.h b/ehci/ehci.h
> index fb8dd9e..4b55391 100644
> --- a/ehci/ehci.h
> +++ b/ehci/ehci.h
> @@ -87,8 +87,14 @@ struct ehci_fat_qh {
>      struct cdi_mem_area *cdi_mem_area;
>      uintptr_t paddr;
>  
> +    cdi_usb_hc_interrupt_cb_t cb;
> +    void *cb_opaque;

Maybe comment that cb != NULL if, and only if, this is a periodic QH?

> +
>      ehci_fat_qh_t *next, *previous;
> +    ehci_fat_qh_t *next_periodic;
>      ehci_fat_qtd_t *first_qtd, *last_qtd;
> +
> +    bool is_pseudo_qh;
>  } __attribute__((aligned(64)));
>  
>  static_assert(!(sizeof(struct ehci_fat_qh) % 64),
> @@ -122,9 +128,17 @@ struct ehci {
>  
>      struct cdi_mem_area *periodic_list_mem;
>      volatile uint32_t *periodic_list;
> +    ehci_fat_qh_t **periodic_qhs;
> +
> +    struct cdi_mem_area *periodic_pseudo_qhs_mem;
> +    ehci_fat_qh_t *periodic_pseudo_qhs;
> +    ehci_fat_qh_t *periodic_functional_qhs;

As I mentioned above, a comment explaining these wouldn't hurt either.

After reading the rest of the patch I have the impression that the job
of the pseudo QHs is just to sit there permanently so that the
(transient) functional QHs have a place to attach to. I'm not sure,
though.

>      ehci_fat_qh_t *async_start;
>      cdi_list_t retired_qhs;
> +
> +    // For a bit of load balancing
> +    int periodic_start_microframe;
>  };
>  
>  #define PCI_CLASS_SERIAL_BUS_CONTROLLER     0x0c

Kevin