mbox series

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

Message ID 20241008113344.23957-1-didi.debian@cknow.org (mailing list archive)
Headers show
Series rockchip: Fix several DT validation errors | expand

Message

Diederik de Haas Oct. 8, 2024, 11:15 a.m. UTC 
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(-)

Comments

Heiko Stübner Oct. 8, 2024, 7:28 p.m. UTC | #1 
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,
Diederik de Haas Oct. 16, 2024, 9:41 a.m. UTC | #2 
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
Diederik de Haas Oct. 16, 2024, 12:35 p.m. UTC | #3 
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
Diederik de Haas Oct. 18, 2024, 9:35 a.m. UTC | #4 
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
Heiko Stübner Oct. 18, 2024, 9:37 a.m. UTC | #5 
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
Diederik de Haas Oct. 18, 2024, 10:01 a.m. UTC | #6 
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