| Message ID | 20240819005345.84255-7-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: add Applied Micro QT2025 PHY driver | expand |
On Sun, Aug 18, 2024 at 8:01 PM 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> > --- > MAINTAINERS | 7 +++ > drivers/net/phy/Kconfig | 7 +++ > drivers/net/phy/Makefile | 1 + > drivers/net/phy/qt2025.rs | 90 +++++++++++++++++++++++++++++++++++++++ > 4 files changed, 105 insertions(+) > create mode 100644 drivers/net/phy/qt2025.rs > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9dbfcf77acb2..d4464e59dfea 100644 > +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 probe(dev: &mut phy::Device) -> Result<()> { > + // The vendor driver does the following checking but we have no idea why. In the module doc comment, could you add a note about where the vendor driver came from? I am not sure how to find it. > + let hw_id = dev.read(C45::new(Mmd::PMAPMD, 0xd001))?; > + if (hw_id >> 8) & 0xff != 0xb3 { > + return Err(code::ENODEV); > + } > + > + // The 8051 will remain in the reset state. What is the 8051 here? > + dev.write(C45::new(Mmd::PMAPMD, 0xC300), 0x0000)?; > + // Configure the 8051 clock frequency. > + dev.write(C45::new(Mmd::PMAPMD, 0xC302), 0x0004)?; > + // Non loopback mode. > + dev.write(C45::new(Mmd::PMAPMD, 0xC319), 0x0038)?; > + // Global control bit to select between LAN and WAN (WIS) mode. > + dev.write(C45::new(Mmd::PMAPMD, 0xC31A), 0x0098)?; > + dev.write(C45::new(Mmd::PCS, 0x0026), 0x0E00)?; > + dev.write(C45::new(Mmd::PCS, 0x0027), 0x0893)?; > + dev.write(C45::new(Mmd::PCS, 0x0028), 0xA528)?; > + dev.write(C45::new(Mmd::PCS, 0x0029), 0x0003)?; The above four writes should probably get a comment based on the discussion at [1]. > + // Configure transmit and recovered clock. > + dev.write(C45::new(Mmd::PMAPMD, 0xC30A), 0x06E1)?; > + // The 8051 will finish the reset state. > + dev.write(C45::new(Mmd::PMAPMD, 0xC300), 0x0002)?; > + // The 8051 will start running from the boot ROM. > + dev.write(C45::new(Mmd::PCS, 0xE854), 0x00C0)?; > + > + let fw = Firmware::request(c_str!("qt2025-2.0.3.3.fw"), dev.as_ref())?; I don't know if this works, but can you put `qt2025-2.0.3.3.fw` in a const to use both here and in the `module_phy_driver!` macro? > + if fw.data().len() > SZ_16K + SZ_8K { > + return Err(code::EFBIG); > + } > + > + // 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. > + let mut j = SZ_32K; > + for (i, val) in fw.data().iter().enumerate() { > + if i == SZ_16K { > + j = SZ_32K; > + } > + > + let mmd = if i < SZ_16K { Mmd::PCS } else { Mmd::PHYXS }; > + dev.write(C45::new(mmd, j as u16), (*val).into())?; > + > + j += 1; > + } Possibly: 1. Hint the MMD name in the comments 2. Give i and j descriptive names (I used `src_idx` and `dst_offset`) 3. Set `mmd` once at the same time you reset the address offset 4. Tracking the offset from 0 rather than from SZ_32K seems more readable E.g.: // 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 (PCS). // The next 8kB of memory is located at 4.8000h - 4.9FFFh (PHYXS). let mut dst_offset = 0; let mut dst_mmd = Mmd::PCS; for (src_idx, val) in fw.data().iter().enumerate() { if src_idx == SZ_16K { // Start writing to the next register with no offset dst_offset = 0; dst_mmd = Mmd::PHYXS; } dev.write(C45::new(dst_mmd, 0x8000 + dst_offset), (*val).into())?; dst_offset += 1; } Alternatively you could split the iterator with `.by_ref().take(SZ_16K)`, but that may not be more readable. > + // The 8051 will start running from SRAM. > + dev.write(C45::new(Mmd::PCS, 0xE854), 0x0040)?; > + > + Ok(()) > + } > + > + fn read_status(dev: &mut phy::Device) -> Result<u16> { > + dev.genphy_read_status::<C45>() > + } > +} Overall this looks pretty reasonable to me, I just don't know what to reference for the initialization sequence. - Trevor [1]: https://lore.kernel.org/netdev/0675cff9-5502-43e4-87ee-97d2e35d72da@lunn.ch/
On Mon, 19 Aug 2024 04:07:30 -0500 Trevor Gross <tmgross@umich.edu> wrote: >> +#[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 probe(dev: &mut phy::Device) -> Result<()> { >> + // The vendor driver does the following checking but we have no idea why. > > In the module doc comment, could you add a note about where the vendor > driver came from? I am not sure how to find it. For example, it's available at Edimax site: https://www.edimax.com/edimax/download/download/data/edimax/global/download/smb_network_adapters_pci_card/en-9320sfp_plus I could add it to the module comment but not sure if the URL will be available for for long-term use. How about uploading the code to github and adding the link? >> + let hw_id = dev.read(C45::new(Mmd::PMAPMD, 0xd001))?; >> + if (hw_id >> 8) & 0xff != 0xb3 { >> + return Err(code::ENODEV); >> + } >> + >> + // The 8051 will remain in the reset state. > > What is the 8051 here? Intel 8051. I'll update the comment. >> + dev.write(C45::new(Mmd::PMAPMD, 0xC300), 0x0000)?; >> + // Configure the 8051 clock frequency. >> + dev.write(C45::new(Mmd::PMAPMD, 0xC302), 0x0004)?; >> + // Non loopback mode. >> + dev.write(C45::new(Mmd::PMAPMD, 0xC319), 0x0038)?; >> + // Global control bit to select between LAN and WAN (WIS) mode. >> + dev.write(C45::new(Mmd::PMAPMD, 0xC31A), 0x0098)?; >> + dev.write(C45::new(Mmd::PCS, 0x0026), 0x0E00)?; >> + dev.write(C45::new(Mmd::PCS, 0x0027), 0x0893)?; >> + dev.write(C45::new(Mmd::PCS, 0x0028), 0xA528)?; >> + dev.write(C45::new(Mmd::PCS, 0x0029), 0x0003)?; > > The above four writes should probably get a comment based on the > discussion at [1]. // The following for writes use standardized registers (3.38 through // 3.41 5/10/25GBASE-R PCS test pattern seed B) for something else. // We don't know what. Looks good? >> + // Configure transmit and recovered clock. >> + dev.write(C45::new(Mmd::PMAPMD, 0xC30A), 0x06E1)?; >> + // The 8051 will finish the reset state. >> + dev.write(C45::new(Mmd::PMAPMD, 0xC300), 0x0002)?; >> + // The 8051 will start running from the boot ROM. >> + dev.write(C45::new(Mmd::PCS, 0xE854), 0x00C0)?; >> + >> + let fw = Firmware::request(c_str!("qt2025-2.0.3.3.fw"), dev.as_ref())?; > > I don't know if this works, but can you put `qt2025-2.0.3.3.fw` in a > const to use both here and in the `module_phy_driver!` macro? It doesn't work. Variables can't be used in the `module_phy_driver!` macro. > 1. Hint the MMD name in the comments > 2. Give i and j descriptive names (I used `src_idx` and `dst_offset`) > 3. Set `mmd` once at the same time you reset the address offset > 4. Tracking the offset from 0 rather than from SZ_32K seems more readable > > E.g.: > > // 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 (PCS). > // The next 8kB of memory is located at 4.8000h - 4.9FFFh (PHYXS). > let mut dst_offset = 0; > let mut dst_mmd = Mmd::PCS; > for (src_idx, val) in fw.data().iter().enumerate() { > if src_idx == SZ_16K { > // Start writing to the next register with no offset > dst_offset = 0; > dst_mmd = Mmd::PHYXS; > } > > dev.write(C45::new(dst_mmd, 0x8000 + dst_offset), (*val).into())?; > > dst_offset += 1; > } Surely, more readable. I'll update the code (I need to add #[derive(Copy, Clone)] to reg::Mmd with this change). > Alternatively you could split the iterator with > `.by_ref().take(SZ_16K)`, but that may not be more readable. > >> + // The 8051 will start running from SRAM. >> + dev.write(C45::new(Mmd::PCS, 0xE854), 0x0040)?; >> + >> + Ok(()) >> + } >> + >> + fn read_status(dev: &mut phy::Device) -> Result<u16> { >> + dev.genphy_read_status::<C45>() >> + } >> +} > > Overall this looks pretty reasonable to me, I just don't know what to > reference for the initialization sequence. You can find the initialization sequence of the vendor driver at: https://github.com/acooks/tn40xx-driver/blob/vendor-drop/v0.3.6.15/QT2025_phy.c Thanks a lot!
On Mon, Aug 19, 2024 at 7:20 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > On Mon, 19 Aug 2024 04:07:30 -0500 > Trevor Gross <tmgross@umich.edu> wrote: > > [...] > > In the module doc comment, could you add a note about where the vendor > > driver came from? I am not sure how to find it. > > For example, it's available at Edimax site: > > https://www.edimax.com/edimax/download/download/data/edimax/global/download/smb_network_adapters_pci_card/en-9320sfp_plus > > I could add it to the module comment but not sure if the URL will be > available for for long-term use. How about uploading the code to github > and adding the link? Great, thanks for the link. I don't even know that you need to include it directly, maybe something like //! //! This driver is based on the vendor driver `qt2025_phy.c` This source //! and firmware can be downloaded on the EN-9320SFP+ support site. so anyone reading in the future knows what to look for without relying on a link. But I don't know what the policy is here. > >> + dev.write(C45::new(Mmd::PCS, 0x0026), 0x0E00)?; > >> + dev.write(C45::new(Mmd::PCS, 0x0027), 0x0893)?; > >> + dev.write(C45::new(Mmd::PCS, 0x0028), 0xA528)?; > >> + dev.write(C45::new(Mmd::PCS, 0x0029), 0x0003)?; > > > > The above four writes should probably get a comment based on the > > discussion at [1]. > > // The following for writes use standardized registers (3.38 through > // 3.41 5/10/25GBASE-R PCS test pattern seed B) for something else. > // We don't know what. > > Looks good? Seems reasonable to me, thanks. > >> + // Configure transmit and recovered clock. > >> + dev.write(C45::new(Mmd::PMAPMD, 0xC30A), 0x06E1)?; > >> + // The 8051 will finish the reset state. > >> + dev.write(C45::new(Mmd::PMAPMD, 0xC300), 0x0002)?; > >> + // The 8051 will start running from the boot ROM. > >> + dev.write(C45::new(Mmd::PCS, 0xE854), 0x00C0)?; > >> + > >> + let fw = Firmware::request(c_str!("qt2025-2.0.3.3.fw"), dev.as_ref())?; > > > > I don't know if this works, but can you put `qt2025-2.0.3.3.fw` in a > > const to use both here and in the `module_phy_driver!` macro? > > It doesn't work. Variables can't be used in the `module_phy_driver!` > macro. Ah, that is unfortunate. Maybe we should try to fix that if the firmware name isn't actually needed at compile time (not here of course). > > E.g.: > > > > // 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 (PCS). > > // The next 8kB of memory is located at 4.8000h - 4.9FFFh (PHYXS). > > let mut dst_offset = 0; > > let mut dst_mmd = Mmd::PCS; > > for (src_idx, val) in fw.data().iter().enumerate() { > > if src_idx == SZ_16K { > > // Start writing to the next register with no offset > > dst_offset = 0; > > dst_mmd = Mmd::PHYXS; > > } > > > > dev.write(C45::new(dst_mmd, 0x8000 + dst_offset), (*val).into())?; > > > > dst_offset += 1; > > } > > Surely, more readable. I'll update the code (I need to add > #[derive(Copy, Clone)] to reg::Mmd with this change). Didn't think about that, but sounds reasonable. `C22` and `C45` as well probably, maybe `Debug` would come in handy in the future too. > > Alternatively you could split the iterator with > > `.by_ref().take(SZ_16K)`, but that may not be more readable. > > > >> + // The 8051 will start running from SRAM. > >> + dev.write(C45::new(Mmd::PCS, 0xE854), 0x0040)?; > >> + > >> + Ok(()) > >> + } > >> + > >> + fn read_status(dev: &mut phy::Device) -> Result<u16> { > >> + dev.genphy_read_status::<C45>() > >> + } > >> +} > > > > Overall this looks pretty reasonable to me, I just don't know what to > > reference for the initialization sequence. > > You can find the initialization sequence of the vendor driver at: > > https://github.com/acooks/tn40xx-driver/blob/vendor-drop/v0.3.6.15/QT2025_phy.c > > Thanks a lot! Thanks! I'll try to cross check against that code later. - Trevor
> //! This driver is based on the vendor driver `qt2025_phy.c` This source > //! and firmware can be downloaded on the EN-9320SFP+ support site. > > so anyone reading in the future knows what to look for without relying > on a link. But I don't know what the policy is here. Ideally, the firmware should be added to linux-firmware. It will then appear in most distros. That however requires there is a clear license which allows distribution. Andrew
On Tue, 20 Aug 2024 00:46:25 +0200 Andrew Lunn <andrew@lunn.ch> wrote: >> //! This driver is based on the vendor driver `qt2025_phy.c` This source >> //! and firmware can be downloaded on the EN-9320SFP+ support site. >> >> so anyone reading in the future knows what to look for without relying >> on a link. But I don't know what the policy is here. > > Ideally, the firmware should be added to linux-firmware. It will then > appear in most distros. That however requires there is a clear license > which allows distribution. The firmware is an array in the header file in the source code distributed under GPL. I plan to add it to linux-firmware after this driver is merged.
diff --git a/MAINTAINERS b/MAINTAINERS index 9dbfcf77acb2..d4464e59dfea 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1609,6 +1609,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 f530fcd092fe..01b235b3bb7e 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -112,6 +112,13 @@ 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 + depends on RUST_FW_LOADER_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 f086a606a87e..669d71959be4 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -37,6 +37,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..45864a7e1120 --- /dev/null +++ b/drivers/net/phy/qt2025.rs @@ -0,0 +1,90 @@ +// 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::error::code; +use kernel::firmware::Firmware; +use kernel::net::phy::{ + self, + reg::{Mmd, C45}, + DeviceId, Driver, +}; +use kernel::prelude::*; +use kernel::sizes::{SZ_16K, SZ_32K, SZ_8K}; + +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", + firmware: ["qt2025-2.0.3.3.fw"], +} + +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 probe(dev: &mut phy::Device) -> Result<()> { + // The vendor driver does the following checking but we have no idea why. + let hw_id = dev.read(C45::new(Mmd::PMAPMD, 0xd001))?; + if (hw_id >> 8) & 0xff != 0xb3 { + return Err(code::ENODEV); + } + + // The 8051 will remain in the reset state. + dev.write(C45::new(Mmd::PMAPMD, 0xC300), 0x0000)?; + // Configure the 8051 clock frequency. + dev.write(C45::new(Mmd::PMAPMD, 0xC302), 0x0004)?; + // Non loopback mode. + dev.write(C45::new(Mmd::PMAPMD, 0xC319), 0x0038)?; + // Global control bit to select between LAN and WAN (WIS) mode. + dev.write(C45::new(Mmd::PMAPMD, 0xC31A), 0x0098)?; + dev.write(C45::new(Mmd::PCS, 0x0026), 0x0E00)?; + dev.write(C45::new(Mmd::PCS, 0x0027), 0x0893)?; + dev.write(C45::new(Mmd::PCS, 0x0028), 0xA528)?; + dev.write(C45::new(Mmd::PCS, 0x0029), 0x0003)?; + // Configure transmit and recovered clock. + dev.write(C45::new(Mmd::PMAPMD, 0xC30A), 0x06E1)?; + // The 8051 will finish the reset state. + dev.write(C45::new(Mmd::PMAPMD, 0xC300), 0x0002)?; + // The 8051 will start running from the boot ROM. + dev.write(C45::new(Mmd::PCS, 0xE854), 0x00C0)?; + + let fw = Firmware::request(c_str!("qt2025-2.0.3.3.fw"), dev.as_ref())?; + if fw.data().len() > SZ_16K + SZ_8K { + return Err(code::EFBIG); + } + + // 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. + let mut j = SZ_32K; + for (i, val) in fw.data().iter().enumerate() { + if i == SZ_16K { + j = SZ_32K; + } + + let mmd = if i < SZ_16K { Mmd::PCS } else { Mmd::PHYXS }; + dev.write(C45::new(mmd, j as u16), (*val).into())?; + + j += 1; + } + // The 8051 will start running from SRAM. + dev.write(C45::new(Mmd::PCS, 0xE854), 0x0040)?; + + Ok(()) + } + + fn read_status(dev: &mut phy::Device) -> Result<u16> { + dev.genphy_read_status::<C45>() + } +}
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 | 7 +++ drivers/net/phy/Makefile | 1 + drivers/net/phy/qt2025.rs | 90 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 105 insertions(+) create mode 100644 drivers/net/phy/qt2025.rs