Date: Thu, 1 Feb 2024 15:06:47 -0600
From: Trevor Gross <tmgross@...ch.edu>
To: 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

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