Re: [Qemu-devel] [PATCH 04/13] usb-ohci: Use universal DMA helper functions

[Benjamin Herrenschmidt]

> > Cc: Gerd Hoffmann<address@hidden>
> > Cc: Michael S. Tsirkin<address@hidden>
> >
> > Signed-off-by: David Gibson<address@hidden>
> > Signed-off-by: Benjamin Herrenschmidt<address@hidden>
> 
> 
> So...  the DMA api is designed to allow for partial result returns which I 
> presume an implementation would use as a simplification.
> 
> But none of these callers actually check the return code?
> 
> Either errors are important and we need to adjust callees to check them or 
> errors aren't important and we should return void.

Errors should matter and I agree callers should check them, this is the
series I inherited and I believe it does need improvements in those
areas (though it would be easier if the driver authors were the one to
do those improvements).

> Why leave pci accessors and not implement usb_memory_rw() wrappers?

Well, "usb" is a bit too generic, ehci and ohci would each need to have
their own sets of wrappers. But yes, that's possible... is it really
worth it ? There's nothing fundamentally wrong with using the dma_*
accessors.

Cheers,
Ben.

> Regards,
> 
> Anthony Liguori
> 
> > ---
> >   hw/usb/hcd-ohci.c |   93 
> > +++++++++++++++++++++++++++++------------------------
> >   1 file changed, 51 insertions(+), 42 deletions(-)
> >
> > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> > index 1a1cc88..844e7ed 100644
> > --- a/hw/usb/hcd-ohci.c
> > +++ b/hw/usb/hcd-ohci.c
> > @@ -31,7 +31,7 @@
> >   #include "hw/usb.h"
> >   #include "hw/pci.h"
> >   #include "hw/sysbus.h"
> > -#include "hw/qdev-addr.h"
> > +#include "hw/qdev-dma.h"
> >
> >   //#define DEBUG_OHCI
> >   /* Dump packet contents.  */
> > @@ -62,6 +62,7 @@ typedef struct {
> >       USBBus bus;
> >       qemu_irq irq;
> >       MemoryRegion mem;
> > +    DMAContext *dma;
> >       int num_ports;
> >       const char *name;
> >
> > @@ -104,7 +105,7 @@ typedef struct {
> >       uint32_t htest;
> >
> >       /* SM501 local memory offset */
> > -    target_phys_addr_t localmem_base;
> > +    dma_addr_t localmem_base;
> >
> >       /* Active packets.  */
> >       uint32_t old_ctl;
> > @@ -482,14 +483,14 @@ static void ohci_reset(void *opaque)
> >
> >   /* Get an array of dwords from main memory */
> >   static inline int get_dwords(OHCIState *ohci,
> > -                             uint32_t addr, uint32_t *buf, int num)
> > +                             dma_addr_t addr, uint32_t *buf, int num)
> >   {
> >       int i;
> >
> >       addr += ohci->localmem_base;
> >
> >       for (i = 0; i<  num; i++, buf++, addr += sizeof(*buf)) {
> > -        cpu_physical_memory_read(addr, buf, sizeof(*buf));
> > +        dma_memory_read(ohci->dma, addr, buf, sizeof(*buf));
> >           *buf = le32_to_cpu(*buf);
> >       }
> >
> > @@ -498,7 +499,7 @@ static inline int get_dwords(OHCIState *ohci,
> >
> >   /* Put an array of dwords in to main memory */
> >   static inline int put_dwords(OHCIState *ohci,
> > -                             uint32_t addr, uint32_t *buf, int num)
> > +                             dma_addr_t addr, uint32_t *buf, int num)
> >   {
> >       int i;
> >
> > @@ -506,7 +507,7 @@ static inline int put_dwords(OHCIState *ohci,
> >
> >       for (i = 0; i<  num; i++, buf++, addr += sizeof(*buf)) {
> >           uint32_t tmp = cpu_to_le32(*buf);
> > -        cpu_physical_memory_write(addr,&tmp, sizeof(tmp));
> > +        dma_memory_write(ohci->dma, addr,&tmp, sizeof(tmp));
> >       }
> >
> >       return 1;
> > @@ -514,14 +515,14 @@ static inline int put_dwords(OHCIState *ohci,
> >
> >   /* Get an array of words from main memory */
> >   static inline int get_words(OHCIState *ohci,
> > -                            uint32_t addr, uint16_t *buf, int num)
> > +                            dma_addr_t addr, uint16_t *buf, int num)
> >   {
> >       int i;
> >
> >       addr += ohci->localmem_base;
> >
> >       for (i = 0; i<  num; i++, buf++, addr += sizeof(*buf)) {
> > -        cpu_physical_memory_read(addr, buf, sizeof(*buf));
> > +        dma_memory_read(ohci->dma, addr, buf, sizeof(*buf));
> >           *buf = le16_to_cpu(*buf);
> >       }
> >
> > @@ -530,7 +531,7 @@ static inline int get_words(OHCIState *ohci,
> >
> >   /* Put an array of words in to main memory */
> >   static inline int put_words(OHCIState *ohci,
> > -                            uint32_t addr, uint16_t *buf, int num)
> > +                            dma_addr_t addr, uint16_t *buf, int num)
> >   {
> >       int i;
> >
> > @@ -538,40 +539,40 @@ static inline int put_words(OHCIState *ohci,
> >
> >       for (i = 0; i<  num; i++, buf++, addr += sizeof(*buf)) {
> >           uint16_t tmp = cpu_to_le16(*buf);
> > -        cpu_physical_memory_write(addr,&tmp, sizeof(tmp));
> > +        dma_memory_write(ohci->dma, addr,&tmp, sizeof(tmp));
> >       }
> >
> >       return 1;
> >   }
> >
> >   static inline int ohci_read_ed(OHCIState *ohci,
> > -                               uint32_t addr, struct ohci_ed *ed)
> > +                               dma_addr_t addr, struct ohci_ed *ed)
> >   {
> >       return get_dwords(ohci, addr, (uint32_t *)ed, sizeof(*ed)>>  2);
> >   }
> >
> >   static inline int ohci_read_td(OHCIState *ohci,
> > -                               uint32_t addr, struct ohci_td *td)
> > +                               dma_addr_t addr, struct ohci_td *td)
> >   {
> >       return get_dwords(ohci, addr, (uint32_t *)td, sizeof(*td)>>  2);
> >   }
> >
> >   static inline int ohci_read_iso_td(OHCIState *ohci,
> > -                                   uint32_t addr, struct ohci_iso_td *td)
> > +                                   dma_addr_t addr, struct ohci_iso_td *td)
> >   {
> >       return (get_dwords(ohci, addr, (uint32_t *)td, 4)&&
> >               get_words(ohci, addr + 16, td->offset, 8));
> >   }
> >
> >   static inline int ohci_read_hcca(OHCIState *ohci,
> > -                                 uint32_t addr, struct ohci_hcca *hcca)
> > +                                 dma_addr_t addr, struct ohci_hcca *hcca)
> >   {
> > -    cpu_physical_memory_read(addr + ohci->localmem_base, hcca, 
> > sizeof(*hcca));
> > +    dma_memory_read(ohci->dma, addr + ohci->localmem_base, hcca, 
> > sizeof(*hcca));
> >       return 1;
> >   }
> >
> >   static inline int ohci_put_ed(OHCIState *ohci,
> > -                              uint32_t addr, struct ohci_ed *ed)
> > +                              dma_addr_t addr, struct ohci_ed *ed)
> >   {
> >       /* ed->tail is under control of the HCD.
> >        * Since just ed->head is changed by HC, just write back this
> > @@ -583,64 +584,63 @@ static inline int ohci_put_ed(OHCIState *ohci,
> >   }
> >
> >   static inline int ohci_put_td(OHCIState *ohci,
> > -                              uint32_t addr, struct ohci_td *td)
> > +                              dma_addr_t addr, struct ohci_td *td)
> >   {
> >       return put_dwords(ohci, addr, (uint32_t *)td, sizeof(*td)>>  2);
> >   }
> >
> >   static inline int ohci_put_iso_td(OHCIState *ohci,
> > -                                  uint32_t addr, struct ohci_iso_td *td)
> > +                                  dma_addr_t addr, struct ohci_iso_td *td)
> >   {
> >       return (put_dwords(ohci, addr, (uint32_t *)td, 4)&&
> >               put_words(ohci, addr + 16, td->offset, 8));
> >   }
> >
> >   static inline int ohci_put_hcca(OHCIState *ohci,
> > -                                uint32_t addr, struct ohci_hcca *hcca)
> > +                                dma_addr_t addr, struct ohci_hcca *hcca)
> >   {
> > -    cpu_physical_memory_write(addr + ohci->localmem_base + 
> > HCCA_WRITEBACK_OFFSET,
> > -                              (char *)hcca + HCCA_WRITEBACK_OFFSET,
> > -                              HCCA_WRITEBACK_SIZE);
> > +    dma_memory_write(ohci->dma,
> > +                     addr + ohci->localmem_base + HCCA_WRITEBACK_OFFSET,
> > +                     (char *)hcca + HCCA_WRITEBACK_OFFSET,
> > +                     HCCA_WRITEBACK_SIZE);
> >       return 1;
> >   }
> >
> >   /* Read/Write the contents of a TD from/to main memory.  */
> >   static void ohci_copy_td(OHCIState *ohci, struct ohci_td *td,
> > -                         uint8_t *buf, int len, int write)
> > +                         uint8_t *buf, int len, DMADirection dir)
> >   {
> > -    uint32_t ptr;
> > -    uint32_t n;
> > +    dma_addr_t ptr, n;
> >
> >       ptr = td->cbp;
> >       n = 0x1000 - (ptr&  0xfff);
> >       if (n>  len)
> >           n = len;
> > -    cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, n, write);
> > +    dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, n, dir);
> >       if (n == len)
> >           return;
> >       ptr = td->be&  ~0xfffu;
> >       buf += n;
> > -    cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, len - n, write);
> > +    dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, len - n, dir);
> >   }
> >
> >   /* Read/Write the contents of an ISO TD from/to main memory.  */
> >   static void ohci_copy_iso_td(OHCIState *ohci,
> >                                uint32_t start_addr, uint32_t end_addr,
> > -                             uint8_t *buf, int len, int write)
> > +                             uint8_t *buf, int len, DMADirection dir)
> >   {
> > -    uint32_t ptr;
> > -    uint32_t n;
> > +    dma_addr_t ptr, n;
> >
> >       ptr = start_addr;
> >       n = 0x1000 - (ptr&  0xfff);
> >       if (n>  len)
> >           n = len;
> > -    cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, n, write);
> > +    dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, n, dir);
> >       if (n == len)
> >           return;
> >       ptr = end_addr&  ~0xfffu;
> >       buf += n;
> > -    cpu_physical_memory_rw(ptr + ohci->localmem_base, buf, len - n, write);
> > +    dma_memory_rw(ohci->dma, ptr + ohci->localmem_base, buf, len - n, dir);
> >   }
> >
> >   static void ohci_process_lists(OHCIState *ohci, int completion);
> > @@ -803,7 +803,8 @@ static int ohci_service_iso_td(OHCIState *ohci, struct 
> > ohci_ed *ed,
> >       }
> >
> >       if (len&&  dir != OHCI_TD_DIR_IN) {
> > -        ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len, 
> > 0);
> > +        ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len,
> > +                         DMA_DIRECTION_TO_DEVICE);
> >       }
> >
> >       if (completion) {
> > @@ -827,7 +828,8 @@ static int ohci_service_iso_td(OHCIState *ohci, struct 
> > ohci_ed *ed,
> >       /* Writeback */
> >       if (dir == OHCI_TD_DIR_IN&&  ret>= 0&&  ret<= len) {
> >           /* IN transfer succeeded */
> > -        ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret, 
> > 1);
> > +        ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, ret,
> > +                         DMA_DIRECTION_FROM_DEVICE);
> >           OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_CC,
> >                       OHCI_CC_NOERROR);
> >           OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_SIZE, 
> > ret);
> > @@ -971,7 +973,8 @@ static int ohci_service_td(OHCIState *ohci, struct 
> > ohci_ed *ed)
> >                   pktlen = len;
> >               }
> >               if (!completion) {
> > -                ohci_copy_td(ohci,&td, ohci->usb_buf, pktlen, 0);
> > +                ohci_copy_td(ohci,&td, ohci->usb_buf, pktlen,
> > +                             DMA_DIRECTION_TO_DEVICE);
> >               }
> >           }
> >       }
> > @@ -1021,7 +1024,8 @@ static int ohci_service_td(OHCIState *ohci, struct 
> > ohci_ed *ed)
> >       }
> >       if (ret>= 0) {
> >           if (dir == OHCI_TD_DIR_IN) {
> > -            ohci_copy_td(ohci,&td, ohci->usb_buf, ret, 1);
> > +            ohci_copy_td(ohci,&td, ohci->usb_buf, ret,
> > +                         DMA_DIRECTION_FROM_DEVICE);
> >   #ifdef DEBUG_PACKET
> >               DPRINTF("  data:");
> >               for (i = 0; i<  ret; i++)
> > @@ -1748,11 +1752,14 @@ static USBBusOps ohci_bus_ops = {
> >   };
> >
> >   static int usb_ohci_init(OHCIState *ohci, DeviceState *dev,
> > -                         int num_ports, uint32_t localmem_base,
> > -                         char *masterbus, uint32_t firstport)
> > +                         int num_ports, dma_addr_t localmem_base,
> > +                         char *masterbus, uint32_t firstport,
> > +                         DMAContext *dma)
> >   {
> >       int i;
> >
> > +    ohci->dma = dma;
> > +
> >       if (usb_frame_time == 0) {
> >   #ifdef OHCI_TIME_WARP
> >           usb_frame_time = get_ticks_per_sec();
> > @@ -1817,7 +1824,8 @@ static int usb_ohci_initfn_pci(struct PCIDevice *dev)
> >       ohci->pci_dev.config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */
> >
> >       if (usb_ohci_init(&ohci->state,&dev->qdev, ohci->num_ports, 0,
> > -                      ohci->masterbus, ohci->firstport) != 0) {
> > +                      ohci->masterbus, ohci->firstport,
> > +                      pci_dma_context(dev)) != 0) {
> >           return -1;
> >       }
> >       ohci->state.irq = ohci->pci_dev.irq[0];
> > @@ -1831,7 +1839,7 @@ typedef struct {
> >       SysBusDevice busdev;
> >       OHCIState ohci;
> >       uint32_t num_ports;
> > -    target_phys_addr_t dma_offset;
> > +    dma_addr_t dma_offset;
> >   } OHCISysBusState;
> >
> >   static int ohci_init_pxa(SysBusDevice *dev)
> > @@ -1839,7 +1847,8 @@ static int ohci_init_pxa(SysBusDevice *dev)
> >       OHCISysBusState *s = FROM_SYSBUS(OHCISysBusState, dev);
> >
> >       /* Cannot fail as we pass NULL for masterbus */
> > -    usb_ohci_init(&s->ohci,&dev->qdev, s->num_ports, s->dma_offset, NULL, 
> > 0);
> > +    usb_ohci_init(&s->ohci,&dev->qdev, s->num_ports, s->dma_offset, NULL, 
> > 0,
> > +                  NULL);
> >       sysbus_init_irq(dev,&s->ohci.irq);
> >       sysbus_init_mmio(dev,&s->ohci.mem);
> >
> > @@ -1875,7 +1884,7 @@ static TypeInfo ohci_pci_info = {
> >
> >   static Property ohci_sysbus_properties[] = {
> >       DEFINE_PROP_UINT32("num-ports", OHCISysBusState, num_ports, 3),
> > -    DEFINE_PROP_TADDR("dma-offset", OHCISysBusState, dma_offset, 3),
> > +    DEFINE_PROP_DMAADDR("dma-offset", OHCISysBusState, dma_offset, 3),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >