Re: [PATCH 07/23] hw/ppc/e500: Extract ppce500_ccsr.c

[Bernhard Beschow <shentey@gmail.com>]

Am 23. September 2024 10:38:46 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>> The device model already has a header file. Also extract its implementation into
>> an accompanying source file like other e500 devices.
>> 
>> This commit is also a preparation for the next commit.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> MAINTAINERS           |  2 +-
>> hw/ppc/e500-ccsr.h    |  2 ++
>> hw/ppc/e500.c         | 17 -----------------
>> hw/ppc/ppce500_ccsr.c | 38 ++++++++++++++++++++++++++++++++++++++
>
>Maybe you could call it e500_ccsr.c and also rename the header to e500_ccsr.h (underscore instead of dash) to match them. Or if you want to match ppce500_spin.c then maybe move contents of e500-ccsr.h to e500.h?

Yeah, naming is hard. I'd actually prefer something like fsl_immr.* (taking inspiration from device tree here) to not confuse the whole SoC with the CPU. With the ppce500 prefix I was sticking to the convention of the PCIE device. Though there is also mpc-i2c and fsl_etsec...

>(More below...)
>
>> hw/ppc/meson.build    |  1 +
>> 5 files changed, 42 insertions(+), 18 deletions(-)
>> create mode 100644 hw/ppc/ppce500_ccsr.c
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ffacd60f40..b7c8b7ae72 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1433,7 +1433,7 @@ e500
>> L: qemu-ppc@nongnu.org
>> S: Orphan
>> F: hw/ppc/e500*
>> -F: hw/ppc/ppce500_spin.c
>> +F: hw/ppc/ppce500_*.c
>> F: hw/gpio/mpc8xxx.c
>> F: hw/i2c/mpc_i2c.c
>> F: hw/net/fsl_etsec/
>> diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
>> index 249c17be3b..3ab7e72568 100644
>> --- a/hw/ppc/e500-ccsr.h
>> +++ b/hw/ppc/e500-ccsr.h
>> @@ -4,6 +4,8 @@
>> #include "hw/sysbus.h"
>> #include "qom/object.h"
>> 
>> +#define MPC8544_CCSRBAR_SIZE       0x00100000ULL
>> +
>> struct PPCE500CCSRState {
>>     /*< private >*/
>>     SysBusDevice parent;
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 2225533e33..4ee4304a8a 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -61,7 +61,6 @@
>> #define RAM_SIZES_ALIGN            (64 * MiB)
>> 
>> /* TODO: parameterize */
>> -#define MPC8544_CCSRBAR_SIZE       0x00100000ULL
>> #define MPC8544_MPIC_REGS_OFFSET   0x40000ULL
>> #define MPC8544_MSI_REGS_OFFSET   0x41600ULL
>> #define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
>> @@ -1264,21 +1263,6 @@ void ppce500_init(MachineState *machine)
>>     pms->boot_info.dt_size = dt_size;
>> }
>> 
>> -static void e500_ccsr_initfn(Object *obj)
>> -{
>> -    PPCE500CCSRState *ccsr = CCSR(obj);
>> -    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
>> -                       MPC8544_CCSRBAR_SIZE);
>> -    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
>> -}
>> -
>> -static const TypeInfo e500_ccsr_info = {
>> -    .name          = TYPE_CCSR,
>> -    .parent        = TYPE_SYS_BUS_DEVICE,
>> -    .instance_size = sizeof(PPCE500CCSRState),
>> -    .instance_init = e500_ccsr_initfn,
>> -};
>> -
>> static const TypeInfo ppce500_info = {
>>     .name          = TYPE_PPCE500_MACHINE,
>>     .parent        = TYPE_MACHINE,
>> @@ -1289,7 +1273,6 @@ static const TypeInfo ppce500_info = {
>> 
>> static void e500_register_types(void)
>> {
>> -    type_register_static(&e500_ccsr_info);
>>     type_register_static(&ppce500_info);
>> }
>> 
>> diff --git a/hw/ppc/ppce500_ccsr.c b/hw/ppc/ppce500_ccsr.c
>> new file mode 100644
>> index 0000000000..191a9ceec3
>> --- /dev/null
>> +++ b/hw/ppc/ppce500_ccsr.c
>> @@ -0,0 +1,38 @@
>> +/*
>> + * QEMU PowerPC E500 embedded processors CCSR space emulation
>> + *
>> + * Copyright (C) 2009 Freescale Semiconductor, Inc. All rights reserved.
>> + *
>> + * Author: Yu Liu,     <yu.liu@freescale.com>
>> + *
>> + * This file is derived from hw/ppc440_bamboo.c,
>> + * the copyright for that material belongs to the original owners.
>
>I think CCSR is a Freescale thing so likely this has nothing to do with ppc440_bamboo so this sentence was for other parts of e500.c not applicable to this part.

Good point. IANAL so erred on the safe side and copied the whole license text. I'm happy to omit this and would also prefer SPDX license identifiers as long as I get the confirmation that this is legal.

Best regards,
Bernhard

>
>> + *
>> + * This is free software; you can redistribute it and/or modify
>> + * it under the terms of  the GNU General  Public License as published by
>> + * the Free Software Foundation;  either version 2 of the  License, or
>> + * (at your option) any later version.
>> + */
>
>SPDX-License-Identifier seems to be preferred by some nowadays, I don't have an opinion on that so just mentioning it for consideration but I'm OK with this one too although it seems a bit long.
>
>Regards,
>BALATON Zoltan
>
>> +
>> +#include "qemu/osdep.h"
>> +#include "e500-ccsr.h"
>> +
>> +static void e500_ccsr_init(Object *obj)
>> +{
>> +    PPCE500CCSRState *ccsr = CCSR(obj);
>> +
>> +    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
>> +                       MPC8544_CCSRBAR_SIZE);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
>> +}
>> +
>> +static const TypeInfo types[] = {
>> +    {
>> +        .name          = TYPE_CCSR,
>> +        .parent        = TYPE_SYS_BUS_DEVICE,
>> +        .instance_size = sizeof(PPCE500CCSRState),
>> +        .instance_init = e500_ccsr_init,
>> +    },
>> +};
>> +
>> +DEFINE_TYPES(types)
>> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
>> index 7cd9189869..43c746795a 100644
>> --- a/hw/ppc/meson.build
>> +++ b/hw/ppc/meson.build
>> @@ -81,6 +81,7 @@ ppc_ss.add(when: 'CONFIG_MPC8544DS', if_true: files('mpc8544ds.c'))
>> ppc_ss.add(when: 'CONFIG_E500', if_true: files(
>>   'e500.c',
>>   'mpc8544_guts.c',
>> +  'ppce500_ccsr.c',
>>   'ppce500_spin.c'
>> ))
>> # PowerPC 440 Xilinx ML507 reference board.
>>