rockchip: Fix several DT validation errors

This is a set of 4 small device-tree validation fixes.

Patch 1 adds the power-domains property to the csi dphy node on rk356x. Patch 2 removes the 2nd interrupt from the hdmi node on rk3328. Patch 3 replaces 'wake' with 'wakeup' on PineNote BT node. Patch 4 replaces 'reset-gpios' with 'shutdown-gpios' on brcm BT nodes.

Changes in v2:
- Added 'Fixes' tag to all commits
- patch 3: Dropped a wakeup fix as Heiko's patch already covered that
- patch 4: Extended commit description to explain why the replacement of 'reset' with 'shutdown' is safe

Diederik de Haas (4):
arm64: dts: rockchip: Add PD to csi dphy node on rk356x
arm64: dts: rockchip: Remove hdmi's 2nd interrupt on rk3328
arm64: dts: rockchip: Fix wakeup prop names on PineNote BT node
arm64: dts: rockchip: Fix reset-gpios property on brcm BT nodes

 arch/arm64/boot/dts/rockchip/rk3328.dtsi           | 3 +--
 arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi  | 6 +++---
 arch/arm64/boot/dts/rockchip/rk3566-radxa-cm3.dtsi | 2 +-
 arch/arm64/boot/dts/rockchip/rk356x.dtsi           | 1 +
 4 files changed, 6 insertions(+), 6 deletions(-)

2.45.2

The "rockchip-inno-csi-dphy.yaml" binding requires the power-domains property. According to RK3568 TRM Part 1 section 7.3 (page 475) the CSIHOST is placed in the PD_VI power domain.
So set the csi_dphy node power-domains property accordingly.

---
changes in v2:
- No change

 arch/arm64/boot/dts/rockchip/rk356x.dtsi | 1 +
 1 file changed, 1 insertion(+)

index 0ee0ada6f0ab..d581170914f9 100644
@@ -1790,6 +1790,7 @@ csi_dphy: phy@fe870000 {
 		clocks = <&cru PCLK_MIPICSIPHY>;
 		clock-names = "pclk";
 		#phy-cells = <0>;
+		power-domains = <&power RK3568_PD_VI>;
 		resets = <&cru SRST_P_MIPICSIPHY>;
 		reset-names = "apb";
 		rockchip,grf = <&grf>;

2.45.2

Hi Diederik,

Thanks for the patch. However, I am not sure about this one.

The CSI host sure is in this power domain, but we are talking about the CSI PHY here, right? According to the table CSIPHY is part of the power domain "ALIVE", which leads me to believe that the power domain is not necessary here. However, I guess you could put "RK3568_PD_LOGIC_ALIVE" here.

It should be noted, though, that I still haven't figured out what the role of this CSI host actually is. I know that the RK3568 ISP has its own MIPI CSI host controller within its register space. But I can only guess right now that this CSI host is somehow linked to the RK3568 VICAP, which is also capable of receiving MIPI CSI. Maybe we can leave this up to however brings up the RK3568 VICAP MIPI CSI feature :-)

Best regards,
Michael

Hi Michael,

Thanks for your reply.

It indeed isn't as clear cut as I want(ed) it to be.

Given that you're also the author of commit 29c99fb085ad ("phy:
rockchip: add support for the rk356x variant to rockchip-inno-csidphy"), which added support to the driver part, your opinion should have more weight then mine (IMO).

Nonetheless, I collected some extra data points:
- The TRM part 1 makes several mentions of 'dphy' in a block describing 'GRF_VI_CON1' (page 381 & 382). The above mentioned commit only added 'PHY_REG' for 'CON0', which I assume was deliberate given your response above. But 'GRF_VI_CON1' does have 'VI' in its name
- In rk356x.dtsi there are 'dsi_dphy0' and 'dsi_dphy1' which have 'RK3568_PD_VO' in their 'power-domains' property. Page 475 has 'DSIHOST' in 'PD_VO', while 'DSIPHY' is (also) in the 'ALIVE' power domain. So to be consistent it seems to me that 'csi_dphy' should be in 'PD_VI' or the 'dsi_dphy' nodes should be placed/moved to 'RK3568_PD_LOGIC_ALIVE'.
- Of all the compatibles from the binding, I only found 'rockchip,px30-csi-dphy' referenced in DT files (next to 'rockchip,rk3568-csi-dphy' in rk356x.dtsi) and in px30.dtsi the 'csi_dphy' node has 'PX30_PD_VI' as value for its 'power-domains' property.

But this is all 'circumstantial'; it would be nice to have a clear answer (from Rockchip?).

Cheers,
Diederik

The "synopsys,dw-hdmi.yaml" binding specifies that the interrupts property of the hdmi node has 'maxItems: 1', so the hdmi node in rk3328.dtsi having 2 is incorrect.

Paragraph 1.3 ("System Interrupt connection") of the RK3328 TRM v1.1 page 16 and 17 define the following hdmi related interrupts:
- 67 hdmi_intr
- 103 hdmi_intr_wakeup

The difference of 32 is due to a different base used in the TRM.

The RK3399 (which uses the same binding) has '23: hdmi_irq' and '24: hdmi_wakeup_irq' according to its TRM (page 19).
The RK3568 (also same binding) has '76: hdmi_wakeup' and '77: hdmi' according to page 17 of its TRM.
In both cases the non-wakeup IRQ was used, so use that too for rk3328.

---
changes in v2:
- Added Fixes tag

 arch/arm64/boot/dts/rockchip/rk3328.dtsi | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index b5cbe7cab10b..0597de415fe0 100644
@@ -754,8 +754,7 @@ hdmi: hdmi@ff3c0000 {
 		compatible = "rockchip,rk3328-dw-hdmi";
 		reg = <0x0 0xff3c0000 0x0 0x20000>;
 		reg-io-width = <4>;
-		interrupts = <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>,
-			     <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
+		interrupts = <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cru PCLK_HDMI>,
 			 <&cru SCLK_HDMI_SFC>,
 			 <&cru SCLK_RTC32K>;

2.45.2

The "brcm,bluetooth.yaml" binding has 'device-wakeup-gpios' and 'host-wakeup-gpios' property names, not '*-wake-gpios'. Fix the incorrect property names.

Note that the "realtek,bluetooth.yaml" binding does use the '*-wake-gpios' property names.

---
changes in v2:
- Dropped change already covered by Heiko's patch; updated commit message accordingly
- Added Fixes tag

 arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index de4c082dce07..7381bb751852 100644
@@ -684,8 +684,8 @@ bluetooth {
 		compatible = "brcm,bcm43438-bt";
 		clocks = <&rk817 1>;
 		clock-names = "lpo";
-		device-wake-gpios = <&gpio0 RK_PC2 GPIO_ACTIVE_HIGH>;
-		host-wake-gpios = <&gpio0 RK_PC3 GPIO_ACTIVE_HIGH>;
+		device-wakeup-gpios = <&gpio0 RK_PC2 GPIO_ACTIVE_HIGH>;
+		host-wakeup-gpios = <&gpio0 RK_PC3 GPIO_ACTIVE_HIGH>;
 		reset-gpios = <&gpio0 RK_PC4 GPIO_ACTIVE_LOW>;
 		pinctrl-0 = <&bt_enable_h>, <&bt_host_wake_l>, <&bt_wake_h>;
 		pinctrl-names = "default";

2.45.2

For most compatibles, the "brcm,bluetooth.yaml" binding doesn't allow the 'reset-gpios' property, but there is a 'shutdown-gpios' property.

Page 12 of the AzureWave-CM256SM datasheet (v1.9) has the following wrt pin 34 'BT_REG_ON' (connected to GPIO0_C4_d on the PineNote):

Used by PMU to power up or power down the internal regulators used by the Bluetooth section. Also, when deasserted, this pin holds the Bluetooth section in reset. This pin has an internal 200k ohm pull down resistor that is enabled by default.

So it is safe to replace 'reset-gpios' with 'shutdown-gpios'.

---
changes in v2:
- Extended commit message to explain why replacing 'reset' with 'shutdown' is safe
- Added Fixes tag

 arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi  | 2 +-
 arch/arm64/boot/dts/rockchip/rk3566-radxa-cm3.dtsi | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index 7381bb751852..100a2774bbb5 100644
@@ -686,9 +686,9 @@ bluetooth {
 		clock-names = "lpo";
 		device-wakeup-gpios = <&gpio0 RK_PC2 GPIO_ACTIVE_HIGH>;
 		host-wakeup-gpios = <&gpio0 RK_PC3 GPIO_ACTIVE_HIGH>;
-		reset-gpios = <&gpio0 RK_PC4 GPIO_ACTIVE_LOW>;
 		pinctrl-0 = <&bt_enable_h>, <&bt_host_wake_l>, <&bt_wake_h>;
 		pinctrl-names = "default";
+		shutdown-gpios = <&gpio0 RK_PC4 GPIO_ACTIVE_LOW>;
 		vbat-supply = <&vcc_wl>;
 		vddio-supply = <&vcca_1v8_pmu>;
 	};

index d09e6542e236..3e0cbfff96d8 100644
@@ -402,9 +402,9 @@ bluetooth {
 		clock-names = "lpo";
 		device-wakeup-gpios = <&gpio2 RK_PB2 GPIO_ACTIVE_HIGH>;
 		host-wakeup-gpios = <&gpio2 RK_PB1 GPIO_ACTIVE_HIGH>;
-		reset-gpios = <&gpio2 RK_PC0 GPIO_ACTIVE_LOW>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&bt_host_wake_h &bt_reg_on_h &bt_wake_host_h>;
+		shutdown-gpios = <&gpio2 RK_PC0 GPIO_ACTIVE_LOW>;
 		vbat-supply = <&vcc_3v3>;
 		vddio-supply = <&vcc_1v8>;
 	};

2.45.2

Applied, thanks!

[2/4] arm64: dts: rockchip: Remove hdmi's 2nd interrupt on rk3328

      commit: de50a7e3681771c6b990238af82bf1dea9b11b21
[3/4] arm64: dts: rockchip: Fix wakeup prop names on PineNote BT node
      commit: 87299d6ee95a37d2d576dd8077ea6860f77ad8e2
[4/4] arm64: dts: rockchip: Fix reset-gpios property on brcm BT nodes
      commit: 2b6a3f857550e52b1cd4872ebb13cb3e3cf12f5f

Best regards,

Heiko Stuebner <[email protected]>

Hi Heiko,

Please revert the 4th patch.

I must have messed up my testing previously, but BT does not work on the PineNote with the 4th patch applied and does work with it reverted.

With the 4th patch applied I get this:

[   20.298465] Bluetooth: hci0: command 0x0c03 tx timeout
[   20.299036] Bluetooth: hci0: BCM: Reset failed (-110)

And then running the list command in bluetoothctl returns no controllers and hciconfig -a hci0 reports DOWN and when trying to reset hci0, I get that timeout error again.

With the 4th patch reverted, the controller does work.

I also get a HUGE amount of 'spam' when running bluetoothctl like:

[bluetooth]# [CHG] Controller 90:E8:68:B9:60:46 UUIDs: 0000110e-0000-1000-8000-00805f9b34fb [bluetooth]# [CHG] Controller 90:E8:68:B9:60:46 UUIDs: 0000112d-0000-1000-8000-00805f9b34fb [bluetooth]# [CHG] Controller 90:E8:68:B9:60:46 UUIDs: 00001200-0000-1000-8000-00805f9b34fb [bluetooth]# [CHG] Controller 90:E8:68:B9:60:46 UUIDs: 0000111f-0000-1000-8000-00805f9b34fb [bluetooth]# [CHG] Controller 90:E8:68:B9:60:46 UUIDs: 0000110c-0000-1000-8000-00805f9b34fb [bluetooth]# [CHG] Controller 90:E8:68:B9:60:46 UUIDs: 00001800-0000-1000-8000-00805f9b34fb [bluetooth]# [CHG] Controller 90:E8:68:B9:60:46 UUIDs: 00001801-0000-1000-8000-00805f9b34fb [bluetooth]# [CHG] Controller 90:E8:68:B9:60:46 UUIDs: 0000180a-0000-1000-8000-00805f9b34fb [bluetooth]# [CHG] Controller 90:E8:68:B9:60:46 UUIDs: 03b80e5a-ede8-4b33-a751-6ce34ec4c700

and this is only a tiny fraction. So there's definitely room for improvement there, but at least it does work.

Dunno yet what to make of it as this will re-introduce the DT validation error, but working hardware seems still be preferable to non-working HW.

Sorry about this.

Regards,
Diederik

Hi Heiko,

FWIW, I figured out what went wrong.
My testing was correct, but redo-ing the implementation to make it ready for submission wasn't very smart.

With ``shutdown-gpios = <&gpio0 RK_PC4 GPIO_ACTIVE_HIGH>;`` it does work correctly, but I forgot to change GPIO_ACTIVE_LOW to GPIO_ACTIVE_HIGH before submitting.

I'll first figure out a better procedure before making a new submission, so the revert is still the best approach IMO.

Cheers,
Diederik

Hi Heiko,

I've now done a new submission:
https://lore.kernel.org/linux-rockchip/[email protected]/

So please don't revert the 4th patch now.

Cheers,
Diederik

Hey,

Am Freitag, 18. Oktober 2024, 11:35:51 CEST schrieb Diederik de Haas:

hehe ok :-) .
I meant to ask if the fix wasn't simply toggling the gpio polarity, and I guess with your patch you were faster than my question.

Heiko

Hi,

I already knew that was the fix the moment I opened the other dts(i) files with the same wireless+bt module.
While it was tempting to immediately sent the fix, I realized that being (too) eager to sent it out would be a recipe for another screw up.

And I wanted to think through why it happened in the first place and that's because my submission process is all manual with an 'insane' long git send-email command, hand-crafted.
So I better learn b4 (properly) so my focus can be on the patches and less on the submission process.

Making a mistake/screw up sucks, but not learning from them is bad.

Cheers,
Diederik