Message ID | 20191019234715.25750-1-f4bug@amsat.org |
---|---|
Headers | show |
Series | hw/arm/raspi: Add thermal/timer, improve address space, run U-boot | expand |
On Sun, 20 Oct 2019 at 00:47, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > Since v2: > - fixed issue in videocore address space > - allow to start with some cores OFF (to boot firmwares) > - add proof-of-concept test for '-smp cores=1' and U-boot > - fixed my email setup > > Previous cover: > > Hi, > > Some patches from v1 are already merged. This v2 addresses the > review comment from v1, and add patches to clean the memory > space when using multiple cores. > > Laurent, if you test U-Boot with this patchset again, do you mind > replying with a "Tested-by:" tag? > > The next patchset is probably about the interrupt controller blocks, > then will come another one about the MBox/Properties. > > The last patch is unrelated to the series, but since I cleaned this > for the raspi and the highbank is the only board with the same issue, > I included the patch in this series. I'm going to apply 1-10 and 14 to target-arm.next. (I've reviewed 10, and the rest have been reviewed.) thanks -- PMM
On 10/24/19 3:42 PM, Peter Maydell wrote: > On Sun, 20 Oct 2019 at 00:47, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> Since v2: >> - fixed issue in videocore address space >> - allow to start with some cores OFF (to boot firmwares) >> - add proof-of-concept test for '-smp cores=1' and U-boot >> - fixed my email setup >> >> Previous cover: >> >> Hi, >> >> Some patches from v1 are already merged. This v2 addresses the >> review comment from v1, and add patches to clean the memory >> space when using multiple cores. >> >> Laurent, if you test U-Boot with this patchset again, do you mind >> replying with a "Tested-by:" tag? >> >> The next patchset is probably about the interrupt controller blocks, >> then will come another one about the MBox/Properties. >> >> The last patch is unrelated to the series, but since I cleaned this >> for the raspi and the highbank is the only board with the same issue, >> I included the patch in this series. > > I'm going to apply 1-10 and 14 to target-arm.next. > (I've reviewed 10, and the rest have been reviewed.) Thanks! Do you mind amending this to patch #3 "hw/timer/bcm2835: Add the BCM2835 SYS_timer" or should I respin (or resend it alone)? -- >8 -- diff --git a/hw/timer/bcm2835_systmr.c b/hw/timer/bcm2835_systmr.c index 49b40b55f9..3387a6214a 100644 --- a/hw/timer/bcm2835_systmr.c +++ b/hw/timer/bcm2835_systmr.c @@ -115,10 +115,7 @@ static void bcm2835_systmr_reset(DeviceState *dev) { BCM2835SystemTimerState *s = BCM2835_SYSTIMER(dev); - s->reg.status = 0; - for (size_t i = 0; i < ARRAY_SIZE(s->reg.compare); i++) { - s->reg.compare[i] = 0; - } + memset(&s->reg, 0, sizeof(s->reg)); } --- Thanks, Phil.
On Thu, 24 Oct 2019 at 14:46, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 10/24/19 3:42 PM, Peter Maydell wrote: > > On Sun, 20 Oct 2019 at 00:47, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >> > >> Since v2: > >> - fixed issue in videocore address space > >> - allow to start with some cores OFF (to boot firmwares) > >> - add proof-of-concept test for '-smp cores=1' and U-boot > >> - fixed my email setup > >> > >> Previous cover: > >> > >> Hi, > >> > >> Some patches from v1 are already merged. This v2 addresses the > >> review comment from v1, and add patches to clean the memory > >> space when using multiple cores. > >> > >> Laurent, if you test U-Boot with this patchset again, do you mind > >> replying with a "Tested-by:" tag? > >> > >> The next patchset is probably about the interrupt controller blocks, > >> then will come another one about the MBox/Properties. > >> > >> The last patch is unrelated to the series, but since I cleaned this > >> for the raspi and the highbank is the only board with the same issue, > >> I included the patch in this series. > > > > I'm going to apply 1-10 and 14 to target-arm.next. > > (I've reviewed 10, and the rest have been reviewed.) > > Thanks! > > Do you mind amending this to patch #3 > "hw/timer/bcm2835: Add the BCM2835 SYS_timer" > or should I respin (or resend it alone)? > > -- >8 -- > diff --git a/hw/timer/bcm2835_systmr.c b/hw/timer/bcm2835_systmr.c > index 49b40b55f9..3387a6214a 100644 > --- a/hw/timer/bcm2835_systmr.c > +++ b/hw/timer/bcm2835_systmr.c > @@ -115,10 +115,7 @@ static void bcm2835_systmr_reset(DeviceState *dev) > { > BCM2835SystemTimerState *s = BCM2835_SYSTIMER(dev); > > - s->reg.status = 0; > - for (size_t i = 0; i < ARRAY_SIZE(s->reg.compare); i++) { > - s->reg.compare[i] = 0; > - } > + memset(&s->reg, 0, sizeof(s->reg)); > } > Sure, I'll just squash that in. -- PMM
On 10/24/19 3:49 PM, Peter Maydell wrote: > On Thu, 24 Oct 2019 at 14:46, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> On 10/24/19 3:42 PM, Peter Maydell wrote: >>> On Sun, 20 Oct 2019 at 00:47, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>>> >>>> Since v2: >>>> - fixed issue in videocore address space >>>> - allow to start with some cores OFF (to boot firmwares) >>>> - add proof-of-concept test for '-smp cores=1' and U-boot >>>> - fixed my email setup >>>> >>>> Previous cover: >>>> >>>> Hi, >>>> >>>> Some patches from v1 are already merged. This v2 addresses the >>>> review comment from v1, and add patches to clean the memory >>>> space when using multiple cores. >>>> >>>> Laurent, if you test U-Boot with this patchset again, do you mind >>>> replying with a "Tested-by:" tag? >>>> >>>> The next patchset is probably about the interrupt controller blocks, >>>> then will come another one about the MBox/Properties. >>>> >>>> The last patch is unrelated to the series, but since I cleaned this >>>> for the raspi and the highbank is the only board with the same issue, >>>> I included the patch in this series. >>> >>> I'm going to apply 1-10 and 14 to target-arm.next. >>> (I've reviewed 10, and the rest have been reviewed.) I guess you meant you reviewed patch 9. >> >> Thanks! >> >> Do you mind amending this to patch #3 >> "hw/timer/bcm2835: Add the BCM2835 SYS_timer" >> or should I respin (or resend it alone)? >> >> -- >8 -- >> diff --git a/hw/timer/bcm2835_systmr.c b/hw/timer/bcm2835_systmr.c >> index 49b40b55f9..3387a6214a 100644 >> --- a/hw/timer/bcm2835_systmr.c >> +++ b/hw/timer/bcm2835_systmr.c >> @@ -115,10 +115,7 @@ static void bcm2835_systmr_reset(DeviceState *dev) >> { >> BCM2835SystemTimerState *s = BCM2835_SYSTIMER(dev); >> >> - s->reg.status = 0; >> - for (size_t i = 0; i < ARRAY_SIZE(s->reg.compare); i++) { >> - s->reg.compare[i] = 0; >> - } >> + memset(&s->reg, 0, sizeof(s->reg)); >> } >> > > Sure, I'll just squash that in. Thanks a lot! Phil.
On Thu, 24 Oct 2019 at 14:42, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Sun, 20 Oct 2019 at 00:47, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > > Since v2: > > - fixed issue in videocore address space > > - allow to start with some cores OFF (to boot firmwares) > > - add proof-of-concept test for '-smp cores=1' and U-boot > > - fixed my email setup > > > > Previous cover: > > > > Hi, > > > > Some patches from v1 are already merged. This v2 addresses the > > review comment from v1, and add patches to clean the memory > > space when using multiple cores. > > > > Laurent, if you test U-Boot with this patchset again, do you mind > > replying with a "Tested-by:" tag? > > > > The next patchset is probably about the interrupt controller blocks, > > then will come another one about the MBox/Properties. > > > > The last patch is unrelated to the series, but since I cleaned this > > for the raspi and the highbank is the only board with the same issue, > > I included the patch in this series. > > I'm going to apply 1-10 and 14 to target-arm.next. > (I've reviewed 10, and the rest have been reviewed.) ...but that causes tests/boot-serial-test to throw a clang sanitizer error and then hang: e104462:bionic:clang$ QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm ./tests/boot-serial-test /arm/boot-serial/raspi2: /home/petmay01/linaro/qemu-from-laptop/qemu/memory.c:2517:27: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/stdlib.h:819:6: note: nonnull attribute specified here The offending patch is "hw/arm/bcm2836: Use per CPU address spaces" (patch 7). So I'm dropping 7/8/9. thanks -- PMM
On 10/24/19 5:31 PM, Peter Maydell wrote: > On Thu, 24 Oct 2019 at 14:42, Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On Sun, 20 Oct 2019 at 00:47, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>> >>> Since v2: >>> - fixed issue in videocore address space >>> - allow to start with some cores OFF (to boot firmwares) >>> - add proof-of-concept test for '-smp cores=1' and U-boot >>> - fixed my email setup >>> >>> Previous cover: >>> >>> Hi, >>> >>> Some patches from v1 are already merged. This v2 addresses the >>> review comment from v1, and add patches to clean the memory >>> space when using multiple cores. >>> >>> Laurent, if you test U-Boot with this patchset again, do you mind >>> replying with a "Tested-by:" tag? >>> >>> The next patchset is probably about the interrupt controller blocks, >>> then will come another one about the MBox/Properties. >>> >>> The last patch is unrelated to the series, but since I cleaned this >>> for the raspi and the highbank is the only board with the same issue, >>> I included the patch in this series. >> >> I'm going to apply 1-10 and 14 to target-arm.next. >> (I've reviewed 10, and the rest have been reviewed.) > > ...but that causes tests/boot-serial-test to throw > a clang sanitizer error and then hang: > > e104462:bionic:clang$ QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm > ./tests/boot-serial-test > /arm/boot-serial/raspi2: > /home/petmay01/linaro/qemu-from-laptop/qemu/memory.c:2517:27: runtime > error: null pointer passed as argument 2, which is declared to never > be null > /usr/include/stdlib.h:819:6: note: nonnull attribute specified here > > The offending patch is "hw/arm/bcm2836: Use per CPU address spaces" > (patch 7). So I'm dropping 7/8/9. With -bios, raspi.c::setup_boot() we call -> load_image_targphys[_as] -> rom_add_file_fixed_as -> rom_add_file with mr=NULL, as=set vl.c::main() call -> rom_check_and_register_reset if (!rom->mr) { as = rom->as; } section = memory_region_find(rom->mr ? rom->mr : get_system_memory(), rom->addr, 1); In my patches I stop using system_memory, each CPU use its own AS view on the GPU AXI bus. Apparently we can not (yet) live without a system_memory bus. At this point I can use the RAM memory region because setup_boot() is called from raspi_init(). What is odd is load_image_targphys[_as]() get a 'addr' argument (as an offset within the address space) but load_image_mr() don't take offset, only loads at base (offset=0). Neither it takes a 'max_sz' argument. This snippet fixed the issue: -- >8 -- diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c index 569d85c11a..eb84f74dc7 100644 --- a/hw/arm/raspi.c +++ b/hw/arm/raspi.c @@ -111,7 +111,8 @@ static void reset_secondary(ARMCPU *cpu, const struct arm_boot_info *info) cpu_set_pc(cs, info->smp_loader_start); } -static void setup_boot(MachineState *machine, int version, size_t ram_size) +static void setup_boot(MachineState *machine, int version, + MemoryRegion *ram, size_t ram_size) { static struct arm_boot_info binfo; int r; @@ -149,9 +150,9 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size) */ if (machine->firmware) { hwaddr firmware_addr = version == 3 ? FIRMWARE_ADDR_3 : FIRMWARE_ADDR_2; + /* load the firmware image (typically kernel.img) */ - r = load_image_targphys(machine->firmware, firmware_addr, - ram_size - firmware_addr); + r = load_image_mr(machine->firmware, firmware_addr, ram); if (r < 0) { error_report("Failed to load firmware from %s", machine->firmware); exit(1); @@ -211,7 +212,7 @@ static void raspi_init(MachineState *machine, int version) vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size", &error_abort); - setup_boot(machine, version, machine->ram_size - vcram_size); + setup_boot(machine, version, &s->ram, machine->ram_size - vcram_size); } static void raspi2_init(MachineState *machine) diff --git a/hw/core/loader.c b/hw/core/loader.c index a3f5333258..0f11d1104a 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -137,7 +137,7 @@ int load_image_targphys_as(const char *filename, return size; } -int load_image_mr(const char *filename, MemoryRegion *mr) +int load_image_mr(const char *filename, hwaddr addr, MemoryRegion *mr) { int size; @@ -152,7 +152,7 @@ int load_image_mr(const char *filename, MemoryRegion *mr) return -1; } if (size > 0) { - if (rom_add_file_mr(filename, mr, -1) < 0) { + if (rom_add_file_mr(filename, addr, mr, -1) < 0) { return -1; } } diff --git a/include/hw/loader.h b/include/hw/loader.h index 48a96cd559..9cb47707de 100644 --- a/include/hw/loader.h +++ b/include/hw/loader.h @@ -65,6 +65,7 @@ int load_image_targphys(const char *filename, hwaddr, /** * load_image_mr: load an image into a memory region * @filename: Path to the image file + * @addr: Address to load the image to (relative to the memory region) * @mr: Memory Region to load into * * Load the specified file into the memory region. @@ -73,7 +74,7 @@ int load_image_targphys(const char *filename, hwaddr, * If the file is larger than the memory region's size the call will fail. * Returns -1 on failure, or the size of the file. */ -int load_image_mr(const char *filename, MemoryRegion *mr); +int load_image_mr(const char *filename, hwaddr addr, MemoryRegion *mr); /* This is the limit on the maximum uncompressed image size that * load_image_gzipped_buffer() and load_image_gzipped() will read. It prevents @@ -293,8 +294,8 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict); rom_add_file(_f, NULL, _a, _i, false, NULL, NULL) #define rom_add_blob_fixed(_f, _b, _l, _a) \ rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL, true) -#define rom_add_file_mr(_f, _mr, _i) \ - rom_add_file(_f, NULL, 0, _i, false, _mr, NULL) +#define rom_add_file_mr(_f, _a, _mr, _i) \ + rom_add_file(_f, NULL, _a, _i, false, _mr, NULL) #define rom_add_file_as(_f, _as, _i) \ rom_add_file(_f, NULL, 0, _i, false, NULL, _as) #define rom_add_file_fixed_as(_f, _a, _i, _as) \ --- rom_add_file_mr() is mostly used by ARM, so it'll be easy to update the other calls. Regards, Phil.
On Thu, 24 Oct 2019 at 20:46, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > In my patches I stop using system_memory, each CPU use its > own AS view on the GPU AXI bus. That seems an odd thing to do -- is there really no common baseline view of the physical address space that the CPUs all share ? thanks -- PMM