| Message ID | 20201217180638.22748-29-digetx@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v2,01/48] dt-bindings: memory: tegra20: emc: Replace core regulator with power domain | expand |
On 17-12-20, 21:06, Dmitry Osipenko wrote: > +++ b/drivers/soc/tegra/core-power-domain.c > @@ -0,0 +1,125 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * NVIDIA Tegra SoC Core Power Domain Driver > + */ > + > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pm_domain.h> > +#include <linux/pm_opp.h> > +#include <linux/slab.h> > + > +#include <soc/tegra/common.h> > + > +static struct lock_class_key tegra_core_domain_lock_class; > +static bool tegra_core_domain_state_synced; > + > +static int tegra_genpd_set_performance_state(struct generic_pm_domain *genpd, > + unsigned int level) > +{ > + struct dev_pm_opp *opp; > + int err; > + > + opp = dev_pm_opp_find_level_ceil(&genpd->dev, &level); We don't need ceil or floor versions for level, but rather _exact() version. Or maybe just call it dev_pm_opp_find_level(). > + if (IS_ERR(opp)) { > + dev_err(&genpd->dev, "failed to find OPP for level %u: %pe\n", > + level, opp); > + return PTR_ERR(opp); > + } > + > + err = dev_pm_opp_set_voltage(&genpd->dev, opp); IIUC, you implemented this callback because you want to use the voltage triplet present in the OPP table ? And so you are setting the regulator ("power") later in this patch ? I am not in favor of implementing this routine, as it just adds a wrapper above the regulator API. What you should be doing rather is get the regulator by yourself here (instead of depending on the OPP core). And then you can do dev_pm_opp_get_voltage() here and set the voltage yourself. You may want to implement a version supporting triplet here though for the same. And you won't require the sync version of the API as well then.
22.12.2020 09:40, Viresh Kumar пишет: > On 17-12-20, 21:06, Dmitry Osipenko wrote: >> +++ b/drivers/soc/tegra/core-power-domain.c >> @@ -0,0 +1,125 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * NVIDIA Tegra SoC Core Power Domain Driver >> + */ >> + >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm_domain.h> >> +#include <linux/pm_opp.h> >> +#include <linux/slab.h> >> + >> +#include <soc/tegra/common.h> >> + >> +static struct lock_class_key tegra_core_domain_lock_class; >> +static bool tegra_core_domain_state_synced; >> + >> +static int tegra_genpd_set_performance_state(struct generic_pm_domain *genpd, >> + unsigned int level) >> +{ >> + struct dev_pm_opp *opp; >> + int err; >> + >> + opp = dev_pm_opp_find_level_ceil(&genpd->dev, &level); > > We don't need ceil or floor versions for level, but rather _exact() version. Or > maybe just call it dev_pm_opp_find_level(). The _exact() version won't find OPP for level=0 if levels don't start with 0. >> + if (IS_ERR(opp)) { >> + dev_err(&genpd->dev, "failed to find OPP for level %u: %pe\n", >> + level, opp); >> + return PTR_ERR(opp); >> + } >> + >> + err = dev_pm_opp_set_voltage(&genpd->dev, opp); > > IIUC, you implemented this callback because you want to use the voltage triplet > present in the OPP table ? > > And so you are setting the regulator ("power") later in this patch ? yes > I am not in favor of implementing this routine, as it just adds a wrapper above > the regulator API. What you should be doing rather is get the regulator by > yourself here (instead of depending on the OPP core). And then you can do > dev_pm_opp_get_voltage() here and set the voltage yourself. You may want to > implement a version supporting triplet here though for the same. > > And you won't require the sync version of the API as well then. > That's what I initially did for this driver. I don't mind to revert back to the initial variant in v3, it appeared to me that it will be nicer and cleaner to have OPP API managing everything here.
22.12.2020 22:21, Dmitry Osipenko пишет: >>> + if (IS_ERR(opp)) { >>> + dev_err(&genpd->dev, "failed to find OPP for level %u: %pe\n", >>> + level, opp); >>> + return PTR_ERR(opp); >>> + } >>> + >>> + err = dev_pm_opp_set_voltage(&genpd->dev, opp); >> IIUC, you implemented this callback because you want to use the voltage triplet >> present in the OPP table ? >> >> And so you are setting the regulator ("power") later in this patch ? > yes > >> I am not in favor of implementing this routine, as it just adds a wrapper above >> the regulator API. What you should be doing rather is get the regulator by >> yourself here (instead of depending on the OPP core). And then you can do >> dev_pm_opp_get_voltage() here and set the voltage yourself. You may want to >> implement a version supporting triplet here though for the same. >> >> And you won't require the sync version of the API as well then. >> > That's what I initially did for this driver. I don't mind to revert back > to the initial variant in v3, it appeared to me that it will be nicer > and cleaner to have OPP API managing everything here. I forgot one important detail (why the initial variant wasn't good).. OPP entries that have unsupportable voltages should be filtered out and OPP core performs the filtering only if regulator is assigned to the OPP table. If regulator is assigned to the OPP table, then we need to use OPP API for driving the regulator, hence that's why I added dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage(). Perhaps it should be possible to add dev_pm_opp_get_regulator() that will return the OPP table regulator in order to allow driver to use the regulator directly. But I'm not sure whether this is a much better option than the opp_sync_regulators() and opp_set_voltage() APIs.
On 22-12-20, 22:39, Dmitry Osipenko wrote: > 22.12.2020 22:21, Dmitry Osipenko пишет: > >>> + if (IS_ERR(opp)) { > >>> + dev_err(&genpd->dev, "failed to find OPP for level %u: %pe\n", > >>> + level, opp); > >>> + return PTR_ERR(opp); > >>> + } > >>> + > >>> + err = dev_pm_opp_set_voltage(&genpd->dev, opp); > >> IIUC, you implemented this callback because you want to use the voltage triplet > >> present in the OPP table ? > >> > >> And so you are setting the regulator ("power") later in this patch ? > > yes > > > >> I am not in favor of implementing this routine, as it just adds a wrapper above > >> the regulator API. What you should be doing rather is get the regulator by > >> yourself here (instead of depending on the OPP core). And then you can do > >> dev_pm_opp_get_voltage() here and set the voltage yourself. You may want to > >> implement a version supporting triplet here though for the same. > >> > >> And you won't require the sync version of the API as well then. > >> > > That's what I initially did for this driver. I don't mind to revert back > > to the initial variant in v3, it appeared to me that it will be nicer > > and cleaner to have OPP API managing everything here. > > I forgot one important detail (why the initial variant wasn't good).. > OPP entries that have unsupportable voltages should be filtered out and > OPP core performs the filtering only if regulator is assigned to the OPP > table. > > If regulator is assigned to the OPP table, then we need to use OPP API > for driving the regulator, hence that's why I added > dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage(). > > Perhaps it should be possible to add dev_pm_opp_get_regulator() that What's wrong with getting the regulator in the driver as well ? Apart from the OPP core ? > will return the OPP table regulator in order to allow driver to use the > regulator directly. But I'm not sure whether this is a much better > option than the opp_sync_regulators() and opp_set_voltage() APIs. set_voltage() is still fine as there is some data that the OPP core has, but sync_regulator() has nothing to do with OPP core. And this may lead to more wrapper helpers in the OPP core, which I am afraid of. And so even if it is not the best, I would like the OPP core to provide the data and not get into this. Ofcourse there is an exception to this, opp_set_rate.
23.12.2020 08:57, Viresh Kumar пишет: > On 22-12-20, 22:39, Dmitry Osipenko wrote: >> 22.12.2020 22:21, Dmitry Osipenko пишет: >>>>> + if (IS_ERR(opp)) { >>>>> + dev_err(&genpd->dev, "failed to find OPP for level %u: %pe\n", >>>>> + level, opp); >>>>> + return PTR_ERR(opp); >>>>> + } >>>>> + >>>>> + err = dev_pm_opp_set_voltage(&genpd->dev, opp); >>>> IIUC, you implemented this callback because you want to use the voltage triplet >>>> present in the OPP table ? >>>> >>>> And so you are setting the regulator ("power") later in this patch ? >>> yes >>> >>>> I am not in favor of implementing this routine, as it just adds a wrapper above >>>> the regulator API. What you should be doing rather is get the regulator by >>>> yourself here (instead of depending on the OPP core). And then you can do >>>> dev_pm_opp_get_voltage() here and set the voltage yourself. You may want to >>>> implement a version supporting triplet here though for the same. >>>> >>>> And you won't require the sync version of the API as well then. >>>> >>> That's what I initially did for this driver. I don't mind to revert back >>> to the initial variant in v3, it appeared to me that it will be nicer >>> and cleaner to have OPP API managing everything here. >> >> I forgot one important detail (why the initial variant wasn't good).. >> OPP entries that have unsupportable voltages should be filtered out and >> OPP core performs the filtering only if regulator is assigned to the OPP >> table. >> >> If regulator is assigned to the OPP table, then we need to use OPP API >> for driving the regulator, hence that's why I added >> dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage(). >> >> Perhaps it should be possible to add dev_pm_opp_get_regulator() that > > What's wrong with getting the regulator in the driver as well ? Apart from the > OPP core ? The voltage syncing should be done for each consumer regulator individually [1]. Secondly, regulator core doesn't work well today if the same regulator is requested more than one time for the same device. >> will return the OPP table regulator in order to allow driver to use the >> regulator directly. But I'm not sure whether this is a much better >> option than the opp_sync_regulators() and opp_set_voltage() APIs. > > set_voltage() is still fine as there is some data that the OPP core has, but > sync_regulator() has nothing to do with OPP core. > > And this may lead to more wrapper helpers in the OPP core, which I am afraid of. > And so even if it is not the best, I would like the OPP core to provide the data > and not get into this. Ofcourse there is an exception to this, opp_set_rate. > The regulator_sync_voltage() should be invoked only if voltage was changed previously [1]. The OPP core already has the info about whether voltage was changed and it provides the necessary locking for both set_voltage() and sync_regulator(). Perhaps I'll need to duplicate that functionality in the PD driver, instead of making it all generic and re-usable by other drivers. [1] https://elixir.bootlin.com/linux/v5.10.2/source/drivers/regulator/core.c#L4107
23.12.2020 23:37, Dmitry Osipenko пишет: > 23.12.2020 08:57, Viresh Kumar пишет: >> On 22-12-20, 22:39, Dmitry Osipenko wrote: >>> 22.12.2020 22:21, Dmitry Osipenko пишет: >>>>>> + if (IS_ERR(opp)) { >>>>>> + dev_err(&genpd->dev, "failed to find OPP for level %u: %pe\n", >>>>>> + level, opp); >>>>>> + return PTR_ERR(opp); >>>>>> + } >>>>>> + >>>>>> + err = dev_pm_opp_set_voltage(&genpd->dev, opp); >>>>> IIUC, you implemented this callback because you want to use the voltage triplet >>>>> present in the OPP table ? >>>>> >>>>> And so you are setting the regulator ("power") later in this patch ? >>>> yes >>>> >>>>> I am not in favor of implementing this routine, as it just adds a wrapper above >>>>> the regulator API. What you should be doing rather is get the regulator by >>>>> yourself here (instead of depending on the OPP core). And then you can do >>>>> dev_pm_opp_get_voltage() here and set the voltage yourself. You may want to >>>>> implement a version supporting triplet here though for the same. >>>>> >>>>> And you won't require the sync version of the API as well then. >>>>> >>>> That's what I initially did for this driver. I don't mind to revert back >>>> to the initial variant in v3, it appeared to me that it will be nicer >>>> and cleaner to have OPP API managing everything here. >>> >>> I forgot one important detail (why the initial variant wasn't good).. >>> OPP entries that have unsupportable voltages should be filtered out and >>> OPP core performs the filtering only if regulator is assigned to the OPP >>> table. >>> >>> If regulator is assigned to the OPP table, then we need to use OPP API >>> for driving the regulator, hence that's why I added >>> dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage(). >>> >>> Perhaps it should be possible to add dev_pm_opp_get_regulator() that >> >> What's wrong with getting the regulator in the driver as well ? Apart from the >> OPP core ? > > The voltage syncing should be done for each consumer regulator > individually [1]. > > Secondly, regulator core doesn't work well today if the same regulator > is requested more than one time for the same device. > >>> will return the OPP table regulator in order to allow driver to use the >>> regulator directly. But I'm not sure whether this is a much better >>> option than the opp_sync_regulators() and opp_set_voltage() APIs. >> >> set_voltage() is still fine as there is some data that the OPP core has, but >> sync_regulator() has nothing to do with OPP core. >> >> And this may lead to more wrapper helpers in the OPP core, which I am afraid of. >> And so even if it is not the best, I would like the OPP core to provide the data >> and not get into this. Ofcourse there is an exception to this, opp_set_rate. >> > > The regulator_sync_voltage() should be invoked only if voltage was > changed previously [1]. > > The OPP core already has the info about whether voltage was changed and > it provides the necessary locking for both set_voltage() and > sync_regulator(). Perhaps I'll need to duplicate that functionality in > the PD driver, instead of making it all generic and re-usable by other > drivers. > > [1] > https://elixir.bootlin.com/linux/v5.10.2/source/drivers/regulator/core.c#L4107 > I just realized that the locking is missing in the v2 patches, something to fix in the next revision :) Still I'm in favor of extending the OPP API with the new common functions. But if you have a strong opinion about that, then I'll try to work around it in the PD driver in v3.
On 23-12-20, 23:37, Dmitry Osipenko wrote: > 23.12.2020 08:57, Viresh Kumar пишет: > > What's wrong with getting the regulator in the driver as well ? Apart from the > > OPP core ? > > The voltage syncing should be done for each consumer regulator > individually [1]. > > Secondly, regulator core doesn't work well today if the same regulator > is requested more than one time for the same device. Hmm... > >> will return the OPP table regulator in order to allow driver to use the > >> regulator directly. But I'm not sure whether this is a much better > >> option than the opp_sync_regulators() and opp_set_voltage() APIs. > > > > set_voltage() is still fine as there is some data that the OPP core has, but > > sync_regulator() has nothing to do with OPP core. > > > > And this may lead to more wrapper helpers in the OPP core, which I am afraid of. > > And so even if it is not the best, I would like the OPP core to provide the data > > and not get into this. Ofcourse there is an exception to this, opp_set_rate. > > > > The regulator_sync_voltage() should be invoked only if voltage was > changed previously [1]. > > The OPP core already has the info about whether voltage was changed and > it provides the necessary locking for both set_voltage() and > sync_regulator(). Perhaps I'll need to duplicate that functionality in > the PD driver, instead of making it all generic and re-usable by other > drivers. > > [1] > https://elixir.bootlin.com/linux/v5.10.2/source/drivers/regulator/core.c#L4107 Lets do it in the OPP core and see where we go.
24.12.2020 09:51, Viresh Kumar пишет: > On 23-12-20, 23:37, Dmitry Osipenko wrote: >> 23.12.2020 08:57, Viresh Kumar пишет: >>> What's wrong with getting the regulator in the driver as well ? Apart from the >>> OPP core ? >> >> The voltage syncing should be done for each consumer regulator >> individually [1]. >> >> Secondly, regulator core doesn't work well today if the same regulator >> is requested more than one time for the same device. > > Hmm... > >>>> will return the OPP table regulator in order to allow driver to use the >>>> regulator directly. But I'm not sure whether this is a much better >>>> option than the opp_sync_regulators() and opp_set_voltage() APIs. >>> >>> set_voltage() is still fine as there is some data that the OPP core has, but >>> sync_regulator() has nothing to do with OPP core. >>> >>> And this may lead to more wrapper helpers in the OPP core, which I am afraid of. >>> And so even if it is not the best, I would like the OPP core to provide the data >>> and not get into this. Ofcourse there is an exception to this, opp_set_rate. >>> >> >> The regulator_sync_voltage() should be invoked only if voltage was >> changed previously [1]. >> >> The OPP core already has the info about whether voltage was changed and >> it provides the necessary locking for both set_voltage() and >> sync_regulator(). Perhaps I'll need to duplicate that functionality in >> the PD driver, instead of making it all generic and re-usable by other >> drivers. >> >> [1] >> https://elixir.bootlin.com/linux/v5.10.2/source/drivers/regulator/core.c#L4107 > > Lets do it in the OPP core and see where we go. > Alright, thank you.
On Thu, 17 Dec 2020 at 19:07, Dmitry Osipenko <digetx@gmail.com> wrote: > > NVIDIA Tegra SoCs have multiple power domains, each domain corresponds > to an external SoC power rail. Core power domain covers vast majority of > hardware blocks within a Tegra SoC. The voltage of a power domain should > be set to a value which satisfies all devices within a power domain. Add > driver for the core power domain in order to manage the voltage state of > the domain. This allows us to support a system-wide DVFS on Tegra. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> FYI: from a genpd provider driver point of view, this looks good to me. However, withholding my ack for the next version, just to make sure I take a second look. Kind regards Uffe > --- > drivers/soc/tegra/Kconfig | 6 ++ > drivers/soc/tegra/Makefile | 1 + > drivers/soc/tegra/core-power-domain.c | 125 ++++++++++++++++++++++++++ > include/soc/tegra/common.h | 6 ++ > 4 files changed, 138 insertions(+) > create mode 100644 drivers/soc/tegra/core-power-domain.c > > diff --git a/drivers/soc/tegra/Kconfig b/drivers/soc/tegra/Kconfig > index bcd61ae59ba3..fccbc168dd87 100644 > --- a/drivers/soc/tegra/Kconfig > +++ b/drivers/soc/tegra/Kconfig > @@ -16,6 +16,7 @@ config ARCH_TEGRA_2x_SOC > select SOC_TEGRA_COMMON > select SOC_TEGRA_FLOWCTRL > select SOC_TEGRA_PMC > + select SOC_TEGRA_CORE_POWER_DOMAIN > select SOC_TEGRA20_VOLTAGE_COUPLER > select TEGRA_TIMER > help > @@ -31,6 +32,7 @@ config ARCH_TEGRA_3x_SOC > select SOC_TEGRA_COMMON > select SOC_TEGRA_FLOWCTRL > select SOC_TEGRA_PMC > + select SOC_TEGRA_CORE_POWER_DOMAIN > select SOC_TEGRA30_VOLTAGE_COUPLER > select TEGRA_TIMER > help > @@ -170,3 +172,7 @@ config SOC_TEGRA20_VOLTAGE_COUPLER > config SOC_TEGRA30_VOLTAGE_COUPLER > bool "Voltage scaling support for Tegra30 SoCs" > depends on ARCH_TEGRA_3x_SOC || COMPILE_TEST > + > +config SOC_TEGRA_CORE_POWER_DOMAIN > + bool > + select PM_GENERIC_DOMAINS > diff --git a/drivers/soc/tegra/Makefile b/drivers/soc/tegra/Makefile > index 9c809c1814bd..8f1294f954b4 100644 > --- a/drivers/soc/tegra/Makefile > +++ b/drivers/soc/tegra/Makefile > @@ -7,3 +7,4 @@ obj-$(CONFIG_SOC_TEGRA_PMC) += pmc.o > obj-$(CONFIG_SOC_TEGRA_POWERGATE_BPMP) += powergate-bpmp.o > obj-$(CONFIG_SOC_TEGRA20_VOLTAGE_COUPLER) += regulators-tegra20.o > obj-$(CONFIG_SOC_TEGRA30_VOLTAGE_COUPLER) += regulators-tegra30.o > +obj-$(CONFIG_SOC_TEGRA_CORE_POWER_DOMAIN) += core-power-domain.o > diff --git a/drivers/soc/tegra/core-power-domain.c b/drivers/soc/tegra/core-power-domain.c > new file mode 100644 > index 000000000000..7c0cec8c79fd > --- /dev/null > +++ b/drivers/soc/tegra/core-power-domain.c > @@ -0,0 +1,125 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * NVIDIA Tegra SoC Core Power Domain Driver > + */ > + > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pm_domain.h> > +#include <linux/pm_opp.h> > +#include <linux/slab.h> > + > +#include <soc/tegra/common.h> > + > +static struct lock_class_key tegra_core_domain_lock_class; > +static bool tegra_core_domain_state_synced; > + > +static int tegra_genpd_set_performance_state(struct generic_pm_domain *genpd, > + unsigned int level) > +{ > + struct dev_pm_opp *opp; > + int err; > + > + opp = dev_pm_opp_find_level_ceil(&genpd->dev, &level); > + if (IS_ERR(opp)) { > + dev_err(&genpd->dev, "failed to find OPP for level %u: %pe\n", > + level, opp); > + return PTR_ERR(opp); > + } > + > + err = dev_pm_opp_set_voltage(&genpd->dev, opp); > + dev_pm_opp_put(opp); > + > + if (err) { > + dev_err(&genpd->dev, "failed to set voltage to %duV: %d\n", > + level, err); > + return err; > + } > + > + return 0; > +} > + > +static unsigned int > +tegra_genpd_opp_to_performance_state(struct generic_pm_domain *genpd, > + struct dev_pm_opp *opp) > +{ > + return dev_pm_opp_get_level(opp); > +} > + > +static int tegra_core_domain_probe(struct platform_device *pdev) > +{ > + struct generic_pm_domain *genpd; > + struct opp_table *opp_table; > + const char *rname = "power"; > + int err; > + > + genpd = devm_kzalloc(&pdev->dev, sizeof(*genpd), GFP_KERNEL); > + if (!genpd) > + return -ENOMEM; > + > + genpd->name = pdev->dev.of_node->name; > + genpd->set_performance_state = tegra_genpd_set_performance_state; > + genpd->opp_to_performance_state = tegra_genpd_opp_to_performance_state; > + > + opp_table = devm_pm_opp_set_regulators(&pdev->dev, &rname, 1); > + if (IS_ERR(opp_table)) > + return dev_err_probe(&pdev->dev, PTR_ERR(opp_table), > + "failed to set OPP regulator\n"); > + > + err = pm_genpd_init(genpd, NULL, false); > + if (err) { > + dev_err(&pdev->dev, "failed to init genpd: %d\n", err); > + return err; > + } > + > + /* > + * We have a "PMC -> Core" hierarchy of the power domains where > + * PMC needs to resume and change performance (voltage) of the > + * Core domain from the PMC GENPD on/off callbacks, hence we need > + * to annotate the lock in order to remove confusion from the > + * lockdep checker when a nested access happens. > + */ > + lockdep_set_class(&genpd->mlock, &tegra_core_domain_lock_class); > + > + err = of_genpd_add_provider_simple(pdev->dev.of_node, genpd); > + if (err) { > + dev_err(&pdev->dev, "failed to add genpd: %d\n", err); > + goto remove_genpd; > + } > + > + return 0; > + > +remove_genpd: > + pm_genpd_remove(genpd); > + > + return err; > +} > + > +bool tegra_soc_core_domain_state_synced(void) > +{ > + return tegra_core_domain_state_synced; > +} > + > +static void tegra_core_domain_sync_state(struct device *dev) > +{ > + tegra_core_domain_state_synced = true; > + > + dev_pm_opp_sync_regulators(dev); > +} > + > +static const struct of_device_id tegra_core_domain_match[] = { > + { .compatible = "nvidia,tegra20-core-domain", }, > + { .compatible = "nvidia,tegra30-core-domain", }, > + { } > +}; > + > +static struct platform_driver tegra_core_domain_driver = { > + .driver = { > + .name = "tegra-core-power", > + .of_match_table = tegra_core_domain_match, > + .suppress_bind_attrs = true, > + .sync_state = tegra_core_domain_sync_state, > + }, > + .probe = tegra_core_domain_probe, > +}; > +builtin_platform_driver(tegra_core_domain_driver); > diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h > index 57b56793a9e5..6c2ccbbbf073 100644 > --- a/include/soc/tegra/common.h > +++ b/include/soc/tegra/common.h > @@ -27,6 +27,7 @@ struct tegra_core_opp_params { > > #ifdef CONFIG_ARCH_TEGRA > bool soc_is_tegra(void); > +bool tegra_soc_core_domain_state_synced(void); > int devm_tegra_core_dev_init_opp_table(struct device *dev, > struct tegra_core_opp_params *cfg); > #else > @@ -35,6 +36,11 @@ static inline bool soc_is_tegra(void) > return false; > } > > +static inline bool tegra_soc_core_domain_state_synced(void) > +{ > + return false; > +} > + > static inline int > devm_tegra_core_dev_init_opp_table(struct device *dev, > struct tegra_core_opp_params *cfg) > -- > 2.29.2 >
diff --git a/drivers/soc/tegra/Kconfig b/drivers/soc/tegra/Kconfig index bcd61ae59ba3..fccbc168dd87 100644 --- a/drivers/soc/tegra/Kconfig +++ b/drivers/soc/tegra/Kconfig @@ -16,6 +16,7 @@ config ARCH_TEGRA_2x_SOC select SOC_TEGRA_COMMON select SOC_TEGRA_FLOWCTRL select SOC_TEGRA_PMC + select SOC_TEGRA_CORE_POWER_DOMAIN select SOC_TEGRA20_VOLTAGE_COUPLER select TEGRA_TIMER help @@ -31,6 +32,7 @@ config ARCH_TEGRA_3x_SOC select SOC_TEGRA_COMMON select SOC_TEGRA_FLOWCTRL select SOC_TEGRA_PMC + select SOC_TEGRA_CORE_POWER_DOMAIN select SOC_TEGRA30_VOLTAGE_COUPLER select TEGRA_TIMER help @@ -170,3 +172,7 @@ config SOC_TEGRA20_VOLTAGE_COUPLER config SOC_TEGRA30_VOLTAGE_COUPLER bool "Voltage scaling support for Tegra30 SoCs" depends on ARCH_TEGRA_3x_SOC || COMPILE_TEST + +config SOC_TEGRA_CORE_POWER_DOMAIN + bool + select PM_GENERIC_DOMAINS diff --git a/drivers/soc/tegra/Makefile b/drivers/soc/tegra/Makefile index 9c809c1814bd..8f1294f954b4 100644 --- a/drivers/soc/tegra/Makefile +++ b/drivers/soc/tegra/Makefile @@ -7,3 +7,4 @@ obj-$(CONFIG_SOC_TEGRA_PMC) += pmc.o obj-$(CONFIG_SOC_TEGRA_POWERGATE_BPMP) += powergate-bpmp.o obj-$(CONFIG_SOC_TEGRA20_VOLTAGE_COUPLER) += regulators-tegra20.o obj-$(CONFIG_SOC_TEGRA30_VOLTAGE_COUPLER) += regulators-tegra30.o +obj-$(CONFIG_SOC_TEGRA_CORE_POWER_DOMAIN) += core-power-domain.o diff --git a/drivers/soc/tegra/core-power-domain.c b/drivers/soc/tegra/core-power-domain.c new file mode 100644 index 000000000000..7c0cec8c79fd --- /dev/null +++ b/drivers/soc/tegra/core-power-domain.c @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * NVIDIA Tegra SoC Core Power Domain Driver + */ + +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/pm_domain.h> +#include <linux/pm_opp.h> +#include <linux/slab.h> + +#include <soc/tegra/common.h> + +static struct lock_class_key tegra_core_domain_lock_class; +static bool tegra_core_domain_state_synced; + +static int tegra_genpd_set_performance_state(struct generic_pm_domain *genpd, + unsigned int level) +{ + struct dev_pm_opp *opp; + int err; + + opp = dev_pm_opp_find_level_ceil(&genpd->dev, &level); + if (IS_ERR(opp)) { + dev_err(&genpd->dev, "failed to find OPP for level %u: %pe\n", + level, opp); + return PTR_ERR(opp); + } + + err = dev_pm_opp_set_voltage(&genpd->dev, opp); + dev_pm_opp_put(opp); + + if (err) { + dev_err(&genpd->dev, "failed to set voltage to %duV: %d\n", + level, err); + return err; + } + + return 0; +} + +static unsigned int +tegra_genpd_opp_to_performance_state(struct generic_pm_domain *genpd, + struct dev_pm_opp *opp) +{ + return dev_pm_opp_get_level(opp); +} + +static int tegra_core_domain_probe(struct platform_device *pdev) +{ + struct generic_pm_domain *genpd; + struct opp_table *opp_table; + const char *rname = "power"; + int err; + + genpd = devm_kzalloc(&pdev->dev, sizeof(*genpd), GFP_KERNEL); + if (!genpd) + return -ENOMEM; + + genpd->name = pdev->dev.of_node->name; + genpd->set_performance_state = tegra_genpd_set_performance_state; + genpd->opp_to_performance_state = tegra_genpd_opp_to_performance_state; + + opp_table = devm_pm_opp_set_regulators(&pdev->dev, &rname, 1); + if (IS_ERR(opp_table)) + return dev_err_probe(&pdev->dev, PTR_ERR(opp_table), + "failed to set OPP regulator\n"); + + err = pm_genpd_init(genpd, NULL, false); + if (err) { + dev_err(&pdev->dev, "failed to init genpd: %d\n", err); + return err; + } + + /* + * We have a "PMC -> Core" hierarchy of the power domains where + * PMC needs to resume and change performance (voltage) of the + * Core domain from the PMC GENPD on/off callbacks, hence we need + * to annotate the lock in order to remove confusion from the + * lockdep checker when a nested access happens. + */ + lockdep_set_class(&genpd->mlock, &tegra_core_domain_lock_class); + + err = of_genpd_add_provider_simple(pdev->dev.of_node, genpd); + if (err) { + dev_err(&pdev->dev, "failed to add genpd: %d\n", err); + goto remove_genpd; + } + + return 0; + +remove_genpd: + pm_genpd_remove(genpd); + + return err; +} + +bool tegra_soc_core_domain_state_synced(void) +{ + return tegra_core_domain_state_synced; +} + +static void tegra_core_domain_sync_state(struct device *dev) +{ + tegra_core_domain_state_synced = true; + + dev_pm_opp_sync_regulators(dev); +} + +static const struct of_device_id tegra_core_domain_match[] = { + { .compatible = "nvidia,tegra20-core-domain", }, + { .compatible = "nvidia,tegra30-core-domain", }, + { } +}; + +static struct platform_driver tegra_core_domain_driver = { + .driver = { + .name = "tegra-core-power", + .of_match_table = tegra_core_domain_match, + .suppress_bind_attrs = true, + .sync_state = tegra_core_domain_sync_state, + }, + .probe = tegra_core_domain_probe, +}; +builtin_platform_driver(tegra_core_domain_driver); diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h index 57b56793a9e5..6c2ccbbbf073 100644 --- a/include/soc/tegra/common.h +++ b/include/soc/tegra/common.h @@ -27,6 +27,7 @@ struct tegra_core_opp_params { #ifdef CONFIG_ARCH_TEGRA bool soc_is_tegra(void); +bool tegra_soc_core_domain_state_synced(void); int devm_tegra_core_dev_init_opp_table(struct device *dev, struct tegra_core_opp_params *cfg); #else @@ -35,6 +36,11 @@ static inline bool soc_is_tegra(void) return false; } +static inline bool tegra_soc_core_domain_state_synced(void) +{ + return false; +} + static inline int devm_tegra_core_dev_init_opp_table(struct device *dev, struct tegra_core_opp_params *cfg)
NVIDIA Tegra SoCs have multiple power domains, each domain corresponds to an external SoC power rail. Core power domain covers vast majority of hardware blocks within a Tegra SoC. The voltage of a power domain should be set to a value which satisfies all devices within a power domain. Add driver for the core power domain in order to manage the voltage state of the domain. This allows us to support a system-wide DVFS on Tegra. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/soc/tegra/Kconfig | 6 ++ drivers/soc/tegra/Makefile | 1 + drivers/soc/tegra/core-power-domain.c | 125 ++++++++++++++++++++++++++ include/soc/tegra/common.h | 6 ++ 4 files changed, 138 insertions(+) create mode 100644 drivers/soc/tegra/core-power-domain.c