[PATCH v5 04/10] coresight: Appropriately disable programming clocks

[Suzuki K Poulose]

On 30/07/2025 09:56, Leo Yan wrote:
> 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);

That may not work, as they may need the ATCLK enabled to
push the trace over ATB. They may skip the APB, as there
is no programming interface.

Suzuki




>        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