[PATCH v2 0/2] rockchip: fix serial output on rk3036

[PATCH v2 0/2] rockchip: fix serial output on rk3036

[Heiko Stuebner]

Recent changes to the 8250-dw variant revealed issues concerning
how the clock rates are handled on the rk3036 uart.

For one, there was an error in the clock declaration, but also the
shared uart-pll-select-mux also as default got supplied from the apll
that also supplies the cpu and thus gets frequency scaled.

The patches in this series remedy this and make the debug uart
function again on 4.10 + current merge window.


As for the merge-path, I've now tested all Rockchip socs I have access
to (3036, 3288, 3368, 3399) and didn't find any more clock-related issues
with the merge-window as of today. So if no other subtle issue turns up
this week, these should be all fixes for the 4.11 cycle.
So these 2 patches could be picked up by clock-maintainers directly if
so desired, or I can send a pull request after the merge-window closes
and we're save to say that nothing else broke.


changes in v2:
- Fixes tag for the missing ","
- do reparenting in the driver itself
- drop clock-id addition, as they're not needed for the fix anymore

Heiko Stuebner (2):
  clk: rockchip: add "," to mux_pll_src_apll_dpll_gpll_usb480m_p on
    rk3036
  clk: rockchip: Make uartpll a child of the gpll on rk3036

 drivers/clk/rockchip/clk-rk3036.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

-- 
2.11.0

[PATCH v2 1/2] clk: rockchip: add "," to mux_pll_src_apll_dpll_gpll_usb480m_p on rk3036

[Heiko Stuebner]

The mux_pll_src_apll_dpll_gpll_usb480m_p parent list was missing a ","
between the 3rd and 4th parent names, making them fall together and thus
lookups fail. Fix that.

Fixes: 5190c08b2989 ("clk: rockchip: add clock controller for rk3036")
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/clk-rk3036.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-rk3036.c b/drivers/clk/rockchip/clk-rk3036.c
index 924f560dcf80..dcde70f4c105 100644
--- a/drivers/clk/rockchip/clk-rk3036.c
+++ b/drivers/clk/rockchip/clk-rk3036.c
@@ -127,7 +127,7 @@ PNAME(mux_ddrphy_p)		= { "dpll_ddr", "gpll_ddr" };
 PNAME(mux_pll_src_3plls_p)	= { "apll", "dpll", "gpll" };
 PNAME(mux_timer_p)		= { "xin24m", "pclk_peri_src" };
 
-PNAME(mux_pll_src_apll_dpll_gpll_usb480m_p)	= { "apll", "dpll", "gpll" "usb480m" };
+PNAME(mux_pll_src_apll_dpll_gpll_usb480m_p)	= { "apll", "dpll", "gpll", "usb480m" };
 
 PNAME(mux_mmc_src_p)	= { "apll", "dpll", "gpll", "xin24m" };
 PNAME(mux_i2s_pre_p)	= { "i2s_src", "i2s_frac", "ext_i2s", "xin12m" };
-- 
2.11.0

[PATCH v2 2/2] clk: rockchip: Make uartpll a child of the gpll on rk3036

[Heiko Stuebner]

The shared uart-pll is on boot a child of the apll that can get changed
by cpu frequency scaling. So move it away to the more stable gpll to
make sure the uart doesn't break on cpu frequency changes.

This turned up during the 4.11 merge-window when commit
6a171b299379 ("serial: 8250_dw: Allow hardware flow control to be used")
added general termios enablement making the uart on rk3036 change
frequency and thus making it susceptible for the frequency scaling issue.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/clk-rk3036.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-rk3036.c b/drivers/clk/rockchip/clk-rk3036.c
index dcde70f4c105..8b8d2c90c884 100644
--- a/drivers/clk/rockchip/clk-rk3036.c
+++ b/drivers/clk/rockchip/clk-rk3036.c
@@ -16,6 +16,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
@@ -442,7 +443,7 @@ static void __init rk3036_clk_init(struct device_node *np)
 {
 	struct rockchip_clk_provider *ctx;
 	void __iomem *reg_base;
-	struct clk *clk;
+	struct clk *clk, *clk2;
 
 	reg_base = of_iomap(np, 0);
 	if (!reg_base) {
@@ -480,6 +481,19 @@ static void __init rk3036_clk_init(struct device_node *np)
 
 	rockchip_register_restart_notifier(ctx, RK2928_GLB_SRST_FST, NULL);
 
+	clk = __clk_lookup("uart_pll_clk");
+	clk2 = __clk_lookup("gpll");
+	if (clk && clk2) {
+		int ret = clk_set_parent(clk, clk2);
+
+		if (ret < 0)
+			pr_warn("%s: could not reparent uart_pll_clk to gpll\n",
+				__func__);
+	} else {
+		pr_warn("%s: missing clocks to reparent uart_pll_clk to gpll\n",
+			__func__);
+	}
+
 	rockchip_clk_of_add_provider(np, ctx);
 }
 CLK_OF_DECLARE(rk3036_cru, "rockchip,rk3036-cru", rk3036_clk_init);
-- 
2.11.0

Re: [PATCH v2 2/2] clk: rockchip: Make uartpll a child of the gpll on rk3036

[Stephen Boyd]

On 02/28, Heiko Stuebner wrote:
> @@ -480,6 +481,19 @@ static void __init rk3036_clk_init(struct device_node *np)
>  
>  	rockchip_register_restart_notifier(ctx, RK2928_GLB_SRST_FST, NULL);
>  
> +	clk = __clk_lookup("uart_pll_clk");
> +	clk2 = __clk_lookup("gpll");

Can we do the register writes directly? Or call the appropriate
function to twiddle some bits to change the parent before
registering the clks? It's nice to avoid using clk API in
provider drivers when we're getting clks from the provider
itself, plus I'd like to get rid of __clk_lookup() one day.

> +	if (clk && clk2) {
> +		int ret = clk_set_parent(clk, clk2);
> +
> +		if (ret < 0)
> +			pr_warn("%s: could not reparent uart_pll_clk to gpll\n",
> +				__func__);
> +	} else {
> +		pr_warn("%s: missing clocks to reparent uart_pll_clk to gpll\n",
> +			__func__);
> +	}
> +
>  	rockchip_clk_of_add_provider(np, ctx);
>  }

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project