| Message ID | 1361515491-16199-3-git-send-email-josephl@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote: > Adding the power on function for secondary CPUs in PMC driver, this can > help us to remove legacy powergate driver and add generic power domain > support later. > > Signed-off-by: Joseph Lo <josephl@nvidia.com> > --- > arch/arm/mach-tegra/pmc.c | 105 +++++++++++++++++++++++++++++++++++++++++++++- > arch/arm/mach-tegra/pmc.h | 4 ++ > 2 files changed, 107 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c > index 2315e25..a2446c2 100644 > --- a/arch/arm/mach-tegra/pmc.c > +++ b/arch/arm/mach-tegra/pmc.c > @@ -21,8 +21,26 @@ > #include <linux/of.h> > #include <linux/of_address.h> > > -#define PMC_CTRL 0x0 > -#define PMC_CTRL_INTR_LOW (1 << 17) > +#define PMC_CTRL 0x0 > +#define PMC_CTRL_INTR_LOW (1 << 17) > +#define PMC_PWRGATE_TOGGLE 0x30 > +#define PMC_PWRGATE_TOGGLE_START (1 << 8) > +#define PMC_REMOVE_CLAMPING 0x34 > +#define PMC_PWRGATE_STATUS 0x38 > + > +#define TEGRA_POWERGATE_PCIE 3 > +#define TEGRA_POWERGATE_VDEC 4 > +#define TEGRA_POWERGATE_CPU1 9 > +#define TEGRA_POWERGATE_CPU2 10 > +#define TEGRA_POWERGATE_CPU3 11 > + > +static u8 tegra_cpu_domains[] = { > + 0xFF, /* not available for CPU0 */ On Tegra114 we can also powergate CPU0, so this needs to be defined then. Cheers, Peter.
On 02/21/2013 11:44 PM, Joseph Lo wrote: > Adding the power on function for secondary CPUs in PMC driver, this can > help us to remove legacy powergate driver and add generic power domain > support later. > diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c > +static u8 tegra_cpu_domains[] = { > + 0xFF, /* not available for CPU0 */ > + TEGRA_POWERGATE_CPU1, > + TEGRA_POWERGATE_CPU2, > + TEGRA_POWERGATE_CPU3, > +}; Per Peter's comment, you probably need SoC-specific arrays here, to support CPU0 having a valid value or not. > +static int tegra_pmc_get_cpu_powerdomain_id(int cpuid) > +{ > + if (cpuid <= 0 || cpuid > num_possible_cpus()) cpuid >= num_possible_cpus()? > +static int tegra_pmc_powergate_set(int id, bool new_state) > +{ > + bool status; > + unsigned long flags; > + > + spin_lock_irqsave(&tegra_powergate_lock, flags); > + > + status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id); I would perform the read and the logical operations separately. Same for the write below. Don't you want to and with ~BIT(id) not BIT(id)? > +static int tegra_pmc_powergate_remove_clamping(int id) > +{ > + u32 mask; > + > + /* > + * Tegra has a bug where PCIE and VDE clamping masks are > + * swapped relatively to the partition ids. > + */ > + if (id == TEGRA_POWERGATE_VDEC) > + mask = (1 << TEGRA_POWERGATE_PCIE); > + else if (id == TEGRA_POWERGATE_PCIE) > + mask = (1 << TEGRA_POWERGATE_VDEC); > + else > + mask = (1 << id); Is this just true for this one register, but not others? If it's true everywhere, why not just fix the TEGRA_POWERGATE_* definitions? I asked this downstream, but you didn't answer. > +bool tegra_pmc_cpu_is_powered(int cpuid) > +{ > + int id; > + > + id = tegra_pmc_get_cpu_powerdomain_id(cpuid); > + if (IS_ERR_VALUE(id)) > + return false; As I pointed out downstream, that should be if (id < 0); IS_ERR_VALUE is intended for use on error-pointers, not on integer error codes.
On Fri, 2013-02-22 at 21:00 +0800, Peter De Schrijver wrote: > On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote: > > Adding the power on function for secondary CPUs in PMC driver, this can > > help us to remove legacy powergate driver and add generic power domain > > support later. > > > > Signed-off-by: Joseph Lo <josephl@nvidia.com> > > --- > > arch/arm/mach-tegra/pmc.c | 105 +++++++++++++++++++++++++++++++++++++++++++++- > > arch/arm/mach-tegra/pmc.h | 4 ++ > > 2 files changed, 107 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c > > index 2315e25..a2446c2 100644 > > --- a/arch/arm/mach-tegra/pmc.c > > +++ b/arch/arm/mach-tegra/pmc.c > > @@ -21,8 +21,26 @@ > > #include <linux/of.h> > > #include <linux/of_address.h> > > > > -#define PMC_CTRL 0x0 > > -#define PMC_CTRL_INTR_LOW (1 << 17) > > +#define PMC_CTRL 0x0 > > +#define PMC_CTRL_INTR_LOW (1 << 17) > > +#define PMC_PWRGATE_TOGGLE 0x30 > > +#define PMC_PWRGATE_TOGGLE_START (1 << 8) > > +#define PMC_REMOVE_CLAMPING 0x34 > > +#define PMC_PWRGATE_STATUS 0x38 > > + > > +#define TEGRA_POWERGATE_PCIE 3 > > +#define TEGRA_POWERGATE_VDEC 4 > > +#define TEGRA_POWERGATE_CPU1 9 > > +#define TEGRA_POWERGATE_CPU2 10 > > +#define TEGRA_POWERGATE_CPU3 11 > > + > > +static u8 tegra_cpu_domains[] = { > > + 0xFF, /* not available for CPU0 */ > > On Tegra114 we can also powergate CPU0, so this needs to be defined then. > No, DON'T DO THAT!! We don't allow any code to power gate CPU0 manually here or elsewhere. The function here only for SMP and hotplug bring up for secondary CPU. Thanks, Joseph
On 02/22/2013 07:38 PM, Joseph Lo wrote: > On Fri, 2013-02-22 at 21:00 +0800, Peter De Schrijver wrote: >> On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote: >>> Adding the power on function for secondary CPUs in PMC driver, this can >>> help us to remove legacy powergate driver and add generic power domain >>> support later. >>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c >>> +static u8 tegra_cpu_domains[] = { >>> + 0xFF, /* not available for CPU0 */ >> >> On Tegra114 we can also powergate CPU0, so this needs to be defined then. >> > No, DON'T DO THAT!! > We don't allow any code to power gate CPU0 manually here or elsewhere. > > The function here only for SMP and hotplug bring up for secondary CPU. Doesn't Tegra114 have improved HW that does actually allow power-gating/hot-unplugging/... CPU 0, even though older HW didn't?
On Sat, 2013-02-23 at 12:32 +0800, Stephen Warren wrote: > On 02/22/2013 07:38 PM, Joseph Lo wrote: > > On Fri, 2013-02-22 at 21:00 +0800, Peter De Schrijver wrote: > >> On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote: > >>> Adding the power on function for secondary CPUs in PMC driver, this can > >>> help us to remove legacy powergate driver and add generic power domain > >>> support later. > > >>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c > > >>> +static u8 tegra_cpu_domains[] = { > >>> + 0xFF, /* not available for CPU0 */ > >> > >> On Tegra114 we can also powergate CPU0, so this needs to be defined then. > >> > > No, DON'T DO THAT!! > > We don't allow any code to power gate CPU0 manually here or elsewhere. > > > > The function here only for SMP and hotplug bring up for secondary CPU. > > Doesn't Tegra114 have improved HW that does actually allow > power-gating/hot-unplugging/... CPU 0, even though older HW didn't? Indeed. It support CPU0 be power gated. But the power up sequence does not through here by toggling PMC directly. Just like the CPU idle "powered-down" mode. The CPU0's power gating/un-gating sequence should set up some event flags in flow-controller and controlled by flow-controller. Thanks, Joseph
On Sat, 2013-02-23 at 02:37 +0800, Stephen Warren wrote: > On 02/21/2013 11:44 PM, Joseph Lo wrote: > > Adding the power on function for secondary CPUs in PMC driver, this can > > help us to remove legacy powergate driver and add generic power domain > > support later. > > > diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c > > +static int tegra_pmc_get_cpu_powerdomain_id(int cpuid) > > +{ > > + if (cpuid <= 0 || cpuid > num_possible_cpus()) > > cpuid >= num_possible_cpus()? > Yes. > > +static int tegra_pmc_powergate_set(int id, bool new_state) > > +{ > > + bool status; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&tegra_powergate_lock, flags); > > + > > + status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id); > > I would perform the read and the logical operations separately. Same for > the write below. > > Don't you want to and with ~BIT(id) not BIT(id)? > This is what I want. WARN_ON(!(!!(status & BIT(id)) ^ new_state)); > > +static int tegra_pmc_powergate_remove_clamping(int id) > > +{ > > + u32 mask; > > + > > + /* > > + * Tegra has a bug where PCIE and VDE clamping masks are > > + * swapped relatively to the partition ids. > > + */ > > + if (id == TEGRA_POWERGATE_VDEC) > > + mask = (1 << TEGRA_POWERGATE_PCIE); > > + else if (id == TEGRA_POWERGATE_PCIE) > > + mask = (1 << TEGRA_POWERGATE_VDEC); > > + else > > + mask = (1 << id); > > Is this just true for this one register, but not others? If it's true > everywhere, why not just fix the TEGRA_POWERGATE_* definitions? > > I asked this downstream, but you didn't answer. > This is because the bit of the powergate id was swapped in PMC_REMOVING_CLAMPING with others. So we only need a fix here. > > +bool tegra_pmc_cpu_is_powered(int cpuid) > > +{ > > + int id; > > + > > + id = tegra_pmc_get_cpu_powerdomain_id(cpuid); > > + if (IS_ERR_VALUE(id)) > > + return false; > > As I pointed out downstream, that should be if (id < 0); IS_ERR_VALUE is > intended for use on error-pointers, not on integer error codes. IS_ERR is for error pointers, IS_ERR_VALUE is for error return code, isn't it? Thanks, Joseph
On Sat, Feb 23, 2013 at 03:28:00PM +0800, Joseph Lo wrote: > On Sat, 2013-02-23 at 02:37 +0800, Stephen Warren wrote: > > On 02/21/2013 11:44 PM, Joseph Lo wrote: > > > + id = tegra_pmc_get_cpu_powerdomain_id(cpuid); > > > + if (IS_ERR_VALUE(id)) > > > + return false; > > > > As I pointed out downstream, that should be if (id < 0); IS_ERR_VALUE is > > intended for use on error-pointers, not on integer error codes. > > IS_ERR is for error pointers, IS_ERR_VALUE is for error return code, > isn't it? No. Just because there's a macro somewhere doesn't mean it should be used. The err.h stuff is only there to deal with functions which can return kernel pointer values and error values, and resolve the conflict between negative integers and pointers which also appear to be negative integers. If you don't have that conflict, then making use of err.h is pointless and is pure obfuscation. So, just use the plain: if (id < 0) return false; here.
On Sat, Feb 23, 2013 at 05:59:33AM +0100, Joseph Lo wrote: > On Sat, 2013-02-23 at 12:32 +0800, Stephen Warren wrote: > > On 02/22/2013 07:38 PM, Joseph Lo wrote: > > > On Fri, 2013-02-22 at 21:00 +0800, Peter De Schrijver wrote: > > >> On Fri, Feb 22, 2013 at 07:44:49AM +0100, Joseph Lo wrote: > > >>> Adding the power on function for secondary CPUs in PMC driver, this can > > >>> help us to remove legacy powergate driver and add generic power domain > > >>> support later. > > > > >>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c > > > > >>> +static u8 tegra_cpu_domains[] = { > > >>> + 0xFF, /* not available for CPU0 */ > > >> > > >> On Tegra114 we can also powergate CPU0, so this needs to be defined then. > > >> > > > No, DON'T DO THAT!! > > > We don't allow any code to power gate CPU0 manually here or elsewhere. > > > > > > The function here only for SMP and hotplug bring up for secondary CPU. > > > > Doesn't Tegra114 have improved HW that does actually allow > > power-gating/hot-unplugging/... CPU 0, even though older HW didn't? > > Indeed. It support CPU0 be power gated. But the power up sequence does > not through here by toggling PMC directly. Just like the CPU idle > "powered-down" mode. The CPU0's power gating/un-gating sequence should > set up some event flags in flow-controller and controlled by > flow-controller. Yes, obviously. We don't use this for powergating cores normally... Sorry for the confusion. Cheers, Peter.
On 02/23/2013 12:28 AM, Joseph Lo wrote: > On Sat, 2013-02-23 at 02:37 +0800, Stephen Warren wrote: >> On 02/21/2013 11:44 PM, Joseph Lo wrote: >>> Adding the power on function for secondary CPUs in PMC driver, this can >>> help us to remove legacy powergate driver and add generic power domain >>> support later. >> >>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c >>> +static int tegra_pmc_get_cpu_powerdomain_id(int cpuid) >>> +{ >>> + if (cpuid <= 0 || cpuid > num_possible_cpus()) >> >> cpuid >= num_possible_cpus()? >> > Yes. > >>> +static int tegra_pmc_powergate_set(int id, bool new_state) >>> +{ >>> + bool status; >>> + unsigned long flags; >>> + >>> + spin_lock_irqsave(&tegra_powergate_lock, flags); >>> + >>> + status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id); >> >> I would perform the read and the logical operations separately. Same for >> the write below. >> >> Don't you want to and with ~BIT(id) not BIT(id)? >> > This is what I want. > WARN_ON(!(!!(status & BIT(id)) ^ new_state)); Oh sorry, I guess this isn't a standard read-modify-write cycle, but something else. So yes, the & in the register read is probably fine as you have it, but yes there is an issue in the current WARN_ON comparison, as you say. The WARN_ON you gave above is rather complex. Simplest might be: u32 reg = tegra_pmc_readl(PMC_PWRGATE_STATUS); bool old_state = (reg >> id) & 1; WARN_ON(new_state == old_state);
On Fri, Feb 22, 2013 at 07:37:06PM +0100, Stephen Warren wrote: > On 02/21/2013 11:44 PM, Joseph Lo wrote: > > Adding the power on function for secondary CPUs in PMC driver, this can > > help us to remove legacy powergate driver and add generic power domain > > support later. > > > diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c > > > +static u8 tegra_cpu_domains[] = { > > + 0xFF, /* not available for CPU0 */ > > + TEGRA_POWERGATE_CPU1, > > + TEGRA_POWERGATE_CPU2, > > + TEGRA_POWERGATE_CPU3, > > +}; > > Per Peter's comment, you probably need SoC-specific arrays here, to > support CPU0 having a valid value or not. That was a mistake, we never use these functions on CPU0. Powergating uses a different path. So this is ok. Cheers, Peter.
diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c index 2315e25..a2446c2 100644 --- a/arch/arm/mach-tegra/pmc.c +++ b/arch/arm/mach-tegra/pmc.c @@ -21,8 +21,26 @@ #include <linux/of.h> #include <linux/of_address.h> -#define PMC_CTRL 0x0 -#define PMC_CTRL_INTR_LOW (1 << 17) +#define PMC_CTRL 0x0 +#define PMC_CTRL_INTR_LOW (1 << 17) +#define PMC_PWRGATE_TOGGLE 0x30 +#define PMC_PWRGATE_TOGGLE_START (1 << 8) +#define PMC_REMOVE_CLAMPING 0x34 +#define PMC_PWRGATE_STATUS 0x38 + +#define TEGRA_POWERGATE_PCIE 3 +#define TEGRA_POWERGATE_VDEC 4 +#define TEGRA_POWERGATE_CPU1 9 +#define TEGRA_POWERGATE_CPU2 10 +#define TEGRA_POWERGATE_CPU3 11 + +static u8 tegra_cpu_domains[] = { + 0xFF, /* not available for CPU0 */ + TEGRA_POWERGATE_CPU1, + TEGRA_POWERGATE_CPU2, + TEGRA_POWERGATE_CPU3, +}; +static DEFINE_SPINLOCK(tegra_powergate_lock); static void __iomem *tegra_pmc_base; static bool tegra_pmc_invert_interrupt; @@ -37,6 +55,89 @@ static inline void tegra_pmc_writel(u32 val, u32 reg) writel_relaxed(val, (tegra_pmc_base + reg)); } +static int tegra_pmc_get_cpu_powerdomain_id(int cpuid) +{ + if (cpuid <= 0 || cpuid > num_possible_cpus()) + return -EINVAL; + return tegra_cpu_domains[cpuid]; +} + +static int tegra_pmc_powergate_set(int id, bool new_state) +{ + bool status; + unsigned long flags; + + spin_lock_irqsave(&tegra_powergate_lock, flags); + + status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id); + + WARN_ON(status == new_state); + + tegra_pmc_writel(PMC_PWRGATE_TOGGLE_START | id, PMC_PWRGATE_TOGGLE); + + spin_unlock_irqrestore(&tegra_powergate_lock, flags); + + return 0; +} + +static bool tegra_pmc_powergate_is_powered(int id) +{ + u32 status; + + status = tegra_pmc_readl(PMC_PWRGATE_STATUS) & (1 << id); + return !!status; +} + +static int tegra_pmc_powergate_remove_clamping(int id) +{ + u32 mask; + + /* + * Tegra has a bug where PCIE and VDE clamping masks are + * swapped relatively to the partition ids. + */ + if (id == TEGRA_POWERGATE_VDEC) + mask = (1 << TEGRA_POWERGATE_PCIE); + else if (id == TEGRA_POWERGATE_PCIE) + mask = (1 << TEGRA_POWERGATE_VDEC); + else + mask = (1 << id); + + tegra_pmc_writel(mask, PMC_REMOVE_CLAMPING); + + return 0; +} + +bool tegra_pmc_cpu_is_powered(int cpuid) +{ + int id; + + id = tegra_pmc_get_cpu_powerdomain_id(cpuid); + if (IS_ERR_VALUE(id)) + return false; + return tegra_pmc_powergate_is_powered(id); +} + +int tegra_pmc_cpu_power_on(int cpuid) +{ + int id; + + id = tegra_pmc_get_cpu_powerdomain_id(cpuid); + if (IS_ERR_VALUE(id)) + return id; + return tegra_pmc_powergate_set(id, true); +} + +int tegra_pmc_cpu_remove_clamping(int cpuid) +{ + int id; + + id = tegra_pmc_get_cpu_powerdomain_id(cpuid); + if (IS_ERR_VALUE(id)) + return id; + return tegra_pmc_powergate_remove_clamping(id); +} + static const struct of_device_id matches[] __initconst = { { .compatible = "nvidia,tegra20-pmc" }, { } diff --git a/arch/arm/mach-tegra/pmc.h b/arch/arm/mach-tegra/pmc.h index 8995ee4..7d44710 100644 --- a/arch/arm/mach-tegra/pmc.h +++ b/arch/arm/mach-tegra/pmc.h @@ -18,6 +18,10 @@ #ifndef __MACH_TEGRA_PMC_H #define __MACH_TEGRA_PMC_H +bool tegra_pmc_cpu_is_powered(int cpuid); +int tegra_pmc_cpu_power_on(int cpuid); +int tegra_pmc_cpu_remove_clamping(int cpuid); + void tegra_pmc_init(void); #endif
Adding the power on function for secondary CPUs in PMC driver, this can help us to remove legacy powergate driver and add generic power domain support later. Signed-off-by: Joseph Lo <josephl@nvidia.com> --- arch/arm/mach-tegra/pmc.c | 105 +++++++++++++++++++++++++++++++++++++++++++++- arch/arm/mach-tegra/pmc.h | 4 ++ 2 files changed, 107 insertions(+), 2 deletions(-)