| Message ID | 20230624-sm6125-dpu-v1-0-1d5a638cebf2@somainline.org |
|---|---|
| Headers | show |
| Series | drm/msm: Add SM6125 MDSS/DPU hardware and enable Sony Xperia 10 II panel | expand |
On 24.06.2023 02:40, Marijn Suijten wrote: > This node has always resided in the wrong spot, making it somewhat > harder to contribute new node entries while maintaining proper sorting > around it. Move the node up to sit after hsusb_phy1 where it maintains > proper numerial numerical sorting on the (first of its many) reg address property. > > Fixes: cff4bbaf2a2d ("arm64: dts: qcom: Add support for SM6125") > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad > arch/arm64/boot/dts/qcom/sm6125.dtsi | 38 ++++++++++++++++++------------------ > 1 file changed, 19 insertions(+), 19 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sm6125.dtsi b/arch/arm64/boot/dts/qcom/sm6125.dtsi > index a596baa6ce3e..722dde560bec 100644 > --- a/arch/arm64/boot/dts/qcom/sm6125.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm6125.dtsi > @@ -679,6 +679,24 @@ hsusb_phy1: phy@1613000 { > status = "disabled"; > }; > > + spmi_bus: spmi@1c40000 { > + compatible = "qcom,spmi-pmic-arb"; > + reg = <0x01c40000 0x1100>, > + <0x01e00000 0x2000000>, > + <0x03e00000 0x100000>, > + <0x03f00000 0xa0000>, > + <0x01c0a000 0x26000>; > + reg-names = "core", "chnls", "obsrvr", "intr", "cnfg"; > + interrupt-names = "periph_irq"; > + interrupts = <GIC_SPI 183 IRQ_TYPE_LEVEL_HIGH>; > + qcom,ee = <0>; > + qcom,channel = <0>; > + #address-cells = <2>; > + #size-cells = <0>; > + interrupt-controller; > + #interrupt-cells = <4>; > + }; > + > rpm_msg_ram: sram@45f0000 { > compatible = "qcom,rpm-msg-ram"; > reg = <0x045f0000 0x7000>; > @@ -1184,27 +1202,9 @@ sram@4690000 { > reg = <0x04690000 0x10000>; > }; > > - spmi_bus: spmi@1c40000 { > - compatible = "qcom,spmi-pmic-arb"; > - reg = <0x01c40000 0x1100>, > - <0x01e00000 0x2000000>, > - <0x03e00000 0x100000>, > - <0x03f00000 0xa0000>, > - <0x01c0a000 0x26000>; > - reg-names = "core", "chnls", "obsrvr", "intr", "cnfg"; > - interrupt-names = "periph_irq"; > - interrupts = <GIC_SPI 183 IRQ_TYPE_LEVEL_HIGH>; > - qcom,ee = <0>; > - qcom,channel = <0>; > - #address-cells = <2>; > - #size-cells = <0>; > - interrupt-controller; > - #interrupt-cells = <4>; > - }; > - > apps_smmu: iommu@c600000 { > compatible = "qcom,sm6125-smmu-500", "qcom,smmu-500", "arm,mmu-500"; > - reg = <0xc600000 0x80000>; > + reg = <0x0c600000 0x80000>; > interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>, >
On Sat, 24 Jun 2023 02:41:05 +0200, Marijn Suijten wrote: > Document the SM6125 MDSS. > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > --- > .../bindings/display/msm/qcom,sm6125-mdss.yaml | 206 +++++++++++++++++++++ > 1 file changed, 206 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/display/msm/qcom,sm6125-mdss.example.dtb: dsi@5e94000: compatible: 'oneOf' conditional failed, one must be fixed: ['qcom,sm6125-dsi-ctrl', 'qcom,mdss-dsi-ctrl'] is too long 'qcom,sm6125-dsi-ctrl' is not one of ['qcom,apq8064-dsi-ctrl', 'qcom,msm8916-dsi-ctrl', 'qcom,msm8953-dsi-ctrl', 'qcom,msm8974-dsi-ctrl', 'qcom,msm8996-dsi-ctrl', 'qcom,msm8998-dsi-ctrl', 'qcom,qcm2290-dsi-ctrl', 'qcom,sc7180-dsi-ctrl', 'qcom,sc7280-dsi-ctrl', 'qcom,sdm660-dsi-ctrl', 'qcom,sdm845-dsi-ctrl', 'qcom,sm6115-dsi-ctrl', 'qcom,sm8150-dsi-ctrl', 'qcom,sm8250-dsi-ctrl', 'qcom,sm8350-dsi-ctrl', 'qcom,sm8450-dsi-ctrl', 'qcom,sm8550-dsi-ctrl'] 'qcom,sm6125-dsi-ctrl' is not one of ['qcom,dsi-ctrl-6g-qcm2290', 'qcom,mdss-dsi-ctrl'] from schema $id: http://devicetree.org/schemas/display/msm/dsi-controller-main.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/display/msm/qcom,sm6125-mdss.example.dtb: dsi@5e94000: Unevaluated properties are not allowed ('compatible' was unexpected) from schema $id: http://devicetree.org/schemas/display/msm/dsi-controller-main.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230624-sm6125-dpu-v1-7-1d5a638cebf2@somainline.org The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 24/06/2023 03:41, Marijn Suijten wrote: > Enable and configure the dispcc node on SM6125 for consumption by MDSS > later on. > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > --- > arch/arm64/boot/dts/qcom/sm6125.dtsi | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm6125.dtsi b/arch/arm64/boot/dts/qcom/sm6125.dtsi > index edb03508dba3..7d78b4e48ebe 100644 > --- a/arch/arm64/boot/dts/qcom/sm6125.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm6125.dtsi > @@ -3,6 +3,7 @@ > * Copyright (c) 2021, Martin Botka <martin.botka@somainline.org> > */ > > +#include <dt-bindings/clock/qcom,dispcc-sm6125.h> > #include <dt-bindings/clock/qcom,gcc-sm6125.h> > #include <dt-bindings/clock/qcom,rpmcc.h> > #include <dt-bindings/dma/qcom-gpi.h> > @@ -1203,6 +1204,28 @@ sram@4690000 { > reg = <0x04690000 0x10000>; > }; > > + dispcc: clock-controller@5f00000 { > + compatible = "qcom,sm6125-dispcc"; > + reg = <0x05f00000 0x20000>; > + clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, > + <0>, > + <0>, > + <0>, > + <0>, > + <0>, > + <&gcc GCC_DISP_GPLL0_DIV_CLK_SRC>; This clock is at the wrong position. > + clock-names = "bi_tcxo", > + "gcc_disp_gpll0_div_clk_src", > + "dsi0_phy_pll_out_byteclk", > + "dsi0_phy_pll_out_dsiclk", > + "dsi1_phy_pll_out_dsiclk", > + "dp_phy_pll_link_clk", > + "dp_phy_pll_vco_div_clk"; > + power-domains = <&rpmpd SM6125_VDDCX>; > + #clock-cells = <1>; > + #power-domain-cells = <1>; > + }; > + > apps_smmu: iommu@c600000 { > compatible = "qcom,sm6125-smmu-500", "qcom,smmu-500", "arm,mmu-500"; > reg = <0x0c600000 0x80000>; >
On 2023-06-24 03:43:21, Konrad Dybcio wrote: > On 24.06.2023 02:40, Marijn Suijten wrote: > > This node has always resided in the wrong spot, making it somewhat > > harder to contribute new node entries while maintaining proper sorting > > around it. Move the node up to sit after hsusb_phy1 where it maintains > > proper numerial > numerical Thanks. > sorting on the (first of its many) reg address property. Why was this continuation of the line not re-quoted? Makes your reply super-hard to read. - Marijn > > > > Fixes: cff4bbaf2a2d ("arm64: dts: qcom: Add support for SM6125") > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > --- > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > Konrad > > arch/arm64/boot/dts/qcom/sm6125.dtsi | 38 ++++++++++++++++++------------------ > > 1 file changed, 19 insertions(+), 19 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/sm6125.dtsi b/arch/arm64/boot/dts/qcom/sm6125.dtsi > > index a596baa6ce3e..722dde560bec 100644 > > --- a/arch/arm64/boot/dts/qcom/sm6125.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sm6125.dtsi > > @@ -679,6 +679,24 @@ hsusb_phy1: phy@1613000 { > > status = "disabled"; > > }; > > > > + spmi_bus: spmi@1c40000 { > > + compatible = "qcom,spmi-pmic-arb"; > > + reg = <0x01c40000 0x1100>, > > + <0x01e00000 0x2000000>, > > + <0x03e00000 0x100000>, > > + <0x03f00000 0xa0000>, > > + <0x01c0a000 0x26000>; > > + reg-names = "core", "chnls", "obsrvr", "intr", "cnfg"; > > + interrupt-names = "periph_irq"; > > + interrupts = <GIC_SPI 183 IRQ_TYPE_LEVEL_HIGH>; > > + qcom,ee = <0>; > > + qcom,channel = <0>; > > + #address-cells = <2>; > > + #size-cells = <0>; > > + interrupt-controller; > > + #interrupt-cells = <4>; > > + }; > > + > > rpm_msg_ram: sram@45f0000 { > > compatible = "qcom,rpm-msg-ram"; > > reg = <0x045f0000 0x7000>; > > @@ -1184,27 +1202,9 @@ sram@4690000 { > > reg = <0x04690000 0x10000>; > > }; > > > > - spmi_bus: spmi@1c40000 { > > - compatible = "qcom,spmi-pmic-arb"; > > - reg = <0x01c40000 0x1100>, > > - <0x01e00000 0x2000000>, > > - <0x03e00000 0x100000>, > > - <0x03f00000 0xa0000>, > > - <0x01c0a000 0x26000>; > > - reg-names = "core", "chnls", "obsrvr", "intr", "cnfg"; > > - interrupt-names = "periph_irq"; > > - interrupts = <GIC_SPI 183 IRQ_TYPE_LEVEL_HIGH>; > > - qcom,ee = <0>; > > - qcom,channel = <0>; > > - #address-cells = <2>; > > - #size-cells = <0>; > > - interrupt-controller; > > - #interrupt-cells = <4>; > > - }; > > - > > apps_smmu: iommu@c600000 { > > compatible = "qcom,sm6125-smmu-500", "qcom,smmu-500", "arm,mmu-500"; > > - reg = <0xc600000 0x80000>; > > + reg = <0x0c600000 0x80000>; > > interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>, > > <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>, > > <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>, > >
On 2023-06-24 03:42:46, Konrad Dybcio wrote: > On 24.06.2023 02:40, Marijn Suijten wrote: > > Bring up the SM6125 DPU now that all preliminary series (such as INTF > > TE) have been merged (for me to test the hardware properly) > We should not repeat the same mistake in the future.. Finding a > balance between releasing early and releasing what we can declare > working and tested code is hard, but we waaaaaaaay overstayed on > this one.. I don't understand what you mean by "mistake" at all. Yes the DPU catalog portion of this series sat in my local branch for a very long time. Yes it had to be rebased on top of conflicts many many times. However, that time has also been used to fix and extend DPU where necessary, instead of submitting a half-broken or half-incomplete catalog entry... Re "we overstayed": you could have asked to clean up and send my patch, so I don't take this as a mistake on my part as you are completely aware of my time schedule ;) > Konrad > , and most Also here, don't forget to re-quote my message if you break half-way in the line. > > other conflicting work (barring ongoing catalog *improvements*) has made > > its way in as well or is still being discussed. > > > > > The second part of the series complements that by immediately utilizing > > this hardware in DT, and even enabling the MDSS/DSI nodes complete with > > a 6.0" 1080x2520 panel for Sony's Seine PDX201 (Xperia 10 II). > > > > The last patch ("sm6125-seine: Configure MDSS, DSI and panel") depends > > on (an impending v2 of) my Sony panel collection series [1]. > > > > [1]: https://lore.kernel.org/linux-arm-msm/20230521-drm-panels-sony-v1-0-541c341d6bee@somainline.org/ > > > > --- > > Marijn Suijten (15): > > arm64: dts: qcom: sm6125: Sort spmi_bus node numerically by reg > > dt-bindings: clock: qcom,dispcc-sm6125: Remove unused GCC_DISP_AHB_CLK > > dt-bindings: clock: qcom,dispcc-sm6125: Require GCC PLL0 DIV clock > > dt-bindings: clock: qcom,dispcc-sm6125: Allow power-domains property > > dt-bindings: display/msm: dsi-controller-main: Document SM6125 > > dt-bindings: display/msm: sc7180-dpu: Describe SM6125 > > dt-bindings: display/msm: Add SM6125 MDSS > > drm/msm/dpu: Add SM6125 support > > drm/msm/mdss: Add SM6125 support > > dt-bindings: msm: dsi-phy-14nm: Document SM6125 variant > > drm/msm/dsi: Add 14nm phy configuration for SM6125 > > arm64: dts: qcom: sm6125: Switch fixed xo_board clock to RPM XO clock > > arm64: dts: qcom: sm6125: Add dispcc node > > arm64: dts: qcom: sm6125: Add display hardware nodes > > arm64: dts: qcom: sm6125-seine: Configure MDSS, DSI and panel > > > > .../bindings/clock/qcom,dispcc-sm6125.yaml | 17 +- > > .../bindings/display/msm/dsi-controller-main.yaml | 2 + > > .../bindings/display/msm/dsi-phy-14nm.yaml | 1 + > > .../bindings/display/msm/qcom,sc7180-dpu.yaml | 1 + > > .../bindings/display/msm/qcom,sm6125-mdss.yaml | 206 +++++++++++++++++ > > .../dts/qcom/sm6125-sony-xperia-seine-pdx201.dts | 59 +++++ > > arch/arm64/boot/dts/qcom/sm6125.dtsi | 244 +++++++++++++++++++-- > > .../gpu/drm/msm/disp/dpu1/catalog/dpu_5_4_sm6125.h | 173 +++++++++++++++ > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 6 + > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 1 + > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 + > > drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 + > > drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 1 + > > drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 15 ++ > > drivers/gpu/drm/msm/msm_mdss.c | 8 + > > 15 files changed, 712 insertions(+), 25 deletions(-) > > --- > > base-commit: 8d2be868b42c08290509c60515865f4de24ea704 > > change-id: 20230624-sm6125-dpu-aedc9637ee7b > > > > Best regards,
On 2023-06-24 04:05:07, Konrad Dybcio wrote: > On 24.06.2023 02:41, Marijn Suijten wrote: > > Add the DT nodes that describe the MDSS hardware on SM6125, containing > > one MDP (display controller) together with a single DSI and DSI PHY. No > > DisplayPort support is added for now. > > > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > --- > > arch/arm64/boot/dts/qcom/sm6125.dtsi | 190 ++++++++++++++++++++++++++++++++++- > > 1 file changed, 186 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/sm6125.dtsi b/arch/arm64/boot/dts/qcom/sm6125.dtsi > > index 7d78b4e48ebe..6852dacf54e6 100644 > > --- a/arch/arm64/boot/dts/qcom/sm6125.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sm6125.dtsi > > @@ -1204,16 +1204,198 @@ sram@4690000 { > > reg = <0x04690000 0x10000>; > > }; > > > > + mdss: display-subsystem@5e00000 { > > + compatible = "qcom,sm6125-mdss"; > > + reg = <0x05e00000 0x1000>; > > + reg-names = "mdss"; > > + > > + power-domains = <&dispcc MDSS_GDSC>; > > + > > + clocks = <&gcc GCC_DISP_AHB_CLK>, > > + <&dispcc DISP_CC_MDSS_AHB_CLK>, > > + <&dispcc DISP_CC_MDSS_MDP_CLK>; > > + clock-names = "iface", "ahb", "core"; > One per line would be nice Sure, it's all over the place elsewhere though. > > + > > + interrupts = <GIC_SPI 186 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-controller; > > + #interrupt-cells = <1>; > > + > > + iommus = <&apps_smmu 0x400 0x0>; > > + > Every node has a different scramble of property orders :P > > To roughly align with what I ask most people to do (and what I should > work on codifying and automating ehhh) would be: Exactly. Until this automated - or even documented - there is barely any point to manually deal with this. > [compat] > [reg] > > [interrupt] Also interrupt-cells here, since that's only related to interrupt-controller; and not interrupts=<>;? > > [clock] > > [reset] > (btw there should be a MDSS_CORE reset at DISP_CC+0x2000, try writing 1 there > and see if MDSS dies) > > [opp] > [pd] > > [iommus] > [icc] > > [phy] > > [addresssizeranges] > > [status] Sure, have reordered them like this for v2. > > > + status = "disabled"; > > + > > + #address-cells = <1>; > > + #size-cells = <1>; > > + ranges; > > + > > + mdss_mdp: display-controller@5e01000 { > > + compatible = "qcom,sm6125-dpu"; > > + reg = <0x05e01000 0x83208>, > > + <0x05eb0000 0x2008>; > > + reg-names = "mdp", "vbif"; > > + > > + clocks = <&gcc GCC_DISP_HF_AXI_CLK>, > > + <&dispcc DISP_CC_MDSS_AHB_CLK>, > > + <&dispcc DISP_CC_MDSS_ROT_CLK>, > > + <&dispcc DISP_CC_MDSS_MDP_LUT_CLK>, > > + <&dispcc DISP_CC_MDSS_MDP_CLK>, > > + <&dispcc DISP_CC_MDSS_VSYNC_CLK>; > > + clock-names = "bus", > > + "iface", > > + "rot", > > + "lut", > > + "core", > > + "vsync"; > > + assigned-clocks = <&dispcc DISP_CC_MDSS_VSYNC_CLK>; > > + assigned-clock-rates = <19200000>; > > + > > + operating-points-v2 = <&mdp_opp_table>; > > + power-domains = <&rpmpd SM6125_VDDCX>; > > + > > + interrupt-parent = <&mdss>; > > + interrupts = <0>; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + dpu_intf1_out: endpoint { > > + remote-endpoint = <&mdss_dsi0_in>; > > + }; > > + }; > > + }; > > + > > + mdp_opp_table: opp-table { > > + compatible = "operating-points-v2"; > > + > > + opp-192000000 { > > + opp-hz = /bits/ 64 <192000000>; > > + required-opps = <&rpmpd_opp_low_svs>; > > + }; > > + > > + opp-256000000 { > > + opp-hz = /bits/ 64 <256000000>; > > + required-opps = <&rpmpd_opp_svs>; > > + }; > > + > > + opp-307200000 { > > + opp-hz = /bits/ 64 <307200000>; > > + required-opps = <&rpmpd_opp_svs_plus>; > > + }; > > + > > + opp-384000000 { > > + opp-hz = /bits/ 64 <384000000>; > > + required-opps = <&rpmpd_opp_nom>; > > + }; > > + > > + opp-400000000 { > > + opp-hz = /bits/ 64 <400000000>; > > + required-opps = <&rpmpd_opp_turbo>; > > + }; > > + }; > > + }; > > + > > + mdss_dsi0: dsi@5e94000 { > > + compatible = "qcom,sm6125-dsi-ctrl", "qcom,mdss-dsi-ctrl"; > > + reg = <0x05e94000 0x400>; > > + reg-names = "dsi_ctrl"; > > + > > + interrupt-parent = <&mdss>; > > + interrupts = <4>; > > + > > + clocks = <&dispcc DISP_CC_MDSS_BYTE0_CLK>, > > + <&dispcc DISP_CC_MDSS_BYTE0_INTF_CLK>, > > + <&dispcc DISP_CC_MDSS_PCLK0_CLK>, > > + <&dispcc DISP_CC_MDSS_ESC0_CLK>, > > + <&dispcc DISP_CC_MDSS_AHB_CLK>, > > + <&gcc GCC_DISP_HF_AXI_CLK>; > > + clock-names = "byte", > > + "byte_intf", > > + "pixel", > > + "core", > > + "iface", > > + "bus"; > > + > > + assigned-clocks = <&dispcc DISP_CC_MDSS_BYTE0_CLK_SRC>, > > + <&dispcc DISP_CC_MDSS_PCLK0_CLK_SRC>; > > + assigned-clock-parents = <&mdss_dsi0_phy 0>, <&mdss_dsi0_phy 1>; > > + > > + operating-points-v2 = <&dsi_opp_table>; > > + power-domains = <&rpmpd SM6125_VDDMX>; > CX Downstream says MX (and only on the phy... but it is the dsi node that defines the corresponding rpmpd opps). Do you have a source to back this up? > > > + > > + phys = <&mdss_dsi0_phy>; > > + phy-names = "dsi"; > > + > > + status = "disabled"; > > + > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + mdss_dsi0_in: endpoint { > > + remote-endpoint = <&dpu_intf1_out>; > > + }; > > + }; > > + > > + port@1 { > > + reg = <1>; > > + mdss_dsi0_out: endpoint { > > + }; > > + }; > > + }; > > + > > + dsi_opp_table: opp-table { > > + compatible = "operating-points-v2"; > > + > > + opp-164000000 { > > + opp-hz = /bits/ 64 <164000000>; > > + required-opps = <&rpmpd_opp_low_svs>; > > + }; > > + > > + opp-187500000 { > > + opp-hz = /bits/ 64 <187500000>; > > + required-opps = <&rpmpd_opp_svs>; > > + }; > > + }; > > + }; > > + > > + mdss_dsi0_phy: phy@5e94400 { > > + compatible = "qcom,dsi-phy-14nm-6125"; > > + reg = <0x05e94400 0x100>, > > + <0x05e94500 0x300>, > > + <0x05e94800 0x188>; > > + reg-names = "dsi_phy", > > + "dsi_phy_lane", > > + "dsi_pll"; > > + > > + #clock-cells = <1>; > > + #phy-cells = <0>; > > + > > + clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>, > > + <&rpmcc RPM_SMD_XO_CLK_SRC>; > > + clock-names = "iface", "ref"; > > + > > + status = "disabled"; > > + }; > > + }; > > + > > dispcc: clock-controller@5f00000 { > > compatible = "qcom,sm6125-dispcc"; > > reg = <0x05f00000 0x20000>; > > clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, > > + <&gcc GCC_DISP_GPLL0_DIV_CLK_SRC>, > > + <&mdss_dsi0_phy 0>, > > + <&mdss_dsi0_phy 1>, > > <0>, > > <0>, > > - <0>, > > - <0>, > > - <0>, > > - <&gcc GCC_DISP_GPLL0_DIV_CLK_SRC>; > > + <0>; > > clock-names = "bi_tcxo", > Wrong patch Yup, as said by many folks in many places. It seems the --fixup went to the wrong commit, even though the clock-names were fixup'ed into the right one... Have fixed this for v2. - Marijn > > Konrad > > "gcc_disp_gpll0_div_clk_src", > > "dsi0_phy_pll_out_byteclk", > >
On 2023-06-24 03:45:02, Konrad Dybcio wrote: > On 24.06.2023 02:41, Marijn Suijten wrote: > > The "gcc_disp_gpll0_div_clk_src" clock is consumed by the driver, will > > be passed from DT, and should be required by the bindings. > > > > Fixes: 8397c9c0c26b ("dt-bindings: clock: add QCOM SM6125 display clock bindings") > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > --- > Ideally, you'd stick it at the bottom of the list, as the items: order > is part of the ABI This isn't an ABI break, as this driver nor its bindings require/declare a fixed order: they declare a relation between clocks and clock-names. This orders the GCC clock just like other dispccs. And the previous patch dropped the unused cfg_ahb_clk from the bindings, so all bets are off anyway. - Marijn > > Konrad > > Documentation/devicetree/bindings/clock/qcom,dispcc-sm6125.yaml | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm6125.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm6125.yaml > > index 2acf487d8a2f..11ec154503a3 100644 > > --- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm6125.yaml > > +++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm6125.yaml > > @@ -23,6 +23,7 @@ properties: > > clocks: > > items: > > - description: Board XO source > > + - description: GPLL0 div source from GCC > > - description: Byte clock from DSI PHY0 > > - description: Pixel clock from DSI PHY0 > > - description: Pixel clock from DSI PHY1 > > @@ -32,6 +33,7 @@ properties: > > clock-names: > > items: > > - const: bi_tcxo > > + - const: gcc_disp_gpll0_div_clk_src > > - const: dsi0_phy_pll_out_byteclk > > - const: dsi0_phy_pll_out_dsiclk > > - const: dsi1_phy_pll_out_dsiclk > > @@ -65,12 +67,14 @@ examples: > > compatible = "qcom,sm6125-dispcc"; > > reg = <0x5f00000 0x20000>; > > clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, > > + <&gcc GCC_DISP_GPLL0_DIV_CLK_SRC>, > > <&dsi0_phy 0>, > > <&dsi0_phy 1>, > > <&dsi1_phy 1>, > > <&dp_phy 0>, > > <&dp_phy 1>; > > clock-names = "bi_tcxo", > > + "gcc_disp_gpll0_div_clk_src", > > "dsi0_phy_pll_out_byteclk", > > "dsi0_phy_pll_out_dsiclk", > > "dsi1_phy_pll_out_dsiclk", > >
On 2023-06-24 11:08:54, Krzysztof Kozlowski wrote: > On 24/06/2023 03:45, Konrad Dybcio wrote: > > On 24.06.2023 02:41, Marijn Suijten wrote: > >> The "gcc_disp_gpll0_div_clk_src" clock is consumed by the driver, will > >> be passed from DT, and should be required by the bindings. > >> > >> Fixes: 8397c9c0c26b ("dt-bindings: clock: add QCOM SM6125 display clock bindings") > >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > >> --- > > Ideally, you'd stick it at the bottom of the list, as the items: order > > is part of the ABI > > Yes, please add them to the end. Order is fixed. Disagreed for bindings that declare clock-names and when the driver adheres to it, see my reply to Konrad's message. - Marijn
On 2023-06-24 03:49:25, Konrad Dybcio wrote: > On 24.06.2023 02:41, Marijn Suijten wrote: > > SM6125 features only a single PHY (despite a secondary PHY PLL source > > being available to the disp_cc_mdss_pclk0_clk_src clock), and downstream > > sources for this "trinket" SoC do not define the typical "vcca" > > regulator to be available nor used. > > > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > --- > The introduced ops are identical to 2290, modulo regulator.. Sure, I can create a "drop unused regulators from 14nm qcm2290 config" and a second "reuse qcm2290 14nm dsi phy for sm6125" patch, instead of this one. > But the regulator is absent on both (VDD_MX powers it instead), so In the DT patch you requested me to use CX instead of MX... Which one is it? Also note that I moved it from DSI PHY to DSI0 because that's where the rpmpd opps reside. - Marijn > feel free to clean that up and reuse it ;) > > Konrad > > drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 ++ > > drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 1 + > > drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 15 +++++++++++++++ > > 3 files changed, 18 insertions(+) > > > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c > > index 9d5795c58a98..8688ed502dcf 100644 > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c > > @@ -559,6 +559,8 @@ static const struct of_device_id dsi_phy_dt_match[] = { > > .data = &dsi_phy_14nm_2290_cfgs }, > > { .compatible = "qcom,dsi-phy-14nm-660", > > .data = &dsi_phy_14nm_660_cfgs }, > > + { .compatible = "qcom,dsi-phy-14nm-6125", > > + .data = &dsi_phy_14nm_6125_cfgs }, > > { .compatible = "qcom,dsi-phy-14nm-8953", > > .data = &dsi_phy_14nm_8953_cfgs }, > > #endif > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > index 8b640d174785..ebf915f5e6c6 100644 > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h > > @@ -52,6 +52,7 @@ extern const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs; > > extern const struct msm_dsi_phy_cfg dsi_phy_14nm_cfgs; > > extern const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs; > > extern const struct msm_dsi_phy_cfg dsi_phy_14nm_2290_cfgs; > > +extern const struct msm_dsi_phy_cfg dsi_phy_14nm_6125_cfgs; > > extern const struct msm_dsi_phy_cfg dsi_phy_14nm_8953_cfgs; > > extern const struct msm_dsi_phy_cfg dsi_phy_10nm_cfgs; > > extern const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs; > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c > > index 3ce45b023e63..5d43c9ec69ae 100644 > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c > > @@ -1068,6 +1068,21 @@ const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs = { > > .num_dsi_phy = 2, > > }; > > > > +const struct msm_dsi_phy_cfg dsi_phy_14nm_6125_cfgs = { > > + .has_phy_lane = true, > > + .ops = { > > + .enable = dsi_14nm_phy_enable, > > + .disable = dsi_14nm_phy_disable, > > + .pll_init = dsi_pll_14nm_init, > > + .save_pll_state = dsi_14nm_pll_save_state, > > + .restore_pll_state = dsi_14nm_pll_restore_state, > > + }, > > + .min_pll_rate = VCO_MIN_RATE, > > + .max_pll_rate = VCO_MAX_RATE, > > + .io_start = { 0x5e94400 }, > > + .num_dsi_phy = 1, > > +}; > > + > > const struct msm_dsi_phy_cfg dsi_phy_14nm_8953_cfgs = { > > .has_phy_lane = true, > > .regulator_data = dsi_phy_14nm_17mA_regulators, > >
On 26.06.2023 16:17, Marijn Suijten wrote: > On 2023-06-26 11:41:39, Konrad Dybcio wrote: >> On 25.06.2023 21:18, Marijn Suijten wrote: >>> On 2023-06-24 03:42:46, Konrad Dybcio wrote: >>>> On 24.06.2023 02:40, Marijn Suijten wrote: >>>>> Bring up the SM6125 DPU now that all preliminary series (such as INTF >>>>> TE) have been merged (for me to test the hardware properly) >>>> We should not repeat the same mistake in the future.. Finding a >>>> balance between releasing early and releasing what we can declare >>>> working and tested code is hard, but we waaaaaaaay overstayed on >>>> this one.. >>> >>> I don't understand what you mean by "mistake" at all. Yes the DPU >>> catalog portion of this series sat in my local branch for a very long >>> time. Yes it had to be rebased on top of conflicts many many times. >>> >>> However, that time has also been used to fix and extend DPU where >>> necessary, instead of submitting a half-broken or half-incomplete >>> catalog entry... >>> >>> Re "we overstayed": you could have asked to clean up and send my patch, >>> so I don't take this as a mistake on my part as you are completely aware >>> of my time schedule ;) >> I didn't mean to pick on you. I just wanted to emphasize that a more >> upstream-forward approach would have saved us quite some time on the >> rebasing and cleaning-up front. > > That is how it comes across ;) - our dream is all about upstream-first > but as you know this becomes a mess really quickly when things are > blocked on dependencies and you're working on 5 different features and > testing across ±8 different Sony platforms on ±14 different devices at > once... all in a limited portion of free time. > > Fwiw cleaning-up would have had to happen either way, and would have > taken the same amount of time regardless of whether this series is > submitted now or two months ago. > >>>> Konrad >>>> , and most >>> >>> Also here, don't forget to re-quote my message if you break half-way in >>> the line. >> Ugh. All the time I've been doing this I thought thunderfox was smart >> enough to do it for me. Apparently not and you're the 1st one to point >> that out. > > You're welcome! > (Though I thought it should be visible in Thunderburd, unless you're not > in plaintext mode? Does it still show the "this is quoted" line in > front of the broken sentence?) It doesn't, but the text stays blue (as if it was) Konrad > > - Marijn
On 2023-06-26 11:43:39, Konrad Dybcio wrote: > On 25.06.2023 21:48, Marijn Suijten wrote: > > On 2023-06-24 03:45:02, Konrad Dybcio wrote: > >> On 24.06.2023 02:41, Marijn Suijten wrote: > >>> The "gcc_disp_gpll0_div_clk_src" clock is consumed by the driver, will > >>> be passed from DT, and should be required by the bindings. > >>> > >>> Fixes: 8397c9c0c26b ("dt-bindings: clock: add QCOM SM6125 display clock bindings") > >>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > >>> --- > >> Ideally, you'd stick it at the bottom of the list, as the items: order > >> is part of the ABI > > > > This isn't an ABI break, as this driver nor its bindings require/declare > > a fixed order: they declare a relation between clocks and clock-names. > Bindings describe the ABI, drivers implement compliant code flow. That is how bindings are supposed to be... However typically the driver is written/ported first and then the bindings are simply created to reflect this, and sometimes (as is the case with this patch) incorrectly. That, together with a lack of DTS and known-working device with it (which is why I'm submitting driver+bindings+dts in one series now!) makes us shoot ourselves in the foot by locking everyone into an ABI that makes no sense. > > This orders the GCC clock just like other dispccs. And the previous > > patch dropped the unused cfg_ahb_clk from the bindings, so all bets are > > off anyway. > Thinking about it again, the binding has not been consumed by any upstream > DT to date, so it should (tm) be fine to let it slide.. Exactly, I hope/doubt anyone was already using these incomplete bindings. And again: the ABI here is the name->phandle mapping, the order Does Not Matter™. So I hope we can let it slide (otherwise the previous patch shouldd have been NAK'ed as well??) (Unless you are SM6115 which uses index-based mapping and does not define clock-names at all) - Marijn > Konrad > > > > - Marijn > > > >> > >> Konrad > >>> Documentation/devicetree/bindings/clock/qcom,dispcc-sm6125.yaml | 4 ++++ > >>> 1 file changed, 4 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm6125.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm6125.yaml > >>> index 2acf487d8a2f..11ec154503a3 100644 > >>> --- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm6125.yaml > >>> +++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm6125.yaml > >>> @@ -23,6 +23,7 @@ properties: > >>> clocks: > >>> items: > >>> - description: Board XO source > >>> + - description: GPLL0 div source from GCC > >>> - description: Byte clock from DSI PHY0 > >>> - description: Pixel clock from DSI PHY0 > >>> - description: Pixel clock from DSI PHY1 > >>> @@ -32,6 +33,7 @@ properties: > >>> clock-names: > >>> items: > >>> - const: bi_tcxo > >>> + - const: gcc_disp_gpll0_div_clk_src > >>> - const: dsi0_phy_pll_out_byteclk > >>> - const: dsi0_phy_pll_out_dsiclk > >>> - const: dsi1_phy_pll_out_dsiclk > >>> @@ -65,12 +67,14 @@ examples: > >>> compatible = "qcom,sm6125-dispcc"; > >>> reg = <0x5f00000 0x20000>; > >>> clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, > >>> + <&gcc GCC_DISP_GPLL0_DIV_CLK_SRC>, > >>> <&dsi0_phy 0>, > >>> <&dsi0_phy 1>, > >>> <&dsi1_phy 1>, > >>> <&dp_phy 0>, > >>> <&dp_phy 1>; > >>> clock-names = "bi_tcxo", > >>> + "gcc_disp_gpll0_div_clk_src", > >>> "dsi0_phy_pll_out_byteclk", > >>> "dsi0_phy_pll_out_dsiclk", > >>> "dsi1_phy_pll_out_dsiclk", > >>>
On 25/06/2023 21:48, Marijn Suijten wrote: > On 2023-06-24 11:08:54, Krzysztof Kozlowski wrote: >> On 24/06/2023 03:45, Konrad Dybcio wrote: >>> On 24.06.2023 02:41, Marijn Suijten wrote: >>>> The "gcc_disp_gpll0_div_clk_src" clock is consumed by the driver, will >>>> be passed from DT, and should be required by the bindings. >>>> >>>> Fixes: 8397c9c0c26b ("dt-bindings: clock: add QCOM SM6125 display clock bindings") >>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> >>>> --- >>> Ideally, you'd stick it at the bottom of the list, as the items: order >>> is part of the ABI >> >> Yes, please add them to the end. Order is fixed. > > Disagreed for bindings that declare clock-names and when the driver > adheres to it, see my reply to Konrad's message. That's the generic rule, with some exceptions of course. Whether one chosen driver (chosen system and chosen version of that system) adheres or not, does not change it. Other driver behaves differently and ABI is for everyone, not only for your specific version of Linux driver. Follow the rule. Best regards, Krzysztof
On 26/06/2023 16:26, Marijn Suijten wrote: > On 2023-06-26 11:43:39, Konrad Dybcio wrote: >> On 25.06.2023 21:48, Marijn Suijten wrote: >>> On 2023-06-24 03:45:02, Konrad Dybcio wrote: >>>> On 24.06.2023 02:41, Marijn Suijten wrote: >>>>> The "gcc_disp_gpll0_div_clk_src" clock is consumed by the driver, will >>>>> be passed from DT, and should be required by the bindings. >>>>> >>>>> Fixes: 8397c9c0c26b ("dt-bindings: clock: add QCOM SM6125 display clock bindings") >>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> >>>>> --- >>>> Ideally, you'd stick it at the bottom of the list, as the items: order >>>> is part of the ABI >>> >>> This isn't an ABI break, as this driver nor its bindings require/declare >>> a fixed order: they declare a relation between clocks and clock-names. >> Bindings describe the ABI, drivers implement compliant code flow. > > That is how bindings are supposed to be... However typically the driver > is written/ported first and then the bindings are simply created to Your development process does not matter for the bindings. Whatever you decide to do "typically" is your choice, although of course I understand why you do it like that. You can argument the same that "I never create bindings in my process, so the driver defines the ABI". > reflect this, and sometimes (as is the case with this patch) > incorrectly. > > That, together with a lack of DTS and known-working device with it > (which is why I'm submitting driver+bindings+dts in one series now!) > makes us shoot ourselves in the foot by locking everyone into an ABI > that makes no sense. No one is locked into the ABI. SoC maintainer decides on this. However unjustified ABI breaking or not caring about it at all is not the way to go. It is not the correct process. > >>> This orders the GCC clock just like other dispccs. And the previous >>> patch dropped the unused cfg_ahb_clk from the bindings, so all bets are >>> off anyway. >> Thinking about it again, the binding has not been consumed by any upstream >> DT to date, so it should (tm) be fine to let it slide.. > > Exactly, I hope/doubt anyone was already using these incomplete > bindings. And again: the ABI here is the name->phandle mapping, the > order Does Not Matter™. No, it's not. Your one driver does not define the ABI. There are many different drivers, many different operating systems and other software components. Best regards, Krzysztof
On 2023-06-26 18:15:13, Krzysztof Kozlowski wrote: > On 26/06/2023 16:26, Marijn Suijten wrote: > > On 2023-06-26 11:43:39, Konrad Dybcio wrote: > >> On 25.06.2023 21:48, Marijn Suijten wrote: > >>> On 2023-06-24 03:45:02, Konrad Dybcio wrote: > >>>> On 24.06.2023 02:41, Marijn Suijten wrote: > >>>>> The "gcc_disp_gpll0_div_clk_src" clock is consumed by the driver, will > >>>>> be passed from DT, and should be required by the bindings. > >>>>> > >>>>> Fixes: 8397c9c0c26b ("dt-bindings: clock: add QCOM SM6125 display clock bindings") > >>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > >>>>> --- > >>>> Ideally, you'd stick it at the bottom of the list, as the items: order > >>>> is part of the ABI > >>> > >>> This isn't an ABI break, as this driver nor its bindings require/declare > >>> a fixed order: they declare a relation between clocks and clock-names. > >> Bindings describe the ABI, drivers implement compliant code flow. > > > > That is how bindings are supposed to be... However typically the driver > > is written/ported first and then the bindings are simply created to > > Your development process does not matter for the bindings. Whatever you > decide to do "typically" is your choice, although of course I understand > why you do it like that. You can argument the same that "I never create > bindings in my process, so the driver defines the ABI". This is not my process, nor my choice. > > reflect this, and sometimes (as is the case with this patch) > > incorrectly. > > > > That, together with a lack of DTS and known-working device with it > > (which is why I'm submitting driver+bindings+dts in one series now!) > > makes us shoot ourselves in the foot by locking everyone into an ABI > > that makes no sense. > > No one is locked into the ABI. SoC maintainer decides on this. However > unjustified ABI breaking or not caring about it at all is not the way to > go. It is not the correct process. It definitely sounds like it. > >>> This orders the GCC clock just like other dispccs. And the previous > >>> patch dropped the unused cfg_ahb_clk from the bindings, so all bets are > >>> off anyway. > >> Thinking about it again, the binding has not been consumed by any upstream > >> DT to date, so it should (tm) be fine to let it slide.. > > > > Exactly, I hope/doubt anyone was already using these incomplete > > bindings. And again: the ABI here is the name->phandle mapping, the > > order Does Not Matter™. > > No, it's not. Your one driver does not define the ABI. There are many > different drivers, many different operating systems and other software > components. You missed the point entirely. The point is that someone contributed a dt-bindings patch that does not represent the hardware (hence not the driver for that hardware either). It was taken without objection. So now what? We are stuck with a broken ABI that does not allow us to describe the hardware? If there are many different drivers and other OSes, why are we solely responsible for describing broken bindings? Why were there no objections elsewhere that this does not allow us to describe the hardware in question? Note that the person signing off on and sending that initial dt-bindings patch for qcom,dispcc-sm6125.yaml is me, by the way. - Marijn
On 2023-06-26 18:10:44, Krzysztof Kozlowski wrote: > On 25/06/2023 21:48, Marijn Suijten wrote: > > On 2023-06-24 11:08:54, Krzysztof Kozlowski wrote: > >> On 24/06/2023 03:45, Konrad Dybcio wrote: > >>> On 24.06.2023 02:41, Marijn Suijten wrote: > >>>> The "gcc_disp_gpll0_div_clk_src" clock is consumed by the driver, will > >>>> be passed from DT, and should be required by the bindings. > >>>> > >>>> Fixes: 8397c9c0c26b ("dt-bindings: clock: add QCOM SM6125 display clock bindings") > >>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > >>>> --- > >>> Ideally, you'd stick it at the bottom of the list, as the items: order > >>> is part of the ABI > >> > >> Yes, please add them to the end. Order is fixed. > > > > Disagreed for bindings that declare clock-names and when the driver > > adheres to it, see my reply to Konrad's message. > > That's the generic rule, with some exceptions of course. Whether one > chosen driver (chosen system and chosen version of that system) adheres > or not, does not change it. Other driver behaves differently and ABI is > for everyone, not only for your specific version of Linux driver. > > Follow the rule. This has no relation to the driver (just that our driver adheres to the bindings, as it is supposed to be). The bindings define a mapping from a clock-names=<> entry to a clock on the same index in the clocks=<> array. That relation remains the same with this change. - Marijn
On 26/06/2023 19:49, Marijn Suijten wrote: > On 2023-06-26 18:10:44, Krzysztof Kozlowski wrote: >> On 25/06/2023 21:48, Marijn Suijten wrote: >>> On 2023-06-24 11:08:54, Krzysztof Kozlowski wrote: >>>> On 24/06/2023 03:45, Konrad Dybcio wrote: >>>>> On 24.06.2023 02:41, Marijn Suijten wrote: >>>>>> The "gcc_disp_gpll0_div_clk_src" clock is consumed by the driver, will >>>>>> be passed from DT, and should be required by the bindings. >>>>>> >>>>>> Fixes: 8397c9c0c26b ("dt-bindings: clock: add QCOM SM6125 display clock bindings") >>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> >>>>>> --- >>>>> Ideally, you'd stick it at the bottom of the list, as the items: order >>>>> is part of the ABI >>>> >>>> Yes, please add them to the end. Order is fixed. >>> >>> Disagreed for bindings that declare clock-names and when the driver >>> adheres to it, see my reply to Konrad's message. >> >> That's the generic rule, with some exceptions of course. Whether one >> chosen driver (chosen system and chosen version of that system) adheres >> or not, does not change it. Other driver behaves differently and ABI is >> for everyone, not only for your specific version of Linux driver. >> >> Follow the rule. > > This has no relation to the driver (just that our driver adheres to the > bindings, as it is supposed to be). The bindings define a mapping from > a clock-names=<> entry to a clock on the same index in the clocks=<> > array. That relation remains the same with this change. Not really, binding also defines the list of clocks - their order and specific entries. This changes. Best regards, Krzysztof
On 2023-06-26 20:29:37, Krzysztof Kozlowski wrote: > On 26/06/2023 19:49, Marijn Suijten wrote: > > On 2023-06-26 18:10:44, Krzysztof Kozlowski wrote: > >> On 25/06/2023 21:48, Marijn Suijten wrote: > >>> On 2023-06-24 11:08:54, Krzysztof Kozlowski wrote: > >>>> On 24/06/2023 03:45, Konrad Dybcio wrote: > >>>>> On 24.06.2023 02:41, Marijn Suijten wrote: > >>>>>> The "gcc_disp_gpll0_div_clk_src" clock is consumed by the driver, will > >>>>>> be passed from DT, and should be required by the bindings. > >>>>>> > >>>>>> Fixes: 8397c9c0c26b ("dt-bindings: clock: add QCOM SM6125 display clock bindings") > >>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > >>>>>> --- > >>>>> Ideally, you'd stick it at the bottom of the list, as the items: order > >>>>> is part of the ABI > >>>> > >>>> Yes, please add them to the end. Order is fixed. > >>> > >>> Disagreed for bindings that declare clock-names and when the driver > >>> adheres to it, see my reply to Konrad's message. > >> > >> That's the generic rule, with some exceptions of course. Whether one > >> chosen driver (chosen system and chosen version of that system) adheres > >> or not, does not change it. Other driver behaves differently and ABI is > >> for everyone, not only for your specific version of Linux driver. > >> > >> Follow the rule. > > > > This has no relation to the driver (just that our driver adheres to the > > bindings, as it is supposed to be). The bindings define a mapping from > > a clock-names=<> entry to a clock on the same index in the clocks=<> > > array. That relation remains the same with this change. > > Not really, binding also defines the list of clocks - their order and > specific entries. This changes. And so it does in "dt-bindings: clock: qcom,dispcc-sm6125: Remove unused GCC_DISP_AHB_CLK"? - Marijn
On 2023-06-26 20:51:38, Marijn Suijten wrote: <snip> > > Not really, binding also defines the list of clocks - their order and > > specific entries. This changes. > > And so it does in "dt-bindings: clock: qcom,dispcc-sm6125: Remove unused > GCC_DISP_AHB_CLK"? Never mind: it is the last item so the order of the other items doesn't change. The total number of items decreases though, which sounds like an ABI-break too? - Marijn
On 2023-06-27 08:24:41, Krzysztof Kozlowski wrote: > On 26/06/2023 20:53, Marijn Suijten wrote: > > On 2023-06-26 20:51:38, Marijn Suijten wrote: > > <snip> > >>> Not really, binding also defines the list of clocks - their order and > >>> specific entries. This changes. > >> > >> And so it does in "dt-bindings: clock: qcom,dispcc-sm6125: Remove unused > >> GCC_DISP_AHB_CLK"? > > > > Never mind: it is the last item so the order of the other items doesn't > > change. The total number of items decreases though, which sounds like > > an ABI-break too? > > How does it break? Old DTS works exactly the same, doesn't it? So deleting a new item at the end does not matter. But what if I respin this patch to add the new clock _at the end_, which will then be at the same index as the previous GCC_DISP_AHB_CLK? - Marijn
On 27/06/2023 08:54, Marijn Suijten wrote: > On 2023-06-27 08:24:41, Krzysztof Kozlowski wrote: >> On 26/06/2023 20:53, Marijn Suijten wrote: >>> On 2023-06-26 20:51:38, Marijn Suijten wrote: >>> <snip> >>>>> Not really, binding also defines the list of clocks - their order and >>>>> specific entries. This changes. >>>> >>>> And so it does in "dt-bindings: clock: qcom,dispcc-sm6125: Remove unused >>>> GCC_DISP_AHB_CLK"? >>> >>> Never mind: it is the last item so the order of the other items doesn't >>> change. The total number of items decreases though, which sounds like >>> an ABI-break too? >> >> How does it break? Old DTS works exactly the same, doesn't it? > > So deleting a new item at the end does not matter. But what if I respin > this patch to add the new clock _at the end_, which will then be at the > same index as the previous GCC_DISP_AHB_CLK? I think you know the answer, right? What do you want to prove? That two independent changes can have together negative effect? We know this. Best regards, Krzysztof
On 2023-06-27 10:21:12, Krzysztof Kozlowski wrote: > On 27/06/2023 09:49, Marijn Suijten wrote: > > On 2023-06-27 09:29:53, Krzysztof Kozlowski wrote: > >> On 27/06/2023 08:54, Marijn Suijten wrote: > >>> On 2023-06-27 08:24:41, Krzysztof Kozlowski wrote: > >>>> On 26/06/2023 20:53, Marijn Suijten wrote: > >>>>> On 2023-06-26 20:51:38, Marijn Suijten wrote: > >>>>> <snip> > >>>>>>> Not really, binding also defines the list of clocks - their order and > >>>>>>> specific entries. This changes. > >>>>>> > >>>>>> And so it does in "dt-bindings: clock: qcom,dispcc-sm6125: Remove unused > >>>>>> GCC_DISP_AHB_CLK"? > >>>>> > >>>>> Never mind: it is the last item so the order of the other items doesn't > >>>>> change. The total number of items decreases though, which sounds like > >>>>> an ABI-break too? > >>>> > >>>> How does it break? Old DTS works exactly the same, doesn't it? > >>> > >>> So deleting a new item at the end does not matter. But what if I respin > >>> this patch to add the new clock _at the end_, which will then be at the > >>> same index as the previous GCC_DISP_AHB_CLK? > >> > >> I think you know the answer, right? What do you want to prove? That two > >> independent changes can have together negative effect? We know this. > > > > The question is whether this is allowed? > > That would be an ABI break and I already explained if it is or is not > allowed. How should we solve it then, if we cannot remove GCC_DISP_AHB_CLK in one patch and add GCC_DISP_GPLL0_DIV_CLK_SRC **at the end** in the next patch? Keep an empty spot at the original index of GCC_DISP_AHB_CLK? - Marijn
On 27/06/2023 11:02, Marijn Suijten wrote: >>>>> So deleting a new item at the end does not matter. But what if I respin >>>>> this patch to add the new clock _at the end_, which will then be at the >>>>> same index as the previous GCC_DISP_AHB_CLK? >>>> >>>> I think you know the answer, right? What do you want to prove? That two >>>> independent changes can have together negative effect? We know this. >>> >>> The question is whether this is allowed? >> >> That would be an ABI break and I already explained if it is or is not >> allowed. > > How should we solve it then, if we cannot remove GCC_DISP_AHB_CLK in one > patch and add GCC_DISP_GPLL0_DIV_CLK_SRC **at the end** in the next > patch? Keep an empty spot at the original index of GCC_DISP_AHB_CLK? I don't know if you are trolling me or really asking question, so just in case it is the latter: "No one is locked into the ABI. SoC maintainer decides on this. " Also: https://lore.kernel.org/linux-arm-msm/20230608152759.GA2721945-robh@kernel.org/ https://lore.kernel.org/linux-arm-msm/CAL_JsqKOq+PdjUPVYqdC7QcjGxp-KbAG_O9e+zrfY7k-wRr1QQ@mail.gmail.com/ https://lore.kernel.org/linux-arm-msm/20220602143245.GA2256965-robh@kernel.org/ https://lore.kernel.org/linux-arm-msm/20220601202452.GA365963-robh@kernel.org/ Any many more. Best regards, Krzysztof
On 2023-06-27 11:07:22, Krzysztof Kozlowski wrote: > On 27/06/2023 11:02, Marijn Suijten wrote: > >>>>> So deleting a new item at the end does not matter. But what if I respin > >>>>> this patch to add the new clock _at the end_, which will then be at the > >>>>> same index as the previous GCC_DISP_AHB_CLK? > >>>> > >>>> I think you know the answer, right? What do you want to prove? That two > >>>> independent changes can have together negative effect? We know this. > >>> > >>> The question is whether this is allowed? > >> > >> That would be an ABI break and I already explained if it is or is not > >> allowed. > > > > How should we solve it then, if we cannot remove GCC_DISP_AHB_CLK in one > > patch and add GCC_DISP_GPLL0_DIV_CLK_SRC **at the end** in the next > > patch? Keep an empty spot at the original index of GCC_DISP_AHB_CLK? > > I don't know if you are trolling me or really asking question, so just > in case it is the latter: Apologies if it comes across that way, but I am genuinely misunderstanding what is and is not allowed as part of this ABI... > "No one is locked into the ABI. SoC maintainer decides on this. " Especially if it is up to the SoC mantainer. > Also: > https://lore.kernel.org/linux-arm-msm/20230608152759.GA2721945-robh@kernel.org/ > > https://lore.kernel.org/linux-arm-msm/CAL_JsqKOq+PdjUPVYqdC7QcjGxp-KbAG_O9e+zrfY7k-wRr1QQ@mail.gmail.com/ > > https://lore.kernel.org/linux-arm-msm/20220602143245.GA2256965-robh@kernel.org/ > > https://lore.kernel.org/linux-arm-msm/20220601202452.GA365963-robh@kernel.org/ > > Any many more. In that sense the question above is not for you, but for the SoC maintainer? Whom, I hope, will say that we can be lenient in changing the ABI for a platform that is only slowly being brought up by a bunch of community developers and unlikely to be touched by anyone else. Thanks for helping out so far! - Marijn
Bring up the SM6125 DPU now that all preliminary series (such as INTF TE) have been merged (for me to test the hardware properly), and most other conflicting work (barring ongoing catalog *improvements*) has made its way in as well or is still being discussed. The second part of the series complements that by immediately utilizing this hardware in DT, and even enabling the MDSS/DSI nodes complete with a 6.0" 1080x2520 panel for Sony's Seine PDX201 (Xperia 10 II). The last patch ("sm6125-seine: Configure MDSS, DSI and panel") depends on (an impending v2 of) my Sony panel collection series [1]. [1]: https://lore.kernel.org/linux-arm-msm/20230521-drm-panels-sony-v1-0-541c341d6bee@somainline.org/ --- Marijn Suijten (15): arm64: dts: qcom: sm6125: Sort spmi_bus node numerically by reg dt-bindings: clock: qcom,dispcc-sm6125: Remove unused GCC_DISP_AHB_CLK dt-bindings: clock: qcom,dispcc-sm6125: Require GCC PLL0 DIV clock dt-bindings: clock: qcom,dispcc-sm6125: Allow power-domains property dt-bindings: display/msm: dsi-controller-main: Document SM6125 dt-bindings: display/msm: sc7180-dpu: Describe SM6125 dt-bindings: display/msm: Add SM6125 MDSS drm/msm/dpu: Add SM6125 support drm/msm/mdss: Add SM6125 support dt-bindings: msm: dsi-phy-14nm: Document SM6125 variant drm/msm/dsi: Add 14nm phy configuration for SM6125 arm64: dts: qcom: sm6125: Switch fixed xo_board clock to RPM XO clock arm64: dts: qcom: sm6125: Add dispcc node arm64: dts: qcom: sm6125: Add display hardware nodes arm64: dts: qcom: sm6125-seine: Configure MDSS, DSI and panel .../bindings/clock/qcom,dispcc-sm6125.yaml | 17 +- .../bindings/display/msm/dsi-controller-main.yaml | 2 + .../bindings/display/msm/dsi-phy-14nm.yaml | 1 + .../bindings/display/msm/qcom,sc7180-dpu.yaml | 1 + .../bindings/display/msm/qcom,sm6125-mdss.yaml | 206 +++++++++++++++++ .../dts/qcom/sm6125-sony-xperia-seine-pdx201.dts | 59 +++++ arch/arm64/boot/dts/qcom/sm6125.dtsi | 244 +++++++++++++++++++-- .../gpu/drm/msm/disp/dpu1/catalog/dpu_5_4_sm6125.h | 173 +++++++++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 6 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 + drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 + drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 1 + drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 15 ++ drivers/gpu/drm/msm/msm_mdss.c | 8 + 15 files changed, 712 insertions(+), 25 deletions(-) --- base-commit: 8d2be868b42c08290509c60515865f4de24ea704 change-id: 20230624-sm6125-dpu-aedc9637ee7b Best regards,