Re: [PATCH v2 3/3] arm64: dts: rockchip: Add Orange Pi 5

[Muhammed Efe Cetin]

Hi Ondřej,

On 19.08.2023 00:24, Ondřej Jirman wrote:
> Hi Muhammed,
> 
> On Fri, Aug 18, 2023 at 07:05:51PM +0300, Muhammed Efe Cetin wrote:
>> Add initial support for OPi5 that includes support for USB2, PCIe2, Sata,
>> Sdmmc, SPI Flash, PMIC.
>>
>> Signed-off-by: Muhammed Efe Cetin <efectn@xxxxxxxx>
>>
>> [...]
>>
>> +
>> +	adc-keys {
>> +		compatible = "adc-keys";
>> +		io-channels = <&saradc 1>;
>> +		io-channel-names = "buttons";
>> +		keyup-threshold-microvolt = <1800000>;
>> +		poll-interval = <100>;
>> +
>> +		button-recovery {
>> +			label = "Recovery";
>> +			linux,code = <KEY_VENDOR>;
>> +			press-threshold-microvolt = <1000>;
> 
> I calculated 1800. (1.8e6 * 10 / 10e3)
> 
>> +		};
>> +	};
>> +
>>
>> [...]
>>
>> +
>> +	vcc_1v1_nldo_s3: vcc-1v1-nldo-s3-regulator {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vcc_1v1_nldo_s3";
>> +		regulator-always-on;
>> +		regulator-boot-on;
>> +		regulator-min-microvolt = <1100000>;
>> +		regulator-max-microvolt = <1100000>;
>> +		vin-supply = <&vcc5v0_sys>;
>> +	};
> 
> This is still wrong. vcc_1v1_nldo_s3 is just alias for dcdc-reg6.

You sound right. Compared opi5 and several rk3588 boards, opi5 has different design than others. Should we also add regulator-min-microvolt and regulator-max-microvolt to dcdc-reg6 or only add vcc_1v1_nldo_s3 as alias? It seems better to add them according to schematics.

> 
>> +&i2c2 {
>> +	status = "okay";
>> +
>> +	vdd_npu_s0: vdd_npu_mem_s0: regulator@42 {
>> +		compatible = "rockchip,rk8602";
>> +		reg = <0x42>;
>> +		fcs,suspend-voltage-selector = <1>;
>> +		regulator-name = "vdd_npu_s0";
>> +		regulator-always-on;
>> +		regulator-boot-on;
>> +		regulator-min-microvolt = <550000>;
>> +		regulator-max-microvolt = <950000>;
>> +		regulator-ramp-delay = <2300>;
>> +		vin-supply = <&vcc5v0_sys>;
>> +
>> +		regulator-state-mem {
>> +			regulator-off-in-suspend;
>> +		};
>> +	};
>> +};
>> +
>> +&i2c6 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&i2c6m3_xfer>;
>> +	status = "okay";
>> +
>> +	hym8563: rtc@51 {
>> +		compatible = "haoyu,hym8563";
>> +		reg = <0x51>;
>> +		#clock-cells = <0>;
>> +		clock-output-names = "hym8563";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&hym8563_int>;
>> +		interrupt-parent = <&gpio0>;
>> +		interrupts = <RK_PB0 IRQ_TYPE_LEVEL_LOW>;
>> +		wakeup-source;
>> +	};
>> +};
>> +
>> +&mdio1 {
>> +	rgmii_phy1: ethernet-phy@1 {
>> +		compatible = "ethernet-phy-ieee802.3-c22";
>> +		reg = <0x1>;
>> +	};
>> +};
>> +
>> +&pcie2x1l2 {
>> +	reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>;
>> +	vpcie3v3-supply = <&vcc3v3_pcie20>;
>> +	status = "okay";
>> +};
>> +
>> +&pinctrl {
>> +	gpio-func {
>> +		leds_gpio: leds-gpio {
>> +			rockchip,pins = <0 RK_PA2 RK_FUNC_GPIO &pcfg_pull_none>;
>> +		};
>> +	};
>> +
>> +	hym8563 {
>> +		hym8563_int: hym8563-int {
>> +			rockchip,pins = <0 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
>> +		};
>> +	};
>> +
>> +	usb-typec {
>> +		usbc0_int: usbc0-int {
>> +			rockchip,pins = <0 RK_PD3 RK_FUNC_GPIO &pcfg_pull_up>;
>> +		};
>> +
>> +		typec5v_pwren: typec5v-pwren {
>> +			rockchip,pins = <3 RK_PC0 RK_FUNC_GPIO &pcfg_pull_none>;
>> +		};
>> +	};
>> +};
>> +
>> +&saradc {
>> +	vref-supply = <&avcc_1v8_s0>;
>> +	status = "okay";
>> +};
>> +
>> +&sdmmc {
>> +	max-frequency = <150000000>;
>> +	no-sdio;
>> +	no-mmc;
>> +	bus-width = <4>;
>> +	cap-mmc-highspeed;
> 
> Is this useful for anything, when you have no-mmc specified?
> 
>> +	cap-sd-highspeed;
>> +	disable-wp;
>> +	sd-uhs-sdr104;
>> +	vmmc-supply = <&vcc_3v3_sd_s0>;
>> +	vqmmc-supply = <&vccio_sd_s0>;
>> +	status = "okay";
>> +};
>> +
> 
> With the above regulator issue fixed:
> 
> Reviewed-by: Ondřej Jirman <megi@xxxxxx>
> 
> (I reviewed just for schematic <-> DT correspondece)
> 
> kind regards,
> 	o.
> 
>> -- 
>> 2.41.0
>>

Regards,
Efe