Received-SPF: pass (google.com: domain of linux-kernel+bounces-118258-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240224052436.3552333-1-saravanak@google.com>
 <ZfvN5jDrftG-YRG4@titan> <CAGETcx8+vw0Vr0NWzjOAvxAZ07M4U7BWPAgO9avCngW0-9e_kA@mail.gmail.com>
 <Zf7I65PiOR2wX1Uo@titan> <CAGETcx_=MmfgDajM16iJ4Of9Yr2Sy6ZpU=MyhYgnmOJFUTD_oA@mail.gmail.com>
 <ZgIZ4LmFOqdiDJBH@titan>
In-Reply-To: <ZgIZ4LmFOqdiDJBH@titan>
From: Saravana Kannan <saravanak@google.com>
Date: Mon, 25 Mar 2024 18:35:45 -0700
Message-ID: <CAGETcx-Emvu41nB3UDnb4Gh2aJEKu_hFcHX89uWnBTnaqvpN8g@mail.gmail.com>
Subject: Re: [PATCH] of: property: fw_devlink: Fix stupid bug in
 remote-endpoint parsing
To: John Watts <contact@jookia.org>
Cc: Rob Herring <robh+dt@kernel.org>, Frank Rowand <frowand.list@gmail.com>, 
	=?UTF-8?Q?Herv=C3=A9_Codina?= <herve.codina@bootlin.com>, 
	Luca Ceresoli <luca.ceresoli@bootlin.com>, kernel-team@android.com, 
	Rob Herring <robh@kernel.org>, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Mar 25, 2024 at 5:42=E2=80=AFPM John Watts <contact@jookia.org> wro=
te:
>
> Hello there,
>
> On Mon, Mar 25, 2024 at 03:49:44PM -0700, Saravana Kannan wrote:
> > Ok, I think I understand now what's going on. fw_devlink does not know
> > that "sound" device will not populate "multi" as a child device.
> > Typically in such situations, "sound" would probe as a device and add
> > its child DT nodes devices. At that point, the cycle is only between
> > "multi" and "test_codec" and fw_devlink will detect that and not
> > enforce any ordering. However, in this case, "sound" doesn't have any
> > child devices and just depends on the remote endpoints directly.
> >
> > We already have "ports", "in-ports" and "out-ports". Is there a reason
> > none of them will work for your use case and it has to be "multi"?
> > When you use one of those 3 recognized node names, things are handled
> > correctly.
>
> audio-graph-card2 uses 'multi' to define DAI links that have multiple
> endpoints. It also suports codec2codec and dpcm.
>
> > I think the right fix is the use of post-init-providers. Because even
> > if you do the above, all it does is let fw_devlink see that there's a
> > cyclic dependency in DT. And it'll stop enforcing the probe and
> > suspend/resume ordering. Ideally we want to enforce a specific order
> > here. test_codec first and then sound.
>
> Is there a way to do this automatically so all the existing audio-graph-c=
ard2
> device trees aren't broken? As it stands it seems like this driver is now
> broken due to this change.

Ok, I have a solution. Have the audio-graph-card2 find the fwnode of
"multi" and mark it as "not a device" by doing something like this in
the driver. That should help fw_devlink handle this correctly.

fwnode.flags |=3D FWNODE_FLAG_NOT_DEVICE;

>
> > Maybe. But the logs would be more helpful.
>
> If you have a way for me to get more logs please tell me.
>
> > > > post-init-provider =3D <&multi>;
> >
> > Did you try this? Did it help?
> >
> > -Saravana
>
> No I haven't tried this yet. I shall try it soon. But I wouldn't consider
> this a useful fix as it requires upgrading existing device trees.

Definitely do this though as a forward looking improvement. It'll help
make the suspend/resume more deterministic and will eventually let
things happen in an async manner.


-Saravana