Re: [PATCH] arm: tegra: initial support for apalis tk1

From: Marcel Ziswiler
Date: Thu May 19 2016 - 09:30:17 EST

On Mon, 2016-05-16 at 10:55 -0500, Rob Herring wrote:
> On Thu, May 12, 2016 at 02:27:12PM +0200, Marcel Ziswiler wrote:
> >
> > This patch adds the device tree to support Toradex Apalis TK1 a
> > computer on module which can be used on different carrier boards.
> >
> > The module consists of a Tegra TK1 SoC, a PMIC solution, 2 GB of
> > DDR3L
> > RAM, a bunch of level shifters, an eMMC, a TMP451 temperature
> > sensor
> > chip and an I210 gigabit Ethernet controller. Furthermore, there is
> > an
> > SGTL5000 audio codec plus a Kinetis MK20DN512 companion micro
> > controller for analogue, CAN and resistive touch functionality
> > which
> > are not yet supported. Anything that is not self contained on the
> > module is disabled by default.
> >
> > The device tree for the Evaluation Board includes the module's
> > device
> > tree and enables the supported peripherals of the carrier board
> > (the
> > Evaluation Board supports almost all of them).
> >
> > While at it also add the device tree binding documentation for
> > Apalis
> > TK1 as well as for Colibri T30 which was missing so far.
> "While at it" and "also" are keywords for put in a separate patch.

While I do agree in general in this case it is about missing device
tree documentation which has absolutely zero potential to break
bisectability or anything the like. Anyway, I will split it for a v2.

> > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>
> >
> > ---
> >
> > ÂDocumentation/devicetree/bindings/arm/tegra.txt |ÂÂÂÂ4 +
> > Âarch/arm/boot/dts/MakefileÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂÂÂ1 +
> > Âarch/arm/boot/dts/tegra124-apalis-emc.dtsiÂÂÂÂÂÂ| 2462
> > +++++++++++++++++++++++
> > Âarch/arm/boot/dts/tegra124-apalis-eval.dtsÂÂÂÂÂÂ|ÂÂ283 +++
> > Âarch/arm/boot/dts/tegra124-apalis.dtsiÂÂÂÂÂÂÂÂÂÂ| 2058
> > +++++++++++++++++++
> > Â5 files changed, 4808 insertions(+)
> > Âcreate mode 100644 arch/arm/boot/dts/tegra124-apalis-emc.dtsi
> > Âcreate mode 100644 arch/arm/boot/dts/tegra124-apalis-eval.dts
> > Âcreate mode 100644 arch/arm/boot/dts/tegra124-apalis.dtsi
> >
> > diff --git a/Documentation/devicetree/bindings/arm/tegra.txt
> > b/Documentation/devicetree/bindings/arm/tegra.txt
> > index 73278c6..7709f3d 100644
> > --- a/Documentation/devicetree/bindings/arm/tegra.txt
> > +++ b/Documentation/devicetree/bindings/arm/tegra.txt
> > @@ -31,8 +31,12 @@ board-specific compatible values:
> > ÂÂÂnvidia,ventana
> > ÂÂÂnvidia,whistler
> > ÂÂÂtoradex,apalis_t30
> > +ÂÂtoradex,apalis_tk1
> > ÂÂÂtoradex,apalis_t30-eval
> > +ÂÂtoradex,apalis_tk1-eval
> > ÂÂÂtoradex,colibri_t20-512
> > +ÂÂtoradex,colibri_t30
> > +ÂÂtoradex,colibri_t30-eval-v3
> > ÂÂÂtoradex,iris
> Please use '-' rather than '_' for new strings even if that's whatÂ
> previous strings have. For ones that already in use in upstream dtsÂ
> files, then keep the underscore.

OK, semantically so far we used '_' aka underscores as having a weaker
separation meaning (e.g. like a space) than '-' aka dashes which
separated the module from the carrier board part. We even once
discussed that the dash between eval and v3 for colibri_t30 should
actually rather be an underscore. I guess something like this is now no
more feasible or do you have any suggestions for us?

> > ÂTrusted Foundations
> > diff --git a/arch/arm/boot/dts/Makefile
> > b/arch/arm/boot/dts/Makefile
> > index 06b6c2d..3a13d4f 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -793,6 +793,7 @@ dtb-$(CONFIG_ARCH_TEGRA_114_SOC) += \
> > Â tegra114-roth.dtb \
> > Â tegra114-tn7.dtb
> > Âdtb-$(CONFIG_ARCH_TEGRA_124_SOC) += \
> > + tegra124-apalis-eval.dtb \
> > Â tegra124-jetson-tk1.dtb \
> > Â tegra124-nyan-big.dtb \
> > Â tegra124-nyan-blaze.dtb \
> > diff --git a/arch/arm/boot/dts/tegra124-apalis-emc.dtsi
> > b/arch/arm/boot/dts/tegra124-apalis-emc.dtsi
> > new file mode 100644
> > index 0000000..31b31ea
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/tegra124-apalis-emc.dtsi
> > @@ -0,0 +1,2462 @@
> > +/*
> > + * Copyright 2016 Toradex AG
> > + *
> > + * This file is dual-licensed: you can use it either under the
> > terms
> > + * of the GPL or the X11 license, at your option. Note that this
> > dual
> > + * licensing only applies to this file, and not this project as a
> > + * whole.
> > + *
> > + *ÂÂa) This file is free software; you can redistribute it and/or
> > + *ÂÂÂÂÂmodify it under the terms of the GNU General Public License
> > + *ÂÂÂÂÂversion 2 as published by the Free Software Foundation.
> > + *
> > + *ÂÂÂÂÂThis file is distributed in the hope that it will be useful
> > + *ÂÂÂÂÂbut WITHOUT ANY WARRANTY; without even the implied warranty
> > of
> > + *ÂÂÂÂÂMERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.ÂÂSee
> > the
> > + *ÂÂÂÂÂGNU General Public License for more details.
> > + *
> > + * Or, alternatively
> > + *
> > + *ÂÂb) Permission is hereby granted, free of charge, to any person
> > + *ÂÂÂÂÂobtaining a copy of this software and associated
> > documentation
> > + *ÂÂÂÂÂfiles (the "Software"), to deal in the Software without
> > + *ÂÂÂÂÂrestriction, including without limitation the rights to use
> > + *ÂÂÂÂÂcopy, modify, merge, publish, distribute, sublicense,
> > and/or
> > + *ÂÂÂÂÂsell copies of the Software, and to permit persons to whom
> > the
> > + *ÂÂÂÂÂSoftware is furnished to do so, subject to the following
> > + *ÂÂÂÂÂconditions:
> > + *
> > + *ÂÂÂÂÂThe above copyright notice and this permission notice shall
> > be
> > + *ÂÂÂÂÂincluded in all copies or substantial portions of the
> > Software.
> > + *
> > + *ÂÂÂÂÂTHE SOFTWARE IS PROVIDED , WITHOUT WARRANTY OF ANY KIND
> > + *ÂÂÂÂÂEXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
> > WARRANTIES
> > + *ÂÂÂÂÂOF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> > + *ÂÂÂÂÂNONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> > + *ÂÂÂÂÂHOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY
> > + *ÂÂÂÂÂWHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > + *ÂÂÂÂÂFROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
> > OR
> > + *ÂÂÂÂÂOTHER DEALINGS IN THE SOFTWARE.
> > + */
> > +
> > +/ {
> > + clock@0,60006000 {
> All these commas should be dropped. These have been fixed in -next
> andÂ
> will go into 4.7.

I guess you mean the zero followed by the comma, right? That only works
if tegra124.dtsi and with it all the other boards get migrated to this
as well. I can certainly send a patch for that as well if this is the
direction we want to go.

> > + emc-timings-1 {
> > + nvidia,ram-code = <1>;
> > +
> > + timing-12750000 {
> > + clock-frequency = <12750000>;
> > + nvidia,parent-clock-frequency =
> > <408000000>;
> > + clocks = <&tegra_car
> > TEGRA124_CLK_PLL_P>;
> > + clock-names = "emc-parent";
> > + };
> > + timing-20400000 {
> > + clock-frequency = <20400000>;
> > + nvidia,parent-clock-frequency =
> > <408000000>;
> > + clocks = <&tegra_car
> > TEGRA124_CLK_PLL_P>;
> > + clock-names = "emc-parent";
> > + };
> > + timing-40800000 {
> > + clock-frequency = <40800000>;
> > + nvidia,parent-clock-frequency =
> > <408000000>;
> > + clocks = <&tegra_car
> > TEGRA124_CLK_PLL_P>;
> > + clock-names = "emc-parent";
> > + };
> > + timing-68000000 {
> > + clock-frequency = <68000000>;
> > + nvidia,parent-clock-frequency =
> > <408000000>;
> > + clocks = <&tegra_car
> > TEGRA124_CLK_PLL_P>;
> > + clock-names = "emc-parent";
> > + };
> > + timing-102000000 {
> > + clock-frequency = <102000000>;
> > + nvidia,parent-clock-frequency =
> > <408000000>;
> > + clocks = <&tegra_car
> > TEGRA124_CLK_PLL_P>;
> > + clock-names = "emc-parent";
> > + };
> > + timing-204000000 {
> > + clock-frequency = <204000000>;
> > + nvidia,parent-clock-frequency =
> > <408000000>;
> > + clocks = <&tegra_car
> > TEGRA124_CLK_PLL_P>;
> > + clock-names = "emc-parent";
> > + };
> > + timing-300000000 {
> > + clock-frequency = <300000000>;
> > + nvidia,parent-clock-frequency =
> > <600000000>;
> > + clocks = <&tegra_car
> > TEGRA124_CLK_PLL_C>;
> > + clock-names = "emc-parent";
> > + };
> > + timing-396000000 {
> > + clock-frequency = <396000000>;
> > + nvidia,parent-clock-frequency =
> > <792000000>;
> > + clocks = <&tegra_car
> > TEGRA124_CLK_PLL_M>;
> > + clock-names = "emc-parent";
> > + };
> > + timing-528000000 {
> > + clock-frequency = <528000000>;
> > + nvidia,parent-clock-frequency =
> > <528000000>;
> > + clocks = <&tegra_car
> > TEGRA124_CLK_PLL_M_UD>;
> > + clock-names = "emc-parent";
> > + };
> > + timing-600000000 {
> > + clock-frequency = <600000000>;
> > + nvidia,parent-clock-frequency =
> > <600000000>;
> > + clocks = <&tegra_car
> > TEGRA124_CLK_PLL_C_UD>;
> > + clock-names = "emc-parent";
> > + };
> > + timing-792000000 {
> > + clock-frequency = <792000000>;
> > + nvidia,parent-clock-frequency =
> > <792000000>;
> > + clocks = <&tegra_car
> > TEGRA124_CLK_PLL_M_UD>;
> > + clock-names = "emc-parent";
> > + };
> > + timing-924000000 {
> > + clock-frequency = <924000000>;
> > + nvidia,parent-clock-frequency =
> > <924000000>;
> > + clocks = <&tegra_car
> > TEGRA124_CLK_PLL_M_UD>;
> > + clock-names = "emc-parent";
> > + };
> > + };
> > + };
> > +
> > + emc@0,7001b000 {
> > + emc-timings-1 {
> > + nvidia,ram-code = <1>;
> > +
> > + timing-12750000 {
> > + clock-frequency = <12750000>;
> > +
> > + nvidia,emc-auto-cal-config =
> > <0xa1430000>;
> > + nvidia,emc-auto-cal-config2 =
> > <0x00000000>;
> > + nvidia,emc-auto-cal-config3 =
> > <0x00000000>;
> > + nvidia,emc-auto-cal-interval =
> > <0x001fffff>;
> > + nvidia,emc-bgbias-ctl0 =
> > <0x00000008>;
> > + nvidia,emc-cfg = <0x73240000>;
> > + nvidia,emc-cfg-2 = <0x000008c5>;
> > + nvidia,emc-ctt-term-ctrl =
> > <0x00000802>;
> > + nvidia,emc-mode-1 = <0x80100003>;
> > + nvidia,emc-mode-2 = <0x80200008>;
> > + nvidia,emc-mode-4 = <0x00000000>;
> > + nvidia,emc-mode-reset =
> > <0x80001221>;
> > + nvidia,emc-mrs-wait-cnt =
> > <0x000e000e>;
> > + nvidia,emc-sel-dpd-ctrl =
> > <0x00040128>;
> > + nvidia,emc-xm2dqspadctrl2 =
> > <0x0130b118>;
> > + nvidia,emc-zcal-cnt-long =
> > <0x00000042>;
> > + nvidia,emc-zcal-interval =
> > <0x00000000>;
> > +
> > + nvidia,emc-configuration = <
> > + 0x00000000
> > + 0x00000003
> This is a bit long. Do multiple values per line.

OK. If keeping the 80 character limit one could get 3 values per line
which otherwise seems rather inconvenient. Should I just do two then or
allow for longer lines and putting 4 or even 8?

> > + 0x00000000
> > + 0x00000000
> > + 0x00000000
> > + 0x00000004
> > + 0x0000000a
> > + 0x00000005

Thanks, Rob.