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

[Muhammed Efe Cetin]

Hi, Ondřej

On 17.08.2023 16:57, Ondřej Jirman wrote:
> Hi Muhammed,
> 
> On Thu, Aug 17, 2023 at 04:33:55PM +0300, Muhammed Efe Cetin wrote:
>>
>> Hi, Ondřej
>>
>> Thanks for reviews, i'll fix them soon.
>>
>> On 15.08.2023 19:39, Ondřej Jirman wrote:
>>> Hello Muhammed,
>>>
>>>> [...]
>>>> +
>>>> +	adc-keys {
>>>> +		compatible = "adc-keys";
>>>> +		io-channels = <&saradc 1>;
>>>> +		io-channel-names = "buttons";
>>>> +		keyup-threshold-microvolt = <1800000>;
>>>> +		poll-interval = <100>;
>>>> +
>>>> +		vol-up-key {
>>>> +			label = "volume up";
>>>> +			linux,code = <KEY_VOLUMEUP>;
>>>> +			press-threshold-microvolt = <17000>;
>>>> +		};
>>>> +
>>>> +		vol-down-key {
>>>> +			label = "volume down";
>>>> +			linux,code = <KEY_VOLUMEDOWN>;
>>>> +			press-threshold-microvolt = <417000>;
>>>> +		};
>>>> +
>>>> +		menu-key {
>>>> +			label = "menu";
>>>> +			linux,code = <KEY_MENU>;
>>>> +			press-threshold-microvolt = <890000>;
>>>> +		};
>>>> +
>>>> +		back-key {
>>>> +			label = "back";
>>>> +			linux,code = <KEY_BACK>;
>>>> +			press-threshold-microvolt = <1235000>;
>>>
>>> None of these 4 buttons are in the scehmatic. There's one recovery button hooked
>>> to saradc 0, instead.
>>
>> Will remove these too. I won't add recovery button because of it's mostly used for rockusb.
> 
> Recovery button is useful from userspace, too. Please keep it. You already have
> the DT nodes, please just reduce them to one node for the recovery key.

Ok i will add it as KEY_VENDOR button.

> 
>>>> [...]
>>>
>>>> +	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>;
>>>> +	};
>>>
>>> There's no such regulator on the board.
>>
>> It's connected to PMIC https://i.imgur.com/sVJdC5K.png
> 
> It's not a separate fixed regulator. It's a PMIC output from buck6 https://megous.com/dl/tmp/8630fa17407c75b9.png
> 

I think it should be fixed regulator. It's used as vcc13-supply and vcc14-supply regulator on PMIC and it's same as other rk3588 boards.

> So this is VDD2_DDR_S3. If you want to keep the alias, just add extra alias to
> dcdc-reg6 like this:
> 
> 		...
> 	vcc_1v1_nldo_s3: vdd2_ddr_s3: dcdc-reg6 {
> 		...
> 
>>>
>>>> +	vbus5v0_typec: vbus5v0-typec-regulator {
>>>> +		compatible = "regulator-fixed";
>>>> +		regulator-name = "vbus5v0_typec";
>>>> +		regulator-min-microvolt = <5000000>;
>>>> +		regulator-max-microvolt = <5000000>;
>>>> +		enable-active-high;
>>>> +		gpio = <&gpio3 RK_PC0 GPIO_ACTIVE_HIGH>;
>>>> +		vin-supply = <&vcc5v0_usb>;
>>>> +		pinctrl-names = "default";
>>>> +		pinctrl-0 = <&typec5v_pwren>;
>>>> +	};
>>>
>>> No such power rail on the board. This should be VBUS_TYPEC power rail.
>>> Please name regulators/power rails as they are named in the schematic.
>>> Otherwise it's impossible to cross-check DT against schematic.
>>
>> Perhaps i can add it as a comment line. Otherwise the current name seems OK to
>> me but still open to suggestions.
> 
> Please just use the name that's in the schematic, even if it's subotpimal. Your
> comment will not be seen in debugfs/sysfs and other places, where it's also
> useful for debugging.

Ok i will make this one "vbus_typec"

> 
>>>
>>> No such regulator on the board. You might have meant VCC3V3_PCIE30,
>>> or not?
>>
>> Yes. However their naming in schematics is pretty nonsense. The board doesn't
>> have PCIe3, i think current naming is more appropriate.
> 
> Same as above. It's more important to be able to match DT/runtime debug outputs
> and schematic than having a subjectively more meaningful name which is not used
> anywhere in the documentation.

I agree with you but VCC3V3_PCIE30 is really confusing regulator name. I guess Xunlong made a mistake about it. I think we can name it as "vcc3v3_pcie20"

> 
>>>> +	sata {
>>>> +		sata_reset:sata-reset{
>>>> +			rockchip,pins = <3 RK_PD1 RK_FUNC_GPIO &pcfg_pull_up>;
>>>
>>> Not what you think: https://megous.com/dl/tmp/f135580b68cb5dfe.png
>>>
>>>> +		};
>>>> +	};
>>>> +};
>>>> +
>>>> +&pwm2 {
>>>> +	pinctrl-names = "active";
>>>> +	pinctrl-0 = <&pwm2m0_pins>;
>>>> +	status = "okay";
>>>> +};
>>>> +
>>>> +&pwm6 {
>>>> +	pinctrl-names = "active";
>>>> +	pinctrl-0 = <&pwm6m0_pins>;
>>>> +	status = "okay";
>>>> +};
>>>> +
>>>> +&saradc {
>>>> +	vref-supply = <&avcc_1v8_s0>;
>>>> +	status = "okay";
>>>> +};
>>>> +
>>>> +&sata0 {
>>>> +	pinctrl-names = "default";
>>>> +	pinctrl-0 = <&sata_reset>;
>>>> +	status = "disabled";
>>>> +};
>>>
>>> Where do you see a SATA port? http://www.orangepi.org/html/hardWare/computerAndMicrocontrollers/details/Orange-Pi-5.html
>>>
>>> Or a reset signal in the schematic?
>>
>> It's needed to use sata node once you want to use mSata SSD. I don't know if
>> it works without sata_reset pinctrl. Don't have mSata SSD to try.
> 
> Ok, that makes sense. It would be better to add this once you can test it, though.
> And if there's no way to switch between SATA and PCIe at runtime, it's probably
> just another thing for an overlay which would enable/disable/add all the appropriate
> DT nodes.

Yeah you are right.

> 
> thank you,
> 	o.

Regards,
Efe