Re: [PATCH net-next v1 1/3] dt-bindings: net: Add Motorcomm yt8xxx ethernet phy Driver bindings

From: Frank
Date: Fri Jan 06 2023 - 01:50:59 EST


Hi Andrew,

On 2023/1/5 21:17, Andrew Lunn wrote:
>> + motorcomm,rx-delay-basic:
>> + description: |
>> + Tristate, setup the basic RGMII RX Clock delay of PHY.
>> + This basic delay is fixed at 2ns (1000Mbps) or 8ns (100Mbps、10Mbps).
>> + This basic delay usually auto set by hardware according to the voltage
>> + of RXD0 pin (low = 0, turn off; high = 1, turn on).
>> + If not exist, this delay is controlled by hardware.
>> + 0: turn off; 1: turn on.
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + enum: [0, 1]
>
> Why is this needed? When the MAC driver connects to the PHY, it passes
> phy-mode. For RGMII, this is one of:

> linux/phy.h: PHY_INTERFACE_MODE_RGMII,
> linux/phy.h: PHY_INTERFACE_MODE_RGMII_ID,
> linux/phy.h: PHY_INTERFACE_MODE_RGMII_RXID,
> linux/phy.h: PHY_INTERFACE_MODE_RGMII_TXID,
>
> This tells you if you need to add a delay for the RX clock line, the
> TX clock line, or both. That is all you need to know for basic RGMII
> delays.
>

This basic delay can be controlled by hardware or the phy-mode which
passes from MAC driver.
Default value depends on power on strapping, according to the voltage
of RXD0 pin (low = 0, turn off; high = 1, turn on).

Add this for the case that This basic delay is controlled by hardware,
and software don't change this.

>> + motorcomm,rx-delay-additional-ps:
>
> ethernet-phy.yaml defines rx-internal-delay-ps. Please use that.
>

I will fix.

>> + description: |
>> + Setup the additional RGMII RX Clock delay of PHY defined in pico seconds.
>> + RGMII RX Clock Delay = rx-delay-basic + rx-delay-additional-ps.
>> + enum:
>> + - 0
>> + - 150
>> + - 300
>> + - 450
>> + - 600
>> + - 750
>> + - 900
>> + - 1050
>> + - 1200
>> + - 1350
>> + - 1500
>> + - 1650
>> + - 1800
>> + - 1950
>> + - 2100
>> + - 2250
>
> Is this property mandatory? If not, please document what value is used
> if it is not present.
>

I will fix.

>> +
>> + motorcomm,tx-delay-ge-ps:
>
> tx-internal-delay-ps
>
> And please define the default.
>
>> + motorcomm,tx-delay-fe-ps:
>
> So you can only set the TX delay? What is RX delay set to? Same as 1G?
> I would suggest you call this motorcomm,tx-internal-delay-fe-ps, so
> that it is similar to the standard tx-internal-delay-ps.
>

TX delay has two type: tx-delay-ge-ps for 1G and tx-delay-fe-ps for 100M.

RX delay set for 1G and 100M, but it has two type, rx-delay-basic and
rx-delay-additional-ps, RX delay = rx-delay-basic + rx-delay-additional-ps.

I will rename to tx-internal-delay-fe-ps and tx-internal-delay-ge-ps.

>> + description: |
>> + Setup PHY's RGMII TX Clock delay defined in pico seconds when the speed
>> + is 100Mbps or 10Mbps.
>> + enum:
>> + - 0
>> + - 150
>> + - 300
>> + - 450
>> + - 600
>> + - 750
>> + - 900
>> + - 1050
>> + - 1200
>> + - 1350
>> + - 1500
>> + - 1650
>> + - 1800
>> + - 1950
>> + - 2100
>> + - 2250
>> +
>> + motorcomm,keep-pll-enabled:
>> + description: |
>> + If set, keep the PLL enabled even if there is no link. Useful if you
>> + want to use the clock output without an ethernet link.
>> + type: boolean
>> +
>> + motorcomm,auto-sleep-disabled:
>> + description: |
>> + If set, PHY will not enter sleep mode and close AFE after unplug cable
>> + for a timer.
>> + type: boolean
>
> These two i can see being useful. But everything afterwards seems like
> just copy/paste from vendor SDK for things which the hardware can do,
> but probably nobody ever uses. Do you have a board using any of the
> following properties?
>

tx-clk-adj-enabled, tx-clk-10-inverted, tx-clk-100-inverted and
tx-clk-1000-inverted is used and tested by Yanhong
Wang<yanhong.wang@xxxxxxxxxxxxxxxx>. They used yt8531 on
jh7110-starfive-visionfive-v2. This will provide an additional way to
adjust the tx clk delay on yt8531.

sds-tx-amplitude can be tested on my yt8531s board.

>> +
>> + motorcomm,tx-clk-adj-enabled:
>> + description: |
>> + Useful if you want to use tx-clk-xxxx-inverted to adj the delay of tx clk.
>> + type: boolean
>> +
>> + motorcomm,tx-clk-10-inverted:
>> + description: |
>> + Use original or inverted RGMII Transmit PHY Clock to drive the RGMII
>> + Transmit PHY Clock delay train configuration when speed is 10Mbps.
>> + type: boolean
>> +
>> + motorcomm,tx-clk-100-inverted:
>> + description: |
>> + Use original or inverted RGMII Transmit PHY Clock to drive the RGMII
>> + Transmit PHY Clock delay train configuration when speed is 100Mbps.
>> + type: boolean
>> +
>> + motorcomm,tx-clk-1000-inverted:
>> + description: |
>> + Use original or inverted RGMII Transmit PHY Clock to drive the RGMII
>> + Transmit PHY Clock delay train configuration when speed is 1000Mbps.
>> + type: boolean
>> +
>> + motorcomm,sds-tx-amplitude:
>> + description: |
>> + Setup the tx driver amplitude control of SerDes. Higher amplitude is
>> + helpful for long distance.
>> + 0: low; 1: middle; 2: high.
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + enum: [0, 1, 2]
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> + - |
>> + ethernet {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + ethernet-phy@5 {
>> + reg = <5>;
>
> PHYs are on MDIO busses, so i would expect to see an MDIO bus here,
> not Ethernet.
>

I will fix.

> Andrew