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

[Jeff Hugo <jeff.hugo@oss.qualcomm.com>]

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@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.

> 
>> 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

>> 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.

>> 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?

-Jeff