| Message ID | 20130701203009.633A8FAAD5@dev.laptop.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/01/2013 10:30 PM, Daniel Drake wrote: > Here is a new patch which should incorporate all your previous feedback. > Now each variant passes clock info to the main driver via a new > armada_clk_info structure. > > A helper function in the core lets each variant find the best clock. > As you suggested we first try external ("dedicated") clocks, where we can > change the rate to find an exact match. We fall back on looking at the rates > of the other clocks and we return the clock that provides us with the closest > match after integer division (rejecting those that don't bring us within 1%). > > There is still the possibility that two CRTCs on the same device end up using > the same clock, so I added a usage counter to each clock to prevent the rate > from being changed by another CRTC. > > This is compile-tested only - but after your feedback I will add the > remaining required hacks and test it on XO. > > diff --git a/drivers/gpu/drm/armada/armada_510.c b/drivers/gpu/drm/armada/armada_510.c > index a016888..7dff2dc 100644 > --- a/drivers/gpu/drm/armada/armada_510.c > +++ b/drivers/gpu/drm/armada/armada_510.c > @@ -17,12 +17,29 @@ > > static int armada510_init(struct armada_private *priv, struct device *dev) > { > - priv->extclk[0] = devm_clk_get(dev, "ext_ref_clk_1"); > + struct armada_clk_info *clk_info = devm_kzalloc(dev, > + sizeof(struct armada_clk_info) * 4, GFP_KERNEL); > > - if (IS_ERR(priv->extclk[0])&& PTR_ERR(priv->extclk[0]) == -ENOENT) > - priv->extclk[0] = ERR_PTR(-EPROBE_DEFER); > + if (!clk_info) > + return -ENOMEM; > > - return PTR_RET(priv->extclk[0]); > + /* External clocks */ > + clk_info[0].clk = devm_clk_get(dev, "ext_ref_clk_0"); > + clk_info[0].is_dedicated = true; Daniel, I guess "extclk0" and "extclk1" should be sufficient for clock names. Also, they are not dedicated as you can have CRTC0 and CRTC1 use e.g. extclk0 simultaneously. See below for .is_dedicated in general. [...] > diff --git a/drivers/gpu/drm/armada/armada_drm.h b/drivers/gpu/drm/armada/armada_drm.h > index e8c4f80..4fe8ec5 100644 > --- a/drivers/gpu/drm/armada/armada_drm.h > +++ b/drivers/gpu/drm/armada/armada_drm.h > @@ -55,6 +55,20 @@ void armada_drm_vbl_event_remove_unlocked(struct armada_crtc *, > __e->fn = _f; \ > } while (0) > > +struct armada_clk_info { > + struct clk *clk; > + > + /* If this clock is dedicated to us, we can change its rate without > + * worrying about any other users in other parts of the system. */ > + bool is_dedicated; > + > + /* However, we cannot share the same dedicated clock between two CRTCs > + * if each CRTC wants a different rate. Track the number of users. */ > + int usage_count; You can share the same clock between two CRTCs. Just consider CRTC1 using a mode with half pixel clock as CRTC0. Not that I think this will ever happen, but it is possible. > + /* The bits in the SCLK register that select this clock */ > + uint32_t sclk; > +}; > > struct armada_private; > > @@ -77,7 +91,8 @@ struct armada_private { > struct drm_fb_helper *fbdev; > struct armada_crtc *dcrtc[2]; > struct drm_mm linear; > - struct clk *extclk[2]; > + int num_clks; > + struct armada_clk_info *clk_info; > struct drm_property *csc_yuv_prop; > struct drm_property *csc_rgb_prop; > struct drm_property *colorkey_prop; > @@ -99,6 +114,9 @@ void __armada_drm_queue_unref_work(struct drm_device *, > void armada_drm_queue_unref_work(struct drm_device *, > struct drm_framebuffer *); > > +struct armada_clk_info *armada_drm_find_best_clock(struct armada_private *priv, > + long rate, int *divider); > + > extern const struct drm_mode_config_funcs armada_drm_mode_config_funcs; > > int armada_fbdev_init(struct drm_device *); > diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c > index e0a08e9..411d56f 100644 > --- a/drivers/gpu/drm/armada/armada_drv.c > +++ b/drivers/gpu/drm/armada/armada_drv.c > @@ -16,6 +16,97 @@ > #include "armada_ioctl.h" > #include "armada_ioctlP.h" > > +/* Find the best clock and integer divisor for a given rate. > + * NULL is returned when no clock can be found. > + * When the return value is non-NULL, the divider output variable is set > + * appropriately, and the clock is returned in prepared state. */ > +struct armada_clk_info *armada_drm_find_best_clock(struct armada_private *priv, > + long rate, int *divider) I prefer not to try to find the best clock (source) at all. Let the user pass the clock name by e.g. platform_data (or DT) and just try to get the requested pixclk or a integer multiple of it. You will never be able to find the best clock for all use cases. Just figure out, if integer division is sufficient to get requested pixclk and clk_set_rate otherwise. This may be useful for current mode running at 148.5MHz (1080p50) and new mode at 74.25MHz (1080i50, 720p). > +{ > + int i; > + int div; > + long residue; > + long res_max; > + long ref; > + struct armada_clk_info *ret = NULL; > + > + /* Look for an unused dedicated clock (e.g. an external clock with > + * no other users) which can provide the desired rate exactly. In this > + * case we can change the clock rate to meet our needs. */ > + for (i = 0; i< priv->num_clks; i++) { > + struct armada_clk_info *clkinfo =&priv->clk_info[i]; > + struct clk *candidate = clkinfo->clk; > + if (IS_ERR(candidate)) > + continue; > + > + if (!clkinfo->is_dedicated || clkinfo->usage_count> 0) > + continue; > + > + if (clk_prepare(candidate)) > + continue; > + > + ref = clk_round_rate(candidate, rate); > + if (ref == rate) { > + *divider = 1; > + clk_set_rate(candidate, ref); > + return clkinfo; > + > + } > + clk_unprepare(candidate); > + } > + > + /* Fallback: look for a clock that can bring us within 1% of the > + * desired rate when an integer divider is applied. In this case we > + * do not change the clock's rate because the clock may be shared with > + * other elements. */ Just use clk_set_rate and don't try to make any assumptions. If the clock you get back is way out of requested pixclk either fail or just take what you get. HD/SD TV modes usually don't work with wrong pixclk while VESA modes do. Audio clock over HDMI will be broken and vert refresh will be wrong but it is nothing you can check here. Otherwise, I think the patch is going in the right direction. Maybe, also clk API should be extended someday to allow to get min/max frequency range. That will allow us to rule out some modes in a mode_valid callback based on the pixclk rate. Sebastian
On Mon, Jul 1, 2013 at 3:48 PM, Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > I guess "extclk0" and "extclk1" should be sufficient for clock names. > Also, they are not dedicated as you can have CRTC0 and CRTC1 use e.g. > extclk0 simultaneously. See below for .is_dedicated in general. Maybe we can find better terminology, or just document it a bit better, but having two CRTCs share the same clock falls within the scheme designed and implemented there. "Dedicated" simply means a clock that is dedicated to the display controller, it is not shared by other devices. >> +struct armada_clk_info { >> + struct clk *clk; >> + >> + /* If this clock is dedicated to us, we can change its rate >> without >> + * worrying about any other users in other parts of the system. */ >> + bool is_dedicated; >> + >> + /* However, we cannot share the same dedicated clock between two >> CRTCs >> + * if each CRTC wants a different rate. Track the number of users. >> */ >> + int usage_count; > > > You can share the same clock between two CRTCs. Just consider CRTC1 > using a mode with half pixel clock as CRTC0. Not that I think this will > ever happen, but it is possible. And my implementation already lets that happen - its just that I didn't document it in enough detail. > I prefer not to try to find the best clock (source) at all. Let the > user pass the clock name by e.g. platform_data (or DT) and just try to > get the requested pixclk or a integer multiple of it. You will never be > able to find the best clock for all use cases. > > Just figure out, if integer division is sufficient to get requested > pixclk and clk_set_rate otherwise. This may be useful for current mode > running at 148.5MHz (1080p50) and new mode at 74.25MHz (1080i50, 720p). I am not opposed to this approach, it is nice and simple, but as Russell points out we do additionally need to distinguish between clocks that are "ours" to play with, vs those that are shared with other devices. It would be a bad idea to try call clk_set_rate() on the AXI bus clock, for example, changing the clock for a whole bunch of other devices. This is what the is_dedicated flag is about. However such a flag could be easily added to the DT/platform data definition that you propose. Daniel
On 07/02/13 03:57, Daniel Drake wrote: > On Mon, Jul 1, 2013 at 3:48 PM, Sebastian Hesselbarth > <sebastian.hesselbarth@gmail.com> wrote: >> I prefer not to try to find the best clock (source) at all. Let the >> user pass the clock name by e.g. platform_data (or DT) and just try to >> get the requested pixclk or a integer multiple of it. You will never be >> able to find the best clock for all use cases. >> >> Just figure out, if integer division is sufficient to get requested >> pixclk and clk_set_rate otherwise. This may be useful for current mode >> running at 148.5MHz (1080p50) and new mode at 74.25MHz (1080i50, 720p). > > I am not opposed to this approach, it is nice and simple, but as > Russell points out we do additionally need to distinguish between > clocks that are "ours" to play with, vs those that are shared with > other devices. It would be a bad idea to try call clk_set_rate() on > the AXI bus clock, for example, changing the clock for a whole bunch > of other devices. This is what the is_dedicated flag is about. However > such a flag could be easily added to the DT/platform data definition > that you propose. Daniel, now I got the idea of .is_dedicated. At least for Dove, we could still implement AXI bus clock as fixed-rate clock, so you cannot mess with it. I am almost sure, it will hang Dove when you change it, as there are some devices depending on it (and the correct rate of it). Moreover, LCDCLK is dedicated to CRTC0/1 so AXICLK is the only clock not dedicated to LCD controllers. But I am fine with .is_dedicated - I just think we should not try to find the best clock source but leave that decision to the user (=developer, DTS author; not userspace user). Sebastian
diff --git a/drivers/gpu/drm/armada/armada_510.c b/drivers/gpu/drm/armada/armada_510.c index a016888..7dff2dc 100644 --- a/drivers/gpu/drm/armada/armada_510.c +++ b/drivers/gpu/drm/armada/armada_510.c @@ -17,12 +17,29 @@ static int armada510_init(struct armada_private *priv, struct device *dev) { - priv->extclk[0] = devm_clk_get(dev, "ext_ref_clk_1"); + struct armada_clk_info *clk_info = devm_kzalloc(dev, + sizeof(struct armada_clk_info) * 4, GFP_KERNEL); - if (IS_ERR(priv->extclk[0]) && PTR_ERR(priv->extclk[0]) == -ENOENT) - priv->extclk[0] = ERR_PTR(-EPROBE_DEFER); + if (!clk_info) + return -ENOMEM; - return PTR_RET(priv->extclk[0]); + /* External clocks */ + clk_info[0].clk = devm_clk_get(dev, "ext_ref_clk_0"); + clk_info[0].is_dedicated = true; + clk_info[0].sclk = SCLK_510_EXTCLK0; + clk_info[1].clk = devm_clk_get(dev, "ext_ref_clk_1"); + clk_info[1].is_dedicated = true; + clk_info[1].sclk = SCLK_510_EXTCLK1; + + /* Internal/shared clocks */ + clk_info[2].clk = devm_clk_get(dev, "axi"); + clk_info[2].sclk = SCLK_510_AXI; + clk_info[3].clk = devm_clk_get(dev, "pll"); + clk_info[3].sclk = SCLK_510_PLL; + + priv->num_clks = 4; + priv->clk_info = clk_info; + return 0; } static int armada510_crtc_init(struct armada_crtc *dcrtc) @@ -38,42 +55,25 @@ static int armada510_crtc_init(struct armada_crtc *dcrtc) * supportable, and again with sclk != NULL to set the clocks up for * that. The former can return an error, but the latter is expected * not to. - * - * We currently are pretty rudimentary here, always selecting - * EXT_REF_CLK_1 for LCD0 and erroring LCD1. This needs improvement! */ static int armada510_crtc_compute_clock(struct armada_crtc *dcrtc, const struct drm_display_mode *mode, uint32_t *sclk) { struct armada_private *priv = dcrtc->crtc.dev->dev_private; - struct clk *clk = priv->extclk[0]; - int ret; - - if (dcrtc->num == 1) - return -EINVAL; - - if (IS_ERR(clk)) - return PTR_ERR(clk); - - if (dcrtc->clk != clk) { - ret = clk_prepare_enable(clk); - if (ret) - return ret; - dcrtc->clk = clk; - } + struct armada_clk_info *clk_info; + int err; + int divider; - if (sclk) { - uint32_t rate, ref, div; + clk_info = armada_drm_find_best_clock(priv, mode->clock * 1000, ÷r); + if (!clk_info) + return -ENOENT; - rate = mode->clock * 1000; - ref = clk_round_rate(clk, rate); - div = DIV_ROUND_UP(ref, rate); - if (div < 1) - div = 1; + err = armada_drm_crtc_switch_clock(dcrtc, clk_info); + if (err) + return err; - clk_set_rate(clk, ref); - *sclk = div | SCLK_510_EXTCLK1; - } + if (sclk) + *sclk = divider | clk_info->sclk; return 0; } diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index f489157..8202be2 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -83,6 +83,34 @@ enum csc_mode { * porch, which we're reprogramming.) */ +/* Switch to a new clock source after disabling and unpreparing the current + * one. If non-NULL, the new clock source is expected to be prepared before + * this function is called. */ +int armada_drm_crtc_switch_clock(struct armada_crtc *dcrtc, + struct armada_clk_info *clk_info) +{ + int err; + + if (dcrtc->active_clk == clk_info) + return 0; + + if (dcrtc->active_clk) { + clk_disable_unprepare(dcrtc->active_clk->clk); + dcrtc->active_clk->usage_count--; + dcrtc->active_clk = NULL; + } + + if (clk_info) { + err = clk_enable(clk_info->clk); + if (err) + return err; + dcrtc->active_clk = clk_info; + clk_info->usage_count++; + } + + return 0; +} + void armada_drm_crtc_update_regs(struct armada_crtc *dcrtc, struct armada_regs *regs) { @@ -647,10 +675,7 @@ static void armada_drm_crtc_destroy(struct drm_crtc *crtc) priv->dcrtc[dcrtc->num] = NULL; drm_crtc_cleanup(&dcrtc->crtc); - - if (!IS_ERR(dcrtc->clk)) - clk_disable_unprepare(dcrtc->clk); - + armada_drm_crtc_switch_clock(dcrtc, NULL); kfree(dcrtc); } @@ -814,7 +839,6 @@ int armada_drm_crtc_create(struct drm_device *dev, unsigned num, dcrtc->base = base; dcrtc->num = num; - dcrtc->clk = ERR_PTR(-EINVAL); dcrtc->csc_yuv_mode = CSC_AUTO; dcrtc->csc_rgb_mode = CSC_AUTO; dcrtc->cfg_dumb_ctrl = DUMB24_RGB888_0; diff --git a/drivers/gpu/drm/armada/armada_crtc.h b/drivers/gpu/drm/armada/armada_crtc.h index 972da53..f260676 100644 --- a/drivers/gpu/drm/armada/armada_crtc.h +++ b/drivers/gpu/drm/armada/armada_crtc.h @@ -37,7 +37,7 @@ struct armada_crtc { struct drm_crtc crtc; unsigned num; void __iomem *base; - struct clk *clk; + struct armada_clk_info *active_clk; struct { uint32_t spu_v_h_total; uint32_t spu_v_porch; @@ -70,5 +70,7 @@ void armada_drm_crtc_irq(struct armada_crtc *, u32); void armada_drm_crtc_disable_irq(struct armada_crtc *, u32); void armada_drm_crtc_enable_irq(struct armada_crtc *, u32); void armada_drm_crtc_update_regs(struct armada_crtc *, struct armada_regs *); +int armada_drm_crtc_switch_clock(struct armada_crtc *dcrtc, + struct armada_clk_info *clk_info); #endif diff --git a/drivers/gpu/drm/armada/armada_drm.h b/drivers/gpu/drm/armada/armada_drm.h index e8c4f80..4fe8ec5 100644 --- a/drivers/gpu/drm/armada/armada_drm.h +++ b/drivers/gpu/drm/armada/armada_drm.h @@ -55,6 +55,20 @@ void armada_drm_vbl_event_remove_unlocked(struct armada_crtc *, __e->fn = _f; \ } while (0) +struct armada_clk_info { + struct clk *clk; + + /* If this clock is dedicated to us, we can change its rate without + * worrying about any other users in other parts of the system. */ + bool is_dedicated; + + /* However, we cannot share the same dedicated clock between two CRTCs + * if each CRTC wants a different rate. Track the number of users. */ + int usage_count; + + /* The bits in the SCLK register that select this clock */ + uint32_t sclk; +}; struct armada_private; @@ -77,7 +91,8 @@ struct armada_private { struct drm_fb_helper *fbdev; struct armada_crtc *dcrtc[2]; struct drm_mm linear; - struct clk *extclk[2]; + int num_clks; + struct armada_clk_info *clk_info; struct drm_property *csc_yuv_prop; struct drm_property *csc_rgb_prop; struct drm_property *colorkey_prop; @@ -99,6 +114,9 @@ void __armada_drm_queue_unref_work(struct drm_device *, void armada_drm_queue_unref_work(struct drm_device *, struct drm_framebuffer *); +struct armada_clk_info *armada_drm_find_best_clock(struct armada_private *priv, + long rate, int *divider); + extern const struct drm_mode_config_funcs armada_drm_mode_config_funcs; int armada_fbdev_init(struct drm_device *); diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c index e0a08e9..411d56f 100644 --- a/drivers/gpu/drm/armada/armada_drv.c +++ b/drivers/gpu/drm/armada/armada_drv.c @@ -16,6 +16,97 @@ #include "armada_ioctl.h" #include "armada_ioctlP.h" +/* Find the best clock and integer divisor for a given rate. + * NULL is returned when no clock can be found. + * When the return value is non-NULL, the divider output variable is set + * appropriately, and the clock is returned in prepared state. */ +struct armada_clk_info *armada_drm_find_best_clock(struct armada_private *priv, + long rate, int *divider) +{ + int i; + int div; + long residue; + long res_max; + long ref; + struct armada_clk_info *ret = NULL; + + /* Look for an unused dedicated clock (e.g. an external clock with + * no other users) which can provide the desired rate exactly. In this + * case we can change the clock rate to meet our needs. */ + for (i = 0; i < priv->num_clks; i++) { + struct armada_clk_info *clkinfo = &priv->clk_info[i]; + struct clk *candidate = clkinfo->clk; + if (IS_ERR(candidate)) + continue; + + if (!clkinfo->is_dedicated || clkinfo->usage_count > 0) + continue; + + if (clk_prepare(candidate)) + continue; + + ref = clk_round_rate(candidate, rate); + if (ref == rate) { + *divider = 1; + clk_set_rate(candidate, ref); + return clkinfo; + + } + clk_unprepare(candidate); + } + + /* Fallback: look for a clock that can bring us within 1% of the + * desired rate when an integer divider is applied. In this case we + * do not change the clock's rate because the clock may be shared with + * other elements. */ + res_max = rate / 100; + for (i = 0; i < priv->num_clks; i++) { + struct armada_clk_info *clkinfo = &priv->clk_info[i]; + struct clk *candidate = clkinfo->clk; + long ref_residue; + + if (IS_ERR(candidate) || clk_prepare(candidate)) + continue; + + ref = clk_get_rate(candidate); + ref_residue = ref % rate; + + /* Normalize the residue value: if its at the high end of + * the range, calculate its offset from the next possible + * divisor. */ + if (ref_residue > res_max) + ref_residue = rate - ref_residue; + + /* Ignore rates worse than 1% */ + if (ref_residue > res_max) { + clk_unprepare(candidate); + continue; + } + + /* Do we have a better previous match? */ + if (ret && residue < ref_residue) { + clk_unprepare(candidate); + continue; + } + + /* We have the best match, store it for later. */ + if (ret) + clk_unprepare(ret->clk); + + ret = clkinfo; + residue = ref_residue; + } + + if (ret) { + div = DIV_ROUND_UP(ref, rate); + if (div < 1) + div = 1; + *divider = div; + } + + return ret; +} + static void armada_drm_unref_work(struct work_struct *work) { struct armada_private *priv =
Hi Russell, Here is a new patch which should incorporate all your previous feedback. Now each variant passes clock info to the main driver via a new armada_clk_info structure. A helper function in the core lets each variant find the best clock. As you suggested we first try external ("dedicated") clocks, where we can change the rate to find an exact match. We fall back on looking at the rates of the other clocks and we return the clock that provides us with the closest match after integer division (rejecting those that don't bring us within 1%). There is still the possibility that two CRTCs on the same device end up using the same clock, so I added a usage counter to each clock to prevent the rate from being changed by another CRTC. This is compile-tested only - but after your feedback I will add the remaining required hacks and test it on XO.