Re: [RFC PATCH v2 2/4] arm64: dts: sdm845: add Inline Crypto Engine registers and clock

From: Stephen Boyd <swboyd@chromium.org>
To: Eric Biggers <ebiggers@kernel.org>,
	linux-arm-msm@vger.kernel.org, linux-scsi@vger.kernel.org
Cc: linux-block@vger.kernel.org, linux-fscrypt@vger.kernel.org,
	Alim Akhtar <alim.akhtar@samsung.com>,
	Andy Gross <agross@kernel.org>, Avri Altman <avri.altman@wdc.com>,
	Barani Muthukumaran <bmuthuku@qti.qualcomm.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Can Guo <cang@codeaurora.org>,
	Elliot Berman <eberman@codeaurora.org>,
	Jaegeuk Kim <jaegeuk@kernel.org>
Subject: Re: [RFC PATCH v2 2/4] arm64: dts: sdm845: add Inline Crypto Engine registers and clock
Date: Wed, 04 Mar 2020 09:08:34 -0800	[thread overview]
Message-ID: <158334171487.7173.5606223900174949177@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <20200304064942.371978-3-ebiggers@kernel.org>

Quoting Eric Biggers (2020-03-03 22:49:40)
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index d42302b8889b..dd6b4e596fcf 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -1367,7 +1367,9 @@ system-cache-controller@1100000 {
>                 ufs_mem_hc: ufshc@1d84000 {
>                         compatible = "qcom,sdm845-ufshc", "qcom,ufshc",
>                                      "jedec,ufs-2.0";
> -                       reg = <0 0x01d84000 0 0x2500>;
> +                       reg = <0 0x01d84000 0 0x2500>,
> +                             <0 0 0 0>,
> +                             <0 0x01d90000 0 0x8000>;

Nothing against this patch but the binding for ufs is really awful. It
doesn't indicate what the order of registers are, doesn't list what clks
are supposed to be there, has weird microamp properties that make no
sense, and has a freq-table-hz property that is almost always full of
zeroes because the driver is written in some weird qcom specific way.

It would be great to fix this binding and convert it to YAML so that we
can drop the cruft and clearly describe why this patch needs to
introduce a reg property that is all zeroes.

>                         interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>;
>                         phys = <&ufs_mem_phy_lanes>;
>                         phy-names = "ufsphy";
> @@ -1387,7 +1389,8 @@ ufs_mem_hc: ufshc@1d84000 {
>                                 "ref_clk",
>                                 "tx_lane0_sync_clk",
>                                 "rx_lane0_sync_clk",
> -                               "rx_lane1_sync_clk";
> +                               "rx_lane1_sync_clk",
> +                               "ice_core_clk";

Would be great to drop _clk postfix on all of these.

>                         clocks =
>                                 <&gcc GCC_UFS_PHY_AXI_CLK>,
>                                 <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>,
> @@ -1396,7 +1399,8 @@ ufs_mem_hc: ufshc@1d84000 {
>                                 <&rpmhcc RPMH_CXO_CLK>,
>                                 <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>,
>                                 <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>,
> -                               <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>;
> +                               <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>,
> +                               <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
>                         freq-table-hz =
>                                 <50000000 200000000>,
>                                 <0 0>,
> @@ -1405,7 +1409,8 @@ ufs_mem_hc: ufshc@1d84000 {
>                                 <0 0>,
>                                 <0 0>,
>                                 <0 0>,
> -                               <0 0>;
> +                               <0 0>,
> +                               <0 300000000>;

This can probably be done with assigned-clock-rates, but the driver is
bad.