[U-Boot] Potential issue with recent OMAP PRCM struct unification
Greetings,
I think http://patchwork.ozlabs.org/patch/218063/ or something related to it has confused OMAP4 clock init. I haven't entirely unraveled the onion but wanted to ping the list to see if this is known or I'm off in the weeds.
Using u-boot master at commit a268170 in DEBUG mode the SPL says (in part):
Enable clock module - 4a008e20 Enable clock module - 4a008e28 Enable clock module - 4a008e40 << later builds omit Enable clock module - 4a009338 << these two clocks Enable clock module - 4a004528 Enable clock module - 4a004530 Enable clock module - 4a004538
That build works. But by commit 417c558 the 2 clocks noted are omitted and the later call to get_ram_size() hangs.
In tracing this I found that the prcm structure initializes one field (cm_l3instr_intrconn_wp1_clkct) but then enable_non_essential_clocks() passes an array populated using a different field (cm_l3instr_intrconn_wp1_clkctrl) to do_enable_clocks(). That latter uninitialized field is zero which terminates that clock init array and results in the two clock omissions above.
Searching for this and leaving out omap5 for clarity I see:
cashwell.ubuntu:u-boot$ rgrep cm_l3instr_intrconn_wp1_clkct | grep -v omap5 ./arch/arm/cpu/armv7/omap4/prcm-regs.c: .cm_l3instr_intrconn_wp1_clkct = 0x4a008e40, ./arch/arm/cpu/armv7/omap4/hw_data.c: (*prcm)->cm_l3instr_intrconn_wp1_clkctrl, ./arch/arm/include/asm/omap_common.h: u32 cm_l3instr_intrconn_wp1_clkctrl; ./arch/arm/include/asm/omap_common.h: u32 cm_l3instr_intrconn_wp1_clkct;
On first blush, it looks like having both cm_l3instr_intrconn_wp1_clkct and cm_l3instr_intrconn_wp1_clkctrl is a mistake.
If that's true can anyone say which should be eliminated and whether or not the order of fields in struct prcm_regs matters?
Thanks, -Mike Cashwell
Hi Mike Cashwell,
On Monday 01 April 2013 09:12 PM, Michael Cashwell wrote:
Greetings,
I think http://patchwork.ozlabs.org/patch/218063/ or something related to it has confused OMAP4 clock init. I haven't entirely unraveled the onion but wanted to ping the list to see if this is known or I'm off in the weeds.
Using u-boot master at commit a268170 in DEBUG mode the SPL says (in part):
Enable clock module - 4a008e20 Enable clock module - 4a008e28 Enable clock module - 4a008e40 << later builds omit Enable clock module - 4a009338 << these two clocks Enable clock module - 4a004528 Enable clock module - 4a004530 Enable clock module - 4a004538
That build works. But by commit 417c558 the 2 clocks noted are omitted and the later call to get_ram_size() hangs.
In tracing this I found that the prcm structure initializes one field (cm_l3instr_intrconn_wp1_clkct) but then enable_non_essential_clocks() passes an array populated using a different field (cm_l3instr_intrconn_wp1_clkctrl) to do_enable_clocks(). That latter uninitialized field is zero which terminates that clock init array and results in the two clock omissions above.
Searching for this and leaving out omap5 for clarity I see:
cashwell.ubuntu:u-boot$ rgrep cm_l3instr_intrconn_wp1_clkct | grep -v omap5 ./arch/arm/cpu/armv7/omap4/prcm-regs.c: .cm_l3instr_intrconn_wp1_clkct = 0x4a008e40, ./arch/arm/cpu/armv7/omap4/hw_data.c: (*prcm)->cm_l3instr_intrconn_wp1_clkctrl, ./arch/arm/include/asm/omap_common.h: u32 cm_l3instr_intrconn_wp1_clkctrl; ./arch/arm/include/asm/omap_common.h: u32 cm_l3instr_intrconn_wp1_clkct;
On first blush, it looks like having both cm_l3instr_intrconn_wp1_clkct and cm_l3instr_intrconn_wp1_clkctrl is a mistake.
If that's true can anyone say which should be eliminated and whether or not the order of fields in struct prcm_regs matters?
First, on which board are you testing ?. I tested the mainline on my 4460 ES1.1 PANDA and it booted. Also why are you enabling the non-essential clocks ? Now enabling non-essential clocks is deprecated and they are **not** by enabled by default. As you said the unnecessary entry in omap_common.h should be removed and typo in prcm-regs.c I can correct this, but does correcting this gets you working again ? Enabling these two clocks should have nothing to do with boot.
Regards, Sricharan
On Apr 2, 2013, at 5:32 AM, Sricharan R r.sricharan@ti.com wrote:
On first blush, it looks like having both cm_l3instr_intrconn_wp1_clkct and cm_l3instr_intrconn_wp1_clkctrl is a mistake.
First, on which board are you testing ?. I tested the mainline on my 4460 ES1.1 PANDA and it booted.
One custom board still under development and also on the same Pandaboard you have.
But further testing showed that CONFIG_SYS_AUTOMATIC_SDRAM_DETECTION (and the related EMIF defines) stopped working somewhere between commits a268170 and 417c558. Returning to the pre-canned SDRAM modes allowed booting to work again.
The reason I suspected the clocks was that they were the *only* difference in the entire debug log between the working and hanging cases. I don't know why the SDRAM state difference which was the real cause did not produce even a single difference in the log.
As you said the unnecessary entry in omap_common.h should be removed and typo in prcm-regs.c I can correct this, but does correcting this gets you working again ? Enabling these two clocks should have nothing to do with boot.
Correct. As I later found, the clocks in question are non-essential for u-boot and were not causing the hang.
Also why are you enabling the non-essential clocks ?
Because I must be able to boot Linux kernels as far back as 3.0.8 which predates this paradigm shift.
Now enabling non-essential clocks is deprecated and they are **not** by enabled by default.
As a point of clarification, are you asserting that CONFIG_SYS_CLOCKS_ENABLE_ALL and CONFIG_SYS_ENABLE_PADS_ALL have been officially deprecated (e.g.: is planned for removal from u-boot)?
There is no mention of this anywhere within the source tree, including in any documentation or README and, IMO, it would be very premature given that at least 4 Linux kernel lines needing these inits are still within their longterm support window.
But clearly until such removal happens dropping any that were previously handled is a regression.
Thanks for the assistance!
-Mike
On Tuesday 02 April 2013 05:59 PM, Michael Cashwell wrote:
On Apr 2, 2013, at 5:32 AM, Sricharan R r.sricharan@ti.com wrote:
On first blush, it looks like having both cm_l3instr_intrconn_wp1_clkct and cm_l3instr_intrconn_wp1_clkctrl is a mistake.
First, on which board are you testing ?. I tested the mainline on my 4460 ES1.1 PANDA and it booted.
One custom board still under development and also on the same Pandaboard you have.
But further testing showed that CONFIG_SYS_AUTOMATIC_SDRAM_DETECTION (and the related EMIF defines) stopped working somewhere between commits a268170 and 417c558. Returning to the pre-canned SDRAM modes allowed booting to work again.
The reason I suspected the clocks was that they were the *only* difference in the entire debug log between the working and hanging cases. I don't know why the SDRAM state difference which was the real cause did not produce even a single difference in the log.
Yes, i think AUTOMATIC_SDRAM_DETECTION is broken. I am having some patches to fix and will post shortly. Sorry,that you hit this issue. But these were any not because of the PRCM changes, it was much before that.
As you said the unnecessary entry in omap_common.h should be removed and typo in prcm-regs.c I can correct this, but does correcting this gets you working again ? Enabling these two clocks should have nothing to do with boot.
Correct. As I later found, the clocks in question are non-essential for u-boot and were not causing the hang.
Also why are you enabling the non-essential clocks ?
Because I must be able to boot Linux kernels as far back as 3.0.8 which predates this paradigm shift.
Now enabling non-essential clocks is deprecated and they are **not** by enabled by default.
As a point of clarification, are you asserting that CONFIG_SYS_CLOCKS_ENABLE_ALL and CONFIG_SYS_ENABLE_PADS_ALL have been officially deprecated (e.g.: is planned for removal from u-boot)?
There is no mention of this anywhere within the source tree, including in any documentation or README and, IMO, it would be very premature given that at least 4 Linux kernel lines needing these inits are still within their longterm support window.
But clearly until such removal happens dropping any that were previously handled is a regression.
Thanks for the assistance!
Yes, thats why we still have kept it for testing. But now, there are already patches to fix this in the kernel being posted, and probably all of them should be fixed shortly. Once that is done, all of this can be removed.
Regards, Sricharan
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/02/2013 11:06 AM, Sricharan R wrote:
On Tuesday 02 April 2013 05:59 PM, Michael Cashwell wrote:
On Apr 2, 2013, at 5:32 AM, Sricharan R r.sricharan@ti.com wrote:
[snip]
Also why are you enabling the non-essential clocks ?
Because I must be able to boot Linux kernels as far back as 3.0.8 which predates this paradigm shift.
Now enabling non-essential clocks is deprecated and they are **not** by enabled by default.
As a point of clarification, are you asserting that CONFIG_SYS_CLOCKS_ENABLE_ALL and CONFIG_SYS_ENABLE_PADS_ALL have been officially deprecated (e.g.: is planned for removal from u-boot)?
There is no mention of this anywhere within the source tree, including in any documentation or README and, IMO, it would be very premature given that at least 4 Linux kernel lines needing these inits are still within their longterm support window.
But clearly until such removal happens dropping any that were previously handled is a regression.
Thanks for the assistance!
Yes, thats why we still have kept it for testing. But now, there are already patches to fix this in the kernel being posted, and probably all of them should be fixed shortly. Once that is done, all of this can be removed.
So, here's my 2 cents on this. We can't up and drop these options from U-Boot until there's a complete / viable kernel that doesn't need them. I'm _not_ saying we need to test every patchset vs an old kernel or anything, but we shouldn't intentionally make life harder on folks, until we can just pull the option all together (and say use a new kernel, or an older u-boot).
- -- Tom
On Tuesday 02 April 2013 08:47 PM, Tom Rini wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/02/2013 11:06 AM, Sricharan R wrote:
On Tuesday 02 April 2013 05:59 PM, Michael Cashwell wrote:
On Apr 2, 2013, at 5:32 AM, Sricharan R r.sricharan@ti.com wrote:
[snip]
Also why are you enabling the non-essential clocks ?
Because I must be able to boot Linux kernels as far back as 3.0.8 which predates this paradigm shift.
Now enabling non-essential clocks is deprecated and they are **not** by enabled by default.
As a point of clarification, are you asserting that CONFIG_SYS_CLOCKS_ENABLE_ALL and CONFIG_SYS_ENABLE_PADS_ALL have been officially deprecated (e.g.: is planned for removal from u-boot)?
There is no mention of this anywhere within the source tree, including in any documentation or README and, IMO, it would be very premature given that at least 4 Linux kernel lines needing these inits are still within their longterm support window.
But clearly until such removal happens dropping any that were previously handled is a regression.
Thanks for the assistance!
Yes, thats why we still have kept it for testing. But now, there are already patches to fix this in the kernel being posted, and probably all of them should be fixed shortly. Once that is done, all of this can be removed.
So, here's my 2 cents on this. We can't up and drop these options from U-Boot until there's a complete / viable kernel tthhat doesn't need them. I'm _not_ saying we need to test every patchset vs an old kernel or anything, but we shouldn't intentionally make life harder on folks, until we can just pull the option all together (and say use a new kernel, or an older u-boot).
Hmm, Agree this should not be broken unintentionally. But because we purposefully deprecated this, kernel is now getting fixed. Fixing any thing towards this deprecated one, will again introduce the luxury of not addressing in kernel, which is not good. If we propose of removing this in U-BOOT after every thing is fixed in kernel, we still will have of need of supporting for older kernels..
Regards, Sricharan
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/02/2013 11:55 AM, Sricharan R wrote:
On Tuesday 02 April 2013 08:47 PM, Tom Rini wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/02/2013 11:06 AM, Sricharan R wrote:
On Tuesday 02 April 2013 05:59 PM, Michael Cashwell wrote:
On Apr 2, 2013, at 5:32 AM, Sricharan R r.sricharan@ti.com wrote:
[snip]
Also why are you enabling the non-essential clocks ?
Because I must be able to boot Linux kernels as far back as 3.0.8 which predates this paradigm shift.
Now enabling non-essential clocks is deprecated and they are **not** by enabled by default.
As a point of clarification, are you asserting that CONFIG_SYS_CLOCKS_ENABLE_ALL and CONFIG_SYS_ENABLE_PADS_ALL have been officially deprecated (e.g.: is planned for removal from u-boot)?
There is no mention of this anywhere within the source tree, including in any documentation or README and, IMO, it would be very premature given that at least 4 Linux kernel lines needing these inits are still within their longterm support window.
But clearly until such removal happens dropping any that were previously handled is a regression.
Thanks for the assistance!
Yes, thats why we still have kept it for testing. But now, there are already patches to fix this in the kernel being posted, and probably all of them should be fixed shortly. Once that is done, all of this can be removed.
So, here's my 2 cents on this. We can't up and drop these options from U-Boot until there's a complete / viable kernel tthhat doesn't need them. I'm _not_ saying we need to test every patchset vs an old kernel or anything, but we shouldn't intentionally make life harder on folks, until we can just pull the option all together (and say use a new kernel, or an older u-boot).
Hmm, Agree this should not be broken unintentionally. But because we purposefully deprecated this, kernel is now getting fixed. Fixing any thing towards this deprecated one, will again introduce the luxury of not addressing in kernel, which is not good. If we propose of removing this in U-BOOT after every thing is fixed in kernel, we still will have of need of supporting for older kernels..
Yes, I'm assuming the kernel folks to continue with adding clocks they need in the right places now that the main event has happened and we aren't enabling more things until / unless we need them. And since I think that's going at reasonable speed, I don't think we need to draw a dated line in the sand, just one that says we shall remove the option, once a reasonable (read: most IO works) kernel tree is available that doesn't need this, we can remove it. Maybe we can set a hope to remove date? How about v2013.07?
- -- Tom
On Tuesday 02 April 2013 10:12 PM, Tom Rini wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/02/2013 11:55 AM, Sricharan R wrote:
On Tuesday 02 April 2013 08:47 PM, Tom Rini wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/02/2013 11:06 AM, Sricharan R wrote:
On Tuesday 02 April 2013 05:59 PM, Michael Cashwell wrote:
On Apr 2, 2013, at 5:32 AM, Sricharan R r.sricharan@ti.com wrote:
[snip]
Also why are you enabling the non-essential clocks ?
Because I must be able to boot Linux kernels as far back as 3.0.8 which predates this paradigm shift.
Now enabling non-essential clocks is deprecated and they are **not** by enabled by default.
As a point of clarification, are you asserting that CONFIG_SYS_CLOCKS_ENABLE_ALL and CONFIG_SYS_ENABLE_PADS_ALL have been officially deprecated (e.g.: is planned for removal from u-boot)?
There is no mention of this anywhere within the source tree, including in any documentation or README and, IMO, it would be very premature given that at least 4 Linux kernel lines needing these inits are still within their longterm support window.
But clearly until such removal happens dropping any that were previously handled is a regression.
Thanks for the assistance!
Yes, thats why we still have kept it for testing. But now, there are already patches to fix this in the kernel being posted, and probably all of them should be fixed shortly. Once that is done, all of this can be removed.
So, here's my 2 cents on this. We can't up and drop these options from U-Boot until there's a complete / viable kernel tthhat doesn't need them. I'm _not_ saying we need to test every patchset vs an old kernel or anything, but we shouldn't intentionally make life harder on folks, until we can just pull the option all together (and say use a new kernel, or an older u-boot).
Hmm, Agree this should not be broken unintentionally. But because we purposefully deprecated this, kernel is now getting fixed. Fixing any thing towards this deprecated one, will again introduce the luxury of not addressing in kernel, which is not good. If we propose of removing this in U-BOOT after every thing is fixed in kernel, we still will have of need of supporting for older kernels..
Yes, I'm assuming the kernel folks to continue with adding clocks they need in the right places now that the main event has happened and we aren't enabling more things until / unless we need them. And since I think that's going at reasonable speed, I don't think we need to draw a dated line in the sand, just one that says we shall remove the option, once a reasonable (read: most IO works) kernel tree is available that doesn't need this, we can remove it. Maybe we can set a hope to remove date? How about v2013.07?
Yes, sounds good. Hopefully kernel fixed by then
Regards, Sricharan
participants (3)
-
Michael Cashwell -
Sricharan R -
Tom Rini