| Message ID | 20161116224024.GA11821@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 16, 2016 at 8:40 PM, Guenter Roeck <linux@roeck-us.net> wrote: > Yes, but not the 'device_type' property, which the kernel seems to expect. > The qemu patch below fixes the problem for sabrelite, I just don't know > if that is really the way to go. You tell me; I'll be happy to submit > the necessary patch(es) into qemu. I can send the patch adding device_type for the i.MX dts files, but in the meantime, it would be nice if you could send your proposal patch to qemu as it will make qemu to work closer to the real hardware. Thanks > The same is true for 'chosen'. Right now qemu expects this node to exist. > It does exist for sabrelite, but apparently not for imx25-pdk. We can fix this too in the dts. Thanks
On Wed, Nov 16, 2016 at 02:40:24PM -0800, Guenter Roeck wrote: > On Wed, Nov 16, 2016 at 08:27:09PM -0200, Fabio Estevam wrote: > > Hi Guenter, > > > > On Wed, Nov 16, 2016 at 8:10 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > > > > > Anyway, I guess the problem is that the "official" dtb files no longer provide > > > the skeleton /chosen and /memory nodes (and maybe others), and qemu seems to > > > expect that they are provided. Is that correct ? > > > > imx6qdl-sabrelite.dtsi provides chosen and memory nodes. > > Yes, but not the 'device_type' property, which the kernel seems to expect. Memory nodes require this property per ePAPR and the devicetree.org spec, so the bug is that we didn't add those when removing the skeleton.dtsi include. > The qemu patch below fixes the problem for sabrelite, I just don't know > if that is really the way to go. You tell me; I'll be happy to submit > the necessary patch(es) into qemu. As above, I don't think the below patch is necessary. The dt should have this property to begin with. > The same is true for 'chosen'. Right now qemu expects this node to exist. > It does exist for sabrelite, but apparently not for imx25-pdk. Having QEMU create a /chosen node if one does not exist already sounds sensible to me. Thanks, Mark. > Guenter > > --- > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > index 1b913a4..080d1e5 100644 > --- a/hw/arm/boot.c > +++ b/hw/arm/boot.c > @@ -486,6 +486,12 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo, > g_free(nodename); > } > } else { > + Error *err = NULL; > + > + if (!qemu_fdt_getprop(fdt, "/memory", "device_type", NULL, &err)) { > + qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory"); > + } > + > rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg", > acells, binfo->loader_start, > scells, binfo->ram_size);
On 11/17/2016 02:55 AM, Mark Rutland wrote: > On Wed, Nov 16, 2016 at 02:40:24PM -0800, Guenter Roeck wrote: >> On Wed, Nov 16, 2016 at 08:27:09PM -0200, Fabio Estevam wrote: >>> Hi Guenter, >>> >>> On Wed, Nov 16, 2016 at 8:10 PM, Guenter Roeck <linux@roeck-us.net> wrote: >>>> >>>> Anyway, I guess the problem is that the "official" dtb files no longer provide >>>> the skeleton /chosen and /memory nodes (and maybe others), and qemu seems to >>>> expect that they are provided. Is that correct ? >>> >>> imx6qdl-sabrelite.dtsi provides chosen and memory nodes. >> >> Yes, but not the 'device_type' property, which the kernel seems to expect. > > Memory nodes require this property per ePAPR and the devicetree.org > spec, so the bug is that we didn't add those when removing the > skeleton.dtsi include. > The downside from qemu perspective is that the real hardware seems to add the property unconditionally, or the boot failure would have been seen there as well. I submitted https://patchwork.ozlabs.org/patch/695951/; we'll see how it goes. Guenter >> The qemu patch below fixes the problem for sabrelite, I just don't know >> if that is really the way to go. You tell me; I'll be happy to submit >> the necessary patch(es) into qemu. > > As above, I don't think the below patch is necessary. The dt should have > this property to begin with. > >> The same is true for 'chosen'. Right now qemu expects this node to exist. >> It does exist for sabrelite, but apparently not for imx25-pdk. > > Having QEMU create a /chosen node if one does not exist already sounds > sensible to me. > > Thanks, > Mark. > >> Guenter >> >> --- >> diff --git a/hw/arm/boot.c b/hw/arm/boot.c >> index 1b913a4..080d1e5 100644 >> --- a/hw/arm/boot.c >> +++ b/hw/arm/boot.c >> @@ -486,6 +486,12 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo, >> g_free(nodename); >> } >> } else { >> + Error *err = NULL; >> + >> + if (!qemu_fdt_getprop(fdt, "/memory", "device_type", NULL, &err)) { >> + qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory"); >> + } >> + >> rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg", >> acells, binfo->loader_start, >> scells, binfo->ram_size); >
On Thu, Nov 17, 2016 at 06:44:55AM -0800, Guenter Roeck wrote: > On 11/17/2016 02:55 AM, Mark Rutland wrote: > >On Wed, Nov 16, 2016 at 02:40:24PM -0800, Guenter Roeck wrote: > >>On Wed, Nov 16, 2016 at 08:27:09PM -0200, Fabio Estevam wrote: > >>>Hi Guenter, > >>> > >>>On Wed, Nov 16, 2016 at 8:10 PM, Guenter Roeck <linux@roeck-us.net> wrote: > >>>> > >>>>Anyway, I guess the problem is that the "official" dtb files no longer provide > >>>>the skeleton /chosen and /memory nodes (and maybe others), and qemu seems to > >>>>expect that they are provided. Is that correct ? > >>> > >>>imx6qdl-sabrelite.dtsi provides chosen and memory nodes. > >> > >>Yes, but not the 'device_type' property, which the kernel seems to expect. > > > >Memory nodes require this property per ePAPR and the devicetree.org > >spec, so the bug is that we didn't add those when removing the > >skeleton.dtsi include. > > The downside from qemu perspective is that the real hardware seems > to add the property unconditionally, or the boot failure would have > been seen there as well. > > I submitted https://patchwork.ozlabs.org/patch/695951/; we'll see how it goes. Sure, the firmare/bootlaoder you're using may add this automatically. My worry is that adding this to a generic file in QEMU only serves to mask this class of bug for other boards (i.e. they'll work fine in QEMU, but not on real HW using whatever bootlaoder happens ot be there). Thanks, Mark.
On 11/17/2016 07:05 AM, Mark Rutland wrote: > On Thu, Nov 17, 2016 at 06:44:55AM -0800, Guenter Roeck wrote: >> On 11/17/2016 02:55 AM, Mark Rutland wrote: >>> On Wed, Nov 16, 2016 at 02:40:24PM -0800, Guenter Roeck wrote: >>>> On Wed, Nov 16, 2016 at 08:27:09PM -0200, Fabio Estevam wrote: >>>>> Hi Guenter, >>>>> >>>>> On Wed, Nov 16, 2016 at 8:10 PM, Guenter Roeck <linux@roeck-us.net> wrote: >>>>>> >>>>>> Anyway, I guess the problem is that the "official" dtb files no longer provide >>>>>> the skeleton /chosen and /memory nodes (and maybe others), and qemu seems to >>>>>> expect that they are provided. Is that correct ? >>>>> >>>>> imx6qdl-sabrelite.dtsi provides chosen and memory nodes. >>>> >>>> Yes, but not the 'device_type' property, which the kernel seems to expect. >>> >>> Memory nodes require this property per ePAPR and the devicetree.org >>> spec, so the bug is that we didn't add those when removing the >>> skeleton.dtsi include. >> >> The downside from qemu perspective is that the real hardware seems >> to add the property unconditionally, or the boot failure would have >> been seen there as well. >> >> I submitted https://patchwork.ozlabs.org/patch/695951/; we'll see how it goes. > > Sure, the firmare/bootlaoder you're using may add this automatically. > > My worry is that adding this to a generic file in QEMU only serves to > mask this class of bug for other boards (i.e. they'll work fine in QEMU, > but not on real HW using whatever bootlaoder happens ot be there). > Good point. What would be the correct behavior for qemu ? Adding a chosen node if it does not exist is one detail we already established. Also, I think a check if /memory/device_type exists (and to bail out if it doesn't) would make sense. What about the memory node ? Does it have to exist, or should it be added (including the device_type property) if not ? Thanks, Guenter
On Thu, Nov 17, 2016 at 08:17:00AM -0800, Guenter Roeck wrote: > On 11/17/2016 07:05 AM, Mark Rutland wrote: > >On Thu, Nov 17, 2016 at 06:44:55AM -0800, Guenter Roeck wrote: > >>On 11/17/2016 02:55 AM, Mark Rutland wrote: > >>>Memory nodes require this property per ePAPR and the devicetree.org > >>>spec, so the bug is that we didn't add those when removing the > >>>skeleton.dtsi include. > >> > >>The downside from qemu perspective is that the real hardware seems > >>to add the property unconditionally, or the boot failure would have > >>been seen there as well. > >> > >>I submitted https://patchwork.ozlabs.org/patch/695951/; we'll see how it goes. > > > >Sure, the firmare/bootlaoder you're using may add this automatically. > > > >My worry is that adding this to a generic file in QEMU only serves to > >mask this class of bug for other boards (i.e. they'll work fine in QEMU, > >but not on real HW using whatever bootlaoder happens ot be there). > > > Good point. > > What would be the correct behavior for qemu ? Adding a chosen node if it does > not exist is one detail we already established. Also, I think a check if > /memory/device_type exists (and to bail out if it doesn't) would make sense. We'd also need to check for /memory@<n> nodes, as they can validly have unit-addresses (and many do). Generally, the "correct" way to find them is to iterate over all ndoes with device_type = "memory", so one could do that and give up if none are found, ignoring the naming entirely. > What about the memory node ? Does it have to exist, or should it be added > (including the device_type property) if not ? I'm not sure what QEMU does in this area. I suspect it may expect a node in some cases, or may generate one in others. There's no point generating one when we don't have the information to hand, certainly. Thanks, Mark.
On 11/17/2016 08:39 AM, Mark Rutland wrote: > On Thu, Nov 17, 2016 at 08:17:00AM -0800, Guenter Roeck wrote: >> On 11/17/2016 07:05 AM, Mark Rutland wrote: >>> On Thu, Nov 17, 2016 at 06:44:55AM -0800, Guenter Roeck wrote: >>>> On 11/17/2016 02:55 AM, Mark Rutland wrote: >>>>> Memory nodes require this property per ePAPR and the devicetree.org >>>>> spec, so the bug is that we didn't add those when removing the >>>>> skeleton.dtsi include. >>>> >>>> The downside from qemu perspective is that the real hardware seems >>>> to add the property unconditionally, or the boot failure would have >>>> been seen there as well. >>>> >>>> I submitted https://patchwork.ozlabs.org/patch/695951/; we'll see how it goes. >>> >>> Sure, the firmare/bootlaoder you're using may add this automatically. >>> >>> My worry is that adding this to a generic file in QEMU only serves to >>> mask this class of bug for other boards (i.e. they'll work fine in QEMU, >>> but not on real HW using whatever bootlaoder happens ot be there). >>> >> Good point. >> >> What would be the correct behavior for qemu ? Adding a chosen node if it does >> not exist is one detail we already established. Also, I think a check if >> /memory/device_type exists (and to bail out if it doesn't) would make sense. > > We'd also need to check for /memory@<n> nodes, as they can validly have > unit-addresses (and many do). > > Generally, the "correct" way to find them is to iterate over all ndoes > with device_type = "memory", so one could do that and give up if none > are found, ignoring the naming entirely. > >> What about the memory node ? Does it have to exist, or should it be added >> (including the device_type property) if not ? > > I'm not sure what QEMU does in this area. I suspect it may expect a node > in some cases, or may generate one in others. > > There's no point generating one when we don't have the information to > hand, certainly. > So far, for arm, qemu assumes that the /memory node exists, and it fills in /memory/reg. This is done if a devicetree file is specified and numa is disabled. Numa node handling is different; if NUMA is enabled, qemu removes an existing /memory node and creates /memory@ nodes as configured. It does not expect to see pre-existing /memory@ nodes. Thanks, Guenter
diff --git a/hw/arm/boot.c b/hw/arm/boot.c index 1b913a4..080d1e5 100644 --- a/hw/arm/boot.c +++ b/hw/arm/boot.c @@ -486,6 +486,12 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo, g_free(nodename); } } else { + Error *err = NULL; + + if (!qemu_fdt_getprop(fdt, "/memory", "device_type", NULL, &err)) { + qemu_fdt_setprop_string(fdt, "/memory", "device_type", "memory"); + } + rc = qemu_fdt_setprop_sized_cells(fdt, "/memory", "reg", acells, binfo->loader_start, scells, binfo->ram_size);