RE: [PATCH net-next 10/11] net: enetc: add preliminary support for i.MX95 ENETC PF

[Wei Fang]

> -----Original Message-----
> From: Frank Li <frank.li@xxxxxxx>
> Sent: 2024年10月11日 12:12
> To: Wei Fang <wei.fang@xxxxxxx>
> Cc: davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; Vladimir Oltean <vladimir.oltean@xxxxxxx>; Claudiu
> Manoil <claudiu.manoil@xxxxxxx>; Clark Wang <xiaoning.wang@xxxxxxx>;
> christophe.leroy@xxxxxxxxxx; linux@xxxxxxxxxxxxxxx; bhelgaas@xxxxxxxxxx;
> imx@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next 10/11] net: enetc: add preliminary support for
> i.MX95 ENETC PF
> 
> On Fri, Oct 11, 2024 at 02:02:03AM +0000, Wei Fang wrote:
> > > -----Original Message-----
> > > From: Frank Li <frank.li@xxxxxxx>
> > > Sent: 2024年10月10日 23:22
> > > To: Wei Fang <wei.fang@xxxxxxx>
> > > Cc: davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> > > pabeni@xxxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx;
> > > conor+dt@xxxxxxxxxx; Vladimir Oltean <vladimir.oltean@xxxxxxx>;
> Claudiu
> > > Manoil <claudiu.manoil@xxxxxxx>; Clark Wang
> <xiaoning.wang@xxxxxxx>;
> > > christophe.leroy@xxxxxxxxxx; linux@xxxxxxxxxxxxxxx;
> bhelgaas@xxxxxxxxxx;
> > > imx@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH net-next 10/11] net: enetc: add preliminary support for
> > > i.MX95 ENETC PF
> > >
> > > On Thu, Oct 10, 2024 at 04:59:45AM +0000, Wei Fang wrote:
> > > > > On Wed, Oct 09, 2024 at 05:51:15PM +0800, Wei Fang wrote:
> > > > > > The i.MX95 ENETC has been upgraded to revision 4.1, which is very
> > > > > > different from the LS1028A ENETC (revision 1.0) except for the SI
> > > > > > part. Therefore, the fsl-enetc driver is incompatible with i.MX95
> > > > > > ENETC PF. So we developed the nxp-enetc4 driver for i.MX95 ENETC
> > > > >             So add new nxp-enetc4 driver for i.MX95 ENETC PF with
> > > > > major revision 4.
> > > > >
> > > > > > PF, and this driver will be used to support the ENETC PF with
> > > > > > major revision 4 in the future.
> > > > > >
> > > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > > b/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > > > index 97524dfa234c..7f1ea11c33a0 100644
> > > > > > --- a/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> > > > > > @@ -14,6 +14,7 @@
> > > > > >  #include <net/xdp.h>
> > > > > >
> > > > > >  #include "enetc_hw.h"
> > > > > > +#include "enetc4_hw.h"
> > > > > >
> > > > > >  #define ENETC_SI_ALIGN	32
> > > > > >
> > > > > > +static inline bool is_enetc_rev1(struct enetc_si *si) {
> > > > > > +	return si->pdev->revision == ENETC_REV1; }
> > > > > > +
> > > > > > +static inline bool is_enetc_rev4(struct enetc_si *si) {
> > > > > > +	return si->pdev->revision == ENETC_REV4; }
> > > > > > +
> > > > >
> > > > > Actually, I suggest you check features, instead of check version number.
> > > > >
> > > > This is mainly used to distinguish between ENETC v1 and ENETC v4 in
> > > > the general interfaces. See enetc_ethtool.c.
> > >
> > > Suggest use flags, such as, IS_SUPPORT_ETHTOOL.
> > >
> > > otherwise, your check may become complex in future.
> > >
> > > If use flags, you just change id table in future.
> >
> > enetc_ethtool just is an example, I meant that the ENETCv4 and ENETCv1
> > use some common drivers, like enect_pf_common, enetc-core, so different
> > hardware versions have different logic, that's all.
> 
> My means is that avoid use v1\v2 to distingiush it and use supported
> features in difference version for example:

I think you misunderstood what I meant. This is not to distinguish different
features supported by different versions of hardware. In fact, the functions
of the two versions of enetc are basically the same, but the hardware
implementations are different, so the code logic of two hardware is different.
Take 802.1 Qbu as an example. The enetc_set_mm() interface directly returns
"unsupported" for enetc v4, but this does not mean that enetc v4 is really not
supported. It's just that for the current patch, we have not added support yet.
And we will add the Qbu support for enetc v4 in the future and add some
different configurations for enetc v4.

> 
> ENETC_FEATURE_1, ENETC_FEATURE_2, ENETC_FEATURE_3,
> ENETC_FEATURE_4.
> 
> { PCI_DEVICE(PCI_VENDOR_ID_NXP2, PCI_DEVICE_ID_NXP2_ENETC_PF)
>   .driver_data = ENETC_FEATURE_1 |  ENETC_FEATURE_2 |
> ENETC_FEATURE_4
>   PCI_DEVICE(....)
>   .driver_data = ENETC_FEATURE_1 | ENETC_FEATURE_3,
>   PCI_DEVICE(...)
>   .driver_data = ENETC_FEATURE_4,
> )
> 
> It will be easy to know the difference between difference version. Your if
> check logic will be simple.
> 
> if (driver_data & ENETC_FEATURE_1)
>   ....
> 
> otherwise
>    if (vers == 1 || vers == 2 || ver == 5), which distribute to difference
> places in whole code.
> 
> It is real hard to know hardware differences between version in future.
> 
> You can ref drivers/misc/pci_endpoint_test.c
> 
> Frank
> 
> >
> > >
> > > { PCI_DEVICE(PCI_VENDOR_ID_NXP2, PCI_DEVICE_ID_NXP2_ENETC_PF),
> > >   .driver_data = IS_SUPPORT_ETHTOOL | .... },
> > >
> > > Frank
> > > >
> > > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > > > > b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..e38ade76260b
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> > > > > > @@ -0,0 +1,761 @@
> > > > > > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> > > > > > +/* Copyright 2024 NXP */
> > > > > > +#include <linux/unaligned.h>
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/of_net.h>
> > > > > > +#include <linux/of_platform.h>
> > > > > > +#include <linux/clk.h>
> > > > > > +#include <linux/pinctrl/consumer.h> #include
> > > > > > +<linux/fsl/netc_global.h>
> > > > >
> > > > > sort headers.
> > > > >
> > > >
> > > > Sure
> > > >
> > > > > > +static int enetc4_pf_probe(struct pci_dev *pdev,
> > > > > > +			   const struct pci_device_id *ent) {
> > > > > > +	struct device *dev = &pdev->dev;
> > > > > > +	struct enetc_si *si;
> > > > > > +	struct enetc_pf *pf;
> > > > > > +	int err;
> > > > > > +
> > > > > > +	err = enetc_pci_probe(pdev, KBUILD_MODNAME, sizeof(*pf));
> > > > > > +	if (err) {
> > > > > > +		dev_err(dev, "PCIe probing failed\n");
> > > > > > +		return err;
> > > > >
> > > > > use dev_err_probe()
> > > > >
> > > >
> > > > Okay
> > > >
> > > > > > +	}
> > > > > > +
> > > > > > +	/* si is the private data. */
> > > > > > +	si = pci_get_drvdata(pdev);
> > > > > > +	if (!si->hw.port || !si->hw.global) {
> > > > > > +		err = -ENODEV;
> > > > > > +		dev_err(dev, "Couldn't map PF only space!\n");
> > > > > > +		goto err_enetc_pci_probe;
> > > > > > +	}
> > > > > > +
> > > > > > +	err = enetc4_pf_struct_init(si);
> > > > > > +	if (err)
> > > > > > +		goto err_pf_struct_init;
> > > > > > +
> > > > > > +	pf = enetc_si_priv(si);
> > > > > > +	err = enetc4_pf_init(pf);
> > > > > > +	if (err)
> > > > > > +		goto err_pf_init;
> > > > > > +
> > > > > > +	pinctrl_pm_select_default_state(dev);
> > > > > > +	enetc_get_si_caps(si);
> > > > > > +	err = enetc4_pf_netdev_create(si);
> > > > > > +	if (err)
> > > > > > +		goto err_netdev_create;
> > > > > > +
> > > > > > +	return 0;
> > > > > > +
> > > > > > +err_netdev_create:
> > > > > > +err_pf_init:
> > > > > > +err_pf_struct_init:
> > > > > > +err_enetc_pci_probe:
> > > > > > +	enetc_pci_remove(pdev);
> > > > >
> > > > > you can use devm_add_action_or_reset() to remove these goto labels.
> > > > >
> > > > Subsequent patches will have corresponding processing for these
> > > > labels, so I don't want to add too many devm_add_action_or_reset ().