| Message ID | 20240415104701.4772-5-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: add Applied Micro QT2025 PHY driver | expand |
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
> + 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
> +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
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() > + } > +}
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
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.
> > + 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
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.
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
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.
> >> + 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
Hi, Sorry for the long delay, On Tue, 16 Apr 2024 14:08:32 +0200 Andrew Lunn <andrew@lunn.ch> wrote: >> > + 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. Confirmed that it doesn't work. After some experiments, I found that the PHY on my environment works without the firmware loaded. So I'll remove the firmware code in v2. I assume that there are PHYs that need the firmware because the original driver loads the firmware. Thus I'll add the firmware support in the future after the abstractions for the firmware API are merged, which the Nova GPU team has been working on [1]. [1] https://lore.kernel.org/all/20240521212333.GA731457-robh@kernel.org/T/
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>
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