Linus Walleij linus.walleij@linaro.org writes:
On Mon, Apr 24, 2017 at 9:45 PM, Eric Anholt eric@anholt.net wrote:
+static long pl111_clk_div_round_rate(struct clk_hw *hw, unsigned long rate,
unsigned long *prate)+{
int div = pl111_clk_div_choose_div(hw, rate, prate, true);...which we seem to assume that we can, actually why do you pass this parameter set_parent at all? It is always true in this code.
Because the other caller just below passes false: When we're being asked to set_rate, the parent rate has been set by the core and we just need to choose our best divider given that.
+static int pl111_clk_div_set_rate(struct clk_hw *hw, unsigned long rate,
unsigned long prate)+{
struct pl111_drm_dev_private *priv =container_of(hw, struct pl111_drm_dev_private, clk_div);int div = pl111_clk_div_choose_div(hw, rate, &prate, false);u32 tim2;spin_lock(&priv->tim2_lock);tim2 = readl(priv->regs + CLCD_TIM2);tim2 &= ~(TIM2_BCD | TIM2_PCD_LO_MASK | TIM2_PCD_HI_MASK);if (div == 1) {tim2 |= TIM2_BCD;} else {div -= 2;tim2 |= div & TIM2_PCD_LO_MASK;tim2 |= (div >> TIM2_PCD_LO_BITS) << TIM2_PCD_HI_SHIFT;}writel(tim2, priv->regs + CLCD_TIM2);spin_unlock(&priv->tim2_lock);return 0;+}
So this will write the divisor, which is nice. But what if we also need to change the rate of the parent?
The clk core will have already done that.
+static int +pl111_init_clock_divider(struct drm_device *drm) +{
struct pl111_drm_dev_private *priv = drm->dev_private;struct clk *parent = devm_clk_get(drm->dev, "clcdclk");struct clk_hw *div = &priv->clk_div;const char *parent_name;struct clk_init_data init = {.name = "pl111_div",.ops = &pl111_clk_div_ops,.parent_names = &parent_name,.num_parents = 1,};I think it is necessary to set .flags in the init data to .flags = CLK_SET_RATE_PARENT, for this code to work with a parent that can change rate.
I was thinking this flag was used internally in things like clk-divider.c, but the core uses it too. I'll fix that, thanks!
- Use the internal clock divisor to reduce power consumption by
- using HCLK (apb_pclk) when appropriate.
- Use the CLKSEL bit to support switching between the two external
- clock parents.
OK so that remains to be done. We discussed this previously so I got a bit confused. The divisor code seems fine, after this we only need some more code to choose the best parent for the divided clock.
It seems that what would pe Perfect(TM) would be to calculate the best end result using clock A and the best end result using clock B, both utilizing the divisor, and then choose the best of those two.
I think struct clk_mux is supposed to do that so it would eventually be:
CLK A -|_ clk_mux --> clk_divider --> pixel clock CLK B -|/
I agree. This patch got things going for this platform, without needing the bindings change (and helped confirm for me that I do understand the platform design correctly).