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

[Roger Quadros <rogerq@kernel.org>]

On 22/08/2024 08:28, MD Danish Anwar wrote:
> 
> 
> 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.

This is good. Thanks!

> 
>>>  	/* 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 */
>>
> 

-- 
cheers,
-roger