Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 16 Sep 2022 11:18:15 +0000
From:   Alessandro Carminati <alessandro.carminati@gmail.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Heiko Stuebner <heiko@sntech.de>, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Changes phy configuration to expose sata in place of
 usb3 on quartz64-a board
Message-ID: <YyRbd0vaAwu0DI7n@lab.hqhome163.com>
References: <YyIyo42QWvgJTBjL@lab.hqhome163.com>
 <c46fd424-3517-9f66-dc8c-ed10ca3ef622@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c46fd424-3517-9f66-dc8c-ed10ca3ef622@linaro.org>
Precedence: bulk

Hello, I'm reviewing the notes you made me on the review.
To tell all the truth, I derived the DTS from an existing one by modifying a small portion of it. 
In the end, your notes are for the already existing DTS.
To address the notes in your message and spare future work, I decided to refactor the rk3566-quartz64-a DTS.
I will post another patch briefly with the new version. 
For the moment, thank you for the review.
Regards
Alessandro

On Thu, Sep 15, 2022 at 04:07:54PM +0100, Krzysztof Kozlowski wrote:
> On 14/09/2022 20:59, Alessandro Carminati wrote:
> > The Quartz64 board is built upon Rockchip RK3566.
> > Rockchip RK3566 has two combo phys.
> > The first connects USB3 and SATA ctrl1, and the second PCIe lane and SATA
> > ctrl2.
> > The second combo phy is hardwired to the PCIe slot, where for the first,
> > the hardware on the board provides both the USB3 connector and the SATA
> > connector.
> > This DT allows the users to switch the combo phy to the SATA connector.
> > 
> > Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
> > ---
> >  .../dts/rockchip/rk3566-quartz64-a.sata.dts   | 839 ++++++++++++++++++
> >  1 file changed, 839 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.sata.dts
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.sata.dts b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.sata.dts
> > new file mode 100644
> > index 000000000000..6ac21b729be7
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.sata.dts
> > @@ -0,0 +1,839 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +
> > +/dts-v1/;
> > +
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/pinctrl/rockchip.h>
> > +#include <dt-bindings/soc/rockchip,vop2.h>
> > +#include "rk3566.dtsi"
> > +
> > +/ {
> > +	model = "Pine64 RK3566 Quartz64-A Board";
> > +	compatible = "pine64,quartz64-a", "rockchip,rk3566";
> > +
> > +	aliases {
> > +		ethernet0 = &gmac1;
> > +		mmc0 = &sdmmc0;
> > +		mmc1 = &sdhci;
> > +	};
> > +
> > +	chosen: chosen {
> > +		stdout-path = "serial2:1500000n8";
> > +	};
> > +
> > +	gmac1_clkin: external-gmac1-clock {
> > +		compatible = "fixed-clock";
> > +		clock-frequency = <125000000>;
> > +		clock-output-names = "gmac1_clkin";
> > +		#clock-cells = <0>;
> > +	};
> > +
> > +	fan: gpio_fan {
> 
> No underscores in node names. Node name just "fan"
> 
> > +		compatible = "gpio-fan";
> > +		gpios = <&gpio0 RK_PD5 GPIO_ACTIVE_HIGH>;
> > +		gpio-fan,speed-map = <0    0
> > +				      4500 1>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&fan_en_h>;
> > +		#cooling-cells = <2>;
> > +	};
> > +
> > +	hdmi-con {
> 
> Node name: connector
> 
> > +		compatible = "hdmi-connector";
> > +		type = "a";
> > +
> > +		port {
> > +			hdmi_con_in: endpoint {
> > +				remote-endpoint = <&hdmi_out_con>;
> > +			};
> > +		};
> > +	};
> > +
> > +	leds {
> > +		compatible = "gpio-leds";
> > +
> > +		led-work {
> > +			label = "work-led";
> > +			default-state = "off";
> > +			gpios = <&gpio0 RK_PD3 GPIO_ACTIVE_HIGH>;
> > +			pinctrl-names = "default";
> > +			pinctrl-0 = <&work_led_enable_h>;
> > +			retain-state-suspended;
> > +		};
> > +
> > +		led-diy {
> > +			label = "diy-led";
> > +			default-state = "on";
> > +			gpios = <&gpio0 RK_PD4 GPIO_ACTIVE_HIGH>;
> > +			linux,default-trigger = "heartbeat";
> > +			pinctrl-names = "default";
> > +			pinctrl-0 = <&diy_led_enable_h>;
> > +			retain-state-suspended;
> > +		};
> > +	};
> > +
> > +	rk817-sound {
> > +		compatible = "simple-audio-card";
> > +		simple-audio-card,format = "i2s";
> > +		simple-audio-card,name = "Analog RK817";
> > +		simple-audio-card,mclk-fs = <256>;
> > +
> > +		simple-audio-card,cpu {
> > +			sound-dai = <&i2s1_8ch>;
> > +		};
> > +
> > +		simple-audio-card,codec {
> > +			sound-dai = <&rk817>;
> > +		};
> > +	};
> > +
> > +	sdio_pwrseq: sdio-pwrseq {
> > +		compatible = "mmc-pwrseq-simple";
> > +		clocks = <&rk817 1>;
> > +		clock-names = "ext_clock";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&wifi_enable_h>;
> > +		post-power-on-delay-ms = <100>;
> > +		power-off-delay-us = <5000000>;
> > +		reset-gpios = <&gpio2 RK_PC2 GPIO_ACTIVE_LOW>;
> > +	};
> > +
> > +	spdif_dit: spdif-dit {
> > +		compatible = "linux,spdif-dit";
> > +		#sound-dai-cells = <0>;
> > +	};
> > +
> > +	spdif_sound: spdif-sound {
> > +		compatible = "simple-audio-card";
> > +		simple-audio-card,name = "SPDIF";
> > +
> > +		simple-audio-card,cpu {
> > +			sound-dai = <&spdif>;
> > +		};
> > +
> > +		simple-audio-card,codec {
> > +			sound-dai = <&spdif_dit>;
> > +		};
> > +	};
> > +
> > +	vcc12v_dcin: vcc12v_dcin {
> 
> No underscores in node names, generic node name, so at least with
> regulator prefix or suffix.
> 
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vcc12v_dcin";
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +		regulator-min-microvolt = <12000000>;
> > +		regulator-max-microvolt = <12000000>;
> > +	};
> > +
> > +	/* vbus feeds the rk817 usb input.
> > +	 * With no battery attached, also feeds vcc_bat+
> > +	 * via ON/OFF_BAT jumper
> > +	 */
> > +	vbus: vbus {
> 
> Ditto
> 
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vbus";
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +		vin-supply = <&vcc12v_dcin>;
> > +	};
> > +
> > +	vcc3v3_pcie_p: vcc3v3-pcie-p-regulator {
> 
> And here you have suffix...
> 
> > +		compatible = "regulator-fixed";
> > +		enable-active-high;
> > +		gpio = <&gpio0 RK_PC6 GPIO_ACTIVE_HIGH>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pcie_enable_h>;
> > +		regulator-name = "vcc3v3_pcie_p";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		vin-supply = <&vcc_3v3>;
> > +	};
> > +
> > +	vcc5v0_usb: vcc5v0_usb {
> 
> Ditto
> 
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vcc5v0_usb";
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +		vin-supply = <&vcc12v_dcin>;
> > +	};
> > +
> > +	/* all four ports are controlled by one gpio
> > +	 * the host ports are sourced from vcc5v0_usb
> > +	 * the otg port is sourced from vcc5v0_midu
> > +	 */
> > +	vcc5v0_usb20_host: vcc5v0_usb20_host {
> 
> and in other places as well
> 
> 
> 
> > +		compatible = "regulator-fixed";
> > +		enable-active-high;
> > +		gpio = <&gpio4 RK_PB5 GPIO_ACTIVE_HIGH>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&vcc5v0_usb20_host_en>;
> > +		regulator-name = "vcc5v0_usb20_host";
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +		vin-supply = <&vcc5v0_usb>;
> > +	};
> > +
> > +	vcc5v0_usb20_otg: vcc5v0_usb20_otg {
> > +		compatible = "regulator-fixed";
> > +		enable-active-high;
> > +		gpio = <&gpio4 RK_PB5 GPIO_ACTIVE_HIGH>;
> > +		regulator-name = "vcc5v0_usb20_otg";
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +		vin-supply = <&dcdc_boost>;
> > +	};
> > +
> > +	vcc3v3_sd: vcc3v3_sd {
> > +		compatible = "regulator-fixed";
> > +		enable-active-low;
> > +		gpio = <&gpio0 RK_PA5 GPIO_ACTIVE_LOW>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&vcc_sd_h>;
> > +		regulator-boot-on;
> > +		regulator-name = "vcc3v3_sd";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		vin-supply = <&vcc_3v3>;
> > +	};
> > +
> > +	/* sourced from vbus and vcc_bat+ via rk817 sw5 */
> > +	vcc_sys: vcc_sys {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vcc_sys";
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +		regulator-min-microvolt = <4400000>;
> > +		regulator-max-microvolt = <4400000>;
> > +		vin-supply = <&vbus>;
> > +	};
> > +
> > +	/* sourced from vcc_sys, sdio module operates internally at 3.3v */
> > +	vcc_wl: vcc_wl {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vcc_wl";
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		vin-supply = <&vcc_sys>;
> > +	};
> > +};
> > +
> > +&combphy1 {
> > +	status = "okay";
> > +};
> > +
> > +&combphy2 {
> > +	status = "okay";
> > +};
> > +
> > +&cpu0 {
> > +	cpu-supply = <&vdd_cpu>;
> > +};
> > +
> > +&cpu1 {
> > +	cpu-supply = <&vdd_cpu>;
> > +};
> > +
> > +&cpu2 {
> > +	cpu-supply = <&vdd_cpu>;
> > +};
> > +
> > +&cpu3 {
> > +	cpu-supply = <&vdd_cpu>;
> > +};
> > +
> > +&cpu_thermal {
> > +	trips {
> > +		cpu_hot: cpu_hot {
> 
> No underscores in node names. Are you sure bindings/schema do not expect
> some specific name?
> 
> > +			temperature = <55000>;
> > +			hysteresis = <2000>;
> > +			type = "active";
> > +		};
> > +	};
> 
> 
> Best regards,
> Krzysztof