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

[Florian Fainelli <florian@openwrt.org>]

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.

>
> 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.

>
> 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?
- 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
--
Florian