On 03/27/2014 09:58 PM, Ian Campbell wrote:
On Tue, 2014-03-25 at 12:59 +0100, Olliver Schinagl wrote:
- sr32(&ccm->apb1_clk_div_cfg, 24, 2, APB1_CLK_SRC_OSC24M);
- sr32(&ccm->apb1_clk_div_cfg, 16, 2, APB1_FACTOR_N);
- sr32(&ccm->apb1_clk_div_cfg, 0, 5, APB1_FACTOR_M);
+ clrsetbits_le32(&ccm->apb1_clk_div_cfg,
+ CCM_APB1_CLK_SRC_OSC24M, CCM_APB1_CLK_SRC_OSC24M);
+ clrsetbits_le32(&ccm->apb1_clk_div_cfg,
+ CCM_APB1_CLK_N_1, CCM_APB1_CLK_N_1);
+ clrsetbits_le32(&ccm->apb1_clk_div_cfg,
+ CCM_APB1_CLK_M(1), CCM_APB1_CLK_M(1));
I'm not convinced we always have to clear the bit, before setting it.
Using setbits is probably enough, but if this gets merged somehow (even
if only in our own tree for now), i'd rather see it initially merged as
such, to confirm everything is still working properly, and then with
tests slowly change it.
I mean, there must be a reason they used sr32 before, right?
sr32 would (I think) have cleared the entire range of bits given, not
just the bits corresponding to the value which it would then set,
IYSWIM.
Consider the case where the source is currently set to PLL6 (0x1) and
you are changing to PLL5 (0x2, because that is more illustrative than
OSC24==0x0). Your code would do effectively:
reg = (reg & ~0x2) | 0x2
if reg started as 0x1 then the result would be 0x3 and not 0x2 as
desired.
when what it should do is (reg & ~0x3) | 1, where 0x3 is the mask of all
the bits i.e. you are clearing the field before you set it.
In other words you don't want:
clrsetbits_le32(&ccm->apb1_clk_div_cfg, CCM_APB1_CLK_SRC_OSC24M,
CCM_APB1_CLK_SRC_OSC24M);
You want:
clrsetbits_le32(&ccm->apb1_clk_div_cfg, CCM_APB1_CLK_SRC_MASK,
CCM_APB1_CLK_SRC_OSC24M);
where
#define CCM_APB1_CLK_SRC_MASK CCM_APB1_CLK_SRC(0x3)
(or however you want to do it)
HTH,
That does indeed help a lot. And I will adapt to it.
Thank you for taking the time to explain it.
I guess your explanation also makes my statement sound silly and wrong ;)
I tried to read the defines to asm stuff, I'm not sure i follow it 100%.
The names however, your explanation and this comment from the code:
by
* specifying the mask in the 'clear' parameter and the new bit pattern
* in the 'set' parameter.
I do understand that you (obviously) are right and it's probably the
recommended way to set them, make sure everything is zero, then write
the bit, over the masked area.
I probably might want to check all the other setbits where the mask is
larger then 1. Since we don't know what WAS there, correct?
Thanks,
Olliver
Ian.
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
