[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20240418.220047.226895073727611433.fujita.tomonori@gmail.com>
Date: Thu, 18 Apr 2024 22:00:47 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: gregkh@...uxfoundation.org
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org, andrew@...n.ch,
rust-for-linux@...r.kernel.org, tmgross@...ch.edu
Subject: Re: [PATCH net-next v1 4/4] net: phy: add Applied Micro QT2025 PHY
driver
Hi,
On Mon, 15 Apr 2024 13:15:08 +0200
Greg KH <gregkh@...uxfoundation.org> wrote:
>> +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",
>> +}
>
> What about support for MODULE_FIRMWARE() so it will be properly loaded
> into the initramfs of systems now that you are needing it for this
> driver? To ignore that is going to cause problems :(
Oops, right. I'll work on it.
>> +const MDIO_MMD_PMAPMD: u8 = uapi::MDIO_MMD_PMAPMD as u8;
>> +const MDIO_MMD_PCS: u8 = uapi::MDIO_MMD_PCS as u8;
>> +const MDIO_MMD_PHYXS: u8 = uapi::MDIO_MMD_PHYXS as u8;
>> +
>> +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 config_init(dev: &mut phy::Device) -> Result<()> {
>> + let fw = Firmware::new(c_str!("qt2025-2.0.3.3.fw"), dev)?;
>> +
>> + let phy_id = dev.c45_read(MDIO_MMD_PMAPMD, 0xd001)?;
>> + if (phy_id >> 8) & 0xff != 0xb3 {
>> + return Ok(());
>> + }
>> +
>> + dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0000)?;
>> + dev.c45_write(MDIO_MMD_PMAPMD, 0xC302, 0x4)?;
>> + dev.c45_write(MDIO_MMD_PMAPMD, 0xC319, 0x0038)?;
>> +
>> + dev.c45_write(MDIO_MMD_PMAPMD, 0xC31A, 0x0098)?;
>> + dev.c45_write(MDIO_MMD_PCS, 0x0026, 0x0E00)?;
>> +
>> + dev.c45_write(MDIO_MMD_PCS, 0x0027, 0x0893)?;
>> +
>> + dev.c45_write(MDIO_MMD_PCS, 0x0028, 0xA528)?;
>> + dev.c45_write(MDIO_MMD_PCS, 0x0029, 0x03)?;
>> + dev.c45_write(MDIO_MMD_PMAPMD, 0xC30A, 0x06E1)?;
>> + dev.c45_write(MDIO_MMD_PMAPMD, 0xC300, 0x0002)?;
>> + dev.c45_write(MDIO_MMD_PCS, 0xE854, 0x00C0)?;
>> +
>> + let mut j = 0x8000;
>> + let mut a = MDIO_MMD_PCS;
>> + for (i, val) in fw.data().iter().enumerate() {
>
> So you are treating the firmware image as able to be iterated over here?
It's Rust way to do the original C code:
for (i = 0; i < the_size_of_fw; i++) {
// write fw_data[i] to a register.
>> + if i == 0x4000 {
>
> What does 0x4000 mean here?
>
>> + a = MDIO_MMD_PHYXS;
>> + j = 0x8000;
>
> What does 0x8000 mean here?
>
>> + }
>> + dev.c45_write(a, j, (*val).into())?;
>> +
>> + j += 1;
>> + }
>> + dev.c45_write(MDIO_MMD_PCS, 0xe854, 0x0040)?;
>
> Lots of magic values in this driver, is that intentional?
The original driver uses lots of magic values. I simply use them. As
Andrew wrote, we could infer some. I'll try to comment these.
Powered by blists - more mailing lists