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

[AngeloGioacchino Del Regno]

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