[PATCH v3 net-next 0/5] net: ipa: some more cleanup

[PATCH v3 net-next 0/5] net: ipa: some more cleanup

[Alex Elder]

Version 3 of this series uses dev_err_probe() in the second patch,
as suggested by Heiner Kallweit.

Version 2 was sent to ensure the series was based on current
net-next/master, and added copyright updates to files touched.

The original introduction is below.

					-Alex

This is another fairly innocuous set of cleanup patches.

The first was motivated by a bug found that would affect IPA v4.5.
It maintain a new GSI address pointer; one is the "raw" (original
mapped) address, and the other will have been adjusted if necessary
for use on newer platforms.

The second just quiets some unnecessary noise during early probe.

The third fixes some errors that show up when IPA_VALIDATION is
enabled.

The last two just create helper functions to improve readability.

					-Alex

Alex Elder (5):
  net: ipa: use a separate pointer for adjusted GSI memory
  net: ipa: use dev_err_probe() in ipa_clock.c
  net: ipa: fix register write command validation
  net: ipa: introduce ipa_table_hash_support()
  net: ipa: introduce gsi_channel_initialized()

 drivers/net/ipa/gsi.c       | 50 +++++++++++++++++++------------------
 drivers/net/ipa/gsi.h       |  5 ++--
 drivers/net/ipa/gsi_reg.h   | 21 ++++++++++------
 drivers/net/ipa/ipa_clock.c |  9 ++++---
 drivers/net/ipa/ipa_cmd.c   | 32 ++++++++++++++++++------
 drivers/net/ipa/ipa_table.c | 16 ++++++------
 drivers/net/ipa/ipa_table.h |  8 +++++-
 7 files changed, 87 insertions(+), 54 deletions(-)

-- 
2.20.1

[PATCH v3 net-next 1/5] net: ipa: use a separate pointer for adjusted GSI memory

[Alex Elder]

This patch actually fixes a bug, though it doesn't affect the two
platforms supported currently.  The fix implements GSI memory
pointers a bit differently.

For IPA version 4.5 and above, the address space for almost all GSI
registers is adjusted downward by a fixed amount.  This is currently
handled by adjusting the I/O virtual address pointer after it has
been mapped.  The bug is that the pointer is not "de-adjusted" as it
should be when it's unmapped.

This patch fixes that error, but it does so by maintaining one "raw"
pointer for the mapped memory range.  This is assigned when the
memory is mapped and used to unmap the memory.  This pointer is also
used to access the two registers that do *not* sit in the "adjusted"
memory space.

Rather than adjusting *that* pointer, we maintain a separate pointer
that's an adjusted copy of the "raw" pointer, and that is used for
most GSI register accesses.

Signed-off-by: Alex Elder <elder@linaro.org>
---
v2: - Update copyrights.

 drivers/net/ipa/gsi.c     | 28 ++++++++++++----------------
 drivers/net/ipa/gsi.h     |  5 +++--
 drivers/net/ipa/gsi_reg.h | 21 +++++++++++++--------
 3 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 4402136461888..9c977f80109a9 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 /* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
- * Copyright (C) 2018-2020 Linaro Ltd.
+ * Copyright (C) 2018-2021 Linaro Ltd.
  */
 
 #include <linux/types.h>
@@ -195,8 +195,6 @@ static void gsi_irq_type_disable(struct gsi *gsi, enum gsi_irq_type_id type_id)
 /* Turn off all GSI interrupts initially */
 static void gsi_irq_setup(struct gsi *gsi)
 {
-	u32 adjust;
-
 	/* Disable all interrupt types */
 	gsi_irq_type_update(gsi, 0);
 
@@ -206,10 +204,9 @@ static void gsi_irq_setup(struct gsi *gsi)
 	iowrite32(0, gsi->virt + GSI_CNTXT_GLOB_IRQ_EN_OFFSET);
 	iowrite32(0, gsi->virt + GSI_CNTXT_SRC_IEOB_IRQ_MSK_OFFSET);
 
-	/* Reverse the offset adjustment for inter-EE register offsets */
-	adjust = gsi->version < IPA_VERSION_4_5 ? 0 : GSI_EE_REG_ADJUST;
-	iowrite32(0, gsi->virt + adjust + GSI_INTER_EE_SRC_CH_IRQ_OFFSET);
-	iowrite32(0, gsi->virt + adjust + GSI_INTER_EE_SRC_EV_CH_IRQ_OFFSET);
+	/* The inter-EE registers are in the non-adjusted address range */
+	iowrite32(0, gsi->virt_raw + GSI_INTER_EE_SRC_CH_IRQ_OFFSET);
+	iowrite32(0, gsi->virt_raw + GSI_INTER_EE_SRC_EV_CH_IRQ_OFFSET);
 
 	iowrite32(0, gsi->virt + GSI_CNTXT_GSI_IRQ_EN_OFFSET);
 }
@@ -2181,9 +2178,8 @@ int gsi_init(struct gsi *gsi, struct platform_device *pdev,
 	gsi->dev = dev;
 	gsi->version = version;
 
-	/* The GSI layer performs NAPI on all endpoints.  NAPI requires a
-	 * network device structure, but the GSI layer does not have one,
-	 * so we must create a dummy network device for this purpose.
+	/* GSI uses NAPI on all channels.  Create a dummy network device
+	 * for the channel NAPI contexts to be associated with.
 	 */
 	init_dummy_netdev(&gsi->dummy_dev);
 
@@ -2208,13 +2204,13 @@ int gsi_init(struct gsi *gsi, struct platform_device *pdev,
 		return -EINVAL;
 	}
 
-	gsi->virt = ioremap(res->start, size);
-	if (!gsi->virt) {
+	gsi->virt_raw = ioremap(res->start, size);
+	if (!gsi->virt_raw) {
 		dev_err(dev, "unable to remap \"gsi\" memory\n");
 		return -ENOMEM;
 	}
-	/* Adjust register range pointer downward for newer IPA versions */
-	gsi->virt -= adjust;
+	/* Most registers are accessed using an adjusted register range */
+	gsi->virt = gsi->virt_raw - adjust;
 
 	init_completion(&gsi->completion);
 
@@ -2233,7 +2229,7 @@ int gsi_init(struct gsi *gsi, struct platform_device *pdev,
 err_irq_exit:
 	gsi_irq_exit(gsi);
 err_iounmap:
-	iounmap(gsi->virt);
+	iounmap(gsi->virt_raw);
 
 	return ret;
 }
@@ -2244,7 +2240,7 @@ void gsi_exit(struct gsi *gsi)
 	mutex_destroy(&gsi->mutex);
 	gsi_channel_exit(gsi);
 	gsi_irq_exit(gsi);
-	iounmap(gsi->virt);
+	iounmap(gsi->virt_raw);
 }
 
 /* The maximum number of outstanding TREs on a channel.  This limits
diff --git a/drivers/net/ipa/gsi.h b/drivers/net/ipa/gsi.h
index d674db0ba4eb0..efc980f96109e 100644
--- a/drivers/net/ipa/gsi.h
+++ b/drivers/net/ipa/gsi.h
@@ -1,7 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
 /* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
- * Copyright (C) 2018-2020 Linaro Ltd.
+ * Copyright (C) 2018-2021 Linaro Ltd.
  */
 #ifndef _GSI_H_
 #define _GSI_H_
@@ -149,7 +149,8 @@ struct gsi {
 	struct device *dev;		/* Same as IPA device */
 	enum ipa_version version;
 	struct net_device dummy_dev;	/* needed for NAPI */
-	void __iomem *virt;
+	void __iomem *virt_raw;		/* I/O mapped address range */
+	void __iomem *virt;		/* Adjusted for most registers */
 	u32 irq;
 	u32 channel_count;
 	u32 evt_ring_count;
diff --git a/drivers/net/ipa/gsi_reg.h b/drivers/net/ipa/gsi_reg.h
index 299456e70f286..1622d8cf8dea4 100644
--- a/drivers/net/ipa/gsi_reg.h
+++ b/drivers/net/ipa/gsi_reg.h
@@ -1,7 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
 /* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
- * Copyright (C) 2018-2020 Linaro Ltd.
+ * Copyright (C) 2018-2021 Linaro Ltd.
  */
 #ifndef _GSI_REG_H_
 #define _GSI_REG_H_
@@ -38,17 +38,21 @@
  * (though the actual limit is hardware-dependent).
  */
 
-/* GSI EE registers as a group are shifted downward by a fixed
- * constant amount for IPA versions 4.5 and beyond.  This applies
- * to all GSI registers we use *except* the ones that disable
- * inter-EE interrupts for channels and event channels.
+/* GSI EE registers as a group are shifted downward by a fixed constant amount
+ * for IPA versions 4.5 and beyond.  This applies to all GSI registers we use
+ * *except* the ones that disable inter-EE interrupts for channels and event
+ * channels.
  *
- * We handle this by adjusting the pointer to the mapped GSI memory
- * region downward.  Then in the one place we use them (gsi_irq_setup())
- * we undo that adjustment for the inter-EE interrupt registers.
+ * The "raw" (not adjusted) GSI register range is mapped, and a pointer to
+ * the mapped range is held in gsi->virt_raw.  The inter-EE interrupt
+ * registers are accessed using that pointer.
+ *
+ * Most registers are accessed using gsi->virt, which is a copy of the "raw"
+ * pointer, adjusted downward by the fixed amount.
  */
 #define GSI_EE_REG_ADJUST			0x0000d000	/* IPA v4.5+ */
 
+/* The two inter-EE IRQ register offsets are relative to gsi->virt_raw */
 #define GSI_INTER_EE_SRC_CH_IRQ_OFFSET \
 			GSI_INTER_EE_N_SRC_CH_IRQ_OFFSET(GSI_EE_AP)
 #define GSI_INTER_EE_N_SRC_CH_IRQ_OFFSET(ee) \
@@ -59,6 +63,7 @@
 #define GSI_INTER_EE_N_SRC_EV_CH_IRQ_OFFSET(ee) \
 			(0x0000c01c + 0x1000 * (ee))
 
+/* All other register offsets are relative to gsi->virt */
 #define GSI_CH_C_CNTXT_0_OFFSET(ch) \
 		GSI_EE_N_CH_C_CNTXT_0_OFFSET((ch), GSI_EE_AP)
 #define GSI_EE_N_CH_C_CNTXT_0_OFFSET(ch, ee) \
-- 
2.20.1

[PATCH v3 net-next 2/5] net: ipa: use dev_err_probe() in ipa_clock.c

[Alex Elder]

When initializing the IPA core clock and interconnects, it's
possible we'll get an EPROBE_DEFER error.  This isn't really an
error, it's just means we need to be re-probed later.

Use dev_err_probe() to report the error rather than dev_err().
This avoids polluting the log with these "error" messages.

Signed-off-by: Alex Elder <elder@linaro.org>
---
v3: - Use dev_err_probe(), as suggested by Heiner Kallweit.
v2: - Update copyright.

 drivers/net/ipa/ipa_clock.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index 354675a643db5..69ef6ea41e619 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 /* Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
- * Copyright (C) 2018-2020 Linaro Ltd.
+ * Copyright (C) 2018-2021 Linaro Ltd.
  */
 
 #include <linux/refcount.h>
@@ -68,8 +68,8 @@ static int ipa_interconnect_init_one(struct device *dev,
 	if (IS_ERR(path)) {
 		int ret = PTR_ERR(path);
 
-		dev_err(dev, "error %d getting %s interconnect\n", ret,
-			data->name);
+		dev_err_probe(dev, ret, "error getting %s interconnect\n",
+			      data->name);
 
 		return ret;
 	}
@@ -281,7 +281,8 @@ ipa_clock_init(struct device *dev, const struct ipa_clock_data *data)
 
 	clk = clk_get(dev, "core");
 	if (IS_ERR(clk)) {
-		dev_err(dev, "error %ld getting core clock\n", PTR_ERR(clk));
+		dev_err_probe(dev, PTR_ERR(clk), "error getting core clock\n");
+
 		return ERR_CAST(clk);
 	}
 
-- 
2.20.1

[PATCH v3 net-next 3/5] net: ipa: fix register write command validation

[Alex Elder]

In ipa_cmd_register_write_valid() we verify that values we will
supply to a REGISTER_WRITE IPA immediate command will fit in
the fields that need to hold them.  This patch fixes some issues
in that function and ipa_cmd_register_write_offset_valid().

The dev_err() call in ipa_cmd_register_write_offset_valid() has
some printf format errors:
  - The name of the register (corresponding to the string format
    specifier) was not supplied.
  - The IPA base offset and offset need to be supplied separately to
    match the other format specifiers.
Also make the ~0 constant used there to compute the maximum
supported offset value explicitly unsigned.

There are two other issues in ipa_cmd_register_write_valid():
  - There's no need to check the hash flush register for platforms
    (like IPA v4.2) that do not support hashed tables
  - The highest possible endpoint number, whose status register
    offset is computed, is COUNT - 1, not COUNT.

Fix these problems, and add some additional commentary.

Signed-off-by: Alex Elder <elder@linaro.org>
---
v2: - Update copyright.

 drivers/net/ipa/ipa_cmd.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ipa/ipa_cmd.c b/drivers/net/ipa/ipa_cmd.c
index 97b50fee60089..fd8bf6468d313 100644
--- a/drivers/net/ipa/ipa_cmd.c
+++ b/drivers/net/ipa/ipa_cmd.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 /* Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
- * Copyright (C) 2019-2020 Linaro Ltd.
+ * Copyright (C) 2019-2021 Linaro Ltd.
  */
 
 #include <linux/types.h>
@@ -244,11 +244,15 @@ static bool ipa_cmd_register_write_offset_valid(struct ipa *ipa,
 	if (ipa->version != IPA_VERSION_3_5_1)
 		bit_count += hweight32(REGISTER_WRITE_FLAGS_OFFSET_HIGH_FMASK);
 	BUILD_BUG_ON(bit_count > 32);
-	offset_max = ~0 >> (32 - bit_count);
+	offset_max = ~0U >> (32 - bit_count);
 
+	/* Make sure the offset can be represented by the field(s)
+	 * that holds it.  Also make sure the offset is not outside
+	 * the overall IPA memory range.
+	 */
 	if (offset > offset_max || ipa->mem_offset > offset_max - offset) {
 		dev_err(dev, "%s offset too large 0x%04x + 0x%04x > 0x%04x)\n",
-				ipa->mem_offset + offset, offset_max);
+			name, ipa->mem_offset, offset, offset_max);
 		return false;
 	}
 
@@ -261,12 +265,24 @@ static bool ipa_cmd_register_write_valid(struct ipa *ipa)
 	const char *name;
 	u32 offset;
 
-	offset = ipa_reg_filt_rout_hash_flush_offset(ipa->version);
-	name = "filter/route hash flush";
-	if (!ipa_cmd_register_write_offset_valid(ipa, name, offset))
-		return false;
+	/* If hashed tables are supported, ensure the hash flush register
+	 * offset will fit in a register write IPA immediate command.
+	 */
+	if (ipa->version != IPA_VERSION_4_2) {
+		offset = ipa_reg_filt_rout_hash_flush_offset(ipa->version);
+		name = "filter/route hash flush";
+		if (!ipa_cmd_register_write_offset_valid(ipa, name, offset))
+			return false;
+	}
 
-	offset = IPA_REG_ENDP_STATUS_N_OFFSET(IPA_ENDPOINT_COUNT);
+	/* Each endpoint can have a status endpoint associated with it,
+	 * and this is recorded in an endpoint register.  If the modem
+	 * crashes, we reset the status endpoint for all modem endpoints
+	 * using a register write IPA immediate command.  Make sure the
+	 * worst case (highest endpoint number) offset of that endpoint
+	 * fits in the register write command field(s) that must hold it.
+	 */
+	offset = IPA_REG_ENDP_STATUS_N_OFFSET(IPA_ENDPOINT_COUNT - 1);
 	name = "maximal endpoint status";
 	if (!ipa_cmd_register_write_offset_valid(ipa, name, offset))
 		return false;
-- 
2.20.1

[PATCH v3 net-next 4/5] net: ipa: introduce ipa_table_hash_support()

[Alex Elder]

Introduce a new function to abstract the knowledge of whether hashed
routing and filter tables are supported for a given IPA instance.

IPA v4.2 is the only one that doesn't support hashed tables (now
and for the foreseeable future), but the name of the helper function
is better for explaining what's going on.

Signed-off-by: Alex Elder <elder@linaro.org>
---
v2: - Update copyrights.

 drivers/net/ipa/ipa_cmd.c   |  2 +-
 drivers/net/ipa/ipa_table.c | 16 +++++++++-------
 drivers/net/ipa/ipa_table.h |  8 +++++++-
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ipa/ipa_cmd.c b/drivers/net/ipa/ipa_cmd.c
index fd8bf6468d313..35e35852c25c5 100644
--- a/drivers/net/ipa/ipa_cmd.c
+++ b/drivers/net/ipa/ipa_cmd.c
@@ -268,7 +268,7 @@ static bool ipa_cmd_register_write_valid(struct ipa *ipa)
 	/* If hashed tables are supported, ensure the hash flush register
 	 * offset will fit in a register write IPA immediate command.
 	 */
-	if (ipa->version != IPA_VERSION_4_2) {
+	if (ipa_table_hash_support(ipa)) {
 		offset = ipa_reg_filt_rout_hash_flush_offset(ipa->version);
 		name = "filter/route hash flush";
 		if (!ipa_cmd_register_write_offset_valid(ipa, name, offset))
diff --git a/drivers/net/ipa/ipa_table.c b/drivers/net/ipa/ipa_table.c
index 32e2d3e052d55..baaab3dd0e63c 100644
--- a/drivers/net/ipa/ipa_table.c
+++ b/drivers/net/ipa/ipa_table.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 /* Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
- * Copyright (C) 2018-2020 Linaro Ltd.
+ * Copyright (C) 2018-2021 Linaro Ltd.
  */
 
 #include <linux/types.h>
@@ -239,6 +239,11 @@ static void ipa_table_validate_build(void)
 
 #endif /* !IPA_VALIDATE */
 
+bool ipa_table_hash_support(struct ipa *ipa)
+{
+	return ipa->version != IPA_VERSION_4_2;
+}
+
 /* Zero entry count means no table, so just return a 0 address */
 static dma_addr_t ipa_table_addr(struct ipa *ipa, bool filter_mask, u16 count)
 {
@@ -412,8 +417,7 @@ int ipa_table_hash_flush(struct ipa *ipa)
 	struct gsi_trans *trans;
 	u32 val;
 
-	/* IPA version 4.2 does not support hashed tables */
-	if (ipa->version == IPA_VERSION_4_2)
+	if (!ipa_table_hash_support(ipa))
 		return 0;
 
 	trans = ipa_cmd_trans_alloc(ipa, 1);
@@ -531,8 +535,7 @@ static void ipa_filter_config(struct ipa *ipa, bool modem)
 	enum gsi_ee_id ee_id = modem ? GSI_EE_MODEM : GSI_EE_AP;
 	u32 ep_mask = ipa->filter_map;
 
-	/* IPA version 4.2 has no hashed route tables */
-	if (ipa->version == IPA_VERSION_4_2)
+	if (!ipa_table_hash_support(ipa))
 		return;
 
 	while (ep_mask) {
@@ -582,8 +585,7 @@ static void ipa_route_config(struct ipa *ipa, bool modem)
 {
 	u32 route_id;
 
-	/* IPA version 4.2 has no hashed route tables */
-	if (ipa->version == IPA_VERSION_4_2)
+	if (!ipa_table_hash_support(ipa))
 		return;
 
 	for (route_id = 0; route_id < IPA_ROUTE_COUNT_MAX; route_id++)
diff --git a/drivers/net/ipa/ipa_table.h b/drivers/net/ipa/ipa_table.h
index 78038d14fcea9..1a68d20f19d6a 100644
--- a/drivers/net/ipa/ipa_table.h
+++ b/drivers/net/ipa/ipa_table.h
@@ -1,7 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
 /* Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
- * Copyright (C) 2019-2020 Linaro Ltd.
+ * Copyright (C) 2019-2021 Linaro Ltd.
  */
 #ifndef _IPA_TABLE_H_
 #define _IPA_TABLE_H_
@@ -51,6 +51,12 @@ static inline bool ipa_filter_map_valid(struct ipa *ipa, u32 filter_mask)
 
 #endif /* !IPA_VALIDATE */
 
+/**
+ * ipa_table_hash_support() - Return true if hashed tables are supported
+ * @ipa:	IPA pointer
+ */
+bool ipa_table_hash_support(struct ipa *ipa);
+
 /**
  * ipa_table_reset() - Reset filter and route tables entries to "none"
  * @ipa:	IPA pointer
-- 
2.20.1

Re: [PATCH v3 net-next 4/5] net: ipa: introduce ipa_table_hash_support()

[Alexander Duyck]

On Fri, Feb 12, 2021 at 6:40 AM Alex Elder <elder@linaro.org> wrote:
>
> Introduce a new function to abstract the knowledge of whether hashed
> routing and filter tables are supported for a given IPA instance.
>
> IPA v4.2 is the only one that doesn't support hashed tables (now
> and for the foreseeable future), but the name of the helper function
> is better for explaining what's going on.
>
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
> v2: - Update copyrights.
>
>  drivers/net/ipa/ipa_cmd.c   |  2 +-
>  drivers/net/ipa/ipa_table.c | 16 +++++++++-------
>  drivers/net/ipa/ipa_table.h |  8 +++++++-
>  3 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ipa/ipa_cmd.c b/drivers/net/ipa/ipa_cmd.c
> index fd8bf6468d313..35e35852c25c5 100644
> --- a/drivers/net/ipa/ipa_cmd.c
> +++ b/drivers/net/ipa/ipa_cmd.c
> @@ -268,7 +268,7 @@ static bool ipa_cmd_register_write_valid(struct ipa *ipa)
>         /* If hashed tables are supported, ensure the hash flush register
>          * offset will fit in a register write IPA immediate command.
>          */
> -       if (ipa->version != IPA_VERSION_4_2) {
> +       if (ipa_table_hash_support(ipa)) {
>                 offset = ipa_reg_filt_rout_hash_flush_offset(ipa->version);
>                 name = "filter/route hash flush";
>                 if (!ipa_cmd_register_write_offset_valid(ipa, name, offset))
> diff --git a/drivers/net/ipa/ipa_table.c b/drivers/net/ipa/ipa_table.c
> index 32e2d3e052d55..baaab3dd0e63c 100644
> --- a/drivers/net/ipa/ipa_table.c
> +++ b/drivers/net/ipa/ipa_table.c
> @@ -1,7 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>
>  /* Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
> - * Copyright (C) 2018-2020 Linaro Ltd.
> + * Copyright (C) 2018-2021 Linaro Ltd.
>   */
>
>  #include <linux/types.h>
> @@ -239,6 +239,11 @@ static void ipa_table_validate_build(void)
>
>  #endif /* !IPA_VALIDATE */
>
> +bool ipa_table_hash_support(struct ipa *ipa)
> +{
> +       return ipa->version != IPA_VERSION_4_2;
> +}
> +

Since this is only a single comparison it might make more sense to
make this a static inline and place it in ipa.h. Otherwise you are
just bloating the code up to jump to such a small function.

>  /* Zero entry count means no table, so just return a 0 address */
>  static dma_addr_t ipa_table_addr(struct ipa *ipa, bool filter_mask, u16 count)
>  {
> @@ -412,8 +417,7 @@ int ipa_table_hash_flush(struct ipa *ipa)
>         struct gsi_trans *trans;
>         u32 val;
>
> -       /* IPA version 4.2 does not support hashed tables */
> -       if (ipa->version == IPA_VERSION_4_2)
> +       if (!ipa_table_hash_support(ipa))
>                 return 0;
>
>         trans = ipa_cmd_trans_alloc(ipa, 1);
> @@ -531,8 +535,7 @@ static void ipa_filter_config(struct ipa *ipa, bool modem)
>         enum gsi_ee_id ee_id = modem ? GSI_EE_MODEM : GSI_EE_AP;
>         u32 ep_mask = ipa->filter_map;
>
> -       /* IPA version 4.2 has no hashed route tables */
> -       if (ipa->version == IPA_VERSION_4_2)
> +       if (!ipa_table_hash_support(ipa))
>                 return;
>
>         while (ep_mask) {
> @@ -582,8 +585,7 @@ static void ipa_route_config(struct ipa *ipa, bool modem)
>  {
>         u32 route_id;
>
> -       /* IPA version 4.2 has no hashed route tables */
> -       if (ipa->version == IPA_VERSION_4_2)
> +       if (!ipa_table_hash_support(ipa))
>                 return;
>
>         for (route_id = 0; route_id < IPA_ROUTE_COUNT_MAX; route_id++)
> diff --git a/drivers/net/ipa/ipa_table.h b/drivers/net/ipa/ipa_table.h
> index 78038d14fcea9..1a68d20f19d6a 100644
> --- a/drivers/net/ipa/ipa_table.h
> +++ b/drivers/net/ipa/ipa_table.h
> @@ -1,7 +1,7 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>
>  /* Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
> - * Copyright (C) 2019-2020 Linaro Ltd.
> + * Copyright (C) 2019-2021 Linaro Ltd.
>   */
>  #ifndef _IPA_TABLE_H_
>  #define _IPA_TABLE_H_
> @@ -51,6 +51,12 @@ static inline bool ipa_filter_map_valid(struct ipa *ipa, u32 filter_mask)
>
>  #endif /* !IPA_VALIDATE */
>
> +/**
> + * ipa_table_hash_support() - Return true if hashed tables are supported
> + * @ipa:       IPA pointer
> + */
> +bool ipa_table_hash_support(struct ipa *ipa);
> +
>  /**
>   * ipa_table_reset() - Reset filter and route tables entries to "none"
>   * @ipa:       IPA pointer

Just define the function here and make it a static inline.

Re: [PATCH v3 net-next 4/5] net: ipa: introduce ipa_table_hash_support()

[Alex Elder]

On 2/12/21 7:05 PM, Alexander Duyck wrote:
> On Fri, Feb 12, 2021 at 6:40 AM Alex Elder <elder@linaro.org> wrote:
>>
>> Introduce a new function to abstract the knowledge of whether hashed
>> routing and filter tables are supported for a given IPA instance.
>>
>> IPA v4.2 is the only one that doesn't support hashed tables (now
>> and for the foreseeable future), but the name of the helper function
>> is better for explaining what's going on.
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>> v2: - Update copyrights.
>>
>>   drivers/net/ipa/ipa_cmd.c   |  2 +-
>>   drivers/net/ipa/ipa_table.c | 16 +++++++++-------
>>   drivers/net/ipa/ipa_table.h |  8 +++++++-
>>   3 files changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ipa/ipa_cmd.c b/drivers/net/ipa/ipa_cmd.c
>> index fd8bf6468d313..35e35852c25c5 100644
>> --- a/drivers/net/ipa/ipa_cmd.c
>> +++ b/drivers/net/ipa/ipa_cmd.c
>> @@ -268,7 +268,7 @@ static bool ipa_cmd_register_write_valid(struct ipa *ipa)
>>          /* If hashed tables are supported, ensure the hash flush register
>>           * offset will fit in a register write IPA immediate command.
>>           */
>> -       if (ipa->version != IPA_VERSION_4_2) {
>> +       if (ipa_table_hash_support(ipa)) {
>>                  offset = ipa_reg_filt_rout_hash_flush_offset(ipa->version);
>>                  name = "filter/route hash flush";
>>                  if (!ipa_cmd_register_write_offset_valid(ipa, name, offset))
>> diff --git a/drivers/net/ipa/ipa_table.c b/drivers/net/ipa/ipa_table.c
>> index 32e2d3e052d55..baaab3dd0e63c 100644
>> --- a/drivers/net/ipa/ipa_table.c
>> +++ b/drivers/net/ipa/ipa_table.c
>> @@ -1,7 +1,7 @@
>>   // SPDX-License-Identifier: GPL-2.0
>>
>>   /* Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
>> - * Copyright (C) 2018-2020 Linaro Ltd.
>> + * Copyright (C) 2018-2021 Linaro Ltd.
>>    */
>>
>>   #include <linux/types.h>
>> @@ -239,6 +239,11 @@ static void ipa_table_validate_build(void)
>>
>>   #endif /* !IPA_VALIDATE */
>>
>> +bool ipa_table_hash_support(struct ipa *ipa)
>> +{
>> +       return ipa->version != IPA_VERSION_4_2;
>> +}
>> +
> 
> Since this is only a single comparison it might make more sense to
> make this a static inline and place it in ipa.h. Otherwise you are
> just bloating the code up to jump to such a small function.

Static inline will duplicate the function everywhere also,
"bloating" it in another way (but it's a fairly trivial
amount of code either way).

Nevertheless I agree with your sentiment.  It's moot now
because the series was just accepted, but I'll keep it
in mind.

Thanks for your input.

					-Alex

>>   /* Zero entry count means no table, so just return a 0 address */
>>   static dma_addr_t ipa_table_addr(struct ipa *ipa, bool filter_mask, u16 count)
>>   {
>> @@ -412,8 +417,7 @@ int ipa_table_hash_flush(struct ipa *ipa)
>>          struct gsi_trans *trans;
>>          u32 val;
>>
>> -       /* IPA version 4.2 does not support hashed tables */
>> -       if (ipa->version == IPA_VERSION_4_2)
>> +       if (!ipa_table_hash_support(ipa))
>>                  return 0;
>>
>>          trans = ipa_cmd_trans_alloc(ipa, 1);
>> @@ -531,8 +535,7 @@ static void ipa_filter_config(struct ipa *ipa, bool modem)
>>          enum gsi_ee_id ee_id = modem ? GSI_EE_MODEM : GSI_EE_AP;
>>          u32 ep_mask = ipa->filter_map;
>>
>> -       /* IPA version 4.2 has no hashed route tables */
>> -       if (ipa->version == IPA_VERSION_4_2)
>> +       if (!ipa_table_hash_support(ipa))
>>                  return;
>>
>>          while (ep_mask) {
>> @@ -582,8 +585,7 @@ static void ipa_route_config(struct ipa *ipa, bool modem)
>>   {
>>          u32 route_id;
>>
>> -       /* IPA version 4.2 has no hashed route tables */
>> -       if (ipa->version == IPA_VERSION_4_2)
>> +       if (!ipa_table_hash_support(ipa))
>>                  return;
>>
>>          for (route_id = 0; route_id < IPA_ROUTE_COUNT_MAX; route_id++)
>> diff --git a/drivers/net/ipa/ipa_table.h b/drivers/net/ipa/ipa_table.h
>> index 78038d14fcea9..1a68d20f19d6a 100644
>> --- a/drivers/net/ipa/ipa_table.h
>> +++ b/drivers/net/ipa/ipa_table.h
>> @@ -1,7 +1,7 @@
>>   /* SPDX-License-Identifier: GPL-2.0 */
>>
>>   /* Copyright (c) 2012-2018, The Linux Foundation. All rights reserved.
>> - * Copyright (C) 2019-2020 Linaro Ltd.
>> + * Copyright (C) 2019-2021 Linaro Ltd.
>>    */
>>   #ifndef _IPA_TABLE_H_
>>   #define _IPA_TABLE_H_
>> @@ -51,6 +51,12 @@ static inline bool ipa_filter_map_valid(struct ipa *ipa, u32 filter_mask)
>>
>>   #endif /* !IPA_VALIDATE */
>>
>> +/**
>> + * ipa_table_hash_support() - Return true if hashed tables are supported
>> + * @ipa:       IPA pointer
>> + */
>> +bool ipa_table_hash_support(struct ipa *ipa);
>> +
>>   /**
>>    * ipa_table_reset() - Reset filter and route tables entries to "none"
>>    * @ipa:       IPA pointer
> 
> Just define the function here and make it a static inline.
> 

[PATCH v3 net-next 5/5] net: ipa: introduce gsi_channel_initialized()

[Alex Elder]

Create a simple helper function that indicates whether a channel has
been initialized.  This abstacts/hides the details of how this is
determined.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/gsi.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 9c977f80109a9..390d3403386aa 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -175,6 +175,12 @@ static u32 gsi_channel_id(struct gsi_channel *channel)
 	return channel - &channel->gsi->channel[0];
 }
 
+/* An initialized channel has a non-null GSI pointer */
+static bool gsi_channel_initialized(struct gsi_channel *channel)
+{
+	return !!channel->gsi;
+}
+
 /* Update the GSI IRQ type register with the cached value */
 static void gsi_irq_type_update(struct gsi *gsi, u32 val)
 {
@@ -1638,8 +1644,8 @@ static int gsi_channel_setup_one(struct gsi *gsi, u32 channel_id)
 	u32 evt_ring_id = channel->evt_ring_id;
 	int ret;
 
-	if (!channel->gsi)
-		return 0;	/* Ignore uninitialized channels */
+	if (!gsi_channel_initialized(channel))
+		return 0;
 
 	ret = gsi_evt_ring_alloc_command(gsi, evt_ring_id);
 	if (ret)
@@ -1675,8 +1681,8 @@ static void gsi_channel_teardown_one(struct gsi *gsi, u32 channel_id)
 	struct gsi_channel *channel = &gsi->channel[channel_id];
 	u32 evt_ring_id = channel->evt_ring_id;
 
-	if (!channel->gsi)
-		return;		/* Ignore uninitialized channels */
+	if (!gsi_channel_initialized(channel))
+		return;
 
 	netif_napi_del(&channel->napi);
 
@@ -1770,8 +1776,8 @@ static int gsi_channel_setup(struct gsi *gsi)
 	while (channel_id < GSI_CHANNEL_COUNT_MAX) {
 		struct gsi_channel *channel = &gsi->channel[channel_id++];
 
-		if (!channel->gsi)
-			continue;	/* Ignore uninitialized channels */
+		if (!gsi_channel_initialized(channel))
+			continue;
 
 		ret = -EINVAL;
 		dev_err(gsi->dev, "channel %u not supported by hardware\n",
@@ -2089,8 +2095,8 @@ static int gsi_channel_init_one(struct gsi *gsi,
 /* Inverse of gsi_channel_init_one() */
 static void gsi_channel_exit_one(struct gsi_channel *channel)
 {
-	if (!channel->gsi)
-		return;		/* Ignore uninitialized channels */
+	if (!gsi_channel_initialized(channel))
+		return;
 
 	if (channel->command)
 		ipa_cmd_pool_exit(channel);
-- 
2.20.1

Re: [PATCH v3 net-next 0/5] net: ipa: some more cleanup

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Fri, 12 Feb 2021 08:33:57 -0600 you wrote:
> Version 3 of this series uses dev_err_probe() in the second patch,
> as suggested by Heiner Kallweit.
> 
> Version 2 was sent to ensure the series was based on current
> net-next/master, and added copyright updates to files touched.
> 
> The original introduction is below.
> 
> [...]

Here is the summary with links:
  - [v3,net-next,1/5] net: ipa: use a separate pointer for adjusted GSI memory
    https://git.kernel.org/netdev/net-next/c/571b1e7e58ad
  - [v3,net-next,2/5] net: ipa: use dev_err_probe() in ipa_clock.c
    https://git.kernel.org/netdev/net-next/c/4c7ccfcd09fd
  - [v3,net-next,3/5] net: ipa: fix register write command validation
    https://git.kernel.org/netdev/net-next/c/2d65ed76924b
  - [v3,net-next,4/5] net: ipa: introduce ipa_table_hash_support()
    https://git.kernel.org/netdev/net-next/c/a266ad6b5deb
  - [v3,net-next,5/5] net: ipa: introduce gsi_channel_initialized()
    https://git.kernel.org/netdev/net-next/c/6170b6dab2d4

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html