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

[BALATON Zoltan]

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?
(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.

> + *
> + * 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.