[PATCH v2] serial: ns16550: Enable clocks during probe

[Samuel Holland]

Hi Johan,

On 12/13/22 03:06, Johan Jonker wrote:
> Hi Samuel,
> 
> Could you provide an example and or details for which SoC type this is in mind?

This is needed for Allwinner sunxi SoCs, which come out of the boot ROM
with all clock gates disabled. Currently, the sunxi SPL board code
enables a clock gate in clock_init_uart() based on the legacy console
UART selection (CONFIG_CONS_INDEX). For new SoCs (e.g. D1), I am using
SPL_DM, so now we rely on the DM serial driver to enable the clock gate.

> For current Rockchip we don't need this.
> Everything works out of the box and is not touched in the clock driver.
> The setting is done with serial_rockchip.c
> 
> Your clk_enable() function makes it a requirement for a enable ops function on all SoCs with
> compatible "snps,dw-apb-uart" nodes.

The patch enables clocks on a best-effort basis, and allows the serial
device to continue probing even if an error occurs while getting or
enabling a clock. So this patch may not be necessary on Rockchip
platforms, but it should not hurt anything either.

> The S clock is on position 0 and is set with "clock-frequency" property.
> The P clock is on position 1 and is not touched.
>> +	ret = clk_get_by_index(dev, 0, &clk);
> 
> This tries to enable the S clock instead of the P clock.

With the phycore-rk3288 fix[1] applied, I plan to go back to v1 of the
patch, which tries to enable all clocks. Presumably both clocks need to
be running for the UART to work. So "enabling" them should be
conceptually correct, even if no action is needed from U-Boot to
accomplish that on Rockchip platforms.

Regards,
Samuel

[1]:
https://lore.kernel.org/u-boot/20221213103809.1407187-1-w.egorov@phytec.de/

> So NAK for now.
> 
> Johan
> 
> ===
> 
> From Linux clk-rk3188.c:
> 	GATE(PCLK_UART2, "pclk_uart2", "pclk_peri", 0, RK2928_CLKGATE_CON(8), 2, GFLAGS),
> 
> static struct rockchip_clk_branch common_uart2_fracmux __initdata =
> 	MUX(SCLK_UART2, "sclk_uart2", mux_sclk_uart2_p, CLK_SET_RATE_PARENT,
> 			RK2928_CLKSEL_CON(15), 8, 2, MFLAGS);
> ===
> 
> From serial_rockchip.c:
> 
> static int rockchip_serial_probe(struct udevice *dev)
> {
> #if CONFIG_IS_ENABLED(OF_PLATDATA)
> 	struct rockchip_uart_plat *plat = dev_get_plat(dev);
> 
> 	/* Create some new platform data for the standard driver */
> 	plat->plat.base = plat->dtplat.reg[0];
> 	plat->plat.reg_shift = plat->dtplat.reg_shift;
> 
> ////
> 	plat->plat.clock = plat->dtplat.clock_frequency;
> ////
> 
> 	plat->plat.fcr = UART_FCR_DEFVAL;
> 	dev_set_plat(dev, &plat->plat);
> 
> 	return ns16550_serial_probe(dev);
> #else
> 	return -ENODEV;
> #endif
> }
> 
> ===
> 
> From clk_rk3066.c:
> 
> static int rk3066_clk_enable(struct clk *clk)
> {
> 	struct rk3066_clk_priv *priv = dev_get_priv(clk->dev);
> 
> 	switch (clk->id) {
> 	case HCLK_NANDC0:
> 		rk_clrreg(&priv->cru->cru_clkgate_con[5], BIT(9));
> 		break;
> 	case HCLK_SDMMC:
> 		rk_clrreg(&priv->cru->cru_clkgate_con[5], BIT(10));
> 		break;
> 	case HCLK_SDIO:
> 		rk_clrreg(&priv->cru->cru_clkgate_con[5], BIT(11));
> 		break;
> 	}
> 
> 	return 0;
> }
> static struct clk_ops rk3066_clk_ops = {
> 	.disable	= rk3066_clk_disable,
> 	.enable	= rk3066_clk_enable,
> 	.get_rate	= rk3066_clk_get_rate,
> 	.set_rate	= rk3066_clk_set_rate,
> };
> 
> ===
> 
> For rk3xxx.dtsi:
> 
> 	uart2: serial at 20064000 {
> 		compatible = "snps,dw-apb-uart";
> 		reg = <0x20064000 0x400>;
> 		interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> 		reg-shift = <2>;
> 		reg-io-width = <1>;
> 		clock-names = "baudclk", "apb_pclk";
> 		clocks = <&cru SCLK_UART2>, <&cru PCLK_UART2>;
> 		status = "disabled";
> 	};
> 
> On 12/13/22 02:50, Samuel Holland wrote:
>> If the UART bus clock has a gate, it must be enabled before the UART
>> can be used.
>>
>> Signed-off-by: Samuel Holland <samuel at sholland.org>
>> ---
>>
>> Changes in v2:
>>  - Only enable the first clock, as using the clk_get_bulk() API pushes
>>    a board (phycore-rk3288) over its SPL size limit.
>>
>>  drivers/serial/ns16550.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> index 7592979cab5..072419343a3 100644
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -507,6 +507,7 @@ int ns16550_serial_probe(struct udevice *dev)
>>  	struct ns16550 *const com_port = dev_get_priv(dev);
>>  	struct reset_ctl_bulk reset_bulk;
>>  	fdt_addr_t addr;
>> +	struct clk clk;
>>  	int ret;
>>  
>>  	/*
>> @@ -524,6 +525,10 @@ int ns16550_serial_probe(struct udevice *dev)
>>  	if (!ret)
>>  		reset_deassert_bulk(&reset_bulk);
>>  
>> +	ret = clk_get_by_index(dev, 0, &clk);
>> +	if (!ret)
>> +		clk_enable(&clk);
>> +
>>  	com_port->plat = dev_get_plat(dev);
>>  	ns16550_init(com_port, -1);
>>