[PATCH v2 0/4] rockchip: Fix several DT validation errors

[PATCH v2 0/4] rockchip: Fix several DT validation errors

[Diederik de Haas]

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

[PATCH v2 1/4] arm64: dts: rockchip: Add PD to csi dphy node on rk356x

[Diederik de Haas]

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.

Fixes: b6c228401b25 ("arm64: dts: rockchip: add csi dphy node to rk356x")
Signed-off-by: Diederik de Haas <didi.debian@cknow.org>
---
changes in v2:
- No change

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

diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index 0ee0ada6f0ab..d581170914f9 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -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

Re: [PATCH v2 1/4] arm64: dts: rockchip: Add PD to csi dphy node on rk356x

[Michael Riesch]

Hi Diederik,

On 10/8/24 13:15, Diederik de Haas wrote:
> 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.

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

> 
> Fixes: b6c228401b25 ("arm64: dts: rockchip: add csi dphy node to rk356x")
> Signed-off-by: Diederik de Haas <didi.debian@cknow.org>
> ---
> changes in v2:
> - No change
> 
>  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> index 0ee0ada6f0ab..d581170914f9 100644
> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> @@ -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>;

Re: [PATCH v2 1/4] arm64: dts: rockchip: Add PD to csi dphy node on rk356x

[Diederik de Haas]

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

Hi Michael,

On Tue Oct 8, 2024 at 2:32 PM CEST, Michael Riesch wrote:
> On 10/8/24 13:15, Diederik de Haas wrote:
> > 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.
>
> Thanks for the patch. However, I am not sure about this one.

Thanks for your reply.

> 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 :-)

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

> > Fixes: b6c228401b25 ("arm64: dts: rockchip: add csi dphy node to rk356x")
> > Signed-off-by: Diederik de Haas <didi.debian@cknow.org>
> > ---
> > changes in v2:
> > - No change
> > 
> >  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > index 0ee0ada6f0ab..d581170914f9 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > @@ -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>;


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

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

[Diederik de Haas]

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.

Helped-by: Heiko Stuebner <heiko@sntech.de>
Fixes: 725e351c265a ("arm64: dts: rockchip: add rk3328 display nodes")
Signed-off-by: Diederik de Haas <didi.debian@cknow.org>
---
changes in v2:
- Added Fixes tag

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

diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index b5cbe7cab10b..0597de415fe0 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -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

[PATCH v2 3/4] arm64: dts: rockchip: Fix wakeup prop names on PineNote BT node

[Diederik de Haas]

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.

Fixes: d449121e5e8a ("arm64: dts: rockchip: Add Pine64 PineNote board")
Signed-off-by: Diederik de Haas <didi.debian@cknow.org>
---
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(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi b/arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi
index de4c082dce07..7381bb751852 100644
--- a/arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi
@@ -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

[PATCH v2 4/4] arm64: dts: rockchip: Fix reset-gpios property on brcm BT nodes

[Diederik de Haas]

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'.

Fixes: d449121e5e8a ("arm64: dts: rockchip: Add Pine64 PineNote board")
Signed-off-by: Diederik de Haas <didi.debian@cknow.org>
---
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(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi b/arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi
index 7381bb751852..100a2774bbb5 100644
--- a/arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi
@@ -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>;
 	};
diff --git a/arch/arm64/boot/dts/rockchip/rk3566-radxa-cm3.dtsi b/arch/arm64/boot/dts/rockchip/rk3566-radxa-cm3.dtsi
index d09e6542e236..3e0cbfff96d8 100644
--- a/arch/arm64/boot/dts/rockchip/rk3566-radxa-cm3.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3566-radxa-cm3.dtsi
@@ -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

Re: (subset) [PATCH v2 0/4] rockchip: Fix several DT validation errors

[Heiko Stuebner]

On Tue, 8 Oct 2024 13:15:35 +0200, Diederik de Haas wrote:
> 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.
> 
> [...]

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 <heiko@sntech.de>

Re: (subset) [PATCH v2 0/4] rockchip: Fix several DT validation errors

[Diederik de Haas]

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

Hi Heiko,

On Tue Oct 8, 2024 at 9:28 PM CEST, Heiko Stuebner wrote:
> On Tue, 8 Oct 2024 13:15:35 +0200, Diederik de Haas wrote:
> > 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.
>
> 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

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

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

Re: (subset) [PATCH v2 0/4] rockchip: Fix several DT validation errors

[Diederik de Haas]

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

Hi Heiko,

On Wed Oct 16, 2024 at 11:41 AM CEST, Diederik de Haas wrote:
> On Tue Oct 8, 2024 at 9:28 PM CEST, Heiko Stuebner wrote:
> > On Tue, 8 Oct 2024 13:15:35 +0200, Diederik de Haas wrote:
> > > 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.
> >
> > 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
>
> 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.

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

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

Re: (subset) [PATCH v2 0/4] rockchip: Fix several DT validation errors

[Diederik de Haas]

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

Hi Heiko,

On Wed Oct 16, 2024 at 2:35 PM CEST, Diederik de Haas wrote:
> On Wed Oct 16, 2024 at 11:41 AM CEST, Diederik de Haas wrote:
> > On Tue Oct 8, 2024 at 9:28 PM CEST, Heiko Stuebner wrote:
> > > On Tue, 8 Oct 2024 13:15:35 +0200, Diederik de Haas wrote:
> > > > 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.
> > >
> > > 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
> >
> > 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.
>
> 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.

I've now done a new submission:
https://lore.kernel.org/linux-rockchip/20241018092237.6774-1-didi.debian@cknow.org/

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

Cheers,
  Diederik

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

Re: (subset) [PATCH v2 0/4] rockchip: Fix several DT validation errors

[Heiko Stübner]

Hey,

Am Freitag, 18. Oktober 2024, 11:35:51 CEST schrieb Diederik de Haas:
> Hi Heiko,
> 
> On Wed Oct 16, 2024 at 2:35 PM CEST, Diederik de Haas wrote:
> > On Wed Oct 16, 2024 at 11:41 AM CEST, Diederik de Haas wrote:
> > > On Tue Oct 8, 2024 at 9:28 PM CEST, Heiko Stuebner wrote:
> > > > On Tue, 8 Oct 2024 13:15:35 +0200, Diederik de Haas wrote:
> > > > > 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.
> > > >
> > > > 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
> > >
> > > 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.
> >
> > 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.
> 
> I've now done a new submission:
> https://lore.kernel.org/linux-rockchip/20241018092237.6774-1-didi.debian@cknow.org/
> 
> So please don't revert the 4th patch now.

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

Re: (subset) [PATCH v2 0/4] rockchip: Fix several DT validation errors

[Diederik de Haas]

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

Hi,

On Fri Oct 18, 2024 at 11:37 AM CEST, Heiko Stübner wrote:
> Am Freitag, 18. Oktober 2024, 11:35:51 CEST schrieb Diederik de Haas:
> > On Wed Oct 16, 2024 at 2:35 PM CEST, Diederik de Haas wrote:
> > > On Wed Oct 16, 2024 at 11:41 AM CEST, Diederik de Haas wrote:
> > > > 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.
> > >
> > > 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.
> > 
> > I've now done a new submission:
> > https://lore.kernel.org/linux-rockchip/20241018092237.6774-1-didi.debian@cknow.org/
> > 
> > So please don't revert the 4th patch now.
>
> 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.

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

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