[PATCH v2 0/2] Slightly improve hardware description of Pine64 RockPro64

[PATCH v2 0/2] Slightly improve hardware description of Pine64 RockPro64

[Dragan Simic]

This is a small series that introduces small improvements to the way
Pine64 RockPro64 [1] single-board-computer is described in the DT files.
This applies to both production-run revisions of the RockPro64.

The introduced improvements boil down to eliminating some warnings from
the kernel log, by adding a previously undefined regulator and by adding
some previously missing references to the regulators.

The patch 2/2 also results in reliability improvements, by reportedly
resolving (some of) the longstanding issues resulting from using certain
PCI Express cards with the Pine64 RockPro64.

Changes in v2:
  - Dissolved the patch 2/2 notes into its description, according to the
    numerous power cycling/rebooting tests performed on a Pine64 RockPro64
    by @ChriChri and reported in the #rock64 channel on irc.pine64.org
  - Adjusted the cover letter to match the latest test results
  - Added "Fixes" and "Cc: stable" tags to both patches in the series
  - Added "Tested-by" to the patch 2/2, thanks to Chris' testing [2]
  - Added a few "Cc" tags to the patch 2/2, to make sure people involved
    in the earlier related patch [3] are in the loop

Link to v1: https://lore.kernel.org/linux-rockchip/cover.1740748439.git.dsimic@manjaro.org/T/#u

[1] https://wiki.pine64.org/wiki/ROCKPro64
[2] https://z9.de/hedgedoc/s/nF4d5G7rg#reboot-tests-for-PCIe-improvements
[3] https://lore.kernel.org/lkml/20230509153912.515218-1-vincenzopalazzodev@gmail.com/T/#u

Dragan Simic (2):
  arm64: dts: rockchip: Add avdd HDMI supplies to RockPro64 board dtsi
  arm64: dts: rockchip: Add missing PCIe supplies to RockPro64 board
    dtsi

 .../boot/dts/rockchip/rk3399-rockpro64.dtsi      | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

[PATCH v2 1/2] arm64: dts: rockchip: Add avdd HDMI supplies to RockPro64 board dtsi

[Dragan Simic]

Add missing "avdd-0v9-supply" and "avdd-1v8-supply" properties to the "hdmi"
node in the Pine64 RockPro64 board dtsi file.  To achieve this, also add the
associated "vcca_0v9" regulator that produces the 0.9 V supply, [1][2] which
hasn't been defined previously in the board dtsi file.

This also eliminates the following warnings from the kernel log:

  dwhdmi-rockchip ff940000.hdmi: supply avdd-0v9 not found, using dummy regulator
  dwhdmi-rockchip ff940000.hdmi: supply avdd-1v8 not found, using dummy regulator

There are no functional changes to the way board works with these additions,
because the "vcc1v8_dvp" and "vcca_0v9" regulators are always enabled, [1][2]
but these additions improve the accuracy of hardware description.

These changes apply to the both supported hardware revisions of the Pine64
RockPro64, i.e. to the production-run revisions 2.0 and 2.1. [1][2]

[1] https://files.pine64.org/doc/rockpro64/rockpro64_v21-SCH.pdf
[2] https://files.pine64.org/doc/rockpro64/rockpro64_v20-SCH.pdf

Fixes: e4f3fb490967 ("arm64: dts: rockchip: add initial dts support for Rockpro64")
Cc: stable@vger.kernel.org
Suggested-by: Diederik de Haas <didi.debian@cknow.org>
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
index 69a9d6170649..47dc198706c8 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
@@ -227,6 +227,16 @@ vcc5v0_usb: regulator-vcc5v0-usb {
 		vin-supply = <&vcc12v_dcin>;
 	};
 
+	vcca_0v9: regulator-vcca-0v9 {
+		compatible = "regulator-fixed";
+		regulator-name = "vcca_0v9";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <900000>;
+		regulator-max-microvolt = <900000>;
+		vin-supply = <&vcc3v3_sys>;
+	};
+
 	vdd_log: regulator-vdd-log {
 		compatible = "pwm-regulator";
 		pwms = <&pwm2 0 25000 1>;
@@ -312,6 +322,8 @@ &gmac {
 };
 
 &hdmi {
+	avdd-0v9-supply = <&vcca_0v9>;
+	avdd-1v8-supply = <&vcc1v8_dvp>;
 	ddc-i2c-bus = <&i2c3>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&hdmi_cec>;

[PATCH v2 2/2] arm64: dts: rockchip: Add missing PCIe supplies to RockPro64 board dtsi

[Dragan Simic]

Add missing "vpcie0v9-supply" and "vpcie1v8-supply" properties to the "pcie0"
node in the Pine64 RockPro64 board dtsi file.  This eliminates the following
warnings from the kernel log:

  rockchip-pcie f8000000.pcie: supply vpcie1v8 not found, using dummy regulator
  rockchip-pcie f8000000.pcie: supply vpcie0v9 not found, using dummy regulator

These additions improve the accuracy of hardware description of the RockPro64
and, in theory, they should result in no functional changes to the way board
works after the changes, because the "vcca_0v9" and "vcca_1v8" regulators are
always enabled. [1][2]  However, extended reliability testing, performed by
Chris, [3] has proven that the age-old issues with some PCI Express cards,
when used with a Pine64 RockPro64, are also resolved.

Those issues were already mentioned in the commit 43853e843aa6 (arm64: dts:
rockchip: Remove unsupported node from the Pinebook Pro dts, 2024-04-01),
together with a brief description of the out-of-tree enumeration delay patch
that reportedly resolves those issues.  In a nutshell, booting a RockPro64
with some PCI Express cards attached to it caused a kernel oops. [4]

Symptomatically enough, to the commit author's best knowledge, only the Pine64
RockPro64, out of all RK3399-based boards and devices supported upstream, has
been reported to suffer from those PCI Express issues, and only the RockPro64
had some of the PCI Express supplies missing in its DT.  Thus, perhaps some
weird timing issues exist that caused the "vcca_1v8" always-on regulator,
which is part of the RK808 PMIC, to actually not be enabled before the PCI
Express is initialized and enumerated on the RockPro64, causing oopses with
some PCIe cards, and the aforementioned enumeration delay patch [4] probably
acted as just a workaround for the underlying timing issue.

Admittedly, the Pine64 RockPro64 is a bit specific board by having a standard
PCI Express slot, allowing use of various standard cards, but pretty much
standard PCI Express cards have been attached to other RK3399 boards as well,
and the commit author is unaware ot such issues reported for them.

It's quite hard to be sure that the PCI Express issues are fully resolved by
these additions to the DT, without some really extensive and time-consuming
testing.  However, these additions to the DT can result in good things and
improvements anyway, making them perfectly safe from the standpoint of being
unable to do any harm or cause some unforeseen regressions.

Shuffle and reorder the "vpcie*-supply" properties a bit, so they're sorted
alphanumerically, which is a bit more logical and more useful than having
these properties listed in their strict alphabetical order.

These changes apply to the both supported hardware revisions of the Pine64
RockPro64, i.e. to the production-run revisions 2.0 and 2.1. [1][2]

[1] https://files.pine64.org/doc/rockpro64/rockpro64_v21-SCH.pdf
[2] https://files.pine64.org/doc/rockpro64/rockpro64_v20-SCH.pdf
[3] https://z9.de/hedgedoc/s/nF4d5G7rg#reboot-tests-for-PCIe-improvements
[4] https://lore.kernel.org/lkml/20230509153912.515218-1-vincenzopalazzodev@gmail.com/T/#u

Fixes: bba821f5479e ("arm64: dts: rockchip: add PCIe nodes on rk3399-rockpro64")
Cc: stable@vger.kernel.org
Cc: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Cc: Peter Geis <pgwipeout@gmail.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>
Reported-by: Diederik de Haas <didi.debian@cknow.org>
Tested-by: Chris Vogel <chris@z9.de>
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
index 47dc198706c8..41ee381ff81f 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
@@ -673,8 +673,10 @@ &pcie0 {
 	num-lanes = <4>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie_perst>;
-	vpcie12v-supply = <&vcc12v_dcin>;
+	vpcie0v9-supply = <&vcca_0v9>;
+	vpcie1v8-supply = <&vcca_1v8>;
 	vpcie3v3-supply = <&vcc3v3_pcie>;
+	vpcie12v-supply = <&vcc12v_dcin>;
 	status = "okay";
 };
 

Re: [PATCH v2 2/2] arm64: dts: rockchip: Add missing PCIe supplies to RockPro64 board dtsi

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 5616 bytes --]

Hi Dragan,

On Sun Mar 2, 2025 at 7:48 PM CET, Dragan Simic wrote:
> Add missing "vpcie0v9-supply" and "vpcie1v8-supply" properties to the "pcie0"
> node in the Pine64 RockPro64 board dtsi file.  This eliminates the following
> warnings from the kernel log:
>
>   rockchip-pcie f8000000.pcie: supply vpcie1v8 not found, using dummy regulator
>   rockchip-pcie f8000000.pcie: supply vpcie0v9 not found, using dummy regulator
>
> These additions improve the accuracy of hardware description of the RockPro64
> and, in theory, they should result in no functional changes to the way board
> works after the changes, because the "vcca_0v9" and "vcca_1v8" regulators are
> always enabled. [1][2]  However, extended reliability testing, performed by
> Chris, [3] has proven that the age-old issues with some PCI Express cards,
> when used with a Pine64 RockPro64, are also resolved.

Thanks for this patch :-)

I 'reported' the issue based on the logs I saw from other people, but
now that I have a RockPro64 (version 2.1) myself, I can confirm that
without this patch I saw those warnings myself.

When booted up without a PCIe card in the slot, I also saw these errors:
[    5.106650] rockchip-pcie f8000000.pcie: PCIe link training gen1 timeout!
[    5.107373] rockchip-pcie f8000000.pcie: probe with driver rockchip-pcie failed with error -110

You indicated that that was due to there being no PCIe card inserted.

After applying this patch I booted up my RockPro64 and found that the
above mentioned warnings are indeed gone.

Then I inserted a Renesas Electronics Corp. uPD720201 USB 3.0 Host
Controller in the PCIe slot and booted up.
The above mentioned errors were indeed gone.
Furthermore ``lspci`` showed that card and when I plugged in an USB 3
drive in one of the ports, it was correctly detected and I could mount
the partition on my system. So feel free to include:

Tested-by: Diederik de Haas <didi.debian@cknow.org>

Cheers,
  Diederik

> Those issues were already mentioned in the commit 43853e843aa6 (arm64: dts:
> rockchip: Remove unsupported node from the Pinebook Pro dts, 2024-04-01),
> together with a brief description of the out-of-tree enumeration delay patch
> that reportedly resolves those issues.  In a nutshell, booting a RockPro64
> with some PCI Express cards attached to it caused a kernel oops. [4]
>
> Symptomatically enough, to the commit author's best knowledge, only the Pine64
> RockPro64, out of all RK3399-based boards and devices supported upstream, has
> been reported to suffer from those PCI Express issues, and only the RockPro64
> had some of the PCI Express supplies missing in its DT.  Thus, perhaps some
> weird timing issues exist that caused the "vcca_1v8" always-on regulator,
> which is part of the RK808 PMIC, to actually not be enabled before the PCI
> Express is initialized and enumerated on the RockPro64, causing oopses with
> some PCIe cards, and the aforementioned enumeration delay patch [4] probably
> acted as just a workaround for the underlying timing issue.
>
> Admittedly, the Pine64 RockPro64 is a bit specific board by having a standard
> PCI Express slot, allowing use of various standard cards, but pretty much
> standard PCI Express cards have been attached to other RK3399 boards as well,
> and the commit author is unaware ot such issues reported for them.
>
> It's quite hard to be sure that the PCI Express issues are fully resolved by
> these additions to the DT, without some really extensive and time-consuming
> testing.  However, these additions to the DT can result in good things and
> improvements anyway, making them perfectly safe from the standpoint of being
> unable to do any harm or cause some unforeseen regressions.
>
> Shuffle and reorder the "vpcie*-supply" properties a bit, so they're sorted
> alphanumerically, which is a bit more logical and more useful than having
> these properties listed in their strict alphabetical order.
>
> These changes apply to the both supported hardware revisions of the Pine64
> RockPro64, i.e. to the production-run revisions 2.0 and 2.1. [1][2]
>
> [1] https://files.pine64.org/doc/rockpro64/rockpro64_v21-SCH.pdf
> [2] https://files.pine64.org/doc/rockpro64/rockpro64_v20-SCH.pdf
> [3] https://z9.de/hedgedoc/s/nF4d5G7rg#reboot-tests-for-PCIe-improvements
> [4] https://lore.kernel.org/lkml/20230509153912.515218-1-vincenzopalazzodev@gmail.com/T/#u
>
> Fixes: bba821f5479e ("arm64: dts: rockchip: add PCIe nodes on rk3399-rockpro64")
> Cc: stable@vger.kernel.org
> Cc: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
> Cc: Peter Geis <pgwipeout@gmail.com>
> Cc: Bjorn Helgaas <helgaas@kernel.org>
> Reported-by: Diederik de Haas <didi.debian@cknow.org>
> Tested-by: Chris Vogel <chris@z9.de>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>  arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> index 47dc198706c8..41ee381ff81f 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> @@ -673,8 +673,10 @@ &pcie0 {
>  	num-lanes = <4>;
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pcie_perst>;
> -	vpcie12v-supply = <&vcc12v_dcin>;
> +	vpcie0v9-supply = <&vcca_0v9>;
> +	vpcie1v8-supply = <&vcca_1v8>;
>  	vpcie3v3-supply = <&vcc3v3_pcie>;
> +	vpcie12v-supply = <&vcc12v_dcin>;
>  	status = "okay";
>  };
>  


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] arm64: dts: rockchip: Add avdd HDMI supplies to RockPro64 board dtsi

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 3057 bytes --]

Hi Dragan,

On Sun Mar 2, 2025 at 7:48 PM CET, Dragan Simic wrote:
> Add missing "avdd-0v9-supply" and "avdd-1v8-supply" properties to the "hdmi"
> node in the Pine64 RockPro64 board dtsi file.  To achieve this, also add the
> associated "vcca_0v9" regulator that produces the 0.9 V supply, [1][2] which
> hasn't been defined previously in the board dtsi file.
>
> This also eliminates the following warnings from the kernel log:
>
>   dwhdmi-rockchip ff940000.hdmi: supply avdd-0v9 not found, using dummy regulator
>   dwhdmi-rockchip ff940000.hdmi: supply avdd-1v8 not found, using dummy regulator

Also thanks for this patch :-)

I've now also confirmed for myself that without this patch I see the
above warnings and I don't see them with this patch applied.

When booting up I saw an U-Boot logo (hadn't seen that one before) and
not longer after that I saw the various boot messages come by on the
monitor I had connected to the HDMI port. This is only a *very*
barebones install (yet), so I couldn't do 'deeper' tests, but HDMI
appears to work. So feel free to include:

Tested-by: Diederik de Haas <didi.debian@cknow.org>

Cheers,
  Diederik
>
> There are no functional changes to the way board works with these additions,
> because the "vcc1v8_dvp" and "vcca_0v9" regulators are always enabled, [1][2]
> but these additions improve the accuracy of hardware description.
>
> These changes apply to the both supported hardware revisions of the Pine64
> RockPro64, i.e. to the production-run revisions 2.0 and 2.1. [1][2]
>
> [1] https://files.pine64.org/doc/rockpro64/rockpro64_v21-SCH.pdf
> [2] https://files.pine64.org/doc/rockpro64/rockpro64_v20-SCH.pdf
>
> Fixes: e4f3fb490967 ("arm64: dts: rockchip: add initial dts support for Rockpro64")
> Cc: stable@vger.kernel.org
> Suggested-by: Diederik de Haas <didi.debian@cknow.org>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>  arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> index 69a9d6170649..47dc198706c8 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> @@ -227,6 +227,16 @@ vcc5v0_usb: regulator-vcc5v0-usb {
>  		vin-supply = <&vcc12v_dcin>;
>  	};
>  
> +	vcca_0v9: regulator-vcca-0v9 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcca_0v9";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <900000>;
> +		regulator-max-microvolt = <900000>;
> +		vin-supply = <&vcc3v3_sys>;
> +	};
> +
>  	vdd_log: regulator-vdd-log {
>  		compatible = "pwm-regulator";
>  		pwms = <&pwm2 0 25000 1>;
> @@ -312,6 +322,8 @@ &gmac {
>  };
>  
>  &hdmi {
> +	avdd-0v9-supply = <&vcca_0v9>;
> +	avdd-1v8-supply = <&vcc1v8_dvp>;
>  	ddc-i2c-bus = <&i2c3>;
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&hdmi_cec>;


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 0/2] Slightly improve hardware description of Pine64 RockPro64

[Heiko Stuebner]

On Sun, 02 Mar 2025 19:48:02 +0100, Dragan Simic wrote:
> This is a small series that introduces small improvements to the way
> Pine64 RockPro64 [1] single-board-computer is described in the DT files.
> This applies to both production-run revisions of the RockPro64.
> 
> The introduced improvements boil down to eliminating some warnings from
> the kernel log, by adding a previously undefined regulator and by adding
> some previously missing references to the regulators.
> 
> [...]

Applied, thanks!

[1/2] arm64: dts: rockchip: Add avdd HDMI supplies to RockPro64 board dtsi
      commit: bd1c959f37f384b477f51572331b0dc828bd009a
[2/2] arm64: dts: rockchip: Add missing PCIe supplies to RockPro64 board dtsi
      commit: 64ef4a4320e7aa3f0f267e01f170f52b90bf0b1b

I've moved the pcie12v supply up one line.
While in a mathematical sense it's true 12 > 3.3, we're sorting
alphabetical, so it's 1?? < 3?? .

And yes I sympathize with 3.3 < 12, but also have come to appreciate not
having overly many special cases :-)


Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>

Re: [PATCH v2 0/2] Slightly improve hardware description of Pine64 RockPro64

[Dragan Simic]

Hello Heiko,

On 2025-03-03 23:36, Heiko Stuebner wrote:
> On Sun, 02 Mar 2025 19:48:02 +0100, Dragan Simic wrote:
>> This is a small series that introduces small improvements to the way
>> Pine64 RockPro64 [1] single-board-computer is described in the DT 
>> files.
>> This applies to both production-run revisions of the RockPro64.
>> 
>> The introduced improvements boil down to eliminating some warnings 
>> from
>> the kernel log, by adding a previously undefined regulator and by 
>> adding
>> some previously missing references to the regulators.
>> 
>> [...]
> 
> Applied, thanks!
> 
> [1/2] arm64: dts: rockchip: Add avdd HDMI supplies to RockPro64 board 
> dtsi
>       commit: bd1c959f37f384b477f51572331b0dc828bd009a
> [2/2] arm64: dts: rockchip: Add missing PCIe supplies to RockPro64 
> board dtsi
>       commit: 64ef4a4320e7aa3f0f267e01f170f52b90bf0b1b
> 
> I've moved the pcie12v supply up one line.
> While in a mathematical sense it's true 12 > 3.3, we're sorting
> alphabetical, so it's 1?? < 3?? .
> 
> And yes I sympathize with 3.3 < 12, but also have come to appreciate 
> not
> having overly many special cases :-)

Great, thanks! :)

I'm fine with the alphabetical ordering, albeit with some caveats
described below, but the following part of the patch description
should also be removed, if possible, so the patch description fully
matches the introduced changes:

   Shuffle and reorder the "vpcie*-supply" properties a bit, so they're 
sorted
   alphanumerically, which is a bit more logical and more useful than 
having
   these properties listed in their strict alphabetical order.

Admittedly, the patch itself indeed looks nicer [1] with the
alphabetical ordering, :) (which is quasi-alphabetical, actually,
please see below) but it might be better to have the DT guidelines
adjusted to specify alphanumerical ordering as well.  Even ls(1)
can sort filenames alphanumerically, with its "-v" option that
performs "natural sort of (version) numbers within text". [2]

The real trouble is that, if we were to apply the alphabetical
ordering strictly, we'd end up with the following order of node
properties in which 12 is before 1.8 and 3.3, which IMHO doesn't
make much sense:

   vpcie0v9-supply = <&vcca_0v9>;
   vpcie12v-supply = <&vcc12v_dcin>;
   vpcie1v8-supply = <&vcca_1v8>;
   vpcie3v3-supply = <&vcc3v3_pcie>;

I'm hoping you'll agree that specifying alphanumerical ordering
for the properties in the DTS coding style is the way to go, just
like it's already specified for the ordering of the nodes.  I'll
go ahead and submit an appropriate patch for the DT guidelines.

[1] 
https://web.git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?id=64ef4a4320e7aa3f0f267e01f170f52b90bf0b1b
[2] https://man.archlinux.org/man/ls.1.en#v

Re: [PATCH v2 0/2] Slightly improve hardware description of Pine64 RockPro64

[Dragan Simic]

On 2025-03-04 07:44, Dragan Simic wrote:
> On 2025-03-03 23:36, Heiko Stuebner wrote:
>> On Sun, 02 Mar 2025 19:48:02 +0100, Dragan Simic wrote:
>>> This is a small series that introduces small improvements to the way
>>> Pine64 RockPro64 [1] single-board-computer is described in the DT 
>>> files.
>>> This applies to both production-run revisions of the RockPro64.
>>> 
>>> The introduced improvements boil down to eliminating some warnings 
>>> from
>>> the kernel log, by adding a previously undefined regulator and by 
>>> adding
>>> some previously missing references to the regulators.
>>> 
>>> [...]
>> 
>> Applied, thanks!
>> 
>> [1/2] arm64: dts: rockchip: Add avdd HDMI supplies to RockPro64 board 
>> dtsi
>>       commit: bd1c959f37f384b477f51572331b0dc828bd009a
>> [2/2] arm64: dts: rockchip: Add missing PCIe supplies to RockPro64 
>> board dtsi
>>       commit: 64ef4a4320e7aa3f0f267e01f170f52b90bf0b1b
>> 
>> I've moved the pcie12v supply up one line.
>> While in a mathematical sense it's true 12 > 3.3, we're sorting
>> alphabetical, so it's 1?? < 3?? .
>> 
>> And yes I sympathize with 3.3 < 12, but also have come to appreciate 
>> not
>> having overly many special cases :-)
> 
> Great, thanks! :)
> 
> I'm fine with the alphabetical ordering, albeit with some caveats
> described below, but the following part of the patch description
> should also be removed, if possible, so the patch description fully
> matches the introduced changes:
> 
>   Shuffle and reorder the "vpcie*-supply" properties a bit, so they're 
> sorted
>   alphanumerically, which is a bit more logical and more useful than 
> having
>   these properties listed in their strict alphabetical order.
> 
> Admittedly, the patch itself indeed looks nicer [1] with the
> alphabetical ordering, :) (which is quasi-alphabetical, actually,
> please see below) but it might be better to have the DT guidelines
> adjusted to specify alphanumerical ordering as well.  Even ls(1)
> can sort filenames alphanumerically, with its "-v" option that
> performs "natural sort of (version) numbers within text". [2]
> 
> The real trouble is that, if we were to apply the alphabetical
> ordering strictly, we'd end up with the following order of node
> properties in which 12 is before 1.8 and 3.3, which IMHO doesn't
> make much sense:
> 
>   vpcie0v9-supply = <&vcca_0v9>;
>   vpcie12v-supply = <&vcc12v_dcin>;
>   vpcie1v8-supply = <&vcca_1v8>;
>   vpcie3v3-supply = <&vcc3v3_pcie>;
> 
> I'm hoping you'll agree that specifying alphanumerical ordering
> for the properties in the DTS coding style is the way to go, just
> like it's already specified for the ordering of the nodes.  I'll
> go ahead and submit an appropriate patch for the DT guidelines.
> 
> [1] 
> https://web.git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?id=64ef4a4320e7aa3f0f267e01f170f52b90bf0b1b
> [2] https://man.archlinux.org/man/ls.1.en#v

For future reference and for anyone interested, here's a link to
the above-mentioned DTS coding style patch:

https://lore.kernel.org/linux-kernel/09d6f2fc111b3d6e58987336944f93ec36b65118.1741071107.git.dsimic@manjaro.org/T/#u

Re: [PATCH v2 0/2] Slightly improve hardware description of Pine64 RockPro64

[Heiko Stübner]

Am Dienstag, 4. März 2025, 07:44:59 MEZ schrieb Dragan Simic:
> Hello Heiko,
> 
> On 2025-03-03 23:36, Heiko Stuebner wrote:
> > On Sun, 02 Mar 2025 19:48:02 +0100, Dragan Simic wrote:
> >> This is a small series that introduces small improvements to the way
> >> Pine64 RockPro64 [1] single-board-computer is described in the DT 
> >> files.
> >> This applies to both production-run revisions of the RockPro64.
> >> 
> >> The introduced improvements boil down to eliminating some warnings 
> >> from
> >> the kernel log, by adding a previously undefined regulator and by 
> >> adding
> >> some previously missing references to the regulators.
> >> 
> >> [...]
> > 
> > Applied, thanks!
> > 
> > [1/2] arm64: dts: rockchip: Add avdd HDMI supplies to RockPro64 board 
> > dtsi
> >       commit: bd1c959f37f384b477f51572331b0dc828bd009a
> > [2/2] arm64: dts: rockchip: Add missing PCIe supplies to RockPro64 
> > board dtsi
> >       commit: 64ef4a4320e7aa3f0f267e01f170f52b90bf0b1b
> > 
> > I've moved the pcie12v supply up one line.
> > While in a mathematical sense it's true 12 > 3.3, we're sorting
> > alphabetical, so it's 1?? < 3?? .
> > 
> > And yes I sympathize with 3.3 < 12, but also have come to appreciate 
> > not
> > having overly many special cases :-)
> 
> Great, thanks! :)
> 
> I'm fine with the alphabetical ordering, albeit with some caveats
> described below, but the following part of the patch description
> should also be removed, if possible, so the patch description fully
> matches the introduced changes:
> 
>    Shuffle and reorder the "vpcie*-supply" properties a bit, so they're 
> sorted
>    alphanumerically, which is a bit more logical and more useful than 
> having
>    these properties listed in their strict alphabetical order.

I've amended the commit, dropping this block

> I'm hoping you'll agree that specifying alphanumerical ordering
> for the properties in the DTS coding style is the way to go, just
> like it's already specified for the ordering of the nodes.  I'll
> go ahead and submit an appropriate patch for the DT guidelines.

vpcie0v9-supply = <&vcca_0v9>;
vpcie1v8-supply = <&vcca_1v8>;
vpcie3v3-supply = <&vcc3v3_pcie>;
vpcie12v-supply = <&vcc12v_dcin>;

In the end I don't care _that_ much, but personally I find that
alphanumerical ordering harder to read ;-) .

Because in the example above, my mind now constantly shouts
"why is vpcie1... after vpcie3... ..... ooooh right, it's alpha-numerical"

But I can live with it I guess ;-) .
As 3.3 is smaller than 12 afterall.


Heiko

Re: [PATCH v2 0/2] Slightly improve hardware description of Pine64 RockPro64

[Dragan Simic]

Hello Heiko,

On 2025-03-04 22:52, Heiko Stübner wrote:
> Am Dienstag, 4. März 2025, 07:44:59 MEZ schrieb Dragan Simic:
>> On 2025-03-03 23:36, Heiko Stuebner wrote:
>> > On Sun, 02 Mar 2025 19:48:02 +0100, Dragan Simic wrote:
>> >> This is a small series that introduces small improvements to the way
>> >> Pine64 RockPro64 [1] single-board-computer is described in the DT
>> >> files.
>> >> This applies to both production-run revisions of the RockPro64.
>> >>
>> >> The introduced improvements boil down to eliminating some warnings
>> >> from
>> >> the kernel log, by adding a previously undefined regulator and by
>> >> adding
>> >> some previously missing references to the regulators.
>> >>
>> >> [...]
>> >
>> > Applied, thanks!
>> >
>> > [1/2] arm64: dts: rockchip: Add avdd HDMI supplies to RockPro64 board
>> > dtsi
>> >       commit: bd1c959f37f384b477f51572331b0dc828bd009a
>> > [2/2] arm64: dts: rockchip: Add missing PCIe supplies to RockPro64
>> > board dtsi
>> >       commit: 64ef4a4320e7aa3f0f267e01f170f52b90bf0b1b
>> >
>> > I've moved the pcie12v supply up one line.
>> > While in a mathematical sense it's true 12 > 3.3, we're sorting
>> > alphabetical, so it's 1?? < 3?? .
>> >
>> > And yes I sympathize with 3.3 < 12, but also have come to appreciate
>> > not
>> > having overly many special cases :-)
>> 
>> Great, thanks! :)
>> 
>> I'm fine with the alphabetical ordering, albeit with some caveats
>> described below, but the following part of the patch description
>> should also be removed, if possible, so the patch description fully
>> matches the introduced changes:
>> 
>>    Shuffle and reorder the "vpcie*-supply" properties a bit, so 
>> they're
>> sorted
>>    alphanumerically, which is a bit more logical and more useful than
>> having
>>    these properties listed in their strict alphabetical order.
> 
> I've amended the commit, dropping this block

Thanks!

>> I'm hoping you'll agree that specifying alphanumerical ordering
>> for the properties in the DTS coding style is the way to go, just
>> like it's already specified for the ordering of the nodes.  I'll
>> go ahead and submit an appropriate patch for the DT guidelines.
> 
> vpcie0v9-supply = <&vcca_0v9>;
> vpcie1v8-supply = <&vcca_1v8>;
> vpcie3v3-supply = <&vcc3v3_pcie>;
> vpcie12v-supply = <&vcc12v_dcin>;
> 
> In the end I don't care _that_ much, but personally I find that
> alphanumerical ordering harder to read ;-) .
> 
> Because in the example above, my mind now constantly shouts
> "why is vpcie1... after vpcie3... ..... ooooh right, it's 
> alpha-numerical"
> 
> But I can live with it I guess ;-) .
> As 3.3 is smaller than 12 afterall.

Oh, I know very well first-hand how alphanumerical ordering may
look and feel strange at first... :)

I used ls(1) without the "-v" option for many, many years, and the
things looked a bit strange after I decided to add "-v" to its set
of options at some point, but after a short period of getting used
to it, alphanumerical sorting became so logical and useful to me that
alphabetical ordering is actually now harder to read and is looking
a bit strange to me. :)