Message ID | 20180217201037.3006-3-paul.burton@mips.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Series | None | expand |
On Sat, Feb 17, 2018 at 12:10:25PM -0800, Paul Burton wrote: > The MIPS Boston development board uses the Intel EG20T Platform > Controller Hub, including its gigabit ethernet controller, and requires > that its RTL8211E PHY be reset much like the Minnow platform. Pull the > PHY reset GPIO handling out of Minnow-specific code such that it can be > shared by later patches. Hi Paul I'm i right in saying the driver currently supports the Atheros AT8031 PHY? The same phy which is supported in drivers/net/phy/at803x.c? If so, i think you are doing this all wrong. You would be much better off throwing away pch_gbe_phy.c and write a proper MDIO driver. You then get the PHY driver for free, and the MDIO code could will handle your GPIO for you, in the standardised way. Andrew
Hi Andrew, On Sat, Feb 17, 2018 at 11:29:33PM +0100, Andrew Lunn wrote: > On Sat, Feb 17, 2018 at 12:10:25PM -0800, Paul Burton wrote: > > The MIPS Boston development board uses the Intel EG20T Platform > > Controller Hub, including its gigabit ethernet controller, and requires > > that its RTL8211E PHY be reset much like the Minnow platform. Pull the > > PHY reset GPIO handling out of Minnow-specific code such that it can be > > shared by later patches. > > Hi Paul > > I'm i right in saying the driver currently supports the Atheros AT8031 > PHY? The same phy which is supported in drivers/net/phy/at803x.c? It looks like the driver does contain some code relating to that PHY, but it's not the one I'm using with the MIPS Boston board - there we have a Realtek RTL8211E (as mentioned in the commit message) which is working fine alongside this pch_gbe driver too. > If so, i think you are doing this all wrong. Note that this is a driver which is already in mainline, and I didn't write it. Claiming that *I* am doing this all wrong is a bit of a stretch - all this patch does is make small changes to some existing code, which only tangentially relates to a PHY driver, such that it ceases to be specific to a single platform. > You would be much better off throwing away pch_gbe_phy.c and write a > proper MDIO driver. You then get the PHY driver for free, and the MDIO > code could will handle your GPIO for you, in the standardised way. Even if that is true, rewriting the driver's PHY handling would be a very separate change to the changes this series make which allow this driver to work on a platform besides the Minnowboard. The *only* thing this series does relating to the PHY is allow the reset GPIO to be handled properly - rewriting the existing PHY handling is beyond it's scope. Note that I do have various cleanups to the driver beyond this series which I intend to submit after it is functional for my system[1], so I am not saying that I don't care about improving the driver. But please, let's do one thing at a time. Thanks, Paul [1] https://git.linux-mips.org/cgit/paul/linux.git/log/?h=up417-boston-eth-cleanup
> Note that this is a driver which is already in mainline, and I didn't > write it. Claiming that *I* am doing this all wrong is a bit of a > stretch - all this patch does is make small changes to some existing > code, which only tangentially relates to a PHY driver, such that it > ceases to be specific to a single platform. Hi Paul I would so you are doing it all wrong for the reset GPIO. > Even if that is true, rewriting the driver's PHY handling would be a > very separate change to the changes this series make which allow this > driver to work on a platform besides the Minnowboard. The *only* thing > this series does relating to the PHY is allow the reset GPIO to be > handled properly - rewriting the existing PHY handling is beyond it's > scope. Well, you are adding a device tree binding, which needs to be supported forever. This is going to make things messy in the future when you do such a cleanup that you follow the PHY binding, in that you have to handle both what you add here, and the official PHY binding. I would prefer that for the moment, you drop the PHY binding patches in this series. That is what i object to the most. Adding an MDIO driver and using the standard PHY driver for this PHY is all internal. You can change that anytime. But adding a binding means an ABI. Andrew
Hi Andrew, On Sun, Feb 18, 2018 at 12:34:42AM +0100, Andrew Lunn wrote: > > Even if that is true, rewriting the driver's PHY handling would be a > > very separate change to the changes this series make which allow this > > driver to work on a platform besides the Minnowboard. The *only* thing > > this series does relating to the PHY is allow the reset GPIO to be > > handled properly - rewriting the existing PHY handling is beyond it's > > scope. > > Well, you are adding a device tree binding, which needs to be > supported forever. This is going to make things messy in the future > when you do such a cleanup that you follow the PHY binding, in that > you have to handle both what you add here, and the official PHY > binding. Thank you - it's useful to know what your concern actually is. > I would prefer that for the moment, you drop the PHY binding patches > in this series. That is what i object to the most. Adding an MDIO > driver and using the standard PHY driver for this PHY is all > internal. You can change that anytime. But adding a binding means an > ABI. The problem is that the device in question doesn't actually work unless we reset the PHY, so just removing the PHY reset GPIO handling would break things. How would you feel if I were to adjust the binding to match the standard PHY binding, but internally leave the driver's PHY handling as-is for now? That would: 1) Allow for the pch_gbe driver to move towards more standard PHY handling in the future without DT changes. 2) Be fairly straightforward to implement in this patchset - the code reading the DT would just follow the phandle to the PHY node to find the reset GPIO - thereby not holding up the rest of the series. 3) Still function on our hardware. Thanks, Paul
> How would you feel if I were to adjust the binding to match the standard > PHY binding, but internally leave the driver's PHY handling as-is for > now? Hi Paul That is a reasonable compromise. Thanks Andrew
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 697e29dd4bd3..8ba9ced2d1fd 100644 --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h @@ -580,15 +580,18 @@ struct pch_gbe_hw_stats { /** * struct pch_gbe_privdata - PCI Device ID driver data + * @phy_reset_gpio: PHY reset GPIO descriptor. * @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, prior to PHY initialization. */ struct pch_gbe_privdata { + struct gpio_desc *phy_reset_gpio; bool phy_tx_clk_delay; bool phy_disable_hibernate; - int (*platform_init)(struct pci_dev *pdev); + int (*platform_init)(struct pci_dev *pdev, + struct pch_gbe_privdata *pdata); }; /** 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 d5c6f2e2d3a2..712ac2f7bb2c 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 @@ -360,6 +360,16 @@ static void pch_gbe_mac_mar_set(struct pch_gbe_hw *hw, u8 * addr, u32 index) pch_gbe_wait_clr_bit(&hw->reg->ADDR_MASK, PCH_GBE_BUSY); } +static void pch_gbe_phy_set_reset(struct pch_gbe_hw *hw, int value) +{ + struct pch_gbe_adapter *adapter = pch_gbe_hw_to_adapter(hw); + + if (!adapter->pdata || !adapter->pdata->phy_reset_gpio) + return; + + gpiod_set_value(adapter->pdata->phy_reset_gpio, value); +} + /** * pch_gbe_mac_reset_hw - Reset hardware * @hw: Pointer to the HW structure @@ -2592,7 +2602,14 @@ static int pch_gbe_probe(struct pci_dev *pdev, 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->pdata->platform_init(pdev, adapter->pdata); + + if (adapter->pdata && adapter->pdata->phy_reset_gpio) { + pch_gbe_phy_set_reset(&adapter->hw, 1); + usleep_range(1250, 1500); + pch_gbe_phy_set_reset(&adapter->hw, 0); + usleep_range(1250, 1500); + } adapter->ptp_pdev = pci_get_domain_bus_and_slot(pci_domain_nr(adapter->pdev->bus), @@ -2686,7 +2703,8 @@ static int pch_gbe_probe(struct pci_dev *pdev, /* 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) +static int pch_gbe_minnow_platform_init(struct pci_dev *pdev, + struct pch_gbe_privdata *pdata) { unsigned long flags = GPIOF_DIR_OUT | GPIOF_INIT_LOW | GPIOF_EXPORT | GPIOF_ACTIVE_LOW; @@ -2695,16 +2713,11 @@ static int pch_gbe_minnow_platform_init(struct pci_dev *pdev) ret = devm_gpio_request_one(&pdev->dev, gpio, flags, "minnow_phy_reset"); - if (ret) { + if (!ret) + pdata->phy_reset_gpio = gpio_to_desc(gpio); + else dev_err(&pdev->dev, "ERR: Can't request PHY reset GPIO line '%d'\n", gpio); - return ret; - } - - gpio_set_value(gpio, 1); - usleep_range(1250, 1500); - gpio_set_value(gpio, 0); - usleep_range(1250, 1500); return ret; }
The MIPS Boston development board uses the Intel EG20T Platform Controller Hub, including its gigabit ethernet controller, and requires that its RTL8211E PHY be reset much like the Minnow platform. Pull the PHY reset GPIO handling out of Minnow-specific code such that it can be shared by later patches. Signed-off-by: Paul Burton <paul.burton@mips.com> Cc: David S. Miller <davem@davemloft.net> Cc: linux-mips@linux-mips.org Cc: netdev@vger.kernel.org --- Changes in v5: - Name struct pch_gbe_privdata's platform_init pdata arg, per checkpatch. Changes in v4: None Changes in v3: - Use adapter->pdata as arg to platform_init, to fix bisectability. Changes in v2: None drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h | 5 +++- .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 33 +++++++++++++++------- 2 files changed, 27 insertions(+), 11 deletions(-)