On Fri, Sep 12, 2025 at 05:37:13PM +0800, hehuan1@eswincomputing.com wrote:

You have one clock, why do you need this?

+
+  '#clock-cells':
+    enum: [0]

const: 0

+    description:
+      Specifies how many cells are used when referencing the
+      exported clock from another node. This property indicates
+      that the clock output has no extra parameters and represents
+      the card clock.

This description is not needed.

+
+  eswin,hsp-sp-csr:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - description: Phandle to HSP(High-Speed Peripheral) device
+      - description: Offset of the stability status register for
+                     internal clock
+      - description: Offset of the stability register for host
+                     regulator voltage.
+    description: |
+      High-Speed Peripheral device needed to configure internal
+      clocks, and the power.
+
+  eswin,syscrg-csr:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - description: Phandle to system CRG(System Clock and Reset
+                     Generator) device
+      - description: Offset of core clock control register
+    description: |
+      System Clock and Reset Generator device needed to configure
+      core clock.

This reeks of improper clock tree description. Why can you not just
request the rate that you need via the common clk framework? Likewise
for reset. You already have a clocks property that has to include the
core clock, so I don't see why you need another property to get around
it.

As a result, I'm also suspicious of your hsp-sp-csr, but these at least
appear to be internal clocks if your description is to be believed.
I'd like you to explain exactly what those clocks do and what the "HSP"
actually is. What other peripherals use it?

Also, your driver turns on this hsp clock but never turns it off. Same
for the power.

I want to see the full dts for what you're doing here before I approve
this, there's too much here that looks wrong.

+
+  drive-impedance-ohm:

How come this one has no eswin prefix? Also, the unit is "Ohms", not
"Ohm".

Additionally, any eswin properties should be restricted to eswin devices
only.

How come you're so drastically different to the other devices?
Also, putting "_rst" in a reset name is pointless. These are all resets
after all by nature.

Cheers,
Conor.

+      required:
+        - clock-output-names
+        - '#clock-cells'
+        - eswin,hsp-sp-csr
+        - eswin,syscrg-csr
+        - drive-impedance-ohm
+    else:
+      properties:
+        resets:
+          maxItems: 5
+        reset-names:
+          items:
+            - const: core
+            - const: bus
+            - const: axi
+            - const: block
+            - const: timer
+
   - if:
       properties:
         compatible:
-- 
2.25.1