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

[skakit@codeaurora.org]

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