Re: [Qemu-devel] [PATCH 2/3] xilinx: Add AXIENET & DMA models

["Edgar E. Iglesias" <edgar.iglesias@petalogix.com>]

On Mon, Mar 14, 2011 at 09:56:23PM +0200, Blue Swirl wrote:
> On Mon, Mar 14, 2011 at 3:46 PM,  <edgar.iglesias@petalogix.com> wrote:
> > From: Edgar E. Iglesias <edgar.iglesias@petalogix.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@petalogix.com>
> > ---
> >  Makefile.target     |    2 +
> >  hw/xilinx_axidma.c  |  463 +++++++++++++++++++++++++++
> >  hw/xilinx_axidma.h  |   40 +++
> >  hw/xilinx_axienet.c |  857 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 1362 insertions(+), 0 deletions(-)
> >  create mode 100644 hw/xilinx_axidma.c
> >  create mode 100644 hw/xilinx_axidma.h
> >  create mode 100644 hw/xilinx_axienet.c

I'll be posting a v2 shortly that addresses most of your comments.
I'll answer the remaining here.


> > +static void stream_process_mem2s(struct axi_stream *s, struct xlx_dma_ch *dmach)
> > +{
> > +    uint32_t prev_d;
> > +    unsigned char txbuf[16 * 1024];
> > +    unsigned int txlen;
> > +    uint32_t app[6];
> > +
> > +    if (!stream_running(s) || stream_idle(s)) {
> > +        return;
> > +    }
> > +
> > +    while (1) {
> > +        stream_desc_load(s, s->regs[R_CURDESC]);
> > +
> > +        if (s->desc.status & (1 << 31)) {
> > +            s->regs[R_DMASR] |= 2;
> > +            break;
> > +        }
> > +
> > +        if (stream_desc_sof(&s->desc)) {
> > +            s->pos = 0;
> > +            memcpy(app, s->desc.app, sizeof app);
> > +        }
> > +
> > +        txlen = s->desc.control & ((1 << 23) - 1);
> > +        if ((txlen + s->pos) > sizeof txbuf) {
> > +            hw_error("%s: too small internal txbuf! %d\n", __func__,
> > +                     txlen + s->pos);
> > +        }
> > +
> > +        cpu_physical_memory_read(s->desc.buffer_address,
> > +                                 txbuf + s->pos, txlen);
> > +        s->pos += txlen;
> > +
> > +        if (stream_desc_eof(&s->desc)) {
> > +            xlx_dma_push_to_client(dmach, txbuf, s->pos, app);
> 
> Maybe the client could instead read the data directly from memory
> (zero copy) without a bounce buffer. Then the DMA API could be useful.

I'll need to investigate this more. Are there any examples in the tree
of scatter-gathering zero-copy DMA to network?

For the moment, I'll leave this part for future work.


> > +static SysBusDeviceInfo xilinx_enet_info = {
> > +    .init = xilinx_enet_init,
> > +    .qdev.name  = "xilinx,axienet",
> > +    .qdev.size  = sizeof(struct xlx_axienet),
> > +    .qdev.props = (Property[]) {
> > +        DEFINE_PROP_UINT32("phyaddr", struct xlx_axienet, c_phyaddr, 7),
> 
> I'm not sure what c_phyaddr does, can you use DEFINE_PROP_MACADDR?

It's a way for boards to choose the PHY's adress on the MDIO bus. It's
a 5 bit nr on MII. It might grow slightly later if we add support for other
PHY connections, not sure. uint32 should be more than enough I think,
uint16 probably too. Doesnt matter so much though...

Thanks again for good review.

Cheers