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

["Anwar, Md Danish" <a0501179@ti.com>]

On 8/22/2024 4:59 PM, Roger Quadros wrote:
> 
> 
> 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!
> 

Sure I will post next version soon.

>>
>>>>  	/* 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,
Md Danish Anwar