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

[Simon Horman <horms@kernel.org>]

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

> 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.