[v4,07/39] serial: msm: add debug UART

Introduce support for early debugging. This relies on the previous stage
bootloader to initialise the UART clocks, when running with U-Boot as
the primary bootloader this feature doesn't work. It will require a way
to configure the clocks before the driver model is available.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/serial/Kconfig      |  8 ++++++++
 drivers/serial/serial_msm.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

On 15/02/2024 21:52, Caleb Connolly wrote:
> Introduce support for early debugging. This relies on the previous stage
> bootloader to initialise the UART clocks, when running with U-Boot as
> the primary bootloader this feature doesn't work. It will require a way
> to configure the clocks before the driver model is available.
> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>   drivers/serial/Kconfig      |  8 ++++++++
>   drivers/serial/serial_msm.c | 37 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index 26460c4e0cab..fbd351a47859 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -319,6 +319,14 @@ config DEBUG_UART_S5P
>   	  will need to provide parameters to make this work. The driver will
>   	  be available until the real driver-model serial is running.
>   
> +config DEBUG_UART_MSM
> +	bool "Qualcomm QUP UART debug"
> +	depends on ARCH_SNAPDRAGON
> +	help
> +	  Select this to enable a debug UART using the serial_msm driver. You
> +	  will need to provide parameters to make this work. The driver will
> +	  be available until the real driver-model serial is running.
> +
>   config DEBUG_UART_MSM_GENI
>   	bool "Qualcomm snapdragon"
>   	depends on ARCH_SNAPDRAGON
> diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
> index f4d96313b931..44b93bd7ff21 100644
> --- a/drivers/serial/serial_msm.c
> +++ b/drivers/serial/serial_msm.c
> @@ -252,3 +252,40 @@ U_BOOT_DRIVER(serial_msm) = {
>   	.probe = msm_serial_probe,
>   	.ops	= &msm_serial_ops,
>   };
> +
> +#ifdef CONFIG_DEBUG_UART_MSM
> +
> +static struct msm_serial_data init_serial_data = {
> +	.base = CONFIG_VAL(DEBUG_UART_BASE),
> +	.clk_rate = 7372800,
> +};
> +
> +#include <debug_uart.h>
> +
> +/* Uncomment to turn on UART clocks when debugging U-Boot as aboot on MSM8916 */
> +//int apq8016_clk_init_uart(phys_addr_t gcc_base);
> +
> +static inline void _debug_uart_init(void)
> +{
> +	/* Uncomment to turn on UART clocks when debugging U-Boot as aboot on MSM8916 */
> +	//apq8016_clk_init_uart(0x1800000);
> +	uart_dm_init(&init_serial_data);
> +}
> +
> +static inline void _debug_uart_putc(int ch)
> +{
> +	struct msm_serial_data *priv = &init_serial_data;
> +
> +	while (!(readl(priv->base + UARTDM_SR) & UARTDM_SR_TX_EMPTY) &&
> +	       !(readl(priv->base + UARTDM_ISR) & UARTDM_ISR_TX_READY))
> +		;
> +
> +	writel(UARTDM_CR_CMD_RESET_TX_READY, priv->base + UARTDM_CR);
> +
> +	writel(1, priv->base + UARTDM_NCF_TX);
> +	writel(ch, priv->base + UARTDM_TF);
> +}
> +
> +DEBUG_UART_FUNCS
> +
> +#endif
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

On Fri, 16 Feb 2024 at 02:22, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>
> Introduce support for early debugging. This relies on the previous stage
> bootloader to initialise the UART clocks, when running with U-Boot as
> the primary bootloader this feature doesn't work. It will require a way
> to configure the clocks before the driver model is available.
>
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  drivers/serial/Kconfig      |  8 ++++++++
>  drivers/serial/serial_msm.c | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
>
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index 26460c4e0cab..fbd351a47859 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -319,6 +319,14 @@ config DEBUG_UART_S5P
>           will need to provide parameters to make this work. The driver will
>           be available until the real driver-model serial is running.
>
> +config DEBUG_UART_MSM
> +       bool "Qualcomm QUP UART debug"
> +       depends on ARCH_SNAPDRAGON

Since this debug UART only works for chainloaded configuration, can we
somehow add explicit dependency here? Something like !REMAKE_ELF?

-Sumit

> +       help
> +         Select this to enable a debug UART using the serial_msm driver. You
> +         will need to provide parameters to make this work. The driver will
> +         be available until the real driver-model serial is running.
> +
>  config DEBUG_UART_MSM_GENI
>         bool "Qualcomm snapdragon"
>         depends on ARCH_SNAPDRAGON
> diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
> index f4d96313b931..44b93bd7ff21 100644
> --- a/drivers/serial/serial_msm.c
> +++ b/drivers/serial/serial_msm.c
> @@ -252,3 +252,40 @@ U_BOOT_DRIVER(serial_msm) = {
>         .probe = msm_serial_probe,
>         .ops    = &msm_serial_ops,
>  };
> +
> +#ifdef CONFIG_DEBUG_UART_MSM
> +
> +static struct msm_serial_data init_serial_data = {
> +       .base = CONFIG_VAL(DEBUG_UART_BASE),
> +       .clk_rate = 7372800,
> +};
> +
> +#include <debug_uart.h>
> +
> +/* Uncomment to turn on UART clocks when debugging U-Boot as aboot on MSM8916 */
> +//int apq8016_clk_init_uart(phys_addr_t gcc_base);
> +
> +static inline void _debug_uart_init(void)
> +{
> +       /* Uncomment to turn on UART clocks when debugging U-Boot as aboot on MSM8916 */
> +       //apq8016_clk_init_uart(0x1800000);
> +       uart_dm_init(&init_serial_data);
> +}
> +
> +static inline void _debug_uart_putc(int ch)
> +{
> +       struct msm_serial_data *priv = &init_serial_data;
> +
> +       while (!(readl(priv->base + UARTDM_SR) & UARTDM_SR_TX_EMPTY) &&
> +              !(readl(priv->base + UARTDM_ISR) & UARTDM_ISR_TX_READY))
> +               ;
> +
> +       writel(UARTDM_CR_CMD_RESET_TX_READY, priv->base + UARTDM_CR);
> +
> +       writel(1, priv->base + UARTDM_NCF_TX);
> +       writel(ch, priv->base + UARTDM_TF);
> +}
> +
> +DEBUG_UART_FUNCS
> +
> +#endif
>
> --
> 2.43.1
>

On 20/02/2024 06:08, Sumit Garg wrote:
> On Fri, 16 Feb 2024 at 02:22, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>>
>> Introduce support for early debugging. This relies on the previous stage
>> bootloader to initialise the UART clocks, when running with U-Boot as
>> the primary bootloader this feature doesn't work. It will require a way
>> to configure the clocks before the driver model is available.
>>
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> ---
>>  drivers/serial/Kconfig      |  8 ++++++++
>>  drivers/serial/serial_msm.c | 37 +++++++++++++++++++++++++++++++++++++
>>  2 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
>> index 26460c4e0cab..fbd351a47859 100644
>> --- a/drivers/serial/Kconfig
>> +++ b/drivers/serial/Kconfig
>> @@ -319,6 +319,14 @@ config DEBUG_UART_S5P
>>           will need to provide parameters to make this work. The driver will
>>           be available until the real driver-model serial is running.
>>
>> +config DEBUG_UART_MSM
>> +       bool "Qualcomm QUP UART debug"
>> +       depends on ARCH_SNAPDRAGON
> 
> Since this debug UART only works for chainloaded configuration, can we
> somehow add explicit dependency here? Something like !REMAKE_ELF?

With a small patch (which didn't make it into v4 apparently) the
apq8016_clk_init_uart() function from clock-apq8016 can be adjusted to
just take a base address rather than "struct msm_clk_priv". It can then
be called from debug_uart_init() and allows for debug UART to be used
when U-Boot is running as the first stage.

This is definitely not ideal (although fwiw if the GPLLs were configured
right then this same function could maybe work on QCS404 as well - the
RCGs are at the same physical addresses), but I don't think gating it
behind REMAKE_ELF or something is a great solution here.
> 
> -Sumit
> 
>> +       help
>> +         Select this to enable a debug UART using the serial_msm driver. You
>> +         will need to provide parameters to make this work. The driver will
>> +         be available until the real driver-model serial is running.
>> +
>>  config DEBUG_UART_MSM_GENI
>>         bool "Qualcomm snapdragon"
>>         depends on ARCH_SNAPDRAGON
>> diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
>> index f4d96313b931..44b93bd7ff21 100644
>> --- a/drivers/serial/serial_msm.c
>> +++ b/drivers/serial/serial_msm.c
>> @@ -252,3 +252,40 @@ U_BOOT_DRIVER(serial_msm) = {
>>         .probe = msm_serial_probe,
>>         .ops    = &msm_serial_ops,
>>  };
>> +
>> +#ifdef CONFIG_DEBUG_UART_MSM
>> +
>> +static struct msm_serial_data init_serial_data = {
>> +       .base = CONFIG_VAL(DEBUG_UART_BASE),
>> +       .clk_rate = 7372800,
>> +};
>> +
>> +#include <debug_uart.h>
>> +
>> +/* Uncomment to turn on UART clocks when debugging U-Boot as aboot on MSM8916 */
>> +//int apq8016_clk_init_uart(phys_addr_t gcc_base);
>> +
>> +static inline void _debug_uart_init(void)
>> +{
>> +       /* Uncomment to turn on UART clocks when debugging U-Boot as aboot on MSM8916 */
>> +       //apq8016_clk_init_uart(0x1800000);
>> +       uart_dm_init(&init_serial_data);
>> +}
>> +
>> +static inline void _debug_uart_putc(int ch)
>> +{
>> +       struct msm_serial_data *priv = &init_serial_data;
>> +
>> +       while (!(readl(priv->base + UARTDM_SR) & UARTDM_SR_TX_EMPTY) &&
>> +              !(readl(priv->base + UARTDM_ISR) & UARTDM_ISR_TX_READY))
>> +               ;
>> +
>> +       writel(UARTDM_CR_CMD_RESET_TX_READY, priv->base + UARTDM_CR);
>> +
>> +       writel(1, priv->base + UARTDM_NCF_TX);
>> +       writel(ch, priv->base + UARTDM_TF);
>> +}
>> +
>> +DEBUG_UART_FUNCS
>> +
>> +#endif
>>
>> --
>> 2.43.1
>>

On Tue, 20 Feb 2024 at 17:09, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>
>
>
> On 20/02/2024 06:08, Sumit Garg wrote:
> > On Fri, 16 Feb 2024 at 02:22, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> >>
> >> Introduce support for early debugging. This relies on the previous stage
> >> bootloader to initialise the UART clocks, when running with U-Boot as
> >> the primary bootloader this feature doesn't work. It will require a way
> >> to configure the clocks before the driver model is available.
> >>
> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> >> ---
> >>  drivers/serial/Kconfig      |  8 ++++++++
> >>  drivers/serial/serial_msm.c | 37 +++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 45 insertions(+)
> >>
> >> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> >> index 26460c4e0cab..fbd351a47859 100644
> >> --- a/drivers/serial/Kconfig
> >> +++ b/drivers/serial/Kconfig
> >> @@ -319,6 +319,14 @@ config DEBUG_UART_S5P
> >>           will need to provide parameters to make this work. The driver will
> >>           be available until the real driver-model serial is running.
> >>
> >> +config DEBUG_UART_MSM
> >> +       bool "Qualcomm QUP UART debug"
> >> +       depends on ARCH_SNAPDRAGON
> >
> > Since this debug UART only works for chainloaded configuration, can we
> > somehow add explicit dependency here? Something like !REMAKE_ELF?
>
> With a small patch (which didn't make it into v4 apparently) the
> apq8016_clk_init_uart() function from clock-apq8016 can be adjusted to
> just take a base address rather than "struct msm_clk_priv". It can then
> be called from debug_uart_init() and allows for debug UART to be used
> when U-Boot is running as the first stage.
>
> This is definitely not ideal (although fwiw if the GPLLs were configured
> right then this same function could maybe work on QCS404 as well

QCS404 is chainloaded config too, so the debug UART should work there.

> - the
> RCGs are at the same physical addresses), but I don't think gating it
> behind REMAKE_ELF or something is a great solution here.

I don't have a strong opinion here and I could live with just a
documentation update for debug UART too.

-Sumit