| Message ID | 20230613132339.150671-6-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | RZ/V2M UART FIFO support | expand |
Hi! > commit 59d6558fb5fd750777edfde028ea1f9e7eed8a46 upstream. > > 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. I don't follow the argument. AFAICT you could simply define UART_FCR to UART_IIR, same readl() is used to read both. Yes, you'd have to add an comment to explain that this is the same register... Best regards, Pavel
Hi Pavel, Thanks for the feedback. > Subject: Re: [PATCH 5.10.y-cip 5/7] serial: 8250_em: Use pseudo offset for > UART_FCR > > Hi! > > > commit 59d6558fb5fd750777edfde028ea1f9e7eed8a46 upstream. > > > > 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. > > I don't follow the argument. AFAICT you could simply define UART_FCR to > UART_IIR, same readl() is used to read both. See https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/serial_reg.h#L53 UART_FCR=2 and UART_IIR=2 On reality from IP point, it is @0xC and @0x08. > > Yes, you'd have to add an comment to explain that this is the same > register... it is not same register. case UART_FCR: /* FCR @ 0x0c (+1) */ case UART_IIR: /* IIR @ 0x08 */ I already put comments and serial maintainer is OK with these. +/* + * 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 Also commit message has details 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. Define UART_FCR_EM macro with a high value to avoid overlapping with existing UART_* register defines and define another macro UART_FCR_EM_HW for the real offset. Use these macros in serial8250_em_serial{_in/_out} function to read/write FCR register. Cheers, Biju
Hi! > Thanks for the feedback. > > > Subject: Re: [PATCH 5.10.y-cip 5/7] serial: 8250_em: Use pseudo offset for > > UART_FCR > > > > Hi! > > > > > commit 59d6558fb5fd750777edfde028ea1f9e7eed8a46 upstream. > > > > > > 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. > > > > I don't follow the argument. AFAICT you could simply define UART_FCR to > > UART_IIR, same readl() is used to read both. > > See https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/serial_reg.h#L53 > > UART_FCR=2 and UART_IIR=2 > > On reality from IP point, it is @0xC and @0x08. Aha, thanks for explanation. So on some hardware IIR and FCR share the same register, but not on yours? It would be better if serial core provided different defines for the two registers, as current situation is quite confusing. Best regards, Pavel
diff --git a/drivers/tty/serial/8250/8250_em.c b/drivers/tty/serial/8250/8250_em.c index dcf1761e8ef5..7614ced9377e 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,15 @@ 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) */ case UART_LCR: /* LCR @ 0x10 (+1) */ case UART_MCR: /* MCR @ 0x14 (+1) */ case UART_SCR: /* SCR @ 0x20 (+1) */ writel(value, p->membase + ((offset + 1) << 2)); break; + case UART_FCR: + case UART_FCR_EM: + writel(value, p->membase + (UART_FCR_EM_HW << 2)); + break; case UART_IER: /* IER @ 0x04 */ value &= 0x0f; /* only 4 valid bits - not Xscale */ fallthrough; @@ -55,6 +65,8 @@ static unsigned int serial8250_em_serial_in(struct uart_port *p, int offset) case UART_MSR: /* MSR @ 0x1c (+1) */ case UART_SCR: /* SCR @ 0x20 (+1) */ return readl(p->membase + ((offset + 1) << 2)); + case UART_FCR_EM: + return readl(p->membase + (UART_FCR_EM_HW << 2)); case UART_IER: /* IER @ 0x04 */ case UART_IIR: /* IIR @ 0x08 */ case UART_DLL_EM: /* DLL @ 0x24 (+9) */
commit 59d6558fb5fd750777edfde028ea1f9e7eed8a46 upstream. 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. Define UART_FCR_EM macro with a high value to avoid overlapping with existing UART_* register defines and define another macro UART_FCR_EM_HW for the real offset. Use these macros in serial8250_em_serial{_in/_out} function to read/write FCR register. Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Link: https://lore.kernel.org/r/20230227114152.22265-7-biju.das.jz@bp.renesas.com Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/tty/serial/8250/8250_em.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)