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 15:10:50 +0530	[thread overview]
Message-ID: <694f2664-3ef2-b3a6-c447-c33086172b02@codeaurora.org> (raw)
In-Reply-To: <db7d1369-33aa-b0b3-ec44-2018ea382887@codeaurora.org>

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
>
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project