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.crosthwa...@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.crosthwa...@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
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
