Re: [PATCH v3 2/2] serial: 8250_dw: Use a fixed CPR value for UltraRISC DP1000 UART

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3124 bytes --]

On Wed, 22 Apr 2026, Jia Wang wrote:

> On 2026-04-21 15:18 +0300, Ilpo Järvinen wrote:
> > On Tue, 21 Apr 2026, Jia Wang wrote:
> > 
> > > The UltraRISC DP1000 UART does not provide the standard CPR register used
> > > by 8250_dw to discover port capabilities.
> > > 
> > > Provide a fixed CPR value for the DP1000-specific compatible so the
> > > driver can configure the port correctly.
> > > 
> > > Signed-off-by: Jia Wang <wangjia@ultrarisc.com>
> > > ---
> > >  drivers/tty/serial/8250/8250_dw.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> > > index 94beadb4024d..ca6dbdf75918 100644
> > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > @@ -962,6 +962,12 @@ static const struct dw8250_platform_data dw8250_intc10ee = {
> > >  	.quirks = DW_UART_QUIRK_IER_KICK,
> > >  };
> > >  
> > > +static const struct dw8250_platform_data dw8250_ultrarisc_dp1000_data = {
> > > +	.usr_reg = DW_UART_USR,
> > > +	.cpr_value = 0x00022022,
> > 
> > Hi,
> > 
> > Please construct the cpr_value by ORing DW_UART_CPR_* defines together.
> > For fields, FIELD_PREP_CONST() may be useful.
> > 
> > In order to be able to use the DW_UART_CPR_* defines, they need to be 
> > moved into 8250_dwlib.h (I'd move all DW_UART register defines in a 
> > preparatory patch).
> > 
> > I know the existing Renesas' .cpr_value doesn't follow this convention but 
> > that could be converted as well (in another patch, or leave the Renesas 
> > entry conversion to me if you don't want to do that).
> >
> 
> Thanks for the review.
> 
> Just to confirm: since you mentioned that the preparatory patch moving
> the DW_UART register defines is already in place, I don't need to move
> them again, correct?

I'm not sure where that preparatory patch is. Those defines still appear 
in 8250_dwlib.c AFAICT.

To be clear, DW_UART_CPR_* defines are in 8250_dwlib.c but you're 
modifying 8250_dw.c file here so you cannot use those pre-existing defines 
as is in 8250_dw.c. To solve that, please add a preparatory patch into 
your series which relocates all those defines from 8250_dwlib.c to 
8250_dwlib.h. If there's a patch which does that already, it has missed my 
radar.

And my suggestion is to move not just DW_UART_CPR_* defines into 
8250_dwlib.h but all DW UART specific register defines to keep them in 
the same place.

Hopefully this clears up any misunderstanding.


My main point all along is that do not do the define move in the same 
patch where you're adding UltraRISC DP1000 UART (which is a common pitfall 
to many submitters so I try to give pro-active instructions while 
reviewing to avoid extra versions because of them).

> I will update my patch to use the DW_UART_CPR_* macros and
> FIELD_PREP_CONST() accordingly, and I’m happy to add a separate patch in
> the next revision to convert the Renesas .cpr_value as well.

Great, thanks.

> > > +	.quirks = DW_UART_QUIRK_CPR_VALUE,
> > > +};

-- 
 i.