On Mon, May 05, 2025 at 12:58:06PM +0530, Anshuman Khandual wrote:
[...]
--- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -1645,6 +1645,51 @@ int coresight_etm_get_trace_id(struct coresight_device *csdev, enum cs_mode mode } EXPORT_SYMBOL_GPL(coresight_etm_get_trace_id); +/*
- Attempt to find and enable programming clock (pclk) and trace clock (atclk)
- for the given device.
- The AMBA bus driver will cover the pclk, to avoid duplicate operations,
- skip to get and enable the pclk for an AMBA device.
- atclk is an optional clock, it will be only enabled when it is existed.
- Otherwise, a NULL pointer will be returned to caller.
- Returns: '0' on Success; Error code otherwise.
- */
+int coresight_get_enable_clocks(struct device *dev, struct clk **pclk,
struct clk **atclk)These arguments probably could be arranged better as pclk and atclk are always contained inside 'xxx_drvdata' structure, which could be derived from the 'dev' structure itself, if [dev|platform]_set_drvdata() always ensured to be called earlier.
Seems to me, the conclusion "pclk and atclk ... could be derived from the 'dev' structure itself" is not true.
The reason is the coresight_get_enable_clocks() function is located in the CoreSight core layer, it has no knowledge for driver data definitions (see etmv4_drvdata, funnel_drvdata, etc). as a result, it cannot directly access the fields "drvdata->pclk" and "drvdata->atclk".
Currently there are only two instances where a NULL is being passed to indicate that 'atclk' clock is not to be probed or enabled. Could not individual clock requirements be passed in a flag argument instead ?
#define CORESIGHT_ENABLE_PCLK 0x1 #define CORESIGHT_ENABLE_ATCLK 0x2
coresight_get_enable_clocks(struct device *dev, unsigned long flags)
- atclk/pclk derived from drdvata which in turn from dev
- flags can be checked for pclk/atclk requirements
Even better - as atlck is the only optional clock here, it could just have a boolean flag argument to indicate for atclk clock.
+{
- WARN_ON(!pclk);
- if (!dev_is_amba(dev)) {
/** "apb_pclk" is the default clock name for an Arm Primecell* peripheral, while "apb" is used only by the CTCU driver.** For easier maintenance, CoreSight drivers should use* "apb_pclk" as the programming clock name.*/*pclk = devm_clk_get_enabled(dev, "apb_pclk");if (IS_ERR(*pclk))*pclk = devm_clk_get_enabled(dev, "apb");if (IS_ERR(*pclk))return PTR_ERR(*pclk);- } else {
/* Don't enable pclk for an AMBA device */*pclk = NULL;- }
Might be better to invert this conditional check as if (dev_is_amba(dev)) for better readability.
Will refine code for this.
Thanks, Leo