Re: [PATCH v6 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver

[hammer hsieh]

Jiri Slaby <jirislaby@xxxxxxxxxx> 於 2022年1月13日 週四 下午3:06寫道：
>
> Hi,
>
> On 12. 01. 22, 10:24, Hammer Hsieh wrote:
> > Add Sunplus SoC UART Driver
> ...
> > --- /dev/null
> > +++ b/drivers/tty/serial/sunplus-uart.c
> > @@ -0,0 +1,756 @@
> ...
> > +/* Register offsets */
> > +#define SUP_UART_DATA                        0x00
> > +#define SUP_UART_LSR                 0x04
> > +#define SUP_UART_MSR                 0x08
> > +#define SUP_UART_LCR                 0x0C
> > +#define SUP_UART_MCR                 0x10
> > +#define SUP_UART_DIV_L                       0x14
> > +#define SUP_UART_DIV_H                       0x18
> > +#define SUP_UART_ISC                 0x1C
> > +#define SUP_UART_TX_RESIDUE          0x20
> > +#define SUP_UART_RX_RESIDUE          0x24
> > +
> > +/* Line Status Register bits */
> > +#define SUP_UART_LSR_BC                      BIT(5) /* break condition status */
> > +#define SUP_UART_LSR_FE                      BIT(4) /* frame error status */
> > +#define SUP_UART_LSR_OE                      BIT(3) /* overrun error status */
> > +#define SUP_UART_LSR_PE                      BIT(2) /* parity error status */
>
> I just wonder why do the HW creators feel so creative to redefine the
> world...
>

Our IC designer create it, I just try to make it work.
Indeed, before Greg KH mentioned 8250-like, I didn't realize our uart
so much like 8250.
Thanks for your review.

> > +static void sunplus_shutdown(struct uart_port *port)
> > +{
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&port->lock, flags);
> > +     writel(0, port->membase + SUP_UART_ISC);
> > +     spin_unlock_irqrestore(&port->lock, flags);
>
> I asked last time:
> * What bus is this -- posting?
>
> You replied:
> * Here just clear interrupt.
> * Not really understand your comment?
>
> So I am asking again:
> What bus is this? Isn't a posted write a problem here? I mean, shouldn't
> you read from the register so that the write hits the device? That
> depends on the bus this sits on, so just asking.
>

Each UART has its own ISC register.
Ex.
dev/ttySUP0 base_adr = 0x9C00-0000 , isc_addr = 0x9C00-001C
dev/ttySUP1 base_adr = 0x9C00-0080 , isc_addr = 0x9C00-009C
dev/ttySUP2 base_adr = 0x9C00-0100 , isc_addr = 0x9C00-011C
dev/ttySUP3 base_adr = 0x9C00-0180 , isc_addr = 0x9C00-019C
dev/ttySUP4 base_adr = 0x9C00-0200 , isc_addr = 0x9C00-021C
So sunplus_shutdown() just simply turn off its own device isc only.
That's why I didn't read register value, just write 0 for it.

> Other than that the driver looks much better now, i.e. LGTM.
>
> thanks,
> --
> js
> suse labs