[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Fabio Estevam]

Hi Robert,

On 2/2/12, Robert Deli?n <robert@delien.nl> wrote:
> 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.
> Applying this patch fixes this problem.
>
> Signed-off-by: Robert Delien (robert at delien.nl)

Very good, Robert! I tested your patch and it fixes the reboot issue
on my mx28evk.

Also checked in the MX28 Reference Manual about the fact that
hw_clkctrl_frac0 can only be accessed as bytes.

I have some suggestions though:

1. Your patch comes as attachment. Please use git send-email instead.

2. Please grep the locations where hw_clkctrl_frac0 is assigned as
32-bit and change those as well.

3. Send the two patches in a series via git send-email: 1/2 and 2/2

Good to know that you fixed the stepping issue as well. Good work!

Regards,

Fabio Estevam

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Fabio Estevam]

On 2/2/12, Fabio Estevam <festevam@gmail.com> wrote:

> 2. Please grep the locations where hw_clkctrl_frac0 is assigned as
> 32-bit and change those as well.

Please do the same for hw_clkctrl_frac1 as well. There is one location
in clock.c where it is read as 32-bit.

Thanks,

Fabio Estevam

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Robert Deliën]

> > 2. Please grep the locations where hw_clkctrl_frac0 is assigned as
> > 32-bit and change those as well.
> 
> Please do the same for hw_clkctrl_frac1 as well. There is one location
> in clock.c where it is read as 32-bit.

I will check all hw_clkctrl_frac* register access, but tomorrow because
the building closes is a couple of minutes.

Ideally I'd like to set up these registers as uint8_t*. Any ideas? I'll also
check if bye-lane shifting is done automatically here.

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Fabio Estevam]

On 2/2/12, Robert Deli?n <robert@delien.nl> wrote:

> Ideally I'd like to set up these registers as uint8_t*. Any ideas? I'll also
> check if bye-lane shifting is done automatically here.

You can write:
writeb(19 , &clkctrl_regs->hw_clkctrl_frac0);

Regards,

Fabio Estevam

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Robert Deliën]

> You can write:
> writeb(19 , &clkctrl_regs->hw_clkctrl_frac0);

Yes, for the first byte I can (and do), but the three higher bytes
would get ugly with this method:
writeb(bitset >> 8, (unit8_t*)&clkctrl_regs->hw_clkctrl_frac0 + 1);

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Robert Deliën]

Hi Fabio,

> Very good, Robert! I tested your patch and it fixes the reboot issue
> on my mx28evk.

You're most welcome! I'm glad to hear it fixes your problem too.

> Also checked in the MX28 Reference Manual about the fact that
> hw_clkctrl_frac0 can only be accessed as bytes.

It's easy to overlook, I have to admit.

> I have some suggestions though:
> 
> 1. Your patch comes as attachment. Please use git send-email instead.

I knew I was violating protocol, but I wanted to get it off my chest. I will
work on this problem today. I work at two locations, and only at my home
office I have an SMTP server I can user. Here at Agilent, unfortunately I
only have an MS Exchange server I can use. If I can get that configured
at all, there's a good chance it will break my patches.

> 2. Please grep the locations where hw_clkctrl_frac0 is assigned as
> 32-bit and change those as well.

Will do!

> 3. Send the two patches in a series via git send-email: 1/2 and 2/2

Will do! I'm reading up on that right now, as Wolfgang suggested
earlier. I may as well switch our own internal repository from SVN
to GIT, but not today.

> Good to know that you fixed the stepping issue as well. Good work!

Thanks. I would really like to urge you guys to test the second patch
as well. Marek made a remark on that too, so that will be my next mail
to answer.

Cheers,

        Robert.

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Marek Vasut]

> 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.
> Applying this patch fixes this problem.

Hi,

you're only writing data to the register, not clearing them. So maybe some bits 
remain set?

Anyway, can you please submit proper patch with git send-email? Thanks!

M

> 
> Signed-off-by: Robert Delien (robert at delien.nl)
> 
> ________________________________________
> From: Marek Vasut [marek.vasut at gmail.com]
> Sent: 26 January 2012 19:32
> To: Fabio Estevam
> Cc: Robert Deli?n; u-boot at lists.denx.de
> Subject: Re: mx28 spl power cpu clock configuration
> 
> > Hi Robert,
> > 
> > On 1/25/12, Marek Vasut <marek.vasut@gmail.com> wrote:
> > >> Shouldn't we configure clkctrl_frac0 - or at least disable CPU clock
> > >> gating - before disabling PLL bypass?
> > > 
> > > This seems reasonable. Fabio, can you comment?
> > 
> > Could you please post a patch with your proposed change so that we can
> > test it?
> 
> Hi Fabio,
> 
> I bought a really crappy custom board a few days ago (some china-made crap)
> sporting mx287, but apparently I'm hitting similar issue you do here.
> 
> When I swap power_init and mem_init though, the board boots fine, othervise
> it hangs.
> 
> M

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Robert Deliën]

Hi,

> you're only writing data to the register, not clearing them. So maybe some bits
> remain set?

The assignment will work here. There're only 8 bits, of which 6 are the fractional
divider, one is de read-only stable indicator (unaffected by writes) and the last
one is the gating bit.
The assignment configures the fractional divider and clears the gating.

> Anyway, can you please submit proper patch with git send-email? Thanks!

Will do; Working on that! And you're welcome.

Cheers,

        Robert.1

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Marek Vasut]

> Hi,
> 
> > you're only writing data to the register, not clearing them. So maybe
> > some bits remain set?
> 
> The assignment will work here. There're only 8 bits, of which 6 are the
> fractional divider, one is de read-only stable indicator (unaffected by
> writes) and the last one is the gating bit.
> The assignment configures the fractional divider and clears the gating.
> 
> > Anyway, can you please submit proper patch with git send-email? Thanks!
> 
> Will do; Working on that! And you're welcome.

Awesome. So after reading your replies, let's just rename mx28_reg to 
mx28_reg_32 and introduce mx28_reg_8 for this particular problem.

M

> 
> Cheers,
> 
>         Robert.1

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Robert Deliën]

> Awesome. So after reading your replies, let's just rename mx28_reg to
> mx28_reg_32 and introduce mx28_reg_8 for this particular problem.

You were probably already foreseeing this when you made your
suggestion to use accessors, but now I'm working it I see introducing
mx28_reg_8 may get too messy. The 4 byte-parts (name, name_set,
name_clr and name_tog) of one register-set (cpu) are interleaved with
the next set (emi), like this:
        uint8_t hw_clkctrl_frac0_cpu;
        uint8_t hw_clkctrl_frac0_emi;
        uint8_t hw_clkctrl_frac0_io1;
        uint8_t hw_clkctrl_frac0_io0;
        uint8_t hw_clkctrl_frac0_cpu_set;
        uint8_t hw_clkctrl_frac0_emi_set;
        uint8_t hw_clkctrl_frac0_io1_set;
        uint8_t hw_clkctrl_frac0_io0_set;
        uint8_t hw_clkctrl_frac0_cpu_clr;
        uint8_t hw_clkctrl_frac0_emi_clr;
        uint8_t hw_clkctrl_frac0_io1_clr;
        uint8_t hw_clkctrl_frac0_io0_clr;
        uint8_t hw_clkctrl_frac0_cpu_tog;
        uint8_t hw_clkctrl_frac0_emi_tog;
        uint8_t hw_clkctrl_frac0_io1_tog;
        uint8_t hw_clkctrl_frac0_io0_tog;

So this won't work due to overlap:
#define __mx28_reg_8(name)              \
        uint8_t name;                   \
        uint8_t name##_set;             \
        uint8_t name##_clr;             \
        uint8_t name##_tog;

Instead I'd have to drop the byte-part names (cpu, emi, io1 and io0)
and replace them with more generic names like b0 and b1 for byte 0
and 1, and declare them all at once, like this:
#define __mx28_reg_8(name)              \
        uint8_t name##_b0;              \
        uint8_t name##_b1;              \
        uint8_t name##_b2;              \
        uint8_t name##_b3;              \
        uint8_t name##_b0_set;          \
        uint8_t name##_b1_set;          \
        uint8_t name##_b2_set;          \
        uint8_t name##_b3_set;          \
        uint8_t name##_b0_clr;          \
        uint8_t name##_b1_clr;          \
        uint8_t name##_b2_clr;          \
        uint8_t name##_b3_clr;          \
        uint8_t name##_b0_tog;          \
        uint8_t name##_b1_tog;          \
        uint8_t name##_b2_tog;          \
        uint8_t name##_b3_tog;
Besides b0 and b1 being confusing in endianity context, it's not a
pretty solution.

I'm going to try a couple of different things now, to find the least-
ugly solution if I can't find a pretty one.

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Marek Vasut]

> > Awesome. So after reading your replies, let's just rename mx28_reg to
> > mx28_reg_32 and introduce mx28_reg_8 for this particular problem.
> 
> You were probably already foreseeing this when you made your
> suggestion to use accessors, but now I'm working it I see introducing
> mx28_reg_8 may get too messy. The 4 byte-parts (name, name_set,
> name_clr and name_tog) of one register-set (cpu) are interleaved with
> the next set (emi), like this:
>         uint8_t hw_clkctrl_frac0_cpu;
>         uint8_t hw_clkctrl_frac0_emi;
>         uint8_t hw_clkctrl_frac0_io1;
>         uint8_t hw_clkctrl_frac0_io0;
>         uint8_t hw_clkctrl_frac0_cpu_set;
>         uint8_t hw_clkctrl_frac0_emi_set;
>         uint8_t hw_clkctrl_frac0_io1_set;
>         uint8_t hw_clkctrl_frac0_io0_set;
>         uint8_t hw_clkctrl_frac0_cpu_clr;
>         uint8_t hw_clkctrl_frac0_emi_clr;
>         uint8_t hw_clkctrl_frac0_io1_clr;
>         uint8_t hw_clkctrl_frac0_io0_clr;
>         uint8_t hw_clkctrl_frac0_cpu_tog;
>         uint8_t hw_clkctrl_frac0_emi_tog;
>         uint8_t hw_clkctrl_frac0_io1_tog;
>         uint8_t hw_clkctrl_frac0_io0_tog;
> 
> So this won't work due to overlap:
> #define __mx28_reg_8(name)              \
>         uint8_t name;                   \
>         uint8_t name##_set;             \
>         uint8_t name##_clr;             \
>         uint8_t name##_tog;
> 
> Instead I'd have to drop the byte-part names (cpu, emi, io1 and io0)
> and replace them with more generic names like b0 and b1 for byte 0
> and 1, and declare them all at once, like this:
> #define __mx28_reg_8(name)              \
>         uint8_t name##_b0;              \
>         uint8_t name##_b1;              \
>         uint8_t name##_b2;              \
>         uint8_t name##_b3;              \
>         uint8_t name##_b0_set;          \
>         uint8_t name##_b1_set;          \
>         uint8_t name##_b2_set;          \
>         uint8_t name##_b3_set;          \
>         uint8_t name##_b0_clr;          \
>         uint8_t name##_b1_clr;          \
>         uint8_t name##_b2_clr;          \
>         uint8_t name##_b3_clr;          \
>         uint8_t name##_b0_tog;          \
>         uint8_t name##_b1_tog;          \
>         uint8_t name##_b2_tog;          \
>         uint8_t name##_b3_tog;
> Besides b0 and b1 being confusing in endianity context, it's not a
> pretty solution.

Make it an array?

M
> 
> I'm going to try a couple of different things now, to find the least-
> ugly solution if I can't find a pretty one.

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Robert Deliën]

> Make it an array?

If that works for you, it works for me. Thanks!

#define __mx28_reg_8(name)              \
        uint8_t name##[4];              \
        uint8_t name##[4]_set;          \
        uint8_t name##[4]_clr;          \
        uint8_t name##[4]_tog;

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Marek Vasut]

> > Make it an array?
> 
> If that works for you, it works for me. Thanks!
> 
> #define __mx28_reg_8(name)              \
>         uint8_t name##[4];              \
>         uint8_t name##[4]_set;          \
>         uint8_t name##[4]_clr;          \
>         uint8_t name##[4]_tog;

Might be

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Marek Vasut]

> 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.
> Applying this patch fixes this problem.
> 
> Signed-off-by: Robert Delien (robert at delien.nl)

Hi,

I was thinking of this and we might need to introduce either special accessor 
for this particular register or rework include/regs-common.h and introduce 
mx28_reg_8 (which I don't think is a good idea).

So Robert, what do you think about introducing a special accessor in 
include/regs-clkctrl for FRAC0/1? This though introduces a problem with usage of 
_set, _clk and _tog accesses, which might need a little thinking through.

Thanks for this patch, you really did a good share of research on this one.

M

> 
> ________________________________________
> From: Marek Vasut [marek.vasut at gmail.com]
> Sent: 26 January 2012 19:32
> To: Fabio Estevam
> Cc: Robert Deli?n; u-boot at lists.denx.de
> Subject: Re: mx28 spl power cpu clock configuration
> 
> > Hi Robert,
> > 
> > On 1/25/12, Marek Vasut <marek.vasut@gmail.com> wrote:
> > >> Shouldn't we configure clkctrl_frac0 - or at least disable CPU clock
> > >> gating - before disabling PLL bypass?
> > > 
> > > This seems reasonable. Fabio, can you comment?
> > 
> > Could you please post a patch with your proposed change so that we can
> > test it?
> 
> Hi Fabio,
> 
> I bought a really crappy custom board a few days ago (some china-made crap)
> sporting mx287, but apparently I'm hitting similar issue you do here.
> 
> When I swap power_init and mem_init though, the board boots fine, othervise
> it hangs.
> 
> M

[U-Boot] [PATCH 1/2] i.MX28: Fix ref_cpu clock setup

[Robert Deliën]

Hi,

> I was thinking of this and we might need to introduce either special accessor
> for this particular register or rework include/regs-common.h and introduce
> mx28_reg_8 (which I don't think is a good idea).

I tend to agree with you about introducing mx28_reg_8. It doesn't 'feel right'
because it violates the orthogonality of it. But I'm afraid there's no other
solution because mx28_reg_32 implies 32-bit access, while it is in fact not
allowed.

I also think the manual should have specified these registers as separate
8-bit wide registers, preventing confusion and allowing for more appropriate
bit-field names.

> So Robert, what do you think about introducing a special accessor in
> include/regs-clkctrl for FRAC0/1? This though introduces a problem with usage of
> _set, _clk and _tog accesses, which might need a little thinking through.

I agree and I'm considering that now. I'm afraid there's no elegant solution for
this. Suggestions are welcome.

> Thanks for this patch, you really did a good share of research on this one.

You're welcome. I now have some grasp on how the clock-tree works, but
unfortunately still no 100% full understanding. What also bugs me is that I cannot
explain why the board continues to work when bypass is disabled on an
ill-configured pll. I'm afraid this code still needs some love and attention
before it's 100% right.

Cheers,

        Robert.