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

[Benno Lossin <benno.lossin@proton.me>]

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
>