qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes
@ 2024-05-31 20:27 Daniel Henrique Barboza
  2024-05-31 20:27 ` [PATCH v2 1/8] hw/riscv/virt.c: add address-cells in create_fdt_one_aplic() Daniel Henrique Barboza
                   ` (9 more replies)
  0 siblings, 10 replies; 19+ messages in thread
From: Daniel Henrique Barboza @ 2024-05-31 20:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-riscv, alistair.francis, bmeng, liwei1518, zhiwei_liu,
	palmer, apatel, ajones, conor, Daniel Henrique Barboza

Hi,

This is a series that is being spun from the reviews given on patch 1
[1]. We'll fix some DT validation issues we have in the 'virt' machine
[2] that aren't related to missing extensions in the DT spec.

I'll leave to maintainers to squash the patches as they see fit. I
split it this way to make it easier to bissect possible bugs that these
individual changes can cause.

These are the types of DT warnings solved by this series:

/home/danielhb/work/qemu/riscv64_virt.dtb: aplic@d000000: $nodename:0: 'aplic@d000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
/home/danielhb/work/qemu/riscv64_virt.dtb: aplic@d000000: compatible:0: 'riscv,aplic' is not one of ['qemu,aplic']
	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
/home/danielhb/work/qemu/riscv64_virt.dtb: aplic@d000000: compatible: ['riscv,aplic'] is too short
	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
/home/danielhb/work/qemu/riscv64_virt.dtb: aplic@d000000: Unevaluated properties are not allowed ('compatible' was unexpected)
	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
(...)
/home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: $nodename:0: 'imsics@28000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
/home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: compatible:0: 'riscv,imsics' is not one of ['qemu,imsics']
	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
/home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: compatible: ['riscv,imsics'] is too short
	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
/home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: '#msi-cells' is a required property
	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
/home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: Unevaluated properties are not allowed ('compatible' was unexpected)
	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#

[3] explains how to run 'dt-validate' to reproduce them. To generate a
'processed schema' file what I did was:

- in the Linux kernel tree, run 'make dt_binding_check'. Please note
  that this might require installation of additional python stuff
  (e.g.swig, python3-devel)

- I used the generated file 'Documentation/devicetree/bindings/processed-schema.json'
  as a 'processed schema'.

Series applicable on both master and alistair/riscv-to-apply.next. 

Changes from v1:
- added patches 2 to 7 to fix the dt-validate warnings on imsics and
  aplic notes
- v1 link: https://lore.kernel.org/qemu-riscv/20240530084949.761034-1-dbarboza@ventanamicro.com/

[1] https://lore.kernel.org/qemu-riscv/20240530084949.761034-1-dbarboza@ventanamicro.com/
[2] https://lore.kernel.org/all/20240529-rust-tile-a05517a6260f@spud/
[3] https://lore.kernel.org/qemu-riscv/20240530-landed-shriek-9362981afade@spud/ 

Daniel Henrique Barboza (8):
  hw/riscv/virt.c: add address-cells in create_fdt_one_aplic()
  hw/riscv/virt.c: add aplic nodename helper
  hw/riscv/virt.c: rename aplic nodename to 'interrupt-controller'
  hw/riscv/virt.c: aplic DT: add 'qemu,aplic' to 'compatible'
  hw/riscv/virt.c: aplic DT: rename prop to 'riscv,delegation'
  hw/riscv/virt.c: change imsic nodename to 'interrupt-controller'
  hw/riscv/virt.c: imsics DT: add 'qemu,imsics' to 'compatible'
  hw/riscv/virt.c: imsics DT: add '#msi-cells'

 hw/riscv/virt.c         | 36 +++++++++++++++++++++++++++---------
 include/hw/riscv/virt.h |  1 +
 2 files changed, 28 insertions(+), 9 deletions(-)

-- 
2.45.1



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

* [PATCH v2 1/8] hw/riscv/virt.c: add address-cells in create_fdt_one_aplic()
  2024-05-31 20:27 [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes Daniel Henrique Barboza
@ 2024-05-31 20:27 ` Daniel Henrique Barboza
  2024-06-05  0:06   ` Alistair Francis
  2024-05-31 20:27 ` [PATCH v2 2/8] hw/riscv/virt.c: add aplic nodename helper Daniel Henrique Barboza
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Daniel Henrique Barboza @ 2024-05-31 20:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-riscv, alistair.francis, bmeng, liwei1518, zhiwei_liu,
	palmer, apatel, ajones, conor, Daniel Henrique Barboza

We need #address-cells properties in all interrupt controllers that are
referred by an interrupt-map [1]. For the RISC-V machine, both PLIC and
APLIC controllers must have this property.

PLIC already sets it in create_fdt_socket_plic(). Set the property for
APLIC in create_fdt_one_aplic().

[1] https://lore.kernel.org/linux-arm-kernel/CAL_JsqJE15D-xXxmELsmuD+JQHZzxGzdXvikChn6KFWqk6NzPw@mail.gmail.com/

Suggested-by: Anup Patel <apatel@ventanamicro.com>
Fixes: e6faee65855b ("hw/riscv: virt: Add optional AIA APLIC support to virt machine")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c         | 2 ++
 include/hw/riscv/virt.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 4fdb660525..1a7e1e73c5 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -609,6 +609,8 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
     aplic_name = g_strdup_printf("/soc/aplic@%lx", aplic_addr);
     qemu_fdt_add_subnode(ms->fdt, aplic_name);
     qemu_fdt_setprop_string(ms->fdt, aplic_name, "compatible", "riscv,aplic");
+    qemu_fdt_setprop_cell(ms->fdt, aplic_name, "#address-cells",
+                          FDT_APLIC_ADDR_CELLS);
     qemu_fdt_setprop_cell(ms->fdt, aplic_name,
                           "#interrupt-cells", FDT_APLIC_INT_CELLS);
     qemu_fdt_setprop(ms->fdt, aplic_name, "interrupt-controller", NULL, 0);
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 3db839160f..c0dc41ff9a 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -118,6 +118,7 @@ enum {
 #define FDT_PLIC_ADDR_CELLS   0
 #define FDT_PLIC_INT_CELLS    1
 #define FDT_APLIC_INT_CELLS   2
+#define FDT_APLIC_ADDR_CELLS  0
 #define FDT_IMSIC_INT_CELLS   0
 #define FDT_MAX_INT_CELLS     2
 #define FDT_MAX_INT_MAP_WIDTH (FDT_PCI_ADDR_CELLS + FDT_PCI_INT_CELLS + \
-- 
2.45.1



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

* [PATCH v2 2/8] hw/riscv/virt.c: add aplic nodename helper
  2024-05-31 20:27 [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes Daniel Henrique Barboza
  2024-05-31 20:27 ` [PATCH v2 1/8] hw/riscv/virt.c: add address-cells in create_fdt_one_aplic() Daniel Henrique Barboza
@ 2024-05-31 20:27 ` Daniel Henrique Barboza
  2024-06-05  0:18   ` Alistair Francis
  2024-05-31 20:27 ` [PATCH v2 3/8] hw/riscv/virt.c: rename aplic nodename to 'interrupt-controller' Daniel Henrique Barboza
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Daniel Henrique Barboza @ 2024-05-31 20:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-riscv, alistair.francis, bmeng, liwei1518, zhiwei_liu,
	palmer, apatel, ajones, conor, Daniel Henrique Barboza

We'll change the aplic DT nodename in the next patch and the name is
hardcoded in 2 different functions. Create a helper to change a single
place later.

While we're at it, in create_fdt_socket_aplic(), move 'aplic_name'
inside the conditional to avoid allocating a string that won't be used
when socket == NULL.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 1a7e1e73c5..07a07f5ce1 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -588,6 +588,12 @@ static void create_fdt_imsic(RISCVVirtState *s, const MemMapEntry *memmap,
 
 }
 
+/* Caller must free string after use */
+static char *fdt_get_aplic_nodename(unsigned long aplic_addr)
+{
+    return g_strdup_printf("/soc/aplic@%lx", aplic_addr);
+}
+
 static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
                                  unsigned long aplic_addr, uint32_t aplic_size,
                                  uint32_t msi_phandle,
@@ -597,7 +603,7 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
                                  bool m_mode, int num_harts)
 {
     int cpu;
-    g_autofree char *aplic_name = NULL;
+    g_autofree char *aplic_name = fdt_get_aplic_nodename(aplic_addr);
     g_autofree uint32_t *aplic_cells = g_new0(uint32_t, num_harts * 2);
     MachineState *ms = MACHINE(s);
 
@@ -606,7 +612,6 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
         aplic_cells[cpu * 2 + 1] = cpu_to_be32(m_mode ? IRQ_M_EXT : IRQ_S_EXT);
     }
 
-    aplic_name = g_strdup_printf("/soc/aplic@%lx", aplic_addr);
     qemu_fdt_add_subnode(ms->fdt, aplic_name);
     qemu_fdt_setprop_string(ms->fdt, aplic_name, "compatible", "riscv,aplic");
     qemu_fdt_setprop_cell(ms->fdt, aplic_name, "#address-cells",
@@ -648,7 +653,6 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
                                     uint32_t *aplic_phandles,
                                     int num_harts)
 {
-    g_autofree char *aplic_name = NULL;
     unsigned long aplic_addr;
     MachineState *ms = MACHINE(s);
     uint32_t aplic_m_phandle, aplic_s_phandle;
@@ -674,9 +678,8 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
                          aplic_s_phandle, 0,
                          false, num_harts);
 
-    aplic_name = g_strdup_printf("/soc/aplic@%lx", aplic_addr);
-
     if (!socket) {
+        g_autofree char *aplic_name = fdt_get_aplic_nodename(aplic_addr);
         platform_bus_add_all_fdt_nodes(ms->fdt, aplic_name,
                                        memmap[VIRT_PLATFORM_BUS].base,
                                        memmap[VIRT_PLATFORM_BUS].size,
-- 
2.45.1



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

* [PATCH v2 3/8] hw/riscv/virt.c: rename aplic nodename to 'interrupt-controller'
  2024-05-31 20:27 [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes Daniel Henrique Barboza
  2024-05-31 20:27 ` [PATCH v2 1/8] hw/riscv/virt.c: add address-cells in create_fdt_one_aplic() Daniel Henrique Barboza
  2024-05-31 20:27 ` [PATCH v2 2/8] hw/riscv/virt.c: add aplic nodename helper Daniel Henrique Barboza
@ 2024-05-31 20:27 ` Daniel Henrique Barboza
  2024-06-05  0:30   ` Alistair Francis
  2024-05-31 20:27 ` [PATCH v2 4/8] hw/riscv/virt.c: aplic DT: add 'qemu, aplic' to 'compatible' Daniel Henrique Barboza
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Daniel Henrique Barboza @ 2024-05-31 20:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-riscv, alistair.francis, bmeng, liwei1518, zhiwei_liu,
	palmer, apatel, ajones, conor, Daniel Henrique Barboza

The correct name of the aplic controller node, as per Linux kernel DT
docs [1], is 'interrupt-controller@addr'.

[1] Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml

Reported-by: Conor Dooley <conor@kernel.org>
Fixes: e6faee65855b ("hw/riscv: virt: Add optional AIA APLIC support to virt machine")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 07a07f5ce1..5505047945 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -591,7 +591,7 @@ static void create_fdt_imsic(RISCVVirtState *s, const MemMapEntry *memmap,
 /* Caller must free string after use */
 static char *fdt_get_aplic_nodename(unsigned long aplic_addr)
 {
-    return g_strdup_printf("/soc/aplic@%lx", aplic_addr);
+    return g_strdup_printf("/soc/interrupt-controller@%lx", aplic_addr);
 }
 
 static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
-- 
2.45.1



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

* [PATCH v2 4/8] hw/riscv/virt.c: aplic DT: add 'qemu, aplic' to 'compatible'
  2024-05-31 20:27 [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes Daniel Henrique Barboza
                   ` (2 preceding siblings ...)
  2024-05-31 20:27 ` [PATCH v2 3/8] hw/riscv/virt.c: rename aplic nodename to 'interrupt-controller' Daniel Henrique Barboza
@ 2024-05-31 20:27 ` Daniel Henrique Barboza
  2024-06-05  0:31   ` Alistair Francis
  2024-05-31 20:27 ` [PATCH v2 5/8] hw/riscv/virt.c: aplic DT: rename prop to 'riscv, delegation' Daniel Henrique Barboza
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Daniel Henrique Barboza @ 2024-05-31 20:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-riscv, alistair.francis, bmeng, liwei1518, zhiwei_liu,
	palmer, apatel, ajones, conor, Daniel Henrique Barboza

The DT docs for riscv,aplic [1] predicts a 'qemu,aplic' enum in the
'compatible' property.

[1] Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml

Reported-by: Conor Dooley <conor@kernel.org>
Fixes: e6faee65855b ("hw/riscv: virt: Add optional AIA APLIC support to virt machine")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 5505047945..366fe042cc 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -606,6 +606,9 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
     g_autofree char *aplic_name = fdt_get_aplic_nodename(aplic_addr);
     g_autofree uint32_t *aplic_cells = g_new0(uint32_t, num_harts * 2);
     MachineState *ms = MACHINE(s);
+    static const char * const aplic_compat[2] = {
+        "qemu,aplic", "riscv,aplic"
+    };
 
     for (cpu = 0; cpu < num_harts; cpu++) {
         aplic_cells[cpu * 2 + 0] = cpu_to_be32(intc_phandles[cpu]);
@@ -613,7 +616,9 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
     }
 
     qemu_fdt_add_subnode(ms->fdt, aplic_name);
-    qemu_fdt_setprop_string(ms->fdt, aplic_name, "compatible", "riscv,aplic");
+    qemu_fdt_setprop_string_array(ms->fdt, aplic_name, "compatible",
+                                  (char **)&aplic_compat,
+                                  ARRAY_SIZE(aplic_compat));
     qemu_fdt_setprop_cell(ms->fdt, aplic_name, "#address-cells",
                           FDT_APLIC_ADDR_CELLS);
     qemu_fdt_setprop_cell(ms->fdt, aplic_name,
-- 
2.45.1



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

* [PATCH v2 5/8] hw/riscv/virt.c: aplic DT: rename prop to 'riscv, delegation'
  2024-05-31 20:27 [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes Daniel Henrique Barboza
                   ` (3 preceding siblings ...)
  2024-05-31 20:27 ` [PATCH v2 4/8] hw/riscv/virt.c: aplic DT: add 'qemu, aplic' to 'compatible' Daniel Henrique Barboza
@ 2024-05-31 20:27 ` Daniel Henrique Barboza
  2024-06-05  0:35   ` Alistair Francis
  2024-05-31 20:27 ` [PATCH v2 6/8] hw/riscv/virt.c: change imsic nodename to 'interrupt-controller' Daniel Henrique Barboza
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Daniel Henrique Barboza @ 2024-05-31 20:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-riscv, alistair.francis, bmeng, liwei1518, zhiwei_liu,
	palmer, apatel, ajones, conor, Daniel Henrique Barboza

The DT docs for riscv,aplic [1] predicts a 'riscv,delegation' property.
Not 'riscv,delegate'.

[1] Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml

Reported-by: Conor Dooley <conor@kernel.org>
Fixes: e6faee65855b ("hw/riscv: virt: Add optional AIA APLIC support to virt machine")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 366fe042cc..0a18547c6d 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -640,7 +640,7 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
     if (aplic_child_phandle) {
         qemu_fdt_setprop_cell(ms->fdt, aplic_name, "riscv,children",
                               aplic_child_phandle);
-        qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegate",
+        qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegation",
                                aplic_child_phandle, 0x1,
                                VIRT_IRQCHIP_NUM_SOURCES);
     }
-- 
2.45.1



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

* [PATCH v2 6/8] hw/riscv/virt.c: change imsic nodename to 'interrupt-controller'
  2024-05-31 20:27 [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes Daniel Henrique Barboza
                   ` (4 preceding siblings ...)
  2024-05-31 20:27 ` [PATCH v2 5/8] hw/riscv/virt.c: aplic DT: rename prop to 'riscv, delegation' Daniel Henrique Barboza
@ 2024-05-31 20:27 ` Daniel Henrique Barboza
  2024-06-05  0:35   ` Alistair Francis
  2024-05-31 20:27 ` [PATCH v2 7/8] hw/riscv/virt.c: imsics DT: add 'qemu, imsics' to 'compatible' Daniel Henrique Barboza
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Daniel Henrique Barboza @ 2024-05-31 20:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-riscv, alistair.francis, bmeng, liwei1518, zhiwei_liu,
	palmer, apatel, ajones, conor, Daniel Henrique Barboza

The Linux DT docs for imsic [1] predicts an 'interrupt-controller@addr'
node, not 'imsic@addr', given this node inherits the
'interrupt-controller' node.

[1] Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml

Reported-by: Conor Dooley <conor@kernel.org>
Fixes: 28d8c281200f ("hw/riscv: virt: Add optional AIA IMSIC support to virt machine")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 0a18547c6d..56d7e945c6 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -538,7 +538,8 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
         }
     }
 
-    imsic_name = g_strdup_printf("/soc/imsics@%lx", (unsigned long)base_addr);
+    imsic_name = g_strdup_printf("/soc/interrupt-controller@%lx",
+                                 (unsigned long)base_addr);
     qemu_fdt_add_subnode(ms->fdt, imsic_name);
     qemu_fdt_setprop_string(ms->fdt, imsic_name, "compatible", "riscv,imsics");
     qemu_fdt_setprop_cell(ms->fdt, imsic_name, "#interrupt-cells",
-- 
2.45.1



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

* [PATCH v2 7/8] hw/riscv/virt.c: imsics DT: add 'qemu, imsics' to 'compatible'
  2024-05-31 20:27 [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes Daniel Henrique Barboza
                   ` (5 preceding siblings ...)
  2024-05-31 20:27 ` [PATCH v2 6/8] hw/riscv/virt.c: change imsic nodename to 'interrupt-controller' Daniel Henrique Barboza
@ 2024-05-31 20:27 ` Daniel Henrique Barboza
  2024-06-05  0:35   ` Alistair Francis
  2024-05-31 20:27 ` [PATCH v2 8/8] hw/riscv/virt.c: imsics DT: add '#msi-cells' Daniel Henrique Barboza
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Daniel Henrique Barboza @ 2024-05-31 20:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-riscv, alistair.francis, bmeng, liwei1518, zhiwei_liu,
	palmer, apatel, ajones, conor, Daniel Henrique Barboza

The DT docs for riscv,imsics [1] predicts a 'qemu,imsics' enum in the
'compatible' property.

[1] Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml

Reported-by: Conor Dooley <conor@kernel.org>
Fixes: 28d8c281200f ("hw/riscv: virt: Add optional AIA IMSIC support to virt machine")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 56d7e945c6..ac70993679 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -515,6 +515,9 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
     uint32_t imsic_max_hart_per_socket, imsic_addr, imsic_size;
     g_autofree uint32_t *imsic_cells = NULL;
     g_autofree uint32_t *imsic_regs = NULL;
+    static const char * const imsic_compat[2] = {
+        "qemu,imsics", "riscv,imsics"
+    };
 
     imsic_cells = g_new0(uint32_t, ms->smp.cpus * 2);
     imsic_regs = g_new0(uint32_t, socket_count * 4);
@@ -541,7 +544,10 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
     imsic_name = g_strdup_printf("/soc/interrupt-controller@%lx",
                                  (unsigned long)base_addr);
     qemu_fdt_add_subnode(ms->fdt, imsic_name);
-    qemu_fdt_setprop_string(ms->fdt, imsic_name, "compatible", "riscv,imsics");
+    qemu_fdt_setprop_string_array(ms->fdt, imsic_name, "compatible",
+                                  (char **)&imsic_compat,
+                                  ARRAY_SIZE(imsic_compat));
+
     qemu_fdt_setprop_cell(ms->fdt, imsic_name, "#interrupt-cells",
                           FDT_IMSIC_INT_CELLS);
     qemu_fdt_setprop(ms->fdt, imsic_name, "interrupt-controller", NULL, 0);
-- 
2.45.1



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

* [PATCH v2 8/8] hw/riscv/virt.c: imsics DT: add '#msi-cells'
  2024-05-31 20:27 [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes Daniel Henrique Barboza
                   ` (6 preceding siblings ...)
  2024-05-31 20:27 ` [PATCH v2 7/8] hw/riscv/virt.c: imsics DT: add 'qemu, imsics' to 'compatible' Daniel Henrique Barboza
@ 2024-05-31 20:27 ` Daniel Henrique Barboza
  2024-06-05  0:36   ` Alistair Francis
  2024-06-04 11:28 ` [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes Conor Dooley
  2024-06-05  0:43 ` Alistair Francis
  9 siblings, 1 reply; 19+ messages in thread
From: Daniel Henrique Barboza @ 2024-05-31 20:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-riscv, alistair.francis, bmeng, liwei1518, zhiwei_liu,
	palmer, apatel, ajones, conor, Daniel Henrique Barboza

The DT docs for riscv,imsics [1] requires a 'msi-cell' property. Add one
and set it zero.

[1] Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml

Reported-by: Conor Dooley <conor@kernel.org>
Fixes: 28d8c281200f ("hw/riscv: virt: Add optional AIA IMSIC support to virt machine")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index ac70993679..8675c3a7d1 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -552,6 +552,7 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
                           FDT_IMSIC_INT_CELLS);
     qemu_fdt_setprop(ms->fdt, imsic_name, "interrupt-controller", NULL, 0);
     qemu_fdt_setprop(ms->fdt, imsic_name, "msi-controller", NULL, 0);
+    qemu_fdt_setprop_cell(ms->fdt, imsic_name, "#msi-cells", 0);
     qemu_fdt_setprop(ms->fdt, imsic_name, "interrupts-extended",
                      imsic_cells, ms->smp.cpus * sizeof(uint32_t) * 2);
     qemu_fdt_setprop(ms->fdt, imsic_name, "reg", imsic_regs,
-- 
2.45.1



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

* Re: [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes
  2024-05-31 20:27 [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes Daniel Henrique Barboza
                   ` (7 preceding siblings ...)
  2024-05-31 20:27 ` [PATCH v2 8/8] hw/riscv/virt.c: imsics DT: add '#msi-cells' Daniel Henrique Barboza
@ 2024-06-04 11:28 ` Conor Dooley
  2024-06-05  0:43 ` Alistair Francis
  9 siblings, 0 replies; 19+ messages in thread
From: Conor Dooley @ 2024-06-04 11:28 UTC (permalink / raw)
  To: Daniel Henrique Barboza
  Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liwei1518,
	zhiwei_liu, palmer, apatel, ajones

[-- Attachment #1: Type: text/plain, Size: 2794 bytes --]

On Fri, May 31, 2024 at 05:27:51PM -0300, Daniel Henrique Barboza wrote:
> Hi,
> 
> This is a series that is being spun from the reviews given on patch 1
> [1]. We'll fix some DT validation issues we have in the 'virt' machine
> [2] that aren't related to missing extensions in the DT spec.
> 
> I'll leave to maintainers to squash the patches as they see fit. I
> split it this way to make it easier to bissect possible bugs that these
> individual changes can cause.
> 
> These are the types of DT warnings solved by this series:
> 
> /home/danielhb/work/qemu/riscv64_virt.dtb: aplic@d000000: $nodename:0: 'aplic@d000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
> 	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> /home/danielhb/work/qemu/riscv64_virt.dtb: aplic@d000000: compatible:0: 'riscv,aplic' is not one of ['qemu,aplic']
> 	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> /home/danielhb/work/qemu/riscv64_virt.dtb: aplic@d000000: compatible: ['riscv,aplic'] is too short
> 	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> /home/danielhb/work/qemu/riscv64_virt.dtb: aplic@d000000: Unevaluated properties are not allowed ('compatible' was unexpected)
> 	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> (...)
> /home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: $nodename:0: 'imsics@28000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
> 	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
> /home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: compatible:0: 'riscv,imsics' is not one of ['qemu,imsics']
> 	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
> /home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: compatible: ['riscv,imsics'] is too short
> 	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
> /home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: '#msi-cells' is a required property
> 	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
> /home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: Unevaluated properties are not allowed ('compatible' was unexpected)
> 	from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#

I'm not sure that a "Tested-by: Conor Dooley <conor.dooley@microchip.com>"
here is really the right thing, given I only checked against the
bindings and didn't run a guest or anything of that sort, but all of the
validation issues relating to interrupt controllers are now gone.

Thanks for the fixes :)

Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

* Re: [PATCH v2 1/8] hw/riscv/virt.c: add address-cells in create_fdt_one_aplic()
  2024-05-31 20:27 ` [PATCH v2 1/8] hw/riscv/virt.c: add address-cells in create_fdt_one_aplic() Daniel Henrique Barboza
@ 2024-06-05  0:06   ` Alistair Francis
  0 siblings, 0 replies; 19+ messages in thread
From: Alistair Francis @ 2024-06-05  0:06 UTC (permalink / raw)
  To: Daniel Henrique Barboza
  Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liwei1518,
	zhiwei_liu, palmer, apatel, ajones, conor

On Sat, Jun 1, 2024 at 6:31 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We need #address-cells properties in all interrupt controllers that are
> referred by an interrupt-map [1]. For the RISC-V machine, both PLIC and
> APLIC controllers must have this property.
>
> PLIC already sets it in create_fdt_socket_plic(). Set the property for
> APLIC in create_fdt_one_aplic().
>
> [1] https://lore.kernel.org/linux-arm-kernel/CAL_JsqJE15D-xXxmELsmuD+JQHZzxGzdXvikChn6KFWqk6NzPw@mail.gmail.com/
>
> Suggested-by: Anup Patel <apatel@ventanamicro.com>
> Fixes: e6faee65855b ("hw/riscv: virt: Add optional AIA APLIC support to virt machine")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  hw/riscv/virt.c         | 2 ++
>  include/hw/riscv/virt.h | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 4fdb660525..1a7e1e73c5 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -609,6 +609,8 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
>      aplic_name = g_strdup_printf("/soc/aplic@%lx", aplic_addr);
>      qemu_fdt_add_subnode(ms->fdt, aplic_name);
>      qemu_fdt_setprop_string(ms->fdt, aplic_name, "compatible", "riscv,aplic");
> +    qemu_fdt_setprop_cell(ms->fdt, aplic_name, "#address-cells",
> +                          FDT_APLIC_ADDR_CELLS);
>      qemu_fdt_setprop_cell(ms->fdt, aplic_name,
>                            "#interrupt-cells", FDT_APLIC_INT_CELLS);
>      qemu_fdt_setprop(ms->fdt, aplic_name, "interrupt-controller", NULL, 0);
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index 3db839160f..c0dc41ff9a 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -118,6 +118,7 @@ enum {
>  #define FDT_PLIC_ADDR_CELLS   0
>  #define FDT_PLIC_INT_CELLS    1
>  #define FDT_APLIC_INT_CELLS   2
> +#define FDT_APLIC_ADDR_CELLS  0
>  #define FDT_IMSIC_INT_CELLS   0
>  #define FDT_MAX_INT_CELLS     2
>  #define FDT_MAX_INT_MAP_WIDTH (FDT_PCI_ADDR_CELLS + FDT_PCI_INT_CELLS + \
> --
> 2.45.1
>
>


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

* Re: [PATCH v2 2/8] hw/riscv/virt.c: add aplic nodename helper
  2024-05-31 20:27 ` [PATCH v2 2/8] hw/riscv/virt.c: add aplic nodename helper Daniel Henrique Barboza
@ 2024-06-05  0:18   ` Alistair Francis
  0 siblings, 0 replies; 19+ messages in thread
From: Alistair Francis @ 2024-06-05  0:18 UTC (permalink / raw)
  To: Daniel Henrique Barboza
  Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liwei1518,
	zhiwei_liu, palmer, apatel, ajones, conor

On Sat, Jun 1, 2024 at 6:31 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We'll change the aplic DT nodename in the next patch and the name is
> hardcoded in 2 different functions. Create a helper to change a single
> place later.
>
> While we're at it, in create_fdt_socket_aplic(), move 'aplic_name'
> inside the conditional to avoid allocating a string that won't be used
> when socket == NULL.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  hw/riscv/virt.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 1a7e1e73c5..07a07f5ce1 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -588,6 +588,12 @@ static void create_fdt_imsic(RISCVVirtState *s, const MemMapEntry *memmap,
>
>  }
>
> +/* Caller must free string after use */
> +static char *fdt_get_aplic_nodename(unsigned long aplic_addr)
> +{
> +    return g_strdup_printf("/soc/aplic@%lx", aplic_addr);
> +}
> +
>  static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
>                                   unsigned long aplic_addr, uint32_t aplic_size,
>                                   uint32_t msi_phandle,
> @@ -597,7 +603,7 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
>                                   bool m_mode, int num_harts)
>  {
>      int cpu;
> -    g_autofree char *aplic_name = NULL;
> +    g_autofree char *aplic_name = fdt_get_aplic_nodename(aplic_addr);
>      g_autofree uint32_t *aplic_cells = g_new0(uint32_t, num_harts * 2);
>      MachineState *ms = MACHINE(s);
>
> @@ -606,7 +612,6 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
>          aplic_cells[cpu * 2 + 1] = cpu_to_be32(m_mode ? IRQ_M_EXT : IRQ_S_EXT);
>      }
>
> -    aplic_name = g_strdup_printf("/soc/aplic@%lx", aplic_addr);
>      qemu_fdt_add_subnode(ms->fdt, aplic_name);
>      qemu_fdt_setprop_string(ms->fdt, aplic_name, "compatible", "riscv,aplic");
>      qemu_fdt_setprop_cell(ms->fdt, aplic_name, "#address-cells",
> @@ -648,7 +653,6 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
>                                      uint32_t *aplic_phandles,
>                                      int num_harts)
>  {
> -    g_autofree char *aplic_name = NULL;
>      unsigned long aplic_addr;
>      MachineState *ms = MACHINE(s);
>      uint32_t aplic_m_phandle, aplic_s_phandle;
> @@ -674,9 +678,8 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
>                           aplic_s_phandle, 0,
>                           false, num_harts);
>
> -    aplic_name = g_strdup_printf("/soc/aplic@%lx", aplic_addr);
> -
>      if (!socket) {
> +        g_autofree char *aplic_name = fdt_get_aplic_nodename(aplic_addr);
>          platform_bus_add_all_fdt_nodes(ms->fdt, aplic_name,
>                                         memmap[VIRT_PLATFORM_BUS].base,
>                                         memmap[VIRT_PLATFORM_BUS].size,
> --
> 2.45.1
>
>


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

* Re: [PATCH v2 3/8] hw/riscv/virt.c: rename aplic nodename to 'interrupt-controller'
  2024-05-31 20:27 ` [PATCH v2 3/8] hw/riscv/virt.c: rename aplic nodename to 'interrupt-controller' Daniel Henrique Barboza
@ 2024-06-05  0:30   ` Alistair Francis
  0 siblings, 0 replies; 19+ messages in thread
From: Alistair Francis @ 2024-06-05  0:30 UTC (permalink / raw)
  To: Daniel Henrique Barboza
  Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liwei1518,
	zhiwei_liu, palmer, apatel, ajones, conor

On Sat, Jun 1, 2024 at 6:30 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The correct name of the aplic controller node, as per Linux kernel DT
> docs [1], is 'interrupt-controller@addr'.
>
> [1] Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
>
> Reported-by: Conor Dooley <conor@kernel.org>
> Fixes: e6faee65855b ("hw/riscv: virt: Add optional AIA APLIC support to virt machine")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  hw/riscv/virt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 07a07f5ce1..5505047945 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -591,7 +591,7 @@ static void create_fdt_imsic(RISCVVirtState *s, const MemMapEntry *memmap,
>  /* Caller must free string after use */
>  static char *fdt_get_aplic_nodename(unsigned long aplic_addr)
>  {
> -    return g_strdup_printf("/soc/aplic@%lx", aplic_addr);
> +    return g_strdup_printf("/soc/interrupt-controller@%lx", aplic_addr);
>  }
>
>  static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
> --
> 2.45.1
>
>


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

* Re: [PATCH v2 4/8] hw/riscv/virt.c: aplic DT: add 'qemu, aplic' to 'compatible'
  2024-05-31 20:27 ` [PATCH v2 4/8] hw/riscv/virt.c: aplic DT: add 'qemu, aplic' to 'compatible' Daniel Henrique Barboza
@ 2024-06-05  0:31   ` Alistair Francis
  0 siblings, 0 replies; 19+ messages in thread
From: Alistair Francis @ 2024-06-05  0:31 UTC (permalink / raw)
  To: Daniel Henrique Barboza
  Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liwei1518,
	zhiwei_liu, palmer, apatel, ajones, conor

On Sat, Jun 1, 2024 at 6:31 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The DT docs for riscv,aplic [1] predicts a 'qemu,aplic' enum in the
> 'compatible' property.
>
> [1] Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
>
> Reported-by: Conor Dooley <conor@kernel.org>
> Fixes: e6faee65855b ("hw/riscv: virt: Add optional AIA APLIC support to virt machine")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  hw/riscv/virt.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 5505047945..366fe042cc 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -606,6 +606,9 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
>      g_autofree char *aplic_name = fdt_get_aplic_nodename(aplic_addr);
>      g_autofree uint32_t *aplic_cells = g_new0(uint32_t, num_harts * 2);
>      MachineState *ms = MACHINE(s);
> +    static const char * const aplic_compat[2] = {
> +        "qemu,aplic", "riscv,aplic"
> +    };
>
>      for (cpu = 0; cpu < num_harts; cpu++) {
>          aplic_cells[cpu * 2 + 0] = cpu_to_be32(intc_phandles[cpu]);
> @@ -613,7 +616,9 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
>      }
>
>      qemu_fdt_add_subnode(ms->fdt, aplic_name);
> -    qemu_fdt_setprop_string(ms->fdt, aplic_name, "compatible", "riscv,aplic");
> +    qemu_fdt_setprop_string_array(ms->fdt, aplic_name, "compatible",
> +                                  (char **)&aplic_compat,
> +                                  ARRAY_SIZE(aplic_compat));
>      qemu_fdt_setprop_cell(ms->fdt, aplic_name, "#address-cells",
>                            FDT_APLIC_ADDR_CELLS);
>      qemu_fdt_setprop_cell(ms->fdt, aplic_name,
> --
> 2.45.1
>
>


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

* Re: [PATCH v2 5/8] hw/riscv/virt.c: aplic DT: rename prop to 'riscv, delegation'
  2024-05-31 20:27 ` [PATCH v2 5/8] hw/riscv/virt.c: aplic DT: rename prop to 'riscv, delegation' Daniel Henrique Barboza
@ 2024-06-05  0:35   ` Alistair Francis
  0 siblings, 0 replies; 19+ messages in thread
From: Alistair Francis @ 2024-06-05  0:35 UTC (permalink / raw)
  To: Daniel Henrique Barboza
  Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liwei1518,
	zhiwei_liu, palmer, apatel, ajones, conor

On Sat, Jun 1, 2024 at 6:31 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The DT docs for riscv,aplic [1] predicts a 'riscv,delegation' property.
> Not 'riscv,delegate'.
>
> [1] Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
>
> Reported-by: Conor Dooley <conor@kernel.org>
> Fixes: e6faee65855b ("hw/riscv: virt: Add optional AIA APLIC support to virt machine")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  hw/riscv/virt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 366fe042cc..0a18547c6d 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -640,7 +640,7 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
>      if (aplic_child_phandle) {
>          qemu_fdt_setprop_cell(ms->fdt, aplic_name, "riscv,children",
>                                aplic_child_phandle);
> -        qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegate",
> +        qemu_fdt_setprop_cells(ms->fdt, aplic_name, "riscv,delegation",
>                                 aplic_child_phandle, 0x1,
>                                 VIRT_IRQCHIP_NUM_SOURCES);
>      }
> --
> 2.45.1
>
>


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

* Re: [PATCH v2 6/8] hw/riscv/virt.c: change imsic nodename to 'interrupt-controller'
  2024-05-31 20:27 ` [PATCH v2 6/8] hw/riscv/virt.c: change imsic nodename to 'interrupt-controller' Daniel Henrique Barboza
@ 2024-06-05  0:35   ` Alistair Francis
  0 siblings, 0 replies; 19+ messages in thread
From: Alistair Francis @ 2024-06-05  0:35 UTC (permalink / raw)
  To: Daniel Henrique Barboza
  Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liwei1518,
	zhiwei_liu, palmer, apatel, ajones, conor

On Sat, Jun 1, 2024 at 6:30 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The Linux DT docs for imsic [1] predicts an 'interrupt-controller@addr'
> node, not 'imsic@addr', given this node inherits the
> 'interrupt-controller' node.
>
> [1] Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
>
> Reported-by: Conor Dooley <conor@kernel.org>
> Fixes: 28d8c281200f ("hw/riscv: virt: Add optional AIA IMSIC support to virt machine")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  hw/riscv/virt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 0a18547c6d..56d7e945c6 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -538,7 +538,8 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
>          }
>      }
>
> -    imsic_name = g_strdup_printf("/soc/imsics@%lx", (unsigned long)base_addr);
> +    imsic_name = g_strdup_printf("/soc/interrupt-controller@%lx",
> +                                 (unsigned long)base_addr);
>      qemu_fdt_add_subnode(ms->fdt, imsic_name);
>      qemu_fdt_setprop_string(ms->fdt, imsic_name, "compatible", "riscv,imsics");
>      qemu_fdt_setprop_cell(ms->fdt, imsic_name, "#interrupt-cells",
> --
> 2.45.1
>
>


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

* Re: [PATCH v2 7/8] hw/riscv/virt.c: imsics DT: add 'qemu, imsics' to 'compatible'
  2024-05-31 20:27 ` [PATCH v2 7/8] hw/riscv/virt.c: imsics DT: add 'qemu, imsics' to 'compatible' Daniel Henrique Barboza
@ 2024-06-05  0:35   ` Alistair Francis
  0 siblings, 0 replies; 19+ messages in thread
From: Alistair Francis @ 2024-06-05  0:35 UTC (permalink / raw)
  To: Daniel Henrique Barboza
  Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liwei1518,
	zhiwei_liu, palmer, apatel, ajones, conor

On Sat, Jun 1, 2024 at 6:31 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The DT docs for riscv,imsics [1] predicts a 'qemu,imsics' enum in the
> 'compatible' property.
>
> [1] Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
>
> Reported-by: Conor Dooley <conor@kernel.org>
> Fixes: 28d8c281200f ("hw/riscv: virt: Add optional AIA IMSIC support to virt machine")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  hw/riscv/virt.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 56d7e945c6..ac70993679 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -515,6 +515,9 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
>      uint32_t imsic_max_hart_per_socket, imsic_addr, imsic_size;
>      g_autofree uint32_t *imsic_cells = NULL;
>      g_autofree uint32_t *imsic_regs = NULL;
> +    static const char * const imsic_compat[2] = {
> +        "qemu,imsics", "riscv,imsics"
> +    };
>
>      imsic_cells = g_new0(uint32_t, ms->smp.cpus * 2);
>      imsic_regs = g_new0(uint32_t, socket_count * 4);
> @@ -541,7 +544,10 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
>      imsic_name = g_strdup_printf("/soc/interrupt-controller@%lx",
>                                   (unsigned long)base_addr);
>      qemu_fdt_add_subnode(ms->fdt, imsic_name);
> -    qemu_fdt_setprop_string(ms->fdt, imsic_name, "compatible", "riscv,imsics");
> +    qemu_fdt_setprop_string_array(ms->fdt, imsic_name, "compatible",
> +                                  (char **)&imsic_compat,
> +                                  ARRAY_SIZE(imsic_compat));
> +
>      qemu_fdt_setprop_cell(ms->fdt, imsic_name, "#interrupt-cells",
>                            FDT_IMSIC_INT_CELLS);
>      qemu_fdt_setprop(ms->fdt, imsic_name, "interrupt-controller", NULL, 0);
> --
> 2.45.1
>
>


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

* Re: [PATCH v2 8/8] hw/riscv/virt.c: imsics DT: add '#msi-cells'
  2024-05-31 20:27 ` [PATCH v2 8/8] hw/riscv/virt.c: imsics DT: add '#msi-cells' Daniel Henrique Barboza
@ 2024-06-05  0:36   ` Alistair Francis
  0 siblings, 0 replies; 19+ messages in thread
From: Alistair Francis @ 2024-06-05  0:36 UTC (permalink / raw)
  To: Daniel Henrique Barboza
  Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liwei1518,
	zhiwei_liu, palmer, apatel, ajones, conor

On Sat, Jun 1, 2024 at 6:31 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The DT docs for riscv,imsics [1] requires a 'msi-cell' property. Add one
> and set it zero.
>
> [1] Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
>
> Reported-by: Conor Dooley <conor@kernel.org>
> Fixes: 28d8c281200f ("hw/riscv: virt: Add optional AIA IMSIC support to virt machine")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  hw/riscv/virt.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index ac70993679..8675c3a7d1 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -552,6 +552,7 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
>                            FDT_IMSIC_INT_CELLS);
>      qemu_fdt_setprop(ms->fdt, imsic_name, "interrupt-controller", NULL, 0);
>      qemu_fdt_setprop(ms->fdt, imsic_name, "msi-controller", NULL, 0);
> +    qemu_fdt_setprop_cell(ms->fdt, imsic_name, "#msi-cells", 0);
>      qemu_fdt_setprop(ms->fdt, imsic_name, "interrupts-extended",
>                       imsic_cells, ms->smp.cpus * sizeof(uint32_t) * 2);
>      qemu_fdt_setprop(ms->fdt, imsic_name, "reg", imsic_regs,
> --
> 2.45.1
>
>


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

* Re: [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes
  2024-05-31 20:27 [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes Daniel Henrique Barboza
                   ` (8 preceding siblings ...)
  2024-06-04 11:28 ` [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes Conor Dooley
@ 2024-06-05  0:43 ` Alistair Francis
  9 siblings, 0 replies; 19+ messages in thread
From: Alistair Francis @ 2024-06-05  0:43 UTC (permalink / raw)
  To: Daniel Henrique Barboza
  Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liwei1518,
	zhiwei_liu, palmer, apatel, ajones, conor

On Sat, Jun 1, 2024 at 6:30 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hi,
>
> This is a series that is being spun from the reviews given on patch 1
> [1]. We'll fix some DT validation issues we have in the 'virt' machine
> [2] that aren't related to missing extensions in the DT spec.
>
> I'll leave to maintainers to squash the patches as they see fit. I
> split it this way to make it easier to bissect possible bugs that these
> individual changes can cause.
>
> These are the types of DT warnings solved by this series:
>
> /home/danielhb/work/qemu/riscv64_virt.dtb: aplic@d000000: $nodename:0: 'aplic@d000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> /home/danielhb/work/qemu/riscv64_virt.dtb: aplic@d000000: compatible:0: 'riscv,aplic' is not one of ['qemu,aplic']
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> /home/danielhb/work/qemu/riscv64_virt.dtb: aplic@d000000: compatible: ['riscv,aplic'] is too short
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> /home/danielhb/work/qemu/riscv64_virt.dtb: aplic@d000000: Unevaluated properties are not allowed ('compatible' was unexpected)
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
> (...)
> /home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: $nodename:0: 'imsics@28000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
> /home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: compatible:0: 'riscv,imsics' is not one of ['qemu,imsics']
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
> /home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: compatible: ['riscv,imsics'] is too short
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
> /home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: '#msi-cells' is a required property
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
> /home/danielhb/work/qemu/riscv64_virt.dtb: imsics@28000000: Unevaluated properties are not allowed ('compatible' was unexpected)
>         from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
>
> [3] explains how to run 'dt-validate' to reproduce them. To generate a
> 'processed schema' file what I did was:
>
> - in the Linux kernel tree, run 'make dt_binding_check'. Please note
>   that this might require installation of additional python stuff
>   (e.g.swig, python3-devel)
>
> - I used the generated file 'Documentation/devicetree/bindings/processed-schema.json'
>   as a 'processed schema'.
>
> Series applicable on both master and alistair/riscv-to-apply.next.
>
> Changes from v1:
> - added patches 2 to 7 to fix the dt-validate warnings on imsics and
>   aplic notes
> - v1 link: https://lore.kernel.org/qemu-riscv/20240530084949.761034-1-dbarboza@ventanamicro.com/
>
> [1] https://lore.kernel.org/qemu-riscv/20240530084949.761034-1-dbarboza@ventanamicro.com/
> [2] https://lore.kernel.org/all/20240529-rust-tile-a05517a6260f@spud/
> [3] https://lore.kernel.org/qemu-riscv/20240530-landed-shriek-9362981afade@spud/
>
> Daniel Henrique Barboza (8):
>   hw/riscv/virt.c: add address-cells in create_fdt_one_aplic()
>   hw/riscv/virt.c: add aplic nodename helper
>   hw/riscv/virt.c: rename aplic nodename to 'interrupt-controller'
>   hw/riscv/virt.c: aplic DT: add 'qemu,aplic' to 'compatible'
>   hw/riscv/virt.c: aplic DT: rename prop to 'riscv,delegation'
>   hw/riscv/virt.c: change imsic nodename to 'interrupt-controller'
>   hw/riscv/virt.c: imsics DT: add 'qemu,imsics' to 'compatible'
>   hw/riscv/virt.c: imsics DT: add '#msi-cells'

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  hw/riscv/virt.c         | 36 +++++++++++++++++++++++++++---------
>  include/hw/riscv/virt.h |  1 +
>  2 files changed, 28 insertions(+), 9 deletions(-)
>
> --
> 2.45.1
>
>


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

end of thread, other threads:[~2024-06-05  0:44 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-31 20:27 [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes Daniel Henrique Barboza
2024-05-31 20:27 ` [PATCH v2 1/8] hw/riscv/virt.c: add address-cells in create_fdt_one_aplic() Daniel Henrique Barboza
2024-06-05  0:06   ` Alistair Francis
2024-05-31 20:27 ` [PATCH v2 2/8] hw/riscv/virt.c: add aplic nodename helper Daniel Henrique Barboza
2024-06-05  0:18   ` Alistair Francis
2024-05-31 20:27 ` [PATCH v2 3/8] hw/riscv/virt.c: rename aplic nodename to 'interrupt-controller' Daniel Henrique Barboza
2024-06-05  0:30   ` Alistair Francis
2024-05-31 20:27 ` [PATCH v2 4/8] hw/riscv/virt.c: aplic DT: add 'qemu, aplic' to 'compatible' Daniel Henrique Barboza
2024-06-05  0:31   ` Alistair Francis
2024-05-31 20:27 ` [PATCH v2 5/8] hw/riscv/virt.c: aplic DT: rename prop to 'riscv, delegation' Daniel Henrique Barboza
2024-06-05  0:35   ` Alistair Francis
2024-05-31 20:27 ` [PATCH v2 6/8] hw/riscv/virt.c: change imsic nodename to 'interrupt-controller' Daniel Henrique Barboza
2024-06-05  0:35   ` Alistair Francis
2024-05-31 20:27 ` [PATCH v2 7/8] hw/riscv/virt.c: imsics DT: add 'qemu, imsics' to 'compatible' Daniel Henrique Barboza
2024-06-05  0:35   ` Alistair Francis
2024-05-31 20:27 ` [PATCH v2 8/8] hw/riscv/virt.c: imsics DT: add '#msi-cells' Daniel Henrique Barboza
2024-06-05  0:36   ` Alistair Francis
2024-06-04 11:28 ` [PATCH v2 0/8] hw/riscv/virt.c: aplic/imsic DT fixes Conor Dooley
2024-06-05  0:43 ` Alistair Francis

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