+ /* in raster mode, minimum divisor is 2: */
+ ret = clk_set_rate(priv->disp_clk, crtc->mode.clock * 1000 * 2);
These things can better be handled with divider clock having a
minimum value (being discussed with Mike on how exactly it should
be) and instead of setting rate over a platform specific clock,
you can set rate over lcd clock using SET_RATE_PARENT at platform
level, more below,
I looked at that patch you were proposing for da8xx-fb.. to be
honest, it did not seem simpler to me, so I was sort of failing to see
the benefit..
Mike was suggesting to model the above using gate/divider/composite
clocks of CCF in the FB driver.
quoted
+ priv->clk = clk_get(dev->dev, "fck");
+ if (IS_ERR(priv->clk)) {
+ dev_err(dev->dev, "failed to get functional clock\n");
+ ret = -ENODEV;
+ goto fail;
+ }
+
+ priv->disp_clk = clk_get(dev->dev, "dpll_disp_ck");
+ if (IS_ERR(priv->clk)) {
+ dev_err(dev->dev, "failed to get display clock\n");
+ ret = -ENODEV;
+ goto fail;
+ }
"dpll_disp_ck" is a platform specific matter, driver should not
be handling platform specifics. And this won't work on DaVinci,
you can probably make use of
meaning the clock has a different name on davinci, or? Presumably
there must be some clock generating the pixel clock for the display,
but I confess to not being too familiar with the details on davinci..
BR,
-R
my series "[PATCH v2 0/4] ARM: AM335x: LCDC platform support"
or something similar to overcome this.
Regards
Afzal
+ /* in raster mode, minimum divisor is 2: */
+ ret = clk_set_rate(priv->disp_clk, crtc->mode.clock * 1000 * 2);
quoted
quoted
These things can better be handled with divider clock having a
minimum value (being discussed with Mike on how exactly it should
be) and instead of setting rate over a platform specific clock,
you can set rate over lcd clock using SET_RATE_PARENT at platform
level, more below,
quoted
I looked at that patch you were proposing for da8xx-fb.. to be
honest, it did not seem simpler to me, so I was sort of failing to see
the benefit..
It's not about being simple, but not doing the wrong way, here you are
relying on a platform specific clock in a driver, think about the case
where same IP is used on another platform. Either way it is not a good
thing to handle platform specific details (about disp_clk) in driver.
Right, but I was trying to understand what was the benefit that the
added complexity is. I didn't realize on davinci that you are limited
to just setting divider values (which is going to drastically limit
the timings you can generate, although maybe not an issue if all you
support is a fixed lcd panel).
And Mike mentioned that one of design goals of CCF is to model these
kinds of clocks in IP's.
Mike was suggesting to model the above using gate/divider/composite
clocks of CCF in the FB driver.
quoted
quoted
quoted
+ priv->clk = clk_get(dev->dev, "fck");
+ if (IS_ERR(priv->clk)) {
+ dev_err(dev->dev, "failed to get functional clock\n");
+ ret = -ENODEV;
+ goto fail;
+ }
+
+ priv->disp_clk = clk_get(dev->dev, "dpll_disp_ck");
+ if (IS_ERR(priv->clk)) {
+ dev_err(dev->dev, "failed to get display clock\n");
+ ret = -ENODEV;
+ goto fail;
+ }
"dpll_disp_ck" is a platform specific matter, driver should not
be handling platform specifics. And this won't work on DaVinci,
you can probably make use of
meaning the clock has a different name on davinci, or? Presumably
there must be some clock generating the pixel clock for the display,
but I confess to not being too familiar with the details on davinci..
The only option for the driver in DaVinci is to configure clock rate
using the divider of LCDC IP, it has "fck", which gives a rate
decided by its ancestors.
Well, from looking at the other patch series it seems CCF doesn't
support minimum divider yet. And davinci doesn't support CCF yet. So
at the moment all this discussion is a bit moot. I'd propose leaving
the driver as it is for now, because that at least makes it useful on
am33xx. And when CCF and davinci have the needed support in place,
then send a patch to change the clock handling in tilcdc. I don't
actually have any davinci hw to test on, but I can easily test on
am33xx.
BR,
-R
From: Rob Clark <hidden> Date: 2013-01-28 16:37:13
On Mon, Jan 28, 2013 at 3:56 AM, Mohammed, Afzal [off-list ref] wrote:
Hi Rob,
On Fri, Jan 25, 2013 at 20:22:55, Rob Clark wrote:
quoted
On Fri, Jan 25, 2013 at 8:15 AM, Mohammed, Afzal [off-list ref] wrote:
quoted
quoted
It's not about being simple, but not doing the wrong way, here you are
relying on a platform specific clock in a driver, think about the case
where same IP is used on another platform. Either way it is not a good
thing to handle platform specific details (about disp_clk) in driver.
quoted
Right, but I was trying to understand what was the benefit that the
added complexity is. I didn't realize on davinci that you are limited
Here I am referring to usage of disp_clk,
Driver is not supposed to do platform hacks - here you are trying to
configure something (PLL) in your driver which is not part of LCDC IP.
And LCDC IP is not aware of PLL which is a platform specific matter
(existent only in AM335x), it is only aware of functional clock.
You can set the rate on "fck" (functional clock) in AM335x using my patch,
"ARM: AM33XX: clock: SET_RATE_PARENT in lcd path", and there
would not be any need for driver to be aware of platform specific PLL.
right, but I think it would be better to just make another patch that
changes tilcdc to just set rate on fck after that patch is merged. I
mean, I'd rather have the driver at least usable on AM33xx until then,
rather than broken for everyone.
BR,
-R
at the moment all this discussion is a bit moot. I'd propose leaving
the driver as it is for now, because that at least makes it useful on
am33xx. And when CCF and davinci have the needed support in place,
Let's forget about leveraging CCF in driver, but sane solution w.r.t PLL
configuration would be to do as mentioned above.
Regards
Afzal