diff mbox series

[net-next,v4,3/6] net: ethernet: ti: cpsw_ale: use regfields for number of Entries and Policers

Message ID 20240910-am65-cpsw-multi-rx-v4-3-077fa6403043@kernel.org
State New
Headers show
Series net: ethernet: ti: am65-cpsw: Add multi queue RX support | expand

Commit Message

Roger Quadros Sept. 10, 2024, 9:24 a.m. UTC 
Use regfields for number of ALE Entries and Policers.

The variants that support Policers/Classifiers have the number
of policers encoded in the ALE_STATUS register.

Use that and show the number of Policers in the ALE info message.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
Reviewed-by: Simon Horman <horms@kernel.org>
---
Changelog:
v4:
- reverse Xmas tree declaration order fixes
v3:
- added Reviewed-by Simon Horman
---
 drivers/net/ethernet/ti/cpsw_ale.c | 25 +++++++++++++++++++------
 drivers/net/ethernet/ti/cpsw_ale.h |  3 +++
 2 files changed, 22 insertions(+), 6 deletions(-)

Comments

Geert Uytterhoeven Sept. 23, 2024, 1:41 p.m. UTC | #1 
Hi Roger,

On Tue, Sep 10, 2024 at 11:25 AM Roger Quadros <rogerq@kernel.org> wrote:
> Use regfields for number of ALE Entries and Policers.
>
> The variants that support Policers/Classifiers have the number
> of policers encoded in the ALE_STATUS register.
>
> Use that and show the number of Policers in the ALE info message.
>
> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> Reviewed-by: Simon Horman <horms@kernel.org>
> ---
> Changelog:
> v4:
> - reverse Xmas tree declaration order fixes

Thanks for your patch, which is now commit 11cbcfeaa79e5c76 ("net:
ethernet: ti: cpsw_ale: use regfields for number of Entries
and Policers").

This is causing the following warning on BeagleBone Black:

    WARNING: CPU: 0 PID: 34 at drivers/base/regmap/regmap.c:1208
devm_regmap_field_alloc+0xac/0xc8
    invalid empty mask defined
    CPU: 0 UID: 0 PID: 34 Comm: kworker/u4:3 Not tainted
6.11.0-rc7-boneblack-01443-g11cbcfeaa79e #152
    Hardware name: Generic AM33XX (Flattened Device Tree)
    Workqueue: events_unbound deferred_probe_work_func
    Call trace:
     unwind_backtrace from show_stack+0x10/0x14
     show_stack from dump_stack_lvl+0x68/0x88
     dump_stack_lvl from __warn+0x6c/0x1a8
     __warn from warn_slowpath_fmt+0x1bc/0x1d0
     warn_slowpath_fmt from devm_regmap_field_alloc+0xac/0xc8
     devm_regmap_field_alloc from cpsw_ale_create+0x10c/0x36c
     cpsw_ale_create from cpsw_init_common+0x1fc/0x310

> --- a/drivers/net/ethernet/ti/cpsw_ale.c
> +++ b/drivers/net/ethernet/ti/cpsw_ale.c
> @@ -1303,6 +1303,9 @@ static const struct reg_field ale_fields_cpsw_nu[] = {
>         /* CPSW_ALE_IDVER_REG */
>         [MINOR_VER]     = REG_FIELD(ALE_IDVER, 0, 7),
>         [MAJOR_VER]     = REG_FIELD(ALE_IDVER, 8, 10),
> +       /* CPSW_ALE_STATUS_REG */
> +       [ALE_ENTRIES]   = REG_FIELD(ALE_STATUS, 0, 7),
> +       [ALE_POLICERS]  = REG_FIELD(ALE_STATUS, 8, 15),

You are adding these entries only to ale_fields_cpsw_nu[], not
to ale_fields_cpsw[], while cpsw_ale_regfield_init() loops over
ALE_FIELDS_MAX entries, whether they are valid or not:

    static int cpsw_ale_regfield_init(struct cpsw_ale *ale)
    {
            const struct reg_field *reg_fields = ale->params.reg_fields;
            struct device *dev = ale->params.dev;
            struct regmap *regmap = ale->regmap;
            int i;

            for (i = 0; i < ALE_FIELDS_MAX; i++) {
                    ale->fields[i] = devm_regmap_field_alloc(dev, regmap,
                                                             reg_fields[i]);

                    [...]
            }

            return 0;
    }

I tried fixing this by skipping entries where all of .reg, .lsb,
and .msb are zero, but that doesn't work as that runs beyond the
end of ale_fields_cpsw[], thus operating on random data.
I think you do have to store the size of the array, instead of assuming
ALE_FIELDS_MAX entries everywhere.

> --- a/drivers/net/ethernet/ti/cpsw_ale.h
> +++ b/drivers/net/ethernet/ti/cpsw_ale.h
> @@ -33,6 +34,8 @@ struct regmap;
>  enum ale_fields {
>         MINOR_VER,
>         MAJOR_VER,
> +       ALE_ENTRIES,
> +       ALE_POLICERS,
>         /* terminator */
>         ALE_FIELDS_MAX,
>  };
>

Gr{oetje,eeting}s,

                        Geert
Roger Quadros Sept. 23, 2024, 2:06 p.m. UTC | #2 
Hi Geert,

On 23/09/2024 16:41, Geert Uytterhoeven wrote:
> Hi Roger,
> 
> On Tue, Sep 10, 2024 at 11:25 AM Roger Quadros <rogerq@kernel.org> wrote:
>> Use regfields for number of ALE Entries and Policers.
>>
>> The variants that support Policers/Classifiers have the number
>> of policers encoded in the ALE_STATUS register.
>>
>> Use that and show the number of Policers in the ALE info message.
>>
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> Reviewed-by: Simon Horman <horms@kernel.org>
>> ---
>> Changelog:
>> v4:
>> - reverse Xmas tree declaration order fixes
> 
> Thanks for your patch, which is now commit 11cbcfeaa79e5c76 ("net:
> ethernet: ti: cpsw_ale: use regfields for number of Entries
> and Policers").
> 
> This is causing the following warning on BeagleBone Black:
> 
>     WARNING: CPU: 0 PID: 34 at drivers/base/regmap/regmap.c:1208
> devm_regmap_field_alloc+0xac/0xc8
>     invalid empty mask defined
>     CPU: 0 UID: 0 PID: 34 Comm: kworker/u4:3 Not tainted
> 6.11.0-rc7-boneblack-01443-g11cbcfeaa79e #152
>     Hardware name: Generic AM33XX (Flattened Device Tree)
>     Workqueue: events_unbound deferred_probe_work_func
>     Call trace:
>      unwind_backtrace from show_stack+0x10/0x14
>      show_stack from dump_stack_lvl+0x68/0x88
>      dump_stack_lvl from __warn+0x6c/0x1a8
>      __warn from warn_slowpath_fmt+0x1bc/0x1d0
>      warn_slowpath_fmt from devm_regmap_field_alloc+0xac/0xc8
>      devm_regmap_field_alloc from cpsw_ale_create+0x10c/0x36c
>      cpsw_ale_create from cpsw_init_common+0x1fc/0x310
> 
>> --- a/drivers/net/ethernet/ti/cpsw_ale.c
>> +++ b/drivers/net/ethernet/ti/cpsw_ale.c
>> @@ -1303,6 +1303,9 @@ static const struct reg_field ale_fields_cpsw_nu[] = {
>>         /* CPSW_ALE_IDVER_REG */
>>         [MINOR_VER]     = REG_FIELD(ALE_IDVER, 0, 7),
>>         [MAJOR_VER]     = REG_FIELD(ALE_IDVER, 8, 10),
>> +       /* CPSW_ALE_STATUS_REG */
>> +       [ALE_ENTRIES]   = REG_FIELD(ALE_STATUS, 0, 7),
>> +       [ALE_POLICERS]  = REG_FIELD(ALE_STATUS, 8, 15),
> 
> You are adding these entries only to ale_fields_cpsw_nu[], not
> to ale_fields_cpsw[], while cpsw_ale_regfield_init() loops over
> ALE_FIELDS_MAX entries, whether they are valid or not:
> 
>     static int cpsw_ale_regfield_init(struct cpsw_ale *ale)
>     {
>             const struct reg_field *reg_fields = ale->params.reg_fields;
>             struct device *dev = ale->params.dev;
>             struct regmap *regmap = ale->regmap;
>             int i;
> 
>             for (i = 0; i < ALE_FIELDS_MAX; i++) {
>                     ale->fields[i] = devm_regmap_field_alloc(dev, regmap,
>                                                              reg_fields[i]);
> 
>                     [...]
>             }
> 
>             return 0;
>     }
> 
> I tried fixing this by skipping entries where all of .reg, .lsb,
> and .msb are zero, but that doesn't work as that runs beyond the
> end of ale_fields_cpsw[], thus operating on random data.
> I think you do have to store the size of the array, instead of assuming
> ALE_FIELDS_MAX entries everywhere.

Thanks for the report and suggestion. I will send a fix soon.

> 
>> --- a/drivers/net/ethernet/ti/cpsw_ale.h
>> +++ b/drivers/net/ethernet/ti/cpsw_ale.h
>> @@ -33,6 +34,8 @@ struct regmap;
>>  enum ale_fields {
>>         MINOR_VER,
>>         MAJOR_VER,
>> +       ALE_ENTRIES,
>> +       ALE_POLICERS,
>>         /* terminator */
>>         ALE_FIELDS_MAX,
>>  };
>>
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
index 979f741a231d..9e45470b4eb9 100644
--- a/drivers/net/ethernet/ti/cpsw_ale.c
+++ b/drivers/net/ethernet/ti/cpsw_ale.c
@@ -103,7 +103,7 @@  struct cpsw_ale_dev_id {
 #define ALE_UCAST_TOUCHED		3
 
 #define ALE_TABLE_SIZE_MULTIPLIER	1024
-#define ALE_STATUS_SIZE_MASK		0x1f
+#define ALE_POLICER_SIZE_MULTIPLIER	8
 
 static inline int cpsw_ale_get_field(u32 *ale_entry, u32 start, u32 bits)
 {
@@ -1303,6 +1303,9 @@  static const struct reg_field ale_fields_cpsw_nu[] = {
 	/* CPSW_ALE_IDVER_REG */
 	[MINOR_VER]	= REG_FIELD(ALE_IDVER, 0, 7),
 	[MAJOR_VER]	= REG_FIELD(ALE_IDVER, 8, 10),
+	/* CPSW_ALE_STATUS_REG */
+	[ALE_ENTRIES]	= REG_FIELD(ALE_STATUS, 0, 7),
+	[ALE_POLICERS]	= REG_FIELD(ALE_STATUS, 8, 15),
 };
 
 static const struct cpsw_ale_dev_id cpsw_ale_id_match[] = {
@@ -1402,8 +1405,8 @@  static int cpsw_ale_regfield_init(struct cpsw_ale *ale)
 
 struct cpsw_ale *cpsw_ale_create(struct cpsw_ale_params *params)
 {
+	u32 ale_entries, rev_major, rev_minor, policers;
 	const struct cpsw_ale_dev_id *ale_dev_id;
-	u32 ale_entries, rev_major, rev_minor;
 	struct cpsw_ale *ale;
 	int ret;
 
@@ -1447,9 +1450,7 @@  struct cpsw_ale *cpsw_ale_create(struct cpsw_ale_params *params)
 
 	if (ale->features & CPSW_ALE_F_STATUS_REG &&
 	    !ale->params.ale_entries) {
-		ale_entries =
-			readl_relaxed(ale->params.ale_regs + ALE_STATUS) &
-			ALE_STATUS_SIZE_MASK;
+		regmap_field_read(ale->fields[ALE_ENTRIES], &ale_entries);
 		/* ALE available on newer NetCP switches has introduced
 		 * a register, ALE_STATUS, to indicate the size of ALE
 		 * table which shows the size as a multiple of 1024 entries.
@@ -1463,8 +1464,20 @@  struct cpsw_ale *cpsw_ale_create(struct cpsw_ale_params *params)
 		ale_entries *= ALE_TABLE_SIZE_MULTIPLIER;
 		ale->params.ale_entries = ale_entries;
 	}
+
+	if (ale->features & CPSW_ALE_F_STATUS_REG &&
+	    !ale->params.num_policers) {
+		regmap_field_read(ale->fields[ALE_POLICERS], &policers);
+		if (!policers)
+			return ERR_PTR(-EINVAL);
+
+		policers *= ALE_POLICER_SIZE_MULTIPLIER;
+		ale->params.num_policers = policers;
+	}
+
 	dev_info(ale->params.dev,
-		 "ALE Table size %ld\n", ale->params.ale_entries);
+		 "ALE Table size %ld, Policers %ld\n", ale->params.ale_entries,
+		 ale->params.num_policers);
 
 	/* set default bits for existing h/w */
 	ale->port_mask_bits = ale->params.ale_ports;
diff --git a/drivers/net/ethernet/ti/cpsw_ale.h b/drivers/net/ethernet/ti/cpsw_ale.h
index 58d377dd7496..e12bb2caf016 100644
--- a/drivers/net/ethernet/ti/cpsw_ale.h
+++ b/drivers/net/ethernet/ti/cpsw_ale.h
@@ -15,6 +15,7 @@  struct cpsw_ale_params {
 	void __iomem		*ale_regs;
 	unsigned long		ale_ageout;	/* in secs */
 	unsigned long		ale_entries;
+	unsigned long		num_policers;
 	unsigned long		ale_ports;
 	/* NU Switch has specific handling as number of bits in ALE entries
 	 * are different than other versions of ALE. Also there are specific
@@ -33,6 +34,8 @@  struct regmap;
 enum ale_fields {
 	MINOR_VER,
 	MAJOR_VER,
+	ALE_ENTRIES,
+	ALE_POLICERS,
 	/* terminator */
 	ALE_FIELDS_MAX,
 };