On Tue, Jun 24, 2025 at 11:14:17AM +0530, Anshuman Khandual wrote:
[...]
CoreSight drivers are refined so that clocks are initialized in one go. As a result, driver data no longer needs to be allocated separately in the static and dynamic probes. Moved the allocation into a low-level function to avoid code duplication.
But why should this change be included here in this patch that consolidates pclk and atclk clock initialization ? Should this be done in a separate patch instead ?
Good point. It indeed we can divide into two smaller patches, one is for clock consolidations, and another patch is for refining driver data allocation.
I will do this in next spin.
+/*
- 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)Moving this helper function here does make sense.
+{
- WARN_ON(!pclk);
That is because pclk will be populated in all possible scenarios including the one assigned as NULL - hence it needs to have been allocated.
- if (dev_is_amba(dev)) {
/* Don't enable pclk for an AMBA device */*pclk = NULL;- } else {
/** "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);- }
- if (atclk) {
But an allocated atclk indicates need for atclk clock init instead.
To be clear, we strictly follow up DT binding doc, atclk clock is always optional. For a atclk pointer:
- If atclk is NULL: the driver does not require atclk at all. - If atclk is not NULL: the driver requires atclk optional.
Probably a 'which all clocks' flag based approach might been better ? But I guess this proposal will create less code churn.
So far, CoreSight driver has a fixed requirement for clocks.
- APB clock (pclk): it is mandatory. It can be either controlled by AMBA bus driver or CoreSight driver.
- atclk: It is not required (CTCU) or it is optional.
Given the pattern is fixed, I don't think an extra flag would be helpful at here. But I understand the code is not very strightforward, I will add comments for easier understanding.
*atclk = devm_clk_get_optional_enabled(dev, "atclk");if (IS_ERR(*atclk))return PTR_ERR(*atclk);- }
atclk when requested - either will have a valid clock or an error pointer but never a NULL pointer unlike the pclk clock ?
As mentioned, atclk can be a NULL pointer - it is optional and may be absent in the device tree binding. This is why we use the optional variant of the clock API to initialize it. If it returns a NULL pointer, it is tolerated by IS_ERR(*atclk), and this is considered a successful case.
Thanks, Leo