Re: [PATCH v3 2/7] net: pch_gbe: Pull PHY GPIO handling out of Minnow code

[Andrew Lunn]

On Mon, Jun 05, 2017 at 10:21:50AM -0700, Paul Burton wrote:
> Hi Andrew,
> 
> On Saturday, 3 June 2017 10:52:00 PDT Andrew Lunn wrote:
> > > 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
> > > d38198718005..cb9b904786e4 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);
> > 
> > Hi Paul
> > 
> > Since you are using the gpiod_ API, the core will take notice of the
> > active low/active high flag when performing this set.
> 
> Correct, and as desired.
> 
> > >   ret = devm_gpio_request_one(>dev, gpio, flags,
> > >   
> > >   "minnow_phy_reset");
> > > 
> > > - if (ret) {
> > > + if (!ret)
> > > + pdata->phy_reset_gpio = gpio_to_desc(gpio);
> > 
> > Here however, you are using the gpio_ API, which ignores the active
> > high/low flag in device tree. And in your binding patch, you give the
> > example:
> > 
> > +   phy-reset-gpios = <_gpio 6
> > +  GPIO_ACTIVE_LOW>;
> > 
> > This active low is totally ignored.
> 
> First of all, this path is for the existing Minnow platform, which doesn't 
> use 
> the device tree. That is, this code is the non-DT path so looking at what 
> happens to flags in the device tree here makes no sense.

Thanks for the explanation. Now it makes sense.

   Andrew

Re: [PATCH v3 2/7] net: pch_gbe: Pull PHY GPIO handling out of Minnow code

[Paul Burton]

Hi Andrew,

On Saturday, 3 June 2017 10:52:00 PDT Andrew Lunn wrote:
> > 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
> > d38198718005..cb9b904786e4 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);
> 
> Hi Paul
> 
> Since you are using the gpiod_ API, the core will take notice of the
> active low/active high flag when performing this set.

Correct, and as desired.

> > ret = devm_gpio_request_one(>dev, gpio, flags,
> > 
> > "minnow_phy_reset");
> > 
> > -   if (ret) {
> > +   if (!ret)
> > +   pdata->phy_reset_gpio = gpio_to_desc(gpio);
> 
> Here however, you are using the gpio_ API, which ignores the active
> high/low flag in device tree. And in your binding patch, you give the
> example:
> 
> +   phy-reset-gpios = <_gpio 6
> +  GPIO_ACTIVE_LOW>;
> 
> This active low is totally ignored.

First of all, this path is for the existing Minnow platform, which doesn't use 
the device tree. That is, this code is the non-DT path so looking at what 
happens to flags in the device tree here makes no sense.

If you want to examine what happens in the DT case then please look at 
pch_gbe_get_priv() which uses devm_gpiod_get() which should honor the flags 
provided by the DT.

> I personally would say this is all messed up, and going to result in
> problems for somebody with a board which actually needs an
> GPIO_ACTIVE_HIGH.

It's a path which only applies to the Minnow board, which is always active 
low. Before patch 1 of this series that was done without the GPIOF_ACTIVE_LOW 
flag by setting GPIO values to reflect the physical GPIO line low/high rather 
than the logical active/not-active. After patch 1 this path began using 
GPIOF_ACTIVE_LOW such that the rest of the code can use logical active/not-
active values which work with either active low or active high GPIOs. In this 
Minnow-specific path GPIOF_ACTIVE_LOW is hardcoded, but again only applies to 
the Minnow board which doesn't take the GPIO value from device tree.

> Please use the gpiod_ API through out and respect the flags in the
> device tree binding.

The gpiod_ API, quite rightly, retrieves GPIOs associated with a device - for 
example via the device tree. The Minnow board, which is what the driver 
already supports in-tree, does not do this but instead hardcodes a GPIO number 
(MINNOW_PHY_RESET_GPIO). I don't own, use or care about the Minnow platform so 
that is not something that I can change. In the path that my patch does add, 
the path which is used with DT, I already do use the gpiod_ API & respect 
flags from the DT.

Thanks,
Paul

signature.asc
Description: This is a digitally signed message part.

Re: [PATCH v3 2/7] net: pch_gbe: Pull PHY GPIO handling out of Minnow code

[Andrew Lunn]

On Fri, Jun 02, 2017 at 04:40:37PM -0700, 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.
> 
> Signed-off-by: Paul Burton 
> Cc: David S. Miller 
> Cc: Eric Dumazet 
> Cc: Jarod Wilson 
> Cc: Tobias Klauser 
> Cc: linux-m...@linux-mips.org
> Cc: netdev@vger.kernel.org
> ---
> 
> 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|  4 ++-
>  .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   | 33 
> +++---
>  2 files changed, 26 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 8d710a3b4db0..de1dd08050f4 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,17 @@ 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 *, struct pch_gbe_privdata *);
>  };
>  
>  /**
> 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 d38198718005..cb9b904786e4 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);

Hi Paul

Since you are using the gpiod_ API, the core will take notice of the
active low/active high flag when performing this set.

> +}
> +
>  /**
>   * pch_gbe_mac_reset_hw - Reset hardware
>   * @hw:  Pointer to the HW structure
>  
>   ret = devm_gpio_request_one(>dev, gpio, flags,
>   "minnow_phy_reset");
> - if (ret) {
> + if (!ret)
> + pdata->phy_reset_gpio = gpio_to_desc(gpio);

Here however, you are using the gpio_ API, which ignores the active
high/low flag in device tree. And in your binding patch, you give the
example:

+   phy-reset-gpios = <_gpio 6
+  GPIO_ACTIVE_LOW>;

This active low is totally ignored.

I personally would say this is all messed up, and going to result in
problems for somebody with a board which actually needs an
GPIO_ACTIVE_HIGH.

Please use the gpiod_ API through out and respect the flags in the
device tree binding.

   Andrew

[PATCH v3 2/7] 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: Eric Dumazet 
Cc: Jarod Wilson 
Cc: Tobias Klauser 
Cc: linux-m...@linux-mips.org
Cc: netdev@vger.kernel.org
---

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|  4 ++-
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   | 33 +++---
 2 files changed, 26 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 8d710a3b4db0..de1dd08050f4 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,17 @@ 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 *, struct pch_gbe_privdata *);
 };
 
 /**
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 d38198718005..cb9b904786e4 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
@@ -2601,7 +2611,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_bus_and_slot(adapter->pdev->bus->number,
   PCI_DEVFN(12, 4));
@@ -2694,7 +2711,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;
@@ -2703,16 +2721,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.13.0