| Message ID | 20241121-ge-ian-debug-imx8-clk-tree-v1-0-0f1b722588fe@bootlin.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | clk: Fix simple video pipelines on i.MX8 | expand |
Hi Miquel, On 11/22/2024, Miquel Raynal wrote: > Recent changes in the clock tree have set CLK_SET_RATE_PARENT to the two > LCDIF pixel clocks. The idea is, instead of using assigned-clock > properties to set upstream PLL rates to high frequencies and hoping that > a single divisor (namely media_disp[12]_pix) will be close enough in > most cases, we should tell the clock core to use the PLL to properly > derive an accurate pixel clock rate in the first place. Here is the > situation. > > [Before ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")] > > Before setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the sequence of events was: > - PLL is assigned to a high rate, > - media_disp[12]_pix is set to approximately freq A by using a single divisor, > - media_ldb is set to approximately freq 7*A by using another single divisor. > => The display was working, but the pixel clock was inaccurate. > > [After ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")] > > After setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the > sequence of events became: > - media_disp[12]_pix is set to freq A by using a divisor of 1 and > setting video_pll1 to freq A. > - media_ldb is trying to compute its divisor to set freq 7*A, but the > upstream PLL is to low, it does not recompute it, so it ends up > setting a divisor of 1 and being at freq A instead of 7*A. > => The display is sadly no longer working > > [After applying PATCH "clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate"] > > This is a commit from Marek, which is, I believe going in the right > direction, so I am including it. Just with this change, the situation is > slightly different, but the result is the same: > - media_disp[12]_pix is set to freq A by using a divisor of 1 and > setting video_pll1 to freq A. > - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1 > to freq 7*A. > /!\ This as the side effect of changing media_disp[12]_pix from freq A > to freq 7*A. Although I'm not of a fan of setting CLK_SET_RATE_PARENT flag to the LDB clock and pixel clocks, would it work if the pixel clock rate is set after the LDB clock rate is set in fsl_ldb_atomic_enable()? The pixel clock can be got from LDB's remote input LCDIF DT node by calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch does. Similar to setting pixel clock rate, I think a chance is that pixel clock enablement can be moved from LCDIF driver to fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change. https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/ Actually, one sibling patch of the above patch reverts ff06ea04e4cf because I thought "fixed PLL rate" is the only solution, though I'm discussing any alternative solution of "dynamically changeable PLL rate" with Marek in the thread of the sibling patch. BTW, as you know the LDB clock rate is 3.5x faster than the pixel clock rate in dual-link LVDS use cases, the lowest PLL rate needs to be explicitly set to 7x faster than the pixel clock rate *before* LDB clock rate is set. This way, the pixel clock would be derived from the PLL with integer division ratio = 7, not the unsupported 3.5. pixel LDB PLL A 3.5*A 7*A --> OK A 3.5*A 3.5*A --> not OK > => The display is still not working > > [After applying this series] > > The goal of the following patches is to prevent clock subtree walks to > "just recalculate" the pixel clocks, ignoring the fact that they should > no longer change. They should adapt their divisors to the new upstream > rates instead. As a result, the display pipeline is working again. > > Note: if more than one display is connected, we need the LDB driver to > act accordingly, thus the LDB driver must be adapted. Also, if accurate > pixel clocks are not possible with two different displays, we will still > need (at least for now) to make sure one of them is reparented to > another PLL, like the audio PLL (but audio PLL are of a different kind, > and are slightly less accurate). > > So this series aims at fixing the i.MX8MP display pipeline for simple > setups. Said otherwise, returning to the same level of support as > before, but with (hopefully) more accurate frequencies. I believe this > approach manages to fix both Marek situation and all people using a > straightforward LCD based setup. For more complex setups, we need more > smartness from DRM and clk, but this is gonna take a bit of time. > > --- > Marek Vasut (1): > clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate > > Miquel Raynal (4): > clk: Add a helper to determine a clock rate > clk: Split clk_calc_subtree() > clk: Add flag to prevent frequency changes when walking subtrees > clk: imx: imx8mp: Prevent media clocks to be incompatibly changed > > drivers/clk/clk.c | 39 ++++++++++++++++++++++++++++++++------- > drivers/clk/imx/clk-imx8mp.c | 6 +++--- > include/linux/clk-provider.h | 2 ++ > 3 files changed, 37 insertions(+), 10 deletions(-) > --- > base-commit: 62facaf164585923d081eedcb6871f4ff3c2e953 > change-id: 20241121-ge-ian-debug-imx8-clk-tree-bd325aa866f1 > > Best regards,
Hello Liu, Thanks for the feedback! On 22/11/2024 at 14:01:49 +08, Liu Ying <victor.liu@nxp.com> wrote: > Hi Miquel, > > On 11/22/2024, Miquel Raynal wrote: >> Recent changes in the clock tree have set CLK_SET_RATE_PARENT to the two >> LCDIF pixel clocks. The idea is, instead of using assigned-clock >> properties to set upstream PLL rates to high frequencies and hoping that >> a single divisor (namely media_disp[12]_pix) will be close enough in >> most cases, we should tell the clock core to use the PLL to properly >> derive an accurate pixel clock rate in the first place. Here is the >> situation. >> >> [Before ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")] >> >> Before setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the sequence of events was: >> - PLL is assigned to a high rate, >> - media_disp[12]_pix is set to approximately freq A by using a single divisor, >> - media_ldb is set to approximately freq 7*A by using another single divisor. >> => The display was working, but the pixel clock was inaccurate. >> >> [After ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")] >> >> After setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the >> sequence of events became: >> - media_disp[12]_pix is set to freq A by using a divisor of 1 and >> setting video_pll1 to freq A. >> - media_ldb is trying to compute its divisor to set freq 7*A, but the >> upstream PLL is to low, it does not recompute it, so it ends up >> setting a divisor of 1 and being at freq A instead of 7*A. >> => The display is sadly no longer working >> >> [After applying PATCH "clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate"] >> >> This is a commit from Marek, which is, I believe going in the right >> direction, so I am including it. Just with this change, the situation is >> slightly different, but the result is the same: >> - media_disp[12]_pix is set to freq A by using a divisor of 1 and >> setting video_pll1 to freq A. >> - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1 >> to freq 7*A. >> /!\ This as the side effect of changing media_disp[12]_pix from freq A >> to freq 7*A. > > Although I'm not of a fan of setting CLK_SET_RATE_PARENT flag to the > LDB clock and pixel clocks, I haven't commented much on this. For me, inaccurate pixel clocks mostly work fine (if not too inaccurate), but it is true that having very powerful PLL like the PLL1443, it is a pity not to use them at their highest capabilities. However, I consider "not breaking users" more important than having "perfect clock rates". This series has one unique goal: accepting more accurate frequencies *and* not breaking users in the most simplest cases. > would it work if the pixel clock rate is > set after the LDB clock rate is set in fsl_ldb_atomic_enable()? The situation would be: - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1 to freq 7*A. - media_disp[12]_pix is set to freq A by using a divisor of 7. So yes, and the explanation of why is there: https://elixir.bootlin.com/linux/v6.11.8/source/drivers/clk/clk-divider.c#L322 > The > pixel clock can be got from LDB's remote input LCDIF DT node by > calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch > does. Similar to setting pixel clock rate, I think a chance is that > pixel clock enablement can be moved from LCDIF driver to > fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change. TBH, this sounds like a hack and is no longer required with this series. You are just trying to circumvent the fact that until now, applying a rate in an upper clock would unconfigure the downstream rates, and I think this is our first real problem. > https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/ > > Actually, one sibling patch of the above patch reverts ff06ea04e4cf > because I thought "fixed PLL rate" is the only solution, though I'm > discussing any alternative solution of "dynamically changeable PLL > rate" with Marek in the thread of the sibling patch. I don't think we want fixed PLL rates. Especially if you start using external (hot-pluggable) displays with different needs: it just does not fly. There is one situation that cannot yet be handled and needs manual reparenting: using 3 displays with a non-divisible pixel frequency. FYI we managed this specific "advanced" case with assigned-clock-parents using an audio PLL as hinted by Marek. It mostly works, event though the PLL1416 are less precise and offer less accurate pixel clocks. > BTW, as you know the LDB clock rate is 3.5x faster than the pixel > clock rate in dual-link LVDS use cases, the lowest PLL rate needs to > be explicitly set to 7x faster than the pixel clock rate *before* > LDB clock rate is set. This way, the pixel clock would be derived > from the PLL with integer division ratio = 7, not the unsupported > 3.5. > > pixel LDB PLL > A 3.5*A 7*A --> OK > A 3.5*A 3.5*A --> not OK This series was mostly solving the simpler case, with one display, but I agree we should probably also consider the dual case. The situation here is that you require the LDB to be aware of some clocks constraints, like the fact that the downstream pixel clocks only feature simple dividors which cannot achieve a 3.5 rate. That is all. It is clearly the LDB driver duty to make this feasible. I cannot test the dual case so I didn't brought any solution to it in this series, but I already had a solution in mind. Please find a patch below, it is very simple, and should, in conjunction with this series, fix the dual case as well. FYI here is the final clock tree with this trick "manually" enabled. You can see video_pll1 is now twice media_ldb, and media ldb is still 7 times media_disp[12]_pix (video_pll1 is assigned in DT to 1039500000, so it has effectively been reconfigured). video_pll1 1 1 0 1006600000 video_pll1_bypass 1 1 0 1006600000 video_pll1_out 2 2 0 1006600000 media_ldb 1 1 0 503300000 media_ldb_root_clk 1 1 0 503300000 media_disp2_pix 1 1 0 71900000 media_disp2_pix_root_clk 1 1 0 71900000 media_disp1_pix 0 0 0 71900000 media_disp1_pix_root_clk 0 0 0 71900000 ---8<--- Author: Miquel Raynal <miquel.raynal@bootlin.com> drm: bridge: ldb: Make sure the upper PLL is compatible with dual output The i.MX8 display pipeline has a number of clock constraints, among which: - The bridge clock must be 7 times faster than the pixel clock in single mode - The bridge clock must be 3.5 times faster than the pixel clocks in dual mode While a ratio of 7 is easy to build with simple divisors, 3.5 is not achievable. In order to make sure we keep these clock ratios correct is to configure the upper clock (usually video_pll1, but that does not matter really) to twice it's usual value. This way, the bridge clock is configured to divide the upstream rate by 2, and the pixel clocks are configured to divide the upstream rate by 7, achieving a final 3.5 ratio between the two. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c index 81ff4e5f52fa..069c960ee56b 100644 --- a/drivers/gpu/drm/bridge/fsl-ldb.c +++ b/drivers/gpu/drm/bridge/fsl-ldb.c @@ -177,6 +177,17 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge, mode = &crtc_state->adjusted_mode; requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock); + /* + * Dual cases require a 3.5 rate division on the pixel clocks, which + * cannot be achieved with regular single divisors. Instead, double the + * parent PLL rate in the first place. In order to do that, we first + * require twice the target clock rate, which will program the upper + * PLL. Then, we ask for the actual targeted rate, which can be achieved + * by dividing by 2 the already configured upper PLL rate, without + * making further changes to it. + */ + if (fsl_ldb_is_dual(fsl_ldb)) + clk_set_rate(fsl_ldb->clk, requested_link_freq * 2); clk_set_rate(fsl_ldb->clk, requested_link_freq); configured_link_freq = clk_get_rate(fsl_ldb->clk);
On 11/22/2024, Miquel Raynal wrote: > Hello Liu, Hello Miquel, > > Thanks for the feedback! > > On 22/11/2024 at 14:01:49 +08, Liu Ying <victor.liu@nxp.com> wrote: > >> Hi Miquel, >> >> On 11/22/2024, Miquel Raynal wrote: >>> Recent changes in the clock tree have set CLK_SET_RATE_PARENT to the two >>> LCDIF pixel clocks. The idea is, instead of using assigned-clock >>> properties to set upstream PLL rates to high frequencies and hoping that >>> a single divisor (namely media_disp[12]_pix) will be close enough in >>> most cases, we should tell the clock core to use the PLL to properly >>> derive an accurate pixel clock rate in the first place. Here is the >>> situation. >>> >>> [Before ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")] >>> >>> Before setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the sequence of events was: >>> - PLL is assigned to a high rate, >>> - media_disp[12]_pix is set to approximately freq A by using a single divisor, >>> - media_ldb is set to approximately freq 7*A by using another single divisor. >>> => The display was working, but the pixel clock was inaccurate. >>> >>> [After ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")] >>> >>> After setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the >>> sequence of events became: >>> - media_disp[12]_pix is set to freq A by using a divisor of 1 and >>> setting video_pll1 to freq A. >>> - media_ldb is trying to compute its divisor to set freq 7*A, but the >>> upstream PLL is to low, it does not recompute it, so it ends up >>> setting a divisor of 1 and being at freq A instead of 7*A. >>> => The display is sadly no longer working >>> >>> [After applying PATCH "clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate"] >>> >>> This is a commit from Marek, which is, I believe going in the right >>> direction, so I am including it. Just with this change, the situation is >>> slightly different, but the result is the same: >>> - media_disp[12]_pix is set to freq A by using a divisor of 1 and >>> setting video_pll1 to freq A. >>> - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1 >>> to freq 7*A. >>> /!\ This as the side effect of changing media_disp[12]_pix from freq A >>> to freq 7*A. >> >> Although I'm not of a fan of setting CLK_SET_RATE_PARENT flag to the >> LDB clock and pixel clocks, > > I haven't commented much on this. For me, inaccurate pixel clocks mostly > work fine (if not too inaccurate), but it is true that having very > powerful PLL like the PLL1443, it is a pity not to use them at their > highest capabilities. However, I consider "not breaking users" more > important than having "perfect clock rates". > > This series has one unique goal: accepting more accurate frequencies > *and* not breaking users in the most simplest cases. > >> would it work if the pixel clock rate is >> set after the LDB clock rate is set in fsl_ldb_atomic_enable()? > > The situation would be: > - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1 > to freq 7*A. > - media_disp[12]_pix is set to freq A by using a divisor of 7. > > So yes, and the explanation of why is there: > https://elixir.bootlin.com/linux/v6.11.8/source/drivers/clk/clk-divider.c#L322 OK, I agree it would work(even without this patch set). > >> The >> pixel clock can be got from LDB's remote input LCDIF DT node by >> calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch >> does. Similar to setting pixel clock rate, I think a chance is that >> pixel clock enablement can be moved from LCDIF driver to >> fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change. > > TBH, this sounds like a hack and is no longer required with this series. Pixel clock is an input signal to LDB, which is reflected in LDB block diagram in i.MX8MP TRM(see Figure 13-70) where "CLOCK" goes into LDB along with "DATA", "HSYNC", "VSYNC", "DATA_EN" and "CONTROL". So, fsl,ldb.yaml should have documented the pixel clock in clocks and clock-names properties, but unfortunately it doesn't and it's too late to do so. The workaround is to get the pixel clock from LCDIF DT node in the LDB driver. I would call it a workaround rather than a hack, since fsl,ldb.yaml should have documented the pixel clock in the first place. > > You are just trying to circumvent the fact that until now, applying a > rate in an upper clock would unconfigure the downstream rates, and I > think this is our first real problem. I'm still not a fan of setting CLK_SET_RATE_PARENT flag to the LDB clock and pixel clocks. If we look at the bigger picture, the first real problem is probably how to support both separated video PLLs and shared video PLL. > >> https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/ >> >> Actually, one sibling patch of the above patch reverts ff06ea04e4cf >> because I thought "fixed PLL rate" is the only solution, though I'm >> discussing any alternative solution of "dynamically changeable PLL >> rate" with Marek in the thread of the sibling patch. > > I don't think we want fixed PLL rates. Especially if you start using I don't want fixed PLL rates, either... > external (hot-pluggable) displays with different needs: it just does not ... but, considering the problem of how to support separated/shared video PLLs, I think we have to accept the fixed PLL rates. So, unfortunately some video modes read from EDID cannot fly. If there is an feasible alternative solution, it will be good to implement it, but till now I don't see any. > fly. There is one situation that cannot yet be handled and needs > manual reparenting: using 3 displays with a non-divisible pixel > frequency. > > FYI we managed this specific "advanced" case with assigned-clock-parents > using an audio PLL as hinted by Marek. It mostly works, event though the > PLL1416 are less precise and offer less accurate pixel clocks. Good to know this. I think if the audio PLL is free(e.g., it is not used by audio subsystem) on your i.MX8MP platform, you may use it for video output. However, i.MX8MP EVK made by NXP has to use shared video PLL between LVDS and MIPI DSI display pipelines, because all audio PLLs and PLL3 are supposed to be used by audio subsystem. > >> BTW, as you know the LDB clock rate is 3.5x faster than the pixel >> clock rate in dual-link LVDS use cases, the lowest PLL rate needs to >> be explicitly set to 7x faster than the pixel clock rate *before* >> LDB clock rate is set. This way, the pixel clock would be derived >> from the PLL with integer division ratio = 7, not the unsupported >> 3.5. >> >> pixel LDB PLL >> A 3.5*A 7*A --> OK >> A 3.5*A 3.5*A --> not OK > > This series was mostly solving the simpler case, with one display, but I > agree we should probably also consider the dual case. > > The situation here is that you require the LDB to be aware of some > clocks constraints, like the fact that the downstream pixel clocks only > feature simple dividors which cannot achieve a 3.5 rate. That is all. > > It is clearly the LDB driver duty to make this feasible. I cannot test > the dual case so I didn't brought any solution to it in this series, but > I already had a solution in mind. Please find a patch below, it is very > simple, and should, in conjunction with this series, fix the dual case > as well. > > FYI here is the final clock tree with this trick "manually" enabled. You > can see video_pll1 is now twice media_ldb, and media ldb is still 7 > times media_disp[12]_pix (video_pll1 is assigned in DT to 1039500000, so > it has effectively been reconfigured). > > video_pll1 1 1 0 1006600000 > video_pll1_bypass 1 1 0 1006600000 > video_pll1_out 2 2 0 1006600000 > media_ldb 1 1 0 503300000 > media_ldb_root_clk 1 1 0 503300000 > media_disp2_pix 1 1 0 71900000 > media_disp2_pix_root_clk 1 1 0 71900000 > media_disp1_pix 0 0 0 71900000 > media_disp1_pix_root_clk 0 0 0 71900000 > > ---8<--- > Author: Miquel Raynal <miquel.raynal@bootlin.com> > > drm: bridge: ldb: Make sure the upper PLL is compatible with dual output > > The i.MX8 display pipeline has a number of clock constraints, among which: > - The bridge clock must be 7 times faster than the pixel clock in single mode > - The bridge clock must be 3.5 times faster than the pixel clocks in dual mode > While a ratio of 7 is easy to build with simple divisors, 3.5 is not > achievable. In order to make sure we keep these clock ratios correct is > to configure the upper clock (usually video_pll1, but that does not > matter really) to twice it's usual value. This way, the bridge clock is > configured to divide the upstream rate by 2, and the pixel clocks are > configured to divide the upstream rate by 7, achieving a final 3.5 ratio > between the two. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c > index 81ff4e5f52fa..069c960ee56b 100644 > --- a/drivers/gpu/drm/bridge/fsl-ldb.c > +++ b/drivers/gpu/drm/bridge/fsl-ldb.c > @@ -177,6 +177,17 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge, > mode = &crtc_state->adjusted_mode; > > requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock); > + /* > + * Dual cases require a 3.5 rate division on the pixel clocks, which > + * cannot be achieved with regular single divisors. Instead, double the > + * parent PLL rate in the first place. In order to do that, we first > + * require twice the target clock rate, which will program the upper > + * PLL. Then, we ask for the actual targeted rate, which can be achieved > + * by dividing by 2 the already configured upper PLL rate, without > + * making further changes to it. > + */ > + if (fsl_ldb_is_dual(fsl_ldb)) > + clk_set_rate(fsl_ldb->clk, requested_link_freq * 2); I don't think i.MX6SX LDB needs this, because the "ldb" clock's parent is a mux clock with "ldb_di0_div_3_5" or "ldb_di0_div_7" parents. > clk_set_rate(fsl_ldb->clk, requested_link_freq); > > configured_link_freq = clk_get_rate(fsl_ldb->clk);
Hi Liu, >>> The >>> pixel clock can be got from LDB's remote input LCDIF DT node by >>> calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch >>> does. Similar to setting pixel clock rate, I think a chance is that >>> pixel clock enablement can be moved from LCDIF driver to >>> fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change. >> >> TBH, this sounds like a hack and is no longer required with this series. > > Pixel clock is an input signal to LDB, which is reflected in LDB block > diagram in i.MX8MP TRM(see Figure 13-70) where "CLOCK" goes into LDB > along with "DATA", "HSYNC", "VSYNC", "DATA_EN" and "CONTROL". So, > fsl,ldb.yaml should have documented the pixel clock in clocks and > clock-names properties, but unfortunately it doesn't and it's too late > to do so. The workaround is to get the pixel clock from LCDIF DT node > in the LDB driver. I would call it a workaround rather than a hack, > since fsl,ldb.yaml should have documented the pixel clock in the first > place. > >> >> You are just trying to circumvent the fact that until now, applying a >> rate in an upper clock would unconfigure the downstream rates, and I >> think this is our first real problem. > > I'm still not a fan of setting CLK_SET_RATE_PARENT flag to the LDB clock > and pixel clocks. If we look at the bigger picture, the first real > problem is probably how to support both separated video PLLs and shared > video PLL. > >> >>> https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/ >>> >>> Actually, one sibling patch of the above patch reverts ff06ea04e4cf >>> because I thought "fixed PLL rate" is the only solution, though I'm >>> discussing any alternative solution of "dynamically changeable PLL >>> rate" with Marek in the thread of the sibling patch. >> >> I don't think we want fixed PLL rates. Especially if you start using > > I don't want fixed PLL rates, either... > >> external (hot-pluggable) displays with different needs: it just does not > > ... but, considering the problem of how to support separated/shared video > PLLs, I think we have to accept the fixed PLL rates. So, unfortunately > some video modes read from EDID cannot fly. If there is an feasible > alternative solution, it will be good to implement it, but till now I > don't see any. Can you please remind me what your exact display setup (and their required pixel clocks) is? AFAIU, you don't want to use dynamic calculations for the PLL because it breaks your use case with HDMI. Of course this is a very limited use case, because using a static rate means almost a single type of display can be plugged. The clock subsystem will not recalculate the video_pll1 if you can achieve a perfect rate change using the LDB/PIX divisors. So let me propose the below addition to this series. With the below diff, your setup should still work with assigned clock rates, while letting us handle our calculations dynamically. The addition I am now proposing is to remove CLK_SET_RATE_PARENT to both media_disp[12]_pix clocks. This should actually fix your situation while keeping pixel clocks accurate as far it is possible as the LDB clock will change video_pll1 only if the PLL rate is not suitable for it in the first place. And then, there will be no other clock messing with this PLL. This is probably a safer approach, which should still allow accurate dynamic rate calculations for "simple" setups *and* let the static assignations work otherwise: - hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_SET_RATE_PARENT | CLK_NO_RATE_CHANGE_DURING_PROPAGATION); + hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_NO_RATE_CHANGE_DURING_PROPAGATION); [...] - hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT | CLK_NO_RATE_CHANGE_DURING_PROPAGATION); + hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_NO_RATE_CHANGE_DURING_PROPAGATION); Can you please give this proposal a try? [...] >> --- a/drivers/gpu/drm/bridge/fsl-ldb.c >> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c >> @@ -177,6 +177,17 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge, >> mode = &crtc_state->adjusted_mode; >> >> requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock); >> + /* >> + * Dual cases require a 3.5 rate division on the pixel clocks, which >> + * cannot be achieved with regular single divisors. Instead, double the >> + * parent PLL rate in the first place. In order to do that, we first >> + * require twice the target clock rate, which will program the upper >> + * PLL. Then, we ask for the actual targeted rate, which can be achieved >> + * by dividing by 2 the already configured upper PLL rate, without >> + * making further changes to it. >> + */ >> + if (fsl_ldb_is_dual(fsl_ldb)) >> + clk_set_rate(fsl_ldb->clk, requested_link_freq * 2); > > I don't think i.MX6SX LDB needs this, because the "ldb" clock's parent > is a mux clock with "ldb_di0_div_3_5" or "ldb_di0_div_7" parents. Ah, you mean we should only do this in the imx8 case, right? Thanks, Miquèl
On 11/26/2024, Miquel Raynal wrote: > Hi Liu, Hi, > >>>> The >>>> pixel clock can be got from LDB's remote input LCDIF DT node by >>>> calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch >>>> does. Similar to setting pixel clock rate, I think a chance is that >>>> pixel clock enablement can be moved from LCDIF driver to >>>> fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change. >>> >>> TBH, this sounds like a hack and is no longer required with this series. >> >> Pixel clock is an input signal to LDB, which is reflected in LDB block >> diagram in i.MX8MP TRM(see Figure 13-70) where "CLOCK" goes into LDB >> along with "DATA", "HSYNC", "VSYNC", "DATA_EN" and "CONTROL". So, >> fsl,ldb.yaml should have documented the pixel clock in clocks and >> clock-names properties, but unfortunately it doesn't and it's too late >> to do so. The workaround is to get the pixel clock from LCDIF DT node >> in the LDB driver. I would call it a workaround rather than a hack, >> since fsl,ldb.yaml should have documented the pixel clock in the first >> place. >> >>> >>> You are just trying to circumvent the fact that until now, applying a >>> rate in an upper clock would unconfigure the downstream rates, and I >>> think this is our first real problem. >> >> I'm still not a fan of setting CLK_SET_RATE_PARENT flag to the LDB clock >> and pixel clocks. If we look at the bigger picture, the first real >> problem is probably how to support both separated video PLLs and shared >> video PLL. >> >>> >>>> https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/ >>>> >>>> Actually, one sibling patch of the above patch reverts ff06ea04e4cf >>>> because I thought "fixed PLL rate" is the only solution, though I'm >>>> discussing any alternative solution of "dynamically changeable PLL >>>> rate" with Marek in the thread of the sibling patch. >>> >>> I don't think we want fixed PLL rates. Especially if you start using >> >> I don't want fixed PLL rates, either... >> >>> external (hot-pluggable) displays with different needs: it just does not >> >> ... but, considering the problem of how to support separated/shared video >> PLLs, I think we have to accept the fixed PLL rates. So, unfortunately >> some video modes read from EDID cannot fly. If there is an feasible >> alternative solution, it will be good to implement it, but till now I >> don't see any. > > Can you please remind me what your exact display setup (and their > required pixel clocks) is? i.MX8MP EVK[1] supports MIPI DSI, LVDS and native HDMI outputs. MIPI DSI and LVDS outputs use MINI-SAS connectors and HDMI output uses a HDMI type A connector. You may easily find the connectors in the EVK pictures in [1]. IMX-MIPI-HDMI(ADV7535), IMX-LVDS-HDMI(IT6263 single LVDS link) and MX8-DSI-OLED1A(RAYDIUM RM67199 DSI panel) accessory boards[2] can be connected/plugged to the EVK through the MINI-SAS connectors. In addition, MX8-DSI-OLED1(RAYDIUM RM67191 DSI panel), IMX-DLVDS-HDMI(IT6263 single DLVDS link) and MX8-DLVDS-LCD1(koe,tx26d202vm0bwa dual-link LVDS panel) can also be connected to the EVK through the MINI-SAS connectors, though not listed in [2]. So, putting native HDMI output aside, there are quite a few MIPI DSI + LVDS display device combinations to be supported with a single video PLL. For ADV7535 and IT6263, they support typical 1080p/720p video modes read from EDID with 148.5MHz and 74.25MHz pixel clock rates. For MX8-DLVDS-LCD1 LVDS panel, it is supported with in-tree imx8mp-evk-mx8-dlvds-lcd1.dtso where nominal 156.72MHz pixel clock rate is overridden to be 148.5MHz with a panel-timing DT node, considering the fixed video PLL rate @1.0395GHz assigned in imx8mp.dtsi. For MX8-DSI-OLED1 and MX8-DSI-OLED1A MIPI DSI panels, both have nominal 132MHz pixel clock rate(see panel-raydium-rm67191.c). NXP downstream kernel uses 148.5MHz pixel clock rate for the two panel. In short, I use 148.5MHz or 74.25MHz for ADV7535, IT6263 and the LVDS panel. I haven't tried the MIPI DSI panels with upstream kernel yet. [1] https://www.nxp.com/design/design-center/development-boards-and-designs/i-mx-evaluation-and-development-boards/evaluation-kit-for-the-i-mx-8m-plus-applications-processor:8MPLUSLPD4-EVK [2] https://www.nxp.com/design/design-center/development-boards-and-designs/i-mx-evaluation-and-development-boards/i-mx-8-series-accessory-boards:i.MX8-ACCESSORY-BOARDS > > AFAIU, you don't want to use dynamic calculations for the PLL because it > breaks your use case with HDMI. Of course this is a very limited use > case, because using a static rate means almost a single type of display > can be plugged. > > The clock subsystem will not recalculate the video_pll1 if you can > achieve a perfect rate change using the LDB/PIX divisors. So let me > propose the below addition to this series. With the below diff, your > setup should still work with assigned clock rates, while letting us > handle our calculations dynamically. > > The addition I am now proposing is to remove CLK_SET_RATE_PARENT to both > media_disp[12]_pix clocks. This should actually fix your situation while > keeping pixel clocks accurate as far it is possible as the LDB clock > will change video_pll1 only if the PLL rate is not suitable for it in > the first place. And then, there will be no other clock messing with > this PLL. This is probably a safer approach, which should still allow > accurate dynamic rate calculations for "simple" setups *and* let the > static assignations work otherwise: > > - hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_SET_RATE_PARENT | CLK_NO_RATE_CHANGE_DURING_PROPAGATION); > + hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_NO_RATE_CHANGE_DURING_PROPAGATION); > [...] > - hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT | CLK_NO_RATE_CHANGE_DURING_PROPAGATION); > + hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_NO_RATE_CHANGE_DURING_PROPAGATION); Removing CLK_SET_RATE_PARENT from the two pixel clocks is also done with my patch[3]. [3] https://patchwork.freedesktop.org/patch/624509/?series=139266&rev=7 > > Can you please give this proposal a try? I tried this/your patch set + your additional patch(remove CLK_SET_RATE_PARENT for pixel clocks) + your additional patch(set LDB clock rate to 2x requested link rate for dual-link LVDS)[4] + my two patches[5][6] with ADV7535 + IT6263 on i.MX8MP EVK. I don't see any particular issue for both single and dual link LVDS cases. Some typical video modes(1080p60/50, 720p60) work ok for ADV7535, though there is still no display for a few video modes(should be not caused by your patch set because there is no mode validation logic for the MIPI DSI display pipeline). So, basically, this has the same test result if simply only applying my patch set[7]. I think the key change for your patch set to work for my test case is your additional patch(remove CLK_SET_RATE_PARENT for pixel clocks). Together with [4][5][6], the video PLL rate is fixed to 1.0395GHz for all video modes. Back to the separated/shared PLLs problem, I still don't see any feasible alternative solution till now, since you are proposing to remove CLK_SET_RATE_PARENT for pixel clocks which defeats the idea of dynamically changing PLL rate(at least for the MIPI DSI display pipeline). Also, I feel that CLK_NO_RATE_CHANGE_DURING_PROPAGATION is not needed, because the pixel clock rate can be set in fsl_ldb_atomic_enable() as I mentioned before. [5] https://patchwork.freedesktop.org/patch/624511/?series=139266&rev=7 [6] https://patchwork.freedesktop.org/patch/624512/?series=139266&rev=7 [7] https://patchwork.freedesktop.org/series/139266/#rev7 > > [...] > >>> --- a/drivers/gpu/drm/bridge/fsl-ldb.c >>> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c >>> @@ -177,6 +177,17 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge, >>> mode = &crtc_state->adjusted_mode; >>> >>> requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock); >>> + /* >>> + * Dual cases require a 3.5 rate division on the pixel clocks, which >>> + * cannot be achieved with regular single divisors. Instead, double the >>> + * parent PLL rate in the first place. In order to do that, we first >>> + * require twice the target clock rate, which will program the upper >>> + * PLL. Then, we ask for the actual targeted rate, which can be achieved >>> + * by dividing by 2 the already configured upper PLL rate, without >>> + * making further changes to it. >>> + */ >>> + if (fsl_ldb_is_dual(fsl_ldb)) >>> + clk_set_rate(fsl_ldb->clk, requested_link_freq * 2); >> >> I don't think i.MX6SX LDB needs this, because the "ldb" clock's parent >> is a mux clock with "ldb_di0_div_3_5" or "ldb_di0_div_7" parents. > > Ah, you mean we should only do this in the imx8 case, right? I think that doing this does no harm for both i.MX8MP LDB and i.MX93 LDB. > > Thanks, > Miquèl
Recent changes in the clock tree have set CLK_SET_RATE_PARENT to the two LCDIF pixel clocks. The idea is, instead of using assigned-clock properties to set upstream PLL rates to high frequencies and hoping that a single divisor (namely media_disp[12]_pix) will be close enough in most cases, we should tell the clock core to use the PLL to properly derive an accurate pixel clock rate in the first place. Here is the situation. [Before ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")] Before setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the sequence of events was: - PLL is assigned to a high rate, - media_disp[12]_pix is set to approximately freq A by using a single divisor, - media_ldb is set to approximately freq 7*A by using another single divisor. => The display was working, but the pixel clock was inaccurate. [After ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")] After setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the sequence of events became: - media_disp[12]_pix is set to freq A by using a divisor of 1 and setting video_pll1 to freq A. - media_ldb is trying to compute its divisor to set freq 7*A, but the upstream PLL is to low, it does not recompute it, so it ends up setting a divisor of 1 and being at freq A instead of 7*A. => The display is sadly no longer working [After applying PATCH "clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate"] This is a commit from Marek, which is, I believe going in the right direction, so I am including it. Just with this change, the situation is slightly different, but the result is the same: - media_disp[12]_pix is set to freq A by using a divisor of 1 and setting video_pll1 to freq A. - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1 to freq 7*A. /!\ This as the side effect of changing media_disp[12]_pix from freq A to freq 7*A. => The display is still not working [After applying this series] The goal of the following patches is to prevent clock subtree walks to "just recalculate" the pixel clocks, ignoring the fact that they should no longer change. They should adapt their divisors to the new upstream rates instead. As a result, the display pipeline is working again. Note: if more than one display is connected, we need the LDB driver to act accordingly, thus the LDB driver must be adapted. Also, if accurate pixel clocks are not possible with two different displays, we will still need (at least for now) to make sure one of them is reparented to another PLL, like the audio PLL (but audio PLL are of a different kind, and are slightly less accurate). So this series aims at fixing the i.MX8MP display pipeline for simple setups. Said otherwise, returning to the same level of support as before, but with (hopefully) more accurate frequencies. I believe this approach manages to fix both Marek situation and all people using a straightforward LCD based setup. For more complex setups, we need more smartness from DRM and clk, but this is gonna take a bit of time. --- Marek Vasut (1): clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate Miquel Raynal (4): clk: Add a helper to determine a clock rate clk: Split clk_calc_subtree() clk: Add flag to prevent frequency changes when walking subtrees clk: imx: imx8mp: Prevent media clocks to be incompatibly changed drivers/clk/clk.c | 39 ++++++++++++++++++++++++++++++++------- drivers/clk/imx/clk-imx8mp.c | 6 +++--- include/linux/clk-provider.h | 2 ++ 3 files changed, 37 insertions(+), 10 deletions(-) --- base-commit: 62facaf164585923d081eedcb6871f4ff3c2e953 change-id: 20241121-ge-ian-debug-imx8-clk-tree-bd325aa866f1 Best regards,