All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Mahapatra, Chandrabhanu" <cmahapatra@ti.com>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Archit Taneja <archit@ti.com>,
	linux-omap@vger.kernel.org, linux-fbdev@vger.kernel.org
Subject: Re: [PATCH V3 3/3] OMAPDSS: DISPC: Correct DISPC functional clock usage
Date: Fri, 20 Apr 2012 06:42:53 +0000	[thread overview]
Message-ID: <CAF0AtAtan9s-Dgp-afgvfBRXHjzLgqC6fE=+WnLeydV9n-Kw=Q@mail.gmail.com> (raw)
In-Reply-To: <1334841071.1911.85.camel@deskari>

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.

WARNING: multiple messages have this Message-ID
From: "Mahapatra, Chandrabhanu" <cmahapatra@ti.com>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Archit Taneja <archit@ti.com>,
	linux-omap@vger.kernel.org, linux-fbdev@vger.kernel.org
Subject: Re: [PATCH V3 3/3] OMAPDSS: DISPC: Correct DISPC functional clock usage
Date: Fri, 20 Apr 2012 12:00:53 +0530	[thread overview]
Message-ID: <CAF0AtAtan9s-Dgp-afgvfBRXHjzLgqC6fE=+WnLeydV9n-Kw=Q@mail.gmail.com> (raw)
In-Reply-To: <1334841071.1911.85.camel@deskari>

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  reply	other threads:[~2012-04-20  6:42 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-04-02 15:13 [PATCH V3 0/3] OMAPDSS: DISPC: Enable predecimation for DMA and VRFB Chandrabhanu Mahapatra
2012-04-02 15:25 ` Chandrabhanu Mahapatra
2012-04-02 15:13 ` [PATCH V3 1/3] OMAPDSS: DISPC: Enable predecimation Chandrabhanu Mahapatra
2012-04-02 15:25   ` Chandrabhanu Mahapatra
2012-04-02 15:13   ` [PATCH V3 2/3] OMAPDSS: DISPC: Handle synclost errors in OMAP3 Chandrabhanu Mahapatra
2012-04-02 15:25     ` Chandrabhanu Mahapatra
2012-04-02 15:13     ` [PATCH V3 3/3] OMAPDSS: DISPC: Correct DISPC functional clock usage Chandrabhanu Mahapatra
2012-04-02 15:25       ` Chandrabhanu Mahapatra
2012-04-19 13:11       ` Tomi Valkeinen
2012-04-19 13:11         ` Tomi Valkeinen
2012-04-20  6:30         ` Mahapatra, Chandrabhanu [this message]
2012-04-20  6:42           ` Mahapatra, Chandrabhanu
2012-04-20 13:31       ` Chandrabhanu Mahapatra
2012-04-20 13:43         ` Chandrabhanu Mahapatra
2012-04-20 14:29         ` Tomi Valkeinen
2012-04-20 14:29           ` Tomi Valkeinen
2012-04-23  4:35           ` Mahapatra, Chandrabhanu
2012-04-23  4:47             ` Mahapatra, Chandrabhanu
2012-04-23  6:19             ` Tomi Valkeinen
2012-04-23  6:19               ` Tomi Valkeinen
2012-04-23  6:46       ` Chandrabhanu Mahapatra
2012-04-23  6:58         ` Chandrabhanu Mahapatra
2012-04-23  7:50         ` Tomi Valkeinen
2012-04-23  7:50           ` Tomi Valkeinen
     [not found] <[PATCH V2 0/3] OMAPDSS: DISPC: Enable predecimation for DMA and VRFB>
2012-04-02 13:59 ` [PATCH V3 0/3] OMAPDSS: DISPC: Enable predecimation for DMA and VRFB Chandrabhanu Mahapatra
2012-04-02 14:11   ` Chandrabhanu Mahapatra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAF0AtAtan9s-Dgp-afgvfBRXHjzLgqC6fE=+WnLeydV9n-Kw=Q@mail.gmail.com' \
    --to=cmahapatra@ti.com \
    --cc=archit@ti.com \
    --cc=linux-fbdev@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=tomi.valkeinen@ti.com \
    --subject='Re: [PATCH V3 3/3] OMAPDSS: DISPC: Correct DISPC functional clock usage' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.