Re: [PATCH 2/4] dt-bindings: media: qcom,sm8150-camss: Add SM8150 CAMSS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt-bindings: media: qcom,sm8150-camss: fix port endpoint schema and clock-lanes
- [Low] dt-bindings: media: qcom,sm8150-camss: drop internal CAMCC RCG *_src clocks
- [Low] dt-bindings: media: qcom,sm8150-camss: wrong voltage in vdda-pll-supply text
--

commit c84531abb5c3a45299a3b568782087233c82947c
Author: Andrew Gigena <mail@andrewgigena.dev>

dt-bindings: media: qcom,sm8150-camss: Add SM8150 CAMSS

This commit adds the Device Tree binding schema for the Qualcomm SM8150
Camera Subsystem (CAMSS), describing its CSIPHY, CSID, VFE, clock,
interrupt, IOMMU, interconnect, and power domain resources.

> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8150-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm8150-camss.yaml
> new file mode 100644
> index 0000000000000..a65067352b58c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/qcom,sm8150-camss.yaml
> @@ -0,0 +1,477 @@

[ ... ]

> +  clocks:
> +    minItems: 42
> +    maxItems: 42
> +
> +  clock-names:
> +    items:
> +      - const: camnoc_axi
> +      - const: camnoc_axi_src
> +      - const: cpas_ahb
> +      - const: cphy_rx_src
> +      - const: csiphy0
> +      - const: csiphy0_timer
> +      - const: csiphy0_timer_src
> +      - const: csiphy1
> +      - const: csiphy1_timer
> +      - const: csiphy1_timer_src
> +      - const: csiphy2
> +      - const: csiphy2_timer
> +      - const: csiphy2_timer_src
> +      - const: csiphy3
> +      - const: csiphy3_timer
> +      - const: csiphy3_timer_src
> +      - const: vfe0_axi
> +      - const: vfe0
> +      - const: vfe0_src
> +      - const: vfe0_cphy_rx
> +      - const: vfe0_csid
> +      - const: vfe0_csid_src
> +      - const: vfe1_axi
> +      - const: vfe1
> +      - const: vfe1_src
> +      - const: vfe1_cphy_rx
> +      - const: vfe1_csid
> +      - const: vfe1_csid_src
> +      - const: vfe_lite0
> +      - const: vfe_lite0_src
> +      - const: vfe_lite0_cphy_rx
> +      - const: vfe_lite0_csid
> +      - const: vfe_lite0_csid_src
> +      - const: vfe_lite1
> +      - const: vfe_lite1_src
> +      - const: vfe_lite1_cphy_rx
> +      - const: vfe_lite1_csid
> +      - const: vfe_lite1_csid_src
> +      - const: slow_ahb_src
> +      - const: gcc_ahb
> +      - const: gcc_axi_hf
> +      - const: gcc_axi_sf

[Severity: Low]
Should the 15 *_src Root Clock Generator (RCG) clocks be dropped from clocks
and clock-names here?

In drivers/clk/qcom/camcc-sm8150.c, each of the corresponding leaf branch
clocks already wires .parent_hws to its respective *_clk_src RCG with
CLK_SET_RATE_PARENT set:

    static struct clk_branch cam_cc_ife_0_clk = {
        ...
        .clkr = {
            ...
            .hw.init = &(const struct clk_init_data) {
                .name = "cam_cc_ife_0_clk",
                .parent_hws = (const struct clk_hw*[]) {
                    &cam_cc_ife_0_clk_src.clkr.hw,
                },
                .num_parents = 1,
                .flags = CLK_SET_RATE_PARENT,
                .ops = &clk_branch2_ops,
            },
        },
    };

Rate configuration and enablement propagate from the leaf branch clocks
automatically, and the CAMSS driver resources (csiphy_res_sm8150,
csid_res_sm8150, and vfe_res_sm8150) never request any of the *_src clocks.
Other Qualcomm CAMSS bindings with the same CAMCC topology (such as
qcom,sc8280xp-camss.yaml, qcom,sc7280-camss.yaml, qcom,sm6150-camss.yaml, and
qcom,sm6350-camss.yaml) only expose the leaf branch clocks in the binding.

[ ... ]

> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    description:
> +      CSI input ports.
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Input port for receiving CSI data from CSIPHY0.
> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              clock-lanes:
> +                maxItems: 1
> +
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +
> +              bus-type:
> +                enum:
> +                  - 1 # MEDIA_BUS_TYPE_CSI2_CPHY
> +                  - 4 # MEDIA_BUS_TYPE_CSI2_DPHY
> +
> +            required:
> +              - clock-lanes
> +              - data-lanes

[Severity: Medium]
Does requiring clock-lanes under port@0..3.properties.endpoint break C-PHY
endpoints and D-PHY endpoints that omit the hardwired clock lane, while the
example's endpoint@0 node bypasses this schema?

In the example below, port@0 defines an addressed endpoint@0 child node:

    port@0 {
        reg = <0>;
        #address-cells = <1>;
        #size-cells = <0>;

        csiphy_ep0: endpoint@0 {
            reg = <0>;
            clock-lanes = <7>;
            data-lanes = <0 1 2 3>;
            remote-endpoint = <&sensor_ep>;
        };
    };

Because port@0..3 only define properties.endpoint (matching a node literally
named endpoint), endpoint@0 is matched only by graph.yaml#/$defs/port-base
and bypasses the endpoint constraints in this file.

Furthermore, on Qualcomm 3-phase Gen2 CSIPHY blocks
(drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c), the D-PHY clock
lane is hardwired to lane 7 in hardware and is not configurable, while C-PHY
mode (bus-type = <1>, MEDIA_BUS_TYPE_CSI2_CPHY) uses 3-wire trios with an
embedded clock and has no clock lane at all. Sibling CAMSS bindings removed
clock-lanes in commit 336136e197e2 ("media: dt-bindings: media: camss: Remove
clock-lane property") and commit b8ef764aff5be ("dt-bindings: media:
qcom,x1e80100-camss: Remove clock-lanes port property"), and use an
unnumbered endpoint node in the example.

[ ... ]

> +  vdda-phy-supply:
> +    description:
> +      Phandle to a regulator supply to PHY core block.
> +
> +  vdda-pll-supply:
> +    description:
> +      Phandle to 1.8V regulator supply to PHY refclk pll block.

[Severity: Low]
Should the vdda-pll-supply description say 1.2V instead of 1.8V?

In the example below, vdda-phy-supply and vdda-pll-supply use 0.88V and 1.2V
rails:

    vdda-phy-supply = <&vreg_l5a_0p88>;
    vdda-pll-supply = <&vreg_l3c_1p2>;

Commit 94d14ac1f2d3 ("dt-bindings: media: Correct camss supply description")
previously updated the other Qualcomm CAMSS bindings to describe
vdda-phy-supply as the 0.88V supply and vdda-pll-supply as the 1.2V supply to
CSIPHY IP blocks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-sm8150-v7-4-rc1-camss-v1-0-7a9534a9a22f@andrewgigena.dev?part=2