| 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 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 04:06:04, Konrad Dybcio wrote: <snip> > > + sde_dsi_sleep: sde-dsi-sleep-state { > > + pins = "gpio90"; > > + function = "gpio"; > > + drive-strength = <2>; > > + bias-pull-down; > > + }; > s/sde/mdss as per Dmitry's recent request Makes sense, SDE doesn't mean anything on mainline. - Marijn
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 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. > >> 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. Konrad > >>> 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 25.06.2023 22:23, Marijn Suijten wrote: > 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. Please do. > >> 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? You're confusing DSI host with DSI PHY. > > Also note that I moved it from DSI PHY to DSI0 because that's where the > rpmpd opps reside. Both of them need their separate power lines to be active! Also, OPP is not necessary for genpd activation. Konrad > > - 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 25/06/2023 21:52, Marijn Suijten wrote: > On 2023-06-24 11:12:52, Krzysztof Kozlowski wrote: >> On 24/06/2023 02:41, Marijn Suijten wrote: >>> SM6125 is identical to SM6375 except that while downstream also defines >>> a throttle clock, its presence results in timeouts whereas SM6375 >>> requires it to not observe any timeouts. >> >> Then it should not be allowed, so you need either "else:" block or >> another "if: properties: compatible:" to disallow it. Because in current >> patch it would be allowed. > > That means this binding is wrong/incomplete for all other SoCs then. > clock(-name)s has 6 items, and sets `minItems: 6`. Only for sm6375-dpu > does it set `minItems: 7`, but an else case is missing. Ask the author why it is done like this. > > Shall I send a Fixes: ed41005f5b7c ("dt-bindings: display/msm: > sc7180-dpu: Describe SM6350 and SM6375") for that, and should maxItems: > 6 be the default under clock(-name)s or in an else:? There is no bug to fix. Or at least it is not yet known. Whether other devices should be constrained as well - sure, sounds reasonable, but I did not check the code exactly. We talk here about this patch. 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 2023-06-26 18:16:58, Krzysztof Kozlowski wrote: > On 25/06/2023 21:52, Marijn Suijten wrote: > > On 2023-06-24 11:12:52, Krzysztof Kozlowski wrote: > >> On 24/06/2023 02:41, Marijn Suijten wrote: > >>> SM6125 is identical to SM6375 except that while downstream also defines > >>> a throttle clock, its presence results in timeouts whereas SM6375 > >>> requires it to not observe any timeouts. > >> > >> Then it should not be allowed, so you need either "else:" block or > >> another "if: properties: compatible:" to disallow it. Because in current > >> patch it would be allowed. > > > > That means this binding is wrong/incomplete for all other SoCs then. > > clock(-name)s has 6 items, and sets `minItems: 6`. Only for sm6375-dpu Of course meant to say that clock(-name)s has **7** items, not 6. > > does it set `minItems: 7`, but an else case is missing. > > Ask the author why it is done like this. Konrad, can you clarify why other > > Shall I send a Fixes: ed41005f5b7c ("dt-bindings: display/msm: > > sc7180-dpu: Describe SM6350 and SM6375") for that, and should maxItems: > > 6 be the default under clock(-name)s or in an else:? > > There is no bug to fix. Or at least it is not yet known. Whether other > devices should be constrained as well - sure, sounds reasonable, but I > did not check the code exactly. I don't know either, but we need this information to decide whether to use `maxItems: 6`: 1. Directly on the property; 2. In an `else:` case on the current `if: sm6375-dpu` (should have the same effect as 1., afaik); 3. In a second `if:` case that lists all SoCS explicitly. Since we don't have this information, I think option 3. is the right way to go, setting `maxItems: 6` for qcom,sm6125-dpu. However, it is not yet understood why downstream is able to use the throttle clock without repercussions. > We talk here about this patch. We used this patch to discover that other SoCs are similarly unconstrained. But if you don't want me to look into it, by all means! Saves me a lot of time. So I will go with option 3. - 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 26.06.2023 19:54, Marijn Suijten wrote: > On 2023-06-26 18:16:58, Krzysztof Kozlowski wrote: >> On 25/06/2023 21:52, Marijn Suijten wrote: >>> On 2023-06-24 11:12:52, Krzysztof Kozlowski wrote: >>>> On 24/06/2023 02:41, Marijn Suijten wrote: >>>>> SM6125 is identical to SM6375 except that while downstream also defines >>>>> a throttle clock, its presence results in timeouts whereas SM6375 >>>>> requires it to not observe any timeouts. >>>> >>>> Then it should not be allowed, so you need either "else:" block or >>>> another "if: properties: compatible:" to disallow it. Because in current >>>> patch it would be allowed. >>> >>> That means this binding is wrong/incomplete for all other SoCs then. >>> clock(-name)s has 6 items, and sets `minItems: 6`. Only for sm6375-dpu > > Of course meant to say that clock(-name)s has **7** items, not 6. > >>> does it set `minItems: 7`, but an else case is missing. >> >> Ask the author why it is done like this. > > Konrad, can you clarify why other 6375 needs the throttle clk and the clock(-names) are strongly ordered so having minItems: 6 discards the last entry Konrad > >>> Shall I send a Fixes: ed41005f5b7c ("dt-bindings: display/msm: >>> sc7180-dpu: Describe SM6350 and SM6375") for that, and should maxItems: >>> 6 be the default under clock(-name)s or in an else:? >> >> There is no bug to fix. Or at least it is not yet known. Whether other >> devices should be constrained as well - sure, sounds reasonable, but I >> did not check the code exactly. > > I don't know either, but we need this information to decide whether to > use `maxItems: 6`: > > 1. Directly on the property; > 2. In an `else:` case on the current `if: sm6375-dpu` (should have the > same effect as 1., afaik); > 3. In a second `if:` case that lists all SoCS explicitly. > > Since we don't have this information, I think option 3. is the right way > to go, setting `maxItems: 6` for qcom,sm6125-dpu. > > However, it is not yet understood why downstream is able to use the > throttle clock without repercussions. > >> We talk here about this patch. > > We used this patch to discover that other SoCs are similarly > unconstrained. But if you don't want me to look into it, by all means! > Saves me a lot of time. So I will go with option 3. > > - Marijn
On 2023-06-26 20:57:51, Konrad Dybcio wrote: > On 26.06.2023 19:54, Marijn Suijten wrote: > > On 2023-06-26 18:16:58, Krzysztof Kozlowski wrote: > >> On 25/06/2023 21:52, Marijn Suijten wrote: > >>> On 2023-06-24 11:12:52, Krzysztof Kozlowski wrote: > >>>> On 24/06/2023 02:41, Marijn Suijten wrote: > >>>>> SM6125 is identical to SM6375 except that while downstream also defines > >>>>> a throttle clock, its presence results in timeouts whereas SM6375 > >>>>> requires it to not observe any timeouts. > >>>> > >>>> Then it should not be allowed, so you need either "else:" block or > >>>> another "if: properties: compatible:" to disallow it. Because in current > >>>> patch it would be allowed. > >>> > >>> That means this binding is wrong/incomplete for all other SoCs then. > >>> clock(-name)s has 6 items, and sets `minItems: 6`. Only for sm6375-dpu > > > > Of course meant to say that clock(-name)s has **7** items, not 6. > > > >>> does it set `minItems: 7`, but an else case is missing. > >> > >> Ask the author why it is done like this. > > > > Konrad, can you clarify why other (Looks like I forgot to complete this sentence before sending, apologies) > 6375 needs the throttle clk and the clock(-names) are strongly ordered > so having minItems: 6 discards the last entry The question is whether or not we should have maxItems: 6 to disallow the clock from being passed: right now it is optional and either is allowed for any !6375 SoC. - Marijn > > Konrad > > > >>> Shall I send a Fixes: ed41005f5b7c ("dt-bindings: display/msm: > >>> sc7180-dpu: Describe SM6350 and SM6375") for that, and should maxItems: > >>> 6 be the default under clock(-name)s or in an else:? > >> > >> There is no bug to fix. Or at least it is not yet known. Whether other > >> devices should be constrained as well - sure, sounds reasonable, but I > >> did not check the code exactly. > > > > I don't know either, but we need this information to decide whether to > > use `maxItems: 6`: > > > > 1. Directly on the property; > > 2. In an `else:` case on the current `if: sm6375-dpu` (should have the > > same effect as 1., afaik); > > 3. In a second `if:` case that lists all SoCS explicitly. > > > > Since we don't have this information, I think option 3. is the right way > > to go, setting `maxItems: 6` for qcom,sm6125-dpu. > > > > However, it is not yet understood why downstream is able to use the > > throttle clock without repercussions. > > > >> We talk here about this patch. > > > > We used this patch to discover that other SoCs are similarly > > unconstrained. But if you don't want me to look into it, by all means! > > Saves me a lot of time. So I will go with option 3. > > > > - Marijn
On 26.06.2023 22:28, Marijn Suijten wrote: > On 2023-06-26 20:57:51, Konrad Dybcio wrote: >> On 26.06.2023 19:54, Marijn Suijten wrote: >>> On 2023-06-26 18:16:58, Krzysztof Kozlowski wrote: >>>> On 25/06/2023 21:52, Marijn Suijten wrote: >>>>> On 2023-06-24 11:12:52, Krzysztof Kozlowski wrote: >>>>>> On 24/06/2023 02:41, Marijn Suijten wrote: >>>>>>> SM6125 is identical to SM6375 except that while downstream also defines >>>>>>> a throttle clock, its presence results in timeouts whereas SM6375 >>>>>>> requires it to not observe any timeouts. >>>>>> >>>>>> Then it should not be allowed, so you need either "else:" block or >>>>>> another "if: properties: compatible:" to disallow it. Because in current >>>>>> patch it would be allowed. >>>>> >>>>> That means this binding is wrong/incomplete for all other SoCs then. >>>>> clock(-name)s has 6 items, and sets `minItems: 6`. Only for sm6375-dpu >>> >>> Of course meant to say that clock(-name)s has **7** items, not 6. >>> >>>>> does it set `minItems: 7`, but an else case is missing. >>>> >>>> Ask the author why it is done like this. >>> >>> Konrad, can you clarify why other > > (Looks like I forgot to complete this sentence before sending, > apologies) > >> 6375 needs the throttle clk and the clock(-names) are strongly ordered >> so having minItems: 6 discards the last entry > > The question is whether or not we should have maxItems: 6 to disallow > the clock from being passed: right now it is optional and either is > allowed for any !6375 SoC. That's a very good question. I don't have a 7180 to test, but for you it seems to cause inexplicable issues on 6125.. Konrad > > - Marijn > >> >> Konrad >>> >>>>> Shall I send a Fixes: ed41005f5b7c ("dt-bindings: display/msm: >>>>> sc7180-dpu: Describe SM6350 and SM6375") for that, and should maxItems: >>>>> 6 be the default under clock(-name)s or in an else:? >>>> >>>> There is no bug to fix. Or at least it is not yet known. Whether other >>>> devices should be constrained as well - sure, sounds reasonable, but I >>>> did not check the code exactly. >>> >>> I don't know either, but we need this information to decide whether to >>> use `maxItems: 6`: >>> >>> 1. Directly on the property; >>> 2. In an `else:` case on the current `if: sm6375-dpu` (should have the >>> same effect as 1., afaik); >>> 3. In a second `if:` case that lists all SoCS explicitly. >>> >>> Since we don't have this information, I think option 3. is the right way >>> to go, setting `maxItems: 6` for qcom,sm6125-dpu. >>> >>> However, it is not yet understood why downstream is able to use the >>> throttle clock without repercussions. >>> >>>> We talk here about this patch. >>> >>> We used this patch to discover that other SoCs are similarly >>> unconstrained. But if you don't want me to look into it, by all means! >>> Saves me a lot of time. So I will go with option 3. >>> >>> - Marijn
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? Best regards, Krzysztof
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 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? - Marijn
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. Best regards, Krzysztof
On 24/06/2023 03:41, Marijn Suijten wrote: > SM6125's UBWC hardware decoder is version 3.0, and supports decoding > UBWC 1.0. I think it's UBWC encoder version, see https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.15.1.r17-rel/msm/sde/sde_hw_top.c?ref_type=heads#L357 This is a part of https://patchwork.freedesktop.org/patch/538279/?series=118074&rev=1 (no, you don't have to rebase on that patchset, it is not reviewed yet). > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > --- > drivers/gpu/drm/msm/msm_mdss.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c > index 05648c910c68..bf68bae23264 100644 > --- a/drivers/gpu/drm/msm/msm_mdss.c > +++ b/drivers/gpu/drm/msm/msm_mdss.c > @@ -559,6 +559,13 @@ static const struct msm_mdss_data sm6115_data = { > .ubwc_static = 0x11f, > }; > > +static const struct msm_mdss_data sm6125_data = { > + .ubwc_version = UBWC_1_0, > + .ubwc_dec_version = UBWC_3_0, > + .ubwc_swizzle = 1, > + .highest_bank_bit = 1, > +}; > + > static const struct msm_mdss_data sm8250_data = { > .ubwc_version = UBWC_4_0, > .ubwc_dec_version = UBWC_4_0, > @@ -579,6 +586,7 @@ static const struct of_device_id mdss_dt_match[] = { > { .compatible = "qcom,sc8180x-mdss", .data = &sc8180x_data }, > { .compatible = "qcom,sc8280xp-mdss", .data = &sc8280xp_data }, > { .compatible = "qcom,sm6115-mdss", .data = &sm6115_data }, > + { .compatible = "qcom,sm6125-mdss", .data = &sm6125_data }, > { .compatible = "qcom,sm6350-mdss", .data = &sm6350_data }, > { .compatible = "qcom,sm6375-mdss", .data = &sm6350_data }, > { .compatible = "qcom,sm8150-mdss", .data = &sm8150_data }, >
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 2023-06-27 11:49:07, Dmitry Baryshkov wrote: > On 24/06/2023 03:41, Marijn Suijten wrote: > > SM6125's UBWC hardware decoder is version 3.0, and supports decoding > > UBWC 1.0. > > I think it's UBWC encoder version, see > https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.15.1.r17-rel/msm/sde/sde_hw_top.c?ref_type=heads#L357 > > This is a part of > https://patchwork.freedesktop.org/patch/538279/?series=118074&rev=1 > > (no, you don't have to rebase on that patchset, it is not reviewed yet). Thanks for clarifying this. I always thought that there only was a (decoder) hardware version, that is able to decode a specific format version of the UBWC data format. And that it was confusingly using the same enum. I will reword the message. (Also didn't really see why MDSS would have to _encode_ to UBWC, for readbacks?) - Marijn > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > --- > > drivers/gpu/drm/msm/msm_mdss.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c > > index 05648c910c68..bf68bae23264 100644 > > --- a/drivers/gpu/drm/msm/msm_mdss.c > > +++ b/drivers/gpu/drm/msm/msm_mdss.c > > @@ -559,6 +559,13 @@ static const struct msm_mdss_data sm6115_data = { > > .ubwc_static = 0x11f, > > }; > > > > +static const struct msm_mdss_data sm6125_data = { > > + .ubwc_version = UBWC_1_0, > > + .ubwc_dec_version = UBWC_3_0, > > + .ubwc_swizzle = 1, > > + .highest_bank_bit = 1, > > +}; > > + > > static const struct msm_mdss_data sm8250_data = { > > .ubwc_version = UBWC_4_0, > > .ubwc_dec_version = UBWC_4_0, > > @@ -579,6 +586,7 @@ static const struct of_device_id mdss_dt_match[] = { > > { .compatible = "qcom,sc8180x-mdss", .data = &sc8180x_data }, > > { .compatible = "qcom,sc8280xp-mdss", .data = &sc8280xp_data }, > > { .compatible = "qcom,sm6115-mdss", .data = &sm6115_data }, > > + { .compatible = "qcom,sm6125-mdss", .data = &sm6125_data }, > > { .compatible = "qcom,sm6350-mdss", .data = &sm6350_data }, > > { .compatible = "qcom,sm6375-mdss", .data = &sm6350_data }, > > { .compatible = "qcom,sm8150-mdss", .data = &sm8150_data }, > > > > -- > With best wishes > Dmitry >
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
On 6/26/2023 7:04 AM, Dmitry Baryshkov wrote: > On 24/06/2023 03:41, Marijn Suijten wrote: >> SM6125 is identical to SM6375 except that while downstream also defines >> a throttle clock, its presence results in timeouts whereas SM6375 >> requires it to not observe any timeouts. > > I see that the vendor DTS still references this clock. > > Abhinav, Tanya, do possibly know what can be wrong here? > From display side, we just enable it without any specific vote. Seeing timeouts without it makes sense but not with it. I dont have experience with this family of chipsets and this clock is specific to this family of chipsets. Will reach out to folks who might have a better idea about this clock and update with possible suggestions. Unless ... tanya has more suggestions. >> >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> >> --- >> Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml | >> 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git >> a/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml >> b/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml >> index 630b11480496..6d2ba9a1cca1 100644 >> --- a/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml >> +++ b/Documentation/devicetree/bindings/display/msm/qcom,sc7180-dpu.yaml >> @@ -15,6 +15,7 @@ properties: >> compatible: >> enum: >> - qcom,sc7180-dpu >> + - qcom,sm6125-dpu >> - qcom,sm6350-dpu >> - qcom,sm6375-dpu >> >
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,