Re: [PATCH v5 5/6] rockchip/soc/drivers: Add DTPM description for rk3399

[Ulf Hansson]

Hi Daniel, Rob,

On Sat, 18 Dec 2021 at 14:00, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
>
> The DTPM framework does support now the hierarchy description.
>
> The platform specific code can call the hierarchy creation function
> with an array of struct dtpm_node pointing to their parent.
>
> This patch provides a description of the big and Little CPUs and the
> GPU and tie them together under a virtual package name. Only rk3399 is
> described now.
>
> The description could be extended in the future with the memory
> controller with devfreq if it has the energy information.
>
> The hierarchy uses the GPU devfreq with the panfrost driver, and this
> one could be loaded as a module. If the hierarchy is created before
> the panfrost driver is loaded, it will fail. For this reason the
> Kconfig option depends on the panfrost Kconfig's option. If this one
> is compiled as a module, automatically the dtpm hierarchy code will be
> a module also. Module loading ordering will fix this chicken-egg
> problem.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> ---
>  drivers/soc/rockchip/Kconfig  |  8 +++++
>  drivers/soc/rockchip/Makefile |  1 +
>  drivers/soc/rockchip/dtpm.c   | 56 +++++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+)
>  create mode 100644 drivers/soc/rockchip/dtpm.c
>
> diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
> index 25eb2c1e31bb..a88fe6d3064a 100644
> --- a/drivers/soc/rockchip/Kconfig
> +++ b/drivers/soc/rockchip/Kconfig
> @@ -34,4 +34,12 @@ config ROCKCHIP_PM_DOMAINS
>
>            If unsure, say N.
>
> +config ROCKCHIP_DTPM
> +       tristate "Rockchip DTPM hierarchy"
> +       depends on DTPM && DRM_PANFROST
> +       help
> +        Describe the hierarchy for the Dynamic Thermal Power
> +        Management tree on this platform. That will create all the
> +        power capping capable devices.
> +
>  endif
> diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
> index 875032f7344e..05f31a4e743c 100644
> --- a/drivers/soc/rockchip/Makefile
> +++ b/drivers/soc/rockchip/Makefile
> @@ -5,3 +5,4 @@
>  obj-$(CONFIG_ROCKCHIP_GRF) += grf.o
>  obj-$(CONFIG_ROCKCHIP_IODOMAIN) += io-domain.o
>  obj-$(CONFIG_ROCKCHIP_PM_DOMAINS) += pm_domains.o
> +obj-$(CONFIG_ROCKCHIP_DTPM) += dtpm.o
> diff --git a/drivers/soc/rockchip/dtpm.c b/drivers/soc/rockchip/dtpm.c
> new file mode 100644
> index 000000000000..77edc565c110
> --- /dev/null
> +++ b/drivers/soc/rockchip/dtpm.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2021 Linaro Limited
> + *
> + * Author: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> + *
> + * DTPM hierarchy description
> + */
> +#include <linux/dtpm.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +static struct dtpm_node __initdata rk3399_hierarchy[] = {
> +       [0]{ .name = "rk3399" },
> +       [1]{ .name = "package",
> +            .parent = &rk3399_hierarchy[0] },
> +       [2]{ .name = "/cpus/cpu@0",
> +            .type = DTPM_NODE_DT,
> +            .parent = &rk3399_hierarchy[1] },
> +       [3]{ .name = "/cpus/cpu@1",
> +            .type = DTPM_NODE_DT,
> +            .parent = &rk3399_hierarchy[1] },
> +       [4]{ .name = "/cpus/cpu@2",
> +            .type = DTPM_NODE_DT,
> +            .parent = &rk3399_hierarchy[1] },
> +       [5]{ .name = "/cpus/cpu@3",
> +            .type = DTPM_NODE_DT,
> +            .parent = &rk3399_hierarchy[1] },
> +       [6]{ .name = "/cpus/cpu@100",
> +            .type = DTPM_NODE_DT,
> +            .parent = &rk3399_hierarchy[1] },
> +       [7]{ .name = "/cpus/cpu@101",
> +            .type = DTPM_NODE_DT,
> +            .parent = &rk3399_hierarchy[1] },
> +       [8]{ .name = "rockchip,rk3399-mali",
> +            .type = DTPM_NODE_DT,
> +            .parent = &rk3399_hierarchy[1] },
> +       [9]{ },
> +};

I will not object to this, as in the end this seems like what we need
to do, unless we can describe things through generic DT bindings for
DTPM. Right?

Although, if the above is correct, I need to stress that I am kind of
worried that this doesn't really scale. We would need to copy lots of
information from the DTS files into platform specific c-files, to be
able to describe the DTPM hierarchy.

> +
> +static struct of_device_id __initdata rockchip_dtpm_match_table[] = {
> +        { .compatible = "rockchip,rk3399", .data = rk3399_hierarchy },
> +        {},
> +};
> +
> +static int __init rockchip_dtpm_init(void)
> +{
> +       return dtpm_create_hierarchy(rockchip_dtpm_match_table);
> +}
> +late_initcall(rockchip_dtpm_init);
> +
> +MODULE_DESCRIPTION("Rockchip DTPM driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:dtpm");
> +MODULE_AUTHOR("Daniel Lezcano <daniel.lezcano@xxxxxxxxxx");
> --
> 2.25.1
>

Kind regards
Uffe