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

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

[Maya Matuszczyk]

This patch adds bindings for the EC firmware running on IT8987 present
on most of X1E80100 devices

Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
---
 .../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@gmail.com>
+
+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
+      - items:
+        - const: lenovo,yoga-slim7x-ec
+        - const: qcom,x1e-it8987-ec
+
+  reg:
+    const: 0x76
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        embedded-controller@76 {
+            compatible = "lenovo,yoga-slim7x-ec", "qcom,x1e-it8987-ec";
+            reg = <0x76>;
+
+            interrupts-extended = <&tlmm 66 IRQ_TYPE_LEVEL_HIGH>;
+        };
+    };
+...
-- 
2.45.2

[PATCH v2 3/3] arm64: dts: qcom: x1e80100-lenovo-yoga-slim7x: Add the EC

[Maya Matuszczyk]

Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
---
 .../dts/qcom/x1e80100-lenovo-yoga-slim7x.dts  | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts b/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts
index 0cdaff9c8cf0..dfe009613b0c 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts
+++ b/arch/arm64/boot/dts/qcom/x1e80100-lenovo-yoga-slim7x.dts
@@ -538,6 +538,22 @@ keyboard@3a {
 	};
 };
 
+&i2c5 {
+	clock-frequency = <400000>;
+
+	status = "okay";
+
+	embedded-controller@76 {
+		compatible = "lenovo,yoga-slim7x-ec", "qcom,x1e-it8987-ec";
+		reg = <0x76>;
+
+		interrupts-extended = <&tlmm 66 IRQ_TYPE_EDGE_FALLING>;
+
+		pinctrl-0 = <&ec_int_n_default>;
+		pinctrl-names = "default";
+	};
+};
+
 &i2c8 {
 	clock-frequency = <400000>;
 
@@ -796,6 +812,12 @@ &tlmm {
 			       <44 4>, /* SPI (TPM) */
 			       <238 1>; /* UFS Reset */
 
+	ec_int_n_default: ec-int-n-state {
+		pins = "gpio66";
+		function = "gpio";
+		bias-disable;
+	};
+
 	edp_reg_en: edp-reg-en-state {
 		pins = "gpio70";
 		function = "gpio";
-- 
2.45.2

Re: [PATCH v2 3/3] arm64: dts: qcom: x1e80100-lenovo-yoga-slim7x: Add the EC

[Aiqun(Maria) Yu]

On 12/20/2024 4:08 AM, Maya Matuszczyk wrote:
> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>

Is the commit message missing for this patch?
> ---
>  .../dts/qcom/x1e80100-lenovo-yoga-slim7x.dts  | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
[...]


-- 
Thx and BRs,
Aiqun(Maria) Yu

Re: [PATCH v2 3/3] arm64: dts: qcom: x1e80100-lenovo-yoga-slim7x: Add the EC

[Konrad Dybcio]

On 20.12.2024 12:52 PM, Aiqun(Maria) Yu wrote:
> On 12/20/2024 4:08 AM, Maya Matuszczyk wrote:
>> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> 
> Is the commit message missing for this patch?

Yeah

Maya, please add a short summary (some backstory about the EC, what
it does or so).

You'll probably be asked to do the same with the bindings commit, but
you can mostly repeat the same thing both here and there.

Konrad

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

[Rob Herring (Arm)]

On Thu, 19 Dec 2024 21:08:18 +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@gmail.com>
> ---
>  .../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
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml:22:9: [warning] wrong indentation: expected 10 but found 8 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241219200821.8328-1-maccraft123mc@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Stephan Gerhold]

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@gmail.com>
> ---
>  .../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@gmail.com>
> +
> +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:

> +      - items:
> +        - const: lenovo,yoga-slim7x-ec
> +        - const: qcom,x1e-it8987-ec

People can add compatible entries for other devices as needed.

Thanks,
Stephan

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

[Stephan Gerhold]

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@linaro.org> 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@gmail.com>
> > > ---
> > >  .../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@gmail.com>
> > > +
> > > +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.
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.

Thanks,
Stephan

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

[Krzysztof Kozlowski]

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@linaro.org> 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@gmail.com>
>>>> ---
>>>>  .../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@gmail.com>
>>>> +
>>>> +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.

> 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.

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

Best regards,
Krzysztof

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@kernel.org> 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@linaro.org> 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@gmail.com>
> >>>> ---
> >>>>  .../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@gmail.com>
> >>>> +
> >>>> +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

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

[Krzysztof Kozlowski]

On 22/12/2024 16:07, Maya Matuszczyk wrote:
> 
> 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.

Go with a device specific compatible (I think I proposed in other
email). It is up to you if you want to add fallbacks. If you add
fallbacks, then please include some summary of above in commit msg or
binding description, so there will be a trace of that explanation/reason
for fallbacks.

Best regards,
Krzysztof

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

[Aiqun(Maria) Yu]

On 12/22/2024 11:07 PM, Maya Matuszczyk wrote:
> niedz., 22 gru 2024 o 15:40 Krzysztof Kozlowski <krzk@kernel.org> 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@linaro.org> 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@gmail.com>
>>>>>> ---
>>>>>>  .../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@gmail.com>
>>>>>> +
>>>>>> +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.
> 

I have raised this topic internally at Qualcomm. However, since some key
personnel are currently on holiday, an immediate response is not
possible. Rest assured, the topic is under review internally. Please
stay tuned for updates.

I want to personally thank you for your current support and efforts.
>>
>> Best regards,
>> Krzysztof


-- 
Thx and BRs,
Aiqun(Maria) Yu

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

[Aiqun(Maria) Yu]

On 12/30/2024 10:45 AM, Aiqun(Maria) Yu wrote:
> On 12/22/2024 11:07 PM, Maya Matuszczyk wrote:
[...]
>>
> 
> I have raised this topic internally at Qualcomm. However, since some key
> personnel are currently on holiday, an immediate response is not
> possible. Rest assured, the topic is under review internally. Please
> stay tuned for updates.

Just want to update the thread to let you know: We are working on this. :)

The information that can be updated here is as follows:

1. Qualcomm has a reference spec design for EC (Embedded Controller),
and OEMs are able to customize it according to their design.
The reference spec design may regardless of ITE or other EC vendors.

2. the reference EC spec will be supported on Qualcomm Compute reference
designs for other platform as well. This is irrespective of the
underlying transport (I2C, I3C, or any other Qualcomm proprietary transport.

Some of the main functionalities in this patch series appear to align
with the spec reference design. Therefore, it could be possibly extended
to become a more generic EC driver that supports a wider range of devices.
> 
> I want to personally thank you for your current support and efforts.
[...]


-- 
Thx and BRs,
Aiqun(Maria) Yu

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

[Krzysztof Kozlowski]

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

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> 
> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> ---
>  .../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.

Drop full stop. Titles don't have them.

> +
> +maintainers:
> +  - Maya Matuszczyk <maccraft123mc@gmail.com>
> +
> +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

That's not a SoC, so I don't understand:
1. referring to the SoC,
2. Having this alone and as fallback.

It does not look like you tested the bindings, at least after quick
look. Please run 'make dt_binding_check' (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint. Don't rely on
distro packages for dtschema and be sure you are using the latest
released dtschema.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 22/12/2024 07:33, Krzysztof Kozlowski wrote:
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - const: qcom,x1e-it8987-ec
> 
> That's not a SoC, so I don't understand:
> 1. referring to the SoC,
> 2. Having this alone and as fallback.
> 
> It does not look like you tested the bindings, at least after quick
> look. Please run 'make dt_binding_check' (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint. Don't rely on
> distro packages for dtschema and be sure you are using the latest
> released dtschema.

BTW, for sure Qualcomm did not develop/create it8987, so it cannot be
used here. Come with specific compatible for this given, one product:
embedded controller on one Lenovo laptop and use it also as filename.

Best regards,
Krzysztof

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

[Maya Matuszczyk]

niedz., 22 gru 2024 o 07:40 Krzysztof Kozlowski <krzk@kernel.org> napisał(a):
>
> On 22/12/2024 07:33, Krzysztof Kozlowski wrote:
> >> +properties:
> >> +  compatible:
> >> +    oneOf:
> >> +      - const: qcom,x1e-it8987-ec
> >
> > That's not a SoC, so I don't understand:
> > 1. referring to the SoC,
> > 2. Having this alone and as fallback.
> >
> > It does not look like you tested the bindings, at least after quick
> > look. Please run 'make dt_binding_check' (see
> > Documentation/devicetree/bindings/writing-schema.rst for instructions).
> > Maybe you need to update your dtschema and yamllint. Don't rely on
> > distro packages for dtschema and be sure you are using the latest
> > released dtschema.
>
> BTW, for sure Qualcomm did not develop/create it8987, so it cannot be
> used here. Come with specific compatible for this given, one product:
> embedded controller on one Lenovo laptop and use it also as filename.

Under these assumptions:

- Qualcomm developed the firmware running on the IT8987 in most x1e machines
- IT8987 is also used in other machines with a non-compatible firmware
- The driver name should reflect the assumptions

I think the name qcom,x1e-it8987-ec is not the worst name for it, as
"ite,it8987-ec" would imply compatibility with other devices running
non-compatible firmware,
and names specifying only the device wouldn't reflect the "firmware is
based on what qcom did and it's driven the same way" part

Which other names do you think would fit this better?

>
> Best regards,
> Krzysztof

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

[Maya Matuszczyk]

niedz., 22 gru 2024 o 08:55 Maya Matuszczyk <maccraft123mc@gmail.com>
napisał(a):
>
> niedz., 22 gru 2024 o 07:40 Krzysztof Kozlowski <krzk@kernel.org> napisał(a):
> >
> > On 22/12/2024 07:33, Krzysztof Kozlowski wrote:
> > >> +properties:
> > >> +  compatible:
> > >> +    oneOf:
> > >> +      - const: qcom,x1e-it8987-ec
> > >
> > > That's not a SoC, so I don't understand:
> > > 1. referring to the SoC,
> > > 2. Having this alone and as fallback.
> > >
> > > It does not look like you tested the bindings, at least after quick
> > > look. Please run 'make dt_binding_check' (see
> > > Documentation/devicetree/bindings/writing-schema.rst for instructions).
> > > Maybe you need to update your dtschema and yamllint. Don't rely on
> > > distro packages for dtschema and be sure you are using the latest
> > > released dtschema.
> >
> > BTW, for sure Qualcomm did not develop/create it8987, so it cannot be
> > used here. Come with specific compatible for this given, one product:
> > embedded controller on one Lenovo laptop and use it also as filename.
>
> Under these assumptions:
>
> - Qualcomm developed the firmware running on the IT8987 in most x1e machines
> - IT8987 is also used in other machines with a non-compatible firmware
> - The driver name should reflect the assumptions
>
> I think the name qcom,x1e-it8987-ec is not the worst name for it, as
> "ite,it8987-ec" would imply compatibility with other devices running
> non-compatible firmware,
> and names specifying only the device wouldn't reflect the "firmware is
> based on what qcom did and it's driven the same way" part
>
> Which other names do you think would fit this better?

In case it wasn't clear:
This was a genuine question - I have no idea how to name this driver
and this was the best I could come up with

>
> >
> > Best regards,
> > Krzysztof

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

[Krzysztof Kozlowski]

On 22/12/2024 08:55, Maya Matuszczyk wrote:
> niedz., 22 gru 2024 o 07:40 Krzysztof Kozlowski <krzk@kernel.org> napisał(a):
>>
>> On 22/12/2024 07:33, Krzysztof Kozlowski wrote:
>>>> +properties:
>>>> +  compatible:
>>>> +    oneOf:
>>>> +      - const: qcom,x1e-it8987-ec
>>>
>>> That's not a SoC, so I don't understand:
>>> 1. referring to the SoC,
>>> 2. Having this alone and as fallback.
>>>
>>> It does not look like you tested the bindings, at least after quick
>>> look. Please run 'make dt_binding_check' (see
>>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>>> Maybe you need to update your dtschema and yamllint. Don't rely on
>>> distro packages for dtschema and be sure you are using the latest
>>> released dtschema.
>>
>> BTW, for sure Qualcomm did not develop/create it8987, so it cannot be
>> used here. Come with specific compatible for this given, one product:
>> embedded controller on one Lenovo laptop and use it also as filename.
> 
> Under these assumptions:
> 
> - Qualcomm developed the firmware running on the IT8987 in most x1e machines

No one here knows whether most x1e machines have this chip...

> - IT8987 is also used in other machines with a non-compatible firmware
> - The driver name should reflect the assumptions

I don't care about driver here, so you can use it for the driver but
these are not correct assumptions for the bindings.



> 
> I think the name qcom,x1e-it8987-ec is not the worst name for it, as
> "ite,it8987-ec" would imply compatibility with other devices running
> non-compatible firmware,
> and names specifying only the device wouldn't reflect the "firmware is
> based on what qcom did and it's driven the same way" part
> 
> Which other names do you think would fit this better?

I suggested the one in second reply:
lenovo,yoga-slim-whatever-model-it-is-ec

If you have any indication that Qualcomm firmware from a reference board
was used as a reference, it could be used as fallback, but I do not
believe you can have such indication considering downstream source code
does not exist and all other docs are confidential. Therefore lenovo EC
is the first implementation we see and we know anything about.


Best regards,
Krzysztof