Message ID | 20201217180638.22748-1-digetx@gmail.com |
---|---|
Headers | show |
Series | Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs | expand |
17.12.2020 21:21, Mikko Perttunen пишет: > On 12/17/20 8:06 PM, Dmitry Osipenko wrote: >> Add suspend/resume and generic power domain support to the Host1x driver. >> This is required for enabling system-wide DVFS and supporting dynamic >> power management using a generic power domain. >> >> Tested-by: Peter Geis <pgwipeout@gmail.com> >> Tested-by: Nicolas Chauvet <kwizart@gmail.com> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/gpu/host1x/dev.c | 102 ++++++++++++++++++++++++++++++++++----- >> 1 file changed, 91 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c >> index d0ebb70e2fdd..c1525cffe7b1 100644 >> --- a/drivers/gpu/host1x/dev.c >> +++ b/drivers/gpu/host1x/dev.c >> @@ -12,6 +12,7 @@ >> #include <linux/module.h> >> #include <linux/of_device.h> >> #include <linux/of.h> >> +#include <linux/pm_runtime.h> >> #include <linux/slab.h> >> #define CREATE_TRACE_POINTS >> @@ -417,7 +418,7 @@ static int host1x_probe(struct platform_device *pdev) >> return err; >> } >> - host->rst = devm_reset_control_get(&pdev->dev, "host1x"); >> + host->rst = devm_reset_control_get_exclusive_released(&pdev->dev, >> "host1x"); >> if (IS_ERR(host->rst)) { >> err = PTR_ERR(host->rst); >> dev_err(&pdev->dev, "failed to get reset: %d\n", err); >> @@ -437,16 +438,15 @@ 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; >> - } >> + pm_runtime_enable(&pdev->dev); >> + err = pm_runtime_get_sync(&pdev->dev); >> + if (err < 0) >> + goto rpm_disable; >> err = reset_control_deassert(host->rst); >> if (err < 0) { >> dev_err(&pdev->dev, "failed to deassert reset: %d\n", err); >> - goto unprepare_disable; >> + goto rpm_disable; >> } >> err = host1x_syncpt_init(host); >> @@ -485,9 +485,10 @@ static int host1x_probe(struct platform_device >> *pdev) >> host1x_syncpt_deinit(host); >> reset_assert: >> reset_control_assert(host->rst); >> -unprepare_disable: >> - clk_disable_unprepare(host->clk); >> -free_channels: >> +rpm_disable: >> + pm_runtime_put(&pdev->dev); >> + pm_runtime_disable(&pdev->dev); >> + >> host1x_channel_list_free(&host->channel_list); >> iommu_exit: >> host1x_iommu_exit(host); >> @@ -504,16 +505,95 @@ static int host1x_remove(struct platform_device >> *pdev) >> host1x_intr_deinit(host); >> host1x_syncpt_deinit(host); >> reset_control_assert(host->rst); >> - clk_disable_unprepare(host->clk); >> + pm_runtime_put(&pdev->dev); >> + pm_runtime_disable(&pdev->dev); >> host1x_iommu_exit(host); >> return 0; >> } >> +static int __maybe_unused host1x_runtime_suspend(struct device *dev) >> +{ >> + struct host1x *host = dev_get_drvdata(dev); >> + >> + clk_disable_unprepare(host->clk); >> + reset_control_release(host->rst); >> + >> + return 0; >> +} >> + >> +static int __maybe_unused host1x_runtime_resume(struct device *dev) >> +{ >> + struct host1x *host = dev_get_drvdata(dev); >> + int err; >> + >> + err = reset_control_acquire(host->rst); >> + if (err) { >> + dev_err(dev, "failed to acquire reset: %d\n", err); >> + return err; >> + } >> + >> + err = clk_prepare_enable(host->clk); >> + if (err) { >> + dev_err(dev, "failed to enable clock: %d\n", err); >> + goto release_reset; >> + } >> + >> + return 0; >> + >> +release_reset: >> + reset_control_release(host->rst); >> + >> + return err; >> +} >> + >> +static __maybe_unused int host1x_suspend(struct device *dev) >> +{ >> + struct host1x *host = dev_get_drvdata(dev); >> + int err; >> + >> + host1x_syncpt_save(host); >> + >> + err = pm_runtime_force_suspend(dev); >> + if (err < 0) >> + return err; >> + >> + return 0; >> +} >> + >> +static __maybe_unused int host1x_resume(struct device *dev) >> +{ >> + struct host1x *host = dev_get_drvdata(dev); >> + struct host1x_channel *channel; >> + unsigned int index; >> + int err; >> + >> + err = pm_runtime_force_resume(dev); >> + if (err < 0) >> + return err; >> + >> + host1x_syncpt_restore(host); > > We also need to execute 'host1x_setup_sid_table' upon resume. Indeed, thanks. I'll correct it in the next revision. Perhaps the actual save/restore needs to be moved to the runtime callbacks. At least I can't remember right now why this wasn't done in the first place.
17.12.2020 21:45, Dmitry Osipenko пишет: > 17.12.2020 21:21, Mikko Perttunen пишет: >> On 12/17/20 8:06 PM, Dmitry Osipenko wrote: >>> Add suspend/resume and generic power domain support to the Host1x driver. >>> This is required for enabling system-wide DVFS and supporting dynamic >>> power management using a generic power domain. >>> >>> Tested-by: Peter Geis <pgwipeout@gmail.com> >>> Tested-by: Nicolas Chauvet <kwizart@gmail.com> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> --- >>> drivers/gpu/host1x/dev.c | 102 ++++++++++++++++++++++++++++++++++----- >>> 1 file changed, 91 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c >>> index d0ebb70e2fdd..c1525cffe7b1 100644 >>> --- a/drivers/gpu/host1x/dev.c >>> +++ b/drivers/gpu/host1x/dev.c >>> @@ -12,6 +12,7 @@ >>> #include <linux/module.h> >>> #include <linux/of_device.h> >>> #include <linux/of.h> >>> +#include <linux/pm_runtime.h> >>> #include <linux/slab.h> >>> #define CREATE_TRACE_POINTS >>> @@ -417,7 +418,7 @@ static int host1x_probe(struct platform_device *pdev) >>> return err; >>> } >>> - host->rst = devm_reset_control_get(&pdev->dev, "host1x"); >>> + host->rst = devm_reset_control_get_exclusive_released(&pdev->dev, >>> "host1x"); >>> if (IS_ERR(host->rst)) { >>> err = PTR_ERR(host->rst); >>> dev_err(&pdev->dev, "failed to get reset: %d\n", err); >>> @@ -437,16 +438,15 @@ 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; >>> - } >>> + pm_runtime_enable(&pdev->dev); >>> + err = pm_runtime_get_sync(&pdev->dev); >>> + if (err < 0) >>> + goto rpm_disable; >>> err = reset_control_deassert(host->rst); >>> if (err < 0) { >>> dev_err(&pdev->dev, "failed to deassert reset: %d\n", err); >>> - goto unprepare_disable; >>> + goto rpm_disable; >>> } >>> err = host1x_syncpt_init(host); >>> @@ -485,9 +485,10 @@ static int host1x_probe(struct platform_device >>> *pdev) >>> host1x_syncpt_deinit(host); >>> reset_assert: >>> reset_control_assert(host->rst); >>> -unprepare_disable: >>> - clk_disable_unprepare(host->clk); >>> -free_channels: >>> +rpm_disable: >>> + pm_runtime_put(&pdev->dev); >>> + pm_runtime_disable(&pdev->dev); >>> + >>> host1x_channel_list_free(&host->channel_list); >>> iommu_exit: >>> host1x_iommu_exit(host); >>> @@ -504,16 +505,95 @@ static int host1x_remove(struct platform_device >>> *pdev) >>> host1x_intr_deinit(host); >>> host1x_syncpt_deinit(host); >>> reset_control_assert(host->rst); >>> - clk_disable_unprepare(host->clk); >>> + pm_runtime_put(&pdev->dev); >>> + pm_runtime_disable(&pdev->dev); >>> host1x_iommu_exit(host); >>> return 0; >>> } >>> +static int __maybe_unused host1x_runtime_suspend(struct device *dev) >>> +{ >>> + struct host1x *host = dev_get_drvdata(dev); >>> + >>> + clk_disable_unprepare(host->clk); >>> + reset_control_release(host->rst); >>> + >>> + return 0; >>> +} >>> + >>> +static int __maybe_unused host1x_runtime_resume(struct device *dev) >>> +{ >>> + struct host1x *host = dev_get_drvdata(dev); >>> + int err; >>> + >>> + err = reset_control_acquire(host->rst); >>> + if (err) { >>> + dev_err(dev, "failed to acquire reset: %d\n", err); >>> + return err; >>> + } >>> + >>> + err = clk_prepare_enable(host->clk); >>> + if (err) { >>> + dev_err(dev, "failed to enable clock: %d\n", err); >>> + goto release_reset; >>> + } >>> + >>> + return 0; >>> + >>> +release_reset: >>> + reset_control_release(host->rst); >>> + >>> + return err; >>> +} >>> + >>> +static __maybe_unused int host1x_suspend(struct device *dev) >>> +{ >>> + struct host1x *host = dev_get_drvdata(dev); >>> + int err; >>> + >>> + host1x_syncpt_save(host); >>> + >>> + err = pm_runtime_force_suspend(dev); >>> + if (err < 0) >>> + return err; >>> + >>> + return 0; >>> +} >>> + >>> +static __maybe_unused int host1x_resume(struct device *dev) >>> +{ >>> + struct host1x *host = dev_get_drvdata(dev); >>> + struct host1x_channel *channel; >>> + unsigned int index; >>> + int err; >>> + >>> + err = pm_runtime_force_resume(dev); >>> + if (err < 0) >>> + return err; >>> + >>> + host1x_syncpt_restore(host); >> >> We also need to execute 'host1x_setup_sid_table' upon resume. > > Indeed, thanks. I'll correct it in the next revision. > > Perhaps the actual save/restore needs to be moved to the runtime > callbacks. At least I can't remember right now why this wasn't done in > the first place. > I looked at the save/restore once again and recalled why it's done so. The reason is that the host1x touches hardware during the driver probe, and thus, RPM needs to be resumed first. It will be a bigger change to properly decouple the hardware initialization so that it all could be put it into the RPM callback. I'll try to do it in v3.
On 17-12-20, 21:05, Dmitry Osipenko wrote: > Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs, which reduces > power consumption and heating of the Tegra chips. Tegra SoC has multiple > hardware units which belong to a core power domain of the SoC and share > the core voltage. The voltage must be selected in accordance to a minimum > requirement of every core hardware unit. Please submit the OPP changes separately (alone), it will never get merged this way. Send fixes at the top, any features you want later in the series. It is fine for you to base your series of patches which are under review, you just need to mention that in your cover letter for your platform's patchset. -- viresh
18.12.2020 10:14, Viresh Kumar пишет: > On 17-12-20, 21:05, Dmitry Osipenko wrote: >> Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs, which reduces >> power consumption and heating of the Tegra chips. Tegra SoC has multiple >> hardware units which belong to a core power domain of the SoC and share >> the core voltage. The voltage must be selected in accordance to a minimum >> requirement of every core hardware unit. > > Please submit the OPP changes separately (alone), it will never get > merged this way. Send fixes at the top, any features you want later in > the series. It is fine for you to base your series of patches which > are under review, you just need to mention that in your cover letter > for your platform's patchset. > Alright, although I haven't pretended that v2 patches should be merged right away since they are fundamentally different from v1, and thus, all patches need to be reviewed first. If the current OPP changes look good to you, then please give yours r-b to the patches. Thanks in advance!
On 18-12-20, 16:51, Dmitry Osipenko wrote: > Alright, although I haven't pretended that v2 patches should be merged > right away since they are fundamentally different from v1, and thus, all > patches need to be reviewed first. I agree. I have done some basic review for the stuff. > If the current OPP changes look good to you, then please give yours r-b > to the patches. Thanks in advance! r-b-y isn't required as they will go through my tree itself. So if everyone is happy with the idea, please submit the patches separately (fixes, improvements, devm_*, etc).
22.12.2020 12:15, Viresh Kumar пишет: > On 18-12-20, 16:51, Dmitry Osipenko wrote: >> Alright, although I haven't pretended that v2 patches should be merged >> right away since they are fundamentally different from v1, and thus, all >> patches need to be reviewed first. > > I agree. I have done some basic review for the stuff. Thank you for the review. >> If the current OPP changes look good to you, then please give yours r-b >> to the patches. Thanks in advance! > > r-b-y isn't required as they will go through my tree itself. So if everyone is > happy with the idea, please submit the patches separately (fixes, improvements, > devm_*, etc). okay
On Thu, Dec 17, 2020 at 09:05:50PM +0300, Dmitry Osipenko wrote: > Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs, which reduces > power consumption and heating of the Tegra chips. Tegra SoC has multiple > hardware units which belong to a core power domain of the SoC and share > the core voltage. The voltage must be selected in accordance to a minimum > requirement of every core hardware unit. > > The minimum core voltage requirement depends on: > > 1. Clock enable state of a hardware unit. > 2. Clock frequency. > 3. Unit's internal idling/active state. > > This series is tested on Acer A500 (T20), AC100 (T20), Nexus 7 (T30), > Ouya (T30), TK1 (T124) and some others. I also added voltage scaling to > the Ventana (T20) and Cardhu (T30) boards which are tested by NVIDIA's CI > farm. Tegra30 is now couple degrees cooler on Nexus 7 and stays cool on > Ouya (instead of becoming burning hot) while system is idling. It should > be possible to improve this further by implementing a more advanced power > management features for the kernel drivers. > > The DVFS support is opt-in for all boards, meaning that older DTBs will > continue to work like they did it before this series. It should be possible > to easily add the core voltage scaling support for Tegra114+ SoCs based on > this grounding work later on, if anyone will want to implement it. The same comment as for your interconnect work: for sets touching multiple systems please mention the dependencies between patches in the cover letter. Not as a reply to such remark like I make here, but as a separate entry in the cover letter. Best regards, Krzysztof
05.01.2021 20:11, Krzysztof Kozlowski пишет: > On Thu, Dec 17, 2020 at 09:05:50PM +0300, Dmitry Osipenko wrote: >> Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs, which reduces >> power consumption and heating of the Tegra chips. Tegra SoC has multiple >> hardware units which belong to a core power domain of the SoC and share >> the core voltage. The voltage must be selected in accordance to a minimum >> requirement of every core hardware unit. >> >> The minimum core voltage requirement depends on: >> >> 1. Clock enable state of a hardware unit. >> 2. Clock frequency. >> 3. Unit's internal idling/active state. >> >> This series is tested on Acer A500 (T20), AC100 (T20), Nexus 7 (T30), >> Ouya (T30), TK1 (T124) and some others. I also added voltage scaling to >> the Ventana (T20) and Cardhu (T30) boards which are tested by NVIDIA's CI >> farm. Tegra30 is now couple degrees cooler on Nexus 7 and stays cool on >> Ouya (instead of becoming burning hot) while system is idling. It should >> be possible to improve this further by implementing a more advanced power >> management features for the kernel drivers. >> >> The DVFS support is opt-in for all boards, meaning that older DTBs will >> continue to work like they did it before this series. It should be possible >> to easily add the core voltage scaling support for Tegra114+ SoCs based on >> this grounding work later on, if anyone will want to implement it. > > The same comment as for your interconnect work: for sets touching > multiple systems please mention the dependencies between patches in the > cover letter. Not as a reply to such remark like I make here, but as a > separate entry in the cover letter. I'll describe all the dependencies in the next revision, thanks.