Re: [PATCH net-next v3 6/6] net: phy: add Applied Micro QT2025 PHY driver

[Andrew Lunn <andrew@lunn.ch>]

On Fri, Aug 16, 2024 at 06:17:10AM +0000, FUJITA Tomonori wrote:
> On Fri, 16 Aug 2024 03:45:09 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> >> +#[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 hardware is configurable?
> >> +        let hw_id = dev.read(C45::new(Mmd::PMAPMD, 0xd001))?;
> >> +        if (hw_id >> 8) & 0xff != 0xb3 {
> >> +            return Ok(());
> >> +        }
> > 
> > I don't understand this bit of code. At a guess, if the upper bytes of
> > that register is not 0xb3, the firmware has already been loaded into
> > the device?
> 
> I've just added debug code and found that the upper bytes of the
> register is 0xb3 even after loading the firmware.
> 
> I checked the original code again and found that if the bytes isn't
> 0xb3, the driver initialization fails. I guess that the probe should
> return an error here (ENODEV?). 

O.K. Maybe add a comment that the vendor driver does this, but we have
no idea why.

> 
> >> +        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)?;
> > 
> > 802.3 says:
> > 
> > 3.38 through 3.4110/25GBASE-R PCS test pattern seed B ????
> 
> Yeah, strange. But I can't find any hints on them in the datasheet or
> the original code.

Seems unlikely it is seeding anything. So i guess they have reused the
registers for something else. This is actually a bit odd, because all
the other registers are in the ranges reserved for vendor usage.

If these were legitimate register usage, i would of suggested adding
#define to include/uapi/linux/mdio.h for them.

	Andrew