[Freedreno] [DPU PATCH 3/3] drm/msm/dp: add support for DP PLL driver

[Stephen Boyd]

Quoting Sean Paul (2018-11-01 14:03:15)
> On Wed, Oct 10, 2018 at 10:15:59AM -0700, Chandan Uddaraju wrote:
> 
> > +}
> > +
> >  static int dp_power_set_pixel_clk_parent(struct dp_power *dp_power)
> >  {
> >       int rc = 0;
> > @@ -472,8 +520,12 @@ static int dp_power_set_pixel_clk_parent(struct dp_power *dp_power)
> >  
> >       power = container_of(dp_power, struct dp_power_private, dp_power);
> >  
> > -     if (power->pixel_clk_rcg && power->pixel_parent)
> > -             clk_set_parent(power->pixel_clk_rcg, power->pixel_parent);
> > +     if (power->pixel_clk_rcg && power->pixel_provider) {
> > +             clk_set_parent(power->pixel_clk_rcg, power->pixel_provider);
> 
> s/pixel_parent/pixel_provider/ isn't related to this change, can you please use
> the final name in the main dp patch so it's correct in this one?
> 
> > +             pr_debug("%s: is the parent of clk=%s\n",
> > +                                     __clk_get_name(power->pixel_provider),
> > +                                     __clk_get_name(power->pixel_clk_rcg));
> 
> Same comment here, this debug can go in the main patch.

Is there any reason why assigned-clock-parents wouldn't work here?

> > diff --git a/drivers/gpu/drm/msm/dp/dp_power.h b/drivers/gpu/drm/msm/dp/dp_power.h
> > index d1adaaf..5effd32 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_power.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_power.h
> > @@ -24,6 +24,7 @@
> >   * @init: initializes the regulators/core clocks/GPIOs/pinctrl
> >   * @deinit: turns off the regulators/core clocks/GPIOs/pinctrl
> >   * @clk_enable: enable/disable the DP clocks
> > + * @set_link_clk_parent: set the parent of DP link clock
> >   * @set_pixel_clk_parent: set the parent of DP pixel clock
> >   */
> >  struct dp_power {
[...]
> > +
> > +static int clk_mux_determine_rate(struct clk_hw *hw,
> > +                                  struct clk_rate_request *req)
> > +{
> > +     int ret = 0;
> 
> no need to init this to 0
> 
> > +
> > +     ret = __clk_mux_determine_rate_closest(hw, req);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Set the new parent of mux if there is a new valid parent */
> > +     if (hw->clk && req->best_parent_hw->clk)
> > +             clk_set_parent(hw->clk, req->best_parent_hw->clk);
> 
> What about the return value? All other clk_set_parent calls in this patch are
> also unchecked, so please add those as well.

The determine_rate clk_op should _never_ change clk hardware. It's a
function that's about determining what rate a clk can support based on
the rate that's requested and what the hardware can do.

Ok, let me just take a pass at the overall clk parts of this patch
instead of doing an "and also" review on Sean's review.