mbox series

[v2,00/16] Support armada-37xx second UART port

Message ID 20171013090200.31034-1-miquel.raynal@free-electrons.com
Headers show
Series Support armada-37xx second UART port | expand

Message

Miquel Raynal Oct. 13, 2017, 9:01 a.m. UTC 
Changes since v1:
- Added a fallback in probe() to allow the use of this driver without
  device tree.
- Used devm_free_irq() instead of free_irq().
- Changed comments on ESPRESSObin DTS.
- Fixed various typos.
- Added Gregory's Acked-by/Reviewed-by tags.


Hi,

This series adds support for the armada-37xx extended UART port. The new
capabilities of this IP compared to the standard port are the possibility
to use DMA, it has bigger FIFOs (128 bytes instead of 32/64), it is
possible to fill the FIFO byte-per-byte or 4 bytes at a time, it as RTS
and CTS control, it supports transfer count for RX/TX with transfer
finish status and finally it may achieve higher baudrates by the use of
special dividers.

Almost none of these extended features are implemented though. For now,
the extended UART is just supported with the same features as the
standard UART.

This IP is present on the Armada 3720 SoC and is now enabled by the
device tree related patches on the Armada-3720-DB. As adding the node to
enable the second port on Armada-3720-ESPRESSObin could break existing
users by changing the configuration of some pins on the main headers,
only a side note is added on how to do it easily.

- The driver patches should go through Greg KH.
- The DT patches should go through the mvebu maintainers.
- We would also like the DT binding documentation update to go through
  the mvebu maintainers, as it conflicts with another DT change.

Thank you,
Miquel


Allen Yan (5):
  serial: mvebu-uart: support probe of multiple ports
  serial: mvebu-uart: add soft reset at probe
  serial: mvebu-uart: add function to change baudrate
  serial: mvebu-uart: clear state register before IRQ request
  serial: mvebu-uart: add TX interrupt trigger for pulse interrupts

Miquel Raynal (10):
  dt-bindings: mvebu-uart: update documentation with extended UART
  pinctrl: dt-bindings: Fix A37xx uart2 group name
  serial: mvebu-uart: use a generic way to access the registers
  serial: mvebu-uart: dissociate RX and TX interrupts
  serial: mvebu-uart: augment the maximum number of ports
  serial: mvebu-uart: support extended port registers layout
  arm64: dts: marvell: armada-37xx: add UART clock
  arm64: dts: marvell: armada-37xx: add second UART port
  arm64: dts: marvell: armada-3720-db: enable second UART port
  arm64: dts: marvell: armada-3720-espressobin: fill UART nodes

Yehuda Yitschak (1):
  serial: mvebu-uart: use driver name when requesting an interrupt

 .../pinctrl/marvell,armada-37xx-pinctrl.txt        |   4 +-
 .../devicetree/bindings/serial/mvebu-uart.txt      |  49 ++-
 arch/arm64/boot/dts/marvell/armada-3720-db.dts     |   9 +-
 .../boot/dts/marvell/armada-3720-espressobin.dts   |  12 +
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi       |  19 +-
 drivers/tty/serial/mvebu-uart.c                    | 483 +++++++++++++++++----
 6 files changed, 473 insertions(+), 103 deletions(-)

Comments

Gregory CLEMENT Oct. 13, 2017, 9:40 a.m. UTC | #1 
Hi Miquel,
 
 On ven., oct. 13 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> -	port = &mvebu_uart_ports[0];
> +	/* Assume that all UART ports have a DT alias or none has */
> +	id = of_alias_get_id(pdev->dev.of_node, "serial");

You stil need to check the pdev->dev.of_node before calling
of_alias_get_id.

Once it will be fixed, you can add:

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory


> +	if (!pdev->dev.of_node || id < 0)
> +		pdev->id = uart_num_counter++;
> +	else
> +		pdev->id = id;
> +
> +	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 +588,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
>
Miquel Raynal Oct. 13, 2017, 11:17 a.m. UTC | #2 
Hi Gregory,

On Fri, 13 Oct 2017 11:40:32 +0200
Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:

> Hi Miquel,
>  
>  On ven., oct. 13 2017, Miquel Raynal
> <miquel.raynal@free-electrons.com> wrote:
> 
> > -	port = &mvebu_uart_ports[0];
> > +	/* Assume that all UART ports have a DT alias or none has
> > */
> > +	id = of_alias_get_id(pdev->dev.of_node, "serial");  
> 
> You stil need to check the pdev->dev.of_node before calling
> of_alias_get_id.

Thanks for your feedback but I don't think we still need to check it,
because the of_alias_get_id() function will either return -ENOSYS if
CONFIG_OF is not defined, or -ENODEV if the node is not found. As the
function does not dereference pdev->dev.of_node at any moment but
instead compares the value with another pointer, I think this call is
safe like this.

Thanks,
Miquèl

> 
> Once it will be fixed, you can add:
> 
> Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> 
> Thanks,
> 
> Gregory
> 
> 
> > +	if (!pdev->dev.of_node || id < 0)
> > +		pdev->id = uart_num_counter++;
> > +	else
> > +		pdev->id = id;
> > +
> > +	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 +588,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
> >  
>
Gregory CLEMENT Oct. 13, 2017, 11:22 a.m. UTC | #3 
Hi Miquel,
 
 On ven., oct. 13 2017, Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:

> Hi Gregory,
>
> On Fri, 13 Oct 2017 11:40:32 +0200
> Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:
>
>> Hi Miquel,
>>  
>>  On ven., oct. 13 2017, Miquel Raynal
>> <miquel.raynal@free-electrons.com> wrote:
>> 
>> > -	port = &mvebu_uart_ports[0];
>> > +	/* Assume that all UART ports have a DT alias or none has
>> > */
>> > +	id = of_alias_get_id(pdev->dev.of_node, "serial");  
>> 
>> You stil need to check the pdev->dev.of_node before calling
>> of_alias_get_id.
>
> Thanks for your feedback but I don't think we still need to check it,
> because the of_alias_get_id() function will either return -ENOSYS if
> CONFIG_OF is not defined, or -ENODEV if the node is not found. As the
> function does not dereference pdev->dev.of_node at any moment but
> instead compares the value with another pointer, I think this call is
> safe like this.

Fair enough, I expected that the pointer was dereferenced but I was
wrong, was so no need for a new version.

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory

>
> Thanks,
> Miquèl
>
>> 
>> Once it will be fixed, you can add:
>> 
>> Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> 
>> Thanks,
>> 
>> Gregory
>> 
>> 
>> > +	if (!pdev->dev.of_node || id < 0)
>> > +		pdev->id = uart_num_counter++;
>> > +	else
>> > +		pdev->id = id;
>> > +
>> > +	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 +588,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
>> >  
>> 
>
>
>
> -- 
> Miquel Raynal, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Gregory CLEMENT Oct. 30, 2017, 3:45 p.m. UTC | #4 
Hi Miquel,
 
 On ven., oct. 13 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Add the missing clock property to armada-3700 UART node.
>
> This clock will be used to derive the prescaler value to comply with
> the requested baudrate.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Applied on mvebu/dt64

Thanks,

Gregory

> ---
>  arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> index b554cdaf5e53..a36d667f770e 100644
> --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> @@ -135,6 +135,7 @@
>  			uart0: serial@12000 {
>  				compatible = "marvell,armada-3700-uart";
>  				reg = <0x12000 0x200>;
> +				clocks = <&xtalclk>;
>  				interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
>  				status = "disabled";
>  			};
> -- 
> 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. 30, 2017, 3:45 p.m. UTC | #5 
Hi Miquel,
 
 On ven., oct. 13 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Add a node in Armada 37xx DTSI file for the second UART, with a
> different compatible due to its extended IP which has some
> differences with the first UART already in place.
>
> Make use of this commit to also fully describe the first port and
> use the same clear and named interrupt bindings for both ports.
>
> The standard UART (UART0) uses level-interrupts while the extended
> UART (UART1) uses edge-triggered interrupts.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Applied on mvebu/dt64

Thanks,

Gregory

> ---
>  arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> index a36d667f770e..72b68f23c001 100644
> --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> @@ -55,6 +55,7 @@
>  
>  	aliases {
>  		serial0 = &uart0;
> +		serial1 = &uart1;
>  	};
>  
>  	cpus {
> @@ -136,7 +137,22 @@
>  				compatible = "marvell,armada-3700-uart";
>  				reg = <0x12000 0x200>;
>  				clocks = <&xtalclk>;
> -				interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> +				interrupts =
> +				<GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>,
> +				<GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
> +				<GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
> +				interrupt-names = "uart-sum", "uart-tx", "uart-rx";
> +				status = "disabled";
> +			};
> +
> +			uart1: serial@12200 {
> +				compatible = "marvell,armada-3700-uart-ext";
> +				reg = <0x12200 0x30>;
> +				clocks = <&xtalclk>;
> +				interrupts =
> +				<GIC_SPI 30 IRQ_TYPE_EDGE_RISING>,
> +				<GIC_SPI 31 IRQ_TYPE_EDGE_RISING>;
> +				interrupt-names = "uart-tx", "uart-rx";
>  				status = "disabled";
>  			};
>  
> -- 
> 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. 30, 2017, 3:46 p.m. UTC | #6 
Hi Miquel,
 
 On ven., oct. 13 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Enable Armada-3720-DB second UART port by adding the corresponding
> device tree node in the board DTS and enabling it.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---

Applied on mvebu/dt64

Thanks,

Gregory

>  arch/arm64/boot/dts/marvell/armada-3720-db.dts | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-db.dts b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
> index 9df0f06ce607..15713c19b3d0 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-db.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
> @@ -216,7 +216,7 @@
>  
>  /*
>   * Exported on the micro USB connector CON30(V2.0)/CON32(V1.4) through
> - * an FTDI
> + * an FTDI (also on CON24(V2.0)/CON26(V1.4)).
>   */
>  &uart0 {
>  	pinctrl-names = "default";
> @@ -224,6 +224,13 @@
>  	status = "okay";
>  };
>  
> +/* CON26(V2.0)/CON28(V1.4) */
> +&uart1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart2_pins>;
> +	status = "okay";
> +};
> +
>  /* CON27(V2.0)/CON29(V1.4) */
>  &usb2 {
>  	status = "okay";
> -- 
> 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. 30, 2017, 3:46 p.m. UTC | #7 
Hi Miquel,
 
 On ven., oct. 13 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Fill ESPRESSObin uart0 node with pinctrl information like in the
> Armada-3720-DB device tree (which uses the same node).
>
> Also explain how to enable the second UART port available on the
> headers. This second port is not enabled by default because both
> headers are dedicated to expose general purpose pins and remapping
> some of them to use the second UART would break existing users.
>
> Suggested-by: László ÁSHIN <laszlo@ashin.hu>
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>

Applied on mvebu/dt64

Thanks,

Gregory

> ---
>
> Changes since v1: comment about UART1 node.
>
>  arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> index 2ce52ba74f73..bdfb5553ddb5 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> @@ -98,9 +98,21 @@
>  
>  /* Exported on the micro USB connector J5 through an FTDI */
>  &uart0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart1_pins>;
>  	status = "okay";
>  };
>  
> +/*
> + * Connector J17 and J18 expose a number of different features. Some pins are
> + * multiplexed. This is the case for instance for the following features:
> + * - UART1 (pin 24 = RX, pin 26 = TX). See armada-3720-db.dts for an example of
> + *   how to enable it. Beware that the signals are 1.8V TTL.
> + * - I2C
> + * - SPI
> + * - MMC
> + */
> +
>  /* J7 */
>  &usb3 {
>  	status = "okay";
> -- 
> 2.11.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel