Re: [PATCH v2 3/3] net: phy: add Rust Rockchip PHY driver

Christina Quast
2024-02-06T19:20:06Z

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