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

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

On 10/24/23 02:08, Philippe Mathieu-Daudé wrote:
> On 23/10/23 19:08, Ninad Palsule wrote:
>> 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.
>
> Don't we have:
>
> #define GENMASK(t, b) MAKE_64BIT_MASK(t, b - t + 1)
>
> ?

You are right. I am able to use this macro. I have removed some unused 
macros.

Thanks for the review.

Regards,

Ninad

>
>> Thanks for the review.
>>
>> Regards,
>>
>> Ninad
>>
>>
>>>> +#define BE_GENMASK(t, b)                   GENMASK(BE_BIT(t), 
>>>> BE_BIT(b))
>>>> +
>>>> +#endif
>>>
>