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

[Jia Wang <wangjia@ultrarisc.com>]

On 2026-04-28 11:41 +0300, Ilpo Järvinen wrote:
> On Tue, 28 Apr 2026, Andy Shevchenko wrote:
> 
> > 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@linux.intel.com>
> > 
> > ...
> > 
> > >  #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?
> 
> Do those BUILD_BUGs even generate code, especially when they are expected 
> to only appear in a struct initializer?
> 
> > #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.
> 
> I'd put also FIELD_PREP_CONST() into the macro itself as I don't see much 
> value for this macro outside of those .cpr_value initializations.
> 
> IMO, the entire macro would be cleaner looking as a truly multi-line 
> construct. Can we use static_assert()s in struct field initialization 
> (I'm not sure), something along these lines:
> 
> #define DW_UART_CPR_FIFO_MODE_FROM_SIZE(size)			\
> ({								\
> 	typeof (size) __size = size;				\
> 								\
> 	static_assert(IS_ALIGNED((__size), 16));		\
> 	static_assert(__size <= DW_UART_CPR_FIFO_MODE_MAX);	\
> 								\
> 	FIELD_PREP_CONST(DW_UART_CPR_FIFO_MODE, __size / 16);	\
> })
> 

Thanks. I tried that approach, but the statement-expression form does
not work in this case because the helper is used in static initializers.
So I'll keep it as a plain expression macro for now, and just rework it
into a cleaner multi-line form.

> -- 
>  i.
> 
>

Best Regards,
Jia Wang