| Message ID | 20240820225719.91410-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 Tue, Aug 20, 2024 at 5:59 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> > --- > diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs > new file mode 100644 > index 000000000000..a1505ffca9f5 > --- /dev/null > +++ b/drivers/net/phy/qt2025.rs > @@ -0,0 +1,98 @@ > +// 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 > +//! > +//! This driver is based on the vendor driver `QT2025_phy.c`. This source > +//! and firmware can be downloaded on the EN-9320SFP+ support site. > +use kernel::c_str; Nit: line between module docs and the first import. Could you add another note to the doc comment that the phy contains an embedded Intel 8051 microcontroller? I was getting confused by the below comments mentioning the 8051 until I realized this. > +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_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); > + } I actually found this described in the datasheet for the QT2022: 1.D000h is a two-byte "product code", "1.D001h" is a one byte revision code followed by one byte reserved. So 0xb3 is presumably something like the major silicon revision number. Based on how the vendor code is written, it seems like they are expecting different phy revs to need different firmware. It might be worth making a note that our firmware only works with 0xb3, whatever exactly that means. The `& 0xff` shouldn't be needed since `dev.read` returns an unsigned number. I went through the datasheet and found some register names, listed them below. Maybe it is worth putting the names in the comments if they exist? Just to make things a bit more searchable if somebody pulls up a datasheet. > + // The Intel 8051 will remain in the reset state. > + dev.write(C45::new(Mmd::PMAPMD, 0xC300), 0x0000)?; This sets `MICRO_RESETN` to hold the embedded micro in reset while configuring. > + // Configure the 8051 clock frequency. > + dev.write(C45::new(Mmd::PMAPMD, 0xC302), 0x0004)?; This one is `SREFCLK_FREQ`, embedded micro clock frequency. I couldn't figure out what the meaning of the value is. > + // 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)?; This LAN/WAN select is called `CUS_LAN_WAN_CONFIG` > + // The following 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. > + 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)?; `MICRO_RESETN` again, this time to start the embedded micro. > + // 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 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; > + } > + // The Intel 8051 will start running from SRAM. > + dev.write(C45::new(Mmd::PCS, 0xE854), 0x0040)?; At this point the vendor driver looks like it does some verification: it attempts to read 3.d7fd until it returns something other than 0x10 or 0, or times out. Could that be done here? > + > + Ok(()) > + } > + > + fn read_status(dev: &mut phy::Device) -> Result<u16> { > + dev.genphy_read_status::<C45>() > + } > +} > -- > 2.34.1 > Consistency nit: this file uses a mix of upper and lowercase hex (mostly uppercase here) - we should probably be consistent. A quick regex search looks like lowercase hex is about twice as common in the kernel as uppercase so I think this may as well be updated. --- Overall this looks pretty good to me, checking against both the datasheet and the vendor driver we have. Mostly small suggestions here, I'm happy to add a RB with my verification question addressed and some rewording of the 0xd001 (phy revision) comment. - Trevor
On Fri, 23 Aug 2024 00:25:23 -0500 Trevor Gross <tmgross@umich.edu> wrote: >> +//! Applied Micro Circuits Corporation QT2025 PHY driver >> +//! >> +//! This driver is based on the vendor driver `QT2025_phy.c`. This source >> +//! and firmware can be downloaded on the EN-9320SFP+ support site. >> +use kernel::c_str; > > Nit: line between module docs and the first import. Oops, will fix. > Could you add another note to the doc comment that the phy contains an > embedded Intel 8051 microcontroller? I was getting confused by the > below comments mentioning the 8051 until I realized this. Sure, will add. >> +#[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); >> + } > > I actually found this described in the datasheet for the QT2022: > 1.D000h is a two-byte "product code", "1.D001h" is a one byte revision > code followed by one byte reserved. So 0xb3 is presumably something > like the major silicon revision number. Thanks! I've not checked the QT2022 datasheet. Looks like both registers are compatible with QT2025. > Based on how the vendor code is written, it seems like they are > expecting different phy revs to need different firmware. It might be > worth making a note that our firmware only works with 0xb3, whatever > exactly that means. I'll update the comment. > The `& 0xff` shouldn't be needed since `dev.read` returns an unsigned number. Yeah, looks like unnecessary. We need the upper 8 bits of u16 here. I'll drop it. > I went through the datasheet and found some register names, listed > them below. Maybe it is worth putting the names in the comments if > they exist? Just to make things a bit more searchable if somebody > pulls up a datasheet. Sure, I'll add them. >> + // The Intel 8051 will remain in the reset state. >> + dev.write(C45::new(Mmd::PMAPMD, 0xC300), 0x0000)?; > > This sets `MICRO_RESETN` to hold the embedded micro in reset while configuring. > >> + // Configure the 8051 clock frequency. >> + dev.write(C45::new(Mmd::PMAPMD, 0xC302), 0x0004)?; > > This one is `SREFCLK_FREQ`, embedded micro clock frequency. I couldn't > figure out what the meaning of the value is. > >> + // 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)?; > > This LAN/WAN select is called `CUS_LAN_WAN_CONFIG` > >> + // The following 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. >> + 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)?; > > `MICRO_RESETN` again, this time to start the embedded micro. > >> + // 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 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; >> + } >> + // The Intel 8051 will start running from SRAM. >> + dev.write(C45::new(Mmd::PCS, 0xE854), 0x0040)?; > > > At this point the vendor driver looks like it does some verification: > it attempts to read 3.d7fd until it returns something other than 0x10 > or 0, or times out. Could that be done here? Yeah, we better to wait here until the hw becomes ready (since the 8051 has just started) and check if it works correctly. A new Rust abstraction for msleep() is necessary. Even without the logic, the driver starts to work eventually (if the hw isn't broken) so I didn't include it in the patchset. I'll work on the abstraction and update the driver after this is merged. >> + >> + Ok(()) >> + } >> + >> + fn read_status(dev: &mut phy::Device) -> Result<u16> { >> + dev.genphy_read_status::<C45>() >> + } >> +} >> -- >> 2.34.1 >> > > Consistency nit: this file uses a mix of upper and lowercase hex > (mostly uppercase here) - we should probably be consistent. A quick > regex search looks like lowercase hex is about twice as common in the > kernel as uppercase so I think this may as well be updated. Ah, I'll use lowercase for all the hex in the driver. It will be a new coding rule for rust code in kernel? If so, can a checker tool warn this? > Overall this looks pretty good to me, checking against both the > datasheet and the vendor driver we have. Mostly small suggestions > here, I'm happy to add a RB with my verification question addressed > and some rewording of the 0xd001 (phy revision) comment. Thanks a lot! I'll send v7 soon.
On Fri, Aug 23, 2024 at 8:37 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > At this point the vendor driver looks like it does some verification: > > it attempts to read 3.d7fd until it returns something other than 0x10 > > or 0, or times out. Could that be done here? > > Yeah, we better to wait here until the hw becomes ready (since the > 8051 has just started) and check if it works correctly. A new Rust > abstraction for msleep() is necessary. > > Even without the logic, the driver starts to work eventually (if the > hw isn't broken) so I didn't include it in the patchset. I'll work on > the abstraction and update the driver after this is merged. It sounds okay to me to not block on this as long as it isn't glitchy - It should probably be a FIXME? > > Consistency nit: this file uses a mix of upper and lowercase hex > > (mostly uppercase here) - we should probably be consistent. A quick > > regex search looks like lowercase hex is about twice as common in the > > kernel as uppercase so I think this may as well be updated. > > Ah, I'll use lowercase for all the hex in the driver. > > It will be a new coding rule for rust code in kernel? If so, can a > checker tool warn this? I don't know of any rule for this, I just noticed that the patch had both and it made me take a look at what is used elsewhere. rustfmt has an option to just commonize it for you, `hex_literal_case` [1], but it is unstable. That would probably be nice at some point. > > Overall this looks pretty good to me, checking against both the > > datasheet and the vendor driver we have. Mostly small suggestions > > here, I'm happy to add a RB with my verification question addressed > > and some rewording of the 0xd001 (phy revision) comment. > > Thanks a lot! I'll send v7 soon. Thanks! - Trevor [1]: https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=#hex_literal_case
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..a1505ffca9f5 --- /dev/null +++ b/drivers/net/phy/qt2025.rs @@ -0,0 +1,98 @@ +// 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 +//! +//! This driver is based on the vendor driver `QT2025_phy.c`. This source +//! and firmware can be downloaded on the EN-9320SFP+ support site. +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_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 Intel 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)?; + // The following 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. + 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 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; + } + // The Intel 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 | 98 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 113 insertions(+) create mode 100644 drivers/net/phy/qt2025.rs