Re: [PATCH net-next] net: ti: icssg-prueth: Add ICSSG FW Stats

[MD Danish Anwar <danishanwar@ti.com>]

On 04/03/25 6:55 am, Jakub Kicinski wrote:
> On Thu, 27 Feb 2025 15:07:12 +0530 MD Danish Anwar wrote:
>> +	ICSSG_PA_STATS(FW_PREEMPT_BAD_FRAG),
>> +	ICSSG_PA_STATS(FW_PREEMPT_ASSEMBLY_ERR),
>> +	ICSSG_PA_STATS(FW_PREEMPT_FRAG_CNT_TX),
>> +	ICSSG_PA_STATS(FW_PREEMPT_ASSEMBLY_OK),
>> +	ICSSG_PA_STATS(FW_PREEMPT_FRAG_CNT_RX),
> 
> I presume frame preemption is implemented in silicon? If yes -
> what makes these "FW statistics"? Does the FW collect them from 

The statistics are maintained / updated by firmware and thus the name.

Preemption is implemented partially in both the hardware and firmware.
The STATE MACHINE for preemption is in the firmware. The decision to
when to PREEMEPT / ASSEMBLE a packet is made in firmware.

These preemption statistics are updated by the firmware based on the
action performed by the firmware. Driver can read these to know the
statistics of preemption. These stats will be able used by
ethtool_mm_stats once the support for Preemption is added in the driver.

> the device or the frames are for FW? 
> 
>> +	ICSSG_PA_STATS(FW_RX_EOF_SHORT_FRMERR),
>> +	ICSSG_PA_STATS(FW_RX_B0_DROP_EARLY_EOF),
>> +	ICSSG_PA_STATS(FW_TX_JUMBO_FRM_CUTOFF),
>> +	ICSSG_PA_STATS(FW_RX_EXP_FRAG_Q_DROP),
>> +	ICSSG_PA_STATS(FW_RX_FIFO_OVERRUN),
>> +	ICSSG_PA_STATS(FW_CUT_THR_PKT),
>> +	ICSSG_PA_STATS(FW_HOST_RX_PKT_CNT),
>> +	ICSSG_PA_STATS(FW_HOST_TX_PKT_CNT),
>> +	ICSSG_PA_STATS(FW_HOST_EGRESS_Q_PRE_OVERFLOW),
>> +	ICSSG_PA_STATS(FW_HOST_EGRESS_Q_EXP_OVERFLOW),
>>  };
>>  
>>  #endif /* __NET_TI_ICSSG_STATS_H */
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
>> index 424a7e945ea8..d30203a0978c 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
>> @@ -231,4 +231,109 @@
>>  /* Start of 32 bits PA_STAT counters */
>>  #define PA_STAT_32b_START_OFFSET                           0x0080
>>  
>> +/* Diagnostic error counter which increments when RTU drops a locally injected
>> + * packet due to port disabled or rule violation.
>> + */
>> +#define FW_RTU_PKT_DROP		0x0088
>> +
>> +/* Tx Queue Overflow Counters */
>> +#define FW_Q0_OVERFLOW		0x0090
>> +#define FW_Q1_OVERFLOW		0x0098
>> +#define FW_Q2_OVERFLOW		0x00A0
>> +#define FW_Q3_OVERFLOW		0x00A8
>> +#define FW_Q4_OVERFLOW		0x00B0
>> +#define FW_Q5_OVERFLOW		0x00B8
>> +#define FW_Q6_OVERFLOW		0x00C0
>> +#define FW_Q7_OVERFLOW		0x00C8
>> +
>> +/* Incremented if a packet is dropped at PRU because of a rule violation */
>> +#define FW_DROPPED_PKT		0x00F8
> 
> Instead of adding comments here please add a file under
> Documentation/networking/device_drivers/ with the explanations.
> That's far more likely to be discovered by users, no?

Sure I will drop these MACRO comments and create a .rst file in
Documentation/networking/device_drivers/

One question though, should I create a table for the stats and it's
description or should I create a section for each stats?

Something like this,

FW_RTU_PKT_DROP
---------------
Diagnostic error counter which increments when RTU drops a locally
injected packet due to port being disabled or rule violation.

Please let me know what do you think.

-- 
Thanks and Regards,
Danish