RE: [PATCH v3 3/3] serial: 8250_em: Add serial_out() to struct serial8250_em_hw_info

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

On Mon, 13 Feb 2023, Biju Das wrote:

> Hi Ilpo,
> 
> Thanks for the feedback.
> 
> > Subject: RE: [PATCH v3 3/3] serial: 8250_em: Add serial_out() to struct
> > serial8250_em_hw_info
> > 
> > On Mon, 13 Feb 2023, Biju Das wrote:
> > 
> > > Hi Jiri Slaby,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: Re: [PATCH v3 3/3] serial: 8250_em: Add serial_out() to
> > > > struct serial8250_em_hw_info
> > > >
> > > > On 13. 02. 23, 10:31, Biju Das wrote:
> > > > > So looks like similar to other macros, UART_FCR_EM (0x3) is sensible
> > one.
> > > > >
> > > > > UART_FCR_RO_OFFSET (9)
> > > > > UART_FCR_RO_EM (UART_FCR_EM + UART_FCR_RO_OFFSET)
> > > > >
> > > > >
> > > > > static unsigned int serial8250_em_serial_in(struct uart_port *p,
> > > > > int
> > > > > offset) case UART_FCR_RO_EM:
> > > > > 	return readl(p->membase + (offset - UART_FCR_RO_OFFSET << 2));
> > > >
> > > >
> > > > Please send a complete patch as a reply. I am completely lost now.
> > >
> > > Please find the complete patc.
> > >
> > >
> > > From e597ae60eb170c1f1b650e1e533bf4e12c09f822 Mon Sep 17 00:00:00 2001
> > > From: Biju Das <biju.das.jz@bp.renesas.com>
> > > Date: Tue, 7 Feb 2023 15:07:13 +0000
> > > Subject: [PATCH] serial: 8250_em: Add serial_out() to struct
> > > serial8250_em_hw_info
> > >
> > > As per RZ/V2M hardware manual(Rev.1.30 Jun, 2022), UART IP has a
> > > restriction as mentioned below.
> > >
> > > 40.6.1 Point for Caution when Changing the Register Settings:
> > >
> > > When changing the settings of the following registers, a PRESETn
> > > master reset or FIFO reset + SW reset (FCR[2],FCR[1], HCR0[7]) must be
> > > input to re-initialize them.
> > >
> > > Target Registers: FCR, LCR, MCR, DLL, DLM, HCR0.
> > >
> > > This patch adds serial_out() to struct serial8250_em_hw_info to handle
> > > this difference between emma mobile and rz/v2m.
> > >
> > > DLL/DLM register can be updated only by setting LCR[7]. So the
> > > updation of LCR[7] will perform reset for DLL/DLM register changes.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > >  drivers/tty/serial/8250/8250_em.c | 70
> > > ++++++++++++++++++++++++++++++-
> > >  1 file changed, 69 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_em.c
> > > b/drivers/tty/serial/8250/8250_em.c
> > > index 69cd3b611501..c1c64f48ee7e 100644
> > > --- a/drivers/tty/serial/8250/8250_em.c
> > > +++ b/drivers/tty/serial/8250/8250_em.c
> > > @@ -17,12 +17,23 @@
> > >
> > >  #include "8250.h"
> > >
> > > +#define UART_FCR_EM 3
> > >  #define UART_DLL_EM 9
> > >  #define UART_DLM_EM 10
> > > +#define UART_HCR0_EM 11
> > > +
> > > +#define UART_FCR_R_EM	(UART_FCR_EM + UART_HCR0_EM)
> > 
> > It's easy to lose track of all this, IMHO this would be simple:
> > 
> > /*
> >  * A high value for UART_FCR_EM avoids overlapping with existing UART_*
> >  * register defines. UART_FCR_EM_HW is the real HW register offset.
> >  */
> > #define UART_FCR_EM 12
> 
> I will change it to #define UART_FCR_EM 14 
> 
> And will add the below unused HW status registers in the driver.
> 
> #define UART_HCR2_EM 12 (@30)
> #define UART_HCR3_EM 13 (@34)
> 
> Is it ok?

It's okay, that number is pseudo one anyway so the actual number doesn't 
matter. One could just as well pick some large number such as 0x10003 or
so if the collision with real regs is a concern.

-- 
 i.