Re: "memory" binding issues

[Kumar Gala]

> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Kumar Gala]

 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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Kumar Gala]

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 

Re: memory binding issues

[Kumar Gala]

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
 

Re: "memory" binding issues

[David Gibson]

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  
> > 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


pgp02mWdxLXTB.pgp
Description: PGP signature

Re: "memory" binding issues

[Stephen Warren]

On 09/17/2013 03:15 PM, Olof Johansson wrote:
> On Tue, Sep 17, 2013 at 2:08 PM, 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.
> 
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Benjamin Herrenschmidt]

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 

Re: "memory" binding issues

[Benjamin Herrenschmidt]

On Tue, 2013-09-17 at 18:38 -0700, Grant Likely wrote:
> On Tue, 17 Sep 2013 08:46:07 +1000, Benjamin Herrenschmidt 
>  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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Grant Likely]

On Tue, 17 Sep 2013 09:56:39 +0200, Tomasz Figa  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Grant Likely]

On Mon, 16 Sep 2013 12:57:54 +1000, 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.

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 

Re: "memory" binding issues

[Grant Likely]

On Tue, 17 Sep 2013 08:46:07 +1000, Benjamin Herrenschmidt 
 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Grant Likely]

On Tue, 17 Sep 2013 08:46:07 +1000, Benjamin Herrenschmidt 
b...@kernel.crashing.org 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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Grant Likely]

On Mon, 16 Sep 2013 12:57:54 +1000, Benjamin Herrenschmidt 
b...@kernel.crashing.org 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 

Re: memory binding issues

[Grant Likely]

On Tue, 17 Sep 2013 09:56:39 +0200, Tomasz Figa tomasz.f...@gmail.com 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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Benjamin Herrenschmidt]

On Tue, 2013-09-17 at 18:38 -0700, Grant Likely wrote:
 On Tue, 17 Sep 2013 08:46:07 +1000, Benjamin Herrenschmidt 
 b...@kernel.crashing.org 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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Benjamin Herrenschmidt]

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 

Re: memory binding issues

[Stephen Warren]

On 09/17/2013 03:15 PM, Olof Johansson wrote:
 On Tue, Sep 17, 2013 at 2:08 PM, Frank Rowand frowand.l...@gmail.com 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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[David Gibson]

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 frowand.l...@gmail.com 
  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


pgp02mWdxLXTB.pgp
Description: PGP signature

Re: "memory" binding issues

[David Gibson]

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


pgpPcxUUztukr.pgp
Description: PGP signature

Re: "memory" binding issues

[Olof Johansson]

On Tue, Sep 17, 2013 at 4:04 PM, Benjamin Herrenschmidt
 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Benjamin Herrenschmidt]

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Olof Johansson]

On Tue, Sep 17, 2013 at 2:19 PM, Tomasz Figa  wrote:
> On Tuesday 17 of September 2013 14:15:52 Olof Johansson wrote:
>> On Tue, Sep 17, 2013 at 2:08 PM, 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.
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Benjamin Herrenschmidt]

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.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Tomasz Figa]

On Tuesday 17 of September 2013 14:15:52 Olof Johansson wrote:
> On Tue, Sep 17, 2013 at 2:08 PM, 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.
> 
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Olof Johansson]

On Tue, Sep 17, 2013 at 2:08 PM, 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Frank Rowand]

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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Olof Johansson]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Kumar Gala]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Tomasz Figa]

On Monday 16 of September 2013 15:48:22 Olof Johansson wrote:
> On Mon, Sep 16, 2013 at 3:46 PM, Benjamin Herrenschmidt
> 
>  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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Tomasz Figa]

On Monday 16 of September 2013 15:48:22 Olof Johansson wrote:
 On Mon, Sep 16, 2013 at 3:46 PM, Benjamin Herrenschmidt
 
 b...@kernel.crashing.org 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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Kumar Gala]

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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Olof Johansson]

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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Frank Rowand]

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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Olof Johansson]

On Tue, Sep 17, 2013 at 2:08 PM, Frank Rowand frowand.l...@gmail.com 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Tomasz Figa]

On Tuesday 17 of September 2013 14:15:52 Olof Johansson wrote:
 On Tue, Sep 17, 2013 at 2:08 PM, Frank Rowand frowand.l...@gmail.com 
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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Benjamin Herrenschmidt]

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.



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Olof Johansson]

On Tue, Sep 17, 2013 at 2:19 PM, Tomasz Figa tomasz.f...@gmail.com wrote:
 On Tuesday 17 of September 2013 14:15:52 Olof Johansson wrote:
 On Tue, Sep 17, 2013 at 2:08 PM, Frank Rowand frowand.l...@gmail.com
 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Benjamin Herrenschmidt]

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.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Olof Johansson]

On Tue, Sep 17, 2013 at 4:04 PM, Benjamin Herrenschmidt
b...@kernel.crashing.org 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[David Gibson]

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


pgpPcxUUztukr.pgp
Description: PGP signature

Re: "memory" binding issues

[Benjamin Herrenschmidt]

On Mon, 2013-09-16 at 16:48 -0700, Olof Johansson wrote:
> On Mon, Sep 16, 2013 at 4:47 PM, Benjamin Herrenschmidt
>  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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Olof Johansson]

On Mon, Sep 16, 2013 at 4:47 PM, Benjamin Herrenschmidt
 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Benjamin Herrenschmidt]

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Olof Johansson]

On Mon, Sep 16, 2013 at 3:46 PM, Benjamin Herrenschmidt
 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Benjamin Herrenschmidt]

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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Benjamin Herrenschmidt]

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Stephen Warren]

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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: "memory" binding issues

[Kumar Gala]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Kumar Gala]

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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Stephen Warren]

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?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Benjamin Herrenschmidt]

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.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Olof Johansson]

On Mon, Sep 16, 2013 at 3:46 PM, Benjamin Herrenschmidt
b...@kernel.crashing.org 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Benjamin Herrenschmidt]

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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Benjamin Herrenschmidt]

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.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Olof Johansson]

On Mon, Sep 16, 2013 at 4:47 PM, Benjamin Herrenschmidt
b...@kernel.crashing.org 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: memory binding issues

[Benjamin Herrenschmidt]

On Mon, 2013-09-16 at 16:48 -0700, Olof Johansson wrote:
 On Mon, Sep 16, 2013 at 4:47 PM, Benjamin Herrenschmidt
 b...@kernel.crashing.org 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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/