mbox series

[v16,00/40] NVIDIA Tegra power management patches for 5.17

Message ID 20211130232347.950-1-digetx@gmail.com
Headers show
Series NVIDIA Tegra power management patches for 5.17 | expand

Message

Dmitry Osipenko Nov. 30, 2021, 11:23 p.m. UTC
This series adds runtime PM support to Tegra drivers and enables core
voltage scaling for Tegra20/30 SoCs, resolving overheating troubles.

All patches in this series are interdependent and should go via Tegra tree
for simplicity.

Changelog:

v16: - Replaced redundant "context->client" with "client" in gr2d/3d
       RPM patches, which was spotted by Michał Mirosław in v15.

     - Added new patch that consolidates the RPM management of older
       UAPI code path.

     - Added stable tag to "submit: Add missing pm_runtime_mark_last_busy()"
       patch and reordered it such that it could be backported without merge
       conflicts.

v15: - Added r-b from Ulf Hansson to "soc/tegra: Enable runtime PM during
       OPP state-syncing" patch and added extra sanity-check to this patch
       which ensures that RPM is indeed enabled.

     - Fixed double RPM-disable on unbind for drivers that used
       devm_pm_runtime_enable() + pm_runtime_force_suspend().

     - Added link with additional info to commit message of "regulators:
       Prepare for suspend" patch.

v14: - Fixed missing runtime PM syncing on removal of drivers, which was
       spotted by Ulf Hansson in v13.

     - clk-device driver now resumes RPM on system suspend instead of
       preparing clock which it backs. This was suggested by Ulf Hansson.

     - clk-device driver now syncs power domain performance unconditionally
       during driver's probe time since GENPD API allows to do this now.
       It was spotted by Ulf Hansson.

     - Added new "Enable runtime PM during OPP state-syncing" patch, which
       allows drivers to sync state at any time. Previously drivers were
       obligated to take care of enabling RPM at the "right" time.

     - Moved runtime PM initialization/uninitialization of DRM drivers that
       use host1x channel to host1x client init/deinit phase. I noticed that
       there is UAF problem because RPM-suspend callback waits until channel
       is idling and channel is already released/freed during driver's removal
       phase.

     - Added system suspend support to the new NVDEC DRM driver.

     - Added missing pm_runtime_mark_last_busy() to DRM driver.

     - Corrected VDE GENPD patch which previously made video decoder clock
       always-enabled by mistake if legacy PD code path was used. It was
       spotted while we were testing VDE on Tegra114 that doesn't support
       GENPD yet.

     - Added ack from Peter Chen to the USB patch that he gave to v13.

     - Changed OPP table names in accordance to the new naming scheme
       required by the recent core OPP binding.

     - Added 500MHz memory OPP entry used by ASUS Transformer tablets.

v13: - Fixed compile-test error reported by build bot by reverting the
       mmc/ patch to v11. The sdhci_suspend/resume_host() functions aren't
       available with the disabled CONFIG_PM_SLEEP, some code needs the
       ifdef.

     - Added last r-b from Rob Herring for the DT patches.

     - Corrected clk/ PM domain-support patch by not using the
       devm_tegra_core_dev_init_opp_table_common() helper, which I
       utilized in v12. The clk driver implements its own power domain
       state syncing and common helper shouldn't be used. This fixes driver
       probing for some clocks on some devices. It was reported by
       Svyatoslav Ryhel for PLLE OPP error on T30 Asus Transformer tablet.

v12: - Added r-b from Rob Herring to the host1x binding patch.

     - Added acks from Hans Verkuil to the video decoder patches.

     - In the v11 changelog I forgot to mention that the clk-binding
       patch was also changed with a corrected regex pattern and removed
       'clocks' sub-node. This patch needs r-b or ack too.

     - Added new "Rename 3d power domains" patch to match the DT schema
       naming requirement. Thanks to David Heidelberg for spotting this
       problem.

     - Replaced #ifdef CONFIG_PM_SLEEP with maybe_unused in the MMC patch
       to make code cleaner.

v11: - Added acks and r-b from Rob Herring, Mark Brown and Miquel Raynal
       that were given to v8.

     - Corrected order of the new memory controller reset entry in
       device-trees and host1x DT binding patch, which was requested by
       Rob Herring.

     - Switched consumer drivers to use power domain state syncing done
       by new Tegra's common OPP-initialization helper.

     - Made use of new devm_pm_runtime_enable() helper that was added to
       v5.15 kernel, where appropriate.

     - Added "fuse: Use resource-managed helpers" patch.

     - Converted Tegra20/30 clk drivers to a proper platform drivers,
       which was requested by Thierry Reding.

     - Removed clk-bulk API usage from the MMC patch, which was requested
       by Thierry Reding.

     - Changed CORE power domain name to "core" in a new patch
       "Change name of core power domain".

     - Misc small fixes for problems that I found since v8, like couple
       typos in error code paths and restored working RPM for Tegra DRM
       UAPI v1 that was removed in v8 by accident.

v9-v10: Figured out remaining GENPD API changes with Ulf Hansson and
        Viresh Kumar. The OPP-sync helper that was used in v8 isn't needed
        anymore because GENPD API now allows consumer drivers to
        init rpm_pstate of power domains.

v8: - Added new generic dev_pm_opp_sync() helper that syncs OPP state with
      hardware. All drivers changed to use it. This replaces GENPD attach_dev
      callback hacks that were used in v7.

    - Added new patch patch "soc/tegra: regulators: Prepare for suspend"
      that fixes dying Tegra20 SoC after enabling VENC power domain during
      resume from suspend. It matches to what downstream kernel does on
      suspend/resume.

    - After a second thought, I dropped patches which added RPM to memory
      drivers since hardware is always-on and RPM not needed.

    - Replaced the "dummy host1x driver" patch with new "Disable unused
      host1x hardware" patch, since it's a cleaner solution.

Dmitry Osipenko (40):
  soc/tegra: Enable runtime PM during OPP state-syncing
  soc/tegra: Add devm_tegra_core_dev_init_opp_table_common()
  soc/tegra: Don't print error message when OPPs not available
  dt-bindings: clock: tegra-car: Document new clock sub-nodes
  clk: tegra: Support runtime PM and power domain
  dt-bindings: host1x: Document OPP and power domain properties
  dt-bindings: host1x: Document Memory Client resets of Host1x, GR2D and
    GR3D
  gpu: host1x: Add initial runtime PM and OPP support
  gpu: host1x: Add host1x_channel_stop()
  drm/tegra: submit: Add missing pm_runtime_mark_last_busy()
  drm/tegra: dc: Support OPP and SoC core voltage scaling
  drm/tegra: hdmi: Add OPP support
  drm/tegra: gr2d: Support generic power domain and runtime PM
  drm/tegra: gr3d: Support generic power domain and runtime PM
  drm/tegra: vic: Stop channel on suspend
  drm/tegra: nvdec: Stop channel on suspend
  drm/tegra: submit: Remove pm_runtime_enabled() checks
  drm/tegra: Consolidate runtime PM management of older UAPI codepath
  usb: chipidea: tegra: Add runtime PM and OPP support
  bus: tegra-gmi: Add runtime PM and OPP support
  pwm: tegra: Add runtime PM and OPP support
  mmc: sdhci-tegra: Add runtime PM and OPP support
  mtd: rawnand: tegra: Add runtime PM and OPP support
  spi: tegra20-slink: Add OPP support
  media: dt: bindings: tegra-vde: Convert to schema
  media: dt: bindings: tegra-vde: Document OPP and power domain
  media: staging: tegra-vde: Support generic power domain
  soc/tegra: fuse: Reset hardware
  soc/tegra: fuse: Use resource-managed helpers
  soc/tegra: regulators: Prepare for suspend
  soc/tegra: pmc: Rename 3d power domains
  soc/tegra: pmc: Rename core power domain
  soc/tegra: pmc: Enable core domain support for Tegra20 and Tegra30
  ARM: tegra: Rename CPU and EMC OPP table device-tree nodes
  ARM: tegra: Add 500MHz entry to Tegra30 memory OPP table
  ARM: tegra: Add OPP tables and power domains to Tegra20 device-trees
  ARM: tegra: Add OPP tables and power domains to Tegra30 device-trees
  ARM: tegra: Add Memory Client resets to Tegra20 GR2D, GR3D and Host1x
  ARM: tegra: Add Memory Client resets to Tegra30 GR2D, GR3D and Host1x
  ARM: tegra20/30: Disable unused host1x hardware

 .../bindings/clock/nvidia,tegra20-car.yaml    |   37 +
 .../display/tegra/nvidia,tegra20-host1x.txt   |   53 +
 .../bindings/media/nvidia,tegra-vde.txt       |   64 -
 .../bindings/media/nvidia,tegra-vde.yaml      |  119 ++
 arch/arm/boot/dts/tegra124-apalis-emc.dtsi    |    4 +-
 .../arm/boot/dts/tegra124-jetson-tk1-emc.dtsi |    4 +-
 arch/arm/boot/dts/tegra124-nyan-big-emc.dtsi  |    8 +-
 .../arm/boot/dts/tegra124-nyan-blaze-emc.dtsi |    8 +-
 .../boot/dts/tegra124-peripherals-opp.dtsi    |  140 +-
 .../boot/dts/tegra20-acer-a500-picasso.dts    |    5 +-
 arch/arm/boot/dts/tegra20-colibri.dtsi        |    5 +-
 .../boot/dts/tegra20-cpu-opp-microvolt.dtsi   |   82 +-
 arch/arm/boot/dts/tegra20-cpu-opp.dtsi        |   82 +-
 arch/arm/boot/dts/tegra20-harmony.dts         |    3 +-
 arch/arm/boot/dts/tegra20-paz00.dts           |    3 +-
 .../arm/boot/dts/tegra20-peripherals-opp.dtsi |  949 +++++++++++-
 arch/arm/boot/dts/tegra20-seaboard.dts        |    3 +-
 arch/arm/boot/dts/tegra20-tamonten.dtsi       |    3 +-
 arch/arm/boot/dts/tegra20-trimslice.dts       |    9 +
 arch/arm/boot/dts/tegra20-ventana.dts         |    1 +
 arch/arm/boot/dts/tegra20.dtsi                |  118 +-
 .../tegra30-asus-nexus7-grouper-common.dtsi   |    1 +
 ...30-asus-nexus7-grouper-memory-timings.dtsi |   12 +-
 arch/arm/boot/dts/tegra30-beaver.dts          |    1 +
 arch/arm/boot/dts/tegra30-cardhu.dtsi         |    1 +
 arch/arm/boot/dts/tegra30-colibri.dtsi        |   17 +-
 .../boot/dts/tegra30-cpu-opp-microvolt.dtsi   |  144 +-
 arch/arm/boot/dts/tegra30-cpu-opp.dtsi        |  144 +-
 arch/arm/boot/dts/tegra30-ouya.dts            |    5 +-
 .../arm/boot/dts/tegra30-peripherals-opp.dtsi | 1373 ++++++++++++++++-
 arch/arm/boot/dts/tegra30.dtsi                |  175 ++-
 drivers/bus/tegra-gmi.c                       |   50 +-
 drivers/clk/tegra/Makefile                    |    1 +
 drivers/clk/tegra/clk-device.c                |  199 +++
 drivers/clk/tegra/clk-pll.c                   |    2 +-
 drivers/clk/tegra/clk-super.c                 |    2 +-
 drivers/clk/tegra/clk-tegra20.c               |   77 +-
 drivers/clk/tegra/clk-tegra30.c               |  116 +-
 drivers/clk/tegra/clk.c                       |   75 +-
 drivers/clk/tegra/clk.h                       |    2 +
 drivers/gpu/drm/tegra/dc.c                    |   79 +
 drivers/gpu/drm/tegra/dc.h                    |    2 +
 drivers/gpu/drm/tegra/drm.c                   |   11 +-
 drivers/gpu/drm/tegra/gr2d.c                  |  174 ++-
 drivers/gpu/drm/tegra/gr3d.c                  |  353 ++++-
 drivers/gpu/drm/tegra/hdmi.c                  |   16 +-
 drivers/gpu/drm/tegra/nvdec.c                 |   43 +-
 drivers/gpu/drm/tegra/submit.c                |   14 +-
 drivers/gpu/drm/tegra/vic.c                   |   48 +-
 drivers/gpu/host1x/channel.c                  |    8 +
 drivers/gpu/host1x/debug.c                    |   15 +
 drivers/gpu/host1x/dev.c                      |  150 +-
 drivers/gpu/host1x/dev.h                      |    3 +-
 drivers/gpu/host1x/hw/channel_hw.c            |   44 +-
 drivers/gpu/host1x/intr.c                     |    3 -
 drivers/gpu/host1x/syncpt.c                   |    5 +-
 drivers/mmc/host/sdhci-tegra.c                |   81 +-
 drivers/mtd/nand/raw/tegra_nand.c             |   58 +-
 drivers/pwm/pwm-tegra.c                       |   82 +-
 drivers/soc/tegra/common.c                    |   29 +-
 drivers/soc/tegra/fuse/fuse-tegra.c           |   51 +-
 drivers/soc/tegra/fuse/fuse-tegra20.c         |   33 +-
 drivers/soc/tegra/fuse/fuse.h                 |    1 +
 drivers/soc/tegra/pmc.c                       |   14 +-
 drivers/soc/tegra/regulators-tegra20.c        |   99 ++
 drivers/soc/tegra/regulators-tegra30.c        |  122 ++
 drivers/spi/spi-tegra20-slink.c               |    9 +-
 drivers/staging/media/tegra-vde/vde.c         |   63 +-
 drivers/usb/chipidea/ci_hdrc_tegra.c          |   53 +-
 include/linux/host1x.h                        |    1 +
 include/soc/tegra/common.h                    |   15 +
 71 files changed, 4930 insertions(+), 846 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt
 create mode 100644 Documentation/devicetree/bindings/media/nvidia,tegra-vde.yaml
 create mode 100644 drivers/clk/tegra/clk-device.c

Comments

Thierry Reding Dec. 15, 2021, 3:55 p.m. UTC | #1
On Wed, Dec 01, 2021 at 02:23:07AM +0300, Dmitry Osipenko wrote:
> This series adds runtime PM support to Tegra drivers and enables core
> voltage scaling for Tegra20/30 SoCs, resolving overheating troubles.
> 
> All patches in this series are interdependent and should go via Tegra tree
> for simplicity.

So these can be applied in any order without breaking anything?

Thierry
Dmitry Osipenko Dec. 15, 2021, 4:11 p.m. UTC | #2
15.12.2021 18:55, Thierry Reding пишет:
> On Wed, Dec 01, 2021 at 02:23:07AM +0300, Dmitry Osipenko wrote:
>> This series adds runtime PM support to Tegra drivers and enables core
>> voltage scaling for Tegra20/30 SoCs, resolving overheating troubles.
>>
>> All patches in this series are interdependent and should go via Tegra tree
>> for simplicity.
> 
> So these can be applied in any order without breaking anything?

Please notice that the word is *inter* dependent, not *in* dependent.

There is a build dependency for the patches. The first two "soc/tegra"
must be applied first.

The "soc/tegra: pmc: Enable core domain support for Tegra20 and Tegra30"
*must* be the last applied patch if we want to preserve bisectability.
The core voltage scaling can be enabled only once all the drivers got
the power management support.

The rest could be applied out-of-order.
Thierry Reding Dec. 16, 2021, 1:14 p.m. UTC | #3
On Wed, Dec 15, 2021 at 07:11:53PM +0300, Dmitry Osipenko wrote:
> 15.12.2021 18:55, Thierry Reding пишет:
> > On Wed, Dec 01, 2021 at 02:23:07AM +0300, Dmitry Osipenko wrote:
> >> This series adds runtime PM support to Tegra drivers and enables core
> >> voltage scaling for Tegra20/30 SoCs, resolving overheating troubles.
> >>
> >> All patches in this series are interdependent and should go via Tegra tree
> >> for simplicity.
> > 
> > So these can be applied in any order without breaking anything?
> 
> Please notice that the word is *inter* dependent, not *in* dependent.
> 
> There is a build dependency for the patches. The first two "soc/tegra"
> must be applied first.

Okay, so I've separated the first two patches out into a separate stable
branch that I can share between the Tegra and drm/tegra trees to pull in
the build dependency and then I've applied the driver patches to those
two trees and I've verified that the two branches build correctly. I've
not done any runtime testing, but I'll trust you on that.

> The "soc/tegra: pmc: Enable core domain support for Tegra20 and Tegra30"
> *must* be the last applied patch if we want to preserve bisectability.
> The core voltage scaling can be enabled only once all the drivers got
> the power management support.
> 
> The rest could be applied out-of-order.

One last remaining question: I don't think I can apply that one patch if
it requires that all the others are enabled first because it would
basically create a circular dependency.

Can I pick up the final 7 patches (the DT ones) independently of that
one patch without things breaking? If so, one option we could try is to
wait for both Tegra and drm/tegra trees to get merged into v5.17-rc1 and
then send that one patch (which is only a 4-line diff) right after
v5.17-rc1 so that it makes it into v5.17-rc2. That avoids the circular
dependency and should get everything enabled for v5.17.

Do you see any problems with that?

Thierry
Dmitry Osipenko Dec. 16, 2021, 2:19 p.m. UTC | #4
16.12.2021 16:14, Thierry Reding пишет:
> On Wed, Dec 15, 2021 at 07:11:53PM +0300, Dmitry Osipenko wrote:
>> 15.12.2021 18:55, Thierry Reding пишет:
>>> On Wed, Dec 01, 2021 at 02:23:07AM +0300, Dmitry Osipenko wrote:
>>>> This series adds runtime PM support to Tegra drivers and enables core
>>>> voltage scaling for Tegra20/30 SoCs, resolving overheating troubles.
>>>>
>>>> All patches in this series are interdependent and should go via Tegra tree
>>>> for simplicity.
>>>
>>> So these can be applied in any order without breaking anything?
>>
>> Please notice that the word is *inter* dependent, not *in* dependent.
>>
>> There is a build dependency for the patches. The first two "soc/tegra"
>> must be applied first.
> 
> Okay, so I've separated the first two patches out into a separate stable
> branch that I can share between the Tegra and drm/tegra trees to pull in
> the build dependency and then I've applied the driver patches to those
> two trees and I've verified that the two branches build correctly. I've
> not done any runtime testing, but I'll trust you on that.

I only compile-tested VIC and NVDEC drivers, but they should be okay,
and thus, everything should be good.

>> The "soc/tegra: pmc: Enable core domain support for Tegra20 and Tegra30"
>> *must* be the last applied patch if we want to preserve bisectability.
>> The core voltage scaling can be enabled only once all the drivers got
>> the power management support.
>>
>> The rest could be applied out-of-order.
> 
> One last remaining question: I don't think I can apply that one patch if
> it requires that all the others are enabled first because it would
> basically create a circular dependency.
> 
> Can I pick up the final 7 patches (the DT ones) independently of that
> one patch without things breaking? If so, one option we could try is to
> wait for both Tegra and drm/tegra trees to get merged into v5.17-rc1 and
> then send that one patch (which is only a 4-line diff) right after
> v5.17-rc1 so that it makes it into v5.17-rc2. That avoids the circular
> dependency and should get everything enabled for v5.17.
> 
> Do you see any problems with that?
Deferring that one patch till v5.17-rc2 will work, thank you.
Uwe Kleine-König Feb. 21, 2022, 8:17 a.m. UTC | #5
Hello,

On Wed, Dec 01, 2021 at 02:23:28AM +0300, Dmitry Osipenko wrote:
> The PWM on Tegra belongs to the core power domain and we're going to
> enable GENPD support for the core domain. Now PWM must be resumed using
> runtime PM API in order to initialize the PWM power state. The PWM clock
> rate must be changed using OPP API that will reconfigure the power domain
> performance state in accordance to the rate. Add runtime PM and OPP
> support to the PWM driver.
> 
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/pwm/pwm-tegra.c | 82 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 64 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 11a10b575ace..18cf974ac776 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -42,12 +42,16 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_opp.h>
>  #include <linux/pwm.h>
>  #include <linux/platform_device.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  #include <linux/reset.h>
>  
> +#include <soc/tegra/common.h>
> +
>  #define PWM_ENABLE	(1 << 31)
>  #define PWM_DUTY_WIDTH	8
>  #define PWM_DUTY_SHIFT	16
> @@ -145,7 +149,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  		required_clk_rate =
>  			(NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
>  
> -		err = clk_set_rate(pc->clk, required_clk_rate);
> +		err = dev_pm_opp_set_rate(pc->dev, required_clk_rate);
>  		if (err < 0)
>  			return -EINVAL;
>  
> @@ -181,8 +185,8 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * before writing the register. Otherwise, keep it enabled.
>  	 */
>  	if (!pwm_is_enabled(pwm)) {
> -		err = clk_prepare_enable(pc->clk);
> -		if (err < 0)
> +		err = pm_runtime_resume_and_get(pc->dev);
> +		if (err)
>  			return err;
>  	} else
>  		val |= PWM_ENABLE;
> @@ -193,7 +197,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * If the PWM is not enabled, turn the clock off again to save power.
>  	 */
>  	if (!pwm_is_enabled(pwm))
> -		clk_disable_unprepare(pc->clk);
> +		pm_runtime_put(pc->dev);
>  
>  	return 0;
>  }
> @@ -204,8 +208,8 @@ static int tegra_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	int rc = 0;
>  	u32 val;
>  
> -	rc = clk_prepare_enable(pc->clk);
> -	if (rc < 0)
> +	rc = pm_runtime_resume_and_get(pc->dev);
> +	if (rc)
>  		return rc;
>  
>  	val = pwm_readl(pc, pwm->hwpwm);
> @@ -224,7 +228,7 @@ static void tegra_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	val &= ~PWM_ENABLE;
>  	pwm_writel(pc, pwm->hwpwm, val);
>  
> -	clk_disable_unprepare(pc->clk);
> +	pm_runtime_put_sync(pc->dev);
>  }
>  
>  static const struct pwm_ops tegra_pwm_ops = {
> @@ -256,11 +260,20 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	if (IS_ERR(pwm->clk))
>  		return PTR_ERR(pwm->clk);
>  
> +	ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_enable(&pdev->dev);
> +	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
>  	/* Set maximum frequency of the IP */
> -	ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
> +	ret = dev_pm_opp_set_rate(pwm->dev, pwm->soc->max_frequency);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to set max frequency: %d\n", ret);
> -		return ret;
> +		goto put_pm;
>  	}
>  
>  	/*
> @@ -278,7 +291,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	if (IS_ERR(pwm->rst)) {
>  		ret = PTR_ERR(pwm->rst);
>  		dev_err(&pdev->dev, "Reset control is not found: %d\n", ret);
> -		return ret;
> +		goto put_pm;
>  	}
>  
>  	reset_control_deassert(pwm->rst);
> @@ -291,10 +304,16 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
>  		reset_control_assert(pwm->rst);
> -		return ret;
> +		goto put_pm;
>  	}
>  
> +	pm_runtime_put(&pdev->dev);
> +
>  	return 0;
> +put_pm:
> +	pm_runtime_put_sync_suspend(&pdev->dev);
> +	pm_runtime_force_suspend(&pdev->dev);
> +	return ret;
>  }
>  
>  static int tegra_pwm_remove(struct platform_device *pdev)
> @@ -305,20 +324,44 @@ static int tegra_pwm_remove(struct platform_device *pdev)
>  
>  	reset_control_assert(pc->rst);
>  
> +	pm_runtime_force_suspend(&pdev->dev);
> +
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> -static int tegra_pwm_suspend(struct device *dev)
> +static int __maybe_unused tegra_pwm_runtime_suspend(struct device *dev)
>  {
> -	return pinctrl_pm_select_sleep_state(dev);
> +	struct tegra_pwm_chip *pc = dev_get_drvdata(dev);
> +	int err;
> +
> +	clk_disable_unprepare(pc->clk);
> +
> +	err = pinctrl_pm_select_sleep_state(dev);
> +	if (err) {
> +		clk_prepare_enable(pc->clk);
> +		return err;
> +	}
> +
> +	return 0;
>  }
>  
> -static int tegra_pwm_resume(struct device *dev)
> +static int __maybe_unused tegra_pwm_runtime_resume(struct device *dev)
>  {
> -	return pinctrl_pm_select_default_state(dev);
> +	struct tegra_pwm_chip *pc = dev_get_drvdata(dev);
> +	int err;
> +
> +	err = pinctrl_pm_select_default_state(dev);
> +	if (err)
> +		return err;
> +
> +	err = clk_prepare_enable(pc->clk);
> +	if (err) {
> +		pinctrl_pm_select_sleep_state(dev);
> +		return err;
> +	}
> +
> +	return 0;
>  }
> -#endif
>  
>  static const struct tegra_pwm_soc tegra20_pwm_soc = {
>  	.num_channels = 4,
> @@ -344,7 +387,10 @@ static const struct of_device_id tegra_pwm_of_match[] = {
>  MODULE_DEVICE_TABLE(of, tegra_pwm_of_match);
>  
>  static const struct dev_pm_ops tegra_pwm_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(tegra_pwm_suspend, tegra_pwm_resume)
> +	SET_RUNTIME_PM_OPS(tegra_pwm_runtime_suspend, tegra_pwm_runtime_resume,
> +			   NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				pm_runtime_force_resume)
>  };
>  
>  static struct platform_driver tegra_pwm_driver = {

I admit to not completely understand the effects of this patch, but I
don't see a problem either. So for me this patch is OK:

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I spot a problem, it's not introduced by this patch however: If the
consumer of the PWM didn't stop the hardware, the suspend should IMHO be
prevented.

I wonder if the patches in this series go in in one go via an ARM or
Tegra tree, or each patch via its respective maintainer tree.

Best regards
Uwe
Dmitry Osipenko Feb. 21, 2022, 9:53 a.m. UTC | #6
Hello Uwe,

21.02.2022 11:17, Uwe Kleine-König пишет:
>> @@ -344,7 +387,10 @@ static const struct of_device_id tegra_pwm_of_match[] = {
>>  MODULE_DEVICE_TABLE(of, tegra_pwm_of_match);
>>  
>>  static const struct dev_pm_ops tegra_pwm_pm_ops = {
>> -	SET_SYSTEM_SLEEP_PM_OPS(tegra_pwm_suspend, tegra_pwm_resume)
>> +	SET_RUNTIME_PM_OPS(tegra_pwm_runtime_suspend, tegra_pwm_runtime_resume,
>> +			   NULL)
>> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> +				pm_runtime_force_resume)
>>  };
>>  
>>  static struct platform_driver tegra_pwm_driver = {
> I admit to not completely understand the effects of this patch, but I
> don't see a problem either. So for me this patch is OK:
> 
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> I spot a problem, it's not introduced by this patch however: If the
> consumer of the PWM didn't stop the hardware, the suspend should IMHO be
> prevented.

Why? The PWM driver itself will stop the h/w on suspend.

> I wonder if the patches in this series go in in one go via an ARM or
> Tegra tree, or each patch via its respective maintainer tree.

This series, including this patch, was already applied to 5.17 via the
tegra/soc tree. No action is needed anymore.
Uwe Kleine-König Feb. 21, 2022, 1:37 p.m. UTC | #7
Hello,

On Mon, Feb 21, 2022 at 12:53:58PM +0300, Dmitry Osipenko wrote:
> 21.02.2022 11:17, Uwe Kleine-König пишет:
> >> @@ -344,7 +387,10 @@ static const struct of_device_id tegra_pwm_of_match[] = {
> >>  MODULE_DEVICE_TABLE(of, tegra_pwm_of_match);
> >>  
> >>  static const struct dev_pm_ops tegra_pwm_pm_ops = {
> >> -	SET_SYSTEM_SLEEP_PM_OPS(tegra_pwm_suspend, tegra_pwm_resume)
> >> +	SET_RUNTIME_PM_OPS(tegra_pwm_runtime_suspend, tegra_pwm_runtime_resume,
> >> +			   NULL)
> >> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> >> +				pm_runtime_force_resume)
> >>  };
> >>  
> >>  static struct platform_driver tegra_pwm_driver = {
> > I admit to not completely understand the effects of this patch, but I
> > don't see a problem either. So for me this patch is OK:
> > 
> > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > I spot a problem, it's not introduced by this patch however: If the
> > consumer of the PWM didn't stop the hardware, the suspend should IMHO be
> > prevented.
> 
> Why? The PWM driver itself will stop the h/w on suspend.

Stopping the PWM might be bad. Only the consumer can know if it's ok to
stop the PWM on suspend. If so the consumer should stop the PWM in their
suspend callback and the PWM should prevent suspend if it wasn't
stopped.

> > I wonder if the patches in this series go in in one go via an ARM or
> > Tegra tree, or each patch via its respective maintainer tree.
> 
> This series, including this patch, was already applied to 5.17 via the
> tegra/soc tree. No action is needed anymore.

Ah, I missed that, thanks.

Best regards
Uwe