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

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

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

Matthias Kaehlcke March 31, 2020, 7:39 p.m. UTC | #1
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.
Akash Asthana April 7, 2020, 9:19 a.m. UTC | #2
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
Akash Asthana April 7, 2020, 9:40 a.m. UTC | #3
Hi Matthias,


>>>     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;

I will make peak_bw = 2 * DEFAULT  to generalize this logic and will 
factor it out in common driver.

Anyway with  peak_bw = GENI_DEFAULT_BW or 2 * GENI_DEFAULT_BW core clock 
is going to tick at 50 MHz.

9600(19.2 MHz) < GENI_DEFAULT_BW, 2 * GENI_DEFAULT_BW < 2500(50 MHz).


Regards,

Akash

>>     } 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
>

Patch
diff mbox series

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);