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

[Jia Wang]

On 2026-04-28 10:28 +0300, 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 at linux.intel.com>
>

Thanks for the review. I'll address them and add your Reviewed-by tag
in v6.
 
> ...
> 
> >  #include <linux/bitfield.h>
> >  #include <linux/bits.h>
> > +#include <linux/build_bug.h>
> > +#include <linux/align.h>
> 
> Preserve order, 'a' goes before 'b'.
> 

Will fix in v6.

> >  #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.
> 

This define will be removed in v6.

> > +#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.
>

Makes sense. I'll fold the upper limit into the macro and switch to
(size) > 2048 and (size) % 16 checks for v6.

In the driver, the macro is used only in static initializers, so the
constants are fully resolved at compile time.

I also checked code generation on x86_64/x86: both IS_ALIGNED() and % 16
produce identical instructions at -O2. I'll still switch to % 16 as you
suggest for clarity.

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
>

Best Regards,
Jia Wang