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

Andy Shevchenko
2026-04-28T10:57:50Z

On Tue, Apr 28, 2026 at 11:41:27AM +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:

...

> > > /* 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?

Good question if this affects the code generation.

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

Yep, and it would make it on par with the existing _FIFO_SIZE() that has
FIELD_GET() there.

> 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:

I believe one may put there static_assert():s.

> #define DW_UART_CPR_FIFO_MODE_FROM_SIZE(size) \
> ({ \
> typeof (size) __size = size; \

Perhaps auto ?

> \
> static_assert(IS_ALIGNED((__size), 16)); \
> static_assert(__size <= DW_UART_CPR_FIFO_MODE_MAX); \

But I still think the % and / paired are clearer (for reading and understanding)
even if they do not affect code generation. Also I think the plain number of the
maximum size is better for the same reasons we do not have it for 16.

> \
> FIELD_PREP_CONST(DW_UART_CPR_FIFO_MODE, __size / 16); \
> })

--
With Best Regards,
Andy Shevchenko