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

From: Akash Asthana <akashast@codeaurora.org>
To: Matthias Kaehlcke <mka@chromium.org>
Cc: gregkh@linuxfoundation.org, agross@kernel.org,
	bjorn.andersson@linaro.org, wsa@the-dreams.de,
	broonie@kernel.org, mark.rutland@arm.com, robh+dt@kernel.org,
	georgi.djakov@linaro.org, linux-i2c@vger.kernel.org,
	linux-spi@vger.kernel.org, devicetree@vger.kernel.org,
	swboyd@chromium.org, mgautam@codeaurora.org,
	linux-arm-msm@vger.kernel.org, linux-serial@vger.kernel.org,
	dianders@chromium.org, evgreen@chromium.org
Subject: Re: [PATCH V3 6/8] tty: serial: qcom_geni_serial: Add interconnect support
Date: Tue, 7 Apr 2020 14:49:20 +0530	[thread overview]
Message-ID: <db7d1369-33aa-b0b3-ec44-2018ea382887@codeaurora.org> (raw)
In-Reply-To: <20200331193949.GK199755@google.com>

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

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project