Hello,
This series adds support for the UFS host controller on
APQ8098/MSM8998-based boards.
I CCed the regulator maintainers to discuss the revert in
patch 6. Basically, the original patch added a quirk which
forcefully disables vccq when the UFSHC is connected to a
Samsung or Hynix Flash chip. Problem is, this disabling
breaks init on my board, and Jeffrey's board as well.
Hence the revert, for which I don't see any adverse
consequences?
Differences between v3 and v4:
- Rebase on top of -next
- Pick up Douglas Anderson's UFSHC doc fix
- Document 8998 UFSHC binding
- Improve UFS PHY binding doc
- Put the UFS DT patch at the end of the series
Difference between v2 and v3:
- Add qcom,msm8998-qmp-ufs-phy compat string and match it in the PHY driver
- Drop vdd-hba-fixed-regulator
- Write the reg addresses with full 32-bit width
- Set regulator-allow-set-load only on the 3 rails used by UFS.
- Revert the patch introducing ufshcd_set_vccq_rail_unused
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
Douglas Anderson (1):
dt-bindings: ufs: Fix the compatible string definition
Marc Gonzalez (6):
dt-bindings: ufs: Add msm8998 compatible string
dt-bindings: phy-qcom-qmp: Add qcom,msm8998-qmp-ufs-phy
phy: qcom-qmp: Add QMP UFS PHY support for msm8998
arm64: dts: qcom: msm8998: Allow drivers to set-load
Revert "scsi: ufs: disable vccq if it's not needed by UFS device"
arm64: dts: qcom: msm8998: Add UFS nodes
.../devicetree/bindings/phy/qcom-qmp-phy.txt | 4 ++
.../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 14 +++--
arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi | 22 +++++++
arch/arm64/boot/dts/qcom/msm8998.dtsi | 62 +++++++++++++++++++
drivers/phy/qualcomm/phy-qcom-qmp.c | 3 +
drivers/scsi/ufs/ufs.h | 1 -
drivers/scsi/ufs/ufshcd.c | 59 ++----------------
7 files changed, 104 insertions(+), 61 deletions(-)
--
2.17.1
From: Douglas Anderson <[email protected]>
If you look at the bindings for the UFS Host Controller it says:
- compatible: must contain "jedec,ufs-1.1" or "jedec,ufs-2.0", may
also list one or more of the following:
"qcom,msm8994-ufshc"
"qcom,msm8996-ufshc"
"qcom,ufshc"
My reading of that is that it's fine to just have either of these:
1. "qcom,msm8996-ufshc", "jedec,ufs-2.0"
2. "qcom,ufshc", "jedec,ufs-2.0"
As far as I can tell neither of the above is actually a good idea.
For #1 it turns out that the driver currently only keys off the
compatible string "qcom,ufshc" so it won't actually probe.
For #2 the driver won't probe but it's not a good idea to keep the SoC
name out of the compatible string.
Let's update the compatible string to make it really explicit. We'll
include a nod to the existing driver and the old binding and say that
we should always include the "qcom,ufshc" string in addition to the
SoC compatible string.
While we're at it we'll also include another example SoC known to have
UFS: sdm845.
Fixes: 47555a5c8a11 ("scsi: ufs: make the UFS variant a platform device")
Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Vivek Gautam <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
index 8cf59452c675..5111e9130bc3 100644
--- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
+++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
@@ -4,11 +4,14 @@ UFSHC nodes are defined to describe on-chip UFS host controllers.
Each UFS controller instance should have its own node.
Required properties:
-- compatible : must contain "jedec,ufs-1.1" or "jedec,ufs-2.0", may
- also list one or more of the following:
- "qcom,msm8994-ufshc"
- "qcom,msm8996-ufshc"
- "qcom,ufshc"
+- compatible : must contain "jedec,ufs-1.1" or "jedec,ufs-2.0"
+
+ For Qualcomm SoCs must contain, as below, an
+ SoC-specific compatible along with "qcom,ufshc" and
+ the appropriate jedec string:
+ "qcom,msm8994-ufshc", "qcom,ufshc", "jedec,ufs-2.0"
+ "qcom,msm8996-ufshc", "qcom,ufshc", "jedec,ufs-2.0"
+ "qcom,sdm845-ufshc", "qcom,ufshc", "jedec,ufs-2.0"
- interrupts : <interrupt mapping for UFS host controller IRQ>
- reg : <registers mapping>
--
2.17.1
Add compatible string for QMP UFS phy on msm8998.
Signed-off-by: Marc Gonzalez <[email protected]>
---
Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
index 4ff26dbf4310..5d181fc3cc18 100644
--- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
+++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
@@ -10,6 +10,7 @@ Required properties:
"qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996,
"qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996,
"qcom,msm8998-qmp-usb3-phy" for USB3 QMP V3 phy on msm8998,
+ "qcom,msm8998-qmp-ufs-phy" for UFS QMP phy on msm8998,
"qcom,sdm845-qmp-usb3-phy" for USB3 QMP V3 phy on sdm845,
"qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845,
"qcom,sdm845-qmp-ufs-phy" for UFS QMP phy on sdm845.
@@ -45,6 +46,8 @@ Required properties:
"aux", "cfg_ahb", "ref".
For "qcom,msm8998-qmp-usb3-phy" must contain:
"aux", "cfg_ahb", "ref".
+ For "qcom,msm8998-qmp-ufs-phy" must contain:
+ "ref", "ref_aux".
For "qcom,sdm845-qmp-usb3-phy" must contain:
"aux", "cfg_ahb", "ref", "com_aux".
For "qcom,sdm845-qmp-usb3-uni-phy" must contain:
@@ -66,6 +69,7 @@ Required properties:
"phy", "common".
For "qcom,msm8998-qmp-usb3-phy" must contain
"phy", "common".
+ For "qcom,msm8998-qmp-ufs-phy": no resets are listed.
For "qcom,sdm845-qmp-usb3-phy" must contain:
"phy", "common".
For "qcom,sdm845-qmp-usb3-uni-phy" must contain:
--
2.17.1
Use same init sequence as sdm845.
Reviewed-by: Jeffrey Hugo <[email protected]>
Signed-off-by: Marc Gonzalez <[email protected]>
---
drivers/phy/qualcomm/phy-qcom-qmp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index daf751500232..08d6f6f7f039 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -1872,6 +1872,9 @@ static const struct of_device_id qcom_qmp_phy_of_match_table[] = {
}, {
.compatible = "qcom,msm8996-qmp-usb3-phy",
.data = &msm8996_usb3phy_cfg,
+ }, {
+ .compatible = "qcom,msm8998-qmp-ufs-phy",
+ .data = &sdm845_ufsphy_cfg,
}, {
.compatible = "qcom,ipq8074-qmp-pcie-phy",
.data = &ipq8074_pciephy_cfg,
--
2.17.1
Add "qcom,msm8998-ufshc" compatible string.
Signed-off-by: Marc Gonzalez <[email protected]>
---
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
index 5111e9130bc3..991e21ee7b44 100644
--- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
+++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
@@ -11,6 +11,7 @@ Required properties:
the appropriate jedec string:
"qcom,msm8994-ufshc", "qcom,ufshc", "jedec,ufs-2.0"
"qcom,msm8996-ufshc", "qcom,ufshc", "jedec,ufs-2.0"
+ "qcom,msm8998-ufshc", "qcom,ufshc", "jedec,ufs-2.0"
"qcom,sdm845-ufshc", "qcom,ufshc", "jedec,ufs-2.0"
- interrupts : <interrupt mapping for UFS host controller IRQ>
- reg : <registers mapping>
--
2.17.1
The UFS host controller driver needs to set the load on 3 power rails
(l20, l26, s4) but the operation fails silently unless we specify the
regulator-allow-set-load property in the corresponding DT nodes.
Reviewed-by: Jeffrey Hugo <[email protected]>
Signed-off-by: Marc Gonzalez <[email protected]>
---
arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
index f0901067b043..19775cae1381 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
@@ -111,6 +111,7 @@
vreg_s4a_1p8: s4 {
regulator-min-microvolt = <1800000>;
regulator-max-microvolt = <1800000>;
+ regulator-allow-set-load;
};
vreg_s5a_2p04: s5 {
regulator-min-microvolt = <1904000>;
@@ -195,6 +196,7 @@
vreg_l20a_2p95: l20 {
regulator-min-microvolt = <2960000>;
regulator-max-microvolt = <2960000>;
+ regulator-allow-set-load;
};
vreg_l21a_2p95: l21 {
regulator-min-microvolt = <2960000>;
@@ -221,6 +223,7 @@
vreg_l26a_1p2: l26 {
regulator-min-microvolt = <1200000>;
regulator-max-microvolt = <1200000>;
+ regulator-allow-set-load;
};
vreg_l28_3p0: l28 {
regulator-min-microvolt = <3008000>;
--
2.17.1
This reverts commit 60f0187031c05e04cbadffb62f557d0ff3564490.
Calling ufshcd_set_vccq_rail_unused() breaks UFS init on two boards.
I would say that vccq is *not* not needed.
Reviewed-by: Jeffrey Hugo <[email protected]>
Signed-off-by: Marc Gonzalez <[email protected]>
---
drivers/scsi/ufs/ufs.h | 1 -
drivers/scsi/ufs/ufshcd.c | 59 +++------------------------------------
2 files changed, 4 insertions(+), 56 deletions(-)
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 6d176815e6ce..21e4ccb5ba6e 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -514,7 +514,6 @@ struct ufs_vreg {
struct regulator *reg;
const char *name;
bool enabled;
- bool unused;
int min_uV;
int max_uV;
int min_uA;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 4bc55a2f5689..c78f5f199aff 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -254,7 +254,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba);
static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
bool skip_ref_clk);
static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on);
-static int ufshcd_set_vccq_rail_unused(struct ufs_hba *hba, bool unused);
static int ufshcd_uic_hibern8_exit(struct ufs_hba *hba);
static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba);
static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba);
@@ -5857,11 +5856,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
ufs_fixup_device_setup(hba, &card);
ufshcd_tune_unipro_params(hba);
- ret = ufshcd_set_vccq_rail_unused(hba,
- (hba->dev_quirks & UFS_DEVICE_NO_VCCQ) ? true : false);
- if (ret)
- goto out;
-
/* UFS device is also active now */
ufshcd_set_ufs_dev_active(hba);
ufshcd_force_reset_auto_bkops(hba);
@@ -6030,24 +6024,13 @@ static int ufshcd_config_vreg_load(struct device *dev, struct ufs_vreg *vreg,
static inline int ufshcd_config_vreg_lpm(struct ufs_hba *hba,
struct ufs_vreg *vreg)
{
- if (!vreg)
- return 0;
- else if (vreg->unused)
- return 0;
- else
- return ufshcd_config_vreg_load(hba->dev, vreg,
- UFS_VREG_LPM_LOAD_UA);
+ return ufshcd_config_vreg_load(hba->dev, vreg, UFS_VREG_LPM_LOAD_UA);
}
static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
struct ufs_vreg *vreg)
{
- if (!vreg)
- return 0;
- else if (vreg->unused)
- return 0;
- else
- return ufshcd_config_vreg_load(hba->dev, vreg, vreg->max_uA);
+ return ufshcd_config_vreg_load(hba->dev, vreg, vreg->max_uA);
}
static int ufshcd_config_vreg(struct device *dev,
@@ -6085,9 +6068,7 @@ static int ufshcd_enable_vreg(struct device *dev, struct ufs_vreg *vreg)
{
int ret = 0;
- if (!vreg)
- goto out;
- else if (vreg->enabled || vreg->unused)
+ if (!vreg || vreg->enabled)
goto out;
ret = ufshcd_config_vreg(dev, vreg, true);
@@ -6107,9 +6088,7 @@ static int ufshcd_disable_vreg(struct device *dev, struct ufs_vreg *vreg)
{
int ret = 0;
- if (!vreg)
- goto out;
- else if (!vreg->enabled || vreg->unused)
+ if (!vreg || !vreg->enabled)
goto out;
ret = regulator_disable(vreg->reg);
@@ -6215,36 +6194,6 @@ static int ufshcd_init_hba_vreg(struct ufs_hba *hba)
return 0;
}
-static int ufshcd_set_vccq_rail_unused(struct ufs_hba *hba, bool unused)
-{
- int ret = 0;
- struct ufs_vreg_info *info = &hba->vreg_info;
-
- if (!info)
- goto out;
- else if (!info->vccq)
- goto out;
-
- if (unused) {
- /* shut off the rail here */
- ret = ufshcd_toggle_vreg(hba->dev, info->vccq, false);
- /*
- * Mark this rail as no longer used, so it doesn't get enabled
- * later by mistake
- */
- if (!ret)
- info->vccq->unused = true;
- } else {
- /*
- * rail should have been already enabled hence just make sure
- * that unused flag is cleared.
- */
- info->vccq->unused = false;
- }
-out:
- return ret;
-}
-
static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
bool skip_ref_clk)
{
--
2.17.1
Add UFS host controller and PHY DT nodes for msm8998.
Signed-off-by: Marc Gonzalez <[email protected]>
---
arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi | 19 +++++++
arch/arm64/boot/dts/qcom/msm8998.dtsi | 62 +++++++++++++++++++++++
2 files changed, 81 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
index 19775cae1381..3e911488d4f3 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
@@ -284,3 +284,22 @@
vdda-phy-supply = <&vreg_l1a_0p875>;
vdda-pll-supply = <&vreg_l2a_1p2>;
};
+
+&ufshc {
+ 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 043ff2d2643c..d69401152089 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -983,6 +983,68 @@
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 = <0x01da4000 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,msm8998-qmp-ufs-phy";
+ reg = <0x01da7000 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 = <0x01da7400 0x128>,
+ <0x01da7600 0x1fc>,
+ <0x01da7c00 0x1dc>,
+ <0x01da7800 0x128>,
+ <0x01da7a00 0x1fc>;
+ #phy-cells = <0>;
+ };
+ };
};
};
--
2.17.1
Hi,
>
> This reverts commit 60f0187031c05e04cbadffb62f557d0ff3564490.
>
> Calling ufshcd_set_vccq_rail_unused() breaks UFS init on two boards.
> I would say that vccq is *not* not needed.
>
> Reviewed-by: Jeffrey Hugo <[email protected]>
> Signed-off-by: Marc Gonzalez <[email protected]>
Those tags got switched off.
I still think that If you are reverting the quirk implementation,
you should remove the quirk listing as well.
Also, as the v3 discussion held on several threads,
and new people might be joining in,
maybe you could reply to this patch with a couple of sentences summing-up
the various theories that this bring-up raised.
Thanks,
Avri
On 09/02/2019 10:07, Avri Altman wrote:
> On 08/02/2019 23:20, Marc Gonzalez wrote:
>
>> This reverts commit 60f0187031c05e04cbadffb62f557d0ff3564490.
>>
>> Calling ufshcd_set_vccq_rail_unused() breaks UFS init on two boards.
>> I would say that vccq is *not* not needed.
>>
>> Reviewed-by: Jeffrey Hugo <[email protected]>
>> Signed-off-by: Marc Gonzalez <[email protected]>
>
> Those tags got switched off.
What do you mean?
> I still think that If you are reverting the quirk implementation,
> you should remove the quirk listing as well.
You're right, of course. I just thought it could wait until
after this series was accepted, but I can fold it in v5, as
a follow-up patch.
> Also, as the v3 discussion held on several threads,
> and new people might be joining in,
> maybe you could reply to this patch with a couple of sentences summing-up
> the various theories that this bring-up raised.
Will do in v5, either in the commit message, or in the
sub-text that is not committed.
Regards.