Linus Walleij <linus.walleij@xxxxxxxxxx> writes:
> On Mon, Apr 24, 2017 at 9:45 PM, Eric Anholt <eric@xxxxxxxxxx> 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).
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
