Message-ID: <ccca2b5c-8904-cc86-0914-2a0d1aa52305@puri.sm>
Date:   Tue, 2 Jun 2020 17:44:11 +0200
From:   Martin Kepplinger <martin.kepplinger@...i.sm>
To:     Marco Felsch <m.felsch@...gutronix.de>
Cc:     robh@...nel.org, kernel@...i.sm, shawnguo@...nel.org,
        s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com,
        linux-imx@....com, mchehab@...nel.org, Anson.Huang@....com,
        agx@...xcpu.org, angus@...ea.ca, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/2] arm64: dts: Add a device tree for the Librem5 phone

Hi Marco,

On 27.05.20 11:35, Marco Felsch wrote:
> Hi Martin,
> 
> On 20-05-14 17:57, Martin Kepplinger wrote:
>> From: "Angus Ainslie (Purism)" <angus@...ea.ca>
>>
>> Add a devicetree description for the Librem 5 phone. The early batches
>> that have been sold are supported as well as the mass-produced device
>> available later this year, see https://puri.sm/products/librem-5/
>>
>> This boots to a working console with working WWAN modem, wifi usdhc,
>> IMU sensor device, proximity sensor, haptic motor, gpio keys, GNSS and LEDs.
>>
>> Signed-off-by: Martin Kepplinger <martin.kepplinger@...i.sm>
>> Signed-off-by: Angus Ainslie (Purism) <angus@...ea.ca>
>> Signed-off-by: Guido Günther <agx@...xcpu.org>
>> ---
>>  arch/arm64/boot/dts/freescale/Makefile        |    1 +
>>  .../boot/dts/freescale/imx8mq-librem5.dts     | 1174 +++++++++++++++++
>>  2 files changed, 1175 insertions(+)
>>  create mode 100644 arch/arm64/boot/dts/freescale/imx8mq-librem5.dts
>>
>> diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
>> index cd38d04da5a7..342579121f98 100644
>> --- a/arch/arm64/boot/dts/freescale/Makefile
>> +++ b/arch/arm64/boot/dts/freescale/Makefile
>> @@ -34,6 +34,7 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mn-ddr4-evk.dtb
>>  dtb-$(CONFIG_ARCH_MXC) += imx8mp-evk.dtb
>>  dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb
>>  dtb-$(CONFIG_ARCH_MXC) += imx8mq-hummingboard-pulse.dtb
>> +dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5.dtb
>>  dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5-devkit.dtb
>>  dtb-$(CONFIG_ARCH_MXC) += imx8mq-nitrogen.dtb
>>  dtb-$(CONFIG_ARCH_MXC) += imx8mq-phanbell.dtb
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5.dts b/arch/arm64/boot/dts/freescale/imx8mq-librem5.dts
>> new file mode 100644
>> index 000000000000..95c105b4c120
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5.dts
>> @@ -0,0 +1,1174 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright 2018-2020 Purism SPC
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "dt-bindings/input/input.h"
>> +#include "dt-bindings/pwm/pwm.h"
>> +#include "dt-bindings/usb/pd.h"
>> +#include "imx8mq.dtsi"
>> +
>> +/ {
>> +	model = "Purism Librem 5";
>> +	compatible = "purism,librem5", "fsl,imx8mq";
>> +
>> +	backlight_dsi: backlight-dsi {
>> +		compatible = "led-backlight";
>> +		leds = <&led_backlight>;
>> +		brightness-levels = <255>;
>> +		default-brightness-level = <100>;
>> +	};
>> +
>> +	bm818_codec: sound-wwan-codec {
>> +		compatible = "broadmobi,bm818", "option,gtm601";
>> +		#sound-dai-cells = <0>;
>> +	};
> 
> Please sort the node names alpabetical.
> 
>> +
>> +	chosen {
>> +		stdout-path = &uart1;
>> +	};
>> +
>> +	gpio-keys {
>> +		compatible = "gpio-keys";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_keys>, <&pinctrl_hp>;
>> +
>> +		hp-det {
>> +			label = "HP_DET";
>> +			gpios = <&gpio3 9 GPIO_ACTIVE_HIGH>;
>> +			wakeup-source;
>> +			linux,code = <KEY_HP>;
> 
> Nit: I would add the wakeup-source behind the linux,code.
> 
>> +		};
>> +
>> +		vol-down {
>> +			label = "VOL_DOWN";
>> +			gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>> +			linux,code = <KEY_VOLUMEDOWN>;
>> +		};
>> +
>> +		vol-up {
>> +			label = "VOL_UP";
>> +			gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
>> +			linux,code = <KEY_VOLUMEUP>;
>> +		};
>> +	};
>> +
>> +	pwmleds {
>> +		compatible = "pwm-leds";
>> +
>> +		blue {
>> +			label = "phone:blue:front";
>> +			max-brightness = <248>;
>> +			pwms = <&pwm2 0 50000>;
>> +		};
>> +
>> +		green {
>> +			label = "phone:green:front";
>> +			max-brightness = <248>;
>> +			pwms = <&pwm4 0 50000>;
>> +		};
>> +
>> +		red {
>> +			label = "phone:red:front";
>> +			max-brightness = <248>;
>> +			pwms = <&pwm3 0 50000>;
>> +		};
>> +	};
>> +
>> +	pmic_osc: clock-pmic {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency = <32768>;
>> +		clock-output-names = "pmic_osc";
>> +	};
> 
> Please sort nodes alphabetical.
> 
>> +
>> +	reg_audio_pwr_en: regulator-audio-pwr-en {
>> +		compatible = "regulator-fixed";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_audiopwr>;
>> +		regulator-name = "AUDIO_PWR_EN";
>> +		regulator-min-microvolt = <1800000>;
>> +		regulator-max-microvolt = <1800000>;
>> +		gpio = <&gpio1 4 GPIO_ACTIVE_HIGH>;
>> +		enable-active-high;
>> +		regulator-always-on;
> 
> Why should this regulator be always on? The wm8962.c driver can handle
> the regualtor enable/disable.

It can indeed be handled by the driver here. We keep it always on
because of issues with the display stack that are not yet part of this
description.

> 
>> +	};
>> +
>> +	reg_aud_1v8: regulator-audio-v1v8 {
> 				^
> 		     regulator-audio-1v8?
> 
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "aud_1v8";
> 
> Is it intended to use capitalized and no-capitalized regulator-name's?
> 
>> +		regulator-min-microvolt = <1800000>;
>> +		regulator-max-microvolt = <1800000>;
>> +		vin-supply = <&reg_audio_pwr_en>;
>> +	};
> 
> Can we squash regulator-audio-pwr-en and regulator-audio-v1v8?

I think we can.

> 
>> +
>> +	reg_gnss: regulator-gnss {
>> +		compatible = "regulator-fixed";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_gnsspwr>;
>> +		regulator-name = "GNSS";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +		gpio = <&gpio3 12 GPIO_ACTIVE_HIGH>;
>> +		enable-active-high;
>> +	};
>> +
>> +	reg_hub: regulator-hub {
>> +		compatible = "regulator-fixed";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_hub_pwr>;
>> +		regulator-name = "HUB";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +		gpio = <&gpio1 14 GPIO_ACTIVE_HIGH>;
>> +		enable-active-high;
>> +	};
>> +
>> +	reg_lcd_1v8: regulator-lcd-1v8 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "lcd_1v8";
>> +		regulator-min-microvolt = <1800000>;
>> +		regulator-max-microvolt = <1800000>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_dsien>;
>> +		vin-supply = <&reg_vdd_1v8>;
>> +		enable-active-high;
>> +		gpio = <&gpio1 5 GPIO_ACTIVE_HIGH>;
>> +	};
> 
> This regulator is never used.
> 
>> +
>> +	reg_lcd_3v4: regulator-lcd-3v4 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "lcd_3v4";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_dsibiasen>;
>> +		vin-supply = <&reg_vsys_3v4>;
>> +		enable-active-high;
>> +		gpio = <&gpio1 20 GPIO_ACTIVE_HIGH>;
>> +	};
>> +
>> +	reg_vdd_sen: regulator-vdd-sen {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vdd_sen";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +	};
>> +
>> +	reg_vdd_3v3: regulator-vdd-3v3 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vdd_3v3";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +	};
>> +
>> +	reg_vdd_1v8: regulator-vdd-1v8 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vdd_1v8";
>> +		regulator-min-microvolt = <1800000>;
>> +		regulator-max-microvolt = <1800000>;
>> +	};
>> +
>> +	reg_vsys_3v4: regulator-vsys-3v4 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vsys_3v4";
>> +		regulator-min-microvolt = <3400000>;
>> +		regulator-max-microvolt = <3400000>;
>> +		regulator-always-on;
>> +	};
>> +
>> +	reg_3v3_wifi: regulator-3v3-wifi {
> 			^
> 	reg_wifi_3v3: regulator-wifi-3v3?
> 
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "3v3_wifi";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +	};
>> +
>> +	sound {
>> +		compatible = "simple-audio-card";
>> +		simple-audio-card,name = "wm8962";
>> +		simple-audio-card,format = "i2s";
>> +		simple-audio-card,widgets =
>> +			"Headphone", "Headphone",
>> +			"Microphone", "Headset Mic",
>> +			"Microphone", "Digital Mic",
>> +			"Speaker", "Speaker";
>> +		simple-audio-card,routing =
>> +			"Headphone", "HPOUTL",
>> +			"Headphone", "HPOUTR",
>> +			"Speaker", "SPKOUTL",
>> +			"Speaker", "SPKOUTR",
>> +			"Headset Mic", "MICBIAS",
>> +			"IN3R", "Headset Mic",
>> +			"DMICDAT", "Digital Mic";
>> +		simple-audio-card,cpu {
>> +			sound-dai = <&sai2>;
>> +		};
>> +		simple-audio-card,codec {
>> +			sound-dai = <&codec>;
>> +			clocks = <&clk IMX8MQ_CLK_SAI2_ROOT>;
>> +			frame-master;
>> +			bitclock-master;
>> +		};
>> +	};
>> +
>> +	sound-wwan {
>> +		compatible = "simple-audio-card";
>> +		simple-audio-card,name = "MODEM";
>> +		simple-audio-card,format = "i2s";
>> +
>> +		simple-audio-card,cpu {
>> +			sound-dai = <&sai6>;
>> +			frame-inversion;
>> +		};
>> +
>> +		telephony_link_master: simple-audio-card,codec {
> 			^
> 		useless phandle?
>> +			sound-dai = <&bm818_codec>;
>> +			frame-master;
>> +			bitclock-master;
>> +		};
>> +	};
>> +
>> +	vibrator {
>> +		compatible = "pwm-vibrator";
>> +		pwms = <&pwm1 0 1000000000 0>;
>> +		pwm-names = "enable";
>> +		vcc-supply = <&reg_vdd_3v3>;
>> +	};
>> +};
>> +
>> +&A53_0 {
>> +	cpu-supply = <&buck2_reg>;
>> +};
>> +
>> +&A53_1 {
>> +	cpu-supply = <&buck2_reg>;
>> +};
>> +
>> +&A53_2 {
>> +	cpu-supply = <&buck2_reg>;
>> +};
>> +
>> +&A53_3 {
>> +	cpu-supply = <&buck2_reg>;
>> +};
>> +
>> +&clk {
>> +	assigned-clocks = <&clk IMX8MQ_AUDIO_PLL1>, <&clk IMX8MQ_AUDIO_PLL2>;
>> +	assigned-clock-rates = <786432000>, <722534400>;
>> +};
> 
> Either I would bundle all clock settings here or within the sai nodes.

You're right, I'll try to move them.

> 
>> +
>> +&ddrc {
>> +	operating-points-v2 = <&ddrc_opp_table>;
>> +
>> +	ddrc_opp_table: ddrc-opp-table {
>> +		compatible = "operating-points-v2";
>> +
>> +		opp-25M {
>> +			opp-hz = /bits/ 64 <25000000>;
>> +		};
>> +		opp-100M {
>> +			opp-hz = /bits/ 64 <100000000>;
>> +		};
>> +		opp-800M {
>> +			opp-hz = /bits/ 64 <800000000>;
>> +		};
>> +	};
>> +};
>> +
>> +&dphy {
>> +	status = "okay";
>> +};
>> +
>> +&ecspi1 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_ecspi1>;
>> +	cs-gpios = <&gpio5 9 GPIO_ACTIVE_HIGH>;
> 
> Missmatch with the pinctrl_ecspi1?
> 
>> +	status = "okay";
> 
> Status is always the last property.
> 
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +
>> +	nor_flash: flash@0 {
>> +		compatible = "jedec,spi-nor";
>> +		spi-max-frequency = <1000000>;
>> +		reg = <0>;
>> +	};
>> +};
>> +
>> +&gpio1 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_pmic_5v>;
>> +
>> +	pmic-5v {
>> +		gpio-hog;
>> +		gpio = <&gpio1 1 GPIO_ACTIVE_HIGH>;
>> +		input;
>> +	};
>> +};
>> +
>> +&iomuxc {
>> +	pinctrl_audiopwr: audiopwrgrp {
>> +		fsl,pins = <
>> +			/* AUDIO_POWER_EN_3V3 */
>> +			MX8MQ_IOMUXC_GPIO1_IO04_GPIO1_IO4	0x83
>> +		>;
>> +	};
>> +
>> +	pinctrl_bl: blgrp {
>> +		fsl,pins = <
>> +			/* BACKLINGE_EN */
>> +			MX8MQ_IOMUXC_NAND_DQS_GPIO3_IO14	0x83
>> +		>;
>> +	};
>> +
>> +	pinctrl_charger_in: chargeringrp {
>> +		fsl,pins = <
>> +			/* CHRG_INT */
>> +			MX8MQ_IOMUXC_NAND_CE2_B_GPIO3_IO3	0x80
>> +			/* CHG_STATUS_B */
>> +			MX8MQ_IOMUXC_NAND_ALE_GPIO3_IO0		0x80
>> +		>;
>> +	};
>> +
>> +	pinctrl_dsibiasen: dsibiasengrp {
>> +		fsl,pins = <
>> +			/* DSI_BIAS_EN */
>> +			MX8MQ_IOMUXC_ENET_TD1_GPIO1_IO20	0x83
>> +		>;
>> +	};
>> +
>> +	pinctrl_dsien: dsiengrp {
>> +		fsl,pins = <
>> +			/* DSI_EN_3V3 */
>> +			MX8MQ_IOMUXC_GPIO1_IO05_GPIO1_IO5	0x83
>> +		>;
>> +	};
>> +
>> +	pinctrl_ecspi1: spi1grp {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_ECSPI1_MOSI_ECSPI1_MOSI	0x83
>> +			MX8MQ_IOMUXC_ECSPI1_MISO_ECSPI1_MISO	0x83
>> +			MX8MQ_IOMUXC_ECSPI1_SS0_GPIO5_IO9	0x19
>> +			MX8MQ_IOMUXC_ECSPI1_SCLK_ECSPI1_SCLK	0x83
>> +			/* SPI_SS1 */
>> +			MX8MQ_IOMUXC_UART4_RXD_GPIO5_IO28	0x19
>> +		>;
>> +	};
>> +
>> +	pinctrl_gauge: gaugegrp {
>> +		fsl,pins = <
>> +			/* BAT_LOW */
>> +			MX8MQ_IOMUXC_SAI5_RXC_GPIO3_IO20	0x80
>> +		>;
>> +	};
>> +
>> +	pinctrl_gnsspwr: gnsspwrgrp {
>> +		fsl,pins = <
>> +			/* GPS3V3_EN */
>> +			MX8MQ_IOMUXC_NAND_DATA06_GPIO3_IO12	0x83
>> +		>;
>> +	};
>> +
>> +	pinctrl_haptic: hapticgrp {
>> +		fsl,pins = <
>> +			/* MOTO */
>> +			MX8MQ_IOMUXC_SPDIF_EXT_CLK_PWM1_OUT	0x83
>> +		>;
>> +	};
>> +
>> +	pinctrl_hp: hpgrp {
>> +		fsl,pins = <
>> +			/* HEADPHONE_DET_1V8 */
>> +			MX8MQ_IOMUXC_NAND_DATA03_GPIO3_IO9	0x180
>> +		>;
>> +	};
>> +
>> +	pinctrl_hub_pwr: hubpwrgrp {
>> +		fsl,pins = <
>> +			/* HUB_PWR_3V3_EN */
>> +			MX8MQ_IOMUXC_GPIO1_IO14_GPIO1_IO14	0x83
>> +		>;
>> +	};
>> +
>> +	pinctrl_i2c1: i2c1grp {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_I2C1_SCL_I2C1_SCL		0x40000026
>> +			MX8MQ_IOMUXC_I2C1_SDA_I2C1_SDA		0x40000026
>> +		>;
>> +	};
>> +
>> +	pinctrl_i2c2: i2c2grp {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_I2C2_SCL_I2C2_SCL		0x40000026
>> +			MX8MQ_IOMUXC_I2C2_SDA_I2C2_SDA		0x40000026
>> +		>;
>> +	};
>> +
>> +	pinctrl_i2c3: i2c3grp {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_I2C3_SCL_I2C3_SCL		0x40000026
>> +			MX8MQ_IOMUXC_I2C3_SDA_I2C3_SDA		0x40000026
>> +		>;
>> +	};
>> +
>> +	pinctrl_i2c4: i2c4grp {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_I2C4_SCL_I2C4_SCL		0x40000026
>> +			MX8MQ_IOMUXC_I2C4_SDA_I2C4_SDA		0x40000026
>> +		>;
>> +	};
>> +
>> +	pinctrl_keys: keysgrp {
>> +		fsl,pins = <
>> +			/* 4G_WAKE */
>> +			MX8MQ_IOMUXC_NAND_RE_B_GPIO3_IO15	0x80
>> +			/* PWR_KEY */
>> +			MX8MQ_IOMUXC_NAND_CLE_GPIO3_IO5		0x01C0
> 
> gpio3 5/15 are never used was this intended?
> 
>> +			/* VOL- */
>> +			MX8MQ_IOMUXC_ENET_MDIO_GPIO1_IO17	0x01C0
>> +			/* VOL+ */
>> +			MX8MQ_IOMUXC_ENET_MDC_GPIO1_IO16	0x01C0
>> +		>;
>> +	};
>> +
>> +	pinctrl_led_b: ledbgrp {
>> +		fsl,pins = <
>> +			/* LED_B */
>> +			MX8MQ_IOMUXC_GPIO1_IO13_PWM2_OUT	0x06
>> +		>;
>> +	};
>> +
>> +	pinctrl_led_g: ledggrp {
>> +		fsl,pins = <
>> +			/* LED_G */
>> +			MX8MQ_IOMUXC_SAI3_MCLK_PWM4_OUT		0x06
>> +		>;
>> +	};
>> +
>> +	pinctrl_led_r: ledrgrp {
>> +		fsl,pins = <
>> +			/* LED_R */
>> +			MX8MQ_IOMUXC_SPDIF_TX_PWM3_OUT		0x06
>> +		>;
>> +	};
>> +
>> +	pinctrl_mag: maggrp {
>> +		fsl,pins = <
>> +			/* INT_MAG */
>> +			MX8MQ_IOMUXC_SAI5_RXD1_GPIO3_IO22	0x80
>> +		>;
>> +	};
>> +
>> +	pinctrl_pmic: pmicgrp {
>> +		fsl,pins = <
>> +			/* PMIC_NINT */
>> +			MX8MQ_IOMUXC_GPIO1_IO07_GPIO1_IO7	0x80
>> +		>;
>> +	};
>> +
>> +	pinctrl_pmic_5v: pmic5vgrp {
>> +		fsl,pins = <
>> +			/* PMIC_5V */
>> +			MX8MQ_IOMUXC_GPIO1_IO01_GPIO1_IO1	0x80
>> +		>;
>> +	};
>> +
>> +	pinctrl_prox: proxgrp {
>> +		fsl,pins = <
>> +			/* INT_LIGHT */
>> +			MX8MQ_IOMUXC_NAND_DATA01_GPIO3_IO7	0x80
>> +		>;
>> +	};
>> +
>> +	pinctrl_rtc: rtcgrp {
>> +		fsl,pins = <
>> +			/* RTC_INT */
>> +			MX8MQ_IOMUXC_GPIO1_IO09_GPIO1_IO9	0x80
>> +		>;
>> +	};
>> +
>> +	pinctrl_sai2: sai2grp {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_SAI2_TXD0_SAI2_TX_DATA0	0xd6
>> +			MX8MQ_IOMUXC_SAI2_TXFS_SAI2_TX_SYNC	0xd6
>> +			MX8MQ_IOMUXC_SAI2_MCLK_SAI2_MCLK	0xd6
>> +			MX8MQ_IOMUXC_SAI2_RXD0_SAI2_RX_DATA0	0xd6
>> +			MX8MQ_IOMUXC_SAI2_TXC_SAI2_TX_BCLK	0xd6
>> +		>;
>> +	};
>> +
>> +	pinctrl_sai6: sai6grp {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_SAI1_RXD5_SAI6_RX_DATA0	0xd6
>> +			MX8MQ_IOMUXC_SAI1_RXD6_SAI6_RX_SYNC	0xd6
>> +			MX8MQ_IOMUXC_SAI1_TXD4_SAI6_RX_BCLK	0xd6
>> +			MX8MQ_IOMUXC_SAI1_TXD5_SAI6_TX_DATA0	0xd6
>> +		>;
>> +	};
>> +
>> +	pinctrl_tcpc: tcpcgrp {
>> +		fsl,pins = <
>> +			/* TCPC_INT */
>> +			MX8MQ_IOMUXC_GPIO1_IO10_GPIO1_IO10	0x01C0
>> +		>;
>> +	};
>> +
>> +	pinctrl_typec: typecgrp {
>> +		fsl,pins = <
>> +			/* TYPEC_MUX_EN */
>> +			MX8MQ_IOMUXC_GPIO1_IO11_GPIO1_IO11	0x83
>> +		>;
>> +	};
>> +
>> +	pinctrl_uart1: uart1grp {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_UART1_RXD_UART1_DCE_RX	0x49
>> +			MX8MQ_IOMUXC_UART1_TXD_UART1_DCE_TX	0x49
>> +		>;
>> +	};
>> +
>> +	pinctrl_uart2: uart2grp {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_UART2_TXD_UART2_DCE_TX	0x49
>> +			MX8MQ_IOMUXC_UART2_RXD_UART2_DCE_RX	0x49
>> +		>;
>> +	};
>> +
>> +	pinctrl_uart3: uart3grp {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_UART3_RXD_UART3_DCE_RX	0x49
>> +			MX8MQ_IOMUXC_UART3_TXD_UART3_DCE_TX	0x49
>> +		>;
>> +	};
>> +
>> +	pinctrl_uart4: uart4grp {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_ECSPI2_SCLK_UART4_DCE_RX		0x49
>> +			MX8MQ_IOMUXC_ECSPI2_MOSI_UART4_DCE_TX		0x49
>> +			MX8MQ_IOMUXC_ECSPI2_MISO_UART4_DCE_CTS_B	0x49
>> +			MX8MQ_IOMUXC_ECSPI2_SS0_UART4_DCE_RTS_B		0x49
>> +		>;
>> +	};
>> +
>> +	pinctrl_usdhc1: usdhc1grp {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK			0x83
>> +			MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD			0xc3
>> +			MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0		0xc3
>> +			MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1		0xc3
>> +			MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2		0xc3
>> +			MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3		0xc3
>> +			MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4		0xc3
>> +			MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5		0xc3
>> +			MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6		0xc3
>> +			MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7		0xc3
>> +			MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE		0x83
>> +			MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B		0xc1
>> +		>;
>> +	};
>> +
>> +	pinctrl_usdhc1_100mhz: usdhc1grp100mhz {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK			0x8d
>> +			MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD			0xcd
>> +			MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0		0xcd
>> +			MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1		0xcd
>> +			MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2		0xcd
>> +			MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3		0xcd
>> +			MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4		0xcd
>> +			MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5		0xcd
>> +			MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6		0xcd
>> +			MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7		0xcd
>> +			MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE		0x8d
>> +			MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B		0xc1
>> +		>;
>> +	};
>> +
>> +	pinctrl_usdhc1_200mhz: usdhc1grp200mhz {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK			0x9f
>> +			MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD			0xdf
>> +			MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0		0xdf
>> +			MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1		0xdf
>> +			MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2		0xdf
>> +			MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3		0xdf
>> +			MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4		0xdf
>> +			MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5		0xdf
>> +			MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6		0xdf
>> +			MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7		0xdf
>> +			MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE		0x9f
>> +			MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B		0xc1
>> +		>;
>> +	};
>> +
>> +	pinctrl_usdhc2: usdhc2grp {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_SD2_CD_B_USDHC2_CD_B	0x80
>> +			MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK		0x83
>> +			MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD		0xc3
>> +			MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0	0xc3
>> +			MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1	0xc3
>> +			MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2	0xc3
>> +			MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3	0xc3
>> +			MX8MQ_IOMUXC_SD2_RESET_B_USDHC2_RESET_B 0xc1
>> +		>;
>> +	};
>> +
>> +	pinctrl_usdhc2_100mhz: usdhc2grp100mhz {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_SD2_CD_B_USDHC2_CD_B	0x80
>> +			MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK		0x8d
>> +			MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD		0xcd
>> +			MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0	0xcd
>> +			MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1	0xcd
>> +			MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2	0xcd
>> +			MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3	0xcd
>> +			MX8MQ_IOMUXC_SD2_RESET_B_USDHC2_RESET_B 0xc1
>> +		>;
>> +	};
>> +
>> +	pinctrl_usdhc2_200mhz: usdhc2grp200mhz {
>> +		fsl,pins = <
>> +			MX8MQ_IOMUXC_SD2_CD_B_USDHC2_CD_B	0x80
>> +			MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK		0x9f
>> +			MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD		0xcf
>> +			MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0	0xcf
>> +			MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1	0xcf
>> +			MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2	0xcf
>> +			MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3	0xcf
>> +			MX8MQ_IOMUXC_SD2_RESET_B_USDHC2_RESET_B 0xc1
>> +		>;
>> +	};
>> +
>> +	pinctrl_wdog: wdoggrp {
>> +		fsl,pins = <
>> +			/* nWDOG */
>> +			MX8MQ_IOMUXC_GPIO1_IO02_WDOG1_WDOG_B	0x1f
>> +		>;
>> +	};
>> +};
>> +
>> +&i2c1 {
>> +	clock-frequency = <387000>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_i2c1>;
>> +	status = "okay";
>> +
>> +	typec_pd: usb-pd@3f {
>> +		compatible = "ti,tps6598x";
>> +		reg = <0x3f>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_typec>, <&pinctrl_tcpc>;
>> +		interrupt-parent = <&gpio1>;
>> +		interrupts = <10 IRQ_TYPE_LEVEL_LOW>;
>> +
>> +		connector {
>> +			ports {
>> +				#address-cells = <1>;
>> +				#size-cells = <0>;
>> +
>> +				port@0 {
>> +					reg = <0>;
>> +
>> +					usb_con_hs: endpoint {
>> +						remote-endpoint = <&typec_hs>;
>> +					};
>> +				};
>> +
>> +				port@1 {
>> +					reg = <1>;
>> +
>> +					usb_con_ss: endpoint {
>> +						remote-endpoint = <&typec_ss>;
>> +					};
>> +				};
>> +			};
>> +		};
>> +	};
>> +
>> +	pmic: pmic@4b {
>> +		compatible = "rohm,bd71837";
>> +		reg = <0x4b>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_pmic>;
>> +		clocks = <&pmic_osc>;
>> +		clock-names = "osc";
>> +		clock-output-names = "pmic_clk";
>> +		interrupt-parent = <&gpio1>;
>> +		interrupts = <7 GPIO_ACTIVE_LOW>;
>> +		interrupt-names = "irq";
>> +		rohm,reset-snvs-powered;
>> +
>> +		regulators {
>> +			buck1_reg: BUCK1 {
>> +				regulator-name = "buck1";
>> +				regulator-min-microvolt = <700000>;
>> +				regulator-max-microvolt = <1300000>;
>> +				regulator-ramp-delay = <1250>;
>> +				rohm,dvs-run-voltage = <900000>;
>> +				rohm,dvs-idle-voltage = <850000>;
>> +				rohm,dvs-suspend-voltage = <800000>;
>> +				regulator-always-on;
>> +			};
>> +
>> +			buck2_reg: BUCK2 {
>> +				regulator-name = "buck2";
>> +				regulator-min-microvolt = <700000>;
>> +				regulator-max-microvolt = <1300000>;
>> +				regulator-ramp-delay = <1250>;
>> +				rohm,dvs-run-voltage = <1000000>;
>> +				rohm,dvs-idle-voltage = <900000>;
>> +				regulator-always-on;
>> +			};
>> +
>> +			buck3_reg: BUCK3 {
>> +				regulator-name = "buck3";
>> +				regulator-min-microvolt = <700000>;
>> +				regulator-max-microvolt = <1300000>;
>> +				rohm,dvs-run-voltage = <900000>;
>> +				regulator-always-on;
>> +			};
>> +
>> +			buck4_reg: BUCK4 {
>> +				regulator-name = "buck4";
>> +				regulator-min-microvolt = <700000>;
>> +				regulator-max-microvolt = <1300000>;
>> +				rohm,dvs-run-voltage = <1000000>;
>> +			};
>> +
>> +			buck5_reg: BUCK5 {
>> +				regulator-name = "buck5";
>> +				regulator-min-microvolt = <700000>;
>> +				regulator-max-microvolt = <1350000>;
>> +				regulator-always-on;
>> +			};
>> +
>> +			buck6_reg: BUCK6 {
>> +				regulator-name = "buck6";
>> +				regulator-min-microvolt = <3000000>;
>> +				regulator-max-microvolt = <3300000>;
>> +				regulator-always-on;
>> +			};
>> +
>> +			buck7_reg: BUCK7 {
>> +				regulator-name = "buck7";
>> +				regulator-min-microvolt = <1605000>;
>> +				regulator-max-microvolt = <1995000>;
>> +				regulator-always-on;
>> +			};
>> +
>> +			buck8_reg: BUCK8 {
>> +				regulator-name = "buck8";
>> +				regulator-min-microvolt = <800000>;
>> +				regulator-max-microvolt = <1400000>;
>> +				regulator-always-on;
>> +			};
>> +
>> +			ldo1_reg: LDO1 {
>> +				regulator-name = "ldo1";
>> +				regulator-min-microvolt = <3000000>;
>> +				regulator-max-microvolt = <3300000>;
>> +				/* leave on for snvs power button */
>> +				regulator-always-on;
>> +			};
>> +
>> +			ldo2_reg: LDO2 {
>> +				regulator-name = "ldo2";
>> +				regulator-min-microvolt = <900000>;
>> +				regulator-max-microvolt = <900000>;
>> +				/* leave on for snvs power button */
>> +				regulator-always-on;
>> +			};
>> +
>> +			ldo3_reg: LDO3 {
>> +				regulator-name = "ldo3";
>> +				regulator-min-microvolt = <1800000>;
>> +				regulator-max-microvolt = <3300000>;
>> +				regulator-always-on;
>> +			};
>> +
>> +			ldo4_reg: LDO4 {
>> +				regulator-name = "ldo4";
>> +				regulator-min-microvolt = <900000>;
>> +				regulator-max-microvolt = <1800000>;
>> +				regulator-always-on;
>> +			};
>> +
>> +			ldo5_reg: LDO5 {
>> +				/* VDD_PHY_0V9 - MIPI and HDMI domains */
>> +				regulator-name = "ldo5";
>> +				regulator-min-microvolt = <1800000>;
>> +				regulator-max-microvolt = <3300000>;
>> +				regulator-always-on;
>> +			};
>> +
>> +			ldo6_reg: LDO6 {
>> +				/* VDD_PHY_0V9 - MIPI, HDMI and USB domains */
>> +				regulator-name = "ldo6";
>> +				regulator-min-microvolt = <900000>;
>> +				regulator-max-microvolt = <1800000>;
>> +				regulator-always-on;
>> +			};
>> +
>> +			ldo7_reg: LDO7 {
>> +				/* VDD_PHY_3V3 - USB domain */
>> +				regulator-name = "ldo7";
>> +				regulator-min-microvolt = <1800000>;
>> +				regulator-max-microvolt = <3300000>;
>> +				regulator-always-on;
>> +			};
> 
> Out of curiosity, why did you marked all regulators as
> regulator-always-on? I thought the librem5 is a smartphone.

It is. But we just currently see an unstable system in case we allow
turning most of them off. We do for the VPU, and should (afaik already)
be able to do so for the GPU. After more work in the drivers the mipi
regulator(s) should be able to be turned off. So we'll look at more and
more of these over time.

> 
>> +		};
>> +	};
>> +
>> +	rtc@68 {
>> +		compatible = "microcrystal,rv4162";
>> +		reg = <0x68>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_rtc>;
>> +		interrupt-parent = <&gpio1>;
>> +		interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
>> +	};
>> +};
>> +
>> +&i2c2 {
>> +	clock-frequency = <387000>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_i2c2>;
>> +	status = "okay";
>> +
>> +	magnetometer@1e	{
>> +		compatible = "st,lsm9ds1-magn";
>> +		reg = <0x1e>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_mag>;
>> +		interrupt-parent = <&gpio3>;
>> +		interrupts = <22 IRQ_TYPE_LEVEL_HIGH>;
>> +		vdd-supply = <&reg_vdd_sen>;
>> +		vddio-supply = <&reg_vdd_1v8>;
>> +	};
>> +
>> +	regulator@3e {
>> +		compatible = "tps65132";
>> +		reg = <0x3e>;
>> +		reg_lcd_avdd: outp {
>> +			regulator-name = "lcd_avdd";
>> +			vin-supply = <&reg_lcd_3v4>;
>> +		};
>> +
>> +		reg_lcd_avee: outn {
>> +			regulator-name = "lcd_avee";
>> +			vin-supply = <&reg_lcd_3v4>;
>> +		};
> 		both phandles are not used.
>> +	};
>> +
>> +	flash@53 {
>> +		compatible = "lm3560";
>> +		reg = <0x53>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		flash@0 {
>> +			reg = <0x0>;
>> +			flash-timeout-us = <150000>;
>> +			flash-max-microamp = <320000>;
>> +			led-max-microamp = <60000>;
>> +			label = "lm3560:flash";
>> +		};
>> +
>> +		torch@1 {
>> +			reg = <0x1>;
>> +			led-max-microamp = <10000>;
>> +			label = "lm3560:torch";
>> +		};
>> +
>> +	};
>> +
>> +	prox@60 {
>> +		compatible = "vishay,vcnl4040";
>> +		reg = <0x60>;
>> +		pinctrl-0 = <&pinctrl_prox>;
>> +		interrupt-parent = <&gpio3>;
>> +		interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
>> +	};
>> +
>> +	accel-gyro@6a	{
>> +		compatible = "st,lsm9ds1-imu";
>> +		reg = <0x6a>;
>> +		vdd-supply = <&reg_vdd_sen>;
>> +		vddio-supply = <&reg_vdd_1v8>;
>> +		mount-matrix =  "1",  "0",  "0",
>> +				"0",  "1",  "0",
>> +				"0",  "0", "-1";
>> +	};
>> +};
>> +
>> +&i2c3 {
>> +	clock-frequency = <387000>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_i2c3>;
>> +	status = "okay";
>> +
>> +	codec: wm8962@1a {
> 
> Please use generic names.
> 
>> +		compatible = "wlf,wm8962";
>> +		reg = <0x1a>; // 0x4a is the test address
>> +		clocks = <&clk IMX8MQ_CLK_SAI2_ROOT>;
>> +		assigned-clocks = <&clk IMX8MQ_CLK_SAI2>;
>> +		assigned-clock-parents = <&clk IMX8MQ_AUDIO_PLL1_OUT>;
>> +		assigned-clock-rates = <24576000>;
>> +		#sound-dai-cells = <0>;
>> +		mic-cfg = <0x200>;
>> +		DCVDD-supply = <&reg_aud_1v8>;
>> +		DBVDD-supply = <&reg_aud_1v8>;
>> +		AVDD-supply = <&reg_aud_1v8>;
>> +		CPVDD-supply = <&reg_aud_1v8>;
>> +		MICVDD-supply = <&reg_aud_1v8>;
>> +		PLLVDD-supply = <&reg_aud_1v8>;
>> +		SPKVDD1-supply = <&reg_vsys_3v4>;
>> +		SPKVDD2-supply = <&reg_vsys_3v4>;
>> +		gpio-cfg = <
>> +			0x0000 /* n/c */
>> +			0x0001 /* gpio2, 1: default */
>> +			0x0013 /* gpio3, 2: dmicclk */
>> +			0x0000 /* n/c, 3: default */
>> +			0x8014 /* gpio5, 4: dmic_dat */
>> +			0x0000 /* gpio6, 5: default */
>> +		>;
>> +		status = "okay";
> 
> status can be dropped
> 
>> +	};
>> +
>> +	backlight@36 {
>> +		compatible = "ti,lm36922";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_bl>;
>> +		reg = <0x36>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		enable-gpios = <&gpio3 14 GPIO_ACTIVE_HIGH>;
>> +		vled-supply = <&reg_vsys_3v4>;
>> +		ti,ovp-microvolt = <25000000>;
>> +
>> +		led_backlight: led@0 {
>> +			reg = <0>;
>> +			label = "white:backlight_cluster";
>> +			linux,default-trigger = "backlight";
>> +			led-max-microamp = <20000>;
>> +		};
>> +	};
>> +
>> +	touchscreen@38 {
>> +		compatible = "edt,edt-ft5506";
>> +		reg = <0x38>;
>> +		interrupt-parent = <&gpio1>;
>> +		interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
> 
> You need to mux the irq gpio.
> 
>> +		irq-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>;
> 
> irq-gpios is not supported by the driver. We only have a
> wake/reset-gpio.
> 
>> +		touchscreen-size-x = <720>;
>> +		touchscreen-size-y = <1440>;
>> +	};
>> +};
>> +
>> +&i2c4 {
>> +	clock-frequency = <387000>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_i2c4>;
>> +	status = "okay";
>> +
>> +	bat: fuel-gauge@36 {
>> +		compatible = "maxim,max17055";
>> +		interrupt-parent = <&gpio3>;
>> +		interrupts = <20 IRQ_TYPE_LEVEL_LOW>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_gauge>;
>> +		reg = <0x36>;
> 
> Please check that "reg" is always the 2nd property after the
> "compatible".
> 
>> +		maxim,over-heat-temp = <700>;
>> +		maxim,over-volt = <4500>;
>> +		maxim,rsns-microohm = <5000>;
>> +	};
>> +
>> +	charger@6a { /* bq25895 */
>> +		compatible = "ti,bq25890";
> 
> The compatible should be "ti,bq25895" if it is a bq25895. So we can drop
> the comment too.
> 
>> +		reg = <0x6a>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_charger_in>;
>> +		interrupt-parent = <&gpio3>;
>> +		interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
>> +		phys = <&usb3_phy0>;
>> +		ti,battery-regulation-voltage = <4192000>; /* 4.192V */
>> +		ti,charge-current = <1600000>; /* 1.6A */
>> +		ti,termination-current = <66000>;  /* 66mA */
>> +		ti,precharge-current = <130000>; /* 130mA */
>> +		ti,minimum-sys-voltage = <3700000>; /* 3.7V */
>> +		ti,boost-voltage = <5000000>; /* 5V */
>> +		ti,boost-max-current = <50000>; /* 50mA */
>> +		ti,use-vinmin-threshold = <1>; /* enable VINDPM */
>> +		ti,vinmin-threshold = <3900000>; /* 3.9V */
> 
> I would only mention the units within a comment because comments like
> this begin to divergence after you fix something.
> 
> Regards,
>   Marco
> 

Basically I try to follow all your suggestions that I haven't commented
here, too.

thank you very much for this in-depth review. That helps a lot and I try
to put together a second revision soon.

                             martin

Powered by blists - more mailing lists