Re: [PATCH v8 01/10] hw/fsi: Introduce IBM's Local bus

[Ninad Palsule]

Hello Cedric,

On 12/12/23 08:46, Cédric Le Goater wrote:
> On 11/29/23 00:56, Ninad Palsule wrote:
> > This is a part of patchset where IBM's Flexible Service Interface is
> > introduced.
> >
> > The LBUS is modelled to maintain mapped memory for the devices. The
> > memory is mapped after CFAM config, peek table and FSI slave registers.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> > [ clg: - removed lbus_add_device() bc unused
> >         - removed lbus_create_device() bc used only once
> >         - removed "address" property
> >         - updated meson.build to build fsi dir
> >         - included an empty hw/fsi/trace-events ]
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >   meson.build           |  1 +
> >   hw/fsi/trace.h        |  1 +
> >   include/hw/fsi/lbus.h | 40 +++++++++++++++++++++++++++++++++
> >   hw/fsi/lbus.c         | 51 +++++++++++++++++++++++++++++++++++++++++++
> >   hw/Kconfig            |  1 +
> >   hw/fsi/Kconfig        |  2 ++
> >   hw/fsi/meson.build    |  1 +
> >   hw/fsi/trace-events   |  1 +
> >   hw/meson.build        |  1 +
> >   9 files changed, 99 insertions(+)
> >   create mode 100644 hw/fsi/trace.h
> >   create mode 100644 include/hw/fsi/lbus.h
> >   create mode 100644 hw/fsi/lbus.c
> >   create mode 100644 hw/fsi/Kconfig
> >   create mode 100644 hw/fsi/meson.build
> >   create mode 100644 hw/fsi/trace-events
> >
> > diff --git a/meson.build b/meson.build
> > index ec01f8b138..b6556efd51 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -3298,6 +3298,7 @@ if have_system
> >       'hw/char',
> >       'hw/display',
> >       'hw/dma',
> > +    'hw/fsi',
> >       'hw/hyperv',
> >       'hw/i2c',
> >       'hw/i386',
> > diff --git a/hw/fsi/trace.h b/hw/fsi/trace.h
> > new file mode 100644
> > index 0000000000..ee67c7fb04
> > --- /dev/null
> > +++ b/hw/fsi/trace.h
> > @@ -0,0 +1 @@
> > +#include "trace/trace-hw_fsi.h"
> > diff --git a/include/hw/fsi/lbus.h b/include/hw/fsi/lbus.h
> > new file mode 100644
> > index 0000000000..a58e33d061
> > --- /dev/null
> > +++ b/include/hw/fsi/lbus.h
> > @@ -0,0 +1,40 @@
> > +/*
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + * Copyright (C) 2023 IBM Corp.
> > + *
> > + * IBM Local bus and connected device structures.
> > + */
> > +#ifndef FSI_LBUS_H
> > +#define FSI_LBUS_H
> > +
> > +#include "exec/memory.h"
> > +#include "hw/qdev-core.h"
> > +
> > +#define TYPE_FSI_LBUS_DEVICE "fsi.lbus.device"
> > +OBJECT_DECLARE_TYPE(FSILBusDevice, FSILBusDeviceClass, FSI_LBUS_DEVICE)
> > +
> > +#define FSI_LBUS_MEM_REGION_SIZE  (2 * 1024 * 1024)
> > +#define FSI_LBUSDEV_IOMEM_START   0xc00 /* 3K used by CFAM config 
> > etc */
>
> I don't think sizing the local bus MMIO region exactly to the size of
> the CFAM MMIO region is necessary. The upper LBUS/CFAM addresses might
> not even be backed by device registers.
>
> I would simplify with :
>
> #define FSI_LBUS_MEM_REGION_SIZE  (1 * MiB)
>
> and forget about the offset.
ok, I made it as 1MB.

Thanks for the review.

Regards,

Ninad