[PATCH] rockchip: clk: add UART0 clock getter/setter
Quentin Schulz
quentin.schulz at cherry.de
Thu Aug 1 12:27:47 CEST 2024
Hi Lukasz,
Maybe also make it explicit in the commit title that this is for px30
only :)
On 8/1/24 12:26 PM, Quentin Schulz wrote:
> 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
More information about the U-Boot
mailing list