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

[Antoine Tenart]

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.

> > +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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161027/c8c162d6/attachment.sig>