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

  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(+)
  

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