Re: [PATCH] ARM: dts: Add support for Liebherr's BK4 device (vf610 based)
On 21.09.2018 08:27, Lukasz Majewski wrote:
> This commit adds DTS support for BK4 device from Liebherr. It
> uses vf610 SoC from NXP.
>
> Signed-off-by: Lukasz Majewski
> ---
> arch/arm/boot/dts/Makefile | 1 +
> arch/arm/boot/dts/vf610-bk4.dts | 504
>
> 2 files changed, 505 insertions(+)
> create mode 100644 arch/arm/boot/dts/vf610-bk4.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index b5bd3de87c33..e6f159895fa9 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -577,6 +577,7 @@ dtb-$(CONFIG_SOC_LS1021A) += \
> ls1021a-twr.dtb
> dtb-$(CONFIG_SOC_VF610) += \
> vf500-colibri-eval-v3.dtb \
> + vf610-bk4.dtb \
> vf610-colibri-eval-v3.dtb \
> vf610m4-colibri.dtb \
> vf610-cosmic.dtb \
> diff --git a/arch/arm/boot/dts/vf610-bk4.dts b/arch/arm/boot/dts/vf610-bk4.dts
> new file mode 100644
> index ..4ad7e739a0ad
> --- /dev/null
> +++ b/arch/arm/boot/dts/vf610-bk4.dts
> @@ -0,0 +1,504 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright 2018
> + * Lukasz Majewski, DENX Software Engineering, lu...@denx.de
> + */
> +
> +/dts-v1/;
> +#include "vf610.dtsi"
> +
> +/ {
> + model = "Liebherr BK4 controller";
> + compatible = "lwn,bk4", "fsl,vf610";
> +
> + chosen {
> + bootargs = "console=ttyLP1,115200";
> + };
> +
> + memory@8000 {
> + reg = <0x8000 0x800>;
> + };
> +
> + audio_ext: mclk_osc {
Node name should be somewhat generic. I know its the same in twr board,
we probably should fix it there. The device tree spec has some
recommendation, I would suggest using just "oscillator-audio".
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <24576000>;
> + };
> +
> + enet_ext: eth_osc {
Same here, oscillator-ethernet maybe.
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <5000>;
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_gpio_leds>;
> +
> + /* LED D5 */
> + led0: heartbeat {
> + label = "heartbeat";
> + gpios = <&gpio3 21 GPIO_ACTIVE_HIGH>;
> + default-state = "on";
> + linux,default-trigger = "heartbeat";
> + };
> + };
> +
> + regulators {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <0>;
We stopped using a regulator subnode. Just move the regulators to the
top and use unique names such as "regulator-3p3v".
> +
> + reg_3p3v: regulator@0 {
> + compatible = "regulator-fixed";
> + reg = <0>;
> + regulator-name = "3P3V";
> + regulator-min-microvolt = <330>;
> + regulator-max-microvolt = <330>;
> + regulator-always-on;
> + };
> +
> + reg_vcc_3v3_mcu: regulator@1 {
> + compatible = "regulator-fixed";
> + reg = <1>;
> + regulator-name = "vcc_3v3_mcu";
> + regulator-min-microvolt = <330>;
> + regulator-max-microvolt = <330>;
> + };
> + };
> +};
> +
> +&adc0 {
> + vref-supply = <®_vcc_3v3_mcu>;
> + status = "okay";
> +};
> +
> +&adc1 {
> + vref-supply = <®_vcc_3v3_mcu>;
> + status = "okay";
> +};
> +
> +&can0 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_can0>;
> + status = "okay";
> +};
> +
> +&can1 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_can1>;
> + status = "okay";
> +};
> +
> +&clks {
> + clocks = <&sxosc>, <&fxosc>, <&enet_ext>, <&audio_ext>;
> + clock-names = "sxosc", "fxosc", "enet_ext", "audio_ext";
> +};
> +
> +&dspi0 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_dspi0>;
> + bus-num = <0>;
> + status = "okay";
> +
> + spidev0@0 {
> + compatible = "fsl,vf610-dspi";
I don't think this is right. A subnode of the dspi controller should use
a device tree binding of SPI device. What is connected to that SPI in
your case?
> + spi-max-frequency = <3000>;
> + reg = <0>;
> + fsl,spi-cs-sck-delay = <200>;
> + fsl,spi-sck-cs-delay = <400>;
> + };
> +};
> +
> +&dspi3 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_dspi3>;
> + bus-num = <3>;
> + status = "okay";
> +
> + spidev3@0 {
> + compatible = "fsl,vf610-dspi";
Same here...
> + spi-max-frequency = <3000>;
> + reg = <0>;
> + fsl,spi-slave-mode;
> +
Re: [PATCH] ARM: dts: Add support for Liebherr's BK4 device (vf610 based)
Hi Stefan,
Thanks for review,
> On 21.09.2018 08:27, Lukasz Majewski wrote:
> > This commit adds DTS support for BK4 device from Liebherr. It
> > uses vf610 SoC from NXP.
> >
> > Signed-off-by: Lukasz Majewski
> > ---
> > arch/arm/boot/dts/Makefile | 1 +
> > arch/arm/boot/dts/vf610-bk4.dts | 504
> > 2 files changed, 505
> > insertions(+) create mode 100644 arch/arm/boot/dts/vf610-bk4.dts
> >
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index b5bd3de87c33..e6f159895fa9 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -577,6 +577,7 @@ dtb-$(CONFIG_SOC_LS1021A) += \
> > ls1021a-twr.dtb
> > dtb-$(CONFIG_SOC_VF610) += \
> > vf500-colibri-eval-v3.dtb \
> > + vf610-bk4.dtb \
> > vf610-colibri-eval-v3.dtb \
> > vf610m4-colibri.dtb \
> > vf610-cosmic.dtb \
> > diff --git a/arch/arm/boot/dts/vf610-bk4.dts
> > b/arch/arm/boot/dts/vf610-bk4.dts new file mode 100644
> > index ..4ad7e739a0ad
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/vf610-bk4.dts
> > @@ -0,0 +1,504 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright 2018
> > + * Lukasz Majewski, DENX Software Engineering, lu...@denx.de
> > + */
> > +
> > +/dts-v1/;
> > +#include "vf610.dtsi"
> > +
> > +/ {
> > + model = "Liebherr BK4 controller";
> > + compatible = "lwn,bk4", "fsl,vf610";
> > +
> > + chosen {
> > + bootargs = "console=ttyLP1,115200";
> > + };
> > +
> > + memory@8000 {
> > + reg = <0x8000 0x800>;
> > + };
> > +
> > + audio_ext: mclk_osc {
>
> Node name should be somewhat generic. I know its the same in twr
> board, we probably should fix it there. The device tree spec has some
> recommendation, I would suggest using just "oscillator-audio".
So it shall be audio_ext: oscillator-audio { ...
>
> > + compatible = "fixed-clock";
> > + #clock-cells = <0>;
> > + clock-frequency = <24576000>;
> > + };
> > +
> > + enet_ext: eth_osc {
>
> Same here, oscillator-ethernet maybe.
Ok - enet_ext: oscillator-ethernet { ...
>
>
> > + compatible = "fixed-clock";
> > + #clock-cells = <0>;
> > + clock-frequency = <5000>;
> > + };
> > +
> > + leds {
> > + compatible = "gpio-leds";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_gpio_leds>;
> > +
> > + /* LED D5 */
> > + led0: heartbeat {
> > + label = "heartbeat";
> > + gpios = <&gpio3 21 GPIO_ACTIVE_HIGH>;
> > + default-state = "on";
> > + linux,default-trigger = "heartbeat";
> > + };
> > + };
> > +
> > + regulators {
> > + compatible = "simple-bus";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
>
> We stopped using a regulator subnode. Just move the regulators to the
> top and use unique names such as "regulator-3p3v".
Ok.
>
> > +
> > + reg_3p3v: regulator@0 {
> > + compatible = "regulator-fixed";
> > + reg = <0>;
> > + regulator-name = "3P3V";
> > + regulator-min-microvolt = <330>;
> > + regulator-max-microvolt = <330>;
> > + regulator-always-on;
> > + };
> > +
> > + reg_vcc_3v3_mcu: regulator@1 {
> > + compatible = "regulator-fixed";
> > + reg = <1>;
> > + regulator-name = "vcc_3v3_mcu";
> > + regulator-min-microvolt = <330>;
> > + regulator-max-microvolt = <330>;
> > + };
> > + };
> > +};
> > +
> > +&adc0 {
> > + vref-supply = <®_vcc_3v3_mcu>;
> > + status = "okay";
> > +};
> > +
> > +&adc1 {
> > + vref-supply = <®_vcc_3v3_mcu>;
> > + status = "okay";
> > +};
> > +
> > +&can0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_can0>;
> > + status = "okay";
> > +};
> > +
> > +&can1 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_can1>;
> > + status = "okay";
> > +};
> > +
> > +&clks {
> > + clocks = <&sxosc>, <&fxosc>, <&enet_ext>, <&audio_ext>;
> > + clock-names = "sxosc", "fxosc", "enet_ext", "audio_ext";
> > +};
> > +
> > +&dspi0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_dspi0>;
> > + bus-num = <0>;
> > + status = "okay";
> > +
> > + spidev0@0 {
> > + compatible = "fsl,vf610-dspi";
>
> I don't think this is right. A subnode of the dspi controller should
> use a device tree binding of SPI device. What is connected to that
> SPI in your case?
The dspi IP block provides SPI communication with variety of devices
(connected via external cable).
From SW point of view, the spidev driver is used. User space program(s)
via the /dev/spidevX.Y device communicate with connected
Re: [PATCH] ARM: dts: Add support for Liebherr's BK4 device (vf610 based)
On Fri, Sep 21, 2018 at 12:27 PM, Lukasz Majewski wrote:
> This commit adds DTS support for BK4 device from Liebherr. It
> uses vf610 SoC from NXP.
>
> Signed-off-by: Lukasz Majewski
> ---
> arch/arm/boot/dts/Makefile | 1 +
> arch/arm/boot/dts/vf610-bk4.dts | 504
>
> 2 files changed, 505 insertions(+)
> create mode 100644 arch/arm/boot/dts/vf610-bk4.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index b5bd3de87c33..e6f159895fa9 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -577,6 +577,7 @@ dtb-$(CONFIG_SOC_LS1021A) += \
> ls1021a-twr.dtb
> dtb-$(CONFIG_SOC_VF610) += \
> vf500-colibri-eval-v3.dtb \
> + vf610-bk4.dtb \
> vf610-colibri-eval-v3.dtb \
> vf610m4-colibri.dtb \
> vf610-cosmic.dtb \
> diff --git a/arch/arm/boot/dts/vf610-bk4.dts b/arch/arm/boot/dts/vf610-bk4.dts
> new file mode 100644
> index ..4ad7e739a0ad
> --- /dev/null
> +++ b/arch/arm/boot/dts/vf610-bk4.dts
> @@ -0,0 +1,504 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright 2018
> + * Lukasz Majewski, DENX Software Engineering, lu...@denx.de
> + */
> +
> +/dts-v1/;
> +#include "vf610.dtsi"
> +
> +/ {
> + model = "Liebherr BK4 controller";
> + compatible = "lwn,bk4", "fsl,vf610";
> +
> + chosen {
> + bootargs = "console=ttyLP1,115200";
You could pass stdout-path instead.
> + };
> +
> + memory@8000 {
> + reg = <0x8000 0x800>;
> + };
> +
> + audio_ext: mclk_osc {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <24576000>;
> + };
This seems to be unused.
> +
> + enet_ext: eth_osc {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <5000>;
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_gpio_leds>;
> +
> + /* LED D5 */
> + led0: heartbeat {
> + label = "heartbeat";
> + gpios = <&gpio3 21 GPIO_ACTIVE_HIGH>;
> + default-state = "on";
> + linux,default-trigger = "heartbeat";
> + };
> + };
> +
> + regulators {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + reg_3p3v: regulator@0 {
Please move all regulators outside of the simple-bus container and use
this naming convention:
reg_3p3v: regulator-3p3v {
> +&dspi3 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_dspi3>;
> + bus-num = <3>;
> + status = "okay";
> +
> + spidev3@0 {
> + compatible = "fsl,vf610-dspi";
> + spi-max-frequency = <3000>;
> + reg = <0>;
> + fsl,spi-slave-mode;
Such property does not exist.
I also thought that spidev nodes in dt were not recommended.
> +&iomuxc {
Like Stefan mentioned it is common practice on imx dts files to place
the iomuxc node as the last one.
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_bk4_common>;
This seems to be not called from any driver.
We usually use a hog group for such purpose.
> +
> + pinctrl_bk4_common: commongrp {
> + fsl,pins = <
> + /* One_Wire_PSU_EN */
> + VF610_PAD_PTC29__GPIO_102 0x1183
> + /* SPI */
> + VF610_PAD_PTB26__GPIO_960x1183
> + VF610_PAD_PTE14__GPIO_119 0x1183
> + VF610_PAD_PTE4__GPIO_1090x1181
> + /* Feedback_Lines */
> + VF610_PAD_PTC31__GPIO_104 0x1181
> + VF610_PAD_PTA7__GPIO_1340x1181
> + VF610_PAD_PTD9__GPIO_88 0x1181
> + VF610_PAD_PTE1__GPIO_1060x1183
> + VF610_PAD_PTB2__GPIO_24 0x1181
> + VF610_PAD_PTB3__GPIO_25 0x1181
> + VF610_PAD_PTB1__GPIO_23 0x1181
> + /* SDHC */
> + VF610_PAD_PTE19__GPIO_124 0x1183
> + VF610_PAD_PTB23__GPIO_930x1181
If they are related to SDHC they should be better placed under the sdhc nodes.
> + /* GPI */
> + VF610_PAD_PTE2__GPIO_1070x1181
> + VF610_PAD_PTE3__GPIO_1080x1181
> + VF610_PAD_PTE5__GPIO_110
Re: [PATCH] ARM: dts: Add support for Liebherr's BK4 device (vf610 based)
Hi Fabio,
Thanks for your review.
> On Fri, Sep 21, 2018 at 12:27 PM, Lukasz Majewski
> wrote:
> > This commit adds DTS support for BK4 device from Liebherr. It
> > uses vf610 SoC from NXP.
> >
> > Signed-off-by: Lukasz Majewski
> > ---
> > arch/arm/boot/dts/Makefile | 1 +
> > arch/arm/boot/dts/vf610-bk4.dts | 504
> > 2 files changed, 505
> > insertions(+) create mode 100644 arch/arm/boot/dts/vf610-bk4.dts
> >
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index b5bd3de87c33..e6f159895fa9 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -577,6 +577,7 @@ dtb-$(CONFIG_SOC_LS1021A) += \
> > ls1021a-twr.dtb
> > dtb-$(CONFIG_SOC_VF610) += \
> > vf500-colibri-eval-v3.dtb \
> > + vf610-bk4.dtb \
> > vf610-colibri-eval-v3.dtb \
> > vf610m4-colibri.dtb \
> > vf610-cosmic.dtb \
> > diff --git a/arch/arm/boot/dts/vf610-bk4.dts
> > b/arch/arm/boot/dts/vf610-bk4.dts new file mode 100644
> > index ..4ad7e739a0ad
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/vf610-bk4.dts
> > @@ -0,0 +1,504 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright 2018
> > + * Lukasz Majewski, DENX Software Engineering, lu...@denx.de
> > + */
> > +
> > +/dts-v1/;
> > +#include "vf610.dtsi"
> > +
> > +/ {
> > + model = "Liebherr BK4 controller";
> > + compatible = "lwn,bk4", "fsl,vf610";
> > +
> > + chosen {
> > + bootargs = "console=ttyLP1,115200";
>
> You could pass stdout-path instead.
Ok.
>
> > + };
> > +
> > + memory@8000 {
> > + reg = <0x8000 0x800>;
> > + };
> > +
> > + audio_ext: mclk_osc {
> > + compatible = "fixed-clock";
> > + #clock-cells = <0>;
> > + clock-frequency = <24576000>;
> > + };
>
> This seems to be unused.
The audio_ext label is used (referenced) in the "clks".
>
> > +
> > + enet_ext: eth_osc {
> > + compatible = "fixed-clock";
> > + #clock-cells = <0>;
> > + clock-frequency = <5000>;
> > + };
> > +
> > + leds {
> > + compatible = "gpio-leds";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_gpio_leds>;
> > +
> > + /* LED D5 */
> > + led0: heartbeat {
> > + label = "heartbeat";
> > + gpios = <&gpio3 21 GPIO_ACTIVE_HIGH>;
> > + default-state = "on";
> > + linux,default-trigger = "heartbeat";
> > + };
> > + };
> > +
> > + regulators {
> > + compatible = "simple-bus";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + reg_3p3v: regulator@0 {
>
> Please move all regulators outside of the simple-bus container and use
> this naming convention:
>
> reg_3p3v: regulator-3p3v {
Ok.
>
> > +&dspi3 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_dspi3>;
> > + bus-num = <3>;
> > + status = "okay";
> > +
> > + spidev3@0 {
> > + compatible = "fsl,vf610-dspi";
> > + spi-max-frequency = <3000>;
> > + reg = <0>;
> > + fsl,spi-slave-mode;
>
> Such property does not exist.
It has been added in the other patch sent to ML:
https://lkml.org/lkml/2018/9/18/792
But till now no comments/reply.
>
> I also thought that spidev nodes in dt were not recommended.
This is a bit "unusual" on BK4, as the spidev driver is the only "user"
of the SPI subsystem on this board. In other words - the /dev/spidevX.Y
provided by spidev is solely used for communication.
Hence, I would prefer to make it explicit in the DTS.
>
> > +&iomuxc {
>
> Like Stefan mentioned it is common practice on imx dts files to place
> the iomuxc node as the last one.
Ok.
>
>
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_bk4_common>;
>
> This seems to be not called from any driver.
Yes. This is "base" setup for the board. Those configured pins are
thereafter used by several user space programs.
>
> We usually use a hog group for such purpose.
I wanted to name it explicit that we do use it for this controller.
However, no problem to rename it to hog.
>
> > +
> > + pinctrl_bk4_common: commongrp {
> > + fsl,pins = <
> > + /* One_Wire_PSU_EN */
> > + VF610_PAD_PTC29__GPIO_102
> > 0x1183
> > + /* SPI */
> > + VF610_PAD_PTB26__GPIO_96
> > 0x1183
> > + VF610_PAD_PTE14__GPIO_119
> > 0x1183
> > + VF610_PAD_PTE4__GPIO_109
> > 0x1181
> > + /* Feedback_Lines */
> > + VF610_PAD
