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! :)
