* [PATCH 0/2] serial, Malta: Fixes to make the CBUS UART work big-endian @ 2021-06-10 18:37 Maciej W. Rozycki 2021-06-10 18:38 ` [PATCH 1/2] serial: 8250: Mask out floating 16/32-bit bus bits Maciej W. Rozycki 2021-06-10 18:38 ` [PATCH 2/2] MIPS: Malta: Do not byte-swap accesses to the CBUS UART Maciej W. Rozycki 0 siblings, 2 replies; 7+ messages in thread From: Maciej W. Rozycki @ 2021-06-10 18:37 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby, Thomas Bogendoerfer Cc: linux-serial, linux-mips, linux-kernel Hi, Earlier this year I noticed the CBUS UART, a discrete TI16C550C part wired directly to the system controller's device bus and supposed to come up as ttyS2 in addition to ttyS0 and ttyS1 ports from a Super I/O device behind the PCI southbridge, is not recognised with my MIPS Malta board booting big-endian. I used to use it just fine, many many years ago, although in the board's little-endian configuration only, and then with a local patch to get it supported with Linux 2.4.x, which I didn't get to submitting however due to the turn of events back then. Support was then added by someone else with 2.6.23. I got to the bottom of the problem now and as it turns out we have two long-standing bugs causing it, one in generic 8250 code and another in Malta platform code, and this has never worked in the big-endian mode. Evidently, this has never been verified, and I guess this is because back in the MIPS UK days we usually ran the boards in the little-endian mode. This pair of patches addresses these bugs individually. See the respective change descriptions for details. Please apply. Maciej ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] serial: 8250: Mask out floating 16/32-bit bus bits 2021-06-10 18:37 [PATCH 0/2] serial, Malta: Fixes to make the CBUS UART work big-endian Maciej W. Rozycki @ 2021-06-10 18:38 ` Maciej W. Rozycki 2021-06-11 6:07 ` Jiri Slaby ` (2 more replies) 2021-06-10 18:38 ` [PATCH 2/2] MIPS: Malta: Do not byte-swap accesses to the CBUS UART Maciej W. Rozycki 1 sibling, 3 replies; 7+ messages in thread From: Maciej W. Rozycki @ 2021-06-10 18:38 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby, Thomas Bogendoerfer Cc: linux-serial, linux-mips, linux-kernel Make sure only actual 8 bits of the IIR register are used in determining the port type in `autoconfig'. The `serial_in' port accessor returns the `unsigned int' type, meaning that with UPIO_AU, UPIO_MEM16, UPIO_MEM32, and UPIO_MEM32BE access types more than 8 bits of data are returned, of which the high order bits will often come from bus lines that are left floating in the data phase. For example with the MIPS Malta board's CBUS UART, where the registers are aligned on 8-byte boundaries and which uses 32-bit accesses, data as follows is returned: YAMON> dump -32 0xbf000900 0x40 BF000900: 1F000942 1F000942 1F000900 1F000900 ...B...B........ BF000910: 1F000901 1F000901 1F000900 1F000900 ................ BF000920: 1F000900 1F000900 1F000960 1F000960 ...........`...` BF000930: 1F000900 1F000900 1F0009FF 1F0009FF ................ YAMON> Evidently high-order 24 bits return values previously driven in the address phase (the 3 highest order address bits used with the command above are masked out in the simple virtual address mapping used here and come out at zeros on the external bus), a common scenario with bus lines left floating, due to bus capacitance. Consequently when the value of IIR, mapped at 0x1f000910, is retrieved in `autoconfig', it comes out at 0x1f0009c1 and when it is right-shifted by 6 and then assigned to 8-bit `scratch' variable, the value calculated is 0x27, not one of 0, 1, 2, 3 expected in port type determination. Fix the issue then, by assigning the value returned from `serial_in' to `scratch' first, which masks out 24 high-order bits retrieved, and only then right-shift the resulting 8-bit data quantity, producing the value of 3 in this case, as expected. Fix the same issue in `serial_dl_read'. The problem first appeared with Linux 2.6.9-rc3 which predates our repo history, but the origin could be identified with the old MIPS/Linux repo also at: <git://git.kernel.org/pub/scm/linux/kernel/git/ralf/linux.git> as commit e0d2356c0777 ("Merge with Linux 2.6.9-rc3."), where code in `serial_in' was updated with this case: + case UPIO_MEM32: + return readl(up->port.membase + offset); + which made it produce results outside the unsigned 8-bit range for the first time, though obviously it is system dependent what actual values appear in the high order bits retrieved and it may well have been zeros in the relevant positions with the system the change originally was intended for. It is at that point that code in `autoconf' should have been updated accordingly, but clearly it was overlooked. Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") --- drivers/tty/serial/8250/8250_port.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) linux-serial-8250-floating-bus-mask.diff Index: linux-malta-cbus-uart/drivers/tty/serial/8250/8250_port.c =================================================================== --- linux-malta-cbus-uart.orig/drivers/tty/serial/8250/8250_port.c +++ linux-malta-cbus-uart/drivers/tty/serial/8250/8250_port.c @@ -311,7 +311,10 @@ static const struct serial8250_config ua /* Uart divisor latch read */ static int default_serial_dl_read(struct uart_8250_port *up) { - return serial_in(up, UART_DLL) | serial_in(up, UART_DLM) << 8; + unsigned char dll = serial_in(up, UART_DLL); + unsigned char dlm = serial_in(up, UART_DLM); + + return dll | dlm << 8; } /* Uart divisor latch write */ @@ -1297,9 +1300,9 @@ static void autoconfig(struct uart_8250_ serial_out(up, UART_LCR, 0); serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO); - scratch = serial_in(up, UART_IIR) >> 6; + scratch = serial_in(up, UART_IIR); - switch (scratch) { + switch (scratch >> 6) { case 0: autoconfig_8250(up); break; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] serial: 8250: Mask out floating 16/32-bit bus bits 2021-06-10 18:38 ` [PATCH 1/2] serial: 8250: Mask out floating 16/32-bit bus bits Maciej W. Rozycki @ 2021-06-11 6:07 ` Jiri Slaby [not found] ` <CAHp75VeGn_wCLevAvD3iyykA73y+mh8k7pjQ2TY-9mt5cqEFWg@mail.gmail.com> 2021-06-11 17:07 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 7+ messages in thread From: Jiri Slaby @ 2021-06-11 6:07 UTC (permalink / raw) To: Maciej W. Rozycki, Greg Kroah-Hartman, Thomas Bogendoerfer Cc: linux-serial, linux-mips, linux-kernel On 10. 06. 21, 20:38, Maciej W. Rozycki wrote: > Make sure only actual 8 bits of the IIR register are used in determining > the port type in `autoconfig'. > > The `serial_in' port accessor returns the `unsigned int' type, meaning > that with UPIO_AU, UPIO_MEM16, UPIO_MEM32, and UPIO_MEM32BE access types > more than 8 bits of data are returned, of which the high order bits will > often come from bus lines that are left floating in the data phase. For > example with the MIPS Malta board's CBUS UART, where the registers are > aligned on 8-byte boundaries and which uses 32-bit accesses, data as > follows is returned: > > YAMON> dump -32 0xbf000900 0x40 > > BF000900: 1F000942 1F000942 1F000900 1F000900 ...B...B........ > BF000910: 1F000901 1F000901 1F000900 1F000900 ................ > BF000920: 1F000900 1F000900 1F000960 1F000960 ...........`...` > BF000930: 1F000900 1F000900 1F0009FF 1F0009FF ................ > > YAMON> > > Evidently high-order 24 bits return values previously driven in the > address phase (the 3 highest order address bits used with the command > above are masked out in the simple virtual address mapping used here and > come out at zeros on the external bus), a common scenario with bus lines > left floating, due to bus capacitance. > > Consequently when the value of IIR, mapped at 0x1f000910, is retrieved > in `autoconfig', it comes out at 0x1f0009c1 and when it is right-shifted > by 6 and then assigned to 8-bit `scratch' variable, the value calculated > is 0x27, not one of 0, 1, 2, 3 expected in port type determination. > > Fix the issue then, by assigning the value returned from `serial_in' to > `scratch' first, which masks out 24 high-order bits retrieved, and only > then right-shift the resulting 8-bit data quantity, producing the value > of 3 in this case, as expected. Fix the same issue in `serial_dl_read'. > > The problem first appeared with Linux 2.6.9-rc3 which predates our repo > history, but the origin could be identified with the old MIPS/Linux repo > also at: <git://git.kernel.org/pub/scm/linux/kernel/git/ralf/linux.git> > as commit e0d2356c0777 ("Merge with Linux 2.6.9-rc3."), where code in > `serial_in' was updated with this case: > > + case UPIO_MEM32: > + return readl(up->port.membase + offset); > + > > which made it produce results outside the unsigned 8-bit range for the > first time, though obviously it is system dependent what actual values > appear in the high order bits retrieved and it may well have been zeros > in the relevant positions with the system the change originally was > intended for. It is at that point that code in `autoconf' should have > been updated accordingly, but clearly it was overlooked. > > Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > --- > drivers/tty/serial/8250/8250_port.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > linux-serial-8250-floating-bus-mask.diff > Index: linux-malta-cbus-uart/drivers/tty/serial/8250/8250_port.c > =================================================================== > --- linux-malta-cbus-uart.orig/drivers/tty/serial/8250/8250_port.c > +++ linux-malta-cbus-uart/drivers/tty/serial/8250/8250_port.c > @@ -311,7 +311,10 @@ static const struct serial8250_config ua > /* Uart divisor latch read */ > static int default_serial_dl_read(struct uart_8250_port *up) > { > - return serial_in(up, UART_DLL) | serial_in(up, UART_DLM) << 8; > + unsigned char dll = serial_in(up, UART_DLL); > + unsigned char dlm = serial_in(up, UART_DLM); > + > + return dll | dlm << 8; > } > > /* Uart divisor latch write */ > @@ -1297,9 +1300,9 @@ static void autoconfig(struct uart_8250_ > serial_out(up, UART_LCR, 0); > > serial_out(up, UART_FCR, UART_FCR_ENABLE_FIFO); > - scratch = serial_in(up, UART_IIR) >> 6; > + scratch = serial_in(up, UART_IIR); > > - switch (scratch) { > + switch (scratch >> 6) { COrrect, but not obvious on the first look. People could revert this change inadverently. So could you add a comment, or simply cast serial_in() output to (u8)? > case 0: > autoconfig_8250(up); > break; > thanks, -- js suse labs ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <CAHp75VeGn_wCLevAvD3iyykA73y+mh8k7pjQ2TY-9mt5cqEFWg@mail.gmail.com>]
* Re: [PATCH 1/2] serial: 8250: Mask out floating 16/32-bit bus bits [not found] ` <CAHp75VeGn_wCLevAvD3iyykA73y+mh8k7pjQ2TY-9mt5cqEFWg@mail.gmail.com> @ 2021-06-11 6:55 ` Greg Kroah-Hartman 2021-06-26 4:12 ` Maciej W. Rozycki 0 siblings, 1 reply; 7+ messages in thread From: Greg Kroah-Hartman @ 2021-06-11 6:55 UTC (permalink / raw) To: Andy Shevchenko Cc: Maciej W. Rozycki, Jiri Slaby, Thomas Bogendoerfer, linux-serial, linux-mips, linux-kernel On Fri, Jun 11, 2021 at 09:40:31AM +0300, Andy Shevchenko wrote: > On Thursday, June 10, 2021, Maciej W. Rozycki <macro@orcam.me.uk> wrote: > > Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12- > > > Please, find the history group repository (Git.kernel.org) and use proper > hash of the real commit. There is no real need to do that, I'll just put a "cc: stable" in here and it will go back as far as we currently maintain. thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] serial: 8250: Mask out floating 16/32-bit bus bits 2021-06-11 6:55 ` Greg Kroah-Hartman @ 2021-06-26 4:12 ` Maciej W. Rozycki 0 siblings, 0 replies; 7+ messages in thread From: Maciej W. Rozycki @ 2021-06-26 4:12 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Andy Shevchenko, Jiri Slaby, Thomas Bogendoerfer, linux-serial, linux-mips, linux-kernel On Fri, 11 Jun 2021, Greg Kroah-Hartman wrote: > On Fri, Jun 11, 2021 at 09:40:31AM +0300, Andy Shevchenko wrote: > > > Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12- > > > > > > Please, find the history group repository (Git.kernel.org) and use proper > > hash of the real commit. Thanks for making me aware of that repo, it can be helpful. > There is no real need to do that, I'll just put a "cc: stable" in here > and it will go back as far as we currently maintain. I've posted v2 then, with Jiri's concern addressed and a "cc: stable" annotation. Originally I didn't think this series is worth backporting, but if you'd rather do so, then of course I'm fine with that. Maciej ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] serial: 8250: Mask out floating 16/32-bit bus bits 2021-06-10 18:38 ` [PATCH 1/2] serial: 8250: Mask out floating 16/32-bit bus bits Maciej W. Rozycki 2021-06-11 6:07 ` Jiri Slaby [not found] ` <CAHp75VeGn_wCLevAvD3iyykA73y+mh8k7pjQ2TY-9mt5cqEFWg@mail.gmail.com> @ 2021-06-11 17:07 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2021-06-11 17:07 UTC (permalink / raw) To: Maciej W. Rozycki Cc: Greg Kroah-Hartman, Jiri Slaby, Thomas Bogendoerfer, open list:SERIAL DRIVERS, open list:BROADCOM NVRAM DRIVER, open list On Thu, Jun 10, 2021 at 8:39 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote: > > Make sure only actual 8 bits of the IIR register are used in determining > the port type in `autoconfig'. > > The `serial_in' port accessor returns the `unsigned int' type, meaning > that with UPIO_AU, UPIO_MEM16, UPIO_MEM32, and UPIO_MEM32BE access types > more than 8 bits of data are returned, of which the high order bits will > often come from bus lines that are left floating in the data phase. For > example with the MIPS Malta board's CBUS UART, where the registers are > aligned on 8-byte boundaries and which uses 32-bit accesses, data as > follows is returned: > > YAMON> dump -32 0xbf000900 0x40 > > BF000900: 1F000942 1F000942 1F000900 1F000900 ...B...B........ > BF000910: 1F000901 1F000901 1F000900 1F000900 ................ > BF000920: 1F000900 1F000900 1F000960 1F000960 ...........`...` > BF000930: 1F000900 1F000900 1F0009FF 1F0009FF ................ > > YAMON> > > Evidently high-order 24 bits return values previously driven in the > address phase (the 3 highest order address bits used with the command > above are masked out in the simple virtual address mapping used here and > come out at zeros on the external bus), a common scenario with bus lines > left floating, due to bus capacitance. > > Consequently when the value of IIR, mapped at 0x1f000910, is retrieved > in `autoconfig', it comes out at 0x1f0009c1 and when it is right-shifted > by 6 and then assigned to 8-bit `scratch' variable, the value calculated > is 0x27, not one of 0, 1, 2, 3 expected in port type determination. > > Fix the issue then, by assigning the value returned from `serial_in' to > `scratch' first, which masks out 24 high-order bits retrieved, and only > then right-shift the resulting 8-bit data quantity, producing the value > of 3 in this case, as expected. Fix the same issue in `serial_dl_read'. > > The problem first appeared with Linux 2.6.9-rc3 which predates our repo > history, but the origin could be identified with the old MIPS/Linux repo > also at: <git://git.kernel.org/pub/scm/linux/kernel/git/ralf/linux.git> > as commit e0d2356c0777 ("Merge with Linux 2.6.9-rc3."), where code in > `serial_in' was updated with this case: > > + case UPIO_MEM32: > + return readl(up->port.membase + offset); > + > > which made it produce results outside the unsigned 8-bit range for the > first time, though obviously it is system dependent what actual values > appear in the high order bits retrieved and it may well have been zeros > in the relevant positions with the system the change originally was > intended for. It is at that point that code in `autoconf' should have > been updated accordingly, but clearly it was overlooked. > > Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > --- > drivers/tty/serial/8250/8250_port.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] MIPS: Malta: Do not byte-swap accesses to the CBUS UART 2021-06-10 18:37 [PATCH 0/2] serial, Malta: Fixes to make the CBUS UART work big-endian Maciej W. Rozycki 2021-06-10 18:38 ` [PATCH 1/2] serial: 8250: Mask out floating 16/32-bit bus bits Maciej W. Rozycki @ 2021-06-10 18:38 ` Maciej W. Rozycki 1 sibling, 0 replies; 7+ messages in thread From: Maciej W. Rozycki @ 2021-06-10 18:38 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby, Thomas Bogendoerfer Cc: linux-serial, linux-mips, linux-kernel Correct big-endian accesses to the CBUS UART, a Malta on-board discrete TI16C550C part wired directly to the system controller's device bus, and do not use byte swapping with the 32-bit accesses to the device. The CBUS is used for devices such as the boot flash memory needed early on in system bootstrap even before PCI has been initialised. Therefore it uses the system controller's device bus, which follows the endianness set with the CPU, which means no byte-swapping is ever required for data accesses to CBUS, unlike with PCI. The CBUS UART uses the UPIO_MEM32 access method, that is the `readl' and `writel' MMIO accessors, which on the MIPS platform imply byte-swapping with PCI systems. Consequently the wrong byte lane is accessed with the big-endian configuration and the UART is not correctly accessed. As it happens the UPIO_MEM32BE access method makes use of the `ioread32' and `iowrite32' MMIO accessors, which still use `readl' and `writel' respectively, however they byte-swap data passed, effectively cancelling swapping done with the accessors themselves and making it suitable for the CBUS UART. Make the CBUS UART switch between UPIO_MEM32 and UPIO_MEM32BE then, based on the endianness selected. With this change in place the device is correctly recognised with big-endian Malta at boot, along with the Super I/O devices behind PCI: Serial: 8250/16550 driver, 5 ports, IRQ sharing enabled printk: console [ttyS0] disabled serial8250.0: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A printk: console [ttyS0] enabled printk: console [ttyS0] enabled printk: bootconsole [uart8250] disabled printk: bootconsole [uart8250] disabled serial8250.0: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200) is a 16550A serial8250.0: ttyS2 at MMIO 0x1f000900 (irq = 20, base_baud = 230400) is a 16550A Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk> Fixes: e7c4782f92fc ("[MIPS] Put an end to <asm/serial.h>'s long and annyoing existence") --- arch/mips/mti-malta/malta-platform.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) linux-mips-malta-cbus-uart-be.diff Index: linux-malta-cbus-uart/arch/mips/mti-malta/malta-platform.c =================================================================== --- linux-malta-cbus-uart.orig/arch/mips/mti-malta/malta-platform.c +++ linux-malta-cbus-uart/arch/mips/mti-malta/malta-platform.c @@ -47,7 +47,8 @@ static struct plat_serial8250_port uart8 .mapbase = 0x1f000900, /* The CBUS UART */ .irq = MIPS_CPU_IRQ_BASE + MIPSCPU_INT_MB2, .uartclk = 3686400, /* Twice the usual clk! */ - .iotype = UPIO_MEM32, + .iotype = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) ? + UPIO_MEM32BE : UPIO_MEM32, .flags = CBUS_UART_FLAGS, .regshift = 3, }, ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-26 4:12 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-10 18:37 [PATCH 0/2] serial, Malta: Fixes to make the CBUS UART work big-endian Maciej W. Rozycki 2021-06-10 18:38 ` [PATCH 1/2] serial: 8250: Mask out floating 16/32-bit bus bits Maciej W. Rozycki 2021-06-11 6:07 ` Jiri Slaby [not found] ` <CAHp75VeGn_wCLevAvD3iyykA73y+mh8k7pjQ2TY-9mt5cqEFWg@mail.gmail.com> 2021-06-11 6:55 ` Greg Kroah-Hartman 2021-06-26 4:12 ` Maciej W. Rozycki 2021-06-11 17:07 ` Philippe Mathieu-Daudé 2021-06-10 18:38 ` [PATCH 2/2] MIPS: Malta: Do not byte-swap accesses to the CBUS UART Maciej W. Rozycki
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.