[PATCH net-next 0/4] net: ipa: support 32-bit targets

[PATCH net-next 0/4] net: ipa: support 32-bit targets

[Alex Elder]

There is currently a configuration dependency that restricts IPA to
be supported only on 64-bit machines.  There are only a few things
that really require that, and those are fixed in this series.  The
last patch in the series removes the CONFIG_64BIT build dependency
for IPA.

					-Alex

Alex Elder (4):
  net: ipa: fix assumptions about DMA address size
  net: ipa: introduce dma_addr_high32()
  net: ipa: fix table alignment requirement
  net: ipa: relax 64-bit build requirement

 drivers/net/ipa/Kconfig     |  2 +-
 drivers/net/ipa/gsi.c       | 14 ++++++++++++--
 drivers/net/ipa/ipa_main.c  | 10 ++++++++--
 drivers/net/ipa/ipa_table.c | 34 ++++++++++++++++++++--------------
 4 files changed, 41 insertions(+), 19 deletions(-)

-- 
2.27.0

[PATCH net-next 1/4] net: ipa: fix assumptions about DMA address size

[Alex Elder]

Some build time checks in ipa_table_validate_build() assume that a
DMA address is 64 bits wide.  That is more restrictive than it has
to be.  A route or filter table is 64 bits wide no matter what the
size of a DMA address is on the AP.  The code actually uses a
pointer to __le64 to access table entries, and a fixed constant
IPA_TABLE_ENTRY_SIZE to describe the size of those entries.

Loosen up two checks so they still verify some requirements, but
such that they do not assume the size of a DMA address is 64 bits.

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

diff --git a/drivers/net/ipa/ipa_table.c b/drivers/net/ipa/ipa_table.c
index 7450e27068f19..dd07fe9dd87a3 100644
--- a/drivers/net/ipa/ipa_table.c
+++ b/drivers/net/ipa/ipa_table.c
@@ -126,13 +126,15 @@ static void ipa_table_validate_build(void)
 	 */
 	BUILD_BUG_ON(ARCH_DMA_MINALIGN % IPA_TABLE_ALIGN);
 
-	/* Filter and route tables contain DMA addresses that refer to
-	 * filter or route rules.  We use a fixed constant to represent
-	 * the size of either type of table entry.  Code in ipa_table_init()
-	 * uses a pointer to __le64 to initialize table entriews.
+	/* Filter and route tables contain DMA addresses that refer
+	 * to filter or route rules.  But the size of a table entry
+	 * is 64 bits regardless of what the size of an AP DMA address
+	 * is.  A fixed constant defines the size of an entry, and
+	 * code in ipa_table_init() uses a pointer to __le64 to
+	 * initialize tables.
 	 */
-	BUILD_BUG_ON(IPA_TABLE_ENTRY_SIZE != sizeof(dma_addr_t));
-	BUILD_BUG_ON(sizeof(dma_addr_t) != sizeof(__le64));
+	BUILD_BUG_ON(sizeof(dma_addr_t) > IPA_TABLE_ENTRY_SIZE);
+	BUILD_BUG_ON(sizeof(__le64) != IPA_TABLE_ENTRY_SIZE);
 
 	/* A "zero rule" is used to represent no filtering or no routing.
 	 * It is a 64-bit block of zeroed memory.  Code in ipa_table_init()
-- 
2.27.0

[PATCH net-next 2/4] net: ipa: introduce dma_addr_high32()

[Alex Elder]

Create a new helper function to encapsulate extracting the
high-order 32 bits of a DMA address.  It returns 0 for builds
in which a DMA address is not 64 bits.

This avoids doing a 32-position shift on a DMA address if it
happens not to be 64 bits wide.

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

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 2119367b93ea9..53698c64cf882 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -688,6 +688,16 @@ static void gsi_evt_ring_doorbell(struct gsi *gsi, u32 evt_ring_id, u32 index)
 	iowrite32(val, gsi->virt + GSI_EV_CH_E_DOORBELL_0_OFFSET(evt_ring_id));
 }
 
+/* Encapsulate extracting high-order 32 bits of DMA address */
+static u32 dma_addr_high32(dma_addr_t addr)
+{
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+	return (u32)(addr >> 32);
+#else /* !CONFIG_ARCH_DMA_ADDR_T_64BIT */
+	return 0;
+#endif /* !CONFIG_ARCH_DMA_ADDR_T_64BIT */
+}
+
 /* Program an event ring for use */
 static void gsi_evt_ring_program(struct gsi *gsi, u32 evt_ring_id)
 {
@@ -711,7 +721,7 @@ static void gsi_evt_ring_program(struct gsi *gsi, u32 evt_ring_id)
 	val = evt_ring->ring.addr & GENMASK(31, 0);
 	iowrite32(val, gsi->virt + GSI_EV_CH_E_CNTXT_2_OFFSET(evt_ring_id));
 
-	val = evt_ring->ring.addr >> 32;
+	val = dma_addr_high32(evt_ring->ring.addr);
 	iowrite32(val, gsi->virt + GSI_EV_CH_E_CNTXT_3_OFFSET(evt_ring_id));
 
 	/* Enable interrupt moderation by setting the moderation delay */
@@ -819,7 +829,7 @@ static void gsi_channel_program(struct gsi_channel *channel, bool doorbell)
 	val = channel->tre_ring.addr & GENMASK(31, 0);
 	iowrite32(val, gsi->virt + GSI_CH_C_CNTXT_2_OFFSET(channel_id));
 
-	val = channel->tre_ring.addr >> 32;
+	val = dma_addr_high32(channel->tre_ring.addr);
 	iowrite32(val, gsi->virt + GSI_CH_C_CNTXT_3_OFFSET(channel_id));
 
 	/* Command channel gets low weighted round-robin priority */
-- 
2.27.0

Re: [PATCH net-next 2/4] net: ipa: introduce dma_addr_high32()

[Florian Fainelli]

On 3/17/2021 3:29 PM, Alex Elder wrote:
> Create a new helper function to encapsulate extracting the
> high-order 32 bits of a DMA address.  It returns 0 for builds
> in which a DMA address is not 64 bits.
> 
> This avoids doing a 32-position shift on a DMA address if it
> happens not to be 64 bits wide.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ipa/gsi.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
> index 2119367b93ea9..53698c64cf882 100644
> --- a/drivers/net/ipa/gsi.c
> +++ b/drivers/net/ipa/gsi.c
> @@ -688,6 +688,16 @@ static void gsi_evt_ring_doorbell(struct gsi *gsi, u32 evt_ring_id, u32 index)
>  	iowrite32(val, gsi->virt + GSI_EV_CH_E_DOORBELL_0_OFFSET(evt_ring_id));
>  }
>  
> +/* Encapsulate extracting high-order 32 bits of DMA address */
> +static u32 dma_addr_high32(dma_addr_t addr)
> +{
> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> +	return (u32)(addr >> 32);

You can probably use upper_32bits() here...

> +#else /* !CONFIG_ARCH_DMA_ADDR_T_64BIT */
> +	return 0;
> +#endif /* !CONFIG_ARCH_DMA_ADDR_T_64BIT */
> +}
> +
>  /* Program an event ring for use */
>  static void gsi_evt_ring_program(struct gsi *gsi, u32 evt_ring_id)
>  {
> @@ -711,7 +721,7 @@ static void gsi_evt_ring_program(struct gsi *gsi, u32 evt_ring_id)
>  	val = evt_ring->ring.addr & GENMASK(31, 0);

...and you can use lower_32bits() here.

>  	iowrite32(val, gsi->virt + GSI_EV_CH_E_CNTXT_2_OFFSET(evt_ring_id));
>  
> -	val = evt_ring->ring.addr >> 32;
> +	val = dma_addr_high32(evt_ring->ring.addr);

Does the compiler do a good job at eliminating the assignment when
CONFIG_ARCH_DMA_ADDR_T_64BIT is not defined?

>  	iowrite32(val, gsi->virt + GSI_EV_CH_E_CNTXT_3_OFFSET(evt_ring_id));
>  
>  	/* Enable interrupt moderation by setting the moderation delay */
> @@ -819,7 +829,7 @@ static void gsi_channel_program(struct gsi_channel *channel, bool doorbell)
>  	val = channel->tre_ring.addr & GENMASK(31, 0);
>  	iowrite32(val, gsi->virt + GSI_CH_C_CNTXT_2_OFFSET(channel_id));
>  
> -	val = channel->tre_ring.addr >> 32;
> +	val = dma_addr_high32(channel->tre_ring.addr);
>  	iowrite32(val, gsi->virt + GSI_CH_C_CNTXT_3_OFFSET(channel_id));
>  
>  	/* Command channel gets low weighted round-robin priority */
> 

-- 
Florian

Re: [PATCH net-next 2/4] net: ipa: introduce dma_addr_high32()

[Alex Elder]

On 3/17/21 5:47 PM, Florian Fainelli wrote:
>> +/* Encapsulate extracting high-order 32 bits of DMA address */
>> +static u32 dma_addr_high32(dma_addr_t addr)
>> +{
>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>> +	return (u32)(addr >> 32);
> You can probably use upper_32bits() here...

Where is that defined?  I'd be glad to use it.	-Alex

Re: [PATCH net-next 2/4] net: ipa: introduce dma_addr_high32()

[Florian Fainelli]

On 3/17/2021 3:49 PM, Alex Elder wrote:
> On 3/17/21 5:47 PM, Florian Fainelli wrote:
>>> +/* Encapsulate extracting high-order 32 bits of DMA address */
>>> +static u32 dma_addr_high32(dma_addr_t addr)
>>> +{
>>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>>> +    return (u32)(addr >> 32);
>> You can probably use upper_32bits() here...
> 
> Where is that defined?  I'd be glad to use it.    -Alex

include/linux/kernel.h, and it is upper_32_bits() and lower_32_bits()
sorry about the missing space.
-- 
Florian

Re: [PATCH net-next 2/4] net: ipa: introduce dma_addr_high32()

[Alex Elder]

On 3/17/21 6:00 PM, Florian Fainelli wrote:
> 
> 
> On 3/17/2021 3:49 PM, Alex Elder wrote:
>> On 3/17/21 5:47 PM, Florian Fainelli wrote:
>>>> +/* Encapsulate extracting high-order 32 bits of DMA address */
>>>> +static u32 dma_addr_high32(dma_addr_t addr)
>>>> +{
>>>> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>>>> +    return (u32)(addr >> 32);
>>> You can probably use upper_32bits() here...
>>
>> Where is that defined?  I'd be glad to use it.    -Alex
> 
> include/linux/kernel.h, and it is upper_32_bits() and lower_32_bits()
> sorry about the missing space.

That's nice.  I'll still use a separate commit (and will
credit you with the suggestion) but it will be much smaller.

Thanks.  I'll post v2 shortly.

					-Alex

[PATCH net-next 3/4] net: ipa: fix table alignment requirement

[Alex Elder]

We currently have a build-time check to ensure that the minimum DMA
allocation alignment satisfies the constraint that IPA filter and
route tables must point to rules that are 128-byte aligned.

But what's really important is that the actual allocated DMA memory
has that alignment, even if the minimum is smaller than that.

Remove the BUILD_BUG_ON() call checking against minimim DMA alignment
and instead verify at rutime that the allocated memory is properly
aligned.

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

diff --git a/drivers/net/ipa/ipa_table.c b/drivers/net/ipa/ipa_table.c
index dd07fe9dd87a3..988f2c2886b95 100644
--- a/drivers/net/ipa/ipa_table.c
+++ b/drivers/net/ipa/ipa_table.c
@@ -118,14 +118,6 @@
 /* Check things that can be validated at build time. */
 static void ipa_table_validate_build(void)
 {
-	/* IPA hardware accesses memory 128 bytes at a time.  Addresses
-	 * referred to by entries in filter and route tables must be
-	 * aligned on 128-byte byte boundaries.  The only rule address
-	 * ever use is the "zero rule", and it's aligned at the base
-	 * of a coherent DMA allocation.
-	 */
-	BUILD_BUG_ON(ARCH_DMA_MINALIGN % IPA_TABLE_ALIGN);
-
 	/* Filter and route tables contain DMA addresses that refer
 	 * to filter or route rules.  But the size of a table entry
 	 * is 64 bits regardless of what the size of an AP DMA address
@@ -665,6 +657,18 @@ int ipa_table_init(struct ipa *ipa)
 	if (!virt)
 		return -ENOMEM;
 
+	/* We put the "zero rule" at the base of our table area.  The IPA
+	 * hardware requires rules to be aligned on a 128-byte boundary.
+	 * Make sure the allocation satisfies this constraint.
+	 */
+	if (addr % IPA_TABLE_ALIGN) {
+		dev_err(dev, "table address %pad not %u-byte aligned\n",
+			&addr, IPA_TABLE_ALIGN);
+		dma_free_coherent(dev, size, virt, addr);
+
+		return -ERANGE;
+	}
+
 	ipa->table_virt = virt;
 	ipa->table_addr = addr;
 
-- 
2.27.0

[PATCH net-next 4/4] net: ipa: relax 64-bit build requirement

[Alex Elder]

We currently assume the IPA driver is built only for a 64 bit kernel.

When this constraint was put in place it eliminated some do_div()
calls, replacing them with the "/" and "%" operators.  We now only
use these operations on u32 and size_t objects.  In a 32-bit kernel
build, size_t will be 32 bits wide, so there remains no reason to
use do_div() for divide and modulo.

A few recent commits also fix some code that assumes that DMA
addresses are 64 bits wide.

With that, we can get rid of the 64-bit build requirement.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/Kconfig    |  2 +-
 drivers/net/ipa/ipa_main.c | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ipa/Kconfig b/drivers/net/ipa/Kconfig
index b68f1289b89ef..90a90262e0d07 100644
--- a/drivers/net/ipa/Kconfig
+++ b/drivers/net/ipa/Kconfig
@@ -1,6 +1,6 @@
 config QCOM_IPA
 	tristate "Qualcomm IPA support"
-	depends on 64BIT && NET && QCOM_SMEM
+	depends on NET && QCOM_SMEM
 	depends on ARCH_QCOM || COMPILE_TEST
 	depends on QCOM_RPROC_COMMON || (QCOM_RPROC_COMMON=n && COMPILE_TEST)
 	select QCOM_MDT_LOADER if ARCH_QCOM
diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index 97c1b55405cbf..d354e3e65ec50 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -735,8 +735,14 @@ MODULE_DEVICE_TABLE(of, ipa_match);
 static void ipa_validate_build(void)
 {
 #ifdef IPA_VALIDATE
-	/* We assume we're working on 64-bit hardware */
-	BUILD_BUG_ON(!IS_ENABLED(CONFIG_64BIT));
+	/* At one time we assumed a 64-bit build, allowing some do_div()
+	 * calls to be replaced by simple division or modulo operations.
+	 * We currently only perform divide and modulo operations on u32,
+	 * u16, or size_t objects, and of those only size_t has any chance
+	 * of being a 64-bit value.  (It should be guaranteed 32 bits wide
+	 * on a 32-bit build, but there is no harm in verifying that.)
+	 */
+	BUILD_BUG_ON(!IS_ENABLED(CONFIG_64BIT) && sizeof(size_t) != 4);
 
 	/* Code assumes the EE ID for the AP is 0 (zeroed structure field) */
 	BUILD_BUG_ON(GSI_EE_AP != 0);
-- 
2.27.0