pch_gbe: Expanding PHY and Board support (MinnowBoard)

pch_gbe: Expanding PHY and Board support (MinnowBoard)

[Darren Hart]

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.

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.

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

Anyway, I'm new to networking drivers and would appreciate any feedback
that would help me start with the preferred approach for this type of
specialization.

Thanks,

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

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

[Florian Fainelli]

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

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

[Darren Hart]

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