Re: [PATCH] m68k: coldfire: Support resources for UART

[Greg Ungerer <gerg@linux-m68k.org>]

[-- Attachment #1: Type: text/plain, Size: 12636 bytes --]

Hi JM,

On 5/12/24 23:06, Jean-Michel Hautbois wrote:
> On 05/12/2024 14:01, Greg Ungerer wrote:
>> On 4/12/24 21:32, Jean-Michel Hautbois wrote:
>>> On 04/12/2024 12:15, Greg Ungerer wrote:
>>>> On 4/12/24 20:58, Jean-Michel Hautbois wrote:
>>>>> On 04/12/2024 11:54, Greg Ungerer wrote:
>>>>>> On 2/12/24 20:34, Jean-Michel Hautbois wrote:
>>>>>>> In order to use the eDMA channels for UART, the mcf_platform_uart needs
>>>>>>> to be changed. Instead of adding another custom member for the
>>>>>>> structure, use a resource tree in a platform_device per UART. It then
>>>>>>> makes it possible to have a device named like "mcfuart.N" with N the
>>>>>>> UART number.
>>>>>>>
>>>>>>> Later, adding the dma channel in the mcf tty driver will also be more
>>>>>>> straightfoward.
>>>>>>>
>>>>>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
>>>>>>> ---
>>>>>>>   arch/m68k/coldfire/device.c | 96 +++++++++++++ +-------------------------------
>>>>>>>   drivers/tty/serial/mcf.c    | 69 +++++++++++++++++++-------------
>>>>>>>   2 files changed, 70 insertions(+), 95 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/m68k/coldfire/device.c b/arch/m68k/coldfire/ device.c
>>>>>>> index b6958ec2a220cf91a78a14fc7fa18749451412f7..fd7d0b0ce7eb2970cb8ffe33589fe8d7e88c268d 100644
>>>>>>> --- a/arch/m68k/coldfire/device.c
>>>>>>> +++ b/arch/m68k/coldfire/device.c
>>>>>>> @@ -24,73 +24,35 @@
>>>>>>>   #include <linux/platform_data/dma-mcf-edma.h>
>>>>>>>   #include <linux/platform_data/mmc-esdhc-mcf.h>
>>>>>>> -/*
>>>>>>> - *    All current ColdFire parts contain from 2, 3, 4 or 10 UARTS.
>>>>>>> - */
>>>>>>> -static struct mcf_platform_uart mcf_uart_platform_data[] = {
>>>>>>> -    {
>>>>>>> -        .mapbase    = MCFUART_BASE0,
>>>>>>> -        .irq        = MCF_IRQ_UART0,
>>>>>>> -    },
>>>>>>> -    {
>>>>>>> -        .mapbase    = MCFUART_BASE1,
>>>>>>> -        .irq        = MCF_IRQ_UART1,
>>>>>>> -    },
>>>>>>> -#ifdef MCFUART_BASE2
>>>>>>> -    {
>>>>>>> -        .mapbase    = MCFUART_BASE2,
>>>>>>> -        .irq        = MCF_IRQ_UART2,
>>>>>>> -    },
>>>>>>> -#endif
>>>>>>> -#ifdef MCFUART_BASE3
>>>>>>> -    {
>>>>>>> -        .mapbase    = MCFUART_BASE3,
>>>>>>> -        .irq        = MCF_IRQ_UART3,
>>>>>>> -    },
>>>>>>> -#endif
>>>>>>> -#ifdef MCFUART_BASE4
>>>>>>> -    {
>>>>>>> -        .mapbase    = MCFUART_BASE4,
>>>>>>> -        .irq        = MCF_IRQ_UART4,
>>>>>>> -    },
>>>>>>> -#endif
>>>>>>> -#ifdef MCFUART_BASE5
>>>>>>> -    {
>>>>>>> -        .mapbase    = MCFUART_BASE5,
>>>>>>> -        .irq        = MCF_IRQ_UART5,
>>>>>>> -    },
>>>>>>> -#endif
>>>>>>> -#ifdef MCFUART_BASE6
>>>>>>> -    {
>>>>>>> -        .mapbase    = MCFUART_BASE6,
>>>>>>> -        .irq        = MCF_IRQ_UART6,
>>>>>>> -    },
>>>>>>> -#endif
>>>>>>> -#ifdef MCFUART_BASE7
>>>>>>> -    {
>>>>>>> -        .mapbase    = MCFUART_BASE7,
>>>>>>> -        .irq        = MCF_IRQ_UART7,
>>>>>>> +static u64 mcf_uart_mask = DMA_BIT_MASK(32);
>>>>>>> +
>>>>>>> +static struct resource mcf_uart0_resource[] = {
>>>>>>> +    [0] = {
>>>>>>> +        .start = MCFUART_BASE0,
>>>>>>> +        .end   = MCFUART_BASE0 + 0x3fff,
>>>>>>> +        .flags = IORESOURCE_MEM,
>>>>>>>       },
>>>>>>> -#endif
>>>>>>> -#ifdef MCFUART_BASE8
>>>>>>> -    {
>>>>>>> -        .mapbase    = MCFUART_BASE8,
>>>>>>> -        .irq        = MCF_IRQ_UART8,
>>>>>>> +    [1] = {
>>>>>>> +        .start = 2,
>>>>>>> +        .end   = 3,
>>>>>>> +        .flags = IORESOURCE_DMA,
>>>>>>>       },
>>>>>>> -#endif
>>>>>>> -#ifdef MCFUART_BASE9
>>>>>>> -    {
>>>>>>> -        .mapbase    = MCFUART_BASE9,
>>>>>>> -        .irq        = MCF_IRQ_UART9,
>>>>>>> +    [2] = {
>>>>>>> +        .start = MCF_IRQ_UART0,
>>>>>>> +        .end   = MCF_IRQ_UART0,
>>>>>>> +        .flags = IORESOURCE_IRQ,
>>>>>>>       },
>>>>>>> -#endif
>>>>>>> -    { },
>>>>>>>   };
>>>>>>> -static struct platform_device mcf_uart = {
>>>>>>> +static struct platform_device mcf_uart0 = {
>>>>>>>       .name            = "mcfuart",
>>>>>>>       .id            = 0,
>>>>>>> -    .dev.platform_data    = mcf_uart_platform_data,
>>>>>>> +    .num_resources = ARRAY_SIZE(mcf_uart0_resource),
>>>>>>> +    .resource = mcf_uart0_resource,
>>>>>>> +    .dev = {
>>>>>>> +        .dma_mask = &mcf_uart_mask,
>>>>>>> +        .coherent_dma_mask = DMA_BIT_MASK(32),
>>>>>>> +    },
>>>>>>>   };
>>>>>>>   #ifdef MCFFEC_BASE0
>>>>>>> @@ -485,12 +447,12 @@ static struct platform_device mcf_i2c5 = {
>>>>>>>   static const struct dma_slave_map mcf_edma_map[] = {
>>>>>>>       { "dreq0", "rx-tx", MCF_EDMA_FILTER_PARAM(0) },
>>>>>>>       { "dreq1", "rx-tx", MCF_EDMA_FILTER_PARAM(1) },
>>>>>>> -    { "uart.0", "rx", MCF_EDMA_FILTER_PARAM(2) },
>>>>>>> -    { "uart.0", "tx", MCF_EDMA_FILTER_PARAM(3) },
>>>>>>> -    { "uart.1", "rx", MCF_EDMA_FILTER_PARAM(4) },
>>>>>>> -    { "uart.1", "tx", MCF_EDMA_FILTER_PARAM(5) },
>>>>>>> -    { "uart.2", "rx", MCF_EDMA_FILTER_PARAM(6) },
>>>>>>> -    { "uart.2", "tx", MCF_EDMA_FILTER_PARAM(7) },
>>>>>>> +    { "mcfuart.0", "rx", MCF_EDMA_FILTER_PARAM(2) },
>>>>>>> +    { "mcfuart.0", "tx", MCF_EDMA_FILTER_PARAM(3) },
>>>>>>> +    { "mcfuart.1", "rx", MCF_EDMA_FILTER_PARAM(4) },
>>>>>>> +    { "mcfuart.1", "tx", MCF_EDMA_FILTER_PARAM(5) },
>>>>>>> +    { "mcfuart.2", "rx", MCF_EDMA_FILTER_PARAM(6) },
>>>>>>> +    { "mcfuart.2", "tx", MCF_EDMA_FILTER_PARAM(7) },
>>>>>>>       { "timer0", "rx-tx", MCF_EDMA_FILTER_PARAM(8) },
>>>>>>>       { "timer1", "rx-tx", MCF_EDMA_FILTER_PARAM(9) },
>>>>>>>       { "timer2", "rx-tx", MCF_EDMA_FILTER_PARAM(10) },
>>>>>>> @@ -623,7 +585,7 @@ static struct platform_device mcf_flexcan0 = {
>>>>>>>   #endif /* MCFFLEXCAN_SIZE */
>>>>>>>   static struct platform_device *mcf_devices[] __initdata = {
>>>>>>> -    &mcf_uart,
>>>>>>> +    &mcf_uart0,
>>>>>>>   #ifdef MCFFEC_BASE0
>>>>>>>       &mcf_fec0,
>>>>>>>   #endif
>>>>>>> diff --git a/drivers/tty/serial/mcf.c b/drivers/tty/serial/mcf.c
>>>>>>> index 93e7dda4d39acd23daf8c0d4c29ac8d666f263c5..07b8decfdb6005f0265dd130765e45c3fd1715eb 100644
>>>>>>> --- a/drivers/tty/serial/mcf.c
>>>>>>> +++ b/drivers/tty/serial/mcf.c
>>>>>>> @@ -570,31 +570,46 @@ static struct uart_driver mcf_driver = {
>>>>>>>   static int mcf_probe(struct platform_device *pdev)
>>>>>>>   {
>>>>>>> -    struct mcf_platform_uart *platp = dev_get_platdata(&pdev->dev);
>>>>>>>       struct uart_port *port;
>>>>>>> -    int i;
>>>>>>> -
>>>>>>> -    for (i = 0; ((i < MCF_MAXPORTS) && (platp[i].mapbase)); i++) {
>>>>>>> -        port = &mcf_ports[i].port;
>>>>>>> -
>>>>>>> -        port->line = i;
>>>>>>> -        port->type = PORT_MCF;
>>>>>>> -        port->mapbase = platp[i].mapbase;
>>>>>>> -        port->membase = (platp[i].membase) ? platp[i].membase :
>>>>>>> -            (unsigned char __iomem *) platp[i].mapbase;
>>>>>>> -        port->dev = &pdev->dev;
>>>>>>> -        port->iotype = SERIAL_IO_MEM;
>>>>>>> -        port->irq = platp[i].irq;
>>>>>>> -        port->uartclk = MCF_BUSCLK;
>>>>>>> -        port->ops = &mcf_uart_ops;
>>>>>>> -        port->flags = UPF_BOOT_AUTOCONF;
>>>>>>> -        port->rs485_config = mcf_config_rs485;
>>>>>>> -        port->rs485_supported = mcf_rs485_supported;
>>>>>>> -        port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_MCF_CONSOLE);
>>>>>>> -
>>>>>>> -        uart_add_one_port(&mcf_driver, port);
>>>>>>> +    struct mcf_uart *pp;
>>>>>>> +    struct resource *res;
>>>>>>> +    void __iomem *base;
>>>>>>> +    int id = pdev->id;
>>>>>>> +
>>>>>>> +    if (id == -1 || id >= MCF_MAXPORTS) {
>>>>>>> +        dev_err(&pdev->dev, "uart%d out of range\n",
>>>>>>> +            id);
>>>>>>> +        return -EINVAL;
>>>>>>>       }
>>>>>>> +    port = &mcf_ports[id].port;
>>>>>>> +    port->line = id;
>>>>>>> +
>>>>>>> +    base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>>>>>>> +    if (IS_ERR(base))
>>>>>>> +        return PTR_ERR(base);
>>>>>>> +
>>>>>>> +    port->mapbase = res->start;
>>>>>>> +    port->membase = base;
>>>>>>> +
>>>>>>> +    port->irq = platform_get_irq(pdev, 0);
>>>>>>> +    if (port->irq < 0)
>>>>>>> +        return port->irq;
>>>>>>> +
>>>>>>> +    port->type = PORT_MCF;
>>>>>>> +    port->dev = &pdev->dev;
>>>>>>> +    port->iotype = SERIAL_IO_MEM;
>>>>>>> +    port->uartclk = MCF_BUSCLK;
>>>>>>> +    port->ops = &mcf_uart_ops;
>>>>>>> +    port->flags = UPF_BOOT_AUTOCONF;
>>>>>>> +    port->rs485_config = mcf_config_rs485;
>>>>>>> +    port->rs485_supported = mcf_rs485_supported;
>>>>>>> +    port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_MCF_CONSOLE);
>>>>>>> +
>>>>>>> +    pp = container_of(port, struct mcf_uart, port);
>>>>>>> +
>>>>>>> +    uart_add_one_port(&mcf_driver, port);
>>>>>>> +
>>>>>>
>>>>>> This breaks platforms with more than one UART - which is quite a few of
>>>>>> the ColdFire platforms. Numerous boards bring and use more than one UART.
>>>>>
>>>>> I don't get why, as I have two uarts here, and each is detected properly when declaring those in my platform ? I get that it breaks existing detection (we are parsing all uarts even when only one or two is used) but it does not prevent it to work ?
>>>>
>>>> Building and testing on an M5208EVB platform.
>>>> With original un-modified code boot console shows:
>>>>
>>>> ...
>>>> [    0.110000] romfs: ROMFS MTD (C) 2007 Red Hat, Inc.
>>>> [    0.110000] ColdFire internal UART serial driver
>>>> [    0.110000] mcfuart.0: ttyS0 at MMIO 0xfc060000 (irq = 90, base_baud = 5208333) is a ColdFire UART
>>>> [    0.120000] printk: legacy console [ttyS0] enabled
>>>> [    0.120000] mcfuart.0: ttyS1 at MMIO 0xfc064000 (irq = 91, base_baud = 5208333) is a ColdFire UART
>>>> [    0.120000] mcfuart.0: ttyS2 at MMIO 0xfc068000 (irq = 92, base_baud = 5208333) is a ColdFire UART
>>>> [    0.130000] brd: module loaded
>>>> ...
>>>>
>>>>
>>>> But with this change applied only the first port is probed:
>>>>
>>>> ...
>>>> [    0.120000] romfs: ROMFS MTD (C) 2007 Red Hat, Inc.
>>>> [    0.120000] ColdFire internal UART serial driver
>>>> [    0.130000] mcfuart.0: ttyS0 at MMIO 0xfc060000 (irq = 90, base_baud = 5208333) is a ColdFire UART
>>>> [    0.130000] printk: legacy console [ttyS0] enabled
>>>> [    0.130000] brd: module loaded
>>>> ...
>>>
>>> OK, I see what you mean. Let me try to explain why I did it :-).
>>>
>>> The idea is to avoid probing a UART device which may exist as such on the core, but not be used as UART at all (on my board, for instance, I have uart2 and uart6, I don't need any other UART to be probed).
>>>
>>> So, based on what I think is the dts philosophy, you declare the devices you really need to probe ?
>>
>> You can do this too, with the old style platform setups.
>>
>> What you want is to have a separate board file just for your board.
>> There is a few examples already in arch/m68k/coldfire/ like amcore.c,
>> firebee.c, nettel.c and stmark2.c. None currently specifically extract
>> out UARTS - no one really seemed to have a need for that in the past.
>> Most ColdFire parts have 2 or 3 UARTS, the 5441x family is an out-lier
>> here with 10.
>>
>> Anyway, the device.c entries are really just a catch-all for the most
>> common devices and their most commonly used configurations.
> 
> Thanks for answering !
> 
> I know I can have a dedicated file for my board (which i have tbh) but device.c is always built when you select CONFIG_COLDFIRE so, I would end up with 10 UARTs probed anyways ?
> 
> Is there no way for this patch to find a path ? I mean, I can keep the existing behavior, and have everything probed in device.c if the BASE address is declared. But I don't want my board to have all 10 UARTs and I don't want to locally patch the Makefile to remove the device.c from the built-in ?

Here is one example way to do it.
Compile tested only - but I am sure you get the idea.

Regards
Greg



[-- Attachment #2: 0001-m68k-example-board-defined-UART-table.patch --]
[-- Type: text/x-patch, Size: 3207 bytes --]

From 085782144a0dbf323999e7e25825bdf089639be4 Mon Sep 17 00:00:00 2001
From: Greg Ungerer <gerg@kernel.org>
Date: Mon, 9 Dec 2024 22:23:32 +1000
Subject: [PATCH] m68k: example board defined UART table

Example changes that allow a specific ColdFire board to define only the
UART devices that it has available and not necessarily all the SoC
actually has.

Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
---
 arch/m68k/Kconfig.machine           | 10 ++++++++++
 arch/m68k/coldfire/Makefile         |  1 +
 arch/m68k/coldfire/device.c         |  8 ++++++++
 arch/m68k/coldfire/jm_54418_board.c | 21 +++++++++++++++++++++
 4 files changed, 40 insertions(+)
 create mode 100644 arch/m68k/coldfire/jm_54418_board.c

diff --git a/arch/m68k/Kconfig.machine b/arch/m68k/Kconfig.machine
index de39f23b180e..536b511f6c31 100644
--- a/arch/m68k/Kconfig.machine
+++ b/arch/m68k/Kconfig.machine
@@ -322,6 +322,16 @@ config MOD5272
 	help
 	  Support for the Netburner MOD-5272 board.
 
+config BOARD_HAS_DEFINED_UARTS
+	bool
+
+config JM_54418_BOARD
+	bool "JM's 54418 based board"
+	depends on M5441x
+	select BOARD_HAS_DEFINED_UARTS
+	help
+	  Support for JM's 54418 based ColdFire platform.
+
 if !MMU || COLDFIRE
 
 comment "Machine Options"
diff --git a/arch/m68k/coldfire/Makefile b/arch/m68k/coldfire/Makefile
index c56bc0dc7f2e..06b9a75a596d 100644
--- a/arch/m68k/coldfire/Makefile
+++ b/arch/m68k/coldfire/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_FIREBEE)	+= firebee.o
 obj-$(CONFIG_MCF8390)	+= mcf8390.o
 obj-$(CONFIG_AMCORE)	+= amcore.o
 obj-$(CONFIG_STMARK2)	+= stmark2.o
+obj-$(CONFIG_JM_54418_BOARD) += jm_54418_board.o
 
 obj-$(CONFIG_PCI)	+= pci.o
 
diff --git a/arch/m68k/coldfire/device.c b/arch/m68k/coldfire/device.c
index b6958ec2a220..f8be795fa9ec 100644
--- a/arch/m68k/coldfire/device.c
+++ b/arch/m68k/coldfire/device.c
@@ -26,7 +26,14 @@
 
 /*
  *	All current ColdFire parts contain from 2, 3, 4 or 10 UARTS.
+ *	This mapping will create all possible UARTs for a platform.
+ *	Some boards only want to use a more limited set of UARTS,
+ *	in that case the board should use BOARD_HAS_DEFINED_UARTS and
+ *	define their own UART platform data in board specific code.
  */
+#ifdef BOARD_HAS_DEFINED_UARTS
+extern struct mcf_platform_uart mcf_uart_platform_data[];
+#else
 static struct mcf_platform_uart mcf_uart_platform_data[] = {
 	{
 		.mapbase	= MCFUART_BASE0,
@@ -86,6 +93,7 @@ static struct mcf_platform_uart mcf_uart_platform_data[] = {
 #endif
 	{ },
 };
+#endif /* HAS_BOARD_DEFINED_UARTS */
 
 static struct platform_device mcf_uart = {
 	.name			= "mcfuart",
diff --git a/arch/m68k/coldfire/jm_54418_board.c b/arch/m68k/coldfire/jm_54418_board.c
new file mode 100644
index 000000000000..6a9d03c03f76
--- /dev/null
+++ b/arch/m68k/coldfire/jm_54418_board.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <asm/coldfire.h>
+#include <asm/mcfsim.h>
+#include <asm/mcfuart.h>
+
+static struct mcf_platform_uart mcf_uart_platform_data[] = {
+	{
+		.mapbase	= MCFUART_BASE2,
+		.irq		= MCF_IRQ_UART2,
+	},
+	{
+		.mapbase	= MCFUART_BASE6,
+		.irq		= MCF_IRQ_UART6,
+	},
+	{ },
+};
+
-- 
2.25.1