Re: [PATCH net-next 1/3] dt-bindings: mfd: syscon: add ti,am654-gtc

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new compatible "ti,am654-gtc"
  (Documentation/devicetree/bindings/mfd/syscon.yaml, line 134) is named…
- [Low] The subject line "dt-bindings: mfd: syscon: add ti,am654-gtc"
  starts the description after the prefix with a lowercase "add".

--

dt-bindings: mfd: syscon: add ti,am654-gtc

This adds a ti,am654-gtc compatible to the generic syscon binding. It is
for the CFG0 register frame of the TI K3 Global Timebase Counter. The CPTS
driver writes GTC_CFG0_PUSHEVT in that frame to pick the counter bit the
GTC exports as its push event, which provides hardware cross-timestamps.

> dt-bindings: mfd: syscon: add ti,am654-gtc

[Severity: Low]
This isn't a bug, but MFD subjects normally capitalize the description
after the prefix. Recent commits to syscon.yaml do this, for example:

  dt-bindings: mfd: syscon: Add ESWIN EIC7700 compatible

Should this be "dt-bindings: mfd: syscon: Add ti,am654-gtc"?

> diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
> index 80ff0e1caca22..b1030f11b2dbf 100644
> --- a/Documentation/devicetree/bindings/mfd/syscon.yaml
> +++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
> @@ -131,6 +131,7 @@ properties:
>                - ti,am625-dss-oldi-io-ctrl
>                - ti,am62p-cpsw-mac-efuse
>                - ti,am654-dss-oldi-io-ctrl
> +              - ti,am654-gtc

[Severity: Low]
Is ti,am654-gtc the right name for this node? The commit message says it
describes only "the CFG0 register frame" of the GTC. The compatible,
though, is named after the whole GTC IP.

The GTC has more than one register frame. k3-am62l.dtsi maps a
0x34000-byte window for it:

arch/arm64/boot/dts/ti/k3-am62l.dtsi:
    ranges = <0x00 0x00a80000 0x00 0x00a80000 0x00 0x00034000>, /* GTC */

The generic syscon schema allows only one register region and no extra
properties:

  reg:
    maxItems: 1
  [ ... ]
unevaluatedProperties: false

So a node with this compatible can never describe the other GTC frames
or resources.

The syscon.yaml description also says syscon is for registers that are
"not cohesive enough to represent as any specific type of device". The
GTC is a specific counter IP, and PUSHEVT is one of its own functions.

Compatibles become DT ABI once merged. A full GTC binding added later
would then need a different name for the same IP.

Would a frame-specific name such as ti,am654-gtc-cfg0, or a dedicated GTC
binding, describe the hardware more accurately?

The ti,gtc-push property in ti,k3-am654-cpts.yaml, added later in the
series, also describes its target as only the GTC CFG0 syscon. No later
patch in the series changes syscon.yaml, so the naming is the same at the
end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794284248.git.jjc%40jclark.com