Re: [PATCH v10 3/5] drm/msm/dp: add support for DP PLL driver

[Tanmay Shah]

On 2020-08-15 04:45, Dmitry Baryshkov wrote:
> On 15/08/2020 02:22, Tanmay Shah wrote:
> > On 2020-08-14 10:05, Dmitry Baryshkov wrote:
> > > On 12/08/2020 07:42, Tanmay Shah wrote:
> > > > From: Chandan Uddaraju <chandanu@xxxxxxxxxxxxxx>
> > > >
> > > > Add the needed DP PLL specific files to support
> > > > display port interface on msm targets.
> > >
> > > [skipped]
> > >
> > > > diff --git a/drivers/gpu/drm/msm/dp/dp_pll_private.h 
> > > > b/drivers/gpu/drm/msm/dp/dp_pll_private.h
> > > > new file mode 100644
> > > > index 000000000000..475ba6ed59ab
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/msm/dp/dp_pll_private.h
> > > > @@ -0,0 +1,98 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +/*
> > > > + * Copyright (c) 2016-2020, The Linux Foundation. All rights 
> > > > reserved.
> > > > + */
> > > > +
> > > > +#ifndef __DP_PLL_10NM_H
> > > > +#define __DP_PLL_10NM_H
> > > > +
> > > > +#include "dp_pll.h"
> > > > +#include "dp_reg.h"
> > > > +
> > > > +#define DP_VCO_HSCLK_RATE_1620MHZDIV1000    1620000UL
> > > > +#define DP_VCO_HSCLK_RATE_2700MHZDIV1000    2700000UL
> > > > +#define DP_VCO_HSCLK_RATE_5400MHZDIV1000    5400000UL
> > > > +#define DP_VCO_HSCLK_RATE_8100MHZDIV1000    8100000UL
> > > > +
> > > > +#define NUM_DP_CLOCKS_MAX            6
> > > > +
> > > > +#define DP_PHY_PLL_POLL_SLEEP_US        500
> > > > +#define DP_PHY_PLL_POLL_TIMEOUT_US        10000
> > > > +
> > > > +#define DP_VCO_RATE_8100MHZDIV1000        8100000UL
> > > > +#define DP_VCO_RATE_9720MHZDIV1000        9720000UL
> > > > +#define DP_VCO_RATE_10800MHZDIV1000        10800000UL
> > > > +
> > > > +struct dp_pll_vco_clk {
> > > > +    struct clk_hw hw;
> > > > +    unsigned long    rate;        /* current vco rate */
> > > > +    u64        min_rate;    /* min vco rate */
> > > > +    u64        max_rate;    /* max vco rate */
> > > > +    void        *priv;
> > > > +};
> > > > +
> > > > +struct dp_pll_db {
> > >
> > > This struct should probably go into dp_pll_10nm.c. dp_pll_7nm.c, for
> > > example, will use slightly different structure.
> >
> > Sure, it sounds good. I will give it try. Thanks!
> >
> > > > +    struct msm_dp_pll *base;
> > > > +
> > > > +    int id;
> > > > +    struct platform_device *pdev;
> > > > +
> > > > +    /* private clocks: */
> > > > +    bool fixed_factor_clk[NUM_DP_CLOCKS_MAX];
> > > > +    struct clk_hw *hws[NUM_DP_CLOCKS_MAX];
> > >
> > > Then these two fields can use exact number of clocks rather than
> > > NUM_DP_CLOCKS_MAX.
> >
> > I didn't get this. I think NUM_DP_CLOCKS_MAX is doing same?
>
> Not exactly. We'd need fixed_factor_clk[4] for 10nm rather than 6.
> It's not that important, just a small nitpick.
>
>
> > > > +    u32 num_hws;
> > > > +
> > > > +    /* lane and orientation settings */
> > > > +    u8 lane_cnt;
> > > > +    u8 orientation;
> > > > +
> > > > +    /* COM PHY settings */
> > > > +    u32 hsclk_sel;
> > > > +    u32 dec_start_mode0;
> > > > +    u32 div_frac_start1_mode0;
> > > > +    u32 div_frac_start2_mode0;
> > > > +    u32 div_frac_start3_mode0;
> > > > +    u32 integloop_gain0_mode0;
> > > > +    u32 integloop_gain1_mode0;
> > > > +    u32 vco_tune_map;
> > > > +    u32 lock_cmp1_mode0;
> > > > +    u32 lock_cmp2_mode0;
> > > > +    u32 lock_cmp3_mode0;
> > > > +    u32 lock_cmp_en;
> > > > +
> > > > +    /* PHY vco divider */
> > > > +    u32 phy_vco_div;
> > > > +    /*
> > > > +     * Certain pll's needs to update the same vco rate after resume 
> > > > in
> > > > +     * suspend/resume scenario. Cached the vco rate for such plls.
> > > > +     */
> > > > +    unsigned long    vco_cached_rate;
> > > > +    u32        cached_cfg0;
> > > > +    u32        cached_cfg1;
> > > > +    u32        cached_outdiv;
> > > > +
> > > > +    uint32_t index;
> > > > +};
> > > > +
> > > > +static inline struct dp_pll_vco_clk *to_dp_vco_hw(struct clk_hw 
> > > > *hw)
> > > > +{
> > > > +    return container_of(hw, struct dp_pll_vco_clk, hw);
> > > > +}
> > > > +
> > > > +#define to_msm_dp_pll(vco) ((struct msm_dp_pll *)vco->priv)
> > > > +
> > > > +#define to_dp_pll_db(x)    ((struct dp_pll_db *)x->priv)
> > > > +
> > > > +int dp_vco_set_rate_10nm(struct clk_hw *hw, unsigned long rate,
> > > > +                unsigned long parent_rate);
> > > > +unsigned long dp_vco_recalc_rate_10nm(struct clk_hw *hw,
> > > > +                unsigned long parent_rate);
> > > > +long dp_vco_round_rate_10nm(struct clk_hw *hw, unsigned long rate,
> > > > +                unsigned long *parent_rate);
> > > > +int dp_vco_prepare_10nm(struct clk_hw *hw);
> > > > +void dp_vco_unprepare_10nm(struct clk_hw *hw);
> > > > +
> > > > +int msm_dp_pll_10nm_init(struct msm_dp_pll *dp_pll, int id);
> > > > +void msm_dp_pll_10nm_deinit(struct msm_dp_pll *dp_pll);
> > >
> > > These functions don't seem to be used outside of dp_pll_10nm. What
> > > about making them static?
> >
> > I can't declare static to "init" and "deinit" as they are exported to 
> > dp_pll.c.
> > Rest of them I can move to dp_pll_10nm and then define static.
>
> Please exuse me for not being exact enough. Of course I meant
> dp_vco_FOO_10nm() functions, not init/exit.
Ok got it. Sorry I didn't mean to nitpick here.

> BTW: as I'm looking onto 7nm dp pll, which naming would you prefer?
I didn't get this. Did you mean naming convention of functions and 
structure between
7nm and 10 nm? Could you point me where 7nm dp pll code is posted?

> We can have separate DP_PLL_10nm/DP_PLL_7nm namespaces (as DSI PLLs
> do) or I can override only a set of constants (like downstream driver
> does).

Having separate namespace sounds good.