[PATCH net-next v21 0/3] net: phy: Add driver for Motorcomm Quad 2.5GbE phy

[PATCH net-next v21 0/3] net: phy: Add driver for Motorcomm Quad 2.5GbE phy

[Kyle Switch]

This patchset mainly implements the phy8824 driver. The phy8824 is
an Ethernet PHY that provides one 10G SerDes output to four 2.5G ports.
The driver mainly covers two application scenarios:
	1. external PHY8824
	2. PHY8824 embedded in a switch.
The patchset mainly consists of three parts:
	1. the DTS for the phy8824
	2. a generic template testmode configuration api.
	3. the phy8824 functional functions.

changes in v21:
patch 1: 1) Add maintainer for motorcomm,yt8824.yaml.
	 2) Using 10g-qxgmii instead of usxgmii.
patch 3: 1) Change the usxgmii space to serdes space.
         2) Add lock operation during phy-package init in probe()
         3) Add check for whether auto-negotiation is enabled
	    when updating the link status in read_status().

changes in v20:
patch 1: Add phy-mode description in properties in motorcomm,yt8824.yaml

changes in v19:
patch 2: Replace the int type with the u16 type for test_mode.
patch 3: 1) Merge yt8824_utp_normal_test_mode_paged() and
         yt8824_utp_invalid_test_mode_paged() into 
         yt8824_utp_set_template_test_mode().
	 2) Add a fatal error return when phy-mode is missing in
         the DTS.

changes in v18:
1. Split the template_testmode() into a small patch 
   and optimize the implementation.
2. Optimize the definition and initialization of 
   interface_mode by adopting the existing approach.

changes in v17:
patch 1: Remove redundant description.
         Use phy-mode instead of motorcomm,interface to describe the
         interface.
         Add the required checks.
patch 2: Use phy-mode to get interface mode.
         Need return after init configuration is done to avoid falling
         into the error path.

changes in v16:
1. Add DTS documentation.
2. Fix redundant initialization.

changes in v14:
1. Refactor the error return path to preserve and return the initial error,
   rather than letting later errors mask it.

changes in v13:
1. Fix the lock release issue on the error path to prevent potential
   deadlock or resource leak.
2. Add a func yt8824_restore_defaults() to restore configuration on
   the error path,ensuring the hardware/device is left in a known
   good state upon failure.

changes in v12:
1. Refactor the yt8824_read_status() function to ensure proper state
   synchronization between hardware registers and the phy_device structure.

changes in v11:
1. Clean build_clang warning.

changes in v10:
1. Within the interface that swaps to the USXGMII reg space,the lock
   include shared_lock and mdio mutex must remain held throughout 
   the entire operation and should only be released after all steps
   have completed.
2. Improve the template interface in phy-c45.c
3. Fix the handling of some failure paths.

changes in v9:
1. Add shared_lock mutex prevents UTPs from interfering with each other 
   due to swapping reg space.

change in v8:
1. Clean up format warning.
2. Fix exception handling code logic based on Sashiko/Gemini.
3. Remove interrupt/handle function

changes in v7:
1. Refactor the using of mdio lock
  In all cases of swapping to the USXGMII side interface, lock the MDIO bus
  at the beginning, switch to the UTP address space after the operation is
  completed, and then release the lock.The purpose of doing this is to
  ensure that it will not affect other UTPs operating in the UTP address
  space.
2. Rename YT8824_RSSR_FIBER_SPACE to YT8824_RSSR_USXGMII_SPACE.

changes in v6:
1. Add  test mode helper in phy-c45.c
2. Refactor the using of swapping paged for utp and fiber

changes in v5:
1. Fix diffs issue which caused by unexpected whitespace.
2. Fix "exceeding 80 columns" warning.

changes in v4:
1. Remove motorcomm,yt8xxx.yaml, will update in other patch thread
2. Fix locking issue. Since every interface requires switching of space, 
   the bus is already locked during the space switching process in
   phy_select_page(), other operations within the interface cannot 
   be locked again before unlock in phy_restore_page().
3. Fix warning log identified during the inspection process using
   checkpatch.pl.
4. Fix the way of using common api in phy_package.c


changes in v3:
1. Using common apis defined in phy_package.c to handle shared top
extend register space.
2. Add dts demo in motorcomm,yt8xxx.yaml.
3. Fix unnecessary redundant judgments.
4. Fix BMCR registers operation using magic number.
5. Rename function based on its approximate functionality.

changes in v2:
1. Remove duplicate code and replace it with existing api.

Kyle Switch (3):
  dt-bindings: net: Document Motorcomm YT8824 PHY package
  net: phy: Add support for Template Control register for PMA
  net: phy: Add driver for Motorcomm Quad 2.5GbE phy

 .../bindings/net/motorcomm,yt8824.yaml        |   69 +
 MAINTAINERS                                   |    2 +
 drivers/net/phy/Kconfig                       |    3 +-
 drivers/net/phy/motorcomm.c                   | 1585 ++++++++++++++++-
 drivers/net/phy/phy-c45.c                     |   23 +
 include/linux/phy.h                           |    1 +
 include/uapi/linux/mdio.h                     |   12 +
 7 files changed, 1692 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml

-- 
2.25.1

[PATCH net-next v21 1/3] dt-bindings: net: Document Motorcomm YT8824 PHY package

[Kyle Switch]

Motorcomm YT8824 Ethernet PHY is PHY package of 4 PHY-s.

Signed-off-by: Kyle Switch <kyle.switch@motor-comm.com>
---
 .../bindings/net/motorcomm,yt8824.yaml        | 69 +++++++++++++++++++
 MAINTAINERS                                   |  2 +
 2 files changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml

diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
new file mode 100644
index 000000000000..fd62b309e7ff
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/motorcomm,yt8824.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MotorComm YT8824 Ethernet PHY
+
+maintainers:
+  - Kyle Switch <kyle.switch@motor-comm.com>
+
+description:
+  Motorcomm YT8824 Ethernet PHY is a PHY package of 4 PHYs.
+
+$ref: ethernet-phy-package.yaml#
+
+properties:
+  compatible:
+    enum:
+      - motorcomm,yt8824-package
+  phy-mode:
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [ internal, 10g-qxgmii ]
+  reg:
+    description:
+      The absolute MDIO address of the YT8824 shared top extend
+      register block.
+      For internal mode, this is fixed at 9;
+      For external mode it is (base MDIO address of the four
+      member PHYs) + 4.
+      Note that this is NOT the base ID of the first member PHY.
+required:
+  - compatible
+  - phy-mode
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-phy-package@9 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "motorcomm,yt8824-package";
+            reg = <9>;
+
+            phy-mode = "internal";
+
+            ethernet-phy@4 {
+                reg = <4>;
+            };
+
+            ethernet-phy@5 {
+                reg = <5>;
+            };
+
+            ethernet-phy@6 {
+                reg = <6>;
+            };
+
+            ethernet-phy@7 {
+                reg = <7>;
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index e3ce77c839b0..ed7af1bd9521 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18409,8 +18409,10 @@ F:	drivers/net/ethernet/stmicro/stmmac/dwmac-motorcomm.c
 
 MOTORCOMM PHY DRIVER
 M:	Frank <Frank.Sae@motor-comm.com>
+M:	Kyle Switch <kyle.switch@motor-comm.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
 F:	Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
 F:	drivers/net/phy/motorcomm.c
 
-- 
2.25.1

Re: [PATCH net-next v21 1/3] dt-bindings: net: Document Motorcomm YT8824 PHY package

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3560 bytes --]

On Thu, Sep 24, 2026 at 03:50:46PM +0800, Kyle Switch wrote:
> Motorcomm YT8824 Ethernet PHY is PHY package of 4 PHY-s.
> 
> Signed-off-by: Kyle Switch <kyle.switch@motor-comm.com>
> ---
>  .../bindings/net/motorcomm,yt8824.yaml        | 69 +++++++++++++++++++
>  MAINTAINERS                                   |  2 +
>  2 files changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
> new file mode 100644
> index 000000000000..fd62b309e7ff
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/motorcomm,yt8824.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MotorComm YT8824 Ethernet PHY
> +
> +maintainers:
> +  - Kyle Switch <kyle.switch@motor-comm.com>
> +
> +description:
> +  Motorcomm YT8824 Ethernet PHY is a PHY package of 4 PHYs.
> +
> +$ref: ethernet-phy-package.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - motorcomm,yt8824-package
> +  phy-mode:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ internal, 10g-qxgmii ]

This should be after ref, but also have a vendor prefix.
Additionally, the qcom ethernet-phy-package user also has a mode
property. Net folks, should this be made common?

> +  reg:
> +    description:
> +      The absolute MDIO address of the YT8824 shared top extend
> +      register block.
> +      For internal mode, this is fixed at 9;
> +      For external mode it is (base MDIO address of the four
> +      member PHYs) + 4.
> +      Note that this is NOT the base ID of the first member PHY.
> +required:

Please use blank lines between properties and other elements of the
binding.


> +  - compatible
> +  - phy-mode
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ethernet-phy-package@9 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "motorcomm,yt8824-package";
> +            reg = <9>;
> +
> +            phy-mode = "internal";
> +
> +            ethernet-phy@4 {
> +                reg = <4>;
> +            };
> +
> +            ethernet-phy@5 {
> +                reg = <5>;
> +            };
> +
> +            ethernet-phy@6 {
> +                reg = <6>;
> +            };
> +
> +            ethernet-phy@7 {
> +                reg = <7>;
> +            };

Unless the addresses and numbers of phys are entirely unconstrained, I
think you should add some pattern properties for them.

Thanks,
Conor.

> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e3ce77c839b0..ed7af1bd9521 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18409,8 +18409,10 @@ F:	drivers/net/ethernet/stmicro/stmmac/dwmac-motorcomm.c
>  
>  MOTORCOMM PHY DRIVER
>  M:	Frank <Frank.Sae@motor-comm.com>
> +M:	Kyle Switch <kyle.switch@motor-comm.com>
>  L:	netdev@vger.kernel.org
>  S:	Maintained
> +F:	Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
>  F:	Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
>  F:	drivers/net/phy/motorcomm.c
>  
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v21 1/3] dt-bindings: net: Document Motorcomm YT8824 PHY package

[Andrew Lunn]

> > +$ref: ethernet-phy-package.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - motorcomm,yt8824-package
> > +  phy-mode:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [ internal, 10g-qxgmii ]
> 
> This should be after ref, but also have a vendor prefix.
> Additionally, the qcom ethernet-phy-package user also has a mode
> property. Net folks, should this be made common?

phy-mode is definitely wrong, it has a different meaning, and reusing
it is just going to cause confusion.

qcom,package-mode does have the same meaning as what is trying to be
expressed here. So yes, a common, vendor independent property would
make sense. It maybe should be in ethernet-phy-package.

	Andrew

Re: [PATCH net-next v21 1/3] dt-bindings: net: Document Motorcomm YT8824 PHY package

[Kyle Switch]

On 9/25/26 03:16, Andrew Lunn wrote:
>>> +$ref: ethernet-phy-package.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - motorcomm,yt8824-package
>>> +  phy-mode:
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +    enum: [ internal, 10g-qxgmii ]
>> This should be after ref, but also have a vendor prefix.
>> Additionally, the qcom ethernet-phy-package user also has a mode
>> property. Net folks, should this be made common?
> phy-mode is definitely wrong, it has a different meaning, and reusing
> it is just going to cause confusion.
>
> qcom,package-mode does have the same meaning as what is trying to be
> expressed here. So yes, a common, vendor independent property would
> make sense. It maybe should be in ethernet-phy-package.

Ans: okay, In the next version, I will revert the previous

         implementation and follow the qcom,package-related approach.

>
> 	Andrew

Re: [PATCH net-next v21 1/3] dt-bindings: net: Document Motorcomm YT8824 PHY package

[Kyle Switch]

On 9/28/26 09:45, Kyle Switch wrote:
>
> On 9/25/26 03:16, Andrew Lunn wrote:
>>>> +$ref: ethernet-phy-package.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - motorcomm,yt8824-package
>>>> +  phy-mode:
>>>> +    $ref: /schemas/types.yaml#/definitions/string
>>>> +    enum: [ internal, 10g-qxgmii ]
>>> This should be after ref, but also have a vendor prefix.
>>> Additionally, the qcom ethernet-phy-package user also has a mode
>>> property. Net folks, should this be made common?
>> phy-mode is definitely wrong, it has a different meaning, and reusing
>> it is just going to cause confusion.
>>
>> qcom,package-mode does have the same meaning as what is trying to be
>> expressed here. So yes, a common, vendor independent property would
>> make sense. It maybe should be in ethernet-phy-package.
>
> Ans: okay, In the next version, I will revert the previous
>
>         implementation and follow the qcom,package-related approach.

Ans: Another question I'd like to get your opinions on. The usage modes of

the phy8824 are not as numerous as those covered by the qcom package.

What we're trying to express here is simply whether it's a phy8824 built 
into the switch or

a standalone phy8824. In both scenarios, there is one 10G serdes 
outputting four UTPs,

and the only difference is that some operations are slightly different, 
such as soft reset,

power down, and power up. How should these two types be defined and 
identified?

In a previous patch version, the "internal" and "10g-qxgmii" modes were 
used to

represent them. Andrew pointed out that "internal" is already a mode 
included in phy-mode,

which could cause confusion, so in subsequent versions phy-mode was used 
directly.

Now phy-mode needs to be replaced with a vendor-defined mode, but since 
the meaning

is different from what qcom expresses, it's not good to make it a common 
definition,

so we'd like to ask for your suggestions on how to identify internal vs. 
external phy8824 more appropriately.

>
>>
>>     Andrew

Re: [PATCH net-next v21 1/3] dt-bindings: net: Document Motorcomm YT8824 PHY package

[Andrew Lunn]

> What we're trying to express here is simply whether it's a phy8824 built
> into the switch or
> 
> a standalone phy8824. In both scenarios, there is one 10G serdes outputting
> four UTPs,
> 
> and the only difference is that some operations are slightly different, such
> as soft reset,

If it is a different PHY, it should have a different ID value in
registers 2 and 3.

> power down, and power up. How should these two types be defined and
> identified?

More details needed. Why does it need a different soft reset? Why is
power up/down different?

	Andrew

Re: [PATCH net-next v21 1/3] dt-bindings: net: Document Motorcomm YT8824 PHY package

[Kyle Switch]

On 9/25/26 01:07, Conor Dooley wrote:
> On Thu, Sep 24, 2026 at 03:50:46PM +0800, Kyle Switch wrote:
>> Motorcomm YT8824 Ethernet PHY is PHY package of 4 PHY-s.
>>
>> Signed-off-by: Kyle Switch <kyle.switch@motor-comm.com>
>> ---
>>   .../bindings/net/motorcomm,yt8824.yaml        | 69 +++++++++++++++++++
>>   MAINTAINERS                                   |  2 +
>>   2 files changed, 71 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
>> new file mode 100644
>> index 000000000000..fd62b309e7ff
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
>> @@ -0,0 +1,69 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/motorcomm,yt8824.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MotorComm YT8824 Ethernet PHY
>> +
>> +maintainers:
>> +  - Kyle Switch <kyle.switch@motor-comm.com>
>> +
>> +description:
>> +  Motorcomm YT8824 Ethernet PHY is a PHY package of 4 PHYs.
>> +
>> +$ref: ethernet-phy-package.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - motorcomm,yt8824-package
>> +  phy-mode:
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    enum: [ internal, 10g-qxgmii ]
> This should be after ref, but also have a vendor prefix.
> Additionally, the qcom ethernet-phy-package user also has a mode
> property. Net folks, should this be made common?
>
Ans: okay, The next version will follow the approach used by the qcom, 
package.
>> +  reg:
>> +    description:
>> +      The absolute MDIO address of the YT8824 shared top extend
>> +      register block.
>> +      For internal mode, this is fixed at 9;
>> +      For external mode it is (base MDIO address of the four
>> +      member PHYs) + 4.
>> +      Note that this is NOT the base ID of the first member PHY.
>> +required:
> Please use blank lines between properties and other elements of the
> binding.
>
Ans: okay.
>> +  - compatible
>> +  - phy-mode
>> +  - reg
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    mdio {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        ethernet-phy-package@9 {
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +            compatible = "motorcomm,yt8824-package";
>> +            reg = <9>;
>> +
>> +            phy-mode = "internal";
>> +
>> +            ethernet-phy@4 {
>> +                reg = <4>;
>> +            };
>> +
>> +            ethernet-phy@5 {
>> +                reg = <5>;
>> +            };
>> +
>> +            ethernet-phy@6 {
>> +                reg = <6>;
>> +            };
>> +
>> +            ethernet-phy@7 {
>> +                reg = <7>;
>> +            };
> Unless the addresses and numbers of phys are entirely unconstrained, I
> think you should add some pattern properties for them.

Ans: I'm somewhat confused on this point. What might the pattern properties

         you mentioned include? Could you provide more detail? thank you.

>
> Thanks,
> Conor.
>
>> +        };
>> +    };
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e3ce77c839b0..ed7af1bd9521 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -18409,8 +18409,10 @@ F:	drivers/net/ethernet/stmicro/stmmac/dwmac-motorcomm.c
>>   
>>   MOTORCOMM PHY DRIVER
>>   M:	Frank <Frank.Sae@motor-comm.com>
>> +M:	Kyle Switch <kyle.switch@motor-comm.com>
>>   L:	netdev@vger.kernel.org
>>   S:	Maintained
>> +F:	Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
>>   F:	Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
>>   F:	drivers/net/phy/motorcomm.c
>>   
>> -- 
>> 2.25.1
>>

Re: [PATCH net-next v21 1/3] dt-bindings: net: Document Motorcomm YT8824 PHY package

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

On Mon, Sep 28, 2026 at 09:43:48AM +0800, Kyle Switch wrote:

> > > +unevaluatedProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    mdio {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        ethernet-phy-package@9 {
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +            compatible = "motorcomm,yt8824-package";
> > > +            reg = <9>;
> > > +
> > > +            phy-mode = "internal";
> > > +
> > > +            ethernet-phy@4 {
> > > +                reg = <4>;
> > > +            };
> > > +
> > > +            ethernet-phy@5 {
> > > +                reg = <5>;
> > > +            };
> > > +
> > > +            ethernet-phy@6 {
> > > +                reg = <6>;
> > > +            };
> > > +
> > > +            ethernet-phy@7 {
> > > +                reg = <7>;
> > > +            };
> > Unless the addresses and numbers of phys are entirely unconstrained, I
> > think you should add some pattern properties for them.
> 
> Ans: I'm somewhat confused on this point. What might the pattern properties

There's no need for this "ans" business, the quoting makes it clear
already.

>         you mentioned include? Could you provide more detail? thank you.

On second thoughts, there's some basic constraints already provided by
ethernet-phy-package for this. Probably don't need to anyhting specific.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v21 1/3] dt-bindings: net: Document Motorcomm YT8824 PHY package

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 5

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new binding
  Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml redefines…
- [Low] The patch adds `M: Kyle Switch <kyle.switch@motor-comm.com>` as a
  second maintainer for the whole existing MOTORCOMM PHY DRIVER section…
- [Low] The `reg` description in motorcomm,yt8824.yaml gives its address
  rule in terms of 'internal mode' and 'external mode'.
- [Low] (Discarded as a false positive in the final series state.) The
  concern was that no in-tree code reads the required `phy-mode` or uses…
- [Low] motorcomm,yt8824.yaml says the package `reg` is 'fixed at 9' in
  internal mode, but the schema does not enforce it.
- [Low] (Discarded as a false positive.) The concern was that the example
  (member PHYs at 4..7, package reg 9) contradicts the 'base + 4' rule.

--

dt-bindings: net: Document Motorcomm YT8824 PHY package

This commit adds a devicetree binding for the Motorcomm YT8824, a package
of four Ethernet PHYs. The binding is built on ethernet-phy-package.yaml.
The commit also updates the MOTORCOMM PHY DRIVER section in MAINTAINERS
with the new file and a new maintainer.

> diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
> new file mode 100644
> index 000000000000..fd62b309e7ff
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
> @@ -0,0 +1,69 @@

[ ... ]

> +properties:
> +  compatible:
> +    enum:
> +      - motorcomm,yt8824-package
> +  phy-mode:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ internal, 10g-qxgmii ]

[Severity: Medium]
Would a vendor-prefixed property fit better here than phy-mode?

ethernet-controller.yaml defines phy-mode as the interface type between
the MAC and the PHY:

  phy-mode:
    $ref: "#/properties/phy-connection-type"

This binding gives phy-mode its own string type and enum on the package
node and uses it to pick the package operating mode. In the driver patch
later in this series, yt8824_phy_package_probe_once() reads it only to
choose between the internal and external init/reset sequences:

  res = of_get_phy_mode(np, &priv->interface_mode);

qcom,qca807x.yaml has a similar PHY package and uses a vendor property
for the same purpose:

  qcom,package-mode:
    ...
    $ref: /schemas/types.yaml#/definitions/string
    enum:
      - qsgmii
      - psgmii

writing-bindings.rst also asks for a vendor prefix on device-specific
property names.

This value on the package node also duplicates the MAC's phy-mode, which
phylink already passes to the PHY as phydev->interface. Can the two
disagree? The YT8824 driver never looks at phydev->interface.

phy-mode is also listed under required, so every YT8824 DT will carry it.
Changing its name or meaning after merge would break those DTs.

> +  reg:
> +    description:
> +      The absolute MDIO address of the YT8824 shared top extend
> +      register block.
> +      For internal mode, this is fixed at 9;
> +      For external mode it is (base MDIO address of the four
> +      member PHYs) + 4.
> +      Note that this is NOT the base ID of the first member PHY.

[Severity: Low]
Which phy-mode value does "external mode" mean? The phy-mode enum above
only allows internal and 10g-qxgmii. The text never says that the base + 4
rule applies to phy-mode = "10g-qxgmii".

That can only be worked out from the driver patch later in the series,
which treats any mode other than PHY_INTERFACE_MODE_INTERNAL as external.

The value matters. of_phy_package_join() stores this reg as
shared->base_addr, and ytphy_read_top_ext() and ytphy_write_top_ext()
reach the top block at that address.

[Severity: Low]
Could the schema enforce the "fixed at 9" rule for internal mode? No
allOf/if block ties reg to phy-mode, so a node with phy-mode = "internal"
and reg = <5> still passes dt_binding_check and dtbs_check.

Something along these lines would cover it:

  allOf:
    - if:
        properties:
          phy-mode:
            const: internal
      then:
        properties:
          reg:
            const: 9

With the driver added later in the series, a wrong reg value is used
directly at runtime:

of_phy_package_join()
  phy_package_join(phydev, base_addr, ...)

ytphy_write_top_ext()
  __phy_package_write(phydev, 0, YTPHY_PAGE_SELECT, ...)

In that case, writes to the top block would go to the wrong MDIO device.
The schema can't express the external base + 4 rule, but it can express
the internal-mode constant.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index e3ce77c839b0..ed7af1bd9521 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18409,8 +18409,10 @@ F:	drivers/net/ethernet/stmicro/stmmac/dwmac-motorcomm.c
>  
>  MOTORCOMM PHY DRIVER
>  M:	Frank <Frank.Sae@motor-comm.com>
> +M:	Kyle Switch <kyle.switch@motor-comm.com>

[Severity: Low]
This line makes Kyle Switch a second maintainer of the whole MOTORCOMM PHY
DRIVER section. That section also covers drivers/net/phy/motorcomm.c and
motorcomm,yt8xxx.yaml, not just the new binding.

The commit message only says:

  Motorcomm YT8824 Ethernet PHY is PHY package of 4 PHY-s.

It doesn't mention the maintainer change, and there's no Acked-by from the
current maintainer.

Could this be split into its own patch, or at least be described in the
changelog?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924075048.4060414-1-kyle.switch%40motor-comm.com

[PATCH net-next v21 2/3] net: phy: Add support for Template Control register for PMA

[Kyle Switch]

Add support for the 10GBASE-T PMA Template Test Mode register
field, which allows selecting one of eight test modes (Normal and
TestMode1..TestMode7) used for PHY validation.

Signed-off-by: Kyle Switch <kyle.switch@motor-comm.com>
---
 drivers/net/phy/phy-c45.c | 23 +++++++++++++++++++++++
 include/linux/phy.h       |  1 +
 include/uapi/linux/mdio.h | 12 ++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 870920311f9a..c5f5753f7194 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1408,6 +1408,29 @@ int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable)
 }
 EXPORT_SYMBOL_GPL(genphy_c45_fast_retrain);
 
+/**
+ * genphy_c45_template_testmode - configure template testmode registers
+ * @phydev: target phy_device struct
+ * @test_mode: testmode includes Normal to Test mode 7
+ *
+ * Description: Set template testmode include Normal to Test mode 7
+ *
+ * Return: 0 on success, or a negative error code on failure (e.g. register
+ * read/write error).
+ */
+int genphy_c45_template_testmode(struct phy_device *phydev, u16 test_mode)
+{
+	u16 ctrl;
+
+	if (test_mode > MDIO_PMA_10GBT_TESTMODE_7)
+		return -EOPNOTSUPP;
+
+	ctrl = FIELD_PREP(MDIO_PMA_10GBT_TESTMODE_MASK, test_mode);
+	return phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_10GBT_TESTMODE,
+			      MDIO_PMA_10GBT_TESTMODE_MASK, ctrl);
+}
+EXPORT_SYMBOL_GPL(genphy_c45_template_testmode);
+
 /**
  * genphy_c45_plca_get_cfg - get PLCA configuration from standard registers
  * @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7c5098a0dd6c..b9dee2655e6b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2357,6 +2357,7 @@ int genphy_c45_loopback(struct phy_device *phydev, bool enable, int speed);
 int genphy_c45_pma_resume(struct phy_device *phydev);
 int genphy_c45_pma_suspend(struct phy_device *phydev);
 int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable);
+int genphy_c45_template_testmode(struct phy_device *phydev, u16 test_mode);
 int genphy_c45_plca_get_cfg(struct phy_device *phydev,
 			    struct phy_plca_cfg *plca_cfg);
 int genphy_c45_plca_set_cfg(struct phy_device *phydev,
diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index 06f4bc3c20c7..353ca13c64fc 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h
@@ -63,6 +63,7 @@
 /* Media-dependent registers. */
 #define MDIO_PMA_10GBT_SWAPPOL	130	/* 10GBASE-T pair swap & polarity */
 #define MDIO_PMA_10GBT_TXPWR	131	/* 10GBASE-T TX power control */
+#define MDIO_PMA_10GBT_TESTMODE 132 /* Test mode control */
 #define MDIO_PMA_10GBT_SNR	133	/* 10GBASE-T SNR margin, lane A.
 					 * Lanes B-D are numbered 134-136. */
 #define MDIO_PMA_10GBR_FSRT_CSR	147	/* 10GBASE-R fast retrain status and control */
@@ -320,6 +321,17 @@
 /* PMA 10GBASE-R Fast Retrain status and control register. */
 #define MDIO_PMA_10GBR_FSRT_ENABLE	0x0001	/* Fast retrain enable */
 
+/* PMA 10GBASE-T Template Test Mode Register*/
+#define MDIO_PMA_10GBT_TESTMODE_MASK GENMASK(15, 13) /* Template test mode */
+#define MDIO_PMA_10GBT_TESTMODE_NORMAL 0x0 /* Template Normal */
+#define MDIO_PMA_10GBT_TESTMODE_1 0x1 /* Template TestMode1 */
+#define MDIO_PMA_10GBT_TESTMODE_2 0x2 /* Template TestMode2 */
+#define MDIO_PMA_10GBT_TESTMODE_3 0x3 /* Template TestMode3 */
+#define MDIO_PMA_10GBT_TESTMODE_4 0x4 /* Template TestMode4 */
+#define MDIO_PMA_10GBT_TESTMODE_5 0x5 /* Template TestMode5 */
+#define MDIO_PMA_10GBT_TESTMODE_6 0x6 /* Template TestMode6 */
+#define MDIO_PMA_10GBT_TESTMODE_7 0x7 /* Template TestMode7 */
+
 /* PCS 10GBASE-R/-T status register 1. */
 #define MDIO_PCS_10GBRT_STAT1_BLKLK	0x0001	/* Block lock attained */
 
-- 
2.25.1

Re: [PATCH net-next v21 2/3] net: phy: Add support for Template Control register for PMA

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The naming doesn't match the register.
- [Low] The patch adds a new UAPI constant, `#define
  MDIO_PMA_10GBT_TESTMODE_MASK GENMASK(15, 13)`, at…
- [Low] phy-c45: genphy_c45_template_testmode() returns -EOPNOTSUPP for
  invalid argument

--

net: phy: Add support for Template Control register for PMA

This adds definitions to the uapi mdio.h header for the 10GBASE-T PMA
test mode register (1.132) and its test mode field. It also adds a
genphy_c45_template_testmode() helper in drivers/net/phy/phy-c45.c,
which writes one of the modes Normal or TestMode1..TestMode7 into that
field.

> diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
> index 06f4bc3c20c7..353ca13c64fc 100644
> --- a/include/uapi/linux/mdio.h
> +++ b/include/uapi/linux/mdio.h
> @@ -63,6 +63,7 @@
>  /* Media-dependent registers. */
>  #define MDIO_PMA_10GBT_SWAPPOL	130	/* 10GBASE-T pair swap & polarity */
>  #define MDIO_PMA_10GBT_TXPWR	131	/* 10GBASE-T TX power control */
> +#define MDIO_PMA_10GBT_TESTMODE 132 /* Test mode control */

[Severity: Low]
This isn't a bug, but is "Template" the right name here?

The word "Template" appears in three places:

  - The subject line: "Add support for Template Control register for
    PMA".
  - The new exported helper: genphy_c45_template_testmode() in
    drivers/net/phy/phy-c45.c.
  - The comment on every new field define.

IEEE 802.3 calls register 1.132 the 10GBASE-T test mode register, and
bits 15:13 "Test mode control". The define above already uses that
wording.

"Template" looks like vendor wording, and it becomes part of a generic
exported phylib symbol. Would names based on the standard terminology
work better for the helper and the subject line?

There are also a few small style nits:

  - This define uses spaces where the defines around it are
    tab-aligned.
  - The new MDIO_PMA_10GBT_TESTMODE_* field defines are not
    tab-aligned either.
  - The comment "/* PMA 10GBASE-T Template Test Mode Register*/" has no
    space before the terminator.

[ ... ]

> @@ -320,6 +321,17 @@
>  /* PMA 10GBASE-R Fast Retrain status and control register. */
>  #define MDIO_PMA_10GBR_FSRT_ENABLE	0x0001	/* Fast retrain enable */
>  
> +/* PMA 10GBASE-T Template Test Mode Register*/
> +#define MDIO_PMA_10GBT_TESTMODE_MASK GENMASK(15, 13) /* Template test mode */

[Severity: Low]
Can userspace actually use this new mask?

GENMASK() is kernel-internal and comes from include/linux/bits.h. The
exported include/uapi/linux/bits.h only provides __GENMASK(),
__GENMASK_ULL() and __GENMASK_U128().

This header only includes <linux/types.h> and <linux/mii.h>, and
neither of them provides GENMASK.

So a userspace program that includes <linux/mdio.h> and uses
MDIO_PMA_10GBT_TESTMODE_MASK would fail to build, because GENMASK is
undeclared. The UAPI header compile test won't catch this, since the
macro is only expanded where it is used.

The MDIO_AN_C73_* masks in this header already have the same problem.
This patch adds another exported symbol that depends on it, though.

Could this use a literal 0xe000, or __GENMASK(15, 13) from
<linux/bits.h>, instead?

[ ... ]

> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -1408,6 +1408,29 @@ int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable)
>  }
>  EXPORT_SYMBOL_GPL(genphy_c45_fast_retrain);
>  
> +/**
> + * genphy_c45_template_testmode - configure template testmode registers
> + * @phydev: target phy_device struct
> + * @test_mode: testmode includes Normal to Test mode 7
> + *
> + * Description: Set template testmode include Normal to Test mode 7
> + *
> + * Return: 0 on success, or a negative error code on failure (e.g. register
> + * read/write error).
> + */
> +int genphy_c45_template_testmode(struct phy_device *phydev, u16 test_mode)
> +{
> +	u16 ctrl;
> +
> +	if (test_mode > MDIO_PMA_10GBT_TESTMODE_7)
> +		return -EOPNOTSUPP;
> +
> +	ctrl = FIELD_PREP(MDIO_PMA_10GBT_TESTMODE_MASK, test_mode);
> +	return phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_10GBT_TESTMODE,
> +			      MDIO_PMA_10GBT_TESTMODE_MASK, ctrl);
> +}
> +EXPORT_SYMBOL_GPL(genphy_c45_template_testmode);
> +
>  /**
>   * genphy_c45_plca_get_cfg - get PLCA configuration from standard registers
>   * @phydev: target phy_device struct

[Severity: Low]
Should this return -EINVAL rather than -EOPNOTSUPP?

A test_mode above MDIO_PMA_10GBT_TESTMODE_7 cannot be encoded in the
3-bit MDIO_PMA_10GBT_TESTMODE_MASK field at all. That makes it a bad
argument from the caller of genphy_c45_template_testmode(), not a
feature the PHY lacks.

-EOPNOTSUPP reads as "this PHY does not support test modes". Suppose
a driver logs the error or passes it up towards userspace. The person
debugging it would then look at the hardware instead of at the caller
that passed the bad value.

Elsewhere in phylib, -EOPNOTSUPP is generally used for a missing
capability and -EINVAL for out-of-range arguments. Would -EINVAL fit
better here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924075048.4060414-1-kyle.switch%40motor-comm.com

[PATCH net-next v21 3/3] net: phy: Add driver for Motorcomm Quad 2.5GbE phy

[Kyle Switch]

Add support for Motorcomm YT8824 quad-port 2.5G PHY to the existing
motorcomm driver, using the phy_package helpers for the shared top
extended register space.

Signed-off-by: Kyle Switch <kyle.switch@motor-comm.com>
---
 drivers/net/phy/Kconfig     |    3 +-
 drivers/net/phy/motorcomm.c | 1585 ++++++++++++++++++++++++++++++++++-
 2 files changed, 1585 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index d3835597e379..996d75afed44 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -361,9 +361,10 @@ config MICROSEMI_PHY
 
 config MOTORCOMM_PHY
 	tristate "Motorcomm PHYs"
+	select PHY_PACKAGE
 	help
 	  Enables support for Motorcomm network PHYs.
-	  Currently supports YT85xx Gigabit Ethernet PHYs.
+	  Currently supports YT85xx Gigabit Ethernet PHYs and YT8824 4 * 2.5G PHY.
 
 config NATIONAL_PHY
 	tristate "National Semiconductor PHYs"
diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
index 90a4f86f2758..c89ffb00e55e 100644
--- a/drivers/net/phy/motorcomm.c
+++ b/drivers/net/phy/motorcomm.c
@@ -1,24 +1,31 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
- * Motorcomm 8511/8521/8522/8531/8531S/8821 PHY driver.
+ * Motorcomm 8511/8521/8522/8531/8531S/8821/8824 PHY driver.
  *
  * Author: Peter Geis <pgwipeout@gmail.com>
  * Author: Frank <Frank.Sae@motor-comm.com>
+ * Author: Kyle <kyle.switch@motor-comm.com>
  */
 
 #include <linux/clk.h>
 #include <linux/etherdevice.h>
 #include <linux/kernel.h>
+#include <linux/mdio.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_net.h>
 #include <linux/phy.h>
 #include <linux/property.h>
 
+#include "phylib.h"
+
 #define PHY_ID_YT8511		0x0000010a
 #define PHY_ID_YT8521		0x0000011a
 #define PHY_ID_YT8522		0x4f51e928
 #define PHY_ID_YT8531		0x4f51e91b
 #define PHY_ID_YT8531S		0x4f51e91a
 #define PHY_ID_YT8821		0x4f51ea19
+#define PHY_ID_YT8824		0x4f51e8b8
 /* YT8521/YT8531S/YT8821 Register Overview
  *	UTP Register space	|	FIBER Register space
  *  ------------------------------------------------------------
@@ -30,6 +37,18 @@
  *  ------------------------------------------------------------
  */
 
+/* YT8824 Register Overview
+ * UTP Register space	| SERDES Register space
+ *  ------------------------------------------------------------
+ * | UTP MII		| SERDES MII		|
+ * | UTP MMD		|			|
+ * | UTP Extended	| SERDES Extended	|
+ * | UTP Top Extended	| SERDES Top Extended	|
+ *  ------------------------------------------------------------
+ * |   Common Top Extended			|
+ *  ------------------------------------------------------------
+ */
+
 /* 0x10 ~ 0x15 , 0x1E and 0x1F are common MII registers of yt phy */
 
 /* Specific Function Control Register */
@@ -381,6 +400,13 @@
 #define YT8821_CHIP_MODE_AUTO_BX2500_SGMII	0
 #define YT8821_CHIP_MODE_FORCE_BX2500		1
 
+#define YT8824_RSSR_SPACE_MASK			BIT(0)
+#define YT8824_RSSR_SERDES_SPACE		(0x1)
+#define YT8824_RSSR_UTP_SPACE			(0x0)
+#define YT8824_SDS_CFG_MIN_PRE_MASK		GENMASK(3, 0)
+#define YT8824_SDS_EN_FILL_PRE			BIT(13)
+#define YT8824_SDS_TX_PRE_PADDING		(0x7)
+
 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
@@ -399,6 +425,12 @@ struct yt8521_priv {
 	u8 reg_page;
 };
 
+struct yt8824_shared_priv {
+	phy_interface_t interface_mode;
+	/* shared_lock used to UTPs operation isolation during swap reg space */
+	struct mutex shared_lock;
+};
+
 /**
  * ytphy_read_ext() - read a PHY's extended register
  * @phydev: a pointer to a &struct phy_device
@@ -437,6 +469,70 @@ static int ytphy_read_ext_with_lock(struct phy_device *phydev, u16 regnum)
 	return ret;
 }
 
+/**
+ * ytphy_read_top_ext() - read a PHY's top extended register for YT8824
+ * @phydev: a pointer to a &struct phy_device
+ * @regnum: register number to read
+ *
+ * Returns: the value of regnum reg or negative error code
+ */
+static int ytphy_read_top_ext(struct phy_device *phydev, u16 regnum)
+{
+	int ret;
+
+	lockdep_assert_held(&phydev->mdio.bus->mdio_lock);
+	ret = __phy_package_write(phydev, 0, YTPHY_PAGE_SELECT, regnum);
+	if (ret < 0)
+		return ret;
+
+	return __phy_package_read(phydev, 0, YTPHY_PAGE_DATA);
+}
+
+/**
+ * ytphy_write_top_ext() - write a PHY's top extended register for YT8824
+ * @phydev: a pointer to a &struct phy_device
+ * @regnum: register number to write
+ * @val: register val to write
+ *
+ * Returns: 0 or negative error code
+ */
+static int ytphy_write_top_ext(struct phy_device *phydev, u16 regnum,
+			       u16 val)
+{
+	int ret;
+
+	lockdep_assert_held(&phydev->mdio.bus->mdio_lock);
+	ret = __phy_package_write(phydev, 0, YTPHY_PAGE_SELECT, regnum);
+	if (ret < 0)
+		return ret;
+
+	return __phy_package_write(phydev, 0, YTPHY_PAGE_DATA, val);
+}
+
+/**
+ * phy8824_page_write_with_lock() - write page for YT8824
+ * @phydev: a pointer to a &struct phy_device
+ * @page: reg page(YT8824_RSSR_SERDES_SPACE/YT8824_RSSR_UTP_SPACE).
+ *
+ * Returns: 0 or negative error code
+ */
+static int phy8824_page_write_with_lock(struct phy_device *phydev, int page)
+{
+	int ret;
+
+	phy_lock_mdio_bus(phydev);
+	ret = ytphy_read_top_ext(phydev, YT8521_REG_SPACE_SELECT_REG);
+	if (ret < 0)
+		goto err;
+	ret &= ~YT8824_RSSR_SPACE_MASK;
+	ret |= (page & YT8824_RSSR_SPACE_MASK);
+	ret = ytphy_write_top_ext(phydev, YT8521_REG_SPACE_SELECT_REG, ret);
+
+err:
+	phy_unlock_mdio_bus(phydev);
+	return ret;
+}
+
 /**
  * ytphy_write_ext() - write a PHY's extended register
  * @phydev: a pointer to a &struct phy_device
@@ -633,6 +729,1036 @@ static int ytphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 	return phy_restore_page(phydev, old_page, ret);
 }
 
+/**
+ * yt8824_read_page() - read PHY8824 reg page
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: current reg space of yt8824 (YT8824_RSSR_SERDES_SPACE/
+ * YT8824_RSSR_UTP_SPACE) or negative errno code
+ */
+static int yt8824_read_page(struct phy_device *phydev)
+{
+	int old_page;
+
+	old_page = ytphy_read_top_ext(phydev, YT8521_REG_SPACE_SELECT_REG);
+	if (old_page < 0)
+		return old_page;
+
+	return old_page & YT8824_RSSR_SPACE_MASK;
+};
+
+/**
+ * yt8824_write_page() - write reg page
+ * @phydev: a pointer to a &struct phy_device
+ * @page: Reg page(YT8824_RSSR_SERDES_SPACE/YT8824_RSSR_UTP_SPACE) to write.
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_write_page(struct phy_device *phydev, int page)
+{
+	int old_page;
+	u16 data;
+
+	old_page = ytphy_read_top_ext(phydev, YT8521_REG_SPACE_SELECT_REG);
+	if (old_page < 0)
+		return old_page;
+	data = old_page & (~YT8824_RSSR_SPACE_MASK);
+	data |= page;
+
+	return ytphy_write_top_ext(phydev, YT8521_REG_SPACE_SELECT_REG, data);
+};
+
+/**
+ * yt8824_utp_set_template_test_mode() - config YT8824 UTP test mode.
+ * @phydev: a pointer to a &struct phy_device
+ * @test_mode: template test mode from normal, testmode1 to testmode7
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_utp_set_template_test_mode(struct phy_device *phydev,
+					     u16 test_mode)
+{
+	int ret;
+
+	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
+	if (ret < 0)
+		return ret;
+
+	return genphy_c45_template_testmode(phydev, test_mode);
+}
+
+/**
+ * yt8824_sds_isolate_paged() - enable YT8824 serdes isolate.
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_sds_isolate_paged(struct phy_device *phydev)
+{
+	int old_page = YT8824_RSSR_UTP_SPACE;
+	int ret = 0;
+
+	old_page = phy_select_page(phydev, YT8824_RSSR_SERDES_SPACE);
+	if (old_page < 0)
+		goto err_restore_page;
+
+	/* enable sds isolate */
+	ret = __phy_modify(phydev, MII_BMCR, BMCR_ISOLATE, BMCR_ISOLATE);
+
+err_restore_page:
+	/* restore page, release the lock */
+	return phy_restore_page(phydev, old_page, ret);
+}
+
+/**
+ * yt8824_utp_softreset_paged() - config YT8824 UTP softreset.
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_utp_softreset_paged(struct phy_device *phydev)
+{
+	int ret = 0;
+	int val;
+
+	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
+	if (ret < 0)
+		return ret;
+	ret = phy_modify(phydev, MII_BMCR, BMCR_RESET, BMCR_RESET);
+	if (ret < 0)
+		return ret;
+	/* wait until softreset done. */
+	return phy_read_poll_timeout(phydev, MII_BMCR, val, !(val & BMCR_RESET),
+				     50000, 600000, true);
+}
+
+/**
+ * yt8824_sds_isolate_and_softreset_paged() - disable YT8824 serdes isolate
+ * and sds softreset.
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_sds_isolate_and_softreset_paged(struct phy_device *phydev)
+{
+	int old_page = YT8824_RSSR_UTP_SPACE;
+	int val = 0;
+	int ret = -1;
+
+	old_page = phy_select_page(phydev, YT8824_RSSR_SERDES_SPACE);
+	if (old_page < 0)
+		goto err_restore_page;
+
+	/* sds softreset and disable isolate */
+	ret = __phy_modify(phydev, MII_BMCR, BMCR_RESET | BMCR_ISOLATE,
+			   BMCR_RESET & ~BMCR_ISOLATE);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* poll while still holding the lock */
+	ret = read_poll_timeout(__phy_read, val,
+				(val < 0) || !(val & BMCR_RESET), 50000, 600000,
+				true, phydev, MII_BMCR);
+	if (val < 0)
+		ret = val;
+
+err_restore_page:
+	/* restore page, release the lock */
+	return phy_restore_page(phydev, old_page, ret);
+}
+
+/**
+ * yt8824_restore_working_status() - called to do store working status
+ * @phydev: a pointer to a &struct phy_device
+ * @ret: operation's return code
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_restore_working_status(struct phy_device *phydev, int ret)
+{
+	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
+	int r;
+
+	/* configure normal test mode */
+	r = yt8824_utp_set_template_test_mode
+		(phydev, MDIO_PMA_10GBT_TESTMODE_NORMAL);
+	if (ret >= 0 && r < 0)
+		ret = r;
+	if (priv->interface_mode != PHY_INTERFACE_MODE_INTERNAL) {
+		/* sds soft reset and disable isolation */
+		r = yt8824_sds_isolate_and_softreset_paged(phydev);
+		if (ret >= 0 && r < 0)
+			ret = r;
+	}
+
+	return ret;
+}
+
+/**
+ * yt8824_soft_reset() - called to do PHY software reset
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_soft_reset(struct phy_device *phydev)
+{
+	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
+	int ret;
+
+	mutex_lock(&priv->shared_lock);
+	if (priv->interface_mode == PHY_INTERFACE_MODE_INTERNAL) {
+		/* test mode 1 */
+		ret = yt8824_utp_set_template_test_mode
+			(phydev, MDIO_PMA_10GBT_TESTMODE_1);
+		if (ret < 0)
+			goto retry;
+		ret = yt8824_utp_softreset_paged(phydev);
+		if (ret < 0)
+			goto retry;
+		/* normal mode */
+		ret = yt8824_utp_set_template_test_mode
+			(phydev, MDIO_PMA_10GBT_TESTMODE_NORMAL);
+		if (ret < 0)
+			goto retry;
+	} else {
+		/* test mode 1 */
+		ret = yt8824_utp_set_template_test_mode
+			(phydev, MDIO_PMA_10GBT_TESTMODE_1);
+		if (ret < 0)
+			goto retry;
+
+		/* sds isolation */
+		ret = yt8824_sds_isolate_paged(phydev);
+		if (ret < 0)
+			goto retry;
+
+		/* utp soft reset */
+		ret = yt8824_utp_softreset_paged(phydev);
+		if (ret < 0)
+			goto retry;
+
+		/* normal mode */
+		ret = yt8824_utp_set_template_test_mode
+			(phydev, MDIO_PMA_10GBT_TESTMODE_NORMAL);
+		if (ret < 0)
+			goto retry;
+
+		/* sds soft reset and disable isolation */
+		ret = yt8824_sds_isolate_and_softreset_paged(phydev);
+		if (ret < 0)
+			goto retry;
+	}
+	mutex_unlock(&priv->shared_lock);
+	return ret;
+retry:
+	ret = yt8824_restore_working_status(phydev, ret);
+	mutex_unlock(&priv->shared_lock);
+
+	return ret;
+}
+
+/**
+ * yt8824_extern_config_utp_init_paged() - config external phy8824 utp init
+ * @phydev: target phy_device struct
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_extern_config_utp_init_paged(struct phy_device *phydev)
+{
+	int ret = 0;
+	int val = 0;
+	int r;
+
+	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
+	if (ret < 0)
+		return ret;
+	/* power down */
+	ret = phy_modify(phydev, MII_BMCR, BMCR_PDOWN, BMCR_PDOWN);
+	if (ret < 0)
+		goto err_restore;
+
+	/* pll calibration */
+	ret = ytphy_write_ext_with_lock(phydev, 0x0001, 0x0003);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0xa20e, 0x0cba);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0xa20a, 0xc3f1);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0xa20c, 0x1620);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0xa2b6, 0x0a00);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0xa2b6, 0x0e00);
+	if (ret < 0)
+		goto err_restore;
+
+	/* optimization utp */
+	ret = ytphy_write_ext_with_lock(phydev, 0x0001, 0x0003);
+	if (ret < 0)
+		goto err_restore;
+
+	/* enable nibble */
+	ret = ytphy_write_ext_with_lock(phydev, 0xa003, 0x0003);
+	if (ret < 0)
+		goto err_restore;
+
+	/* idle err detect enable */
+	ret = ytphy_write_ext_with_lock(phydev, 0x03d0, 0x5210);
+	if (ret < 0)
+		goto err_restore;
+
+	/* optimized 2.5G long cable performance */
+	ret = ytphy_write_ext_with_lock(phydev, 0x0372, 0x5038);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x037c, 0x6068);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x0388, 0x00a0);
+	if (ret < 0)
+		goto err_restore;
+
+	/* optimized fast retrain */
+	ret = ytphy_write_ext_with_lock(phydev, 0x0359, 0x2140);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x000c, 0xc1a0);
+	if (ret < 0)
+		goto err_restore;
+
+	/* 2.5G template tone */
+	ret = ytphy_write_ext_with_lock(phydev, 0xa2fa, 0x0083);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x04e2, 0x0149);
+	if (ret < 0)
+		goto err_restore;
+
+	/* optimized 2.5G template */
+	ret = ytphy_write_ext_with_lock(phydev, 0x047e, 0x3939);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x047f, 0x3939);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x0480, 0x3939);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x0481, 0x3939);
+	if (ret < 0)
+		goto err_restore;
+
+	/* optimized 1000M cable length threshold */
+	ret = ytphy_write_ext_with_lock(phydev, 0x0336, 0xab0a);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x0340, 0x301d);
+	if (ret < 0)
+		goto err_restore;
+
+	/* 100M template amplitude */
+	ret = ytphy_write_ext_with_lock(phydev, 0x046e, 0x4545);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x046f, 0x4545);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x0470, 0x4545);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x0471, 0x4545);
+	if (ret < 0)
+		goto err_restore;
+
+	/* optimized 100M cable length threshold */
+	ret = ytphy_write_ext_with_lock(phydev, 0x030b, 0xaa1d);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x071f, 0x0036);
+	if (ret < 0)
+		goto err_restore;
+
+	/* 10M template amplitude */
+	ret = ytphy_write_ext_with_lock(phydev, 0x046b, 0x1818);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x046c, 0x1818);
+	if (ret < 0)
+		goto err_restore;
+
+	/* optimized 10M cable length threshold */
+	ret = ytphy_write_ext_with_lock(phydev, 0x0466, 0x6c6c);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x0467, 0x6c6c);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x0468, 0x6c6c);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x0469, 0x6c6c);
+	if (ret < 0)
+		goto err_restore;
+
+	/* optimize utp 1000M performance */
+	ret = ytphy_write_ext_with_lock(phydev, 0x034a, 0xff03);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x00f8, 0xb3ff);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x0059, 0x4040);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x032c, 0x5094);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x032d, 0xd094);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x032e, 0x5308);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x0322, 0x6440);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x04d3, 0x5220);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x04d2, 0x5220);
+	if (ret < 0)
+		goto err_restore;
+
+	/* optimized EMC CS */
+	ret = ytphy_write_ext_with_lock(phydev, 0x00c8, 0xffff);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x00be, 0x6406);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x037a, 0x40ff);
+	if (ret < 0)
+		goto err_restore;
+
+	/* optimized EMC RE */
+	ret = ytphy_write_ext_with_lock(phydev, 0x0482, 0xffff);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0xa2d5, 0x1f1f);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0xa2d6, 0x1f1f);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0xa2d7, 0x1f1f);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0xa2d8, 0x1f1f);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0xa218, 0x006e);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0xa01d, 0xfff0);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0xa01e, 0xfff0);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0xa01d, 0xffff);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0xa01e, 0xffff);
+	if (ret < 0)
+		goto err_restore;
+
+	ret = genphy_c45_template_testmode(phydev, MDIO_PMA_10GBT_TESTMODE_1);
+	if (ret < 0)
+		goto err_restore_normal;
+	/* reset */
+	ret = phy_modify(phydev, MII_BMCR, BMCR_RESET | BMCR_ANENABLE,
+			 BMCR_RESET | BMCR_ANENABLE);
+	if (ret < 0)
+		goto err_restore_normal;
+	ret = phy_read_poll_timeout(phydev, MII_BMCR, val, !(val & BMCR_RESET),
+				    50000, 600000, true);
+	if (ret < 0)
+		goto err_restore_normal;
+
+	ret = genphy_c45_template_testmode(phydev,
+					   MDIO_PMA_10GBT_TESTMODE_NORMAL);
+	if (ret < 0)
+		goto err_restore_normal;
+	return 0;
+
+err_restore:
+	r = phy_modify(phydev, MII_BMCR, BMCR_PDOWN, 0);
+	if (ret >= 0 && r < 0)
+		ret = r;
+	return ret;
+
+err_restore_normal:
+	r = genphy_c45_template_testmode(phydev,
+					 MDIO_PMA_10GBT_TESTMODE_NORMAL);
+	if (ret >= 0 && r < 0)
+		ret = r;
+	r = phy_modify(phydev, MII_BMCR, BMCR_PDOWN, 0);
+	if (ret >= 0 && r < 0)
+		ret = r;
+	return ret;
+}
+
+/**
+ * yt8824_extern_config_sds_init_paged() - config external phy8824 sds init
+ * @phydev: target phy_device struct
+ *
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_extern_config_sds_init_paged(struct phy_device *phydev)
+{
+	int old_page = YT8824_RSSR_UTP_SPACE;
+	int val_1, val_2, val_3, tmp;
+	int ret = -1;
+	int val;
+
+	old_page = phy_select_page(phydev, YT8824_RSSR_SERDES_SPACE);
+	if (old_page < 0)
+		goto err_restore_page;
+
+	/* read efuse */
+	ret = ytphy_read_top_ext(phydev, 0xa13e);
+	if (ret < 0)
+		goto err_restore_page;
+	else
+		val_1 = ret;
+
+	ret = ytphy_read_top_ext(phydev, 0xa13f);
+	if (ret < 0)
+		goto err_restore_page;
+	else
+		val_2 = ret;
+
+	ret = ytphy_read_top_ext(phydev, 0xa140);
+	if (ret < 0)
+		goto err_restore_page;
+	else
+		val_3 = ret;
+
+	/* Serdes optimization */
+	ret = ytphy_write_ext(phydev, 0x04be, 0x000d);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x049f, 0x7ded);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x04a9, 0x009f);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* analog CDR */
+	ret = ytphy_write_ext(phydev, 0x0406, 0x0800);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* optimized VCO */
+	ret = ytphy_write_ext(phydev, 0x0438, 0x9024);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x0439, 0x00c0);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* optimized PLL lock */
+	ret = ytphy_read_ext(phydev, 0x0429);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret &= ~(BIT(13) | BIT(12));
+	tmp = (val_1 & (BIT(7) | BIT(6))) >> 6;
+	ret |= (tmp << 12);
+	ret = ytphy_write_ext(phydev, 0x0429, ret);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_read_ext(phydev, 0x0441);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret &= ~(BIT(1) | BIT(0));
+	tmp = (val_1 & (BIT(5) | BIT(4))) >> 4;
+	ret |= tmp;
+	ret = ytphy_write_ext(phydev, 0x0441, ret);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_read_ext(phydev, 0x042b);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret &= ~(BIT(13) | BIT(12));
+	tmp = (val_3 & (BIT(1) | BIT(0)));
+	ret |= (tmp << 12);
+	ret = ytphy_write_ext(phydev, 0x042b, ret);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x043a, 0x1006);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x042a, 0xf070);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* cable length threshold */
+	ret = ytphy_write_ext(phydev, 0x0491, 0x007f);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x0492, 0x7f7f);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* Serdes training threshold */
+	ret = ytphy_write_ext(phydev, 0x0454, 0x0f14);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x0497, 0x0a44);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* digital eye diagram of SerDes */
+	ret = ytphy_write_ext(phydev, 0x04cd, 0x0000);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* Serdes LDO */
+	ret = ytphy_read_ext(phydev, 0x04b5);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret &= ~(BIT(6) | BIT(5) | BIT(4));
+	tmp = (val_2 & (BIT(4) | BIT(3) | BIT(2))) >> 2;
+	ret |= (tmp << 4);
+	ret = ytphy_write_ext(phydev, 0x04b5, ret);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_read_ext(phydev, 0x04b4);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret &= ~(BIT(10) | BIT(9) | BIT(8));
+	tmp = (val_2 & (BIT(7) | BIT(6) | BIT(5))) >> 5;
+	ret |= (tmp << 8);
+	ret = ytphy_write_ext(phydev, 0x04b4, ret);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* optimized Serdes RX */
+	ret = ytphy_write_ext(phydev, 0x04af, 0x45e3);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x048a, 0x0fff);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x0408, 0x7c00);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x04d6, 0x007f);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x044f, 0xff08);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* optimized Serdes TX */
+	ret = ytphy_write_ext(phydev, 0x048e, 0x7d00);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x000d, 0x0606);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* Serdes manual config */
+	ret = ytphy_write_ext(phydev, 0x04b0, 0x0804);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x04b1, 0x7074);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x04af, 0x45e7);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* restart calibration */
+	ret = ytphy_write_ext(phydev, 0x0003, 0x5603);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x0492, 0x7fff);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x0492, 0x7f7f);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x2000, 0x0040);
+	if (ret < 0)
+		goto err_restore_page;
+
+	ret = ytphy_write_ext(phydev, 0x2000, 0x0000);
+	if (ret < 0)
+		goto err_restore_page;
+
+	/* TX preamble padded to 8; RX IPG always > 8 */
+	ret = __phy_read(phydev, MII_RESV1);
+	if (ret < 0)
+		goto err_restore_page;
+	ret &= ~YT8824_SDS_CFG_MIN_PRE_MASK;
+	ret |= YT8824_SDS_TX_PRE_PADDING;
+	ret |= YT8824_SDS_EN_FILL_PRE;
+	ret = __phy_write(phydev, MII_RESV1, ret);
+	if (ret < 0)
+		goto err_restore_page;
+	/* reset serdes */
+	ret = __phy_modify(phydev, MII_BMCR, BMCR_RESET | BMCR_ANENABLE,
+			   BMCR_RESET | BMCR_ANENABLE);
+	if (ret < 0)
+		goto err_restore_page;
+	/* poll while still holding the lock; __phy_read takes no lock */
+	ret = read_poll_timeout(__phy_read, val,
+				(val < 0) || !(val & BMCR_RESET), 50000, 600000,
+				true, phydev, MII_BMCR);
+	if (val < 0)
+		ret = val;
+err_restore_page:
+	/* restore page, release the lock */
+	return phy_restore_page(phydev, old_page, ret);
+}
+
+/**
+ * yt8824_internal_config_init_paged() - config internal phy8824 init
+ * @phydev: target phy_device struct
+ *
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_internal_config_init_paged(struct phy_device *phydev)
+{
+	int ret = 0;
+	int val = 0;
+	int r = 0;
+
+	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
+	if (ret < 0)
+		return ret;
+
+	ret = ytphy_write_ext_with_lock(phydev, 0x1, 0x3);
+	if (ret < 0)
+		return ret;
+	/* power down */
+	ret = phy_modify(phydev, MII_BMCR, BMCR_PDOWN, BMCR_PDOWN);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa20e, 0xcba);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa20a, 0xc3f1);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa20c, 0x1620);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa2b6, 0xa00);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa2b6, 0xe00);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa003, 0x3);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x3d0, 0x5210);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x372, 0x5038);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x37c, 0x6068);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x388, 0xa0);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x359, 0x2140);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa2fa, 0x83);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x4e2, 0x149);
+	if (ret < 0)
+		goto err_restore;
+	/* 2.5G tempate */
+	ret = ytphy_write_ext_with_lock(phydev, 0x47e, 0x3939);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x47f, 0x3939);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x480, 0x3939);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x481, 0x3939);
+	if (ret < 0)
+		goto err_restore;
+	/* 1000 cable length threshold */
+	ret = ytphy_write_ext_with_lock(phydev, 0x336, 0xab0a);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x340, 0x301d);
+	if (ret < 0)
+		goto err_restore;
+	/* 1000 performance */
+	ret = ytphy_write_ext_with_lock(phydev, 0x34a, 0xff03);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xf8, 0xb3ff);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x32c, 0x5094);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x32d, 0xd094);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x32e, 0x5308);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x322, 0x6440);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x4d3, 0x5220);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x4d2, 0x5220);
+	if (ret < 0)
+		goto err_restore;
+	/* 100 tempate */
+	ret = ytphy_write_ext_with_lock(phydev, 0x46e, 0x4545);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x46f, 0x4545);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x470, 0x4545);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x471, 0x4545);
+	if (ret < 0)
+		goto err_restore;
+	/* 100 cable length threshold */
+	ret = ytphy_write_ext_with_lock(phydev, 0x30b, 0xaa1d);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x71f, 0x36);
+	if (ret < 0)
+		goto err_restore;
+	/* 10 tempate */
+	ret = ytphy_write_ext_with_lock(phydev, 0x46b, 0x1818);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x46c, 0x1818);
+	if (ret < 0)
+		goto err_restore;
+	/* 10 tempate MAU*/
+	ret = ytphy_write_ext_with_lock(phydev, 0x466, 0x6c6c);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x467, 0x6c6c);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x468, 0x6c6c);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x469, 0x6c6c);
+	if (ret < 0)
+		goto err_restore;
+	/* EMC CS, Inconsistent with external phy */
+	ret = ytphy_write_ext_with_lock(phydev, 0xc8, 0xfff);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xbe, 0x6406);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0x37a, 0x40ff);
+	if (ret < 0)
+		goto err_restore;
+	/* EMC RE*/
+	ret = ytphy_write_ext_with_lock(phydev, 0x482, 0xffff);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa2d5, 0x1f1f);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa2d6, 0x1f1f);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa2d7, 0x1f1f);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa2d8, 0x1f1f);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa218, 0x6e);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa01d, 0xfff0);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa01e, 0xfff0);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa01d, 0xffff);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xa01e, 0xffff);
+	if (ret < 0)
+		goto err_restore;
+	ret = ytphy_write_ext_with_lock(phydev, 0xc, 0x41a1);
+	if (ret < 0)
+		goto err_restore;
+	ret = genphy_c45_template_testmode(phydev, MDIO_PMA_10GBT_TESTMODE_1);
+	if (ret)
+		goto err_restore_normal;
+	/* reset */
+	ret = phy_modify(phydev, MII_BMCR, BMCR_RESET | BMCR_ANENABLE,
+			 BMCR_RESET | BMCR_ANENABLE);
+	if (ret < 0)
+		goto err_restore_normal;
+	ret = phy_read_poll_timeout(phydev, MII_BMCR, val, !(val & BMCR_RESET),
+				    50000, 600000, true);
+	if (ret < 0)
+		goto err_restore_normal;
+
+	ret = genphy_c45_template_testmode(phydev,
+					   MDIO_PMA_10GBT_TESTMODE_NORMAL);
+	if (ret < 0)
+		goto err_restore_normal;
+
+	return 0;
+
+err_restore:
+	r = phy_modify(phydev, MII_BMCR, BMCR_PDOWN, 0);
+	if (ret >= 0 && r < 0)
+		ret = r;
+	return ret;
+
+err_restore_normal:
+	r = genphy_c45_template_testmode(phydev,
+					 MDIO_PMA_10GBT_TESTMODE_NORMAL);
+	if (ret >= 0 && r < 0)
+		ret = r;
+	r = phy_modify(phydev, MII_BMCR, BMCR_PDOWN, 0);
+	if (ret >= 0 && r < 0)
+		ret = r;
+	return ret;
+}
+
+/**
+ * yt8824_config_init() - phy initializatioin
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_config_init(struct phy_device *phydev)
+{
+	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
+	int ret;
+
+	mutex_lock(&priv->shared_lock);
+	if (priv->interface_mode == PHY_INTERFACE_MODE_INTERNAL) {
+		ret = yt8824_internal_config_init_paged(phydev);
+		if (ret < 0)
+			goto err;
+	} else {
+		ret = yt8824_extern_config_sds_init_paged(phydev);
+		if (ret < 0)
+			goto err;
+		ret = yt8824_extern_config_utp_init_paged(phydev);
+		if (ret < 0)
+			goto err;
+	}
+	mutex_unlock(&priv->shared_lock);
+	ret = yt8824_soft_reset(phydev);
+
+	phydev_dbg(phydev, "%s done, phy addr: %d\n", __func__,
+		   phydev->mdio.addr);
+	return ret;
+err:
+	mutex_unlock(&priv->shared_lock);
+	return ret;
+}
+
 static int yt8531_set_wol(struct phy_device *phydev,
 			  struct ethtool_wolinfo *wol)
 {
@@ -3104,6 +4230,444 @@ static int yt8821_resume(struct phy_device *phydev)
 	return yt8821_modify_utp_fiber_bmcr(phydev, BMCR_PDOWN, 0);
 }
 
+/**
+ * yt8824_get_features - read mmd register to get 2.5G capability
+ * @phydev: target phy_device struct
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_get_features(struct phy_device *phydev)
+{
+	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
+	int ret;
+
+	mutex_lock(&priv->shared_lock);
+	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
+	if (ret < 0)
+		goto err;
+	ret = yt8821_get_features(phydev);
+
+err:
+	mutex_unlock(&priv->shared_lock);
+	return ret;
+}
+
+/**
+ * yt8824_aneg_done()  - check negotiation state.
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: auto-negotiation complete status or negative errno code
+ */
+static int yt8824_aneg_done(struct phy_device *phydev)
+{
+	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
+	int auto_neg;
+	int ret;
+
+	mutex_lock(&priv->shared_lock);
+	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
+	if (ret < 0)
+		goto err;
+
+	ret = phy_read(phydev, MII_BMSR);
+	if (ret < 0)
+		goto err;
+	mutex_unlock(&priv->shared_lock);
+	auto_neg = !!(ret & BMSR_ANEGCOMPLETE);
+
+	phydev_dbg(phydev, "%s, phy addr: %d, auto negotiation done: %d\n",
+		   __func__, phydev->mdio.addr, auto_neg);
+	return auto_neg;
+err:
+	mutex_unlock(&priv->shared_lock);
+	return ret;
+}
+
+/**
+ * yt8824_read_status_paged() -  determines the speed and duplex of one page
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_read_status_paged(struct phy_device *phydev)
+{
+	int ret;
+	int val;
+
+	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
+	if (ret < 0)
+		return ret;
+
+	ret = genphy_read_status(phydev);
+	if (ret < 0)
+		return ret;
+
+	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
+		ret = genphy_c45_read_lpa(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (!phydev->link) {
+		phydev->speed = SPEED_UNKNOWN;
+		phydev->duplex = DUPLEX_UNKNOWN;
+		if (phydev->autoneg == AUTONEG_ENABLE)
+			phy_resolve_aneg_pause(phydev);
+		return 0;
+	}
+
+	ret = phy_read(phydev, YTPHY_SPECIFIC_STATUS_REG);
+	if (ret < 0)
+		return ret;
+
+	val = ret;
+
+	yt8821_adjust_status(phydev, val);
+
+	if (phydev->autoneg == AUTONEG_ENABLE)
+		phy_resolve_aneg_pause(phydev);
+
+	return 0;
+}
+
+/**
+ * yt8824_read_status() -  determines the negotiated speed and duplex
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_read_status(struct phy_device *phydev)
+{
+	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
+	int ret;
+
+	mutex_lock(&priv->shared_lock);
+	ret = yt8824_read_status_paged(phydev);
+	mutex_unlock(&priv->shared_lock);
+
+	return ret;
+}
+
+/**
+ * yt8824_utp_power_on(): utp power on.
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_utp_power_on(struct phy_device *phydev)
+{
+	int ret = 0;
+
+	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
+	if (ret < 0)
+		return ret;
+
+	return phy_modify(phydev, MII_BMCR, BMCR_PDOWN | BMCR_ISOLATE, 0x0);
+}
+
+/**
+ * yt8824_utp_power_down(): utp power down.
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_utp_power_down(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
+	if (ret < 0)
+		return ret;
+
+	return phy_modify(phydev, MII_BMCR, BMCR_PDOWN, BMCR_PDOWN);
+}
+
+/**
+ * yt8824_power_on()  - set utp power on.
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * NOTE: need WA like softreset
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_power_on(struct phy_device *phydev)
+{
+	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
+	int ret;
+	int r;
+
+	if (priv->interface_mode == PHY_INTERFACE_MODE_INTERNAL) {
+		/* test mode 1 */
+		ret = yt8824_utp_set_template_test_mode
+			(phydev, MDIO_PMA_10GBT_TESTMODE_1);
+		if (ret < 0)
+			goto retry;
+		/* utp power on */
+		ret = yt8824_utp_power_on(phydev);
+		if (ret < 0)
+			goto retry;
+		/* normal mode */
+		ret = yt8824_utp_set_template_test_mode
+			(phydev, MDIO_PMA_10GBT_TESTMODE_NORMAL);
+		if (ret < 0)
+			goto retry;
+	} else {
+		/* test mode 1 */
+		ret = yt8824_utp_set_template_test_mode
+			(phydev, MDIO_PMA_10GBT_TESTMODE_1);
+		if (ret < 0)
+			goto retry;
+
+		/* sds isolation */
+		ret = yt8824_sds_isolate_paged(phydev);
+		if (ret < 0)
+			goto retry;
+
+		/* utp power on */
+		ret = yt8824_utp_power_on(phydev);
+		if (ret < 0)
+			goto retry;
+
+		/* normal mode */
+		ret = yt8824_utp_set_template_test_mode
+			(phydev, MDIO_PMA_10GBT_TESTMODE_NORMAL);
+		if (ret < 0)
+			goto retry;
+
+		/* sds soft reset and disable isolation */
+		ret = yt8824_sds_isolate_and_softreset_paged(phydev);
+		if (ret < 0)
+			goto retry;
+	}
+	return 0;
+
+retry:
+	/*
+	 * If the PHY up operation succeeds but the subsequent operation
+	 * fails, revert to the default state.
+	 */
+	r = yt8824_utp_power_down(phydev);
+	if (ret >= 0 && r < 0)
+		ret = r;
+	ret = yt8824_restore_working_status(phydev, ret);
+	return ret;
+}
+
+/**
+ * yt8824_resume() - resume the hardware
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_resume(struct phy_device *phydev)
+{
+	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
+	int ret;
+
+	mutex_lock(&priv->shared_lock);
+	ret = yt8824_power_on(phydev);
+	mutex_unlock(&priv->shared_lock);
+
+	return ret;
+}
+
+/**
+ * yt8824_power_down()  - set utp power down.
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * NOTE: need WA like softreset
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_power_down(struct phy_device *phydev)
+{
+	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
+	int ret;
+	int r;
+
+	if (priv->interface_mode == PHY_INTERFACE_MODE_INTERNAL) {
+		/* test mode 1 */
+		ret = yt8824_utp_set_template_test_mode
+			(phydev, MDIO_PMA_10GBT_TESTMODE_1);
+		if (ret < 0)
+			goto retry;
+		/* utp power down */
+		ret = yt8824_utp_power_down(phydev);
+		if (ret < 0)
+			goto retry;
+		/* normal mode */
+		ret = yt8824_utp_set_template_test_mode
+			(phydev, MDIO_PMA_10GBT_TESTMODE_NORMAL);
+		if (ret < 0)
+			goto retry;
+	} else {
+		/* test mode 1 */
+		ret = yt8824_utp_set_template_test_mode
+			(phydev, MDIO_PMA_10GBT_TESTMODE_1);
+		if (ret < 0)
+			goto retry;
+
+		/* sds isolation */
+		ret = yt8824_sds_isolate_paged(phydev);
+		if (ret < 0)
+			goto retry;
+
+		/* utp power down */
+		ret = yt8824_utp_power_down(phydev);
+		if (ret < 0)
+			goto retry;
+
+		/* normal mode */
+		ret = yt8824_utp_set_template_test_mode
+			(phydev, MDIO_PMA_10GBT_TESTMODE_NORMAL);
+		if (ret < 0)
+			goto retry;
+
+		/* sds soft reset and disable isolation */
+		ret = yt8824_sds_isolate_and_softreset_paged(phydev);
+		if (ret < 0)
+			goto retry;
+	}
+	return 0;
+
+retry:
+	/*
+	 * If the PHY down operation succeeds but the subsequent operation
+	 * fails, revert to the default state.
+	 */
+	r = yt8824_utp_power_on(phydev);
+	if (ret >= 0 && r < 0)
+		ret = r;
+	ret = yt8824_restore_working_status(phydev, ret);
+	return ret;
+}
+
+/**
+ * yt8824_suspend() - suspend the hardware
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_suspend(struct phy_device *phydev)
+{
+	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
+	int ret;
+
+	mutex_lock(&priv->shared_lock);
+	ret = yt8824_power_down(phydev);
+	mutex_unlock(&priv->shared_lock);
+
+	return ret;
+}
+
+/**
+ * yt8824_config_aneg() - config negotiation
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_config_aneg(struct phy_device *phydev)
+{
+	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
+	int phy_ctrl = 0;
+	int ret;
+
+	mutex_lock(&priv->shared_lock);
+	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
+	if (ret < 0)
+		goto err;
+
+	/*
+	 * Only advertise 2.5G when autoneg is enabled, or when 2.5G is
+	 * explicitly forced.  When a different speed is forced, clear
+	 * ADV2_5G so a 2.5G-capable link partner cannot negotiate 2.5G.
+	 * __genphy_config_aneg() only rewrites the
+	 * clause 22 registers on the forced-speed path, so it will not
+	 * clear this bit.
+	 */
+	if ((phydev->autoneg == AUTONEG_ENABLE ||
+	     phydev->speed == SPEED_2500) &&
+	    linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
+			      phydev->advertising))
+		phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;
+
+	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
+				     MDIO_AN_10GBT_CTRL_ADV2_5G, phy_ctrl);
+	if (ret < 0)
+		goto err;
+
+	ret = __genphy_config_aneg(phydev, ret);
+
+err:
+	mutex_unlock(&priv->shared_lock);
+	return ret;
+}
+
+/**
+ * yt8824_phy_package_probe_once()  - init phy package for phy8824.
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_phy_package_probe_once(struct phy_device *phydev)
+{
+	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
+	struct device_node *np = phy_package_get_node(phydev);
+	int res;
+
+	if (!priv || !np) {
+		phydev_err(phydev, "%s: missing shared priv or DT node\n",
+			   __func__);
+		return -EINVAL;
+	}
+	/* Initialise shared lock for YT8824 */
+	mutex_init(&priv->shared_lock);
+	res = of_get_phy_mode(np, &priv->interface_mode);
+	if (res < 0) {
+		phydev_err(phydev, "missing phy-mode in PHY package node.\n");
+		return res;
+	}
+
+	if (priv->interface_mode != PHY_INTERFACE_MODE_INTERNAL &&
+	    priv->interface_mode != PHY_INTERFACE_MODE_10G_QXGMII) {
+		phydev_err(phydev, "unsupported phy-mode %s\n",
+			   phy_modes(priv->interface_mode));
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/**
+ * yt8824_probe() - phy8824 probe.
+ * @phydev: a pointer to a &struct phy_device
+ *
+ * Returns: 0 or negative errno code
+ */
+static int yt8824_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct yt8824_shared_priv *shared_priv;
+	int ret;
+
+	ret = devm_of_phy_package_join(dev, phydev, sizeof(*shared_priv));
+	if (ret)
+		return ret;
+
+	/* Ensure other PHY probes wait for shared mutex initialization. */
+	phy_package_lock(phydev);
+	if (phy_package_probe_once(phydev)) {
+		ret = yt8824_phy_package_probe_once(phydev);
+		if (ret) {
+			phy_package_unlock(phydev);
+			return ret;
+		}
+	}
+	phy_package_unlock(phydev);
+
+	return 0;
+}
+
 static struct phy_driver motorcomm_phy_drvs[] = {
 	{
 		PHY_ID_MATCH_EXACT(PHY_ID_YT8511),
@@ -3190,13 +4754,29 @@ static struct phy_driver motorcomm_phy_drvs[] = {
 		.suspend		= yt8821_suspend,
 		.resume			= yt8821_resume,
 	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_YT8824),
+		.name = "YT8824 Quad Ports 2.5Gbps Ethernet",
+		.get_features = yt8824_get_features,
+		.read_page = yt8824_read_page,
+		.write_page = yt8824_write_page,
+		.probe = yt8824_probe,
+		.config_aneg = yt8824_config_aneg,
+		.aneg_done = yt8824_aneg_done,
+		.config_init = yt8824_config_init,
+		.read_status = yt8824_read_status,
+		.soft_reset = yt8824_soft_reset,
+		.suspend = yt8824_suspend,
+		.resume = yt8824_resume,
+	},
 };
 
 module_phy_driver(motorcomm_phy_drvs);
 
-MODULE_DESCRIPTION("Motorcomm 8511/8521/8531/8531S/8821 PHY driver");
+MODULE_DESCRIPTION("Motorcomm 8511/8521/8531/8531S/8821/8824 PHY driver");
 MODULE_AUTHOR("Peter Geis");
 MODULE_AUTHOR("Frank");
+MODULE_AUTHOR("Kyle");
 MODULE_LICENSE("GPL");
 
 static const struct mdio_device_id __maybe_unused motorcomm_tbl[] = {
@@ -3206,6 +4786,7 @@ static const struct mdio_device_id __maybe_unused motorcomm_tbl[] = {
 	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8531) },
 	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8531S) },
 	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8821) },
+	{ PHY_ID_MATCH_EXACT(PHY_ID_YT8824) },
 	{ /* sentinel */ }
 };
 
-- 
2.25.1

Re: [PATCH net-next v21 3/3] net: phy: Add driver for Motorcomm Quad 2.5GbE phy

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 4 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] yt8824_read_status_paged() clears the forced speed and duplex
  whenever the link is down, even when autoneg is disabled: `if…
- [Medium] yt8824_probe() takes the package once-bit with
  phy_package_probe_once() (test_and_set_bit, never cleared) before the…
- [Medium] yt8824_sds_isolate_and_softreset_paged() and
  yt8824_extern_config_sds_init_paged() hold the bus-wide…
- [Medium] yt8824_read_status_paged() calls genphy_c45_read_lpa() only
  when `autoneg == AUTONEG_ENABLE && autoneg_complete`.
- [Low] yt8824_config_aneg() has a 'forced 2.5G' condition,
  `(phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_2500) &&…
- [Low] The commit message is one sentence and leaves out behaviour a
  maintainer needs to judge this ~1.5k-line addition.
- [Low] yt8824_power_on() and yt8824_power_down() say UTP power-state
  changes 'need WA like softreset' and wrap the BMCR_PDOWN toggle in…

--

net: phy: Add driver for Motorcomm Quad 2.5GbE phy

This adds support for the Motorcomm YT8824 quad-port 2.5G PHY to the
motorcomm driver. It uses the phy_package helpers to reach the shared top
extended register space. It picks the internal or 10G-QXGMII init sequence
based on a phy-mode property in the package node.

> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index 90a4f86f2758..c89ffb00e55e 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c

[ ... ]

> @@ -614,23 +710,1053 @@ static int ytphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)

[ ... ]

> +static int yt8824_sds_isolate_and_softreset_paged(struct phy_device *phydev)
> +{
> +	int old_page = YT8824_RSSR_UTP_SPACE;
> +	int val = 0;
> +	int ret = -1;
> +
> +	old_page = phy_select_page(phydev, YT8824_RSSR_SERDES_SPACE);
> +	if (old_page < 0)
> +		goto err_restore_page;
> +
> +	/* sds softreset and disable isolate */
> +	ret = __phy_modify(phydev, MII_BMCR, BMCR_RESET | BMCR_ISOLATE,
> +			   BMCR_RESET & ~BMCR_ISOLATE);
> +	if (ret < 0)
> +		goto err_restore_page;
> +
> +	/* poll while still holding the lock */
> +	ret = read_poll_timeout(__phy_read, val,
> +				(val < 0) || !(val & BMCR_RESET), 50000, 600000,
> +				true, phydev, MII_BMCR);

[Severity: Medium]
Does this hold the bus-wide mdio_lock across a sleeping poll that can last
up to 600ms?

phy_select_page() takes phydev->mdio.bus->mdio_lock, and the lock is only
released in phy_restore_page(). Because sleep_before_read is true,
read_poll_timeout() sleeps before the first read. It then keeps polling
with 50ms sleeps for up to 600ms.

In 10G-QXGMII mode this runs on every soft_reset, config_init, suspend and
resume. It runs again from yt8824_restore_working_status() on error paths.

yt8824_extern_config_sds_init_paged() does the same thing. It first issues
about 50 extended register writes, all inside the same phy_select_page()
window:

	/* poll while still holding the lock; __phy_read takes no lock */
	ret = read_poll_timeout(__phy_read, val,
				(val < 0) || !(val & BMCR_RESET), 50000, 600000,
				true, phydev, MII_BMCR);

While the lock is held, the other three YT8824 ports and any other PHY or
switch on the same MDIO bus are blocked.

Is there a way to avoid holding mdio_lock while waiting for the SerDes
reset to finish?

> +	if (val < 0)
> +		ret = val;
> +
> +err_restore_page:
> +	/* restore page, release the lock */
> +	return phy_restore_page(phydev, old_page, ret);
> +}

[ ... ]

> +static int yt8824_soft_reset(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int ret;
> +
> +	mutex_lock(&priv->shared_lock);
> +	if (priv->interface_mode == PHY_INTERFACE_MODE_INTERNAL) {
> +		/* test mode 1 */
> +		ret = yt8824_utp_set_template_test_mode
> +			(phydev, MDIO_PMA_10GBT_TESTMODE_1);

[Severity: Low]
This isn't a bug, but could the commit message explain the sequencing
here? The commit message is a single sentence:

  Add support for Motorcomm YT8824 quad-port 2.5G PHY to the existing
  motorcomm driver, using the phy_package helpers for the shared top
  extended register space.

Several things aren't mentioned.

soft_reset, config_init, suspend and resume all put the PMA into 10GBASE-T
template test mode 1 around the operation and then switch back to normal.
The only rationale in the code is "NOTE: need WA like softreset" above
yt8824_power_on() and yt8824_power_down().

The earlier commit "net: phy: Add support for Template Control register
for PMA" describes test modes as being used for PHY validation, not as a
reset workaround.

There are two init flavours, internal and 10g-qxgmii. A phy-mode property
in the package node selects between them.

The PHY only binds inside an ethernet-phy-package DT node that has reg and
phy-mode. Otherwise probe fails.

phy_init_hw() calls .soft_reset (yt8824_soft_reset()) and then
.config_init. yt8824_config_init() ends with another call:

	mutex_unlock(&priv->shared_lock);
	ret = yt8824_soft_reset(phydev);

So the full reset sequence runs twice per phy_init_hw(). Is that intended?

[ ... ]

> +static int yt8824_extern_config_utp_init_paged(struct phy_device *phydev)
> +{
> +	int ret = 0;
> +	int val = 0;
> +	int r;
> +
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
> +	if (ret < 0)
> +		return ret;
> +	/* power down */
> +	ret = phy_modify(phydev, MII_BMCR, BMCR_PDOWN, BMCR_PDOWN);
> +	if (ret < 0)
> +		goto err_restore;

[Severity: Low]
Should the BMCR_PDOWN toggles here use the same sequence as
yt8824_power_on() and yt8824_power_down()?

Those functions say BMCR_PDOWN changes "need WA like softreset". They wrap
the change in template test mode 1 and, in external mode, SerDes
isolation.

Here the init code differs in three ways:

- BMCR_PDOWN is set in normal test mode, without SerDes isolation.
- The later BMCR_RESET runs in test mode 1, but without isolation.
- The err_restore paths clear BMCR_PDOWN with a plain phy_modify().

yt8824_internal_config_init_paged() also sets BMCR_PDOWN outside test
mode 1.

On the success path, yt8824_config_init() ends with yt8824_soft_reset(),
which runs the full workaround, so this may not matter in practice.

[ ... ]

> @@ -3104,6 +4230,444 @@ static int yt8821_resume(struct phy_device *phydev)

[ ... ]

> +static int yt8824_read_status_paged(struct phy_device *phydev)
> +{
> +	int ret;
> +	int val;
> +
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
> +		ret = genphy_c45_read_lpa(phydev);
> +		if (ret < 0)
> +			return ret;
> +	}

[Severity: Medium]
Can this leave a stale 2500baseT_Full bit in phydev->lp_advertising?

After a link with a 2.5G-capable partner, lp_advertising has
2500baseT_Full set. When the link drops or negotiation restarts,
autoneg_complete is cleared. genphy_read_lpa() then clears only the
Clause 22 bits:

drivers/net/phy/phy_device.c:genphy_read_lpa() {
    ...
		if (!phydev->autoneg_complete) {
			mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
							0);
			mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
			return 0;
		}
    ...
}

genphy_c45_read_lpa() is skipped here, so nothing clears the 10GBT status
bits. Until negotiation completes again, ethtool keeps reporting that the
link partner supports 2.5G.

The same pattern exists in yt8821_read_status().

> +
> +	if (!phydev->link) {
> +		phydev->speed = SPEED_UNKNOWN;
> +		phydev->duplex = DUPLEX_UNKNOWN;
> +		if (phydev->autoneg == AUTONEG_ENABLE)
> +			phy_resolve_aneg_pause(phydev);
> +		return 0;
> +	}

[Severity: Medium]
Does this overwrite a forced speed and duplex when autoneg is disabled?

With autoneg off, phydev->speed and phydev->duplex hold the user's forced
settings. genphy_read_status()->genphy_read_status_fixed() has just read
them back from BMCR.

Suppose the user forces 100/full and then unplugs the cable. This branch
sets speed and duplex to SPEED_UNKNOWN and DUPLEX_UNKNOWN. On the next
phy_start(), the state machine goes through _phy_start_aneg():

	if (AUTONEG_DISABLE == phydev->autoneg)
		phy_sanitize_settings(phydev);

phy_caps_lookup() finds no match for SPEED_UNKNOWN and returns the lowest
supported mode. yt8824_config_aneg()->__genphy_config_aneg()->
genphy_setup_forced() then programs 10Mb/s instead of 100/full.

phy_state_machine() only clears speed and duplex in PHY_HALTED when
phydev->autoneg == AUTONEG_ENABLE. Should this branch do the same?

[ ... ]

> +static int yt8824_config_aneg(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int phy_ctrl = 0;
> +	int ret;
> +
> +	mutex_lock(&priv->shared_lock);
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
> +	if (ret < 0)
> +		goto err;
> +
> +	/*
> +	 * Only advertise 2.5G when autoneg is enabled, or when 2.5G is
> +	 * explicitly forced.  When a different speed is forced, clear
> +	 * ADV2_5G so a 2.5G-capable link partner cannot negotiate 2.5G.
> +	 * __genphy_config_aneg() only rewrites the
> +	 * clause 22 registers on the forced-speed path, so it will not
> +	 * clear this bit.
> +	 */
> +	if ((phydev->autoneg == AUTONEG_ENABLE ||
> +	     phydev->speed == SPEED_2500) &&
> +	    linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +			      phydev->advertising))
> +		phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;

[Severity: Low]
Can phydev->speed == SPEED_2500 ever be true here with autoneg disabled?

phy_ethtool_ksettings_set() rejects AUTONEG_DISABLE with any speed other
than 10, 100 or 1000:

	if (autoneg == AUTONEG_DISABLE &&
	    ((speed != SPEED_1000 &&
	      speed != SPEED_100 &&
	      speed != SPEED_10) ||

With autoneg enabled, the first operand already decides the result. So
the SPEED_2500 check looks like dead code, and the comment describes a
forced 2.5G case that the core never produces.

Could the condition and the comment be simplified?

[ ... ]

> +static int yt8824_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct yt8824_shared_priv *shared_priv;
> +	int ret;
> +
> +	ret = devm_of_phy_package_join(dev, phydev, sizeof(*shared_priv));
> +	if (ret)
> +		return ret;
> +
> +	/* Ensure other PHY probes wait for shared mutex initialization. */
> +	phy_package_lock(phydev);
> +	if (phy_package_probe_once(phydev)) {
> +		ret = yt8824_phy_package_probe_once(phydev);
> +		if (ret) {
> +			phy_package_unlock(phydev);
> +			return ret;
> +		}
> +	}
> +	phy_package_unlock(phydev);
> +
> +	return 0;
> +}

[Severity: Medium]
What happens to the other PHYs in the package when
yt8824_phy_package_probe_once() fails?

phy_package_probe_once() does a test_and_set_bit() on the shared flags,
and the bit is never cleared. So the once-bit is used up before the
phy-mode validation runs.

If phy-mode is missing, of_get_phy_mode() leaves priv->interface_mode as
PHY_INTERFACE_MODE_NA. If phy-mode is unsupported, the rejected mode stays
in priv.

A sibling PHY may already have joined the package, for example with async
probing. It waits on phy_package_lock() and then sees
phy_package_probe_once() return false. It skips validation and returns 0.

That PHY then binds with an NA or rejected interface_mode. Every YT8824
op treats anything other than PHY_INTERFACE_MODE_INTERNAL as the external
SerDes case. So config_init, soft_reset, suspend and resume run the
SerDes sequences on a package that the driver itself rejected.

Could the validation result be recorded in the shared priv, or phy-mode
be validated on every probe, so all PHYs in the package fail the same
way?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924075048.4060414-1-kyle.switch%40motor-comm.com