> -----Original Message-----
> From: Frank Li <frank.li@xxxxxxx>
> Sent: 2024年10月16日 2:38
> 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;
> horms@xxxxxxxxxx; imx@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-pci@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 net-next 12/13] net: enetc: add preliminary support for
> i.MX95 ENETC PF
>
> On Tue, Oct 15, 2024 at 08:58:40PM +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 add new nxp-enetc4 driver to support i.MX95 ENETC PF, and
> > this driver will be used to support the ENETC PF with major revision 4
> > for other SoCs in the future.
> >
> > Currently, the nxp-enetc4 driver only supports basic transmission
> > feature for i.MX95 ENETC PF, the more basic and advanced features will
> > be added in the subsequent patches.
>
> Nit: wrap at 75 char.
All lines are within 75 characters. I think you removed extra line breaks
from the email.
> > + if (is_enetc_rev1(si))
> > + si->clk_freq = ENETC_CLK;
> > + else
> > + si->clk_freq = ENETC_CLK_333M;
> > +
>
>
> As pervious suggested
> if use pci drvdata, needn't this check branch
> si->clk_freq = drvdata->clk_frq;
>
Subsequent ENETCs basically use the same device ID, so if we match drvdata
by vendor ID and device ID, their drvdata are the same. However, their system
clock frequencies are not exactly the same, some may be 400MHz, and some
may be 333MHz.
Of course, we can also match through compatible string, but VF node does not
have a corresponding DTS node, so this is not suitable for VF. However, this function
is shared by both PF and VF drivers. So I still think that different clock frequencies
should be distinguished according to the IP revision.
> > /* find out how many of various resources we have to work with */
> > val = enetc_rd(hw, ENETC_SICAPR0);
> > si->num_rx_rings = (val >> 16) & 0xff;
> > si->num_tx_rings = val & 0xff;
> >
> > @@ -2080,9 +2097,15 @@ void enetc_init_si_rings_params(struct
> enetc_ndev_priv *priv)
> > */
> > priv->num_rx_rings = min_t(int, cpus, si->num_rx_rings);
> > priv->num_tx_rings = si->num_tx_rings;
> > - priv->bdr_int_num = cpus;
> > + if (is_enetc_rev1(si)) {
> > + priv->bdr_int_num = cpus;
> > + priv->tx_ictt = enetc_usecs_to_cycles(600, ENETC_CLK);
> > + } else {
> > + priv->bdr_int_num = priv->num_rx_rings;
> > + priv->tx_ictt = enetc_usecs_to_cycles(500, ENETC_CLK_333M);
> > + }
> > +
>
> why need 500/600 for difference version?
>
Because the clock freq is different.
> Maybe
> priv->tx_ictt = enetc_usecs_to_cycles(500, si-> clk_freq);
>
Yes, I'll refine it, thanks
> > priv->ic_mode = ENETC_IC_RX_ADAPTIVE | ENETC_IC_TX_MANUAL;
> > - priv->tx_ictt = ENETC_TXIC_TIMETHR;
> > }
> > EXPORT_SYMBOL_GPL(enetc_init_si_rings_params);
> > +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_probe(dev, err, "PCIe probing failed\n");
> > + return err;
> > + }
>
> return dev_err_probe(dev, err, "PCIe probing failed\n");
>
Okay, thanks.
> > +
> > + /* si is the private data. */
> > + si = pci_get_drvdata(pdev);
> > + if (!si->hw.port || !si->hw.global) {
> > + err = -ENODEV;
> > + dev_err_probe(dev, err, "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);
>
> still suggest use devm_add_action_or_reset() to simplify error handle.
I'm not sure, but I will consider it. thanks.
>
> > +
> > + return err;
> > +}
