i.MX28: Fix ref_cpu clock setup

PATCH Superseded Fabio Estevam 3 participants 15 messages Feb 2, 2012 – Feb 3, 2012 1 message not in the archive

Share & tools

A message that is not in the archive (sent off-list or not yet received): 6EA3E0BCC03CC34B89B01BD57ECBC718F529B9@POBOX.postoffice.danego.net

Fabio Estevam

author

2012-02-02 16:58 UTC #1 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

Hi Robert,

Quoted 14 lines · Robert Deli?n

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

Quoted 6 lines

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

author

2012-02-02 17:09 UTC #2 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

Quoted 2 lines · Fabio Estevam

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

Robert Deliën

2012-02-02 17:21 UTC #3 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

Quoted 5 lines

Quoted 2 lines
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.

Fabio Estevam

author

2012-02-02 19:03 UTC #4 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

Quoted 2 lines · Robert Deli?n

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

Robert Deliën

2012-02-03 10:38 UTC #5 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

Quoted 2 lines

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);

Robert Deliën

2012-02-03 10:32 UTC #6 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

In reply to Fabio Estevam

Hi Fabio,

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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

Will do!

Quoted 1 line

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.

Quoted 1 line

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.

Marek Vasut

2012-02-02 19:19 UTC #7 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

In reply to a message not in the archive

Quoted 6 lines

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)

Quoted 8 lines

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!

Quoted 30 lines

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
Quoted 10 lines
Hi Robert,
Quoted 4 lines · Marek Vasut
Quoted 2 lines
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

Robert Deliën

2012-02-03 10:42 UTC #8 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

Hi,

Quoted 2 lines

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.

Quoted 1 line

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

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

Cheers,

Robert.1

Marek Vasut

2012-02-03 12:27 UTC #9 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

Quoted 5 lines

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

Quoted 8 lines

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.
Quoted 1 line
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.

Quoted 4 lines

Cheers,
Robert.1

Robert Deliën

2012-02-03 14:00 UTC #10 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

Quoted 2 lines

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.

Marek Vasut

2012-02-03 14:09 UTC #11 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

Quoted 52 lines

Quoted 2 lines

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.

        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;

Quoted 2 lines

Besides b0 and b1 being confusing in endianity context, it's not a pretty solution.

Make it an array?

Quoted 3 lines

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.

Robert Deliën

2012-02-03 14:18 UTC #12 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

Quoted 1 line

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;

Marek Vasut

2012-02-03 14:28 UTC #13 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

Quoted 9 lines

Quoted 1 line

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

Marek Vasut

2012-02-03 07:23 UTC #14 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

In reply to a message not in the archive

Quoted 10 lines

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

Quoted 6 lines

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.

Quoted 28 lines

________________________________________
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
Quoted 10 lines
Hi Robert,
Quoted 4 lines · Marek Vasut
Quoted 2 lines
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

Robert Deliën

2012-02-03 11:13 UTC #15 lore raw

PATCH 1/2 i.MX28: Fix ref_cpu clock setup

Hi,

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 1 line

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.