m68k: coldfire: Support resources for UART

Message ID	20241202-m5441x_uart_resource-v1-1-6b28cb295fb5@yoseli.org
State	New
Headers	show Received: from mslow1.mail.gandi.net (mslow1.mail.gandi.net [217.70.178.240]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id AE4D11FA167; Mon, 2 Dec 2024 10:39:51 +0000 (UTC) From: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org> Date: Mon, 02 Dec 2024 11:34:18 +0100 Subject: [PATCH] m68k: coldfire: Support resources for UART Precedence: bulk MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20241202-m5441x_uart_resource-v1-1-6b28cb295fb5@yoseli.org> To: Greg Ungerer <gerg@linux-m68k.org>, Geert Uytterhoeven <geert@linux-m68k.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jirislaby@kernel.org> Cc: linux-m68k@lists.linux-m68k.org, linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org, Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
Series	m68k: coldfire: Support resources for UART \| expand m68k: coldfire: Support resources for UART

Jean-Michel Hautbois Dec. 2, 2024, 10:34 a.m. UTC

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(-)


---
base-commit: e457f18d7f25288d143c1fe024a620d0b15caec1
change-id: 20241202-m5441x_uart_resource-729b30c15363

Best regards,

Greg Ungerer Dec. 4, 2024, 10:54 a.m. UTC | #1

Hi JM,

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.

Regards
Greg



>   	return 0;
>   }
>   
> @@ -603,13 +618,11 @@ static int mcf_probe(struct platform_device *pdev)
>   static void mcf_remove(struct platform_device *pdev)
>   {
>   	struct uart_port *port;
> -	int i;
> +	int id = pdev->id;
>   
> -	for (i = 0; (i < MCF_MAXPORTS); i++) {
> -		port = &mcf_ports[i].port;
> -		if (port)
> -			uart_remove_one_port(&mcf_driver, port);
> -	}
> +	port = &mcf_ports[id].port;
> +	if (port)
> +		uart_remove_one_port(&mcf_driver, port);
>   }
>   
>   /****************************************************************************/
> 
> ---
> base-commit: e457f18d7f25288d143c1fe024a620d0b15caec1
> change-id: 20241202-m5441x_uart_resource-729b30c15363
> 
> Best regards,

Jean-Michel Hautbois Dec. 4, 2024, 10:58 a.m. UTC | #2

Hi Greg,

On 04/12/2024 11:54, Greg Ungerer wrote:
> Hi JM,
> 
> 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 ?

static struct resource mcf_uart2_resource[] = {
	[0] = {
		.start = MCFUART_BASE2,
		.end   = MCFUART_BASE2 + 0x3fff,
		.flags = IORESOURCE_MEM,
	},
	[1] = {
		.start = 6,
		.end   = 7,
		.flags = IORESOURCE_DMA,
	},
	[2] = {
		.start = MCF_IRQ_UART2,
		.end   = MCF_IRQ_UART2,
		.flags = IORESOURCE_IRQ,
	},
};

static struct platform_device mcf_uart2 = {
	.name			= "mcfuart",
	.id			= 2,
	.num_resources = ARRAY_SIZE(mcf_uart2_resource),
	.resource = mcf_uart2_resource,
	.dev = {
		.dma_mask = &mcf_uart_mask,
		.coherent_dma_mask = DMA_BIT_MASK(32),
	},
};

static struct resource mcf_uart6_resource[] = {
	[0] = {
		.start = MCFUART_BASE6,
		.end   = MCFUART_BASE6 + 0x3fff,
		.flags = IORESOURCE_MEM,
	},
	[1] = {
		.start = 22,
		.end   = 23,
		.flags = IORESOURCE_DMA,
	},
	[2] = {
		.start = MCF_IRQ_UART6,
		.end   = MCF_IRQ_UART6,
		.flags = IORESOURCE_IRQ,
	},
};

static struct platform_device mcf_uart6 = {
	.name			= "mcfuart",
	.id			= 6,
	.num_resources = ARRAY_SIZE(mcf_uart6_resource),
	.resource = mcf_uart6_resource,
	.dev = {
		.dma_mask = &mcf_uart_mask,
		.coherent_dma_mask = DMA_BIT_MASK(32),
	},
};

JM

> 
> Regards
> Greg
> 
> 
> 
>>       return 0;
>>   }
>> @@ -603,13 +618,11 @@ static int mcf_probe(struct platform_device *pdev)
>>   static void mcf_remove(struct platform_device *pdev)
>>   {
>>       struct uart_port *port;
>> -    int i;
>> +    int id = pdev->id;
>> -    for (i = 0; (i < MCF_MAXPORTS); i++) {
>> -        port = &mcf_ports[i].port;
>> -        if (port)
>> -            uart_remove_one_port(&mcf_driver, port);
>> -    }
>> +    port = &mcf_ports[id].port;
>> +    if (port)
>> +        uart_remove_one_port(&mcf_driver, port);
>>   }
>>   / 
>> ****************************************************************************/
>>
>> ---
>> base-commit: e457f18d7f25288d143c1fe024a620d0b15caec1
>> change-id: 20241202-m5441x_uart_resource-729b30c15363
>>
>> Best regards,

Greg Ungerer Dec. 4, 2024, 11:15 a.m. UTC | #3

Hi JM,

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
...

Regards
Greg



> static struct resource mcf_uart2_resource[] = {
>      [0] = {
>          .start = MCFUART_BASE2,
>          .end   = MCFUART_BASE2 + 0x3fff,
>          .flags = IORESOURCE_MEM,
>      },
>      [1] = {
>          .start = 6,
>          .end   = 7,
>          .flags = IORESOURCE_DMA,
>      },
>      [2] = {
>          .start = MCF_IRQ_UART2,
>          .end   = MCF_IRQ_UART2,
>          .flags = IORESOURCE_IRQ,
>      },
> };
> 
> static struct platform_device mcf_uart2 = {
>      .name            = "mcfuart",
>      .id            = 2,
>      .num_resources = ARRAY_SIZE(mcf_uart2_resource),
>      .resource = mcf_uart2_resource,
>      .dev = {
>          .dma_mask = &mcf_uart_mask,
>          .coherent_dma_mask = DMA_BIT_MASK(32),
>      },
> };
> 
> static struct resource mcf_uart6_resource[] = {
>      [0] = {
>          .start = MCFUART_BASE6,
>          .end   = MCFUART_BASE6 + 0x3fff,
>          .flags = IORESOURCE_MEM,
>      },
>      [1] = {
>          .start = 22,
>          .end   = 23,
>          .flags = IORESOURCE_DMA,
>      },
>      [2] = {
>          .start = MCF_IRQ_UART6,
>          .end   = MCF_IRQ_UART6,
>          .flags = IORESOURCE_IRQ,
>      },
> };
> 
> static struct platform_device mcf_uart6 = {
>      .name            = "mcfuart",
>      .id            = 6,
>      .num_resources = ARRAY_SIZE(mcf_uart6_resource),
>      .resource = mcf_uart6_resource,
>      .dev = {
>          .dma_mask = &mcf_uart_mask,
>          .coherent_dma_mask = DMA_BIT_MASK(32),
>      },
> };
> 
> JM
> 
>>
>> Regards
>> Greg
>>
>>
>>
>>>       return 0;
>>>   }
>>> @@ -603,13 +618,11 @@ static int mcf_probe(struct platform_device *pdev)
>>>   static void mcf_remove(struct platform_device *pdev)
>>>   {
>>>       struct uart_port *port;
>>> -    int i;
>>> +    int id = pdev->id;
>>> -    for (i = 0; (i < MCF_MAXPORTS); i++) {
>>> -        port = &mcf_ports[i].port;
>>> -        if (port)
>>> -            uart_remove_one_port(&mcf_driver, port);
>>> -    }
>>> +    port = &mcf_ports[id].port;
>>> +    if (port)
>>> +        uart_remove_one_port(&mcf_driver, port);
>>>   }
>>>   / ****************************************************************************/
>>>
>>> ---
>>> base-commit: e457f18d7f25288d143c1fe024a620d0b15caec1
>>> change-id: 20241202-m5441x_uart_resource-729b30c15363
>>>
>>> Best regards,
>

Jean-Michel Hautbois Dec. 4, 2024, 11:32 a.m. UTC | #4

Hi Greg,

On 04/12/2024 12:15, Greg Ungerer wrote:
> Hi JM,
> 
> 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 ?

I can add all the uarts as resources with the ifdefs like before, but on 
the M54418 it will always probe 10 devices, which sounds like a bit 
overkill ?

Thanks !
JM

> 
> Regards
> Greg
> 
> 
> 
>> static struct resource mcf_uart2_resource[] = {
>>      [0] = {
>>          .start = MCFUART_BASE2,
>>          .end   = MCFUART_BASE2 + 0x3fff,
>>          .flags = IORESOURCE_MEM,
>>      },
>>      [1] = {
>>          .start = 6,
>>          .end   = 7,
>>          .flags = IORESOURCE_DMA,
>>      },
>>      [2] = {
>>          .start = MCF_IRQ_UART2,
>>          .end   = MCF_IRQ_UART2,
>>          .flags = IORESOURCE_IRQ,
>>      },
>> };
>>
>> static struct platform_device mcf_uart2 = {
>>      .name            = "mcfuart",
>>      .id            = 2,
>>      .num_resources = ARRAY_SIZE(mcf_uart2_resource),
>>      .resource = mcf_uart2_resource,
>>      .dev = {
>>          .dma_mask = &mcf_uart_mask,
>>          .coherent_dma_mask = DMA_BIT_MASK(32),
>>      },
>> };
>>
>> static struct resource mcf_uart6_resource[] = {
>>      [0] = {
>>          .start = MCFUART_BASE6,
>>          .end   = MCFUART_BASE6 + 0x3fff,
>>          .flags = IORESOURCE_MEM,
>>      },
>>      [1] = {
>>          .start = 22,
>>          .end   = 23,
>>          .flags = IORESOURCE_DMA,
>>      },
>>      [2] = {
>>          .start = MCF_IRQ_UART6,
>>          .end   = MCF_IRQ_UART6,
>>          .flags = IORESOURCE_IRQ,
>>      },
>> };
>>
>> static struct platform_device mcf_uart6 = {
>>      .name            = "mcfuart",
>>      .id            = 6,
>>      .num_resources = ARRAY_SIZE(mcf_uart6_resource),
>>      .resource = mcf_uart6_resource,
>>      .dev = {
>>          .dma_mask = &mcf_uart_mask,
>>          .coherent_dma_mask = DMA_BIT_MASK(32),
>>      },
>> };
>>
>> JM
>>
>>>
>>> Regards
>>> Greg
>>>
>>>
>>>
>>>>       return 0;
>>>>   }
>>>> @@ -603,13 +618,11 @@ static int mcf_probe(struct platform_device 
>>>> *pdev)
>>>>   static void mcf_remove(struct platform_device *pdev)
>>>>   {
>>>>       struct uart_port *port;
>>>> -    int i;
>>>> +    int id = pdev->id;
>>>> -    for (i = 0; (i < MCF_MAXPORTS); i++) {
>>>> -        port = &mcf_ports[i].port;
>>>> -        if (port)
>>>> -            uart_remove_one_port(&mcf_driver, port);
>>>> -    }
>>>> +    port = &mcf_ports[id].port;
>>>> +    if (port)
>>>> +        uart_remove_one_port(&mcf_driver, port);
>>>>   }
>>>>   / 
>>>> ****************************************************************************/
>>>>
>>>> ---
>>>> base-commit: e457f18d7f25288d143c1fe024a620d0b15caec1
>>>> change-id: 20241202-m5441x_uart_resource-729b30c15363
>>>>
>>>> Best regards,
>>

Greg Ungerer Dec. 5, 2024, 1:01 p.m. UTC | #5

Hi JM,

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.

Regards
Greg



> I can add all the uarts as resources with the ifdefs like before, but on the M54418 it will always probe 10 devices, which sounds like a bit overkill ?
> 
> Thanks !
> JM
> 
>>
>> Regards
>> Greg
>>
>>
>>
>>> static struct resource mcf_uart2_resource[] = {
>>>      [0] = {
>>>          .start = MCFUART_BASE2,
>>>          .end   = MCFUART_BASE2 + 0x3fff,
>>>          .flags = IORESOURCE_MEM,
>>>      },
>>>      [1] = {
>>>          .start = 6,
>>>          .end   = 7,
>>>          .flags = IORESOURCE_DMA,
>>>      },
>>>      [2] = {
>>>          .start = MCF_IRQ_UART2,
>>>          .end   = MCF_IRQ_UART2,
>>>          .flags = IORESOURCE_IRQ,
>>>      },
>>> };
>>>
>>> static struct platform_device mcf_uart2 = {
>>>      .name            = "mcfuart",
>>>      .id            = 2,
>>>      .num_resources = ARRAY_SIZE(mcf_uart2_resource),
>>>      .resource = mcf_uart2_resource,
>>>      .dev = {
>>>          .dma_mask = &mcf_uart_mask,
>>>          .coherent_dma_mask = DMA_BIT_MASK(32),
>>>      },
>>> };
>>>
>>> static struct resource mcf_uart6_resource[] = {
>>>      [0] = {
>>>          .start = MCFUART_BASE6,
>>>          .end   = MCFUART_BASE6 + 0x3fff,
>>>          .flags = IORESOURCE_MEM,
>>>      },
>>>      [1] = {
>>>          .start = 22,
>>>          .end   = 23,
>>>          .flags = IORESOURCE_DMA,
>>>      },
>>>      [2] = {
>>>          .start = MCF_IRQ_UART6,
>>>          .end   = MCF_IRQ_UART6,
>>>          .flags = IORESOURCE_IRQ,
>>>      },
>>> };
>>>
>>> static struct platform_device mcf_uart6 = {
>>>      .name            = "mcfuart",
>>>      .id            = 6,
>>>      .num_resources = ARRAY_SIZE(mcf_uart6_resource),
>>>      .resource = mcf_uart6_resource,
>>>      .dev = {
>>>          .dma_mask = &mcf_uart_mask,
>>>          .coherent_dma_mask = DMA_BIT_MASK(32),
>>>      },
>>> };
>>>
>>> JM
>>>
>>>>
>>>> Regards
>>>> Greg
>>>>
>>>>
>>>>
>>>>>       return 0;
>>>>>   }
>>>>> @@ -603,13 +618,11 @@ static int mcf_probe(struct platform_device *pdev)
>>>>>   static void mcf_remove(struct platform_device *pdev)
>>>>>   {
>>>>>       struct uart_port *port;
>>>>> -    int i;
>>>>> +    int id = pdev->id;
>>>>> -    for (i = 0; (i < MCF_MAXPORTS); i++) {
>>>>> -        port = &mcf_ports[i].port;
>>>>> -        if (port)
>>>>> -            uart_remove_one_port(&mcf_driver, port);
>>>>> -    }
>>>>> +    port = &mcf_ports[id].port;
>>>>> +    if (port)
>>>>> +        uart_remove_one_port(&mcf_driver, port);
>>>>>   }
>>>>>   / ****************************************************************************/
>>>>>
>>>>> ---
>>>>> base-commit: e457f18d7f25288d143c1fe024a620d0b15caec1
>>>>> change-id: 20241202-m5441x_uart_resource-729b30c15363
>>>>>
>>>>> Best regards,
>>>
>

Jean-Michel Hautbois Dec. 5, 2024, 1:06 p.m. UTC | #6

Hi Greg,

On 05/12/2024 14:01, Greg Ungerer wrote:
> Hi JM,
> 
> 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 ?

Thanks,
JM

> 
> Regards
> Greg
> 
> 
> 
>> I can add all the uarts as resources with the ifdefs like before, but 
>> on the M54418 it will always probe 10 devices, which sounds like a bit 
>> overkill ?
>>
>> Thanks !
>> JM
>>
>>>
>>> Regards
>>> Greg
>>>
>>>
>>>
>>>> static struct resource mcf_uart2_resource[] = {
>>>>      [0] = {
>>>>          .start = MCFUART_BASE2,
>>>>          .end   = MCFUART_BASE2 + 0x3fff,
>>>>          .flags = IORESOURCE_MEM,
>>>>      },
>>>>      [1] = {
>>>>          .start = 6,
>>>>          .end   = 7,
>>>>          .flags = IORESOURCE_DMA,
>>>>      },
>>>>      [2] = {
>>>>          .start = MCF_IRQ_UART2,
>>>>          .end   = MCF_IRQ_UART2,
>>>>          .flags = IORESOURCE_IRQ,
>>>>      },
>>>> };
>>>>
>>>> static struct platform_device mcf_uart2 = {
>>>>      .name            = "mcfuart",
>>>>      .id            = 2,
>>>>      .num_resources = ARRAY_SIZE(mcf_uart2_resource),
>>>>      .resource = mcf_uart2_resource,
>>>>      .dev = {
>>>>          .dma_mask = &mcf_uart_mask,
>>>>          .coherent_dma_mask = DMA_BIT_MASK(32),
>>>>      },
>>>> };
>>>>
>>>> static struct resource mcf_uart6_resource[] = {
>>>>      [0] = {
>>>>          .start = MCFUART_BASE6,
>>>>          .end   = MCFUART_BASE6 + 0x3fff,
>>>>          .flags = IORESOURCE_MEM,
>>>>      },
>>>>      [1] = {
>>>>          .start = 22,
>>>>          .end   = 23,
>>>>          .flags = IORESOURCE_DMA,
>>>>      },
>>>>      [2] = {
>>>>          .start = MCF_IRQ_UART6,
>>>>          .end   = MCF_IRQ_UART6,
>>>>          .flags = IORESOURCE_IRQ,
>>>>      },
>>>> };
>>>>
>>>> static struct platform_device mcf_uart6 = {
>>>>      .name            = "mcfuart",
>>>>      .id            = 6,
>>>>      .num_resources = ARRAY_SIZE(mcf_uart6_resource),
>>>>      .resource = mcf_uart6_resource,
>>>>      .dev = {
>>>>          .dma_mask = &mcf_uart_mask,
>>>>          .coherent_dma_mask = DMA_BIT_MASK(32),
>>>>      },
>>>> };
>>>>
>>>> JM
>>>>
>>>>>
>>>>> Regards
>>>>> Greg
>>>>>
>>>>>
>>>>>
>>>>>>       return 0;
>>>>>>   }
>>>>>> @@ -603,13 +618,11 @@ static int mcf_probe(struct platform_device 
>>>>>> *pdev)
>>>>>>   static void mcf_remove(struct platform_device *pdev)
>>>>>>   {
>>>>>>       struct uart_port *port;
>>>>>> -    int i;
>>>>>> +    int id = pdev->id;
>>>>>> -    for (i = 0; (i < MCF_MAXPORTS); i++) {
>>>>>> -        port = &mcf_ports[i].port;
>>>>>> -        if (port)
>>>>>> -            uart_remove_one_port(&mcf_driver, port);
>>>>>> -    }
>>>>>> +    port = &mcf_ports[id].port;
>>>>>> +    if (port)
>>>>>> +        uart_remove_one_port(&mcf_driver, port);
>>>>>>   }
>>>>>>   / 
>>>>>> ****************************************************************************/
>>>>>>
>>>>>> ---
>>>>>> base-commit: e457f18d7f25288d143c1fe024a620d0b15caec1
>>>>>> change-id: 20241202-m5441x_uart_resource-729b30c15363
>>>>>>
>>>>>> Best regards,
>>>>
>>

Greg Ungerer Dec. 9, 2024, 12:30 p.m. UTC | #7

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

Jean-Michel Hautbois Dec. 9, 2024, 12:57 p.m. UTC | #8

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@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.

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

m68k: coldfire: Support resources for UART

Commit Message

Comments

Patch