On 2024/9/5 11:40, Chukun Pan wrote:
> Hi Junhao,
>
>> ...
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-ariaboard-photonicat.dts
>
> This should be 'rk3568-photonicat.dts',
> e.g. "Radxa ROCK 3A" -> rk3568-rock-3a.dts
>
>> ...
>> + model = "Ariaboard Photonicat RK3568";
>> + compatible = "ariaboard,photonicat", "rockchip,rk3568";
>
> The official model name does not include 'RK3568'.
I will rename it.
>
>> ...
>> + firmware {
>> + optee: optee {
>> + compatible = "linaro,optee-tz";
>> + method = "smc";
>> + };
>> + };
>> +
>> ...
>> + reserved-memory {
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + ranges;
>> +
>> + ramoops: ramoops@110000 {
>> + compatible = "ramoops";
>> + reg = <0 0x110000 0 0xf0000>;
>> + console-size = <0x80000>;
>> + ftrace-size = <0x00000>;
>> + pmsg-size = <0x50000>;
>> + record-size = <0x20000>;
>> + };
>> + };
>
> Maybe these can be moved to rk356x.dtsi?
Yes, I will split them.
>
>> ...
>> + vcca1v8: regulator-1v8-vcca {
>
> schematics: VCCA_1V8
>
>> ...
> + vcc3v3_pcie: regulator-3v3-vcc-pcie {
> + compatible = "regulator-fixed";
> + enable-active-high;
> + gpios = <&gpio0 RK_PA6 GPIO_ACTIVE_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pcie_enable_h>;
>
> schematics: pcie_pwren_h
> vcc_syson -> vcc3v3_pi6c
> vcc_syson -> vcc3v3_pcie
>
>> + regulator-always-on;
>> + regulator-boot-on;
>
> No need.
>
>> ...
>> + vcc5v0_sys: regulator-5v0-vcc-sys {
>
> There is no vcc5v0_sys, but vcc_syson.
>
> vcc_syson (5v) -> vcc3v3_sys
> vcc_sysin (5v) - (mcu) -> vcc_syson
> vccin_5v -> vcc_sysin
>
>> ...
>> + vcc5v0_usb_host: regulator-5v0-vcc-usb-host {
>
> schematics: VCC5V0_USB30_OTG0 and usb_host_pwren_h
> It's a little weird, but that's what they're calling it.
> Also: VCCIN_5V -> VCC5V0_USB30_OTG0
>
>> ...
>> + vcc5v0_usb_modem: regulator-5v0-vcc-usb-modem {
>
> Are you sure this regulator is 5v?
>
It should actually be 3.3V, I will fix it and rename to vcc3v3_usb_modem
>
>> ...
>> + vdda0v9: regulator-0v9-vdda {
>
> schematics: VDDA_0V9
>
>> + wifi_pwrseq: wifi-pwrseq {
>> + compatible = "mmc-pwrseq-simple";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&wifi_enable_h>;
>
> schematics: wifi_reg_on_h
> Also you need to enable the clk:
>
> clocks = <&pmucru CLK_RTC_32K>;
> clock-names = "ext_clock";
> pinctrl-names = "default";
> pinctrl-0 = <&wifi_reg_on_h &clk32k_out1>;
>
>> + post-power-on-delay-ms = <200>;
>> + reset-gpios = <&gpio2 RK_PB1 GPIO_ACTIVE_LOW>;
>> + };
>
>> ...
>> +&pcie30phy {
>> + phy-supply = <&vcc3v3_pcie>;
>
> phy-supply = <&vcc3v3_pi6c>;
I will change it to vcc3v3_pi6c.
But there seems to be a warning here, maybe phy-supply is missing in rockchip,pcie3-phy.yaml?
/tmp/build/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dtb: phy@fe8c0000: 'phy-supply' does not match any of the regexes: 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/phy/rockchip,pcie3-phy.yaml#
>
>> ...
>> +&pcie3x2 {
>> + max-link-speed = <1>;
>> + num-lanes = <1>;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pcie30x2m1_pins>;
>
> These are actually pcie30x1m0_pins.
pcie30x1m0_pins seems to conflict with sdmmc0, I changed it to pcie30x1m1_pins
>
>> ...
>> +&pmugrf {
>> + reboot-mode {
>
> Maybe these can be moved to rk356x.dtsi?
>
>> ...
>> +&sdhci {
>
> Missing mmc-hs200-1_8v;
>
[...]>
> ath10k does not need compatible.
I will remove them
>
>> ...
>> +&uart1 {
>> ...
>> + clocks = <&pmucru CLK_RTC_32K>;
>> + enable-gpios = <&gpio2 RK_PB7 GPIO_ACTIVE_HIGH>;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&bt_enable_h>;
>
> schematics: bt_reg_on_h
> Missing clock-names = "lpo";
When I add clocks-name, check_dtbs gives me a warning
/tmp/build/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dtb: bluetooth: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/net/bluetooth/qualcomm-bluetooth.yaml#
Maybe clock-name is missing in qualcomm-bluetooth.yaml?
>
Thanks for your review, I will fix all problems in PATCH v2.
Best regards,
Junhao
