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

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

On 4/28/2025 7:44 PM, Baochen Qiang wrote:
> 
> 
> On 4/28/2025 11:09 PM, Jeff Hugo wrote:
>> On 4/27/2025 7:37 PM, Baochen Qiang wrote:
>>>
>>>
>>> On 4/8/2025 10:56 PM, 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@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
>>>
>>> as an MHI controller driver, ath11k is doing
>>>
>>>          mhi_ctrl->iova_start = 0;
>>>          mhi_ctrl->iova_stop = ab_pci->dma_mask;
>>>
>>> where ab_pci->dma_mask is defined as
>>>
>>>          ab_pci->dma_mask = DMA_BIT_MASK(36)
>>>
>>> clearly this does not align with the 32bit requirement above, however there is no issue
>>> hit.
>>>
>>> What happens if the most significant 32 bits of MHICTRLBASE and MHICTRLLIMIT is not equal?
>>> SYS_ERR or RDDM?
>>
>> On the device we experienced this with, we would be able to transition to M0, but the
>> later the device would have an internal fault and crash.
> 
> so with this patch, MHICTRLBASE/MHICTRLLIMIT are untied from iova_start/iova_stop, meaning
> current ath11k settings does not break the requirement, hence no ath11k change needed, right?

I think so.

-Jeff