[RFC,v1,3/4] xilinx_axienet: Create Proxy object for stream

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 <peter.crosthwaite@xilinx.com>
---

 hw/petalogix_ml605_mmu.c |    8 ++++++-
 hw/xilinx_axienet.c      |   47 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 50 insertions(+), 5 deletions(-)

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.

Regards,
Peter

On Tue, Feb 12, 2013 at 11:17 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> 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 <peter.crosthwaite@xilinx.com>
> ---
>
>  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);
>
>      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;
> +
> +    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);
>
>      sysbus_init_irq(dev, &s->irq);
>
> @@ -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,
>      .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
>

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
> <peter.crosthwaite@xilinx.com> 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 <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  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