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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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)

?

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