Could the DTS experts look at this?

Could the DTS experts look at this?

[Sean MacLennan]

The Rev B warp is moving to a 4M NOR / 256M NAND flash setup from the 
current 64M NOR / 64M NAND. I would like to keep support for the 64M NOR 
so I modified the boot code to be a bit more dynamic. Here is the new 
NOR parition layout in the DTS:

nor_flash@0,0 {
	compatible = "amd,s29gl512n", "cfi-flash";
	bank-width = <2>;
	reg = <0 0 4000000>;
	#address-cells = <1>;
	#size-cells = <1>;
	partition@300000 {
		label = "fpga";
		reg = <300000 40000>;
	};
	partition@340000 {
		label = "env";
		reg = <340000 40000>;
	};
	partition@380000 {
		label = "u-boot";
		reg = <380000 80000>;
	};
};

Yes, the top of the NOR will be empty. Here is the code from 
cuboot-warp.c to handle fixups for the 64M flash:

static void warp_fixup_one_nor(u32 from, u32 to)
{
	void *devp;
	char name[40];
	u32 v[2];

	sprintf(name, "/plb/opb/ebc/nor_flash@0,0/partition@%x", from);

	devp = finddevice(name);
	if (!devp) return;

	if (getprop(devp, "reg", v, sizeof(v)) == sizeof(v)) {
		v[0] = to;
		setprop(devp, "reg", v, sizeof(v));

		printf("NOR 64M fixup %x -> %x\n", from, to);
	}
}


static void warp_fixups(void)
{
	unsigned long sysclk = 66000000;

	ibm440ep_fixup_clocks(sysclk, 11059200, 50000000);
	ibm4xx_sdram_fixup_memsize();
	ibm4xx_fixup_ebc_ranges("/plb/opb/ebc");
	dt_fixup_mac_addresses(&bd.bi_enetaddr);

	/* Fixup for 64M flash on Rev A boards. */
	if(bd.bi_flashsize == 0x4000000) {
		warp_fixup_one_nor(0x300000, 0x3f00000);
		warp_fixup_one_nor(0x340000, 0x3f40000);
		warp_fixup_one_nor(0x380000, 0x3f80000);
	}
}

I have tested this with the 64M NOR, and it seems to work. However, are 
there going to be problems with the partition name not matching the reg 
address entry?

If anybody has suggestions on better ways to do this, fire away.

And looking at this code, and other board ports, why is sysclk a local 
variable and all the other numbers hardcoded in the args? I left it the 
same way as the others but it does look a bit strange.

Cheers,
   Sean

Re: Could the DTS experts look at this?

[Arnd Bergmann]

On Saturday 09 February 2008, Sean MacLennan wrote:
> If anybody has suggestions on better ways to do this, fire away.

I guess the cleanest solution would be to include two complete device trees
for this platform, and then choose the correct one in cuboot-warp.c based
on the board revision.

The obvious disadvantage of this is that you'd get two device trees
that you need to keep in sync with every change, so it might not
be very practical.

Maybe we can introduce a more generic way of having conditional
device nodes in the tree that get knocked out in the boot wrapper.

	Arnd <><

Re: Could the DTS experts look at this?

[Sean MacLennan]

Arnd Bergmann wrote:
> I guess the cleanest solution would be to include two complete device trees
> for this platform, and then choose the correct one in cuboot-warp.c based
> on the board revision.
>
> The obvious disadvantage of this is that you'd get two device trees
> that you need to keep in sync with every change, so it might not
> be very practical.
>
>   
Keeping two device trees would be awkward. My final solution will 
probably be to just have
the 4M flash partition layout. Since we really only use the partition 
information to write to the partitions from a user mode program, I can 
live with having to flash the images from u-boot.

But if we where planning on moving forward with two configurations, 
which we where planning to do, this would not be an option. We would 
need to auto configure *and* possibly add partitions based on size.

> Maybe we can introduce a more generic way of having conditional
> device nodes in the tree that get knocked out in the boot wrapper.
>   
Of even a way to have ifdefs in the dts. In this case, we have to build 
a specific version of u-boot for each nor size (WARNING: This may not be 
*technically* true, but this is how we are handling it). Having to set a 
config and build a specific kernel, while not ideal, would also be a 
reasonable solution.

Just throwing out ideas. In the short term this is not a problem, but in 
the long term?

Cheers,
   Sean

Re: Could the DTS experts look at this?

[Timur Tabi]

Arnd Bergmann wrote:

> Maybe we can introduce a more generic way of having conditional
> device nodes in the tree that get knocked out in the boot wrapper.

I've been thinking about doing just this for quite some time now.  I've had a 
few informal discussions without people about.

One idea is to allow attaching simple conditional expressions (like X is <, =, 
or > than Y) to a node.  It is the responsibility of the code that parses the 
device tree to assign values to X and Y.  For instance, they could be the names 
of U-Boot environment variables.  If the expression is false, then the node is 
removed (or ignored) from the device tree.  If it's true, then it's kept in.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Could the DTS experts look at this?

[David Gibson]

On Mon, Feb 11, 2008 at 11:57:10AM -0600, Timur Tabi wrote:
> Arnd Bergmann wrote:
> 
> > Maybe we can introduce a more generic way of having conditional
> > device nodes in the tree that get knocked out in the boot wrapper.
> 
> I've been thinking about doing just this for quite some time now.  I've had a 
> few informal discussions without people about.
> 
> One idea is to allow attaching simple conditional expressions (like
> X is <, =, or > than Y) to a node.  It is the responsibility of the
> code that parses the device tree to assign values to X and Y.  For
> instance, they could be the names of U-Boot environment variables.
> If the expression is false, then the node is removed (or ignored)
> from the device tree.  If it's true, then it's kept in.

In the binary tree representation itself?  No way.

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

Re: Could the DTS experts look at this?

[David Gibson]

On Tue, Feb 12, 2008 at 10:54:09AM +1100, David Gibson wrote:
> On Mon, Feb 11, 2008 at 11:57:10AM -0600, Timur Tabi wrote:
> > Arnd Bergmann wrote:
> > 
> > > Maybe we can introduce a more generic way of having conditional
> > > device nodes in the tree that get knocked out in the boot wrapper.
> > 
> > I've been thinking about doing just this for quite some time now.  I've had a 
> > few informal discussions without people about.
> > 
> > One idea is to allow attaching simple conditional expressions (like
> > X is <, =, or > than Y) to a node.  It is the responsibility of the
> > code that parses the device tree to assign values to X and Y.  For
> > instance, they could be the names of U-Boot environment variables.
> > If the expression is false, then the node is removed (or ignored)
> > from the device tree.  If it's true, then it's kept in.
> 
> In the binary tree representation itself?  No way.

Or to expand.  It's relatively easy now to just include multiple nodes
in the tree and either delete or nop some of them out conditionally
using libfdt.  But the conditional logic should be in the manipulating
agent (u-boot or bootwrapper or whatever), there's no way we're going
to require a conditional expression parser to interpret the device
tree blob itself.

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

Re: Could the DTS experts look at this?

[Arnd Bergmann]

On Tuesday 12 February 2008, David Gibson wrote:
> Or to expand. =A0It's relatively easy now to just include multiple nodes
> in the tree and either delete or nop some of them out conditionally
> using libfdt. =A0But the conditional logic should be in the manipulating
> agent (u-boot or bootwrapper or whatever), there's no way we're going
> to require a conditional expression parser to interpret the device
> tree blob itself.

How about making the logic to nop out nodes a little more generic
without changes to the binary format?
E.g. you could have a "linux,conditional-node" property in the device
tree whose value is compared to a HW configuration specific string.
In Sean's example, you can have linux,conditional-node=3D"Rev.A" in
some nodes and linux,conditional-node=3D"Rev.B" in others, then
knock out all devices that have a non-matching linux,conditional-node
property, and finally remove the properties themselves before starting
the kernel.

	Arnd <><

Re: Could the DTS experts look at this?

[David Gibson]

On Tue, Feb 12, 2008 at 01:21:44AM +0100, Arnd Bergmann wrote:
> On Tuesday 12 February 2008, David Gibson wrote:
> > Or to expand.  It's relatively easy now to just include multiple nodes
> > in the tree and either delete or nop some of them out conditionally
> > using libfdt.  But the conditional logic should be in the manipulating
> > agent (u-boot or bootwrapper or whatever), there's no way we're going
> > to require a conditional expression parser to interpret the device
> > tree blob itself.
> 
> How about making the logic to nop out nodes a little more generic
> without changes to the binary format?
> E.g. you could have a "linux,conditional-node" property in the device
> tree whose value is compared to a HW configuration specific string.
> In Sean's example, you can have linux,conditional-node="Rev.A" in
> some nodes and linux,conditional-node="Rev.B" in others, then
> knock out all devices that have a non-matching linux,conditional-node
> property, and finally remove the properties themselves before starting
> the kernel.

Well, that's basically a u-boot issue.  If they want to do their input
trees that way, and have helper functions that deal with it...

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

Re: Could the DTS experts look at this?

[Scott Wood]

On Tue, Feb 12, 2008 at 11:36:33AM +1100, David Gibson wrote:
> On Tue, Feb 12, 2008 at 01:21:44AM +0100, Arnd Bergmann wrote:
> > On Tuesday 12 February 2008, David Gibson wrote:
> > > Or to expand.  It's relatively easy now to just include multiple nodes
> > > in the tree and either delete or nop some of them out conditionally
> > > using libfdt.  But the conditional logic should be in the manipulating
> > > agent (u-boot or bootwrapper or whatever), there's no way we're going
> > > to require a conditional expression parser to interpret the device
> > > tree blob itself.
> > 
> > How about making the logic to nop out nodes a little more generic
> > without changes to the binary format?
> > E.g. you could have a "linux,conditional-node" property in the device
> > tree whose value is compared to a HW configuration specific string.
> > In Sean's example, you can have linux,conditional-node="Rev.A" in
> > some nodes and linux,conditional-node="Rev.B" in others, then
> > knock out all devices that have a non-matching linux,conditional-node
> > property, and finally remove the properties themselves before starting
> > the kernel.
> 
> Well, that's basically a u-boot issue.  If they want to do their input
> trees that way, and have helper functions that deal with it...

The actual mechanism that we originially discussed, which Timur later
morphed into conditions-on-nodes, was to have a separate hwoptions node,
under which would be described various hwoptions (jumpers and such) whose
state could be either detected by u-boot or set by environment variable. 
Each hwoption setting would contain a device tree fragment to be merged into
the main device tree.

-Scott

Re: Could the DTS experts look at this?

[David Gibson]

On Tue, Feb 12, 2008 at 12:51:06PM -0600, Scott Wood wrote:
> On Tue, Feb 12, 2008 at 11:36:33AM +1100, David Gibson wrote:
> > On Tue, Feb 12, 2008 at 01:21:44AM +0100, Arnd Bergmann wrote:
> > > On Tuesday 12 February 2008, David Gibson wrote:
> > > > Or to expand.  It's relatively easy now to just include multiple nodes
> > > > in the tree and either delete or nop some of them out conditionally
> > > > using libfdt.  But the conditional logic should be in the manipulating
> > > > agent (u-boot or bootwrapper or whatever), there's no way we're going
> > > > to require a conditional expression parser to interpret the device
> > > > tree blob itself.
> > > 
> > > How about making the logic to nop out nodes a little more generic
> > > without changes to the binary format?
> > > E.g. you could have a "linux,conditional-node" property in the device
> > > tree whose value is compared to a HW configuration specific string.
> > > In Sean's example, you can have linux,conditional-node="Rev.A" in
> > > some nodes and linux,conditional-node="Rev.B" in others, then
> > > knock out all devices that have a non-matching linux,conditional-node
> > > property, and finally remove the properties themselves before starting
> > > the kernel.
> > 
> > Well, that's basically a u-boot issue.  If they want to do their input
> > trees that way, and have helper functions that deal with it...
> 
> The actual mechanism that we originially discussed, which Timur later
> morphed into conditions-on-nodes, was to have a separate hwoptions node,
> under which would be described various hwoptions (jumpers and such) whose
> state could be either detected by u-boot or set by environment variable. 
> Each hwoption setting would contain a device tree fragment to be merged into
> the main device tree.

I'm not sure I'm entirely happy about storing the fragments under a
special node - but certainly u-boot could do that if it wants.  What
would certainly be ok is to store various fragments as separate blobs
and fold them together as necessary.  Which reminds me, I meant to
implement a "graft" function in libfdt.

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

Re: Could the DTS experts look at this?

[Timur Tabi]

David Gibson wrote:

> I'm not sure I'm entirely happy about storing the fragments under a
> special node - but certainly u-boot could do that if it wants.  What
> would certainly be ok is to store various fragments as separate blobs
> and fold them together as necessary.  Which reminds me, I meant to
> implement a "graft" function in libfdt.

Most likely, U-Boot would strip out the special node after processing it.  The 
idea is for the boot loader to customize the device tree based before sending it 
to the kernel.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Could the DTS experts look at this?

[David Gibson]

On Tue, Feb 12, 2008 at 05:41:12PM -0600, Timur Tabi wrote:
> David Gibson wrote:
> 
> > I'm not sure I'm entirely happy about storing the fragments under a
> > special node - but certainly u-boot could do that if it wants.  What
> > would certainly be ok is to store various fragments as separate blobs
> > and fold them together as necessary.  Which reminds me, I meant to
> > implement a "graft" function in libfdt.
> 
> Most likely, U-Boot would strip out the special node after
> processing it.  The idea is for the boot loader to customize the
> device tree based before sending it to the kernel.

Of course.  U-boot can use whatever representation it likes
internally, as long as it's all fixed up by the time it reaches the
kernel.  I just think using sepa`rate "device tree fragment" blobs
might be a better way of doing it.

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

Re: Could the DTS experts look at this?

[Timur Tabi]

Arnd Bergmann wrote:

> On Tuesday 12 February 2008, David Gibson wrote:
>> Or to expand.  It's relatively easy now to just include multiple nodes
>> in the tree and either delete or nop some of them out conditionally
>> using libfdt.  

Yes, but what better place to store the conditions than in the device tree 
itself?  How would libfdt know where the conditions are?  Do you want to have 
two binary blobs?

>> But the conditional logic should be in the manipulating
>> agent (u-boot or bootwrapper or whatever), there's no way we're going
>> to require a conditional expression parser to interpret the device
>> tree blob itself.

I think it's a great feature that solves a lot of problems, and it does so in an 
elegant and efficient manner.  I look forward to trying to change your mind when 
I get around to implementing it.

> How about making the logic to nop out nodes a little more generic
> without changes to the binary format?
> E.g. you could have a "linux,conditional-node" property in the device
> tree whose value is compared to a HW configuration specific string.

The problem with this is that if you use a version of libfdt that does not 
understand "linux,conditional-node", then your device tree will be wrong, 
because it could contain nodes that don't belong.  We would need a new, 
incompatible version number for the device tree to make sure that this doesn't 
happen, even though nothing has changed in the binary layout of the tree.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Could the DTS experts look at this?

[Grant Likely]

On Feb 12, 2008 8:44 AM, Timur Tabi <timur@freescale.com> wrote:
> Arnd Bergmann wrote:
>
> > On Tuesday 12 February 2008, David Gibson wrote:
> >> Or to expand.  It's relatively easy now to just include multiple nodes
> >> in the tree and either delete or nop some of them out conditionally
> >> using libfdt.
>
> Yes, but what better place to store the conditions than in the device tree
> itself?  How would libfdt know where the conditions are?  Do you want to have
> two binary blobs?

The transient state of the dts before it is handed to the kernel
proper is almost irrelevant.  It is totally reasonable to add in
whatever properties/nodes that are needed to *eventually* describe the
hardware correctly.  Heck, we already do this will all dts files that
go through u-boot is a simple sense.  We put placeholder properties
for mac addresses and bus frequencies, but u-boot fills them in.

However, if a designer does write a device tree containing more nodes
than is needed, then it is also the responsibility of that designer to
make sure the boot loader can use that tree to generate a real
description of hardware.  This requires coordination and
documentation, but id does not requires special formatting or
versioning of the device tree blob.

The dtb is a data structure, not a programming language.  I think it
is a slippery slope to try and encode conditionals into the structure;
but it is entirely reasonable to encode *data* into the dt that helps
make those conditional decisions.

> >> But the conditional logic should be in the manipulating
> >> agent (u-boot or bootwrapper or whatever), there's no way we're going
> >> to require a conditional expression parser to interpret the device
> >> tree blob itself.
>
> I think it's a great feature that solves a lot of problems, and it does so in an
> elegant and efficient manner.  I look forward to trying to change your mind when
> I get around to implementing it.

I agree with David here; logic belongs in the agent code, not the data
structure.

> > How about making the logic to nop out nodes a little more generic
> > without changes to the binary format?
> > E.g. you could have a "linux,conditional-node" property in the device
> > tree whose value is compared to a HW configuration specific string.
>
> The problem with this is that if you use a version of libfdt that does not
> understand "linux,conditional-node", then your device tree will be wrong,
> because it could contain nodes that don't belong.  We would need a new,
> incompatible version number for the device tree to make sure that this doesn't
> happen, even though nothing has changed in the binary layout of the tree.

We've already got that issue.  If you pass the device tree for the
wrong board it will still validate correctly, but the board is not
going to boot.  The device tree must match what the bootloader
expects.  Changing the version number will do nothing to help this.
The version number ensures that the tree is parsable.  It does not
ensure that it is correct.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Could the DTS experts look at this?

[Timur Tabi]

Grant Likely wrote:
> On Feb 12, 2008 8:44 AM, Timur Tabi <timur@freescale.com> wrote:
>> Arnd Bergmann wrote:
>>
>>> On Tuesday 12 February 2008, David Gibson wrote:
>>>> Or to expand.  It's relatively easy now to just include multiple nodes
>>>> in the tree and either delete or nop some of them out conditionally
>>>> using libfdt.
>> Yes, but what better place to store the conditions than in the device tree
>> itself?  How would libfdt know where the conditions are?  Do you want to have
>> two binary blobs?
> 
> The transient state of the dts before it is handed to the kernel
> proper is almost irrelevant.  It is totally reasonable to add in
> whatever properties/nodes that are needed to *eventually* describe the
> hardware correctly.  Heck, we already do this will all dts files that
> go through u-boot is a simple sense.  We put placeholder properties
> for mac addresses and bus frequencies, but u-boot fills them in.

I agree with that.

> However, if a designer does write a device tree containing more nodes
> than is needed, then it is also the responsibility of that designer to
> make sure the boot loader can use that tree to generate a real
> description of hardware.  

No problem here.

> This requires coordination and
> documentation, but id does not requires special formatting or
> versioning of the device tree blob.

Unless the mechanism by which the designer ensures that the boot loader presents 
a proper device tree to the kernel requires special versioning.

> The dtb is a data structure, not a programming language.

But we have a problem with the current device tree definition that makes it 
difficult to use in real-world situations.  The current "solution" is to have 
multiple DTBs, each one covering a different variant of the hardware.  My 
proposal is to expand the definition of the DTB to allow the boot loader to 
modify it in a standard manner.  I believe that such a change would be both 
useful and not problematic.

 >  I think it
> is a slippery slope to try and encode conditionals into the structure;

Perhaps, which is why I said it should be simple conditionals - two values and a 
comparison.

> but it is entirely reasonable to encode *data* into the dt that helps
> make those conditional decisions.

That's okay too, except that if we just add additional nodes that describe 
conditions, then we need to make sure that whatever parses that DTB must also 
parse those additional nodes.  The only way to do that is create a new version 
number, like 18, which is marked as being incompatible with the current version 
(17).  Otherwise, a person could pass that DTB to an old version of U-Boot, and 
U-boot will just pass it on to the kernel as-is.

> We've already got that issue.  If you pass the device tree for the
> wrong board it will still validate correctly, but the board is not
> going to boot.

There's nothing stopping U-Boot today from scanning the device tree and making 
sure the SOC's compatible node is correct.  That's not currently done, but it 
could be.

 >  The device tree must match what the bootloader
> expects.  Changing the version number will do nothing to help this.
> The version number ensures that the tree is parsable.  It does not
> ensure that it is correct.

I think you're missing the point.  If we add conditional expressions to the 
device tree (whether attached to a node or as part of a separate node or 
whatever), we must also add a mechanism to ensure that these conditions are 
parsed by the boot loader.  As far as I know, the only mechanism that can do 
this is the version identifier.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Could the DTS experts look at this?

[Grant Likely]

On Feb 12, 2008 12:08 PM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
> > On Feb 12, 2008 8:44 AM, Timur Tabi <timur@freescale.com> wrote:
> > This requires coordination and
> > documentation, but id does not requires special formatting or
> > versioning of the device tree blob.
>
> Unless the mechanism by which the designer ensures that the boot loader presents
> a proper device tree to the kernel requires special versioning.

Then use a local version in the data; don't overload the purpose of
the dtb version.

> > The dtb is a data structure, not a programming language.
>
> But we have a problem with the current device tree definition that makes it
> difficult to use in real-world situations.  The current "solution" is to have
> multiple DTBs, each one covering a different variant of the hardware.  My
> proposal is to expand the definition of the DTB to allow the boot loader to
> modify it in a standard manner.  I believe that such a change would be both
> useful and not problematic.

I don't think it is yet possible to define a reasonable 'standard
manner' for massaging device trees.  There are going to be a lot of
experiments and false starts before we come to consensus on common
manipulations which everyone will want to have access too.  Therefore
for the time being dtb fixups and manipulations will remain a board
specific thing.

Plus, even when we do have a common set of manipulations, they can be
implemented with property values.  It is not something that needs to
change the dtb version.

>  >  I think it
> > is a slippery slope to try and encode conditionals into the structure;
>
> Perhaps, which is why I said it should be simple conditionals - two values and a
> comparison.

And when something comes along that doesn't fit into that model?  Add
more constructs?  I say better to handle that within the existing data
format.  If we get it wrong, then we just change the affected device
trees and boot loaders.  We don't have to upgrade every platform that
uses dt blobs.

> > but it is entirely reasonable to encode *data* into the dt that helps
> > make those conditional decisions.
>
> That's okay too, except that if we just add additional nodes that describe
> conditions, then we need to make sure that whatever parses that DTB must also
> parse those additional nodes.  The only way to do that is create a new version
> number, like 18, which is marked as being incompatible with the current version
> (17).  Otherwise, a person could pass that DTB to an old version of U-Boot, and
> U-boot will just pass it on to the kernel as-is.

That's not a dtb version issue.  That is a dtb content issue.  It does
not warrant changing the dtb version number.

> > We've already got that issue.  If you pass the device tree for the
> > wrong board it will still validate correctly, but the board is not
> > going to boot.
>
> There's nothing stopping U-Boot today from scanning the device tree and making
> sure the SOC's compatible node is correct.  That's not currently done, but it
> could be.

Fair enough, and it is also reasonable for the boot loader to look for
a specific property name to decide how to massage the data structure.
This alone does not require a dtb version change.

>  >  The device tree must match what the bootloader
> > expects.  Changing the version number will do nothing to help this.
> > The version number ensures that the tree is parsable.  It does not
> > ensure that it is correct.
>
> I think you're missing the point.  If we add conditional expressions to the
> device tree (whether attached to a node or as part of a separate node or
> whatever), we must also add a mechanism to ensure that these conditions are
> parsed by the boot loader.  As far as I know, the only mechanism that can do
> this is the version identifier.

I'm not missing the point because I disagree entirely with the
addition of conditional expressions to the device tree.  Instead, I
think properties can be added to the device tree that a bootloader can
look for and decide to apply conditions against them which means the
conditions are encoded in the boot loader, not the device tree.  (the
device tree simply contains data which supports the boot loaders
decision; a rather different thing).

Finally, it is *always* required that the user (or installer) know
enough to pass the correct device tree to the correct board.  No
amount of versioning at the dtb level is going to change this
situation.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Could the DTS experts look at this?

[Timur Tabi]

Grant Likely wrote:

> Then use a local version in the data; don't overload the purpose of
> the dtb version.

I don't know what you mean by that.

> I don't think it is yet possible to define a reasonable 'standard
> manner' for massaging device trees.  There are going to be a lot of
> experiments and false starts before we come to consensus on common
> manipulations which everyone will want to have access too.  Therefore
> for the time being dtb fixups and manipulations will remain a board
> specific thing.

That's okay with me.  I'm just proposing one method that I like, and Scott 
proposed another.

> And when something comes along that doesn't fit into that model?  Add
> more constructs?

If necessary, yes.  What's wrong with expanding the power of the device tree 
format when it solves more problems?

>  I say better to handle that within the existing data
> format.

And the point I've been trying to make is that we have real problems today that 
cannot be solved elegantly with the current device tree problem.  Having 
board-specific code in U-Boot that is hard-coded to look for specific nodes in 
the device tree, and making hard-coded edits on that tree, is *not* elegant.

>  If we get it wrong, then we just change the affected device
> trees and boot loaders.  We don't have to upgrade every platform that
> uses dt blobs.

Only the platforms that need to take advantage of conditional nodes need to be 
upgraded in the first place.  Most platforms are happy with just one device tree.

>> That's okay too, except that if we just add additional nodes that describe
>> conditions, then we need to make sure that whatever parses that DTB must also
>> parse those additional nodes.  The only way to do that is create a new version
>> number, like 18, which is marked as being incompatible with the current version
>> (17).  Otherwise, a person could pass that DTB to an old version of U-Boot, and
>> U-boot will just pass it on to the kernel as-is.
> 
> That's not a dtb version issue.  That is a dtb content issue. 

Technically, that's true, but ...

> It does
> not warrant changing the dtb version number.

Then how do you solve the problem of passing a device tree to a boot loader that 
does not know how to parse it properly?  A device tree with these additional 
nodes *must* be parsed by a boot loader that is aware of them.  Otherwise, it 
will pass the device tree as-is to the kernel without warning.  This is a bad 
thing, and steps should be taken to prevent that.  If you can think of a way to 
make this happen without changing the version number, I'd love to hear.  All I'm 
hearing from you now is denial that this is a problem.

>>> We've already got that issue.  If you pass the device tree for the
>>> wrong board it will still validate correctly, but the board is not
>>> going to boot.
>> There's nothing stopping U-Boot today from scanning the device tree and making
>> sure the SOC's compatible node is correct.  That's not currently done, but it
>> could be.
> 
> Fair enough, and it is also reasonable for the boot loader to look for
> a specific property name to decide how to massage the data structure.
> This alone does not require a dtb version change.

Current versions of U-Boot do not know how to do this.  So again, I'm asking 
you: how do you solve the problem of passing a device tree with additional nodes 
to a boot loader that does not know how to parse them properly?  How do you 
prevent that old U-Boot from ignoring those nodes?

> I'm not missing the point because I disagree entirely with the
> addition of conditional expressions to the device tree.  Instead, I
> think properties can be added to the device tree that a bootloader can
> look for and decide to apply conditions against them which means the
> conditions are encoded in the boot loader, not the device tree.  (the
> device tree simply contains data which supports the boot loaders
> decision; a rather different thing).

Then why bother passing a DTB to the boot loader at all?  Why not just have the 
boot loader create the device tree from scratch?


-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Could the DTS experts look at this?

[Grant Likely]

On Feb 12, 2008 12:45 PM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>
> > Then use a local version in the data; don't overload the purpose of
> > the dtb version.
>
> I don't know what you mean by that.

I'm saying that the dtb version is to describe the binary format of
the dtb; not the content.  Using it to say "this dtb contains data
that needs to be massaged in a particular way" is using (overloading)
the dtb version for a different and orthogonal purpose.

> > And when something comes along that doesn't fit into that model?  Add
> > more constructs?
>
> If necessary, yes.  What's wrong with expanding the power of the device tree
> format when it solves more problems?

Nothing at all; but the flipside of that is it should only be done
when it is the appropriate place to do so.

> >  I say better to handle that within the existing data
> > format.
>
> And the point I've been trying to make is that we have real problems today that
> cannot be solved elegantly with the current device tree problem.  Having
> board-specific code in U-Boot that is hard-coded to look for specific nodes in
> the device tree, and making hard-coded edits on that tree, is *not* elegant.
>
> >  If we get it wrong, then we just change the affected device
> > trees and boot loaders.  We don't have to upgrade every platform that
> > uses dt blobs.
>
> Only the platforms that need to take advantage of conditional nodes need to be
> upgraded in the first place.  Most platforms are happy with just one device tree.
>
> >> That's okay too, except that if we just add additional nodes that describe
> >> conditions, then we need to make sure that whatever parses that DTB must also
> >> parse those additional nodes.  The only way to do that is create a new version
> >> number, like 18, which is marked as being incompatible with the current version
> >> (17).  Otherwise, a person could pass that DTB to an old version of U-Boot, and
> >> U-boot will just pass it on to the kernel as-is.
> >
> > That's not a dtb version issue.  That is a dtb content issue.
>
> Technically, that's true, but ...
>
> > It does
> > not warrant changing the dtb version number.
>
> Then how do you solve the problem of passing a device tree to a boot loader that
> does not know how to parse it properly?  A device tree with these additional
> nodes *must* be parsed by a boot loader that is aware of them.  Otherwise, it
> will pass the device tree as-is to the kernel without warning.  This is a bad
> thing, and steps should be taken to prevent that.  If you can think of a way to
> make this happen without changing the version number, I'd love to hear.  All I'm
> hearing from you now is denial that this is a problem.

I do not deny that it is a problem.  I assert that the proposed
solution does not at all solve the greater problem.  Changing the DTB
version solves the specific problem of a dtb that needs to be modified
being passed into to an older version of u-boot.  This is a very
narrow use case and it is not the greater problem.  The greater
problem is passing the wrong device tree either because of a
u-boot/dtb mismatch or the dtb is for an entirely different board.
Changing the dtb version is just a bandaid fix that makes things more
complex and only provides the illusion of a solution.

Your suggestion of checking for specific property values is a far more
reliable and workable solution.

> > Fair enough, and it is also reasonable for the boot loader to look for
> > a specific property name to decide how to massage the data structure.
> > This alone does not require a dtb version change.
>
> Current versions of U-Boot do not know how to do this.  So again, I'm asking
> you: how do you solve the problem of passing a device tree with additional nodes
> to a boot loader that does not know how to parse them properly?  How do you
> prevent that old U-Boot from ignoring those nodes?

We don't.  We've got no sane way to do so (except perhaps going ahead
and renaming the soc nodes from soc<chip>@addr to soc@addr.  That will
break almost all existing ports.  :-).  I do not agree that changing
the dtb version is a sane change.

For older u-boots, this is a configuration management problem.  Just
like for current platforms we need to make sure that the kernel is
compiled for the correct platform before trying to boot.  There are no
protections in u-boot to make sure the kernel image matches the arch
yet it all works just fine.

> > I'm not missing the point because I disagree entirely with the
> > addition of conditional expressions to the device tree.  Instead, I
> > think properties can be added to the device tree that a bootloader can
> > look for and decide to apply conditions against them which means the
> > conditions are encoded in the boot loader, not the device tree.  (the
> > device tree simply contains data which supports the boot loaders
> > decision; a rather different thing).
>
> Then why bother passing a DTB to the boot loader at all?  Why not just have the
> boot loader create the device tree from scratch?

There is absolutely no reason for the boot loader not to create the
device tree from scratch.  dtbs are just more convenient to get up and
running with.  OpenFirmware has been doing that from day one and it
works quite well.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Could the DTS experts look at this?

[David Gibson]

On Tue, Feb 12, 2008 at 01:45:39PM -0600, Timur Tabi wrote:
> Grant Likely wrote:
[snip]
> > That's not a dtb version issue.  That is a dtb content issue. 
> 
> Technically, that's true, but ...
> 
> > It does
> > not warrant changing the dtb version number.
> 
> Then how do you solve the problem of passing a device tree to a boot
> loader that does not know how to parse it properly?  A device tree
> with these additional nodes *must* be parsed by a boot loader that
> is aware of them.

Correct.  Just as you must give a dtb with the information to the
correct board to a bootloader or things won't work.  Changing this is
not within the reasonable scope of what dtbs will do.

>  Otherwise, it will pass the device tree as-is to
> the kernel without warning.  This is a bad thing, and steps should
> be taken to prevent that.  If you can think of a way to make this
> happen without changing the version number, I'd love to hear.  All
> I'm hearing from you now is denial that this is a problem.
> 
> >>> We've already got that issue.  If you pass the device tree for the
> >>> wrong board it will still validate correctly, but the board is not
> >>> going to boot.
> >> There's nothing stopping U-Boot today from scanning the device tree and making
> >> sure the SOC's compatible node is correct.  That's not currently done, but it
> >> could be.
> > 
> > Fair enough, and it is also reasonable for the boot loader to look for
> > a specific property name to decide how to massage the data structure.
> > This alone does not require a dtb version change.
> 
> Current versions of U-Boot do not know how to do this.  So again,
> I'm asking you: how do you solve the problem of passing a device
> tree with additional nodes to a boot loader that does not know how
> to parse them properly?  How do you prevent that old U-Boot from
> ignoring those nodes?

You don't.  If your agent takes a dtb, dtb layout and agent must
match.

> > I'm not missing the point because I disagree entirely with the
> > addition of conditional expressions to the device tree.  Instead, I
> > think properties can be added to the device tree that a bootloader can
> > look for and decide to apply conditions against them which means the
> > conditions are encoded in the boot loader, not the device tree.  (the
> > device tree simply contains data which supports the boot loaders
> > decision; a rather different thing).
> 
> Then why bother passing a DTB to the boot loader at all?  Why not
> just have the boot loader create the device tree from scratch?

That's a perfectly acceptable option - and it's what I'd expect if a
real OF decided to add support for flattened device trees (which might
happen with ePAPR).  libfdt's serial-write functions are designed for
exactly this use case.

In fact, in one way of looking at it that's always what happens: the
dtb format is defined for passing hardware information from the
bootloader to the kernel; nothing else.  Passing a dtb *into* the
bootloader is just a bootloader implementation convenience, because
the possible variations on an output tree are small, so it's useful to
have a skeleton tree built-in.  But in order for the bootloader to
process those variations correctly, the skeleton *must* be in the
right format.  dtb input to a bootloader must match the bootloaders
expectations.  This has always been true, and will continue to be
true.

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

Re: Could the DTS experts look at this?

[Timur Tabi]

David Gibson wrote:

> You don't.  If your agent takes a dtb, dtb layout and agent must
> match.

So what I would like to see is a way for the agent to validate the dtb.  U-Boot 
could currently validate the SOC's compatible field.  However, if we add a 
special node that contains rules for modifying the rest of the tree, the only 
possible way to block older, incompatible U-Boots from accepting the tree is to 
bump the version number.  Since that is not the right thing to do, the best 
approach is to define a new node type that has conditional expression attached 
to it.  Then we can bump the version.

> In fact, in one way of looking at it that's always what happens: the
> dtb format is defined for passing hardware information from the
> bootloader to the kernel; nothing else.  Passing a dtb *into* the
> bootloader is just a bootloader implementation convenience, because
> the possible variations on an output tree are small, so it's useful to
> have a skeleton tree built-in.  But in order for the bootloader to
> process those variations correctly, the skeleton *must* be in the
> right format.  dtb input to a bootloader must match the bootloaders
> expectations.  This has always been true, and will continue to be
> true.

The problem with this approach is that you're replacing data with code, and that 
always makes things more difficult.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Could the DTS experts look at this?

[Grant Likely]

On Feb 12, 2008 4:35 PM, David Gibson <david@gibson.dropbear.id.au> wrote:
> In fact, in one way of looking at it that's always what happens: the
> dtb format is defined for passing hardware information from the
> bootloader to the kernel; nothing else.  Passing a dtb *into* the
> bootloader is just a bootloader implementation convenience, because
> the possible variations on an output tree are small, so it's useful to
> have a skeleton tree built-in.  But in order for the bootloader to
> process those variations correctly, the skeleton *must* be in the
> right format.  dtb input to a bootloader must match the bootloaders
> expectations.  This has always been true, and will continue to be
> true.

Well said.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Could the DTS experts look at this?

[David Gibson]

[snip]
On Tue, Feb 12, 2008 at 01:08:24PM -0600, Timur Tabi wrote:
> Grant Likely wrote:
> > On Feb 12, 2008 8:44 AM, Timur Tabi <timur@freescale.com> wrote:
> >> Arnd Bergmann wrote:
>  >  I think it
> > is a slippery slope to try and encode conditionals into the structure;
> 
> Perhaps, which is why I said it should be simple conditionals - two
> values and a comparison.

I can pretty much guarantee you that someone will find that
insufficient and want to expand the conditional representation.  This
way madness lies.

> > but it is entirely reasonable to encode *data* into the dt that helps
> > make those conditional decisions.
> 
> That's okay too, except that if we just add additional nodes that
> describe conditions, then we need to make sure that whatever parses
> that DTB must also parse those additional nodes.  The only way to do
> that is create a new version number, like 18, which is marked as
> being incompatible with the current version (17).  Otherwise, a
> person could pass that DTB to an old version of U-Boot, and U-boot
> will just pass it on to the kernel as-is.

No.  As Grant says, that's not what the version number is for.  It
represents the version of the encoding, not the content.  If you must
version the content (which you should try really hard to avoid) the
correct way is to add versioning properties to the root node.

> > We've already got that issue.  If you pass the device tree for the
> > wrong board it will still validate correctly, but the board is not
> > going to boot.
> 
> There's nothing stopping U-Boot today from scanning the device tree
> and making sure the SOC's compatible node is correct.  That's not
> currently done, but it could be.
> 
>  >  The device tree must match what the bootloader
> > expects.  Changing the version number will do nothing to help this.
> > The version number ensures that the tree is parsable.  It does not
> > ensure that it is correct.
> 
> I think you're missing the point.  If we add conditional expressions
> to the device tree (whether attached to a node or as part of a
> separate node or whatever), we must also add a mechanism to ensure
> that these conditions are parsed by the boot loader.  As far as I
> know, the only mechanism that can do this is the version identifier.

No. a) the version identifier is not a mechanism for doing that and b)
the conditional mechanism is inherently agent-specific, therefore in
any case you *must* match an input incomplete tree (by which I mean
*anything* that requires processing before it's ready for consumption
by the kernel) to the specific bootloader or agent that will process
it and pass to the kernel.

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

Re: Could the DTS experts look at this?

[Timur Tabi]

David Gibson wrote:

> I can pretty much guarantee you that someone will find that
> insufficient and want to expand the conditional representation.  This
> way madness lies.

Then let them.  We can have version numbers associated with the conditional 
expressions.  If they want to make more complex condition expressions, they can 
bump the version number and define a spec and write code to parse it.

> No.  As Grant says, that's not what the version number is for.  It
> represents the version of the encoding, not the content.  If you must
> version the content (which you should try really hard to avoid) the
> correct way is to add versioning properties to the root node.

And that's why I prefer updating the DTB format to allow attaching conditional 
expressions to nodes.  This would then necessitate bumping the version number. 
Older U-Boots will reject this new DTB.  We can also modify DTC to support 
conditional nodes, so that if a customer has an older U-Boot he can't update, he 
can use DTC to generate a V17 DTB that has the conditionals already processed.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Could the DTS experts look at this?

[David Gibson]

On Tue, Feb 12, 2008 at 05:47:23PM -0600, Timur Tabi wrote:
> David Gibson wrote:
> 
> > I can pretty much guarantee you that someone will find that
> > insufficient and want to expand the conditional representation.  This
> > way madness lies.
> 
> Then let them.  We can have version numbers associated with the conditional 
> expressions.  If they want to make more complex condition expressions, they can 
> bump the version number and define a spec and write code to parse it.
> 
> > No.  As Grant says, that's not what the version number is for.  It
> > represents the version of the encoding, not the content.  If you must
> > version the content (which you should try really hard to avoid) the
> > correct way is to add versioning properties to the root node.
> 
> And that's why I prefer updating the DTB format to allow attaching
> conditional expressions to nodes.  This would then necessitate
> bumping the version number.  Older U-Boots will reject this new DTB.
> We can also modify DTC to support conditional nodes, so that if a
> customer has an older U-Boot he can't update, he can use DTC to
> generate a V17 DTB that has the conditionals already processed.

Well, yes, folding conditionals into the format would mean a version
bump.  But I am *not* going to put conditionals into a dtb version,
and I'm pretty sure BenH and Paulus would also reject this notion.
It's simply not what the format is for.  dtbs are for parsing by the
kernel, giving a complete device representation.  If bootloaders and
other things also find them a useful input format, that's great, but
I'm not going to significantly extend the semantics just to support
other uses.

Now, if you want to define a new binary-level "meta-dtb" format,
designed for representing various conditionals or whatnot, which can
be processed into a final true dtb, go for it.  But a) if you base it
on the dtb format, make sure you use a different magic number so as
not to interfere with the actual dtbs version progression and b) I
think you'll find it's more trouble than it's worth, at least at this
stage (feel free to try to prove me wrong on this point, of course).

At present, I think the "meta-dtb" format which makes the most sense,
based on a balance between simplicity and flexibility is C, with one
or more dtb fragments embedded.  This can be done now (though if there
are libfdt extensions which would make this usage easier, please
suggest them - fdt_graft() is an obvious one).

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

Re: Could the DTS experts look at this?

[Grant Likely]

On Feb 12, 2008 4:47 PM, Timur Tabi <timur@freescale.com> wrote:
> David Gibson wrote:
>
> > I can pretty much guarantee you that someone will find that
> > insufficient and want to expand the conditional representation.  This
> > way madness lies.
>
> Then let them.  We can have version numbers associated with the conditional
> expressions.  If they want to make more complex condition expressions, they can
> bump the version number and define a spec and write code to parse it.

You say that as if bumping the version number is cheap to do.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Could the DTS experts look at this?

[David Gibson]

On Tue, Feb 12, 2008 at 09:44:39AM -0600, Timur Tabi wrote:
> Arnd Bergmann wrote:
> 
> > On Tuesday 12 February 2008, David Gibson wrote:
> >> Or to expand.  It's relatively easy now to just include multiple nodes
> >> in the tree and either delete or nop some of them out conditionally
> >> using libfdt.  
> 
> Yes, but what better place to store the conditions than in the
> device tree itself?  How would libfdt know where the conditions are?
> Do you want to have two binary blobs?

libfdt wouldn't.  The conditional logic must be in the agent using
libfdt.

> >> But the conditional logic should be in the manipulating
> >> agent (u-boot or bootwrapper or whatever), there's no way we're going
> >> to require a conditional expression parser to interpret the device
> >> tree blob itself.
> 
> I think it's a great feature that solves a lot of problems, and it
> does so in an elegant and efficient manner.  I look forward to
> trying to change your mind when I get around to implementing it.
> 
> > How about making the logic to nop out nodes a little more generic
> > without changes to the binary format?
> > E.g. you could have a "linux,conditional-node" property in the device
> > tree whose value is compared to a HW configuration specific string.
> 
> The problem with this is that if you use a version of libfdt that
> does not understand "linux,conditional-node", then your device tree
> will be wrong, because it could contain nodes that don't belong.  We
> would need a new, incompatible version number for the device tree to
> make sure that this doesn't happen, even though nothing has changed
> in the binary layout of the tree.

Passing an incomplete device tree to an agent that doesn't expect it
is always going to cause trouble.  This is nothing new.  And as you've
said the interpretation of these variables in the conditionals is
already agent specific, so you'd still have to pass these
conditionalised trees to the correct agent in order for them to be
correctly interpreted.

No, this has to be agent-local logic.  If you want to annotate your
agent's input device trees with information that will help it do this,
go for it, but don't expect it to be in any way a standardized aspect
of the device tree format.

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

Re: Could the DTS experts look at this?

[David Gibson]

On Fri, Feb 08, 2008 at 06:30:56PM -0500, Sean MacLennan wrote:
> The Rev B warp is moving to a 4M NOR / 256M NAND flash setup from the 
> current 64M NOR / 64M NAND. I would like to keep support for the 64M NOR 
> so I modified the boot code to be a bit more dynamic. Here is the new 
> NOR parition layout in the DTS:
> 
> nor_flash@0,0 {
> 	compatible = "amd,s29gl512n", "cfi-flash";
> 	bank-width = <2>;
> 	reg = <0 0 4000000>;
> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 	partition@300000 {
> 		label = "fpga";
> 		reg = <300000 40000>;
> 	};
> 	partition@340000 {
> 		label = "env";
> 		reg = <340000 40000>;
> 	};
> 	partition@380000 {
> 		label = "u-boot";
> 		reg = <380000 80000>;
> 	};
> };
> 
> Yes, the top of the NOR will be empty. Here is the code from 
> cuboot-warp.c to handle fixups for the 64M flash:
> 
> static void warp_fixup_one_nor(u32 from, u32 to)
> {
> 	void *devp;
> 	char name[40];
> 	u32 v[2];
> 
> 	sprintf(name, "/plb/opb/ebc/nor_flash@0,0/partition@%x", from);
> 
> 	devp = finddevice(name);
> 	if (!devp) return;
> 
> 	if (getprop(devp, "reg", v, sizeof(v)) == sizeof(v)) {
> 		v[0] = to;
> 		setprop(devp, "reg", v, sizeof(v));
> 
> 		printf("NOR 64M fixup %x -> %x\n", from, to);
> 	}
> }
> 
> 
> static void warp_fixups(void)
> {
> 	unsigned long sysclk = 66000000;
> 
> 	ibm440ep_fixup_clocks(sysclk, 11059200, 50000000);
> 	ibm4xx_sdram_fixup_memsize();
> 	ibm4xx_fixup_ebc_ranges("/plb/opb/ebc");
> 	dt_fixup_mac_addresses(&bd.bi_enetaddr);
> 
> 	/* Fixup for 64M flash on Rev A boards. */
> 	if(bd.bi_flashsize == 0x4000000) {
> 		warp_fixup_one_nor(0x300000, 0x3f00000);
> 		warp_fixup_one_nor(0x340000, 0x3f40000);
> 		warp_fixup_one_nor(0x380000, 0x3f80000);
> 	}
> }

This doesn't seem right.  warp_fixup_one_nor() changes only the
partition's offset, so you're not changing the size of any
partitions.  If you're not going to actually use any of the extra
flash space with 64M, I can't see why you'd bother moving around the
partitions you have.

> I have tested this with the 64M NOR, and it seems to work. However, are 
> there going to be problems with the partition name not matching the reg 
> address entry?

In practice, probably not.  We already do a similar fixup on Ebony for
different flash layouts that won't leave the unit names correct.  We
really should get this right - and the fdt_set_name() function that's
now in libfdt should make that possible, it just needs some wiring up
to use in the bootwrapper.  That can come later, though, for now I
think applying your fixups without correcting the unit addresses is
adequate.

> If anybody has suggestions on better ways to do this, fire away.
> 
> And looking at this code, and other board ports, why is sysclk a local 
> variable and all the other numbers hardcoded in the args? I left it the 
> same way as the others but it does look a bit strange.

I think this also came from Ebony.  IIRC, the sysclk isn't strictly
speaking fixed, although it almost always has initialized value.  The
point of the local variable was that I planned to replace the static
initialization with some sort of probing once I figured out the
details.

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

Re: Could the DTS experts look at this?

[Sean MacLennan]

David Gibson wrote:
> This doesn't seem right.  warp_fixup_one_nor() changes only the
> partition's offset, so you're not changing the size of any
> partitions.  If you're not going to actually use any of the extra
> flash space with 64M, I can't see why you'd bother moving around the
> partitions you have.
>   
u-boot must be at the bottom of the flash. Also, for the 64M NOR flash 
you can put everything in the NOR flash, I just don't show the 
partitions. Booting from NOR is *much* faster than  booting from NAND.
>
> In practice, probably not.  We already do a similar fixup on Ebony for
> different flash layouts that won't leave the unit names correct.  We
> really should get this right - and the fdt_set_name() function that's
> now in libfdt should make that possible, it just needs some wiring up
> to use in the bootwrapper.  That can come later, though, for now I
> think applying your fixups without correcting the unit addresses is
> adequate.
>
>   
Ok.
>> If anybody has suggestions on better ways to do this, fire away.
>>
>> And looking at this code, and other board ports, why is sysclk a local 
>> variable and all the other numbers hardcoded in the args? I left it the 
>> same way as the others but it does look a bit strange.
>>     
>
> I think this also came from Ebony.  IIRC, the sysclk isn't strictly
> speaking fixed, although it almost always has initialized value.  The
> point of the local variable was that I planned to replace the static
> initialization with some sort of probing once I figured out the
> details.
>   
That makes sense. I don't think you can probe for the sysclk on the 
taco, so I may just put it as a constant to the function.

Cheers,
   Sean

Re: Could the DTS experts look at this?

[David Gibson]

On Sun, Feb 10, 2008 at 09:40:19PM -0500, Sean MacLennan wrote:
> David Gibson wrote:
> > This doesn't seem right.  warp_fixup_one_nor() changes only the
> > partition's offset, so you're not changing the size of any
> > partitions.  If you're not going to actually use any of the extra
> > flash space with 64M, I can't see why you'd bother moving around the
> > partitions you have.
> >   
> u-boot must be at the bottom of the flash. Also, for the 64M NOR flash 
> you can put everything in the NOR flash, I just don't show the 
> partitions. Booting from NOR is *much* faster than  booting from
> NAND.

Sorry, still not really following what's going on.  Without worrying
about the dts formatting or fixup code, can you summarise what the two
flash maps look like?

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

Re: Could the DTS experts look at this?

[Sean MacLennan]

David Gibson wrote:
> On Sun, Feb 10, 2008 at 09:40:19PM -0500, Sean MacLennan wrote:
>   
>> David Gibson wrote:
>>     
>>> This doesn't seem right.  warp_fixup_one_nor() changes only the
>>> partition's offset, so you're not changing the size of any
>>> partitions.  If you're not going to actually use any of the extra
>>> flash space with 64M, I can't see why you'd bother moving around the
>>> partitions you have.
>>>   
>>>       
>> u-boot must be at the bottom of the flash. Also, for the 64M NOR flash 
>> you can put everything in the NOR flash, I just don't show the 
>> partitions. Booting from NOR is *much* faster than  booting from
>> NAND.
>>     
>
> Sorry, still not really following what's going on.  Without worrying
> about the dts formatting or fixup code, can you summarise what the two
> flash maps look like?
>
>   
I guess what is confusing is that I am actually working with 3 flash 
maps right now, although there will only be one map in the final version.

Map1:

NOR:
  Kernel @ 0
  Ramdisk
  User
  FPGA
  Env
  U-boot @ 63.5M

Map 2:

NOR:
  FPGA
  Env
  U-boot @ 63.5M
NAND:
  Kernel @ 0
  Ramdisk
  User

Map 3:
  Same as Map 2 only 4M NOR rather than 64M, so u-boot @ 3.5M.


The u-boot, env, and FPGA are anchored at the bottom of the flash. 
Kernel is anchored at the top. Everything else goes in the middle.

The FPGA partition contains the FPGA image. The user partition contains 
a persistent JFFS2 file system. I don't use the user partition, so it 
doesn't show up in the map I sent.

So map 1 was used until we got the NAND working. Map 2 is an interim 
solution until we get the 4M flash. Map 3 is the final version.

Cheers,
   Sean

Re: Could the DTS experts look at this?

[David Gibson]

On Sun, Feb 10, 2008 at 10:49:55PM -0500, Sean MacLennan wrote:
> David Gibson wrote:
> > On Sun, Feb 10, 2008 at 09:40:19PM -0500, Sean MacLennan wrote:
> >   
> >> David Gibson wrote:
> >>     
> >>> This doesn't seem right.  warp_fixup_one_nor() changes only the
> >>> partition's offset, so you're not changing the size of any
> >>> partitions.  If you're not going to actually use any of the extra
> >>> flash space with 64M, I can't see why you'd bother moving around the
> >>> partitions you have.
> >>>   
> >>>       
> >> u-boot must be at the bottom of the flash. Also, for the 64M NOR flash 
> >> you can put everything in the NOR flash, I just don't show the 
> >> partitions. Booting from NOR is *much* faster than  booting from
> >> NAND.
> >>     
> >
> > Sorry, still not really following what's going on.  Without worrying
> > about the dts formatting or fixup code, can you summarise what the two
> > flash maps look like?
> >
> >   
> I guess what is confusing is that I am actually working with 3 flash 
> maps right now, although there will only be one map in the final version.
> 
> Map1:
> 
> NOR:
>   Kernel @ 0
>   Ramdisk
>   User
>   FPGA
>   Env
>   U-boot @ 63.5M
> 
> Map 2:
> 
> NOR:
>   FPGA
>   Env
>   U-boot @ 63.5M
> NAND:
>   Kernel @ 0
>   Ramdisk
>   User
> 
> Map 3:
>   Same as Map 2 only 4M NOR rather than 64M, so u-boot @ 3.5M.

But the partitions are all the same size, so in Map 2 there's a great
big gap between Env and U-boot?  Or there's a great big gap before
FPGA?

> The u-boot, env, and FPGA are anchored at the bottom of the flash. 
> Kernel is anchored at the top. Everything else goes in the middle.

Um.. so "bottom" actually means "high addresses" in the above?

> The FPGA partition contains the FPGA image. The user partition contains 
> a persistent JFFS2 file system. I don't use the user partition, so it 
> doesn't show up in the map I sent.
> 
> So map 1 was used until we got the NAND working. Map 2 is an interim 
> solution until we get the 4M flash. Map 3 is the final version.

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

Re: Could the DTS experts look at this?

[Sean MacLennan]

David Gibson wrote:
> But the partitions are all the same size, so in Map 2 there's a great
> big gap between Env and U-boot?  Or there's a great big gap before
> FPGA?
>   
There's a great big gap before the FPGA, 63M worth. Before we got the 
NAND working, we stored the kernel, the ramdisk image, and a persistent 
store there. But we moved everything to the NAND and are now going to a 
much smaller NOR chip.
>   
>> The u-boot, env, and FPGA are anchored at the bottom of the flash. 
>> Kernel is anchored at the top. Everything else goes in the middle.
>>     
>
> Um.. so "bottom" actually means "high addresses" in the above?
>   
Yes. Top = offset 0, bottom = size-of-flash.

And there is a bug in the initial code, I have to update the reg length 
field. I now have a Rev B board in my hands and tested that it works for 
both Rev A and Rev B.

Cheers,
   Sean

Re: Could the DTS experts look at this?

[David Gibson]

On Mon, Feb 11, 2008 at 08:07:14PM -0500, Sean MacLennan wrote:
> David Gibson wrote:
> > But the partitions are all the same size, so in Map 2 there's a great
> > big gap between Env and U-boot?  Or there's a great big gap before
> > FPGA?
> >   
> There's a great big gap before the FPGA, 63M worth. Before we got the 
> NAND working, we stored the kernel, the ramdisk image, and a persistent 
> store there. But we moved everything to the NAND and are now going to a 
> much smaller NOR chip.

Ok.

> >> The u-boot, env, and FPGA are anchored at the bottom of the flash. 
> >> Kernel is anchored at the top. Everything else goes in the middle.
> >>     
> >
> > Um.. so "bottom" actually means "high addresses" in the above?
> >   
> Yes. Top = offset 0, bottom = size-of-flash.

Um.. ok.  Not the way around I'd use, certainly...

> And there is a bug in the initial code, I have to update the reg length 
> field. 

Err.. now I'm doubly confused.  Initially I thought you'd need to
change the size part of reg somewhere, but your description above just
convinced me you didn't (because you were essentially just shifting a
4M map up into the high rather than low 4M of the 64M space).  Now
you're saying you do..

> I now have a Rev B board in my hands and tested that it works for 
> both Rev A and Rev B.

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

Re: Could the DTS experts look at this?

[Sean MacLennan]

David Gibson wrote:
> Err.. now I'm doubly confused.  Initially I thought you'd need to
> change the size part of reg somewhere, but your description above just
> convinced me you didn't (because you were essentially just shifting a
> 4M map up into the high rather than low 4M of the 64M space).  Now
> you're saying you do..
>   
If you tell the mtd driver that the flash is 64M, when it is really 4M, 
it goes oops. So you do have to get the size right in the reg field.

Cheers,
   Sean

Re: Could the DTS experts look at this?

[David Gibson]

On Mon, Feb 11, 2008 at 07:41:07PM -0500, Sean MacLennan wrote:
> David Gibson wrote:
> > Err.. now I'm doubly confused.  Initially I thought you'd need to
> > change the size part of reg somewhere, but your description above just
> > convinced me you didn't (because you were essentially just shifting a
> > 4M map up into the high rather than low 4M of the 64M space).  Now
> > you're saying you do..
> >   
> If you tell the mtd driver that the flash is 64M, when it is really 4M, 
> it goes oops. So you do have to get the size right in the reg field.

Ah!  The top-level reg field, not the partitions.  Now I understand.

Except, since the 4M layout is the expected future norm, I would have
thought that's what you'd encode into the dts, and mangle it into the
64M version, rather than the other way around.

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

Re: Could the DTS experts look at this?

[Scott Wood]

On Mon, Feb 11, 2008 at 07:41:07PM -0500, Sean MacLennan wrote:
> David Gibson wrote:
> > Err.. now I'm doubly confused.  Initially I thought you'd need to
> > change the size part of reg somewhere, but your description above just
> > convinced me you didn't (because you were essentially just shifting a
> > 4M map up into the high rather than low 4M of the 64M space).  Now
> > you're saying you do..
> >   
> If you tell the mtd driver that the flash is 64M, when it is really 4M, 
> it goes oops. So you do have to get the size right in the reg field.

It'd be nice if we could pass in a flag to tell it not to try to find
additional consecutive chips in the mapping...  It's a shame to have
probable chips, and still have to know how big they are anyway.

-Scott

Re: Could the DTS experts look at this?

[Grant Likely]

On Feb 12, 2008 11:52 AM, Scott Wood <scottwood@freescale.com> wrote:
> On Mon, Feb 11, 2008 at 07:41:07PM -0500, Sean MacLennan wrote:
> > David Gibson wrote:
> > > Err.. now I'm doubly confused.  Initially I thought you'd need to
> > > change the size part of reg somewhere, but your description above just
> > > convinced me you didn't (because you were essentially just shifting a
> > > 4M map up into the high rather than low 4M of the 64M space).  Now
> > > you're saying you do..
> > >
> > If you tell the mtd driver that the flash is 64M, when it is really 4M,
> > it goes oops. So you do have to get the size right in the reg field.
>
> It'd be nice if we could pass in a flag to tell it not to try to find
> additional consecutive chips in the mapping...  It's a shame to have
> probable chips, and still have to know how big they are anyway.

That is the job of the boot loader or wrapper.  The whole concept of
the device tree is that by the time it gets to the kernel it is an
accurate representation of the hardware; not a list of things which
might or might not be present.

I see two choices here;
1. have a different .dts variant for each board config (or a .dts with
macros that can generate different .dtb variants)
2. make the boot code massage the tree so it is accurate before it
gets to the kernel.

Either way, the mtd driver must be able to trust that the dt is
correct and complete.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Could the DTS experts look at this?

[Scott Wood]

Grant Likely wrote:
> On Feb 12, 2008 11:52 AM, Scott Wood <scottwood@freescale.com> wrote:
>> It'd be nice if we could pass in a flag to tell it not to try to find
>> additional consecutive chips in the mapping...  It's a shame to have
>> probable chips, and still have to know how big they are anyway.
> 
> That is the job of the boot loader or wrapper.

Hmm?  All I meant was that it'd be nice if there were an option in the 
Linux mtd code to disable the "look for another chip and cause a machine 
check if it isn't there" functionality.  It was an aside from the 
dts-variant issue.

> The whole concept of
> the device tree is that by the time it gets to the kernel it is an
> accurate representation of the hardware; not a list of things which
> might or might not be present.

But we don't generally include things which can be probed in a standard 
manner... which includes flash size.

-Scott

Re: Could the DTS experts look at this?

[David Woodhouse]

On Tue, 2008-02-12 at 13:10 -0600, Scott Wood wrote:
> Hmm?  All I meant was that it'd be nice if there were an option in the 
> Linux mtd code to disable the "look for another chip and cause a machine 
> check if it isn't there" functionality.  It was an aside from the 
> dts-variant issue.

Yeah, maybe -- although the probe code is hairy enough already; I'm not
massively keen on adding to it. Or even looking at it hard, if the truth
be told.

Alternatively, perhaps we should catch the machine check and recover
(like we do for failing I/O accesses on ppc32).

-- 
dwmw2