Re: [PATCH v1 1/2] ethernet: eswin: Document for eic7700 SoC

[Bo Gan <ganboing@gmail.com>]

On 5/15/25 18:10, weishangjuan@eswincomputing.com wrote:> From: Shangjuan Wei <weishangjuan@eswincomputing.com>
> 
> Add ESWIN EIC7700 Ethernet controller, supporting
> multi-rate (10M/100M/1G) auto-negotiation, PHY LED configuration,
> clock/reset control, and AXI bus parameter optimization.
> 
> Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
> Signed-off-by: Shangjuan Wei <weishangjuan@eswincomputing.com>
> ---...> +  # Custom properties
> +  eswin,hsp_sp_csr:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: HSP SP control register> +...> +additionalProperties: false
> +
> +  eswin,syscrg_csr:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: System clock registers
> +
> +  eswin,dly_hsp_reg:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description: HSP delay control registers
...
> +examples:
> +  - |
> +    gmac0: ethernet@50400000 {...> +        dma-noncoherent;
> +        eswin,hsp_sp_csr = <&hsp_sp_csr 0x1030 0x100 0x108>;
> +        eswin,syscrg_csr = <&sys_crg 0x148 0x14c>;
> +        eswin,dly_hsp_reg = <0x114 0x118 0x11c>;

Please help explain the meaning of eswin,<reg> array, and also the expected
number of elements in it, like what starfive did to their JH71x0 device-
tree bindings. E.g., this is what net/starfive,jh7110-dwmac.yaml looks like:

...
   starfive,syscon:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     items:
       - items:
           - description: phandle to syscon that configures phy mode
           - description: Offset of phy mode selection
           - description: Shift of phy mode selection
     description:
       A phandle to syscon with two arguments that configure phy mode.
       The argument one is the offset of phy mode selection, the
       argument two is the shift of phy mode selection.
...

Otherwise, there's no way for people to reason about the driver code.
The same should apply for your sdhci/usb/pcie/... patchsets as well.
Also there's no reference to the first element of the hsp_sp_csr array.
 From the vendor code, I'm reading that you are using the first element
as the register to set the stream ID of the device to tag the memory
transactions for SMMU, but in the patch, there's no mentioning of it.
I'm guessing you are planning to upstream that part later. If so, I
think it's better to put that register index at the end of the array,
and make it optional. It should then be properly documented as well.

Bo