LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Thierry Reding <thierry.reding@gmail.com>
To: Dmitry Osipenko <digetx@gmail.com>
Cc: "Jonathan Hunter" <jonathanh@nvidia.com>,
	"Ulf Hansson" <ulf.hansson@linaro.org>,
	"Viresh Kumar" <vireshk@kernel.org>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"Peter De Schrijver" <pdeschrijver@nvidia.com>,
	"Mikko Perttunen" <mperttunen@nvidia.com>,
	"Peter Chen" <peter.chen@kernel.org>,
	"Mark Brown" <broonie@kernel.org>,
	"Lee Jones" <lee.jones@linaro.org>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Nishanth Menon" <nm@ti.com>,
	"Vignesh Raghavendra" <vigneshr@ti.com>,
	"Richard Weinberger" <richard@nod.at>,
	"Miquel Raynal" <miquel.raynal@bootlin.com>,
	"Lucas Stach" <dev@lynxeye.de>, "Stefan Agner" <stefan@agner.ch>,
	"Adrian Hunter" <adrian.hunter@intel.com>,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Michael Turquette" <mturquette@baylibre.com>,
	linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-pm@vger.kernel.org, linux-usb@vger.kernel.org,
	linux-staging@lists.linux.dev, linux-spi@vger.kernel.org,
	linux-pwm@vger.kernel.org, linux-mtd@lists.infradead.org,
	linux-mmc@vger.kernel.org, linux-media@vger.kernel.org,
	dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
	linux-clk@vger.kernel.org
Subject: Re: [PATCH v8 07/34] clk: tegra: Support runtime PM and power domain
Date: Fri, 20 Aug 2021 13:42:05 +0200	[thread overview]
Message-ID: <YR+VDZzTihmpENp6@orome.fritz.box> (raw)
In-Reply-To: <e17bbe8d-7c0f-fc3d-03c7-d75c54c24a43@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 7960 bytes --]

On Fri, Aug 20, 2021 at 01:09:46AM +0300, Dmitry Osipenko wrote:
> 19.08.2021 19:54, Thierry Reding пишет:
> > On Wed, Aug 18, 2021 at 08:11:03PM +0300, Dmitry Osipenko wrote:
> >> 18.08.2021 19:42, Thierry Reding пишет:
> >>> On Wed, Aug 18, 2021 at 06:05:21PM +0300, Dmitry Osipenko wrote:
> >>>> 18.08.2021 17:07, Thierry Reding пишет:
> >>>>> On Tue, Aug 17, 2021 at 04:27:27AM +0300, Dmitry Osipenko wrote:
> >>>>> [...]
> >>>>>> +struct clk *tegra_clk_register(struct clk_hw *hw)
> >>>>>> +{
> >>>>>> +	struct platform_device *pdev;
> >>>>>> +	struct device *dev = NULL;
> >>>>>> +	struct device_node *np;
> >>>>>> +	const char *dev_name;
> >>>>>> +
> >>>>>> +	np = tegra_clk_get_of_node(hw);
> >>>>>> +
> >>>>>> +	if (!of_device_is_available(np))
> >>>>>> +		goto put_node;
> >>>>>> +
> >>>>>> +	dev_name = kasprintf(GFP_KERNEL, "tegra_clk_%s", hw->init->name);
> >>>>>> +	if (!dev_name)
> >>>>>> +		goto put_node;
> >>>>>> +
> >>>>>> +	pdev = of_platform_device_create(np, dev_name, NULL);
> >>>>>> +	if (!pdev) {
> >>>>>> +		pr_err("%s: failed to create device for %pOF\n", __func__, np);
> >>>>>> +		kfree(dev_name);
> >>>>>> +		goto put_node;
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	dev = &pdev->dev;
> >>>>>> +	pm_runtime_enable(dev);
> >>>>>> +put_node:
> >>>>>> +	of_node_put(np);
> >>>>>> +
> >>>>>> +	return clk_register(dev, hw);
> >>>>>> +}
> >>>>>
> >>>>> This looks wrong. Why do we need struct platform_device objects for each
> >>>>> of these clocks? That's going to be a massive amount of platform devices
> >>>>> and they will completely mess up sysfs.
> >>>>
> >>>> RPM works with a device. It's not a massive amount of devices, it's one
> >>>> device for T20 and four devices for T30.
> >>>
> >>> I'm still not sure I understand why we need to call RPM functions on a
> >>> clock. And even if they are few, it seems wrong to make these platform
> >>> devices.
> >>
> >> Before clock is enabled, we need to raise core voltage. After clock is
> >> disabled, the voltage should be dropped. CCF+RPM takes care of handling
> >> this for us.
> > 
> > That's the part that I do understand. What I don't understand is why a
> > clock needs to be runtime suspend/resumed. Typically we suspend/resume
> > devices, and doing so typically involves disabling/enabling clocks. So
> > I don't understand why the clocks themselves now need to be runtime
> > suspended/resumed.
> 
> CCF provides RPM management for a device that backs clock. When clock
> is enabled, it resumes the backing device.
> 
> RPM, GENPD and OPP frameworks work with a device. We use all these
> frameworks here. Since we don't have a dedicated device for a PLL
> clock, we need to create it in order to leverage the existing generic
> kernel APIs.
> 
> In this case clocks are not runtime suspended/resumed, the device
> which backs clock is suspended/resumed.
> 
> >>> Perhaps they can be simple struct device:s instead? Ideally they would
> >>> also be parented to the CAR so that they appear in the right place in
> >>> the sysfs hierarchy.
> >>
> >> Could you please clarify what do you mean by 'simple struct device:s'?
> >> These clock devices should be OF devices with a of_node and etc,
> >> otherwise we can't use OPP framework.
> > 
> > Perhaps I misunderstand the goal of the OPP framework. My understanding
> > was that this was to attach a table of operating points with a device so
> > that appropriate operating points could be selected and switched to when
> > the workload changes.
> > 
> > Typically these operating points would be roughly a clock rate and a
> > corresponding voltage for a regulator, so that when a certain clock rate
> > is requested, the regulator can be set to the matching voltage.
> > 
> > Hm... so is it that each of these clocks that you want to create a
> > platform device for has its own regulator? Because the patch series only
> > mentions the CORE domain, so I assumed that we would accumulate all the
> > clock rates for the clocks that are part of that CORE domain and then
> > derive a voltage to be supplied to that CORE domain.
> > 
> > But perhaps I just don't understand correctly how this is tied together.
> 
> We don't use regulators, we use power domain that controls regulator.
> GENPD takes care of accumulating performance requests on a per-device
> basis.
> 
> I'm creating platform device for the clocks that require DVFS. These
> clocks don't use regulator, they are attached to the CORE domain.
> GENPD framework manages the performance state, aggregating perf votes
> from each device, i.e. from each clock individually.
> 
> You want to reinvent another layer of aggregation on top of GENPD.
> This doesn't worth the effort, we won't get anything from it, it
> should be a lot of extra complexity for nothing. We will also lose
> from it because pm_genpd_summary won't show you a per-device info.
> 
> domain                          status          children                           performance
>     /device                                             runtime status
> ----------------------------------------------------------------------------------------------
> heg                             on                                                 1000000
>     /devices/soc0/50000000.host1x                       active                     1000000
>     /devices/soc0/50000000.host1x/54140000.gr2d         suspended                  0
> mpe                             off-0                                              0
> vdec                            off-0                                              0
>     /devices/soc0/6001a000.vde                          suspended                  0
> venc                            off-0                                              0
> 3d1                             off-0                                              0
>     /devices/genpd:1:54180000.gr3d                      suspended                  0
> 3d0                             off-0                                              0
>     /devices/genpd:0:54180000.gr3d                      suspended                  0
> core-domain                     on                                                 1000000
>                                                 3d0, 3d1, venc, vdec, mpe, heg
>     /devices/soc0/7d000000.usb                          active                     1000000
>     /devices/soc0/78000400.mmc                          active                     950000
>     /devices/soc0/7000f400.memory-controller            unsupported                1000000
>     /devices/soc0/7000a000.pwm                          active                     1000000
>     /devices/soc0/60006000.clock/tegra_clk_pll_c        active                     1000000
>     /devices/soc0/60006000.clock/tegra_clk_pll_e        suspended                  0
>     /devices/soc0/60006000.clock/tegra_clk_pll_m        active                     1000000
>     /devices/soc0/60006000.clock/tegra_clk_sclk         active                     1000000
> 

I suppose if there's really no good way of doing this other than
providing a struct device, then so be it. I think the cleaned up sysfs
shown in the summary above looks much better than what the original
would've looked like.

Perhaps an additional tweak to that would be to not create platform
devices. Instead, just create struct device. Those really have
everything you need (.of_node, and can be used with RPM and GENPD). As I
mentioned earlier, platform device implies a CPU-memory-mapped bus,
which this clearly isn't. It's kind of a separate "bus" if you want, so
just using struct device directly seems more appropriate.

We did something similar for XUSB pads, see drivers/phy/tegra/xusb.[ch]
for an example of how that was done. I think you can do something
similar here.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2021-08-20 11:42 UTC|newest]

Thread overview: 119+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-17  1:27 [PATCH v8 00/34] NVIDIA Tegra power management patches for 5.16 Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper Dmitry Osipenko
2021-08-17  7:55   ` Viresh Kumar
2021-08-17 15:49     ` Dmitry Osipenko
2021-08-18  3:55       ` Viresh Kumar
2021-08-18  4:12         ` Dmitry Osipenko
2021-08-18  4:29           ` Dmitry Osipenko
2021-08-18  4:30             ` Dmitry Osipenko
2021-08-18  4:34               ` Viresh Kumar
2021-08-18  4:31           ` Viresh Kumar
2021-08-18  4:37             ` Dmitry Osipenko
2021-08-18  4:53               ` Viresh Kumar
2021-08-18  5:21                 ` Dmitry Osipenko
2021-08-18  5:58                   ` Viresh Kumar
2021-08-18  6:00                     ` Viresh Kumar
2021-08-18  6:22                     ` Dmitry Osipenko
2021-08-18  6:27                       ` Viresh Kumar
2021-08-18  8:29                         ` Ulf Hansson
2021-08-18  9:14                           ` Viresh Kumar
2021-08-18  9:41                             ` Ulf Hansson
2021-08-18  9:42                               ` Ulf Hansson
2021-08-18  9:50                               ` Viresh Kumar
2021-08-18 10:08                                 ` Ulf Hansson
2021-08-18 15:43                                   ` Dmitry Osipenko
2021-08-18 15:46                                     ` Dmitry Osipenko
2021-08-19 13:07                                     ` Ulf Hansson
2021-08-19 19:35                                       ` Dmitry Osipenko
2021-08-20  5:07                                         ` Viresh Kumar
2021-08-20 12:42                                         ` Ulf Hansson
2021-08-21 17:34                                           ` Dmitry Osipenko
2021-08-23 10:46                                             ` Ulf Hansson
2021-08-23 15:54                                               ` Dmitry Osipenko
2021-08-18 15:55                               ` Dmitry Osipenko
2021-08-19  6:16                                 ` Viresh Kumar
2021-08-19 14:55                                   ` Ulf Hansson
2021-08-20  5:18                                     ` Viresh Kumar
2021-08-20 12:57                                       ` Ulf Hansson
2021-08-23 20:24                                         ` Dmitry Osipenko
2021-08-24  3:04                                           ` Viresh Kumar
2021-08-22 18:35                                       ` Dmitry Osipenko
2021-08-25 15:41                                         ` Dmitry Osipenko
2021-08-26  2:54                                           ` Viresh Kumar
2021-08-26  2:55                                             ` Viresh Kumar
2021-08-17  1:27 ` [PATCH v8 02/34] soc/tegra: pmc: Disable PMC state syncing Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 03/34] soc/tegra: Don't print error message when OPPs not available Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 04/34] soc/tegra: Add devm_tegra_core_dev_init_opp_table_simple() Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 05/34] soc/tegra: Use dev_pm_opp_sync() Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 06/34] dt-bindings: clock: tegra-car: Document new tegra-clocks sub-node Dmitry Osipenko
2021-08-18  1:15   ` Rob Herring
2021-08-18  1:44     ` Dmitry Osipenko
2021-08-18 13:52       ` Thierry Reding
2021-08-18 15:04         ` Dmitry Osipenko
2021-08-18 13:59   ` Thierry Reding
2021-08-18 15:05     ` Dmitry Osipenko
2021-08-18 16:39       ` Thierry Reding
2021-08-18 16:57         ` Dmitry Osipenko
2021-08-18 17:16           ` Dmitry Osipenko
2021-08-19 16:31           ` Thierry Reding
2021-08-19 22:20             ` Dmitry Osipenko
2021-08-20  2:51             ` Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 07/34] clk: tegra: Support runtime PM and power domain Dmitry Osipenko
2021-08-18 14:07   ` Thierry Reding
2021-08-18 15:05     ` Dmitry Osipenko
2021-08-18 16:42       ` Thierry Reding
2021-08-18 17:11         ` Dmitry Osipenko
2021-08-19 16:54           ` Thierry Reding
2021-08-19 22:09             ` Dmitry Osipenko
2021-08-20 11:42               ` Thierry Reding [this message]
2021-08-20 13:08                 ` Ulf Hansson
2021-08-21 17:45                   ` Dmitry Osipenko
2021-08-23 14:33                     ` Thierry Reding
2021-08-23 18:54                       ` Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 08/34] dt-bindings: host1x: Document OPP and power domain properties Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 09/34] dt-bindings: host1x: Document Memory Client resets of Host1x, GR2D and GR3D Dmitry Osipenko
2021-08-18  1:16   ` Rob Herring
2021-08-18  1:37     ` Dmitry Osipenko
2021-08-18  2:04       ` Dmitry Osipenko
2021-08-18  2:07         ` Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 10/34] gpu: host1x: Add host1x_channel_stop() Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 11/34] gpu: host1x: Add runtime PM and OPP support Dmitry Osipenko
2021-08-17 12:04   ` Ulf Hansson
2021-08-17 14:02     ` Thierry Reding
2021-08-18  8:35       ` Ulf Hansson
2021-08-18 17:24         ` Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 12/34] drm/tegra: dc: Support OPP and SoC core voltage scaling Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 13/34] drm/tegra: hdmi: Add OPP support Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 14/34] drm/tegra: gr2d: Support power management Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 15/34] drm/tegra: gr3d: " Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 16/34] drm/tegra: vic: Support system suspend Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 17/34] usb: chipidea: tegra: Add runtime PM and OPP support Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 18/34] bus: tegra-gmi: " Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 19/34] pwm: tegra: " Dmitry Osipenko
2021-08-19 13:21   ` Thierry Reding
2021-08-19 14:04     ` Ulf Hansson
2021-08-19 16:17       ` Thierry Reding
2021-08-17  1:27 ` [PATCH v8 20/34] mmc: sdhci-tegra: " Dmitry Osipenko
2021-08-19 17:03   ` Thierry Reding
2021-08-19 22:37     ` Dmitry Osipenko
2021-08-20 11:35       ` Thierry Reding
2021-08-25  9:45         ` Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 21/34] mtd: rawnand: tegra: " Dmitry Osipenko
2021-08-17  8:41   ` Miquel Raynal
2021-08-17  1:27 ` [PATCH v8 22/34] spi: tegra20-slink: Add " Dmitry Osipenko
2021-08-17 12:22   ` Mark Brown
2021-08-17 15:53     ` Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 23/34] media: dt: bindings: tegra-vde: Convert to schema Dmitry Osipenko
2021-08-18  1:17   ` Rob Herring
2021-08-17  1:27 ` [PATCH v8 24/34] media: dt: bindings: tegra-vde: Document OPP and power domain Dmitry Osipenko
2021-08-18  1:17   ` Rob Herring
2021-08-17  1:27 ` [PATCH v8 25/34] media: staging: tegra-vde: Support generic power domain and OPP Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 26/34] soc/tegra: fuse: Add OPP support Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 27/34] soc/tegra: fuse: Reset hardware Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 28/34] soc/tegra: regulators: Prepare for suspend Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 29/34] soc/tegra: pmc: Enable core domain support for Tegra20 and Tegra30 Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 30/34] ARM: tegra: Add OPP tables and power domains to Tegra20 device-trees Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 31/34] ARM: tegra: Add OPP tables and power domains to Tegra30 device-trees Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 32/34] ARM: tegra: Add Memory Client resets to Tegra20 GR2D, GR3D and Host1x Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 33/34] ARM: tegra: Add Memory Client resets to Tegra30 " Dmitry Osipenko
2021-08-17  1:27 ` [PATCH v8 34/34] ARM: tegra20/30: Disable unused host1x hardware Dmitry Osipenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YR+VDZzTihmpENp6@orome.fritz.box \
    --to=thierry.reding@gmail.com \
    --cc=adrian.hunter@intel.com \
    --cc=broonie@kernel.org \
    --cc=dev@lynxeye.de \
    --cc=devicetree@vger.kernel.org \
    --cc=digetx@gmail.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jonathanh@nvidia.com \
    --cc=lee.jones@linaro.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-mmc@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=linux-tegra@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=miquel.raynal@bootlin.com \
    --cc=mperttunen@nvidia.com \
    --cc=mturquette@baylibre.com \
    --cc=nm@ti.com \
    --cc=pdeschrijver@nvidia.com \
    --cc=peter.chen@kernel.org \
    --cc=richard@nod.at \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=stefan@agner.ch \
    --cc=u.kleine-koenig@pengutronix.de \
    --cc=ulf.hansson@linaro.org \
    --cc=vigneshr@ti.com \
    --cc=vireshk@kernel.org \
    --subject='Re: [PATCH v8 07/34] clk: tegra: Support runtime PM and power domain' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).