| Message ID | 20230209132630.194947-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add Identification and 64 bit fifo support to Renesas RZ/V2M 16750 UART | expand |
On Thu, 9 Feb 2023, Biju Das wrote: > Add identification support for RZ/V2M 16750 UART. > > Currently, RZ/V2M UART is detected as 16550A instead of 16750. > "a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 14, base_baud = 3000000) > is a 16550A" > > After adding identification support, it is detected as > "a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24, base_baud = 3000000) > is a Renesas RZ/V2M 16750". > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > drivers/tty/serial/8250/8250_port.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index e61753c295d5..e4b205e3756b 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -111,6 +111,15 @@ static const struct serial8250_config uart_config[] = { > .rxtrig_bytes = {1, 16, 32, 56}, > .flags = UART_CAP_FIFO | UART_CAP_SLEEP | UART_CAP_AFE, > }, > + [PORT_16750] = { > + .name = "Renesas RZ/V2M 16750", > + .fifo_size = 64, > + .tx_loadsz = 64, > + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 | > + UART_FCR7_64BYTE, > + .rxtrig_bytes = {1, 16, 32, 56}, > + .flags = UART_CAP_FIFO | UART_CAP_AFE, > + }, Eh, how can you reuse [PORT_16750] again in the initializer like that?
Hi Ilpo Järvinen, Thanks for the feedback. > -----Original Message----- > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Sent: Thursday, February 9, 2023 2:09 PM > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby > <jirislaby@kernel.org>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>; > Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; Maciej W. Rozycki > <macro@orcam.me.uk>; Eric Tremblay <etremblay@distech-controls.com>; Wander > Lairson Costa <wander@redhat.com>; linux-serial <linux- > serial@vger.kernel.org>; Geert Uytterhoeven <geert+renesas@glider.be>; > Fabrizio Castro <fabrizio.castro.jz@renesas.com>; linux-renesas- > soc@vger.kernel.org > Subject: Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART > > On Thu, 9 Feb 2023, Biju Das wrote: > > > Add identification support for RZ/V2M 16750 UART. > > > > Currently, RZ/V2M UART is detected as 16550A instead of 16750. > > "a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 14, base_baud = > > 3000000) is a 16550A" > > > > After adding identification support, it is detected as > > "a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24, base_baud = > > 3000000) is a Renesas RZ/V2M 16750". > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > drivers/tty/serial/8250/8250_port.c | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/drivers/tty/serial/8250/8250_port.c > > b/drivers/tty/serial/8250/8250_port.c > > index e61753c295d5..e4b205e3756b 100644 > > --- a/drivers/tty/serial/8250/8250_port.c > > +++ b/drivers/tty/serial/8250/8250_port.c > > @@ -111,6 +111,15 @@ static const struct serial8250_config uart_config[] = > { > > .rxtrig_bytes = {1, 16, 32, 56}, > > .flags = UART_CAP_FIFO | UART_CAP_SLEEP | UART_CAP_AFE, > > }, > > + [PORT_16750] = { > > + .name = "Renesas RZ/V2M 16750", > > + .fifo_size = 64, > > + .tx_loadsz = 64, > > + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 | > > + UART_FCR7_64BYTE, > > + .rxtrig_bytes = {1, 16, 32, 56}, > > + .flags = UART_CAP_FIFO | UART_CAP_AFE, > > + }, > > Eh, how can you reuse [PORT_16750] again in the initializer like that? Oops. Missed it. Is it ok to introduce PORT_RENESAS_16750_F64 instead as PORT_16750 is used by TI16750? Cheers, Biju > > -- > i. > > > [PORT_STARTECH] = { > > .name = "Startech", > > .fifo_size = 1, > > @@ -1142,6 +1151,24 @@ static void autoconfig_16550a(struct uart_8250_port > *up) > > return; > > } > > > > + /* > > + * No EFR. Try to detect a Renesas RZ/V2M 16750, which only sets bit > 5 > > + * of the IIR when 64 byte FIFO mode is enabled. > > + * Try setting/clear bit5 of FCR. > > + */ > > + serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO); > > + status1 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | > > +UART_IIR_FIFO_ENABLED); > > + > > + serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO | UART_FCR7_64BYTE); > > + status2 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | > > +UART_IIR_FIFO_ENABLED); > > + > > + if (status1 == UART_IIR_FIFO_ENABLED_16550A && > > + status2 == (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED_16550A)) > { > > + up->port.type = PORT_16750; > > + up->capabilities |= UART_CAP_AFE; > > + return; > > + } > > + > > /* > > * Try writing and reading the UART_IER_UUE bit (b6). > > * If it works, this is probably one of the Xscale platform's > >
On Thu, Feb 09, 2023 at 02:28:55PM +0000, Biju Das wrote: > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Sent: Thursday, February 9, 2023 2:09 PM > > On Thu, 9 Feb 2023, Biju Das wrote: > > > + [PORT_16750] = { > > > + .name = "Renesas RZ/V2M 16750", > > > + .fifo_size = 64, > > > + .tx_loadsz = 64, > > > + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 | > > > + UART_FCR7_64BYTE, > > > + .rxtrig_bytes = {1, 16, 32, 56}, > > > + .flags = UART_CAP_FIFO | UART_CAP_AFE, > > > + }, > > > > Eh, how can you reuse [PORT_16750] again in the initializer like that? > > Oops. Missed it. Is it ok to introduce PORT_RENESAS_16750_F64 instead > as PORT_16750 is used by TI16750? What the difference to the 16750 from TI that prevents you from using it?
Hi Andy Shevchenko, Thanks for the feedback. > Subject: Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART > > On Thu, Feb 09, 2023 at 02:28:55PM +0000, Biju Das wrote: > > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > Sent: Thursday, February 9, 2023 2:09 PM On Thu, 9 Feb 2023, Biju > > > Das wrote: > > > > > + [PORT_16750] = { > > > > + .name = "Renesas RZ/V2M 16750", > > > > + .fifo_size = 64, > > > > + .tx_loadsz = 64, > > > > + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 | > > > > + UART_FCR7_64BYTE, > > > > + .rxtrig_bytes = {1, 16, 32, 56}, > > > > + .flags = UART_CAP_FIFO | UART_CAP_AFE, > > > > + }, > > > > > > Eh, how can you reuse [PORT_16750] again in the initializer like that? > > > > Oops. Missed it. Is it ok to introduce PORT_RENESAS_16750_F64 instead > > as PORT_16750 is used by TI16750? > > What the difference to the 16750 from TI that prevents you from using it? Mostly it is identical. The main difference is detection method, and we don't have UART_IERX_SLEEP bit in IER. On TI, it sets bit 5 of the IIR when 64-byte FIFO mode is enabled when DLAB is set. Whereas in our case DLAB does n't have any role for Identification, It set bit 5 of the IIR when 64 byte FIFO mode is enabled. and it clears bit 5 of the IIR when 64 byte FIFO mode is disabled. Other than that, when I use PORT_16750 type and capabilities in 8250_em driver and add identification method for Renesas UART in 8250_port driver, It detected as PORT_16750 UART, but I get below prints during autoconf which is confusing for the end user [ 0.214926] serial8250-em a4040000.serial: detected caps 00000900 should be 00000d00 [ 0.214975] a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24, base_baud = 3000000) is a TI16750 Modification in 8250_em driver + up.port.type = PORT_16750; + up.port.name = "Renesas RZ/V2M 16750"; + up.port.fifosize = 64; + up.tx_loadsz = 64; + up.capabilities = UART_CAP_FIFO | UART_CAP_AFE; Identification method in 8250_port.c driver + /* + * No EFR. Try to detect a Renesas RZ/V2M 16750, which only sets bit 5 + * of the IIR when 64 byte FIFO mode is enabled. + * Try setting/clear bit5 of FCR. + */ + serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO); + status1 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED); + + serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO | UART_FCR7_64BYTE); + status2 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED); + + if (status1 == UART_IIR_FIFO_ENABLED_16550A && + status2 == (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED_16550A)) { + up->port.type = PORT_16750; + up->capabilities |= UART_CAP_AFE; + return; + } + Cheers, Biju
On Fri, Feb 10, 2023 at 07:14:54AM +0000, Biju Das wrote: > > Subject: Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART > > On Thu, Feb 09, 2023 at 02:28:55PM +0000, Biju Das wrote: > > > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > > Sent: Thursday, February 9, 2023 2:09 PM On Thu, 9 Feb 2023, Biju > > > > Das wrote: ... > > > > > + [PORT_16750] = { > > > > > + .name = "Renesas RZ/V2M 16750", > > > > > + .fifo_size = 64, > > > > > + .tx_loadsz = 64, > > > > > + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 | > > > > > + UART_FCR7_64BYTE, > > > > > + .rxtrig_bytes = {1, 16, 32, 56}, > > > > > + .flags = UART_CAP_FIFO | UART_CAP_AFE, > > > > > + }, > > > > > > > > Eh, how can you reuse [PORT_16750] again in the initializer like that? > > > > > > Oops. Missed it. Is it ok to introduce PORT_RENESAS_16750_F64 instead > > > as PORT_16750 is used by TI16750? > > > > What the difference to the 16750 from TI that prevents you from using it? > > Mostly it is identical. > > The main difference is detection method, and we don't have UART_IERX_SLEEP bit in IER. > > On TI, it sets bit 5 of the IIR when 64-byte FIFO mode is enabled when DLAB is set. > > Whereas in our case DLAB does n't have any role for Identification, > > It set bit 5 of the IIR when 64 byte FIFO mode is enabled. > and it clears bit 5 of the IIR when 64 byte FIFO mode is disabled. So the question here is do these minor deviations affect the actual functionality? Note, on Intel hardware we use directly TI16750 while we have no sleep functionality available IIRC. Ilpo may correct me if I'm wrong. > Other than that, when I use PORT_16750 type and capabilities in 8250_em driver and > add identification method for Renesas UART in 8250_port driver, > > It detected as PORT_16750 UART, but I get below prints during autoconf which is confusing for the end user > > [ 0.214926] serial8250-em a4040000.serial: detected caps 00000900 should be 00000d00 > [ 0.214975] a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24, base_baud = 3000000) is a TI16750 > > > Modification in 8250_em driver > > + up.port.type = PORT_16750; > + up.port.name = "Renesas RZ/V2M 16750"; > + up.port.fifosize = 64; > + up.tx_loadsz = 64; > + up.capabilities = UART_CAP_FIFO | UART_CAP_AFE; > > Identification method in 8250_port.c driver > > + /* > + * No EFR. Try to detect a Renesas RZ/V2M 16750, which only sets bit 5 > + * of the IIR when 64 byte FIFO mode is enabled. > + * Try setting/clear bit5 of FCR. > + */ > + serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO); > + status1 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED); > + > + serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO | UART_FCR7_64BYTE); > + status2 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED); > + > + if (status1 == UART_IIR_FIFO_ENABLED_16550A && > + status2 == (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED_16550A)) { > + up->port.type = PORT_16750; > + up->capabilities |= UART_CAP_AFE; > + return; > + } What I don't like is increasing quirks in the 8250_port. Can't you simply use FIXED_PORT facility? Again, look how 8250_mid is written.
Hi Andy Shevchenko, Thanks for the feedback. > Subject: Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART > > On Fri, Feb 10, 2023 at 07:14:54AM +0000, Biju Das wrote: > > > Subject: Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 > > > UART On Thu, Feb 09, 2023 at 02:28:55PM +0000, Biju Das wrote: > > > > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > > > Sent: Thursday, February 9, 2023 2:09 PM On Thu, 9 Feb 2023, > > > > > Biju Das wrote: > > ... > > > > > > > + [PORT_16750] = { > > > > > > + .name = "Renesas RZ/V2M 16750", > > > > > > + .fifo_size = 64, > > > > > > + .tx_loadsz = 64, > > > > > > + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 | > > > > > > + UART_FCR7_64BYTE, > > > > > > + .rxtrig_bytes = {1, 16, 32, 56}, > > > > > > + .flags = UART_CAP_FIFO | UART_CAP_AFE, > > > > > > + }, > > > > > > > > > > Eh, how can you reuse [PORT_16750] again in the initializer like > that? > > > > > > > > Oops. Missed it. Is it ok to introduce PORT_RENESAS_16750_F64 > > > > instead as PORT_16750 is used by TI16750? > > > > > > What the difference to the 16750 from TI that prevents you from using > it? > > > > Mostly it is identical. > > > > The main difference is detection method, and we don't have UART_IERX_SLEEP > bit in IER. > > > > On TI, it sets bit 5 of the IIR when 64-byte FIFO mode is enabled when > DLAB is set. > > > > Whereas in our case DLAB does n't have any role for Identification, > > > > It set bit 5 of the IIR when 64 byte FIFO mode is enabled. > > and it clears bit 5 of the IIR when 64 byte FIFO mode is disabled. > > So the question here is do these minor deviations affect the actual > functionality? OK. > > Note, on Intel hardware we use directly TI16750 while we have no sleep > functionality available IIRC. Ilpo may correct me if I'm wrong. > > > Other than that, when I use PORT_16750 type and capabilities in > > 8250_em driver and add identification method for Renesas UART in > > 8250_port driver, > > > > It detected as PORT_16750 UART, but I get below prints during autoconf > > which is confusing for the end user > > > > [ 0.214926] serial8250-em a4040000.serial: detected caps 00000900 > should be 00000d00 > > [ 0.214975] a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24, > base_baud = 3000000) is a TI16750 > > > > > > Modification in 8250_em driver > > > > + up.port.type = PORT_16750; > > + up.port.name = "Renesas RZ/V2M 16750"; > > + up.port.fifosize = 64; > > + up.tx_loadsz = 64; > > + up.capabilities = UART_CAP_FIFO | UART_CAP_AFE; > > > > Identification method in 8250_port.c driver > > > > + /* > > + * No EFR. Try to detect a Renesas RZ/V2M 16750, which only sets > bit 5 > > + * of the IIR when 64 byte FIFO mode is enabled. > > + * Try setting/clear bit5 of FCR. > > + */ > > + serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO); > > + status1 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | > > + UART_IIR_FIFO_ENABLED); > > + > > + serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO | UART_FCR7_64BYTE); > > + status2 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | > > + UART_IIR_FIFO_ENABLED); > > + > > + if (status1 == UART_IIR_FIFO_ENABLED_16550A && > > + status2 == (UART_IIR_64BYTE_FIFO | > UART_IIR_FIFO_ENABLED_16550A)) { > > + up->port.type = PORT_16750; > > + up->capabilities |= UART_CAP_AFE; > > + return; > > + } > > What I don't like is increasing quirks in the 8250_port. Can't you simply > use FIXED_PORT facility? > Again, look how 8250_mid is written. Coll. I have referred 8250_mid and added below changes and it is working as expected. I will sent next version after incorporating comments from Geert and ilpo. + up.port.type = PORT_16750; + up.port.flags |= UPF_FIXED_TYPE; a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24, base_baud = 3000000) is a TI16750 root@rzv2m:~# /uart_dump.sh Read at address 0xA4040000 (0xffff933ec000): 0x00000000 Read at address 0xA4040004 (0xffffb4994004): 0x00000005 Read at address 0xA4040008 (0xffff8041c008): 0x000000E1 Read at address 0xA404000C (0xffffbb62100c): 0x000000A1 Read at address 0xA4040010 (0xffff94158010): 0x00000013 Read at address 0xA4040014 (0xffff8018c014): 0x0000000B Read at address 0xA4040018 (0xffff971af018): 0x00000060 Read at address 0xA404001C (0xffffb057d01c): 0x00000010 Read at address 0xA4040020 (0xffff8e8b6020): 0x00000000 Read at address 0xA4040024 (0xffff8094a024): 0x0000001A Read at address 0xA4040028 (0xffffb6e81028): 0x00000000 Read at address 0xA404002C (0xffffa663902c): 0x00000000 root@rzv2m:~# stty -F /dev/ttyS0 115200 crtscts root@rzv2m:~# /uart_dump.sh Read at address 0xA4040000 (0xffff9672f000): 0x00000000 Read at address 0xA4040004 (0xffffb4d63004): 0x0000000D Read at address 0xA4040008 (0xffffbdd07008): 0x000000E1 Read at address 0xA404000C (0xffff9573a00c): 0x000000A1 Read at address 0xA4040010 (0xffffbd5be010): 0x00000013 Read at address 0xA4040014 (0xffff9451c014): 0x0000002B Read at address 0xA4040018 (0xffffb47cb018): 0x00000060 Read at address 0xA404001C (0xffffa680901c): 0x00000010 Read at address 0xA4040020 (0xffffb76d1020): 0x00000000 Read at address 0xA4040024 (0xffffb8667024): 0x0000001A Read at address 0xA4040028 (0xffffad9c8028): 0x00000000 Read at address 0xA404002C (0xffff8f46702c): 0x00000000 Cheers, Biju
On Fri, Feb 10, 2023 at 11:53:43AM +0000, Biju Das wrote: > > Subject: Re: [PATCH 1/3] serial: 8250: Identify Renesas RZ/V2M 16750 UART > > On Fri, Feb 10, 2023 at 07:14:54AM +0000, Biju Das wrote: ... > > What I don't like is increasing quirks in the 8250_port. Can't you simply > > use FIXED_PORT facility? > > Again, look how 8250_mid is written. > > Coll. I have referred 8250_mid and added below changes and it is working as expected. > I will sent next version after incorporating comments from Geert and ilpo. Glad to hear that and thank you for cooperation!
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index e61753c295d5..e4b205e3756b 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -111,6 +111,15 @@ static const struct serial8250_config uart_config[] = { .rxtrig_bytes = {1, 16, 32, 56}, .flags = UART_CAP_FIFO | UART_CAP_SLEEP | UART_CAP_AFE, }, + [PORT_16750] = { + .name = "Renesas RZ/V2M 16750", + .fifo_size = 64, + .tx_loadsz = 64, + .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 | + UART_FCR7_64BYTE, + .rxtrig_bytes = {1, 16, 32, 56}, + .flags = UART_CAP_FIFO | UART_CAP_AFE, + }, [PORT_STARTECH] = { .name = "Startech", .fifo_size = 1, @@ -1142,6 +1151,24 @@ static void autoconfig_16550a(struct uart_8250_port *up) return; } + /* + * No EFR. Try to detect a Renesas RZ/V2M 16750, which only sets bit 5 + * of the IIR when 64 byte FIFO mode is enabled. + * Try setting/clear bit5 of FCR. + */ + serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO); + status1 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED); + + serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO | UART_FCR7_64BYTE); + status2 = serial_in(up, UART_IIR) & (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED); + + if (status1 == UART_IIR_FIFO_ENABLED_16550A && + status2 == (UART_IIR_64BYTE_FIFO | UART_IIR_FIFO_ENABLED_16550A)) { + up->port.type = PORT_16750; + up->capabilities |= UART_CAP_AFE; + return; + } + /* * Try writing and reading the UART_IER_UUE bit (b6). * If it works, this is probably one of the Xscale platform's
Add identification support for RZ/V2M 16750 UART. Currently, RZ/V2M UART is detected as 16550A instead of 16750. "a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 14, base_baud = 3000000) is a 16550A" After adding identification support, it is detected as "a4040000.serial: ttyS0 at MMIO 0xa4040000 (irq = 24, base_baud = 3000000) is a Renesas RZ/V2M 16750". Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/tty/serial/8250/8250_port.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)