Re: [PATCH v6 2/5] soc: qcom: Add GENI based QUP Wrapper driver

[Evan Green]

On Fri, Mar 30, 2018 at 10:08 AM Karthikeyan Ramasubramanian <
krama...@codeaurora.org> wrote:

> This driver manages the Generic Interface (GENI) firmware based Qualcomm
> Universal Peripheral (QUP) Wrapper. GENI based QUP is the next generation
> programmable module composed of multiple Serial Engines (SE) and supports
> a wide range of serial interfaces like UART, SPI, I2C, I3C, etc. This
> driver also enables managing the serial interface independent aspects of
> Serial Engines.

> Signed-off-by: Karthikeyan Ramasubramanian <krama...@codeaurora.org>
> Signed-off-by: Sagar Dharia <sdha...@codeaurora.org>
> Signed-off-by: Girish Mahadevan <giri...@codeaurora.org>
> ---
>   drivers/soc/qcom/Kconfig|   9 +
>   drivers/soc/qcom/Makefile   |   1 +
>   drivers/soc/qcom/qcom-geni-se.c | 748

>   include/linux/qcom-geni-se.h| 425 +++
>   4 files changed, 1183 insertions(+)
>   create mode 100644 drivers/soc/qcom/qcom-geni-se.c
>   create mode 100644 include/linux/qcom-geni-se.h


Reviewed-by: Evan Green <evgr...@chromium.org>
--
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 v4 4/6] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP

[Evan Green]

On Tue, Mar 20, 2018 at 4:44 PM Karthik Ramasubramanian <
krama...@codeaurora.org> wrote:



> On 3/20/2018 12:39 PM, Evan Green wrote:
> > Hi Karthik,
> >
> > On Wed, Mar 14, 2018 at 4:59 PM Karthikeyan Ramasubramanian <
> > krama...@codeaurora.org> wrote:
> >
> >> +
> >> +static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> >> +   int offset, int 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();
> >
> > These barriers sprinkled around everywhere are new. Did
> > you find that you needed them for something? In this case, the
> > readl_poll_timeout_atomic should already have a read barrier (nor do you
> > probably care about read reordering, right?) Perhaps this should
> > only be a mmiowb rather than a full barrier, because you really just
want
> > to say "make sure all my old writes got out to hardware before spinning"
> These barriers have been there from v3. Regarding this barrier - since
> readl_poll_timeout_atomic has a read barrier, this one can be converted
> to just write barrier.

Thanks. I must have missed them in v3. Is that mmiowb that would go there,
or wmb? I'm unsure.

> >
> >> +
> >> +   if (uport->private_data) {
> >> +   port = to_dev_port(uport, uport);
> >> +   baud = port->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;
> >> +   }
> >> +
> >> +   return !readl_poll_timeout_atomic(uport->membase + offset, reg,
> >> +(bool)(reg & field) == set, 10, timeout_us);
> >> +}
> > [...]
> >> +
> >> +static void qcom_geni_serial_start_tx(struct uart_port *uport)
> >> +{
> >> +   u32 irq_en;
> >> +   struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> >> +   u32 status;
> >> +
> >> +   if (port->xfer_mode == GENI_SE_FIFO) {
> >> +   status = readl_relaxed(uport->membase +
SE_GENI_STATUS);
> >> +   if (status & M_GENI_CMD_ACTIVE)
> >> +   return;
> >> +
> >> +   if (!qcom_geni_serial_tx_empty(uport))
> >> +   return;
> >> +
> >> +   /*
> >> +* Ensure writing to IRQ_EN & watermark registers are
not
> >> +* re-ordered before checking the status of the Serial
> >> +* Engine and TX FIFO
> >> +*/
> >> +   mb();
> >
> > Here's another one. You should just be able to use a regular readl when
> > reading SE_GENI_STATUS and remove this barrier, since readl has an rmb
> > which orders your read of M_IRQ_EN below. I don't think you need to
worry
> > about the writes below going above the read above, since there's logic
in
> > between that needs the result of the read. Maybe that also saves you in
the
> > read case, too. Either way, this barrier seems heavy handed.
> Write to the watermark register does not have any dependency on the
> reads. Hence it can be re-ordered. But writing to that register alone
> without enabling the watermark interrupt will not have any impact. From
> that perspective, using readl while checking the GENI_STATUS is
> sufficient to maintain the required order. I will put a comment
> regarding the use of readl so that the reason is not lost.

Yes, I see what you mean, and without the branch I'd agree. My thinking
though was that you have a branch between the read and writes. So to
determine whether or not to even execute the writes, you must have
successfully evaluated the read, since program flow depends on it. I will
admit this is where my barrier knowledge gets fuzzy. Can speculative
execution perform register writes? (ie if that branch was guessed wrong
before the 

Re: [PATCH v4 4/6] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP

[Evan Green]

Hi Karthik,

On Wed, Mar 14, 2018 at 4:59 PM Karthikeyan Ramasubramanian <
krama...@codeaurora.org> wrote:

> +
> +static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> +   int offset, int 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();

These barriers sprinkled around everywhere are new. Did
you find that you needed them for something? In this case, the
readl_poll_timeout_atomic should already have a read barrier (nor do you
probably care about read reordering, right?) Perhaps this should
only be a mmiowb rather than a full barrier, because you really just want
to say "make sure all my old writes got out to hardware before spinning"

> +
> +   if (uport->private_data) {
> +   port = to_dev_port(uport, uport);
> +   baud = port->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;
> +   }
> +
> +   return !readl_poll_timeout_atomic(uport->membase + offset, reg,
> +(bool)(reg & field) == set, 10, timeout_us);
> +}
[...]
> +
> +static void qcom_geni_serial_start_tx(struct uart_port *uport)
> +{
> +   u32 irq_en;
> +   struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +   u32 status;
> +
> +   if (port->xfer_mode == GENI_SE_FIFO) {
> +   status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> +   if (status & M_GENI_CMD_ACTIVE)
> +   return;
> +
> +   if (!qcom_geni_serial_tx_empty(uport))
> +   return;
> +
> +   /*
> +* Ensure writing to IRQ_EN & watermark registers are not
> +* re-ordered before checking the status of the Serial
> +* Engine and TX FIFO
> +*/
> +   mb();

Here's another one. You should just be able to use a regular readl when
reading SE_GENI_STATUS and remove this barrier, since readl has an rmb
which orders your read of M_IRQ_EN below. I don't think you need to worry
about the writes below going above the read above, since there's logic in
between that needs the result of the read. Maybe that also saves you in the
read case, too. Either way, this barrier seems heavy handed.

> +
> +   irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> +   irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
> +
> +   writel_relaxed(port->tx_wm, uport->membase +
> +   SE_GENI_TX_WATERMARK_REG);
> +   writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> +   }
> +}
> +
> +static void qcom_geni_serial_stop_tx(struct uart_port *uport)
> +{
> +   u32 irq_en;
> +   u32 status;
> +   struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> +   irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> +   irq_en &= ~M_CMD_DONE_EN;
> +   if (port->xfer_mode == GENI_SE_FIFO) {
> +   irq_en &= ~M_TX_FIFO_WATERMARK_EN;
> +   writel_relaxed(0, uport->membase +
> +SE_GENI_TX_WATERMARK_REG);
> +   }
> +   port->xmit_size = 0;
> +   writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> +   status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> +   /* Possible stop tx is called multiple times. */
> +   if (!(status & M_GENI_CMD_ACTIVE))
> +   return;
> +
> +   /*
> +* Ensure cancel command write is not re-ordered before checking
> +* the status of the Primary Sequencer.
> +*/
> +   mb();

I don't see how what's stated in your comment could happen, as that would
be a logic error. This barrier seems unneeded to me.

> +
> +   geni_se_cancel_m_cmd(>se);
> +   if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> +   M_CMD_CANCEL_EN, true)) {
> +   geni_se_abort_m_cmd(>se);
> +   qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> +   M_CMD_ABORT_EN, true);
> +   writel_relaxed(M_CMD_ABORT_EN, uport->membase +
> +
SE_GENI_M_IRQ_CLEAR);
> +   }
> +   writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
SE_GENI_M_IRQ_CLEAR);
> +}
> +
> +static void qcom_geni_serial_start_rx(struct uart_port *uport)
> +{
> 

Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP

[Evan Green]

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 littl

Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP

[Evan Green]

Hello Karthik,

On Tue, Feb 27, 2018 at 5:38 PM, Karthikeyan Ramasubramanian
 wrote:
> 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: [v2,3/7] soc: qcom: Add GENI based QUP Wrapper driver

[Evan Green]

On Fri, Jan 12, 2018 at 5:05 PM, Karthikeyan Ramasubramanian
 wrote:
> This driver manages the Generic Interface (GENI) firmware based Qualcomm
> Universal Peripheral (QUP) Wrapper. GENI based QUP is the next generation
> programmable module composed of multiple Serial Engines (SE) and supports
> a wide range of serial interfaces like UART, SPI, I2C, I3C, etc. This
> driver also enables managing the serial interface independent aspects of
> Serial Engines.
>
> Signed-off-by: Karthikeyan Ramasubramanian 
> Signed-off-by: Sagar Dharia 
> Signed-off-by: Girish Mahadevan 
> ---
>  drivers/soc/qcom/Kconfig|8 +
>  drivers/soc/qcom/Makefile   |1 +
>  drivers/soc/qcom/qcom-geni-se.c | 1016 
> +++
>  include/linux/qcom-geni-se.h|  807 +++
>  4 files changed, 1832 insertions(+)
>  create mode 100644 drivers/soc/qcom/qcom-geni-se.c
>  create mode 100644 include/linux/qcom-geni-se.h

I'm a newbie here, just starting to get up to speed. Please forgive
any indiscretions. But I am interested in this driver, so I'm throwing
in my minor comments.


> +/**
> + * geni_se_setup_m_cmd() - Setup the primary sequencer
> + * @base:  Base address of the serial engine's register block.
> + * @cmd:   Command/Operation to setup in the primary sequencer.
> + * @params:Parameter for the sequencer command.
> + *
> + * This function is used to configure the primary sequencer with the
> + * command and its assoicated parameters.
> + */
> +void geni_se_setup_m_cmd(void __iomem *base, u32 cmd, u32 params);
> +
> +/**
> + * geni_se_setup_s_cmd() - Setup the secondary sequencer
> + * @base:  Base address of the serial engine's register block.
> + * @cmd:   Command/Operation to setup in the secondary sequencer.
> + * @params:Parameter for the sequencer command.
> + *
> + * This function is used to configure the secondary sequencer with the
> + * command and its assoicated parameters.
> + */
> +void geni_se_setup_s_cmd(void __iomem *base, u32 cmd, u32 params);
> +

s/assoicated/associated/ (twice)


> +/**
> + * geni_se_tx_dma_prep() - Prepare the Serial Engine for TX DMA transfer
> + * @wrapper_dev:   QUP Wrapper Device to which the TX buffer is mapped.
> + * @base:  Base address of the SE register block.
> + * @tx_buf:Pointer to the TX buffer.
> + * @tx_len:Length of the TX buffer.
> + * @tx_dma:Pointer to store the mapped DMA address.
> + *
> + * This function is used to prepare the buffers for DMA TX.
> + *
> + * Return: 0 on success, standard Linux error codes on error/failure.
> + */
> +int geni_se_tx_dma_prep(struct device *wrapper_dev, void __iomem *base,
> +   void *tx_buf, int tx_len, dma_addr_t *tx_dma)
> +{
> +   int ret;
> +
> +   if (unlikely(!wrapper_dev || !base || !tx_buf || !tx_len || !tx_dma))
> +   return -EINVAL;
> +
> +   ret = geni_se_map_buf(wrapper_dev, tx_dma, tx_buf, tx_len,
> +   DMA_TO_DEVICE);
> +   if (ret)
> +   return ret;
> +
> +   geni_write_reg(7, base, SE_DMA_TX_IRQ_EN_SET);
> +   geni_write_reg((u32)(*tx_dma), base, SE_DMA_TX_PTR_L);
> +   geni_write_reg((u32)((*tx_dma) >> 32), base, SE_DMA_TX_PTR_H);
> +   geni_write_reg(1, base, SE_DMA_TX_ATTR);

Bjorn touched on this, but as someone trying to understand what's
going on it would be great to see #defines for the magic values in
here (7 and 1)

> +void geni_se_get_packing_config(int bpw, int pack_words, bool msb_to_lsb,
> +   unsigned long *cfg0, unsigned long *cfg1)
> +{
> +   u32 cfg[4] = {0};
> +   int len;
> +   int temp_bpw = bpw;
> +   int idx_start = (msb_to_lsb ? (bpw - 1) : 0);
> +   int idx = idx_start;
> +   int idx_delta = (msb_to_lsb ? -BITS_PER_BYTE : BITS_PER_BYTE);
> +   int ceil_bpw = ((bpw & (BITS_PER_BYTE - 1)) ?
> +   ((bpw & ~(BITS_PER_BYTE - 1)) + BITS_PER_BYTE) : bpw);
> +   int iter = (ceil_bpw * pack_words) >> 3;

Is the shift by 3 here really meant to divide by BITS_PER_BYTE? It
might be clearer to divide by BITS_PER_BYTE and let the compiler
convert that into a shift.

> +   int i;
> +
> +   if (unlikely(iter <= 0 || iter > 4)) {
> +   *cfg0 = 0;
> +   *cfg1 = 0;
> +   return;
> +   }
> +
> +   for (i = 0; i < iter; i++) {
> +   len = (temp_bpw < BITS_PER_BYTE) ?
> +   (temp_bpw - 1) : BITS_PER_BYTE - 1;
> +   cfg[i] = ((idx << 5) | (msb_to_lsb << 4) | (len << 1));

...more magic numbers here. These are register bitfields? It would be
great to get the field shifts defined.

> +   idx = ((temp_bpw - BITS_PER_BYTE) <= 0) ?
> +   ((i + 1) * 

Re: [v2, 7/7] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP

[Evan Green]

On Fri, Jan 12, 2018 at 5:05 PM, Karthikeyan Ramasubramanian
 wrote:
> +static int get_clk_cfg(unsigned long clk_freq, unsigned long *ser_clk)
> +{
> +   unsigned long root_freq[] = {7372800, 14745600, 1920, 29491200,
> +   3200, 4800, 6400, 8000, 9600, 1};

This table should be static const, right?
--
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: [v2, 5/7] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller

[Evan Green]

On Fri, Jan 12, 2018 at 5:05 PM, Karthikeyan Ramasubramanian
 wrote:
> This bus driver supports the GENI based i2c hardware controller in the
> Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable
> module supporting a wide range of serial interfaces including I2C. The
> driver supports FIFO mode and DMA mode of transfer and switches modes
> dynamically depending on the size of the transfer.
>
> Signed-off-by: Karthikeyan Ramasubramanian 
> Signed-off-by: Sagar Dharia 
> Signed-off-by: Girish Mahadevan 
> ---
>  drivers/i2c/busses/Kconfig |  10 +
>  drivers/i2c/busses/Makefile|   1 +
>  drivers/i2c/busses/i2c-qcom-geni.c | 656 
> +
>  3 files changed, 667 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-qcom-geni.c

Newbie again, throwing in my two cents.

> +static inline void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c, int dfs)
> +{
> +   struct geni_i2c_clk_fld *itr = geni_i2c_clk_map + gi2c->clk_fld_idx;
> +
> +   geni_write_reg(dfs, gi2c->base, SE_GENI_CLK_SEL);
> +
> +   geni_write_reg((itr->clk_div << 4) | 1, gi2c->base, 
> GENI_SER_M_CLK_CFG);
> +   geni_write_reg(((itr->t_high << 20) | (itr->t_low << 10) |
> +   itr->t_cycle), gi2c->base, SE_I2C_SCL_COUNTERS);

It would be great to get these register field shifts defined, as
they're otherwise fairly opaque.

> +static irqreturn_t geni_i2c_irq(int irq, void *dev)
> +{
> +   struct geni_i2c_dev *gi2c = dev;
> +   int i, j;
> +   u32 m_stat = readl_relaxed(gi2c->base + SE_GENI_M_IRQ_STATUS);
> +   u32 rx_st = readl_relaxed(gi2c->base + SE_GENI_RX_FIFO_STATUS);
> +   u32 dm_tx_st = readl_relaxed(gi2c->base + SE_DMA_TX_IRQ_STAT);
> +   u32 dm_rx_st = readl_relaxed(gi2c->base + SE_DMA_RX_IRQ_STAT);
> +   u32 dma = readl_relaxed(gi2c->base + SE_GENI_DMA_MODE_EN);
> +   struct i2c_msg *cur = gi2c->cur;
> +
> +   if (!cur || (m_stat & M_CMD_FAILURE_EN) ||
> +   (dm_rx_st & (DM_I2C_CB_ERR)) ||
> +   (m_stat & M_CMD_ABORT_EN)) {
> +
> +   if (m_stat & M_GP_IRQ_1_EN)
> +   geni_i2c_err(gi2c, I2C_NACK);
> +   if (m_stat & M_GP_IRQ_3_EN)
> +   geni_i2c_err(gi2c, I2C_BUS_PROTO);
> +   if (m_stat & M_GP_IRQ_4_EN)
> +   geni_i2c_err(gi2c, I2C_ARB_LOST);
> +   if (m_stat & M_CMD_OVERRUN_EN)
> +   geni_i2c_err(gi2c, GENI_OVERRUN);
> +   if (m_stat & M_ILLEGAL_CMD_EN)
> +   geni_i2c_err(gi2c, GENI_ILLEGAL_CMD);
> +   if (m_stat & M_CMD_ABORT_EN)
> +   geni_i2c_err(gi2c, GENI_ABORT_DONE);
> +   if (m_stat & M_GP_IRQ_0_EN)
> +   geni_i2c_err(gi2c, GP_IRQ0);
> +
> +   if (!dma)
> +   writel_relaxed(0, (gi2c->base +
> +  SE_GENI_TX_WATERMARK_REG));

The writes to the TX_WATERMARK_REG here and a little further down in
the irq handler stick out to me. A comment as to why poking at the
watermark register is necessary here would be very helpful.

-Evan
--
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