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

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

Hello Philippe,

On 10/23/23 10:00, Philippe Mathieu-Daudé wrote:
> On 21/10/23 23:17, 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.
>> ---
>>   meson.build                        |  1 +
>>   hw/fsi/trace.h                     |  1 +
>>   include/hw/fsi/engine-scratchpad.h | 32 ++++++++++
>>   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, 150 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/include/hw/fsi/fsi.h b/include/hw/fsi/fsi.h
>> new file mode 100644
>> index 0000000000..e65f26f17b
>> --- /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
>> +
>> +/* Bitwise operations at the word level. */
>> +#define BE_BIT(x)                          BIT(31 - (x))
>> +#define GENMASK(t, b) \
>> +    (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) - 1))
>
> Please use MAKE_64BIT_MASK() from "qemu/bitops.h".

The GENMASK and MAKE_64BIT_MASK macros are invoke differently.

GENMASK is invoked with bit t and bit b (t:b) and it provides the mask and

MAKE_64BIT_MASK uses shift and length.

Thanks for the review.

Regards,

Ninad


>> +#define BE_GENMASK(t, b)                   GENMASK(BE_BIT(t), 
>> BE_BIT(b))
>> +
>> +#endif
>