On 9/6/18 4:59 AM, r yang wrote:
[snip]
PLL_M is kinda reserved for the memory, hence its rate can't be changed.
Are you referring to EMC scaling? It appears to me that the EMC clock
scales as a divided clock running off PLL_M. PLL_M itself remains locked
at the initial rate.
Yes, I was thinking about the EMC scaling. That could be implemented in a different
ways. Lowering PLL_M clock rate should be a bit more energy efficient, but maybe
that's not really important.
On this tablet the memory is 600Mhz which so happens to accomodate for
the gpu clocks as well as sclk and friends. That's the way the the
downstream kernel works for this tablet.
Okay, that's something to keep in mind till we'll get to implementing EMC scaling.
It's not a big deal to me if this clock config becomes an option or not.
There is a the option to run DISP1 on PLL_D which will work.
The assigned-clocks variant seems to be working fine, see my reply to the other
email in the thread.
I don't have the 76Mhz variant anymore. The device died last year but
until then I was primarily using that to create the upstream device
tree. The clock configuration requirements are the same for both
variants aside from the different clock rate.
Ok.
Secondly, the issue with the workaround in the display driver still
exists. The display driver is setting the display clock rate to zero
in atomic check and applies the disp clock on the commit.
This results in the device locking up.
It appears to me that there isn't a way to avoid the issue with this
workaround.
I thought this lock up was due to the display panel chip in this
particular tablet. I have discovered that the device will lock up if
I set pll_d or pll_c clock rate to zero. And that this happens even
if disp1 is not running off the pll.
Please show the clock tree of the upstream kernel from
"/sys/kernel/debug/clk/clk_summary".
It's attached below due to length.
There is no lock up when trying to set pll_p rate to zero. I suspect
this might be due to it being a fixed rate clock.
Is this expected behavior? Can you reproduce this lock up?
Yes, PLLP is defined as fixed clock in the clock driver.
It is a bug in the DC driver. The disp clocks are registered with the
CLK_SET_RATE_PARENT flag, hence disp clock rate changes propagate to the parent.
Please feel free to send a patch, just remove the clk_set_rate(dc->clk,
state->pclk) from tegra_dc_commit_state().
Are you certain about removing this? That clk_set_rate() appears to have been
added for a reason in commit:
39e08affecf0998be1b01f4752016e33fa98eb9a
I think this is possibly needed for later Tegra boards with more
advanced clock configurations.
Indeed, good catch.
I took a closer look and the real bug is in clk rate-rounding code, kernel loops
infinitely in the rounding code if rate = 0. Here is a draft patch:
diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
index ddb431247f08..11bdc39dc740 100644
--- a/drivers/clk/tegra/clk-pll.c
+++ b/drivers/clk/tegra/clk-pll.c
@@ -840,6 +840,11 @@ static long clk_pll_round_rate(struct clk_hw *hw, unsigned long rate,
return pll->params->fixed_rate;
}
+ if (rate == 0)
+ return -EINVAL;
+
+ rate = max(rate, pll->params->cf_min);
+
if (_get_table_rate(hw, &cfg, rate, *prate) &&
pll->params->calc_rate(hw, &cfg, rate, *prate))
return -EINVAL;
@@ -1325,6 +1330,11 @@ static long clk_pll_ramp_round_rate(struct clk_hw *hw, unsigned long rate,
int ret, p_div;
u64 output_rate = *prate;
+ if (rate == 0)
+ return -EINVAL;
+
+ rate = max(rate, pll->params->cf_min);
+
ret = _pll_ramp_calc_pll(hw, &cfg, rate, *prate);
if (ret < 0)
return ret;
@@ -1567,6 +1577,11 @@ static long clk_pllre_round_rate(struct clk_hw *hw, unsigned long rate,
{
struct tegra_clk_pll *pll = to_clk_pll(hw);
+ if (rate == 0)
+ return -EINVAL;
+
+ rate = max(rate, pll->params->cf_min);
+
return _pllre_calc_rate(pll, NULL, rate, *prate);
}
Still it is likely that machine will hang later if discrepancy of displays rate with the
panels rate is big.
Looks okay so far. The parent clocks (PLL_D / PLL_C) alone don't lock up
anymore when trying to set the rate to zero.
Nice, I'll make a proper patch then.
[snip]
