Re: [PATCH V3 3/3] OMAPDSS: DISPC: Correct DISPC functional clock usage

["Mahapatra, Chandrabhanu" <cmahapatra@ti.com>]

On Thu, Apr 19, 2012 at 6:41 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Mon, 2012-04-02 at 20:43 +0530, Chandrabhanu Mahapatra wrote:
>> DISPC_FCLK is incorrectly used as functional clock of DISPC in scaling
>> calculations. So, DISPC_CORE_CLK replaces as functional clock of DISPC.
>> DISPC_CORE_CLK is derived from DISPC_FCLK divided by an independent DISPC
>> divisor LCD.
>>
>> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
>> ---
>>  drivers/video/omap2/dss/dispc.c |   28 ++++++++++++++++++++++------
>>  drivers/video/omap2/dss/dss.h   |    1 +
>>  2 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
>> index 17ffa71..cfde674 100644
>> --- a/drivers/video/omap2/dss/dispc.c
>> +++ b/drivers/video/omap2/dss/dispc.c
>> @@ -1813,6 +1813,7 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
>>                               dss_feat_get_param_max(FEAT_PARAM_LINEWIDTH);
>>       const int max_decim_limit = 16;
>>       unsigned long fclk = 0;
>> +     unsigned long dispc_core_clk = dispc_core_clk_rate(channel);
>>       int decim_x, decim_y, error, min_factor;
>>       u16 in_width, in_height, in_width_max = 0;
>>
>> @@ -1855,7 +1856,7 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
>>                       fclk = calc_fclk(channel, in_width, in_height,
>>                                       out_width, out_height);
>>                       error = (in_width > maxsinglelinewidth || !fclk ||
>> -                             fclk > dispc_fclk_rate());
>> +                             fclk > dispc_core_clk);
>>                       if (error) {
>>                               if (decim_x = decim_y) {
>>                                       decim_x = min_factor;
>> @@ -1893,7 +1894,7 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
>>                                       out_width, out_height);
>>                       error = (error || in_width > maxsinglelinewidth * 2 ||
>>                               (in_width > maxsinglelinewidth && *five_taps) ||
>> -                             !fclk || fclk > dispc_fclk_rate());
>> +                             !fclk || fclk > dispc_core_clk);
>>                       if (error) {
>>                               if (decim_x = decim_y) {
>>                                       decim_x = min_factor;
>> @@ -1926,7 +1927,7 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
>>       } else {
>>               int decim_x_min = decim_x;
>>               in_height = DIV_ROUND_UP(height, decim_y);
>> -             in_width_max = dispc_fclk_rate() /
>> +             in_width_max = dispc_core_clk /
>>                               DIV_ROUND_UP(dispc_mgr_pclk_rate(channel),
>>                                               out_width);
>>               decim_x = DIV_ROUND_UP(width, in_width_max);
>> @@ -1950,13 +1951,13 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
>>       }
>>
>>       DSSDBG("required fclk rate = %lu Hz\n", fclk);
>> -     DSSDBG("current fclk rate = %lu Hz\n", dispc_fclk_rate());
>> +     DSSDBG("current fclk rate = %lu Hz\n", dispc_core_clk);
>>
>> -     if (!fclk || fclk > dispc_fclk_rate()) {
>> +     if (!fclk || fclk > dispc_core_clk) {
>>               DSSERR("failed to set up scaling, "
>>                       "required fclk rate = %lu Hz, "
>>                       "current fclk rate = %lu Hz\n",
>> -                     fclk, dispc_fclk_rate());
>> +                     fclk, dispc_core_clk);
>>               return -EINVAL;
>>       }
>>
>> @@ -2646,6 +2647,21 @@ unsigned long dispc_mgr_pclk_rate(enum omap_channel channel)
>>       }
>>  }
>>
>> +unsigned long dispc_core_clk_rate(enum omap_channel channel)
>> +{
>> +     int lcd = 1;
>> +     unsigned long r = dispc_fclk_rate();
>> +
>> +     if (dss_has_feature(FEAT_CORE_CLK_DIV)) {
>> +             lcd = REG_GET(DISPC_DIVISOR, 23, 16);
>> +     } else {
>> +             if (dispc_mgr_is_lcd(channel))
>> +                     lcd = REG_GET(DISPC_DIVISORo(channel), 23, 16);
>> +     }
>> +
>> +     return r / lcd ;
>> +}
>> +
>
> I wonder if this is correct. "channel" for dispc core clock doesn't make
> sense, there's no channel related to that. At least on OMAP4.
>
> If I'm not mistaken, in omap2/3 case (i.e.
> dss_has_feature(FEAT_CORE_CLK_DIV) = false) we can just use channel 0
> to get the lcd divisor. Although that would mean that LCD output's
> divisor affects the tv-out's scaling calculations, which feels a bit
> strange...
>

Yes, you are right. I had failed to look at channel 0 or
OMAP_DSS_CHANNEL_LCD can simply be used.

> So... I don't know... Do we really have two different dispc core clocks
> int omap2/3 (for lcd output and for tv output), but only one in omap4?
> If so, then the above code is ok. Have you found explanations from TRM
> which say what clock is required and where?
>

Though the term Dispc Core Clock is never used in OMAP2/3 TRM, it is
actually the logic clock that drives the various logics of DSS
subsystem. So we can say that we have one dispc core clock for OMAP2/3
and that being the logic clock. I took two different logics as LCD
factor returned zero for TV.  However, LCD factor of DISPC_DIVISOR of
channel 0 can be used always.

> In any case, please remove the initialization of lcd variable, and add:
>
> else
>        lcd = 1;
>
> I think that's much clearer. And "r" variable is commonly used as a
> return value. I would rename the variable to something else, say, "fck".
>
>  Tomi
>

Yes, sure it makes code more understandable and clean.


-- 
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.