Re: [PATCH V4 0/5] arm_scmi: vendors: Qualcomm Generic Vendor Extensions

[Sudeep Holla <sudeep.holla@arm.com>]

On Tue, Dec 17, 2024 at 05:55:35PM +0530, Sibi Sankar wrote:
> 
> 
> On 12/5/24 22:31, Sudeep Holla wrote:
> > On Fri, Nov 22, 2024 at 09:37:47AM +0100, Johan Hovold wrote:
> > > On Thu, Nov 14, 2024 at 09:52:12AM +0530, Sibi Sankar wrote:
> > > > On 11/8/24 20:44, Johan Hovold wrote:
> > > 
> > > > > > On Wed, Nov 06, 2024 at 01:55:33PM +0100, Johan Hovold wrote:
> > > 
> > > > > > > Second, after loading the protocol and client drivers manually (in that
> > > > > > > order, shouldn't the client driver pull in the protocol?), I got:
> > > > > > > 
> > > > > > > 	scmi_module: Loaded SCMI Vendor Protocol 0x80 - Qualcomm  20000
> > > > > > > 	arm-scmi arm-scmi.0.auto: QCOM Generic Vendor Version 1.0
> > > > > > > 	scmi-qcom-generic-ext-memlat scmi_dev.5: error -EOPNOTSUPP: failed to configure common events
> > > > > > > 	scmi-qcom-generic-ext-memlat scmi_dev.5: probe with driver scmi-qcom-generic-ext-memlat failed with error -95
> > > > > > > 
> > > > > > > which seems to suggest that the firmware on my CRD does not support this
> > > > > > > feature. Is that the way this should be interpreted? And does that mean
> > > > > > > that non of the commercial laptops supports this either?
> > > 
> > > > > Yeah, hopefully Sibi can shed some light on this. I'm using the DT
> > > > > patch (5/5) from this series, which according to the commit message is
> > > > > supposed to enable bus scaling on the x1e80100 platform. So I guess
> > > > > something is missing in my firmware.
> > > > 
> > > > Nah, it's probably just because of the algo string used.
> > > > The past few series used caps MEMLAT string instead of
> > > > memlat to pass the tuneables, looks like all the laptops
> > > > havn't really switched to it yet. Will revert back to
> > > > using to lower case memlat so that all devices are
> > > > supported. Thanks for trying the series out!
> > > 
> > > I have a Lenovo ThinkPad T14s set up now so I gave this series a spin
> > > there too, and there I do *not* see the above mentioned -EOPNOSUPP error
> > > and the memlat driver probes successfully.
> > > 
> > > On the other hand, this series seems to have no effect on a kernel
> > > compilation benchmark. Is that expected?
> > > 
> > 
> > Hijacking this thread to rant about state of firmware implementation on
> > this platform that gives me zero confidence in merging any of these without
> > examining each of the interface details in depth and at lengths.
> > 
> 
> Hey Sudeep,
> 
> Thanks for taking time to review the series.
> 
> > Also I see the standard protocol like PERF seem to have so many issues which
> > adds to my no confidence. I can't comment on that thread for specific reasons.
> 
> ^^ is largely untrue, a lot of finger pointing and a gross
> misrepresentation of reality :/
>

Sorry if I was not clear, I just said I don't have confidence yet and if
the firmware is stable, then it is just the impression I have arrived based
on the discussions.

> crash in the LEVEL_GET regular message implementation. This
> pretty much went unnoticed because of messaging in perf implementation
> in kernel.

OK, is there any scope to improve in your opinion ? Please suggest and
discuss or post a patch to have separate discussion.

> Given the fastchannel implementation isn't mandatory
> according to spec, the kernel clearly says it switches to
> regular messaging when it clearly doesn't do that and uses
> stale data structures instead.

Interesting, it sounds like a bug. Please provide details or patch to
fix the bug. That would probably fix it on whatever platform we are
concerned here.

> This ensured that level get regular messaging never got tested.
>

You seem to point at this bug several time now, we need to get it fixed,
but we need to understand it first if you want us to fix it or as mentioned
before you can as well post the patch.

> We pretty much have been good upstream citizens, finding bugs and
> sending fixes wherever we can. We clearly don't deserve such a hostile
> stance.
>

Not sure what made you think we are hostile towards your contributions.
We just need a maintainable solution merged upstream and we are working
towards the same. The documents written as part of this series is not
there yet to help me understand the protocol yet. I have asked questions
and answer to those can be made part of the next version to improve it
IMO.

> > I will briefly mention my suspicion here. This Lenovo ThinkPad T14s being
> > primarily targeting other OS using ACPI might have just implemented what is
> > required for ACPI CPPC which conveniently doesn't have to discover lot of
> > fastchannel details since they are supplied in the tables straight away.
> > But that also would mean it could be not fully compliant to SCMI spec.
>
> Not fully compliant to the spec? I am pretty sure this series would
> have been shot down completely and NAKd on the list by you if that
> was the case lol.
>

Honestly I am still trying to make any sense out of this vendor protocols.
The documents produced as part of this series doesn't help me understand
the same and that is my main feedback so far on this thread. I haven't
looked at the code yet so I can't comment on the same as I first need
to understand the vendor protocol document/specification.

-- 
Regards,
Sudeep