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

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

[FUJITA Tomonori]

Hi,

After the discussion on C45 register support, I implemented a more rusty
solution based on Benno's idea (hopefully, I understand correctly):

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.
+///
+/// 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>;
+
+    /// Writes a PHY register.
+    fn write(&self, dev: &mut Device, val: u16) -> Result;
+}
+
+/// Accesses to C22 registers.
+#[derive(Clone, Copy)]
+pub enum RegC22 {
+    /// 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 {
+    devad: u8,
+    regnum: u16,
+}
+
+impl RegC45 {
+    /// Creates a new instance of `RegC45`.
+    pub fn new(devad: u8, regnum: u16) -> Self {
+        Self { devad, regnum }
+    }
+}
+
+impl RegAccess for RegC45 {
+    /// 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.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.into(), self.regnum.into(), val)
+        })
+    }
+}
+
+/// Accesses to C45 registers with C45 bus protocol.
+///
+/// This struct is used to access C45 registers with C45 bus protocol;
+/// C45 over C22 bus protocol isn't used.
+pub struct RegC45Direct {
+    devad: u8,
+    regnum: u16,
+}
+
+impl RegC45Direct {
+    /// Creates a new instance of `RegC45Direct`.
+    pub fn new(devad: u8, regnum: u16) -> Self {
+        Self { devad, regnum }
+    }
+}
+
+impl RegAccess for RegC45Direct {
+    /// 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::mdiobus_c45_read(
+                (*phydev).mdio.bus,
+                (*phydev).mdio.addr,
+                self.devad.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::mdiobus_c45_write(
+                (*phydev).mdio.bus,
+                (*phydev).mdio.addr,
+                self.devad.into(),
+                self.regnum.into(),
+                val,
+            )
+        })
+    }
+}
+
 /// Defines certain other features this PHY supports (like interrupts).
 ///
 /// These flag values are used in [`Driver::FLAGS`].
-- 
2.34.1

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

[Andrew Lunn]

> +/// Accesses to C22 registers.
> +#[derive(Clone, Copy)]
> +pub enum RegC22 {
> +    /// 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,

802.3 C22 defines 16 through 31 as vendor specific. Most of the
#defines appear to come from the S390 driver. We probably should not
add them in the Rust API when there meaning is not clearly
defined. However, a driver should be able to use them, e.g, by
defining their own vendor specific name to a given value.

	 Andrew

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

[FUJITA Tomonori]

Hi,

On Mon, 27 May 2024 23:57:37 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +/// Accesses to C22 registers.
>> +#[derive(Clone, Copy)]
>> +pub enum RegC22 {
>> +    /// 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,
> 
> 802.3 C22 defines 16 through 31 as vendor specific. Most of the
> #defines appear to come from the S390 driver. We probably should not
> add them in the Rust API when there meaning is not clearly
> defined. However, a driver should be able to use them, e.g, by
> defining their own vendor specific name to a given value.

Ah, I was wondering about the difference between 802.C C22 and mii.h:
- the definition of 7, 8, b, and c are missing
- why vendoer specific addresses are defined

So what we should do are:

- define 0-15 according to 802.3.
- doesn't define 16-31 but allow a driver to use

Right?

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

[Andrew Lunn]

> Ah, I was wondering about the difference between 802.C C22 and mii.h:
> - the definition of 7, 8, b, and c are missing

They have not been used yet. There has been talk of using the two
next-page registers, but no patches so far. The PSE registers might be
interesting soon, since PoE is being added.

> - why vendoer specific addresses are defined
> 
> So what we should do are:
> 
> - define 0-15 according to 802.3.
> - doesn't define 16-31 but allow a driver to use
> 
> Right?

Yes.

	Andrew

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

[FUJITA Tomonori]

Hi,

On Tue, 28 May 2024 02:18:05 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> Ah, I was wondering about the difference between 802.C C22 and mii.h:
>> - the definition of 7, 8, b, and c are missing
> 
> They have not been used yet. There has been talk of using the two
> next-page registers, but no patches so far. The PSE registers might be
> interesting soon, since PoE is being added.

Understood.

>> - why vendoer specific addresses are defined
>> 
>> So what we should do are:
>> 
>> - define 0-15 according to 802.3.
>> - doesn't define 16-31 but allow a driver to use
>> 
>> Right?
> 
> Yes.

Thanks for the confirmation.

With updating this patch, I've started to think that it would be more
consistent for the driver to use phy_read_mmd/phy_write_mmd rather
than mdiobus_c45_read and mdiobus_c45_write (RegC45 than RegC45Direct
in this patch). This is because the driver uses
genphy_c45_read_status() helper for the read_status callback, which
also lets phylib to choose an appropriate bus protocol.

Additionally, I thought that we don't need to add RegC45Direct until a
driver that requires it emerges.

What do you think?

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

[Andrew Lunn]

> With updating this patch, I've started to think that it would be more
> consistent for the driver to use phy_read_mmd/phy_write_mmd rather
> than mdiobus_c45_read and mdiobus_c45_write (RegC45 than RegC45Direct
> in this patch). This is because the driver uses
> genphy_c45_read_status() helper for the read_status callback, which
> also lets phylib to choose an appropriate bus protocol.

genphy_c45_read_status() is a good point. We need to keep with the
normal way phylib works, otherwise all the helper functions will
break. So i think you need to implement the struct phy_driver
.read_mmd and .write_mmd ops to do direct C45 accesses. That is not
really how it should work, but that is a phylib problem, not a Rust
problem.

	Andrew

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

[Benno Lossin]

On 27.05.24 23:57, Andrew Lunn wrote:
>> +/// Accesses to C22 registers.
>> +#[derive(Clone, Copy)]
>> +pub enum RegC22 {
>> +    /// 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,
> 
> 802.3 C22 defines 16 through 31 as vendor specific. Most of the
> #defines appear to come from the S390 driver. We probably should not
> add them in the Rust API when there meaning is not clearly
> defined. However, a driver should be able to use them, e.g, by
> defining their own vendor specific name to a given value.

@Fujita, you could fix this by adding a `VendorSpecific(T)` enum variant
and add a generic parameter to `C22`. Then the user has the option to
declare their own variant. You will probably need another trait for the
`T` value to extract the correct register number (you can try to use
`build_assert!` to ensure that the custom registers are not in the 0..16
range).

---
Cheers,
Benno

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

[FUJITA Tomonori]

Hi,

On Thu, 30 May 2024 08:05:50 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 27.05.24 23:57, Andrew Lunn wrote:
>>> +/// Accesses to C22 registers.
>>> +#[derive(Clone, Copy)]
>>> +pub enum RegC22 {
>>> +    /// 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,
>> 
>> 802.3 C22 defines 16 through 31 as vendor specific. Most of the
>> #defines appear to come from the S390 driver. We probably should not
>> add them in the Rust API when there meaning is not clearly
>> defined. However, a driver should be able to use them, e.g, by
>> defining their own vendor specific name to a given value.
> 
> @Fujita, you could fix this by adding a `VendorSpecific(T)` enum variant
> and add a generic parameter to `C22`. Then the user has the option to

I'm not sure. You meant something like:

pub enum C22<T> {
    Bmcr,
    Bmsr,
    VendorSpecific(T),
}

?

Then it's not handy to use the defined registers?

let ret = phy.read(reg::C22::<u8>::Bmcr)?;

I prefer:

let ret = phy.read(reg::C22::Bmcr)?;


> declare their own variant. You will probably need another trait for the
> `T` value to extract the correct register number (you can try to use
> `build_assert!` to ensure that the custom registers are not in the 0..16
> range).

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

[Benno Lossin]

On 31.05.24 15:58, FUJITA Tomonori wrote:
> Hi,
> 
> On Thu, 30 May 2024 08:05:50 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>>> 802.3 C22 defines 16 through 31 as vendor specific. Most of the
>>> #defines appear to come from the S390 driver. We probably should not
>>> add them in the Rust API when there meaning is not clearly
>>> defined. However, a driver should be able to use them, e.g, by
>>> defining their own vendor specific name to a given value.
>>
>> @Fujita, you could fix this by adding a `VendorSpecific(T)` enum variant
>> and add a generic parameter to `C22`. Then the user has the option to
> 
> I'm not sure. You meant something like:
> 
> pub enum C22<T> {
>     Bmcr,
>     Bmsr,
>     VendorSpecific(T),
> }
> 
> ?

Yes.

> Then it's not handy to use the defined registers?
> 
> let ret = phy.read(reg::C22::<u8>::Bmcr)?;
> 
> I prefer:
> 
> let ret = phy.read(reg::C22::Bmcr)?;

Hmm I guess that it probably can't infer a default type. In that case,
you can also try if a struct approach for C22 is better. So:

    pub struct C22(u16); // choose the correct size

    impl C22 {
        pub fn new(reg: u16) -> Self;

        pub const BMCR: Self = Self::new(uapi::MII_BMCR);
        // ...
    }

And then a specific driver can declare its own constants.

---
Cheers,
Benno

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

[FUJITA Tomonori]

On Fri, 31 May 2024 14:04:18 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> Hmm I guess that it probably can't infer a default type. In that case,
> you can also try if a struct approach for C22 is better. So:
> 
>     pub struct C22(u16); // choose the correct size

Thanks, I think that this works nicely.

>     impl C22 {
>         pub fn new(reg: u16) -> Self;
> 
>         pub const BMCR: Self = Self::new(uapi::MII_BMCR);
>         // ...
>     }
> 
> And then a specific driver can declare its own constants.

I uses const generic to declare a vendor specific register in the v2
patch, I've just sent. Please let me know what do you think.

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

[Benno Lossin]

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

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

[FUJITA Tomonori]

Hi,
Thanks for reviewing the patch!

On Thu, 30 May 2024 08:02:35 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

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

Great.

>> 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(-)

(snip)

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

Yeah, fixed.

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

Yes.

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

Makes sense. I did both and like them.


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

Changed.

>> +    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(/* ... */);

I think that there are combinations that are often used. But they
aren't defined in such way, I guess. I suppose that the developpers
are familier with a way to access a register with devad and regnum.