Re: [PATCH v5 2/4] serial: 8250_dw: build Renesas RZN1 CPR value from DW_UART_CPR_* definitions

[Andy Shevchenko]

On Tue, Apr 28, 2026 at 01:26:27PM +0800, Jia Wang wrote:
> Replace the magic CPR value for Renesas RZ/N1 with a composition using
> DW_UART_CPR_* bit/field definitions and FIELD_PREP_CONST().
> 
> Introduce a helper macro to convert a FIFO size (bytes) into the CPR
> FIFO_MODE field value, with BUILD_BUG_ON_ZERO() checks for alignment and
> bounds. Use it to replace the literal FIFO_MODE values in the RZN1.

A couple of nit-picks below. After addressing them you can add

Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

...

>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
> +#include <linux/build_bug.h>
> +#include <linux/align.h>

Preserve order, 'a' goes before 'b'.

>  #include <linux/io.h>
>  #include <linux/types.h>

...

>  /* Helper for FIFO size calculation */
>  #define DW_UART_CPR_FIFO_SIZE(a)	(FIELD_GET(DW_UART_CPR_FIFO_MODE, (a)) * 16)

> +#define DW_UART_CPR_FIFO_MODE_MAX	0x80

You used decimal values elsewhere (id est 16), use upper limit in decimal
as well.

> +#define DW_UART_CPR_FIFO_MODE_FROM_SIZE(size)				\
> +	(BUILD_BUG_ON_ZERO(!IS_ALIGNED((size), 16)) +			\
> +	 BUILD_BUG_ON_ZERO(((size) / 16) > DW_UART_CPR_FIFO_MODE_MAX) +	\
> +	 ((size) / 16))

I don't see the need in having that maximum being defined separately (we don't
have that for 16, no need to have it for 128.

Since some ISA:s have one assembly instruction to get both / and % divisions,
it's better to use that instead of IS_ALIGNED(). Can you check code generation
for x86_64 / x86?

#define DW_UART_CPR_FIFO_MODE_FROM_SIZE(size)				\
	(BUILD_BUG_ON_ZERO((size) > 2048) + BUILD_BUG_ON_ZERO((size) % 16) + ((size) / 16))

Note, I dropped first division in order to show the upper limit in a plain
number since 16 is also FIFO size in bytes.

Also note, this evaluates (size) three times, which might be problematic,
but I think we can leave with that for now.


-- 
With Best Regards,
Andy Shevchenko