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 at 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 at lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-rockchip >> > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
