| Message ID | 20240530084949.761034-1-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/riscv/virt.c: add address-cells in create_fdt_one_aplic() | expand |
On Thu, May 30, 2024 at 05:49:49AM GMT, Daniel Henrique Barboza 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/ There are other issues[2] with the DT nodes that we should address at the same time. [2] https://lore.kernel.org/all/20240529-rust-tile-a05517a6260f@spud/ Thanks, drew > > 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 > >
On Thu, May 30, 2024 at 01:05:41PM GMT, Andrew Jones wrote: > On Thu, May 30, 2024 at 05:49:49AM GMT, Daniel Henrique Barboza 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/ > > There are other issues[2] with the DT nodes that we should address at the > same time. > > [2] https://lore.kernel.org/all/20240529-rust-tile-a05517a6260f@spud/ I meant to CC Conor. Doing that now. > > Thanks, > drew > > > > > 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 > > > >
On 5/30/24 08:06, Andrew Jones wrote: > On Thu, May 30, 2024 at 01:05:41PM GMT, Andrew Jones wrote: >> On Thu, May 30, 2024 at 05:49:49AM GMT, Daniel Henrique Barboza 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/ >> >> There are other issues[2] with the DT nodes that we should address at the >> same time. >> >> [2] https://lore.kernel.org/all/20240529-rust-tile-a05517a6260f@spud/ > > I meant to CC Conor. Doing that now. I'll take a look at these other DT nodes issues. Conor, mind give me pointers on how do I reproduce the validation you did in [2]? Using upstream 'dtc' I have stuff like: ../qemu/qemu_dts.dts:261.4-68: Warning (interrupts_extended_property): /soc/aplic@d000000:interrupts-extended: cell 0 is not a phandle reference Which seems to also be an error but it's not what you reported. Are you using 'dt-validate' from dt-schema? Thanks, Daniel > >> >> Thanks, >> drew >> >>> >>> 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 >>> >>>
On Thu, May 30, 2024 at 09:26:58AM -0300, Daniel Henrique Barboza wrote: > > > On 5/30/24 08:06, Andrew Jones wrote: > > On Thu, May 30, 2024 at 01:05:41PM GMT, Andrew Jones wrote: > > > On Thu, May 30, 2024 at 05:49:49AM GMT, Daniel Henrique Barboza 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/ > > > > > > There are other issues[2] with the DT nodes that we should address at the > > > same time. > > > > > > [2] https://lore.kernel.org/all/20240529-rust-tile-a05517a6260f@spud/ > > > > I meant to CC Conor. Doing that now. > > I'll take a look at these other DT nodes issues. > > Conor, mind give me pointers on how do I reproduce the validation you did > in [2]? Using upstream 'dtc' I have stuff like: > > ../qemu/qemu_dts.dts:261.4-68: Warning (interrupts_extended_property): /soc/aplic@d000000:interrupts-extended: cell 0 is not a phandle reference > > Which seems to also be an error but it's not what you reported. Are you > using 'dt-validate' from dt-schema? Yeah, dt-validate. There's probably some stuff that I could add to my machine to make it more interesting, but I ran: $(qemu) -smp 4 -M virt,aia=aplic-imsic,dumpdtb=$(qemu_dtb) -cpu max -m 1G -nographic dt-validate --schema $(processed_schema) $(qemu_dtb) 2>&1 | tee logs/dtbdump.log A processed schema is a pre-requisite, and I usually have one sitting around from running dtbs_check or dt_binding_check in Linux, but I think you can use dt-rebasing to generate one either: https://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git/tree/Bindings/Makefile You'll see a bunch of noise from undocumented isa extensions, but at the end of the output should be the aplic complaints. I forgot I had it disabled to test something when I did that test the other day, there's also complaints about the imsics: qemu.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# qemu.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# qemu.dtb: imsics@28000000: compatible: ['riscv,imsics'] is too short from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml# qemu.dtb: imsics@28000000: '#msi-cells' is a required property from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml# qemu.dtb: imsics@28000000: Unevaluated properties are not allowed ('compatible' was unexpected) from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml# qemu.dtb: imsics@24000000: $nodename:0: 'imsics@24000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$' from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml# qemu.dtb: imsics@24000000: compatible:0: 'riscv,imsics' is not one of ['qemu,imsics'] from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml# qemu.dtb: imsics@24000000: compatible: ['riscv,imsics'] is too short from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml# qemu.dtb: imsics@24000000: '#msi-cells' is a required property from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml# qemu.dtb: imsics@24000000: Unevaluated properties are not allowed ('compatible' was unexpected) from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml# Cheers, Conor.
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 + \
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(+)