[v5,00/14] Consolidate and improve NVIDIA Tegra CPUIDLE driver(s)
mbox series

Message ID 20190929175952.22690-1-digetx@gmail.com
Headers show
Series
  • Consolidate and improve NVIDIA Tegra CPUIDLE driver(s)
Related show

Message

Dmitry Osipenko Sept. 29, 2019, 5:59 p.m. UTC
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:

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 (14):
  ARM: tegra: Remove cpuidle drivers to replace them with a new driver
  ARM: tegra: Change tegra_set_cpu_in_lp2() type to void
  ARM: tegra: Propagate error from tegra_idle_lp2_last()
  ARM: tegra: Compile sleep-tegra20/30.S unconditionally
  ARM: tegra: Expose PM functions required for new cpuidle driver
  ARM: tegra: Rename some of the newly exposed PM functions
  ARM: tegra: Add tegra_pm_park_secondary_cpu()
  ARM: tegra: Make outer_disable() open-coded
  clk: tegra: Add missing stubs for the case of !CONFIG_PM_SLEEP
  cpuidle: Introduce unified driver for NVIDIA Tegra SoCs
  cpuidle: tegra: Support CPU cluster power-down state on Tegra30
  ARM: tegra: Create simple platform device for cpuidle 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                  |  23 +-
 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                      |  50 +--
 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               | 349 ++++++++++++++++++
 drivers/soc/tegra/Kconfig                     |   1 -
 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 ++
 25 files changed, 453 insertions(+), 764 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%)

Comments

Thierry Reding Sept. 30, 2019, 8:26 a.m. UTC | #1
On Sun, Sep 29, 2019 at 08:59:38PM +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!

I generally like this series and it looks really good from a diffstat
point of view. However, removing existing drivers completely and then
incrementally add a new one make this impossible to review.

If you think about it, it also makes it really difficult to find what
went wrong if at any point in the future we find a regression caused by
the new driver. A bisection will always point at the commit that removes
the old driver because between that and the point where you add the new
driver, CPU idle just doesn't work at all anymore.

While I understand that it's very convenient to just throw away old code
and rewrite it from scratch, it's also impractical (and a little rude).
It's not how we do things in the kernel. Unless maybe under specific
circumstances.

Can you please try and make this a little more iterative? At the very
least I'd expect a series where you do all the preliminary work in
preparatory patches and then replace the old driver by the new driver in
a single patch. That way at least there will be an unambiguous commit in
a bisection.

Ideally, you'd also break up that last conversion patch into smaller
incremental patches to make it easier for people to review. Remember
that your chances to attract reviewers increases if you make the patches
easy to review, which means your patches should be small, logical
changes that (ideally) are obviously correct.

Thierry

> Changelog:
> 
> 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 (14):
>   ARM: tegra: Remove cpuidle drivers to replace them with a new driver
>   ARM: tegra: Change tegra_set_cpu_in_lp2() type to void
>   ARM: tegra: Propagate error from tegra_idle_lp2_last()
>   ARM: tegra: Compile sleep-tegra20/30.S unconditionally
>   ARM: tegra: Expose PM functions required for new cpuidle driver
>   ARM: tegra: Rename some of the newly exposed PM functions
>   ARM: tegra: Add tegra_pm_park_secondary_cpu()
>   ARM: tegra: Make outer_disable() open-coded
>   clk: tegra: Add missing stubs for the case of !CONFIG_PM_SLEEP
>   cpuidle: Introduce unified driver for NVIDIA Tegra SoCs
>   cpuidle: tegra: Support CPU cluster power-down state on Tegra30
>   ARM: tegra: Create simple platform device for cpuidle 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                  |  23 +-
>  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                      |  50 +--
>  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               | 349 ++++++++++++++++++
>  drivers/soc/tegra/Kconfig                     |   1 -
>  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 ++
>  25 files changed, 453 insertions(+), 764 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%)
> 
> -- 
> 2.23.0
>
Dmitry Osipenko Sept. 30, 2019, 6:32 p.m. UTC | #2
30.09.2019 11:26, Thierry Reding пишет:
> On Sun, Sep 29, 2019 at 08:59:38PM +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!
> 
> I generally like this series and it looks really good from a diffstat
> point of view. However, removing existing drivers completely and then
> incrementally add a new one make this impossible to review.
> 
> If you think about it, it also makes it really difficult to find what
> went wrong if at any point in the future we find a regression caused by
> the new driver. A bisection will always point at the commit that removes
> the old driver because between that and the point where you add the new
> driver, CPU idle just doesn't work at all anymore.
> 
> While I understand that it's very convenient to just throw away old code
> and rewrite it from scratch, it's also impractical (and a little rude).
> It's not how we do things in the kernel. Unless maybe under specific
> circumstances.
> 
> Can you please try and make this a little more iterative? At the very
> least I'd expect a series where you do all the preliminary work in
> preparatory patches and then replace the old driver by the new driver in
> a single patch. That way at least there will be an unambiguous commit in
> a bisection.
> 
> Ideally, you'd also break up that last conversion patch into smaller
> incremental patches to make it easier for people to review. Remember
> that your chances to attract reviewers increases if you make the patches
> easy to review, which means your patches should be small, logical
> changes that (ideally) are obviously correct.

Thanks for the detailed explanation, probably this is the same what Jon
was asking to do. Now I see what you're are asking for.