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,
[PATCH 3/3] serial: msm: calculate bit clock divider
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
---
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, 768,
512, 384,
+ 256, 192, 128, 96, 64, 48,
32, 16};
+ int i = ARRAY_SIZE(csr_div_table) - 1;
+ /* Currently we only support one baudrate */
+ int baud = 115200;
+
+ for (; i >= 0; i--) {
+ int x = priv->clk_rate / csr_div_table[i];
+
+ if (x == baud)
+ /* Duplicate the configuration for RX
+* as the lower nybble only configures TX
+*/
+ return i + (i << 4);
+ }
+
+
