Re: [PATCH v5 02/14] net: pch_gbe: Pull PHY GPIO handling out of Minnow code

[Andrew Lunn]

> 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

Re: [PATCH v5 02/14] net: pch_gbe: Pull PHY GPIO handling out of Minnow code

[Paul Burton]

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

Re: [PATCH v5 02/14] net: pch_gbe: Pull PHY GPIO handling out of Minnow code

[Andrew Lunn]

> 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

Re: [PATCH v5 02/14] net: pch_gbe: Pull PHY GPIO handling out of Minnow code

[Paul Burton]

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

Re: [PATCH v5 02/14] net: pch_gbe: Pull PHY GPIO handling out of Minnow code

[Andrew Lunn]

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

[PATCH v5 02/14] net: pch_gbe: Pull PHY GPIO handling out of Minnow code

[Paul Burton]

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 
Cc: David S. Miller 
Cc: linux-m...@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(-)

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(>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(>hw, 1);
+   usleep_range(1250, 1500);
+   pch_gbe_phy_set_reset(>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(>dev, gpio, flags,
"minnow_phy_reset");
-   if (ret) {
+   if (!ret)
+   pdata->phy_reset_gpio = gpio_to_desc(gpio);
+   else
dev_err(>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;
 }
-- 
2.16.1