[PATCH V2 00/30] OPP: Add new configuration interface: dev_pm_opp_set_config()

[PATCH V2 00/30] OPP: Add new configuration interface: dev_pm_opp_set_config()

[Viresh Kumar]

Hello,

We have too many configuration specific APIs currently, six of them already,
like dev_pm_opp_set_regulators(). This makes it complex/messy for both the OPP
core and its users to manage. There is also code redundancy in these APIs, in
the way they add/manage the OPP table specific stuff.

This patch series is an attempt to simplify these interfaces by adding a single
interface, dev_pm_opp_set_config(), which replaces all the existing ones. This
series also migrates the users to the new API.

The first two patches help get the API in place, followed by patches to migrate
the end users. Once all the users are migrated, the last few patches remove the
now unused interfaces.

This is pushed here:

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next

This is already tested by various folks now.

The entire patchset shall get merged via the OPP tree in 5.20-rc1, please do not
merge individual patches.

V1->V2:
- dev_pm_opp_set_config() doesn't return the OPP table anymore, but a token
  allocated with xa_alloc(). The same needs to be passed to clear-config API.
- Updated all users according to that as well.
- The clk_names interface is updated to allow multiple clocks.
- Converted few // comments to /* */.
- Added tags by few people.
- Dropped the last patch to rearrange stuff, not required anymore.

Thanks.

--
Viresh

Viresh Kumar (30):
  OPP: Track if clock name is configured by platform
  OPP: Add dev_pm_opp_set_config() and friends
  cpufreq: dt: Migrate to dev_pm_opp_set_config()
  cpufreq: imx: Migrate to dev_pm_opp_set_config()
  cpufreq: qcom-nvmem: Migrate to dev_pm_opp_set_config()
  cpufreq: sti: Migrate to dev_pm_opp_set_config()
  cpufreq: sun50i: Migrate to dev_pm_opp_set_config()
  cpufreq: tegra20: Migrate to dev_pm_opp_set_config()
  cpufreq: ti: Migrate to dev_pm_opp_set_config()
  devfreq: exynos: Migrate to dev_pm_opp_set_config()
  devfreq: sun8i: Migrate to dev_pm_opp_set_config()
  devfreq: tegra30: Migrate to dev_pm_opp_set_config()
  drm/lima: Migrate to dev_pm_opp_set_config()
  drm/msm: Migrate to dev_pm_opp_set_config()
  drm/panfrost: Migrate to dev_pm_opp_set_config()
  drm/tegra: Migrate to dev_pm_opp_set_config()
  media: venus: Migrate to dev_pm_opp_set_config()
  memory: tegra: Migrate to dev_pm_opp_set_config()
  mmc: sdhci-msm: Migrate to dev_pm_opp_set_config()
  OPP: ti: Migrate to dev_pm_opp_set_config()
  soc/tegra: Add comment over devm_pm_opp_set_clkname()
  soc/tegra: Migrate to dev_pm_opp_set_config()
  spi: qcom: Migrate to dev_pm_opp_set_config()
  serial: qcom: Migrate to dev_pm_opp_set_config()
  OPP: Remove dev_pm_opp_set_regulators() and friends
  OPP: Remove dev_pm_opp_set_supported_hw() and friends
  OPP: Remove dev_pm_opp_set_clkname() and friends
  OPP: Remove dev_pm_opp_register_set_opp_helper() and friends
  OPP: Remove dev_pm_opp_attach_genpd() and friends
  OPP: Remove dev_pm_opp_set_prop_name() and friends

 drivers/cpufreq/cpufreq-dt.c                  |  20 +-
 drivers/cpufreq/imx-cpufreq-dt.c              |  18 +-
 drivers/cpufreq/qcom-cpufreq-nvmem.c          | 109 +--
 drivers/cpufreq/sti-cpufreq.c                 |  27 +-
 drivers/cpufreq/sun50i-cpufreq-nvmem.c        |  36 +-
 drivers/cpufreq/tegra20-cpufreq.c             |  18 +-
 drivers/cpufreq/ti-cpufreq.c                  |  38 +-
 drivers/devfreq/exynos-bus.c                  |  25 +-
 drivers/devfreq/sun8i-a33-mbus.c              |   8 +-
 drivers/devfreq/tegra30-devfreq.c             |   8 +-
 drivers/gpu/drm/lima/lima_devfreq.c           |  12 +-
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c         |   8 +-
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c         |  10 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |   6 +-
 drivers/gpu/drm/msm/dp/dp_ctrl.c              |   6 +-
 drivers/gpu/drm/msm/dsi/dsi_host.c            |   6 +-
 drivers/gpu/drm/panfrost/panfrost_devfreq.c   |   9 +-
 drivers/gpu/drm/tegra/gr3d.c                  |   6 +-
 .../media/platform/qcom/venus/pm_helpers.c    |  18 +-
 drivers/memory/tegra/tegra124-emc.c           |  17 +-
 drivers/mmc/host/sdhci-msm.c                  |   6 +-
 drivers/opp/core.c                            | 632 ++++++++----------
 drivers/opp/opp.h                             |  23 +
 drivers/opp/ti-opp-supply.c                   |   8 +-
 drivers/soc/tegra/common.c                    |  45 +-
 drivers/soc/tegra/pmc.c                       |   8 +-
 drivers/spi/spi-geni-qcom.c                   |   6 +-
 drivers/spi/spi-qcom-qspi.c                   |   6 +-
 drivers/tty/serial/qcom_geni_serial.c         |   6 +-
 include/linux/pm_opp.h                        | 121 +---
 30 files changed, 605 insertions(+), 661 deletions(-)

-- 
2.31.1.272.g89b43f80a514

[PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

The OPP core now provides a unified API for setting all configuration
types, i.e. dev_pm_opp_set_config().

Lets start using it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 4733a233bd0c..ab667838d082 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1347,6 +1347,10 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 	int irq;
 	bool console = false;
 	struct uart_driver *drv;
+	struct dev_pm_opp_config config = {
+		.clk_names = (const char *[]){ "se" },
+		.clk_count = 1,
+	};
 
 	if (of_device_is_compatible(pdev->dev.of_node, "qcom,geni-debug-uart"))
 		console = true;
@@ -1430,7 +1434,7 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 	if (of_property_read_bool(pdev->dev.of_node, "cts-rts-swap"))
 		port->cts_rts_swap = true;
 
-	ret = devm_pm_opp_set_clkname(&pdev->dev, "se");
+	ret = devm_pm_opp_set_config(&pdev->dev, &config);
 	if (ret)
 		return ret;
 	/* OPP table is optional */
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Greg Kroah-Hartman]

On Fri, Jul 01, 2022 at 01:50:19PM +0530, Viresh Kumar wrote:
> The OPP core now provides a unified API for setting all configuration
> types, i.e. dev_pm_opp_set_config().
> 
> Lets start using it.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 4733a233bd0c..ab667838d082 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1347,6 +1347,10 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  	int irq;
>  	bool console = false;
>  	struct uart_driver *drv;
> +	struct dev_pm_opp_config config = {
> +		.clk_names = (const char *[]){ "se" },
> +		.clk_count = 1,
> +	};
>  
>  	if (of_device_is_compatible(pdev->dev.of_node, "qcom,geni-debug-uart"))
>  		console = true;
> @@ -1430,7 +1434,7 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  	if (of_property_read_bool(pdev->dev.of_node, "cts-rts-swap"))
>  		port->cts_rts_swap = true;
>  
> -	ret = devm_pm_opp_set_clkname(&pdev->dev, "se");
> +	ret = devm_pm_opp_set_config(&pdev->dev, &config);

This feels like a step back.  This is much harder now, what's wrong with
devm_pm_opp_set_clkname() as is?

thanks,

greg k-h

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

On 01-07-22, 10:44, Greg Kroah-Hartman wrote:
> On Fri, Jul 01, 2022 at 01:50:19PM +0530, Viresh Kumar wrote:
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > +	struct dev_pm_opp_config config = {
> > +		.clk_names = (const char *[]){ "se" },
> > +		.clk_count = 1,
> > +	};
> >  
> > -	ret = devm_pm_opp_set_clkname(&pdev->dev, "se");
> > +	ret = devm_pm_opp_set_config(&pdev->dev, &config);
> 
> This feels like a step back.  This is much harder now, what's wrong with
> devm_pm_opp_set_clkname() as is?

Hi Greg,

There are a number of configurations one can do for a device's OPP
table currently:

- clk, single or multiple (new)
- helper to configure multiple clocks (for multiple clocks)
- supplies or regulators
- helper to configure supplies (for multiple supplies)
- OPP supported-hw property
- OPP Property-name
- Genpd specific one
- etc

One problem was that it was a mess within the OPP core with a separate
interface for each of these interfaces, almost duplicate code, etc.

But then it was a bigger mess for the user drivers that need to manage
a few of these. They were required to call multiple APIs, with all the
interfaces returning tokens, which the callers need to save and supply
back to free the resources later. More code, hard to manage, easy to
abuse and add bugs to.

The new interface makes it easier and clean for everyone and allows
easy upgrades of interfaces in future. Adding a new interface, like
support for multiple clocks for a device that I just did, is much
easier now.

I really believe this is a step in the right direction :)

-- 
viresh

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Greg Kroah-Hartman]

On Fri, Jul 01, 2022 at 02:54:58PM +0530, Viresh Kumar wrote:
> On 01-07-22, 10:44, Greg Kroah-Hartman wrote:
> > On Fri, Jul 01, 2022 at 01:50:19PM +0530, Viresh Kumar wrote:
> > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > > +	struct dev_pm_opp_config config = {
> > > +		.clk_names = (const char *[]){ "se" },
> > > +		.clk_count = 1,
> > > +	};
> > >  
> > > -	ret = devm_pm_opp_set_clkname(&pdev->dev, "se");
> > > +	ret = devm_pm_opp_set_config(&pdev->dev, &config);
> > 
> > This feels like a step back.  This is much harder now, what's wrong with
> > devm_pm_opp_set_clkname() as is?
> 
> Hi Greg,
> 
> There are a number of configurations one can do for a device's OPP
> table currently:
> 
> - clk, single or multiple (new)
> - helper to configure multiple clocks (for multiple clocks)
> - supplies or regulators
> - helper to configure supplies (for multiple supplies)
> - OPP supported-hw property
> - OPP Property-name
> - Genpd specific one
> - etc
> 
> One problem was that it was a mess within the OPP core with a separate
> interface for each of these interfaces, almost duplicate code, etc.
> 
> But then it was a bigger mess for the user drivers that need to manage
> a few of these. They were required to call multiple APIs, with all the
> interfaces returning tokens, which the callers need to save and supply
> back to free the resources later. More code, hard to manage, easy to
> abuse and add bugs to.
> 
> The new interface makes it easier and clean for everyone and allows
> easy upgrades of interfaces in future. Adding a new interface, like
> support for multiple clocks for a device that I just did, is much
> easier now.
> 
> I really believe this is a step in the right direction :)

It's now more complex for simple drivers like this, right?  Why not
provide translations of the devm_pm_opp_set_clkname() to use internally
devm_pm_opp_set_config() if you want to do complex things, allowing you
to continue to do simple things without the overhead of a driver having
to create a structure on the stack and remember how the "const char *[]"
syntax looks like (seriously, that's crazy).

Make it simple for simple things, and provide the ability to do complex
things only if that is required.

thanks,

greg k-h

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

On 01-07-22, 11:39, Greg Kroah-Hartman wrote:
> It's now more complex for simple drivers like this, right?

They need to add a structure, yes.

> Why not
> provide translations of the devm_pm_opp_set_clkname() to use internally
> devm_pm_opp_set_config() if you want to do complex things,

That can be done, yes.

> allowing you
> to continue to do simple things without the overhead of a driver having
> to create a structure on the stack

I didn't think of it as complexity, and I still feel it is okay-ish.

> and remember how the "const char *[]"
> syntax looks like (seriously, that's crazy).

The syntax can be fixed, if we want, by avoiding the cast with
something like this:

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index a018b45c5a9a..1a5480214a43 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -2559,8 +2559,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
        const struct sdhci_msm_offset *msm_offset;
        const struct sdhci_msm_variant_info *var_info;
        struct device_node *node = pdev->dev.of_node;
+       const char *clks[] = { "core" };
        struct dev_pm_opp_config opp_config = {
-               .clk_names = (const char *[]){ "core" },
+               .clk_names = clks,
                .clk_count = 1,
        };

> Make it simple for simple things, and provide the ability to do complex
> things only if that is required.

I still feel it isn't too bad for simple cases right now too, it is
just a structure to fill out but I don't have hard feelings for
keeping the old API around. I just feel it isn't too helpful to keep
the old interfaces around, it will just confuse people at the best.

Anyway, I will keep them around.

-- 
viresh

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Greg Kroah-Hartman]

On Fri, Jul 01, 2022 at 03:31:00PM +0530, Viresh Kumar wrote:
> On 01-07-22, 11:39, Greg Kroah-Hartman wrote:
> > It's now more complex for simple drivers like this, right?
> 
> They need to add a structure, yes.
> 
> > Why not
> > provide translations of the devm_pm_opp_set_clkname() to use internally
> > devm_pm_opp_set_config() if you want to do complex things,
> 
> That can be done, yes.
> 
> > allowing you
> > to continue to do simple things without the overhead of a driver having
> > to create a structure on the stack
> 
> I didn't think of it as complexity, and I still feel it is okay-ish.
> 
> > and remember how the "const char *[]"
> > syntax looks like (seriously, that's crazy).
> 
> The syntax can be fixed, if we want, by avoiding the cast with
> something like this:
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index a018b45c5a9a..1a5480214a43 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -2559,8 +2559,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>         const struct sdhci_msm_offset *msm_offset;
>         const struct sdhci_msm_variant_info *var_info;
>         struct device_node *node = pdev->dev.of_node;
> +       const char *clks[] = { "core" };
>         struct dev_pm_opp_config opp_config = {
> -               .clk_names = (const char *[]){ "core" },
> +               .clk_names = clks,
>                 .clk_count = 1,
>         };

Still crazy, but a bit better.

Why do you need the clk_count?  A null terminated list is better, as the
compiler can do it for you and you do not have to keep things in sync
like you are expecting people to be forced to do now.

> > Make it simple for simple things, and provide the ability to do complex
> > things only if that is required.
> 
> I still feel it isn't too bad for simple cases right now too, it is
> just a structure to fill out but I don't have hard feelings for
> keeping the old API around. I just feel it isn't too helpful to keep
> the old interfaces around, it will just confuse people at the best.

The above is much more complex than a simple function call to make.
Remember to make it very simple for driver authors, and more
importantly, reviewers.

> Anyway, I will keep them around.

Thanks, and drop the count field please.

thanks,

greg k-h

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

On 01-07-22, 12:18, Greg Kroah-Hartman wrote:
> On Fri, Jul 01, 2022 at 03:31:00PM +0530, Viresh Kumar wrote:
> Still crazy, but a bit better.

:)

> Why do you need the clk_count?  A null terminated list is better,

Because I am not a big fan of the null terminated lists :)

I had to chase a bug once where someone removed that NULL at the end
and it was a nightmare to understand what's going on.

> as the
> compiler can do it for you and you do not have to keep things in sync
> like you are expecting people to be forced to do now.

I am not sure I understand what the compiler can do for us here.

The users will be required to do this here, isn't it ?

        const char *clks[] = { "core", NULL };
        struct dev_pm_opp_config opp_config = {
               .clk_names = clks,
        };


> The above is much more complex than a simple function call to make.
> Remember to make it very simple for driver authors, and more
> importantly, reviewers.

Hmm.

> Thanks, and drop the count field please.

There is one case at least [1] where we actually have to pass NULL in
the clk name. This is basically to allow the same code to run on
different devices, one where an OPP table is present and one where it
isn't. We don't want to do clk_set_rate() for the second case but just
use dev_pm_opp_set_rate() (which does a lot of stuff apart from just
clk).

-- 
viresh

[1] https://lore.kernel.org/lkml/b19a02422cae2408f953b92ae3c46a37fba688a3.1656660185.git.viresh.kumar@linaro.org/

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Greg Kroah-Hartman]

On Fri, Jul 01, 2022 at 03:59:26PM +0530, Viresh Kumar wrote:
> On 01-07-22, 12:18, Greg Kroah-Hartman wrote:
> > On Fri, Jul 01, 2022 at 03:31:00PM +0530, Viresh Kumar wrote:
> > Still crazy, but a bit better.
> 
> :)
> 
> > Why do you need the clk_count?  A null terminated list is better,
> 
> Because I am not a big fan of the null terminated lists :)
> 
> I had to chase a bug once where someone removed that NULL at the end
> and it was a nightmare to understand what's going on.

But that's the "normal" way the kernel does things.  Trying to keep a
count in sync with a list is a pain, and just gets harder and harder
over time.  Make it a null-terminated list so that the cpu makes this
always work and prevents errors.

> > as the
> > compiler can do it for you and you do not have to keep things in sync
> > like you are expecting people to be forced to do now.
> 
> I am not sure I understand what the compiler can do for us here.
> 
> The users will be required to do this here, isn't it ?
> 
>         const char *clks[] = { "core", NULL };
>         struct dev_pm_opp_config opp_config = {
>                .clk_names = clks,
>         };
> 

The "in sync" is the count issue.  Don't force humans to count up the
number of items in a list please.

> > The above is much more complex than a simple function call to make.
> > Remember to make it very simple for driver authors, and more
> > importantly, reviewers.
> 
> Hmm.
> 
> > Thanks, and drop the count field please.
> 
> There is one case at least [1] where we actually have to pass NULL in
> the clk name. This is basically to allow the same code to run on
> different devices, one where an OPP table is present and one where it
> isn't. We don't want to do clk_set_rate() for the second case but just
> use dev_pm_opp_set_rate() (which does a lot of stuff apart from just
> clk).

That feels completely wrong, don't have NULL for a name, make a fake name
or something.  Don't make all users in the kernel have a horrible
interface just for one piece of broken hardware out there.

Worst case, name it "".

thanks,

greg k-h

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

On 01-07-22, 12:41, Greg Kroah-Hartman wrote:
> That feels completely wrong, don't have NULL for a name, make a fake name
> or something.  Don't make all users in the kernel have a horrible
> interface just for one piece of broken hardware out there.
> 
> Worst case, name it "".

This name goes into the second argument of clk_get(dev, const char *con_id);

I will see how else I should hack it :)

-- 
viresh