Re: [PATCH v2 1/3] dt-bindings: net: Add T-HEAD dwmac support

Krzysztof Kozlowski
2024-09-28T07:28:58Z

On 27/09/2024 22:51, Drew Fustini wrote:
> On Fri, Sep 27, 2024 at 11:34:48AM +0200, Krzysztof Kozlowski wrote:
>> On Thu, Sep 26, 2024 at 11:15:50AM -0700, Drew Fustini wrote:
>>> From: Jisheng Zhang <jszhang@kernel.org>
>>>
>>> Add documentation to describe T-HEAD dwmac.
>>>
>>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>>> Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
>>> [drew: change apb registers from syscon to second reg of gmac node]
>>> [drew: rename compatible, add thead rx/tx internal delay properties]
>>> Signed-off-by: Drew Fustini <dfustini@tenstorrent.com>
>>> ---
>>> .../devicetree/bindings/net/snps,dwmac.yaml | 1 +
>>> .../devicetree/bindings/net/thead,th1520-gmac.yaml | 109 +++++++++++++++++++++
>>> MAINTAINERS | 1 +
>>> 3 files changed, 111 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> index 4e2ba1bf788c..474ade185033 100644
>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> @@ -99,6 +99,7 @@ properties:
>>> - snps,dwxgmac-2.10
>>> - starfive,jh7100-dwmac
>>> - starfive,jh7110-dwmac
>>> + - thead,th1520-gmac
>>>
>>> reg:
>>> minItems: 1
>>> diff --git a/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
>>> new file mode 100644
>>> index 000000000000..1070e891c025
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
>>> @@ -0,0 +1,109 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/net/thead,th1520-gmac.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: T-HEAD TH1520 GMAC Ethernet controller
>>> +
>>> +maintainers:
>>> + - Drew Fustini <dfustini@tenstorrent.com>
>>> +
>>> +description: |
>>> + The TH1520 GMAC is described in the TH1520 Peripheral Interface User Manual
>>> + https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/tree/main/docs
>>> +
>>> + Features include
>>> + - Compliant with IEEE802.3 Specification
>>> + - IEEE 1588-2008 standard for precision networked clock synchronization
>>> + - Supports 10/100/1000Mbps data transfer rate
>>> + - Supports RGMII/MII interface
>>> + - Preamble and start of frame data (SFD) insertion in Transmit path
>>> + - Preamble and SFD deletion in the Receive path
>>> + - Automatic CRC and pad generation options for receive frames
>>> + - MDIO master interface for PHY device configuration and management
>>> +
>>> + The GMAC Registers consists of two parts
>>> + - APB registers are used to configure clock frequency/clock enable/clock
>>> + direction/PHY interface type.
>>> + - AHB registers are use to configure GMAC core (DesignWare Core part).
>>> + GMAC core register consists of DMA registers and GMAC registers.
>>> +
>>> +select:
>>> + properties:
>>> + compatible:
>>> + contains:
>>> + enum:
>>> + - thead,th1520-gmac
>>> + required:
>>> + - compatible
>>> +
>>> +allOf:
>>> + - $ref: snps,dwmac.yaml#
>>> +
>>> +properties:
>>> + compatible:
>>> + items:
>>> + - enum:
>>> + - thead,th1520-gmac
>>> + - const: snps,dwmac-3.70a
>>> +
>>> + reg:
>>> + items:
>>> + - description: DesignWare GMAC IP core registers
>>> + - description: GMAC APB registers
>>> +
>>> + reg-names:
>>> + items:
>>> + - const: dwmac
>>> + - const: apb
>>> +
>>> + thead,rx-internal-delay:
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + description: |
>>> + RGMII receive clock delay. The value is used for the delay_ctrl
>>> + field in GMAC_RXCLK_DELAY_CTRL. Units are not specified.
>>
>> What do you mean by "unspecified units"? They are always specified,
>> hardware does not work randomly, e.g. once uses clock cycles, but next
>> time you run it will use picoseconds.
>>
>> You also miss default (property is not required) and some sort of constraints.
>
> I should have stated that I don't know the units for delay_ctrl. The
> 5-bit field has a max value of 31 which seems far too small for
> picoseconds. Unfortunately, the documentation from the SoC vendor does
> not give anymore details about what the value represents.
>
> Andrew Lunn replied [1] to my cover letter that it is best to hard code
> the field to 0 (which is the hardware reset value) if I don't know what
> the units are for delay_ctrl. The hardware that I have works okay with
> delay_ctrl of 0, so it seems these new vendor properties are not needed.

Then just say that this is using register values directly.

Best regards,
Krzysztof