[RFC PATCH v2 0/2] net::phy support for C45

[RFC PATCH v2 0/2] net::phy support for C45

[FUJITA Tomonori]

Adds support for reading/writing C45 registers and genphy helper
functions executed via C45 registers.

We could add methods for C45 like read/write_c45 and
genphy_c45_read_status. Instead, this implements the API for C22 and
C45 with trait in an unified way.

v2:
- add genphy helper support and split the patch
- create a 'reg' module
- define C22 according to the specs
- handle C22 vendor specific registers
- add const for C45 MMD
v1: https://lore.kernel.org/all/20240530.145258.456915814420167538.fujita.tomonori@gmail.com/T/


FUJITA Tomonori (2):
  rust: net::phy support for C45 register access
  rust: net::phy support for C45 genphy helpers

 drivers/net/phy/ax88796b_rust.rs |   7 +-
 rust/kernel/net/phy.rs           | 241 ++++++++++++++++++++++++++-----
 rust/uapi/uapi_helper.h          |   1 +
 3 files changed, 212 insertions(+), 37 deletions(-)


base-commit: e19de2064fdf6f1f2488e36cf3927c3f1d01803c
-- 
2.34.1

[RFC PATCH v2 1/2] rust: net::phy support for C45 register access

[FUJITA Tomonori]

Adds support for C45 register access. Instead of adding read/write_c45
methods, this unifies the API to access C22 and C45 registers with
trait.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 drivers/net/phy/ax88796b_rust.rs |   7 +-
 rust/kernel/net/phy.rs           | 192 +++++++++++++++++++++++++++----
 rust/uapi/uapi_helper.h          |   1 +
 3 files changed, 173 insertions(+), 27 deletions(-)

diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
index 5c92572962dc..8c7eb009d9fc 100644
--- a/drivers/net/phy/ax88796b_rust.rs
+++ b/drivers/net/phy/ax88796b_rust.rs
@@ -6,7 +6,7 @@
 //! C version of this driver: [`drivers/net/phy/ax88796b.c`](./ax88796b.c)
 use kernel::{
     c_str,
-    net::phy::{self, DeviceId, Driver},
+    net::phy::{self, reg::C22, DeviceId, Driver},
     prelude::*,
     uapi,
 };
@@ -24,7 +24,6 @@
     license: "GPL",
 }
 
-const MII_BMCR: u16 = uapi::MII_BMCR as u16;
 const BMCR_SPEED100: u16 = uapi::BMCR_SPEED100 as u16;
 const BMCR_FULLDPLX: u16 = uapi::BMCR_FULLDPLX as u16;
 
@@ -33,7 +32,7 @@
 // Toggle BMCR_RESET bit off to accommodate broken AX8796B PHY implementation
 // such as used on the Individual Computers' X-Surf 100 Zorro card.
 fn asix_soft_reset(dev: &mut phy::Device) -> Result {
-    dev.write(uapi::MII_BMCR as u16, 0)?;
+    dev.write(C22::BMCR, 0)?;
     dev.genphy_soft_reset()
 }
 
@@ -55,7 +54,7 @@ fn read_status(dev: &mut phy::Device) -> Result<u16> {
         }
         // If MII_LPA is 0, phy_resolve_aneg_linkmode() will fail to resolve
         // linkmode so use MII_BMCR as default values.
-        let ret = dev.read(MII_BMCR)?;
+        let ret = dev.read(C22::BMCR)?;
 
         if ret & BMCR_SPEED100 != 0 {
             dev.set_speed(uapi::SPEED_100);
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index fd40b703d224..286fd5a7ec91 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -175,32 +175,15 @@ pub fn set_duplex(&mut self, mode: DuplexMode) {
         unsafe { (*phydev).duplex = v };
     }
 
-    /// Reads a given C22 PHY register.
+    /// Reads a PHY register.
     // This function reads a hardware register and updates the stats so takes `&mut self`.
-    pub fn read(&mut self, regnum: u16) -> Result<u16> {
-        let phydev = self.0.get();
-        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
-        // So it's just an FFI call, open code of `phy_read()` with a valid `phy_device` pointer
-        // `phydev`.
-        let ret = unsafe {
-            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
-        };
-        if ret < 0 {
-            Err(Error::from_errno(ret))
-        } else {
-            Ok(ret as u16)
-        }
+    pub fn read<R: reg::Access>(&mut self, reg: R) -> Result<u16> {
+        reg.read(self)
     }
 
-    /// Writes a given C22 PHY register.
-    pub fn write(&mut self, regnum: u16, val: u16) -> Result {
-        let phydev = self.0.get();
-        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
-        // So it's just an FFI call, open code of `phy_write()` with a valid `phy_device` pointer
-        // `phydev`.
-        to_result(unsafe {
-            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
-        })
+    /// Writes a PHY register.
+    pub fn write<R: reg::Access>(&mut self, reg: R, val: u16) -> Result {
+        reg.write(self, val)
     }
 
     /// Reads a paged register.
@@ -302,6 +285,169 @@ pub fn genphy_read_abilities(&mut self) -> Result {
     }
 }
 
+/// Defines PHY registers.
+pub mod reg {
+    use super::Device;
+    use crate::build_assert;
+    use crate::error::*;
+    use crate::uapi;
+
+    /// Reads and writes PHY registers.
+    ///
+    /// This trait is used to implement multiple ways to read and write
+    /// PHY registers.
+    pub trait Access {
+        /// Reads a PHY register.
+        fn read(&self, dev: &mut Device) -> Result<u16>;
+
+        /// Writes a PHY register.
+        fn write(&self, dev: &mut Device, val: u16) -> Result;
+    }
+
+    /// C22 registers.
+    pub struct C22(u8);
+
+    impl C22 {
+        /// Basic mode control.
+        pub const BMCR: Self = C22(0x00);
+        /// Basic mode status.
+        pub const BMSR: Self = C22(0x01);
+        /// PHY identifier 1.
+        pub const PHYSID1: Self = C22(0x02);
+        /// PHY identifier 2.
+        pub const PHYSID2: Self = C22(0x03);
+        /// Auto-negotiation advertisement.
+        pub const ADVERTISE: Self = C22(0x04);
+        /// Auto-negotiation link partner base page ability.
+        pub const LPA: Self = C22(0x05);
+        /// Auto-negotiation expansion.
+        pub const EXPANSION: Self = C22(0x06);
+        /// Auto-negotiation next page transmit.
+        pub const NEXT_PAGE_TRANSMIT: Self = C22(0x07);
+        /// Auto-negotiation link partner received next page.
+        pub const LP_RECEIVED_NEXT_PAGE: Self = C22(0x08);
+        /// Master-slave control.
+        pub const MASTER_SLAVE_CONTROL: Self = C22(0x09);
+        /// Master-slave status.
+        pub const MASTER_SLAVE_STATUS: Self = C22(0x0a);
+        /// PSE Control.
+        pub const PSE_CONTROL: Self = C22(0x0b);
+        /// PSE Status.
+        pub const PSE_STATUS: Self = C22(0x0c);
+        /// MMD access control.
+        pub const MMD_CONTROL: Self = C22(0x0d);
+        /// MMD access address data.
+        pub const MMD_DATA: Self = C22(0x0e);
+        /// Extended status.
+        pub const EXTENDED_STATUS: Self = C22(0x0f);
+
+        /// Creates a new instance of `C22` with a vendor specific register.
+        pub const fn vendor_specific<const N: u8>() -> Self {
+            build_assert!(N > 0x0f && N < 0x20);
+            C22(N)
+        }
+    }
+
+    impl Access for C22 {
+        /// Reads a given C22 PHY register.
+        fn read(&self, dev: &mut Device) -> Result<u16> {
+            let phydev = dev.0.get();
+            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
+            // So it's just an FFI call, open code of `phy_read()` with a valid `phy_device` pointer
+            // `phydev`.
+            let ret = unsafe {
+                bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, self.0.into())
+            };
+            if ret < 0 {
+                Err(Error::from_errno(ret))
+            } else {
+                Ok(ret as u16)
+            }
+        }
+
+        /// Writes a given C22 PHY register.
+        fn write(&self, dev: &mut Device, val: u16) -> Result {
+            let phydev = dev.0.get();
+            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
+            // So it's just an FFI call, open code of `phy_write()` with a valid `phy_device` pointer
+            // `phydev`.
+            to_result(unsafe {
+                bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, self.0.into(), val)
+            })
+        }
+    }
+
+    /// MDIO manageable devices.
+    pub struct Mmd(u8);
+
+    impl Mmd {
+        /// Physical Medium Attachment/Dependent.
+        pub const PMAPMD: Self = Mmd(uapi::MDIO_MMD_PMAPMD as u8);
+        /// WAN interface sublayer.
+        pub const WIS: Self = Mmd(uapi::MDIO_MMD_WIS as u8);
+        /// Physical coding sublayer.
+        pub const PCS: Self = Mmd(uapi::MDIO_MMD_PCS as u8);
+        /// PHY Extender sublayer.
+        pub const PHYXS: Self = Mmd(uapi::MDIO_MMD_PHYXS as u8);
+        /// DTE Extender sublayer.
+        pub const DTEXS: Self = Mmd(uapi::MDIO_MMD_DTEXS as u8);
+        /// Transmission convergence.
+        pub const TC: Self = Mmd(uapi::MDIO_MMD_TC as u8);
+        /// Auto negotiation.
+        pub const AN: Self = Mmd(uapi::MDIO_MMD_AN as u8);
+        /// Clause 22 extension.
+        pub const C22_EXT: Self = Mmd(uapi::MDIO_MMD_C22EXT as u8);
+        /// Vendor specific 1.
+        pub const VEND1: Self = Mmd(uapi::MDIO_MMD_VEND1 as u8);
+        /// Vendor specific 2.
+        pub const VEND2: Self = Mmd(uapi::MDIO_MMD_VEND2 as u8);
+
+        /// Creates a new instance of `Mmd` with a specific device address.
+        pub fn new(devad: u8) -> Self {
+            Mmd(devad)
+        }
+    }
+
+    /// C45 registers.
+    pub struct C45 {
+        devad: Mmd,
+        regnum: u16,
+    }
+
+    impl C45 {
+        /// Creates a new instance of `C45`.
+        pub fn new(devad: Mmd, regnum: u16) -> Self {
+            Self { devad, regnum }
+        }
+    }
+
+    impl Access for C45 {
+        /// Reads a given C45 PHY register.
+        fn read(&self, dev: &mut Device) -> Result<u16> {
+            let phydev = dev.0.get();
+            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
+            // So it's just an FFI call.
+            let ret =
+                unsafe { bindings::phy_read_mmd(phydev, self.devad.0.into(), self.regnum.into()) };
+            if ret < 0 {
+                Err(Error::from_errno(ret))
+            } else {
+                Ok(ret as u16)
+            }
+        }
+
+        /// Writes a given C45 PHY register.
+        fn write(&self, dev: &mut Device, val: u16) -> Result {
+            let phydev = dev.0.get();
+            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
+            // So it's just an FFI call.
+            to_result(unsafe {
+                bindings::phy_write_mmd(phydev, self.devad.0.into(), self.regnum.into(), val)
+            })
+        }
+    }
+}
+
 /// Defines certain other features this PHY supports (like interrupts).
 ///
 /// These flag values are used in [`Driver::FLAGS`].
diff --git a/rust/uapi/uapi_helper.h b/rust/uapi/uapi_helper.h
index 08f5e9334c9e..76d3f103e764 100644
--- a/rust/uapi/uapi_helper.h
+++ b/rust/uapi/uapi_helper.h
@@ -7,5 +7,6 @@
  */
 
 #include <uapi/asm-generic/ioctl.h>
+#include <uapi/linux/mdio.h>
 #include <uapi/linux/mii.h>
 #include <uapi/linux/ethtool.h>
-- 
2.34.1

Re: [RFC PATCH v2 1/2] rust: net::phy support for C45 register access

[Benno Lossin]

On 01.06.24 06:35, FUJITA Tomonori wrote:
> Adds support for C45 register access. Instead of adding read/write_c45
> methods, this unifies the API to access C22 and C45 registers with
> trait.

The short commit message only mentions C45, should it also include C22?

> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  drivers/net/phy/ax88796b_rust.rs |   7 +-
>  rust/kernel/net/phy.rs           | 192 +++++++++++++++++++++++++++----
>  rust/uapi/uapi_helper.h          |   1 +
>  3 files changed, 173 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
> index 5c92572962dc..8c7eb009d9fc 100644
> --- a/drivers/net/phy/ax88796b_rust.rs
> +++ b/drivers/net/phy/ax88796b_rust.rs
> @@ -6,7 +6,7 @@
>  //! C version of this driver: [`drivers/net/phy/ax88796b.c`](./ax88796b.c)
>  use kernel::{
>      c_str,
> -    net::phy::{self, DeviceId, Driver},
> +    net::phy::{self, reg::C22, DeviceId, Driver},
>      prelude::*,
>      uapi,
>  };
> @@ -24,7 +24,6 @@
>      license: "GPL",
>  }
> 
> -const MII_BMCR: u16 = uapi::MII_BMCR as u16;
>  const BMCR_SPEED100: u16 = uapi::BMCR_SPEED100 as u16;
>  const BMCR_FULLDPLX: u16 = uapi::BMCR_FULLDPLX as u16;
> 
> @@ -33,7 +32,7 @@
>  // Toggle BMCR_RESET bit off to accommodate broken AX8796B PHY implementation
>  // such as used on the Individual Computers' X-Surf 100 Zorro card.
>  fn asix_soft_reset(dev: &mut phy::Device) -> Result {
> -    dev.write(uapi::MII_BMCR as u16, 0)?;
> +    dev.write(C22::BMCR, 0)?;
>      dev.genphy_soft_reset()
>  }
> 
> @@ -55,7 +54,7 @@ fn read_status(dev: &mut phy::Device) -> Result<u16> {
>          }
>          // If MII_LPA is 0, phy_resolve_aneg_linkmode() will fail to resolve
>          // linkmode so use MII_BMCR as default values.
> -        let ret = dev.read(MII_BMCR)?;
> +        let ret = dev.read(C22::BMCR)?;
> 
>          if ret & BMCR_SPEED100 != 0 {
>              dev.set_speed(uapi::SPEED_100);
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index fd40b703d224..286fd5a7ec91 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -175,32 +175,15 @@ pub fn set_duplex(&mut self, mode: DuplexMode) {
>          unsafe { (*phydev).duplex = v };
>      }
> 
> -    /// Reads a given C22 PHY register.
> +    /// Reads a PHY register.
>      // This function reads a hardware register and updates the stats so takes `&mut self`.
> -    pub fn read(&mut self, regnum: u16) -> Result<u16> {
> -        let phydev = self.0.get();
> -        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> -        // So it's just an FFI call, open code of `phy_read()` with a valid `phy_device` pointer
> -        // `phydev`.
> -        let ret = unsafe {
> -            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
> -        };
> -        if ret < 0 {
> -            Err(Error::from_errno(ret))
> -        } else {
> -            Ok(ret as u16)
> -        }
> +    pub fn read<R: reg::Access>(&mut self, reg: R) -> Result<u16> {
> +        reg.read(self)
>      }
> 
> -    /// Writes a given C22 PHY register.
> -    pub fn write(&mut self, regnum: u16, val: u16) -> Result {
> -        let phydev = self.0.get();
> -        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> -        // So it's just an FFI call, open code of `phy_write()` with a valid `phy_device` pointer
> -        // `phydev`.
> -        to_result(unsafe {
> -            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
> -        })
> +    /// Writes a PHY register.
> +    pub fn write<R: reg::Access>(&mut self, reg: R, val: u16) -> Result {
> +        reg.write(self, val)
>      }
> 
>      /// Reads a paged register.
> @@ -302,6 +285,169 @@ pub fn genphy_read_abilities(&mut self) -> Result {
>      }
>  }
> 
> +/// Defines PHY registers.
> +pub mod reg {

I would have put this into its own file under `phy/reg.rs`.

> +    use super::Device;
> +    use crate::build_assert;
> +    use crate::error::*;
> +    use crate::uapi;
> +
> +    /// Reads and writes PHY registers.
> +    ///
> +    /// This trait is used to implement multiple ways to read and write
> +    /// PHY registers.
> +    pub trait Access {

I think `Register` is a better name.

> +        /// Reads a PHY register.
> +        fn read(&self, dev: &mut Device) -> Result<u16>;
> +
> +        /// Writes a PHY register.
> +        fn write(&self, dev: &mut Device, val: u16) -> Result;
> +    }
> +
> +    /// C22 registers.
> +    pub struct C22(u8);
> +
> +    impl C22 {
> +        /// Basic mode control.
> +        pub const BMCR: Self = C22(0x00);
> +        /// Basic mode status.
> +        pub const BMSR: Self = C22(0x01);
> +        /// PHY identifier 1.
> +        pub const PHYSID1: Self = C22(0x02);
> +        /// PHY identifier 2.
> +        pub const PHYSID2: Self = C22(0x03);
> +        /// Auto-negotiation advertisement.
> +        pub const ADVERTISE: Self = C22(0x04);
> +        /// Auto-negotiation link partner base page ability.
> +        pub const LPA: Self = C22(0x05);
> +        /// Auto-negotiation expansion.
> +        pub const EXPANSION: Self = C22(0x06);
> +        /// Auto-negotiation next page transmit.
> +        pub const NEXT_PAGE_TRANSMIT: Self = C22(0x07);
> +        /// Auto-negotiation link partner received next page.
> +        pub const LP_RECEIVED_NEXT_PAGE: Self = C22(0x08);
> +        /// Master-slave control.
> +        pub const MASTER_SLAVE_CONTROL: Self = C22(0x09);
> +        /// Master-slave status.
> +        pub const MASTER_SLAVE_STATUS: Self = C22(0x0a);
> +        /// PSE Control.
> +        pub const PSE_CONTROL: Self = C22(0x0b);
> +        /// PSE Status.
> +        pub const PSE_STATUS: Self = C22(0x0c);
> +        /// MMD access control.
> +        pub const MMD_CONTROL: Self = C22(0x0d);
> +        /// MMD access address data.
> +        pub const MMD_DATA: Self = C22(0x0e);
> +        /// Extended status.
> +        pub const EXTENDED_STATUS: Self = C22(0x0f);
> +
> +        /// Creates a new instance of `C22` with a vendor specific register.
> +        pub const fn vendor_specific<const N: u8>() -> Self {
> +            build_assert!(N > 0x0f && N < 0x20);

An error message would be nice.

> +            C22(N)
> +        }
> +    }
> +
> +    impl Access for C22 {
> +        /// Reads a given C22 PHY register.
> +        fn read(&self, dev: &mut Device) -> Result<u16> {
> +            let phydev = dev.0.get();
> +            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
> +            // So it's just an FFI call, open code of `phy_read()` with a valid `phy_device` pointer
> +            // `phydev`.
> +            let ret = unsafe {
> +                bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, self.0.into())
> +            };
> +            if ret < 0 {
> +                Err(Error::from_errno(ret))
> +            } else {
> +                Ok(ret as u16)
> +            }
> +        }
> +
> +        /// Writes a given C22 PHY register.
> +        fn write(&self, dev: &mut Device, val: u16) -> Result {
> +            let phydev = dev.0.get();
> +            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
> +            // So it's just an FFI call, open code of `phy_write()` with a valid `phy_device` pointer
> +            // `phydev`.
> +            to_result(unsafe {
> +                bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, self.0.into(), val)
> +            })
> +        }
> +    }
> +
> +    /// MDIO manageable devices.
> +    pub struct Mmd(u8);
> +
> +    impl Mmd {
> +        /// Physical Medium Attachment/Dependent.
> +        pub const PMAPMD: Self = Mmd(uapi::MDIO_MMD_PMAPMD as u8);
> +        /// WAN interface sublayer.
> +        pub const WIS: Self = Mmd(uapi::MDIO_MMD_WIS as u8);
> +        /// Physical coding sublayer.
> +        pub const PCS: Self = Mmd(uapi::MDIO_MMD_PCS as u8);
> +        /// PHY Extender sublayer.
> +        pub const PHYXS: Self = Mmd(uapi::MDIO_MMD_PHYXS as u8);
> +        /// DTE Extender sublayer.
> +        pub const DTEXS: Self = Mmd(uapi::MDIO_MMD_DTEXS as u8);
> +        /// Transmission convergence.
> +        pub const TC: Self = Mmd(uapi::MDIO_MMD_TC as u8);
> +        /// Auto negotiation.
> +        pub const AN: Self = Mmd(uapi::MDIO_MMD_AN as u8);
> +        /// Clause 22 extension.
> +        pub const C22_EXT: Self = Mmd(uapi::MDIO_MMD_C22EXT as u8);
> +        /// Vendor specific 1.
> +        pub const VEND1: Self = Mmd(uapi::MDIO_MMD_VEND1 as u8);
> +        /// Vendor specific 2.
> +        pub const VEND2: Self = Mmd(uapi::MDIO_MMD_VEND2 as u8);
> +
> +        /// Creates a new instance of `Mmd` with a specific device address.
> +        pub fn new(devad: u8) -> Self {
> +            Mmd(devad)

Is there also a range that makes sense to exclude (similar to C22)?

> +        }
> +    }
> +
> +    /// C45 registers.
> +    pub struct C45 {
> +        devad: Mmd,
> +        regnum: u16,
> +    }
> +
> +    impl C45 {
> +        /// Creates a new instance of `C45`.
> +        pub fn new(devad: Mmd, regnum: u16) -> Self {

I really like that the first argument has a proper type now, do you
think that the same is also possible for `regnum`? Or are there no
common values for `regnum` (that also have the same meaning across
devices/drivers)? In that case, keep the `u16`.

---
Cheers,
Benno

> +            Self { devad, regnum }
> +        }
> +    }
> +
> +    impl Access for C45 {
> +        /// Reads a given C45 PHY register.
> +        fn read(&self, dev: &mut Device) -> Result<u16> {
> +            let phydev = dev.0.get();
> +            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
> +            // So it's just an FFI call.
> +            let ret =
> +                unsafe { bindings::phy_read_mmd(phydev, self.devad.0.into(), self.regnum.into()) };
> +            if ret < 0 {
> +                Err(Error::from_errno(ret))
> +            } else {
> +                Ok(ret as u16)
> +            }
> +        }
> +
> +        /// Writes a given C45 PHY register.
> +        fn write(&self, dev: &mut Device, val: u16) -> Result {
> +            let phydev = dev.0.get();
> +            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
> +            // So it's just an FFI call.
> +            to_result(unsafe {
> +                bindings::phy_write_mmd(phydev, self.devad.0.into(), self.regnum.into(), val)
> +            })
> +        }
> +    }
> +}
> +
>  /// Defines certain other features this PHY supports (like interrupts).
>  ///
>  /// These flag values are used in [`Driver::FLAGS`].
> diff --git a/rust/uapi/uapi_helper.h b/rust/uapi/uapi_helper.h
> index 08f5e9334c9e..76d3f103e764 100644
> --- a/rust/uapi/uapi_helper.h
> +++ b/rust/uapi/uapi_helper.h
> @@ -7,5 +7,6 @@
>   */
> 
>  #include <uapi/asm-generic/ioctl.h>
> +#include <uapi/linux/mdio.h>
>  #include <uapi/linux/mii.h>
>  #include <uapi/linux/ethtool.h>
> --
> 2.34.1
> 

Re: [RFC PATCH v2 1/2] rust: net::phy support for C45 register access

[FUJITA Tomonori]

On Sat, 01 Jun 2024 12:21:14 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 01.06.24 06:35, FUJITA Tomonori wrote:
>> Adds support for C45 register access. Instead of adding read/write_c45
>> methods, this unifies the API to access C22 and C45 registers with
>> trait.
> 
> The short commit message only mentions C45, should it also include C22?

How about the following?

Adds support for C45 register access. The abstractions support access
to only C22 registers now. Instead of adding read/write_c45 methods
specifically for C45, a new reg module supports the unified API to
access to C22 and C45 registers with trait, by calling an appropriate
phylib functions.


>>      /// Reads a paged register.
>> @@ -302,6 +285,169 @@ pub fn genphy_read_abilities(&mut self) -> Result {
>>      }
>>  }
>> 
>> +/// Defines PHY registers.
>> +pub mod reg {
> 
> I would have put this into its own file under `phy/reg.rs`.

I'll do.

>> +    use super::Device;
>> +    use crate::build_assert;
>> +    use crate::error::*;
>> +    use crate::uapi;
>> +
>> +    /// Reads and writes PHY registers.
>> +    ///
>> +    /// This trait is used to implement multiple ways to read and write
>> +    /// PHY registers.
>> +    pub trait Access {
> 
> I think `Register` is a better name.

Ok, I'll go with `Register`.

>> +        /// Reads a PHY register.
>> +        fn read(&self, dev: &mut Device) -> Result<u16>;
>> +
>> +        /// Writes a PHY register.
>> +        fn write(&self, dev: &mut Device, val: u16) -> Result;
>> +    }
>> +
>> +    /// C22 registers.
>> +    pub struct C22(u8);
>> +
>> +    impl C22 {
>> +        /// Basic mode control.
>> +        pub const BMCR: Self = C22(0x00);
>> +        /// Basic mode status.
>> +        pub const BMSR: Self = C22(0x01);
>> +        /// PHY identifier 1.
>> +        pub const PHYSID1: Self = C22(0x02);
>> +        /// PHY identifier 2.
>> +        pub const PHYSID2: Self = C22(0x03);
>> +        /// Auto-negotiation advertisement.
>> +        pub const ADVERTISE: Self = C22(0x04);
>> +        /// Auto-negotiation link partner base page ability.
>> +        pub const LPA: Self = C22(0x05);
>> +        /// Auto-negotiation expansion.
>> +        pub const EXPANSION: Self = C22(0x06);
>> +        /// Auto-negotiation next page transmit.
>> +        pub const NEXT_PAGE_TRANSMIT: Self = C22(0x07);
>> +        /// Auto-negotiation link partner received next page.
>> +        pub const LP_RECEIVED_NEXT_PAGE: Self = C22(0x08);
>> +        /// Master-slave control.
>> +        pub const MASTER_SLAVE_CONTROL: Self = C22(0x09);
>> +        /// Master-slave status.
>> +        pub const MASTER_SLAVE_STATUS: Self = C22(0x0a);
>> +        /// PSE Control.
>> +        pub const PSE_CONTROL: Self = C22(0x0b);
>> +        /// PSE Status.
>> +        pub const PSE_STATUS: Self = C22(0x0c);
>> +        /// MMD access control.
>> +        pub const MMD_CONTROL: Self = C22(0x0d);
>> +        /// MMD access address data.
>> +        pub const MMD_DATA: Self = C22(0x0e);
>> +        /// Extended status.
>> +        pub const EXTENDED_STATUS: Self = C22(0x0f);
>> +
>> +        /// Creates a new instance of `C22` with a vendor specific register.
>> +        pub const fn vendor_specific<const N: u8>() -> Self {
>> +            build_assert!(N > 0x0f && N < 0x20);
> 
> An error message would be nice.

I'll add.

>> +            C22(N)
>> +        }
>> +    }
>> +
>> +    impl Access for C22 {
>> +        /// Reads a given C22 PHY register.
>> +        fn read(&self, dev: &mut Device) -> Result<u16> {
>> +            let phydev = dev.0.get();
>> +            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
>> +            // So it's just an FFI call, open code of `phy_read()` with a valid `phy_device` pointer
>> +            // `phydev`.
>> +            let ret = unsafe {
>> +                bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, self.0.into())
>> +            };
>> +            if ret < 0 {
>> +                Err(Error::from_errno(ret))
>> +            } else {
>> +                Ok(ret as u16)
>> +            }
>> +        }
>> +
>> +        /// Writes a given C22 PHY register.
>> +        fn write(&self, dev: &mut Device, val: u16) -> Result {
>> +            let phydev = dev.0.get();
>> +            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
>> +            // So it's just an FFI call, open code of `phy_write()` with a valid `phy_device` pointer
>> +            // `phydev`.
>> +            to_result(unsafe {
>> +                bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, self.0.into(), val)
>> +            })
>> +        }
>> +    }
>> +
>> +    /// MDIO manageable devices.
>> +    pub struct Mmd(u8);
>> +
>> +    impl Mmd {
>> +        /// Physical Medium Attachment/Dependent.
>> +        pub const PMAPMD: Self = Mmd(uapi::MDIO_MMD_PMAPMD as u8);
>> +        /// WAN interface sublayer.
>> +        pub const WIS: Self = Mmd(uapi::MDIO_MMD_WIS as u8);
>> +        /// Physical coding sublayer.
>> +        pub const PCS: Self = Mmd(uapi::MDIO_MMD_PCS as u8);
>> +        /// PHY Extender sublayer.
>> +        pub const PHYXS: Self = Mmd(uapi::MDIO_MMD_PHYXS as u8);
>> +        /// DTE Extender sublayer.
>> +        pub const DTEXS: Self = Mmd(uapi::MDIO_MMD_DTEXS as u8);
>> +        /// Transmission convergence.
>> +        pub const TC: Self = Mmd(uapi::MDIO_MMD_TC as u8);
>> +        /// Auto negotiation.
>> +        pub const AN: Self = Mmd(uapi::MDIO_MMD_AN as u8);
>> +        /// Clause 22 extension.
>> +        pub const C22_EXT: Self = Mmd(uapi::MDIO_MMD_C22EXT as u8);
>> +        /// Vendor specific 1.
>> +        pub const VEND1: Self = Mmd(uapi::MDIO_MMD_VEND1 as u8);
>> +        /// Vendor specific 2.
>> +        pub const VEND2: Self = Mmd(uapi::MDIO_MMD_VEND2 as u8);
>> +
>> +        /// Creates a new instance of `Mmd` with a specific device address.
>> +        pub fn new(devad: u8) -> Self {
>> +            Mmd(devad)
> 
> Is there also a range that makes sense to exclude (similar to C22)?

Not, I think. C22 saves 0x10..0x1f for vendor specific registers. But
C45 has VEND1 and VEND2 for the same purpose.

Andrew, we need to allow a driver to use reserved addresses (14-28) in
C45? If not, we don't new() func to allow a driver to use them.

I overlooked some addresses in 802. I'll add const for all the
addresses in v3.


>> +        }
>> +    }
>> +
>> +    /// C45 registers.
>> +    pub struct C45 {
>> +        devad: Mmd,
>> +        regnum: u16,
>> +    }
>> +
>> +    impl C45 {
>> +        /// Creates a new instance of `C45`.
>> +        pub fn new(devad: Mmd, regnum: u16) -> Self {
> 
> I really like that the first argument has a proper type now, do you
> think that the same is also possible for `regnum`? Or are there no

I think that it's difficult.

> common values for `regnum` (that also have the same meaning across
> devices/drivers)? In that case, keep the `u16`.

Many are defined in the specification. Drivers might use registers
undefined in the spec for some reasons, I assume.

Re: [RFC PATCH v2 1/2] rust: net::phy support for C45 register access

[Benno Lossin]

On 02.06.24 09:24, FUJITA Tomonori wrote:
> On Sat, 01 Jun 2024 12:21:14 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>> On 01.06.24 06:35, FUJITA Tomonori wrote:
>>> Adds support for C45 register access. Instead of adding read/write_c45
>>> methods, this unifies the API to access C22 and C45 registers with
>>> trait.
>>
>> The short commit message only mentions C45, should it also include C22?
> 
> How about the following?
> 
> Adds support for C45 register access. The abstractions support access
> to only C22 registers now. Instead of adding read/write_c45 methods
> specifically for C45, a new reg module supports the unified API to
> access to C22 and C45 registers with trait, by calling an appropriate
> phylib functions.

Sure you can use that, but my comment above was about the short message
(ie the first line/the email title. That one only mentions C22).

[...]

>>> +    /// MDIO manageable devices.
>>> +    pub struct Mmd(u8);
>>> +
>>> +    impl Mmd {
>>> +        /// Physical Medium Attachment/Dependent.
>>> +        pub const PMAPMD: Self = Mmd(uapi::MDIO_MMD_PMAPMD as u8);
>>> +        /// WAN interface sublayer.
>>> +        pub const WIS: Self = Mmd(uapi::MDIO_MMD_WIS as u8);
>>> +        /// Physical coding sublayer.
>>> +        pub const PCS: Self = Mmd(uapi::MDIO_MMD_PCS as u8);
>>> +        /// PHY Extender sublayer.
>>> +        pub const PHYXS: Self = Mmd(uapi::MDIO_MMD_PHYXS as u8);
>>> +        /// DTE Extender sublayer.
>>> +        pub const DTEXS: Self = Mmd(uapi::MDIO_MMD_DTEXS as u8);
>>> +        /// Transmission convergence.
>>> +        pub const TC: Self = Mmd(uapi::MDIO_MMD_TC as u8);
>>> +        /// Auto negotiation.
>>> +        pub const AN: Self = Mmd(uapi::MDIO_MMD_AN as u8);
>>> +        /// Clause 22 extension.
>>> +        pub const C22_EXT: Self = Mmd(uapi::MDIO_MMD_C22EXT as u8);
>>> +        /// Vendor specific 1.
>>> +        pub const VEND1: Self = Mmd(uapi::MDIO_MMD_VEND1 as u8);
>>> +        /// Vendor specific 2.
>>> +        pub const VEND2: Self = Mmd(uapi::MDIO_MMD_VEND2 as u8);
>>> +
>>> +        /// Creates a new instance of `Mmd` with a specific device address.
>>> +        pub fn new(devad: u8) -> Self {
>>> +            Mmd(devad)
>>
>> Is there also a range that makes sense to exclude (similar to C22)?
> 
> Not, I think. C22 saves 0x10..0x1f for vendor specific registers. But
> C45 has VEND1 and VEND2 for the same purpose.
> 
> Andrew, we need to allow a driver to use reserved addresses (14-28) in
> C45? If not, we don't new() func to allow a driver to use them.

You can also just leave it out and if some driver needs it at a later
point, you can just add it at that time.

> I overlooked some addresses in 802. I'll add const for all the
> addresses in v3.
> 
> 
>>> +        }
>>> +    }
>>> +
>>> +    /// C45 registers.
>>> +    pub struct C45 {
>>> +        devad: Mmd,
>>> +        regnum: u16,
>>> +    }
>>> +
>>> +    impl C45 {
>>> +        /// Creates a new instance of `C45`.
>>> +        pub fn new(devad: Mmd, regnum: u16) -> Self {
>>
>> I really like that the first argument has a proper type now, do you
>> think that the same is also possible for `regnum`? Or are there no
> 
> I think that it's difficult.
> 
>> common values for `regnum` (that also have the same meaning across
>> devices/drivers)? In that case, keep the `u16`.
> 
> Many are defined in the specification. Drivers might use registers
> undefined in the spec for some reasons, I assume.

I see, well then just use the `u16` and the drivers can define their own
constants.

---
Cheers,
Benno

[RFC PATCH v2 2/2] rust: net::phy support for C45 genphy helpers

[FUJITA Tomonori]

Add support for genphy_c45_* helper functions. Instead of adding
genphy_c45_ methods, this unifies the API for genphy functions with
trait.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/net/phy.rs | 49 +++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 286fd5a7ec91..aabd714ae3f6 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -248,16 +248,8 @@ pub fn genphy_suspend(&mut self) -> Result {
     }
 
     /// Checks the link status and updates current link state.
-    pub fn genphy_read_status(&mut self) -> Result<u16> {
-        let phydev = self.0.get();
-        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
-        // So it's just an FFI call.
-        let ret = unsafe { bindings::genphy_read_status(phydev) };
-        if ret < 0 {
-            Err(Error::from_errno(ret))
-        } else {
-            Ok(ret as u16)
-        }
+    pub fn genphy_read_status<G: reg::GenPhyOps>(&mut self) -> Result<u16> {
+        G::read_status(self)
     }
 
     /// Updates the link status.
@@ -446,6 +438,43 @@ fn write(&self, dev: &mut Device, val: u16) -> Result {
             })
         }
     }
+
+    /// PHY helper functions.
+    ///
+    /// This trait is used to implement some helper functions executed either
+    /// via C22 or C45 registers.
+    pub trait GenPhyOps {
+        /// Checks the link status and updates current link state.
+        fn read_status(dev: &mut Device) -> Result<u16>;
+    }
+
+    impl GenPhyOps for C22 {
+        fn read_status(dev: &mut Device) -> Result<u16> {
+            let phydev = dev.0.get();
+            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+            // So it's just an FFI call.
+            let ret = unsafe { bindings::genphy_read_status(phydev) };
+            if ret < 0 {
+                Err(Error::from_errno(ret))
+            } else {
+                Ok(ret as u16)
+            }
+        }
+    }
+
+    impl GenPhyOps for C45 {
+        fn read_status(dev: &mut Device) -> Result<u16> {
+            let phydev = dev.0.get();
+            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+            // So it's just an FFI call.
+            let ret = unsafe { bindings::genphy_c45_read_status(phydev) };
+            if ret < 0 {
+                Err(Error::from_errno(ret))
+            } else {
+                Ok(ret as u16)
+            }
+        }
+    }
 }
 
 /// Defines certain other features this PHY supports (like interrupts).
-- 
2.34.1

Re: [RFC PATCH v2 2/2] rust: net::phy support for C45 genphy helpers

[Benno Lossin]

On 01.06.24 06:35, FUJITA Tomonori wrote:
> Add support for genphy_c45_* helper functions. Instead of adding
> genphy_c45_ methods, this unifies the API for genphy functions with
> trait.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/net/phy.rs | 49 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 10 deletions(-)
> 
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 286fd5a7ec91..aabd714ae3f6 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -248,16 +248,8 @@ pub fn genphy_suspend(&mut self) -> Result {
>      }
> 
>      /// Checks the link status and updates current link state.
> -    pub fn genphy_read_status(&mut self) -> Result<u16> {
> -        let phydev = self.0.get();
> -        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> -        // So it's just an FFI call.
> -        let ret = unsafe { bindings::genphy_read_status(phydev) };
> -        if ret < 0 {
> -            Err(Error::from_errno(ret))
> -        } else {
> -            Ok(ret as u16)
> -        }
> +    pub fn genphy_read_status<G: reg::GenPhyOps>(&mut self) -> Result<u16> {

With this change, don't you also need a change in the ax88796b Rust
driver?

> +        G::read_status(self)
>      }
> 
>      /// Updates the link status.
> @@ -446,6 +438,43 @@ fn write(&self, dev: &mut Device, val: u16) -> Result {
>              })
>          }
>      }
> +
> +    /// PHY helper functions.
> +    ///
> +    /// This trait is used to implement some helper functions executed either
> +    /// via C22 or C45 registers.
> +    pub trait GenPhyOps {

Why does this need another trait? Can't it be part of the `Register`
(currently named `Access`) trait?

---
Cheers,
Benno

> +        /// Checks the link status and updates current link state.
> +        fn read_status(dev: &mut Device) -> Result<u16>;
> +    }
> +
> +    impl GenPhyOps for C22 {
> +        fn read_status(dev: &mut Device) -> Result<u16> {
> +            let phydev = dev.0.get();
> +            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +            // So it's just an FFI call.
> +            let ret = unsafe { bindings::genphy_read_status(phydev) };
> +            if ret < 0 {
> +                Err(Error::from_errno(ret))
> +            } else {
> +                Ok(ret as u16)
> +            }
> +        }
> +    }
> +
> +    impl GenPhyOps for C45 {
> +        fn read_status(dev: &mut Device) -> Result<u16> {
> +            let phydev = dev.0.get();
> +            // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +            // So it's just an FFI call.
> +            let ret = unsafe { bindings::genphy_c45_read_status(phydev) };
> +            if ret < 0 {
> +                Err(Error::from_errno(ret))
> +            } else {
> +                Ok(ret as u16)
> +            }
> +        }
> +    }
>  }
> 
>  /// Defines certain other features this PHY supports (like interrupts).
> --
> 2.34.1
> 

Re: [RFC PATCH v2 2/2] rust: net::phy support for C45 genphy helpers

[FUJITA Tomonori]

On Sat, 01 Jun 2024 12:23:16 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 01.06.24 06:35, FUJITA Tomonori wrote:
>> Add support for genphy_c45_* helper functions. Instead of adding
>> genphy_c45_ methods, this unifies the API for genphy functions with
>> trait.
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> ---
>>  rust/kernel/net/phy.rs | 49 +++++++++++++++++++++++++++++++++---------
>>  1 file changed, 39 insertions(+), 10 deletions(-)
>> 
>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>> index 286fd5a7ec91..aabd714ae3f6 100644
>> --- a/rust/kernel/net/phy.rs
>> +++ b/rust/kernel/net/phy.rs
>> @@ -248,16 +248,8 @@ pub fn genphy_suspend(&mut self) -> Result {
>>      }
>> 
>>      /// Checks the link status and updates current link state.
>> -    pub fn genphy_read_status(&mut self) -> Result<u16> {
>> -        let phydev = self.0.get();
>> -        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> -        // So it's just an FFI call.
>> -        let ret = unsafe { bindings::genphy_read_status(phydev) };
>> -        if ret < 0 {
>> -            Err(Error::from_errno(ret))
>> -        } else {
>> -            Ok(ret as u16)
>> -        }
>> +    pub fn genphy_read_status<G: reg::GenPhyOps>(&mut self) -> Result<u16> {
> 
> With this change, don't you also need a change in the ax88796b Rust
> driver?

No, the driver doesn't use genphy_read_status().


>> +        G::read_status(self)
>>      }
>> 
>>      /// Updates the link status.
>> @@ -446,6 +438,43 @@ fn write(&self, dev: &mut Device, val: u16) -> Result {
>>              })
>>          }
>>      }
>> +
>> +    /// PHY helper functions.
>> +    ///
>> +    /// This trait is used to implement some helper functions executed either
>> +    /// via C22 or C45 registers.
>> +    pub trait GenPhyOps {
> 
> Why does this need another trait? Can't it be part of the `Register`
> (currently named `Access`) trait?

Indeed, it can be. I'll do with the comment on `Register` updated.

Thanks a lot!