Am 24. September 2024 20:02:31 UTC schrieb Bernhard Beschow <shen...@gmail.com>:
>
>
>Am 23. September 2024 10:38:46 UTC schrieb BALATON Zoltan <bala...@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 <shen...@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...
I'll name the header like the struct it defines, i.e. ppce500_ccsr.h.
>
>>(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-...@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....@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.
I'll remove the reference to ppc440_bamboo.c which is clearly not relevant
here. Beyond that I rather leave the license text unchanged.
Best regards,
Bernhard
>
>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.
>>>
