[PATCH v1 0/4] Restore ULPI USB on Tegra20

[PATCH v1 0/4] Restore ULPI USB on Tegra20

[Dmitry Osipenko]

Hello,

This series of patches fixes ULPI USB on Tegra20. The original problem
was reported by Marcel Ziswiler, he found that "ulpi-link" clock was
incorrectly set to CDEV2 instead of PLL_P_OUT4. Marcel made a patch
that changed the "ulpi-link" clock to PLL_P_OUT4 and that fixed issue
with the USB for the devices that have CDEV2 being enabled by bootloader.
The patch got into the kernel and later Marc Dietrich found that USB
stopped working on the "paz00" Tegra20 board. After a bit of discussion
was revealed that PLL_P_OUT4 is the parent clock of the CDEV2 and clock
driver was setting CDEV2's parent incorrectly. The parent clock is actually
determined by the pinmuxing config of CDEV2 pingroup. This patchset fixes
the parent of CDEV2 clock by making Tegra's pinctrl driver a clock provider,
providing CDEV1/2 clock muxes (thanks to Peter De Schrijver for the
suggestion), and then setting these clock muxes as parents for the CDEV1/2
clocks. In the end Marcel's CDEV2->PLL_P_OUT4 change is reverted since CDEV2
(aka MCLK2) is the actual clock source for "ulpi-link".

Dmitry Osipenko (4):
  clk: tegra20: Add DEV1/DEV2 OSC dividers
  pinctrl: tegra20: Provide CDEV1/2 clock muxes
  clk: tegra20: Set correct parents for CDEV1/2 clocks
  ARM: dts: tegra20: Revert "Fix ULPI regression on Tegra20"

 arch/arm/boot/dts/tegra20.dtsi          |  2 +-
 drivers/clk/tegra/clk-tegra20.c         | 18 +++++++++++----
 drivers/pinctrl/tegra/pinctrl-tegra.c   | 11 ---------
 drivers/pinctrl/tegra/pinctrl-tegra.h   | 11 +++++++++
 drivers/pinctrl/tegra/pinctrl-tegra20.c | 30 ++++++++++++++++++++++++-
 5 files changed, 55 insertions(+), 17 deletions(-)

-- 
2.17.0

[PATCH v1 1/4] clk: tegra20: Add DEV1/DEV2 OSC dividers

[Dmitry Osipenko]

CDEV1/CDEV2 clocks could have corresponding oscillator clock divider as
a parent. Add these dividers in order to be able to provide that parent
option.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clk/tegra/clk-tegra20.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index 0ee56dd04cec..16cf4108f2ff 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -26,6 +26,8 @@
 #include "clk.h"
 #include "clk-id.h"
 
+#define MISC_CLK_ENB 0x48
+
 #define OSC_CTRL 0x50
 #define OSC_CTRL_OSC_FREQ_MASK (3<<30)
 #define OSC_CTRL_OSC_FREQ_13MHZ (0<<30)
@@ -831,6 +833,16 @@ static void __init tegra20_periph_clk_init(void)
 				    periph_clk_enb_refcnt);
 	clks[TEGRA20_CLK_PEX] = clk;
 
+	/* cdev1 OSC divider */
+	clk_register_divider(NULL, "cdev1_osc_div", "clk_m",
+			     0, clk_base + MISC_CLK_ENB, 20, 2,
+			     CLK_DIVIDER_POWER_OF_TWO, NULL);
+
+	/* cdev2 OSC divider */
+	clk_register_divider(NULL, "cdev2_osc_div", "clk_m",
+			     0, clk_base + MISC_CLK_ENB, 22, 2,
+			     CLK_DIVIDER_POWER_OF_TWO, NULL);
+
 	/* cdev1 */
 	clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL, 0, 26000000);
 	clk = tegra_clk_register_periph_gate("cdev1", "cdev1_fixed", 0,
-- 
2.17.0

Re: [PATCH v1 1/4] clk: tegra20: Add DEV1/DEV2 OSC dividers

[Marcel Ziswiler]

Hi Dmitry

Isn't the CLK_RST_CONTROLLER_MISC_CLK_ENB_0 the other way around e.g.
DEV1_OSC_DIV_SEL at bit 23:22 and DEV2_OSC_DIV_SEL at 21:20?

On Fri, 2018-04-27 at 02:58 +0300, Dmitry Osipenko wrote:
> CDEV1/CDEV2 clocks could have corresponding oscillator clock divider
> as
> a parent. Add these dividers in order to be able to provide that
> parent
> option.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/clk/tegra/clk-tegra20.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-
> tegra20.c
> index 0ee56dd04cec..16cf4108f2ff 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -26,6 +26,8 @@
>  #include "clk.h"
>  #include "clk-id.h"
>  
> +#define MISC_CLK_ENB 0x48
> +
>  #define OSC_CTRL 0x50
>  #define OSC_CTRL_OSC_FREQ_MASK (3<<30)
>  #define OSC_CTRL_OSC_FREQ_13MHZ (0<<30)
> @@ -831,6 +833,16 @@ static void __init tegra20_periph_clk_init(void)
>  				    periph_clk_enb_refcnt);
>  	clks[TEGRA20_CLK_PEX] = clk;
>  
> +	/* cdev1 OSC divider */
> +	clk_register_divider(NULL, "cdev1_osc_div", "clk_m",
> +			     0, clk_base + MISC_CLK_ENB, 20, 2,

So it would be:

+			     0, clk_base + MISC_CLK_ENB, 22, 2,

> +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
> +
> +	/* cdev2 OSC divider */
> +	clk_register_divider(NULL, "cdev2_osc_div", "clk_m",
> +			     0, clk_base + MISC_CLK_ENB, 22, 2,

And:

+			     0, clk_base + MISC_CLK_ENB, 20, 2,

> +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
> +
>  	/* cdev1 */
>  	clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL, 0,
> 26000000);
>  	clk = tegra_clk_register_periph_gate("cdev1", "cdev1_fixed",
> 0,

Cheers

Marcel

Re: [PATCH v1 1/4] clk: tegra20: Add DEV1/DEV2 OSC dividers

[Dmitry Osipenko]

Hi Marcel,

On 27.04.2018 15:33,  Ziswiler wrote:
> Hi Dmitry
> 
> Isn't the CLK_RST_CONTROLLER_MISC_CLK_ENB_0 the other way around e.g.
> DEV1_OSC_DIV_SEL at bit 23:22 and DEV2_OSC_DIV_SEL at 21:20?
> 
> On Fri, 2018-04-27 at 02:58 +0300, Dmitry Osipenko wrote:
>> CDEV1/CDEV2 clocks could have corresponding oscillator clock divider
>> as
>> a parent. Add these dividers in order to be able to provide that
>> parent
>> option.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/clk/tegra/clk-tegra20.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-
>> tegra20.c
>> index 0ee56dd04cec..16cf4108f2ff 100644
>> --- a/drivers/clk/tegra/clk-tegra20.c
>> +++ b/drivers/clk/tegra/clk-tegra20.c
>> @@ -26,6 +26,8 @@
>>  #include "clk.h"
>>  #include "clk-id.h"
>>  
>> +#define MISC_CLK_ENB 0x48
>> +
>>  #define OSC_CTRL 0x50
>>  #define OSC_CTRL_OSC_FREQ_MASK (3<<30)
>>  #define OSC_CTRL_OSC_FREQ_13MHZ (0<<30)
>> @@ -831,6 +833,16 @@ static void __init tegra20_periph_clk_init(void)
>>  				    periph_clk_enb_refcnt);
>>  	clks[TEGRA20_CLK_PEX] = clk;
>>  
>> +	/* cdev1 OSC divider */
>> +	clk_register_divider(NULL, "cdev1_osc_div", "clk_m",
>> +			     0, clk_base + MISC_CLK_ENB, 20, 2,
> 
> So it would be:
> 
> +			     0, clk_base + MISC_CLK_ENB, 22, 2,
> 
>> +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
>> +
>> +	/* cdev2 OSC divider */
>> +	clk_register_divider(NULL, "cdev2_osc_div", "clk_m",
>> +			     0, clk_base + MISC_CLK_ENB, 22, 2,
> 
> And:
> 
> +			     0, clk_base + MISC_CLK_ENB, 20, 2,
> 
>> +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
>> +
>>  	/* cdev1 */
>>  	clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL, 0,
>> 26000000);
>>  	clk = tegra_clk_register_periph_gate("cdev1", "cdev1_fixed",
>> 0,
Indeed, good catch! I'll wait for more comments and then re-spin patchset with
the fix. Thank you.

Re: [PATCH v1 1/4] clk: tegra20: Add DEV1/DEV2 OSC dividers

[Marcel Ziswiler]

On Fri, 2018-04-27 at 15:54 +0300, Dmitry Osipenko wrote:
> Hi Marcel,
> 
> On 27.04.2018 15:33,  Ziswiler wrote:
> > Hi Dmitry
> > 
> > Isn't the CLK_RST_CONTROLLER_MISC_CLK_ENB_0 the other way around
> > e.g.
> > DEV1_OSC_DIV_SEL at bit 23:22 and DEV2_OSC_DIV_SEL at 21:20?
> > 
> > On Fri, 2018-04-27 at 02:58 +0300, Dmitry Osipenko wrote:
> > > CDEV1/CDEV2 clocks could have corresponding oscillator clock
> > > divider
> > > as
> > > a parent. Add these dividers in order to be able to provide that
> > > parent
> > > option.
> > > 
> > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > > ---
> > >  drivers/clk/tegra/clk-tegra20.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/clk/tegra/clk-tegra20.c
> > > b/drivers/clk/tegra/clk-
> > > tegra20.c
> > > index 0ee56dd04cec..16cf4108f2ff 100644
> > > --- a/drivers/clk/tegra/clk-tegra20.c
> > > +++ b/drivers/clk/tegra/clk-tegra20.c
> > > @@ -26,6 +26,8 @@
> > >  #include "clk.h"
> > >  #include "clk-id.h"
> > >  
> > > +#define MISC_CLK_ENB 0x48
> > > +
> > >  #define OSC_CTRL 0x50
> > >  #define OSC_CTRL_OSC_FREQ_MASK (3<<30)
> > >  #define OSC_CTRL_OSC_FREQ_13MHZ (0<<30)
> > > @@ -831,6 +833,16 @@ static void __init
> > > tegra20_periph_clk_init(void)
> > >  				    periph_clk_enb_refcnt);
> > >  	clks[TEGRA20_CLK_PEX] = clk;
> > >  
> > > +	/* cdev1 OSC divider */
> > > +	clk_register_divider(NULL, "cdev1_osc_div", "clk_m",
> > > +			     0, clk_base + MISC_CLK_ENB, 20, 2,
> > 
> > So it would be:
> > 
> > +			     0, clk_base + MISC_CLK_ENB, 22, 2,
> > 
> > > +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
> > > +
> > > +	/* cdev2 OSC divider */
> > > +	clk_register_divider(NULL, "cdev2_osc_div", "clk_m",
> > > +			     0, clk_base + MISC_CLK_ENB, 22, 2,
> > 
> > And:
> > 
> > +			     0, clk_base + MISC_CLK_ENB, 20, 2,
> > 
> > > +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
> > > +
> > >  	/* cdev1 */
> > >  	clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL,
> > > 0,
> > > 26000000);
> > >  	clk = tegra_clk_register_periph_gate("cdev1",
> > > "cdev1_fixed",
> > > 0,
> 
> Indeed, good catch! I'll wait for more comments and then re-spin
> patchset with
> the fix. Thank you.

You are very welcome. Thank you!

Other than that it all looks proper and works fine at least in the
configuration we use on Colibri T20. So you may add my reviewed and
tested bys to the whole series:

Reviewed-by: Marcel Ziswiler <marcel@ziswiler.com>
Tested-by: Marcel Ziswiler <marcel@ziswiler.com>

Re: [PATCH v1 1/4] clk: tegra20: Add DEV1/DEV2 OSC dividers

[Dmitry Osipenko]

On 27.04.2018 16:00, Marcel Ziswiler wrote:
> On Fri, 2018-04-27 at 15:54 +0300, Dmitry Osipenko wrote:
>> Hi Marcel,
>>
>> On 27.04.2018 15:33,  Ziswiler wrote:
>>> Hi Dmitry
>>>
>>> Isn't the CLK_RST_CONTROLLER_MISC_CLK_ENB_0 the other way around
>>> e.g.
>>> DEV1_OSC_DIV_SEL at bit 23:22 and DEV2_OSC_DIV_SEL at 21:20?
>>>
>>> On Fri, 2018-04-27 at 02:58 +0300, Dmitry Osipenko wrote:
>>>> CDEV1/CDEV2 clocks could have corresponding oscillator clock
>>>> divider
>>>> as
>>>> a parent. Add these dividers in order to be able to provide that
>>>> parent
>>>> option.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  drivers/clk/tegra/clk-tegra20.c | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/tegra/clk-tegra20.c
>>>> b/drivers/clk/tegra/clk-
>>>> tegra20.c
>>>> index 0ee56dd04cec..16cf4108f2ff 100644
>>>> --- a/drivers/clk/tegra/clk-tegra20.c
>>>> +++ b/drivers/clk/tegra/clk-tegra20.c
>>>> @@ -26,6 +26,8 @@
>>>>  #include "clk.h"
>>>>  #include "clk-id.h"
>>>>  
>>>> +#define MISC_CLK_ENB 0x48
>>>> +
>>>>  #define OSC_CTRL 0x50
>>>>  #define OSC_CTRL_OSC_FREQ_MASK (3<<30)
>>>>  #define OSC_CTRL_OSC_FREQ_13MHZ (0<<30)
>>>> @@ -831,6 +833,16 @@ static void __init
>>>> tegra20_periph_clk_init(void)
>>>>  				    periph_clk_enb_refcnt);
>>>>  	clks[TEGRA20_CLK_PEX] = clk;
>>>>  
>>>> +	/* cdev1 OSC divider */
>>>> +	clk_register_divider(NULL, "cdev1_osc_div", "clk_m",
>>>> +			     0, clk_base + MISC_CLK_ENB, 20, 2,
>>>
>>> So it would be:
>>>
>>> +			     0, clk_base + MISC_CLK_ENB, 22, 2,
>>>
>>>> +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
>>>> +
>>>> +	/* cdev2 OSC divider */
>>>> +	clk_register_divider(NULL, "cdev2_osc_div", "clk_m",
>>>> +			     0, clk_base + MISC_CLK_ENB, 22, 2,
>>>
>>> And:
>>>
>>> +			     0, clk_base + MISC_CLK_ENB, 20, 2,
>>>
>>>> +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
>>>> +
>>>>  	/* cdev1 */
>>>>  	clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL,
>>>> 0,
>>>> 26000000);
>>>>  	clk = tegra_clk_register_periph_gate("cdev1",
>>>> "cdev1_fixed",
>>>> 0,
>>
>> Indeed, good catch! I'll wait for more comments and then re-spin
>> patchset with
>> the fix. Thank you.
> 
> You are very welcome. Thank you!
> 
> Other than that it all looks proper and works fine at least in the
> configuration we use on Colibri T20. So you may add my reviewed and
> tested bys to the whole series:
> 
> Reviewed-by: Marcel Ziswiler <marcel@ziswiler.com>
> Tested-by: Marcel Ziswiler <marcel@ziswiler.com>

Marcel, you previously mentioned that reverting of your DT patch works for the
Colibri now. Does that reverting also work for the 4.17 kernel? If yes, I may
add stable tag to the revert-patch to get back paz00 working with 4.17. If it's
not working, we should figure out why pll_p_out4 is getting disabled as it
should be a distinct issue.

Re: [PATCH v1 1/4] clk: tegra20: Add DEV1/DEV2 OSC dividers

[Dmitry Osipenko]

On 03.05.2018 14:59, Dmitry Osipenko wrote:
> On 27.04.2018 16:00, Marcel Ziswiler wrote:
>> On Fri, 2018-04-27 at 15:54 +0300, Dmitry Osipenko wrote:
>>> Hi Marcel,
>>>
>>> On 27.04.2018 15:33,  Ziswiler wrote:
>>>> Hi Dmitry
>>>>
>>>> Isn't the CLK_RST_CONTROLLER_MISC_CLK_ENB_0 the other way around
>>>> e.g.
>>>> DEV1_OSC_DIV_SEL at bit 23:22 and DEV2_OSC_DIV_SEL at 21:20?
>>>>
>>>> On Fri, 2018-04-27 at 02:58 +0300, Dmitry Osipenko wrote:
>>>>> CDEV1/CDEV2 clocks could have corresponding oscillator clock
>>>>> divider
>>>>> as
>>>>> a parent. Add these dividers in order to be able to provide that
>>>>> parent
>>>>> option.
>>>>>
>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> ---
>>>>>  drivers/clk/tegra/clk-tegra20.c | 12 ++++++++++++
>>>>>  1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/tegra/clk-tegra20.c
>>>>> b/drivers/clk/tegra/clk-
>>>>> tegra20.c
>>>>> index 0ee56dd04cec..16cf4108f2ff 100644
>>>>> --- a/drivers/clk/tegra/clk-tegra20.c
>>>>> +++ b/drivers/clk/tegra/clk-tegra20.c
>>>>> @@ -26,6 +26,8 @@
>>>>>  #include "clk.h"
>>>>>  #include "clk-id.h"
>>>>>  
>>>>> +#define MISC_CLK_ENB 0x48
>>>>> +
>>>>>  #define OSC_CTRL 0x50
>>>>>  #define OSC_CTRL_OSC_FREQ_MASK (3<<30)
>>>>>  #define OSC_CTRL_OSC_FREQ_13MHZ (0<<30)
>>>>> @@ -831,6 +833,16 @@ static void __init
>>>>> tegra20_periph_clk_init(void)
>>>>>  				    periph_clk_enb_refcnt);
>>>>>  	clks[TEGRA20_CLK_PEX] = clk;
>>>>>  
>>>>> +	/* cdev1 OSC divider */
>>>>> +	clk_register_divider(NULL, "cdev1_osc_div", "clk_m",
>>>>> +			     0, clk_base + MISC_CLK_ENB, 20, 2,
>>>>
>>>> So it would be:
>>>>
>>>> +			     0, clk_base + MISC_CLK_ENB, 22, 2,
>>>>
>>>>> +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
>>>>> +
>>>>> +	/* cdev2 OSC divider */
>>>>> +	clk_register_divider(NULL, "cdev2_osc_div", "clk_m",
>>>>> +			     0, clk_base + MISC_CLK_ENB, 22, 2,
>>>>
>>>> And:
>>>>
>>>> +			     0, clk_base + MISC_CLK_ENB, 20, 2,
>>>>
>>>>> +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
>>>>> +
>>>>>  	/* cdev1 */
>>>>>  	clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL,
>>>>> 0,
>>>>> 26000000);
>>>>>  	clk = tegra_clk_register_periph_gate("cdev1",
>>>>> "cdev1_fixed",
>>>>> 0,
>>>
>>> Indeed, good catch! I'll wait for more comments and then re-spin
>>> patchset with
>>> the fix. Thank you.
>>
>> You are very welcome. Thank you!
>>
>> Other than that it all looks proper and works fine at least in the
>> configuration we use on Colibri T20. So you may add my reviewed and
>> tested bys to the whole series:
>>
>> Reviewed-by: Marcel Ziswiler <marcel@ziswiler.com>
>> Tested-by: Marcel Ziswiler <marcel@ziswiler.com>
> 
> Marcel, you previously mentioned that reverting of your DT patch works for the
> Colibri now. Does that reverting also work for the 4.17 kernel? If yes, I may
> add stable tag to the revert-patch to get back paz00 working with 4.17. If it's
> not working, we should figure out why pll_p_out4 is getting disabled as it
> should be a distinct issue.

Oh, I just found that Thierry already asked to get this patches included into
4.17. Anyway would be nice to know if a sole DT revert works with 4.17 for you.

Re: [PATCH v1 1/4] clk: tegra20: Add DEV1/DEV2 OSC dividers

[Marcel Ziswiler]

On Thu, 2018-05-03 at 15:02 +0300, Dmitry Osipenko wrote:
> ...
> > Marcel, you previously mentioned that reverting of your DT patch
> > works for the
> > Colibri now. Does that reverting also work for the 4.17 kernel? If
> > yes, I may
> > add stable tag to the revert-patch to get back paz00 working with
> > 4.17. If it's
> > not working, we should figure out why pll_p_out4 is getting
> > disabled as it
> > should be a distinct issue.
> 
> Oh, I just found that Thierry already asked to get this patches
> included into
> 4.17. Anyway would be nice to know if a sole DT revert works with
> 4.17 for you.

Yes, 4.17.0-rc3 with just 4c9a27a6c66d4427f3cba4019d4ba738fe99fa87
reverted works fine as well.

Re: [PATCH v1 1/4] clk: tegra20: Add DEV1/DEV2 OSC dividers

[Dmitry Osipenko]

On 03.05.2018 15:30, Marcel Ziswiler wrote:
> On Thu, 2018-05-03 at 15:02 +0300, Dmitry Osipenko wrote:
>> ...
>>> Marcel, you previously mentioned that reverting of your DT patch
>>> works for the
>>> Colibri now. Does that reverting also work for the 4.17 kernel? If
>>> yes, I may
>>> add stable tag to the revert-patch to get back paz00 working with
>>> 4.17. If it's
>>> not working, we should figure out why pll_p_out4 is getting
>>> disabled as it
>>> should be a distinct issue.
>>
>> Oh, I just found that Thierry already asked to get this patches
>> included into
>> 4.17. Anyway would be nice to know if a sole DT revert works with
>> 4.17 for you.
> 
> Yes, 4.17.0-rc3 with just 4c9a27a6c66d4427f3cba4019d4ba738fe99fa87
> reverted works fine as well.

Okay, thank you very much. So pll_p_out4 disabling was either an intermediate
issue that got fixed or it is masked by something else now.

Re: [PATCH v1 1/4] clk: tegra20: Add DEV1/DEV2 OSC dividers

[Peter De Schrijver]

On Fri, Apr 27, 2018 at 02:58:15AM +0300, Dmitry Osipenko wrote:
> CDEV1/CDEV2 clocks could have corresponding oscillator clock divider as
> a parent. Add these dividers in order to be able to provide that parent
> option.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/clk/tegra/clk-tegra20.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 0ee56dd04cec..16cf4108f2ff 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -26,6 +26,8 @@
>  #include "clk.h"
>  #include "clk-id.h"
>  
> +#define MISC_CLK_ENB 0x48
> +
>  #define OSC_CTRL 0x50
>  #define OSC_CTRL_OSC_FREQ_MASK (3<<30)
>  #define OSC_CTRL_OSC_FREQ_13MHZ (0<<30)
> @@ -831,6 +833,16 @@ static void __init tegra20_periph_clk_init(void)
>  				    periph_clk_enb_refcnt);
>  	clks[TEGRA20_CLK_PEX] = clk;
>  
> +	/* cdev1 OSC divider */
> +	clk_register_divider(NULL, "cdev1_osc_div", "clk_m",
> +			     0, clk_base + MISC_CLK_ENB, 20, 2,
> +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
> +

I don't know if this divider can be changed glitchlessly so to be safe,
I would mark this readonly, so add the CLK_DIVIDER_READ_ONLY flag.

> +	/* cdev2 OSC divider */
> +	clk_register_divider(NULL, "cdev2_osc_div", "clk_m",
> +			     0, clk_base + MISC_CLK_ENB, 22, 2,
> +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
> +
>  	/* cdev1 */
>  	clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL, 0, 26000000);
>  	clk = tegra_clk_register_periph_gate("cdev1", "cdev1_fixed", 0,
> -- 
> 2.17.0
> 

Peter.

Re: [PATCH v1 1/4] clk: tegra20: Add DEV1/DEV2 OSC dividers

[Peter De Schrijver]

On Fri, Apr 27, 2018 at 02:58:15AM +0300, Dmitry Osipenko wrote:
> CDEV1/CDEV2 clocks could have corresponding oscillator clock divider as
> a parent. Add these dividers in order to be able to provide that parent
> option.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/clk/tegra/clk-tegra20.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 0ee56dd04cec..16cf4108f2ff 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -26,6 +26,8 @@
>  #include "clk.h"
>  #include "clk-id.h"
>  
> +#define MISC_CLK_ENB 0x48
> +
>  #define OSC_CTRL 0x50
>  #define OSC_CTRL_OSC_FREQ_MASK (3<<30)
>  #define OSC_CTRL_OSC_FREQ_13MHZ (0<<30)
> @@ -831,6 +833,16 @@ static void __init tegra20_periph_clk_init(void)
>  				    periph_clk_enb_refcnt);
>  	clks[TEGRA20_CLK_PEX] = clk;
>  
> +	/* cdev1 OSC divider */
> +	clk_register_divider(NULL, "cdev1_osc_div", "clk_m",
> +			     0, clk_base + MISC_CLK_ENB, 20, 2,
> +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
> +

I don't know if this divider can be changed glitchlessly so to be safe,
I would mark this readonly, so add the CLK_DIVIDER_READ_ONLY flag.

> +	/* cdev2 OSC divider */
> +	clk_register_divider(NULL, "cdev2_osc_div", "clk_m",
> +			     0, clk_base + MISC_CLK_ENB, 22, 2,
> +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
> +
>  	/* cdev1 */
>  	clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL, 0, 26000000);
>  	clk = tegra_clk_register_periph_gate("cdev1", "cdev1_fixed", 0,
> -- 
> 2.17.0
> 

Peter.

[PATCH v1 2/4] pinctrl: tegra20: Provide CDEV1/2 clock muxes

[Dmitry Osipenko]

Muxing of pins MCLK1/2 determine the muxing of the corresponding clocks.
Make pinctrl driver to provide clock muxes for the CDEV1/2 pingroups, so
that main clk-controller driver could get an actual parent clock for the
CDEV1/2 clocks.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/pinctrl/tegra/pinctrl-tegra.c   | 11 ---------
 drivers/pinctrl/tegra/pinctrl-tegra.h   | 11 +++++++++
 drivers/pinctrl/tegra/pinctrl-tegra20.c | 30 ++++++++++++++++++++++++-
 3 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
index 72c718e66ebb..49c7c1499bc3 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
@@ -33,17 +33,6 @@
 #include "../pinctrl-utils.h"
 #include "pinctrl-tegra.h"
 
-struct tegra_pmx {
-	struct device *dev;
-	struct pinctrl_dev *pctl;
-
-	const struct tegra_pinctrl_soc_data *soc;
-	const char **group_pins;
-
-	int nbanks;
-	void __iomem **regs;
-};
-
 static inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
 {
 	return readl(pmx->regs[bank] + reg);
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.h b/drivers/pinctrl/tegra/pinctrl-tegra.h
index 33b17cb1471e..aa33c20766c4 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra.h
+++ b/drivers/pinctrl/tegra/pinctrl-tegra.h
@@ -16,6 +16,17 @@
 #ifndef __PINMUX_TEGRA_H__
 #define __PINMUX_TEGRA_H__
 
+struct tegra_pmx {
+	struct device *dev;
+	struct pinctrl_dev *pctl;
+
+	const struct tegra_pinctrl_soc_data *soc;
+	const char **group_pins;
+
+	int nbanks;
+	void __iomem **regs;
+};
+
 enum tegra_pinconf_param {
 	/* argument: tegra_pinconf_pull */
 	TEGRA_PINCONF_PARAM_PULL,
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra20.c b/drivers/pinctrl/tegra/pinctrl-tegra20.c
index 7e38ee9bae78..f31e39d797f9 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra20.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra20.c
@@ -19,6 +19,7 @@
  * more details.
  */
 
+#include <linux/clk-provider.h>
 #include <linux/init.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -2231,9 +2232,36 @@ static const struct tegra_pinctrl_soc_data tegra20_pinctrl = {
 	.drvtype_in_mux = false,
 };
 
+static const char *cdev1_parents[4] = {
+	"cdev1_osc_div", "pll_a_out0", "pll_m_out1", "audio",
+};
+
+static const char *cdev2_parents[4] = {
+	"cdev2_osc_div", "hclk", "pclk", "pll_p_out4",
+};
+
+static void tegra20_pinctrl_register_clock_muxes(struct platform_device *pdev)
+{
+	struct tegra_pmx *pmx = platform_get_drvdata(pdev);
+
+	clk_register_mux(NULL, "cdev1_mux", cdev1_parents, 4, 0,
+			 pmx->regs[1] + 0x8, 2, 2, CLK_MUX_READ_ONLY, NULL);
+
+	clk_register_mux(NULL, "cdev2_mux", cdev2_parents, 4, 0,
+			 pmx->regs[1] + 0x8, 4, 2, CLK_MUX_READ_ONLY, NULL);
+}
+
 static int tegra20_pinctrl_probe(struct platform_device *pdev)
 {
-	return tegra_pinctrl_probe(pdev, &tegra20_pinctrl);
+	int err;
+
+	err = tegra_pinctrl_probe(pdev, &tegra20_pinctrl);
+	if (err)
+		return err;
+
+	tegra20_pinctrl_register_clock_muxes(pdev);
+
+	return 0;
 }
 
 static const struct of_device_id tegra20_pinctrl_of_match[] = {
-- 
2.17.0

Re: [PATCH v1 2/4] pinctrl: tegra20: Provide CDEV1/2 clock muxes

[Peter De Schrijver]

On Fri, Apr 27, 2018 at 02:58:16AM +0300, Dmitry Osipenko wrote:
> Muxing of pins MCLK1/2 determine the muxing of the corresponding clocks.
> Make pinctrl driver to provide clock muxes for the CDEV1/2 pingroups, so
> that main clk-controller driver could get an actual parent clock for the
> CDEV1/2 clocks.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/pinctrl/tegra/pinctrl-tegra.c   | 11 ---------
>  drivers/pinctrl/tegra/pinctrl-tegra.h   | 11 +++++++++
>  drivers/pinctrl/tegra/pinctrl-tegra20.c | 30 ++++++++++++++++++++++++-
>  3 files changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
> index 72c718e66ebb..49c7c1499bc3 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> @@ -33,17 +33,6 @@
>  #include "../pinctrl-utils.h"
>  #include "pinctrl-tegra.h"
>  
> -struct tegra_pmx {
> -	struct device *dev;
> -	struct pinctrl_dev *pctl;
> -
> -	const struct tegra_pinctrl_soc_data *soc;
> -	const char **group_pins;
> -
> -	int nbanks;
> -	void __iomem **regs;
> -};
> -
>  static inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
>  {
>  	return readl(pmx->regs[bank] + reg);
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.h b/drivers/pinctrl/tegra/pinctrl-tegra.h
> index 33b17cb1471e..aa33c20766c4 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.h
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.h
> @@ -16,6 +16,17 @@
>  #ifndef __PINMUX_TEGRA_H__
>  #define __PINMUX_TEGRA_H__
>  
> +struct tegra_pmx {
> +	struct device *dev;
> +	struct pinctrl_dev *pctl;
> +
> +	const struct tegra_pinctrl_soc_data *soc;
> +	const char **group_pins;
> +
> +	int nbanks;
> +	void __iomem **regs;
> +};
> +
>  enum tegra_pinconf_param {
>  	/* argument: tegra_pinconf_pull */
>  	TEGRA_PINCONF_PARAM_PULL,
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra20.c b/drivers/pinctrl/tegra/pinctrl-tegra20.c
> index 7e38ee9bae78..f31e39d797f9 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra20.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra20.c
> @@ -19,6 +19,7 @@
>   * more details.
>   */
>  
> +#include <linux/clk-provider.h>
>  #include <linux/init.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -2231,9 +2232,36 @@ static const struct tegra_pinctrl_soc_data tegra20_pinctrl = {
>  	.drvtype_in_mux = false,
>  };
>  
> +static const char *cdev1_parents[4] = {

No need to have 4 here, just cdev1_parents[] will do. 

> +	"cdev1_osc_div", "pll_a_out0", "pll_m_out1", "audio",
> +};
> +
> +static const char *cdev2_parents[4] = {
> +	"cdev2_osc_div", "hclk", "pclk", "pll_p_out4",
> +};
> +

Same here.

> +static void tegra20_pinctrl_register_clock_muxes(struct platform_device *pdev)
> +{
> +	struct tegra_pmx *pmx = platform_get_drvdata(pdev);
> +
> +	clk_register_mux(NULL, "cdev1_mux", cdev1_parents, 4, 0,
> +			 pmx->regs[1] + 0x8, 2, 2, CLK_MUX_READ_ONLY, NULL);
> +
> +	clk_register_mux(NULL, "cdev2_mux", cdev2_parents, 4, 0,
> +			 pmx->regs[1] + 0x8, 4, 2, CLK_MUX_READ_ONLY, NULL);
> +}
> +
>  static int tegra20_pinctrl_probe(struct platform_device *pdev)
>  {
> -	return tegra_pinctrl_probe(pdev, &tegra20_pinctrl);
> +	int err;
> +
> +	err = tegra_pinctrl_probe(pdev, &tegra20_pinctrl);
> +	if (err)
> +		return err;
> +
> +	tegra20_pinctrl_register_clock_muxes(pdev);
> +
> +	return 0;
>  }
>  
>  static const struct of_device_id tegra20_pinctrl_of_match[] = {

Apart from these nitpicks:

Acked-By:  Peter De Schrijver <pdeschrijver@nvidia.com>

Re: [PATCH v1 2/4] pinctrl: tegra20: Provide CDEV1/2 clock muxes

[Peter De Schrijver]

On Fri, Apr 27, 2018 at 02:58:16AM +0300, Dmitry Osipenko wrote:
> Muxing of pins MCLK1/2 determine the muxing of the corresponding clocks.
> Make pinctrl driver to provide clock muxes for the CDEV1/2 pingroups, so
> that main clk-controller driver could get an actual parent clock for the
> CDEV1/2 clocks.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/pinctrl/tegra/pinctrl-tegra.c   | 11 ---------
>  drivers/pinctrl/tegra/pinctrl-tegra.h   | 11 +++++++++
>  drivers/pinctrl/tegra/pinctrl-tegra20.c | 30 ++++++++++++++++++++++++-
>  3 files changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
> index 72c718e66ebb..49c7c1499bc3 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> @@ -33,17 +33,6 @@
>  #include "../pinctrl-utils.h"
>  #include "pinctrl-tegra.h"
>  
> -struct tegra_pmx {
> -	struct device *dev;
> -	struct pinctrl_dev *pctl;
> -
> -	const struct tegra_pinctrl_soc_data *soc;
> -	const char **group_pins;
> -
> -	int nbanks;
> -	void __iomem **regs;
> -};
> -
>  static inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
>  {
>  	return readl(pmx->regs[bank] + reg);
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.h b/drivers/pinctrl/tegra/pinctrl-tegra.h
> index 33b17cb1471e..aa33c20766c4 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.h
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.h
> @@ -16,6 +16,17 @@
>  #ifndef __PINMUX_TEGRA_H__
>  #define __PINMUX_TEGRA_H__
>  
> +struct tegra_pmx {
> +	struct device *dev;
> +	struct pinctrl_dev *pctl;
> +
> +	const struct tegra_pinctrl_soc_data *soc;
> +	const char **group_pins;
> +
> +	int nbanks;
> +	void __iomem **regs;
> +};
> +
>  enum tegra_pinconf_param {
>  	/* argument: tegra_pinconf_pull */
>  	TEGRA_PINCONF_PARAM_PULL,
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra20.c b/drivers/pinctrl/tegra/pinctrl-tegra20.c
> index 7e38ee9bae78..f31e39d797f9 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra20.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra20.c
> @@ -19,6 +19,7 @@
>   * more details.
>   */
>  
> +#include <linux/clk-provider.h>
>  #include <linux/init.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -2231,9 +2232,36 @@ static const struct tegra_pinctrl_soc_data tegra20_pinctrl = {
>  	.drvtype_in_mux = false,
>  };
>  
> +static const char *cdev1_parents[4] = {

No need to have 4 here, just cdev1_parents[] will do. 

> +	"cdev1_osc_div", "pll_a_out0", "pll_m_out1", "audio",
> +};
> +
> +static const char *cdev2_parents[4] = {
> +	"cdev2_osc_div", "hclk", "pclk", "pll_p_out4",
> +};
> +

Same here.

> +static void tegra20_pinctrl_register_clock_muxes(struct platform_device *pdev)
> +{
> +	struct tegra_pmx *pmx = platform_get_drvdata(pdev);
> +
> +	clk_register_mux(NULL, "cdev1_mux", cdev1_parents, 4, 0,
> +			 pmx->regs[1] + 0x8, 2, 2, CLK_MUX_READ_ONLY, NULL);
> +
> +	clk_register_mux(NULL, "cdev2_mux", cdev2_parents, 4, 0,
> +			 pmx->regs[1] + 0x8, 4, 2, CLK_MUX_READ_ONLY, NULL);
> +}
> +
>  static int tegra20_pinctrl_probe(struct platform_device *pdev)
>  {
> -	return tegra_pinctrl_probe(pdev, &tegra20_pinctrl);
> +	int err;
> +
> +	err = tegra_pinctrl_probe(pdev, &tegra20_pinctrl);
> +	if (err)
> +		return err;
> +
> +	tegra20_pinctrl_register_clock_muxes(pdev);
> +
> +	return 0;
>  }
>  
>  static const struct of_device_id tegra20_pinctrl_of_match[] = {

Apart from these nitpicks:

Acked-By:  Peter De Schrijver <pdeschrijver@nvidia.com>

Re: [PATCH v1 2/4] pinctrl: tegra20: Provide CDEV1/2 clock muxes

[Linus Walleij]

On Fri, Apr 27, 2018 at 1:58 AM, Dmitry Osipenko <digetx@gmail.com> wrote:

> Muxing of pins MCLK1/2 determine the muxing of the corresponding clocks.
> Make pinctrl driver to provide clock muxes for the CDEV1/2 pingroups, so
> that main clk-controller driver could get an actual parent clock for the
> CDEV1/2 clocks.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

If this can be applied separately from the other patches, then please
send it separately after fixing Peter's review nits and adding his
review-tag, thanks!

Yours,
Linus Walleij

[PATCH v1 3/4] clk: tegra20: Set correct parents for CDEV1/2 clocks

[Dmitry Osipenko]

Parents of CDEV1/2 clocks are determined by muxing of the corresponding
pins. Pinctrl driver now provides the CDEV1/2 clock muxes and hence
CDEV1/2 clocks could have correct parents. Set CDEV1/2 parents to the
corresponding muxes to fix the parents.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clk/tegra/clk-tegra20.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index 16cf4108f2ff..7e8b6de86d89 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -844,14 +844,12 @@ static void __init tegra20_periph_clk_init(void)
 			     CLK_DIVIDER_POWER_OF_TWO, NULL);
 
 	/* cdev1 */
-	clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL, 0, 26000000);
-	clk = tegra_clk_register_periph_gate("cdev1", "cdev1_fixed", 0,
+	clk = tegra_clk_register_periph_gate("cdev1", "cdev1_mux", 0,
 				    clk_base, 0, 94, periph_clk_enb_refcnt);
 	clks[TEGRA20_CLK_CDEV1] = clk;
 
 	/* cdev2 */
-	clk = clk_register_fixed_rate(NULL, "cdev2_fixed", NULL, 0, 26000000);
-	clk = tegra_clk_register_periph_gate("cdev2", "cdev2_fixed", 0,
+	clk = tegra_clk_register_periph_gate("cdev2", "cdev2_mux", 0,
 				    clk_base, 0, 93, periph_clk_enb_refcnt);
 	clks[TEGRA20_CLK_CDEV2] = clk;
 
-- 
2.17.0

Re: [PATCH v1 3/4] clk: tegra20: Set correct parents for CDEV1/2 clocks

[Peter De Schrijver]

On Fri, Apr 27, 2018 at 02:58:17AM +0300, Dmitry Osipenko wrote:
> Parents of CDEV1/2 clocks are determined by muxing of the corresponding
> pins. Pinctrl driver now provides the CDEV1/2 clock muxes and hence
> CDEV1/2 clocks could have correct parents. Set CDEV1/2 parents to the
> corresponding muxes to fix the parents.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Acked-By:  Peter De Schrijver <pdeschrijver@nvidia.com>

> ---
>  drivers/clk/tegra/clk-tegra20.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 16cf4108f2ff..7e8b6de86d89 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -844,14 +844,12 @@ static void __init tegra20_periph_clk_init(void)
>  			     CLK_DIVIDER_POWER_OF_TWO, NULL);
>  
>  	/* cdev1 */
> -	clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL, 0, 26000000);
> -	clk = tegra_clk_register_periph_gate("cdev1", "cdev1_fixed", 0,
> +	clk = tegra_clk_register_periph_gate("cdev1", "cdev1_mux", 0,
>  				    clk_base, 0, 94, periph_clk_enb_refcnt);
>  	clks[TEGRA20_CLK_CDEV1] = clk;
>  
>  	/* cdev2 */
> -	clk = clk_register_fixed_rate(NULL, "cdev2_fixed", NULL, 0, 26000000);
> -	clk = tegra_clk_register_periph_gate("cdev2", "cdev2_fixed", 0,
> +	clk = tegra_clk_register_periph_gate("cdev2", "cdev2_mux", 0,
>  				    clk_base, 0, 93, periph_clk_enb_refcnt);
>  	clks[TEGRA20_CLK_CDEV2] = clk;
>  
> -- 
> 2.17.0
> 

Re: [PATCH v1 3/4] clk: tegra20: Set correct parents for CDEV1/2 clocks

[Peter De Schrijver]

On Fri, Apr 27, 2018 at 02:58:17AM +0300, Dmitry Osipenko wrote:
> Parents of CDEV1/2 clocks are determined by muxing of the corresponding
> pins. Pinctrl driver now provides the CDEV1/2 clock muxes and hence
> CDEV1/2 clocks could have correct parents. Set CDEV1/2 parents to the
> corresponding muxes to fix the parents.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Acked-By:  Peter De Schrijver <pdeschrijver@nvidia.com>

> ---
>  drivers/clk/tegra/clk-tegra20.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 16cf4108f2ff..7e8b6de86d89 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -844,14 +844,12 @@ static void __init tegra20_periph_clk_init(void)
>  			     CLK_DIVIDER_POWER_OF_TWO, NULL);
>  
>  	/* cdev1 */
> -	clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL, 0, 26000000);
> -	clk = tegra_clk_register_periph_gate("cdev1", "cdev1_fixed", 0,
> +	clk = tegra_clk_register_periph_gate("cdev1", "cdev1_mux", 0,
>  				    clk_base, 0, 94, periph_clk_enb_refcnt);
>  	clks[TEGRA20_CLK_CDEV1] = clk;
>  
>  	/* cdev2 */
> -	clk = clk_register_fixed_rate(NULL, "cdev2_fixed", NULL, 0, 26000000);
> -	clk = tegra_clk_register_periph_gate("cdev2", "cdev2_fixed", 0,
> +	clk = tegra_clk_register_periph_gate("cdev2", "cdev2_mux", 0,
>  				    clk_base, 0, 93, periph_clk_enb_refcnt);
>  	clks[TEGRA20_CLK_CDEV2] = clk;
>  
> -- 
> 2.17.0
> 

Re: [PATCH v1 3/4] clk: tegra20: Set correct parents for CDEV1/2 clocks

[Stephen Boyd]

Quoting Dmitry Osipenko (2018-04-26 16:58:17)
> Parents of CDEV1/2 clocks are determined by muxing of the corresponding
> pins. Pinctrl driver now provides the CDEV1/2 clock muxes and hence
> CDEV1/2 clocks could have correct parents. Set CDEV1/2 parents to the
> corresponding muxes to fix the parents.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

Re: [PATCH v1 3/4] clk: tegra20: Set correct parents for CDEV1/2 clocks

[Stephen Boyd]

Quoting Dmitry Osipenko (2018-04-26 16:58:17)
> Parents of CDEV1/2 clocks are determined by muxing of the corresponding
> pins. Pinctrl driver now provides the CDEV1/2 clock muxes and hence
> CDEV1/2 clocks could have correct parents. Set CDEV1/2 parents to the
> corresponding muxes to fix the parents.
> =

> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

[PATCH v1 4/4] ARM: dts: tegra20: Revert "Fix ULPI regression on Tegra20"

[Dmitry Osipenko]

Commit 4c9a27a6c66d ("ARM: tegra: Fix ULPI regression on Tegra20") changed
"ulpi-link" clock from CDEV2 to PLL_P_OUT4. Turned out that PLL_P_OUT4 is
the parent of CDEV2 clock and original clock setup of "ulpi-link" was
correct. The reverted patch was fixing USB for one board and broke the
other, now Tegra's clk driver correctly sets parent for the CDEV2 clock
and hence patch could be reverted safely, restoring USB for all of the
boards.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 arch/arm/boot/dts/tegra20.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 0a7136462a1a..983dd5c14794 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -741,7 +741,7 @@
 		phy_type = "ulpi";
 		clocks = <&tegra_car TEGRA20_CLK_USB2>,
 			 <&tegra_car TEGRA20_CLK_PLL_U>,
-			 <&tegra_car TEGRA20_CLK_PLL_P_OUT4>;
+			 <&tegra_car TEGRA20_CLK_CDEV2>;
 		clock-names = "reg", "pll_u", "ulpi-link";
 		resets = <&tegra_car 58>, <&tegra_car 22>;
 		reset-names = "usb", "utmi-pads";
-- 
2.17.0

Re: [PATCH v1 0/4] Restore ULPI USB on Tegra20

[Marc Dietrich]

Hi Dmitry,

Am Freitag, 27. April 2018, 01:58:14 CEST schrieb Dmitry Osipenko:
> Hello,
> 
> This series of patches fixes ULPI USB on Tegra20. The original problem
> was reported by Marcel Ziswiler, he found that "ulpi-link" clock was
> incorrectly set to CDEV2 instead of PLL_P_OUT4. Marcel made a patch
> that changed the "ulpi-link" clock to PLL_P_OUT4 and that fixed issue
> with the USB for the devices that have CDEV2 being enabled by bootloader.
> The patch got into the kernel and later Marc Dietrich found that USB
> stopped working on the "paz00" Tegra20 board. After a bit of discussion
> was revealed that PLL_P_OUT4 is the parent clock of the CDEV2 and clock
> driver was setting CDEV2's parent incorrectly. The parent clock is actually
> determined by the pinmuxing config of CDEV2 pingroup. This patchset fixes
> the parent of CDEV2 clock by making Tegra's pinctrl driver a clock provider,
> providing CDEV1/2 clock muxes (thanks to Peter De Schrijver for the
> suggestion), and then setting these clock muxes as parents for the CDEV1/2
> clocks. In the end Marcel's CDEV2->PLL_P_OUT4 change is reverted since
> CDEV2 (aka MCLK2) is the actual clock source for "ulpi-link".
> 
> Dmitry Osipenko (4):
>   clk: tegra20: Add DEV1/DEV2 OSC dividers
>   pinctrl: tegra20: Provide CDEV1/2 clock muxes
>   clk: tegra20: Set correct parents for CDEV1/2 clocks
>   ARM: dts: tegra20: Revert "Fix ULPI regression on Tegra20"
> 
>  arch/arm/boot/dts/tegra20.dtsi          |  2 +-
>  drivers/clk/tegra/clk-tegra20.c         | 18 +++++++++++----
>  drivers/pinctrl/tegra/pinctrl-tegra.c   | 11 ---------
>  drivers/pinctrl/tegra/pinctrl-tegra.h   | 11 +++++++++
>  drivers/pinctrl/tegra/pinctrl-tegra20.c | 30 ++++++++++++++++++++++++-
>  5 files changed, 55 insertions(+), 17 deletions(-)

Thanks for taking a look at this!

Series is:

Tested-by: Marc Dietrich <marvin24@gmx.de>

Marc

Re: [PATCH v1 0/4] Restore ULPI USB on Tegra20

[Thierry Reding]

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

On Fri, Apr 27, 2018 at 02:58:14AM +0300, Dmitry Osipenko wrote:
> Hello,
> 
> This series of patches fixes ULPI USB on Tegra20. The original problem
> was reported by Marcel Ziswiler, he found that "ulpi-link" clock was
> incorrectly set to CDEV2 instead of PLL_P_OUT4. Marcel made a patch
> that changed the "ulpi-link" clock to PLL_P_OUT4 and that fixed issue
> with the USB for the devices that have CDEV2 being enabled by bootloader.
> The patch got into the kernel and later Marc Dietrich found that USB
> stopped working on the "paz00" Tegra20 board. After a bit of discussion
> was revealed that PLL_P_OUT4 is the parent clock of the CDEV2 and clock
> driver was setting CDEV2's parent incorrectly. The parent clock is actually
> determined by the pinmuxing config of CDEV2 pingroup. This patchset fixes
> the parent of CDEV2 clock by making Tegra's pinctrl driver a clock provider,
> providing CDEV1/2 clock muxes (thanks to Peter De Schrijver for the
> suggestion), and then setting these clock muxes as parents for the CDEV1/2
> clocks. In the end Marcel's CDEV2->PLL_P_OUT4 change is reverted since CDEV2
> (aka MCLK2) is the actual clock source for "ulpi-link".
> 
> Dmitry Osipenko (4):
>   clk: tegra20: Add DEV1/DEV2 OSC dividers
>   pinctrl: tegra20: Provide CDEV1/2 clock muxes
>   clk: tegra20: Set correct parents for CDEV1/2 clocks
>   ARM: dts: tegra20: Revert "Fix ULPI regression on Tegra20"
> 
>  arch/arm/boot/dts/tegra20.dtsi          |  2 +-
>  drivers/clk/tegra/clk-tegra20.c         | 18 +++++++++++----
>  drivers/pinctrl/tegra/pinctrl-tegra.c   | 11 ---------
>  drivers/pinctrl/tegra/pinctrl-tegra.h   | 11 +++++++++
>  drivers/pinctrl/tegra/pinctrl-tegra20.c | 30 ++++++++++++++++++++++++-
>  5 files changed, 55 insertions(+), 17 deletions(-)

Stephen, Michael, Linus,

as far as I can tell there aren't any build dependencies between the
above, so technically these could all be merged through the individual
trees. There's a runtime dependency from patch 2 on patch 1 and from
patch 3 on patch 2, though I don't think they will cause any actual
failures at runtime.

But I can also pick this up into the Tegra tree and send out pull
requests to you for v4.18 (at around v4.17-rc6), if that's what you
prefer.

Thierry

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

Re: [PATCH v1 0/4] Restore ULPI USB on Tegra20

[Thierry Reding]

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

On Mon, Apr 30, 2018 at 11:48:21AM +0200, Thierry Reding wrote:
> On Fri, Apr 27, 2018 at 02:58:14AM +0300, Dmitry Osipenko wrote:
> > Hello,
> > 
> > This series of patches fixes ULPI USB on Tegra20. The original problem
> > was reported by Marcel Ziswiler, he found that "ulpi-link" clock was
> > incorrectly set to CDEV2 instead of PLL_P_OUT4. Marcel made a patch
> > that changed the "ulpi-link" clock to PLL_P_OUT4 and that fixed issue
> > with the USB for the devices that have CDEV2 being enabled by bootloader.
> > The patch got into the kernel and later Marc Dietrich found that USB
> > stopped working on the "paz00" Tegra20 board. After a bit of discussion
> > was revealed that PLL_P_OUT4 is the parent clock of the CDEV2 and clock
> > driver was setting CDEV2's parent incorrectly. The parent clock is actually
> > determined by the pinmuxing config of CDEV2 pingroup. This patchset fixes
> > the parent of CDEV2 clock by making Tegra's pinctrl driver a clock provider,
> > providing CDEV1/2 clock muxes (thanks to Peter De Schrijver for the
> > suggestion), and then setting these clock muxes as parents for the CDEV1/2
> > clocks. In the end Marcel's CDEV2->PLL_P_OUT4 change is reverted since CDEV2
> > (aka MCLK2) is the actual clock source for "ulpi-link".
> > 
> > Dmitry Osipenko (4):
> >   clk: tegra20: Add DEV1/DEV2 OSC dividers
> >   pinctrl: tegra20: Provide CDEV1/2 clock muxes
> >   clk: tegra20: Set correct parents for CDEV1/2 clocks
> >   ARM: dts: tegra20: Revert "Fix ULPI regression on Tegra20"
> > 
> >  arch/arm/boot/dts/tegra20.dtsi          |  2 +-
> >  drivers/clk/tegra/clk-tegra20.c         | 18 +++++++++++----
> >  drivers/pinctrl/tegra/pinctrl-tegra.c   | 11 ---------
> >  drivers/pinctrl/tegra/pinctrl-tegra.h   | 11 +++++++++
> >  drivers/pinctrl/tegra/pinctrl-tegra20.c | 30 ++++++++++++++++++++++++-
> >  5 files changed, 55 insertions(+), 17 deletions(-)
> 
> Stephen, Michael, Linus,
> 
> as far as I can tell there aren't any build dependencies between the
> above, so technically these could all be merged through the individual
> trees. There's a runtime dependency from patch 2 on patch 1 and from
> patch 3 on patch 2, though I don't think they will cause any actual
> failures at runtime.
> 
> But I can also pick this up into the Tegra tree and send out pull
> requests to you for v4.18 (at around v4.17-rc6), if that's what you
> prefer.

Marc just pointed out to me on IRC that this fixes a regression that was
introduced in v4.17-rc1.

Can you guys pick this up into your -fixes branches? Again, I volunteer
to collect these into a separate branch and submit via ARM SoC (the DTS
patch is crucial in enabling the fix) with your Acked-bys if that's the
preferred option.

Thierry

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

Re: [PATCH v1 0/4] Restore ULPI USB on Tegra20

[Stephen Boyd]

Quoting Thierry Reding (2018-04-30 04:28:04)
> On Mon, Apr 30, 2018 at 11:48:21AM +0200, Thierry Reding wrote:
> > 
> > as far as I can tell there aren't any build dependencies between the
> > above, so technically these could all be merged through the individual
> > trees. There's a runtime dependency from patch 2 on patch 1 and from
> > patch 3 on patch 2, though I don't think they will cause any actual
> > failures at runtime.
> > 
> > But I can also pick this up into the Tegra tree and send out pull
> > requests to you for v4.18 (at around v4.17-rc6), if that's what you
> > prefer.
> 
> Marc just pointed out to me on IRC that this fixes a regression that was
> introduced in v4.17-rc1.
> 
> Can you guys pick this up into your -fixes branches? Again, I volunteer
> to collect these into a separate branch and submit via ARM SoC (the DTS
> patch is crucial in enabling the fix) with your Acked-bys if that's the
> preferred option.
> 

Will it go via ARM SoC? I can ack 1 and 3 and it can go that way.

Re: [PATCH v1 0/4] Restore ULPI USB on Tegra20

[Stephen Boyd]

Quoting Thierry Reding (2018-04-30 04:28:04)
> On Mon, Apr 30, 2018 at 11:48:21AM +0200, Thierry Reding wrote:
> > =

> > as far as I can tell there aren't any build dependencies between the
> > above, so technically these could all be merged through the individual
> > trees. There's a runtime dependency from patch 2 on patch 1 and from
> > patch 3 on patch 2, though I don't think they will cause any actual
> > failures at runtime.
> > =

> > But I can also pick this up into the Tegra tree and send out pull
> > requests to you for v4.18 (at around v4.17-rc6), if that's what you
> > prefer.
> =

> Marc just pointed out to me on IRC that this fixes a regression that was
> introduced in v4.17-rc1.
> =

> Can you guys pick this up into your -fixes branches? Again, I volunteer
> to collect these into a separate branch and submit via ARM SoC (the DTS
> patch is crucial in enabling the fix) with your Acked-bys if that's the
> preferred option.
> =


Will it go via ARM SoC? I can ack 1 and 3 and it can go that way.

Re: [PATCH v1 0/4] Restore ULPI USB on Tegra20

[Linus Walleij]

On Mon, Apr 30, 2018 at 1:28 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:

> Marc just pointed out to me on IRC that this fixes a regression that was
> introduced in v4.17-rc1.
>
> Can you guys pick this up into your -fixes branches?

OK I will pick mine independently to my fixes.

> Again, I volunteer
> to collect these into a separate branch and submit via ARM SoC (the DTS
> patch is crucial in enabling the fix) with your Acked-bys if that's the
> preferred option.

No need for me at least.

Yours,
Linus Walleij