mbox series

[v2,00/15] dt-bindings: ufs: add common platform bindings + fixes

Message ID 20220222145854.358646-1-krzysztof.kozlowski@canonical.com
Headers show
Series dt-bindings: ufs: add common platform bindings + fixes | expand

Message

Krzysztof Kozlowski Feb. 22, 2022, 2:58 p.m. UTC
Hi,

Not tested on hardware, so please kindly test.
Bindings maintainers might need checking - taken from git log.

Changes since v1
================
1. Make freq-table as matrix of tuples (Nishanth).
2. New patches: convert all bindings and fix up DTS files.
3. Several minor fixes in UFS bindings.

Dependencies
============
1. The DTS patches (10-15) depend on scsi/ufs driver change (patch 9), so
   they should go in a following release, after merging driver.

   This is also might affect out-of-tree users of these DTSes (other
   projects, e.g. U-boot).

Best regards,
Krzysztof

Krzysztof Kozlowski (15):
  dt-bindings: ufs: add common platform bindings
  dt-bindings: ufs: samsung,exynos-ufs: use common bindings
  dt-bindings: ufs: cdns,ufshc: convert to dtschema
  dt-bindings: ufs: drop unused/old ufs-qcom PHY bindings
  dt-bindings: ufs: qcom,ufs: convert to dtschema
  dt-bindings: ufs: hisilicon,ufs: convert to dtschema
  dt-bindings: ufs: mediatek,ufs: convert to dtschema
  dt-bindings: ufs: snps,tc-dwc-g210: convert to dtschema
  scsi: ufs: deprecate 'freq-table-hz' property
  arm64: dts: hi3670: use 'freq-table' in UFS node
  arm64: dts: ti: use 'freq-table' in UFS node
  arm64: dts: qcom: use 'freq-table' in UFS node
  arm64: dts: qcom: msm8996: drop unsupported UFS
    vddp-ref-clk-max-microamp
  arm64: dts: qcom: msm8996: correct UFS compatible
  arm64: dts: qcom: sm8350: drop duplicated ref_clk in UFS

 .../devicetree/bindings/ufs/cdns,ufshc.txt    |  32 ---
 .../devicetree/bindings/ufs/cdns,ufshc.yaml   |  68 +++++
 .../bindings/ufs/hisilicon,ufs.yaml           |  90 +++++++
 .../devicetree/bindings/ufs/mediatek,ufs.yaml |  67 +++++
 .../devicetree/bindings/ufs/qcom,ufs.yaml     | 241 ++++++++++++++++++
 .../bindings/ufs/samsung,exynos-ufs.yaml      |  13 +-
 .../bindings/ufs/snps,tc-dwc-g210.yaml        |  51 ++++
 .../bindings/ufs/tc-dwc-g210-pltfrm.txt       |  26 --
 .../devicetree/bindings/ufs/ti,j721e-ufs.yaml |   9 +-
 .../devicetree/bindings/ufs/ufs-common.yaml   |  88 +++++++
 .../devicetree/bindings/ufs/ufs-hisi.txt      |  42 ---
 .../devicetree/bindings/ufs/ufs-mediatek.txt  |  45 ----
 .../devicetree/bindings/ufs/ufs-qcom.txt      |  63 -----
 .../devicetree/bindings/ufs/ufshcd-pltfrm.txt |  90 -------
 MAINTAINERS                                   |   1 +
 arch/arm64/boot/dts/hisilicon/hi3670.dtsi     |   4 +-
 .../boot/dts/qcom/msm8996-xiaomi-common.dtsi  |   1 -
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |   5 +-
 arch/arm64/boot/dts/qcom/msm8998.dtsi         |   2 +-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |   2 +-
 arch/arm64/boot/dts/qcom/sm8150.dtsi          |   2 +-
 arch/arm64/boot/dts/qcom/sm8250.dtsi          |   2 +-
 arch/arm64/boot/dts/qcom/sm8350.dtsi          |   5 +-
 arch/arm64/boot/dts/qcom/sm8450.dtsi          |   2 +-
 arch/arm64/boot/dts/ti/k3-j721e-main.dtsi     |   4 +-
 drivers/scsi/ufs/ufshcd-pltfrm.c              |  21 +-
 26 files changed, 641 insertions(+), 335 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/ufs/cdns,ufshc.txt
 create mode 100644 Documentation/devicetree/bindings/ufs/cdns,ufshc.yaml
 create mode 100644 Documentation/devicetree/bindings/ufs/hisilicon,ufs.yaml
 create mode 100644 Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
 create mode 100644 Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
 create mode 100644 Documentation/devicetree/bindings/ufs/snps,tc-dwc-g210.yaml
 delete mode 100644 Documentation/devicetree/bindings/ufs/tc-dwc-g210-pltfrm.txt
 create mode 100644 Documentation/devicetree/bindings/ufs/ufs-common.yaml
 delete mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt
 delete mode 100644 Documentation/devicetree/bindings/ufs/ufs-mediatek.txt
 delete mode 100644 Documentation/devicetree/bindings/ufs/ufs-qcom.txt
 delete mode 100644 Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt

Comments

Alim Akhtar Feb. 22, 2022, 4:44 p.m. UTC | #1
Hi Krzysztof

>-----Original Message-----
>From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@canonical.com]
>Sent: Tuesday, February 22, 2022 8:29 PM
>To: Alim Akhtar <alim.akhtar@samsung.com>; Avri Altman
><avri.altman@wdc.com>; Rob Herring <robh+dt@kernel.org>; Krzysztof
>Kozlowski <krzysztof.kozlowski@canonical.com>; Andy Gross
><agross@kernel.org>; Bjorn Andersson <bjorn.andersson@linaro.org>; Wei
>Xu <xuwei5@hisilicon.com>; Nishanth Menon <nm@ti.com>; Vignesh
>Raghavendra <vigneshr@ti.com>; Tero Kristo <kristo@kernel.org>; James E.J.
>Bottomley <jejb@linux.ibm.com>; Martin K. Petersen
><martin.petersen@oracle.com>; Jan Kotas <jank@cadence.com>; Li Wei
><liwei213@huawei.com>; Stanley Chu <stanley.chu@mediatek.com>; Yaniv
>Gardi <ygardi@codeaurora.org>; linux-scsi@vger.kernel.org;
>devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>msm@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-samsung-
>soc@vger.kernel.org; linux-mediatek@lists.infradead.org
>Subject: [PATCH v2 09/15] scsi: ufs: deprecate 'freq-table-hz' property
>
>The 'freq-table-hz' is not correct in DT schema, because '-hz' suffix
defines
>uint32 type, not an array.  Therefore deprecate 'freq-table-hz'
>and use 'freq-table' instead.
>
May be once all the dts change migrate to using 'freq-table', just remove
'freq-table-hz' from driver.

>Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>---
> drivers/scsi/ufs/ufshcd-pltfrm.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c
b/drivers/scsi/ufs/ufshcd-pltfrm.c
>index 87975d1a21c8..2b192477d158 100644
>--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
>+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
>@@ -47,8 +47,9 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
> 	if (cnt <= 0)
> 		goto out;
>
>-	if (!of_get_property(np, "freq-table-hz", &len)) {
>-		dev_info(dev, "freq-table-hz property not specified\n");
>+	if (!of_get_property(np, "freq-table", &len) ||
>+	    !of_get_property(np, "freq-table-hz", &len)) {
>+		dev_info(dev, "freq-table property not specified\n");
> 		goto out;
> 	}
>
>@@ -57,7 +58,7 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>
> 	sz = len / sizeof(*clkfreq);
> 	if (sz != 2 * cnt) {
>-		dev_err(dev, "%s len mismatch\n", "freq-table-hz");
>+		dev_err(dev, "%s len mismatch\n", "freq-table");
> 		ret = -EINVAL;
> 		goto out;
> 	}
>@@ -69,12 +70,16 @@ static int ufshcd_parse_clock_info(struct ufs_hba
>*hba)
> 		goto out;
> 	}
>
>-	ret = of_property_read_u32_array(np, "freq-table-hz",
>+	ret = of_property_read_u32_array(np, "freq-table",
> 			clkfreq, sz);
> 	if (ret && (ret != -EINVAL)) {
>-		dev_err(dev, "%s: error reading array %d\n",
>-				"freq-table-hz", ret);
>-		return ret;
>+		ret = of_property_read_u32_array(np, "freq-table-hz",
>+						 clkfreq, sz);
>+		if (ret && (ret != -EINVAL)) {
>+			dev_err(dev, "%s: error reading array %d\n",
>+				"freq-table", ret);
>+			return ret;
>+		}
> 	}
>
> 	for (i = 0; i < sz; i += 2) {
>@@ -99,7 +104,7 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>
> 		if (!strcmp(name, "ref_clk"))
> 			clki->keep_link_active = true;
>-		dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-
>hz",
>+		dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table",
> 				clki->min_freq, clki->max_freq, clki->name);
> 		list_add_tail(&clki->list, &hba->clk_list_head);
> 	}
>--
>2.32.0
Bjorn Andersson Feb. 22, 2022, 6:16 p.m. UTC | #2
On Tue 22 Feb 06:58 PST 2022, Krzysztof Kozlowski wrote:

> The 'freq-table-hz' is not correct in DT schema, because '-hz' suffix
> defines uint32 type, not an array.  Therefore deprecate 'freq-table-hz'
> and use 'freq-table' instead.
> 

Patch looks good in itself, but why don't we use opp-table to describe
the performance states?

In particular looking at the two columns of frequencies for various
Qualcomm boards they require different performance-states.

A concrete example is sm8350.dtsi, which specifies 75MHz and 300MHz as
the first frequency pair. The lower level requires the VDD_CX power rail
to be at least &rpmhpd_opp_low_svs, the higher frequency has a
required-opps of &rpmhpd_opp_nom.


As this isn't possible to express in the current binding we've just been
forced to always run at a higher voltage level and kept this in the todo
list.

But rather than migrating freq-table-hz to freq-table and then having to
introduce an opp table to express the power constraints, could we
perhaps skip the intermediate step?

Or would you have any other suggestion about how we can represent the
required-opps level together with the freq-table (if that's what we want
to stick with).

Regards,
Bjorn

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
>  drivers/scsi/ufs/ufshcd-pltfrm.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
> index 87975d1a21c8..2b192477d158 100644
> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
> @@ -47,8 +47,9 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>  	if (cnt <= 0)
>  		goto out;
>  
> -	if (!of_get_property(np, "freq-table-hz", &len)) {
> -		dev_info(dev, "freq-table-hz property not specified\n");
> +	if (!of_get_property(np, "freq-table", &len) ||
> +	    !of_get_property(np, "freq-table-hz", &len)) {
> +		dev_info(dev, "freq-table property not specified\n");
>  		goto out;
>  	}
>  
> @@ -57,7 +58,7 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>  
>  	sz = len / sizeof(*clkfreq);
>  	if (sz != 2 * cnt) {
> -		dev_err(dev, "%s len mismatch\n", "freq-table-hz");
> +		dev_err(dev, "%s len mismatch\n", "freq-table");
>  		ret = -EINVAL;
>  		goto out;
>  	}
> @@ -69,12 +70,16 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>  		goto out;
>  	}
>  
> -	ret = of_property_read_u32_array(np, "freq-table-hz",
> +	ret = of_property_read_u32_array(np, "freq-table",
>  			clkfreq, sz);
>  	if (ret && (ret != -EINVAL)) {
> -		dev_err(dev, "%s: error reading array %d\n",
> -				"freq-table-hz", ret);
> -		return ret;
> +		ret = of_property_read_u32_array(np, "freq-table-hz",
> +						 clkfreq, sz);
> +		if (ret && (ret != -EINVAL)) {
> +			dev_err(dev, "%s: error reading array %d\n",
> +				"freq-table", ret);
> +			return ret;
> +		}
>  	}
>  
>  	for (i = 0; i < sz; i += 2) {
> @@ -99,7 +104,7 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>  
>  		if (!strcmp(name, "ref_clk"))
>  			clki->keep_link_active = true;
> -		dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-hz",
> +		dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table",
>  				clki->min_freq, clki->max_freq, clki->name);
>  		list_add_tail(&clki->list, &hba->clk_list_head);
>  	}
> -- 
> 2.32.0
>
Krzysztof Kozlowski Feb. 23, 2022, 8:23 a.m. UTC | #3
On 22/02/2022 17:44, Alim Akhtar wrote:
> Hi Krzysztof
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@canonical.com]
>> Sent: Tuesday, February 22, 2022 8:29 PM
>> To: Alim Akhtar <alim.akhtar@samsung.com>; Avri Altman
>> <avri.altman@wdc.com>; Rob Herring <robh+dt@kernel.org>; Krzysztof
>> Kozlowski <krzysztof.kozlowski@canonical.com>; Andy Gross
>> <agross@kernel.org>; Bjorn Andersson <bjorn.andersson@linaro.org>; Wei
>> Xu <xuwei5@hisilicon.com>; Nishanth Menon <nm@ti.com>; Vignesh
>> Raghavendra <vigneshr@ti.com>; Tero Kristo <kristo@kernel.org>; James E.J.
>> Bottomley <jejb@linux.ibm.com>; Martin K. Petersen
>> <martin.petersen@oracle.com>; Jan Kotas <jank@cadence.com>; Li Wei
>> <liwei213@huawei.com>; Stanley Chu <stanley.chu@mediatek.com>; Yaniv
>> Gardi <ygardi@codeaurora.org>; linux-scsi@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>> msm@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-samsung-
>> soc@vger.kernel.org; linux-mediatek@lists.infradead.org
>> Subject: [PATCH v2 09/15] scsi: ufs: deprecate 'freq-table-hz' property
>>
>> The 'freq-table-hz' is not correct in DT schema, because '-hz' suffix
> defines
>> uint32 type, not an array.  Therefore deprecate 'freq-table-hz'
>> and use 'freq-table' instead.
>>
> May be once all the dts change migrate to using 'freq-table', just remove
> 'freq-table-hz' from driver.

At some point in the future 'freq-table-hz' parsing could be removed,
but now you have both old and new DTS, so it should stay. Also for DTB
compatibility.


Best regards,
Krzysztof
Krzysztof Kozlowski Feb. 23, 2022, 9:15 a.m. UTC | #4
On 22/02/2022 19:16, Bjorn Andersson wrote:
> On Tue 22 Feb 06:58 PST 2022, Krzysztof Kozlowski wrote:
> 
>> The 'freq-table-hz' is not correct in DT schema, because '-hz' suffix
>> defines uint32 type, not an array.  Therefore deprecate 'freq-table-hz'
>> and use 'freq-table' instead.
>>
> 
> Patch looks good in itself, but why don't we use opp-table to describe
> the performance states?
> 
> In particular looking at the two columns of frequencies for various
> Qualcomm boards they require different performance-states.
> 
> A concrete example is sm8350.dtsi, which specifies 75MHz and 300MHz as
> the first frequency pair. The lower level requires the VDD_CX power rail
> to be at least &rpmhpd_opp_low_svs, the higher frequency has a
> required-opps of &rpmhpd_opp_nom.
> 
> 
> As this isn't possible to express in the current binding we've just been
> forced to always run at a higher voltage level and kept this in the todo
> list.
> 
> But rather than migrating freq-table-hz to freq-table and then having to
> introduce an opp table to express the power constraints, could we
> perhaps skip the intermediate step?
> 
> Or would you have any other suggestion about how we can represent the
> required-opps level together with the freq-table (if that's what we want
> to stick with).

Usage of OPP tables is interesting solution. It would solve your problem
of power rail levels. This would need several opp-tables - one for each
clock, which is not a big problem.

The problem is that I do not have any UFS hardware (none of my Samsung
Exynos boards have UFS... I don't have even arm64 Exynos chips :( ), so
implementing it theoretically will be painful.
OTOH, I believe that having a working dtschema is very useful. Having
dtschema without errors/warnings is even worth some churn/intermediary work.

The intermediary work is also not that big. Once proper OPP is
implemented, we will have "just" two deprecated properties in the bindings.

Best regards,
Krzysztof
Bjorn Andersson Feb. 24, 2022, 4:51 p.m. UTC | #5
On Wed 23 Feb 03:15 CST 2022, Krzysztof Kozlowski wrote:

> On 22/02/2022 19:16, Bjorn Andersson wrote:
> > On Tue 22 Feb 06:58 PST 2022, Krzysztof Kozlowski wrote:
> > 
> >> The 'freq-table-hz' is not correct in DT schema, because '-hz' suffix
> >> defines uint32 type, not an array.  Therefore deprecate 'freq-table-hz'
> >> and use 'freq-table' instead.
> >>
> > 
> > Patch looks good in itself, but why don't we use opp-table to describe
> > the performance states?
> > 
> > In particular looking at the two columns of frequencies for various
> > Qualcomm boards they require different performance-states.
> > 
> > A concrete example is sm8350.dtsi, which specifies 75MHz and 300MHz as
> > the first frequency pair. The lower level requires the VDD_CX power rail
> > to be at least &rpmhpd_opp_low_svs, the higher frequency has a
> > required-opps of &rpmhpd_opp_nom.
> > 
> > 
> > As this isn't possible to express in the current binding we've just been
> > forced to always run at a higher voltage level and kept this in the todo
> > list.
> > 
> > But rather than migrating freq-table-hz to freq-table and then having to
> > introduce an opp table to express the power constraints, could we
> > perhaps skip the intermediate step?
> > 
> > Or would you have any other suggestion about how we can represent the
> > required-opps level together with the freq-table (if that's what we want
> > to stick with).
> 
> Usage of OPP tables is interesting solution. It would solve your problem
> of power rail levels. This would need several opp-tables - one for each
> clock, which is not a big problem.
> 

Ahh, so we can only have a single clock, but multiple regulators and
interconnect paths tied to the opp table.

We have a couple of cases where it would have been nice to be able to
key the opp-table off some index (e.g. the UFS gear or PCI Gen) and
control multiple clocks. So I think we need to look into this further...

> The problem is that I do not have any UFS hardware (none of my Samsung
> Exynos boards have UFS... I don't have even arm64 Exynos chips :( ), so
> implementing it theoretically will be painful.
> OTOH, I believe that having a working dtschema is very useful. Having
> dtschema without errors/warnings is even worth some churn/intermediary work.
> 
> The intermediary work is also not that big. Once proper OPP is
> implemented, we will have "just" two deprecated properties in the bindings.
> 

Fair enough, was just hoping to avoid the middle step. But that's fine,
we'll continue to carry this on our todo list then.

Thanks,
Bjorn
Rob Herring (Arm) Feb. 25, 2022, 7:26 p.m. UTC | #6
On Tue, 22 Feb 2022 15:58:41 +0100, Krzysztof Kozlowski wrote:
> Use common UFS bindings in Samsung Exynos UFS to cover generic/common
> properties in DTS.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
>  .../devicetree/bindings/ufs/samsung,exynos-ufs.yaml | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Rob Herring (Arm) Feb. 25, 2022, 7:28 p.m. UTC | #7
On Tue, 22 Feb 2022 15:58:42 +0100, Krzysztof Kozlowski wrote:
> Convert the Cadence Universal Flash Storage (UFS) Controlle to DT schema
> format.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
>  .../devicetree/bindings/ufs/cdns,ufshc.txt    | 32 ---------
>  .../devicetree/bindings/ufs/cdns,ufshc.yaml   | 68 +++++++++++++++++++
>  .../devicetree/bindings/ufs/ti,j721e-ufs.yaml |  7 +-
>  3 files changed, 71 insertions(+), 36 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/ufs/cdns,ufshc.txt
>  create mode 100644 Documentation/devicetree/bindings/ufs/cdns,ufshc.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Rob Herring (Arm) Feb. 25, 2022, 7:34 p.m. UTC | #8
On Tue, 22 Feb 2022 15:58:46 +0100, Krzysztof Kozlowski wrote:
> Convert the Mediatek Universal Flash Storage (UFS) Controller to DT
> schema format.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
>  .../devicetree/bindings/ufs/mediatek,ufs.yaml | 67 +++++++++++++++++++
>  .../devicetree/bindings/ufs/ufs-mediatek.txt  | 45 -------------
>  2 files changed, 67 insertions(+), 45 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/ufs/mediatek,ufs.yaml
>  delete mode 100644 Documentation/devicetree/bindings/ufs/ufs-mediatek.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>