Re: [PATCH] hw/riscv/virt.c: add address-cells in create_fdt_one_aplic()

[Daniel Henrique Barboza]

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