diff mbox

[16/16] arm64: dts: marvell: armada-3720-espressobin: fill UART nodes

Message ID 20171006101344.15590-17-miquel.raynal@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Miquel Raynal Oct. 6, 2017, 10:13 a.m. UTC
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>
---
 arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Gregory CLEMENT Oct. 6, 2017, 1:01 p.m. UTC | #1
Hi Miquel,
 
 On ven., oct. 06 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>
> ---
>  arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 8 ++++++++
>  1 file changed, 8 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..c05b274ab1a9 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> @@ -98,9 +98,17 @@
>  
>  /* Exported on the micro USB connector J5 through an FTDI */
>  &uart0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart1_pins>;
>  	status = "okay";
>  };
>  
> +/*
> + * Enabling the second UART on J17 (pins 24,26) is just a matter of copying the
> + * uart1 node from armada-3720-db.dts with one difference: it works with 1.8V
> + * TTL levels.

This difference is not related to the device tree. So I would write:
 /*
  * To enable the second UART on J17 (pins 24,26) refer to the uart1
  * node from armada-3720-db.dts.
  * Note that TX and RX signal are the ones coming directly from the SoC:
  * 1.8V TTL.
  */

Thanks,

Gregory

> + */
> +
>  /* J7 */
>  &usb3 {
>  	status = "okay";
> -- 
> 2.11.0
>
Thomas Petazzoni Oct. 6, 2017, 1:15 p.m. UTC | #2
Hello,

On Fri, 06 Oct 2017 15:01:18 +0200, Gregory CLEMENT wrote:

>  /*
>   * To enable the second UART on J17 (pins 24,26) refer to the uart1
>   * node from armada-3720-db.dts.
>   * Note that TX and RX signal are the ones coming directly from the SoC:
>   * 1.8V TTL.
>   */

One issue with this comment (and Miquèl's version as well) is that it
does not explain why you don't enable this UART by default.

The real reason is in the commit log from Miquèl, and should probably
be part of the comment. Perhaps something like:

/*

 * Connector J17 (pins X, Y, Z) exposes a number of different
 * features:
 *  - UART1 (pins 24 = RX, pins 26 = TX), see armada-3720-db.dts for an
 *    example on how to enable UART1. Beware that the signals are 1.8V
 *    TTL.
 *  - SPIxyz
 *  - I2Cxyz
 */

Otherwise, it's not clear at all why you don't just enable UART1. Or
perhaps I misunderstood Miquèl's commit log ?

Best regards,

Thomas
Miquel Raynal Oct. 9, 2017, 7:30 a.m. UTC | #3
Hi Thomas, Gregory,

On Fri, 6 Oct 2017 15:15:21 +0200
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> Hello,
> 
> On Fri, 06 Oct 2017 15:01:18 +0200, Gregory CLEMENT wrote:
> 
> >  /*
> >   * To enable the second UART on J17 (pins 24,26) refer to the uart1
> >   * node from armada-3720-db.dts.
> >   * Note that TX and RX signal are the ones coming directly from
> > the SoC:
> >   * 1.8V TTL.
> >   */  
> 
> One issue with this comment (and Miquèl's version as well) is that it
> does not explain why you don't enable this UART by default.
> 
> The real reason is in the commit log from Miquèl, and should probably
> be part of the comment. Perhaps something like:
> 
> /*
> 
>  * Connector J17 (pins X, Y, Z) exposes a number of different
>  * features:
>  *  - UART1 (pins 24 = RX, pins 26 = TX), see armada-3720-db.dts for
> an
>  *    example on how to enable UART1. Beware that the signals are 1.8V
>  *    TTL.
>  *  - SPIxyz
>  *  - I2Cxyz
>  */

Thanks for both your comments, there is my version, inspired from both
comments:

/*
 * Connector J17 exposes a number of different features. Some pins are
 * multiplexed. This is the case for the UART1 feature (pins 24 = RX,
 * pins 26 = TX). See armada-3720-db.dts for an example of how to enable it.
 * Beware that the signals are 1.8V TTL.
 */

Thanks,
Miquèl

> 
> Otherwise, it's not clear at all why you don't just enable UART1. Or
> perhaps I misunderstood Miquèl's commit log ?
> 
> Best regards,
> 
> Thomas
Gregory CLEMENT Oct. 12, 2017, 11:24 a.m. UTC | #4
Hi Miquel,
 
 On lun., oct. 09 2017, Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:

> Hi Thomas, Gregory,
>
> On Fri, 6 Oct 2017 15:15:21 +0200
> Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:
>
>> Hello,
>> 
>> On Fri, 06 Oct 2017 15:01:18 +0200, Gregory CLEMENT wrote:
>> 
>> >  /*
>> >   * To enable the second UART on J17 (pins 24,26) refer to the uart1
>> >   * node from armada-3720-db.dts.
>> >   * Note that TX and RX signal are the ones coming directly from
>> > the SoC:
>> >   * 1.8V TTL.
>> >   */  
>> 
>> One issue with this comment (and Miquèl's version as well) is that it
>> does not explain why you don't enable this UART by default.
>> 
>> The real reason is in the commit log from Miquèl, and should probably
>> be part of the comment. Perhaps something like:
>> 
>> /*
>> 
>>  * Connector J17 (pins X, Y, Z) exposes a number of different
>>  * features:
>>  *  - UART1 (pins 24 = RX, pins 26 = TX), see armada-3720-db.dts for
>> an
>>  *    example on how to enable UART1. Beware that the signals are 1.8V
>>  *    TTL.
>>  *  - SPIxyz
>>  *  - I2Cxyz
>>  */
>
> Thanks for both your comments, there is my version, inspired from both
> comments:
>
> /*
>  * Connector J17 exposes a number of different features. Some pins are
>  * multiplexed. This is the case for the UART1 feature (pins 24 = RX,
>  * pins 26 = TX). See armada-3720-db.dts for an example of how to enable it.
>  * Beware that the signals are 1.8V TTL.
>  */

Seems good for me however I prefer Thomas version, easier to read and to
extend latter with the description of other pins if needed.

Gregory

>
> Thanks,
> Miquèl
>
>> 
>> Otherwise, it's not clear at all why you don't just enable UART1. Or
>> perhaps I misunderstood Miquèl's commit log ?
>> 
>> Best regards,
>> 
>> Thomas
>
>
>
> -- 
> Miquel Raynal, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Miquel Raynal Oct. 13, 2017, 7:01 a.m. UTC | #5
Hello Gregory,

On Thu, 12 Oct 2017 13:24:42 +0200
Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:

> Hi Miquel,
>  
>  On lun., oct. 09 2017, Miquel RAYNAL
> <miquel.raynal@free-electrons.com> wrote:
> 
> > Hi Thomas, Gregory,
> >
> > On Fri, 6 Oct 2017 15:15:21 +0200
> > Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:
> >  
> >> Hello,
> >> 
> >> On Fri, 06 Oct 2017 15:01:18 +0200, Gregory CLEMENT wrote:
> >>   
> >> >  /*
> >> >   * To enable the second UART on J17 (pins 24,26) refer to the
> >> > uart1
> >> >   * node from armada-3720-db.dts.
> >> >   * Note that TX and RX signal are the ones coming directly from
> >> > the SoC:
> >> >   * 1.8V TTL.
> >> >   */    
> >> 
> >> One issue with this comment (and Miquèl's version as well) is that
> >> it does not explain why you don't enable this UART by default.
> >> 
> >> The real reason is in the commit log from Miquèl, and should
> >> probably be part of the comment. Perhaps something like:
> >> 
> >> /*
> >> 
> >>  * Connector J17 (pins X, Y, Z) exposes a number of different
> >>  * features:
> >>  *  - UART1 (pins 24 = RX, pins 26 = TX), see armada-3720-db.dts
> >> for an
> >>  *    example on how to enable UART1. Beware that the signals are
> >> 1.8V
> >>  *    TTL.
> >>  *  - SPIxyz
> >>  *  - I2Cxyz
> >>  */  
> >
> > Thanks for both your comments, there is my version, inspired from
> > both comments:
> >
> > /*
> >  * Connector J17 exposes a number of different features. Some pins
> > are
> >  * multiplexed. This is the case for the UART1 feature (pins 24 =
> > RX,
> >  * pins 26 = TX). See armada-3720-db.dts for an example of how to
> > enable it.
> >  * Beware that the signals are 1.8V TTL.
> >  */  
> 
> Seems good for me however I prefer Thomas version, easier to read and
> to extend latter with the description of other pins if needed.

Ok, I reused the 'dash' presentation to be later extended.

See v2 coming soon.

Thanks,
Miquèl

> 
> Gregory
> 
> >
> > Thanks,
> > Miquèl
> >  
> >> 
> >> Otherwise, it's not clear at all why you don't just enable UART1.
> >> Or perhaps I misunderstood Miquèl's commit log ?
> >> 
> >> Best regards,
> >> 
> >> Thomas  
> >
> >
> >
> > -- 
> > Miquel Raynal, Free Electrons
> > Embedded Linux and Kernel engineering
> > http://free-electrons.com  
>
diff mbox

Patch

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
index 2ce52ba74f73..c05b274ab1a9 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
@@ -98,9 +98,17 @@ 
 
 /* Exported on the micro USB connector J5 through an FTDI */
 &uart0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart1_pins>;
 	status = "okay";
 };
 
+/*
+ * Enabling the second UART on J17 (pins 24,26) is just a matter of copying the
+ * uart1 node from armada-3720-db.dts with one difference: it works with 1.8V
+ * TTL levels.
+ */
+
 /* J7 */
 &usb3 {
 	status = "okay";