Re: pch_gbe: Expanding PHY and Board support (MinnowBoard)

[Darren Hart <dvhart@linux.intel.com>]

On 03/27/2013 03:53 AM, Florian Fainelli wrote:
> Hello,
> 
> Le 03/27/13 00:25, Darren Hart a écrit :
>> I'm working on adding support for the MinnowBoard (minnowboard.org) as
>> we're working through the last of the hardware changes. There are a few
>> things about the pch_gbe driver that I'm having to update to support
>> this board. I'd appreciate your thoughts on how to best go about the
>> specializations.
>>
>> 1) It uses an Atheros AR8031 PHY instead of the more typical Realtek
>>     a) This PHY hibernates after 10s of no cable and can prevent
>> successful PHY
>>        init during PCI probe of the device. This can be addressed by
>> disabling
>>        hibernation in firmware and enabling later in the Kernel or by
>> adding support
>>        for the HW Reset of this PHY with the GPIO line tied to it on this
>> board.
>>     b) The board doesn't have a long trace to add the 2ns delay to TX
>> clock like
>>        other boards using this platform. This can be addressed by
>> instructing the
>>        PHY to introduce the delay.
>>
>> 2) Like another board which led to the following commit, this board does not
>>     have an EEPROM for the Ethernet address (MAC).
>>
>>      commit 2b53d07891630dead46d65c8f896955fd3ae0302
>>      Author: Darren Hart <dvhart@linux.intel.com>
>>      Date:   Mon Jan 16 09:50:19 2012 +0000
>>
>>          pch_gbe: Do not abort probe on bad MAC
>>
>>     On this board, we plan to have the MAC provided by the firmware
>> (NVRAM, EFI
>>     VAR, something along those lines).
>>
>> Looking at the driver and our options, using a PCI subsystem
>> vendor/device IDs
>> to identify the board seems like the best way to go. I have a start at
>> this but
>> would appreciate your thoughts on the following:
>>
>> 1) Is the PCI subsystem vendor/device IDs the right approach, e.g. in
>> probe():
>>
>> 	adapter = netdev_priv(netdev);
>>
>> 	/* Identify PCI subsystem ID tweaks */
>> 	adapter->hw.mac_in_nvram = NULL
>> 	adapter->hw.phy.hw_rst_gpio = -1;
>> 	adapter->hw.phy.tx_clk_delay = false;
>> 	switch (pdev->subsystem_vendor) {
>> 	case PCI_VENDOR_ID_INTEL:
>> 		switch (pdev->subsystem_device) {
>> 		case PCI_SUBDEVICE_ID_MINNOWBOARD:
>> 			pr_debug("MinnowBoard detected\n");
>> 			adapter->hw.mac_in_nvram = MINNOW_NVRAM_MAC_ADDR;
>> 			adapter->hw.phy.hw_reset_gpio = MINNOW_PHY_RST_GPIO;
>> 			adapter->hw.phy.tx_clk_delay = true;
>> 	}
>>
>>     The mac_in_nvram, hw_rst_gpio, and tx_clk_delay vars were added to the
>>     existing structures in support of this.
> 
> Each PCI device entry in your PCI ID table can be tight to a driver_data 
> cookie holding device specific information like the one you mention 
> above, this should eliminate the need for the switch/case here. The 
> 8250_pci.c driver extensively makes use of this for instance.


Excellent, thanks for the pointer. Looks like I should be looking at pci
quirks, keep the changes to the structures, but use the quirks device
specific setup functions to assign them. Do I have that about right?

> 
>>
>> 2) The physical reset of the PHY is extremely board and PHY specifc as it
>>     requires knowledge of the GPIO line used and the specifics of the
>> reset and
>>     clock timings on the particular PHY. I believe I can avoid this by just
>>     disabling hibernation in firmware and enabling after the driver is
>> up. There
>>     are scenarios where this could fail though, such as loading the module,
>>     unloading the module, removing the cable, waiting 10+ seconds, and
>> trying to
>>     load the driver again. It should however load successfully after a
>> cable was
>>     reinserted.
> 
> If this board can be uniquely identified using a PCI vendor/device id, 
> then you should be good with this, otherwise, you may need to find for 
> alternate solutions, like checking for a specific DMI string, or 
> whatever the firmware/BIOS can provide to you to uniquely identify this 
> board, and thus deduce the GPIO line.


My original approach used the DMI_BOARD_NAME/DMI_PRODUCT_NAME, but I got
some negative feedback on that, which led me down the PCI
subvendor/subdevice path. We still have to validate we can write these
IDs, but it seems consistent in the documentation.


> 
>>
>> 3) It appears as though the pch_gbe was written with a very specific PHY in
>>     mind. I've switched on phy.id where needed:
>>
>> 	switch (hw->phy.id) {
>> 	case PHY_AR803X_ID:
>> 		pr_info("AR803x PHY: configuring tx clock delay.\n");
>> 		...
>>
>>     But I wonder if converting pch_gbe over to the PHY Abstraction Layer and
>>     fleshing out the two known PHYs for this platform would be "the right"
>>     approach. Sounds like a lot more work...
> 
> I would recommend you switch over to PHYLIB which nicely abstracts all 
> PHY specific details for you, provided that you write a corresponding 
> PHY driver if needed. Looking at the existing pch_gbe driver, it should 
> not be too much of work since it seems to already have everything in 
> place, just not tight together. Specifically, here are roughly the steps 
> needed:
> 
> - pch_gbe_phy.c should be turned into a proper PHY driver (looks like 
> this file supports some kind of internal PHY?

No internal PHY as far as I know. Most boards using this device use a
realtek phy (the part number escapes me atm).

> - most of the code in pch_gbe_main.c, especially pch_gbe_init_phy() 
> should be removed or moved to the corresponding PHY driver
> - you should register and implement a mdio bus driver for the pch_gbe 
> adapter

Thank you for this, very helpful. I'm sure I'll have more questions, but
I'll see about starting in on this.

> --
> Florian
> 

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Technical Lead - Linux Kernel