Re: [PATCH v6 4/6] arm64: dts: qcom: sm6125: Add UFS nodes

[Caleb Connolly <caleb.connolly@linaro.org>]

On 11/01/2023 12:04, Konrad Dybcio wrote:
> 
> 
> On 11.01.2023 03:53, Lux Aliaga wrote:
>>
>> On 09/01/2023 09:18, Konrad Dybcio wrote:
>>>
>>> On 8.01.2023 20:53, Lux Aliaga wrote:
>>>> Adds a UFS host controller node and its corresponding PHY to
>>>> the sm6125 platform.
>>>>
>>>> Signed-off-by: Lux Aliaga <they@mint.lgbt>
>>>> ---
>>>>   arch/arm64/boot/dts/qcom/sm6125.dtsi | 57 ++++++++++++++++++++++++++++
>>>>   1 file changed, 57 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6125.dtsi b/arch/arm64/boot/dts/qcom/sm6125.dtsi
>>>> index df5453fcf2b9..cec7071d5279 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sm6125.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sm6125.dtsi
>>>> @@ -511,6 +511,63 @@ sdhc_2: mmc@4784000 {
>>>>               status = "disabled";
>>>>           };
>>>>   +        ufs_mem_hc: ufs@4804000 {
>>>> +            compatible = "qcom,sm6125-ufshc", "qcom,ufshc", "jedec,ufs-2.0";
>>>> +            reg = <0x04804000 0x3000>, <0x04810000 0x8000>;
>>> You need reg-names for ICE to probe, otherwise the second reg sits unused.
>>>
>>>> +            interrupts = <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>;
>>>> +            phys = <&ufs_mem_phy>;
>>>> +            phy-names = "ufsphy";
>>>> +            lanes-per-direction = <1>;
>>>> +            #reset-cells = <1>;
>>>> +            resets = <&gcc GCC_UFS_PHY_BCR>;
>>>> +            reset-names = "rst";
>>>> +            iommus = <&apps_smmu 0x200 0x0>;
>>>> +
>>>> +            clock-names = "core_clk",
>>>> +                      "bus_aggr_clk",
>>>> +                      "iface_clk",
>>>> +                      "core_clk_unipro",
>>>> +                      "ref_clk",
>>>> +                      "tx_lane0_sync_clk",
>>>> +                      "rx_lane0_sync_clk",
>>>> +                      "ice_core_clk";
>>>> +            clocks = <&gcc GCC_UFS_PHY_AXI_CLK>,
>>>> +                 <&gcc GCC_SYS_NOC_UFS_PHY_AXI_CLK>,
>>>> +                 <&gcc GCC_UFS_PHY_AHB_CLK>,
>>>> +                 <&gcc GCC_UFS_PHY_UNIPRO_CORE_CLK>,
>>>> +                 <&rpmcc RPM_SMD_XO_CLK_SRC>,
>>>> +                 <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>,
>>>> +                 <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>,
>>>> +                 <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
>>>> +            freq-table-hz = <50000000 240000000>,
>>>> +                    <0 0>,
>>>> +                    <0 0>,
>>>> +                    <37500000 150000000>,
>>>> +                    <0 0>,
>>>> +                    <0 0>,
>>>> +                    <0 0>,
>>>> +                    <75000000 300000000>;
>>>> +
>>>> +            status = "disabled";
>>>> +        };
>>>> +
>>>> +        ufs_mem_phy: phy@4807000 {
>>>> +            compatible = "qcom,sm6125-qmp-ufs-phy";
>>>> +            reg = <0x04807000 0x1c4>;
>>> Isn't this too small? Downstream says 0xdb8, but it's probably even bigger..
>> What do you think could help me find the new length of the registers? I tried 0x1000 and it probed just fine, but I'm not really sure until what extent I could push it.
> The "true" values are probably only in documentation, which
> I don't have.

This patch series uses the "new" DT layout, where there isn't a subnode
to define the address ranges of the different components.

The reg size would be correct if it used the "legacy" DT layout.

Confirming in downstream, 0xdb8 is the correct value (it's what DT uses
there and the phy-qcom-ufs-qmp-v3-660 driver confirms the biggest
register offset is PHY_BASE (0xc00) + PHY_SIZE (0x1b4) = 0xdb4 inclusive)

I'd suggest going for that in your next revision Lux.
> 
> Konrad
>>

-- 
Kind Regards,
Caleb (they/them)