[PATCH net-next v1 0/4] net: phy: add Applied Micro QT2025 PHY driver

[PATCH net-next v1 0/4] net: phy: add Applied Micro QT2025 PHY driver

[FUJITA Tomonori]

This patchset adds a PHY driver for Applied Micro Circuits Corporation
QT2025. The 1-3th patches simply add more support functions for the
PHYLIB Rust bindings, which are necessary for the driver (the fourth
patch).

QT2025 PHY support was implemented as a part of an ethernet driver for
Tehuti Networks TN40xx chips. Multiple vendors (DLink, Asus, Edimax,
QNAP, etc) developed adapters based on TN40xx chips. Tehuti Networks
went out of business but the drivers are still distributed with some
of the hardware (and also available on some sites).

The original driver handles various PHY hardware (AMCC QT2025, TI
TLK10232, Aqrate AQR105, and Marvell MV88X3120, MV88X3310, and
MV88E2010). This driver is extracted from the original driver and
modified to a PHY driver in Rust.

This driver and a modified ethernet driver using PHYLIB have been
tested with Edimax EN-9320SFP+ 10G network adapter.

Note that the third patch adds Firmware API for PHY drivers. Firmware
API isn't specific to PHY drivers. I think that it would be
appropriate to add the feature to rust/kerel/device.rs. However,
recently drm developers have worked on that area actively so I added
it to phy.rs for now to avoid conflict (I assume that this patchset
would be merged via netdev tree and drm work would via rust or drm
tree). Once things calm down a bit, I'll move this feature (I suspect
that drm also needs rust bindings for Firmware API too).


FUJITA Tomonori (4):
  rust: net::phy support config_init driver callback
  rust: net::phy support C45 helpers
  rust: net::phy support Firmware API
  net: phy: add Applied Micro QT2025 PHY driver

 MAINTAINERS                     |   7 ++
 drivers/net/phy/Kconfig         |   7 ++
 drivers/net/phy/Makefile        |   1 +
 drivers/net/phy/qt2025.rs       |  75 ++++++++++++++++++++
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/net/phy.rs          | 121 ++++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h         |   1 +
 7 files changed, 213 insertions(+)
 create mode 100644 drivers/net/phy/qt2025.rs


base-commit: 32affa5578f0e6b9abef3623d3976395afbd265c
-- 
2.34.1

[PATCH net-next v1 1/4] rust: net::phy support config_init driver callback

[FUJITA Tomonori]

This patch adds phy_driver's config_init callback support for
initializing the hardware.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/net/phy.rs | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 96e09c6e8530..44b59bbf1a52 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -338,6 +338,22 @@ impl<T: Driver> Adapter<T> {
         })
     }
 
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn config_init_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: This callback is called only in contexts
+            // where we hold `phy_device->lock`, so the accessors on
+            // `Device` are okay to call.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::config_init(dev)?;
+            Ok(0)
+        })
+    }
+
     /// # Safety
     ///
     /// `phydev` must be passed by the corresponding callback in `phy_driver`.
@@ -511,6 +527,11 @@ pub const fn create_phy_driver<T: Driver>() -> DriverVTable {
         } else {
             None
         },
+        config_init: if T::HAS_CONFIG_INIT {
+            Some(Adapter::<T>::config_init_callback)
+        } else {
+            None
+        },
         get_features: if T::HAS_GET_FEATURES {
             Some(Adapter::<T>::get_features_callback)
         } else {
@@ -583,6 +604,11 @@ fn soft_reset(_dev: &mut Device) -> Result {
         kernel::build_error(VTABLE_DEFAULT_ERROR)
     }
 
+    /// Initializes the hardware, including after a reset.
+    fn config_init(_dev: &mut Device) -> Result {
+        kernel::build_error(VTABLE_DEFAULT_ERROR)
+    }
+
     /// Probes the hardware to determine what abilities it has.
     fn get_features(_dev: &mut Device) -> Result {
         kernel::build_error(VTABLE_DEFAULT_ERROR)
-- 
2.34.1

[PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[FUJITA Tomonori]

This patch adds the following helper functions for Clause 45:

- mdiobus_c45_read
- mdiobus_c45_write
- genphy_c45_read_status

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/net/phy.rs | 50 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 44b59bbf1a52..421a231421f5 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -203,6 +203,43 @@ pub fn write(&mut self, regnum: u16, val: u16) -> Result {
         })
     }
 
+    /// Reads a given C45 PHY register.
+    /// This function reads a hardware register and updates the stats so takes `&mut self`.
+    pub fn c45_read(&mut self, devad: u8, regnum: u16) -> Result<u16> {
+        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.
+        let ret = unsafe {
+            bindings::mdiobus_c45_read(
+                (*phydev).mdio.bus,
+                (*phydev).mdio.addr,
+                devad as i32,
+                regnum.into(),
+            )
+        };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    /// Writes a given C45 PHY register.
+    pub fn c45_write(&mut self, devad: u8, regnum: u16, val: u16) -> 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.
+        to_result(unsafe {
+            bindings::mdiobus_c45_write(
+                (*phydev).mdio.bus,
+                (*phydev).mdio.addr,
+                devad as i32,
+                regnum.into(),
+                val,
+            )
+        })
+    }
+
     /// Reads a paged register.
     pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> {
         let phydev = self.0.get();
@@ -277,6 +314,19 @@ pub fn genphy_read_status(&mut self) -> Result<u16> {
         }
     }
 
+    /// Checks the link status and updates current link state via C45 registers.
+    pub fn genphy_c45_read_status(&mut self) -> Result<u16> {
+        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.
+        let ret = unsafe { bindings::genphy_c45_read_status(phydev) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
     /// Updates the link status.
     pub fn genphy_update_link(&mut self) -> Result {
         let phydev = self.0.get();
-- 
2.34.1

[PATCH net-next v1 3/4] rust: net::phy support Firmware API

[FUJITA Tomonori]

This patch adds support to the following basic Firmware API:

- request_firmware
- release_firmware

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
CC: Luis Chamberlain <mcgrof@kernel.org>
CC: Russ Weight <russ.weight@linux.dev>
---
 drivers/net/phy/Kconfig         |  1 +
 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/net/phy.rs          | 45 +++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 7fddc8306d82..3ad04170aa4e 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -64,6 +64,7 @@ config RUST_PHYLIB_ABSTRACTIONS
         bool "Rust PHYLIB abstractions support"
         depends on RUST
         depends on PHYLIB=y
+        depends on FW_LOADER=y
         help
           Adds support needed for PHY drivers written in Rust. It provides
           a wrapper around the C phylib core.
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 65b98831b975..556f95c55b7b 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -9,6 +9,7 @@
 #include <kunit/test.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
+#include <linux/firmware.h>
 #include <linux/jiffies.h>
 #include <linux/mdio.h>
 #include <linux/phy.h>
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 421a231421f5..095dc3ccc553 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -9,6 +9,51 @@
 use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
 
 use core::marker::PhantomData;
+use core::ptr::{self, NonNull};
+
+/// A pointer to the kernel's `struct firmware`.
+///
+/// # Invariants
+///
+/// The pointer points at a `struct firmware`, and has ownership over the object.
+pub struct Firmware(NonNull<bindings::firmware>);
+
+impl Firmware {
+    /// Loads a firmware.
+    pub fn new(name: &CStr, dev: &Device) -> Result<Firmware> {
+        let phydev = dev.0.get();
+        let mut ptr: *mut bindings::firmware = ptr::null_mut();
+        let p_ptr: *mut *mut bindings::firmware = &mut ptr;
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
+        // So it's just an FFI call.
+        let ret = unsafe {
+            bindings::request_firmware(
+                p_ptr as *mut *const bindings::firmware,
+                name.as_char_ptr().cast(),
+                &mut (*phydev).mdio.dev,
+            )
+        };
+        let fw = NonNull::new(ptr).ok_or_else(|| Error::from_errno(ret))?;
+        // INVARIANT: We checked that the firmware was successfully loaded.
+        Ok(Firmware(fw))
+    }
+
+    /// Accesses the firmware contents.
+    pub fn data(&self) -> &[u8] {
+        // SAFETY: The type invariants guarantee that `self.0.as_ptr()` is valid.
+        // They also guarantee that `self.0.as_ptr().data` pointers to
+        // a valid memory region of size `self.0.as_ptr().size`.
+        unsafe { core::slice::from_raw_parts((*self.0.as_ptr()).data, (*self.0.as_ptr()).size) }
+    }
+}
+
+impl Drop for Firmware {
+    fn drop(&mut self) {
+        // SAFETY: By the type invariants, `self.0.as_ptr()` is valid and
+        // we have ownership of the object so can free it.
+        unsafe { bindings::release_firmware(self.0.as_ptr()) }
+    }
+}
 
 /// PHY state machine states.
 ///
-- 
2.34.1

[PATCH net-next v1 4/4] net: phy: add Applied Micro QT2025 PHY driver

[FUJITA Tomonori]

This driver supports Applied Micro Circuits Corporation QT2025 PHY,
based on a driver for Tehuti Networks TN40xx chips.

The original driver for TN40xx chips supports multiple PHY hardware
(AMCC QT2025, TI TLK10232, Aqrate AQR105, and Marvell 88X3120,
88X3310, and MV88E2010). This driver is extracted from the original
driver and modified to a PHY driver in Rust.

This has been tested with Edimax EN-9320SFP+ 10G network adapter.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 MAINTAINERS               |  7 ++++
 drivers/net/phy/Kconfig   |  6 ++++
 drivers/net/phy/Makefile  |  1 +
 drivers/net/phy/qt2025.rs | 75 +++++++++++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h   |  1 +
 5 files changed, 90 insertions(+)
 create mode 100644 drivers/net/phy/qt2025.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 5ba3fe6ac09c..f2d86e221ba3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1540,6 +1540,13 @@ F:	Documentation/admin-guide/perf/xgene-pmu.rst
 F:	Documentation/devicetree/bindings/perf/apm-xgene-pmu.txt
 F:	drivers/perf/xgene_pmu.c
 
+APPLIED MICRO QT2025 PHY DRIVER
+M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
+L:	netdev@vger.kernel.org
+L:	rust-for-linux@vger.kernel.org
+S:	Maintained
+F:	drivers/net/phy/qt2025.rs
+
 APTINA CAMERA SENSOR PLL
 M:	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 3ad04170aa4e..8293c3d14229 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -110,6 +110,12 @@ config ADIN1100_PHY
 	  Currently supports the:
 	  - ADIN1100 - Robust,Industrial, Low Power 10BASE-T1L Ethernet PHY
 
+config AMCC_QT2025_PHY
+	tristate "AMCC QT2025 PHY"
+	depends on RUST_PHYLIB_ABSTRACTIONS
+	help
+	  Adds support for the Applied Micro Circuits Corporation QT2025 PHY.
+
 source "drivers/net/phy/aquantia/Kconfig"
 
 config AX88796B_PHY
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 1d8be374915f..75d0b07a392a 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_ADIN_PHY)		+= adin.o
 obj-$(CONFIG_ADIN1100_PHY)	+= adin1100.o
 obj-$(CONFIG_AIR_EN8811H_PHY)   += air_en8811h.o
 obj-$(CONFIG_AMD_PHY)		+= amd.o
+obj-$(CONFIG_AMCC_QT2025_PHY)	+= qt2025.o
 obj-$(CONFIG_AQUANTIA_PHY)	+= aquantia/
 ifdef CONFIG_AX88796B_RUST_PHY
   obj-$(CONFIG_AX88796B_PHY)	+= ax88796b_rust.o
diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
new file mode 100644
index 000000000000..e42b77753717
--- /dev/null
+++ b/drivers/net/phy/qt2025.rs
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) Tehuti Networks Ltd.
+// Copyright (C) 2024 FUJITA Tomonori <fujita.tomonori@gmail.com>
+
+//! Applied Micro Circuits Corporation QT2025 PHY driver
+use kernel::c_str;
+use kernel::net::phy::{self, DeviceId, Driver, Firmware};
+use kernel::prelude::*;
+use kernel::uapi;
+
+kernel::module_phy_driver! {
+    drivers: [PhyQT2025],
+    device_table: [
+        DeviceId::new_with_driver::<PhyQT2025>(),
+    ],
+    name: "qt2025_phy",
+    author: "FUJITA Tomonori <fujita.tomonori@gmail.com>",
+    description: "AMCC QT2025 PHY driver",
+    license: "GPL",
+}
+
+const MDIO_MMD_PMAPMD: u8 = uapi::MDIO_MMD_PMAPMD as u8;
+const MDIO_MMD_PCS: u8 = uapi::MDIO_MMD_PCS as u8;
+const MDIO_MMD_PHYXS: u8 = uapi::MDIO_MMD_PHYXS as u8;
+
+struct PhyQT2025;
+
+#[vtable]
+impl Driver for PhyQT2025 {
+    const NAME: &'static CStr = c_str!("QT2025 10Gpbs SFP+");
+    const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x0043A400);
+
+    fn config_init(dev: &mut phy::Device) -> Result<()> {
+        let fw = Firmware::new(c_str!("qt2025-2.0.3.3.fw"), dev)?;
+
+        let phy_id = dev.c45_read(MDIO_MMD_PMAPMD, 0xd001)?;
+        if (phy_id >> 8) & 0xff != 0xb3 {
+            return Ok(());
+        }
+
+        dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0000)?;
+        dev.c45_write(MDIO_MMD_PMAPMD, 0xC302, 0x4)?;
+        dev.c45_write(MDIO_MMD_PMAPMD, 0xC319, 0x0038)?;
+
+        dev.c45_write(MDIO_MMD_PMAPMD, 0xC31A, 0x0098)?;
+        dev.c45_write(MDIO_MMD_PCS, 0x0026, 0x0E00)?;
+
+        dev.c45_write(MDIO_MMD_PCS, 0x0027, 0x0893)?;
+
+        dev.c45_write(MDIO_MMD_PCS, 0x0028, 0xA528)?;
+        dev.c45_write(MDIO_MMD_PCS, 0x0029, 0x03)?;
+        dev.c45_write(MDIO_MMD_PMAPMD, 0xC30A, 0x06E1)?;
+        dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0002)?;
+        dev.c45_write(MDIO_MMD_PCS, 0xE854, 0x00C0)?;
+
+        let mut j = 0x8000;
+        let mut a = MDIO_MMD_PCS;
+        for (i, val) in fw.data().iter().enumerate() {
+            if i == 0x4000 {
+                a = MDIO_MMD_PHYXS;
+                j = 0x8000;
+            }
+            dev.c45_write(a, j, (*val).into())?;
+
+            j += 1;
+        }
+        dev.c45_write(MDIO_MMD_PCS, 0xe854, 0x0040)?;
+
+        Ok(())
+    }
+
+    fn read_status(dev: &mut phy::Device) -> Result<u16> {
+        dev.genphy_c45_read_status()
+    }
+}
diff --git a/rust/uapi/uapi_helper.h b/rust/uapi/uapi_helper.h
index 08f5e9334c9e..76d3f103e764 100644
--- a/rust/uapi/uapi_helper.h
+++ b/rust/uapi/uapi_helper.h
@@ -7,5 +7,6 @@
  */
 
 #include <uapi/asm-generic/ioctl.h>
+#include <uapi/linux/mdio.h>
 #include <uapi/linux/mii.h>
 #include <uapi/linux/ethtool.h>
-- 
2.34.1

Re: [PATCH net-next v1 3/4] rust: net::phy support Firmware API

[Greg KH]

On Mon, Apr 15, 2024 at 07:47:00PM +0900, FUJITA Tomonori wrote:
> This patch adds support to the following basic Firmware API:
> 
> - request_firmware
> - release_firmware
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> CC: Luis Chamberlain <mcgrof@kernel.org>
> CC: Russ Weight <russ.weight@linux.dev>
> ---
>  drivers/net/phy/Kconfig         |  1 +
>  rust/bindings/bindings_helper.h |  1 +
>  rust/kernel/net/phy.rs          | 45 +++++++++++++++++++++++++++++++++

Please do not bury this in the phy.rs file, put it in drivers/base/ next
to the firmware functions it is calling.

>  3 files changed, 47 insertions(+)
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 7fddc8306d82..3ad04170aa4e 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -64,6 +64,7 @@ config RUST_PHYLIB_ABSTRACTIONS
>          bool "Rust PHYLIB abstractions support"
>          depends on RUST
>          depends on PHYLIB=y
> +        depends on FW_LOADER=y
>          help
>            Adds support needed for PHY drivers written in Rust. It provides
>            a wrapper around the C phylib core.
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 65b98831b975..556f95c55b7b 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -9,6 +9,7 @@
>  #include <kunit/test.h>
>  #include <linux/errname.h>
>  #include <linux/ethtool.h>
> +#include <linux/firmware.h>
>  #include <linux/jiffies.h>
>  #include <linux/mdio.h>
>  #include <linux/phy.h>
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 421a231421f5..095dc3ccc553 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -9,6 +9,51 @@
>  use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
>  
>  use core::marker::PhantomData;
> +use core::ptr::{self, NonNull};
> +
> +/// A pointer to the kernel's `struct firmware`.
> +///
> +/// # Invariants
> +///
> +/// The pointer points at a `struct firmware`, and has ownership over the object.
> +pub struct Firmware(NonNull<bindings::firmware>);
> +
> +impl Firmware {
> +    /// Loads a firmware.
> +    pub fn new(name: &CStr, dev: &Device) -> Result<Firmware> {
> +        let phydev = dev.0.get();

That's risky, how do you "know" this device really is a phydev?  That's
not how the firmware api works, use a real 'struct device' please.


> +        let mut ptr: *mut bindings::firmware = ptr::null_mut();
> +        let p_ptr: *mut *mut bindings::firmware = &mut ptr;

I'm sorry, but I don't understand the two step thing here, you need a
pointer for where the C code can put something, is this really how you
do that in rust?  Shouldn't it point to data somehow down below?

> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.

Again, phydev is not part of the firmware api.

> +        // So it's just an FFI call.
> +        let ret = unsafe {
> +            bindings::request_firmware(
> +                p_ptr as *mut *const bindings::firmware,

Where is this coming from?

> +                name.as_char_ptr().cast(),
> +                &mut (*phydev).mdio.dev,
> +            )
> +        };
> +        let fw = NonNull::new(ptr).ok_or_else(|| Error::from_errno(ret))?;
> +        // INVARIANT: We checked that the firmware was successfully loaded.
> +        Ok(Firmware(fw))
> +    }
> +
> +    /// Accesses the firmware contents.
> +    pub fn data(&self) -> &[u8] {

But firmware is not a u8, it's a structure.  Can't the rust bindings
handle that?  Oh wait, data is a u8, but the bindings should show that,
right?


> +        // SAFETY: The type invariants guarantee that `self.0.as_ptr()` is valid.
> +        // They also guarantee that `self.0.as_ptr().data` pointers to
> +        // a valid memory region of size `self.0.as_ptr().size`.
> +        unsafe { core::slice::from_raw_parts((*self.0.as_ptr()).data, (*self.0.as_ptr()).size) }

If new fails, does accessing this also fail?

And how are you using this?  I guess I'll dig in the next patch...

> +    }
> +}
> +
> +impl Drop for Firmware {
> +    fn drop(&mut self) {
> +        // SAFETY: By the type invariants, `self.0.as_ptr()` is valid and
> +        // we have ownership of the object so can free it.
> +        unsafe { bindings::release_firmware(self.0.as_ptr()) }

So drop will never be called if new fails?

Again, please don't put this in the phy driver, put it where it belongs
so we can add the other firmware functions when needed.

thanks,

greg k-h

Re: [PATCH net-next v1 4/4] net: phy: add Applied Micro QT2025 PHY driver

[Greg KH]

On Mon, Apr 15, 2024 at 07:47:01PM +0900, FUJITA Tomonori wrote:
> This driver supports Applied Micro Circuits Corporation QT2025 PHY,
> based on a driver for Tehuti Networks TN40xx chips.
> 
> The original driver for TN40xx chips supports multiple PHY hardware
> (AMCC QT2025, TI TLK10232, Aqrate AQR105, and Marvell 88X3120,
> 88X3310, and MV88E2010). This driver is extracted from the original
> driver and modified to a PHY driver in Rust.
> 
> This has been tested with Edimax EN-9320SFP+ 10G network adapter.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  MAINTAINERS               |  7 ++++
>  drivers/net/phy/Kconfig   |  6 ++++
>  drivers/net/phy/Makefile  |  1 +
>  drivers/net/phy/qt2025.rs | 75 +++++++++++++++++++++++++++++++++++++++
>  rust/uapi/uapi_helper.h   |  1 +
>  5 files changed, 90 insertions(+)
>  create mode 100644 drivers/net/phy/qt2025.rs
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5ba3fe6ac09c..f2d86e221ba3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1540,6 +1540,13 @@ F:	Documentation/admin-guide/perf/xgene-pmu.rst
>  F:	Documentation/devicetree/bindings/perf/apm-xgene-pmu.txt
>  F:	drivers/perf/xgene_pmu.c
>  
> +APPLIED MICRO QT2025 PHY DRIVER
> +M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
> +L:	netdev@vger.kernel.org
> +L:	rust-for-linux@vger.kernel.org
> +S:	Maintained
> +F:	drivers/net/phy/qt2025.rs
> +
>  APTINA CAMERA SENSOR PLL
>  M:	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>  L:	linux-media@vger.kernel.org
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 3ad04170aa4e..8293c3d14229 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -110,6 +110,12 @@ config ADIN1100_PHY
>  	  Currently supports the:
>  	  - ADIN1100 - Robust,Industrial, Low Power 10BASE-T1L Ethernet PHY
>  
> +config AMCC_QT2025_PHY
> +	tristate "AMCC QT2025 PHY"
> +	depends on RUST_PHYLIB_ABSTRACTIONS
> +	help
> +	  Adds support for the Applied Micro Circuits Corporation QT2025 PHY.
> +
>  source "drivers/net/phy/aquantia/Kconfig"
>  
>  config AX88796B_PHY
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 1d8be374915f..75d0b07a392a 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_ADIN_PHY)		+= adin.o
>  obj-$(CONFIG_ADIN1100_PHY)	+= adin1100.o
>  obj-$(CONFIG_AIR_EN8811H_PHY)   += air_en8811h.o
>  obj-$(CONFIG_AMD_PHY)		+= amd.o
> +obj-$(CONFIG_AMCC_QT2025_PHY)	+= qt2025.o
>  obj-$(CONFIG_AQUANTIA_PHY)	+= aquantia/
>  ifdef CONFIG_AX88796B_RUST_PHY
>    obj-$(CONFIG_AX88796B_PHY)	+= ax88796b_rust.o
> diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
> new file mode 100644
> index 000000000000..e42b77753717
> --- /dev/null
> +++ b/drivers/net/phy/qt2025.rs
> @@ -0,0 +1,75 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) Tehuti Networks Ltd.
> +// Copyright (C) 2024 FUJITA Tomonori <fujita.tomonori@gmail.com>
> +
> +//! Applied Micro Circuits Corporation QT2025 PHY driver
> +use kernel::c_str;
> +use kernel::net::phy::{self, DeviceId, Driver, Firmware};
> +use kernel::prelude::*;
> +use kernel::uapi;
> +
> +kernel::module_phy_driver! {
> +    drivers: [PhyQT2025],
> +    device_table: [
> +        DeviceId::new_with_driver::<PhyQT2025>(),
> +    ],
> +    name: "qt2025_phy",
> +    author: "FUJITA Tomonori <fujita.tomonori@gmail.com>",
> +    description: "AMCC QT2025 PHY driver",
> +    license: "GPL",
> +}

What about support for MODULE_FIRMWARE() so it will be properly loaded
into the initramfs of systems now that you are needing it for this
driver?  To ignore that is going to cause problems :(



> +
> +const MDIO_MMD_PMAPMD: u8 = uapi::MDIO_MMD_PMAPMD as u8;
> +const MDIO_MMD_PCS: u8 = uapi::MDIO_MMD_PCS as u8;
> +const MDIO_MMD_PHYXS: u8 = uapi::MDIO_MMD_PHYXS as u8;
> +
> +struct PhyQT2025;
> +
> +#[vtable]
> +impl Driver for PhyQT2025 {
> +    const NAME: &'static CStr = c_str!("QT2025 10Gpbs SFP+");
> +    const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x0043A400);
> +
> +    fn config_init(dev: &mut phy::Device) -> Result<()> {
> +        let fw = Firmware::new(c_str!("qt2025-2.0.3.3.fw"), dev)?;
> +
> +        let phy_id = dev.c45_read(MDIO_MMD_PMAPMD, 0xd001)?;
> +        if (phy_id >> 8) & 0xff != 0xb3 {
> +            return Ok(());
> +        }
> +
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0000)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC302, 0x4)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC319, 0x0038)?;
> +
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC31A, 0x0098)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0x0026, 0x0E00)?;
> +
> +        dev.c45_write(MDIO_MMD_PCS, 0x0027, 0x0893)?;
> +
> +        dev.c45_write(MDIO_MMD_PCS, 0x0028, 0xA528)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0x0029, 0x03)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC30A, 0x06E1)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0002)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0xE854, 0x00C0)?;
> +
> +        let mut j = 0x8000;
> +        let mut a = MDIO_MMD_PCS;
> +        for (i, val) in fw.data().iter().enumerate() {

So you are treating the firmware image as able to be iterated over here?


> +            if i == 0x4000 {

What does 0x4000 mean here?

> +                a = MDIO_MMD_PHYXS;
> +                j = 0x8000;

What does 0x8000 mean here?

> +            }
> +            dev.c45_write(a, j, (*val).into())?;
> +
> +            j += 1;
> +        }
> +        dev.c45_write(MDIO_MMD_PCS, 0xe854, 0x0040)?;

Lots of magic values in this driver, is that intentional?

thanks,

greg k-h

Re: [PATCH net-next v1 3/4] rust: net::phy support Firmware API

[Andrew Lunn]

On Mon, Apr 15, 2024 at 07:47:00PM +0900, FUJITA Tomonori wrote:
> This patch adds support to the following basic Firmware API:
> 
> - request_firmware
> - release_firmware

I fully agree with GregKH here. You should be writing a generic Rust
abstraction around firmware loading any Linux driver can use. There
should not be anything PHY specific in it.

	Andrew

Re: [PATCH net-next v1 4/4] net: phy: add Applied Micro QT2025 PHY driver

[Andrew Lunn]

> +        let phy_id = dev.c45_read(MDIO_MMD_PMAPMD, 0xd001)?;
> +        if (phy_id >> 8) & 0xff != 0xb3 {
> +            return Ok(());
> +        }
> +
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0000)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC302, 0x4)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC319, 0x0038)?;
> +
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC31A, 0x0098)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0x0026, 0x0E00)?;
> +
> +        dev.c45_write(MDIO_MMD_PCS, 0x0027, 0x0893)?;
> +
> +        dev.c45_write(MDIO_MMD_PCS, 0x0028, 0xA528)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0x0029, 0x03)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC30A, 0x06E1)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0002)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0xE854, 0x00C0)?;
> +
> +        let mut j = 0x8000;
> +        let mut a = MDIO_MMD_PCS;
> +        for (i, val) in fw.data().iter().enumerate() {
> +            if i == 0x4000 {

I'm assuming enumerate() does the same as Python enumerate. So `i` will
be the offset into the firmware. There is actually two different
firmware blobs in the file, and you write one into the PCS and the
second into the PHYXS?

> +                a = MDIO_MMD_PHYXS;
> +                j = 0x8000;
> +            }
> +            dev.c45_write(a, j, (*val).into())?;

Maybe my limitation with Rust. Is this writing a byte or a u16 to the
register? PHY registers are generally u16. And if it is a u16, what
about endianness?

Firmware should not be trusted. What about the case the firmware does
not have 0x4000 words in it? Is the size of the second blob known?
Also 0x4000?

It would be more normal to load firmware into the PHY during probe,
not config_init.

    Andrew

Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[Andrew Lunn]

On Mon, Apr 15, 2024 at 07:46:59PM +0900, FUJITA Tomonori wrote:
> This patch adds the following helper functions for Clause 45:
> 
> - mdiobus_c45_read
> - mdiobus_c45_write
> - genphy_c45_read_status

We need to be very careful here, and try to avoid an issue we have in
the phylib core, if possible. C45 is a mix of two different things:

* The C45 MDIO bus protocol
* The C45 registers

You can access C45 registers using the C45 bus protocol. You can also
access C45 registers using C45 over C22. So there are two access
mechanisms to the C45 registers.

A PHY driver just wants to access C45 registers. It should not care
about what access mechanism is used, be it C45 bus protocol, or C45
over C22.

> +    /// Reads a given C45 PHY register.
> +    /// This function reads a hardware register and updates the stats so takes `&mut self`.
> +    pub fn c45_read(&mut self, devad: u8, regnum: u16) -> Result<u16> {
> +        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.
> +        let ret = unsafe {
> +            bindings::mdiobus_c45_read(
> +                (*phydev).mdio.bus,
> +                (*phydev).mdio.addr,
> +                devad as i32,
> +                regnum.into(),
> +            )

So you have wrapped the C function mdiobus_c45_read(). This is going
to do a C45 bus protocol read. For this to work, the MDIO bus master
needs to support C45 bus protocol, and the PHY also needs to support
the C45 bus protocol. Not all MDIO bus masters and PHY devices
do. Some will need to use C45 over C22.

A PHY driver should know if a PHY device supports C45 bus protocol,
and if it supports C45 over C22. However, i PHY driver has no idea
what the bus master supports.

In phylib, we have a poorly defined phydev->is_c45. Its current
meaning is: "The device was found using the C45 bus protocol, not C22
protocol". One of the things phylib core then uses is_c45 for it to
direct the C functions phy_read_mmd() and phy_write_mmd() to perform a
C45 bus protocol access if true. If it is false, C45 over C22 is
performed instead. As a result, if a PHY is discovered using C22, C45
register access is then performed using C45 over C22, even thought the
PHY and bus might support C45 bus protocol. is_c45 is also used in
other places, e.g. to trigger auto-negotiation using C45 registers,
not C22 registers.

In summary, the C API is a bit of a mess.

For the Rust API we have two sensible choices:

1) It is the same mess as the C API, so hopefully one day we will fix
   both at the same time.

2) We define a different API which correctly separate C45 bus access
   from C45 registers.

How you currently defined the Rust API is neither of these.

       Andrew

Re: [PATCH net-next v1 3/4] rust: net::phy support Firmware API

[Danilo Krummrich]

On 4/15/24 12:47, FUJITA Tomonori wrote:
> This patch adds support to the following basic Firmware API:
> 
> - request_firmware
> - release_firmware
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> CC: Luis Chamberlain <mcgrof@kernel.org>
> CC: Russ Weight <russ.weight@linux.dev>
> ---
>   drivers/net/phy/Kconfig         |  1 +
>   rust/bindings/bindings_helper.h |  1 +
>   rust/kernel/net/phy.rs          | 45 +++++++++++++++++++++++++++++++++
>   3 files changed, 47 insertions(+)

As Greg already mentioned, this shouldn't be implemented specifically for struct
phy_device, but rather for a generic struct device.

I already got some generic firmware abstractions [1][2] sitting on top of a patch
series adding some basic generic device / driver abstractions [3].

I won't send out an isolated version of the device / driver series, but the full
patch series for the Nova stub driver [4] once I got everything in place. This was
requested by Greg to be able to see the full picture.

The series will then also include the firmware abstractions.

In order to use them from your PHY driver, I think all you need to do is to implement
AsRef<> for your phy::Device:

impl AsRef<device::Device> for Device {
     fn as_ref(&self) -> &device::Device {
         // SAFETY: By the type invariants, we know that `self.ptr` is non-null and valid.
         unsafe { device::Device::from_raw(&mut (*self.ptr).mdio.dev) }
     }
}

- Danilo

[1] https://gitlab.freedesktop.org/drm/nova/-/commit/e9bb608206f3c30a0f8d71fe472719778a113b28
[2] https://gitlab.freedesktop.org/drm/nova/-/tree/topic/firmware
[3] https://github.com/Rust-for-Linux/linux/tree/staging/rust-device
[4] https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u

> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 7fddc8306d82..3ad04170aa4e 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -64,6 +64,7 @@ config RUST_PHYLIB_ABSTRACTIONS
>           bool "Rust PHYLIB abstractions support"
>           depends on RUST
>           depends on PHYLIB=y
> +        depends on FW_LOADER=y
>           help
>             Adds support needed for PHY drivers written in Rust. It provides
>             a wrapper around the C phylib core.
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 65b98831b975..556f95c55b7b 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -9,6 +9,7 @@
>   #include <kunit/test.h>
>   #include <linux/errname.h>
>   #include <linux/ethtool.h>
> +#include <linux/firmware.h>
>   #include <linux/jiffies.h>
>   #include <linux/mdio.h>
>   #include <linux/phy.h>
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 421a231421f5..095dc3ccc553 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -9,6 +9,51 @@
>   use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
>   
>   use core::marker::PhantomData;
> +use core::ptr::{self, NonNull};
> +
> +/// A pointer to the kernel's `struct firmware`.
> +///
> +/// # Invariants
> +///
> +/// The pointer points at a `struct firmware`, and has ownership over the object.
> +pub struct Firmware(NonNull<bindings::firmware>);
> +
> +impl Firmware {
> +    /// Loads a firmware.
> +    pub fn new(name: &CStr, dev: &Device) -> Result<Firmware> {
> +        let phydev = dev.0.get();
> +        let mut ptr: *mut bindings::firmware = ptr::null_mut();
> +        let p_ptr: *mut *mut bindings::firmware = &mut ptr;
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
> +        // So it's just an FFI call.
> +        let ret = unsafe {
> +            bindings::request_firmware(
> +                p_ptr as *mut *const bindings::firmware,
> +                name.as_char_ptr().cast(),
> +                &mut (*phydev).mdio.dev,
> +            )
> +        };
> +        let fw = NonNull::new(ptr).ok_or_else(|| Error::from_errno(ret))?;
> +        // INVARIANT: We checked that the firmware was successfully loaded.
> +        Ok(Firmware(fw))
> +    }
> +
> +    /// Accesses the firmware contents.
> +    pub fn data(&self) -> &[u8] {
> +        // SAFETY: The type invariants guarantee that `self.0.as_ptr()` is valid.
> +        // They also guarantee that `self.0.as_ptr().data` pointers to
> +        // a valid memory region of size `self.0.as_ptr().size`.
> +        unsafe { core::slice::from_raw_parts((*self.0.as_ptr()).data, (*self.0.as_ptr()).size) }
> +    }
> +}
> +
> +impl Drop for Firmware {
> +    fn drop(&mut self) {
> +        // SAFETY: By the type invariants, `self.0.as_ptr()` is valid and
> +        // we have ownership of the object so can free it.
> +        unsafe { bindings::release_firmware(self.0.as_ptr()) }
> +    }
> +}
>   
>   /// PHY state machine states.
>   ///

Re: [PATCH net-next v1 4/4] net: phy: add Applied Micro QT2025 PHY driver

[Andrew Lunn]

> +const MDIO_MMD_PMAPMD: u8 = uapi::MDIO_MMD_PMAPMD as u8;
> +const MDIO_MMD_PCS: u8 = uapi::MDIO_MMD_PCS as u8;
> +const MDIO_MMD_PHYXS: u8 = uapi::MDIO_MMD_PHYXS as u8;

These probably belong somewhere else, where all Rust PHY drivers can
use them.

> +
> +struct PhyQT2025;
> +
> +#[vtable]
> +impl Driver for PhyQT2025 {
> +    const NAME: &'static CStr = c_str!("QT2025 10Gpbs SFP+");
> +    const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x0043A400);
> +
> +    fn config_init(dev: &mut phy::Device) -> Result<()> {
> +        let fw = Firmware::new(c_str!("qt2025-2.0.3.3.fw"), dev)?;
> +
> +        let phy_id = dev.c45_read(MDIO_MMD_PMAPMD, 0xd001)?;
> +        if (phy_id >> 8) & 0xff != 0xb3 {
> +            return Ok(());
> +        }

I'm guessing that is checking if the firmware has already been
downloaded? It would be good to add a comment about this. I also
wounder about the name phy_id? They are normally stored in registers
0x2 and 0x3, not 0xd001. What sort of ID is this?

> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0000)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC302, 0x4)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC319, 0x0038)?;
> +
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC31A, 0x0098)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0x0026, 0x0E00)?;
> +
> +        dev.c45_write(MDIO_MMD_PCS, 0x0027, 0x0893)?;
> +
> +        dev.c45_write(MDIO_MMD_PCS, 0x0028, 0xA528)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0x0029, 0x03)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC30A, 0x06E1)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0002)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0xE854, 0x00C0)?;

Some of these registers are partially documented in the
datasheet. e854 controls where the 8051 executes code from. If bit 6
is set, it executes from SRAM, if it is cleared, to runs the Boot
ROM. Bit 7 is not defined, but i guess it halts the 8051 when
set. Please add some const for these. C302 is setting the CPU clock
speed, c30A is about TX clock rate. Please try to document what you
can using information from the datasheet.

> +
> +        let mut j = 0x8000;
> +        let mut a = MDIO_MMD_PCS;
> +        for (i, val) in fw.data().iter().enumerate() {
> +            if i == 0x4000 {

Is this was C code, i would of said use SZ_16K, to give a hint this is
about reading the first 16K of the firmware. The device actually has
24K of SRAM, which gets mapped into two different C45 address spaces.
You should also add a check that the firmware does not have a total
size of > 24K. Never trust firmware blobs.

> +                a = MDIO_MMD_PHYXS;
> +                j = 0x8000;
> +            }
> +            dev.c45_write(a, j, (*val).into())?;
> +
> +            j += 1;
> +        }
> +        dev.c45_write(MDIO_MMD_PCS, 0xe854, 0x0040)?;
> +
> +        Ok(())
> +    }

  Andrew

Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[Trevor Gross]

On Mon, Apr 15, 2024 at 6:47 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> +    /// Reads a given C45 PHY register.
> +    /// This function reads a hardware register and updates the stats so takes `&mut self`.
> +    pub fn c45_read(&mut self, devad: u8, regnum: u16) -> Result<u16> {
> +        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.

Depending on the response to Andrew's notes, these SAFETY comments
will probably need to be updated to say why we know C45 is supported.

> +        let ret = unsafe {
> +            bindings::mdiobus_c45_read(
> +                (*phydev).mdio.bus,
> +                (*phydev).mdio.addr,
> +                devad as i32,

This could probably also be from/.into()

> +                regnum.into(),
> +            )
> +        };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u16)
> +        }

Could this be simplified with to_result?

> +    }
> +
> +    /// Writes a given C45 PHY register.
> +    pub fn c45_write(&mut self, devad: u8, regnum: u16, val: u16) -> 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.
> +        to_result(unsafe {
> +            bindings::mdiobus_c45_write(
> +                (*phydev).mdio.bus,
> +                (*phydev).mdio.addr,
> +                devad as i32,

Same as above

> +                regnum.into(),
> +                val,
> +            )
> +        })
> +    }
> +

Re: [PATCH net-next v1 4/4] net: phy: add Applied Micro QT2025 PHY driver

[Trevor Gross]

On Mon, Apr 15, 2024 at 6:47 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> This driver supports Applied Micro Circuits Corporation QT2025 PHY,
> based on a driver for Tehuti Networks TN40xx chips.
>
> The original driver for TN40xx chips supports multiple PHY hardware
> (AMCC QT2025, TI TLK10232, Aqrate AQR105, and Marvell 88X3120,
> 88X3310, and MV88E2010). This driver is extracted from the original
> driver and modified to a PHY driver in Rust.
>
> This has been tested with Edimax EN-9320SFP+ 10G network adapter.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> [...]
> diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
> new file mode 100644
> index 000000000000..e42b77753717
> --- /dev/null
> +++ b/drivers/net/phy/qt2025.rs
> @@ -0,0 +1,75 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) Tehuti Networks Ltd.
> +// Copyright (C) 2024 FUJITA Tomonori <fujita.tomonori@gmail.com>
> +
> +//! Applied Micro Circuits Corporation QT2025 PHY driver
> +use kernel::c_str;
> +use kernel::net::phy::{self, DeviceId, Driver, Firmware};
> +use kernel::prelude::*;
> +use kernel::uapi;
> +
> +kernel::module_phy_driver! {
> +    drivers: [PhyQT2025],
> +    device_table: [
> +        DeviceId::new_with_driver::<PhyQT2025>(),
> +    ],
> +    name: "qt2025_phy",
> +    author: "FUJITA Tomonori <fujita.tomonori@gmail.com>",
> +    description: "AMCC QT2025 PHY driver",
> +    license: "GPL",
> +}
> +
> +const MDIO_MMD_PMAPMD: u8 = uapi::MDIO_MMD_PMAPMD as u8;
> +const MDIO_MMD_PCS: u8 = uapi::MDIO_MMD_PCS as u8;
> +const MDIO_MMD_PHYXS: u8 = uapi::MDIO_MMD_PHYXS as u8;
> +
> +struct PhyQT2025;
> +
> +#[vtable]
> +impl Driver for PhyQT2025 {
> +    const NAME: &'static CStr = c_str!("QT2025 10Gpbs SFP+");

Since 1.77 we have C string literals, `c"QT2025 10Gpbs SFP+"` (woohoo)

> +    const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x0043A400);
> +
> +    fn config_init(dev: &mut phy::Device) -> Result<()> {
> +        let fw = Firmware::new(c_str!("qt2025-2.0.3.3.fw"), dev)?;

Same as above

> +        let phy_id = dev.c45_read(MDIO_MMD_PMAPMD, 0xd001)?;
> +        if (phy_id >> 8) & 0xff != 0xb3 {
> +            return Ok(());
> +        }

Could you add a note about why you are returning early? Also some magic numbers

> +
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0000)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC302, 0x4)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC319, 0x0038)?;
> +
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC31A, 0x0098)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0x0026, 0x0E00)?;
> +
> +        dev.c45_write(MDIO_MMD_PCS, 0x0027, 0x0893)?;
> +
> +        dev.c45_write(MDIO_MMD_PCS, 0x0028, 0xA528)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0x0029, 0x03)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC30A, 0x06E1)?;
> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0002)?;
> +        dev.c45_write(MDIO_MMD_PCS, 0xE854, 0x00C0)?;

It might be nicer to put this in a table, like

    const QT2025_INIT_ROUTINE: &[(u8, u16, u16)] = &[
        // Add some notes about what the registers do, or put them in
separate consts
        (MDIO_MMD_PMAPMD, 0xC300, 0x0000),
        (MDIO_MMD_PMAPMD, 0xC302, 0x4),
        // ...
    ];

    for (add reg, val) in QT2025_INIT_ROUTINE {
        dev.c45_write(add, reg, val)?;
    }

> +        let mut j = 0x8000;

Could you give this one a name?

> +        let mut a = MDIO_MMD_PCS;
> +        for (i, val) in fw.data().iter().enumerate() {
> +            if i == 0x4000 {
> +                a = MDIO_MMD_PHYXS;
> +                j = 0x8000;
> +            }

Looks like firmware is split between PCS and PHYXS at 0x4000, but like
Greg said you should probably explain where this comes from.

> +            dev.c45_write(a, j, (*val).into())?;

I think this is writing one byte at a time, to answer Andrew's
question. Can you write a `u16::from_le_bytes(...)` to alternating
addresses instead? This would be pretty easy by doing
`fw.data().chunks(2)`.

> +
> +            j += 1;
> +        }
> +        dev.c45_write(MDIO_MMD_PCS, 0xe854, 0x0040)?;
> +
> +        Ok(())
> +    }
> +
> +    fn read_status(dev: &mut phy::Device) -> Result<u16> {
> +        dev.genphy_c45_read_status()
> +    }
> +}

Re: [PATCH net-next v1 4/4] net: phy: add Applied Micro QT2025 PHY driver

[Benno Lossin]

On 16.04.24 06:34, Trevor Gross wrote:
> On Mon, Apr 15, 2024 at 6:47 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>> +struct PhyQT2025;
>> +
>> +#[vtable]
>> +impl Driver for PhyQT2025 {
>> +    const NAME: &'static CStr = c_str!("QT2025 10Gpbs SFP+");
> 
> Since 1.77 we have C string literals, `c"QT2025 10Gpbs SFP+"` (woohoo)

We have our own `CStr` in the `kernel::str::CStr`, so we cannot replace
the `c_str!` macro by the literal directly. Instead we also need to
remove our own `CStr`. We already have an issue for that:
https://github.com/Rust-for-Linux/linux/issues/1075

-- 
Cheers,
Benno

Re: [PATCH net-next v1 4/4] net: phy: add Applied Micro QT2025 PHY driver

[FUJITA Tomonori]

Hi,

On Tue, 16 Apr 2024 06:58:38 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 16.04.24 06:34, Trevor Gross wrote:
>> On Mon, Apr 15, 2024 at 6:47 AM FUJITA Tomonori
>> <fujita.tomonori@gmail.com> wrote:
>>> +struct PhyQT2025;
>>> +
>>> +#[vtable]
>>> +impl Driver for PhyQT2025 {
>>> +    const NAME: &'static CStr = c_str!("QT2025 10Gpbs SFP+");
>> 
>> Since 1.77 we have C string literals, `c"QT2025 10Gpbs SFP+"` (woohoo)
> 
> We have our own `CStr` in the `kernel::str::CStr`, so we cannot replace
> the `c_str!` macro by the literal directly. Instead we also need to
> remove our own `CStr`. We already have an issue for that:
> https://github.com/Rust-for-Linux/linux/issues/1075

Seems that someone already started to work on this issue so I'll keep
this alone for now.

Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[FUJITA Tomonori]

Hi,

On Mon, 15 Apr 2024 16:20:16 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +    /// Reads a given C45 PHY register.
>> +    /// This function reads a hardware register and updates the stats so takes `&mut self`.
>> +    pub fn c45_read(&mut self, devad: u8, regnum: u16) -> Result<u16> {
>> +        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.
>> +        let ret = unsafe {
>> +            bindings::mdiobus_c45_read(
>> +                (*phydev).mdio.bus,
>> +                (*phydev).mdio.addr,
>> +                devad as i32,
>> +                regnum.into(),
>> +            )
> 
> So you have wrapped the C function mdiobus_c45_read(). This is going
> to do a C45 bus protocol read. For this to work, the MDIO bus master
> needs to support C45 bus protocol, and the PHY also needs to support
> the C45 bus protocol. Not all MDIO bus masters and PHY devices
> do. Some will need to use C45 over C22.
> 
> A PHY driver should know if a PHY device supports C45 bus protocol,
> and if it supports C45 over C22. However, i PHY driver has no idea
> what the bus master supports.
> 
> In phylib, we have a poorly defined phydev->is_c45. Its current
> meaning is: "The device was found using the C45 bus protocol, not C22
> protocol". One of the things phylib core then uses is_c45 for it to
> direct the C functions phy_read_mmd() and phy_write_mmd() to perform a
> C45 bus protocol access if true. If it is false, C45 over C22 is
> performed instead. As a result, if a PHY is discovered using C22, C45
> register access is then performed using C45 over C22, even thought the
> PHY and bus might support C45 bus protocol. is_c45 is also used in
> other places, e.g. to trigger auto-negotiation using C45 registers,
> not C22 registers.

Thanks a lot for the details!


> In summary, the C API is a bit of a mess.
> 
> For the Rust API we have two sensible choices:
> 
> 1) It is the same mess as the C API, so hopefully one day we will fix
>    both at the same time.
> 
> 2) We define a different API which correctly separate C45 bus access
>    from C45 registers.
> 
> How you currently defined the Rust API is neither of these.

Which do your prefer?

If I correctly understand the original driver code, C45 bus protocol
is used. Adding functions for C45 bus protocol read/write would be
enough for this driver, I guess.

Re: [PATCH net-next v1 4/4] net: phy: add Applied Micro QT2025 PHY driver

[Andrew Lunn]

> > +        let mut a = MDIO_MMD_PCS;
> > +        for (i, val) in fw.data().iter().enumerate() {
> > +            if i == 0x4000 {
> > +                a = MDIO_MMD_PHYXS;
> > +                j = 0x8000;
> > +            }
> 
> Looks like firmware is split between PCS and PHYXS at 0x4000, but like
> Greg said you should probably explain where this comes from.
> 
> > +            dev.c45_write(a, j, (*val).into())?;
> 
> I think this is writing one byte at a time, to answer Andrew's
> question. Can you write a `u16::from_le_bytes(...)` to alternating
> addresses instead? This would be pretty easy by doing
> `fw.data().chunks(2)`.

That probably does not work, given my understanding of what is going
on. A C45 register is a u16.

The data sheet says:

  The 24kB of program memory space is accessible by
  MDIO. The first 16kB of memory is located in the
  address range 3.8000h - 3.BFFFh. The next 8kB of
  memory is located at 4.8000h - 4.9FFFh.

0x3bfff-0x3800 = 0x0x3fff = 16K.

So there are 16K u16 registers mapped onto 16K bytes of SRAM. So each
register holds one byte. Trying to write two bytes every other
register is not something which the datasheet talks about. So i doubt
it will work. Which is shame, because it would double the download
speed.

	Andrew

Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[Andrew Lunn]

> > In summary, the C API is a bit of a mess.
> > 
> > For the Rust API we have two sensible choices:
> > 
> > 1) It is the same mess as the C API, so hopefully one day we will fix
> >    both at the same time.
> > 
> > 2) We define a different API which correctly separate C45 bus access
> >    from C45 registers.
> > 
> > How you currently defined the Rust API is neither of these.
> 
> Which do your prefer?
> 
> If I correctly understand the original driver code, C45 bus protocol
> is used. Adding functions for C45 bus protocol read/write would be
> enough for this driver, I guess.

Now i've read more of the patches, i can see that the MDIO bus master
is C45 only. At least, that is all that is implemented in the
driver. So for this combination of MAC and PHY, forcing C45 register
access using C45 bus protocol will work.

However, can you combine this PHY with some other MDIO bus master,
which does not support C45? Then C45 over C22 would need to be used?
Looking at the data sheet i found online, there is no suggestion it
does support C22 bus protocol. All the diagrams/tables only show C45
bus protocol.

So this PHY is a special case. So you can use wrapper methods which
force C45 bus protocol, and ignore phylib support for performing C45
over C22 when needed. However, while doing this:

1: Clearly document that these helpers are not generic, they force C45
   register access using C45 bus protocol, and should only by used PHY
   drivers which know the PHY device does not support C45 over C22

2: Think about naming. At some point we are going to add the generic
   helpers for accessing C45 registers which leave the core to decide
   if to perform a C45 bus protocol access or C45 over C22. Those
   generic helpers need to have the natural name for accessing a C45
   register since 99% of drivers will be using them. The helpers you
   add now need to use a less common name.

	Andrew

Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[FUJITA Tomonori]

Hi,

On Tue, 16 Apr 2024 14:38:11 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> If I correctly understand the original driver code, C45 bus protocol
>> is used. Adding functions for C45 bus protocol read/write would be
>> enough for this driver, I guess.
> 
> Now i've read more of the patches, i can see that the MDIO bus master
> is C45 only. At least, that is all that is implemented in the
> driver. So for this combination of MAC and PHY, forcing C45 register
> access using C45 bus protocol will work.

Thanks a lot!

> However, can you combine this PHY with some other MDIO bus master,
> which does not support C45? Then C45 over C22 would need to be used?
> Looking at the data sheet i found online, there is no suggestion it
> does support C22 bus protocol. All the diagrams/tables only show C45
> bus protocol.

qt2025_ds3014.pdf?

> So this PHY is a special case. So you can use wrapper methods which
> force C45 bus protocol, and ignore phylib support for performing C45
> over C22 when needed. However, while doing this:
> 
> 1: Clearly document that these helpers are not generic, they force C45
>    register access using C45 bus protocol, and should only by used PHY
>    drivers which know the PHY device does not support C45 over C22
> 
> 2: Think about naming. At some point we are going to add the generic
>    helpers for accessing C45 registers which leave the core to decide
>    if to perform a C45 bus protocol access or C45 over C22. Those
>    generic helpers need to have the natural name for accessing a C45
>    register since 99% of drivers will be using them. The helpers you
>    add now need to use a less common name.

Sounds like we should save the names of c45_read and c45_write.

read_with_c45_bus_protocol and write_with_c45_bus_protocol?

They call mdiobus_c45_*, right?

Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[Benno Lossin]

On 16.04.24 15:21, FUJITA Tomonori wrote:
> Hi,
> 
> On Tue, 16 Apr 2024 14:38:11 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
>>> If I correctly understand the original driver code, C45 bus protocol
>>> is used. Adding functions for C45 bus protocol read/write would be
>>> enough for this driver, I guess.
>>
>> Now i've read more of the patches, i can see that the MDIO bus master
>> is C45 only. At least, that is all that is implemented in the
>> driver. So for this combination of MAC and PHY, forcing C45 register
>> access using C45 bus protocol will work.
> 
> Thanks a lot!
> 
>> However, can you combine this PHY with some other MDIO bus master,
>> which does not support C45? Then C45 over C22 would need to be used?
>> Looking at the data sheet i found online, there is no suggestion it
>> does support C22 bus protocol. All the diagrams/tables only show C45
>> bus protocol.
> 
> qt2025_ds3014.pdf?
> 
>> So this PHY is a special case. So you can use wrapper methods which
>> force C45 bus protocol, and ignore phylib support for performing C45
>> over C22 when needed. However, while doing this:
>>
>> 1: Clearly document that these helpers are not generic, they force C45
>>     register access using C45 bus protocol, and should only by used PHY
>>     drivers which know the PHY device does not support C45 over C22
>>
>> 2: Think about naming. At some point we are going to add the generic
>>     helpers for accessing C45 registers which leave the core to decide
>>     if to perform a C45 bus protocol access or C45 over C22. Those
>>     generic helpers need to have the natural name for accessing a C45
>>     register since 99% of drivers will be using them. The helpers you
>>     add now need to use a less common name.
> 
> Sounds like we should save the names of c45_read and c45_write.
> 
> read_with_c45_bus_protocol and write_with_c45_bus_protocol?
> 
> They call mdiobus_c45_*, right?

I think we could also do a more rusty solution. For example we could
have these generic functions for `phy::Device`:

     fn read_register<R: Register>(&mut self, which: R::Index) -> Result<R::Value>;
     fn write_register<R: Register>(&mut self, which: R::Index, value: R::Value) -> Result;

That way we can support many different registers without polluting the
namespace. We can then have a `C45` register and a `C22` register and a
`C45OrC22` (maybe we should use `C45_OrC22` instead, since I can read
that better, but let's bikeshed when we have the actual patch).

Calling those functions would look like this:

     let value = dev.read_register::<C45>(reg1)?;
     dev.write_register::<C45>(reg2, value)?;

-- 
Cheers,
Benno

Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[Andrew Lunn]

> I think we could also do a more rusty solution. For example we could
> have these generic functions for `phy::Device`:
> 
>      fn read_register<R: Register>(&mut self, which: R::Index) -> Result<R::Value>;
>      fn write_register<R: Register>(&mut self, which: R::Index, value: R::Value) -> Result;
> 
> That way we can support many different registers without polluting the
> namespace. We can then have a `C45` register and a `C22` register and a
> `C45OrC22` (maybe we should use `C45_OrC22` instead, since I can read
> that better, but let's bikeshed when we have the actual patch).
> 
> Calling those functions would look like this:
> 
>      let value = dev.read_register::<C45>(reg1)?;
>      dev.write_register::<C45>(reg2, value)?;

I don't know how well that will work out in practice. The addressing
schemes for C22 and C45 are different.

C22 simply has 32 registers, numbered 0-31.

C45 has 32 MDIO manageable devices (MMD) each with 65536 registers.

All of the 32 C22 registers have well defined names, which are listed
in mii.h. Ideally we want to keep the same names. The most of the MMD
also have defined names, listed in mdio.h. Many of the registers are
also named in mdio.h, although vendors do tend to add more vendor
proprietary registers.

Your R::Index would need to be a single value for C22 and a tuple of
two values for C45.

There is nothing like `C45OrC22`. A register is either in C22
namespace, or in the C45 namespace.

Where it gets interesting is that there are two methods to access the
C45 register namespace. The PHY driver should not care about this, it
is the MDIO bus driver and the PHYLIB core which handles the two
access mechanisms.

       Andrew

Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[Benno Lossin]

On 17.04.24 00:30, Andrew Lunn wrote:
>> I think we could also do a more rusty solution. For example we could
>> have these generic functions for `phy::Device`:
>>
>>      fn read_register<R: Register>(&mut self, which: R::Index) -> Result<R::Value>;
>>      fn write_register<R: Register>(&mut self, which: R::Index, value: R::Value) -> Result;
>>
>> That way we can support many different registers without polluting the
>> namespace. We can then have a `C45` register and a `C22` register and a
>> `C45OrC22` (maybe we should use `C45_OrC22` instead, since I can read
>> that better, but let's bikeshed when we have the actual patch).
>>
>> Calling those functions would look like this:
>>
>>      let value = dev.read_register::<C45>(reg1)?;
>>      dev.write_register::<C45>(reg2, value)?;
> 
> I don't know how well that will work out in practice. The addressing
> schemes for C22 and C45 are different.
> 
> C22 simply has 32 registers, numbered 0-31.
> 
> C45 has 32 MDIO manageable devices (MMD) each with 65536 registers.
> 
> All of the 32 C22 registers have well defined names, which are listed
> in mii.h. Ideally we want to keep the same names. The most of the MMD
> also have defined names, listed in mdio.h. Many of the registers are
> also named in mdio.h, although vendors do tend to add more vendor
> proprietary registers.
> 
> Your R::Index would need to be a single value for C22 and a tuple of
> two values for C45.

Yes that was my idea:

    enum C22Index {
        // The unique 32 names of the C22 registers...
    }

    impl Register for C22 {
        type Index = C22Index;
        type Value = u16;
    }

    impl Register for C45 {
        type Index = (u8, u16); // We could also create a newtype that wraps this and give it a better name.
        type Value = u16;
    }

You can then do:

    let val = dev.read_register::<C45>((4, 406))?;
    dev.write_register::<C22>(4, val)?;

If you only have these two registers types, then I would say this is
overkill, but I assumed that there are more.

> There is nothing like `C45OrC22`. A register is either in C22
> namespace, or in the C45 namespace.

I see, I got this idea from:

> [...] At some point we are going to add the generic
> helpers for accessing C45 registers which leave the core to decide
> if to perform a C45 bus protocol access or C45 over C22. [...]

Is this not accessing a C45 register via C22 and letting phylib decide?

> Where it gets interesting is that there are two methods to access the
> C45 register namespace. The PHY driver should not care about this, it
> is the MDIO bus driver and the PHYLIB core which handles the two
> access mechanisms.

If the driver shouldn't be concerned with how the access gets handled,
why do we even have a naming problem?

-- 
Cheers,
Benno

Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[Andrew Lunn]

> If the driver shouldn't be concerned with how the access gets handled,
> why do we even have a naming problem?

History.

The current C code does not cleanly separate register spaces from
access mechanisms.

C22 register space is simple, you can only access it using C22 bus
protocol. However C45 register space can be accessed in two ways,
either using C45 bus protocol, or using C45 over C22. The driver
should not care, it just wants to read/write a C45 register.  But the
current core mixes the two concepts of C45 register space and access
mechanisms. There have been a few attempts to clean this up, but
nothing landed yet.

Now this driver is somewhat special. The PHY itself only implements
one of the two access mechanisms, C45 bus protocol. So this driver
could side-step this mess and define access functions which go
straight to C45 bus protocol. However, some day a non-special
device/driver will come along, and we will need the generic access
functions, which leave the core to decide on C45 bus protocol or C45
over C22. Ideally these generic functions should have the natural name
for accessing C45 registers, and the special case in this driver
should use a different name.

       Andrew

Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[Benno Lossin]

On 17.04.24 15:34, Andrew Lunn wrote:
>> If the driver shouldn't be concerned with how the access gets handled,
>> why do we even have a naming problem?
> 
> History.
> 
> The current C code does not cleanly separate register spaces from
> access mechanisms.
> 
> C22 register space is simple, you can only access it using C22 bus
> protocol. However C45 register space can be accessed in two ways,
> either using C45 bus protocol, or using C45 over C22. The driver
> should not care, it just wants to read/write a C45 register.  But the
> current core mixes the two concepts of C45 register space and access
> mechanisms. There have been a few attempts to clean this up, but
> nothing landed yet.
> 
> Now this driver is somewhat special. The PHY itself only implements
> one of the two access mechanisms, C45 bus protocol. So this driver
> could side-step this mess and define access functions which go
> straight to C45 bus protocol. However, some day a non-special
> device/driver will come along, and we will need the generic access
> functions, which leave the core to decide on C45 bus protocol or C45
> over C22. Ideally these generic functions should have the natural name
> for accessing C45 registers, and the special case in this driver
> should use a different name.

Thanks for the explanation. What about having the following register
representing types:
- `C22` accesses a C22 register
- `C45` accesses a C45 register using whatever method phylib decides
- `C45Bus` accesses a C45 register over the C45 bus protocol (or
   `C45Direct`)

Or are you opposed to the idea of accessing any type of register via
`dev.read_register::<RegType>(..)`?

-- 
Cheers,
Benno

Re: [PATCH net-next v1 3/4] rust: net::phy support Firmware API

[FUJITA Tomonori]

Hi,

On Mon, 15 Apr 2024 13:10:59 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Mon, Apr 15, 2024 at 07:47:00PM +0900, FUJITA Tomonori wrote:
>> This patch adds support to the following basic Firmware API:
>> 
>> - request_firmware
>> - release_firmware
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> CC: Luis Chamberlain <mcgrof@kernel.org>
>> CC: Russ Weight <russ.weight@linux.dev>
>> ---
>>  drivers/net/phy/Kconfig         |  1 +
>>  rust/bindings/bindings_helper.h |  1 +
>>  rust/kernel/net/phy.rs          | 45 +++++++++++++++++++++++++++++++++
> 
> Please do not bury this in the phy.rs file, put it in drivers/base/ next
> to the firmware functions it is calling.

Sure. I had a version of creating rust/kernel/firmware.rs but I wanted
to know if a temporary solution could be accepted.

With the build system for Rust, we can't put it in drivers/base/ yet.


>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>> index 421a231421f5..095dc3ccc553 100644
>> --- a/rust/kernel/net/phy.rs
>> +++ b/rust/kernel/net/phy.rs
>> @@ -9,6 +9,51 @@
>>  use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
>>  
>>  use core::marker::PhantomData;
>> +use core::ptr::{self, NonNull};
>> +
>> +/// A pointer to the kernel's `struct firmware`.
>> +///
>> +/// # Invariants
>> +///
>> +/// The pointer points at a `struct firmware`, and has ownership over the object.
>> +pub struct Firmware(NonNull<bindings::firmware>);
>> +
>> +impl Firmware {
>> +    /// Loads a firmware.
>> +    pub fn new(name: &CStr, dev: &Device) -> Result<Firmware> {
>> +        let phydev = dev.0.get();
> 
> That's risky, how do you "know" this device really is a phydev?

That's guaranteed. The above `Device` is phy::Device.

> That's not how the firmware api works, use a real 'struct device' please.

Right, I'll do in v2.

> 
>> +        let mut ptr: *mut bindings::firmware = ptr::null_mut();
>> +        let p_ptr: *mut *mut bindings::firmware = &mut ptr;
> 
> I'm sorry, but I don't understand the two step thing here, you need a
> pointer for where the C code can put something, is this really how you
> do that in rust?  Shouldn't it point to data somehow down below?

Oops, can be simpler:

let mut ptr: *const bindings::firmware = ptr::null_mut();
let ret = unsafe {
        bindings::request_firmware(&mut ptr, name.as_char_ptr().cast(), &mut (*phydev).mdio.dev)
};

>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
> 
> Again, phydev is not part of the firmware api.
> 
>> +        // So it's just an FFI call.
>> +        let ret = unsafe {
>> +            bindings::request_firmware(
>> +                p_ptr as *mut *const bindings::firmware,
> 
> Where is this coming from?
> 
>> +                name.as_char_ptr().cast(),
>> +                &mut (*phydev).mdio.dev,
>> +            )
>> +        };
>> +        let fw = NonNull::new(ptr).ok_or_else(|| Error::from_errno(ret))?;
>> +        // INVARIANT: We checked that the firmware was successfully loaded.
>> +        Ok(Firmware(fw))
>> +    }
>> +
>> +    /// Accesses the firmware contents.
>> +    pub fn data(&self) -> &[u8] {
> 
> But firmware is not a u8, it's a structure.  Can't the rust bindings
> handle that?  Oh wait, data is a u8, but the bindings should show that,
> right?

In the C side:

struct firmware {
        size_t size;
        const u8 *data;
        /* firmware loader private fields */
        void *priv;
};

data() function allows a driver in Rust to access to the above data
member in Rust.

A driver could define its own structure over &[u8]. 

> 
>> +        // SAFETY: The type invariants guarantee that `self.0.as_ptr()` is valid.
>> +        // They also guarantee that `self.0.as_ptr().data` pointers to
>> +        // a valid memory region of size `self.0.as_ptr().size`.
>> +        unsafe { core::slice::from_raw_parts((*self.0.as_ptr()).data, (*self.0.as_ptr()).size) }
> 
> If new fails, does accessing this also fail?

If a new() fails, a Firmware object isn't created. So data() function
cannot be called.


> And how are you using this?  I guess I'll dig in the next patch...
> 
>> +    }
>> +}
>> +
>> +impl Drop for Firmware {
>> +    fn drop(&mut self) {
>> +        // SAFETY: By the type invariants, `self.0.as_ptr()` is valid and
>> +        // we have ownership of the object so can free it.
>> +        unsafe { bindings::release_firmware(self.0.as_ptr()) }
> 
> So drop will never be called if new fails?

Yes, like data() function.

Re: [PATCH net-next v1 4/4] net: phy: add Applied Micro QT2025 PHY driver

[FUJITA Tomonori]

Hi,

On Mon, 15 Apr 2024 13:15:08 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

>> +kernel::module_phy_driver! {
>> +    drivers: [PhyQT2025],
>> +    device_table: [
>> +        DeviceId::new_with_driver::<PhyQT2025>(),
>> +    ],
>> +    name: "qt2025_phy",
>> +    author: "FUJITA Tomonori <fujita.tomonori@gmail.com>",
>> +    description: "AMCC QT2025 PHY driver",
>> +    license: "GPL",
>> +}
> 
> What about support for MODULE_FIRMWARE() so it will be properly loaded
> into the initramfs of systems now that you are needing it for this
> driver?  To ignore that is going to cause problems :(

Oops, right. I'll work on it.


>> +const MDIO_MMD_PMAPMD: u8 = uapi::MDIO_MMD_PMAPMD as u8;
>> +const MDIO_MMD_PCS: u8 = uapi::MDIO_MMD_PCS as u8;
>> +const MDIO_MMD_PHYXS: u8 = uapi::MDIO_MMD_PHYXS as u8;
>> +
>> +struct PhyQT2025;
>> +
>> +#[vtable]
>> +impl Driver for PhyQT2025 {
>> +    const NAME: &'static CStr = c_str!("QT2025 10Gpbs SFP+");
>> +    const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x0043A400);
>> +
>> +    fn config_init(dev: &mut phy::Device) -> Result<()> {
>> +        let fw = Firmware::new(c_str!("qt2025-2.0.3.3.fw"), dev)?;
>> +
>> +        let phy_id = dev.c45_read(MDIO_MMD_PMAPMD, 0xd001)?;
>> +        if (phy_id >> 8) & 0xff != 0xb3 {
>> +            return Ok(());
>> +        }
>> +
>> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0000)?;
>> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC302, 0x4)?;
>> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC319, 0x0038)?;
>> +
>> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC31A, 0x0098)?;
>> +        dev.c45_write(MDIO_MMD_PCS, 0x0026, 0x0E00)?;
>> +
>> +        dev.c45_write(MDIO_MMD_PCS, 0x0027, 0x0893)?;
>> +
>> +        dev.c45_write(MDIO_MMD_PCS, 0x0028, 0xA528)?;
>> +        dev.c45_write(MDIO_MMD_PCS, 0x0029, 0x03)?;
>> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC30A, 0x06E1)?;
>> +        dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0002)?;
>> +        dev.c45_write(MDIO_MMD_PCS, 0xE854, 0x00C0)?;
>> +
>> +        let mut j = 0x8000;
>> +        let mut a = MDIO_MMD_PCS;
>> +        for (i, val) in fw.data().iter().enumerate() {
> 
> So you are treating the firmware image as able to be iterated over here?

It's Rust way to do the original C code:

for (i = 0; i < the_size_of_fw; i++) {
     // write fw_data[i] to a register.
     

>> +            if i == 0x4000 {
> 
> What does 0x4000 mean here?
> 
>> +                a = MDIO_MMD_PHYXS;
>> +                j = 0x8000;
> 
> What does 0x8000 mean here?
> 
>> +            }
>> +            dev.c45_write(a, j, (*val).into())?;
>> +
>> +            j += 1;
>> +        }
>> +        dev.c45_write(MDIO_MMD_PCS, 0xe854, 0x0040)?;
> 
> Lots of magic values in this driver, is that intentional?

The original driver uses lots of magic values. I simply use them. As
Andrew wrote, we could infer some. I'll try to comment these.

Re: [PATCH net-next v1 3/4] rust: net::phy support Firmware API

[Greg KH]

On Thu, Apr 18, 2024 at 09:51:08PM +0900, FUJITA Tomonori wrote:
> Hi,
> 
> On Mon, 15 Apr 2024 13:10:59 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Mon, Apr 15, 2024 at 07:47:00PM +0900, FUJITA Tomonori wrote:
> >> This patch adds support to the following basic Firmware API:
> >> 
> >> - request_firmware
> >> - release_firmware
> >> 
> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> >> CC: Luis Chamberlain <mcgrof@kernel.org>
> >> CC: Russ Weight <russ.weight@linux.dev>
> >> ---
> >>  drivers/net/phy/Kconfig         |  1 +
> >>  rust/bindings/bindings_helper.h |  1 +
> >>  rust/kernel/net/phy.rs          | 45 +++++++++++++++++++++++++++++++++
> > 
> > Please do not bury this in the phy.rs file, put it in drivers/base/ next
> > to the firmware functions it is calling.
> 
> Sure. I had a version of creating rust/kernel/firmware.rs but I wanted
> to know if a temporary solution could be accepted.
> 
> With the build system for Rust, we can't put it in drivers/base/ yet.

What is the status of fixing that?

Re: [PATCH net-next v1 3/4] rust: net::phy support Firmware API

[Greg KH]

On Thu, Apr 18, 2024 at 09:51:08PM +0900, FUJITA Tomonori wrote:
> >> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> >> index 421a231421f5..095dc3ccc553 100644
> >> --- a/rust/kernel/net/phy.rs
> >> +++ b/rust/kernel/net/phy.rs
> >> @@ -9,6 +9,51 @@
> >>  use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
> >>  
> >>  use core::marker::PhantomData;
> >> +use core::ptr::{self, NonNull};
> >> +
> >> +/// A pointer to the kernel's `struct firmware`.
> >> +///
> >> +/// # Invariants
> >> +///
> >> +/// The pointer points at a `struct firmware`, and has ownership over the object.
> >> +pub struct Firmware(NonNull<bindings::firmware>);
> >> +
> >> +impl Firmware {
> >> +    /// Loads a firmware.
> >> +    pub fn new(name: &CStr, dev: &Device) -> Result<Firmware> {
> >> +        let phydev = dev.0.get();
> > 
> > That's risky, how do you "know" this device really is a phydev?
> 
> That's guaranteed. The above `Device` is phy::Device.

How are we supposed to know that from reading this diff?  I think you
all need to work on naming things better, as that's going to get _VERY_
confusing very quickly.

thanks,

greg k-h

Re: [PATCH net-next v1 3/4] rust: net::phy support Firmware API

[FUJITA Tomonori]

Hi,

On Mon, 15 Apr 2024 17:45:46 +0200
Danilo Krummrich <dakr@redhat.com> wrote:

> On 4/15/24 12:47, FUJITA Tomonori wrote:
>> This patch adds support to the following basic Firmware API:
>> - request_firmware
>> - release_firmware
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> CC: Luis Chamberlain <mcgrof@kernel.org>
>> CC: Russ Weight <russ.weight@linux.dev>
>> ---
>>   drivers/net/phy/Kconfig         |  1 +
>>   rust/bindings/bindings_helper.h |  1 +
>>   rust/kernel/net/phy.rs          | 45 +++++++++++++++++++++++++++++++++
>>   3 files changed, 47 insertions(+)
> 
> As Greg already mentioned, this shouldn't be implemented specifically
> for struct
> phy_device, but rather for a generic struct device.

Yeah, I have a version of creating rust/kernel/firmware.rs locally but
I wanted to know if a temporary solution could be accepted.


> In order to use them from your PHY driver, I think all you need to do
> is to implement
> AsRef<> for your phy::Device:
> 
> impl AsRef<device::Device> for Device {
>     fn as_ref(&self) -> &device::Device {
>         // SAFETY: By the type invariants, we know that `self.ptr` is non-null
>         and valid.
>         unsafe { device::Device::from_raw(&mut (*self.ptr).mdio.dev) }
>     }
> }

My implementation uses RawDevice trait in old rust branch (Wedson
implemented, I suppose):

https://github.com/Rust-for-Linux/linux/blob/18b7491480025420896e0c8b73c98475c3806c6f/rust/kernel/device.rs#L37

pub unsafe trait RawDevice {
    /// Returns the raw `struct device` related to `self`.
    fn raw_device(&self) -> *mut bindings::device;


Which is better?

Re: [PATCH net-next v1 4/4] net: phy: add Applied Micro QT2025 PHY driver

[Greg KH]

On Thu, Apr 18, 2024 at 10:00:47PM +0900, FUJITA Tomonori wrote:
> >> +            if i == 0x4000 {
> > 
> > What does 0x4000 mean here?
> > 
> >> +                a = MDIO_MMD_PHYXS;
> >> +                j = 0x8000;
> > 
> > What does 0x8000 mean here?
> > 
> >> +            }
> >> +            dev.c45_write(a, j, (*val).into())?;
> >> +
> >> +            j += 1;
> >> +        }
> >> +        dev.c45_write(MDIO_MMD_PCS, 0xe854, 0x0040)?;
> > 
> > Lots of magic values in this driver, is that intentional?
> 
> The original driver uses lots of magic values. I simply use them. As
> Andrew wrote, we could infer some. I'll try to comment these.

Wait, this is a rewrite of an existing driver in the tree into Rust?
Why is that happening again?  Why not a new one instead?

thanks,

greg k-h

Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[FUJITA Tomonori]

On Wed, 17 Apr 2024 08:20:25 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 17.04.24 00:30, Andrew Lunn wrote:
>>> I think we could also do a more rusty solution. For example we could
>>> have these generic functions for `phy::Device`:
>>>
>>>      fn read_register<R: Register>(&mut self, which: R::Index) -> Result<R::Value>;
>>>      fn write_register<R: Register>(&mut self, which: R::Index, value: R::Value) -> Result;
>>>
>>> That way we can support many different registers without polluting the
>>> namespace. We can then have a `C45` register and a `C22` register and a
>>> `C45OrC22` (maybe we should use `C45_OrC22` instead, since I can read
>>> that better, but let's bikeshed when we have the actual patch).
>>>
>>> Calling those functions would look like this:
>>>
>>>      let value = dev.read_register::<C45>(reg1)?;
>>>      dev.write_register::<C45>(reg2, value)?;
>> 
>> I don't know how well that will work out in practice. The addressing
>> schemes for C22 and C45 are different.
>> 
>> C22 simply has 32 registers, numbered 0-31.
>> 
>> C45 has 32 MDIO manageable devices (MMD) each with 65536 registers.
>> 
>> All of the 32 C22 registers have well defined names, which are listed
>> in mii.h. Ideally we want to keep the same names. The most of the MMD
>> also have defined names, listed in mdio.h. Many of the registers are
>> also named in mdio.h, although vendors do tend to add more vendor
>> proprietary registers.
>> 
>> Your R::Index would need to be a single value for C22 and a tuple of
>> two values for C45.
> 
> Yes that was my idea:
> 
>     enum C22Index {
>         // The unique 32 names of the C22 registers...
>     }
> 
>     impl Register for C22 {
>         type Index = C22Index;
>         type Value = u16;
>     }
> 
>     impl Register for C45 {
>         type Index = (u8, u16); // We could also create a newtype that wraps this and give it a better name.
>         type Value = u16;
>     }

C45 looks a bit odd to me because C45 has a member which isn't a
register. It's a value to specify which MMD you access to.

Re: [PATCH net-next v1 4/4] net: phy: add Applied Micro QT2025 PHY driver

[FUJITA Tomonori]

Hi,

On Thu, 18 Apr 2024 15:10:36 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Thu, Apr 18, 2024 at 10:00:47PM +0900, FUJITA Tomonori wrote:
>> >> +            if i == 0x4000 {
>> > 
>> > What does 0x4000 mean here?
>> > 
>> >> +                a = MDIO_MMD_PHYXS;
>> >> +                j = 0x8000;
>> > 
>> > What does 0x8000 mean here?
>> > 
>> >> +            }
>> >> +            dev.c45_write(a, j, (*val).into())?;
>> >> +
>> >> +            j += 1;
>> >> +        }
>> >> +        dev.c45_write(MDIO_MMD_PCS, 0xe854, 0x0040)?;
>> > 
>> > Lots of magic values in this driver, is that intentional?
>> 
>> The original driver uses lots of magic values. I simply use them. As
>> Andrew wrote, we could infer some. I'll try to comment these.
> 
> Wait, this is a rewrite of an existing driver in the tree into Rust?

No. As I exampled in the cover-letter of the patchset, the vendor
released drivers but they haven't merged into the tree. The vendor
went out of bushiness and the drivers have been abandoned.

Re: [PATCH net-next v1 3/4] rust: net::phy support Firmware API

[FUJITA Tomonori]

Hi,

On Thu, 18 Apr 2024 15:07:19 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Thu, Apr 18, 2024 at 09:51:08PM +0900, FUJITA Tomonori wrote:
>> >> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>> >> index 421a231421f5..095dc3ccc553 100644
>> >> --- a/rust/kernel/net/phy.rs
>> >> +++ b/rust/kernel/net/phy.rs
>> >> @@ -9,6 +9,51 @@
>> >>  use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
>> >>  
>> >>  use core::marker::PhantomData;
>> >> +use core::ptr::{self, NonNull};
>> >> +
>> >> +/// A pointer to the kernel's `struct firmware`.
>> >> +///
>> >> +/// # Invariants
>> >> +///
>> >> +/// The pointer points at a `struct firmware`, and has ownership over the object.
>> >> +pub struct Firmware(NonNull<bindings::firmware>);
>> >> +
>> >> +impl Firmware {
>> >> +    /// Loads a firmware.
>> >> +    pub fn new(name: &CStr, dev: &Device) -> Result<Firmware> {
>> >> +        let phydev = dev.0.get();
>> > 
>> > That's risky, how do you "know" this device really is a phydev?
>> 
>> That's guaranteed. The above `Device` is phy::Device.
> 
> How are we supposed to know that from reading this diff?  I think you
> all need to work on naming things better, as that's going to get _VERY_
> confusing very quickly.

I guess that usually a path to a module prevent confusion. For
example, a MAC driver could look like:

fn probe(dev: &mut pci::Device, _: pci::DeviceId) -> Result {
    let netdev = net::Device::new()?;
    let phydev = phy::Device::new()?;
    ...

    Ok(())
}

Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

[Andrew Lunn]

> Thanks for the explanation. What about having the following register
> representing types:
> - `C22` accesses a C22 register
> - `C45` accesses a C45 register using whatever method phylib decides
> - `C45Bus` accesses a C45 register over the C45 bus protocol (or
>    `C45Direct`)
> 
> Or are you opposed to the idea of accessing any type of register via
> `dev.read_register::<RegType>(..)`?

I'm not against this, it is good to use Rust features if they make the
code easier to understand.

We can bike shed C45Bus vs C45Direct vs ... . I would probably go for
C45Direct, because C45 over C22 is often called indirect.

	Andrew

Re: [PATCH net-next v1 4/4] net: phy: add Applied Micro QT2025 PHY driver

[Andrew Lunn]

> >> +            if i == 0x4000 {
> > 
> > What does 0x4000 mean here?
> > 
> >> +                a = MDIO_MMD_PHYXS;
> >> +                j = 0x8000;
> > 
> > What does 0x8000 mean here?
> > 
> >> +            }
> >> +            dev.c45_write(a, j, (*val).into())?;
> >> +
> >> +            j += 1;
> >> +        }
> >> +        dev.c45_write(MDIO_MMD_PCS, 0xe854, 0x0040)?;
> > 
> > Lots of magic values in this driver, is that intentional?
> 
> The original driver uses lots of magic values. I simply use them. As
> Andrew wrote, we could infer some. I'll try to comment these.

When you start from a Vendor crap driver, part of the process of
getting it into Mainline is getting it up to Mainline quality. If this
was C code, i would be trying to replace as many of the magic numbers
with #define. And then add comments about the best guess what things
are doing, based on the datasheet. The data sheet however does not
explain all the bits, nor give every register a name. But you should
use as much information as possible from the datasheet to make the
code as readable as possible.

	Andrew