Message-ID: <20190930082645.GF1518582@ulmo>
Date:   Mon, 30 Sep 2019 10:26:45 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Jonathan Hunter <jonathanh@...dia.com>,
        Peter De Schrijver <pdeschrijver@...dia.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        linux-pm@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 00/14] Consolidate and improve NVIDIA Tegra CPUIDLE
 driver(s)

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
> 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists