Re: [Qemu-devel] [PATCH 6/6] e500: Add support for eTSEC in device tree

[Alexander Graf <agraf@suse.de>]

On 02.07.14 00:56, Scott Wood wrote:
> On Tue, 2014-07-01 at 23:49 +0200, Alexander Graf wrote:
>> This patch adds support to expose eTSEC devices in the dynamically created
>> guest facing device tree. This allows us to expose eTSEC devices into guests
>> without changes in the machine file.
>>
>> Because we can now tell the guest about eTSEC devices this patch allows the
>> user to specify eTSEC devices via -device at all.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>   hw/ppc/e500.c | 34 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index bf704b0..bebff6f 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -37,6 +37,7 @@
>>   #include "qemu/host-utils.h"
>>   #include "hw/pci-host/ppce500.h"
>>   #include "qemu/error-report.h"
>> +#include "hw/net/fsl_etsec/etsec.h"
>>   
>>   #define EPAPR_MAGIC                (0x45504150)
>>   #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
>> @@ -139,6 +140,34 @@ typedef struct PlatformDevtreeData {
>>       int id;
>>   } PlatformDevtreeData;
>>   
>> +static int create_devtree_etsec(eTSEC *etsec, PlatformDevtreeData *data)
>> +{
>> +    SysBusDevice *sbdev = &etsec->busdev;
>> +    gchar *node = g_strdup_printf("/platform/ethernet@%d", data->id);
> The unit address is supposed to match reg.  It's not an arbitrary
> disambiguator.

So what do we do in case we don't have any reg, but only an IRQ line? Oh 
well - I guess we can cross that line when we get to it.

>
>> +    gchar *group = g_strdup_printf("%s/queue-group", node);
>> +    void *fdt = data->fdt;
>> +
>> +    qemu_fdt_add_subnode(fdt, node);
>> +    qemu_fdt_setprop_string(fdt, node, "device_type", "network");
>> +    qemu_fdt_setprop_string(fdt, node, "compatible", "fsl,etsec2");
>> +    qemu_fdt_setprop_string(fdt, node, "model", "eTSEC");
>> +    qemu_fdt_setprop(fdt, node, "local-mac-address", etsec->conf.macaddr.a, 6);
>> +    qemu_fdt_setprop_cells(fdt, node, "fixed-link", 0, 1, 1000, 0, 0);
>> +
>> +    qemu_fdt_add_subnode(fdt, group);
>> +    qemu_fdt_setprop_cells(fdt, group, "reg", sbdev->user_mmios[0], 0x1000);
>> +    qemu_fdt_setprop_phandle(fdt, group, "interrupt-parent", data->mpic);
> Why not do interrupt-parent in the parent node, or top of tree?

Parent sounds appealing :). In fact, it's already there - this copy is 
simply useless.

>
>> +    qemu_fdt_setprop_cells(fdt, group, "interrupts",
>> +        data->irq_start + sbdev->user_irqs[0], 0x0,
>> +        data->irq_start + sbdev->user_irqs[1], 0x0,
>> +        data->irq_start + sbdev->user_irqs[2], 0x0);
> Are we still using two-cell interrupt specifiers?  If so, we should
> switch before the assumption gets encoded into random device files.

Random device files should never get any device tree bits encoded. 
Device tree generation is responsibility of the machine file.

So we can easily convert the whole thing to the 4-cell when we start to 
support different interrupt types :)

> Also, why are these interrupts edge triggered?

Good catch - they're 0x2 on real hardware.


Alex