Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <17cd1e70-73bc-78d5-7e9d-7b133d6f464b@ti.com>
Date:   Thu, 20 Jul 2023 17:12:50 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [EXTERNAL] Re: [PATCH v10 2/2] net: ti: icssg-prueth: Add ICSSG
 ethernet driver
Content-Language: en-US
To:     Jakub Kicinski <kuba@kernel.org>,
        MD Danish Anwar <danishanwar@ti.com>
CC:     Randy Dunlap <rdunlap@infradead.org>,
        Roger Quadros <rogerq@kernel.org>,
        Simon Horman <simon.horman@corigine.com>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Richard Cochran <richardcochran@gmail.com>,
        Conor Dooley <conor+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>, <nm@ti.com>, <srk@ti.com>,
        <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <netdev@vger.kernel.org>, <linux-omap@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>
References: <20230719082755.3399424-1-danishanwar@ti.com>
 <20230719082755.3399424-3-danishanwar@ti.com>
 <20230719213543.0380e13e@kernel.org>
From:   Md Danish Anwar <a0501179@ti.com>
Organization: Texas Instruments
In-Reply-To: <20230719213543.0380e13e@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Jakub,

On 20/07/23 10:05 am, Jakub Kicinski wrote:
> The patch is too big to review.
> 
> Please break it apart separating into individual features, targeting
> around 10 patches in the series. That will make it easier for reviewers
> to take a look at the features in which they have expertise.
> 

Sure Jakub. I will try to break this patch in multiple patches as below.

Patch 1: Introduce Firmware mapping for the driver (icss_switch_map.h)

Patch 2: Introduce mii helper APIs. (icssg_mii_rt.h and icssg_mii_cfg.h). This
patch will also introduce basic prueth and emac structures in icssg_prueth.h as
these structures will be used by the helper APIs.

Patch 3: Introduce firmware configuration and classification APIs.
(icssg_classifier.c, icssg_config.h and icssg_config.c)

Patch 4: Introduce APIs for ICSSG Queues (icssg_queues.c)

Patch 5: Introduce ICSSG Ethernet driver. (icssg_prueth.c and icssg_prueth.h)
This patch will enable the driver and basic functionality can work after this
patch. This patch will be using all the APIs introduced earlier. This patch
will also include Kconfig and Makefile changes.

Patch 6: Enable standard statistics via ndo_get_stats64

Patch 7: Introduce ethtool ops for ICSSG

Patch 8: Introduce power management support (suspend / resume APIs)

However this structure of patches will introduce some APIs earlier (in patch
2,3 and 4) which will be used later by patch 5. I hope it will be OK to
introduce APIs and macros earlier and use them later.

This restructuring will shorten all the individual patches. However patch 5
will still be a bit large as patch 5 introduces all the neccessary APIs as
driver probe / remove, ndo open / close, tx/rx etc.

Currnetly this single patch has close to 4000 insertion and is touching 12
files. After restructring patch 5 will have around 1800 insertions and will
touch only 4 files (icssg_prueth.c, icssg_prueth.h, Kconfig, Makefile). This is
still significant improvement.

Please let me know if this is OK.

Also this patch has Reviewed-By tag of Andrew. Can I carry forward his
Reviewed-By tag in all patches or do I need to drop it?

> See two things which jumped out at me immediately below:
> 
> On Wed, 19 Jul 2023 13:57:55 +0530 MD Danish Anwar wrote:
>> +	ICSSG_STATS(rx_crc_error_frames),
> 
>> +	ICSSG_STATS(rx_max_size_error_frames),
>> +	ICSSG_STATS(rx_frame_min_size),
>> +	ICSSG_STATS(rx_min_size_error_frames),
>> +	ICSSG_STATS(rx_overrun_frames),
> 
>> +	ICSSG_STATS(rx_64B_frames),
>> +	ICSSG_STATS(rx_bucket1_frames),
>> +	ICSSG_STATS(rx_bucket2_frames),
>> +	ICSSG_STATS(rx_bucket3_frames),
>> +	ICSSG_STATS(rx_bucket4_frames),
>> +	ICSSG_STATS(rx_bucket5_frames),
>> +	ICSSG_STATS(rx_total_bytes),
>> +	ICSSG_STATS(rx_tx_total_bytes),
>> +	/* Tx */
>> +	ICSSG_STATS(tx_good_frames),
>> +	ICSSG_STATS(tx_broadcast_frames),
>> +	ICSSG_STATS(tx_multicast_frames),
>> +	ICSSG_STATS(tx_odd_nibble_frames),
>> +	ICSSG_STATS(tx_underflow_errors),
>> +	ICSSG_STATS(tx_frame_max_size),
>> +	ICSSG_STATS(tx_max_size_error_frames),
>> +	ICSSG_STATS(tx_frame_min_size),
>> +	ICSSG_STATS(tx_min_size_error_frames),
>> +	ICSSG_STATS(tx_bucket1_size),
>> +	ICSSG_STATS(tx_bucket2_size),
>> +	ICSSG_STATS(tx_bucket3_size),
>> +	ICSSG_STATS(tx_bucket4_size),
>> +	ICSSG_STATS(tx_64B_frames),
>> +	ICSSG_STATS(tx_bucket1_frames),
>> +	ICSSG_STATS(tx_bucket2_frames),
>> +	ICSSG_STATS(tx_bucket3_frames),
>> +	ICSSG_STATS(tx_bucket4_frames),
>> +	ICSSG_STATS(tx_bucket5_frames),
>> +	ICSSG_STATS(tx_total_bytes),
> 
> Please use standard stats:
> https://docs.kernel.org/next/networking/statistics.html
> 

Sure. I will use standard stats in patch 6.

> And do not duplicate those stats in the ethool -S output.
> 

Sure I will make sure to not duplicate standard stats in driver specific stats
of ethtool -S output.

>> +static const char emac_ethtool_priv_flags[][ETH_GSTRING_LEN] = {
>> +	"iet-frame-preemption",
>> +	"iet-mac-verify",
>> +};
> 
> What are these? We have a proper ethtool API for frame preemption.

I will drop this.

Please let me know if this approach looks ok. I will go ahead and start working
on it. I Will send next revision at the earliest.

-- 
Thanks and Regards,
Danish.