Re: [PATCH v1 2/2] pmdomain: thead: Add GPU-specific clock and reset handling for TH1520

[Ulf Hansson]

On Wed, 9 Apr 2025 at 11:30, Michal Wilczynski <m.wilczynski@xxxxxxxxxxx> wrote:
>
> Extend the TH1520 power domain driver to manage GPU related clocks and
> resets via generic PM domain start/stop callbacks.
>
> The TH1520 GPU requires a special sequence to correctly initialize:
> - Enable the GPU clocks
> - Deassert the GPU clkgen reset
> - Delay for a few cycles to satisfy hardware requirements
> - Deassert the GPU core reset
>
> This sequence is SoC-specific and needs to be abstracted away from the
> Imagination GPU driver, which expects a standard single reset line.
> Following discussions with kernel maintainers, this logic is placed
> inside a PM domain instead of polluting the clock or reset frameworks,
> or the GPU driver itself [1].
>
> Managing this inside a generic power domain allows better coordination
> of clocks, resets, and power state, and aligns with the direction of
> treating PM domains as SoC-specific "power management drivers".
>
> [1] - https://lore.kernel.org/all/CAPDyKFqsJaTrF0tBSY-TjpqdVt5=6aPQHYfnDebtphfRZSU=-Q@xxxxxxxxxxxxxx/
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>
> ---
>  drivers/pmdomain/thead/th1520-pm-domains.c | 119 +++++++++++++++++++++
>  1 file changed, 119 insertions(+)
>
> diff --git a/drivers/pmdomain/thead/th1520-pm-domains.c b/drivers/pmdomain/thead/th1520-pm-domains.c
> index f702e20306f4..aa85c3954c39 100644
> --- a/drivers/pmdomain/thead/th1520-pm-domains.c
> +++ b/drivers/pmdomain/thead/th1520-pm-domains.c
> @@ -5,17 +5,29 @@
>   * Author: Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>
>   */
>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/firmware/thead/thead,th1520-aon.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_domain.h>
> +#include <linux/reset.h>
>
>  #include <dt-bindings/power/thead,th1520-power.h>
>
> +#define TH1520_GPU_RESET_IDX 0
> +#define TH1520_GPU_CLKGEN_RESET_IDX 1
> +
>  struct th1520_power_domain {
>         struct th1520_aon_chan *aon_chan;
>         struct generic_pm_domain genpd;
>         u32 rsrc;
> +
> +       struct clk_bulk_data *clks;
> +       int num_clks;
> +       struct reset_control_bulk_data *resets;
> +       int num_resets;
> +
>  };
>
>  struct th1520_power_info {
> @@ -61,6 +73,99 @@ static int th1520_pd_power_off(struct generic_pm_domain *domain)
>         return th1520_aon_power_update(pd->aon_chan, pd->rsrc, false);
>  }
>
> +static int th1520_gpu_init_clocks(struct device *dev,
> +                                 struct th1520_power_domain *pd)
> +{
> +       static const char *const clk_names[] = { "gpu-core", "gpu-sys" };
> +       int i, ret;
> +
> +       pd->num_clks = ARRAY_SIZE(clk_names);
> +       pd->clks = devm_kcalloc(dev, pd->num_clks, sizeof(*pd->clks), GFP_KERNEL);
> +       if (!pd->clks)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < pd->num_clks; i++)
> +               pd->clks[i].id = clk_names[i];
> +
> +       ret = devm_clk_bulk_get(dev, pd->num_clks, pd->clks);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to get GPU clocks\n");
> +
> +       return 0;
> +}
> +
> +static int th1520_gpu_init_resets(struct device *dev,
> +                                 struct th1520_power_domain *pd)
> +{
> +       static const char *const reset_names[] = { "gpu", "gpu-clkgen" };
> +       int i, ret;
> +
> +       pd->num_resets = ARRAY_SIZE(reset_names);
> +       pd->resets = devm_kcalloc(dev, pd->num_resets, sizeof(*pd->resets),
> +                                 GFP_KERNEL);
> +       if (!pd->resets)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < pd->num_resets; i++)
> +               pd->resets[i].id = reset_names[i];
> +
> +       ret = devm_reset_control_bulk_get_exclusive(dev, pd->num_resets,
> +                                                   pd->resets);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to get GPU resets\n");
> +
> +       return 0;
> +}
> +
> +static int th1520_gpu_domain_start(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +       struct th1520_power_domain *pd = to_th1520_power_domain(genpd);
> +       int ret;
> +
> +       ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
> +       if (ret)
> +               return ret;
> +
> +       ret = reset_control_deassert(pd->resets[TH1520_GPU_CLKGEN_RESET_IDX].rstc);
> +       if (ret)
> +               goto err_disable_clks;
> +
> +       /*
> +        * According to the hardware manual, a delay of at least 32 clock
> +        * cycles is required between de-asserting the clkgen reset and
> +        * de-asserting the GPU reset. Assuming a worst-case scenario with
> +        * a very high GPU clock frequency, a delay of 1 microsecond is
> +        * sufficient to ensure this requirement is met across all
> +        * feasible GPU clock speeds.
> +        */
> +       udelay(1);
> +
> +       ret = reset_control_deassert(pd->resets[TH1520_GPU_RESET_IDX].rstc);
> +       if (ret)
> +               goto err_assert_clkgen;
> +
> +       return 0;
> +
> +err_assert_clkgen:
> +       reset_control_assert(pd->resets[TH1520_GPU_CLKGEN_RESET_IDX].rstc);
> +err_disable_clks:
> +       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> +       return ret;
> +}
> +
> +static int th1520_gpu_domain_stop(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +       struct th1520_power_domain *pd = to_th1520_power_domain(genpd);
> +
> +       reset_control_assert(pd->resets[TH1520_GPU_RESET_IDX].rstc);
> +       reset_control_assert(pd->resets[TH1520_GPU_CLKGEN_RESET_IDX].rstc);
> +       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> +
> +       return 0;
> +}
> +
>  static struct generic_pm_domain *th1520_pd_xlate(const struct of_phandle_args *spec,
>                                                  void *data)
>  {
> @@ -99,6 +204,20 @@ th1520_add_pm_domain(struct device *dev, const struct th1520_power_info *pi)
>         pd->genpd.power_off = th1520_pd_power_off;
>         pd->genpd.name = pi->name;
>
> +       /* there are special callbacks for the GPU */
> +       if (pi == &th1520_pd_ranges[TH1520_GPU_PD]) {
> +               ret = th1520_gpu_init_clocks(dev, pd);

Assuming both the clocks and the reset are really resources that
belong to the GPU, I would suggest to use two callbacks for the genpd
in question to implement the get/put of the clocks and reset from
there.

pd->genpd->attach_dev
pd->genpd->detach_dev

> +               if (ret)
> +                       return ERR_PTR(ret);
> +
> +               ret = th1520_gpu_init_resets(dev, pd);
> +               if (ret)
> +                       return ERR_PTR(ret);
> +
> +               pd->genpd.dev_ops.start = th1520_gpu_domain_start;
> +               pd->genpd.dev_ops.stop = th1520_gpu_domain_stop;
> +       }
> +
>         ret = pm_genpd_init(&pd->genpd, NULL, true);
>         if (ret)
>                 return ERR_PTR(ret);
> --
> 2.34.1
>

Kind regards
Uffe