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

[Leo Yan <leo.yan@arm.com>]

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