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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting skakit@xxxxxxxxxxxxxx (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@xxxxxxxxxxxxxx>
> > 
> > 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! :)




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux