removing get_immrbase()??

removing get_immrbase()??

[Kumar Gala]

I'm not sure if we can actually get away with completely removing  
get_immrbase() but I figured I'd give everyone something to flame me  
about.  The current users are:

CPM/QE related users.

arch/powerpc/include/asm/cpm1.h:#define IMAP_ADDR		(get_immrbase())

	only used drivers/net/fs_enet/fs_enet-main.c which seems evil.

arch/powerpc/platforms/8xx/mpc885ads.h:#define CPM_MAP_ADDR		 
(get_immrbase() + MPC8xx_CPM_OFFSET)
	not used

arch/powerpc/sysdev/cpm1.c:	mpc8xx_immr = ioremap(get_immrbase(),  
0x4000);
	not sure? ideas?

arch/powerpc/include/asm/cpm2.h:#define CPM_MAP_ADDR (get_immrbase() +  
0x80000)
arch/powerpc/sysdev/cpm2.c:	cpm2_immr = ioremap(get_immrbase(),  
CPM_MAP_SIZE);
	these two are related and seem like we could look for "fsl,cpm2"

arch/powerpc/platforms/83xx/misc.c:	restart_reg_base =  
ioremap(get_immrbase() + 0x900, 0xff);
arch/powerpc/platforms/83xx/misc.c:	__be32 __iomem *spcr =  
ioremap(get_immrbase() + SPCR_OFFSET, 4);
arch/powerpc/platforms/83xx/mpc836x_mds.c:			immap =  
ioremap(get_immrbase() + 0x14a8, 8);
arch/powerpc/platforms/83xx/suspend.c:	rcw_regs =  
ioremap(get_immrbase() + IMMR_RCW_OFFSET,
arch/powerpc/platforms/83xx/suspend.c:	immrbase = get_immrbase();
arch/powerpc/platforms/83xx/usb.c:	immap = ioremap(get_immrbase(),  
0x1000);
arch/powerpc/platforms/83xx/usb.c:	immap = ioremap(get_immrbase(),  
0x1000);
arch/powerpc/platforms/83xx/usb.c:	immap = ioremap(get_immrbase(),  
0x1000);
	83xx cruft -- needs more investigation:


arch/powerpc/platforms/86xx/mpc8610_hpcd.c:	clkdvdr =  
ioremap(get_immrbase() + 0xe0800, sizeof(u32));
arch/powerpc/platforms/86xx/mpc86xx_smp.c:	mcm_vaddr =  
ioremap(get_immrbase() + MPC86xx_MCM_OFFSET,
	86xx: the first can be fixed to use guts, the second can be fixed to  
use the new "fsl,mcm" that I proposed:

arch/powerpc/sysdev/fsl_pci.c:	immr_base = get_immrbase();
	This one is a bit more problematic but can use the new "fsl,mcm-law",  
"fsl,ecm-law"

- k

Re: removing get_immrbase()??

[Timur Tabi]

On Wed, Apr 22, 2009 at 1:38 PM, Kumar Gala <galak@kernel.crashing.org> wro=
te:
> I'm not sure if we can actually get away with completely removing
> get_immrbase() but I figured I'd give everyone something to flame me abou=
t.
> =A0The current users are:
>
> CPM/QE related users.

I'm okay with doing this for QE, but I don't see it used in any QE code.

> arch/powerpc/include/asm/cpm2.h:#define CPM_MAP_ADDR (get_immrbase() +
> 0x80000)
> arch/powerpc/sysdev/cpm2.c: =A0 =A0 cpm2_immr =3D ioremap(get_immrbase(),
> CPM_MAP_SIZE);
> =A0 =A0 =A0 =A0these two are related and seem like we could look for "fsl=
,cpm2"

That's okay, as long as you don't break compatibility with older
device trees that don't have that property, unless you can demonstrate
that these trees would never work with the current kernel anyway.

I think this is a good idea, but it looks like we'll need to define a
whole bunch of new structures to replace all of the "+ offset" usages.

--=20
Timur Tabi
Linux kernel developer at Freescale

Re: removing get_immrbase()??

[Scott Wood]

Timur Tabi wrote:
>>        these two are related and seem like we could look for "fsl,cpm2"
> 
> That's okay, as long as you don't break compatibility with older
> device trees that don't have that property, unless you can demonstrate
> that these trees would never work with the current kernel anyway.

All CPM2 device trees should have fsl,cpm2 listed in the compatible of 
the CPM node.

-Scott

Re: removing get_immrbase()??

[Timur Tabi]

Scott Wood wrote:
> Timur Tabi wrote:
>>>        these two are related and seem like we could look for "fsl,cpm2"
>> That's okay, as long as you don't break compatibility with older
>> device trees that don't have that property, unless you can demonstrate
>> that these trees would never work with the current kernel anyway.
> 
> All CPM2 device trees should have fsl,cpm2 listed in the compatible of 
> the CPM node.

Yes, but did they always have that compatible field?  I'm concerned
about situations where someone updates his kernel but not his device
tree.  This is a scenerio that we always need to try to support.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: removing get_immrbase()??

[Scott Wood]

Timur Tabi wrote:
> Scott Wood wrote:
>> Timur Tabi wrote:
>>>>        these two are related and seem like we could look for "fsl,cpm2"
>>> That's okay, as long as you don't break compatibility with older
>>> device trees that don't have that property, unless you can demonstrate
>>> that these trees would never work with the current kernel anyway.
>> All CPM2 device trees should have fsl,cpm2 listed in the compatible of 
>> the CPM node.
> 
> Yes, but did they always have that compatible field? 

Yes, except for trees from the previous era of CPM2 bindings which are 
not supported at all.  This isn't new.

-Scott

Re: removing get_immrbase()??

[Kumar Gala]

On Apr 22, 2009, at 3:16 PM, Timur Tabi wrote:

> Scott Wood wrote:
>> Timur Tabi wrote:
>>>>       these two are related and seem like we could look for  
>>>> "fsl,cpm2"
>>> That's okay, as long as you don't break compatibility with older
>>> device trees that don't have that property, unless you can  
>>> demonstrate
>>> that these trees would never work with the current kernel anyway.
>>
>> All CPM2 device trees should have fsl,cpm2 listed in the compatible  
>> of
>> the CPM node.
>
> Yes, but did they always have that compatible field?  I'm concerned
> about situations where someone updates his kernel but not his device
> tree.  This is a scenerio that we always need to try to support.

I disagree.  If you update your kernel you should update your device  
tree (thus we have .dts in the kernel tree and not somewhere else).

- k

Re: removing get_immrbase()??

[Timur Tabi]

Kumar Gala wrote:

> I disagree.  If you update your kernel you should update your device  
> tree (thus we have .dts in the kernel tree and not somewhere else).

Is this a new policy?  I was under the impression that supporting older
device trees, if not too inconvenient, is desirable.  I've nack'd
patches before that broke backwards compatibility unnecessarily.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: removing get_immrbase()??

[Kumar Gala]

On Apr 22, 2009, at 4:33 PM, Timur Tabi wrote:

> Kumar Gala wrote:
>
>> I disagree.  If you update your kernel you should update your device
>> tree (thus we have .dts in the kernel tree and not somewhere else).
>
> Is this a new policy?  I was under the impression that supporting  
> older
> device trees, if not too inconvenient, is desirable.  I've nack'd
> patches before that broke backwards compatibility unnecessarily.

The specific issue I'm talking about is the addition of new nodes that  
might break old device trees.  I have no desire to try and say that I  
can't add new nodes and code related to them just because old device  
tree's didn't have them.

- k

Re: removing get_immrbase()??

[Timur Tabi]

Kumar Gala wrote:

> The specific issue I'm talking about is the addition of new nodes that  
> might break old device trees. 

New nodes or new properties?  The CPM nodes are not new.  On some device
trees, the original versions did not have a compatible property for the
CPM nodes (e.g. commit 0b5cf10691eb2c95a9126bf25f5e084d83d5d743).
Therefore, there are device trees out there that are missing some property.

Like I said earlier, if you can demonstrate that *all* of these device
tree would be broken with the latest kernel anyway, then we don't need
to worry about backwards compatibility.

I'm tired of debugging customer issues where the kernel is updated but
the firmware and device tree aren't.  IMHO, Kernel developers are
generally too lax when it comes to firmware and device tree backwards
compatibility, and I think that's wrong.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: removing get_immrbase()??

[Kumar Gala]

On Apr 22, 2009, at 4:46 PM, Timur Tabi wrote:

> Kumar Gala wrote:
>
>> The specific issue I'm talking about is the addition of new nodes  
>> that
>> might break old device trees.
>
> New nodes or new properties?  The CPM nodes are not new.  On some  
> device
> trees, the original versions did not have a compatible property for  
> the
> CPM nodes (e.g. commit 0b5cf10691eb2c95a9126bf25f5e084d83d5d743).
> Therefore, there are device trees out there that are missing some  
> property.
>
> Like I said earlier, if you can demonstrate that *all* of these device
> tree would be broken with the latest kernel anyway, then we don't need
> to worry about backwards compatibility.
>
> I'm tired of debugging customer issues where the kernel is updated but
> the firmware and device tree aren't.  IMHO, Kernel developers are
> generally too lax when it comes to firmware and device tree backwards
> compatibility, and I think that's wrong.

New nodes.  For example I've proposed a "local access window" node.   
Once I add it I plan on changing code to use it.  This will break an  
old device tree booting with the new kernel and I'm completely ok with  
that.

- k

Re: removing get_immrbase()??

[Timur Tabi]

Kumar Gala wrote:

> New nodes.  For example I've proposed a "local access window" node.   
> Once I add it I plan on changing code to use it.  This will break an  
> old device tree booting with the new kernel and I'm completely ok with  
> that.

Are we having two different conversations?  I was talking about this
block from your email:

>> arch/powerpc/include/asm/cpm2.h:#define CPM_MAP_ADDR (get_immrbase() +
>> 0x80000)
>> arch/powerpc/sysdev/cpm2.c:     cpm2_immr = ioremap(get_immrbase(),
>> CPM_MAP_SIZE);
>>        these two are related and seem like we could look for "fsl,cpm2"
> 
> That's okay, as long as you don't break compatibility with older
> device trees that don't have that property, unless you can demonstrate
> that these trees would never work with the current kernel anyway.

Specifically, I was referring to this comment:
	
	these two are related and seem like we could look for "fsl,cpm2"

And my point was that not all device trees have "fsl,cpm2" in their CPM
nodes.
	
-- 
Timur Tabi
Linux kernel developer at Freescale

Re: removing get_immrbase()??

[Kumar Gala]

On Apr 22, 2009, at 4:57 PM, Timur Tabi wrote:

> Kumar Gala wrote:
>
>> New nodes.  For example I've proposed a "local access window" node.
>> Once I add it I plan on changing code to use it.  This will break an
>> old device tree booting with the new kernel and I'm completely ok  
>> with
>> that.
>
> Are we having two different conversations?  I was talking about this
> block from your email:
>
>>> arch/powerpc/include/asm/cpm2.h:#define CPM_MAP_ADDR  
>>> (get_immrbase() +
>>> 0x80000)
>>> arch/powerpc/sysdev/cpm2.c:     cpm2_immr = ioremap(get_immrbase(),
>>> CPM_MAP_SIZE);
>>>       these two are related and seem like we could look for  
>>> "fsl,cpm2"
>>
>> That's okay, as long as you don't break compatibility with older
>> device trees that don't have that property, unless you can  
>> demonstrate
>> that these trees would never work with the current kernel anyway.
>
> Specifically, I was referring to this comment:
> 	
> 	these two are related and seem like we could look for "fsl,cpm2"
>
> And my point was that not all device trees have "fsl,cpm2" in their  
> CPM
> nodes.

Yes -- we are having two different conversations.  I've moved on from  
the specific issue of "fsl,cpm2" not existing in old device trees.   
I've moved to a more general statement about how I can solve some of  
the CPM2 related uses of cpm2_immr.  For example we assign cpmp based  
on cpm2_immr.  I could stop using cpm2_immr and solve this problem by  
adding a new device node for the comm-proc registers in the device  
trees at which point I'd break older .dts working with the kernel.

- k

Re: removing get_immrbase()??

[Scott Wood]

Timur Tabi wrote:
> Kumar Gala wrote:
> 
>> The specific issue I'm talking about is the addition of new nodes that  
>> might break old device trees. 
> 
> New nodes or new properties?  The CPM nodes are not new.  On some device
> trees, the original versions did not have a compatible property for the
> CPM nodes (e.g. commit 0b5cf10691eb2c95a9126bf25f5e084d83d5d743).

As I said earlier, old-style CPM trees are a completely different 
binding.  They are not supported.  They have not been supported for a 
long time.  They were only supported very briefly at the introduction of 
CPM hardware to arch/powerpc, and were full of problems.

> Like I said earlier, if you can demonstrate that *all* of these device
> tree would be broken with the latest kernel anyway, then we don't need
> to worry about backwards compatibility.

That is indeed the case.  A "demonstration" of that for the tree you 
quote is that the "reg" address changed -- if you tried feeding the old 
tree into the new kernel, it would not find the CPM command register. 
There is no code in the kernel that looks for the command-proc property 
anymore.

> I'm tired of debugging customer issues where the kernel is updated but
> the firmware and device tree aren't.  IMHO, Kernel developers are
> generally too lax when it comes to firmware and device tree backwards
> compatibility, and I think that's wrong.

I understand and agree (and it would be easier to get the backwards 
compatibility right if we didn't have the attitude of "we'll fix the dts 
later if we decide we actually care about that aspect of the hardware" 
when adding it in the first place) -- it just isn't an issue in this 
particular case.

-Scott

Re: removing get_immrbase()??

[Timur Tabi]

Scott Wood wrote:

> That is indeed the case.  A "demonstration" of that for the tree you 
> quote is that the "reg" address changed -- if you tried feeding the old 
> tree into the new kernel, it would not find the CPM command register. 
> There is no code in the kernel that looks for the command-proc property 
> anymore.

In that case, I have no issues.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: removing get_immrbase()??

[Grant Likely]

On Wed, Apr 22, 2009 at 3:39 PM, Kumar Gala <galak@kernel.crashing.org> wro=
te:
>
> On Apr 22, 2009, at 4:33 PM, Timur Tabi wrote:
>
>> Kumar Gala wrote:
>>
>>> I disagree. =A0If you update your kernel you should update your device
>>> tree (thus we have .dts in the kernel tree and not somewhere else).
>>
>> Is this a new policy? =A0I was under the impression that supporting olde=
r
>> device trees, if not too inconvenient, is desirable. =A0I've nack'd
>> patches before that broke backwards compatibility unnecessarily.
>
> The specific issue I'm talking about is the addition of new nodes that mi=
ght
> break old device trees. =A0I have no desire to try and say that I can't a=
dd
> new nodes and code related to them just because old device tree's didn't
> have them.

Ah... yes, you're right.  Never mind my previous reply.

g.

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

Re: removing get_immrbase()??

[Scott Wood]

Kumar Gala wrote:
> I disagree.  If you update your kernel you should update your device 
> tree (thus we have .dts in the kernel tree and not somewhere else).

No.  The device tree is a means to pass information from the firmware to 
the kernel.  It is part of the firmware.  That the repository of trees 
is in the Linux kernel for any boards which are not including the tree 
inside a bootwrapper is a historical accident.

Updating the dtb with the kernel just shifts the risk of incompatibility 
to interactions between the firmware and the dtb.  The same backwards 
compatibility considerations when making kernel changes that depend on 
firmware changes should be made when making kernel changes that depend 
on dts changes.

-Scott

Re: removing get_immrbase()??

[Kumar Gala]

On Apr 22, 2009, at 4:38 PM, Scott Wood wrote:

> Kumar Gala wrote:
>> I disagree.  If you update your kernel you should update your  
>> device tree (thus we have .dts in the kernel tree and not somewhere  
>> else).
>
> No.  The device tree is a means to pass information from the  
> firmware to the kernel.  It is part of the firmware.  That the  
> repository of trees is in the Linux kernel for any boards which are  
> not including the tree inside a bootwrapper is a historical accident.

I think its a point of view argument.  I don't agree its part of the  
firmware, at least not part of the firmware we use (u-boot).

> Updating the dtb with the kernel just shifts the risk of  
> incompatibility to interactions between the firmware and the dtb.   
> The same backwards compatibility considerations when making kernel  
> changes that depend on firmware changes should be made when making  
> kernel changes that depend on dts changes.

As I told Timur, I'm speaking of addition of new nodes and code that  
parses and expect those nodes to be there.

- k

Re: removing get_immrbase()??

[Scott Wood]

Kumar Gala wrote:
> On Apr 22, 2009, at 4:38 PM, Scott Wood wrote:
>> Kumar Gala wrote:
>>> I disagree.  If you update your kernel you should update your device 
>>> tree (thus we have .dts in the kernel tree and not somewhere else).
>>
>> No.  The device tree is a means to pass information from the firmware 
>> to the kernel.  It is part of the firmware.  That the repository of 
>> trees is in the Linux kernel for any boards which are not including 
>> the tree inside a bootwrapper is a historical accident.
> 
> I think its a point of view argument.

I don't.  It is the responsibility of u-boot to produce a complete 
device tree; what bits are in the dts and what bits are programatically 
generated is an internal implementation detail.  We are currently 
hosting some parts of that implementation detail in the Linux tree, but 
it is still a u-boot implementation detail.  U-boot is perfectly within 
its rights to generate the entire tree from scratch if it wanted to.

Just a few hours ago you were telling me that you didn't want to put
accurate information in the device tree because you wanted u-boot to 
generate it instead. :-)

The reason we have standards such as ePAPR in the first place is so that 
we are dealing with well-defined interfaces that can be used even when 
the firmware is not U-Boot at all (but merely something that is 
standards-compliant) and/or the kernel is not Linux at all (but merely 
something that is standards-compliant).  The downside to that is that 
revising standards is a bit more of a pain than revising code.  We can 
do it if it's worthwhile, but we should try to avoid doing it gratuitously.

> I don't agree its part of the 
> firmware, at least not part of the firmware we use (u-boot).

We have had many instances of certain versions of device trees being 
incompatible with certain versions of u-boot.

All I'm asking is that we treat a mandatory dts upgrade as seriously as 
a mandatory firmware upgrade.

>> Updating the dtb with the kernel just shifts the risk of 
>> incompatibility to interactions between the firmware and the dtb.  The 
>> same backwards compatibility considerations when making kernel changes 
>> that depend on firmware changes should be made when making kernel 
>> changes that depend on dts changes.
> 
> As I told Timur, I'm speaking of addition of new nodes and code that 
> parses and expect those nodes to be there.

And what do we gain from this change in interface with the firmware on 
hardware that is not exactly under active development?  What problem are 
you trying to solve?  Why do we need to get rid of get_immrbase() (as 
opposed to being less reliant on it going forward, and tolerating its 
failure on platforms that may be virtualized and thus not have the 
complete IMMR/CCSR)?

Will any of these new nodes need anything to be filled in by u-boot?

-Scott

Re: removing get_immrbase()??

[Timur Tabi]

On Wed, Apr 22, 2009 at 5:33 PM, Scott Wood <scottwood@freescale.com> wrote:

> All I'm asking is that we treat a mandatory dts upgrade as seriously as a
> mandatory firmware upgrade.

I agree with this statement 100%.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: removing get_immrbase()??

[David Gibson]

On Wed, Apr 22, 2009 at 04:55:42PM -0500, Kumar Gala wrote:
>
> On Apr 22, 2009, at 4:38 PM, Scott Wood wrote:
>
>> Kumar Gala wrote:
>>> I disagree.  If you update your kernel you should update your device 
>>> tree (thus we have .dts in the kernel tree and not somewhere else).
>>
>> No.  The device tree is a means to pass information from the firmware 
>> to the kernel.  It is part of the firmware.  That the repository of 
>> trees is in the Linux kernel for any boards which are not including the 
>> tree inside a bootwrapper is a historical accident.
>
> I think its a point of view argument.  I don't agree its part of the  
> firmware, at least not part of the firmware we use (u-boot).

It's not so much point of view as situation.  Putting the device tree
in the firmware and putting the device tree in the kernel are both
valid choices, with their own distinct advantages and drawbacks.  With
OF it's clearly in the firmware, with cuboot it's clearly in the
kernel.  With modern u-boot, it's a bit fuzzier.  But if the dts is
flashed into the device in the same way as the bootloader, then it's
fair to avoid having to change it, in the same way we usually provide
workarounds to work with old firmware versions.

-- 
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: removing get_immrbase()??

[Kumar Gala]

On Apr 22, 2009, at 9:26 PM, David Gibson wrote:

> On Wed, Apr 22, 2009 at 04:55:42PM -0500, Kumar Gala wrote:
>>
>> On Apr 22, 2009, at 4:38 PM, Scott Wood wrote:
>>
>>> Kumar Gala wrote:
>>>> I disagree.  If you update your kernel you should update your  
>>>> device
>>>> tree (thus we have .dts in the kernel tree and not somewhere else).
>>>
>>> No.  The device tree is a means to pass information from the  
>>> firmware
>>> to the kernel.  It is part of the firmware.  That the repository of
>>> trees is in the Linux kernel for any boards which are not  
>>> including the
>>> tree inside a bootwrapper is a historical accident.
>>
>> I think its a point of view argument.  I don't agree its part of the
>> firmware, at least not part of the firmware we use (u-boot).
>
> It's not so much point of view as situation.  Putting the device tree
> in the firmware and putting the device tree in the kernel are both
> valid choices, with their own distinct advantages and drawbacks.  With
> OF it's clearly in the firmware, with cuboot it's clearly in the
> kernel.  With modern u-boot, it's a bit fuzzier.  But if the dts is
> flashed into the device in the same way as the bootloader, then it's
> fair to avoid having to change it, in the same way we usually provide
> workarounds to work with old firmware versions.

I think this all sounds great in theory but in reality the vast  
majority (I'd say over 80-90%) we are talking about embedded reference  
boards.  They are subject to change as we evolve support over time.   
Our firmware isn't well defined and stable like a x86 PC system or  
true OF platform.  I will also say we have made mistakes as learned  
from them and one we keep repeating is NOT ensuring at a minimum that  
all parts of the SOC memory map are actually described in the device  
tree to start with.

If I look at the MPC8572 SoC from FSL I will see that the following  
items aren't described today:

* Local access windows
* ECM
* GPIO - we have a binding and its possible the board doesn't use them
* PME
* TLU
* perf mon
* watchpoint debug

We also don't describe the following interrupt sources:
* ECM
* perf mon
* GPIO
* PME
* TLU
* IEEE 1588

Until we meet the most basic level of properly describing 95% of the  
HW I don't see the value you guys prescribe to FW compatibility.   
Additionally I believe for embedded developers its perfectly  
reasonable to expect them (if they are using u-boot) to possibly have  
to update their .dts/dtb if they want to update their kernel.

- k

Re: removing get_immrbase()??

[David Gibson]

On Wed, Apr 22, 2009 at 10:36:36PM -0500, Kumar Gala wrote:
>
> On Apr 22, 2009, at 9:26 PM, David Gibson wrote:
>
>> On Wed, Apr 22, 2009 at 04:55:42PM -0500, Kumar Gala wrote:
>>>
>>> On Apr 22, 2009, at 4:38 PM, Scott Wood wrote:
>>>
>>>> Kumar Gala wrote:
>>>>> I disagree.  If you update your kernel you should update your  
>>>>> device
>>>>> tree (thus we have .dts in the kernel tree and not somewhere else).
>>>>
>>>> No.  The device tree is a means to pass information from the  
>>>> firmware
>>>> to the kernel.  It is part of the firmware.  That the repository of
>>>> trees is in the Linux kernel for any boards which are not  
>>>> including the
>>>> tree inside a bootwrapper is a historical accident.
>>>
>>> I think its a point of view argument.  I don't agree its part of the
>>> firmware, at least not part of the firmware we use (u-boot).
>>
>> It's not so much point of view as situation.  Putting the device tree
>> in the firmware and putting the device tree in the kernel are both
>> valid choices, with their own distinct advantages and drawbacks.  With
>> OF it's clearly in the firmware, with cuboot it's clearly in the
>> kernel.  With modern u-boot, it's a bit fuzzier.  But if the dts is
>> flashed into the device in the same way as the bootloader, then it's
>> fair to avoid having to change it, in the same way we usually provide
>> workarounds to work with old firmware versions.
>
> I think this all sounds great in theory but in reality the vast majority 
> (I'd say over 80-90%) we are talking about embedded reference boards.  
> They are subject to change as we evolve support over time.  Our firmware 
> isn't well defined and stable like a x86 PC system or true OF platform.  
> I will also say we have made mistakes as learned from them and one we 
> keep repeating is NOT ensuring at a minimum that all parts of the SOC 
> memory map are actually described in the device tree to start with.

Well, yes, I guess I agree.  How immutable you consider the device
tree blob to be is a judgement call based on the specific details of
platform/board in question.  If it is indeed a reference platform, in
the early stages of development where it's reasonably easy to change
the dtb, then it's probably best to change the dtb in sync with the
kernel to reduce long-term cruft build-up.  But once the board is
sufficiently widely deployed, you want to stop doing that and include
backwards compatibility workarounds in the kernel to cope with the
widely deployed broken trees.

-- 
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: removing get_immrbase()??

[Kumar Gala]

On Apr 22, 2009, at 11:06 PM, David Gibson wrote:

> Well, yes, I guess I agree.  How immutable you consider the device
> tree blob to be is a judgement call based on the specific details of
> platform/board in question.  If it is indeed a reference platform, in
> the early stages of development where it's reasonably easy to change
> the dtb, then it's probably best to change the dtb in sync with the
> kernel to reduce long-term cruft build-up.  But once the board is
> sufficiently widely deployed, you want to stop doing that and include
> backwards compatibility workarounds in the kernel to cope with the
> widely deployed broken trees.

I disagree with the point about providing workarounds to cope w/ 
deployed device trees (at least for the problems I'm thinking off in  
which nodes didn't exist).  This just sounds like double work and is a  
disincentive to actually making such changes.

Lets say I had an error driver for our MCM (core to soc coherency  
module).  It was getting the base address by using get_immrbase().   
Today I proposed a proper device node for the MCM block as it doesn't  
exist in .dts today.  We add such a node into .dts and I can clean up  
my error driver to use proper device node information.  However I've  
just broken any old .dts that didn't have this node.  You are saying I  
need to add code into the kernel to create this new node and we have  
to keep that code around for ever in the kernel.. why would I ever  
bother to actually changing anything than.

- k

Re: removing get_immrbase()??

[David Gibson]

On Wed, Apr 22, 2009 at 11:41:31PM -0500, Kumar Gala wrote:
>
> On Apr 22, 2009, at 11:06 PM, David Gibson wrote:
>
>> Well, yes, I guess I agree.  How immutable you consider the device
>> tree blob to be is a judgement call based on the specific details of
>> platform/board in question.  If it is indeed a reference platform, in
>> the early stages of development where it's reasonably easy to change
>> the dtb, then it's probably best to change the dtb in sync with the
>> kernel to reduce long-term cruft build-up.  But once the board is
>> sufficiently widely deployed, you want to stop doing that and include
>> backwards compatibility workarounds in the kernel to cope with the
>> widely deployed broken trees.
>
> I disagree with the point about providing workarounds to cope w/deployed 
> device trees (at least for the problems I'm thinking off in which nodes 
> didn't exist).  This just sounds like double work and is a disincentive 
> to actually making such changes.
>
> Lets say I had an error driver for our MCM (core to soc coherency  
> module).  It was getting the base address by using get_immrbase().   
> Today I proposed a proper device node for the MCM block as it doesn't  
> exist in .dts today.  We add such a node into .dts and I can clean up my 
> error driver to use proper device node information.  However I've just 
> broken any old .dts that didn't have this node.  You are saying I need to 
> add code into the kernel to create this new node and we have to keep that 
> code around for ever in the kernel.. why would I ever bother to actually 
> changing anything than.

Well, again.  It's a judgement call, balancing the pain of having to
update the dts files (which depends on how widely deployed the
platform is) versus the pain of having to keep the bacwards
compatibility shim in 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: removing get_immrbase()??

[Timur Tabi]

David Gibson wrote:
> On Wed, Apr 22, 2009 at 11:41:31PM -0500, Kumar Gala wrote:
>> Lets say I had an error driver for our MCM (core to soc coherency  
>> module).  It was getting the base address by using get_immrbase().   
>> Today I proposed a proper device node for the MCM block as it doesn't  
>> exist in .dts today.  We add such a node into .dts and I can clean up my 
>> error driver to use proper device node information.  However I've just 
>> broken any old .dts that didn't have this node.  You are saying I need to 
>> add code into the kernel to create this new node and we have to keep that 
>> code around for ever in the kernel.. why would I ever bother to actually 
>> changing anything than.
> 
> Well, again.  It's a judgement call, balancing the pain of having to
> update the dts files (which depends on how widely deployed the
> platform is) versus the pain of having to keep the bacwards
> compatibility shim in the kernel.

I agree with this sentiment.  I'm only asking for a reasonable attempt
at adding backwards compatibility via an isolated code block.  Sprinkle
in a few comments, and that should be enough.  It won't always be
possible to add such code, but at the very least, I expect the
driver/kernel to clearly indicate what's missing from the device tree.
In Kumar's example above, I expect the kernel to say that the MCM node
is missing.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: removing get_immrbase()??

[Timur Tabi]

Kumar Gala wrote:

> Until we meet the most basic level of properly describing 95% of the  
> HW I don't see the value you guys prescribe to FW compatibility.   
> Additionally I believe for embedded developers its perfectly  
> reasonable to expect them (if they are using u-boot) to possibly have  
> to update their .dts/dtb if they want to update their kernel.

That sounds like you want to throw out the baby with the bathwater.
Just because we can't get close to 100% representation of the hardware
in the DTS, that does not mean that the representation we do have should
be considered tenuous (for lack of a better word).

We should *strive* to maintain backwards compatibility.  If that means
adding a few lines of isolated code every now and then, I don't see that
as a bad thing at all.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: removing get_immrbase()??

[Scott Wood]

On Wed, Apr 22, 2009 at 10:36:36PM -0500, Kumar Gala wrote:
> I think this all sounds great in theory but in reality the vast  
> majority (I'd say over 80-90%) we are talking about embedded reference  
> boards.

I've handled plenty of support e-mails from people using custom 82xx
boards with device trees that have already forked from what's in the
kernel.  I'm not looking forward to another round of "why doesn't this
work?" unless there's something significant to be gained from it.

-Scott

Re: removing get_immrbase()??

[Timur Tabi]

David Gibson wrote:

> It's not so much point of view as situation.  Putting the device tree
> in the firmware and putting the device tree in the kernel are both
> valid choices, with their own distinct advantages and drawbacks. 

I was under the impression that the reason we put the device trees in
the kernel is because we didn't have a better place to put them.
Keeping them in the kernel repository was just convenient.

So I personally don't consider the *location* of the DTS files to be a
basis for deciding what they really mean.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: removing get_immrbase()??

[Anton Vorontsov]

On Thu, Apr 23, 2009 at 08:02:20AM -0500, Timur Tabi wrote:
> David Gibson wrote:
> 
> > It's not so much point of view as situation.  Putting the device tree
> > in the firmware and putting the device tree in the kernel are both
> > valid choices, with their own distinct advantages and drawbacks. 
> 
> I was under the impression that the reason we put the device trees in
> the kernel is because we didn't have a better place to put them.
> Keeping them in the kernel repository was just convenient.
> 
> So I personally don't consider the *location* of the DTS files to be a
> basis for deciding what they really mean.

I'm not sure why are we trying to make things harder for ourselves?
x86 has a long history fighting with bogus bioses/dmi/acpi tables,
up to the point where they completely ignore any information that
BIOS provides, because that information is unreliable or hard to
deal with.

With FDT (i.e. not hard-coded OF), we have a great opportunity to
keep both device tree and Linux code legacyjunk-free.

All we have to do is to declare one simple thing: don't put
device-tree into the read-only memory. Upgrading device tree blob
in the rewritable NOR/NAND flashes is safe in overwhelming cases,
just as safe as upgrading kernel image in the NOR/NAND.

And for those who didn't listen our pleases, i.e. for those
who put device-tree blob into some kind of ROM or dangerous-
to-upgrade memory or region of memory, we can always provide
boot wrappers, and thus isolate the legacy stuff. I mean isolate
it in their small legacy world, if anyone actually cares about
these cases.

As for Freescale parts, all the reference board I've seen were
very friendly wrt upgrading their device-trees, i.e. none of
the boards were shipping with device-tree soldered into the
firmware.

And note that most developers are using up-to-date firmwares
(U-Boots), device trees, and kernels. And that means that old
device-tree + new kernel combination is left untested for years.
And untested stuff is broken stuff, by definition.

Sure, there is a completely different story wrt device-tree
changes that might break firmwares. And that I believe we'd
better avoid. For example device_type = "soc", if removed,
most firmwares would not fix-up {clock,bus}-frequency properties.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: removing get_immrbase()??

[Timur Tabi]

Anton Vorontsov wrote:

> And note that most developers are using up-to-date firmwares
> (U-Boots), device trees, and kernels. 

Developers? Yes.
End-users? No.

Updating U-Boot itself is often unacceptable for end-users.  There's
also a strong connection between U-Boot and the device tree.  That
connection gets stronger with every release, as U-Boot makes more and
more changes to the device tree before passing it to the kernel.  This
means that if you cannot update U-Boot, you might not be able to update
your device tree either.

We've run into plenty of situations where customers will update the
kernel, but insist that U-Boot and the device tree remain unchanged.

> And that means that old
> device-tree + new kernel combination is left untested for years.
> And untested stuff is broken stuff, by definition.

I'm not saying that should officially support it.  I'm saying we should
make an effort to minimize the problem.  Adding a few isolated lines of
code to maintain that compatibility, and running a few tests, is not a
bad idea and can save headaches for some people in the future.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: removing get_immrbase()??

[Kumar Gala]

On Apr 23, 2009, at 9:02 AM, Timur Tabi wrote:

> We've run into plenty of situations where customers will update the
> kernel, but insist that U-Boot and the device tree remain unchanged.

when?  I'm not aware of any significant # of cases that customer is  
willing to update kernel & not dts.  Usually if they are willing to  
update kernel they tend to be willing to update everything.

- k

Re: removing get_immrbase()??

[Timur Tabi]

Kumar Gala wrote:

> when?  I'm not aware of any significant # of cases that customer is  
> willing to update kernel & not dts.  Usually if they are willing to  
> update kernel they tend to be willing to update everything.

Well, now that you ask, I can't give you specifics, because I don't
remember.

-- 
Timur Tabi
Linux kernel developer at Freescale

RE: removing get_immrbase()??

[Wrobel Heinz-R39252]

> > We've run into plenty of situations where customers will update the=20
> > kernel, but insist that U-Boot and the device tree remain unchanged.
>=20
> when?  I'm not aware of any significant # of cases that=20
> customer is willing to update kernel & not dts.  Usually if=20
> they are willing to update kernel they tend to be willing to=20
> update everything.

I recently fried a U-Boot flash on a machine at home when upgrading and
the machine sits there and is dead of course. Luckily that machine
doesn't have to be up 24x7, so I can wait until I have time to fix the
situation ... and I can either pull the socketed flash or hook up a JTAG
debugger.

But Freescale or other eval(!) boards or isolated Power Architecture
based home use machines like my fried one should not be a reference for
this kind of discussion.

I see a very common scenario with Telecom/Networking customers. They
have a boot flash with firmware and basic startup/BIST SW which they do
not really ever want to touch at all even if they should after it
shipped. If a remote upgrade on just a few out of <n> installed systems
fails, they can send technicians to Mars to pull the board(s), and
service guarantees, and profit, are out the window then. That makes
U-Boot and/or subsequent ultra-low-level startup/BIST SW from the same
boot source *very* firm. If a device-tree is served from there for
whichever reason, then you have a *very* firm device-tree, too.

Then these customers either have a second flash with a filesystem or
more volatile images in the sense that they see that other flash as
upgradable (if they have to), or a physical link to some remote fs that
serves them the kernel and application load. They still do not like
upgrades too much as any upgrade can have service impact. But they do
them when necessary because they know they have a way to revert to
previous or fixed releases remotely as they can always depend on the
original boot loader to be there.

It would not be smart to prohibit any change ever, but we also shouldn't
cause device-tree changes a lot just because we felt like it today. Both
seems a bit extreme.

There must be some middle ground, err'ing towards the conservative side.
A change affecting "lower" levels than the kernel should be very well
thought through, and if it is necessary, we should strive to keep
compatibility to a level that makes sense and possibly eases a
transition over some time.

If a few clearly marked code lines (with a possibly marked planned max
lifetime) ease compatibility and transition, they should be considered.

Hope this helps

Heinz

Re: removing get_immrbase()??

[Anton Vorontsov]

On Thu, Apr 23, 2009 at 09:02:49AM -0500, Timur Tabi wrote:
> Anton Vorontsov wrote:
> 
> > And note that most developers are using up-to-date firmwares
> > (U-Boots), device trees, and kernels. 
> 
> Developers? Yes.
> End-users? No.

If end-users upgraded the kernel on some FSL board, then there
should be no technical problem upgrading device tree too.

> Updating U-Boot itself is often unacceptable for end-users.  There's
> also a strong connection between U-Boot and the device tree.  That
> connection gets stronger with every release, as U-Boot makes more and
> more changes to the device tree before passing it to the kernel.  This
> means that if you cannot update U-Boot, you might not be able to update
> your device tree either.

As I said, this case is a separate matter. Just as device_type = "soc",
yes we should avoid removing it. But if we 100% sure that our device
tree changes won't break compatibility with officially supported
firmware, then IMO we should just go ahead with the changes.

> We've run into plenty of situations where customers will update the
> kernel, but insist that U-Boot

That I can understand.

> and the device tree remain unchanged.

That I can't. I wonder what was the rationale behind this.

> > And that means that old
> > device-tree + new kernel combination is left untested for years.
> > And untested stuff is broken stuff, by definition.
> 
> I'm not saying that should officially support it.  I'm saying we should
> make an effort to minimize the problem.

That doesn't work in practice. I bet if I'll try booting recent Linux
with FSL-provided device-tree on, say, MPC8323E-RDB it simply won't
boot.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: removing get_immrbase()??

[Scott Wood]

On Thu, Apr 23, 2009 at 05:50:05PM +0400, Anton Vorontsov wrote:
> As for Freescale parts, all the reference board I've seen were
> very friendly wrt upgrading their device-trees, i.e. none of
> the boards were shipping with device-tree soldered into the
> firmware.

But many of them have broken when a dtb that u-boot didn't like was
inserted.

> And note that most developers are using up-to-date firmwares
> (U-Boots), device trees, and kernels. 

So then why did we have to make cuImage?

> And that means that old device-tree + new kernel combination is left
> untested for years. And untested stuff is broken stuff, by definition.

There's a difference between risking that something may be broken, and
gratuitously making it broken.

> Sure, there is a completely different story wrt device-tree
> changes that might break firmwares. And that I believe we'd
> better avoid. For example device_type = "soc", if removed,
> most firmwares would not fix-up {clock,bus}-frequency properties.

Even if the given change may not break the firmware, it could force an
update in which a prior change breaks the firmware.

-Scott

Re: removing get_immrbase()??

[Anton Vorontsov]

On Thu, Apr 23, 2009 at 11:00:48AM -0500, Scott Wood wrote:
> On Thu, Apr 23, 2009 at 05:50:05PM +0400, Anton Vorontsov wrote:
> > As for Freescale parts, all the reference board I've seen were
> > very friendly wrt upgrading their device-trees, i.e. none of
> > the boards were shipping with device-tree soldered into the
> > firmware.
> 
> But many of them have broken when a dtb that u-boot didn't like was
> inserted.

Yes, I agree here. And I'm not going to contradict on this
matter. If we stick with these two rules, I think we should always
be OK:

(1) We should avoid any changes that might break compatibility with
    an officially supported firmware;
(2) Breaking "new Linux" <-> "old device tree" compatibility should
    be OK if (1) is satisfied.

Note that this applies only for targets that have no problem w/
device-tree upgrades.

> > And note that most developers are using up-to-date firmwares
> > (U-Boots), device trees, and kernels. 
> 
> So then why did we have to make cuImage?

I don't know. I've never used them. ;-) Honestly. But I believe
it's a great stuff for those who use really old and now unsupported
firmwares.

It has nothing to do with device-tree changes though. We can easily
maintain device-tree compatibility w/ firmwares, but what is the
point in maintaining Linux' compatibility for older device trees
when you can easily upgrade it?

That is, I still don't get why somebody don't want to upgrade
device tree along with Linux (assuming it won't cause breakages in
the firmware)?

[...]
> > Sure, there is a completely different story wrt device-tree
> > changes that might break firmwares. And that I believe we'd
> > better avoid. For example device_type = "soc", if removed,
> > most firmwares would not fix-up {clock,bus}-frequency properties.
> 
> Even if the given change may not break the firmware, it could force an
> update in which a prior change breaks the firmware.

I'm not sure I'm following here. Could you give an example?

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: removing get_immrbase()??

[Scott Wood]

Anton Vorontsov wrote:
> On Thu, Apr 23, 2009 at 11:00:48AM -0500, Scott Wood wrote:
>> Even if the given change may not break the firmware, it could force an
>> update in which a prior change breaks the firmware.
> 
> I'm not sure I'm following here. Could you give an example?

1. U-boot X1 is used with kernel Y1 and device tree Z1.  All is well.
2. U-boot X2 needs device tree Z2.  Device tree Z2 goes in kernel Y2. 
It will break when used with u-boot X1, so users of u-boot X1 do not 
upgrade their device trees.  All is well.
3. Kernel Y3 makes a change to the device tree (now Z3) that it needs. 
This change does not have u-boot compatibility issues by itself, so the 
kernel developer feels confident saying "go ahead and upgrade your tree, 
it should be fine".  Users of U-boot X1 pull in the latest device tree, 
which includes the changes made in device tree Z2.  Breakage.

-Scott

Re: removing get_immrbase()??

[Anton Vorontsov]

On Thu, Apr 23, 2009 at 12:03:06PM -0500, Scott Wood wrote:
> Anton Vorontsov wrote:
>> On Thu, Apr 23, 2009 at 11:00:48AM -0500, Scott Wood wrote:
>>> Even if the given change may not break the firmware, it could force an
>>> update in which a prior change breaks the firmware.
>>
>> I'm not sure I'm following here. Could you give an example?
>
[...]
> Device tree Z2 goes in kernel Y2. It 
> will break when used with u-boot X1,

This is exactly what we should try to avoid. We should not accept
any changes that might break older firmwares. That is, keep the
device tree workable with X1 and X2, by any means.

For example, if X1 looks for "/soc8323@..." name instead of
compatible entry, we'll have to live with the name forever,
but we should be free to switch Linux so that it'll use only the
"compatible" property matching.

If X1 looks for "/soc8323@..."  and X2 looks for "/soc@...", so that
Z1 and Z2 aren't compatible at all, then we're in trouble. But we're
in trouble regardless of what Linux is doing, no?

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: removing get_immrbase()??

[Scott Wood]

Anton Vorontsov wrote:
> This is exactly what we should try to avoid. We should not accept
> any changes that might break older firmwares. That is, keep the
> device tree workable with X1 and X2, by any means.

I think that's a much bigger restriction than trying to keep the kernel 
compatible with older device trees.  Consider changes in the physical 
memory map, etc.

> If X1 looks for "/soc8323@..."  and X2 looks for "/soc@...", so that
> Z1 and Z2 aren't compatible at all, then we're in trouble. But we're
> in trouble regardless of what Linux is doing, no?

Not if Linux can continue to deal with X1 and Z1.

-Scott

Re: removing get_immrbase()??

[David Gibson]

On Thu, Apr 23, 2009 at 05:50:05PM +0400, Anton Vorontsov wrote:
> On Thu, Apr 23, 2009 at 08:02:20AM -0500, Timur Tabi wrote:
> > David Gibson wrote:
> > 
> > > It's not so much point of view as situation.  Putting the device tree
> > > in the firmware and putting the device tree in the kernel are both
> > > valid choices, with their own distinct advantages and drawbacks. 
> > 
> > I was under the impression that the reason we put the device trees in
> > the kernel is because we didn't have a better place to put them.
> > Keeping them in the kernel repository was just convenient.
> > 
> > So I personally don't consider the *location* of the DTS files to be a
> > basis for deciding what they really mean.
> 
> I'm not sure why are we trying to make things harder for ourselves?
> x86 has a long history fighting with bogus bioses/dmi/acpi tables,
> up to the point where they completely ignore any information that
> BIOS provides, because that information is unreliable or hard to
> deal with.
> 
> With FDT (i.e. not hard-coded OF), we have a great opportunity to
> keep both device tree and Linux code legacyjunk-free.
> 
> All we have to do is to declare one simple thing: don't put
> device-tree into the read-only memory. Upgrading device tree blob
> in the rewritable NOR/NAND flashes is safe in overwhelming cases,
> just as safe as upgrading kernel image in the NOR/NAND.
> 
> And for those who didn't listen our pleases, i.e. for those
> who put device-tree blob into some kind of ROM or dangerous-
> to-upgrade memory or region of memory, we can always provide
> boot wrappers, and thus isolate the legacy stuff. I mean isolate
> it in their small legacy world, if anyone actually cares about
> these cases.

Indeed, this is a large part of the idea for the flattened device
tree.  Which is why I don't understand why when the idea of dtbs has
been floated, everyone seems to have been in a rush to put the blobs
into the firmware.  That's certainly one approach, and a good one for
certain needs (one kernel binary on many platforms, for example), but
putting the blob in the kernel is also an entirely valid approach and
avoids some drawbacks of having the blob in the firmware.

We're never going to get away from having shitty firmwares, but at
least with fdts we can isolate the handling of them from the bulk of
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: removing get_immrbase()??

[David Gibson]

On Thu, Apr 23, 2009 at 08:02:20AM -0500, Timur Tabi wrote:
> David Gibson wrote:
> 
> > It's not so much point of view as situation.  Putting the device tree
> > in the firmware and putting the device tree in the kernel are both
> > valid choices, with their own distinct advantages and drawbacks. 
> 
> I was under the impression that the reason we put the device trees in
> the kernel is because we didn't have a better place to put them.
> Keeping them in the kernel repository was just convenient.
> 
> So I personally don't consider the *location* of the DTS files to be a
> basis for deciding what they really mean.

I'm not talking abou where the DTS files sit, I'm talking about where
the compiled blob sits.  There are a number of options here:
	* built into the firmware
	* loaded by the firmware, say from a separate flash partition
(e.g. modern uboot)
	* generated at runtime by the firmware 
	* built into the kernel's bootwrapper (e.g. cuboot).
	* generated by the bootwrapper at runtime from
firmware information in another format (e.g. paul's proposed res2dt
approach for PReP)

Which method is appropriate depends on the needs of the platform,
including what legacy stuff we need to deal with for the platform.
Then in turn, how hard we should work to make the kernel backwards
compatible with old device tree formats depends on what choice we made
here (for platform specific devices, obviously).

-- 
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: removing get_immrbase()??

[Grant Likely]

On Wed, Apr 22, 2009 at 3:31 PM, Kumar Gala <galak@kernel.crashing.org> wro=
te:
>
> On Apr 22, 2009, at 3:16 PM, Timur Tabi wrote:
>
>> Scott Wood wrote:
>>>
>>> Timur Tabi wrote:
>>>>>
>>>>> =A0 =A0 =A0these two are related and seem like we could look for "fsl=
,cpm2"
>>>>
>>>> That's okay, as long as you don't break compatibility with older
>>>> device trees that don't have that property, unless you can demonstrate
>>>> that these trees would never work with the current kernel anyway.
>>>
>>> All CPM2 device trees should have fsl,cpm2 listed in the compatible of
>>> the CPM node.
>>
>> Yes, but did they always have that compatible field? =A0I'm concerned
>> about situations where someone updates his kernel but not his device
>> tree. =A0This is a scenerio that we always need to try to support.
>
> I disagree. =A0If you update your kernel you should update your device tr=
ee
> (thus we have .dts in the kernel tree and not somewhere else).

Not always possible.  The device tree may be 'softer' than firmware,
and easier to update, but it is still firmer than the kernel.  That is
why so much effort has been spent to not break compatibility with
older device trees.

g.

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

Re: removing get_immrbase()??

[Anton Vorontsov]

On Thu, Apr 23, 2009 at 07:53:11AM -0600, Grant Likely wrote:
> On Wed, Apr 22, 2009 at 3:31 PM, Kumar Gala <galak@kernel.crashing.org> wrote:
> >
> > On Apr 22, 2009, at 3:16 PM, Timur Tabi wrote:
> >
> >> Scott Wood wrote:
> >>>
> >>> Timur Tabi wrote:
> >>>>>
> >>>>>      these two are related and seem like we could look for "fsl,cpm2"
> >>>>
> >>>> That's okay, as long as you don't break compatibility with older
> >>>> device trees that don't have that property, unless you can demonstrate
> >>>> that these trees would never work with the current kernel anyway.
> >>>
> >>> All CPM2 device trees should have fsl,cpm2 listed in the compatible of
> >>> the CPM node.
> >>
> >> Yes, but did they always have that compatible field?  I'm concerned
> >> about situations where someone updates his kernel but not his device
> >> tree.  This is a scenerio that we always need to try to support.
> >
> > I disagree.  If you update your kernel you should update your device tree
> > (thus we have .dts in the kernel tree and not somewhere else).
> 
> Not always possible.  The device tree may be 'softer' than firmware,
> and easier to update, but it is still firmer than the kernel.  That is
> why so much effort has been spent to not break compatibility with
> older device trees.

I'd suggest to deal with this on case-by-case basis. In every MPC8xx
and MPC8xxx boards I've seen from Freescale there is absolutely no
difference in upgrading kernel or device-tree blob.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: removing get_immrbase()??

[David Gibson]

On Thu, Apr 23, 2009 at 07:53:11AM -0600, Grant Likely wrote:
> On Wed, Apr 22, 2009 at 3:31 PM, Kumar Gala <galak@kernel.crashing.org> wrote:
> >
> > On Apr 22, 2009, at 3:16 PM, Timur Tabi wrote:
> >
> >> Scott Wood wrote:
> >>>
> >>> Timur Tabi wrote:
> >>>>>
> >>>>>      these two are related and seem like we could look for "fsl,cpm2"
> >>>>
> >>>> That's okay, as long as you don't break compatibility with older
> >>>> device trees that don't have that property, unless you can demonstrate
> >>>> that these trees would never work with the current kernel anyway.
> >>>
> >>> All CPM2 device trees should have fsl,cpm2 listed in the compatible of
> >>> the CPM node.
> >>
> >> Yes, but did they always have that compatible field?  I'm concerned
> >> about situations where someone updates his kernel but not his device
> >> tree.  This is a scenerio that we always need to try to support.
> >
> > I disagree.  If you update your kernel you should update your device tree
> > (thus we have .dts in the kernel tree and not somewhere else).
> 
> Not always possible.  The device tree may be 'softer' than firmware,
> and easier to update, but it is still firmer than the kernel.  That
> is

Again, this is not inherent, it's a platform design choice.  It's this
way for modern u-boot, but not for all platforms.

> why so much effort has been spent to not break compatibility with
> older device trees.

-- 
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: removing get_immrbase()??

[Scott Wood]

Kumar Gala wrote:
> I'm not sure if we can actually get away with completely removing 
> get_immrbase() but I figured I'd give everyone something to flame me 
> about.  The current users are:

I think we want to keep it for an eventual re-introduction of a large 
TLB entry to cover IMMR (but no longer at a fixed virtual address, of 
course).

> CPM/QE related users.
> 
> arch/powerpc/include/asm/cpm1.h:#define IMAP_ADDR        (get_immrbase())
> 
>     only used drivers/net/fs_enet/fs_enet-main.c which seems evil.

That driver *is* evil. :-)

It looks like the only remaining user of fs_enet_immap (which is what 
IMAP_ADDR is used to initialize) is in mac-fec.c, under CONFIG_DUET -- 
which hasn't been touched since 2005 and appears to have died with arch/ppc.

I'm fine with removing it -- it probably no longer compiles anyway.

> arch/powerpc/sysdev/cpm1.c:    mpc8xx_immr = ioremap(get_immrbase(), 
> 0x4000);
>     not sure? ideas?

This is used for accessing a variety of registers, not all of which are 
currently expressed in the device tree.

> arch/powerpc/include/asm/cpm2.h:#define CPM_MAP_ADDR (get_immrbase() + 
> 0x80000)
> arch/powerpc/sysdev/cpm2.c:    cpm2_immr = ioremap(get_immrbase(), 
> CPM_MAP_SIZE);
>     these two are related and seem like we could look for "fsl,cpm2"

And do what with it that wouldn't be a reimplementation of get_immrbase()?

> arch/powerpc/platforms/83xx/suspend.c:    rcw_regs = 
> ioremap(get_immrbase() + IMMR_RCW_OFFSET,
> arch/powerpc/platforms/83xx/suspend.c:    immrbase = get_immrbase();

The suspend code touches a variety of SOC registers in various blocks -- 
likely including some which are not described by any device node at present.

-Scott

Re: removing get_immrbase()??

[Kumar Gala]

On Apr 22, 2009, at 2:44 PM, Scott Wood wrote:

>> CPM/QE related users.
>> arch/powerpc/include/asm/cpm1.h:#define IMAP_ADDR         
>> (get_immrbase())
>>    only used drivers/net/fs_enet/fs_enet-main.c which seems evil.
>
> That driver *is* evil. :-)
>
> It looks like the only remaining user of fs_enet_immap (which is  
> what IMAP_ADDR is used to initialize) is in mac-fec.c, under  
> CONFIG_DUET -- which hasn't been touched since 2005 and appears to  
> have died with arch/ppc.
>
> I'm fine with removing it -- it probably no longer compiles anyway.

I'll send DaveM a bit to kill it off.

- k

Re: removing get_immrbase()??

[Scott Wood]

Scott Wood wrote:
>> arch/powerpc/include/asm/cpm2.h:#define CPM_MAP_ADDR (get_immrbase() + 
>> 0x80000)
>> arch/powerpc/sysdev/cpm2.c:    cpm2_immr = ioremap(get_immrbase(), 
>> CPM_MAP_SIZE);
>>     these two are related and seem like we could look for "fsl,cpm2"
> 
> And do what with it that wouldn't be a reimplementation of get_immrbase()?

Sorry, I missed that you're referring to the CPM node rather than the 
IMMR node.  The CPM node's address points specifically to some CPM 
control registers, not to the start of a CPM "region" of IMMR/CCSR -- it 
has an empty ranges property to bypass address translation.

I think this needs more careful untangling, and some new device tree 
nodes (sorry Timur) if we want to get rid of the magic offsets and huge 
multiple-block-spanning structures.  I'm not sure it's worth it given 
the microscopic odds of a new CPM2 chip coming out, unless it's part of 
a CPM/QE merge.

-Scott

Re: removing get_immrbase()??

[Arnd Bergmann]

[-- Attachment #1: Type: text/plain, Size: 1412 bytes --]

On Wednesday 22 April 2009, Kumar Gala wrote:

First of all, thanks for bringing this up, I'd love to see get_immrbase() gone.

> arch/powerpc/sysdev/cpm1.c:     mpc8xx_immr = ioremap(get_immrbase(),  
> 0x4000);
>         not sure? ideas?

Nobody has commented on this, so I've taken a brief look at it.

I'd suggest moving the logic up one step at a time. im_cpm, im_siu_conf and
im_ioport could be defined locally in sysdev/cpm1.c rather than through
mpc8xx_immr, all you need for this is to export accessor functions from cpm1 for
iop_pcso and cp_cptr:

void cpm1_set_iop_pcso(u16 pcso)
{
	setbits16(cpm1_ioport.iop_pcso, pcso);
}
void cpm1_clear_iop_pcso(u16 pcso)
{
	clearbits16(cpm1_ioport.iop_pcso, pcso);
}
...

im_sit, im_sitk, im_clkrst and im_clkrstk should be defined locally in m8xx_setup.c,
which is the only place that they are used in. Fortunately, the are all contiguous
in the address sapce, so they can be moved into one new data structure with 
a single static pointer to it in m8xx_setup.c:

static struct {
	struct­ sys_int_timers sit;
	struct clk_and_reset clkrst;
	struct sitk sitk;
	struct cark clkrstk;
} *m8xx_setup_regs;

When this is done, 8xx_immap.h along with all the unused stuff therein can be removed.
In the last step, the device trees can be cleaned up so that you can of_iomap
the regions in those two files directly.

	Arnd <><


[-- Attachment #2: Type: text/html, Size: 8243 bytes --]