| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8a34b1ac-5681-4cd8-b960-a154d8678fa2@quicinc.com>
Date: Wed, 2 Apr 2025 08:55:51 +0800
From: Jie Gan <quic_jiegan@...cinc.com>
To: James Clark <james.clark@...aro.org>, Leo Yan <leo.yan@....com>
CC: Suzuki K Poulose <suzuki.poulose@....com>,
Mike Leach
<mike.leach@...aro.org>,
Anshuman Khandual <anshuman.khandual@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Maxime Coquelin
<mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
<coresight@...ts.linaro.org>, <linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>
Subject: Re: [PATCH v1 9/9] coresight: Consolidate clock enabling
On 4/1/2025 10:58 PM, James Clark wrote:
>
>
> On 27/03/2025 11:38 am, Leo Yan wrote:
>> CoreSight drivers enable pclk and atclk conditionally. For example,
>> pclk is only enabled in the static probe, while atclk is an optional
>> clock that it is enabled for both dynamic and static probes, if it is
>> present. In the current CoreSight drivers, these two clocks are
>> initialized separately. This causes complex and duplicate codes.
>>
>> This patch consolidates clock enabling into a central place. It renames
>> coresight_get_enable_apb_pclk() to coresight_get_enable_clocks() and
>> encapsulates clock initialization logic:
>>
>> - If a clock is initialized successfully, its clock pointer is assigned
>> to the double pointer passed as an argument.
>> - If pclk is skipped for an AMBA device, or if atclk is not found, the
>> corresponding double pointer is set to NULL. The function returns
>> Success (0) to guide callers can proceed with no error.
>> - Otherwise, an error number is returned for failures.
>>
>> 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.
>>
>> Suggested-by: Suzuki K Poulose <suzuki.poulose@....com>
>> Signed-off-by: Leo Yan <leo.yan@....com>
>> ---
>> drivers/hwtracing/coresight/coresight-catu.c | 30 +++++++++
>> +--------------------
>> drivers/hwtracing/coresight/coresight-cpu-debug.c | 29 ++++++++++
>> +------------------
>> drivers/hwtracing/coresight/coresight-ctcu-core.c | 8 ++++----
>> drivers/hwtracing/coresight/coresight-etm4x-core.c | 11 ++++-------
>> drivers/hwtracing/coresight/coresight-funnel.c | 11 ++++-------
>> drivers/hwtracing/coresight/coresight-replicator.c | 11 ++++-------
>> drivers/hwtracing/coresight/coresight-stm.c | 9 +++------
>> drivers/hwtracing/coresight/coresight-tmc-core.c | 30 +++++++++
>> +--------------------
>> drivers/hwtracing/coresight/coresight-tpiu.c | 10 ++++------
>> include/linux/coresight.h | 38 ++++++++++++
>> +++++++++++++++-----------
>> 10 files changed, 81 insertions(+), 106 deletions(-)
>>
> [...]
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> index 26eb4a61b992..cf3fbbc0076a 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -471,25 +471,41 @@ static inline bool is_coresight_device(void
>> __iomem *base)
>> }
>> /*
>> - * Attempt to find and enable "APB clock" for the given device
>> + * Attempt to find and enable programming clock (pclk) and trace
>> clock (atclk)
>> + * for the given device.
>> *
>> - * Returns:
>> + * The AMBA bus driver will cover the pclk, to avoid duplicate
>> operations,
>> + * skip to get and enable the pclk for an AMBA device.
>> *
>> - * clk - Clock is found and enabled
>> - * NULL - Clock is not needed as it is managed by the AMBA bus driver
>> - * ERROR - Clock is found but failed to enable
>> + * 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.
>> */
>> -static inline struct clk *coresight_get_enable_apb_pclk(struct device
>> *dev)
>> +static inline int coresight_get_enable_clocks(struct device *dev,
>> + struct clk **pclk,
>> + struct clk **atclk)
>
> This function has grown a bit now, probably best to remove it from the
> header and export it instead.
>
>> {
>> - struct clk *pclk = NULL;
>> + WARN_ON(!pclk);
>> if (!dev_is_amba(dev)) {
>> - pclk = devm_clk_get_enabled(dev, "apb_pclk");
>> - if (IS_ERR(pclk))
>> - pclk = devm_clk_get_enabled(dev, "apb");
>> + *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;
>
> Now the "apb" clock won't be enabled for amba devices. I'm assuming
> that's fine if the clock was always called "apb_pclk" for them, but the
> commit that added the new clock name didn't specify any special casing
> either.
>
> Can we have a comment that says it's deliberate? But the more I think
> about it the more I'm confused why CTCU needed a different clock name to
> be defined, when all the other Coresight devices use "apb_pclk".
Hi James,
The original clock-name for CTCU is apb_pclk, but the dt-binding
maintainer request me to change it to apb, that's why the clock name is
different from others.
I am not why we need apb instead of apb_pclk in dt-binding. Maybe some
rules have changed for dt-binding requirement.
Thanks,
Jie
>
>> }
>> - return pclk;
>> + if (atclk) {
>> + *atclk = devm_clk_get_optional_enabled(dev, "atclk");
>> + if (IS_ERR(*atclk))
>> + return PTR_ERR(*atclk);
>> + }
>> +
>> + return 0;
>> }
>> #define CORESIGHT_PIDRn(i) (0xFE0 + ((i) * 4))
>
Powered by blists - more mailing lists