Re: [PATCH v2 2/5] apq8016: Add support for UART1 clocks and pinmux

[Stephan Gerhold <stephan@gerhold.net>]

On Mon, Mar 11, 2024 at 12:27:11PM +0000, Caleb Connolly wrote:
> On 11/03/2024 11:10, Sumit Garg wrote:
> > SE HMIBSC board uses UART1 as the main debug console, so add
> > corresponding clocks and pinmux support. Along with that update
> > instructions to enable clocks for debug UART support.
> > 
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >  drivers/clk/qcom/clock-apq8016.c       | 50 +++++++++++++++++++++-----
> >  drivers/pinctrl/qcom/pinctrl-apq8016.c |  1 +
> >  drivers/serial/serial_msm.c            |  6 ++--
> >  3 files changed, 47 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/clk/qcom/clock-apq8016.c b/drivers/clk/qcom/clock-apq8016.c
> > index e6647f7c41d..a620a10a520 100644
> > --- a/drivers/clk/qcom/clock-apq8016.c
> > +++ b/drivers/clk/qcom/clock-apq8016.c
> > @@ -43,6 +43,14 @@
> >  #define BLSP1_UART2_APPS_N		(0x3040)
> >  #define BLSP1_UART2_APPS_D		(0x3044)
> >  
> > +#define BLSP1_UART1_BCR			(0x2038)
> > +#define BLSP1_UART1_APPS_CBCR		(0x203C)
> > +#define BLSP1_UART1_APPS_CMD_RCGR	(0x2044)
> > +#define BLSP1_UART1_APPS_CFG_RCGR	(0x2048)
> > +#define BLSP1_UART1_APPS_M		(0x204C)
> > +#define BLSP1_UART1_APPS_N		(0x2050)
> > +#define BLSP1_UART1_APPS_D		(0x2054)
> > +
> >  /* GPLL0 clock control registers */
> >  #define GPLL0_STATUS_ACTIVE BIT(17)
> >  
> > [...]
> > @@ -94,6 +102,33 @@ static int clk_init_sdc(struct msm_clk_priv *priv, int slot, uint rate)
> >  	return rate;
> >  }
> >  
> > +static const struct bcr_regs uart1_regs = {
> > +	.cfg_rcgr = BLSP1_UART1_APPS_CFG_RCGR,
> > +	.cmd_rcgr = BLSP1_UART1_APPS_CMD_RCGR,
> > +	.M = BLSP1_UART1_APPS_M,
> > +	.N = BLSP1_UART1_APPS_N,
> > +	.D = BLSP1_UART1_APPS_D,
> > +};
> > +
> > +/* UART: 115200 */
> > +static int apq8016_clk_init_uart1(phys_addr_t base)
> 
> I know we're still dealing with some tech debt here, but I'm not a big
> fan of this approach. I notice that the RCG and CBCR registers are all
> offset by exactly 0xff0 between UART1 and UART2, what about adding a
> second "index" parameter to apq8016_clk_init_uart() and then offsetting
> the addresses by (0xff0 * index)?
> 

This would work for MSM8916 where you have just two UARTs, but it might
be misleading since the UART blocks are actually separated in 4 KiB
(0x1000) blocks and not offset by 0xff0. UART2 is a special case that
has different offsets within the 4 KiB block, for some weird reason.

If you want to calculate the register offsets properly you would need
special handling for UART2, e.g. I have the following (admittedly rather
ugly) define in the TF-A port for MSM8916 and similar [1]:

#define GCC_BLSP1_UART_APPS_CBCR(n)	(GCC_BASE + \
	(((n) == 2) ? (0x0302c) : (0x0203c + (((n) - 1) * 0x1000))))

where n is the UART number (here 1 or 2). As a different example,
MDM9607 has 5 UARTs (1 to 5) and there only UART2 is special-cased,
while all others follow the standard offset with 0x1000 offset.

Thanks,
Stephan

[1]: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/plat/qti/msm8916/msm8916_setup.c?h=v2.10#n40