| Message ID | 17053542.QUGvfkeRmc@wuerfel |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Jul 16, 2014 at 04:12:29PM +0200, Arnd Bergmann wrote: > On Wednesday 16 July 2014 15:22:41 Thierry Reding wrote: > > On Wed, Jul 16, 2014 at 01:56:44PM +0200, Arnd Bergmann wrote: > > > On Friday 11 July 2014, Thierry Reding wrote: > > > > +/* > > > > + * PMC > > > > + */ > > > > +enum tegra_suspend_mode { > > > > + TEGRA_SUSPEND_NONE = 0, > > > > + TEGRA_SUSPEND_LP2, /* CPU voltage off */ > > > > + TEGRA_SUSPEND_LP1, /* CPU voltage off, DRAM self-refresh */ > > > > + TEGRA_SUSPEND_LP0, /* CPU + core voltage off, DRAM self-refresh */ > > > > + TEGRA_MAX_SUSPEND_MODE, > > > > +}; > > > > + > > > > +#ifdef CONFIG_PM_SLEEP > > > > +enum tegra_suspend_mode tegra_pmc_get_suspend_mode(void); > > > > +void tegra_pmc_set_suspend_mode(enum tegra_suspend_mode mode); > > > > +void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode); > > > > + > > > > +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_restart(enum reboot_mode mode, const char *cmd); > > > > +#endif > > > > + > > > > +/* > > > > > > This part is causing multiple build failures in the randconfig tests. > > > You can avoid them by removing the #ifdef. > > > > How is this causing build failures? I only see them used wherever > > CONFIG_PM_SLEEP is defined. > > > > Although I guess tegra-pmc.c could cause sparse warnings since it > > implements these functions regardless of CONFIG_PM_SLEEP, which probably > > is the bug that should be fixed. > > > > Do you have a randconfig that I can use to reproduce this and come up > > with a fix? > > I got this error (always): > > /git/arm-soc/arch/arm/mach-tegra/tegra.c:165:13: error: 'tegra_pmc_restart' undeclared here (not in a function) > .restart = tegra_pmc_restart, > ^ > make[3]: *** [arch/arm/mach-tegra/tegra.o] Error 1 > > when CONFIG_PM_SLEEP is disabled, and also this one when SMP is > turned on in addition: > > > /git/arm-soc/arch/arm/mach-tegra/platsmp.c: In function 'tegra30_boot_secondary': > /git/arm-soc/arch/arm/mach-tegra/platsmp.c:97:4: error: implicit declaration of function 'tegra_pmc_cpu_is_powered' [-Werror=implicit-function-declaration] > if (tegra_pmc_cpu_is_powered(cpu)) > ^ > /git/arm-soc/arch/arm/mach-tegra/platsmp.c:110:3: error: implicit declaration of function 'tegra_pmc_cpu_power_on' [-Werror=implicit-function-declaration] > ret = tegra_pmc_cpu_power_on(cpu); > ^ > /git/arm-soc/arch/arm/mach-tegra/platsmp.c:129:2: error: implicit declaration of function 'tegra_pmc_cpu_remove_clamping' [-Werror=implicit-function-declaration] > ret = tegra_pmc_cpu_remove_clamping(cpu); > ^ > cc1: some warnings being treated as errors > > I applied this hack locally to work around that: > > diff --git a/include/linux/tegra-soc.h b/include/linux/tegra-soc.h > index 70a612442cda..a89fe9e588ac 100644 > --- a/include/linux/tegra-soc.h > +++ b/include/linux/tegra-soc.h > @@ -73,7 +73,6 @@ enum tegra_suspend_mode { > TEGRA_MAX_SUSPEND_MODE, > }; > > -#ifdef CONFIG_PM_SLEEP > enum tegra_suspend_mode tegra_pmc_get_suspend_mode(void); > void tegra_pmc_set_suspend_mode(enum tegra_suspend_mode mode); > void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode); > @@ -83,7 +82,6 @@ int tegra_pmc_cpu_power_on(int cpuid); > int tegra_pmc_cpu_remove_clamping(int cpuid); > > void tegra_pmc_restart(enum reboot_mode mode, const char *cmd); > -#endif > > /* > * PM I pushed fixes for these to Tegra's for-next branch (and for-3.17/soc). > > > > On a more general note, why are you adding this stuff into a global > > > header file in the first place? All users are in the same directory > > > in which the functions are defined. > > > > That's mostly preparatory work. We'll need to move tegra-pmc.c out of > > arch/arm/mach-tegra at some point. I've proposed two patches already to > > do that, one move the driver to drivers/soc/tegra and was massively > > NAK'ed (I'm still not sure I agree) and people requested that this be > > moved into drivers/power. I then posted a 2 patch series to move power > > supply drivers into a subdirectory (drivers/power/supply) so that > > drivers in drivers/power didn't have to depend on CONFIG_POWER_SUPPLY > > for no good reason. > > > > The latter series didn't receive any comments whatsoever in over a week, > > so in order to keep things moving forward I respun the PMC patch to do > > the conversion without moving the code out of arch/arm/mach-tegra yet. > > That way moving the driver out of arch/arm/mach-tegra will be a simple > > matter of moving and the rework will already be done. You were Cc'ed on > > the second series, so if you want to take a look (and maybe help get > > things moving) the patches are called: > > > > [PATCH 0/2] Restructure driver/power and add Tegra PMC driver > > [PATCH 1/2] power: Move power-supply drivers to subdirectory > > [PATCH 2/2] ARM: tegra: Convert PMC to a driver > > Ok, I'll have a look. I think when this becomes a separate driver, it > should also have its own header file, so maybe you can in the meantime > make it a local header file in mach-tegra until we have found a good > place for it. Why do you think it should be a separate header? We already have a couple in include/linux and I'm not sure it's useful to add even more. If anything I would've thought it made sense to move the content of the other headers into tegra-soc.h. Thierry
On Wednesday 16 July 2014 17:14:29 Thierry Reding wrote: > > > > Ok, I'll have a look. I think when this becomes a separate driver, it > > should also have its own header file, so maybe you can in the meantime > > make it a local header file in mach-tegra until we have found a good > > place for it. > > Why do you think it should be a separate header? We already have a > couple in include/linux and I'm not sure it's useful to add even more. > If anything I would've thought it made sense to move the content of the > other headers into tegra-soc.h. I very much dislike the idea of having a per-vendor header file that everything gets crammed into. We should try to have proper subsystems and generic interfaces for these wherever possible. Arnd -- 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
On Wed, Jul 16, 2014 at 05:22:03PM +0200, Arnd Bergmann wrote: > On Wednesday 16 July 2014 17:14:29 Thierry Reding wrote: > > > > > > Ok, I'll have a look. I think when this becomes a separate driver, it > > > should also have its own header file, so maybe you can in the meantime > > > make it a local header file in mach-tegra until we have found a good > > > place for it. > > > > Why do you think it should be a separate header? We already have a > > couple in include/linux and I'm not sure it's useful to add even more. > > If anything I would've thought it made sense to move the content of the > > other headers into tegra-soc.h. > > I very much dislike the idea of having a per-vendor header file that > everything gets crammed into. We should try to have proper subsystems > and generic interfaces for these wherever possible. I completely agree. However spreading the SoC-specific functions across multiple header files isn't going to help. If we keep all the per-vendor APIs in one file it makes it easier to see what could still be moved off into a separate subsystem. Now for PMC specifically, we've investigated converting the powergate API to power domains. I don't think it will be possible to make that work. The issue is that there's a defined sequence that needs to be respected to make sure the device is powered up properly. That sequence involves the primary clock and reset of the device. It's been proposed to make these clocks available to the PMC driver so that it can control them, but then we can't make sure that clocks are really off if they need to be, since we have two drivers accessing them. The only way I see to make that work reliably is by moving complete control of the powergate into drivers so that they can make sure clocks and resets are in the correct states. The PMC driver also provides access to I/O rails and specifically a deep power down state. Some modules are in deep power down state by default, so they need to be brought out of that state. I suppose this would be easier to turn into a generic framework because there aren't any cross- dependencies like for powergates, but I'm not aware of any other SoC having a similar feature (or implementation thereof in the kernel). And adding a subsystem just for the sake of it if only one implementation is available isn't a good idea in my opinion because it will be naturally designed to work best (and therefore maybe only) for the one instance. This issue is a fundamental one and there are bound to be other SoCs that have similarly unique blocks for which it's impractical to add a framework. I suspect the primary reason why we haven't run into it this frequently is because a lot of it is still hidden in arch/arm/mach-*. I'm open to suggestions of course, but the best option I currently see is to collect these custom APIs in a central place so that we can easily compare various SoCs for commonalities as time goes by and factor them out into subsystems where appropriate. For the same reason I think it's valid to put this type of code into drivers/soc. That way we have one subdirectory to look through for potential unification rather than various ones sprinkled across arch directories. It makes little sense in my opinion to move this code to drivers/power if there's no common framework anyway. Thierry
On Wed, Jul 16, 2014 at 11:57 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Jul 16, 2014 at 05:22:03PM +0200, Arnd Bergmann wrote: >> On Wednesday 16 July 2014 17:14:29 Thierry Reding wrote: >> > > >> > > Ok, I'll have a look. I think when this becomes a separate driver, it >> > > should also have its own header file, so maybe you can in the meantime >> > > make it a local header file in mach-tegra until we have found a good >> > > place for it. >> > >> > Why do you think it should be a separate header? We already have a >> > couple in include/linux and I'm not sure it's useful to add even more. >> > If anything I would've thought it made sense to move the content of the >> > other headers into tegra-soc.h. >> >> I very much dislike the idea of having a per-vendor header file that >> everything gets crammed into. We should try to have proper subsystems >> and generic interfaces for these wherever possible. > > I completely agree. However spreading the SoC-specific functions across > multiple header files isn't going to help. If we keep all the per-vendor > APIs in one file it makes it easier to see what could still be moved off > into a separate subsystem. > > Now for PMC specifically, we've investigated converting the powergate > API to power domains. I don't think it will be possible to make that > work. The issue is that there's a defined sequence that needs to be > respected to make sure the device is powered up properly. That sequence > involves the primary clock and reset of the device. It's been proposed > to make these clocks available to the PMC driver so that it can control > them, but then we can't make sure that clocks are really off if they > need to be, since we have two drivers accessing them. The only way I see > to make that work reliably is by moving complete control of the > powergate into drivers so that they can make sure clocks and resets are > in the correct states. > > The PMC driver also provides access to I/O rails and specifically a deep > power down state. Some modules are in deep power down state by default, > so they need to be brought out of that state. I suppose this would be > easier to turn into a generic framework because there aren't any cross- > dependencies like for powergates, but I'm not aware of any other SoC > having a similar feature (or implementation thereof in the kernel). And > adding a subsystem just for the sake of it if only one implementation is > available isn't a good idea in my opinion because it will be naturally > designed to work best (and therefore maybe only) for the one instance. > > This issue is a fundamental one and there are bound to be other SoCs > that have similarly unique blocks for which it's impractical to add a > framework. I suspect the primary reason why we haven't run into it this > frequently is because a lot of it is still hidden in arch/arm/mach-*. > > I'm open to suggestions of course, but the best option I currently see > is to collect these custom APIs in a central place so that we can easily > compare various SoCs for commonalities as time goes by and factor them > out into subsystems where appropriate. > > For the same reason I think it's valid to put this type of code into > drivers/soc. That way we have one subdirectory to look through for > potential unification rather than various ones sprinkled across arch > directories. It makes little sense in my opinion to move this code to > drivers/power if there's no common framework anyway. I agree. We can move them out and make them common them later if needed. We're sometimes trying too hard to find proper homes for various new drivers, which means that we're proliferating the kernel with a lot of new driver directories that have only one or two drivers in them. I'd rather collect stuff in drivers/soc, and move it out as needed later. Especially since we merge drivers/soc through one merge path (arm-soc) and can keep an eye on it, while the scatter-drivers-everywhere approach merges through various maintainers. -Olof -- 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
On Wed, Jul 16, 2014 at 08:57:16PM +0200, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Wed, Jul 16, 2014 at 05:22:03PM +0200, Arnd Bergmann wrote: > > On Wednesday 16 July 2014 17:14:29 Thierry Reding wrote: > > > > > > > > Ok, I'll have a look. I think when this becomes a separate driver, it > > > > should also have its own header file, so maybe you can in the meantime > > > > make it a local header file in mach-tegra until we have found a good > > > > place for it. > > > > > > Why do you think it should be a separate header? We already have a > > > couple in include/linux and I'm not sure it's useful to add even more. > > > If anything I would've thought it made sense to move the content of the > > > other headers into tegra-soc.h. > > > > I very much dislike the idea of having a per-vendor header file that > > everything gets crammed into. We should try to have proper subsystems > > and generic interfaces for these wherever possible. > > I completely agree. However spreading the SoC-specific functions across > multiple header files isn't going to help. If we keep all the per-vendor > APIs in one file it makes it easier to see what could still be moved off > into a separate subsystem. > > Now for PMC specifically, we've investigated converting the powergate > API to power domains. I don't think it will be possible to make that > work. The issue is that there's a defined sequence that needs to be > respected to make sure the device is powered up properly. That sequence > involves the primary clock and reset of the device. It's been proposed > to make these clocks available to the PMC driver so that it can control > them, but then we can't make sure that clocks are really off if they > need to be, since we have two drivers accessing them. The only way I see resets do not have reference counts, so they can be controlled by a powerdomain driver without any problems. For clocks, there would only be a problem for the module clocks if the drivers don't use runtime PM. If we move all drivers to runtime PM, the clock control can move into the powerdomain code and runtime PM will ensure domains are not turned off with active modules. > to make that work reliably is by moving complete control of the > powergate into drivers so that they can make sure clocks and resets are > in the correct states. > Which won't work if you have domains which contain several modules. Peter. -- 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
On Wednesday 16 July 2014 12:34:56 Olof Johansson wrote: > On Wed, Jul 16, 2014 at 11:57 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Wed, Jul 16, 2014 at 05:22:03PM +0200, Arnd Bergmann wrote: > >> On Wednesday 16 July 2014 17:14:29 Thierry Reding wrote: > >> > > > >> > > Ok, I'll have a look. I think when this becomes a separate driver, it > >> > > should also have its own header file, so maybe you can in the meantime > >> > > make it a local header file in mach-tegra until we have found a good > >> > > place for it. > >> > > >> > Why do you think it should be a separate header? We already have a > >> > couple in include/linux and I'm not sure it's useful to add even more. > >> > If anything I would've thought it made sense to move the content of the > >> > other headers into tegra-soc.h. > >> > >> I very much dislike the idea of having a per-vendor header file that > >> everything gets crammed into. We should try to have proper subsystems > >> and generic interfaces for these wherever possible. > > > > I completely agree. However spreading the SoC-specific functions across > > multiple header files isn't going to help. If we keep all the per-vendor > > APIs in one file it makes it easier to see what could still be moved off > > into a separate subsystem. > > > > Now for PMC specifically, we've investigated converting the powergate > > API to power domains. I don't think it will be possible to make that > > work. The issue is that there's a defined sequence that needs to be > > respected to make sure the device is powered up properly. That sequence > > involves the primary clock and reset of the device. It's been proposed > > to make these clocks available to the PMC driver so that it can control > > them, but then we can't make sure that clocks are really off if they > > need to be, since we have two drivers accessing them. The only way I see > > to make that work reliably is by moving complete control of the > > powergate into drivers so that they can make sure clocks and resets are > > in the correct states. I don't completely follow, but that's ok ;-) > > The PMC driver also provides access to I/O rails and specifically a deep > > power down state. Some modules are in deep power down state by default, > > so they need to be brought out of that state. I suppose this would be > > easier to turn into a generic framework because there aren't any cross- > > dependencies like for powergates, but I'm not aware of any other SoC > > having a similar feature (or implementation thereof in the kernel). And > > adding a subsystem just for the sake of it if only one implementation is > > available isn't a good idea in my opinion because it will be naturally > > designed to work best (and therefore maybe only) for the one instance. > > > > This issue is a fundamental one and there are bound to be other SoCs > > that have similarly unique blocks for which it's impractical to add a > > framework. I suspect the primary reason why we haven't run into it this > > frequently is because a lot of it is still hidden in arch/arm/mach-*. > > > > I'm open to suggestions of course, but the best option I currently see > > is to collect these custom APIs in a central place so that we can easily > > compare various SoCs for commonalities as time goes by and factor them > > out into subsystems where appropriate. > > > > For the same reason I think it's valid to put this type of code into > > drivers/soc. That way we have one subdirectory to look through for > > potential unification rather than various ones sprinkled across arch > > directories. It makes little sense in my opinion to move this code to > > drivers/power if there's no common framework anyway. > > I agree. We can move them out and make them common them later if needed. > > We're sometimes trying too hard to find proper homes for various new > drivers, which means that we're proliferating the kernel with a lot of > new driver directories that have only one or two drivers in them. > > I'd rather collect stuff in drivers/soc, and move it out as needed > later. Especially since we merge drivers/soc through one merge path > (arm-soc) and can keep an eye on it, while the > scatter-drivers-everywhere approach merges through various > maintainers. Ok. I'm fine with having one driver in drivers/soc for the pmc (and a few associated bits if necessary) and a header file for that. If you end up with two separate drivers in drivers/soc, I'd also prefer two separate header files. It may be a good idea to put these headers somewhere other than include/linux/*.h, which is completely overloaded by random stuff. We could use include/linux/soc/*.h or include/soc/*.h for those. Arnd -- 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
On Thu, Jul 17, 2014 at 10:53:08AM +0200, Peter De Schrijver wrote: > On Wed, Jul 16, 2014 at 08:57:16PM +0200, Thierry Reding wrote: > > * PGP Signed by an unknown key > > > > On Wed, Jul 16, 2014 at 05:22:03PM +0200, Arnd Bergmann wrote: > > > On Wednesday 16 July 2014 17:14:29 Thierry Reding wrote: > > > > > > > > > > Ok, I'll have a look. I think when this becomes a separate driver, it > > > > > should also have its own header file, so maybe you can in the meantime > > > > > make it a local header file in mach-tegra until we have found a good > > > > > place for it. > > > > > > > > Why do you think it should be a separate header? We already have a > > > > couple in include/linux and I'm not sure it's useful to add even more. > > > > If anything I would've thought it made sense to move the content of the > > > > other headers into tegra-soc.h. > > > > > > I very much dislike the idea of having a per-vendor header file that > > > everything gets crammed into. We should try to have proper subsystems > > > and generic interfaces for these wherever possible. > > > > I completely agree. However spreading the SoC-specific functions across > > multiple header files isn't going to help. If we keep all the per-vendor > > APIs in one file it makes it easier to see what could still be moved off > > into a separate subsystem. > > > > Now for PMC specifically, we've investigated converting the powergate > > API to power domains. I don't think it will be possible to make that > > work. The issue is that there's a defined sequence that needs to be > > respected to make sure the device is powered up properly. That sequence > > involves the primary clock and reset of the device. It's been proposed > > to make these clocks available to the PMC driver so that it can control > > them, but then we can't make sure that clocks are really off if they > > need to be, since we have two drivers accessing them. The only way I see > > resets do not have reference counts, so they can be controlled by a > powerdomain driver without any problems. For clocks, there would only be > a problem for the module clocks if the drivers don't use runtime PM. If > we move all drivers to runtime PM, the clock control can move into the > powerdomain code and runtime PM will ensure domains are not turned off > with active modules. > > > to make that work reliably is by moving complete control of the > > powergate into drivers so that they can make sure clocks and resets are > > in the correct states. > > > > Which won't work if you have domains which contain several modules. We also need to control the memory clients in the domains using MC_CLIENT_HOTRESET_CTRL. Cheers, Peter. -- 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
On Thu, Jul 17, 2014 at 12:01:56PM +0300, Peter De Schrijver wrote: > On Thu, Jul 17, 2014 at 10:53:08AM +0200, Peter De Schrijver wrote: > > On Wed, Jul 16, 2014 at 08:57:16PM +0200, Thierry Reding wrote: > > > * PGP Signed by an unknown key > > > > > > On Wed, Jul 16, 2014 at 05:22:03PM +0200, Arnd Bergmann wrote: > > > > On Wednesday 16 July 2014 17:14:29 Thierry Reding wrote: > > > > > > > > > > > > Ok, I'll have a look. I think when this becomes a separate driver, it > > > > > > should also have its own header file, so maybe you can in the meantime > > > > > > make it a local header file in mach-tegra until we have found a good > > > > > > place for it. > > > > > > > > > > Why do you think it should be a separate header? We already have a > > > > > couple in include/linux and I'm not sure it's useful to add even more. > > > > > If anything I would've thought it made sense to move the content of the > > > > > other headers into tegra-soc.h. > > > > > > > > I very much dislike the idea of having a per-vendor header file that > > > > everything gets crammed into. We should try to have proper subsystems > > > > and generic interfaces for these wherever possible. > > > > > > I completely agree. However spreading the SoC-specific functions across > > > multiple header files isn't going to help. If we keep all the per-vendor > > > APIs in one file it makes it easier to see what could still be moved off > > > into a separate subsystem. > > > > > > Now for PMC specifically, we've investigated converting the powergate > > > API to power domains. I don't think it will be possible to make that > > > work. The issue is that there's a defined sequence that needs to be > > > respected to make sure the device is powered up properly. That sequence > > > involves the primary clock and reset of the device. It's been proposed > > > to make these clocks available to the PMC driver so that it can control > > > them, but then we can't make sure that clocks are really off if they > > > need to be, since we have two drivers accessing them. The only way I see > > > > resets do not have reference counts, so they can be controlled by a > > powerdomain driver without any problems. For clocks, there would only be > > a problem for the module clocks if the drivers don't use runtime PM. If > > we move all drivers to runtime PM, the clock control can move into the > > powerdomain code and runtime PM will ensure domains are not turned off > > with active modules. > > > > > to make that work reliably is by moving complete control of the > > > powergate into drivers so that they can make sure clocks and resets are > > > in the correct states. > > > > > > > Which won't work if you have domains which contain several modules. > > We also need to control the memory clients in the domains using > MC_CLIENT_HOTRESET_CTRL. Oh, great. More interdependencies... Thierry
On Thu, Jul 17, 2014 at 10:54:17AM +0200, Arnd Bergmann wrote: > On Wednesday 16 July 2014 12:34:56 Olof Johansson wrote: > > On Wed, Jul 16, 2014 at 11:57 AM, Thierry Reding > > <thierry.reding@gmail.com> wrote: > > > On Wed, Jul 16, 2014 at 05:22:03PM +0200, Arnd Bergmann wrote: > > >> On Wednesday 16 July 2014 17:14:29 Thierry Reding wrote: > > >> > > > > >> > > Ok, I'll have a look. I think when this becomes a separate driver, it > > >> > > should also have its own header file, so maybe you can in the meantime > > >> > > make it a local header file in mach-tegra until we have found a good > > >> > > place for it. > > >> > > > >> > Why do you think it should be a separate header? We already have a > > >> > couple in include/linux and I'm not sure it's useful to add even more. > > >> > If anything I would've thought it made sense to move the content of the > > >> > other headers into tegra-soc.h. > > >> > > >> I very much dislike the idea of having a per-vendor header file that > > >> everything gets crammed into. We should try to have proper subsystems > > >> and generic interfaces for these wherever possible. > > > > > > I completely agree. However spreading the SoC-specific functions across > > > multiple header files isn't going to help. If we keep all the per-vendor > > > APIs in one file it makes it easier to see what could still be moved off > > > into a separate subsystem. > > > > > > Now for PMC specifically, we've investigated converting the powergate > > > API to power domains. I don't think it will be possible to make that > > > work. The issue is that there's a defined sequence that needs to be > > > respected to make sure the device is powered up properly. That sequence > > > involves the primary clock and reset of the device. It's been proposed > > > to make these clocks available to the PMC driver so that it can control > > > them, but then we can't make sure that clocks are really off if they > > > need to be, since we have two drivers accessing them. The only way I see > > > to make that work reliably is by moving complete control of the > > > powergate into drivers so that they can make sure clocks and resets are > > > in the correct states. > > I don't completely follow, but that's ok ;-) > > > > The PMC driver also provides access to I/O rails and specifically a deep > > > power down state. Some modules are in deep power down state by default, > > > so they need to be brought out of that state. I suppose this would be > > > easier to turn into a generic framework because there aren't any cross- > > > dependencies like for powergates, but I'm not aware of any other SoC > > > having a similar feature (or implementation thereof in the kernel). And > > > adding a subsystem just for the sake of it if only one implementation is > > > available isn't a good idea in my opinion because it will be naturally > > > designed to work best (and therefore maybe only) for the one instance. > > > > > > This issue is a fundamental one and there are bound to be other SoCs > > > that have similarly unique blocks for which it's impractical to add a > > > framework. I suspect the primary reason why we haven't run into it this > > > frequently is because a lot of it is still hidden in arch/arm/mach-*. > > > > > > I'm open to suggestions of course, but the best option I currently see > > > is to collect these custom APIs in a central place so that we can easily > > > compare various SoCs for commonalities as time goes by and factor them > > > out into subsystems where appropriate. > > > > > > For the same reason I think it's valid to put this type of code into > > > drivers/soc. That way we have one subdirectory to look through for > > > potential unification rather than various ones sprinkled across arch > > > directories. It makes little sense in my opinion to move this code to > > > drivers/power if there's no common framework anyway. > > > > I agree. We can move them out and make them common them later if needed. > > > > We're sometimes trying too hard to find proper homes for various new > > drivers, which means that we're proliferating the kernel with a lot of > > new driver directories that have only one or two drivers in them. > > > > I'd rather collect stuff in drivers/soc, and move it out as needed > > later. Especially since we merge drivers/soc through one merge path > > (arm-soc) and can keep an eye on it, while the > > scatter-drivers-everywhere approach merges through various > > maintainers. > > Ok. I'm fine with having one driver in drivers/soc for the pmc (and > a few associated bits if necessary) and a header file for that. If you > end up with two separate drivers in drivers/soc, I'd also prefer two > separate header files. > > It may be a good idea to put these headers somewhere other than > include/linux/*.h, which is completely overloaded by random stuff. > We could use include/linux/soc/*.h or include/soc/*.h for those. We could go all the way and make it include/soc/tegra/*.h for better namespacing. I guess either way would be fine, really, since the number of files in those directories should be small by definition, so we should be able to do without the extra SoC directory, too. I have a slight preference for a separate SoC directory, do you have any objections? Thierry
On 07/17/2014 07:01 PM, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Thu, Jul 17, 2014 at 12:01:56PM +0300, Peter De Schrijver wrote: >> On Thu, Jul 17, 2014 at 10:53:08AM +0200, Peter De Schrijver wrote: >>> On Wed, Jul 16, 2014 at 08:57:16PM +0200, Thierry Reding wrote: >>>>> Old Signed by an unknown key >>>> On Wed, Jul 16, 2014 at 05:22:03PM +0200, Arnd Bergmann wrote: >>>>> On Wednesday 16 July 2014 17:14:29 Thierry Reding wrote: >>>>>>> Ok, I'll have a look. I think when this becomes a separate driver, it >>>>>>> should also have its own header file, so maybe you can in the meantime >>>>>>> make it a local header file in mach-tegra until we have found a good >>>>>>> place for it. >>>>>> Why do you think it should be a separate header? We already have a >>>>>> couple in include/linux and I'm not sure it's useful to add even more. >>>>>> If anything I would've thought it made sense to move the content of the >>>>>> other headers into tegra-soc.h. >>>>> I very much dislike the idea of having a per-vendor header file that >>>>> everything gets crammed into. We should try to have proper subsystems >>>>> and generic interfaces for these wherever possible. >>>> I completely agree. However spreading the SoC-specific functions across >>>> multiple header files isn't going to help. If we keep all the per-vendor >>>> APIs in one file it makes it easier to see what could still be moved off >>>> into a separate subsystem. >>>> >>>> Now for PMC specifically, we've investigated converting the powergate >>>> API to power domains. I don't think it will be possible to make that >>>> work. The issue is that there's a defined sequence that needs to be >>>> respected to make sure the device is powered up properly. That sequence >>>> involves the primary clock and reset of the device. It's been proposed >>>> to make these clocks available to the PMC driver so that it can control >>>> them, but then we can't make sure that clocks are really off if they >>>> need to be, since we have two drivers accessing them. The only way I see >>> resets do not have reference counts, so they can be controlled by a >>> powerdomain driver without any problems. For clocks, there would only be >>> a problem for the module clocks if the drivers don't use runtime PM. If >>> we move all drivers to runtime PM, the clock control can move into the >>> powerdomain code and runtime PM will ensure domains are not turned off >>> with active modules. >>> >>>> to make that work reliably is by moving complete control of the >>>> powergate into drivers so that they can make sure clocks and resets are >>>> in the correct states. >>>> >>> Which won't work if you have domains which contain several modules. >> We also need to control the memory clients in the domains using >> MC_CLIENT_HOTRESET_CTRL. > Oh, great. More interdependencies... Some domains depend on others. Can we take this into account? Thanks, Vince -- 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
On Mon, Jul 21, 2014 at 03:09:29PM +0800, Vince Hsu wrote: > > On 07/17/2014 07:01 PM, Thierry Reding wrote: > >* PGP Signed by an unknown key > > > >On Thu, Jul 17, 2014 at 12:01:56PM +0300, Peter De Schrijver wrote: > >>On Thu, Jul 17, 2014 at 10:53:08AM +0200, Peter De Schrijver wrote: > >>>On Wed, Jul 16, 2014 at 08:57:16PM +0200, Thierry Reding wrote: > >>>>>Old Signed by an unknown key > >>>>On Wed, Jul 16, 2014 at 05:22:03PM +0200, Arnd Bergmann wrote: > >>>>>On Wednesday 16 July 2014 17:14:29 Thierry Reding wrote: > >>>>>>>Ok, I'll have a look. I think when this becomes a separate driver, it > >>>>>>>should also have its own header file, so maybe you can in the meantime > >>>>>>>make it a local header file in mach-tegra until we have found a good > >>>>>>>place for it. > >>>>>>Why do you think it should be a separate header? We already have a > >>>>>>couple in include/linux and I'm not sure it's useful to add even more. > >>>>>>If anything I would've thought it made sense to move the content of the > >>>>>>other headers into tegra-soc.h. > >>>>>I very much dislike the idea of having a per-vendor header file that > >>>>>everything gets crammed into. We should try to have proper subsystems > >>>>>and generic interfaces for these wherever possible. > >>>>I completely agree. However spreading the SoC-specific functions across > >>>>multiple header files isn't going to help. If we keep all the per-vendor > >>>>APIs in one file it makes it easier to see what could still be moved off > >>>>into a separate subsystem. > >>>> > >>>>Now for PMC specifically, we've investigated converting the powergate > >>>>API to power domains. I don't think it will be possible to make that > >>>>work. The issue is that there's a defined sequence that needs to be > >>>>respected to make sure the device is powered up properly. That sequence > >>>>involves the primary clock and reset of the device. It's been proposed > >>>>to make these clocks available to the PMC driver so that it can control > >>>>them, but then we can't make sure that clocks are really off if they > >>>>need to be, since we have two drivers accessing them. The only way I see > >>>resets do not have reference counts, so they can be controlled by a > >>>powerdomain driver without any problems. For clocks, there would only be > >>>a problem for the module clocks if the drivers don't use runtime PM. If > >>>we move all drivers to runtime PM, the clock control can move into the > >>>powerdomain code and runtime PM will ensure domains are not turned off > >>>with active modules. > >>> > >>>>to make that work reliably is by moving complete control of the > >>>>powergate into drivers so that they can make sure clocks and resets are > >>>>in the correct states. > >>>> > >>>Which won't work if you have domains which contain several modules. > >>We also need to control the memory clients in the domains using > >>MC_CLIENT_HOTRESET_CTRL. > >Oh, great. More interdependencies... > Some domains depend on others. Can we take this into account? I'm not aware of any dependencies. Can you point me at the relevant section in the TRM? Thierry
On 07/21/2014 05:02 PM, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Mon, Jul 21, 2014 at 03:09:29PM +0800, Vince Hsu wrote: >> On 07/17/2014 07:01 PM, Thierry Reding wrote: >>>> Old Signed by an unknown key >>> On Thu, Jul 17, 2014 at 12:01:56PM +0300, Peter De Schrijver wrote: >>>> On Thu, Jul 17, 2014 at 10:53:08AM +0200, Peter De Schrijver wrote: >>>>> On Wed, Jul 16, 2014 at 08:57:16PM +0200, Thierry Reding wrote: >>>>>>> Old Signed by an unknown key >>>>>> On Wed, Jul 16, 2014 at 05:22:03PM +0200, Arnd Bergmann wrote: >>>>>>> On Wednesday 16 July 2014 17:14:29 Thierry Reding wrote: >>>>>>>>> Ok, I'll have a look. I think when this becomes a separate driver, it >>>>>>>>> should also have its own header file, so maybe you can in the meantime >>>>>>>>> make it a local header file in mach-tegra until we have found a good >>>>>>>>> place for it. >>>>>>>> Why do you think it should be a separate header? We already have a >>>>>>>> couple in include/linux and I'm not sure it's useful to add even more. >>>>>>>> If anything I would've thought it made sense to move the content of the >>>>>>>> other headers into tegra-soc.h. >>>>>>> I very much dislike the idea of having a per-vendor header file that >>>>>>> everything gets crammed into. We should try to have proper subsystems >>>>>>> and generic interfaces for these wherever possible. >>>>>> I completely agree. However spreading the SoC-specific functions across >>>>>> multiple header files isn't going to help. If we keep all the per-vendor >>>>>> APIs in one file it makes it easier to see what could still be moved off >>>>>> into a separate subsystem. >>>>>> >>>>>> Now for PMC specifically, we've investigated converting the powergate >>>>>> API to power domains. I don't think it will be possible to make that >>>>>> work. The issue is that there's a defined sequence that needs to be >>>>>> respected to make sure the device is powered up properly. That sequence >>>>>> involves the primary clock and reset of the device. It's been proposed >>>>>> to make these clocks available to the PMC driver so that it can control >>>>>> them, but then we can't make sure that clocks are really off if they >>>>>> need to be, since we have two drivers accessing them. The only way I see >>>>> resets do not have reference counts, so they can be controlled by a >>>>> powerdomain driver without any problems. For clocks, there would only be >>>>> a problem for the module clocks if the drivers don't use runtime PM. If >>>>> we move all drivers to runtime PM, the clock control can move into the >>>>> powerdomain code and runtime PM will ensure domains are not turned off >>>>> with active modules. >>>>> >>>>>> to make that work reliably is by moving complete control of the >>>>>> powergate into drivers so that they can make sure clocks and resets are >>>>>> in the correct states. >>>>>> >>>>> Which won't work if you have domains which contain several modules. >>>> We also need to control the memory clients in the domains using >>>> MC_CLIENT_HOTRESET_CTRL. >>> Oh, great. More interdependencies... >> Some domains depend on others. Can we take this into account? > I'm not aware of any dependencies. Can you point me at the relevant > section in the TRM? As we talked offline yesterday, that's in 5.6.6.7 - 5.6.6.9. Thanks, Vince -- 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/include/linux/tegra-soc.h b/include/linux/tegra-soc.h index 70a612442cda..a89fe9e588ac 100644 --- a/include/linux/tegra-soc.h +++ b/include/linux/tegra-soc.h @@ -73,7 +73,6 @@ enum tegra_suspend_mode { TEGRA_MAX_SUSPEND_MODE, }; -#ifdef CONFIG_PM_SLEEP enum tegra_suspend_mode tegra_pmc_get_suspend_mode(void); void tegra_pmc_set_suspend_mode(enum tegra_suspend_mode mode); void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode); @@ -83,7 +82,6 @@ int tegra_pmc_cpu_power_on(int cpuid); int tegra_pmc_cpu_remove_clamping(int cpuid); void tegra_pmc_restart(enum reboot_mode mode, const char *cmd); -#endif /* * PM