Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   "Voon, Weifeng" <weifeng.voon@intel.com>
To:     Andrew Lunn <andrew@lunn.ch>
CC:     "David S. Miller" <davem@davemloft.net>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Jose Abreu <joabreu@synopsys.com>,
        "Giuseppe Cavallaro" <peppe.cavallaro@st.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Alexandre Torgue <alexandre.torgue@st.com>,
        "Gomes, Vinicius" <vinicius.gomes@intel.com>,
        "Ong, Boon Leong" <boon.leong.ong@intel.com>
Subject: RE: [RFC net-next 1/5] net: stmmac: introduce IEEE 802.1Qbv
 configuration functionalities
Thread-Topic: [RFC net-next 1/5] net: stmmac: introduce IEEE 802.1Qbv
 configuration functionalities
Thread-Index: AQHVJdrUhwXCZv9SNEKube71uT/CUqahxnCAgADin9A=
Date:   Wed, 19 Jun 2019 08:48:16 +0000
Message-ID: <D6759987A7968C4889FDA6FA91D5CBC81472623A@PGSMSX103.gar.corp.intel.com>
References: <1560893778-6838-1-git-send-email-weifeng.voon@intel.com>
 <1560893778-6838-2-git-send-email-weifeng.voon@intel.com>
 <20190619030729.GA26784@lunn.ch>
In-Reply-To: <20190619030729.GA26784@lunn.ch>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-version: 11.0.600.7
dlp-reaction: no-action
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> > +static int est_poll_srwo(void *ioaddr) {
> > +	/* Poll until the EST GCL Control[SRWO] bit clears.
> > +	 * Total wait = 12 x 50ms ~= 0.6s.
> > +	 */
> > +	unsigned int retries = 12;
> > +	unsigned int value;
> > +
> > +	do {
> > +		value = TSN_RD32(ioaddr + MTL_EST_GCL_CTRL);
> > +		if (!(value & MTL_EST_GCL_CTRL_SRWO))
> > +			return 0;
> > +		msleep(50);
> > +	} while (--retries);
> > +
> > +	return -ETIMEDOUT;
> 
> Maybe use one of the readx_poll_timeout() macros?
> 
> > +static int est_read_gce(void *ioaddr, unsigned int row,
> > +			unsigned int *gates, unsigned int *ti_nsec,
> > +			unsigned int dbgb, unsigned int dbgm) {
> > +	struct tsn_hw_cap *cap = &dw_tsn_hwcap;
> > +	unsigned int ti_wid = cap->ti_wid;
> > +	unsigned int gates_mask;
> > +	unsigned int ti_mask;
> > +	unsigned int value;
> > +	int ret;
> > +
> > +	gates_mask = (1 << cap->txqcnt) - 1;
> > +	ti_mask = (1 << ti_wid) - 1;
> > +
> > +	ret = est_read_gcl_config(ioaddr, &value, row, 0, dbgb, dbgm);
> > +	if (ret) {
> > +		TSN_ERR("Read GCE failed! row=%u\n", row);
> 
> It is generally not a good idea to put wrappers around the kernel print
> functions. It would be better if all these functions took struct
> stmmac_priv *priv rather than ioaddr, so you could then do
> 
> 	netdev_err(priv->dev, "Read GCE failed! row=%u\n", row);
> 
> > +	/* Ensure that HW is not in the midst of GCL transition */
> > +	value = TSN_RD32(ioaddr + MTL_EST_CTRL);
> 
> Also, don't put wrapper around readl()/writel().
> 
> > +	value &= ~MTL_EST_CTRL_SSWL;
> > +
> > +	/* MTL_EST_CTRL value has been read earlier, if TILS value
> > +	 * differs, we update here.
> > +	 */
> > +	if (tils != dw_tsn_hwtunable[TSN_HWTUNA_TX_EST_TILS]) {
> > +		value &= ~MTL_EST_CTRL_TILS;
> > +		value |= (tils << MTL_EST_CTRL_TILS_SHIFT);
> > +
> > +		TSN_WR32(value, ioaddr + MTL_EST_CTRL);
> > +		dw_tsn_hwtunable[TSN_HWTUNA_TX_EST_TILS] = tils;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int est_set_ov(void *ioaddr,
> > +		      const unsigned int *ptov,
> > +		      const unsigned int *ctov)
> > +{
> > +	unsigned int value;
> > +
> > +	if (!dw_tsn_feat_en[TSN_FEAT_ID_EST])
> > +		return -ENOTSUPP;
> > +
> > +	value = TSN_RD32(ioaddr + MTL_EST_CTRL);
> > +	value &= ~MTL_EST_CTRL_SSWL;
> > +
> > +	if (ptov) {
> > +		if (*ptov > EST_PTOV_MAX) {
> > +			TSN_WARN("EST: invalid PTOV(%u), max=%u\n",
> > +				 *ptov, EST_PTOV_MAX);
> 
> It looks like most o the TSN_WARN should actually be netdev_dbg().
> 
>    Andrew

Hi Andrew,
This file is targeted for dual licensing which is GPL-2.0 OR BSD-3-Clause.
This is the reason why we are using wrappers around the functions so that
all the function call is generic.
	
Regards,
Weifeng