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

[Ninad Palsule <ninad@linux.ibm.com>]

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