| Message ID | 20180601144328.23817-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | armv7m: Remove armv7m_init() function | expand |
Hi Peter, On 2 June 2018 at 00:13, Peter Maydell <peter.maydell@linaro.org> wrote: > Remove the now-unused armv7m_init() function. This was a legacy from > before we properly QOMified ARMv7M, and it has some flaws: > > * it combines work that needs to be done by an SoC object (creating > and initializing the TYPE_ARMV7M object) with work that needs to > be done by the board model (setting the system up to load the ELF > file specified with -kernel) > * TYPE_ARMV7M creation failure is fatal, but an SoC object wants to > arrange to propagate the failure outward > * it uses allocate-and-create via qdev_create() whereas the current > preferred style for SoC objects is to do creation in-place > > Board and SoC models can instead do the two jobs this function > was doing themselves, in the right places and with whatever their > preferred style/error handling is. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/hw/arm/arm.h | 8 ++------ > hw/arm/armv7m.c | 21 --------------------- > 2 files changed, 2 insertions(+), 27 deletions(-) > > diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h > index 70fa2287e2..ffed39252d 100644 > --- a/include/hw/arm/arm.h > +++ b/include/hw/arm/arm.h > @@ -23,9 +23,6 @@ typedef enum { > ARM_ENDIANNESS_BE32, > } arm_endianness; > > -/* armv7m.c */ > -DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq, > - const char *kernel_filename, const char *cpu_type); > /** > * armv7m_load_kernel: > * @cpu: CPU > @@ -33,9 +30,8 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq, > * @mem_size: mem_size: maximum image size to load > * > * Load the guest image for an ARMv7M system. This must be called by > - * any ARMv7M board, either directly or via armv7m_init(). (This is > - * necessary to ensure that the CPU resets correctly on system reset, > - * as well as for kernel loading.) > + * any ARMv7M board. (This is necessary to ensure that the CPU resets > + * correctly on system reset, as well as for kernel loading.) > */ > void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size); > > diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c > index f123cc7d3d..a4ab7d2069 100644 > --- a/hw/arm/armv7m.c > +++ b/hw/arm/armv7m.c > @@ -261,27 +261,6 @@ static void armv7m_reset(void *opaque) > cpu_reset(CPU(cpu)); > } > > -/* Init CPU and memory for a v7-M based board. > - mem_size is in bytes. > - Returns the ARMv7M device. */ > - > -DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq, > - const char *kernel_filename, const char *cpu_type) > -{ > - DeviceState *armv7m; > - > - armv7m = qdev_create(NULL, TYPE_ARMV7M); > - qdev_prop_set_uint32(armv7m, "num-irq", num_irq); > - qdev_prop_set_string(armv7m, "cpu-type", cpu_type); > - object_property_set_link(OBJECT(armv7m), OBJECT(get_system_memory()), > - "memory", &error_abort); It looks like the snippet above is going to be a cut/paste for all v7m machines. Would it achieve your goal by instead removing the armv7m_load_kernel call from this function? Cheers, Joel > - /* This will exit with an error if the user passed us a bad cpu_type */ > - qdev_init_nofail(armv7m); > - > - armv7m_load_kernel(ARM_CPU(first_cpu), kernel_filename, mem_size); > - return armv7m; > -} > - > void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size) > { > int image_size; > -- > 2.17.1 >
On 3 June 2018 at 01:48, Joel Stanley <joel@jms.id.au> wrote: > Hi Peter, > > On 2 June 2018 at 00:13, Peter Maydell <peter.maydell@linaro.org> wrote: >> Remove the now-unused armv7m_init() function. This was a legacy from >> before we properly QOMified ARMv7M, and it has some flaws: >> >> * it combines work that needs to be done by an SoC object (creating >> and initializing the TYPE_ARMV7M object) with work that needs to >> be done by the board model (setting the system up to load the ELF >> file specified with -kernel) >> * TYPE_ARMV7M creation failure is fatal, but an SoC object wants to >> arrange to propagate the failure outward >> * it uses allocate-and-create via qdev_create() whereas the current >> preferred style for SoC objects is to do creation in-place >> >> Board and SoC models can instead do the two jobs this function >> was doing themselves, in the right places and with whatever their >> preferred style/error handling is. >> -DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq, >> - const char *kernel_filename, const char *cpu_type) >> -{ >> - DeviceState *armv7m; >> - >> - armv7m = qdev_create(NULL, TYPE_ARMV7M); >> - qdev_prop_set_uint32(armv7m, "num-irq", num_irq); >> - qdev_prop_set_string(armv7m, "cpu-type", cpu_type); >> - object_property_set_link(OBJECT(armv7m), OBJECT(get_system_memory()), >> - "memory", &error_abort); > > It looks like the snippet above is going to be a cut/paste for all v7m machines. If you look at pretty much all the v7m machines except stellaris, they don't use this code fragment. (Instead you have an object_initialize()/qdev_set_parent_bus() in the SoC container's init function, calls to set properties and realize in the container's realize, and a call to armv7m_load_kernel() in the board code.) I agree that what you might call the "modern style" of writing SoC containers like that has a lot of boilerplate, but that's not specific to the armv7m object, and if we want to reduce boilerplate we should look at how we can do it consistently across devices. thanks -- PMM
On Fri, Jun 01, 2018 at 03:43:28PM +0100, Peter Maydell wrote: > Remove the now-unused armv7m_init() function. This was a legacy from > before we properly QOMified ARMv7M, and it has some flaws: > > * it combines work that needs to be done by an SoC object (creating > and initializing the TYPE_ARMV7M object) with work that needs to > be done by the board model (setting the system up to load the ELF > file specified with -kernel) > * TYPE_ARMV7M creation failure is fatal, but an SoC object wants to > arrange to propagate the failure outward > * it uses allocate-and-create via qdev_create() whereas the current > preferred style for SoC objects is to do creation in-place > > Board and SoC models can instead do the two jobs this function > was doing themselves, in the right places and with whatever their > preferred style/error handling is. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/hw/arm/arm.h | 8 ++------ > hw/arm/armv7m.c | 21 --------------------- > 2 files changed, 2 insertions(+), 27 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h index 70fa2287e2..ffed39252d 100644 --- a/include/hw/arm/arm.h +++ b/include/hw/arm/arm.h @@ -23,9 +23,6 @@ typedef enum { ARM_ENDIANNESS_BE32, } arm_endianness; -/* armv7m.c */ -DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq, - const char *kernel_filename, const char *cpu_type); /** * armv7m_load_kernel: * @cpu: CPU @@ -33,9 +30,8 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq, * @mem_size: mem_size: maximum image size to load * * Load the guest image for an ARMv7M system. This must be called by - * any ARMv7M board, either directly or via armv7m_init(). (This is - * necessary to ensure that the CPU resets correctly on system reset, - * as well as for kernel loading.) + * any ARMv7M board. (This is necessary to ensure that the CPU resets + * correctly on system reset, as well as for kernel loading.) */ void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size); diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c index f123cc7d3d..a4ab7d2069 100644 --- a/hw/arm/armv7m.c +++ b/hw/arm/armv7m.c @@ -261,27 +261,6 @@ static void armv7m_reset(void *opaque) cpu_reset(CPU(cpu)); } -/* Init CPU and memory for a v7-M based board. - mem_size is in bytes. - Returns the ARMv7M device. */ - -DeviceState *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq, - const char *kernel_filename, const char *cpu_type) -{ - DeviceState *armv7m; - - armv7m = qdev_create(NULL, TYPE_ARMV7M); - qdev_prop_set_uint32(armv7m, "num-irq", num_irq); - qdev_prop_set_string(armv7m, "cpu-type", cpu_type); - object_property_set_link(OBJECT(armv7m), OBJECT(get_system_memory()), - "memory", &error_abort); - /* This will exit with an error if the user passed us a bad cpu_type */ - qdev_init_nofail(armv7m); - - armv7m_load_kernel(ARM_CPU(first_cpu), kernel_filename, mem_size); - return armv7m; -} - void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size) { int image_size;
Remove the now-unused armv7m_init() function. This was a legacy from before we properly QOMified ARMv7M, and it has some flaws: * it combines work that needs to be done by an SoC object (creating and initializing the TYPE_ARMV7M object) with work that needs to be done by the board model (setting the system up to load the ELF file specified with -kernel) * TYPE_ARMV7M creation failure is fatal, but an SoC object wants to arrange to propagate the failure outward * it uses allocate-and-create via qdev_create() whereas the current preferred style for SoC objects is to do creation in-place Board and SoC models can instead do the two jobs this function was doing themselves, in the right places and with whatever their preferred style/error handling is. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/hw/arm/arm.h | 8 ++------ hw/arm/armv7m.c | 21 --------------------- 2 files changed, 2 insertions(+), 27 deletions(-) -- 2.17.1