Re: [PATCH] soc: mediatek: cmdq: Don't log an error when gce-client-reg is not found

From: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
To: "Nícolas F. R. A. Prado" <nfraprado@collabora.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>,
	kernel@collabora.com, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH] soc: mediatek: cmdq: Don't log an error when gce-client-reg is not found
Date: Wed, 28 Feb 2024 15:48:32 +0100	[thread overview]
Message-ID: <7860ea98-da64-4722-8521-ad75c9123f42@collabora.com> (raw)
In-Reply-To: <957b44d3-ef48-4682-8db5-3261e582ac8f@notapiano>

Il 28/02/24 15:44, Nícolas F. R. A. Prado ha scritto:
> On Wed, Feb 28, 2024 at 10:41:15AM +0100, AngeloGioacchino Del Regno wrote:
>> Il 26/02/24 22:31, Nícolas F. R. A. Prado ha scritto:
>>> Most of the callers to this function do not require CMDQ support, it is
>>> optional, so the missing property shouldn't cause an error message.
>>> Furthermore, the callers that do require CMDQ support already log at the
>>> error level when an error is returned.
>>>
>>> Change the log message in this helper to be printed at the debug level
>>> instead.
>>
>> CMDQ is optional, yes. At least, for some devices it is.
>>
>> Full story, though, wants that if you use the CPU for register manipulation
>> instead of programming the GCE (even with threading, fantastic!) you will
>> trigger various performance issues.
>>
>> In the end, you *don't want* to use the CPU if GCE is available!
>>
>> The reasons why the CMDQ/GCE is optional are:
>>   - You can check register-by-register r/w for debugging scenarios by using
>>     the CPU to manipulate them instead of having something magically doing
>>     that for you at a certain (pre-set, yes, but still!) point;
>>   - Not all SoCs have the same amount of GCE threads and channels, some may
>>     support writing to IP block X through the GCE, some may not, but both
>>     may support writing for IP block Y through this mailbox;
>>   - MediaTek chose to support both ways, enabling means to debug stuff upstream,
>>     the other choice would've been to never support CPU register R/W on some IPs
>>
>>     ... and btw - about the last part: Kudos, MediaTek.
> 
> Thank you for all the insight! :)
> 
>>
>> Now, I also get why you're raising this, but we have to find an agreement here
>> on a different way to proceed that satisfies all of us.
>>
>> First of all..
>>
>> Which device on which SoC is missing the GCE client register DT property?
>> Do said SoC really not have a GCE client register for that device?
> 
> This is what I see:
> 
> On mt8192-asurada-spherion:
> mediatek-mutex 14001000.mutex: error -2 can't parse gce-client-reg property (0)
> 
> On mt8195-cherry-tomato:
> mtk-mmsys 14000000.syscon: error -2 can't parse gce-client-reg property (0)
> mtk-mmsys 14f00000.syscon: error -2 can't parse gce-client-reg property (0)
> mediatek-mutex 1c016000.mutex: error -2 can't parse gce-client-reg property (0)
> mtk-mmsys 1c01a000.syscon: error -2 can't parse gce-client-reg property (0)
> mediatek-mutex 1c101000.mutex: error -2 can't parse gce-client-reg property (0)
> 
> So yes, there are a few missing. I will send patches adding them so we can get
> the best performance possible upstream.
> 
>>
>> Is any upstream supported SoC really lacking a GCE register for the upstream
>> supported IPs?
>>
>> I'm not sure.... :-)
>>
>> P.S.: I guess that the alternative (that I somewhat dislike, and you can probably
>>        understand why after reading the reasons above) would be to turn that into a
>>        dev_info() instead...
>>
>> P.P.S.: Having no GCE usually means that there's a performance issue! In that case,
>>          it's ... a mistake, so, an error, kind-of.... :-)
> 
> Given the performance impact, I agree that debug, and even info level is not a
> good option. At the same time, the hardware still works correctly without the
> GCE (as we have been running it so far without even realizing!), so I think
> calling it an error is too much, and a warning would be the most suitable level,
> as we just want to warn userspace: "Hey user, be advised that you're missing
> GCE, so you'll get worse performance! It will still work though". What do you
> think?
> 

Agreed.

Cheers,
Angelo