LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "chenhui.zhao@freescale.com" <chenhui.zhao@freescale.com>
To: Scott Wood <scottwood@freescale.com>
Cc: "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Jason.Jin@freescale.com" <Jason.Jin@freescale.com>
Subject: Re: [2/4] powerpc/rcpm: add RCPM driver
Date: Thu, 2 Apr 2015 10:33:24 +0000	[thread overview]
Message-ID: <1427970805609.21194@freescale.com> (raw)
In-Reply-To: <20150331013057.GB5667@home.buserror.net>



________________________________________
From: Wood Scott-B07421
Sent: Tuesday, March 31, 2015 9:30
To: Zhao Chenhui-B35336
Cc: linuxppc-dev@lists.ozlabs.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Jin Zhengxiong-R64188
Subject: Re: [2/4] powerpc/rcpm: add RCPM driver

On Thu, Mar 26, 2015 at 06:18:13PM +0800, chenhui zhao wrote:
> There is a RCPM (Run Control/Power Management) in Freescale QorIQ
> series processors. The device performs tasks associated with device
> run control and power management.
>
> The driver implements some features: mask/unmask irq, enter/exit low
> power states, freeze time base, etc.
>
> Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com>
> ---
>  Documentation/devicetree/bindings/soc/fsl/rcpm.txt |  23 ++
>  arch/powerpc/include/asm/fsl_guts.h                | 105 ++++++
>  arch/powerpc/include/asm/fsl_pm.h                  |  49 +++
>  arch/powerpc/platforms/85xx/Kconfig                |   1 +
>  arch/powerpc/sysdev/Kconfig                        |   5 +
>  arch/powerpc/sysdev/Makefile                       |   1 +
>  arch/powerpc/sysdev/fsl_rcpm.c                     | 353 +++++++++++++++++++++
>  7 files changed, 537 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/fsl/rcpm.txt
>  create mode 100644 arch/powerpc/include/asm/fsl_pm.h
>  create mode 100644 arch/powerpc/sysdev/fsl_rcpm.c
>
> diff --git a/Documentation/devicetree/bindings/soc/fsl/rcpm.txt b/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> new file mode 100644
> index 0000000..8c21b6c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> @@ -0,0 +1,23 @@
> +* Run Control and Power Management
> +
> +The RCPM performs all device-level tasks associated with device run control
> +and power management.
> +
> +Required properites:
> +  - reg : Offset and length of the register set of RCPM block.
> +  - compatible : Specifies the compatibility list for the RCPM. The type
> +    should be string, such as "fsl,qoriq-rcpm-1.0", "fsl,qoriq-rcpm-2.0".
> +
> +Example:
> +The RCPM node for T4240:
> +     rcpm: global-utilities@e2000 {
> +             compatible = "fsl,t4240-rcpm", "fsl,qoriq-rcpm-2.0";
> +             reg = <0xe2000 0x1000>;
> +     };
> +
> +The RCPM node for P4080:
> +     rcpm: global-utilities@e2000 {
> +             compatible = "fsl,qoriq-rcpm-1.0";
> +             reg = <0xe2000 0x1000>;
> +             #sleep-cells = <1>;
> +     };

Where is #sleep-cells documented?  It's copy-and-paste from something
that was never finished from many years ago.

[chenhui] Will get rid of them.

> diff --git a/arch/powerpc/include/asm/fsl_pm.h b/arch/powerpc/include/asm/fsl_pm.h
> new file mode 100644
> index 0000000..bbe6089
> --- /dev/null
> +++ b/arch/powerpc/include/asm/fsl_pm.h
> @@ -0,0 +1,49 @@
> +/*
> + * Support Power Management
> + *
> + * Copyright 2014-2015 Freescale Semiconductor Inc.
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +#ifndef __PPC_FSL_PM_H
> +#define __PPC_FSL_PM_H
> +#ifdef       __KERNEL__

Put a space after #ifdef, not a tab.

[Chenhui] Will change it.

> +#define E500_PM_PH10 1
> +#define E500_PM_PH15 2
> +#define E500_PM_PH20 3
> +#define E500_PM_PH30 4
> +#define E500_PM_DOZE E500_PM_PH10
> +#define E500_PM_NAP  E500_PM_PH15
> +
> +#define PLAT_PM_SLEEP        20
> +#define PLAT_PM_LPM20        30
> +
> +#define FSL_PM_SLEEP         (1 << 0)
> +#define FSL_PM_DEEP_SLEEP    (1 << 1)
> +
> +struct fsl_pm_ops {
> +     /* mask pending interrupts to the RCPM from MPIC */
> +     void (*irq_mask)(int cpu);
> +     /* unmask pending interrupts to the RCPM from MPIC */
> +     void (*irq_unmask)(int cpu);
> +     /* place the CPU in the specified state */
> +     void (*cpu_enter_state)(int cpu, int state);
> +     /* exit the CPU from the specified state */
> +     void (*cpu_exit_state)(int cpu, int state);
> +     /* place the platform in the sleep state */
> +     int (*plat_enter_sleep)(void);
> +     /* freeze the time base */
> +     void (*freeze_time_base)(int freeze);
> +     /* keep the power of IP blocks during sleep/deep sleep */
> +     void (*set_ip_power)(int enable, u32 *mask);
> +     /* get platform supported power management modes */
> +     unsigned int (*get_pm_modes)(void);
> +};

Drop the comments that are basically just a restatement of the function
name.  Where there are comments, it'd be easier to read with a blank line
between a function and the next comment.

s/int enable/bool enable/
s/int freeze/bool freeze/

[chenhui] Yes, you are right.

> +#endif       /* __KERNEL__ */
> +#endif  /* __PPC_FSL_PM_H */

Please be consistent with whitespace.

> +     default:
> +             pr_err("%s: Unknown cpu PM state (%d)\n", __func__, state);

WARN?

> +static int rcpm_v2_plat_enter_state(int state)
> +{
> +     u32 *pmcsr_reg = &rcpm_v2_regs->powmgtcsr;
> +     int ret = 0;
> +     int result;
> +
> +     switch (state) {
> +     case PLAT_PM_LPM20:
> +             /* clear previous LPM20 status */
> +             setbits32(pmcsr_reg, RCPM_POWMGTCSR_P_LPM20_ST);

How would the bit be set when you enter here, given that you wait for it
to clear when leaving?

[chenhui] Actually, the bit is not used by software. Just follow the instruction in RM.

> +             /* enter LPM20 status */
> +             setbits32(pmcsr_reg, RCPM_POWMGTCSR_LPM20_RQ);
> +
> +             /* At this point, the device is in LPM20 status. */
> +
> +             /* resume ... */
> +             result = spin_event_timeout(
> +               !(in_be32(pmcsr_reg) & RCPM_POWMGTCSR_LPM20_ST), 10000, 10);
> +             if (!result) {
> +                     pr_err("%s: timeout waiting for LPM20 bit to be cleared\n",
> +                            __func__);
> +                     ret = -ETIMEDOUT;
> +             }
> +             break;

"At this point" is a bit misleading.  I think it's clear enough if you
just drop that comment.

> +     default:
> +             pr_err("%s: Unknown platform PM state (%d)\n",
> +                    __func__, state);
> +             ret = -EINVAL;
> +     }

WARN?

> +static const struct of_device_id rcpm_matches[] = {
> +     {
> +             .compatible = "fsl,qoriq-rcpm-1.0",
> +             .data = (void *)RCPM_V1,
> +     },
> +     {
> +             .compatible = "fsl,qoriq-rcpm-2.0",
> +             .data = (void *)RCPM_V2,
> +     },

Why not point .data directly at the ops?

[chenhui] I agree.

> +     switch ((unsigned long)match->data) {
> +     case RCPM_V1:
> +             rcpm_v1_regs = base;
> +             qoriq_pm_ops = &qoriq_rcpm_v1_ops;
> +             break;
> +
> +     case RCPM_V2:
> +             rcpm_v2_regs = base;
> +             qoriq_pm_ops = &qoriq_rcpm_v2_ops;
> +             break;
> +
> +     default:
> +             break;
> +     }

default: break; is unnecessary (and impossible to hit -- if you really
want default: it should probably WARN).

-Scott

[chenhui] Will get rid of them.

  reply	other threads:[~2015-04-02 10:33 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-26 10:18 [PATCH 1/4] powerpc/cache: add cache flush operation for various e500 Chenhui Zhao
2015-03-26 10:18 ` [PATCH 2/4] powerpc/rcpm: add RCPM driver Chenhui Zhao
2015-03-31  1:30   ` [2/4] " Scott Wood
2015-04-02 10:33     ` chenhui.zhao [this message]
2015-04-02 15:50       ` Scott Wood
2015-03-26 10:18 ` [PATCH 3/4] powerpc: support CPU hotplug for e500mc, e5500 and e6500 Chenhui Zhao
2015-03-31  2:07   ` [3/4] " Scott Wood
2015-04-02 11:16     ` chenhui.zhao
2015-04-02 16:03       ` Scott Wood
2015-04-03  2:54         ` chenhui.zhao
2015-03-26 10:18 ` [PATCH 4/4] powerpc/85xx: support sleep feature on QorIQ SoCs with RCPM Chenhui Zhao
2015-03-31  2:35   ` [4/4] " Scott Wood
2015-04-02 11:18     ` chenhui.zhao
2015-03-31  1:10 ` [1/4] powerpc/cache: add cache flush operation for various e500 Scott Wood
2015-04-02 10:14   ` chenhui.zhao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1427970805609.21194@freescale.com \
    --to=chenhui.zhao@freescale.com \
    --cc=Jason.Jin@freescale.com \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=scottwood@freescale.com \
    --subject='Re: [2/4] powerpc/rcpm: add RCPM driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).