Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
Hi Karthik, On Tue, Mar 13, 2018 at 1:16 PM Karthik Ramasubramanian < krama...@codeaurora.org> wrote: > On 3/2/2018 5:11 PM, Evan Green wrote: > >> + > >> +#ifdef CONFIG_CONSOLE_POLL > >> +#define RX_BYTES_PW 1 > >> +#else > >> +#define RX_BYTES_PW 4 > >> +#endif > > > > This seems fishy to me. Does either setting work? If so, why not just > > have one value? > Yes, either one works. In the interrupt driven mode, sometimes due to > increased interrupt latency the RX FIFO may overflow if we use only 1 > byte per FIFO word - given there are no flow control lines in the debug > uart. Hence using 4 bytes in the FIFO word will help to prevent the FIFO > overflow - especially in the case where commands are executed through > scripts. > In polling mode, using 1 byte per word helps to use the hardware to > buffer the data instead of software buffering especially when the > framework keeps reading one byte at a time. Ok, I think I understand. Let me paraphrase in case I'm wrong: Normally, you want all 4 bytes per word so that you use the hardware's full FIFO capability. This works out well since on receive you tell the system how many bytes you've received, so you're never stuck with leftover bytes. In polling mode, however, the system asks you for one byte, and the problem is with 4 bytes per FIFO word you may end up having read three additional bytes that you don't know what to do with. Configuring the UART to 1 byte per word allows you to skip coding up a couple of conditionals and an extra couple u32s in the device struct for saving those extra bytes, but divides the hardware FIFO size by 4. It seems a little cheesy to me just to avoid a bit of logic, but I'm not going to put my foot down about it. I guess it might get complicated when the console pulls in four bytes, but then only ends up eating one of them, and then we have to figure out how to get the other three into the normal ISR-based path. > > > >> +static bool qcom_geni_serial_poll_bit(struct uart_port *uport, > >> + int offset, int bit_field, bool set) > >> +{ > >> + u32 reg; > >> + struct qcom_geni_serial_port *port; > >> + unsigned int baud; > >> + unsigned int fifo_bits; > >> + unsigned long timeout_us = 2; > >> + > >> + /* Ensure polling is not re-ordered before the prior writes/reads */ > >> + mb(); > >> + > >> + if (uport->private_data) { > >> + port = to_dev_port(uport, uport); > >> + baud = port->cur_baud; > >> + if (!baud) > >> + baud = 115200; > >> + fifo_bits = port->tx_fifo_depth * port->tx_fifo_width; > >> + /* > >> +* Total polling iterations based on FIFO worth of bytes to be > >> +* sent at current baud .Add a little fluff to the wait. > >> +*/ > >> + timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500; > > > > This fluff is a little mysterious, can it be explained at all? Do you > > think the fluff factor is in units of time (as you have it) or bits? > > Time makes sense I guess if we're worried about clock source > > differences. > The fluff is in micro-seconds and can help with unforeseen delays in > emulation platforms. So emulated platforms go out to lunch, but that generally doesn't depend on baud rate or how many bits there are. Ok. Might be worth noting what that's for. > > > >> + > >> +static void qcom_geni_serial_console_write(struct console *co, const char *s, > >> + unsigned int count) > >> +{ > >> + struct uart_port *uport; > >> + struct qcom_geni_serial_port *port; > >> + bool locked = true; > >> + unsigned long flags; > >> + > >> + WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS); > >> + > >> + port = get_port_from_line(co->index); > >> + if (IS_ERR(port)) > >> + return; > >> + > >> + uport = >uport; > >> + if (oops_in_progress) > >> + locked = spin_trylock_irqsave(>lock, flags); > >> + else > >> + spin_lock_irqsave(>lock, flags); > >> + > >> + if (locked) { > >> + __qcom_geni_serial_console_write(uport, s, count); > >> + spin_unlock_irqrestore(>lock, flags); > > > > I too am a little lost on the locking here. What exactly is the lock > > protecting? Looks like for the most part it's trying to synchronize > > with the ISR? What specifically in the ISR? I just wanted to go > > through and check to make sure whatever the shared resource is is > > appropriately protected. > The lock protects 2 simultaneous writers from putting the hardware in > the bad state. The output of the command entered in a shell can trigger > a write in the interrupt context while logging activity can trigger a > simultaneous write. Can you be any more specific here? What puts the hardware in a bad state? Maybe I see what you're
Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
On 3/2/2018 5:11 PM, Evan Green wrote: >> + >> +#ifdef CONFIG_CONSOLE_POLL >> +#define RX_BYTES_PW 1 >> +#else >> +#define RX_BYTES_PW 4 >> +#endif > > This seems fishy to me. Does either setting work? If so, why not just > have one value? Yes, either one works. In the interrupt driven mode, sometimes due to increased interrupt latency the RX FIFO may overflow if we use only 1 byte per FIFO word - given there are no flow control lines in the debug uart. Hence using 4 bytes in the FIFO word will help to prevent the FIFO overflow - especially in the case where commands are executed through scripts. In polling mode, using 1 byte per word helps to use the hardware to buffer the data instead of software buffering especially when the framework keeps reading one byte at a time. > >> +static bool qcom_geni_serial_poll_bit(struct uart_port *uport, >> + int offset, int bit_field, bool set) >> +{ >> + u32 reg; >> + struct qcom_geni_serial_port *port; >> + unsigned int baud; >> + unsigned int fifo_bits; >> + unsigned long timeout_us = 2; >> + >> + /* Ensure polling is not re-ordered before the prior writes/reads */ >> + mb(); >> + >> + if (uport->private_data) { >> + port = to_dev_port(uport, uport); >> + baud = port->cur_baud; >> + if (!baud) >> + baud = 115200; >> + fifo_bits = port->tx_fifo_depth * port->tx_fifo_width; >> + /* >> +* Total polling iterations based on FIFO worth of bytes to >> be >> +* sent at current baud .Add a little fluff to the wait. >> +*/ >> + timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500; > > This fluff is a little mysterious, can it be explained at all? Do you > think the fluff factor is in units of time (as you have it) or bits? > Time makes sense I guess if we're worried about clock source > differences. The fluff is in micro-seconds and can help with unforeseen delays in emulation platforms. > >> + >> +static void qcom_geni_serial_console_write(struct console *co, const char >> *s, >> + unsigned int count) >> +{ >> + struct uart_port *uport; >> + struct qcom_geni_serial_port *port; >> + bool locked = true; >> + unsigned long flags; >> + >> + WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS); >> + >> + port = get_port_from_line(co->index); >> + if (IS_ERR(port)) >> + return; >> + >> + uport = >uport; >> + if (oops_in_progress) >> + locked = spin_trylock_irqsave(>lock, flags); >> + else >> + spin_lock_irqsave(>lock, flags); >> + >> + if (locked) { >> + __qcom_geni_serial_console_write(uport, s, count); >> + spin_unlock_irqrestore(>lock, flags); > > I too am a little lost on the locking here. What exactly is the lock > protecting? Looks like for the most part it's trying to synchronize > with the ISR? What specifically in the ISR? I just wanted to go > through and check to make sure whatever the shared resource is is > appropriately protected. The lock protects 2 simultaneous writers from putting the hardware in the bad state. The output of the command entered in a shell can trigger a write in the interrupt context while logging activity can trigger a simultaneous write. > >> + } >> +} >> + >> +static int handle_rx_console(struct uart_port *uport, u32 rx_bytes, bool >> drop) >> +{ >> + u32 i = rx_bytes; >> + u32 rx_fifo; >> + unsigned char *buf; >> + struct tty_port *tport; >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); >> + >> + tport = >state->port; >> + while (i > 0) { >> + int c; >> + int bytes = i > port->rx_bytes_pw ? port->rx_bytes_pw : i; > > Please replace this with a min macro. Ok. > >> +static int qcom_geni_serial_handle_tx(struct uart_port *uport) >> +{ >> + int ret = 0; >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); >> + struct circ_buf *xmit = >state->xmit; >> + size_t avail; >> + size_t remaining; >> + int i = 0; >> + u32 status; >> + unsigned int chunk; >> + int tail; >> + >> + chunk = uart_circ_chars_pending(xmit); >> + status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS); >> + /* Both FIFO and framework buffer are drained */ >> + if ((chunk == port->xmit_size) && !status) { >> + port->xmit_size = 0; >> + uart_circ_clear(xmit); >> + qcom_geni_serial_stop_tx(uport); >> + goto out_write_wakeup; >> + } >> + chunk -= port->xmit_size; >> + >> + avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw; >> + tail = (xmit->tail + port->xmit_size) & (UART_XMIT_SIZE -
Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
On 3/8/2018 3:32 PM, Stephen Boyd wrote: Quoting Karthik Ramasubramanian (2018-03-07 22:06:29) On 3/6/2018 2:45 PM, Stephen Boyd wrote: Quoting Karthik Ramasubramanian (2018-03-05 16:51:23) On 3/2/2018 3:11 PM, Stephen Boyd wrote: Ok. I've seen similar designs in some mmc drivers. For example, you can look at drivers/mmc/host/meson-gx-mmc.c and see that they register a clk_ops and then just start using that clk directly from the driver. There are also some helper functions for dividers that would hopefully make the job of calculating the best divider easier. Thanks for the pointers. I will take a look at it. In the meanwhile I had discussions with our clock team. They pointed out that the register to write the divider value here is outside the scope of clock controller which makes it trickier to implement your suggestion. They are already in the mailing list and we will discuss further and get back to you in this regard. Ok. Let me know if I can help answer any questions. Ok. Why are these noirq variants? Please add a comment. The intention is to enable the console UART port usage as late as possible in the suspend cycle. Hence noirq variants. I will add this as a comment. Please let me know if the usage does not match the intention. Hm.. Does no_console_suspend not work? Or not work well enough? It works. When console suspend is disabled, the suspend operation does not get triggered and the resume operation checks if the console suspend is disabled and performs the needed thing. Ok so then do we need the noirq variants? Or console suspend is special enough for this to not matter? I am a little confused as to whether you prefer the console to not suspend at all or you prefer the console suspend at an earlier stage than no_irq stage. If it is former, then with the console_suspend_enabled flag set by default this seems the right thing to do. Atleast my understanding is that console is expecting the serial port to suspend as well. If it is latter, then I will check the stage at which suspend_console() is initiated and can suspend the serial port after that. Regards, Karthik. -- Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
Quoting Karthik Ramasubramanian (2018-03-07 22:06:29) > > > On 3/6/2018 2:45 PM, Stephen Boyd wrote: > > Quoting Karthik Ramasubramanian (2018-03-05 16:51:23) > >> On 3/2/2018 3:11 PM, Stephen Boyd wrote: > > > > Ok. I've seen similar designs in some mmc drivers. For example, you can > > look at drivers/mmc/host/meson-gx-mmc.c and see that they register a > > clk_ops and then just start using that clk directly from the driver. > > There are also some helper functions for dividers that would hopefully > > make the job of calculating the best divider easier. > Thanks for the pointers. I will take a look at it. In the meanwhile I > had discussions with our clock team. They pointed out that the register > to write the divider value here is outside the scope of clock controller > which makes it trickier to implement your suggestion. They are already > in the mailing list and we will discuss further and get back to you in > this regard. Ok. Let me know if I can help answer any questions. > >>> > >>> Why are these noirq variants? Please add a comment. > >> The intention is to enable the console UART port usage as late as > >> possible in the suspend cycle. Hence noirq variants. I will add this as > >> a comment. Please let me know if the usage does not match the intention. > > > > Hm.. Does no_console_suspend not work? Or not work well enough? > It works. When console suspend is disabled, the suspend operation does > not get triggered and the resume operation checks if the console suspend > is disabled and performs the needed thing. Ok so then do we need the noirq variants? Or console suspend is special enough for this to not matter? -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
On 3/6/2018 2:45 PM, Stephen Boyd wrote: Quoting Karthik Ramasubramanian (2018-03-05 16:51:23) On 3/2/2018 3:11 PM, Stephen Boyd wrote: Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:09) + size_t chars_to_write = 0; + size_t avail = DEF_FIFO_DEPTH_WORDS - DEF_TX_WM; + + /* +* If the WM bit never set, then the Tx state machine is not +* in a valid state, so break, cancel/abort any existing +* command. Unfortunately the current data being written is +* lost. +*/ + while (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS, + M_TX_FIFO_WATERMARK_EN, true)) Does this ever timeout? So many nested while loops makes it hard to follow. Yes. Based on the baud rate of 115200 and the FIFO Depth & Width of (16 * 32), the poll should not take more than 5 ms under the timeout scenario. Sure, but I'm asking if this has any sort of timeout associated with it. It looks to be a while loop that could possibly go forever? I will change it from a while loop to if condition to make it clear. +static void qcom_geni_serial_console_write(struct console *co, const char *s, + unsigned int count) +{ + struct uart_port *uport; + struct qcom_geni_serial_port *port; + bool locked = true; + unsigned long flags; + + WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS); + + port = get_port_from_line(co->index); + if (IS_ERR(port)) + return; + + uport = >uport; + if (oops_in_progress) + locked = spin_trylock_irqsave(>lock, flags); + else + spin_lock_irqsave(>lock, flags); + + if (locked) { + __qcom_geni_serial_console_write(uport, s, count); So if oops is in progress, and we didn't lock here, we don't output data? I'd think we would always want to write to the fifo, just make the lock grab/release conditional. If we fail to grab the lock, then there is another active writer. So trying to write to the fifo will put the hardware in bad state because writer has programmed the hardware to write 'x' number of words and this thread will over-write it with 'y' number of words. Also the data that you might see in the console might be garbled. I suspect that because this is the serial console, and we want it to always output stuff even when we're going down in flames, we may want to ignore that case and just wait for the fifo to free up and then overwrite the number of words that we want to output and push out more characters. I always get confused though because there seem to be lots of different ways to do things in serial drivers and not too much clear documentation that I've read describing what to do. Ok. If the active writer is interrupted due to OOPS handler, then the interrupted write can be cancelled and the write from OOPS handler can be performed. + spin_unlock_irqrestore(>lock, flags); + } +} [...] + + if (m_irq_status & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN) && + m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN)) + qcom_geni_serial_handle_tx(uport); + + if (s_irq_status & S_GP_IRQ_0_EN || s_irq_status & S_GP_IRQ_1_EN) { + if (s_irq_status & S_GP_IRQ_0_EN) + uport->icount.parity++; + drop_rx = true; + } else if (s_irq_status & S_GP_IRQ_2_EN || + s_irq_status & S_GP_IRQ_3_EN) { + uport->icount.brk++; How does break character handling work? I see the accounting here, but don't see any uart_handle_break() call anywhere. The reason it is not added is because the hardware does not indicate when the break character occured. It can be any one of the FIFO words. The statistics is updated to give an idea that the break happened. We can add uart_handle_break() but it may not be at an accurate position for the above mentioned reason. Sounds like the previous uart design. We would want uart_handle_break() support for kgdb to work over serial. Do we still get an interrupt signal that a break character is somewhere in the fifo? If we get that, then does it also put a NUL (0) character into the fifo that we can find? I had to do something like that before, where we detect the irq and then we go through the fifo a character at a time to find the break character that looks like a NUL, and then let sysrq core handle the characters after that break character comes in. I will use the same logic as in blsp2 serial to catch the break character, same as NULL and push the break character to the framework. +} + +static unsigned long get_clk_div_rate(unsigned int baud, unsigned int *clk_div) +{ + unsigned long ser_clk; + unsigned long desired_clk; + + desired_clk = baud * UART_OVERSAMPLING; +
Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
Quoting Karthik Ramasubramanian (2018-03-05 16:51:23) > On 3/2/2018 3:11 PM, Stephen Boyd wrote: > > Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:09) > > > >> + size_t chars_to_write = 0; > >> + size_t avail = DEF_FIFO_DEPTH_WORDS - DEF_TX_WM; > >> + > >> + /* > >> +* If the WM bit never set, then the Tx state machine is > >> not > >> +* in a valid state, so break, cancel/abort any existing > >> +* command. Unfortunately the current data being written is > >> +* lost. > >> +*/ > >> + while (!qcom_geni_serial_poll_bit(uport, > >> SE_GENI_M_IRQ_STATUS, > >> + M_TX_FIFO_WATERMARK_EN, > >> true)) > > > > Does this ever timeout? So many nested while loops makes it hard to > > follow. > Yes. Based on the baud rate of 115200 and the FIFO Depth & Width of (16 > * 32), the poll should not take more than 5 ms under the timeout scenario. Sure, but I'm asking if this has any sort of timeout associated with it. It looks to be a while loop that could possibly go forever? > >> +static void qcom_geni_serial_console_write(struct console *co, const char > >> *s, > >> + unsigned int count) > >> +{ > >> + struct uart_port *uport; > >> + struct qcom_geni_serial_port *port; > >> + bool locked = true; > >> + unsigned long flags; > >> + > >> + WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS); > >> + > >> + port = get_port_from_line(co->index); > >> + if (IS_ERR(port)) > >> + return; > >> + > >> + uport = >uport; > >> + if (oops_in_progress) > >> + locked = spin_trylock_irqsave(>lock, flags); > >> + else > >> + spin_lock_irqsave(>lock, flags); > >> + > >> + if (locked) { > >> + __qcom_geni_serial_console_write(uport, s, count); > > > > So if oops is in progress, and we didn't lock here, we don't output > > data? I'd think we would always want to write to the fifo, just make the > > lock grab/release conditional. > If we fail to grab the lock, then there is another active writer. So > trying to write to the fifo will put the hardware in bad state because > writer has programmed the hardware to write 'x' number of words and this > thread will over-write it with 'y' number of words. Also the data that > you might see in the console might be garbled. I suspect that because this is the serial console, and we want it to always output stuff even when we're going down in flames, we may want to ignore that case and just wait for the fifo to free up and then overwrite the number of words that we want to output and push out more characters. I always get confused though because there seem to be lots of different ways to do things in serial drivers and not too much clear documentation that I've read describing what to do. > > > >> + spin_unlock_irqrestore(>lock, flags); > >> + } > >> +} [...] > >> + > >> + if (m_irq_status & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN) && > >> + m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN)) > >> + qcom_geni_serial_handle_tx(uport); > >> + > >> + if (s_irq_status & S_GP_IRQ_0_EN || s_irq_status & S_GP_IRQ_1_EN) { > >> + if (s_irq_status & S_GP_IRQ_0_EN) > >> + uport->icount.parity++; > >> + drop_rx = true; > >> + } else if (s_irq_status & S_GP_IRQ_2_EN || > >> + s_irq_status & S_GP_IRQ_3_EN) { > >> + uport->icount.brk++; > > > > How does break character handling work? I see the accounting here, but > > don't see any uart_handle_break() call anywhere. > The reason it is not added is because the hardware does not indicate > when the break character occured. It can be any one of the FIFO words. > The statistics is updated to give an idea that the break happened. We > can add uart_handle_break() but it may not be at an accurate position > for the above mentioned reason. Sounds like the previous uart design. We would want uart_handle_break() support for kgdb to work over serial. Do we still get an interrupt signal that a break character is somewhere in the fifo? If we get that, then does it also put a NUL (0) character into the fifo that we can find? I had to do something like that before, where we detect the irq and then we go through the fifo a character at a time to find the break character that looks like a NUL, and then let sysrq core handle the characters after that break character comes in. > > > > > >> +} > >> + > >> +static unsigned long get_clk_div_rate(unsigned int baud, unsigned int > >> *clk_div) > >> +{ > >> + unsigned long ser_clk; > >> + unsigned long desired_clk; > >> + > >> + desired_clk = baud * UART_OVERSAMPLING; > >> + ser_clk =
Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
On 3/2/2018 3:11 PM, Stephen Boyd wrote: Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:09) diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig index 3682fd3..c6b1500 100644 --- a/drivers/tty/serial/Kconfig +++ b/drivers/tty/serial/Kconfig @@ -1104,6 +1104,17 @@ config SERIAL_MSM_CONSOLE select SERIAL_CORE_CONSOLE select SERIAL_EARLYCON +config SERIAL_QCOM_GENI + bool "QCOM on-chip GENI based serial port support" This can be tristate. + depends on ARCH_QCOM || COMPILE_TEST ? Ok. + depends on QCOM_GENI_SE + select SERIAL_CORE This can stay. + select SERIAL_CORE_CONSOLE + select SERIAL_EARLYCON These two can go to a new config option, like SERIAL_QCOM_GENI_CONSOLE, and that would be bool. Please take a look at the existing SERIAL_MSM and SERIAL_MSM_CONSOLE setup to understand how to do it. Ok. + help + Serial driver for Qualcomm Technologies Inc's GENI based QUP + hardware. + config SERIAL_VT8500 bool "VIA VT8500 on-chip serial port support" depends on ARCH_VT8500 diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c new file mode 100644 index 000..8536b7d --- /dev/null +++ b/drivers/tty/serial/qcom_geni_serial.c @@ -0,0 +1,1181 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2017-2018, The Linux foundation. All rights reserved. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* UART specific GENI registers */ +#define SE_UART_TX_TRANS_CFG 0x25c +#define SE_UART_TX_WORD_LEN0x268 +#define SE_UART_TX_STOP_BIT_LEN0x26c +#define SE_UART_TX_TRANS_LEN 0x270 +#define SE_UART_RX_TRANS_CFG 0x280 +#define SE_UART_RX_WORD_LEN0x28c +#define SE_UART_RX_STALE_CNT 0x294 +#define SE_UART_TX_PARITY_CFG 0x2a4 +#define SE_UART_RX_PARITY_CFG 0x2a8 + +/* SE_UART_TRANS_CFG */ +#define UART_TX_PAR_EN BIT(0) +#define UART_CTS_MASK BIT(1) + +/* SE_UART_TX_WORD_LEN */ +#define TX_WORD_LEN_MSKGENMASK(9, 0) + +/* SE_UART_TX_STOP_BIT_LEN */ +#define TX_STOP_BIT_LEN_MSKGENMASK(23, 0) +#define TX_STOP_BIT_LEN_1 0 +#define TX_STOP_BIT_LEN_1_51 +#define TX_STOP_BIT_LEN_2 2 + +/* SE_UART_TX_TRANS_LEN */ +#define TX_TRANS_LEN_MSK GENMASK(23, 0) + +/* SE_UART_RX_TRANS_CFG */ +#define UART_RX_INS_STATUS_BIT BIT(2) +#define UART_RX_PAR_EN BIT(3) + +/* SE_UART_RX_WORD_LEN */ +#define RX_WORD_LEN_MASK GENMASK(9, 0) + +/* SE_UART_RX_STALE_CNT */ +#define RX_STALE_CNT GENMASK(23, 0) + +/* SE_UART_TX_PARITY_CFG/RX_PARITY_CFG */ +#define PAR_CALC_ENBIT(0) +#define PAR_MODE_MSK GENMASK(2, 1) +#define PAR_MODE_SHFT 1 +#define PAR_EVEN 0x00 +#define PAR_ODD0x01 +#define PAR_SPACE 0x10 +#define PAR_MARK 0x11 + +/* UART M_CMD OP codes */ +#define UART_START_TX 0x1 +#define UART_START_BREAK 0x4 +#define UART_STOP_BREAK0x5 +/* UART S_CMD OP codes */ +#define UART_START_READ0x1 +#define UART_PARAM 0x1 + +#define UART_OVERSAMPLING 32 +#define STALE_TIMEOUT 16 +#define DEFAULT_BITS_PER_CHAR 10 +#define GENI_UART_CONS_PORTS 1 +#define DEF_FIFO_DEPTH_WORDS 16 +#define DEF_TX_WM 2 +#define DEF_FIFO_WIDTH_BITS32 +#define UART_CONSOLE_RX_WM 2 + +#ifdef CONFIG_CONSOLE_POLL +#define RX_BYTES_PW 1 +#else +#define RX_BYTES_PW 4 +#endif + +struct qcom_geni_serial_port { + struct uart_port uport; + struct geni_se se; + char name[20]; + u32 tx_fifo_depth; + u32 tx_fifo_width; + u32 rx_fifo_depth; + u32 tx_wm; + u32 rx_wm; + u32 rx_rfr; + int xfer_mode; Can this be an enum? Ok. + bool port_setup; Maybe just 'setup'? Port is in the type already. Ok. + int (*handle_rx)(struct uart_port *uport, + u32 rx_bytes, bool drop_rx); s/rx_bytes/bytes/ s/drop_rx/drop/ Ok. + unsigned int xmit_size; + unsigned int cur_baud; s/cur// Ok. + unsigned int tx_bytes_pw; + unsigned int rx_bytes_pw; +}; + +static const struct uart_ops qcom_geni_serial_pops; +static struct uart_driver qcom_geni_console_driver; +static int handle_rx_console(struct uart_port *uport, + u32 rx_bytes, bool drop_rx); s/rx_bytes/bytes/ s/drop_rx/drop/ Ok. +static unsigned int qcom_geni_serial_tx_empty(struct uart_port *port); +static bool qcom_geni_serial_poll_bit(struct uart_port *uport, + int offset, int bit_field, bool set); No need to forward declare this? I will check and remove if not required. s/bit_// ? Ok. +static void
Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
Hello Karthik, On Tue, Feb 27, 2018 at 5:38 PM, Karthikeyan Ramasubramanianwrote: > This driver supports GENI based UART Controller in the Qualcomm SOCs. The > Qualcomm Generic Interface (GENI) is a programmable module supporting a > wide range of serial interfaces including UART. This driver support console > operations using FIFO mode of transfer. > > Signed-off-by: Girish Mahadevan > Signed-off-by: Karthikeyan Ramasubramanian > Signed-off-by: Sagar Dharia > Signed-off-by: Doug Anderson > --- > drivers/tty/serial/Kconfig| 11 + > drivers/tty/serial/Makefile |1 + > drivers/tty/serial/qcom_geni_serial.c | 1181 > + > 3 files changed, 1193 insertions(+) > create mode 100644 drivers/tty/serial/qcom_geni_serial.c > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > index 3682fd3..c6b1500 100644 > --- a/drivers/tty/serial/Kconfig > +++ b/drivers/tty/serial/Kconfig > @@ -1104,6 +1104,17 @@ config SERIAL_MSM_CONSOLE > select SERIAL_CORE_CONSOLE > select SERIAL_EARLYCON > > +config SERIAL_QCOM_GENI > + bool "QCOM on-chip GENI based serial port support" > + depends on ARCH_QCOM > + depends on QCOM_GENI_SE My understanding is that this has to be "bool" because there's a console in there, and consoles cannot be built as modules. Stephen is suggesting splitting this option up into two, so you could have serial with or without the console. That's fine, and probably the preferred way. However, you do want to make sure that if serial (or what's soon to be serial+console) is enabled, that QCOM_GENI_SE has to be built =y, and not =m. I'd suggest "select QCOM_GENI_SE" in the new SERIAL_QCOM_GENI_CONSOLE (or whatever it's called). As it is now, if SERIAL_QCOM_GENI=y and QCOM_GENI_SE=m, there's a build failure. GENI_SE is allowed to be built as a module if serial is not enabled and I2C is built as a module. In order to keep the dependency arrows going in the same direction, you might want the I2C driver to "select QCOM_GENI_SE" as well, in order to upgrade GENI_SE to y if I2C is y. > diff --git a/drivers/tty/serial/qcom_geni_serial.c > b/drivers/tty/serial/qcom_geni_serial.c > new file mode 100644 > index 000..8536b7d > --- /dev/null > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -0,0 +1,1181 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2017-2018, The Linux foundation. All rights reserved. > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* UART specific GENI registers */ > +#define SE_UART_TX_TRANS_CFG 0x25c > +#define SE_UART_TX_WORD_LEN0x268 > +#define SE_UART_TX_STOP_BIT_LEN0x26c > +#define SE_UART_TX_TRANS_LEN 0x270 > +#define SE_UART_RX_TRANS_CFG 0x280 > +#define SE_UART_RX_WORD_LEN0x28c > +#define SE_UART_RX_STALE_CNT 0x294 > +#define SE_UART_TX_PARITY_CFG 0x2a4 > +#define SE_UART_RX_PARITY_CFG 0x2a8 > + > +/* SE_UART_TRANS_CFG */ > +#define UART_TX_PAR_EN BIT(0) > +#define UART_CTS_MASK BIT(1) > + > +/* SE_UART_TX_WORD_LEN */ > +#define TX_WORD_LEN_MSKGENMASK(9, 0) > + > +/* SE_UART_TX_STOP_BIT_LEN */ > +#define TX_STOP_BIT_LEN_MSKGENMASK(23, 0) > +#define TX_STOP_BIT_LEN_1 0 > +#define TX_STOP_BIT_LEN_1_51 > +#define TX_STOP_BIT_LEN_2 2 > + > +/* SE_UART_TX_TRANS_LEN */ > +#define TX_TRANS_LEN_MSK GENMASK(23, 0) > + > +/* SE_UART_RX_TRANS_CFG */ > +#define UART_RX_INS_STATUS_BIT BIT(2) > +#define UART_RX_PAR_EN BIT(3) > + > +/* SE_UART_RX_WORD_LEN */ > +#define RX_WORD_LEN_MASK GENMASK(9, 0) > + > +/* SE_UART_RX_STALE_CNT */ > +#define RX_STALE_CNT GENMASK(23, 0) > + > +/* SE_UART_TX_PARITY_CFG/RX_PARITY_CFG */ > +#define PAR_CALC_ENBIT(0) > +#define PAR_MODE_MSK GENMASK(2, 1) > +#define PAR_MODE_SHFT 1 > +#define PAR_EVEN 0x00 > +#define PAR_ODD0x01 > +#define PAR_SPACE 0x10 > +#define PAR_MARK 0x11 > + > +/* UART M_CMD OP codes */ > +#define UART_START_TX 0x1 > +#define UART_START_BREAK 0x4 > +#define UART_STOP_BREAK0x5 > +/* UART S_CMD OP codes */ > +#define UART_START_READ0x1 > +#define UART_PARAM 0x1 > + > +#define UART_OVERSAMPLING 32 > +#define STALE_TIMEOUT 16 > +#define DEFAULT_BITS_PER_CHAR 10 > +#define GENI_UART_CONS_PORTS 1 > +#define DEF_FIFO_DEPTH_WORDS 16 > +#define DEF_TX_WM 2 > +#define DEF_FIFO_WIDTH_BITS32 > +#define UART_CONSOLE_RX_WM 2 > + > +#ifdef CONFIG_CONSOLE_POLL > +#define RX_BYTES_PW 1 > +#else > +#define RX_BYTES_PW 4 >
Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:09) > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > index 3682fd3..c6b1500 100644 > --- a/drivers/tty/serial/Kconfig > +++ b/drivers/tty/serial/Kconfig > @@ -1104,6 +1104,17 @@ config SERIAL_MSM_CONSOLE > select SERIAL_CORE_CONSOLE > select SERIAL_EARLYCON > > +config SERIAL_QCOM_GENI > + bool "QCOM on-chip GENI based serial port support" This can be tristate. > + depends on ARCH_QCOM || COMPILE_TEST ? > + depends on QCOM_GENI_SE > + select SERIAL_CORE This can stay. > + select SERIAL_CORE_CONSOLE > + select SERIAL_EARLYCON These two can go to a new config option, like SERIAL_QCOM_GENI_CONSOLE, and that would be bool. Please take a look at the existing SERIAL_MSM and SERIAL_MSM_CONSOLE setup to understand how to do it. > + help > + Serial driver for Qualcomm Technologies Inc's GENI based QUP > + hardware. > + > config SERIAL_VT8500 > bool "VIA VT8500 on-chip serial port support" > depends on ARCH_VT8500 > diff --git a/drivers/tty/serial/qcom_geni_serial.c > b/drivers/tty/serial/qcom_geni_serial.c > new file mode 100644 > index 000..8536b7d > --- /dev/null > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -0,0 +1,1181 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2017-2018, The Linux foundation. All rights reserved. > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* UART specific GENI registers */ > +#define SE_UART_TX_TRANS_CFG 0x25c > +#define SE_UART_TX_WORD_LEN0x268 > +#define SE_UART_TX_STOP_BIT_LEN0x26c > +#define SE_UART_TX_TRANS_LEN 0x270 > +#define SE_UART_RX_TRANS_CFG 0x280 > +#define SE_UART_RX_WORD_LEN0x28c > +#define SE_UART_RX_STALE_CNT 0x294 > +#define SE_UART_TX_PARITY_CFG 0x2a4 > +#define SE_UART_RX_PARITY_CFG 0x2a8 > + > +/* SE_UART_TRANS_CFG */ > +#define UART_TX_PAR_EN BIT(0) > +#define UART_CTS_MASK BIT(1) > + > +/* SE_UART_TX_WORD_LEN */ > +#define TX_WORD_LEN_MSKGENMASK(9, 0) > + > +/* SE_UART_TX_STOP_BIT_LEN */ > +#define TX_STOP_BIT_LEN_MSKGENMASK(23, 0) > +#define TX_STOP_BIT_LEN_1 0 > +#define TX_STOP_BIT_LEN_1_51 > +#define TX_STOP_BIT_LEN_2 2 > + > +/* SE_UART_TX_TRANS_LEN */ > +#define TX_TRANS_LEN_MSK GENMASK(23, 0) > + > +/* SE_UART_RX_TRANS_CFG */ > +#define UART_RX_INS_STATUS_BIT BIT(2) > +#define UART_RX_PAR_EN BIT(3) > + > +/* SE_UART_RX_WORD_LEN */ > +#define RX_WORD_LEN_MASK GENMASK(9, 0) > + > +/* SE_UART_RX_STALE_CNT */ > +#define RX_STALE_CNT GENMASK(23, 0) > + > +/* SE_UART_TX_PARITY_CFG/RX_PARITY_CFG */ > +#define PAR_CALC_ENBIT(0) > +#define PAR_MODE_MSK GENMASK(2, 1) > +#define PAR_MODE_SHFT 1 > +#define PAR_EVEN 0x00 > +#define PAR_ODD0x01 > +#define PAR_SPACE 0x10 > +#define PAR_MARK 0x11 > + > +/* UART M_CMD OP codes */ > +#define UART_START_TX 0x1 > +#define UART_START_BREAK 0x4 > +#define UART_STOP_BREAK0x5 > +/* UART S_CMD OP codes */ > +#define UART_START_READ0x1 > +#define UART_PARAM 0x1 > + > +#define UART_OVERSAMPLING 32 > +#define STALE_TIMEOUT 16 > +#define DEFAULT_BITS_PER_CHAR 10 > +#define GENI_UART_CONS_PORTS 1 > +#define DEF_FIFO_DEPTH_WORDS 16 > +#define DEF_TX_WM 2 > +#define DEF_FIFO_WIDTH_BITS32 > +#define UART_CONSOLE_RX_WM 2 > + > +#ifdef CONFIG_CONSOLE_POLL > +#define RX_BYTES_PW 1 > +#else > +#define RX_BYTES_PW 4 > +#endif > + > +struct qcom_geni_serial_port { > + struct uart_port uport; > + struct geni_se se; > + char name[20]; > + u32 tx_fifo_depth; > + u32 tx_fifo_width; > + u32 rx_fifo_depth; > + u32 tx_wm; > + u32 rx_wm; > + u32 rx_rfr; > + int xfer_mode; Can this be an enum? > + bool port_setup; Maybe just 'setup'? Port is in the type already. > + int (*handle_rx)(struct uart_port *uport, > + u32 rx_bytes, bool drop_rx); s/rx_bytes/bytes/ s/drop_rx/drop/ > + unsigned int xmit_size; > + unsigned int cur_baud; s/cur// > + unsigned int tx_bytes_pw; > + unsigned int rx_bytes_pw; > +}; > + > +static const struct uart_ops qcom_geni_serial_pops; > +static struct uart_driver qcom_geni_console_driver; > +static int handle_rx_console(struct uart_port *uport, > + u32 rx_bytes, bool drop_rx); s/rx_bytes/bytes/ s/drop_rx/drop/ > +static unsigned int qcom_geni_serial_tx_empty(struct uart_port *port); > +static bool qcom_geni_serial_poll_bit(struct uart_port *uport, > +