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,
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 [email protected].
For more options, visit https://groups.google.com/d/optout.
