diff mbox

[2/2] ARM: tegra: Add high speed UARTs to Jetson TK1 device tree

Message ID 1453209155-6213-3-git-send-email-ralf@ramses-pyramidenbau.de
State Changes Requested
Headers show

Commit Message

Ralf Ramsauer Jan. 19, 2016, 1:12 p.m. UTC 
This patch enables the APB DMA high speed UARTs of the Jetson TK1.

Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
---
 arch/arm/boot/dts/tegra124-jetson-tk1.dts | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Stephen Warren Jan. 19, 2016, 4:32 p.m. UTC | #1 
On 01/19/2016 06:12 AM, Ralf Ramsauer wrote:
> This patch enables the APB DMA high speed UARTs of the Jetson TK1.

In the cover letter, you mentioned that these ports are exposed on the 
expansion connector. It'd be useful to mention that fact here too, since 
the cover letter doesn't make it into the git commit log.

However, given that, I don't think we should apply this patch. We can't 
know ahead of time what hardware is connected to the expansion header; 
the pins aren't dedicated to be serial ports (they could be GPIOs and 
probably other functions too depending on the pinmux settings), and 
different people will connect different hardware or none at all. Hence, 
the default DT should not enable IO controllers for the pins on the 
expansion connector. The best approach these days is to use DT overlays. 
You may need to add some "hooks" into the Tegra base DT files to allow 
overlays to be used; they weren't written with overlays in mind.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ralf Ramsauer Jan. 19, 2016, 5:16 p.m. UTC | #2 
Thanks for review, Stephen.

On 01/19/16 17:32, Stephen Warren wrote:
> On 01/19/2016 06:12 AM, Ralf Ramsauer wrote:
>> This patch enables the APB DMA high speed UARTs of the Jetson TK1.
>
> In the cover letter, you mentioned that these ports are exposed on the
> expansion connector. It'd be useful to mention that fact here too,
> since the cover letter doesn't make it into the git commit log.
Good point.
>
> However, given that, I don't think we should apply this patch. We
> can't know ahead of time what hardware is connected to the expansion
> header; the pins aren't dedicated to be serial ports (they could be
> GPIOs and probably other functions too depending on the pinmux
> settings), and different people will connect different hardware or
> none at all. Hence, the default DT should not enable IO controllers
> for the pins on the expansion connector. The best approach these days
> is to use DT overlays. You may need to add some "hooks" into the Tegra
> base DT files to allow overlays to be used; they weren't written with
> overlays in mind.
If the argument is that those pins could also differently be used, then
the same would also apply to I2C busses, which are apparently _all_
exported in the current device tree, even if no hardware is connected by
default. Those pins are also available on the expansion headers.

You are right that those pins of the CPU can generally be used as GPIOs,
but regarding the Jetson TK1 board, the meaning of those pins on the
expansion header is the UART functionality. This is also what the
official documentation of Nvidia states on those pins [2]. So if you do
want to have a special non-UART setup, then you should have to disable
them on your own, by default they should be enabled.

On the Jetson TK1 board, only GPIO_PU* and GPIO_PH* should be used as
GPIOs [2].

  Ralf

[1]
http://developer.download.nvidia.com/embedded/jetson/TK1/2014-03-24/JetsonTK1_ModuleSpecification_PM375_V1.0.pdf
[2] http://elinux.org/Jetson/GPIO#Jetson_TK1_GPIO_pinouts
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Jan. 20, 2016, 10:54 p.m. UTC | #3 
On 01/19/2016 10:16 AM, Ralf Ramsauer wrote:
> Thanks for review, Stephen.
>
> On 01/19/16 17:32, Stephen Warren wrote:
>> On 01/19/2016 06:12 AM, Ralf Ramsauer wrote:
>>> This patch enables the APB DMA high speed UARTs of the Jetson TK1.
>>
>> In the cover letter, you mentioned that these ports are exposed on the
>> expansion connector. It'd be useful to mention that fact here too,
>> since the cover letter doesn't make it into the git commit log.
> Good point.
>>
>> However, given that, I don't think we should apply this patch. We
>> can't know ahead of time what hardware is connected to the expansion
>> header; the pins aren't dedicated to be serial ports (they could be
>> GPIOs and probably other functions too depending on the pinmux
>> settings), and different people will connect different hardware or
>> none at all. Hence, the default DT should not enable IO controllers
>> for the pins on the expansion connector. The best approach these days
>> is to use DT overlays. You may need to add some "hooks" into the Tegra
>> base DT files to allow overlays to be used; they weren't written with
>> overlays in mind.
 >
> If the argument is that those pins could also differently be used, then
> the same would also apply to I2C busses, which are apparently _all_
> exported in the current device tree, even if no hardware is connected by
> default. Those pins are also available on the expansion headers.
>
> You are right that those pins of the CPU can generally be used as GPIOs,
> but regarding the Jetson TK1 board, the meaning of those pins on the
> expansion header is the UART functionality. This is also what the
> official documentation of Nvidia states on those pins [2]. So if you do
> want to have a special non-UART setup, then you should have to disable
> them on your own, by default they should be enabled.
>
> On the Jetson TK1 board, only GPIO_PU* and GPIO_PH* should be used as
> GPIOs [2].

Sorry, you're correct for this board. On other board I've dealt with 
(e.g. RPi, and NVIDIA's more recent Jetson TX1) the designers have been 
more careful not to commit any of the expansion pins to a particular use 
without end-user configuration.

I'll go back and re-review this commit with this in mind.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Jan. 20, 2016, 11:13 p.m. UTC | #4 
On 01/19/2016 06:12 AM, Ralf Ramsauer wrote:
> This patch enables the APB DMA high speed UARTs of the Jetson TK1.

> diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts b/arch/arm/boot/dts/tegra124-jetson-tk1.dts

> +	/* First high speed UART */
> +	serial@0,70006000 {
> +		compatible = "nvidia,tegra124-hsuart", "nvidia,tegra30-hsuart";
> +		status = "okay";
> +	};

It would be nice if the comment described the HW connectivity, i.e. 
which signals the UART was connected to on the board, just like the 
comments for other IO controllers already enabled in the DT file. I'd 
suggest replacing the comment above with:

/* Expansion BR_UART1_RXD/_TXD */

> +	/* Second high speed UART */
> +	serial@0,70006040 {
> +		compatible = "nvidia,tegra124-hsuart", "nvidia,tegra30-hsuart";
> +		status = "okay";
> +	};

... and that commetn with:

/* Expansion UART2_RXD/_TXD/_RTS/_CTS */

> +
> +	/* Third high speed UART */
> +	serial@0,70006200 {
> +		compatible = "nvidia,tegra124-hsuart", "nvidia,tegra30-hsuart";
> +		status = "okay";
> +	};

That UART doesn't seem to be used at all according to the schematics and 
pinmux spreadsheet. Do you have any reference to the contrary aside from 
the L4T DT file? I believe it shouldn't be enabled.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ralf Ramsauer Jan. 21, 2016, 12:10 a.m. UTC | #5 
Hi Stephen,

On 01/21/16 00:13, Stephen Warren wrote:
> On 01/19/2016 06:12 AM, Ralf Ramsauer wrote:
>> This patch enables the APB DMA high speed UARTs of the Jetson TK1.
>
>> diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
>> b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
>
>> +    /* First high speed UART */
>> +    serial@0,70006000 {
>> +        compatible = "nvidia,tegra124-hsuart", "nvidia,tegra30-hsuart";
>> +        status = "okay";
>> +    };
>
> It would be nice if the comment described the HW connectivity, i.e.
> which signals the UART was connected to on the board, just like the
> comments for other IO controllers already enabled in the DT file. I'd
> suggest replacing the comment above with:
>
> /* Expansion BR_UART1_RXD/_TXD */
I'll be more verbose with commenting in the next round
>
>> +    /* Second high speed UART */
>> +    serial@0,70006040 {
>> +        compatible = "nvidia,tegra124-hsuart", "nvidia,tegra30-hsuart";
>> +        status = "okay";
>> +    };
>
> ... and that commetn with:
>
> /* Expansion UART2_RXD/_TXD/_RTS/_CTS */
Ack
>
>> +
>> +    /* Third high speed UART */
>> +    serial@0,70006200 {
>> +        compatible = "nvidia,tegra124-hsuart", "nvidia,tegra30-hsuart";
>> +        status = "okay";
>> +    };
>
> That UART doesn't seem to be used at all according to the schematics
> and pinmux spreadsheet. Do you have any reference to the contrary
> aside from the L4T DT file? I believe it shouldn't be enabled.
Just checked it, and yes, you're absolutely right. I used the L4T device
tree as reference. Can't tell you why, but it appears that they
activated all uarts.
This is a short excerpt of Nvidias official DT:

            serial@70006000 {
                    compatible = "nvidia,tegra114-hsuart";
                    status = "okay";
            };

            serial@70006040 {
                    compatible = "nvidia,tegra114-hsuart";
                    status = "okay";
            };

            serial@70006200 {
                    compatible = "nvidia,tegra114-hsuart";
                    status = "okay";
            };

        serial@70006300 {
            compatible = "nvidia,tegra20-uart", "nvidia,tegra114-hsuart";
            console-port;
            sqa-automation-port;
            status = "okay";
        };


This is a short excerpt of /proc/iomem of a Jetson TK1 running latest
stock L4T linux:

    70006000-7000603f : /serial@70006000
    70006040-7000607f : /serial@70006040
    70006200-7000623f : /serial@70006200
    70006300-7000631f : serial

Let me doublecheck that again - in fact only two of three high speed
UARTs are actually exposed to the expansion header. (besides the strange
fact, that nvidia seems to have all uarts enabled...)

According to [1]:
uart1 - expansion connector
uart2 - expansion connector
uart3 - ???
uart4 - DB9 RS232 connector

I'll check this in a few days again. Thanks for review, Stephen!

Ralf

[1]
http://developer.download.nvidia.com/embedded/jetson/TK1/2014-03-24/JetsonTK1_ModuleSpecification_PM375_V1.0.pdf
Ralf Ramsauer Jan. 26, 2016, 4:26 p.m. UTC | #6 
Hi,

Now it's for sure: According to the TK1 schematics [1], uart3 is not
connected. Hence it makes no sense to enable it.
I'll prepare a next patch round.

Cheers
  Ralf

[1]
https://developer.nvidia.com/rdp/assets/jetson-tk1-datasheet-orcad-schematics
(warning, restricted access)

On 01/21/16 00:13, Stephen Warren wrote:
> On 01/19/2016 06:12 AM, Ralf Ramsauer wrote:
>> This patch enables the APB DMA high speed UARTs of the Jetson TK1.
>
>> diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
>> b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
>
>> +    /* First high speed UART */
>> +    serial@0,70006000 {
>> +        compatible = "nvidia,tegra124-hsuart", "nvidia,tegra30-hsuart";
>> +        status = "okay";
>> +    };
>
> It would be nice if the comment described the HW connectivity, i.e.
> which signals the UART was connected to on the board, just like the
> comments for other IO controllers already enabled in the DT file. I'd
> suggest replacing the comment above with:
>
> /* Expansion BR_UART1_RXD/_TXD */
>
>> +    /* Second high speed UART */
>> +    serial@0,70006040 {
>> +        compatible = "nvidia,tegra124-hsuart", "nvidia,tegra30-hsuart";
>> +        status = "okay";
>> +    };
>
> ... and that commetn with:
>
> /* Expansion UART2_RXD/_TXD/_RTS/_CTS */
>
>> +
>> +    /* Third high speed UART */
>> +    serial@0,70006200 {
>> +        compatible = "nvidia,tegra124-hsuart", "nvidia,tegra30-hsuart";
>> +        status = "okay";
>> +    };
>
> That UART doesn't seem to be used at all according to the schematics
> and pinmux spreadsheet. Do you have any reference to the contrary
> aside from the L4T DT file? I believe it shouldn't be enabled.
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
index 66b4451..0bd9be7 100644
--- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
+++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
@@ -12,7 +12,12 @@ 
 	aliases {
 		rtc0 = "/i2c@0,7000d000/pmic@40";
 		rtc1 = "/rtc@0,7000e000";
+
+		/* This order keeps the mapping DB9 connector <-> ttyS0 */
 		serial0 = &uartd;
+		serial1 = &uarta;
+		serial2 = &uartb;
+		serial3 = &uartc;
 	};
 
 	memory {
@@ -1367,6 +1372,24 @@ 
 		};
 	};
 
+	/* First high speed UART */
+	serial@0,70006000 {
+		compatible = "nvidia,tegra124-hsuart", "nvidia,tegra30-hsuart";
+		status = "okay";
+	};
+
+	/* Second high speed UART */
+	serial@0,70006040 {
+		compatible = "nvidia,tegra124-hsuart", "nvidia,tegra30-hsuart";
+		status = "okay";
+	};
+
+	/* Third high speed UART */
+	serial@0,70006200 {
+		compatible = "nvidia,tegra124-hsuart", "nvidia,tegra30-hsuart";
+		status = "okay";
+	};
+
 	/* DB9 serial port */
 	serial@0,70006300 {
 		status = "okay";