[04/16] serial: mvebu-uart: support probe of multiple ports

Message ID 20171006101344.15590-5-miquel.raynal@free-electrons.com
State New
Headers show
Series
  • Support armada-37xx second UART port
Related show

Commit Message

Miquel RAYNAL Oct. 6, 2017, 10:13 a.m.
From: Allen Yan <yanwei@marvell.com>

Until now, the mvebu-uart driver only supported probing a single UART
port. However, some platforms have multiple instances of this UART
controller, and therefore the driver should support multiple ports.

In order to achieve this, we make sure to assign port->line properly,
instead of hardcoding it to zero.

Signed-off-by: Allen Yan <yanwei@marvell.com>
Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/tty/serial/mvebu-uart.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Comments

Gregory CLEMENT Oct. 6, 2017, 12:23 p.m. | #1
Hi Miquel,
 
 On ven., oct. 06 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> From: Allen Yan <yanwei@marvell.com>
>
> Until now, the mvebu-uart driver only supported probing a single UART
> port. However, some platforms have multiple instances of this UART
> controller, and therefore the driver should support multiple ports.
>
> In order to achieve this, we make sure to assign port->line properly,
> instead of hardcoding it to zero.
>
> Signed-off-by: Allen Yan <yanwei@marvell.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  drivers/tty/serial/mvebu-uart.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/mvebu-uart.c b/drivers/tty/serial/mvebu-uart.c
> index 7e0a3e9fee15..25b11ede3a97 100644
> --- a/drivers/tty/serial/mvebu-uart.c
> +++ b/drivers/tty/serial/mvebu-uart.c
> @@ -560,7 +560,16 @@ static int mvebu_uart_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	port = &mvebu_uart_ports[0];
> +	if (pdev->dev.of_node)
> +		pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");

If the id is retrieved using an of_ function, then I think that the
driver would depend on OF_CONFIG.

Gregory


> +
> +	if (pdev->id >= MVEBU_NR_UARTS) {
> +		dev_err(&pdev->dev, "cannot have more than %d UART ports\n",
> +			MVEBU_NR_UARTS);
> +		return -EINVAL;
> +	}
> +
> +	port = &mvebu_uart_ports[pdev->id];
>  
>  	spin_lock_init(&port->lock);
>  
> @@ -572,7 +581,7 @@ static int mvebu_uart_probe(struct platform_device *pdev)
>  	port->fifosize   = 32;
>  	port->iotype     = UPIO_MEM32;
>  	port->flags      = UPF_FIXED_PORT;
> -	port->line       = 0; /* single port: force line number to  0 */
> +	port->line       = pdev->id;
>  
>  	port->irq        = irq->start;
>  	port->irqflags   = 0;
> -- 
> 2.11.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Miquel RAYNAL Oct. 9, 2017, 7:17 a.m. | #2
On Fri, 06 Oct 2017 14:23:55 +0200
Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:

> Hi Miquel,
>  
>  On ven., oct. 06 2017, Miquel Raynal
> <miquel.raynal@free-electrons.com> wrote:
> 
> > From: Allen Yan <yanwei@marvell.com>
> >
> > Until now, the mvebu-uart driver only supported probing a single
> > UART port. However, some platforms have multiple instances of this
> > UART controller, and therefore the driver should support multiple
> > ports.
> >
> > In order to achieve this, we make sure to assign port->line
> > properly, instead of hardcoding it to zero.
> >
> > Signed-off-by: Allen Yan <yanwei@marvell.com>
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > ---
> >  drivers/tty/serial/mvebu-uart.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/serial/mvebu-uart.c
> > b/drivers/tty/serial/mvebu-uart.c index 7e0a3e9fee15..25b11ede3a97
> > 100644 --- a/drivers/tty/serial/mvebu-uart.c
> > +++ b/drivers/tty/serial/mvebu-uart.c
> > @@ -560,7 +560,16 @@ static int mvebu_uart_probe(struct
> > platform_device *pdev) return -EINVAL;
> >  	}
> >  
> > -	port = &mvebu_uart_ports[0];
> > +	if (pdev->dev.of_node)
> > +		pdev->id = of_alias_get_id(pdev->dev.of_node,
> > "serial");  
> 
> If the id is retrieved using an of_ function, then I think that the
> driver would depend on OF_CONFIG.

Is this okay?

if (pdev->dev.of_node && IS_ENABLED(CONFIG_OF))
        pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");
else
        pdev->id = 0;

BTW, I could not test without CONFIG_OF as it is defined by default in
our case: Selected by: ARM64 [=y]
I don't think there will be a 32-bit SoC with this UART IP?

Thanks,
Miquèl

> 
> Gregory
> 
> 
> > +
> > +	if (pdev->id >= MVEBU_NR_UARTS) {
> > +		dev_err(&pdev->dev, "cannot have more than %d UART
> > ports\n",
> > +			MVEBU_NR_UARTS);
> > +		return -EINVAL;
> > +	}
> > +
> > +	port = &mvebu_uart_ports[pdev->id];
> >  
> >  	spin_lock_init(&port->lock);
> >  
> > @@ -572,7 +581,7 @@ static int mvebu_uart_probe(struct
> > platform_device *pdev) port->fifosize   = 32;
> >  	port->iotype     = UPIO_MEM32;
> >  	port->flags      = UPF_FIXED_PORT;
> > -	port->line       = 0; /* single port: force line number
> > to  0 */
> > +	port->line       = pdev->id;
> >  
> >  	port->irq        = irq->start;
> >  	port->irqflags   = 0;
> > -- 
> > 2.11.0
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel  
>
Gregory CLEMENT Oct. 12, 2017, 12:22 p.m. | #3
Hi Miquel,
 
 On lun., oct. 09 2017, Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:

> On Fri, 06 Oct 2017 14:23:55 +0200
> Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:
>
>> Hi Miquel,
>>  
>>  On ven., oct. 06 2017, Miquel Raynal
>> <miquel.raynal@free-electrons.com> wrote:
>> 
>> > From: Allen Yan <yanwei@marvell.com>
>> >
>> > Until now, the mvebu-uart driver only supported probing a single
>> > UART port. However, some platforms have multiple instances of this
>> > UART controller, and therefore the driver should support multiple
>> > ports.
>> >
>> > In order to achieve this, we make sure to assign port->line
>> > properly, instead of hardcoding it to zero.
>> >
>> > Signed-off-by: Allen Yan <yanwei@marvell.com>
>> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
>> > ---
>> >  drivers/tty/serial/mvebu-uart.c | 13 +++++++++++--
>> >  1 file changed, 11 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/tty/serial/mvebu-uart.c
>> > b/drivers/tty/serial/mvebu-uart.c index 7e0a3e9fee15..25b11ede3a97
>> > 100644 --- a/drivers/tty/serial/mvebu-uart.c
>> > +++ b/drivers/tty/serial/mvebu-uart.c
>> > @@ -560,7 +560,16 @@ static int mvebu_uart_probe(struct
>> > platform_device *pdev) return -EINVAL;
>> >  	}
>> >  
>> > -	port = &mvebu_uart_ports[0];
>> > +	if (pdev->dev.of_node)
>> > +		pdev->id = of_alias_get_id(pdev->dev.of_node,
>> > "serial");  
>> 
>> If the id is retrieved using an of_ function, then I think that the
>> driver would depend on OF_CONFIG.
>
> Is this okay?
>
> if (pdev->dev.of_node && IS_ENABLED(CONFIG_OF))
>         pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");

Actually if CONFIG_OF is not enabled, then dev->dev.of_node. So you can
keep the test but just remove the test on IS_ENABLED(CONFIG_OF).

But my remark about CONFIG_OF, was more to point that if there is no
device tree support then this part of code won't work well if you have
several ports using the same driver.

So either you keep your driver as is but make it depends on CONFIG_OF to
make it clear that it won't work with ACPI. Or you add the case for
ACPI, but I think you don't have a board with ACPI support so you won't
be able to test it.

A third solution would be to have a fallback when of_alias_get_id failed
(either because there is no alias in the device tree or there is no
device tree at all). In this case you can just increment the id for each
new port.

Gregory


> else
>         pdev->id = 0;
>
> BTW, I could not test without CONFIG_OF as it is defined by default in
> our case: Selected by: ARM64 [=y]
> I don't think there will be a 32-bit SoC with this UART IP?
>
> Thanks,
> Miquèl
>
>> 
>> Gregory
>> 
>> 
>> > +
>> > +	if (pdev->id >= MVEBU_NR_UARTS) {
>> > +		dev_err(&pdev->dev, "cannot have more than %d UART
>> > ports\n",
>> > +			MVEBU_NR_UARTS);
>> > +		return -EINVAL;
>> > +	}
>> > +
>> > +	port = &mvebu_uart_ports[pdev->id];
>> >  
>> >  	spin_lock_init(&port->lock);
>> >  
>> > @@ -572,7 +581,7 @@ static int mvebu_uart_probe(struct
>> > platform_device *pdev) port->fifosize   = 32;
>> >  	port->iotype     = UPIO_MEM32;
>> >  	port->flags      = UPF_FIXED_PORT;
>> > -	port->line       = 0; /* single port: force line number
>> > to  0 */
>> > +	port->line       = pdev->id;
>> >  
>> >  	port->irq        = irq->start;
>> >  	port->irqflags   = 0;
>> > -- 
>> > 2.11.0
>> >
>> >
>> > _______________________________________________
>> > linux-arm-kernel mailing list
>> > linux-arm-kernel@lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel  
>> 
>
>
>
> -- 
> Miquel Raynal, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Miquel RAYNAL Oct. 13, 2017, 7:29 a.m. | #4
Hello Gregory,

On Thu, 12 Oct 2017 14:22:18 +0200
Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:

> Hi Miquel,
>  
>  On lun., oct. 09 2017, Miquel RAYNAL
> <miquel.raynal@free-electrons.com> wrote:
> 
> > On Fri, 06 Oct 2017 14:23:55 +0200
> > Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:
> >  
> >> Hi Miquel,
> >>  
> >>  On ven., oct. 06 2017, Miquel Raynal
> >> <miquel.raynal@free-electrons.com> wrote:
> >>   
> >> > From: Allen Yan <yanwei@marvell.com>
> >> >
> >> > Until now, the mvebu-uart driver only supported probing a single
> >> > UART port. However, some platforms have multiple instances of
> >> > this UART controller, and therefore the driver should support
> >> > multiple ports.
> >> >
> >> > In order to achieve this, we make sure to assign port->line
> >> > properly, instead of hardcoding it to zero.
> >> >
> >> > Signed-off-by: Allen Yan <yanwei@marvell.com>
> >> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> >> > ---
> >> >  drivers/tty/serial/mvebu-uart.c | 13 +++++++++++--
> >> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/tty/serial/mvebu-uart.c
> >> > b/drivers/tty/serial/mvebu-uart.c index
> >> > 7e0a3e9fee15..25b11ede3a97 100644 ---
> >> > a/drivers/tty/serial/mvebu-uart.c +++
> >> > b/drivers/tty/serial/mvebu-uart.c @@ -560,7 +560,16 @@ static
> >> > int mvebu_uart_probe(struct platform_device *pdev) return
> >> > -EINVAL; }
> >> >  
> >> > -	port = &mvebu_uart_ports[0];
> >> > +	if (pdev->dev.of_node)
> >> > +		pdev->id = of_alias_get_id(pdev->dev.of_node,
> >> > "serial");    
> >> 
> >> If the id is retrieved using an of_ function, then I think that the
> >> driver would depend on OF_CONFIG.  
> >
> > Is this okay?
> >
> > if (pdev->dev.of_node && IS_ENABLED(CONFIG_OF))
> >         pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");  
> 
> Actually if CONFIG_OF is not enabled, then dev->dev.of_node. So you
> can keep the test but just remove the test on IS_ENABLED(CONFIG_OF).
> 
> But my remark about CONFIG_OF, was more to point that if there is no
> device tree support then this part of code won't work well if you have
> several ports using the same driver.

Ok.

> 
> So either you keep your driver as is but make it depends on CONFIG_OF
> to make it clear that it won't work with ACPI. Or you add the case for
> ACPI, but I think you don't have a board with ACPI support so you
> won't be able to test it.
> 
> A third solution would be to have a fallback when of_alias_get_id
> failed (either because there is no alias in the device tree or there
> is no device tree at all). In this case you can just increment the id
> for each new port.

This is the solution I choose. I used this driver as an example:
drivers/gpu/drm/omapdrm/dss/display.c

Please have a look at it in the next version.

Thanks,
Miquèl

> 
> Gregory
> 
> 
> > else
> >         pdev->id = 0;
> >
> > BTW, I could not test without CONFIG_OF as it is defined by default
> > in our case: Selected by: ARM64 [=y]
> > I don't think there will be a 32-bit SoC with this UART IP?
> >
> > Thanks,
> > Miquèl
> >  
> >> 
> >> Gregory
> >> 
> >>   
> >> > +
> >> > +	if (pdev->id >= MVEBU_NR_UARTS) {
> >> > +		dev_err(&pdev->dev, "cannot have more than %d
> >> > UART ports\n",
> >> > +			MVEBU_NR_UARTS);
> >> > +		return -EINVAL;
> >> > +	}
> >> > +
> >> > +	port = &mvebu_uart_ports[pdev->id];
> >> >  
> >> >  	spin_lock_init(&port->lock);
> >> >  
> >> > @@ -572,7 +581,7 @@ static int mvebu_uart_probe(struct
> >> > platform_device *pdev) port->fifosize   = 32;
> >> >  	port->iotype     = UPIO_MEM32;
> >> >  	port->flags      = UPF_FIXED_PORT;
> >> > -	port->line       = 0; /* single port: force line number
> >> > to  0 */
> >> > +	port->line       = pdev->id;
> >> >  
> >> >  	port->irq        = irq->start;
> >> >  	port->irqflags   = 0;
> >> > -- 
> >> > 2.11.0
> >> >
> >> >
> >> > _______________________________________________
> >> > linux-arm-kernel mailing list
> >> > linux-arm-kernel@lists.infradead.org
> >> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel    
> >>   
> >
> >
> >
> > -- 
> > Miquel Raynal, Free Electrons
> > Embedded Linux and Kernel engineering
> > http://free-electrons.com  
>

Patch

diff --git a/drivers/tty/serial/mvebu-uart.c b/drivers/tty/serial/mvebu-uart.c
index 7e0a3e9fee15..25b11ede3a97 100644
--- a/drivers/tty/serial/mvebu-uart.c
+++ b/drivers/tty/serial/mvebu-uart.c
@@ -560,7 +560,16 @@  static int mvebu_uart_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	port = &mvebu_uart_ports[0];
+	if (pdev->dev.of_node)
+		pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");
+
+	if (pdev->id >= MVEBU_NR_UARTS) {
+		dev_err(&pdev->dev, "cannot have more than %d UART ports\n",
+			MVEBU_NR_UARTS);
+		return -EINVAL;
+	}
+
+	port = &mvebu_uart_ports[pdev->id];
 
 	spin_lock_init(&port->lock);
 
@@ -572,7 +581,7 @@  static int mvebu_uart_probe(struct platform_device *pdev)
 	port->fifosize   = 32;
 	port->iotype     = UPIO_MEM32;
 	port->flags      = UPF_FIXED_PORT;
-	port->line       = 0; /* single port: force line number to  0 */
+	port->line       = pdev->id;
 
 	port->irq        = irq->start;
 	port->irqflags   = 0;