| Message ID | 20240201-rockchip-rust-phy_depend-v2-3-c5fa4faab924@christina-quast.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Rust Rockchip PHY driver | expand |
On Thu, Feb 01, 2024 at 07:07:00PM +0100, Christina Quast wrote: > This is the Rust implementation of drivers/net/phy/rockchip.c. The > features are equivalent. You can choose C or Rust version kernel > configuration. > > Signed-off-by: Christina Quast <contact@christina-quast.de> Cool, but why? Is this going to happen for all phy drivers going forward? What's the end-game here, dropping all .c phy drivers that are in rust? Or having duplicates for all of them? thanks, greg k-h
On 2024-02-01 19:23, Greg KH wrote: > On Thu, Feb 01, 2024 at 07:07:00PM +0100, Christina Quast wrote: >> This is the Rust implementation of drivers/net/phy/rockchip.c. The >> features are equivalent. You can choose C or Rust version kernel >> configuration. >> >> Signed-off-by: Christina Quast <contact@christina-quast.de> > > Cool, but why? Is this going to happen for all phy drivers going > forward? What's the end-game here, dropping all .c phy drivers that > are in rust? Or having duplicates for all of them? I'd also like to know what's the intended purpose of rewriting a mature existing PHY driver in Rust?
On Thu, Feb 1, 2024 at 12:07 PM Christina Quast <contact@christina-quast.de> wrote: > +++ b/drivers/net/phy/rockchip_rust.rs > @@ -0,0 +1,131 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2024 Christina Quast <contact@christina-quast.de> > + > +//! Rust Rockchip PHY driver > +//! > +//! C version of this driver: [`drivers/net/phy/rockchip.c`](./rockchip.c) > +use kernel::{ > + c_str, > + net::phy::{self, DeviceId, Driver}, > + prelude::*, > + uapi, > +}; > + > +kernel::module_phy_driver! { > + drivers: [PhyRockchip], > + device_table: [ > + DeviceId::new_with_driver::<PhyRockchip>(), > + ], > + name: "rust_asix_phy", > + author: "FUJITA Tomonori <fujita.tomonori@gmail.com>", Tomo wrote this? :) > + description: "Rust Asix PHYs driver", > + license: "GPL", > +} > + > + > +const MII_INTERNAL_CTRL_STATUS: u16 = 17; > +const SMI_ADDR_TSTCNTL: u16 = 20; > +const SMI_ADDR_TSTWRITE: u16 = 23; > + > +const MII_AUTO_MDIX_EN: u16 = bit(7); > +const MII_MDIX_EN: u16 = bit(6); > + > +const TSTCNTL_WR: u16 = bit(14) | bit(10); > + > +const TSTMODE_ENABLE: u16 = 0x400; > +const TSTMODE_DISABLE: u16 = 0x0; > + > +const WR_ADDR_A7CFG: u16 = 0x18; Most of these are clear enough, but could you add comments about what the more ambiguous constants are for? (e.g. A7CFG). > +struct PhyRockchip; > + > +impl PhyRockchip { Remove the `helper_` prefix for these functions, and change the docs. Their use as helpers is obvious enough based on where they are called, better to say what they actually accomplish. Since they don't take `self`, these could also just be standalone functions rather than in an `impl PhyRockchip` block. This makes calling them a bit cleaner since you don't need the `PhyRockchip::` prefix. > + /// Helper function for helper_integrated_phy_analog_init > + fn helper_init_tstmode(dev: &mut phy::Device) -> Result { > + // Enable access to Analog and DSP register banks > + dev.write(SMI_ADDR_TSTCNTL, TSTMODE_ENABLE)?; > + dev.write(SMI_ADDR_TSTCNTL, TSTMODE_DISABLE)?; > + dev.write(SMI_ADDR_TSTCNTL, TSTMODE_ENABLE) > + } For consistency, just make the last write also end with `?;` and add a `Ok(())` line. > + > + /// Helper function for helper_integrated_phy_analog_init > + fn helper_close_tstmode(dev: &mut phy::Device) -> Result { > + dev.write(SMI_ADDR_TSTCNTL, TSTMODE_DISABLE) > + } > + > + /// Helper function for rockchip_config_init > + fn helper_integrated_phy_analog_init(dev: &mut phy::Device) -> Result { > + Self::helper_init_tstmode(dev)?; > + dev.write(SMI_ADDR_TSTWRITE, 0xB)?; > + dev.write(SMI_ADDR_TSTCNTL, TSTCNTL_WR | WR_ADDR_A7CFG)?; > + Self::helper_close_tstmode(dev) > + } > + > + /// Helper function for config_init > + fn helper_config_init(dev: &mut phy::Device) -> Result { > + let val = !MII_AUTO_MDIX_EN & dev.read(MII_INTERNAL_CTRL_STATUS)?; > + dev.write(MII_INTERNAL_CTRL_STATUS, val)?; > + Self::helper_integrated_phy_analog_init(dev) > + } > + > + fn helper_set_polarity(dev: &mut phy::Device, polarity: u8) -> Result { > + let reg = !MII_AUTO_MDIX_EN & dev.read(MII_INTERNAL_CTRL_STATUS)?; > + let val = match polarity as u32 { > + // status: MDI; control: force MDI > + uapi::ETH_TP_MDI => Some(reg & !MII_MDIX_EN), > + // status: MDI-X; control: force MDI-X > + uapi::ETH_TP_MDI_X => Some(reg | MII_MDIX_EN), > + // uapi::ETH_TP_MDI_AUTO => control: auto-select > + // uapi::ETH_TP_MDI_INVALID => status: unknown; control: unsupported > + _ => None, Is receiving an invalid value not an error? I.e. uapi::ETH_TP_MDI_AUTO | uapi::ETH_TP_MDI_INVALID => None, _ => return Err(...) I know the current implementation came from the C version, just wondering about correctness here. > + }; > + if let Some(v) = val { > + if v != reg { > + return dev.write(MII_INTERNAL_CTRL_STATUS, v); > + } > + } In the match statement above - I think you can replace `=> None` with `=> return Ok(())` and drop the `Some(...)` wrappers. Then you don't need to destructure val here. > + Ok(()) > + > + } > +} > + > +#[vtable] > +impl Driver for PhyRockchip { > + const FLAGS: u32 = 0; > + const NAME: &'static CStr = c_str!("Rockchip integrated EPHY"); > + const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0x1234d400, 0xfffffff0); > + > + fn link_change_notify(dev: &mut phy::Device) { > + // If mode switch happens from 10BT to 100BT, all DSP/AFE > + // registers are set to default values. So any AFE/DSP > + // registers have to be re-initialized in this case. Comment indent > + if dev.state() == phy::DeviceState::Running && dev.speed() == uapi::SPEED_100 { > + if let Err(e) = Self::helper_integrated_phy_analog_init(dev) { > + pr_err!("rockchip: integrated_phy_analog_init err: {:?}", e); > + } > + } > + } > + > + fn soft_reset(dev: &mut phy::Device) -> Result { > + dev.genphy_soft_reset() > + } > + > + fn config_init(dev: &mut phy::Device) -> Result { > + PhyRockchip::helper_config_init(dev) > + } > + > + fn config_aneg(dev: &mut phy::Device) -> Result { > + PhyRockchip::helper_set_polarity(dev, dev.mdix())?; > + dev.genphy_config_aneg() > + } > + > + fn suspend(dev: &mut phy::Device) -> Result { > + dev.genphy_suspend() > + } > + > + fn resume(dev: &mut phy::Device) -> Result { > + let _ = dev.genphy_resume(); Why not `?` the possible error? > + > + PhyRockchip::helper_config_init(dev) > + } > +} > > -- > 2.43.0 > As Greg and Dragan mentioned, duplicate drivers are generally not accepted in-tree to avoid double maintenance and confusing config. Is there a specific goal? It is quite alright to request feedback on Rust drivers (and I have provided some) or even ask if anyone is willing to help test it out, but please use RFC PATCH and make it clear that this is for experimentation rather than upstreaming. Netdev has seemed relatively open to adding Rust drivers for new phys that don't have a C implementation, but these phys are of course tougher to find. Also for future reference, changes intended for the net tree should be labeled [PATCH v? net-next]. Best regards, Trevor
On Thu, Feb 01, 2024 at 10:23:03AM -0800, Greg KH wrote: > On Thu, Feb 01, 2024 at 07:07:00PM +0100, Christina Quast wrote: > > This is the Rust implementation of drivers/net/phy/rockchip.c. The > > features are equivalent. You can choose C or Rust version kernel > > configuration. > > > > Signed-off-by: Christina Quast <contact@christina-quast.de> > Cool, but why? Is this going to happen for all phy drivers going > forward? What's the end-game here, dropping all .c phy drivers that are > in rust? Or having duplicates for all of them? As one of the PHY Maintainers, i would say no. Now we have an example, i think we should be a lot more strict about what we actually merge. It should be a driver for hardware which does not have a C driver. We cannot drop C drivers since Rust at the moment does not support all architectures GCC/Clang does. PHY drivers are architecture independent, and in real life used on multiple architectures. When Rust eventually catches up, we could consider dropping C drivers when there is an equivalent Rust driver, but from what i hear, that is a few years away. I don't want to be supporting a C and Rust driver for the same hardware. Andrew
On Thu, Feb 1, 2024 at 10:30 PM Andrew Lunn <andrew@lunn.ch> wrote: > > As one of the PHY Maintainers, i would say no. > > Now we have an example, i think we should be a lot more strict about > what we actually merge. It should be a driver for hardware which does > not have a C driver. Yeah, a single "Rust reference driver" is likely enough to give a good example of how things would look. I guess more than one could be justified if there are significant differences, e.g. if the maintainers want to cover more of the abstractions API for some reason. > We cannot drop C drivers since Rust at the moment does not support all > architectures GCC/Clang does. PHY drivers are architecture > independent, and in real life used on multiple architectures. When > Rust eventually catches up, we could consider dropping C drivers when > there is an equivalent Rust driver, but from what i hear, that is a > few years away. I don't want to be supporting a C and Rust driver for > the same hardware. The `rustc_codegen_gcc` backend can already build the kernel without changes, so hopefully we will see some results sooner than that. If we are talking multiple years, GCC Rust likely enters the equation too. But, indeed, there is a lot of work to do until we can drop C code in general. Cheers, Miguel
Hi Trevor! Thanks a lot for your review, I learned a lot! I felt, from the feedback in the Zulip forum that rewriting more phy drivers might be interesting, but I think I misunderstood something. There is no specific goal behind the rewrite, I just thought it would be useful to bring more Rust into the Kernel. Cheers, Christina On 2/1/24 22:06, Trevor Gross wrote: > On Thu, Feb 1, 2024 at 12:07 PM Christina Quast > <contact@christina-quast.de> wrote: >> +++ b/drivers/net/phy/rockchip_rust.rs >> @@ -0,0 +1,131 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (C) 2024 Christina Quast <contact@christina-quast.de> >> + >> +//! Rust Rockchip PHY driver >> +//! >> +//! C version of this driver: [`drivers/net/phy/rockchip.c`](./rockchip.c) >> +use kernel::{ >> + c_str, >> + net::phy::{self, DeviceId, Driver}, >> + prelude::*, >> + uapi, >> +}; >> + >> +kernel::module_phy_driver! { >> + drivers: [PhyRockchip], >> + device_table: [ >> + DeviceId::new_with_driver::<PhyRockchip>(), >> + ], >> + name: "rust_asix_phy", >> + author: "FUJITA Tomonori <fujita.tomonori@gmail.com>", > Tomo wrote this? :) > >> + description: "Rust Asix PHYs driver", >> + license: "GPL", >> +} >> + >> + >> +const MII_INTERNAL_CTRL_STATUS: u16 = 17; >> +const SMI_ADDR_TSTCNTL: u16 = 20; >> +const SMI_ADDR_TSTWRITE: u16 = 23; >> + >> +const MII_AUTO_MDIX_EN: u16 = bit(7); >> +const MII_MDIX_EN: u16 = bit(6); >> + >> +const TSTCNTL_WR: u16 = bit(14) | bit(10); >> + >> +const TSTMODE_ENABLE: u16 = 0x400; >> +const TSTMODE_DISABLE: u16 = 0x0; >> + >> +const WR_ADDR_A7CFG: u16 = 0x18; > Most of these are clear enough, but could you add comments about what > the more ambiguous constants are for? (e.g. A7CFG). > >> +struct PhyRockchip; >> + >> +impl PhyRockchip { > Remove the `helper_` prefix for these functions, and change the docs. > Their use as helpers is obvious enough based on where they are called, > better to say what they actually accomplish. > > Since they don't take `self`, these could also just be standalone > functions rather than in an `impl PhyRockchip` block. This makes > calling them a bit cleaner since you don't need the `PhyRockchip::` > prefix. > >> + /// Helper function for helper_integrated_phy_analog_init >> + fn helper_init_tstmode(dev: &mut phy::Device) -> Result { >> + // Enable access to Analog and DSP register banks >> + dev.write(SMI_ADDR_TSTCNTL, TSTMODE_ENABLE)?; >> + dev.write(SMI_ADDR_TSTCNTL, TSTMODE_DISABLE)?; >> + dev.write(SMI_ADDR_TSTCNTL, TSTMODE_ENABLE) >> + } > For consistency, just make the last write also end with `?;` and add a > `Ok(())` line. > >> + >> + /// Helper function for helper_integrated_phy_analog_init >> + fn helper_close_tstmode(dev: &mut phy::Device) -> Result { >> + dev.write(SMI_ADDR_TSTCNTL, TSTMODE_DISABLE) >> + } >> + >> + /// Helper function for rockchip_config_init >> + fn helper_integrated_phy_analog_init(dev: &mut phy::Device) -> Result { >> + Self::helper_init_tstmode(dev)?; >> + dev.write(SMI_ADDR_TSTWRITE, 0xB)?; >> + dev.write(SMI_ADDR_TSTCNTL, TSTCNTL_WR | WR_ADDR_A7CFG)?; >> + Self::helper_close_tstmode(dev) >> + } >> + >> + /// Helper function for config_init >> + fn helper_config_init(dev: &mut phy::Device) -> Result { >> + let val = !MII_AUTO_MDIX_EN & dev.read(MII_INTERNAL_CTRL_STATUS)?; >> + dev.write(MII_INTERNAL_CTRL_STATUS, val)?; >> + Self::helper_integrated_phy_analog_init(dev) >> + } >> + >> + fn helper_set_polarity(dev: &mut phy::Device, polarity: u8) -> Result { >> + let reg = !MII_AUTO_MDIX_EN & dev.read(MII_INTERNAL_CTRL_STATUS)?; >> + let val = match polarity as u32 { >> + // status: MDI; control: force MDI >> + uapi::ETH_TP_MDI => Some(reg & !MII_MDIX_EN), >> + // status: MDI-X; control: force MDI-X >> + uapi::ETH_TP_MDI_X => Some(reg | MII_MDIX_EN), >> + // uapi::ETH_TP_MDI_AUTO => control: auto-select >> + // uapi::ETH_TP_MDI_INVALID => status: unknown; control: unsupported >> + _ => None, > Is receiving an invalid value not an error? I.e. > > uapi::ETH_TP_MDI_AUTO | uapi::ETH_TP_MDI_INVALID => None, > _ => return Err(...) > > I know the current implementation came from the C version, just > wondering about correctness here. > >> + }; >> + if let Some(v) = val { >> + if v != reg { >> + return dev.write(MII_INTERNAL_CTRL_STATUS, v); >> + } >> + } > In the match statement above - I think you can replace `=> None` with > `=> return Ok(())` and drop the `Some(...)` wrappers. Then you don't > need to destructure val here. > >> + Ok(()) >> + >> + } >> +} >> + >> +#[vtable] >> +impl Driver for PhyRockchip { >> + const FLAGS: u32 = 0; >> + const NAME: &'static CStr = c_str!("Rockchip integrated EPHY"); >> + const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0x1234d400, 0xfffffff0); >> + >> + fn link_change_notify(dev: &mut phy::Device) { >> + // If mode switch happens from 10BT to 100BT, all DSP/AFE >> + // registers are set to default values. So any AFE/DSP >> + // registers have to be re-initialized in this case. > Comment indent > >> + if dev.state() == phy::DeviceState::Running && dev.speed() == uapi::SPEED_100 { >> + if let Err(e) = Self::helper_integrated_phy_analog_init(dev) { >> + pr_err!("rockchip: integrated_phy_analog_init err: {:?}", e); >> + } >> + } >> + } >> + >> + fn soft_reset(dev: &mut phy::Device) -> Result { >> + dev.genphy_soft_reset() >> + } >> + >> + fn config_init(dev: &mut phy::Device) -> Result { >> + PhyRockchip::helper_config_init(dev) >> + } >> + >> + fn config_aneg(dev: &mut phy::Device) -> Result { >> + PhyRockchip::helper_set_polarity(dev, dev.mdix())?; >> + dev.genphy_config_aneg() >> + } >> + >> + fn suspend(dev: &mut phy::Device) -> Result { >> + dev.genphy_suspend() >> + } >> + >> + fn resume(dev: &mut phy::Device) -> Result { >> + let _ = dev.genphy_resume(); > Why not `?` the possible error? > >> + >> + PhyRockchip::helper_config_init(dev) >> + } >> +} >> >> -- >> 2.43.0 >> > As Greg and Dragan mentioned, duplicate drivers are generally not > accepted in-tree to avoid double maintenance and confusing config. Is > there a specific goal? > > It is quite alright to request feedback on Rust drivers (and I have > provided some) or even ask if anyone is willing to help test it out, > but please use RFC PATCH and make it clear that this is for > experimentation rather than upstreaming. > > Netdev has seemed relatively open to adding Rust drivers for new phys > that don't have a C implementation, but these phys are of course > tougher to find. > > Also for future reference, changes intended for the net tree should be > labeled [PATCH v? net-next]. > > Best regards, > Trevor
On Tue, Feb 6, 2024 at 2:20 PM Christina Quast <chrysh@christina-quast.de> wrote: > > Hi Trevor! > > Thanks a lot for your review, I learned a lot! I felt, from the feedback > in the Zulip forum that rewriting more phy drivers might be interesting, > but I think I misunderstood something. > > There is no specific goal behind the rewrite, I just thought it would be > useful to bring more Rust into the Kernel. > > Cheers, > > Christina Happy to help :) There is definitely no harm in experimenting, but the general (reasonable) rule is that there shouldn't be duplicate drivers in-tree, even across languages. What you probably saw on Zulip is that we were trying to locate newer phys that don't already have a C driver. Shashwat reached out to a few companies and mentioned that Microchip was interested in drivers for some of their VSC84xx/82xx families. They are a bit more complicated (MACSec, XFI/SFP+) and apparently somewhat difficult to get hardware for, but that might be an option if you are interested in working on something like this. Andrew might have some ideas too. There are a lot of new phys coming out since MACsec is getting more popular, also some newer specs like 10GBase-T1 and 10Base-T1S. - Trevor
> Andrew might have some ideas too. There are a lot of new phys coming > out since MACsec is getting more popular, also some newer specs like > 10GBase-T1 and 10Base-T1S. Those can also be hard to get hold of, unless a vendor is interested. Another source to look at might be OpenWRT and DD-WRT. These projects are not always good at upstreaming. They sometimes just use the vendor crap drivers. See if they have any PHY drivers for devices not in mainline. Any they do have should be for COTS hardware you probably can buy off the shelf. Andrew
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 9e2672800f0b..8b73edb7e836 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -362,6 +362,14 @@ config ROCKCHIP_PHY help Currently supports the integrated Ethernet PHY. +config ROCKCHIP_RUST_PHY + bool "Rust driver for Rockchip Ethernet PHYs" + depends on RUST_PHYLIB_ABSTRACTIONS && ROCKCHIP_PHY + help + Uses the Rust reference driver for Rockchip PHYs (rockchip_rust.ko). + The features are equivalent. It supports the integrated Ethernet PHY. + + config SMSC_PHY tristate "SMSC PHYs" select CRC16 diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index 6097afd44392..045d2913bf2e 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -94,7 +94,11 @@ obj-$(CONFIG_NXP_TJA11XX_PHY) += nxp-tja11xx.o obj-$(CONFIG_QSEMI_PHY) += qsemi.o obj-$(CONFIG_REALTEK_PHY) += realtek.o obj-$(CONFIG_RENESAS_PHY) += uPD60620.o +ifdef CONFIG_ROCKCHIP_RUST_PHY +obj-$(CONFIG_ROCKCHIP_PHY) += rockchip_rust.o +else obj-$(CONFIG_ROCKCHIP_PHY) += rockchip.o +endif obj-$(CONFIG_SMSC_PHY) += smsc.o obj-$(CONFIG_STE10XP) += ste10Xp.o obj-$(CONFIG_TERANETICS_PHY) += teranetics.o diff --git a/drivers/net/phy/rockchip_rust.rs b/drivers/net/phy/rockchip_rust.rs new file mode 100644 index 000000000000..17a1f94da8c1 --- /dev/null +++ b/drivers/net/phy/rockchip_rust.rs @@ -0,0 +1,131 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2024 Christina Quast <contact@christina-quast.de> + +//! Rust Rockchip PHY driver +//! +//! C version of this driver: [`drivers/net/phy/rockchip.c`](./rockchip.c) +use kernel::{ + c_str, + net::phy::{self, DeviceId, Driver}, + prelude::*, + uapi, +}; + +kernel::module_phy_driver! { + drivers: [PhyRockchip], + device_table: [ + DeviceId::new_with_driver::<PhyRockchip>(), + ], + name: "rust_asix_phy", + author: "FUJITA Tomonori <fujita.tomonori@gmail.com>", + description: "Rust Asix PHYs driver", + license: "GPL", +} + + +const MII_INTERNAL_CTRL_STATUS: u16 = 17; +const SMI_ADDR_TSTCNTL: u16 = 20; +const SMI_ADDR_TSTWRITE: u16 = 23; + +const MII_AUTO_MDIX_EN: u16 = bit(7); +const MII_MDIX_EN: u16 = bit(6); + +const TSTCNTL_WR: u16 = bit(14) | bit(10); + +const TSTMODE_ENABLE: u16 = 0x400; +const TSTMODE_DISABLE: u16 = 0x0; + +const WR_ADDR_A7CFG: u16 = 0x18; + +struct PhyRockchip; + +impl PhyRockchip { + /// Helper function for helper_integrated_phy_analog_init + fn helper_init_tstmode(dev: &mut phy::Device) -> Result { + // Enable access to Analog and DSP register banks + dev.write(SMI_ADDR_TSTCNTL, TSTMODE_ENABLE)?; + dev.write(SMI_ADDR_TSTCNTL, TSTMODE_DISABLE)?; + dev.write(SMI_ADDR_TSTCNTL, TSTMODE_ENABLE) + } + + /// Helper function for helper_integrated_phy_analog_init + fn helper_close_tstmode(dev: &mut phy::Device) -> Result { + dev.write(SMI_ADDR_TSTCNTL, TSTMODE_DISABLE) + } + + /// Helper function for rockchip_config_init + fn helper_integrated_phy_analog_init(dev: &mut phy::Device) -> Result { + Self::helper_init_tstmode(dev)?; + dev.write(SMI_ADDR_TSTWRITE, 0xB)?; + dev.write(SMI_ADDR_TSTCNTL, TSTCNTL_WR | WR_ADDR_A7CFG)?; + Self::helper_close_tstmode(dev) + } + + /// Helper function for config_init + fn helper_config_init(dev: &mut phy::Device) -> Result { + let val = !MII_AUTO_MDIX_EN & dev.read(MII_INTERNAL_CTRL_STATUS)?; + dev.write(MII_INTERNAL_CTRL_STATUS, val)?; + Self::helper_integrated_phy_analog_init(dev) + } + + fn helper_set_polarity(dev: &mut phy::Device, polarity: u8) -> Result { + let reg = !MII_AUTO_MDIX_EN & dev.read(MII_INTERNAL_CTRL_STATUS)?; + let val = match polarity as u32 { + // status: MDI; control: force MDI + uapi::ETH_TP_MDI => Some(reg & !MII_MDIX_EN), + // status: MDI-X; control: force MDI-X + uapi::ETH_TP_MDI_X => Some(reg | MII_MDIX_EN), + // uapi::ETH_TP_MDI_AUTO => control: auto-select + // uapi::ETH_TP_MDI_INVALID => status: unknown; control: unsupported + _ => None, + }; + if let Some(v) = val { + if v != reg { + return dev.write(MII_INTERNAL_CTRL_STATUS, v); + } + } + Ok(()) + + } +} + +#[vtable] +impl Driver for PhyRockchip { + const FLAGS: u32 = 0; + const NAME: &'static CStr = c_str!("Rockchip integrated EPHY"); + const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0x1234d400, 0xfffffff0); + + fn link_change_notify(dev: &mut phy::Device) { + // If mode switch happens from 10BT to 100BT, all DSP/AFE + // registers are set to default values. So any AFE/DSP + // registers have to be re-initialized in this case. + if dev.state() == phy::DeviceState::Running && dev.speed() == uapi::SPEED_100 { + if let Err(e) = Self::helper_integrated_phy_analog_init(dev) { + pr_err!("rockchip: integrated_phy_analog_init err: {:?}", e); + } + } + } + + fn soft_reset(dev: &mut phy::Device) -> Result { + dev.genphy_soft_reset() + } + + fn config_init(dev: &mut phy::Device) -> Result { + PhyRockchip::helper_config_init(dev) + } + + fn config_aneg(dev: &mut phy::Device) -> Result { + PhyRockchip::helper_set_polarity(dev, dev.mdix())?; + dev.genphy_config_aneg() + } + + fn suspend(dev: &mut phy::Device) -> Result { + dev.genphy_suspend() + } + + fn resume(dev: &mut phy::Device) -> Result { + let _ = dev.genphy_resume(); + + PhyRockchip::helper_config_init(dev) + } +}
This is the Rust implementation of drivers/net/phy/rockchip.c. The features are equivalent. You can choose C or Rust version kernel configuration. Signed-off-by: Christina Quast <contact@christina-quast.de> --- drivers/net/phy/Kconfig | 8 +++ drivers/net/phy/Makefile | 4 ++ drivers/net/phy/rockchip_rust.rs | 131 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 143 insertions(+)