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

[Manivannan Sadhasivam]

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

- Mani

-- 
மணிவண்ணன் சதாசிவம்