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

[Scott Wood]

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 <address@hidden>
> ---
>  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/address@hidden", data->id);

The unit address is supposed to match reg.  It's not an arbitrary
disambiguator.

> +    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?

> +    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.

Also, why are these interrupts edge triggered?

-Scott