[PATCH 0/3] drivers: usb: serial: mos7840: Improved baud rate support

[PATCH 0/3] drivers: usb: serial: mos7840: Improved baud rate support

[Tony Chung]

This patch set has made some improvements for baud rate setup:
1. Optimized register configurations for common baud rates.
2. Added clock select registers to choose between 96M, 30M, or external clock sources,
   allowing baud rate setup to 6M, matching the hardware's maximum data rate.

Tony Chung (3):
  drivers: usb: serial: mos7840: Add defines for clock select register
    offset
  drivers: usb: serial: mos7840: added optimized register setups for
    common baudrates.
  driver: usb: serial: mos7840: add more baudrate options

 drivers/usb/serial/mos7840.c | 273 ++++++++++++++++++++++++++++++++++-
 1 file changed, 269 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH 1/3] drivers: usb: serial: mos7840: Add defines for clock select register offset

[Tony Chung]

This patch adds define for CLOCK_SELECT_REG1 & CLOCK_SELECT_REG2 offsets.
These two registers can select clock source between 30M/96M/External.

Signed-off-by: Tony Chung <tony467913@gmail.com>
---
 drivers/usb/serial/mos7840.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index ca3da79af..362875a53 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -144,6 +144,9 @@
 
 #define SERIAL_LCR_DLAB            ((__u16)(0x0080))
 
+#define CLOCK_SELECT_REG1          ((__u16)(0x13))
+#define CLOCK_SELECT_REG2          ((__u16)(0x14))
+
 /*
  * URB POOL related defines
  */
-- 
2.34.1

Re: [PATCH 1/3] drivers: usb: serial: mos7840: Add defines for clock select register offset

[Johan Hovold]

On Thu, Oct 24, 2024 at 06:09:01PM +0800, Tony Chung wrote:
> This patch adds define for CLOCK_SELECT_REG1 & CLOCK_SELECT_REG2 offsets.
> These two registers can select clock source between 30M/96M/External.

I think should just add these as part of the patch that uses them. If
you have some information on how they are used, that would be useful to
have in the commit message and possibly also in a comment above the
defines.

Where did you learn about these settings?

Johan

[PATCH 2/3] drivers: usb: serial: mos7840: added optimized register setups for common baudrates.

[Tony Chung]

added optimized register setups for common baudrates.

Signed-off-by: Tony Chung <tony467913@gmail.com>
---
 drivers/usb/serial/mos7840.c | 114 ++++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index 362875a53..acc16737b 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -1063,11 +1063,116 @@ static int mos7840_calc_baud_rate_divisor(struct usb_serial_port *port,
 {
 	dev_dbg(&port->dev, "%s - %d\n", __func__, baudRate);
 
-	if (baudRate <= 115200) {
+	// divisor = (256*DLM)+DLL
+	// baudrate = InputCLK/(16*Divisor)
+	if (baudRate == 50) {
+		*divisor = (256*0x09)+0x04; // DLM=0x09, DLL=0x04
+		*clk_sel_val = 0x0;	    // clock source = 1.846153846M
+	} else if (baudRate == 75) {
+		*divisor = (256*0x06)+0x02; // DLM=0x06, DLL=0x02
+		*clk_sel_val = 0x0;	    // clock source = 1.846153846M
+	} else if (baudRate == 110) {
+		*divisor = (256*0x04)+0x19; // DLM=0x04, DLL=0x19
+		*clk_sel_val = 0x0;	    // clock source = 1.846153846M
+	} else if (baudRate == 134) {
+		*divisor = (256*0x03)+0x5d; // DLM=0x03, DLL=0x5d
+		*clk_sel_val = 0x0;	    // clock source = 1.846153846M
+	} else if (baudRate == 150) {
+		*divisor = (256*0x03)+0x01; // DLM=0x03, DLL=0x01
+		*clk_sel_val = 0x0;	    // clock source = 1.846153846M
+	} else if (baudRate == 300) {
+		*divisor = (256*0x01)+0x81; // DLM=0x01, DLL=0x81
+		*clk_sel_val = 0x0;	    // clock source = 1.846153846M
+	} else if (baudRate == 600) {
+		*divisor = 0xc0;		// DLM=0, DLL=0xc0
+		*clk_sel_val = 0x0;		// clock source = 1.846153846M
+	} else if (baudRate == 1200) {
+		*divisor = 0x60;		// DLM=0, DLL=0x60
+		*clk_sel_val = 0x0;		// clock source = 1.846153846M
+	} else if (baudRate == 1800) {
+		*divisor = 0x40;		// DLM=0, DLL=0x40
+		*clk_sel_val = 0x0;		// clock source = 1.846153846M
+	} else if (baudRate == 2400) {
+		*divisor = 0x30;		// DLM=0, DLL=0x30
+		*clk_sel_val = 0x0;		// clock source = 1.846153846M
+	} else if (baudRate == 4800) {
+		*divisor = 0x18;		// DLM=0, DLL=0x18
+		*clk_sel_val = 0x0;		// clock source = 1.846153846M
+	} else if (baudRate == 7200) {
+		*divisor = 0x10;		// DLM=0, DLL=0x10
+		*clk_sel_val = 0x0;		// clock source = 1.846153846M
+	} else if (baudRate == 9600) {
+		*divisor = 0x0c;		// DLM=0, DLL=0x0c
+		*clk_sel_val = 0x0;		// clock source = 1.846153846M
+	} else if (baudRate == 14400) {
+		*divisor = 0x08;		// DLM=0, DLL=0x08
+		*clk_sel_val = 0x0;		// clock source = 1.846153846M
+	} else if (baudRate == 19200) {
+		*divisor = 0x06;		// DLM=0, DLL=0x06
+		*clk_sel_val = 0x0;		// clock source = 1.846153846M
+	} else if (baudRate == 28800) {
+		*divisor = 0x04;		// DLM=0, DLL=0x04
+		*clk_sel_val = 0x0;		// clock source = 1.846153846M
+	} else if (baudRate == 38400) {
+		*divisor = 0x03;		// DLM=0, DLL=0x03
+		*clk_sel_val = 0x0;		// clock source = 1.846153846M
+	} else if (baudRate == 57600) {
+		*divisor = 0x02;		// DLM=0, DLL=0x02
+		*clk_sel_val = 0x0;		// clock source = 1.846153846M
+	} else if (baudRate == 115200) {
+		*divisor = 0x01;		// DLM=0, DLL=0x01
+		*clk_sel_val = 0x0;		// clock source = 1.846153846M
+	} else if (baudRate == 230400) {
+		*divisor = 0x01;		// DLM=0, DLL=0x01
+		*clk_sel_val = 0x10;	// clock source = 3.692307692M
+	} else if (baudRate == 460800) {
+		*divisor = 0x01;		// DLM=0, DLL=0x01
+		*clk_sel_val = 0x30;	// clock source = 7.384615384M
+	} else if (baudRate == 806400) {
+		*divisor = 0x01;		// DLM=0, DLL=0x01
+		*clk_sel_val = 0x40;	// clock source = 12.923076922M
+	} else if (baudRate == 921600) {
+		*divisor = 0x01;		// DLM=0, DLL=0x01
+		*clk_sel_val = 0x50;	// clock source = 14.769230768M
+	} else if (baudRate == 25000) {
+		*divisor = 0x78;		// DLM=0, DLL=0x78
+		*clk_sel_val = 0x70;	// clock source=48M
+	} else if (baudRate == 50000) {
+		*divisor = 0x3c;		// DLM=0, DLL=0x3c
+		*clk_sel_val = 0x70;	// clock source=48M
+	} else if (baudRate == 75000) {
+		*divisor = 0x28;		// DLM=0, DLL=0x28
+		*clk_sel_val = 0x70;	// clock source=48M
+	} else if (baudRate == 100000) {
+		*divisor = 0x1e;		// DLM=0, DLL=0x1e
+		*clk_sel_val = 0x70;	// clock source=48M
+	} else if (baudRate == 250000) {
+		*divisor = 0x0c;		// DLM=0, DLL=0x0c
+		*clk_sel_val = 0x70;	// clock source=48M
+	} else if (baudRate == 300000) {
+		*divisor = 0x0a;		// DLM=0, DLL=0x0a
+		*clk_sel_val = 0x70;	// clock source=48M
+	} else if (baudRate == 500000) {
+		*divisor = 0x06;		// DLM=0, DLL=0x06
+		*clk_sel_val = 0x70;	// clock source=48M
+	} else if (baudRate == 600000) {
+		*divisor = 0x05;		// DLM=0, DLL=0x05
+		*clk_sel_val = 0x70;	// clock source=48M
+	} else if (baudRate == 1000000) {
+		*divisor = 0x03;		// DLM=0, DLL=0x03
+		*clk_sel_val = 0x70;	// clock source=48M
+	} else if (baudRate == 3000000) {
+		*divisor = 0x01;		// DLM=0, DLL=0x01
+		*clk_sel_val = 0x70;	// clock source=48M
+
+	} else if (baudRate == 1500000) {
+		*divisor = 0x01;		// DLM=0, DLL=0x01
+		*clk_sel_val = 0x60;	// clock source=24M
+
+	} else if (baudRate <= 115200) {
 		*divisor = 115200 / baudRate;
 		*clk_sel_val = 0x0;
-	}
-	if ((baudRate > 115200) && (baudRate <= 230400)) {
+	} else if ((baudRate > 115200) && (baudRate <= 230400)) {
 		*divisor = 230400 / baudRate;
 		*clk_sel_val = 0x10;
 	} else if ((baudRate > 230400) && (baudRate <= 403200)) {
@@ -1088,6 +1193,9 @@ static int mos7840_calc_baud_rate_divisor(struct usb_serial_port *port,
 	} else if ((baudRate > 1572864) && (baudRate <= 3145728)) {
 		*divisor = 3145728 / baudRate;
 		*clk_sel_val = 0x70;
+	} else {
+		dev_dbg(&port->dev, "func: %s -baudrate %d not supported.\n", __func__, baudRate);
+		return -1;
 	}
 	return 0;
 }
-- 
2.34.1

Re: [PATCH 2/3] drivers: usb: serial: mos7840: added optimized register setups for common baudrates.

[Johan Hovold]

On Thu, Oct 24, 2024 at 06:09:03PM +0800, Tony Chung wrote:
> added optimized register setups for common baudrates.

First, how did you determine the base clocks here? Do you have access to
some documentation or did you just measure them?

These are all derived from the internal PLL IIUC?

> Signed-off-by: Tony Chung <tony467913@gmail.com>
> ---
>  drivers/usb/serial/mos7840.c | 114 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 111 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
> index 362875a53..acc16737b 100644
> --- a/drivers/usb/serial/mos7840.c
> +++ b/drivers/usb/serial/mos7840.c
> @@ -1063,11 +1063,116 @@ static int mos7840_calc_baud_rate_divisor(struct usb_serial_port *port,
>  {
>  	dev_dbg(&port->dev, "%s - %d\n", __func__, baudRate);
>  
> -	if (baudRate <= 115200) {
> +	// divisor = (256*DLM)+DLL
> +	// baudrate = InputCLK/(16*Divisor)

Style nit: Please avoid using c99 comments (here and below), just use a
block comment here and add spaces around the operators.

> +	if (baudRate == 50) {
> +		*divisor = (256*0x09)+0x04; // DLM=0x09, DLL=0x04
> +		*clk_sel_val = 0x0;	    // clock source = 1.846153846M
> +	} else if (baudRate == 75) {
> +		*divisor = (256*0x06)+0x02; // DLM=0x06, DLL=0x02
> +		*clk_sel_val = 0x0;	    // clock source = 1.846153846M
> +	} else if (baudRate == 110) {
> +		*divisor = (256*0x04)+0x19; // DLM=0x04, DLL=0x19
> +		*clk_sel_val = 0x0;	    // clock source = 1.846153846M
> +	} else if (baudRate == 134) {
> +		*divisor = (256*0x03)+0x5d; // DLM=0x03, DLL=0x5d
> +		*clk_sel_val = 0x0;	    // clock source = 1.846153846M
> +	} else if (baudRate == 150) {
> +		*divisor = (256*0x03)+0x01; // DLM=0x03, DLL=0x01
> +		*clk_sel_val = 0x0;	    // clock source = 1.846153846M
> +	} else if (baudRate == 300) {
> +		*divisor = (256*0x01)+0x81; // DLM=0x01, DLL=0x81
> +		*clk_sel_val = 0x0;	    // clock source = 1.846153846M
> +	} else if (baudRate == 600) {
> +		*divisor = 0xc0;		// DLM=0, DLL=0xc0
> +		*clk_sel_val = 0x0;		// clock source = 1.846153846M
> +	} else if (baudRate == 1200) {
> +		*divisor = 0x60;		// DLM=0, DLL=0x60
> +		*clk_sel_val = 0x0;		// clock source = 1.846153846M
> +	} else if (baudRate == 1800) {
> +		*divisor = 0x40;		// DLM=0, DLL=0x40
> +		*clk_sel_val = 0x0;		// clock source = 1.846153846M
> +	} else if (baudRate == 2400) {
> +		*divisor = 0x30;		// DLM=0, DLL=0x30
> +		*clk_sel_val = 0x0;		// clock source = 1.846153846M
> +	} else if (baudRate == 4800) {
> +		*divisor = 0x18;		// DLM=0, DLL=0x18
> +		*clk_sel_val = 0x0;		// clock source = 1.846153846M
> +	} else if (baudRate == 7200) {
> +		*divisor = 0x10;		// DLM=0, DLL=0x10
> +		*clk_sel_val = 0x0;		// clock source = 1.846153846M
> +	} else if (baudRate == 9600) {
> +		*divisor = 0x0c;		// DLM=0, DLL=0x0c
> +		*clk_sel_val = 0x0;		// clock source = 1.846153846M
> +	} else if (baudRate == 14400) {
> +		*divisor = 0x08;		// DLM=0, DLL=0x08
> +		*clk_sel_val = 0x0;		// clock source = 1.846153846M
> +	} else if (baudRate == 19200) {
> +		*divisor = 0x06;		// DLM=0, DLL=0x06
> +		*clk_sel_val = 0x0;		// clock source = 1.846153846M
> +	} else if (baudRate == 28800) {
> +		*divisor = 0x04;		// DLM=0, DLL=0x04
> +		*clk_sel_val = 0x0;		// clock source = 1.846153846M
> +	} else if (baudRate == 38400) {
> +		*divisor = 0x03;		// DLM=0, DLL=0x03
> +		*clk_sel_val = 0x0;		// clock source = 1.846153846M
> +	} else if (baudRate == 57600) {
> +		*divisor = 0x02;		// DLM=0, DLL=0x02
> +		*clk_sel_val = 0x0;		// clock source = 1.846153846M
> +	} else if (baudRate == 115200) {
> +		*divisor = 0x01;		// DLM=0, DLL=0x01
> +		*clk_sel_val = 0x0;		// clock source = 1.846153846M
> +	} else if (baudRate == 230400) {
> +		*divisor = 0x01;		// DLM=0, DLL=0x01
> +		*clk_sel_val = 0x10;	// clock source = 3.692307692M
> +	} else if (baudRate == 460800) {
> +		*divisor = 0x01;		// DLM=0, DLL=0x01
> +		*clk_sel_val = 0x30;	// clock source = 7.384615384M
> +	} else if (baudRate == 806400) {
> +		*divisor = 0x01;		// DLM=0, DLL=0x01
> +		*clk_sel_val = 0x40;	// clock source = 12.923076922M
> +	} else if (baudRate == 921600) {
> +		*divisor = 0x01;		// DLM=0, DLL=0x01
> +		*clk_sel_val = 0x50;	// clock source = 14.769230768M
> +	} else if (baudRate == 25000) {
> +		*divisor = 0x78;		// DLM=0, DLL=0x78
> +		*clk_sel_val = 0x70;	// clock source=48M
> +	} else if (baudRate == 50000) {
> +		*divisor = 0x3c;		// DLM=0, DLL=0x3c
> +		*clk_sel_val = 0x70;	// clock source=48M
> +	} else if (baudRate == 75000) {
> +		*divisor = 0x28;		// DLM=0, DLL=0x28
> +		*clk_sel_val = 0x70;	// clock source=48M
> +	} else if (baudRate == 100000) {
> +		*divisor = 0x1e;		// DLM=0, DLL=0x1e
> +		*clk_sel_val = 0x70;	// clock source=48M
> +	} else if (baudRate == 250000) {
> +		*divisor = 0x0c;		// DLM=0, DLL=0x0c
> +		*clk_sel_val = 0x70;	// clock source=48M
> +	} else if (baudRate == 300000) {
> +		*divisor = 0x0a;		// DLM=0, DLL=0x0a
> +		*clk_sel_val = 0x70;	// clock source=48M
> +	} else if (baudRate == 500000) {
> +		*divisor = 0x06;		// DLM=0, DLL=0x06
> +		*clk_sel_val = 0x70;	// clock source=48M
> +	} else if (baudRate == 600000) {
> +		*divisor = 0x05;		// DLM=0, DLL=0x05
> +		*clk_sel_val = 0x70;	// clock source=48M
> +	} else if (baudRate == 1000000) {
> +		*divisor = 0x03;		// DLM=0, DLL=0x03
> +		*clk_sel_val = 0x70;	// clock source=48M
> +	} else if (baudRate == 3000000) {
> +		*divisor = 0x01;		// DLM=0, DLL=0x01
> +		*clk_sel_val = 0x70;	// clock source=48M
> +
> +	} else if (baudRate == 1500000) {
> +		*divisor = 0x01;		// DLM=0, DLL=0x01
> +		*clk_sel_val = 0x60;	// clock source=24M
> +

You should be able to determine at least most of the above by just
calculating the divisors from the base clocks. Perhaps you need a lookup
table as well to determine which base to use in some cases too.

Note sure we need to add non-standard rates like 25000 etc either.

> +	} else if (baudRate <= 115200) {
>  		*divisor = 115200 / baudRate;
>  		*clk_sel_val = 0x0;
> -	}
> -	if ((baudRate > 115200) && (baudRate <= 230400)) {
> +	} else if ((baudRate > 115200) && (baudRate <= 230400)) {
>  		*divisor = 230400 / baudRate;
>  		*clk_sel_val = 0x10;
>  	} else if ((baudRate > 230400) && (baudRate <= 403200)) {
> @@ -1088,6 +1193,9 @@ static int mos7840_calc_baud_rate_divisor(struct usb_serial_port *port,
>  	} else if ((baudRate > 1572864) && (baudRate <= 3145728)) {
>  		*divisor = 3145728 / baudRate;
>  		*clk_sel_val = 0x70;
> +	} else {
> +		dev_dbg(&port->dev, "func: %s -baudrate %d not supported.\n", __func__, baudRate);

Here you could drop "func: %s -" and the period, if you need this at
all.

> +		return -1;

The calling code does not check for errors, so either fix that or,
better still, make sure the function always returns a valid divisor
(e.g. by making sure the input rate is always valid).

>  	}
>  	return 0;
>  }

Johan

[PATCH 3/3] driver: usb: serial: mos7840: add more baudrate options

[Tony Chung]

Adds more baud rate options using 96M/30M/External clock sources.
To use these clock sources,
set through Clk_Select_Reg1 and Clk_Select_Reg2.

Signed-off-by: Tony Chung <tony467913@gmail.com>
---
 drivers/usb/serial/mos7840.c | 156 ++++++++++++++++++++++++++++++++++-
 1 file changed, 155 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index acc16737b..70ee4a638 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -1169,6 +1169,37 @@ static int mos7840_calc_baud_rate_divisor(struct usb_serial_port *port,
 		*divisor = 0x01;		// DLM=0, DLL=0x01
 		*clk_sel_val = 0x60;	// clock source=24M
 
+	/* below are using 96M or 30M clock source
+	 * will determine the clock source later
+	 * in function mos7840_send_cmd_write_baud_rate
+	 */
+	} else if (baudRate == 6000000) {
+		*divisor = 0x01;		// DLM=0, DLL=0x01
+		*clk_sel_val = 0x80;	// DUMMY val, clock source = 96M
+	} else if (baudRate == 2000000) {
+		*divisor = 0x03;		// DLM=0, DLL=0x03
+		*clk_sel_val = 0x80;	// DUMMY val, clock source = 96M
+	} else if (baudRate == 403200) {
+		*divisor = 0x0f;		// DLM=0, DLL=0x0f
+		*clk_sel_val = 0x80;	// DUMMY val, clock source = 96M
+	} else if (baudRate == 225000) {
+		*divisor = 0x1b;		// DLM=0, DLL=0x1b
+		*clk_sel_val = 0x80;	// DUMMY val, clock source = 96M
+	} else if (baudRate == 153600) {
+		*divisor = 0x27;		// DLM=0, DLL=0x27
+		*clk_sel_val = 0x80;	// DUMMY val, clock source = 96M
+
+	} else if (baudRate == 10000) {
+		*divisor = 0xbb;		// DLM=0, DLL=0xbb
+		*clk_sel_val = 0x80;	// DUMMY val, clock source = 30M
+	} else if (baudRate == 125000) {
+		*divisor = 0x0f;		// DLM=0, DLL=0x0f
+		*clk_sel_val = 0x80;	// DUMMY val, clock source = 30M
+	} else if (baudRate == 625000) {
+		*divisor = 0x03;		// DLM=0, DLL=0x03
+		*clk_sel_val = 0x80;	// DUMMY val, clock source = 30M
+
+
 	} else if (baudRate <= 115200) {
 		*divisor = 115200 / baudRate;
 		*clk_sel_val = 0x0;
@@ -1246,11 +1277,134 @@ static int mos7840_send_cmd_write_baud_rate(struct moschip_port *mos7840_port,
 
 	}
 
-	if (1) {		/* baudRate <= 115200) */
+	if (1) {
 		clk_sel_val = 0x0;
 		Data = 0x0;
 		status = mos7840_calc_baud_rate_divisor(port, baudRate, &divisor,
 						   &clk_sel_val);
+		if (status < 0) {
+			dev_dbg(&port->dev, "%s failed in set_serial_baud\n", __func__);
+			return -1;
+		}
+
+		/* Write clk_sel_val to SP_Reg or Clk_Select_Reg*/
+		// check clk_sel_val before setting the clk_sel_val
+		if (clk_sel_val == 0x80) {
+		// clk_sel_val is DUMMY value -> Write the corresponding value to Clk_Select_Reg
+			// 0x01:30M, 0x02:96M, 0x05:External Clock
+			if (baudRate == 125000 || baudRate == 625000 || baudRate == 10000) {
+				clk_sel_val = 0x01;
+			} else if (baudRate == 153600 || baudRate == 225000 || baudRate == 403200 ||
+					baudRate == 2000000 || baudRate == 6000000) {
+				clk_sel_val = 0x02;
+			} else {
+				clk_sel_val = 0x05; // externel clk for custom case.
+			}
+
+			// needs to set clock source through
+			// Clk_Select_Reg1(offset 0x13) & Clk_Select_Reg2(offset 0x14)
+			// Clk_Select_Reg1 for port1,2		Clk_Select_Reg2 for port3,4
+			if (mos7840_port->port_num <= 2) {
+				status = mos7840_get_reg_sync(port, CLOCK_SELECT_REG1, &Data);
+				if (status < 0) {
+					dev_dbg(&port->dev, "reading Clk_Select_Reg failed in set_serial_baud\n");
+					return -1;
+				}
+				if (mos7840_port->port_num == 1) {
+					Data = (Data & 0xf8) | clk_sel_val;
+					status =
+						mos7840_set_reg_sync(port, CLOCK_SELECT_REG1, Data);
+				} else if (mos7840_port->port_num == 2) {
+					Data = (Data & 0xc7) | (clk_sel_val<<3);
+					status =
+						mos7840_set_reg_sync(port, CLOCK_SELECT_REG1, Data);
+				}
+				if (status < 0) {
+					dev_dbg(&port->dev, "setting Clk_Select_Reg failed\n");
+					return -1;
+				}
+			} else if (mos7840_port->port_num <= 4) {
+				status = mos7840_get_reg_sync(port, CLOCK_SELECT_REG2, &Data);
+				if (status < 0) {
+					dev_dbg(&port->dev, "reading Clk_Select_Reg failed in set_serial_baud\n");
+					return -1;
+				}
+				if (mos7840_port->port_num == 3) {
+					Data = (Data & 0xf8) | clk_sel_val;
+					status =
+						mos7840_set_reg_sync(port, CLOCK_SELECT_REG2, Data);
+				} else if (mos7840_port->port_num == 4) {
+					Data = (Data & 0xc7) | (clk_sel_val<<3);
+					status =
+						mos7840_set_reg_sync(port, CLOCK_SELECT_REG2, Data);
+				}
+				if (status < 0) {
+					dev_dbg(&port->dev, "setting Clk_Select_Reg failed\n");
+					return -1;
+				}
+			}
+		} else {
+		// clk_sel_val is not DUMMY value -> Write the corresponding value to SP_Reg
+
+			/* First, needs to write default value to
+			 * Clk_Select_Reg1(offset 0x13) & Clk_Select_Reg2(offset 0x14)
+			 * Clk_Select_Reg1 for port1,2		Clk_Select_Reg2 for port3,4
+			 */
+			if (mos7840_port->port_num <= 2) {
+				status = mos7840_get_reg_sync(port, CLOCK_SELECT_REG1, &Data);
+				if (status < 0) {
+					dev_dbg(&port->dev, "reading Clk_Select_Reg failed in set_serial_baud\n");
+					return -1;
+				}
+				if (mos7840_port->port_num == 1) {
+					Data = (Data & 0xf8) | 0x00;
+					status =
+						mos7840_set_reg_sync(port, CLOCK_SELECT_REG1, Data);
+				} else if (mos7840_port->port_num == 2) {
+					Data = (Data & 0xc7) | (0x00<<3);
+					status =
+						mos7840_set_reg_sync(port, CLOCK_SELECT_REG1, Data);
+				}
+				if (status < 0) {
+					dev_dbg(&port->dev, "setting Clk_Select_Reg failed\n");
+					return -1;
+				}
+			} else if (mos7840_port->port_num <= 4) {
+				status = mos7840_get_reg_sync(port, CLOCK_SELECT_REG2, &Data);
+				if (status < 0) {
+					dev_dbg(&port->dev, "reading Clk_Select_Reg failed in set_serial_baud\n");
+					return -1;
+				}
+				if (mos7840_port->port_num == 3) {
+					Data = (Data & 0xf8) | 0x00;
+					status =
+						mos7840_set_reg_sync(port, CLOCK_SELECT_REG2, Data);
+				} else if (mos7840_port->port_num == 4) {
+					Data = (Data & 0xc7) | (0x00<<3);
+					status =
+						mos7840_set_reg_sync(port, CLOCK_SELECT_REG2, Data);
+				}
+				if (status < 0) {
+					dev_dbg(&port->dev, "setting Clk_Select_Reg failed\n");
+					return -1;
+				}
+			}
+			// select clock source by writing clk_sel_val to SPx_Reg
+			status = mos7840_get_reg_sync(port, mos7840_port->SpRegOffset,
+									 &Data);
+			if (status < 0) {
+				dev_dbg(&port->dev, "reading spreg failed in set_serial_baud\n");
+				return -1;
+			}
+			Data = (Data & 0x8f) | clk_sel_val;
+			status = mos7840_set_reg_sync(port, mos7840_port->SpRegOffset,
+									Data);
+			if (status < 0) {
+				dev_dbg(&port->dev, "Writing spreg failed in set_serial_baud\n");
+				return -1;
+			}
+		}
+
 		status = mos7840_get_reg_sync(port, mos7840_port->SpRegOffset,
 								 &Data);
 		if (status < 0) {
-- 
2.34.1

Re: [PATCH 3/3] driver: usb: serial: mos7840: add more baudrate options

[Johan Hovold]

On Thu, Oct 24, 2024 at 06:09:05PM +0800, Tony Chung wrote:
> Adds more baud rate options using 96M/30M/External clock sources.
> To use these clock sources,
> set through Clk_Select_Reg1 and Clk_Select_Reg2.
> 
> Signed-off-by: Tony Chung <tony467913@gmail.com>
> ---
>  drivers/usb/serial/mos7840.c | 156 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 155 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
> index acc16737b..70ee4a638 100644
> --- a/drivers/usb/serial/mos7840.c
> +++ b/drivers/usb/serial/mos7840.c
> @@ -1169,6 +1169,37 @@ static int mos7840_calc_baud_rate_divisor(struct usb_serial_port *port,
>  		*divisor = 0x01;		// DLM=0, DLL=0x01
>  		*clk_sel_val = 0x60;	// clock source=24M
>  
> +	/* below are using 96M or 30M clock source
> +	 * will determine the clock source later
> +	 * in function mos7840_send_cmd_write_baud_rate
> +	 */

Block comment style, also try to follow style of driver and capitalise
"Below".

That said, this one should not be needed after you add proper
abstractions for this. For example, instead of returning a raw
clk_sel_val you return an enum which you handle in the caller.

> +	} else if (baudRate == 6000000) {
> +		*divisor = 0x01;		// DLM=0, DLL=0x01
> +		*clk_sel_val = 0x80;	// DUMMY val, clock source = 96M
> +	} else if (baudRate == 2000000) {
> +		*divisor = 0x03;		// DLM=0, DLL=0x03
> +		*clk_sel_val = 0x80;	// DUMMY val, clock source = 96M

The above looks useful.

> +	} else if (baudRate == 403200) {
> +		*divisor = 0x0f;		// DLM=0, DLL=0x0f
> +		*clk_sel_val = 0x80;	// DUMMY val, clock source = 96M
> +	} else if (baudRate == 225000) {
> +		*divisor = 0x1b;		// DLM=0, DLL=0x1b
> +		*clk_sel_val = 0x80;	// DUMMY val, clock source = 96M
> +	} else if (baudRate == 153600) {
> +		*divisor = 0x27;		// DLM=0, DLL=0x27
> +		*clk_sel_val = 0x80;	// DUMMY val, clock source = 96M

But why would anyone use these?

> +	} else if (baudRate == 10000) {
> +		*divisor = 0xbb;		// DLM=0, DLL=0xbb
> +		*clk_sel_val = 0x80;	// DUMMY val, clock source = 30M
> +	} else if (baudRate == 125000) {
> +		*divisor = 0x0f;		// DLM=0, DLL=0x0f
> +		*clk_sel_val = 0x80;	// DUMMY val, clock source = 30M
> +	} else if (baudRate == 625000) {
> +		*divisor = 0x03;		// DLM=0, DLL=0x03
> +		*clk_sel_val = 0x80;	// DUMMY val, clock source = 30M

Or these?

In any case, these would probably also need to go in a lookup table.

> +
> +
>  	} else if (baudRate <= 115200) {
>  		*divisor = 115200 / baudRate;
>  		*clk_sel_val = 0x0;
> @@ -1246,11 +1277,134 @@ static int mos7840_send_cmd_write_baud_rate(struct moschip_port *mos7840_port,
>  
>  	}
>  
> -	if (1) {		/* baudRate <= 115200) */
> +	if (1) {

Probably a good time to get rid of this in a preparatory patch to reduce
the indentation.

>  		clk_sel_val = 0x0;
>  		Data = 0x0;
>  		status = mos7840_calc_baud_rate_divisor(port, baudRate, &divisor,
>  						   &clk_sel_val);
> +		if (status < 0) {
> +			dev_dbg(&port->dev, "%s failed in set_serial_baud\n", __func__);
> +			return -1;
> +		}

Ok, here you add the error handling. But as I mentioned it's better to
fix the caller so that it does not pass in a speed that too high for the
driver. Just keep the old rate and update the termios to reflect that
that requested rate was rejected.

> +
> +		/* Write clk_sel_val to SP_Reg or Clk_Select_Reg*/

Space before */

> +		// check clk_sel_val before setting the clk_sel_val

No c99 comments, please.

> +		if (clk_sel_val == 0x80) {
> +		// clk_sel_val is DUMMY value -> Write the corresponding value to Clk_Select_Reg

Odd indentation.

> +			// 0x01:30M, 0x02:96M, 0x05:External Clock

Ok, so here's the comment I asked for earlier. That should probably go
above the clock register defines, and/or with defines for these
constants added as well.

> +			if (baudRate == 125000 || baudRate == 625000 || baudRate == 10000) {
> +				clk_sel_val = 0x01;
> +			} else if (baudRate == 153600 || baudRate == 225000 || baudRate == 403200 ||
> +					baudRate == 2000000 || baudRate == 6000000) {
> +				clk_sel_val = 0x02;
> +			} else {
> +				clk_sel_val = 0x05; // externel clk for custom case.
> +			}

This needs to be cleaned up using a lookup table and a clk_sel enum.

If there are product that would benefit from using the external clock
input, this would need to be handled on a per-device basis so that we
know its frequency.

> +			// needs to set clock source through
> +			// Clk_Select_Reg1(offset 0x13) & Clk_Select_Reg2(offset 0x14)
> +			// Clk_Select_Reg1 for port1,2		Clk_Select_Reg2 for port3,4
> +			if (mos7840_port->port_num <= 2) {
> +				status = mos7840_get_reg_sync(port, CLOCK_SELECT_REG1, &Data);
> +				if (status < 0) {
> +					dev_dbg(&port->dev, "reading Clk_Select_Reg failed in set_serial_baud\n");

Looks like the driver is currently using dev_dbg() for errors, but this
should most likely be dev_err() throughout. No need to say in which
function it fails, just 
	
	failed to read clock select register: %d\n

or similar should do.

> +					return -1;
> +				}
> +				if (mos7840_port->port_num == 1) {
> +					Data = (Data & 0xf8) | clk_sel_val;
> +					status =
> +						mos7840_set_reg_sync(port, CLOCK_SELECT_REG1, Data);
> +				} else if (mos7840_port->port_num == 2) {
> +					Data = (Data & 0xc7) | (clk_sel_val<<3);

Spaces around binary operator throughout.

> +					status =
> +						mos7840_set_reg_sync(port, CLOCK_SELECT_REG1, Data);
> +				}
> +				if (status < 0) {
> +					dev_dbg(&port->dev, "setting Clk_Select_Reg failed\n");
> +					return -1;
> +				}
> +			} else if (mos7840_port->port_num <= 4) {
> +				status = mos7840_get_reg_sync(port, CLOCK_SELECT_REG2, &Data);
> +				if (status < 0) {
> +					dev_dbg(&port->dev, "reading Clk_Select_Reg failed in set_serial_baud\n");
> +					return -1;
> +				}
> +				if (mos7840_port->port_num == 3) {
> +					Data = (Data & 0xf8) | clk_sel_val;
> +					status =
> +						mos7840_set_reg_sync(port, CLOCK_SELECT_REG2, Data);
> +				} else if (mos7840_port->port_num == 4) {
> +					Data = (Data & 0xc7) | (clk_sel_val<<3);
> +					status =
> +						mos7840_set_reg_sync(port, CLOCK_SELECT_REG2, Data);
> +				}
> +				if (status < 0) {
> +					dev_dbg(&port->dev, "setting Clk_Select_Reg failed\n");
> +					return -1;
> +				}
> +			}

The above needs to be cleaned up and abstracted better too. It's hardly
readable currently and the patterns looks too similar to be repeated
like this (i.e. a helper function may be the right choice).

> +		} else {
> +		// clk_sel_val is not DUMMY value -> Write the corresponding value to SP_Reg
> +
> +			/* First, needs to write default value to
> +			 * Clk_Select_Reg1(offset 0x13) & Clk_Select_Reg2(offset 0x14)
> +			 * Clk_Select_Reg1 for port1,2		Clk_Select_Reg2 for port3,4
> +			 */
> +			if (mos7840_port->port_num <= 2) {
> +				status = mos7840_get_reg_sync(port, CLOCK_SELECT_REG1, &Data);
> +				if (status < 0) {
> +					dev_dbg(&port->dev, "reading Clk_Select_Reg failed in set_serial_baud\n");
> +					return -1;
> +				}
> +				if (mos7840_port->port_num == 1) {
> +					Data = (Data & 0xf8) | 0x00;
> +					status =
> +						mos7840_set_reg_sync(port, CLOCK_SELECT_REG1, Data);
> +				} else if (mos7840_port->port_num == 2) {
> +					Data = (Data & 0xc7) | (0x00<<3);
> +					status =
> +						mos7840_set_reg_sync(port, CLOCK_SELECT_REG1, Data);
> +				}
> +				if (status < 0) {
> +					dev_dbg(&port->dev, "setting Clk_Select_Reg failed\n");
> +					return -1;
> +				}
> +			} else if (mos7840_port->port_num <= 4) {
> +				status = mos7840_get_reg_sync(port, CLOCK_SELECT_REG2, &Data);
> +				if (status < 0) {
> +					dev_dbg(&port->dev, "reading Clk_Select_Reg failed in set_serial_baud\n");
> +					return -1;
> +				}
> +				if (mos7840_port->port_num == 3) {
> +					Data = (Data & 0xf8) | 0x00;
> +					status =
> +						mos7840_set_reg_sync(port, CLOCK_SELECT_REG2, Data);
> +				} else if (mos7840_port->port_num == 4) {
> +					Data = (Data & 0xc7) | (0x00<<3);
> +					status =
> +						mos7840_set_reg_sync(port, CLOCK_SELECT_REG2, Data);
> +				}
> +				if (status < 0) {
> +					dev_dbg(&port->dev, "setting Clk_Select_Reg failed\n");
> +					return -1;
> +				}
> +			}

This is the same code as above, just writing the default value. This
obviously needs to be refactored.

> +			// select clock source by writing clk_sel_val to SPx_Reg
> +			status = mos7840_get_reg_sync(port, mos7840_port->SpRegOffset,
> +									 &Data);
> +			if (status < 0) {
> +				dev_dbg(&port->dev, "reading spreg failed in set_serial_baud\n");
> +				return -1;
> +			}
> +			Data = (Data & 0x8f) | clk_sel_val;
> +			status = mos7840_set_reg_sync(port, mos7840_port->SpRegOffset,
> +									Data);
> +			if (status < 0) {
> +				dev_dbg(&port->dev, "Writing spreg failed in set_serial_baud\n");
> +				return -1;
> +			}
> +		}

Johan

Re: [PATCH 0/3] drivers: usb: serial: mos7840: Improved baud rate support

[Johan Hovold]

On Thu, Oct 24, 2024 at 06:08:59PM +0800, Tony Chung wrote:
> This patch set has made some improvements for baud rate setup:
> 1. Optimized register configurations for common baud rates.
> 2. Added clock select registers to choose between 96M, 30M, or external clock sources,
>    allowing baud rate setup to 6M, matching the hardware's maximum data rate.
> 
> Tony Chung (3):
>   drivers: usb: serial: mos7840: Add defines for clock select register
>     offset
>   drivers: usb: serial: mos7840: added optimized register setups for
>     common baudrates.
>   driver: usb: serial: mos7840: add more baudrate options

Thanks for these. As for the clean up series, use the established
patch prefix here too:

	USB: serial: mos7840: ...

Johan