Re: [PATCH v7 02/10] hw/fsi: Introduce IBM's scratchpad

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

Hello Cedric,

On 11/27/23 10:09, Cédric Le Goater wrote:
> On 10/26/23 18:47, Ninad Palsule wrote:
>> This is a part of patchset where scratchpad is introduced.
>>
>> The scratchpad provides a set of non-functional registers. The firmware
>> is free to use them, hardware does not support any special management
>> support. The scratchpad registers can be read or written from LBUS
>> slave.
>>
>> In this model, The LBUS device is parent for the scratchpad.
>>
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>> v2:
>> - Incorporated Joel's review comments.
>> v5:
>> - Incorporated review comments by Cedric.
>> v6:
>> - Incorporated review comments by Daniel.
>> v7:
>> - Incorporated review comments by Philippe.
>> - Cleaned up unused bits.
>> ---
>>   meson.build                        |  1 +
>>   hw/fsi/trace.h                     |  1 +
>>   include/hw/fsi/engine-scratchpad.h | 27 +++++++++
>>   include/hw/fsi/fsi.h               | 16 +++++
>>   hw/fsi/engine-scratchpad.c         | 93 ++++++++++++++++++++++++++++++
>>   hw/fsi/Kconfig                     |  4 ++
>>   hw/fsi/meson.build                 |  1 +
>>   hw/fsi/trace-events                |  2 +
>>   8 files changed, 145 insertions(+)
>>   create mode 100644 hw/fsi/trace.h
>>   create mode 100644 include/hw/fsi/engine-scratchpad.h
>>   create mode 100644 include/hw/fsi/fsi.h
>>   create mode 100644 hw/fsi/engine-scratchpad.c
>>   create mode 100644 hw/fsi/trace-events
>>
>> diff --git a/meson.build b/meson.build
>> index dcef8b1e79..793c7c1f20 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -3257,6 +3257,7 @@ if have_system
>>       'hw/char',
>>       'hw/display',
>>       'hw/dma',
>> +    'hw/fsi',
>
> This should be introduced in the first patch.
ok, Thanks for fixing it.
>
>>       '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/engine-scratchpad.h 
>> b/include/hw/fsi/engine-scratchpad.h
>> new file mode 100644
>> index 0000000000..4ffa871965
>> --- /dev/null
>> +++ b/include/hw/fsi/engine-scratchpad.h
>
> This model is introduced to early please drop from this patch.
ok, Thanks for fixing it. We should update the commit message. I will 
send it.
>
>> @@ -0,0 +1,27 @@
>> +/*
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + * Copyright (C) 2023 IBM Corp.
>> + *
>> + * IBM scratchpad engne
>> + */
>> +#ifndef FSI_ENGINE_SCRATCHPAD_H
>> +#define FSI_ENGINE_SCRATCHPAD_H
>> +
>> +#include "hw/fsi/lbus.h"
>> +#include "hw/fsi/fsi.h"
>> +
>> +#define ENGINE_CONFIG_NEXT            BE_BIT(0)
>> +#define ENGINE_CONFIG_TYPE_PEEK       (0x02 << 4)
>> +#define ENGINE_CONFIG_TYPE_FSI        (0x03 << 4)
>> +#define ENGINE_CONFIG_TYPE_SCRATCHPAD (0x06 << 4)
>> +
>> +#define TYPE_FSI_SCRATCHPAD "fsi.scratchpad"
>> +#define SCRATCHPAD(obj) OBJECT_CHECK(FSIScratchPad, (obj), 
>> TYPE_FSI_SCRATCHPAD)
>> +
>> +typedef struct FSIScratchPad {
>> +        FSILBusDevice parent;
>> +
>> +        uint32_t reg;
>> +} FSIScratchPad;
>> +
>> +#endif /* FSI_ENGINE_SCRATCHPAD_H */
>> diff --git a/include/hw/fsi/fsi.h b/include/hw/fsi/fsi.h
>> new file mode 100644
>> index 0000000000..b08b97f62b
>> --- /dev/null
>> +++ b/include/hw/fsi/fsi.h
>> @@ -0,0 +1,16 @@
>> +/*
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + * Copyright (C) 2023 IBM Corp.
>> + *
>> + * IBM Flexible Service Interface
>> + */
>> +#ifndef FSI_FSI_H
>> +#define FSI_FSI_H
>> +
>> +#include "qemu/bitops.h"
>> +
>> +/* Bitwise operations at the word level. */
>> +#define BE_BIT(x)           BIT(31 - (x))
>
> 31 ? BITS_PER_LONG would be better I think.
>
>> +#define BE_GENMASK(hb, lb) MAKE_64BIT_MASK((lb), ((hb) - (lb) + 1))
>> +
>> +#endif
>> diff --git a/hw/fsi/engine-scratchpad.c b/hw/fsi/engine-scratchpad.c
>> new file mode 100644
>> index 0000000000..a8887cd613
>> --- /dev/null
>> +++ b/hw/fsi/engine-scratchpad.c
>
> DRop FSIScratchPad from this patch. It should be introduced later.
>
> However, TYPE_FSI_BUS should be introcuded now because the following
> patch needs it.
ok, The scratpad is a engine/device which is attached to the LBUS hence 
it was introduced early. But I am fine with the change. Thanks for the 
change.
>
>
>> @@ -0,0 +1,93 @@
>> +/*
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + * Copyright (C) 2023 IBM Corp.
>> + *
>> + * IBM scratchpad engine
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +
>> +#include "qapi/error.h"
>> +#include "qemu/log.h"
>> +#include "trace.h"
>> +
>> +#include "hw/fsi/engine-scratchpad.h"
>> +
>> +static uint64_t fsi_scratchpad_read(void *opaque, hwaddr addr, 
>> unsigned size)
>> +{
>> +    FSIScratchPad *s = SCRATCHPAD(opaque);
>> +
>> +    trace_fsi_scratchpad_read(addr, size);
>> +
>> +    if (addr) {
>> +        return 0;
>> +    }
>> +
>> +    return s->reg;
>> +}
>> +
>> +static void fsi_scratchpad_write(void *opaque, hwaddr addr, uint64_t 
>> data,
>> +                                 unsigned size)
>> +{
>> +    FSIScratchPad *s = SCRATCHPAD(opaque);
>> +
>> +    trace_fsi_scratchpad_write(addr, size, data);
>> +
>> +    if (addr) {
>> +        return;
>> +    }
>> +
>> +    s->reg = data;
>> +}
>> +
>> +static const struct MemoryRegionOps scratchpad_ops = {
>> +    .read = fsi_scratchpad_read,
>> +    .write = fsi_scratchpad_write,
>> +    .endianness = DEVICE_BIG_ENDIAN,
>> +};
>> +
>> +static void fsi_scratchpad_realize(DeviceState *dev, Error **errp)
>> +{
>> +    FSILBusDevice *ldev = FSI_LBUS_DEVICE(dev);
>> +
>> +    memory_region_init_io(&ldev->iomem, OBJECT(ldev), &scratchpad_ops,
>> +                          ldev, TYPE_FSI_SCRATCHPAD, 0x400);
>> +}
>> +
>> +static void fsi_scratchpad_reset(DeviceState *dev)
>> +{
>> +    FSIScratchPad *s = SCRATCHPAD(dev);
>> +
>> +    s->reg = 0;
>> +}
>> +
>> +static void fsi_scratchpad_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    FSILBusDeviceClass *ldc = FSI_LBUS_DEVICE_CLASS(klass);
>> +
>> +    dc->realize = fsi_scratchpad_realize;
>> +    dc->reset = fsi_scratchpad_reset;
>> +
>> +    ldc->config =
>> +          ENGINE_CONFIG_NEXT            | /* valid */
>> +          0x00010000                    | /* slots */
>> +          0x00001000                    | /* version */
>> +          ENGINE_CONFIG_TYPE_SCRATCHPAD | /* type */
>> +          0x00000007;                     /* crc */
>> +}
>> +
>> +static const TypeInfo fsi_scratchpad_info = {
>> +    .name = TYPE_FSI_SCRATCHPAD,
>> +    .parent = TYPE_FSI_LBUS_DEVICE,
>> +    .instance_size = sizeof(FSIScratchPad),
>> +    .class_init = fsi_scratchpad_class_init,
>> +    .class_size = sizeof(FSILBusDeviceClass),
>> +};
>> +
>> +static void fsi_scratchpad_register_types(void)
>> +{
>> +    type_register_static(&fsi_scratchpad_info);
>> +}
>> +
>> +type_init(fsi_scratchpad_register_types);
>> diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig
>> index e650c660f0..f7c7fd1b28 100644
>> --- a/hw/fsi/Kconfig
>> +++ b/hw/fsi/Kconfig
>> @@ -1,2 +1,6 @@
>> +config FSI_SCRATCHPAD
>> +    bool
>> +    select FSI_LBUS
>> +
>>   config FSI_LBUS
>>       bool
>
>
> I am not sure we need all this config option. We will see that at the end
> when the models are ready.

OK

Thanks for the review.

Regards,

Ninad

>
>
> Thanks,
>
> C.
>
>
>
>> diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
>> index 4074d3a7d2..d45a98c223 100644
>> --- a/hw/fsi/meson.build
>> +++ b/hw/fsi/meson.build
>> @@ -1 +1,2 @@
>>   system_ss.add(when: 'CONFIG_FSI_LBUS', if_true: files('lbus.c'))
>> +system_ss.add(when: 'CONFIG_FSI_SCRATCHPAD', if_true: 
>> files('engine-scratchpad.c'))
>> diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
>> new file mode 100644
>> index 0000000000..c5753e2791
>> --- /dev/null
>> +++ b/hw/fsi/trace-events
>> @@ -0,0 +1,2 @@
>> +fsi_scratchpad_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " 
>> size=%d"
>> +fsi_scratchpad_write(uint64_t addr, uint32_t size, uint64_t data) 
>> "@0x%" PRIx64 " size=%d value=0x%"PRIx64
>