Re: [PATCH v2 04/24] dt-bindings: clock: Add ZTE ZX279128S CRM clock bindings

["Stefan Dösinger" <stefandoesinger@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4300 bytes --]

Am Mittwoch, 14. Oktober 2026, 09:20:44 Mitteleuropäische Sommerzeit schrieb 
Navid Ghahremani:
> Add Device Tree binding schemas for the ZTE ZX279128S clock controllers:
> topcrm, lspcrpm, and crm2. Define fixed clock lists and simple-mfd
> for child nodes inside topcrm.

This would be easier to review if it were split in 3 patches, one yaml per 
patch. I'll try to comment on the details anyway.

> +properties:
> +  compatible:
> +    oneOf:
> +      - const: zte,zx279128s-crm2
> +      - items:
> +          - const: zte,zx279128s-crm2
> +          - const: syscon

Afaiu this makes "syscon" optional. Why do you need syscon? Why might you not 
need syscon?

The goal should be to avoid "syscon" as much as possible. The meaning of the 
syscon compatible is that a different driver will access these registers 
directly instead of going through a properly exported interface like #clock-
cells. Is there anything in crm2 that is not a clock or reset?

> a/Documentation/devicetree/bindings/clock/zte,zx279128s-lspcrpm.yaml
> b/Documentation/devicetree/bindings/clock/zte,zx279128s-lspcrpm.yaml new

> +maintainers:
> +  - Stefan Dösinger <stefandoesinger@gmail.com>
> +  - Navid Ghahremani <ghahramani.navid@gmail.com>

I think you should be the maintainer alone here, I don't have the hardware to 
do proper testing of patches.

> +description: |
> +  Each of the two groups of low speed peripherals (LSP) has a peripheral
> clock +  module (LSP CRPM) of its own, fed by the top CRM.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - zte,zx279128s-lsp0crpm
> +      - zte,zx279128s-lsp1crpm

I think splitting those into different bindings is still a viable choice. Yeah, 
ZTE gave them very similar names, but from what I can see the two controllers 
have nothing in common.

> +patternProperties:
> +  "^phy@[01]$":
> +    type: object
> +    description: PCIe PHY child nodes

You can drop the phy child node and just add "#phy-cells=1". See https://
docs.kernel.org/devicetree/bindings/writing-bindings.html : "DON’T create 
nodes just for the sake of instantiating drivers. Multi-function devices only 
need child nodes when the child nodes have their own DT resources. A single 
node can be multiple providers (e.g. clocks and resets)."

In practical terms, putting a child node into the DT makes this a public ABI 
contract. The alternative is to split the device with an MFD driver or using 
the aux bus. In the latter case, what components a device consists of is a 
driver implementation detail that can easily be changed later.

The PHY subsystem has a quirk that devm_of_phy_provider_register wants the 
device node to have a DT node, which is not the case if the device is an MFD 
child. device_set_of_node_from_dev(dev, dev->parent); helps.

The other purpose of having a DT child node is if a sub-component is likely to 
be used in a different piece of hardware. syscon-reboot is a pretty generic 
example of that.

> --- /dev/null
> +++ b/include/dt-bindings/clock/zte,zx279128s-crm.h

I'd suggest to rename clock/zte,zx279128s-crm.h to clock/zte,zx279128s-clk.h. 
"CRM" implies clock-reset-manager, but the -reset.h header has the reset 
handles, so the "crm.h" header won't have them.

> +#define ZX279128S_TOP_PLL_A9		0
> +#define ZX279128S_TOP_PLL_LSP		1
> +#define ZX279128S_TOP_CPU		2

Do they all need to be exported? You only need those that will be used for a 
clock = <&topcrm ZX279128S_TOP_SOMETHING> handle. That could e.g. be the CPU 
wclk and pclk, but the parent PLL itself can remain a driver detail. Most 
likely all your device clocks have a gate somewhere. In my zx29 case I only 
export gates from the clock driver, with the exception of the Cortex M0 mux - 
the Cortex M0 can't be turned off, so it has no gate.

I realize that a lot of devices may be missing from the DTSI in patch 23.

> +/* Low speed peripherals 0 */
> +#define ZX279128S_LSP0_UART0_WCLK	0
> +#define ZX279128S_LSP0_UART0_PCLK	1
> +#define ZX279128S_LSP0_UART1_WCLK	2
> +#define ZX279128S_LSP0_UART1_PCLK	3
> +#define ZX279128S_LSP0_SPI_WCLK		4
> +#define ZX279128S_LSP0_SPI_PCLK		5
> +#define ZX279128S_LSP0_GPIO_PCLK	6

E.g. exporting those intuitively makes sense.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 870 bytes --]