Date: Tue, 6 Feb 2024 20:19:50 +0100
From: Christina Quast <chrysh@...istina-quast.de>
To: Trevor Gross <tmgross@...ch.edu>,
 Christina Quast <contact@...istina-quast.de>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng
 <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...sung.com>, Alice Ryhl
 <aliceryhl@...gle.com>, FUJITA Tomonori <fujita.tomonori@...il.com>,
 Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
 Russell King <linux@...linux.org.uk>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Heiko Stuebner <heiko@...ech.de>, rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v2 3/3] net: phy: add Rust Rockchip PHY driver

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@...istina-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@...istina-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@...il.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

Powered by blists - more mailing lists