diff mbox series

[v2,28/48] soc/tegra: Introduce core power domain driver

Message ID 20201217180638.22748-29-digetx@gmail.com
State New
Headers show
Series [v2,01/48] dt-bindings: memory: tegra20: emc: Replace core regulator with power domain | expand

Commit Message

Dmitry Osipenko Dec. 17, 2020, 6:06 p.m. UTC
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

Comments

Viresh Kumar Dec. 22, 2020, 6:40 a.m. UTC | #1
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.
Dmitry Osipenko Dec. 22, 2020, 7:21 p.m. UTC | #2
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.
Dmitry Osipenko Dec. 22, 2020, 7:39 p.m. UTC | #3
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.
Viresh Kumar Dec. 23, 2020, 5:57 a.m. UTC | #4
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.
Dmitry Osipenko Dec. 23, 2020, 8:37 p.m. UTC | #5
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
Dmitry Osipenko Dec. 23, 2020, 8:59 p.m. UTC | #6
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.
Viresh Kumar Dec. 24, 2020, 6:51 a.m. UTC | #7
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.
Dmitry Osipenko Dec. 24, 2020, 12:14 p.m. UTC | #8
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.
Ulf Hansson Jan. 12, 2021, 1:57 p.m. UTC | #9
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 mbox series

Patch

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)