| 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
| ||
|
Date: Tue, 21 Mar 2023 22:23:04 +0800
From: Jacky Huang <ychuang570808@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
lee@...nel.org, mturquette@...libre.com, sboyd@...nel.org,
p.zabel@...gutronix.de,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>, devicetree@...r.kernel.org,
linux-clk@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
linux-serial <linux-serial@...r.kernel.org>, schung@...oton.com,
Jacky Huang <ychuang3@...oton.com>, mjchen@...oton.com
Subject: Re: [PATCH 14/15] tty: serial: Add Nuvoton ma35d1 serial driver
support
Dear Ilpo,
On 2023/3/20 下午 06:04, Ilpo Järvinen wrote:
> On Mon, 20 Mar 2023, Jacky Huang wrote:
>
>> Dear Ilpo,
>>
>>
>> Thanks for your advice.
>>
>> On 2023/3/16 下午 10:54, Ilpo Järvinen wrote:
>>> Hi,
>>>
>>> I'll not note all things below because others have already seemingly
>>> commented many things.
>>>
>>> On Wed, 15 Mar 2023, Jacky Huang wrote:
>>>
>>>> From: Jacky Huang <ychuang3@...oton.com>
>>>>
>>>> This adds UART and console driver for Nuvoton ma35d1 Soc.
>>>> + }
>>>> + ch = (u8)serial_in(up, UART_REG_RBR);
>>> Drop the case.
>> I will fix it.
> I meant "cast" in case it wasn't obvious.
I know that, thank you.
>>>> +/* Enable or disable the rs485 support */
>>>> +static int ma35d1serial_config_rs485(struct uart_port *port,
>>>> + struct ktermios *termios,
>>>> + struct serial_rs485 *rs485conf)
>>>> +{
>>>> + struct uart_ma35d1_port *p = to_ma35d1_uart_port(port);
>>>> +
>>>> + p->rs485 = *rs485conf;
>>>> +
>>>> + if (p->rs485.delay_rts_before_send >= 1000)
>>>> + p->rs485.delay_rts_before_send = 1000;
>>> Don't do this in driver, the core handles the delay limits. You don't seem
>>> to be using the value anyway for anything???
>>>
>>> Please separate the RS485 support into its own patch.
>>
>> OK, we will remove RS485 support from this initial patch.
>> Once this initial patch was merged, we will submit the patch for RS485
>> support.
> You could do that but you could just as well include it into the same
> series as another patch after the main patch.
>>>> + serial_out(p, UART_FUN_SEL,
>>>> + (serial_in(p, UART_FUN_SEL) & ~FUN_SEL_MASK));
>>>> +
>>>> + if (rs485conf->flags & SER_RS485_ENABLED) {
>>>> + serial_out(p, UART_FUN_SEL,
>>>> + (serial_in(p, UART_FUN_SEL) | FUN_SEL_RS485));
>>> Does this pair of serial_out()s glitch the RS485 line if ->rs485_config()
>>> is called while RS485 mode is already set?
>>>
>>> Why you need to do serial_in() from the UART_FUN_SEL twice?
>> UART_FUN_SEL (2 bits) definition:
>> 00 - UART function
>> 01 - IrDA function
>> 11 - RS485 function
>>
>> The first searial_in() is used to clear set as UART function.
>> The second one is used to set RS485 function if SER_RS485_ENABLED is true.
> I got that, but it doesn't answer either of my questions which are:
>
> Can you clear the UART function without causing a glitch in the RS485?
> ->rs485_config() can be called while already in RS485 mode so does it
> cause the UART to temporarily switch away from RS485 mode to "UART
> function" until the second write.
>
> Also, you didn't explain why you need to read the register again, does
> the HW play with other bits when you do the clearing or to they remain
> the same (in which case you can just use a temporary variable to store
> the value)? ...It would be better to just write once too so this question
> might not matter in the end.
Thank you for the detailed explanation.
OK, the register won't change. I will modify the code to read once and
write once only.
>>>> + if (pdev->dev.of_node) {
>>>> + ret = of_alias_get_id(pdev->dev.of_node, "serial");
>>>> + if (ret < 0) {
>>>> + dev_err(&pdev->dev,
>>>> + "failed to get alias/pdev id, errno %d\n",
>>>> + ret);
>>> Just put error prints to one line if you don't break 100 chars limit.
>> But the checkpatch limitation is 80 characters.
> No, it isn't. It was changed years ago already.
I have a test on the checkpatch script.
You are right. It won't complain about over 80 characters now.
>>>> +++ b/drivers/tty/serial/ma35d1_serial.h
>>>> @@ -0,0 +1,93 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + * MA35D1 serial driver header file
>>>> + * Copyright (C) 2023 Nuvoton Technology Corp.
>>>> + */
>>>> +#ifndef __MA35D1_SERIAL_H__
>>>> +#define __MA35D1_SERIAL_H__
>>>> +
>>>> +/* UART Receive/Transmit Buffer Register */
>>>> +#define UART_REG_RBR 0x00
>>>> +#define UART_REG_THR 0x00
>>>> +
>>>> +/* UART Interrupt Enable Register */
>>>> +#define UART_REG_IER 0x04
>>>> +#define RDA_IEN 0x00000001 /* RBR Available Interrupt Enable
>>>> */
>>>> +#define THRE_IEN 0x00000002 /* THR Empty Interrupt Enable */
>>>> +#define RLS_IEN 0x00000004 /* RX Line Status Interrupt Enable
>>>> */
>>>> +#define RTO_IEN 0x00000010 /* RX Time-out Interrupt Enable */
>>>> +#define BUFERR_IEN 0x00000020 /* Buffer Error Interrupt Enable */
>>>> +#define TIME_OUT_EN 0x00000800 /* RX Buffer Time-out Counter
>>>> Enable */
>>>> +
>>>> +/* UART FIFO Control Register */
>>>> +#define UART_REG_FCR 0x08
>>>> +#define RFR 0x00000002 /* RX Field Software Reset */
>>>> +#define TFR 0x00000004 /* TX Field Software Reset */
>>>> +
>>>> +/* UART Line Control Register */
>>>> +#define UART_REG_LCR 0x0C
>>>> +#define NSB 0x00000004 /* Number of “STOP Bit” */
>>>> +#define PBE 0x00000008 /* Parity Bit Enable */
>>>> +#define EPE 0x00000010 /* Even Parity Enable */
>>>> +#define SPE 0x00000020 /* Stick Parity Enable */
>>>> +#define BCB 0x00000040 /* Break Control */
>>>> +
>>>> +/* UART Modem Control Register */
>>>> +#define UART_REG_MCR 0x10
>>>> +#define RTS 0x00000020 /* nRTS Signal Control */
>>>> +#define RTSACTLV 0x00000200 /* nRTS Pin Active Level */
>>>> +#define RTSSTS 0x00002000 /* nRTS Pin Status (Read Only) */
>>>> +
>>>> +/* UART Modem Status Register */
>>>> +#define UART_REG_MSR 0x14
>>>> +#define CTSDETF 0x00000001 /* Detect nCTS State Change Flag */
>>>> +#define CTSSTS 0x00000010 /* nCTS Pin Status (Read Only) */
>>>> +#define CTSACTLV 0x00000100 /* nCTS Pin Active Level */
>>>> +
>>>> +/* UART FIFO Status Register */
>>>> +#define UART_REG_FSR 0x18
>>>> +#define RX_OVER_IF 0x00000001 /* RX Overflow Error Interrupt Flag */
>>>> +#define PEF 0x00000010 /* Parity Error Flag*/
>>>> +#define FEF 0x00000020 /* Framing Error Flag */
>>>> +#define BIF 0x00000040 /* Break Interrupt Flag */
>>>> +#define RX_EMPTY 0x00004000 /* Receiver FIFO Empty (Read Only) */
>>>> +#define RX_FULL 0x00008000 /* Receiver FIFO Full (Read Only)
>>>> */
>>>> +#define TX_EMPTY 0x00400000 /* Transmitter FIFO Empty (Read Only) */
>>>> +#define TX_FULL 0x00800000 /* Transmitter FIFO Full (Read
>>>> Only) */
>>>> +#define TX_OVER_IF 0x01000000 /* TX Overflow Error Interrupt Flag */
>>>> +#define TE_FLAG 0x10000000 /* Transmitter Empty Flag (Read
>>>> Only) */
>>>> +
>>>> +/* UART Interrupt Status Register */
>>>> +#define UART_REG_ISR 0x1C
>>>> +#define RDA_IF 0x00000001 /* RBR Available Interrupt Flag */
>>>> +#define THRE_IF 0x00000002 /* THR Empty Interrupt Flag */
>>>> +#define RLSIF 0x00000004 /* Receive Line Interrupt Flag */
>>>> +#define MODEMIF 0x00000008 /* MODEM Interrupt Flag */
>>>> +#define RXTO_IF 0x00000010 /* RX Time-out Interrupt Flag */
>>>> +#define BUFEIF 0x00000020 /* Buffer Error Interrupt Flag */
>>>> +#define WK_IF 0x00000040 /* UART Wake-up Interrupt Flag */
>>>> +#define RDAINT 0x00000100 /* RBR Available Interrupt
>>>> Indicator */
>>>> +#define THRE_INT 0x00000200 /* THR Empty Interrupt Indicator */
> I forgot to mention earlier, there are many defines above which should use
> BIT().
>
Sure we will fix them all.
Best regards,
Jacky Huang
Powered by blists - more mailing lists