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

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

On Fri, 10 Feb 2023, Biju Das wrote:

> Hi Ilpo,
> 
> Thanks for the update.
> 
> > Subject: Re: [PATCH v2 3/3] serial: 8250_em: Add serial_out() to struct
> > serial8250_em_hw_info
> > 
> > On Fri, 10 Feb 2023, Biju Das wrote:
> > 
> > > As per HW manual section 40.6.1, we need to perform FIFO reset + SW
> > > reset before updating the below registers.
> > >
> > > FCR[7:5], FCR[3:0], LCR[7][5:0], MCR[6:4], DLL[7:0], DLM[7:0] and
> > > HCR0[6:5][3:2].
> > >
> > > 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>
> > > ---
> > > v1->v2:
> > >  * Added serial_out to struct serial8250_em_hw_info
> > 
> > Thanks, this looks much better. One additional comment below.
> > 
> > > ---
> > >  drivers/tty/serial/8250/8250_em.c | 59
> > > ++++++++++++++++++++++++++++++-
> > >  1 file changed, 58 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_em.c
> > > b/drivers/tty/serial/8250/8250_em.c
> > > index 628a6846bfdc..1816362a3a3a 100644
> > > --- a/drivers/tty/serial/8250/8250_em.c
> > > +++ b/drivers/tty/serial/8250/8250_em.c
> > > @@ -19,10 +19,14 @@
> > >
> > >  #define UART_DLL_EM 9
> > >  #define UART_DLM_EM 10
> > > +#define UART_HCR0 11
> > > +
> > > +#define UART_HCR0_SW_RESET	BIT(7) /* SW Reset */
> > >
> > >  struct serial8250_em_hw_info {
> > >  	unsigned int type;
> > >  	upf_t flags;
> > > +	void (*serial_out)(struct uart_port *p, int off, int value);
> > >  };
> > >
> > >  struct serial8250_em_priv {
> > > @@ -31,6 +35,40 @@ struct serial8250_em_priv {
> > >  	const struct serial8250_em_hw_info *info;  };
> > >
> > > +static void serial8250_rzv2m_reg_update(struct uart_port *p, int off,
> > > +int value) {
> > > +	unsigned int ier, fcr, lcr, mcr, hcr0;
> > > +
> > > +	ier = readl(p->membase + (UART_IER << 2));
> > > +	hcr0 = readl(p->membase + (UART_HCR0 << 2));
> > > +	fcr = readl(p->membase + ((UART_FCR + 1) << 2));
> > > +	lcr = readl(p->membase + ((UART_LCR + 1) << 2));
> > > +	mcr = readl(p->membase + ((UART_MCR + 1) << 2));
> > > +
> > > +	writel(fcr | UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT,
> > > +	       p->membase + ((UART_FCR + 1) << 2));
> > > +	writel(hcr0 | UART_HCR0_SW_RESET, p->membase + (UART_HCR0 << 2));
> > > +	writel(hcr0 & ~UART_HCR0_SW_RESET, p->membase + (UART_HCR0 << 2));
> > > +
> > > +	switch (off) {
> > > +	case UART_FCR:
> > > +		fcr = value;
> > > +		break;
> > > +	case UART_LCR:
> > > +		lcr = value;
> > > +		break;
> > > +	case UART_MCR:
> > > +		mcr = value;
> > > +		break;
> > > +	}
> > > +
> > > +	writel(ier, p->membase + (UART_IER << 2));
> > > +	writel(fcr, p->membase + ((UART_FCR + 1) << 2));
> > > +	writel(mcr, p->membase + ((UART_MCR + 1) << 2));
> > > +	writel(lcr, p->membase + ((UART_LCR + 1) << 2));
> > > +	writel(hcr0, p->membase + (UART_HCR0 << 2));
> > 
> > Perhaps it would make sense to instead of using readl/writel() directly to
> > call serial8250_em_serial_in/out() so all the offset trickery wouldn't need
> > to be duplicated inside this function?
> 
> HCR0 register is not available for emma mobile. Is it ok if I just do readl/writel for
> that register and rest will use serial8250_em_serial_in/out()??

Yes, it's fine.

-- 
 i.