| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <42eb99eb-5882-489f-b9f2-71d131429052@quicinc.com>
Date: Thu, 26 Jun 2025 21:27:11 +0530
From: Praveen Talari <quic_ptalari@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby
<jirislaby@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski
<krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Konrad Dybcio
<konradybcio@...nel.org>,
<linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-serial@...r.kernel.org>, <devicetree@...r.kernel.org>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>, <psodagud@...cinc.com>,
<djaggi@...cinc.com>, <quic_msavaliy@...cinc.com>,
<quic_vtanuku@...cinc.com>, <quic_arandive@...cinc.com>,
<quic_mnaresh@...cinc.com>, <quic_shazhuss@...cinc.com>
Subject: Re: [PATCH v6 6/8] serial: qcom-geni: move clock-rate logic to
separate function
HI Bjorn,
Thank you for review.
On 6/17/2025 9:37 PM, Bjorn Andersson wrote:
> On Mon, Jun 16, 2025 at 09:34:27PM +0530, Praveen Talari wrote:
>> Hi Bryan,
>>
>> Gentle reminder!!
>>
>
> As I've told you all countless times, if you want attention to your
> patchset review each others patches! For some reason you're the only one
> showing interest in getting this series merged.
My intention is to CC Bryan with a polite reminder, one week after the
initial post.
>
>> Thanks,
>> Praveen Talari
>>
>> On 6/6/2025 10:51 PM, Praveen Talari wrote:
>>> Facilitates future modifications within the new function,
>>> leading to better readability and maintainability of the code.
>>>
>>> Move the code that handles the actual logic of clock-rate
>>> calculations to a separate function geni_serial_set_rate()
>>> which enhances code readability.
>>>
>>> Signed-off-by: Praveen Talari <quic_ptalari@...cinc.com>
>>> ---
>>> v5 -> v6
>>> - used "unsigned int" instead of "unsigned long" in newly
>>> added API function params to avoid the format specifier
>>> warnings.
>>>
>>> v3 -> v4
>>> - added version log after ---
>>>
>>> v1 -> v2
>>> - resolved build warnings for datatype format specifiers
>>> - removed double spaces in log
>>> ---
>>> drivers/tty/serial/qcom_geni_serial.c | 56 +++++++++++++++++----------
>>> 1 file changed, 36 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>>> index 715db35bab2f..b6fa7dc9b1fb 100644
>>> --- a/drivers/tty/serial/qcom_geni_serial.c
>>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>>> @@ -1283,27 +1283,14 @@ static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud,
>>> return ser_clk;
>>> }
>>> -static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>> - struct ktermios *termios,
>>> - const struct ktermios *old)
>>> +static int geni_serial_set_rate(struct uart_port *uport, unsigned int baud)
>>> {
>>> - unsigned int baud;
>>> - u32 bits_per_char;
>>> - u32 tx_trans_cfg;
>>> - u32 tx_parity_cfg;
>>> - u32 rx_trans_cfg;
>>> - u32 rx_parity_cfg;
>>> - u32 stop_bit_len;
>>> - unsigned int clk_div;
>>> - u32 ser_clk_cfg;
>>> struct qcom_geni_serial_port *port = to_dev_port(uport);
>>> unsigned long clk_rate;
>>> - u32 ver, sampling_rate;
>>> unsigned int avg_bw_core;
>>> - unsigned long timeout;
>>> -
>>> - /* baud rate */
>>> - baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
>>> + unsigned int clk_div;
>>> + u32 ver, sampling_rate;
>>> + u32 ser_clk_cfg;
>>> sampling_rate = UART_OVERSAMPLING;
>>> /* Sampling rate is halved for IP versions >= 2.5 */
>>> @@ -1317,7 +1304,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>> dev_err(port->se.dev,
>>> "Couldn't find suitable clock rate for %u\n",
>>> baud * sampling_rate);
>>> - return;
>>> + return -EINVAL;
>>> }
>>> dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div = %u\n",
>>> @@ -1339,6 +1326,37 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>> port->se.icc_paths[CPU_TO_GENI].avg_bw = Bps_to_icc(baud);
>>> geni_icc_set_bw(&port->se);
>>> + writel(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
>>> + writel(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
>>> + return 0;
>>> +}
>>> +
>>> +static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>> + struct ktermios *termios,
>>> + const struct ktermios *old)
>>> +{
>>> + struct qcom_geni_serial_port *port = to_dev_port(uport);
>>> + unsigned int baud;
>>> + unsigned long timeout;
>>> + u32 bits_per_char;
>>> + u32 tx_trans_cfg;
>>> + u32 tx_parity_cfg;
>>> + u32 rx_trans_cfg;
>>> + u32 rx_parity_cfg;
>>> + u32 stop_bit_len;
>>> + int ret = 0;
>>> +
>>> + /* baud rate */
>>> + baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
>>> +
>>> + ret = geni_serial_set_rate(uport, baud);
>>> + if (ret) {
>>> + dev_err(port->se.dev,
>>> + "%s: Failed to set baud:%u ret:%d\n",
>>> + __func__, baud, ret);
>
> As far as I can tell there's one error path in geni_serial_set_rate()
> and there you already printed a more specific error message, as such
> this doesn't add any value.
Sure, will review and update in next patch-set.
Thanks,
Praveen Talari
>
> PS. In general, please don't use __func__, write helpful error messages
> instead.
>
> Regards,
> Bjorn
>
>>> + return;
>>> + }
>>> +
>>> /* parity */
>>> tx_trans_cfg = readl(uport->membase + SE_UART_TX_TRANS_CFG);
>>> tx_parity_cfg = readl(uport->membase + SE_UART_TX_PARITY_CFG);
>>> @@ -1406,8 +1424,6 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>> writel(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN);
>>> writel(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN);
>>> writel(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN);
>>> - writel(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
>>> - writel(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
>>> }
>>> #ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
Powered by blists - more mailing lists