| Message ID | 20250130165126.2292107-3-detlev.casanova@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add HDMI audio on the Radxa ROCK 5B | expand |
Hi Detlev, On 1/30/25 5:45 PM, Detlev Casanova wrote: > Use the simple-audio-card driver with the hdmi0 QP node as CODEC and > the i2s5 device as CPU. > > The simple-audio-card,mclk-fs value is set to 128 as it is done in > the downstream driver. > > The #sound-dai-cells value is set to 0 in the hdmi0 node so that it can be > used as an audio codec node. > > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> > --- > arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 1 + > .../boot/dts/rockchip/rk3588-rock-5b.dts | 19 +++++++++++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi > index 8cfa30837ce72..790c1c25a3e41 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi > @@ -1394,6 +1394,7 @@ hdmi0: hdmi@fde80000 { > reset-names = "ref", "hdp"; > rockchip,grf = <&sys_grf>; > rockchip,vo-grf = <&vo1_grf>; > + #sound-dai-cells = <0>; > status = "disabled"; > > ports { > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts > index d597112f1d5b8..1909078538367 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts > @@ -49,6 +49,21 @@ hdmi0_con_in: endpoint { > }; > }; > > + hdmi0-sound { > + compatible = "simple-audio-card"; > + simple-audio-card,format = "i2s"; > + simple-audio-card,mclk-fs = <128>; > + simple-audio-card,name = "hdmi0"; > + > + simple-audio-card,codec { > + sound-dai = <&hdmi0>; > + }; > + > + simple-audio-card,cpu { > + sound-dai = <&i2s5_8ch>; > + }; > + }; > + This is SoC specific and not board specific I believe. I2S5 is not usable anywhere else but with HDMI0 controller if I've read the TRM properly. Therefore, I would suggest to move this to rk3588-base.dtsi (with status=disabled; and when hdmi1-sound comes up, to rk3588-extra.dtsi), the same way we've done for RK3399 for example. The only hesitation I have is that HDMI0 can use either I2S or SPDIF for audio, both audio controllers internal exclusive to HDMI0/1 controller. But the user could anyway define their own simple audio card for spdif or modify this one if they wanted to. I've tested with my suggested change and same changes than for Rock 5B DTS on RK3588 Tiger Haikou with speaker-test -c 2 -t wav, left is left, right is right :) I'm not giving my Tb here as the commit title is specifically about Rock 5B and I haven't tested this series on it. If you had a separate patch for the hdmi sound node and enabling it on Rock 5b, I would have given my Tb on the former and not the latter. Thanks! Quentin
Hi Quentin, On Friday, 31 January 2025 11:38:34 EST Quentin Schulz wrote: > Hi Detlev, > > On 1/30/25 5:45 PM, Detlev Casanova wrote: > > Use the simple-audio-card driver with the hdmi0 QP node as CODEC and > > the i2s5 device as CPU. > > > > The simple-audio-card,mclk-fs value is set to 128 as it is done in > > the downstream driver. > > > > The #sound-dai-cells value is set to 0 in the hdmi0 node so that it can be > > used as an audio codec node. > > > > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> > > --- > > > > arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 1 + > > .../boot/dts/rockchip/rk3588-rock-5b.dts | 19 +++++++++++++++++++ > > 2 files changed, 20 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi > > b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi index > > 8cfa30837ce72..790c1c25a3e41 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi > > @@ -1394,6 +1394,7 @@ hdmi0: hdmi@fde80000 { > > > > reset-names = "ref", "hdp"; > > rockchip,grf = <&sys_grf>; > > rockchip,vo-grf = <&vo1_grf>; > > > > + #sound-dai-cells = <0>; > > > > status = "disabled"; > > > > ports { > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts > > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index > > d597112f1d5b8..1909078538367 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts > > @@ -49,6 +49,21 @@ hdmi0_con_in: endpoint { > > > > }; > > > > }; > > > > + hdmi0-sound { > > + compatible = "simple-audio-card"; > > + simple-audio-card,format = "i2s"; > > + simple-audio-card,mclk-fs = <128>; > > + simple-audio-card,name = "hdmi0"; > > + > > + simple-audio-card,codec { > > + sound-dai = <&hdmi0>; > > + }; > > + > > + simple-audio-card,cpu { > > + sound-dai = <&i2s5_8ch>; > > + }; > > + }; > > + > > This is SoC specific and not board specific I believe. I2S5 is not > usable anywhere else but with HDMI0 controller if I've read the TRM > properly. > > Therefore, I would suggest to move this to rk3588-base.dtsi (with > status=disabled; and when hdmi1-sound comes up, to rk3588-extra.dtsi), > the same way we've done for RK3399 for example. Indeed, I only could test on a Rock 5B, but I think it can be moved to the SoC DTS. > The only hesitation I have is that HDMI0 can use either I2S or SPDIF for > audio, both audio controllers internal exclusive to HDMI0/1 controller. > But the user could anyway define their own simple audio card for spdif > or modify this one if they wanted to. So some boards will use I2S and some SPDIF ? Or any board can be used with one or the other ? The disabled status makes sense as hdmi is disabled in the SoC tree as well. So if a user wants to use SPDIF instead, they could keep hdmi0-sound disabled and add their own simple-audio-card compatible node. > I've tested with my suggested change and same changes than for Rock 5B > DTS on RK3588 Tiger Haikou with speaker-test -c 2 -t wav, left is left, > right is right :) > > I'm not giving my Tb here as the commit title is specifically about Rock > 5B and I haven't tested this series on it. If you had a separate patch > for the hdmi sound node and enabling it on Rock 5b, I would have given > my Tb on the former and not the latter. Thank you for testing anyway ! I will move the node and enable it in all board dts that already enable hdmi0. Detlev.
Hi Detlev, On 1/31/25 6:18 PM, Detlev Casanova wrote: > Hi Quentin, > > On Friday, 31 January 2025 11:38:34 EST Quentin Schulz wrote: >> Hi Detlev, >> >> On 1/30/25 5:45 PM, Detlev Casanova wrote: [...] >> The only hesitation I have is that HDMI0 can use either I2S or SPDIF for >> audio, both audio controllers internal exclusive to HDMI0/1 controller. >> But the user could anyway define their own simple audio card for spdif >> or modify this one if they wanted to. > > So some boards will use I2S and some SPDIF ? Or any board can be used with one > or the other ? Considering this is all SoC-internal, boards could decide what they want to use but I'm not sure what would make them pick one over the other? I still don't really understand why this option even exists to be honest (why Rockchip provided it I mean), I'm not sure what does using SPDIF bring over using I2S in that context? Maybe the number of channels? Maybe specific rates? Different power domain, clock domain, etc? > The disabled status makes sense as hdmi is disabled in the SoC tree as well. Yup, we're not sure the board will have an HDMI connector (or bridge using the HDMI controller), so it needs to be disabled on the SoC level. > So if a user wants to use SPDIF instead, they could keep hdmi0-sound disabled > and add their own simple-audio-card compatible node. > Exactly, or reuse but modify hdmi0-sound as well. Plenty of options :) >> I've tested with my suggested change and same changes than for Rock 5B >> DTS on RK3588 Tiger Haikou with speaker-test -c 2 -t wav, left is left, >> right is right :) >> >> I'm not giving my Tb here as the commit title is specifically about Rock >> 5B and I haven't tested this series on it. If you had a separate patch >> for the hdmi sound node and enabling it on Rock 5b, I would have given >> my Tb on the former and not the latter. > > Thank you for testing anyway ! I will move the node and enable it in all board > dts that already enable hdmi0. > Up to you, I usually refrain from sending patches for boards I cannot test especially for new features and let each board's maintainer/contributor send a patch for it. I for sure won't mind if RK3588 Tiger support isn't in your v4 :) I'm not actively following the linux-rockchip ML, so please consider adding me in Cc of the v4 if you want my Rb or Tb trailers :) Cheers, Quentin
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi index 8cfa30837ce72..790c1c25a3e41 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi @@ -1394,6 +1394,7 @@ hdmi0: hdmi@fde80000 { reset-names = "ref", "hdp"; rockchip,grf = <&sys_grf>; rockchip,vo-grf = <&vo1_grf>; + #sound-dai-cells = <0>; status = "disabled"; ports { diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index d597112f1d5b8..1909078538367 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts @@ -49,6 +49,21 @@ hdmi0_con_in: endpoint { }; }; + hdmi0-sound { + compatible = "simple-audio-card"; + simple-audio-card,format = "i2s"; + simple-audio-card,mclk-fs = <128>; + simple-audio-card,name = "hdmi0"; + + simple-audio-card,codec { + sound-dai = <&hdmi0>; + }; + + simple-audio-card,cpu { + sound-dai = <&i2s5_8ch>; + }; + }; + leds { compatible = "gpio-leds"; pinctrl-names = "default"; @@ -318,6 +333,10 @@ i2s0_8ch_p0_0: endpoint { }; }; +&i2s5_8ch { + status = "okay"; +}; + &package_thermal { polling-delay = <1000>;
Use the simple-audio-card driver with the hdmi0 QP node as CODEC and the i2s5 device as CPU. The simple-audio-card,mclk-fs value is set to 128 as it is done in the downstream driver. The #sound-dai-cells value is set to 0 in the hdmi0 node so that it can be used as an audio codec node. Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> --- arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 1 + .../boot/dts/rockchip/rk3588-rock-5b.dts | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+)