Re: [PATCH net-next v2 5/7] net: ethernet: ti: cpsw_ale: add policer/classifier helpers

[Roger Quadros <rogerq@kernel.org>]

On 01/07/2024 10:35, Simon Horman wrote:
> On Fri, Jun 28, 2024 at 03:01:54PM +0300, Roger Quadros wrote:
>> The Policer registers in the ALE register space are just shadow registers
>> and use an index field in the policer table control register to read/write
>> to the actual Polier registers.
>> Add helper functions to Read and Write to Policer registers.
>>
>> Also add a helper function to set the thread value to classifier/policer
>> mapping. Any packet that first matches the classifier will be sent to the
>> thread (flow) that is set in the classifer to thread mapping table.
> 
> nit: classifier
> 
>      Flagged by checkpatch.pl --codespell

will fix.

> 
>> If not set then it goes to the default flow.
>>
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  drivers/net/ethernet/ti/cpsw_ale.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
>> index 7bd0dc20f894..75a17184d34c 100644
>> --- a/drivers/net/ethernet/ti/cpsw_ale.c
>> +++ b/drivers/net/ethernet/ti/cpsw_ale.c
>> @@ -1626,3 +1626,27 @@ u32 cpsw_ale_get_num_entries(struct cpsw_ale *ale)
>>  {
>>  	return ale ? ale->params.ale_entries : 0;
>>  }
>> +
>> +/* Reads the specified policer index into ALE POLICER registers */
>> +static void cpsw_ale_policer_read_idx(struct cpsw_ale *ale, u32 idx)
>> +{
>> +	idx &= ALE_POLICER_TBL_INDEX_MASK;
>> +	writel_relaxed(idx, ale->params.ale_regs + ALE_POLICER_TBL_CTL);
>> +}
>> +
>> +/* Writes the ALE POLICER registers into the specified policer index */
>> +static void cpsw_ale_policer_write_idx(struct cpsw_ale *ale, u32 idx)
>> +{
>> +	idx &= ALE_POLICER_TBL_INDEX_MASK;
>> +	idx |= ALE_POLICER_TBL_WRITE_ENABLE;
>> +	writel_relaxed(idx, ale->params.ale_regs + ALE_POLICER_TBL_CTL);
>> +}
>> +
>> +/* enables/disables the custom thread value for the specified policer index */
>> +static void cpsw_ale_policer_thread_idx_enable(struct cpsw_ale *ale, u32 idx,
>> +					       u32 thread_id, bool enable)
>> +{
>> +	regmap_field_write(ale->fields[ALE_THREAD_CLASS_INDEX], idx);
>> +	regmap_field_write(ale->fields[ALE_THREAD_VALUE], thread_id);
>> +	regmap_field_write(ale->fields[ALE_THREAD_ENABLE], enable ? 1 : 0);
>> +}
>>
> 
> I like that this patch-set is broken out into nice discrete patches,
> including this one. So I'm in two minds about the comment I'm about to
> make, but here goes.
> 
> As these helpers are unused this raises Warnings with W=1 builds on
> gcc-13 and clang-18, which is generally undesirable for networking patches.
> 
> I can think of a few options here;
> * Ignore the warnings
> * Squash this patch into the following one
> * Add some annotations, e.g. __maybe_unused or __always_unused.
>   Likely dropped in the next patch.
> 
> I think I lean towards the last option.
> But I won't push the point any further regardless.

Thanks for the suggestions. I do not like using any of the __unused flags.
So I'm leaning towards your second solution. Will fix this in next revision.

-- 
cheers,
-roger