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: <5se3wgpfabzlcidflef5orwtl62jk2dtg4lx47gnqcqn7mya46@i6zir5uny7gi>
Date: Mon, 11 Aug 2025 14:57:05 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Krzysztof Kozlowski <krzk@...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 Fri, Aug 01, 2025 at 06:09:18PM GMT, Krzysztof Kozlowski wrote:
> 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.
> 

I was worried about the ICE overlap, but I got access to the documentation and
verified myself (also with Nitin) that there is no ICE overlap. So yes, we can
map the entire MCQ region and live with the hardcoded offsets.

- Mani

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