[PATCH v2 0/3] Add Rust Rockchip PHY driver

[PATCH v2 0/3] Add Rust Rockchip PHY driver

[Christina Quast]

Based on the example shown in drivers/net/phy/ax88796b_rust.rs, I ported
the rockchip phy driver to Rust. The code in drivers/net/phy/rockchip.c
was basically rewritten in Rust.  The patchset includes changes to
phy.rs, adding more struct driver functions for the abstraction with
Rust.

The driver was not tested on real hardware, because I do not have a
board with this phy, and I would appreciate it if somebody could try
out the driver on their board.

Signed-off-by: Christina Quast <contact@christina-quast.de>
---
Christina Quast (3):
      DONOTMERGE: rust: prelude: add bit function
      rust: phy: add some phy_driver and genphy_ functions
      net: phy: add Rust Rockchip PHY driver

 drivers/net/phy/Kconfig          |   8 +++
 drivers/net/phy/Makefile         |   4 ++
 drivers/net/phy/rockchip_rust.rs | 131 +++++++++++++++++++++++++++++++++++++++
 rust/kernel/net/phy.rs           |  31 +++++++++
 rust/kernel/prelude.rs           |  16 +++++
 5 files changed, 190 insertions(+)
---
base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
change-id: 20240201-rockchip-rust-phy_depend-681db4707777

Best regards,
-- 
Christina Quast <contact@christina-quast.de>

[PATCH v2 1/3] DONOTMERGE: rust: prelude: add bit function

[Christina Quast]

In order to create masks easily, the define BIT() is used in C code.
This commit adds the same functionality to the rust kernel.

Do not merge this commit, because rust/kernel/types.rs in Rust-for-Linux
already contains this functionality and will be merged into next-net
soon.
But this driver does not compile without this commit, so I am adding it
to the patchset to get more feedback on the actual driver.

Signed-off-by: Christina Quast <contact@christina-quast.de>
---
 rust/kernel/prelude.rs | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index ae21600970b3..16e483de2f27 100644
--- a/rust/kernel/prelude.rs
+++ b/rust/kernel/prelude.rs
@@ -38,3 +38,19 @@
 pub use super::init::{InPlaceInit, Init, PinInit};
 
 pub use super::current;
+
+/// Returns a `u32` number that has only the `n`th bit set.
+///
+/// # Arguments
+///
+/// * `n` - A `u32` that specifies the bit position (zero-based index)
+///
+/// # Example
+///
+/// ```
+/// let b = bit(2);
+/// assert_eq!(b, 4);
+#[inline]
+pub const fn bit(n: u32) -> u32 {
+    1 << n
+}

-- 
2.43.0

[PATCH v2 2/3] rust: phy: add some phy_driver and genphy_ functions

[Christina Quast]

Those functions are need for the rockchip_rust.rs implementation.

Added functions:
genphy_config_aneg
config_init

Getter functions for mdix and speed.

Signed-off-by: Christina Quast <contact@christina-quast.de>
---
 rust/kernel/net/phy.rs | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index e457b3c7cb2f..373a4d358e9f 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -95,6 +95,22 @@ pub fn phy_id(&self) -> u32 {
         unsafe { (*phydev).phy_id }
     }
 
+    /// Gets the current crossover of the PHY.
+    pub fn mdix(&self) -> u8 {
+        let phydev = self.0.get();
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        unsafe { (*phydev).mdix }
+    }
+
+    /// Gets the speed of the PHY.
+    pub fn speed(&mut self) -> u32 {
+        let phydev = self.0.get();
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        unsafe { (*phydev).speed as u32 }
+    }
+
     /// Gets the state of PHY state machine states.
     pub fn state(&self) -> DeviceState {
         let phydev = self.0.get();
@@ -300,6 +316,15 @@ pub fn genphy_read_abilities(&mut self) -> Result {
         // So it's just an FFI call.
         to_result(unsafe { bindings::genphy_read_abilities(phydev) })
     }
+
+    /// Writes BMCR
+    pub fn genphy_config_aneg(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So it's just an FFI call.
+        // second param = false => autoneg not requested
+        to_result(unsafe { bindings::__genphy_config_aneg(phydev, false) })
+    }
 }
 
 /// Defines certain other features this PHY supports (like interrupts).
@@ -583,6 +608,12 @@ fn soft_reset(_dev: &mut Device) -> Result {
         Err(code::ENOTSUPP)
     }
 
+    /// Called to initialize the PHY,
+    /// including after a reset
+    fn config_init(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
     /// Probes the hardware to determine what abilities it has.
     fn get_features(_dev: &mut Device) -> Result {
         Err(code::ENOTSUPP)

-- 
2.43.0

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

[Christina Quast]

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

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

-- 
2.43.0

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

[Greg KH]

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

Re: [PATCH v2 1/3] DONOTMERGE: rust: prelude: add bit function

[Miguel Ojeda]

On Thu, Feb 1, 2024 at 7:07 PM Christina Quast
<contact@christina-quast.de> wrote:
>
> In order to create masks easily, the define BIT() is used in C code.
> This commit adds the same functionality to the rust kernel.

Note that it is the same kernel :) Typically we would say "to the Rust
side" or similar.

> Do not merge this commit, because rust/kernel/types.rs in Rust-for-Linux
> already contains this functionality and will be merged into next-net
> soon.

I think you mean the archived `rust` branch (it is useful to point
this out -- Rust for Linux is not just that branch).

However, has the `Bit` type (assuming you mean that) been submitted? I
don't recall seeing it, and normally something like that would not go
through `net-next`. If it has been, could you please send the Lore
link?

> But this driver does not compile without this commit, so I am adding it
> to the patchset to get more feedback on the actual driver.

Assuming the patch was not sent, I would suggest replacing this commit
with the dependency you want to use, e.g. the `Bit` type, since it is
a small enough one.

In addition, this series has v2 in the title -- I think you did that
because this patch was already submitted, but this is not really a v2
of that series since this is mainly about the driver (and anyway this
patch in particular is not meant to be merged; plus you didn't change
it from what I can see).

Thanks!

Cheers,
Miguel

Re: [PATCH v2 2/3] rust: phy: add some phy_driver and genphy_ functions

[Trevor Gross]

On Thu, Feb 1, 2024 at 12:07 PM Christina Quast
<contact@christina-quast.de> wrote:
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index e457b3c7cb2f..373a4d358e9f 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -95,6 +95,22 @@ pub fn phy_id(&self) -> u32 {
>          unsafe { (*phydev).phy_id }
>      }
>
> +    /// Gets the current crossover of the PHY.
> +    pub fn mdix(&self) -> u8 {

Are possible values for mdix always ETH_TP_MDI{,_INVALID,_X,_AUTO}? If
so, this would be better as an enum.

> +        let phydev = self.0.get();
> +        // SAFETY: The struct invariant ensures that we may access
> +        // this field without additional synchronization.
> +        unsafe { (*phydev).mdix }
> +    }

> +
>      /// Gets the state of PHY state machine states.
>      pub fn state(&self) -> DeviceState {
>          let phydev = self.0.get();
> @@ -300,6 +316,15 @@ pub fn genphy_read_abilities(&mut self) -> Result {
>          // So it's just an FFI call.
>          to_result(unsafe { bindings::genphy_read_abilities(phydev) })
>      }
> +
> +    /// Writes BMCR
> +    pub fn genphy_config_aneg(&mut self) -> Result {

The docs need an update here

> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So it's just an FFI call.
> +        // second param = false => autoneg not requested
> +        to_result(unsafe { bindings::__genphy_config_aneg(phydev, false) })

I assume you did this since the non-dunder `genphy_config_aneg` is
inline. I think that is ok since the implementation is so minimal, but
you could also add a binding helper and call that (rust/helpers.c).

> +    }
>  }
>
>  /// Defines certain other features this PHY supports (like interrupts).
> @@ -583,6 +608,12 @@ fn soft_reset(_dev: &mut Device) -> Result {
>          Err(code::ENOTSUPP)
>      }
>
> +    /// Called to initialize the PHY,
> +    /// including after a reset

Docs wrapping

> +    fn config_init(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }

These have been changed to raise a build error rather than ENOTSUPP in
recent , see [1]. That patch is in net-next so you should see it next
time you rebase.

Also - these functions are meant for the vtable and don't do anything
if they are not wired up. See the create_phy_driver function, you will
need to add the field.

>      /// Probes the hardware to determine what abilities it has.
>      fn get_features(_dev: &mut Device) -> Result {
>          Err(code::ENOTSUPP)
>
> --
> 2.43.0
>

[1]: https://lore.kernel.org/rust-for-linux/20240125014502.3527275-2-fujita.tomonori@gmail.com/

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

[Dragan Simic]

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?

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

[Trevor Gross]

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

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

[Andrew Lunn]

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

Re: [PATCH v2 0/3] Add Rust Rockchip PHY driver

[Andrew Lunn]

On Thu, Feb 01, 2024 at 07:06:57PM +0100, Christina Quast wrote:
> Based on the example shown in drivers/net/phy/ax88796b_rust.rs, I ported
> the rockchip phy driver to Rust. The code in drivers/net/phy/rockchip.c
> was basically rewritten in Rust.  The patchset includes changes to
> phy.rs, adding more struct driver functions for the abstraction with
> Rust.
> 
> The driver was not tested on real hardware, because I do not have a
> board with this phy, and I would appreciate it if somebody could try
> out the driver on their board.

As a general point, please post such code RFT. Also adding RFC would
be good, and explain what sort of comments you are requesting.

    Andrew

---
pw-bot: cr

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

[Miguel Ojeda]

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

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

[Christina Quast]

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

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

[Trevor Gross]

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

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

[Andrew Lunn]

> 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