| Message ID | 20230512-amlogic-v6-4-upstream-dsi-ccf-vim3-v4-1-2592c29ea263@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/meson: add support for MIPI DSI Display | expand |
On 12/05/2023 15:11, Neil Armstrong wrote: > Expose VCLK2_SEL clock id and add new ids for the CTS_ENCL and CTS_ENCL_SEL > clocks on G12A compatible SoCs. > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/clk/meson/g12a.h | 1 - > include/dt-bindings/clock/g12a-clkc.h | 3 +++ > 2 files changed, 3 insertions(+), 1 deletion(-) Bindings must be a separate patch from the driver changes. If this causes bisectability issues, this means entire solution breaks ABI and is not appropriate anyway... Best regards, Krzysztof
On 13/05/2023 20:28, Krzysztof Kozlowski wrote: > On 12/05/2023 15:11, Neil Armstrong wrote: >> Expose VCLK2_SEL clock id and add new ids for the CTS_ENCL and CTS_ENCL_SEL >> clocks on G12A compatible SoCs. >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> drivers/clk/meson/g12a.h | 1 - >> include/dt-bindings/clock/g12a-clkc.h | 3 +++ >> 2 files changed, 3 insertions(+), 1 deletion(-) > > Bindings must be a separate patch from the driver changes. If this > causes bisectability issues, this means entire solution breaks ABI and > is not appropriate anyway... This is basically how we handled CLK IDs on Amlogic clk bindings for the last years, the amount of changes is very low and rather exceptional compared to early development stage. Neil > > Best regards, > Krzysztof >
On 15/05/2023 18:06, Neil Armstrong wrote: > On 13/05/2023 20:28, Krzysztof Kozlowski wrote: >> On 12/05/2023 15:11, Neil Armstrong wrote: >>> Expose VCLK2_SEL clock id and add new ids for the CTS_ENCL and CTS_ENCL_SEL >>> clocks on G12A compatible SoCs. >>> >>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>> --- >>> drivers/clk/meson/g12a.h | 1 - >>> include/dt-bindings/clock/g12a-clkc.h | 3 +++ >>> 2 files changed, 3 insertions(+), 1 deletion(-) >> >> Bindings must be a separate patch from the driver changes. If this >> causes bisectability issues, this means entire solution breaks ABI and >> is not appropriate anyway... > > This is basically how we handled CLK IDs on Amlogic clk bindings for the > last years, the amount of changes is very low and rather exceptional > compared to early development stage. The commits with bindings are used in devicetree-rebasing repo, so we want them to be separate. Meson is the only or almost the only platform making such changes. I don't get why, because the conflict could be easily avoided with using different names for defines in bindings and local clock. Approach of having bindings strictly tied with driver commit is never desired. Best regards, Krzysztof
On 15/05/2023 18:13, Krzysztof Kozlowski wrote: > On 15/05/2023 18:06, Neil Armstrong wrote: >> On 13/05/2023 20:28, Krzysztof Kozlowski wrote: >>> On 12/05/2023 15:11, Neil Armstrong wrote: >>>> Expose VCLK2_SEL clock id and add new ids for the CTS_ENCL and CTS_ENCL_SEL >>>> clocks on G12A compatible SoCs. >>>> >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>> --- >>>> drivers/clk/meson/g12a.h | 1 - >>>> include/dt-bindings/clock/g12a-clkc.h | 3 +++ >>>> 2 files changed, 3 insertions(+), 1 deletion(-) >>> >>> Bindings must be a separate patch from the driver changes. If this >>> causes bisectability issues, this means entire solution breaks ABI and >>> is not appropriate anyway... >> >> This is basically how we handled CLK IDs on Amlogic clk bindings for the >> last years, the amount of changes is very low and rather exceptional >> compared to early development stage. > > The commits with bindings are used in devicetree-rebasing repo, so we > want them to be separate. > > Meson is the only or almost the only platform making such changes. I > don't get why, because the conflict could be easily avoided with using > different names for defines in bindings and local clock. Approach of > having bindings strictly tied with driver commit is never desired. Also one more argument maybe not relevant here but for other cases - this makes literally impossible to include the clock ID in DTS in the same kernel revision, because you must not merge driver branch to DTS branch. SoC folks were complaining about this many times. Best regards, Krzysztof
On 15/05/2023 18:15, Krzysztof Kozlowski wrote: > On 15/05/2023 18:13, Krzysztof Kozlowski wrote: >> On 15/05/2023 18:06, Neil Armstrong wrote: >>> On 13/05/2023 20:28, Krzysztof Kozlowski wrote: >>>> On 12/05/2023 15:11, Neil Armstrong wrote: >>>>> Expose VCLK2_SEL clock id and add new ids for the CTS_ENCL and CTS_ENCL_SEL >>>>> clocks on G12A compatible SoCs. >>>>> >>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>> --- >>>>> drivers/clk/meson/g12a.h | 1 - >>>>> include/dt-bindings/clock/g12a-clkc.h | 3 +++ >>>>> 2 files changed, 3 insertions(+), 1 deletion(-) >>>> >>>> Bindings must be a separate patch from the driver changes. If this >>>> causes bisectability issues, this means entire solution breaks ABI and >>>> is not appropriate anyway... >>> >>> This is basically how we handled CLK IDs on Amlogic clk bindings for the >>> last years, the amount of changes is very low and rather exceptional >>> compared to early development stage. >> >> The commits with bindings are used in devicetree-rebasing repo, so we >> want them to be separate. A lot of commits changes the bindings and other part of the kernel source, it was solved with git filter-repo a long time ago. While I understand in an ideal world those commits should only touch Documentation/bindings, it's sometime not possible. >> >> Meson is the only or almost the only platform making such changes. I >> don't get why, because the conflict could be easily avoided with using >> different names for defines in bindings and local clock. Approach of >> having bindings strictly tied with driver commit is never desired. If we did it now, we would have make it differently and expose all the clock IDs on the bindings like on Qcom, be sure of that. > > Also one more argument maybe not relevant here but for other cases - > this makes literally impossible to include the clock ID in DTS in the > same kernel revision, because you must not merge driver branch to DTS > branch. SoC folks were complaining about this many times. Actually we handle this very simply by having such patches merged in a immutable branch merged in the clock and DT pull-requests, it worked perfectly so far and neither Stephen or Arnd complained about that. > > Best regards, > Krzysztof >
On 15/05/2023 18:22, neil.armstrong@linaro.org wrote: >>> Meson is the only or almost the only platform making such changes. I >>> don't get why, because the conflict could be easily avoided with using >>> different names for defines in bindings and local clock. Approach of >>> having bindings strictly tied with driver commit is never desired. > > If we did it now, we would have make it differently and expose all the clock > IDs on the bindings like on Qcom, be sure of that. No, you just keep different names. The only problem here is that your clock name is the same thus you cannot split bindings into separate patch. > >> >> Also one more argument maybe not relevant here but for other cases - >> this makes literally impossible to include the clock ID in DTS in the >> same kernel revision, because you must not merge driver branch to DTS >> branch. SoC folks were complaining about this many times. > > Actually we handle this very simply by having such patches merged in a immutable > branch merged in the clock and DT pull-requests, it worked perfectly so far > and neither Stephen or Arnd complained about that. Arnd, Olof, Any changes in the policies? Do you allow now driver branches (with driver code) to be merged into DT branch? Best regards, Krzysztof
On Mon, May 15, 2023, at 18:22, neil.armstrong@linaro.org wrote: > On 15/05/2023 18:15, Krzysztof Kozlowski wrote: >> On 15/05/2023 18:13, Krzysztof Kozlowski wrote: >> >> Also one more argument maybe not relevant here but for other cases - >> this makes literally impossible to include the clock ID in DTS in the >> same kernel revision, because you must not merge driver branch to DTS >> branch. SoC folks were complaining about this many times. > > Actually we handle this very simply by having such patches merged in a immutable > branch merged in the clock and DT pull-requests, it worked perfectly so far > and neither Stephen or Arnd complained about that. It's usually benign if you just add a new clk at the end of the binding header, as that doesn't touch the internal header file in the same commit. I'm certainly happier about drivers that just use numbers from a datasheet instead of having to come up with numbers to stick in a binding because the hardware is entirely irregular, but there is usually no point trying to complain about bad hardware to the driver authors -- I unsterstand you are just trying to make things work. I agree with Krzysztof that using the same identifiers in the local header and in the binding is just making your life harder for no reason, and if you are the only ones doing it this way, it would help to change it. Maybe just add a namespace prefix to all the internal macros so the next time you move one into the documented bindings you can do it with the same immutable branch hack but not include the driver changes in the dt branch. Arnd
On 16/05/2023 10:44, Arnd Bergmann wrote: > On Mon, May 15, 2023, at 18:22, neil.armstrong@linaro.org wrote: >> On 15/05/2023 18:15, Krzysztof Kozlowski wrote: >>> On 15/05/2023 18:13, Krzysztof Kozlowski wrote: >>> >>> Also one more argument maybe not relevant here but for other cases - >>> this makes literally impossible to include the clock ID in DTS in the >>> same kernel revision, because you must not merge driver branch to DTS >>> branch. SoC folks were complaining about this many times. >> >> Actually we handle this very simply by having such patches merged in a immutable >> branch merged in the clock and DT pull-requests, it worked perfectly so far >> and neither Stephen or Arnd complained about that. > > It's usually benign if you just add a new clk at the end of the binding > header, as that doesn't touch the internal header file in the same > commit. I'm certainly happier about drivers that just use numbers from > a datasheet instead of having to come up with numbers to stick in a binding > because the hardware is entirely irregular, but there is usually no point > trying to complain about bad hardware to the driver authors -- I unsterstand > you are just trying to make things work. > > I agree with Krzysztof that using the same identifiers in the local > header and in the binding is just making your life harder for no > reason, and if you are the only ones doing it this way, it would > help to change it. Maybe just add a namespace prefix to all the internal > macros so the next time you move one into the documented bindings you > can do it with the same immutable branch hack but not include the > driver changes in the dt branch. Ack, I'll try to find a simple intermediate solution to avoid this situation. Thanks, Neil > > Arnd
On Tue 16 May 2023 at 11:00, Neil Armstrong <neil.armstrong@linaro.org> wrote: > On 16/05/2023 10:44, Arnd Bergmann wrote: >> On Mon, May 15, 2023, at 18:22, neil.armstrong@linaro.org wrote: >>> On 15/05/2023 18:15, Krzysztof Kozlowski wrote: >>>> On 15/05/2023 18:13, Krzysztof Kozlowski wrote: >>>> >>>> Also one more argument maybe not relevant here but for other cases - >>>> this makes literally impossible to include the clock ID in DTS in the >>>> same kernel revision, because you must not merge driver branch to DTS >>>> branch. SoC folks were complaining about this many times. >>> >>> Actually we handle this very simply by having such patches merged in a immutable >>> branch merged in the clock and DT pull-requests, it worked perfectly so far >>> and neither Stephen or Arnd complained about that. >> It's usually benign if you just add a new clk at the end of the binding >> header, as that doesn't touch the internal header file in the same >> commit. I'm certainly happier about drivers that just use numbers from >> a datasheet instead of having to come up with numbers to stick in a binding >> because the hardware is entirely irregular, but there is usually no point >> trying to complain about bad hardware to the driver authors -- I unsterstand >> you are just trying to make things work. >> I agree with Krzysztof that using the same identifiers in the local >> header and in the binding is just making your life harder for no >> reason, and if you are the only ones doing it this way, it would >> help to change it. Maybe just add a namespace prefix to all the internal >> macros so the next time you move one into the documented bindings you >> can do it with the same immutable branch hack but not include the >> driver changes in the dt branch. > > Ack, I'll try to find a simple intermediate solution to avoid this situation. I'd in favor of keeping things simple and just put all the IDs in the bindings. We have been doodling with the idea for while, I think now is the time > > Thanks, > Neil > >> Arnd
diff --git a/drivers/clk/meson/g12a.h b/drivers/clk/meson/g12a.h index a97613df38b3..1a4a626c2c63 100644 --- a/drivers/clk/meson/g12a.h +++ b/drivers/clk/meson/g12a.h @@ -168,7 +168,6 @@ #define CLKID_VID_PLL_SEL 130 #define CLKID_VID_PLL_DIV 131 #define CLKID_VCLK_SEL 132 -#define CLKID_VCLK2_SEL 133 #define CLKID_VCLK_INPUT 134 #define CLKID_VCLK2_INPUT 135 #define CLKID_VCLK_DIV 136 diff --git a/include/dt-bindings/clock/g12a-clkc.h b/include/dt-bindings/clock/g12a-clkc.h index a93b58c5e18e..80421d7982dd 100644 --- a/include/dt-bindings/clock/g12a-clkc.h +++ b/include/dt-bindings/clock/g12a-clkc.h @@ -108,6 +108,7 @@ #define CLKID_VAPB 124 #define CLKID_HDMI_PLL 128 #define CLKID_VID_PLL 129 +#define CLKID_VCLK2_SEL 133 #define CLKID_VCLK 138 #define CLKID_VCLK2 139 #define CLKID_VCLK_DIV1 148 @@ -149,5 +150,7 @@ #define CLKID_NNA_CORE_CLK 267 #define CLKID_MIPI_DSI_PXCLK_SEL 269 #define CLKID_MIPI_DSI_PXCLK 270 +#define CLKID_CTS_ENCL 271 +#define CLKID_CTS_ENCL_SEL 272 #endif /* __G12A_CLKC_H */
Expose VCLK2_SEL clock id and add new ids for the CTS_ENCL and CTS_ENCL_SEL clocks on G12A compatible SoCs. Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- drivers/clk/meson/g12a.h | 1 - include/dt-bindings/clock/g12a-clkc.h | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-)