diff mbox series

[V3,6/8] tty: serial: qcom_geni_serial: Add interconnect support

Message ID 1585652976-17481-7-git-send-email-akashast@codeaurora.org
State New
Headers show
Series Add interconnect support to QSPI and QUP drivers | expand

Commit Message

Akash Asthana March 31, 2020, 11:09 a.m. UTC
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

Changes in V3:
 - As per Matthias comment, use common library APIs defined in geni-se
   driver for ICC functionality.

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

Comments

Akash Asthana April 7, 2020, 9:19 a.m. UTC | #1
Hi Matthias,

On 4/1/2020 1:09 AM, Matthias Kaehlcke wrote:
> Hi Akash,
>
> On Tue, Mar 31, 2020 at 04:39:34PM +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
>>
>> Changes in V3:
>>   - As per Matthias comment, use common library APIs defined in geni-se
>>     driver for ICC functionality.
>>
>>   drivers/tty/serial/qcom_geni_serial.c | 28 +++++++++++++++++++++++++---
>>   1 file changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>> index 8c5d97c..2befe72 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -965,6 +965,14 @@ 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;
>>   
>> +	/*
>> +	 * Bump up BW vote on CPU path as driver supports FIFO mode only.
>> +	 * Assume peak_bw as twice of avg_bw.
>> +	 */
>> +	port->se.from_cpu.avg_bw = Bps_to_icc(baud);
>> +	port->se.from_cpu.peak_bw = Bps_to_icc(2 * baud);
>> +	geni_icc_vote_on(&port->se);
>> +
>>   	/* parity */
>>   	tx_trans_cfg = readl(uport->membase + SE_UART_TX_TRANS_CFG);
>>   	tx_parity_cfg = readl(uport->membase + SE_UART_TX_PARITY_CFG);
>> @@ -1202,11 +1210,14 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
>>   	if (old_state == UART_PM_STATE_UNDEFINED)
>>   		old_state = UART_PM_STATE_OFF;
>>   
>> -	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
>> +	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) {
>> +		geni_icc_vote_on(&port->se);
>>   		geni_se_resources_on(&port->se);
>> -	else if (new_state == UART_PM_STATE_OFF &&
>> -			old_state == UART_PM_STATE_ON)
>> +	} else if (new_state == UART_PM_STATE_OFF &&
>> +			old_state == UART_PM_STATE_ON) {
>>   		geni_se_resources_off(&port->se);
>> +		geni_icc_vote_off(&port->se);
>> +	}
>>   }
>>   
>>   static const struct uart_ops qcom_geni_console_pops = {
>> @@ -1304,6 +1315,17 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>>   			return -ENOMEM;
>>   	}
>>   
>> +	ret = geni_icc_get(&port->se, "qup-core", "qup-config", NULL);
>> +	if (ret)
>> +		return ret;
>> +	/* Set the bus quota to a reasonable value */
>> +	port->se.to_core.avg_bw = console ? GENI_DEFAULT_BW :
>> +		Bps_to_icc(CORE_2X_50_MHZ);
>> +	port->se.to_core.peak_bw = console ? GENI_DEFAULT_BW :
>> +		Bps_to_icc(CORE_2X_100_MHZ);
> I'm still unconvinced about the setting of the core bandwidth based on
> whether the port is used as console or not. It could possibly break
> consoles working at speeds > 115kbs and reserve more bandwidth than
> necessary for ports with 'slow' devices.
>
> Why not scale the core bandwidth dynamically? You said earlier that there
> is no clear/linear translation of port speed to bandwidth, but you could
> use the same logic that is implicitly used here:
>
> 	if (baudrate <= 115200) {
> 		avg_bw = GENI_DEFAULT_BW;
> 		peak_bw = GENI_DEFAULT_BW;
> 	} else {
> 		avg_bw = Bps_to_icc(CORE_2X_50_MHZ);
> 		peak_bw = Bps_to_icc(CORE_2X_100_MHZ);
> 	}
>
> This would be more robust, power efficient and future readers of the
> code don't have to wonder "why is the console special?" when our
> discussions on this will be long forgotten.

Okay, I will add this piece of code in set_termios call of the driver 
because I don't have baudrate information during probe. It covers the 
console case mentioned in probe function.

Regards,

Akash
diff mbox series

Patch

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 8c5d97c..2befe72 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -965,6 +965,14 @@  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;
 
+	/*
+	 * Bump up BW vote on CPU path as driver supports FIFO mode only.
+	 * Assume peak_bw as twice of avg_bw.
+	 */
+	port->se.from_cpu.avg_bw = Bps_to_icc(baud);
+	port->se.from_cpu.peak_bw = Bps_to_icc(2 * baud);
+	geni_icc_vote_on(&port->se);
+
 	/* parity */
 	tx_trans_cfg = readl(uport->membase + SE_UART_TX_TRANS_CFG);
 	tx_parity_cfg = readl(uport->membase + SE_UART_TX_PARITY_CFG);
@@ -1202,11 +1210,14 @@  static void qcom_geni_serial_pm(struct uart_port *uport,
 	if (old_state == UART_PM_STATE_UNDEFINED)
 		old_state = UART_PM_STATE_OFF;
 
-	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
+	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) {
+		geni_icc_vote_on(&port->se);
 		geni_se_resources_on(&port->se);
-	else if (new_state == UART_PM_STATE_OFF &&
-			old_state == UART_PM_STATE_ON)
+	} else if (new_state == UART_PM_STATE_OFF &&
+			old_state == UART_PM_STATE_ON) {
 		geni_se_resources_off(&port->se);
+		geni_icc_vote_off(&port->se);
+	}
 }
 
 static const struct uart_ops qcom_geni_console_pops = {
@@ -1304,6 +1315,17 @@  static int qcom_geni_serial_probe(struct platform_device *pdev)
 			return -ENOMEM;
 	}
 
+	ret = geni_icc_get(&port->se, "qup-core", "qup-config", NULL);
+	if (ret)
+		return ret;
+	/* Set the bus quota to a reasonable value */
+	port->se.to_core.avg_bw = console ? GENI_DEFAULT_BW :
+		Bps_to_icc(CORE_2X_50_MHZ);
+	port->se.to_core.peak_bw = console ? GENI_DEFAULT_BW :
+		Bps_to_icc(CORE_2X_100_MHZ);
+	port->se.from_cpu.avg_bw = GENI_DEFAULT_BW;
+	port->se.from_cpu.peak_bw = GENI_DEFAULT_BW;
+
 	port->name = devm_kasprintf(uport->dev, GFP_KERNEL,
 			"qcom_geni_serial_%s%d",
 			uart_console(uport) ? "console" : "uart", uport->line);