| Message ID | 20191015170015.1135-1-digetx@gmail.com |
|---|---|
| Headers | show |
| Series | Consolidate and improve NVIDIA Tegra CPUIDLE driver(s) | expand |
On Tue, Oct 15, 2019 at 07:59:57PM +0300, Dmitry Osipenko wrote: > Hello, > > This series does the following: > > 1. Unifies Tegra20/30/114 drivers into a single driver and moves it out > into common drivers/cpuidle/ directory. > > 2. Enables CPU cluster power-down idling state on Tegra30. > > In the end there is a quite nice clean up of the Tegra CPUIDLE drivers > and of the Tegra's arch code in general. Please review, thanks! > > Changelog: > > v6: - Addressed request from Thierry Reding to change the way patches are > organized by making changes in a more incremental manner. > > - tegra_sleep_cpu() now checks for the secondary CPUs to be offline > in the "Make outer_disable() open-coded" patch. > > v5: - Rebased on a recent linux-next, fixed one minor conflict in Kconfig. > > - Improved commit's message of the "Support CPU cluster power-down state > on Tegra30" patch. > > - The "Support CPU cluster power-down state on Tegra30" patch is also > got split and now there is additional "Make outer_disable() open-coded" > patch. > > - Made minor cosmetic changes to the "Introduce unified driver for > NVIDIA Tegra SoCs" patch by improving error message and renaming > one variable. > > v4: - Fixed compilation with !CONFIG_CACHE_L2X0 (and tested that it still > works). > > - Replaced ktime_compare() with ktime_before() in the new driver, > for consistency. > > v3: - Addressed review comments that were made by Jon Hunter to v2 by > splitting patches into smaller (and simpler) chunks, better > documenting changes in the commit messages and using proper error > codes in the code. > > Warnings are replaced with a useful error messages in the code of > "Introduce unified driver for NVIDIA Tegra SoCs" patch. > > Secondary CPUs parking timeout increased to 100ms because I found > that it actually may happen to take more than 1ms if CPU is running > on a *very* low frequency. > > Added diagnostic messages that are reporting Flow Controller state > when CPU parking fails. > > Further polished cpuidle driver's code. > > The coupled state entering is now aborted if there is a pending SGI > (Software Generated Interrupt) because it will be lost after GIC's > power-cycling. Like it was done by the old Tegra20 CPUIDLE driver. > > v2: - Added patches to enable the new cpuidle driver in the defconfigs: > > ARM: multi_v7_defconfig: Enable Tegra cpuidle driver > ARM: tegra: Enable Tegra cpuidle driver in tegra_defconfig > > - Dropped patches that removed CPUIDLE_FLAG_TIMER_STOP from the idling > states because that flag actually doesn't have any negative effects, > but still is correct for the case of a local CPU timer on older Tegra > SoCs: > > cpuidle: tegra: Remove CPUIDLE_FLAG_TIMER_STOP from Tegra114/124 idle-state > cpuidle: tegra: Remove CPUIDLE_FLAG_TIMER_STOP from all states > > - The "Add unified driver for NVIDIA Tegra SoCs" patch got more polish. > Tegra30 and Terga114 states are now squashed into a single common C7 > state (following Parker TRM terminology, see 17.2.2.2 Power Management > States), more comments added, etc minor changes. It would be useful to switch the power state terminology to the one used for later chips: LP0 becomes SC7 LP1 becomes C1 LP2 becomes CC7 Meaning of these states is as follows C is a core state: C1 clock gating C2 not defined C3 not defined C4 not defined C5 not defined C6 not defined for ARM cores C7 power-gating CC is a CPU cluster C state: CC1 cluster clock gated CC2 not defined CC3 fmax@Vmin: not used prior to Tegra186 CC4: cluster retention: no longer supported CC5: not defined CC6: cluster power gating CC7: cluster rail gating SC is a System C state: SC1: not defined SC2: not defined SC3: not defined SC4: not defined SC5: not defined SC6: not defined SC7: VDD_SOC off Cheers, Peter.
16.10.2019 22:21, Peter De Schrijver пишет: > On Tue, Oct 15, 2019 at 07:59:57PM +0300, Dmitry Osipenko wrote: >> Hello, >> >> This series does the following: >> >> 1. Unifies Tegra20/30/114 drivers into a single driver and moves it out >> into common drivers/cpuidle/ directory. >> >> 2. Enables CPU cluster power-down idling state on Tegra30. >> >> In the end there is a quite nice clean up of the Tegra CPUIDLE drivers >> and of the Tegra's arch code in general. Please review, thanks! >> >> Changelog: >> >> v6: - Addressed request from Thierry Reding to change the way patches are >> organized by making changes in a more incremental manner. >> >> - tegra_sleep_cpu() now checks for the secondary CPUs to be offline >> in the "Make outer_disable() open-coded" patch. >> >> v5: - Rebased on a recent linux-next, fixed one minor conflict in Kconfig. >> >> - Improved commit's message of the "Support CPU cluster power-down state >> on Tegra30" patch. >> >> - The "Support CPU cluster power-down state on Tegra30" patch is also >> got split and now there is additional "Make outer_disable() open-coded" >> patch. >> >> - Made minor cosmetic changes to the "Introduce unified driver for >> NVIDIA Tegra SoCs" patch by improving error message and renaming >> one variable. >> >> v4: - Fixed compilation with !CONFIG_CACHE_L2X0 (and tested that it still >> works). >> >> - Replaced ktime_compare() with ktime_before() in the new driver, >> for consistency. >> >> v3: - Addressed review comments that were made by Jon Hunter to v2 by >> splitting patches into smaller (and simpler) chunks, better >> documenting changes in the commit messages and using proper error >> codes in the code. >> >> Warnings are replaced with a useful error messages in the code of >> "Introduce unified driver for NVIDIA Tegra SoCs" patch. >> >> Secondary CPUs parking timeout increased to 100ms because I found >> that it actually may happen to take more than 1ms if CPU is running >> on a *very* low frequency. >> >> Added diagnostic messages that are reporting Flow Controller state >> when CPU parking fails. >> >> Further polished cpuidle driver's code. >> >> The coupled state entering is now aborted if there is a pending SGI >> (Software Generated Interrupt) because it will be lost after GIC's >> power-cycling. Like it was done by the old Tegra20 CPUIDLE driver. >> >> v2: - Added patches to enable the new cpuidle driver in the defconfigs: >> >> ARM: multi_v7_defconfig: Enable Tegra cpuidle driver >> ARM: tegra: Enable Tegra cpuidle driver in tegra_defconfig >> >> - Dropped patches that removed CPUIDLE_FLAG_TIMER_STOP from the idling >> states because that flag actually doesn't have any negative effects, >> but still is correct for the case of a local CPU timer on older Tegra >> SoCs: >> >> cpuidle: tegra: Remove CPUIDLE_FLAG_TIMER_STOP from Tegra114/124 idle-state >> cpuidle: tegra: Remove CPUIDLE_FLAG_TIMER_STOP from all states >> >> - The "Add unified driver for NVIDIA Tegra SoCs" patch got more polish. >> Tegra30 and Terga114 states are now squashed into a single common C7 >> state (following Parker TRM terminology, see 17.2.2.2 Power Management >> States), more comments added, etc minor changes. > > It would be useful to switch the power state terminology to the one used > for later chips: > > LP0 becomes SC7 > LP1 becomes C1 > LP2 becomes CC7 > > Meaning of these states is as follows > > C is a core state: > > C1 clock gating > C2 not defined > C3 not defined > C4 not defined > C5 not defined > C6 not defined for ARM cores > C7 power-gating > > CC is a CPU cluster C state: > > CC1 cluster clock gated > CC2 not defined > CC3 fmax@Vmin: not used prior to Tegra186 > CC4: cluster retention: no longer supported > CC5: not defined > CC6: cluster power gating > CC7: cluster rail gating > > SC is a System C state: > > SC1: not defined > SC2: not defined > SC3: not defined > SC4: not defined > SC5: not defined > SC6: not defined > SC7: VDD_SOC off Hello Peter, But new "drivers/cpuidle/cpuidle-tegra.c" uses exactly that terminology, please see "cpuidle: Refactor and move NVIDIA Tegra20 driver into drivers/cpuidle/" and further patches. Am I missing something? Or do you want the renaming to be a separate patch?
16.10.2019 22:47, Dmitry Osipenko пишет: > 16.10.2019 22:21, Peter De Schrijver пишет: >> On Tue, Oct 15, 2019 at 07:59:57PM +0300, Dmitry Osipenko wrote: >>> Hello, >>> >>> This series does the following: >>> >>> 1. Unifies Tegra20/30/114 drivers into a single driver and moves it out >>> into common drivers/cpuidle/ directory. >>> >>> 2. Enables CPU cluster power-down idling state on Tegra30. >>> >>> In the end there is a quite nice clean up of the Tegra CPUIDLE drivers >>> and of the Tegra's arch code in general. Please review, thanks! >>> >>> Changelog: >>> >>> v6: - Addressed request from Thierry Reding to change the way patches are >>> organized by making changes in a more incremental manner. >>> >>> - tegra_sleep_cpu() now checks for the secondary CPUs to be offline >>> in the "Make outer_disable() open-coded" patch. >>> >>> v5: - Rebased on a recent linux-next, fixed one minor conflict in Kconfig. >>> >>> - Improved commit's message of the "Support CPU cluster power-down state >>> on Tegra30" patch. >>> >>> - The "Support CPU cluster power-down state on Tegra30" patch is also >>> got split and now there is additional "Make outer_disable() open-coded" >>> patch. >>> >>> - Made minor cosmetic changes to the "Introduce unified driver for >>> NVIDIA Tegra SoCs" patch by improving error message and renaming >>> one variable. >>> >>> v4: - Fixed compilation with !CONFIG_CACHE_L2X0 (and tested that it still >>> works). >>> >>> - Replaced ktime_compare() with ktime_before() in the new driver, >>> for consistency. >>> >>> v3: - Addressed review comments that were made by Jon Hunter to v2 by >>> splitting patches into smaller (and simpler) chunks, better >>> documenting changes in the commit messages and using proper error >>> codes in the code. >>> >>> Warnings are replaced with a useful error messages in the code of >>> "Introduce unified driver for NVIDIA Tegra SoCs" patch. >>> >>> Secondary CPUs parking timeout increased to 100ms because I found >>> that it actually may happen to take more than 1ms if CPU is running >>> on a *very* low frequency. >>> >>> Added diagnostic messages that are reporting Flow Controller state >>> when CPU parking fails. >>> >>> Further polished cpuidle driver's code. >>> >>> The coupled state entering is now aborted if there is a pending SGI >>> (Software Generated Interrupt) because it will be lost after GIC's >>> power-cycling. Like it was done by the old Tegra20 CPUIDLE driver. >>> >>> v2: - Added patches to enable the new cpuidle driver in the defconfigs: >>> >>> ARM: multi_v7_defconfig: Enable Tegra cpuidle driver >>> ARM: tegra: Enable Tegra cpuidle driver in tegra_defconfig >>> >>> - Dropped patches that removed CPUIDLE_FLAG_TIMER_STOP from the idling >>> states because that flag actually doesn't have any negative effects, >>> but still is correct for the case of a local CPU timer on older Tegra >>> SoCs: >>> >>> cpuidle: tegra: Remove CPUIDLE_FLAG_TIMER_STOP from Tegra114/124 idle-state >>> cpuidle: tegra: Remove CPUIDLE_FLAG_TIMER_STOP from all states >>> >>> - The "Add unified driver for NVIDIA Tegra SoCs" patch got more polish. >>> Tegra30 and Terga114 states are now squashed into a single common C7 >>> state (following Parker TRM terminology, see 17.2.2.2 Power Management >>> States), more comments added, etc minor changes. >> >> It would be useful to switch the power state terminology to the one used >> for later chips: >> >> LP0 becomes SC7 >> LP1 becomes C1 >> LP2 becomes CC7 >> >> Meaning of these states is as follows >> >> C is a core state: >> >> C1 clock gating >> C2 not defined >> C3 not defined >> C4 not defined >> C5 not defined >> C6 not defined for ARM cores >> C7 power-gating >> >> CC is a CPU cluster C state: >> >> CC1 cluster clock gated >> CC2 not defined >> CC3 fmax@Vmin: not used prior to Tegra186 >> CC4: cluster retention: no longer supported >> CC5: not defined >> CC6: cluster power gating >> CC7: cluster rail gating >> >> SC is a System C state: >> >> SC1: not defined >> SC2: not defined >> SC3: not defined >> SC4: not defined >> SC5: not defined >> SC6: not defined >> SC7: VDD_SOC off > > Hello Peter, > > But new "drivers/cpuidle/cpuidle-tegra.c" uses exactly that terminology, > please see "cpuidle: Refactor and move NVIDIA Tegra20 driver into > drivers/cpuidle/" and further patches. Am I missing something? Or do you > want the renaming to be a separate patch? > Or maybe you're suggesting to change the names everywhere and not only in the cpuidle driver? Please clarify :)
On Wed, Oct 16, 2019 at 11:14:07PM +0300, Dmitry Osipenko wrote: > 16.10.2019 22:47, Dmitry Osipenko пишет: > > 16.10.2019 22:21, Peter De Schrijver пишет: > >> On Tue, Oct 15, 2019 at 07:59:57PM +0300, Dmitry Osipenko wrote: > >>> Hello, > >>> > >>> This series does the following: > >>> > >>> 1. Unifies Tegra20/30/114 drivers into a single driver and moves it out > >>> into common drivers/cpuidle/ directory. > >>> > >>> 2. Enables CPU cluster power-down idling state on Tegra30. > >>> > >>> In the end there is a quite nice clean up of the Tegra CPUIDLE drivers > >>> and of the Tegra's arch code in general. Please review, thanks! > >>> > >>> Changelog: > >>> > >>> v6: - Addressed request from Thierry Reding to change the way patches are > >>> organized by making changes in a more incremental manner. > >>> > >>> - tegra_sleep_cpu() now checks for the secondary CPUs to be offline > >>> in the "Make outer_disable() open-coded" patch. > >>> > >>> v5: - Rebased on a recent linux-next, fixed one minor conflict in Kconfig. > >>> > >>> - Improved commit's message of the "Support CPU cluster power-down state > >>> on Tegra30" patch. > >>> > >>> - The "Support CPU cluster power-down state on Tegra30" patch is also > >>> got split and now there is additional "Make outer_disable() open-coded" > >>> patch. > >>> > >>> - Made minor cosmetic changes to the "Introduce unified driver for > >>> NVIDIA Tegra SoCs" patch by improving error message and renaming > >>> one variable. > >>> > >>> v4: - Fixed compilation with !CONFIG_CACHE_L2X0 (and tested that it still > >>> works). > >>> > >>> - Replaced ktime_compare() with ktime_before() in the new driver, > >>> for consistency. > >>> > >>> v3: - Addressed review comments that were made by Jon Hunter to v2 by > >>> splitting patches into smaller (and simpler) chunks, better > >>> documenting changes in the commit messages and using proper error > >>> codes in the code. > >>> > >>> Warnings are replaced with a useful error messages in the code of > >>> "Introduce unified driver for NVIDIA Tegra SoCs" patch. > >>> > >>> Secondary CPUs parking timeout increased to 100ms because I found > >>> that it actually may happen to take more than 1ms if CPU is running > >>> on a *very* low frequency. > >>> > >>> Added diagnostic messages that are reporting Flow Controller state > >>> when CPU parking fails. > >>> > >>> Further polished cpuidle driver's code. > >>> > >>> The coupled state entering is now aborted if there is a pending SGI > >>> (Software Generated Interrupt) because it will be lost after GIC's > >>> power-cycling. Like it was done by the old Tegra20 CPUIDLE driver. > >>> > >>> v2: - Added patches to enable the new cpuidle driver in the defconfigs: > >>> > >>> ARM: multi_v7_defconfig: Enable Tegra cpuidle driver > >>> ARM: tegra: Enable Tegra cpuidle driver in tegra_defconfig > >>> > >>> - Dropped patches that removed CPUIDLE_FLAG_TIMER_STOP from the idling > >>> states because that flag actually doesn't have any negative effects, > >>> but still is correct for the case of a local CPU timer on older Tegra > >>> SoCs: > >>> > >>> cpuidle: tegra: Remove CPUIDLE_FLAG_TIMER_STOP from Tegra114/124 idle-state > >>> cpuidle: tegra: Remove CPUIDLE_FLAG_TIMER_STOP from all states > >>> > >>> - The "Add unified driver for NVIDIA Tegra SoCs" patch got more polish. > >>> Tegra30 and Terga114 states are now squashed into a single common C7 > >>> state (following Parker TRM terminology, see 17.2.2.2 Power Management > >>> States), more comments added, etc minor changes. > >> > >> It would be useful to switch the power state terminology to the one used > >> for later chips: > >> > >> LP0 becomes SC7 > >> LP1 becomes C1 > >> LP2 becomes CC7 > >> > >> Meaning of these states is as follows > >> > >> C is a core state: > >> > >> C1 clock gating > >> C2 not defined > >> C3 not defined > >> C4 not defined > >> C5 not defined > >> C6 not defined for ARM cores > >> C7 power-gating > >> > >> CC is a CPU cluster C state: > >> > >> CC1 cluster clock gated > >> CC2 not defined > >> CC3 fmax@Vmin: not used prior to Tegra186 > >> CC4: cluster retention: no longer supported > >> CC5: not defined > >> CC6: cluster power gating > >> CC7: cluster rail gating > >> > >> SC is a System C state: > >> > >> SC1: not defined > >> SC2: not defined > >> SC3: not defined > >> SC4: not defined > >> SC5: not defined > >> SC6: not defined > >> SC7: VDD_SOC off > > > > Hello Peter, > > > > But new "drivers/cpuidle/cpuidle-tegra.c" uses exactly that terminology, > > please see "cpuidle: Refactor and move NVIDIA Tegra20 driver into > > drivers/cpuidle/" and further patches. Am I missing something? Or do you > > want the renaming to be a separate patch? > > > > Or maybe you're suggesting to change the names everywhere and not only > in the cpuidle driver? Please clarify :) At least some of the variable and function names still say lp2? Peter.
28.10.2019 17:04, Peter De Schrijver пишет: > On Wed, Oct 16, 2019 at 11:14:07PM +0300, Dmitry Osipenko wrote: >> 16.10.2019 22:47, Dmitry Osipenko пишет: >>> 16.10.2019 22:21, Peter De Schrijver пишет: >>>> On Tue, Oct 15, 2019 at 07:59:57PM +0300, Dmitry Osipenko wrote: >>>>> Hello, >>>>> >>>>> This series does the following: >>>>> >>>>> 1. Unifies Tegra20/30/114 drivers into a single driver and moves it out >>>>> into common drivers/cpuidle/ directory. >>>>> >>>>> 2. Enables CPU cluster power-down idling state on Tegra30. >>>>> >>>>> In the end there is a quite nice clean up of the Tegra CPUIDLE drivers >>>>> and of the Tegra's arch code in general. Please review, thanks! >>>>> >>>>> Changelog: >>>>> >>>>> v6: - Addressed request from Thierry Reding to change the way patches are >>>>> organized by making changes in a more incremental manner. >>>>> >>>>> - tegra_sleep_cpu() now checks for the secondary CPUs to be offline >>>>> in the "Make outer_disable() open-coded" patch. >>>>> >>>>> v5: - Rebased on a recent linux-next, fixed one minor conflict in Kconfig. >>>>> >>>>> - Improved commit's message of the "Support CPU cluster power-down state >>>>> on Tegra30" patch. >>>>> >>>>> - The "Support CPU cluster power-down state on Tegra30" patch is also >>>>> got split and now there is additional "Make outer_disable() open-coded" >>>>> patch. >>>>> >>>>> - Made minor cosmetic changes to the "Introduce unified driver for >>>>> NVIDIA Tegra SoCs" patch by improving error message and renaming >>>>> one variable. >>>>> >>>>> v4: - Fixed compilation with !CONFIG_CACHE_L2X0 (and tested that it still >>>>> works). >>>>> >>>>> - Replaced ktime_compare() with ktime_before() in the new driver, >>>>> for consistency. >>>>> >>>>> v3: - Addressed review comments that were made by Jon Hunter to v2 by >>>>> splitting patches into smaller (and simpler) chunks, better >>>>> documenting changes in the commit messages and using proper error >>>>> codes in the code. >>>>> >>>>> Warnings are replaced with a useful error messages in the code of >>>>> "Introduce unified driver for NVIDIA Tegra SoCs" patch. >>>>> >>>>> Secondary CPUs parking timeout increased to 100ms because I found >>>>> that it actually may happen to take more than 1ms if CPU is running >>>>> on a *very* low frequency. >>>>> >>>>> Added diagnostic messages that are reporting Flow Controller state >>>>> when CPU parking fails. >>>>> >>>>> Further polished cpuidle driver's code. >>>>> >>>>> The coupled state entering is now aborted if there is a pending SGI >>>>> (Software Generated Interrupt) because it will be lost after GIC's >>>>> power-cycling. Like it was done by the old Tegra20 CPUIDLE driver. >>>>> >>>>> v2: - Added patches to enable the new cpuidle driver in the defconfigs: >>>>> >>>>> ARM: multi_v7_defconfig: Enable Tegra cpuidle driver >>>>> ARM: tegra: Enable Tegra cpuidle driver in tegra_defconfig >>>>> >>>>> - Dropped patches that removed CPUIDLE_FLAG_TIMER_STOP from the idling >>>>> states because that flag actually doesn't have any negative effects, >>>>> but still is correct for the case of a local CPU timer on older Tegra >>>>> SoCs: >>>>> >>>>> cpuidle: tegra: Remove CPUIDLE_FLAG_TIMER_STOP from Tegra114/124 idle-state >>>>> cpuidle: tegra: Remove CPUIDLE_FLAG_TIMER_STOP from all states >>>>> >>>>> - The "Add unified driver for NVIDIA Tegra SoCs" patch got more polish. >>>>> Tegra30 and Terga114 states are now squashed into a single common C7 >>>>> state (following Parker TRM terminology, see 17.2.2.2 Power Management >>>>> States), more comments added, etc minor changes. >>>> >>>> It would be useful to switch the power state terminology to the one used >>>> for later chips: >>>> >>>> LP0 becomes SC7 >>>> LP1 becomes C1 >>>> LP2 becomes CC7 >>>> >>>> Meaning of these states is as follows >>>> >>>> C is a core state: >>>> >>>> C1 clock gating >>>> C2 not defined >>>> C3 not defined >>>> C4 not defined >>>> C5 not defined >>>> C6 not defined for ARM cores >>>> C7 power-gating >>>> >>>> CC is a CPU cluster C state: >>>> >>>> CC1 cluster clock gated >>>> CC2 not defined >>>> CC3 fmax@Vmin: not used prior to Tegra186 >>>> CC4: cluster retention: no longer supported >>>> CC5: not defined >>>> CC6: cluster power gating >>>> CC7: cluster rail gating >>>> >>>> SC is a System C state: >>>> >>>> SC1: not defined >>>> SC2: not defined >>>> SC3: not defined >>>> SC4: not defined >>>> SC5: not defined >>>> SC6: not defined >>>> SC7: VDD_SOC off >>> >>> Hello Peter, >>> >>> But new "drivers/cpuidle/cpuidle-tegra.c" uses exactly that terminology, >>> please see "cpuidle: Refactor and move NVIDIA Tegra20 driver into >>> drivers/cpuidle/" and further patches. Am I missing something? Or do you >>> want the renaming to be a separate patch? >>> >> >> Or maybe you're suggesting to change the names everywhere and not only >> in the cpuidle driver? Please clarify :) > > At least some of the variable and function names still say lp2? The cpuidle driver uses LP2 terminology for everything that comes from the external arch / firmware includes. But it says CC6 for everything that is internal to the driver. So yes, there is a bit of new/old terminology mixing in the code. The arch code / PMC driver / TF firmware are all saying LP2. The LP2 naming is also a part of the device-tree binding. It will be a lot of mess to rename the mach-tegra/pm.c code. I guess eventually it could be moved to drivers/soc/, so maybe it will be better to postpone the renaming until then?
On Tue, Oct 29, 2019 at 03:47:56AM +0300, Dmitry Osipenko wrote: .. > >>>> It would be useful to switch the power state terminology to the one used > >>>> for later chips: > >>>> > >>>> LP0 becomes SC7 > >>>> LP1 becomes C1 > >>>> LP2 becomes CC7 > >>>> > >>>> Meaning of these states is as follows > >>>> > >>>> C is a core state: > >>>> > >>>> C1 clock gating > >>>> C2 not defined > >>>> C3 not defined > >>>> C4 not defined > >>>> C5 not defined > >>>> C6 not defined for ARM cores > >>>> C7 power-gating > >>>> > >>>> CC is a CPU cluster C state: > >>>> > >>>> CC1 cluster clock gated > >>>> CC2 not defined > >>>> CC3 fmax@Vmin: not used prior to Tegra186 > >>>> CC4: cluster retention: no longer supported > >>>> CC5: not defined > >>>> CC6: cluster power gating > >>>> CC7: cluster rail gating > >>>> > >>>> SC is a System C state: > >>>> > >>>> SC1: not defined > >>>> SC2: not defined > >>>> SC3: not defined > >>>> SC4: not defined > >>>> SC5: not defined > >>>> SC6: not defined > >>>> SC7: VDD_SOC off > >>> > >>> Hello Peter, > >>> > >>> But new "drivers/cpuidle/cpuidle-tegra.c" uses exactly that terminology, > >>> please see "cpuidle: Refactor and move NVIDIA Tegra20 driver into > >>> drivers/cpuidle/" and further patches. Am I missing something? Or do you > >>> want the renaming to be a separate patch? > >>> > >> > >> Or maybe you're suggesting to change the names everywhere and not only > >> in the cpuidle driver? Please clarify :) > > > > At least some of the variable and function names still say lp2? > > The cpuidle driver uses LP2 terminology for everything that comes from > the external arch / firmware includes. But it says CC6 for everything > that is internal to the driver. So yes, there is a bit of new/old > terminology mixing in the code. > > The arch code / PMC driver / TF firmware are all saying LP2. The LP2 > naming is also a part of the device-tree binding. > > It will be a lot of mess to rename the mach-tegra/pm.c code. I guess > eventually it could be moved to drivers/soc/, so maybe it will be better > to postpone the renaming until then? Or maybe add a comment somewhere indicating: LP2 = CC6 LP1 = C1 LP0 = SC7 TF predates the new naming, so that may make some sense. Peter.
01.11.2019 15:33, Peter De Schrijver пишет: > On Tue, Oct 29, 2019 at 03:47:56AM +0300, Dmitry Osipenko wrote: > .. > >>>>>> It would be useful to switch the power state terminology to the one used >>>>>> for later chips: >>>>>> >>>>>> LP0 becomes SC7 >>>>>> LP1 becomes C1 >>>>>> LP2 becomes CC7 >>>>>> >>>>>> Meaning of these states is as follows >>>>>> >>>>>> C is a core state: >>>>>> >>>>>> C1 clock gating >>>>>> C2 not defined >>>>>> C3 not defined >>>>>> C4 not defined >>>>>> C5 not defined >>>>>> C6 not defined for ARM cores >>>>>> C7 power-gating >>>>>> >>>>>> CC is a CPU cluster C state: >>>>>> >>>>>> CC1 cluster clock gated >>>>>> CC2 not defined >>>>>> CC3 fmax@Vmin: not used prior to Tegra186 >>>>>> CC4: cluster retention: no longer supported >>>>>> CC5: not defined >>>>>> CC6: cluster power gating >>>>>> CC7: cluster rail gating >>>>>> >>>>>> SC is a System C state: >>>>>> >>>>>> SC1: not defined >>>>>> SC2: not defined >>>>>> SC3: not defined >>>>>> SC4: not defined >>>>>> SC5: not defined >>>>>> SC6: not defined >>>>>> SC7: VDD_SOC off >>>>> >>>>> Hello Peter, >>>>> >>>>> But new "drivers/cpuidle/cpuidle-tegra.c" uses exactly that terminology, >>>>> please see "cpuidle: Refactor and move NVIDIA Tegra20 driver into >>>>> drivers/cpuidle/" and further patches. Am I missing something? Or do you >>>>> want the renaming to be a separate patch? >>>>> >>>> >>>> Or maybe you're suggesting to change the names everywhere and not only >>>> in the cpuidle driver? Please clarify :) >>> >>> At least some of the variable and function names still say lp2? >> >> The cpuidle driver uses LP2 terminology for everything that comes from >> the external arch / firmware includes. But it says CC6 for everything >> that is internal to the driver. So yes, there is a bit of new/old >> terminology mixing in the code. >> >> The arch code / PMC driver / TF firmware are all saying LP2. The LP2 >> naming is also a part of the device-tree binding. >> >> It will be a lot of mess to rename the mach-tegra/pm.c code. I guess >> eventually it could be moved to drivers/soc/, so maybe it will be better >> to postpone the renaming until then? > > Or maybe add a comment somewhere indicating: > > LP2 = CC6 > LP1 = C1 > LP0 = SC7 > > TF predates the new naming, so that may make some sense. Today it should make more sense just to add an explicit comment to the cpuidle driver that clarifies the new naming (IMHO). I'll prepare v7 with that change. Maybe later on, once more code will be consolidated in drivers/soc/tegra/, it will become useful to duplicate the clarification there as well. Please let me know if you disagree or think that something better could be done.
01.11.2019 16:22, Dmitry Osipenko пишет: > 01.11.2019 15:33, Peter De Schrijver пишет: >> On Tue, Oct 29, 2019 at 03:47:56AM +0300, Dmitry Osipenko wrote: >> .. >> >>>>>>> It would be useful to switch the power state terminology to the one used >>>>>>> for later chips: >>>>>>> >>>>>>> LP0 becomes SC7 >>>>>>> LP1 becomes C1 >>>>>>> LP2 becomes CC7 >>>>>>> >>>>>>> Meaning of these states is as follows >>>>>>> >>>>>>> C is a core state: >>>>>>> >>>>>>> C1 clock gating >>>>>>> C2 not defined >>>>>>> C3 not defined >>>>>>> C4 not defined >>>>>>> C5 not defined >>>>>>> C6 not defined for ARM cores >>>>>>> C7 power-gating >>>>>>> >>>>>>> CC is a CPU cluster C state: >>>>>>> >>>>>>> CC1 cluster clock gated >>>>>>> CC2 not defined >>>>>>> CC3 fmax@Vmin: not used prior to Tegra186 >>>>>>> CC4: cluster retention: no longer supported >>>>>>> CC5: not defined >>>>>>> CC6: cluster power gating >>>>>>> CC7: cluster rail gating >>>>>>> >>>>>>> SC is a System C state: >>>>>>> >>>>>>> SC1: not defined >>>>>>> SC2: not defined >>>>>>> SC3: not defined >>>>>>> SC4: not defined >>>>>>> SC5: not defined >>>>>>> SC6: not defined >>>>>>> SC7: VDD_SOC off >>>>>> >>>>>> Hello Peter, >>>>>> >>>>>> But new "drivers/cpuidle/cpuidle-tegra.c" uses exactly that terminology, >>>>>> please see "cpuidle: Refactor and move NVIDIA Tegra20 driver into >>>>>> drivers/cpuidle/" and further patches. Am I missing something? Or do you >>>>>> want the renaming to be a separate patch? >>>>>> >>>>> >>>>> Or maybe you're suggesting to change the names everywhere and not only >>>>> in the cpuidle driver? Please clarify :) >>>> >>>> At least some of the variable and function names still say lp2? >>> >>> The cpuidle driver uses LP2 terminology for everything that comes from >>> the external arch / firmware includes. But it says CC6 for everything >>> that is internal to the driver. So yes, there is a bit of new/old >>> terminology mixing in the code. >>> >>> The arch code / PMC driver / TF firmware are all saying LP2. The LP2 >>> naming is also a part of the device-tree binding. >>> >>> It will be a lot of mess to rename the mach-tegra/pm.c code. I guess >>> eventually it could be moved to drivers/soc/, so maybe it will be better >>> to postpone the renaming until then? >> >> Or maybe add a comment somewhere indicating: >> >> LP2 = CC6 >> LP1 = C1 >> LP0 = SC7 >> >> TF predates the new naming, so that may make some sense. > > Today it should make more sense just to add an explicit comment to the > cpuidle driver that clarifies the new naming (IMHO). I'll prepare v7 > with that change. > > Maybe later on, once more code will be consolidated in > drivers/soc/tegra/, it will become useful to duplicate the clarification > there as well. > > Please let me know if you disagree or think that something better could > be done. BTW, LP3 = C1. I don't think that the new terminology has equivalent to LP1 (CPU cluster power gating + DRAM in self-refresh).
Hello, This series does the following: 1. Unifies Tegra20/30/114 drivers into a single driver and moves it out into common drivers/cpuidle/ directory. 2. Enables CPU cluster power-down idling state on Tegra30. In the end there is a quite nice clean up of the Tegra CPUIDLE drivers and of the Tegra's arch code in general. Please review, thanks! Changelog: v6: - Addressed request from Thierry Reding to change the way patches are organized by making changes in a more incremental manner. - tegra_sleep_cpu() now checks for the secondary CPUs to be offline in the "Make outer_disable() open-coded" patch. v5: - Rebased on a recent linux-next, fixed one minor conflict in Kconfig. - Improved commit's message of the "Support CPU cluster power-down state on Tegra30" patch. - The "Support CPU cluster power-down state on Tegra30" patch is also got split and now there is additional "Make outer_disable() open-coded" patch. - Made minor cosmetic changes to the "Introduce unified driver for NVIDIA Tegra SoCs" patch by improving error message and renaming one variable. v4: - Fixed compilation with !CONFIG_CACHE_L2X0 (and tested that it still works). - Replaced ktime_compare() with ktime_before() in the new driver, for consistency. v3: - Addressed review comments that were made by Jon Hunter to v2 by splitting patches into smaller (and simpler) chunks, better documenting changes in the commit messages and using proper error codes in the code. Warnings are replaced with a useful error messages in the code of "Introduce unified driver for NVIDIA Tegra SoCs" patch. Secondary CPUs parking timeout increased to 100ms because I found that it actually may happen to take more than 1ms if CPU is running on a *very* low frequency. Added diagnostic messages that are reporting Flow Controller state when CPU parking fails. Further polished cpuidle driver's code. The coupled state entering is now aborted if there is a pending SGI (Software Generated Interrupt) because it will be lost after GIC's power-cycling. Like it was done by the old Tegra20 CPUIDLE driver. v2: - Added patches to enable the new cpuidle driver in the defconfigs: ARM: multi_v7_defconfig: Enable Tegra cpuidle driver ARM: tegra: Enable Tegra cpuidle driver in tegra_defconfig - Dropped patches that removed CPUIDLE_FLAG_TIMER_STOP from the idling states because that flag actually doesn't have any negative effects, but still is correct for the case of a local CPU timer on older Tegra SoCs: cpuidle: tegra: Remove CPUIDLE_FLAG_TIMER_STOP from Tegra114/124 idle-state cpuidle: tegra: Remove CPUIDLE_FLAG_TIMER_STOP from all states - The "Add unified driver for NVIDIA Tegra SoCs" patch got more polish. Tegra30 and Terga114 states are now squashed into a single common C7 state (following Parker TRM terminology, see 17.2.2.2 Power Management States), more comments added, etc minor changes. Dmitry Osipenko (18): ARM: tegra: Compile sleep-tegra20/30.S unconditionally ARM: tegra: Add tegra_pm_park_secondary_cpu() ARM: tegra: Remove pen-locking from cpuidle-tegra20 ARM: tegra: Change tegra_set_cpu_in_lp2() type to void ARM: tegra: Propagate error from tegra_idle_lp2_last() ARM: tegra: Expose PM functions required for new cpuidle driver ARM: tegra: Rename some of the newly exposed PM functions ARM: tegra: Make outer_disable() open-coded clk: tegra: Add missing stubs for the case of !CONFIG_PM_SLEEP arm: tegra20: cpuidle: Handle case where secondary CPU hangs on entering LP2 arm: tegra20: cpuidle: Make abort_flag atomic arm: tegra20/30: cpuidle: Remove unnecessary memory barrier cpuidle: Refactor and move NVIDIA Tegra20 driver into drivers/cpuidle/ cpuidle: tegra: Squash Tegra30 driver into the common driver cpuidle: tegra: Support CPU cluster power-down state on Tegra30 cpuidle: tegra: Squash Tegra114 driver into the common driver ARM: multi_v7_defconfig: Enable Tegra cpuidle driver ARM: tegra: Enable Tegra cpuidle driver in tegra_defconfig arch/arm/configs/multi_v7_defconfig | 1 + arch/arm/configs/tegra_defconfig | 1 + arch/arm/mach-tegra/Makefile | 19 +- arch/arm/mach-tegra/cpuidle-tegra114.c | 89 ----- arch/arm/mach-tegra/cpuidle-tegra20.c | 212 ----------- arch/arm/mach-tegra/cpuidle-tegra30.c | 132 ------- arch/arm/mach-tegra/cpuidle.c | 50 --- arch/arm/mach-tegra/cpuidle.h | 21 -- arch/arm/mach-tegra/irq.c | 3 +- arch/arm/mach-tegra/pm.c | 54 +-- arch/arm/mach-tegra/pm.h | 4 - arch/arm/mach-tegra/reset-handler.S | 11 - arch/arm/mach-tegra/reset.h | 9 +- arch/arm/mach-tegra/sleep-tegra20.S | 170 --------- arch/arm/mach-tegra/sleep-tegra30.S | 6 +- arch/arm/mach-tegra/sleep.h | 15 - arch/arm/mach-tegra/tegra.c | 7 +- drivers/cpuidle/Kconfig.arm | 8 + drivers/cpuidle/Makefile | 1 + drivers/cpuidle/cpuidle-tegra.c | 357 ++++++++++++++++++ include/linux/clk/tegra.h | 13 + include/soc/tegra/cpuidle.h | 2 +- .../mach-tegra => include/soc/tegra}/irq.h | 8 +- include/soc/tegra/pm.h | 31 ++ 24 files changed, 463 insertions(+), 761 deletions(-) delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra114.c delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra20.c delete mode 100644 arch/arm/mach-tegra/cpuidle-tegra30.c delete mode 100644 arch/arm/mach-tegra/cpuidle.c delete mode 100644 arch/arm/mach-tegra/cpuidle.h create mode 100644 drivers/cpuidle/cpuidle-tegra.c rename {arch/arm/mach-tegra => include/soc/tegra}/irq.h (59%)