Re: [PATCH v2 3/3] arm64: dts: rockchip: fix the pcie refclock oscillator on Rock 5 ITX

[Dragan Simic <dsimic@manjaro.org>]

Hello Anand,

On 2024-07-18 09:26, Anand Moon wrote:
> On Mon, 15 Jul 2024 at 16:35, Heiko Stuebner <heiko@sntech.de> wrote:
>> 
>> The Rock 5 ITX uses two PCIe controllers to drive both a M.2 slot and 
>> its
>> SATA controller with 2 lanes each. The supply for the refclk 
>> oscillator is
>> the same that supplies the M.2 slot, but the SATA controller port is
>> supplied by a different rail.
>> 
>> This leads to the effect that if the PCIe30x4 controller for the M.2
>> probes first, everything works normally. But if the PCIe30x2 
>> controller
>> that is connected to the SATA controller probes first, it will hang on
>> the first DBI read as nothing will have enabled the refclock before.
> 
> I just checked the rk3588-rock-5-itx.dts in the linux-next.
> You have not enabled sata0 and sata2, which might be the problem
> for the SATA controller not getting initialized.

Rock 5 ITX doesn't use RK5588's built-in SATA interfaces, so that's 
fine.
Please have a look at the board schematic, it uses a separate PCI 
Express
SATA controller for its four SATA ports.

>> Fix this by describing the clock generator with its supplies so that
>> both controllers can reference it as needed.
>> 
>> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> ---
>>  .../boot/dts/rockchip/rk3588-rock-5-itx.dts   | 38 
>> ++++++++++++++++++-
>>  1 file changed, 36 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts 
>> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
>> index d0b922b8d67e8..37bc53f2796fc 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5-itx.dts
>> @@ -72,6 +72,15 @@ hdd-led2 {
>>                 };
>>         };
>> 
>> +       /* Unnamed voltage oscillator: 100MHz,3.3V,3225 */
>> +       pcie30_port0_refclk: pcie30_port1_refclk: 
>> pcie-voltage-oscillator {
>> +               compatible = "voltage-oscillator";
>> +               #clock-cells = <0>;
>> +               clock-frequency = <100000000>;
>> +               clock-output-names = "pcie30_refclk";
>> +               vdd-supply = <&vcc3v3_pi6c_05>;
>> +       };
>> +
>>         fan0: pwm-fan {
>>                 compatible = "pwm-fan";
>>                 #cooling-cells = <2>;
>> @@ -146,13 +155,14 @@ vcc3v3_lan: vcc3v3_lan_phy2: 
>> regulator-vcc3v3-lan {
>>                 vin-supply = <&vcc_3v3_s3>;
>>         };
>> 
>> -       vcc3v3_mkey: regulator-vcc3v3-mkey {
>> +       /* The PCIE30x4_PWREN_H controls two regulators */
>> +       vcc3v3_mkey: vcc3v3_pi6c_05: regulator-vcc3v3-pi6c-05 {
>>                 compatible = "regulator-fixed";
>>                 enable-active-high;
>>                 gpios = <&gpio1 RK_PA4 GPIO_ACTIVE_HIGH>;
>>                 pinctrl-names = "default";
>>                 pinctrl-0 = <&pcie30x4_pwren_h>;
>> -               regulator-name = "vcc3v3_mkey";
>> +               regulator-name = "vcc3v3_pi6c_05";
>>                 regulator-min-microvolt = <3300000>;
>>                 regulator-max-microvolt = <3300000>;
>>                 startup-delay-us = <5000>;
>> @@ -513,6 +523,18 @@ &pcie30phy {
>> 
>>  /* ASMedia ASM1164 Sata controller */
>>  &pcie3x2 {
>> +       /*
>> +        * The board has a "pcie_refclk" oscillator that needs 
>> enabling,
>> +        * so add it to the list of clocks.
>> +        */
>> +       clocks = <&cru ACLK_PCIE_2L_MSTR>, <&cru ACLK_PCIE_2L_SLV>,
>> +                <&cru ACLK_PCIE_2L_DBI>, <&cru PCLK_PCIE_2L>,
>> +                <&cru CLK_PCIE_AUX1>, <&cru CLK_PCIE2L_PIPE>,
>> +                <&pcie30_port1_refclk>;
>> +       clock-names = "aclk_mst", "aclk_slv",
>> +                     "aclk_dbi", "pclk",
>> +                     "aux", "pipe",
>> +                     "ref";
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&pcie30x2_perstn_m1_l>;
>>         reset-gpios = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
>> @@ -522,6 +544,18 @@ &pcie3x2 {
>> 
>>  /* M.2 M.key */
>>  &pcie3x4 {
>> +       /*
>> +        * The board has a "pcie_refclk" oscillator that needs 
>> enabling,
>> +        * so add it to the list of clocks.
>> +        */
>> +       clocks = <&cru ACLK_PCIE_4L_MSTR>, <&cru ACLK_PCIE_4L_SLV>,
>> +                <&cru ACLK_PCIE_4L_DBI>, <&cru PCLK_PCIE_4L>,
>> +                <&cru CLK_PCIE_AUX0>, <&cru CLK_PCIE4L_PIPE>,
>> +                <&pcie30_port0_refclk>;
>> +       clock-names = "aclk_mst", "aclk_slv",
>> +                     "aclk_dbi", "pclk",
>> +                     "aux", "pipe",
>> +                     "ref";
>>         num-lanes = <2>;
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&pcie30x4_perstn_m1_l>;
>> --
>> 2.39.2