[PATCH] bus: mhi: host: Allocate entire MHI control config once

[Jeff Hugo]

On 4/24/2025 11:37 PM, Manivannan Sadhasivam wrote:
> + ath11k list, Jeff and Baochen (for question regarding the use of reserved
> memory for allocating the MHI data structures in host)
> 
> On Tue, Apr 08, 2025 at 08:56:43AM -0600, Jeff Hugo wrote:
>> On 4/8/2025 1:01 AM, Manivannan Sadhasivam wrote:
>>> On Fri, Mar 28, 2025 at 10:59:13AM -0600, Jeff Hugo wrote:
>>>> From: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy at quicinc.com>
>>>>
>>>> MHI control configurations such as channel context, event context, command
>>>> context and rings, are currently allocated individually. During MHI
>>>> initialization MHI bus driver needs to configure the address space in
>>>> which this control configuration resides. Since different component of the
>>>> config is being allocated separately, only logical solution is to give the
>>>> entire RAM address space, as they could be anywhere.
>>>>
>>>
>>> This is fine...
>>
>> We tripped over this when experimenting with an automotive market product.
>> The FW for that product had a rather strict interpretation of the spec,
>> which we confirmed with the spec owner.
>>
>> In the specific FW implementation, the device maps the entire MHI space of
>> shared structures in a single ATU entry. The device cannot map an entire
>> 64-bit address space, and it expects all of the shared structures in a
>> single compact range.
>>
>> This applies to the control structures, not the data buffers per the device
>> implementation.
>>
>> This restriction seems backed by the spec.  I can't find a reason why the
>> device is invalid, if limited.  I don't think this should break anything,
>> but more on that below.
>>
> 
> Yes, atleast I have heard about that limitation before.
> 
>>>
>>>> As per MHI specification the MHI control configuration address space should
>>>> not be more them 4GB.
>>>>
>>>
>>> Where exactly this limitation is specified in the spec? The spec supports full
>>> 64 bit address space for the MHI control/data structures. But due to the device
>>> DMA limitations, MHI controller drivers often use 32 bit address space. But
>>> that's not a spec limitation.
>>
>> Its not the clearest thing, sadly.
>>
>> Document 80-NF223-11 Rev AB "MHI spec v1.2" Section 6.2 "MHI Registers"
>> table 6-19 (page 106) -
>>
>> Describing MHICTRLLIMIT: "The most significant 32 bits of MHICTRLBASE and
>> MHICTRLLIMIT registers must be equal."
>>
>> This means we have a 4GB range (32-bit) to play with in a 64-bit address
>> space.  If the upper 32-bits of the 64-bit address for both the base and the
>> limit must be the same, then the range of addresses from the base to the
>> limit can only vary the lower 32-bits.
>>
>> Invalid:
>> BASE: 0x0
>> LIMIT: 0xffffffff_ffffffff
>>
>> Valid:
>> BASE: 0x0f_00000000
>> LIMIT: 0x0f_ffffffff
>>
> 
> Ah. Didn't spot this before, thanks for explaining!
> 
>>>> Since the current implementation is in violation of MHI specification.
>>
>> For example mhi_init_dev_ctxt()
>>
>> We allocate the chan_ctxt with dma_alloc_coherent() as an individual
>> allocation.  In the case of AIC100, the device can access the full 64-bit
>> address space, but the DMA engine is limited to a 32-bit transfer size.  The
>> chan_ctxt probably won't be larger than 4GB, so the size is rather
>> irrelevant.  Can be allocated anywhere.  Lets say that it gets put in the
>> lower 32-bit address space - 0x0_XXXXXXXX
>>
>> Then a little bit later we allocate er_ctxt with a different
>> dma_alloc_coherent() instance.  Being a unique allocation, it is not tied to
>> the chan_ctxt and can exist anywhere.  Lets assume that it gets put
>> somewhere in the non-lower 32-bits - 0x1000_XXXXXXXX
>>
>> Now we have a problem because we cannot describe a single range covering
>> both of these allocations via MHICTRLBASE/MHICTRLLIMIT where the upper
>> 32-bits of both registers is the same.
>>
> 
> Yes, I get it. I do not have issues in allocating all the structures in one go.
> But the problem is with MHICTRL_BASE/LIMIT. More below.
> 
>>>> Allocate a single giant DMA buffer for MHI control configurations and
>>>> limit the configuration address space to that buffer.
>>>>
>>>
>>> I don't think this could work for all devices. For instance, some ath11k devices
>>> use a fixed reserved region in host address space for MHICTRL/BASE.
>>
>> Why would we be unable to allocate all of the control structures in a single
>> allocation out of that reserved region?  Is it larger than 4GB in size?
>>
> 
> I was confused by the fact that ath11k driver adds an offset of 0x1000000 to the
> reserved region for the iova_start:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ath/ath11k/mhi.c?h=v6.15-rc3#n331
> 
> So this means the firmware will only map the memory from reserved + 0x1000000
> for MHI data structures. But even with current implementation, the MHI stack
> doesn't know anything about the offset, because it just relies on
> dma_alloc_coherent() API which will only honor the reserved region pointed by
> 'memory' property in the node (without the offset).
> 
> So I'm not sure how the firmware makes sure that the data structures lives
> within the offset region. This is more of a question to ath11k folks.
> 
> But your series is not going to make it any worse. Sorry about the confusion.

No problem.  I appreciate the sanity check.

-Jeff