RE: [PATCH v3 net-next 12/13] net: enetc: add preliminary support for i.MX95 ENETC PF

[Wei Fang]

> -----Original Message-----
> From: Wei Fang
> Sent: 2024年10月18日 10:04
> To: Frank Li <frank.li@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 v3 net-next 12/13] net: enetc: add preliminary support for
> i.MX95 ENETC PF
> 
> [...]
> > > @@ -1721,14 +1724,25 @@ void enetc_get_si_caps(struct enetc_si *si)
> > >  	struct enetc_hw *hw = &si->hw;
> > >  	u32 val;
> > >
> > > +	if (is_enetc_rev1(si))
> > > +		si->clk_freq = ENETC_CLK;
> > > +	else
> > > +		si->clk_freq = ENETC_CLK_333M;
> >
> > can you use clk_gate_rate() to get frequency instead of hardcode here.
> 
> clk_gate_rate() is not possible to be used here, enetc_get_si_caps() is shared
> by PF and VFs, but VF does not have DT node. Second, LS1028A and S32
> platform do not use the clocks property.
> 
> > Or you should use standard PCIe version information.
> >
> 
> What do you mean standard PCIe version? is_enetc_rev1() gets the revision
> from struct pci_dev:: revision, my understanding is that this is the revision
> provided by PCIe.
> 
> [...]
> > > +
> > > @@ -593,6 +620,9 @@ static int enetc_get_rxnfc(struct net_device *ndev,
> > struct ethtool_rxnfc *rxnfc,
> > >  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > >  	int i, j;
> > >
> > > +	if (is_enetc_rev4(priv->si))
> > > +		return -EOPNOTSUPP;
> > > +
> > >  	switch (rxnfc->cmd) {
> > >  	case ETHTOOL_GRXRINGS:
> > >  		rxnfc->data = priv->num_rx_rings;
> > > @@ -643,6 +673,9 @@ static int enetc_set_rxnfc(struct net_device *ndev,
> > struct ethtool_rxnfc *rxnfc)
> > >  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > >  	int err;
> > >
> > > +	if (is_enetc_rev4(priv->si))
> > > +		return -EOPNOTSUPP;
> > > +
> > >  	switch (rxnfc->cmd) {
> > >  	case ETHTOOL_SRXCLSRLINS:
> > >  		if (rxnfc->fs.location >= priv->si->num_fs_entries) @@ -678,6
> > > +711,9 @@ static u32 enetc_get_rxfh_key_size(struct net_device *ndev)
> > > {
> > >  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > >
> > > @@ -843,8 +890,12 @@ static int enetc_set_coalesce(struct net_device
> > > *ndev,  static int enetc_get_ts_info(struct net_device *ndev,
> > >  			     struct kernel_ethtool_ts_info *info)  {
> > > +	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > >  	int *phc_idx;
> > >
> > > +	if (is_enetc_rev4(priv->si))
> > > +		return -EOPNOTSUPP;
> > > +
> >
> > Can you just not set enetc_pf_ethtool_ops if it is imx95 instead of check each
> > ethtools function? or use difference enetc_pf_ethtool_ops for imx95?
> >
> 
> For the first question, in the current patch, i.MX95 already supports some
> ethtool interfaces, so there is no need to remove them.
> 
> For the second question, for LS1028A and i.MX95, the logic of these ethtool
> interfaces is basically the same, the difference is the hardware operation part.
> It's just that support for i.MX95 has not yet been added. Both the current
> approach and the approach you suggested will eventually merge into using the
> same enetc_pf_ethtool_ops, so I don't think there is much practical point in
> switching to the approach you mentioned.

I thought about it again, your suggestion is more reasonable and easier to
understand. I will merge the two enetc_pf_ethtool_ops into one after I
complete the support of all ethtool interfaces of i.MX95. Thanks.