Message-ID: <CALNs47siFZQDE8_N2FyLhCMfszrcX7f5Q=rj8c9dzO9Q=hQsmQ@mail.gmail.com>
Date: Mon, 19 Aug 2024 04:07:30 -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 v5 6/6] net: phy: add Applied Micro QT2025 PHY driver

On Sun, Aug 18, 2024 at 8:01 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>
> ---
>  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/

Powered by blists - more mailing lists