[resent to the right list this time around]
Hi folks !
So I don't have the bandwidth to follow closely what's going on, but I
just today noticed the crackpot that went into 3.11 as part of commit:
9d8eab7af79cb4ce2de5de39f82c455b1f796963
drivers: of: add initialization code for dma reserved memory
Fist of all, do NOT add (or change) a binding as part of a patch
implementing code, it's gross.
Secondly, I don't know how much that binding was discussed on the list
(I assume it was and I just missed it) but it's gross.
It duplicates a binding that Jeremy Kerr had proposed a while ago for
a /reserved-ranges (and /reserved-names) pair of properties, possibly in
a better way but the fact is that the original binding received little
or no feedback and we went on and implemented support for it in powerpc
back in early 3.11 merge window.
Additionally, it has the following issues:
- It describes the "memory" node as /memory, which is WRONG
It should be "/memory@unit-address, this is important because the Linux
kernel of_find_device_by_path() isn't smart enough to do partial
searches (unlike the real OFW one) and thus to ignore the unit address
for search purposes, and you *need* the unit address if you have
multiple memory nodes (which you typically do on NUMA machines).
- To add to the above mistake, it defines "reserved memory" as a child
node of the "/memory" node. That is wrong or at least poorly thought
out. There can be several "memory" nodes, representing different areas
of memory, possibly even interleaved, having the reserved ranges as
children of a specific memory nodes thus doesn't work very well.
Breaking them up into regions based on what memory nodes they cover is
really nasty. Basically, the "reserved-memory" node should have been at
the root of the device-tree.
- It provides no indication of what a given region is used for (or used
by). In the example, "display_region" is a label (thus information that
is lost) and unless it's referenced by another node there is no good way
to know what this region is about which is quite annoying.
- The "memory-region" property is a phandle to a "reserved-memory"
region, this is not intuitive. If anything, the property should have
been called "reserved-memory-region".
- The way the "compatible" property is documented breaks a basic
premise that the device-tree is a hardware description and not some
convenient way to pass linux specific kernel parameters accross. It is
*ok* to pass some linux specific stuff and to make compromise based on
what a driver generally might expect but the whole business of using
that to describe what to put in CMA is pushing it pretty far ...
- The implementation of early_init_dt_scan_reserved_mem() will not work
on a setup whose /memory node has a unit address (typically /memory@0)
Now, I'd like to understand why not use the simpler binding originally
proposed by Jeremy, which had the advantage of proposing a unique name
per region in the traditional form "vendor,name", which then allows
drivers to pick up the region directly if they wish to query, remove or
update it in the tree for example (because they changed the framebuffer
address for example and want kexec to continue working).
I don't object to having a node per region, though it seemed unnecessary
at the time, but in any case, the current binding is crap and need to be
fixed urgently before its use spreads.
Ben.
On Sep 15, 2013, at 9:57 PM, Benjamin Herrenschmidt wrote:
> [resent to the right list this time around]
>
> Hi folks !
>
> So I don't have the bandwidth to follow closely what's going on, but I
> just today noticed the crackpot that went into 3.11 as part of commit:
>
> 9d8eab7af79cb4ce2de5de39f82c455b1f796963
> drivers: of: add initialization code for dma reserved memory
>
> Fist of all, do NOT add (or change) a binding as part of a patch
> implementing code, it's gross.
>
> Secondly, I don't know how much that binding was discussed on the list
> (I assume it was and I just missed it) but it's gross.
>
> It duplicates a binding that Jeremy Kerr had proposed a while ago for
> a /reserved-ranges (and /reserved-names) pair of properties, possibly in
> a better way but the fact is that the original binding received little
> or no feedback and we went on and implemented support for it in powerpc
> back in early 3.11 merge window.
>
> Additionally, it has the following issues:
>
> - It describes the "memory" node as /memory, which is WRONG
>
> It should be "/memory@unit-address, this is important because the Linux
> kernel of_find_device_by_path() isn't smart enough to do partial
> searches (unlike the real OFW one) and thus to ignore the unit address
> for search purposes, and you *need* the unit address if you have
> multiple memory nodes (which you typically do on NUMA machines).
>
> - To add to the above mistake, it defines "reserved memory" as a child
> node of the "/memory" node. That is wrong or at least poorly thought
> out. There can be several "memory" nodes, representing different areas
> of memory, possibly even interleaved, having the reserved ranges as
> children of a specific memory nodes thus doesn't work very well.
> Breaking them up into regions based on what memory nodes they cover is
> really nasty. Basically, the "reserved-memory" node should have been at
> the root of the device-tree.
>
> - It provides no indication of what a given region is used for (or used
> by). In the example, "display_region" is a label (thus information that
> is lost) and unless it's referenced by another node there is no good way
> to know what this region is about which is quite annoying.
>
> - The "memory-region" property is a phandle to a "reserved-memory"
> region, this is not intuitive. If anything, the property should have
> been called "reserved-memory-region".
>
> - The way the "compatible" property is documented breaks a basic
> premise that the device-tree is a hardware description and not some
> convenient way to pass linux specific kernel parameters accross. It is
> *ok* to pass some linux specific stuff and to make compromise based on
> what a driver generally might expect but the whole business of using
> that to describe what to put in CMA is pushing it pretty far ...
>
> - The implementation of early_init_dt_scan_reserved_mem() will not work
> on a setup whose /memory node has a unit address (typically /memory@0)
>
> Now, I'd like to understand why not use the simpler binding originally
> proposed by Jeremy, which had the advantage of proposing a unique name
> per region in the traditional form "vendor,name", which then allows
> drivers to pick up the region directly if they wish to query, remove or
> update it in the tree for example (because they changed the framebuffer
> address for example and want kexec to continue working).
>
> I don't object to having a node per region, though it seemed unnecessary
> at the time, but in any case, the current binding is crap and need to be
> fixed urgently before its use spreads.
>
> Ben.
Where is Jermey's binding documented ?
Is there concern of "breaking" whatever got merged in powerpc?
- k
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On 09/15/2013 08:57 PM, Benjamin Herrenschmidt wrote:
> [resent to the right list this time around]
>
> Hi folks !
>
> So I don't have the bandwidth to follow closely what's going on, but I
> just today noticed the crackpot that went into 3.11 as part of commit:
>
> 9d8eab7af79cb4ce2de5de39f82c455b1f796963
> drivers: of: add initialization code for dma reserved memory
>
> Fist of all, do NOT add (or change) a binding as part of a patch
> implementing code, it's gross.
Personally, I would argue the opposite; it's much easier to see what's
going on when it's all together in one patch. Ensuring ABI stability can
only be achieved through code review, i.e. splitting into separate
DT/code patches won't achieve that, so that argument doesn't affect this.
...
> Additionally, it has the following issues:
>
> - It describes the "memory" node as /memory, which is WRONG
>
> It should be "/memory@unit-address, this is important because the Linux
> kernel of_find_device_by_path() isn't smart enough to do partial
> searches (unlike the real OFW one) and thus to ignore the unit address
> for search purposes, and you *need* the unit address if you have
> multiple memory nodes (which you typically do on NUMA machines).
Perhaps /memory should have had a unit-address, but it never has had on
ARM; see arch/arm/boot/dts/skeleton.dtsi which says:
memory { device_type = "memory"; reg = <0 0>; };
... and the fact that reg in /memory can have multiple entries seems to
support the expectation we only have a single node here. I'm not sure
how we could possibly change this now it's become so entrenched?
On Mon, 2013-09-16 at 10:22 -0500, Kumar Gala wrote:
> Where is Jermey's binding documented ?
It looks like I actually came up with this binding :-) Jeremy reminded
me yesterday. It was posted to the DT list a while back, arguably we
should have merged it.
> Is there concern of "breaking" whatever got merged in powerpc?
No. I'm happy to switch over to something else but if anybody uses the
current one we have on powerpc, we can keep the code to support it as
well, it's not a lot of it.
Cheers,
Ben.
On Mon, 2013-09-16 at 10:17 -0600, Stephen Warren wrote:
> On 09/15/2013 08:57 PM, Benjamin Herrenschmidt wrote:
> > [resent to the right list this time around]
> >
> > Hi folks !
> >
> > So I don't have the bandwidth to follow closely what's going on, but I
> > just today noticed the crackpot that went into 3.11 as part of commit:
> >
> > 9d8eab7af79cb4ce2de5de39f82c455b1f796963
> > drivers: of: add initialization code for dma reserved memory
> >
> > Fist of all, do NOT add (or change) a binding as part of a patch
> > implementing code, it's gross.
>
> Personally, I would argue the opposite; it's much easier to see what's
> going on when it's all together in one patch.
One patch series eventually, but not the same patch.
> Ensuring ABI stability can
> only be achieved through code review, i.e. splitting into separate
> DT/code patches won't achieve that, so that argument doesn't affect this.
>
> ...
> > Additionally, it has the following issues:
> >
> > - It describes the "memory" node as /memory, which is WRONG
> >
> > It should be "/memory@unit-address, this is important because the Linux
> > kernel of_find_device_by_path() isn't smart enough to do partial
> > searches (unlike the real OFW one) and thus to ignore the unit address
> > for search purposes, and you *need* the unit address if you have
> > multiple memory nodes (which you typically do on NUMA machines).
>
> Perhaps /memory should have had a unit-address, but it never has had on
> ARM; see arch/arm/boot/dts/skeleton.dtsi which says:
Well, this is a mistake ARM folks might have done from day one but it
should still be fixed :-)
A node that has a "reg" property should have the corresponding unit
address.
> memory { device_type = "memory"; reg = <0 0>; };
>
> ... and the fact that reg in /memory can have multiple entries seems to
> support the expectation we only have a single node here. I'm not sure
> how we could possibly change this now it's become so entrenched?
Because everybody else does differently ? If you have things like NUMA
configurations where some memory ranges pertain to different nodes, you
need a memory node per NUMA node so you can add the other node-local
properties there.
The above should have been memory@0
The best way to fix this in a backward compatible manner is to once and
for all for our implementation of path-lookup to be able to work with
partial path components like a real OFW does, ie, name only, unit
address only, or both.
In anycase, just "/memory" will break on at least powerpc.
Ben.
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 16, 2013 at 3:46 PM, Benjamin Herrenschmidt
<[email protected]> wrote:
> On Mon, 2013-09-16 at 10:17 -0600, Stephen Warren wrote:
>> On 09/15/2013 08:57 PM, Benjamin Herrenschmidt wrote:
>> > [resent to the right list this time around]
>> >
>> > Hi folks !
>> >
>> > So I don't have the bandwidth to follow closely what's going on, but I
>> > just today noticed the crackpot that went into 3.11 as part of commit:
>> >
>> > 9d8eab7af79cb4ce2de5de39f82c455b1f796963
>> > drivers: of: add initialization code for dma reserved memory
>> >
>> > Fist of all, do NOT add (or change) a binding as part of a patch
>> > implementing code, it's gross.
>>
>> Personally, I would argue the opposite; it's much easier to see what's
>> going on when it's all together in one patch.
>
> One patch series eventually, but not the same patch.
>
>> Ensuring ABI stability can
>> only be achieved through code review, i.e. splitting into separate
>> DT/code patches won't achieve that, so that argument doesn't affect this.
>>
>> ...
>> > Additionally, it has the following issues:
>> >
>> > - It describes the "memory" node as /memory, which is WRONG
>> >
>> > It should be "/memory@unit-address, this is important because the Linux
>> > kernel of_find_device_by_path() isn't smart enough to do partial
>> > searches (unlike the real OFW one) and thus to ignore the unit address
>> > for search purposes, and you *need* the unit address if you have
>> > multiple memory nodes (which you typically do on NUMA machines).
>>
>> Perhaps /memory should have had a unit-address, but it never has had on
>> ARM; see arch/arm/boot/dts/skeleton.dtsi which says:
>
> Well, this is a mistake ARM folks might have done from day one but it
> should still be fixed :-)
>
> A node that has a "reg" property should have the corresponding unit
> address.
No, absolutely _NOT_ a requirement. Unit address is only required if
needed to disambiguate two properties with the same name.
If there are no ambiguities, then leaving off the unit address is much
preferred.
-Olof
On Mon, 2013-09-16 at 15:48 -0700, Olof Johansson wrote:
> > A node that has a "reg" property should have the corresponding unit
> > address.
>
> No, absolutely _NOT_ a requirement. Unit address is only required if
> needed to disambiguate two properties with the same name.
>
> If there are no ambiguities, then leaving off the unit address is much
> preferred.
I disagree :-)
Also this would be only true of our find_node_by_path was capable of
handling it, which it isn't. Thus you end up with generic code looking
for /memory and finding nothing ...
Cheers,
Ben.
On Mon, Sep 16, 2013 at 4:47 PM, Benjamin Herrenschmidt
<[email protected]> wrote:
> On Mon, 2013-09-16 at 15:48 -0700, Olof Johansson wrote:
>> > A node that has a "reg" property should have the corresponding unit
>> > address.
>>
>> No, absolutely _NOT_ a requirement. Unit address is only required if
>> needed to disambiguate two properties with the same name.
>>
>> If there are no ambiguities, then leaving off the unit address is much
>> preferred.
>
> I disagree :-)
Well, good thing you've got your own arch to litter the device trees
with unit specifiers in then. :)
> Also this would be only true of our find_node_by_path was capable of
> handling it, which it isn't. Thus you end up with generic code looking
> for /memory and finding nothing ...
Yes, this should be fixed.
-Olof
On Mon, 2013-09-16 at 16:48 -0700, Olof Johansson wrote:
> On Mon, Sep 16, 2013 at 4:47 PM, Benjamin Herrenschmidt
> <[email protected]> wrote:
> > On Mon, 2013-09-16 at 15:48 -0700, Olof Johansson wrote:
> >> > A node that has a "reg" property should have the corresponding unit
> >> > address.
> >>
> >> No, absolutely _NOT_ a requirement. Unit address is only required if
> >> needed to disambiguate two properties with the same name.
> >>
> >> If there are no ambiguities, then leaving off the unit address is much
> >> preferred.
> >
> > I disagree :-)
>
> Well, good thing you've got your own arch to litter the device trees
> with unit specifiers in then. :)
Right :-) We tend to have multiple memory nodes on server anyway so it's
not a big deal.
> > Also this would be only true of our find_node_by_path was capable of
> > handling it, which it isn't. Thus you end up with generic code looking
> > for /memory and finding nothing ...
>
> Yes, this should be fixed.
Right, the whole thing becomes mostly a non-issue once that's fixed. My
main objection isn't that ARM doesn't use unit address specifiers. My
objection is that the binding documents no unit address :-) It should
instead document the unit address with a note indicating that it can be
omitted if there is no ambiguity.
But first, do we have a volunteer to fix the path parsing code ? Also do
we *really* need to keep the path parsing code for fdt ? IE. It would be
annoying to have to duplicate that code for before and after
expansion...
Cheers,
Ben.
> -Olof
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 16 of September 2013 15:48:22 Olof Johansson wrote:
> On Mon, Sep 16, 2013 at 3:46 PM, Benjamin Herrenschmidt
>
> <[email protected]> wrote:
> > On Mon, 2013-09-16 at 10:17 -0600, Stephen Warren wrote:
> >> On 09/15/2013 08:57 PM, Benjamin Herrenschmidt wrote:
> >> > [resent to the right list this time around]
> >> >
> >> > Hi folks !
> >> >
> >> > So I don't have the bandwidth to follow closely what's going on,
> >> > but I
> >> > just today noticed the crackpot that went into 3.11 as part of
> >> > commit:
> >> >
> >> > 9d8eab7af79cb4ce2de5de39f82c455b1f796963
> >> > drivers: of: add initialization code for dma reserved memory
> >> >
> >> > Fist of all, do NOT add (or change) a binding as part of a patch
> >> > implementing code, it's gross.
> >>
> >> Personally, I would argue the opposite; it's much easier to see
> >> what's
> >> going on when it's all together in one patch.
> >
> > One patch series eventually, but not the same patch.
> >
> >> Ensuring ABI stability can
> >> only be achieved through code review, i.e. splitting into separate
> >> DT/code patches won't achieve that, so that argument doesn't affect
> >> this.
> >>
> >> ...
> >>
> >> > Additionally, it has the following issues:
> >> > - It describes the "memory" node as /memory, which is WRONG
> >> >
> >> > It should be "/memory@unit-address, this is important because the
> >> > Linux
> >> > kernel of_find_device_by_path() isn't smart enough to do partial
> >> > searches (unlike the real OFW one) and thus to ignore the unit
> >> > address
> >> > for search purposes, and you *need* the unit address if you have
> >> > multiple memory nodes (which you typically do on NUMA machines).
> >>
> >> Perhaps /memory should have had a unit-address, but it never has had
> >> on
> >
> >> ARM; see arch/arm/boot/dts/skeleton.dtsi which says:
> > Well, this is a mistake ARM folks might have done from day one but it
> > should still be fixed :-)
> >
> > A node that has a "reg" property should have the corresponding unit
> > address.
>
> No, absolutely _NOT_ a requirement. Unit address is only required if
> needed to disambiguate two properties with the same name.
>
> If there are no ambiguities, then leaving off the unit address is much
> preferred.
I'm afraid that I must disagree. For consistency I'd rather go with what
Ben said. Please see ePAPR chapter 2.2.1.1, which clearly defines how
nodes should be named.
Having unit-address whenever the node has a reg property has the nice
property of eliminating the need to rename any nodes when adding new one.
(Consider the case that you have one subnode somewhere and you omit the
unit-address and then you find out that you have to add another subnode
with the same name, but another reg value.)
Best regards,
Tomasz
On Sep 16, 2013, at 5:42 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2013-09-16 at 10:22 -0500, Kumar Gala wrote:
>> Where is Jermey's binding documented ?
>
> It looks like I actually came up with this binding :-) Jeremy reminded
> me yesterday. It was posted to the DT list a while back, arguably we
> should have merged it.
:), can you either repost the binding for discussion or post a link to the old thread.
>> Is there concern of "breaking" whatever got merged in powerpc?
>
> No. I'm happy to switch over to something else but if anybody uses the
> current one we have on powerpc, we can keep the code to support it as
> well, it's not a lot of it.
cool
- k
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On Tue, Sep 17, 2013 at 09:56:39AM +0200, Tomasz Figa wrote:
> I'm afraid that I must disagree. For consistency I'd rather go with what
> Ben said. Please see ePAPR chapter 2.2.1.1, which clearly defines how
> nodes should be named.
2.2.1.1 is there to point out that unit address _has_ to reflect reg.
2.2.3 says that unit addresses can be omitted.
> Having unit-address whenever the node has a reg property has the nice
> property of eliminating the need to rename any nodes when adding new one.
> (Consider the case that you have one subnode somewhere and you omit the
> unit-address and then you find out that you have to add another subnode
> with the same name, but another reg value.)
This motivation doesn't bother me at all -- it should be relatively rare.
-Olof
On 9/17/2013 9:43 AM, Olof Johansson wrote:
> On Tue, Sep 17, 2013 at 09:56:39AM +0200, Tomasz Figa wrote:
>> I'm afraid that I must disagree. For consistency I'd rather go with what
>> Ben said. Please see ePAPR chapter 2.2.1.1, which clearly defines how
>> nodes should be named.
>
> 2.2.1.1 is there to point out that unit address _has_ to reflect reg.
>
> 2.2.3 says that unit addresses can be omitted.
2.2.3 is talking about path names.
2.2.1.1 is talking about node names.
2.2.1.1 _does_ require the unit address in the node name, 2.2.3 does not
remove that requirement.
-Frank
>
>> Having unit-address whenever the node has a reg property has the nice
>> property of eliminating the need to rename any nodes when adding new one.
>> (Consider the case that you have one subnode somewhere and you omit the
>> unit-address and then you find out that you have to add another subnode
>> with the same name, but another reg value.)
>
> This motivation doesn't bother me at all -- it should be relatively rare.
>
>
> -Olof
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Tue, Sep 17, 2013 at 2:08 PM, Frank Rowand <[email protected]> wrote:
> On 9/17/2013 9:43 AM, Olof Johansson wrote:
>> On Tue, Sep 17, 2013 at 09:56:39AM +0200, Tomasz Figa wrote:
>>> I'm afraid that I must disagree. For consistency I'd rather go with what
>>> Ben said. Please see ePAPR chapter 2.2.1.1, which clearly defines how
>>> nodes should be named.
>>
>> 2.2.1.1 is there to point out that unit address _has_ to reflect reg.
>>
>> 2.2.3 says that unit addresses can be omitted.
>
> 2.2.3 is talking about path names.
>
> 2.2.1.1 is talking about node names.
>
> 2.2.1.1 _does_ require the unit address in the node name, 2.2.3 does not
> remove that requirement.
Sigh, that's horrible. OF clearly doesn't require it.
I guess people prefer to follow ePAPR even though it's broken? That
means someone needs to cleanup the current dts files. Any takers?
-Olof
On Tuesday 17 of September 2013 14:15:52 Olof Johansson wrote:
> On Tue, Sep 17, 2013 at 2:08 PM, Frank Rowand <[email protected]>
wrote:
> > On 9/17/2013 9:43 AM, Olof Johansson wrote:
> >> On Tue, Sep 17, 2013 at 09:56:39AM +0200, Tomasz Figa wrote:
> >>> I'm afraid that I must disagree. For consistency I'd rather go with
> >>> what Ben said. Please see ePAPR chapter 2.2.1.1, which clearly
> >>> defines how nodes should be named.
> >>
> >> 2.2.1.1 is there to point out that unit address _has_ to reflect reg.
> >>
> >> 2.2.3 says that unit addresses can be omitted.
> >
> > 2.2.3 is talking about path names.
> >
> > 2.2.1.1 is talking about node names.
> >
> > 2.2.1.1 _does_ require the unit address in the node name, 2.2.3 does
> > not remove that requirement.
>
> Sigh, that's horrible. OF clearly doesn't require it.
>
> I guess people prefer to follow ePAPR even though it's broken? That
> means someone needs to cleanup the current dts files. Any takers?
I don't think it's broken, why do you think so? It's at least consistent.
Probably not perfect and not complete, but IMHO a reasonable base for
further work. (Also at least something written down that people can learn
from and/or refer to.)
Best regards,
Tomasz
On Tue, Sep 17, 2013 at 2:19 PM, Tomasz Figa <[email protected]> wrote:
> On Tuesday 17 of September 2013 14:15:52 Olof Johansson wrote:
>> On Tue, Sep 17, 2013 at 2:08 PM, Frank Rowand <[email protected]>
> wrote:
>> > On 9/17/2013 9:43 AM, Olof Johansson wrote:
>> >> On Tue, Sep 17, 2013 at 09:56:39AM +0200, Tomasz Figa wrote:
>> >>> I'm afraid that I must disagree. For consistency I'd rather go with
>> >>> what Ben said. Please see ePAPR chapter 2.2.1.1, which clearly
>> >>> defines how nodes should be named.
>> >>
>> >> 2.2.1.1 is there to point out that unit address _has_ to reflect reg.
>> >>
>> >> 2.2.3 says that unit addresses can be omitted.
>> >
>> > 2.2.3 is talking about path names.
>> >
>> > 2.2.1.1 is talking about node names.
>> >
>> > 2.2.1.1 _does_ require the unit address in the node name, 2.2.3 does
>> > not remove that requirement.
>>
>> Sigh, that's horrible. OF clearly doesn't require it.
>>
>> I guess people prefer to follow ePAPR even though it's broken? That
>> means someone needs to cleanup the current dts files. Any takers?
>
> I don't think it's broken, why do you think so? It's at least consistent.
> Probably not perfect and not complete, but IMHO a reasonable base for
> further work. (Also at least something written down that people can learn
> from and/or refer to.)
So, I stand corrected. It seems that at least one legacy system I'm
looking at always specifies unit address as well. I don't like it but
I'll stop arguing.
Ben: The interesting part is that it does _not_ specify it on /memory
though. Nor, of course, on /cpus or /openprom. So assuming /memory@0
exists will break even on some powerpc platforms.
-Olof
On Tue, 2013-09-17 at 14:15 -0700, Olof Johansson wrote:
> Sigh, that's horrible. OF clearly doesn't require it.
Doesn't it ?
All OF implementations will create it, you would have to explicitly
remove the encode-unit method of the parent to make it disappear...
All I can find in 1275 is:
<<
Some nodes in the device tree do not represent physical devices. These
system nodes are used instead for various
general firmware purposes. System nodes do not have physical addresses.
Their node names have a driver name
field but not a unit address field.
>>
That implies that such nodes also don't have a "reg" property (ie. "do
not have physical address").
I don't see anything else, if anything, the definition of the node name
seems to not have provisions for a missing unit address.
The only case in OF that I know where the unit address is not present is
wildcard nodes (also known as protocol nodes) which also don't have a
"reg" property such as used by some SCSI controllers when the fcode
doesn't want to probe the bus at boot and requires the unit address to
be explicitly passed in the "open" call. This is clearly not the case
here.
Or did I miss something ?
> I guess people prefer to follow ePAPR even though it's broken? That
> means someone needs to cleanup the current dts files. Any takers?
It's not broken. I don't understand why you are so adamant about
that :-)
Cheers,
Ben.
On Tue, 2013-09-17 at 14:33 -0700, Olof Johansson wrote:
> > I don't think it's broken, why do you think so? It's at least
> consistent.
> > Probably not perfect and not complete, but IMHO a reasonable base
> for
> > further work. (Also at least something written down that people can
> learn
> > from and/or refer to.)
>
> So, I stand corrected. It seems that at least one legacy system I'm
> looking at always specifies unit address as well. I don't like it but
> I'll stop arguing.
>
> Ben: The interesting part is that it does _not_ specify it on /memory
> though. Nor, of course, on /cpus or /openprom. So assuming /memory@0
> exists will break even on some powerpc platforms.
What system is that out of curiosity ? Also make sure it's not just
Linux being an idiot and stripping the @0 in /proc/device-tree ...
(I think some old versions of /proc code would strip it)
Or is that some insanely broken OF like Apple old world or Pegasos ?
If it's just embedded .dts files, yes, I fixed some, but we might still
have some bad ones.
In any case, we all agree, the right thing to do first is to fix our
path parser to cope either way.
Cheers,
Ben.
On Tue, Sep 17, 2013 at 4:04 PM, Benjamin Herrenschmidt
<[email protected]> wrote:
> On Tue, 2013-09-17 at 14:33 -0700, Olof Johansson wrote:
>> > I don't think it's broken, why do you think so? It's at least
>> consistent.
>> > Probably not perfect and not complete, but IMHO a reasonable base
>> for
>> > further work. (Also at least something written down that people can
>> learn
>> > from and/or refer to.)
>>
>> So, I stand corrected. It seems that at least one legacy system I'm
>> looking at always specifies unit address as well. I don't like it but
>> I'll stop arguing.
>>
>> Ben: The interesting part is that it does _not_ specify it on /memory
>> though. Nor, of course, on /cpus or /openprom. So assuming /memory@0
>> exists will break even on some powerpc platforms.
>
> What system is that out of curiosity ? Also make sure it's not just
> Linux being an idiot and stripping the @0 in /proc/device-tree ...
>
> (I think some old versions of /proc code would strip it)
>
> Or is that some insanely broken OF like Apple old world or Pegasos ?
>
> If it's just embedded .dts files, yes, I fixed some, but we might still
> have some bad ones.
The only powerpc hardware I still have these days is PA Semi systems,
so it's from one of them, with current -next kernel. Booted with OF
client interface, no dts file that can be fixed.
> In any case, we all agree, the right thing to do first is to fix our
> path parser to cope either way.
Yep, I just wanted to alert you that there's powerpc systems out there
with just /memory as well.
-Olof
On Tue, Sep 17, 2013 at 02:08:33PM -0700, Frank Rowand wrote:
> On 9/17/2013 9:43 AM, Olof Johansson wrote:
> > On Tue, Sep 17, 2013 at 09:56:39AM +0200, Tomasz Figa wrote:
> >> I'm afraid that I must disagree. For consistency I'd rather go with what
> >> Ben said. Please see ePAPR chapter 2.2.1.1, which clearly defines how
> >> nodes should be named.
> >
> > 2.2.1.1 is there to point out that unit address _has_ to reflect reg.
> >
> > 2.2.3 says that unit addresses can be omitted.
>
> 2.2.3 is talking about path names.
>
> 2.2.1.1 is talking about node names.
>
> 2.2.1.1 _does_ require the unit address in the node name, 2.2.3 does not
> remove that requirement.
Certainly the recommendation I've been giving from the early days of
ePAPR has been that a node should have a unit address if and only if
it has a 'reg' property.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On Tue, 17 Sep 2013 08:46:07 +1000, Benjamin Herrenschmidt <[email protected]> wrote:
> In anycase, just "/memory" will break on at least powerpc.
Ummm, really?
~/hacking/linux$ git grep 'memory {' arch/powerpc/boot/dts/* | wc -l
159
~/hacking/linux$ git grep 'memory@' arch/powerpc/boot/dts/* | wc -l
4
g.
On Mon, 16 Sep 2013 12:57:54 +1000, Benjamin Herrenschmidt <[email protected]> wrote:
> [resent to the right list this time around]
>
> Hi folks !
>
> So I don't have the bandwidth to follow closely what's going on, but I
> just today noticed the crackpot that went into 3.11 as part of commit:
>
> 9d8eab7af79cb4ce2de5de39f82c455b1f796963
> drivers: of: add initialization code for dma reserved memory
>
> Fist of all, do NOT add (or change) a binding as part of a patch
> implementing code, it's gross.
I really don't have any problem with a single patch including both. In
many cases it is easier to review when they are in the same patch...
That said, there is a plan to move the bindings out into a separate
repository which will make the issue moot in the near future.
> Secondly, I don't know how much that binding was discussed on the list
> (I assume it was and I just missed it) but it's gross.
>
> It duplicates a binding that Jeremy Kerr had proposed a while ago for
> a /reserved-ranges (and /reserved-names) pair of properties, possibly in
> a better way but the fact is that the original binding received little
> or no feedback and we went on and implemented support for it in powerpc
> back in early 3.11 merge window.
reserved-ranges seemed too be too limited. Specifically there is a need
to have devices bind to specific regions. Consider for example a device
with two video devices with initialized framebuffers. The intent was to
reference a specific region from the device. A phandle is a natural way
to do that, and it allows for later additional attributes or
descriptions to be added to reserved region nodes.
BTW, at the risk of sounding petty, I noticed that the
early_reserve_mem_dt() implementation was made powerpc-only despite none
of it being powerpc specific. That really should have been put into
common code. :-p
> Additionally, it has the following issues:
>
> - It describes the "memory" node as /memory, which is WRONG
>
> It should be "/memory@unit-address, this is important because the Linux
> kernel of_find_device_by_path() isn't smart enough to do partial
> searches (unlike the real OFW one) and thus to ignore the unit address
> for search purposes, and you *need* the unit address if you have
> multiple memory nodes (which you typically do on NUMA machines).
As discussed elsewhere in this thread, there is precidence for both
/memory and /memory@unit-address. Both need to work.
> - To add to the above mistake, it defines "reserved memory" as a child
> node of the "/memory" node. That is wrong or at least poorly thought
> out. There can be several "memory" nodes, representing different areas
> of memory, possibly even interleaved, having the reserved ranges as
> children of a specific memory nodes thus doesn't work very well.
> Breaking them up into regions based on what memory nodes they cover is
> really nasty. Basically, the "reserved-memory" node should have been at
> the root of the device-tree.
I would be okay with that. We went back and forth on the best place for
it to live a number of times. The thought when placing it under the
memory node was that a region is (probably) going to be associated with
a particular bank of memory. Therefore it makes sense to be a child of
that bank of memory. If you feel strongly on this one then I have no
problem moving it to the root of the tree.
> - It provides no indication of what a given region is used for (or used
> by). In the example, "display_region" is a label (thus information that
> is lost) and unless it's referenced by another node there is no good way
> to know what this region is about which is quite annoying.
Does this actually matter? In most cases the regions are completely
anonymous until referenced by a specific device and by default the
kernel should not use until it knows what the region is for.
We can however add properties to give the kernel hints on the usage. For
instance, if a regions isn't in use at boot time, then it would be fine
for the kernel to use it for movable pages up until the time a device
asks for the region (ie. CMA regions can be used this way).
> - The "memory-region" property is a phandle to a "reserved-memory"
> region, this is not intuitive. If anything, the property should have
> been called "reserved-memory-region".
Sure. I don't see the problem, but I have no problem changing it if you
feel strongly about it.
> - The way the "compatible" property is documented breaks a basic
> premise that the device-tree is a hardware description and not some
> convenient way to pass linux specific kernel parameters accross. It is
> *ok* to pass some linux specific stuff and to make compromise based on
> what a driver generally might expect but the whole business of using
> that to describe what to put in CMA is pushing it pretty far ...
I disagree. Originally I had the same reaction, but I've been swayed to
the point of view that setting aside regions is actually a pretty
hardware-centric thing because it describes how the hardware needs to be
used. I've already used the example of a framebuffer. It may not stricly
be hardware, but it is a description of how the hardware is setup that
the kernel should respect. Similarly, the size and placement of CMA
regions are more than merely a software parameter because they are
defined specifically to support the hardware devices.
> - The implementation of early_init_dt_scan_reserved_mem() will not work
> on a setup whose /memory node has a unit address (typically /memory@0)
Agreed, that should be fixed. It should work regardless of whether or
not the memory node(s) have a unit address.
> Now, I'd like to understand why not use the simpler binding originally
> proposed by Jeremy, which had the advantage of proposing a unique name
> per region in the traditional form "vendor,name", which then allows
> drivers to pick up the region directly if they wish to query, remove or
> update it in the tree for example (because they changed the framebuffer
> address for example and want kexec to continue working).
Hmmm... I don't follow. How is query/remove/update any more or less
difficult between the two approaches? Updating the reg property should
be doable in-place with both methods, but finding a specific region
associated with a device is explicit in the nodes-and-phandles approach.
(unless the 'name' part of vendor,name is an instance name and the
device node has a property containing the name; ie "acme,framebuffer1",
"acme,framebuffer2", and then in the device node something like:
framebuffer-region = "acme,framebuffer2";)
> I don't object to having a node per region, though it seemed unnecessary
> at the time, but in any case, the current binding is crap and need to be
> fixed urgently before its use spreads.
It seems to me that the 'top-level' objection is the creation of a new
binding using a node per region. I think it is warrented. If you
disagree strongly then we'll revert the whole series and try again for
v3.13. Otherwise I think the other objections are fixable in this cycle.
g.
On Tue, 17 Sep 2013 09:56:39 +0200, Tomasz Figa <[email protected]> wrote:
> On Monday 16 of September 2013 15:48:22 Olof Johansson wrote:
> > On Mon, Sep 16, 2013 at 3:46 PM, Benjamin Herrenschmidt
> > > On Mon, 2013-09-16 at 10:17 -0600, Stephen Warren wrote:
> > >> On 09/15/2013 08:57 PM, Benjamin Herrenschmidt wrote:
> > >> > It should be "/memory@unit-address, this is important because the
> > >> > Linux
> > >> > kernel of_find_device_by_path() isn't smart enough to do partial
> > >> > searches (unlike the real OFW one) and thus to ignore the unit
> > >> > address
> > >> > for search purposes, and you *need* the unit address if you have
> > >> > multiple memory nodes (which you typically do on NUMA machines).
> > >>
> > >> Perhaps /memory should have had a unit-address, but it never has had
> > >> on
> > >
> > >> ARM; see arch/arm/boot/dts/skeleton.dtsi which says:
> > > Well, this is a mistake ARM folks might have done from day one but it
> > > should still be fixed :-)
> > >
> > > A node that has a "reg" property should have the corresponding unit
> > > address.
> >
> > No, absolutely _NOT_ a requirement. Unit address is only required if
> > needed to disambiguate two properties with the same name.
> >
> > If there are no ambiguities, then leaving off the unit address is much
> > preferred.
>
> I'm afraid that I must disagree. For consistency I'd rather go with what
> Ben said. Please see ePAPR chapter 2.2.1.1, which clearly defines how
> nodes should be named.
Regardless of the ePAPR spec, there is loads of precidence of having a
single memory node without the unit address, and we have to continue to
support those device trees. Just do a "git grep 'memory ' arch/*/boot/dts/*"
to get a big list. You'll note that there are a lot of powerpc matches
in that list.
Similarly, if the kernel chokes on memory node(s) that have the unit
address then that is a bug and needs to be fixed.
g.
On Tue, 2013-09-17 at 18:38 -0700, Grant Likely wrote:
> On Tue, 17 Sep 2013 08:46:07 +1000, Benjamin Herrenschmidt <[email protected]> wrote:
> > In anycase, just "/memory" will break on at least powerpc.
>
> Ummm, really?
I meant the search for just '/memory' will break with the current path
searching algorithm on all powerpc machines that have the unit address.
We might have been also omitting it from some of our device-trees but
most of our real OF based machines will break and the stuff I'm
currently working on that does exploit the reserved stuff will.
Anything that supports multiple memory nodes will break for example.
It's a separate argument as to whether omitting the unit address is the
right thing to do in the dts. I don't like it but it's indeed a common
practice so we shouldn't make it not work anymore. However we shouldn't
*document* the memory node binding as having no unit address.
But as we have agreed, fixing the path searching would go a long way
toward fixing the issue kernel-side while retaining the compatibility
with both type of device-trees.
Now regarding the specific issue of reserved memory, I still maintain
that this shouldn't be a child of the memory nodes since that's simply
not practical the minute you have multiple memory nodes.
I also think we are mixing up two very different concepts here and we
might be better off just handling them separately:
- Marking areas of memory as reserved via the device-tree in order to
prevent SW (Linux, bootloader, ...) from stomping over them because they
are in use typically by some kind of driver or contain other information
that should be preserved. This is basically a better form of the
existing reserve map. The two main feature we are after here as far as
I'm concerned are
1) be explicit in the device-tree instead of the header
which means they are visible in /proc/device-tree,
can potentially survive kexec, etc....
2) be "named" (using vendor,function) so that the user can
have a rough idea of what this is about *and* the driver
can take ownership of them if needed. For example, if the
firmware has configured an in-memory framebuffer, it can
be reserved that way. Later, when the Linux DRM driver
loads, it might decide to move that region around and can
thus find and update or remove that property so it remains
consistent for kexec.
Both of these are covered by the original binding I proposed (and
implemented on powerpc) of having a /reserved-ranges and /reserved-names
in the device-tree. But I'm not married to that binding and if the
general consensus is that we should make them nodes, then so be it,
but I disagree with having them as children of the /memory node.
- "Segmenting" the physical memory into regions for use either by CMA
or by devices to indicate, possibly, the preferred areas for use by
those drivers. Fundamentally that memory is perfectly good to use by
Linux, and in fact this is arguably not HW description (though it's
acceptable as a "hint" to the operating system of what a good memory
usage would be for the platform).
Whether it makes sense to collate both into the same representation,
I am very unsure of.
Cheers,
Ben.
> ~/hacking/linux$ git grep 'memory {' arch/powerpc/boot/dts/* | wc -l
> 159
> ~/hacking/linux$ git grep 'memory@' arch/powerpc/boot/dts/* | wc -l
> 4
>
> g.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Tue, 2013-09-17 at 21:57 -0500, Grant Likely wrote:
> > - It provides no indication of what a given region is used for (or used
> > by). In the example, "display_region" is a label (thus information that
> > is lost) and unless it's referenced by another node there is no good way
> > to know what this region is about which is quite annoying.
>
> Does this actually matter? In most cases the regions are completely
> anonymous until referenced by a specific device and by default the
> kernel should not use until it knows what the region is for.
First it's handy for the developer to know for
debug/diagnostic/you-name-it purposes. You don't always know what the
heck the firmware is doing and there are some cases where such regions
exist independently of any specific device.
The ability of the node of the driver to have a phandle pointing to a is
indeed a nice feature and that's a good point in favor of making them
nodes. But I would be against *requiring* it.
I might have some architecture code that knows that this region is for
use by some internal DMA translation cache or god knows what without
having a clear device node as the "owner" of the region, there are going
to be a few special cases like that and we still want to be able to
identify it.
So I would definitely want them named. Guess what ? They are all
children of /reserved-memory right ? So their individual name doesn't
matter one bit, thus the node name can perfectly well serve that
purpose.
> We can however add properties to give the kernel hints on the usage. For
> instance, if a regions isn't in use at boot time, then it would be fine
> for the kernel to use it for movable pages up until the time a device
> asks for the region (ie. CMA regions can be used this way).
Let's not get into something overly Linux-centric though...
> > - The "memory-region" property is a phandle to a "reserved-memory"
> > region, this is not intuitive. If anything, the property should have
> > been called "reserved-memory-region".
>
> Sure. I don't see the problem, but I have no problem changing it if you
> feel strongly about it.
Well it all boils down to whether we turn that whole thing into a way to
describe arbitrary memory regions (and not just reserved ones), for
example, CMA stuff as mentioned above doesn't strictly need to be
reserved, in which case, we would call the whole thing /memory-regions
and the property could be named the same. In that case we do want a
specific property however in each region node to indicate whether it
should be strictly reserved or not.
But I would argue against that unless we have a very clear use case,
because it's starting to smell a lot like trying to solve world hunger
with over engineering :-)
> > - The way the "compatible" property is documented breaks a basic
> > premise that the device-tree is a hardware description and not some
> > convenient way to pass linux specific kernel parameters accross. It is
> > *ok* to pass some linux specific stuff and to make compromise based on
> > what a driver generally might expect but the whole business of using
> > that to describe what to put in CMA is pushing it pretty far ...
>
> I disagree. Originally I had the same reaction, but I've been swayed to
> the point of view that setting aside regions is actually a pretty
> hardware-centric thing because it describes how the hardware needs to be
> used.
I would still not use the "compatible" property for that. Maybe
recommended-usage ? Or simply "usage" property with well defined
semantics ? "reserved" would be one, "consistent-memory" would be
another (not strictly reserved but thrown into the CMA pool at first
notice) etc... ?
> I've already used the example of a framebuffer. It may not stricly
> be hardware, but it is a description of how the hardware is setup that
> the kernel should respect. Similarly, the size and placement of CMA
> regions are more than merely a software parameter because they are
> defined specifically to support the hardware devices.
Right and the advantage of using a node with a "reg" property here is
that a region can actually be made of separate ranges.
> > - The implementation of early_init_dt_scan_reserved_mem() will not work
> > on a setup whose /memory node has a unit address (typically /memory@0)
>
> Agreed, that should be fixed. It should work regardless of whether or
> not the memory node(s) have a unit address.
>
> > Now, I'd like to understand why not use the simpler binding originally
> > proposed by Jeremy, which had the advantage of proposing a unique name
> > per region in the traditional form "vendor,name", which then allows
> > drivers to pick up the region directly if they wish to query, remove or
> > update it in the tree for example (because they changed the framebuffer
> > address for example and want kexec to continue working).
>
> Hmmm... I don't follow. How is query/remove/update any more or less
> difficult between the two approaches? Updating the reg property should
> be doable in-place with both methods, but finding a specific region
> associated with a device is explicit in the nodes-and-phandles approach.
> (unless the 'name' part of vendor,name is an instance name and the
> device node has a property containing the name; ie "acme,framebuffer1",
> "acme,framebuffer2", and then in the device node something like:
> framebuffer-region = "acme,framebuffer2";)
>
> > I don't object to having a node per region, though it seemed unnecessary
> > at the time, but in any case, the current binding is crap and need to be
> > fixed urgently before its use spreads.
>
> It seems to me that the 'top-level' objection is the creation of a new
> binding using a node per region. I think it is warrented. If you
> disagree strongly then we'll revert the whole series and try again for
> v3.13. Otherwise I think the other objections are fixable in this cycle.
No. I don't fundamentally object to using a node per region, it does
have some advantages indeed as I have mentioned in a few places.
My main issues are:
- Documentation of the "memory" binding. This is a separate thing that
shouldn't have been there and should not document the node without the
unit address (it's ok to specify I suppose that it can be ommitted
though I don't like it).
- Location. I don't want that under /memory. I have to deal with
machines with multiple /memory nodes and regions potentially straddling
them.
- The choice of properties for describing, naming and defining their
usage. I think we can come to an agreement here.
In fact the use of a node per region is the *least* of my worries :-)
Cheers,
Ben.
> g.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On 09/17/2013 03:15 PM, Olof Johansson wrote:
> On Tue, Sep 17, 2013 at 2:08 PM, Frank Rowand <[email protected]> wrote:
>> On 9/17/2013 9:43 AM, Olof Johansson wrote:
>>> On Tue, Sep 17, 2013 at 09:56:39AM +0200, Tomasz Figa wrote:
>>>> I'm afraid that I must disagree. For consistency I'd rather go with what
>>>> Ben said. Please see ePAPR chapter 2.2.1.1, which clearly defines how
>>>> nodes should be named.
>>>
>>> 2.2.1.1 is there to point out that unit address _has_ to reflect reg.
>>>
>>> 2.2.3 says that unit addresses can be omitted.
>>
>> 2.2.3 is talking about path names.
>>
>> 2.2.1.1 is talking about node names.
>>
>> 2.2.1.1 _does_ require the unit address in the node name, 2.2.3 does not
>> remove that requirement.
>
> Sigh, that's horrible. OF clearly doesn't require it.
>
> I guess people prefer to follow ePAPR even though it's broken? That
> means someone needs to cleanup the current dts files. Any takers?
FWIW, I investigated enhancing dtc to enforce this rule. Here are the
results:
********** TEST SUMMARY
* Total testcases: 1446
* PASS: 1252
* FAIL: 58
* Bad configuration: 136
* Strange test result: 0
**********
That's just in dtc itself, and not any of the *.dts in the kernel or
U-Boot source trees...
I'll see how much of patch it takes to fix up all the test-cases in dtc.
On Wed, Sep 18, 2013 at 10:28:44AM -0600, Stephen Warren wrote:
> On 09/17/2013 03:15 PM, Olof Johansson wrote:
> > On Tue, Sep 17, 2013 at 2:08 PM, Frank Rowand <[email protected]> wrote:
> >> On 9/17/2013 9:43 AM, Olof Johansson wrote:
> >>> On Tue, Sep 17, 2013 at 09:56:39AM +0200, Tomasz Figa wrote:
> >>>> I'm afraid that I must disagree. For consistency I'd rather go with what
> >>>> Ben said. Please see ePAPR chapter 2.2.1.1, which clearly defines how
> >>>> nodes should be named.
> >>>
> >>> 2.2.1.1 is there to point out that unit address _has_ to reflect reg.
> >>>
> >>> 2.2.3 says that unit addresses can be omitted.
> >>
> >> 2.2.3 is talking about path names.
> >>
> >> 2.2.1.1 is talking about node names.
> >>
> >> 2.2.1.1 _does_ require the unit address in the node name, 2.2.3 does not
> >> remove that requirement.
> >
> > Sigh, that's horrible. OF clearly doesn't require it.
> >
> > I guess people prefer to follow ePAPR even though it's broken? That
> > means someone needs to cleanup the current dts files. Any takers?
>
> FWIW, I investigated enhancing dtc to enforce this rule. Here are the
> results:
>
> ********** TEST SUMMARY
> * Total testcases: 1446
> * PASS: 1252
> * FAIL: 58
> * Bad configuration: 136
> * Strange test result: 0
> **********
>
> That's just in dtc itself, and not any of the *.dts in the kernel or
> U-Boot source trees...
Uh.. yeah. The trees in the dtc testsuite are rather contrived and
not good examples of device trees in general. They're really purely
examples of dts syntax, and don't at all resemble typical dt contents.
> I'll see how much of patch it takes to fix up all the test-cases in dtc.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On Sep 18, 2013, at 3:21 AM, Benjamin Herrenschmidt wrote:
> On Tue, 2013-09-17 at 21:57 -0500, Grant Likely wrote:
>
>>> - It provides no indication of what a given region is used for (or used
>>> by). In the example, "display_region" is a label (thus information that
>>> is lost) and unless it's referenced by another node there is no good way
>>> to know what this region is about which is quite annoying.
>>
>> Does this actually matter? In most cases the regions are completely
>> anonymous until referenced by a specific device and by default the
>> kernel should not use until it knows what the region is for.
>
> First it's handy for the developer to know for
> debug/diagnostic/you-name-it purposes. You don't always know what the
> heck the firmware is doing and there are some cases where such regions
> exist independently of any specific device.
>
> The ability of the node of the driver to have a phandle pointing to a is
> indeed a nice feature and that's a good point in favor of making them
> nodes. But I would be against *requiring* it.
>
> I might have some architecture code that knows that this region is for
> use by some internal DMA translation cache or god knows what without
> having a clear device node as the "owner" of the region, there are going
> to be a few special cases like that and we still want to be able to
> identify it.
>
> So I would definitely want them named. Guess what ? They are all
> children of /reserved-memory right ? So their individual name doesn't
> matter one bit, thus the node name can perfectly well serve that
> purpose.
>
>> We can however add properties to give the kernel hints on the usage. For
>> instance, if a regions isn't in use at boot time, then it would be fine
>> for the kernel to use it for movable pages up until the time a device
>> asks for the region (ie. CMA regions can be used this way).
>
> Let's not get into something overly Linux-centric though...
>
>>> - The "memory-region" property is a phandle to a "reserved-memory"
>>> region, this is not intuitive. If anything, the property should have
>>> been called "reserved-memory-region".
>>
>> Sure. I don't see the problem, but I have no problem changing it if you
>> feel strongly about it.
>
> Well it all boils down to whether we turn that whole thing into a way to
> describe arbitrary memory regions (and not just reserved ones), for
> example, CMA stuff as mentioned above doesn't strictly need to be
> reserved, in which case, we would call the whole thing /memory-regions
> and the property could be named the same. In that case we do want a
> specific property however in each region node to indicate whether it
> should be strictly reserved or not.
>
> But I would argue against that unless we have a very clear use case,
> because it's starting to smell a lot like trying to solve world hunger
> with over engineering :-)
>
>>> - The way the "compatible" property is documented breaks a basic
>>> premise that the device-tree is a hardware description and not some
>>> convenient way to pass linux specific kernel parameters accross. It is
>>> *ok* to pass some linux specific stuff and to make compromise based on
>>> what a driver generally might expect but the whole business of using
>>> that to describe what to put in CMA is pushing it pretty far ...
>>
>> I disagree. Originally I had the same reaction, but I've been swayed to
>> the point of view that setting aside regions is actually a pretty
>> hardware-centric thing because it describes how the hardware needs to be
>> used.
>
> I would still not use the "compatible" property for that. Maybe
> recommended-usage ? Or simply "usage" property with well defined
> semantics ? "reserved" would be one, "consistent-memory" would be
> another (not strictly reserved but thrown into the CMA pool at first
> notice) etc... ?
>
>> I've already used the example of a framebuffer. It may not stricly
>> be hardware, but it is a description of how the hardware is setup that
>> the kernel should respect. Similarly, the size and placement of CMA
>> regions are more than merely a software parameter because they are
>> defined specifically to support the hardware devices.
>
> Right and the advantage of using a node with a "reg" property here is
> that a region can actually be made of separate ranges.
>
>>> - The implementation of early_init_dt_scan_reserved_mem() will not work
>>> on a setup whose /memory node has a unit address (typically /memory@0)
>>
>> Agreed, that should be fixed. It should work regardless of whether or
>> not the memory node(s) have a unit address.
>>
>>> Now, I'd like to understand why not use the simpler binding originally
>>> proposed by Jeremy, which had the advantage of proposing a unique name
>>> per region in the traditional form "vendor,name", which then allows
>>> drivers to pick up the region directly if they wish to query, remove or
>>> update it in the tree for example (because they changed the framebuffer
>>> address for example and want kexec to continue working).
>>
>> Hmmm... I don't follow. How is query/remove/update any more or less
>> difficult between the two approaches? Updating the reg property should
>> be doable in-place with both methods, but finding a specific region
>> associated with a device is explicit in the nodes-and-phandles approach.
>> (unless the 'name' part of vendor,name is an instance name and the
>> device node has a property containing the name; ie "acme,framebuffer1",
>> "acme,framebuffer2", and then in the device node something like:
>> framebuffer-region = "acme,framebuffer2";)
>>
>>> I don't object to having a node per region, though it seemed unnecessary
>>> at the time, but in any case, the current binding is crap and need to be
>>> fixed urgently before its use spreads.
>>
>> It seems to me that the 'top-level' objection is the creation of a new
>> binding using a node per region. I think it is warrented. If you
>> disagree strongly then we'll revert the whole series and try again for
>> v3.13. Otherwise I think the other objections are fixable in this cycle.
>
> No. I don't fundamentally object to using a node per region, it does
> have some advantages indeed as I have mentioned in a few places.
>
> My main issues are:
>
> - Documentation of the "memory" binding. This is a separate thing that
> shouldn't have been there and should not document the node without the
> unit address (it's ok to specify I suppose that it can be ommitted
> though I don't like it).
>
> - Location. I don't want that under /memory. I have to deal with
> machines with multiple /memory nodes and regions potentially straddling
> them.
>
> - The choice of properties for describing, naming and defining their
> usage. I think we can come to an agreement here.
>
> In fact the use of a node per region is the *least* of my worries :-)
So where have we gotten on this?
It seems we are in agreement that:
1. reserve memory should be probably be described in nodes
2. it should be pulled out of the memory node and put at root level
3. Use reg to describe the memory regions for a given node
Now to figure out about how to convey usage information for the region and possibly driver association. I agree with Ben that there are probably cases that an associated device node may not exist so that shouldn't be a hard requirement.
- k
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> So where have we gotten on this?
>
> It seems we are in agreement that:
> 1. reserve memory should be probably be described in nodes
> 2. it should be pulled out of the memory node and put at root level
> 3. Use reg to describe the memory regions for a given node
>
> Now to figure out about how to convey usage information for the region and possibly driver association. I agree with Ben that there are probably cases that an associated device node may not exist so that shouldn't be a hard requirement.
>
> - k
So I think we should revert the patches as we don't seem to be getting anywhere both on discussion or code updates and the v3.12-rc's keep moving along.
- k
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation