Re: [PATCH 3/3] serial: msm: calculate bit clock divider
On Mon, Apr 15, 2024 at 4:18 PM Caleb Connolly
wrote:
>
>
>
> On 15/04/2024 14:05, Robert Marko wrote:
> > On Mon, Apr 15, 2024 at 2:44 PM Caleb Connolly
> > wrote:
> >>
> >> The driver currently requires the bit clock divider be hardcoded in
> >> devicetree (or use the hardcoded default from apq8016).
> >>
> >> The bit clock divider is used to derive the baud rate from the core
> >> clock:
> >>
> >> baudrate = clk_rate / csr_div
> >>
> >> clk_rate is the actual programmed core clock rate which is returned by
> >> clk_set_rate(), and this UART driver only supports a baudrate of 115200.
> >> We can therefore determine the appropriate value for UARTDM_CSR by
> >> iterating over the possible values and finding the one where the
> >> equation above holds true for a baudrate of 115200.
> >>
> >> Implement this logic and drop the non-standard DT bindings for this
> >> driver.
> >>
> >> Tested on dragonboard410c.
> >>
> >> Signed-off-by: Caleb Connolly
> >
> > Works on Alfa AP120C (IPQ4018) with full DM UART, but debug UART
> > prints junk since .clk_rate = 7372800 is not correct for IPQ40xx.
> > I would suggest using .clk_rate = CONFIG_VAL(DEBUG_UART_CLOCK) instead
> > to populate the value per board, this also avoids per ARCH ifdefs.
>
> Ok awesome, thanks for trying this out. I'll send a v2 with your suggestion.
>
> Can I add your Tested-by?
Sure,
Tested-by: Robert Marko
Regards,
Robert
> >
> > Regards,
> > Robert
> >> ---
> >> Cc: Robert Marko
> >> ---
> >> doc/device-tree-bindings/serial/msm-serial.txt | 10 ---
> >> drivers/serial/serial_msm.c| 87
> >> +-
> >> 2 files changed, 70 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/doc/device-tree-bindings/serial/msm-serial.txt
> >> b/doc/device-tree-bindings/serial/msm-serial.txt
> >> deleted file mode 100644
> >> index dca995798a90..
> >> --- a/doc/device-tree-bindings/serial/msm-serial.txt
> >> +++ /dev/null
> >> @@ -1,10 +0,0 @@
> >> -Qualcomm UART (Data Mover mode)
> >> -
> >> -Required properties:
> >> -- compatible: must be "qcom,msm-uartdm-v1.4"
> >> -- reg: start address and size of the registers
> >> -- clock: interface clock (must accept baudrate as a frequency)
> >> -
> >> -Optional properties:
> >> -- bit-rate: Data Mover bit rate register value
> >> - (If not defined then 0xCC is used as default)
> >> diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
> >> index 8044d38518db..e461929b4338 100644
> >> --- a/drivers/serial/serial_msm.c
> >> +++ b/drivers/serial/serial_msm.c
> >> @@ -31,8 +31,18 @@
> >> #define UARTDM_RXFS_BUF_SHIFT 0x7 /* Number of bytes in the packing
> >> buffer */
> >> #define UARTDM_RXFS_BUF_MASK0x7
> >> #define UARTDM_MR1 0x00
> >> #define UARTDM_MR2 0x04
> >> +/*
> >> + * This is documented on page 1817 of the apq8016e technical reference
> >> manual.
> >> + * section 6.2.5.3.26
> >> + *
> >> + * The upper nybble contains the bit clock divider for the RX pin, the
> >> lower
> >> + * nybble defines the TX pin. In almost all cases these should be the
> >> same value.
> >> + *
> >> + * The baud rate is the core clock frequency divided by the fixed divider
> >> value
> >> + * programmed into this register (defined in calc_csr_bitrate()).
> >> + */
> >> #define UARTDM_CSR 0xA0
> >>
> >> #define UARTDM_SR0xA4 /* Status register */
> >> #define UARTDM_SR_RX_READY (1 << 0) /* Word is the receiver FIFO */
> >> @@ -52,9 +62,8 @@
> >>
> >> #define UARTDM_TF 0x100 /* UART Transmit FIFO register */
> >> #define UARTDM_RF 0x140 /* UART Receive FIFO register */
> >>
> >> -#define UART_DM_CLK_RX_TX_BIT_RATE 0xCC
> >> #define MSM_BOOT_UART_DM_8_N_1_MODE 0x34
> >> #define MSM_BOOT_UART_DM_CMD_RESET_RX 0x10
> >> #define MSM_BOOT_UART_DM_CMD_RESET_TX 0x20
> >>
> >> @@ -63,9 +72,9 @@ DECLARE_GLOBAL_DATA_PTR;
> >> struct msm_serial_data {
> >> phys_addr_t base;
> >> unsigned chars_cnt; /* number of buffered chars */
> >> uint32_t chars_buf; /* buffered chars */
> >> - uint32_t clk_bit_rate; /* data mover mode bit rate register value
> >> */
> >> + uint32_t clk_rate; /* core clock rate */
> >> };
> >>
> >> static int msm_serial_fetch(struct udevice *dev)
> >> {
> >> @@ -155,34 +164,63 @@ static const struct dm_serial_ops msm_serial_ops = {
> >> .pending = msm_serial_pending,
> >> .getc = msm_serial_getc,
> >> };
> >>
> >> -static int msm_uart_clk_init(struct udevice *dev)
> >> +static long msm_uart_clk_init(struct udevice *dev)
> >> {
> >> - uint clk_rate = fdtdec_get_uint(gd->fdt_blob, dev_of_offset(dev),
> >> - "clock-frequency", 115200);
> >> + struct msm_serial_data *priv = dev_get_priv(dev);
> >> struct clk clk;
> >> int ret;
> >> + long rate;
Re: [PATCH 3/3] serial: msm: calculate bit clock divider
On 15/04/2024 14:05, Robert Marko wrote:
> On Mon, Apr 15, 2024 at 2:44 PM Caleb Connolly
> wrote:
>>
>> The driver currently requires the bit clock divider be hardcoded in
>> devicetree (or use the hardcoded default from apq8016).
>>
>> The bit clock divider is used to derive the baud rate from the core
>> clock:
>>
>> baudrate = clk_rate / csr_div
>>
>> clk_rate is the actual programmed core clock rate which is returned by
>> clk_set_rate(), and this UART driver only supports a baudrate of 115200.
>> We can therefore determine the appropriate value for UARTDM_CSR by
>> iterating over the possible values and finding the one where the
>> equation above holds true for a baudrate of 115200.
>>
>> Implement this logic and drop the non-standard DT bindings for this
>> driver.
>>
>> Tested on dragonboard410c.
>>
>> Signed-off-by: Caleb Connolly
>
> Works on Alfa AP120C (IPQ4018) with full DM UART, but debug UART
> prints junk since .clk_rate = 7372800 is not correct for IPQ40xx.
> I would suggest using .clk_rate = CONFIG_VAL(DEBUG_UART_CLOCK) instead
> to populate the value per board, this also avoids per ARCH ifdefs.
Ok awesome, thanks for trying this out. I'll send a v2 with your suggestion.
Can I add your Tested-by?
>
> Regards,
> Robert
>> ---
>> Cc: Robert Marko
>> ---
>> doc/device-tree-bindings/serial/msm-serial.txt | 10 ---
>> drivers/serial/serial_msm.c| 87
>> +-
>> 2 files changed, 70 insertions(+), 27 deletions(-)
>>
>> diff --git a/doc/device-tree-bindings/serial/msm-serial.txt
>> b/doc/device-tree-bindings/serial/msm-serial.txt
>> deleted file mode 100644
>> index dca995798a90..
>> --- a/doc/device-tree-bindings/serial/msm-serial.txt
>> +++ /dev/null
>> @@ -1,10 +0,0 @@
>> -Qualcomm UART (Data Mover mode)
>> -
>> -Required properties:
>> -- compatible: must be "qcom,msm-uartdm-v1.4"
>> -- reg: start address and size of the registers
>> -- clock: interface clock (must accept baudrate as a frequency)
>> -
>> -Optional properties:
>> -- bit-rate: Data Mover bit rate register value
>> - (If not defined then 0xCC is used as default)
>> diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
>> index 8044d38518db..e461929b4338 100644
>> --- a/drivers/serial/serial_msm.c
>> +++ b/drivers/serial/serial_msm.c
>> @@ -31,8 +31,18 @@
>> #define UARTDM_RXFS_BUF_SHIFT 0x7 /* Number of bytes in the packing
>> buffer */
>> #define UARTDM_RXFS_BUF_MASK0x7
>> #define UARTDM_MR1 0x00
>> #define UARTDM_MR2 0x04
>> +/*
>> + * This is documented on page 1817 of the apq8016e technical reference
>> manual.
>> + * section 6.2.5.3.26
>> + *
>> + * The upper nybble contains the bit clock divider for the RX pin, the lower
>> + * nybble defines the TX pin. In almost all cases these should be the same
>> value.
>> + *
>> + * The baud rate is the core clock frequency divided by the fixed divider
>> value
>> + * programmed into this register (defined in calc_csr_bitrate()).
>> + */
>> #define UARTDM_CSR 0xA0
>>
>> #define UARTDM_SR0xA4 /* Status register */
>> #define UARTDM_SR_RX_READY (1 << 0) /* Word is the receiver FIFO */
>> @@ -52,9 +62,8 @@
>>
>> #define UARTDM_TF 0x100 /* UART Transmit FIFO register */
>> #define UARTDM_RF 0x140 /* UART Receive FIFO register */
>>
>> -#define UART_DM_CLK_RX_TX_BIT_RATE 0xCC
>> #define MSM_BOOT_UART_DM_8_N_1_MODE 0x34
>> #define MSM_BOOT_UART_DM_CMD_RESET_RX 0x10
>> #define MSM_BOOT_UART_DM_CMD_RESET_TX 0x20
>>
>> @@ -63,9 +72,9 @@ DECLARE_GLOBAL_DATA_PTR;
>> struct msm_serial_data {
>> phys_addr_t base;
>> unsigned chars_cnt; /* number of buffered chars */
>> uint32_t chars_buf; /* buffered chars */
>> - uint32_t clk_bit_rate; /* data mover mode bit rate register value */
>> + uint32_t clk_rate; /* core clock rate */
>> };
>>
>> static int msm_serial_fetch(struct udevice *dev)
>> {
>> @@ -155,34 +164,63 @@ static const struct dm_serial_ops msm_serial_ops = {
>> .pending = msm_serial_pending,
>> .getc = msm_serial_getc,
>> };
>>
>> -static int msm_uart_clk_init(struct udevice *dev)
>> +static long msm_uart_clk_init(struct udevice *dev)
>> {
>> - uint clk_rate = fdtdec_get_uint(gd->fdt_blob, dev_of_offset(dev),
>> - "clock-frequency", 115200);
>> + struct msm_serial_data *priv = dev_get_priv(dev);
>> struct clk clk;
>> int ret;
>> + long rate;
>>
>> ret = clk_get_by_name(dev, "core", );
>> if (ret < 0) {
>> pr_warn("%s: Failed to get clock: %d\n", __func__, ret);
>> - return ret;
>> + return 0;
>> }
>>
>> - ret = clk_set_rate(, clk_rate);
>> - if (ret < 0)
>> - return ret;
>> + rate = clk_set_rate(,
Re: [PATCH 3/3] serial: msm: calculate bit clock divider
On Mon, Apr 15, 2024 at 2:44 PM Caleb Connolly
wrote:
>
> The driver currently requires the bit clock divider be hardcoded in
> devicetree (or use the hardcoded default from apq8016).
>
> The bit clock divider is used to derive the baud rate from the core
> clock:
>
> baudrate = clk_rate / csr_div
>
> clk_rate is the actual programmed core clock rate which is returned by
> clk_set_rate(), and this UART driver only supports a baudrate of 115200.
> We can therefore determine the appropriate value for UARTDM_CSR by
> iterating over the possible values and finding the one where the
> equation above holds true for a baudrate of 115200.
>
> Implement this logic and drop the non-standard DT bindings for this
> driver.
>
> Tested on dragonboard410c.
>
> Signed-off-by: Caleb Connolly
Works on Alfa AP120C (IPQ4018) with full DM UART, but debug UART
prints junk since .clk_rate = 7372800 is not correct for IPQ40xx.
I would suggest using .clk_rate = CONFIG_VAL(DEBUG_UART_CLOCK) instead
to populate the value per board, this also avoids per ARCH ifdefs.
Regards,
Robert
> ---
> Cc: Robert Marko
> ---
> doc/device-tree-bindings/serial/msm-serial.txt | 10 ---
> drivers/serial/serial_msm.c| 87
> +-
> 2 files changed, 70 insertions(+), 27 deletions(-)
>
> diff --git a/doc/device-tree-bindings/serial/msm-serial.txt
> b/doc/device-tree-bindings/serial/msm-serial.txt
> deleted file mode 100644
> index dca995798a90..
> --- a/doc/device-tree-bindings/serial/msm-serial.txt
> +++ /dev/null
> @@ -1,10 +0,0 @@
> -Qualcomm UART (Data Mover mode)
> -
> -Required properties:
> -- compatible: must be "qcom,msm-uartdm-v1.4"
> -- reg: start address and size of the registers
> -- clock: interface clock (must accept baudrate as a frequency)
> -
> -Optional properties:
> -- bit-rate: Data Mover bit rate register value
> - (If not defined then 0xCC is used as default)
> diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
> index 8044d38518db..e461929b4338 100644
> --- a/drivers/serial/serial_msm.c
> +++ b/drivers/serial/serial_msm.c
> @@ -31,8 +31,18 @@
> #define UARTDM_RXFS_BUF_SHIFT 0x7 /* Number of bytes in the packing
> buffer */
> #define UARTDM_RXFS_BUF_MASK0x7
> #define UARTDM_MR1 0x00
> #define UARTDM_MR2 0x04
> +/*
> + * This is documented on page 1817 of the apq8016e technical reference
> manual.
> + * section 6.2.5.3.26
> + *
> + * The upper nybble contains the bit clock divider for the RX pin, the lower
> + * nybble defines the TX pin. In almost all cases these should be the same
> value.
> + *
> + * The baud rate is the core clock frequency divided by the fixed divider
> value
> + * programmed into this register (defined in calc_csr_bitrate()).
> + */
> #define UARTDM_CSR 0xA0
>
> #define UARTDM_SR0xA4 /* Status register */
> #define UARTDM_SR_RX_READY (1 << 0) /* Word is the receiver FIFO */
> @@ -52,9 +62,8 @@
>
> #define UARTDM_TF 0x100 /* UART Transmit FIFO register */
> #define UARTDM_RF 0x140 /* UART Receive FIFO register */
>
> -#define UART_DM_CLK_RX_TX_BIT_RATE 0xCC
> #define MSM_BOOT_UART_DM_8_N_1_MODE 0x34
> #define MSM_BOOT_UART_DM_CMD_RESET_RX 0x10
> #define MSM_BOOT_UART_DM_CMD_RESET_TX 0x20
>
> @@ -63,9 +72,9 @@ DECLARE_GLOBAL_DATA_PTR;
> struct msm_serial_data {
> phys_addr_t base;
> unsigned chars_cnt; /* number of buffered chars */
> uint32_t chars_buf; /* buffered chars */
> - uint32_t clk_bit_rate; /* data mover mode bit rate register value */
> + uint32_t clk_rate; /* core clock rate */
> };
>
> static int msm_serial_fetch(struct udevice *dev)
> {
> @@ -155,34 +164,63 @@ static const struct dm_serial_ops msm_serial_ops = {
> .pending = msm_serial_pending,
> .getc = msm_serial_getc,
> };
>
> -static int msm_uart_clk_init(struct udevice *dev)
> +static long msm_uart_clk_init(struct udevice *dev)
> {
> - uint clk_rate = fdtdec_get_uint(gd->fdt_blob, dev_of_offset(dev),
> - "clock-frequency", 115200);
> + struct msm_serial_data *priv = dev_get_priv(dev);
> struct clk clk;
> int ret;
> + long rate;
>
> ret = clk_get_by_name(dev, "core", );
> if (ret < 0) {
> pr_warn("%s: Failed to get clock: %d\n", __func__, ret);
> - return ret;
> + return 0;
> }
>
> - ret = clk_set_rate(, clk_rate);
> - if (ret < 0)
> - return ret;
> + rate = clk_set_rate(, priv->clk_rate);
>
> - return 0;
> + return rate;
> +}
> +
> +static int calc_csr_bitrate(struct msm_serial_data *priv)
> +{
> + /* This table is from the TRE. See the definition of UARTDM_CSR */
> + unsigned int csr_div_table[] = {24576, 12288, 6144, 3072, 1536,
