Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250

[Florian Fainelli <f.fainelli@gmail.com>]

+PPC folks

On 11/23/18 10:20 AM, Guenter Roeck wrote:
> On Mon, Nov 19, 2018 at 12:50:50PM -0800, Guenter Roeck wrote:
>> On Mon, Nov 19, 2018 at 10:44:30AM -0800, Florian Fainelli wrote:
>>> On 11/15/18 5:16 PM, Guenter Roeck wrote:
>>>> On Thu, Nov 15, 2018 at 11:48:20AM -0800, Florian Fainelli wrote:
>>>>>
>>>>> OK, would you mind testing this below? It seems to me that 8250_of.c is
>>>>> incompatible with arch/powerpc/kernel/legacy_serial.c and that is what
>>>>> is causing the issue here.
>>>>>
>>>>> diff --git a/drivers/tty/serial/8250/Kconfig
>>>>> b/drivers/tty/serial/8250/Kconfig
>>>>> index d7737dca0e48..21cb14cbd34a 100644
>>>>> --- a/drivers/tty/serial/8250/Kconfig
>>>>> +++ b/drivers/tty/serial/8250/Kconfig
>>>>> @@ -483,7 +483,7 @@ config SERIAL_8250_PXA
>>>>>
>>>>>  config SERIAL_OF_PLATFORM
>>>>>         tristate "Devicetree based probing for 8250 ports"
>>>>> -       depends on SERIAL_8250 && OF
>>>>> +       depends on SERIAL_8250 && OF && !(PPC && PPC_UDBG_16550)
>>>>>         default SERIAL_8250
>>>>>         help
>>>>>           This option is used for all 8250 compatible serial ports that
>>>>
>>>> 44x/virtex5_defconfig has both PPC_UDBG_16550 and SERIAL_OF_PLATFORM enabled
>>>> and fails to boot (or display anything on the console) with this patch applied.
>>>
>>> Thanks for trying, can you either share or provide a link to the mpc85xx
>>> and ml507 qemu command lines that you use? I spent a good chunk of my
>>> time trying to get a kernel to boot but has failed so far.
>>>
>>
> 
> Any update ? I still see the boot failures in next-20181123.

Yes, took most of last week's off sorry for the delay. I have finally
been able to boot a kernel using your instructions, thanks. The problem
is the following call chain:

of_platform_serial_probe()
 -> serial8250_register_8250_port()
    -> up->port.uartclk == 0, return -EINVAL
	-> irq_dispose_mapping()

and then we basically unwind what we just did with
of_platform_serial_probe() including disabling the UART's IRQ, comment
out the irq_dispose_mapping() and you will have a functional console
again. 8250_of.c is clearly not a full replacement for legacy_serial.c
(that was a first attempt), so we need to keep that code around. Making
the depends even more conditional also does not sound too appealing,
because while we have identified mpc85xx as being problematic, who knows
about other platforms. So the best I have that does not involve a revert
is this below.

Ben, Michael, would that sound reasonable to you? It seems to me that
there is a million ways to shoot the legacy_serial 8250 registration in
the foot in any cases, and having CONFIG_SERIAL_OF_PLATFORM just made it
painfully obvious.

diff --git a/arch/powerpc/kernel/legacy_serial.c
b/arch/powerpc/kernel/legacy_serial.c
index 33b34a58fc62..31353a27d714 100644
--- a/arch/powerpc/kernel/legacy_serial.c
+++ b/arch/powerpc/kernel/legacy_serial.c
@@ -16,7 +16,7 @@
 #include <asm/pci-bridge.h>
 #include <asm/ppc-pci.h>

-#undef DEBUG
+#define DEBUG

 #ifdef DEBUG
 #define DBG(fmt...) do { printk(fmt); } while(0)
@@ -70,6 +70,29 @@ static void tsi_serial_out(struct uart_port *p, int
offset, int value)
                writeb(value, p->membase + offset);
 }

+#ifdef CONFIG_SERIAL_OF_PLATFORM
+static struct property uart_prop = {
+       .value = "disabled",
+       .name = "status",
+       .length = strlen("disabled"),
+};
+
+static void __init disable_uart_node(struct device_node *np)
+{
+       /* To avoid having 8250_of.c attempt to register the same device
+        * and failing to do, and calling irq_dispose_mapping(), just
+        * disable the device_node for now.
+        */
+       of_update_property(np, &uart_prop);
+       pr_info("%s: disabled UART node\n", __func__);
+}
+#else
+static inline int disable_uart_node(struct device_node *np)
+{
+       return 0;
+}
+#endif
+
 static int __init add_legacy_port(struct device_node *np, int want_index,
                                  int iotype, phys_addr_t base,
                                  phys_addr_t taddr, unsigned long irq,
@@ -80,6 +103,8 @@ static int __init add_legacy_port(struct device_node
*np, int want_index,
        u32 shift = 0;
        int index;

+       disable_uart_node(np);
+
        /* get clock freq. if present */
        clk = of_get_property(np, "clock-frequency", NULL);
        if (clk && *clk)



-- 
Florian