[net-next,2/3] net: nixge: Add support for having nixge as subdevice

Add support for instantiating nixge as subdevice using
fixed-link and platform data to configure it.

Signed-off-by: Moritz Fischer <mdf@kernel.org>
---

Hi,

not this patch is still in the early stages,
and as the rest of this series goes on top of [1].

The actual platform data might still change since
the parent device driver is still under development.

Thanks for your time,

Moritz


[1] https://lkml.org/lkml/2018/8/28/1011

---
 drivers/net/ethernet/ni/nixge.c     | 71 ++++++++++++++++++++++++++---
 include/linux/platform_data/nixge.h | 19 ++++++++
 2 files changed, 83 insertions(+), 7 deletions(-)
 create mode 100644 include/linux/platform_data/nixge.h

On Wed, Aug 29, 2018 at 5:49 PM Moritz Fischer <mdf@kernel.org> wrote:
>
> Add support for instantiating nixge as subdevice using
> fixed-link and platform data to configure it.
>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
>
> Hi,
>
> not this patch is still in the early stages,
> and as the rest of this series goes on top of [1].
>
> The actual platform data might still change since
> the parent device driver is still under development.
>
> Thanks for your time,
>
> Moritz
>
>
> [1] https://lkml.org/lkml/2018/8/28/1011
>
> ---
>  drivers/net/ethernet/ni/nixge.c     | 71 ++++++++++++++++++++++++++---
>  include/linux/platform_data/nixge.h | 19 ++++++++
>  2 files changed, 83 insertions(+), 7 deletions(-)
>  create mode 100644 include/linux/platform_data/nixge.h
>
> diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
> index 670249313ff0..fd8e5b02c459 100644
> --- a/drivers/net/ethernet/ni/nixge.c
> +++ b/drivers/net/ethernet/ni/nixge.c
> @@ -7,6 +7,8 @@
>  #include <linux/etherdevice.h>
>  #include <linux/module.h>
>  #include <linux/netdevice.h>
> +#include <linux/phy_fixed.h>
> +#include <linux/platform_data/nixge.h>
>  #include <linux/of_address.h>
>  #include <linux/of_mdio.h>
>  #include <linux/of_net.h>
> @@ -167,6 +169,7 @@ struct nixge_priv {
>         /* Connection to PHY device */
>         struct device_node *phy_node;
>         phy_interface_t         phy_mode;
> +       struct phy_device *phydev;
>
>         int link;
>         unsigned int speed;
> @@ -859,15 +862,25 @@ static void nixge_dma_err_handler(unsigned long data)
>  static int nixge_open(struct net_device *ndev)
>  {
>         struct nixge_priv *priv = netdev_priv(ndev);
> -       struct phy_device *phy;
> +       struct phy_device *phy = NULL;
>         int ret;
>
>         nixge_device_reset(ndev);
>
> -       phy = of_phy_connect(ndev, priv->phy_node,
> -                            &nixge_handle_link_change, 0, priv->phy_mode);
> -       if (!phy)
> -               return -ENODEV;
> +       if (priv->dev->of_node) {
> +               phy = of_phy_connect(ndev, priv->phy_node,
> +                                    &nixge_handle_link_change, 0,
> +                                    priv->phy_mode);
> +               if (!phy)
> +                       return -ENODEV;
> +       } else if (priv->phydev) {
> +               ret = phy_connect_direct(ndev, priv->phydev,
> +                                        &nixge_handle_link_change,
> +                                        priv->phy_mode);
> +               if (ret)
> +                       return ret;
> +               phy = priv->phydev;
> +       }
>
>         phy_start(phy);
>
> @@ -1215,10 +1228,41 @@ static int nixge_of_get_phy(struct nixge_priv *priv, struct device_node *np)
>         return 0;
>  }
>
> +static int nixge_pdata_get_phy(struct nixge_priv *priv,
> +                              struct nixge_platform_data *pdata)
> +{
> +       struct phy_device *phy = NULL;
> +
> +       if (!pdata)
> +               return -EINVAL;
> +
> +       if (pdata && pdata->phy_interface == PHY_INTERFACE_MODE_NA) {
> +               struct fixed_phy_status fphy_status = {
> +                       .link = 1,
> +                       .duplex = pdata->phy_duplex,
> +                       .speed = pdata->phy_speed,
> +                       .pause = 0,
> +                       .asym_pause = 0,
> +               };
> +
> +               /* TODO: Pull out GPIO from pdata */
> +               phy = fixed_phy_register(PHY_POLL, &fphy_status, -1,
> +                                        NULL);
> +               if (IS_ERR_OR_NULL(phy)) {
> +                       dev_err(priv->dev,
> +                               "failed to register fixed PHY device\n");
> +                       return -ENODEV;
> +               }
> +       }
> +       priv->phy_mode = pdata->phy_interface;
> +       priv->phydev = phy;
> +
> +       return 0;
> +}
> +
>  static int nixge_mdio_setup(struct nixge_priv *priv, struct device_node *np)
>  {
>         struct mii_bus *bus;
> -       int err;
>
>         bus = devm_mdiobus_alloc(priv->dev);
>         if (!bus)
> @@ -1254,6 +1298,7 @@ static void *nixge_get_nvmem_address(struct device *dev)
>
>  static int nixge_probe(struct platform_device *pdev)
>  {
> +       struct nixge_platform_data *pdata = NULL;
>         struct nixge_priv *priv;
>         struct net_device *ndev;
>         struct resource *dmares;
> @@ -1320,10 +1365,16 @@ static int nixge_probe(struct platform_device *pdev)
>                 err = nixge_of_get_phy(priv, np);
>                 if (err)
>                         goto free_netdev;
> +       } else {
> +               pdata = dev_get_platdata(&pdev->dev);
> +               err = nixge_pdata_get_phy(priv, pdata);
> +               if (err)
> +                       goto free_netdev;
>         }
>
>         /* only if it's not a fixed link, do we care about MDIO at all */
> -       if (priv->phy_node && !of_phy_is_fixed_link(np)) {
> +       if ((priv->phy_node && !of_phy_is_fixed_link(np)) ||
> +           (pdata && pdata->phy_interface != PHY_INTERFACE_MODE_NA)) {

Must've messed up the rebase. Missing a parents. I'll resubmit this
one. Sorry for the noise.
>                 err = nixge_mdio_setup(priv, np);
>                 if (err) {
>                         dev_err(&pdev->dev, "error registering mdio bus");
> @@ -1347,6 +1398,9 @@ static int nixge_probe(struct platform_device *pdev)
>                 of_phy_deregister_fixed_link(np);
>                 of_node_put(np);
>         }
> +
> +       if (priv->phydev && phy_is_pseudo_fixed_link(priv->phydev))
> +               fixed_phy_unregister(priv->phydev);
>  free_netdev:
>         free_netdev(ndev);
>
> @@ -1357,6 +1411,7 @@ static int nixge_remove(struct platform_device *pdev)
>  {
>         struct net_device *ndev = platform_get_drvdata(pdev);
>         struct nixge_priv *priv = netdev_priv(ndev);
> +       struct device_node *np = pdev->dev.of_node;
>
>         unregister_netdev(ndev);
>
> @@ -1365,6 +1420,8 @@ static int nixge_remove(struct platform_device *pdev)
>
>         if (np && of_phy_is_fixed_link(np))
>                 of_phy_deregister_fixed_link(np);
> +       else if (priv->phydev && phy_is_pseudo_fixed_link(priv->phydev))
> +               fixed_phy_unregister(priv->phydev);
>
>         free_netdev(ndev);
>
> diff --git a/include/linux/platform_data/nixge.h b/include/linux/platform_data/nixge.h
> new file mode 100644
> index 000000000000..aa5dd5760074
> --- /dev/null
> +++ b/include/linux/platform_data/nixge.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2018 National Instruments Corp.
> + *
> + * Author: Moritz Fischer <mdf@kernel.org>
> + */
> +
> +#ifndef __NIXGE_PDATA_H__
> +#define __NIXGE_PDATA_H__
> +
> +#include <linux/phy.h>
> +
> +struct nixge_platform_data {
> +       phy_interface_t phy_interface;
> +       int phy_speed;
> +       int phy_duplex;
> +};
> +
> +#endif /* __NIXGE_PDATA_H__ */
> +
> --
> 2.18.0
>

Thanks,
Moritz

On Wed, Aug 29, 2018 at 05:40:45PM -0700, Moritz Fischer wrote:
> Add support for instantiating nixge as subdevice using
> fixed-link and platform data to configure it.
> 
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
> 
> Hi,
> 
> not this patch is still in the early stages,
> and as the rest of this series goes on top of [1].
> 
> The actual platform data might still change since
> the parent device driver is still under development.

Hi Moritz

Could you tell us more about the parent device. I'm guessing PCIe.  Is
it x86 so no device tree? Are there cases where it does not have a PHY
connected? What is connected instead? SFP? A switch? Can there be
multiple PHYs on the MDIO bus?

Answering these questions will help decide how best to structure this.

	 Andrew

Hi Andrew,

On Wed, Aug 29, 2018 at 8:11 PM, Andrew Lunn <andrew@lunn.ch> wrote:

> Could you tell us more about the parent device. I'm guessing PCIe.  Is
> it x86 so no device tree? Are there cases where it does not have a PHY
> connected? What is connected instead? SFP? A switch? Can there be
> multiple PHYs on the MDIO bus?

The device is part of a larger FPGA design. One use case that I was trying
to support with this patch is PCIe with x86 (hopefully on it's own PF...)
Since the whole design isn't completely done, these are the use cases I
see upcoming and current:

ARM(64):
a) DT: PHY over MDIO (current use case), fixed-link with GPIO (coming)
b) DT: SFP (potentially coming)

x86:
a) no PHY (coming)-> fixed-link with GPIO
b) SFP (potentially), PHY over MDIO (potentially)

Thanks for your help,

Moritz

On Thu, Aug 30, 2018 at 09:39:39AM -0700, Moritz Fischer wrote:
> Hi Andrew,
> 
> On Wed, Aug 29, 2018 at 8:11 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > Could you tell us more about the parent device. I'm guessing PCIe.  Is
> > it x86 so no device tree? Are there cases where it does not have a PHY
> > connected? What is connected instead? SFP? A switch? Can there be
> > multiple PHYs on the MDIO bus?
> 
> The device is part of a larger FPGA design. One use case that I was trying
> to support with this patch is PCIe with x86 (hopefully on it's own PF...)
> Since the whole design isn't completely done, these are the use cases I
> see upcoming and current:
> 
> ARM(64):
> a) DT: PHY over MDIO (current use case), fixed-link with GPIO (coming)
> b) DT: SFP (potentially coming)
> 
> x86:
> a) no PHY (coming)-> fixed-link with GPIO
> b) SFP (potentially), PHY over MDIO (potentially)

Hi Mortiz

For SFP, you need to convert this driver to use phylink. That will
also help you with fixed-link, since phylink will handle probing that
for you.

But this brings its own problems. phylink and sfp currently has no
support for platform devices. The SFP driver needs to know which i2c
bus to use, and which GPIOs are connected to the SFP. phylink parses
the device tree to find out if there is an SFP device, or a fixed
link, etc. I don't know of any conceptual reason why platform data
would not work, it just needs implementing.

There also does not appear to be any in kernel users of the device
tree binding. That gives you some flexibility in that you could think
about making non-backwards compatible changes in the binding. I would
definitely want to move PHYs into an mdio subnode.

I'm not aware of any x86 drivers using fixed link. What they generally
do is register the mdio bus using mdiobus_register() and then use
phy_find_first() to get a PHY. This works O.K. for PCs, laptops, and
PCIe cards where there is only one PHY on the bus. What you might be
able to do is always register the MDIO bus, and if you fail to find a
PHY, instantiate a fixed-link and use that instead.

Reality is, all the core work in this area has been pushed by the
embedded world, which is ARM and device tree. For Intel and Server
style networking, drivers tend to either ignore the Linux core code
and write there own PHY and MDIO bus drivers, or it is all done in
firmware.

     Andrew