Hi Andy,
Thanks for the feedback.
> Subject: Re: [PATCH v4 6/6] serial: 8250_em: Add
> serial8250_em_{reg_update(),out_helper()}
>
> On Fri, Feb 17, 2023 at 11:42:55AM +0000, Biju Das wrote:
> > As per RZ/V2M hardware manual(Rev.1.30 Jun, 2022), UART IP has a
> > restriction as mentioned below.
> >
> > 40.6.1 Point for Caution when Changing the Register Settings:
> >
> > When changing the settings of the following registers, a PRESETn
> > master reset or FIFO reset + SW reset (FCR[2],FCR[1], HCR0[7]) must be
> > input to re-initialize them.
> >
> > Target Registers: FCR, LCR, MCR, DLL, DLM, HCR0.
> >
> > This patch adds serial8250_em_reg_update() and serial8250_em_serial_
> > out_helper to handle it.
> >
> > 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.
> >
> > EMMA mobile has the same register set as RZ/V2M and this patch is
> > tested on
> > EMEV2 board. So, there is no harm in applying the same restriction
> > here as well as the HW manual for EMMA mobile is not updated for a long
> time.
>
> ...
>
> > + serial8250_em_serial_out_helper(p, UART_FCR_EM, fcr |
> UART_FCR_CLEAR_RCVR |
> > + UART_FCR_CLEAR_XMIT);
>
>
> I would put it like
>
> serial8250_em_serial_out_helper(p, UART_FCR_EM, fcr |
> UART_FCR_CLEAR_RCVR |
> UART_FCR_CLEAR_XMIT);
>
> ...
OK.
>
> > + switch (off) {
> > + case UART_FCR_EM:
> > + fcr = value;
> > + break;
> > + case UART_LCR:
> > + lcr = value;
> > + break;
> > + case UART_MCR:
> > + mcr = value;
>
> Missing break; statement.
OK, will fix this in next version.
The original driver has some missing breaks[1].
So for consistency I dropped this. same for below.
[1] https://elixir.bootlin.com/linux/v6.2-rc8/source/drivers/tty/serial/8250/8250_em.c#L45
Cheers,
Biju
>
> > + }
>
> ...
>
> > + switch (offset) {
> > + case UART_TX:
> > + case UART_SCR:
> > + case UART_IER:
> > + case UART_DLL_EM:
> > + case UART_DLM_EM:
> > + serial8250_em_serial_out_helper(p, offset, value);
> > + break;
> > + case UART_FCR:
> > + serial8250_em_reg_update(p, UART_FCR_EM, value);
> > + break;
> > + case UART_LCR:
> > + case UART_MCR:
> > + serial8250_em_reg_update(p, offset, value);
>
> Missing break; statement.
>
> > + }
>
> --
> With Best Regards,
> Andy Shevchenko
>
