| Message ID | 1505725539-6309-1-git-send-email-nickey.yang@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Nickey, On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote: > This patch correct Feedback divider setting: > 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN > 2、Due to the use of a "by 2 pre-scaler," the range of the > feedback multiplication Feedback divider is limited to even > division numbers, and Feedback divider must be greater than > 12, less than 1000. > 3、Make the previously configured Feedback divider(LSB) > factors effective > > Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com> > --- > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ > 1 file changed, 54 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > index 9a20b9d..52698b7 100644 > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > @@ -228,7 +228,7 @@ > #define LOW_PROGRAM_EN 0 > #define HIGH_PROGRAM_EN BIT(7) > #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) > -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) > +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) > #define PLL_LOOP_DIV_EN BIT(5) > #define PLL_INPUT_DIV_EN BIT(4) > > @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) > dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); > dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | > LOW_PROGRAM_EN); > + dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); > dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | > HIGH_PROGRAM_EN); > dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); > @@ -521,11 +522,16 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) > static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, > struct drm_display_mode *mode) > { > - unsigned int i, pre; > - unsigned long mpclk, pllref, tmp; > - unsigned int m = 1, n = 1, target_mbps = 1000; > + unsigned long mpclk, tmp; > + unsigned int target_mbps = 1000; > unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps; > int bpp; > + unsigned long best_freq = 0; > + unsigned long fvco_min, fvco_max, fin ,fout; > + unsigned int min_prediv, max_prediv; > + unsigned int _prediv, uninitialized_var(best_prediv); > + unsigned long _fbdiv, uninitialized_var(best_fbdiv); > + unsigned long min_delta = UINT_MAX; > > bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > if (bpp < 0) { > @@ -544,34 +550,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, > dev_err(dsi->dev, "DPHY clock frequency is out of range\n"); > } > > - pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC); > - tmp = pllref; > - > - /* > - * The limits on the PLL divisor are: > - * > - * 5MHz <= (pllref / n) <= 40MHz > - * > - * we walk over these values in descreasing order so that if we hit > - * an exact match for target_mbps it is more likely that "m" will be > - * even. > - * > - * TODO: ensure that "m" is even after this loop. > - */ > - for (i = pllref / 5; i > (pllref / 40); i--) { > - pre = pllref / i; > - if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) { > - tmp = target_mbps % pre; > - n = i; > - m = target_mbps / pre; > + fin = clk_get_rate(dsi->pllref_clk); > + fout = target_mbps * USEC_PER_SEC; > + > + /* constraint: 5Mhz < Fref / N < 40MHz */ > + min_prediv = DIV_ROUND_UP(fin, 40 * USEC_PER_SEC); > + max_prediv = fin / (5 * USEC_PER_SEC); > + > + /* constraint: 80MHz < Fvco < 1500Mhz */ > + fvco_min = 80 * USEC_PER_SEC; > + fvco_max = 1500 * USEC_PER_SEC; > + > + for (_prediv = min_prediv; _prediv <= max_prediv; _prediv++) { > + u32 delta; > + /* Fvco = Fref * M / N */ > + tmp = fout * _prediv; > + do_div(tmp, fin); > + _fbdiv = tmp; > + /* > + * Due to the use of a "by 2 pre-scaler," the range of the > + * feedback multiplication value M is limited to even division > + * numbers, and m must be greater than 12, less than 1000. > + */ > + if (_fbdiv < 12 || _fbdiv > 1000) > + continue; > + > + if (_fbdiv % 2) > + ++_fbdiv; > + > + tmp = (u64)_fbdiv * fin; Are you relying on this being able to handle >32-bit integers? They you should declare 'tmp' as a 64-bit type; 'unsigned long' isn't guaranteed on 32-bit architectures. Try 'u64'? Brian > + do_div(tmp, _prediv); > + if (tmp < fvco_min || tmp > fvco_max) > + continue; > + > + delta = abs(fout - tmp); > + if (delta < min_delta) { > + best_prediv = _prediv; > + best_fbdiv = _fbdiv; > + min_delta = delta; > + best_freq = tmp; > } > - if (tmp == 0) > - break; > } > > - dsi->lane_mbps = pllref / n * m; > - dsi->input_div = n; > - dsi->feedback_div = m; > + if (best_freq) { > + dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC); > + dsi->input_div = best_prediv; > + dsi->feedback_div = best_fbdiv; > + } > > return 0; > } > -- > 1.9.1 > >
On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote: > This patch correct Feedback divider setting: > 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN > 2、Due to the use of a "by 2 pre-scaler," the range of the > feedback multiplication Feedback divider is limited to even > division numbers, and Feedback divider must be greater than > 12, less than 1000. > 3、Make the previously configured Feedback divider(LSB) > factors effective > > Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com> > --- > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ > 1 file changed, 54 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > index 9a20b9d..52698b7 100644 > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > @@ -228,7 +228,7 @@ > #define LOW_PROGRAM_EN 0 > #define HIGH_PROGRAM_EN BIT(7) > #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) > -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) > +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) > #define PLL_LOOP_DIV_EN BIT(5) > #define PLL_INPUT_DIV_EN BIT(4) > > @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) > dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); > dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | > LOW_PROGRAM_EN); > + dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); You do the same write 2 lines down. Are both needed? It would be nice if the register names were also defined, so this is easier to read. > dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | > HIGH_PROGRAM_EN); > dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); > @@ -521,11 +522,16 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) > static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, > struct drm_display_mode *mode) > { > - unsigned int i, pre; > - unsigned long mpclk, pllref, tmp; > - unsigned int m = 1, n = 1, target_mbps = 1000; > + unsigned long mpclk, tmp; > + unsigned int target_mbps = 1000; > unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps; > int bpp; > + unsigned long best_freq = 0; > + unsigned long fvco_min, fvco_max, fin ,fout; > + unsigned int min_prediv, max_prediv; > + unsigned int _prediv, uninitialized_var(best_prediv); > + unsigned long _fbdiv, uninitialized_var(best_fbdiv); > + unsigned long min_delta = UINT_MAX; Once you explicitly type these, make sure you use the appropriate _MAX value (ie: U64_MAX) > > bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > if (bpp < 0) { > @@ -544,34 +550,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, > dev_err(dsi->dev, "DPHY clock frequency is out of range\n"); > } > > - pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC); > - tmp = pllref; > - > - /* > - * The limits on the PLL divisor are: > - * > - * 5MHz <= (pllref / n) <= 40MHz > - * > - * we walk over these values in descreasing order so that if we hit > - * an exact match for target_mbps it is more likely that "m" will be > - * even. > - * > - * TODO: ensure that "m" is even after this loop. > - */ > - for (i = pllref / 5; i > (pllref / 40); i--) { > - pre = pllref / i; > - if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) { > - tmp = target_mbps % pre; > - n = i; > - m = target_mbps / pre; > + fin = clk_get_rate(dsi->pllref_clk); > + fout = target_mbps * USEC_PER_SEC; > + > + /* constraint: 5Mhz < Fref / N < 40MHz */ > + min_prediv = DIV_ROUND_UP(fin, 40 * USEC_PER_SEC); > + max_prediv = fin / (5 * USEC_PER_SEC); > + > + /* constraint: 80MHz < Fvco < 1500Mhz */ > + fvco_min = 80 * USEC_PER_SEC; > + fvco_max = 1500 * USEC_PER_SEC; > + > + for (_prediv = min_prediv; _prediv <= max_prediv; _prediv++) { > + u32 delta; > + /* Fvco = Fref * M / N */ > + tmp = fout * _prediv; > + do_div(tmp, fin); > + _fbdiv = tmp; Why use tmp at all? Can't you just use _fbdiv directly in do_div? > + /* > + * Due to the use of a "by 2 pre-scaler," the range of the > + * feedback multiplication value M is limited to even division > + * numbers, and m must be greater than 12, less than 1000. > + */ > + if (_fbdiv < 12 || _fbdiv > 1000) > + continue; > + > + if (_fbdiv % 2) > + ++_fbdiv; You can reduce this down to: _fbdiv += _fbdiv % 2; > + > + tmp = (u64)_fbdiv * fin; I'll echo Brian's concerns on type mixing here. Please be explicit with size when you declare your variables. > + do_div(tmp, _prediv); > + if (tmp < fvco_min || tmp > fvco_max) > + continue; > + > + delta = abs(fout - tmp); > + if (delta < min_delta) { > + best_prediv = _prediv; > + best_fbdiv = _fbdiv; > + min_delta = delta; > + best_freq = tmp; > } > - if (tmp == 0) > - break; > } > > - dsi->lane_mbps = pllref / n * m; > - dsi->input_div = n; > - dsi->feedback_div = m; > + if (best_freq) { Should you return an error or log something if best_freq is not found? > + dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC); > + dsi->input_div = best_prediv; > + dsi->feedback_div = best_fbdiv; > + } > > return 0; > } > -- > 1.9.1 > >
Hi Sean, On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote: > On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote: > > This patch correct Feedback divider setting: > > 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN > > 2、Due to the use of a "by 2 pre-scaler," the range of the > > feedback multiplication Feedback divider is limited to even > > division numbers, and Feedback divider must be greater than > > 12, less than 1000. > > 3、Make the previously configured Feedback divider(LSB) > > factors effective > > > > Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com> > > --- > > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ > > 1 file changed, 54 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > index 9a20b9d..52698b7 100644 > > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > @@ -228,7 +228,7 @@ > > #define LOW_PROGRAM_EN 0 > > #define HIGH_PROGRAM_EN BIT(7) > > #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) > > -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) > > +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) > > #define PLL_LOOP_DIV_EN BIT(5) > > #define PLL_INPUT_DIV_EN BIT(4) > > > > @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) > > dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); > > dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | > > LOW_PROGRAM_EN); > > + dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); > > You do the same write 2 lines down. Are both needed? It would be nice if the > register names were also defined, so this is easier to read. If I'm reading correctly, I think this is what Nickey meant by: "3、Make the previously configured Feedback divider(LSB) factors effective" . My reading of the databook is that this step finalizes the previous two writes (to test code 0x17 and 0x18). Given this was buggy (?) previously, it does seem like having some extra language to document this could help. Register names (or "test codes", per the docs?) could help, but additionally, maybe a few more comments. > > dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | > > HIGH_PROGRAM_EN); > > dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); [...] Brian
On Tue, Sep 19, 2017 at 11:19:01AM -0700, Brian Norris wrote: > Hi Sean, > > On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote: > > On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote: > > > This patch correct Feedback divider setting: > > > 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN > > > 2、Due to the use of a "by 2 pre-scaler," the range of the > > > feedback multiplication Feedback divider is limited to even > > > division numbers, and Feedback divider must be greater than > > > 12, less than 1000. > > > 3、Make the previously configured Feedback divider(LSB) > > > factors effective > > > > > > Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com> > > > --- > > > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ > > > 1 file changed, 54 insertions(+), 29 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > index 9a20b9d..52698b7 100644 > > > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > @@ -228,7 +228,7 @@ > > > #define LOW_PROGRAM_EN 0 > > > #define HIGH_PROGRAM_EN BIT(7) > > > #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) > > > -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) > > > +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) > > > #define PLL_LOOP_DIV_EN BIT(5) > > > #define PLL_INPUT_DIV_EN BIT(4) > > > > > > @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) > > > dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); > > > dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | > > > LOW_PROGRAM_EN); > > > + dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); > > > > You do the same write 2 lines down. Are both needed? It would be nice if the > > register names were also defined, so this is easier to read. > > If I'm reading correctly, I think this is what Nickey meant by: > > "3、Make the previously configured Feedback divider(LSB) > factors effective" > > . My reading of the databook is that this step finalizes the previous > two writes (to test code 0x17 and 0x18). > > Given this was buggy (?) previously, it does seem like having some extra > language to document this could help. Register names (or "test codes", > per the docs?) could help, but additionally, maybe a few more comments. > Ah, yeah, thanks for the explanation. It's not clear that this latches the values above. I think register names and comments would be immensely helpful. Sean > > > dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | > > > HIGH_PROGRAM_EN); > > > dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); > > [...] > > Brian
On Tue, Sep 19, 2017 at 01:27:40PM -0700, Sean Paul wrote: > On Tue, Sep 19, 2017 at 11:19:01AM -0700, Brian Norris wrote: > > Hi Sean, > > > > On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote: > > > On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote: > > > > This patch correct Feedback divider setting: > > > > 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN > > > > 2、Due to the use of a "by 2 pre-scaler," the range of the > > > > feedback multiplication Feedback divider is limited to even > > > > division numbers, and Feedback divider must be greater than > > > > 12, less than 1000. > > > > 3、Make the previously configured Feedback divider(LSB) > > > > factors effective > > > > > > > > Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com> > > > > --- > > > > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ > > > > 1 file changed, 54 insertions(+), 29 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > index 9a20b9d..52698b7 100644 > > > > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > > > > @@ -228,7 +228,7 @@ > > > > #define LOW_PROGRAM_EN 0 > > > > #define HIGH_PROGRAM_EN BIT(7) > > > > #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) > > > > -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) > > > > +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) > > > > #define PLL_LOOP_DIV_EN BIT(5) > > > > #define PLL_INPUT_DIV_EN BIT(4) > > > > > > > > @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) > > > > dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); > > > > dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | > > > > LOW_PROGRAM_EN); > > > > + dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); > > > > > > You do the same write 2 lines down. Are both needed? It would be nice if the > > > register names were also defined, so this is easier to read. > > > > If I'm reading correctly, I think this is what Nickey meant by: > > > > "3、Make the previously configured Feedback divider(LSB) > > factors effective" > > > > . My reading of the databook is that this step finalizes the previous > > two writes (to test code 0x17 and 0x18). > > > > Given this was buggy (?) previously, it does seem like having some extra > > language to document this could help. Register names (or "test codes", > > per the docs?) could help, but additionally, maybe a few more comments. > > > > Ah, yeah, thanks for the explanation. It's not clear that this latches the > values above. I think register names and comments would be immensely helpful. According to the databook, 0x19 controls whether the loop/input dividers are derived from the hsfreqrange configuration or use the values in 0x17 and 0x18. I can't see why writing the same value to this register multiple times is necessary. > > > > dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | > > > > HIGH_PROGRAM_EN); > > > > dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); > > > > [...]
On Wednesday, September 20, 2017 06:08 PM, John Keeping wrote: > On Tue, Sep 19, 2017 at 01:27:40PM -0700, Sean Paul wrote: >> On Tue, Sep 19, 2017 at 11:19:01AM -0700, Brian Norris wrote: >>> Hi Sean, >>> >>> On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote: >>>> On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote: >>>>> This patch correct Feedback divider setting: >>>>> 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN >>>>> 2、Due to the use of a "by 2 pre-scaler," the range of the >>>>> feedback multiplication Feedback divider is limited to even >>>>> division numbers, and Feedback divider must be greater than >>>>> 12, less than 1000. >>>>> 3、Make the previously configured Feedback divider(LSB) >>>>> factors effective >>>>> >>>>> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com> >>>>> --- >>>>> drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ >>>>> 1 file changed, 54 insertions(+), 29 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c >>>>> index 9a20b9d..52698b7 100644 >>>>> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c >>>>> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c >>>>> @@ -228,7 +228,7 @@ >>>>> #define LOW_PROGRAM_EN 0 >>>>> #define HIGH_PROGRAM_EN BIT(7) >>>>> #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) >>>>> -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) >>>>> +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) >>>>> #define PLL_LOOP_DIV_EN BIT(5) >>>>> #define PLL_INPUT_DIV_EN BIT(4) >>>>> >>>>> @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) >>>>> dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); >>>>> dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | >>>>> LOW_PROGRAM_EN); >>>>> + dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); >>>> You do the same write 2 lines down. Are both needed? It would be nice if the >>>> register names were also defined, so this is easier to read. >>> If I'm reading correctly, I think this is what Nickey meant by: >>> >>> "3、Make the previously configured Feedback divider(LSB) >>> factors effective" >>> >>> . My reading of the databook is that this step finalizes the previous >>> two writes (to test code 0x17 and 0x18). >>> >>> Given this was buggy (?) previously, it does seem like having some extra >>> language to document this could help. Register names (or "test codes", >>> per the docs?) could help, but additionally, maybe a few more comments. >>> >> Ah, yeah, thanks for the explanation. It's not clear that this latches the >> values above. I think register names and comments would be immensely helpful. > According to the databook, 0x19 controls whether the loop/input dividers > are derived from the hsfreqrange configuration or use the values in 0x17 > and 0x18. I can't see why writing the same value to this register > multiple times is necessary. According to databook, set 0x19 to 0x30 make the previously configured N(0x17) and M(0x18) factors effective, 0x18 need to be setted by twice, since we need to set 0x18 LSB and MSB separately, As we test, after set 0x18 LSB, if we do not set 0x19 immediately to make the configrued effective, when we set 0x18 MSB, the 0x18 LSB value will gone, and we will get wrong pll frequency. Anyway, I think should add some comment here to clear that. > >>>>> dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | >>>>> HIGH_PROGRAM_EN); >>>>> dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); >>> [...] > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
On Wed, Sep 20, 2017 at 07:08:11PM +0800, hl wrote: > > > On Wednesday, September 20, 2017 06:08 PM, John Keeping wrote: > > On Tue, Sep 19, 2017 at 01:27:40PM -0700, Sean Paul wrote: > >> On Tue, Sep 19, 2017 at 11:19:01AM -0700, Brian Norris wrote: > >>> Hi Sean, > >>> > >>> On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote: > >>>> On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote: > >>>>> This patch correct Feedback divider setting: > >>>>> 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN > >>>>> 2、Due to the use of a "by 2 pre-scaler," the range of the > >>>>> feedback multiplication Feedback divider is limited to even > >>>>> division numbers, and Feedback divider must be greater than > >>>>> 12, less than 1000. > >>>>> 3、Make the previously configured Feedback divider(LSB) > >>>>> factors effective > >>>>> > >>>>> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com> > >>>>> --- > >>>>> drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ > >>>>> 1 file changed, 54 insertions(+), 29 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > >>>>> index 9a20b9d..52698b7 100644 > >>>>> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > >>>>> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > >>>>> @@ -228,7 +228,7 @@ > >>>>> #define LOW_PROGRAM_EN 0 > >>>>> #define HIGH_PROGRAM_EN BIT(7) > >>>>> #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) > >>>>> -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) > >>>>> +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) > >>>>> #define PLL_LOOP_DIV_EN BIT(5) > >>>>> #define PLL_INPUT_DIV_EN BIT(4) > >>>>> > >>>>> @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) > >>>>> dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); > >>>>> dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | > >>>>> LOW_PROGRAM_EN); > >>>>> + dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); > >>>> You do the same write 2 lines down. Are both needed? It would be nice if the > >>>> register names were also defined, so this is easier to read. > >>> If I'm reading correctly, I think this is what Nickey meant by: > >>> > >>> "3、Make the previously configured Feedback divider(LSB) > >>> factors effective" > >>> > >>> . My reading of the databook is that this step finalizes the previous > >>> two writes (to test code 0x17 and 0x18). > >>> > >>> Given this was buggy (?) previously, it does seem like having some extra > >>> language to document this could help. Register names (or "test codes", > >>> per the docs?) could help, but additionally, maybe a few more comments. > >>> > >> Ah, yeah, thanks for the explanation. It's not clear that this latches the > >> values above. I think register names and comments would be immensely helpful. > > According to the databook, 0x19 controls whether the loop/input dividers > > are derived from the hsfreqrange configuration or use the values in 0x17 > > and 0x18. I can't see why writing the same value to this register > > multiple times is necessary. > According to databook, set 0x19 to 0x30 make the previously configured > N(0x17) and M(0x18) > factors effective, 0x18 need to be setted by twice, since we need to set > 0x18 LSB and MSB separately, > As we test, after set 0x18 LSB, if we do not set 0x19 immediately to > make the configrued effective, > when we set 0x18 MSB, the 0x18 LSB value will gone, and we will get > wrong pll frequency. Anyway, > I think should add some comment here to clear that. That's surprising, the examples in sections 6.2.1 and 6.2.2 of the databook I have both show the high and low parts of 0x18 being written before 0x19 is set. When reading 0x18 are you setting bit 7 in TESTDIN correctly in order to select the correct bits of the feedback divider? > > > >>>>> dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | > >>>>> HIGH_PROGRAM_EN); > >>>>> dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); > >>> [...] > > _______________________________________________ > > Linux-rockchip mailing list > > Linux-rockchip@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-rockchip > >
On Wednesday, September 20, 2017 08:07 PM, John Keeping wrote: > On Wed, Sep 20, 2017 at 07:08:11PM +0800, hl wrote: >> >> On Wednesday, September 20, 2017 06:08 PM, John Keeping wrote: >>> On Tue, Sep 19, 2017 at 01:27:40PM -0700, Sean Paul wrote: >>>> On Tue, Sep 19, 2017 at 11:19:01AM -0700, Brian Norris wrote: >>>>> Hi Sean, >>>>> >>>>> On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote: >>>>>> On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote: >>>>>>> This patch correct Feedback divider setting: >>>>>>> 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN >>>>>>> 2、Due to the use of a "by 2 pre-scaler," the range of the >>>>>>> feedback multiplication Feedback divider is limited to even >>>>>>> division numbers, and Feedback divider must be greater than >>>>>>> 12, less than 1000. >>>>>>> 3、Make the previously configured Feedback divider(LSB) >>>>>>> factors effective >>>>>>> >>>>>>> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ >>>>>>> 1 file changed, 54 insertions(+), 29 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c >>>>>>> index 9a20b9d..52698b7 100644 >>>>>>> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c >>>>>>> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c >>>>>>> @@ -228,7 +228,7 @@ >>>>>>> #define LOW_PROGRAM_EN 0 >>>>>>> #define HIGH_PROGRAM_EN BIT(7) >>>>>>> #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) >>>>>>> -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) >>>>>>> +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) >>>>>>> #define PLL_LOOP_DIV_EN BIT(5) >>>>>>> #define PLL_INPUT_DIV_EN BIT(4) >>>>>>> >>>>>>> @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) >>>>>>> dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); >>>>>>> dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | >>>>>>> LOW_PROGRAM_EN); >>>>>>> + dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); >>>>>> You do the same write 2 lines down. Are both needed? It would be nice if the >>>>>> register names were also defined, so this is easier to read. >>>>> If I'm reading correctly, I think this is what Nickey meant by: >>>>> >>>>> "3、Make the previously configured Feedback divider(LSB) >>>>> factors effective" >>>>> >>>>> . My reading of the databook is that this step finalizes the previous >>>>> two writes (to test code 0x17 and 0x18). >>>>> >>>>> Given this was buggy (?) previously, it does seem like having some extra >>>>> language to document this could help. Register names (or "test codes", >>>>> per the docs?) could help, but additionally, maybe a few more comments. >>>>> >>>> Ah, yeah, thanks for the explanation. It's not clear that this latches the >>>> values above. I think register names and comments would be immensely helpful. >>> According to the databook, 0x19 controls whether the loop/input dividers >>> are derived from the hsfreqrange configuration or use the values in 0x17 >>> and 0x18. I can't see why writing the same value to this register >>> multiple times is necessary. >> According to databook, set 0x19 to 0x30 make the previously configured >> N(0x17) and M(0x18) >> factors effective, 0x18 need to be setted by twice, since we need to set >> 0x18 LSB and MSB separately, >> As we test, after set 0x18 LSB, if we do not set 0x19 immediately to >> make the configrued effective, >> when we set 0x18 MSB, the 0x18 LSB value will gone, and we will get >> wrong pll frequency. Anyway, >> I think should add some comment here to clear that. > That's surprising, the examples in sections 6.2.1 and 6.2.2 of the > databook I have both show the high and low parts of 0x18 being written > before 0x19 is set. > > When reading 0x18 are you setting bit 7 in TESTDIN correctly in order to > select the correct bits of the feedback divider? We scope the mipi lane clock, found it got wrong clock frequency use flow: set 0x17 set 0x18 LSB set 0x18 MSB set 0x19 = 0x30 and we can get right mipi lane clock use flow: set 0x17 set 0x18 LSB set 0x19 = 0x30 set 0x18 MSB set 0x19 = 0x30 > >>>>>>> dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | >>>>>>> HIGH_PROGRAM_EN); >>>>>>> dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); >>>>> [...] >>> _______________________________________________ >>> Linux-rockchip mailing list >>> Linux-rockchip@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-rockchip >> > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
Hi Nickey, El Mon, Sep 18, 2017 at 05:05:33PM +0800 Nickey Yang ha dit: > This patch correct Feedback divider setting: > 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN > 2、Due to the use of a "by 2 pre-scaler," the range of the > feedback multiplication Feedback divider is limited to even > division numbers, and Feedback divider must be greater than > 12, less than 1000. > 3、Make the previously configured Feedback divider(LSB) > factors effective > > Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com> > --- > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ > 1 file changed, 54 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > index 9a20b9d..52698b7 100644 > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c > @@ -228,7 +228,7 @@ > #define LOW_PROGRAM_EN 0 > #define HIGH_PROGRAM_EN BIT(7) > #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) > -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) > +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) > #define PLL_LOOP_DIV_EN BIT(5) > #define PLL_INPUT_DIV_EN BIT(4) > > @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) > dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); > dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | > LOW_PROGRAM_EN); > + dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); > dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | > HIGH_PROGRAM_EN); > dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); > @@ -521,11 +522,16 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) > static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, > struct drm_display_mode *mode) > { > - unsigned int i, pre; > - unsigned long mpclk, pllref, tmp; > - unsigned int m = 1, n = 1, target_mbps = 1000; > + unsigned long mpclk, tmp; > + unsigned int target_mbps = 1000; > unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps; > int bpp; > + unsigned long best_freq = 0; > + unsigned long fvco_min, fvco_max, fin ,fout; nit: should be 'fin, fout'. > + unsigned int min_prediv, max_prediv; > + unsigned int _prediv, uninitialized_var(best_prediv); > + unsigned long _fbdiv, uninitialized_var(best_fbdiv); > + unsigned long min_delta = UINT_MAX; > > bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > if (bpp < 0) { > @@ -544,34 +550,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, > dev_err(dsi->dev, "DPHY clock frequency is out of range\n"); > } > > - pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC); > - tmp = pllref; > - > - /* > - * The limits on the PLL divisor are: > - * > - * 5MHz <= (pllref / n) <= 40MHz > - * > - * we walk over these values in descreasing order so that if we hit > - * an exact match for target_mbps it is more likely that "m" will be > - * even. > - * > - * TODO: ensure that "m" is even after this loop. > - */ > - for (i = pllref / 5; i > (pllref / 40); i--) { > - pre = pllref / i; > - if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) { > - tmp = target_mbps % pre; > - n = i; > - m = target_mbps / pre; > + fin = clk_get_rate(dsi->pllref_clk); > + fout = target_mbps * USEC_PER_SEC; > + > + /* constraint: 5Mhz < Fref / N < 40MHz */ According to the previous comment above it should be '<=' instead of '<'. > + min_prediv = DIV_ROUND_UP(fin, 40 * USEC_PER_SEC); > + max_prediv = fin / (5 * USEC_PER_SEC); > + > + /* constraint: 80MHz < Fvco < 1500Mhz */ Same here. > + fvco_min = 80 * USEC_PER_SEC; > + fvco_max = 1500 * USEC_PER_SEC; > + > + for (_prediv = min_prediv; _prediv <= max_prediv; _prediv++) { > + u32 delta; > + /* Fvco = Fref * M / N */ > + tmp = fout * _prediv; > + do_div(tmp, fin); > + _fbdiv = tmp; > + /* > + * Due to the use of a "by 2 pre-scaler," the range of the > + * feedback multiplication value M is limited to even division > + * numbers, and m must be greater than 12, less than 1000. > + */ I didn't encounter the second part of the constraint in my version of the databook, judging from the code below it seems the comment should be "12 <= m <= 1000". > + if (_fbdiv < 12 || _fbdiv > 1000) > + continue; > + > + if (_fbdiv % 2) > + ++_fbdiv; > + > + tmp = (u64)_fbdiv * fin; > + do_div(tmp, _prediv); > + if (tmp < fvco_min || tmp > fvco_max) > + continue; > + > + delta = abs(fout - tmp); > + if (delta < min_delta) { > + best_prediv = _prediv; > + best_fbdiv = _fbdiv; > + min_delta = delta; > + best_freq = tmp; > } > - if (tmp == 0) > - break; > } > > - dsi->lane_mbps = pllref / n * m; > - dsi->input_div = n; > - dsi->feedback_div = m; > + if (best_freq) { > + dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC); > + dsi->input_div = best_prediv; > + dsi->feedback_div = best_fbdiv; > + } > > return 0; > }
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 9a20b9d..52698b7 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -228,7 +228,7 @@ #define LOW_PROGRAM_EN 0 #define HIGH_PROGRAM_EN BIT(7) #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) #define PLL_LOOP_DIV_EN BIT(5) #define PLL_INPUT_DIV_EN BIT(4) @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | LOW_PROGRAM_EN); + dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | HIGH_PROGRAM_EN); dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); @@ -521,11 +522,16 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, struct drm_display_mode *mode) { - unsigned int i, pre; - unsigned long mpclk, pllref, tmp; - unsigned int m = 1, n = 1, target_mbps = 1000; + unsigned long mpclk, tmp; + unsigned int target_mbps = 1000; unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps; int bpp; + unsigned long best_freq = 0; + unsigned long fvco_min, fvco_max, fin ,fout; + unsigned int min_prediv, max_prediv; + unsigned int _prediv, uninitialized_var(best_prediv); + unsigned long _fbdiv, uninitialized_var(best_fbdiv); + unsigned long min_delta = UINT_MAX; bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); if (bpp < 0) { @@ -544,34 +550,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, dev_err(dsi->dev, "DPHY clock frequency is out of range\n"); } - pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC); - tmp = pllref; - - /* - * The limits on the PLL divisor are: - * - * 5MHz <= (pllref / n) <= 40MHz - * - * we walk over these values in descreasing order so that if we hit - * an exact match for target_mbps it is more likely that "m" will be - * even. - * - * TODO: ensure that "m" is even after this loop. - */ - for (i = pllref / 5; i > (pllref / 40); i--) { - pre = pllref / i; - if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) { - tmp = target_mbps % pre; - n = i; - m = target_mbps / pre; + fin = clk_get_rate(dsi->pllref_clk); + fout = target_mbps * USEC_PER_SEC; + + /* constraint: 5Mhz < Fref / N < 40MHz */ + min_prediv = DIV_ROUND_UP(fin, 40 * USEC_PER_SEC); + max_prediv = fin / (5 * USEC_PER_SEC); + + /* constraint: 80MHz < Fvco < 1500Mhz */ + fvco_min = 80 * USEC_PER_SEC; + fvco_max = 1500 * USEC_PER_SEC; + + for (_prediv = min_prediv; _prediv <= max_prediv; _prediv++) { + u32 delta; + /* Fvco = Fref * M / N */ + tmp = fout * _prediv; + do_div(tmp, fin); + _fbdiv = tmp; + /* + * Due to the use of a "by 2 pre-scaler," the range of the + * feedback multiplication value M is limited to even division + * numbers, and m must be greater than 12, less than 1000. + */ + if (_fbdiv < 12 || _fbdiv > 1000) + continue; + + if (_fbdiv % 2) + ++_fbdiv; + + tmp = (u64)_fbdiv * fin; + do_div(tmp, _prediv); + if (tmp < fvco_min || tmp > fvco_max) + continue; + + delta = abs(fout - tmp); + if (delta < min_delta) { + best_prediv = _prediv; + best_fbdiv = _fbdiv; + min_delta = delta; + best_freq = tmp; } - if (tmp == 0) - break; } - dsi->lane_mbps = pllref / n * m; - dsi->input_div = n; - dsi->feedback_div = m; + if (best_freq) { + dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC); + dsi->input_div = best_prediv; + dsi->feedback_div = best_fbdiv; + } return 0; }
This patch correct Feedback divider setting: 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN 2、Due to the use of a "by 2 pre-scaler," the range of the feedback multiplication Feedback divider is limited to even division numbers, and Feedback divider must be greater than 12, less than 1000. 3、Make the previously configured Feedback divider(LSB) factors effective Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com> --- drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 29 deletions(-)