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(-)

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
-- 
2.47.1

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>
>> 
>