2015-05-21 17:46:08

by Peter Seiderer

[permalink] [raw]
Subject: [PATCH v2] ARM: dts: nitrogen6x: add CAN support

Regulator stuff copied from imx6qdl-tx6.dtsi, pin configuration
taken from Boundary Devices linux kernel tree ([1] and [2]).

[1] https://github.com/boundarydevices/linux-imx6/blob/boundary-imx_3.10.17_1.0.2_ga/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi
[2] https://github.com/boundarydevices/linux-imx6/blob/boundary-imx_3.10.17_1.0.2_ga/arch/arm/boot/dts/imx6qdl.dtsi

Signed-off-by: Peter Seiderer <[email protected]>
---
v2:
- fix imx6qdl-nitrogen6x.dtsi url
- use real PAD settings (suggested by Fabio Estevam)
- remove _1 suffix (suggested by Shawn Guo)
---
arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi | 32 +++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi b/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi
index fd096dc..d40092e 100644
--- a/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi
@@ -54,6 +54,17 @@
gpio = <&gpio3 22 0>;
enable-active-high;
};
+
+ reg_can_xcvr: regulator@3 {
+ compatible = "regulator-fixed";
+ reg = <3>;
+ regulator-name = "CAN XCVR";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_can_xcvr>;
+ gpio = <&gpio1 2 GPIO_ACTIVE_HIGH>;
+ };
};

gpio-keys {
@@ -138,6 +149,13 @@
status = "okay";
};

+&can1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_can1>;
+ xceiver-supply = <&reg_can_xcvr>;
+ status = "okay";
+};
+
&ecspi1 {
fsl,spi-num-chipselects = <1>;
cs-gpios = <&gpio3 19 0>;
@@ -234,6 +252,20 @@
>;
};

+ pinctrl_can1: can1grp {
+ fsl,pins = <
+ MX6QDL_PAD_KEY_COL2__FLEXCAN1_TX 0x1b0b0
+ MX6QDL_PAD_KEY_ROW2__FLEXCAN1_RX 0x1b0b0
+ >;
+ };
+
+ pinctrl_can_xcvr: can-xcvrgrp {
+ fsl,pins = <
+ /* Flexcan XCVR enable */
+ MX6QDL_PAD_GPIO_2__GPIO1_IO02 0x1b0b0
+ >;
+ };
+
pinctrl_ecspi1: ecspi1grp {
fsl,pins = <
MX6QDL_PAD_EIM_D17__ECSPI1_MISO 0x100b1
--
2.1.4


2015-05-22 11:05:41

by Philipp Zabel

[permalink] [raw]
Subject: Re: [PATCH v2] ARM: dts: nitrogen6x: add CAN support

Hi Peter,

Am Donnerstag, den 21.05.2015, 19:45 +0200 schrieb Peter Seiderer:
> Regulator stuff copied from imx6qdl-tx6.dtsi, pin configuration
> taken from Boundary Devices linux kernel tree ([1] and [2]).
>
> [1] https://github.com/boundarydevices/linux-imx6/blob/boundary-imx_3.10.17_1.0.2_ga/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi
> [2] https://github.com/boundarydevices/linux-imx6/blob/boundary-imx_3.10.17_1.0.2_ga/arch/arm/boot/dts/imx6qdl.dtsi
>
> Signed-off-by: Peter Seiderer <[email protected]>
> ---
> v2:
> - fix imx6qdl-nitrogen6x.dtsi url
> - use real PAD settings (suggested by Fabio Estevam)
> - remove _1 suffix (suggested by Shawn Guo)
> ---
> arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi | 32 +++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi b/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi
> index fd096dc..d40092e 100644
> --- a/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi
> @@ -54,6 +54,17 @@
> gpio = <&gpio3 22 0>;
> enable-active-high;
> };
> +
> + reg_can_xcvr: regulator@3 {
> + compatible = "regulator-fixed";
> + reg = <3>;
> + regulator-name = "CAN XCVR";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_can_xcvr>;
> + gpio = <&gpio1 2 GPIO_ACTIVE_HIGH>;

According to
Documentation/devicetree/bindings/regulator/fixed-regulator.txt
this should have:
enable-active-high;

instead of the gpio phandle flag (which is ignored). Otherwise an active
low GPIO is assumed.

regards
Philipp

2015-05-22 19:30:37

by Peter Seiderer

[permalink] [raw]
Subject: Aw: Re: [PATCH v2] ARM: dts: nitrogen6x: add CAN support

Hello Philipp,

> Gesendet: Freitag, 22. Mai 2015 um 13:05 Uhr
> Von: "Philipp Zabel" <[email protected]>
> An: "Peter Seiderer" <[email protected]>
> Cc: [email protected], "Mark Rutland" <[email protected]>, [email protected], "Russell King" <[email protected]>, "Pawel Moll" <[email protected]>, "Ian Campbell" <[email protected]>, "Rob Herring" <[email protected]>, "Sascha Hauer" <[email protected]>, "Kumar Gala" <[email protected]>, "Shawn Guo" <[email protected]>, [email protected]
> Betreff: Re: [PATCH v2] ARM: dts: nitrogen6x: add CAN support
>
> Hi Peter,
>
> Am Donnerstag, den 21.05.2015, 19:45 +0200 schrieb Peter Seiderer:
> > Regulator stuff copied from imx6qdl-tx6.dtsi, pin configuration
> > taken from Boundary Devices linux kernel tree ([1] and [2]).
> >
> > [1] https://github.com/boundarydevices/linux-imx6/blob/boundary-imx_3.10.17_1.0.2_ga/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi
> > [2] https://github.com/boundarydevices/linux-imx6/blob/boundary-imx_3.10.17_1.0.2_ga/arch/arm/boot/dts/imx6qdl.dtsi
> >
> > Signed-off-by: Peter Seiderer <[email protected]>
> > ---
> > v2:
> > - fix imx6qdl-nitrogen6x.dtsi url
> > - use real PAD settings (suggested by Fabio Estevam)
> > - remove _1 suffix (suggested by Shawn Guo)
> > ---
> > arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi | 32 +++++++++++++++++++++++++++++++
> > 1 file changed, 32 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi b/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi
> > index fd096dc..d40092e 100644
> > --- a/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi
> > +++ b/arch/arm/boot/dts/imx6qdl-nitrogen6x.dtsi
> > @@ -54,6 +54,17 @@
> > gpio = <&gpio3 22 0>;
> > enable-active-high;
> > };
> > +
> > + reg_can_xcvr: regulator@3 {
> > + compatible = "regulator-fixed";
> > + reg = <3>;
> > + regulator-name = "CAN XCVR";
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_can_xcvr>;
> > + gpio = <&gpio1 2 GPIO_ACTIVE_HIGH>;
>
> According to
> Documentation/devicetree/bindings/regulator/fixed-regulator.txt
> this should have:
> enable-active-high;
>
> instead of the gpio phandle flag (which is ignored). Otherwise an active
> low GPIO is assumed.
>

Thanks for review...

I was a bit confused from the original:

imx6qdl-tx6.dtsi:103: reg_can_xcvr: regulator@3 {
imx6qdl-tx6.dtsi-104- compatible = "regulator-fixed";
imx6qdl-tx6.dtsi-105- reg = <3>;
imx6qdl-tx6.dtsi-106- regulator-name = "CAN XCVR";
imx6qdl-tx6.dtsi-107- regulator-min-microvolt = <3300000>;
imx6qdl-tx6.dtsi-108- regulator-max-microvolt = <3300000>;
imx6qdl-tx6.dtsi-109- pinctrl-names = "default";
imx6qdl-tx6.dtsi:110: pinctrl-0 = <&pinctrl_flexcan_xcvr>;
imx6qdl-tx6.dtsi-111- gpio = <&gpio4 21 GPIO_ACTIVE_HIGH>;
imx6qdl-tx6.dtsi-112- enable-active-low;
imx6qdl-tx6.dtsi-113- };

...and removed the default 'enable-active-low'...

Maybe GPIO_ACTIVE_LOW is the right thing?

>From the other files:

imx28-tx28.dts:90: reg_can_xcvr: regulator@4 {
imx28-tx28.dts-91- compatible = "regulator-fixed";
imx28-tx28.dts-92- reg = <4>;
imx28-tx28.dts-93- regulator-name = "CAN XCVR";
imx28-tx28.dts-94- regulator-min-microvolt = <3300000>;
imx28-tx28.dts-95- regulator-max-microvolt = <3300000>;
imx28-tx28.dts-96- gpio = <&gpio1 0 GPIO_ACTIVE_HIGH>;
imx28-tx28.dts-97- pinctrl-names = "default";
imx28-tx28.dts:98: pinctrl-0 = <&tx28_flexcan_xcvr_pins>;
imx28-tx28.dts-99- };


imx53-tx53.dtsi:90: reg_can_xcvr: regulator@2 {
imx53-tx53.dtsi-91- compatible = "regulator-fixed";
imx53-tx53.dtsi-92- reg = <2>;
imx53-tx53.dtsi-93- regulator-name = "CAN XCVR";
imx53-tx53.dtsi-94- regulator-min-microvolt = <3300000>;
imx53-tx53.dtsi-95- regulator-max-microvolt = <3300000>;
imx53-tx53.dtsi-96- pinctrl-names = "default";
imx53-tx53.dtsi:97: pinctrl-0 = <&pinctrl_can_xcvr>;
imx53-tx53.dtsi-98- gpio = <&gpio4 21 GPIO_ACTIVE_HIGH>;
imx53-tx53.dtsi-99- };

But there are also imx regulator-fixed definitions with GPIO_ACTIVE_HIGH/enable-active-high:

imx28-tx28.dts-101- reg_lcd: regulator@5 {
imx28-tx28.dts-102- compatible = "regulator-fixed";
imx28-tx28.dts-103- reg = <5>;
imx28-tx28.dts-104- regulator-name = "LCD POWER";
imx28-tx28.dts-105- regulator-min-microvolt = <3300000>;
imx28-tx28.dts-106- regulator-max-microvolt = <3300000>;
imx28-tx28.dts-107- gpio = <&gpio1 31 GPIO_ACTIVE_HIGH>;
imx28-tx28.dts-108- enable-active-high;
imx28-tx28.dts-109- };


imx53-tx53.dtsi-101- reg_usbh1_vbus: regulator@3 {
imx53-tx53.dtsi-102- compatible = "regulator-fixed";
imx53-tx53.dtsi-103- reg = <3>;
imx53-tx53.dtsi-104- regulator-name = "usbh1_vbus";
imx53-tx53.dtsi-105- regulator-min-microvolt = <5000000>;
imx53-tx53.dtsi-106- regulator-max-microvolt = <5000000>;
imx53-tx53.dtsi-107- pinctrl-names = "default";
imx53-tx53.dtsi-108- pinctrl-0 = <&pinctrl_usbh1_vbus>;
imx53-tx53.dtsi-109- gpio = <&gpio3 31 GPIO_ACTIVE_HIGH>;
imx53-tx53.dtsi-110- enable-active-high;
imx53-tx53.dtsi-111- };


imx6qdl-tx6.dtsi-115- reg_lcd0_pwr: regulator@4 {
imx6qdl-tx6.dtsi-116- compatible = "regulator-fixed";
imx6qdl-tx6.dtsi-117- reg = <4>;
imx6qdl-tx6.dtsi-118- regulator-name = "LCD0 POWER";
imx6qdl-tx6.dtsi-119- regulator-min-microvolt = <3300000>;
imx6qdl-tx6.dtsi-120- regulator-max-microvolt = <3300000>;
imx6qdl-tx6.dtsi-121- pinctrl-names = "default";
imx6qdl-tx6.dtsi-122- pinctrl-0 = <&pinctrl_lcd0_pwr>;
imx6qdl-tx6.dtsi-123- gpio = <&gpio3 29 GPIO_ACTIVE_HIGH>;
imx6qdl-tx6.dtsi-124- enable-active-high;
imx6qdl-tx6.dtsi-125- regulator-boot-on;
imx6qdl-tx6.dtsi-126- regulator-always-on;
imx6qdl-tx6.dtsi-127- };

Any further advice from your side which solution is the right one?

- GPIO_ACTIVE_HIGH/enable-active-high
- GPIO_ACTIVE_LOW

Regards,
Peter

> regards
> Philipp
>
>

2015-05-23 00:44:09

by Eric Nelson

[permalink] [raw]
Subject: Aw: [PATCH v2] ARM: dts: nitrogen6x: add CAN support

Hello Peter,

On 05/22/2015 12:30 PM, Peter Seiderer wrote:
> Hello Philipp,
>
>> Gesendet: Freitag, 22. Mai 2015 um 13:05 Uhr
>> Von: "Philipp Zabel" <[email protected]>
>> Am Donnerstag, den 21.05.2015, 19:45 +0200 schrieb Peter Seiderer:
>>>
>>> <snip>
>>>
>>> +
>>> + reg_can_xcvr: regulator@3 {
>>> + compatible = "regulator-fixed";
>>> + reg = <3>;
>>> + regulator-name = "CAN XCVR";
>>> + regulator-min-microvolt = <3300000>;
>>> + regulator-max-microvolt = <3300000>;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&pinctrl_can_xcvr>;
>>> + gpio = <&gpio1 2 GPIO_ACTIVE_HIGH>;
>>
>> According to
>> Documentation/devicetree/bindings/regulator/fixed-regulator.txt
>> this should have:
>> enable-active-high;
>>
>> instead of the gpio phandle flag (which is ignored). Otherwise an active
>> low GPIO is assumed.
>>
>
> Thanks for review...
>
> I was a bit confused from the original:
>
> imx6qdl-tx6.dtsi:103: reg_can_xcvr: regulator@3 {
> imx6qdl-tx6.dtsi-104- compatible = "regulator-fixed";
> imx6qdl-tx6.dtsi-105- reg = <3>;
> imx6qdl-tx6.dtsi-106- regulator-name = "CAN XCVR";
> imx6qdl-tx6.dtsi-107- regulator-min-microvolt = <3300000>;
> imx6qdl-tx6.dtsi-108- regulator-max-microvolt = <3300000>;
> imx6qdl-tx6.dtsi-109- pinctrl-names = "default";
> imx6qdl-tx6.dtsi:110: pinctrl-0 = <&pinctrl_flexcan_xcvr>;
> imx6qdl-tx6.dtsi-111- gpio = <&gpio4 21 GPIO_ACTIVE_HIGH>;
> imx6qdl-tx6.dtsi-112- enable-active-low;
> imx6qdl-tx6.dtsi-113- };
>
> ...and removed the default 'enable-active-low'...
>
> Maybe GPIO_ACTIVE_LOW is the right thing?
>

No. The flags aren't read from the device tree and enable-active-low
is the default.
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/regulator/fixed.c#n81

> From the other files:
>

So if this board really wants an "active high" enable pin, it's likely
not operating properly:

> imx28-tx28.dts:90: reg_can_xcvr: regulator@4 {
> imx28-tx28.dts-91- compatible = "regulator-fixed";
> imx28-tx28.dts-92- reg = <4>;
> imx28-tx28.dts-93- regulator-name = "CAN XCVR";
> imx28-tx28.dts-94- regulator-min-microvolt = <3300000>;
> imx28-tx28.dts-95- regulator-max-microvolt = <3300000>;
> imx28-tx28.dts-96- gpio = <&gpio1 0 GPIO_ACTIVE_HIGH>;
> imx28-tx28.dts-97- pinctrl-names = "default";
> imx28-tx28.dts:98: pinctrl-0 = <&tx28_flexcan_xcvr_pins>;
> imx28-tx28.dts-99- };
>
> <snip>
>
> Any further advice from your side which solution is the right one?
>
> - GPIO_ACTIVE_HIGH/enable-active-high
> - GPIO_ACTIVE_LOW
>

The pad is active low on the TJA1040 transceiver on the Nitrogen6x,
so you don't want "enable-active-high" and could be more explicit with
GPIO_ACTIVE_LOW in the gpio reference, but it won't be parsed or
acted upon.

i.e.
reg_can_xcvr: regulator@3 {
compatible = "regulator-fixed";
reg = <3>;
regulator-name = "CAN XCVR";
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3300000>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_can_xcvr>;
gpio = <&gpio1 2 0>;
}

Regards,


Eric