[PATCH 3/3] pch_gbe: Add MinnowBoard support

[PATCH 3/3] 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.

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
Cc: <stable@vger.kernel.org> # 3.8.x: 5829e9b mfd: lpc_sch: Accomodate partial
Cc: <stable@vger.kernel.org> # 3.8.x: 3cbf182 gpio-sch: Allow for more than 8
Cc: <stable@vger.kernel.org> # 3.8.x: 91bbe92: PCI: Add CircuitCo vendor ID
Cc: <stable@vger.kernel.org> # 3.8.x: bd79680: pch_gbe: remove inline keyword
Cc: <stable@vger.kernel.org> # 3.8.x: 453ca93: pch_gbe: convert pr_* to
Cc: <stable@vger.kernel.org> # 3.8.x: 29cc436: pch_gbe: use managed functions
Cc: <stable@vger.kernel.org> # 3.8.x
Cc: <stable@vger.kernel.org> # 3.10.x: 91bbe92: PCI: Add CircuitCo vendor ID
Cc: <stable@vger.kernel.org> # 3.10.x: bd79680: pch_gbe: remove inline keyword
Cc: <stable@vger.kernel.org> # 3.10.x: 453ca93: pch_gbe: convert pr_* to
Cc: <stable@vger.kernel.org> # 3.10.x: 29cc436: pch_gbe: use managed functions
Cc: <stable@vger.kernel.org> # 3.10.x
Signed-off-by: Darren Hart <dvhart@linux.intel.com>
---
 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    | 15 ++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   | 48 +++++++++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c    | 98 ++++++++++++++++++++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h    |  2 +
 4 files changed, 163 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..d7d71cd 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
@@ -582,6 +582,19 @@ struct pch_gbe_hw_stats {
 };
 
 /**
+ * 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
  * @ethtool_lock:	Spinlock structure for ethtool
@@ -604,6 +617,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 +645,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..da83657 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,47 @@ 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 ret = 0;
+	int flags = GPIOF_DIR_OUT | GPIOF_INIT_HIGH | GPIOF_EXPORT;
+	int gpio = MINNOW_PHY_RESET_GPIO;
+
+	ret = gpio_request_one(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..7de93cd 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,93 @@ 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->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 3/3] pch_gbe: Add MinnowBoard support

[Joe Perches]

On Fri, 2013-07-12 at 17:58 -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.

trivial comments only:

Please use scripts/checkpatch.pl

[]

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[]
[]
> +static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
[]
> +	if (ret){

Missing space before brace

> 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
[]
> +int pch_gbe_phy_tx_clk_delay(struct pch_gbe_hw *hw)
[]
> +	case PHY_AR803X_ID:
> +		netdev_dbg(adapter->netdev,
> +			   "Configuring AR803X PHY for 2ns TX clock delay\n"); 
[]
> +		netdev_err(adapter->netdev,
> +			   "Unknown PHY (%x), could not set TX clock delay.\n",
> +			   hw->phy.id);
[]
> +		netdev_err(adapter->netdev,
> +			   "Could not configure tx clock delay for PHY.\n");
[]
> +int pch_gbe_phy_disable_hibernate(struct pch_gbe_hw *hw)
[]
> +	case PHY_AR803X_ID:
> +		netdev_dbg(adapter->netdev,
> +			   "Disabling hibernation for AR803X PHY\n");

It'd be nice if no period before newline were used
everywhere.

> +		netdev_err(adapter->netdev,
> +			   "Unknown PHY (%x), could not disable hibernation\n",
> +			   hw->phy.id);
[]
> +		netdev_err(adapter->netdev,
> +			   "Could not disable PHY hibernation.\n");

Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support

[Darren Hart]

On Fri, 2013-07-12 at 18:10 -0700, Joe Perches wrote:
> On Fri, 2013-07-12 at 17:58 -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.
> 
> trivial comments only:
> 
> Please use scripts/checkpatch.pl

Always good advice. I did actually do that. Some of the reports
conflict with existing formatting throughout the file. I opted for
consistency.

The others.... sigh, I did a last minute cleanup and somehow introduced
the whitespace errors. I do know better and should have waited until
tonight instead of sending them out when I was rushed. Apologies.

Fixed in V3 and awaiting additional feedback.

> 
> []
> 
> 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[]
> []
> > +static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
> []
> > +	if (ret){
> 
> Missing space before brace

Fixed.

> 
> > 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
> []
> > +int pch_gbe_phy_tx_clk_delay(struct pch_gbe_hw *hw)
> []
> > +	case PHY_AR803X_ID:
> > +		netdev_dbg(adapter->netdev,
> > +			   "Configuring AR803X PHY for 2ns TX clock delay\n"); 
> []
> > +		netdev_err(adapter->netdev,
> > +			   "Unknown PHY (%x), could not set TX clock delay.\n",
> > +			   hw->phy.id);
> []
> > +		netdev_err(adapter->netdev,
> > +			   "Could not configure tx clock delay for PHY.\n");
> []
> > +int pch_gbe_phy_disable_hibernate(struct pch_gbe_hw *hw)
> []
> > +	case PHY_AR803X_ID:
> > +		netdev_dbg(adapter->netdev,
> > +			   "Disabling hibernation for AR803X PHY\n");
> 
> It'd be nice if no period before newline were used
> everywhere.

Indeed, fixed.

Thank you for the review.

> 
> > +		netdev_err(adapter->netdev,
> > +			   "Unknown PHY (%x), could not disable hibernation\n",
> > +			   hw->phy.id);
> []
> > +		netdev_err(adapter->netdev,
> > +			   "Could not disable PHY hibernation.\n");
> 
> 

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

Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support

[Greg KH]

On Fri, Jul 12, 2013 at 05:58:07PM -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.
> 
> 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
> Cc: <stable@vger.kernel.org> # 3.8.x: 5829e9b mfd: lpc_sch: Accomodate partial
> Cc: <stable@vger.kernel.org> # 3.8.x: 3cbf182 gpio-sch: Allow for more than 8
> Cc: <stable@vger.kernel.org> # 3.8.x: 91bbe92: PCI: Add CircuitCo vendor ID
> Cc: <stable@vger.kernel.org> # 3.8.x: bd79680: pch_gbe: remove inline keyword
> Cc: <stable@vger.kernel.org> # 3.8.x: 453ca93: pch_gbe: convert pr_* to
> Cc: <stable@vger.kernel.org> # 3.8.x: 29cc436: pch_gbe: use managed functions
> Cc: <stable@vger.kernel.org> # 3.8.x
> Cc: <stable@vger.kernel.org> # 3.10.x: 91bbe92: PCI: Add CircuitCo vendor ID
> Cc: <stable@vger.kernel.org> # 3.10.x: bd79680: pch_gbe: remove inline keyword
> Cc: <stable@vger.kernel.org> # 3.10.x: 453ca93: pch_gbe: convert pr_* to
> Cc: <stable@vger.kernel.org> # 3.10.x: 29cc436: pch_gbe: use managed functions
> Cc: <stable@vger.kernel.org> # 3.10.x
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> ---
>  drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    | 15 ++++
>  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   | 48 +++++++++++
>  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c    | 98 ++++++++++++++++++++++
>  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h    |  2 +
>  4 files changed, 163 insertions(+)

This is _far_ more than just a simple "add a new device id" for a stable
kernel update.   Please go read Documentation/stable_kernel_rules.txt
again for why there's no way I can take this type of thing.

You know better than this.

greg k-h

Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support

[Darren Hart]

On Fri, 2013-07-12 at 18:17 -0700, Greg KH wrote:
> On Fri, Jul 12, 2013 at 05:58:07PM -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.
> > 
> > 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
> > Cc: <stable@vger.kernel.org> # 3.8.x: 5829e9b mfd: lpc_sch: Accomodate partial
> > Cc: <stable@vger.kernel.org> # 3.8.x: 3cbf182 gpio-sch: Allow for more than 8
> > Cc: <stable@vger.kernel.org> # 3.8.x: 91bbe92: PCI: Add CircuitCo vendor ID
> > Cc: <stable@vger.kernel.org> # 3.8.x: bd79680: pch_gbe: remove inline keyword
> > Cc: <stable@vger.kernel.org> # 3.8.x: 453ca93: pch_gbe: convert pr_* to
> > Cc: <stable@vger.kernel.org> # 3.8.x: 29cc436: pch_gbe: use managed functions
> > Cc: <stable@vger.kernel.org> # 3.8.x
> > Cc: <stable@vger.kernel.org> # 3.10.x: 91bbe92: PCI: Add CircuitCo vendor ID
> > Cc: <stable@vger.kernel.org> # 3.10.x: bd79680: pch_gbe: remove inline keyword
> > Cc: <stable@vger.kernel.org> # 3.10.x: 453ca93: pch_gbe: convert pr_* to
> > Cc: <stable@vger.kernel.org> # 3.10.x: 29cc436: pch_gbe: use managed functions
> > Cc: <stable@vger.kernel.org> # 3.10.x
> > Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> > ---
> >  drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    | 15 ++++
> >  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   | 48 +++++++++++
> >  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c    | 98 ++++++++++++++++++++++
> >  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h    |  2 +
> >  4 files changed, 163 insertions(+)
> 
> This is _far_ more than just a simple "add a new device id" for a stable
> kernel update.   Please go read Documentation/stable_kernel_rules.txt
> again for why there's no way I can take this type of thing.
>
> You know better than this.

I do appreciate the documentation that is there, and I have read it
(several times). The first two for 3.8 should be acceptable. The three
pre-reqs from pch_gbe are very unfortunate, but Andy pushed his in
response to my initial patch and they were merged first, making things
unnecessarily complicated for stable. This left me with the option of
massaging the patch (easy enough to do - I did this first actually) or
including them as pre-reqs, and I opted for the latter after re-reading
the stable docs and various threads on -stable to try and figure out
what was preferable, and there you had agreed to take a 120 patch
series. I've also seen patches longer than the 100 line limit go in, so
despite the documentation, sometimes it's difficult to tell what is
preferred, and if I don't include stable initially, it takes a lot of
effort and time to get things in afterward. I don't intend this as a
criticism, just an explanation of how I arrived here.

What would you prefer I do with this? I can break up the patch into
infrastructure and then MinnowBoard specific bits (I didn't think the
infrastructure-only patches would be well received on netdev, but maybe
I'm wrong there). I could massage the patch around Andy's three pch_gbe
cleanups which indeed are not stable candidates on their own. Or I could
drop the idea of trying to get Ethernet working on the MinnowBoard
outside of vendor trees and the next upstream release (I'd rather not do
that).

Thanks,

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

Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support

[Greg KH]

On Fri, Jul 12, 2013 at 10:46:21PM -0700, Darren Hart wrote:
> On Fri, 2013-07-12 at 18:17 -0700, Greg KH wrote:
> > On Fri, Jul 12, 2013 at 05:58:07PM -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.
> > > 
> > > 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
> > > Cc: <stable@vger.kernel.org> # 3.8.x: 5829e9b mfd: lpc_sch: Accomodate partial
> > > Cc: <stable@vger.kernel.org> # 3.8.x: 3cbf182 gpio-sch: Allow for more than 8
> > > Cc: <stable@vger.kernel.org> # 3.8.x: 91bbe92: PCI: Add CircuitCo vendor ID
> > > Cc: <stable@vger.kernel.org> # 3.8.x: bd79680: pch_gbe: remove inline keyword
> > > Cc: <stable@vger.kernel.org> # 3.8.x: 453ca93: pch_gbe: convert pr_* to
> > > Cc: <stable@vger.kernel.org> # 3.8.x: 29cc436: pch_gbe: use managed functions
> > > Cc: <stable@vger.kernel.org> # 3.8.x
> > > Cc: <stable@vger.kernel.org> # 3.10.x: 91bbe92: PCI: Add CircuitCo vendor ID
> > > Cc: <stable@vger.kernel.org> # 3.10.x: bd79680: pch_gbe: remove inline keyword
> > > Cc: <stable@vger.kernel.org> # 3.10.x: 453ca93: pch_gbe: convert pr_* to
> > > Cc: <stable@vger.kernel.org> # 3.10.x: 29cc436: pch_gbe: use managed functions
> > > Cc: <stable@vger.kernel.org> # 3.10.x
> > > Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> > > ---
> > >  drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    | 15 ++++
> > >  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   | 48 +++++++++++
> > >  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c    | 98 ++++++++++++++++++++++
> > >  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h    |  2 +
> > >  4 files changed, 163 insertions(+)
> > 
> > This is _far_ more than just a simple "add a new device id" for a stable
> > kernel update.   Please go read Documentation/stable_kernel_rules.txt
> > again for why there's no way I can take this type of thing.
> >
> > You know better than this.
> 
> I do appreciate the documentation that is there, and I have read it
> (several times). The first two for 3.8 should be acceptable.

I didn't even look at those other patches (and hint, 3.8 is dead and
burried, no stable releases are happening for it that really matter.)

I'm talking about _this_ patch.  Look at it.  How big it is.  How it's
not just "add a new device id."  Now please explain how _this_ patch
meets the stable kernel rules as documented?  Let alone the pr_*
conversions, ick, don't get me started...

Would you really send something like this to Linus after -rc4?  If not,
then it's not a stable patch.  See the 3.10.1-rc1 thread on lkml for the
details as to why I now need to push back and be harsher for this.

Just have users use 3.11, it's not like you have shipping hardware yet,
and again, who cares about 3.8.  Heck, who cares about 3.9 or even 3.10
for brand-new hardware.  Just use 3.11 or 3.12 and don't worry about
backport hell.

This isn't going to land in Linus's tree until 3.12 anyway, so what's
the rush?

greg  k-h

Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support

[Darren Hart]

On Fri, 2013-07-12 at 23:26 -0700, Greg KH wrote:
> On Fri, Jul 12, 2013 at 10:46:21PM -0700, Darren Hart wrote:
> > On Fri, 2013-07-12 at 18:17 -0700, Greg KH wrote:
> > > On Fri, Jul 12, 2013 at 05:58:07PM -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.
> > > > 
> > > > 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
> > > > Cc: <stable@vger.kernel.org> # 3.8.x: 5829e9b mfd: lpc_sch: Accomodate partial
> > > > Cc: <stable@vger.kernel.org> # 3.8.x: 3cbf182 gpio-sch: Allow for more than 8
> > > > Cc: <stable@vger.kernel.org> # 3.8.x: 91bbe92: PCI: Add CircuitCo vendor ID
> > > > Cc: <stable@vger.kernel.org> # 3.8.x: bd79680: pch_gbe: remove inline keyword
> > > > Cc: <stable@vger.kernel.org> # 3.8.x: 453ca93: pch_gbe: convert pr_* to
> > > > Cc: <stable@vger.kernel.org> # 3.8.x: 29cc436: pch_gbe: use managed functions
> > > > Cc: <stable@vger.kernel.org> # 3.8.x
> > > > Cc: <stable@vger.kernel.org> # 3.10.x: 91bbe92: PCI: Add CircuitCo vendor ID
> > > > Cc: <stable@vger.kernel.org> # 3.10.x: bd79680: pch_gbe: remove inline keyword
> > > > Cc: <stable@vger.kernel.org> # 3.10.x: 453ca93: pch_gbe: convert pr_* to
> > > > Cc: <stable@vger.kernel.org> # 3.10.x: 29cc436: pch_gbe: use managed functions
> > > > Cc: <stable@vger.kernel.org> # 3.10.x
> > > > Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> > > > ---
> > > >  drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    | 15 ++++
> > > >  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   | 48 +++++++++++
> > > >  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c    | 98 ++++++++++++++++++++++
> > > >  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h    |  2 +
> > > >  4 files changed, 163 insertions(+)
> > > 
> > > This is _far_ more than just a simple "add a new device id" for a stable
> > > kernel update.   Please go read Documentation/stable_kernel_rules.txt
> > > again for why there's no way I can take this type of thing.
> > >
> > > You know better than this.
> > 
> > I do appreciate the documentation that is there, and I have read it
> > (several times). The first two for 3.8 should be acceptable.
> 
> I didn't even look at those other patches (and hint, 3.8 is dead and
> burried, no stable releases are happening for it that really matter.)
> 
> I'm talking about _this_ patch.  Look at it.  How big it is.  How it's
> not just "add a new device id."  Now please explain how _this_ patch
> meets the stable kernel rules as documented?  Let alone the pr_*
> conversions, ick, don't get me started...

I was looking at it as a quirk:

" - New device IDs and quirks are also accepted."

I even considered implementation as a pci quirk. I didn't because the
PHY work needed to happen too late during probe. The frustrating thing
is there is probably 15 lines of code that are needed to get it to
work, all the rest is infrastructure to make it generic.

I get the size argument. It's too big. The docs say 100 lines with
context, I've seen much larger go in... I just wasn't sure where the
line is. It seems things are getting more strict here, not less. OK.
Lesson learned.


> Would you really send something like this to Linus after -rc4?  If not,
> then it's not a stable patch.  See the 3.10.1-rc1 thread on lkml for the
> details as to why I now need to push back and be harsher for this.

Wow, good thread. I don't appear to be the only one unsure of the
rules. I'll keep watching that thread for a definitive policy as you
and Linus do appear to be saying slightly different things.

> Just have users use 3.11, it's not like you have shipping hardware yet,

We're talking days here, but yes.

> and again, who cares about 3.8.  Heck, who cares about 3.9 or even 3.10
> for brand-new hardware.  Just use 3.11 or 3.12 and don't worry about
> backport hell.
> 
> This isn't going to land in Linus's tree until 3.12 anyway, so what's
> the rush?

My reasoning is that the BSP for this is based on 3.8. I would like to
bring 3.8 in sync with master for support of this board so I can update
the release BSP to use the same sources. People can use my code from
the linux-yocto_3.8 standard/minnow branch, but it would be preferable
if that code was also destined for upstream. Not the end of the world,
and I certainly don't mind directing people to 3.12 and ensuring the
next BSP release is fully aligned with mainline. And yes, none of the
above qualifies it for stable - it's only the "and quirks" that I felt
qualified this patch for stable, and I recognize it as a "freakish
giant" of a patch for stable. See, I really did read that entire
thread.

If you still feel this doesn't qualify as a "quirk", I'll drop the
stable push. If you can see that argument, I can look at breaking it up
into smaller chunks, or possibly reducing some of the generic bits and
doing the bare minimum to get it working and follow-up in master with
more generic bits.

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

Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support

[Greg KH]

On Sat, Jul 13, 2013 at 09:08:01AM -0700, Darren Hart wrote:
> On Fri, 2013-07-12 at 23:26 -0700, Greg KH wrote:
> > On Fri, Jul 12, 2013 at 10:46:21PM -0700, Darren Hart wrote:
> > > On Fri, 2013-07-12 at 18:17 -0700, Greg KH wrote:
> > > > > ---
> > > > >  drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    | 15 ++++
> > > > >  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   | 48 +++++++++++
> > > > >  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c    | 98 ++++++++++++++++++++++
> > > > >  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h    |  2 +
> > > > >  4 files changed, 163 insertions(+)
> > > > 
> > > > This is _far_ more than just a simple "add a new device id" for a stable
> > > > kernel update.   Please go read Documentation/stable_kernel_rules.txt
> > > > again for why there's no way I can take this type of thing.
> > > >
> > > > You know better than this.
> > > 
> > > I do appreciate the documentation that is there, and I have read it
> > > (several times). The first two for 3.8 should be acceptable.
> > 
> > I didn't even look at those other patches (and hint, 3.8 is dead and
> > burried, no stable releases are happening for it that really matter.)
> > 
> > I'm talking about _this_ patch.  Look at it.  How big it is.  How it's
> > not just "add a new device id."  Now please explain how _this_ patch
> > meets the stable kernel rules as documented?  Let alone the pr_*
> > conversions, ick, don't get me started...
> 
> I was looking at it as a quirk:
> 
> " - New device IDs and quirks are also accepted."
> 
> I even considered implementation as a pci quirk. I didn't because the
> PHY work needed to happen too late during probe. The frustrating thing
> is there is probably 15 lines of code that are needed to get it to
> work, all the rest is infrastructure to make it generic.

163 lines of code is not a "quirk" I can accept.  When I wrote, "new
device ids or quirks can be accpted for stable", I meant things like
commit 9e9dd0e889c76c786e8f2e164c825c3c06dea30c.  That's acceptable.
Not thing huge thing.

> I get the size argument. It's too big. The docs say 100 lines with
> context, I've seen much larger go in... I just wasn't sure where the
> line is. It seems things are getting more strict here, not less. OK.
> Lesson learned.

You have seen larger "quirks" than this go into the stable tree?
Examples for where I've been sleeping on the job would be good.

And that's the whole point of the huge thread I pointed you at, people
are trying to get stuff into stable that they shouldn't be (and they are
being lazy, but that's not the issue with this patch...)

> > Just have users use 3.11, it's not like you have shipping hardware yet,
> 
> We're talking days here, but yes.
> 
> > and again, who cares about 3.8.  Heck, who cares about 3.9 or even 3.10
> > for brand-new hardware.  Just use 3.11 or 3.12 and don't worry about
> > backport hell.
> > 
> > This isn't going to land in Linus's tree until 3.12 anyway, so what's
> > the rush?
> 
> My reasoning is that the BSP for this is based on 3.8. I would like to
> bring 3.8 in sync with master for support of this board so I can update
> the release BSP to use the same sources. People can use my code from
> the linux-yocto_3.8 standard/minnow branch, but it would be preferable
> if that code was also destined for upstream.

That's your choice to pick 3.8, not upstream's (and frankly, not
something that I would have picked, but that's another topic...)

> If you still feel this doesn't qualify as a "quirk", I'll drop the
> stable push. If you can see that argument, I can look at breaking it up
> into smaller chunks, or possibly reducing some of the generic bits and
> doing the bare minimum to get it working and follow-up in master with
> more generic bits.

You are adding functionality for new devices that take much more than a
simple "add an id to a table", so no, it's not ok for stable releases.

thanks,

greg k-h

Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support

[Darren Hart]

On Sat, 2013-07-13 at 10:09 -0700, Greg KH wrote:
> On Sat, Jul 13, 2013 at 09:08:01AM -0700, Darren Hart wrote:

...
> > I was looking at it as a quirk:
> > 
> > " - New device IDs and quirks are also accepted."
> > 
> > I even considered implementation as a pci quirk. I didn't because the
> > PHY work needed to happen too late during probe. The frustrating thing
> > is there is probably 15 lines of code that are needed to get it to
> > work, all the rest is infrastructure to make it generic.
> 
> 163 lines of code is not a "quirk" I can accept.  When I wrote, "new
> device ids or quirks can be accpted for stable", I meant things like
> commit 9e9dd0e889c76c786e8f2e164c825c3c06dea30c.  That's acceptable.
> Not thing huge thing.

Got it. I was confused on "new device IDs and quirks" versus "support
for new hardware". My fault, that's clear now. 

...
> > I get the size argument. It's too big. The docs say 100 lines with
> > context, I've seen much larger go in... I just wasn't sure where the
> > line is. It seems things are getting more strict here, not less. OK.
> > Lesson learned.
> 
> You have seen larger "quirks" than this go into the stable tree?
> Examples for where I've been sleeping on the job would be good.

No, sorry, I just meant patches larger than 100 lines with context,
that's all.

...
> > > This isn't going to land in Linus's tree until 3.12 anyway, so what's
> > > the rush?
> > 
> > My reasoning is that the BSP for this is based on 3.8. I would like to
> > bring 3.8 in sync with master for support of this board so I can update
> > the release BSP to use the same sources. People can use my code from
> > the linux-yocto_3.8 standard/minnow branch, but it would be preferable
> > if that code was also destined for upstream.
> 
> That's your choice to pick 3.8, not upstream's (and frankly, not
> something that I would have picked, but that's another topic...)

Maybe we can catch up at LPC or something, I'd like to hear your
thoughts on that. Of course there are a lot of factors that go into
that decision, and the bulk of it is consolidating effort on a single
tree across BSPs in a project that has a 6 month release cadence.

...
> You are adding functionality for new devices that take much more than a
> simple "add an id to a table", so no, it's not ok for stable releases.

Got it, I'll drop the stable lines from the subsequent versions and
keep that in mind for future projects. I'm trying to think if I can
polish up the docs to help clarify things, thinking on it.

Thank you Greg.

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

Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support

[Andy Shevchenko]

On Fri, 2013-07-12 at 17:58 -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
> @@ -582,6 +582,19 @@ struct pch_gbe_hw_stats {
>  };
>  
>  /**
> + * 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);

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

I'll explain below why.

> @@ -604,6 +617,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

Missed colon.

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


> @@ -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);

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

> @@ -2720,9 +2730,47 @@ 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 ret = 0;
> +	int flags = GPIOF_DIR_OUT | GPIOF_INIT_HIGH | GPIOF_EXPORT;
> +	int gpio = MINNOW_PHY_RESET_GPIO;
> +
> +	ret = gpio_request_one(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);

First of all, who is going to release gpio? Perhaps
devm_gpio_request_one?

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

> +
> +	return ret;
> +}

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.


> +
> +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),

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

> @@ -277,4 +286,93 @@ 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->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;

No need to assign. Are you trying to shut (sparse?) warning?

> +
> +	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;

Ditto.

> +	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;
>  }


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

Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support

[Darren Hart]

On Mon, 2013-07-15 at 11:34 +0300, Andy Shevchenko wrote:
> On Fri, 2013-07-12 at 17:58 -0700, Darren Hart wrote: 

... 
> > +/* 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 ret = 0;
> > +	int flags = GPIOF_DIR_OUT | GPIOF_INIT_HIGH | GPIOF_EXPORT;
> > +	int gpio = MINNOW_PHY_RESET_GPIO;
> > +
> > +	ret = gpio_request_one(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);
> 
> First of all, who is going to release gpio? Perhaps
> devm_gpio_request_one?

Thanks for the pointer, this works and appears to be the best way to
handle this.

...
> > + */
> > +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;
> 
> No need to assign. Are you trying to shut (sparse?) warning?

On this point, I did find one instance where I initialized init only to
have my first line of executable code assign to ret, that's obviously
silly, so I fixed that one.

Here, where the first assignment is nested inside case statements or
skipped altogether with explicit returns of error codes, I prefer to
assign it explicitly. A quick survey lists 4k initialized "int ret;"
lines and 16k uninitialized.  The latter appears to be the more common,
but there is certainly precedent for the former.

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

Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support

[Darren Hart]

(resending as the original appears not to have made it past my sent box)

On Mon, 2013-07-15 at 11:34 +0300, Andy Shevchenko wrote:
> On Fri, 2013-07-12 at 17:58 -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.

Hi Andy,

Thank you for taking the time to review...

> 
> > --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
> > +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
> > @@ -582,6 +582,19 @@ struct pch_gbe_hw_stats {
> >  };
> >  
> >  /**
> > + * 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);
> 
> 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.


> I'll explain below why.
> 
> > @@ -604,6 +617,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
> 
> Missed colon.


Ack, thanks.


> > --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> > +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
> 
> 
> > @@ -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);
> 
> 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 specific GPIO, I should 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.


> > @@ -2720,9 +2730,47 @@ 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 ret = 0;
> > +	int flags = GPIOF_DIR_OUT | GPIOF_INIT_HIGH | GPIOF_EXPORT;
> > +	int gpio = MINNOW_PHY_RESET_GPIO;
> > +
> > +	ret = gpio_request_one(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);
> 
> First of all, who is going to release gpio? Perhaps
> devm_gpio_request_one?


Oh right, thanks for catching that. I'll need to think on that, there
is no direct way to know I need to release it on module exit in this
implementation.


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

> 
> > +
> > +	return ret;
> > +}
> 
> 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.

...

> > +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;
> 
> No need to assign. Are you trying to shut (sparse?) warning?


Personal preference I guess. When it comes to return codes I prefer an
explicit assignment.


Thank you for taking the time to review!

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

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

Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support

[Darren Hart]

On Fri, 2013-07-12 at 17:58 -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.
> 
> 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

...

> @@ -277,4 +286,93 @@ 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->phy_tx_clk_delay)


This is missing an adapter->pdata !NULL test. Fixed in V3. Holding off
a bit more on that in case Dave M. has anything he wants to see changed
in the next revision.

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

Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support

[Darren Hart]

On Fri, 2013-07-12 at 17:58 -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.

If anyone is following along to see where this goes, I have made this
change independent of the minnowboard platform drivers and have resent
it with all the collected feedback on to netdev following the netdev
rules I was recently made aware of. Please follow along there if you are
interested.

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