On Sat, Jun 14, 2025 at 8:09 PM Michal Wilczynski <[email protected]> wrote: > > Introduce the pwrseq-thead-gpu driver, a power sequencer provider for > the Imagination BXM-4-64 GPU on the T-HEAD TH1520 SoC. This driver is > an auxiliary driver instantiated by the AON power domain driver.
Just a technicality: this driver controls an auxiliary *device* instantiated by the AON power domain driver. > > The TH1520 GPU requires a specific sequence to correctly initialize and > power down its resources: > - Enable GPU clocks (core and sys). > - De-assert the GPU clock generator reset (clkgen_reset). > - Introduce a short hardware-required delay. > - De-assert the GPU core reset. The power-down sequence performs these > steps in reverse. > > Implement this sequence via the pwrseq_power_on and pwrseq_power_off > callbacks. > > Crucially, the driver's match function is called when a consumer (the > Imagination GPU driver) requests the "gpu-power" target. During this > match, the sequencer uses devm_clk_bulk_get() and > devm_reset_control_get_exclusive() on the consumer's device to obtain > handles to the GPU's "core" and "sys" clocks, and the GPU core reset. > These, along with clkgen_reset obtained from parent aon node, allow it > to perform the complete sequence. > > Reviewed-by: Ulf Hansson <[email protected]> > Signed-off-by: Michal Wilczynski <[email protected]> > --- [snip] > + > +static int pwrseq_thead_gpu_power_on(struct pwrseq_device *pwrseq) Please follow the naming convention of the callbacks: this should be pwrseq_thead_gpu_enable(). [snip] > + > +static int pwrseq_thead_gpu_power_off(struct pwrseq_device *pwrseq) Same here. [snip] > +static int pwrseq_thead_gpu_match(struct pwrseq_device *pwrseq, > + struct device *dev) > +{ > + struct pwrseq_thead_gpu_ctx *ctx = pwrseq_device_get_drvdata(pwrseq); > + static const char *const clk_names[] = { "core", "sys" }; > + struct of_phandle_args pwr_spec; > + int i, ret; > + > + /* We only match the specific T-HEAD TH1520 GPU compatible */ > + if (!of_device_is_compatible(dev->of_node, "thead,th1520-gpu")) > + return 0; > + > + ret = of_parse_phandle_with_args(dev->of_node, "power-domains", > + "#power-domain-cells", 0, &pwr_spec); > + if (ret) > + return 0; > + > + /* Additionally verify consumer device has AON as power-domain */ > + if (pwr_spec.np != ctx->aon_node || pwr_spec.args[0] != > TH1520_GPU_PD) { > + of_node_put(pwr_spec.np); > + return 0; > + } > + > + of_node_put(pwr_spec.np); > + > + /* Prevent multiple consumers from attaching */ > + if (ctx->gpu_reset || ctx->clks) > + return -EBUSY; Isn't it the whole point of pwrseq - to allow multiple consumers to seamlessly attach to the provider and control the underlying resources in a safe way? I think you should just not request the relevant resources for the second time (really only applies to the exclusive reset and even then it's not clear why it needs to be exclusive) but still return 1 for a valid consumer and let pwrseq handle the refcount? Also: can this even happen at all? > + > + ctx->num_clks = ARRAY_SIZE(clk_names); > + ctx->clks = devm_kcalloc(dev, ctx->num_clks, sizeof(*ctx->clks), > + GFP_KERNEL); > + if (!ctx->clks) > + return -ENOMEM; > + > + for (i = 0; i < ctx->num_clks; i++) > + ctx->clks[i].id = clk_names[i]; > + > + ret = devm_clk_bulk_get(dev, ctx->num_clks, ctx->clks); This is interesting. I admit I had not considered the pwrseq provider being able to acquire the resources from the consumer node at the time of writing the subsystem. As the pwrseq framework aims at being as flexible as possible, this is definitely something that we should allow but the usage of devres here is problematic on at least two levels. First: you're acquiring the resources from the struct device of the consumer and so the devres entries are added to its devres list. They will get released when the consumer device is detached and the pwrseq provider may end up accessing them afterwards. Second: if .match() fails or even returns 0, the resource is still acquired. Call .match() enough times and you have the devres list needlessly clobbered with unused resources. You should stick to non-devres variants and make sure they are all cleaned-up unless returning 1. (Note to self: these shouldn't be magic values really). You can then release them in this driver's remove callback. > + if (ret) > + return ret; > + > + ctx->gpu_reset = devm_reset_control_get_exclusive(dev, NULL); > + if (IS_ERR(ctx->gpu_reset)) > + return PTR_ERR(ctx->gpu_reset); > + > + return 1; > +} > + > +static int pwrseq_thead_gpu_probe(struct auxiliary_device *adev, > + const struct auxiliary_device_id *id) > +{ > + struct device *dev = &adev->dev; > + struct device *parent_dev = dev->parent; > + struct pwrseq_thead_gpu_ctx *ctx; > + struct pwrseq_config config = {}; > + > + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); > + if (!ctx) > + return -ENOMEM; > + > + ctx->aon_node = parent_dev->of_node; > + > + ctx->clkgen_reset = > + devm_reset_control_get_exclusive(parent_dev, "gpu-clkgen"); > + if (IS_ERR(ctx->clkgen_reset)) > + return dev_err_probe( > + dev, PTR_ERR(ctx->clkgen_reset), > + "Failed to get GPU clkgen reset from parent\n"); > + > + config.parent = dev; > + config.owner = THIS_MODULE; > + config.drvdata = ctx; > + config.match = pwrseq_thead_gpu_match; > + config.targets = pwrseq_thead_gpu_targets; > + > + ctx->pwrseq = devm_pwrseq_device_register(dev, &config); > + if (IS_ERR(ctx->pwrseq)) > + return dev_err_probe(dev, PTR_ERR(ctx->pwrseq), > + "Failed to register power sequencer\n"); > + > + return 0; > +} > + > +static const struct auxiliary_device_id pwrseq_thead_gpu_id_table[] = { > + { .name = "th1520_pm_domains.pwrseq-gpu" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(auxiliary, pwrseq_thead_gpu_id_table); > + > +static struct auxiliary_driver pwrseq_thead_gpu_driver = { > + .driver = { > + .name = "pwrseq-thead-gpu", > + }, > + .probe = pwrseq_thead_gpu_probe, > + .id_table = pwrseq_thead_gpu_id_table, > +}; > +module_auxiliary_driver(pwrseq_thead_gpu_driver); > + > +MODULE_AUTHOR("Michal Wilczynski <[email protected]>"); > +MODULE_DESCRIPTION("T-HEAD TH1520 GPU power sequencer driver"); > +MODULE_LICENSE("GPL"); > > -- > 2.34.1 > Thanks! Bartosz
