Re: [PATCH] clk: tegra: fix pllu rate configuration

Re: [PATCH] clk: tegra: fix pllu rate configuration

[Dmitry Osipenko]

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@apalis-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.
> 
> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> ---
> 
>  drivers/clk/tegra/clk-pll.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index 7c369e21c91c..830d1c87fa7c 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -1151,6 +1151,8 @@ static const struct clk_ops tegra_clk_pllu_ops = {
>  	.enable = clk_pllu_enable,
>  	.disable = clk_pll_disable,
>  	.recalc_rate = clk_pll_recalc_rate,
> +	.round_rate = clk_pll_round_rate,
> +	.set_rate = clk_pll_set_rate,
>  };
>  
>  static int _pll_fixed_mdiv(struct tegra_clk_pll_params *pll_params,
> 

Tegra's USB PHY driver only enables clock and clk driver doesn't specify the
clock rate in the init table. Could you please clarify where in the kernels code
PLL_U rate is getting set?

Re: [PATCH] clk: tegra: fix pllu rate configuration

[Marcel Ziswiler]

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@apalis-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.
> > 
> > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > 
> > ---
> > 
> >  drivers/clk/tegra/clk-pll.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-
> > pll.c
> > index 7c369e21c91c..830d1c87fa7c 100644
> > --- a/drivers/clk/tegra/clk-pll.c
> > +++ b/drivers/clk/tegra/clk-pll.c
> > @@ -1151,6 +1151,8 @@ static const struct clk_ops
> > tegra_clk_pllu_ops = {
> >  	.enable = clk_pllu_enable,
> >  	.disable = clk_pll_disable,
> >  	.recalc_rate = clk_pll_recalc_rate,
> > +	.round_rate = clk_pll_round_rate,
> > +	.set_rate = clk_pll_set_rate,
> >  };
> >  
> >  static int _pll_fixed_mdiv(struct tegra_clk_pll_params
> > *pll_params,
> > 
> 
> Tegra's USB PHY driver only enables clock and clk driver doesn't
> specify the
> clock rate in the init table. Could you please clarify where in the
> kernels code
> PLL_U rate is getting set?

I guess that would be according to the following table isn't it:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree
/drivers/clk/tegra/clk-tegra30.c?h=v4.16-rc3#n287

Re: [PATCH] clk: tegra: fix pllu rate configuration

[Dmitry Osipenko]

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@apalis-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.
>>>
>>> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
>>>
>>> ---
>>>
>>>  drivers/clk/tegra/clk-pll.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-
>>> pll.c
>>> index 7c369e21c91c..830d1c87fa7c 100644
>>> --- a/drivers/clk/tegra/clk-pll.c
>>> +++ b/drivers/clk/tegra/clk-pll.c
>>> @@ -1151,6 +1151,8 @@ static const struct clk_ops
>>> tegra_clk_pllu_ops = {
>>>  	.enable = clk_pllu_enable,
>>>  	.disable = clk_pll_disable,
>>>  	.recalc_rate = clk_pll_recalc_rate,
>>> +	.round_rate = clk_pll_round_rate,
>>> +	.set_rate = clk_pll_set_rate,
>>>  };
>>>  
>>>  static int _pll_fixed_mdiv(struct tegra_clk_pll_params
>>> *pll_params,
>>>
>>
>> Tegra's USB PHY driver only enables clock and clk driver doesn't
>> specify the
>> clock rate in the init table. Could you please clarify where in the
>> kernels code
>> PLL_U rate is getting set?
> 
> I guess that would be according to the following table isn't it:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree
> /drivers/clk/tegra/clk-tegra30.c?h=v4.16-rc3#n287
> 

I see now that T30 has PLL_U in the init table [0], but T20 doesn't.

There is only one 480MHz rate defined in PLL_U lookup table. PLL_U also provides
12MHz and 60MHz outputs using fixed dividers and hence PLL_U shouldn't need
set/round_rate(), we can't change PLL_U rate at all and only enable / disable it.

Looks like for some reason PLL_U enabling fails without set/round_rate, but then
PHY driver should fail to probe [1]. Is it the case? Do you know why PLL_U isn't
actually getting enabled in HW? It is quite fishy, seems like clk driver /
framework bug or I'm missing something.

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/tegra/clk-tegra30.c?h=v4.16-rc3#n1274

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy-tegra-usb.c?h=v4.16-rc3#n777

Re: [PATCH] clk: tegra: fix pllu rate configuration

[Peter De Schrijver]

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@apalis-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.
This is how it's done for Tegra210. The reason is that the USB IP blocks
can control the pll_u state in hw. This means that if sw would disable
and then re-enable the pll_u clock, but there is no USB activity, pll_u
will still be disable and therefor not lock, causing an error. Today
this is worked around by not polling the lock bit for pll_u, but a better
solution would be to just remove all sw controls for pll_u.

Peter.

Re: [PATCH] clk: tegra: fix pllu rate configuration

[Dmitry Osipenko]

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

> This is how it's done for Tegra210. The reason is that the USB IP blocks
> can control the pll_u state in hw. This means that if sw would disable
> and then re-enable the pll_u clock, but there is no USB activity, pll_u
> will still be disable and therefor not lock, causing an error. Today
> this is worked around by not polling the lock bit for pll_u, but a better
> solution would be to just remove all sw controls for pll_u.

SW controls could be removed, but I don't think it is really necessary as in our
case SW is the PHY driver and we know what it does. Alternatively we can enable
PLL_U in the init table to keep it "always" enabled.

Re: [PATCH] clk: tegra: fix pllu rate configuration

[Peter De Schrijver]

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@apalis-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?

> > This is how it's done for Tegra210. The reason is that the USB IP blocks
> > can control the pll_u state in hw. This means that if sw would disable
> > and then re-enable the pll_u clock, but there is no USB activity, pll_u
> > will still be disable and therefor not lock, causing an error. Today
> > this is worked around by not polling the lock bit for pll_u, but a better
> > solution would be to just remove all sw controls for pll_u.
> 
> SW controls could be removed, but I don't think it is really necessary as in our
> case SW is the PHY driver and we know what it does. Alternatively we can enable
> PLL_U in the init table to keep it "always" enabled.

Re: [PATCH] clk: tegra: fix pllu rate configuration

[Dmitry Osipenko]

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

>>> This is how it's done for Tegra210. The reason is that the USB IP blocks
>>> can control the pll_u state in hw. This means that if sw would disable
>>> and then re-enable the pll_u clock, but there is no USB activity, pll_u
>>> will still be disable and therefor not lock, causing an error. Today
>>> this is worked around by not polling the lock bit for pll_u, but a better
>>> solution would be to just remove all sw controls for pll_u.
>>
>> SW controls could be removed, but I don't think it is really necessary as in our
>> case SW is the PHY driver and we know what it does. Alternatively we can enable
>> PLL_U in the init table to keep it "always" enabled.

Re: [PATCH] clk: tegra: fix pllu rate configuration

[Peter De Schrijver]

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

Peter.

Re: [PATCH] clk: tegra: fix pllu rate configuration

[Dmitry Osipenko]

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@apalis-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).

Re: [PATCH] clk: tegra: fix pllu rate configuration

[Dmitry Osipenko]

On 01.03.2018 16:19, Dmitry Osipenko wrote:
> 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@apalis-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.
I meant Marcel.

Re: [PATCH] clk: tegra: fix pllu rate configuration

[Jon Hunter]

On 01/03/18 07: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@apalis-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.

Peter, are you suggesting we implement the equivalent to
tegra210_init_pllu()? This does look a bit more involved that what this
change is doing. Is there a simple way to do what you are suggesting?

Cheers
Jon

-- 
nvpublic

Re: [PATCH] clk: tegra: fix pllu rate configuration

[Peter De Schrijver]

On Fri, Mar 02, 2018 at 09:02:07AM +0000, Jon Hunter wrote:
> 
> On 01/03/18 07: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@apalis-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.
> 
> Peter, are you suggesting we implement the equivalent to
> tegra210_init_pllu()? This does look a bit more involved that what this
> change is doing. Is there a simple way to do what you are suggesting?
> 

Basically yes.

Peter.

Re: [PATCH] clk: tegra: fix pllu rate configuration

[Jon Hunter]

On 22/02/18 23: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@apalis-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.
> 
> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> ---
> 
>  drivers/clk/tegra/clk-pll.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index 7c369e21c91c..830d1c87fa7c 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -1151,6 +1151,8 @@ static const struct clk_ops tegra_clk_pllu_ops = {
>  	.enable = clk_pllu_enable,
>  	.disable = clk_pll_disable,
>  	.recalc_rate = clk_pll_recalc_rate,
> +	.round_rate = clk_pll_round_rate,
> +	.set_rate = clk_pll_set_rate,
>  };
>  
>  static int _pll_fixed_mdiv(struct tegra_clk_pll_params *pll_params,

Thanks for the fix. I have tested this on Tegra30 cardhu and this
resolves a problem booting this board with the upstream uboot bootloader.

We just need to align with Peter on the best way to fix.

Cheers,
Jon

-- 
nvpublic

Re: [PATCH] clk: tegra: fix pllu rate configuration

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]

On Fri, Feb 23, 2018 at 12:04:51AM +0100, 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@apalis-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.
> 
> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> ---
> 
>  drivers/clk/tegra/clk-pll.c | 2 ++
>  1 file changed, 2 insertions(+)

I'm aware of the discussion about how best to implement this, but this
seems like a reasonable fix until we have something better.

Peter, Jon: if you want to fix this more properly, please take point on
this.

Applied this to for-4.17/clk, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]