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 will let Leo sort this out
Suzuki
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
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
On Wed, Jul 30, 2025 at 10:27:48AM +0100, Suzuki Kuruppassery Poulose wrote:
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.
If so, I will use an extra patch to skip pclk enabling for static funnel and replicator, as a result, patch 04 will be:
if (res) { drvdata->pclk = coresight_get_enable_apb_pclk(dev); if (IS_ERR(drvdata->pclk)) return PTR_ERR(drvdata->pclk); }
Then, when consolidation in patch 07, it will have a code:
/* Only enable pclk for a device with I/O resource */ ret = coresight_get_enable_clocks(dev, res ? &drvdata->pclk : NULL, &drvdata->atclk);
This turns out to be the case for both static funnel and replicator devices — regardless of whether the DT binding includes "apb_pclk" or not, the driver will always skip enabling it. Any concerns?
Thanks, Leo
On 30/07/2025 11:54, Leo Yan wrote:
On Wed, Jul 30, 2025 at 10:27:48AM +0100, Suzuki Kuruppassery Poulose wrote:
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.
If so, I will use an extra patch to skip pclk enabling for static funnel and replicator, as a result, patch 04 will be:
if (res) { drvdata->pclk = coresight_get_enable_apb_pclk(dev); if (IS_ERR(drvdata->pclk)) return PTR_ERR(drvdata->pclk); }
Then, when consolidation in patch 07, it will have a code:
/* Only enable pclk for a device with I/O resource */ ret = coresight_get_enable_clocks(dev, res ? &drvdata->pclk : NULL, &drvdata->atclk);
Can we not differentiate the error code in devm_get..() for
ENOENT (not found) vs Some other failure ?
I would recommend using that and don't force the use of apb_clk/apb for AMBA devices. If the firmware doesn't specify a clock, but does specify the CoreSight components, it knows it better.
Suzuki
This turns out to be the case for both static funnel and replicator devices — regardless of whether the DT binding includes "apb_pclk" or not, the driver will always skip enabling it. Any concerns?
Thanks, Leo
-
Leo Yan -
Suzuki K Poulose