Re: [Qemu-devel] [RFC PATCH v1 3/4] xilinx_axienet: Create Proxy object for stream

[Andreas Färber]

Am 21.02.2013 05:17, schrieb Peter Crosthwaite:
> Ping!
> 
> Edgar merged patches 1&2 (trivials) before he went on vacation, but
> left this out because he wanted to give everyone a chance to review in
> the context of the active QOM discussions. I probably should have ccd
> Paolo as this little sub system started way back with his refactoring
> of axienet to get rid of the DEFINE_PROP_PTRs we had there.

I'm sorry, I don't fully understand the current code or the changes
you're applying... Some minor comments below.

> On Tue, Feb 12, 2013 at 11:17 AM, Peter Crosthwaite
> <address@hidden> wrote:
>> Create a separate child object to proxy the stream slave connection. This is
>> setup for future work where a second stream slave connection is needed. The
>> new child object is created at qdev init time and is linked back to the 
>> parent
>> (the ethernet device itself) automatically.
>>
>> Stream slave masters differentiate which slave connection they are connected 
>> to
>> by linking to the proxy object rather than the parent.
>>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> ---
>>
>>  hw/petalogix_ml605_mmu.c |    8 ++++++-
>>  hw/xilinx_axienet.c      |   47 
>> ++++++++++++++++++++++++++++++++++++++++++---
>>  2 files changed, 50 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/petalogix_ml605_mmu.c b/hw/petalogix_ml605_mmu.c
>> index 82d7183..3636993 100644
>> --- a/hw/petalogix_ml605_mmu.c
>> +++ b/hw/petalogix_ml605_mmu.c
>> @@ -79,6 +79,7 @@ petalogix_ml605_init(QEMUMachineInitArgs *args)
>>      const char *cpu_model = args->cpu_model;
>>      MemoryRegion *address_space_mem = get_system_memory();
>>      DeviceState *dev, *dma, *eth0;
>> +    Object *peer;
>>      MicroBlazeCPU *cpu;
>>      SysBusDevice *busdev;
>>      CPUMBState *env;
>> @@ -136,11 +137,16 @@ petalogix_ml605_init(QEMUMachineInitArgs *args)
>>      /* FIXME: attach to the sysbus instead */
>>      object_property_add_child(container_get(qdev_get_machine(), 
>> "/unattached"),
>>                                    "xilinx-dma", OBJECT(dma), NULL);
>> +    object_property_add_child(container_get(qdev_get_machine(), 
>> "/unattached"),
>> +                                  "xilinx-eth0", OBJECT(eth0), NULL);

Why are you adding properties to /machine/unassigned? It's your machine
so you can add things to /machine as you like, modulo Anthony's ABI
stability rules (no type changes for existing properties).

Copied indentation looks a bit weird btw.

>>
>>      xilinx_axiethernet_init(eth0, &nd_table[0], STREAM_SLAVE(dma),
>>                                     0x82780000, irq[3], 0x1000, 0x1000);
>>
>> -    xilinx_axidma_init(dma, STREAM_SLAVE(eth0), 0x84600000, irq[1], irq[0],
>> +    peer = object_property_get_link(OBJECT(eth0), "data-stream", NULL);
>> +    assert(peer);
>> +
>> +    xilinx_axidma_init(dma, STREAM_SLAVE(peer), 0x84600000, irq[1], irq[0],
>>                         100 * 1000000);
>>
>>      {
>> diff --git a/hw/xilinx_axienet.c b/hw/xilinx_axienet.c
>> index 34e344c..7adb24d 100644
>> --- a/hw/xilinx_axienet.c
>> +++ b/hw/xilinx_axienet.c
>> @@ -364,6 +364,18 @@ struct XilinxAXIEnet {
>>      uint8_t *rxmem;
>>  };
>>
>> +#define TYPE_XILINX_AXI_ENET_DATA_STREAM "xilinx-axienet-data-stream"
>> +
>> +#define XILINX_AXI_ENET_DATA_STREAM(obj) \
>> +     OBJECT_CHECK(XilinxAXIEnetStreamSlave, (obj),\
>> +     TYPE_XILINX_AXI_ENET_DATA_STREAM)
>> +
>> +typedef struct XilinxAXIEnetStreamSlave {
>> +    Object parent;
>> +
>> +    struct XilinxAXIEnet *enet;
>> +} XilinxAXIEnetStreamSlave;
>> +
>>  static void axienet_rx_reset(struct XilinxAXIEnet *s)
>>  {
>>      s->rcw[1] = RCW1_JUM | RCW1_FCS | RCW1_RX | RCW1_VLAN;
>> @@ -791,9 +803,11 @@ static void eth_cleanup(NetClientState *nc)
>>  }
>>
>>  static void
>> -axienet_stream_push(StreamSlave *obj, uint8_t *buf, size_t size, uint32_t 
>> *hdr)
>> +axienet_data_stream_push(StreamSlave *obj, uint8_t *buf, size_t size,
>> +                         uint32_t *hdr)
>>  {
>> -    struct XilinxAXIEnet *s = FROM_SYSBUS(typeof(*s), SYS_BUS_DEVICE(obj));
>> +    XilinxAXIEnetStreamSlave *ds = XILINX_AXI_ENET_DATA_STREAM(obj);
>> +    struct XilinxAXIEnet *s = ds->enet;
>>
>>      /* TX enable ?  */
>>      if (!(s->tc & TC_TX)) {
>> @@ -844,6 +858,17 @@ static NetClientInfo net_xilinx_enet_info = {
>>  static int xilinx_enet_init(SysBusDevice *dev)
>>  {
>>      struct XilinxAXIEnet *s = FROM_SYSBUS(typeof(*s), dev);
>> +    XilinxAXIEnetStreamSlave *ds = XILINX_AXI_ENET_DATA_STREAM(
>> +                        object_new(TYPE_XILINX_AXI_ENET_DATA_STREAM));
>> +    Error *errp = NULL;

This SysBus initfn function will at some point need to be turned into a
realize function, which gets an Error **errp argument. If you want to do
local error checking, better to name this err, error or local_err to
leave errp for Error** argument.
Keep in mind that there is no guarantee that errp will be non-NULL, thus
the possible need for a local Error* despite Error** argument.

>> +
>> +    object_property_add_child(OBJECT(s), "data-stream", (Object *)ds, 
>> &errp);
>> +    assert_no_error(errp);
>> +    object_property_add_link(OBJECT(ds), "enet", "xlnx.axi-ethernet",
>> +                             (Object **) &ds->enet, &errp);
>> +    assert_no_error(errp);
>> +    object_property_set_link(OBJECT(ds), OBJECT(s), "enet", &errp);
>> +    assert_no_error(errp);

This should not assert but be turned into a realize function and set
errp argument via error_propagate() and return.

>>
>>      sysbus_init_irq(dev, &s->irq);

Outside the scope of this patch, but this looks like a candidate for
instance_init, to allow connecting IRQs before realization.

>>
>> @@ -886,11 +911,16 @@ static void xilinx_enet_class_init(ObjectClass *klass, 
>> void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>> -    StreamSlaveClass *ssc = STREAM_SLAVE_CLASS(klass);
>>
>>      k->init = xilinx_enet_init;
>>      dc->props = xilinx_enet_properties;
>> -    ssc->push = axienet_stream_push;
>> +}
>> +
>> +static void xilinx_enet_stream_class_init(ObjectClass *klass, void *data)
>> +{
>> +    StreamSlaveClass *ssc = STREAM_SLAVE_CLASS(klass);
>> +
>> +    ssc->push = data;
>>  }
>>
>>  static const TypeInfo xilinx_enet_info = {
>> @@ -899,6 +929,14 @@ static const TypeInfo xilinx_enet_info = {
>>      .instance_size = sizeof(struct XilinxAXIEnet),
>>      .class_init    = xilinx_enet_class_init,
>>      .instance_init = xilinx_enet_initfn,
>> +};
>> +
>> +static const TypeInfo xilinx_enet_data_stream_info = {
>> +    .name          = TYPE_XILINX_AXI_ENET_DATA_STREAM,
>> +    .parent        = TYPE_OBJECT,
>> +    .instance_size = sizeof(struct XilinxAXIEnetStreamSlave),
>> +    .class_init    = xilinx_enet_stream_class_init,
>> +    .class_data    = axienet_data_stream_push,

Why pass a single function as .class_data rather than setting it in
xilinx_enet_stream_class_init() directly without void* in between?

>>      .interfaces = (InterfaceInfo[]) {
>>              { TYPE_STREAM_SLAVE },
>>              { }
>> @@ -908,6 +946,7 @@ static const TypeInfo xilinx_enet_info = {
>>  static void xilinx_enet_register_types(void)
>>  {
>>      type_register_static(&xilinx_enet_info);
>> +    type_register_static(&xilinx_enet_data_stream_info);
>>  }
>>
>>  type_init(xilinx_enet_register_types)
>> --
>> 1.7.0.4

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg