| Message ID | 20240628-am65-cpsw-multi-rx-v2-5-c399cb77db56@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: ti: am65-cpsw: Add multi queue RX support | expand |
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.
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.
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); +}
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. 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(+)