| Message ID | 20241024065328.521518-7-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add basic support for i.MX95 NETC | expand |
On Thu, Oct 24, 2024 at 02:53:21PM +0800, Wei Fang wrote: > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.h b/drivers/net/ethernet/freescale/enetc/enetc_pf.h > index 92a26b09cf57..39db9d5c2e50 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.h > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.h > @@ -28,6 +28,16 @@ struct enetc_vf_state { > enum enetc_vf_flags flags; > }; > > +struct enetc_pf; > + > +struct enetc_pf_ops { > + void (*set_si_primary_mac)(struct enetc_hw *hw, int si, const u8 *addr); > + void (*get_si_primary_mac)(struct enetc_hw *hw, int si, u8 *addr); > + struct phylink_pcs *(*create_pcs)(struct enetc_pf *pf, struct mii_bus *bus); > + void (*destroy_pcs)(struct phylink_pcs *pcs); > + int (*enable_psfp)(struct enetc_ndev_priv *priv); > +}; > + > struct enetc_pf { > struct enetc_si *si; > int num_vfs; /* number of active VFs, after sriov_init */ > @@ -50,6 +60,8 @@ struct enetc_pf { > > phy_interface_t if_mode; > struct phylink_config phylink_config; > + > + const struct enetc_pf_ops *ops; > }; > > #define phylink_to_enetc_pf(config) \ > @@ -59,9 +71,6 @@ int enetc_msg_psi_init(struct enetc_pf *pf); > void enetc_msg_psi_free(struct enetc_pf *pf); > void enetc_msg_handle_rxmsg(struct enetc_pf *pf, int mbox_id, u16 *status); > > -void enetc_pf_get_primary_mac_addr(struct enetc_hw *hw, int si, u8 *addr); > -void enetc_pf_set_primary_mac_addr(struct enetc_hw *hw, int si, > - const u8 *addr); > int enetc_pf_set_mac_addr(struct net_device *ndev, void *addr); > int enetc_setup_mac_addresses(struct device_node *np, struct enetc_pf *pf); > void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev, > @@ -71,3 +80,9 @@ void enetc_mdiobus_destroy(struct enetc_pf *pf); > int enetc_phylink_create(struct enetc_ndev_priv *priv, struct device_node *node, > const struct phylink_mac_ops *ops); > void enetc_phylink_destroy(struct enetc_ndev_priv *priv); > + > +static inline void enetc_pf_ops_register(struct enetc_pf *pf, > + const struct enetc_pf_ops *ops) > +{ > + pf->ops = ops; > +} I think this is more confusing than helpful? "register" suggests there should also be an "unregister" coming. Either "set" or just open-code the assignment? > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c > index bce81a4f6f88..94690ed92e3f 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c > @@ -8,19 +8,37 @@ > > #include "enetc_pf.h" > > +static int enetc_set_si_hw_addr(struct enetc_pf *pf, int si, u8 *mac_addr) > +{ > + struct enetc_hw *hw = &pf->si->hw; > + > + if (pf->ops->set_si_primary_mac) > + pf->ops->set_si_primary_mac(hw, si, mac_addr); > + else > + return -EOPNOTSUPP; > + > + return 0; Don't artificially create errors when there are really no errors to handle. Both enetc_pf_ops and enetc4_pf_ops provide .set_si_primary_mac(), so it is unnecessary to handle the case where it isn't present. Those functions return void, and void can be propagated to enetc_set_si_hw_addr() as well. > +} > + > int enetc_pf_set_mac_addr(struct net_device *ndev, void *addr) > { > struct enetc_ndev_priv *priv = netdev_priv(ndev); > + struct enetc_pf *pf = enetc_si_priv(priv->si); > struct sockaddr *saddr = addr; > + int err; > > if (!is_valid_ether_addr(saddr->sa_data)) > return -EADDRNOTAVAIL; > > + err = enetc_set_si_hw_addr(pf, 0, saddr->sa_data); > + if (err) > + return err; > + > eth_hw_addr_set(ndev, saddr->sa_data); > - enetc_pf_set_primary_mac_addr(&priv->si->hw, 0, saddr->sa_data); This isn't one for one replacement, it also moves the function call relative to eth_hw_addr_set() without making any mention about that movement being safe. And even if safe, it is logically a separate change which deserves its own merit analysis in another patch (if there's no merit, leave it where it is). I believe the merit was: enetc_set_si_hw_addr() can return error, thus we simplify the control flow if we call it prior to eth_hw_addr_set() - nothing to unroll. But as explained above, enetc_set_si_hw_addr() cannot actually fail, so there is no real merit. > > return 0; > } > +EXPORT_SYMBOL_GPL(enetc_pf_set_mac_addr); > > static int enetc_setup_mac_address(struct device_node *np, struct enetc_pf *pf, > int si) > @@ -38,8 +56,8 @@ static int enetc_setup_mac_address(struct device_node *np, struct enetc_pf *pf, > } > > /* (2) bootloader supplied MAC address */ > - if (is_zero_ether_addr(mac_addr)) > - enetc_pf_get_primary_mac_addr(hw, si, mac_addr); > + if (is_zero_ether_addr(mac_addr) && pf->ops->get_si_primary_mac) > + pf->ops->get_si_primary_mac(hw, si, mac_addr); Again, if there's no reason for the method to be optional (both PF drivers support it), don't make it optional. A bit inconsistent that pf->ops->set_si_primary_mac() goes through a wrapper function but this doesn't. > > /* (3) choose a random one */ > if (is_zero_ether_addr(mac_addr)) { > @@ -48,7 +66,9 @@ static int enetc_setup_mac_address(struct device_node *np, struct enetc_pf *pf, > si, mac_addr); > } > > - enetc_pf_set_primary_mac_addr(hw, si, mac_addr); > + err = enetc_set_si_hw_addr(pf, si, mac_addr); > + if (err) > + return err; This should go back to normal (no "err = ...; if (err) return err") once the function is made void. > > return 0; > } > @@ -107,7 +129,8 @@ void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev, > NETDEV_XDP_ACT_NDO_XMIT | NETDEV_XDP_ACT_RX_SG | > NETDEV_XDP_ACT_NDO_XMIT_SG; > > - if (si->hw_features & ENETC_SI_F_PSFP && !enetc_psfp_enable(priv)) { > + if (si->hw_features & ENETC_SI_F_PSFP && pf->ops->enable_psfp && > + !pf->ops->enable_psfp(priv)) { This one looks extremely weird in the existing code, but I suppose I'm too late to the party to request you to clean up any of the PSFP code, so I'll make a note to do it myself after your work. I haven't spotted any actual bug, just weird coding patterns. No change request here. I see the netc4_pf doesn't implement enable_psfp(), so making it optional here is fine. > priv->active_offloads |= ENETC_F_QCI; > ndev->features |= NETIF_F_HW_TC; > ndev->hw_features |= NETIF_F_HW_TC; > @@ -116,6 +139,7 @@ void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev, > /* pick up primary MAC address from SI */ > enetc_load_primary_mac_addr(&si->hw, ndev); > } > +EXPORT_SYMBOL_GPL(enetc_pf_netdev_setup); > > static int enetc_mdio_probe(struct enetc_pf *pf, struct device_node *np) > { > @@ -162,6 +186,9 @@ static int enetc_imdio_create(struct enetc_pf *pf) > struct mii_bus *bus; > int err; > > + if (!pf->ops->create_pcs) > + return -EOPNOTSUPP; > + I don't understand how this works. You don't implement create_pcs() for netc4_pf until the very end of the series. Probing will fail for SerDes interfaces (enetc_port_has_pcs() returns true) and that's fine? A message maybe, stating what's the deal? Just that users figure out quickly that it's an expected behavior, and not spend hours debugging until they find out it's not their fault? > bus = mdiobus_alloc_size(sizeof(*mdio_priv)); > if (!bus) > return -ENOMEM;
> > +struct enetc_pf; > > + > > +struct enetc_pf_ops { > > + void (*set_si_primary_mac)(struct enetc_hw *hw, int si, const u8 *addr); > > + void (*get_si_primary_mac)(struct enetc_hw *hw, int si, u8 *addr); > > + struct phylink_pcs *(*create_pcs)(struct enetc_pf *pf, struct mii_bus *bus); > > + void (*destroy_pcs)(struct phylink_pcs *pcs); > > + int (*enable_psfp)(struct enetc_ndev_priv *priv); > > +}; > > + > > struct enetc_pf { > > struct enetc_si *si; > > int num_vfs; /* number of active VFs, after sriov_init */ > > @@ -50,6 +60,8 @@ struct enetc_pf { > > > > phy_interface_t if_mode; > > struct phylink_config phylink_config; > > + > > + const struct enetc_pf_ops *ops; > > }; > > > > #define phylink_to_enetc_pf(config) \ > > @@ -59,9 +71,6 @@ int enetc_msg_psi_init(struct enetc_pf *pf); > > void enetc_msg_psi_free(struct enetc_pf *pf); > > void enetc_msg_handle_rxmsg(struct enetc_pf *pf, int mbox_id, u16 > *status); > > > > -void enetc_pf_get_primary_mac_addr(struct enetc_hw *hw, int si, u8 *addr); > > -void enetc_pf_set_primary_mac_addr(struct enetc_hw *hw, int si, > > - const u8 *addr); > > int enetc_pf_set_mac_addr(struct net_device *ndev, void *addr); > > int enetc_setup_mac_addresses(struct device_node *np, struct enetc_pf > *pf); > > void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev, > > @@ -71,3 +80,9 @@ void enetc_mdiobus_destroy(struct enetc_pf *pf); > > int enetc_phylink_create(struct enetc_ndev_priv *priv, struct device_node > *node, > > const struct phylink_mac_ops *ops); > > void enetc_phylink_destroy(struct enetc_ndev_priv *priv); > > + > > +static inline void enetc_pf_ops_register(struct enetc_pf *pf, > > + const struct enetc_pf_ops *ops) > > +{ > > + pf->ops = ops; > > +} > > I think this is more confusing than helpful? "register" suggests there > should also be an "unregister" coming. Either "set" or just open-code > the assignment? Okay, I can remove this helper function, just use open-code. > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c > b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c > > index bce81a4f6f88..94690ed92e3f 100644 > > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c > > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c > > @@ -8,19 +8,37 @@ > > > > #include "enetc_pf.h" > > > > +static int enetc_set_si_hw_addr(struct enetc_pf *pf, int si, u8 *mac_addr) > > +{ > > + struct enetc_hw *hw = &pf->si->hw; > > + > > + if (pf->ops->set_si_primary_mac) > > + pf->ops->set_si_primary_mac(hw, si, mac_addr); > > + else > > + return -EOPNOTSUPP; > > + > > + return 0; > > Don't artificially create errors when there are really no errors to handle. > Both enetc_pf_ops and enetc4_pf_ops provide .set_si_primary_mac(), so it > is unnecessary to handle the case where it isn't present. Those functions > return void, and void can be propagated to enetc_set_si_hw_addr() as well. > I thought checking the pointer is safer, so you mean that pointers that are definitely present in the current driver do not need to be checked? > > +} > > + > > int enetc_pf_set_mac_addr(struct net_device *ndev, void *addr) > > { > > struct enetc_ndev_priv *priv = netdev_priv(ndev); > > + struct enetc_pf *pf = enetc_si_priv(priv->si); > > struct sockaddr *saddr = addr; > > + int err; > > > > if (!is_valid_ether_addr(saddr->sa_data)) > > return -EADDRNOTAVAIL; > > > > + err = enetc_set_si_hw_addr(pf, 0, saddr->sa_data); > > + if (err) > > + return err; > > + > > eth_hw_addr_set(ndev, saddr->sa_data); > > - enetc_pf_set_primary_mac_addr(&priv->si->hw, 0, saddr->sa_data); > > This isn't one for one replacement, it also moves the function call > relative to eth_hw_addr_set() without making any mention about that > movement being safe. And even if safe, it is logically a separate change > which deserves its own merit analysis in another patch (if there's no > merit, leave it where it is). > > I believe the merit was: enetc_set_si_hw_addr() can return error, thus > we simplify the control flow if we call it prior to eth_hw_addr_set() - > nothing to unroll. But as explained above, enetc_set_si_hw_addr() cannot > actually fail, so there is no real merit. > > > > > return 0; > > } > > +EXPORT_SYMBOL_GPL(enetc_pf_set_mac_addr); > > > > static int enetc_setup_mac_address(struct device_node *np, struct > enetc_pf *pf, > > int si) > > @@ -38,8 +56,8 @@ static int enetc_setup_mac_address(struct device_node > *np, struct enetc_pf *pf, > > } > > > > /* (2) bootloader supplied MAC address */ > > - if (is_zero_ether_addr(mac_addr)) > > - enetc_pf_get_primary_mac_addr(hw, si, mac_addr); > > + if (is_zero_ether_addr(mac_addr) && pf->ops->get_si_primary_mac) > > + pf->ops->get_si_primary_mac(hw, si, mac_addr); > > Again, if there's no reason for the method to be optional (both PF > drivers support it), don't make it optional. > > A bit inconsistent that pf->ops->set_si_primary_mac() goes through a > wrapper function but this doesn't. > If we really do not need to check these callback pointers, then I think I can remove the wrapper. > > > > /* (3) choose a random one */ > > if (is_zero_ether_addr(mac_addr)) { > > @@ -48,7 +66,9 @@ static int enetc_setup_mac_address(struct device_node > *np, struct enetc_pf *pf, > > si, mac_addr); > > } > > > > - enetc_pf_set_primary_mac_addr(hw, si, mac_addr); > > + err = enetc_set_si_hw_addr(pf, si, mac_addr); > > + if (err) > > + return err; > > This should go back to normal (no "err = ...; if (err) return err") once > the function is made void. > > > > > return 0; > > } > > @@ -107,7 +129,8 @@ void enetc_pf_netdev_setup(struct enetc_si *si, > struct net_device *ndev, > > NETDEV_XDP_ACT_NDO_XMIT | > NETDEV_XDP_ACT_RX_SG | > > NETDEV_XDP_ACT_NDO_XMIT_SG; > > > > - if (si->hw_features & ENETC_SI_F_PSFP && !enetc_psfp_enable(priv)) { > > + if (si->hw_features & ENETC_SI_F_PSFP && pf->ops->enable_psfp && > > + !pf->ops->enable_psfp(priv)) { > > This one looks extremely weird in the existing code, but I suppose I'm > too late to the party to request you to clean up any of the PSFP code, > so I'll make a note to do it myself after your work. I haven't spotted > any actual bug, just weird coding patterns. > > No change request here. I see the netc4_pf doesn't implement enable_psfp(), > so making it optional here is fine. Yes, PSFP is not supported in this patch set, I will remove it in future. > > > priv->active_offloads |= ENETC_F_QCI; > > ndev->features |= NETIF_F_HW_TC; > > ndev->hw_features |= NETIF_F_HW_TC; > > @@ -116,6 +139,7 @@ void enetc_pf_netdev_setup(struct enetc_si *si, > struct net_device *ndev, > > /* pick up primary MAC address from SI */ > > enetc_load_primary_mac_addr(&si->hw, ndev); > > } > > +EXPORT_SYMBOL_GPL(enetc_pf_netdev_setup); > > > > static int enetc_mdio_probe(struct enetc_pf *pf, struct device_node *np) > > { > > @@ -162,6 +186,9 @@ static int enetc_imdio_create(struct enetc_pf *pf) > > struct mii_bus *bus; > > int err; > > > > + if (!pf->ops->create_pcs) > > + return -EOPNOTSUPP; > > + > > I don't understand how this works. You don't implement create_pcs() for > netc4_pf until the very end of the series. Probing will fail for SerDes > interfaces (enetc_port_has_pcs() returns true) and that's fine? > Currently, we have not add the PCS support, so the 10G ENETC is not supported yet. And we also disable the 10G ENETC in DTS. Only the 1G ENETCs (without PCS) are supported for i.MX95. > A message maybe, stating what's the deal? Just that users figure out > quickly that it's an expected behavior, and not spend hours debugging > until they find out it's not their fault? I will explain in the commit message that i.MX95 10G ENETC is not currently supported. > > > bus = mdiobus_alloc_size(sizeof(*mdio_priv)); > > if (!bus) > > return -ENOMEM;
On Fri, Oct 25, 2024 at 06:00:42AM +0300, Wei Fang wrote: > > Don't artificially create errors when there are really no errors to handle. > > Both enetc_pf_ops and enetc4_pf_ops provide .set_si_primary_mac(), so it > > is unnecessary to handle the case where it isn't present. Those functions > > return void, and void can be propagated to enetc_set_si_hw_addr() as well. > > I thought checking the pointer is safer, so you mean that pointers that are > definitely present in the current driver do not need to be checked? Yes, there is no point to check for a condition which is impossible to trigger in the current code. The callee and the caller are tightly coupled (in the same driver), it's not a rigid API, so if the function pointers should be made optional for some future hardware IP, the error handling will be added when necessary. Ideally any change passes through review, and any inconsistency should be spotted when added. > > A bit inconsistent that pf->ops->set_si_primary_mac() goes through a > > wrapper function but this doesn't. > > > > If we really do not need to check these callback pointers, then I think I can > remove the wrapper. Fine without wrapping throughout, my comment was first and foremost about consistency. > > This one looks extremely weird in the existing code, but I suppose I'm > > too late to the party to request you to clean up any of the PSFP code, > > so I'll make a note to do it myself after your work. I haven't spotted > > any actual bug, just weird coding patterns. > > > > No change request here. I see the netc4_pf doesn't implement enable_psfp(), > > so making it optional here is fine. > > Yes, PSFP is not supported in this patch set, I will remove it in future. If by "I will remove it in future" you mean "once NETC4 gains PSFP support, I will make enable_psfp() non-optional", then ok, great. > Currently, we have not add the PCS support, so the 10G ENETC is not supported > yet. And we also disable the 10G ENETC in DTS. Only the 1G ENETCs (without PCS) > are supported for i.MX95. Also think about the case where the current version of the kernel will boot on a newer version of the device tree, which does not have 'status = "disabled";' for those ports. It should do something reasonable. In any case, "they're now disabled in the device tree" is not an argument. My anecdotal and vague understanding of the Arm SystemReady (IR I think) requirements is that the device tree is provided by the platform, separately from the kernel/rootfs. It relies on the device tree ABI being stable, backwards-compatible and forwards-compatible. > > A message maybe, stating what's the deal? Just that users figure out > > quickly that it's an expected behavior, and not spend hours debugging > > until they find out it's not their fault? > > I will explain in the commit message that i.MX95 10G ENETC is not currently > supported. By "a message, maybe" I actually meant dev_err("Message here\n"); rather than silent/imprecise failure.
> On Fri, Oct 25, 2024 at 06:00:42AM +0300, Wei Fang wrote: > > > Don't artificially create errors when there are really no errors to handle. > > > Both enetc_pf_ops and enetc4_pf_ops provide .set_si_primary_mac(), so it > > > is unnecessary to handle the case where it isn't present. Those functions > > > return void, and void can be propagated to enetc_set_si_hw_addr() as well. > > > > I thought checking the pointer is safer, so you mean that pointers that are > > definitely present in the current driver do not need to be checked? > > Yes, there is no point to check for a condition which is impossible > to trigger in the current code. The callee and the caller are tightly > coupled (in the same driver), it's not a rigid API, so if the function > pointers should be made optional for some future hardware IP, the error > handling will be added when necessary. Ideally any change passes through > review, and any inconsistency should be spotted when added. > > > > A bit inconsistent that pf->ops->set_si_primary_mac() goes through a > > > wrapper function but this doesn't. > > > > > > > If we really do not need to check these callback pointers, then I think I can > > remove the wrapper. > > Fine without wrapping throughout, my comment was first and foremost > about consistency. > > > > This one looks extremely weird in the existing code, but I suppose I'm > > > too late to the party to request you to clean up any of the PSFP code, > > > so I'll make a note to do it myself after your work. I haven't spotted > > > any actual bug, just weird coding patterns. > > > > > > No change request here. I see the netc4_pf doesn't implement > enable_psfp(), > > > so making it optional here is fine. > > > > Yes, PSFP is not supported in this patch set, I will remove it in future. > > If by "I will remove it in future" you mean "once NETC4 gains PSFP > support, I will make enable_psfp() non-optional", then ok, great. > > > Currently, we have not add the PCS support, so the 10G ENETC is not > supported > > yet. And we also disable the 10G ENETC in DTS. Only the 1G ENETCs (without > PCS) > > are supported for i.MX95. > > Also think about the case where the current version of the kernel > will boot on a newer version of the device tree, which does not have > 'status = "disabled";' for those ports. It should do something reasonable. > In any case, "they're now disabled in the device tree" is not an argument. > For this case where the kernel is lower but the device tree is newer, I think there is no problem. It just fails in probe() and does not affect other functions. The old kernel does not support PCS, it is reasonable to return a '- EOPNOTSUPP' error. > My anecdotal and vague understanding of the Arm SystemReady (IR I think) > requirements is that the device tree is provided by the platform, > separately from the kernel/rootfs. It relies on the device tree ABI > being stable, backwards-compatible and forwards-compatible. > > > > A message maybe, stating what's the deal? Just that users figure out > > > quickly that it's an expected behavior, and not spend hours debugging > > > until they find out it's not their fault? > > > > I will explain in the commit message that i.MX95 10G ENETC is not currently > > supported. > > By "a message, maybe" I actually meant dev_err("Message here\n"); rather > than silent/imprecise failure. Okay, I'll add an explicit error message.
diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig b/drivers/net/ethernet/freescale/enetc/Kconfig index 51d80ea959d4..e1b151a98b41 100644 --- a/drivers/net/ethernet/freescale/enetc/Kconfig +++ b/drivers/net/ethernet/freescale/enetc/Kconfig @@ -7,6 +7,14 @@ config FSL_ENETC_CORE If compiled as module (M), the module name is fsl-enetc-core. +config NXP_ENETC_PF_COMMON + tristate + help + This module supports common functionality between drivers of + different versions of NXP ENETC PF controllers. + + If compiled as module (M), the module name is nxp-enetc-pf-common. + config FSL_ENETC tristate "ENETC PF driver" depends on PCI_MSI @@ -14,6 +22,7 @@ config FSL_ENETC select FSL_ENETC_CORE select FSL_ENETC_IERB select FSL_ENETC_MDIO + select NXP_ENETC_PF_COMMON select PHYLINK select PCS_LYNX select DIMLIB diff --git a/drivers/net/ethernet/freescale/enetc/Makefile b/drivers/net/ethernet/freescale/enetc/Makefile index 8f4d8e9c37a0..ebe232673ed4 100644 --- a/drivers/net/ethernet/freescale/enetc/Makefile +++ b/drivers/net/ethernet/freescale/enetc/Makefile @@ -3,8 +3,11 @@ obj-$(CONFIG_FSL_ENETC_CORE) += fsl-enetc-core.o fsl-enetc-core-y := enetc.o enetc_cbdr.o enetc_ethtool.o +obj-$(CONFIG_NXP_ENETC_PF_COMMON) += nxp-enetc-pf-common.o +nxp-enetc-pf-common-y := enetc_pf_common.o + obj-$(CONFIG_FSL_ENETC) += fsl-enetc.o -fsl-enetc-y := enetc_pf.o enetc_pf_common.o +fsl-enetc-y := enetc_pf.o fsl-enetc-$(CONFIG_PCI_IOV) += enetc_msg.o fsl-enetc-$(CONFIG_FSL_ENETC_QOS) += enetc_qos.o diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c index 3cdd149056f9..7522316ddfea 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c @@ -11,7 +11,7 @@ #define ENETC_DRV_NAME_STR "ENETC PF driver" -void enetc_pf_get_primary_mac_addr(struct enetc_hw *hw, int si, u8 *addr) +static void enetc_pf_get_primary_mac_addr(struct enetc_hw *hw, int si, u8 *addr) { u32 upper = __raw_readl(hw->port + ENETC_PSIPMAR0(si)); u16 lower = __raw_readw(hw->port + ENETC_PSIPMAR1(si)); @@ -20,8 +20,8 @@ void enetc_pf_get_primary_mac_addr(struct enetc_hw *hw, int si, u8 *addr) put_unaligned_le16(lower, addr + 4); } -void enetc_pf_set_primary_mac_addr(struct enetc_hw *hw, int si, - const u8 *addr) +static void enetc_pf_set_primary_mac_addr(struct enetc_hw *hw, int si, + const u8 *addr) { u32 upper = get_unaligned_le32(addr); u16 lower = get_unaligned_le16(addr + 4); @@ -30,6 +30,17 @@ void enetc_pf_set_primary_mac_addr(struct enetc_hw *hw, int si, __raw_writew(lower, hw->port + ENETC_PSIPMAR1(si)); } +static struct phylink_pcs *enetc_pf_create_pcs(struct enetc_pf *pf, + struct mii_bus *bus) +{ + return lynx_pcs_create_mdiodev(bus, 0); +} + +static void enetc_pf_destroy_pcs(struct phylink_pcs *pcs) +{ + lynx_pcs_destroy(pcs); +} + static void enetc_set_vlan_promisc(struct enetc_hw *hw, char si_map) { u32 val = enetc_port_rd(hw, ENETC_PSIPVMR); @@ -970,6 +981,14 @@ static void enetc_psi_destroy(struct pci_dev *pdev) enetc_pci_remove(pdev); } +static const struct enetc_pf_ops enetc_pf_ops = { + .set_si_primary_mac = enetc_pf_set_primary_mac_addr, + .get_si_primary_mac = enetc_pf_get_primary_mac_addr, + .create_pcs = enetc_pf_create_pcs, + .destroy_pcs = enetc_pf_destroy_pcs, + .enable_psfp = enetc_psfp_enable, +}; + static int enetc_pf_probe(struct pci_dev *pdev, const struct pci_device_id *ent) { @@ -997,6 +1016,7 @@ static int enetc_pf_probe(struct pci_dev *pdev, pf = enetc_si_priv(si); pf->si = si; pf->total_vfs = pci_sriov_get_totalvfs(pdev); + enetc_pf_ops_register(pf, &enetc_pf_ops); err = enetc_setup_mac_addresses(node, pf); if (err) diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.h b/drivers/net/ethernet/freescale/enetc/enetc_pf.h index 92a26b09cf57..39db9d5c2e50 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.h +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.h @@ -28,6 +28,16 @@ struct enetc_vf_state { enum enetc_vf_flags flags; }; +struct enetc_pf; + +struct enetc_pf_ops { + void (*set_si_primary_mac)(struct enetc_hw *hw, int si, const u8 *addr); + void (*get_si_primary_mac)(struct enetc_hw *hw, int si, u8 *addr); + struct phylink_pcs *(*create_pcs)(struct enetc_pf *pf, struct mii_bus *bus); + void (*destroy_pcs)(struct phylink_pcs *pcs); + int (*enable_psfp)(struct enetc_ndev_priv *priv); +}; + struct enetc_pf { struct enetc_si *si; int num_vfs; /* number of active VFs, after sriov_init */ @@ -50,6 +60,8 @@ struct enetc_pf { phy_interface_t if_mode; struct phylink_config phylink_config; + + const struct enetc_pf_ops *ops; }; #define phylink_to_enetc_pf(config) \ @@ -59,9 +71,6 @@ int enetc_msg_psi_init(struct enetc_pf *pf); void enetc_msg_psi_free(struct enetc_pf *pf); void enetc_msg_handle_rxmsg(struct enetc_pf *pf, int mbox_id, u16 *status); -void enetc_pf_get_primary_mac_addr(struct enetc_hw *hw, int si, u8 *addr); -void enetc_pf_set_primary_mac_addr(struct enetc_hw *hw, int si, - const u8 *addr); int enetc_pf_set_mac_addr(struct net_device *ndev, void *addr); int enetc_setup_mac_addresses(struct device_node *np, struct enetc_pf *pf); void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev, @@ -71,3 +80,9 @@ void enetc_mdiobus_destroy(struct enetc_pf *pf); int enetc_phylink_create(struct enetc_ndev_priv *priv, struct device_node *node, const struct phylink_mac_ops *ops); void enetc_phylink_destroy(struct enetc_ndev_priv *priv); + +static inline void enetc_pf_ops_register(struct enetc_pf *pf, + const struct enetc_pf_ops *ops) +{ + pf->ops = ops; +} diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c index bce81a4f6f88..94690ed92e3f 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c @@ -8,19 +8,37 @@ #include "enetc_pf.h" +static int enetc_set_si_hw_addr(struct enetc_pf *pf, int si, u8 *mac_addr) +{ + struct enetc_hw *hw = &pf->si->hw; + + if (pf->ops->set_si_primary_mac) + pf->ops->set_si_primary_mac(hw, si, mac_addr); + else + return -EOPNOTSUPP; + + return 0; +} + int enetc_pf_set_mac_addr(struct net_device *ndev, void *addr) { struct enetc_ndev_priv *priv = netdev_priv(ndev); + struct enetc_pf *pf = enetc_si_priv(priv->si); struct sockaddr *saddr = addr; + int err; if (!is_valid_ether_addr(saddr->sa_data)) return -EADDRNOTAVAIL; + err = enetc_set_si_hw_addr(pf, 0, saddr->sa_data); + if (err) + return err; + eth_hw_addr_set(ndev, saddr->sa_data); - enetc_pf_set_primary_mac_addr(&priv->si->hw, 0, saddr->sa_data); return 0; } +EXPORT_SYMBOL_GPL(enetc_pf_set_mac_addr); static int enetc_setup_mac_address(struct device_node *np, struct enetc_pf *pf, int si) @@ -38,8 +56,8 @@ static int enetc_setup_mac_address(struct device_node *np, struct enetc_pf *pf, } /* (2) bootloader supplied MAC address */ - if (is_zero_ether_addr(mac_addr)) - enetc_pf_get_primary_mac_addr(hw, si, mac_addr); + if (is_zero_ether_addr(mac_addr) && pf->ops->get_si_primary_mac) + pf->ops->get_si_primary_mac(hw, si, mac_addr); /* (3) choose a random one */ if (is_zero_ether_addr(mac_addr)) { @@ -48,7 +66,9 @@ static int enetc_setup_mac_address(struct device_node *np, struct enetc_pf *pf, si, mac_addr); } - enetc_pf_set_primary_mac_addr(hw, si, mac_addr); + err = enetc_set_si_hw_addr(pf, si, mac_addr); + if (err) + return err; return 0; } @@ -70,11 +90,13 @@ int enetc_setup_mac_addresses(struct device_node *np, struct enetc_pf *pf) return 0; } +EXPORT_SYMBOL_GPL(enetc_setup_mac_addresses); void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev, const struct net_device_ops *ndev_ops) { struct enetc_ndev_priv *priv = netdev_priv(ndev); + struct enetc_pf *pf = enetc_si_priv(si); SET_NETDEV_DEV(ndev, &si->pdev->dev); priv->ndev = ndev; @@ -107,7 +129,8 @@ void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev, NETDEV_XDP_ACT_NDO_XMIT | NETDEV_XDP_ACT_RX_SG | NETDEV_XDP_ACT_NDO_XMIT_SG; - if (si->hw_features & ENETC_SI_F_PSFP && !enetc_psfp_enable(priv)) { + if (si->hw_features & ENETC_SI_F_PSFP && pf->ops->enable_psfp && + !pf->ops->enable_psfp(priv)) { priv->active_offloads |= ENETC_F_QCI; ndev->features |= NETIF_F_HW_TC; ndev->hw_features |= NETIF_F_HW_TC; @@ -116,6 +139,7 @@ void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev, /* pick up primary MAC address from SI */ enetc_load_primary_mac_addr(&si->hw, ndev); } +EXPORT_SYMBOL_GPL(enetc_pf_netdev_setup); static int enetc_mdio_probe(struct enetc_pf *pf, struct device_node *np) { @@ -162,6 +186,9 @@ static int enetc_imdio_create(struct enetc_pf *pf) struct mii_bus *bus; int err; + if (!pf->ops->create_pcs) + return -EOPNOTSUPP; + bus = mdiobus_alloc_size(sizeof(*mdio_priv)); if (!bus) return -ENOMEM; @@ -184,7 +211,7 @@ static int enetc_imdio_create(struct enetc_pf *pf) goto free_mdio_bus; } - phylink_pcs = lynx_pcs_create_mdiodev(bus, 0); + phylink_pcs = pf->ops->create_pcs(pf, bus); if (IS_ERR(phylink_pcs)) { err = PTR_ERR(phylink_pcs); dev_err(dev, "cannot create lynx pcs (%d)\n", err); @@ -205,8 +232,8 @@ static int enetc_imdio_create(struct enetc_pf *pf) static void enetc_imdio_remove(struct enetc_pf *pf) { - if (pf->pcs) - lynx_pcs_destroy(pf->pcs); + if (pf->pcs && pf->ops->destroy_pcs) + pf->ops->destroy_pcs(pf->pcs); if (pf->imdio) { mdiobus_unregister(pf->imdio); @@ -246,12 +273,14 @@ int enetc_mdiobus_create(struct enetc_pf *pf, struct device_node *node) return 0; } +EXPORT_SYMBOL_GPL(enetc_mdiobus_create); void enetc_mdiobus_destroy(struct enetc_pf *pf) { enetc_mdio_remove(pf); enetc_imdio_remove(pf); } +EXPORT_SYMBOL_GPL(enetc_mdiobus_destroy); int enetc_phylink_create(struct enetc_ndev_priv *priv, struct device_node *node, const struct phylink_mac_ops *ops) @@ -288,8 +317,13 @@ int enetc_phylink_create(struct enetc_ndev_priv *priv, struct device_node *node, return 0; } +EXPORT_SYMBOL_GPL(enetc_phylink_create); void enetc_phylink_destroy(struct enetc_ndev_priv *priv) { phylink_destroy(priv->phylink); } +EXPORT_SYMBOL_GPL(enetc_phylink_destroy); + +MODULE_DESCRIPTION("NXP ENETC PF common functionality driver"); +MODULE_LICENSE("Dual BSD/GPL");