| Message ID | 20240817051939.77735-7-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 |
> + fn read_status(dev: &mut phy::Device) -> Result<u16> { > + dev.genphy_read_status::<C45>() > + } Probably a dumb Rust question. Shouldn't this have a ? at the end? It can return a negative error code. Andrew
On 17.08.24 20:51, Andrew Lunn wrote: >> + fn read_status(dev: &mut phy::Device) -> Result<u16> { >> + dev.genphy_read_status::<C45>() >> + } > > Probably a dumb Rust question. Shouldn't this have a ? at the end? It > can return a negative error code. `read_status` returns a `Result<u16>` and `Device::genphy_read_status` also returns a `Result<u16>`. In the function body we just delegate to the latter, so no `?` is needed. We just return the entire result. Here is the equivalent pseudo-C code: int genphy_read_status(struct phy_device *dev); int read_status(struct phy_device *dev) { return genphy_read_status(dev); } There you also don't need an if for the negative error code, since it's just propagated. Hope this helps! --- Cheers, Benno
On Sat, Aug 17, 2024 at 09:34:13PM +0000, Benno Lossin wrote: > On 17.08.24 20:51, Andrew Lunn wrote: > >> + fn read_status(dev: &mut phy::Device) -> Result<u16> { > >> + dev.genphy_read_status::<C45>() > >> + } > > > > Probably a dumb Rust question. Shouldn't this have a ? at the end? It > > can return a negative error code. > > `read_status` returns a `Result<u16>` and `Device::genphy_read_status` > also returns a `Result<u16>`. In the function body we just delegate to > the latter, so no `?` is needed. We just return the entire result. > > Here is the equivalent pseudo-C code: > > int genphy_read_status(struct phy_device *dev); > > int read_status(struct phy_device *dev) > { > return genphy_read_status(dev); > } > > There you also don't need an if for the negative error code, since it's > just propagated. O.K, it seems to work. But one of the things we try to think about in the kernel is avoiding future bugs. Say sometime in the future i extend it: fn read_status(dev: &mut phy::Device) -> Result<u16> { dev.genphy_read_status::<C45>() dev.genphy_read_foo() } By forgetting to add the ? to dev.genphy_read_status, have i just introduced a bug? Could i have avoided that by always having the ? even when it is not needed? Andrew
On 17.08.24 07:19, FUJITA Tomonori wrote: > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index 7fddc8306d82..e0ff386d90cd 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -109,6 +109,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 This is missing a depends on RUST_FW_LOADER_ABSTRACTIONS. --- Cheers, Benno > + help > + Adds support for the Applied Micro Circuits Corporation QT2025 PHY. > + > source "drivers/net/phy/aquantia/Kconfig"
On 18.08.24 17:44, Andrew Lunn wrote: > On Sat, Aug 17, 2024 at 09:34:13PM +0000, Benno Lossin wrote: >> On 17.08.24 20:51, Andrew Lunn wrote: >>>> + fn read_status(dev: &mut phy::Device) -> Result<u16> { >>>> + dev.genphy_read_status::<C45>() >>>> + } >>> >>> Probably a dumb Rust question. Shouldn't this have a ? at the end? It >>> can return a negative error code. >> >> `read_status` returns a `Result<u16>` and `Device::genphy_read_status` >> also returns a `Result<u16>`. In the function body we just delegate to >> the latter, so no `?` is needed. We just return the entire result. >> >> Here is the equivalent pseudo-C code: >> >> int genphy_read_status(struct phy_device *dev); >> >> int read_status(struct phy_device *dev) >> { >> return genphy_read_status(dev); >> } >> >> There you also don't need an if for the negative error code, since it's >> just propagated. > > O.K, it seems to work. But one of the things we try to think about in > the kernel is avoiding future bugs. Say sometime in the future i > extend it: > > fn read_status(dev: &mut phy::Device) -> Result<u16> { > dev.genphy_read_status::<C45>() > > dev.genphy_read_foo() > } > > By forgetting to add the ? to dev.genphy_read_status, have i just > introduced a bug? Could i have avoided that by always having the ? > even when it is not needed? The above code will not compile, since there is a missing `;` in the second line. If you try to do it with the semicolon: fn read_status(dev: &mut phy::Device) -> Result<u16> { dev.genphy_read_status::<C45>(); dev.genphy_read_foo() } Then you get this error: error: unused `core::result::Result` that must be used --> drivers/net/phy/qt2025.rs:88:9 | 88 | dev.genphy_read_status::<C45>(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled = note: `-D unused-must-use` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(unused_must_use)]` help: use `let _ = ...` to ignore the resulting value | 88 | let _ = dev.genphy_read_status::<C45>(); | +++++++ If you want to use `?` regardless, you will have to do this: fn read_status(dev: &mut phy::Device) -> Result<u16> { Ok(dev.genphy_read_status::<C45>()?) } In my opinion this does not add significant protection for the scenario that you outlined and is a lot more verbose. But if you're not used to Rust, this might be different, since the code below looks more wrong: fn read_status(dev: &mut phy::Device) -> Result<u16> { Ok(dev.genphy_read_status::<C45>()?); dev.genphy_read_foo() } But I would keep it the way it currently is. --- Cheers, Benno
> Then you get this error: > > error: unused `core::result::Result` that must be used > --> drivers/net/phy/qt2025.rs:88:9 > | > 88 | dev.genphy_read_status::<C45>(); > | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > | > = note: this `Result` may be an `Err` variant, which should be handled > = note: `-D unused-must-use` implied by `-D warnings` > = help: to override `-D warnings` add `#[allow(unused_must_use)]` > help: use `let _ = ...` to ignore the resulting value > | > 88 | let _ = dev.genphy_read_status::<C45>(); > | +++++++ O.K, so the compiler tells you, which is great. The C compiler would not, which is why i tend to think of these things. Andrew
On Sun, 18 Aug 2024 16:10:25 +0000 Benno Lossin <benno.lossin@proton.me> wrote: > On 17.08.24 07:19, FUJITA Tomonori wrote: >> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig >> index 7fddc8306d82..e0ff386d90cd 100644 >> --- a/drivers/net/phy/Kconfig >> +++ b/drivers/net/phy/Kconfig >> @@ -109,6 +109,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 > > This is missing a depends on RUST_FW_LOADER_ABSTRACTIONS. Oops, added. Thanks!
On Sun, Aug 18, 2024 at 7:39 PM Andrew Lunn <andrew@lunn.ch> wrote: > > O.K, so the compiler tells you, which is great. The C compiler would > not, which is why i tend to think of these things. For the unused result, there is `__must_check` of course, though it does not seem to apply to types in GCC like it is applied to `Result` in Rust (Clang is OK with it). C23 brings `[[nodiscard]]`, which can be. GCC >= 11 and Clang >= 17 support it, so one can do things like: typedef struct [[nodiscard]] Result { ... } Result; Result g(void); Result f(void) { g(); // will warn. return g(); } https://godbolt.org/z/PqK36rjWY And have a `Result`-like type (or, rather, like our alias in the kernel, given the generics). So if one is happy using new types, that is great. It would be nice to be able to do something like: #define Result __must_check int ...but that faces trouble when using it for e.g. local variables. Cheers, Miguel
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 7fddc8306d82..e0ff386d90cd 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -109,6 +109,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 202ed7f450da..461d6a3b9997 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..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 | 6 +++ drivers/net/phy/Makefile | 1 + drivers/net/phy/qt2025.rs | 90 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+) create mode 100644 drivers/net/phy/qt2025.rs