[PATCH] rockchip: clk: add UART0 clock getter/setter

[Quentin Schulz]

Hi Lukasz,

On 7/31/24 11:43 AM, Lukasz Czechowski wrote:
> Add dedicated getter and setter for SCLK_UART0_PMU.
> This allows the driver to correctly handle UART0 clocks, and thus
> it fixes the issues with UART0 not working in case DEBUG_UART is
> disabled.
> Unlike other Rockchip SoCs, i.e. rk3399, in the PX30 the default
> clock source for UART is GPLL, instead of external oscillator.
> If the DEBUG_UART is enabled, the clock source is changed in
> board_debug_uart_init function to 24Mhz oscillator, which also
> matches the fallback value obtained from DT node.
> In case the DEBUG_UART is disabled, the UART clock source remains
> default, and the DM serial driver wrongly configures the baud rate,
> resulting in broken communication.
> By implementing the UART clock getter/setter, the serial driver
> can probe the actual configuration and corectly configure itself.
> The DEBUG_UART settings now should not affect it.
> 
> The driver supports GPLL and 24M oscillator. NPLL and USBPHY480M
> sources, that are managed by CRU, are not yet handled, as likely
> they won't be used in real scenarios.
> 
> Signed-off-by: Lukasz Czechowski <lukasz.czechowski at thaumatec.com>
> ---
>   arch/arm/include/asm/arch-rockchip/cru_px30.h |   7 ++
>   drivers/clk/rockchip/clk_px30.c               | 107 ++++++++++++++++++
>   2 files changed, 114 insertions(+)
> 
> diff --git a/arch/arm/include/asm/arch-rockchip/cru_px30.h b/arch/arm/include/asm/arch-rockchip/cru_px30.h
> index b66277fc7f3..504459bd93d 100644
> --- a/arch/arm/include/asm/arch-rockchip/cru_px30.h
> +++ b/arch/arm/include/asm/arch-rockchip/cru_px30.h
> @@ -464,5 +464,12 @@ enum {
>   	UART0_CLK_SEL_UART0_FRAC,
>   	UART0_DIVNP5_SHIFT		= 0,
>   	UART0_DIVNP5_MASK		= 0x1f << UART0_DIVNP5_SHIFT,
> +
> +	/* CRU_PMU_CLKSEL5_CON */
> +	CLK_UART_FRAC_NUMERATOR_SHIFT	= 16,
> +	CLK_UART_FRAC_NUMERATOR_MASK	= 0xffff << CLK_UART_FRAC_NUMERATOR_SHIFT,
> +	CLK_UART_FRAC_DENOMINATOR_SHIFT	= 0,
> +	CLK_UART_FRAC_DENOMINATOR_MASK =
> +	    0xffff << CLK_UART_FRAC_DENOMINATOR_SHIFT,
>   };
>   #endif
> diff --git a/drivers/clk/rockchip/clk_px30.c b/drivers/clk/rockchip/clk_px30.c
> index 2875c152b20..64f1a335cb0 100644
> --- a/drivers/clk/rockchip/clk_px30.c
> +++ b/drivers/clk/rockchip/clk_px30.c
> @@ -1589,6 +1589,107 @@ static ulong px30_pmuclk_set_gpll_rate(struct px30_pmuclk_priv *priv, ulong hz)
>   	return priv->gpll_hz;
>   }
>   
> +static ulong px30_pmu_uart0_get_clk(struct px30_pmuclk_priv *priv)
> +{
> +	struct px30_pmucru *pmucru = priv->pmucru;
> +	u32 clk_uart0_div_con;
> +	u32 clk_uart0_pll_sel;
> +	u32 clk_uart0_sel;
> +	u32 con;
> +	ulong pll_rate;
> +	ulong clk_uart0;
> +

You could group all declarations of the same type on the same line and 
also respect reverse xmas tree ordering 
(https://docs.kernel.org/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs) 
(though I don't think we are that strict in U-Boot).

> +	con = readl(&pmucru->pmu_clksel_con[3]);
> +	clk_uart0_div_con = bitfield_extract_by_mask(con, UART0_DIV_CON_MASK);
> +	clk_uart0_pll_sel = bitfield_extract_by_mask(con, UART0_PLL_SEL_MASK);
> +
> +	switch (clk_uart0_pll_sel) {
> +	case UART0_PLL_SEL_GPLL:
> +		pll_rate = px30_pmuclk_get_gpll_rate(priv);
> +		break;
> +	case UART0_PLL_SEL_24M:
> +		pll_rate = OSC_HZ;
> +		break;
> +	case UART0_PLL_SEL_480M:
> +	case UART0_PLL_SEL_NPLL:
> +		/* usbphy480M and NPLL clocks, generated by CRU, are not supported yet */
> +	default:
> +		return -ENOENT;
> +	}
> +
> +	clk_uart0 = DIV_TO_RATE(pll_rate, clk_uart0_div_con);
> +	con = readl(&pmucru->pmu_clksel_con[4]);
> +	clk_uart0_sel = bitfield_extract_by_mask(con, UART0_CLK_SEL_MASK);
> +
> +	switch (clk_uart0_sel) {
> +	case UART0_CLK_SEL_UART0:
> +		return clk_uart0;
> +	case UART0_CLK_SEL_UART0_NP5:{
> +			u32 clk_uart0_divnp5_div_con;
> +
> +			clk_uart0_divnp5_div_con =
> +			    bitfield_extract_by_mask(con, UART0_DIVNP5_MASK);
> +			return DIV_TO_RATE((2 * clk_uart0),
> +					   (2 * clk_uart0_divnp5_div_con + 3));

Are you sure about this DIV_TO_RATE here?

The TRM states:

clk_uart0_divnp5_div_con
clk_uart0_np5=2*clk_uart0/(2*div_con+3)

but this would make it
clk_uart0_np5=2*clk_uart0/((2*div_con+3) + 1)

I believe?

The kernel seems to be handling it like the TRM says (but I wouldn't be 
100% certain here as the clock subsystem and Rockchip drivers have many 
indirections and I may have gotten lost at some point :) ).

I think we're fine also regarding u32 overflows as we would hit this if 
the rates were abore 2.1GHz and I think GPLL and NPLL aren't that fast.

> +		} > +	case UART0_CLK_SEL_UART0_FRAC:{
> +			u32 fracdiv, n, m;
> +
> +			fracdiv = readl(&pmucru->pmu_clksel_con[5]);
> +			n = bitfield_extract_by_mask(fracdiv,
> +						     CLK_UART_FRAC_NUMERATOR_MASK);
> +			m = bitfield_extract_by_mask(fracdiv,
> +						     CLK_UART_FRAC_DENOMINATOR_MASK);
> +			return (u64) clk_uart0 * n / m;
> +		}
> +	default:
> +		return -ENOENT;
> +	}
> +}
> +
> +static ulong px30_pmu_uart0_set_clk(struct px30_pmuclk_priv *priv, ulong rate)
> +{
> +	struct px30_pmucru *pmucru = priv->pmucru;
> +	ulong gpll_rate;
> +	u32 clk_uart0_div_con;
> +	u32 clk_uart0_pll_sel;
> +	u32 clk_uart0_sel;
> +	ulong m = 0, n = 0;
> +
> +	gpll_rate = px30_pmuclk_get_gpll_rate(priv);
> +	if (gpll_rate % rate == 0) {
> +		clk_uart0_pll_sel = UART0_PLL_SEL_GPLL;
> +		clk_uart0_sel = UART0_CLK_SEL_UART0;
> +		clk_uart0_div_con = DIV_ROUND_UP(priv->gpll_hz, rate);
> +	} else if (rate == OSC_HZ) {
> +		clk_uart0_pll_sel = UART0_PLL_SEL_24M;
> +		clk_uart0_sel = UART0_CLK_SEL_UART0;
> +		clk_uart0_div_con = 1;
> +	} else {
> +		clk_uart0_pll_sel = UART0_PLL_SEL_GPLL;
> +		clk_uart0_sel = UART0_CLK_SEL_UART0_FRAC;
> +		clk_uart0_div_con = 1;
> +		rational_best_approximation(rate, priv->gpll_hz,
> +					    GENMASK(16 - 1, 0),
> +					    GENMASK(16 - 1, 0), &m, &n);

I guess we could try to find whether OSC_HZ could produce a closer 
approximation than GPLL but I'm not sure it's worth it right now.

Otherwise looking good,
Cheers,
Quentin