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

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

On Mon, 13 Feb 2023, Jiri Slaby wrote:

> On 13. 02. 23, 10:10, Jiri Slaby wrote:
> > On 13. 02. 23, 9:53, Biju Das wrote:
> > > > > +static void serial8250_rzv2m_reg_update(struct uart_port *p, int off,
> > > > > +int value) {
> > > > > +    unsigned int ier, fcr, lcr, mcr, hcr0;
> > > > > +
> > > > > +    ier = serial8250_em_serial_in(p, UART_IER);
> > > > > +    lcr = serial8250_em_serial_in(p, UART_LCR);
> > > > > +    mcr = serial8250_em_serial_in(p, UART_MCR);
> > > > > +    hcr0 = serial8250_em_serial_in(p, UART_HCR0);
> > > > > +    /*
> > > > > +     * The value of UART_IIR and UART_FCR are 2, but corresponding
> > > > > +     * RZ/V2M address offset are different(0x08 and 0x0c). So we need
> > > > > to
> > > > > +     * use readl() here.
> > > > > +     */
> > > > > +    fcr = readl(p->membase + ((UART_FCR + 1) << 2));
> > > > 
> > > > I don't get the meaning of that comment. It doesn't seem to match what
> > > > your
> > > > code does as the code seemingly has nothing to do with IIR (and none of
> > > > you
> > > > changelogs refer to IIR either)?
> > > 
> > > The generic macro UART_IIR and UART_FCR in linux/serial_reg.h has a value
> > > of 2.
> > 
> > Sure, IIR is normally WO and FCR RO and share the same register. I would
> > simply define UART_FCR_RZ (or alike)
> 
> Or even UART_FCR_RO_RZ?
> 
> > for 0x12.
> 
> I mean 12 or 0xc.

Won't that collide with LCR reads then? They are currently mapped by 
return 0; but after adding a case for UART_FCR_RO_RZ they'll read from 
what is the FCR register on this HW?


-- 
 i.