>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
