Message-ID: <CALNs47uvG_yjzX7Ewszb6M__jMZFtPu1rtw8DqvL5CceqCw4Zg@mail.gmail.com>
Date: Fri, 23 Aug 2024 00:25:23 -0500
From: Trevor Gross <tmgross@...ch.edu>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, 
	miguel.ojeda.sandonis@...il.com, benno.lossin@...ton.me, aliceryhl@...gle.com
Subject: Re: [PATCH net-next v6 6/6] net: phy: add Applied Micro QT2025 PHY driver

On Tue, Aug 20, 2024 at 5:59 PM FUJITA Tomonori
<fujita.tomonori@...il.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@...il.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@...il.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@...il.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

Powered by blists - more mailing lists