Message ID | 20210314151342.23404-1-dariobin@libero.it (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/tilcdc: fix LCD pixel clock setting | expand |
On 14/03/2021 17:13, Dario Binacchi wrote: > As reported by TI spruh73x RM, the LCD pixel clock (LCD_PCLK) frequency > is obtained by dividing LCD_CLK, the LCD controller reference clock, > for CLKDIV: > > LCD_PCLK = LCD_CLK / CLKDIV > > where CLKDIV must be greater than 1. > > Therefore LCD_CLK must be set to 'req_rate * CLKDIV' instead of req_rate The above doesn't make sense, the code already sets LCD_CLK to 'req_rate * clkdiv', not req_rate. > and the real LCD_CLK rate must be compared with 'req_rate * CLKDIV' and > not with req_rate. This is true, the code looks at the wrong value. > Passing req_rate instead of 'req_rate * CLKDIV' to the tilcdc_pclk_diff > routine caused it to fail even if LCD_CLK was properly set. > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > > --- > > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > index 30213708fc99..02f56c9a5da5 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > @@ -203,7 +203,7 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc) > struct drm_device *dev = crtc->dev; > struct tilcdc_drm_private *priv = dev->dev_private; > struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); > - unsigned long clk_rate, real_rate, req_rate; > + unsigned long clk_rate, real_rate, req_rate, clk_div_rate; > unsigned int clkdiv; > int ret; > > @@ -211,10 +211,11 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc) > > /* mode.clock is in KHz, set_rate wants parameter in Hz */ > req_rate = crtc->mode.clock * 1000; > - > - ret = clk_set_rate(priv->clk, req_rate * clkdiv); > + /* LCD clock divisor input rate */ > + clk_div_rate = req_rate * clkdiv; "clk_div_rate" sounds a bit odd to me. Why not lcd_fck_rate, as that's the name used later? Or lcd_clk_rate. Or maybe lcd_clk_req_rate... > + ret = clk_set_rate(priv->clk, clk_div_rate); > clk_rate = clk_get_rate(priv->clk); > - if (ret < 0 || tilcdc_pclk_diff(req_rate, clk_rate) > 5) { > + if (ret < 0 || tilcdc_pclk_diff(clk_div_rate, clk_rate) > 5) { > /* > * If we fail to set the clock rate (some architectures don't > * use the common clock framework yet and may not implement > I think this fix is fine, but looking at the current code, it's calling tilcdc_pclk_diff(), but doesn't actually provide pixel clocks to the function, but fclk. Tomi
> Il 17/03/2021 09:19 Tomi Valkeinen <tomba@kernel.org> ha scritto: > > > On 14/03/2021 17:13, Dario Binacchi wrote: > > As reported by TI spruh73x RM, the LCD pixel clock (LCD_PCLK) frequency > > is obtained by dividing LCD_CLK, the LCD controller reference clock, > > for CLKDIV: > > > > LCD_PCLK = LCD_CLK / CLKDIV > > > > where CLKDIV must be greater than 1. > > > > Therefore LCD_CLK must be set to 'req_rate * CLKDIV' instead of req_rate > > The above doesn't make sense, the code already sets LCD_CLK to 'req_rate > * clkdiv', not req_rate. > > > and the real LCD_CLK rate must be compared with 'req_rate * CLKDIV' and > > not with req_rate. > > This is true, the code looks at the wrong value. > > > Passing req_rate instead of 'req_rate * CLKDIV' to the tilcdc_pclk_diff > > routine caused it to fail even if LCD_CLK was properly set. > > > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > > > > --- > > > > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > > index 30213708fc99..02f56c9a5da5 100644 > > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > > @@ -203,7 +203,7 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc) > > struct drm_device *dev = crtc->dev; > > struct tilcdc_drm_private *priv = dev->dev_private; > > struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); > > - unsigned long clk_rate, real_rate, req_rate; > > + unsigned long clk_rate, real_rate, req_rate, clk_div_rate; > > unsigned int clkdiv; > > int ret; > > > > @@ -211,10 +211,11 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc) > > > > /* mode.clock is in KHz, set_rate wants parameter in Hz */ > > req_rate = crtc->mode.clock * 1000; > > - > > - ret = clk_set_rate(priv->clk, req_rate * clkdiv); > > + /* LCD clock divisor input rate */ > > + clk_div_rate = req_rate * clkdiv; > > "clk_div_rate" sounds a bit odd to me. Why not lcd_fck_rate, as that's > the name used later? Or lcd_clk_rate. Or maybe lcd_clk_req_rate... I prefer lcd_clk_rate. How about adding an additional patch that changes the variable names to make the code more readable? req_rate -> lcd_pclk_rate clk_rate -> real_lcd_clk_rate And add a comment to the function which highlights the relationship LCD_CLK = LCD_PCLK * CLDIV ? > > > + ret = clk_set_rate(priv->clk, clk_div_rate); > > clk_rate = clk_get_rate(priv->clk); > > - if (ret < 0 || tilcdc_pclk_diff(req_rate, clk_rate) > 5) { > > + if (ret < 0 || tilcdc_pclk_diff(clk_div_rate, clk_rate) > 5) { > > /* > > * If we fail to set the clock rate (some architectures don't > > * use the common clock framework yet and may not implement > > > > I think this fix is fine, but looking at the current code, it's calling > tilcdc_pclk_diff(), but doesn't actually provide pixel clocks to the > function, but fclk. Yes, I agree. Thanks and regards, Dario > > Tomi
On 2021-03-18 23:47, Dario Binacchi wrote: >> Il 17/03/2021 09:19 Tomi Valkeinen <tomba@kernel.org> ha scritto: >> >> >> On 14/03/2021 17:13, Dario Binacchi wrote: >> > As reported by TI spruh73x RM, the LCD pixel clock (LCD_PCLK) frequency >> > is obtained by dividing LCD_CLK, the LCD controller reference clock, >> > for CLKDIV: >> > >> > LCD_PCLK = LCD_CLK / CLKDIV >> > >> > where CLKDIV must be greater than 1. >> > >> > Therefore LCD_CLK must be set to 'req_rate * CLKDIV' instead of req_rate >> >> The above doesn't make sense, the code already sets LCD_CLK to >> 'req_rate >> * clkdiv', not req_rate. >> >> > and the real LCD_CLK rate must be compared with 'req_rate * CLKDIV' and >> > not with req_rate. >> >> This is true, the code looks at the wrong value. >> >> > Passing req_rate instead of 'req_rate * CLKDIV' to the tilcdc_pclk_diff >> > routine caused it to fail even if LCD_CLK was properly set. >> > >> > Signed-off-by: Dario Binacchi <dariobin@libero.it> >> > >> > --- >> > >> > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 9 +++++---- >> > 1 file changed, 5 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c >> > index 30213708fc99..02f56c9a5da5 100644 >> > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c >> > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c >> > @@ -203,7 +203,7 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc) >> > struct drm_device *dev = crtc->dev; >> > struct tilcdc_drm_private *priv = dev->dev_private; >> > struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); >> > - unsigned long clk_rate, real_rate, req_rate; >> > + unsigned long clk_rate, real_rate, req_rate, clk_div_rate; >> > unsigned int clkdiv; >> > int ret; >> > >> > @@ -211,10 +211,11 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc) >> > >> > /* mode.clock is in KHz, set_rate wants parameter in Hz */ >> > req_rate = crtc->mode.clock * 1000; >> > - >> > - ret = clk_set_rate(priv->clk, req_rate * clkdiv); >> > + /* LCD clock divisor input rate */ >> > + clk_div_rate = req_rate * clkdiv; >> >> "clk_div_rate" sounds a bit odd to me. Why not lcd_fck_rate, as that's >> the name used later? Or lcd_clk_rate. Or maybe lcd_clk_req_rate... > > I prefer lcd_clk_rate. > > How about adding an additional patch that changes the variable names to > make > the code more readable? > > req_rate -> lcd_pclk_rate > clk_rate -> real_lcd_clk_rate > > And add a comment to the function which highlights the relationship > LCD_CLK = LCD_PCLK * CLDIV ? > What about renaming current req_rate to pclk_rate (for pixel clock rate), and calling pclk_rate * clkdiv = req_rate, as that is the rate we need to request from the input clock? Adding lcd to local variable names here is quite redundant after all. In any case req_rate is bit misleading name here and probably part of the reason why the bug exists in the first place. Best regards, Jyri >> >> > + ret = clk_set_rate(priv->clk, clk_div_rate); >> > clk_rate = clk_get_rate(priv->clk); >> > - if (ret < 0 || tilcdc_pclk_diff(req_rate, clk_rate) > 5) { >> > + if (ret < 0 || tilcdc_pclk_diff(clk_div_rate, clk_rate) > 5) { >> > /* >> > * If we fail to set the clock rate (some architectures don't >> > * use the common clock framework yet and may not implement >> > >> >> I think this fix is fine, but looking at the current code, it's >> calling >> tilcdc_pclk_diff(), but doesn't actually provide pixel clocks to the >> function, but fclk. > > Yes, I agree. > > Thanks and regards, > Dario > >> >> Tomi
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 30213708fc99..02f56c9a5da5 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -203,7 +203,7 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc) struct drm_device *dev = crtc->dev; struct tilcdc_drm_private *priv = dev->dev_private; struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); - unsigned long clk_rate, real_rate, req_rate; + unsigned long clk_rate, real_rate, req_rate, clk_div_rate; unsigned int clkdiv; int ret; @@ -211,10 +211,11 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc) /* mode.clock is in KHz, set_rate wants parameter in Hz */ req_rate = crtc->mode.clock * 1000; - - ret = clk_set_rate(priv->clk, req_rate * clkdiv); + /* LCD clock divisor input rate */ + clk_div_rate = req_rate * clkdiv; + ret = clk_set_rate(priv->clk, clk_div_rate); clk_rate = clk_get_rate(priv->clk); - if (ret < 0 || tilcdc_pclk_diff(req_rate, clk_rate) > 5) { + if (ret < 0 || tilcdc_pclk_diff(clk_div_rate, clk_rate) > 5) { /* * If we fail to set the clock rate (some architectures don't * use the common clock framework yet and may not implement
As reported by TI spruh73x RM, the LCD pixel clock (LCD_PCLK) frequency is obtained by dividing LCD_CLK, the LCD controller reference clock, for CLKDIV: LCD_PCLK = LCD_CLK / CLKDIV where CLKDIV must be greater than 1. Therefore LCD_CLK must be set to 'req_rate * CLKDIV' instead of req_rate and the real LCD_CLK rate must be compared with 'req_rate * CLKDIV' and not with req_rate. Passing req_rate instead of 'req_rate * CLKDIV' to the tilcdc_pclk_diff routine caused it to fail even if LCD_CLK was properly set. Signed-off-by: Dario Binacchi <dariobin@libero.it> --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)