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

[Jean-Michel Hautbois <jeanmichel.hautbois@xxxxxxxxxx>]

Hi Greg,

On 12/9/24 13:30, Greg Ungerer wrote:
> 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@xxxxxxxxxx>
> > > > > > > > ---
> > > > > > > >   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.

Thank you, it is clear. Yes, I get the idea :-).
I will submit a v2 once I have something (very) inspired ;-).
I just have to find a name for this new configuration, and test it.

JM