| Message ID | 1449241037-22193-7-git-send-email-jonathanh@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Dec 04, 2015 at 02:57:07PM +0000, Jon Hunter wrote: > Currently, the function tegra_powergate_set() simply sets the desired > powergate state but does not wait for the state to change. In most cases > we should wait for the state to change before proceeding. Currently, there > is a case for tegra114 and tegra124 devices where we do not wait when > starting the secondary CPU as this is not necessary. However, this is only > done at boot time and so waiting here will only have a small impact on boot > time. Therefore, update tegra_powergate_set() to wait when setting the > powergate. > > By adding this feature, we can also eliminate the polling loop from > tegra30_boot_secondary(). > > A macro has also been adding for checking the status of the powergate and "added" > so update the tegra_powergate_is_powered() to use this macro as well. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > arch/arm/mach-tegra/platsmp.c | 16 +++------------- > drivers/soc/tegra/pmc.c | 21 +++++++++++++++------ > 2 files changed, 18 insertions(+), 19 deletions(-) > > diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c > index b45086666648..40cb761e7c95 100644 > --- a/arch/arm/mach-tegra/platsmp.c > +++ b/arch/arm/mach-tegra/platsmp.c > @@ -108,19 +108,9 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle) > * be un-gated by un-toggling the power gate register > * manually. > */ > - if (!tegra_pmc_cpu_is_powered(cpu)) { > - ret = tegra_pmc_cpu_power_on(cpu); > - if (ret) > - return ret; > - > - /* Wait for the power to come up. */ > - timeout = jiffies + msecs_to_jiffies(100); > - while (!tegra_pmc_cpu_is_powered(cpu)) { > - if (time_after(jiffies, timeout)) > - return -ETIMEDOUT; > - udelay(10); > - } > - } > + ret = tegra_pmc_cpu_power_on(cpu); > + if (ret) > + return ret; > > remove_clamps: > /* CPU partition is powered. Enable the CPU clock. */ I vaguely remember this being very brittle. I assume you've tested this extensively and made sure it doesn't regress? > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c > index fdd1a8d0940f..f94d970089ce 100644 > --- a/drivers/soc/tegra/pmc.c > +++ b/drivers/soc/tegra/pmc.c > @@ -28,6 +28,7 @@ > #include <linux/export.h> > #include <linux/init.h> > #include <linux/io.h> > +#include <linux/iopoll.h> > #include <linux/of.h> > #include <linux/of_address.h> > #include <linux/platform_device.h> > @@ -101,6 +102,8 @@ > > #define GPU_RG_CNTRL 0x2d4 > > +#define PMC_PWRGATE_STATE(status, id) ((status & BIT(id)) != 0) > + This looks suspiciously like a register or register field definition. Maybe turn this into a static inline function, such as: static inline bool tegra_powergate_state(u32 status, int id) { return (status & BIT(id)) != 0; } ? > struct tegra_pmc_soc { > unsigned int num_powergates; > const char *const *powergates; > @@ -181,22 +184,27 @@ static void tegra_pmc_writel(u32 value, unsigned long offset) > */ > static int tegra_powergate_set(int id, bool new_state) > { > - bool status; > + u32 status; > + int err = 0; Initialization to 0 doesn't seem necessary here. Thierry
On 14/01/16 14:01, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Fri, Dec 04, 2015 at 02:57:07PM +0000, Jon Hunter wrote: >> Currently, the function tegra_powergate_set() simply sets the desired >> powergate state but does not wait for the state to change. In most cases >> we should wait for the state to change before proceeding. Currently, there >> is a case for tegra114 and tegra124 devices where we do not wait when >> starting the secondary CPU as this is not necessary. However, this is only >> done at boot time and so waiting here will only have a small impact on boot >> time. Therefore, update tegra_powergate_set() to wait when setting the >> powergate. >> >> By adding this feature, we can also eliminate the polling loop from >> tegra30_boot_secondary(). >> >> A macro has also been adding for checking the status of the powergate and > > "added" Ok, thanks. >> so update the tegra_powergate_is_powered() to use this macro as well. >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> arch/arm/mach-tegra/platsmp.c | 16 +++------------- >> drivers/soc/tegra/pmc.c | 21 +++++++++++++++------ >> 2 files changed, 18 insertions(+), 19 deletions(-) >> >> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c >> index b45086666648..40cb761e7c95 100644 >> --- a/arch/arm/mach-tegra/platsmp.c >> +++ b/arch/arm/mach-tegra/platsmp.c >> @@ -108,19 +108,9 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle) >> * be un-gated by un-toggling the power gate register >> * manually. >> */ >> - if (!tegra_pmc_cpu_is_powered(cpu)) { >> - ret = tegra_pmc_cpu_power_on(cpu); >> - if (ret) >> - return ret; >> - >> - /* Wait for the power to come up. */ >> - timeout = jiffies + msecs_to_jiffies(100); >> - while (!tegra_pmc_cpu_is_powered(cpu)) { >> - if (time_after(jiffies, timeout)) >> - return -ETIMEDOUT; >> - udelay(10); >> - } >> - } >> + ret = tegra_pmc_cpu_power_on(cpu); >> + if (ret) >> + return ret; >> >> remove_clamps: >> /* CPU partition is powered. Enable the CPU clock. */ > > I vaguely remember this being very brittle. I assume you've tested this > extensively and made sure it doesn't regress? I have boot tested several times, but if you are concerned about this secondary boot failing specifically, I could setup a test over the weekend to boot test say 1000 iterations for all boards. >> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c >> index fdd1a8d0940f..f94d970089ce 100644 >> --- a/drivers/soc/tegra/pmc.c >> +++ b/drivers/soc/tegra/pmc.c >> @@ -28,6 +28,7 @@ >> #include <linux/export.h> >> #include <linux/init.h> >> #include <linux/io.h> >> +#include <linux/iopoll.h> >> #include <linux/of.h> >> #include <linux/of_address.h> >> #include <linux/platform_device.h> >> @@ -101,6 +102,8 @@ >> >> #define GPU_RG_CNTRL 0x2d4 >> >> +#define PMC_PWRGATE_STATE(status, id) ((status & BIT(id)) != 0) >> + > > This looks suspiciously like a register or register field definition. > Maybe turn this into a static inline function, such as: > > static inline bool tegra_powergate_state(u32 status, int id) > { > return (status & BIT(id)) != 0; > } > > ? Ok. >> struct tegra_pmc_soc { >> unsigned int num_powergates; >> const char *const *powergates; >> @@ -181,22 +184,27 @@ static void tegra_pmc_writel(u32 value, unsigned long offset) >> */ >> static int tegra_powergate_set(int id, bool new_state) >> { >> - bool status; >> + u32 status; >> + int err = 0; > > Initialization to 0 doesn't seem necessary here. Yes, ok. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c index b45086666648..40cb761e7c95 100644 --- a/arch/arm/mach-tegra/platsmp.c +++ b/arch/arm/mach-tegra/platsmp.c @@ -108,19 +108,9 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle) * be un-gated by un-toggling the power gate register * manually. */ - if (!tegra_pmc_cpu_is_powered(cpu)) { - ret = tegra_pmc_cpu_power_on(cpu); - if (ret) - return ret; - - /* Wait for the power to come up. */ - timeout = jiffies + msecs_to_jiffies(100); - while (!tegra_pmc_cpu_is_powered(cpu)) { - if (time_after(jiffies, timeout)) - return -ETIMEDOUT; - udelay(10); - } - } + ret = tegra_pmc_cpu_power_on(cpu); + if (ret) + return ret; remove_clamps: /* CPU partition is powered. Enable the CPU clock. */ diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c index fdd1a8d0940f..f94d970089ce 100644 --- a/drivers/soc/tegra/pmc.c +++ b/drivers/soc/tegra/pmc.c @@ -28,6 +28,7 @@ #include <linux/export.h> #include <linux/init.h> #include <linux/io.h> +#include <linux/iopoll.h> #include <linux/of.h> #include <linux/of_address.h> #include <linux/platform_device.h> @@ -101,6 +102,8 @@ #define GPU_RG_CNTRL 0x2d4 +#define PMC_PWRGATE_STATE(status, id) ((status & BIT(id)) != 0) + struct tegra_pmc_soc { unsigned int num_powergates; const char *const *powergates; @@ -181,22 +184,27 @@ static void tegra_pmc_writel(u32 value, unsigned long offset) */ static int tegra_powergate_set(int id, bool new_state) { - bool status; + u32 status; + int err = 0; mutex_lock(&pmc->powergates_lock); - status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id); + status = tegra_pmc_readl(PWRGATE_STATUS); - if (status == new_state) { + if (PMC_PWRGATE_STATE(status, id) == new_state) { mutex_unlock(&pmc->powergates_lock); return 0; } tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE); + err = readx_poll_timeout(tegra_pmc_readl, PWRGATE_STATUS, status, + PMC_PWRGATE_STATE(status, id) == new_state, + 10, 100000); + mutex_unlock(&pmc->powergates_lock); - return 0; + return err; } /** @@ -235,8 +243,9 @@ int tegra_powergate_is_powered(int id) if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates) return -EINVAL; - status = tegra_pmc_readl(PWRGATE_STATUS) & (1 << id); - return !!status; + status = tegra_pmc_readl(PWRGATE_STATUS); + + return PMC_PWRGATE_STATE(status, id); } /**
Currently, the function tegra_powergate_set() simply sets the desired powergate state but does not wait for the state to change. In most cases we should wait for the state to change before proceeding. Currently, there is a case for tegra114 and tegra124 devices where we do not wait when starting the secondary CPU as this is not necessary. However, this is only done at boot time and so waiting here will only have a small impact on boot time. Therefore, update tegra_powergate_set() to wait when setting the powergate. By adding this feature, we can also eliminate the polling loop from tegra30_boot_secondary(). A macro has also been adding for checking the status of the powergate and so update the tegra_powergate_is_powered() to use this macro as well. Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- arch/arm/mach-tegra/platsmp.c | 16 +++------------- drivers/soc/tegra/pmc.c | 21 +++++++++++++++------ 2 files changed, 18 insertions(+), 19 deletions(-)