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 robert@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 / frac) / div;
} @@ -152,11 +148,12 @@ void mx28_set_ioclk(enum mxs_ioclock io, uint32_t freq) struct mx28_clkctrl_regs *clkctrl_regs = (struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE; uint32_t div;
int io_reg;
if (freq == 0) return;
- if (io > MXC_IOCLK1)
if ((io < MXC_IOCLK0) || (io > MXC_IOCLK1)) return;
div = (PLL_FREQ_KHZ * PLL_FREQ_COEF) / freq;
@@ -167,23 +164,13 @@ void mx28_set_ioclk(enum mxs_ioclock io, uint32_t freq) if (div > 35) div = 35;
- 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.
- 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.
- writeb(CLKCTRL_FRAC0_CLKGATE,
&clkctrl_regs->hw_clkctrl_frac0_set[io_reg]);- writeb(CLKCTRL_FRAC0_CLKGATE | (div & CLKCTRL_FRAC0_FRAC_MASK),
&clkctrl_regs->hw_clkctrl_frac0[io_reg]);- writeb(CLKCTRL_FRAC0_CLKGATE,
&clkctrl_regs->hw_clkctrl_frac0_clr[io_reg]);}
/* @@ -193,19 +180,16 @@ static uint32_t mx28_get_ioclk(enum mxs_ioclock io) { struct mx28_clkctrl_regs *clkctrl_regs = (struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE;
- uint32_t tmp, ret;
- uint8_t ret;
- int io_reg;
- if (io > MXC_IOCLK1)
- if ((io < MXC_IOCLK0) || (io > MXC_IOCLK1)) return 0;
- tmp = readl(&clkctrl_regs->hw_clkctrl_frac0);
- io_reg = CLKCTRL_FRAC0_IO0 - (io - MXC_IOCLK0);
- if (io == MXC_IOCLK0)
ret = (tmp & CLKCTRL_FRAC0_IO0FRAC_MASK) >>CLKCTRL_FRAC0_IO0FRAC_OFFSET;- else
ret = (tmp & CLKCTRL_FRAC0_IO1FRAC_MASK) >>CLKCTRL_FRAC0_IO1FRAC_OFFSET;
ret = readb(&clkctrl_regs->hw_clkctrl_frac0[io_reg]) &
CLKCTRL_FRAC0_FRAC_MASK;return (PLL_FREQ_KHZ * PLL_FREQ_COEF) / ret;
} @@ -223,7 +207,7 @@ void mx28_set_sspclk(enum mxs_sspclock ssp, uint32_t freq, int xtal) return;
clkreg = (uint32_t)(&clkctrl_regs->hw_clkctrl_ssp0) +
(ssp * sizeof(struct mx28_register));
(ssp * sizeof(struct mx28_register_32));clrbits_le32(clkreg, CLKCTRL_SSP_CLKGATE); while (readl(clkreg) & CLKCTRL_SSP_CLKGATE)
@@ -272,7 +256,7 @@ static uint32_t mx28_get_sspclk(enum mxs_sspclock ssp) return XTAL_FREQ_KHZ;
clkreg = (uint32_t)(&clkctrl_regs->hw_clkctrl_ssp0) +
(ssp * sizeof(struct mx28_register));
(ssp * sizeof(struct mx28_register_32));tmp = readl(clkreg) & CLKCTRL_SSP_DIV_MASK;
diff --git a/arch/arm/cpu/arm926ejs/mx28/iomux.c b/arch/arm/cpu/arm926ejs/mx28/iomux.c index 9ea411f..12916b6 100644 --- a/arch/arm/cpu/arm926ejs/mx28/iomux.c +++ b/arch/arm/cpu/arm926ejs/mx28/iomux.c @@ -43,7 +43,7 @@ int mxs_iomux_setup_pad(iomux_cfg_t pad) { u32 reg, ofs, bp, bm; void *iomux_base = (void *)MXS_PINCTRL_BASE;
- struct mx28_register *mxs_reg;
struct mx28_register_32 *mxs_reg;
/* muxsel */ ofs = 0x100;
@@ -70,7 +70,7 @@ int mxs_iomux_setup_pad(iomux_cfg_t pad) /* vol */ if (PAD_VOL_VALID(pad)) { bp = PAD_PIN(pad) % 8 * 4 + 2;
mxs_reg = (struct mx28_register *)(iomux_base + ofs);
mxs_reg = (struct mx28_register_32 *)(iomux_base + ofs);@@ -82,7 +82,7 @@ int mxs_iomux_setup_pad(iomux_cfg_t pad) ofs = PULL_OFFSET; ofs += PAD_BANK(pad) * 0x10; bp = PAD_PIN(pad);
mxs_reg = (struct mx28_register *)(iomux_base + ofs);
mxs_reg = (struct mx28_register_32 *)(iomux_base + ofs);diff --git a/arch/arm/cpu/arm926ejs/mx28/mx28.c b/arch/arm/cpu/arm926ejs/mx28/mx28.c index 683777f..0e69193 100644 --- a/arch/arm/cpu/arm926ejs/mx28/mx28.c +++ b/arch/arm/cpu/arm926ejs/mx28/mx28.c @@ -63,7 +63,7 @@ void reset_cpu(ulong ignored) ; }
-int mx28_wait_mask_set(struct mx28_register *reg, uint32_t mask, int timeout) +int mx28_wait_mask_set(struct mx28_register_32 *reg, uint32_t
Can you actually separate out the rename to register_32 and then the fixup patch for register_8?
Rest seems ok
M
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.
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.
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.
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.
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
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.
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
participants (2)
-
Marek Vasut -
Robert Deliën