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@xxxxxxxxxxx>
> ---
> 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
