Messages in this thread | | | Date | Tue, 20 Dec 2022 22:32:54 +0200 | Subject | Re: [PATCH v4 2/4] arm64: dts: qcom: sm6125: Add UFS nodes | From | Dmitry Baryshkov <> |
| |
On 20/12/2022 22:30, Dmitry Baryshkov wrote: > On Tue, 20 Dec 2022 at 21:33, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> >> >> On 20.12.2022 19:57, Lux Aliaga wrote: >>> On 16/12/2022 08:24, Konrad Dybcio wrote: >>> >>>> >>>> On 15.12.2022 20:04, 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> >>>>> --- >>>> Please include a changelog, I don't know what you changed and >>>> what you didn't. Also, you sent 4 revisions in one day, not >>>> letting others review it. >>>> >>>> >>>>> arch/arm64/boot/dts/qcom/sm6125.dtsi | 67 ++++++++++++++++++++++++++++ >>>>> 1 file changed, 67 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6125.dtsi b/arch/arm64/boot/dts/qcom/sm6125.dtsi >>>>> index 7e25a4f85594..b64c5bc1452f 100644 >>>>> --- a/arch/arm64/boot/dts/qcom/sm6125.dtsi >>>>> +++ b/arch/arm64/boot/dts/qcom/sm6125.dtsi >>>>> @@ -508,6 +508,73 @@ 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>; >>>>> + reg-names = "std", "ice"; >>>>> + interrupts = <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>; >>>>> + phys = <&ufs_mem_phy_lanes>; >>>>> + phy-names = "ufsphy"; >>>>> + lanes-per-direction = <1>; >>>>> + #reset-cells = <1>; >>>>> + resets = <&gcc GCC_UFS_PHY_BCR>; >>>>> + reset-names = "rst"; >>>>> + >>>>> + 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>; >>>>> + >>>>> + non-removable; >>>>> + status = "disabled"; >>>>> + }; >>>>> + >>>>> + ufs_mem_phy: phy@4807000 { >>>>> + compatible = "qcom,sm6115-qmp-ufs-phy"; >>>> Krzysztof asked you to add a SoC-specific compatible in v1. >>> I'm working on adding a new compatible for sm6125's UFS PHY. Should I copy sm6115's tables or just reference them in the sm6125's config in drivers/phy/qualcomm/phy-qcom-qmp-ufs.c? >> If they're identical, you can just do something like this: >> >> compatible = "qcom,sm6125-qmp-ufs-phy", "qcom,sm6115-qmp-ufs-phy"; > > Ugh. I'd prefer to see either of the compatible strings here, but not > both of them.
I hit send too quick, so the justification didn't get in.
Currently we list a single compatibility string for all QMP PHYs. Having just a single exception stands out, so I'd advise against doing that (despite Konrad's suggestion being technically correct).
> >> >> And ensure your newly added compatible is documented in bindings. >> This way, the driver will fall back to the 6115 compatible that's >> defined in .c, but if we ever need to adjust something specific >> for 6125, we will just use the define that we added here. That's >> important, as we're supposed to stay backwards-compatible with >> old device trees. >> >> Also, wrap your emails at around 80 chars or so, some people >> are grumpy about that :P >> >> Konrad >>> > > >
-- With best wishes Dmitry
| |