| 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: Mon, 20 Mar 2023 12:04:08 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Jacky Huang <ychuang570808@...il.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
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.
> > > +/* 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.
> > > + 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.
> > > +++ 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().
--
i.
Powered by blists - more mailing lists