Re: [PATCH net-next v6 2/2] net: ti: icssg-prueth: Add support for PA Stats

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

On 21/08/24 6:05 pm, Roger Quadros wrote:
> 
> 
> On 20/08/2024 12:16, MD Danish Anwar wrote:
>> Add support for dumping PA stats registers via ethtool.
>> Firmware maintained stats are stored at PA Stats registers.
>> Also modify emac_get_strings() API to use ethtool_puts().
>>
>> This commit also renames the array icssg_all_stats to icssg_mii_g_rt_stats
>> and creates a new array named icssg_all_pa_stats for PA Stats.
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---

[ ... ]

>> +
>>  #define ICSSG_STATS(field, stats_type)			\
>>  {							\
>>  	#field,						\
>> @@ -84,13 +98,24 @@ struct miig_stats_regs {
>>  	stats_type					\
>>  }
>>  
>> +#define ICSSG_PA_STATS(field)			\
>> +{						\
>> +	#field,					\
>> +	offsetof(struct pa_stats_regs, field),	\
>> +}
>> +
>>  struct icssg_stats {
> 
> icssg_mii_stats?
> 

Sure Roger. I will name it icssg_miig_stats to be consistent with
'struct miig_stats_regs'

>>  	char name[ETH_GSTRING_LEN];
>>  	u32 offset;
>>  	bool standard_stats;
>>  };
>>  
>> -static const struct icssg_stats icssg_all_stats[] = {
>> +struct icssg_pa_stats {
>> +	char name[ETH_GSTRING_LEN];
>> +	u32 offset;
>> +};
>> +
>> +static const struct icssg_stats icssg_mii_g_rt_stats[] = {
> 
> icssg_all_mii_stats? to be consistend with the newly added
> icssg_pa_stats and icssg_all_pa_stats.
> 
> Could you please group all mii_stats data strucutres and arrays together
> followed by pa_stats data structures and arrays?
> 

Sure Roger, I will group all mii stats related data structures and
pa_stats related data structures together.

The sequence and naming will be something like this,

struct miig_stats_regs
#define ICSSG_MIIG_STATS(field, stats_type)
struct icssg_miig_stats
static const struct icssg_miig_stats icssg_all_miig_stats[]

struct pa_stats_regs
#define ICSSG_PA_STATS(field)
struct icssg_pa_stats
static const struct icssg_pa_stats icssg_all_pa_stats[]

Let me know if this looks ok to you.

>>  	/* Rx */
>>  	ICSSG_STATS(rx_packets, true),
>>  	ICSSG_STATS(rx_broadcast_frames, false),
>> @@ -155,4 +180,11 @@ static const struct icssg_stats icssg_all_stats[] = {
>>  	ICSSG_STATS(tx_bytes, true),t
>>  };
>>  
>> +static const struct icssg_pa_stats icssg_all_pa_stats[] = > +	ICSSG_PA_STATS(fw_rx_cnt),
>> +	ICSSG_PA_STATS(fw_tx_cnt),
>> +	ICSSG_PA_STATS(fw_tx_pre_overflow),
>> +	ICSSG_PA_STATS(fw_tx_exp_overflow),
>> +};
>> +
>>  #endif /* __NET_TI_ICSSG_STATS_H */
> 

-- 
Thanks and Regards,
Danish