| Message ID | 20250108092538.11474-13-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | i.MX and SDHCI improvements | expand |
On 8/1/25 10:25, Bernhard Beschow wrote: > Also print the MMIO address when tracing. This allows to distinguishing the > many instances a typical i.MX SoC has. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/i2c/imx_i2c.c | 21 +++++---------------- > hw/i2c/trace-events | 5 +++++ > 2 files changed, 10 insertions(+), 16 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 9/1/25 12:43, Philippe Mathieu-Daudé wrote: > On 8/1/25 10:25, Bernhard Beschow wrote: >> Also print the MMIO address when tracing. This allows to >> distinguishing the >> many instances a typical i.MX SoC has. I'm not a fan of using peripheral address access, because it can change i.e. when a vCPU is accessing it from secure or non-secure mode. I'd rather use an 'id', a 'name' or even the QOM (canonical?) path. Maybe we should directly cache that as Device::qom_path, so all devices can use it for tracing, and we don't need to set an id/name property when creating the device... >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/i2c/imx_i2c.c | 21 +++++---------------- >> hw/i2c/trace-events | 5 +++++ >> 2 files changed, 10 insertions(+), 16 deletions(-) > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 9/1/25 12:56, Philippe Mathieu-Daudé wrote: > On 9/1/25 12:43, Philippe Mathieu-Daudé wrote: >> On 8/1/25 10:25, Bernhard Beschow wrote: >>> Also print the MMIO address when tracing. This allows to >>> distinguishing the >>> many instances a typical i.MX SoC has. > > I'm not a fan of using peripheral address access, because it > can change i.e. when a vCPU is accessing it from secure or > non-secure mode. > > I'd rather use an 'id', a 'name' or even the QOM (canonical?) > path. > > Maybe we should directly cache that as Device::qom_path, so > all devices can use it for tracing, and we don't need to set > an id/name property when creating the device... We already have that, it is Device::canonical_path :) > >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> hw/i2c/imx_i2c.c | 21 +++++---------------- >>> hw/i2c/trace-events | 5 +++++ >>> 2 files changed, 10 insertions(+), 16 deletions(-) >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org> >
Am 9. Januar 2025 12:38:11 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >On 9/1/25 12:56, Philippe Mathieu-Daudé wrote: >> On 9/1/25 12:43, Philippe Mathieu-Daudé wrote: >>> On 8/1/25 10:25, Bernhard Beschow wrote: >>>> Also print the MMIO address when tracing. This allows to distinguishing the >>>> many instances a typical i.MX SoC has. >> >> I'm not a fan of using peripheral address access, because it >> can change i.e. when a vCPU is accessing it from secure or >> non-secure mode. >> >> I'd rather use an 'id', a 'name' or even the QOM (canonical?) >> path. >> >> Maybe we should directly cache that as Device::qom_path, so >> all devices can use it for tracing, and we don't need to set >> an id/name property when creating the device... > >We already have that, it is Device::canonical_path :) Will do! > >> >>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>> --- >>>> hw/i2c/imx_i2c.c | 21 +++++---------------- >>>> hw/i2c/trace-events | 5 +++++ >>>> 2 files changed, 10 insertions(+), 16 deletions(-) >>> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> >
diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c index c565fd5b8a..be1688c064 100644 --- a/hw/i2c/imx_i2c.c +++ b/hw/i2c/imx_i2c.c @@ -25,18 +25,7 @@ #include "hw/i2c/i2c.h" #include "qemu/log.h" #include "qemu/module.h" - -#ifndef DEBUG_IMX_I2C -#define DEBUG_IMX_I2C 0 -#endif - -#define DPRINTF(fmt, args...) \ - do { \ - if (DEBUG_IMX_I2C) { \ - fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX_I2C, \ - __func__, ##args); \ - } \ - } while (0) +#include "trace.h" static const char *imx_i2c_get_regname(unsigned offset) { @@ -152,8 +141,8 @@ static uint64_t imx_i2c_read(void *opaque, hwaddr offset, break; } - DPRINTF("read %s [0x%" HWADDR_PRIx "] -> 0x%02x\n", - imx_i2c_get_regname(offset), offset, value); + trace_imx_i2c_read(s->iomem.addr, imx_i2c_get_regname(offset), offset, + value); return (uint64_t)value; } @@ -163,8 +152,8 @@ static void imx_i2c_write(void *opaque, hwaddr offset, { IMXI2CState *s = IMX_I2C(opaque); - DPRINTF("write %s [0x%" HWADDR_PRIx "] <- 0x%02x\n", - imx_i2c_get_regname(offset), offset, (int)value); + trace_imx_i2c_read(s->iomem.addr, imx_i2c_get_regname(offset), offset, + value); value &= 0xff; diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events index f708a7ace1..c6cba1ecf6 100644 --- a/hw/i2c/trace-events +++ b/hw/i2c/trace-events @@ -56,3 +56,8 @@ npcm7xx_smbus_recv_fifo(const char *id, uint8_t received, uint8_t expected) "%s pca954x_write_bytes(uint8_t value) "PCA954X write data: 0x%02x" pca954x_read_data(uint8_t value) "PCA954X read data: 0x%02x" + +# imx_i2c.c + +imx_i2c_read(uint64_t addr, const char *reg, uint64_t ofs, uint64_t value) "0x%" PRIx64 ":[%s (0x%" PRIx64 ")] -> 0x%02" PRIx64 +imx_i2c_write(uint64_t addr, const char *reg, uint64_t ofs, uint64_t value) "0x%" PRIx64 ":[%s (0x%" PRIx64 ")] <- 0x%02" PRIx64
Also print the MMIO address when tracing. This allows to distinguishing the many instances a typical i.MX SoC has. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/i2c/imx_i2c.c | 21 +++++---------------- hw/i2c/trace-events | 5 +++++ 2 files changed, 10 insertions(+), 16 deletions(-)