| Message ID | 20210817012754.8710-1-digetx@gmail.com |
|---|---|
| Headers | show |
| Series | NVIDIA Tegra power management patches for 5.16 | expand |
On Tue, Aug 17, 2021 at 04:27:42AM +0300, Dmitry Osipenko wrote: > The SPI on Tegra belongs to the core power domain and we're going to > enable GENPD support for the core domain. Now SPI driver must use OPP > API for driving the controller's clock rate because OPP API takes care > of reconfiguring the domain's performance state in accordance to the > rate. Add OPP support to the driver. Acked-by: Mark Brown <broonie@kernel.org> Is there a concrete dependency here or can I merge this separately?
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 15:22, Mark Brown пишет: > On Tue, Aug 17, 2021 at 04:27:42AM +0300, Dmitry Osipenko wrote: >> The SPI on Tegra belongs to the core power domain and we're going to >> enable GENPD support for the core domain. Now SPI driver must use OPP >> API for driving the controller's clock rate because OPP API takes care >> of reconfiguring the domain's performance state in accordance to the >> rate. Add OPP support to the driver. > > Acked-by: Mark Brown <broonie@kernel.org> > > Is there a concrete dependency here or can I merge this separately? This patch depends on the new OPP helpers added earlier in this series. In particular it depends on these patches: opp: Add dev_pm_opp_sync() helper soc/tegra: Add devm_tegra_core_dev_init_opp_table_simple() Thank you for the ack!
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. > > Optional properties: > @@ -197,6 +198,7 @@ of the following host1x client modules: > - 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 > - 2d > > Optional properties: > @@ -222,6 +224,8 @@ of the following host1x client modules: > - 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 > + - mc2 (Only required on SoCs with two 3D clocks) > - 3d > - 3d2 (Only required on SoCs with two 3D clocks) > > -- > 2.32.0 > >
On Tue, 17 Aug 2021 04:27:44 +0300, Dmitry Osipenko wrote: > Document new OPP table and power domain properties of the video decoder > hardware. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > .../devicetree/bindings/media/nvidia,tegra-vde.yaml | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > Reviewed-by: Rob Herring <robh@kernel.org>
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.
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.
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 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 Tue, Aug 17, 2021 at 04:27:27AM +0300, Dmitry Osipenko wrote: [...] > +struct clk *tegra_clk_register(struct clk_hw *hw) > +{ > + struct platform_device *pdev; > + struct device *dev = NULL; > + struct device_node *np; > + const char *dev_name; > + > + np = tegra_clk_get_of_node(hw); > + > + if (!of_device_is_available(np)) > + goto put_node; > + > + dev_name = kasprintf(GFP_KERNEL, "tegra_clk_%s", hw->init->name); > + if (!dev_name) > + goto put_node; > + > + pdev = of_platform_device_create(np, dev_name, NULL); > + if (!pdev) { > + pr_err("%s: failed to create device for %pOF\n", __func__, np); > + kfree(dev_name); > + goto put_node; > + } > + > + dev = &pdev->dev; > + pm_runtime_enable(dev); > +put_node: > + of_node_put(np); > + > + return clk_register(dev, hw); > +} This looks wrong. Why do we need struct platform_device objects for each of these clocks? That's going to be a massive amount of platform devices and they will completely mess up sysfs. Thierry
18.08.2021 17:07, Thierry Reding пишет: > On Tue, Aug 17, 2021 at 04:27:27AM +0300, Dmitry Osipenko wrote: > [...] >> +struct clk *tegra_clk_register(struct clk_hw *hw) >> +{ >> + struct platform_device *pdev; >> + struct device *dev = NULL; >> + struct device_node *np; >> + const char *dev_name; >> + >> + np = tegra_clk_get_of_node(hw); >> + >> + if (!of_device_is_available(np)) >> + goto put_node; >> + >> + dev_name = kasprintf(GFP_KERNEL, "tegra_clk_%s", hw->init->name); >> + if (!dev_name) >> + goto put_node; >> + >> + pdev = of_platform_device_create(np, dev_name, NULL); >> + if (!pdev) { >> + pr_err("%s: failed to create device for %pOF\n", __func__, np); >> + kfree(dev_name); >> + goto put_node; >> + } >> + >> + dev = &pdev->dev; >> + pm_runtime_enable(dev); >> +put_node: >> + of_node_put(np); >> + >> + return clk_register(dev, hw); >> +} > > This looks wrong. Why do we need struct platform_device objects for each > of these clocks? That's going to be a massive amount of platform devices > and they will completely mess up sysfs. RPM works with a device. It's not a massive amount of devices, it's one device for T20 and four devices for T30.
On Wed, Aug 18, 2021 at 06:05:21PM +0300, Dmitry Osipenko wrote: > 18.08.2021 17:07, Thierry Reding пишет: > > On Tue, Aug 17, 2021 at 04:27:27AM +0300, Dmitry Osipenko wrote: > > [...] > >> +struct clk *tegra_clk_register(struct clk_hw *hw) > >> +{ > >> + struct platform_device *pdev; > >> + struct device *dev = NULL; > >> + struct device_node *np; > >> + const char *dev_name; > >> + > >> + np = tegra_clk_get_of_node(hw); > >> + > >> + if (!of_device_is_available(np)) > >> + goto put_node; > >> + > >> + dev_name = kasprintf(GFP_KERNEL, "tegra_clk_%s", hw->init->name); > >> + if (!dev_name) > >> + goto put_node; > >> + > >> + pdev = of_platform_device_create(np, dev_name, NULL); > >> + if (!pdev) { > >> + pr_err("%s: failed to create device for %pOF\n", __func__, np); > >> + kfree(dev_name); > >> + goto put_node; > >> + } > >> + > >> + dev = &pdev->dev; > >> + pm_runtime_enable(dev); > >> +put_node: > >> + of_node_put(np); > >> + > >> + return clk_register(dev, hw); > >> +} > > > > This looks wrong. Why do we need struct platform_device objects for each > > of these clocks? That's going to be a massive amount of platform devices > > and they will completely mess up sysfs. > > RPM works with a device. It's not a massive amount of devices, it's one > device for T20 and four devices for T30. I'm still not sure I understand why we need to call RPM functions on a clock. And even if they are few, it seems wrong to make these platform devices. Perhaps they can be simple struct device:s instead? Ideally they would also be parented to the CAR so that they appear in the right place in the sysfs hierarchy. Thierry
18.08.2021 19:42, Thierry Reding пишет: > On Wed, Aug 18, 2021 at 06:05:21PM +0300, Dmitry Osipenko wrote: >> 18.08.2021 17:07, Thierry Reding пишет: >>> On Tue, Aug 17, 2021 at 04:27:27AM +0300, Dmitry Osipenko wrote: >>> [...] >>>> +struct clk *tegra_clk_register(struct clk_hw *hw) >>>> +{ >>>> + struct platform_device *pdev; >>>> + struct device *dev = NULL; >>>> + struct device_node *np; >>>> + const char *dev_name; >>>> + >>>> + np = tegra_clk_get_of_node(hw); >>>> + >>>> + if (!of_device_is_available(np)) >>>> + goto put_node; >>>> + >>>> + dev_name = kasprintf(GFP_KERNEL, "tegra_clk_%s", hw->init->name); >>>> + if (!dev_name) >>>> + goto put_node; >>>> + >>>> + pdev = of_platform_device_create(np, dev_name, NULL); >>>> + if (!pdev) { >>>> + pr_err("%s: failed to create device for %pOF\n", __func__, np); >>>> + kfree(dev_name); >>>> + goto put_node; >>>> + } >>>> + >>>> + dev = &pdev->dev; >>>> + pm_runtime_enable(dev); >>>> +put_node: >>>> + of_node_put(np); >>>> + >>>> + return clk_register(dev, hw); >>>> +} >>> >>> This looks wrong. Why do we need struct platform_device objects for each >>> of these clocks? That's going to be a massive amount of platform devices >>> and they will completely mess up sysfs. >> >> RPM works with a device. It's not a massive amount of devices, it's one >> device for T20 and four devices for T30. > > I'm still not sure I understand why we need to call RPM functions on a > clock. And even if they are few, it seems wrong to make these platform > devices. Before clock is enabled, we need to raise core voltage. After clock is disabled, the voltage should be dropped. CCF+RPM takes care of handling this for us. > Perhaps they can be simple struct device:s instead? Ideally they would > also be parented to the CAR so that they appear in the right place in > the sysfs hierarchy. Could you please clarify what do you mean by 'simple struct device:s'? These clock devices should be OF devices with a of_node and etc, otherwise we can't use OPP framework. We don't have driver for CAR to bind. I guess we could try to add a 'dummy' CAR driver that will create sub-devices for the rpm-clocks, is this what you're wanting?
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 Tue, Aug 17, 2021 at 04:27:39AM +0300, Dmitry Osipenko wrote: > The PWM on Tegra belongs to the core power domain and we're going to > enable GENPD support for the core domain. Now PWM must be resumed using > runtime PM API in order to initialize the PWM power state. The PWM clock > rate must be changed using OPP API that will reconfigure the power domain > performance state in accordance to the rate. Add runtime PM and OPP > support to the PWM driver. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/pwm/pwm-tegra.c | 104 ++++++++++++++++++++++++++++++++-------- > 1 file changed, 85 insertions(+), 19 deletions(-) Can this be safely applied independently of the rest of the series, or are there any dependencies on earlier patches? Thierry
On Thu, 19 Aug 2021 at 15:21, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Tue, Aug 17, 2021 at 04:27:39AM +0300, Dmitry Osipenko wrote: > > The PWM on Tegra belongs to the core power domain and we're going to > > enable GENPD support for the core domain. Now PWM must be resumed using > > runtime PM API in order to initialize the PWM power state. The PWM clock > > rate must be changed using OPP API that will reconfigure the power domain > > performance state in accordance to the rate. Add runtime PM and OPP > > support to the PWM driver. > > > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > --- > > drivers/pwm/pwm-tegra.c | 104 ++++++++++++++++++++++++++++++++-------- > > 1 file changed, 85 insertions(+), 19 deletions(-) > > Can this be safely applied independently of the rest of the series, or > are there any dependencies on earlier patches? Just to make sure we don't rush something in, I would rather withhold all runtime PM related patches in the series, until we have agreed on how to fix the in genpd/opp core parts. Simply, because those may very well affect the deployments in the drivers. > > Thierry Kind regards Uffe
On Thu, Aug 19, 2021 at 04:04:50PM +0200, Ulf Hansson wrote: > On Thu, 19 Aug 2021 at 15:21, Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Tue, Aug 17, 2021 at 04:27:39AM +0300, Dmitry Osipenko wrote: > > > The PWM on Tegra belongs to the core power domain and we're going to > > > enable GENPD support for the core domain. Now PWM must be resumed using > > > runtime PM API in order to initialize the PWM power state. The PWM clock > > > rate must be changed using OPP API that will reconfigure the power domain > > > performance state in accordance to the rate. Add runtime PM and OPP > > > support to the PWM driver. > > > > > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > > --- > > > drivers/pwm/pwm-tegra.c | 104 ++++++++++++++++++++++++++++++++-------- > > > 1 file changed, 85 insertions(+), 19 deletions(-) > > > > Can this be safely applied independently of the rest of the series, or > > are there any dependencies on earlier patches? > > Just to make sure we don't rush something in, I would rather withhold > all runtime PM related patches in the series, until we have agreed on > how to fix the in genpd/opp core parts. Simply, because those may very > well affect the deployments in the drivers. Okay, understood. I didn't realize this may have an impact on how drivers need to cooperate. I'll hold off on applying any of these patches until the discussion has settled, then. Thierry
On Wed, Aug 18, 2021 at 08:11:03PM +0300, Dmitry Osipenko wrote: > 18.08.2021 19:42, Thierry Reding пишет: > > On Wed, Aug 18, 2021 at 06:05:21PM +0300, Dmitry Osipenko wrote: > >> 18.08.2021 17:07, Thierry Reding пишет: > >>> On Tue, Aug 17, 2021 at 04:27:27AM +0300, Dmitry Osipenko wrote: > >>> [...] > >>>> +struct clk *tegra_clk_register(struct clk_hw *hw) > >>>> +{ > >>>> + struct platform_device *pdev; > >>>> + struct device *dev = NULL; > >>>> + struct device_node *np; > >>>> + const char *dev_name; > >>>> + > >>>> + np = tegra_clk_get_of_node(hw); > >>>> + > >>>> + if (!of_device_is_available(np)) > >>>> + goto put_node; > >>>> + > >>>> + dev_name = kasprintf(GFP_KERNEL, "tegra_clk_%s", hw->init->name); > >>>> + if (!dev_name) > >>>> + goto put_node; > >>>> + > >>>> + pdev = of_platform_device_create(np, dev_name, NULL); > >>>> + if (!pdev) { > >>>> + pr_err("%s: failed to create device for %pOF\n", __func__, np); > >>>> + kfree(dev_name); > >>>> + goto put_node; > >>>> + } > >>>> + > >>>> + dev = &pdev->dev; > >>>> + pm_runtime_enable(dev); > >>>> +put_node: > >>>> + of_node_put(np); > >>>> + > >>>> + return clk_register(dev, hw); > >>>> +} > >>> > >>> This looks wrong. Why do we need struct platform_device objects for each > >>> of these clocks? That's going to be a massive amount of platform devices > >>> and they will completely mess up sysfs. > >> > >> RPM works with a device. It's not a massive amount of devices, it's one > >> device for T20 and four devices for T30. > > > > I'm still not sure I understand why we need to call RPM functions on a > > clock. And even if they are few, it seems wrong to make these platform > > devices. > > Before clock is enabled, we need to raise core voltage. After clock is > disabled, the voltage should be dropped. CCF+RPM takes care of handling > this for us. That's the part that I do understand. What I don't understand is why a clock needs to be runtime suspend/resumed. Typically we suspend/resume devices, and doing so typically involves disabling/enabling clocks. So I don't understand why the clocks themselves now need to be runtime suspended/resumed. > > Perhaps they can be simple struct device:s instead? Ideally they would > > also be parented to the CAR so that they appear in the right place in > > the sysfs hierarchy. > > Could you please clarify what do you mean by 'simple struct device:s'? > These clock devices should be OF devices with a of_node and etc, > otherwise we can't use OPP framework. Perhaps I misunderstand the goal of the OPP framework. My understanding was that this was to attach a table of operating points with a device so that appropriate operating points could be selected and switched to when the workload changes. Typically these operating points would be roughly a clock rate and a corresponding voltage for a regulator, so that when a certain clock rate is requested, the regulator can be set to the matching voltage. Hm... so is it that each of these clocks that you want to create a platform device for has its own regulator? Because the patch series only mentions the CORE domain, so I assumed that we would accumulate all the clock rates for the clocks that are part of that CORE domain and then derive a voltage to be supplied to that CORE domain. But perhaps I just don't understand correctly how this is tied together. > We don't have driver for CAR to bind. I guess we could try to add a > 'dummy' CAR driver that will create sub-devices for the rpm-clocks, is > this what you're wanting? I got confused by the "tegra-clock" driver that this series was adding. This is actually a driver that will bind to the virtual clocks rather than the CAR device itself. For some reason I had assumed that you wanted to create a CAR driver in order to get at the struct device embedded in the CAR's platform device and use that as the parent for all these clocks. So even if we absolutely need some struct device for these clocks, maybe adding that CAR driver and making the clock struct device:s children of the CAR device will help keep a bit of a proper hierarchy in sysfs. Thierry
19.08.2021 19:54, Thierry Reding пишет: > On Wed, Aug 18, 2021 at 08:11:03PM +0300, Dmitry Osipenko wrote: >> 18.08.2021 19:42, Thierry Reding пишет: >>> On Wed, Aug 18, 2021 at 06:05:21PM +0300, Dmitry Osipenko wrote: >>>> 18.08.2021 17:07, Thierry Reding пишет: >>>>> On Tue, Aug 17, 2021 at 04:27:27AM +0300, Dmitry Osipenko wrote: >>>>> [...] >>>>>> +struct clk *tegra_clk_register(struct clk_hw *hw) >>>>>> +{ >>>>>> + struct platform_device *pdev; >>>>>> + struct device *dev = NULL; >>>>>> + struct device_node *np; >>>>>> + const char *dev_name; >>>>>> + >>>>>> + np = tegra_clk_get_of_node(hw); >>>>>> + >>>>>> + if (!of_device_is_available(np)) >>>>>> + goto put_node; >>>>>> + >>>>>> + dev_name = kasprintf(GFP_KERNEL, "tegra_clk_%s", hw->init->name); >>>>>> + if (!dev_name) >>>>>> + goto put_node; >>>>>> + >>>>>> + pdev = of_platform_device_create(np, dev_name, NULL); >>>>>> + if (!pdev) { >>>>>> + pr_err("%s: failed to create device for %pOF\n", __func__, np); >>>>>> + kfree(dev_name); >>>>>> + goto put_node; >>>>>> + } >>>>>> + >>>>>> + dev = &pdev->dev; >>>>>> + pm_runtime_enable(dev); >>>>>> +put_node: >>>>>> + of_node_put(np); >>>>>> + >>>>>> + return clk_register(dev, hw); >>>>>> +} >>>>> >>>>> This looks wrong. Why do we need struct platform_device objects for each >>>>> of these clocks? That's going to be a massive amount of platform devices >>>>> and they will completely mess up sysfs. >>>> >>>> RPM works with a device. It's not a massive amount of devices, it's one >>>> device for T20 and four devices for T30. >>> >>> I'm still not sure I understand why we need to call RPM functions on a >>> clock. And even if they are few, it seems wrong to make these platform >>> devices. >> >> Before clock is enabled, we need to raise core voltage. After clock is >> disabled, the voltage should be dropped. CCF+RPM takes care of handling >> this for us. > > That's the part that I do understand. What I don't understand is why a > clock needs to be runtime suspend/resumed. Typically we suspend/resume > devices, and doing so typically involves disabling/enabling clocks. So > I don't understand why the clocks themselves now need to be runtime > suspended/resumed. CCF provides RPM management for a device that backs clock. When clock is enabled, it resumes the backing device. RPM, GENPD and OPP frameworks work with a device. We use all these frameworks here. Since we don't have a dedicated device for a PLL clock, we need to create it in order to leverage the existing generic kernel APIs. In this case clocks are not runtime suspended/resumed, the device which backs clock is suspended/resumed. >>> Perhaps they can be simple struct device:s instead? Ideally they would >>> also be parented to the CAR so that they appear in the right place in >>> the sysfs hierarchy. >> >> Could you please clarify what do you mean by 'simple struct device:s'? >> These clock devices should be OF devices with a of_node and etc, >> otherwise we can't use OPP framework. > > Perhaps I misunderstand the goal of the OPP framework. My understanding > was that this was to attach a table of operating points with a device so > that appropriate operating points could be selected and switched to when > the workload changes. > > Typically these operating points would be roughly a clock rate and a > corresponding voltage for a regulator, so that when a certain clock rate > is requested, the regulator can be set to the matching voltage. > > Hm... so is it that each of these clocks that you want to create a > platform device for has its own regulator? Because the patch series only > mentions the CORE domain, so I assumed that we would accumulate all the > clock rates for the clocks that are part of that CORE domain and then > derive a voltage to be supplied to that CORE domain. > > But perhaps I just don't understand correctly how this is tied together. We don't use regulators, we use power domain that controls regulator. GENPD takes care of accumulating performance requests on a per-device basis. I'm creating platform device for the clocks that require DVFS. These clocks don't use regulator, they are attached to the CORE domain. GENPD framework manages the performance state, aggregating perf votes from each device, i.e. from each clock individually. You want to reinvent another layer of aggregation on top of GENPD. This doesn't worth the effort, we won't get anything from it, it should be a lot of extra complexity for nothing. We will also lose from it because pm_genpd_summary won't show you a per-device info. domain status children performance /device runtime status ---------------------------------------------------------------------------------------------- heg on 1000000 /devices/soc0/50000000.host1x active 1000000 /devices/soc0/50000000.host1x/54140000.gr2d suspended 0 mpe off-0 0 vdec off-0 0 /devices/soc0/6001a000.vde suspended 0 venc off-0 0 3d1 off-0 0 /devices/genpd:1:54180000.gr3d suspended 0 3d0 off-0 0 /devices/genpd:0:54180000.gr3d suspended 0 core-domain on 1000000 3d0, 3d1, venc, vdec, mpe, heg /devices/soc0/7d000000.usb active 1000000 /devices/soc0/78000400.mmc active 950000 /devices/soc0/7000f400.memory-controller unsupported 1000000 /devices/soc0/7000a000.pwm active 1000000 /devices/soc0/60006000.clock/tegra_clk_pll_c active 1000000 /devices/soc0/60006000.clock/tegra_clk_pll_e suspended 0 /devices/soc0/60006000.clock/tegra_clk_pll_m active 1000000 /devices/soc0/60006000.clock/tegra_clk_sclk active 1000000 >> We don't have driver for CAR to bind. I guess we could try to add a >> 'dummy' CAR driver that will create sub-devices for the rpm-clocks, is >> this what you're wanting? > > I got confused by the "tegra-clock" driver that this series was adding. > This is actually a driver that will bind to the virtual clocks rather > than the CAR device itself. > > For some reason I had assumed that you wanted to create a CAR driver in > order to get at the struct device embedded in the CAR's platform device > and use that as the parent for all these clocks. > > So even if we absolutely need some struct device for these clocks, maybe > adding that CAR driver and making the clock struct device:s children of > the CAR device will help keep a bit of a proper hierarchy in sysfs. Alright, that's easy to do. We will have to move out some clk data out of __init then. I already implemented it as you may see in the above PD summary.
On Fri, Aug 20, 2021 at 01:09:46AM +0300, Dmitry Osipenko wrote: > 19.08.2021 19:54, Thierry Reding пишет: > > On Wed, Aug 18, 2021 at 08:11:03PM +0300, Dmitry Osipenko wrote: > >> 18.08.2021 19:42, Thierry Reding пишет: > >>> On Wed, Aug 18, 2021 at 06:05:21PM +0300, Dmitry Osipenko wrote: > >>>> 18.08.2021 17:07, Thierry Reding пишет: > >>>>> On Tue, Aug 17, 2021 at 04:27:27AM +0300, Dmitry Osipenko wrote: > >>>>> [...] > >>>>>> +struct clk *tegra_clk_register(struct clk_hw *hw) > >>>>>> +{ > >>>>>> + struct platform_device *pdev; > >>>>>> + struct device *dev = NULL; > >>>>>> + struct device_node *np; > >>>>>> + const char *dev_name; > >>>>>> + > >>>>>> + np = tegra_clk_get_of_node(hw); > >>>>>> + > >>>>>> + if (!of_device_is_available(np)) > >>>>>> + goto put_node; > >>>>>> + > >>>>>> + dev_name = kasprintf(GFP_KERNEL, "tegra_clk_%s", hw->init->name); > >>>>>> + if (!dev_name) > >>>>>> + goto put_node; > >>>>>> + > >>>>>> + pdev = of_platform_device_create(np, dev_name, NULL); > >>>>>> + if (!pdev) { > >>>>>> + pr_err("%s: failed to create device for %pOF\n", __func__, np); > >>>>>> + kfree(dev_name); > >>>>>> + goto put_node; > >>>>>> + } > >>>>>> + > >>>>>> + dev = &pdev->dev; > >>>>>> + pm_runtime_enable(dev); > >>>>>> +put_node: > >>>>>> + of_node_put(np); > >>>>>> + > >>>>>> + return clk_register(dev, hw); > >>>>>> +} > >>>>> > >>>>> This looks wrong. Why do we need struct platform_device objects for each > >>>>> of these clocks? That's going to be a massive amount of platform devices > >>>>> and they will completely mess up sysfs. > >>>> > >>>> RPM works with a device. It's not a massive amount of devices, it's one > >>>> device for T20 and four devices for T30. > >>> > >>> I'm still not sure I understand why we need to call RPM functions on a > >>> clock. And even if they are few, it seems wrong to make these platform > >>> devices. > >> > >> Before clock is enabled, we need to raise core voltage. After clock is > >> disabled, the voltage should be dropped. CCF+RPM takes care of handling > >> this for us. > > > > That's the part that I do understand. What I don't understand is why a > > clock needs to be runtime suspend/resumed. Typically we suspend/resume > > devices, and doing so typically involves disabling/enabling clocks. So > > I don't understand why the clocks themselves now need to be runtime > > suspended/resumed. > > CCF provides RPM management for a device that backs clock. When clock > is enabled, it resumes the backing device. > > RPM, GENPD and OPP frameworks work with a device. We use all these > frameworks here. Since we don't have a dedicated device for a PLL > clock, we need to create it in order to leverage the existing generic > kernel APIs. > > In this case clocks are not runtime suspended/resumed, the device > which backs clock is suspended/resumed. > > >>> Perhaps they can be simple struct device:s instead? Ideally they would > >>> also be parented to the CAR so that they appear in the right place in > >>> the sysfs hierarchy. > >> > >> Could you please clarify what do you mean by 'simple struct device:s'? > >> These clock devices should be OF devices with a of_node and etc, > >> otherwise we can't use OPP framework. > > > > Perhaps I misunderstand the goal of the OPP framework. My understanding > > was that this was to attach a table of operating points with a device so > > that appropriate operating points could be selected and switched to when > > the workload changes. > > > > Typically these operating points would be roughly a clock rate and a > > corresponding voltage for a regulator, so that when a certain clock rate > > is requested, the regulator can be set to the matching voltage. > > > > Hm... so is it that each of these clocks that you want to create a > > platform device for has its own regulator? Because the patch series only > > mentions the CORE domain, so I assumed that we would accumulate all the > > clock rates for the clocks that are part of that CORE domain and then > > derive a voltage to be supplied to that CORE domain. > > > > But perhaps I just don't understand correctly how this is tied together. > > We don't use regulators, we use power domain that controls regulator. > GENPD takes care of accumulating performance requests on a per-device > basis. > > I'm creating platform device for the clocks that require DVFS. These > clocks don't use regulator, they are attached to the CORE domain. > GENPD framework manages the performance state, aggregating perf votes > from each device, i.e. from each clock individually. > > You want to reinvent another layer of aggregation on top of GENPD. > This doesn't worth the effort, we won't get anything from it, it > should be a lot of extra complexity for nothing. We will also lose > from it because pm_genpd_summary won't show you a per-device info. > > domain status children performance > /device runtime status > ---------------------------------------------------------------------------------------------- > heg on 1000000 > /devices/soc0/50000000.host1x active 1000000 > /devices/soc0/50000000.host1x/54140000.gr2d suspended 0 > mpe off-0 0 > vdec off-0 0 > /devices/soc0/6001a000.vde suspended 0 > venc off-0 0 > 3d1 off-0 0 > /devices/genpd:1:54180000.gr3d suspended 0 > 3d0 off-0 0 > /devices/genpd:0:54180000.gr3d suspended 0 > core-domain on 1000000 > 3d0, 3d1, venc, vdec, mpe, heg > /devices/soc0/7d000000.usb active 1000000 > /devices/soc0/78000400.mmc active 950000 > /devices/soc0/7000f400.memory-controller unsupported 1000000 > /devices/soc0/7000a000.pwm active 1000000 > /devices/soc0/60006000.clock/tegra_clk_pll_c active 1000000 > /devices/soc0/60006000.clock/tegra_clk_pll_e suspended 0 > /devices/soc0/60006000.clock/tegra_clk_pll_m active 1000000 > /devices/soc0/60006000.clock/tegra_clk_sclk active 1000000 > I suppose if there's really no good way of doing this other than providing a struct device, then so be it. I think the cleaned up sysfs shown in the summary above looks much better than what the original would've looked like. Perhaps an additional tweak to that would be to not create platform devices. Instead, just create struct device. Those really have everything you need (.of_node, and can be used with RPM and GENPD). As I mentioned earlier, platform device implies a CPU-memory-mapped bus, which this clearly isn't. It's kind of a separate "bus" if you want, so just using struct device directly seems more appropriate. We did something similar for XUSB pads, see drivers/phy/tegra/xusb.[ch] for an example of how that was done. I think you can do something similar here. Thierry
[...] > > > > I'm creating platform device for the clocks that require DVFS. These > > clocks don't use regulator, they are attached to the CORE domain. > > GENPD framework manages the performance state, aggregating perf votes > > from each device, i.e. from each clock individually. > > > > You want to reinvent another layer of aggregation on top of GENPD. > > This doesn't worth the effort, we won't get anything from it, it > > should be a lot of extra complexity for nothing. We will also lose > > from it because pm_genpd_summary won't show you a per-device info. > > > > domain status children performance > > /device runtime status > > ---------------------------------------------------------------------------------------------- > > heg on 1000000 > > /devices/soc0/50000000.host1x active 1000000 > > /devices/soc0/50000000.host1x/54140000.gr2d suspended 0 > > mpe off-0 0 > > vdec off-0 0 > > /devices/soc0/6001a000.vde suspended 0 > > venc off-0 0 > > 3d1 off-0 0 > > /devices/genpd:1:54180000.gr3d suspended 0 > > 3d0 off-0 0 > > /devices/genpd:0:54180000.gr3d suspended 0 > > core-domain on 1000000 > > 3d0, 3d1, venc, vdec, mpe, heg > > /devices/soc0/7d000000.usb active 1000000 > > /devices/soc0/78000400.mmc active 950000 > > /devices/soc0/7000f400.memory-controller unsupported 1000000 > > /devices/soc0/7000a000.pwm active 1000000 > > /devices/soc0/60006000.clock/tegra_clk_pll_c active 1000000 > > /devices/soc0/60006000.clock/tegra_clk_pll_e suspended 0 > > /devices/soc0/60006000.clock/tegra_clk_pll_m active 1000000 > > /devices/soc0/60006000.clock/tegra_clk_sclk active 1000000 > > > > I suppose if there's really no good way of doing this other than > providing a struct device, then so be it. I think the cleaned up sysfs > shown in the summary above looks much better than what the original > would've looked like. > > Perhaps an additional tweak to that would be to not create platform > devices. Instead, just create struct device. Those really have > everything you need (.of_node, and can be used with RPM and GENPD). As I > mentioned earlier, platform device implies a CPU-memory-mapped bus, > which this clearly isn't. It's kind of a separate "bus" if you want, so > just using struct device directly seems more appropriate. Just a heads up. If you don't use a platform device or have a driver associated with it for probing, you need to manage the attachment to genpd yourself. That means calling one of the dev_pm_domain_attach*() APIs, but that's perfectly fine, ofcourse. > > We did something similar for XUSB pads, see drivers/phy/tegra/xusb.[ch] > for an example of how that was done. I think you can do something > similar here. > > Thierry Kind regards Uffe
20.08.2021 16:08, Ulf Hansson пишет: ... >> I suppose if there's really no good way of doing this other than >> providing a struct device, then so be it. I think the cleaned up sysfs >> shown in the summary above looks much better than what the original >> would've looked like. >> >> Perhaps an additional tweak to that would be to not create platform >> devices. Instead, just create struct device. Those really have >> everything you need (.of_node, and can be used with RPM and GENPD). As I >> mentioned earlier, platform device implies a CPU-memory-mapped bus, >> which this clearly isn't. It's kind of a separate "bus" if you want, so >> just using struct device directly seems more appropriate. > > Just a heads up. If you don't use a platform device or have a driver > associated with it for probing, you need to manage the attachment to > genpd yourself. That means calling one of the dev_pm_domain_attach*() > APIs, but that's perfectly fine, ofcourse. > >> >> We did something similar for XUSB pads, see drivers/phy/tegra/xusb.[ch] >> for an example of how that was done. I think you can do something >> similar here. We need a platform device because we have a platform device driver that must be bound to the device, otherwise PMC driver state won't be synced since it it's synced after all drivers of devices that reference PMC node in DT are probed.
On Sat, Aug 21, 2021 at 08:45:54PM +0300, Dmitry Osipenko wrote: > 20.08.2021 16:08, Ulf Hansson пишет: > ... > >> I suppose if there's really no good way of doing this other than > >> providing a struct device, then so be it. I think the cleaned up sysfs > >> shown in the summary above looks much better than what the original > >> would've looked like. > >> > >> Perhaps an additional tweak to that would be to not create platform > >> devices. Instead, just create struct device. Those really have > >> everything you need (.of_node, and can be used with RPM and GENPD). As I > >> mentioned earlier, platform device implies a CPU-memory-mapped bus, > >> which this clearly isn't. It's kind of a separate "bus" if you want, so > >> just using struct device directly seems more appropriate. > > > > Just a heads up. If you don't use a platform device or have a driver > > associated with it for probing, you need to manage the attachment to > > genpd yourself. That means calling one of the dev_pm_domain_attach*() > > APIs, but that's perfectly fine, ofcourse. > > > >> > >> We did something similar for XUSB pads, see drivers/phy/tegra/xusb.[ch] > >> for an example of how that was done. I think you can do something > >> similar here. > > We need a platform device because we have a platform device driver that > must be bound to the device, otherwise PMC driver state won't be synced > since it it's synced after all drivers of devices that reference PMC > node in DT are probed. I think the causality is the wrong way around. It's more likely that you added the platform driver because you have a platform device that you want to bind against. You can have drivers bind to other types of devices, although it's a bit more work than abusing platform devices for it. There's the "auxiliary" bus that seems like it would be a somewhat better fit (see Documentation/driver-api/auxiliary_bus.rst), though it doesn't look like this fits the purpose exactly. I think a custom bus (or perhaps something that could be deployed more broadly across CCF) would be more appropriate. Looking around, it seems like clk/imx and clk/samsung abuse the platform bus in a similar way, so they would benefit from a "clk" bus as well. Thierry
23.08.2021 17:33, Thierry Reding пишет: > On Sat, Aug 21, 2021 at 08:45:54PM +0300, Dmitry Osipenko wrote: >> 20.08.2021 16:08, Ulf Hansson пишет: >> ... >>>> I suppose if there's really no good way of doing this other than >>>> providing a struct device, then so be it. I think the cleaned up sysfs >>>> shown in the summary above looks much better than what the original >>>> would've looked like. >>>> >>>> Perhaps an additional tweak to that would be to not create platform >>>> devices. Instead, just create struct device. Those really have >>>> everything you need (.of_node, and can be used with RPM and GENPD). As I >>>> mentioned earlier, platform device implies a CPU-memory-mapped bus, >>>> which this clearly isn't. It's kind of a separate "bus" if you want, so >>>> just using struct device directly seems more appropriate. >>> >>> Just a heads up. If you don't use a platform device or have a driver >>> associated with it for probing, you need to manage the attachment to >>> genpd yourself. That means calling one of the dev_pm_domain_attach*() >>> APIs, but that's perfectly fine, ofcourse. >>> >>>> >>>> We did something similar for XUSB pads, see drivers/phy/tegra/xusb.[ch] >>>> for an example of how that was done. I think you can do something >>>> similar here. >> >> We need a platform device because we have a platform device driver that >> must be bound to the device, otherwise PMC driver state won't be synced >> since it it's synced after all drivers of devices that reference PMC >> node in DT are probed. > > I think the causality is the wrong way around. It's more likely that you > added the platform driver because you have a platform device that you > want to bind against. > > You can have drivers bind to other types of devices, although it's a bit > more work than abusing platform devices for it. > > There's the "auxiliary" bus that seems like it would be a somewhat > better fit (see Documentation/driver-api/auxiliary_bus.rst), though it > doesn't look like this fits the purpose exactly. I think a custom bus > (or perhaps something that could be deployed more broadly across CCF) > would be more appropriate. > > Looking around, it seems like clk/imx and clk/samsung abuse the platform > bus in a similar way, so they would benefit from a "clk" bus as well. It may be nice to have a dedicated clk bus, but this is too much effort for nearly nothing in our case. It shouldn't be a problem to convert drivers to use clk bus once it will be implemented. It shouldn't be a part of this series, IMO.
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