Re: [PATCH 06/24] dt-bindings: clock: Add ZTE ZX279128S CRM clocks and resets

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

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

Hi Navid,

I don't have enough knowledge about about PCIe and the A9 l2cache to comment 
on the previous patches. For the clock ones I have a few points of hardware 
investigation and the actual implementation.

Am Samstag, 10. Oktober 2026, 14:59:44 Mitteleuropäische Sommerzeit schrieb 
Navid Ghahremani:

> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - const: zte,zx279128s-topcrm
> +          - const: syscon
> +      - enum:
> +          - zte,zx279128s-lsp0crpm
> +          - zte,zx279128s-lsp1crpm

The advice I got for my zx297520v3 bindings was to write a separate binding 
for the clock controllers rather than try to write a one-size-fits-all one. So 
you'd have a zte,zx279128s-topcrm.yaml that has compatible zte,zx279128s-
topcrm, syscon and a separate lspcrpm.yaml (or maybe even one lsp0crpm and one 
lsp1crpm) that take the other compatible

> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 5
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 5

If you split the bindings the clocks becomes a fixed list in each, simplifying 
the if-then-that rules you have later.
 
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - '#clock-cells'

Your topcrm should have the reboot node as a child. syscon-reboot's "regmap" 
property is deprecated and shouldn't be used for new bindings.

This will either mean adding the simple-mfd compatible to auto-probe the 
reboot child or writing a MFD driver that does that. Since you also have clock 
and reset functionality in there the MFD approach seems best, and it is what I 
ended up doing for my CRM controllers.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: zte,zx279128s-lsp0crpm
> +    then:
> +      properties:
> +        '#reset-cells': false

Are you sure there are no resets in lsp0crpm? Given what I know from my zx29 
board, it is likely that there *are* resets, but the ZTE kernel doesn't bother 
asserting/deasserting them and they come deasserted by the time U-Boot takes 
over. (So either they are deasserted after powerup or the boot rom deasserts 
them.

The way I discovered the rest controls on my board: I set all the registers in 
the LSP controller to 0 via a hack in arch/arm/kernel/head.S and looked at 
what broke. It turned out that there are some bits that need to be set to 1 
for the device to work that aren't mentioned at all in ZTE's code dump.

The follow-up check was to see what the impact of those bits was: Setting a 
pclk gate to 0 usually causes the mmio registers to read 0, 0xff, garbage or 
just hang (busybox's devmem is your friend). A reset bit, when asserted (set 
to 0) does the same. The difference is that when you re-enable a clock, the 
registers retain their old contents, whereas after deasserting a reset, the 
device lost previous state.

Since most devices have 2 reset bits YMMV. One of them might not do anything 
at all. Or one resets bus access properties while the other resets working 
state.

> +++ b/include/dt-bindings/clock/zte,zx279128s-crm.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * Copyright (C) 2026 Navid Ghahremani <ghahramani.navid@gmail.com>
> + */
> +
> +#ifndef _DT_BINDINGS_CLOCK_ZTE_ZX279128S_CRM_H
> +#define _DT_BINDINGS_CLOCK_ZTE_ZX279128S_CRM_H
...
> + * Top CRM resets: (register offset / 4) * 32 + bit. The USB 3.0 controller
> + * has four in register 0x4c; what each one resets isn't known.
> + */
> +#define ZX279128S_TOP_RST_USB_B9	617
> +#define ZX279128S_TOP_RST_USB_B10	618
> +#define ZX279128S_TOP_RST_USB_B11	619
> +#define ZX279128S_TOP_RST_USB_B14	622

Please put the resets in their own reset header.

Using hardware locations (offset, bits) for the binding values is discouraged 
as far as I understand it. I saw a comment by Krzk on a different binding patch 
stating that. I'll try to find it.

It is better to give them logical values (0, 1, 2, ...) and have the reset 
driver map them to the actual hardware location. This also helps if you find 
out that you got a reset bit wrong. Since the bindings are considered ABI, 
changing them later is tricky - you don't want to have to do it because you 
identified a reset incorrectly.

Cheers,
Stefan

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