Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
On Thu, Apr 23, 2009 at 07:53:11AM -0600, Grant Likely wrote: On Wed, Apr 22, 2009 at 3:31 PM, Kumar Gala ga...@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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
RE: removing get_immrbase()??
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. 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
On Wed, Apr 22, 2009 at 3:31 PM, Kumar Gala ga...@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. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
On Wed, Apr 22, 2009 at 3:39 PM, Kumar Gala ga...@kernel.crashing.org wrote: 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. Ah... yes, you're right. Never mind my previous reply. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
On Thu, Apr 23, 2009 at 07:53:11AM -0600, Grant Likely wrote: On Wed, Apr 22, 2009 at 3:31 PM, Kumar Gala ga...@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: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 /soc8...@... 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 /soc8...@... and X2 looks for /s...@..., 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: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 /soc8...@... and X2 looks for /s...@..., 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
On Wed, Apr 22, 2009 at 1:38 PM, Kumar Gala ga...@kernel.crashing.org 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: 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() + 0x8) 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. 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. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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() + 0x8) 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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() + 0x8) 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: removing get_immrbase()??
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
