On Tue, Jul 29, 2025 at 01:30:28PM +0100, Suzuki Kuruppassery Poulose wrote:
On 29/07/2025 12:31, Mark Brown wrote:
On Mon, Jul 28, 2025 at 05:45:04PM +0100, Mark Brown wrote:
On Thu, Jul 24, 2025 at 04:22:34PM +0100, Leo Yan wrote:
Previously we would return NULL for any error (which isn't super great for deferred probe but never mind).
- pclk = devm_clk_get_enabled(dev, "apb_pclk");
- if (IS_ERR(pclk))
pclk = devm_clk_get_enabled(dev, "apb");...
return pclk; }
Now we pass errors back to the caller, making missing clocks fatal.
Thinking about this some more I think for compatiblity these clocks need to be treated as optional - that's what the original code was effectively doing, and I can imagine this isn't the only SoC which has (hopefully) always on clocks and didn't wire things up in DT.
You're right. The static components (funnels, replicators) don't have APB programming interface and hence no clocks. That said, may be the "is amba device" check could be used to enforce the presence of a clock.
I was wondering how this issue slipped through when I tested it on the Hikey960 board. The Hikey960 also has one static funnel, but it binds pclk with the static funnel node. That's why I didn't detect the issue.
I don't think using optional clock API is right thing, as DT binding schema claims the pclk is mandatory for dynamic components. My proposal is to enable the clocks only when IORESOURCE_MEM is available, something like:
if (res) { ret = coresight_get_enable_clocks(dev, &drvdata->pclk, &drvdata->atclk); if (ret) return ret;
base = devm_ioremap_resource(dev, res); ... }
The static components don't bind I/O resources, it is naturally not to enable clocks for them. Please let me know if this is reasonable solution.
@Mark, thanks a lot for testing and bisection.
Leo