Re: [U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup
Hi, This patch fixes ref_cpu clock setup. This bug leads to a hanging board after rebooting from the Kernel, due to failing memory size detection: U-Boot 2011.12-svn342 (Feb 02 2012 - 17:20:00) Freescale i.MX28 family I2C: ready DRAM: 0 Bytes The cause of the bug is register hw_clkctrl_frac0 being accessed as a 32-bit long, whereas the manual specifically states it can be accessed as bytes only. This patches introduces an 8-bit wide register type, mx28_register_8. The already existing mx28_register has been renamed mx28_register_32. With this patch, U-Boot no longer hangs after an i.mx28 based board was reset from the Kernel. (PS: I hope this email is properly formatted now, after fight our exchange server for a whole morning and loosing in the end) Signed-off-by: Robert Delien rob...@delien.nl --- arch/arm/cpu/arm926ejs/mx28/clock.c | 74 +++- arch/arm/cpu/arm926ejs/mx28/iomux.c |6 +- arch/arm/cpu/arm926ejs/mx28/mx28.c|6 +- arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c| 23 +-- arch/arm/include/asm/arch-mx28/regs-apbh.h| 254 arch/arm/include/asm/arch-mx28/regs-bch.h | 42 ++-- arch/arm/include/asm/arch-mx28/regs-clkctrl.h | 101 +-- arch/arm/include/asm/arch-mx28/regs-common.h | 28 ++- arch/arm/include/asm/arch-mx28/regs-gpmi.h| 26 ++-- arch/arm/include/asm/arch-mx28/regs-i2c.h | 28 ++-- arch/arm/include/asm/arch-mx28/regs-ocotp.h | 86 arch/arm/include/asm/arch-mx28/regs-pinctrl.h | 168 arch/arm/include/asm/arch-mx28/regs-power.h | 28 ++-- arch/arm/include/asm/arch-mx28/regs-rtc.h | 28 ++-- arch/arm/include/asm/arch-mx28/regs-ssp.h | 40 ++-- arch/arm/include/asm/arch-mx28/regs-timrot.h | 38 ++-- arch/arm/include/asm/arch-mx28/regs-usbphy.h | 20 +- arch/arm/include/asm/arch-mx28/sys_proto.h|6 +- drivers/gpio/mxs_gpio.c | 16 +- drivers/usb/host/ehci-mxs.c |8 +- 20 files changed, 505 insertions(+), 521 deletions(-) diff --git a/arch/arm/cpu/arm926ejs/mx28/clock.c b/arch/arm/cpu/arm926ejs/mx28/clock.c index f698506..c0eea9e 100644 --- a/arch/arm/cpu/arm926ejs/mx28/clock.c +++ b/arch/arm/cpu/arm926ejs/mx28/clock.c @@ -46,8 +46,8 @@ static uint32_t mx28_get_pclk(void) struct mx28_clkctrl_regs *clkctrl_regs = (struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE; - uint32_t clkctrl, clkseq, clkfrac; - uint32_t frac, div; + uint32_t clkctrl, clkseq, div; + uint8_t clkfrac, frac; clkctrl = readl(clkctrl_regs-hw_clkctrl_cpu); @@ -67,8 +67,8 @@ static uint32_t mx28_get_pclk(void) } /* REF Path */ - clkfrac = readl(clkctrl_regs-hw_clkctrl_frac0); - frac = clkfrac CLKCTRL_FRAC0_CPUFRAC_MASK; + clkfrac = readb(clkctrl_regs-hw_clkctrl_frac0[CLKCTRL_FRAC0_CPU]); + frac = clkfrac CLKCTRL_FRAC0_FRAC_MASK; div = clkctrl CLKCTRL_CPU_DIV_CPU_MASK; return (PLL_FREQ_MHZ * PLL_FREQ_COEF / frac) / div; } @@ -96,8 +96,8 @@ static uint32_t mx28_get_emiclk(void) struct mx28_clkctrl_regs *clkctrl_regs = (struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE; - uint32_t frac, div; - uint32_t clkctrl, clkseq, clkfrac; + uint32_t clkctrl, clkseq, div; + uint8_t clkfrac, frac; clkseq = readl(clkctrl_regs-hw_clkctrl_clkseq); clkctrl = readl(clkctrl_regs-hw_clkctrl_emi); @@ -109,11 +109,9 @@ static uint32_t mx28_get_emiclk(void) return XTAL_FREQ_MHZ / div; } - clkfrac = readl(clkctrl_regs-hw_clkctrl_frac0); - /* REF Path */ - frac = (clkfrac CLKCTRL_FRAC0_EMIFRAC_MASK) - CLKCTRL_FRAC0_EMIFRAC_OFFSET; + clkfrac = readb(clkctrl_regs-hw_clkctrl_frac0[CLKCTRL_FRAC0_EMI]); + frac = clkfrac CLKCTRL_FRAC0_FRAC_MASK; div = clkctrl CLKCTRL_EMI_DIV_EMI_MASK; return (PLL_FREQ_MHZ * PLL_FREQ_COEF / frac) / div; } @@ -123,8 +121,8 @@ static uint32_t mx28_get_gpmiclk(void) struct mx28_clkctrl_regs *clkctrl_regs = (struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE; - uint32_t frac, div; - uint32_t clkctrl, clkseq, clkfrac; + uint32_t clkctrl, clkseq, div; + uint8_t clkfrac, frac; clkseq = readl(clkctrl_regs-hw_clkctrl_clkseq); clkctrl = readl(clkctrl_regs-hw_clkctrl_gpmi); @@ -135,11 +133,9 @@ static uint32_t mx28_get_gpmiclk(void) return XTAL_FREQ_MHZ / div; } - clkfrac = readl(clkctrl_regs-hw_clkctrl_frac1); - /* REF Path */ - frac = (clkfrac CLKCTRL_FRAC1_GPMIFRAC_MASK) - CLKCTRL_FRAC1_GPMIFRAC_OFFSET; + clkfrac = readb(clkctrl_regs-hw_clkctrl_frac1[CLKCTRL_FRAC1_GPMI]); + frac = clkfrac CLKCTRL_FRAC1_FRAC_MASK; div = clkctrl CLKCTRL_GPMI_DIV_MASK; return (PLL_FREQ_MHZ * PLL_FREQ_COEF /
Re: [U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup
Hi, This patch fixes ref_cpu clock setup. This bug leads to a hanging board after rebooting from the Kernel, due to failing memory size detection: U-Boot 2011.12-svn342 (Feb 02 2012 - 17:20:00) Freescale i.MX28 family I2C: ready DRAM: 0 Bytes The cause of the bug is register hw_clkctrl_frac0 being accessed as a 32-bit long, whereas the manual specifically states it can be accessed as bytes only. This patches introduces an 8-bit wide register type, mx28_register_8. The already existing mx28_register has been renamed mx28_register_32. With this patch, U-Boot no longer hangs after an i.mx28 based board was reset from the Kernel. (PS: I hope this email is properly formatted now, after fight our exchange server for a whole morning and loosing in the end) btw 2/2 is missing? M ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup
Hi Marek,
- if (io == MXC_IOCLK0) {
- writel(CLKCTRL_FRAC0_CLKGATEIO0,
- clkctrl_regs-hw_clkctrl_frac0_set);
- clrsetbits_le32(clkctrl_regs-hw_clkctrl_frac0,
- CLKCTRL_FRAC0_IO0FRAC_MASK,
- div CLKCTRL_FRAC0_IO0FRAC_OFFSET);
- writel(CLKCTRL_FRAC0_CLKGATEIO0,
- clkctrl_regs-hw_clkctrl_frac0_clr);
- } else {
- writel(CLKCTRL_FRAC0_CLKGATEIO1,
- clkctrl_regs-hw_clkctrl_frac0_set);
- clrsetbits_le32(clkctrl_regs-hw_clkctrl_frac0,
- CLKCTRL_FRAC0_IO1FRAC_MASK,
- div CLKCTRL_FRAC0_IO1FRAC_OFFSET);
- writel(CLKCTRL_FRAC0_CLKGATEIO1,
- clkctrl_regs-hw_clkctrl_frac0_clr);
- }
I think you're mixing two things together above. This patch and some kind of a
cleanup. But ok, thinking of this, it seems context related.
Yeah, duplicate code didn't feel right. Besides, the code didn't check for
values
larger than MXC_IOCLK0, which is does now.
+ io_reg = CLKCTRL_FRAC0_IO0 - (io - MXC_IOCLK0);
Uh ... ioreg = (io == MXC_IOCLK0) ? something : another; or stuff like that
might be better. The math above is really confusing. Or you can even enumerate
enum mxs_ioclock so that you won't need this math at all, which is even
better.
I came accross an enumerator in the code, but I found that too big of a change
to slip in, because it alters the external interface. But we're not done with
the code
yet. Plenty of oppertunities left.
ioreg = (io == MXC_IOCLK0) is not the same. The actual value of
io != MXC_IOCLK0 it compiler implementation depending.
Can you actually separate out the rename to register_32 and then the fixup
patch
for register_8?
I'd rather not: I'm down to my neck it work and I don't these two parts in my
archive
so that means manual merging, verifying and for another two hours.
Rest seems ok
Can you see if our server swapped tabs for spaces? It's beyond my grasp how M$
considers it a good idea to alter to contents of my email.
Cheers,
Robert.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup
btw 2/2 is missing? Not missing, just not sent again; It hasn't been changed. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup
Hi Marek,
- if (io == MXC_IOCLK0) {
- writel(CLKCTRL_FRAC0_CLKGATEIO0,
- clkctrl_regs-hw_clkctrl_frac0_set);
- clrsetbits_le32(clkctrl_regs-hw_clkctrl_frac0,
- CLKCTRL_FRAC0_IO0FRAC_MASK,
- div CLKCTRL_FRAC0_IO0FRAC_OFFSET);
- writel(CLKCTRL_FRAC0_CLKGATEIO0,
- clkctrl_regs-hw_clkctrl_frac0_clr);
- } else {
- writel(CLKCTRL_FRAC0_CLKGATEIO1,
- clkctrl_regs-hw_clkctrl_frac0_set);
- clrsetbits_le32(clkctrl_regs-hw_clkctrl_frac0,
- CLKCTRL_FRAC0_IO1FRAC_MASK,
- div CLKCTRL_FRAC0_IO1FRAC_OFFSET);
- writel(CLKCTRL_FRAC0_CLKGATEIO1,
- clkctrl_regs-hw_clkctrl_frac0_clr);
- }
I think you're mixing two things together above. This patch and some kind
of a cleanup. But ok, thinking of this, it seems context related.
Yeah, duplicate code didn't feel right. Besides, the code didn't check for
values larger than MXC_IOCLK0, which is does now.
+ io_reg = CLKCTRL_FRAC0_IO0 - (io - MXC_IOCLK0);
Uh ... ioreg = (io == MXC_IOCLK0) ? something : another; or stuff like
that might be better. The math above is really confusing. Or you can
even enumerate enum mxs_ioclock so that you won't need this math at all,
which is even better.
I came accross an enumerator in the code, but I found that too big of a
change to slip in, because it alters the external interface. But we're not
done with the code yet. Plenty of oppertunities left.
Not really, fixing the enumeration should be fine I believe.
ioreg = (io == MXC_IOCLK0) is not the same. The actual value of
io != MXC_IOCLK0 it compiler implementation depending.
That's why I put the ternary operator there ?
Can you actually separate out the rename to register_32 and then the
fixup patch for register_8?
I'd rather not: I'm down to my neck it work and I don't these two parts in
my archive so that means manual merging, verifying and for another two
hours.
It's impossible to review like this though and much more prone to pull in bugs.
Rest seems ok
Can you see if our server swapped tabs for spaces? It's beyond my grasp how
M$ considers it a good idea to alter to contents of my email.
It didnt.
Cheers,
Robert.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup
btw 2/2 is missing? Not missing, just not sent again; It hasn't been changed. Uh ... I don't see this patch in in-reply-to either. So I can't find it, really. M ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup
Hi Marek,
- if (io == MXC_IOCLK0) {
- writel(CLKCTRL_FRAC0_CLKGATEIO0,
- clkctrl_regs-hw_clkctrl_frac0_set);
- clrsetbits_le32(clkctrl_regs-hw_clkctrl_frac0,
- CLKCTRL_FRAC0_IO0FRAC_MASK,
- div CLKCTRL_FRAC0_IO0FRAC_OFFSET);
- writel(CLKCTRL_FRAC0_CLKGATEIO0,
- clkctrl_regs-hw_clkctrl_frac0_clr);
- } else {
- writel(CLKCTRL_FRAC0_CLKGATEIO1,
- clkctrl_regs-hw_clkctrl_frac0_set);
- clrsetbits_le32(clkctrl_regs-hw_clkctrl_frac0,
- CLKCTRL_FRAC0_IO1FRAC_MASK,
- div CLKCTRL_FRAC0_IO1FRAC_OFFSET);
- writel(CLKCTRL_FRAC0_CLKGATEIO1,
- clkctrl_regs-hw_clkctrl_frac0_clr);
- }
I think you're mixing two things together above. This patch and some kind
of a cleanup. But ok, thinking of this, it seems context related.
Yeah, duplicate code didn't feel right. Besides, the code didn't check for
values larger than MXC_IOCLK0, which is does now.
+ io_reg = CLKCTRL_FRAC0_IO0 - (io - MXC_IOCLK0);
Uh ... ioreg = (io == MXC_IOCLK0) ? something : another; or stuff like
that might be better. The math above is really confusing. Or you can
even enumerate enum mxs_ioclock so that you won't need this math at all,
which is even better.
I came accross an enumerator in the code, but I found that too big of a
change to slip in, because it alters the external interface. But we're not
done with the code yet. Plenty of oppertunities left.
ioreg = (io == MXC_IOCLK0) is not the same. The actual value of
io != MXC_IOCLK0 it compiler implementation depending.
Can you actually separate out the rename to register_32 and then the
fixup patch for register_8?
I'd rather not: I'm down to my neck it work and I don't these two parts in
my archive so that means manual merging, verifying and for another two
hours.
Rest seems ok
Can you see if our server swapped tabs for spaces? It's beyond my grasp how
M$ considers it a good idea to alter to contents of my email.
Cheers,
Robert.
btw. a quick hint:
git reset HEAD^
git add -p
Add only the reg32 changes, commit, add the rest, commit. Send ;-)
M
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup
Hi, That's why I put the ternary operator there ? Ah; without anything behind it, it really just looked like a question mark. But using a ternary caters only for a set of two. So enumeration would be better ideed. It's impossible to review like this though and much more prone to pull in bugs. Can you point me to a good and tutorial for GIT then? Just a brief description of the work flow, from cloning the public archive, to submitting patches with a couple of examples. Now, for every bit of rework, I clone a new archive and manually patch in all my changes to make a clean patch. That is two hours of work and I'm not willing to spend that time on every remark. Cheers, Robert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup
Hi, That's why I put the ternary operator there ? Ah; without anything behind it, it really just looked like a question mark. But using a ternary caters only for a set of two. So enumeration would be better ideed. It's impossible to review like this though and much more prone to pull in bugs. Can you point me to a good and tutorial for GIT then? Just a brief description of the work flow, from cloning the public archive, to submitting patches with a couple of examples. I just sent you an howto in a subsequent mail. Now, for every bit of rework, I clone a new archive and manually patch in all my changes to make a clean patch. No, it's really simple, see the email. That is two hours of work and I'm not willing to spend that time on every remark. Yep ... basically, you need to learn: git rebase -i (select patches you want to edit, reword ...) git commit --amend (add stuff to top-of-head patch) And the stuff I sent you -- git reset to reset changes from the index, git add -p to add changes selectively. M Cheers, Robert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup
btw. a quick hint: git reset HEAD^ git add -p Add only the reg32 changes, commit, add the rest, commit. Send ;-) Well, I made it a little more work than that. But who would have thought? GIT is actually growing on me! I've got my changes in my repository now, in 4 separate commits. I've even found an SMTP server on the network here. So we're looking good. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup
btw. a quick hint: git reset HEAD^ git add -p Add only the reg32 changes, commit, add the rest, commit. Send ;-) Well, I made it a little more work than that. But who would have thought? GIT is actually growing on me! I've got my changes in my repository now, in 4 separate commits. I've even found an SMTP server on the network here. So we're looking good. Yes, finally run git format-patch -o somewhere and tools/checkpatch.pl on the patches, fix the remaining trouble and git send-email ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
