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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZxKkhykJ5joWZxaR@lizhi-Precision-Tower-5810>
Date: Fri, 18 Oct 2024 14:10:15 -0400
From: Frank Li <Frank.li@....com>
To: Wei Fang <wei.fang@....com>
Cc: "davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>,
	Vladimir Oltean <vladimir.oltean@....com>,
	Claudiu Manoil <claudiu.manoil@....com>,
	Clark Wang <xiaoning.wang@....com>,
	"christophe.leroy@...roup.eu" <christophe.leroy@...roup.eu>,
	"linux@...linux.org.uk" <linux@...linux.org.uk>,
	"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
	"horms@...nel.org" <horms@...nel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: Re: [PATCH v3 net-next 12/13] net: enetc: add preliminary support
 for i.MX95 ENETC PF

On Fri, Oct 18, 2024 at 03:05:52AM +0000, Wei Fang wrote:
> > -----Original Message-----
> > From: Wei Fang
> > Sent: 2024年10月18日 10:04
> > To: Frank Li <frank.li@....com>
> > Cc: davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
> > pabeni@...hat.com; robh@...nel.org; krzk+dt@...nel.org;
> > conor+dt@...nel.org; Vladimir Oltean <vladimir.oltean@....com>; Claudiu
> > Manoil <claudiu.manoil@....com>; Clark Wang <xiaoning.wang@....com>;
> > christophe.leroy@...roup.eu; linux@...linux.org.uk; bhelgaas@...gle.com;
> > horms@...nel.org; imx@...ts.linux.dev; netdev@...r.kernel.org;
> > devicetree@...r.kernel.org; linux-kernel@...r.kernel.org;
> > linux-pci@...r.kernel.org
> > 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.

It should be set when pf probe.

enetc4_pf_netdev_create()
{
	...
	priv->ref_clk = devm_clk_get_optional(dev, "ref");

	I am sure if it is "ref" clock.

	if (ref_clk)
		si->clk_freq = clk_get_rate(ref_clk);
	else
		si->clk_freq = ENETC_CLK; //default one for old LS1028A.

	Next time, it may be become 444MHz, 555Mhz...
}

> >
> > > 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.