diff mbox series

[3/3] coresight etm4x: Add pr_debug statement for Coresight component PID/CID

Message ID dffc2af50362c1446d74c3574909d786b102369f.1674174972.git.scclevenger@os.amperecomputing.com (mailing list archive)
State New, archived
Headers show
Series Ampere Computing ETMv4.x Support | expand

Commit Message

Steve Clevenger Jan. 20, 2023, 12:51 a.m. UTC
Add pr_debug statement to provide visibility into Coresight component PID
and CID settings. This helped debug an intermittent clock related issue
resulting in bad PID/CID values.

Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
---
 drivers/amba/bus.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Suzuki K Poulose Jan. 20, 2023, 11:23 a.m. UTC | #1
Cc: Russell

nit: Subject line doesn't match the patch. This could be :

  "amba: bus: Add pr_debug for AMBA PID/CID"

On 20/01/2023 00:51, Steve Clevenger wrote:
> Add pr_debug statement to provide visibility into Coresight component PID
> and CID settings. This helped debug an intermittent clock related issue
> resulting in bad PID/CID values.

And this change belongs to the AMBA subsystem. Please run :

scripts/get_maintainer.pl on your patch and add the necessary people 
from that list for your patch.

As such, I don't think brings any value to be added to the tree.
I will leave it for the maintainers to comment.

Suzuki

> 
> Signed-off-by: Steve Clevenger <scclevenger@os.amperecomputing.com>
> ---
>   drivers/amba/bus.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index ff7454a38058..7c432442862c 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -136,6 +136,7 @@ static int amba_read_periphid(struct amba_device *dev)
>   	u32 size, pid, cid;
>   	void __iomem *tmp;
>   	int i, ret;
> +	u32 cid_addr, pid_addr;
>   
>   	ret = dev_pm_domain_attach(&dev->dev, true);
>   	if (ret) {
> @@ -178,6 +179,12 @@ static int amba_read_periphid(struct amba_device *dev)
>   	for (cid = 0, i = 0; i < 4; i++)
>   		cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
>   
> +	/* physical address as meaningful */
> +	cid_addr = (u64)(dev->res.start + size - 0x20);
> +	pid_addr = (u64)(dev->res.start + size - 0x10);
> +
> +	pr_debug("pid  (%llX): %08X  cid  (%llX): %08X\n", pid_addr, pid, cid_addr, cid);
> +
>   	if (cid == CORESIGHT_CID) {
>   		/* set the base to the start of the last 4k block */
>   		void __iomem *csbase = tmp + size - 4096;
Russell King (Oracle) Jan. 20, 2023, 12:40 p.m. UTC | #2
On Fri, Jan 20, 2023 at 11:23:53AM +0000, Suzuki K Poulose wrote:
> 
> Cc: Russell
> 
> nit: Subject line doesn't match the patch. This could be :
> 
>  "amba: bus: Add pr_debug for AMBA PID/CID"
> 
> On 20/01/2023 00:51, Steve Clevenger wrote:
> > Add pr_debug statement to provide visibility into Coresight component PID
> > and CID settings. This helped debug an intermittent clock related issue
> > resulting in bad PID/CID values.
> 
> And this change belongs to the AMBA subsystem. Please run :
> 
> scripts/get_maintainer.pl on your patch and add the necessary people from
> that list for your patch.
> 
> As such, I don't think brings any value to be added to the tree.
> I will leave it for the maintainers to comment.

Looking at the context in this patch, I see code that is reading at
least the CID but likely also the PID from the device, duplicating
the code that is already in the bus layer, and stored in the
amba_device's periphid and cid members...
Steve Clevenger Jan. 21, 2023, 7:31 a.m. UTC | #3
Hi Russell,

Comments inline.

Steve

On 1/20/2023 4:40 AM, Russell King (Oracle) wrote:
> On Fri, Jan 20, 2023 at 11:23:53AM +0000, Suzuki K Poulose wrote:
>>
>> Cc: Russell
>>
>> nit: Subject line doesn't match the patch. This could be :
>>
>>  "amba: bus: Add pr_debug for AMBA PID/CID"
>>
>> On 20/01/2023 00:51, Steve Clevenger wrote:
>>> Add pr_debug statement to provide visibility into Coresight component PID
>>> and CID settings. This helped debug an intermittent clock related issue
>>> resulting in bad PID/CID values.
>>
>> And this change belongs to the AMBA subsystem. Please run :
>>
>> scripts/get_maintainer.pl on your patch and add the necessary people from
>> that list for your patch.
>>
>> As such, I don't think brings any value to be added to the tree.
>> I will leave it for the maintainers to comment.
> 
> Looking at the context in this patch, I see code that is reading at
> least the CID but likely also the PID from the device, duplicating
> the code that is already in the bus layer, and stored in the
> amba_device's periphid and cid members...
> 
The idea here was to capture and report an intermittent Ampere PID/CID
read error at the first read. It's not going to disturb me if this patch
is not considered value added, but it served its' purpose in this case
so I thought it might be useful for others to turn on under debug. I can
resubmit the patch for AMBA, or not.
diff mbox series

Patch

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index ff7454a38058..7c432442862c 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -136,6 +136,7 @@  static int amba_read_periphid(struct amba_device *dev)
 	u32 size, pid, cid;
 	void __iomem *tmp;
 	int i, ret;
+	u32 cid_addr, pid_addr;
 
 	ret = dev_pm_domain_attach(&dev->dev, true);
 	if (ret) {
@@ -178,6 +179,12 @@  static int amba_read_periphid(struct amba_device *dev)
 	for (cid = 0, i = 0; i < 4; i++)
 		cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
 
+	/* physical address as meaningful */
+	cid_addr = (u64)(dev->res.start + size - 0x20);
+	pid_addr = (u64)(dev->res.start + size - 0x10);
+
+	pr_debug("pid  (%llX): %08X  cid  (%llX): %08X\n", pid_addr, pid, cid_addr, cid);
+
 	if (cid == CORESIGHT_CID) {
 		/* set the base to the start of the last 4k block */
 		void __iomem *csbase = tmp + size - 4096;