[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/13] usb-ohci: Use universal DMA helper functi
From: |
Benjamin Herrenschmidt |
Subject: |
Re: [Qemu-devel] [PATCH 04/13] usb-ohci: Use universal DMA helper functions |
Date: |
Thu, 21 Jun 2012 07:36:08 +1000 |
> > 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(),
> > };
> >
- [Qemu-devel] [PATCH 00/13] iommu series, Benjamin Herrenschmidt, 2012/06/19
- [Qemu-devel] [PATCH 04/13] usb-ohci: Use universal DMA helper functions, Benjamin Herrenschmidt, 2012/06/19
- Re: [Qemu-devel] [PATCH 04/13] usb-ohci: Use universal DMA helper functions, Anthony Liguori, 2012/06/20
- Re: [Qemu-devel] [PATCH 04/13] usb-ohci: Use universal DMA helper functions,
Benjamin Herrenschmidt <=
- Re: [Qemu-devel] [PATCH 04/13] usb-ohci: Use universal DMA helper functions, Anthony Liguori, 2012/06/20
- Re: [Qemu-devel] [PATCH 04/13] usb-ohci: Use universal DMA helper functions, Benjamin Herrenschmidt, 2012/06/20
- Re: [Qemu-devel] [PATCH 04/13] usb-ohci: Use universal DMA helper functions, Michael S. Tsirkin, 2012/06/21
- Re: [Qemu-devel] [PATCH 04/13] usb-ohci: Use universal DMA helper functions, Anthony Liguori, 2012/06/21
- Re: [Qemu-devel] [PATCH 04/13] usb-ohci: Use universal DMA helper functions, Michael S. Tsirkin, 2012/06/21
- Re: [Qemu-devel] [PATCH 04/13] usb-ohci: Use universal DMA helper functions, Benjamin Herrenschmidt, 2012/06/21
- Re: [Qemu-devel] [PATCH 04/13] usb-ohci: Use universal DMA helper functions, Gerd Hoffmann, 2012/06/21
[Qemu-devel] [PATCH 05/13] iommu: Make sglists and dma_bdrv helpers use new universal DMA helpers, Benjamin Herrenschmidt, 2012/06/19