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

[Alexander Graf <agraf@suse.de>]

On 02.07.14 19:32, Scott Wood wrote:
> On Wed, 2014-07-02 at 19:24 +0200, Alexander Graf wrote:
>> 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.
> To be theoretically correct (i.e. something that wouldn't break if used
> in a real Open Firmware) you'd either leave out the unit address and put
> the disambiguation directly in the name, or have a zero-length reg that
> corresponds to ranges or a child node's reg.
>
> If you just want to match what we currently do in the real hardware fdt,
> use the reg of the first group node.
>
>>>> +    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.
> Sigh.  I missed that this is in e500.c rather than the eTSEC file.  This
> approach will not scale if we ever have multiple platforms wanting to
> create device trees with overlap in the devices they want to describe.

It has to - device trees differ too much between architectures (and 
potentially even machines) to have them reasonably live in device files.


Alex