| Message ID | 20210817012754.8710-1-digetx@gmail.com |
|---|---|
| Headers | show |
| Series | NVIDIA Tegra power management patches for 5.16 | expand |
On 17-08-21, 04:27, Dmitry Osipenko wrote: > Add dev_pm_opp_sync() helper which syncs OPP table with hardware state > and vice versa. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/opp/core.c | 42 +++++++++++++++++++++++++++++++++++++++--- > include/linux/pm_opp.h | 6 ++++++ > 2 files changed, 45 insertions(+), 3 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 5543c54dacc5..18016e49605f 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -939,7 +939,8 @@ static int _set_required_opps(struct device *dev, > return ret; > } > > -static void _find_current_opp(struct device *dev, struct opp_table *opp_table) > +static struct dev_pm_opp * > +_find_current_opp(struct device *dev, struct opp_table *opp_table) > { > struct dev_pm_opp *opp = ERR_PTR(-ENODEV); > unsigned long freq; > @@ -961,7 +962,7 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table) > mutex_unlock(&opp_table->lock); > } > > - opp_table->current_opp = opp; > + return opp; > } > > static int _disable_opp_table(struct device *dev, struct opp_table *opp_table) > @@ -1003,7 +1004,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table, > > /* Find the currently set OPP if we don't know already */ > if (unlikely(!opp_table->current_opp)) > - _find_current_opp(dev, opp_table); > + opp_table->current_opp = _find_current_opp(dev, opp_table); > > old_opp = opp_table->current_opp; > > @@ -2931,3 +2932,38 @@ int dev_pm_opp_sync_regulators(struct device *dev) > return ret; > } > EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators); > + > +/** > + * dev_pm_opp_sync() - Sync OPP state > + * @dev: device for which we do this operation > + * > + * Initialize OPP table accordingly to current clock rate or > + * first available OPP if clock not available for this device. > + * > + * Return: 0 on success or a negative error value. > + */ > +int dev_pm_opp_sync(struct device *dev) > +{ > + struct opp_table *opp_table; > + struct dev_pm_opp *opp; > + int ret = 0; > + > + /* Device may not have OPP table */ > + opp_table = _find_opp_table(dev); > + if (IS_ERR(opp_table)) > + return 0; > + > + if (!_get_opp_count(opp_table)) > + goto put_table; > + > + opp = _find_current_opp(dev, opp_table); > + ret = _set_opp(dev, opp_table, opp, opp->rate); And I am not sure how this will end up working, since new OPP will be equal to old one. Since I see you call this from resume() at many places. what exactly are you trying to do here ? Those details would be good to have in commit log as well, I haven't really followed V7 of your series.
On Tue, Aug 17, 2021 at 02:04:38PM +0200, Ulf Hansson wrote: > On Tue, 17 Aug 2021 at 03:30, Dmitry Osipenko <digetx@gmail.com> wrote: > > > > Add runtime PM and OPP support to the Host1x driver. It's required for > > enabling system-wide DVFS and supporting dynamic power management using > > a generic power domain. For the starter we will keep host1x always-on > > because dynamic power management require a major refactoring of the driver > > code since lot's of code paths will need the RPM handling and we're going > > to remove some of these paths in the future. Host1x doesn't consume much > > power so it is good enough, we at least need to resume Host1x in order > > to initialize the power state. > > > > Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30 > > Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20 > > Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124 > > Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30 > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > --- > > [...] > > > + > > static int host1x_probe(struct platform_device *pdev) > > { > > struct host1x *host; > > @@ -394,6 +423,10 @@ static int host1x_probe(struct platform_device *pdev) > > /* set common host1x device data */ > > platform_set_drvdata(pdev, host); > > > > + err = devm_tegra_core_dev_init_opp_table_simple(&pdev->dev); > > + if (err) > > + return err; > > + > > host->regs = devm_ioremap_resource(&pdev->dev, regs); > > if (IS_ERR(host->regs)) > > return PTR_ERR(host->regs); > > @@ -423,12 +456,9 @@ static int host1x_probe(struct platform_device *pdev) > > return err; > > } > > > > - host->rst = devm_reset_control_get(&pdev->dev, "host1x"); > > - if (IS_ERR(host->rst)) { > > - err = PTR_ERR(host->rst); > > - dev_err(&pdev->dev, "failed to get reset: %d\n", err); > > + err = host1x_get_resets(host); > > + if (err) > > return err; > > - } > > > > err = host1x_iommu_init(host); > > if (err < 0) { > > @@ -443,22 +473,10 @@ static int host1x_probe(struct platform_device *pdev) > > goto iommu_exit; > > } > > > > - err = clk_prepare_enable(host->clk); > > - if (err < 0) { > > - dev_err(&pdev->dev, "failed to enable clock\n"); > > - goto free_channels; > > - } > > - > > - err = reset_control_deassert(host->rst); > > - if (err < 0) { > > - dev_err(&pdev->dev, "failed to deassert reset: %d\n", err); > > - goto unprepare_disable; > > - } > > - > > Removing the clk_prepare_enable() and reset_control_deassert() from > host1x_probe(), might not be a good idea. See more about why, below. > > > err = host1x_syncpt_init(host); > > if (err) { > > dev_err(&pdev->dev, "failed to initialize syncpts\n"); > > - goto reset_assert; > > + goto free_channels; > > } > > > > err = host1x_intr_init(host, syncpt_irq); > > @@ -467,10 +485,14 @@ static int host1x_probe(struct platform_device *pdev) > > goto deinit_syncpt; > > } > > > > - host1x_debug_init(host); > > + pm_runtime_enable(&pdev->dev); > > > > - if (host->info->has_hypervisor) > > - host1x_setup_sid_table(host); > > + /* the driver's code isn't ready yet for the dynamic RPM */ > > + err = pm_runtime_resume_and_get(&pdev->dev); > > If the driver is being built with the CONFIG_PM Kconfig option being > unset, pm_runtime_resume_and_get() will return 0 to indicate success - > and without calling the ->runtime_resume() callback. > In other words, the clock will remain gated and the reset will not be > deasserted, likely causing the driver to be malfunctioning. > > If the driver isn't ever being built with CONFIG_PM unset, feel free > to ignore my above comments. > > Otherwise, if it needs to work both with and without CONFIG_PM being > set, you may use the following pattern in host1x_probe() to deploy > runtime PM support: > > "Enable the needed resources to probe the device" > pm_runtime_get_noresume() > pm_runtime_set_active() > pm_runtime_enable() > > "Before successfully completing probe" > pm_runtime_put() We made a conscious decision a few years ago to have ARCH_TEGRA select PM on both 32-bit and 64-bit ARM, specifically to avoid the need to do this dance (though there are still a few drivers left that do this, I think). So I think this should be unnecessary. Unless perhaps if the sysfs PM controls have any influence on this. As far as I know, as long as the PM kconfig option is enabled, the sysfs control only influence the runtime behaviour (i.e. setting the sysfs PM control to "on" is going to force runtime PM to be resumed) but there's no way to disable runtime PM altogether via sysfs that would make the above necessary. Thierry
17.08.2021 10:55, Viresh Kumar пишет: ... >> +int dev_pm_opp_sync(struct device *dev) >> +{ >> + struct opp_table *opp_table; >> + struct dev_pm_opp *opp; >> + int ret = 0; >> + >> + /* Device may not have OPP table */ >> + opp_table = _find_opp_table(dev); >> + if (IS_ERR(opp_table)) >> + return 0; >> + >> + if (!_get_opp_count(opp_table)) >> + goto put_table; >> + >> + opp = _find_current_opp(dev, opp_table); >> + ret = _set_opp(dev, opp_table, opp, opp->rate); > > And I am not sure how this will end up working, since new OPP will be > equal to old one. Since I see you call this from resume() at many > places. Initially OPP table is "uninitialized" and opp_table->enabled=false, hence the first sync always works even if OPP is equal to old one. Once OPP has been synced, all further syncs are NO-OPs, hence it doesn't matter how many times syncing is called. https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012 > what exactly are you trying to do here ? Those details would be good > to have in commit log as well, I haven't really followed V7 of your > series. I'm initializing voltage/power state of OPP table in accordance to the clock rate, bumping voltage before clock is enabled by device driver. I'll improve the commit message. An alternative to the explicit syncing could be something like a new dev_pm_opp_resume/suspend helpers that will take care of enabling/disabling the OPP table clock/etc and syncing the OPP state with h/w.
On Tue, Aug 17, 2021 at 04:27:26AM +0300, Dmitry Osipenko wrote: > Document tegra-clocks sub-node which describes Tegra SoC clocks that > require a higher voltage of the core power domain in order to operate > properly on a higher clock rates. Each node contains a phandle to OPP > table and power domain. > > The root PLLs and system clocks don't have any specific device dedicated > to them, clock controller is in charge of managing power for them. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > .../bindings/clock/nvidia,tegra20-car.yaml | 51 +++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml > index 459d2a525393..7f5cd27e4ce0 100644 > --- a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml > +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml > @@ -42,6 +42,48 @@ properties: > "#reset-cells": > const: 1 > > + tegra-clocks: > + description: child nodes are the output clocks from the CAR > + type: object > + > + patternProperties: > + "^[a-z]+[0-9]+$": > + type: object > + properties: > + compatible: > + allOf: > + - items: > + - enum: > + - nvidia,tegra20-sclk > + - nvidia,tegra30-sclk > + - nvidia,tegra30-pllc > + - nvidia,tegra30-plle > + - nvidia,tegra30-pllm > + - const: nvidia,tegra-clock You are saying the first string must be both one of the enums and 'nvidia,tegra-clock'. You don't get an error because your pattern doesn't match 'sclk'. > + > + operating-points-v2: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + Phandle to OPP table that contains frequencies, voltages and > + opp-supported-hw property, which is a bitfield indicating > + SoC process or speedo ID mask. Just 'operating-points-v2: true' is enough. > + > + clocks: > + items: > + - description: node's clock > + > + power-domains: > + maxItems: 1 > + description: phandle to the core SoC power domain > + > + required: > + - compatible > + - operating-points-v2 > + - clocks > + - power-domains > + > + additionalProperties: false > + > required: > - compatible > - reg > @@ -59,6 +101,15 @@ examples: > reg = <0x60006000 0x1000>; > #clock-cells = <1>; > #reset-cells = <1>; > + > + tegra-clocks { > + sclk { > + compatible = "nvidia,tegra20-sclk", "nvidia,tegra-clock"; > + operating-points-v2 = <&opp_table>; > + clocks = <&tegra_car TEGRA20_CLK_SCLK>; > + power-domains = <&domain>; > + }; > + }; > }; > > usb-controller@c5004000 { > -- > 2.32.0 > >
18.08.2021 04:15, Rob Herring пишет: >> + tegra-clocks: >> + description: child nodes are the output clocks from the CAR >> + type: object >> + >> + patternProperties: >> + "^[a-z]+[0-9]+$": >> + type: object >> + properties: >> + compatible: >> + allOf: >> + - items: >> + - enum: >> + - nvidia,tegra20-sclk >> + - nvidia,tegra30-sclk >> + - nvidia,tegra30-pllc >> + - nvidia,tegra30-plle >> + - nvidia,tegra30-pllm >> + - const: nvidia,tegra-clock > You are saying the first string must be both one of the enums and > 'nvidia,tegra-clock'. You don't get an error because your pattern > doesn't match 'sclk'. > Could you please rephrase or clarify? If pattern doesn't match 'sclk', then it must match any other enum. I'm not sure what you're meaning. The 'nvidia,tegra-clock' actually could be removed since it's superfluous now. I'll consider the removal in v9.
18.08.2021 05:04, Dmitry Osipenko пишет: > 18.08.2021 04:37, Dmitry Osipenko пишет: >> 18.08.2021 04:16, Rob Herring пишет: >>> On Tue, Aug 17, 2021 at 04:27:29AM +0300, Dmitry Osipenko wrote: >>>> Memory Client should be blocked before hardware reset is asserted in order >>>> to prevent memory corruption and hanging of memory controller. >>>> >>>> Document Memory Client resets of Host1x, GR2D and GR3D hardware units. >>>> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>> --- >>>> .../bindings/display/tegra/nvidia,tegra20-host1x.txt | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt >>>> index 62861a8fb5c6..07a08653798b 100644 >>>> --- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt >>>> +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt >>>> @@ -18,6 +18,7 @@ Required properties: >>>> - resets: Must contain an entry for each entry in reset-names. >>>> See ../reset/reset.txt for details. >>>> - reset-names: Must include the following entries: >>>> + - mc >>>> - host1x >>> >>> New entries should be at the end. Order matters. >> >> Indeed, order matters. In this case it matters by the hardware because >> memory reset must be asserted before the controller's reset. We rely on >> it in the code of the GENPD driver. Hence it's the intended order in >> this patch. >> > > Although, my bad. It should be to reorder items here, it's not a GENPD > binding. > * should be fine I'll change it in v9.
On 17-08-21, 18:49, Dmitry Osipenko wrote: > 17.08.2021 10:55, Viresh Kumar пишет: > ... > >> +int dev_pm_opp_sync(struct device *dev) > >> +{ > >> + struct opp_table *opp_table; > >> + struct dev_pm_opp *opp; > >> + int ret = 0; > >> + > >> + /* Device may not have OPP table */ > >> + opp_table = _find_opp_table(dev); > >> + if (IS_ERR(opp_table)) > >> + return 0; > >> + > >> + if (!_get_opp_count(opp_table)) > >> + goto put_table; > >> + > >> + opp = _find_current_opp(dev, opp_table); > >> + ret = _set_opp(dev, opp_table, opp, opp->rate); > > > > And I am not sure how this will end up working, since new OPP will be > > equal to old one. Since I see you call this from resume() at many > > places. > > Initially OPP table is "uninitialized" and opp_table->enabled=false, > hence the first sync always works even if OPP is equal to old one. Once > OPP has been synced, all further syncs are NO-OPs, hence it doesn't > matter how many times syncing is called. > > https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012 Right, but how will this work from Resume ? Won't that be a no-op ?
18.08.2021 06:55, Viresh Kumar пишет: > On 17-08-21, 18:49, Dmitry Osipenko wrote: >> 17.08.2021 10:55, Viresh Kumar пишет: >> ... >>>> +int dev_pm_opp_sync(struct device *dev) >>>> +{ >>>> + struct opp_table *opp_table; >>>> + struct dev_pm_opp *opp; >>>> + int ret = 0; >>>> + >>>> + /* Device may not have OPP table */ >>>> + opp_table = _find_opp_table(dev); >>>> + if (IS_ERR(opp_table)) >>>> + return 0; >>>> + >>>> + if (!_get_opp_count(opp_table)) >>>> + goto put_table; >>>> + >>>> + opp = _find_current_opp(dev, opp_table); >>>> + ret = _set_opp(dev, opp_table, opp, opp->rate); >>> >>> And I am not sure how this will end up working, since new OPP will be >>> equal to old one. Since I see you call this from resume() at many >>> places. >> >> Initially OPP table is "uninitialized" and opp_table->enabled=false, >> hence the first sync always works even if OPP is equal to old one. Once >> OPP has been synced, all further syncs are NO-OPs, hence it doesn't >> matter how many times syncing is called. >> >> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012 > > Right, but how will this work from Resume ? Won't that be a no-op ? The first resume initializes the OPP state on sync, all further syncs on resume are no-ops.
18.08.2021 07:12, Dmitry Osipenko пишет: > 18.08.2021 06:55, Viresh Kumar пишет: >> On 17-08-21, 18:49, Dmitry Osipenko wrote: >>> 17.08.2021 10:55, Viresh Kumar пишет: >>> ... >>>>> +int dev_pm_opp_sync(struct device *dev) >>>>> +{ >>>>> + struct opp_table *opp_table; >>>>> + struct dev_pm_opp *opp; >>>>> + int ret = 0; >>>>> + >>>>> + /* Device may not have OPP table */ >>>>> + opp_table = _find_opp_table(dev); >>>>> + if (IS_ERR(opp_table)) >>>>> + return 0; >>>>> + >>>>> + if (!_get_opp_count(opp_table)) >>>>> + goto put_table; >>>>> + >>>>> + opp = _find_current_opp(dev, opp_table); >>>>> + ret = _set_opp(dev, opp_table, opp, opp->rate); >>>> >>>> And I am not sure how this will end up working, since new OPP will be >>>> equal to old one. Since I see you call this from resume() at many >>>> places. >>> >>> Initially OPP table is "uninitialized" and opp_table->enabled=false, >>> hence the first sync always works even if OPP is equal to old one. Once >>> OPP has been synced, all further syncs are NO-OPs, hence it doesn't >>> matter how many times syncing is called. >>> >>> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012 >> >> Right, but how will this work from Resume ? Won't that be a no-op ? > > The first resume initializes the OPP state on sync, all further syncs on > resume are no-ops. > Notice that we use GENPD here. GENPD core takes care of storing PD's performance state (voltage in case of Tegra) and dropping it to 0 after rpm-suspend, GENPD core also restores the state before rpm-resume.
18.08.2021 07:29, Dmitry Osipenko пишет: > 18.08.2021 07:12, Dmitry Osipenko пишет: >> 18.08.2021 06:55, Viresh Kumar пишет: >>> On 17-08-21, 18:49, Dmitry Osipenko wrote: >>>> 17.08.2021 10:55, Viresh Kumar пишет: >>>> ... >>>>>> +int dev_pm_opp_sync(struct device *dev) >>>>>> +{ >>>>>> + struct opp_table *opp_table; >>>>>> + struct dev_pm_opp *opp; >>>>>> + int ret = 0; >>>>>> + >>>>>> + /* Device may not have OPP table */ >>>>>> + opp_table = _find_opp_table(dev); >>>>>> + if (IS_ERR(opp_table)) >>>>>> + return 0; >>>>>> + >>>>>> + if (!_get_opp_count(opp_table)) >>>>>> + goto put_table; >>>>>> + >>>>>> + opp = _find_current_opp(dev, opp_table); >>>>>> + ret = _set_opp(dev, opp_table, opp, opp->rate); >>>>> >>>>> And I am not sure how this will end up working, since new OPP will be >>>>> equal to old one. Since I see you call this from resume() at many >>>>> places. >>>> >>>> Initially OPP table is "uninitialized" and opp_table->enabled=false, >>>> hence the first sync always works even if OPP is equal to old one. Once >>>> OPP has been synced, all further syncs are NO-OPs, hence it doesn't >>>> matter how many times syncing is called. >>>> >>>> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012 >>> >>> Right, but how will this work from Resume ? Won't that be a no-op ? >> >> The first resume initializes the OPP state on sync, all further syncs on >> resume are no-ops. >> > > Notice that we use GENPD here. GENPD core takes care of storing PD's > performance state (voltage in case of Tegra) and dropping it to 0 after > rpm-suspend, GENPD core also restores the state before rpm-resume. By 'here' I mean in this series.
On 18-08-21, 07:12, Dmitry Osipenko wrote: > 18.08.2021 06:55, Viresh Kumar пишет: > > On 17-08-21, 18:49, Dmitry Osipenko wrote: > >> 17.08.2021 10:55, Viresh Kumar пишет: > >> ... > >>>> +int dev_pm_opp_sync(struct device *dev) > >>>> +{ > >>>> + struct opp_table *opp_table; > >>>> + struct dev_pm_opp *opp; > >>>> + int ret = 0; > >>>> + > >>>> + /* Device may not have OPP table */ > >>>> + opp_table = _find_opp_table(dev); > >>>> + if (IS_ERR(opp_table)) > >>>> + return 0; > >>>> + > >>>> + if (!_get_opp_count(opp_table)) > >>>> + goto put_table; > >>>> + > >>>> + opp = _find_current_opp(dev, opp_table); > >>>> + ret = _set_opp(dev, opp_table, opp, opp->rate); > >>> > >>> And I am not sure how this will end up working, since new OPP will be > >>> equal to old one. Since I see you call this from resume() at many > >>> places. > >> > >> Initially OPP table is "uninitialized" and opp_table->enabled=false, > >> hence the first sync always works even if OPP is equal to old one. Once > >> OPP has been synced, all further syncs are NO-OPs, hence it doesn't > >> matter how many times syncing is called. > >> > >> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012 > > > > Right, but how will this work from Resume ? Won't that be a no-op ? > > The first resume initializes the OPP state on sync, all further syncs on > resume are no-ops. But the OPPs should already be initialized as someone must have called opp-set-rate earlier ? Why do this from resume and not probe ?
On 18-08-21, 07:30, Dmitry Osipenko wrote: > 18.08.2021 07:29, Dmitry Osipenko пишет: > >> The first resume initializes the OPP state on sync, all further syncs on > >> resume are no-ops. > >> > > > > Notice that we use GENPD here. GENPD core takes care of storing PD's > > performance state (voltage in case of Tegra) and dropping it to 0 after > > rpm-suspend, GENPD core also restores the state before rpm-resume. > > By 'here' I mean in this series. It is still not clear to me why you need to this on resume, and not probe.
18.08.2021 07:31, Viresh Kumar пишет: > On 18-08-21, 07:12, Dmitry Osipenko wrote: >> 18.08.2021 06:55, Viresh Kumar пишет: >>> On 17-08-21, 18:49, Dmitry Osipenko wrote: >>>> 17.08.2021 10:55, Viresh Kumar пишет: >>>> ... >>>>>> +int dev_pm_opp_sync(struct device *dev) >>>>>> +{ >>>>>> + struct opp_table *opp_table; >>>>>> + struct dev_pm_opp *opp; >>>>>> + int ret = 0; >>>>>> + >>>>>> + /* Device may not have OPP table */ >>>>>> + opp_table = _find_opp_table(dev); >>>>>> + if (IS_ERR(opp_table)) >>>>>> + return 0; >>>>>> + >>>>>> + if (!_get_opp_count(opp_table)) >>>>>> + goto put_table; >>>>>> + >>>>>> + opp = _find_current_opp(dev, opp_table); >>>>>> + ret = _set_opp(dev, opp_table, opp, opp->rate); >>>>> >>>>> And I am not sure how this will end up working, since new OPP will be >>>>> equal to old one. Since I see you call this from resume() at many >>>>> places. >>>> >>>> Initially OPP table is "uninitialized" and opp_table->enabled=false, >>>> hence the first sync always works even if OPP is equal to old one. Once >>>> OPP has been synced, all further syncs are NO-OPs, hence it doesn't >>>> matter how many times syncing is called. >>>> >>>> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012 >>> >>> Right, but how will this work from Resume ? Won't that be a no-op ? >> >> The first resume initializes the OPP state on sync, all further syncs on >> resume are no-ops. > > But the OPPs should already be initialized as someone must have called > opp-set-rate earlier ? Why do this from resume and not probe ? This will set voltage level without having an actively used hardware. Take a 3d driver for example, if you set the rate on probe and rpm-resume will never be called, then the voltage will be set high, while hardware is kept suspended if userspace will never wake it up by executing a 3d job.
On 18-08-21, 07:37, Dmitry Osipenko wrote: > This will set voltage level without having an actively used hardware. > Take a 3d driver for example, if you set the rate on probe and > rpm-resume will never be called, then the voltage will be set high, > while hardware is kept suspended if userspace will never wake it up by > executing a 3d job. What exactly are we looking to achieve with this stuff ? Cache the current performance state with genpd (based on the state bootloader's has set) ? Or anything else as well ?
18.08.2021 07:53, Viresh Kumar пишет: > On 18-08-21, 07:37, Dmitry Osipenko wrote: >> This will set voltage level without having an actively used hardware. >> Take a 3d driver for example, if you set the rate on probe and >> rpm-resume will never be called, then the voltage will be set high, >> while hardware is kept suspended if userspace will never wake it up by >> executing a 3d job. > > What exactly are we looking to achieve with this stuff ? Cache the > current performance state with genpd (based on the state bootloader's > has set) ? Yes, GENPD will cache the perf state across suspend/resume and initially cached value is out of sync with h/w. > Or anything else as well ? Nothing else. But let me clarify it all again. Initially the performance state of all GENPDs is 0 for all devices. The clock rate is preinitialized for all devices to a some default rate by clk driver, or by bootloader or by assigned-clocks in DT. When device is rpm-resumed, the resume callback of a device driver enables the clock. Before clock is enabled, the voltage needs to be configured in accordance to the clk rate. So now we have a GENPD with pstate=0 on a first rpm-resume, which doesn't match the h/w configuration. Calling dev_pm_opp_sync() sets the pstate in accordance to the h/w config. In a previous v7 I proposed to preset the rpm_pstate of GENPD (perf level that is restored before device is rpm-resumed) from PD's attach_dev callback, but Ulf didn't like that because it requires to use and modify GENPD 'private' variables from a PD driver. We decided that will be better to make device drivers to explicitly sync the perf state, which I implemented in this v8.
On 18-08-21, 08:21, Dmitry Osipenko wrote: > Yes, GENPD will cache the perf state across suspend/resume and initially > cached value is out of sync with h/w. > > Nothing else. But let me clarify it all again. Thanks for your explanation. > Initially the performance state of all GENPDs is 0 for all devices. > > The clock rate is preinitialized for all devices to a some default rate > by clk driver, or by bootloader or by assigned-clocks in DT. > > When device is rpm-resumed, the resume callback of a device driver > enables the clock. > > Before clock is enabled, the voltage needs to be configured in > accordance to the clk rate. > > So now we have a GENPD with pstate=0 on a first rpm-resume, which > doesn't match the h/w configuration. Calling dev_pm_opp_sync() sets the > pstate in accordance to the h/w config. What about calling dev_pm_opp_set_rate(dev, clk_get_rate(dev)) here instead ? That will work, right ? The advantage is it works without any special routine to do so. I also wonder looking at your gr3d.c changes, you set a set-opp helper, but the driver doesn't call set_opp_rate at all. Who calls it ? And if it is all about just syncing the genpd core, then can the genpd core do something like what clk framework does? i.e. allow a new optional genpd callback, get_performance_state() (just like set_performance_state()), which can be called initially by the core to get the performance to something other than zero. opp-set-rate is there to set the performance state and enable the stuff as well. That's why it looks incorrect in your case, where the function was only required to be called once, and you are ending up calling it on each resume. Limiting that with another local variable is bad as well. > In a previous v7 I proposed to preset the rpm_pstate of GENPD (perf > level that is restored before device is rpm-resumed) from PD's > attach_dev callback, but Ulf didn't like that because it requires to use > and modify GENPD 'private' variables from a PD driver. We decided that > will be better to make device drivers to explicitly sync the perf state, > which I implemented in this v8.
On 18-08-21, 11:28, Viresh Kumar wrote: > On 18-08-21, 08:21, Dmitry Osipenko wrote: > > Yes, GENPD will cache the perf state across suspend/resume and initially > > cached value is out of sync with h/w. > > > > Nothing else. But let me clarify it all again. > > Thanks for your explanation. > > > Initially the performance state of all GENPDs is 0 for all devices. > > > > The clock rate is preinitialized for all devices to a some default rate > > by clk driver, or by bootloader or by assigned-clocks in DT. > > > > When device is rpm-resumed, the resume callback of a device driver > > enables the clock. > > > > Before clock is enabled, the voltage needs to be configured in > > accordance to the clk rate. > > > > So now we have a GENPD with pstate=0 on a first rpm-resume, which > > doesn't match the h/w configuration. Calling dev_pm_opp_sync() sets the > > pstate in accordance to the h/w config. > > What about calling dev_pm_opp_set_rate(dev, clk_get_rate(dev)) here > instead ? That will work, right ? The advantage is it works without > any special routine to do so. > > I also wonder looking at your gr3d.c changes, you set a set-opp > helper, but the driver doesn't call set_opp_rate at all. Who calls it > ? > > And if it is all about just syncing the genpd core, then can the genpd > core do something like what clk framework does? i.e. allow a new > optional genpd callback, get_performance_state() (just like > set_performance_state()), which can be called initially by the core to > get the performance to something other than zero. opp-set-rate is > there to set the performance state and enable the stuff as well. > That's why it looks incorrect in your case, where the function was > only required to be called once, and you are ending up calling it on > each resume. Limiting that with another local variable is bad as well. Ulf, this last part is for you :)
18.08.2021 08:58, Viresh Kumar пишет: > On 18-08-21, 08:21, Dmitry Osipenko wrote: >> Yes, GENPD will cache the perf state across suspend/resume and initially >> cached value is out of sync with h/w. >> >> Nothing else. But let me clarify it all again. > > Thanks for your explanation. > >> Initially the performance state of all GENPDs is 0 for all devices. >> >> The clock rate is preinitialized for all devices to a some default rate >> by clk driver, or by bootloader or by assigned-clocks in DT. >> >> When device is rpm-resumed, the resume callback of a device driver >> enables the clock. >> >> Before clock is enabled, the voltage needs to be configured in >> accordance to the clk rate. >> >> So now we have a GENPD with pstate=0 on a first rpm-resume, which >> doesn't match the h/w configuration. Calling dev_pm_opp_sync() sets the >> pstate in accordance to the h/w config. > > What about calling dev_pm_opp_set_rate(dev, clk_get_rate(dev)) here > instead ? That will work, right ? The advantage is it works without > any special routine to do so. It will work, but a dedicated helper is nicer. > I also wonder looking at your gr3d.c changes, you set a set-opp > helper, but the driver doesn't call set_opp_rate at all. Who calls it > ? dev_pm_opp_sync() calls it from _set_opp(). > And if it is all about just syncing the genpd core, then can the genpd > core do something like what clk framework does? i.e. allow a new > optional genpd callback, get_performance_state() (just like > set_performance_state()), which can be called initially by the core to > get the performance to something other than zero. opp-set-rate is > there to set the performance state and enable the stuff as well. > That's why it looks incorrect in your case, where the function was > only required to be called once, and you are ending up calling it on > each resume. Limiting that with another local variable is bad as well. We discussed variant with get_performance_state() previously and Ulf didn't like it either since it still requires to touch 'internals' of GENPD.
On 18-08-21, 09:22, Dmitry Osipenko wrote: > 18.08.2021 08:58, Viresh Kumar пишет: > > What about calling dev_pm_opp_set_rate(dev, clk_get_rate(dev)) here > > instead ? That will work, right ? The advantage is it works without > > any special routine to do so. > > It will work, but a dedicated helper is nicer. > > > I also wonder looking at your gr3d.c changes, you set a set-opp > > helper, but the driver doesn't call set_opp_rate at all. Who calls it > > ? > > dev_pm_opp_sync() calls it from _set_opp(). Okay, please use dev_pm_opp_set_rate() instead then. New helper just adds to the confusion and isn't doing anything special apart from doing clk_get_rate() for you. > > And if it is all about just syncing the genpd core, then can the genpd > > core do something like what clk framework does? i.e. allow a new > > optional genpd callback, get_performance_state() (just like > > set_performance_state()), which can be called initially by the core to > > get the performance to something other than zero. opp-set-rate is > > there to set the performance state and enable the stuff as well. > > That's why it looks incorrect in your case, where the function was > > only required to be called once, and you are ending up calling it on > > each resume. Limiting that with another local variable is bad as well. > > We discussed variant with get_performance_state() previously and Ulf > didn't like it either since it still requires to touch 'internals' of GENPD. Hmm, I wonder if that would be a problem since only genpd core is going to call that routine anyway.
On Tue, 17 Aug 2021 at 16:03, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Tue, Aug 17, 2021 at 02:04:38PM +0200, Ulf Hansson wrote: > > On Tue, 17 Aug 2021 at 03:30, Dmitry Osipenko <digetx@gmail.com> wrote: > > > > > > Add runtime PM and OPP support to the Host1x driver. It's required for > > > enabling system-wide DVFS and supporting dynamic power management using > > > a generic power domain. For the starter we will keep host1x always-on > > > because dynamic power management require a major refactoring of the driver > > > code since lot's of code paths will need the RPM handling and we're going > > > to remove some of these paths in the future. Host1x doesn't consume much > > > power so it is good enough, we at least need to resume Host1x in order > > > to initialize the power state. > > > > > > Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30 > > > Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20 > > > Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124 > > > Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30 > > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > > --- > > > > [...] > > > > > + > > > static int host1x_probe(struct platform_device *pdev) > > > { > > > struct host1x *host; > > > @@ -394,6 +423,10 @@ static int host1x_probe(struct platform_device *pdev) > > > /* set common host1x device data */ > > > platform_set_drvdata(pdev, host); > > > > > > + err = devm_tegra_core_dev_init_opp_table_simple(&pdev->dev); > > > + if (err) > > > + return err; > > > + > > > host->regs = devm_ioremap_resource(&pdev->dev, regs); > > > if (IS_ERR(host->regs)) > > > return PTR_ERR(host->regs); > > > @@ -423,12 +456,9 @@ static int host1x_probe(struct platform_device *pdev) > > > return err; > > > } > > > > > > - host->rst = devm_reset_control_get(&pdev->dev, "host1x"); > > > - if (IS_ERR(host->rst)) { > > > - err = PTR_ERR(host->rst); > > > - dev_err(&pdev->dev, "failed to get reset: %d\n", err); > > > + err = host1x_get_resets(host); > > > + if (err) > > > return err; > > > - } > > > > > > err = host1x_iommu_init(host); > > > if (err < 0) { > > > @@ -443,22 +473,10 @@ static int host1x_probe(struct platform_device *pdev) > > > goto iommu_exit; > > > } > > > > > > - err = clk_prepare_enable(host->clk); > > > - if (err < 0) { > > > - dev_err(&pdev->dev, "failed to enable clock\n"); > > > - goto free_channels; > > > - } > > > - > > > - err = reset_control_deassert(host->rst); > > > - if (err < 0) { > > > - dev_err(&pdev->dev, "failed to deassert reset: %d\n", err); > > > - goto unprepare_disable; > > > - } > > > - > > > > Removing the clk_prepare_enable() and reset_control_deassert() from > > host1x_probe(), might not be a good idea. See more about why, below. > > > > > err = host1x_syncpt_init(host); > > > if (err) { > > > dev_err(&pdev->dev, "failed to initialize syncpts\n"); > > > - goto reset_assert; > > > + goto free_channels; > > > } > > > > > > err = host1x_intr_init(host, syncpt_irq); > > > @@ -467,10 +485,14 @@ static int host1x_probe(struct platform_device *pdev) > > > goto deinit_syncpt; > > > } > > > > > > - host1x_debug_init(host); > > > + pm_runtime_enable(&pdev->dev); > > > > > > - if (host->info->has_hypervisor) > > > - host1x_setup_sid_table(host); > > > + /* the driver's code isn't ready yet for the dynamic RPM */ > > > + err = pm_runtime_resume_and_get(&pdev->dev); > > > > If the driver is being built with the CONFIG_PM Kconfig option being > > unset, pm_runtime_resume_and_get() will return 0 to indicate success - > > and without calling the ->runtime_resume() callback. > > In other words, the clock will remain gated and the reset will not be > > deasserted, likely causing the driver to be malfunctioning. > > > > If the driver isn't ever being built with CONFIG_PM unset, feel free > > to ignore my above comments. > > > > Otherwise, if it needs to work both with and without CONFIG_PM being > > set, you may use the following pattern in host1x_probe() to deploy > > runtime PM support: > > > > "Enable the needed resources to probe the device" > > pm_runtime_get_noresume() > > pm_runtime_set_active() > > pm_runtime_enable() > > > > "Before successfully completing probe" > > pm_runtime_put() > > We made a conscious decision a few years ago to have ARCH_TEGRA select > PM on both 32-bit and 64-bit ARM, specifically to avoid the need to do > this dance (though there are still a few drivers left that do this, I > think). > > So I think this should be unnecessary. Unless perhaps if the sysfs PM > controls have any influence on this. As far as I know, as long as the > PM kconfig option is enabled, the sysfs control only influence the > runtime behaviour (i.e. setting the sysfs PM control to "on" is going > to force runtime PM to be resumed) but there's no way to disable > runtime PM altogether via sysfs that would make the above necessary. Thanks for clarifying! As I said, feel free to ignore my comments then. For this and the other patches in the series, I assume you only need to care about whether the driver is a cross SoC driver and used on other platforms than Tegra then. > > Thierry Kind regards Uffe
On 18-08-21, 10:29, Ulf Hansson wrote: > Me and Dmitry discussed adding a new genpd callback for this. I agreed > that it seems like a reasonable thing to add, if he insists. > > The intent was to invoke the new callback from __genpd_dev_pm_attach() > when the device has been attached to its genpd. This allows the > callback, to invoke clk_get_rate() and then dev_pm_opp_set_rate(), to > update the vote according to the current state of the HW. I wouldn't call dev_pm_opp_set_rate() from there, since it means configure and enable (both) for different resources, clk, regulator, genpd, etc.. What we need here is just configure. So something like this then: - genpd->get_performance_state() -> dev_pm_opp_get_current_opp() //New API -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate); This can be done just once from probe() then. > I am not sure if/why that approach seemed insufficient? > > Another option to solve the problem, I think, is simply to patch > drivers to let them call dev_pm_opp_set_rate() during ->probe(), this > should synchronize the HW state too. Dmitry already mentioned that this will make the device start consuming power, and he doesn't want that, else we need an explicit disble call as well.
On Wed, 18 Aug 2021 at 11:14, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 18-08-21, 10:29, Ulf Hansson wrote: > > Me and Dmitry discussed adding a new genpd callback for this. I agreed > > that it seems like a reasonable thing to add, if he insists. > > > > The intent was to invoke the new callback from __genpd_dev_pm_attach() > > when the device has been attached to its genpd. This allows the > > callback, to invoke clk_get_rate() and then dev_pm_opp_set_rate(), to > > update the vote according to the current state of the HW. > > I wouldn't call dev_pm_opp_set_rate() from there, since it means > configure and enable (both) for different resources, clk, regulator, > genpd, etc.. Right, good point! dev_pm_opp_set_rate() is best called from consumer drivers, as they need to be in control. > > What we need here is just configure. So something like this then: > > - genpd->get_performance_state() > -> dev_pm_opp_get_current_opp() //New API > -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate); > > This can be done just once from probe() then. How would dev_pm_opp_get_current_opp() work? Do you have a suggestion? > > > I am not sure if/why that approach seemed insufficient? > > > > Another option to solve the problem, I think, is simply to patch > > drivers to let them call dev_pm_opp_set_rate() during ->probe(), this > > should synchronize the HW state too. > > Dmitry already mentioned that this will make the device start > consuming power, and he doesn't want that, else we need an explicit > disble call as well. I am sure I understand the problem. When a device is getting probed, it needs to consume power, how else can the corresponding driver successfully probe it? > > -- > viresh Kind regards Uffe
On Wed, Aug 18, 2021 at 04:44:30AM +0300, Dmitry Osipenko wrote: > 18.08.2021 04:15, Rob Herring пишет: > >> + tegra-clocks: > >> + description: child nodes are the output clocks from the CAR > >> + type: object > >> + > >> + patternProperties: > >> + "^[a-z]+[0-9]+$": > >> + type: object > >> + properties: > >> + compatible: > >> + allOf: > >> + - items: > >> + - enum: > >> + - nvidia,tegra20-sclk > >> + - nvidia,tegra30-sclk > >> + - nvidia,tegra30-pllc > >> + - nvidia,tegra30-plle > >> + - nvidia,tegra30-pllm > >> + - const: nvidia,tegra-clock > > You are saying the first string must be both one of the enums and > > 'nvidia,tegra-clock'. You don't get an error because your pattern > > doesn't match 'sclk'. > > > > Could you please rephrase or clarify? If pattern doesn't match 'sclk', > then it must match any other enum. I'm not sure what you're meaning. "sclk" doesn't match "^[a-z]+[0-9]+$" because it's missing at least one digit at the end. Perhaps that last + was supposed to be *? > > The 'nvidia,tegra-clock' actually could be removed since it's > superfluous now. I'll consider the removal in v9. It also looks like your schema was meant to be something like: compatible: - items: - enum: - nvidia,tegra20-sclk - nvidia,tegra30-sclk - nvidia,tegra30-pllc - nvidia,tegra30-plle - nvidia,tegra30-pllm - const: nvidia,tegra-clock Note how the const: element is indented one more level. Now this means: one of the enumeration values, followed by the constant value. That matches what the example has. That said, I agree that nvidia,tegra-clock seems a bit useless. There's really no such thing as a generic clock, they're all different in some way. Thierry
18.08.2021 16:52, Thierry Reding пишет: > On Wed, Aug 18, 2021 at 04:44:30AM +0300, Dmitry Osipenko wrote: >> 18.08.2021 04:15, Rob Herring пишет: >>>> + tegra-clocks: >>>> + description: child nodes are the output clocks from the CAR >>>> + type: object >>>> + >>>> + patternProperties: >>>> + "^[a-z]+[0-9]+$": >>>> + type: object >>>> + properties: >>>> + compatible: >>>> + allOf: >>>> + - items: >>>> + - enum: >>>> + - nvidia,tegra20-sclk >>>> + - nvidia,tegra30-sclk >>>> + - nvidia,tegra30-pllc >>>> + - nvidia,tegra30-plle >>>> + - nvidia,tegra30-pllm >>>> + - const: nvidia,tegra-clock >>> You are saying the first string must be both one of the enums and >>> 'nvidia,tegra-clock'. You don't get an error because your pattern >>> doesn't match 'sclk'. >>> >> >> Could you please rephrase or clarify? If pattern doesn't match 'sclk', >> then it must match any other enum. I'm not sure what you're meaning. > > "sclk" doesn't match "^[a-z]+[0-9]+$" because it's missing at least one > digit at the end. Perhaps that last + was supposed to be *? Ah, the regex pattern. Yes, I forgot to update it. >> The 'nvidia,tegra-clock' actually could be removed since it's >> superfluous now. I'll consider the removal in v9. > > It also looks like your schema was meant to be something like: > > compatible: > - items: > - enum: > - nvidia,tegra20-sclk > - nvidia,tegra30-sclk > - nvidia,tegra30-pllc > - nvidia,tegra30-plle > - nvidia,tegra30-pllm > - const: nvidia,tegra-clock > > Note how the const: element is indented one more level. Now this means: > one of the enumeration values, followed by the constant value. That > matches what the example has. > > That said, I agree that nvidia,tegra-clock seems a bit useless. There's > really no such thing as a generic clock, they're all different in some > way. It's a leftover from older versions of this patchset, I'll remove it.
18.08.2021 16:59, Thierry Reding пишет: > On Tue, Aug 17, 2021 at 04:27:26AM +0300, Dmitry Osipenko wrote: >> Document tegra-clocks sub-node which describes Tegra SoC clocks that >> require a higher voltage of the core power domain in order to operate >> properly on a higher clock rates. Each node contains a phandle to OPP >> table and power domain. >> >> The root PLLs and system clocks don't have any specific device dedicated >> to them, clock controller is in charge of managing power for them. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> .../bindings/clock/nvidia,tegra20-car.yaml | 51 +++++++++++++++++++ >> 1 file changed, 51 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml >> index 459d2a525393..7f5cd27e4ce0 100644 >> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml >> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml >> @@ -42,6 +42,48 @@ properties: >> "#reset-cells": >> const: 1 >> >> + tegra-clocks: >> + description: child nodes are the output clocks from the CAR >> + type: object >> + >> + patternProperties: >> + "^[a-z]+[0-9]+$": >> + type: object >> + properties: >> + compatible: >> + allOf: >> + - items: >> + - enum: >> + - nvidia,tegra20-sclk >> + - nvidia,tegra30-sclk >> + - nvidia,tegra30-pllc >> + - nvidia,tegra30-plle >> + - nvidia,tegra30-pllm >> + - const: nvidia,tegra-clock >> + >> + operating-points-v2: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + Phandle to OPP table that contains frequencies, voltages and >> + opp-supported-hw property, which is a bitfield indicating >> + SoC process or speedo ID mask. >> + >> + clocks: >> + items: >> + - description: node's clock >> + >> + power-domains: >> + maxItems: 1 >> + description: phandle to the core SoC power domain >> + >> + required: >> + - compatible >> + - operating-points-v2 >> + - clocks >> + - power-domains >> + >> + additionalProperties: false >> + >> required: >> - compatible >> - reg >> @@ -59,6 +101,15 @@ examples: >> reg = <0x60006000 0x1000>; >> #clock-cells = <1>; >> #reset-cells = <1>; >> + >> + tegra-clocks { >> + sclk { >> + compatible = "nvidia,tegra20-sclk", "nvidia,tegra-clock"; >> + operating-points-v2 = <&opp_table>; >> + clocks = <&tegra_car TEGRA20_CLK_SCLK>; >> + power-domains = <&domain>; >> + }; >> + }; > > I wonder if it'd be better to match on the name of the node rather than > add an artificial compatible string. We usually use the compatible > string to match a device, but here you're really trying to add > information about a resource provided by the CAR controller. > > We do similar things for example in PMIC bindings where the individual > regulators are represented in the device tree via nodes named after the > regulator. > > You could then also leave out the clocks property, which is weird as it > is because it's basically a self-reference. But you don't really need > the reference here in the first place because the CAR is already the > parent of SCLK. We don't have a platform device for CaR. I don't see how it's going to work. We need to create a platform device for each RPM-capable clock because that's how RPM works. The compatible string is required for instantiating OF-devices from a node, otherwise we will have to re-invent the OF core. > Also, I don't think the tegra- prefix is necessary here. The parent node > is already identified as Tegra via the compatible string. > > In the case of CAR, I'd imagine something like: > > clocks { > sclk { > operating-points-v2 = <&opp_table>; > power-domains = <&domain>; > }; > }; > > Now you've only got the bare minimum in here that you actually add. All > the other data that you used to have is simply derived from the parent. 'clocks' is already a generic keyword in DT. It's probably not okay to redefine it.
18.08.2021 18:43, Dmitry Osipenko пишет: > 18.08.2021 13:08, Ulf Hansson пишет: >> On Wed, 18 Aug 2021 at 11:50, Viresh Kumar <viresh.kumar@linaro.org> wrote: >>> >>> On 18-08-21, 11:41, Ulf Hansson wrote: >>>> On Wed, 18 Aug 2021 at 11:14, Viresh Kumar <viresh.kumar@linaro.org> wrote: >>>>> What we need here is just configure. So something like this then: >>>>> >>>>> - genpd->get_performance_state() >>>>> -> dev_pm_opp_get_current_opp() //New API >>>>> -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate); >>>>> >>>>> This can be done just once from probe() then. >>>> >>>> How would dev_pm_opp_get_current_opp() work? Do you have a suggestion? >>> >>> The opp core already has a way of finding current OPP, that's what >>> Dmitry is trying to use here. It finds it using clk_get_rate(), if >>> that is zero, it picks the lowest freq possible. >>> >>>> I am sure I understand the problem. When a device is getting probed, >>>> it needs to consume power, how else can the corresponding driver >>>> successfully probe it? >>> >>> Dmitry can answer that better, but a device doesn't necessarily need >>> to consume energy in probe. It can consume bus clock, like APB we >>> have, but the more energy consuming stuff can be left disabled until >>> the time a user comes up. Probe will just end up registering the >>> driver and initializing it. >> >> That's perfectly fine, as then it's likely that it won't vote for an >> OPP, but can postpone that as well. >> >> Perhaps the problem is rather that the HW may already carry a non-zero >> vote made from a bootloader. If the consumer driver tries to clear >> that vote (calling dev_pm_opp_set_rate(dev, 0), for example), it would >> still not lead to any updates of the performance state in genpd, >> because genpd internally has initialized the performance-state to >> zero. > > We don't need to discover internal SoC devices because we use > device-tree on ARM. For most devices power isn't required at a probe > time because probe function doesn't touch h/w at all, thus devices are > left in suspended state after probe. > > We have three components comprising PM on Tegra: > > 1. Power gate > 2. Clock state > 3. Voltage state > > GENPD on/off represents the 'power gate'. > > Clock and reset are controlled by device drivers using clk and rst APIs. > > Voltage state is represented by GENPD's performance level. OPP framework couples the performance level with the clock rate. > GENPD core assumes that at a first rpm-resume of a consumer device, its > genpd_performance=0. Not true for Tegra because h/w of the device is > preconfigured to a non-zero perf level initially, h/w may not support > zero level at all. > > GENPD core assumes that consumer devices can work at any performance > level. Not true for Tegra because voltage needs to be set in accordance > to the clock rate before clock is enabled, otherwise h/w won't work > properly, perhaps clock may be unstable or h/w won't be latching. > > Performance level should be set to 0 while device is suspended. > Performance level needs to be bumped on rpm-resume of a device in > accordance to h/w state before hardware is enabled. >
On Wed, Aug 18, 2021 at 06:05:11PM +0300, Dmitry Osipenko wrote: > 18.08.2021 16:59, Thierry Reding пишет: > > On Tue, Aug 17, 2021 at 04:27:26AM +0300, Dmitry Osipenko wrote: > >> Document tegra-clocks sub-node which describes Tegra SoC clocks that > >> require a higher voltage of the core power domain in order to operate > >> properly on a higher clock rates. Each node contains a phandle to OPP > >> table and power domain. > >> > >> The root PLLs and system clocks don't have any specific device dedicated > >> to them, clock controller is in charge of managing power for them. > >> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> .../bindings/clock/nvidia,tegra20-car.yaml | 51 +++++++++++++++++++ > >> 1 file changed, 51 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml > >> index 459d2a525393..7f5cd27e4ce0 100644 > >> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml > >> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml > >> @@ -42,6 +42,48 @@ properties: > >> "#reset-cells": > >> const: 1 > >> > >> + tegra-clocks: > >> + description: child nodes are the output clocks from the CAR > >> + type: object > >> + > >> + patternProperties: > >> + "^[a-z]+[0-9]+$": > >> + type: object > >> + properties: > >> + compatible: > >> + allOf: > >> + - items: > >> + - enum: > >> + - nvidia,tegra20-sclk > >> + - nvidia,tegra30-sclk > >> + - nvidia,tegra30-pllc > >> + - nvidia,tegra30-plle > >> + - nvidia,tegra30-pllm > >> + - const: nvidia,tegra-clock > >> + > >> + operating-points-v2: > >> + $ref: /schemas/types.yaml#/definitions/phandle > >> + description: > >> + Phandle to OPP table that contains frequencies, voltages and > >> + opp-supported-hw property, which is a bitfield indicating > >> + SoC process or speedo ID mask. > >> + > >> + clocks: > >> + items: > >> + - description: node's clock > >> + > >> + power-domains: > >> + maxItems: 1 > >> + description: phandle to the core SoC power domain > >> + > >> + required: > >> + - compatible > >> + - operating-points-v2 > >> + - clocks > >> + - power-domains > >> + > >> + additionalProperties: false > >> + > >> required: > >> - compatible > >> - reg > >> @@ -59,6 +101,15 @@ examples: > >> reg = <0x60006000 0x1000>; > >> #clock-cells = <1>; > >> #reset-cells = <1>; > >> + > >> + tegra-clocks { > >> + sclk { > >> + compatible = "nvidia,tegra20-sclk", "nvidia,tegra-clock"; > >> + operating-points-v2 = <&opp_table>; > >> + clocks = <&tegra_car TEGRA20_CLK_SCLK>; > >> + power-domains = <&domain>; > >> + }; > >> + }; > > > > I wonder if it'd be better to match on the name of the node rather than > > add an artificial compatible string. We usually use the compatible > > string to match a device, but here you're really trying to add > > information about a resource provided by the CAR controller. > > > > We do similar things for example in PMIC bindings where the individual > > regulators are represented in the device tree via nodes named after the > > regulator. > > > > You could then also leave out the clocks property, which is weird as it > > is because it's basically a self-reference. But you don't really need > > the reference here in the first place because the CAR is already the > > parent of SCLK. > > We don't have a platform device for CaR. I don't see how it's going to > work. We need to create a platform device for each RPM-capable clock > because that's how RPM works. The compatible string is required for > instantiating OF-devices from a node, otherwise we will have to > re-invent the OF core. I think we do have a platform device for CAR. It's just not bound against by the driver because these clock drivers are "special". But from other parts of the series you're already trying to fix that, at least partially. But it doesn't seem right to create a platform device for each RPM- capable clock. Why do they need to be devices? They aren't, so why pretend? Is it that some API that we want to use here requires the struct device? > > Also, I don't think the tegra- prefix is necessary here. The parent node > > is already identified as Tegra via the compatible string. > > > > In the case of CAR, I'd imagine something like: > > > > clocks { > > sclk { > > operating-points-v2 = <&opp_table>; > > power-domains = <&domain>; > > }; > > }; > > > > Now you've only got the bare minimum in here that you actually add. All > > the other data that you used to have is simply derived from the parent. > > 'clocks' is already a generic keyword in DT. It's probably not okay to > redefine it. "clocks" is not a generic keyword. It's the name of a property and given that we're talking about the clock provider here, it doesn't need a clocks property of its own, so it should be fine to use that for the node. Thierry
18.08.2021 19:39, Thierry Reding пишет: >> We don't have a platform device for CaR. I don't see how it's going to >> work. We need to create a platform device for each RPM-capable clock >> because that's how RPM works. The compatible string is required for >> instantiating OF-devices from a node, otherwise we will have to >> re-invent the OF core. > I think we do have a platform device for CAR. It's just not bound > against by the driver because these clock drivers are "special". But > from other parts of the series you're already trying to fix that, at > least partially. > > But it doesn't seem right to create a platform device for each RPM- > capable clock. Why do they need to be devices? They aren't, so why > pretend? Is it that some API that we want to use here requires the > struct device? The "device" representation is internal to the kernel. It's okay to me to have PLLs represented by a device, it's a distinct h/w by itself. CCF supports managing of clock's RPM and it requires to have clock to be backed by a device. That's what we are using here. Please see https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/clk/clk.c#L109 >>> Also, I don't think the tegra- prefix is necessary here. The parent node >>> is already identified as Tegra via the compatible string. >>> >>> In the case of CAR, I'd imagine something like: >>> >>> clocks { >>> sclk { >>> operating-points-v2 = <&opp_table>; >>> power-domains = <&domain>; >>> }; >>> }; >>> >>> Now you've only got the bare minimum in here that you actually add. All >>> the other data that you used to have is simply derived from the parent. >> 'clocks' is already a generic keyword in DT. It's probably not okay to >> redefine it. > "clocks" is not a generic keyword. It's the name of a property and given > that we're talking about the clock provider here, it doesn't need a > clocks property of its own, so it should be fine to use that for the > node. I'm curious what Rob thinks about it. Rob, does this sound okay to you?
18.08.2021 19:57, Dmitry Osipenko пишет: >>>> Also, I don't think the tegra- prefix is necessary here. The parent node >>>> is already identified as Tegra via the compatible string. >>>> >>>> In the case of CAR, I'd imagine something like: >>>> >>>> clocks { >>>> sclk { >>>> operating-points-v2 = <&opp_table>; >>>> power-domains = <&domain>; >>>> }; >>>> }; >>>> >>>> Now you've only got the bare minimum in here that you actually add. All >>>> the other data that you used to have is simply derived from the parent. >>> 'clocks' is already a generic keyword in DT. It's probably not okay to >>> redefine it. >> "clocks" is not a generic keyword. It's the name of a property and given >> that we're talking about the clock provider here, it doesn't need a >> clocks property of its own, so it should be fine to use that for the >> node. > I'm curious what Rob thinks about it. Rob, does this sound okay to you? I assume dt-schema won't be happy with a different meaning for the 'clocks'.
18.08.2021 11:35, Ulf Hansson пишет: > Thanks for clarifying! As I said, feel free to ignore my comments then. > > For this and the other patches in the series, I assume you only need > to care about whether the driver is a cross SoC driver and used on > other platforms than Tegra then. Yes, and all drivers touched by this series are Tegra-only drivers.
On Thu, 19 Aug 2021 at 08:17, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 18-08-21, 18:55, Dmitry Osipenko wrote: > > 18.08.2021 12:41, Ulf Hansson пишет: > > > > Either way gives the equal result. The new callback allows to remove the > > boilerplate dev_pm_opp_set_rate(clk_get_rate() code from the rpm-resume > > of consumer devices, that's it. > > It may not be equal, as dev_pm_opp_set_rate() may do additional stuff, > now or in a later implementation. Currently it only does > regulator_enable() as a special case, but it can be clk_enable() as > well. Also, this tries to solve the problem in a tricky/hacky way, > while all you wanted was to make the genpd aware of what the > performance state should be. > > Your driver can break tomorrow if we started to do more stuff from > this API at another time. > > > > dev_pm_opp_set_rate() is best called from consumer drivers, as they > > > need to be in control. > > >> What we need here is just configure. So something like this then: > > The intent wasn't to use dev_pm_opp_set_rate() from > > __genpd_dev_pm_attach(), but to set genpd->rpm_pstate in accordance to > > the h/w configuration. > > Right. > > > On Tegra we have a chain of PDs and it's not trivial to convert the > > device's OPP into pstate because only the parent domain can translate > > the required OPP. > > The driver should just be required to make a call, and OPP/genpd core > should return it a value. This is already done today while setting the > pstate for a device. The same frameworks must be able to supply a > value to be used for the device. Right, that sounds reasonable. We already have pm_genpd_opp_to_performance_state() which translates an OPP to a performance state. This function invokes the ->opp_to_performance_state() for a genpd. Maybe we need to allow a genpd to not have ->opp_to_performance_state() callback assigned though, but continue up in the hierarchy to see if the parent has the callback assigned, to make this work for Tegra? Perhaps we should add an API dev_pm_genpd_opp_to_performance_state(), allowing us to pass the device instead of the genpd. But that's a minor thing. Finally, the precondition to use the above, is to first get a handle to an OPP table. This is where I am struggling to find a generic solution, because I guess that would be platform or even consumer driver specific for how to do this. And at what point should we do this? > > > Viresh, please take a look at what I did in [1]. Maybe it could be done > > in another way. > > I looked into this and looked like too much trouble. The > implementation needs to be simple. I am not sure I understand all the > problems you faced while doing that, would be better to start with a > simpler implementation of get_performance_state() kind of API for > genpd, after the domain is attached and its OPP table is initialized. > > Note, that the OPP table isn't required to be fully initialized for > the device at this point, we can parse the DT as well if needed be. Sure, but as I indicated above, you need some kind of input data to figure out what OPP table to pick, before you can translate that into a performance state. Is that always the clock rate, for example? Perhaps, we should start with adding a dev_pm_opp_get_from_rate() or what do you think? Do you have other suggestions? > > -- > viresh Kind regards Uffe
On Wed, Aug 18, 2021 at 07:57:04PM +0300, Dmitry Osipenko wrote: > 18.08.2021 19:39, Thierry Reding пишет: > >> We don't have a platform device for CaR. I don't see how it's going to > >> work. We need to create a platform device for each RPM-capable clock > >> because that's how RPM works. The compatible string is required for > >> instantiating OF-devices from a node, otherwise we will have to > >> re-invent the OF core. > > I think we do have a platform device for CAR. It's just not bound > > against by the driver because these clock drivers are "special". But > > from other parts of the series you're already trying to fix that, at > > least partially. > > > > But it doesn't seem right to create a platform device for each RPM- > > capable clock. Why do they need to be devices? They aren't, so why > > pretend? Is it that some API that we want to use here requires the > > struct device? > > The "device" representation is internal to the kernel. It's okay to me > to have PLLs represented by a device, it's a distinct h/w by itself. > > CCF supports managing of clock's RPM and it requires to have clock to be > backed by a device. That's what we are using here. > > Please see > https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/clk/clk.c#L109 Looking at the implementation of __clk_register() and where that device pointer typically comes from, I don't think the way this is used here is what was intended. The way I interpret the code is that a clock is registered with a parent device (i.e. its provider) and clk_pm_runtime_get() is then used internally as a way to make sure that when a clock is prepared, it's parent device is runtime resumed. This is presumably to ensure that any registers that the driver might need to access in order to prepare and enable the clock are accessible (i.e. the CAR is not powered off or in reset). So the struct device that is passed to __clk_register() (or its callers) should be that of the CAR rather than virtual struct devices created by the CAR. And it's a bit debatable whether or not PLLs represent distinct hardware. Ultimately every transistor on a chip could be considered distinct hardware. But a platform device is a device on a platform bus, which is really just another way of saying it's a hardware block that's accessible from the CPU via a memory-mapped address. A PLL (just like other clocks) is merely a resource exposed by means of access to these registers. So I don't think they should be platform devices. Even making them struct device:s seems a bit of a stretch. Is there any reason why struct clk can't be used for this? I mean, the whole purpose of that structure is to represent clocks. Why do we need to make them special? > >>> Also, I don't think the tegra- prefix is necessary here. The parent node > >>> is already identified as Tegra via the compatible string. > >>> > >>> In the case of CAR, I'd imagine something like: > >>> > >>> clocks { > >>> sclk { > >>> operating-points-v2 = <&opp_table>; > >>> power-domains = <&domain>; > >>> }; > >>> }; > >>> > >>> Now you've only got the bare minimum in here that you actually add. All > >>> the other data that you used to have is simply derived from the parent. > >> 'clocks' is already a generic keyword in DT. It's probably not okay to > >> redefine it. > > "clocks" is not a generic keyword. It's the name of a property and given > > that we're talking about the clock provider here, it doesn't need a > > clocks property of its own, so it should be fine to use that for the > > node. > > I'm curious what Rob thinks about it. Rob, does this sound okay to you? Another alternative would be to omit that level altogether and just make sclk and siblings direct children of the CAR node. Thierry
19.08.2021 19:31, Thierry Reding пишет: >> The "device" representation is internal to the kernel. It's okay to me >> to have PLLs represented by a device, it's a distinct h/w by itself. >> >> CCF supports managing of clock's RPM and it requires to have clock to be >> backed by a device. That's what we are using here. >> >> Please see >> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/clk/clk.c#L109 > Looking at the implementation of __clk_register() and where that device > pointer typically comes from, I don't think the way this is used here is > what was intended. The way I interpret the code is that a clock is > registered with a parent device (i.e. its provider) and > clk_pm_runtime_get() is then used internally as a way to make sure that > when a clock is prepared, it's parent device is runtime resumed. This is > presumably to ensure that any registers that the driver might need to > access in order to prepare and enable the clock are accessible (i.e. the > CAR is not powered off or in reset). > > So the struct device that is passed to __clk_register() (or its callers) > should be that of the CAR rather than virtual struct devices created by > the CAR. > > And it's a bit debatable whether or not PLLs represent distinct > hardware. Ultimately every transistor on a chip could be considered > distinct hardware. But a platform device is a device on a platform bus, > which is really just another way of saying it's a hardware block that's > accessible from the CPU via a memory-mapped address. A PLL (just like > other clocks) is merely a resource exposed by means of access to these > registers. So I don't think they should be platform devices. Even making > them struct device:s seems a bit of a stretch. > > Is there any reason why struct clk can't be used for this? I mean, the > whole purpose of that structure is to represent clocks. Why do we need > to make them special? Because we need to perform DVFS for PLLs. The only way to do it without having to reinvent existing frameworks is to use these frameworks and they require a device.
On 19-08-21, 22:35, Dmitry Osipenko wrote: > 19.08.2021 16:07, Ulf Hansson пишет: > > In the other scenario where a consumer driver prefers to *not* call > > pm_runtime_resume_and_get() in its ->probe(), because it doesn't need > > to power on the device to complete probing, then we don't want to vote > > for an OPP at all - and we also want the performance state for the > > device in genpd to be set to zero. Correct? > > Yes > > > Is this the main problem you are trying to solve, because I think this > > doesn't work out of the box as of today? > > The main problem is that the restored performance state is zero for the > first genpd_runtime_resume(), while it's not zero from the h/w perspective. This is exactly why I have been advocating that the genpd needs to sync up with the hardware before any calls are made to it from the consumer driver. Just what clock framework does to get the clock rate. > > There is another concern though, but perhaps it's not a problem after > > all. Viresh told us that dev_pm_opp_set_rate() may turn on resources > > like clock/regulators. That could certainly be problematic, in > > particular if the device and its genpd have OPP tables associated with > > it and the consumer driver wants to follow the above sequence in > > probe. > > dev_pm_opp_set_rate() won't enable clocks and regulators, but it may It does enable regulators right now, it may choose to enable clocks later on, no guarantees. > change the clock rate and voltage. This is also platform/driver specific > because it's up to OPP user how to configure OPP table. On Tegra we only > assign clock to OPP table, regulators are unused. Right, over that platforms can set their own version of set-opp callback, where all this is done from a platform specific callback. > > Viresh, can you please chime in here and elaborate on some of the > > magic happening behind dev_pm_opp_set_rate() API - is there a problem > > here or not? It configures clock, regulators, genpds, any required OPPs, + it enables regulators right now.
On 19-08-21, 16:55, Ulf Hansson wrote: > Right, that sounds reasonable. > > We already have pm_genpd_opp_to_performance_state() which translates > an OPP to a performance state. This function invokes the > ->opp_to_performance_state() for a genpd. Maybe we need to allow a > genpd to not have ->opp_to_performance_state() callback assigned > though, but continue up in the hierarchy to see if the parent has the > callback assigned, to make this work for Tegra? > > Perhaps we should add an API dev_pm_genpd_opp_to_performance_state(), > allowing us to pass the device instead of the genpd. But that's a > minor thing. I am not concerned a lot about how it gets implemented, and am not sure as well, as I haven't looked into these details since sometime. Any reasonable thing will be accepted, as simple as that. > Finally, the precondition to use the above, is to first get a handle > to an OPP table. This is where I am struggling to find a generic > solution, because I guess that would be platform or even consumer > driver specific for how to do this. And at what point should we do > this? Hmm, I am not very clear with the whole picture at this point of time. Dmitry, can you try to frame a sequence of events/calls/etc that will define what kind of devices we are looking at here, and how this can be made to work ? > > > Viresh, please take a look at what I did in [1]. Maybe it could be done > > > in another way. > > > > I looked into this and looked like too much trouble. The > > implementation needs to be simple. I am not sure I understand all the > > problems you faced while doing that, would be better to start with a > > simpler implementation of get_performance_state() kind of API for > > genpd, after the domain is attached and its OPP table is initialized. > > > > Note, that the OPP table isn't required to be fully initialized for > > the device at this point, we can parse the DT as well if needed be. > > Sure, but as I indicated above, you need some kind of input data to > figure out what OPP table to pick, before you can translate that into > a performance state. Is that always the clock rate, for example? Eventually it can be clock, bandwidth, or pstate of anther genpd, not sure what all we are looking for now. It should be just clock right now as far as I can imagine :) > Perhaps, we should start with adding a dev_pm_opp_get_from_rate() or > what do you think? Do you have other suggestions? We already have similar APIs, so that won't be a problem. We also have a mechanism inside the OPP core, frequency based, which is used to guess the current OPP. Maybe we can enhance and use that directly here. -- viresh
On Thu, 19 Aug 2021 at 21:35, Dmitry Osipenko <digetx@gmail.com> wrote: > > 19.08.2021 16:07, Ulf Hansson пишет: > > On Wed, 18 Aug 2021 at 17:43, Dmitry Osipenko <digetx@gmail.com> wrote: > >> > >> 18.08.2021 13:08, Ulf Hansson пишет: > >>> On Wed, 18 Aug 2021 at 11:50, Viresh Kumar <viresh.kumar@linaro.org> wrote: > >>>> > >>>> On 18-08-21, 11:41, Ulf Hansson wrote: > >>>>> On Wed, 18 Aug 2021 at 11:14, Viresh Kumar <viresh.kumar@linaro.org> wrote: > >>>>>> What we need here is just configure. So something like this then: > >>>>>> > >>>>>> - genpd->get_performance_state() > >>>>>> -> dev_pm_opp_get_current_opp() //New API > >>>>>> -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate); > >>>>>> > >>>>>> This can be done just once from probe() then. > >>>>> > >>>>> How would dev_pm_opp_get_current_opp() work? Do you have a suggestion? > >>>> > >>>> The opp core already has a way of finding current OPP, that's what > >>>> Dmitry is trying to use here. It finds it using clk_get_rate(), if > >>>> that is zero, it picks the lowest freq possible. > >>>> > >>>>> I am sure I understand the problem. When a device is getting probed, > >>>>> it needs to consume power, how else can the corresponding driver > >>>>> successfully probe it? > >>>> > >>>> Dmitry can answer that better, but a device doesn't necessarily need > >>>> to consume energy in probe. It can consume bus clock, like APB we > >>>> have, but the more energy consuming stuff can be left disabled until > >>>> the time a user comes up. Probe will just end up registering the > >>>> driver and initializing it. > >>> > >>> That's perfectly fine, as then it's likely that it won't vote for an > >>> OPP, but can postpone that as well. > >>> > >>> Perhaps the problem is rather that the HW may already carry a non-zero > >>> vote made from a bootloader. If the consumer driver tries to clear > >>> that vote (calling dev_pm_opp_set_rate(dev, 0), for example), it would > >>> still not lead to any updates of the performance state in genpd, > >>> because genpd internally has initialized the performance-state to > >>> zero. > >> > >> We don't need to discover internal SoC devices because we use > >> device-tree on ARM. For most devices power isn't required at a probe > >> time because probe function doesn't touch h/w at all, thus devices are > >> left in suspended state after probe. > >> > >> We have three components comprising PM on Tegra: > >> > >> 1. Power gate > >> 2. Clock state > >> 3. Voltage state > >> > >> GENPD on/off represents the 'power gate'. > >> > >> Clock and reset are controlled by device drivers using clk and rst APIs. > >> > >> Voltage state is represented by GENPD's performance level. > >> > >> GENPD core assumes that at a first rpm-resume of a consumer device, its > >> genpd_performance=0. Not true for Tegra because h/w of the device is > >> preconfigured to a non-zero perf level initially, h/w may not support > >> zero level at all. > > > > I think you may be misunderstanding genpd's behaviour around this, but > > let me elaborate. > > > > In genpd_runtime_resume(), we try to restore the performance state for > > the device that genpd_runtime_suspend() *may* have dropped earlier. > > That means, if genpd_runtime_resume() is called prior > > genpd_runtime_suspend() for the first time, it means that > > genpd_runtime_resume() will *not* restore a performance state, but > > instead just leave the performance state as is for the device (see > > genpd_restore_performance_state()). > > > > In other words, a consumer driver may use the following sequence to > > set an initial performance state for the device during ->probe(): > > > > ... > > rate = clk_get_rate() > > dev_pm_opp_set_rate(rate) > > > > pm_runtime_enable() > > pm_runtime_resume_and_get() > > ... > > > > Note that, it's the consumer driver's responsibility to manage device > > specific resources, in its ->runtime_suspend|resume() callbacks. > > Typically that means dealing with clock gating/ungating, for example. > > > > In the other scenario where a consumer driver prefers to *not* call > > pm_runtime_resume_and_get() in its ->probe(), because it doesn't need > > to power on the device to complete probing, then we don't want to vote > > for an OPP at all - and we also want the performance state for the > > device in genpd to be set to zero. Correct? > > Yes > > > Is this the main problem you are trying to solve, because I think this > > doesn't work out of the box as of today? > > The main problem is that the restored performance state is zero for the > first genpd_runtime_resume(), while it's not zero from the h/w perspective. This should not be a problem, but can be handled by the consumer driver. genpd_runtime_resume() calls genpd_restore_performance_state() to restore a performance state for the device. However, in the scenario you describe, "gpd_data->rpm_pstate" is zero, which makes genpd_restore_performance_state() to just leave the device's performance state as is - it will *not* restore the performance state to zero. To make the consumer driver deal with this, it would need to call dev_pm_opp_set_rate() from within its ->runtime_resume() callback. > > > There is another concern though, but perhaps it's not a problem after > > all. Viresh told us that dev_pm_opp_set_rate() may turn on resources > > like clock/regulators. That could certainly be problematic, in > > particular if the device and its genpd have OPP tables associated with > > it and the consumer driver wants to follow the above sequence in > > probe. > > dev_pm_opp_set_rate() won't enable clocks and regulators, but it may > change the clock rate and voltage. This is also platform/driver specific > because it's up to OPP user how to configure OPP table. On Tegra we only > assign clock to OPP table, regulators are unused. > > > Viresh, can you please chime in here and elaborate on some of the > > magic happening behind dev_pm_opp_set_rate() API - is there a problem > > here or not? > > > >> > >> GENPD core assumes that consumer devices can work at any performance > >> level. Not true for Tegra because voltage needs to be set in accordance > >> to the clock rate before clock is enabled, otherwise h/w won't work > >> properly, perhaps clock may be unstable or h/w won't be latching. > > > > Correct. Genpd relies on the callers to use the OPP framework if there > > are constraints like you describe above. > > > > That said, it's not forbidden for a consumer driver to call > > dev_pm_genpd_set_performance_state() directly, but then it better > > knows exactly what it's doing. > > > >> > >> Performance level should be set to 0 while device is suspended. > > > > Do you mean system suspend or runtime suspend? Or both? > > Runtime suspend. Alright. So that's already taken care of for us in genpd_runtime_suspend(). Or perhaps you have discovered some problem with this? > > >> Performance level needs to be bumped on rpm-resume of a device in > >> accordance to h/w state before hardware is enabled. > > > > Assuming there was a performance state set for the device when > > genpd_runtime_suspend() was called, genpd_runtime_resume() will > > restore that state according to the sequence you described. > > What do you think about adding API that will allow drivers to explicitly > set the restored performance state of a power domain? > > Another option could be to change the GENPD core, making it to set the > rpm_pstate when dev_pm_genpd_set_performance_state(dev) is invoked and > device is rpm-suspended, instead of calling the > genpd->set_performance_state callback. > > Then drivers will be able to sync the perf state at a probe time. > > What do you think? I don't think it's needed, see my reply earlier above. However your change touches another problem though, see below. > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index a934c679e6ce..cc15ab9eacc9 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -435,7 +435,7 @@ static void genpd_restore_performance_state(struct > device *dev, > int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int > state) > { > struct generic_pm_domain *genpd; > - int ret; > + int ret = 0; > > genpd = dev_to_genpd_safe(dev); > if (!genpd) > @@ -446,7 +446,10 @@ int dev_pm_genpd_set_performance_state(struct > device *dev, unsigned int state) > return -EINVAL; > > genpd_lock(genpd); > - ret = genpd_set_performance_state(dev, state); > + if (pm_runtime_suspended(dev)) > + dev_gpd_data(dev)->rpm_pstate = state; > + else > + ret = genpd_set_performance_state(dev, state); > genpd_unlock(genpd); This doesn't work for all cases. For example, when a consumer driver deploys runtime PM support in its ->probe() according to the below sequence: ... dev_pm_opp_set_rate(rate) pm_runtime_get_noresume() pm_runtime_set_active() pm_runtime_enable() ... pm_runtime_put() ... We need to call genpd_set_performance_state() independently of whether the device is runtime suspended or not. Although, it actually seems like good idea to update dev_gpd_data(dev)->rpm_pstate = state here, as to make sure genpd_runtime_resume() doesn't restore an old/invalid value that was saved while dropping the performance state vote for the device in genpd_runtime_suspend() earlier. Let me send a patch for this shortly, to close this window of a possible error. > > return ret; > > Kind regards Uffe
On Fri, 20 Aug 2021 at 07:18, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 19-08-21, 16:55, Ulf Hansson wrote: > > Right, that sounds reasonable. > > > > We already have pm_genpd_opp_to_performance_state() which translates > > an OPP to a performance state. This function invokes the > > ->opp_to_performance_state() for a genpd. Maybe we need to allow a > > genpd to not have ->opp_to_performance_state() callback assigned > > though, but continue up in the hierarchy to see if the parent has the > > callback assigned, to make this work for Tegra? > > > > Perhaps we should add an API dev_pm_genpd_opp_to_performance_state(), > > allowing us to pass the device instead of the genpd. But that's a > > minor thing. > > I am not concerned a lot about how it gets implemented, and am not > sure as well, as I haven't looked into these details since sometime. > Any reasonable thing will be accepted, as simple as that. > > > Finally, the precondition to use the above, is to first get a handle > > to an OPP table. This is where I am struggling to find a generic > > solution, because I guess that would be platform or even consumer > > driver specific for how to do this. And at what point should we do > > this? > > Hmm, I am not very clear with the whole picture at this point of time. > > Dmitry, can you try to frame a sequence of events/calls/etc that will > define what kind of devices we are looking at here, and how this can > be made to work ? > > > > > Viresh, please take a look at what I did in [1]. Maybe it could be done > > > > in another way. > > > > > > I looked into this and looked like too much trouble. The > > > implementation needs to be simple. I am not sure I understand all the > > > problems you faced while doing that, would be better to start with a > > > simpler implementation of get_performance_state() kind of API for > > > genpd, after the domain is attached and its OPP table is initialized. > > > > > > Note, that the OPP table isn't required to be fully initialized for > > > the device at this point, we can parse the DT as well if needed be. > > > > Sure, but as I indicated above, you need some kind of input data to > > figure out what OPP table to pick, before you can translate that into > > a performance state. Is that always the clock rate, for example? > > Eventually it can be clock, bandwidth, or pstate of anther genpd, not > sure what all we are looking for now. It should be just clock right > now as far as I can imagine :) > > > Perhaps, we should start with adding a dev_pm_opp_get_from_rate() or > > what do you think? Do you have other suggestions? > > We already have similar APIs, so that won't be a problem. We also have > a mechanism inside the OPP core, frequency based, which is used to > guess the current OPP. Maybe we can enhance and use that directly > here. After reading the last reply from Dmitry, I am starting to think that the problem he is facing can be described and solved in a much easier way. If I am correct, it looks like we don't need to add APIs to get OPPs for a clock rate or set initial performance state values according to the HW in genpd. See my other response to Dmitry, let's see where that leads us. Kind regards Uffe
20.08.2021 08:18, Viresh Kumar пишет: > On 19-08-21, 16:55, Ulf Hansson wrote: >> Right, that sounds reasonable. >> >> We already have pm_genpd_opp_to_performance_state() which translates >> an OPP to a performance state. This function invokes the >> ->opp_to_performance_state() for a genpd. Maybe we need to allow a >> genpd to not have ->opp_to_performance_state() callback assigned >> though, but continue up in the hierarchy to see if the parent has the >> callback assigned, to make this work for Tegra? >> >> Perhaps we should add an API dev_pm_genpd_opp_to_performance_state(), >> allowing us to pass the device instead of the genpd. But that's a >> minor thing. > > I am not concerned a lot about how it gets implemented, and am not > sure as well, as I haven't looked into these details since sometime. > Any reasonable thing will be accepted, as simple as that. > >> Finally, the precondition to use the above, is to first get a handle >> to an OPP table. This is where I am struggling to find a generic >> solution, because I guess that would be platform or even consumer >> driver specific for how to do this. And at what point should we do >> this? GENPD core can't get OPP table handle, setting up OPP table is a platform/driver specific operation. > Hmm, I am not very clear with the whole picture at this point of time. > > Dmitry, can you try to frame a sequence of events/calls/etc that will > define what kind of devices we are looking at here, and how this can > be made to work ? Could you please clarify what do you mean by a "kind of devices"? I made hack based on the recent discussions and it partially works. Getting clock rate involves resuming device which backs the clock and it also may use GENPD, so lockings are becoming complicated. It doesn't work at all if device uses multiple domains because virtual domain device doesn't have OPP table. Setting up the performance state from a consumer driver is a cleaner variant so far. diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index e1c8994ae225..faa0bbe99c98 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -410,11 +410,16 @@ static int genpd_drop_performance_state(struct device *dev) return 0; } -static void genpd_restore_performance_state(struct device *dev, - unsigned int state) +static int genpd_restore_performance_state(struct generic_pm_domain *genpd, + struct device *dev, + unsigned int state) { + int ret = 0; + if (state) - genpd_set_performance_state(dev, state); + ret = genpd_set_performance_state(dev, state); + + return ret; } /** @@ -435,7 +440,7 @@ static void genpd_restore_performance_state(struct device *dev, int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state) { struct generic_pm_domain *genpd; - int ret; + int ret = 0; genpd = dev_to_genpd_safe(dev); if (!genpd) @@ -446,7 +451,10 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state) return -EINVAL; genpd_lock(genpd); - ret = genpd_set_performance_state(dev, state); + if (pm_runtime_suspended(dev)) + dev_gpd_data(dev)->rpm_pstate = state; + else + ret = genpd_set_performance_state(dev, state); genpd_unlock(genpd); return ret; @@ -959,10 +967,25 @@ static int genpd_runtime_resume(struct device *dev) goto out; } + if (genpd->get_performance_state) { + ret = genpd->get_performance_state(genpd, dev); + if (ret < 0) + return ret; + + if (ret > 0) + gpd_data->rpm_pstate = ret; + } + genpd_lock(genpd); ret = genpd_power_on(genpd, 0); - if (!ret) - genpd_restore_performance_state(dev, gpd_data->rpm_pstate); + if (!ret) { + ret = genpd_restore_performance_state(genpd, dev, + gpd_data->rpm_pstate); + if (ret) + genpd_power_off(genpd, true, 0); + else + gpd_data->rpm_pstate = 0; + } genpd_unlock(genpd); if (ret) diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 18016e49605f..982be2dba21e 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -2967,3 +2967,33 @@ int dev_pm_opp_sync(struct device *dev) return ret; } EXPORT_SYMBOL_GPL(dev_pm_opp_sync); + +/** + * dev_pm_opp_from_clk_rate() - Get OPP from current clock rate + * @dev: device for which we do this operation + * + * Get OPP which corresponds to the current clock rate of a device. + * + * Return: pointer to 'struct dev_pm_opp' on success and errorno otherwise. + */ +struct dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev) +{ + struct dev_pm_opp *opp = ERR_PTR(-ENODEV); + struct opp_table *opp_table; + unsigned long freq; + + opp_table = _find_opp_table(dev); + if (IS_ERR(opp_table)) + return ERR_CAST(opp_table); + + if (!IS_ERR(opp_table->clk)) { + freq = clk_get_rate(opp_table->clk); + opp = _find_freq_ceil(opp_table, &freq); + } + + /* Drop reference taken by _find_opp_table() */ + dev_pm_opp_put_opp_table(opp_table); + + return opp; +} +EXPORT_SYMBOL_GPL(dev_pm_opp_from_clk_rate); diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c index 7c9bc93147f1..03bad16e5318 100644 --- a/drivers/soc/tegra/pmc.c +++ b/drivers/soc/tegra/pmc.c @@ -506,6 +506,63 @@ static void tegra_pmc_scratch_writel(struct tegra_pmc *pmc, u32 value, writel(value, pmc->scratch + offset); } +static const char * const tegra_skip_compats[] = { + "nvidia,tegra20-sclk", + "nvidia,tegra30-sclk", + "nvidia,tegra30-pllc", + "nvidia,tegra30-plle", + "nvidia,tegra30-pllm", + "nvidia,tegra20-dc", + "nvidia,tegra30-dc", + "nvidia,tegra20-emc", + "nvidia,tegra30-emc", + NULL, +}; + +static int tegra_pmc_pd_get_performance_state(struct generic_pm_domain *genpd, + struct device *dev) +{ + struct dev_pm_opp *opp; + int ret; + + /* + * Tegra114+ SocS don't support OPP yet. But if they will get OPP + * support, then we want to skip OPP for older kernels to preserve + * compatibility of newer DTBs with older kernels. + */ + if (!pmc->soc->supports_core_domain) + return 0; + + /* + * The EMC devices are a special case because we have a protection + * from non-EMC drivers getting clock handle before EMC driver is + * fully initialized. The goal of the protection is to prevent + * devfreq driver from getting failures if it will try to change + * EMC clock rate until clock is fully initialized. The EMC drivers + * will initialize the performance state by themselves. + * + * Display controller also is a special case because only controller + * driver could get the clock rate based on configuration of internal + * divider. + * + * Clock driver uses its own state syncing. + */ + if (of_device_compatible_match(dev->of_node, tegra_skip_compats)) + return 0; + + opp = dev_pm_opp_from_clk_rate(dev); + if (IS_ERR(opp)) { + dev_err(&genpd->dev, "failed to get current OPP for %s: %pe\n", + dev_name(dev), opp); + ret = PTR_ERR(opp); + } else { + ret = dev_pm_opp_get_required_pstate(opp, 0); + dev_pm_opp_put(opp); + } + + return ret; +} + /* * TODO Figure out a way to call this with the struct tegra_pmc * passed in. * This currently doesn't work because readx_poll_timeout() can only operate @@ -1238,6 +1295,7 @@ static int tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np) pg->id = id; pg->genpd.name = np->name; + pg->genpd.get_performance_state = tegra_pmc_pd_get_performance_state; pg->genpd.power_off = tegra_genpd_power_off; pg->genpd.power_on = tegra_genpd_power_on; pg->pmc = pmc; @@ -1354,6 +1412,7 @@ static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np) return -ENOMEM; genpd->name = "core"; + genpd->get_performance_state = tegra_pmc_pd_get_performance_state; genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state; genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state; diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 67017c9390c8..abe33be9828f 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -133,6 +133,8 @@ struct generic_pm_domain { struct dev_pm_opp *opp); int (*set_performance_state)(struct generic_pm_domain *genpd, unsigned int state); + int (*get_performance_state)(struct generic_pm_domain *genpd, + struct device *dev); struct gpd_dev_ops dev_ops; s64 max_off_time_ns; /* Maximum allowed "suspended" time. */ ktime_t next_wakeup; /* Maintained by the domain governor */ diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 686122b59935..e7fd0dd493ca 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -169,6 +169,7 @@ void dev_pm_opp_remove_table(struct device *dev); void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask); int dev_pm_opp_sync_regulators(struct device *dev); int dev_pm_opp_sync(struct device *dev); +struct dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev); #else static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev) { @@ -440,6 +441,11 @@ static inline int dev_pm_opp_sync(struct device *dev) return -EOPNOTSUPP; } +static struct inline dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev) +{ + return ERR_PTR(-EOPNOTSUPP); +} + #endif /* CONFIG_PM_OPP */ #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF) --
20.08.2021 15:57, Ulf Hansson пишет: ... >> We already have similar APIs, so that won't be a problem. We also have >> a mechanism inside the OPP core, frequency based, which is used to >> guess the current OPP. Maybe we can enhance and use that directly >> here. > > After reading the last reply from Dmitry, I am starting to think that > the problem he is facing can be described and solved in a much easier > way. > > If I am correct, it looks like we don't need to add APIs to get OPPs > for a clock rate or set initial performance state values according to > the HW in genpd. > > See my other response to Dmitry, let's see where that leads us. I'm going to start preparing v9 with GENPD performance state syncing moved into driver's probe where appropriate. It's not clear to me whether it will be okay to add a generic OPP syncing by clock rate or should it be a Tegra-specific helper. Viresh, what do you think about this generic OPP helper: /** * dev_pm_opp_sync_with_clk_rate() - Sync OPP state with clock rate * @dev: device for which we do this operation * * Sync OPP table state with the current clock rate of device. * * Return: 0 on success or a negative error value. */ int dev_pm_opp_sync_with_clk_rate(struct device *dev) { struct opp_table *opp_table; int ret = 0; /* Device may not have OPP table */ opp_table = _find_opp_table(dev); if (IS_ERR(opp_table)) return 0; /* Device may not use clock */ if (IS_ERR(opp_table->clk)) goto put_table; /* Device may have empty OPP table */ if (!_get_opp_count(opp_table)) goto put_table; ret = dev_pm_opp_set_rate(dev, clk_get_rate(opp_table->clk)); put_table: /* Drop reference taken by _find_opp_table() */ dev_pm_opp_put_opp_table(opp_table); return ret; } EXPORT_SYMBOL_GPL(dev_pm_opp_sync_with_clk_rate);
On 23-08-21, 23:24, Dmitry Osipenko wrote: > It's not clear to me whether it will be okay to add a generic OPP syncing by clock rate or should it be a Tegra-specific helper. Viresh, what do you think about this generic OPP helper: > > /** > * dev_pm_opp_sync_with_clk_rate() - Sync OPP state with clock rate > * @dev: device for which we do this operation > * > * Sync OPP table state with the current clock rate of device. > * > * Return: 0 on success or a negative error value. > */ > int dev_pm_opp_sync_with_clk_rate(struct device *dev) > { > struct opp_table *opp_table; > int ret = 0; > > /* Device may not have OPP table */ > opp_table = _find_opp_table(dev); > if (IS_ERR(opp_table)) > return 0; > > /* Device may not use clock */ > if (IS_ERR(opp_table->clk)) > goto put_table; > > /* Device may have empty OPP table */ > if (!_get_opp_count(opp_table)) > goto put_table; > > ret = dev_pm_opp_set_rate(dev, clk_get_rate(opp_table->clk)); > put_table: > /* Drop reference taken by _find_opp_table() */ > dev_pm_opp_put_opp_table(opp_table); > > return ret; > } > EXPORT_SYMBOL_GPL(dev_pm_opp_sync_with_clk_rate); I am not sure why you still need this, hope we were going another way ? Anyway I will have a look at what you have posted now.
On 25-08-21, 18:41, Dmitry Osipenko wrote: > Thinking a bit more about this, I got a nicer variant which actually works in all cases for Tegra. > > Viresh / Ulf, what do you think about this: This is what I have been suggesting from day 1 :) https://lore.kernel.org/linux-staging/20210818055849.ybfajzu75ecpdrbn@vireshk-i7/ " And if it is all about just syncing the genpd core, then can the genpd core do something like what clk framework does? i.e. allow a new optional genpd callback, get_performance_state() (just like set_performance_state()), which can be called initially by the core to get the performance to something other than zero. " Looks good to me :) -- viresh
On 26-08-21, 08:24, Viresh Kumar wrote: > On 25-08-21, 18:41, Dmitry Osipenko wrote: > > Thinking a bit more about this, I got a nicer variant which actually works in all cases for Tegra. > > > > Viresh / Ulf, what do you think about this: > > This is what I have been suggesting from day 1 :) > > https://lore.kernel.org/linux-staging/20210818055849.ybfajzu75ecpdrbn@vireshk-i7/ > > " > And if it is all about just syncing the genpd core, then can the > genpd core do something like what clk framework does? i.e. allow a > new optional genpd callback, get_performance_state() (just like > set_performance_state()), which can be called initially by the core > to get the performance to something other than zero. > " > > Looks good to me :) When you refresh this stuff, please send only 3-4 patches to update the core stuff and show an example. Once we finalize with the interface, you can update all the users. Else this is just noise for everyone else.
This series adds runtime PM support to Tegra drivers and enables core voltage scaling for Tegra20/30 SoCs, resolving overheating troubles. All patches should go via Tegra tree because they are interdependent, please review and ack. If you haven't seen this series before, that's because I wanted to finalize the GENPD part at first and didn't bother you previously. Changelog: 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 (34): opp: Add dev_pm_opp_sync() helper soc/tegra: pmc: Disable PMC state syncing soc/tegra: Don't print error message when OPPs not available soc/tegra: Add devm_tegra_core_dev_init_opp_table_simple() soc/tegra: Use dev_pm_opp_sync() dt-bindings: clock: tegra-car: Document new tegra-clocks sub-node 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 host1x_channel_stop() gpu: host1x: Add runtime PM and OPP support drm/tegra: dc: Support OPP and SoC core voltage scaling drm/tegra: hdmi: Add OPP support drm/tegra: gr2d: Support power management drm/tegra: gr3d: Support power management drm/tegra: vic: Support system suspend 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 and OPP soc/tegra: fuse: Add OPP support soc/tegra: fuse: Reset hardware soc/tegra: regulators: Prepare for suspend soc/tegra: pmc: Enable core domain support for Tegra20 and Tegra30 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 | 51 + .../display/tegra/nvidia,tegra20-host1x.txt | 53 + .../bindings/media/nvidia,tegra-vde.txt | 64 - .../bindings/media/nvidia,tegra-vde.yaml | 119 ++ .../boot/dts/tegra20-acer-a500-picasso.dts | 1 + arch/arm/boot/dts/tegra20-colibri.dtsi | 3 +- arch/arm/boot/dts/tegra20-harmony.dts | 3 +- arch/arm/boot/dts/tegra20-paz00.dts | 1 + .../arm/boot/dts/tegra20-peripherals-opp.dtsi | 941 +++++++++++ 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 | 119 +- .../tegra30-asus-nexus7-grouper-common.dtsi | 1 + arch/arm/boot/dts/tegra30-beaver.dts | 1 + arch/arm/boot/dts/tegra30-cardhu.dtsi | 1 + arch/arm/boot/dts/tegra30-colibri.dtsi | 17 +- arch/arm/boot/dts/tegra30-ouya.dts | 1 + .../arm/boot/dts/tegra30-peripherals-opp.dtsi | 1412 +++++++++++++++++ arch/arm/boot/dts/tegra30.dtsi | 181 ++- drivers/bus/tegra-gmi.c | 92 +- drivers/clk/tegra/Makefile | 1 + drivers/clk/tegra/clk-device.c | 222 +++ drivers/clk/tegra/clk-pll.c | 2 +- drivers/clk/tegra/clk-super.c | 2 +- drivers/clk/tegra/clk-tegra20.c | 39 +- drivers/clk/tegra/clk-tegra30.c | 70 +- drivers/clk/tegra/clk.c | 64 + drivers/clk/tegra/clk.h | 2 + drivers/gpu/drm/tegra/dc.c | 74 + drivers/gpu/drm/tegra/dc.h | 2 + drivers/gpu/drm/tegra/gr2d.c | 154 +- drivers/gpu/drm/tegra/gr3d.c | 393 ++++- drivers/gpu/drm/tegra/hdmi.c | 15 +- drivers/gpu/drm/tegra/vic.c | 4 + drivers/gpu/host1x/channel.c | 8 + drivers/gpu/host1x/debug.c | 15 + drivers/gpu/host1x/dev.c | 157 +- 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 | 146 +- drivers/mtd/nand/raw/tegra_nand.c | 62 +- drivers/opp/core.c | 42 +- drivers/pwm/pwm-tegra.c | 104 +- drivers/soc/tegra/common.c | 34 +- drivers/soc/tegra/fuse/fuse-tegra.c | 36 + drivers/soc/tegra/fuse/fuse.h | 1 + drivers/soc/tegra/pmc.c | 17 + drivers/soc/tegra/regulators-tegra20.c | 99 ++ drivers/soc/tegra/regulators-tegra30.c | 122 ++ drivers/spi/spi-tegra20-slink.c | 15 +- drivers/staging/media/tegra-vde/vde.c | 65 +- drivers/usb/chipidea/ci_hdrc_tegra.c | 61 +- include/linux/host1x.h | 1 + include/linux/pm_opp.h | 6 + include/soc/tegra/common.h | 13 + 59 files changed, 4796 insertions(+), 384 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