Re: [PATCH V1 2/3] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller

[Krzysztof Kozlowski <krzk@kernel.org>]

On 01/08/2025 17:33, Manivannan Sadhasivam wrote:
> On Fri, Aug 01, 2025 at 04:20:37PM GMT, Krzysztof Kozlowski wrote:
>> On 01/08/2025 14:24, Manivannan Sadhasivam wrote:
>>> On Thu, Jul 31, 2025 at 10:38:56AM GMT, Krzysztof Kozlowski wrote:
>>>> On 31/07/2025 10:34, Ram Kumar Dwivedi wrote:
>>>>>
>>>>>
>>>>> On 31-Jul-25 12:15 PM, Krzysztof Kozlowski wrote:
>>>>>> On 30/07/2025 10:22, Ram Kumar Dwivedi wrote:
>>>>>>> Enable Multi-Circular Queue (MCQ) support for the UFS host controller
>>>>>>> on the Qualcomm SM8650 platform by updating the device tree node. This
>>>>>>> includes adding new register regions and specifying the MSI parent
>>>>>>> required for MCQ operation.
>>>>>>>
>>>>>>> MCQ is a modern queuing model for UFS that improves performance and
>>>>>>> scalability by allowing multiple hardware queues. 
>>>>>>>
>>>>>>> Changes:
>>>>>>> - Add reg entries for mcq_sqd and mcq_vs regions.
>>>>>>> - Define reg-names for the new regions.
>>>>>>> - Specify msi-parent for interrupt routing.
>>>>>>>
>>>>>>> Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
>>>>>>> ---
>>>>>>>  arch/arm64/boot/dts/qcom/sm8650.dtsi | 9 ++++++++-
>>>>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
>>>>>>> index e14d3d778b71..5d164fe511ba 100644
>>>>>>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
>>>>>>> @@ -3982,7 +3982,12 @@ ufs_mem_phy: phy@1d80000 {
>>>>>>>  
>>>>>>>  		ufs_mem_hc: ufshc@1d84000 {
>>>>>>>  			compatible = "qcom,sm8650-ufshc", "qcom,ufshc", "jedec,ufs-2.0";
>>>>>>> -			reg = <0 0x01d84000 0 0x3000>;
>>>>>>> +			reg = <0 0x01d84000 0 0x3000>,
>>>>>>> +			      <0 0x01da5000 0 0x2000>,
>>>>>>> +			      <0 0x01da4000 0 0x0010>;
>>>>>>
>>>>>>
>>>>>> These are wrong address spaces. Open your datasheet and look there.
>>>>>>
>>>>> Hi Krzysztof,
>>>>>
>>>>> I’ve reviewed it again, and it is correct and functioning as expected both on our upstream and downstream codebase.
>>>>> I think it is probably overlooked by you. Can you please double check from your end?
>>>>>
>>>>
>>>> No, it is not overlooked. There is no address space of length 0x10 at
>>>> 0x01da4000 in qcom doc/datasheet system. Just open the doc and look
>>>> there by yourself. The size is 0x15000.
>>>>
>>>
>>> The whole UFS MCQ region is indeed of size 0x15000, but the SQD and VS registers
>>> are at random offsets, not fixed across the SoC revisions. And there are some
>>> big holes within the whole region for things like ICE and all.
>>>
>>> So it makes sense to map only the part of these regions and leave the unused
>>> ones.
>> Each item in the reg represents some continuous, dedicated address
>> space, not individual registers or artificially decided subsection. The
>> holes in such address space is not a problem, we do it all the time for
>> all other devices as well.
>>
>> You need to use the definition of that address space.
>>
> 
> What if some of the registers in that whole address space is shared with other
> peripherals such as ICE?


It will be a different address space. We don't talk about imaginary
3rd-party SoC. Qualcomm datasheet lists address spaces in very precise
way. We were recently fixing all address spaces for remoterpocs based on
that.

> 
> I agree with the fact that artifically creating separate register spaces leads
> to issues, but here I'm worried about hardcoding the offsets in the driver which
> can change between SoCs and also the shared address space with ICE.

Drivers are expected to hard-code offsets and all drivers do it. Look at
display, sound codecs (both SoC and soundwire devices). Everything
hard-coded offsets internal to address space.

What you essentially want is (making it border case) "reg" per register.

Best regards,
Krzysztof