| Message ID | dffc2af50362c1446d74c3574909d786b102369f.1674174972.git.scclevenger@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Ampere Computing ETMv4.x Support | expand |
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;
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...
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 --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;
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(+)