| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2370153.n0vTqR1ltx@phil>
Date: Fri, 23 Nov 2018 13:31:17 +0100
From: Heiko Stuebner <heiko@...ech.de>
To: Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Ezequiel Garcia <ezequiel@...labora.com>,
Enric Balletbo i Serra <enric.balletbo@...labora.com>,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] arm64: dts: rockchip: Add DT for nanopc-t4
Hi Tomeu,
Am Freitag, 23. November 2018, 08:46:30 CET schrieb Tomeu Vizoso:
> diff --git a/Documentation/devicetree/bindings/arm/rockchip.txt b/Documentation/devicetree/bindings/arm/rockchip.txt
> index 0cc71236d639..e907d309486e 100644
> --- a/Documentation/devicetree/bindings/arm/rockchip.txt
> +++ b/Documentation/devicetree/bindings/arm/rockchip.txt
> @@ -71,6 +71,10 @@ Rockchip platforms device tree bindings
> Required root node properties:
> - compatible = "firefly,roc-rk3399-pc", "rockchip,rk3399";
>
> +- FriendlyElec NanoPC-T4 board:
> + Required root node properties:
> + - compatible = "friendlyarm,nanopc-t4", "rockchip,rk3399";
> +
alphabetical please
> - ChipSPARK PopMetal-RK3288 board:
> Required root node properties:
> - compatible = "chipspark,popmetal-rk3288", "rockchip,rk3288";
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> index 49042c477870..ed90cd1e5a8b 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -20,3 +20,4 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rock960.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rockpro64.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire.dtb
> dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire-excavator.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
alphabetical please
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
> new file mode 100644
> index 000000000000..148f85b4bd49
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
[...]
General comment about regulators, the vendor-kernel dts' regularly
don't model regulators in a nice way representing the hardware.
There is obviously schematics available for the board
http://wiki.friendlyarm.com/wiki/images/d/dd/NanoPi-M4-2GB-1807-Schematic.pdf
Please model the regulator tree following the naming scheme from the
schematics and including correct supply chaining, so that
$debug/regulator/regulator_summary looks nice.
This makes it way easier to find issues later on if needed and represents
the hardware in a correct way.
I guess in the end it should look pretty similar to the rock960 or other
rk3399 boards (except gru), as most boards follow the reference schematics
for a big part.
> + vcc3v3_sys: vcc3v3-sys {
> + compatible = "regulator-fixed";
> + regulator-name = "vcc3v3_sys";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + };
> +
> + vcc5v0_host: vcc5v0-host-regulator {
> + compatible = "regulator-fixed";
> + regulator-name = "vcc5v0_host";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + };
> +
> + vcc5v0_sys: vcc5v0-sys {
> + compatible = "regulator-fixed";
> + regulator-name = "vcc5v0_sys";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + };
> +
> + vccadc_ref: vccadc-ref {
> + compatible = "regulator-fixed";
> + regulator-name = "vcc1v8_sys";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + };
> +
> + vcc_sd: vcc-sd {
> + compatible = "regulator-fixed";
> + enable-active-high;
> + gpio = <&gpio0 1 GPIO_ACTIVE_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&vcc_sd_h>;
> + regulator-name = "vcc_sd";
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3000000>;
> + };
> +
> + vcc_phy: vcc-phy-regulator {
> + compatible = "regulator-fixed";
> + regulator-name = "vcc_phy";
> + regulator-always-on;
> + regulator-boot-on;
> + };
> +
> + vcc_lcd: vcc-lcd {
> + compatible = "regulator-fixed";
> + regulator-name = "vcc_lcd";
> + gpio = <&gpio4 30 GPIO_ACTIVE_HIGH>;
> + startup-delay-us = <20000>;
> + enable-active-high;
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + vin-supply = <&vcc5v0_sys>;
> + };
> +
> + vcc5v0_typec: vcc5v0-typec-regulator {
> + compatible = "regulator-fixed";
> + enable-active-high;
> + gpios = <&gpio4 26 GPIO_ACTIVE_HIGH>;
> + regulator-name = "vcc5v0_typec";
> + regulator-always-on;
> + vin-supply = <&vcc5v0_sys>;
> + };
> +
> + vdd_log: vdd-log {
> + compatible = "pwm-regulator";
> + pwms = <&pwm2 0 25000 1>;
> + regulator-name = "vdd_log";
> + regulator-min-microvolt = <800000>;
> + regulator-max-microvolt = <1400000>;
> + regulator-always-on;
> + regulator-boot-on;
> +
> + /* for rockchip boot on */
> + rockchip,pwm_id = <2>;
> + rockchip,pwm_voltage = <900000>;
> + };
you might want to drop vdd_log for the time being. See
https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=v4.20-armsoc/dts64-fixes&id=13682e524167cbd7e2a26c5e91bec765f0f96273
> + sdio_pwrseq: sdio-pwrseq {
> + compatible = "mmc-pwrseq-simple";
> + clocks = <&rk808 1>;
> + clock-names = "ext_clock";
> + pinctrl-names = "default";
> + pinctrl-0 = <&wifi_enable_h>;
> +
> + /*
> + * On the module itself this is one of these (depending
> + * on the actual card populated):
> + * - SDIO_RESET_L_WL_REG_ON
> + * - PDN (power down when low)
> + */
> + reset-gpios = <&gpio0 10 GPIO_ACTIVE_LOW>; /* GPIO0_B2 */
general for all gpios: <&gpio RK_PB2 ...> for new boards please
> + };
> +};
[...]
> +&rga {
> + status = "disabled";
Why disabled? It shouldn't hurt.
> +};
> +
> +&cdn_dp {
> +// TODO: typec/fusb302 doesn't have extcon support yet
> +// status = "enabled";
> + extcon = <&fusb0>;
extcon is not specified and as we talked about yesterday, the
whole thing doesn't work with the type-c framework yet,
so ideally just remove the whole &cdn_dp node here.
> + phys = <&tcphy0_dp>;
> +};
> +
> +&hdmi {
general for node-phandles: alphabetical please
> + ddc-i2c-bus = <&i2c7>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&hdmi_cec>;
> + status = "okay";
> +};
> +
> +&i2c0 {
> + status = "okay";
> + i2c-scl-rising-time-ns = <160>;
> + i2c-scl-falling-time-ns = <30>;
> + clock-frequency = <400000>;
> +
> + vdd_cpu_b: syr827@40 {
> + compatible = "silergy,syr827";
> + reg = <0x40>;
> + vin-supply = <&vcc3v3_sys>;
> + regulator-compatible = "fan53555-reg";
deprecated (and unusued) property
> + pinctrl-names = "default";
> + pinctrl-0 = <&vsel1_gpio>;
> + vsel-gpios = <&gpio1 17 GPIO_ACTIVE_HIGH>;
not specified in mainline, ideally look at rock960 and friends
for reference
> + regulator-name = "vdd_cpu_b";
> + regulator-min-microvolt = <712500>;
> + regulator-max-microvolt = <1500000>;
> + regulator-ramp-delay = <1000>;
> + fcs,suspend-voltage-selector = <1>;
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-initial-state = <3>;
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + vdd_gpu: syr828@41 {
> + compatible = "silergy,syr828";
> + reg = <0x41>;
> + vin-supply = <&vcc3v3_sys>;
> + regulator-compatible = "fan53555-reg";
> + pinctrl-names = "default";
> + pinctrl-0 = <&vsel2_gpio>;
> + vsel-gpios = <&gpio1 14 GPIO_ACTIVE_HIGH>;
same here
> + regulator-name = "vdd_gpu";
> + regulator-min-microvolt = <712500>;
> + regulator-max-microvolt = <1500000>;
> + regulator-ramp-delay = <1000>;
> + fcs,suspend-voltage-selector = <1>;
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-initial-state = <3>;
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
[...]
> +&i2c4 {
> + status = "okay";
> + i2c-scl-rising-time-ns = <160>;
> + i2c-scl-falling-time-ns = <30>;
> + clock-frequency = <400000>;
> +
> + fusb0: typec-portc@22 {
> + compatible = "fcs,fusb302";
> + reg = <0x22>;
> + interrupt-parent = <&gpio1>;
> + interrupts = <RK_PA2 IRQ_TYPE_LEVEL_LOW>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&fusb0_int>;
> + vbus-supply = <&vcc5v0_typec>;
> + status = "okay";
> + };
> +};
> +
> +&i2c7 {
> + status = "okay";
> +};
> +
> +
double empty line
> +&io_domains {
> + status = "okay";
> +
> + bt656-supply = <&vcc1v8_dvp>; /* bt656_gpio2ab_ms */
> + audio-supply = <&vcca1v8_codec>;
> + sdmmc-supply = <&vccio_sd>; /* sdmmc_gpio4b_ms */
> + gpio1830-supply = <&vcc_3v0>; /* gpio1833_gpio4cd_ms */
I think we can do without the comments.
> +};
> +
> +&pmu_io_domains {
> + status = "okay";
> + pmu1830-supply = <&vcc_3v0>;
> +};
> +
> +&pcie_phy {
> + status = "okay";
> + assigned-clocks = <&cru SCLK_PCIEPHY_REF>;
> + assigned-clock-parents = <&cru SCLK_PCIEPHY_REF100M>;
> + assigned-clock-rates = <100000000>;
> +};
> +
> +&pcie0 {
> + status = "okay";
> + ep-gpios = <&gpio2 4 GPIO_ACTIVE_HIGH>;
> + num-lanes = <4>;
> + max-link-speed = <2>;
> +};
> +
> +&pwm0 {
> + status = "okay";
> +};
> +
> +&pwm1 {
> + status = "okay";
> +};
> +
> +&pwm2 {
> + status = "okay";
> + pinctrl-names = "active";
> + pinctrl-0 = <&pwm2_pin_pull_down>;
> +};
> +
> +&saradc {
> + status = "okay";
> + vref-supply = <&vccadc_ref>; /* TBD */
> +};
> +
> +&sdhci {
> + bus-width = <8>;
> + mmc-hs400-1_8v;
> + supports-emmc;
> + non-removable;
> + keep-power-in-suspend;
> + mmc-hs400-enhanced-strobe;
> + status = "okay";
> +};
> +
> +&emmc_phy {
> + status = "okay";
> +};
> +
> +&sdio0 {
> + clock-frequency = <50000000>;
We have a ciu clock, so there should be no need for "clock-frquency"
> + clock-freq-min-max = <200000 50000000>;
Not part of a binding and the mmc code also seems to ignore it
> + supports-sdio;
unused and undocumented
> + bus-width = <4>;
> + disable-wp;
> + cap-sd-highspeed;
> + cap-sdio-irq;
> + keep-power-in-suspend;
> + mmc-pwrseq = <&sdio_pwrseq>;
> + non-removable;
> + num-slots = <1>;
outdated and unused property
> + pinctrl-names = "default";
> + pinctrl-0 = <&sdio0_bus4 &sdio0_cmd &sdio0_clk>;
> + sd-uhs-sdr104;
> + status = "okay";
> +};
> +
> +&sdmmc {
> + clock-frequency = <150000000>;
> + clock-freq-min-max = <100000 150000000>;
same as sdio
> + supports-sd;
unused and undocumented
> + bus-width = <4>;
> + cap-mmc-highspeed;
> + cap-sd-highspeed;
> + disable-wp;
> + num-slots = <1>;
outdated and unused property
> + sd-uhs-sdr104;
> + vmmc-supply = <&vcc_sd>;
> + vqmmc-supply = <&vccio_sd>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
> + status = "okay";
> +};
> +
> +&tsadc {
> + /* tshut mode 0:CRU 1:GPIO */
> + rockchip,hw-tshut-mode = <1>;
> + /* tshut polarity 0:LOW 1:HIGH */
> + rockchip,hw-tshut-polarity = <1>;
> + status = "okay";
> +};
> +
> +&tcphy0 {
> + extcon = <&fusb0>;
right now the fusb302 does not provide this extcon and should also
never do so. When omitting it, the tcphy will at least work in usb3
host mode.
> + status = "okay";
> +};
> +
> +&tcphy1 {
> + status = "okay";
> +};
> +
> +&u2phy0 {
> + status = "okay";
> + extcon = <&fusb0>;
same with the extcon
> +
> + u2phy0_otg: otg-port {
> + status = "okay";
> + };
> +
> + u2phy0_host: host-port {
> + phy-supply = <&vcc5v0_host>;
> + status = "okay";
> + };
> +};
> +
> +&u2phy1 {
> + status = "okay";
> +
> + u2phy1_otg: otg-port {
> + status = "okay";
> + };
> +
> + u2phy1_host: host-port {
> + phy-supply = <&vcc5v0_host>;
> + status = "okay";
> + };
> +};
> +
> +&usbdrd3_0 {
> + status = "okay";
> + extcon = <&fusb0>;
not part of any binding I think?
> +&pinctrl {
> +
> + hdmi {
> + /delete-node/ hdmi-i2c-xfer;
> + };
No need to delete the node, the hdmi-pinctrl above does not use the
internal i2c.
Heiko
Powered by blists - more mailing lists