On 04.09.25 08:52, Leonid Komarianskyi wrote:
Hi Julien, Volodymyr and Oleksandr,
Hello all
Thank you for your comments.
On 04.09.25 02:04, Julien Grall wrote:
Hi,
On 03/09/2025 22:37, Volodymyr Babchuk wrote:
Hi Oleksandr,
Oleksandr Tyshchenko <olekst...@gmail.com> writes:
[...]
+static inline uint32_t vgic_get_reg_offset(uint32_t reg, uint32_t
spi_base,
+ uint32_t espi_base)
+{
+ if ( reg < espi_base )
+ return reg - spi_base;
+ else
+ return reg - espi_base;
+}
I am wondering (I do not request a change) whether
vgic_get_reg_offset() is really helpfull,
e.g. is
offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR, GICD_IPRIORITYRnE);
much better than:
offset = reg < GICD_IPRIORITYRnE ? reg - GICD_IPRIORITYR : reg -
GICD_IPRIORITYRnE;
>>>
IMO, it is easy to make a mistake, because you need to write register
name 3 times. Can cause errors during copy-pasting.
+1.
But I saw clever
trick by Mykola Kvach, something like this:
#define vgic_get_reg_offset(addr, reg_name) ( addr < reg_name##nE ? \
addr - reg_name : addr - reg_name##nE )
And then you can just use this as
offset = vgic_get_reg_offset(reg, GICD_IPRIORITYR)
I don't know what maintainers think about this type of preprocessor
trickery, but in my opinion it is justified in this case, because it
leaves less room for a mistake.
I don't have a strong opinion between the macro version or the static
inline helper. However:
* for the macro version, you want to store 'addr' in a local variable
to ensure it is only evaluated once.
* for both case, I would prefer if we assert (for the static inline
helper) or use BUILD_BUG_ON() to confirm that spi_base < espi_base
Cheers,
I was considering introducing this kind of macro, but I think it may
lead to issues in the future because it requires us to always maintain
the pattern reg_name/reg_name##nE for all registers. I understand that
the names of the defines are unlikely to change, but I would prefer to
use an inline function along with the suggested ASSERT(), as it seems
more versatile to me.
I was leaning more towards the macro, but would be happy with static
inline helper (my R-b will stay).
I guess, the suggested ASSERT() for the static inline
helper vgic_get_reg_offset would be also applicable for helper
vgic_get_rank (or whatever name it will gain).
Best regards,
Leonid
