Re: [PATCH v2 01/10] dt-bindings: ptp: Document the TSCSS on Qualcomm SA8775P and QCS8300 SoCs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] dt-bindings: ptp: qcom,sa8775p-tscss: misuse of pinctrl
  states for ETU slices
- [Medium] dt-bindings: ptp: qcom,sa8775p-tscss: coupling SoC etu reg to
  board properties
- [Low] dt-bindings: ptp: qcom,sa8775p-tscss: missing bounds on slice
  and event arrays
--

commit b07780b80d655036ec2e7df24ec2f89f821cf257
Author: Imran Shaik <imran.shaik@oss.qualcomm.com>

dt-bindings: ptp: Document the TSCSS on Qualcomm SA8775P and QCS8300 SoCs

This commit adds the Devicetree binding schema for the Timestamp Counter
Subsystem (TSCSS) on Qualcomm SA8775P and QCS8300 SoCs.

> diff --git a/Documentation/devicetree/bindings/ptp/qcom,sa8775p-tscss.yaml b/Documentation/devicetree/bindings/ptp/qcom,sa8775p-tscss.yaml
> new file mode 100644
> index 0000000000000..b3e3a7cd71664
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ptp/qcom,sa8775p-tscss.yaml

[ ... ]

> +  reg:
> +    minItems: 1
> +    maxItems: 2
> +
> +  reg-names:
> +    minItems: 1
> +    items:
> +      - const: tsc
> +      - const: etu
> +
> +  clocks:
> +    items:
> +      - description: TSC AHB configuration clock.
> +      - description: TSC global counter clock.
> +      - description: TSC ETU clock.
> +
> +  clock-names:
> +    items:
> +      - const: ahb
> +      - const: cntr
> +      - const: etu
> +
> +  interrupts:
> +    items:
> +      - description:
> +          Summary interrupt raised based on an external event for any
> +          of the ETU slices.
> +
> +  interrupt-names:
> +    items:
> +      - const: etu_summary
> +
> +  pinctrl-names:
> +    minItems: 1
> +    maxItems: 16
> +    items:
> +      pattern: "^slice[0-9]+$"

[Severity: Medium]
Does defining pinctrl-names as per-slice entries ("slice1", "slice2", ...)
and placing each slice's pin into a separate pinctrl-<n> property (as shown
in the second example with pinctrl-0 and pinctrl-1) misuse pinctrl states?

In the Devicetree pinctrl binding and drivers/pinctrl/core.c, pinctrl-0
through pinctrl-N represent mutually exclusive device-wide states such as
"default" and "sleep", not concurrent channels.  When pinctrl_commit_state()
switches to a new state on the device, it calls
pinctrl_cond_disable_mux_setting(old_state, NULL), which disables the pinmux
settings of the previously active slice state so multiple slices cannot be
active at the same time.  Restricting pinctrl-names to "^slice[0-9]+$" also
prevents standard "default" and "sleep" pin states from being used; all
concurrently active ETU slice pins belong in a single state such as
pinctrl-0 with pinctrl-names = "default".

> +  qcom,etu-slice:
> +    description:
> +      ETU slice number for each entry in pinctrl-<n>, pinctrl-names and
> +      qcom,etu-event-sel.
> +    minItems: 1
> +    maxItems: 16
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +
> +  qcom,etu-event-sel:
> +    description:
> +      External event selection value for each ETU slice listed in
> +      qcom,etu-slice.
> +    minItems: 1
> +    maxItems: 16
> +    $ref: /schemas/types.yaml#/definitions/uint32-array

[Severity: Low]
Should qcom,etu-slice and qcom,etu-event-sel define per-item minimum/maximum
bounds (and uniqueItems: true on qcom,etu-slice)?

Without item bounds, out-of-range slice indices or event selector values and
duplicate slice entries will not be caught by dtbs_check.  Also, encoding the
slice index in both pinctrl-names ("^slice[0-9]+$") and qcom,etu-slice
duplicates the slice number across two properties without ensuring they match.

> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +
> +allOf:
> +  - if:
> +      properties:
> +        reg-names:
> +          contains:
> +            const: etu
> +    then:
> +      properties:
> +        reg:
> +          minItems: 2
> +          maxItems: 2
> +      required:
> +        - interrupts
> +        - interrupt-names
> +        - pinctrl-names
> +        - qcom,etu-slice
> +        - qcom,etu-event-sel

[Severity: Medium]
Should the SoC-level etu register region and interrupt be decoupled from the
board-specific ETU slice properties?

The tsc and etu register blocks, clocks, and etu_summary interrupt are fixed
SoC resources, whereas pinctrl-names, qcom,etu-slice, and qcom,etu-event-sel
depend on board pin wiring.  Requiring the board slice properties whenever
reg-names contains "etu" prevents an SoC dtsi from describing the full TSCSS
register map and interrupt unless every board also configures ETU slices.

In addition, this if/then block has no else branch when "etu" is absent,
which allows reg to have 2 entries when reg-names only lists "tsc" and leaves
the ETU properties allowed without the etu register region.  Finally, the
description notes that the TSC block is functional with just the AHB and
counter clocks, yet clocks and clock-names unconditionally require all three
clocks including "etu".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-tscss-v2-0-d5f575010427@oss.qualcomm.com?part=1