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.
Version 2 of this series uses upper_32_bits() rather than creating
a new function to extract bits out of a DMA address.
-Alex
Alex Elder (4):
net: ipa: fix assumptions about DMA address size
net: ipa: use upper_32_bits()
net: ipa: fix table alignment requirement
net: ipa: relax 64-bit build requirement
drivers/net/ipa/Kconfig | 2 +-
drivers/net/ipa/gsi.c | 4 ++--
drivers/net/ipa/ipa_main.c | 10 ++++++++--
drivers/net/ipa/ipa_table.c | 34 ++++++++++++++++++++--------------
4 files changed, 31 insertions(+), 19 deletions(-)
--
2.27.0
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Use upper_32_bits() to extract the high-order 32 bits of a DMA
address. This avoids doing a 32-position shift on a DMA address
if it happens not to be 64 bits wide.
Suggested-by: Florian Fainelli <[email protected]>
Signed-off-by: Alex Elder <[email protected]>
---
v2: - Switched to use the existing function, as suggested by Florian.
drivers/net/ipa/gsi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 2119367b93ea9..82c5a0d431ee5 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -711,7 +711,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 = upper_32_bits(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 +819,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 = upper_32_bits(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
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 <[email protected]>
---
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
On 3/18/2021 6:51 AM, Alex Elder wrote:
> Use upper_32_bits() to extract the high-order 32 bits of a DMA
> address. This avoids doing a 32-position shift on a DMA address
> if it happens not to be 64 bits wide.
>
> Suggested-by: Florian Fainelli <[email protected]>
> Signed-off-by: Alex Elder <[email protected]>
> ---
> v2: - Switched to use the existing function, as suggested by Florian.
>
> drivers/net/ipa/gsi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
> index 2119367b93ea9..82c5a0d431ee5 100644
> --- a/drivers/net/ipa/gsi.c
> +++ b/drivers/net/ipa/gsi.c
> @@ -711,7 +711,7 @@ static void gsi_evt_ring_program(struct gsi *gsi, u32 evt_ring_id)
> val = evt_ring->ring.addr & GENMASK(31, 0);
Did you want to introduce another patch to use lower_32_bits() for the
assignment above?
> iowrite32(val, gsi->virt + GSI_EV_CH_E_CNTXT_2_OFFSET(evt_ring_id));
>
> - val = evt_ring->ring.addr >> 32;
> + val = upper_32_bits(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 +819,7 @@ static void gsi_channel_program(struct gsi_channel *channel, bool doorbell)
> val = channel->tre_ring.addr & GENMASK(31, 0);
And likewise?
> iowrite32(val, gsi->virt + GSI_CH_C_CNTXT_2_OFFSET(channel_id));
>
> - val = channel->tre_ring.addr >> 32;
> + val = upper_32_bits(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
On 3/18/21 11:03 AM, Florian Fainelli wrote:
>
>
> On 3/18/2021 6:51 AM, Alex Elder wrote:
>> Use upper_32_bits() to extract the high-order 32 bits of a DMA
>> address. This avoids doing a 32-position shift on a DMA address
>> if it happens not to be 64 bits wide.
>>
>> Suggested-by: Florian Fainelli <[email protected]>
>> Signed-off-by: Alex Elder <[email protected]>
>> ---
>> v2: - Switched to use the existing function, as suggested by Florian.
>>
>> drivers/net/ipa/gsi.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
>> index 2119367b93ea9..82c5a0d431ee5 100644
>> --- a/drivers/net/ipa/gsi.c
>> +++ b/drivers/net/ipa/gsi.c
>> @@ -711,7 +711,7 @@ static void gsi_evt_ring_program(struct gsi *gsi, u32 evt_ring_id)
>> val = evt_ring->ring.addr & GENMASK(31, 0);
>
> Did you want to introduce another patch to use lower_32_bits() for the
> assignment above?
Bah... Yes, I should have done that. I was really just
focused on the new function I created and didn't need
to. I'll do as you suggest.
I'll also see if I do that anywhere else in the driver
and will use these functions as well if appropriate.
I'll do so in the same patch 2 of version 3.
Thanks Florian.
-Alex
>
>> iowrite32(val, gsi->virt + GSI_EV_CH_E_CNTXT_2_OFFSET(evt_ring_id));
>>
>> - val = evt_ring->ring.addr >> 32;
>> + val = upper_32_bits(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 +819,7 @@ static void gsi_channel_program(struct gsi_channel *channel, bool doorbell)
>> val = channel->tre_ring.addr & GENMASK(31, 0);
>
> And likewise?
>
>> iowrite32(val, gsi->virt + GSI_CH_C_CNTXT_2_OFFSET(channel_id));
>>
>> - val = channel->tre_ring.addr >> 32;
>> + val = upper_32_bits(channel->tre_ring.addr);
>> iowrite32(val, gsi->virt + GSI_CH_C_CNTXT_3_OFFSET(channel_id));
>>
>> /* Command channel gets low weighted round-robin priority */
>>
>