>Subject: Re: [PATCH 3/7] powerpc/85xx: add sleep and deep sleep support > >On 11/04/2011 07:33 AM, Zhao Chenhui wrote: >> +/* Cast the ccsrbar to 64-bit parameter so that the assembly >> + * code can be compatible with both 32-bit & 36-bit */ >> +extern void mpc85xx_enter_deep_sleep(u64 ccsrbar, u32 powmgtreq); > >/* > * Please use proper > * Linux multi-line comment format. > */ > >> static int pmc_suspend_enter(suspend_state_t state) >> { >> int ret; >> + u32 powmgtreq = 0x00500000; > >Where does this 0x00500000 come from? Please symbolically define >individual bits. > >The comment in the asm code says it should be 0x00100000, BTW.
I think we should get rid of the powmgtreq argument, as we don't support multiple modes for mpc85xx_enter_deep_sleep() now. > >> + >> + switch (state) { >> + case PM_SUSPEND_MEM: >> +#ifdef CONFIG_SPE >> + enable_kernel_spe(); >> +#endif > >Should comment that currently only e500v2 hardware supports deep sleep >-- else we'd need to save normal FP here. > >> + pr_debug("Entering deep sleep\n"); >> + >> + local_irq_disable(); >> + mpc85xx_enter_deep_sleep(get_immrbase(), >> + powmgtreq); >> + pr_debug("Resumed from deep sleep\n"); >> + >> + return 0; >> + >> + /* else fall-through */ >> + case PM_SUSPEND_STANDBY: > >What fall-through? You just returned. > >> + } >> >> - /* Upon resume, wait for SLP bit to be clear. */ >> - ret = spin_event_timeout((in_be32(&pmc_regs->pmcsr) & PMCSR_SLP) == >0, >> - 10000, 10) ? 0 : -ETIMEDOUT; >> - if (ret) >> - dev_err(pmc_dev, "tired waiting for SLP bit to clear\n"); >> - return ret; >> } > >Remove that blank line as well. > >> @@ -58,13 +101,23 @@ static const struct platform_suspend_ops >pmc_suspend_ops = { >> .enter = pmc_suspend_enter, >> }; >> >> -static int pmc_probe(struct platform_device *ofdev) >> +static int pmc_probe(struct platform_device *pdev) >> { >> - pmc_regs = of_iomap(ofdev->dev.of_node, 0); >> + struct device_node *np = pdev->dev.of_node; >> + >> + pmc_regs = of_iomap(pdev->dev.of_node, 0); >> if (!pmc_regs) >> return -ENOMEM; >> >> - pmc_dev = &ofdev->dev; >> + has_deep_sleep = 0; >> + if (of_device_is_compatible(np, "fsl,mpc8536-pmc")) >> + has_deep_sleep = 1; >> + >> + has_lossless = 0; >> + if (of_device_is_compatible(np, "fsl,p1022-pmc")) >> + has_lossless = 1; >> + > >You never use has_lossless. It will be used in the later patch in the series. - Leo _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev