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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4fa9074e-609a-42aa-975a-a6daa7dd6d42@kernel.org>
Date: Fri, 1 Aug 2025 18:09:18 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Manivannan Sadhasivam <mani@...nel.org>
Cc: Ram Kumar Dwivedi <quic_rdwivedi@...cinc.com>, alim.akhtar@...sung.com,
 avri.altman@....com, bvanassche@....org, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, andersson@...nel.org,
 konradybcio@...nel.org, agross@...nel.org, linux-arm-msm@...r.kernel.org,
 linux-scsi@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH V1 2/3] arm64: dts: qcom: sm8650: Enable MCQ support for
 UFS controller

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@...cinc.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@...0000 {
>>>>>>>  
>>>>>>>  		ufs_mem_hc: ufshc@...4000 {
>>>>>>>  			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