diff mbox

[6/6] e500: Add support for eTSEC in device tree

Message ID 1404251378-5242-7-git-send-email-agraf@suse.de
State New
Headers show

Commit Message

Alexander Graf July 1, 2014, 9:49 p.m. UTC
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(+)

Comments

Scott Wood July 1, 2014, 10:56 p.m. UTC | #1
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.

> +    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
Alexander Graf July 2, 2014, 5:24 p.m. UTC | #2
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
Scott Wood July 2, 2014, 5:32 p.m. UTC | #3
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.

-Scott
Alexander Graf July 2, 2014, 5:34 p.m. UTC | #4
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
diff mbox

Patch

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);
+    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);
+    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);
+
+    g_free(node);
+    g_free(group);
+
+    return 0;
+}
+
 static int sysbus_device_create_devtree(Object *obj, void *opaque)
 {
     PlatformDevtreeData *data = opaque;
@@ -154,6 +183,11 @@  static int sysbus_device_create_devtree(Object *obj, void *opaque)
         return object_child_foreach(obj, sysbus_device_create_devtree, data);
     }
 
+    if (object_dynamic_cast(obj, TYPE_ETSEC_COMMON)) {
+        create_devtree_etsec(ETSEC_COMMON(dev), data);
+        matched = true;
+    }
+
     if (matched) {
         data->id++;
     } else {