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: 2484 bytes --]

On Mon, 13 Feb 2023, Jiri Slaby wrote:

> On 13. 02. 23, 10:22, Ilpo Järvinen wrote:
> > 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?
> 
> LCR is WO, right? But maybe I'm confused by this really weird HW design?

LCR is not WO and it is read during autoconfig too:

8250_port.c=static int size_fifo(struct uart_8250_port *up)
8250_port.c:    old_lcr = serial_in(up, UART_LCR);
8250_port.c=static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
8250_port.c:    old_lcr = serial_in(p, UART_LCR);
8250_port.c=static void autoconfig(struct uart_8250_port *up)
8250_port.c:    save_lcr = serial_in(up, UART_LCR);

Although the only thing the value is used for is to write it back to LCR.


-- 
 i.