[PATCH v3] arm64: dts: rockchip: add SPDIF node for rk3399-rockpro64

[PATCH v3] arm64: dts: rockchip: add SPDIF node for rk3399-rockpro64

[Katsuhiro Suzuki]

This patch adds 'disabled' SPDIF sound node and related settings
of SPDIF for rk3399-rockpro64.

RockPro64 has output pins for SPDIF Tx. But RK3399 does not have
enough DMA channel for enabling SPDIF tx. Current settings are:

  - I2S0     (Req num 0, 1): Enabled : Output to 40pins header CON40
  - I2S1     (Req num 2, 3): Enabled : Output to ES8316 on board
  - I2S2     (Req num 4, 5): Enabled : Output to internal HDMI core
  - SPDIF Tx (Req num 7)   : Disabled: Output to connector J10

If users want to enable ALL sound I/Os, we need 7 DMA channels for
it. But unfortunately, RK3399 has only 6 DMA channels. So users have
to choose from the following:

  - Disable one of I2S (Ex. I2S0) and enable SPDIF tx
  - Keep enable I2S0/1/2 and disable SPDIF tx

Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>

---

Changes in v3:
  - Refine commit description why adding disabled node

Changes in v2:
  - Remove redundant status property
---
 .../boot/dts/rockchip/rk3399-rockpro64.dtsi   | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
index 6e553ff47534..58097245994a 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
@@ -76,6 +76,23 @@ sound {
 		dais = <&i2s1_p0>;
 	};
 
+	sound-dit {
+		compatible = "audio-graph-card";
+		label = "rockchip,rk3399";
+		dais = <&spdif_p0>;
+	};
+
+	spdif-dit {
+		compatible = "linux,spdif-dit";
+		#sound-dai-cells = <0>;
+
+		port {
+			dit_p0_0: endpoint {
+				remote-endpoint = <&spdif_p0_0>;
+			};
+		};
+	};
+
 	vcc12v_dcin: vcc12v-dcin {
 		compatible = "regulator-fixed";
 		regulator-name = "vcc12v_dcin";
@@ -698,6 +715,16 @@ &sdhci {
 	status = "okay";
 };
 
+&spdif {
+	pinctrl-0 = <&spdif_bus_1>;
+
+	spdif_p0: port {
+		spdif_p0_0: endpoint {
+			remote-endpoint = <&dit_p0_0>;
+		};
+	};
+};
+
 &spi1 {
 	status = "okay";
 
-- 
2.28.0

Re: [PATCH v3] arm64: dts: rockchip: add SPDIF node for rk3399-rockpro64

[Johan Jonker]

Hi Katsuhiro, Heiko,

Question for the maintainer:
Should we add a SPDIF node if the connector is not physical on a board,
only a header?

Thanks Katsuhiro for the "aplay -l" screen print.

**** List of PLAYBACK Hardware Devices ****
card 0: hdmisound [hdmi-sound], device 0: ff8a0000.i2s-i2s-hifi
i2s-hifi-0 [ff8a0000.i2s-i2s-hifi i2s-hifi-0]
  Subdevices: 1/1
  Subdevice #0: subdevice #0
card 1: rockchiprk3399 [rockchip,rk3399], device 0: ff890000.i2s-ES8316
HiFi ES8316 HiFi-0 [ff890000.i2s-ES8316 HiFi ES8316 HiFi-0]
  Subdevices: 1/1
  Subdevice #0: subdevice #0
card 2: rockchiprk339_1 [rockchip,rk3399], device 0:
ff870000.spdif-dit-hifi dit-hifi-0 [ff870000.spdif-dit-hifi dit-hifi-0]
  Subdevices: 1/1
  Subdevice #0: subdevice #0


On 10/5/20 4:03 PM, Katsuhiro Suzuki wrote:
> This patch adds 'disabled' SPDIF sound node and related settings
> of SPDIF for rk3399-rockpro64.
> 
> RockPro64 has output pins for SPDIF Tx. But RK3399 does not have
> enough DMA channel for enabling SPDIF tx. Current settings are:
> 
>   - I2S0     (Req num 0, 1): Enabled : Output to 40pins header CON40
>   - I2S1     (Req num 2, 3): Enabled : Output to ES8316 on board
>   - I2S2     (Req num 4, 5): Enabled : Output to internal HDMI core
>   - SPDIF Tx (Req num 7)   : Disabled: Output to connector J10
> 
> If users want to enable ALL sound I/Os, we need 7 DMA channels for
> it. But unfortunately, RK3399 has only 6 DMA channels. So users have
> to choose from the following:
> 
>   - Disable one of I2S (Ex. I2S0) and enable SPDIF tx
>   - Keep enable I2S0/1/2 and disable SPDIF tx
> 
> Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
> 
> ---
> 
> Changes in v3:
>   - Refine commit description why adding disabled node
> 
> Changes in v2:
>   - Remove redundant status property
> ---
>  .../boot/dts/rockchip/rk3399-rockpro64.dtsi   | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> index 6e553ff47534..58097245994a 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> @@ -76,6 +76,23 @@ sound {
>  		dais = <&i2s1_p0>;
>  	};
>  


	hdmi_sound: hdmi-sound {
		compatible = "simple-audio-card";
		simple-audio-card,name = "hdmi-sound";

Maybe rename to "HDMI"?

[..]
	};

	sound {
		compatible = "audio-graph-card";
		label = "rockchip,rk3399";

Maybe change this to "ES8316" to prevent confusion?

		dais = <&i2s1_p0>;
	};


> +	sound-dit {
> +		compatible = "audio-graph-card"
> +		label = "rockchip,rk3399";

This would be the second sound card with the same label.
It seems that aplay/linux? adds "-1" to it and removes the comma, so we get:

hdmisound
rockchiprk3399
rockchiprk339_1

Shouldn't we label it with something that reflect the function/output.
Shouldn't we standardize to SPDIF, HDMI and Analog similar to rk3318/rk3328?
Make a shorter label without spaces or special chars, so that chars
don't get removed?

Proposal:

HDMI
ES8316
SPDIF

> +		dais = <&spdif_p0>;

Maybe disable too?

The "sound-dit" node is standard enabled and will start some process
with a dia in a node that is disabled.


> +	};
> +
> +	spdif-dit {
> +		compatible = "linux,spdif-dit";
> +		#sound-dai-cells = <0>;

Maybe disable too?

> +
> +		port {
> +			dit_p0_0: endpoint {

> +				remote-endpoint = <&spdif_p0_0>;

This also points to something that's disabled.

> +			};
> +		};
> +	};
> +
>  	vcc12v_dcin: vcc12v-dcin {
>  		compatible = "regulator-fixed";
>  		regulator-name = "vcc12v_dcin";
> @@ -698,6 +715,16 @@ &sdhci {
>  	status = "okay";
>  };
>  
> +&spdif {
> +	pinctrl-0 = <&spdif_bus_1>;

This node is disabled.

> +
> +	spdif_p0: port {
> +		spdif_p0_0: endpoint {
> +			remote-endpoint = <&dit_p0_0>;
> +		};
> +	};
> +};
> +
>  &spi1 {
>  	status = "okay";
>  
> 

Re: [PATCH v3] arm64: dts: rockchip: add SPDIF node for rk3399-rockpro64

[Katsuhiro Suzuki]

Hello Heiko, Johan,

This is ping... How about Johan's question?

> Question for the maintainer:
> Should we add a SPDIF node if the connector is not physical on a board,
> only a header?

If we don't need to add nodes for sound I/F that has only header not
physical on a board, we should drop this patch and also remove I2S0
because I2S0 has no physical connector on a board.

Additionally, I2S1 share the I2S_CLK pin with I2S0. So ROCKPro64 users
perhaps cannot use I2S0 and I2S1 (for ES8316) at same time. If users
try to use I2S1 after I2S0, I2S1 overwrites clock setting and I2S0
will face strange behavior.

I don't know why ROCKPro64 has such strange pin implementation...


For Johan:

Sorry for late reply. Thank you for your review.
And I agree with you renaming sound nodes.

- HDMI
- ES8316 (or "Analog" is more useful for media player SW??)
- SPDIF (if we need)


Best Regards,
Katsuhiro Suzuki


On 2020/10/06 18:51, Johan Jonker wrote:
> Hi Katsuhiro, Heiko,
> 
> Question for the maintainer:
> Should we add a SPDIF node if the connector is not physical on a board,
> only a header?
> 
> Thanks Katsuhiro for the "aplay -l" screen print.
> 
> **** List of PLAYBACK Hardware Devices ****
> card 0: hdmisound [hdmi-sound], device 0: ff8a0000.i2s-i2s-hifi
> i2s-hifi-0 [ff8a0000.i2s-i2s-hifi i2s-hifi-0]
>    Subdevices: 1/1
>    Subdevice #0: subdevice #0
> card 1: rockchiprk3399 [rockchip,rk3399], device 0: ff890000.i2s-ES8316
> HiFi ES8316 HiFi-0 [ff890000.i2s-ES8316 HiFi ES8316 HiFi-0]
>    Subdevices: 1/1
>    Subdevice #0: subdevice #0
> card 2: rockchiprk339_1 [rockchip,rk3399], device 0:
> ff870000.spdif-dit-hifi dit-hifi-0 [ff870000.spdif-dit-hifi dit-hifi-0]
>    Subdevices: 1/1
>    Subdevice #0: subdevice #0
> 
> 
> On 10/5/20 4:03 PM, Katsuhiro Suzuki wrote:
>> This patch adds 'disabled' SPDIF sound node and related settings
>> of SPDIF for rk3399-rockpro64.
>>
>> RockPro64 has output pins for SPDIF Tx. But RK3399 does not have
>> enough DMA channel for enabling SPDIF tx. Current settings are:
>>
>>    - I2S0     (Req num 0, 1): Enabled : Output to 40pins header CON40
>>    - I2S1     (Req num 2, 3): Enabled : Output to ES8316 on board
>>    - I2S2     (Req num 4, 5): Enabled : Output to internal HDMI core
>>    - SPDIF Tx (Req num 7)   : Disabled: Output to connector J10
>>
>> If users want to enable ALL sound I/Os, we need 7 DMA channels for
>> it. But unfortunately, RK3399 has only 6 DMA channels. So users have
>> to choose from the following:
>>
>>    - Disable one of I2S (Ex. I2S0) and enable SPDIF tx
>>    - Keep enable I2S0/1/2 and disable SPDIF tx
>>
>> Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
>>
>> ---
>>
>> Changes in v3:
>>    - Refine commit description why adding disabled node
>>
>> Changes in v2:
>>    - Remove redundant status property
>> ---
>>   .../boot/dts/rockchip/rk3399-rockpro64.dtsi   | 27 +++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
>> index 6e553ff47534..58097245994a 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
>> @@ -76,6 +76,23 @@ sound {
>>   		dais = <&i2s1_p0>;
>>   	};
>>   
> 
> 
> 	hdmi_sound: hdmi-sound {
> 		compatible = "simple-audio-card";
> 		simple-audio-card,name = "hdmi-sound";
> 
> Maybe rename to "HDMI"?
> 
> [..]
> 	};
> 
> 	sound {
> 		compatible = "audio-graph-card";
> 		label = "rockchip,rk3399";
> 
> Maybe change this to "ES8316" to prevent confusion?
> 
> 		dais = <&i2s1_p0>;
> 	};
> 
> 
>> +	sound-dit {
>> +		compatible = "audio-graph-card"
>> +		label = "rockchip,rk3399";
> 
> This would be the second sound card with the same label.
> It seems that aplay/linux? adds "-1" to it and removes the comma, so we get:
> 
> hdmisound
> rockchiprk3399
> rockchiprk339_1
> 
> Shouldn't we label it with something that reflect the function/output.
> Shouldn't we standardize to SPDIF, HDMI and Analog similar to rk3318/rk3328?
> Make a shorter label without spaces or special chars, so that chars
> don't get removed?
> 
> Proposal:
> 
> HDMI
> ES8316
> SPDIF
> 
>> +		dais = <&spdif_p0>;
> 
> Maybe disable too?
> 
> The "sound-dit" node is standard enabled and will start some process
> with a dia in a node that is disabled.
> 
> 
>> +	};
>> +
>> +	spdif-dit {
>> +		compatible = "linux,spdif-dit";
>> +		#sound-dai-cells = <0>;
> 
> Maybe disable too?
> 
>> +
>> +		port {
>> +			dit_p0_0: endpoint {
> 
>> +				remote-endpoint = <&spdif_p0_0>;
> 
> This also points to something that's disabled.
> 
>> +			};
>> +		};
>> +	};
>> +
>>   	vcc12v_dcin: vcc12v-dcin {
>>   		compatible = "regulator-fixed";
>>   		regulator-name = "vcc12v_dcin";
>> @@ -698,6 +715,16 @@ &sdhci {
>>   	status = "okay";
>>   };
>>   
>> +&spdif {
>> +	pinctrl-0 = <&spdif_bus_1>;
> 
> This node is disabled.
> 
>> +
>> +	spdif_p0: port {
>> +		spdif_p0_0: endpoint {
>> +			remote-endpoint = <&dit_p0_0>;
>> +		};
>> +	};
>> +};
>> +
>>   &spi1 {
>>   	status = "okay";
>>   
>>
> 

Re: [PATCH v3] arm64: dts: rockchip: add SPDIF node for rk3399-rockpro64

[Heiko Stübner]

Am Dienstag, 6. Oktober 2020, 11:51:58 CET schrieb Johan Jonker:
> Hi Katsuhiro, Heiko,
> 
> Question for the maintainer:
> Should we add a SPDIF node if the connector is not physical on a board,
> only a header?

I think so ... the connector always is just like n-pins on a board,
only "sometimes" someone solders a fancy jack to it ;-)


Heiko

> 
> Thanks Katsuhiro for the "aplay -l" screen print.
> 
> **** List of PLAYBACK Hardware Devices ****
> card 0: hdmisound [hdmi-sound], device 0: ff8a0000.i2s-i2s-hifi
> i2s-hifi-0 [ff8a0000.i2s-i2s-hifi i2s-hifi-0]
>   Subdevices: 1/1
>   Subdevice #0: subdevice #0
> card 1: rockchiprk3399 [rockchip,rk3399], device 0: ff890000.i2s-ES8316
> HiFi ES8316 HiFi-0 [ff890000.i2s-ES8316 HiFi ES8316 HiFi-0]
>   Subdevices: 1/1
>   Subdevice #0: subdevice #0
> card 2: rockchiprk339_1 [rockchip,rk3399], device 0:
> ff870000.spdif-dit-hifi dit-hifi-0 [ff870000.spdif-dit-hifi dit-hifi-0]
>   Subdevices: 1/1
>   Subdevice #0: subdevice #0
> 
> 
> On 10/5/20 4:03 PM, Katsuhiro Suzuki wrote:
> > This patch adds 'disabled' SPDIF sound node and related settings
> > of SPDIF for rk3399-rockpro64.
> > 
> > RockPro64 has output pins for SPDIF Tx. But RK3399 does not have
> > enough DMA channel for enabling SPDIF tx. Current settings are:
> > 
> >   - I2S0     (Req num 0, 1): Enabled : Output to 40pins header CON40
> >   - I2S1     (Req num 2, 3): Enabled : Output to ES8316 on board
> >   - I2S2     (Req num 4, 5): Enabled : Output to internal HDMI core
> >   - SPDIF Tx (Req num 7)   : Disabled: Output to connector J10
> > 
> > If users want to enable ALL sound I/Os, we need 7 DMA channels for
> > it. But unfortunately, RK3399 has only 6 DMA channels. So users have
> > to choose from the following:
> > 
> >   - Disable one of I2S (Ex. I2S0) and enable SPDIF tx
> >   - Keep enable I2S0/1/2 and disable SPDIF tx
> > 
> > Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
> > 
> > ---
> > 
> > Changes in v3:
> >   - Refine commit description why adding disabled node
> > 
> > Changes in v2:
> >   - Remove redundant status property
> > ---
> >  .../boot/dts/rockchip/rk3399-rockpro64.dtsi   | 27 +++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> > index 6e553ff47534..58097245994a 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> > @@ -76,6 +76,23 @@ sound {
> >  		dais = <&i2s1_p0>;
> >  	};
> >  
> 
> 
> 	hdmi_sound: hdmi-sound {
> 		compatible = "simple-audio-card";
> 		simple-audio-card,name = "hdmi-sound";
> 
> Maybe rename to "HDMI"?
> 
> [..]
> 	};
> 
> 	sound {
> 		compatible = "audio-graph-card";
> 		label = "rockchip,rk3399";
> 
> Maybe change this to "ES8316" to prevent confusion?
> 
> 		dais = <&i2s1_p0>;
> 	};
> 
> 
> > +	sound-dit {
> > +		compatible = "audio-graph-card"
> > +		label = "rockchip,rk3399";
> 
> This would be the second sound card with the same label.
> It seems that aplay/linux? adds "-1" to it and removes the comma, so we get:
> 
> hdmisound
> rockchiprk3399
> rockchiprk339_1
> 
> Shouldn't we label it with something that reflect the function/output.
> Shouldn't we standardize to SPDIF, HDMI and Analog similar to rk3318/rk3328?
> Make a shorter label without spaces or special chars, so that chars
> don't get removed?
> 
> Proposal:
> 
> HDMI
> ES8316
> SPDIF
> 
> > +		dais = <&spdif_p0>;
> 
> Maybe disable too?
> 
> The "sound-dit" node is standard enabled and will start some process
> with a dia in a node that is disabled.
> 
> 
> > +	};
> > +
> > +	spdif-dit {
> > +		compatible = "linux,spdif-dit";
> > +		#sound-dai-cells = <0>;
> 
> Maybe disable too?
> 
> > +
> > +		port {
> > +			dit_p0_0: endpoint {
> 
> > +				remote-endpoint = <&spdif_p0_0>;
> 
> This also points to something that's disabled.
> 
> > +			};
> > +		};
> > +	};
> > +
> >  	vcc12v_dcin: vcc12v-dcin {
> >  		compatible = "regulator-fixed";
> >  		regulator-name = "vcc12v_dcin";
> > @@ -698,6 +715,16 @@ &sdhci {
> >  	status = "okay";
> >  };
> >  
> > +&spdif {
> > +	pinctrl-0 = <&spdif_bus_1>;
> 
> This node is disabled.
> 
> > +
> > +	spdif_p0: port {
> > +		spdif_p0_0: endpoint {
> > +			remote-endpoint = <&dit_p0_0>;
> > +		};
> > +	};
> > +};
> > +
> >  &spi1 {
> >  	status = "okay";
> >  
> > 
> 
>