| Message ID | 20241024065328.521518-1-wei.fang@nxp.com |
|---|---|
| Headers | show |
| Series | add basic support for i.MX95 NETC | expand |
On Thu, Oct 24, 2024 at 02:53:19PM +0800, Wei Fang wrote: > The netc-blk-ctrl driver is used to configure Integrated Endpoint > Register Block (IERB) and Privileged Register Block (PRB) of NETC. > For i.MX platforms, it is also used to configure the NETCMIX block. > > The IERB contains registers that are used for pre-boot initialization, > debug, and non-customer configuration. The PRB controls global reset > and global error handling for NETC. The NETCMIX block is mainly used > to set MII protocol and PCS protocol of the links, it also contains > settings for some other functions. > > Note the IERB configuration registers can only be written after being > unlocked by PRB, otherwise, all write operations are inhibited. A warm > reset is performed when the IERB is unlocked, and it results in an FLR > to all NETC devices. Therefore, all NETC device drivers must be probed > or initialized after the warm reset is finished. > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > Reviewed-by: Frank Li <Frank.Li@nxp.com> > --- Can U-Boot deal with the IERB/PRB configuration? For LS1028A, the platform which initiated the IERB driver "trend", the situation was a bit more complicated, as we realized the reset-time defaults aren't what we need very late in the product life cycle, when customer boards already had bootloaders and we didn't want to complicate their process to have to redeploy in order to get access to such a basic feature as flow control. Though if we knew it from day one, we would have put the IERB fixups in U-Boot. What is written in the IERB for MII/PCS protocols by default? I suppose there's some other mechanism to preinitialize it with good values?
On Thu, Oct 24, 2024 at 02:53:20PM +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 c26bd66e4597..92a26b09cf57 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.h > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.h > @@ -58,3 +58,16 @@ struct enetc_pf { > 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, > + const struct net_device_ops *ndev_ops); > +int enetc_mdiobus_create(struct enetc_pf *pf, struct device_node *node); > +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); Could you put the prototypes of functions exported by enetc_pf_common.c into a header named enetc_pf_common.h? It should be self-contained, i.e. a dummy C file with just #include "enetc_pf_common.h" in it should compile fine. I know the enetc driver isn't there yet when it comes to thoroughly respecting that, but for code we touch now, we should try to follow it.
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;
On Thu, Oct 24, 2024 at 02:53:22PM +0800, Wei Fang wrote: > @@ -62,6 +65,12 @@ static int enetc_pci_mdio_probe(struct pci_dev *pdev, > goto err_pci_mem_reg; > } > > + if (pdev->vendor == PCI_VENDOR_ID_FREESCALE && > + pdev->device == ENETC_MDIO_DEV_ID) { > + static_branch_inc(&enetc_has_err050089); > + dev_info(&pdev->dev, "Enabled ERR050089 workaround\n"); > + } > + > err = of_mdiobus_register(bus, dev->of_node); > if (err) > goto err_mdiobus_reg; If of_mdiobus_register() fails, we should disable the static key. Perhaps the snippets to enable and disable the static key should be in helper functions to more easily insert the error teardown logic.
On Thu, Oct 24, 2024 at 02:53:24PM +0800, Wei Fang wrote: > The verdor ID and device ID of i.MX95 EMDIO are different from LS1028A > EMDIO, so add new vendor ID and device ID to pci_device_id table to > support i.MX95 EMDIO. And the i.MX95 EMDIO has two pins that need to be > controlled, namely MDC and MDIO. > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > Reviewed-by: Frank Li <Frank.Li@nxp.com> > --- > v5: no changes > --- > drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c b/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c > index 2445e35a764a..9968a1e9b5ef 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c > @@ -2,6 +2,7 @@ > /* Copyright 2019 NXP */ > #include <linux/fsl/enetc_mdio.h> > #include <linux/of_mdio.h> > +#include <linux/pinctrl/consumer.h> > #include "enetc_pf.h" > > #define ENETC_MDIO_DEV_ID 0xee01 > @@ -71,6 +72,8 @@ static int enetc_pci_mdio_probe(struct pci_dev *pdev, > dev_info(&pdev->dev, "Enabled ERR050089 workaround\n"); > } > > + pinctrl_pm_select_default_state(dev); > + Not an expert on pinctrl by any means.. but is this needed? Documentation/driver-api/pin-control.rst says: | When a device driver is about to probe the device core will automatically | attempt to issue ``pinctrl_get_select_default()`` on these devices. | This way driver writers do not need to add any of the boilerplate code | of the type found below. The documentation is obsolete, because pinctrl_get_select_default() doesn't seem to be the current mechanism through which that happens. But there is a pinctrl_bind_pins() function in really_probe() which looks like it does that job. So.. is this needed? Also, pinctrl_pm_select_default_state() seems to be the suspend/resume (hence _pm_ name) variant of pinctrl_select_default_state(), but this is called from probe(). > err = of_mdiobus_register(bus, dev->of_node); > if (err) > goto err_mdiobus_reg; > @@ -113,6 +116,7 @@ static void enetc_pci_mdio_remove(struct pci_dev *pdev) > > static const struct pci_device_id enetc_pci_mdio_id_table[] = { > { PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, ENETC_MDIO_DEV_ID) }, > + { PCI_DEVICE(PCI_VENDOR_ID_NXP2, PCI_DEVICE_ID_NXP2_NETC_EMDIO) }, > { 0, } /* End of table. */ > }; > MODULE_DEVICE_TABLE(pci, enetc_pci_mdio_id_table); > -- > 2.34.1 >
On Thu, Oct 24, 2024 at 02:53:24PM +0800, Wei Fang wrote:
> The verdor ID and device ID of i.MX95 EMDIO are different from LS1028A
Also: s/verdor/vendor/
On Thu, Oct 24, 2024 at 02:53:25PM +0800, Wei Fang wrote: > From: Clark Wang <xiaoning.wang@nxp.com> > > Extract enetc_int_vector_init() and enetc_int_vector_destroy() from > enetc_alloc_msix() so that the code is more concise and readable. In > addition, slightly different from before, the cleanup helper function > is used to manage dynamically allocated memory resources. > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > Signed-off-by: Wei Fang <wei.fang@nxp.com> > Reviewed-by: Frank Li <Frank.Li@nxp.com> > Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com> > --- > v5: no changes > --- > drivers/net/ethernet/freescale/enetc/enetc.c | 172 ++++++++++--------- > 1 file changed, 87 insertions(+), 85 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index 032d8eadd003..bd725561b8a2 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > @@ -2965,6 +2965,87 @@ int enetc_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd) > } > EXPORT_SYMBOL_GPL(enetc_ioctl); > > +static int enetc_int_vector_init(struct enetc_ndev_priv *priv, int i, > + int v_tx_rings) > +{ > + struct enetc_int_vector *v __free(kfree); Please limit yourself to refactoring the code as-is into a separate function. This function does not benefit in any way from the use of __free() and no_free_ptr(). The established norm is that the normal teardown path should be identical to the error unwind path, minus the last step. Combining normal function calls with devres/scope-based cleanup/whatever other "look, you don't get to care about error handling!!!" APIs there may be makes that much more difficult to reason about. If the function is really simple and you really don't get to care about error handling by using __free(), then maybe its usage is tolerable, but that is hardly the general case. The more intricate the error handling becomes, the less useful __free() is, and the more it starts getting in the way of a human correctness reviewer. FWIW, Documentation/process/maintainer-netdev.rst, section "Using device-managed and cleanup.h constructs", appears to mostly state the same position as me here. Obviously, here, the established cleanup norm isn't followed anyway, but a patch which brings it in line would be appreciated. I think that a multi-patch approach, where the code is first moved and just moved, and then successively transformed in obviously correct and easy to review steps, would be best. Since you're quite close currently to the 15-patch limit, I would try to create a patch set just for preparing the enetc drivers, and introduce the i.mx95 support in a separate set which has mostly "+" lines in its diff. That way, there is also some time to not rush the ongoing IERB/PRB dt-binding discussion, while you can progress on pure driver refactoring. > + struct enetc_bdr *bdr; > + int j, err; > + > + v = kzalloc(struct_size(v, tx_ring, v_tx_rings), GFP_KERNEL); > + if (!v) > + return -ENOMEM; > + > + bdr = &v->rx_ring; > + bdr->index = i; > + bdr->ndev = priv->ndev; > + bdr->dev = priv->dev; > + bdr->bd_count = priv->rx_bd_count; > + bdr->buffer_offset = ENETC_RXB_PAD; > + priv->rx_ring[i] = bdr; > + > + err = xdp_rxq_info_reg(&bdr->xdp.rxq, priv->ndev, i, 0); > + if (err) > + return err; > + > + err = xdp_rxq_info_reg_mem_model(&bdr->xdp.rxq, > + MEM_TYPE_PAGE_SHARED, NULL); MEM_TYPE_PAGE_SHARED fits on the previous line. > + if (err) { > + xdp_rxq_info_unreg(&bdr->xdp.rxq); > + return err; > + } > + > + /* init defaults for adaptive IC */ > + if (priv->ic_mode & ENETC_IC_RX_ADAPTIVE) { > + v->rx_ictt = 0x1; > + v->rx_dim_en = true; > + } > + > + INIT_WORK(&v->rx_dim.work, enetc_rx_dim_work); > + netif_napi_add(priv->ndev, &v->napi, enetc_poll); > + v->count_tx_rings = v_tx_rings; > + > + for (j = 0; j < v_tx_rings; j++) { > + int idx; > + > + /* default tx ring mapping policy */ > + idx = priv->bdr_int_num * j + i; > + __set_bit(idx, &v->tx_rings_map); > + bdr = &v->tx_ring[j]; > + bdr->index = idx; > + bdr->ndev = priv->ndev; > + bdr->dev = priv->dev; > + bdr->bd_count = priv->tx_bd_count; > + priv->tx_ring[idx] = bdr; > + } > + > + priv->int_vector[i] = no_free_ptr(v); > + > + return 0; > +}
> On Thu, Oct 24, 2024 at 02:53:19PM +0800, Wei Fang wrote: > > The netc-blk-ctrl driver is used to configure Integrated Endpoint > > Register Block (IERB) and Privileged Register Block (PRB) of NETC. > > For i.MX platforms, it is also used to configure the NETCMIX block. > > > > The IERB contains registers that are used for pre-boot initialization, > > debug, and non-customer configuration. The PRB controls global reset > > and global error handling for NETC. The NETCMIX block is mainly used > > to set MII protocol and PCS protocol of the links, it also contains > > settings for some other functions. > > > > Note the IERB configuration registers can only be written after being > > unlocked by PRB, otherwise, all write operations are inhibited. A warm > > reset is performed when the IERB is unlocked, and it results in an FLR > > to all NETC devices. Therefore, all NETC device drivers must be probed > > or initialized after the warm reset is finished. > > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > --- > > Can U-Boot deal with the IERB/PRB configuration? > > For LS1028A, the platform which initiated the IERB driver "trend", the situation > was a bit more complicated, as we realized the reset-time defaults aren't what > we need very late in the product life cycle, when customer boards already had > bootloaders and we didn't want to complicate their process to have to redeploy > in order to get access to such a basic feature as flow control. Though if we knew > it from day one, we would have put the IERB fixups in U-Boot. The situation of i.MX95 is different from LS1028A, i.MX95 needs to support system suspend/resume feature. If the i.MX95 enters suspend mode, the NETC may power off (depends on user case), so IERB and PRB will be reset, in this case, we need to reconfigure the IERB & PRB, including NETCMIX. > > What is written in the IERB for MII/PCS protocols by default? I suppose there's > some other mechanism to preinitialize it with good values? The MII/PCS protocols are set in NETCMIX not IERB, but the IERB will get these info from NETCMIX, I mean the hardware, not the software. The default values are all 0.
> On Thu, Oct 24, 2024 at 02:53:20PM +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 c26bd66e4597..92a26b09cf57 100644 > > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.h > > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.h > > @@ -58,3 +58,16 @@ struct enetc_pf { > > 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, > > + const struct net_device_ops *ndev_ops); > > +int enetc_mdiobus_create(struct enetc_pf *pf, struct device_node *node); > > +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); > > Could you put the prototypes of functions exported by enetc_pf_common.c > into a header named enetc_pf_common.h? It should be self-contained, i.e. > a dummy C file with just #include "enetc_pf_common.h" in it should compile > fine. > Sure, I'll add a new header file. > I know the enetc driver isn't there yet when it comes to thoroughly > respecting that, but for code we touch now, we should try to follow it.
> > +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 Thu, Oct 24, 2024 at 02:53:22PM +0800, Wei Fang wrote: > > @@ -62,6 +65,12 @@ static int enetc_pci_mdio_probe(struct pci_dev *pdev, > > goto err_pci_mem_reg; > > } > > > > + if (pdev->vendor == PCI_VENDOR_ID_FREESCALE && > > + pdev->device == ENETC_MDIO_DEV_ID) { > > + static_branch_inc(&enetc_has_err050089); > > + dev_info(&pdev->dev, "Enabled ERR050089 workaround\n"); > > + } > > + > > err = of_mdiobus_register(bus, dev->of_node); > > if (err) > > goto err_mdiobus_reg; > > If of_mdiobus_register() fails, we should disable the static key. > > Perhaps the snippets to enable and disable the static key should be in > helper functions to more easily insert the error teardown logic. Okay, I will refine it
> > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > --- > > v5: no changes > > --- > > drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c > b/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c > > index 2445e35a764a..9968a1e9b5ef 100644 > > --- a/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c > > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pci_mdio.c > > @@ -2,6 +2,7 @@ > > /* Copyright 2019 NXP */ > > #include <linux/fsl/enetc_mdio.h> > > #include <linux/of_mdio.h> > > +#include <linux/pinctrl/consumer.h> > > #include "enetc_pf.h" > > > > #define ENETC_MDIO_DEV_ID 0xee01 > > @@ -71,6 +72,8 @@ static int enetc_pci_mdio_probe(struct pci_dev *pdev, > > dev_info(&pdev->dev, "Enabled ERR050089 workaround\n"); > > } > > > > + pinctrl_pm_select_default_state(dev); > > + > > Not an expert on pinctrl by any means.. but is this needed? > Documentation/driver-api/pin-control.rst says: > > | When a device driver is about to probe the device core will automatically > | attempt to issue ``pinctrl_get_select_default()`` on these devices. > | This way driver writers do not need to add any of the boilerplate code > | of the type found below. > > The documentation is obsolete, because pinctrl_get_select_default() > doesn't seem to be the current mechanism through which that happens. But > there is a pinctrl_bind_pins() function in really_probe() which looks > like it does that job. So.. is this needed? I think it is no need, I will remove it, thanks.
> On Thu, Oct 24, 2024 at 02:53:25PM +0800, Wei Fang wrote: > > From: Clark Wang <xiaoning.wang@nxp.com> > > > > Extract enetc_int_vector_init() and enetc_int_vector_destroy() from > > enetc_alloc_msix() so that the code is more concise and readable. In > > addition, slightly different from before, the cleanup helper function > > is used to manage dynamically allocated memory resources. > > > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com> > > --- > > v5: no changes > > --- > > drivers/net/ethernet/freescale/enetc/enetc.c | 172 ++++++++++--------- > > 1 file changed, 87 insertions(+), 85 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > b/drivers/net/ethernet/freescale/enetc/enetc.c > > index 032d8eadd003..bd725561b8a2 100644 > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > @@ -2965,6 +2965,87 @@ int enetc_ioctl(struct net_device *ndev, struct > ifreq *rq, int cmd) > > } > > EXPORT_SYMBOL_GPL(enetc_ioctl); > > > > +static int enetc_int_vector_init(struct enetc_ndev_priv *priv, int i, > > + int v_tx_rings) > > +{ > > + struct enetc_int_vector *v __free(kfree); > > Please limit yourself to refactoring the code as-is into a separate function. Okay > This function does not benefit in any way from the use of __free() and > no_free_ptr(). The established norm is that the normal teardown path > should be identical to the error unwind path, minus the last step. > Combining normal function calls with devres/scope-based cleanup/whatever > other "look, you don't get to care about error handling!!!" APIs there may be > makes that much more difficult to reason about. If the function is really > simple and you really don't get to care about error handling by using __free(), > then maybe its usage is tolerable, but that is hardly the general case. > The more intricate the error handling becomes, the less useful __free() is, > and the more it starts getting in the way of a human correctness reviewer. > > FWIW, Documentation/process/maintainer-netdev.rst, section "Using > device-managed and cleanup.h constructs", appears to mostly state the > same position as me here. > > Obviously, here, the established cleanup norm isn't followed anyway, but > a patch which brings it in line would be appreciated. I think that a > multi-patch approach, where the code is first moved and just moved, and > then successively transformed in obviously correct and easy to review > steps, would be best. > > Since you're quite close currently to the 15-patch limit, I would try to > create a patch set just for preparing the enetc drivers, and introduce > the i.mx95 support in a separate set which has mostly "+" lines in its diff. > That way, there is also some time to not rush the ongoing IERB/PRB > dt-binding discussion, while you can progress on pure driver refactoring. > Thanks for your suggestion.
On Thu, Oct 24, 2024 at 02:53:26PM +0800, Wei Fang wrote: > From: Clark Wang <xiaoning.wang@nxp.com> > > There is a situation where num_tx_rings cannot be divided by bdr_int_num. > For example, num_tx_rings is 8 and bdr_int_num is 3. According to the > previous logic, this results in two tx_bdr corresponding memories not > being allocated, so when sending packets to tx ring 6 or 7, wild pointers > will be accessed. Of course, this issue doesn't exist on LS1028A, because > its num_tx_rings is 8, and bdr_int_num is either 1 or 2. However, there > is a risk for the upcoming i.MX95. Therefore, it is necessary to ensure > that each tx_bdr can be allocated to the corresponding memory. > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > Signed-off-by: Wei Fang <wei.fang@nxp.com> > Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com> > Reviewed-by: Frank Li <Frank.Li@nxp.com> > --- > v5: no changes > --- > drivers/net/ethernet/freescale/enetc/enetc.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index bd725561b8a2..bccbeb1f355c 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > @@ -3049,10 +3049,10 @@ static void enetc_int_vector_destroy(struct enetc_ndev_priv *priv, int i) > int enetc_alloc_msix(struct enetc_ndev_priv *priv) > { > struct pci_dev *pdev = priv->si->pdev; > + int v_tx_rings, v_remainder; > int num_stack_tx_queues; > int first_xdp_tx_ring; > int i, n, err, nvec; > - int v_tx_rings; > > nvec = ENETC_BDR_INT_BASE_IDX + priv->bdr_int_num; > /* allocate MSIX for both messaging and Rx/Tx interrupts */ > @@ -3066,9 +3066,12 @@ int enetc_alloc_msix(struct enetc_ndev_priv *priv) > > /* # of tx rings per int vector */ > v_tx_rings = priv->num_tx_rings / priv->bdr_int_num; > + v_remainder = priv->num_tx_rings % priv->bdr_int_num; > > for (i = 0; i < priv->bdr_int_num; i++) { > - err = enetc_int_vector_init(priv, i, v_tx_rings); > + int num_tx_rings = i < v_remainder ? v_tx_rings + 1 : v_tx_rings; It took me a moment to understand the mechanism through which this works, even though I read the intention in the commit message. Do you think this additional comment would help? /* Distribute the remaining TX rings to the first * v_tx_rings interrupt vectors */ > + > + err = enetc_int_vector_init(priv, i, num_tx_rings); > if (err) > goto fail; > } > -- > 2.34.1 >
On Fri, Oct 25, 2024 at 04:44:50AM +0300, Wei Fang wrote: > > On Thu, Oct 24, 2024 at 02:53:19PM +0800, Wei Fang wrote: > > Can U-Boot deal with the IERB/PRB configuration? > > > > For LS1028A, the platform which initiated the IERB driver "trend", the situation > > was a bit more complicated, as we realized the reset-time defaults aren't what > > we need very late in the product life cycle, when customer boards already had > > bootloaders and we didn't want to complicate their process to have to redeploy > > in order to get access to such a basic feature as flow control. Though if we knew > > it from day one, we would have put the IERB fixups in U-Boot. > > The situation of i.MX95 is different from LS1028A, i.MX95 needs to support system > suspend/resume feature. If the i.MX95 enters suspend mode, the NETC may > power off (depends on user case), so IERB and PRB will be reset, in this case, we need > to reconfigure the IERB & PRB, including NETCMIX. > > > What is written in the IERB for MII/PCS protocols by default? I suppose there's > > some other mechanism to preinitialize it with good values? > > The MII/PCS protocols are set in NETCMIX not IERB, but the IERB will get these > info from NETCMIX, I mean the hardware, not the software. The default values > are all 0. I am shocked that the NETCMIX/IERB blocks does not have a separate power domain from the ENETC, to avoid powering them off, which loses the settings. Please provide this explanation in the opening comments of this driver, it is its entire "raison d'être".
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 Thu, Oct 24, 2024 at 02:53:26PM +0800, Wei Fang wrote: > > From: Clark Wang <xiaoning.wang@nxp.com> > > > > There is a situation where num_tx_rings cannot be divided by bdr_int_num. > > For example, num_tx_rings is 8 and bdr_int_num is 3. According to the > > previous logic, this results in two tx_bdr corresponding memories not > > being allocated, so when sending packets to tx ring 6 or 7, wild pointers > > will be accessed. Of course, this issue doesn't exist on LS1028A, because > > its num_tx_rings is 8, and bdr_int_num is either 1 or 2. However, there > > is a risk for the upcoming i.MX95. Therefore, it is necessary to ensure > > that each tx_bdr can be allocated to the corresponding memory. > > > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com> > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > --- > > v5: no changes > > --- > > drivers/net/ethernet/freescale/enetc/enetc.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > b/drivers/net/ethernet/freescale/enetc/enetc.c > > index bd725561b8a2..bccbeb1f355c 100644 > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > @@ -3049,10 +3049,10 @@ static void enetc_int_vector_destroy(struct > enetc_ndev_priv *priv, int i) > > int enetc_alloc_msix(struct enetc_ndev_priv *priv) > > { > > struct pci_dev *pdev = priv->si->pdev; > > + int v_tx_rings, v_remainder; > > int num_stack_tx_queues; > > int first_xdp_tx_ring; > > int i, n, err, nvec; > > - int v_tx_rings; > > > > nvec = ENETC_BDR_INT_BASE_IDX + priv->bdr_int_num; > > /* allocate MSIX for both messaging and Rx/Tx interrupts */ > > @@ -3066,9 +3066,12 @@ int enetc_alloc_msix(struct enetc_ndev_priv > *priv) > > > > /* # of tx rings per int vector */ > > v_tx_rings = priv->num_tx_rings / priv->bdr_int_num; > > + v_remainder = priv->num_tx_rings % priv->bdr_int_num; > > > > for (i = 0; i < priv->bdr_int_num; i++) { > > - err = enetc_int_vector_init(priv, i, v_tx_rings); > > + int num_tx_rings = i < v_remainder ? v_tx_rings + 1 : v_tx_rings; > > It took me a moment to understand the mechanism through which this > works, even though I read the intention in the commit message. > > Do you think this additional comment would help? Yeah, it does help users understand quickly. I will add this comment. > > /* Distribute the remaining TX rings to the first > * v_tx_rings interrupt vectors > */ > > > + > > + err = enetc_int_vector_init(priv, i, num_tx_rings); > > if (err) > > goto fail; > > } > > -- > > 2.34.1 > >
> -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Sent: 2024年10月25日 20:43 > To: Wei Fang <wei.fang@nxp.com> > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org; > conor+dt@kernel.org; Claudiu Manoil <claudiu.manoil@nxp.com>; Clark Wang > <xiaoning.wang@nxp.com>; Frank Li <frank.li@nxp.com>; > christophe.leroy@csgroup.eu; linux@armlinux.org.uk; bhelgaas@google.com; > horms@kernel.org; imx@lists.linux.dev; netdev@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-pci@vger.kernel.org; alexander.stein@ew.tq-group.com > Subject: Re: [PATCH v5 net-next 04/13] net: enetc: add initial netc-blk-ctrl > driver support > > On Fri, Oct 25, 2024 at 04:44:50AM +0300, Wei Fang wrote: > > > On Thu, Oct 24, 2024 at 02:53:19PM +0800, Wei Fang wrote: > > > Can U-Boot deal with the IERB/PRB configuration? > > > > > > For LS1028A, the platform which initiated the IERB driver "trend", the > situation > > > was a bit more complicated, as we realized the reset-time defaults aren't > what > > > we need very late in the product life cycle, when customer boards already > had > > > bootloaders and we didn't want to complicate their process to have to > redeploy > > > in order to get access to such a basic feature as flow control. Though if we > knew > > > it from day one, we would have put the IERB fixups in U-Boot. > > > > The situation of i.MX95 is different from LS1028A, i.MX95 needs to support > system > > suspend/resume feature. If the i.MX95 enters suspend mode, the NETC may > > power off (depends on user case), so IERB and PRB will be reset, in this case, > we need > > to reconfigure the IERB & PRB, including NETCMIX. > > > > > What is written in the IERB for MII/PCS protocols by default? I suppose > there's > > > some other mechanism to preinitialize it with good values? > > > > The MII/PCS protocols are set in NETCMIX not IERB, but the IERB will get > these > > info from NETCMIX, I mean the hardware, not the software. The default > values > > are all 0. > > I am shocked that the NETCMIX/IERB blocks does not have a separate power > domain from the ENETC, to avoid powering them off, which loses the settings. > Please provide this explanation in the opening comments of this driver, it > is its entire "raison d'être". Hmm, it's a good idea, I can add this annotation at the beginning of the driver. But this in not the entire "raison d'être", because we also hope to be able to dynamically configure based on DTS, which is more flexible than under uboot. For example, Timer binding for ENETC and switch if there are multiple Timer instances, port selection if two ENETCs or one ENETC and one switch port share the same physical port.
> 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.
This is first time that the NETC IP is applied on i.MX MPU platform. Its revision has been upgraded to 4.1, which is very different from the NETC of LS1028A (its revision is 1.0). Therefore, some existing drivers of NETC devices in the Linux kernel are not compatible with the current hardware. For example, the fsl-enetc driver is used to drive the ENETC PF of LS1028A, but for i.MX95 ENETC PF, its registers and tables configuration are very different from those of LS1028A, and only the station interface (SI) part remains basically the same. For the SI part, Vladimir has separated the fsl-enetc-core driver, so we can reuse this driver on i.MX95. However, for other parts of PF, the fsl-enetc driver cannot be reused, so the nxp-enetc4 driver is added to support revision 4.1 and later. During the development process, we found that the two PF drivers have some interfaces with basically the same logic, and the only difference is the hardware configuration. So in order to reuse these interfaces and reduce code redundancy, we extracted these interfaces and compiled them into a separate nxp-enetc-pf-common driver for use by the two PF drivers. In addition, we have developed the nxp-netc-blk-ctrl driver, which is used to control three blocks, namely Integrated Endpoint Register Block (IERB), Privileged Register Block (PRB) and NETCMIX block. The IERB contains registers that are used for pre-boot initialization, debug, and non-customer configuration. The PRB controls global reset and global error handling for NETC. The NETCMIX block is mainly used to set MII protocol and PCS protocol of the links, it also contains settings for some other functions. --- v1 Link: https://lore.kernel.org/imx/20241009095116.147412-1-wei.fang@nxp.com/ v2 Link: https://lore.kernel.org/imx/20241015125841.1075560-1-wei.fang@nxp.com/ v3 Link: https://lore.kernel.org/imx/20241017074637.1265584-1-wei.fang@nxp.com/ v4 Link: https://lore.kernel.org/imx/20241022055223.382277-1-wei.fang@nxp.com/ --- Clark Wang (2): net: enetc: extract enetc_int_vector_init/destroy() from enetc_alloc_msix() net: enetc: optimize the allocation of tx_bdr Vladimir Oltean (1): net: enetc: remove ERR050089 workaround for i.MX95 Wei Fang (10): dt-bindings: net: add compatible string for i.MX95 EMDIO dt-bindings: net: add i.MX95 ENETC support dt-bindings: net: add bindings for NETC blocks control net: enetc: add initial netc-blk-ctrl driver support net: enetc: extract common ENETC PF parts for LS1028A and i.MX95 platforms net: enetc: build enetc_pf_common.c as a separate module PCI: Add NXP NETC vendor ID and device IDs net: enetc: add i.MX95 EMDIO support net: enetc: add preliminary support for i.MX95 ENETC PF MAINTAINERS: update ENETC driver files and maintainers .../bindings/net/fsl,enetc-mdio.yaml | 11 +- .../devicetree/bindings/net/fsl,enetc.yaml | 34 +- .../bindings/net/nxp,netc-blk-ctrl.yaml | 104 +++ MAINTAINERS | 7 + drivers/net/ethernet/freescale/enetc/Kconfig | 40 + drivers/net/ethernet/freescale/enetc/Makefile | 9 + drivers/net/ethernet/freescale/enetc/enetc.c | 261 +++--- drivers/net/ethernet/freescale/enetc/enetc.h | 30 +- .../net/ethernet/freescale/enetc/enetc4_hw.h | 152 ++++ .../net/ethernet/freescale/enetc/enetc4_pf.c | 759 ++++++++++++++++++ .../ethernet/freescale/enetc/enetc_ethtool.c | 35 +- .../net/ethernet/freescale/enetc/enetc_hw.h | 53 +- .../ethernet/freescale/enetc/enetc_pci_mdio.c | 21 + .../net/ethernet/freescale/enetc/enetc_pf.c | 310 +------ .../net/ethernet/freescale/enetc/enetc_pf.h | 42 + .../freescale/enetc/enetc_pf_common.c | 338 ++++++++ .../net/ethernet/freescale/enetc/enetc_qos.c | 2 +- .../net/ethernet/freescale/enetc/enetc_vf.c | 6 + .../ethernet/freescale/enetc/netc_blk_ctrl.c | 438 ++++++++++ include/linux/fsl/netc_global.h | 19 + include/linux/pci_ids.h | 7 + 21 files changed, 2263 insertions(+), 415 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml create mode 100644 drivers/net/ethernet/freescale/enetc/enetc4_hw.h create mode 100644 drivers/net/ethernet/freescale/enetc/enetc4_pf.c create mode 100644 drivers/net/ethernet/freescale/enetc/enetc_pf_common.c create mode 100644 drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c create mode 100644 include/linux/fsl/netc_global.h