| Message ID | 1459290646-3592-3-git-send-email-manabian@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Joachim, On 30.03.2016 01:30, Joachim Eastwood wrote: > Add unit name to memory to remove the following warning: > Warning (unit_address_vs_reg): Node /memory has a reg or ranges > property, but no unit name > > Signed-off-by: Joachim Eastwood <manabian@gmail.com> > --- > arch/arm/boot/dts/skeleton.dtsi | 2 +- > arch/arm/boot/dts/skeleton64.dtsi | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/skeleton.dtsi b/arch/arm/boot/dts/skeleton.dtsi > index b41d241..a20da0a 100644 > --- a/arch/arm/boot/dts/skeleton.dtsi > +++ b/arch/arm/boot/dts/skeleton.dtsi > @@ -9,5 +9,5 @@ > #size-cells = <1>; > chosen { }; > aliases { }; > - memory { device_type = "memory"; reg = <0 0>; }; > + memory@0 { device_type = "memory"; reg = <0 0>; }; > }; > diff --git a/arch/arm/boot/dts/skeleton64.dtsi b/arch/arm/boot/dts/skeleton64.dtsi > index b5d7f36..6dbe9f9 100644 > --- a/arch/arm/boot/dts/skeleton64.dtsi > +++ b/arch/arm/boot/dts/skeleton64.dtsi > @@ -9,5 +9,5 @@ > #size-cells = <2>; > chosen { }; > aliases { }; > - memory { device_type = "memory"; reg = <0 0 0 0>; }; > + memory@0 { device_type = "memory"; reg = <0 0 0 0>; }; > }; > this looks wrong, because all inherited compiled dtbs will contain memory@0 device node, for example (with applied 4/7 from this series): % scripts/dtc/dtc -I dtb -O dts arch/arm/boot/dts/lpc4357-ea4357-devkit.dtb | grep -A3 memory@ memory@0 { device_type = "memory"; reg = <0x0 0x0>; }; -- memory@28000000 { device_type = "memory"; reg = <0x28000000 0x2000000>; }; -- With best wishes, Vladimir
On Wed, Mar 30, 2016 at 12:30:41AM +0200, Joachim Eastwood wrote: > Add unit name to memory to remove the following warning: > Warning (unit_address_vs_reg): Node /memory has a reg or ranges > property, but no unit name If anything, it would be better to get rid of the memory node from the skeleton DTs. For DTs which have a memory node there's no problem, and DTs which expect a bootlaoder to fill things in have a logical place to document that fact. Thanks, Mark. > > Signed-off-by: Joachim Eastwood <manabian@gmail.com> > --- > arch/arm/boot/dts/skeleton.dtsi | 2 +- > arch/arm/boot/dts/skeleton64.dtsi | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/skeleton.dtsi b/arch/arm/boot/dts/skeleton.dtsi > index b41d241..a20da0a 100644 > --- a/arch/arm/boot/dts/skeleton.dtsi > +++ b/arch/arm/boot/dts/skeleton.dtsi > @@ -9,5 +9,5 @@ > #size-cells = <1>; > chosen { }; > aliases { }; > - memory { device_type = "memory"; reg = <0 0>; }; > + memory@0 { device_type = "memory"; reg = <0 0>; }; > }; > diff --git a/arch/arm/boot/dts/skeleton64.dtsi b/arch/arm/boot/dts/skeleton64.dtsi > index b5d7f36..6dbe9f9 100644 > --- a/arch/arm/boot/dts/skeleton64.dtsi > +++ b/arch/arm/boot/dts/skeleton64.dtsi > @@ -9,5 +9,5 @@ > #size-cells = <2>; > chosen { }; > aliases { }; > - memory { device_type = "memory"; reg = <0 0 0 0>; }; > + memory@0 { device_type = "memory"; reg = <0 0 0 0>; }; > }; > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Hi Mark, On 30 March 2016 at 13:06, Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Mar 30, 2016 at 12:30:41AM +0200, Joachim Eastwood wrote: >> Add unit name to memory to remove the following warning: >> Warning (unit_address_vs_reg): Node /memory has a reg or ranges >> property, but no unit name > > If anything, it would be better to get rid of the memory node from the > skeleton DTs. Yes, I agree and as Vladimir notes just by adding a unit name we create duplicate memory nodes. I'll send an updated patch set once Rob has a change to look at it also. > For DTs which have a memory node there's no problem, and DTs which > expect a bootlaoder to fill things in have a logical place to document > that fact. I'll take a look at which DTs that currently doesn't have a memory and add it. Thanks for looking. regards, Joachim Eastwood
On 30.03.2016 14:06, Mark Rutland wrote: > On Wed, Mar 30, 2016 at 12:30:41AM +0200, Joachim Eastwood wrote: >> Add unit name to memory to remove the following warning: >> Warning (unit_address_vs_reg): Node /memory has a reg or ranges >> property, but no unit name > > If anything, it would be better to get rid of the memory node from the > skeleton DTs. > > For DTs which have a memory node there's no problem, and DTs which > expect a bootlaoder to fill things in have a logical place to document > that fact. Generally I support this. U-boot still creates or fixes up "/memory" node only, assuming that a bootloader is updated rarely the kernel should continue to process in expected way "/memory" device node, also note that ePAPR says ePAPR> If a system has multiple ranges of memory, multiple memory nodes ePAPR> can be created, or the ranges can be specified in the reg property ePAPR> of a single memory node. Having just a DT on hand it won't be possible to make assumptions about a bootloader, also "/memory" without a given unit address is used by other kernels and a bootloader can respect this. The only problem I see if DTB is updated on a board but a board bootloader on fix-up is capable to fill a preexisting "/memory" device node in only, otherwise it is not clear why the device node is present in skeleton.dtsi. >> Signed-off-by: Joachim Eastwood <manabian@gmail.com> >> --- >> arch/arm/boot/dts/skeleton.dtsi | 2 +- >> arch/arm/boot/dts/skeleton64.dtsi | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/boot/dts/skeleton.dtsi b/arch/arm/boot/dts/skeleton.dtsi >> index b41d241..a20da0a 100644 >> --- a/arch/arm/boot/dts/skeleton.dtsi >> +++ b/arch/arm/boot/dts/skeleton.dtsi >> @@ -9,5 +9,5 @@ >> #size-cells = <1>; >> chosen { }; >> aliases { }; >> - memory { device_type = "memory"; reg = <0 0>; }; >> + memory@0 { device_type = "memory"; reg = <0 0>; }; >> }; >> diff --git a/arch/arm/boot/dts/skeleton64.dtsi b/arch/arm/boot/dts/skeleton64.dtsi >> index b5d7f36..6dbe9f9 100644 >> --- a/arch/arm/boot/dts/skeleton64.dtsi >> +++ b/arch/arm/boot/dts/skeleton64.dtsi >> @@ -9,5 +9,5 @@ >> #size-cells = <2>; >> chosen { }; >> aliases { }; >> - memory { device_type = "memory"; reg = <0 0 0 0>; }; >> + memory@0 { device_type = "memory"; reg = <0 0 0 0>; }; >> }; >> -- >> 2.7.4 >> -- With best wishes, Vladimir
On Wed, Mar 30, 2016 at 04:06:56PM +0300, Vladimir Zapolskiy wrote: > On 30.03.2016 14:06, Mark Rutland wrote: > > On Wed, Mar 30, 2016 at 12:30:41AM +0200, Joachim Eastwood wrote: > >> Add unit name to memory to remove the following warning: > >> Warning (unit_address_vs_reg): Node /memory has a reg or ranges > >> property, but no unit name > > > > If anything, it would be better to get rid of the memory node from the > > skeleton DTs. > > > > For DTs which have a memory node there's no problem, and DTs which > > expect a bootlaoder to fill things in have a logical place to document > > that fact. > The only problem I see if DTB is updated on a board but a board bootloader > on fix-up is capable to fill a preexisting "/memory" device node in only, > otherwise it is not clear why the device node is present in skeleton.dtsi. Sure. To clarify the above, what I expect that for this case is that the empty memory node would exist in the dts for that particular board, along with a comment, e.g. /* The firmware/bootloader for $BOARD fills this in */ memory { device_type = "memory"; reg = <0 0 0 0>; }; That way you can tell at a glance that the lack of memory information in the DT for a board is intentional, and the bootloader still gets the node it expects. Thanks, Mark.
Hi Mark, On 30 March 2016 at 15:41, Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Mar 30, 2016 at 04:06:56PM +0300, Vladimir Zapolskiy wrote: >> On 30.03.2016 14:06, Mark Rutland wrote: >> > On Wed, Mar 30, 2016 at 12:30:41AM +0200, Joachim Eastwood wrote: >> >> Add unit name to memory to remove the following warning: >> >> Warning (unit_address_vs_reg): Node /memory has a reg or ranges >> >> property, but no unit name >> > >> > If anything, it would be better to get rid of the memory node from the >> > skeleton DTs. >> > >> > For DTs which have a memory node there's no problem, and DTs which >> > expect a bootlaoder to fill things in have a logical place to document >> > that fact. > >> The only problem I see if DTB is updated on a board but a board bootloader >> on fix-up is capable to fill a preexisting "/memory" device node in only, >> otherwise it is not clear why the device node is present in skeleton.dtsi. > > Sure. To clarify the above, what I expect that for this case is that the > empty memory node would exist in the dts for that particular board, > along with a comment, e.g. > > /* The firmware/bootloader for $BOARD fills this in */ > memory { > device_type = "memory"; > reg = <0 0 0 0>; > }; To avoid the warning with the new dtc this would need to be memory@0. > That way you can tell at a glance that the lack of memory information in > the DT for a board is intentional, and the bootloader still gets the > node it expects. But this doesn't seem to be a "problem" with any of the DTs in arch/arm/boot as they all defined a memory node. I used the following script to check for the memory node in all built dtb's. make ARCH=arm CONFIG_OF_ALL_DTBS=y dtbs for i in $(ls arch/arm/boot/dts/*.dtb); do m=$(scripts/dtc/dtc -I dtb -O dts $i | grep -m1 'memory.*{') if [ -z "$m" ]; then echo "Missing memory node in $i" fi done So it should be pretty safe to just remove the memory node entry in the skeleton files. Unless I have missed something with the script above. regards, Joachim Eastwood
On Wed, Mar 30, 2016 at 06:15:35PM +0200, Joachim Eastwood wrote: > Hi Mark, > > On 30 March 2016 at 15:41, Mark Rutland <mark.rutland@arm.com> wrote: > > On Wed, Mar 30, 2016 at 04:06:56PM +0300, Vladimir Zapolskiy wrote: > >> On 30.03.2016 14:06, Mark Rutland wrote: > >> > On Wed, Mar 30, 2016 at 12:30:41AM +0200, Joachim Eastwood wrote: > >> >> Add unit name to memory to remove the following warning: > >> >> Warning (unit_address_vs_reg): Node /memory has a reg or ranges > >> >> property, but no unit name > >> > > >> > If anything, it would be better to get rid of the memory node from the > >> > skeleton DTs. > >> > > >> > For DTs which have a memory node there's no problem, and DTs which > >> > expect a bootlaoder to fill things in have a logical place to document > >> > that fact. > > > >> The only problem I see if DTB is updated on a board but a board bootloader > >> on fix-up is capable to fill a preexisting "/memory" device node in only, > >> otherwise it is not clear why the device node is present in skeleton.dtsi. > > > > Sure. To clarify the above, what I expect that for this case is that the > > empty memory node would exist in the dts for that particular board, > > along with a comment, e.g. > > > > /* The firmware/bootloader for $BOARD fills this in */ > > memory { > > device_type = "memory"; > > reg = <0 0 0 0>; > > }; > > To avoid the warning with the new dtc this would need to be memory@0. Hmm... That's a little sub-optimal in the case that a bootloader is patching this. Presumably a bootloader that needs an existing node won't patch the unit-address to match the reg (which might not start at 0). I'd rather not have the unit-address than have an incorrect unit-address, though I guess we don't have much of a choice here, unless there's some override we can place in the dts. > > That way you can tell at a glance that the lack of memory information in > > the DT for a board is intentional, and the bootloader still gets the > > node it expects. > > But this doesn't seem to be a "problem" with any of the DTs in > arch/arm/boot as they all defined a memory node. > > I used the following script to check for the memory node in all built dtb's. > make ARCH=arm CONFIG_OF_ALL_DTBS=y dtbs > for i in $(ls arch/arm/boot/dts/*.dtb); do > m=$(scripts/dtc/dtc -I dtb -O dts $i | grep -m1 'memory.*{') > if [ -z "$m" ]; then > echo "Missing memory node in $i" > fi > done > > So it should be pretty safe to just remove the memory node entry in > the skeleton files. Unless I have missed something with the script > above. The above might match reserved-memory nodes; it might be better to check for 'device_type\s*=\s*"memory"'. I assume that was run after deleting the memory node from the skeletons? Otherwise, that looks fairly convincing! Thanks, Mark.
On 30 March 2016 at 19:06, Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Mar 30, 2016 at 06:15:35PM +0200, Joachim Eastwood wrote: >> Hi Mark, >> >> On 30 March 2016 at 15:41, Mark Rutland <mark.rutland@arm.com> wrote: >> > On Wed, Mar 30, 2016 at 04:06:56PM +0300, Vladimir Zapolskiy wrote: >> >> On 30.03.2016 14:06, Mark Rutland wrote: >> >> > On Wed, Mar 30, 2016 at 12:30:41AM +0200, Joachim Eastwood wrote: >> >> >> Add unit name to memory to remove the following warning: >> >> >> Warning (unit_address_vs_reg): Node /memory has a reg or ranges >> >> >> property, but no unit name >> >> > >> >> > If anything, it would be better to get rid of the memory node from the >> >> > skeleton DTs. >> >> > >> >> > For DTs which have a memory node there's no problem, and DTs which >> >> > expect a bootlaoder to fill things in have a logical place to document >> >> > that fact. >> > >> >> The only problem I see if DTB is updated on a board but a board bootloader >> >> on fix-up is capable to fill a preexisting "/memory" device node in only, >> >> otherwise it is not clear why the device node is present in skeleton.dtsi. >> > >> > Sure. To clarify the above, what I expect that for this case is that the >> > empty memory node would exist in the dts for that particular board, >> > along with a comment, e.g. >> > >> > /* The firmware/bootloader for $BOARD fills this in */ >> > memory { >> > device_type = "memory"; >> > reg = <0 0 0 0>; >> > }; >> >> To avoid the warning with the new dtc this would need to be memory@0. > > Hmm... That's a little sub-optimal in the case that a bootloader is > patching this. Presumably a bootloader that needs an existing node won't > patch the unit-address to match the reg (which might not start at 0). > > I'd rather not have the unit-address than have an incorrect > unit-address, though I guess we don't have much of a choice here, unless > there's some override we can place in the dts. > >> > That way you can tell at a glance that the lack of memory information in >> > the DT for a board is intentional, and the bootloader still gets the >> > node it expects. >> >> But this doesn't seem to be a "problem" with any of the DTs in >> arch/arm/boot as they all defined a memory node. >> >> I used the following script to check for the memory node in all built dtb's. >> make ARCH=arm CONFIG_OF_ALL_DTBS=y dtbs >> for i in $(ls arch/arm/boot/dts/*.dtb); do >> m=$(scripts/dtc/dtc -I dtb -O dts $i | grep -m1 'memory.*{') >> if [ -z "$m" ]; then >> echo "Missing memory node in $i" >> fi >> done >> >> So it should be pretty safe to just remove the memory node entry in >> the skeleton files. Unless I have missed something with the script >> above. > > The above might match reserved-memory nodes; it might be better to check > for 'device_type\s*=\s*"memory"'. I did check the output of the grep and it looks good. But there are indeed DTs that are missing the 'device_type = "memory"' parameter. Actually; _a lot_ or 438 of 741 to be exact. ugh... I guess all those should be fixed up before we can remove the memory node from skeleton. :/ > I assume that was run after deleting the memory node from the skeletons? Yes :) regards, Joachim Eastwood
On Wed, Mar 30, 2016 at 10:45:06PM +0200, Joachim Eastwood wrote: > On 30 March 2016 at 19:06, Mark Rutland <mark.rutland@arm.com> wrote: > > On Wed, Mar 30, 2016 at 06:15:35PM +0200, Joachim Eastwood wrote: > >> I used the following script to check for the memory node in all built dtb's. > >> make ARCH=arm CONFIG_OF_ALL_DTBS=y dtbs > >> for i in $(ls arch/arm/boot/dts/*.dtb); do > >> m=$(scripts/dtc/dtc -I dtb -O dts $i | grep -m1 'memory.*{') > >> if [ -z "$m" ]; then > >> echo "Missing memory node in $i" > >> fi > >> done > >> > >> So it should be pretty safe to just remove the memory node entry in > >> the skeleton files. Unless I have missed something with the script > >> above. > > > > The above might match reserved-memory nodes; it might be better to check > > for 'device_type\s*=\s*"memory"'. > > I did check the output of the grep and it looks good. But there are > indeed DTs that are missing the 'device_type = "memory"' parameter. > Actually; _a lot_ or 438 of 741 to be exact. ugh... > > I guess all those should be fixed up before we can remove the memory > node from skeleton. :/ Ouch, yes. :( That said, the cahnges don't need to be an atomic operation. We could start adding device_type = "memory" to dts immediately (in as whatever size batches maintainers are happy with), as a duplicate device_type shouldn't be problematic. When we hit critical mass, we could then remove the skeleton memory nodes, fixing up any remaining fallout. As for the mechanical changes, it sounds like we need coccinelle for DT. That, or a laptop, a long flight, and a gin and tonic. Thanks, Mark.
On 31 March 2016 at 12:38, Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Mar 30, 2016 at 10:45:06PM +0200, Joachim Eastwood wrote: >> On 30 March 2016 at 19:06, Mark Rutland <mark.rutland@arm.com> wrote: >> > On Wed, Mar 30, 2016 at 06:15:35PM +0200, Joachim Eastwood wrote: >> >> I used the following script to check for the memory node in all built dtb's. >> >> make ARCH=arm CONFIG_OF_ALL_DTBS=y dtbs >> >> for i in $(ls arch/arm/boot/dts/*.dtb); do >> >> m=$(scripts/dtc/dtc -I dtb -O dts $i | grep -m1 'memory.*{') >> >> if [ -z "$m" ]; then >> >> echo "Missing memory node in $i" >> >> fi >> >> done >> >> >> >> So it should be pretty safe to just remove the memory node entry in >> >> the skeleton files. Unless I have missed something with the script >> >> above. >> > >> > The above might match reserved-memory nodes; it might be better to check >> > for 'device_type\s*=\s*"memory"'. >> >> I did check the output of the grep and it looks good. But there are >> indeed DTs that are missing the 'device_type = "memory"' parameter. >> Actually; _a lot_ or 438 of 741 to be exact. ugh... >> >> I guess all those should be fixed up before we can remove the memory >> node from skeleton. :/ > > Ouch, yes. :( > > That said, the cahnges don't need to be an atomic operation. We could > start adding device_type = "memory" to dts immediately (in as whatever > size batches maintainers are happy with), as a duplicate device_type > shouldn't be problematic. Yes, that is true. > When we hit critical mass, we could then remove the skeleton memory > nodes, fixing up any remaining fallout. > > As for the mechanical changes, it sounds like we need coccinelle for DT. > > That, or a laptop, a long flight, and a gin and tonic. :-) I'll see if I can cook up something with awk. Anyway, I am dropping the memory node changes from this patch set so I can get a pull request sent to arm-soc for lpc18xx sooner rather than later. regards, Joachim Eastwood
On Wed, Mar 30, 2016 at 11:15 AM, Joachim Eastwood <manabian@gmail.com> wrote: > Hi Mark, > > On 30 March 2016 at 15:41, Mark Rutland <mark.rutland@arm.com> wrote: >> On Wed, Mar 30, 2016 at 04:06:56PM +0300, Vladimir Zapolskiy wrote: >>> On 30.03.2016 14:06, Mark Rutland wrote: >>> > On Wed, Mar 30, 2016 at 12:30:41AM +0200, Joachim Eastwood wrote: >>> >> Add unit name to memory to remove the following warning: >>> >> Warning (unit_address_vs_reg): Node /memory has a reg or ranges >>> >> property, but no unit name >>> > >>> > If anything, it would be better to get rid of the memory node from the >>> > skeleton DTs. >>> > >>> > For DTs which have a memory node there's no problem, and DTs which >>> > expect a bootlaoder to fill things in have a logical place to document >>> > that fact. >> >>> The only problem I see if DTB is updated on a board but a board bootloader >>> on fix-up is capable to fill a preexisting "/memory" device node in only, >>> otherwise it is not clear why the device node is present in skeleton.dtsi. >> >> Sure. To clarify the above, what I expect that for this case is that the >> empty memory node would exist in the dts for that particular board, >> along with a comment, e.g. >> >> /* The firmware/bootloader for $BOARD fills this in */ >> memory { >> device_type = "memory"; >> reg = <0 0 0 0>; >> }; > > To avoid the warning with the new dtc this would need to be memory@0. Except memory is probably not actually at 0 here. This has come up before and been beaten to death. Bottom line is plain "memory" is allowed, and I plan to add that exception to dtc. >> That way you can tell at a glance that the lack of memory information in >> the DT for a board is intentional, and the bootloader still gets the >> node it expects. > > > But this doesn't seem to be a "problem" with any of the DTs in > arch/arm/boot as they all defined a memory node. > > I used the following script to check for the memory node in all built dtb's. > make ARCH=arm CONFIG_OF_ALL_DTBS=y dtbs > for i in $(ls arch/arm/boot/dts/*.dtb); do > m=$(scripts/dtc/dtc -I dtb -O dts $i | grep -m1 'memory.*{') > if [ -z "$m" ]; then > echo "Missing memory node in $i" > fi > done > > So it should be pretty safe to just remove the memory node entry in > the skeleton files. Unless I have missed something with the script > above. You've got to make sure they have 'device_type = "memory";' which they could rely on inheriting. Rob
On Thu, Mar 31, 2016 at 11:27 AM, Rob Herring <robh+dt@kernel.org> wrote: > On Wed, Mar 30, 2016 at 11:15 AM, Joachim Eastwood <manabian@gmail.com> wrote: >> Hi Mark, >> >> On 30 March 2016 at 15:41, Mark Rutland <mark.rutland@arm.com> wrote: >>> On Wed, Mar 30, 2016 at 04:06:56PM +0300, Vladimir Zapolskiy wrote: >>>> On 30.03.2016 14:06, Mark Rutland wrote: >>>> > On Wed, Mar 30, 2016 at 12:30:41AM +0200, Joachim Eastwood wrote: >>>> >> Add unit name to memory to remove the following warning: >>>> >> Warning (unit_address_vs_reg): Node /memory has a reg or ranges >>>> >> property, but no unit name >>>> > >>>> > If anything, it would be better to get rid of the memory node from the >>>> > skeleton DTs. >>>> > >>>> > For DTs which have a memory node there's no problem, and DTs which >>>> > expect a bootlaoder to fill things in have a logical place to document >>>> > that fact. >>> >>>> The only problem I see if DTB is updated on a board but a board bootloader >>>> on fix-up is capable to fill a preexisting "/memory" device node in only, >>>> otherwise it is not clear why the device node is present in skeleton.dtsi. >>> >>> Sure. To clarify the above, what I expect that for this case is that the >>> empty memory node would exist in the dts for that particular board, >>> along with a comment, e.g. >>> >>> /* The firmware/bootloader for $BOARD fills this in */ >>> memory { >>> device_type = "memory"; >>> reg = <0 0 0 0>; >>> }; >> >> To avoid the warning with the new dtc this would need to be memory@0. > > Except memory is probably not actually at 0 here. This has come up > before and been beaten to death. Bottom line is plain "memory" is > allowed, and I plan to add that exception to dtc. > >>> That way you can tell at a glance that the lack of memory information in >>> the DT for a board is intentional, and the bootloader still gets the >>> node it expects. >> >> >> But this doesn't seem to be a "problem" with any of the DTs in >> arch/arm/boot as they all defined a memory node. >> >> I used the following script to check for the memory node in all built dtb's. >> make ARCH=arm CONFIG_OF_ALL_DTBS=y dtbs >> for i in $(ls arch/arm/boot/dts/*.dtb); do >> m=$(scripts/dtc/dtc -I dtb -O dts $i | grep -m1 'memory.*{') >> if [ -z "$m" ]; then >> echo "Missing memory node in $i" >> fi >> done >> >> So it should be pretty safe to just remove the memory node entry in >> the skeleton files. Unless I have missed something with the script >> above. > > You've got to make sure they have 'device_type = "memory";' which they > could rely on inheriting. BTW, you could add this as a check to dtc and then it will find all the missing ones for you. Rob
On 31 March 2016 at 18:31, Rob Herring <robh+dt@kernel.org> wrote: > On Thu, Mar 31, 2016 at 11:27 AM, Rob Herring <robh+dt@kernel.org> wrote: >> On Wed, Mar 30, 2016 at 11:15 AM, Joachim Eastwood <manabian@gmail.com> wrote: >>> Hi Mark, >>> >>> On 30 March 2016 at 15:41, Mark Rutland <mark.rutland@arm.com> wrote: >>>> On Wed, Mar 30, 2016 at 04:06:56PM +0300, Vladimir Zapolskiy wrote: >>>>> On 30.03.2016 14:06, Mark Rutland wrote: >>>>> > On Wed, Mar 30, 2016 at 12:30:41AM +0200, Joachim Eastwood wrote: >>>>> >> Add unit name to memory to remove the following warning: >>>>> >> Warning (unit_address_vs_reg): Node /memory has a reg or ranges >>>>> >> property, but no unit name >>>>> > >>>>> > If anything, it would be better to get rid of the memory node from the >>>>> > skeleton DTs. >>>>> > >>>>> > For DTs which have a memory node there's no problem, and DTs which >>>>> > expect a bootlaoder to fill things in have a logical place to document >>>>> > that fact. >>>> >>>>> The only problem I see if DTB is updated on a board but a board bootloader >>>>> on fix-up is capable to fill a preexisting "/memory" device node in only, >>>>> otherwise it is not clear why the device node is present in skeleton.dtsi. >>>> >>>> Sure. To clarify the above, what I expect that for this case is that the >>>> empty memory node would exist in the dts for that particular board, >>>> along with a comment, e.g. >>>> >>>> /* The firmware/bootloader for $BOARD fills this in */ >>>> memory { >>>> device_type = "memory"; >>>> reg = <0 0 0 0>; >>>> }; >>> >>> To avoid the warning with the new dtc this would need to be memory@0. >> >> Except memory is probably not actually at 0 here. This has come up >> before and been beaten to death. Bottom line is plain "memory" is >> allowed, and I plan to add that exception to dtc. Okey, good. >>>> That way you can tell at a glance that the lack of memory information in >>>> the DT for a board is intentional, and the bootloader still gets the >>>> node it expects. >>> >>> >>> But this doesn't seem to be a "problem" with any of the DTs in >>> arch/arm/boot as they all defined a memory node. >>> >>> I used the following script to check for the memory node in all built dtb's. >>> make ARCH=arm CONFIG_OF_ALL_DTBS=y dtbs >>> for i in $(ls arch/arm/boot/dts/*.dtb); do >>> m=$(scripts/dtc/dtc -I dtb -O dts $i | grep -m1 'memory.*{') >>> if [ -z "$m" ]; then >>> echo "Missing memory node in $i" >>> fi >>> done >>> >>> So it should be pretty safe to just remove the memory node entry in >>> the skeleton files. Unless I have missed something with the script >>> above. >> >> You've got to make sure they have 'device_type = "memory";' which they >> could rely on inheriting. > > BTW, you could add this as a check to dtc and then it will find all > the missing ones for you. I have created a script now that will check and add it if it is missing. regards, Joachim Eastwood
On 31 March 2016 at 17:21, Joachim Eastwood <manabian@gmail.com> wrote: > On 31 March 2016 at 12:38, Mark Rutland <mark.rutland@arm.com> wrote: >> On Wed, Mar 30, 2016 at 10:45:06PM +0200, Joachim Eastwood wrote: >>> On 30 March 2016 at 19:06, Mark Rutland <mark.rutland@arm.com> wrote: >>> > On Wed, Mar 30, 2016 at 06:15:35PM +0200, Joachim Eastwood wrote: >>> >> I used the following script to check for the memory node in all built dtb's. >>> >> make ARCH=arm CONFIG_OF_ALL_DTBS=y dtbs >>> >> for i in $(ls arch/arm/boot/dts/*.dtb); do >>> >> m=$(scripts/dtc/dtc -I dtb -O dts $i | grep -m1 'memory.*{') >>> >> if [ -z "$m" ]; then >>> >> echo "Missing memory node in $i" >>> >> fi >>> >> done >>> >> >>> >> So it should be pretty safe to just remove the memory node entry in >>> >> the skeleton files. Unless I have missed something with the script >>> >> above. >>> > >>> > The above might match reserved-memory nodes; it might be better to check >>> > for 'device_type\s*=\s*"memory"'. >>> >>> I did check the output of the grep and it looks good. But there are >>> indeed DTs that are missing the 'device_type = "memory"' parameter. >>> Actually; _a lot_ or 438 of 741 to be exact. ugh... >>> >>> I guess all those should be fixed up before we can remove the memory >>> node from skeleton. :/ >> >> Ouch, yes. :( >> >> That said, the cahnges don't need to be an atomic operation. We could >> start adding device_type = "memory" to dts immediately (in as whatever >> size batches maintainers are happy with), as a duplicate device_type >> shouldn't be problematic. > > Yes, that is true. > > >> When we hit critical mass, we could then remove the skeleton memory >> nodes, fixing up any remaining fallout. >> >> As for the mechanical changes, it sounds like we need coccinelle for DT. >> >> That, or a laptop, a long flight, and a gin and tonic. > > :-) > > I'll see if I can cook up something with awk. Something like this should do it: #!/usr/bin/gawk -f # find arch/arm/boot/dts/ -type f -name *.dts* | xargs ./mem_node_add_dev_type.awk -i inplace BEGIN { go = 0 } /[^-]memory ?{/ { go = 1; idx = 0; device_type = 0 } go == 1 { buf[idx++] = $0 if ($0 ~ /device_type/) device_type = 1 if ($0 ~ /};/) { print buf[0] if (!device_type) { buf[0] = buf[1] gsub(/[^\t]*/, "", buf[0]) buf[0] = buf[0] "device_type = \"memory\";" } else { delete buf[0] } for (i in buf) print buf[i] delete buf go = 0 device_type = 0 } next } { print } Gives me: 249 files changed, 249 insertions(+), 1 deletion(-) on v4.6-rc1. Running the my check script after this reveals that 83 DTs lack the device_type parameter, but these seem to also lack the memory node as well. Wonder why my script didn't pick up the missing memory node in those... I also doubt that the memory node is set by the boot loader in some of these boards. regards, Joachim Eastwood
On Thu, Mar 31, 2016 at 11:34 AM, Joachim Eastwood <manabian@gmail.com> wrote: > On 31 March 2016 at 17:21, Joachim Eastwood <manabian@gmail.com> wrote: >> On 31 March 2016 at 12:38, Mark Rutland <mark.rutland@arm.com> wrote: >>> On Wed, Mar 30, 2016 at 10:45:06PM +0200, Joachim Eastwood wrote: >>>> On 30 March 2016 at 19:06, Mark Rutland <mark.rutland@arm.com> wrote: >>>> > On Wed, Mar 30, 2016 at 06:15:35PM +0200, Joachim Eastwood wrote: >>>> >> I used the following script to check for the memory node in all built dtb's. >>>> >> make ARCH=arm CONFIG_OF_ALL_DTBS=y dtbs >>>> >> for i in $(ls arch/arm/boot/dts/*.dtb); do >>>> >> m=$(scripts/dtc/dtc -I dtb -O dts $i | grep -m1 'memory.*{') >>>> >> if [ -z "$m" ]; then >>>> >> echo "Missing memory node in $i" >>>> >> fi >>>> >> done >>>> >> >>>> >> So it should be pretty safe to just remove the memory node entry in >>>> >> the skeleton files. Unless I have missed something with the script >>>> >> above. >>>> > >>>> > The above might match reserved-memory nodes; it might be better to check >>>> > for 'device_type\s*=\s*"memory"'. >>>> >>>> I did check the output of the grep and it looks good. But there are >>>> indeed DTs that are missing the 'device_type = "memory"' parameter. >>>> Actually; _a lot_ or 438 of 741 to be exact. ugh... >>>> >>>> I guess all those should be fixed up before we can remove the memory >>>> node from skeleton. :/ >>> >>> Ouch, yes. :( >>> >>> That said, the cahnges don't need to be an atomic operation. We could >>> start adding device_type = "memory" to dts immediately (in as whatever >>> size batches maintainers are happy with), as a duplicate device_type >>> shouldn't be problematic. >> >> Yes, that is true. >> >> >>> When we hit critical mass, we could then remove the skeleton memory >>> nodes, fixing up any remaining fallout. >>> >>> As for the mechanical changes, it sounds like we need coccinelle for DT. >>> >>> That, or a laptop, a long flight, and a gin and tonic. >> >> :-) >> >> I'll see if I can cook up something with awk. > > Something like this should do it: > #!/usr/bin/gawk -f > # find arch/arm/boot/dts/ -type f -name *.dts* | xargs > ./mem_node_add_dev_type.awk -i inplace > BEGIN { go = 0 } > /[^-]memory ?{/ { go = 1; idx = 0; device_type = 0 } > > go == 1 { > buf[idx++] = $0 > if ($0 ~ /device_type/) > device_type = 1 > > if ($0 ~ /};/) { > print buf[0] > > if (!device_type) { > buf[0] = buf[1] > gsub(/[^\t]*/, "", buf[0]) > buf[0] = buf[0] "device_type = \"memory\";" > } else { > delete buf[0] > } > > for (i in buf) > print buf[i] > delete buf > > go = 0 > device_type = 0 > } > next > } > > { print } > > Gives me: 249 files changed, 249 insertions(+), 1 deletion(-) on v4.6-rc1. > > Running the my check script after this reveals that 83 DTs lack the > device_type parameter, but these seem to also lack the memory node as > well. Wonder why my script didn't pick up the missing memory node in > those... I also doubt that the memory node is set by the boot loader > in some of these boards. I don't think this script is right. It will not work if the memory node is spread across dtsi files. You would need to compiler everything to dtb and then back to a flat dts file before running thru this script. I don't see the rest of the series in -next. Is that going to happen soon? Rob > > > regards, > Joachim Eastwood
On 13 April 2016 at 00:45, Rob Herring <robh+dt@kernel.org> wrote: > On Thu, Mar 31, 2016 at 11:34 AM, Joachim Eastwood <manabian@gmail.com> wrote: >> On 31 March 2016 at 17:21, Joachim Eastwood <manabian@gmail.com> wrote: >>> On 31 March 2016 at 12:38, Mark Rutland <mark.rutland@arm.com> wrote: >>>> On Wed, Mar 30, 2016 at 10:45:06PM +0200, Joachim Eastwood wrote: >>>>> On 30 March 2016 at 19:06, Mark Rutland <mark.rutland@arm.com> wrote: >>>>> > On Wed, Mar 30, 2016 at 06:15:35PM +0200, Joachim Eastwood wrote: >>>>> >> I used the following script to check for the memory node in all built dtb's. >>>>> >> make ARCH=arm CONFIG_OF_ALL_DTBS=y dtbs >>>>> >> for i in $(ls arch/arm/boot/dts/*.dtb); do >>>>> >> m=$(scripts/dtc/dtc -I dtb -O dts $i | grep -m1 'memory.*{') >>>>> >> if [ -z "$m" ]; then >>>>> >> echo "Missing memory node in $i" >>>>> >> fi >>>>> >> done >>>>> >> >>>>> >> So it should be pretty safe to just remove the memory node entry in >>>>> >> the skeleton files. Unless I have missed something with the script >>>>> >> above. >>>>> > >>>>> > The above might match reserved-memory nodes; it might be better to check >>>>> > for 'device_type\s*=\s*"memory"'. >>>>> >>>>> I did check the output of the grep and it looks good. But there are >>>>> indeed DTs that are missing the 'device_type = "memory"' parameter. >>>>> Actually; _a lot_ or 438 of 741 to be exact. ugh... >>>>> >>>>> I guess all those should be fixed up before we can remove the memory >>>>> node from skeleton. :/ >>>> >>>> Ouch, yes. :( >>>> >>>> That said, the cahnges don't need to be an atomic operation. We could >>>> start adding device_type = "memory" to dts immediately (in as whatever >>>> size batches maintainers are happy with), as a duplicate device_type >>>> shouldn't be problematic. >>> >>> Yes, that is true. >>> >>> >>>> When we hit critical mass, we could then remove the skeleton memory >>>> nodes, fixing up any remaining fallout. >>>> >>>> As for the mechanical changes, it sounds like we need coccinelle for DT. >>>> >>>> That, or a laptop, a long flight, and a gin and tonic. >>> >>> :-) >>> >>> I'll see if I can cook up something with awk. >> >> Something like this should do it: >> #!/usr/bin/gawk -f >> # find arch/arm/boot/dts/ -type f -name *.dts* | xargs >> ./mem_node_add_dev_type.awk -i inplace >> BEGIN { go = 0 } >> /[^-]memory ?{/ { go = 1; idx = 0; device_type = 0 } >> >> go == 1 { >> buf[idx++] = $0 >> if ($0 ~ /device_type/) >> device_type = 1 >> >> if ($0 ~ /};/) { >> print buf[0] >> >> if (!device_type) { >> buf[0] = buf[1] >> gsub(/[^\t]*/, "", buf[0]) >> buf[0] = buf[0] "device_type = \"memory\";" >> } else { >> delete buf[0] >> } >> >> for (i in buf) >> print buf[i] >> delete buf >> >> go = 0 >> device_type = 0 >> } >> next >> } >> >> { print } >> >> Gives me: 249 files changed, 249 insertions(+), 1 deletion(-) on v4.6-rc1. >> >> Running the my check script after this reveals that 83 DTs lack the >> device_type parameter, but these seem to also lack the memory node as >> well. Wonder why my script didn't pick up the missing memory node in >> those... I also doubt that the memory node is set by the boot loader >> in some of these boards. > > I don't think this script is right. It will not work if the memory > node is spread across dtsi files. You would need to compiler > everything to dtb and then back to a flat dts file before running thru > this script. The script will simply add 'device_type = memory' to all dts and dtsi files that are missing this property. What do you by spread across dtsi files? Got an example DT that I could take a look at? > I don't see the rest of the series in -next. Is that going to happen soon? Pull request was sent to arm-soc some time ago, but it seems they haven't pulled in anything for 4.7 yet. Link: http://permalink.gmane.org/gmane.linux.ports.arm.kernel/490303 (All memory node "fixes" was removed from this pull request) regards, Joachim Eastwood
diff --git a/arch/arm/boot/dts/skeleton.dtsi b/arch/arm/boot/dts/skeleton.dtsi index b41d241..a20da0a 100644 --- a/arch/arm/boot/dts/skeleton.dtsi +++ b/arch/arm/boot/dts/skeleton.dtsi @@ -9,5 +9,5 @@ #size-cells = <1>; chosen { }; aliases { }; - memory { device_type = "memory"; reg = <0 0>; }; + memory@0 { device_type = "memory"; reg = <0 0>; }; }; diff --git a/arch/arm/boot/dts/skeleton64.dtsi b/arch/arm/boot/dts/skeleton64.dtsi index b5d7f36..6dbe9f9 100644 --- a/arch/arm/boot/dts/skeleton64.dtsi +++ b/arch/arm/boot/dts/skeleton64.dtsi @@ -9,5 +9,5 @@ #size-cells = <2>; chosen { }; aliases { }; - memory { device_type = "memory"; reg = <0 0 0 0>; }; + memory@0 { device_type = "memory"; reg = <0 0 0 0>; }; };
Add unit name to memory to remove the following warning: Warning (unit_address_vs_reg): Node /memory has a reg or ranges property, but no unit name Signed-off-by: Joachim Eastwood <manabian@gmail.com> --- arch/arm/boot/dts/skeleton.dtsi | 2 +- arch/arm/boot/dts/skeleton64.dtsi | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)