* [PATCH 0/3] MinnowBoard Support V2 (Serial and Ethernet) @ 2013-07-13 0:58 Darren Hart 2013-07-13 0:58 ` [PATCH 1/3] pch_uart: Use DMI interface for board detection Darren Hart 0 siblings, 1 reply; 19+ messages in thread From: Darren Hart @ 2013-07-13 0:58 UTC (permalink / raw) To: Linux Kernel Mailing List Use the DMI interface to detect the board for UART_CLOCK selection per Greg K-H. Resend PCH_GBE_PHY_REGS_LEN define cleanup. Rewrite of PCH_GBE MinnowBoard support to be completely independent from any platform or board files. It requests the GPIO line in the pch_gbe driver and minimizes the pch_gbe_privdata (PCI driver_data) structure. Tested on 3.8, 3.10, and current master. Patches to lpc_sch and gpio_sch required for 3.8, included in stable Cc lines. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/3] pch_uart: Use DMI interface for board detection 2013-07-13 0:58 [PATCH 0/3] MinnowBoard Support V2 (Serial and Ethernet) Darren Hart @ 2013-07-13 0:58 ` Darren Hart 2013-07-13 0:58 ` [PATCH 2/3] pch_gbe: Use PCH_GBE_PHY_REGS_LEN instead of 32 Darren Hart ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Darren Hart @ 2013-07-13 0:58 UTC (permalink / raw) To: Linux Kernel Mailing List Cc: Darren Hart, Michael Brunner, Greg Kroah-Hartman Use the DMI interface rather than manually matching DMI strings. Signed-off-by: Darren Hart <dvhart@linux.intel.com> Cc: Michael Brunner <mibru@gmx.de> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/tty/serial/pch_uart.c | 71 +++++++++++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 22 deletions(-) diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c index 572d481..271cc73 100644 --- a/drivers/tty/serial/pch_uart.c +++ b/drivers/tty/serial/pch_uart.c @@ -373,35 +373,62 @@ static const struct file_operations port_regs_ops = { }; #endif /* CONFIG_DEBUG_FS */ +static struct dmi_system_id __initdata pch_uart_dmi_table[] = { + { + .ident = "CM-iTC", + { + DMI_MATCH(DMI_BOARD_NAME, "CM-iTC"), + }, + (void *)CMITC_UARTCLK, + }, + { + .ident = "FRI2", + { + DMI_MATCH(DMI_BIOS_VERSION, "FRI2"), + }, + (void *)FRI2_64_UARTCLK, + }, + { + .ident = "Fish River Island II", + { + DMI_MATCH(DMI_PRODUCT_NAME, "Fish River Island II"), + }, + (void *)FRI2_48_UARTCLK, + }, + { + .ident = "COMe-mTT", + { + DMI_MATCH(DMI_BOARD_NAME, "COMe-mTT"), + }, + (void *)NTC1_UARTCLK, + }, + { + .ident = "nanoETXexpress-TT", + { + DMI_MATCH(DMI_BOARD_NAME, "nanoETXexpress-TT"), + }, + (void *)NTC1_UARTCLK, + }, + { + .ident = "MinnowBoard", + { + DMI_MATCH(DMI_BOARD_NAME, "MinnowBoard"), + }, + (void *)MINNOW_UARTCLK, + }, +}; + /* Return UART clock, checking for board specific clocks. */ static int pch_uart_get_uartclk(void) { - const char *cmp; + const struct dmi_system_id *d; if (user_uartclk) return user_uartclk; - cmp = dmi_get_system_info(DMI_BOARD_NAME); - if (cmp && strstr(cmp, "CM-iTC")) - return CMITC_UARTCLK; - - cmp = dmi_get_system_info(DMI_BIOS_VERSION); - if (cmp && strnstr(cmp, "FRI2", 4)) - return FRI2_64_UARTCLK; - - cmp = dmi_get_system_info(DMI_PRODUCT_NAME); - if (cmp && strstr(cmp, "Fish River Island II")) - return FRI2_48_UARTCLK; - - /* Kontron COMe-mTT10 (nanoETXexpress-TT) */ - cmp = dmi_get_system_info(DMI_BOARD_NAME); - if (cmp && (strstr(cmp, "COMe-mTT") || - strstr(cmp, "nanoETXexpress-TT"))) - return NTC1_UARTCLK; - - cmp = dmi_get_system_info(DMI_BOARD_NAME); - if (cmp && strstr(cmp, "MinnowBoard")) - return MINNOW_UARTCLK; + d = dmi_first_match(pch_uart_dmi_table); + if (d) + return (int)d->driver_data; return DEFAULT_UARTCLK; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/3] pch_gbe: Use PCH_GBE_PHY_REGS_LEN instead of 32 2013-07-13 0:58 ` [PATCH 1/3] pch_uart: Use DMI interface for board detection Darren Hart @ 2013-07-13 0:58 ` Darren Hart 2013-07-13 0:58 ` [PATCH 3/3] pch_gbe: Add MinnowBoard support Darren Hart 2013-07-17 20:15 ` [PATCH 1/3] pch_uart: Use DMI interface for board detection Darren Hart 2 siblings, 0 replies; 19+ messages in thread From: Darren Hart @ 2013-07-13 0:58 UTC (permalink / raw) To: Linux Kernel Mailing List Cc: Darren Hart, David S. Miller, H. Peter Anvin, Peter Waskiewicz, Andy Shevchenko, netdev 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/3] pch_gbe: Add MinnowBoard support 2013-07-13 0:58 ` [PATCH 1/3] pch_uart: Use DMI interface for board detection Darren Hart 2013-07-13 0:58 ` [PATCH 2/3] pch_gbe: Use PCH_GBE_PHY_REGS_LEN instead of 32 Darren Hart @ 2013-07-13 0:58 ` Darren Hart 2013-07-13 1:10 ` Joe Perches ` (4 more replies) 2013-07-17 20:15 ` [PATCH 1/3] pch_uart: Use DMI interface for board detection Darren Hart 2 siblings, 5 replies; 19+ messages in thread From: Darren Hart @ 2013-07-13 0:58 UTC (permalink / raw) To: Linux Kernel Mailing List Cc: Darren Hart, David S. Miller, H. Peter Anvin, Peter Waskiewicz, Andy Shevchenko, netdev, stable 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support 2013-07-13 0:58 ` [PATCH 3/3] pch_gbe: Add MinnowBoard support Darren Hart @ 2013-07-13 1:10 ` Joe Perches 2013-07-13 5:52 ` Darren Hart 2013-07-13 1:17 ` Greg KH ` (3 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Joe Perches @ 2013-07-13 1:10 UTC (permalink / raw) To: Darren Hart Cc: Linux Kernel Mailing List, David S. Miller, H. Peter Anvin, Peter Waskiewicz, Andy Shevchenko, netdev, stable 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"); ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support 2013-07-13 1:10 ` Joe Perches @ 2013-07-13 5:52 ` Darren Hart 0 siblings, 0 replies; 19+ messages in thread From: Darren Hart @ 2013-07-13 5:52 UTC (permalink / raw) To: Joe Perches Cc: Linux Kernel Mailing List, David S. Miller, H. Peter Anvin, Peter Waskiewicz, Andy Shevchenko, netdev, stable 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support 2013-07-13 0:58 ` [PATCH 3/3] pch_gbe: Add MinnowBoard support Darren Hart 2013-07-13 1:10 ` Joe Perches @ 2013-07-13 1:17 ` Greg KH 2013-07-13 5:46 ` Darren Hart 2013-07-15 8:34 ` Andy Shevchenko ` (2 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Greg KH @ 2013-07-13 1:17 UTC (permalink / raw) To: Darren Hart Cc: Linux Kernel Mailing List, David S. Miller, H. Peter Anvin, Peter Waskiewicz, Andy Shevchenko, netdev, stable 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support 2013-07-13 1:17 ` Greg KH @ 2013-07-13 5:46 ` Darren Hart 2013-07-13 6:26 ` Greg KH 0 siblings, 1 reply; 19+ messages in thread From: Darren Hart @ 2013-07-13 5:46 UTC (permalink / raw) To: Greg KH Cc: Linux Kernel Mailing List, David S. Miller, H. Peter Anvin, Peter Waskiewicz, Andy Shevchenko, netdev, stable 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support 2013-07-13 5:46 ` Darren Hart @ 2013-07-13 6:26 ` Greg KH 2013-07-13 16:08 ` Darren Hart 0 siblings, 1 reply; 19+ messages in thread From: Greg KH @ 2013-07-13 6:26 UTC (permalink / raw) To: Darren Hart Cc: Linux Kernel Mailing List, David S. Miller, H. Peter Anvin, Peter Waskiewicz, Andy Shevchenko, netdev, stable 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support 2013-07-13 6:26 ` Greg KH @ 2013-07-13 16:08 ` Darren Hart 2013-07-13 17:09 ` Greg KH 0 siblings, 1 reply; 19+ messages in thread From: Darren Hart @ 2013-07-13 16:08 UTC (permalink / raw) To: Greg KH Cc: Linux Kernel Mailing List, David S. Miller, H. Peter Anvin, Peter Waskiewicz, Andy Shevchenko, netdev, stable 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support 2013-07-13 16:08 ` Darren Hart @ 2013-07-13 17:09 ` Greg KH 2013-07-13 19:33 ` Darren Hart 0 siblings, 1 reply; 19+ messages in thread From: Greg KH @ 2013-07-13 17:09 UTC (permalink / raw) To: Darren Hart Cc: Linux Kernel Mailing List, David S. Miller, H. Peter Anvin, Peter Waskiewicz, Andy Shevchenko, netdev, stable 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support 2013-07-13 17:09 ` Greg KH @ 2013-07-13 19:33 ` Darren Hart 0 siblings, 0 replies; 19+ messages in thread From: Darren Hart @ 2013-07-13 19:33 UTC (permalink / raw) To: Greg KH Cc: Linux Kernel Mailing List, David S. Miller, H. Peter Anvin, Peter Waskiewicz, Andy Shevchenko, netdev, stable 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support 2013-07-13 0:58 ` [PATCH 3/3] pch_gbe: Add MinnowBoard support Darren Hart 2013-07-13 1:10 ` Joe Perches 2013-07-13 1:17 ` Greg KH @ 2013-07-15 8:34 ` Andy Shevchenko 2013-07-15 20:41 ` Darren Hart 2013-07-18 17:05 ` Darren Hart 2013-07-15 20:55 ` Darren Hart 2013-07-17 20:13 ` Darren Hart 4 siblings, 2 replies; 19+ messages in thread From: Andy Shevchenko @ 2013-07-15 8:34 UTC (permalink / raw) To: Darren Hart Cc: Linux Kernel Mailing List, David S. Miller, H. Peter Anvin, Peter Waskiewicz, netdev, stable 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support 2013-07-15 8:34 ` Andy Shevchenko @ 2013-07-15 20:41 ` Darren Hart 2013-07-18 17:05 ` Darren Hart 1 sibling, 0 replies; 19+ messages in thread From: Darren Hart @ 2013-07-15 20:41 UTC (permalink / raw) To: Andy Shevchenko Cc: Linux Kernel Mailing List, David S. Miller, H. Peter Anvin, Peter Waskiewicz, netdev, stable 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support 2013-07-15 8:34 ` Andy Shevchenko 2013-07-15 20:41 ` Darren Hart @ 2013-07-18 17:05 ` Darren Hart 1 sibling, 0 replies; 19+ messages in thread From: Darren Hart @ 2013-07-18 17:05 UTC (permalink / raw) To: Andy Shevchenko Cc: Linux Kernel Mailing List, David S. Miller, H. Peter Anvin, Peter Waskiewicz, netdev, stable (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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support 2013-07-13 0:58 ` [PATCH 3/3] pch_gbe: Add MinnowBoard support Darren Hart ` (2 preceding siblings ...) 2013-07-15 8:34 ` Andy Shevchenko @ 2013-07-15 20:55 ` Darren Hart 2013-07-17 20:13 ` Darren Hart 4 siblings, 0 replies; 19+ messages in thread From: Darren Hart @ 2013-07-15 20:55 UTC (permalink / raw) To: Linux Kernel Mailing List Cc: David S. Miller, H. Peter Anvin, Peter Waskiewicz, Andy Shevchenko, netdev, stable 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] pch_gbe: Add MinnowBoard support 2013-07-13 0:58 ` [PATCH 3/3] pch_gbe: Add MinnowBoard support Darren Hart ` (3 preceding siblings ...) 2013-07-15 20:55 ` Darren Hart @ 2013-07-17 20:13 ` Darren Hart 4 siblings, 0 replies; 19+ messages in thread From: Darren Hart @ 2013-07-17 20:13 UTC (permalink / raw) To: Linux Kernel Mailing List Cc: David S. Miller, H. Peter Anvin, Peter Waskiewicz, Andy Shevchenko, netdev, stable 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] pch_uart: Use DMI interface for board detection 2013-07-13 0:58 ` [PATCH 1/3] pch_uart: Use DMI interface for board detection Darren Hart 2013-07-13 0:58 ` [PATCH 2/3] pch_gbe: Use PCH_GBE_PHY_REGS_LEN instead of 32 Darren Hart 2013-07-13 0:58 ` [PATCH 3/3] pch_gbe: Add MinnowBoard support Darren Hart @ 2013-07-17 20:15 ` Darren Hart 2013-07-17 21:55 ` Greg Kroah-Hartman 2 siblings, 1 reply; 19+ messages in thread From: Darren Hart @ 2013-07-17 20:15 UTC (permalink / raw) To: Linux Kernel Mailing List; +Cc: Michael Brunner, Greg Kroah-Hartman On Fri, 2013-07-12 at 17:58 -0700, Darren Hart wrote: > Use the DMI interface rather than manually matching DMI strings. Greg, The rest of this series is either being dropped or has moved on to netdev. This patch however remains unchanged. Would you like me to resend individually or can you pull in this one? Thanks, Darren > > Signed-off-by: Darren Hart <dvhart@linux.intel.com> > Cc: Michael Brunner <mibru@gmx.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/tty/serial/pch_uart.c | 71 +++++++++++++++++++++++++++++-------------- > 1 file changed, 49 insertions(+), 22 deletions(-) > > diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c > index 572d481..271cc73 100644 > --- a/drivers/tty/serial/pch_uart.c > +++ b/drivers/tty/serial/pch_uart.c > @@ -373,35 +373,62 @@ static const struct file_operations port_regs_ops = { > }; > #endif /* CONFIG_DEBUG_FS */ > > +static struct dmi_system_id __initdata pch_uart_dmi_table[] = { > + { > + .ident = "CM-iTC", > + { > + DMI_MATCH(DMI_BOARD_NAME, "CM-iTC"), > + }, > + (void *)CMITC_UARTCLK, > + }, > + { > + .ident = "FRI2", > + { > + DMI_MATCH(DMI_BIOS_VERSION, "FRI2"), > + }, > + (void *)FRI2_64_UARTCLK, > + }, > + { > + .ident = "Fish River Island II", > + { > + DMI_MATCH(DMI_PRODUCT_NAME, "Fish River Island II"), > + }, > + (void *)FRI2_48_UARTCLK, > + }, > + { > + .ident = "COMe-mTT", > + { > + DMI_MATCH(DMI_BOARD_NAME, "COMe-mTT"), > + }, > + (void *)NTC1_UARTCLK, > + }, > + { > + .ident = "nanoETXexpress-TT", > + { > + DMI_MATCH(DMI_BOARD_NAME, "nanoETXexpress-TT"), > + }, > + (void *)NTC1_UARTCLK, > + }, > + { > + .ident = "MinnowBoard", > + { > + DMI_MATCH(DMI_BOARD_NAME, "MinnowBoard"), > + }, > + (void *)MINNOW_UARTCLK, > + }, > +}; > + > /* Return UART clock, checking for board specific clocks. */ > static int pch_uart_get_uartclk(void) > { > - const char *cmp; > + const struct dmi_system_id *d; > > if (user_uartclk) > return user_uartclk; > > - cmp = dmi_get_system_info(DMI_BOARD_NAME); > - if (cmp && strstr(cmp, "CM-iTC")) > - return CMITC_UARTCLK; > - > - cmp = dmi_get_system_info(DMI_BIOS_VERSION); > - if (cmp && strnstr(cmp, "FRI2", 4)) > - return FRI2_64_UARTCLK; > - > - cmp = dmi_get_system_info(DMI_PRODUCT_NAME); > - if (cmp && strstr(cmp, "Fish River Island II")) > - return FRI2_48_UARTCLK; > - > - /* Kontron COMe-mTT10 (nanoETXexpress-TT) */ > - cmp = dmi_get_system_info(DMI_BOARD_NAME); > - if (cmp && (strstr(cmp, "COMe-mTT") || > - strstr(cmp, "nanoETXexpress-TT"))) > - return NTC1_UARTCLK; > - > - cmp = dmi_get_system_info(DMI_BOARD_NAME); > - if (cmp && strstr(cmp, "MinnowBoard")) > - return MINNOW_UARTCLK; > + d = dmi_first_match(pch_uart_dmi_table); > + if (d) > + return (int)d->driver_data; > > return DEFAULT_UARTCLK; > } -- Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] pch_uart: Use DMI interface for board detection 2013-07-17 20:15 ` [PATCH 1/3] pch_uart: Use DMI interface for board detection Darren Hart @ 2013-07-17 21:55 ` Greg Kroah-Hartman 0 siblings, 0 replies; 19+ messages in thread From: Greg Kroah-Hartman @ 2013-07-17 21:55 UTC (permalink / raw) To: Darren Hart; +Cc: Linux Kernel Mailing List, Michael Brunner On Wed, Jul 17, 2013 at 01:15:40PM -0700, Darren Hart wrote: > On Fri, 2013-07-12 at 17:58 -0700, Darren Hart wrote: > > Use the DMI interface rather than manually matching DMI strings. > > Greg, > > The rest of this series is either being dropped or has moved on to > netdev. This patch however remains unchanged. Would you like me to > resend individually or can you pull in this one? No need, it's still in my "todo" queue. I'm trying to get the 3.11 patches merged first, before the 3.12 stuff hits my trees. I only have 1549 more patches to get through... greg k-h ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2013-07-18 17:06 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-13 0:58 [PATCH 0/3] MinnowBoard Support V2 (Serial and Ethernet) Darren Hart 2013-07-13 0:58 ` [PATCH 1/3] pch_uart: Use DMI interface for board detection Darren Hart 2013-07-13 0:58 ` [PATCH 2/3] pch_gbe: Use PCH_GBE_PHY_REGS_LEN instead of 32 Darren Hart 2013-07-13 0:58 ` [PATCH 3/3] pch_gbe: Add MinnowBoard support Darren Hart 2013-07-13 1:10 ` Joe Perches 2013-07-13 5:52 ` Darren Hart 2013-07-13 1:17 ` Greg KH 2013-07-13 5:46 ` Darren Hart 2013-07-13 6:26 ` Greg KH 2013-07-13 16:08 ` Darren Hart 2013-07-13 17:09 ` Greg KH 2013-07-13 19:33 ` Darren Hart 2013-07-15 8:34 ` Andy Shevchenko 2013-07-15 20:41 ` Darren Hart 2013-07-18 17:05 ` Darren Hart 2013-07-15 20:55 ` Darren Hart 2013-07-17 20:13 ` Darren Hart 2013-07-17 20:15 ` [PATCH 1/3] pch_uart: Use DMI interface for board detection Darren Hart 2013-07-17 21:55 ` Greg Kroah-Hartman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.