Re: [PATCH v1 3/3] Revert "Revert "driver core: Set fw_devlink=on by default""

[Saravana Kannan]

On Tue, Apr 27, 2021 at 8:10 AM Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
>
> On Tue, Apr 27, 2021 at 03:11:16PM +0100, Cristian Marussi wrote:
> > On Tue, Apr 27, 2021 at 09:33:31AM -0400, Jim Quinlan wrote:
> [...]
> > > >
> > > I believe that the brcmstb-mbox node is in our DT for backwards
> > > compatibility with our older Linux only.   Note that  we use the compatible
> > > string '"arm,scmi-smc", "arm,scmi"'; the former chooses SMC transport and
> > > ignores custom  mailboxes such as brcmstb-mbox.
> > >
> >
> > Right..so it is even more wrong that it is waiting for the mailboxes...but
> > looking at the DT:
> >
> > brcm_scmi_mailbox@0 {
> >                 #mbox-cells = <0x01>;
> >                 compatible = "brcm,brcmstb-mbox";
> >                 status = "disabled";
> >                 linux,phandle = <0x04>;
> >                 phandle = <0x04>;
> >         };
> >
> > brcm_scmi@0 {
> >                 compatible = "arm,scmi-smc\0arm,scmi";
> >                 mboxes = <0x04 0x00 0x04 0x01>;
> >                 mbox-names = "tx\0rx";
> >                 shmem = <0x05>;
> >                 status = "disabled";
> >                 arm,smc-id = <0x83000400>;
> >                 interrupt-names = "a2p";
> >                 #address-cells = <0x01>;
> >                 #size-cells = <0x00>;
> >
> > it seems to me that even though you declare an SMC based transport (and in fact
> > you define the smc-id too) you also still define mboxes (as a fallback I suppose)
> > referring to the brcm_scmi_mailbox phandle, and while this is ignored by the SCMI
> > driver (because you have selected a compatible SMC transport) I imagine this dep
> > is picked up by fw_devlink which in fact says:
> >
> > > [    0.300086] platform brcm_scmi@0: Linked as a consumer to brcm_scmi_mailbox@0
> >
> > and stalls waiting for it. (but I'm not really familiar on how fw_devlink
> > internals works really...so I maybe off in these regards)

Cristian,

Great debugging work for not having worked on this before! Your
comments about the dependencies are right. If you grep the logs for
"probe deferral", you'll see these lines and more:

[    0.942998] platform brcm_scmi@0: probe deferral - supplier
brcm_scmi_mailbox@0 not ready
[    3.622741] platform 8b20000.pcie: probe deferral - supplier
brcm_scmi@0 not ready
[    5.695929] platform 840c000.serial: probe deferral - supplier
brcm_scmi@0 not ready

Florian,

Sorry I wasn't clear in my earlier email. I was asking for the path to
the board file DT in upstream so I could look at it and the files it
references. I didn't mean to ask for an "decompiled" DTS attachment.
The decompiled ones make it a pain to track the phandles.

The part that's confusing to me is that the mbox node is disabled in
the DT you attached. fw_devlink is smart enough to ignore disabled
nodes. Is it getting enabled by the bootloader? Can you please try
deleting the reference to the brcm_scmi_mailbox from the scmi node and
see if it helps? Or leave it really disabled?

Also, as a separate test of workarounds, can you please add
deferred_probe_timeout=1 to your commandline and see if it helps? I'm
assuming you have modules enabled? Otherwise, the existing smarts in
fw_devlink to ignore devices with no drivers would have kicked in too.

> I was about to mention/ask the same when I saw Jim's reply. I see you have
> already asked that. Couple of my opinions based on my very limited knowledge
> on fw_devlink and how it works.
>
> 1. Since we have different compatible for SMC and mailbox, I am not sure
>    if it correct to leave mailbox information in scmi node. Once we have
>    proper yaml scheme, we must flag that error IMO.
>
> 2. IIUC, the fw_devlink might use information from DT to establish the
>    dependency and having mailbox information in this context may be
>    considered wrong as there is no dependency if it is using SMC.

If this mbox reference from scmi is wrong for the current kernel and
never used, then I'd recommend deleting that.

-Saravana