Re: [PATCH 3/3] net: ipa: Add IPA v5.1 data

[Esteban Urrutia <esteuwu@proton.me>]

On 7/8/26 4:06 PM, Alex Elder wrote:
> I think the DDR values might be wrong, but it's difficult to be
> sure.  In some cases, in arrays like this in the downstream code,
> if there is no entry found in an array, the *earlier* version
> values should be used.  (Unless someone better informed states
> that this is wrong, I think it's fine as-is.)
> 
> This information is found in the ipa3_qmb_outstanding[IPA_5_1][]
> array in the downstream code.  However there is no entry for that
> version.  Given that, all zeroes (as you have it) makes sense.
> But it's possible this applies instead:
> 
>          [IPA_5_0][IPA_QMB_INSTANCE_DDR]         = {12, 12, 0},
>          [IPA_5_0][IPA_QMB_INSTANCE_PCIE]        = {0, 0, 0},
> 
> I have no way of knowing; perhaps someone from Qualcomm can
> get confirmation that all zeroes is correct.
> 
> (Note the order of values presented in the downstream code
> differs from upstream.)

In downstream ipa_utils.c there's a function called ipa3_cfg_qsb() which
is in charge of reading these values from ipa3_qmb_outstanding.
Since these values are not present for IPA v5.1, one would assume
they're not set and that, since this is the first time such behavior is
found, additional changes would need to be made to the IPA driver.
However, looking at this function, which I'll leave as a snippet below
given its shortness:

static void ipa3_cfg_qsb(void)
{
	u8 hw_type_idx;
	const struct ipa_qmb_outstanding *qmb_ot;
	struct ipahal_reg_qsb_max_reads max_reads = { 0 };
	struct ipahal_reg_qsb_max_writes max_writes = { 0 };

	hw_type_idx = ipa3_ctx->hw_type_index;

	/*
	 * Read the register values before writing to them to ensure
	 * other values are not overwritten
	 */
	ipahal_read_reg_fields(IPA_QSB_MAX_WRITES, &max_writes);
	ipahal_read_reg_fields(IPA_QSB_MAX_READS, &max_reads);

	qmb_ot = &(ipa3_qmb_outstanding[hw_type_idx][IPA_QMB_INSTANCE_DDR]);
	max_reads.qmb_0_max_reads = qmb_ot->ot_reads;
	max_writes.qmb_0_max_writes = qmb_ot->ot_writes;
	max_reads.qmb_0_max_read_beats = qmb_ot->ot_read_beats;

	qmb_ot = &(ipa3_qmb_outstanding[hw_type_idx][IPA_QMB_INSTANCE_PCIE]);
	max_reads.qmb_1_max_reads = qmb_ot->ot_reads;
	max_writes.qmb_1_max_writes = qmb_ot->ot_writes;

	ipahal_write_reg_fields(IPA_QSB_MAX_WRITES, &max_writes);
	ipahal_write_reg_fields(IPA_QSB_MAX_READS, &max_reads);
}

There are no conditions for writing ot_reads, ot_writes and
ot_read_beats, which would correspond to max_reads, max_writes and
max_reads_beats in upstream.
Since hw_type_idx is set to IPA_5_1, this should give a null pointer.
With this info, there are two possibilities:

1. Null pointer dereference, resulting in a kernel oops downstream.
2. qmb_ot is set with all values to 0.

I have never seen a null pointer dereference downstream, so I'm more
inclined to believe option 2 is what's actually happening.
IPA v5.0 also zeroes these values, but I wasn't able to actually confirm
this.
This because in downstream, IPA versions are split into subversions,
which are:

1. Normal IPA (IPA_X_Y)
2. MHI IPA (IPA_X_Y_MHI)
3. APQ IPA (IPA_X_Y_APQ)

Data for IPA v5.0 seems to be of the MHI type, since it's only used in
the SDX65 SoC, which I believe is mostly used in 5G modems.
I realized this while cross-checking since some values didn't really
match, so I had to cross-check with different ipa_data-vX.Y.c files
because of this.

> And although ipa_gsi_ep_config is not defined in this code
> base, here is what it looks like:
> 
> struct ipa_gsi_ep_config {
>          int ipa_ep_num;
>          int ipa_gsi_chan_num;
>          int ipa_if_tlv;
>          int ipa_if_aos;
>          int ee;
>          enum gsi_prefetch_mode prefetch_mode;
>          uint8_t prefetch_threshold;
> };
> 
> This might not be current; I'm using code found here:
>    https://git.codelinaro.org/clo/la/kernel/msm-5.15.git

Many thanks for providing the declaration for this struct.

> In the downstream code--confusingly--ipa_gsi_setup_channel()
> doubles the desc_fifo_sz value (for GSI, versus the older BAM
> interface).  So the ring size becomes 4096 bytes, and that
> works out to 256 16-byte GSI TRE entries.  I'm not sure why
> 512 is used for IPA v3.5.1, but it probably just means it's
> bigger than it needs to be.
> 
> The event_count should be the same as the tre_count.  Again
> I no longer know why that's not the case for IPA v3.5.1.

For the record (since this is off-topic). I tried to get the modem up in
a device whose SoC was using IPA v3.5.1 and I was experimenting weird
behavior, such as IPA crashing the SoC when removing the module or when
it automatically loaded at boot.
The reason I mention this is because a warning similar to "channel 4
limited to 256 TREs" appeared whenever IPA was loaded. That may be the
reason I was experimenting those issues.

>> +static const struct ipa_mem ipa_mem_local_data[] = {
> 
> IPA has local memory that is partitioned as defined by this
> array.  The regions are used by IPA/GSI firmware and/or
> hardware.  The configuration defined here is sent to
> the modem in an ipa_init_modem_driver_req QMI message
> so both the modem and AP have a consistent view of
> how the memory is used.
> 
> Many memory regions are preceded by 0-2 "canaries", which
> are 32-byte values initialized to IPA_MEM_CANARY_VAL.
> 
> In the downstream code there is structure ipa3_mem_partition
> that defines these things, and structures of this type are
> defined in "ipa_utils.c".  For IPA v5.1, ipa_5_1_mem_part
> defines them all.  The mapping between downstream and
> upstream is not trivial and direct, but it should be
> obvious how they get translated.
> 
> 
> With two exceptions, what I see here looks like you
> correctly transferred everything.  (The two exceptions
> are entries that from what I can tell, should not be
> present.)

[...]

> The next two entries look wrong to me.  Can you explain where
> you got these offsets and sizes?  Is it from "ipa_data-v5.0.c"?
> 
> Here are the relevant entries I see in ipa_5_1_mem_part
> in the downstream code:
>          .stats_flt_v4_ofst = 0,
>          .stats_flt_v4_size = 0,
>          .stats_flt_v6_ofst = 0,
>          .stats_flt_v6_size = 0,
>          .stats_rt_v4_ofst = 0,
>          .stats_rt_v4_size = 0,
>          .stats_rt_v6_ofst = 0,
>          .stats_rt_v6_size = 0,
> (Since their size is zero, their entries can be omitted.)
> 
>> +	{
>> +		.id		= IPA_MEM_AP_V4_FILTER,
>> +		.offset		= 0x29b8,
>> +		.size		= 0x0188,
>> +		.canary_count	= 2,
>> +	},
>> +	{
>> +		.id		= IPA_MEM_AP_V6_FILTER,
>> +		.offset		= 0x2b40,
>> +		.size		= 0x0228,
>> +		.canary_count	= 0,
>> +	},
> 
> The remaining entries (below) look good.

While cross-referencing IPA data files upstream, I found these two
regions to be present in data for IPA v5.5, even though they aren't
defined in ipa_5_5_mem_part. At the start I assumed these were correct
since they were present upstream, but I took a closer look at that file
and I believe data for this version was directly added without going
through a proper review process.

The reason why I used IPA v5.5 memory regions in IPA v5.1 is because
their memory partitions are identical.

Now, with this information, I would like to ask whether both of these
memory regions are correct for both IPA v5.1 and v5.5 data files.

>> +/* Memory configuration data for an SoC having IPA v5.1 */
>> +static const struct ipa_mem_data ipa_mem_data = {
>> +	.local_count	= ARRAY_SIZE(ipa_mem_local_data),
>> +	.local		= ipa_mem_local_data,
>> +	.imem_addr	= 0x146a8000,
> 
> I think I needed to look up the imem offset value
> in Qualcomm documentation I no longer have access
> to.  Perhaps someone from there could confirm you
> are using the right values here.

After cross-checking I believe this may be the qcom,additional-mapping
property downstream, which specifies the IMEM starting address and size.
I'll leave (2) and (3) for reference.
(2) corresponds to SM8450, while (3) corresponds to SM8475.

> 
>> +	.imem_size	= 0x00002000,
>> +	/*
>> +	 * While this value is 0xb000 on SM8450 and 0x9000 on SM8475,
>> +	 * it has been left set to 0x9000 for compatibility with SM8475
>> +	 */
> 
> As I said earlier, I'm not completely sure this will still
> work on the SM8450.  Someone should confirm this, and it
> really ought to be tested somehow.

I have clarified this in my previous email (4), so I'll skip this part.

> 
>> +	.smem_size	= 0x00009000,
>> +};
>> +
>> +/* Interconnect rates are in 1000 byte/second units */
>> +static const struct ipa_interconnect_data ipa_interconnect_data[] = {
>> +	{
>> +		.name			= "memory",
>> +		.peak_bandwidth		= 1900000,	/* 1.9 GBps */
>> +		.average_bandwidth	= 590000,	/* 590 MBps */
> 
> I no longer recall where to get these bandwidth values
> for the interconnects.  Perhaps someone from Qualcomm
> can find this out/confirm what you have.

This was a tricky part. These seem to come from the qcom,svs2 property
which seems to be mapped to the interconnects specified downstream.
Since the IPA interconnects declared in device trees are different in
both downstream and upstream I had to make some adjustments, such as
using the minimum value between both ipa_to_llcc and llcc_to_ebi1
interconnects.
An example of this can be seen in (5), which corresponds to the SM8350
SoC using IPA v4.9.

Again, thanks for taking the time to properly explain things.

(1) https://github.com/LineageOS/android_kernel_qcom_sm8450-modules/blob/lineage-20/qcom/opensource/dataipa/drivers/platform/msm/ipa/ipa_v3/ipa_utils.c#L7881
https://github.com/LineageOS/android_kernel_qcom_sm8450-devicetrees/blob/lineage-20/qcom/waipio.dtsi#L3404
https://github.com/LineageOS/android_kernel_qcom_sm8450-devicetrees/blob/lineage-20/qcom/cape.dtsi#L2723
(4) https://lore.kernel.org/all/3e70d77e-6bec-4e16-ae88-a4f5161f182e@proton.me/
(5) https://github.com/LineageOS/android_kernel_motorola_sm7325/blob/lineage-23.2/arch/arm64/boot/dts/vendor/qcom/lahaina.dtsi#L4683

Regards,
Esteban