Re: [PATCH 5/6] arm64: dts: qcom: sm6350: Add UFS nodes

["Luca Weiss" <luca.weiss@fairphone.com>]

Hi Krzysztof,

On Sat Mar 19, 2022 at 3:43 PM CET, Krzysztof Kozlowski wrote:
> On 18/03/2022 19:30, Luca Weiss wrote:
> > Add the necessary nodes for UFS and its PHY.
> > 
> > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > ---
> >  arch/arm64/boot/dts/qcom/sm6350.dtsi | 79 ++++++++++++++++++++++++++++
> >  1 file changed, 79 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > index d7c9edff19f7..c5c93b6bcd2a 100644
> > --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > @@ -541,6 +541,85 @@ uart2: serial@98c000 {
> >  			};
> >  		};
> >  
> > +		ufs_mem_hc: ufshc@1d84000 {
>
> Generic node name, so ufs.

With the node name changes UFS doesn't probe anymore.

[    1.893762] ufshcd-qcom 1d84000.ufs: ufshcd_variant_hba_init: variant qcom init failed err -19
[    1.902674] ufshcd-qcom 1d84000.ufs: Initialization failed
[    1.908391] ufshcd-qcom 1d84000.ufs: ufshcd_pltfrm_init() failed -19

I didn't debug this in detail but it's likely from the
androidboot.bootdevice=1d84000.ufshc parameter in cmdline that
ufs-qcom.c uses to fail probe with -ENODEV for all UFS other than the
selected one. Not sure why this behavior exists in mainline (didn't look
into this either).

This cmdline parameter (among many others) is added by the stock
bootloader and as far as I know there's no way to turn that off.


>
> > +			compatible = "qcom,sm6350-ufshc", "qcom,ufshc",
> > +				     "jedec,ufs-2.0";
> > +			reg = <0 0x01d84000 0 0x3000>,
> > +			      <0 0x01d90000 0 0x8000>;
> > +			reg-names = "std", "ice";
> > +			interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>;
> > +			phys = <&ufs_mem_phy_lanes>;
> > +			phy-names = "ufsphy";
> > +			lanes-per-direction = <2>;
> > +			#reset-cells = <1>;
> > +			resets = <&gcc GCC_UFS_PHY_BCR>;
> > +			reset-names = "rst";
> > +
> > +			power-domains = <&gcc UFS_PHY_GDSC>;
> > +
> > +			iommus = <&apps_smmu 0x80 0x0>;
> > +
> > +			clock-names =
>
> Drop unneeded blank line, start just after '=' and align next elements
> with it.

Sure.

>
> > +				"core_clk",
> > +				"bus_aggr_clk",
> > +				"iface_clk",
> > +				"core_clk_unipro",
> > +				"core_clk_ice",
> > +				"ref_clk",
> > +				"tx_lane0_sync_clk",
> > +				"rx_lane0_sync_clk",
> > +				"rx_lane1_sync_clk";
> > +			clocks =
>
> The same.

Sure. I also adjusted the clock name/order to match the bindings (thanks
for spotting the problems there!)

Regards
Luca

>
>
> Best regards,
> Krzysztof