[RFC] drm/tegra: Add a flag to mark that there is only one display pll

There is a workaround in which the tegra rgb driver initializes
the tegra dc pclk to 0 so that it will skip setting the parent clk rate.

The relevant commits:
3cebae6737b100323baca21de6bce6647249e778
76d59ed049197bdaaa24c0e061a105af6ce74457

A more recent commit sets the rate of the dc clk itself:
39e08affecf0998be1b01f4752016e33fa98eb9a

This doesn't make sense because it always sets the dc clk to 0.
Is this intended behavior or does it just happen to be working for
the current tegra 2 boards in the mainline kernel?


For context I am running the kernel on a tegra 2 based
Galaxy Tab 10.1. The display panel driver is out of tree.
This panel has very low clock rate tolerances so it must be
driven at a rate very close to the required specification (68.75Mhz).
pll_p is not adequate to drive this panel so pll_d must be used.

Another issue is the current workaround is always forcing the disp1
clk to zero. The Galaxy Tab 10.1 locks up completely when calling
clk_set_rate with a clk rate of 0 whether the parent is pll_p or
pll_d.


This patch adds a flag single_display_pll to mark that the device has
only one display pll. This replaces the the pclk = 0 workaround.

There is a comment in rgb.c about using the shift clock divider for
tegra 2 but the divider is not set due to the has_nvdisplay flag.
This patch also sets the shift clock divider based on the
single_display_pll flag.

A change I'm uncertain about. In tegra_dc_commit_state()
the dc clock is now being set to the display panel rate rather than zero.

I don't have any other tegra devices to test with. So I don't know
if this breaks other devices. Previously the code was trying to set the
clock rate to zero anyways.

Signed-off-by: ryang <decatf@gmail.com>
---
 drivers/gpu/drm/tegra/dc.c  | 9 +++++++--
 drivers/gpu/drm/tegra/dc.h  | 1 +
 drivers/gpu/drm/tegra/rgb.c | 1 -
 3 files changed, 8 insertions(+), 3 deletions(-)

On Friday 24 August 2018 03:53:22 ryang wrote:
> There is a workaround in which the tegra rgb driver initializes
> the tegra dc pclk to 0 so that it will skip setting the parent clk rate.
>
> The relevant commits:
> 3cebae6737b100323baca21de6bce6647249e778
> 76d59ed049197bdaaa24c0e061a105af6ce74457
>
> A more recent commit sets the rate of the dc clk itself:
> 39e08affecf0998be1b01f4752016e33fa98eb9a
>
> This doesn't make sense because it always sets the dc clk to 0.
> Is this intended behavior or does it just happen to be working for
> the current tegra 2 boards in the mainline kernel?
>
>
> For context I am running the kernel on a tegra 2 based
> Galaxy Tab 10.1. The display panel driver is out of tree.
> This panel has very low clock rate tolerances so it must be
> driven at a rate very close to the required specification (68.75Mhz).
> pll_p is not adequate to drive this panel so pll_d must be used.
>
> Another issue is the current workaround is always forcing the disp1
> clk to zero. The Galaxy Tab 10.1 locks up completely when calling
> clk_set_rate with a clk rate of 0 whether the parent is pll_p or
> pll_d.
>
>
> This patch adds a flag single_display_pll to mark that the device has
> only one display pll. This replaces the the pclk = 0 workaround.
>
> There is a comment in rgb.c about using the shift clock divider for
> tegra 2 but the divider is not set due to the has_nvdisplay flag.
> This patch also sets the shift clock divider based on the
> single_display_pll flag.
>
> A change I'm uncertain about. In tegra_dc_commit_state()
> the dc clock is now being set to the display panel rate rather than zero.
>
> I don't have any other tegra devices to test with. So I don't know
> if this breaks other devices. Previously the code was trying to set the
> clock rate to zero anyways.
>
> Signed-off-by: ryang <decatf@gmail.com>
> ---
>  drivers/gpu/drm/tegra/dc.c  | 9 +++++++--
>  drivers/gpu/drm/tegra/dc.h  | 1 +
>  drivers/gpu/drm/tegra/rgb.c | 1 -
>  3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 965088afcfad..03f1ad630254 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -1664,7 +1664,7 @@ static void tegra_dc_commit_state(struct tegra_dc
> *dc, * which is shared with other peripherals. Changing the clock rate *
> should therefore be avoided.
>  	 */
> -	if (state->pclk > 0) {
> +	if (!dc->soc->single_display_pll) {

We don't want to change pll_p rate here. It will be better to check whether 
parent clock for disp is pll_d in tegra_rgb_encoder_atomic_check() and if it 
is the parent, then allow to propagate pclk into commit_state. We may request 
the pll_d using clk_get_sys(), then get the parent clock of disp by 
clk_get_parent(disp_clk) and finally compare the parent with pll_d using 
clk_is_math(parent, pll_d).

Certainly parent clock selection could be more advanced, ideally the best 
parent clock should be selected in the atomic check and applied to disp clk 
on the commit by changing the disp's parent. So pll_d could be always 
selected if only one display controller is active at a time, otherwise the 
atomic check should resolve the parent clocks based on the required rates. It 
also could be permitable to adjust the pll_c rate.

On 24.08.2018 11:45, Peter Geis wrote:
> If no other users are taking control of the plld, couldn't he just use assign
> clocks to set up the clock in the device tree?
Yes, but then you need to know that disp clock is running off the pll_d. Also
exclusive ownership over the parent clock shall be declared to make sure that
nothing else will change the parents clock rate. I'll try to make a draft patch.

Again, ideally displays shouldn't be tied to a particular clock on Tegra20. I
think for now allowance to change the parent rate if that parent is pll_d should
be a reasonable stopgap solution.

On Fri, Aug 31, 2018 at 11:44:11PM +0300, Dmitry Osipenko wrote:
> On 8/31/18 10:31 PM, r yang wrote:
> > On Wed, Aug 29, 2018 at 08:52:37PM +0300, Dmitry Osipenko wrote:
> >> On 25.08.2018 04:11, r yang wrote:
> >>> On Fri, Aug 24, 2018 at 05:40:11AM +0300, Dmitry Osipenko wrote:
> >>>> On Friday 24 August 2018 03:53:22 ryang wrote:
> >>>>> There is a workaround in which the tegra rgb driver initializes
> >>>>> the tegra dc pclk to 0 so that it will skip setting the parent clk rate.
> >>>>>
> >>>>> The relevant commits:
> >>>>> 3cebae6737b100323baca21de6bce6647249e778
> >>>>> 76d59ed049197bdaaa24c0e061a105af6ce74457
> >>>>>
> >>>>> A more recent commit sets the rate of the dc clk itself:
> >>>>> 39e08affecf0998be1b01f4752016e33fa98eb9a
> >>>>>
> >>>>> This doesn't make sense because it always sets the dc clk to 0.
> >>>>> Is this intended behavior or does it just happen to be working for
> >>>>> the current tegra 2 boards in the mainline kernel?
> >>>>>
> >>>>>
> >>>>> For context I am running the kernel on a tegra 2 based
> >>>>> Galaxy Tab 10.1. The display panel driver is out of tree.
> >>>>> This panel has very low clock rate tolerances so it must be
> >>>>> driven at a rate very close to the required specification (68.75Mhz).
> >>>>> pll_p is not adequate to drive this panel so pll_d must be used.
> >>>>>
> >>>>> Another issue is the current workaround is always forcing the disp1
> >>>>> clk to zero. The Galaxy Tab 10.1 locks up completely when calling
> >>>>> clk_set_rate with a clk rate of 0 whether the parent is pll_p or
> >>>>> pll_d.
> >>>>>
> >>>>>
> >>>>> This patch adds a flag single_display_pll to mark that the device has
> >>>>> only one display pll. This replaces the the pclk = 0 workaround.
> >>>>>
> >>>>> There is a comment in rgb.c about using the shift clock divider for
> >>>>> tegra 2 but the divider is not set due to the has_nvdisplay flag.
> >>>>> This patch also sets the shift clock divider based on the
> >>>>> single_display_pll flag.
> >>>>>
> >>>>> A change I'm uncertain about. In tegra_dc_commit_state()
> >>>>> the dc clock is now being set to the display panel rate rather than zero.
> >>>>>
> >>>>> I don't have any other tegra devices to test with. So I don't know
> >>>>> if this breaks other devices. Previously the code was trying to set the
> >>>>> clock rate to zero anyways.
> >>>>>
> >>>>> Signed-off-by: ryang <decatf@gmail.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/tegra/dc.c  | 9 +++++++--
> >>>>>  drivers/gpu/drm/tegra/dc.h  | 1 +
> >>>>>  drivers/gpu/drm/tegra/rgb.c | 1 -
> >>>>>  3 files changed, 8 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> >>>>> index 965088afcfad..03f1ad630254 100644
> >>>>> --- a/drivers/gpu/drm/tegra/dc.c
> >>>>> +++ b/drivers/gpu/drm/tegra/dc.c
> >>>>> @@ -1664,7 +1664,7 @@ static void tegra_dc_commit_state(struct tegra_dc
> >>>>> *dc, * which is shared with other peripherals. Changing the clock rate *
> >>>>> should therefore be avoided.
> >>>>>  	 */
> >>>>> -	if (state->pclk > 0) {
> >>>>> +	if (!dc->soc->single_display_pll) {
> >>>>
> >>>> We don't want to change pll_p rate here. It will be better to check whether 
> >>>> parent clock for disp is pll_d in tegra_rgb_encoder_atomic_check() and if it 
> >>>> is the parent, then allow to propagate pclk into commit_state. We may request 
> >>>> the pll_d using clk_get_sys(), then get the parent clock of disp by 
> >>>> clk_get_parent(disp_clk) and finally compare the parent with pll_d using 
> >>>> clk_is_math(parent, pll_d).
> >>>
> >>> Understood.
> >>>
> >>>>
> >>>> Certainly parent clock selection could be more advanced, ideally the best 
> >>>> parent clock should be selected in the atomic check and applied to disp clk 
> >>>> on the commit by changing the disp's parent. So pll_d could be always 
> >>>> selected if only one display controller is active at a time, otherwise the 
> >>>> atomic check should resolve the parent clocks based on the required rates. It 
> >>>> also could be permitable to adjust the pll_c rate.
> >>>
> >>> Right, the TRM describes quite a wide range of clock selection which currently
> >>> aren't all possible to use on the kernel. The Galaxy Tab 10.1 stock kernel uses
> >>> pll_c solely for disp1. Every clock normally on pll_c such as gr3d/gr2d/vde/etc
> >>> are on pll_m. I suspect it was designed this way because the TRM says pll_d has
> >>> a very power usage and that it should be avoided if possible.
> >>>
> >>> In absense of more advanced clock selection. It was my intention with this
> >>> patch to try to at least have pll_p and pll_d working right for the internal
> >>> display.
> >>>
> >>
> >> Could you show the clock tree from downstream kernel? I'm curious what it looks
> >> like, that should be in "/sys/kernel/debug/clock/clock_tree".
> >>
> >>
> >> The patch below seems to work reasonably good, both HDMI and panel either are
> >> working simultaneously or only panel works when both displays are running off
> >> the pll_d. Could you give it a try?
> >>
> >>
> >> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> >> index bdc418bcd898..c91f1203418d 100644
> >> --- a/drivers/gpu/drm/tegra/dc.c
> >> +++ b/drivers/gpu/drm/tegra/dc.c
> >> @@ -26,6 +26,33 @@
> >>  #include <drm/drm_atomic_helper.h>
> >>  #include <drm/drm_plane_helper.h>
> >>
> >> +unsigned int tegra_dc_div61(unsigned long  parent, unsigned long pixel)
> >> +{
> >> +	unsigned int rem, fract = 0;
> >> +	unsigned int div61;
> >> +	u64 integer;
> >> +
> >> +	if (parent <= pixel)
> >> +		return 0;
> >> +
> >> +	integer = parent;
> >> +	rem = do_div(integer, pixel);
> >> +
> >> +	if (rem > pixel / 4) {
> >> +		if (rem > pixel * 2 / 3)
> >> +			integer += 1;
> >> +		else
> >> +			fract = 1;
> >> +	}
> >> +
> >> +	div61 = (integer << 1) | fract;
> >> +
> >> +	if (div61 > 255)
> >> +		return 255;
> >> +
> >> +	return div61;
> >> +}
> >> +
> >>  static void tegra_dc_stats_reset(struct tegra_dc_stats *stats)
> >>  {
> >>  	stats->frames = 0;
> >> diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
> >> index 382d38b1524b..dc3f681ddf1f 100644
> >> --- a/drivers/gpu/drm/tegra/dc.h
> >> +++ b/drivers/gpu/drm/tegra/dc.h
> >> @@ -174,6 +174,7 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc,
> >>  			       struct drm_crtc_state *crtc_state,
> >>  			       struct clk *clk, unsigned long pclk,
> >>  			       unsigned int div);
> >> +unsigned int tegra_dc_div61(unsigned long  parent, unsigned long pixel);
> >>
> >>  /* from rgb.c */
> >>  int tegra_dc_rgb_probe(struct tegra_dc *dc);
> >> diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
> >> index 0082468f703c..3c19af0eafe7 100644
> >> --- a/drivers/gpu/drm/tegra/hdmi.c
> >> +++ b/drivers/gpu/drm/tegra/hdmi.c
> >> @@ -1450,10 +1450,13 @@ tegra_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
> >>  	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
> >>  	unsigned long pclk = crtc_state->mode.clock * 1000;
> >>  	struct tegra_hdmi *hdmi = to_hdmi(output);
> >> +	unsigned long rate;
> >>  	int err;
> >>
> >> +	rate = clk_round_rate(hdmi->clk_parent, pclk);
> >> +
> >>  	err = tegra_dc_state_setup_clock(dc, crtc_state, hdmi->clk_parent,
> >> -					 pclk, 0);
> >> +					 rate, tegra_dc_div61(rate, pclk));
> >>  	if (err < 0) {
> >>  		dev_err(output->dev, "failed to setup CRTC state: %d\n", err);
> >>  		return err;
> >> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
> >> index 28a78d3120bc..3ec3e13ce47f 100644
> >> --- a/drivers/gpu/drm/tegra/rgb.c
> >> +++ b/drivers/gpu/drm/tegra/rgb.c
> >> @@ -19,6 +19,7 @@ struct tegra_rgb {
> >>  	struct tegra_output output;
> >>  	struct tegra_dc *dc;
> >>
> >> +	struct clk *pll_d_out0;
> >>  	struct clk *clk_parent;
> >>  	struct clk *clk;
> >>  };
> >> @@ -130,6 +131,8 @@ static void tegra_rgb_encoder_disable(struct drm_encoder
> >> *encoder)
> >>
> >>  	if (output->panel)
> >>  		drm_panel_unprepare(output->panel);
> >> +
> >> +	clk_rate_exclusive_put(rgb->clk);
> >>  }
> >>
> >>  static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)
> >> @@ -165,6 +168,8 @@ static void tegra_rgb_encoder_enable(struct drm_encoder
> >> *encoder)
> >>
> >>  	if (output->panel)
> >>  		drm_panel_enable(output->panel);
> >> +
> >> +	clk_rate_exclusive_get(rgb->clk);
> >>  }
> >>
> >>  static int
> >> @@ -174,11 +179,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
> >>  {
> >>  	struct tegra_output *output = encoder_to_output(encoder);
> >>  	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
> >> -	unsigned long pclk = crtc_state->mode.clock * 1000;
> >>  	struct tegra_rgb *rgb = to_rgb(output);
> >> -	unsigned int div;
> >> +	unsigned long pclk = crtc_state->mode.clock * 1000;
> >> +	unsigned long rate;
> >> +	bool pll_d_parent;
> >>  	int err;
> >>
> >> +	if (clk_is_match(rgb->clk_parent, rgb->pll_d_out0))
> >> +		pll_d_parent = true;
> >> +	else
> >> +		pll_d_parent = false;
> >> +
> >>  	/*
> >>  	 * We may not want to change the frequency of the parent clock, since
> >>  	 * it may be a parent for other peripherals. This is due to the fact
> >> @@ -194,12 +205,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
> >>  	 * The best we can do at this point is to use the shift clock divider
> >>  	 * and hope that the desired frequency can be matched (or at least
> >>  	 * matched sufficiently close that the panel will still work).
> >> +	 *
> >> +	 * Make excuse for pll_d_out0 if it is explicitly set as a parent
> >> +	 * for display panel.
> >>  	 */
> >> -	div = ((clk_get_rate(rgb->clk) * 2) / pclk) - 2;
> >> -	pclk = 0;
> >> +	if (pll_d_parent)
> >> +		rate = clk_round_rate(rgb->clk_parent, pclk * 4);
> >> +	else
> >> +		rate = clk_get_rate(rgb->clk);
> >>
> >>  	err = tegra_dc_state_setup_clock(dc, crtc_state, rgb->clk_parent,
> >> -					 pclk, div);
> >> +					 rate, tegra_dc_div61(rate, pclk));
> >>  	if (err < 0) {
> >>  		dev_err(output->dev, "failed to setup CRTC state: %d\n", err);
> >>  		return err;
> >> @@ -254,6 +270,12 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
> >>  		return err;
> >>  	}
> >>
> >> +	rgb->pll_d_out0 = clk_get_sys(NULL, "pll_d_out0");
> >> +	if (IS_ERR(rgb->pll_d_out0)) {
> >> +		dev_err(dc->dev, "failed to get pll_d_out0\n");
> >> +		return PTR_ERR(rgb->pll_d_out0);
> >> +	}
> >> +
> >>  	dc->rgb = &rgb->output;
> >>
> >>  	return 0;
> >> @@ -265,6 +287,7 @@ int tegra_dc_rgb_remove(struct tegra_dc *dc)
> >>  		return 0;
> >>
> >>  	tegra_output_remove(dc->rgb);
> >> +	clk_put(to_rgb(dc->rgb)->pll_d_out0);
> >>  	dc->rgb = NULL;
> >>
> >>  	return 0;
> > 
> > I've tested the patch and it works. The disp1 will run off pll_d without
> > any issues.
> > 
> > Here is the clock tree from the downstream kernel.
> 
> Okay, thank you. Now I think that it's better not to touch the display driver at
> all, but change the PLLC clock to 586 MHz in the clock driver. There is no
> appreciable difference in 600 vs 586 MHz, so that should be good enough.
> 
> It would be ideal if clock rates could be specified via device tree, but that's
> not possible today. So just change the PLLC rate globally in the clocks
> init_table. Maybe later we'll manage to implement something that will allow to
> set rates via device tree.
> 

There are two variants of the display panel chip for this tablet.
They run at different clock rates:

 - 68.9Mhz divided down from 586Mhz
 - 72Mhhz divided down from 570Mhz

With this in consideration I don't think it makes sense to change the
initial pll_c rate.

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.

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?

Is it not possible to go the route of your first proposed patch to check
if the parent of display clock is pll_p?

> Note that changing PLLC rate will makes sense only if you're going to upstream
> the device tree for your device. Please prepare and submit the device tree and
> clock patches. I think Thierry and Peter won't oppose to the variant with
> changing rate of PLLC, for now that should be the best option.
> 

I would like to eventually upstream the device tree. There are only two things
at the moment that are blocking this device from working on upstream kernel.
One is this display clock initialization issue. The other is the
non-standard partition table which required a patch from the Anrdoid kernels.
For now I am using CONFIG_CMDLINE_PARTITION to specify the partition
layout.

> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index c8146e65e7ad..3b725d7c7f46 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -1096,7 +1096,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
>         { TEGRA20_CLK_PLL_P_OUT2, TEGRA20_CLK_CLK_MAX, 48000000, 1 },
>         { TEGRA20_CLK_PLL_P_OUT3, TEGRA20_CLK_CLK_MAX, 72000000, 1 },
>         { TEGRA20_CLK_PLL_P_OUT4, TEGRA20_CLK_CLK_MAX, 24000000, 1 },
> -       { TEGRA20_CLK_PLL_C, TEGRA20_CLK_CLK_MAX, 600000000, 0 },
> +       { TEGRA20_CLK_PLL_C, TEGRA20_CLK_CLK_MAX, 586000000, 0 },
>         { TEGRA20_CLK_PLL_C_OUT1, TEGRA20_CLK_CLK_MAX, 240000000, 0 },
>         { TEGRA20_CLK_SCLK, TEGRA20_CLK_PLL_C_OUT1, 240000000, 0 },
>         { TEGRA20_CLK_HCLK, TEGRA20_CLK_CLK_MAX, 240000000, 0 },

On 9/2/18 3:27 AM, r yang wrote:
> On Fri, Aug 31, 2018 at 11:44:11PM +0300, Dmitry Osipenko wrote:
>> On 8/31/18 10:31 PM, r yang wrote:
>>> On Wed, Aug 29, 2018 at 08:52:37PM +0300, Dmitry Osipenko wrote:
>>>> On 25.08.2018 04:11, r yang wrote:
>>>>> On Fri, Aug 24, 2018 at 05:40:11AM +0300, Dmitry Osipenko wrote:
>>>>>> On Friday 24 August 2018 03:53:22 ryang wrote:
>>>>>>> There is a workaround in which the tegra rgb driver initializes
>>>>>>> the tegra dc pclk to 0 so that it will skip setting the parent clk rate.
>>>>>>>
>>>>>>> The relevant commits:
>>>>>>> 3cebae6737b100323baca21de6bce6647249e778
>>>>>>> 76d59ed049197bdaaa24c0e061a105af6ce74457
>>>>>>>
>>>>>>> A more recent commit sets the rate of the dc clk itself:
>>>>>>> 39e08affecf0998be1b01f4752016e33fa98eb9a
>>>>>>>
>>>>>>> This doesn't make sense because it always sets the dc clk to 0.
>>>>>>> Is this intended behavior or does it just happen to be working for
>>>>>>> the current tegra 2 boards in the mainline kernel?
>>>>>>>
>>>>>>>
>>>>>>> For context I am running the kernel on a tegra 2 based
>>>>>>> Galaxy Tab 10.1. The display panel driver is out of tree.
>>>>>>> This panel has very low clock rate tolerances so it must be
>>>>>>> driven at a rate very close to the required specification (68.75Mhz).
>>>>>>> pll_p is not adequate to drive this panel so pll_d must be used.
>>>>>>>
>>>>>>> Another issue is the current workaround is always forcing the disp1
>>>>>>> clk to zero. The Galaxy Tab 10.1 locks up completely when calling
>>>>>>> clk_set_rate with a clk rate of 0 whether the parent is pll_p or
>>>>>>> pll_d.
>>>>>>>
>>>>>>>
>>>>>>> This patch adds a flag single_display_pll to mark that the device has
>>>>>>> only one display pll. This replaces the the pclk = 0 workaround.
>>>>>>>
>>>>>>> There is a comment in rgb.c about using the shift clock divider for
>>>>>>> tegra 2 but the divider is not set due to the has_nvdisplay flag.
>>>>>>> This patch also sets the shift clock divider based on the
>>>>>>> single_display_pll flag.
>>>>>>>
>>>>>>> A change I'm uncertain about. In tegra_dc_commit_state()
>>>>>>> the dc clock is now being set to the display panel rate rather than zero.
>>>>>>>
>>>>>>> I don't have any other tegra devices to test with. So I don't know
>>>>>>> if this breaks other devices. Previously the code was trying to set the
>>>>>>> clock rate to zero anyways.
>>>>>>>
>>>>>>> Signed-off-by: ryang <decatf@gmail.com>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/tegra/dc.c  | 9 +++++++--
>>>>>>>  drivers/gpu/drm/tegra/dc.h  | 1 +
>>>>>>>  drivers/gpu/drm/tegra/rgb.c | 1 -
>>>>>>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>>>>>> index 965088afcfad..03f1ad630254 100644
>>>>>>> --- a/drivers/gpu/drm/tegra/dc.c
>>>>>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>>>>>> @@ -1664,7 +1664,7 @@ static void tegra_dc_commit_state(struct tegra_dc
>>>>>>> *dc, * which is shared with other peripherals. Changing the clock rate *
>>>>>>> should therefore be avoided.
>>>>>>>  	 */
>>>>>>> -	if (state->pclk > 0) {
>>>>>>> +	if (!dc->soc->single_display_pll) {
>>>>>>
>>>>>> We don't want to change pll_p rate here. It will be better to check whether 
>>>>>> parent clock for disp is pll_d in tegra_rgb_encoder_atomic_check() and if it 
>>>>>> is the parent, then allow to propagate pclk into commit_state. We may request 
>>>>>> the pll_d using clk_get_sys(), then get the parent clock of disp by 
>>>>>> clk_get_parent(disp_clk) and finally compare the parent with pll_d using 
>>>>>> clk_is_math(parent, pll_d).
>>>>>
>>>>> Understood.
>>>>>
>>>>>>
>>>>>> Certainly parent clock selection could be more advanced, ideally the best 
>>>>>> parent clock should be selected in the atomic check and applied to disp clk 
>>>>>> on the commit by changing the disp's parent. So pll_d could be always 
>>>>>> selected if only one display controller is active at a time, otherwise the 
>>>>>> atomic check should resolve the parent clocks based on the required rates. It 
>>>>>> also could be permitable to adjust the pll_c rate.
>>>>>
>>>>> Right, the TRM describes quite a wide range of clock selection which currently
>>>>> aren't all possible to use on the kernel. The Galaxy Tab 10.1 stock kernel uses
>>>>> pll_c solely for disp1. Every clock normally on pll_c such as gr3d/gr2d/vde/etc
>>>>> are on pll_m. I suspect it was designed this way because the TRM says pll_d has
>>>>> a very power usage and that it should be avoided if possible.
>>>>>
>>>>> In absense of more advanced clock selection. It was my intention with this
>>>>> patch to try to at least have pll_p and pll_d working right for the internal
>>>>> display.
>>>>>
>>>>
>>>> Could you show the clock tree from downstream kernel? I'm curious what it looks
>>>> like, that should be in "/sys/kernel/debug/clock/clock_tree".
>>>>
>>>>
>>>> The patch below seems to work reasonably good, both HDMI and panel either are
>>>> working simultaneously or only panel works when both displays are running off
>>>> the pll_d. Could you give it a try?
>>>>
>>>>
>>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>>> index bdc418bcd898..c91f1203418d 100644
>>>> --- a/drivers/gpu/drm/tegra/dc.c
>>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>>> @@ -26,6 +26,33 @@
>>>>  #include <drm/drm_atomic_helper.h>
>>>>  #include <drm/drm_plane_helper.h>
>>>>
>>>> +unsigned int tegra_dc_div61(unsigned long  parent, unsigned long pixel)
>>>> +{
>>>> +	unsigned int rem, fract = 0;
>>>> +	unsigned int div61;
>>>> +	u64 integer;
>>>> +
>>>> +	if (parent <= pixel)
>>>> +		return 0;
>>>> +
>>>> +	integer = parent;
>>>> +	rem = do_div(integer, pixel);
>>>> +
>>>> +	if (rem > pixel / 4) {
>>>> +		if (rem > pixel * 2 / 3)
>>>> +			integer += 1;
>>>> +		else
>>>> +			fract = 1;
>>>> +	}
>>>> +
>>>> +	div61 = (integer << 1) | fract;
>>>> +
>>>> +	if (div61 > 255)
>>>> +		return 255;
>>>> +
>>>> +	return div61;
>>>> +}
>>>> +
>>>>  static void tegra_dc_stats_reset(struct tegra_dc_stats *stats)
>>>>  {
>>>>  	stats->frames = 0;
>>>> diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
>>>> index 382d38b1524b..dc3f681ddf1f 100644
>>>> --- a/drivers/gpu/drm/tegra/dc.h
>>>> +++ b/drivers/gpu/drm/tegra/dc.h
>>>> @@ -174,6 +174,7 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc,
>>>>  			       struct drm_crtc_state *crtc_state,
>>>>  			       struct clk *clk, unsigned long pclk,
>>>>  			       unsigned int div);
>>>> +unsigned int tegra_dc_div61(unsigned long  parent, unsigned long pixel);
>>>>
>>>>  /* from rgb.c */
>>>>  int tegra_dc_rgb_probe(struct tegra_dc *dc);
>>>> diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
>>>> index 0082468f703c..3c19af0eafe7 100644
>>>> --- a/drivers/gpu/drm/tegra/hdmi.c
>>>> +++ b/drivers/gpu/drm/tegra/hdmi.c
>>>> @@ -1450,10 +1450,13 @@ tegra_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
>>>>  	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
>>>>  	unsigned long pclk = crtc_state->mode.clock * 1000;
>>>>  	struct tegra_hdmi *hdmi = to_hdmi(output);
>>>> +	unsigned long rate;
>>>>  	int err;
>>>>
>>>> +	rate = clk_round_rate(hdmi->clk_parent, pclk);
>>>> +
>>>>  	err = tegra_dc_state_setup_clock(dc, crtc_state, hdmi->clk_parent,
>>>> -					 pclk, 0);
>>>> +					 rate, tegra_dc_div61(rate, pclk));
>>>>  	if (err < 0) {
>>>>  		dev_err(output->dev, "failed to setup CRTC state: %d\n", err);
>>>>  		return err;
>>>> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
>>>> index 28a78d3120bc..3ec3e13ce47f 100644
>>>> --- a/drivers/gpu/drm/tegra/rgb.c
>>>> +++ b/drivers/gpu/drm/tegra/rgb.c
>>>> @@ -19,6 +19,7 @@ struct tegra_rgb {
>>>>  	struct tegra_output output;
>>>>  	struct tegra_dc *dc;
>>>>
>>>> +	struct clk *pll_d_out0;
>>>>  	struct clk *clk_parent;
>>>>  	struct clk *clk;
>>>>  };
>>>> @@ -130,6 +131,8 @@ static void tegra_rgb_encoder_disable(struct drm_encoder
>>>> *encoder)
>>>>
>>>>  	if (output->panel)
>>>>  		drm_panel_unprepare(output->panel);
>>>> +
>>>> +	clk_rate_exclusive_put(rgb->clk);
>>>>  }
>>>>
>>>>  static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)
>>>> @@ -165,6 +168,8 @@ static void tegra_rgb_encoder_enable(struct drm_encoder
>>>> *encoder)
>>>>
>>>>  	if (output->panel)
>>>>  		drm_panel_enable(output->panel);
>>>> +
>>>> +	clk_rate_exclusive_get(rgb->clk);
>>>>  }
>>>>
>>>>  static int
>>>> @@ -174,11 +179,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
>>>>  {
>>>>  	struct tegra_output *output = encoder_to_output(encoder);
>>>>  	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
>>>> -	unsigned long pclk = crtc_state->mode.clock * 1000;
>>>>  	struct tegra_rgb *rgb = to_rgb(output);
>>>> -	unsigned int div;
>>>> +	unsigned long pclk = crtc_state->mode.clock * 1000;
>>>> +	unsigned long rate;
>>>> +	bool pll_d_parent;
>>>>  	int err;
>>>>
>>>> +	if (clk_is_match(rgb->clk_parent, rgb->pll_d_out0))
>>>> +		pll_d_parent = true;
>>>> +	else
>>>> +		pll_d_parent = false;
>>>> +
>>>>  	/*
>>>>  	 * We may not want to change the frequency of the parent clock, since
>>>>  	 * it may be a parent for other peripherals. This is due to the fact
>>>> @@ -194,12 +205,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
>>>>  	 * The best we can do at this point is to use the shift clock divider
>>>>  	 * and hope that the desired frequency can be matched (or at least
>>>>  	 * matched sufficiently close that the panel will still work).
>>>> +	 *
>>>> +	 * Make excuse for pll_d_out0 if it is explicitly set as a parent
>>>> +	 * for display panel.
>>>>  	 */
>>>> -	div = ((clk_get_rate(rgb->clk) * 2) / pclk) - 2;
>>>> -	pclk = 0;
>>>> +	if (pll_d_parent)
>>>> +		rate = clk_round_rate(rgb->clk_parent, pclk * 4);
>>>> +	else
>>>> +		rate = clk_get_rate(rgb->clk);
>>>>
>>>>  	err = tegra_dc_state_setup_clock(dc, crtc_state, rgb->clk_parent,
>>>> -					 pclk, div);
>>>> +					 rate, tegra_dc_div61(rate, pclk));
>>>>  	if (err < 0) {
>>>>  		dev_err(output->dev, "failed to setup CRTC state: %d\n", err);
>>>>  		return err;
>>>> @@ -254,6 +270,12 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
>>>>  		return err;
>>>>  	}
>>>>
>>>> +	rgb->pll_d_out0 = clk_get_sys(NULL, "pll_d_out0");
>>>> +	if (IS_ERR(rgb->pll_d_out0)) {
>>>> +		dev_err(dc->dev, "failed to get pll_d_out0\n");
>>>> +		return PTR_ERR(rgb->pll_d_out0);
>>>> +	}
>>>> +
>>>>  	dc->rgb = &rgb->output;
>>>>
>>>>  	return 0;
>>>> @@ -265,6 +287,7 @@ int tegra_dc_rgb_remove(struct tegra_dc *dc)
>>>>  		return 0;
>>>>
>>>>  	tegra_output_remove(dc->rgb);
>>>> +	clk_put(to_rgb(dc->rgb)->pll_d_out0);
>>>>  	dc->rgb = NULL;
>>>>
>>>>  	return 0;
>>>
>>> I've tested the patch and it works. The disp1 will run off pll_d without
>>> any issues.
>>>
>>> Here is the clock tree from the downstream kernel.
>>
>> Okay, thank you. Now I think that it's better not to touch the display driver at
>> all, but change the PLLC clock to 586 MHz in the clock driver. There is no
>> appreciable difference in 600 vs 586 MHz, so that should be good enough.
>>
>> It would be ideal if clock rates could be specified via device tree, but that's
>> not possible today. So just change the PLLC rate globally in the clocks
>> init_table. Maybe later we'll manage to implement something that will allow to
>> set rates via device tree.
>>
> 
> There are two variants of the display panel chip for this tablet.
> They run at different clock rates:
> 
>  - 68.9Mhz divided down from 586Mhz
>  - 72Mhhz divided down from 570Mhz
> 
> With this in consideration I don't think it makes sense to change the
> initial pll_c rate.

The 72MHz variant will work perfectly with the PLLP as a parent. Have you tried
to use timings and rate of the 72MHz variant?

> 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".

> 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().

> Is it not possible to go the route of your first proposed patch to check
> if the parent of display clock is pll_p?

It is possible. If you'll decide to choose that route, then please drop the
tegra_dc_div61() stuff as it is actually div71 and is not needed at all, I was
just trying something using it.

I think both variants are acceptable, though let's wait for the comment from
Thierry, as the last word will be after him.

>> Note that changing PLLC rate will makes sense only if you're going to upstream
>> the device tree for your device. Please prepare and submit the device tree and
>> clock patches. I think Thierry and Peter won't oppose to the variant with
>> changing rate of PLLC, for now that should be the best option.
>>
> 
> I would like to eventually upstream the device tree. There are only two things
> at the moment that are blocking this device from working on upstream kernel.
> One is this display clock initialization issue. The other is the
> non-standard partition table which required a patch from the Anrdoid kernels.
> For now I am using CONFIG_CMDLINE_PARTITION to specify the partition
> layout.

The partition layout shouldn't be a real stopper as seems there is external MMC.

IIRC, the custom partition is the ms-dos partition with a custom offsets, isn't
it? At least some of Tegra devices also have a spare GPT partition that is
specified by bootloader via gpt_sector argument of the kernels commandline.

On 9/1/2018 9:54 PM, Dmitry Osipenko wrote:
> On 9/2/18 3:27 AM, r yang wrote:
>> On Fri, Aug 31, 2018 at 11:44:11PM +0300, Dmitry Osipenko wrote:
>>> On 8/31/18 10:31 PM, r yang wrote:
>>>> On Wed, Aug 29, 2018 at 08:52:37PM +0300, Dmitry Osipenko wrote:
>>>>> On 25.08.2018 04:11, r yang wrote:
>>>>>> On Fri, Aug 24, 2018 at 05:40:11AM +0300, Dmitry Osipenko wrote:
>>>>>>> On Friday 24 August 2018 03:53:22 ryang wrote:
>>>>>>>> There is a workaround in which the tegra rgb driver initializes
>>>>>>>> the tegra dc pclk to 0 so that it will skip setting the parent clk rate.
>>>>>>>>
>>>>>>>> The relevant commits:
>>>>>>>> 3cebae6737b100323baca21de6bce6647249e778
>>>>>>>> 76d59ed049197bdaaa24c0e061a105af6ce74457
>>>>>>>>
>>>>>>>> A more recent commit sets the rate of the dc clk itself:
>>>>>>>> 39e08affecf0998be1b01f4752016e33fa98eb9a
>>>>>>>>
>>>>>>>> This doesn't make sense because it always sets the dc clk to 0.
>>>>>>>> Is this intended behavior or does it just happen to be working for
>>>>>>>> the current tegra 2 boards in the mainline kernel?
>>>>>>>>
>>>>>>>>
>>>>>>>> For context I am running the kernel on a tegra 2 based
>>>>>>>> Galaxy Tab 10.1. The display panel driver is out of tree.
>>>>>>>> This panel has very low clock rate tolerances so it must be
>>>>>>>> driven at a rate very close to the required specification (68.75Mhz).
>>>>>>>> pll_p is not adequate to drive this panel so pll_d must be used.
>>>>>>>>
>>>>>>>> Another issue is the current workaround is always forcing the disp1
>>>>>>>> clk to zero. The Galaxy Tab 10.1 locks up completely when calling
>>>>>>>> clk_set_rate with a clk rate of 0 whether the parent is pll_p or
>>>>>>>> pll_d.
>>>>>>>>
>>>>>>>>
>>>>>>>> This patch adds a flag single_display_pll to mark that the device has
>>>>>>>> only one display pll. This replaces the the pclk = 0 workaround.
>>>>>>>>
>>>>>>>> There is a comment in rgb.c about using the shift clock divider for
>>>>>>>> tegra 2 but the divider is not set due to the has_nvdisplay flag.
>>>>>>>> This patch also sets the shift clock divider based on the
>>>>>>>> single_display_pll flag.
>>>>>>>>
>>>>>>>> A change I'm uncertain about. In tegra_dc_commit_state()
>>>>>>>> the dc clock is now being set to the display panel rate rather than zero.
>>>>>>>>
>>>>>>>> I don't have any other tegra devices to test with. So I don't know
>>>>>>>> if this breaks other devices. Previously the code was trying to set the
>>>>>>>> clock rate to zero anyways.
>>>>>>>>
>>>>>>>> Signed-off-by: ryang <decatf@gmail.com>
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/tegra/dc.c  | 9 +++++++--
>>>>>>>>   drivers/gpu/drm/tegra/dc.h  | 1 +
>>>>>>>>   drivers/gpu/drm/tegra/rgb.c | 1 -
>>>>>>>>   3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>>>>>>> index 965088afcfad..03f1ad630254 100644
>>>>>>>> --- a/drivers/gpu/drm/tegra/dc.c
>>>>>>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>>>>>>> @@ -1664,7 +1664,7 @@ static void tegra_dc_commit_state(struct tegra_dc
>>>>>>>> *dc, * which is shared with other peripherals. Changing the clock rate *
>>>>>>>> should therefore be avoided.
>>>>>>>>   	 */
>>>>>>>> -	if (state->pclk > 0) {
>>>>>>>> +	if (!dc->soc->single_display_pll) {
>>>>>>>
>>>>>>> We don't want to change pll_p rate here. It will be better to check whether
>>>>>>> parent clock for disp is pll_d in tegra_rgb_encoder_atomic_check() and if it
>>>>>>> is the parent, then allow to propagate pclk into commit_state. We may request
>>>>>>> the pll_d using clk_get_sys(), then get the parent clock of disp by
>>>>>>> clk_get_parent(disp_clk) and finally compare the parent with pll_d using
>>>>>>> clk_is_math(parent, pll_d).
>>>>>>
>>>>>> Understood.
>>>>>>
>>>>>>>
>>>>>>> Certainly parent clock selection could be more advanced, ideally the best
>>>>>>> parent clock should be selected in the atomic check and applied to disp clk
>>>>>>> on the commit by changing the disp's parent. So pll_d could be always
>>>>>>> selected if only one display controller is active at a time, otherwise the
>>>>>>> atomic check should resolve the parent clocks based on the required rates. It
>>>>>>> also could be permitable to adjust the pll_c rate.
>>>>>>
>>>>>> Right, the TRM describes quite a wide range of clock selection which currently
>>>>>> aren't all possible to use on the kernel. The Galaxy Tab 10.1 stock kernel uses
>>>>>> pll_c solely for disp1. Every clock normally on pll_c such as gr3d/gr2d/vde/etc
>>>>>> are on pll_m. I suspect it was designed this way because the TRM says pll_d has
>>>>>> a very power usage and that it should be avoided if possible.
>>>>>>
>>>>>> In absense of more advanced clock selection. It was my intention with this
>>>>>> patch to try to at least have pll_p and pll_d working right for the internal
>>>>>> display.
>>>>>>
>>>>>
>>>>> Could you show the clock tree from downstream kernel? I'm curious what it looks
>>>>> like, that should be in "/sys/kernel/debug/clock/clock_tree".
>>>>>
>>>>>
>>>>> The patch below seems to work reasonably good, both HDMI and panel either are
>>>>> working simultaneously or only panel works when both displays are running off
>>>>> the pll_d. Could you give it a try?
>>>>>
>>>>>
>>>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>>>> index bdc418bcd898..c91f1203418d 100644
>>>>> --- a/drivers/gpu/drm/tegra/dc.c
>>>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>>>> @@ -26,6 +26,33 @@
>>>>>   #include <drm/drm_atomic_helper.h>
>>>>>   #include <drm/drm_plane_helper.h>
>>>>>
>>>>> +unsigned int tegra_dc_div61(unsigned long  parent, unsigned long pixel)
>>>>> +{
>>>>> +	unsigned int rem, fract = 0;
>>>>> +	unsigned int div61;
>>>>> +	u64 integer;
>>>>> +
>>>>> +	if (parent <= pixel)
>>>>> +		return 0;
>>>>> +
>>>>> +	integer = parent;
>>>>> +	rem = do_div(integer, pixel);
>>>>> +
>>>>> +	if (rem > pixel / 4) {
>>>>> +		if (rem > pixel * 2 / 3)
>>>>> +			integer += 1;
>>>>> +		else
>>>>> +			fract = 1;
>>>>> +	}
>>>>> +
>>>>> +	div61 = (integer << 1) | fract;
>>>>> +
>>>>> +	if (div61 > 255)
>>>>> +		return 255;
>>>>> +
>>>>> +	return div61;
>>>>> +}
>>>>> +
>>>>>   static void tegra_dc_stats_reset(struct tegra_dc_stats *stats)
>>>>>   {
>>>>>   	stats->frames = 0;
>>>>> diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
>>>>> index 382d38b1524b..dc3f681ddf1f 100644
>>>>> --- a/drivers/gpu/drm/tegra/dc.h
>>>>> +++ b/drivers/gpu/drm/tegra/dc.h
>>>>> @@ -174,6 +174,7 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc,
>>>>>   			       struct drm_crtc_state *crtc_state,
>>>>>   			       struct clk *clk, unsigned long pclk,
>>>>>   			       unsigned int div);
>>>>> +unsigned int tegra_dc_div61(unsigned long  parent, unsigned long pixel);
>>>>>
>>>>>   /* from rgb.c */
>>>>>   int tegra_dc_rgb_probe(struct tegra_dc *dc);
>>>>> diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
>>>>> index 0082468f703c..3c19af0eafe7 100644
>>>>> --- a/drivers/gpu/drm/tegra/hdmi.c
>>>>> +++ b/drivers/gpu/drm/tegra/hdmi.c
>>>>> @@ -1450,10 +1450,13 @@ tegra_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
>>>>>   	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
>>>>>   	unsigned long pclk = crtc_state->mode.clock * 1000;
>>>>>   	struct tegra_hdmi *hdmi = to_hdmi(output);
>>>>> +	unsigned long rate;
>>>>>   	int err;
>>>>>
>>>>> +	rate = clk_round_rate(hdmi->clk_parent, pclk);
>>>>> +
>>>>>   	err = tegra_dc_state_setup_clock(dc, crtc_state, hdmi->clk_parent,
>>>>> -					 pclk, 0);
>>>>> +					 rate, tegra_dc_div61(rate, pclk));
>>>>>   	if (err < 0) {
>>>>>   		dev_err(output->dev, "failed to setup CRTC state: %d\n", err);
>>>>>   		return err;
>>>>> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
>>>>> index 28a78d3120bc..3ec3e13ce47f 100644
>>>>> --- a/drivers/gpu/drm/tegra/rgb.c
>>>>> +++ b/drivers/gpu/drm/tegra/rgb.c
>>>>> @@ -19,6 +19,7 @@ struct tegra_rgb {
>>>>>   	struct tegra_output output;
>>>>>   	struct tegra_dc *dc;
>>>>>
>>>>> +	struct clk *pll_d_out0;
>>>>>   	struct clk *clk_parent;
>>>>>   	struct clk *clk;
>>>>>   };
>>>>> @@ -130,6 +131,8 @@ static void tegra_rgb_encoder_disable(struct drm_encoder
>>>>> *encoder)
>>>>>
>>>>>   	if (output->panel)
>>>>>   		drm_panel_unprepare(output->panel);
>>>>> +
>>>>> +	clk_rate_exclusive_put(rgb->clk);
>>>>>   }
>>>>>
>>>>>   static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)
>>>>> @@ -165,6 +168,8 @@ static void tegra_rgb_encoder_enable(struct drm_encoder
>>>>> *encoder)
>>>>>
>>>>>   	if (output->panel)
>>>>>   		drm_panel_enable(output->panel);
>>>>> +
>>>>> +	clk_rate_exclusive_get(rgb->clk);
>>>>>   }
>>>>>
>>>>>   static int
>>>>> @@ -174,11 +179,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
>>>>>   {
>>>>>   	struct tegra_output *output = encoder_to_output(encoder);
>>>>>   	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
>>>>> -	unsigned long pclk = crtc_state->mode.clock * 1000;
>>>>>   	struct tegra_rgb *rgb = to_rgb(output);
>>>>> -	unsigned int div;
>>>>> +	unsigned long pclk = crtc_state->mode.clock * 1000;
>>>>> +	unsigned long rate;
>>>>> +	bool pll_d_parent;
>>>>>   	int err;
>>>>>
>>>>> +	if (clk_is_match(rgb->clk_parent, rgb->pll_d_out0))
>>>>> +		pll_d_parent = true;
>>>>> +	else
>>>>> +		pll_d_parent = false;
>>>>> +
>>>>>   	/*
>>>>>   	 * We may not want to change the frequency of the parent clock, since
>>>>>   	 * it may be a parent for other peripherals. This is due to the fact
>>>>> @@ -194,12 +205,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
>>>>>   	 * The best we can do at this point is to use the shift clock divider
>>>>>   	 * and hope that the desired frequency can be matched (or at least
>>>>>   	 * matched sufficiently close that the panel will still work).
>>>>> +	 *
>>>>> +	 * Make excuse for pll_d_out0 if it is explicitly set as a parent
>>>>> +	 * for display panel.
>>>>>   	 */
>>>>> -	div = ((clk_get_rate(rgb->clk) * 2) / pclk) - 2;
>>>>> -	pclk = 0;
>>>>> +	if (pll_d_parent)
>>>>> +		rate = clk_round_rate(rgb->clk_parent, pclk * 4);
>>>>> +	else
>>>>> +		rate = clk_get_rate(rgb->clk);
>>>>>
>>>>>   	err = tegra_dc_state_setup_clock(dc, crtc_state, rgb->clk_parent,
>>>>> -					 pclk, div);
>>>>> +					 rate, tegra_dc_div61(rate, pclk));
>>>>>   	if (err < 0) {
>>>>>   		dev_err(output->dev, "failed to setup CRTC state: %d\n", err);
>>>>>   		return err;
>>>>> @@ -254,6 +270,12 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
>>>>>   		return err;
>>>>>   	}
>>>>>
>>>>> +	rgb->pll_d_out0 = clk_get_sys(NULL, "pll_d_out0");
>>>>> +	if (IS_ERR(rgb->pll_d_out0)) {
>>>>> +		dev_err(dc->dev, "failed to get pll_d_out0\n");
>>>>> +		return PTR_ERR(rgb->pll_d_out0);
>>>>> +	}
>>>>> +
>>>>>   	dc->rgb = &rgb->output;
>>>>>
>>>>>   	return 0;
>>>>> @@ -265,6 +287,7 @@ int tegra_dc_rgb_remove(struct tegra_dc *dc)
>>>>>   		return 0;
>>>>>
>>>>>   	tegra_output_remove(dc->rgb);
>>>>> +	clk_put(to_rgb(dc->rgb)->pll_d_out0);
>>>>>   	dc->rgb = NULL;
>>>>>
>>>>>   	return 0;
>>>>
>>>> I've tested the patch and it works. The disp1 will run off pll_d without
>>>> any issues.
>>>>
>>>> Here is the clock tree from the downstream kernel.
>>>
>>> Okay, thank you. Now I think that it's better not to touch the display driver at
>>> all, but change the PLLC clock to 586 MHz in the clock driver. There is no
>>> appreciable difference in 600 vs 586 MHz, so that should be good enough.
>>>
>>> It would be ideal if clock rates could be specified via device tree, but that's
>>> not possible today. So just change the PLLC rate globally in the clocks
>>> init_table. Maybe later we'll manage to implement something that will allow to
>>> set rates via device tree.
>>>
>>
>> There are two variants of the display panel chip for this tablet.
>> They run at different clock rates:
>>
>>   - 68.9Mhz divided down from 586Mhz
>>   - 72Mhhz divided down from 570Mhz
>>
>> With this in consideration I don't think it makes sense to change the
>> initial pll_c rate.
> 
> The 72MHz variant will work perfectly with the PLLP as a parent. Have you tried
> to use timings and rate of the 72MHz variant?
> 
>> 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".
> 
>> 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().
> 
>> Is it not possible to go the route of your first proposed patch to check
>> if the parent of display clock is pll_p?
> 
> It is possible. If you'll decide to choose that route, then please drop the
> tegra_dc_div61() stuff as it is actually div71 and is not needed at all, I was
> just trying something using it.
> 
> I think both variants are acceptable, though let's wait for the comment from
> Thierry, as the last word will be after him.
> 
>>> Note that changing PLLC rate will makes sense only if you're going to upstream
>>> the device tree for your device. Please prepare and submit the device tree and
>>> clock patches. I think Thierry and Peter won't oppose to the variant with
>>> changing rate of PLLC, for now that should be the best option.
>>>
>>
>> I would like to eventually upstream the device tree. There are only two things
>> at the moment that are blocking this device from working on upstream kernel.
>> One is this display clock initialization issue. The other is the
>> non-standard partition table which required a patch from the Anrdoid kernels.
>> For now I am using CONFIG_CMDLINE_PARTITION to specify the partition
>> layout.
> 
> The partition layout shouldn't be a real stopper as seems there is external MMC.
> 
> IIRC, the custom partition is the ms-dos partition with a custom offsets, isn't
> it? At least some of Tegra devices also have a spare GPT partition that is
> specified by bootloader via gpt_sector argument of the kernels commandline.
> 

Indeed, both the Ouya and Moto Xoom use the gpt_sector method, however 
this does not work without a patch.
Currently this is the only actual blocker to supporting these two devices.
Does anyone know why the force gpt sector patch was never submitted to 
mainline?

On 9/3/18 2:28 PM, Peter Geis wrote:
> 
> 
> On 9/1/2018 9:54 PM, Dmitry Osipenko wrote:
>> On 9/2/18 3:27 AM, r yang wrote:
>>> On Fri, Aug 31, 2018 at 11:44:11PM +0300, Dmitry Osipenko wrote:
>>>> On 8/31/18 10:31 PM, r yang wrote:
>>>>> On Wed, Aug 29, 2018 at 08:52:37PM +0300, Dmitry Osipenko wrote:
>>>>>> On 25.08.2018 04:11, r yang wrote:
>>>>>>> On Fri, Aug 24, 2018 at 05:40:11AM +0300, Dmitry Osipenko wrote:
>>>>>>>> On Friday 24 August 2018 03:53:22 ryang wrote:
>>>>>>>>> There is a workaround in which the tegra rgb driver initializes
>>>>>>>>> the tegra dc pclk to 0 so that it will skip setting the parent clk rate.
>>>>>>>>>
>>>>>>>>> The relevant commits:
>>>>>>>>> 3cebae6737b100323baca21de6bce6647249e778
>>>>>>>>> 76d59ed049197bdaaa24c0e061a105af6ce74457
>>>>>>>>>
>>>>>>>>> A more recent commit sets the rate of the dc clk itself:
>>>>>>>>> 39e08affecf0998be1b01f4752016e33fa98eb9a
>>>>>>>>>
>>>>>>>>> This doesn't make sense because it always sets the dc clk to 0.
>>>>>>>>> Is this intended behavior or does it just happen to be working for
>>>>>>>>> the current tegra 2 boards in the mainline kernel?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> For context I am running the kernel on a tegra 2 based
>>>>>>>>> Galaxy Tab 10.1. The display panel driver is out of tree.
>>>>>>>>> This panel has very low clock rate tolerances so it must be
>>>>>>>>> driven at a rate very close to the required specification (68.75Mhz).
>>>>>>>>> pll_p is not adequate to drive this panel so pll_d must be used.
>>>>>>>>>
>>>>>>>>> Another issue is the current workaround is always forcing the disp1
>>>>>>>>> clk to zero. The Galaxy Tab 10.1 locks up completely when calling
>>>>>>>>> clk_set_rate with a clk rate of 0 whether the parent is pll_p or
>>>>>>>>> pll_d.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This patch adds a flag single_display_pll to mark that the device has
>>>>>>>>> only one display pll. This replaces the the pclk = 0 workaround.
>>>>>>>>>
>>>>>>>>> There is a comment in rgb.c about using the shift clock divider for
>>>>>>>>> tegra 2 but the divider is not set due to the has_nvdisplay flag.
>>>>>>>>> This patch also sets the shift clock divider based on the
>>>>>>>>> single_display_pll flag.
>>>>>>>>>
>>>>>>>>> A change I'm uncertain about. In tegra_dc_commit_state()
>>>>>>>>> the dc clock is now being set to the display panel rate rather than zero.
>>>>>>>>>
>>>>>>>>> I don't have any other tegra devices to test with. So I don't know
>>>>>>>>> if this breaks other devices. Previously the code was trying to set the
>>>>>>>>> clock rate to zero anyways.
>>>>>>>>>
>>>>>>>>> Signed-off-by: ryang <decatf@gmail.com>
>>>>>>>>> ---
>>>>>>>>>   drivers/gpu/drm/tegra/dc.c  | 9 +++++++--
>>>>>>>>>   drivers/gpu/drm/tegra/dc.h  | 1 +
>>>>>>>>>   drivers/gpu/drm/tegra/rgb.c | 1 -
>>>>>>>>>   3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>>>>>>>> index 965088afcfad..03f1ad630254 100644
>>>>>>>>> --- a/drivers/gpu/drm/tegra/dc.c
>>>>>>>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>>>>>>>> @@ -1664,7 +1664,7 @@ static void tegra_dc_commit_state(struct tegra_dc
>>>>>>>>> *dc, * which is shared with other peripherals. Changing the clock rate *
>>>>>>>>> should therefore be avoided.
>>>>>>>>>        */
>>>>>>>>> -    if (state->pclk > 0) {
>>>>>>>>> +    if (!dc->soc->single_display_pll) {
>>>>>>>>
>>>>>>>> We don't want to change pll_p rate here. It will be better to check whether
>>>>>>>> parent clock for disp is pll_d in tegra_rgb_encoder_atomic_check() and
>>>>>>>> if it
>>>>>>>> is the parent, then allow to propagate pclk into commit_state. We may
>>>>>>>> request
>>>>>>>> the pll_d using clk_get_sys(), then get the parent clock of disp by
>>>>>>>> clk_get_parent(disp_clk) and finally compare the parent with pll_d using
>>>>>>>> clk_is_math(parent, pll_d).
>>>>>>>
>>>>>>> Understood.
>>>>>>>
>>>>>>>>
>>>>>>>> Certainly parent clock selection could be more advanced, ideally the best
>>>>>>>> parent clock should be selected in the atomic check and applied to disp clk
>>>>>>>> on the commit by changing the disp's parent. So pll_d could be always
>>>>>>>> selected if only one display controller is active at a time, otherwise the
>>>>>>>> atomic check should resolve the parent clocks based on the required
>>>>>>>> rates. It
>>>>>>>> also could be permitable to adjust the pll_c rate.
>>>>>>>
>>>>>>> Right, the TRM describes quite a wide range of clock selection which
>>>>>>> currently
>>>>>>> aren't all possible to use on the kernel. The Galaxy Tab 10.1 stock
>>>>>>> kernel uses
>>>>>>> pll_c solely for disp1. Every clock normally on pll_c such as
>>>>>>> gr3d/gr2d/vde/etc
>>>>>>> are on pll_m. I suspect it was designed this way because the TRM says
>>>>>>> pll_d has
>>>>>>> a very power usage and that it should be avoided if possible.
>>>>>>>
>>>>>>> In absense of more advanced clock selection. It was my intention with this
>>>>>>> patch to try to at least have pll_p and pll_d working right for the internal
>>>>>>> display.
>>>>>>>
>>>>>>
>>>>>> Could you show the clock tree from downstream kernel? I'm curious what it
>>>>>> looks
>>>>>> like, that should be in "/sys/kernel/debug/clock/clock_tree".
>>>>>>
>>>>>>
>>>>>> The patch below seems to work reasonably good, both HDMI and panel either are
>>>>>> working simultaneously or only panel works when both displays are running off
>>>>>> the pll_d. Could you give it a try?
>>>>>>
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>>>>> index bdc418bcd898..c91f1203418d 100644
>>>>>> --- a/drivers/gpu/drm/tegra/dc.c
>>>>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>>>>> @@ -26,6 +26,33 @@
>>>>>>   #include <drm/drm_atomic_helper.h>
>>>>>>   #include <drm/drm_plane_helper.h>
>>>>>>
>>>>>> +unsigned int tegra_dc_div61(unsigned long  parent, unsigned long pixel)
>>>>>> +{
>>>>>> +    unsigned int rem, fract = 0;
>>>>>> +    unsigned int div61;
>>>>>> +    u64 integer;
>>>>>> +
>>>>>> +    if (parent <= pixel)
>>>>>> +        return 0;
>>>>>> +
>>>>>> +    integer = parent;
>>>>>> +    rem = do_div(integer, pixel);
>>>>>> +
>>>>>> +    if (rem > pixel / 4) {
>>>>>> +        if (rem > pixel * 2 / 3)
>>>>>> +            integer += 1;
>>>>>> +        else
>>>>>> +            fract = 1;
>>>>>> +    }
>>>>>> +
>>>>>> +    div61 = (integer << 1) | fract;
>>>>>> +
>>>>>> +    if (div61 > 255)
>>>>>> +        return 255;
>>>>>> +
>>>>>> +    return div61;
>>>>>> +}
>>>>>> +
>>>>>>   static void tegra_dc_stats_reset(struct tegra_dc_stats *stats)
>>>>>>   {
>>>>>>       stats->frames = 0;
>>>>>> diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
>>>>>> index 382d38b1524b..dc3f681ddf1f 100644
>>>>>> --- a/drivers/gpu/drm/tegra/dc.h
>>>>>> +++ b/drivers/gpu/drm/tegra/dc.h
>>>>>> @@ -174,6 +174,7 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc,
>>>>>>                      struct drm_crtc_state *crtc_state,
>>>>>>                      struct clk *clk, unsigned long pclk,
>>>>>>                      unsigned int div);
>>>>>> +unsigned int tegra_dc_div61(unsigned long  parent, unsigned long pixel);
>>>>>>
>>>>>>   /* from rgb.c */
>>>>>>   int tegra_dc_rgb_probe(struct tegra_dc *dc);
>>>>>> diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
>>>>>> index 0082468f703c..3c19af0eafe7 100644
>>>>>> --- a/drivers/gpu/drm/tegra/hdmi.c
>>>>>> +++ b/drivers/gpu/drm/tegra/hdmi.c
>>>>>> @@ -1450,10 +1450,13 @@ tegra_hdmi_encoder_atomic_check(struct drm_encoder
>>>>>> *encoder,
>>>>>>       struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
>>>>>>       unsigned long pclk = crtc_state->mode.clock * 1000;
>>>>>>       struct tegra_hdmi *hdmi = to_hdmi(output);
>>>>>> +    unsigned long rate;
>>>>>>       int err;
>>>>>>
>>>>>> +    rate = clk_round_rate(hdmi->clk_parent, pclk);
>>>>>> +
>>>>>>       err = tegra_dc_state_setup_clock(dc, crtc_state, hdmi->clk_parent,
>>>>>> -                     pclk, 0);
>>>>>> +                     rate, tegra_dc_div61(rate, pclk));
>>>>>>       if (err < 0) {
>>>>>>           dev_err(output->dev, "failed to setup CRTC state: %d\n", err);
>>>>>>           return err;
>>>>>> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
>>>>>> index 28a78d3120bc..3ec3e13ce47f 100644
>>>>>> --- a/drivers/gpu/drm/tegra/rgb.c
>>>>>> +++ b/drivers/gpu/drm/tegra/rgb.c
>>>>>> @@ -19,6 +19,7 @@ struct tegra_rgb {
>>>>>>       struct tegra_output output;
>>>>>>       struct tegra_dc *dc;
>>>>>>
>>>>>> +    struct clk *pll_d_out0;
>>>>>>       struct clk *clk_parent;
>>>>>>       struct clk *clk;
>>>>>>   };
>>>>>> @@ -130,6 +131,8 @@ static void tegra_rgb_encoder_disable(struct drm_encoder
>>>>>> *encoder)
>>>>>>
>>>>>>       if (output->panel)
>>>>>>           drm_panel_unprepare(output->panel);
>>>>>> +
>>>>>> +    clk_rate_exclusive_put(rgb->clk);
>>>>>>   }
>>>>>>
>>>>>>   static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)
>>>>>> @@ -165,6 +168,8 @@ static void tegra_rgb_encoder_enable(struct drm_encoder
>>>>>> *encoder)
>>>>>>
>>>>>>       if (output->panel)
>>>>>>           drm_panel_enable(output->panel);
>>>>>> +
>>>>>> +    clk_rate_exclusive_get(rgb->clk);
>>>>>>   }
>>>>>>
>>>>>>   static int
>>>>>> @@ -174,11 +179,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder
>>>>>> *encoder,
>>>>>>   {
>>>>>>       struct tegra_output *output = encoder_to_output(encoder);
>>>>>>       struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
>>>>>> -    unsigned long pclk = crtc_state->mode.clock * 1000;
>>>>>>       struct tegra_rgb *rgb = to_rgb(output);
>>>>>> -    unsigned int div;
>>>>>> +    unsigned long pclk = crtc_state->mode.clock * 1000;
>>>>>> +    unsigned long rate;
>>>>>> +    bool pll_d_parent;
>>>>>>       int err;
>>>>>>
>>>>>> +    if (clk_is_match(rgb->clk_parent, rgb->pll_d_out0))
>>>>>> +        pll_d_parent = true;
>>>>>> +    else
>>>>>> +        pll_d_parent = false;
>>>>>> +
>>>>>>       /*
>>>>>>        * We may not want to change the frequency of the parent clock, since
>>>>>>        * it may be a parent for other peripherals. This is due to the fact
>>>>>> @@ -194,12 +205,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder
>>>>>> *encoder,
>>>>>>        * The best we can do at this point is to use the shift clock divider
>>>>>>        * and hope that the desired frequency can be matched (or at least
>>>>>>        * matched sufficiently close that the panel will still work).
>>>>>> +     *
>>>>>> +     * Make excuse for pll_d_out0 if it is explicitly set as a parent
>>>>>> +     * for display panel.
>>>>>>        */
>>>>>> -    div = ((clk_get_rate(rgb->clk) * 2) / pclk) - 2;
>>>>>> -    pclk = 0;
>>>>>> +    if (pll_d_parent)
>>>>>> +        rate = clk_round_rate(rgb->clk_parent, pclk * 4);
>>>>>> +    else
>>>>>> +        rate = clk_get_rate(rgb->clk);
>>>>>>
>>>>>>       err = tegra_dc_state_setup_clock(dc, crtc_state, rgb->clk_parent,
>>>>>> -                     pclk, div);
>>>>>> +                     rate, tegra_dc_div61(rate, pclk));
>>>>>>       if (err < 0) {
>>>>>>           dev_err(output->dev, "failed to setup CRTC state: %d\n", err);
>>>>>>           return err;
>>>>>> @@ -254,6 +270,12 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
>>>>>>           return err;
>>>>>>       }
>>>>>>
>>>>>> +    rgb->pll_d_out0 = clk_get_sys(NULL, "pll_d_out0");
>>>>>> +    if (IS_ERR(rgb->pll_d_out0)) {
>>>>>> +        dev_err(dc->dev, "failed to get pll_d_out0\n");
>>>>>> +        return PTR_ERR(rgb->pll_d_out0);
>>>>>> +    }
>>>>>> +
>>>>>>       dc->rgb = &rgb->output;
>>>>>>
>>>>>>       return 0;
>>>>>> @@ -265,6 +287,7 @@ int tegra_dc_rgb_remove(struct tegra_dc *dc)
>>>>>>           return 0;
>>>>>>
>>>>>>       tegra_output_remove(dc->rgb);
>>>>>> +    clk_put(to_rgb(dc->rgb)->pll_d_out0);
>>>>>>       dc->rgb = NULL;
>>>>>>
>>>>>>       return 0;
>>>>>
>>>>> I've tested the patch and it works. The disp1 will run off pll_d without
>>>>> any issues.
>>>>>
>>>>> Here is the clock tree from the downstream kernel.
>>>>
>>>> Okay, thank you. Now I think that it's better not to touch the display
>>>> driver at
>>>> all, but change the PLLC clock to 586 MHz in the clock driver. There is no
>>>> appreciable difference in 600 vs 586 MHz, so that should be good enough.
>>>>
>>>> It would be ideal if clock rates could be specified via device tree, but that's
>>>> not possible today. So just change the PLLC rate globally in the clocks
>>>> init_table. Maybe later we'll manage to implement something that will allow to
>>>> set rates via device tree.
>>>>
>>>
>>> There are two variants of the display panel chip for this tablet.
>>> They run at different clock rates:
>>>
>>>   - 68.9Mhz divided down from 586Mhz
>>>   - 72Mhhz divided down from 570Mhz
>>>
>>> With this in consideration I don't think it makes sense to change the
>>> initial pll_c rate.
>>
>> The 72MHz variant will work perfectly with the PLLP as a parent. Have you tried
>> to use timings and rate of the 72MHz variant?
>>
>>> 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".
>>
>>> 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().
>>
>>> Is it not possible to go the route of your first proposed patch to check
>>> if the parent of display clock is pll_p?
>>
>> It is possible. If you'll decide to choose that route, then please drop the
>> tegra_dc_div61() stuff as it is actually div71 and is not needed at all, I was
>> just trying something using it.
>>
>> I think both variants are acceptable, though let's wait for the comment from
>> Thierry, as the last word will be after him.
>>
>>>> Note that changing PLLC rate will makes sense only if you're going to upstream
>>>> the device tree for your device. Please prepare and submit the device tree and
>>>> clock patches. I think Thierry and Peter won't oppose to the variant with
>>>> changing rate of PLLC, for now that should be the best option.
>>>>
>>>
>>> I would like to eventually upstream the device tree. There are only two things
>>> at the moment that are blocking this device from working on upstream kernel.
>>> One is this display clock initialization issue. The other is the
>>> non-standard partition table which required a patch from the Anrdoid kernels.
>>> For now I am using CONFIG_CMDLINE_PARTITION to specify the partition
>>> layout.
>>
>> The partition layout shouldn't be a real stopper as seems there is external MMC.
>>
>> IIRC, the custom partition is the ms-dos partition with a custom offsets, isn't
>> it? At least some of Tegra devices also have a spare GPT partition that is
>> specified by bootloader via gpt_sector argument of the kernels commandline.
>>
> 
> Indeed, both the Ouya and Moto Xoom use the gpt_sector method, however this does
> not work without a patch.
> Currently this is the only actual blocker to supporting these two devices.
> Does anyone know why the force gpt sector patch was never submitted to mainline?

Probably because nobody cared to do that yet.

It isn't really a blocker since you could utilize other storage devices, like
external MMC; USB drive or even NFS over usb-net, you could also use the
CONFIG_CMDLINE_PARTITION.

On Sun, Sep 02, 2018 at 04:54:09AM +0300, Dmitry Osipenko wrote:
> On 9/2/18 3:27 AM, r yang wrote:
> > On Fri, Aug 31, 2018 at 11:44:11PM +0300, Dmitry Osipenko wrote:
> >> On 8/31/18 10:31 PM, r yang wrote:
> >>> On Wed, Aug 29, 2018 at 08:52:37PM +0300, Dmitry Osipenko wrote:
> >>>> On 25.08.2018 04:11, r yang wrote:
> >>>>> On Fri, Aug 24, 2018 at 05:40:11AM +0300, Dmitry Osipenko wrote:
> >>>>>> On Friday 24 August 2018 03:53:22 ryang wrote:
> >>>>>>> There is a workaround in which the tegra rgb driver initializes
> >>>>>>> the tegra dc pclk to 0 so that it will skip setting the parent clk rate.
> >>>>>>>
> >>>>>>> The relevant commits:
> >>>>>>> 3cebae6737b100323baca21de6bce6647249e778
> >>>>>>> 76d59ed049197bdaaa24c0e061a105af6ce74457
> >>>>>>>
> >>>>>>> A more recent commit sets the rate of the dc clk itself:
> >>>>>>> 39e08affecf0998be1b01f4752016e33fa98eb9a
> >>>>>>>
> >>>>>>> This doesn't make sense because it always sets the dc clk to 0.
> >>>>>>> Is this intended behavior or does it just happen to be working for
> >>>>>>> the current tegra 2 boards in the mainline kernel?
> >>>>>>>
> >>>>>>>
> >>>>>>> For context I am running the kernel on a tegra 2 based
> >>>>>>> Galaxy Tab 10.1. The display panel driver is out of tree.
> >>>>>>> This panel has very low clock rate tolerances so it must be
> >>>>>>> driven at a rate very close to the required specification (68.75Mhz).
> >>>>>>> pll_p is not adequate to drive this panel so pll_d must be used.
> >>>>>>>
> >>>>>>> Another issue is the current workaround is always forcing the disp1
> >>>>>>> clk to zero. The Galaxy Tab 10.1 locks up completely when calling
> >>>>>>> clk_set_rate with a clk rate of 0 whether the parent is pll_p or
> >>>>>>> pll_d.
> >>>>>>>
> >>>>>>>
> >>>>>>> This patch adds a flag single_display_pll to mark that the device has
> >>>>>>> only one display pll. This replaces the the pclk = 0 workaround.
> >>>>>>>
> >>>>>>> There is a comment in rgb.c about using the shift clock divider for
> >>>>>>> tegra 2 but the divider is not set due to the has_nvdisplay flag.
> >>>>>>> This patch also sets the shift clock divider based on the
> >>>>>>> single_display_pll flag.
> >>>>>>>
> >>>>>>> A change I'm uncertain about. In tegra_dc_commit_state()
> >>>>>>> the dc clock is now being set to the display panel rate rather than zero.
> >>>>>>>
> >>>>>>> I don't have any other tegra devices to test with. So I don't know
> >>>>>>> if this breaks other devices. Previously the code was trying to set the
> >>>>>>> clock rate to zero anyways.
> >>>>>>>
> >>>>>>> Signed-off-by: ryang <decatf@gmail.com>
> >>>>>>> ---
> >>>>>>>  drivers/gpu/drm/tegra/dc.c  | 9 +++++++--
> >>>>>>>  drivers/gpu/drm/tegra/dc.h  | 1 +
> >>>>>>>  drivers/gpu/drm/tegra/rgb.c | 1 -
> >>>>>>>  3 files changed, 8 insertions(+), 3 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> >>>>>>> index 965088afcfad..03f1ad630254 100644
> >>>>>>> --- a/drivers/gpu/drm/tegra/dc.c
> >>>>>>> +++ b/drivers/gpu/drm/tegra/dc.c
> >>>>>>> @@ -1664,7 +1664,7 @@ static void tegra_dc_commit_state(struct tegra_dc
> >>>>>>> *dc, * which is shared with other peripherals. Changing the clock rate *
> >>>>>>> should therefore be avoided.
> >>>>>>>  	 */
> >>>>>>> -	if (state->pclk > 0) {
> >>>>>>> +	if (!dc->soc->single_display_pll) {
> >>>>>>
> >>>>>> We don't want to change pll_p rate here. It will be better to check whether 
> >>>>>> parent clock for disp is pll_d in tegra_rgb_encoder_atomic_check() and if it 
> >>>>>> is the parent, then allow to propagate pclk into commit_state. We may request 
> >>>>>> the pll_d using clk_get_sys(), then get the parent clock of disp by 
> >>>>>> clk_get_parent(disp_clk) and finally compare the parent with pll_d using 
> >>>>>> clk_is_math(parent, pll_d).
> >>>>>
> >>>>> Understood.
> >>>>>
> >>>>>>
> >>>>>> Certainly parent clock selection could be more advanced, ideally the best 
> >>>>>> parent clock should be selected in the atomic check and applied to disp clk 
> >>>>>> on the commit by changing the disp's parent. So pll_d could be always 
> >>>>>> selected if only one display controller is active at a time, otherwise the 
> >>>>>> atomic check should resolve the parent clocks based on the required rates. It 
> >>>>>> also could be permitable to adjust the pll_c rate.
> >>>>>
> >>>>> Right, the TRM describes quite a wide range of clock selection which currently
> >>>>> aren't all possible to use on the kernel. The Galaxy Tab 10.1 stock kernel uses
> >>>>> pll_c solely for disp1. Every clock normally on pll_c such as gr3d/gr2d/vde/etc
> >>>>> are on pll_m. I suspect it was designed this way because the TRM says pll_d has
> >>>>> a very power usage and that it should be avoided if possible.
> >>>>>
> >>>>> In absense of more advanced clock selection. It was my intention with this
> >>>>> patch to try to at least have pll_p and pll_d working right for the internal
> >>>>> display.
> >>>>>
> >>>>
> >>>> Could you show the clock tree from downstream kernel? I'm curious what it looks
> >>>> like, that should be in "/sys/kernel/debug/clock/clock_tree".
> >>>>
> >>>>
> >>>> The patch below seems to work reasonably good, both HDMI and panel either are
> >>>> working simultaneously or only panel works when both displays are running off
> >>>> the pll_d. Could you give it a try?
> >>>>
> >>>>
> >>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> >>>> index bdc418bcd898..c91f1203418d 100644
> >>>> --- a/drivers/gpu/drm/tegra/dc.c
> >>>> +++ b/drivers/gpu/drm/tegra/dc.c
> >>>> @@ -26,6 +26,33 @@
> >>>>  #include <drm/drm_atomic_helper.h>
> >>>>  #include <drm/drm_plane_helper.h>
> >>>>
> >>>> +unsigned int tegra_dc_div61(unsigned long  parent, unsigned long pixel)
> >>>> +{
> >>>> +	unsigned int rem, fract = 0;
> >>>> +	unsigned int div61;
> >>>> +	u64 integer;
> >>>> +
> >>>> +	if (parent <= pixel)
> >>>> +		return 0;
> >>>> +
> >>>> +	integer = parent;
> >>>> +	rem = do_div(integer, pixel);
> >>>> +
> >>>> +	if (rem > pixel / 4) {
> >>>> +		if (rem > pixel * 2 / 3)
> >>>> +			integer += 1;
> >>>> +		else
> >>>> +			fract = 1;
> >>>> +	}
> >>>> +
> >>>> +	div61 = (integer << 1) | fract;
> >>>> +
> >>>> +	if (div61 > 255)
> >>>> +		return 255;
> >>>> +
> >>>> +	return div61;
> >>>> +}
> >>>> +
> >>>>  static void tegra_dc_stats_reset(struct tegra_dc_stats *stats)
> >>>>  {
> >>>>  	stats->frames = 0;
> >>>> diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
> >>>> index 382d38b1524b..dc3f681ddf1f 100644
> >>>> --- a/drivers/gpu/drm/tegra/dc.h
> >>>> +++ b/drivers/gpu/drm/tegra/dc.h
> >>>> @@ -174,6 +174,7 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc,
> >>>>  			       struct drm_crtc_state *crtc_state,
> >>>>  			       struct clk *clk, unsigned long pclk,
> >>>>  			       unsigned int div);
> >>>> +unsigned int tegra_dc_div61(unsigned long  parent, unsigned long pixel);
> >>>>
> >>>>  /* from rgb.c */
> >>>>  int tegra_dc_rgb_probe(struct tegra_dc *dc);
> >>>> diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
> >>>> index 0082468f703c..3c19af0eafe7 100644
> >>>> --- a/drivers/gpu/drm/tegra/hdmi.c
> >>>> +++ b/drivers/gpu/drm/tegra/hdmi.c
> >>>> @@ -1450,10 +1450,13 @@ tegra_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
> >>>>  	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
> >>>>  	unsigned long pclk = crtc_state->mode.clock * 1000;
> >>>>  	struct tegra_hdmi *hdmi = to_hdmi(output);
> >>>> +	unsigned long rate;
> >>>>  	int err;
> >>>>
> >>>> +	rate = clk_round_rate(hdmi->clk_parent, pclk);
> >>>> +
> >>>>  	err = tegra_dc_state_setup_clock(dc, crtc_state, hdmi->clk_parent,
> >>>> -					 pclk, 0);
> >>>> +					 rate, tegra_dc_div61(rate, pclk));
> >>>>  	if (err < 0) {
> >>>>  		dev_err(output->dev, "failed to setup CRTC state: %d\n", err);
> >>>>  		return err;
> >>>> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
> >>>> index 28a78d3120bc..3ec3e13ce47f 100644
> >>>> --- a/drivers/gpu/drm/tegra/rgb.c
> >>>> +++ b/drivers/gpu/drm/tegra/rgb.c
> >>>> @@ -19,6 +19,7 @@ struct tegra_rgb {
> >>>>  	struct tegra_output output;
> >>>>  	struct tegra_dc *dc;
> >>>>
> >>>> +	struct clk *pll_d_out0;
> >>>>  	struct clk *clk_parent;
> >>>>  	struct clk *clk;
> >>>>  };
> >>>> @@ -130,6 +131,8 @@ static void tegra_rgb_encoder_disable(struct drm_encoder
> >>>> *encoder)
> >>>>
> >>>>  	if (output->panel)
> >>>>  		drm_panel_unprepare(output->panel);
> >>>> +
> >>>> +	clk_rate_exclusive_put(rgb->clk);
> >>>>  }
> >>>>
> >>>>  static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)
> >>>> @@ -165,6 +168,8 @@ static void tegra_rgb_encoder_enable(struct drm_encoder
> >>>> *encoder)
> >>>>
> >>>>  	if (output->panel)
> >>>>  		drm_panel_enable(output->panel);
> >>>> +
> >>>> +	clk_rate_exclusive_get(rgb->clk);
> >>>>  }
> >>>>
> >>>>  static int
> >>>> @@ -174,11 +179,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
> >>>>  {
> >>>>  	struct tegra_output *output = encoder_to_output(encoder);
> >>>>  	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
> >>>> -	unsigned long pclk = crtc_state->mode.clock * 1000;
> >>>>  	struct tegra_rgb *rgb = to_rgb(output);
> >>>> -	unsigned int div;
> >>>> +	unsigned long pclk = crtc_state->mode.clock * 1000;
> >>>> +	unsigned long rate;
> >>>> +	bool pll_d_parent;
> >>>>  	int err;
> >>>>
> >>>> +	if (clk_is_match(rgb->clk_parent, rgb->pll_d_out0))
> >>>> +		pll_d_parent = true;
> >>>> +	else
> >>>> +		pll_d_parent = false;
> >>>> +
> >>>>  	/*
> >>>>  	 * We may not want to change the frequency of the parent clock, since
> >>>>  	 * it may be a parent for other peripherals. This is due to the fact
> >>>> @@ -194,12 +205,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
> >>>>  	 * The best we can do at this point is to use the shift clock divider
> >>>>  	 * and hope that the desired frequency can be matched (or at least
> >>>>  	 * matched sufficiently close that the panel will still work).
> >>>> +	 *
> >>>> +	 * Make excuse for pll_d_out0 if it is explicitly set as a parent
> >>>> +	 * for display panel.
> >>>>  	 */
> >>>> -	div = ((clk_get_rate(rgb->clk) * 2) / pclk) - 2;
> >>>> -	pclk = 0;
> >>>> +	if (pll_d_parent)
> >>>> +		rate = clk_round_rate(rgb->clk_parent, pclk * 4);
> >>>> +	else
> >>>> +		rate = clk_get_rate(rgb->clk);
> >>>>
> >>>>  	err = tegra_dc_state_setup_clock(dc, crtc_state, rgb->clk_parent,
> >>>> -					 pclk, div);
> >>>> +					 rate, tegra_dc_div61(rate, pclk));
> >>>>  	if (err < 0) {
> >>>>  		dev_err(output->dev, "failed to setup CRTC state: %d\n", err);
> >>>>  		return err;
> >>>> @@ -254,6 +270,12 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
> >>>>  		return err;
> >>>>  	}
> >>>>
> >>>> +	rgb->pll_d_out0 = clk_get_sys(NULL, "pll_d_out0");
> >>>> +	if (IS_ERR(rgb->pll_d_out0)) {
> >>>> +		dev_err(dc->dev, "failed to get pll_d_out0\n");
> >>>> +		return PTR_ERR(rgb->pll_d_out0);
> >>>> +	}
> >>>> +
> >>>>  	dc->rgb = &rgb->output;
> >>>>
> >>>>  	return 0;
> >>>> @@ -265,6 +287,7 @@ int tegra_dc_rgb_remove(struct tegra_dc *dc)
> >>>>  		return 0;
> >>>>
> >>>>  	tegra_output_remove(dc->rgb);
> >>>> +	clk_put(to_rgb(dc->rgb)->pll_d_out0);
> >>>>  	dc->rgb = NULL;
> >>>>
> >>>>  	return 0;
> >>>
> >>> I've tested the patch and it works. The disp1 will run off pll_d without
> >>> any issues.
> >>>
> >>> Here is the clock tree from the downstream kernel.
> >>
> >> Okay, thank you. Now I think that it's better not to touch the display driver at
> >> all, but change the PLLC clock to 586 MHz in the clock driver. There is no
> >> appreciable difference in 600 vs 586 MHz, so that should be good enough.
> >>
> >> It would be ideal if clock rates could be specified via device tree, but that's
> >> not possible today. So just change the PLLC rate globally in the clocks
> >> init_table. Maybe later we'll manage to implement something that will allow to
> >> set rates via device tree.
> >>
> > 
> > There are two variants of the display panel chip for this tablet.
> > They run at different clock rates:
> > 
> >  - 68.9Mhz divided down from 586Mhz
> >  - 72Mhhz divided down from 570Mhz
> > 
> > With this in consideration I don't think it makes sense to change the
> > initial pll_c rate.
> 
> The 72MHz variant will work perfectly with the PLLP as a parent. Have you tried
> to use timings and rate of the 72MHz variant?

Sorry, I had it wrong. It is 76Mhz. Not 72Mhz.
I have spent a great deal of time trying to find alternate clock
configuration for the display panel that can work. The only ways are:

 - Run disp1 on pll_d
 - Dedicate pll_c to disp1. Move all gpu clocks and sclk/hclk/ahbdma to pll_m.

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.

> 
> > 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.

> > Is it not possible to go the route of your first proposed patch to check
> > if the parent of display clock is pll_p?
> 
> It is possible. If you'll decide to choose that route, then please drop the
> tegra_dc_div61() stuff as it is actually div71 and is not needed at all, I was
> just trying something using it.
> 
> I think both variants are acceptable, though let's wait for the comment from
> Thierry, as the last word will be after him.

Okay. It appears this route will have a minimal impact.
It would fix that existing workaround for Tegra 2 allowing the
pll_d clock rate to be set properly for the actual pclk rate.

In the future if there is more advanced clock selection then this
workaround would possibly go away all together.

> 
> >> Note that changing PLLC rate will makes sense only if you're going to upstream
> >> the device tree for your device. Please prepare and submit the device tree and
> >> clock patches. I think Thierry and Peter won't oppose to the variant with
> >> changing rate of PLLC, for now that should be the best option.
> >>
> > 
> > I would like to eventually upstream the device tree. There are only two things
> > at the moment that are blocking this device from working on upstream kernel.
> > One is this display clock initialization issue. The other is the
> > non-standard partition table which required a patch from the Anrdoid kernels.
> > For now I am using CONFIG_CMDLINE_PARTITION to specify the partition
> > layout.
> 
> The partition layout shouldn't be a real stopper as seems there is external MMC.
> 
> IIRC, the custom partition is the ms-dos partition with a custom offsets, isn't
> it? At least some of Tegra devices also have a spare GPT partition that is
> specified by bootloader via gpt_sector argument of the kernels commandline.

Yes, this device has the custom offsets. It is same as the many other Tegra 2/3
devices.


Here is the clock tree from upstream kernel.

                                 enable  prepare  protect                               
   clock                          count    count    count        rate   accuracy   phase
----------------------------------------------------------------------------------------
 clock                                0        0        0       32768          0 0  
 clk_32k                              3        3        0       32768          0 0  
    blink_override                    1        1        0       32768          0 0  
       blink                          1        1        0       32768          0 0  
    kbc                               1        1        0       32768          0 0  
    rtc                               2        2        0       32768          0 0  
 clk_m                                4        8        0    26000000          0 0  
    csus                              0        0        0    26000000          0 0  
    usb3                              1        1        0    26000000          0 0  
    usb2                              0        0        0    26000000          0 0  
    usbd                              1        1        0    26000000          0 0  
    bsev                              0        0        0    26000000          0 0  
    bsea                              0        0        0    26000000          0 0  
    kfuse                             0        0        0    26000000          0 0  
    fuse                              0        0        0    26000000          0 0  
    vcp                               0        0        0    26000000          0 0  
    isp                               0        0        0    26000000          0 0  
    timer                             2        2        0    26000000          0 0  
    ndflash                           0        0        0    26000000          0 0  
    tvdac                             0        0        0    26000000          0 0  
    tvo                               0        0        0    26000000          0 0  
    cve                               0        0        0    26000000          0 0  
    mipi                              0        0        0    26000000          0 0  
    nor                               0        0        0    26000000          0 0  
    owr                               0        0        0    26000000          0 0  
    la                                0        0        0    26000000          0 0  
    sdmmc2                            0        0        0    26000000          0 0  
    vfir                              0        0        0    26000000          0 0  
    spdif_out                         0        0        0    26000000          0 0  
    pwm                               0        0        0    26000000          0 0  
    hdmi                              0        0        0    26000000          0 0  
    i2c3                              0        1        0     2888889          0 0  
    i2c2                              0        1        0      787879          0 0  
    i2c1                              0        1        0     2888889          0 0  
    dvc                               0        1        0     2888889          0 0  
    ide                               0        0        0    26000000          0 0  
    twc                               0        0        0    26000000          0 0  
    xio                               0        0        0    26000000          0 0  
    dev2_osc_div                      0        0        0    26000000          0 0  
    dev1_osc_div                      0        0        0    26000000          0 0  
    pex                               0        0        0    26000000          0 0  
    pll_ref                           5        5        0    26000000          0 0  
       pll_p                         10       10        0   216000000          0 0  
          cclk                        3        3        0   216000000          0 0  
             twd                      1        1        0    54000000          0 0  
          disp2                       0        0        0   216000000          0 0  
          sbc4                        0        0        0    86400000          0 0  
          sbc3                        0        0        0    86400000          0 0  
          sbc2                        0        0        0    86400000          0 0  
          sbc1                        0        0        0    86400000          0 0  
          spi                         0        0        0    19636364          0 0  
          sdmmc3                      0        0        0    48000000          0 0  
          sdmmc1                      1        1        0    48000000          0 0  
          uarte                       0        0        0   216000000          0 0  
          uartc                       2        2        0   216000000          0 0  
          uarta                       0        0        0   216000000          0 0  
          csite                       1        1        0   144000000          0 0  
          sdmmc4                      1        1        0    48000000          0 0  
          spdif_in                    0        0        0    36000000          0 0  
          pll_p_out4_div              1        1        0    24000000          0 0  
             pll_p_out4               1        1        0    24000000          0 0  
                cdev2_mux             0        0        0    24000000          0 0  
                   cdev2              0        0        0    24000000          0 0  
          pll_p_out3_div              1        1        0    72000000          0 0  
             pll_p_out3               1        5        0    72000000          0 0  
                csi                   0        0        0    72000000          0 0  
          pll_p_out2_div              1        1        0    48000000          0 0  
             pll_p_out2               1        1        0    48000000          0 0  
          pll_p_out1_div              1        1        0    28800000          0 0  
             pll_p_out1               2        2        0    28800000          0 0  
                pll_a                 2        2        0    56448000          0 0  
                   pll_a_out0_div       1        1        0    11289600          0 0  
                      pll_a_out0       2        2        0    11289600          0 0  
                         cdev1_mux       1        1        0    11289600          0 0  
                            cdev1       1        1        0    11289600          0 0  
                         i2s2         0        0        0    11289600          0 0  
                         i2s1         0        0        0    11289600          0 0  
                         audio_mux       0        0        0    11289600          0 0  
                            audio       0        0        0    11289600          0 0  
                               audio_doubler       0        0        0    22579200          0 0  
                                  audio_2x       0        0        0    22579200          0 0  
                         ac97         0        0        0    11289600          0 0  
          uartb                       0        0        0   216000000          0 0  
          uartd                       0        0        0   216000000          0 0  
       pll_e                          0        0        0   216666666          0 0  
       pll_d                          1        1        0   137500000          0 0  
          dsi                         0        0        0   137500000          0 0  
          pll_d_out0                  1        1        0    68750000          0 0  
             disp1                    1        1        0    68750000          0 0  
       pll_u                          2        2        0   480000000          0 0  
       pll_x                          0        0        0   312000000          0 0  
       pll_m                          1        1        0   600000000          0 0  
          vi_sensor                   0        0        0   100000000          0 0  
          mpe                         0        0        0   100000000          0 0  
          epp                         0        0        0   100000000          0 0  
          vi                          0        0        0   100000000          0 0  
          emc_mux                     1        1        0   600000000          0 0  
             emc                      1        1        0   600000000          0 0  
             mc                       0        0        0   300000000          0 0  
          pll_m_out1_div              0        0        0   240000000          0 0  
             pll_m_out1               0        0        0   240000000          0 0  
       pll_c                          4        4        0   600000000          0 0  
          3d                          1        1        0   300000000          0 0  
          2d                          1        1        0   300000000          0 0  
          host1x                      1        1        0   150000000          0 0  
          vde                         0        0        0   300000000          0 0  
          pll_c_out1_div              1        1        0   240000000          0 0  
             pll_c_out1               1        1        0   240000000          0 0  
                sclk                  2        2        0   240000000          0 0  
                   hclk_div           1        1        0   240000000          0 0  
                      hclk            2        2        0   240000000          0 0  
                         ahbdma       0        0        0   240000000          0 0  
                         pclk_div       1        1        0    60000000          0 0  
                            pclk       2        2        0    60000000          0 0  
                               apbdma       1        1        0    60000000          0 0  
 afi                                  0        0        0           0          0 0

On 9/3/18 7:49 PM, r yang wrote:
> On Sun, Sep 02, 2018 at 04:54:09AM +0300, Dmitry Osipenko wrote:
>> On 9/2/18 3:27 AM, r yang wrote:
>>> On Fri, Aug 31, 2018 at 11:44:11PM +0300, Dmitry Osipenko wrote:
>>>> On 8/31/18 10:31 PM, r yang wrote:
>>>>> On Wed, Aug 29, 2018 at 08:52:37PM +0300, Dmitry Osipenko wrote:
>>>>>> On 25.08.2018 04:11, r yang wrote:
>>>>>>> On Fri, Aug 24, 2018 at 05:40:11AM +0300, Dmitry Osipenko wrote:
>>>>>>>> On Friday 24 August 2018 03:53:22 ryang wrote:
>>>>>>>>> There is a workaround in which the tegra rgb driver initializes
>>>>>>>>> the tegra dc pclk to 0 so that it will skip setting the parent clk rate.
>>>>>>>>>
>>>>>>>>> The relevant commits:
>>>>>>>>> 3cebae6737b100323baca21de6bce6647249e778
>>>>>>>>> 76d59ed049197bdaaa24c0e061a105af6ce74457
>>>>>>>>>
>>>>>>>>> A more recent commit sets the rate of the dc clk itself:
>>>>>>>>> 39e08affecf0998be1b01f4752016e33fa98eb9a
>>>>>>>>>
>>>>>>>>> This doesn't make sense because it always sets the dc clk to 0.
>>>>>>>>> Is this intended behavior or does it just happen to be working for
>>>>>>>>> the current tegra 2 boards in the mainline kernel?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> For context I am running the kernel on a tegra 2 based
>>>>>>>>> Galaxy Tab 10.1. The display panel driver is out of tree.
>>>>>>>>> This panel has very low clock rate tolerances so it must be
>>>>>>>>> driven at a rate very close to the required specification (68.75Mhz).
>>>>>>>>> pll_p is not adequate to drive this panel so pll_d must be used.
>>>>>>>>>
>>>>>>>>> Another issue is the current workaround is always forcing the disp1
>>>>>>>>> clk to zero. The Galaxy Tab 10.1 locks up completely when calling
>>>>>>>>> clk_set_rate with a clk rate of 0 whether the parent is pll_p or
>>>>>>>>> pll_d.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This patch adds a flag single_display_pll to mark that the device has
>>>>>>>>> only one display pll. This replaces the the pclk = 0 workaround.
>>>>>>>>>
>>>>>>>>> There is a comment in rgb.c about using the shift clock divider for
>>>>>>>>> tegra 2 but the divider is not set due to the has_nvdisplay flag.
>>>>>>>>> This patch also sets the shift clock divider based on the
>>>>>>>>> single_display_pll flag.
>>>>>>>>>
>>>>>>>>> A change I'm uncertain about. In tegra_dc_commit_state()
>>>>>>>>> the dc clock is now being set to the display panel rate rather than zero.
>>>>>>>>>
>>>>>>>>> I don't have any other tegra devices to test with. So I don't know
>>>>>>>>> if this breaks other devices. Previously the code was trying to set the
>>>>>>>>> clock rate to zero anyways.
>>>>>>>>>
>>>>>>>>> Signed-off-by: ryang <decatf@gmail.com>
>>>>>>>>> ---
>>>>>>>>>   drivers/gpu/drm/tegra/dc.c  | 9 +++++++--
>>>>>>>>>   drivers/gpu/drm/tegra/dc.h  | 1 +
>>>>>>>>>   drivers/gpu/drm/tegra/rgb.c | 1 -
>>>>>>>>>   3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>>>>>>>> index 965088afcfad..03f1ad630254 100644
>>>>>>>>> --- a/drivers/gpu/drm/tegra/dc.c
>>>>>>>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>>>>>>>> @@ -1664,7 +1664,7 @@ static void tegra_dc_commit_state(struct tegra_dc
>>>>>>>>> *dc, * which is shared with other peripherals. Changing the clock rate *
>>>>>>>>> should therefore be avoided.
>>>>>>>>>   	 */
>>>>>>>>> -	if (state->pclk > 0) {
>>>>>>>>> +	if (!dc->soc->single_display_pll) {
>>>>>>>>
>>>>>>>> We don't want to change pll_p rate here. It will be better to check whether
>>>>>>>> parent clock for disp is pll_d in tegra_rgb_encoder_atomic_check() and if it
>>>>>>>> is the parent, then allow to propagate pclk into commit_state. We may request
>>>>>>>> the pll_d using clk_get_sys(), then get the parent clock of disp by
>>>>>>>> clk_get_parent(disp_clk) and finally compare the parent with pll_d using
>>>>>>>> clk_is_math(parent, pll_d).
>>>>>>>
>>>>>>> Understood.
>>>>>>>
>>>>>>>>
>>>>>>>> Certainly parent clock selection could be more advanced, ideally the best
>>>>>>>> parent clock should be selected in the atomic check and applied to disp clk
>>>>>>>> on the commit by changing the disp's parent. So pll_d could be always
>>>>>>>> selected if only one display controller is active at a time, otherwise the
>>>>>>>> atomic check should resolve the parent clocks based on the required rates. It
>>>>>>>> also could be permitable to adjust the pll_c rate.
>>>>>>>
>>>>>>> Right, the TRM describes quite a wide range of clock selection which currently
>>>>>>> aren't all possible to use on the kernel. The Galaxy Tab 10.1 stock kernel uses
>>>>>>> pll_c solely for disp1. Every clock normally on pll_c such as gr3d/gr2d/vde/etc
>>>>>>> are on pll_m. I suspect it was designed this way because the TRM says pll_d has
>>>>>>> a very power usage and that it should be avoided if possible.
>>>>>>>
>>>>>>> In absense of more advanced clock selection. It was my intention with this
>>>>>>> patch to try to at least have pll_p and pll_d working right for the internal
>>>>>>> display.
>>>>>>>
>>>>>>
>>>>>> Could you show the clock tree from downstream kernel? I'm curious what it looks
>>>>>> like, that should be in "/sys/kernel/debug/clock/clock_tree".
>>>>>>
>>>>>>
>>>>>> The patch below seems to work reasonably good, both HDMI and panel either are
>>>>>> working simultaneously or only panel works when both displays are running off
>>>>>> the pll_d. Could you give it a try?
>>>>>>
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>>>>> index bdc418bcd898..c91f1203418d 100644
>>>>>> --- a/drivers/gpu/drm/tegra/dc.c
>>>>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>>>>> @@ -26,6 +26,33 @@
>>>>>>   #include <drm/drm_atomic_helper.h>
>>>>>>   #include <drm/drm_plane_helper.h>
>>>>>>
>>>>>> +unsigned int tegra_dc_div61(unsigned long  parent, unsigned long pixel)
>>>>>> +{
>>>>>> +	unsigned int rem, fract = 0;
>>>>>> +	unsigned int div61;
>>>>>> +	u64 integer;
>>>>>> +
>>>>>> +	if (parent <= pixel)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	integer = parent;
>>>>>> +	rem = do_div(integer, pixel);
>>>>>> +
>>>>>> +	if (rem > pixel / 4) {
>>>>>> +		if (rem > pixel * 2 / 3)
>>>>>> +			integer += 1;
>>>>>> +		else
>>>>>> +			fract = 1;
>>>>>> +	}
>>>>>> +
>>>>>> +	div61 = (integer << 1) | fract;
>>>>>> +
>>>>>> +	if (div61 > 255)
>>>>>> +		return 255;
>>>>>> +
>>>>>> +	return div61;
>>>>>> +}
>>>>>> +
>>>>>>   static void tegra_dc_stats_reset(struct tegra_dc_stats *stats)
>>>>>>   {
>>>>>>   	stats->frames = 0;
>>>>>> diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
>>>>>> index 382d38b1524b..dc3f681ddf1f 100644
>>>>>> --- a/drivers/gpu/drm/tegra/dc.h
>>>>>> +++ b/drivers/gpu/drm/tegra/dc.h
>>>>>> @@ -174,6 +174,7 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc,
>>>>>>   			       struct drm_crtc_state *crtc_state,
>>>>>>   			       struct clk *clk, unsigned long pclk,
>>>>>>   			       unsigned int div);
>>>>>> +unsigned int tegra_dc_div61(unsigned long  parent, unsigned long pixel);
>>>>>>
>>>>>>   /* from rgb.c */
>>>>>>   int tegra_dc_rgb_probe(struct tegra_dc *dc);
>>>>>> diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
>>>>>> index 0082468f703c..3c19af0eafe7 100644
>>>>>> --- a/drivers/gpu/drm/tegra/hdmi.c
>>>>>> +++ b/drivers/gpu/drm/tegra/hdmi.c
>>>>>> @@ -1450,10 +1450,13 @@ tegra_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
>>>>>>   	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
>>>>>>   	unsigned long pclk = crtc_state->mode.clock * 1000;
>>>>>>   	struct tegra_hdmi *hdmi = to_hdmi(output);
>>>>>> +	unsigned long rate;
>>>>>>   	int err;
>>>>>>
>>>>>> +	rate = clk_round_rate(hdmi->clk_parent, pclk);
>>>>>> +
>>>>>>   	err = tegra_dc_state_setup_clock(dc, crtc_state, hdmi->clk_parent,
>>>>>> -					 pclk, 0);
>>>>>> +					 rate, tegra_dc_div61(rate, pclk));
>>>>>>   	if (err < 0) {
>>>>>>   		dev_err(output->dev, "failed to setup CRTC state: %d\n", err);
>>>>>>   		return err;
>>>>>> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
>>>>>> index 28a78d3120bc..3ec3e13ce47f 100644
>>>>>> --- a/drivers/gpu/drm/tegra/rgb.c
>>>>>> +++ b/drivers/gpu/drm/tegra/rgb.c
>>>>>> @@ -19,6 +19,7 @@ struct tegra_rgb {
>>>>>>   	struct tegra_output output;
>>>>>>   	struct tegra_dc *dc;
>>>>>>
>>>>>> +	struct clk *pll_d_out0;
>>>>>>   	struct clk *clk_parent;
>>>>>>   	struct clk *clk;
>>>>>>   };
>>>>>> @@ -130,6 +131,8 @@ static void tegra_rgb_encoder_disable(struct drm_encoder
>>>>>> *encoder)
>>>>>>
>>>>>>   	if (output->panel)
>>>>>>   		drm_panel_unprepare(output->panel);
>>>>>> +
>>>>>> +	clk_rate_exclusive_put(rgb->clk);
>>>>>>   }
>>>>>>
>>>>>>   static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)
>>>>>> @@ -165,6 +168,8 @@ static void tegra_rgb_encoder_enable(struct drm_encoder
>>>>>> *encoder)
>>>>>>
>>>>>>   	if (output->panel)
>>>>>>   		drm_panel_enable(output->panel);
>>>>>> +
>>>>>> +	clk_rate_exclusive_get(rgb->clk);
>>>>>>   }
>>>>>>
>>>>>>   static int
>>>>>> @@ -174,11 +179,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
>>>>>>   {
>>>>>>   	struct tegra_output *output = encoder_to_output(encoder);
>>>>>>   	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
>>>>>> -	unsigned long pclk = crtc_state->mode.clock * 1000;
>>>>>>   	struct tegra_rgb *rgb = to_rgb(output);
>>>>>> -	unsigned int div;
>>>>>> +	unsigned long pclk = crtc_state->mode.clock * 1000;
>>>>>> +	unsigned long rate;
>>>>>> +	bool pll_d_parent;
>>>>>>   	int err;
>>>>>>
>>>>>> +	if (clk_is_match(rgb->clk_parent, rgb->pll_d_out0))
>>>>>> +		pll_d_parent = true;
>>>>>> +	else
>>>>>> +		pll_d_parent = false;
>>>>>> +
>>>>>>   	/*
>>>>>>   	 * We may not want to change the frequency of the parent clock, since
>>>>>>   	 * it may be a parent for other peripherals. This is due to the fact
>>>>>> @@ -194,12 +205,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
>>>>>>   	 * The best we can do at this point is to use the shift clock divider
>>>>>>   	 * and hope that the desired frequency can be matched (or at least
>>>>>>   	 * matched sufficiently close that the panel will still work).
>>>>>> +	 *
>>>>>> +	 * Make excuse for pll_d_out0 if it is explicitly set as a parent
>>>>>> +	 * for display panel.
>>>>>>   	 */
>>>>>> -	div = ((clk_get_rate(rgb->clk) * 2) / pclk) - 2;
>>>>>> -	pclk = 0;
>>>>>> +	if (pll_d_parent)
>>>>>> +		rate = clk_round_rate(rgb->clk_parent, pclk * 4);
>>>>>> +	else
>>>>>> +		rate = clk_get_rate(rgb->clk);
>>>>>>
>>>>>>   	err = tegra_dc_state_setup_clock(dc, crtc_state, rgb->clk_parent,
>>>>>> -					 pclk, div);
>>>>>> +					 rate, tegra_dc_div61(rate, pclk));
>>>>>>   	if (err < 0) {
>>>>>>   		dev_err(output->dev, "failed to setup CRTC state: %d\n", err);
>>>>>>   		return err;
>>>>>> @@ -254,6 +270,12 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
>>>>>>   		return err;
>>>>>>   	}
>>>>>>
>>>>>> +	rgb->pll_d_out0 = clk_get_sys(NULL, "pll_d_out0");
>>>>>> +	if (IS_ERR(rgb->pll_d_out0)) {
>>>>>> +		dev_err(dc->dev, "failed to get pll_d_out0\n");
>>>>>> +		return PTR_ERR(rgb->pll_d_out0);
>>>>>> +	}
>>>>>> +
>>>>>>   	dc->rgb = &rgb->output;
>>>>>>
>>>>>>   	return 0;
>>>>>> @@ -265,6 +287,7 @@ int tegra_dc_rgb_remove(struct tegra_dc *dc)
>>>>>>   		return 0;
>>>>>>
>>>>>>   	tegra_output_remove(dc->rgb);
>>>>>> +	clk_put(to_rgb(dc->rgb)->pll_d_out0);
>>>>>>   	dc->rgb = NULL;
>>>>>>
>>>>>>   	return 0;
>>>>>
>>>>> I've tested the patch and it works. The disp1 will run off pll_d without
>>>>> any issues.
>>>>>
>>>>> Here is the clock tree from the downstream kernel.
>>>>
>>>> Okay, thank you. Now I think that it's better not to touch the display driver at
>>>> all, but change the PLLC clock to 586 MHz in the clock driver. There is no
>>>> appreciable difference in 600 vs 586 MHz, so that should be good enough.
>>>>
>>>> It would be ideal if clock rates could be specified via device tree, but that's
>>>> not possible today. So just change the PLLC rate globally in the clocks
>>>> init_table. Maybe later we'll manage to implement something that will allow to
>>>> set rates via device tree.
>>>>
>>>
>>> There are two variants of the display panel chip for this tablet.
>>> They run at different clock rates:
>>>
>>>   - 68.9Mhz divided down from 586Mhz
>>>   - 72Mhhz divided down from 570Mhz
>>>
>>> With this in consideration I don't think it makes sense to change the
>>> initial pll_c rate.
>>
>> The 72MHz variant will work perfectly with the PLLP as a parent. Have you tried
>> to use timings and rate of the 72MHz variant?
> 
> Sorry, I had it wrong. It is 76Mhz. Not 72Mhz.
> I have spent a great deal of time trying to find alternate clock
> configuration for the display panel that can work. The only ways are:
> 
>   - Run disp1 on pll_d
>   - Dedicate pll_c to disp1. Move all gpu clocks and sclk/hclk/ahbdma to pll_m.

PLL_M is kinda reserved for the memory, hence its rate can't be changed.

> 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.

>>> Is it not possible to go the route of your first proposed patch to check
>>> if the parent of display clock is pll_p?
>>
>> It is possible. If you'll decide to choose that route, then please drop the
>> tegra_dc_div61() stuff as it is actually div71 and is not needed at all, I was
>> just trying something using it.
>>
>> I think both variants are acceptable, though let's wait for the comment from
>> Thierry, as the last word will be after him.
> 
> Okay. It appears this route will have a minimal impact.
> It would fix that existing workaround for Tegra 2 allowing the
> pll_d clock rate to be set properly for the actual pclk rate.
> 
> In the future if there is more advanced clock selection then this
> workaround would possibly go away all together.

I don't mind. In the end it's up to Thierry to decide, so will see.

>>
>>>> Note that changing PLLC rate will makes sense only if you're going to upstream
>>>> the device tree for your device. Please prepare and submit the device tree and
>>>> clock patches. I think Thierry and Peter won't oppose to the variant with
>>>> changing rate of PLLC, for now that should be the best option.
>>>>
>>>
>>> I would like to eventually upstream the device tree. There are only two things
>>> at the moment that are blocking this device from working on upstream kernel.
>>> One is this display clock initialization issue. The other is the
>>> non-standard partition table which required a patch from the Anrdoid kernels.
>>> For now I am using CONFIG_CMDLINE_PARTITION to specify the partition
>>> layout.
>>
>> The partition layout shouldn't be a real stopper as seems there is external MMC.
>>
>> IIRC, the custom partition is the ms-dos partition with a custom offsets, isn't
>> it? At least some of Tegra devices also have a spare GPT partition that is
>> specified by bootloader via gpt_sector argument of the kernels commandline.
> 
> Yes, this device has the custom offsets. It is same as the many other Tegra 2/3
> devices.

Okay.

> 
> Here is the clock tree from upstream kernel.

Looks good, thank you.

> >
> >Sorry, I had it wrong. It is 76Mhz. Not 72Mhz.
> >I have spent a great deal of time trying to find alternate clock
> >configuration for the display panel that can work. The only ways are:
> >
> >  - Run disp1 on pll_d
> >  - Dedicate pll_c to disp1. Move all gpu clocks and sclk/hclk/ahbdma to pll_m.
> 
> PLL_M is kinda reserved for the memory, hence its rate can't be changed.
> 

I think the best solution here would be to make use of the
'Assigned clock parents and rates' and rates feature to allow selecting the
display clock sources on a per board basis. 

Peter.

> >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.
> 
> >>>Is it not possible to go the route of your first proposed patch to check
> >>>if the parent of display clock is pll_p?
> >>
> >>It is possible. If you'll decide to choose that route, then please drop the
> >>tegra_dc_div61() stuff as it is actually div71 and is not needed at all, I was
> >>just trying something using it.
> >>
> >>I think both variants are acceptable, though let's wait for the comment from
> >>Thierry, as the last word will be after him.
> >
> >Okay. It appears this route will have a minimal impact.
> >It would fix that existing workaround for Tegra 2 allowing the
> >pll_d clock rate to be set properly for the actual pclk rate.
> >
> >In the future if there is more advanced clock selection then this
> >workaround would possibly go away all together.
> 
> I don't mind. In the end it's up to Thierry to decide, so will see.
> 
> >>
> >>>>Note that changing PLLC rate will makes sense only if you're going to upstream
> >>>>the device tree for your device. Please prepare and submit the device tree and
> >>>>clock patches. I think Thierry and Peter won't oppose to the variant with
> >>>>changing rate of PLLC, for now that should be the best option.
> >>>>
> >>>
> >>>I would like to eventually upstream the device tree. There are only two things
> >>>at the moment that are blocking this device from working on upstream kernel.
> >>>One is this display clock initialization issue. The other is the
> >>>non-standard partition table which required a patch from the Anrdoid kernels.
> >>>For now I am using CONFIG_CMDLINE_PARTITION to specify the partition
> >>>layout.
> >>
> >>The partition layout shouldn't be a real stopper as seems there is external MMC.
> >>
> >>IIRC, the custom partition is the ms-dos partition with a custom offsets, isn't
> >>it? At least some of Tegra devices also have a spare GPT partition that is
> >>specified by bootloader via gpt_sector argument of the kernels commandline.
> >
> >Yes, this device has the custom offsets. It is same as the many other Tegra 2/3
> >devices.
> 
> Okay.
> 
> >
> >Here is the clock tree from upstream kernel.
> 
> Looks good, thank you.

On Tue, Sep 04, 2018 at 04:25:31PM +0300, Dmitry Osipenko wrote:
> On 9/3/18 7:49 PM, r yang wrote:
> > On Sun, Sep 02, 2018 at 04:54:09AM +0300, Dmitry Osipenko wrote:
> > > On 9/2/18 3:27 AM, r yang wrote:
> > > > On Fri, Aug 31, 2018 at 11:44:11PM +0300, Dmitry Osipenko wrote:
> > > > > On 8/31/18 10:31 PM, r yang wrote:
> > > > > > On Wed, Aug 29, 2018 at 08:52:37PM +0300, Dmitry Osipenko wrote:
> > > > > > > On 25.08.2018 04:11, r yang wrote:
> > > > > > > > On Fri, Aug 24, 2018 at 05:40:11AM +0300, Dmitry Osipenko wrote:
> > > > > > > > > On Friday 24 August 2018 03:53:22 ryang wrote:
> > > > > > > > > > There is a workaround in which the tegra rgb driver initializes
> > > > > > > > > > the tegra dc pclk to 0 so that it will skip setting the parent clk rate.
> > > > > > > > > > 
> > > > > > > > > > The relevant commits:
> > > > > > > > > > 3cebae6737b100323baca21de6bce6647249e778
> > > > > > > > > > 76d59ed049197bdaaa24c0e061a105af6ce74457
> > > > > > > > > > 
> > > > > > > > > > A more recent commit sets the rate of the dc clk itself:
> > > > > > > > > > 39e08affecf0998be1b01f4752016e33fa98eb9a
> > > > > > > > > > 
> > > > > > > > > > This doesn't make sense because it always sets the dc clk to 0.
> > > > > > > > > > Is this intended behavior or does it just happen to be working for
> > > > > > > > > > the current tegra 2 boards in the mainline kernel?
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > For context I am running the kernel on a tegra 2 based
> > > > > > > > > > Galaxy Tab 10.1. The display panel driver is out of tree.
> > > > > > > > > > This panel has very low clock rate tolerances so it must be
> > > > > > > > > > driven at a rate very close to the required specification (68.75Mhz).
> > > > > > > > > > pll_p is not adequate to drive this panel so pll_d must be used.
> > > > > > > > > > 
> > > > > > > > > > Another issue is the current workaround is always forcing the disp1
> > > > > > > > > > clk to zero. The Galaxy Tab 10.1 locks up completely when calling
> > > > > > > > > > clk_set_rate with a clk rate of 0 whether the parent is pll_p or
> > > > > > > > > > pll_d.
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > This patch adds a flag single_display_pll to mark that the device has
> > > > > > > > > > only one display pll. This replaces the the pclk = 0 workaround.
> > > > > > > > > > 
> > > > > > > > > > There is a comment in rgb.c about using the shift clock divider for
> > > > > > > > > > tegra 2 but the divider is not set due to the has_nvdisplay flag.
> > > > > > > > > > This patch also sets the shift clock divider based on the
> > > > > > > > > > single_display_pll flag.
> > > > > > > > > > 
> > > > > > > > > > A change I'm uncertain about. In tegra_dc_commit_state()
> > > > > > > > > > the dc clock is now being set to the display panel rate rather than zero.
> > > > > > > > > > 
> > > > > > > > > > I don't have any other tegra devices to test with. So I don't know
> > > > > > > > > > if this breaks other devices. Previously the code was trying to set the
> > > > > > > > > > clock rate to zero anyways.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: ryang <decatf@gmail.com>
> > > > > > > > > > ---
> > > > > > > > > >   drivers/gpu/drm/tegra/dc.c  | 9 +++++++--
> > > > > > > > > >   drivers/gpu/drm/tegra/dc.h  | 1 +
> > > > > > > > > >   drivers/gpu/drm/tegra/rgb.c | 1 -
> > > > > > > > > >   3 files changed, 8 insertions(+), 3 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > > > > > > > > > index 965088afcfad..03f1ad630254 100644
> > > > > > > > > > --- a/drivers/gpu/drm/tegra/dc.c
> > > > > > > > > > +++ b/drivers/gpu/drm/tegra/dc.c
> > > > > > > > > > @@ -1664,7 +1664,7 @@ static void tegra_dc_commit_state(struct tegra_dc
> > > > > > > > > > *dc, * which is shared with other peripherals. Changing the clock rate *
> > > > > > > > > > should therefore be avoided.
> > > > > > > > > >   	 */
> > > > > > > > > > -	if (state->pclk > 0) {
> > > > > > > > > > +	if (!dc->soc->single_display_pll) {
> > > > > > > > > 
> > > > > > > > > We don't want to change pll_p rate here. It will be better to check whether
> > > > > > > > > parent clock for disp is pll_d in tegra_rgb_encoder_atomic_check() and if it
> > > > > > > > > is the parent, then allow to propagate pclk into commit_state. We may request
> > > > > > > > > the pll_d using clk_get_sys(), then get the parent clock of disp by
> > > > > > > > > clk_get_parent(disp_clk) and finally compare the parent with pll_d using
> > > > > > > > > clk_is_math(parent, pll_d).
> > > > > > > > 
> > > > > > > > Understood.
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Certainly parent clock selection could be more advanced, ideally the best
> > > > > > > > > parent clock should be selected in the atomic check and applied to disp clk
> > > > > > > > > on the commit by changing the disp's parent. So pll_d could be always
> > > > > > > > > selected if only one display controller is active at a time, otherwise the
> > > > > > > > > atomic check should resolve the parent clocks based on the required rates. It
> > > > > > > > > also could be permitable to adjust the pll_c rate.
> > > > > > > > 
> > > > > > > > Right, the TRM describes quite a wide range of clock selection which currently
> > > > > > > > aren't all possible to use on the kernel. The Galaxy Tab 10.1 stock kernel uses
> > > > > > > > pll_c solely for disp1. Every clock normally on pll_c such as gr3d/gr2d/vde/etc
> > > > > > > > are on pll_m. I suspect it was designed this way because the TRM says pll_d has
> > > > > > > > a very power usage and that it should be avoided if possible.
> > > > > > > > 
> > > > > > > > In absense of more advanced clock selection. It was my intention with this
> > > > > > > > patch to try to at least have pll_p and pll_d working right for the internal
> > > > > > > > display.
> > > > > > > > 
> > > > > > > 
> > > > > > > Could you show the clock tree from downstream kernel? I'm curious what it looks
> > > > > > > like, that should be in "/sys/kernel/debug/clock/clock_tree".
> > > > > > > 
> > > > > > > 
> > > > > > > The patch below seems to work reasonably good, both HDMI and panel either are
> > > > > > > working simultaneously or only panel works when both displays are running off
> > > > > > > the pll_d. Could you give it a try?
> > > > > > > 
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > > > > > > index bdc418bcd898..c91f1203418d 100644
> > > > > > > --- a/drivers/gpu/drm/tegra/dc.c
> > > > > > > +++ b/drivers/gpu/drm/tegra/dc.c
> > > > > > > @@ -26,6 +26,33 @@
> > > > > > >   #include <drm/drm_atomic_helper.h>
> > > > > > >   #include <drm/drm_plane_helper.h>
> > > > > > > 
> > > > > > > +unsigned int tegra_dc_div61(unsigned long  parent, unsigned long pixel)
> > > > > > > +{
> > > > > > > +	unsigned int rem, fract = 0;
> > > > > > > +	unsigned int div61;
> > > > > > > +	u64 integer;
> > > > > > > +
> > > > > > > +	if (parent <= pixel)
> > > > > > > +		return 0;
> > > > > > > +
> > > > > > > +	integer = parent;
> > > > > > > +	rem = do_div(integer, pixel);
> > > > > > > +
> > > > > > > +	if (rem > pixel / 4) {
> > > > > > > +		if (rem > pixel * 2 / 3)
> > > > > > > +			integer += 1;
> > > > > > > +		else
> > > > > > > +			fract = 1;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	div61 = (integer << 1) | fract;
> > > > > > > +
> > > > > > > +	if (div61 > 255)
> > > > > > > +		return 255;
> > > > > > > +
> > > > > > > +	return div61;
> > > > > > > +}
> > > > > > > +
> > > > > > >   static void tegra_dc_stats_reset(struct tegra_dc_stats *stats)
> > > > > > >   {
> > > > > > >   	stats->frames = 0;
> > > > > > > diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
> > > > > > > index 382d38b1524b..dc3f681ddf1f 100644
> > > > > > > --- a/drivers/gpu/drm/tegra/dc.h
> > > > > > > +++ b/drivers/gpu/drm/tegra/dc.h
> > > > > > > @@ -174,6 +174,7 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc,
> > > > > > >   			       struct drm_crtc_state *crtc_state,
> > > > > > >   			       struct clk *clk, unsigned long pclk,
> > > > > > >   			       unsigned int div);
> > > > > > > +unsigned int tegra_dc_div61(unsigned long  parent, unsigned long pixel);
> > > > > > > 
> > > > > > >   /* from rgb.c */
> > > > > > >   int tegra_dc_rgb_probe(struct tegra_dc *dc);
> > > > > > > diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
> > > > > > > index 0082468f703c..3c19af0eafe7 100644
> > > > > > > --- a/drivers/gpu/drm/tegra/hdmi.c
> > > > > > > +++ b/drivers/gpu/drm/tegra/hdmi.c
> > > > > > > @@ -1450,10 +1450,13 @@ tegra_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
> > > > > > >   	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
> > > > > > >   	unsigned long pclk = crtc_state->mode.clock * 1000;
> > > > > > >   	struct tegra_hdmi *hdmi = to_hdmi(output);
> > > > > > > +	unsigned long rate;
> > > > > > >   	int err;
> > > > > > > 
> > > > > > > +	rate = clk_round_rate(hdmi->clk_parent, pclk);
> > > > > > > +
> > > > > > >   	err = tegra_dc_state_setup_clock(dc, crtc_state, hdmi->clk_parent,
> > > > > > > -					 pclk, 0);
> > > > > > > +					 rate, tegra_dc_div61(rate, pclk));
> > > > > > >   	if (err < 0) {
> > > > > > >   		dev_err(output->dev, "failed to setup CRTC state: %d\n", err);
> > > > > > >   		return err;
> > > > > > > diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
> > > > > > > index 28a78d3120bc..3ec3e13ce47f 100644
> > > > > > > --- a/drivers/gpu/drm/tegra/rgb.c
> > > > > > > +++ b/drivers/gpu/drm/tegra/rgb.c
> > > > > > > @@ -19,6 +19,7 @@ struct tegra_rgb {
> > > > > > >   	struct tegra_output output;
> > > > > > >   	struct tegra_dc *dc;
> > > > > > > 
> > > > > > > +	struct clk *pll_d_out0;
> > > > > > >   	struct clk *clk_parent;
> > > > > > >   	struct clk *clk;
> > > > > > >   };
> > > > > > > @@ -130,6 +131,8 @@ static void tegra_rgb_encoder_disable(struct drm_encoder
> > > > > > > *encoder)
> > > > > > > 
> > > > > > >   	if (output->panel)
> > > > > > >   		drm_panel_unprepare(output->panel);
> > > > > > > +
> > > > > > > +	clk_rate_exclusive_put(rgb->clk);
> > > > > > >   }
> > > > > > > 
> > > > > > >   static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)
> > > > > > > @@ -165,6 +168,8 @@ static void tegra_rgb_encoder_enable(struct drm_encoder
> > > > > > > *encoder)
> > > > > > > 
> > > > > > >   	if (output->panel)
> > > > > > >   		drm_panel_enable(output->panel);
> > > > > > > +
> > > > > > > +	clk_rate_exclusive_get(rgb->clk);
> > > > > > >   }
> > > > > > > 
> > > > > > >   static int
> > > > > > > @@ -174,11 +179,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
> > > > > > >   {
> > > > > > >   	struct tegra_output *output = encoder_to_output(encoder);
> > > > > > >   	struct tegra_dc *dc = to_tegra_dc(conn_state->crtc);
> > > > > > > -	unsigned long pclk = crtc_state->mode.clock * 1000;
> > > > > > >   	struct tegra_rgb *rgb = to_rgb(output);
> > > > > > > -	unsigned int div;
> > > > > > > +	unsigned long pclk = crtc_state->mode.clock * 1000;
> > > > > > > +	unsigned long rate;
> > > > > > > +	bool pll_d_parent;
> > > > > > >   	int err;
> > > > > > > 
> > > > > > > +	if (clk_is_match(rgb->clk_parent, rgb->pll_d_out0))
> > > > > > > +		pll_d_parent = true;
> > > > > > > +	else
> > > > > > > +		pll_d_parent = false;
> > > > > > > +
> > > > > > >   	/*
> > > > > > >   	 * We may not want to change the frequency of the parent clock, since
> > > > > > >   	 * it may be a parent for other peripherals. This is due to the fact
> > > > > > > @@ -194,12 +205,17 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
> > > > > > >   	 * The best we can do at this point is to use the shift clock divider
> > > > > > >   	 * and hope that the desired frequency can be matched (or at least
> > > > > > >   	 * matched sufficiently close that the panel will still work).
> > > > > > > +	 *
> > > > > > > +	 * Make excuse for pll_d_out0 if it is explicitly set as a parent
> > > > > > > +	 * for display panel.
> > > > > > >   	 */
> > > > > > > -	div = ((clk_get_rate(rgb->clk) * 2) / pclk) - 2;
> > > > > > > -	pclk = 0;
> > > > > > > +	if (pll_d_parent)
> > > > > > > +		rate = clk_round_rate(rgb->clk_parent, pclk * 4);
> > > > > > > +	else
> > > > > > > +		rate = clk_get_rate(rgb->clk);
> > > > > > > 
> > > > > > >   	err = tegra_dc_state_setup_clock(dc, crtc_state, rgb->clk_parent,
> > > > > > > -					 pclk, div);
> > > > > > > +					 rate, tegra_dc_div61(rate, pclk));
> > > > > > >   	if (err < 0) {
> > > > > > >   		dev_err(output->dev, "failed to setup CRTC state: %d\n", err);
> > > > > > >   		return err;
> > > > > > > @@ -254,6 +270,12 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
> > > > > > >   		return err;
> > > > > > >   	}
> > > > > > > 
> > > > > > > +	rgb->pll_d_out0 = clk_get_sys(NULL, "pll_d_out0");
> > > > > > > +	if (IS_ERR(rgb->pll_d_out0)) {
> > > > > > > +		dev_err(dc->dev, "failed to get pll_d_out0\n");
> > > > > > > +		return PTR_ERR(rgb->pll_d_out0);
> > > > > > > +	}
> > > > > > > +
> > > > > > >   	dc->rgb = &rgb->output;
> > > > > > > 
> > > > > > >   	return 0;
> > > > > > > @@ -265,6 +287,7 @@ int tegra_dc_rgb_remove(struct tegra_dc *dc)
> > > > > > >   		return 0;
> > > > > > > 
> > > > > > >   	tegra_output_remove(dc->rgb);
> > > > > > > +	clk_put(to_rgb(dc->rgb)->pll_d_out0);
> > > > > > >   	dc->rgb = NULL;
> > > > > > > 
> > > > > > >   	return 0;
> > > > > > 
> > > > > > I've tested the patch and it works. The disp1 will run off pll_d without
> > > > > > any issues.
> > > > > > 
> > > > > > Here is the clock tree from the downstream kernel.
> > > > > 
> > > > > Okay, thank you. Now I think that it's better not to touch the display driver at
> > > > > all, but change the PLLC clock to 586 MHz in the clock driver. There is no
> > > > > appreciable difference in 600 vs 586 MHz, so that should be good enough.
> > > > > 
> > > > > It would be ideal if clock rates could be specified via device tree, but that's
> > > > > not possible today. So just change the PLLC rate globally in the clocks
> > > > > init_table. Maybe later we'll manage to implement something that will allow to
> > > > > set rates via device tree.
> > > > > 
> > > > 
> > > > There are two variants of the display panel chip for this tablet.
> > > > They run at different clock rates:
> > > > 
> > > >   - 68.9Mhz divided down from 586Mhz
> > > >   - 72Mhhz divided down from 570Mhz
> > > > 
> > > > With this in consideration I don't think it makes sense to change the
> > > > initial pll_c rate.
> > > 
> > > The 72MHz variant will work perfectly with the PLLP as a parent. Have you tried
> > > to use timings and rate of the 72MHz variant?
> > 
> > Sorry, I had it wrong. It is 76Mhz. Not 72Mhz.
> > I have spent a great deal of time trying to find alternate clock
> > configuration for the display panel that can work. The only ways are:
> > 
> >   - Run disp1 on pll_d
> >   - Dedicate pll_c to disp1. Move all gpu clocks and sclk/hclk/ahbdma to pll_m.
> 
> 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.

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.

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.

> > 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.

> 
> > > > Is it not possible to go the route of your first proposed patch to check
> > > > if the parent of display clock is pll_p?
> > > 
> > > It is possible. If you'll decide to choose that route, then please drop the
> > > tegra_dc_div61() stuff as it is actually div71 and is not needed at all, I was
> > > just trying something using it.
> > > 
> > > I think both variants are acceptable, though let's wait for the comment from
> > > Thierry, as the last word will be after him.
> > 
> > Okay. It appears this route will have a minimal impact.
> > It would fix that existing workaround for Tegra 2 allowing the
> > pll_d clock rate to be set properly for the actual pclk rate.
> > 
> > In the future if there is more advanced clock selection then this
> > workaround would possibly go away all together.
> 
> I don't mind. In the end it's up to Thierry to decide, so will see.
> 
> > > 
> > > > > Note that changing PLLC rate will makes sense only if you're going to upstream
> > > > > the device tree for your device. Please prepare and submit the device tree and
> > > > > clock patches. I think Thierry and Peter won't oppose to the variant with
> > > > > changing rate of PLLC, for now that should be the best option.
> > > > > 
> > > > 
> > > > I would like to eventually upstream the device tree. There are only two things
> > > > at the moment that are blocking this device from working on upstream kernel.
> > > > One is this display clock initialization issue. The other is the
> > > > non-standard partition table which required a patch from the Anrdoid kernels.
> > > > For now I am using CONFIG_CMDLINE_PARTITION to specify the partition
> > > > layout.
> > > 
> > > The partition layout shouldn't be a real stopper as seems there is external MMC.
> > > 
> > > IIRC, the custom partition is the ms-dos partition with a custom offsets, isn't
> > > it? At least some of Tegra devices also have a spare GPT partition that is
> > > specified by bootloader via gpt_sector argument of the kernels commandline.
> > 
> > Yes, this device has the custom offsets. It is same as the many other Tegra 2/3
> > devices.
> 
> Okay.
> 
> > 
> > Here is the clock tree from upstream kernel.
> 
> Looks good, thank you.

On 9/5/18 12:37 PM, Peter De Schrijver wrote:
>>>
>>> Sorry, I had it wrong. It is 76Mhz. Not 72Mhz.
>>> I have spent a great deal of time trying to find alternate clock
>>> configuration for the display panel that can work. The only ways are:
>>>
>>>   - Run disp1 on pll_d
>>>   - Dedicate pll_c to disp1. Move all gpu clocks and sclk/hclk/ahbdma to pll_m.
>>
>> PLL_M is kinda reserved for the memory, hence its rate can't be changed.
>>
> 
> I think the best solution here would be to make use of the
> 'Assigned clock parents and rates' and rates feature to allow selecting the
> display clock sources on a per board basis.

Looks like it works, here the device-tree that I used:

	host1x@50000000 {
		dc@54200000 {
			clocks = <&tegra_car TEGRA20_CLK_DISP1>,
				 <&tegra_car TEGRA20_CLK_PLL_C>;

			/* Re-parent SCLK to P_OUT4 and set PLL_C to 586MHz */
			assigned-clocks = <&tegra_car TEGRA20_CLK_SCLK>,
					  <&tegra_car TEGRA20_CLK_HCLK>,
					  <&tegra_car TEGRA20_CLK_PCLK>,
					  <&tegra_car TEGRA20_CLK_PLL_C>;

			assigned-clock-rates = <216000000>, /* SCLK */
					       <216000000>, /* HCLK */
					       <54000000>,  /* PCLK */
					       <586000000>; /* PLL_C */

			assigned-clock-parents = <&tegra_car TEGRA20_CLK_PLL_P_OUT4>;

			rgb {
				status = "okay";

				nvidia,panel = <&panel>;
			};
		};

		hdmi@54280000 {
			status = "okay";

			vdd-supply = <&hdmi_vdd_reg>;
			pll-supply = <&hdmi_pll_reg>;

			nvidia,ddc-i2c-bus = <&hdmi_ddc>;
			nvidia,hpd-gpio = <&gpio TEGRA_GPIO(N, 7)
				GPIO_ACTIVE_HIGH>;
		};
	};

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]

On Fri, Sep 07, 2018 at 09:01:14PM +0300, Dmitry Osipenko wrote:
> On 9/5/18 12:37 PM, Peter De Schrijver wrote:
> > > > 
> > > > Sorry, I had it wrong. It is 76Mhz. Not 72Mhz.
> > > > I have spent a great deal of time trying to find alternate clock
> > > > configuration for the display panel that can work. The only ways are:
> > > > 
> > > >   - Run disp1 on pll_d
> > > >   - Dedicate pll_c to disp1. Move all gpu clocks and sclk/hclk/ahbdma to pll_m.
> > > 
> > > PLL_M is kinda reserved for the memory, hence its rate can't be changed.
> > > 
> > 
> > I think the best solution here would be to make use of the
> > 'Assigned clock parents and rates' and rates feature to allow selecting the
> > display clock sources on a per board basis.
> 
> Looks like it works, here the device-tree that I used:
> 
> 	host1x@50000000 {
> 		dc@54200000 {
> 			clocks = <&tegra_car TEGRA20_CLK_DISP1>,
> 				 <&tegra_car TEGRA20_CLK_PLL_C>;
> 
> 			/* Re-parent SCLK to P_OUT4 and set PLL_C to 586MHz */
> 			assigned-clocks = <&tegra_car TEGRA20_CLK_SCLK>,
> 					  <&tegra_car TEGRA20_CLK_HCLK>,
> 					  <&tegra_car TEGRA20_CLK_PCLK>,
> 					  <&tegra_car TEGRA20_CLK_PLL_C>;
> 
> 			assigned-clock-rates = <216000000>, /* SCLK */
> 					       <216000000>, /* HCLK */
> 					       <54000000>,  /* PCLK */
> 					       <586000000>; /* PLL_C */
> 
> 			assigned-clock-parents = <&tegra_car TEGRA20_CLK_PLL_P_OUT4>;
> 
> 			rgb {
> 				status = "okay";
> 
> 				nvidia,panel = <&panel>;
> 			};
> 		};
> 
> 		hdmi@54280000 {
> 			status = "okay";
> 
> 			vdd-supply = <&hdmi_vdd_reg>;
> 			pll-supply = <&hdmi_pll_reg>;
> 
> 			nvidia,ddc-i2c-bus = <&hdmi_ddc>;
> 			nvidia,hpd-gpio = <&gpio TEGRA_GPIO(N, 7)
> 				GPIO_ACTIVE_HIGH>;
> 		};
> 	};

This configuration works. Using this feature doesn't require any
change to the display driver.

Thanks.

On 9/9/18 6:22 AM, r yang wrote:
> On Fri, Sep 07, 2018 at 09:01:14PM +0300, Dmitry Osipenko wrote:
>> On 9/5/18 12:37 PM, Peter De Schrijver wrote:
>>>>>
>>>>> Sorry, I had it wrong. It is 76Mhz. Not 72Mhz.
>>>>> I have spent a great deal of time trying to find alternate clock
>>>>> configuration for the display panel that can work. The only ways are:
>>>>>
>>>>>    - Run disp1 on pll_d
>>>>>    - Dedicate pll_c to disp1. Move all gpu clocks and sclk/hclk/ahbdma to pll_m.
>>>>
>>>> PLL_M is kinda reserved for the memory, hence its rate can't be changed.
>>>>
>>>
>>> I think the best solution here would be to make use of the
>>> 'Assigned clock parents and rates' and rates feature to allow selecting the
>>> display clock sources on a per board basis.
>>
>> Looks like it works, here the device-tree that I used:
>>
>> 	host1x@50000000 {
>> 		dc@54200000 {
>> 			clocks = <&tegra_car TEGRA20_CLK_DISP1>,
>> 				 <&tegra_car TEGRA20_CLK_PLL_C>;
>>
>> 			/* Re-parent SCLK to P_OUT4 and set PLL_C to 586MHz */
>> 			assigned-clocks = <&tegra_car TEGRA20_CLK_SCLK>,
>> 					  <&tegra_car TEGRA20_CLK_HCLK>,
>> 					  <&tegra_car TEGRA20_CLK_PCLK>,
>> 					  <&tegra_car TEGRA20_CLK_PLL_C>;
>>
>> 			assigned-clock-rates = <216000000>, /* SCLK */
>> 					       <216000000>, /* HCLK */
>> 					       <54000000>,  /* PCLK */
>> 					       <586000000>; /* PLL_C */
>>
>> 			assigned-clock-parents = <&tegra_car TEGRA20_CLK_PLL_P_OUT4>;
>>
>> 			rgb {
>> 				status = "okay";
>>
>> 				nvidia,panel = <&panel>;
>> 			};
>> 		};
>>
>> 		hdmi@54280000 {
>> 			status = "okay";
>>
>> 			vdd-supply = <&hdmi_vdd_reg>;
>> 			pll-supply = <&hdmi_pll_reg>;
>>
>> 			nvidia,ddc-i2c-bus = <&hdmi_ddc>;
>> 			nvidia,hpd-gpio = <&gpio TEGRA_GPIO(N, 7)
>> 				GPIO_ACTIVE_HIGH>;
>> 		};
>> 	};
> 
> This configuration works. Using this feature doesn't require any
> change to the display driver.
> 
> Thanks.
> 

Nice! Though the pclk=0 still need to be fixed in the DRM driver. Peter 
suggested earlier (on IRC) that rather than error'ing out on freq=0 in the clk 
driver, the freq shall round up to the lowest possible value.

On Fri, Sep 14, 2018 at 01:58:57PM +0300, Dmitry Osipenko wrote:
> On 9/9/18 6:22 AM, r yang wrote:
> > On Fri, Sep 07, 2018 at 09:01:14PM +0300, Dmitry Osipenko wrote:
> > > On 9/5/18 12:37 PM, Peter De Schrijver wrote:
> > > > > > 
> > > > > > Sorry, I had it wrong. It is 76Mhz. Not 72Mhz.
> > > > > > I have spent a great deal of time trying to find alternate clock
> > > > > > configuration for the display panel that can work. The only ways are:
> > > > > > 
> > > > > >    - Run disp1 on pll_d
> > > > > >    - Dedicate pll_c to disp1. Move all gpu clocks and sclk/hclk/ahbdma to pll_m.
> > > > > 
> > > > > PLL_M is kinda reserved for the memory, hence its rate can't be changed.
> > > > > 
> > > > 
> > > > I think the best solution here would be to make use of the
> > > > 'Assigned clock parents and rates' and rates feature to allow selecting the
> > > > display clock sources on a per board basis.
> > > 
> > > Looks like it works, here the device-tree that I used:
> > > 
> > > 	host1x@50000000 {
> > > 		dc@54200000 {
> > > 			clocks = <&tegra_car TEGRA20_CLK_DISP1>,
> > > 				 <&tegra_car TEGRA20_CLK_PLL_C>;
> > > 
> > > 			/* Re-parent SCLK to P_OUT4 and set PLL_C to 586MHz */
> > > 			assigned-clocks = <&tegra_car TEGRA20_CLK_SCLK>,
> > > 					  <&tegra_car TEGRA20_CLK_HCLK>,
> > > 					  <&tegra_car TEGRA20_CLK_PCLK>,
> > > 					  <&tegra_car TEGRA20_CLK_PLL_C>;
> > > 
> > > 			assigned-clock-rates = <216000000>, /* SCLK */
> > > 					       <216000000>, /* HCLK */
> > > 					       <54000000>,  /* PCLK */
> > > 					       <586000000>; /* PLL_C */
> > > 
> > > 			assigned-clock-parents = <&tegra_car TEGRA20_CLK_PLL_P_OUT4>;
> > > 
> > > 			rgb {
> > > 				status = "okay";
> > > 
> > > 				nvidia,panel = <&panel>;
> > > 			};
> > > 		};
> > > 
> > > 		hdmi@54280000 {
> > > 			status = "okay";
> > > 
> > > 			vdd-supply = <&hdmi_vdd_reg>;
> > > 			pll-supply = <&hdmi_pll_reg>;
> > > 
> > > 			nvidia,ddc-i2c-bus = <&hdmi_ddc>;
> > > 			nvidia,hpd-gpio = <&gpio TEGRA_GPIO(N, 7)
> > > 				GPIO_ACTIVE_HIGH>;
> > > 		};
> > > 	};
> > 
> > This configuration works. Using this feature doesn't require any
> > change to the display driver.
> > 
> > Thanks.
> > 
> 
> Nice! Though the pclk=0 still need to be fixed in the DRM driver. Peter
> suggested earlier (on IRC) that rather than error'ing out on freq=0 in the
> clk driver, the freq shall round up to the lowest possible value.

Makes sense since the round_rate documentation says it should return the
closest rate actually supported by the clock. Has anyone taken to making
this change?
I think the minimum rate can be derived based on the same pll constraints
used in the _calc_rate() function. I've tested it on pll_c and pll_d but
I'm not 100% certain about my method yet.

On 9/16/18 4:55 AM, r yang wrote:
> On Fri, Sep 14, 2018 at 01:58:57PM +0300, Dmitry Osipenko wrote:
>> On 9/9/18 6:22 AM, r yang wrote:
>>> On Fri, Sep 07, 2018 at 09:01:14PM +0300, Dmitry Osipenko wrote:
>>>> On 9/5/18 12:37 PM, Peter De Schrijver wrote:
>>>>>>>
>>>>>>> Sorry, I had it wrong. It is 76Mhz. Not 72Mhz.
>>>>>>> I have spent a great deal of time trying to find alternate clock
>>>>>>> configuration for the display panel that can work. The only ways are:
>>>>>>>
>>>>>>>     - Run disp1 on pll_d
>>>>>>>     - Dedicate pll_c to disp1. Move all gpu clocks and sclk/hclk/ahbdma to pll_m.
>>>>>>
>>>>>> PLL_M is kinda reserved for the memory, hence its rate can't be changed.
>>>>>>
>>>>>
>>>>> I think the best solution here would be to make use of the
>>>>> 'Assigned clock parents and rates' and rates feature to allow selecting the
>>>>> display clock sources on a per board basis.
>>>>
>>>> Looks like it works, here the device-tree that I used:
>>>>
>>>> 	host1x@50000000 {
>>>> 		dc@54200000 {
>>>> 			clocks = <&tegra_car TEGRA20_CLK_DISP1>,
>>>> 				 <&tegra_car TEGRA20_CLK_PLL_C>;
>>>>
>>>> 			/* Re-parent SCLK to P_OUT4 and set PLL_C to 586MHz */
>>>> 			assigned-clocks = <&tegra_car TEGRA20_CLK_SCLK>,
>>>> 					  <&tegra_car TEGRA20_CLK_HCLK>,
>>>> 					  <&tegra_car TEGRA20_CLK_PCLK>,
>>>> 					  <&tegra_car TEGRA20_CLK_PLL_C>;
>>>>
>>>> 			assigned-clock-rates = <216000000>, /* SCLK */
>>>> 					       <216000000>, /* HCLK */
>>>> 					       <54000000>,  /* PCLK */
>>>> 					       <586000000>; /* PLL_C */
>>>>
>>>> 			assigned-clock-parents = <&tegra_car TEGRA20_CLK_PLL_P_OUT4>;
>>>>
>>>> 			rgb {
>>>> 				status = "okay";
>>>>
>>>> 				nvidia,panel = <&panel>;
>>>> 			};
>>>> 		};
>>>>
>>>> 		hdmi@54280000 {
>>>> 			status = "okay";
>>>>
>>>> 			vdd-supply = <&hdmi_vdd_reg>;
>>>> 			pll-supply = <&hdmi_pll_reg>;
>>>>
>>>> 			nvidia,ddc-i2c-bus = <&hdmi_ddc>;
>>>> 			nvidia,hpd-gpio = <&gpio TEGRA_GPIO(N, 7)
>>>> 				GPIO_ACTIVE_HIGH>;
>>>> 		};
>>>> 	};
>>>
>>> This configuration works. Using this feature doesn't require any
>>> change to the display driver.
>>>
>>> Thanks.
>>>
>>
>> Nice! Though the pclk=0 still need to be fixed in the DRM driver. Peter
>> suggested earlier (on IRC) that rather than error'ing out on freq=0 in the
>> clk driver, the freq shall round up to the lowest possible value.
> 
> Makes sense since the round_rate documentation says it should return the
> closest rate actually supported by the clock. Has anyone taken to making
> this change?

I may take a closer look at it later this / next week.

> I think the minimum rate can be derived based on the same pll constraints
> used in the _calc_rate() function. I've tested it on pll_c and pll_d but
> I'm not 100% certain about my method yet.
> 
Feel free to send out RFC patch, it's always better to look at the actual code.