[Intel-wired-lan] [next] igb: allow setting MAC address on i211 using a device tree blob

The Intel i211 LOM pcie ethernet controllers' iNVM operates as an OTP 
and has no externel EEPROM interface [1]. The following allows the 
driver to pickup the MAC address from a device tree blob when CONFIG_OF 
has been enabled.

[1] 
http://www.intel.com/content/www/us/en/embedded/products/networking/i211-ethernet-controller-datasheet.html

Signed-off-by: John Holland <redacted>
---
  drivers/net/ethernet/intel/igb/igb_main.c | 30 
++++++++++++++++++++++++++++++
  1 file changed, 30 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c 

b/drivers/net/ethernet/intel/igb/igb_main.c
index 31e5f39..9c92443 100644

--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c

@@ -56,6 +56,11 @@
  #include <linux/i2c.h>
  #include "igb.h"

+#ifdef defined(CONFIG_OF)
+#include <linux/of_net.h>
+#include <linux/etherdevice.h>
+#endif
+
  #define MAJ 5
  #define MIN 3
  #define BUILD 0

@@ -2217,6 +2222,26 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
  }

  /**
+ *  igb_read_mac_addr_dts - Read mac addres from the device tree blob.
+ *  @hw: pointer to the e1000 hardware structure
+ **/
+#ifdef defined(CONFIG_OF)
+static void igb_read_mac_addr_dts(struct e1000_hw *hw)
+{
+       const u8 *mac;
+       struct device_node *dn;
+
+       dn = of_find_compatible_node(NULL, NULL, "intel,i211");
+       if (!dn)
+               return;
+
+       mac = of_get_mac_address(dn);
+       if (mac)
+               ether_addr_copy(hw->mac.addr, mac);
+}
+#endif
+
+/**
   *  igb_probe - Device Initialization Routine
   *  @pdev: PCI device information struct
   *  @ent: entry in igb_pci_tbl

@@ -2420,6 +2445,11 @@ static int igb_probe(struct pci_dev *pdev, const 

struct pci_device_id *ent)
         if (hw->mac.ops.read_mac_addr(hw))
                 dev_err(&pdev->dev, "NVM Read Error\n");

+#ifdef defined(CONFIG_OF)
+       if (!is_valid_ether_addr(hw->mac.addr))
+               igb_read_mac_addr_dts(hw);
+#endif
+
         memcpy(netdev->dev_addr, hw->mac.addr, netdev->addr_len);

         if (!is_valid_ether_addr(netdev->dev_addr)) {

On Fri, 2016-01-29 at 23:11 +0100, John Holland wrote:

Address is mis-spelled above

+ *  @hw: pointer to the e1000 hardware structure
+ **/
+#ifdef defined(CONFIG_OF)

Minor nitpick, you should have the function comment header wrapped in
the #ifdef as well.

quoted

+static void igb_read_mac_addr_dts(struct e1000_hw *hw)
+{
+       const u8 *mac;
+       struct device_node *dn;
+
+       dn = of_find_compatible_node(NULL, NULL, "intel,i211");

Hi John

Would this also work for the i210?

If so, you normally use the compatible string for the first device
this works with. So maybe this should be changed to intel,i210?

Thanks
	Andrew

quoted

+       dn = of_find_compatible_node(NULL, NULL, "intel,i211");

Humm, NULL, NULL. That means find the first node anywhere in the
device tree which matches. This is not going to work too well when you
have multiple i211s.

There is a way so specify a DT node is attached to a specific PCIe
bus/slot. I think you should search only there, so solving the
multiple device issue.

	 Andrew

It seem to me this should be using eth_platform_get_mac_address(), a
slightly more generic method to do this.  See the i40e driver for an
example, commit d9a84324e6 I believe.

sln

On Tue, Feb 9, 2016 at 3:59 AM, Andrew Lunn [off-list ref] wrote:

quoted

quoted

+       dn = of_find_compatible_node(NULL, NULL, "intel,i211");

Humm, NULL, NULL. That means find the first node anywhere in the
device tree which matches. This is not going to work too well when you
have multiple i211s.

There is a way so specify a DT node is attached to a specific PCIe
bus/slot. I think you should search only there, so solving the
multiple device issue.

         Andrew

-- 
==============================================

Mr. Shannon Nelson                        Network Division, Intel Corp.

Shannon.Nelson@intel.com                I don't speak for Intel

                 Parents can't afford to be squeamish

From: Shannon Nelson <redacted>
Date: Tue, 9 Feb 2016 09:42:45 -0800

It seem to me this should be using eth_platform_get_mac_address(), a
slightly more generic method to do this.  See the i40e driver for an
example, commit d9a84324e6 I believe.

+1

On Feb 9, 2016, at 18:42, Shannon Nelson [off-list ref] wrote:

It seem to me this should be using eth_platform_get_mac_address(), a
slightly more generic method to do this.  See the i40e driver for an
example, commit d9a84324e6 I believe.

I believe you are referring to https://patchwork.ozlabs.org/patch/566806. Haven't seen this used upstream yet. And, does that not mandate CONFIG_OF?

John

On Feb 9, 2016, at 12:02, Jeff Kirsher [off-list ref] wrote:

quoted

On Fri, 2016-01-29 at 23:11 +0100, John Holland wrote:
The Intel i211 LOM pcie ethernet controllers' iNVM operates as an
OTP 
and has no externel EEPROM interface [1]. The following allows the 
driver to pickup the MAC address from a device tree blob when
CONFIG_OF 
has been enabled.

[1] 
http://www.intel.com/content/www/us/en/embedded/products/networking/i
211-ethernet-controller-datasheet.html

Signed-off-by: John Holland <redacted>
---
  drivers/net/ethernet/intel/igb/igb_main.c | 30 
++++++++++++++++++++++++++++++
  1 file changed, 30 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c 

b/drivers/net/ethernet/intel/igb/igb_main.c
index 31e5f39..9c92443 100644

--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c

@@ -56,6 +56,11 @@
  #include <linux/i2c.h>
  #include "igb.h"

+#ifdef defined(CONFIG_OF)
+#include <linux/of_net.h>
+#include <linux/etherdevice.h>
+#endif
+
  #define MAJ 5
  #define MIN 3
  #define BUILD 0

@@ -2217,6 +2222,26 @@ static s32 igb_init_i2c(struct igb_adapter

*adapter)
  }

  /**
+ *  igb_read_mac_addr_dts - Read mac addres from the device tree
blob.

Address is mis-spelled above

Correct. Will fix this.

quoted

+ *  @hw: pointer to the e1000 hardware structure
+ **/
+#ifdef defined(CONFIG_OF)

Minor nitpick, you should have the function comment header wrapped in
the #ifdef as well.

No problem...
I'll do that right too.

quoted

+static void igb_read_mac_addr_dts(struct e1000_hw *hw)
+{
+       const u8 *mac;
+       struct device_node *dn;
+
+       dn = of_find_compatible_node(NULL, NULL, "intel,i211");
+       if (!dn)
+               return;
+
+       mac = of_get_mac_address(dn);
+       if (mac)
+               ether_addr_copy(hw->mac.addr, mac);
+}
+#endif
+
+/**
   *  igb_probe - Device Initialization Routine
   *  @pdev: PCI device information struct
   *  @ent: entry in igb_pci_tbl

@@ -2420,6 +2445,11 @@ static int igb_probe(struct pci_dev *pdev,

const 
struct pci_device_id *ent)
         if (hw->mac.ops.read_mac_addr(hw))
                 dev_err(&pdev->dev, "NVM Read Error\n");

+#ifdef defined(CONFIG_OF)
+       if (!is_valid_ether_addr(hw->mac.addr))
+               igb_read_mac_addr_dts(hw);
+#endif
+
         memcpy(netdev->dev_addr, hw->mac.addr, netdev->addr_len);

         if (!is_valid_ether_addr(netdev->dev_addr)) {
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Sent from my iPad
On Feb 9, 2016, at 12:54, Andrew Lunn [off-list ref] wrote:

quoted

quoted

+static void igb_read_mac_addr_dts(struct e1000_hw *hw)
+{
+       const u8 *mac;
+       struct device_node *dn;
+
+       dn = of_find_compatible_node(NULL, NULL, "intel,i211");

Hi John

Would this also work for the i210?

The usability scenario of i210 and i211 seem similar enough. Although, I will not be able to test the i210, I will use it as the compatible keyword.

If so, you normally use the compatible string for the first device
this works with. So maybe this should be changed to intel,i210?

Thanks
   Andrew

On Feb 9, 2016, at 12:59, Andrew Lunn [off-list ref] wrote:

quoted

quoted

+       dn = of_find_compatible_node(NULL, NULL, "intel,i211");

Humm, NULL, NULL. That means find the first node anywhere in the
device tree which matches. This is not going to work too well when you
have multiple i211s.

There is a way so specify a DT node is attached to a specific PCIe
bus/slot. I think you should search only there, so solving the
multiple device issue.

Good point. Will specify the current PCIe node.

On Wed, Feb 10, 2016 at 10:13:56AM +0100, John Holland wrote:

Sent from my iPad
On Feb 9, 2016, at 12:54, Andrew Lunn [off-list ref] wrote:

quoted

quoted

quoted

+static void igb_read_mac_addr_dts(struct e1000_hw *hw)
+{
+       const u8 *mac;
+       struct device_node *dn;
+
+       dn = of_find_compatible_node(NULL, NULL, "intel,i211");

Hi John

Would this also work for the i210?

The usability scenario of i210 and i211 seem similar
enough. Although, I will not be able to test the i210, I will use it
as the compatible keyword.

I can test on i210.

  Andrew