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@xxxxxxxxxxxxxx>
> > ---
> > 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()??
Cheers,
Biju
>
> --
> i.
>
> > +}
> > +
> > static void serial8250_em_serial_out(struct uart_port *p, int offset,
> > int value) {
> > switch (offset) {
> > @@ -52,6 +90,23 @@ static void serial8250_em_serial_out(struct uart_port
> *p, int offset, int value)
> > }
> > }
> >
> > +static void serial8250_em_rzv2m_serial_out(struct uart_port *p, int
> > +offset, int value) {
> > + switch (offset) {
> > + case UART_TX:
> > + case UART_SCR:
> > + case UART_IER:
> > + case UART_DLL_EM:
> > + case UART_DLM_EM:
> > + serial8250_em_serial_out(p, offset, value);
> > + break;
> > + case UART_FCR:
> > + case UART_LCR:
> > + case UART_MCR:
> > + serial8250_rzv2m_reg_update(p, offset, value);
> > + }
> > +}
> > +
> > static unsigned int serial8250_em_serial_in(struct uart_port *p, int
> > offset) {
> > switch (offset) {
> > @@ -119,7 +174,7 @@ static int serial8250_em_probe(struct
> > platform_device *pdev)
> >
> > up.port.iotype = UPIO_MEM32;
> > up.port.serial_in = serial8250_em_serial_in;
> > - up.port.serial_out = serial8250_em_serial_out;
> > + up.port.serial_out = priv->info->serial_out;
> > up.dl_read = serial8250_em_serial_dl_read;
> > up.dl_write = serial8250_em_serial_dl_write;
> >
> > @@ -144,11 +199,13 @@ static int serial8250_em_remove(struct
> > platform_device *pdev) static const struct serial8250_em_hw_info
> emma_mobile_uart_hw_info = {
> > .type = PORT_UNKNOWN,
> > .flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP,
> > + .serial_out = serial8250_em_serial_out,
> > };
> >
> > static const struct serial8250_em_hw_info rzv2m_uart_hw_info = {
> > .type = PORT_16750,
> > .flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP |
> > UPF_FIXED_TYPE,
> > + .serial_out = serial8250_em_rzv2m_serial_out,
> > };
> >
> > static const struct of_device_id serial8250_em_dt_ids[] = {
> >
