Hello,
After weeks (literally!) of poking the system at random, Jeffrey found why UFS
refused to work on APQ8098: we were not setting the load on the vregs.
Difference between v1 and v2:
- New patch to add 'regulator-allow-set-load' prop to all vreg nodes
- Rename rpmcc node to 'clock-controller' + Add Review tags
- Drop UFS pinctrl gymnastics (not required, probably left enabled in bootloader)
- Delete GCC_UFS_ICE_CORE_CLK (ICE not used upstream, I think)
- Fix sizes of ufsphy register areas based on Jeffrey's feedback
- Hack ufshcd_set_vccq_rail_unused into a NOP to work around lock up + reboot
Marc Gonzalez (4):
arm64: dts: qcom: msm8998: Add rpmcc node
arm64: dts: qcom: msm8998: Add UFS nodes
Add regulator-allow-set-load
ufshcd_set_vccq_rail_unused locks up the board
arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi | 52 +++++++++++++++++
arch/arm64/boot/dts/qcom/msm8998.dtsi | 69 +++++++++++++++++++++++
drivers/scsi/ufs/ufshcd.c | 1 +
3 files changed, 122 insertions(+)
--
2.17.1
Add host controller and PHY DT nodes.
Signed-off-by: Marc Gonzalez <[email protected]>
---
TODO: check whether the driver uses the 'resets' prop
---
arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi | 20 +++++++
arch/arm64/boot/dts/qcom/msm8998.dtsi | 63 +++++++++++++++++++++++
2 files changed, 83 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
index 50e9033aa7f6..cd1c9e84eab7 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
@@ -257,3 +257,23 @@
pinctrl-0 = <&sdc2_clk_on &sdc2_cmd_on &sdc2_data_on &sdc2_cd_on>;
pinctrl-1 = <&sdc2_clk_off &sdc2_cmd_off &sdc2_data_off &sdc2_cd_off>;
};
+
+&ufshc {
+ vdd-hba-fixed-regulator;
+ vcc-supply = <&vreg_l20a_2p95>;
+ vccq-supply = <&vreg_l26a_1p2>;
+ vccq2-supply = <&vreg_s4a_1p8>;
+ vcc-max-microamp = <750000>;
+ vccq-max-microamp = <560000>;
+ vccq2-max-microamp = <750000>;
+};
+
+&ufsphy {
+ vdda-phy-supply = <&vreg_l1a_0p875>;
+ vdda-pll-supply = <&vreg_l2a_1p2>;
+ vddp-ref-clk-supply = <&vreg_l26a_1p2>;
+ vdda-phy-max-microamp = <51400>;
+ vdda-pll-max-microamp = <14600>;
+ vddp-ref-clk-max-microamp = <100>;
+ vddp-ref-clk-always-on;
+};
diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index 6f4f4b79853b..36fd2e614464 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -711,6 +711,69 @@
redistributor-stride = <0x0 0x20000>;
interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
};
+
+ ufshc: ufshc@1da4000 {
+ compatible = "qcom,msm8998-ufshc", "qcom,ufshc",
+ "jedec,ufs-2.0";
+ reg = <0x1da4000 0x2500>;
+ interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>;
+ phys = <&ufsphy_lanes>;
+ phy-names = "ufsphy";
+ lanes-per-direction = <2>;
+ power-domains = <&gcc UFS_GDSC>;
+
+ clock-names =
+ "core_clk",
+ "bus_aggr_clk",
+ "iface_clk",
+ "core_clk_unipro",
+ "ref_clk",
+ "tx_lane0_sync_clk",
+ "rx_lane0_sync_clk",
+ "rx_lane1_sync_clk";
+ clocks =
+ <&gcc GCC_UFS_AXI_CLK>,
+ <&gcc GCC_AGGRE1_UFS_AXI_CLK>,
+ <&gcc GCC_UFS_AHB_CLK>,
+ <&gcc GCC_UFS_UNIPRO_CORE_CLK>,
+ <&rpmcc RPM_SMD_LN_BB_CLK1>,
+ <&gcc GCC_UFS_TX_SYMBOL_0_CLK>,
+ <&gcc GCC_UFS_RX_SYMBOL_0_CLK>,
+ <&gcc GCC_UFS_RX_SYMBOL_1_CLK>;
+ freq-table-hz =
+ <50000000 200000000>,
+ <0 0>,
+ <0 0>,
+ <37500000 150000000>,
+ <0 0>,
+ <0 0>,
+ <0 0>,
+ <0 0>;
+
+ resets = <&gcc GCC_UFS_BCR>;
+ reset-names = "rst";
+ };
+
+ ufsphy: phy@1da7000 {
+ compatible = "qcom,sdm845-qmp-ufs-phy";
+ reg = <0x1da7000 0x18c>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+ clock-names = "ref", "ref_aux";
+ clocks =
+ <&gcc GCC_UFS_CLKREF_CLK>,
+ <&gcc GCC_UFS_PHY_AUX_CLK>;
+
+ ufsphy_lanes: lanes@1da7400 {
+ reg = <0x1da7400 0x128>,
+ <0x1da7600 0x1fc>,
+ <0x1da7c00 0x1dc>,
+ <0x1da7800 0x128>,
+ <0x1da7a00 0x1fc>;
+ #phy-cells = <0>;
+ };
+ };
};
};
--
2.17.1
Better solution required.
Suggestions? A quirk?
Specify that the vccq rail cannot be disabled in the DT?
---
drivers/scsi/ufs/ufshcd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9ba7671b84f8..d0d340210ccf 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7196,6 +7196,7 @@ static int ufshcd_set_vccq_rail_unused(struct ufs_hba *hba, bool unused)
{
int ret = 0;
struct ufs_vreg_info *info = &hba->vreg_info;
+ return 0; /*** NOP ***/
if (!info)
goto out;
--
2.17.1
Add MSM8998 Resource Power Manager Clock Controller DT node.
Reviewed-by: Jeffrey Hugo <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Signed-off-by: Marc Gonzalez <[email protected]>
---
arch/arm64/boot/dts/qcom/msm8998.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index a6d66cf77403..6f4f4b79853b 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -3,6 +3,7 @@
#include <dt-bindings/interrupt-controller/arm-gic.h>
#include <dt-bindings/clock/qcom,gcc-msm8998.h>
+#include <dt-bindings/clock/qcom,rpmcc.h>
#include <dt-bindings/gpio/gpio.h>
/ {
@@ -266,6 +267,11 @@
rpm_requests: rpm-requests {
compatible = "qcom,rpm-msm8998";
qcom,glink-channels = "rpm_requests";
+
+ rpmcc: clock-controller {
+ compatible = "qcom,rpmcc-msm8998", "qcom,rpmcc";
+ #clock-cells = <1>;
+ };
};
};
--
2.17.1
BLURB
---
Set the prop for all vregs or just the UFS vregs?
---
arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi | 32 +++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
index cd1c9e84eab7..3d18a97d1563 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
@@ -100,132 +100,164 @@
vreg_s3a_1p35: s3 {
regulator-min-microvolt = <1352000>;
regulator-max-microvolt = <1352000>;
+ regulator-allow-set-load;
};
vreg_s4a_1p8: s4 {
regulator-min-microvolt = <1800000>;
regulator-max-microvolt = <1800000>;
+ regulator-allow-set-load;
};
vreg_s5a_2p04: s5 {
regulator-min-microvolt = <1904000>;
regulator-max-microvolt = <2040000>;
+ regulator-allow-set-load;
};
vreg_s7a_1p025: s7 {
regulator-min-microvolt = <900000>;
regulator-max-microvolt = <1028000>;
+ regulator-allow-set-load;
};
vreg_l1a_0p875: l1 {
regulator-min-microvolt = <880000>;
regulator-max-microvolt = <880000>;
+ regulator-allow-set-load;
};
vreg_l2a_1p2: l2 {
regulator-min-microvolt = <1200000>;
regulator-max-microvolt = <1200000>;
+ regulator-allow-set-load;
};
vreg_l3a_1p0: l3 {
regulator-min-microvolt = <1000000>;
regulator-max-microvolt = <1000000>;
+ regulator-allow-set-load;
};
vreg_l5a_0p8: l5 {
regulator-min-microvolt = <800000>;
regulator-max-microvolt = <800000>;
+ regulator-allow-set-load;
};
vreg_l6a_1p8: l6 {
regulator-min-microvolt = <1808000>;
regulator-max-microvolt = <1808000>;
+ regulator-allow-set-load;
};
vreg_l7a_1p8: l7 {
regulator-min-microvolt = <1800000>;
regulator-max-microvolt = <1800000>;
+ regulator-allow-set-load;
};
vreg_l8a_1p2: l8 {
regulator-min-microvolt = <1200000>;
regulator-max-microvolt = <1200000>;
+ regulator-allow-set-load;
};
vreg_l9a_1p8: l9 {
regulator-min-microvolt = <1808000>;
regulator-max-microvolt = <2960000>;
+ regulator-allow-set-load;
};
vreg_l10a_1p8: l10 {
regulator-min-microvolt = <1808000>;
regulator-max-microvolt = <2960000>;
+ regulator-allow-set-load;
};
vreg_l11a_1p0: l11 {
regulator-min-microvolt = <1000000>;
regulator-max-microvolt = <1000000>;
+ regulator-allow-set-load;
};
vreg_l12a_1p8: l12 {
regulator-min-microvolt = <1800000>;
regulator-max-microvolt = <1800000>;
+ regulator-allow-set-load;
};
vreg_l13a_2p95: l13 {
regulator-min-microvolt = <1808000>;
regulator-max-microvolt = <2960000>;
+ regulator-allow-set-load;
};
vreg_l14a_1p88: l14 {
regulator-min-microvolt = <1880000>;
regulator-max-microvolt = <1880000>;
+ regulator-allow-set-load;
};
vreg_15a_1p8: l15 {
regulator-min-microvolt = <1800000>;
regulator-max-microvolt = <1800000>;
+ regulator-allow-set-load;
};
vreg_l16a_2p7: l16 {
regulator-min-microvolt = <2704000>;
regulator-max-microvolt = <2704000>;
+ regulator-allow-set-load;
};
vreg_l17a_1p3: l17 {
regulator-min-microvolt = <1304000>;
regulator-max-microvolt = <1304000>;
+ regulator-allow-set-load;
};
vreg_l18a_2p7: l18 {
regulator-min-microvolt = <2704000>;
regulator-max-microvolt = <2704000>;
+ regulator-allow-set-load;
};
vreg_l19a_3p0: l19 {
regulator-min-microvolt = <3008000>;
regulator-max-microvolt = <3008000>;
+ regulator-allow-set-load;
};
vreg_l20a_2p95: l20 {
regulator-min-microvolt = <2960000>;
regulator-max-microvolt = <2960000>;
+ regulator-allow-set-load;
};
vreg_l21a_2p95: l21 {
regulator-min-microvolt = <2960000>;
regulator-max-microvolt = <2960000>;
+ regulator-allow-set-load;
};
vreg_l22a_2p85: l22 {
regulator-min-microvolt = <2864000>;
regulator-max-microvolt = <2864000>;
+ regulator-allow-set-load;
};
vreg_l23a_3p3: l23 {
regulator-min-microvolt = <3312000>;
regulator-max-microvolt = <3312000>;
+ regulator-allow-set-load;
};
vreg_l24a_3p075: l24 {
regulator-min-microvolt = <3088000>;
regulator-max-microvolt = <3088000>;
+ regulator-allow-set-load;
};
vreg_l25a_3p3: l25 {
regulator-min-microvolt = <3104000>;
regulator-max-microvolt = <3312000>;
+ regulator-allow-set-load;
};
vreg_l26a_1p2: l26 {
regulator-min-microvolt = <1200000>;
regulator-max-microvolt = <1200000>;
+ regulator-allow-set-load;
};
vreg_l28_3p0: l28 {
regulator-min-microvolt = <3008000>;
regulator-max-microvolt = <3008000>;
+ regulator-allow-set-load;
};
vreg_lvs1a_1p8: lvs1 {
regulator-min-microvolt = <1800000>;
regulator-max-microvolt = <1800000>;
+ regulator-allow-set-load;
};
vreg_lvs2a_1p8: lvs2 {
regulator-min-microvolt = <1800000>;
regulator-max-microvolt = <1800000>;
+ regulator-allow-set-load;
};
};
--
2.17.1
On 1/16/2019 3:56 AM, Marc Gonzalez wrote:
> Add host controller and PHY DT nodes.
>
> Signed-off-by: Marc Gonzalez <[email protected]>
> ---
> TODO: check whether the driver uses the 'resets' prop
> ---
> arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi | 20 +++++++
> arch/arm64/boot/dts/qcom/msm8998.dtsi | 63 +++++++++++++++++++++++
> 2 files changed, 83 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
> index 50e9033aa7f6..cd1c9e84eab7 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
> @@ -257,3 +257,23 @@
> pinctrl-0 = <&sdc2_clk_on &sdc2_cmd_on &sdc2_data_on &sdc2_cd_on>;
> pinctrl-1 = <&sdc2_clk_off &sdc2_cmd_off &sdc2_data_off &sdc2_cd_off>;
> };
> +
> +&ufshc {
> + vdd-hba-fixed-regulator;
Since we are not specifying the vdd anymore, I suspect this should be
dropped. Do you know of any reason why we'd still need it?
> + vcc-supply = <&vreg_l20a_2p95>;
> + vccq-supply = <&vreg_l26a_1p2>;
> + vccq2-supply = <&vreg_s4a_1p8>;
> + vcc-max-microamp = <750000>;
> + vccq-max-microamp = <560000>;
> + vccq2-max-microamp = <750000>;
> +};
> +
> +&ufsphy {
> + vdda-phy-supply = <&vreg_l1a_0p875>;
> + vdda-pll-supply = <&vreg_l2a_1p2>;
> + vddp-ref-clk-supply = <&vreg_l26a_1p2>;
> + vdda-phy-max-microamp = <51400>;
> + vdda-pll-max-microamp = <14600>;
> + vddp-ref-clk-max-microamp = <100>;
> + vddp-ref-clk-always-on;
> +};
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index 6f4f4b79853b..36fd2e614464 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> @@ -711,6 +711,69 @@
> redistributor-stride = <0x0 0x20000>;
> interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> };
> +
> + ufshc: ufshc@1da4000 {
> + compatible = "qcom,msm8998-ufshc", "qcom,ufshc",
> + "jedec,ufs-2.0";
> + reg = <0x1da4000 0x2500>;
Bjorn would like it if reg addresses are full width, ie 0x01da4000
> + interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>;
> + phys = <&ufsphy_lanes>;
> + phy-names = "ufsphy";
> + lanes-per-direction = <2>;
> + power-domains = <&gcc UFS_GDSC>;
> +
> + clock-names =
> + "core_clk",
> + "bus_aggr_clk",
> + "iface_clk",
> + "core_clk_unipro",
> + "ref_clk",
> + "tx_lane0_sync_clk",
> + "rx_lane0_sync_clk",
> + "rx_lane1_sync_clk";
> + clocks =
> + <&gcc GCC_UFS_AXI_CLK>,
> + <&gcc GCC_AGGRE1_UFS_AXI_CLK>,
> + <&gcc GCC_UFS_AHB_CLK>,
> + <&gcc GCC_UFS_UNIPRO_CORE_CLK>,
> + <&rpmcc RPM_SMD_LN_BB_CLK1>,
> + <&gcc GCC_UFS_TX_SYMBOL_0_CLK>,
> + <&gcc GCC_UFS_RX_SYMBOL_0_CLK>,
> + <&gcc GCC_UFS_RX_SYMBOL_1_CLK>;
> + freq-table-hz =
> + <50000000 200000000>,
> + <0 0>,
> + <0 0>,
> + <37500000 150000000>,
> + <0 0>,
> + <0 0>,
> + <0 0>,
> + <0 0>;
> +
> + resets = <&gcc GCC_UFS_BCR>;
> + reset-names = "rst";
> + };
> +
> + ufsphy: phy@1da7000 {
> + compatible = "qcom,sdm845-qmp-ufs-phy";
We should make an 8998 compatible. Also, don't you have phy changes
since the init sequence differs between 845 and 8998?
> + reg = <0x1da7000 0x18c>;
0x01da7000, see above comment
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> + clock-names = "ref", "ref_aux";
> + clocks =
> + <&gcc GCC_UFS_CLKREF_CLK>,
> + <&gcc GCC_UFS_PHY_AUX_CLK>;
> +
> + ufsphy_lanes: lanes@1da7400 {
> + reg = <0x1da7400 0x128>,
> + <0x1da7600 0x1fc>,
> + <0x1da7c00 0x1dc>,
> + <0x1da7800 0x128>,
> + <0x1da7a00 0x1fc>;
> + #phy-cells = <0>;
> + };
> + };
> };
> };
>
>
--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
On 16/01/2019 16:36, Jeffrey Hugo wrote:
> On 1/16/2019 3:56 AM, Marc Gonzalez wrote:
>
>> Add host controller and PHY DT nodes.
>>
>> Signed-off-by: Marc Gonzalez <[email protected]>
>> ---
>> TODO: check whether the driver uses the 'resets' prop
>> ---
>> arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi | 20 +++++++
>> arch/arm64/boot/dts/qcom/msm8998.dtsi | 63 +++++++++++++++++++++++
>> 2 files changed, 83 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
>> index 50e9033aa7f6..cd1c9e84eab7 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
>> @@ -257,3 +257,23 @@
>> pinctrl-0 = <&sdc2_clk_on &sdc2_cmd_on &sdc2_data_on &sdc2_cd_on>;
>> pinctrl-1 = <&sdc2_clk_off &sdc2_cmd_off &sdc2_data_off &sdc2_cd_off>;
>> };
>> +
>> +&ufshc {
>> + vdd-hba-fixed-regulator;
>
> Since we are not specifying the vdd anymore, I suspect this should be
> dropped. Do you know of any reason why we'd still need it?
Will drop in v3.
>> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
>> index 6f4f4b79853b..36fd2e614464 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
>> @@ -711,6 +711,69 @@
>> redistributor-stride = <0x0 0x20000>;
>> interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> };
>> +
>> + ufshc: ufshc@1da4000 {
>> + compatible = "qcom,msm8998-ufshc", "qcom,ufshc",
>> + "jedec,ufs-2.0";
>> + reg = <0x1da4000 0x2500>;
>
> Bjorn would like it if reg addresses are full width, ie 0x01da4000
Will tweak in v3.
>> + ufsphy: phy@1da7000 {
>> + compatible = "qcom,sdm845-qmp-ufs-phy";
>
> We should make an 8998 compatible. Also, don't you have phy changes
> since the init sequence differs between 845 and 8998?
Will create a specific binding.
I don't have any PHY changes, I just used the 845 init sequence.
I tested this by using the 845 init sequence downstream.
However, no point in sending v3 until someone comments on patches 3 and 4 :-)
Patch 4 needs to become a real patch.
Regards.