Re: [PATCH] clk: tegra: fix HOST1X clock divider on Tegra20 and Tegra30

[Dmitry Osipenko]

On 11/2/22 11:32, Luca Ceresoli wrote:
> Hello Dmitry,
> 
> On Mon, 31 Oct 2022 03:34:07 +0300
> Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx> wrote:
> 
>> On 10/28/22 10:48, luca.ceresoli@xxxxxxxxxxx wrote:
>>> From: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
>>>
>>> On Tegra20 and Tegra30 the HOST1X clock is a fractional clock divider with
>>> 7 integer bits + 1 decimal bit. This has been verified on both
>>> documentation and real hardware for Tegra20 an on the documentation I was
>>> able to find for Tegra30.
>>>
>>> However in the kernel code this clock is declared as an integer divider. A
>>> consequence of this is that requesting 144 MHz for HOST1X which is fed by
>>> pll_p running at 216 MHz would result in 108 MHz (216 / 2) instead of 144
>>> MHz (216 / 1.5).
>>>
>>> Fix by replacing the INT() macro with the MUX() macro which, despite the
>>> name, defines a fractional divider. The only difference between the two
>>> macros is the former does not have the TEGRA_DIVIDER_INT flag.
>>>
>>> Also move the line together with the other MUX*() ones to keep the existing
>>> file organization.
>>>
>>> Fixes: 76ebc134d45d ("clk: tegra: move periph clocks to common file")
>>> Cc: stable@xxxxxxxxxxxxxxx
>>> Cc: Peter De Schrijver <pdeschrijver@xxxxxxxxxx>
>>> Signed-off-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
>>> ---
>>>  drivers/clk/tegra/clk-tegra-periph.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/tegra/clk-tegra-periph.c b/drivers/clk/tegra/clk-tegra-periph.c
>>> index 4dcf7f7cb8a0..806d835ca0d2 100644
>>> --- a/drivers/clk/tegra/clk-tegra-periph.c
>>> +++ b/drivers/clk/tegra/clk-tegra-periph.c
>>> @@ -615,7 +615,6 @@ static struct tegra_periph_init_data periph_clks[] = {
>>>  	INT("vde", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_VDE, 61, 0, tegra_clk_vde),
>>>  	INT("vi", mux_pllm_pllc_pllp_plla, CLK_SOURCE_VI, 20, 0, tegra_clk_vi),
>>>  	INT("epp", mux_pllm_pllc_pllp_plla, CLK_SOURCE_EPP, 19, 0, tegra_clk_epp),
>>> -	INT("host1x", mux_pllm_pllc_pllp_plla, CLK_SOURCE_HOST1X, 28, 0, tegra_clk_host1x),
>>>  	INT("mpe", mux_pllm_pllc_pllp_plla, CLK_SOURCE_MPE, 60, 0, tegra_clk_mpe),
>>>  	INT("2d", mux_pllm_pllc_pllp_plla, CLK_SOURCE_2D, 21, 0, tegra_clk_gr2d),
>>>  	INT("3d", mux_pllm_pllc_pllp_plla, CLK_SOURCE_3D, 24, 0, tegra_clk_gr3d),
>>> @@ -664,6 +663,7 @@ static struct tegra_periph_init_data periph_clks[] = {
>>>  	MUX("owr", mux_pllp_pllc_clkm, CLK_SOURCE_OWR, 71, TEGRA_PERIPH_ON_APB, tegra_clk_owr_8),
>>>  	MUX("nor", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_NOR, 42, 0, tegra_clk_nor),
>>>  	MUX("mipi", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_MIPI, 50, TEGRA_PERIPH_ON_APB, tegra_clk_mipi),
>>> +	MUX("host1x", mux_pllm_pllc_pllp_plla, CLK_SOURCE_HOST1X, 28, 0, tegra_clk_host1x),
>>>  	MUX("vi_sensor", mux_pllm_pllc_pllp_plla, CLK_SOURCE_VI_SENSOR, 20, TEGRA_PERIPH_NO_RESET, tegra_clk_vi_sensor),
>>>  	MUX("vi_sensor", mux_pllc_pllp_plla, CLK_SOURCE_VI_SENSOR, 20, TEGRA_PERIPH_NO_RESET, tegra_clk_vi_sensor_9),
>>>  	MUX("cilab", mux_pllp_pllc_clkm, CLK_SOURCE_CILAB, 144, 0, tegra_clk_cilab),  
>>
>> This was attempted in the past
>> https://lore.kernel.org/all/20180723085010.GK1636@xxxxxxxxxxxxxxxxxxxxxxxxx/
>>
>> I assume here you're also porting the downstream patches to upstream.
>> This one is too questionable. The host1x clock shouldn't affect overall
>> performance to begin with. It doesn't make sense to use fractional clock
>> just for getting extra KHz.
> 
> Thank you for the review and for the pointer!
> 
> Indeed I'm not sure this patch brings an actual improvement to my use
> case, however I reached it by trying to replicate the configuration on
> a known-working kernel 3.1, which uses a 1.5 divider. This seems to be
> the same reason that led to the 2018 patch that also got rejected.
> 
> I'll be OK with dropping this patch after I have a 100% working setup
> with an integer divider, which is very likely given your reply. But it
> took time before I found the root cause of this issue, and I would like
> to avoid other people waste time in the future, so what about adding a
> comment there?
> 
> What about:
> 
>   /*
>    * The host1x clock shouldn't affect overall performance. It doesn't
>    * make sense to use fractional clock just for getting extra KHz, so
>    * let's pretend it's an integer divider
>    */

If host1x isn't the only clock like that, then comment shouldn't be
directed to host1x. Have you checked other clocks?

I'm curious who made that change originally in your downstream, was it
coming from NVIDIA?

-- 
Best regards,
Dmitry