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@xxxxxxxxx>
> > ---
> > .../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
