Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[FUJITA Tomonori <fujita.tomonori@gmail.com>]

Hi,

On Mon, 15 Apr 2024 16:20:16 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +    /// Reads a given C45 PHY register.
>> +    /// This function reads a hardware register and updates the stats so takes `&mut self`.
>> +    pub fn c45_read(&mut self, devad: u8, 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.
>> +        let ret = unsafe {
>> +            bindings::mdiobus_c45_read(
>> +                (*phydev).mdio.bus,
>> +                (*phydev).mdio.addr,
>> +                devad as i32,
>> +                regnum.into(),
>> +            )
> 
> So you have wrapped the C function mdiobus_c45_read(). This is going
> to do a C45 bus protocol read. For this to work, the MDIO bus master
> needs to support C45 bus protocol, and the PHY also needs to support
> the C45 bus protocol. Not all MDIO bus masters and PHY devices
> do. Some will need to use C45 over C22.
> 
> A PHY driver should know if a PHY device supports C45 bus protocol,
> and if it supports C45 over C22. However, i PHY driver has no idea
> what the bus master supports.
> 
> In phylib, we have a poorly defined phydev->is_c45. Its current
> meaning is: "The device was found using the C45 bus protocol, not C22
> protocol". One of the things phylib core then uses is_c45 for it to
> direct the C functions phy_read_mmd() and phy_write_mmd() to perform a
> C45 bus protocol access if true. If it is false, C45 over C22 is
> performed instead. As a result, if a PHY is discovered using C22, C45
> register access is then performed using C45 over C22, even thought the
> PHY and bus might support C45 bus protocol. is_c45 is also used in
> other places, e.g. to trigger auto-negotiation using C45 registers,
> not C22 registers.

Thanks a lot for the details!


> In summary, the C API is a bit of a mess.
> 
> For the Rust API we have two sensible choices:
> 
> 1) It is the same mess as the C API, so hopefully one day we will fix
>    both at the same time.
> 
> 2) We define a different API which correctly separate C45 bus access
>    from C45 registers.
> 
> How you currently defined the Rust API is neither of these.

Which do your prefer?

If I correctly understand the original driver code, C45 bus protocol
is used. Adding functions for C45 bus protocol read/write would be
enough for this driver, I guess.