[PATCH net-next 0/2] pch_gbe: Minow fix and MinnowBoard support

[PATCH net-next 0/2] pch_gbe: Minow fix and MinnowBoard support

[Darren Hart]

Add support for the MinnowBoard in the pch_gbe driver. This was originally sent
to LKML as part of the MinnowBoard support series. That is now partially merged
and this version of the patch has been isolated from those changes and is now
completely self-contained.

Since the previous version:
    Whitespace cleanup
    Remove trailing period in messages
    Comment formatting (added a colon)
    Use devm_gpio_request_one
    Fix missing adapter->pdata NULL check in PHY
    Use netdev style comment blocks

[PATCH net-next 1/2] pch_gbe: Use PCH_GBE_PHY_REGS_LEN instead of 32

[Darren Hart]

Avoid using magic numbers when we have perfectly good defines just lying
around.

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: netdev@vger.kernel.org
---
 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index ab1039a..749ddd9 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -682,7 +682,7 @@ static int pch_gbe_init_phy(struct pch_gbe_adapter *adapter)
 	}
 	adapter->hw.phy.addr = adapter->mii.phy_id;
 	netdev_dbg(netdev, "phy_addr = %d\n", adapter->mii.phy_id);
-	if (addr == 32)
+	if (addr == PCH_GBE_PHY_REGS_LEN)
 		return -EAGAIN;
 	/* Selected the phy and isolate the rest */
 	for (addr = 0; addr < PCH_GBE_PHY_REGS_LEN; addr++) {
-- 
1.8.3.1

[PATCH net-next 2/2] pch_gbe: Add MinnowBoard support

[Darren Hart]

The MinnowBoard uses an AR803x PHY with the PCH GBE which requires
special handling. Use the MinnowBoard PCI Subsystem ID to detect this
and add a pci_device_id.driver_data structure and functions to handle
platform setup.

The AR803x does not implement the RGMII 2ns TX clock delay in the trace
routing nor via strapping. Add a detection method for the board and the
PHY and enable the TX clock delay via the registers.

This PHY will hibernate without link for 10 seconds. Ensure the PHY is
awake for probe and then disable hibernation. A future improvement would
be to convert pch_gbe to using PHYLIB and making sure we can wake the
PHY at the necessary times rather than permanently disabling it.

V3: Whitespace cleanup. Remove trailing period in messages.
    Comment formatting (added a colon).
    Use devm_gpio_request_one
    Fix missing adapter->pdata NULL check in PHY
    Use netdev style comment blocks

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Joe Perches <joe@perches.com>
Cc: netdev@vger.kernel.org
---
 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    | 14 ++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   | 49 +++++++++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c    | 96 ++++++++++++++++++++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h    |  2 +
 4 files changed, 161 insertions(+)

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
index 7779036..0d124d7 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
@@ -581,6 +581,18 @@ struct pch_gbe_hw_stats {
 	u32 intr_tcpip_err_count;
 };
 
+/* struct pch_gbe_privdata - PCI Device ID driver data
+ * @phy_tx_clk_delay:		Bool, configure the PHY TX delay in software
+ * @phy_disable_hibernate:	Bool, disable PHY hibernation
+ * @platform_init:		Platform initialization callback, called from
+ *				probe, prio to PHY initialization.
+ */
+struct pch_gbe_privdata {
+	bool phy_tx_clk_delay;
+	bool phy_disable_hibernate;
+	int(*platform_init)(struct pci_dev *pdev);
+};
+
 /**
  * struct pch_gbe_adapter - board specific private data structure
  * @stats_lock:	Spinlock structure for status
@@ -604,6 +616,7 @@ struct pch_gbe_hw_stats {
  * @rx_buffer_len:	Receive buffer length
  * @tx_queue_len:	Transmit queue length
  * @have_msi:		PCI MSI mode flag
+ * @pch_gbe_privdata:	PCI Device ID driver_data
  */
 
 struct pch_gbe_adapter {
@@ -631,6 +644,7 @@ struct pch_gbe_adapter {
 	int hwts_tx_en;
 	int hwts_rx_en;
 	struct pci_dev *ptp_pdev;
+	struct pch_gbe_privdata *pdata;
 };
 
 #define pch_gbe_hw_to_adapter(hw)	container_of(hw, struct pch_gbe_adapter, hw)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 749ddd9..d37b10f 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -23,6 +23,7 @@
 #include <linux/module.h>
 #include <linux/net_tstamp.h>
 #include <linux/ptp_classify.h>
+#include <linux/gpio.h>
 
 #define DRV_VERSION     "1.01"
 const char pch_driver_version[] = DRV_VERSION;
@@ -111,6 +112,8 @@ const char pch_driver_version[] = DRV_VERSION;
 #define PTP_L4_MULTICAST_SA "01:00:5e:00:01:81"
 #define PTP_L2_MULTICAST_SA "01:1b:19:00:00:00"
 
+#define MINNOW_PHY_RESET_GPIO		13
+
 static unsigned int copybreak __read_mostly = PCH_GBE_COPYBREAK_DEFAULT;
 
 static int pch_gbe_mdio_read(struct net_device *netdev, int addr, int reg);
@@ -2635,6 +2638,9 @@ static int pch_gbe_probe(struct pci_dev *pdev,
 	adapter->pdev = pdev;
 	adapter->hw.back = adapter;
 	adapter->hw.reg = pcim_iomap_table(pdev)[PCH_GBE_PCI_BAR];
+	adapter->pdata = (struct pch_gbe_privdata *)pci_id->driver_data;
+	if (adapter->pdata && adapter->pdata->platform_init)
+		adapter->pdata->platform_init(pdev);
 
 	adapter->ptp_pdev = pci_get_bus_and_slot(adapter->pdev->bus->number,
 					       PCI_DEVFN(12, 4));
@@ -2710,6 +2716,10 @@ static int pch_gbe_probe(struct pci_dev *pdev,
 
 	dev_dbg(&pdev->dev, "PCH Network Connection\n");
 
+	/* Disable hibernation on certain platforms */
+	if (adapter->pdata && adapter->pdata->phy_disable_hibernate)
+		pch_gbe_phy_disable_hibernate(&adapter->hw);
+
 	device_set_wakeup_enable(&pdev->dev, 1);
 	return 0;
 
@@ -2720,9 +2730,48 @@ err_free_netdev:
 	return ret;
 }
 
+/* The AR803X PHY on the MinnowBoard requires a physical pin to be toggled to
+ * ensure it is awake for probe and init. Request the line and reset the PHY.
+ */
+static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
+{
+	int flags = GPIOF_DIR_OUT | GPIOF_INIT_HIGH | GPIOF_EXPORT;
+	int gpio = MINNOW_PHY_RESET_GPIO;
+	int ret;
+
+	ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
+				    "minnow_phy_reset");
+	if (ret) {
+		dev_err(&pdev->dev,
+			"ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
+		return ret;
+	}
+
+	gpio_set_value(gpio, 0);
+	usleep_range(1250, 1500);
+	gpio_set_value(gpio, 1);
+	usleep_range(1250, 1500);
+
+	return ret;
+}
+
+static struct pch_gbe_privdata pch_gbe_minnow_privdata = {
+	.phy_tx_clk_delay = true,
+	.phy_disable_hibernate = true,
+	.platform_init = pch_gbe_minnow_platform_init,
+};
+
 static DEFINE_PCI_DEVICE_TABLE(pch_gbe_pcidev_id) = {
 	{.vendor = PCI_VENDOR_ID_INTEL,
 	 .device = PCI_DEVICE_ID_INTEL_IOH1_GBE,
+	 .subvendor = PCI_VENDOR_ID_CIRCUITCO,
+	 .subdevice = PCI_SUBSYSTEM_ID_CIRCUITCO_MINNOWBOARD,
+	 .class = (PCI_CLASS_NETWORK_ETHERNET << 8),
+	 .class_mask = (0xFFFF00),
+	 .driver_data = (unsigned long) &pch_gbe_minnow_privdata
+	 },
+	{.vendor = PCI_VENDOR_ID_INTEL,
+	 .device = PCI_DEVICE_ID_INTEL_IOH1_GBE,
 	 .subvendor = PCI_ANY_ID,
 	 .subdevice = PCI_ANY_ID,
 	 .class = (PCI_CLASS_NETWORK_ETHERNET << 8),
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
index da07907..be513a9 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
@@ -74,6 +74,15 @@
 #define MII_SR_100X_FD_CAPS      0x4000	/* 100X  Full Duplex Capable */
 #define MII_SR_100T4_CAPS        0x8000	/* 100T4 Capable */
 
+/* AR8031 PHY Debug Registers */
+#define PHY_AR803X_ID           0x00001374
+#define PHY_AR8031_DBG_OFF      0x1D
+#define PHY_AR8031_DBG_DAT      0x1E
+#define PHY_AR8031_SERDES       0x05
+#define PHY_AR8031_HIBERNATE    0x0B
+#define PHY_AR8031_SERDES_TX_CLK_DLY   0x0100 /* TX clock delay of 2.0ns */
+#define PHY_AR8031_PS_HIB_EN           0x8000 /* Hibernate enable */
+
 /* Phy Id Register (word 2) */
 #define PHY_REVISION_MASK        0x000F
 
@@ -277,4 +286,91 @@ void pch_gbe_phy_init_setting(struct pch_gbe_hw *hw)
 	pch_gbe_phy_read_reg_miic(hw, PHY_PHYSP_CONTROL, &mii_reg);
 	mii_reg |= PHYSP_CTRL_ASSERT_CRS_TX;
 	pch_gbe_phy_write_reg_miic(hw, PHY_PHYSP_CONTROL, mii_reg);
+
+	/* Setup a TX clock delay on certain platforms */
+	if (adapter->pdata && adapter->pdata->phy_tx_clk_delay)
+		pch_gbe_phy_tx_clk_delay(hw);
+}
+
+/* pch_gbe_phy_tx_clk_delay - Setup TX clock delay via the PHY
+ * @hw:	            Pointer to the HW structure
+ * Returns
+ *	0:		Successful.
+ *	-EINVAL:	Invalid argument.
+ */
+int pch_gbe_phy_tx_clk_delay(struct pch_gbe_hw *hw)
+{
+	/* The RGMII interface requires a ~2ns TX clock delay. This is typically
+	 * done in layout with a longer trace or via PHY strapping, but can also
+	 * be done via PHY configuration registers.
+	 */
+	struct pch_gbe_adapter *adapter = pch_gbe_hw_to_adapter(hw);
+	u16 mii_reg;
+	int ret = 0;
+
+	switch (hw->phy.id) {
+	case PHY_AR803X_ID:
+		netdev_dbg(adapter->netdev,
+			   "Configuring AR803X PHY for 2ns TX clock delay\n");
+		pch_gbe_phy_read_reg_miic(hw, PHY_AR8031_DBG_OFF, &mii_reg);
+		ret = pch_gbe_phy_write_reg_miic(hw, PHY_AR8031_DBG_OFF,
+						 PHY_AR8031_SERDES);
+		if (ret)
+			break;
+
+		pch_gbe_phy_read_reg_miic(hw, PHY_AR8031_DBG_DAT, &mii_reg);
+		mii_reg |= PHY_AR8031_SERDES_TX_CLK_DLY;
+		ret = pch_gbe_phy_write_reg_miic(hw, PHY_AR8031_DBG_DAT,
+						 mii_reg);
+		break;
+	default:
+		netdev_err(adapter->netdev,
+			   "Unknown PHY (%x), could not set TX clock delay\n",
+			   hw->phy.id);
+		return -EINVAL;
+	}
+
+	if (ret)
+		netdev_err(adapter->netdev,
+			   "Could not configure tx clock delay for PHY\n");
+	return ret;
+}
+
+/* pch_gbe_phy_disable_hibernate - Disable the PHY low power state
+ * @hw:	            Pointer to the HW structure
+ * Returns
+ *	0:		Successful.
+ *	-EINVAL:	Invalid argument.
+ */
+int pch_gbe_phy_disable_hibernate(struct pch_gbe_hw *hw)
+{
+	struct pch_gbe_adapter *adapter = pch_gbe_hw_to_adapter(hw);
+	u16 mii_reg;
+	int ret = 0;
+
+	switch (hw->phy.id) {
+	case PHY_AR803X_ID:
+		netdev_dbg(adapter->netdev,
+			   "Disabling hibernation for AR803X PHY\n");
+		ret = pch_gbe_phy_write_reg_miic(hw, PHY_AR8031_DBG_OFF,
+						 PHY_AR8031_HIBERNATE);
+		if (ret)
+			break;
+
+		pch_gbe_phy_read_reg_miic(hw, PHY_AR8031_DBG_DAT, &mii_reg);
+		mii_reg &= ~PHY_AR8031_PS_HIB_EN;
+		ret = pch_gbe_phy_write_reg_miic(hw, PHY_AR8031_DBG_DAT,
+						 mii_reg);
+		break;
+	default:
+		netdev_err(adapter->netdev,
+			   "Unknown PHY (%x), could not disable hibernation\n",
+			   hw->phy.id);
+		return -EINVAL;
+	}
+
+	if (ret)
+		netdev_err(adapter->netdev,
+			   "Could not disable PHY hibernation\n");
+	return ret;
 }
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
index 03264dc..e3e4bc9 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
@@ -33,5 +33,7 @@ void pch_gbe_phy_power_up(struct pch_gbe_hw *hw);
 void pch_gbe_phy_power_down(struct pch_gbe_hw *hw);
 void pch_gbe_phy_set_rgmii(struct pch_gbe_hw *hw);
 void pch_gbe_phy_init_setting(struct pch_gbe_hw *hw);
+int pch_gbe_phy_tx_clk_delay(struct pch_gbe_hw *hw);
+int pch_gbe_phy_disable_hibernate(struct pch_gbe_hw *hw);
 
 #endif /* _PCH_GBE_PHY_H_ */
-- 
1.8.3.1

Re: [PATCH net-next 0/2] pch_gbe: Minow fix and MinnowBoard support

[David Miller]

From: Darren Hart <dvhart@linux.intel.com>
Date: Wed, 17 Jul 2013 13:10:41 -0700

> Add support for the MinnowBoard in the pch_gbe driver. This was originally sent
> to LKML as part of the MinnowBoard support series. That is now partially merged
> and this version of the patch has been isolated from those changes and is now
> completely self-contained.

The net-next tree is not open yet, please resubmit this when it is.

Re: [PATCH net-next 2/2] pch_gbe: Add MinnowBoard support

[Andy Shevchenko]

On Wed, 2013-07-17 at 13:10 -0700, Darren Hart wrote: 
> The MinnowBoard uses an AR803x PHY with the PCH GBE which requires
> special handling. Use the MinnowBoard PCI Subsystem ID to detect this
> and add a pci_device_id.driver_data structure and functions to handle
> platform setup.
> 
> The AR803x does not implement the RGMII 2ns TX clock delay in the trace
> routing nor via strapping. Add a detection method for the board and the
> PHY and enable the TX clock delay via the registers.
> 
> This PHY will hibernate without link for 10 seconds. Ensure the PHY is
> awake for probe and then disable hibernation. A future improvement would
> be to convert pch_gbe to using PHYLIB and making sure we can wake the
> PHY at the necessary times rather than permanently disabling it.

Few comments below.

> --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
> +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
> @@ -581,6 +581,18 @@ struct pch_gbe_hw_stats {
>  	u32 intr_tcpip_err_count;
>  };
>  
> +/* struct pch_gbe_privdata - PCI Device ID driver data

Perhaps
/**
* struct ...
?

> --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c


> @@ -2720,9 +2730,48 @@ err_free_netdev:
>  	return ret;
>  }
>  
> +/* The AR803X PHY on the MinnowBoard requires a physical pin to be toggled to
> + * ensure it is awake for probe and init. Request the line and reset the PHY.
> + */
> +static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
> +{
> +	int flags = GPIOF_DIR_OUT | GPIOF_INIT_HIGH | GPIOF_EXPORT;
> +	int gpio = MINNOW_PHY_RESET_GPIO;
> +	int ret;
> +
> +	ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
> +				    "minnow_phy_reset");
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
> +		return ret;
> +	}
> +
> +	gpio_set_value(gpio, 0);
> +	usleep_range(1250, 1500);
> +	gpio_set_value(gpio, 1);
> +	usleep_range(1250, 1500);
> +
> +	return ret;
> +}

You ignored couple of comments regarding this function. Why?

>  static DEFINE_PCI_DEVICE_TABLE(pch_gbe_pcidev_id) = {
>  	{.vendor = PCI_VENDOR_ID_INTEL,
>  	 .device = PCI_DEVICE_ID_INTEL_IOH1_GBE,
> +	 .subvendor = PCI_VENDOR_ID_CIRCUITCO,
> +	 .subdevice = PCI_SUBSYSTEM_ID_CIRCUITCO_MINNOWBOARD,
> +	 .class = (PCI_CLASS_NETWORK_ETHERNET << 8),
> +	 .class_mask = (0xFFFF00),
> +	 .driver_data = (unsigned long) &pch_gbe_minnow_privdata

No need space before &.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH net-next 2/2] pch_gbe: Add MinnowBoard support

[Darren Hart]

On Thu, 2013-07-18 at 11:14 +0300, Andy Shevchenko wrote:
> On Wed, 2013-07-17 at 13:10 -0700, Darren Hart wrote: 
> > The MinnowBoard uses an AR803x PHY with the PCH GBE which requires
> > special handling. Use the MinnowBoard PCI Subsystem ID to detect this
> > and add a pci_device_id.driver_data structure and functions to handle
> > platform setup.
> > 
> > The AR803x does not implement the RGMII 2ns TX clock delay in the trace
> > routing nor via strapping. Add a detection method for the board and the
> > PHY and enable the TX clock delay via the registers.
> > 
> > This PHY will hibernate without link for 10 seconds. Ensure the PHY is
> > awake for probe and then disable hibernation. A future improvement would
> > be to convert pch_gbe to using PHYLIB and making sure we can wake the
> > PHY at the necessary times rather than permanently disabling it.
> 
> Few comments below.
> 
> > --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
> > +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
> > @@ -581,6 +581,18 @@ struct pch_gbe_hw_stats {
> >  	u32 intr_tcpip_err_count;
> >  };
> >  
> > +/* struct pch_gbe_privdata - PCI Device ID driver data
> 
> Perhaps
> /**
> * struct ...
> ?

This is a /net patch and multiline comments are to be formatted as I did
here. checkpatch catches this and it was also added to the under-review
netdev FAQ.

> 
> > --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> > +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> 
> 
> > @@ -2720,9 +2730,48 @@ err_free_netdev:
> >  	return ret;
> >  }
> >  
> > +/* The AR803X PHY on the MinnowBoard requires a physical pin to be toggled to
> > + * ensure it is awake for probe and init. Request the line and reset the PHY.
> > + */
> > +static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
> > +{
> > +	int flags = GPIOF_DIR_OUT | GPIOF_INIT_HIGH | GPIOF_EXPORT;
> > +	int gpio = MINNOW_PHY_RESET_GPIO;
> > +	int ret;
> > +
> > +	ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
> > +				    "minnow_phy_reset");
> > +	if (ret) {
> > +		dev_err(&pdev->dev,
> > +			"ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
> > +		return ret;
> > +	}
> > +
> > +	gpio_set_value(gpio, 0);
> > +	usleep_range(1250, 1500);
> > +	gpio_set_value(gpio, 1);
> > +	usleep_range(1250, 1500);
> > +
> > +	return ret;
> > +}
> 
> You ignored couple of comments regarding this function. Why?


Hrm? I either incorporated or justified why I didn't for each bit of
feedback.... And looking through the archive on lkml.org, I only see my
second response to you and not the first which has all the detail....
which would explain why you didn't respond then. I have it in my sent
mail, but it doesn't appear to have gone out. I've resent and I'll
summarize here.

> I don't like the platform_init() approach here. 
> I think here should be plain GPIO line number.

I went around and around on this myself. I originally added several
callback functions to this privdata, but ultimately felt it was more
infrastructure than was necessary now, and if it became necessary (a
big "if" in my opinion) I could always augment that as a follow-on
patch.

> Here perhaps you check pdata and GPIO line number (let's say != -1)   
> and call GPIO request helper.

I was doing exactly this in local previous version, but I decided
against it for a few reasons:

o If I specify GPIO, I must also specify GPIO flags, GPIO label,
  GPIO assertion level, GPIO reset and rest timings (and that is
  assuming it's just a set, release, wait cycle).

o Setting all this in pdata makes very specific to resetting this
  specific PHY, while others may have any number of other methods or
  procedures. It also excludes other sorts of platform initialization
  which might necessary. Specifying GPIO here makes the interface overly
  specific in my opinion.

> Next is the name of the function, since you are resetting PHY, what if
> you call it like pch_gbe_reset_by_gpio ?

We would need to include PHY in here as we are not resetting the MAC,
we are resetting the PHY. I think specifying it as being by gpio is
also overly restrictive. If you look at my two new functions (for tx
clock delay and hibernate) you can see an example of how we might go
about such a function (which indeed I had in a previous version as
pch_gbe_phy_physical_reset()). I dropped this as I felt it required too
many fields to be added to the pdata and I was better off with platform
specific init routines.

You've presented an alternative approach, but it isn't clear to me what
your reservations are with the one I took here. What are the problems
with it?

> And most important one is the ACPI case. As far as I understand Minnow
> board supports / will support ACPI5 variant of device enumeration. In
> such case the GPIO line will come in the ACPI resources. Moreover, you
> will have no struct pci_dev. I highly recommend to rewrite this as a
> generic helper, which takes GPIO line number as an input parameter and
> does the job.

While the MinnowBoard will be expanding its use of ACPI, we do not
intend to rewrite existing drivers (such as this one) or firmware
platform code. We are looking only to describe the Lure's (daughter
cards) with additional tables.


> 
> >  static DEFINE_PCI_DEVICE_TABLE(pch_gbe_pcidev_id) = {
> >  	{.vendor = PCI_VENDOR_ID_INTEL,
> >  	 .device = PCI_DEVICE_ID_INTEL_IOH1_GBE,
> > +	 .subvendor = PCI_VENDOR_ID_CIRCUITCO,
> > +	 .subdevice = PCI_SUBSYSTEM_ID_CIRCUITCO_MINNOWBOARD,
> > +	 .class = (PCI_CLASS_NETWORK_ETHERNET << 8),
> > +	 .class_mask = (0xFFFF00),
> > +	 .driver_data = (unsigned long) &pch_gbe_minnow_privdata
> 
> No need space before &.

Indeed. Fixed. I'll include when I resubmit after netdev opens.

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

Re: [PATCH net-next 2/2] pch_gbe: Add MinnowBoard support

[Andy Shevchenko]

On Thu, 2013-07-18 at 10:12 -0700, Darren Hart wrote: 
> On Thu, 2013-07-18 at 11:14 +0300, Andy Shevchenko wrote:
> > On Wed, 2013-07-17 at 13:10 -0700, Darren Hart wrote: 

[]

> > > --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> > > +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c

> > > +static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)

[]


> > Here perhaps you check pdata and GPIO line number (let's say != -1)   
> > and call GPIO request helper.
> 
> I was doing exactly this in local previous version, but I decided
> against it for a few reasons:
> 
> o If I specify GPIO, I must also specify GPIO flags, GPIO label,
>   GPIO assertion level, GPIO reset and rest timings (and that is
>   assuming it's just a set, release, wait cycle).

Why so complicated? Just keep them as defaults inside function like you
did until we will have the actual board which requires those to be
altered.

> o Setting all this in pdata makes very specific to resetting this
>   specific PHY, while others may have any number of other methods or
>   procedures. It also excludes other sorts of platform initialization
>   which might necessary. Specifying GPIO here makes the interface overly
>   specific in my opinion.
> 
> > Next is the name of the function, since you are resetting PHY, what if
> > you call it like pch_gbe_reset_by_gpio ?
> 
> We would need to include PHY in here as we are not resetting the MAC,
> we are resetting the PHY. I think specifying it as being by gpio is
> also overly restrictive. If you look at my two new functions (for tx
> clock delay and hibernate) you can see an example of how we might go
> about such a function (which indeed I had in a previous version as
> pch_gbe_phy_physical_reset()). I dropped this as I felt it required too
> many fields to be added to the pdata and I was better off with platform
> specific init routines.
> 
> You've presented an alternative approach, but it isn't clear to me what
> your reservations are with the one I took here. What are the problems
> with it?

My point is there should be no callback function like
pch_gbe_minnow_platform_init. It may be transformed to a more generic
helper which could be reused by an ACPI case as well.

But okay, it might be I missed the point. Are you going to provide an
ACPI tables for this IP or you just rely on PCI forever?

> > And most important one is the ACPI case. As far as I understand Minnow
> > board supports / will support ACPI5 variant of device enumeration. In
> > such case the GPIO line will come in the ACPI resources. Moreover, you
> > will have no struct pci_dev. I highly recommend to rewrite this as a
> > generic helper, which takes GPIO line number as an input parameter and
> > does the job.
> 
> While the MinnowBoard will be expanding its use of ACPI, we do not
> intend to rewrite existing drivers (such as this one)

Again, what is wrong with ACPI, if ACPI could manage by itself that
stuff like specific GPIO lines. I assume ACPI will help a lot to avoid
this legacy approach with driver_data / callbacks and such things.

What did I miss?

> > >  static DEFINE_PCI_DEVICE_TABLE(pch_gbe_pcidev_id) = {
> > >  	{.vendor = PCI_VENDOR_ID_INTEL,
> > >  	 .device = PCI_DEVICE_ID_INTEL_IOH1_GBE,
> > > +	 .subvendor = PCI_VENDOR_ID_CIRCUITCO,
> > > +	 .subdevice = PCI_SUBSYSTEM_ID_CIRCUITCO_MINNOWBOARD,
> > > +	 .class = (PCI_CLASS_NETWORK_ETHERNET << 8),
> > > +	 .class_mask = (0xFFFF00),
> > > +	 .driver_data = (unsigned long) &pch_gbe_minnow_privdata
> > 
> > No need space before &.
> 
> Indeed. Fixed. I'll include when I resubmit after netdev opens.

And you perhaps have to use kernel_ulong_t instead of unsigned long.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH net-next 2/2] pch_gbe: Add MinnowBoard support

[Darren Hart]

On Fri, 2013-07-19 at 11:48 +0300, Andy Shevchenko wrote:
> On Thu, 2013-07-18 at 10:12 -0700, Darren Hart wrote: 
> > On Thu, 2013-07-18 at 11:14 +0300, Andy Shevchenko wrote:
> > > On Wed, 2013-07-17 at 13:10 -0700, Darren Hart wrote: 
> 
> []
> 
> > > > --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> > > > +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> 
> > > > +static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
> 
> []
> 
> 
> > > Here perhaps you check pdata and GPIO line number (let's say != -1)   
> > > and call GPIO request helper.
> > 
> > I was doing exactly this in local previous version, but I decided
> > against it for a few reasons:
> > 
> > o If I specify GPIO, I must also specify GPIO flags, GPIO label,
> >   GPIO assertion level, GPIO reset and rest timings (and that is
> >   assuming it's just a set, release, wait cycle).
> 
> Why so complicated? Just keep them as defaults inside function like you
> did until we will have the actual board which requires those to be
> altered.

The short answer here is that calling this a generic PHY reset routine
and passing in only the GPIO number embeds very platform specific data
and behavior as defaults which would have to abstracted if the routine
were to be used on any other board. The approach I take here is more
explicit in calling out what is platform specific data.

I do appreciate the suggestion and I did approach this as you suggest
initially, but as I worked through it, it became apparent (to me at
least) that the method presented here kept the required infrastructure
to a minimum while clearly identifying platform specific code and
behavior.

> 
> > o Setting all this in pdata makes very specific to resetting this
> >   specific PHY, while others may have any number of other methods or
> >   procedures. It also excludes other sorts of platform initialization
> >   which might necessary. Specifying GPIO here makes the interface overly
> >   specific in my opinion.
> > 
> > > Next is the name of the function, since you are resetting PHY, what if
> > > you call it like pch_gbe_reset_by_gpio ?
> > 
> > We would need to include PHY in here as we are not resetting the MAC,
> > we are resetting the PHY. I think specifying it as being by gpio is
> > also overly restrictive. If you look at my two new functions (for tx
> > clock delay and hibernate) you can see an example of how we might go
> > about such a function (which indeed I had in a previous version as
> > pch_gbe_phy_physical_reset()). I dropped this as I felt it required too
> > many fields to be added to the pdata and I was better off with platform
> > specific init routines.
> > 
> > You've presented an alternative approach, but it isn't clear to me what
> > your reservations are with the one I took here. What are the problems
> > with it?
> 
> My point is there should be no callback function like
> pch_gbe_minnow_platform_init. It may be transformed to a more generic
> helper which could be reused by an ACPI case as well.
> 
> But okay, it might be I missed the point. Are you going to provide an
> ACPI tables for this IP or you just rely on PCI forever?


For this particular driver, I don't see a lot of value in introducing
new ACPI constructs. But more importantly, the release firmware does not
do so. If the firmware is updated in the future to deal with this, or if
someone else writes their own firmware (as many have expressed interest
in doing) we may have some follow-up to do here.


> > > And most important one is the ACPI case. As far as I understand Minnow
> > > board supports / will support ACPI5 variant of device enumeration. In
> > > such case the GPIO line will come in the ACPI resources. Moreover, you
> > > will have no struct pci_dev. I highly recommend to rewrite this as a
> > > generic helper, which takes GPIO line number as an input parameter and
> > > does the job.
> > 
> > While the MinnowBoard will be expanding its use of ACPI, we do not
> > intend to rewrite existing drivers (such as this one)
> 
> Again, what is wrong with ACPI, if ACPI could manage by itself that
> stuff like specific GPIO lines. I assume ACPI will help a lot to avoid
> this legacy approach with driver_data / callbacks and such things.
> 
> What did I miss?

Nothing is wrong with ACPI. I don't disagree that this could be done
with ACPI, it just isn't currently.

> 
> > > >  static DEFINE_PCI_DEVICE_TABLE(pch_gbe_pcidev_id) = {
> > > >  	{.vendor = PCI_VENDOR_ID_INTEL,
> > > >  	 .device = PCI_DEVICE_ID_INTEL_IOH1_GBE,
> > > > +	 .subvendor = PCI_VENDOR_ID_CIRCUITCO,
> > > > +	 .subdevice = PCI_SUBSYSTEM_ID_CIRCUITCO_MINNOWBOARD,
> > > > +	 .class = (PCI_CLASS_NETWORK_ETHERNET << 8),
> > > > +	 .class_mask = (0xFFFF00),
> > > > +	 .driver_data = (unsigned long) &pch_gbe_minnow_privdata
> > > 
> > > No need space before &.
> > 
> > Indeed. Fixed. I'll include when I resubmit after netdev opens.
> 
> And you perhaps have to use kernel_ulong_t instead of unsigned long.

Thanks for catching that and pointing me at the correct usage. Fixed in
V2.

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