Re: [PATCH v7 1/3] dt-bindings: i3c: Add support for Qualcomm I3C controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] dt-bindings: i3c: missing i3c child node pattern in qcom,geni-se.yaml
- [Medium] dt-bindings: i3c: required pinctrl-1 contradicts pinctrl-names minItems: 1
- [Medium] dt-bindings: i3c: missing power-domains and operating-points-v2 properties
- [Low] dt-bindings: i3c: example uses QUP wrapper address for IBI_C and mixes SoCs
--

commit 53511a9ac0238442394253f95c17db4e7c486559
Author: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>

dt-bindings: i3c: Add support for Qualcomm I3C controller

This commit adds the device tree binding schema for the Qualcomm GENI-based
I3C controller and its associated In-Band Interrupt (IBI) controller block.

> diff --git a/Documentation/devicetree/bindings/i3c/qcom,geni-i3c.yaml b/Documentation/devicetree/bindings/i3c/qcom,geni-i3c.yaml
> new file mode 100644
> index 0000000000000..bab89aeee23f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i3c/qcom,geni-i3c.yaml
> @@ -0,0 +1,115 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i3c/qcom,geni-i3c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Geni based QUP I3C Controller
> +
> +maintainers:
> +  - Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
> +
> +description:
> +  Qualcomm GENI-based I3C controller implementing an MIPI I3C bus master.
> +  The controller supports SDR transfers in mixed bus mode with both I2C and
> +  I3C target devices present on the same bus.
> +
> +  The controller consists of a GENI Serial Engine (SE) responsible for I3C
> +  bus transactions and an associated dedicated IBI controller(IBI_C) block
> +  used for Hot-Join, In-Band Interrupt (IBI) handling and management of GPII
> +  resources associated with the I3C controller.
> +
> +  I3C controller nodes must be child nodes of a GENI-based Qualcomm Universal
> +  Peripheral(QUP) wrapper. Refer to the GENI QUP wrapper binding described in
> +  Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.yaml.

[Severity: Medium]
Since I3C controller nodes must be child nodes of the GENI QUP wrapper node,
will dtbs_check fail unless Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.yaml
is also updated to permit i3c@[0-9a-f]+$ child nodes?

Currently, qcom,geni-se.yaml sets additionalProperties: false and only allows
spi, i2c, and serial children in patternProperties:

  patternProperties:
    "spi@[0-9a-f]+$":
      ...
    "i2c@[0-9a-f]+$":
      ...
    "serial@[0-9a-f]+$":
      ...
  additionalProperties: false

Should qcom,geni-se.yaml be updated to reference /schemas/i3c/qcom,geni-i3c.yaml#
under patternProperties (and disallow it under qcom,geni-se-i2c-master-hub)?

[ ... ]

> +  pinctrl-names:
> +    minItems: 1
> +    items:
> +      - const: default
> +      - const: sleep
> +
> +  dmas:
> +    maxItems: 2
> +
> +  dma-names:
> +    items:
> +      - const: tx
> +      - const: rx
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - interconnects
> +  - interconnect-names
> +  - pinctrl-0
> +  - pinctrl-1
> +  - pinctrl-names

[Severity: Medium]
Does requiring pinctrl-1 here contradict setting minItems: 1 on pinctrl-names
above?

If a board or SoC DTSI only defines the default pinctrl state (pinctrl-0 and
pinctrl-names = "default"), schema validation will fail because pinctrl-1 is
unconditionally listed in required. In addition, sibling GENI bindings such
as Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml do not
require pinctrl properties in the controller schema.

> +
> +unevaluatedProperties: false

[Severity: Medium]
Should operating-points-v2 and power-domains also be permitted in properties
before setting unevaluatedProperties: false?

Neither i3c.yaml nor qcom,se-common-props.yaml defines these properties,
whereas sibling GENI Serial Engine bindings (qcom,i2c-geni-qcom.yaml,
qcom,spi-geni-qcom.yaml, and qcom,serial-geni-qcom.yaml) declare:

  operating-points-v2: true
  power-domains:
    maxItems: 1

Because Qualcomm SoC DTSI files (such as arch/arm64/boot/dts/qcom/sm8550.dtsi)
populate power-domains and operating-points-v2 on QUP Serial Engine nodes and
the common helpers in drivers/soc/qcom/qcom-geni-se.c consume them, won't
dtbs_check reject those properties on qcom,geni-i3c nodes?

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,sm8550-gcc.h>
> +    #include <dt-bindings/interconnect/qcom,kaanapali-rpmh.h>
> +
> +    i3c@884000 {
> +        compatible = "qcom,geni-i3c";
> +        reg = <0x00884000 0x4000>,
> +              <0x008c0000 0x10000>;
> +        clocks = <&gcc GCC_QUPV3_WRAP2_S1_CLK>;
> +        clock-names = "se";
> +        interrupts = <GIC_SPI 583 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 796 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 797 IRQ_TYPE_LEVEL_HIGH>;
> +        interconnects = <&clk_virt MASTER_QUP_CORE_2 &clk_virt SLAVE_QUP_CORE_2>,
> +                        <&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_2>,
> +                        <&aggre2_noc MASTER_QUP_2 &mc_virt SLAVE_EBI1>;

[Severity: Low]
Is 0x008c0000 the intended register address for the IBI controller in this
example, and are the headers meant to mix sm8550 and kaanapali?

In arch/arm64/boot/dts/qcom/sm8550.dtsi and kaanapali.dtsi, 0x008c0000 is the
register base of the parent QUPv3 wrapper node (geniqup@8c0000):

  qupv3_id_1: geniqup@8c0000 {
          compatible = "qcom,geni-se-qup";
          reg = <0 0x008c0000 0 0x2000>;

which geni_se_probe() in drivers/soc/qcom/qcom-geni-se.c maps via
devm_platform_ioremap_resource(pdev, 0). The example also includes both
qcom,sm8550-gcc.h and qcom,kaanapali-rpmh.h while referencing &aggre2_noc.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104111610.3739095-1-mukesh.savaliya@oss.qualcomm.com?part=1