* [PATCH] modify one dead link
From: Dongliang Mu @ 2018-03-20 14:54 UTC (permalink / raw)
To: corbet, yamada.masahiro, slyfox, tony.luck, bjorn.forsman,
mudongliangabcd
Cc: linux-doc, linux-kernel
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
Documentation/ia64/xen.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/ia64/xen.txt b/Documentation/ia64/xen.txt
index a12c74ce2773..aea4bb94aa59 100644
--- a/Documentation/ia64/xen.txt
+++ b/Documentation/ia64/xen.txt
@@ -26,7 +26,7 @@ Getting and Building Xen and Dom0
DomainU OS : RHEL5
1. Download source
- # hg clone http://xenbits.xensource.com/ext/ia64/xen-unstable.hg
+ # hg clone http://xenbits.xensource.com/ext/ia64/xen-unstable
# cd xen-unstable.hg
# hg clone http://xenbits.xensource.com/ext/ia64/linux-2.6.18-xen.hg
--
2.16.2
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v4 4/6] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
From: Stephen Boyd @ 2018-03-20 15:37 UTC (permalink / raw)
To: Karthikeyan Ramasubramanian, andy.gross, corbet, david.brown,
gregkh, mark.rutland, robh+dt, wsa
Cc: Karthikeyan Ramasubramanian, linux-doc, linux-arm-msm, devicetree,
linux-i2c, linux-serial, jslaby, evgreen, acourbot,
Girish Mahadevan, Sagar Dharia, Doug Anderson
In-Reply-To: <1521071931-9294-5-git-send-email-kramasub@codeaurora.org>
Quoting Karthikeyan Ramasubramanian (2018-03-14 16:58:49)
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> new file mode 100644
> index 0000000..1442777
> --- /dev/null
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -0,0 +1,1158 @@
> +
> +#ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
> +static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch)
> +{
> + writel_relaxed(ch, uport->membase + SE_GENI_TX_FIFOn);
Does this expect the whole word to have data to write? Or does the FIFO
output a character followed by three NUL bytes each time it gets
written? The way that uart_console_write() works is to take each
character a byte at a time, put it into an int (so extend that byte with
zero) and then pass it to the putchar function. I would expect that at
this point the hardware sees the single character and then 3 NULs enter
the FIFO each time.
For previous MSM uarts I had to handle this oddity by packing the words
into the fifo four at a time. You may need to do the same here.
> +}
> +
> +static void
> +__qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
> + unsigned int count)
> +{
> + int i;
> + u32 bytes_to_send = count;
> +
> + for (i = 0; i < count; i++) {
> + if (s[i] == '\n')
Can you add a comment that uart_console_write() adds a carriage return
for each newline?
> + bytes_to_send++;
> + }
> +
> + writel_relaxed(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
> + qcom_geni_serial_setup_tx(uport, bytes_to_send);
> + for (i = 0; i < count; ) {
> + 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.
> + */
> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_TX_FIFO_WATERMARK_EN, true))
> + break;
> + chars_to_write = min_t(size_t, (size_t)(count - i), avail / 2);
The _t part of min_t should do the casting already, so we can drop the
cast on count - i?
> + uart_console_write(uport, s + i, chars_to_write,
> + qcom_geni_serial_wr_char);
> + writel_relaxed(M_TX_FIFO_WATERMARK_EN, uport->membase +
> + SE_GENI_M_IRQ_CLEAR);
> + i += chars_to_write;
> + }
> + qcom_geni_serial_poll_tx_done(uport);
> +}
> +
> +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 = &port->uport;
> + if (oops_in_progress)
> + locked = spin_trylock_irqsave(&uport->lock, flags);
> + else
> + spin_lock_irqsave(&uport->lock, flags);
> +
> + /* Cancel the current write to log the fault */
> + if (!locked) {
> + geni_se_cancel_m_cmd(&port->se);
> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_CMD_CANCEL_EN, true)) {
> + geni_se_abort_m_cmd(&port->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);
> + }
> +
> + __qcom_geni_serial_console_write(uport, s, count);
> + if (locked)
> + spin_unlock_irqrestore(&uport->lock, flags);
> +}
Can you also support the OF_EARLYCON_DECLARE method of console writing
so we can get an early printk style debug console?
> +
> +static int handle_rx_console(struct uart_port *uport, u32 bytes, bool drop)
> +{
> + u32 i;
> + unsigned char buf[sizeof(u32)];
> + struct tty_port *tport;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> + tport = &uport->state->port;
> + for (i = 0; i < bytes; ) {
> + int c;
> + int chunk = min_t(int, bytes - i, port->rx_bytes_pw);
> +
> + ioread32_rep(uport->membase + SE_GENI_RX_FIFOn, buf, 1);
> + i += chunk;
> + if (drop)
> + continue;
> +
> + for (c = 0; c < chunk; c++) {
> + int sysrq;
> +
> + uport->icount.rx++;
> + if (port->brk && buf[c] == 0) {
> + port->brk = false;
> + if (uart_handle_break(uport))
> + continue;
Thanks!
> + }
> +
> + sysrq = uart_handle_sysrq_char(uport, buf[c]);
> + if (!sysrq)
> + tty_insert_flip_char(tport, buf[c], TTY_NORMAL);
> + }
> + }
> + if (!drop)
> + tty_flip_buffer_push(tport);
> + return 0;
> +}
[...]
> +
> +static void qcom_geni_serial_handle_tx(struct uart_port *uport)
> +{
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> + struct circ_buf *xmit = &uport->state->xmit;
> + size_t avail;
> + size_t remaining;
> + int i;
> + 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 - 1);
> + if (chunk > (UART_XMIT_SIZE - tail))
> + chunk = UART_XMIT_SIZE - tail;
> + if (chunk > avail)
> + chunk = avail;
chunk = min3(chunk, UART_XMIT_SIZE - tail, avail);
> +
> + if (!chunk)
> + goto out_write_wakeup;
> +
> + qcom_geni_serial_setup_tx(uport, chunk);
> +
> + remaining = chunk;
> + for (i = 0; i < chunk; ) {
> + unsigned int tx_bytes;
> + unsigned int buf = 0;
Make buf into a u8 array of 4?
> + int c;
> +
> + tx_bytes = min_t(size_t, remaining, (size_t)port->tx_bytes_pw);
> + for (c = 0; c < tx_bytes ; c++)
> + buf |= (xmit->buf[tail + c] << (c * BITS_PER_BYTE));
And then just put buf[c] = xmit->buf[tail + c] here?
> +
> + writel_relaxed(buf, uport->membase + SE_GENI_TX_FIFOn);
This also needs to be an iowrite32_rep(uport->membase, buf, 1) for
endian reasons.
> +
> + i += tx_bytes;
> + tail = (tail + tx_bytes) & (UART_XMIT_SIZE - 1);
> + uport->icount.tx += tx_bytes;
> + remaining -= tx_bytes;
> + }
> + qcom_geni_serial_poll_tx_done(uport);
> + port->xmit_size += chunk;
> +out_write_wakeup:
> + uart_write_wakeup(uport);
> +}
> +
> +static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
> +{
> + unsigned int m_irq_status;
> + unsigned int s_irq_status;
> + struct uart_port *uport = dev;
> + unsigned long flags;
> + unsigned int m_irq_en;
> + bool drop_rx = false;
> + struct tty_port *tport = &uport->state->port;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> + if (uport->suspended)
> + return IRQ_HANDLED;
Is it a spurious IRQ if this happens? Return IRQ_NONE instead?
> +
> + spin_lock_irqsave(&uport->lock, flags);
> + m_irq_status = readl_relaxed(uport->membase + SE_GENI_M_IRQ_STATUS);
> + s_irq_status = readl_relaxed(uport->membase + SE_GENI_S_IRQ_STATUS);
> + m_irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> + writel_relaxed(m_irq_status, uport->membase + SE_GENI_M_IRQ_CLEAR);
> + writel_relaxed(s_irq_status, uport->membase + SE_GENI_S_IRQ_CLEAR);
> +
> + if (WARN_ON(m_irq_status & M_ILLEGAL_CMD_EN))
> + goto out_unlock;
> +
> + if (s_irq_status & S_RX_FIFO_WR_ERR_EN) {
> + uport->icount.overrun++;
> + tty_insert_flip_char(tport, 0, TTY_OVERRUN);
> + }
> +
> + 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++;
Maybe move this stat accounting to the place where brk is handled?
> + port->brk = true;
> + }
> +
> + if (s_irq_status & S_RX_FIFO_WATERMARK_EN ||
> + s_irq_status & S_RX_FIFO_LAST_EN)
> + qcom_geni_serial_handle_rx(uport, drop_rx);
> +
> +out_unlock:
> + spin_unlock_irqrestore(&uport->lock, flags);
> + return IRQ_HANDLED;
> +}
> +
> +static int get_tx_fifo_size(struct qcom_geni_serial_port *port)
> +{
> + struct uart_port *uport;
> +
> + if (!port)
> + return -ENODEV;
This happens?
> +
> + uport = &port->uport;
> + port->tx_fifo_depth = geni_se_get_tx_fifo_depth(&port->se);
> + port->tx_fifo_width = geni_se_get_tx_fifo_width(&port->se);
> + port->rx_fifo_depth = geni_se_get_rx_fifo_depth(&port->se);
> + uport->fifosize =
> + (port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
> + return 0;
> +}
> +
[...]
> +
> +#ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
> +static int __init qcom_geni_console_setup(struct console *co, char *options)
> +{
> + struct uart_port *uport;
> + struct qcom_geni_serial_port *port;
> + int baud;
> + int bits = 8;
> + int parity = 'n';
> + int flow = 'n';
> +
> + if (co->index >= GENI_UART_CONS_PORTS || co->index < 0)
> + return -ENXIO;
> +
> + port = get_port_from_line(co->index);
> + if (IS_ERR(port)) {
> + pr_err("Invalid line %d(%d)\n", co->index, (int)PTR_ERR(port));
Use %ld to avoid casting the error value? Or just don't print it at all
because it doesn't really help the end user.
> + return PTR_ERR(port);
> + }
> +
> + uport = &port->uport;
> +
> + if (unlikely(!uport->membase))
> + return -ENXIO;
> +
> + if (geni_se_resources_on(&port->se)) {
> + dev_err(port->se.dev, "Error turning on resources\n");
> + return -ENXIO;
> + }
> +
> + if (unlikely(geni_se_read_proto(&port->se) != GENI_SE_UART)) {
> + geni_se_resources_off(&port->se);
> + return -ENXIO;
> + }
> +
> + if (!port->setup) {
> + port->tx_bytes_pw = 1;
> + port->rx_bytes_pw = RX_BYTES_PW;
> + qcom_geni_serial_stop_rx(uport);
> + qcom_geni_serial_port_setup(uport);
> + }
> +
> + if (options)
> + uart_parse_options(options, &baud, &parity, &bits, &flow);
> +
> + return uart_set_options(uport, co, baud, parity, bits, flow);
> +}
[..]
> +
> +static int qcom_geni_serial_probe(struct platform_device *pdev)
> +{
> + int ret = 0;
> + int line = -1;
> + struct qcom_geni_serial_port *port;
> + struct uart_port *uport;
> + struct resource *res;
> +
> + if (pdev->dev.of_node)
> + line = of_alias_get_id(pdev->dev.of_node, "serial");
> + else
> + line = pdev->id;
Do we need to support the non-alias method?
> +
> + if (line < 0 || line >= GENI_UART_CONS_PORTS)
> + return -ENXIO;
> + port = get_port_from_line(line);
> + if (IS_ERR(port)) {
> + ret = PTR_ERR(port);
> + dev_err(&pdev->dev, "Invalid line %d(%d)\n", line, ret);
> + return ret;
> + }
> +
> + uport = &port->uport;
> + /* Don't allow 2 drivers to access the same port */
> + if (uport->private_data)
> + return -ENODEV;
> +
> + uport->dev = &pdev->dev;
> + port->se.dev = &pdev->dev;
> + port->se.wrapper = dev_get_drvdata(pdev->dev.parent);
> + port->se.clk = devm_clk_get(&pdev->dev, "se");
> + if (IS_ERR(port->se.clk)) {
> + ret = PTR_ERR(port->se.clk);
> + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
> + return ret;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (IS_ERR(res))
> + return PTR_ERR(res);
> + uport->mapbase = res->start;
> +
> + port->tx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> + port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> + port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
> +
> + uport->irq = platform_get_irq(pdev, 0);
> + if (uport->irq < 0) {
> + dev_err(&pdev->dev, "Failed to get IRQ %d\n", uport->irq);
> + return uport->irq;
> + }
> +
> + uport->private_data = &qcom_geni_console_driver;
> + platform_set_drvdata(pdev, port);
> + port->handle_rx = handle_rx_console;
> + port->setup = false;
This would be set to false already?
> + return uart_add_one_port(&qcom_geni_console_driver, uport);
> +}
> +
> +static int qcom_geni_serial_remove(struct platform_device *pdev)
> +{
> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> + struct uart_driver *drv = port->uport.private_data;
> +
> + uart_remove_one_port(drv, &port->uport);
> + return 0;
> +}
> +
> +static int __maybe_unused qcom_geni_serial_sys_suspend_noirq(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> + struct uart_port *uport = &port->uport;
> +
> + uart_suspend_port(uport->private_data, uport);
> + return 0;
> +}
> +
> +static int __maybe_unused qcom_geni_serial_sys_resume_noirq(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> + struct uart_port *uport = &port->uport;
> +
> + if (console_suspend_enabled && uport->suspended) {
> + uart_resume_port(uport->private_data, uport);
> + disable_irq(uport->irq);
I missed the enable_irq() part. Is this still necessary?
> + }
> + return 0;
> +}
> +
> +static int __init qcom_geni_serial_init(void)
> +{
> + int ret;
> +
> + qcom_geni_console_port.uport.iotype = UPIO_MEM;
> + qcom_geni_console_port.uport.ops = &qcom_geni_console_pops;
> + qcom_geni_console_port.uport.flags = UPF_BOOT_AUTOCONF;
> + qcom_geni_console_port.uport.line = 0;
Why can't this be done statically?
> +
> + ret = console_register(&qcom_geni_console_driver);
> + if (ret)
> + return ret;
> +
> + ret = platform_driver_register(&qcom_geni_serial_platform_driver);
> + if (ret)
> + console_unregister(&qcom_geni_console_driver);
> + return ret;
> +}
> +module_init(qcom_geni_serial_init);
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 1/6] dt-bindings: soc: qcom: Add device tree binding for GENI SE
From: Stephen Boyd @ 2018-03-20 15:39 UTC (permalink / raw)
To: Karthikeyan Ramasubramanian, andy.gross, corbet, david.brown,
gregkh, mark.rutland, robh+dt, wsa
Cc: Karthikeyan Ramasubramanian, linux-doc, linux-arm-msm, devicetree,
linux-i2c, linux-serial, jslaby, evgreen, acourbot, Sagar Dharia,
Girish Mahadevan
In-Reply-To: <1521071931-9294-2-git-send-email-kramasub@codeaurora.org>
Quoting Karthikeyan Ramasubramanian (2018-03-14 16:58:46)
> Add device tree binding support for the QCOM GENI SE driver.
>
> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
Assuming Rob's comment is addressed:
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Non-DoD Source] Re: [PATCH v3 14/15] selinux: allow setxattr on rootfs so initramfs code can set them
From: Stephen Smalley @ 2018-03-20 16:33 UTC (permalink / raw)
To: Victor Kamensky
Cc: Taras Kondratiuk, H. Peter Anvin, Al Viro, Arnd Bergmann,
Rob Landley, Mimi Zohar, Jonathan Corbet, James McMechan,
initramfs, linux-doc, linux-kernel, linux-security-module,
xe-linux-external, Paul Moore, Eric Paris
In-Reply-To: <alpine.LRH.2.00.1803101848580.24419@sjc-ads-6991.cisco.com>
On 03/10/2018 10:07 PM, Victor Kamensky wrote:
>
>
> On Tue, 20 Feb 2018, Stephen Smalley wrote:
>
>> On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
>>> From: Victor Kamensky <kamensky@cisco.com>
>>>
>>> initramfs code supporting extended cpio format have ability to
>>> fill extended attributes from cpio archive, but if SELinux enabled
>>> and security server is not initialized yet, selinux callback would
>>> refuse setxattr made by initramfs code.
>>>
>>> Solution enable SBLABEL_MNT on rootfs even if secrurity server is
>>> not initialized yet.
>>
>> What if we were to instead skip the SBLABEL_MNT check in
>> selinux_inode_setxattr() if !ss_initialized? Not dependent on
>> filesystem type.
>
> Stephen, thank you for looking into this. Sorry, for dealyed reponse -
> I needed to find time to require context about these changes.
>
> As you suggested I've tried this and it works:
>
>> From 6bf35bd055fdb12e94f3d5188eccfdbaa30dbcf4 Mon Sep 17 00:00:00 2001
> From: Victor Kamensky <kamensky@cisco.com>
> Date: Fri, 9 Mar 2018 23:01:20 -0800
> Subject: [PATCH 1/2] selinux: allow setxattr on file systems if policy is not
> loaded
>
> initramfs code supporting extended cpio format have ability to
> fill extended attributes from cpio archive, but if SELinux enabled
> and security server is not initialized yet, selinux callback would
> refuse setxattr made by initramfs code because file system is not
> yet marked as one that support labeling (SBLABEL_MNT flag).
>
> Solution do not refuse setxattr even if SBLABEL_MNT is not set
> for file systems when policy is not loaded yet.
>
> Signed-off-by: Victor Kamensky <kamensky@cisco.com>
> ---
> security/selinux/hooks.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 819fd68..31303ed 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3120,7 +3120,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
> return selinux_inode_setotherxattr(dentry, name);
>
> sbsec = inode->i_sb->s_security;
> - if (!(sbsec->flags & SBLABEL_MNT))
> + if (!(sbsec->flags & SBLABEL_MNT) && ss_initialized)
> return -EOPNOTSUPP;
>
> if (!inode_owner_or_capable(inode))
I favor the first option.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Non-DoD Source] Re: [PATCH v3 15/15] selinux: delay sid population for rootfs till init is complete
From: Stephen Smalley @ 2018-03-20 16:30 UTC (permalink / raw)
To: Victor Kamensky
Cc: Taras Kondratiuk, H. Peter Anvin, Al Viro, Arnd Bergmann,
Rob Landley, Mimi Zohar, Jonathan Corbet, James McMechan,
initramfs, linux-doc, linux-kernel, linux-security-module,
xe-linux-external, Paul Moore, Eric Paris
In-Reply-To: <alpine.LRH.2.00.1803101900250.24419@sjc-ads-6991.cisco.com>
On 03/10/2018 10:08 PM, Victor Kamensky wrote:
>
>
> On Tue, 20 Feb 2018, Stephen Smalley wrote:
>
>> On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
>>> From: Victor Kamensky <kamensky@cisco.com>
>>>
>>> With initramfs cpio format that supports extended attributes
>>> we need to skip sid population on sys_lsetxattr call from
>>> initramfs for rootfs if security server is not initialized yet.
>>>
>>> Otherwise callback in selinux_inode_post_setxattr will try to
>>> translate give security.selinux label into sid context and since
>>> security server is not available yet inode will receive default
>>> sid (typically kernel_t). Note that in the same time proper
>>> label will be stored in inode xattrs. Later, since inode sid
>>> would be already populated system will never look back at
>>> actual xattrs. But if we skip sid population for rootfs and
>>> we have policy that direct use of xattrs for rootfs, proper
>>> sid will be filled in from extended attributes one node is
>>> accessed and server is initialized.
>>>
>>> Note new DELAYAFTERINIT_MNT super block flag is introduced
>>> to only mark rootfs for such behavior. For other types of
>>> tmpfs original logic is still used.
>>
>> (cc selinux maintainers)
>>
>> Wondering if we shouldn't just do this always, for all filesystem
>> types.
>
> Ok, I think it makes sense. The one that do not support xattrs
> will not reach selinux_inode_post_setxattr anyway. And try
> to cache sid while !ss_initialized is not good idea for any
> filesystem types.
>
>> Also, I think this should likely also be done in
>> selinux_inode_setsecurity() for consistency.
>
> I am not sure that I follow selinux_inode_setsecurity suggestion.
> selinux_inode_setsecurity is about permission check. And
> selinux_inode_post_setxattr deals with processing and setting
> side effects if xattr was "security.selinux", it does not
> matter what happens in selinux_inode_setsecurity if it
> returns access_ok, LSM will still call selinux_inode_post_setxattr
> and we would need to check and not produce any sid caching
> side effects if !ss_initialized.
selinux_inode_setsecurity is the vfs fallback for setting security
attributes when the filesystem/inode does not support setxattr itself,
and is also used by kernfs.
So you need to update both selinux_inode_post_setxattr and selinux_inode_setsecurity
in the same way.
>
> Sitll keeping logic in selinux_inode_post_setxattr, checked
> that the following with much simple code works too:
>
>> From bfc54e4805f3059671417ff2cda1266bc68e18f9 Mon Sep 17 00:00:00 2001
> From: Victor Kamensky <kamensky@cisco.com>
> Date: Fri, 9 Mar 2018 23:06:08 -0800
> Subject: [PATCH 2/2] selinux: delay sid population in setxattr till policy
> loaded
>
> With initramfs cpio format that supports extended attributes
> we need to skip sid population on sys_lsetxattr call from
> initramfs for rootfs if security server is not initialized yet.
>
> Otherwise callback in selinux_inode_post_setxattr will try to
> translate give security.selinux label into sid context and since
> security server is not available yet inode will receive default
> sid (typically kernel_t). Note that in the same time proper
> label will be stored in inode xattrs. Later, since inode sid
> would be already populated system will never look back at
> actual xattrs. But if we skip sid population for rootfs and
> we have policy that direct use of xattrs for rootfs, proper
> sid will be filled in from extended attributes one node is
> accessed and server is initialized.
>
> Signed-off-by: Victor Kamensky <kamensky@cisco.com>
> ---
> security/selinux/hooks.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 31303ed..4c13759 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3197,6 +3197,10 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
> return;
> }
>
> + if (!ss_initialized) {
> + return;
> + }
> +
> rc = security_context_to_sid_force(value, size, &newsid);
> if (rc) {
> printk(KERN_ERR "SELinux: unable to map context to SID"
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 00/16] remove eight obsolete architectures
From: Palmer Dabbelt @ 2018-03-20 17:11 UTC (permalink / raw)
To: Arnd Bergmann
Cc: hare, dhowells, linux-arch, linux-kernel, linux-doc, linux-block,
linux-ide, linux-input, netdev, linux-wireless, linux-pwm,
linux-rtc, linux-spi, linux-usb, dri-devel, linux-fbdev,
linux-watchdog, linux-fsdevel, linux-mm
In-Reply-To: <CAK8P3a01pfvsdM1mR8raU9dA7p4H-jRJz2Y8-+KEY76W_Mukpg@mail.gmail.com>
On Thu, 15 Mar 2018 03:42:25 PDT (-0700), Arnd Bergmann wrote:
> On Thu, Mar 15, 2018 at 10:59 AM, Hannes Reinecke <hare@suse.de> wrote:
>> On 03/15/2018 10:42 AM, David Howells wrote:
>>> Do we have anything left that still implements NOMMU?
>>>
>> RISC-V ?
>> (evil grin :-)
>
> Is anyone producing a chip that includes enough of the Privileged ISA spec
> to have things like system calls, but not the MMU parts?
>
> I thought at least initially the kernel only supports hardware that has a rather
> complete feature set.
We currently do not have a NOMMU port. As far as I know, everyone who's
currently producing RISC-V hardware with enough memory to run Linux has S mode
with paging support. The ISA allows for S mode without paging but there's no
hardware for that -- if you're going to put a DRAM controller on there then
paging seems pretty cheap. You could run a NOMMU port on a system with S-mode
and paging, but With all the superpage stuff I don't think you'll get an
appreciable performance win for any workload running without an MMU so there's
nothing to justify the work (and incompatibility) of a NOMMU port there.
While I think you could implement a NOMMU port on a machine with only M and U
modes (and therefor no address translation at all), I don't know of any MU-only
machines that have enough memory to run Linux (ours have less than 32KiB). A
SBI-free Linux would be a prerequisite for this, but there's some interest in
that outside of a NOMMU port so it might materialize anyway.
Of course, QEMU could probably be tricked into emulating one of these machines
with little to no effort :)... That said, I doubt we'll see a NOMMU port
materialize without some real hardware as it's a lot of work for a QEMU-only
target.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 4/6] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
From: Evan Green @ 2018-03-20 18:39 UTC (permalink / raw)
To: Karthikeyan Ramasubramanian
Cc: Jonathan Corbet, Andy Gross, David Brown, robh+dt, mark.rutland,
wsa, gregkh, linux-doc, linux-arm-msm, devicetree, linux-i2c,
linux-serial, jslaby, acourbot, swboyd, Girish Mahadevan,
Sagar Dharia, Doug Anderson
In-Reply-To: <1521071931-9294-5-git-send-email-kramasub@codeaurora.org>
Hi Karthik,
On Wed, Mar 14, 2018 at 4:59 PM Karthikeyan Ramasubramanian <
kramasub@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 = 20000;
> +
> + /* 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(&port->se);
> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_CMD_CANCEL_EN, true)) {
> + geni_se_abort_m_cmd(&port->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)
> +{
> + u32 irq_en;
> + u32 status;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> + status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> + if (status & S_GENI_CMD_ACTIVE)
> + qcom_geni_serial_stop_rx(uport);
> +
> + /*
> + * Ensure setup command write is not re-ordered before checking
> + * the status of the Secondary Sequencer.
> + */
> + mb();
Also here, I think the reordering you're worried about would mean the CPU
is executing incorrectly.
> +
> + geni_se_setup_s_cmd(&port->se, UART_START_READ, 0);
> +
> + if (port->xfer_mode == GENI_SE_FIFO) {
> + irq_en = readl_relaxed(uport->membase + SE_GENI_S_IRQ_EN);
> + irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN;
> + writel_relaxed(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
> +
> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> + irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> + }
> +}
> +
> +static void qcom_geni_serial_stop_rx(struct uart_port *uport)
> +{
> + u32 irq_en;
> + u32 status;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> + u32 irq_clear = S_CMD_DONE_EN;
> +
> + if (port->xfer_mode == GENI_SE_FIFO) {
> + irq_en = readl_relaxed(uport->membase + SE_GENI_S_IRQ_EN);
> + irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN);
> + writel_relaxed(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
> +
> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> + irq_en &= ~(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> + }
> +
> + status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> + /* Possible stop rx is called multiple times. */
> + if (!(status & S_GENI_CMD_ACTIVE))
> + return;
> +
> + /*
> + * Ensure cancel command write is not re-ordered before checking
> + * the status of the Secondary Sequencer.
> + */
> + mb();
Same deal here.
> +
> + geni_se_cancel_s_cmd(&port->se);
> + qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
> + S_GENI_CMD_CANCEL, false);
> + status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> + writel_relaxed(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
> + if (status & S_GENI_CMD_ACTIVE)
> + qcom_geni_serial_abort_rx(uport);
> +}
> +
Sorry gmail decided to wrap some of the context lines.
-Evan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 5/6] arm64: dts: sdm845: Add serial console support
From: Stephen Boyd @ 2018-03-20 19:39 UTC (permalink / raw)
To: Karthikeyan Ramasubramanian, andy.gross, corbet, david.brown,
gregkh, mark.rutland, robh+dt, wsa
Cc: Rajendra Nayak, linux-doc, linux-arm-msm, devicetree, linux-i2c,
linux-serial, jslaby, evgreen, acourbot
In-Reply-To: <1521071931-9294-6-git-send-email-kramasub@codeaurora.org>
Quoting Karthikeyan Ramasubramanian (2018-03-14 16:58:50)
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index 979ab49..ea3efc5 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> @@ -12,4 +12,43 @@
> / {
> model = "Qualcomm Technologies, Inc. SDM845 MTP";
> compatible = "qcom,sdm845-mtp";
> +
> + aliases {
> + serial0 = &uart2;
> + };
> +
> + chosen {
> + stdout-path = "serial0";
Also add :115200n8 ?
> + };
> +};
> +
> +&soc {
I think the method is to put these inside soc node without using the
phandle reference. So indent everything once more.
> + geniqup@ac0000 {
> + serial@a84000 {
> + status = "okay";
> + };
> + };
> +
> + pinctrl@3400000 {
> + qup-uart2-default {
> + pinconf_tx {
> + pins = "gpio4";
> + drive-strength = <2>;
> + bias-disable;
> + };
> +
> + pinconf_rx {
> + pins = "gpio5";
> + drive-strength = <2>;
> + bias-pull-up;
> + };
> + };
> +
> + qup-uart2-sleep {
> + pinconf {
> + pins = "gpio4", "gpio5";
> + bias-pull-down;
> + };
> + };
> + };
> };
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 32f8561..59334d9 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -6,6 +6,7 @@
> */
>
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/qcom,gcc-sdm845.h>
>
> / {
> interrupt-parent = <&intc>;
> @@ -194,6 +195,20 @@
> #gpio-cells = <2>;
> interrupt-controller;
> #interrupt-cells = <2>;
> +
> + qup_uart2_default: qup-uart2-default {
> + pinmux {
> + function = "qup9";
> + pins = "gpio4", "gpio5";
> + };
> + };
> +
> + qup_uart2_sleep: qup-uart2-sleep {
> + pinmux {
> + function = "gpio";
> + pins = "gpio4", "gpio5";
> + };
> + };
Are these supposed to go to the board file?
> };
>
> timer@17c90000 {
> @@ -272,5 +287,28 @@
> #interrupt-cells = <4>;
> cell-index = <0>;
> };
> +
> + geniqup@ac0000 {
> + compatible = "qcom,geni-se-qup";
> + reg = <0xac0000 0x6000>;
> + clock-names = "m-ahb", "s-ahb";
> + clocks = <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>,
> + <&gcc GCC_QUPV3_WRAP_1_S_AHB_CLK>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
Add a status = "disabled" here too.
> +
> + uart2: serial@a84000 {
> + compatible = "qcom,geni-debug-uart";
> + reg = <0xa84000 0x4000>;
> + clock-names = "se";
> + clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>;
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&qup_uart2_default>;
> + pinctrl-1 = <&qup_uart2_sleep>;
> + interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
> + };
> + };
> };
> };
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] modify one dead link
From: Sergei Trofimovich @ 2018-03-20 19:42 UTC (permalink / raw)
To: Dongliang Mu
Cc: corbet, yamada.masahiro, tony.luck, bjorn.forsman, linux-doc,
linux-kernel
In-Reply-To: <20180320145422.24331-1-mudongliangabcd@gmail.com>
On Tue, 20 Mar 2018 10:54:22 -0400
Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> - # hg clone http://xenbits.xensource.com/ext/ia64/xen-unstable.hg
> + # hg clone http://xenbits.xensource.com/ext/ia64/xen-unstable
> # cd xen-unstable.hg
> # hg clone http://xenbits.xensource.com/ext/ia64/linux-2.6.18-xen.hg
You will need to fix a 'cd' as well:
cd xen-unstable
Otherwise looks good.
--
Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] modify one dead link
From: Dongliang Mu @ 2018-03-20 19:49 UTC (permalink / raw)
To: Sergei Trofimovich
Cc: corbet, yamada.masahiro, tony.luck, bjorn.forsman, linux-doc,
linux-kernel
In-Reply-To: <20180320194203.205c6c60@sf>
On 03/20/2018 03:42 PM, Sergei Trofimovich wrote:
> On Tue, 20 Mar 2018 10:54:22 -0400
> Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
>> - # hg clone http://xenbits.xensource.com/ext/ia64/xen-unstable.hg
>> + # hg clone http://xenbits.xensource.com/ext/ia64/xen-unstable
>> # cd xen-unstable.hg
>> # hg clone http://xenbits.xensource.com/ext/ia64/linux-2.6.18-xen.hg
> You will need to fix a 'cd' as well:
> cd xen-unstable
I will modify that problem. Then resubmit the patch.
Thanks.
>
> Otherwise looks good.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] fix one dead link in ia64/xen.txt
From: Dongliang Mu @ 2018-03-20 19:56 UTC (permalink / raw)
To: corbet, yamada.masahiro, slyfox, tony.luck, bjorn.forsman,
mudongliangabcd
Cc: linux-doc, linux-kernel
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
Documentation/ia64/xen.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/ia64/xen.txt b/Documentation/ia64/xen.txt
index a12c74ce2773..464d4c29b8b5 100644
--- a/Documentation/ia64/xen.txt
+++ b/Documentation/ia64/xen.txt
@@ -26,8 +26,8 @@ Getting and Building Xen and Dom0
DomainU OS : RHEL5
1. Download source
- # hg clone http://xenbits.xensource.com/ext/ia64/xen-unstable.hg
- # cd xen-unstable.hg
+ # hg clone http://xenbits.xensource.com/ext/ia64/xen-unstable
+ # cd xen-unstable
# hg clone http://xenbits.xensource.com/ext/ia64/linux-2.6.18-xen.hg
2. # make world
--
2.16.2
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v5 1/2] cpuset: Enable cpuset controller in default hierarchy
From: Tejun Heo @ 2018-03-20 20:10 UTC (permalink / raw)
To: Waiman Long
Cc: Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, linux-doc, kernel-team, pjt, luto, efault, torvalds,
Roman Gushchin
In-Reply-To: <1881c1da-56ec-d76b-b736-fd0919737ec6@redhat.com>
Hello, Waiman.
On Tue, Mar 20, 2018 at 09:51:20AM -0400, Waiman Long wrote:
> >> + It lists the onlined CPUs that are actually allowed to be
> >> + used by tasks within the current cgroup. It is a subset of
> >> + "cpuset.cpus". Its value will be affected by CPU hotplug
> >> + events.
> > Can we do cpuset.cpus.availble which lists the cpus available to the
> > cgroup instead of the eventual computed mask for the cgroup? That'd
> > be more useful as it doesn't lose the information by and'ing what's
> > available with the cgroup's mask and it's trivial to determine the
> > effective from the two masks.
>
> I don't get what you want here. cpus is the cpuset's cpus_allowed mask.
> effective_cpus is the effective_cpus mask. When you say cpus available
> to the cgroup, do you mean the cpu_online_mask or the list of cpus from
> the parent? Or do you just want to change the name to cpus.available
> instead of effective_cpus?
The available cpus from the parent, where the effective is AND between
cpuset.available and cpuset.cpus of the cgroup, so that the user can
see what's available for the cgroup unfiltered by cpuset.cpus.
> Right, I will set CFTYPE_NOT_ON_ROOT to "cpus" and "mems" as we are not
> supposed to change them in the root. The effective_cpus and
> effective_mems will be there in the root to show what are available.
So, we can do that in the future but let's not do that for now. It's
the same problem we have for basically everything else and we've
stayed away from replicating the information in the root cgroup. This
might change in the future but if we do that let's do that
consistently.
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 2/2] cpuset: Add cpuset.flags control knob to v2
From: Waiman Long @ 2018-03-20 20:12 UTC (permalink / raw)
To: Tejun Heo
Cc: Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, linux-doc, kernel-team, pjt, luto, efault, torvalds,
Roman Gushchin
In-Reply-To: <8e553b7e-cab6-1b5d-9110-cc5770ae16c4@redhat.com>
On 03/19/2018 12:33 PM, Waiman Long wrote:
> On 03/19/2018 12:26 PM, Tejun Heo wrote:
>> Hello, Waiman.
>>
>> On Thu, Mar 15, 2018 at 05:20:42PM -0400, Waiman Long wrote:
>>> + The currently supported flag is:
>>> +
>>> + sched_load_balance
>>> + When it is not set, there will be no load balancing
>>> + among CPUs on this cpuset. Tasks will stay in the
>>> + CPUs they are running on and will not be moved to
>>> + other CPUs.
>>> +
>>> + When it is set, tasks within this cpuset will be
>>> + load-balanced by the kernel scheduler. Tasks will be
>>> + moved from CPUs with high load to other CPUs within
>>> + the same cpuset with less load periodically.
>> Hmm... looks like this is something which can be decided by the cgroup
>> itself and should be made delegatable. Given that different flags
>> might need different delegation settings and the precedence of
>> memory.oom_group, I think it'd be better to make the flags separate
>> bool files - ie. cpuset.sched_load_balance which contains 0/1 and
>> marked delegatable.
>>
>> Thanks.
>>
> Sure. Will do that.
After some thought, I am planning to impose the following additional
constraints on how sched_load_balance works in v2.
1) sched_load_balance will be made hierarchical, the child will inherit
the flag from its parent.
2) cpu_exclusive will be implicitly associated with sched_load_balance.
IOW, sched_load_balance => !cpu_exclusive, and !sched_load_balance =>
cpu_exclusive.
3) sched_load_balance cannot be 1 on a child if it is 0 on the parent.
With these changes, sched_load_balance will have to be set by the parent
and so will not be delegatable. Please let me know your thought on that.
Cheers,
Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] fix one dead link in ia64/xen.txt
From: Andrew Cooper @ 2018-03-20 20:17 UTC (permalink / raw)
To: Dongliang Mu, corbet, yamada.masahiro, slyfox, tony.luck,
bjorn.forsman
Cc: linux-doc, linux-kernel
In-Reply-To: <20180320195658.17498-1-mudongliangabcd@gmail.com>
On 20/03/18 19:56, Dongliang Mu wrote:
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
> Documentation/ia64/xen.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/ia64/xen.txt b/Documentation/ia64/xen.txt
> index a12c74ce2773..464d4c29b8b5 100644
> --- a/Documentation/ia64/xen.txt
> +++ b/Documentation/ia64/xen.txt
> @@ -26,8 +26,8 @@ Getting and Building Xen and Dom0
> DomainU OS : RHEL5
>
> 1. Download source
> - # hg clone http://xenbits.xensource.com/ext/ia64/xen-unstable.hg
> - # cd xen-unstable.hg
> + # hg clone http://xenbits.xensource.com/ext/ia64/xen-unstable
> + # cd xen-unstable
> # hg clone http://xenbits.xensource.com/ext/ia64/linux-2.6.18-xen.hg
>
> 2. # make world
The last commit in that repository is almost 9 years old, and IA64
support was dropped from Xen mainline6 years ago.
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=570c311ca2c7a1131570cdfc77e977bc7a9bf4c0
There are a number of other dead links in this doc, and those which
aren't dead refer to Linux 2.6.x. I'd just remove the entire file,
rather than pretend that any of this still might work. (If by some
miracle it does still function, its 10 years behind on security fixes...)
~Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 2/2] cpuset: Add cpuset.flags control knob to v2
From: Tejun Heo @ 2018-03-20 20:22 UTC (permalink / raw)
To: Waiman Long
Cc: Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, linux-doc, kernel-team, pjt, luto, efault, torvalds,
Roman Gushchin
In-Reply-To: <cbe9b852-80b6-8341-b51c-4d754d00ce53@redhat.com>
Hello, Waiman.
On Tue, Mar 20, 2018 at 04:12:25PM -0400, Waiman Long wrote:
> After some thought, I am planning to impose the following additional
> constraints on how sched_load_balance works in v2.
>
> 1) sched_load_balance will be made hierarchical, the child will inherit
> the flag from its parent.
> 2) cpu_exclusive will be implicitly associated with sched_load_balance.
> IOW, sched_load_balance => !cpu_exclusive, and !sched_load_balance =>
> cpu_exclusive.
> 3) sched_load_balance cannot be 1 on a child if it is 0 on the parent.
>
> With these changes, sched_load_balance will have to be set by the parent
> and so will not be delegatable. Please let me know your thought on that.
So, for configurations, we usually don't let them interact across
hierarchy because that can lead to configurations surprise-changing
and delegated children locking the parent into the current config.
This case could be different and as long as we always guarantee that
an ancestor isn't limited by its descendants in what it can configure,
it should be okay (e.g. an ancestor should always be able to turn on
sched_load_balance regardless of how the descendants are configured).
Hmmm... can you explain why sched_load_balance needs to behave this
way?
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 2/2] cpuset: Add cpuset.flags control knob to v2
From: Waiman Long @ 2018-03-20 20:43 UTC (permalink / raw)
To: Tejun Heo
Cc: Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, linux-doc, kernel-team, pjt, luto, efault, torvalds,
Roman Gushchin
In-Reply-To: <20180320202247.GQ519464@devbig577.frc2.facebook.com>
On 03/20/2018 04:22 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Tue, Mar 20, 2018 at 04:12:25PM -0400, Waiman Long wrote:
>> After some thought, I am planning to impose the following additional
>> constraints on how sched_load_balance works in v2.
>>
>> 1) sched_load_balance will be made hierarchical, the child will inherit
>> the flag from its parent.
>> 2) cpu_exclusive will be implicitly associated with sched_load_balance.
>> IOW, sched_load_balance => !cpu_exclusive, and !sched_load_balance =>
>> cpu_exclusive.
>> 3) sched_load_balance cannot be 1 on a child if it is 0 on the parent.
>>
>> With these changes, sched_load_balance will have to be set by the parent
>> and so will not be delegatable. Please let me know your thought on that.
> So, for configurations, we usually don't let them interact across
> hierarchy because that can lead to configurations surprise-changing
> and delegated children locking the parent into the current config.
>
> This case could be different and as long as we always guarantee that
> an ancestor isn't limited by its descendants in what it can configure,
> it should be okay (e.g. an ancestor should always be able to turn on
> sched_load_balance regardless of how the descendants are configured).
Yes, I will do some testing to make sure that a descendant won't be able
to affect how the ancestors can behave.
> Hmmm... can you explain why sched_load_balance needs to behave this
> way?
It boils down to the fact that it doesn't make sense to have a CPU in an
isolated cpuset to participate in load balancing in another cpuset as
Mike has said before. It is especially true in a parent-child
relationship where a delegatee can escape CPU isolation by re-enabling
sched_load_balance in a child cpuset.
Cheers,
Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 1/2] cpuset: Enable cpuset controller in default hierarchy
From: Waiman Long @ 2018-03-20 20:53 UTC (permalink / raw)
To: Tejun Heo
Cc: Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, linux-doc, kernel-team, pjt, luto, efault, torvalds,
Roman Gushchin
In-Reply-To: <20180320201029.GO519464@devbig577.frc2.facebook.com>
On 03/20/2018 04:10 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Tue, Mar 20, 2018 at 09:51:20AM -0400, Waiman Long wrote:
>>>> + It lists the onlined CPUs that are actually allowed to be
>>>> + used by tasks within the current cgroup. It is a subset of
>>>> + "cpuset.cpus". Its value will be affected by CPU hotplug
>>>> + events.
>>> Can we do cpuset.cpus.availble which lists the cpus available to the
>>> cgroup instead of the eventual computed mask for the cgroup? That'd
>>> be more useful as it doesn't lose the information by and'ing what's
>>> available with the cgroup's mask and it's trivial to determine the
>>> effective from the two masks.
>> I don't get what you want here. cpus is the cpuset's cpus_allowed mask.
>> effective_cpus is the effective_cpus mask. When you say cpus available
>> to the cgroup, do you mean the cpu_online_mask or the list of cpus from
>> the parent? Or do you just want to change the name to cpus.available
>> instead of effective_cpus?
> The available cpus from the parent, where the effective is AND between
> cpuset.available and cpuset.cpus of the cgroup, so that the user can
> see what's available for the cgroup unfiltered by cpuset.cpus.
ASAIK for v2, when cpuset.cpus is empty, cpuset.effective_cpus will show
all the cpus available from the parent. It is a different behavior from
v1. So do we still need a cpuset.cpus_available?
>> Right, I will set CFTYPE_NOT_ON_ROOT to "cpus" and "mems" as we are not
>> supposed to change them in the root. The effective_cpus and
>> effective_mems will be there in the root to show what are available.
> So, we can do that in the future but let's not do that for now. It's
> the same problem we have for basically everything else and we've
> stayed away from replicating the information in the root cgroup. This
> might change in the future but if we do that let's do that
> consistently.
That is fine. I will make them all disappears in the root cgroup.
Cheers,
Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 1/2] cpuset: Enable cpuset controller in default hierarchy
From: Tejun Heo @ 2018-03-20 21:14 UTC (permalink / raw)
To: Waiman Long
Cc: Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, linux-doc, kernel-team, pjt, luto, efault, torvalds,
Roman Gushchin
In-Reply-To: <2b9261b8-6a3a-81d1-9c9a-394524a0d413@redhat.com>
Hello,
On Tue, Mar 20, 2018 at 04:53:37PM -0400, Waiman Long wrote:
> ASAIK for v2, when cpuset.cpus is empty, cpuset.effective_cpus will show
> all the cpus available from the parent. It is a different behavior from
> v1. So do we still need a cpuset.cpus_available?
Heh, you're right. Let's forget about available and do
cpuset.cpus.effective. The primary reason for suggesting that was
because of the similarity with cgroup.controllers and
cgroup.subtree_control; however, they're that way because
subtree_control is delegatable. For a normal resource knob like
cpuset.cpus, the knob is owned by the parent and what's interesting to
the parent is its effective set that it's distributing from.
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 1/2] cpuset: Enable cpuset controller in default hierarchy
From: Waiman Long @ 2018-03-20 22:01 UTC (permalink / raw)
To: Tejun Heo
Cc: Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, linux-doc, kernel-team, pjt, luto, efault, torvalds,
Roman Gushchin
In-Reply-To: <20180320211417.GA2149215@devbig577.frc2.facebook.com>
On 03/20/2018 05:14 PM, Tejun Heo wrote:
> Hello,
>
> On Tue, Mar 20, 2018 at 04:53:37PM -0400, Waiman Long wrote:
>> ASAIK for v2, when cpuset.cpus is empty, cpuset.effective_cpus will show
>> all the cpus available from the parent. It is a different behavior from
>> v1. So do we still need a cpuset.cpus_available?
> Heh, you're right. Let's forget about available and do
> cpuset.cpus.effective. The primary reason for suggesting that was
> because of the similarity with cgroup.controllers and
> cgroup.subtree_control; however, they're that way because
> subtree_control is delegatable. For a normal resource knob like
> cpuset.cpus, the knob is owned by the parent and what's interesting to
> the parent is its effective set that it's distributing from.
OK, will change the names as suggested.
-Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 3/6] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller
From: Karthik Ramasubramanian @ 2018-03-20 22:10 UTC (permalink / raw)
To: Doug Anderson
Cc: Jonathan Corbet, Andy Gross, David Brown, Rob Herring,
Mark Rutland, Wolfram Sang, Greg Kroah-Hartman, linux-doc,
linux-arm-msm, devicetree, linux-i2c, linux-serial, Jiri Slaby,
evgreen, acourbot, swboyd, Sagar Dharia, Girish Mahadevan
In-Reply-To: <CAD=FV=W+eiy80y2QKp_-jPEp4SG0WxmdKFTn-5de0A1F-XPrqA@mail.gmail.com>
On 3/19/2018 3:08 PM, Doug Anderson wrote:
> Hi,
>
> On Wed, Mar 14, 2018 at 4:58 PM, Karthikeyan Ramasubramanian
> <kramasub@codeaurora.org> 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 <kramasub@codeaurora.org>
>> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
>> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
>> ---
>> drivers/i2c/busses/Kconfig | 13 +
>> drivers/i2c/busses/Makefile | 1 +
>> drivers/i2c/busses/i2c-qcom-geni.c | 648 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 662 insertions(+)
>
> Since I reviewed v3 of the i2c patch, can you make sure that you CC me
> on future patches? Thanks! :)
>
> Overall this looks pretty nice to me now. A few minor comments...
Sorry for not keeping you in CC for this patch. I will keep you in the
CC going forward.
>
>
>> +static void geni_i2c_abort_xfer(struct geni_i2c_dev *gi2c)
>> +{
>> + u32 val;
>> + unsigned long time_left = ABORT_TIMEOUT;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&gi2c->lock, flags);
>> + geni_i2c_err(gi2c, GENI_TIMEOUT);
>> + gi2c->cur = NULL;
>> + geni_se_abort_m_cmd(&gi2c->se);
>> + spin_unlock_irqrestore(&gi2c->lock, flags);
>> + do {
>> + time_left = wait_for_completion_timeout(&gi2c->done, time_left);
>> + val = readl_relaxed(gi2c->se.base + SE_GENI_M_IRQ_STATUS);
>> + } while (!(val & M_CMD_ABORT_EN) && time_left);
>> +
>> + if (!(val & M_CMD_ABORT_EN) && !time_left)
>
> Why do you need to check !time_left? Just "if (!(val & M_CMD_ABORT_EN))".
I will remove here and in the other places you mentioned.
>
>
>> +
>> + gi2c->se.dev = &pdev->dev;
>> + gi2c->se.wrapper = dev_get_drvdata(pdev->dev.parent);
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + gi2c->se.base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(gi2c->se.base))
>> + return PTR_ERR(gi2c->se.base);
>> +
>> + gi2c->se.clk = devm_clk_get(&pdev->dev, "se");
>> + if (IS_ERR(gi2c->se.clk)) {
>> + ret = PTR_ERR(gi2c->se.clk);
>> + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = device_property_read_u32(&pdev->dev, "clock-frequency",
>> + &gi2c->clk_freq_out);
>> + if (ret) {
>> + /* All GENI I2C slaves support 400kHz. So default to 400kHz. */
>
> Can you explain this comment? Are you saying that GENI only supports
> talking to I2C devices (devices are also known as "slaves" and GENI
> should be the I2C master) that talk 400 kHz I2C or better? Why do you
> even have 100 kHz in your table above then? I don't think this is
> what you meant...
>
> Perhaps you meant to say "All GENI I2C masters support at least 400
> kHz, so default ot 400 kHz"
>
> ...however, even if you changed the comment as I suggested I'm still
> not a fan. As I said in my review of the prevous version:
>
>> I feel like it should default to 100KHz. i2c_parse_fw_timings()
>> defaults to this and to me the wording "New drivers almost always
>> should use the defaults" makes me feel this should be the defaults.
>
> I would also say that even if all GENI I2C masters support at least
> 400 kHz that doesn't mean that all I2C devices on the bus support 400
> kHz. You could certainly imagine someone sticking something on this
> bus that was super low cost and supported only 100 kHz I2C.
We felt that all the current slaves that are connected to our masters
support 400kHz. Hence we used that as a default frequency.
I agree with you regarding 100kHz as default frequency. I will update
that way in the next patch.
>
>
> -Doug
>
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 6/6] arm64: dts: sdm845: Add I2C controller support
From: Sagar Dharia @ 2018-03-20 22:16 UTC (permalink / raw)
To: Doug Anderson
Cc: Karthikeyan Ramasubramanian, Jonathan Corbet, Andy Gross,
David Brown, Rob Herring, Mark Rutland, Wolfram Sang,
Greg Kroah-Hartman, linux-doc, linux-arm-msm, devicetree,
linux-i2c, linux-serial, Jiri Slaby, evgreen, acourbot, swboyd,
Bjorn Andersson
In-Reply-To: <CAD=FV=Vq4Q2LvY_O-YsdyUmQOUANa0ap+U9j7PXSD3_ao6sCdQ@mail.gmail.com>
Hi,
On 3/19/2018 5:56 PM, Doug Anderson wrote:
> Hi,
>
> On Mon, Mar 19, 2018 at 3:15 PM, Sagar Dharia <sdharia@codeaurora.org> wrote:
>>>> + pinconf {
>>>> + pins = "gpio55", "gpio56";
>>>> + drive-strength = <2>;
>>>> + bias-disable;
>>>> + };
>>>> + };
>>>> +
>>>> + qup-i2c10-sleep {
>>>> + pinconf {
>>>> + pins = "gpio55", "gpio56";
>>>> + bias-pull-up;
>>>
>>> Are you sure that you want pullups enabled for sleep here? There are
>>> external pulls on this line (as there are on many i2c busses) so doing
>>> this will double-enable pulls. It probably won't hurt, but I'm
>>> curious if there's some sort of reason here.
>>>
>> 1. We need the lines to remain high to avoid slaves sensing a false
>> start-condition (this can happen if the SDA goes down before SCL).
>> 2. Disclaimer: I'm not a HW expert, but we were told that
>> tri-state/bias-disabled lines can draw more current. I will find out
>> more about that.
>
> Agreed that they need to remain high, but you've got very strong
> pullups external to the SoC. Those will keep it high. You don't need
> the internal ones too.
>
> As extra evidence that the external pullups _must_ be present on your
> board: you specify bias-disable in the active state. That can only
> work if there are external pullups (or if there were some special
> extra secret internal pullups that were part of geni). i2c is an
> open-drain bus and thus there must be pullups on the bus in order to
> communicate.
>
You are right, I followed up about the pull-up recommendation and that
was for a GPIO where there was no external pull-up (GPIO was not used
for I2C). It's safe to assume I2C will always have external pullup. We
will change sleep-config of I2C GPIOs to no-pull.
>
>>>> + };
>>>> + };
>>>> };
>>>> };
>>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>>> index 59334d9..9ef056f 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>>> @@ -209,6 +209,21 @@
>>>> pins = "gpio4", "gpio5";
>>>> };
>>>> };
>>>> +
>>>> + qup_i2c10_default: qup-i2c10-default {
>>>> + pinmux {
>>>> + function = "qup10";
>>>> + pins = "gpio55", "gpio56";
>>>> + };
>>>> + };
>>>> +
>>>> + qup_i2c10_sleep: qup-i2c10-sleep {
>>>> + pinmux {
>>>> + function = "gpio";
>>>> + pins = "gpio55", "gpio56";
>>>> + };
>>>> + };
>>>> +
>>>> };
>>>>
>>>> timer@17c90000 {
>>>> @@ -309,6 +324,20 @@
>>>> interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>;
>>>> status = "disabled";
>>>> };
>>>> +
>>>> + i2c10: i2c@a88000 {
>>>
>>> Seems like it might be nice to add all the i2c busses into the main
>>> sdm845.dtsi file. Sure, most won't be enabled, but it seems like it
>>> would avoid churn later.
>>>
>>> ...if you're sure you want to add only one i2c controller, subject of
>>> this patch should indicate that.
>>>
>>
>> Yes, we typically have a "platform(sdm845 here)-qupv3.dtsi" defining
>> most of the serial-bus instances (i2c, spi, and uart with
>> status=disabled) that we include from the common header. The boards
>> enable instances they need.
>> Will that be okay?
>
> Unless you really feel the need to put these in a separate file I'd
> just put them straight in sdm845.dtsi. Yeah, it'll get big, but
> that's OK by me. I _think_ this matches what Bjorn was suggesting on
> previous device tree patches, but CCing him just in case. I'm
> personally OK with whatever Bjorn and other folks with more Qualcomm
> history would like.
>
> ...but yeah, I'm asking for them all to be listed with status="disabled".
>
Sure, we will change the subject of this patch to indicate that we are
adding 1 controller as of now. Later we will add all I2C controllers to
dtsi as another patch since that will need pinctrl settings for GPIOs
used by those instances and the wrappers devices needed by them.
Thanks
Sagar
>
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 3/6] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller
From: Sagar Dharia @ 2018-03-20 22:23 UTC (permalink / raw)
To: Doug Anderson, Karthikeyan Ramasubramanian
Cc: Jonathan Corbet, Andy Gross, David Brown, Rob Herring,
Mark Rutland, Wolfram Sang, Greg Kroah-Hartman, linux-doc,
linux-arm-msm, devicetree, linux-i2c, linux-serial, Jiri Slaby,
evgreen, acourbot, swboyd, Girish Mahadevan
In-Reply-To: <CAD=FV=W+eiy80y2QKp_-jPEp4SG0WxmdKFTn-5de0A1F-XPrqA@mail.gmail.com>
Hi Doug,
On 3/19/2018 3:08 PM, Doug Anderson wrote:
> Hi,
>
> On Wed, Mar 14, 2018 at 4:58 PM, Karthikeyan Ramasubramanian
> <kramasub@codeaurora.org> 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 <kramasub@codeaurora.org>
>> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
>> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
>
>> +/*
>> + * Hardware uses the underlying formula to calculate time periods of
>> + * SCL clock cycle.
>> + *
>> + * time of high period of SCL: t_high = (t_high_cnt * clk_div) / source_clock
>> + * time of low period of SCL: t_low = (t_low_cnt * clk_div) / source_clock
>> + * time of full period of SCL: t_cycle = (t_cycle_cnt * clk_div) / source_clock
>> + * clk_freq_out = t / t_cycle
>> + * source_clock = 19.2 MHz
>> + */
>> +static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = {
>> + {KHZ(100), 7, 10, 11, 26},
>> + {KHZ(400), 2, 5, 12, 24},
>> + {KHZ(1000), 1, 3, 9, 18},
>> +};
>
> Thanks for the docs. ...though if these docs are indeed correct and
> there's no extra magic fudge factor I'm still a bit baffled why the
> frequency isn't out of spec for 100 kHz and 1 MHz. I know you said
> hardware validated it, but are you really certain they validated 100
> kHz and 1 MHz?
>
>>>> 19200. / (1 * 18)
> 1066.6666666666667
>
>>>> 19200. / (2 * 24)
> 400.0
>
>>>> 19200. / (7 * 26)
> 105.49450549450549
>
> Specifically: either the docs you wrote above are wrong (and there's a
> magic fudge factor that you didn't document) or your hardware guys are
> wrong. See the I2C spec at
> <https://www.nxp.com/docs/en/user-guide/UM10204.pdf>. Table 10.
> ("Characteristics of the SDA and SCL bus lines for Standard, Fast, and
> Fast-mode Plus I2C-bus devices") says fSCL Max is 100 kHz, 400 kHz, or
> 1000 kHz.
>
>
> I could believe that 99.9% of all devices that support 100 kHz also
> support 105.5 kHz and that 99.9% of all devices that support 1000 kHz
> also support 1066.7 kHz. However, it's still not in spec. If you
> want to enable a turbo boost mode that runs 5% faster (really?) then I
> suppose you could add that as an optional feature, but this shouldn't
> be the default.
>
Yes, we will document that there is a fudge-factor (cycles needed to run
the firmware per inputs from HW team). The actual frequencies seen for
100KHz and 1MHz configurations were close to 98KHz, and 960KHz
respectively. t-high, and t-low were within specs for all frequencies.
Thanks
Sagar
>
>> +static void geni_i2c_abort_xfer(struct geni_i2c_dev *gi2c)
>> +{
>> + u32 val;
>> + unsigned long time_left = ABORT_TIMEOUT;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&gi2c->lock, flags);
>> + geni_i2c_err(gi2c, GENI_TIMEOUT);
>> + gi2c->cur = NULL;
>> + geni_se_abort_m_cmd(&gi2c->se);
>> + spin_unlock_irqrestore(&gi2c->lock, flags);
>> + do {
>> + time_left = wait_for_completion_timeout(&gi2c->done, time_left);
>> + val = readl_relaxed(gi2c->se.base + SE_GENI_M_IRQ_STATUS);
>> + } while (!(val & M_CMD_ABORT_EN) && time_left);
>> +
>> + if (!(val & M_CMD_ABORT_EN) && !time_left)
>
> Why do you need to check !time_left? Just "if (!(val & M_CMD_ABORT_EN))".
>
>
>> + dev_err(gi2c->se.dev, "Timeout abort_m_cmd\n");
>> +}
>> +
>> +static void geni_i2c_rx_fsm_rst(struct geni_i2c_dev *gi2c)
>> +{
>> + u32 val;
>> + unsigned long time_left = RST_TIMEOUT;
>> +
>> + writel_relaxed(1, gi2c->se.base + SE_DMA_RX_FSM_RST);
>> + do {
>> + time_left = wait_for_completion_timeout(&gi2c->done, time_left);
>> + val = readl_relaxed(gi2c->se.base + SE_DMA_RX_IRQ_STAT);
>> + } while (!(val & RX_RESET_DONE) && time_left);
>> +
>> + if (!(val & RX_RESET_DONE) && !time_left)
>
> Similar. Don't need "&& !time_left"
>
>
>> + dev_err(gi2c->se.dev, "Timeout resetting RX_FSM\n");
>> +}
>> +
>> +static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c)
>> +{
>> + u32 val;
>> + unsigned long time_left = RST_TIMEOUT;
>> +
>> + writel_relaxed(1, gi2c->se.base + SE_DMA_TX_FSM_RST);
>> + do {
>> + time_left = wait_for_completion_timeout(&gi2c->done, time_left);
>> + val = readl_relaxed(gi2c->se.base + SE_DMA_TX_IRQ_STAT);
>> + } while (!(val & TX_RESET_DONE) && time_left);
>> +
>> + if (!(val & TX_RESET_DONE) && !time_left)
>
> Similar. Don't need "&& !time_left"
>
>
>> +static int geni_i2c_probe(struct platform_device *pdev)
>> +{
>> + struct geni_i2c_dev *gi2c;
>> + struct resource *res;
>> + u32 proto, tx_depth;
>> + int ret;
>> +
>> + gi2c = devm_kzalloc(&pdev->dev, sizeof(*gi2c), GFP_KERNEL);
>> + if (!gi2c)
>> + return -ENOMEM;
>> +
>> + gi2c->se.dev = &pdev->dev;
>> + gi2c->se.wrapper = dev_get_drvdata(pdev->dev.parent);
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + gi2c->se.base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(gi2c->se.base))
>> + return PTR_ERR(gi2c->se.base);
>> +
>> + gi2c->se.clk = devm_clk_get(&pdev->dev, "se");
>> + if (IS_ERR(gi2c->se.clk)) {
>> + ret = PTR_ERR(gi2c->se.clk);
>> + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = device_property_read_u32(&pdev->dev, "clock-frequency",
>> + &gi2c->clk_freq_out);
>> + if (ret) {
>> + /* All GENI I2C slaves support 400kHz. So default to 400kHz. */
>
> Can you explain this comment? Are you saying that GENI only supports
> talking to I2C devices (devices are also known as "slaves" and GENI
> should be the I2C master) that talk 400 kHz I2C or better? Why do you
> even have 100 kHz in your table above then? I don't think this is
> what you meant...
>
> Perhaps you meant to say "All GENI I2C masters support at least 400
> kHz, so default ot 400 kHz"
>
> ...however, even if you changed the comment as I suggested I'm still
> not a fan. As I said in my review of the prevous version:
>
>> I feel like it should default to 100KHz. i2c_parse_fw_timings()
>> defaults to this and to me the wording "New drivers almost always
>> should use the defaults" makes me feel this should be the defaults.
>
> I would also say that even if all GENI I2C masters support at least
> 400 kHz that doesn't mean that all I2C devices on the bus support 400
> kHz. You could certainly imagine someone sticking something on this
> bus that was super low cost and supported only 100 kHz I2C.
>
>
> -Doug
>
--
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 4/6] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
From: Karthik Ramasubramanian @ 2018-03-20 22:53 UTC (permalink / raw)
To: Stephen Boyd, andy.gross, corbet, david.brown, gregkh,
mark.rutland, robh+dt, wsa
Cc: linux-doc, linux-arm-msm, devicetree, linux-i2c, linux-serial,
jslaby, evgreen, acourbot, Girish Mahadevan, Sagar Dharia,
Doug Anderson
In-Reply-To: <152156025613.254778.431774948232120027@swboyd.mtv.corp.google.com>
On 3/20/2018 9:37 AM, Stephen Boyd wrote:
> Quoting Karthikeyan Ramasubramanian (2018-03-14 16:58:49)
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>> new file mode 100644
>> index 0000000..1442777
>> --- /dev/null
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -0,0 +1,1158 @@
>> +
>> +#ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
>> +static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch)
>> +{
>> + writel_relaxed(ch, uport->membase + SE_GENI_TX_FIFOn);
>
> Does this expect the whole word to have data to write? Or does the FIFO
> output a character followed by three NUL bytes each time it gets
> written? The way that uart_console_write() works is to take each
> character a byte at a time, put it into an int (so extend that byte with
> zero) and then pass it to the putchar function. I would expect that at
> this point the hardware sees the single character and then 3 NULs enter
> the FIFO each time.
>
> For previous MSM uarts I had to handle this oddity by packing the words
> into the fifo four at a time. You may need to do the same here.
The packing configuration 1 * 8 (done using geni_se_config_packing)
ensures that only one byte per FIFO word needs to be transmitted. From
that perspective, we need not have such oddity.
>
>> +}
>> +
>> +static void
>> +__qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
>> + unsigned int count)
>> +{
>> + int i;
>> + u32 bytes_to_send = count;
>> +
>> + for (i = 0; i < count; i++) {
>> + if (s[i] == '\n')
>
> Can you add a comment that uart_console_write() adds a carriage return
> for each newline?
Ok.
>
>> + bytes_to_send++;
>> + }
>> +
>> + writel_relaxed(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
>> + qcom_geni_serial_setup_tx(uport, bytes_to_send);
>> + for (i = 0; i < count; ) {
>> + 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.
>> + */
>> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
>> + M_TX_FIFO_WATERMARK_EN, true))
>> + break;
>> + chars_to_write = min_t(size_t, (size_t)(count - i), avail / 2);
>
> The _t part of min_t should do the casting already, so we can drop the
> cast on count - i?
Ok.
>
>> + uart_console_write(uport, s + i, chars_to_write,
>> + qcom_geni_serial_wr_char);
>> + writel_relaxed(M_TX_FIFO_WATERMARK_EN, uport->membase +
>> + SE_GENI_M_IRQ_CLEAR);
>> + i += chars_to_write;
>> + }
>> + qcom_geni_serial_poll_tx_done(uport);
>> +}
>> +
>> +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 = &port->uport;
>> + if (oops_in_progress)
>> + locked = spin_trylock_irqsave(&uport->lock, flags);
>> + else
>> + spin_lock_irqsave(&uport->lock, flags);
>> +
>> + /* Cancel the current write to log the fault */
>> + if (!locked) {
>> + geni_se_cancel_m_cmd(&port->se);
>> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
>> + M_CMD_CANCEL_EN, true)) {
>> + geni_se_abort_m_cmd(&port->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);
>> + }
>> +
>> + __qcom_geni_serial_console_write(uport, s, count);
>> + if (locked)
>> + spin_unlock_irqrestore(&uport->lock, flags);
>> +}
>
> Can you also support the OF_EARLYCON_DECLARE method of console writing
> so we can get an early printk style debug console?
Do you prefer that as part of this patch itself or is it ok if I upload
the earlycon support once this gets merged.
>
>> +
>> +static void qcom_geni_serial_handle_tx(struct uart_port *uport)
>> +{
>> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>> + struct circ_buf *xmit = &uport->state->xmit;
>> + size_t avail;
>> + size_t remaining;
>> + int i;
>> + 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 - 1);
>> + if (chunk > (UART_XMIT_SIZE - tail))
>> + chunk = UART_XMIT_SIZE - tail;
>> + if (chunk > avail)
>> + chunk = avail;
>
> chunk = min3(chunk, UART_XMIT_SIZE - tail, avail);
Ok.
>
>> +
>> + if (!chunk)
>> + goto out_write_wakeup;
>> +
>> + qcom_geni_serial_setup_tx(uport, chunk);
>> +
>> + remaining = chunk;
>> + for (i = 0; i < chunk; ) {
>> + unsigned int tx_bytes;
>> + unsigned int buf = 0;
>
> Make buf into a u8 array of 4?
Ok.
>
>> + int c;
>> +
>> + tx_bytes = min_t(size_t, remaining, (size_t)port->tx_bytes_pw);
>> + for (c = 0; c < tx_bytes ; c++)
>> + buf |= (xmit->buf[tail + c] << (c * BITS_PER_BYTE));
>
> And then just put buf[c] = xmit->buf[tail + c] here?
Ok.
>
>> +
>> + writel_relaxed(buf, uport->membase + SE_GENI_TX_FIFOn);
>
> This also needs to be an iowrite32_rep(uport->membase, buf, 1) for
> endian reasons.
Ok.
>
>> +
>> + i += tx_bytes;
>> + tail = (tail + tx_bytes) & (UART_XMIT_SIZE - 1);
>> + uport->icount.tx += tx_bytes;
>> + remaining -= tx_bytes;
>> + }
>> + qcom_geni_serial_poll_tx_done(uport);
>> + port->xmit_size += chunk;
>> +out_write_wakeup:
>> + uart_write_wakeup(uport);
>> +}
>> +
>> +static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
>> +{
>> + unsigned int m_irq_status;
>> + unsigned int s_irq_status;
>> + struct uart_port *uport = dev;
>> + unsigned long flags;
>> + unsigned int m_irq_en;
>> + bool drop_rx = false;
>> + struct tty_port *tport = &uport->state->port;
>> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>> +
>> + if (uport->suspended)
>> + return IRQ_HANDLED;
>
> Is it a spurious IRQ if this happens? Return IRQ_NONE instead?
For the debug UART, this is a spurious IRQ. We can return IRQ_NONE.
>
>> +
>> + spin_lock_irqsave(&uport->lock, flags);
>> + m_irq_status = readl_relaxed(uport->membase + SE_GENI_M_IRQ_STATUS);
>> + s_irq_status = readl_relaxed(uport->membase + SE_GENI_S_IRQ_STATUS);
>> + m_irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
>> + writel_relaxed(m_irq_status, uport->membase + SE_GENI_M_IRQ_CLEAR);
>> + writel_relaxed(s_irq_status, uport->membase + SE_GENI_S_IRQ_CLEAR);
>> +
>> + if (WARN_ON(m_irq_status & M_ILLEGAL_CMD_EN))
>> + goto out_unlock;
>> +
>> + if (s_irq_status & S_RX_FIFO_WR_ERR_EN) {
>> + uport->icount.overrun++;
>> + tty_insert_flip_char(tport, 0, TTY_OVERRUN);
>> + }
>> +
>> + 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++;
>
> Maybe move this stat accounting to the place where brk is handled?
Since other error accounting like overrun, parity are happening here, it
feels logical to keep that accounting here.
>
>> + port->brk = true;
>> + }
>> +
>> + if (s_irq_status & S_RX_FIFO_WATERMARK_EN ||
>> + s_irq_status & S_RX_FIFO_LAST_EN)
>> + qcom_geni_serial_handle_rx(uport, drop_rx);
>> +
>> +out_unlock:
>> + spin_unlock_irqrestore(&uport->lock, flags);
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int get_tx_fifo_size(struct qcom_geni_serial_port *port)
>> +{
>> + struct uart_port *uport;
>> +
>> + if (!port)
>> + return -ENODEV;
>
> This happens?
It should not happen. I will remove the check.
>
>> +
>> + uport = &port->uport;
>> + port->tx_fifo_depth = geni_se_get_tx_fifo_depth(&port->se);
>> + port->tx_fifo_width = geni_se_get_tx_fifo_width(&port->se);
>> + port->rx_fifo_depth = geni_se_get_rx_fifo_depth(&port->se);
>> + uport->fifosize =
>> + (port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
>> + return 0;
>> +}
>> +
> [...]
>> +
>> +#ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
>> +static int __init qcom_geni_console_setup(struct console *co, char *options)
>> +{
>> + struct uart_port *uport;
>> + struct qcom_geni_serial_port *port;
>> + int baud;
>> + int bits = 8;
>> + int parity = 'n';
>> + int flow = 'n';
>> +
>> + if (co->index >= GENI_UART_CONS_PORTS || co->index < 0)
>> + return -ENXIO;
>> +
>> + port = get_port_from_line(co->index);
>> + if (IS_ERR(port)) {
>> + pr_err("Invalid line %d(%d)\n", co->index, (int)PTR_ERR(port));
>
> Use %ld to avoid casting the error value? Or just don't print it at all
> because it doesn't really help the end user.
Currently get_port_from_line returns only one error code. So I will
remove it from printing.
>
>> + return PTR_ERR(port);
>> + }
>> +
>> + uport = &port->uport;
>> +
>> + if (unlikely(!uport->membase))
>> + return -ENXIO;
>> +
>> + if (geni_se_resources_on(&port->se)) {
>> + dev_err(port->se.dev, "Error turning on resources\n");
>> + return -ENXIO;
>> + }
>> +
>> + if (unlikely(geni_se_read_proto(&port->se) != GENI_SE_UART)) {
>> + geni_se_resources_off(&port->se);
>> + return -ENXIO;
>> + }
>> +
>> + if (!port->setup) {
>> + port->tx_bytes_pw = 1;
>> + port->rx_bytes_pw = RX_BYTES_PW;
>> + qcom_geni_serial_stop_rx(uport);
>> + qcom_geni_serial_port_setup(uport);
>> + }
>> +
>> + if (options)
>> + uart_parse_options(options, &baud, &parity, &bits, &flow);
>> +
>> + return uart_set_options(uport, co, baud, parity, bits, flow);
>> +}
> [..]
>> +
>> +static int qcom_geni_serial_probe(struct platform_device *pdev)
>> +{
>> + int ret = 0;
>> + int line = -1;
>> + struct qcom_geni_serial_port *port;
>> + struct uart_port *uport;
>> + struct resource *res;
>> +
>> + if (pdev->dev.of_node)
>> + line = of_alias_get_id(pdev->dev.of_node, "serial");
>> + else
>> + line = pdev->id;
>
> Do we need to support the non-alias method?
Sorry, I forgot to remove the else part. I remember we agreed to go the
alias method.
>
>> +
>> + if (line < 0 || line >= GENI_UART_CONS_PORTS)
>> + return -ENXIO;
>> + port = get_port_from_line(line);
>> + if (IS_ERR(port)) {
>> + ret = PTR_ERR(port);
>> + dev_err(&pdev->dev, "Invalid line %d(%d)\n", line, ret);
>> + return ret;
>> + }
>> +
>> + uport = &port->uport;
>> + /* Don't allow 2 drivers to access the same port */
>> + if (uport->private_data)
>> + return -ENODEV;
>> +
>> + uport->dev = &pdev->dev;
>> + port->se.dev = &pdev->dev;
>> + port->se.wrapper = dev_get_drvdata(pdev->dev.parent);
>> + port->se.clk = devm_clk_get(&pdev->dev, "se");
>> + if (IS_ERR(port->se.clk)) {
>> + ret = PTR_ERR(port->se.clk);
>> + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
>> + return ret;
>> + }
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (IS_ERR(res))
>> + return PTR_ERR(res);
>> + uport->mapbase = res->start;
>> +
>> + port->tx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
>> + port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
>> + port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
>> +
>> + uport->irq = platform_get_irq(pdev, 0);
>> + if (uport->irq < 0) {
>> + dev_err(&pdev->dev, "Failed to get IRQ %d\n", uport->irq);
>> + return uport->irq;
>> + }
>> +
>> + uport->private_data = &qcom_geni_console_driver;
>> + platform_set_drvdata(pdev, port);
>> + port->handle_rx = handle_rx_console;
>> + port->setup = false;
>
> This would be set to false already?
Yes, it should be set to false already. This is redundant.
>
>> + return uart_add_one_port(&qcom_geni_console_driver, uport);
>> +}
>> +
>> +static int qcom_geni_serial_remove(struct platform_device *pdev)
>> +{
>> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
>> + struct uart_driver *drv = port->uport.private_data;
>> +
>> + uart_remove_one_port(drv, &port->uport);
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused qcom_geni_serial_sys_suspend_noirq(struct device *dev)
>> +{
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
>> + struct uart_port *uport = &port->uport;
>> +
>> + uart_suspend_port(uport->private_data, uport);
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused qcom_geni_serial_sys_resume_noirq(struct device *dev)
>> +{
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
>> + struct uart_port *uport = &port->uport;
>> +
>> + if (console_suspend_enabled && uport->suspended) {
>> + uart_resume_port(uport->private_data, uport);
>> + disable_irq(uport->irq);
>
> I missed the enable_irq() part. Is this still necessary?
Suspending the uart console port invokes the uart port shutdown
operation. The shutdown operation disables and frees the concerned IRQ.
Resuming the uart console port invokes the uart port startup operation
which requests for the IRQ. The request_irq operation auto-enables the
IRQ. In addition, resume_noirq implicitly enables the IRQ. This leads to
an unbalanced IRQ enable warning.
This disable_irq() helps with suppressing that warning.
>
>> + }
>> + return 0;
>> +}
>> +
>> +static int __init qcom_geni_serial_init(void)
>> +{
>> + int ret;
>> +
>> + qcom_geni_console_port.uport.iotype = UPIO_MEM;
>> + qcom_geni_console_port.uport.ops = &qcom_geni_console_pops;
>> + qcom_geni_console_port.uport.flags = UPF_BOOT_AUTOCONF;
>> + qcom_geni_console_port.uport.line = 0;
>
> Why can't this be done statically?
It can be done statically.
>> +
>> + ret = console_register(&qcom_geni_console_driver);
>> + if (ret)
>> + return ret;
>> +
>> + ret = platform_driver_register(&qcom_geni_serial_platform_driver);
>> + if (ret)
>> + console_unregister(&qcom_geni_console_driver);
>> + return ret;
>> +}
>> +module_init(qcom_geni_serial_init);
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 4/6] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
From: Karthik Ramasubramanian @ 2018-03-20 23:44 UTC (permalink / raw)
To: Evan Green
Cc: Jonathan Corbet, Andy Gross, David Brown, robh+dt, mark.rutland,
wsa, gregkh, linux-doc, linux-arm-msm, devicetree, linux-i2c,
linux-serial, jslaby, acourbot, swboyd, Girish Mahadevan,
Sagar Dharia, Doug Anderson
In-Reply-To: <CAE=gft7QAE0+3YpLu9BL2uvN=A0YgF=Xp2ocmbX=pvwszWtZew@mail.gmail.com>
On 3/20/2018 12:39 PM, Evan Green wrote:
> Hi Karthik,
>
> On Wed, Mar 14, 2018 at 4:59 PM Karthikeyan Ramasubramanian <
> kramasub@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 = 20000;
>> +
>> + /* 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.
>
>> +
>> + 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.
>
>> +
>> + 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.
Issuing a cancel command to the primary sequencer, makes the primary
sequencer to go to inactive state. Even though they are 2 different
registers, writing to one register impacts the other. From that
perspective, we want to ensure that the order is maintained. Else if the
cancel command goes through and then the GENI_STATUS is read, it might
say that the primary sequencer is not active and the function might
return prematurely. Same argument applies for the below mentioned cases.
>
>> +
>> + geni_se_cancel_m_cmd(&port->se);
>> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
>> + M_CMD_CANCEL_EN, true)) {
>> + geni_se_abort_m_cmd(&port->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)
>> +{
>> + u32 irq_en;
>> + u32 status;
>> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>> +
>> + status = readl_relaxed(uport->membase + SE_GENI_STATUS);
>> + if (status & S_GENI_CMD_ACTIVE)
>> + qcom_geni_serial_stop_rx(uport);
>> +
>> + /*
>> + * Ensure setup command write is not re-ordered before checking
>> + * the status of the Secondary Sequencer.
>> + */
>> + mb();
>
> Also here, I think the reordering you're worried about would mean the CPU
> is executing incorrectly.
>
>> +
>> + geni_se_setup_s_cmd(&port->se, UART_START_READ, 0);
>> +
>> + if (port->xfer_mode == GENI_SE_FIFO) {
>> + irq_en = readl_relaxed(uport->membase + SE_GENI_S_IRQ_EN);
>> + irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN;
>> + writel_relaxed(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
>> +
>> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
>> + irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
>> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
>> + }
>> +}
>> +
>> +static void qcom_geni_serial_stop_rx(struct uart_port *uport)
>> +{
>> + u32 irq_en;
>> + u32 status;
>> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>> + u32 irq_clear = S_CMD_DONE_EN;
>> +
>> + if (port->xfer_mode == GENI_SE_FIFO) {
>> + irq_en = readl_relaxed(uport->membase + SE_GENI_S_IRQ_EN);
>> + irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN);
>> + writel_relaxed(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
>> +
>> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
>> + irq_en &= ~(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
>> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
>> + }
>> +
>> + status = readl_relaxed(uport->membase + SE_GENI_STATUS);
>> + /* Possible stop rx is called multiple times. */
>> + if (!(status & S_GENI_CMD_ACTIVE))
>> + return;
>> +
>> + /*
>> + * Ensure cancel command write is not re-ordered before checking
>> + * the status of the Secondary Sequencer.
>> + */
>> + mb();
>
> Same deal here.
>
>> +
>> + geni_se_cancel_s_cmd(&port->se);
>> + qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
>> + S_GENI_CMD_CANCEL, false);
>> + status = readl_relaxed(uport->membase + SE_GENI_STATUS);
>> + writel_relaxed(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
>> + if (status & S_GENI_CMD_ACTIVE)
>> + qcom_geni_serial_abort_rx(uport);
>> +}
>> +
>
> Sorry gmail decided to wrap some of the context lines.
> -Evan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 4/6] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
From: Evan Green @ 2018-03-21 0:18 UTC (permalink / raw)
To: Karthikeyan Ramasubramanian
Cc: Jonathan Corbet, Andy Gross, David Brown, robh+dt, mark.rutland,
wsa, gregkh, linux-doc, linux-arm-msm, devicetree, linux-i2c,
linux-serial, jslaby, acourbot, swboyd, Girish Mahadevan,
Sagar Dharia, Doug Anderson
In-Reply-To: <1fa9262a-28bf-8432-5672-4f8c09898493@codeaurora.org>
On Tue, Mar 20, 2018 at 4:44 PM Karthik Ramasubramanian <
kramasub@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 <
> > kramasub@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 = 20000;
> >> +
> >> + /* 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 read actually completes, and then the writes happen before the
read? That seems like it couldn't possibly happen, as it would result in
weird spurious speculative writes to registers. Perhaps the mapping bits
prevent this sort of thing...)
If what I've said makes any sort of sense, and you still want to keep the
barriers here and below, then I'll let it go.
> >
> >> +
> >> + 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.
> Issuing a cancel command to the primary sequencer, makes the primary
> sequencer to go to inactive state. Even though they are 2 different
> registers, writing to one register impacts the other. From that
> perspective, we want to ensure that the order is maintained. Else if the
> cancel command goes through and then the GENI_STATUS is read, it might
> say that the primary sequencer is not active and the function might
> return prematurely. Same argument applies for the below mentioned cases.
> >
> >> +
> >> + geni_se_cancel_m_cmd(&port->se);
> >> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> >> + M_CMD_CANCEL_EN,
true)) {
> >> + geni_se_abort_m_cmd(&port->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)
> >> +{
> >> + u32 irq_en;
> >> + u32 status;
> >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> >> +
> >> + status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> >> + if (status & S_GENI_CMD_ACTIVE)
> >> + qcom_geni_serial_stop_rx(uport);
> >> +
> >> + /*
> >> + * Ensure setup command write is not re-ordered before checking
> >> + * the status of the Secondary Sequencer.
> >> + */
> >> + mb();
> >
> > Also here, I think the reordering you're worried about would mean the
CPU
> > is executing incorrectly.
> >
> >> +
> >> + geni_se_setup_s_cmd(&port->se, UART_START_READ, 0);
> >> +
> >> + if (port->xfer_mode == GENI_SE_FIFO) {
> >> + irq_en = readl_relaxed(uport->membase +
SE_GENI_S_IRQ_EN);
> >> + irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN;
> >> + writel_relaxed(irq_en, uport->membase +
SE_GENI_S_IRQ_EN);
> >> +
> >> + irq_en = readl_relaxed(uport->membase +
SE_GENI_M_IRQ_EN);
> >> + irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
> >> + writel_relaxed(irq_en, uport->membase +
SE_GENI_M_IRQ_EN);
> >> + }
> >> +}
> >> +
> >> +static void qcom_geni_serial_stop_rx(struct uart_port *uport)
> >> +{
> >> + u32 irq_en;
> >> + u32 status;
> >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> >> + u32 irq_clear = S_CMD_DONE_EN;
> >> +
> >> + if (port->xfer_mode == GENI_SE_FIFO) {
> >> + irq_en = readl_relaxed(uport->membase +
SE_GENI_S_IRQ_EN);
> >> + irq_en &= ~(S_RX_FIFO_WATERMARK_EN |
S_RX_FIFO_LAST_EN);
> >> + writel_relaxed(irq_en, uport->membase +
SE_GENI_S_IRQ_EN);
> >> +
> >> + irq_en = readl_relaxed(uport->membase +
SE_GENI_M_IRQ_EN);
> >> + irq_en &= ~(M_RX_FIFO_WATERMARK_EN |
M_RX_FIFO_LAST_EN);
> >> + writel_relaxed(irq_en, uport->membase +
SE_GENI_M_IRQ_EN);
> >> + }
> >> +
> >> + status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> >> + /* Possible stop rx is called multiple times. */
> >> + if (!(status & S_GENI_CMD_ACTIVE))
> >> + return;
> >> +
> >> + /*
> >> + * Ensure cancel command write is not re-ordered before
checking
> >> + * the status of the Secondary Sequencer.
> >> + */
> >> + mb();
> >
> > Same deal here.
> >
> >> +
> >> + geni_se_cancel_s_cmd(&port->se);
> >> + qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
> >> + S_GENI_CMD_CANCEL, false);
> >> + status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> >> + writel_relaxed(irq_clear, uport->membase +
SE_GENI_S_IRQ_CLEAR);
> >> + if (status & S_GENI_CMD_ACTIVE)
> >> + qcom_geni_serial_abort_rx(uport);
> >> +}
> >> +
> >
> > Sorry gmail decided to wrap some of the context lines.
> > -Evan
> > --
> > To unsubscribe from this list: send the line "unsubscribe
linux-arm-msm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> 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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox