Message-ID: <957b44d3-ef48-4682-8db5-3261e582ac8f@notapiano>
Date: Wed, 28 Feb 2024 09:44:04 -0500
From: Nícolas F. R. A. Prado <nfraprado@...labora.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Matthias Brugger <matthias.bgg@...il.com>, kernel@...labora.com,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH] soc: mediatek: cmdq: Don't log an error when
 gce-client-reg is not found

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?

Thanks,
Nícolas

Powered by blists - more mailing lists