| Message ID | 1453998832-27383-6-git-send-email-jonathanh@nvidia.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 28 January 2016 at 09:33, Jon Hunter <jonathanh@nvidia.com> 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 function has been added 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 | 9 ++++++++- > 2 files changed, 11 insertions(+), 14 deletions(-) > > diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c > index f3f61dbbda97..75620ae73913 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 99cb2fdd29e1..35ee60fd17be 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> > @@ -186,6 +187,9 @@ static inline bool tegra_powergate_state(int id) > */ > static int tegra_powergate_set(unsigned int id, bool new_state) > { > + bool status; > + int err; > + > mutex_lock(&pmc->powergates_lock); > > if (tegra_powergate_state(id) == new_state) { > @@ -195,9 +199,12 @@ static int tegra_powergate_set(unsigned int id, bool new_state) > > tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE); > > + err = readx_poll_timeout(tegra_powergate_state, id, status, > + status == new_state, 10, 100000); > + I understand (and agree) with the goal of this patch but I wonder (and I don't know much about the system/context) if holding a mutex while sleeping won't incur adverse effect on other parts of the system that weren't use to see this wait. One way to fix this might be to use "mutex_trylock()" and let callers retry as they see fit if an operation is already in progress. > mutex_unlock(&pmc->powergates_lock); > > - return 0; > + return err; > } > > /** > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29/01/16 16:58, Mathieu Poirier wrote: > On 28 January 2016 at 09:33, Jon Hunter <jonathanh@nvidia.com> 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 function has been added 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 | 9 ++++++++- >> 2 files changed, 11 insertions(+), 14 deletions(-) >> >> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c >> index f3f61dbbda97..75620ae73913 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 99cb2fdd29e1..35ee60fd17be 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> >> @@ -186,6 +187,9 @@ static inline bool tegra_powergate_state(int id) >> */ >> static int tegra_powergate_set(unsigned int id, bool new_state) >> { >> + bool status; >> + int err; >> + >> mutex_lock(&pmc->powergates_lock); >> >> if (tegra_powergate_state(id) == new_state) { >> @@ -195,9 +199,12 @@ static int tegra_powergate_set(unsigned int id, bool new_state) >> >> tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE); >> >> + err = readx_poll_timeout(tegra_powergate_state, id, status, >> + status == new_state, 10, 100000); >> + > > I understand (and agree) with the goal of this patch but I wonder (and > I don't know much about the system/context) if holding a mutex while > sleeping won't incur adverse effect on other parts of the system that > weren't use to see this wait. One way to fix this might be to use > "mutex_trylock()" and let callers retry as they see fit if an > operation is already in progress. I could unlock the mutex after writing the TOGGLE_START register. I don't believe that we need to waiting for a powergate to change before writing again. Jon -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/02/16 13:44, Jon Hunter wrote: > > On 29/01/16 16:58, Mathieu Poirier wrote: >> On 28 January 2016 at 09:33, Jon Hunter <jonathanh@nvidia.com> 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 function has been added 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 | 9 ++++++++- >>> 2 files changed, 11 insertions(+), 14 deletions(-) >>> >>> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c >>> index f3f61dbbda97..75620ae73913 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 99cb2fdd29e1..35ee60fd17be 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> >>> @@ -186,6 +187,9 @@ static inline bool tegra_powergate_state(int id) >>> */ >>> static int tegra_powergate_set(unsigned int id, bool new_state) >>> { >>> + bool status; >>> + int err; >>> + >>> mutex_lock(&pmc->powergates_lock); >>> >>> if (tegra_powergate_state(id) == new_state) { >>> @@ -195,9 +199,12 @@ static int tegra_powergate_set(unsigned int id, bool new_state) >>> >>> tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE); >>> >>> + err = readx_poll_timeout(tegra_powergate_state, id, status, >>> + status == new_state, 10, 100000); >>> + >> >> I understand (and agree) with the goal of this patch but I wonder (and >> I don't know much about the system/context) if holding a mutex while >> sleeping won't incur adverse effect on other parts of the system that >> weren't use to see this wait. One way to fix this might be to use >> "mutex_trylock()" and let callers retry as they see fit if an >> operation is already in progress. > > I could unlock the mutex after writing the TOGGLE_START register. I > don't believe that we need to waiting for a powergate to change before > writing again. I was not thinking clearly here, the whole reason for the locking is to protect the base address as we swap it during the probe. In general, I prefer to keep it the way we have it in this patch. By using mutex_trylock() would mean that now clients would have to handle retrying if they cannot get the lock and make it more complex. Powergates should not be turned on and off that frequently and so hopefully a delay here should be tolerable. For CPUs, this should only be used on cold boot and not during CPU hotplug events. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3 February 2016 at 02:20, Jon Hunter <jonathanh@nvidia.com> wrote: > > On 01/02/16 13:44, Jon Hunter wrote: >> >> On 29/01/16 16:58, Mathieu Poirier wrote: >>> On 28 January 2016 at 09:33, Jon Hunter <jonathanh@nvidia.com> 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 function has been added 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 | 9 ++++++++- >>>> 2 files changed, 11 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c >>>> index f3f61dbbda97..75620ae73913 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 99cb2fdd29e1..35ee60fd17be 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> >>>> @@ -186,6 +187,9 @@ static inline bool tegra_powergate_state(int id) >>>> */ >>>> static int tegra_powergate_set(unsigned int id, bool new_state) >>>> { >>>> + bool status; >>>> + int err; >>>> + >>>> mutex_lock(&pmc->powergates_lock); >>>> >>>> if (tegra_powergate_state(id) == new_state) { >>>> @@ -195,9 +199,12 @@ static int tegra_powergate_set(unsigned int id, bool new_state) >>>> >>>> tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE); >>>> >>>> + err = readx_poll_timeout(tegra_powergate_state, id, status, >>>> + status == new_state, 10, 100000); >>>> + >>> >>> I understand (and agree) with the goal of this patch but I wonder (and >>> I don't know much about the system/context) if holding a mutex while >>> sleeping won't incur adverse effect on other parts of the system that >>> weren't use to see this wait. One way to fix this might be to use >>> "mutex_trylock()" and let callers retry as they see fit if an >>> operation is already in progress. >> >> I could unlock the mutex after writing the TOGGLE_START register. I >> don't believe that we need to waiting for a powergate to change before >> writing again. > > I was not thinking clearly here, the whole reason for the locking is to > protect the base address as we swap it during the probe. > > In general, I prefer to keep it the way we have it in this patch. By > using mutex_trylock() would mean that now clients would have to handle > retrying if they cannot get the lock and make it more complex. Yes, the complexity is indeed pushed to the caller but that way we know exactly what is going on. Another way to implement this without any locks at all would be through RCUs. I'll let Stephen and Thierry make the call here. Thanks, Mahtieu > Powergates should not be turned on and off that frequently and so > hopefully a delay here should be tolerable. For CPUs, this should only > be used on cold boot and not during CPU hotplug events. > > Cheers > Jon -- To unsubscribe from this list: send the line "unsubscribe linux-pm" 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 f3f61dbbda97..75620ae73913 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 99cb2fdd29e1..35ee60fd17be 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> @@ -186,6 +187,9 @@ static inline bool tegra_powergate_state(int id) */ static int tegra_powergate_set(unsigned int id, bool new_state) { + bool status; + int err; + mutex_lock(&pmc->powergates_lock); if (tegra_powergate_state(id) == new_state) { @@ -195,9 +199,12 @@ static int tegra_powergate_set(unsigned int id, bool new_state) tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE); + err = readx_poll_timeout(tegra_powergate_state, id, status, + status == new_state, 10, 100000); + mutex_unlock(&pmc->powergates_lock); - return 0; + return err; } /**
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 function has been added 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 | 9 ++++++++- 2 files changed, 11 insertions(+), 14 deletions(-)