Rockchip I2S commit possibly ignored

Rockchip I2S commit possibly ignored

[Geraldo Nascimento]

Hi Mark,

I was looking at Rockchip I2S commits and it seems "ASoC: rockchip: i2s:
Reset the controller if soft reset failed" was supposed to have been
merged to your sound.git but never was. I don't know if this was
intentional or a real miss but in any case I'm letting you know.

According to 515b436be291ff197c52198282bbb19e79c9d197 'Merge series
"Patches to update for rockchip i2s" from Sugar Zhang
<sugar.zhang@rock-chips.com>:' it should have been merged to your tree.
In rockchip-i2s.yaml there's even documentation that references the
missing commit.

However in the alsa-devel archives,
https://mailman.alsa-project.org/pipermail/alsa-devel/2021-August/189050.html
I see there was no commit info for the unmerged patch. Perhaps this
caused it to be black-holed?

Thanks,
Geraldo Nascimento

Re: Rockchip I2S commit possibly ignored

[Mark Brown]

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

On Tue, Aug 16, 2022 at 10:15:16AM -0300, Geraldo Nascimento wrote:

> I was looking at Rockchip I2S commits and it seems "ASoC: rockchip: i2s:
> Reset the controller if soft reset failed" was supposed to have been
> merged to your sound.git but never was. I don't know if this was
> intentional or a real miss but in any case I'm letting you know.

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, so sending again is generally a better approach though there are
some other maintainers who like them - if in doubt look at how patches
for the subsystem are normally handled.

> However in the alsa-devel archives,
> https://mailman.alsa-project.org/pipermail/alsa-devel/2021-August/189050.html
> I see there was no commit info for the unmerged patch. Perhaps this
> caused it to be black-holed?

If there was no commit info that means it wasn't applied.

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

Re: Rockchip I2S commit possibly ignored

[Geraldo Nascimento]

On Tue, Aug 16, 2022 at 02:19:42PM +0100, Mark Brown wrote:
> On Tue, Aug 16, 2022 at 10:15:16AM -0300, Geraldo Nascimento wrote:
> 
> > I was looking at Rockchip I2S commits and it seems "ASoC: rockchip: i2s:
> > Reset the controller if soft reset failed" was supposed to have been
> > merged to your sound.git but never was. I don't know if this was
> > intentional or a real miss but in any case I'm letting you know.
> 
> Please don't send content free pings and please allow a reasonable time
> for review.  People get busy, go on holiday, attend conferences and so 
> on so unless there is some reason for urgency (like critical bug fixes)
> please allow at least a couple of weeks for review.  If there have been
> review comments then people may be waiting for those to be addressed.
> 
> Sending content free pings adds to the mail volume (if they are seen at
> all) which is often the problem and since they can't be reviewed
> directly if something has gone wrong you'll have to resend the patches
> anyway, so sending again is generally a better approach though there are
> some other maintainers who like them - if in doubt look at how patches
> for the subsystem are normally handled.

This isn't my patch, it's a patch by Sugar Zhang from Rockchip that was
supposed to have been applied one year ago, give or take 10 days. I
can't resend a patch that wasn't authored by me.

Therefore I don't see the point of your complaint about "content free pings".

> 
> > However in the alsa-devel archives,
> > https://mailman.alsa-project.org/pipermail/alsa-devel/2021-August/189050.html
> > I see there was no commit info for the unmerged patch. Perhaps this
> > caused it to be black-holed?
> 
> If there was no commit info that means it wasn't applied.

That's what I thought. Cc:ing Sugar Zhang now.

Re: Rockchip I2S commit possibly ignored

[Mark Brown]

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

On Tue, Aug 16, 2022 at 10:37:18AM -0300, Geraldo Nascimento wrote:

> This isn't my patch, it's a patch by Sugar Zhang from Rockchip that was
> supposed to have been applied one year ago, give or take 10 days. I
> can't resend a patch that wasn't authored by me.

There's absolutely no problem with reposting someone else's patch - just
add your Signed-off-by at the end of the signoff chain.

> Therefore I don't see the point of your complaint about "content free pings".

In general the answer to "what's the status of this old patch?" is
almost always going to be the same so there's a form letter for it,
especially with such an old patch the chances of me having any
recollection of what's going on are minimal.

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

Re: Rockchip I2S commit possibly ignored

[Geraldo Nascimento]

On Tue, Aug 16, 2022 at 04:22:37PM +0100, Mark Brown wrote:
> On Tue, Aug 16, 2022 at 10:37:18AM -0300, Geraldo Nascimento wrote:
> 
> > This isn't my patch, it's a patch by Sugar Zhang from Rockchip that was
> > supposed to have been applied one year ago, give or take 10 days. I
> > can't resend a patch that wasn't authored by me.
> 
> There's absolutely no problem with reposting someone else's patch - just
> add your Signed-off-by at the end of the signoff chain.

Cool. I'm going to wait a couple of days to see if there's any interest
from the side of Sugar Zhang of resending the patch with a proper commit
message. If not, then I'll resend like you suggested. Thank you.

> 
> > Therefore I don't see the point of your complaint about "content free pings".
> 
> In general the answer to "what's the status of this old patch?" is
> almost always going to be the same so there's a form letter for it,
> especially with such an old patch the chances of me having any
> recollection of what's going on are minimal.

I understand it.

Thank you,
Geraldo Nascimento

Re: Rockchip I2S commit possibly ignored

[Mark Brown]

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

On Tue, Aug 16, 2022 at 02:54:22PM -0300, Geraldo Nascimento wrote:
> On Tue, Aug 16, 2022 at 04:22:37PM +0100, Mark Brown wrote:

> > There's absolutely no problem with reposting someone else's patch - just
> > add your Signed-off-by at the end of the signoff chain.

> Mark, I'm in no position to lecture anyone, least of who, you, hard-working
> ASoC maintainer that you are. But there are *tons* of problems with
> reposting someone else's patch, even if they had been previously given
> the green-light but misteriously vanished.

> You are assuming responsibility for someone else's work! Let's see in

Oh, sure - but TBH if you're chasing a patch via e-mail you're pretty
much going to be in a similar situation if that results in the patch
getting applied.  A big part of the goal behind getting things reposted
is to push them through the normal test/review cycle properly so if
there was some reason for it to not get applied the first time around
it's more likely that someone will notice than if it's just pulled out
of list archives or whatever.

> My main point is that without adding "resets" and "reset-names" to *at
> least* every Rockchip device tree that enables sound over HDMI, just an
> example, you get systems with non-working HDMI. I just tested it, I
> omitted "resets" and "reset-names" from my device tree and then had
> to SSH into the black screen to revert the changes to my boot partition.

> So it's not trivial to RESEND this. It amounts to device tree overhaul
> of arch/arm64/boot/dts/rockchip

> This would require community effort of say, equivalent to LibreELEC
> resources, which are not many, but they have enough patience to test
> every proposed change to Rockchip device trees, and could help upstream
> this.

Right, that's a problem.  Even with those changes if we start requiring
new properties that'd be an ABI break anyway which isn't something we
want for DT - ideally the patch should be reworked so that existing
systems continue to work even without DT updates.

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

Re: Rockchip I2S commit possibly ignored

[sugar zhang]

Hi Geraldo, Mark,

在 2022/8/17 1:54, Geraldo Nascimento 写道:
> On Tue, Aug 16, 2022 at 04:22:37PM +0100, Mark Brown wrote:
>> On Tue, Aug 16, 2022 at 10:37:18AM -0300, Geraldo Nascimento wrote:
>>
>>> This isn't my patch, it's a patch by Sugar Zhang from Rockchip that was
>>> supposed to have been applied one year ago, give or take 10 days. I
>>> can't resend a patch that wasn't authored by me.
>> There's absolutely no problem with reposting someone else's patch - just
>> add your Signed-off-by at the end of the signoff chain.
> Mark, I'm in no position to lecture anyone, least of who, you, hard-working
> ASoC maintainer that you are. But there are *tons* of problems with
> reposting someone else's patch, even if they had been previously given
> the green-light but misteriously vanished.
>
> You are assuming responsibility for someone else's work! Let's see in
> this case what would happen:
>
> In this case, the Rockchip I2S missing reset affair, I have tested
> the proposed changes, and they are OK, *if* you add the reset properties
> in the device tree.
>
> For example, I was playing with HDMI sound on the RK3399pro Radxa Rock Pi
> N10 and I added the following properties to my private rk3399.dtsi:
>
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -1695,6 +1695,8 @@ i2s2: i2s@ff8a0000 {
>   		dma-names = "tx", "rx";
>   		clock-names = "i2s_clk", "i2s_hclk";
>   		clocks = <&cru SCLK_I2S2_8CH>, <&cru HCLK_I2S2_8CH>;
> +		reset-names = "reset-m", "reset-h";
> +		resets = <&cru SRST_I2S2_8CH>, <&cru SRST_H_I2S2_8CH>;
>   		power-domains = <&power RK3399_PD_SDIOAUDIO>;
>   		#sound-dai-cells = <0>;
>   		status = "disabled";
>
> ---
>
>
> And once I okay &i2s2 and &hdmi_sound inside rk3399pro-vmarc-som.dtsi I
> get kind of working HDMI sound, very clipped, I have to software adjust
> the volume to 30% to get clean, not speaker-blowing sound. But I
> digress, the point that it kinda works is not the point at all.
>
> My main point is that without adding "resets" and "reset-names" to *at
> least* every Rockchip device tree that enables sound over HDMI, just an
> example, you get systems with non-working HDMI. I just tested it, I
> omitted "resets" and "reset-names" from my device tree and then had
> to SSH into the black screen to revert the changes to my boot partition.

Actually,  the reset is used for i2s SLAVE mode which may fail to clear 
if the MASTER

side stop the clk before i2s done. in this situation, we do reset the 
controller and

bring it back to normal state.

But, for HDMI sound,  the i2s works as MASTER mode and should not be 
fail to clear.

could you share your I2S and HDMI register dump to me to dig out the 
root cause.

OTOH，I have noticed that some patchs ignored on upstream, I will check 
and repost

those patchs.

> So it's not trivial to RESEND this. It amounts to device tree overhaul
> of arch/arm64/boot/dts/rockchip
>
> This would require community effort of say, equivalent to LibreELEC
> resources, which are not many, but they have enough patience to test
> every proposed change to Rockchip device trees, and could help upstream
> this.
>
> Thank you,
> Geraldo Nascimento

-- 
Best Regards!
张学广/Sugar
瑞芯微电子股份有限公司
Rockchip Electronics Co., Ltd.

Re: Rockchip I2S commit possibly ignored

[Geraldo Nascimento]

On Wed, Aug 17, 2022 at 11:17:32AM +0800, sugar zhang wrote:
> Hi Geraldo, Mark,

Hi Sugar!

> could you share your I2S and HDMI register dump to me to dig out the 
> root cause.

I ended up sorting it out on my own. HDMI AudioSample Register aud_conf2 must
be set to 0x4. This sets insert_pcuv bit and solves the extreme clipping
problem.

I'll post a patch later.

Thank you,
Geraldo Nascimento