linux-kernel - Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170920100851.GI1601@john.keeping.me.uk>
Date:   Wed, 20 Sep 2017 11:08:51 +0100
From:   John Keeping <john@...ping.me.uk>
To:     Sean Paul <seanpaul@...omium.org>
Cc:     Brian Norris <briannorris@...omium.org>, mark.rutland@....com,
        bivvy.bi@...k-chips.com, hl@...k-chips.com, heiko@...ech.de,
        airlied@...ux.ie, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org,
        linux-rockchip@...ts.infradead.org,
        Nickey Yang <nickey.yang@...k-chips.com>, robh+dt@...nel.org,
        zyw@...k-chips.com, xbl@...k-chips.com, mark.yao@...k-chips.com
Subject: Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting

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@...k-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);
> > 
> > [...]