Hi Ilpo,
Thanks for the feedback.
> Subject: RE: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for UART_FCR
>
> On Fri, 17 Feb 2023, Biju Das wrote:
>
> > HI Ilpo,
> >
> > > Subject: RE: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset for
> > > UART_FCR
> > >
> > > On Fri, 17 Feb 2023, Biju Das wrote:
> > >
> > > > Hi Ilpo,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > Subject: Re: [PATCH v4 5/6] serial: 8250_em: Use pseudo offset
> > > > > for UART_FCR
> > > > >
> > > > > On Fri, 17 Feb 2023, Biju Das wrote:
> > > > >
> > > > > > UART_FCR shares the same offset with UART_IIR. We cannot use
> > > > > > UART_FCR in serial8250_em_serial_in() as it overlaps with
> UART_IIR.
> > > > > >
> > > > > > This patch introduces a macro UART_FCR_EM with a high value to
> > > > > > avoid overlapping with existing UART_* register defines.
> > > > > >
> > > > > > This will help us to use UART_FCR_EM consistently in
> > > > > > serial8250_em_ serial{_in/_out} function to read/write FCR
> register.
> > > > > >
> > > > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > > > > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > > > > > ---
> > > > > > v4:
> > > > > > * New patch. Used UART_FCR_EM for read/write of FCR register.
> > > > > > ---
> > > > > > drivers/tty/serial/8250/8250_em.c | 13 ++++++++++++-
> > > > > > 1 file changed, 12 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/tty/serial/8250/8250_em.c
> > > > > > b/drivers/tty/serial/8250/8250_em.c
> > > > > > index 499d7a8847ec..4165fd3bb17a 100644
> > > > > > --- a/drivers/tty/serial/8250/8250_em.c
> > > > > > +++ b/drivers/tty/serial/8250/8250_em.c
> > > > > > @@ -19,6 +19,13 @@
> > > > > > #define UART_DLL_EM 9
> > > > > > #define UART_DLM_EM 10
> > > > > >
> > > > > > +/*
> > > > > > + * A high value for UART_FCR_EM avoids overlapping with
> > > > > > +existing
> > > > > > +UART_*
> > > > > > + * register defines. UART_FCR_EM_HW is the real HW register
> offset.
> > > > > > + */
> > > > > > +#define UART_FCR_EM 0x10003
> > > > > > +#define UART_FCR_EM_HW 3
> > > > > > +
> > > > > > struct serial8250_em_priv {
> > > > > > int line;
> > > > > > };
> > > > > > @@ -29,12 +36,14 @@ static void
> > > > > > serial8250_em_serial_out(struct uart_port
> > > > > *p, int offset, int value)
> > > > > > case UART_TX: /* TX @ 0x00 */
> > > > > > writeb(value, p->membase);
> > > > > > break;
> > > > > > - case UART_FCR: /* FCR @ 0x0c (+1) */
> > > > >
> > > > > 8250_port wants this to remain in place, I think. Otherwise it's
> > > > > attempts to set UART_FCR will end up into wrong destination.
> > > >
> > > > Oops, next patch has this change.
> > > >
> > > > + case UART_FCR:
> > > > + serial8250_em_reg_update(p, UART_FCR_EM, value);
> > > >
> > > > I just need to keep UART_FCR for this patch and remove it from
> > > > "serial8250_em_serial_out_helper" on the next patch
> > >
> > > IMHO, the extra layer + _helper seems pretty unnecessary. I don't
> > > see any useful thing it achieves over just having to serial_out
> functions.
> >
> > Without helper, it will become cyclic right??
> >
> > serial8250_em_serial_out() needs to call serial8250_em_reg_update()
> >
> > serial8250_em_reg_update() will call serial8250_em_serial_out() for
> writes??
>
> With your most recent code, yes it seems to happen. But why did you remove
> the separate serial_out for RZ. There wasn't any cyclicness when it called
> serial8250_em_serial_out().
>
> I'm a bit lost now, are you saying that this change is needed for all
> devices 8250_em supports (which is different from what you initially
> presented in the early versions of this patchset)?
Yes, That is correct. Please see the discussion thread related to this[1].
Geert pointed out that UART register sets between RZ/V2M and
EMMA mobile SoC are almost identical.
"According to R19UH0040EJ0400 Rev.4.00 it is available on EMEV2, and the
layout looks identical to RZ/V2M."
Niklas tested previous patch series on EMEV2 board and
It detected port type as 16750 and done read/write test
with 64-bytes fifo enabled.
EMMA mobile SoC is very old and hardware manual
is not updated for long time related.
So we guess it is safe to apply this restriction for this SoC
aswell, since there is no regression.
[1]
https://lore.kernel.org/linux-renesas-soc/8585f736-cf3b-b63c-753f-892d4051ada3@xxxxxxxxxxxxxxx/T/#mf5e3059e792ab1a5dd96a769b9c69ae6befb25c5
Cheers,
Biju
