| Message ID | 20230512-amlogic-v6-4-upstream-dsi-ccf-vim3-v4-0-2592c29ea263@linaro.org |
|---|---|
| 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
The Amlogic G12A, G12B & SM1 SoCs embeds a Synopsys DW-MIPI-DSI transceiver (ver 1.21a), with a custom glue managing the IP resets, clock and data input similar to the DW-HDMI glue on the same Amlogic SoCs. This adds support for the glue managing the transceiver, mimicing the init flow provided by Amlogic to setup the ENCL encoder, the glue, the transceiver, the digital D-PHY and the Analog PHY in the proper way. The DW-MIPI-DSI transceiver + D-PHY are clocked by the GP0 PLL, and the ENCL encoder + VIU pixel reader by the VCLK2 clock using the HDMI PLL. The DW-MIPI-DSI transceiver gets this pixel stream as input clocked with the VCLK2 clock. An optional "MEAS" clock can be enabled to measure the delay between each vsync feeding the DW-MIPI-DSI transceiver. This patchset is based on an earlier attempt at [1] for the AXG SoCs, but: - previous glue code was a single monolitic code mixing encoders & bridges, this version is aligned on the previous cleanup done on HDMI & CVBS bridges architecture at [2] - since the only output of AXG is DSI, AXG VPU support is post-poned until we implement single-clock DSI support specific case on top of this. This is a re-spin of v3 at [5], the main change is about clock control, the clock setup has been redesigned to use CCF, a common PLL (GP0) and the VCLK2 clock path for DSI in preparation of full CCF support and possibly dual display with HDMI. I kept review tags when the content was only slighly changed. To: Jerome Brunet <jbrunet@baylibre.com> To: Michael Turquette <mturquette@baylibre.com> To: Stephen Boyd <sboyd@kernel.org> To: Kevin Hilman <khilman@baylibre.com> To: Martin Blumenstingl <martin.blumenstingl@googlemail.com> To: Rob Herring <robh+dt@kernel.org> To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> To: Conor Dooley <conor+dt@kernel.org> To: David Airlie <airlied@gmail.com> To: Daniel Vetter <daniel@ffwll.ch> To: Philipp Zabel <p.zabel@pengutronix.de> To: Vinod Koul <vkoul@kernel.org> To: Kishon Vijay Abraham I <kishon@kernel.org> To: Sam Ravnborg <sam@ravnborg.org> Cc: Nicolas Belin <nbelin@baylibre.com> Cc: linux-amlogic@lists.infradead.org Cc: linux-clk@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Cc: devicetree@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linux-phy@lists.infradead.org Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> Changes from v3 at [5]: - switched all clk setup via CCF - using single PLL for DSI controller & ENCL encoder - added ENCL clocks to CCF - make the VCLK2 clocks configuration by CCF - fixed probe/bind of DSI controller to work with panels & bridges - added bit_clk to controller to it can setup the BIT clock aswell - added fix for components unbind - added fix for analog phy setup value - added TS050 timings fix - dropped previous clk control patch Changes from v2 at [4]: - Fixed patch 3 - Added reviews from Jagan - Rebased on v5.19-rc1 Changes from v1 at [3]: - fixed DSI host bindings - add reviewed-by tags for bindings - moved magic values to defines thanks to Martin's searches - added proper prefixes to defines - moved phy_configure to phy_init() dw-mipi-dsi callback - moved phy_on to a new phy_power_on() dw-mipi-dsi callback - correctly return phy_init/configure errors to callback returns [1] https://lore.kernel.org/r/20200907081825.1654-1-narmstrong@baylibre.com [2] https://lore.kernel.org/r/20211020123947.2585572-1-narmstrong@baylibre.com [3] https://lore.kernel.org/r/20200907081825.1654-1-narmstrong@baylibre.com [4] https://lore.kernel.org/r/20220120083357.1541262-1-narmstrong@baylibre.com [5] https://lore.kernel.org/r/20220617072723.1742668-1-narmstrong@baylibre.com --- Neil Armstrong (13): dt-bindings: clk: g12a-clkc: export VCLK2_SEL and add CTS_ENCL clock ids clk: meson: g12a: add CTS_ENCL & CTS_ENCL_SEL clocks clk: meson: g12a: make VCLK2 and ENCL clock path configurable by CCF dt-bindings: display: add Amlogic MIPI DSI Host Controller bindings dt-bindings: display: meson-vpu: add third DPI output port drm/meson: fix unbind path if HDMI fails to bind drm/meson: venc: add ENCL encoder setup for MIPI-DSI output drm/meson: add DSI encoder drm/meson: add support for MIPI-DSI transceiver phy: amlogic: phy-meson-g12a-mipi-dphy-analog: fix CNTL2_DIF_TX_CTL0 value drm/panel: khadas-ts050: update timings to achieve 60Hz refresh rate arm64: meson: g12-common: add the MIPI DSI nodes DONOTMERGE: arm64: meson: khadas-vim3l: add DSI panel .../display/amlogic,meson-g12a-dw-mipi-dsi.yaml | 117 +++++++ .../bindings/display/amlogic,meson-vpu.yaml | 5 + arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi | 70 ++++ .../boot/dts/amlogic/meson-g12b-khadas-vim3.dtsi | 2 +- arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi | 76 +++++ .../boot/dts/amlogic/meson-sm1-khadas-vim3l.dts | 2 +- drivers/clk/meson/g12a.c | 169 +++++++++- drivers/clk/meson/g12a.h | 3 +- drivers/gpu/drm/meson/Kconfig | 7 + drivers/gpu/drm/meson/Makefile | 3 +- drivers/gpu/drm/meson/meson_drv.c | 32 +- drivers/gpu/drm/meson/meson_drv.h | 1 + drivers/gpu/drm/meson/meson_dw_mipi_dsi.c | 364 +++++++++++++++++++++ drivers/gpu/drm/meson/meson_dw_mipi_dsi.h | 160 +++++++++ drivers/gpu/drm/meson/meson_encoder_dsi.c | 174 ++++++++++ drivers/gpu/drm/meson/meson_encoder_dsi.h | 13 + drivers/gpu/drm/meson/meson_registers.h | 25 ++ drivers/gpu/drm/meson/meson_venc.c | 211 +++++++++++- drivers/gpu/drm/meson/meson_venc.h | 6 + drivers/gpu/drm/meson/meson_vpp.h | 2 + drivers/gpu/drm/panel/panel-khadas-ts050.c | 16 +- .../phy/amlogic/phy-meson-g12a-mipi-dphy-analog.c | 2 +- include/dt-bindings/clock/g12a-clkc.h | 3 + 23 files changed, 1428 insertions(+), 35 deletions(-) --- base-commit: ac9a78681b921877518763ba0e89202254349d1b change-id: 20230512-amlogic-v6-4-upstream-dsi-ccf-vim3-b8e5217e1f4a Best regards,