| Message ID | 20230108195336.388349-5-they@mint.lgbt (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | arm64: dts: qcom: sm6125: UFS and xiaomi-laurel-sprout support | expand |
On 8.01.2023 20:53, Lux Aliaga wrote: > Adds a UFS host controller node and its corresponding PHY to > the sm6125 platform. > > Signed-off-by: Lux Aliaga <they@mint.lgbt> > --- > arch/arm64/boot/dts/qcom/sm6125.dtsi | 57 ++++++++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm6125.dtsi b/arch/arm64/boot/dts/qcom/sm6125.dtsi > index df5453fcf2b9..cec7071d5279 100644 > --- a/arch/arm64/boot/dts/qcom/sm6125.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm6125.dtsi > @@ -511,6 +511,63 @@ sdhc_2: mmc@4784000 { > status = "disabled"; > }; > > + ufs_mem_hc: ufs@4804000 { > + compatible = "qcom,sm6125-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; > + reg = <0x04804000 0x3000>, <0x04810000 0x8000>; You need reg-names for ICE to probe, otherwise the second reg sits unused. > + interrupts = <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>; > + phys = <&ufs_mem_phy>; > + phy-names = "ufsphy"; > + lanes-per-direction = <1>; > + #reset-cells = <1>; > + resets = <&gcc GCC_UFS_PHY_BCR>; > + reset-names = "rst"; > + iommus = <&apps_smmu 0x200 0x0>; > + > + clock-names = "core_clk", > + "bus_aggr_clk", > + "iface_clk", > + "core_clk_unipro", > + "ref_clk", > + "tx_lane0_sync_clk", > + "rx_lane0_sync_clk", > + "ice_core_clk"; > + clocks = <&gcc GCC_UFS_PHY_AXI_CLK>, > + <&gcc GCC_SYS_NOC_UFS_PHY_AXI_CLK>, > + <&gcc GCC_UFS_PHY_AHB_CLK>, > + <&gcc GCC_UFS_PHY_UNIPRO_CORE_CLK>, > + <&rpmcc RPM_SMD_XO_CLK_SRC>, > + <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>, > + <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, > + <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; > + freq-table-hz = <50000000 240000000>, > + <0 0>, > + <0 0>, > + <37500000 150000000>, > + <0 0>, > + <0 0>, > + <0 0>, > + <75000000 300000000>; > + > + status = "disabled"; > + }; > + > + ufs_mem_phy: phy@4807000 { > + compatible = "qcom,sm6125-qmp-ufs-phy"; > + reg = <0x04807000 0x1c4>; Isn't this too small? Downstream says 0xdb8, but it's probably even bigger.. > + > + clock-names = "ref", "ref_aux"; We recently started an endless quest, trying to unify property order [1], please adjust this to: compat reg clocks clock-names [freq-table-hz seems fitting here tbf] resets reset-names power-domains #phy-cells status so in your case, put power-domains after reset-names and flip clocks/clock-names similarly for the node above this order would look good: compat reg reg-names interrupts clocks clock-names resets reset-names reset-cells phys phy-names lines-per-direction iommus status Konrad [1] https://github.com/konradybcio-work/dt_review > + clocks = <&gcc GCC_UFS_MEM_CLKREF_CLK>, <&gcc GCC_UFS_PHY_PHY_AUX_CLK>; > + > + power-domains = <&gcc UFS_PHY_GDSC>; > + > + resets = <&ufs_mem_hc 0>; > + reset-names = "ufsphy"; > + > + #phy-cells = <0>; > + > + status = "disabled"; > + }; > + > gpi_dma0: dma-controller@4a00000 { > compatible = "qcom,sm6125-gpi-dma", "qcom,sdm845-gpi-dma"; > reg = <0x04a00000 0x60000>;
On 09/01/2023 09:18, Konrad Dybcio wrote: > > On 8.01.2023 20:53, Lux Aliaga wrote: >> Adds a UFS host controller node and its corresponding PHY to >> the sm6125 platform. >> >> Signed-off-by: Lux Aliaga <they@mint.lgbt> >> --- >> arch/arm64/boot/dts/qcom/sm6125.dtsi | 57 ++++++++++++++++++++++++++++ >> 1 file changed, 57 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sm6125.dtsi b/arch/arm64/boot/dts/qcom/sm6125.dtsi >> index df5453fcf2b9..cec7071d5279 100644 >> --- a/arch/arm64/boot/dts/qcom/sm6125.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm6125.dtsi >> @@ -511,6 +511,63 @@ sdhc_2: mmc@4784000 { >> status = "disabled"; >> }; >> >> + ufs_mem_hc: ufs@4804000 { >> + compatible = "qcom,sm6125-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; >> + reg = <0x04804000 0x3000>, <0x04810000 0x8000>; > You need reg-names for ICE to probe, otherwise the second reg sits unused. > >> + interrupts = <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>; >> + phys = <&ufs_mem_phy>; >> + phy-names = "ufsphy"; >> + lanes-per-direction = <1>; >> + #reset-cells = <1>; >> + resets = <&gcc GCC_UFS_PHY_BCR>; >> + reset-names = "rst"; >> + iommus = <&apps_smmu 0x200 0x0>; >> + >> + clock-names = "core_clk", >> + "bus_aggr_clk", >> + "iface_clk", >> + "core_clk_unipro", >> + "ref_clk", >> + "tx_lane0_sync_clk", >> + "rx_lane0_sync_clk", >> + "ice_core_clk"; >> + clocks = <&gcc GCC_UFS_PHY_AXI_CLK>, >> + <&gcc GCC_SYS_NOC_UFS_PHY_AXI_CLK>, >> + <&gcc GCC_UFS_PHY_AHB_CLK>, >> + <&gcc GCC_UFS_PHY_UNIPRO_CORE_CLK>, >> + <&rpmcc RPM_SMD_XO_CLK_SRC>, >> + <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>, >> + <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, >> + <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; >> + freq-table-hz = <50000000 240000000>, >> + <0 0>, >> + <0 0>, >> + <37500000 150000000>, >> + <0 0>, >> + <0 0>, >> + <0 0>, >> + <75000000 300000000>; >> + >> + status = "disabled"; >> + }; >> + >> + ufs_mem_phy: phy@4807000 { >> + compatible = "qcom,sm6125-qmp-ufs-phy"; >> + reg = <0x04807000 0x1c4>; > Isn't this too small? Downstream says 0xdb8, but it's probably even bigger.. What do you think could help me find the new length of the registers? I tried 0x1000 and it probed just fine, but I'm not really sure until what extent I could push it.
On 11.01.2023 03:53, Lux Aliaga wrote: > > On 09/01/2023 09:18, Konrad Dybcio wrote: >> >> On 8.01.2023 20:53, Lux Aliaga wrote: >>> Adds a UFS host controller node and its corresponding PHY to >>> the sm6125 platform. >>> >>> Signed-off-by: Lux Aliaga <they@mint.lgbt> >>> --- >>> arch/arm64/boot/dts/qcom/sm6125.dtsi | 57 ++++++++++++++++++++++++++++ >>> 1 file changed, 57 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/sm6125.dtsi b/arch/arm64/boot/dts/qcom/sm6125.dtsi >>> index df5453fcf2b9..cec7071d5279 100644 >>> --- a/arch/arm64/boot/dts/qcom/sm6125.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sm6125.dtsi >>> @@ -511,6 +511,63 @@ sdhc_2: mmc@4784000 { >>> status = "disabled"; >>> }; >>> + ufs_mem_hc: ufs@4804000 { >>> + compatible = "qcom,sm6125-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; >>> + reg = <0x04804000 0x3000>, <0x04810000 0x8000>; >> You need reg-names for ICE to probe, otherwise the second reg sits unused. >> >>> + interrupts = <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>; >>> + phys = <&ufs_mem_phy>; >>> + phy-names = "ufsphy"; >>> + lanes-per-direction = <1>; >>> + #reset-cells = <1>; >>> + resets = <&gcc GCC_UFS_PHY_BCR>; >>> + reset-names = "rst"; >>> + iommus = <&apps_smmu 0x200 0x0>; >>> + >>> + clock-names = "core_clk", >>> + "bus_aggr_clk", >>> + "iface_clk", >>> + "core_clk_unipro", >>> + "ref_clk", >>> + "tx_lane0_sync_clk", >>> + "rx_lane0_sync_clk", >>> + "ice_core_clk"; >>> + clocks = <&gcc GCC_UFS_PHY_AXI_CLK>, >>> + <&gcc GCC_SYS_NOC_UFS_PHY_AXI_CLK>, >>> + <&gcc GCC_UFS_PHY_AHB_CLK>, >>> + <&gcc GCC_UFS_PHY_UNIPRO_CORE_CLK>, >>> + <&rpmcc RPM_SMD_XO_CLK_SRC>, >>> + <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>, >>> + <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, >>> + <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; >>> + freq-table-hz = <50000000 240000000>, >>> + <0 0>, >>> + <0 0>, >>> + <37500000 150000000>, >>> + <0 0>, >>> + <0 0>, >>> + <0 0>, >>> + <75000000 300000000>; >>> + >>> + status = "disabled"; >>> + }; >>> + >>> + ufs_mem_phy: phy@4807000 { >>> + compatible = "qcom,sm6125-qmp-ufs-phy"; >>> + reg = <0x04807000 0x1c4>; >> Isn't this too small? Downstream says 0xdb8, but it's probably even bigger.. > What do you think could help me find the new length of the registers? I tried 0x1000 and it probed just fine, but I'm not really sure until what extent I could push it. The "true" values are probably only in documentation, which I don't have. Konrad >
On 11/01/2023 12:04, Konrad Dybcio wrote: > > > On 11.01.2023 03:53, Lux Aliaga wrote: >> >> On 09/01/2023 09:18, Konrad Dybcio wrote: >>> >>> On 8.01.2023 20:53, Lux Aliaga wrote: >>>> Adds a UFS host controller node and its corresponding PHY to >>>> the sm6125 platform. >>>> >>>> Signed-off-by: Lux Aliaga <they@mint.lgbt> >>>> --- >>>> arch/arm64/boot/dts/qcom/sm6125.dtsi | 57 ++++++++++++++++++++++++++++ >>>> 1 file changed, 57 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/qcom/sm6125.dtsi b/arch/arm64/boot/dts/qcom/sm6125.dtsi >>>> index df5453fcf2b9..cec7071d5279 100644 >>>> --- a/arch/arm64/boot/dts/qcom/sm6125.dtsi >>>> +++ b/arch/arm64/boot/dts/qcom/sm6125.dtsi >>>> @@ -511,6 +511,63 @@ sdhc_2: mmc@4784000 { >>>> status = "disabled"; >>>> }; >>>> + ufs_mem_hc: ufs@4804000 { >>>> + compatible = "qcom,sm6125-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; >>>> + reg = <0x04804000 0x3000>, <0x04810000 0x8000>; >>> You need reg-names for ICE to probe, otherwise the second reg sits unused. >>> >>>> + interrupts = <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>; >>>> + phys = <&ufs_mem_phy>; >>>> + phy-names = "ufsphy"; >>>> + lanes-per-direction = <1>; >>>> + #reset-cells = <1>; >>>> + resets = <&gcc GCC_UFS_PHY_BCR>; >>>> + reset-names = "rst"; >>>> + iommus = <&apps_smmu 0x200 0x0>; >>>> + >>>> + clock-names = "core_clk", >>>> + "bus_aggr_clk", >>>> + "iface_clk", >>>> + "core_clk_unipro", >>>> + "ref_clk", >>>> + "tx_lane0_sync_clk", >>>> + "rx_lane0_sync_clk", >>>> + "ice_core_clk"; >>>> + clocks = <&gcc GCC_UFS_PHY_AXI_CLK>, >>>> + <&gcc GCC_SYS_NOC_UFS_PHY_AXI_CLK>, >>>> + <&gcc GCC_UFS_PHY_AHB_CLK>, >>>> + <&gcc GCC_UFS_PHY_UNIPRO_CORE_CLK>, >>>> + <&rpmcc RPM_SMD_XO_CLK_SRC>, >>>> + <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>, >>>> + <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, >>>> + <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; >>>> + freq-table-hz = <50000000 240000000>, >>>> + <0 0>, >>>> + <0 0>, >>>> + <37500000 150000000>, >>>> + <0 0>, >>>> + <0 0>, >>>> + <0 0>, >>>> + <75000000 300000000>; >>>> + >>>> + status = "disabled"; >>>> + }; >>>> + >>>> + ufs_mem_phy: phy@4807000 { >>>> + compatible = "qcom,sm6125-qmp-ufs-phy"; >>>> + reg = <0x04807000 0x1c4>; >>> Isn't this too small? Downstream says 0xdb8, but it's probably even bigger.. >> What do you think could help me find the new length of the registers? I tried 0x1000 and it probed just fine, but I'm not really sure until what extent I could push it. > The "true" values are probably only in documentation, which > I don't have. This patch series uses the "new" DT layout, where there isn't a subnode to define the address ranges of the different components. The reg size would be correct if it used the "legacy" DT layout. Confirming in downstream, 0xdb8 is the correct value (it's what DT uses there and the phy-qcom-ufs-qmp-v3-660 driver confirms the biggest register offset is PHY_BASE (0xc00) + PHY_SIZE (0x1b4) = 0xdb4 inclusive) I'd suggest going for that in your next revision Lux. > > Konrad >>
diff --git a/arch/arm64/boot/dts/qcom/sm6125.dtsi b/arch/arm64/boot/dts/qcom/sm6125.dtsi index df5453fcf2b9..cec7071d5279 100644 --- a/arch/arm64/boot/dts/qcom/sm6125.dtsi +++ b/arch/arm64/boot/dts/qcom/sm6125.dtsi @@ -511,6 +511,63 @@ sdhc_2: mmc@4784000 { status = "disabled"; }; + ufs_mem_hc: ufs@4804000 { + compatible = "qcom,sm6125-ufshc", "qcom,ufshc", "jedec,ufs-2.0"; + reg = <0x04804000 0x3000>, <0x04810000 0x8000>; + interrupts = <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>; + phys = <&ufs_mem_phy>; + phy-names = "ufsphy"; + lanes-per-direction = <1>; + #reset-cells = <1>; + resets = <&gcc GCC_UFS_PHY_BCR>; + reset-names = "rst"; + iommus = <&apps_smmu 0x200 0x0>; + + clock-names = "core_clk", + "bus_aggr_clk", + "iface_clk", + "core_clk_unipro", + "ref_clk", + "tx_lane0_sync_clk", + "rx_lane0_sync_clk", + "ice_core_clk"; + clocks = <&gcc GCC_UFS_PHY_AXI_CLK>, + <&gcc GCC_SYS_NOC_UFS_PHY_AXI_CLK>, + <&gcc GCC_UFS_PHY_AHB_CLK>, + <&gcc GCC_UFS_PHY_UNIPRO_CORE_CLK>, + <&rpmcc RPM_SMD_XO_CLK_SRC>, + <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>, + <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>, + <&gcc GCC_UFS_PHY_ICE_CORE_CLK>; + freq-table-hz = <50000000 240000000>, + <0 0>, + <0 0>, + <37500000 150000000>, + <0 0>, + <0 0>, + <0 0>, + <75000000 300000000>; + + status = "disabled"; + }; + + ufs_mem_phy: phy@4807000 { + compatible = "qcom,sm6125-qmp-ufs-phy"; + reg = <0x04807000 0x1c4>; + + clock-names = "ref", "ref_aux"; + clocks = <&gcc GCC_UFS_MEM_CLKREF_CLK>, <&gcc GCC_UFS_PHY_PHY_AUX_CLK>; + + power-domains = <&gcc UFS_PHY_GDSC>; + + resets = <&ufs_mem_hc 0>; + reset-names = "ufsphy"; + + #phy-cells = <0>; + + status = "disabled"; + }; + gpi_dma0: dma-controller@4a00000 { compatible = "qcom,sm6125-gpi-dma", "qcom,sdm845-gpi-dma"; reg = <0x04a00000 0x60000>;
Adds a UFS host controller node and its corresponding PHY to the sm6125 platform. Signed-off-by: Lux Aliaga <they@mint.lgbt> --- arch/arm64/boot/dts/qcom/sm6125.dtsi | 57 ++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)