linux-kernel - Re: [PATCH] clk: tegra: fix pllu rate configuration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 1 Mar 2018 16:19:03 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Peter De Schrijver <pdeschrijver@...dia.com>
Cc:     Marcel Ziswiler <marcel.ziswiler@...adex.com>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jonathanh@...dia.com" <jonathanh@...dia.com>,
        "mturquette@...libre.com" <mturquette@...libre.com>,
        "pgaikwad@...dia.com" <pgaikwad@...dia.com>,
        "sboyd@...nel.org" <sboyd@...nel.org>,
        "thierry.reding@...il.com" <thierry.reding@...il.com>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>
Subject: Re: [PATCH] clk: tegra: fix pllu rate configuration

On 01.03.2018 10:41, Peter De Schrijver wrote:
> On Wed, Feb 28, 2018 at 08:20:47PM +0300, Dmitry Osipenko wrote:
>> On 28.02.2018 17:14, Peter De Schrijver wrote:
>>> On Wed, Feb 28, 2018 at 03:00:23PM +0300, Dmitry Osipenko wrote:
>>>> On 28.02.2018 12:36, Peter De Schrijver wrote:
>>>>> On Tue, Feb 27, 2018 at 02:59:11PM +0300, Dmitry Osipenko wrote:
>>>>>> On 27.02.2018 02:04, Marcel Ziswiler wrote:
>>>>>>> On Mon, 2018-02-26 at 15:42 +0300, Dmitry Osipenko wrote:
>>>>>>>> On 23.02.2018 02:04, Marcel Ziswiler wrote:
>>>>>>>>> Turns out latest upstream U-Boot does not configure/enable pllu
>>>>>>>>> which
>>>>>>>>> leaves it at some default rate of 500 kHz:
>>>>>>>>>
>>>>>>>>> root@...lis-t30:~# cat /sys/kernel/debug/clk/clk_summary | grep
>>>>>>>>> pll_u
>>>>>>>>>        pll_u                  3        3        0      500000      
>>>>>>>>>     0
>>>>>>>>>
>>>>>>>>> Of course this won't quite work leading to the following messages:
>>>>>>>>>
>>>>>>>>> [    6.559593] usb 2-1: new full-speed USB device number 2 using
>>>>>>>>> tegra-
>>>>>>>>> ehci
>>>>>>>>> [   11.759173] usb 2-1: device descriptor read/64, error -110
>>>>>>>>> [   27.119453] usb 2-1: device descriptor read/64, error -110
>>>>>>>>> [   27.389217] usb 2-1: new full-speed USB device number 3 using
>>>>>>>>> tegra-
>>>>>>>>> ehci
>>>>>>>>> [   32.559454] usb 2-1: device descriptor read/64, error -110
>>>>>>>>> [   47.929777] usb 2-1: device descriptor read/64, error -110
>>>>>>>>> [   48.049658] usb usb2-port1: attempt power cycle
>>>>>>>>> [   48.759475] usb 2-1: new full-speed USB device number 4 using
>>>>>>>>> tegra-
>>>>>>>>> ehci
>>>>>>>>> [   59.349457] usb 2-1: device not accepting address 4, error -110
>>>>>>>>> [   59.509449] usb 2-1: new full-speed USB device number 5 using
>>>>>>>>> tegra-
>>>>>>>>> ehci
>>>>>>>>> [   70.069457] usb 2-1: device not accepting address 5, error -110
>>>>>>>>> [   70.079721] usb usb2-port1: unable to enumerate USB device
>>>>>>>>>
>>>>>>>>> Fix this by actually allowing the rate also being set from within
>>>>>>>>> the Linux kernel.
>>>>>
>>>>> I think the best solution to this problem would be to make pll_u a fixed
>>>>> clock and enable it and program the rate if it's not enabled at boot.
>>>>
>>>> Oh, right. PLL_U rate is actually configurable, somehow I missed it in TRM
>>>> yesterday.. So set/round_rate() for PLL_U are actually needed and the patch is
>>>> correct. Seems only T20 misses PLL_U in the init table, probably worth to add it
>>>> there.
>>>>
>>>
>>> AFAIK we only use one rate ever?
>>
>> IIUC, PLL_U has 3 outputs and output dividers are fixed in HW. So yes, we are
>> setting PLL_U to one rate - 480MHz to get out1-480MHz, out2-60MHz and out3-12MHz.
>>
> 
> Indeed. And given that it's hw controlled anyway, I don't see why we can't make
> it a fixed clock and handle the init at kernel boot depending on what the
> bootloader has done.

We can, I just don't think you can demand from Mark to do it. This patch is fine
on its own, everything else could be done later.

On the other hand, it's USB driver responsibility to do the right thing. Why
just not to correct the driver? It would be kinda weird to mask one driver bug
by adding workaround to another. So I'd even suggest to go other way round by
implementing PLL lock polling and removing PLL_U enabling/disabling from the USB
PHY driver (if it's really fully-controlled by HW).