| Message ID | 20230624-sm6125-dpu-v1-3-1d5a638cebf2@somainline.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | drm/msm: Add SM6125 MDSS/DPU hardware and enable Sony Xperia 10 II panel | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 28 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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 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 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. Best regards, Krzysztof
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 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. > > 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.. 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 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 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 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
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",
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> --- Documentation/devicetree/bindings/clock/qcom,dispcc-sm6125.yaml | 4 ++++ 1 file changed, 4 insertions(+)