On 11/01/2019 16:03, Jeffrey Hugo wrote:
> On 1/11/2019 7:50 AM, Marc Gonzalez wrote:
>
>> Add UFS host controller and PHY DT nodes.
>>
>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@xxxxxxx>
>> ---
>> arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi | 42 +++++++++++++++
>> arch/arm64/boot/dts/qcom/msm8998.dtsi | 66 +++++++++++++++++++++++
>> 2 files changed, 108 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
>> index 50e9033aa7f6..0fbc8ba9dc30 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
>> @@ -244,6 +244,24 @@
>>
>> &tlmm {
>> gpio-reserved-ranges = <0 4>, <81 4>;
>> +
>> + ufs_dev_reset_assert: ufs_dev_reset_assert {
>> + config {
>> + pins = "ufs_reset";
>> + bias-pull-down;
>> + drive-strength = <8>;
>> + output-low;
>> + };
>> + };
>> +
>> + ufs_dev_reset_deassert: ufs_dev_reset_deassert {
>> + config {
>> + pins = "ufs_reset";
>> + bias-pull-down;
>> + drive-strength = <8>;
>> + output-high;
>> + };
>> + };
>> };
>>
>> &sdhc2 {
>> @@ -257,3 +275,27 @@
>> 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-supply = <&gcc UFS_GDSC>; -EPROBE_DEFER ***/
>
> Downstream the GDSCs are also exposed as regulators. It doesn't appear
> that this is the case upstream. I think this should just be removed.
Makes sense. I only left that definition to elicit comments.
>> +&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;
>
> Oh? It is?
I just copied what I found downstream:
https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-cdp.dtsi?h=LE.UM.1.3.r3.25#n45
>> + clock-names =
>> + "core_clk",
>> + "bus_aggr_clk",
>> + "iface_clk",
>> + "core_clk_unipro",
>> + "core_clk_ice",
>
> I'm not positive the ICE clk is actually required.
I think I copied that part from the sdm845 DT node.
I'll try removing it.
>> + "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>,
>> + <&gcc GCC_UFS_ICE_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>,
>> + <75000000 300000000>,
>> + <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 0x200>;
>
> The proper length is:
> reg = <0x1da7000 0x18c>;
It seemed to me that the HW designer had decided to carve out 0x200-byte
sized regions for the different PHY blocks.
Do you mean there are no registers documented from 0x1da718c to 0x1da7200?
Is there nothing from 0x1da7200 to 0x1da7400?
>> + #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 0x200>,
>> + <0x1da7600 0x200>,
>> + <0x1da7c00 0x200>,
>> + <0x1da7800 0x200>,
>> + <0x1da7a00 0x200>;
>
> The proper lengths are:
> reg = <0x1da7400 0x128>,
> <0x1da7600 0x1fc>,
> <0x1da7c00 0x1dc>,
> <0x1da7800 0x128>,
> <0x1da7a00 0x1fc>;
It's weird to specify a length of 0x1fc IMO.
Is it because there's a register at +0x1f8 and none at +0x1fc?
(All this talk is academic, since the granularity of a virtual mapping
is minimum 4096 bytes page-aligned.)
Some maintainers suggest rounding the number up to the next power of 2.
I don't know what the convention is for qcom DTBs.
Regards.
