Re: [PATCH v2 1/3] dt-bindings: platform: Add bindings for Qcom's EC on IT8987

[Maya Matuszczyk]

niedz., 22 gru 2024 o 15:40 Krzysztof Kozlowski <krzk@xxxxxxxxxx> napisał(a):
>
> On 20/12/2024 19:24, Stephan Gerhold wrote:
> > On Fri, Dec 20, 2024 at 07:16:34PM +0100, Maya Matuszczyk wrote:
> >> Excuse the formatting, I've typed this reply from my phone
> >>
> >> pt., 20 gru 2024, 19:05 użytkownik Stephan Gerhold <
> >> stephan.gerhold@xxxxxxxxxx> napisał:
> >>
> >>> On Thu, Dec 19, 2024 at 09:08:18PM +0100, Maya Matuszczyk wrote:
> >>>> This patch adds bindings for the EC firmware running on IT8987 present
> >>>> on most of X1E80100 devices
> >>>>
> >>>> Signed-off-by: Maya Matuszczyk <maccraft123mc@xxxxxxxxx>
> >>>> ---
> >>>>  .../bindings/platform/qcom,x1e-it8987-ec.yaml | 52 +++++++++++++++++++
> >>>>  1 file changed, 52 insertions(+)
> >>>>  create mode 100644
> >>> Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml
> >>>>
> >>>> diff --git
> >>> a/Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml
> >>> b/Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..4a4f6eb63072
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml
> >>>> @@ -0,0 +1,52 @@
> >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>> +%YAML 1.2
> >>>> +---
> >>>> +$id: http://devicetree.org/schemas/platform/qcom,x1e-it8987-ec.yaml#
> >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>> +
> >>>> +title: Qualcomm Embedded Controller on IT8987 chip.
> >>>> +
> >>>> +maintainers:
> >>>> +  - Maya Matuszczyk <maccraft123mc@xxxxxxxxx>
> >>>> +
> >>>> +description:
> >>>> +  Most x1e80100 laptops have an EC running on IT8987 MCU chip. The EC
> >>> controls
> >>>> +  minor functions, like fans, power LED, and on some laptops it also
> >>> handles
> >>>> +  keyboard hotkeys.
> >>>> +
> >>>> +properties:
> >>>> +  compatible:
> >>>> +    oneOf:
> >>>> +      - const: qcom,x1e-it8987-ec
> >>>
> >>> Given that ECs tend to be somewhat device-specific and many vendors
> >>> might have slightly customized the EC firmware(?), I think it would be
> >>> better to disallow using this generic compatible without a more specific
> >>> one. In other words, I would drop this line and just keep the case
> >>> below:
> >>>
> >> I've looked at DSDT of other devices and they look to be compatible with
> >> what's on the devkit, with differences being extra features on magicbook
> >> art 14 and yoga slim 7x. Though this isn't a hill I'm willing to die on.
> >>
> >
> > I think it's fine to keep qcom,x1e-it8987-ec as second compatible.
>
>
> No, because:
> 1. There is no such thing as x1e
> 2. If there was a soc like this, this has nothing to do with SoC. It is
> not a component inside SoC and that is the only allowed case when you
> use SoC compatibles.

It was the closest thing I had for a "platform name"

>
> > However, without a more specific compatible, there is a risk we have
> > nothing to match on in case device-specific handling becomes necessary
> > in the driver at some point.
> >
> > It's certainly subjective, but it might be better to play it safe here
> > and have a specific compatible that one can match, even if the behavior
> > is 99% the same. There will often be subtly different behavior across
> > devices, you mentioned the "keyboard backlight turning off and the power
> > LED slowly blinking", who knows what else exists.
> >
> > I suppose worst case we could also use of_machine_is_compatible() to
> > just match the device the EC is running on, but I'm not sure if that
> > would be frowned upon.
>
>
> Unless you have some sort of insights or secret knowledge from Qualcomm
> (Bjorn or Konrad can chime in here), I think these are pure guesses that
> this is a Qualcomm product (implied by vendor prefix) or some sort of
> re-usable generic firmware from Qualcomm present on multiple devices.

The x elite devkit also has the IT8987 EC chip, and when comparing the
firmware of it and Yoga Slim 7x's EC there are similarities when
running them through strings
On both of them at the beginning there are strings that look like
version identifiers:
Devkit:
UUBBK V:00.20.00$
BBK-V20$

Slim7x:
UUBBK V:00.21.00$
BBK-V21$

With similar ones at the end:
Devkit:
EC VER:00.29.00$
LsFv:00.29.00$
Qualcomm$
WoS 8c GenX$
ODM$
MB:A0$
BUILD DATE:
02/0//2/24$
TIME:
14:33:35$

Slim7x:
EC VER:00.60.00$
LsFv:00.20.00$
Qualcomm$
WoS 8c GenX$
ODM$
MB:A0$
BUILD DATE:
2024/07/25$
TIME:
09:58:00$



>
> If the FW across devices is the same, then fallbacks for these are fine
> with me.

As the devkit has EC firmware that is handled the same way in DSDT
tables of most of other x1e laptops with the same EC, and is a subset
of what's done on Lenovo Yoga Slim 7x and Honor Magicbook Art 14 I
think the devkit's compatible  + -ec would be a good pick.

This conversation is getting long and I feel like I've said everything
I wanted to say, I'll just do what you tell me to do about the
fallback and binding filename.

>
> Best regards,
> Krzysztof