[PATCH net 0/5] net: ipa: minor bug fixes

[PATCH net 0/5] net: ipa: minor bug fixes

[Alex Elder]

This series fixes several bugs.  They are minor, in that the code
currently works on supported platforms even without these patches
applied, but they're bugs nevertheless and should be fixed.

					-Alex

Alex Elder (5):
  net: ipa: assign proper packet context base
  net: ipa: fix resource group field mask definition
  net: ipa: assign endpoint to a resource group
  net: ipa: distinguish between resource group types
  net: ipa: avoid going past end of resource group array

 drivers/net/ipa/ipa_data-sc7180.c |   4 +
 drivers/net/ipa/ipa_data-sdm845.c |   4 +
 drivers/net/ipa/ipa_data.h        |  12 +--
 drivers/net/ipa/ipa_endpoint.c    |  11 +++
 drivers/net/ipa/ipa_main.c        | 121 +++++++++++++++---------------
 drivers/net/ipa/ipa_mem.c         |   2 +-
 drivers/net/ipa/ipa_reg.h         |  48 +++++++++++-
 7 files changed, 136 insertions(+), 66 deletions(-)

-- 
2.20.1

[PATCH net 1/5] net: ipa: assign proper packet context base

[Alex Elder]

At the end of ipa_mem_setup() we write the local packet processing
context base register to tell it where the processing context memory
is.  But we are writing the wrong value.

The value written turns out to be the offset of the modem header
memory region (assigned earlier in the function).  Fix this bug.

Fixes: ba764c4dad7bd ("soc: qcom: ipa: clocking, interrupts, and memory")
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ipa/ipa_mem.c b/drivers/net/ipa/ipa_mem.c
index 2d45c444a67fa..ecfd1f91fce3b 100644
--- a/drivers/net/ipa/ipa_mem.c
+++ b/drivers/net/ipa/ipa_mem.c
@@ -89,7 +89,7 @@ int ipa_mem_setup(struct ipa *ipa)
 	gsi_trans_commit_wait(trans);
 
 	/* Tell the hardware where the processing context area is located */
-	iowrite32(ipa->mem_offset + offset,
+	iowrite32(ipa->mem_offset + ipa->mem[IPA_MEM_MODEM_PROC_CTX].offset,
 		  ipa->reg_virt + IPA_REG_LOCAL_PKT_PROC_CNTXT_BASE_OFFSET);
 
 	return 0;
-- 
2.20.1

[PATCH net 2/5] net: ipa: fix resource group field mask definition

[Alex Elder]

The mask for the RSRC_GRP field in the INIT_RSRC_GRP endpoint
initialization register is incorrectly defined for IPA v4.2 (where
it is only one bit wide).  So we need to fix this.

The fix is not straightforward, however.  Field masks are passed to
functions like u32_encode_bits(), and for that they must be constant.

To address this, we define a new inline function that returns the
*encoded* value to use for a given RSRC_GRP field, which depends on
the IPA version.  The caller can then use something like this, to
assign a given endpoint resource id 1:

    u32 offset = IPA_REG_ENDP_INIT_RSRC_GRP_N_OFFSET(endpoint_id);
    u32 val = rsrc_grp_encoded(ipa->version, 1);

    iowrite32(val, ipa->reg_virt + offset);

The next patch requires this fix.

Fixes: cdf2e9419dd91 ("soc: qcom: ipa: main code")
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_reg.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ipa/ipa_reg.h b/drivers/net/ipa/ipa_reg.h
index e542598fd7759..7dcfa07180f9f 100644
--- a/drivers/net/ipa/ipa_reg.h
+++ b/drivers/net/ipa/ipa_reg.h
@@ -341,7 +341,16 @@ static inline u32 ipa_reg_idle_indication_cfg_offset(enum ipa_version version)
 
 #define IPA_REG_ENDP_INIT_RSRC_GRP_N_OFFSET(ep) \
 					(0x00000838 + 0x0070 * (ep))
-#define RSRC_GRP_FMASK				GENMASK(1, 0)
+/* Encoded value for RSRC_GRP endpoint register RSRC_GRP field */
+static inline u32 rsrc_grp_encoded(enum ipa_version version, u32 rsrc_grp)
+{
+	switch (version) {
+	case IPA_VERSION_4_2:
+		return u32_encode_bits(rsrc_grp, GENMASK(0, 0));
+	default:
+		return u32_encode_bits(rsrc_grp, GENMASK(1, 0));
+	}
+}
 
 /* Valid only for TX (IPA consumer) endpoints */
 #define IPA_REG_ENDP_INIT_SEQ_N_OFFSET(txep) \
-- 
2.20.1

[PATCH net 3/5] net: ipa: assign endpoint to a resource group

[Alex Elder]

The IPA hardware manages various resources (e.g. descriptors)
internally to perform its functions.  The resources are grouped,
allowing different endpoints to use separate resource pools.  This
way one group of endpoints can be configured to operate unaffected
by the resource use of endpoints in a different group.

Endpoints should be assigned to a resource group, but we currently
don't do that.

Define a new resource_group field in the endpoint configuration
data, and use it to assign the proper resource group to use for
each AP endpoint.

Fixes: 84f9bd12d46db ("soc: qcom: ipa: IPA endpoints")
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_data-sc7180.c |  4 ++++
 drivers/net/ipa/ipa_data-sdm845.c |  4 ++++
 drivers/net/ipa/ipa_data.h        |  2 ++
 drivers/net/ipa/ipa_endpoint.c    | 11 +++++++++++
 4 files changed, 21 insertions(+)

diff --git a/drivers/net/ipa/ipa_data-sc7180.c b/drivers/net/ipa/ipa_data-sc7180.c
index d4c2bc7ad24bf..37dada4da6808 100644
--- a/drivers/net/ipa/ipa_data-sc7180.c
+++ b/drivers/net/ipa/ipa_data-sc7180.c
@@ -24,6 +24,7 @@ static const struct ipa_gsi_endpoint_data ipa_gsi_endpoint_data[] = {
 		.endpoint = {
 			.seq_type	= IPA_SEQ_DMA_ONLY,
 			.config = {
+				.resource_group	= 0,
 				.dma_mode	= true,
 				.dma_endpoint	= IPA_ENDPOINT_AP_LAN_RX,
 			},
@@ -42,6 +43,7 @@ static const struct ipa_gsi_endpoint_data ipa_gsi_endpoint_data[] = {
 		.endpoint = {
 			.seq_type	= IPA_SEQ_INVALID,
 			.config = {
+				.resource_group	= 0,
 				.aggregation	= true,
 				.status_enable	= true,
 				.rx = {
@@ -65,6 +67,7 @@ static const struct ipa_gsi_endpoint_data ipa_gsi_endpoint_data[] = {
 			.seq_type	=
 				IPA_SEQ_PKT_PROCESS_NO_DEC_NO_UCP_DMAP,
 			.config = {
+				.resource_group	= 0,
 				.checksum	= true,
 				.qmap		= true,
 				.status_enable	= true,
@@ -88,6 +91,7 @@ static const struct ipa_gsi_endpoint_data ipa_gsi_endpoint_data[] = {
 		.endpoint = {
 			.seq_type	= IPA_SEQ_INVALID,
 			.config = {
+				.resource_group	= 0,
 				.checksum	= true,
 				.qmap		= true,
 				.aggregation	= true,
diff --git a/drivers/net/ipa/ipa_data-sdm845.c b/drivers/net/ipa/ipa_data-sdm845.c
index de2768d71ab56..a9a992404b39f 100644
--- a/drivers/net/ipa/ipa_data-sdm845.c
+++ b/drivers/net/ipa/ipa_data-sdm845.c
@@ -26,6 +26,7 @@ static const struct ipa_gsi_endpoint_data ipa_gsi_endpoint_data[] = {
 		.endpoint = {
 			.seq_type	= IPA_SEQ_DMA_ONLY,
 			.config = {
+				.resource_group	= 1,
 				.dma_mode	= true,
 				.dma_endpoint	= IPA_ENDPOINT_AP_LAN_RX,
 			},
@@ -44,6 +45,7 @@ static const struct ipa_gsi_endpoint_data ipa_gsi_endpoint_data[] = {
 		.endpoint = {
 			.seq_type	= IPA_SEQ_INVALID,
 			.config = {
+				.resource_group	= 1,
 				.aggregation	= true,
 				.status_enable	= true,
 				.rx = {
@@ -67,6 +69,7 @@ static const struct ipa_gsi_endpoint_data ipa_gsi_endpoint_data[] = {
 			.seq_type	=
 				IPA_SEQ_2ND_PKT_PROCESS_PASS_NO_DEC_UCP,
 			.config = {
+				.resource_group	= 1,
 				.checksum	= true,
 				.qmap		= true,
 				.status_enable	= true,
@@ -90,6 +93,7 @@ static const struct ipa_gsi_endpoint_data ipa_gsi_endpoint_data[] = {
 		.endpoint = {
 			.seq_type	= IPA_SEQ_INVALID,
 			.config = {
+				.resource_group	= 1,
 				.checksum	= true,
 				.qmap		= true,
 				.aggregation	= true,
diff --git a/drivers/net/ipa/ipa_data.h b/drivers/net/ipa/ipa_data.h
index 7fc1058a5ca93..d084a83069db2 100644
--- a/drivers/net/ipa/ipa_data.h
+++ b/drivers/net/ipa/ipa_data.h
@@ -109,6 +109,7 @@ struct ipa_endpoint_rx_data {
 
 /**
  * struct ipa_endpoint_config_data - IPA endpoint hardware configuration
+ * @resource_group:	resource group to assign endpoint to
  * @checksum:		whether checksum offload is enabled
  * @qmap:		whether endpoint uses QMAP protocol
  * @aggregation:	whether endpoint supports aggregation
@@ -119,6 +120,7 @@ struct ipa_endpoint_rx_data {
  * @rx:			RX-specific endpoint information (see above)
  */
 struct ipa_endpoint_config_data {
+	u32 resource_group;
 	bool checksum;
 	bool qmap;
 	bool aggregation;
diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index b40b711cf4bd5..7386e10615d99 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -751,6 +751,16 @@ static void ipa_endpoint_init_deaggr(struct ipa_endpoint *endpoint)
 	iowrite32(val, endpoint->ipa->reg_virt + offset);
 }
 
+static void ipa_endpoint_init_rsrc_grp(struct ipa_endpoint *endpoint)
+{
+	u32 offset = IPA_REG_ENDP_INIT_RSRC_GRP_N_OFFSET(endpoint->endpoint_id);
+	struct ipa *ipa = endpoint->ipa;
+	u32 val;
+
+	val = rsrc_grp_encoded(ipa->version, endpoint->data->resource_group);
+	iowrite32(val, ipa->reg_virt + offset);
+}
+
 static void ipa_endpoint_init_seq(struct ipa_endpoint *endpoint)
 {
 	u32 offset = IPA_REG_ENDP_INIT_SEQ_N_OFFSET(endpoint->endpoint_id);
@@ -1328,6 +1338,7 @@ static void ipa_endpoint_program(struct ipa_endpoint *endpoint)
 	ipa_endpoint_init_mode(endpoint);
 	ipa_endpoint_init_aggr(endpoint);
 	ipa_endpoint_init_deaggr(endpoint);
+	ipa_endpoint_init_rsrc_grp(endpoint);
 	ipa_endpoint_init_seq(endpoint);
 	ipa_endpoint_status(endpoint);
 }
-- 
2.20.1

[PATCH net 4/5] net: ipa: distinguish between resource group types

[Alex Elder]

The number of resource groups supported by the hardware can be
different for source and destination resources.  Determine the
number supported for each using separate functions.  Make the
functions inline end move their definitions into "ipa_reg.h",
because they determine whether certain register definitions are
valid.  Pass just the IPA hardware version as argument.

Change IPA_RESOURCE_GROUP_COUNT to be two separate constants, one
for source and the other for destination resource groups.  Rename
those to end with "_MAX" rather than "_COUNT", to reflect their
true purpose.

Fixes: 1ed7d0c0fdbac ("soc: qcom: ipa: configuration data")
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_data.h | 10 +++++-----
 drivers/net/ipa/ipa_main.c | 34 ++++++++++------------------------
 drivers/net/ipa/ipa_reg.h  | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ipa/ipa_data.h b/drivers/net/ipa/ipa_data.h
index d084a83069db2..0225d81d85028 100644
--- a/drivers/net/ipa/ipa_data.h
+++ b/drivers/net/ipa/ipa_data.h
@@ -45,10 +45,10 @@
  * the IPA endpoint.
  */
 
-/* The maximum value returned by ipa_resource_group_count() */
-#define IPA_RESOURCE_GROUP_COUNT	4
+/* The maximum value returned by ipa_resource_group_{src,dst}_count() */
+#define IPA_RESOURCE_GROUP_SRC_MAX	4
+#define IPA_RESOURCE_GROUP_DST_MAX	4
 
-/** enum ipa_resource_type_src - source resource types */
 /**
  * struct gsi_channel_data - GSI channel configuration data
  * @tre_count:		number of TREs in the channel ring
@@ -208,7 +208,7 @@ struct ipa_resource_limits {
  */
 struct ipa_resource_src {
 	enum ipa_resource_type_src type;
-	struct ipa_resource_limits limits[IPA_RESOURCE_GROUP_COUNT];
+	struct ipa_resource_limits limits[IPA_RESOURCE_GROUP_SRC_MAX];
 };
 
 /**
@@ -218,7 +218,7 @@ struct ipa_resource_src {
  */
 struct ipa_resource_dst {
 	enum ipa_resource_type_dst type;
-	struct ipa_resource_limits limits[IPA_RESOURCE_GROUP_COUNT];
+	struct ipa_resource_limits limits[IPA_RESOURCE_GROUP_DST_MAX];
 };
 
 /**
diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index cd4d993b0bbb2..74b1e15ebd6b2 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -363,52 +363,38 @@ static void ipa_hardware_deconfig(struct ipa *ipa)
 
 #ifdef IPA_VALIDATION
 
-/* # IPA resources used based on version (see IPA_RESOURCE_GROUP_COUNT) */
-static int ipa_resource_group_count(struct ipa *ipa)
-{
-	switch (ipa->version) {
-	case IPA_VERSION_3_5_1:
-		return 3;
-
-	case IPA_VERSION_4_0:
-	case IPA_VERSION_4_1:
-		return 4;
-
-	case IPA_VERSION_4_2:
-		return 1;
-
-	default:
-		return 0;
-	}
-}
-
 static bool ipa_resource_limits_valid(struct ipa *ipa,
 				      const struct ipa_resource_data *data)
 {
-	u32 group_count = ipa_resource_group_count(ipa);
+	u32 group_count;
 	u32 i;
 	u32 j;
 
+	group_count = ipa_resource_group_src_count(ipa->version);
 	if (!group_count)
 		return false;
 
-	/* Return an error if a non-zero resource group limit is specified
-	 * for a resource not supported by hardware.
+	/* Return an error if a non-zero resource limit is specified
+	 * for a resource group not supported by hardware.
 	 */
 	for (i = 0; i < data->resource_src_count; i++) {
 		const struct ipa_resource_src *resource;
 
 		resource = &data->resource_src[i];
-		for (j = group_count; j < IPA_RESOURCE_GROUP_COUNT; j++)
+		for (j = group_count; j < IPA_RESOURCE_GROUP_SRC_MAX; j++)
 			if (resource->limits[j].min || resource->limits[j].max)
 				return false;
 	}
 
+	group_count = ipa_resource_group_dst_count(ipa->version);
+	if (!group_count)
+		return false;
+
 	for (i = 0; i < data->resource_dst_count; i++) {
 		const struct ipa_resource_dst *resource;
 
 		resource = &data->resource_dst[i];
-		for (j = group_count; j < IPA_RESOURCE_GROUP_COUNT; j++)
+		for (j = group_count; j < IPA_RESOURCE_GROUP_DST_MAX; j++)
 			if (resource->limits[j].min || resource->limits[j].max)
 				return false;
 	}
diff --git a/drivers/net/ipa/ipa_reg.h b/drivers/net/ipa/ipa_reg.h
index 7dcfa07180f9f..8eaf5f2096270 100644
--- a/drivers/net/ipa/ipa_reg.h
+++ b/drivers/net/ipa/ipa_reg.h
@@ -244,6 +244,43 @@ static inline u32 ipa_reg_idle_indication_cfg_offset(enum ipa_version version)
 #define ENTER_IDLE_DEBOUNCE_THRESH_FMASK	GENMASK(15, 0)
 #define CONST_NON_IDLE_ENABLE_FMASK		GENMASK(16, 16)
 
+/* # IPA source resource groups available based on version */
+static inline u32 ipa_resource_group_src_count(enum ipa_version version)
+{
+	switch (version) {
+	case IPA_VERSION_3_5_1:
+	case IPA_VERSION_4_0:
+	case IPA_VERSION_4_1:
+		return 4;
+
+	case IPA_VERSION_4_2:
+		return 1;
+
+	default:
+		return 0;
+	}
+}
+
+/* # IPA destination resource groups available based on version */
+static inline u32 ipa_resource_group_dst_count(enum ipa_version version)
+{
+	switch (version) {
+	case IPA_VERSION_3_5_1:
+		return 3;
+
+	case IPA_VERSION_4_0:
+	case IPA_VERSION_4_1:
+		return 4;
+
+	case IPA_VERSION_4_2:
+		return 1;
+
+	default:
+		return 0;
+	}
+}
+
+/* Not all of the following are valid (depends on the count, above) */
 #define IPA_REG_SRC_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(rt) \
 					(0x00000400 + 0x0020 * (rt))
 #define IPA_REG_SRC_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(rt) \
-- 
2.20.1

[PATCH net 5/5] net: ipa: avoid going past end of resource group array

[Alex Elder]

The minimum and maximum limits for resources assigned to a given
resource group are programmed in pairs, with the limits for two
groups set in a single register.

If the number of supported resource groups is odd, only half of the
register that defines these limits is valid for the last group; that
group has no second group in the pair.

Currently we ignore this constraint, and it turns out to be harmless,
but it is not guaranteed to be.  This patch addresses that, and adds
support for programming the 5th resource group's limits.

Rework how the resource group limit registers are programmed by
having a single function program all group pairs rather than having
one function program each pair.  Add the programming of the 4-5
resource group pair limits to this function.  If a resource group is
not supported, pass a null pointer to ipa_resource_config_common()
for that group and have that function write zeroes in that case.

Fixes: cdf2e9419dd91 ("soc: qcom: ipa: main code")
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_main.c | 89 +++++++++++++++++++++++---------------
 1 file changed, 53 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index 74b1e15ebd6b2..09c8a16d216df 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -370,8 +370,11 @@ static bool ipa_resource_limits_valid(struct ipa *ipa,
 	u32 i;
 	u32 j;
 
+	/* We program at most 6 source or destination resource group limits */
+	BUILD_BUG_ON(IPA_RESOURCE_GROUP_SRC_MAX > 6);
+
 	group_count = ipa_resource_group_src_count(ipa->version);
-	if (!group_count)
+	if (!group_count || group_count >= IPA_RESOURCE_GROUP_SRC_MAX)
 		return false;
 
 	/* Return an error if a non-zero resource limit is specified
@@ -387,7 +390,7 @@ static bool ipa_resource_limits_valid(struct ipa *ipa,
 	}
 
 	group_count = ipa_resource_group_dst_count(ipa->version);
-	if (!group_count)
+	if (!group_count || group_count >= IPA_RESOURCE_GROUP_DST_MAX)
 		return false;
 
 	for (i = 0; i < data->resource_dst_count; i++) {
@@ -421,46 +424,64 @@ ipa_resource_config_common(struct ipa *ipa, u32 offset,
 
 	val = u32_encode_bits(xlimits->min, X_MIN_LIM_FMASK);
 	val |= u32_encode_bits(xlimits->max, X_MAX_LIM_FMASK);
-	val |= u32_encode_bits(ylimits->min, Y_MIN_LIM_FMASK);
-	val |= u32_encode_bits(ylimits->max, Y_MAX_LIM_FMASK);
+	if (ylimits) {
+		val |= u32_encode_bits(ylimits->min, Y_MIN_LIM_FMASK);
+		val |= u32_encode_bits(ylimits->max, Y_MAX_LIM_FMASK);
+	}
 
 	iowrite32(val, ipa->reg_virt + offset);
 }
 
-static void ipa_resource_config_src_01(struct ipa *ipa,
-				       const struct ipa_resource_src *resource)
+static void ipa_resource_config_src(struct ipa *ipa,
+				    const struct ipa_resource_src *resource)
 {
-	u32 offset = IPA_REG_SRC_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type);
+	u32 group_count = ipa_resource_group_src_count(ipa->version);
+	const struct ipa_resource_limits *ylimits;
+	u32 offset;
 
-	ipa_resource_config_common(ipa, offset,
-				   &resource->limits[0], &resource->limits[1]);
-}
+	offset = IPA_REG_SRC_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type);
+	ylimits = group_count == 1 ? NULL : &resource->limits[1];
+	ipa_resource_config_common(ipa, offset, &resource->limits[0], ylimits);
 
-static void ipa_resource_config_src_23(struct ipa *ipa,
-				       const struct ipa_resource_src *resource)
-{
-	u32 offset = IPA_REG_SRC_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type);
+	if (group_count < 2)
+		return;
 
-	ipa_resource_config_common(ipa, offset,
-				   &resource->limits[2], &resource->limits[3]);
-}
+	offset = IPA_REG_SRC_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type);
+	ylimits = group_count == 3 ? NULL : &resource->limits[3];
+	ipa_resource_config_common(ipa, offset, &resource->limits[2], ylimits);
 
-static void ipa_resource_config_dst_01(struct ipa *ipa,
-				       const struct ipa_resource_dst *resource)
-{
-	u32 offset = IPA_REG_DST_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type);
+	if (group_count < 4)
+		return;
 
-	ipa_resource_config_common(ipa, offset,
-				   &resource->limits[0], &resource->limits[1]);
+	offset = IPA_REG_SRC_RSRC_GRP_45_RSRC_TYPE_N_OFFSET(resource->type);
+	ylimits = group_count == 5 ? NULL : &resource->limits[5];
+	ipa_resource_config_common(ipa, offset, &resource->limits[4], ylimits);
 }
 
-static void ipa_resource_config_dst_23(struct ipa *ipa,
-				       const struct ipa_resource_dst *resource)
+static void ipa_resource_config_dst(struct ipa *ipa,
+				    const struct ipa_resource_dst *resource)
 {
-	u32 offset = IPA_REG_DST_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type);
+	u32 group_count = ipa_resource_group_dst_count(ipa->version);
+	const struct ipa_resource_limits *ylimits;
+	u32 offset;
+
+	offset = IPA_REG_DST_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type);
+	ylimits = group_count == 1 ? NULL : &resource->limits[1];
+	ipa_resource_config_common(ipa, offset, &resource->limits[0], ylimits);
+
+	if (group_count < 2)
+		return;
+
+	offset = IPA_REG_DST_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type);
+	ylimits = group_count == 3 ? NULL : &resource->limits[3];
+	ipa_resource_config_common(ipa, offset, &resource->limits[2], ylimits);
+
+	if (group_count < 4)
+		return;
 
-	ipa_resource_config_common(ipa, offset,
-				   &resource->limits[2], &resource->limits[3]);
+	offset = IPA_REG_DST_RSRC_GRP_45_RSRC_TYPE_N_OFFSET(resource->type);
+	ylimits = group_count == 5 ? NULL : &resource->limits[5];
+	ipa_resource_config_common(ipa, offset, &resource->limits[4], ylimits);
 }
 
 static int
@@ -471,15 +492,11 @@ ipa_resource_config(struct ipa *ipa, const struct ipa_resource_data *data)
 	if (!ipa_resource_limits_valid(ipa, data))
 		return -EINVAL;
 
-	for (i = 0; i < data->resource_src_count; i++) {
-		ipa_resource_config_src_01(ipa, &data->resource_src[i]);
-		ipa_resource_config_src_23(ipa, &data->resource_src[i]);
-	}
+	for (i = 0; i < data->resource_src_count; i++)
+		ipa_resource_config_src(ipa, data->resource_src);
 
-	for (i = 0; i < data->resource_dst_count; i++) {
-		ipa_resource_config_dst_01(ipa, &data->resource_dst[i]);
-		ipa_resource_config_dst_23(ipa, &data->resource_dst[i]);
-	}
+	for (i = 0; i < data->resource_dst_count; i++)
+		ipa_resource_config_dst(ipa, data->resource_dst);
 
 	return 0;
 }
-- 
2.20.1

Re: [PATCH net 5/5] net: ipa: avoid going past end of resource group array

[Willem de Bruijn]

On Tue, Oct 27, 2020 at 12:38 PM Alex Elder <elder@linaro.org> wrote:
>
> The minimum and maximum limits for resources assigned to a given
> resource group are programmed in pairs, with the limits for two
> groups set in a single register.
>
> If the number of supported resource groups is odd, only half of the
> register that defines these limits is valid for the last group; that
> group has no second group in the pair.
>
> Currently we ignore this constraint, and it turns out to be harmless,

If nothing currently calls it with an odd number of registers, is this
a bugfix or a new feature (anticipating future expansion, I guess)?

> but it is not guaranteed to be.  This patch addresses that, and adds
> support for programming the 5th resource group's limits.
>
> Rework how the resource group limit registers are programmed by
> having a single function program all group pairs rather than having
> one function program each pair.  Add the programming of the 4-5
> resource group pair limits to this function.  If a resource group is
> not supported, pass a null pointer to ipa_resource_config_common()
> for that group and have that function write zeroes in that case.
>
> Fixes: cdf2e9419dd91 ("soc: qcom: ipa: main code")
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ipa/ipa_main.c | 89 +++++++++++++++++++++++---------------
>  1 file changed, 53 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index 74b1e15ebd6b2..09c8a16d216df 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c
> @@ -370,8 +370,11 @@ static bool ipa_resource_limits_valid(struct ipa *ipa,
>         u32 i;
>         u32 j;
>
> +       /* We program at most 6 source or destination resource group limits */
> +       BUILD_BUG_ON(IPA_RESOURCE_GROUP_SRC_MAX > 6);
> +
>         group_count = ipa_resource_group_src_count(ipa->version);
> -       if (!group_count)
> +       if (!group_count || group_count >= IPA_RESOURCE_GROUP_SRC_MAX)
>                 return false;

Perhaps more a comment to the previous patch, but _MAX usually denotes
the end of an inclusive range, here 5. The previous name COUNT better
reflects the number of elements in range [0, 5], which is 6.

>         /* Return an error if a non-zero resource limit is specified
> @@ -387,7 +390,7 @@ static bool ipa_resource_limits_valid(struct ipa *ipa,
>         }
>
>         group_count = ipa_resource_group_dst_count(ipa->version);
> -       if (!group_count)
> +       if (!group_count || group_count >= IPA_RESOURCE_GROUP_DST_MAX)
>                 return false;
>
>         for (i = 0; i < data->resource_dst_count; i++) {
> @@ -421,46 +424,64 @@ ipa_resource_config_common(struct ipa *ipa, u32 offset,
>
>         val = u32_encode_bits(xlimits->min, X_MIN_LIM_FMASK);
>         val |= u32_encode_bits(xlimits->max, X_MAX_LIM_FMASK);
> -       val |= u32_encode_bits(ylimits->min, Y_MIN_LIM_FMASK);
> -       val |= u32_encode_bits(ylimits->max, Y_MAX_LIM_FMASK);
> +       if (ylimits) {
> +               val |= u32_encode_bits(ylimits->min, Y_MIN_LIM_FMASK);
> +               val |= u32_encode_bits(ylimits->max, Y_MAX_LIM_FMASK);
> +       }
>
>         iowrite32(val, ipa->reg_virt + offset);
>  }
>
> -static void ipa_resource_config_src_01(struct ipa *ipa,
> -                                      const struct ipa_resource_src *resource)
> +static void ipa_resource_config_src(struct ipa *ipa,
> +                                   const struct ipa_resource_src *resource)
>  {
> -       u32 offset = IPA_REG_SRC_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type);
> +       u32 group_count = ipa_resource_group_src_count(ipa->version);
> +       const struct ipa_resource_limits *ylimits;
> +       u32 offset;
>
> -       ipa_resource_config_common(ipa, offset,
> -                                  &resource->limits[0], &resource->limits[1]);
> -}
> +       offset = IPA_REG_SRC_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type);
> +       ylimits = group_count == 1 ? NULL : &resource->limits[1];
> +       ipa_resource_config_common(ipa, offset, &resource->limits[0], ylimits);
>
> -static void ipa_resource_config_src_23(struct ipa *ipa,
> -                                      const struct ipa_resource_src *resource)
> -{
> -       u32 offset = IPA_REG_SRC_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type);
> +       if (group_count < 2)
> +               return;
>
> -       ipa_resource_config_common(ipa, offset,
> -                                  &resource->limits[2], &resource->limits[3]);
> -}
> +       offset = IPA_REG_SRC_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type);
> +       ylimits = group_count == 3 ? NULL : &resource->limits[3];
> +       ipa_resource_config_common(ipa, offset, &resource->limits[2], ylimits);
>
> -static void ipa_resource_config_dst_01(struct ipa *ipa,
> -                                      const struct ipa_resource_dst *resource)
> -{
> -       u32 offset = IPA_REG_DST_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type);
> +       if (group_count < 4)
> +               return;
>
> -       ipa_resource_config_common(ipa, offset,
> -                                  &resource->limits[0], &resource->limits[1]);
> +       offset = IPA_REG_SRC_RSRC_GRP_45_RSRC_TYPE_N_OFFSET(resource->type);
> +       ylimits = group_count == 5 ? NULL : &resource->limits[5];

Due to the check

> +       if (!group_count || group_count >= IPA_RESOURCE_GROUP_DST_MAX)
>                 return false;

above, group_count can never be greater than 5. Should be greater than?

Re: [PATCH net 5/5] net: ipa: avoid going past end of resource group array

[Alex Elder]

On 10/27/20 7:14 PM, Willem de Bruijn wrote:
> On Tue, Oct 27, 2020 at 12:38 PM Alex Elder <elder@linaro.org> wrote:
>>
>> The minimum and maximum limits for resources assigned to a given
>> resource group are programmed in pairs, with the limits for two
>> groups set in a single register.
>>
>> If the number of supported resource groups is odd, only half of the
>> register that defines these limits is valid for the last group; that
>> group has no second group in the pair.
>>
>> Currently we ignore this constraint, and it turns out to be harmless,
> 
> If nothing currently calls it with an odd number of registers, is this
> a bugfix or a new feature (anticipating future expansion, I guess)?
> 
>> but it is not guaranteed to be.  This patch addresses that, and adds
>> support for programming the 5th resource group's limits.
>>
>> Rework how the resource group limit registers are programmed by
>> having a single function program all group pairs rather than having
>> one function program each pair.  Add the programming of the 4-5
>> resource group pair limits to this function.  If a resource group is
>> not supported, pass a null pointer to ipa_resource_config_common()
>> for that group and have that function write zeroes in that case.
>>
>> Fixes: cdf2e9419dd91 ("soc: qcom: ipa: main code")
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>  drivers/net/ipa/ipa_main.c | 89 +++++++++++++++++++++++---------------
>>  1 file changed, 53 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
>> index 74b1e15ebd6b2..09c8a16d216df 100644
>> --- a/drivers/net/ipa/ipa_main.c
>> +++ b/drivers/net/ipa/ipa_main.c
>> @@ -370,8 +370,11 @@ static bool ipa_resource_limits_valid(struct ipa *ipa,
>>         u32 i;
>>         u32 j;
>>
>> +       /* We program at most 6 source or destination resource group limits */
>> +       BUILD_BUG_ON(IPA_RESOURCE_GROUP_SRC_MAX > 6);
>> +
>>         group_count = ipa_resource_group_src_count(ipa->version);
>> -       if (!group_count)
>> +       if (!group_count || group_count >= IPA_RESOURCE_GROUP_SRC_MAX)
>>                 return false;
> 
> Perhaps more a comment to the previous patch, but _MAX usually denotes
> the end of an inclusive range, here 5. The previous name COUNT better
> reflects the number of elements in range [0, 5], which is 6.

I agree with your point, but the max here represents something different
from what you're expecting.

For a given resource type (source or destination) there is some fixed
number (count) of resources available based on the version of SoC.
The *driver* can handle any number of them up to the maximum number
(max) for any SoC it supports.  In that respect, it *does* represent
the largest value in an inclusive range.

I could change the suffix to something like SRC_COUNT_MAX, but in
general the symbol names are longer than I like in this driver and
I'm trying to shorten them where possible.

If you still want me to change this, please say so and tell me what
you suggest I use instead.

Your observation below about using ">" rather than ">=" is correct,
and applies here as well.  That error might have led you to make
your comment about "max" representing an inclusive maximum.

I will send out a new version of the series to fix that.  I'll
wait until later today to give some time for more feedback before
I prepare them.

Thanks a lot for reviewing this.

					-Alex

>>         /* Return an error if a non-zero resource limit is specified
>> @@ -387,7 +390,7 @@ static bool ipa_resource_limits_valid(struct ipa *ipa,
>>         }
>>
>>         group_count = ipa_resource_group_dst_count(ipa->version);
>> -       if (!group_count)
>> +       if (!group_count || group_count >= IPA_RESOURCE_GROUP_DST_MAX)
>>                 return false;
>>
>>         for (i = 0; i < data->resource_dst_count; i++) {
>> @@ -421,46 +424,64 @@ ipa_resource_config_common(struct ipa *ipa, u32 offset,
>>
>>         val = u32_encode_bits(xlimits->min, X_MIN_LIM_FMASK);
>>         val |= u32_encode_bits(xlimits->max, X_MAX_LIM_FMASK);
>> -       val |= u32_encode_bits(ylimits->min, Y_MIN_LIM_FMASK);
>> -       val |= u32_encode_bits(ylimits->max, Y_MAX_LIM_FMASK);
>> +       if (ylimits) {
>> +               val |= u32_encode_bits(ylimits->min, Y_MIN_LIM_FMASK);
>> +               val |= u32_encode_bits(ylimits->max, Y_MAX_LIM_FMASK);
>> +       }
>>
>>         iowrite32(val, ipa->reg_virt + offset);
>>  }
>>
>> -static void ipa_resource_config_src_01(struct ipa *ipa,
>> -                                      const struct ipa_resource_src *resource)
>> +static void ipa_resource_config_src(struct ipa *ipa,
>> +                                   const struct ipa_resource_src *resource)
>>  {
>> -       u32 offset = IPA_REG_SRC_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type);
>> +       u32 group_count = ipa_resource_group_src_count(ipa->version);
>> +       const struct ipa_resource_limits *ylimits;
>> +       u32 offset;
>>
>> -       ipa_resource_config_common(ipa, offset,
>> -                                  &resource->limits[0], &resource->limits[1]);
>> -}
>> +       offset = IPA_REG_SRC_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type);
>> +       ylimits = group_count == 1 ? NULL : &resource->limits[1];
>> +       ipa_resource_config_common(ipa, offset, &resource->limits[0], ylimits);
>>
>> -static void ipa_resource_config_src_23(struct ipa *ipa,
>> -                                      const struct ipa_resource_src *resource)
>> -{
>> -       u32 offset = IPA_REG_SRC_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type);
>> +       if (group_count < 2)
>> +               return;
>>
>> -       ipa_resource_config_common(ipa, offset,
>> -                                  &resource->limits[2], &resource->limits[3]);
>> -}
>> +       offset = IPA_REG_SRC_RSRC_GRP_23_RSRC_TYPE_N_OFFSET(resource->type);
>> +       ylimits = group_count == 3 ? NULL : &resource->limits[3];
>> +       ipa_resource_config_common(ipa, offset, &resource->limits[2], ylimits);
>>
>> -static void ipa_resource_config_dst_01(struct ipa *ipa,
>> -                                      const struct ipa_resource_dst *resource)
>> -{
>> -       u32 offset = IPA_REG_DST_RSRC_GRP_01_RSRC_TYPE_N_OFFSET(resource->type);
>> +       if (group_count < 4)
>> +               return;
>>
>> -       ipa_resource_config_common(ipa, offset,
>> -                                  &resource->limits[0], &resource->limits[1]);
>> +       offset = IPA_REG_SRC_RSRC_GRP_45_RSRC_TYPE_N_OFFSET(resource->type);
>> +       ylimits = group_count == 5 ? NULL : &resource->limits[5];
> 
> Due to the check
> 
>> +       if (!group_count || group_count >= IPA_RESOURCE_GROUP_DST_MAX)
>>                 return false;
> 
> above, group_count can never be greater than 5. Should be greater than?
> 

Re: [PATCH net 5/5] net: ipa: avoid going past end of resource group array

[Alex Elder]

On 10/27/20 7:14 PM, Willem de Bruijn wrote:
> On Tue, Oct 27, 2020 at 12:38 PM Alex Elder <elder@linaro.org> wrote:
>>
>> The minimum and maximum limits for resources assigned to a given
>> resource group are programmed in pairs, with the limits for two
>> groups set in a single register.
>>
>> If the number of supported resource groups is odd, only half of the
>> register that defines these limits is valid for the last group; that
>> group has no second group in the pair.
>>
>> Currently we ignore this constraint, and it turns out to be harmless,
> 
> If nothing currently calls it with an odd number of registers, is this
> a bugfix or a new feature (anticipating future expansion, I guess)?

. . .

Sorry, I missed this comment.  Yes, I'm working on support for
an upcoming IPA hardware version that has 5 resources of each
type.  And it is a bug fix, though the bug doesn't bite us
(because the maximum number of resources supported is even),
so "it turns out to be harmless."

					-Alex

Re: [PATCH net 5/5] net: ipa: avoid going past end of resource group array

[Willem de Bruijn]

> >> +       /* We program at most 6 source or destination resource group limits */
> >> +       BUILD_BUG_ON(IPA_RESOURCE_GROUP_SRC_MAX > 6);
> >> +
> >>         group_count = ipa_resource_group_src_count(ipa->version);
> >> -       if (!group_count)
> >> +       if (!group_count || group_count >= IPA_RESOURCE_GROUP_SRC_MAX)
> >>                 return false;
> >
> > Perhaps more a comment to the previous patch, but _MAX usually denotes
> > the end of an inclusive range, here 5. The previous name COUNT better
> > reflects the number of elements in range [0, 5], which is 6.
>
> I agree with your point, but the max here represents something different
> from what you're expecting.
>
> For a given resource type (source or destination) there is some fixed
> number (count) of resources available based on the version of SoC.
> The *driver* can handle any number of them up to the maximum number
> (max) for any SoC it supports.  In that respect, it *does* represent
> the largest value in an inclusive range.
>
> I could change the suffix to something like SRC_COUNT_MAX, but in
> general the symbol names are longer than I like in this driver and
> I'm trying to shorten them where possible.

Makes sense. Can you then call this out more explicitly in the commit
message? That MAX here means max count, not max element id.