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