| Message ID | 1584105134-13583-1-git-send-email-akashast@codeaurora.org |
|---|---|
| Headers | show |
| Series | Add interconnect support to QSPI and QUP drivers | expand |
On Fri, Mar 13, 2020 at 06:42:07PM +0530, Akash Asthana wrote: > Users can use devm version of of_icc_get() to benefit from automatic > resource release. > > Signed-off-by: Akash Asthana <akashast@codeaurora.org> > --- > drivers/interconnect/core.c | 25 +++++++++++++++++++++++++ > include/linux/interconnect.h | 7 +++++++ > 2 files changed, 32 insertions(+) Reviewed by: Matthias Kaehlcke <mka@chromium.org>
Hi Matthias, On 3/13/2020 10:12 PM, Matthias Kaehlcke wrote: > Hi Akash, > > On Fri, Mar 13, 2020 at 06:42:08PM +0530, Akash Asthana wrote: >> Add necessary macros and structure variables to support ICC BW >> voting from individual SE drivers. >> >> Signed-off-by: Akash Asthana <akashast@codeaurora.org> >> --- >> Changes in V2: >> - As per Bjorn's comment dropped enums for ICC paths, given the three >> paths individual members >> >> include/linux/qcom-geni-se.h | 28 ++++++++++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h >> index dd46494..eaae16e 100644 >> --- a/include/linux/qcom-geni-se.h >> +++ b/include/linux/qcom-geni-se.h >> @@ -6,6 +6,8 @@ >> #ifndef _LINUX_QCOM_GENI_SE >> #define _LINUX_QCOM_GENI_SE >> >> +#include <linux/interconnect.h> >> + >> /* Transfer mode supported by GENI Serial Engines */ >> enum geni_se_xfer_mode { >> GENI_SE_INVALID, >> @@ -33,6 +35,15 @@ struct clk; >> * @clk: Handle to the core serial engine clock >> * @num_clk_levels: Number of valid clock levels in clk_perf_tbl >> * @clk_perf_tbl: Table of clock frequency input to serial engine clock >> + * @icc_path_geni_to_core: ICC path handle for geni to core >> + * @icc_path_cpu_to_geni: ICC path handle for cpu to geni >> + * @icc_path_geni_to_ddr: ICC path handle for geni to ddr >> + * @avg_bw_core: Average bus bandwidth value for QUP core 2x clock >> + * @peak_bw_core: Peak bus bandwidth value for QUP core 2x clock >> + * @avg_bw_cpu: Average bus bandwidth value for CPU >> + * @peak_bw_cpu: Peak bus bandwidth value for CPU >> + * @avg_bw_ddr: Average bus bandwidth value for DDR >> + * @peak_bw_ddr: Peak bus bandwidth value for DDR >> */ >> struct geni_se { >> void __iomem *base; >> @@ -41,6 +52,15 @@ struct geni_se { >> struct clk *clk; >> unsigned int num_clk_levels; >> unsigned long *clk_perf_tbl; >> + struct icc_path *icc_path_geni_to_core; >> + struct icc_path *icc_path_cpu_to_geni; >> + struct icc_path *icc_path_geni_to_ddr; >> + unsigned int avg_bw_core; >> + unsigned int peak_bw_core; >> + unsigned int avg_bw_cpu; >> + unsigned int peak_bw_cpu; >> + unsigned int avg_bw_ddr; >> + unsigned int peak_bw_ddr; > Those are a lot of new individual struct members. How about clustering > them, e.g.: > > struct geni_icc_path { > struct icc_path *path; > unsigned int avg_bw; > unsigned int peak_bw; > }; I guess it would be better to add this structure ICC driver as you suggested@https://patchwork.kernel.org/patch/11436905/. > struct geni_iccs_paths { > struct geni_icc_path to_core; > struct geni_icc_path from_cpu; > struct geni_icc_path to_ddr; > }; > > And 'struct geni_se' just gets this entry: > > struct geni_icc_paths icc; > > or alternatively three 'struct geni_icc_path' entries. ok Thanks for reviewing. Regards Akash
On Tue, Mar 17, 2020 at 05:18:34PM +0530, Akash Asthana wrote: > Hi Matthias, > > On 3/14/2020 2:58 AM, Matthias Kaehlcke wrote: > > Hi Akash, > > > > On Fri, Mar 13, 2020 at 06:42:10PM +0530, Akash Asthana wrote: > > > Get the interconnect paths for Uart based Serial Engine device > > > and vote according to the baud rate requirement of the driver. > > > > > > Signed-off-by: Akash Asthana <akashast@codeaurora.org> > > > --- > > > Changes in V2: > > > - As per Bjorn's comment, removed se == NULL check from geni_serial_icc_get > > > - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure > > > - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting > > > path handle > > > - As per Matthias comment, added error handling for icc_set_bw call > > > > > > drivers/tty/serial/qcom_geni_serial.c | 69 +++++++++++++++++++++++++++++++++-- > > > 1 file changed, 65 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > > > index 272bae0..c8ad7e9 100644 > > > --- a/drivers/tty/serial/qcom_geni_serial.c > > > +++ b/drivers/tty/serial/qcom_geni_serial.c > > > > > > ... > > > > > > static int qcom_geni_serial_request_port(struct uart_port *uport) > > > { > > > struct platform_device *pdev = to_platform_device(uport->dev); > > > @@ -962,6 +975,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport, > > > struct qcom_geni_serial_port *port = to_dev_port(uport, uport); > > > unsigned long clk_rate; > > > u32 ver, sampling_rate; > > > + int ret; > > > qcom_geni_serial_stop_rx(uport); > > > /* baud rate */ > > > @@ -983,6 +997,18 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport, > > > ser_clk_cfg = SER_CLK_EN; > > > ser_clk_cfg |= clk_div << CLK_DIV_SHFT; > > > + /* > > > + * Put BW vote only on CPU path as driver supports FIFO mode only. > > > + * Assume peak_bw as twice of avg_bw. > > > + */ > > > + port->se.avg_bw_cpu = Bps_to_icc(baud); > > > + port->se.peak_bw_cpu = Bps_to_icc(2 * baud); > > > + ret = icc_set_bw(port->se.icc_path_cpu_to_geni, port->se.avg_bw_cpu, > > > + port->se.peak_bw_cpu); > > > + if (ret) > > > + dev_err(uport->dev, "%s: ICC BW voting failed for cpu\n", > > > + __func__); > > Should this return an error? The port might not operate properly if the ICC > > bandwidth couldn't be configured > > This is void function we can't return error from here. I guess it would be > somewhat okay if BW voting failed for CPU path but clk_set_rate failure is > more serious which is called from this function, I don't think it can be > move to somewhere else. ok, I missed that _set_termios() is void. > > > static const struct uart_ops qcom_geni_console_pops = { > > > @@ -1308,6 +1358,17 @@ static int qcom_geni_serial_probe(struct platform_device *pdev) > > > port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS; > > > port->tx_fifo_width = DEF_FIFO_WIDTH_BITS; > > > + ret = geni_serial_icc_get(&port->se); > > > + if (ret) > > > + return ret; > > > + /* Set the bus quota to a reasonable value */ > > > + port->se.avg_bw_core = console ? Bps_to_icc(1000) : > > > + Bps_to_icc(CORE_2X_50_MHZ); > > Why different settings for console vs. non-console? > > QUP FW runs on core clock. To support higher throughput we want FW to run at > higher speed. > > Since Console operate at 115200bps and BT operate at 3.2Mbps baud. We are > voting higher on core for BT usecase. > > These value are recommended from HW team. IIUC none of the values you mention are set in stone. 115200bps seems to be a 'standard' value for the serial console, but it could be a different baudrate. I guess you are referring to Qualcomm Bluetooth controllers, which are only one of many things that could be connected to the port. And what happens when a QCA BT controller is connected to a non-geni/QCA port, which doesn't know about its 'requirements'? The answer is that both the BT controller and the serial console configure the baudrate they need, hence using different values in _probe() is pointless. Unsurprisingly one of the first things the QCA BT driver does is to configure the baudrate. It typically starts with a lower ('init') speed, and then switches to the higher ('operational') baudrate: https://elixir.bootlin.com/linux/v5.5.8/source/drivers/bluetooth/hci_qca.c#L1256
On Tue, Mar 17, 2020 at 5:13 AM Akash Asthana <akashast@codeaurora.org> wrote: > > Hi Matthias, > > On 3/14/2020 6:28 AM, Matthias Kaehlcke wrote: > > Hi, > > > > On Fri, Mar 13, 2020 at 06:42:13PM +0530, Akash Asthana wrote: > >> Get the interconnect paths for QSPI device and vote according to the > >> current bus speed of the driver. > >> > >> Signed-off-by: Akash Asthana <akashast@codeaurora.org> > >> --- > >> - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting > >> path handle > >> - As per Matthias comment, added error handling for icc_set_bw call > >> > >> drivers/spi/spi-qcom-qspi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 45 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c > >> index 3c4f83b..ad48f43 100644 > >> --- a/drivers/spi/spi-qcom-qspi.c > >> +++ b/drivers/spi/spi-qcom-qspi.c > >> @@ -2,6 +2,7 @@ > >> // Copyright (c) 2017-2018, The Linux foundation. All rights reserved. > >> > >> #include <linux/clk.h> > >> +#include <linux/interconnect.h> > >> #include <linux/interrupt.h> > >> #include <linux/io.h> > >> #include <linux/module.h> > >> @@ -139,7 +140,10 @@ struct qcom_qspi { > >> struct device *dev; > >> struct clk_bulk_data *clks; > >> struct qspi_xfer xfer; > >> - /* Lock to protect xfer and IRQ accessed registers */ > >> + struct icc_path *icc_path_cpu_to_qspi; > >> + unsigned int avg_bw_cpu; > >> + unsigned int peak_bw_cpu; > > This triplet is a recurring pattern, and is probably not limited to geni SE/QSPI. > > On https://patchwork.kernel.org/patch/11436889/#23221925 I suggested the creation > > of a geni SE specific struct, however adding a generic convenience struct to > > 'linux/interconnect.h' might be the better solution: > > > > struct icc_client { > > struct icc_path *path; > > unsigned int avg_bw; > > unsigned int peak_bw; > > }; > > > > I'm sure there are better names for it, but this would be the idea. > > Yeah, I think introducing this to ICC header would be better solution. +Georgi I'm not as convinced this structure is generally useful and belongs in the interconnect core. The thing that strikes me as weird with putting it in the core is now we're saving these values both inside and outside the interconnect core. In the GENI case here, we only really need them to undo the 0 votes we cast during suspend. If "vote for 0 in suspend and whatever it was before at resume" is a recurring theme, maybe the core should give us path_disable() and path_enable() calls instead. I'm thinking out loud, maybe Georgi has some thoughts. Akash, for now if you want to avoid wading into a larger discussion maybe just refactor to a common structure local to GENI. -Evan
Hi Matthias, On 3/18/2020 12:38 AM, Matthias Kaehlcke wrote: > On Tue, Mar 17, 2020 at 05:18:34PM +0530, Akash Asthana wrote: >> Hi Matthias, >> >> On 3/14/2020 2:58 AM, Matthias Kaehlcke wrote: >>> Hi Akash, >>> >>> On Fri, Mar 13, 2020 at 06:42:10PM +0530, Akash Asthana wrote: >>>> Get the interconnect paths for Uart based Serial Engine device >>>> and vote according to the baud rate requirement of the driver. >>>> >>>> Signed-off-by: Akash Asthana <akashast@codeaurora.org> >>>> --- >>>> Changes in V2: >>>> - As per Bjorn's comment, removed se == NULL check from geni_serial_icc_get >>>> - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure >>>> - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting >>>> path handle >>>> - As per Matthias comment, added error handling for icc_set_bw call >>>> >>>> drivers/tty/serial/qcom_geni_serial.c | 69 +++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 65 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c >>>> index 272bae0..c8ad7e9 100644 >>>> --- a/drivers/tty/serial/qcom_geni_serial.c >>>> +++ b/drivers/tty/serial/qcom_geni_serial.c >>>> >>>> ... >>>> >>>> static int qcom_geni_serial_request_port(struct uart_port *uport) >>>> { >>>> struct platform_device *pdev = to_platform_device(uport->dev); >>>> @@ -962,6 +975,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport, >>>> struct qcom_geni_serial_port *port = to_dev_port(uport, uport); >>>> unsigned long clk_rate; >>>> u32 ver, sampling_rate; >>>> + int ret; >>>> qcom_geni_serial_stop_rx(uport); >>>> /* baud rate */ >>>> @@ -983,6 +997,18 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport, >>>> ser_clk_cfg = SER_CLK_EN; >>>> ser_clk_cfg |= clk_div << CLK_DIV_SHFT; >>>> + /* >>>> + * Put BW vote only on CPU path as driver supports FIFO mode only. >>>> + * Assume peak_bw as twice of avg_bw. >>>> + */ >>>> + port->se.avg_bw_cpu = Bps_to_icc(baud); >>>> + port->se.peak_bw_cpu = Bps_to_icc(2 * baud); >>>> + ret = icc_set_bw(port->se.icc_path_cpu_to_geni, port->se.avg_bw_cpu, >>>> + port->se.peak_bw_cpu); >>>> + if (ret) >>>> + dev_err(uport->dev, "%s: ICC BW voting failed for cpu\n", >>>> + __func__); >>> Should this return an error? The port might not operate properly if the ICC >>> bandwidth couldn't be configured >> This is void function we can't return error from here. I guess it would be >> somewhat okay if BW voting failed for CPU path but clk_set_rate failure is >> more serious which is called from this function, I don't think it can be >> move to somewhere else. > ok, I missed that _set_termios() is void. > >>>> static const struct uart_ops qcom_geni_console_pops = { >>>> @@ -1308,6 +1358,17 @@ static int qcom_geni_serial_probe(struct platform_device *pdev) >>>> port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS; >>>> port->tx_fifo_width = DEF_FIFO_WIDTH_BITS; >>>> + ret = geni_serial_icc_get(&port->se); >>>> + if (ret) >>>> + return ret; >>>> + /* Set the bus quota to a reasonable value */ >>>> + port->se.avg_bw_core = console ? Bps_to_icc(1000) : >>>> + Bps_to_icc(CORE_2X_50_MHZ); >>> Why different settings for console vs. non-console? >> QUP FW runs on core clock. To support higher throughput we want FW to run at >> higher speed. >> >> Since Console operate at 115200bps and BT operate at 3.2Mbps baud. We are >> voting higher on core for BT usecase. >> >> These value are recommended from HW team. > IIUC none of the values you mention are set in stone. 115200bps seems to be a > 'standard' value for the serial console, but it could be a different baudrate. > I guess you are referring to Qualcomm Bluetooth controllers, which are only one > of many things that could be connected to the port. And what happens when a > QCA BT controller is connected to a non-geni/QCA port, which doesn't know about > its 'requirements'? The answer is that both the BT controller and the serial > console configure the baudrate they need, hence using different values in > _probe() is pointless. Are you refering other UART drivers(not based on geni HW) as non-geni/QCA port? We are not scaling core BW request based on real time need like we are doing for other paths(CPU/DDR) instead we are using some fail proof value because, FW runs on core clock and core behaves a bit different than other NOCs. We don't have any functional relation which maps actual throughput requirement to core frequency need. In the past we faced few latency issues because of core slowness (Although it was running much higher than actual throughput requirement). To avoid such scenario we are using recommend value from HW team. These fix value can support SE drivers operating at their max possible speed(4Mbps in case of non-console). I agree that 115200bps seems to be a 'standard' value for the serial console, but it could be a different baudrate. We are voting 1000 in case of console because it has low power mode use-case in android, where voting CORE_2X_50_MHZ can be reported as a power issue. Actually we wanted to vote 960 for console but that is not possible with current ICC design where the minimum value is 1000bps. So any way core is running at 50 MHz as 1000 crosses the threshold for 19.2 MHz (960) only with console. regards, Akash > Unsurprisingly one of the first things the QCA BT driver does is to configure > the baudrate. It typically starts with a lower ('init') speed, and then switches > to the higher ('operational') baudrate: > > https://elixir.bootlin.com/linux/v5.5.8/source/drivers/bluetooth/hci_qca.c#L1256
Hi Evan, >> IIUC, you meant to say struct icc_req(inside icc_path) will be saving >> avg_bw and peak_bw so no need to save it outside icc_path? > Correct, it seems silly to store the same set of values twice in the > framework, but with different semantics about who's watching it. > -Evan Thanks for clarification! Yeah make sense not to introduce the structure in ICC framework Regards, Akash
On Fri, Mar 20, 2020 at 4:03 AM Akash Asthana <akashast@codeaurora.org> wrote: > > Hi Evan, > > +/* Core 2X clock frequency to BCM threshold mapping */ > +#define CORE_2X_19_2_MHZ 960 > +#define CORE_2X_50_MHZ 2500 > +#define CORE_2X_100_MHZ 5000 > +#define CORE_2X_150_MHZ 7500 > +#define CORE_2X_200_MHZ 10000 > +#define CORE_2X_236_MHZ 16383 > > These are all just 50 * clock_rate. Can you instead specify that one > define of CLK_TO_BW_RATIO 50, and then use clk_get_rate() to get the > input clock frequency. That way, if these end up getting clocked at a > different rate, the bandwidth also scales appropriately. Also, can you > enumerate why 50 is an appropriate ratio? > -Evan > > -Evan > > Clock rate for Core 2X is controlled by BW voting only, we don't set clock rate for core 2X clock either by DFS or calling clk_set_rate API like we do for SE clocks from individual driver. > > In DT node it's not mentioned as clock. > > As discussed in patch@ https://patchwork.kernel.org/patch/11436897/ We are not scaling Core 2X clock based on dynamic need of driver instead we are putting recommended value from HW team for each driver. Oh I get it. This is pretty opaque, since this table is saying "here are the bandwidth values that happen to work out to a Core2X clock rate of N". But it's not obvious why setting the Core2X clock rate to N is desirable or appropriate. The answer seems to be hardware guys told us these thresholds work well in practice. And if I'm reading into it more, probably they're saying these bandwidths are too low to be worth dynamically managing beyond on/off. At the very least we should explain some of this in the comment above these defines. Something like: /* Define bandwidth thresholds that cause the underlying Core 2X interconnect clock to run at the named frequency. These baseline values are recommended by the hardware team, and are not dynamically scaled with GENI bandwidth beyond basic on/off. */ -Evan