[PATCH net-next v1 0/3] add dts for yt8521 and yt8531s, add driver for yt8531

[PATCH net-next v1 0/3] add dts for yt8521 and yt8531s, add driver for yt8531

[Frank]

Add dts support for yt8521 and yt8531s, add driver for yt8531. These patches have
been tested on our AM335x platform (motherboard) which has one RGMII interface.
It can connect to daughter boards like yt8531s or yt8521 board.
Passed all test cases.

Frank (3):
  dt-bindings: net: Add Motorcomm yt8xxx ethernet phy Driver bindings
  net: phy: Add dts support for Motorcomm yt8521/yt8531s gigabit
    ethernet phy
  net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy

 .../bindings/net/motorcomm,yt8xxx.yaml        | 180 +++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   1 +
 drivers/net/phy/Kconfig                       |   2 +-
 drivers/net/phy/motorcomm.c                   | 638 +++++++++++++++---
 5 files changed, 740 insertions(+), 83 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml

-- 
2.34.1

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

[Frank]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 6394 bytes --]

Add a YAML binding document for the Motorcom yt8xxx Ethernet phy driver.

Signed-off-by: Frank <Frank.Sae@motor-comm.com>
---
 .../bindings/net/motorcomm,yt8xxx.yaml        | 180 ++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   1 +
 3 files changed, 183 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml

diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
new file mode 100644
index 000000000000..337a562d864c
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
@@ -0,0 +1,180 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/motorcomm,yt8xxx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MotorComm yt8xxx Ethernet PHY
+
+maintainers:
+  - frank <frank.sae@motor-comm.com>
+
+description: |
+  Bindings for MotorComm yt8xxx PHYs.
+  yt8511 will be supported later.
+
+allOf:
+  - $ref: ethernet-phy.yaml#
+
+properties:
+  motorcomm,clk-out-frequency:
+    description: clock output in Hertz on clock output pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 25000000, 125000000]
+    default: 0
+
+  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]
+
+  motorcomm,rx-delay-additional-ps:
+    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
+
+  motorcomm,tx-delay-ge-ps:
+    description: |
+      Setup PHY's RGMII TX Clock delay defined in pico seconds when the speed
+      is 1000Mbps.
+    enum:
+      - 0
+      - 150
+      - 300
+      - 450
+      - 600
+      - 750
+      - 900
+      - 1050
+      - 1200
+      - 1350
+      - 1500
+      - 1650
+      - 1800
+      - 1950
+      - 2100
+      - 2250
+
+  motorcomm,tx-delay-fe-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
+
+  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>;
+
+            motorcomm,clk-out-frequency = <0>;
+            #motorcomm,rx-delay-basic = <1>;
+            motorcomm,rx-delay-additional-ps = <0>;
+            motorcomm,tx-delay-fe-ps = <2250>;
+            motorcomm,tx-delay-ge-ps = <150>;
+
+            motorcomm,keep-pll-enabled;
+            motorcomm,auto-sleep-disabled;
+            motorcomm,sds-tx-amplitude = <1>;
+        };
+    };
+  - |
+    ethernet {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        phy-mode = "rgmii-id";
+        ethernet-phy@5 {
+            reg = <5>;
+
+            motorcomm,clk-out-frequency = <125000000>;
+
+            motorcomm,keep-pll-enabled;
+            motorcomm,auto-sleep-disabled;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 70ffb3780621..8d19157e85b7 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -845,6 +845,8 @@ patternProperties:
     description: Moortec Semiconductor Ltd.
   "^mosaixtech,.*":
     description: Mosaix Technologies, Inc.
+  "^motorcomm,.*":
+    description: MotorComm, Inc.
   "^motorola,.*":
     description: Motorola, Inc.
   "^moxa,.*":
diff --git a/MAINTAINERS b/MAINTAINERS
index 7f0b7181e60a..a1e714980154 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14161,6 +14161,7 @@ M:	Peter Geis <pgwipeout@gmail.com>
 M:	Frank <Frank.Sae@motor-comm.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
 F:	drivers/net/phy/motorcomm.c
 
 MOXA SMARTIO/INDUSTIO/INTELLIO SERIAL CARD
-- 
2.34.1

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

[Andrew Lunn]

> +  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.

> +  motorcomm,rx-delay-additional-ps:

ethernet-phy.yaml defines rx-internal-delay-ps. Please use that.

> +    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.

> +
> +  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.

> +    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?

> +
> +  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.

    Andrew

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

[Frank]

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@starfivetech.com>. 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

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

[Andrew Lunn]

> > 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.

You should always do what phy-mode contains. Always. We have had
problems in the past where a PHY driver ignored the phy-mode, and left
the PHY however it was strapped. Which worked. But developers put the
wrong phy-mode value in DT. Then somebody had a board which actually
required that the DT value really did work, because the strapping was
wrong. So the driver was fixed to respect the PHY mode, made that
board work, and broke all the other boards which had the wrong
phy-mode in DT.

If the user want the driver to leave the mode alone, use the
strapping, they should use PHY_INTERFACE_MODE_NA. It is not well
documented, but it is used in a few places. However, i don't recommend
it.

> >> +  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.

So you can set the TX delay for 1G and Fast, but RX delay has a single
setting for both 1G and Fast? Have you seen boards what actually need
different TX delays like this?

Just because the hardware supports something does not mean Linux needs
to support it. Unless there is a real need for it. So i would suggest
your drop this DT property, and set the Fast delay to the same as the
1G delay. If any board actually requires this in the future, the
property can be added then.

> 
> > 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@starfivetech.com>. They used yt8531 on
> jh7110-starfive-visionfive-v2. This will provide an additional way to
> adjust the tx clk delay on yt8531.

O.K. So they are used with a real board. Can we reduce this down to
tx-clk-inverted? Have you ever seen a board which only needs the
invert for one speed and not the others? To me, that would be a very
odd design.

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

Does the board break with the default value? Just because you can test
it on your RDK does not mean anybody will ever use it.

   Andrew

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

[Frank.Sae]

Hi Andrew,

On 2023/1/6 21:55, Andrew Lunn wrote:
>>> 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.
> 
> You should always do what phy-mode contains. Always. We have had
> problems in the past where a PHY driver ignored the phy-mode, and left
> the PHY however it was strapped. Which worked. But developers put the
> wrong phy-mode value in DT. Then somebody had a board which actually
> required that the DT value really did work, because the strapping was
> wrong. So the driver was fixed to respect the PHY mode, made that
> board work, and broke all the other boards which had the wrong
> phy-mode in DT.
> 
> If the user want the driver to leave the mode alone, use the
> strapping, they should use PHY_INTERFACE_MODE_NA. It is not well
> documented, but it is used in a few places. However, i don't recommend
> it.
> 

RX delay = rx-delay-basic (0ns or 1.9ns) + x-delay-additional-ps
(N*150ps, N = 0 ~ 15)
 If rx-delay-basic is removed and controlled by phy-mode.
 when phy-mode is  rgmii-id or rgmii-rxid, RX delay is 1.9ns + N*150ps.
 But sometimes 1.9ns is still too big, we just need  0ns + N*150ps.

For this case, can we do like following ?
rx-internal-delay-ps:
    enum: [ 0, 150, 300, 450, 600, 750, 900, 1050, 1200, 1350, 1500,
1650, 1800, 1900, 1950, 2050, 2100, 2200, 2250, 2350, 2500, 2650, 2800,
2950, 3100, 3250, 3400, 3550, 3700, 3850, 4000, 4150 ]
    default: 0
 rx-internal-delay-ps is 0ns + N*150ps and  1.9ns + N*150ps.
 And check whether need rx-delay-basic (1.9ns) by the val of
rx-internal-delay-ps?

>>>> +  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.
> 
> So you can set the TX delay for 1G and Fast, but RX delay has a single
> setting for both 1G and Fast? Have you seen boards what actually need
> different TX delays like this?
> 
> Just because the hardware supports something does not mean Linux needs
> to support it. Unless there is a real need for it. So i would suggest
> your drop this DT property, and set the Fast delay to the same as the
> 1G delay. If any board actually requires this in the future, the
> property can be added then.
> 
>>
>>> 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@starfivetech.com>. They used yt8531 on
>> jh7110-starfive-visionfive-v2. This will provide an additional way to
>> adjust the tx clk delay on yt8531.
> 
> O.K. So they are used with a real board. Can we reduce this down to
> tx-clk-inverted? Have you ever seen a board which only needs the
> invert for one speed and not the others? To me, that would be a very
> odd design.
> 

We can't reduce this down to tx-clk-inverted.
There are two mac and two yt8531 on their board. Each of yt8531 need
different config in DTS. They need adjust tx clk delay in
link_change_notify callback function according to current speed.

 They configured tx-clk-xxxx-inverted like this :

    speed     GMAC0             GMAC1
    1000M      1                  0		tx-clk-1000-inverted
    100M       1                  1		tx-clk-100-inverted
    10M       0/1                0/1     	tx-clk-10-inverted

Can we put tx-clk-adj-enabled, tx-clk-10-inverted, tx-clk-100-inverted
and tx-clk-1000-inverted in tx-clk-10-inverted like bit in byte?

tx-clk-10-inverted = tx-clk-adj-enabled (bit0),
tx-clk-10-inverted(bit1), tx-clk-100-inverted(bit1) and
tx-clk-1000-inverted(bit2).

>> sds-tx-amplitude can be tested on my yt8531s board.
> 
> Does the board break with the default value? Just because you can test
> it on your RDK does not mean anybody will ever use it.
> 
>    Andrew

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

[Andrew Lunn]

> RX delay = rx-delay-basic (0ns or 1.9ns) + x-delay-additional-ps
> (N*150ps, N = 0 ~ 15)
>  If rx-delay-basic is removed and controlled by phy-mode.
>  when phy-mode is  rgmii-id or rgmii-rxid, RX delay is 1.9ns + N*150ps.
>  But sometimes 1.9ns is still too big, we just need  0ns + N*150ps.
> 
> For this case, can we do like following ?
> rx-internal-delay-ps:
>     enum: [ 0, 150, 300, 450, 600, 750, 900, 1050, 1200, 1350, 1500,
> 1650, 1800, 1900, 1950, 2050, 2100, 2200, 2250, 2350, 2500, 2650, 2800,
> 2950, 3100, 3250, 3400, 3550, 3700, 3850, 4000, 4150 ]
>     default: 0
>  rx-internal-delay-ps is 0ns + N*150ps and  1.9ns + N*150ps.
>  And check whether need rx-delay-basic (1.9ns) by the val of
> rx-internal-delay-ps?

Nothing says delays are only positive. So you could have rgmii-id or
rgmii-rxid and a rx-internal-delay-ps of -150, if you need less than
1.9ns.

As i said, rx-internal-delay-ps is used to fine tune the delay.

> We can't reduce this down to tx-clk-inverted.
> There are two mac and two yt8531 on their board. Each of yt8531 need
> different config in DTS. They need adjust tx clk delay in
> link_change_notify callback function according to current speed.
> 
>  They configured tx-clk-xxxx-inverted like this :
> 
>     speed     GMAC0             GMAC1
>     1000M      1                  0		tx-clk-1000-inverted
>     100M       1                  1		tx-clk-100-inverted
>     10M       0/1                0/1     	tx-clk-10-inverted

What MAC is this? It seems very oddly designed, getting close to
broken. I've not seen any other MAC/PHY combination need anything like
this. 

> Can we put tx-clk-adj-enabled, tx-clk-10-inverted, tx-clk-100-inverted
> and tx-clk-1000-inverted in tx-clk-10-inverted like bit in byte?

No, they are individual boolean properties, so should be kept as they
are. But i really think somebody should be looking deep into the MAC
design to understand why it is like this, and if the MAC can sort out
this mess itself.

	Andrew

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

[Frank.Sae]

Hi Andrew,

On 2023/1/11 21:07, Andrew Lunn wrote:
>> RX delay = rx-delay-basic (0ns or 1.9ns) + x-delay-additional-ps
>> (N*150ps, N = 0 ~ 15)
>>  If rx-delay-basic is removed and controlled by phy-mode.
>>  when phy-mode is  rgmii-id or rgmii-rxid, RX delay is 1.9ns + N*150ps.
>>  But sometimes 1.9ns is still too big, we just need  0ns + N*150ps.
>>
>> For this case, can we do like following ?
>> rx-internal-delay-ps:
>>     enum: [ 0, 150, 300, 450, 600, 750, 900, 1050, 1200, 1350, 1500,
>> 1650, 1800, 1900, 1950, 2050, 2100, 2200, 2250, 2350, 2500, 2650, 2800,
>> 2950, 3100, 3250, 3400, 3550, 3700, 3850, 4000, 4150 ]
>>     default: 0
>>  rx-internal-delay-ps is 0ns + N*150ps and  1.9ns + N*150ps.
>>  And check whether need rx-delay-basic (1.9ns) by the val of
>> rx-internal-delay-ps?
> 
> Nothing says delays are only positive. So you could have rgmii-id or
> rgmii-rxid and a rx-internal-delay-ps of -150, if you need less than
> 1.9ns.
> 
> As i said, rx-internal-delay-ps is used to fine tune the delay.

The standard type of rx-internal-delay-ps is uint32-array, so it can't
be -150.

https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

 "-ps$":
    $ref: types.yaml#/definitions/uint32-array
    description: picosecond


Can we used rx-internal-delay-ps with int32 type?
like this:
  rx-internal-delay-ps:
    $ref: /schemas/types.yaml#/definitions/int32
    enum: [ -1900, -1750, -1600, -1450, -1300, -1150, -1000, -850, -700,
-550, -400, -250, -100, 0, 50, 150, 200, 300, 350, 450, 600, 750, 900,
1050, 1200, 1350, 1500, 1650, 1800, 1950, 2100, 2250 ]
    default: 0


> 
>> We can't reduce this down to tx-clk-inverted.
>> There are two mac and two yt8531 on their board. Each of yt8531 need
>> different config in DTS. They need adjust tx clk delay in
>> link_change_notify callback function according to current speed.
>>
>>  They configured tx-clk-xxxx-inverted like this :
>>
>>     speed     GMAC0             GMAC1
>>     1000M      1                  0		tx-clk-1000-inverted
>>     100M       1                  1		tx-clk-100-inverted
>>     10M       0/1                0/1     	tx-clk-10-inverted
> 
> What MAC is this? It seems very oddly designed, getting close to
> broken. I've not seen any other MAC/PHY combination need anything like
> this. 
> 
>> Can we put tx-clk-adj-enabled, tx-clk-10-inverted, tx-clk-100-inverted
>> and tx-clk-1000-inverted in tx-clk-10-inverted like bit in byte?
> 
> No, they are individual boolean properties, so should be kept as they
> are. But i really think somebody should be looking deep into the MAC
> design to understand why it is like this, and if the MAC can sort out
> this mess itself.
> 
> 	Andrew

Tanks. We will remove tx-clk-xxxx-inverted and tx-clk-adj-enabled.

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

[Andrew Lunn]

> Hi Andrew, i'm an engineer from StarFive Technology Co.
> 
> This configuration is mainly to adapt to VF2 with JH7110 SoC.
> This is a defect in the design of JH7110. Only the basic delay 
> configuration of PHY can't meet the delay needs of JH7110,
> must with the configuration of tx-clk-x-inverted together can
> gmac work normally. Otherwise, gmac cannot work normally at 
> different rates. JH7110 has been taped and cannot be modified 
> in design, so it can only be corrected on software.

So you have a choice of one PHY, since i don't know of any other PHY
with this capability. I hope this is well documented, since PHYs tend
to be considered interchangeable, use whatever is currently the
cheapest. And i assume you will fix the MAC for the next version of
the silicon?

So all these clock invert properties are fine.

I will take another look at the delay property..

  Andrew

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

[Andrew Lunn]

On Wed, Jan 11, 2023 at 05:20:18PM +0800, Frank.Sae wrote:
> Hi Andrew,
> 
> On 2023/1/6 21:55, Andrew Lunn wrote:
> >>> 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.
> > 
> > You should always do what phy-mode contains. Always. We have had
> > problems in the past where a PHY driver ignored the phy-mode, and left
> > the PHY however it was strapped. Which worked. But developers put the
> > wrong phy-mode value in DT. Then somebody had a board which actually
> > required that the DT value really did work, because the strapping was
> > wrong. So the driver was fixed to respect the PHY mode, made that
> > board work, and broke all the other boards which had the wrong
> > phy-mode in DT.
> > 
> > If the user want the driver to leave the mode alone, use the
> > strapping, they should use PHY_INTERFACE_MODE_NA. It is not well
> > documented, but it is used in a few places. However, i don't recommend
> > it.
> > 
> 
> RX delay = rx-delay-basic (0ns or 1.9ns) + x-delay-additional-ps
> (N*150ps, N = 0 ~ 15)
>  If rx-delay-basic is removed and controlled by phy-mode.
>  when phy-mode is  rgmii-id or rgmii-rxid, RX delay is 1.9ns + N*150ps.
>  But sometimes 1.9ns is still too big, we just need  0ns + N*150ps.
> 
> For this case, can we do like following ?
> rx-internal-delay-ps:
>     enum: [ 0, 150, 300, 450, 600, 750, 900, 1050, 1200, 1350, 1500,
> 1650, 1800, 1900, 1950, 2050, 2100, 2200, 2250, 2350, 2500, 2650, 2800,
> 2950, 3100, 3250, 3400, 3550, 3700, 3850, 4000, 4150 ]
>     default: 0
>  rx-internal-delay-ps is 0ns + N*150ps and  1.9ns + N*150ps.
>  And check whether need rx-delay-basic (1.9ns) by the val of
> rx-internal-delay-ps?

Please take a look at phy_get_internal_delay() and the drivers which
use it.

    Andrew

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

[Frank.Sae]

Hi Andrew,

On 2023/1/18 21:40, Andrew Lunn wrote:
> On Wed, Jan 11, 2023 at 05:20:18PM +0800, Frank.Sae wrote:
>> Hi Andrew,
>>
>> On 2023/1/6 21:55, Andrew Lunn wrote:
>>>>> 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.
>>>
>>> You should always do what phy-mode contains. Always. We have had
>>> problems in the past where a PHY driver ignored the phy-mode, and left
>>> the PHY however it was strapped. Which worked. But developers put the
>>> wrong phy-mode value in DT. Then somebody had a board which actually
>>> required that the DT value really did work, because the strapping was
>>> wrong. So the driver was fixed to respect the PHY mode, made that
>>> board work, and broke all the other boards which had the wrong
>>> phy-mode in DT.
>>>
>>> If the user want the driver to leave the mode alone, use the
>>> strapping, they should use PHY_INTERFACE_MODE_NA. It is not well
>>> documented, but it is used in a few places. However, i don't recommend
>>> it.
>>>
>>
>> RX delay = rx-delay-basic (0ns or 1.9ns) + x-delay-additional-ps
>> (N*150ps, N = 0 ~ 15)
>>  If rx-delay-basic is removed and controlled by phy-mode.
>>  when phy-mode is  rgmii-id or rgmii-rxid, RX delay is 1.9ns + N*150ps.
>>  But sometimes 1.9ns is still too big, we just need  0ns + N*150ps.
>>
>> For this case, can we do like following ?
>> rx-internal-delay-ps:
>>     enum: [ 0, 150, 300, 450, 600, 750, 900, 1050, 1200, 1350, 1500,
>> 1650, 1800, 1900, 1950, 2050, 2100, 2200, 2250, 2350, 2500, 2650, 2800,
>> 2950, 3100, 3250, 3400, 3550, 3700, 3850, 4000, 4150 ]
>>     default: 0
>>  rx-internal-delay-ps is 0ns + N*150ps and  1.9ns + N*150ps.
>>  And check whether need rx-delay-basic (1.9ns) by the val of
>> rx-internal-delay-ps?
> 
> Please take a look at phy_get_internal_delay() and the drivers which
> use it.
> 
>     Andrew

 Thanks. But it may be not suitable.

rx-internal-delay-ps has two part:
0ns + N*150ps =
0,150,300,450,600,750,900,1050,1200,1350,1500,1650,1800,1950,2100,2250

1.9ns + N*150ps =
1900,2050,2200,2350,2500,2650,2800,2950,3100,3250,3400,3550,3700,3850,4000,4150

The problem is "1900,2050,2200" is less than "2250".

If I take this two parts in one sorted table, there will be three
tables, one for tx-internal-delay-ps, one for rx-internal-delay-ps and
one for the rx index to reg(delay value and 1.9ns on or off) value.

So we tend to use the following methods.

#define YT8521_CCR_RXC_DLY_1_900_NS		1900

#define YT8521_RC1R_RGMII_0_000_NS		0
#define YT8521_RC1R_RGMII_0_150_NS		1
...
#define	YT8521_RC1R_RGMII_2_250_NS		15

struct ytphy_cfg_reg_map {
	u32 cfg;
	u32 reg;
};

static const struct ytphy_cfg_reg_map ytphy_rgmii_delays[] = {
	/* for tx delay / rx delay with YT8521_CCR_RXC_DLY_EN is not set. */
	{ 0,	YT8521_RC1R_RGMII_0_000_NS },
	{ 150,	YT8521_RC1R_RGMII_0_150_NS },
	...
	{ 2250,	YT8521_RC1R_RGMII_2_250_NS },

	/* only for rx delay with YT8521_CCR_RXC_DLY_EN is set. */
	{ 0    + YT8521_CCR_RXC_DLY_1_900_NS,YT8521_RC1R_RGMII_0_000_NS },
	{ 150  + YT8521_CCR_RXC_DLY_1_900_NS,YT8521_RC1R_RGMII_0_150_NS },
	...
	{ 2250 + YT8521_CCR_RXC_DLY_1_900_NS,YT8521_RC1R_RGMII_2_250_NS }
};


static u32 ytphy_get_delay_reg_value(struct phy_device *phydev,
				     const char *prop_name,
				     const struct ytphy_cfg_reg_map *tbl,
				     int tb_size,
				     u16 *rxc_dly_en,
				     u32 dflt)
{
	struct device_node *node = phydev->mdio.dev.of_node;
	int tb_size_half = tb_size / 2;
	u32 val;
	int i;

	if (of_property_read_u32(node, prop_name, &val))
		return dflt;

	/* when rxc_dly_en is NULL, it is get the delay for tx, only half of
	 * tb_size is valid.
	 */
	if (!rxc_dly_en)
		tb_size = tb_size_half;

	for (i = 0; i < tb_size; i++) {
		if (tbl[i].cfg == val) {
			if (rxc_dly_en && i < tb_size_half)
				*rxc_dly_en = 0;
			return tbl[i].reg;
		}
	}

	phydev_warn(phydev, "Unsupported value %d for %s using default (%u)\n",
		    val, prop_name, dflt);
	return dflt;
}

static int ytphy_rgmii_clk_delay_config(struct phy_device *phydev)
{
	int tb_size = ARRAY_SIZE(ytphy_rgmii_delays);
	u16 rxc_dly_en = YT8521_CCR_RXC_DLY_EN;
	u32 rx_reg, tx_reg;
	u16 mask, val = 0;
	int ret;

	rx_reg = ytphy_get_delay_reg_value(phydev, "rx-internal-delay-ps",
					   ytphy_rgmii_delays, tb_size,
					   &rxc_dly_en,
					   YT8521_RC1R_RGMII_0_000_NS);
	tx_reg = ytphy_get_delay_reg_value(phydev, "tx-internal-delay-ps",
					   ytphy_rgmii_delays, tb_size, NULL,
					   YT8521_RC1R_RGMII_0_150_NS);

	switch (phydev->interface) {
	case PHY_INTERFACE_MODE_RGMII:
		rxc_dly_en = 0;
		break;
	case PHY_INTERFACE_MODE_RGMII_RXID:
		val |= FIELD_PREP(YT8521_RC1R_RX_DELAY_MASK, rx_reg);
		break;
	case PHY_INTERFACE_MODE_RGMII_TXID:
		rxc_dly_en = 0;
		val |= FIELD_PREP(YT8521_RC1R_GE_TX_DELAY_MASK, tx_reg);
		break;
	case PHY_INTERFACE_MODE_RGMII_ID:
		val |= FIELD_PREP(YT8521_RC1R_RX_DELAY_MASK, rx_reg) |
		       FIELD_PREP(YT8521_RC1R_GE_TX_DELAY_MASK, tx_reg);
		break;
	default: /* do not support other modes */
		return -EOPNOTSUPP;
	}

	ret = ytphy_modify_ext(phydev, YT8521_CHIP_CONFIG_REG,
			       YT8521_CCR_RXC_DLY_EN, rxc_dly_en);
	if (ret < 0)
		return ret;

	/* Generally, it is not necessary to adjust YT8521_RC1R_FE_TX_DELAY */
	mask = YT8521_RC1R_RX_DELAY_MASK | YT8521_RC1R_GE_TX_DELAY_MASK;
	return ytphy_modify_ext(phydev, YT8521_RGMII_CONFIG1_REG, mask, val);
}

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

[Krzysztof Kozlowski]

On 05/01/2023 08:30, Frank wrote:
> Add a YAML binding document for the Motorcom yt8xxx Ethernet phy driver.
> 

Subject: drop second, redundant "Driver bindings".

> Signed-off-by: Frank <Frank.Sae@motor-comm.com>

Use full first and last name. Your email suggests something more than
only "Frank".

> ---
>  .../bindings/net/motorcomm,yt8xxx.yaml        | 180 ++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
>  MAINTAINERS                                   |   1 +
>  3 files changed, 183 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
> new file mode 100644
> index 000000000000..337a562d864c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
> @@ -0,0 +1,180 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/motorcomm,yt8xxx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MotorComm yt8xxx Ethernet PHY
> +
> +maintainers:
> +  - frank <frank.sae@motor-comm.com>
> +
> +description: |
> +  Bindings for MotorComm yt8xxx PHYs.

Instead describe the hardware. No need to state the obvious that these
are bindings.

> +  yt8511 will be supported later.

Bindings should be complete. Your driver support is not relevant here.

> +
> +allOf:
> +  - $ref: ethernet-phy.yaml#
> +
> +properties:
> +  motorcomm,clk-out-frequency:

Use property suffixes matching the type.

> +    description: clock output in Hertz on clock output pin.

Drop "Hertz". It should be obvious from the suffix.

> +    $ref: /schemas/types.yaml#/definitions/uint32

Drop.

Anyway, does it fit standard clock-frequency property?

> +    enum: [0, 25000000, 125000000]
> +    default: 0
> +
> +  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.

I don't understand that at all. What "not exist"? There is no verb and
no subject.

The type and description are really unclear.

> +      0: turn off;   1: turn on.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]

So this is bool?

> +
> +  motorcomm,rx-delay-additional-ps:
> +    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:

Best regards,
Krzysztof

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

[Frank.Sae]

Hi Krzysztof Kozlowski,

On 2023/1/6 16:26, Krzysztof Kozlowski wrote:
> On 05/01/2023 08:30, Frank wrote:
>> Add a YAML binding document for the Motorcom yt8xxx Ethernet phy driver.
>>
> 
> Subject: drop second, redundant "Driver bindings".

Change Subject from
dt-bindings: net: Add Motorcomm yt8xxx ethernet phy Driver bindings
to
dt-bindings: net: Add Motorcomm yt8xxx ethernet phy
?

> 
>> Signed-off-by: Frank <Frank.Sae@motor-comm.com>
> 
> Use full first and last name. Your email suggests something more than
> only "Frank".
> 

OK , I will use  Frank.Sae <Frank.Sae@motor-comm.com>

>> ---
>>  .../bindings/net/motorcomm,yt8xxx.yaml        | 180 ++++++++++++++++++
>>  .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
>>  MAINTAINERS                                   |   1 +
>>  3 files changed, 183 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
>> new file mode 100644
>> index 000000000000..337a562d864c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
>> @@ -0,0 +1,180 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/motorcomm,yt8xxx.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MotorComm yt8xxx Ethernet PHY
>> +
>> +maintainers:
>> +  - frank <frank.sae@motor-comm.com>
>> +
>> +description: |
>> +  Bindings for MotorComm yt8xxx PHYs.
> 
> Instead describe the hardware. No need to state the obvious that these
> are bindings.
> 

I will fix.

>> +  yt8511 will be supported later.
> 
> Bindings should be complete. Your driver support is not relevant here.

I will fix.

> 
>> +
>> +allOf:
>> +  - $ref: ethernet-phy.yaml#
>> +
>> +properties:
>> +  motorcomm,clk-out-frequency:
> 
> Use property suffixes matching the type.
> 
>> +    description: clock output in Hertz on clock output pin.
> 
> Drop "Hertz". It should be obvious from the suffix.
> 
>> +    $ref: /schemas/types.yaml#/definitions/uint32
> 
> Drop.
> 
> Anyway, does it fit standard clock-frequency property?
> 
>> +    enum: [0, 25000000, 125000000]
>> +    default: 0
>> +

Yes, I will fix.

>> +  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.
> 
> I don't understand that at all. What "not exist"? There is no verb and
> no subject.
> 
> The type and description are really unclear.
> 
>> +      0: turn off;   1: turn on.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [0, 1]
> 
> So this is bool?
> 

This basic delay can be controlled by hardware or dts.

Default value depends on power on strapping, according to the voltage
of RXD0 pin (low = 0, turn off;   high = 1, turn on).

"not exist" means that This basic delay is controlled by hardware,
and software don't change this.

I will fix.

>> +
>> +  motorcomm,rx-delay-additional-ps:
>> +    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:
> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 06/01/2023 10:17, Frank.Sae wrote:
> Hi Krzysztof Kozlowski,
> 
> On 2023/1/6 16:26, Krzysztof Kozlowski wrote:
>> On 05/01/2023 08:30, Frank wrote:
>>> Add a YAML binding document for the Motorcom yt8xxx Ethernet phy driver.
>>>
>>
>> Subject: drop second, redundant "Driver bindings".
> 
> Change Subject from
> dt-bindings: net: Add Motorcomm yt8xxx ethernet phy Driver bindings
> to
> dt-bindings: net: Add Motorcomm yt8xxx ethernet phy
> ?

Yes.

> 
>>
>>> Signed-off-by: Frank <Frank.Sae@motor-comm.com>
>>
>> Use full first and last name. Your email suggests something more than
>> only "Frank".
>>
> 
> OK , I will use  Frank.Sae <Frank.Sae@motor-comm.com>

Without "dot" between parts of name.

> 
>>> ---
>>>  .../bindings/net/motorcomm,yt8xxx.yaml        | 180 ++++++++++++++++++
>>>  .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
>>>  MAINTAINERS                                   |   1 +
Best regards,
Krzysztof

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

[Andrew Lunn]

> >> +  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.
> > 
> > I don't understand that at all. What "not exist"? There is no verb and
> > no subject.
> > 
> > The type and description are really unclear.
> > 
> >> +      0: turn off;   1: turn on.
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    enum: [0, 1]
> > 
> > So this is bool?
> > 
> 
> This basic delay can be controlled by hardware or dts.
> 
> Default value depends on power on strapping, according to the voltage
> of RXD0 pin (low = 0, turn off;   high = 1, turn on).
> 
> "not exist" means that This basic delay is controlled by hardware,
> and software don't change this.

As i said in my other reply, this is not how Linux controls this. This
property should be removed.

	 Andrew

[PATCH net-next v1 2/3] net: phy: Add dts support for Motorcomm yt8521/yt8531s gigabit ethernet phy

[Frank]

Add dts support for yt8521 and yt8531s. This patch has
been tested on AM335x platform which has one YT8531S interface
card and passed all test cases.

Signed-off-by: Frank <Frank.Sae@motor-comm.com>
---
 drivers/net/phy/motorcomm.c | 517 ++++++++++++++++++++++++++++++------
 1 file changed, 434 insertions(+), 83 deletions(-)

diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
index 685190db72de..7ebcca374a67 100644
--- a/drivers/net/phy/motorcomm.c
+++ b/drivers/net/phy/motorcomm.c
@@ -10,10 +10,11 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/phy.h>
+#include <linux/of.h>
 
 #define PHY_ID_YT8511		0x0000010a
-#define PHY_ID_YT8521		0x0000011A
-#define PHY_ID_YT8531S		0x4F51E91A
+#define PHY_ID_YT8521		0x0000011a
+#define PHY_ID_YT8531S		0x4f51e91a
 
 /* YT8521/YT8531S Register Overview
  *	UTP Register space	|	FIBER Register space
@@ -144,6 +145,16 @@
 #define YT8521_ESC1R_SLEEP_SW			BIT(15)
 #define YT8521_ESC1R_PLLON_SLP			BIT(14)
 
+/* Phy Serdes analog cfg2 Register */
+#define YTPHY_SERDES_ANALOG_CFG2_REG		0xA1
+#define YTPHY_SAC2R_TX_AMPLITUDE_MASK		((0x7 << 13) | (0x7 << 1))
+#define YT8521_SAC2R_TX_AMPLITUDE_LOW		((0x7 << 13) | (0x0 << 1))
+#define YT8521_SAC2R_TX_AMPLITUDE_MIDDLE	((0x5 << 13) | (0x5 << 1))
+#define YT8521_SAC2R_TX_AMPLITUDE_HIGH		((0x3 << 13) | (0x6 << 1))
+#define YT8531S_SAC2R_TX_AMPLITUDE_LOW		((0x0 << 13) | (0x0 << 1))
+#define YT8531S_SAC2R_TX_AMPLITUDE_MIDDLE	((0x0 << 13) | (0x1 << 1))
+#define YT8531S_SAC2R_TX_AMPLITUDE_HIGH		((0x0 << 13) | (0x2 << 1))
+
 /* Phy fiber Link timer cfg2 Register */
 #define YT8521_LINK_TIMER_CFG2_REG		0xA5
 #define YT8521_LTCR_EN_AUTOSEN			BIT(15)
@@ -161,6 +172,7 @@
 
 #define YT8521_CHIP_CONFIG_REG			0xA001
 #define YT8521_CCR_SW_RST			BIT(15)
+#define YT8521_CCR_RXC_DLY_EN			BIT(8)
 
 #define YT8521_CCR_MODE_SEL_MASK		(BIT(2) | BIT(1) | BIT(0))
 #define YT8521_CCR_MODE_UTP_TO_RGMII		0
@@ -178,22 +190,27 @@
 #define YT8521_MODE_POLL			0x3
 
 #define YT8521_RGMII_CONFIG1_REG		0xA003
-
+#define YT8521_RC1R_TX_CLK_SEL_MASK		BIT(14)
+#define YT8521_RC1R_TX_CLK_SEL_ORIGINAL		(0x0 << 14)
+#define YT8521_RC1R_TX_CLK_SEL_INVERTED		(0x1 << 14)
 /* TX Gig-E Delay is bits 3:0, default 0x1
  * TX Fast-E Delay is bits 7:4, default 0xf
  * RX Delay is bits 13:10, default 0x0
  * Delay = 150ps * N
  * On = 2250ps, off = 0ps
  */
-#define YT8521_RC1R_RX_DELAY_MASK		(0xF << 10)
-#define YT8521_RC1R_RX_DELAY_EN			(0xF << 10)
-#define YT8521_RC1R_RX_DELAY_DIS		(0x0 << 10)
-#define YT8521_RC1R_FE_TX_DELAY_MASK		(0xF << 4)
-#define YT8521_RC1R_FE_TX_DELAY_EN		(0xF << 4)
-#define YT8521_RC1R_FE_TX_DELAY_DIS		(0x0 << 4)
-#define YT8521_RC1R_GE_TX_DELAY_MASK		(0xF << 0)
-#define YT8521_RC1R_GE_TX_DELAY_EN		(0xF << 0)
-#define YT8521_RC1R_GE_TX_DELAY_DIS		(0x0 << 0)
+#define YT8521_RC1R_GE_TX_DELAY_BIT		(0)
+#define YT8521_RC1R_FE_TX_DELAY_BIT		(4)
+#define YT8521_RC1R_RX_DELAY_BIT		(10)
+#define YT8521_RC1R_RX_DELAY_MASK		(0xF << YT8521_RC1R_RX_DELAY_BIT)
+#define YT8521_RC1R_RX_DELAY_EN			(0xF << YT8521_RC1R_RX_DELAY_BIT)
+#define YT8521_RC1R_RX_DELAY_DIS		(0x0 << YT8521_RC1R_RX_DELAY_BIT)
+#define YT8521_RC1R_FE_TX_DELAY_MASK		(0xF << YT8521_RC1R_FE_TX_DELAY_BIT)
+#define YT8521_RC1R_FE_TX_DELAY_EN		(0xF << YT8521_RC1R_FE_TX_DELAY_BIT)
+#define YT8521_RC1R_FE_TX_DELAY_DIS		(0x0 << YT8521_RC1R_FE_TX_DELAY_BIT)
+#define YT8521_RC1R_GE_TX_DELAY_MASK		(0xF << YT8521_RC1R_GE_TX_DELAY_BIT)
+#define YT8521_RC1R_GE_TX_DELAY_EN		(0xF << YT8521_RC1R_GE_TX_DELAY_BIT)
+#define YT8521_RC1R_GE_TX_DELAY_DIS		(0x0 << YT8521_RC1R_GE_TX_DELAY_BIT)
 
 #define YTPHY_MISC_CONFIG_REG			0xA006
 #define YTPHY_MCR_FIBER_SPEED_MASK		BIT(0)
@@ -222,11 +239,33 @@
  */
 #define YTPHY_WCR_TYPE_PULSE			BIT(0)
 
-#define YT8531S_SYNCE_CFG_REG			0xA012
-#define YT8531S_SCR_SYNCE_ENABLE		BIT(6)
+#define YTPHY_SYNCE_CFG_REG			0xA012
+#define YT8521_SCR_CLK_SRC_MASK			(BIT(2) | BIT(1))
+#define YT8521_SCR_CLK_SRC_PLL_125M		(0x0 << 1)
+#define YT8521_SCR_CLK_SRC_REF_25M		(0x3 << 1)
+#define YT8521_SCR_SYNCE_ENABLE			BIT(5)
+#define YT8521_SCR_CLK_FRE_SEL_MASK		BIT(3)
+#define YT8521_SCR_CLK_FRE_SEL_125M		(0x1 << 3)
+#define YT8521_SCR_CLK_FRE_SEL_25M		(0x0 << 3)
+#define YT8531_SCR_CLK_SRC_MASK			(BIT(3) | BIT(2) | BIT(1))
+#define YT8531_SCR_CLK_SRC_PLL_125M		(0x0 << 1)
+#define YT8531_SCR_CLK_SRC_REF_25M		(0x4 << 1)
+#define YT8531_SCR_SYNCE_ENABLE			BIT(6)
+#define YT8531_SCR_CLK_FRE_SEL_MASK		BIT(4)
+#define YT8531_SCR_CLK_FRE_SEL_125M		(0x1 << 4)
+#define YT8531_SCR_CLK_FRE_SEL_25M		(0x0 << 4)
 
 /* Extended Register  end */
 
+#define YTPHY_DTS_MAX_TX_AMPLITUDE		0x2
+#define YTPHY_DTS_MAX_DELAY_VAL			2250
+#define YTPHY_DTS_STEP_DELAY_VAL		150
+#define YTPHY_DTS_INVAL_VAL			0xFF
+
+#define YTPHY_DTS_OUTPUT_CLK_DIS		0
+#define YTPHY_DTS_OUTPUT_CLK_25M		25000000
+#define YTPHY_DTS_OUTPUT_CLK_125M		125000000
+
 struct yt8521_priv {
 	/* combo_advertising is used for case of YT8521 in combo mode,
 	 * this means that yt8521 may work in utp or fiber mode which depends
@@ -243,6 +282,30 @@ struct yt8521_priv {
 	 * YT8521_RSSR_TO_BE_ARBITRATED
 	 */
 	u8 reg_page;
+
+	/* The following parameters are from dts */
+	/* rx delay = rx_delay_basic + rx_delay_additional
+	 * basic delay is ~2ns, 0 = off, 1 = on
+	 * rx_delay_additional,delay time = 150ps * val
+	 */
+	u8 rx_delay_basic;
+	u8 rx_delay_additional;
+
+	/* tx_delay_ge is tx_delay for 1000Mbps
+	 * tx_delay_fe is tx_delay for 100Mbps or 10Mbps
+	 * delay time = 150ps * val
+	 */
+	u8 tx_delay_ge;
+	u8 tx_delay_fe;
+	u8 sds_tx_amplitude;
+	bool keep_pll_enabled;
+	bool auto_sleep_disabled;
+	bool clock_ouput;	/* output clock ctl: 0=off, 1=on */
+	bool clock_freq_125M;	/* output clock freq selcect: 0=25M, 1=125M */
+	bool tx_clk_adj_enabled;/* tx clk adj ctl: 0=off, 1=on */
+	bool tx_clk_10_inverted;
+	bool tx_clk_100_inverted;
+	bool tx_clk_1000_inverted;
 };
 
 /**
@@ -593,6 +656,325 @@ static int yt8521_write_page(struct phy_device *phydev, int page)
 	return ytphy_modify_ext(phydev, YT8521_REG_SPACE_SELECT_REG, mask, set);
 };
 
+static int ytphy_parse_dt(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct yt8521_priv *priv = phydev->priv;
+	u32 freq, val;
+	int ret;
+
+	priv->rx_delay_additional = YTPHY_DTS_INVAL_VAL;
+	priv->sds_tx_amplitude = YTPHY_DTS_INVAL_VAL;
+	priv->rx_delay_basic = YTPHY_DTS_INVAL_VAL;
+	priv->tx_delay_ge = YTPHY_DTS_INVAL_VAL;
+	priv->tx_delay_fe = YTPHY_DTS_INVAL_VAL;
+
+	if (!IS_ENABLED(CONFIG_OF_MDIO)) {
+		priv->auto_sleep_disabled = true;
+		priv->keep_pll_enabled = true;
+		return 0;
+	}
+
+	ret = of_property_read_u32(node, "motorcomm,clk-out-frequency", &freq);
+	if (ret < 0)
+		freq = YTPHY_DTS_OUTPUT_CLK_DIS;/* default value as dts*/
+
+	switch (freq) {
+	case YTPHY_DTS_OUTPUT_CLK_DIS:
+		priv->clock_ouput = false;
+		break;
+	case YTPHY_DTS_OUTPUT_CLK_25M:
+		priv->clock_freq_125M = false;
+		priv->clock_ouput = true;
+		break;
+	case YTPHY_DTS_OUTPUT_CLK_125M:
+		priv->clock_freq_125M = true;
+		priv->clock_ouput = true;
+		break;
+	default:
+		phydev_err(phydev, "invalid motorcomm,clk-out-frequency\n");
+		return -EINVAL;
+	}
+
+	if (!of_property_read_u32(node, "motorcomm,rx-delay-basic", &val)) {
+		if (val > 1) {
+			phydev_err(phydev,
+				   "invalid motorcomm,rx-delay-basic\n");
+			return -EINVAL;
+		}
+		priv->rx_delay_basic = val;
+	}
+
+	if (!of_property_read_u32(node, "motorcomm,rx-delay-additional-ps", &val)) {
+		if (val > YTPHY_DTS_MAX_DELAY_VAL) {
+			phydev_err(phydev, "invalid motorcomm,rx-delay-additional-ps\n");
+			return -EINVAL;
+		}
+		if (val)
+			val /= YTPHY_DTS_STEP_DELAY_VAL;
+		priv->rx_delay_additional = val;
+	}
+
+	if (!of_property_read_u32(node, "motorcomm,tx-delay-fe-ps", &val)) {
+		if (val > YTPHY_DTS_MAX_DELAY_VAL) {
+			phydev_err(phydev,
+				   "invalid motorcomm,tx-delay-fe-ps\n");
+			return -EINVAL;
+		}
+		if (val)
+			val /= YTPHY_DTS_STEP_DELAY_VAL;
+		priv->tx_delay_fe = val;
+	}
+
+	if (!of_property_read_u32(node, "motorcomm,tx-delay-ge-ps", &val)) {
+		if (val > YTPHY_DTS_MAX_DELAY_VAL) {
+			phydev_err(phydev,
+				   "invalid motorcomm,tx-delay-ge-ps\n");
+			return -EINVAL;
+		}
+		if (val)
+			val /= YTPHY_DTS_STEP_DELAY_VAL;
+		priv->tx_delay_ge = val;
+	}
+
+	if (of_property_read_bool(node, "motorcomm,keep-pll-enabled"))
+		priv->keep_pll_enabled = true;
+
+	if (of_property_read_bool(node, "motorcomm,auto-sleep-disabled"))
+		priv->auto_sleep_disabled = true;
+
+	if (of_property_read_bool(node, "motorcomm,tx-clk-adj-enabled"))
+		priv->tx_clk_adj_enabled = true;
+	if (priv->tx_clk_adj_enabled) {
+		if (of_property_read_bool(node, "motorcomm,tx-clk-10-inverted"))
+			priv->tx_clk_10_inverted = true;
+		if (of_property_read_bool(node, "motorcomm,tx-clk-100-inverted"))
+			priv->tx_clk_100_inverted = true;
+		if (of_property_read_bool(node, "motorcomm,tx-clk-1000-inverted"))
+			priv->tx_clk_1000_inverted = true;
+	}
+
+	if (!of_property_read_u32(node, "motorcomm,sds-tx-amplitude", &val)) {
+		if (val > YTPHY_DTS_MAX_TX_AMPLITUDE) {
+			phydev_err(phydev,
+				   "invalid motorcomm,sds-tx-amplitude\n");
+			return -EINVAL;
+		}
+		priv->sds_tx_amplitude = val;
+	}
+
+	return 0;
+}
+
+static int ytphy_clk_out_config(struct phy_device *phydev)
+{
+	struct yt8521_priv *priv = phydev->priv;
+	u16 set = 0;
+	u16 mask;
+
+	switch (phydev->drv->phy_id) {
+	case PHY_ID_YT8511:
+		/* YT8511 will be supported later */
+		return -EOPNOTSUPP;
+	case PHY_ID_YT8521:
+		mask = YT8521_SCR_SYNCE_ENABLE;
+		if (priv->clock_ouput) {
+			mask |= YT8521_SCR_CLK_SRC_MASK;
+			mask |= YT8521_SCR_CLK_FRE_SEL_MASK;
+			set |= YT8521_SCR_SYNCE_ENABLE;
+			if (priv->clock_freq_125M) {
+				set |= YT8521_SCR_CLK_FRE_SEL_125M;
+				set |= YT8521_SCR_CLK_SRC_PLL_125M;
+			} else {
+				set |= YT8521_SCR_CLK_FRE_SEL_25M;
+				set |= YT8521_SCR_CLK_SRC_REF_25M;
+			}
+		}
+		break;
+	case PHY_ID_YT8531:
+	case PHY_ID_YT8531S:
+		mask = YT8531_SCR_SYNCE_ENABLE;
+		if (priv->clock_ouput) {
+			mask |= YT8531_SCR_CLK_SRC_MASK;
+			mask |= YT8531_SCR_CLK_FRE_SEL_MASK;
+			set |= YT8531_SCR_SYNCE_ENABLE;
+			if (priv->clock_freq_125M) {
+				set |= YT8531_SCR_CLK_FRE_SEL_125M;
+				set |= YT8531_SCR_CLK_SRC_PLL_125M;
+			} else {
+				set |= YT8531_SCR_CLK_FRE_SEL_25M;
+				set |= YT8531_SCR_CLK_SRC_REF_25M;
+			}
+		}
+		break;
+	default:
+		phydev_err(phydev, "invalid phy id\n");
+		return -EINVAL;
+	}
+
+	return ytphy_modify_ext(phydev, YTPHY_SYNCE_CFG_REG, mask, set);
+}
+
+static int ytphy_serdes_tx_amplitude_config(struct phy_device *phydev)
+{
+	u16 yt8531s_tx_amplitude[] = { YT8531S_SAC2R_TX_AMPLITUDE_LOW,
+				       YT8531S_SAC2R_TX_AMPLITUDE_MIDDLE,
+				       YT8531S_SAC2R_TX_AMPLITUDE_HIGH };
+	u16 yt8521_tx_amplitude[] = { YT8521_SAC2R_TX_AMPLITUDE_LOW,
+				      YT8521_SAC2R_TX_AMPLITUDE_MIDDLE,
+				      YT8521_SAC2R_TX_AMPLITUDE_HIGH };
+	struct yt8521_priv *priv = phydev->priv;
+	u16 tx_amplitude;
+
+	if (priv->sds_tx_amplitude == YTPHY_DTS_INVAL_VAL)
+		/* don't config Serdes tx amplitude.*/
+		return 0;
+
+	switch (phydev->drv->phy_id) {
+	case PHY_ID_YT8511:
+	case PHY_ID_YT8531:
+		/* YT8511 and YT8531 not support this function.*/
+		return -EOPNOTSUPP;
+	case PHY_ID_YT8521:
+		tx_amplitude = yt8521_tx_amplitude[priv->sds_tx_amplitude];
+		break;
+	case PHY_ID_YT8531S:
+		tx_amplitude = yt8531s_tx_amplitude[priv->sds_tx_amplitude];
+		break;
+	default:
+		phydev_err(phydev, "invalid phy id\n");
+		return -EINVAL;
+	}
+
+	return ytphy_modify_ext(phydev, YTPHY_SERDES_ANALOG_CFG2_REG,
+				YTPHY_SAC2R_TX_AMPLITUDE_MASK, tx_amplitude);
+}
+
+static int ytphy_rgmii_clk_delay_config(struct phy_device *phydev)
+{
+	struct yt8521_priv *priv = phydev->priv;
+	u16 mask = 0;
+	u16 val = 0;
+	int ret;
+
+	/* rx delay basic controlled by dts.*/
+	if (priv->rx_delay_basic != YTPHY_DTS_INVAL_VAL) {
+		if (priv->rx_delay_basic)
+			val = YT8521_CCR_RXC_DLY_EN;
+		ret = ytphy_modify_ext(phydev, YT8521_CHIP_CONFIG_REG,
+				       YT8521_CCR_RXC_DLY_EN, val);
+		if (ret < 0)
+			return ret;
+	}
+
+	val = 0;
+	/* If rx_delay_additional and tx_delay_* are all not be seted in dts,
+	 * then used the fixed *_DELAY_DIS or *_DELAY_EN. Otherwise, use the
+	 * value set by rx_delay_additional, tx_delay_ge and tx_delay_fe.
+	 */
+	if ((priv->rx_delay_additional & priv->tx_delay_ge & priv->tx_delay_fe)
+	   == YTPHY_DTS_INVAL_VAL) {
+		switch (phydev->interface) {
+		case PHY_INTERFACE_MODE_RGMII:
+			val |= YT8521_RC1R_GE_TX_DELAY_DIS;
+			val |= YT8521_RC1R_FE_TX_DELAY_DIS;
+			val |= YT8521_RC1R_RX_DELAY_DIS;
+			break;
+		case PHY_INTERFACE_MODE_RGMII_RXID:
+			val |= YT8521_RC1R_GE_TX_DELAY_DIS;
+			val |= YT8521_RC1R_FE_TX_DELAY_DIS;
+			val |= YT8521_RC1R_RX_DELAY_EN;
+			break;
+		case PHY_INTERFACE_MODE_RGMII_TXID:
+			val |= YT8521_RC1R_GE_TX_DELAY_EN;
+			val |= YT8521_RC1R_FE_TX_DELAY_EN;
+			val |= YT8521_RC1R_RX_DELAY_DIS;
+			break;
+		case PHY_INTERFACE_MODE_RGMII_ID:
+			val |= YT8521_RC1R_GE_TX_DELAY_EN;
+			val |= YT8521_RC1R_FE_TX_DELAY_EN;
+			val |= YT8521_RC1R_RX_DELAY_EN;
+			break;
+		default: /* do not support other modes */
+			return -EOPNOTSUPP;
+		}
+		mask = YT8521_RC1R_RX_DELAY_MASK | YT8521_RC1R_FE_TX_DELAY_MASK
+		       | YT8521_RC1R_GE_TX_DELAY_MASK;
+	} else {
+		switch (phydev->interface) {
+		case PHY_INTERFACE_MODE_RGMII:
+		case PHY_INTERFACE_MODE_RGMII_RXID:
+		case PHY_INTERFACE_MODE_RGMII_TXID:
+		case PHY_INTERFACE_MODE_RGMII_ID:
+			if (priv->rx_delay_additional != YTPHY_DTS_INVAL_VAL) {
+				mask |= YT8521_RC1R_RX_DELAY_MASK;
+				val |= (priv->rx_delay_additional) << YT8521_RC1R_RX_DELAY_BIT;
+			}
+			if (priv->tx_delay_fe != YTPHY_DTS_INVAL_VAL) {
+				mask |= YT8521_RC1R_FE_TX_DELAY_MASK;
+				val |= (priv->tx_delay_fe) << YT8521_RC1R_FE_TX_DELAY_BIT;
+			}
+			if (priv->tx_delay_ge != YTPHY_DTS_INVAL_VAL) {
+				mask |= YT8521_RC1R_GE_TX_DELAY_MASK;
+				val |= (priv->tx_delay_ge) << YT8521_RC1R_GE_TX_DELAY_BIT;
+			}
+			break;
+		default: /* do not support other modes */
+			return -EOPNOTSUPP;
+		}
+	}
+
+	return ytphy_modify_ext(phydev, YT8521_RGMII_CONFIG1_REG, mask, val);
+}
+
+static int ytphy_clk_delay_config(struct phy_device *phydev)
+{
+	switch (phydev->drv->phy_id) {
+	case PHY_ID_YT8511:
+		/* YT8511 will be supported later */
+		return -EOPNOTSUPP;
+	case PHY_ID_YT8521:
+	case PHY_ID_YT8531S:
+		/* YT8521 and YT8531S support SGMII mode, but don't need
+		 * delay.
+		 */
+		if (phydev->interface == PHY_INTERFACE_MODE_SGMII)
+			return 0;
+
+		return ytphy_rgmii_clk_delay_config(phydev);
+	case PHY_ID_YT8531:
+		/* YT8531 don't support SGMII mode. */
+		return ytphy_rgmii_clk_delay_config(phydev);
+	default:
+		phydev_err(phydev, "invalid phy id\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ytphy_probe_helper(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct yt8521_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phydev->priv = priv;
+
+	ret = ytphy_parse_dt(phydev);
+	if (ret < 0)
+		return ret;
+
+	phy_lock_mdio_bus(phydev);
+	ret = ytphy_clk_out_config(phydev);
+	phy_unlock_mdio_bus(phydev);
+	return ret;
+}
+
 /**
  * yt8521_probe() - read chip config then set suitable polling_mode
  * @phydev: a pointer to a &struct phy_device
@@ -601,16 +983,15 @@ static int yt8521_write_page(struct phy_device *phydev, int page)
  */
 static int yt8521_probe(struct phy_device *phydev)
 {
-	struct device *dev = &phydev->mdio.dev;
 	struct yt8521_priv *priv;
 	int chip_config;
 	int ret;
 
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
+	ret = ytphy_probe_helper(phydev);
+	if (ret < 0)
+		return ret;
 
-	phydev->priv = priv;
+	priv = phydev->priv;
 
 	chip_config = ytphy_read_ext_with_lock(phydev, YT8521_CHIP_CONFIG_REG);
 	if (chip_config < 0)
@@ -651,26 +1032,6 @@ static int yt8521_probe(struct phy_device *phydev)
 	return 0;
 }
 
-/**
- * yt8531s_probe() - read chip config then set suitable polling_mode
- * @phydev: a pointer to a &struct phy_device
- *
- * returns 0 or negative errno code
- */
-static int yt8531s_probe(struct phy_device *phydev)
-{
-	int ret;
-
-	/* Disable SyncE clock output by default */
-	ret = ytphy_modify_ext_with_lock(phydev, YT8531S_SYNCE_CFG_REG,
-					 YT8531S_SCR_SYNCE_ENABLE, 0);
-	if (ret < 0)
-		return ret;
-
-	/* same as yt8521_probe */
-	return yt8521_probe(phydev);
-}
-
 /**
  * ytphy_utp_read_lpa() - read LPA then setup lp_advertising for utp
  * @phydev: a pointer to a &struct phy_device
@@ -1125,6 +1486,34 @@ static int yt8521_resume(struct phy_device *phydev)
 	return yt8521_modify_utp_fiber_bmcr(phydev, BMCR_PDOWN, 0);
 }
 
+static int ytphy_config_init_helper(struct phy_device *phydev)
+{
+	struct yt8521_priv *priv = phydev->priv;
+	int ret;
+
+	ret = ytphy_clk_delay_config(phydev);
+	if (ret < 0)
+		return ret;
+
+	/* disable auto sleep */
+	if (priv->auto_sleep_disabled) {
+		ret = ytphy_modify_ext(phydev, YT8521_EXTREG_SLEEP_CONTROL1_REG,
+				       YT8521_ESC1R_SLEEP_SW, 0);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* enable RXC clock when no wire plug */
+	if (priv->keep_pll_enabled) {
+		ret = ytphy_modify_ext(phydev, YT8521_CLOCK_GATING_REG,
+				       YT8521_CGR_RX_CLK_EN, 0);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 /**
  * yt8521_config_init() - called to initialize the PHY
  * @phydev: a pointer to a &struct phy_device
@@ -1135,59 +1524,21 @@ static int yt8521_config_init(struct phy_device *phydev)
 {
 	int old_page;
 	int ret = 0;
-	u16 val;
 
 	old_page = phy_select_page(phydev, YT8521_RSSR_UTP_SPACE);
 	if (old_page < 0)
 		goto err_restore_page;
 
-	switch (phydev->interface) {
-	case PHY_INTERFACE_MODE_RGMII:
-		val = YT8521_RC1R_GE_TX_DELAY_DIS | YT8521_RC1R_FE_TX_DELAY_DIS;
-		val |= YT8521_RC1R_RX_DELAY_DIS;
-		break;
-	case PHY_INTERFACE_MODE_RGMII_RXID:
-		val = YT8521_RC1R_GE_TX_DELAY_DIS | YT8521_RC1R_FE_TX_DELAY_DIS;
-		val |= YT8521_RC1R_RX_DELAY_EN;
-		break;
-	case PHY_INTERFACE_MODE_RGMII_TXID:
-		val = YT8521_RC1R_GE_TX_DELAY_EN | YT8521_RC1R_FE_TX_DELAY_EN;
-		val |= YT8521_RC1R_RX_DELAY_DIS;
-		break;
-	case PHY_INTERFACE_MODE_RGMII_ID:
-		val = YT8521_RC1R_GE_TX_DELAY_EN | YT8521_RC1R_FE_TX_DELAY_EN;
-		val |= YT8521_RC1R_RX_DELAY_EN;
-		break;
-	case PHY_INTERFACE_MODE_SGMII:
-		break;
-	default: /* do not support other modes */
-		ret = -EOPNOTSUPP;
-		goto err_restore_page;
-	}
-
-	/* set rgmii delay mode */
-	if (phydev->interface != PHY_INTERFACE_MODE_SGMII) {
-		ret = ytphy_modify_ext(phydev, YT8521_RGMII_CONFIG1_REG,
-				       (YT8521_RC1R_RX_DELAY_MASK |
-				       YT8521_RC1R_FE_TX_DELAY_MASK |
-				       YT8521_RC1R_GE_TX_DELAY_MASK),
-				       val);
-		if (ret < 0)
-			goto err_restore_page;
-	}
-
-	/* disable auto sleep */
-	ret = ytphy_modify_ext(phydev, YT8521_EXTREG_SLEEP_CONTROL1_REG,
-			       YT8521_ESC1R_SLEEP_SW, 0);
+	ret = ytphy_config_init_helper(phydev);
 	if (ret < 0)
 		goto err_restore_page;
 
-	/* enable RXC clock when no wire plug */
-	ret = ytphy_modify_ext(phydev, YT8521_CLOCK_GATING_REG,
-			       YT8521_CGR_RX_CLK_EN, 0);
+	ret = yt8521_write_page(phydev, YT8521_RSSR_FIBER_SPACE);
 	if (ret < 0)
 		goto err_restore_page;
 
+	ret =  ytphy_serdes_tx_amplitude_config(phydev);
+
 err_restore_page:
 	return phy_restore_page(phydev, old_page, ret);
 }
@@ -1778,7 +2129,7 @@ static struct phy_driver motorcomm_phy_drvs[] = {
 		PHY_ID_MATCH_EXACT(PHY_ID_YT8531S),
 		.name		= "YT8531S Gigabit Ethernet",
 		.get_features	= yt8521_get_features,
-		.probe		= yt8531s_probe,
+		.probe		= yt8521_probe,
 		.read_page	= yt8521_read_page,
 		.write_page	= yt8521_write_page,
 		.get_wol	= ytphy_get_wol,
@@ -1804,7 +2155,7 @@ static const struct mdio_device_id __maybe_unused motorcomm_tbl[] = {
 	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8511) },
 	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8521) },
 	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8531S) },
-	{ /* sentinal */ }
+	{ /* sentinel */ }
 };
 
 MODULE_DEVICE_TABLE(mdio, motorcomm_tbl);
-- 
2.34.1

Re: [PATCH net-next v1 2/3] net: phy: Add dts support for Motorcomm yt8521/yt8531s gigabit ethernet phy

[Arun.Ramadoss]

Hi Frank,

On Thu, 2023-01-05 at 15:30 +0800, Frank wrote:
> Add dts support for yt8521 and yt8531s. This patch has
> been tested on AM335x platform which has one YT8531S interface
> card and passed all test cases.

As per the commit message and description, it mentions adding dts
support. But this patch does lot of things. Add elaborate description
or split the patch logically. 

> 
> Signed-off-by: Frank <Frank.Sae@motor-comm.com>
> ---
>  drivers/net/phy/motorcomm.c | 517 ++++++++++++++++++++++++++++++--
> ----
>  1 file changed, 434 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/net/phy/motorcomm.c
> b/drivers/net/phy/motorcomm.c
> index 685190db72de..7ebcca374a67 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c
> @@ -10,10 +10,11 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/phy.h>
> +#include <linux/of.h>
>  
>  #define PHY_ID_YT8511		0x0000010a
> -#define PHY_ID_YT8521		0x0000011A
> -#define PHY_ID_YT8531S		0x4F51E91A
> +#define PHY_ID_YT8521		0x0000011a
> +#define PHY_ID_YT8531S		0x4f51e91a
>  
>  /* YT8521/YT8531S Register Overview
>   *	UTP Register space	|	FIBER Register space
> @@ -144,6 +145,16 @@
>  #define YT8521_ESC1R_SLEEP_SW			BIT(15)
>  #define YT8521_ESC1R_PLLON_SLP			BIT(14)
>  
> +/* Phy Serdes analog cfg2 Register */
> +#define YTPHY_SERDES_ANALOG_CFG2_REG		0xA1
> +#define YTPHY_SAC2R_TX_AMPLITUDE_MASK		((0x7 << 13) |
> (0x7 << 1))
> +#define YT8521_SAC2R_TX_AMPLITUDE_LOW		((0x7 << 13) |
> (0x0 << 1))
> +#define YT8521_SAC2R_TX_AMPLITUDE_MIDDLE	((0x5 << 13) | (0x5 <<
> 1))
> +#define YT8521_SAC2R_TX_AMPLITUDE_HIGH		((0x3 << 13) |
> (0x6 << 1))
> +#define YT8531S_SAC2R_TX_AMPLITUDE_LOW		((0x0 << 13) |
> (0x0 << 1))
> +#define YT8531S_SAC2R_TX_AMPLITUDE_MIDDLE	((0x0 << 13) | (0x1 <<
> 1))
> +#define YT8531S_SAC2R_TX_AMPLITUDE_HIGH		((0x0 << 13) |
> (0x2 << 1))
> +
>  /* Phy fiber Link timer cfg2 Register */
>  #define YT8521_LINK_TIMER_CFG2_REG		0xA5
>  #define YT8521_LTCR_EN_AUTOSEN			BIT(15)
> @@ -161,6 +172,7 @@
>  
>  #define YT8521_CHIP_CONFIG_REG			0xA001
>  #define YT8521_CCR_SW_RST			BIT(15)
> +#define YT8521_CCR_RXC_DLY_EN			BIT(8)
>  
>  #define YT8521_CCR_MODE_SEL_MASK		(BIT(2) | BIT(1) |
> BIT(0))
>  #define YT8521_CCR_MODE_UTP_TO_RGMII		0
> @@ -178,22 +190,27 @@
>  #define YT8521_MODE_POLL			0x3
>  
>  #define YT8521_RGMII_CONFIG1_REG		0xA003
> -
> +#define YT8521_RC1R_TX_CLK_SEL_MASK		BIT(14)
> +#define YT8521_RC1R_TX_CLK_SEL_ORIGINAL		(0x0 << 14)
> +#define YT8521_RC1R_TX_CLK_SEL_INVERTED		(0x1 << 14)
>  /* TX Gig-E Delay is bits 3:0, default 0x1
>   * TX Fast-E Delay is bits 7:4, default 0xf
>   * RX Delay is bits 13:10, default 0x0
>   * Delay = 150ps * N
>   * On = 2250ps, off = 0ps
>   */
> -#define YT8521_RC1R_RX_DELAY_MASK		(0xF << 10)
> -#define YT8521_RC1R_RX_DELAY_EN			(0xF << 10)
> -#define YT8521_RC1R_RX_DELAY_DIS		(0x0 << 10)
> -#define YT8521_RC1R_FE_TX_DELAY_MASK		(0xF << 4)
> -#define YT8521_RC1R_FE_TX_DELAY_EN		(0xF << 4)
> -#define YT8521_RC1R_FE_TX_DELAY_DIS		(0x0 << 4)
> -#define YT8521_RC1R_GE_TX_DELAY_MASK		(0xF << 0)
> -#define YT8521_RC1R_GE_TX_DELAY_EN		(0xF << 0)
> -#define YT8521_RC1R_GE_TX_DELAY_DIS		(0x0 << 0)
> +#define YT8521_RC1R_GE_TX_DELAY_BIT		(0)
> +#define YT8521_RC1R_FE_TX_DELAY_BIT		(4)
> +#define YT8521_RC1R_RX_DELAY_BIT		(10)
> +#define YT8521_RC1R_RX_DELAY_MASK		(0xF <<
> YT8521_RC1R_RX_DELAY_BIT)
> +#define YT8521_RC1R_RX_DELAY_EN			(0xF <<
> YT8521_RC1R_RX_DELAY_BIT)
> +#define YT8521_RC1R_RX_DELAY_DIS		(0x0 <<
> YT8521_RC1R_RX_DELAY_BIT)
> +#define YT8521_RC1R_FE_TX_DELAY_MASK		(0xF <<
> YT8521_RC1R_FE_TX_DELAY_BIT)
> +#define YT8521_RC1R_FE_TX_DELAY_EN		(0xF <<
> YT8521_RC1R_FE_TX_DELAY_BIT)
> +#define YT8521_RC1R_FE_TX_DELAY_DIS		(0x0 <<
> YT8521_RC1R_FE_TX_DELAY_BIT)
> +#define YT8521_RC1R_GE_TX_DELAY_MASK		(0xF <<
> YT8521_RC1R_GE_TX_DELAY_BIT)
> +#define YT8521_RC1R_GE_TX_DELAY_EN		(0xF <<
> YT8521_RC1R_GE_TX_DELAY_BIT)
> +#define YT8521_RC1R_GE_TX_DELAY_DIS		(0x0 <<
> YT8521_RC1R_GE_TX_DELAY_BIT)
> 

This can be splitted as preparatory patch like using BIT macro instead
of magic number.

>  
>  #define YTPHY_MISC_CONFIG_REG			0xA006
>  #define YTPHY_MCR_FIBER_SPEED_MASK		BIT(0)
> @@ -222,11 +239,33 @@
>   */
>  #define YTPHY_WCR_TYPE_PULSE			BIT(0)
>  
> -#define YT8531S_SYNCE_CFG_REG			0xA012
> -#define YT8531S_SCR_SYNCE_ENABLE		BIT(6)
> +#define YTPHY_SYNCE_CFG_REG			0xA012
> +#define YT8521_SCR_CLK_SRC_MASK			(BIT(2) |
> BIT(1))

For the mask, you can consider using GENMASK macro

> +#define YT8521_SCR_CLK_SRC_PLL_125M		(0x0 << 1)
> +#define YT8521_SCR_CLK_SRC_REF_25M		(0x3 << 1)
> +#define YT8521_SCR_SYNCE_ENABLE			BIT(5)
> +#define YT8521_SCR_CLK_FRE_SEL_MASK		BIT(3)
> +#define YT8521_SCR_CLK_FRE_SEL_125M		(0x1 << 3)
> +#define YT8521_SCR_CLK_FRE_SEL_25M		(0x0 << 3)
> +#define YT8531_SCR_CLK_SRC_MASK			(BIT(3) |
> BIT(2) | BIT(1))
> +#define YT8531_SCR_CLK_SRC_PLL_125M		(0x0 << 1)
> +#define YT8531_SCR_CLK_SRC_REF_25M		(0x4 << 1)
> +#define YT8531_SCR_SYNCE_ENABLE			BIT(6)
> +#define YT8531_SCR_CLK_FRE_SEL_MASK		BIT(4)
> +#define YT8531_SCR_CLK_FRE_SEL_125M		(0x1 << 4)
> +#define YT8531_SCR_CLK_FRE_SEL_25M		(0x0 << 4)
>  
>   
> +
> +static int ytphy_clk_out_config(struct phy_device *phydev)
> +{
> +	struct yt8521_priv *priv = phydev->priv;
> +	u16 set = 0;
> +	u16 mask;
> +
> +	switch (phydev->drv->phy_id) {
> +	case PHY_ID_YT8511:
> +		/* YT8511 will be supported later */
> +		return -EOPNOTSUPP;
> +	case PHY_ID_YT8521:
> +		mask = YT8521_SCR_SYNCE_ENABLE;
> +		if (priv->clock_ouput) {
> +			mask |= YT8521_SCR_CLK_SRC_MASK;
> +			mask |= YT8521_SCR_CLK_FRE_SEL_MASK;

You can consider assigning mask in single statement.

> +			set |= YT8521_SCR_SYNCE_ENABLE;
> +			if (priv->clock_freq_125M) {
> +				set |= YT8521_SCR_CLK_FRE_SEL_125M;
> +				set |= YT8521_SCR_CLK_SRC_PLL_125M;

Similarly here.

> +			} else {
> +				set |= YT8521_SCR_CLK_FRE_SEL_25M;
> +				set |= YT8521_SCR_CLK_SRC_REF_25M;
> +			}
> +		}
> +		break;
> +	case PHY_ID_YT8531:
> +	case PHY_ID_YT8531S:
> +		mask = YT8531_SCR_SYNCE_ENABLE;
> +		if (priv->clock_ouput) {
> +			mask |= YT8531_SCR_CLK_SRC_MASK;
> +			mask |= YT8531_SCR_CLK_FRE_SEL_MASK;
> +			set |= YT8531_SCR_SYNCE_ENABLE;
> +			if (priv->clock_freq_125M) {
> +				set |= YT8531_SCR_CLK_FRE_SEL_125M;
> +				set |= YT8531_SCR_CLK_SRC_PLL_125M;
> +			} else {
> +				set |= YT8531_SCR_CLK_FRE_SEL_25M;
> +				set |= YT8531_SCR_CLK_SRC_REF_25M;
> +			}
> +		}
> +		break;
> +	default:
> +		phydev_err(phydev, "invalid phy id\n");
> +		return -EINVAL;
> +	}
> +
> +	return ytphy_modify_ext(phydev, YTPHY_SYNCE_CFG_REG, mask,
> set);
> +}
> +
> ++static int ytphy_rgmii_clk_delay_config(struct phy_device *phydev)
> +{
> +	struct yt8521_priv *priv = phydev->priv;
> +	u16 mask = 0;
> +	u16 val = 0;
> +	int ret;
> +
> +	/* rx delay basic controlled by dts.*/
> +	if (priv->rx_delay_basic != YTPHY_DTS_INVAL_VAL) {
> +		if (priv->rx_delay_basic)
> +			val = YT8521_CCR_RXC_DLY_EN;
> +		ret = ytphy_modify_ext(phydev, YT8521_CHIP_CONFIG_REG,
> +				       YT8521_CCR_RXC_DLY_EN, val);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	val = 0;
> +	/* If rx_delay_additional and tx_delay_* are all not be seted
> in dts,
> +	 * then used the fixed *_DELAY_DIS or *_DELAY_EN. Otherwise,
> use the
> +	 * value set by rx_delay_additional, tx_delay_ge and
> tx_delay_fe.
> +	 */
> +	if ((priv->rx_delay_additional & priv->tx_delay_ge & priv-
> >tx_delay_fe)
> +	   == YTPHY_DTS_INVAL_VAL) {
> +		switch (phydev->interface) {
> +		case PHY_INTERFACE_MODE_RGMII:
> +			val |= YT8521_RC1R_GE_TX_DELAY_DIS;
> +			val |= YT8521_RC1R_FE_TX_DELAY_DIS;
> +			val |= YT8521_RC1R_RX_DELAY_DIS;

Single statement would be suffice.

> +			break;
> +		case PHY_INTERFACE_MODE_RGMII_RXID:
> +			val |= YT8521_RC1R_GE_TX_DELAY_DIS;
> +			val |= YT8521_RC1R_FE_TX_DELAY_DIS;
> +			val |= YT8521_RC1R_RX_DELAY_EN;
> +			break;
> +		case PHY_INTERFACE_MODE_RGMII_TXID:
> +			val |= YT8521_RC1R_GE_TX_DELAY_EN;
> +			val |= YT8521_RC1R_FE_TX_DELAY_EN;
> +			val |= YT8521_RC1R_RX_DELAY_DIS;
> +			break;
> +		case PHY_INTERFACE_MODE_RGMII_ID:
> +			val |= YT8521_RC1R_GE_TX_DELAY_EN;
> +			val |= YT8521_RC1R_FE_TX_DELAY_EN;
> +			val |= YT8521_RC1R_RX_DELAY_EN;
> +			break;
> +		default: /* do not support other modes */
> +			return -EOPNOTSUPP;
> +		}
> +		mask = YT8521_RC1R_RX_DELAY_MASK |
> YT8521_RC1R_FE_TX_DELAY_MASK
> +		       | YT8521_RC1R_GE_TX_DELAY_MASK;
> +	} 
> +
>  
>  /**
>   * ytphy_utp_read_lpa() - read LPA then setup lp_advertising for utp
>   * @phydev: a pointer to a &struct phy_device
> @@ -1125,6 +1486,34 @@ static int yt8521_resume(struct phy_device
> *phydev)
>  	return yt8521_modify_utp_fiber_bmcr(phydev, BMCR_PDOWN, 0);
>  }
>  
> 
> @@ -1778,7 +2129,7 @@ static struct phy_driver motorcomm_phy_drvs[] =
> {
>  		PHY_ID_MATCH_EXACT(PHY_ID_YT8531S),
>  		.name		= "YT8531S Gigabit Ethernet",
>  		.get_features	= yt8521_get_features,
> -		.probe		= yt8531s_probe,
> +		.probe		= yt8521_probe,
>  		.read_page	= yt8521_read_page,
>  		.write_page	= yt8521_write_page,
>  		.get_wol	= ytphy_get_wol,
> @@ -1804,7 +2155,7 @@ static const struct mdio_device_id
> __maybe_unused motorcomm_tbl[] = {
>  	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8511) },
>  	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8521) },
>  	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8531S) },
> -	{ /* sentinal */ }
> +	{ /* sentinel */ }

It should go as separate patch.
>  };
>  
>  MODULE_DEVICE_TABLE(mdio, motorcomm_tbl);

Re: [PATCH net-next v1 2/3] net: phy: Add dts support for Motorcomm yt8521/yt8531s gigabit ethernet phy

[Frank]

Hi Arun Ramadoss,

On 2023/1/5 17:01, Arun.Ramadoss@microchip.com wrote:
> Hi Frank,
>
> On Thu, 2023-01-05 at 15:30 +0800, Frank wrote:
>> Add dts support for yt8521 and yt8531s. This patch has
>> been tested on AM335x platform which has one YT8531S interface
>> card and passed all test cases.
>
> As per the commit message and description, it mentions adding dts
> support. But this patch does lot of things. Add elaborate description
> or split the patch logically.
>

I will fix.

>>
>> Signed-off-by: Frank <Frank.Sae@motor-comm.com>
>> ---
>>  drivers/net/phy/motorcomm.c | 517 ++++++++++++++++++++++++++++++--
>> ----
>>  1 file changed, 434 insertions(+), 83 deletions(-)
>>
>> diff --git a/drivers/net/phy/motorcomm.c
>> b/drivers/net/phy/motorcomm.c
>> index 685190db72de..7ebcca374a67 100644
>> --- a/drivers/net/phy/motorcomm.c
>> +++ b/drivers/net/phy/motorcomm.c
>> @@ -10,10 +10,11 @@
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>>  #include <linux/phy.h>
>> +#include <linux/of.h>
>>
>>  #define PHY_ID_YT8511		0x0000010a
>> -#define PHY_ID_YT8521		0x0000011A
>> -#define PHY_ID_YT8531S		0x4F51E91A
>> +#define PHY_ID_YT8521		0x0000011a
>> +#define PHY_ID_YT8531S		0x4f51e91a
>>
>>  /* YT8521/YT8531S Register Overview
>>   *	UTP Register space	|	FIBER Register space
>> @@ -144,6 +145,16 @@
>>  #define YT8521_ESC1R_SLEEP_SW			BIT(15)
>>  #define YT8521_ESC1R_PLLON_SLP			BIT(14)
>>
>> +/* Phy Serdes analog cfg2 Register */
>> +#define YTPHY_SERDES_ANALOG_CFG2_REG		0xA1
>> +#define YTPHY_SAC2R_TX_AMPLITUDE_MASK		((0x7 << 13) |
>> (0x7 << 1))
>> +#define YT8521_SAC2R_TX_AMPLITUDE_LOW		((0x7 << 13) |
>> (0x0 << 1))
>> +#define YT8521_SAC2R_TX_AMPLITUDE_MIDDLE	((0x5 << 13) | (0x5 <<
>> 1))
>> +#define YT8521_SAC2R_TX_AMPLITUDE_HIGH		((0x3 << 13) |
>> (0x6 << 1))
>> +#define YT8531S_SAC2R_TX_AMPLITUDE_LOW		((0x0 << 13) |
>> (0x0 << 1))
>> +#define YT8531S_SAC2R_TX_AMPLITUDE_MIDDLE	((0x0 << 13) | (0x1 <<
>> 1))
>> +#define YT8531S_SAC2R_TX_AMPLITUDE_HIGH		((0x0 << 13) |
>> (0x2 << 1))
>> +
>>  /* Phy fiber Link timer cfg2 Register */
>>  #define YT8521_LINK_TIMER_CFG2_REG		0xA5
>>  #define YT8521_LTCR_EN_AUTOSEN			BIT(15)
>> @@ -161,6 +172,7 @@
>>
>>  #define YT8521_CHIP_CONFIG_REG			0xA001
>>  #define YT8521_CCR_SW_RST			BIT(15)
>> +#define YT8521_CCR_RXC_DLY_EN			BIT(8)
>>
>>  #define YT8521_CCR_MODE_SEL_MASK		(BIT(2) | BIT(1) |
>> BIT(0))
>>  #define YT8521_CCR_MODE_UTP_TO_RGMII		0
>> @@ -178,22 +190,27 @@
>>  #define YT8521_MODE_POLL			0x3
>>
>>  #define YT8521_RGMII_CONFIG1_REG		0xA003
>> -
>> +#define YT8521_RC1R_TX_CLK_SEL_MASK		BIT(14)
>> +#define YT8521_RC1R_TX_CLK_SEL_ORIGINAL		(0x0 << 14)
>> +#define YT8521_RC1R_TX_CLK_SEL_INVERTED		(0x1 << 14)
>>  /* TX Gig-E Delay is bits 3:0, default 0x1
>>   * TX Fast-E Delay is bits 7:4, default 0xf
>>   * RX Delay is bits 13:10, default 0x0
>>   * Delay = 150ps * N
>>   * On = 2250ps, off = 0ps
>>   */
>> -#define YT8521_RC1R_RX_DELAY_MASK		(0xF << 10)
>> -#define YT8521_RC1R_RX_DELAY_EN			(0xF << 10)
>> -#define YT8521_RC1R_RX_DELAY_DIS		(0x0 << 10)
>> -#define YT8521_RC1R_FE_TX_DELAY_MASK		(0xF << 4)
>> -#define YT8521_RC1R_FE_TX_DELAY_EN		(0xF << 4)
>> -#define YT8521_RC1R_FE_TX_DELAY_DIS		(0x0 << 4)
>> -#define YT8521_RC1R_GE_TX_DELAY_MASK		(0xF << 0)
>> -#define YT8521_RC1R_GE_TX_DELAY_EN		(0xF << 0)
>> -#define YT8521_RC1R_GE_TX_DELAY_DIS		(0x0 << 0)
>> +#define YT8521_RC1R_GE_TX_DELAY_BIT		(0)
>> +#define YT8521_RC1R_FE_TX_DELAY_BIT		(4)
>> +#define YT8521_RC1R_RX_DELAY_BIT		(10)
>> +#define YT8521_RC1R_RX_DELAY_MASK		(0xF <<
>> YT8521_RC1R_RX_DELAY_BIT)
>> +#define YT8521_RC1R_RX_DELAY_EN			(0xF <<
>> YT8521_RC1R_RX_DELAY_BIT)
>> +#define YT8521_RC1R_RX_DELAY_DIS		(0x0 <<
>> YT8521_RC1R_RX_DELAY_BIT)
>> +#define YT8521_RC1R_FE_TX_DELAY_MASK		(0xF <<
>> YT8521_RC1R_FE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_FE_TX_DELAY_EN		(0xF <<
>> YT8521_RC1R_FE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_FE_TX_DELAY_DIS		(0x0 <<
>> YT8521_RC1R_FE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_GE_TX_DELAY_MASK		(0xF <<
>> YT8521_RC1R_GE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_GE_TX_DELAY_EN		(0xF <<
>> YT8521_RC1R_GE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_GE_TX_DELAY_DIS		(0x0 <<
>> YT8521_RC1R_GE_TX_DELAY_BIT)
>>
>
> This can be splitted as preparatory patch like using BIT macro instead
> of magic number.
>

I will fix.

>>
>>  #define YTPHY_MISC_CONFIG_REG			0xA006
>>  #define YTPHY_MCR_FIBER_SPEED_MASK		BIT(0)
>> @@ -222,11 +239,33 @@
>>   */
>>  #define YTPHY_WCR_TYPE_PULSE			BIT(0)
>>
>> -#define YT8531S_SYNCE_CFG_REG			0xA012
>> -#define YT8531S_SCR_SYNCE_ENABLE		BIT(6)
>> +#define YTPHY_SYNCE_CFG_REG			0xA012
>> +#define YT8521_SCR_CLK_SRC_MASK			(BIT(2) |
>> BIT(1))
>
> For the mask, you can consider using GENMASK macro
>

I will fix.

>> +#define YT8521_SCR_CLK_SRC_PLL_125M		(0x0 << 1)
>> +#define YT8521_SCR_CLK_SRC_REF_25M		(0x3 << 1)
>> +#define YT8521_SCR_SYNCE_ENABLE			BIT(5)
>> +#define YT8521_SCR_CLK_FRE_SEL_MASK		BIT(3)
>> +#define YT8521_SCR_CLK_FRE_SEL_125M		(0x1 << 3)
>> +#define YT8521_SCR_CLK_FRE_SEL_25M		(0x0 << 3)
>> +#define YT8531_SCR_CLK_SRC_MASK			(BIT(3) |
>> BIT(2) | BIT(1))
>> +#define YT8531_SCR_CLK_SRC_PLL_125M		(0x0 << 1)
>> +#define YT8531_SCR_CLK_SRC_REF_25M		(0x4 << 1)
>> +#define YT8531_SCR_SYNCE_ENABLE			BIT(6)
>> +#define YT8531_SCR_CLK_FRE_SEL_MASK		BIT(4)
>> +#define YT8531_SCR_CLK_FRE_SEL_125M		(0x1 << 4)
>> +#define YT8531_SCR_CLK_FRE_SEL_25M		(0x0 << 4)
>>
>>
>> +
>> +static int ytphy_clk_out_config(struct phy_device *phydev)
>> +{
>> +	struct yt8521_priv *priv = phydev->priv;
>> +	u16 set = 0;
>> +	u16 mask;
>> +
>> +	switch (phydev->drv->phy_id) {
>> +	case PHY_ID_YT8511:
>> +		/* YT8511 will be supported later */
>> +		return -EOPNOTSUPP;
>> +	case PHY_ID_YT8521:
>> +		mask = YT8521_SCR_SYNCE_ENABLE;
>> +		if (priv->clock_ouput) {
>> +			mask |= YT8521_SCR_CLK_SRC_MASK;
>> +			mask |= YT8521_SCR_CLK_FRE_SEL_MASK;
>
> You can consider assigning mask in single statement.

I will fix.

>
>> +			set |= YT8521_SCR_SYNCE_ENABLE;
>> +			if (priv->clock_freq_125M) {
>> +				set |= YT8521_SCR_CLK_FRE_SEL_125M;
>> +				set |= YT8521_SCR_CLK_SRC_PLL_125M;
>
> Similarly here.

I will fix.

>
>> +			} else {
>> +				set |= YT8521_SCR_CLK_FRE_SEL_25M;
>> +				set |= YT8521_SCR_CLK_SRC_REF_25M;
>> +			}
>> +		}
>> +		break;
>> +	case PHY_ID_YT8531:
>> +	case PHY_ID_YT8531S:
>> +		mask = YT8531_SCR_SYNCE_ENABLE;
>> +		if (priv->clock_ouput) {
>> +			mask |= YT8531_SCR_CLK_SRC_MASK;
>> +			mask |= YT8531_SCR_CLK_FRE_SEL_MASK;
>> +			set |= YT8531_SCR_SYNCE_ENABLE;
>> +			if (priv->clock_freq_125M) {
>> +				set |= YT8531_SCR_CLK_FRE_SEL_125M;
>> +				set |= YT8531_SCR_CLK_SRC_PLL_125M;
>> +			} else {
>> +				set |= YT8531_SCR_CLK_FRE_SEL_25M;
>> +				set |= YT8531_SCR_CLK_SRC_REF_25M;
>> +			}
>> +		}
>> +		break;
>> +	default:
>> +		phydev_err(phydev, "invalid phy id\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return ytphy_modify_ext(phydev, YTPHY_SYNCE_CFG_REG, mask,
>> set);
>> +}
>> +
>> ++static int ytphy_rgmii_clk_delay_config(struct phy_device *phydev)
>> +{
>> +	struct yt8521_priv *priv = phydev->priv;
>> +	u16 mask = 0;
>> +	u16 val = 0;
>> +	int ret;
>> +
>> +	/* rx delay basic controlled by dts.*/
>> +	if (priv->rx_delay_basic != YTPHY_DTS_INVAL_VAL) {
>> +		if (priv->rx_delay_basic)
>> +			val = YT8521_CCR_RXC_DLY_EN;
>> +		ret = ytphy_modify_ext(phydev, YT8521_CHIP_CONFIG_REG,
>> +				       YT8521_CCR_RXC_DLY_EN, val);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	val = 0;
>> +	/* If rx_delay_additional and tx_delay_* are all not be seted
>> in dts,
>> +	 * then used the fixed *_DELAY_DIS or *_DELAY_EN. Otherwise,
>> use the
>> +	 * value set by rx_delay_additional, tx_delay_ge and
>> tx_delay_fe.
>> +	 */
>> +	if ((priv->rx_delay_additional & priv->tx_delay_ge & priv-
>>> tx_delay_fe)
>> +	   == YTPHY_DTS_INVAL_VAL) {
>> +		switch (phydev->interface) {
>> +		case PHY_INTERFACE_MODE_RGMII:
>> +			val |= YT8521_RC1R_GE_TX_DELAY_DIS;
>> +			val |= YT8521_RC1R_FE_TX_DELAY_DIS;
>> +			val |= YT8521_RC1R_RX_DELAY_DIS;
>
> Single statement would be suffice.

I will fix.

>
>> +			break;
>> +		case PHY_INTERFACE_MODE_RGMII_RXID:
>> +			val |= YT8521_RC1R_GE_TX_DELAY_DIS;
>> +			val |= YT8521_RC1R_FE_TX_DELAY_DIS;
>> +			val |= YT8521_RC1R_RX_DELAY_EN;
>> +			break;
>> +		case PHY_INTERFACE_MODE_RGMII_TXID:
>> +			val |= YT8521_RC1R_GE_TX_DELAY_EN;
>> +			val |= YT8521_RC1R_FE_TX_DELAY_EN;
>> +			val |= YT8521_RC1R_RX_DELAY_DIS;
>> +			break;
>> +		case PHY_INTERFACE_MODE_RGMII_ID:
>> +			val |= YT8521_RC1R_GE_TX_DELAY_EN;
>> +			val |= YT8521_RC1R_FE_TX_DELAY_EN;
>> +			val |= YT8521_RC1R_RX_DELAY_EN;
>> +			break;
>> +		default: /* do not support other modes */
>> +			return -EOPNOTSUPP;
>> +		}
>> +		mask = YT8521_RC1R_RX_DELAY_MASK |
>> YT8521_RC1R_FE_TX_DELAY_MASK
>> +		       | YT8521_RC1R_GE_TX_DELAY_MASK;
>> +	}
>> +
>>
>>  /**
>>   * ytphy_utp_read_lpa() - read LPA then setup lp_advertising for utp
>>   * @phydev: a pointer to a &struct phy_device
>> @@ -1125,6 +1486,34 @@ static int yt8521_resume(struct phy_device
>> *phydev)
>>  	return yt8521_modify_utp_fiber_bmcr(phydev, BMCR_PDOWN, 0);
>>  }
>>
>>
>> @@ -1778,7 +2129,7 @@ static struct phy_driver motorcomm_phy_drvs[] =
>> {
>>  		PHY_ID_MATCH_EXACT(PHY_ID_YT8531S),
>>  		.name		= "YT8531S Gigabit Ethernet",
>>  		.get_features	= yt8521_get_features,
>> -		.probe		= yt8531s_probe,
>> +		.probe		= yt8521_probe,
>>  		.read_page	= yt8521_read_page,
>>  		.write_page	= yt8521_write_page,
>>  		.get_wol	= ytphy_get_wol,
>> @@ -1804,7 +2155,7 @@ static const struct mdio_device_id
>> __maybe_unused motorcomm_tbl[] = {
>>  	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8511) },
>>  	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8521) },
>>  	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8531S) },
>> -	{ /* sentinal */ }
>> +	{ /* sentinel */ }
>
> It should go as separate patch.

I will fix.

>>  };
>>
>>  MODULE_DEVICE_TABLE(mdio, motorcomm_tbl);

Re: [PATCH net-next v1 2/3] net: phy: Add dts support for Motorcomm yt8521/yt8531s gigabit ethernet phy

[Andrew Lunn]

On Thu, Jan 05, 2023 at 03:30:23PM +0800, Frank wrote:
> Add dts support for yt8521 and yt8531s. This patch has
> been tested on AM335x platform which has one YT8531S interface
> card and passed all test cases.
> 
> Signed-off-by: Frank <Frank.Sae@motor-comm.com>
> ---
>  drivers/net/phy/motorcomm.c | 517 ++++++++++++++++++++++++++++++------
>  1 file changed, 434 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index 685190db72de..7ebcca374a67 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c
> @@ -10,10 +10,11 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/phy.h>
> +#include <linux/of.h>
>  
>  #define PHY_ID_YT8511		0x0000010a
> -#define PHY_ID_YT8521		0x0000011A
> -#define PHY_ID_YT8531S		0x4F51E91A
> +#define PHY_ID_YT8521		0x0000011a
> +#define PHY_ID_YT8531S		0x4f51e91a

Please do the lower case conversion as a separate patch.

>  
>  /* YT8521/YT8531S Register Overview
>   *	UTP Register space	|	FIBER Register space
> @@ -144,6 +145,16 @@
>  #define YT8521_ESC1R_SLEEP_SW			BIT(15)
>  #define YT8521_ESC1R_PLLON_SLP			BIT(14)
>  
> +/* Phy Serdes analog cfg2 Register */
> +#define YTPHY_SERDES_ANALOG_CFG2_REG		0xA1
> +#define YTPHY_SAC2R_TX_AMPLITUDE_MASK		((0x7 << 13) | (0x7 << 1))
> +#define YT8521_SAC2R_TX_AMPLITUDE_LOW		((0x7 << 13) | (0x0 << 1))
> +#define YT8521_SAC2R_TX_AMPLITUDE_MIDDLE	((0x5 << 13) | (0x5 << 1))
> +#define YT8521_SAC2R_TX_AMPLITUDE_HIGH		((0x3 << 13) | (0x6 << 1))

So there are two values which control the amplitude? Buts 1-3, and bit
7-9?  Can they be used independently?  Also, 7, 5, 3 is also add. Does
bit 0 of this value have some special meaning? Please document this
fully.

> +#define YT8531S_SAC2R_TX_AMPLITUDE_LOW		((0x0 << 13) | (0x0 << 1))
> +#define YT8531S_SAC2R_TX_AMPLITUDE_MIDDLE	((0x0 << 13) | (0x1 << 1))
> +#define YT8531S_SAC2R_TX_AMPLITUDE_HIGH		((0x0 << 13) | (0x2 << 1))

This more sense, but why the 0 << 13? What do the bits 13-? mean?

> +
>  /* Phy fiber Link timer cfg2 Register */
>  #define YT8521_LINK_TIMER_CFG2_REG		0xA5
>  #define YT8521_LTCR_EN_AUTOSEN			BIT(15)
> @@ -161,6 +172,7 @@
>  
>  #define YT8521_CHIP_CONFIG_REG			0xA001
>  #define YT8521_CCR_SW_RST			BIT(15)
> +#define YT8521_CCR_RXC_DLY_EN			BIT(8)
>  
>  #define YT8521_CCR_MODE_SEL_MASK		(BIT(2) | BIT(1) | BIT(0))
>  #define YT8521_CCR_MODE_UTP_TO_RGMII		0
> @@ -178,22 +190,27 @@
>  #define YT8521_MODE_POLL			0x3
>  
>  #define YT8521_RGMII_CONFIG1_REG		0xA003
> -
> +#define YT8521_RC1R_TX_CLK_SEL_MASK		BIT(14)
> +#define YT8521_RC1R_TX_CLK_SEL_ORIGINAL		(0x0 << 14)
> +#define YT8521_RC1R_TX_CLK_SEL_INVERTED		(0x1 << 14)

Please use the BIT macro.


>  /* TX Gig-E Delay is bits 3:0, default 0x1
>   * TX Fast-E Delay is bits 7:4, default 0xf
>   * RX Delay is bits 13:10, default 0x0
>   * Delay = 150ps * N
>   * On = 2250ps, off = 0ps
>   */
> -#define YT8521_RC1R_RX_DELAY_MASK		(0xF << 10)
> -#define YT8521_RC1R_RX_DELAY_EN			(0xF << 10)
> -#define YT8521_RC1R_RX_DELAY_DIS		(0x0 << 10)
> -#define YT8521_RC1R_FE_TX_DELAY_MASK		(0xF << 4)
> -#define YT8521_RC1R_FE_TX_DELAY_EN		(0xF << 4)
> -#define YT8521_RC1R_FE_TX_DELAY_DIS		(0x0 << 4)
> -#define YT8521_RC1R_GE_TX_DELAY_MASK		(0xF << 0)
> -#define YT8521_RC1R_GE_TX_DELAY_EN		(0xF << 0)
> -#define YT8521_RC1R_GE_TX_DELAY_DIS		(0x0 << 0)
> +#define YT8521_RC1R_GE_TX_DELAY_BIT		(0)
> +#define YT8521_RC1R_FE_TX_DELAY_BIT		(4)
> +#define YT8521_RC1R_RX_DELAY_BIT		(10)
> +#define YT8521_RC1R_RX_DELAY_MASK		(0xF << YT8521_RC1R_RX_DELAY_BIT)
> +#define YT8521_RC1R_RX_DELAY_EN			(0xF << YT8521_RC1R_RX_DELAY_BIT)
> +#define YT8521_RC1R_RX_DELAY_DIS		(0x0 << YT8521_RC1R_RX_DELAY_BIT)
> +#define YT8521_RC1R_FE_TX_DELAY_MASK		(0xF << YT8521_RC1R_FE_TX_DELAY_BIT)
> +#define YT8521_RC1R_FE_TX_DELAY_EN		(0xF << YT8521_RC1R_FE_TX_DELAY_BIT)
> +#define YT8521_RC1R_FE_TX_DELAY_DIS		(0x0 << YT8521_RC1R_FE_TX_DELAY_BIT)
> +#define YT8521_RC1R_GE_TX_DELAY_MASK		(0xF << YT8521_RC1R_GE_TX_DELAY_BIT)
> +#define YT8521_RC1R_GE_TX_DELAY_EN		(0xF << YT8521_RC1R_GE_TX_DELAY_BIT)
> +#define YT8521_RC1R_GE_TX_DELAY_DIS		(0x0 << YT8521_RC1R_GE_TX_DELAY_BIT)
>  
>  #define YTPHY_MISC_CONFIG_REG			0xA006
>  #define YTPHY_MCR_FIBER_SPEED_MASK		BIT(0)
> @@ -222,11 +239,33 @@
>   */
>  #define YTPHY_WCR_TYPE_PULSE			BIT(0)
>  
> -#define YT8531S_SYNCE_CFG_REG			0xA012
> -#define YT8531S_SCR_SYNCE_ENABLE		BIT(6)
> +#define YTPHY_SYNCE_CFG_REG			0xA012
> +#define YT8521_SCR_CLK_SRC_MASK			(BIT(2) | BIT(1))
> +#define YT8521_SCR_CLK_SRC_PLL_125M		(0x0 << 1)
> +#define YT8521_SCR_CLK_SRC_REF_25M		(0x3 << 1)
> +#define YT8521_SCR_SYNCE_ENABLE			BIT(5)
> +#define YT8521_SCR_CLK_FRE_SEL_MASK		BIT(3)
> +#define YT8521_SCR_CLK_FRE_SEL_125M		(0x1 << 3)
> +#define YT8521_SCR_CLK_FRE_SEL_25M		(0x0 << 3)

Whenever it is a single bit, please use the BIT macro.

> +#define YT8531_SCR_CLK_SRC_MASK			(BIT(3) | BIT(2) | BIT(1))
> +#define YT8531_SCR_CLK_SRC_PLL_125M		(0x0 << 1)
> +#define YT8531_SCR_CLK_SRC_REF_25M		(0x4 << 1)
> +#define YT8531_SCR_SYNCE_ENABLE			BIT(6)
> +#define YT8531_SCR_CLK_FRE_SEL_MASK		BIT(4)
> +#define YT8531_SCR_CLK_FRE_SEL_125M		(0x1 << 4)
> +#define YT8531_SCR_CLK_FRE_SEL_25M		(0x0 << 4)
>  
>  /* Extended Register  end */
>  
> +#define YTPHY_DTS_MAX_TX_AMPLITUDE		0x2
> +#define YTPHY_DTS_MAX_DELAY_VAL			2250
> +#define YTPHY_DTS_STEP_DELAY_VAL		150
> +#define YTPHY_DTS_INVAL_VAL			0xFF
> +
> +#define YTPHY_DTS_OUTPUT_CLK_DIS		0
> +#define YTPHY_DTS_OUTPUT_CLK_25M		25000000
> +#define YTPHY_DTS_OUTPUT_CLK_125M		125000000
> +
>  struct yt8521_priv {
>  	/* combo_advertising is used for case of YT8521 in combo mode,
>  	 * this means that yt8521 may work in utp or fiber mode which depends
> @@ -243,6 +282,30 @@ struct yt8521_priv {
>  	 * YT8521_RSSR_TO_BE_ARBITRATED
>  	 */
>  	u8 reg_page;
> +
> +	/* The following parameters are from dts */
> +	/* rx delay = rx_delay_basic + rx_delay_additional
> +	 * basic delay is ~2ns, 0 = off, 1 = on
> +	 * rx_delay_additional,delay time = 150ps * val
> +	 */
> +	u8 rx_delay_basic;
> +	u8 rx_delay_additional;
> +
> +	/* tx_delay_ge is tx_delay for 1000Mbps
> +	 * tx_delay_fe is tx_delay for 100Mbps or 10Mbps
> +	 * delay time = 150ps * val
> +	 */
> +	u8 tx_delay_ge;
> +	u8 tx_delay_fe;
> +	u8 sds_tx_amplitude;
> +	bool keep_pll_enabled;
> +	bool auto_sleep_disabled;
> +	bool clock_ouput;	/* output clock ctl: 0=off, 1=on */
> +	bool clock_freq_125M;	/* output clock freq selcect: 0=25M, 1=125M */
> +	bool tx_clk_adj_enabled;/* tx clk adj ctl: 0=off, 1=on */
> +	bool tx_clk_10_inverted;
> +	bool tx_clk_100_inverted;
> +	bool tx_clk_1000_inverted;

Do you need to store all these values? In general, PHY drivers parse
DT, and program the hardware directly. If these values are lost on
reset, and you need to perform a reset for normal operation, then yes,
it makes sense to store them. But in general, that is not how hardware
works.

> +static int ytphy_parse_dt(struct phy_device *phydev)
> +{
> +	struct device_node *node = phydev->mdio.dev.of_node;
> +	struct yt8521_priv *priv = phydev->priv;
> +	u32 freq, val;
> +	int ret;
> +
> +	priv->rx_delay_additional = YTPHY_DTS_INVAL_VAL;
> +	priv->sds_tx_amplitude = YTPHY_DTS_INVAL_VAL;
> +	priv->rx_delay_basic = YTPHY_DTS_INVAL_VAL;
> +	priv->tx_delay_ge = YTPHY_DTS_INVAL_VAL;
> +	priv->tx_delay_fe = YTPHY_DTS_INVAL_VAL;
> +
> +	if (!IS_ENABLED(CONFIG_OF_MDIO)) {

No other PHY driver does this. Why is this here?

As a general rule, if you do something which no other driver does, you
are doing something wrong. All PHY drivers should basically look the
same, follow the same structure, etc. So it is a good idea to review 5
other drivers, and make your driver look similar.

> +		priv->auto_sleep_disabled = true;
> +		priv->keep_pll_enabled = true;
> +		return 0;
> +	}
> +
> +	ret = of_property_read_u32(node, "motorcomm,clk-out-frequency", &freq);
> +	if (ret < 0)
> +		freq = YTPHY_DTS_OUTPUT_CLK_DIS;/* default value as dts*/
> +
> +	switch (freq) {
> +	case YTPHY_DTS_OUTPUT_CLK_DIS:
> +		priv->clock_ouput = false;
> +		break;
> +	case YTPHY_DTS_OUTPUT_CLK_25M:
> +		priv->clock_freq_125M = false;
> +		priv->clock_ouput = true;
> +		break;
> +	case YTPHY_DTS_OUTPUT_CLK_125M:
> +		priv->clock_freq_125M = true;
> +		priv->clock_ouput = true;
> +		break;
> +	default:
> +		phydev_err(phydev, "invalid motorcomm,clk-out-frequency\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!of_property_read_u32(node, "motorcomm,rx-delay-basic", &val)) {
> +		if (val > 1) {
> +			phydev_err(phydev,
> +				   "invalid motorcomm,rx-delay-basic\n");
> +			return -EINVAL;
> +		}
> +		priv->rx_delay_basic = val;
> +	}
> +
> +	if (!of_property_read_u32(node, "motorcomm,rx-delay-additional-ps", &val)) {
> +		if (val > YTPHY_DTS_MAX_DELAY_VAL) {
> +			phydev_err(phydev, "invalid motorcomm,rx-delay-additional-ps\n");
> +			return -EINVAL;
> +		}

Please check the value is also one of the supported values. Please do
that for all your delays.

> +	if (of_property_read_bool(node, "motorcomm,keep-pll-enabled"))
> +		priv->keep_pll_enabled = true;

I think this only makes sense when priv->clock_output is true? Please
test for that.


> +static int ytphy_rgmii_clk_delay_config(struct phy_device *phydev)
> +{
> +	struct yt8521_priv *priv = phydev->priv;
> +	u16 mask = 0;
> +	u16 val = 0;
> +	int ret;
> +
> +	/* rx delay basic controlled by dts.*/
> +	if (priv->rx_delay_basic != YTPHY_DTS_INVAL_VAL) {
> +		if (priv->rx_delay_basic)
> +			val = YT8521_CCR_RXC_DLY_EN;
> +		ret = ytphy_modify_ext(phydev, YT8521_CHIP_CONFIG_REG,
> +				       YT8521_CCR_RXC_DLY_EN, val);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	val = 0;
> +	/* If rx_delay_additional and tx_delay_* are all not be seted in dts,
> +	 * then used the fixed *_DELAY_DIS or *_DELAY_EN. Otherwise, use the
> +	 * value set by rx_delay_additional, tx_delay_ge and tx_delay_fe.
> +	 */

So what you should be doing here is always respecting
phydev->interface. You can then fine tune the delays using
rx-internal-delay-ps and tx-internal-delay-ps.

> +static int ytphy_probe_helper(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct yt8521_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	phydev->priv = priv;
> +
> +	ret = ytphy_parse_dt(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	phy_lock_mdio_bus(phydev);
> +	ret = ytphy_clk_out_config(phydev);
> +	phy_unlock_mdio_bus(phydev);
> +	return ret;
> +}
> +
>  /**
>   * yt8521_probe() - read chip config then set suitable polling_mode
>   * @phydev: a pointer to a &struct phy_device
> @@ -601,16 +983,15 @@ static int yt8521_write_page(struct phy_device *phydev, int page)
>   */
>  static int yt8521_probe(struct phy_device *phydev)
>  {
> -	struct device *dev = &phydev->mdio.dev;
>  	struct yt8521_priv *priv;
>  	int chip_config;
>  	int ret;
>  
> -	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> -	if (!priv)
> -		return -ENOMEM;
> +	ret = ytphy_probe_helper(phydev);
> +	if (ret < 0)
> +		return ret;
>  
> -	phydev->priv = priv;
> +	priv = phydev->priv;

I don't see why you added this probe helper.

Is this to make the driver look more like the vendor driver? Please
seperate refactoring patches from new functionality. We want to see
lots of simple, obviously correct patches which are easy to review.

     Andrew
  

Re: [PATCH net-next v1 2/3] net: phy: Add dts support for Motorcomm yt8521/yt8531s gigabit ethernet phy

[Frank.Sae]

Hi Andrew,

On 2023/1/6 01:03, Andrew Lunn wrote:
> On Thu, Jan 05, 2023 at 03:30:23PM +0800, Frank wrote:
>> Add dts support for yt8521 and yt8531s. This patch has
>> been tested on AM335x platform which has one YT8531S interface
>> card and passed all test cases.
>>
>> Signed-off-by: Frank <Frank.Sae@motor-comm.com>
>> ---
>>  drivers/net/phy/motorcomm.c | 517 ++++++++++++++++++++++++++++++------
>>  1 file changed, 434 insertions(+), 83 deletions(-)
>>
>> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
>> index 685190db72de..7ebcca374a67 100644
>> --- a/drivers/net/phy/motorcomm.c
>> +++ b/drivers/net/phy/motorcomm.c
>> @@ -10,10 +10,11 @@
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>>  #include <linux/phy.h>
>> +#include <linux/of.h>
>>  
>>  #define PHY_ID_YT8511		0x0000010a
>> -#define PHY_ID_YT8521		0x0000011A
>> -#define PHY_ID_YT8531S		0x4F51E91A
>> +#define PHY_ID_YT8521		0x0000011a
>> +#define PHY_ID_YT8531S		0x4f51e91a
> 
> Please do the lower case conversion as a separate patch.
> 

I will fix.

>>  
>>  /* YT8521/YT8531S Register Overview
>>   *	UTP Register space	|	FIBER Register space
>> @@ -144,6 +145,16 @@
>>  #define YT8521_ESC1R_SLEEP_SW			BIT(15)
>>  #define YT8521_ESC1R_PLLON_SLP			BIT(14)
>>  
>> +/* Phy Serdes analog cfg2 Register */
>> +#define YTPHY_SERDES_ANALOG_CFG2_REG		0xA1
>> +#define YTPHY_SAC2R_TX_AMPLITUDE_MASK		((0x7 << 13) | (0x7 << 1))
>> +#define YT8521_SAC2R_TX_AMPLITUDE_LOW		((0x7 << 13) | (0x0 << 1))
>> +#define YT8521_SAC2R_TX_AMPLITUDE_MIDDLE	((0x5 << 13) | (0x5 << 1))
>> +#define YT8521_SAC2R_TX_AMPLITUDE_HIGH		((0x3 << 13) | (0x6 << 1))
> 
> So there are two values which control the amplitude? Buts 1-3, and bit
> 7-9?  Can they be used independently?  Also, 7, 5, 3 is also add. Does
> bit 0 of this value have some special meaning? Please document this
> fully.
> 

There are three values which control the amplitude,  13-15,3 and 1-2.
They be used independently. Maybe it is good to expose three type ( 0:
low;   1: middle;   2: high)to dts.

Bit	Symbol		Access	default	Description
15:13	Tx_swing_sel	RW	0x0	TX driver stage2 amplitude control
12	Tx_driver_stg1	RW	0x1	TX driver stage1 amplitude control
11	Reserved	RO	0x0	Reserved
10:8	Tx_ckdiv10_con	RW	0x0	tx divide by 10 clock delay control
7:4	Reserved	RO	0x0	Reserved
3	Tx_post_stg1	RW	0x0	TX driver post stage1 amplitude control
2:1	Tx_de_sel	RW	0x0	TX driver post stage2 amplitude control
0	Tx_pd		RW	0x0	power down analog tx


>> +#define YT8531S_SAC2R_TX_AMPLITUDE_LOW		((0x0 << 13) | (0x0 << 1))
>> +#define YT8531S_SAC2R_TX_AMPLITUDE_MIDDLE	((0x0 << 13) | (0x1 << 1))
>> +#define YT8531S_SAC2R_TX_AMPLITUDE_HIGH		((0x0 << 13) | (0x2 << 1))
> 
> This more sense, but why the 0 << 13? What do the bits 13-? mean?

0 << 13 means 13-15 bit all zero.
bits 13- mans the start bit is 13.

> 
>> +
>>  /* Phy fiber Link timer cfg2 Register */
>>  #define YT8521_LINK_TIMER_CFG2_REG		0xA5
>>  #define YT8521_LTCR_EN_AUTOSEN			BIT(15)
>> @@ -161,6 +172,7 @@
>>  
>>  #define YT8521_CHIP_CONFIG_REG			0xA001
>>  #define YT8521_CCR_SW_RST			BIT(15)
>> +#define YT8521_CCR_RXC_DLY_EN			BIT(8)
>>  
>>  #define YT8521_CCR_MODE_SEL_MASK		(BIT(2) | BIT(1) | BIT(0))
>>  #define YT8521_CCR_MODE_UTP_TO_RGMII		0
>> @@ -178,22 +190,27 @@
>>  #define YT8521_MODE_POLL			0x3
>>  
>>  #define YT8521_RGMII_CONFIG1_REG		0xA003
>> -
>> +#define YT8521_RC1R_TX_CLK_SEL_MASK		BIT(14)
>> +#define YT8521_RC1R_TX_CLK_SEL_ORIGINAL		(0x0 << 14)
>> +#define YT8521_RC1R_TX_CLK_SEL_INVERTED		(0x1 << 14)
> 
> Please use the BIT macro.
> 

ok, change

+#define YT8521_RC1R_TX_CLK_SEL_MASK		BIT(14)
+#define YT8521_RC1R_TX_CLK_SEL_ORIGINAL		(0x0 << 14)
+#define YT8521_RC1R_TX_CLK_SEL_INVERTED		(0x1 << 14)

to

+#define YT8521_RC1R_TX_CLK_SEL_INVERTED		BIT(14)

?

> 
>>  /* TX Gig-E Delay is bits 3:0, default 0x1
>>   * TX Fast-E Delay is bits 7:4, default 0xf
>>   * RX Delay is bits 13:10, default 0x0
>>   * Delay = 150ps * N
>>   * On = 2250ps, off = 0ps
>>   */
>> -#define YT8521_RC1R_RX_DELAY_MASK		(0xF << 10)
>> -#define YT8521_RC1R_RX_DELAY_EN			(0xF << 10)
>> -#define YT8521_RC1R_RX_DELAY_DIS		(0x0 << 10)
>> -#define YT8521_RC1R_FE_TX_DELAY_MASK		(0xF << 4)
>> -#define YT8521_RC1R_FE_TX_DELAY_EN		(0xF << 4)
>> -#define YT8521_RC1R_FE_TX_DELAY_DIS		(0x0 << 4)
>> -#define YT8521_RC1R_GE_TX_DELAY_MASK		(0xF << 0)
>> -#define YT8521_RC1R_GE_TX_DELAY_EN		(0xF << 0)
>> -#define YT8521_RC1R_GE_TX_DELAY_DIS		(0x0 << 0)
>> +#define YT8521_RC1R_GE_TX_DELAY_BIT		(0)
>> +#define YT8521_RC1R_FE_TX_DELAY_BIT		(4)
>> +#define YT8521_RC1R_RX_DELAY_BIT		(10)
>> +#define YT8521_RC1R_RX_DELAY_MASK		(0xF << YT8521_RC1R_RX_DELAY_BIT)
>> +#define YT8521_RC1R_RX_DELAY_EN			(0xF << YT8521_RC1R_RX_DELAY_BIT)
>> +#define YT8521_RC1R_RX_DELAY_DIS		(0x0 << YT8521_RC1R_RX_DELAY_BIT)
>> +#define YT8521_RC1R_FE_TX_DELAY_MASK		(0xF << YT8521_RC1R_FE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_FE_TX_DELAY_EN		(0xF << YT8521_RC1R_FE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_FE_TX_DELAY_DIS		(0x0 << YT8521_RC1R_FE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_GE_TX_DELAY_MASK		(0xF << YT8521_RC1R_GE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_GE_TX_DELAY_EN		(0xF << YT8521_RC1R_GE_TX_DELAY_BIT)
>> +#define YT8521_RC1R_GE_TX_DELAY_DIS		(0x0 << YT8521_RC1R_GE_TX_DELAY_BIT)
>>  
>>  #define YTPHY_MISC_CONFIG_REG			0xA006
>>  #define YTPHY_MCR_FIBER_SPEED_MASK		BIT(0)
>> @@ -222,11 +239,33 @@
>>   */
>>  #define YTPHY_WCR_TYPE_PULSE			BIT(0)
>>  
>> -#define YT8531S_SYNCE_CFG_REG			0xA012
>> -#define YT8531S_SCR_SYNCE_ENABLE		BIT(6)
>> +#define YTPHY_SYNCE_CFG_REG			0xA012
>> +#define YT8521_SCR_CLK_SRC_MASK			(BIT(2) | BIT(1))
>> +#define YT8521_SCR_CLK_SRC_PLL_125M		(0x0 << 1)
>> +#define YT8521_SCR_CLK_SRC_REF_25M		(0x3 << 1)
>> +#define YT8521_SCR_SYNCE_ENABLE			BIT(5)
>> +#define YT8521_SCR_CLK_FRE_SEL_MASK		BIT(3)
>> +#define YT8521_SCR_CLK_FRE_SEL_125M		(0x1 << 3)
>> +#define YT8521_SCR_CLK_FRE_SEL_25M		(0x0 << 3)
> 
> Whenever it is a single bit, please use the BIT macro.

I wwill fix.

> 
>> +#define YT8531_SCR_CLK_SRC_MASK			(BIT(3) | BIT(2) | BIT(1))
>> +#define YT8531_SCR_CLK_SRC_PLL_125M		(0x0 << 1)
>> +#define YT8531_SCR_CLK_SRC_REF_25M		(0x4 << 1)
>> +#define YT8531_SCR_SYNCE_ENABLE			BIT(6)
>> +#define YT8531_SCR_CLK_FRE_SEL_MASK		BIT(4)
>> +#define YT8531_SCR_CLK_FRE_SEL_125M		(0x1 << 4)
>> +#define YT8531_SCR_CLK_FRE_SEL_25M		(0x0 << 4)
>>  
>>  /* Extended Register  end */
>>  
>> +#define YTPHY_DTS_MAX_TX_AMPLITUDE		0x2
>> +#define YTPHY_DTS_MAX_DELAY_VAL			2250
>> +#define YTPHY_DTS_STEP_DELAY_VAL		150
>> +#define YTPHY_DTS_INVAL_VAL			0xFF
>> +
>> +#define YTPHY_DTS_OUTPUT_CLK_DIS		0
>> +#define YTPHY_DTS_OUTPUT_CLK_25M		25000000
>> +#define YTPHY_DTS_OUTPUT_CLK_125M		125000000
>> +
>>  struct yt8521_priv {
>>  	/* combo_advertising is used for case of YT8521 in combo mode,
>>  	 * this means that yt8521 may work in utp or fiber mode which depends
>> @@ -243,6 +282,30 @@ struct yt8521_priv {
>>  	 * YT8521_RSSR_TO_BE_ARBITRATED
>>  	 */
>>  	u8 reg_page;
>> +
>> +	/* The following parameters are from dts */
>> +	/* rx delay = rx_delay_basic + rx_delay_additional
>> +	 * basic delay is ~2ns, 0 = off, 1 = on
>> +	 * rx_delay_additional,delay time = 150ps * val
>> +	 */
>> +	u8 rx_delay_basic;
>> +	u8 rx_delay_additional;
>> +
>> +	/* tx_delay_ge is tx_delay for 1000Mbps
>> +	 * tx_delay_fe is tx_delay for 100Mbps or 10Mbps
>> +	 * delay time = 150ps * val
>> +	 */
>> +	u8 tx_delay_ge;
>> +	u8 tx_delay_fe;
>> +	u8 sds_tx_amplitude;
>> +	bool keep_pll_enabled;
>> +	bool auto_sleep_disabled;
>> +	bool clock_ouput;	/* output clock ctl: 0=off, 1=on */
>> +	bool clock_freq_125M;	/* output clock freq selcect: 0=25M, 1=125M */
>> +	bool tx_clk_adj_enabled;/* tx clk adj ctl: 0=off, 1=on */
>> +	bool tx_clk_10_inverted;
>> +	bool tx_clk_100_inverted;
>> +	bool tx_clk_1000_inverted;
> 
> Do you need to store all these values? In general, PHY drivers parse
> DT, and program the hardware directly. If these values are lost on
> reset, and you need to perform a reset for normal operation, then yes,
> it makes sense to store them. But in general, that is not how hardware
> works.
> 

I will fix.

>> +static int ytphy_parse_dt(struct phy_device *phydev)
>> +{
>> +	struct device_node *node = phydev->mdio.dev.of_node;
>> +	struct yt8521_priv *priv = phydev->priv;
>> +	u32 freq, val;
>> +	int ret;
>> +
>> +	priv->rx_delay_additional = YTPHY_DTS_INVAL_VAL;
>> +	priv->sds_tx_amplitude = YTPHY_DTS_INVAL_VAL;
>> +	priv->rx_delay_basic = YTPHY_DTS_INVAL_VAL;
>> +	priv->tx_delay_ge = YTPHY_DTS_INVAL_VAL;
>> +	priv->tx_delay_fe = YTPHY_DTS_INVAL_VAL;
>> +
>> +	if (!IS_ENABLED(CONFIG_OF_MDIO)) {
> 
> No other PHY driver does this. Why is this here?
> 
> As a general rule, if you do something which no other driver does, you
> are doing something wrong. All PHY drivers should basically look the
> same, follow the same structure, etc. So it is a good idea to review 5
> other drivers, and make your driver look similar.
> 

I will fix.

>> +		priv->auto_sleep_disabled = true;
>> +		priv->keep_pll_enabled = true;
>> +		return 0;
>> +	}
>> +
>> +	ret = of_property_read_u32(node, "motorcomm,clk-out-frequency", &freq);
>> +	if (ret < 0)
>> +		freq = YTPHY_DTS_OUTPUT_CLK_DIS;/* default value as dts*/
>> +
>> +	switch (freq) {
>> +	case YTPHY_DTS_OUTPUT_CLK_DIS:
>> +		priv->clock_ouput = false;
>> +		break;
>> +	case YTPHY_DTS_OUTPUT_CLK_25M:
>> +		priv->clock_freq_125M = false;
>> +		priv->clock_ouput = true;
>> +		break;
>> +	case YTPHY_DTS_OUTPUT_CLK_125M:
>> +		priv->clock_freq_125M = true;
>> +		priv->clock_ouput = true;
>> +		break;
>> +	default:
>> +		phydev_err(phydev, "invalid motorcomm,clk-out-frequency\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!of_property_read_u32(node, "motorcomm,rx-delay-basic", &val)) {
>> +		if (val > 1) {
>> +			phydev_err(phydev,
>> +				   "invalid motorcomm,rx-delay-basic\n");
>> +			return -EINVAL;
>> +		}
>> +		priv->rx_delay_basic = val;
>> +	}
>> +
>> +	if (!of_property_read_u32(node, "motorcomm,rx-delay-additional-ps", &val)) {
>> +		if (val > YTPHY_DTS_MAX_DELAY_VAL) {
>> +			phydev_err(phydev, "invalid motorcomm,rx-delay-additional-ps\n");
>> +			return -EINVAL;
>> +		}
> 
> Please check the value is also one of the supported values. Please do
> that for all your delays.
> 

The following code check the supported values.

if (val)
			val /= YTPHY_DTS_STEP_DELAY_VAL;
		priv->rx_delay_additional = val;


rx-delay-additional-ps = reg_val(0-15) *150ps. so val >
YTPHY_DTS_MAX_DELAY_VAL 2250 is err.
rx_delay_additional is store the reg_val (0-15).

>> +	if (of_property_read_bool(node, "motorcomm,keep-pll-enabled"))
>> +		priv->keep_pll_enabled = true;
> 
> I think this only makes sense when priv->clock_output is true? Please
> test for that.
> 

No,priv->clock_output is used to output 25Mhz or 125Mhz for mac.
priv->keep_pll_enabled is used to enable RXC clock when no wire plug.

> 
>> +static int ytphy_rgmii_clk_delay_config(struct phy_device *phydev)
>> +{
>> +	struct yt8521_priv *priv = phydev->priv;
>> +	u16 mask = 0;
>> +	u16 val = 0;
>> +	int ret;
>> +
>> +	/* rx delay basic controlled by dts.*/
>> +	if (priv->rx_delay_basic != YTPHY_DTS_INVAL_VAL) {
>> +		if (priv->rx_delay_basic)
>> +			val = YT8521_CCR_RXC_DLY_EN;
>> +		ret = ytphy_modify_ext(phydev, YT8521_CHIP_CONFIG_REG,
>> +				       YT8521_CCR_RXC_DLY_EN, val);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	val = 0;
>> +	/* If rx_delay_additional and tx_delay_* are all not be seted in dts,
>> +	 * then used the fixed *_DELAY_DIS or *_DELAY_EN. Otherwise, use the
>> +	 * value set by rx_delay_additional, tx_delay_ge and tx_delay_fe.
>> +	 */
> 
> So what you should be doing here is always respecting
> phydev->interface. You can then fine tune the delays using
> rx-internal-delay-ps and tx-internal-delay-ps.
> 

I will fix.

>> +static int ytphy_probe_helper(struct phy_device *phydev)
>> +{
>> +	struct device *dev = &phydev->mdio.dev;
>> +	struct yt8521_priv *priv;
>> +	int ret;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	phydev->priv = priv;
>> +
>> +	ret = ytphy_parse_dt(phydev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	phy_lock_mdio_bus(phydev);
>> +	ret = ytphy_clk_out_config(phydev);
>> +	phy_unlock_mdio_bus(phydev);
>> +	return ret;
>> +}
>> +
>>  /**
>>   * yt8521_probe() - read chip config then set suitable polling_mode
>>   * @phydev: a pointer to a &struct phy_device
>> @@ -601,16 +983,15 @@ static int yt8521_write_page(struct phy_device *phydev, int page)
>>   */
>>  static int yt8521_probe(struct phy_device *phydev)
>>  {
>> -	struct device *dev = &phydev->mdio.dev;
>>  	struct yt8521_priv *priv;
>>  	int chip_config;
>>  	int ret;
>>  
>> -	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> -	if (!priv)
>> -		return -ENOMEM;
>> +	ret = ytphy_probe_helper(phydev);
>> +	if (ret < 0)
>> +		return ret;
>>  
>> -	phydev->priv = priv;
>> +	priv = phydev->priv;
> 
> I don't see why you added this probe helper.
> 
> Is this to make the driver look more like the vendor driver? Please
> seperate refactoring patches from new functionality. We want to see
> lots of simple, obviously correct patches which are easy to review.
> 

I will fix.

>      Andrew
>   

Re: [PATCH net-next v1 2/3] net: phy: Add dts support for Motorcomm yt8521/yt8531s gigabit ethernet phy

[Andrew Lunn]

> >>  #define PHY_ID_YT8511		0x0000010a
> >> -#define PHY_ID_YT8521		0x0000011A
> >> -#define PHY_ID_YT8531S		0x4F51E91A
> >> +#define PHY_ID_YT8521		0x0000011a
> >> +#define PHY_ID_YT8531S		0x4f51e91a
> > 
> > Please do the lower case conversion as a separate patch.
> > 
> 
> I will fix.

Hi Frank

There is no need to say this. Please just reply to parts you want to
add more information, ask questions, or disagree with etc.

> >> +/* Phy Serdes analog cfg2 Register */
> >> +#define YTPHY_SERDES_ANALOG_CFG2_REG		0xA1
> >> +#define YTPHY_SAC2R_TX_AMPLITUDE_MASK		((0x7 << 13) | (0x7 << 1))
> >> +#define YT8521_SAC2R_TX_AMPLITUDE_LOW		((0x7 << 13) | (0x0 << 1))
> >> +#define YT8521_SAC2R_TX_AMPLITUDE_MIDDLE	((0x5 << 13) | (0x5 << 1))
> >> +#define YT8521_SAC2R_TX_AMPLITUDE_HIGH		((0x3 << 13) | (0x6 << 1))
> > 
> > So there are two values which control the amplitude? Buts 1-3, and bit
> > 7-9?  Can they be used independently?  Also, 7, 5, 3 is also add. Does
> > bit 0 of this value have some special meaning? Please document this
> > fully.
> > 
> 
> There are three values which control the amplitude,  13-15,3 and 1-2.
> They be used independently. Maybe it is good to expose three type ( 0:
> low;   1: middle;   2: high)to dts.
> 
> Bit	Symbol		Access	default	Description
> 15:13	Tx_swing_sel	RW	0x0	TX driver stage2 amplitude control
> 12	Tx_driver_stg1	RW	0x1	TX driver stage1 amplitude control
> 11	Reserved	RO	0x0	Reserved
> 10:8	Tx_ckdiv10_con	RW	0x0	tx divide by 10 clock delay control
> 7:4	Reserved	RO	0x0	Reserved
> 3	Tx_post_stg1	RW	0x0	TX driver post stage1 amplitude control
> 2:1	Tx_de_sel	RW	0x0	TX driver post stage2 amplitude control
> 0	Tx_pd		RW	0x0	power down analog tx

It would be nice to add defines for the individual parts, and then
combine them to give YT8521_SAC2R_TX_AMPLITUDE_HIGH. We then have full
documentation of the hardware.

> >> +#define YT8531S_SAC2R_TX_AMPLITUDE_LOW		((0x0 << 13) | (0x0 << 1))
> >> +#define YT8531S_SAC2R_TX_AMPLITUDE_MIDDLE	((0x0 << 13) | (0x1 << 1))
> >> +#define YT8531S_SAC2R_TX_AMPLITUDE_HIGH		((0x0 << 13) | (0x2 << 1))
> > 
> > This more sense, but why the 0 << 13? What do the bits 13-? mean?
> 
> 0 << 13 means 13-15 bit all zero.
> bits 13- mans the start bit is 13.

Sorry, my question was unclear. Why do bits 13-14 need to be zero? It
suggests these bits have some meaning. But the defines do not explain
the meaning.

> >> +	if (!of_property_read_u32(node, "motorcomm,rx-delay-additional-ps", &val)) {
> >> +		if (val > YTPHY_DTS_MAX_DELAY_VAL) {
> >> +			phydev_err(phydev, "invalid motorcomm,rx-delay-additional-ps\n");
> >> +			return -EINVAL;
> >> +		}
> > 
> > Please check the value is also one of the supported values. Please do
> > that for all your delays.
> > 
> 
> The following code check the supported values.
> 
> if (val)
> 			val /= YTPHY_DTS_STEP_DELAY_VAL;
> 		priv->rx_delay_additional = val;

It does not check it is a supported value. Say DT contained 42? That
is not a supported value. the /= YTPHY_DTS_STEP_DELAY_VAL will
calculate 0, which is the nearest rounded down value, but it will not
return -EINVAL.

> rx-delay-additional-ps = reg_val(0-15) *150ps. so val >
> YTPHY_DTS_MAX_DELAY_VAL 2250 is err.
> rx_delay_additional is store the reg_val (0-15).
> 
> >> +	if (of_property_read_bool(node, "motorcomm,keep-pll-enabled"))
> >> +		priv->keep_pll_enabled = true;
> > 
> > I think this only makes sense when priv->clock_output is true? Please
> > test for that.
> > 
> 
> No,priv->clock_output is used to output 25Mhz or 125Mhz for mac.
> priv->keep_pll_enabled is used to enable RXC clock when no wire plug.

So if priv->clock_output is set to 25MHz, it will always output 25Mhz,
independent of if there is a link peer or not?

Sometimes this clock is the recovered clock from the line, and if
there is no link peer, there is no recovered clock. In order to keep
this clock ticking, you need to keep the PLL locked by supplying it
from the local oscillator. I'm just trying to understand if that is
what is happening here. But you say it is completely independent. O.K.

     Andrew

[PATCH net-next v1 3/3] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy

[Frank]

Add driver for Motorcomm yt8531 gigabit ethernet phy. This patch has
been tested on AM335x platform which has one YT8531 interface
card and passed all test cases.

Signed-off-by: Frank <Frank.Sae@motor-comm.com>
---
 drivers/net/phy/Kconfig     |   2 +-
 drivers/net/phy/motorcomm.c | 127 +++++++++++++++++++++++++++++++++++-
 2 files changed, 126 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 1327290decab..e25c061e619a 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -257,7 +257,7 @@ config MOTORCOMM_PHY
 	tristate "Motorcomm PHYs"
 	help
 	  Enables support for Motorcomm network PHYs.
-	  Currently supports the YT8511, YT8521, YT8531S Gigabit Ethernet PHYs.
+	  Currently supports the YT8511, YT8521, YT8531, YT8531S Gigabit Ethernet PHYs.
 
 config NATIONAL_PHY
 	tristate "National Semiconductor PHYs"
diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
index 7ebcca374a67..23d7e48587cf 100644
--- a/drivers/net/phy/motorcomm.c
+++ b/drivers/net/phy/motorcomm.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
- * Motorcomm 8511/8521/8531S PHY driver.
+ * Motorcomm 8511/8521/8531/8531S PHY driver.
  *
  * Author: Peter Geis <pgwipeout@gmail.com>
  * Author: Frank <Frank.Sae@motor-comm.com>
@@ -14,6 +14,7 @@
 
 #define PHY_ID_YT8511		0x0000010a
 #define PHY_ID_YT8521		0x0000011a
+#define PHY_ID_YT8531		0x4f51e91b
 #define PHY_ID_YT8531S		0x4f51e91a
 
 /* YT8521/YT8531S Register Overview
@@ -542,6 +543,69 @@ static int ytphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 	return phy_restore_page(phydev, old_page, ret);
 }
 
+static int yt8531_set_wol(struct phy_device *phydev,
+			  struct ethtool_wolinfo *wol)
+{
+	struct net_device *p_attached_dev;
+	const u16 mac_addr_reg[] = {
+		YTPHY_WOL_MACADDR2_REG,
+		YTPHY_WOL_MACADDR1_REG,
+		YTPHY_WOL_MACADDR0_REG,
+	};
+	const u8 *mac_addr;
+	u16 mask;
+	u16 val;
+	int ret;
+	u8 i;
+
+	if (wol->wolopts & WAKE_MAGIC) {
+		p_attached_dev = phydev->attached_dev;
+		if (!p_attached_dev)
+			return -ENODEV;
+
+		mac_addr = (const u8 *)p_attached_dev->dev_addr;
+		if (!is_valid_ether_addr(mac_addr))
+			return -EINVAL;
+
+		/* Store the device address for the magic packet */
+		for (i = 0; i < 3; i++) {
+			ret = ytphy_write_ext_with_lock(phydev, mac_addr_reg[i],
+							((mac_addr[i * 2] << 8)) |
+							(mac_addr[i * 2 + 1]));
+			if (ret < 0)
+				return ret;
+		}
+
+		/* Enable WOL feature */
+		mask = YTPHY_WCR_PULSE_WIDTH_MASK | YTPHY_WCR_INTR_SEL;
+		val = YTPHY_WCR_ENABLE | YTPHY_WCR_INTR_SEL;
+		val |= YTPHY_WCR_TYPE_PULSE | YTPHY_WCR_PULSE_WIDTH_672MS;
+		ret = ytphy_modify_ext_with_lock(phydev, YTPHY_WOL_CONFIG_REG,
+						 mask, val);
+		if (ret < 0)
+			return ret;
+
+		/* Enable WOL interrupt */
+		ret = phy_modify(phydev, YTPHY_INTERRUPT_ENABLE_REG, 0,
+				 YTPHY_IER_WOL);
+		if (ret < 0)
+			return ret;
+	} else {
+		/* Disable WOL feature */
+		mask = YTPHY_WCR_ENABLE | YTPHY_WCR_INTR_SEL;
+		ret = ytphy_modify_ext_with_lock(phydev, YTPHY_WOL_CONFIG_REG,
+						 mask, 0);
+
+		/* Disable WOL interrupt */
+		ret = phy_modify(phydev, YTPHY_INTERRUPT_ENABLE_REG,
+				 YTPHY_IER_WOL, 0);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int yt8511_read_page(struct phy_device *phydev)
 {
 	return __phy_read(phydev, YT8511_PAGE_SELECT);
@@ -1032,6 +1096,11 @@ static int yt8521_probe(struct phy_device *phydev)
 	return 0;
 }
 
+static int yt8531_probe(struct phy_device *phydev)
+{
+	return ytphy_probe_helper(phydev);
+}
+
 /**
  * ytphy_utp_read_lpa() - read LPA then setup lp_advertising for utp
  * @phydev: a pointer to a &struct phy_device
@@ -1543,6 +1612,48 @@ static int yt8521_config_init(struct phy_device *phydev)
 	return phy_restore_page(phydev, old_page, ret);
 }
 
+static int yt8531_config_init(struct phy_device *phydev)
+{
+	int ret;
+
+	phy_lock_mdio_bus(phydev);
+	ret = ytphy_config_init_helper(phydev);
+	phy_unlock_mdio_bus(phydev);
+
+	return ret;
+}
+
+static void yt8531_link_change_notify(struct phy_device *phydev)
+{
+	struct yt8521_priv *priv = phydev->priv;
+	u16 val = 0;
+
+	if (!(priv->tx_clk_adj_enabled))
+		return;
+
+	if (phydev->speed < 0)
+		return;
+
+	switch (phydev->speed) {
+	case SPEED_1000:
+		if (priv->tx_clk_1000_inverted)
+			val = YT8521_RC1R_TX_CLK_SEL_INVERTED;
+		break;
+	case SPEED_100:
+		if (priv->tx_clk_100_inverted)
+			val = YT8521_RC1R_TX_CLK_SEL_INVERTED;
+		break;
+	case SPEED_10:
+		if (priv->tx_clk_10_inverted)
+			val = YT8521_RC1R_TX_CLK_SEL_INVERTED;
+		break;
+	default:
+		return;
+	}
+	ytphy_modify_ext_with_lock(phydev, YT8521_RGMII_CONFIG1_REG,
+				   YT8521_RC1R_TX_CLK_SEL_MASK, val);
+}
+
 /**
  * yt8521_prepare_fiber_features() -  A small helper function that setup
  * fiber's features.
@@ -2125,6 +2236,17 @@ static struct phy_driver motorcomm_phy_drvs[] = {
 		.suspend	= yt8521_suspend,
 		.resume		= yt8521_resume,
 	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_YT8531),
+		.name		= "YT8531 Gigabit Ethernet",
+		.probe		= yt8531_probe,
+		.config_init	= yt8531_config_init,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.get_wol	= ytphy_get_wol,
+		.set_wol	= yt8531_set_wol,
+		.link_change_notify = yt8531_link_change_notify,
+	},
 	{
 		PHY_ID_MATCH_EXACT(PHY_ID_YT8531S),
 		.name		= "YT8531S Gigabit Ethernet",
@@ -2146,7 +2268,7 @@ static struct phy_driver motorcomm_phy_drvs[] = {
 
 module_phy_driver(motorcomm_phy_drvs);
 
-MODULE_DESCRIPTION("Motorcomm 8511/8521/8531S PHY driver");
+MODULE_DESCRIPTION("Motorcomm 8511/8521/8531/8531S PHY driver");
 MODULE_AUTHOR("Peter Geis");
 MODULE_AUTHOR("Frank");
 MODULE_LICENSE("GPL");
@@ -2154,6 +2276,7 @@ MODULE_LICENSE("GPL");
 static const struct mdio_device_id __maybe_unused motorcomm_tbl[] = {
 	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8511) },
 	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8521) },
+	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8531) },
 	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8531S) },
 	{ /* sentinel */ }
 };
-- 
2.34.1