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

From: Florian Fainelli <f.fainelli@gmail.com>
To: Sudeep Holla <sudeep.holla@arm.com>,
	Cristian Marussi <cristian.marussi@arm.com>
Cc: Jim Quinlan <james.quinlan@broadcom.com>,
	Saravana Kannan <saravanak@google.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Al Cooper <alcooperx@gmail.com>, Michael Walle <michael@walle.cc>,
	Jon Hunter <jonathanh@nvidia.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Android Kernel Team <kernel-team@android.com>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 3/3] Revert "Revert "driver core: Set fw_devlink=on by default""
Date: Tue, 27 Apr 2021 09:24:55 -0700	[thread overview]
Message-ID: <0887ce92-e9d8-47ec-0077-4c1f2fd46f87@gmail.com> (raw)
In-Reply-To: <20210427151042.j7hku7pxqz56uyt6@bogus>

On 4/27/2021 8:10 AM, Sudeep Holla 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)
>>
> 
> 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.

This is a self inflicted problem that we have in that the bootloader
provides a Device Tree to the kernel which is massaged in different ways
and intends to stay backwards compatible as much as possible. And indeed
after removing the 'mboxes' property gets us going with fw_devlink=on.

> 
> 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.

Right, unfortunately, short of having some special casing for SCMI and
checking that if we have both an "arm,smc-id" and "mboxes" phandle we
should prefer the former, there is not probably much that can be done
here. Do we want to do that?

Thank you all for looking into this!
-- 
Florian