* [PATCH] RISC-V: virt: This is a "sifive,test1" test finisher @ 2019-11-07 22:25 Palmer Dabbelt 2019-11-08 17:13 ` Alistair Francis 0 siblings, 1 reply; 9+ messages in thread From: Palmer Dabbelt @ 2019-11-07 22:25 UTC (permalink / raw) To: qemu-riscv; +Cc: Christoph Hellwig, Palmer Dabbelt, Palmer Dabbelt, qemu-devel The test finisher implements the reset command, which means it's a "sifive,test1" device. This is a backwards compatible change, so it's also a "sifive,test0" device. I copied the odd idiom for adding a two-string compatible field from the ARM virt board. Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality") Signed-off-by: Palmer Dabbelt <palmer@sifive.com> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com> --- hw/riscv/virt.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 23f340df19..74f2dce81c 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -359,7 +359,10 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap, nodename = g_strdup_printf("/test@%lx", (long)memmap[VIRT_TEST].base); qemu_fdt_add_subnode(fdt, nodename); - qemu_fdt_setprop_string(fdt, nodename, "compatible", "sifive,test0"); + { + const char compat[] = "sifive,test1\0sifive,test0"; + qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat)); + } qemu_fdt_setprop_cells(fdt, nodename, "reg", 0x0, memmap[VIRT_TEST].base, 0x0, memmap[VIRT_TEST].size); -- 2.21.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: virt: This is a "sifive,test1" test finisher 2019-11-07 22:25 [PATCH] RISC-V: virt: This is a "sifive,test1" test finisher Palmer Dabbelt @ 2019-11-08 17:13 ` Alistair Francis 2019-11-08 18:04 ` Peter Maydell 0 siblings, 1 reply; 9+ messages in thread From: Alistair Francis @ 2019-11-08 17:13 UTC (permalink / raw) To: Palmer Dabbelt Cc: Christoph Hellwig, Palmer Dabbelt, open list:RISC-V, qemu-devel@nongnu.org Developers On Fri, Nov 8, 2019 at 9:05 AM Palmer Dabbelt <palmer@sifive.com> wrote: > > The test finisher implements the reset command, which means it's a > "sifive,test1" device. This is a backwards compatible change, so it's > also a "sifive,test0" device. I copied the odd idiom for adding a > two-string compatible field from the ARM virt board. > > Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality") > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com> > --- > hw/riscv/virt.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index 23f340df19..74f2dce81c 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -359,7 +359,10 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap, > nodename = g_strdup_printf("/test@%lx", > (long)memmap[VIRT_TEST].base); > qemu_fdt_add_subnode(fdt, nodename); > - qemu_fdt_setprop_string(fdt, nodename, "compatible", "sifive,test0"); > + { > + const char compat[] = "sifive,test1\0sifive,test0"; Does this really work? Why not use qemu_fdt_setprop_cells()? Alistair > + qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat)); > + } > qemu_fdt_setprop_cells(fdt, nodename, "reg", > 0x0, memmap[VIRT_TEST].base, > 0x0, memmap[VIRT_TEST].size); > -- > 2.21.0 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: virt: This is a "sifive,test1" test finisher 2019-11-08 17:13 ` Alistair Francis @ 2019-11-08 18:04 ` Peter Maydell 2019-11-08 18:13 ` Palmer Dabbelt 2019-11-10 21:09 ` David Gibson 0 siblings, 2 replies; 9+ messages in thread From: Peter Maydell @ 2019-11-08 18:04 UTC (permalink / raw) To: Alistair Francis Cc: open list:RISC-V, Palmer Dabbelt, qemu-devel@nongnu.org Developers, Christoph Hellwig, Palmer Dabbelt, David Gibson On Fri, 8 Nov 2019 at 17:15, Alistair Francis <alistair23@gmail.com> wrote: > > On Fri, Nov 8, 2019 at 9:05 AM Palmer Dabbelt <palmer@sifive.com> wrote: > > > > The test finisher implements the reset command, which means it's a > > "sifive,test1" device. This is a backwards compatible change, so it's > > also a "sifive,test0" device. I copied the odd idiom for adding a > > two-string compatible field from the ARM virt board. > > > > Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality") > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com> > > --- > > hw/riscv/virt.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > > index 23f340df19..74f2dce81c 100644 > > --- a/hw/riscv/virt.c > > +++ b/hw/riscv/virt.c > > @@ -359,7 +359,10 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap, > > nodename = g_strdup_printf("/test@%lx", > > (long)memmap[VIRT_TEST].base); > > qemu_fdt_add_subnode(fdt, nodename); > > - qemu_fdt_setprop_string(fdt, nodename, "compatible", "sifive,test0"); > > + { > > + const char compat[] = "sifive,test1\0sifive,test0"; > > Does this really work? Why not use qemu_fdt_setprop_cells()? > > Alistair > > > + qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat)); > > + } qemu_fdt_setprop_cells() is for "set this property to contain this list of 32-bit integers" (and it does a byteswap of each 32-bit value from host to BE). That's not what you want for a string (or a string list, which is what we have here). Cc'ing David Gibson who's our device tree expert to see if there's a nicer way to write this. Oddly, given that it's used in the ubiquitous 'compatible' prop, the dtc Documentation/manual.txt doesn't say anything about properties being able to be 'string lists', only 'strings', '32 bit numbers', 'lists of 32-bit numbers' and 'byte sequences'. You have to dig through the header file comments to deduce that a string list is represented by a string with embedded NULs separating each list item. thanks -- PMM ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: virt: This is a "sifive,test1" test finisher 2019-11-08 18:04 ` Peter Maydell @ 2019-11-08 18:13 ` Palmer Dabbelt 2019-11-10 21:10 ` David Gibson 2019-11-10 21:09 ` David Gibson 1 sibling, 1 reply; 9+ messages in thread From: Palmer Dabbelt @ 2019-11-08 18:13 UTC (permalink / raw) To: Peter Maydell Cc: qemu-riscv, palmer, qemu-devel, Christoph Hellwig, alistair23, david On Fri, 08 Nov 2019 10:04:47 PST (-0800), Peter Maydell wrote: > On Fri, 8 Nov 2019 at 17:15, Alistair Francis <alistair23@gmail.com> wrote: >> >> On Fri, Nov 8, 2019 at 9:05 AM Palmer Dabbelt <palmer@sifive.com> wrote: >> > >> > The test finisher implements the reset command, which means it's a >> > "sifive,test1" device. This is a backwards compatible change, so it's >> > also a "sifive,test0" device. I copied the odd idiom for adding a >> > two-string compatible field from the ARM virt board. >> > >> > Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality") >> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> >> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com> >> > --- >> > hw/riscv/virt.c | 5 ++++- >> > 1 file changed, 4 insertions(+), 1 deletion(-) >> > >> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c >> > index 23f340df19..74f2dce81c 100644 >> > --- a/hw/riscv/virt.c >> > +++ b/hw/riscv/virt.c >> > @@ -359,7 +359,10 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap, >> > nodename = g_strdup_printf("/test@%lx", >> > (long)memmap[VIRT_TEST].base); >> > qemu_fdt_add_subnode(fdt, nodename); >> > - qemu_fdt_setprop_string(fdt, nodename, "compatible", "sifive,test0"); >> > + { >> > + const char compat[] = "sifive,test1\0sifive,test0"; >> >> Does this really work? Why not use qemu_fdt_setprop_cells()? >> >> Alistair >> >> > + qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat)); >> > + } > > qemu_fdt_setprop_cells() is for "set this property to > contain this list of 32-bit integers" (and it does a byteswap > of each 32-bit value from host to BE). That's not what > you want for a string (or a string list, which is what > we have here). > > Cc'ing David Gibson who's our device tree expert to see if there's > a nicer way to write this. Oddly, given that it's used in the > ubiquitous 'compatible' prop, the dtc Documentation/manual.txt > doesn't say anything about properties being able to be > 'string lists', only 'strings', '32 bit numbers', 'lists of > 32-bit numbers' and 'byte sequences'. You have to dig through > the header file comments to deduce that a string list is > represented by a string with embedded NULs separating > each list item. I copied this from hw/arm/virt.c, but messed up. There they use const char compat[] = "arm,armv8-timer\0arm,armv7-timer"; qemu_fdt_setprop(vms->fdt, "/timer", "compatible", compat, sizeof(compat)); I'll send a v2, but I'd be happy to add some sort of setprop_stringlist function. Maybe we just indicate the length with two '\0's? IIRC that's how other similar-looking data structures are encoded. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: virt: This is a "sifive,test1" test finisher 2019-11-08 18:13 ` Palmer Dabbelt @ 2019-11-10 21:10 ` David Gibson 2019-11-21 2:40 ` Palmer Dabbelt 0 siblings, 1 reply; 9+ messages in thread From: David Gibson @ 2019-11-10 21:10 UTC (permalink / raw) To: Palmer Dabbelt Cc: Peter Maydell, qemu-riscv, palmer, qemu-devel, Christoph Hellwig, alistair23 [-- Attachment #1: Type: text/plain, Size: 3340 bytes --] On Fri, Nov 08, 2019 at 10:13:16AM -0800, Palmer Dabbelt wrote: > On Fri, 08 Nov 2019 10:04:47 PST (-0800), Peter Maydell wrote: > > On Fri, 8 Nov 2019 at 17:15, Alistair Francis <alistair23@gmail.com> wrote: > > > > > > On Fri, Nov 8, 2019 at 9:05 AM Palmer Dabbelt <palmer@sifive.com> wrote: > > > > > > > > The test finisher implements the reset command, which means it's a > > > > "sifive,test1" device. This is a backwards compatible change, so it's > > > > also a "sifive,test0" device. I copied the odd idiom for adding a > > > > two-string compatible field from the ARM virt board. > > > > > > > > Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality") > > > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > > > > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com> > > > > --- > > > > hw/riscv/virt.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > > > > index 23f340df19..74f2dce81c 100644 > > > > --- a/hw/riscv/virt.c > > > > +++ b/hw/riscv/virt.c > > > > @@ -359,7 +359,10 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap, > > > > nodename = g_strdup_printf("/test@%lx", > > > > (long)memmap[VIRT_TEST].base); > > > > qemu_fdt_add_subnode(fdt, nodename); > > > > - qemu_fdt_setprop_string(fdt, nodename, "compatible", "sifive,test0"); > > > > + { > > > > + const char compat[] = "sifive,test1\0sifive,test0"; > > > > > > Does this really work? Why not use qemu_fdt_setprop_cells()? > > > > > > Alistair > > > > > > > + qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat)); > > > > + } > > > > qemu_fdt_setprop_cells() is for "set this property to > > contain this list of 32-bit integers" (and it does a byteswap > > of each 32-bit value from host to BE). That's not what > > you want for a string (or a string list, which is what > > we have here). > > > > Cc'ing David Gibson who's our device tree expert to see if there's > > a nicer way to write this. Oddly, given that it's used in the > > ubiquitous 'compatible' prop, the dtc Documentation/manual.txt > > doesn't say anything about properties being able to be > > 'string lists', only 'strings', '32 bit numbers', 'lists of > > 32-bit numbers' and 'byte sequences'. You have to dig through > > the header file comments to deduce that a string list is > > represented by a string with embedded NULs separating > > each list item. > > I copied this from hw/arm/virt.c, but messed up. There they use > > const char compat[] = "arm,armv8-timer\0arm,armv7-timer"; > qemu_fdt_setprop(vms->fdt, "/timer", "compatible", > compat, sizeof(compat)); I'm not sure what you're saying is messed up. AFAICT, this matches the code you have above, and both should be correct. > I'll send a v2, but I'd be happy to add some sort of setprop_stringlist > function. Maybe we just indicate the length with two '\0's? IIRC that's > how other similar-looking data structures are encoded. > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: virt: This is a "sifive,test1" test finisher 2019-11-10 21:10 ` David Gibson @ 2019-11-21 2:40 ` Palmer Dabbelt 2019-11-21 18:55 ` Alistair Francis 0 siblings, 1 reply; 9+ messages in thread From: Palmer Dabbelt @ 2019-11-21 2:40 UTC (permalink / raw) To: david Cc: Peter Maydell, alistair23, palmer, Christoph Hellwig, qemu-riscv, qemu-devel On Sun, 10 Nov 2019 13:10:33 PST (-0800), david@gibson.dropbear.id.au wrote: > On Fri, Nov 08, 2019 at 10:13:16AM -0800, Palmer Dabbelt wrote: >> On Fri, 08 Nov 2019 10:04:47 PST (-0800), Peter Maydell wrote: >> > On Fri, 8 Nov 2019 at 17:15, Alistair Francis <alistair23@gmail.com> wrote: >> > > >> > > On Fri, Nov 8, 2019 at 9:05 AM Palmer Dabbelt <palmer@sifive.com> wrote: >> > > > >> > > > The test finisher implements the reset command, which means it's a >> > > > "sifive,test1" device. This is a backwards compatible change, so it's >> > > > also a "sifive,test0" device. I copied the odd idiom for adding a >> > > > two-string compatible field from the ARM virt board. >> > > > >> > > > Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality") >> > > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> >> > > > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com> >> > > > --- >> > > > hw/riscv/virt.c | 5 ++++- >> > > > 1 file changed, 4 insertions(+), 1 deletion(-) >> > > > >> > > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c >> > > > index 23f340df19..74f2dce81c 100644 >> > > > --- a/hw/riscv/virt.c >> > > > +++ b/hw/riscv/virt.c >> > > > @@ -359,7 +359,10 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap, >> > > > nodename = g_strdup_printf("/test@%lx", >> > > > (long)memmap[VIRT_TEST].base); >> > > > qemu_fdt_add_subnode(fdt, nodename); >> > > > - qemu_fdt_setprop_string(fdt, nodename, "compatible", "sifive,test0"); >> > > > + { >> > > > + const char compat[] = "sifive,test1\0sifive,test0"; >> > > >> > > Does this really work? Why not use qemu_fdt_setprop_cells()? >> > > >> > > Alistair >> > > >> > > > + qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat)); >> > > > + } >> > >> > qemu_fdt_setprop_cells() is for "set this property to >> > contain this list of 32-bit integers" (and it does a byteswap >> > of each 32-bit value from host to BE). That's not what >> > you want for a string (or a string list, which is what >> > we have here). >> > >> > Cc'ing David Gibson who's our device tree expert to see if there's >> > a nicer way to write this. Oddly, given that it's used in the >> > ubiquitous 'compatible' prop, the dtc Documentation/manual.txt >> > doesn't say anything about properties being able to be >> > 'string lists', only 'strings', '32 bit numbers', 'lists of >> > 32-bit numbers' and 'byte sequences'. You have to dig through >> > the header file comments to deduce that a string list is >> > represented by a string with embedded NULs separating >> > each list item. >> >> I copied this from hw/arm/virt.c, but messed up. There they use >> >> const char compat[] = "arm,armv8-timer\0arm,armv7-timer"; >> qemu_fdt_setprop(vms->fdt, "/timer", "compatible", >> compat, sizeof(compat)); > > I'm not sure what you're saying is messed up. AFAICT, this matches > the code you have above, and both should be correct. Sorry, I must have been hallucinating. For some reason I though I wrote qemu_fdt_setprop_string(... compat). I'd like to take this for 4.2 if possible, but I don't think I have a reviewed-by (I just got my email set up on my Google computer, so it might be messy for a bit). I'm happy to submit the cleaner valist version after 4.2, as per Peter's suggestion. Alistair: are you OK with this? >> I'll send a v2, but I'd be happy to add some sort of setprop_stringlist >> function. Maybe we just indicate the length with two '\0's? IIRC that's >> how other similar-looking data structures are encoded. >> > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: virt: This is a "sifive,test1" test finisher 2019-11-21 2:40 ` Palmer Dabbelt @ 2019-11-21 18:55 ` Alistair Francis 2019-11-21 19:06 ` Palmer Dabbelt 0 siblings, 1 reply; 9+ messages in thread From: Alistair Francis @ 2019-11-21 18:55 UTC (permalink / raw) To: Palmer Dabbelt Cc: Peter Maydell, open list:RISC-V, Palmer Dabbelt, qemu-devel@nongnu.org Developers, Christoph Hellwig, David Gibson On Wed, Nov 20, 2019 at 6:40 PM Palmer Dabbelt <palmerdabbelt@google.com> wrote: > > On Sun, 10 Nov 2019 13:10:33 PST (-0800), david@gibson.dropbear.id.au wrote: > > On Fri, Nov 08, 2019 at 10:13:16AM -0800, Palmer Dabbelt wrote: > >> On Fri, 08 Nov 2019 10:04:47 PST (-0800), Peter Maydell wrote: > >> > On Fri, 8 Nov 2019 at 17:15, Alistair Francis <alistair23@gmail.com> wrote: > >> > > > >> > > On Fri, Nov 8, 2019 at 9:05 AM Palmer Dabbelt <palmer@sifive.com> wrote: > >> > > > > >> > > > The test finisher implements the reset command, which means it's a > >> > > > "sifive,test1" device. This is a backwards compatible change, so it's > >> > > > also a "sifive,test0" device. I copied the odd idiom for adding a > >> > > > two-string compatible field from the ARM virt board. > >> > > > > >> > > > Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality") > >> > > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > >> > > > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com> > >> > > > --- > >> > > > hw/riscv/virt.c | 5 ++++- > >> > > > 1 file changed, 4 insertions(+), 1 deletion(-) > >> > > > > >> > > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > >> > > > index 23f340df19..74f2dce81c 100644 > >> > > > --- a/hw/riscv/virt.c > >> > > > +++ b/hw/riscv/virt.c > >> > > > @@ -359,7 +359,10 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap, > >> > > > nodename = g_strdup_printf("/test@%lx", > >> > > > (long)memmap[VIRT_TEST].base); > >> > > > qemu_fdt_add_subnode(fdt, nodename); > >> > > > - qemu_fdt_setprop_string(fdt, nodename, "compatible", "sifive,test0"); > >> > > > + { > >> > > > + const char compat[] = "sifive,test1\0sifive,test0"; > >> > > > >> > > Does this really work? Why not use qemu_fdt_setprop_cells()? > >> > > > >> > > Alistair > >> > > > >> > > > + qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat)); > >> > > > + } > >> > > >> > qemu_fdt_setprop_cells() is for "set this property to > >> > contain this list of 32-bit integers" (and it does a byteswap > >> > of each 32-bit value from host to BE). That's not what > >> > you want for a string (or a string list, which is what > >> > we have here). > >> > > >> > Cc'ing David Gibson who's our device tree expert to see if there's > >> > a nicer way to write this. Oddly, given that it's used in the > >> > ubiquitous 'compatible' prop, the dtc Documentation/manual.txt > >> > doesn't say anything about properties being able to be > >> > 'string lists', only 'strings', '32 bit numbers', 'lists of > >> > 32-bit numbers' and 'byte sequences'. You have to dig through > >> > the header file comments to deduce that a string list is > >> > represented by a string with embedded NULs separating > >> > each list item. > >> > >> I copied this from hw/arm/virt.c, but messed up. There they use > >> > >> const char compat[] = "arm,armv8-timer\0arm,armv7-timer"; > >> qemu_fdt_setprop(vms->fdt, "/timer", "compatible", > >> compat, sizeof(compat)); > > > > I'm not sure what you're saying is messed up. AFAICT, this matches > > the code you have above, and both should be correct. > > Sorry, I must have been hallucinating. For some reason I though I wrote > qemu_fdt_setprop_string(... compat). > > I'd like to take this for 4.2 if possible, but I don't think I have a > reviewed-by (I just got my email set up on my Google computer, so it might be > messy for a bit). I'm happy to submit the cleaner valist version after 4.2, as > per Peter's suggestion. > > Alistair: are you OK with this? Yeah, that works for me. For 5.0 we can then merge Anup's patch and your series improving the multi compat support. Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > > >> I'll send a v2, but I'd be happy to add some sort of setprop_stringlist > >> function. Maybe we just indicate the length with two '\0's? IIRC that's > >> how other similar-looking data structures are encoded. > >> > > > > -- > > David Gibson | I'll have my music baroque, and my code > > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > > | _way_ _around_! > > http://www.ozlabs.org/~dgibson ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: virt: This is a "sifive,test1" test finisher 2019-11-21 18:55 ` Alistair Francis @ 2019-11-21 19:06 ` Palmer Dabbelt 0 siblings, 0 replies; 9+ messages in thread From: Palmer Dabbelt @ 2019-11-21 19:06 UTC (permalink / raw) To: alistair23 Cc: david, Peter Maydell, palmer, Christoph Hellwig, qemu-riscv, qemu-devel On Thu, 21 Nov 2019 10:55:32 PST (-0800), alistair23@gmail.com wrote: > On Wed, Nov 20, 2019 at 6:40 PM Palmer Dabbelt <palmerdabbelt@google.com> wrote: >> >> On Sun, 10 Nov 2019 13:10:33 PST (-0800), david@gibson.dropbear.id.au wrote: >> > On Fri, Nov 08, 2019 at 10:13:16AM -0800, Palmer Dabbelt wrote: >> >> On Fri, 08 Nov 2019 10:04:47 PST (-0800), Peter Maydell wrote: >> >> > On Fri, 8 Nov 2019 at 17:15, Alistair Francis <alistair23@gmail.com> wrote: >> >> > > >> >> > > On Fri, Nov 8, 2019 at 9:05 AM Palmer Dabbelt <palmer@sifive.com> wrote: >> >> > > > >> >> > > > The test finisher implements the reset command, which means it's a >> >> > > > "sifive,test1" device. This is a backwards compatible change, so it's >> >> > > > also a "sifive,test0" device. I copied the odd idiom for adding a >> >> > > > two-string compatible field from the ARM virt board. >> >> > > > >> >> > > > Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality") >> >> > > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> >> >> > > > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com> >> >> > > > --- >> >> > > > hw/riscv/virt.c | 5 ++++- >> >> > > > 1 file changed, 4 insertions(+), 1 deletion(-) >> >> > > > >> >> > > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c >> >> > > > index 23f340df19..74f2dce81c 100644 >> >> > > > --- a/hw/riscv/virt.c >> >> > > > +++ b/hw/riscv/virt.c >> >> > > > @@ -359,7 +359,10 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap, >> >> > > > nodename = g_strdup_printf("/test@%lx", >> >> > > > (long)memmap[VIRT_TEST].base); >> >> > > > qemu_fdt_add_subnode(fdt, nodename); >> >> > > > - qemu_fdt_setprop_string(fdt, nodename, "compatible", "sifive,test0"); >> >> > > > + { >> >> > > > + const char compat[] = "sifive,test1\0sifive,test0"; >> >> > > >> >> > > Does this really work? Why not use qemu_fdt_setprop_cells()? >> >> > > >> >> > > Alistair >> >> > > >> >> > > > + qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat)); >> >> > > > + } >> >> > >> >> > qemu_fdt_setprop_cells() is for "set this property to >> >> > contain this list of 32-bit integers" (and it does a byteswap >> >> > of each 32-bit value from host to BE). That's not what >> >> > you want for a string (or a string list, which is what >> >> > we have here). >> >> > >> >> > Cc'ing David Gibson who's our device tree expert to see if there's >> >> > a nicer way to write this. Oddly, given that it's used in the >> >> > ubiquitous 'compatible' prop, the dtc Documentation/manual.txt >> >> > doesn't say anything about properties being able to be >> >> > 'string lists', only 'strings', '32 bit numbers', 'lists of >> >> > 32-bit numbers' and 'byte sequences'. You have to dig through >> >> > the header file comments to deduce that a string list is >> >> > represented by a string with embedded NULs separating >> >> > each list item. >> >> >> >> I copied this from hw/arm/virt.c, but messed up. There they use >> >> >> >> const char compat[] = "arm,armv8-timer\0arm,armv7-timer"; >> >> qemu_fdt_setprop(vms->fdt, "/timer", "compatible", >> >> compat, sizeof(compat)); >> > >> > I'm not sure what you're saying is messed up. AFAICT, this matches >> > the code you have above, and both should be correct. >> >> Sorry, I must have been hallucinating. For some reason I though I wrote >> qemu_fdt_setprop_string(... compat). >> >> I'd like to take this for 4.2 if possible, but I don't think I have a >> reviewed-by (I just got my email set up on my Google computer, so it might be >> messy for a bit). I'm happy to submit the cleaner valist version after 4.2, as >> per Peter's suggestion. >> >> Alistair: are you OK with this? > > Yeah, that works for me. > > For 5.0 we can then merge Anup's patch and your series improving the > multi compat support. Sounds good. I think the syscon driver is the right way to go, but I don't think it's 4.2 material as it's all a bit late. This one at least fixes the bug where the DTS and hardware don't match. > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > Alistair > >> >> >> I'll send a v2, but I'd be happy to add some sort of setprop_stringlist >> >> function. Maybe we just indicate the length with two '\0's? IIRC that's >> >> how other similar-looking data structures are encoded. >> >> >> > >> > -- >> > David Gibson | I'll have my music baroque, and my code >> > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ >> > | _way_ _around_! >> > http://www.ozlabs.org/~dgibson ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: virt: This is a "sifive,test1" test finisher 2019-11-08 18:04 ` Peter Maydell 2019-11-08 18:13 ` Palmer Dabbelt @ 2019-11-10 21:09 ` David Gibson 1 sibling, 0 replies; 9+ messages in thread From: David Gibson @ 2019-11-10 21:09 UTC (permalink / raw) To: Peter Maydell Cc: open list:RISC-V, Palmer Dabbelt, qemu-devel@nongnu.org Developers, Christoph Hellwig, Palmer Dabbelt, Alistair Francis [-- Attachment #1: Type: text/plain, Size: 3494 bytes --] On Fri, Nov 08, 2019 at 06:04:47PM +0000, Peter Maydell wrote: > On Fri, 8 Nov 2019 at 17:15, Alistair Francis <alistair23@gmail.com> wrote: > > > > On Fri, Nov 8, 2019 at 9:05 AM Palmer Dabbelt <palmer@sifive.com> wrote: > > > > > > The test finisher implements the reset command, which means it's a > > > "sifive,test1" device. This is a backwards compatible change, so it's > > > also a "sifive,test0" device. I copied the odd idiom for adding a > > > two-string compatible field from the ARM virt board. > > > > > > Fixes: 9a2551ed6f ("riscv: sifive_test: Add reset functionality") > > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > > > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com> > > > --- > > > hw/riscv/virt.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > > > index 23f340df19..74f2dce81c 100644 > > > --- a/hw/riscv/virt.c > > > +++ b/hw/riscv/virt.c > > > @@ -359,7 +359,10 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap, > > > nodename = g_strdup_printf("/test@%lx", > > > (long)memmap[VIRT_TEST].base); > > > qemu_fdt_add_subnode(fdt, nodename); > > > - qemu_fdt_setprop_string(fdt, nodename, "compatible", "sifive,test0"); > > > + { > > > + const char compat[] = "sifive,test1\0sifive,test0"; > > > > Does this really work? Why not use qemu_fdt_setprop_cells()? > > > > Alistair > > > > > + qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat)); > > > + } > > qemu_fdt_setprop_cells() is for "set this property to > contain this list of 32-bit integers" (and it does a byteswap > of each 32-bit value from host to BE). That's not what > you want for a string (or a string list, which is what > we have here). Yes. > Cc'ing David Gibson who's our device tree expert to see if there's > a nicer way to write this. Not amongst the qemu wrappers AFAIK. With raw libfdt you could build it up in stages using fdt_setprop_string() followed by fdt_appendprop_string(), although that would require additional memmove()s to complete. A stringlist setprop function using varargs might be a useful addition to libfdt. The tricky bit is that libfdt can be used in environments with no allocator, which makes implementation tricky. > Oddly, given that it's used in the > ubiquitous 'compatible' prop, the dtc Documentation/manual.txt > doesn't say anything about properties being able to be > 'string lists', only 'strings', '32 bit numbers', 'lists of > 32-bit numbers' and 'byte sequences'. Oof, yeah that manual hasn't really been updated for ages. That really only describes the pieces from which a property can be mode. You can then concatenate those together using ,. So in .dts you can make a string list as: compatible = "foo", "bar"; The , is effectively a bytestring append operator. > You have to dig through > the header file comments to deduce that a string list is > represented by a string with embedded NULs separating > each list item. Well, you can do that as well, but using commas is more common in modern dts files. The two are equivalent by construction (in .dtb properties are simply bytestrings). -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-11-21 19:06 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-07 22:25 [PATCH] RISC-V: virt: This is a "sifive,test1" test finisher Palmer Dabbelt 2019-11-08 17:13 ` Alistair Francis 2019-11-08 18:04 ` Peter Maydell 2019-11-08 18:13 ` Palmer Dabbelt 2019-11-10 21:10 ` David Gibson 2019-11-21 2:40 ` Palmer Dabbelt 2019-11-21 18:55 ` Alistair Francis 2019-11-21 19:06 ` Palmer Dabbelt 2019-11-10 21:09 ` David Gibson
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).