Re: [RFC PATCH v1] rust: net::phy support to C45 registers access

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

On 27.05.24 03:46, FUJITA Tomonori wrote:
> Hi,
> 
> After the discussion on C45 register support, I implemented a more rusty
> solution based on Benno's idea (hopefully, I understand correctly):

Yes this looks similar to what I imagined.

> https://lore.kernel.org/all/20240415104701.4772-3-fujita.tomonori@gmail.com/
> 
> I'm not sure so I sent a RFC patch to rust-for-linux only. Once I have
> something that we could agree on, I'll send non RFC patch to netdev.
> 
> 
> Adds support for C45 registers access. C45 registers can be accessed
> in two ways: either C45 bus protocol or C45 over C22. Normally, a PHY
> driver shouldn't care how to access. PHYLIB chooses the appropriate
> one. But there is an exception; PHY hardware supporting only C45 bus
> protocol.
> 
> This adds two ways to access C45; RegC45 and RegC45Direct. Normally, a
> driver should use the former; let the PHYLIB take care of how to
> access (by calling phy_read_mmd/phy_write_mmd). The latter calls
> mdiobus_c45_read/mdiobus_c45_write; C45 over C22 isn't used.
> 
> The API to access to C22 registers is also updated to align with the
> above changes.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  drivers/net/phy/ax88796b_rust.rs |   7 +-
>  rust/kernel/net/phy.rs           | 218 +++++++++++++++++++++++++++----
>  2 files changed, 198 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
> index 5c92572962dc..9526729dc35a 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, DeviceId, Driver, RegC22},
>      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(RegC22::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(RegC22::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..5bb4a8c9e493 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: RegAccess>(&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: RegAccess>(&mut self, reg: R, val: u16) -> Result {
> +        reg.write(self, val)
>      }
> 
>      /// Reads a paged register.
> @@ -302,6 +285,195 @@ pub fn genphy_read_abilities(&mut self) -> Result {
>      }
>  }
> 
> +/// Reads and writes C22 and C45 registers.

You used "PHY register" below, so you should probably use it here as
well.

> +///
> +/// This trait is used to implement multiple ways to read and write
> +/// PHY registers.
> +pub trait RegAccess {
> +    /// Reads a PHY register.
> +    fn read(&self, dev: &mut Device) -> Result<u16>;

Do all registers have a size of exactly `u16`? If there are also
registers with `u8`, then I would prefer an associated type in the trait
that you then use as the return type.

> +
> +    /// Writes a PHY register.
> +    fn write(&self, dev: &mut Device, val: u16) -> Result;
> +}
> +
> +/// Accesses to C22 registers.
> +#[derive(Clone, Copy)]
> +pub enum RegC22 {

I think we could just name this `C22`.
It might also make sense to move all registers into a `reg` module. What
do you think?

> +    /// Basic mode control.
> +    Bmcr = uapi::MII_BMCR as isize,
> +    /// Basic mode status.
> +    Bmsr = uapi::MII_BMSR as isize,
> +    /// PHY identifier 1.
> +    PhysId1 = uapi::MII_PHYSID1 as isize,
> +    /// PHY identifier 2.
> +    PhysId2 = uapi::MII_PHYSID2 as isize,
> +    /// Auto-negotiation advertisement.
> +    Advertise = uapi::MII_ADVERTISE as isize,
> +    /// Auto-negotiation link partner base page ability.
> +    Lpa = uapi::MII_LPA as isize,
> +    /// Auto-negotiation expansion.
> +    Expansion = uapi::MII_EXPANSION as isize,
> +    /// 1000BASE-T control.
> +    Ctrl1000 = uapi::MII_CTRL1000 as isize,
> +    /// 1000BASE-T status.
> +    Stat1000 = uapi::MII_STAT1000 as isize,
> +    /// MMD access control.
> +    MmdCtrl = uapi::MII_MMD_CTRL as isize,
> +    /// MMD access data.
> +    MmdData = uapi::MII_MMD_DATA as isize,
> +    /// Extended status.
> +    Estatus = uapi::MII_ESTATUS as isize,
> +    /// Disconnect counter.
> +    Dcounter = uapi::MII_DCOUNTER as isize,
> +    /// False carrier sense counter.
> +    Fcscounter = uapi::MII_FCSCOUNTER as isize,
> +    /// N-way auto-negotiation test.
> +    Nwaytest = uapi::MII_NWAYTEST as isize,
> +    /// Receive error counter.
> +    Rerrcounter = uapi::MII_RERRCOUNTER as isize,
> +    /// Silicon revision.
> +    Srervision = uapi::MII_SREVISION as isize,
> +    /// Reserved.
> +    Resv1 = uapi::MII_RESV1 as isize,
> +    /// Lpback, rx, bypass error.
> +    Lbrerror = uapi::MII_LBRERROR as isize,
> +    /// PHY address.
> +    Phyaddr = uapi::MII_PHYADDR as isize,
> +    /// Reserved.
> +    Resv2 = uapi::MII_RESV2 as isize,
> +    /// TPI status for 10Mbps.
> +    TpiStatus = uapi::MII_TPISTATUS as isize,
> +    /// Network interface configuration.
> +    Nconfig = uapi::MII_NCONFIG as isize,
> +}
> +
> +impl RegAccess for RegC22 {
> +    /// 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 as u32)
> +        };
> +        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 as u32, val)
> +        })
> +    }
> +}
> +
> +/// Accesses to C45 registers.
> +///
> +/// C45 registers can be accessed in two ways: either C45 bus protocol or
> +/// C45 over C22 bus protocol. Normally, a PHY driver shouldn't care how to
> +/// access. `PHYLIB` chooses the appropriate one. `RegC45` is preferred over
> +/// `RegC45Direct`.
> +pub struct RegC45 {

Same here, this could just be named `C45`.

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

Are there really no common values that you would use as the parameters
here? If you want to add any, you can do so via associated constants:

    const COMMON_REGISTER: RegC45 = RegC45::new(/* ... */);

Will every driver have their own custom, magic values as input to this
function?

---
Cheers,
Benno

> +        Self { devad, regnum }
> +    }
> +}
> +