Re: [PATCH net-next v2 2/9] net: ethernet: ti: ale: add static configuration

2020-09-09 Thread Grygorii Strashko




On 09/09/2020 05:28, David Miller wrote:

From: Grygorii Strashko 
Date: Mon, 7 Sep 2020 17:31:36 +0300


+   ale_dev_id = cpsw_ale_match_id(cpsw_ale_id_match, params->dev_id);
+   if (ale_dev_id) {
+   params->ale_entries = ale_dev_id->tbl_entries;
+   params->major_ver_mask = ale_dev_id->major_ver_mask;

...

-   if (!ale->params.major_ver_mask)
-   ale->params.major_ver_mask = 0xff;


This is exactly the kind of change that causes regressions.

The default for the mask if no dev_id is found is now zero, whereas
before the default mask would be 0xff.

Please don't make changes like this, they are very risky.

In every step of these changes, existing behavior should be maintained
as precisely as possible.  Be as conservative as possible.



Sorry and thank you for catching it.
This part belongs to Patch 6. I'll fix it.

--
Best regards,
grygorii


Re: [PATCH net-next v2 2/9] net: ethernet: ti: ale: add static configuration

2020-09-08 Thread David Miller
From: Grygorii Strashko 
Date: Mon, 7 Sep 2020 17:31:36 +0300

> + ale_dev_id = cpsw_ale_match_id(cpsw_ale_id_match, params->dev_id);
> + if (ale_dev_id) {
> + params->ale_entries = ale_dev_id->tbl_entries;
> + params->major_ver_mask = ale_dev_id->major_ver_mask;
...
> - if (!ale->params.major_ver_mask)
> - ale->params.major_ver_mask = 0xff;

This is exactly the kind of change that causes regressions.

The default for the mask if no dev_id is found is now zero, whereas
before the default mask would be 0xff.

Please don't make changes like this, they are very risky.

In every step of these changes, existing behavior should be maintained
as precisely as possible.  Be as conservative as possible.



[PATCH net-next v2 2/9] net: ethernet: ti: ale: add static configuration

2020-09-07 Thread Grygorii Strashko
As existing, as newly introduced CPSW ALE versions have differences in
supported features and ALE table formats. Especially it's actual for the
recent AM65x/J721E/J7200 SoC and feature AM64x, which supports features
like: auto-aging, classifiers, Link aggregation, additional hw filtering,
etc.

Existing ALE configuration interface is not practical in terms of adding
new features and requires consumers to program a lot static parameters. Any
attempt to add new options will case endless adding and maintaining
different combination of flags and options.

Hence CPSW ALE configuration is static and fixed for SoC (or set of SoC) It
is reasonable to add support for static ALE configurations inside ALE
module. This patch adds static ALE configuration table for different ALE
versions and provides option for consumers to select required ALE
configuration by providing ALE const char *dev_id identifier.

This feature is not enabled by default until existing CPSW drivers will be
modified by follow up patches.

Signed-off-by: Grygorii Strashko 
---
 drivers/net/ethernet/ti/cpsw_ale.c | 84 +-
 drivers/net/ethernet/ti/cpsw_ale.h |  1 +
 2 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw_ale.c 
b/drivers/net/ethernet/ti/cpsw_ale.c
index a94aef3f54a5..766197003971 100644
--- a/drivers/net/ethernet/ti/cpsw_ale.c
+++ b/drivers/net/ethernet/ti/cpsw_ale.c
@@ -46,6 +46,29 @@
 
 #define AM65_CPSW_ALE_THREAD_DEF_REG 0x134
 
+enum {
+   CPSW_ALE_F_STATUS_REG = BIT(0), /* Status register present */
+   CPSW_ALE_F_HW_AUTOAGING = BIT(1), /* HW auto aging */
+
+   CPSW_ALE_F_COUNT
+};
+
+/**
+ * struct ale_dev_id - The ALE version/SoC specific configuration
+ * @dev_id: ALE version/SoC id
+ * @features: features supported by ALE
+ * @tbl_entries: number of ALE entries
+ * @major_ver_mask: mask of ALE Major Version Value in ALE_IDVER reg.
+ * @nu_switch_ale: NU Switch ALE
+ */
+struct cpsw_ale_dev_id {
+   const char *dev_id;
+   u32 features;
+   u32 tbl_entries;
+   u32 major_ver_mask;
+   bool nu_switch_ale;
+};
+
 #define ALE_TABLE_WRITEBIT(31)
 
 #define ALE_TYPE_FREE  0
@@ -979,11 +1002,70 @@ void cpsw_ale_stop(struct cpsw_ale *ale)
cpsw_ale_control_set(ale, 0, ALE_ENABLE, 0);
 }
 
+static const struct cpsw_ale_dev_id cpsw_ale_id_match[] = {
+   {
+   /* am3/4/5, dra7. dm814x, 66ak2hk-gbe */
+   .dev_id = "cpsw",
+   .tbl_entries = 1024,
+   .major_ver_mask = 0xff,
+   },
+   {
+   /* 66ak2h_xgbe */
+   .dev_id = "66ak2h-xgbe",
+   .tbl_entries = 2048,
+   .major_ver_mask = 0xff,
+   },
+   {
+   .dev_id = "66ak2el",
+   .features = CPSW_ALE_F_STATUS_REG,
+   .major_ver_mask = 0x7,
+   .nu_switch_ale = true,
+   },
+   {
+   .dev_id = "66ak2g",
+   .features = CPSW_ALE_F_STATUS_REG,
+   .tbl_entries = 64,
+   .major_ver_mask = 0x7,
+   .nu_switch_ale = true,
+   },
+   {
+   .dev_id = "am65x-cpsw2g",
+   .features = CPSW_ALE_F_STATUS_REG | CPSW_ALE_F_HW_AUTOAGING,
+   .tbl_entries = 64,
+   .major_ver_mask = 0x7,
+   .nu_switch_ale = true,
+   },
+   { },
+};
+
+static const struct
+cpsw_ale_dev_id *cpsw_ale_match_id(const struct cpsw_ale_dev_id *id,
+  const char *dev_id)
+{
+   if (!dev_id)
+   return NULL;
+
+   while (id->dev_id) {
+   if (strcmp(dev_id, id->dev_id) == 0)
+   return id;
+   id++;
+   }
+   return NULL;
+}
+
 struct cpsw_ale *cpsw_ale_create(struct cpsw_ale_params *params)
 {
+   const struct cpsw_ale_dev_id *ale_dev_id;
struct cpsw_ale *ale;
u32 rev, ale_entries;
 
+   ale_dev_id = cpsw_ale_match_id(cpsw_ale_id_match, params->dev_id);
+   if (ale_dev_id) {
+   params->ale_entries = ale_dev_id->tbl_entries;
+   params->major_ver_mask = ale_dev_id->major_ver_mask;
+   params->nu_switch_ale = ale_dev_id->nu_switch_ale;
+   }
+
ale = devm_kzalloc(params->dev, sizeof(*ale), GFP_KERNEL);
if (!ale)
return ERR_PTR(-ENOMEM);
@@ -999,8 +1081,6 @@ struct cpsw_ale *cpsw_ale_create(struct cpsw_ale_params 
*params)
ale->ageout = ale->params.ale_ageout * HZ;
 
rev = readl_relaxed(ale->params.ale_regs + ALE_IDVER);
-   if (!ale->params.major_ver_mask)
-   ale->params.major_ver_mask = 0xff;
ale->version =
(ALE_VERSION_MAJOR(rev, ale->params.major_ver_mask) << 8) |
 ALE_VERSION_MINOR(rev);
diff --git a/drivers/net/ethernet/ti/cpsw_ale.h 
b/drivers/net/ethernet/ti/cpsw_ale.h
index 735692f066bf..53ad4246617e 100644