diff mbox series

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

Message ID 20200907143143.13735-3-grygorii.strashko@ti.com
State Superseded
Headers show
Series net: ethernet: ti: ale: add static configuration | expand

Commit Message

Grygorii Strashko Sept. 7, 2020, 2:31 p.m. UTC
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 <grygorii.strashko@ti.com>

---
 drivers/net/ethernet/ti/cpsw_ale.c | 84 +++++++++++++++++++++++++++++-
 drivers/net/ethernet/ti/cpsw_ale.h |  1 +
 2 files changed, 83 insertions(+), 2 deletions(-)

-- 
2.17.1

Comments

David Miller Sept. 9, 2020, 2:28 a.m. UTC | #1
From: Grygorii Strashko <grygorii.strashko@ti.com>

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.
Grygorii Strashko Sept. 9, 2020, 4:51 p.m. UTC | #2
On 09/09/2020 05:28, David Miller wrote:
> From: Grygorii Strashko <grygorii.strashko@ti.com>

> 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
diff mbox series

Patch

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_WRITE		BIT(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
--- a/drivers/net/ethernet/ti/cpsw_ale.h
+++ b/drivers/net/ethernet/ti/cpsw_ale.h
@@ -24,6 +24,7 @@  struct cpsw_ale_params {
 	 * pass it from caller.
 	 */
 	u32			major_ver_mask;
+	const char		*dev_id;
 };
 
 struct cpsw_ale {