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

[PATCH v2 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".

Changelog:

v2:
	- Added new patch "Add quirk for getting CDEV1/2 clocks", assuring
	  that clk user won't get CDEV1/2 clocks until parent clk muxes
	  are available, i.e. resolves potential issue with CDEV-user driver
	  vs pinctrl driver probe order.

	- Factored out "pinctrl" patch from the patchset as was requested by
	  Linus Walleij.

	- Addressed v1 review comments: fixed swapped DEV1/2 clk div bits,
	  made DEV1/2 divs read-only, etc minor changes.

Dmitry Osipenko (4):
  clk: tegra20: Add DEV1/DEV2 OSC dividers
  clk: tegra20: Correct parents of CDEV1/2 clocks
  clk: tegra: Add quirk for getting 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 | 20 +++++++++++++++----
 drivers/clk/tegra/clk.c         | 34 ++++++++++++++++++++++++++++++++-
 3 files changed, 50 insertions(+), 6 deletions(-)

-- 
2.17.0

[PATCH v2 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>
Reviewed-by: Marcel Ziswiler <marcel@ziswiler.com>
Tested-by: Marcel Ziswiler <marcel@ziswiler.com>
Tested-by: Marc Dietrich <marvin24@gmx.de>
Acked-by: Peter De Schrijver <pdeschrijver@nvidia.com>
Acked-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/tegra/clk-tegra20.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index 0ee56dd04cec..ad5a7b5e3a39 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,18 @@ static void __init tegra20_periph_clk_init(void)
 				    periph_clk_enb_refcnt);
 	clks[TEGRA20_CLK_PEX] = clk;
 
+	/* dev1 OSC divider */
+	clk_register_divider(NULL, "dev1_osc_div", "clk_m",
+			     0, clk_base + MISC_CLK_ENB, 22, 2,
+			     CLK_DIVIDER_POWER_OF_TWO | CLK_DIVIDER_READ_ONLY,
+			     NULL);
+
+	/* dev2 OSC divider */
+	clk_register_divider(NULL, "dev2_osc_div", "clk_m",
+			     0, clk_base + MISC_CLK_ENB, 20, 2,
+			     CLK_DIVIDER_POWER_OF_TWO | CLK_DIVIDER_READ_ONLY,
+			     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

[PATCH v2] 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>
Reviewed-by: Marcel Ziswiler <marcel@ziswiler.com>
Tested-by: Marcel Ziswiler <marcel@ziswiler.com>
Tested-by: Marc Dietrich <marvin24@gmx.de>
Acked-by: Peter De Schrijver <pdeschrijver@nvidia.com>
---

v2: This patch is factored out from the v1 clk/DT series so that it could be
    applied separately.

 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..b6dd939d32cc 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[] = {
+	"dev1_osc_div", "pll_a_out0", "pll_m_out1", "audio",
+};
+
+static const char *cdev2_parents[] = {
+	"dev2_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

[PATCH v2 2/4] clk: tegra20: Correct parents of 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>
Reviewed-by: Marcel Ziswiler <marcel@ziswiler.com>
Tested-by: Marcel Ziswiler <marcel@ziswiler.com>
Tested-by: Marc Dietrich <marvin24@gmx.de>
Acked-by: Peter De Schrijver <pdeschrijver@nvidia.com>
Acked-by: Stephen Boyd <sboyd@kernel.org>
---
 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 ad5a7b5e3a39..636500a98561 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -846,14 +846,12 @@ static void __init tegra20_periph_clk_init(void)
 			     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

[PATCH v2 3/4] clk: tegra: Add quirk for getting CDEV1/2 clocks

[Dmitry Osipenko]

CDEV1 and CDEV2 clocks are a bit special case, their parent clock is
created by the pinctrl driver. It should be possible for clk user to
request these clocks before pinctrl driver got probed and hence user will
get an orphaned clock. That might be undesirable because user expects
parent clock to be enabled by the child, so let's return EPROBE_DEFER
until parent clock appear.

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

diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
index ba923f0d5953..04cbe7e9eff3 100644
--- a/drivers/clk/tegra/clk.c
+++ b/drivers/clk/tegra/clk.c
@@ -298,6 +298,38 @@ static struct reset_controller_dev rst_ctlr = {
 	.of_reset_n_cells = 1,
 };
 
+static struct clk *tegra_of_clk_src_onecell_get(struct of_phandle_args *clkspec,
+						void *data)
+{
+	struct clk_hw *parent_hw;
+	struct clk_hw *hw;
+	struct clk *clk;
+	const char *name;
+
+	clk = of_clk_src_onecell_get(clkspec, data);
+	if (IS_ERR(clk))
+		return clk;
+
+	name = __clk_get_name(clk);
+
+	/*
+	 * Tegra20 CDEV1 and CDEV2 clocks are a bit special case, their parent
+	 * clock is created by the pinctrl driver. It is possible for clk user
+	 * to request these clocks before pinctrl driver got probed and hence
+	 * user will get an orphaned clock. That might be undesirable because
+	 * user may expect parent clock to be enabled by the child.
+	 */
+	if (!strcmp(name, "cdev1") || !strcmp(name, "cdev2")) {
+		hw = __clk_get_hw(clk);
+
+		parent_hw = clk_hw_get_parent(hw);
+		if (!parent_hw)
+			return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	return clk;
+}
+
 void __init tegra_add_of_provider(struct device_node *np)
 {
 	int i;
@@ -314,7 +346,7 @@ void __init tegra_add_of_provider(struct device_node *np)
 
 	clk_data.clks = clks;
 	clk_data.clk_num = clk_num;
-	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
+	of_clk_add_provider(np, tegra_of_clk_src_onecell_get, &clk_data);
 
 	rst_ctlr.of_node = np;
 	rst_ctlr.nr_resets = periph_banks * 32 + num_special_reset;
-- 
2.17.0

[PATCH v2 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>
Reviewed-by: Marcel Ziswiler <marcel@ziswiler.com>
Tested-by: Marcel Ziswiler <marcel@ziswiler.com>
Tested-by: Marc Dietrich <marvin24@gmx.de>
---
 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 v2 4/4] ARM: dts: tegra20: Revert "Fix ULPI regression on Tegra20"

[Thierry Reding]

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

On Fri, May 04, 2018 at 01:55:37AM +0300, Dmitry Osipenko wrote:
> 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>
> Reviewed-by: Marcel Ziswiler <marcel@ziswiler.com>
> Tested-by: Marcel Ziswiler <marcel@ziswiler.com>
> Tested-by: Marc Dietrich <marvin24@gmx.de>
> ---
>  arch/arm/boot/dts/tegra20.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Is it still true that this patch alone fixes the known regression? I'm
just asking because the remainder of the series, even though it sounds
to be the right thing to do, is fairly big for a fix against v4.17.

So if this alone fixes the regression I think it'd be best to queue it
up for v4.17 and get the rest of the patches into v4.18.

Thierry

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

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

[Dmitry Osipenko]

On 04.05.2018 13:40, Thierry Reding wrote:
> On Fri, May 04, 2018 at 01:55:37AM +0300, Dmitry Osipenko wrote:
>> 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>
>> Reviewed-by: Marcel Ziswiler <marcel@ziswiler.com>
>> Tested-by: Marcel Ziswiler <marcel@ziswiler.com>
>> Tested-by: Marc Dietrich <marvin24@gmx.de>
>> ---
>>  arch/arm/boot/dts/tegra20.dtsi | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Is it still true that this patch alone fixes the known regression? I'm
> just asking because the remainder of the series, even though it sounds
> to be the right thing to do, is fairly big for a fix against v4.17.

Yes, Marcel says that reverting just this patch works for him now.

> So if this alone fixes the regression I think it'd be best to queue it
> up for v4.17 and get the rest of the patches into v4.18.

Sounds good.

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

[Marcel Ziswiler]

Hi Dmitry

On Fri, 2018-05-04 at 01:55 +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".
> 
> Changelog:
> 
> v2:
> 	- Added new patch "Add quirk for getting CDEV1/2 clocks",
> assuring
> 	  that clk user won't get CDEV1/2 clocks until parent clk muxes
> 	  are available, i.e. resolves potential issue with CDEV-user
> driver
> 	  vs pinctrl driver probe order.
> 
> 	- Factored out "pinctrl" patch from the patchset as was
> requested by
> 	  Linus Walleij.
> 
> 	- Addressed v1 review comments: fixed swapped DEV1/2 clk div
> bits,
> 	  made DEV1/2 divs read-only, etc minor changes.
> 
> Dmitry Osipenko (4):
>   clk: tegra20: Add DEV1/DEV2 OSC dividers
>   clk: tegra20: Correct parents of CDEV1/2 clocks
>   clk: tegra: Add quirk for getting 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 | 20 +++++++++++++++----
>  drivers/clk/tegra/clk.c         | 34
> ++++++++++++++++++++++++++++++++-
>  3 files changed, 50 insertions(+), 6 deletions(-)

Beautiful (;-p).

root@colibri-t20:~# cat /sys/kernel/debug/clk/clk_summary | grep -E
'pll_p_out4|cdev2'
          pll_p_out4_div              1        1        0    24000000  
        0 0  
             pll_p_out4               2        2        0    24000000  
        0 0  
                cdev2_mux             1        1        0    24000000  
        0 0  
                   cdev2              1        1        0    24000000  
        0 0  

You may add my reviewed and tested bys to the remaining patches as
well.

Thanks!

Cheers

Marcel

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

[Dmitry Osipenko]

On 04.05.2018 16:33, Marcel Ziswiler wrote:
> Hi Dmitry
> 
> On Fri, 2018-05-04 at 01:55 +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".
>>
>> Changelog:
>>
>> v2:
>> 	- Added new patch "Add quirk for getting CDEV1/2 clocks",
>> assuring
>> 	  that clk user won't get CDEV1/2 clocks until parent clk muxes
>> 	  are available, i.e. resolves potential issue with CDEV-user
>> driver
>> 	  vs pinctrl driver probe order.
>>
>> 	- Factored out "pinctrl" patch from the patchset as was
>> requested by
>> 	  Linus Walleij.
>>
>> 	- Addressed v1 review comments: fixed swapped DEV1/2 clk div
>> bits,
>> 	  made DEV1/2 divs read-only, etc minor changes.
>>
>> Dmitry Osipenko (4):
>>   clk: tegra20: Add DEV1/DEV2 OSC dividers
>>   clk: tegra20: Correct parents of CDEV1/2 clocks
>>   clk: tegra: Add quirk for getting 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 | 20 +++++++++++++++----
>>  drivers/clk/tegra/clk.c         | 34
>> ++++++++++++++++++++++++++++++++-
>>  3 files changed, 50 insertions(+), 6 deletions(-)
> 
> Beautiful (;-p).
> 
> root@colibri-t20:~# cat /sys/kernel/debug/clk/clk_summary | grep -E
> 'pll_p_out4|cdev2'
>           pll_p_out4_div              1        1        0    24000000  
>         0 0  
>              pll_p_out4               2        2        0    24000000  
>         0 0  
>                 cdev2_mux             1        1        0    24000000  
>         0 0  
>                    cdev2              1        1        0    24000000  
>         0 0  
> 
> You may add my reviewed and tested bys to the remaining patches as
> well.
> 
> Thanks!
Awesome, thank you!

Re: [PATCH v2 3/4] clk: tegra: Add quirk for getting CDEV1/2 clocks

[Peter De Schrijver]

On Fri, May 04, 2018 at 01:55:36AM +0300, Dmitry Osipenko wrote:
> CDEV1 and CDEV2 clocks are a bit special case, their parent clock is
> created by the pinctrl driver. It should be possible for clk user to
> request these clocks before pinctrl driver got probed and hence user will
> get an orphaned clock. That might be undesirable because user expects
> parent clock to be enabled by the child, so let's return EPROBE_DEFER
> until parent clock appear.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/clk/tegra/clk.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
> index ba923f0d5953..04cbe7e9eff3 100644
> --- a/drivers/clk/tegra/clk.c
> +++ b/drivers/clk/tegra/clk.c
> @@ -298,6 +298,38 @@ static struct reset_controller_dev rst_ctlr = {
>  	.of_reset_n_cells = 1,
>  };
>  
> +static struct clk *tegra_of_clk_src_onecell_get(struct of_phandle_args *clkspec,
> +						void *data)
> +{
> +	struct clk_hw *parent_hw;
> +	struct clk_hw *hw;
> +	struct clk *clk;
> +	const char *name;
> +
> +	clk = of_clk_src_onecell_get(clkspec, data);
> +	if (IS_ERR(clk))
> +		return clk;
> +
> +	name = __clk_get_name(clk);
> +
> +	/*
> +	 * Tegra20 CDEV1 and CDEV2 clocks are a bit special case, their parent
> +	 * clock is created by the pinctrl driver. It is possible for clk user
> +	 * to request these clocks before pinctrl driver got probed and hence
> +	 * user will get an orphaned clock. That might be undesirable because
> +	 * user may expect parent clock to be enabled by the child.
> +	 */
> +	if (!strcmp(name, "cdev1") || !strcmp(name, "cdev2")) {
> +		hw = __clk_get_hw(clk);
> +
> +		parent_hw = clk_hw_get_parent(hw);
> +		if (!parent_hw)
> +			return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	return clk;
> +}
> +

Rather than retrieving the struct clk * and comparing the names, you can use
the DT ID in  clkspec->args[0] and compare against TEGRA20_CLK_CDEV1 and
TEGRA20_CLK_CDEV2.

Peter.

>  void __init tegra_add_of_provider(struct device_node *np)
>  {
>  	int i;
> @@ -314,7 +346,7 @@ void __init tegra_add_of_provider(struct device_node *np)
>  
>  	clk_data.clks = clks;
>  	clk_data.clk_num = clk_num;
> -	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> +	of_clk_add_provider(np, tegra_of_clk_src_onecell_get, &clk_data);
>  
>  	rst_ctlr.of_node = np;
>  	rst_ctlr.nr_resets = periph_banks * 32 + num_special_reset;
> -- 
> 2.17.0
> 

Re: [PATCH v2 3/4] clk: tegra: Add quirk for getting CDEV1/2 clocks

[Peter De Schrijver]

On Fri, May 04, 2018 at 01:55:36AM +0300, Dmitry Osipenko wrote:
> CDEV1 and CDEV2 clocks are a bit special case, their parent clock is
> created by the pinctrl driver. It should be possible for clk user to
> request these clocks before pinctrl driver got probed and hence user will
> get an orphaned clock. That might be undesirable because user expects
> parent clock to be enabled by the child, so let's return EPROBE_DEFER
> until parent clock appear.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/clk/tegra/clk.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
> index ba923f0d5953..04cbe7e9eff3 100644
> --- a/drivers/clk/tegra/clk.c
> +++ b/drivers/clk/tegra/clk.c
> @@ -298,6 +298,38 @@ static struct reset_controller_dev rst_ctlr = {
>  	.of_reset_n_cells = 1,
>  };
>  
> +static struct clk *tegra_of_clk_src_onecell_get(struct of_phandle_args *clkspec,
> +						void *data)
> +{
> +	struct clk_hw *parent_hw;
> +	struct clk_hw *hw;
> +	struct clk *clk;
> +	const char *name;
> +
> +	clk = of_clk_src_onecell_get(clkspec, data);
> +	if (IS_ERR(clk))
> +		return clk;
> +
> +	name = __clk_get_name(clk);
> +
> +	/*
> +	 * Tegra20 CDEV1 and CDEV2 clocks are a bit special case, their parent
> +	 * clock is created by the pinctrl driver. It is possible for clk user
> +	 * to request these clocks before pinctrl driver got probed and hence
> +	 * user will get an orphaned clock. That might be undesirable because
> +	 * user may expect parent clock to be enabled by the child.
> +	 */
> +	if (!strcmp(name, "cdev1") || !strcmp(name, "cdev2")) {
> +		hw = __clk_get_hw(clk);
> +
> +		parent_hw = clk_hw_get_parent(hw);
> +		if (!parent_hw)
> +			return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	return clk;
> +}
> +

Rather than retrieving the struct clk * and comparing the names, you can use
the DT ID in  clkspec->args[0] and compare against TEGRA20_CLK_CDEV1 and
TEGRA20_CLK_CDEV2.

Peter.

>  void __init tegra_add_of_provider(struct device_node *np)
>  {
>  	int i;
> @@ -314,7 +346,7 @@ void __init tegra_add_of_provider(struct device_node *np)
>  
>  	clk_data.clks = clks;
>  	clk_data.clk_num = clk_num;
> -	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> +	of_clk_add_provider(np, tegra_of_clk_src_onecell_get, &clk_data);
>  
>  	rst_ctlr.of_node = np;
>  	rst_ctlr.nr_resets = periph_banks * 32 + num_special_reset;
> -- 
> 2.17.0
> 

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

[Linus Walleij]

On Fri, May 4, 2018 at 12:55 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>
> Reviewed-by: Marcel Ziswiler <marcel@ziswiler.com>
> Tested-by: Marcel Ziswiler <marcel@ziswiler.com>
> Tested-by: Marc Dietrich <marvin24@gmx.de>
> Acked-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> ---
>
> v2: This patch is factored out from the v1 clk/DT series so that it could be
>     applied separately.

Patch applied unless Stephen W protests.
Please include swarren on future patches to this driver.

Yours,
Linus Walleij

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

[Dmitry Osipenko]

On 16.05.2018 15:23, Linus Walleij wrote:
> On Fri, May 4, 2018 at 12:55 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>
>> Reviewed-by: Marcel Ziswiler <marcel@ziswiler.com>
>> Tested-by: Marcel Ziswiler <marcel@ziswiler.com>
>> Tested-by: Marc Dietrich <marvin24@gmx.de>
>> Acked-by: Peter De Schrijver <pdeschrijver@nvidia.com>
>> ---
>>
>> v2: This patch is factored out from the v1 clk/DT series so that it could be
>>     applied separately.
> 
> Patch applied unless Stephen W protests.
> Please include swarren on future patches to this driver.

I think previously get_maintainer script suggested driver authors, probably it
changed sometime ago. I'm pretty sure that Stephen follows the linux-tegra ML
(Stephen, are you?) and would have jumped into the thread if there was anything
wrong in the patch, though indeed won't hurt to ping him for this patch explicitly.

Stephen, please let me know if you disagree with anything in this patch.