[PATCH] tty: serial: qcom_geni_serial: Add 51.2MHz frequency support

[PATCH] tty: serial: qcom_geni_serial: Add 51.2MHz frequency support

[satya priya]

To support BT use case over UART at baud rate of 3.2 Mbps,
we need SE clocks to run at 51.2MHz frequency. Previously this
frequency was not available in clk src, so, we were requesting
for 102.4 MHz and dividing it internally by 2 to get 51.2MHz.

As now 51.2MHz frequency is made available in clk src,
adding this frequency to UART frequency table.

We will save significant amount of power, if 51.2 is used
because it belongs to LowSVS range whereas 102.4 fall into
Nominal category.

Signed-off-by: satya priya <skakit@codeaurora.org>
---

Note: This depend on clk patch https://patchwork.kernel.org/patch/11554073/

 drivers/tty/serial/qcom_geni_serial.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 6119090..168e1c0 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -141,9 +141,10 @@ static void qcom_geni_serial_stop_rx(struct uart_port *uport);
 static void qcom_geni_serial_handle_rx(struct uart_port *uport, bool drop);
 
 static const unsigned long root_freq[] = {7372800, 14745600, 19200000, 29491200,
-					32000000, 48000000, 64000000, 80000000,
-					96000000, 100000000, 102400000,
-					112000000, 120000000, 128000000};
+					32000000, 48000000, 51200000, 64000000,
+					80000000, 96000000, 100000000,
+					102400000, 112000000, 120000000,
+					128000000};
 
 #define to_dev_port(ptr, member) \
 		container_of(ptr, struct qcom_geni_serial_port, member)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] tty: serial: qcom_geni_serial: Add 51.2MHz frequency support

[Stephen Boyd]

Quoting satya priya (2020-05-29 03:14:42)
> To support BT use case over UART at baud rate of 3.2 Mbps,
> we need SE clocks to run at 51.2MHz frequency. Previously this
> frequency was not available in clk src, so, we were requesting
> for 102.4 MHz and dividing it internally by 2 to get 51.2MHz.
> 
> As now 51.2MHz frequency is made available in clk src,
> adding this frequency to UART frequency table.
> 
> We will save significant amount of power, if 51.2 is used
> because it belongs to LowSVS range whereas 102.4 fall into
> Nominal category.
> 
> Signed-off-by: satya priya <skakit@codeaurora.org>

Great commit text! Maybe point to the commit that adds it to the
frequency table in the gcc clk driver instead of the patchwork link.

> ---
> 
> Note: This depend on clk patch https://patchwork.kernel.org/patch/11554073/
> 
>  drivers/tty/serial/qcom_geni_serial.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 6119090..168e1c0 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -141,9 +141,10 @@ static void qcom_geni_serial_stop_rx(struct uart_port *uport);
>  static void qcom_geni_serial_handle_rx(struct uart_port *uport, bool drop);
>  
>  static const unsigned long root_freq[] = {7372800, 14745600, 19200000, 29491200,
> -                                       32000000, 48000000, 64000000, 80000000,
> -                                       96000000, 100000000, 102400000,
> -                                       112000000, 120000000, 128000000};
> +                                       32000000, 48000000, 51200000, 64000000,
> +                                       80000000, 96000000, 100000000,
> +                                       102400000, 112000000, 120000000,
> +                                       128000000};

Will this break sdm845? That clk frequency table hasn't been updated to
add 51.2 MHz.

Furthermore, it would be nice to get rid of this table and use
clk_round_rate() to find a frequency that will work with the requested
baud rate. Can we do that instead? That would make it work regardless of
what the clk driver supports for the particular SoC. Presumably we can
just call clk_round_rate() and then make sure it is evenly divisible by
the requested rate and then it will be mostly the same as before.

Or if we need to we can keep multiplying the rate 10 or 20 times and
test with clk_round_rate() each time and then give up if we don't find a
frequency that will work. The divider value looks like it is 12 bits
wide so there are 4095 possible dividers. If we need to loop through all
possible dividers then it may make sense to register a clk in this
driver and have it call divider_round_rate() to find the closest rate to
the desired rate. That would avoid reinventing a bunch of code that we
already have to implement clk dividers.

And one more thing, I see that this driver doesn't use DFS. Instead it
relies on the clk_set_rate() call to change the qup clk frequency. We
could support DFS by adding a driver specific member to struct
clk_rate_request that can be used to communicate back extra info to the
child clk. The idea is that the DFS clk (the qup uart one) can round the
rate and jam in the DFS index that corresponds to the rate into the new
member. Then the clk implemented in this serial driver can stash away
that index into some table that maps frequency of parent to DFS index
and then look up the DFS index during clk_set_rate() based on the parent
rate the clk_op is called with to program the DFS value in the uart
registers in addition to the divider.

---8<---
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 6119090ce045..7d147be997e5 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -140,11 +140,6 @@ static unsigned int qcom_geni_serial_tx_empty(struct uart_port *port);
 static void qcom_geni_serial_stop_rx(struct uart_port *uport);
 static void qcom_geni_serial_handle_rx(struct uart_port *uport, bool drop);
 
-static const unsigned long root_freq[] = {7372800, 14745600, 19200000, 29491200,
-					32000000, 48000000, 64000000, 80000000,
-					96000000, 100000000, 102400000,
-					112000000, 120000000, 128000000};
-
 #define to_dev_port(ptr, member) \
 		container_of(ptr, struct qcom_geni_serial_port, member)
 
@@ -900,30 +895,22 @@ static int qcom_geni_serial_startup(struct uart_port *uport)
 	return 0;
 }
 
-static unsigned long get_clk_cfg(unsigned long clk_freq)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(root_freq); i++) {
-		if (!(root_freq[i] % clk_freq))
-			return root_freq[i];
-	}
-	return 0;
-}
-
-static unsigned long get_clk_div_rate(unsigned int baud,
+static unsigned long get_clk_div_rate(const struct geni_se *se,
+			unsigned int baud,
 			unsigned int sampling_rate, unsigned int *clk_div)
 {
 	unsigned long ser_clk;
 	unsigned long desired_clk;
+	long actual_clk;
 
 	desired_clk = baud * sampling_rate;
-	ser_clk = get_clk_cfg(desired_clk);
-	if (!ser_clk) {
+	actual_clk = clk_round_rate(se->clk, desired_clk);
+	if (actual_clk % desired_clk != 0) {
 		pr_err("%s: Can't find matching DFS entry for baud %d\n",
 								__func__, baud);
-		return ser_clk;
+		return 0;
 	}
+	ser_clk = actual_clk;
 
 	*clk_div = ser_clk / desired_clk;
 	return ser_clk;
@@ -956,7 +943,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	if (GENI_SE_VERSION_MAJOR(ver) >= 2 && GENI_SE_VERSION_MINOR(ver) >= 5)
 		sampling_rate /= 2;
 
-	clk_rate = get_clk_div_rate(baud, sampling_rate, &clk_div);
+	clk_rate = get_clk_div_rate(&port->se, baud, sampling_rate, &clk_div);
 	if (!clk_rate)
 		goto out_restart_rx;

Re: [PATCH] tty: serial: qcom_geni_serial: Add 51.2MHz frequency support

[Stephen Boyd]

Quoting skakit@codeaurora.org (2020-08-11 22:44:22)
> Hi Stephen,
> 
> On 2020-05-31 00:39, Stephen Boyd wrote:
> > Quoting satya priya (2020-05-29 03:14:42)
> >> To support BT use case over UART at baud rate of 3.2 Mbps,
> >> we need SE clocks to run at 51.2MHz frequency. Previously this
> >> frequency was not available in clk src, so, we were requesting
> >> for 102.4 MHz and dividing it internally by 2 to get 51.2MHz.
> >> 
> >> As now 51.2MHz frequency is made available in clk src,
> >> adding this frequency to UART frequency table.
> >> 
> >> We will save significant amount of power, if 51.2 is used
> >> because it belongs to LowSVS range whereas 102.4 fall into
> >> Nominal category.
> >> 
> >> Signed-off-by: satya priya <skakit@codeaurora.org>
> > 
> > Great commit text! Maybe point to the commit that adds it to the
> > frequency table in the gcc clk driver instead of the patchwork link.
> > 
> >> ---
> >> 
> >> Note: This depend on clk patch 
> >> https://patchwork.kernel.org/patch/11554073/
> >> 
> >>  drivers/tty/serial/qcom_geni_serial.c | 7 ++++---
> >>  1 file changed, 4 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/tty/serial/qcom_geni_serial.c 
> >> b/drivers/tty/serial/qcom_geni_serial.c
> >> index 6119090..168e1c0 100644
> >> --- a/drivers/tty/serial/qcom_geni_serial.c
> >> +++ b/drivers/tty/serial/qcom_geni_serial.c
> >> @@ -141,9 +141,10 @@ static void qcom_geni_serial_stop_rx(struct 
> >> uart_port *uport);
> >>  static void qcom_geni_serial_handle_rx(struct uart_port *uport, bool 
> >> drop);
> >> 
> >>  static const unsigned long root_freq[] = {7372800, 14745600, 
> >> 19200000, 29491200,
> >> -                                       32000000, 48000000, 64000000, 
> >> 80000000,
> >> -                                       96000000, 100000000, 
> >> 102400000,
> >> -                                       112000000, 120000000, 
> >> 128000000};
> >> +                                       32000000, 48000000, 51200000, 
> >> 64000000,
> >> +                                       80000000, 96000000, 100000000,
> >> +                                       102400000, 112000000, 
> >> 120000000,
> >> +                                       128000000};
> > 
> > Will this break sdm845? That clk frequency table hasn't been updated to
> > add 51.2 MHz.
> 
> No, as the sampling rate in sdm845 is 32, we will not be requesting 51.2 
> over there.

Ok.

> 
> > 
> > Furthermore, it would be nice to get rid of this table and use
> > clk_round_rate() to find a frequency that will work with the requested
> > baud rate. Can we do that instead? That would make it work regardless 
> > of
> > what the clk driver supports for the particular SoC. Presumably we can
> > just call clk_round_rate() and then make sure it is evenly divisible by
> > the requested rate and then it will be mostly the same as before.
> 
> Okay.
> 
> > 
> > Or if we need to we can keep multiplying the rate 10 or 20 times and
> > test with clk_round_rate() each time and then give up if we don't find 
> > a
> > frequency that will work. The divider value looks like it is 12 bits
> > wide so there are 4095 possible dividers. If we need to loop through 
> > all
> > possible dividers then it may make sense to register a clk in this
> > driver and have it call divider_round_rate() to find the closest rate 
> > to
> > the desired rate. That would avoid reinventing a bunch of code that we
> > already have to implement clk dividers.
> > 
> 
> If i understand correctly, clk_round_rate gives the nearest possible 
> value to the desired clk rate, but i am not very clear about 
> divider_round_rate API, is it an alternate method to get nearest clk 
> rate? Also it has few parameters like prate, clk_hw etc which we are not 
> sure of. For now we are planning to post the change for clk_round_rate 
> API.

The clk_round_rate() API is to tell a consumer what rate they'll get on
a clk if they call clk_set_rate() with the same arguments that they pass
to clk_round_rate(). If this driver implemented a clk provider for
itself then it could hook into the clk_set_rate() and clk_round_rate()
logic and turn whatever 'desired_clk' it is that's passed in into the
rate rounding code that is implemented here.

The idea is that instead of hard coding the frequency table in this
driver, this driver implements a clk_hw for itself. Then that clk_hw
clk_ops implementation asks the parent clk (i.e. the one this driver is
currently clk_get()ing during probe) what frequency it can support if it
uses 'desired_clk * 2' or 'desired_clk * 3', up to how ever many divider
possibilities this uart hardware has. Then when the clk_op
implementation figures out the best rate that the gcc clk can provide it
tells the CCF that the parent rate will be prate = desired_clk * N and
then uses N as the divider that this driver controls.

This way everything is generic and we don't have to copy/paste tables
from one place to another. Does it make sense?

> 
> > And one more thing, I see that this driver doesn't use DFS. Instead it
> > relies on the clk_set_rate() call to change the qup clk frequency. We
> > could support DFS by adding a driver specific member to struct
> > clk_rate_request that can be used to communicate back extra info to the
> > child clk. The idea is that the DFS clk (the qup uart one) can round 
> > the
> > rate and jam in the DFS index that corresponds to the rate into the new
> > member. Then the clk implemented in this serial driver can stash away
> > that index into some table that maps frequency of parent to DFS index
> > and then look up the DFS index during clk_set_rate() based on the 
> > parent
> > rate the clk_op is called with to program the DFS value in the uart
> > registers in addition to the divider.
> > 
> okay will look into this.
> 
> > ---8<---
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c
> > b/drivers/tty/serial/qcom_geni_serial.c
> > index 6119090ce045..7d147be997e5 100644

Whoa this looks familiar! :)

Re: [PATCH] tty: serial: qcom_geni_serial: Add 51.2MHz frequency support

[skakit]

Hi Stephen,

On 2020-05-31 00:39, Stephen Boyd wrote:
> Quoting satya priya (2020-05-29 03:14:42)
>> To support BT use case over UART at baud rate of 3.2 Mbps,
>> we need SE clocks to run at 51.2MHz frequency. Previously this
>> frequency was not available in clk src, so, we were requesting
>> for 102.4 MHz and dividing it internally by 2 to get 51.2MHz.
>> 
>> As now 51.2MHz frequency is made available in clk src,
>> adding this frequency to UART frequency table.
>> 
>> We will save significant amount of power, if 51.2 is used
>> because it belongs to LowSVS range whereas 102.4 fall into
>> Nominal category.
>> 
>> Signed-off-by: satya priya <skakit@codeaurora.org>
> 
> Great commit text! Maybe point to the commit that adds it to the
> frequency table in the gcc clk driver instead of the patchwork link.
> 
>> ---
>> 
>> Note: This depend on clk patch 
>> https://patchwork.kernel.org/patch/11554073/
>> 
>>  drivers/tty/serial/qcom_geni_serial.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c 
>> b/drivers/tty/serial/qcom_geni_serial.c
>> index 6119090..168e1c0 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -141,9 +141,10 @@ static void qcom_geni_serial_stop_rx(struct 
>> uart_port *uport);
>>  static void qcom_geni_serial_handle_rx(struct uart_port *uport, bool 
>> drop);
>> 
>>  static const unsigned long root_freq[] = {7372800, 14745600, 
>> 19200000, 29491200,
>> -                                       32000000, 48000000, 64000000, 
>> 80000000,
>> -                                       96000000, 100000000, 
>> 102400000,
>> -                                       112000000, 120000000, 
>> 128000000};
>> +                                       32000000, 48000000, 51200000, 
>> 64000000,
>> +                                       80000000, 96000000, 100000000,
>> +                                       102400000, 112000000, 
>> 120000000,
>> +                                       128000000};
> 
> Will this break sdm845? That clk frequency table hasn't been updated to
> add 51.2 MHz.

No, as the sampling rate in sdm845 is 32, we will not be requesting 51.2 
over there.

> 
> Furthermore, it would be nice to get rid of this table and use
> clk_round_rate() to find a frequency that will work with the requested
> baud rate. Can we do that instead? That would make it work regardless 
> of
> what the clk driver supports for the particular SoC. Presumably we can
> just call clk_round_rate() and then make sure it is evenly divisible by
> the requested rate and then it will be mostly the same as before.

Okay.

> 
> Or if we need to we can keep multiplying the rate 10 or 20 times and
> test with clk_round_rate() each time and then give up if we don't find 
> a
> frequency that will work. The divider value looks like it is 12 bits
> wide so there are 4095 possible dividers. If we need to loop through 
> all
> possible dividers then it may make sense to register a clk in this
> driver and have it call divider_round_rate() to find the closest rate 
> to
> the desired rate. That would avoid reinventing a bunch of code that we
> already have to implement clk dividers.
> 

If i understand correctly, clk_round_rate gives the nearest possible 
value to the desired clk rate, but i am not very clear about 
divider_round_rate API, is it an alternate method to get nearest clk 
rate? Also it has few parameters like prate, clk_hw etc which we are not 
sure of. For now we are planning to post the change for clk_round_rate 
API.

> And one more thing, I see that this driver doesn't use DFS. Instead it
> relies on the clk_set_rate() call to change the qup clk frequency. We
> could support DFS by adding a driver specific member to struct
> clk_rate_request that can be used to communicate back extra info to the
> child clk. The idea is that the DFS clk (the qup uart one) can round 
> the
> rate and jam in the DFS index that corresponds to the rate into the new
> member. Then the clk implemented in this serial driver can stash away
> that index into some table that maps frequency of parent to DFS index
> and then look up the DFS index during clk_set_rate() based on the 
> parent
> rate the clk_op is called with to program the DFS value in the uart
> registers in addition to the divider.
> 
okay will look into this.

> ---8<---
> diff --git a/drivers/tty/serial/qcom_geni_serial.c
> b/drivers/tty/serial/qcom_geni_serial.c
> index 6119090ce045..7d147be997e5 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -140,11 +140,6 @@ static unsigned int
> qcom_geni_serial_tx_empty(struct uart_port *port);
>  static void qcom_geni_serial_stop_rx(struct uart_port *uport);
>  static void qcom_geni_serial_handle_rx(struct uart_port *uport, bool 
> drop);
> 
> -static const unsigned long root_freq[] = {7372800, 14745600,
> 19200000, 29491200,
> -					32000000, 48000000, 64000000, 80000000,
> -					96000000, 100000000, 102400000,
> -					112000000, 120000000, 128000000};
> -
>  #define to_dev_port(ptr, member) \
>  		container_of(ptr, struct qcom_geni_serial_port, member)
> 
> @@ -900,30 +895,22 @@ static int qcom_geni_serial_startup(struct
> uart_port *uport)
>  	return 0;
>  }
> 
> -static unsigned long get_clk_cfg(unsigned long clk_freq)
> -{
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(root_freq); i++) {
> -		if (!(root_freq[i] % clk_freq))
> -			return root_freq[i];
> -	}
> -	return 0;
> -}
> -
> -static unsigned long get_clk_div_rate(unsigned int baud,
> +static unsigned long get_clk_div_rate(const struct geni_se *se,
> +			unsigned int baud,
>  			unsigned int sampling_rate, unsigned int *clk_div)
>  {
>  	unsigned long ser_clk;
>  	unsigned long desired_clk;
> +	long actual_clk;
> 
>  	desired_clk = baud * sampling_rate;
> -	ser_clk = get_clk_cfg(desired_clk);
> -	if (!ser_clk) {
> +	actual_clk = clk_round_rate(se->clk, desired_clk);
> +	if (actual_clk % desired_clk != 0) {
>  		pr_err("%s: Can't find matching DFS entry for baud %d\n",
>  								__func__, baud);
> -		return ser_clk;
> +		return 0;
>  	}
> +	ser_clk = actual_clk;
> 
>  	*clk_div = ser_clk / desired_clk;
>  	return ser_clk;
> @@ -956,7 +943,7 @@ static void qcom_geni_serial_set_termios(struct
> uart_port *uport,
>  	if (GENI_SE_VERSION_MAJOR(ver) >= 2 && GENI_SE_VERSION_MINOR(ver) >= 
> 5)
>  		sampling_rate /= 2;
> 
> -	clk_rate = get_clk_div_rate(baud, sampling_rate, &clk_div);
> +	clk_rate = get_clk_div_rate(&port->se, baud, sampling_rate, 
> &clk_div);
>  	if (!clk_rate)
>  		goto out_restart_rx;

Thanks,
Satya Priya