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

[Sumit Garg]

On Mon, 11 Mar 2024 at 20:20, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>
>
>
> On 11/03/2024 13:32, Stephan Gerhold wrote:
> > 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 at 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.
>
> fwiw I would also be fine with just treating it like a binary switch and
> only allowing UART1 or UART2 to be selected if that's simpler.

Let me rather pass GCC_BLSP1_UART1_APPS_CLK/GCC_BLSP1_UART2_APPS_CLK
as an argument to the apq8016_clk_init_uart().

-Sumit

> >
> > Thanks,
> > Stephan
> >
> > [1]: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/plat/qti/msm8916/msm8916_setup.c?h=v2.10#n40
>
> --
> // Caleb (they/them)