| Message ID | 20240415104701.4772-3-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: add Applied Micro QT2025 PHY driver | expand |
On Mon, Apr 15, 2024 at 07:46:59PM +0900, FUJITA Tomonori wrote: > This patch adds the following helper functions for Clause 45: > > - mdiobus_c45_read > - mdiobus_c45_write > - genphy_c45_read_status We need to be very careful here, and try to avoid an issue we have in the phylib core, if possible. C45 is a mix of two different things: * The C45 MDIO bus protocol * The C45 registers You can access C45 registers using the C45 bus protocol. You can also access C45 registers using C45 over C22. So there are two access mechanisms to the C45 registers. A PHY driver just wants to access C45 registers. It should not care about what access mechanism is used, be it C45 bus protocol, or C45 over C22. > + /// 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. 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. Andrew
On Mon, Apr 15, 2024 at 6:47 AM FUJITA Tomonori <fujita.tomonori@gmail.com> 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. Depending on the response to Andrew's notes, these SAFETY comments will probably need to be updated to say why we know C45 is supported. > + let ret = unsafe { > + bindings::mdiobus_c45_read( > + (*phydev).mdio.bus, > + (*phydev).mdio.addr, > + devad as i32, This could probably also be from/.into() > + regnum.into(), > + ) > + }; > + if ret < 0 { > + Err(Error::from_errno(ret)) > + } else { > + Ok(ret as u16) > + } Could this be simplified with to_result? > + } > + > + /// Writes a given C45 PHY register. > + pub fn c45_write(&mut self, devad: u8, 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. > + to_result(unsafe { > + bindings::mdiobus_c45_write( > + (*phydev).mdio.bus, > + (*phydev).mdio.addr, > + devad as i32, Same as above > + regnum.into(), > + val, > + ) > + }) > + } > +
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.
> > 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. Now i've read more of the patches, i can see that the MDIO bus master is C45 only. At least, that is all that is implemented in the driver. So for this combination of MAC and PHY, forcing C45 register access using C45 bus protocol will work. However, can you combine this PHY with some other MDIO bus master, which does not support C45? Then C45 over C22 would need to be used? Looking at the data sheet i found online, there is no suggestion it does support C22 bus protocol. All the diagrams/tables only show C45 bus protocol. So this PHY is a special case. So you can use wrapper methods which force C45 bus protocol, and ignore phylib support for performing C45 over C22 when needed. However, while doing this: 1: Clearly document that these helpers are not generic, they force C45 register access using C45 bus protocol, and should only by used PHY drivers which know the PHY device does not support C45 over C22 2: Think about naming. At some point we are going to add the generic helpers for accessing C45 registers which leave the core to decide if to perform a C45 bus protocol access or C45 over C22. Those generic helpers need to have the natural name for accessing a C45 register since 99% of drivers will be using them. The helpers you add now need to use a less common name. Andrew
Hi, On Tue, 16 Apr 2024 14:38:11 +0200 Andrew Lunn <andrew@lunn.ch> wrote: >> 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. > > Now i've read more of the patches, i can see that the MDIO bus master > is C45 only. At least, that is all that is implemented in the > driver. So for this combination of MAC and PHY, forcing C45 register > access using C45 bus protocol will work. Thanks a lot! > However, can you combine this PHY with some other MDIO bus master, > which does not support C45? Then C45 over C22 would need to be used? > Looking at the data sheet i found online, there is no suggestion it > does support C22 bus protocol. All the diagrams/tables only show C45 > bus protocol. qt2025_ds3014.pdf? > So this PHY is a special case. So you can use wrapper methods which > force C45 bus protocol, and ignore phylib support for performing C45 > over C22 when needed. However, while doing this: > > 1: Clearly document that these helpers are not generic, they force C45 > register access using C45 bus protocol, and should only by used PHY > drivers which know the PHY device does not support C45 over C22 > > 2: Think about naming. At some point we are going to add the generic > helpers for accessing C45 registers which leave the core to decide > if to perform a C45 bus protocol access or C45 over C22. Those > generic helpers need to have the natural name for accessing a C45 > register since 99% of drivers will be using them. The helpers you > add now need to use a less common name. Sounds like we should save the names of c45_read and c45_write. read_with_c45_bus_protocol and write_with_c45_bus_protocol? They call mdiobus_c45_*, right?
On 16.04.24 15:21, FUJITA Tomonori wrote: > Hi, > > On Tue, 16 Apr 2024 14:38:11 +0200 > Andrew Lunn <andrew@lunn.ch> wrote: > >>> 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. >> >> Now i've read more of the patches, i can see that the MDIO bus master >> is C45 only. At least, that is all that is implemented in the >> driver. So for this combination of MAC and PHY, forcing C45 register >> access using C45 bus protocol will work. > > Thanks a lot! > >> However, can you combine this PHY with some other MDIO bus master, >> which does not support C45? Then C45 over C22 would need to be used? >> Looking at the data sheet i found online, there is no suggestion it >> does support C22 bus protocol. All the diagrams/tables only show C45 >> bus protocol. > > qt2025_ds3014.pdf? > >> So this PHY is a special case. So you can use wrapper methods which >> force C45 bus protocol, and ignore phylib support for performing C45 >> over C22 when needed. However, while doing this: >> >> 1: Clearly document that these helpers are not generic, they force C45 >> register access using C45 bus protocol, and should only by used PHY >> drivers which know the PHY device does not support C45 over C22 >> >> 2: Think about naming. At some point we are going to add the generic >> helpers for accessing C45 registers which leave the core to decide >> if to perform a C45 bus protocol access or C45 over C22. Those >> generic helpers need to have the natural name for accessing a C45 >> register since 99% of drivers will be using them. The helpers you >> add now need to use a less common name. > > Sounds like we should save the names of c45_read and c45_write. > > read_with_c45_bus_protocol and write_with_c45_bus_protocol? > > They call mdiobus_c45_*, right? I think we could also do a more rusty solution. For example we could have these generic functions for `phy::Device`: fn read_register<R: Register>(&mut self, which: R::Index) -> Result<R::Value>; fn write_register<R: Register>(&mut self, which: R::Index, value: R::Value) -> Result; That way we can support many different registers without polluting the namespace. We can then have a `C45` register and a `C22` register and a `C45OrC22` (maybe we should use `C45_OrC22` instead, since I can read that better, but let's bikeshed when we have the actual patch). Calling those functions would look like this: let value = dev.read_register::<C45>(reg1)?; dev.write_register::<C45>(reg2, value)?;
> I think we could also do a more rusty solution. For example we could > have these generic functions for `phy::Device`: > > fn read_register<R: Register>(&mut self, which: R::Index) -> Result<R::Value>; > fn write_register<R: Register>(&mut self, which: R::Index, value: R::Value) -> Result; > > That way we can support many different registers without polluting the > namespace. We can then have a `C45` register and a `C22` register and a > `C45OrC22` (maybe we should use `C45_OrC22` instead, since I can read > that better, but let's bikeshed when we have the actual patch). > > Calling those functions would look like this: > > let value = dev.read_register::<C45>(reg1)?; > dev.write_register::<C45>(reg2, value)?; I don't know how well that will work out in practice. The addressing schemes for C22 and C45 are different. C22 simply has 32 registers, numbered 0-31. C45 has 32 MDIO manageable devices (MMD) each with 65536 registers. All of the 32 C22 registers have well defined names, which are listed in mii.h. Ideally we want to keep the same names. The most of the MMD also have defined names, listed in mdio.h. Many of the registers are also named in mdio.h, although vendors do tend to add more vendor proprietary registers. Your R::Index would need to be a single value for C22 and a tuple of two values for C45. There is nothing like `C45OrC22`. A register is either in C22 namespace, or in the C45 namespace. Where it gets interesting is that there are two methods to access the C45 register namespace. The PHY driver should not care about this, it is the MDIO bus driver and the PHYLIB core which handles the two access mechanisms. Andrew
On 17.04.24 00:30, Andrew Lunn wrote: >> I think we could also do a more rusty solution. For example we could >> have these generic functions for `phy::Device`: >> >> fn read_register<R: Register>(&mut self, which: R::Index) -> Result<R::Value>; >> fn write_register<R: Register>(&mut self, which: R::Index, value: R::Value) -> Result; >> >> That way we can support many different registers without polluting the >> namespace. We can then have a `C45` register and a `C22` register and a >> `C45OrC22` (maybe we should use `C45_OrC22` instead, since I can read >> that better, but let's bikeshed when we have the actual patch). >> >> Calling those functions would look like this: >> >> let value = dev.read_register::<C45>(reg1)?; >> dev.write_register::<C45>(reg2, value)?; > > I don't know how well that will work out in practice. The addressing > schemes for C22 and C45 are different. > > C22 simply has 32 registers, numbered 0-31. > > C45 has 32 MDIO manageable devices (MMD) each with 65536 registers. > > All of the 32 C22 registers have well defined names, which are listed > in mii.h. Ideally we want to keep the same names. The most of the MMD > also have defined names, listed in mdio.h. Many of the registers are > also named in mdio.h, although vendors do tend to add more vendor > proprietary registers. > > Your R::Index would need to be a single value for C22 and a tuple of > two values for C45. Yes that was my idea: enum C22Index { // The unique 32 names of the C22 registers... } impl Register for C22 { type Index = C22Index; type Value = u16; } impl Register for C45 { type Index = (u8, u16); // We could also create a newtype that wraps this and give it a better name. type Value = u16; } You can then do: let val = dev.read_register::<C45>((4, 406))?; dev.write_register::<C22>(4, val)?; If you only have these two registers types, then I would say this is overkill, but I assumed that there are more. > There is nothing like `C45OrC22`. A register is either in C22 > namespace, or in the C45 namespace. I see, I got this idea from: > [...] At some point we are going to add the generic > helpers for accessing C45 registers which leave the core to decide > if to perform a C45 bus protocol access or C45 over C22. [...] Is this not accessing a C45 register via C22 and letting phylib decide? > Where it gets interesting is that there are two methods to access the > C45 register namespace. The PHY driver should not care about this, it > is the MDIO bus driver and the PHYLIB core which handles the two > access mechanisms. If the driver shouldn't be concerned with how the access gets handled, why do we even have a naming problem?
> If the driver shouldn't be concerned with how the access gets handled, > why do we even have a naming problem? History. The current C code does not cleanly separate register spaces from access mechanisms. C22 register space is simple, you can only access it using C22 bus protocol. However C45 register space can be accessed in two ways, either using C45 bus protocol, or using C45 over C22. The driver should not care, it just wants to read/write a C45 register. But the current core mixes the two concepts of C45 register space and access mechanisms. There have been a few attempts to clean this up, but nothing landed yet. Now this driver is somewhat special. The PHY itself only implements one of the two access mechanisms, C45 bus protocol. So this driver could side-step this mess and define access functions which go straight to C45 bus protocol. However, some day a non-special device/driver will come along, and we will need the generic access functions, which leave the core to decide on C45 bus protocol or C45 over C22. Ideally these generic functions should have the natural name for accessing C45 registers, and the special case in this driver should use a different name. Andrew
On 17.04.24 15:34, Andrew Lunn wrote: >> If the driver shouldn't be concerned with how the access gets handled, >> why do we even have a naming problem? > > History. > > The current C code does not cleanly separate register spaces from > access mechanisms. > > C22 register space is simple, you can only access it using C22 bus > protocol. However C45 register space can be accessed in two ways, > either using C45 bus protocol, or using C45 over C22. The driver > should not care, it just wants to read/write a C45 register. But the > current core mixes the two concepts of C45 register space and access > mechanisms. There have been a few attempts to clean this up, but > nothing landed yet. > > Now this driver is somewhat special. The PHY itself only implements > one of the two access mechanisms, C45 bus protocol. So this driver > could side-step this mess and define access functions which go > straight to C45 bus protocol. However, some day a non-special > device/driver will come along, and we will need the generic access > functions, which leave the core to decide on C45 bus protocol or C45 > over C22. Ideally these generic functions should have the natural name > for accessing C45 registers, and the special case in this driver > should use a different name. Thanks for the explanation. What about having the following register representing types: - `C22` accesses a C22 register - `C45` accesses a C45 register using whatever method phylib decides - `C45Bus` accesses a C45 register over the C45 bus protocol (or `C45Direct`) Or are you opposed to the idea of accessing any type of register via `dev.read_register::<RegType>(..)`?
On Wed, 17 Apr 2024 08:20:25 +0000 Benno Lossin <benno.lossin@proton.me> wrote: > On 17.04.24 00:30, Andrew Lunn wrote: >>> I think we could also do a more rusty solution. For example we could >>> have these generic functions for `phy::Device`: >>> >>> fn read_register<R: Register>(&mut self, which: R::Index) -> Result<R::Value>; >>> fn write_register<R: Register>(&mut self, which: R::Index, value: R::Value) -> Result; >>> >>> That way we can support many different registers without polluting the >>> namespace. We can then have a `C45` register and a `C22` register and a >>> `C45OrC22` (maybe we should use `C45_OrC22` instead, since I can read >>> that better, but let's bikeshed when we have the actual patch). >>> >>> Calling those functions would look like this: >>> >>> let value = dev.read_register::<C45>(reg1)?; >>> dev.write_register::<C45>(reg2, value)?; >> >> I don't know how well that will work out in practice. The addressing >> schemes for C22 and C45 are different. >> >> C22 simply has 32 registers, numbered 0-31. >> >> C45 has 32 MDIO manageable devices (MMD) each with 65536 registers. >> >> All of the 32 C22 registers have well defined names, which are listed >> in mii.h. Ideally we want to keep the same names. The most of the MMD >> also have defined names, listed in mdio.h. Many of the registers are >> also named in mdio.h, although vendors do tend to add more vendor >> proprietary registers. >> >> Your R::Index would need to be a single value for C22 and a tuple of >> two values for C45. > > Yes that was my idea: > > enum C22Index { > // The unique 32 names of the C22 registers... > } > > impl Register for C22 { > type Index = C22Index; > type Value = u16; > } > > impl Register for C45 { > type Index = (u8, u16); // We could also create a newtype that wraps this and give it a better name. > type Value = u16; > } C45 looks a bit odd to me because C45 has a member which isn't a register. It's a value to specify which MMD you access to.
> Thanks for the explanation. What about having the following register > representing types: > - `C22` accesses a C22 register > - `C45` accesses a C45 register using whatever method phylib decides > - `C45Bus` accesses a C45 register over the C45 bus protocol (or > `C45Direct`) > > Or are you opposed to the idea of accessing any type of register via > `dev.read_register::<RegType>(..)`? I'm not against this, it is good to use Rust features if they make the code easier to understand. We can bike shed C45Bus vs C45Direct vs ... . I would probably go for C45Direct, because C45 over C22 is often called indirect. Andrew
Hi, Sorry about the long delay, On Mon, 15 Apr 2024 23:25:22 -0400 Trevor Gross <tmgross@umich.edu> wrote: > On Mon, Apr 15, 2024 at 6:47 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> 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. > > Depending on the response to Andrew's notes, these SAFETY comments > will probably need to be updated to say why we know C45 is supported. If a driver uses only-c45 API (RegC45Direct in the new patch), it returns an error. We need to write something in SAFETY? >> + let ret = unsafe { >> + bindings::mdiobus_c45_read( >> + (*phydev).mdio.bus, >> + (*phydev).mdio.addr, >> + devad as i32, > > This could probably also be from/.into() Fixed. >> + regnum.into(), >> + ) >> + }; >> + if ret < 0 { >> + Err(Error::from_errno(ret)) >> + } else { >> + Ok(ret as u16) >> + } > > Could this be simplified with to_result? to_result returns Ok(()). So I think that we need something to_result_with_value(). I think that we discussed something like that ago: https://lore.kernel.org/all/CANiq72=VaNtZ_B0ji94xFftb4Pctfep+4c2cNWkmCcKhpZY2kQ@mail.gmail.com/ A new function was introduced, which I missed? >> + } >> + >> + /// Writes a given C45 PHY register. >> + pub fn c45_write(&mut self, devad: u8, 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. >> + to_result(unsafe { >> + bindings::mdiobus_c45_write( >> + (*phydev).mdio.bus, >> + (*phydev).mdio.addr, >> + devad as i32, > > Same as above Fixed.
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index 44b59bbf1a52..421a231421f5 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -203,6 +203,43 @@ pub fn write(&mut self, regnum: u16, val: u16) -> Result { }) } + /// 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(), + ) + }; + if ret < 0 { + Err(Error::from_errno(ret)) + } else { + Ok(ret as u16) + } + } + + /// Writes a given C45 PHY register. + pub fn c45_write(&mut self, devad: u8, 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. + to_result(unsafe { + bindings::mdiobus_c45_write( + (*phydev).mdio.bus, + (*phydev).mdio.addr, + devad as i32, + regnum.into(), + val, + ) + }) + } + /// Reads a paged register. pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> { let phydev = self.0.get(); @@ -277,6 +314,19 @@ pub fn genphy_read_status(&mut self) -> Result<u16> { } } + /// Checks the link status and updates current link state via C45 registers. + pub fn genphy_c45_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_c45_read_status(phydev) }; + if ret < 0 { + Err(Error::from_errno(ret)) + } else { + Ok(ret as u16) + } + } + /// Updates the link status. pub fn genphy_update_link(&mut self) -> Result { let phydev = self.0.get();
This patch adds the following helper functions for Clause 45: - mdiobus_c45_read - mdiobus_c45_write - genphy_c45_read_status Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/kernel/net/phy.rs | 50 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)