Message-Id: <20240823.133656.1425422314833390920.fujita.tomonori@gmail.com>
Date: Fri, 23 Aug 2024 13:36:56 +0000 (UTC)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: tmgross@...ch.edu
Cc: fujita.tomonori@...il.com, 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 Fri, 23 Aug 2024 00:25:23 -0500
Trevor Gross <tmgross@...ch.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.

Powered by blists - more mailing lists