Hi,
this series adds support for using nixge with fixed-link nodes,
as well as an early attempt to support nixge as a subdevice of
another device.
This series goes on top of: https://lkml.org/lkml/2018/8/28/1011
Patch 1: Adds of based fixed-link support, and hopefully isn't too
controversial barring grave mistakes.
Patch 2: Is an attempt at making sure that nixge works as a subdevice
of another (pci) device without a PHY. Note that same as Patch
3 the actual platform data might still change since the parent
device driver is still under development. So this is more of an
RFC, too.
Patch 3: Is more of an RFC at this point since the PCI parent device is
still under development, but required to run with IOMMU
support. Feedback on this one would be appreciated.
Thanks for your input,
Moritz
Moritz Fischer (3):
net: nixge: Add support for fixed-link subnodes
net: nixge: Add support for having nixge as subdevice
net: nixge: Use sysdev instead of ndev->dev.parent for DMA
drivers/net/ethernet/ni/nixge.c | 187 ++++++++++++++++++++++------
include/linux/platform_data/nixge.h | 19 +++
2 files changed, 165 insertions(+), 41 deletions(-)
create mode 100644 include/linux/platform_data/nixge.h
--
2.18.0
Use sysdev instead of ndev->dev.parent for DMA in order to
enable usecases where an IOMMU might get in the way otherwise.
In the PCIe wrapper case on x86 the IOMMU group is not inherited
by the platform device that is created as a subdevice of the
PCI wrapper driver.
Signed-off-by: Moritz Fischer <[email protected]>
---
Hi,
not this patch is still in the early stages,
and as the rest of this series goes on top of [1].
The actual parent device might still change since
the parent device driver is still under development.
If there are clever suggestions on how to generalize
the assignment of sysdev, I'm all ears.
Thanks for your time,
Moritz
[1] https://lkml.org/lkml/2018/8/28/1011
---
drivers/net/ethernet/ni/nixge.c | 50 +++++++++++++++++++++------------
1 file changed, 32 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
index fd8e5b02c459..25fb1642d558 100644
--- a/drivers/net/ethernet/ni/nixge.c
+++ b/drivers/net/ethernet/ni/nixge.c
@@ -16,6 +16,7 @@
#include <linux/of_irq.h>
#include <linux/skbuff.h>
#include <linux/phy.h>
+#include <linux/pci.h>
#include <linux/mii.h>
#include <linux/nvmem-consumer.h>
#include <linux/ethtool.h>
@@ -165,6 +166,7 @@ struct nixge_priv {
struct net_device *ndev;
struct napi_struct napi;
struct device *dev;
+ struct device *sysdev;
/* Connection to PHY device */
struct device_node *phy_node;
@@ -250,7 +252,7 @@ static void nixge_hw_dma_bd_release(struct net_device *ndev)
phys_addr = nixge_hw_dma_bd_get_addr(&priv->rx_bd_v[i],
phys);
- dma_unmap_single(ndev->dev.parent, phys_addr,
+ dma_unmap_single(priv->sysdev, phys_addr,
NIXGE_MAX_JUMBO_FRAME_SIZE,
DMA_FROM_DEVICE);
@@ -261,16 +263,16 @@ static void nixge_hw_dma_bd_release(struct net_device *ndev)
}
if (priv->rx_bd_v)
- dma_free_coherent(ndev->dev.parent,
+ dma_free_coherent(priv->sysdev,
sizeof(*priv->rx_bd_v) * RX_BD_NUM,
priv->rx_bd_v,
priv->rx_bd_p);
if (priv->tx_skb)
- devm_kfree(ndev->dev.parent, priv->tx_skb);
+ devm_kfree(priv->sysdev, priv->tx_skb);
if (priv->tx_bd_v)
- dma_free_coherent(ndev->dev.parent,
+ dma_free_coherent(priv->sysdev,
sizeof(*priv->tx_bd_v) * TX_BD_NUM,
priv->tx_bd_v,
priv->tx_bd_p);
@@ -290,19 +292,19 @@ static int nixge_hw_dma_bd_init(struct net_device *ndev)
priv->rx_bd_ci = 0;
/* Allocate the Tx and Rx buffer descriptors. */
- priv->tx_bd_v = dma_zalloc_coherent(ndev->dev.parent,
+ priv->tx_bd_v = dma_zalloc_coherent(priv->sysdev,
sizeof(*priv->tx_bd_v) * TX_BD_NUM,
&priv->tx_bd_p, GFP_KERNEL);
if (!priv->tx_bd_v)
goto out;
- priv->tx_skb = devm_kcalloc(ndev->dev.parent,
+ priv->tx_skb = devm_kcalloc(priv->sysdev,
TX_BD_NUM, sizeof(*priv->tx_skb),
GFP_KERNEL);
if (!priv->tx_skb)
goto out;
- priv->rx_bd_v = dma_zalloc_coherent(ndev->dev.parent,
+ priv->rx_bd_v = dma_zalloc_coherent(priv->sysdev,
sizeof(*priv->rx_bd_v) * RX_BD_NUM,
&priv->rx_bd_p, GFP_KERNEL);
if (!priv->rx_bd_v)
@@ -327,7 +329,7 @@ static int nixge_hw_dma_bd_init(struct net_device *ndev)
goto out;
nixge_hw_dma_bd_set_offset(&priv->rx_bd_v[i], skb);
- phys = dma_map_single(ndev->dev.parent, skb->data,
+ phys = dma_map_single(priv->sysdev, skb->data,
NIXGE_MAX_JUMBO_FRAME_SIZE,
DMA_FROM_DEVICE);
@@ -438,10 +440,10 @@ static void nixge_tx_skb_unmap(struct nixge_priv *priv,
{
if (tx_skb->mapping) {
if (tx_skb->mapped_as_page)
- dma_unmap_page(priv->ndev->dev.parent, tx_skb->mapping,
+ dma_unmap_page(priv->sysdev, tx_skb->mapping,
tx_skb->size, DMA_TO_DEVICE);
else
- dma_unmap_single(priv->ndev->dev.parent,
+ dma_unmap_single(priv->sysdev,
tx_skb->mapping,
tx_skb->size, DMA_TO_DEVICE);
tx_skb->mapping = 0;
@@ -519,9 +521,9 @@ static int nixge_start_xmit(struct sk_buff *skb, struct net_device *ndev)
return NETDEV_TX_OK;
}
- cur_phys = dma_map_single(ndev->dev.parent, skb->data,
+ cur_phys = dma_map_single(priv->sysdev, skb->data,
skb_headlen(skb), DMA_TO_DEVICE);
- if (dma_mapping_error(ndev->dev.parent, cur_phys))
+ if (dma_mapping_error(priv->sysdev, cur_phys))
goto drop;
nixge_hw_dma_bd_set_phys(cur_p, cur_phys);
@@ -539,10 +541,10 @@ static int nixge_start_xmit(struct sk_buff *skb, struct net_device *ndev)
tx_skb = &priv->tx_skb[priv->tx_bd_tail];
frag = &skb_shinfo(skb)->frags[ii];
- cur_phys = skb_frag_dma_map(ndev->dev.parent, frag, 0,
+ cur_phys = skb_frag_dma_map(priv->sysdev, frag, 0,
skb_frag_size(frag),
DMA_TO_DEVICE);
- if (dma_mapping_error(ndev->dev.parent, cur_phys))
+ if (dma_mapping_error(priv->sysdev, cur_phys))
goto frag_err;
nixge_hw_dma_bd_set_phys(cur_p, cur_phys);
@@ -579,7 +581,7 @@ static int nixge_start_xmit(struct sk_buff *skb, struct net_device *ndev)
cur_p = &priv->tx_bd_v[priv->tx_bd_tail];
cur_p->status = 0;
}
- dma_unmap_single(priv->ndev->dev.parent,
+ dma_unmap_single(priv->sysdev,
tx_skb->mapping,
tx_skb->size, DMA_TO_DEVICE);
drop:
@@ -611,7 +613,7 @@ static int nixge_recv(struct net_device *ndev, int budget)
if (length > NIXGE_MAX_JUMBO_FRAME_SIZE)
length = NIXGE_MAX_JUMBO_FRAME_SIZE;
- dma_unmap_single(ndev->dev.parent,
+ dma_unmap_single(priv->sysdev,
nixge_hw_dma_bd_get_addr(cur_p, phys),
NIXGE_MAX_JUMBO_FRAME_SIZE,
DMA_FROM_DEVICE);
@@ -636,10 +638,10 @@ static int nixge_recv(struct net_device *ndev, int budget)
if (!new_skb)
return packets;
- cur_phys = dma_map_single(ndev->dev.parent, new_skb->data,
+ cur_phys = dma_map_single(priv->sysdev, new_skb->data,
NIXGE_MAX_JUMBO_FRAME_SIZE,
DMA_FROM_DEVICE);
- if (dma_mapping_error(ndev->dev.parent, cur_phys)) {
+ if (dma_mapping_error(priv->sysdev, cur_phys)) {
/* FIXME: bail out and clean up */
netdev_err(ndev, "Failed to map ...\n");
}
@@ -1303,6 +1305,7 @@ static int nixge_probe(struct platform_device *pdev)
struct net_device *ndev;
struct resource *dmares;
struct device_node *np;
+ struct device *sysdev;
const u8 *mac_addr;
int err;
@@ -1331,9 +1334,20 @@ static int nixge_probe(struct platform_device *pdev)
eth_hw_addr_random(ndev);
}
+ sysdev = &pdev->dev;
+#ifdef CONFIG_PCI
+ /* if this is a subdevice of a PCI device we'll have to
+ * make sure we'll use parent instead of our own platform
+ * device to make sure the DMA API if we use an IOMMU
+ */
+ if (sysdev->parent && sysdev->parent->bus == &pci_bus_type)
+ sysdev = sysdev->parent;
+#endif
+
priv = netdev_priv(ndev);
priv->ndev = ndev;
priv->dev = &pdev->dev;
+ priv->sysdev = sysdev;
netif_napi_add(ndev, &priv->napi, nixge_poll, NAPI_POLL_WEIGHT);
--
2.18.0
Add support for instantiating nixge as subdevice using
fixed-link and platform data to configure it.
Signed-off-by: Moritz Fischer <[email protected]>
---
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)) {
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 <[email protected]>
+ */
+
+#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
Add support for fixed-link cases where no MDIO is
actually required to run the device.
In that case no MDIO bus is instantiated since the
actual registers are not available in hardware.
Signed-off-by: Moritz Fischer <[email protected]>
---
drivers/net/ethernet/ni/nixge.c | 72 ++++++++++++++++++++++++---------
1 file changed, 53 insertions(+), 19 deletions(-)
diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
index 74cf52e3fb09..670249313ff0 100644
--- a/drivers/net/ethernet/ni/nixge.c
+++ b/drivers/net/ethernet/ni/nixge.c
@@ -1189,9 +1189,36 @@ static int nixge_mdio_write(struct mii_bus *bus, int phy_id, int reg, u16 val)
return err;
}
+static int nixge_of_get_phy(struct nixge_priv *priv, struct device_node *np)
+{
+ priv->phy_mode = of_get_phy_mode(np);
+ if (priv->phy_mode < 0) {
+ dev_err(priv->dev, "not find \"phy-mode\" property\n");
+ return -EINVAL;
+ }
+
+ if (of_phy_is_fixed_link(np)) {
+ if (of_phy_register_fixed_link(np) < 0) {
+ dev_err(priv->dev, "broken fixed link spec\n");
+ return -EINVAL;
+ }
+
+ priv->phy_node = of_node_get(np);
+ } else {
+ priv->phy_node = of_parse_phandle(np, "phy-handle", 0);
+ if (!priv->phy_node) {
+ dev_err(priv->dev, "not find \"phy-handle\" property\n");
+ return -EINVAL;
+ }
+ }
+
+ 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)
@@ -1230,6 +1257,7 @@ static int nixge_probe(struct platform_device *pdev)
struct nixge_priv *priv;
struct net_device *ndev;
struct resource *dmares;
+ struct device_node *np;
const u8 *mac_addr;
int err;
@@ -1237,6 +1265,8 @@ static int nixge_probe(struct platform_device *pdev)
if (!ndev)
return -ENOMEM;
+ np = pdev->dev.of_node;
+
platform_set_drvdata(pdev, ndev);
SET_NETDEV_DEV(ndev, &pdev->dev);
@@ -1286,24 +1316,19 @@ static int nixge_probe(struct platform_device *pdev)
priv->coalesce_count_rx = XAXIDMA_DFT_RX_THRESHOLD;
priv->coalesce_count_tx = XAXIDMA_DFT_TX_THRESHOLD;
- err = nixge_mdio_setup(priv, pdev->dev.of_node);
- if (err) {
- netdev_err(ndev, "error registering mdio bus");
- goto free_netdev;
- }
-
- priv->phy_mode = of_get_phy_mode(pdev->dev.of_node);
- if (priv->phy_mode < 0) {
- netdev_err(ndev, "not find \"phy-mode\" property\n");
- err = -EINVAL;
- goto unregister_mdio;
+ if (np) {
+ err = nixge_of_get_phy(priv, np);
+ if (err)
+ goto free_netdev;
}
- priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
- if (!priv->phy_node) {
- netdev_err(ndev, "not find \"phy-handle\" property\n");
- err = -EINVAL;
- goto unregister_mdio;
+ /* 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)) {
+ err = nixge_mdio_setup(priv, np);
+ if (err) {
+ dev_err(&pdev->dev, "error registering mdio bus");
+ goto free_phy;
+ }
}
err = register_netdev(priv->ndev);
@@ -1315,8 +1340,13 @@ static int nixge_probe(struct platform_device *pdev)
return 0;
unregister_mdio:
- mdiobus_unregister(priv->mii_bus);
-
+ if (priv->mii_bus)
+ mdiobus_unregister(priv->mii_bus);
+free_phy:
+ if (np && of_phy_is_fixed_link(np)) {
+ of_phy_deregister_fixed_link(np);
+ of_node_put(np);
+ }
free_netdev:
free_netdev(ndev);
@@ -1330,7 +1360,11 @@ static int nixge_remove(struct platform_device *pdev)
unregister_netdev(ndev);
- mdiobus_unregister(priv->mii_bus);
+ if (priv->mii_bus)
+ mdiobus_unregister(priv->mii_bus);
+
+ if (np && of_phy_is_fixed_link(np))
+ of_phy_deregister_fixed_link(np);
free_netdev(ndev);
--
2.18.0
On Wed, Aug 29, 2018 at 5:49 PM Moritz Fischer <[email protected]> wrote:
>
> Add support for instantiating nixge as subdevice using
> fixed-link and platform data to configure it.
>
> Signed-off-by: Moritz Fischer <[email protected]>
> ---
>
> 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 <[email protected]>
> + */
> +
> +#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:44PM -0700, Moritz Fischer wrote:
> Add support for fixed-link cases where no MDIO is
> actually required to run the device.
> In that case no MDIO bus is instantiated since the
> actual registers are not available in hardware.
Hi Moritz
There are a few different use cases here:
The hardware is missing MDIO - You need fixed-link.
The hardware has MDIO, but you don't have a PHY connected on it, and
use fixed link.
The hardware has MDIO, and it is used e.g. for an Ethernet switch, or
a PHY for another Ethernet interface. Plus you need fixed link.
The binding typically looks like:
&fec1 {
phy-mode = "rmii";
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_fec1>;
status = "okay";
fixed-link {
speed = <100>;
full-duplex;
};
mdio1: mdio {
#address-cells = <1>;
#size-cells = <0>;
status = "okay";
switch0: switch0@0 {
compatible = "marvell,mv88e6085";
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_switch>;
reg = <0>;
eeprom-length = <512>;
interrupt-parent = <&gpio3>;
It is important you have the mdio subnode, with PHYs and switches as
children. The driver currently gets this wrong, it uses
pdev->dev.of_node.
So the first patch should be to extend this behaviour. Look for a
child node called mdio. If it exists, call nixge_mdio_setup() passing
that child. Otherwise continue using pdev->dev.of_node, so you don't
break backwards compatibility.
Then a patch adding support for fixed-link. If the mdio child node
exists, you still need to register the MDIO bus. If there is no child
node, but there is a fixed-link, skip registering the mdio bus with
pdev->dev.of_node.
Andrew
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 <[email protected]>
> ---
>
> 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 <[email protected]> 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
Hi Andrew,
On Wed, Aug 29, 2018 at 8:04 PM, Andrew Lunn <[email protected]> wrote:
> On Wed, Aug 29, 2018 at 05:40:44PM -0700, Moritz Fischer wrote:
>> Add support for fixed-link cases where no MDIO is
>> actually required to run the device.
>> In that case no MDIO bus is instantiated since the
>> actual registers are not available in hardware.
>
> Hi Moritz
>
> There are a few different use cases here:
>
> The hardware is missing MDIO - You need fixed-link.
Agreed.
>
> The hardware has MDIO, but you don't have a PHY connected on it, and
> use fixed link.
Since it's an FPGA design in that case we'd probably build the hardware without
MDIO to save resources.
> The hardware has MDIO, and it is used e.g. for an Ethernet switch, or
> a PHY for another Ethernet interface. Plus you need fixed link.
We haven't had that yet but I can see that happen.
>
> The binding typically looks like:
>
> &fec1 {
> phy-mode = "rmii";
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_fec1>;
> status = "okay";
>
> fixed-link {
> speed = <100>;
> full-duplex;
> };
>
> mdio1: mdio {
> #address-cells = <1>;
> #size-cells = <0>;
> status = "okay";
>
> switch0: switch0@0 {
> compatible = "marvell,mv88e6085";
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_switch>;
> reg = <0>;
> eeprom-length = <512>;
> interrupt-parent = <&gpio3>;
>
> It is important you have the mdio subnode, with PHYs and switches as
> children. The driver currently gets this wrong, it uses
> pdev->dev.of_node.
Oh, whoops. Yeah I should look into that. Any good examples of drivers doing
it right? Is the one going with the DT snippet above a good example?
>
> So the first patch should be to extend this behaviour. Look for a
> child node called mdio. If it exists, call nixge_mdio_setup() passing
> that child. Otherwise continue using pdev->dev.of_node, so you don't
> break backwards compatibility.
Ok will do.
>
> Then a patch adding support for fixed-link. If the mdio child node
> exists, you still need to register the MDIO bus. If there is no child
> node, but there is a fixed-link, skip registering the mdio bus with
> pdev->dev.of_node.
>
> Andrew
Thanks for your feedback, much appreciated!
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 <[email protected]> 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
Hi Moritz,
I love your patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Moritz-Fischer/nixge-fixed-link-support/20180830-150857
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
Note: the linux-review/Moritz-Fischer/nixge-fixed-link-support/20180830-150857 HEAD 300a42d41dc76f270bff67d414dc7fc127d3f17c builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
drivers/net/ethernet/ni/nixge.c: In function 'nixge_mdio_setup':
drivers/net/ethernet/ni/nixge.c:1221:6: warning: unused variable 'err' [-Wunused-variable]
int err;
^~~
drivers/net/ethernet/ni/nixge.c: In function 'nixge_remove':
>> drivers/net/ethernet/ni/nixge.c:1366:6: error: 'np' undeclared (first use in this function); did you mean 'up'?
if (np && of_phy_is_fixed_link(np))
^~
up
drivers/net/ethernet/ni/nixge.c:1366:6: note: each undeclared identifier is reported only once for each function it appears in
vim +1366 drivers/net/ethernet/ni/nixge.c
1217
1218 static int nixge_mdio_setup(struct nixge_priv *priv, struct device_node *np)
1219 {
1220 struct mii_bus *bus;
> 1221 int err;
1222
1223 bus = devm_mdiobus_alloc(priv->dev);
1224 if (!bus)
1225 return -ENOMEM;
1226
1227 snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(priv->dev));
1228 bus->priv = priv;
1229 bus->name = "nixge_mii_bus";
1230 bus->read = nixge_mdio_read;
1231 bus->write = nixge_mdio_write;
1232 bus->parent = priv->dev;
1233
1234 priv->mii_bus = bus;
1235
1236 return of_mdiobus_register(bus, np);
1237 }
1238
1239 static void *nixge_get_nvmem_address(struct device *dev)
1240 {
1241 struct nvmem_cell *cell;
1242 size_t cell_size;
1243 char *mac;
1244
1245 cell = nvmem_cell_get(dev, "address");
1246 if (IS_ERR(cell))
1247 return NULL;
1248
1249 mac = nvmem_cell_read(cell, &cell_size);
1250 nvmem_cell_put(cell);
1251
1252 return mac;
1253 }
1254
1255 static int nixge_probe(struct platform_device *pdev)
1256 {
1257 struct nixge_priv *priv;
1258 struct net_device *ndev;
1259 struct resource *dmares;
1260 struct device_node *np;
1261 const u8 *mac_addr;
1262 int err;
1263
1264 ndev = alloc_etherdev(sizeof(*priv));
1265 if (!ndev)
1266 return -ENOMEM;
1267
1268 np = pdev->dev.of_node;
1269
1270 platform_set_drvdata(pdev, ndev);
1271 SET_NETDEV_DEV(ndev, &pdev->dev);
1272
1273 ndev->features = NETIF_F_SG;
1274 ndev->netdev_ops = &nixge_netdev_ops;
1275 ndev->ethtool_ops = &nixge_ethtool_ops;
1276
1277 /* MTU range: 64 - 9000 */
1278 ndev->min_mtu = 64;
1279 ndev->max_mtu = NIXGE_JUMBO_MTU;
1280
1281 mac_addr = nixge_get_nvmem_address(&pdev->dev);
1282 if (mac_addr && is_valid_ether_addr(mac_addr)) {
1283 ether_addr_copy(ndev->dev_addr, mac_addr);
1284 kfree(mac_addr);
1285 } else {
1286 eth_hw_addr_random(ndev);
1287 }
1288
1289 priv = netdev_priv(ndev);
1290 priv->ndev = ndev;
1291 priv->dev = &pdev->dev;
1292
1293 netif_napi_add(ndev, &priv->napi, nixge_poll, NAPI_POLL_WEIGHT);
1294
1295 dmares = platform_get_resource(pdev, IORESOURCE_MEM, 0);
1296 priv->dma_regs = devm_ioremap_resource(&pdev->dev, dmares);
1297 if (IS_ERR(priv->dma_regs)) {
1298 netdev_err(ndev, "failed to map dma regs\n");
1299 return PTR_ERR(priv->dma_regs);
1300 }
1301 priv->ctrl_regs = priv->dma_regs + NIXGE_REG_CTRL_OFFSET;
1302 __nixge_hw_set_mac_address(ndev);
1303
1304 priv->tx_irq = platform_get_irq_byname(pdev, "tx");
1305 if (priv->tx_irq < 0) {
1306 netdev_err(ndev, "could not find 'tx' irq");
1307 return priv->tx_irq;
1308 }
1309
1310 priv->rx_irq = platform_get_irq_byname(pdev, "rx");
1311 if (priv->rx_irq < 0) {
1312 netdev_err(ndev, "could not find 'rx' irq");
1313 return priv->rx_irq;
1314 }
1315
1316 priv->coalesce_count_rx = XAXIDMA_DFT_RX_THRESHOLD;
1317 priv->coalesce_count_tx = XAXIDMA_DFT_TX_THRESHOLD;
1318
1319 if (np) {
1320 err = nixge_of_get_phy(priv, np);
1321 if (err)
1322 goto free_netdev;
1323 }
1324
1325 /* only if it's not a fixed link, do we care about MDIO at all */
1326 if (priv->phy_node && !of_phy_is_fixed_link(np)) {
1327 err = nixge_mdio_setup(priv, np);
1328 if (err) {
1329 dev_err(&pdev->dev, "error registering mdio bus");
1330 goto free_phy;
1331 }
1332 }
1333
1334 err = register_netdev(priv->ndev);
1335 if (err) {
1336 netdev_err(ndev, "register_netdev() error (%i)\n", err);
1337 goto unregister_mdio;
1338 }
1339
1340 return 0;
1341
1342 unregister_mdio:
1343 if (priv->mii_bus)
1344 mdiobus_unregister(priv->mii_bus);
1345 free_phy:
1346 if (np && of_phy_is_fixed_link(np)) {
1347 of_phy_deregister_fixed_link(np);
1348 of_node_put(np);
1349 }
1350 free_netdev:
1351 free_netdev(ndev);
1352
1353 return err;
1354 }
1355
1356 static int nixge_remove(struct platform_device *pdev)
1357 {
1358 struct net_device *ndev = platform_get_drvdata(pdev);
1359 struct nixge_priv *priv = netdev_priv(ndev);
1360
1361 unregister_netdev(ndev);
1362
1363 if (priv->mii_bus)
1364 mdiobus_unregister(priv->mii_bus);
1365
> 1366 if (np && of_phy_is_fixed_link(np))
1367 of_phy_deregister_fixed_link(np);
1368
1369 free_netdev(ndev);
1370
1371 return 0;
1372 }
1373
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
> > The hardware has MDIO, but you don't have a PHY connected on it, and
> > use fixed link.
>
> Since it's an FPGA design in that case we'd probably build the hardware without
> MDIO to save resources.
You can save resources, but is it worth the complexity else where,
like in the software?
> > It is important you have the mdio subnode, with PHYs and switches as
> > children. The driver currently gets this wrong, it uses
> > pdev->dev.of_node.
>
> Oh, whoops.
Yes, and i also missed it. I generally review all new network drivers
and look at their MDIO and PHY code.
> Any good examples of drivers doing it right? Is the one going with
> the DT snippet above a good example?
That comes from the Freescale fec_main.c. It only supports DT, and
always uses of_mdiobus_register. You need to be a bit more flexible
for when you don't have DT. I'm not sure there are good example of
this, since they either don't need this flexibility, or they get it
wrong :-(
Andrew
Andrew,
On Thu, Aug 30, 2018 at 10:54 AM, Andrew Lunn <[email protected]> wrote:
>> > The hardware has MDIO, but you don't have a PHY connected on it, and
>> > use fixed link.
>>
>> Since it's an FPGA design in that case we'd probably build the hardware without
>> MDIO to save resources.
>
> You can save resources, but is it worth the complexity else where,
> like in the software?
Depends. For now I definitely have build versions that don't have MDIO regs
there. I might be able to chat with HW folks ...
>
>> > It is important you have the mdio subnode, with PHYs and switches as
>> > children. The driver currently gets this wrong, it uses
>> > pdev->dev.of_node.
>>
>> Oh, whoops.
>
> Yes, and i also missed it. I generally review all new network drivers
> and look at their MDIO and PHY code.
I had looked at macb as an example and there were a bunch of other cases
where there was no 'mdio' subnode.
>
>> Any good examples of drivers doing it right? Is the one going with
>> the DT snippet above a good example?
>
> That comes from the Freescale fec_main.c. It only supports DT, and
> always uses of_mdiobus_register. You need to be a bit more flexible
> for when you don't have DT. I'm not sure there are good example of
> this, since they either don't need this flexibility, or they get it
> wrong :-(
Alright, no problem. I'll take a stab at it. And come back with a v2 ;-)
Need to look at your response in the other patch.
Cheers,
Moritz