Message ID | 1453209155-6213-3-git-send-email-ralf@ramses-pyramidenbau.de |
---|---|
State | Changes Requested |
Headers | show |
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
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
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
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
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
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 --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";
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(+)