qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH-for-5.1? 0/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize
@ 2020-08-03 10:56 Philippe Mathieu-Daudé
  2020-08-03 10:56 ` [PATCH-for-5.1? 1/4] hw/arm/xilinx_zynq: Uninline cadence_uart_create() Philippe Mathieu-Daudé
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-03 10:56 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Daniel P. Berrangé,
	Eduardo Habkost, Alistair Francis, Philippe Mathieu-Daudé,
	qemu-arm, Edgar E. Iglesias, Paolo Bonzini

This is not a critical issue, but still annoying if you use tracing.

Maybe it is worth fixing for forks that are based on release tags,
else consider it 5.2 material.

Anyway we can still discuss if qdev_connect_clock_in() is supposed to
be callable *after* DeviceRealize.

Philippe Mathieu-Daudé (4):
  hw/arm/xilinx_zynq: Uninline cadence_uart_create()
  hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize
  hw/qdev-clock: Uninline qdev_connect_clock_in()
  hw/qdev-clock: Avoid calling qdev_connect_clock_in after DeviceRealize

 include/hw/char/cadence_uart.h | 17 -----------------
 include/hw/qdev-clock.h        |  8 +++-----
 hw/arm/xilinx_zynq.c           | 24 +++++++++++++++++-------
 hw/core/qdev-clock.c           |  6 ++++++
 4 files changed, 26 insertions(+), 29 deletions(-)

-- 
2.21.3



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH-for-5.1? 1/4] hw/arm/xilinx_zynq: Uninline cadence_uart_create()
  2020-08-03 10:56 [PATCH-for-5.1? 0/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize Philippe Mathieu-Daudé
@ 2020-08-03 10:56 ` Philippe Mathieu-Daudé
  2020-08-10 14:55   ` Alistair Francis
  2020-08-03 10:56 ` [PATCH-for-5.1? 2/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize Philippe Mathieu-Daudé
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-03 10:56 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Daniel P. Berrangé,
	Eduardo Habkost, Alistair Francis, Philippe Mathieu-Daudé,
	qemu-arm, Edgar E. Iglesias, Paolo Bonzini

As we want to call qdev_connect_clock_in() before the device
is realized, we need to uninline cadence_uart_create() first.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/char/cadence_uart.h | 17 -----------------
 hw/arm/xilinx_zynq.c           | 14 ++++++++++++--
 2 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/include/hw/char/cadence_uart.h b/include/hw/char/cadence_uart.h
index ed7b58d31d..dabc49ea4f 100644
--- a/include/hw/char/cadence_uart.h
+++ b/include/hw/char/cadence_uart.h
@@ -53,21 +53,4 @@ typedef struct {
     Clock *refclk;
 } CadenceUARTState;
 
-static inline DeviceState *cadence_uart_create(hwaddr addr,
-                                        qemu_irq irq,
-                                        Chardev *chr)
-{
-    DeviceState *dev;
-    SysBusDevice *s;
-
-    dev = qdev_new(TYPE_CADENCE_UART);
-    s = SYS_BUS_DEVICE(dev);
-    qdev_prop_set_chr(dev, "chardev", chr);
-    sysbus_realize_and_unref(s, &error_fatal);
-    sysbus_mmio_map(s, 0, addr);
-    sysbus_connect_irq(s, 0, irq);
-
-    return dev;
-}
-
 #endif
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 32aa7323d9..cf6d9757b5 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -254,10 +254,20 @@ static void zynq_init(MachineState *machine)
     sysbus_create_simple(TYPE_CHIPIDEA, 0xE0002000, pic[53 - IRQ_OFFSET]);
     sysbus_create_simple(TYPE_CHIPIDEA, 0xE0003000, pic[76 - IRQ_OFFSET]);
 
-    dev = cadence_uart_create(0xE0000000, pic[59 - IRQ_OFFSET], serial_hd(0));
+    dev = qdev_new(TYPE_CADENCE_UART);
+    busdev = SYS_BUS_DEVICE(dev);
+    qdev_prop_set_chr(dev, "chardev", serial_hd(0));
+    sysbus_realize_and_unref(busdev, &error_fatal);
+    sysbus_mmio_map(busdev, 0, 0xE0000000);
+    sysbus_connect_irq(busdev, 0, pic[59 - IRQ_OFFSET]);
     qdev_connect_clock_in(dev, "refclk",
                           qdev_get_clock_out(slcr, "uart0_ref_clk"));
-    dev = cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hd(1));
+    dev = qdev_new(TYPE_CADENCE_UART);
+    busdev = SYS_BUS_DEVICE(dev);
+    qdev_prop_set_chr(dev, "chardev", serial_hd(1));
+    sysbus_realize_and_unref(busdev, &error_fatal);
+    sysbus_mmio_map(busdev, 0, 0xE0001000);
+    sysbus_connect_irq(busdev, 0, pic[82 - IRQ_OFFSET]);
     qdev_connect_clock_in(dev, "refclk",
                           qdev_get_clock_out(slcr, "uart1_ref_clk"));
 
-- 
2.21.3



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH-for-5.1? 2/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize
  2020-08-03 10:56 [PATCH-for-5.1? 0/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize Philippe Mathieu-Daudé
  2020-08-03 10:56 ` [PATCH-for-5.1? 1/4] hw/arm/xilinx_zynq: Uninline cadence_uart_create() Philippe Mathieu-Daudé
@ 2020-08-03 10:56 ` Philippe Mathieu-Daudé
  2020-08-10 14:57   ` Alistair Francis
  2020-08-03 10:56 ` [PATCH-for-5.1? 3/4] hw/qdev-clock: Uninline qdev_connect_clock_in() Philippe Mathieu-Daudé
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-03 10:56 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Daniel P. Berrangé,
	Eduardo Habkost, Alistair Francis, Philippe Mathieu-Daudé,
	qemu-arm, Edgar E. Iglesias, Paolo Bonzini

Clock canonical name is set in device_set_realized (see the block
added to hw/core/qdev.c in commit 0e6934f264).
If we connect a clock after the device is realized, this code is
not executed. This is currently not a problem as this name is only
used for trace events, however this disrupt tracing.

Fix by calling qdev_connect_clock_in() before realizing.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/xilinx_zynq.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index cf6d9757b5..969ef0727c 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -222,18 +222,18 @@ static void zynq_init(MachineState *machine)
                           1, 0x0066, 0x0022, 0x0000, 0x0000, 0x0555, 0x2aa,
                           0);
 
-    /* Create slcr, keep a pointer to connect clocks */
-    slcr = qdev_new("xilinx,zynq_slcr");
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(slcr), &error_fatal);
-    sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF8000000);
-
     /* Create the main clock source, and feed slcr with it */
     zynq_machine->ps_clk = CLOCK(object_new(TYPE_CLOCK));
     object_property_add_child(OBJECT(zynq_machine), "ps_clk",
                               OBJECT(zynq_machine->ps_clk));
     object_unref(OBJECT(zynq_machine->ps_clk));
     clock_set_hz(zynq_machine->ps_clk, PS_CLK_FREQUENCY);
+
+    /* Create slcr, keep a pointer to connect clocks */
+    slcr = qdev_new("xilinx,zynq_slcr");
     qdev_connect_clock_in(slcr, "ps_clk", zynq_machine->ps_clk);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(slcr), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF8000000);
 
     dev = qdev_new(TYPE_A9MPCORE_PRIV);
     qdev_prop_set_uint32(dev, "num-cpu", 1);
@@ -257,19 +257,19 @@ static void zynq_init(MachineState *machine)
     dev = qdev_new(TYPE_CADENCE_UART);
     busdev = SYS_BUS_DEVICE(dev);
     qdev_prop_set_chr(dev, "chardev", serial_hd(0));
+    qdev_connect_clock_in(dev, "refclk",
+                          qdev_get_clock_out(slcr, "uart0_ref_clk"));
     sysbus_realize_and_unref(busdev, &error_fatal);
     sysbus_mmio_map(busdev, 0, 0xE0000000);
     sysbus_connect_irq(busdev, 0, pic[59 - IRQ_OFFSET]);
-    qdev_connect_clock_in(dev, "refclk",
-                          qdev_get_clock_out(slcr, "uart0_ref_clk"));
     dev = qdev_new(TYPE_CADENCE_UART);
     busdev = SYS_BUS_DEVICE(dev);
     qdev_prop_set_chr(dev, "chardev", serial_hd(1));
+    qdev_connect_clock_in(dev, "refclk",
+                          qdev_get_clock_out(slcr, "uart1_ref_clk"));
     sysbus_realize_and_unref(busdev, &error_fatal);
     sysbus_mmio_map(busdev, 0, 0xE0001000);
     sysbus_connect_irq(busdev, 0, pic[82 - IRQ_OFFSET]);
-    qdev_connect_clock_in(dev, "refclk",
-                          qdev_get_clock_out(slcr, "uart1_ref_clk"));
 
     sysbus_create_varargs("cadence_ttc", 0xF8001000,
             pic[42-IRQ_OFFSET], pic[43-IRQ_OFFSET], pic[44-IRQ_OFFSET], NULL);
-- 
2.21.3



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH-for-5.1? 3/4] hw/qdev-clock: Uninline qdev_connect_clock_in()
  2020-08-03 10:56 [PATCH-for-5.1? 0/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize Philippe Mathieu-Daudé
  2020-08-03 10:56 ` [PATCH-for-5.1? 1/4] hw/arm/xilinx_zynq: Uninline cadence_uart_create() Philippe Mathieu-Daudé
  2020-08-03 10:56 ` [PATCH-for-5.1? 2/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize Philippe Mathieu-Daudé
@ 2020-08-03 10:56 ` Philippe Mathieu-Daudé
  2020-08-10 14:59   ` Alistair Francis
  2020-08-03 10:56 ` [PATCH-for-5.1? 4/4] hw/qdev-clock: Avoid calling qdev_connect_clock_in after DeviceRealize Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-03 10:56 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Daniel P. Berrangé,
	Eduardo Habkost, Alistair Francis, Philippe Mathieu-Daudé,
	qemu-arm, Edgar E. Iglesias, Paolo Bonzini

We want to assert the device is not realized. To avoid overloading
this header including "hw/qdev-core.h", uninline the function first.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/qdev-clock.h | 6 +-----
 hw/core/qdev-clock.c    | 5 +++++
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
index a340f65ff9..a897f7c9d0 100644
--- a/include/hw/qdev-clock.h
+++ b/include/hw/qdev-clock.h
@@ -71,11 +71,7 @@ Clock *qdev_get_clock_out(DeviceState *dev, const char *name);
  * Set the source clock of input clock @name of device @dev to @source.
  * @source period update will be propagated to @name clock.
  */
-static inline void qdev_connect_clock_in(DeviceState *dev, const char *name,
-                                         Clock *source)
-{
-    clock_set_source(qdev_get_clock_in(dev, name), source);
-}
+void qdev_connect_clock_in(DeviceState *dev, const char *name, Clock *source);
 
 /**
  * qdev_alias_clock:
diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
index 5cc1e82e51..f139b68b88 100644
--- a/hw/core/qdev-clock.c
+++ b/hw/core/qdev-clock.c
@@ -183,3 +183,8 @@ Clock *qdev_alias_clock(DeviceState *dev, const char *name,
 
     return ncl->clock;
 }
+
+void qdev_connect_clock_in(DeviceState *dev, const char *name, Clock *source)
+{
+    clock_set_source(qdev_get_clock_in(dev, name), source);
+}
-- 
2.21.3



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH-for-5.1? 4/4] hw/qdev-clock: Avoid calling qdev_connect_clock_in after DeviceRealize
  2020-08-03 10:56 [PATCH-for-5.1? 0/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2020-08-03 10:56 ` [PATCH-for-5.1? 3/4] hw/qdev-clock: Uninline qdev_connect_clock_in() Philippe Mathieu-Daudé
@ 2020-08-03 10:56 ` Philippe Mathieu-Daudé
  2020-08-10 14:58   ` Alistair Francis
  2020-08-22 20:11 ` [PATCH-for-5.1? 0/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize Philippe Mathieu-Daudé
  2020-08-24 16:39 ` Peter Maydell
  5 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-03 10:56 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Daniel P. Berrangé,
	Eduardo Habkost, Alistair Francis, Philippe Mathieu-Daudé,
	qemu-arm, Edgar E. Iglesias, Paolo Bonzini

Clock canonical name is set in device_set_realized (see the block
added to hw/core/qdev.c in commit 0e6934f264).
If we connect a clock after the device is realized, this code is
not executed. This is currently not a problem as this name is only
used for trace events, however this disrupt tracing.

Add a comment to document qdev_connect_clock_in() must be called
before the device is realized, and assert this condition.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/qdev-clock.h | 2 ++
 hw/core/qdev-clock.c    | 1 +
 2 files changed, 3 insertions(+)

diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
index a897f7c9d0..64ca4d266f 100644
--- a/include/hw/qdev-clock.h
+++ b/include/hw/qdev-clock.h
@@ -70,6 +70,8 @@ Clock *qdev_get_clock_out(DeviceState *dev, const char *name);
  *
  * Set the source clock of input clock @name of device @dev to @source.
  * @source period update will be propagated to @name clock.
+ *
+ * Must be called before @dev is realized.
  */
 void qdev_connect_clock_in(DeviceState *dev, const char *name, Clock *source);
 
diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
index f139b68b88..47ecb5b4fa 100644
--- a/hw/core/qdev-clock.c
+++ b/hw/core/qdev-clock.c
@@ -186,5 +186,6 @@ Clock *qdev_alias_clock(DeviceState *dev, const char *name,
 
 void qdev_connect_clock_in(DeviceState *dev, const char *name, Clock *source)
 {
+    assert(!dev->realized);
     clock_set_source(qdev_get_clock_in(dev, name), source);
 }
-- 
2.21.3



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH-for-5.1? 1/4] hw/arm/xilinx_zynq: Uninline cadence_uart_create()
  2020-08-03 10:56 ` [PATCH-for-5.1? 1/4] hw/arm/xilinx_zynq: Uninline cadence_uart_create() Philippe Mathieu-Daudé
@ 2020-08-10 14:55   ` Alistair Francis
  0 siblings, 0 replies; 11+ messages in thread
From: Alistair Francis @ 2020-08-10 14:55 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Damien Hedde, Peter Maydell, Daniel P. Berrangé,
	Eduardo Habkost, Alistair Francis,
	qemu-devel@nongnu.org Developers, qemu-arm, Paolo Bonzini,
	Edgar E. Iglesias

On Mon, Aug 3, 2020 at 3:57 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> As we want to call qdev_connect_clock_in() before the device
> is realized, we need to uninline cadence_uart_create() first.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/char/cadence_uart.h | 17 -----------------
>  hw/arm/xilinx_zynq.c           | 14 ++++++++++++--
>  2 files changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/include/hw/char/cadence_uart.h b/include/hw/char/cadence_uart.h
> index ed7b58d31d..dabc49ea4f 100644
> --- a/include/hw/char/cadence_uart.h
> +++ b/include/hw/char/cadence_uart.h
> @@ -53,21 +53,4 @@ typedef struct {
>      Clock *refclk;
>  } CadenceUARTState;
>
> -static inline DeviceState *cadence_uart_create(hwaddr addr,
> -                                        qemu_irq irq,
> -                                        Chardev *chr)
> -{
> -    DeviceState *dev;
> -    SysBusDevice *s;
> -
> -    dev = qdev_new(TYPE_CADENCE_UART);
> -    s = SYS_BUS_DEVICE(dev);
> -    qdev_prop_set_chr(dev, "chardev", chr);
> -    sysbus_realize_and_unref(s, &error_fatal);
> -    sysbus_mmio_map(s, 0, addr);
> -    sysbus_connect_irq(s, 0, irq);
> -
> -    return dev;
> -}
> -
>  #endif
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 32aa7323d9..cf6d9757b5 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -254,10 +254,20 @@ static void zynq_init(MachineState *machine)
>      sysbus_create_simple(TYPE_CHIPIDEA, 0xE0002000, pic[53 - IRQ_OFFSET]);
>      sysbus_create_simple(TYPE_CHIPIDEA, 0xE0003000, pic[76 - IRQ_OFFSET]);
>
> -    dev = cadence_uart_create(0xE0000000, pic[59 - IRQ_OFFSET], serial_hd(0));
> +    dev = qdev_new(TYPE_CADENCE_UART);
> +    busdev = SYS_BUS_DEVICE(dev);
> +    qdev_prop_set_chr(dev, "chardev", serial_hd(0));
> +    sysbus_realize_and_unref(busdev, &error_fatal);
> +    sysbus_mmio_map(busdev, 0, 0xE0000000);
> +    sysbus_connect_irq(busdev, 0, pic[59 - IRQ_OFFSET]);
>      qdev_connect_clock_in(dev, "refclk",
>                            qdev_get_clock_out(slcr, "uart0_ref_clk"));
> -    dev = cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hd(1));
> +    dev = qdev_new(TYPE_CADENCE_UART);
> +    busdev = SYS_BUS_DEVICE(dev);
> +    qdev_prop_set_chr(dev, "chardev", serial_hd(1));
> +    sysbus_realize_and_unref(busdev, &error_fatal);
> +    sysbus_mmio_map(busdev, 0, 0xE0001000);
> +    sysbus_connect_irq(busdev, 0, pic[82 - IRQ_OFFSET]);
>      qdev_connect_clock_in(dev, "refclk",
>                            qdev_get_clock_out(slcr, "uart1_ref_clk"));
>
> --
> 2.21.3
>
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH-for-5.1? 2/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize
  2020-08-03 10:56 ` [PATCH-for-5.1? 2/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize Philippe Mathieu-Daudé
@ 2020-08-10 14:57   ` Alistair Francis
  0 siblings, 0 replies; 11+ messages in thread
From: Alistair Francis @ 2020-08-10 14:57 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Damien Hedde, Peter Maydell, Daniel P. Berrangé,
	Eduardo Habkost, Alistair Francis,
	qemu-devel@nongnu.org Developers, qemu-arm, Paolo Bonzini,
	Edgar E. Iglesias

On Mon, Aug 3, 2020 at 3:57 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Clock canonical name is set in device_set_realized (see the block
> added to hw/core/qdev.c in commit 0e6934f264).
> If we connect a clock after the device is realized, this code is
> not executed. This is currently not a problem as this name is only
> used for trace events, however this disrupt tracing.
>
> Fix by calling qdev_connect_clock_in() before realizing.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/xilinx_zynq.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index cf6d9757b5..969ef0727c 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -222,18 +222,18 @@ static void zynq_init(MachineState *machine)
>                            1, 0x0066, 0x0022, 0x0000, 0x0000, 0x0555, 0x2aa,
>                            0);
>
> -    /* Create slcr, keep a pointer to connect clocks */
> -    slcr = qdev_new("xilinx,zynq_slcr");
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(slcr), &error_fatal);
> -    sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF8000000);
> -
>      /* Create the main clock source, and feed slcr with it */
>      zynq_machine->ps_clk = CLOCK(object_new(TYPE_CLOCK));
>      object_property_add_child(OBJECT(zynq_machine), "ps_clk",
>                                OBJECT(zynq_machine->ps_clk));
>      object_unref(OBJECT(zynq_machine->ps_clk));
>      clock_set_hz(zynq_machine->ps_clk, PS_CLK_FREQUENCY);
> +
> +    /* Create slcr, keep a pointer to connect clocks */
> +    slcr = qdev_new("xilinx,zynq_slcr");
>      qdev_connect_clock_in(slcr, "ps_clk", zynq_machine->ps_clk);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(slcr), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF8000000);
>
>      dev = qdev_new(TYPE_A9MPCORE_PRIV);
>      qdev_prop_set_uint32(dev, "num-cpu", 1);
> @@ -257,19 +257,19 @@ static void zynq_init(MachineState *machine)
>      dev = qdev_new(TYPE_CADENCE_UART);
>      busdev = SYS_BUS_DEVICE(dev);
>      qdev_prop_set_chr(dev, "chardev", serial_hd(0));
> +    qdev_connect_clock_in(dev, "refclk",
> +                          qdev_get_clock_out(slcr, "uart0_ref_clk"));
>      sysbus_realize_and_unref(busdev, &error_fatal);
>      sysbus_mmio_map(busdev, 0, 0xE0000000);
>      sysbus_connect_irq(busdev, 0, pic[59 - IRQ_OFFSET]);
> -    qdev_connect_clock_in(dev, "refclk",
> -                          qdev_get_clock_out(slcr, "uart0_ref_clk"));
>      dev = qdev_new(TYPE_CADENCE_UART);
>      busdev = SYS_BUS_DEVICE(dev);
>      qdev_prop_set_chr(dev, "chardev", serial_hd(1));
> +    qdev_connect_clock_in(dev, "refclk",
> +                          qdev_get_clock_out(slcr, "uart1_ref_clk"));
>      sysbus_realize_and_unref(busdev, &error_fatal);
>      sysbus_mmio_map(busdev, 0, 0xE0001000);
>      sysbus_connect_irq(busdev, 0, pic[82 - IRQ_OFFSET]);
> -    qdev_connect_clock_in(dev, "refclk",
> -                          qdev_get_clock_out(slcr, "uart1_ref_clk"));
>
>      sysbus_create_varargs("cadence_ttc", 0xF8001000,
>              pic[42-IRQ_OFFSET], pic[43-IRQ_OFFSET], pic[44-IRQ_OFFSET], NULL);
> --
> 2.21.3
>
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH-for-5.1? 4/4] hw/qdev-clock: Avoid calling qdev_connect_clock_in after DeviceRealize
  2020-08-03 10:56 ` [PATCH-for-5.1? 4/4] hw/qdev-clock: Avoid calling qdev_connect_clock_in after DeviceRealize Philippe Mathieu-Daudé
@ 2020-08-10 14:58   ` Alistair Francis
  0 siblings, 0 replies; 11+ messages in thread
From: Alistair Francis @ 2020-08-10 14:58 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Damien Hedde, Peter Maydell, Daniel P. Berrangé,
	Eduardo Habkost, Alistair Francis,
	qemu-devel@nongnu.org Developers, qemu-arm, Paolo Bonzini,
	Edgar E. Iglesias

On Mon, Aug 3, 2020 at 3:59 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Clock canonical name is set in device_set_realized (see the block
> added to hw/core/qdev.c in commit 0e6934f264).
> If we connect a clock after the device is realized, this code is
> not executed. This is currently not a problem as this name is only
> used for trace events, however this disrupt tracing.
>
> Add a comment to document qdev_connect_clock_in() must be called
> before the device is realized, and assert this condition.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/qdev-clock.h | 2 ++
>  hw/core/qdev-clock.c    | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
> index a897f7c9d0..64ca4d266f 100644
> --- a/include/hw/qdev-clock.h
> +++ b/include/hw/qdev-clock.h
> @@ -70,6 +70,8 @@ Clock *qdev_get_clock_out(DeviceState *dev, const char *name);
>   *
>   * Set the source clock of input clock @name of device @dev to @source.
>   * @source period update will be propagated to @name clock.
> + *
> + * Must be called before @dev is realized.
>   */
>  void qdev_connect_clock_in(DeviceState *dev, const char *name, Clock *source);
>
> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
> index f139b68b88..47ecb5b4fa 100644
> --- a/hw/core/qdev-clock.c
> +++ b/hw/core/qdev-clock.c
> @@ -186,5 +186,6 @@ Clock *qdev_alias_clock(DeviceState *dev, const char *name,
>
>  void qdev_connect_clock_in(DeviceState *dev, const char *name, Clock *source)
>  {
> +    assert(!dev->realized);
>      clock_set_source(qdev_get_clock_in(dev, name), source);
>  }
> --
> 2.21.3
>
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH-for-5.1? 3/4] hw/qdev-clock: Uninline qdev_connect_clock_in()
  2020-08-03 10:56 ` [PATCH-for-5.1? 3/4] hw/qdev-clock: Uninline qdev_connect_clock_in() Philippe Mathieu-Daudé
@ 2020-08-10 14:59   ` Alistair Francis
  0 siblings, 0 replies; 11+ messages in thread
From: Alistair Francis @ 2020-08-10 14:59 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Damien Hedde, Peter Maydell, Daniel P. Berrangé,
	Eduardo Habkost, Alistair Francis,
	qemu-devel@nongnu.org Developers, qemu-arm, Paolo Bonzini,
	Edgar E. Iglesias

On Mon, Aug 3, 2020 at 3:57 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> We want to assert the device is not realized. To avoid overloading
> this header including "hw/qdev-core.h", uninline the function first.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/qdev-clock.h | 6 +-----
>  hw/core/qdev-clock.c    | 5 +++++
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
> index a340f65ff9..a897f7c9d0 100644
> --- a/include/hw/qdev-clock.h
> +++ b/include/hw/qdev-clock.h
> @@ -71,11 +71,7 @@ Clock *qdev_get_clock_out(DeviceState *dev, const char *name);
>   * Set the source clock of input clock @name of device @dev to @source.
>   * @source period update will be propagated to @name clock.
>   */
> -static inline void qdev_connect_clock_in(DeviceState *dev, const char *name,
> -                                         Clock *source)
> -{
> -    clock_set_source(qdev_get_clock_in(dev, name), source);
> -}
> +void qdev_connect_clock_in(DeviceState *dev, const char *name, Clock *source);
>
>  /**
>   * qdev_alias_clock:
> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
> index 5cc1e82e51..f139b68b88 100644
> --- a/hw/core/qdev-clock.c
> +++ b/hw/core/qdev-clock.c
> @@ -183,3 +183,8 @@ Clock *qdev_alias_clock(DeviceState *dev, const char *name,
>
>      return ncl->clock;
>  }
> +
> +void qdev_connect_clock_in(DeviceState *dev, const char *name, Clock *source)
> +{
> +    clock_set_source(qdev_get_clock_in(dev, name), source);
> +}
> --
> 2.21.3
>
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH-for-5.1? 0/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize
  2020-08-03 10:56 [PATCH-for-5.1? 0/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize Philippe Mathieu-Daudé
                   ` (3 preceding siblings ...)
  2020-08-03 10:56 ` [PATCH-for-5.1? 4/4] hw/qdev-clock: Avoid calling qdev_connect_clock_in after DeviceRealize Philippe Mathieu-Daudé
@ 2020-08-22 20:11 ` Philippe Mathieu-Daudé
  2020-08-24 16:39 ` Peter Maydell
  5 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-22 20:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Daniel P. Berrangé,
	Eduardo Habkost, Alistair Francis, qemu-arm, Paolo Bonzini

ping now that 5.2 is opened :)

On 8/3/20 12:56 PM, Philippe Mathieu-Daudé wrote:
> This is not a critical issue, but still annoying if you use tracing.
> 
> Maybe it is worth fixing for forks that are based on release tags,
> else consider it 5.2 material.
> 
> Anyway we can still discuss if qdev_connect_clock_in() is supposed to
> be callable *after* DeviceRealize.
> 
> Philippe Mathieu-Daudé (4):
>   hw/arm/xilinx_zynq: Uninline cadence_uart_create()
>   hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize
>   hw/qdev-clock: Uninline qdev_connect_clock_in()
>   hw/qdev-clock: Avoid calling qdev_connect_clock_in after DeviceRealize
> 
>  include/hw/char/cadence_uart.h | 17 -----------------
>  include/hw/qdev-clock.h        |  8 +++-----
>  hw/arm/xilinx_zynq.c           | 24 +++++++++++++++++-------
>  hw/core/qdev-clock.c           |  6 ++++++
>  4 files changed, 26 insertions(+), 29 deletions(-)
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH-for-5.1? 0/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize
  2020-08-03 10:56 [PATCH-for-5.1? 0/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize Philippe Mathieu-Daudé
                   ` (4 preceding siblings ...)
  2020-08-22 20:11 ` [PATCH-for-5.1? 0/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize Philippe Mathieu-Daudé
@ 2020-08-24 16:39 ` Peter Maydell
  5 siblings, 0 replies; 11+ messages in thread
From: Peter Maydell @ 2020-08-24 16:39 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Damien Hedde, Daniel P. Berrangé,
	Eduardo Habkost, Alistair Francis, QEMU Developers, qemu-arm,
	Edgar E. Iglesias, Paolo Bonzini

On Mon, 3 Aug 2020 at 11:56, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> This is not a critical issue, but still annoying if you use tracing.
>
> Maybe it is worth fixing for forks that are based on release tags,
> else consider it 5.2 material.
>
> Anyway we can still discuss if qdev_connect_clock_in() is supposed to
> be callable *after* DeviceRealize.
>
> Philippe Mathieu-Daudé (4):
>   hw/arm/xilinx_zynq: Uninline cadence_uart_create()
>   hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize
>   hw/qdev-clock: Uninline qdev_connect_clock_in()
>   hw/qdev-clock: Avoid calling qdev_connect_clock_in after DeviceRealize
>


Applied to target-arm.next, thanks.

-- PMM


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-08-24 16:40 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-03 10:56 [PATCH-for-5.1? 0/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize Philippe Mathieu-Daudé
2020-08-03 10:56 ` [PATCH-for-5.1? 1/4] hw/arm/xilinx_zynq: Uninline cadence_uart_create() Philippe Mathieu-Daudé
2020-08-10 14:55   ` Alistair Francis
2020-08-03 10:56 ` [PATCH-for-5.1? 2/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize Philippe Mathieu-Daudé
2020-08-10 14:57   ` Alistair Francis
2020-08-03 10:56 ` [PATCH-for-5.1? 3/4] hw/qdev-clock: Uninline qdev_connect_clock_in() Philippe Mathieu-Daudé
2020-08-10 14:59   ` Alistair Francis
2020-08-03 10:56 ` [PATCH-for-5.1? 4/4] hw/qdev-clock: Avoid calling qdev_connect_clock_in after DeviceRealize Philippe Mathieu-Daudé
2020-08-10 14:58   ` Alistair Francis
2020-08-22 20:11 ` [PATCH-for-5.1? 0/4] hw/arm/xilinx_zynq: Call qdev_connect_clock_in() before DeviceRealize Philippe Mathieu-Daudé
2020-08-24 16:39 ` Peter Maydell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).