Re: [PATCH 1/4] arm64: dts: rockchip: drop rockchip,trcm-sync-tx-only from rk3588 i2s

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
To: Heiko Stuebner <heiko@sntech.de>, linux-rockchip@lists.infradead.org
Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	Heiko Stuebner <heiko.stuebner@cherry.de>,
	Sugar Zhang <sugar.zhang@rock-chips.com>,
	Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
Subject: Re: [PATCH 1/4] arm64: dts: rockchip: drop rockchip,trcm-sync-tx-only from rk3588 i2s
Date: Wed, 28 Feb 2024 11:43:16 +0100	[thread overview]
Message-ID: <27011333-8fed-414f-a2e1-b49760f4c82f@theobroma-systems.com> (raw)
In-Reply-To: <20240227164659.705271-2-heiko@sntech.de>

Hi Heiko,

On 2/27/24 17:46, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> The rockchip,trcm-sync-tx-only property is at this time only documented
> for the tdm variant of Rockchip i2s controllers.
> 
> While there was a series [0] adding code and binding for the property,
> it doesn't seem to have gone forward back in 2021.
> 
> So for now fix the devicetree check by removing the property from rk3588
> i2s controllers until support for it gets merged.
> 

It seems like tx-only should be supported if the dai_link has a 
symmetric rate, c.f. 
https://elixir.bootlin.com/linux/latest/source/sound/soc/rockchip/rockchip_i2s.c#L455 
is doing the same as the patch from 3 years ago[1] was trying to do, 
only in the probe.

[1] 
https://patchwork.kernel.org/project/linux-rockchip/patch/1629796734-4243-4-git-send-email-sugar.zhang@rock-chips.com/

Considering that the RK3588 doesn't have RX only support according to 
the documentation of that register....

However, I have no clue how the dai_link would get the symmetrical rate 
set, because the only place I could see it set is in set_link_flags if 
SND_SOC_TPLG_LNK_FLGBIT_SYMMETRIC_RATES if set in the flag mask and the 
flags, but my grep-fu returned nothing setting this flag anywhere... so 
maybe that's just dead code?

In any case, with the current code:

I2S_CKR_TRCM_TXRX is put into I2S_CKR register, regardless of that DT 
property. While the naming seems to suggest TX+RX should be possible, 
its value is 0 (offset 28) and in the TRM it says:

2'b00: Generates LRCK for TX only.

Soooo... we are essentially in tx-only mode today with or without that 
property. Some things to fix later on :)

Which made me look for the same thing for the i2s_tdm driver and the 
same mistake is made, though here there's no support for tx-only or 
rx-only according to the TRM, and it is required to write 0b01 (offset 
28)... which we do by abusing the tx-only DT property which writes 
TRCM_TX whose value is 1 (offset 28). Considering that this is stored in 
clk_trcm member in the struct and that we do checks on that member to 
know in which situation we are (txrx, tx, rx), this seems very incorrect 
to me as we would configure it in the only mode it knows (txrx) but make 
the driver assume tx-only is selected. One more thing to fix later on :)

In any case, this patch is for matching the dt-bindings so there's no 
reason to not merge this, so:

Reviewed-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Thanks,
Quentin