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

[Caleb Connolly]

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.
> 
> 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)