[U-Boot] [PATCH v3 08/10] sun5/7i: add an implementation of the psci suspend function

[Chen-Yu Tsai]

On Thu, Oct 27, 2016 at 9:10 PM, Antoine Tenart
<antoine.tenart at free-electrons.com> wrote:
> Hi,
>
> On Wed, Oct 26, 2016 at 02:38:10PM +0200, Maxime Ripard wrote:
>> On Wed, Oct 26, 2016 at 02:10:31PM +0200, Antoine Tenart wrote:
>> > +
>> > +#ifndef CONFIG_MACH_SUN7I
>> > +   /* switch cpuclk to losc */
>> > +   clrbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT);
>> > +#endif
>>
>> Some kind of comment here would be nice.
>
> That's based on my experiments, switching the cpu clk to losc wasn't
> working (the board hanged). I agree that's not the best explanation
> ever...
>
>> > +
>> > +   /* disable ldo */
>> > +   clrbits_le32(&ccm->osc24m_cfg, OSC24M_LDO_EN);
>>
>> I realise I'm the one who suggested doing this, but that might turn
>> out to be wrong. Have you tested devices that use the oscillator
>> directly, like the PWM?
>
> No, and good point.

I think the sun4i-timer and ARM arch timer are also clocked from OSC24M,
so turning it off is really not a good idea.

ChenYu

>> > +static void __secure sunxi_clock_leave_idle(struct sunxi_ccm_reg *ccm)
>> > +{
>> > +   /* enable ldo */
>> > +   setbits_le32(&ccm->osc24m_cfg, OSC24M_LDO_EN);
>> > +
>> > +#ifndef CONFIG_MACH_SUN7I
>> > +   /* switch cpuclk to osc24m */
>> > +   clrsetbits_le32(&ccm->cpu_ahb_apb0_cfg, 0x3 << CPU_CLK_SRC_SHIFT,
>> > +                   CPU_CLK_SRC_OSC24M << CPU_CLK_SRC_SHIFT);
>> > +#endif
>> > +
>> > +   /* enable pll1 */
>> > +   setbits_le32(&ccm->pll1_cfg, CCM_PLL1_CTRL_EN);
>>
>> It might be worth waiting for the PLL to lock by polling the bit 28
>> before switching the mux to it.
>
> I was doing this before, but it turned out this wasn't necessary to have
> the psci suspend function working. It would probably be safer though.
>
>> Looks good otherwise,
>
> Thanks for the review!
>
> Antoine
>
> --
> Antoine Ténart, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com