Re: [RFC PATCH v1 07/14] soc: thead: power-domain: Add skeleton power-domain driver for TH1520

[Krzysztof Kozlowski <krzk@kernel.org>]

On 03/12/2024 14:41, Michal Wilczynski wrote:
> The T-Head TH1520 SoC contains multiple power islands that can be
> programmatically turned on and off using the AON (Always-On) protocol
> and a hardware mailbox [1]. The relevant mailbox driver has already been
> merged into the mainline kernel in commit 5d4d263e1c6b ("mailbox:
> Introduce support for T-head TH1520 Mailbox driver"); however, the AON
> implementation is still under development.
> 
> This commit introduces a skeleton power-domain driver for the TH1520
> SoC, designed to be easily extended to work with the AON protocol in the
> future.  Currently, it only supports the GPU. Since there is no
> mechanism yet to turn the GPU power island on, the driver will only set
> the relevant registers to bring the GPU out of the reset state.  This
> should be done after the power-up sequence requested through the mailbox
> is completed.
> 
> Link: https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf [1]
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  MAINTAINERS                                   |   2 +
>  drivers/pmdomain/Kconfig                      |   1 +
>  drivers/pmdomain/Makefile                     |   1 +
>  drivers/pmdomain/thead/Kconfig                |  12 ++
>  drivers/pmdomain/thead/Makefile               |   2 +
>  drivers/pmdomain/thead/th1520-pm-domains.c    | 195 ++++++++++++++++++
>  .../dt-bindings/power/thead,th1520-power.h    |  19 ++
>  7 files changed, 232 insertions(+)


Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.


>  create mode 100644 drivers/pmdomain/thead/Kconfig
>  create mode 100644 drivers/pmdomain/thead/Makefile
>  create mode 100644 drivers/pmdomain/thead/th1520-pm-domains.c
>  create mode 100644 include/dt-bindings/power/thead,th1520-power.h
> 


...

> +
> +static int th1520_pd_power_off(struct generic_pm_domain *domain)
> +{
> +	struct th1520_power_domain *pd = to_th1520_power_domain(domain);
> +
> +	/* The missing component here is the call to E902 core through the

Use Linux coding style comments (see coding style). This applies to
multiple places in your code.

> +	 * AON protocol using hardware mailbox.
> +	 */
> +
> +	/* Put the GPU into reset state after powering it off */
> +	th1520_rst_gpu_disable(pd->reg);
> +
> +	return 0;
> +}
> +
> +static struct generic_pm_domain *th1520_pd_xlate(const struct of_phandle_args *spec,
> +						 void *data)
> +{
> +	struct generic_pm_domain *domain = ERR_PTR(-ENOENT);
> +	struct genpd_onecell_data *pd_data = data;
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(th1520_pd_ranges); i++) {
> +		struct th1520_power_domain *pd;
> +
> +		pd = to_th1520_power_domain(pd_data->domains[i]);
> +		if (pd->rsrc == spec->args[0]) {
> +			domain = &pd->genpd;
> +			break;
> +		}
> +	}
> +
> +	return domain;
> +}
> +
> +static struct th1520_power_domain *
> +th1520_add_pm_domain(struct device *dev, const struct th1520_power_info *pi)
> +{
> +	struct th1520_power_domain *pd;
> +	int ret;
> +
> +	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pd->rsrc = pi->rsrc;
> +	pd->genpd.power_on = th1520_pd_power_on;
> +	pd->genpd.power_off = th1520_pd_power_off;
> +	pd->genpd.name = pi->name;
> +
> +	ret = pm_genpd_init(&pd->genpd, NULL, true);
> +	if (ret) {
> +		devm_kfree(dev, pd);

You should rather fail the probe. Failures of power domains are important.

> +		return ERR_PTR(ret);
> +	}
> +
> +	return pd;
> +}
> +
> +static int th1520_pd_probe(struct platform_device *pdev)
> +{
> +	struct generic_pm_domain **domains;
> +	struct genpd_onecell_data *pd_data;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct regmap *reg;
> +	int i;
> +
> +	reg = syscon_regmap_lookup_by_phandle(np, "thead,vosys-regmap");
> +	if (IS_ERR(reg))
> +		return PTR_ERR(reg);
> +
> +	domains = devm_kcalloc(dev, ARRAY_SIZE(th1520_pd_ranges),
> +			       sizeof(*domains), GFP_KERNEL);
> +	if (!domains)
> +		return -ENOMEM;
> +
> +	pd_data = devm_kzalloc(dev, sizeof(*pd_data), GFP_KERNEL);
> +	if (!pd_data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < ARRAY_SIZE(th1520_pd_ranges); i++) {
> +		struct th1520_power_domain *pd;
> +
> +		pd = th1520_add_pm_domain(dev, &th1520_pd_ranges[i]);
> +		if (IS_ERR_OR_NULL(pd))
> +			continue;
> +
> +		pd->reg = reg;
> +		domains[i] = &pd->genpd;
> +		dev_dbg(dev, "added power domain %s\n", pd->genpd.name);
> +	}
> +
> +	pd_data->domains = domains;
> +	pd_data->num_domains = ARRAY_SIZE(th1520_pd_ranges);
> +	pd_data->xlate = th1520_pd_xlate;
> +
> +	return of_genpd_add_provider_onecell(dev->of_node, pd_data);
> +}
> +
> +static const struct of_device_id th1520_pd_match[] = {
> +	{ .compatible = "thead,th1520-pd",},
> +	{ /* sentinel */ }
> +};
> +


Make the driver tristate and module. There is nothing here which
prevents it being a module.


> +builtin_platform_driver(th1520_pd_driver);
> diff --git a/include/dt-bindings/power/thead,th1520-power.h b/include/dt-bindings/power/thead,th1520-power.h
> new file mode 100644
> index 000000000000..30fb4e9892e7
> --- /dev/null
> +++ b/include/dt-bindings/power/thead,th1520-power.h
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0+


Wrong license. See checkpatch.



Best regards,
Krzysztof