* [PATCH 2/8] dt-bindings: serial: Add bindings for nvidia,tegra194-tcu
From: Mikko Perttunen @ 2018-05-08 11:43 UTC (permalink / raw)
To: robh+dt, mark.rutland, jassisinghbrar, gregkh, thierry.reding,
jonathanh
Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
linux-kernel, Mikko Perttunen
In-Reply-To: <20180508114403.14499-1-mperttunen@nvidia.com>
Add bindings for the Tegra Combined UART device used to talk to the
UART console on Tegra194 systems.
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
.../bindings/serial/nvidia,tegra194-tcu.txt | 35 ++++++++++++++++++++++
1 file changed, 35 insertions(+)
create mode 100644 Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt
diff --git a/Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt b/Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt
new file mode 100644
index 000000000000..86763bc5d74f
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt
@@ -0,0 +1,35 @@
+NVIDIA Tegra Combined UART (TCU)
+
+The TCU is a system for sharing a hardware UART instance among multiple
+systems withing the Tegra SoC. It is implemented through a mailbox-
+based protocol where each "virtual UART" has a pair of mailboxes, one
+for transmitting and one for receiving, that is used to communicate
+with the hardware implementing the TCU.
+
+Required properties:
+- name : Should be tcu
+- compatible
+ Array of strings
+ One of:
+ - "nvidia,tegra194-tcu"
+- mbox-names:
+ "rx" - Mailbox for receiving data from hardware UART
+ "tx" - Mailbox for transmitting data to hardware UART
+- mboxes: Mailboxes corresponding to the mbox-names.
+
+This node is a mailbox consumer. See the following files for details of
+the mailbox subsystem, and the specifiers implemented by the relevant
+provider(s):
+
+- .../mailbox/mailbox.txt
+- .../mailbox/nvidia,tegra186-hsp.txt
+
+Example bindings:
+-----------------
+
+tcu: tcu {
+ compatible = "nvidia,tegra194-tcu";
+ mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_SM 0>,
+ <&hsp_aon TEGRA_HSP_MBOX_TYPE_SM 1>;
+ mbox-names = "rx", "tx";
+};
--
2.16.1
^ permalink raw reply related
* [PATCH 3/8] mailbox: Add transmit done by blocking option
From: Mikko Perttunen @ 2018-05-08 11:43 UTC (permalink / raw)
To: robh+dt, mark.rutland, jassisinghbrar, gregkh, thierry.reding,
jonathanh
Cc: devicetree, araza, linux-kernel, Mikko Perttunen, linux-serial,
linux-tegra, linux-arm-kernel
In-Reply-To: <20180508114403.14499-1-mperttunen@nvidia.com>
Add a new TXDONE option, TXDONE_BY_BLOCK. With this option, the
send_data function of the mailbox driver is expected to block until
the message has been sent. The new option is used with the Tegra
Combined UART driver to minimize unnecessary overhead when transmitting
data.
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
drivers/mailbox/mailbox.c | 30 +++++++++++++++++++++---------
drivers/mailbox/mailbox.h | 1 +
2 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 674b35f402f5..5c76b70e673c 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -53,6 +53,8 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
return idx;
}
+static void tx_tick(struct mbox_chan *chan, int r, bool submit_next);
+
static void msg_submit(struct mbox_chan *chan)
{
unsigned count, idx;
@@ -60,10 +62,13 @@ static void msg_submit(struct mbox_chan *chan)
void *data;
int err = -EBUSY;
+next:
spin_lock_irqsave(&chan->lock, flags);
- if (!chan->msg_count || chan->active_req)
- goto exit;
+ if (!chan->msg_count || chan->active_req) {
+ spin_unlock_irqrestore(&chan->lock, flags);
+ return;
+ }
count = chan->msg_count;
idx = chan->msg_free;
@@ -82,15 +87,21 @@ static void msg_submit(struct mbox_chan *chan)
chan->active_req = data;
chan->msg_count--;
}
-exit:
+
spin_unlock_irqrestore(&chan->lock, flags);
if (!err && (chan->txdone_method & TXDONE_BY_POLL))
/* kick start the timer immediately to avoid delays */
hrtimer_start(&chan->mbox->poll_hrt, 0, HRTIMER_MODE_REL);
+
+ if (chan->txdone_method & TXDONE_BY_BLOCK) {
+ tx_tick(chan, err, false);
+ if (!err)
+ goto next;
+ }
}
-static void tx_tick(struct mbox_chan *chan, int r)
+static void tx_tick(struct mbox_chan *chan, int r, bool submit_next)
{
unsigned long flags;
void *mssg;
@@ -101,7 +112,8 @@ static void tx_tick(struct mbox_chan *chan, int r)
spin_unlock_irqrestore(&chan->lock, flags);
/* Submit next message */
- msg_submit(chan);
+ if (submit_next)
+ msg_submit(chan);
if (!mssg)
return;
@@ -127,7 +139,7 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
if (chan->active_req && chan->cl) {
txdone = chan->mbox->ops->last_tx_done(chan);
if (txdone)
- tx_tick(chan, 0);
+ tx_tick(chan, 0, true);
else
resched = true;
}
@@ -176,7 +188,7 @@ void mbox_chan_txdone(struct mbox_chan *chan, int r)
return;
}
- tx_tick(chan, r);
+ tx_tick(chan, r, true);
}
EXPORT_SYMBOL_GPL(mbox_chan_txdone);
@@ -196,7 +208,7 @@ void mbox_client_txdone(struct mbox_chan *chan, int r)
return;
}
- tx_tick(chan, r);
+ tx_tick(chan, r, true);
}
EXPORT_SYMBOL_GPL(mbox_client_txdone);
@@ -275,7 +287,7 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
ret = wait_for_completion_timeout(&chan->tx_complete, wait);
if (ret == 0) {
t = -ETIME;
- tx_tick(chan, t);
+ tx_tick(chan, t, true);
}
}
diff --git a/drivers/mailbox/mailbox.h b/drivers/mailbox/mailbox.h
index 456ba68513bb..ec68e2e28cd6 100644
--- a/drivers/mailbox/mailbox.h
+++ b/drivers/mailbox/mailbox.h
@@ -10,5 +10,6 @@
#define TXDONE_BY_IRQ BIT(0) /* controller has remote RTR irq */
#define TXDONE_BY_POLL BIT(1) /* controller can read status of last TX */
#define TXDONE_BY_ACK BIT(2) /* S/W ACK recevied by Client ticks the TX */
+#define TXDONE_BY_BLOCK BIT(3) /* mailbox driver send_data blocks until done */
#endif /* __MAILBOX_H */
--
2.16.1
^ permalink raw reply related
* [PATCH 4/8] mailbox: tegra-hsp: Refactor in preparation of mailboxes
From: Mikko Perttunen @ 2018-05-08 11:43 UTC (permalink / raw)
To: robh+dt, mark.rutland, jassisinghbrar, gregkh, thierry.reding,
jonathanh
Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
linux-kernel, Mikko Perttunen
In-Reply-To: <20180508114403.14499-1-mperttunen@nvidia.com>
The HSP driver is currently in many places written with the assumption
of only supporting doorbells. Prepare for the addition of shared
mailbox support by removing these assumptions and cleaning up the code.
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
drivers/mailbox/tegra-hsp.c | 124 +++++++++++++++++++++++++++++---------------
1 file changed, 82 insertions(+), 42 deletions(-)
diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
index 0cde356c11ab..16eb970f2c9f 100644
--- a/drivers/mailbox/tegra-hsp.c
+++ b/drivers/mailbox/tegra-hsp.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
+ * Copyright (c) 2016-2018, NVIDIA CORPORATION. All rights reserved.
*
* This program is free software; you can redistribute it and/or modify it
* under the terms and conditions of the GNU General Public License,
@@ -42,6 +42,7 @@ struct tegra_hsp_channel;
struct tegra_hsp;
struct tegra_hsp_channel {
+ unsigned int type;
struct tegra_hsp *hsp;
struct mbox_chan *chan;
void __iomem *regs;
@@ -55,6 +56,12 @@ struct tegra_hsp_doorbell {
unsigned int index;
};
+static inline struct tegra_hsp_doorbell *
+channel_to_doorbell(struct tegra_hsp_channel *channel)
+{
+ return container_of(channel, struct tegra_hsp_doorbell, channel);
+}
+
struct tegra_hsp_db_map {
const char *name;
unsigned int master;
@@ -69,7 +76,7 @@ struct tegra_hsp {
const struct tegra_hsp_soc *soc;
struct mbox_controller mbox;
void __iomem *regs;
- unsigned int irq;
+ unsigned int doorbell_irq;
unsigned int num_sm;
unsigned int num_as;
unsigned int num_ss;
@@ -194,7 +201,7 @@ tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const char *name,
if (!db)
return ERR_PTR(-ENOMEM);
- offset = (1 + (hsp->num_sm / 2) + hsp->num_ss + hsp->num_as) << 16;
+ offset = (1 + (hsp->num_sm / 2) + hsp->num_ss + hsp->num_as) * SZ_64K;
offset += index * 0x100;
db->channel.regs = hsp->regs + offset;
@@ -218,18 +225,8 @@ static void __tegra_hsp_doorbell_destroy(struct tegra_hsp_doorbell *db)
kfree(db);
}
-static int tegra_hsp_doorbell_send_data(struct mbox_chan *chan, void *data)
-{
- struct tegra_hsp_doorbell *db = chan->con_priv;
-
- tegra_hsp_channel_writel(&db->channel, 1, HSP_DB_TRIGGER);
-
- return 0;
-}
-
-static int tegra_hsp_doorbell_startup(struct mbox_chan *chan)
+static int tegra_hsp_doorbell_startup(struct tegra_hsp_doorbell *db)
{
- struct tegra_hsp_doorbell *db = chan->con_priv;
struct tegra_hsp *hsp = db->channel.hsp;
struct tegra_hsp_doorbell *ccplex;
unsigned long flags;
@@ -260,9 +257,8 @@ static int tegra_hsp_doorbell_startup(struct mbox_chan *chan)
return 0;
}
-static void tegra_hsp_doorbell_shutdown(struct mbox_chan *chan)
+static void tegra_hsp_doorbell_shutdown(struct tegra_hsp_doorbell *db)
{
- struct tegra_hsp_doorbell *db = chan->con_priv;
struct tegra_hsp *hsp = db->channel.hsp;
struct tegra_hsp_doorbell *ccplex;
unsigned long flags;
@@ -281,35 +277,61 @@ static void tegra_hsp_doorbell_shutdown(struct mbox_chan *chan)
spin_unlock_irqrestore(&hsp->lock, flags);
}
-static const struct mbox_chan_ops tegra_hsp_doorbell_ops = {
- .send_data = tegra_hsp_doorbell_send_data,
- .startup = tegra_hsp_doorbell_startup,
- .shutdown = tegra_hsp_doorbell_shutdown,
+static int tegra_hsp_send_data(struct mbox_chan *chan, void *data)
+{
+ struct tegra_hsp_channel *channel = chan->con_priv;
+ struct tegra_hsp_doorbell *db;
+
+ switch (channel->type) {
+ case TEGRA_HSP_MBOX_TYPE_DB:
+ db = channel_to_doorbell(channel);
+ tegra_hsp_channel_writel(&db->channel, 1, HSP_DB_TRIGGER);
+ }
+
+ return -EINVAL;
+}
+
+static int tegra_hsp_startup(struct mbox_chan *chan)
+{
+ struct tegra_hsp_channel *channel = chan->con_priv;
+
+ switch (channel->type) {
+ case TEGRA_HSP_MBOX_TYPE_DB:
+ return tegra_hsp_doorbell_startup(channel_to_doorbell(channel));
+ }
+
+ return -EINVAL;
+}
+
+static void tegra_hsp_shutdown(struct mbox_chan *chan)
+{
+ struct tegra_hsp_channel *channel = chan->con_priv;
+
+ switch (channel->type) {
+ case TEGRA_HSP_MBOX_TYPE_DB:
+ tegra_hsp_doorbell_shutdown(channel_to_doorbell(channel));
+ break;
+ }
+}
+
+static const struct mbox_chan_ops tegra_hsp_ops = {
+ .send_data = tegra_hsp_send_data,
+ .startup = tegra_hsp_startup,
+ .shutdown = tegra_hsp_shutdown,
};
-static struct mbox_chan *of_tegra_hsp_xlate(struct mbox_controller *mbox,
- const struct of_phandle_args *args)
+static struct mbox_chan *tegra_hsp_doorbell_xlate(struct tegra_hsp *hsp,
+ unsigned int master)
{
struct tegra_hsp_channel *channel = ERR_PTR(-ENODEV);
- struct tegra_hsp *hsp = to_tegra_hsp(mbox);
- unsigned int type = args->args[0];
- unsigned int master = args->args[1];
struct tegra_hsp_doorbell *db;
struct mbox_chan *chan;
unsigned long flags;
unsigned int i;
- switch (type) {
- case TEGRA_HSP_MBOX_TYPE_DB:
- db = tegra_hsp_doorbell_get(hsp, master);
- if (db)
- channel = &db->channel;
-
- break;
-
- default:
- break;
- }
+ db = tegra_hsp_doorbell_get(hsp, master);
+ if (db)
+ channel = &db->channel;
if (IS_ERR(channel))
return ERR_CAST(channel);
@@ -321,6 +343,7 @@ static struct mbox_chan *of_tegra_hsp_xlate(struct mbox_controller *mbox,
if (!chan->con_priv) {
chan->con_priv = channel;
channel->chan = chan;
+ channel->type = TEGRA_HSP_MBOX_TYPE_DB;
break;
}
@@ -332,6 +355,22 @@ static struct mbox_chan *of_tegra_hsp_xlate(struct mbox_controller *mbox,
return chan ?: ERR_PTR(-EBUSY);
}
+static struct mbox_chan *of_tegra_hsp_xlate(struct mbox_controller *mbox,
+ const struct of_phandle_args *args)
+{
+ struct tegra_hsp *hsp = to_tegra_hsp(mbox);
+ unsigned int type = args->args[0];
+ unsigned int param = args->args[1];
+
+ switch (type) {
+ case TEGRA_HSP_MBOX_TYPE_DB:
+ return tegra_hsp_doorbell_xlate(hsp, param);
+
+ default:
+ return ERR_PTR(-EINVAL);
+ }
+}
+
static void tegra_hsp_remove_doorbells(struct tegra_hsp *hsp)
{
struct tegra_hsp_doorbell *db, *tmp;
@@ -397,14 +436,14 @@ static int tegra_hsp_probe(struct platform_device *pdev)
return err;
}
- hsp->irq = err;
+ hsp->doorbell_irq = err;
hsp->mbox.of_xlate = of_tegra_hsp_xlate;
hsp->mbox.num_chans = 32;
hsp->mbox.dev = &pdev->dev;
hsp->mbox.txdone_irq = false;
hsp->mbox.txdone_poll = false;
- hsp->mbox.ops = &tegra_hsp_doorbell_ops;
+ hsp->mbox.ops = &tegra_hsp_ops;
hsp->mbox.chans = devm_kcalloc(&pdev->dev, hsp->mbox.num_chans,
sizeof(*hsp->mbox.chans),
@@ -427,11 +466,12 @@ static int tegra_hsp_probe(struct platform_device *pdev)
return err;
}
- err = devm_request_irq(&pdev->dev, hsp->irq, tegra_hsp_doorbell_irq,
- IRQF_NO_SUSPEND, dev_name(&pdev->dev), hsp);
+ err = devm_request_irq(&pdev->dev, hsp->doorbell_irq,
+ tegra_hsp_doorbell_irq, IRQF_NO_SUSPEND,
+ dev_name(&pdev->dev), hsp);
if (err < 0) {
- dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n",
- hsp->irq, err);
+ dev_err(&pdev->dev, "failed to request doorbell IRQ#%u: %d\n",
+ hsp->doorbell_irq, err);
return err;
}
--
2.16.1
^ permalink raw reply related
* [PATCH 5/8] mailbox: tegra-hsp: Add support for shared mailboxes
From: Mikko Perttunen @ 2018-05-08 11:44 UTC (permalink / raw)
To: robh+dt, mark.rutland, jassisinghbrar, gregkh, thierry.reding,
jonathanh
Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
linux-kernel, Mikko Perttunen
In-Reply-To: <20180508114403.14499-1-mperttunen@nvidia.com>
The Tegra HSP block supports 'shared mailboxes' that are simple 32-bit
registers consisting of a FULL bit in MSB position and 31 bits of data.
The hardware can be configured to trigger interrupts when a mailbox
is empty or full. Add support for these shared mailboxes to the HSP
driver.
The initial use for the mailboxes is the Tegra Combined UART. For this
purpose, we use interrupts to receive data, and spinning to wait for
the transmit mailbox to be emptied to minimize unnecessary overhead.
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
drivers/mailbox/tegra-hsp.c | 216 +++++++++++++++++++++++++++++++++++++++-----
1 file changed, 193 insertions(+), 23 deletions(-)
diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
index 16eb970f2c9f..77bc8ed7ef15 100644
--- a/drivers/mailbox/tegra-hsp.c
+++ b/drivers/mailbox/tegra-hsp.c
@@ -21,6 +21,11 @@
#include <dt-bindings/mailbox/tegra186-hsp.h>
+#include "mailbox.h"
+
+#define HSP_INT0_IE 0x100
+#define HSP_INT_IR 0x304
+
#define HSP_INT_DIMENSIONING 0x380
#define HSP_nSM_SHIFT 0
#define HSP_nSS_SHIFT 4
@@ -34,6 +39,8 @@
#define HSP_DB_RAW 0x8
#define HSP_DB_PENDING 0xc
+#define HSP_SM_SHRD_MBOX 0x0
+
#define HSP_DB_CCPLEX 1
#define HSP_DB_BPMP 3
#define HSP_DB_MAX 7
@@ -68,6 +75,18 @@ struct tegra_hsp_db_map {
unsigned int index;
};
+struct tegra_hsp_mailbox {
+ struct tegra_hsp_channel channel;
+ unsigned int index;
+ bool sending;
+};
+
+static inline struct tegra_hsp_mailbox *
+channel_to_mailbox(struct tegra_hsp_channel *channel)
+{
+ return container_of(channel, struct tegra_hsp_mailbox, channel);
+}
+
struct tegra_hsp_soc {
const struct tegra_hsp_db_map *map;
};
@@ -77,6 +96,7 @@ struct tegra_hsp {
struct mbox_controller mbox;
void __iomem *regs;
unsigned int doorbell_irq;
+ unsigned int shared_irq;
unsigned int num_sm;
unsigned int num_as;
unsigned int num_ss;
@@ -85,6 +105,7 @@ struct tegra_hsp {
spinlock_t lock;
struct list_head doorbells;
+ struct tegra_hsp_mailbox *mailboxes;
};
static inline struct tegra_hsp *
@@ -189,6 +210,35 @@ static irqreturn_t tegra_hsp_doorbell_irq(int irq, void *data)
return IRQ_HANDLED;
}
+static irqreturn_t tegra_hsp_shared_irq(int irq, void *data)
+{
+ struct tegra_hsp_mailbox *mb;
+ struct tegra_hsp *hsp = data;
+ unsigned long bit, mask;
+ u32 value;
+
+ mask = tegra_hsp_readl(hsp, HSP_INT_IR);
+ /* Only interested in FULL interrupts */
+ mask &= 0xff << 8;
+
+ for_each_set_bit(bit, &mask, 16) {
+ unsigned int mb_i = bit % 8;
+
+ mb = &hsp->mailboxes[mb_i];
+
+ if (!mb->sending) {
+ value = tegra_hsp_channel_readl(&mb->channel,
+ HSP_SM_SHRD_MBOX);
+ value &= ~BIT(31);
+ mbox_chan_received_data(mb->channel.chan, &value);
+ tegra_hsp_channel_writel(&mb->channel, value,
+ HSP_SM_SHRD_MBOX);
+ }
+ }
+
+ return IRQ_HANDLED;
+}
+
static struct tegra_hsp_channel *
tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const char *name,
unsigned int master, unsigned int index)
@@ -277,15 +327,58 @@ static void tegra_hsp_doorbell_shutdown(struct tegra_hsp_doorbell *db)
spin_unlock_irqrestore(&hsp->lock, flags);
}
+static int tegra_hsp_mailbox_startup(struct tegra_hsp_mailbox *mb)
+{
+ struct tegra_hsp *hsp = mb->channel.hsp;
+ u32 value;
+
+ mb->channel.chan->txdone_method = TXDONE_BY_BLOCK;
+
+ /* Route FULL interrupt to external IRQ 0 */
+ value = tegra_hsp_readl(hsp, HSP_INT0_IE);
+ value |= BIT(mb->index + 8);
+ tegra_hsp_writel(hsp, value, HSP_INT0_IE);
+
+ return 0;
+}
+
+static int tegra_hsp_mailbox_shutdown(struct tegra_hsp_mailbox *mb)
+{
+ struct tegra_hsp *hsp = mb->channel.hsp;
+ u32 value;
+
+ value = tegra_hsp_readl(hsp, HSP_INT0_IE);
+ value &= ~BIT(mb->index + 8);
+ tegra_hsp_writel(hsp, value, HSP_INT0_IE);
+
+ return 0;
+}
+
static int tegra_hsp_send_data(struct mbox_chan *chan, void *data)
{
struct tegra_hsp_channel *channel = chan->con_priv;
- struct tegra_hsp_doorbell *db;
+ struct tegra_hsp_mailbox *mailbox;
+ uint32_t value;
switch (channel->type) {
case TEGRA_HSP_MBOX_TYPE_DB:
- db = channel_to_doorbell(channel);
- tegra_hsp_channel_writel(&db->channel, 1, HSP_DB_TRIGGER);
+ tegra_hsp_channel_writel(channel, 1, HSP_DB_TRIGGER);
+ return 0;
+ case TEGRA_HSP_MBOX_TYPE_SM:
+ mailbox = channel_to_mailbox(channel);
+ mailbox->sending = true;
+
+ value = *(uint32_t *)data;
+ /* Mark mailbox full */
+ value |= BIT(31);
+
+ tegra_hsp_channel_writel(channel, value, HSP_SM_SHRD_MBOX);
+
+ while (tegra_hsp_channel_readl(channel, HSP_SM_SHRD_MBOX)
+ & BIT(31))
+ cpu_relax();
+
+ return 0;
}
return -EINVAL;
@@ -298,6 +391,8 @@ static int tegra_hsp_startup(struct mbox_chan *chan)
switch (channel->type) {
case TEGRA_HSP_MBOX_TYPE_DB:
return tegra_hsp_doorbell_startup(channel_to_doorbell(channel));
+ case TEGRA_HSP_MBOX_TYPE_SM:
+ return tegra_hsp_mailbox_startup(channel_to_mailbox(channel));
}
return -EINVAL;
@@ -311,6 +406,9 @@ static void tegra_hsp_shutdown(struct mbox_chan *chan)
case TEGRA_HSP_MBOX_TYPE_DB:
tegra_hsp_doorbell_shutdown(channel_to_doorbell(channel));
break;
+ case TEGRA_HSP_MBOX_TYPE_SM:
+ tegra_hsp_mailbox_shutdown(channel_to_mailbox(channel));
+ break;
}
}
@@ -364,7 +462,16 @@ static struct mbox_chan *of_tegra_hsp_xlate(struct mbox_controller *mbox,
switch (type) {
case TEGRA_HSP_MBOX_TYPE_DB:
- return tegra_hsp_doorbell_xlate(hsp, param);
+ if (hsp->doorbell_irq)
+ return tegra_hsp_doorbell_xlate(hsp, param);
+ else
+ return ERR_PTR(-EINVAL);
+
+ case TEGRA_HSP_MBOX_TYPE_SM:
+ if (hsp->shared_irq && param < hsp->num_sm)
+ return hsp->mailboxes[param].channel.chan;
+ else
+ return ERR_PTR(-EINVAL);
default:
return ERR_PTR(-EINVAL);
@@ -403,6 +510,31 @@ static int tegra_hsp_add_doorbells(struct tegra_hsp *hsp)
return 0;
}
+static int tegra_hsp_add_mailboxes(struct tegra_hsp *hsp, struct device *dev)
+{
+ int i;
+
+ hsp->mailboxes = devm_kcalloc(dev, hsp->num_sm, sizeof(*hsp->mailboxes),
+ GFP_KERNEL);
+ if (!hsp->mailboxes)
+ return -ENOMEM;
+
+ for (i = 0; i < hsp->num_sm; i++) {
+ struct tegra_hsp_mailbox *mb = &hsp->mailboxes[i];
+
+ mb->index = i;
+ mb->sending = false;
+
+ mb->channel.hsp = hsp;
+ mb->channel.type = TEGRA_HSP_MBOX_TYPE_SM;
+ mb->channel.regs = hsp->regs + SZ_64K + i * SZ_32K;
+ mb->channel.chan = &hsp->mbox.chans[i];
+ mb->channel.chan->con_priv = &mb->channel;
+ }
+
+ return 0;
+}
+
static int tegra_hsp_probe(struct platform_device *pdev)
{
struct tegra_hsp *hsp;
@@ -431,14 +563,19 @@ static int tegra_hsp_probe(struct platform_device *pdev)
hsp->num_si = (value >> HSP_nSI_SHIFT) & HSP_nINT_MASK;
err = platform_get_irq_byname(pdev, "doorbell");
- if (err < 0) {
- dev_err(&pdev->dev, "failed to get doorbell IRQ: %d\n", err);
- return err;
- }
+ if (err < 0)
+ hsp->doorbell_irq = 0;
+ else
+ hsp->doorbell_irq = err;
- hsp->doorbell_irq = err;
+ err = platform_get_irq_byname(pdev, "shared0");
+ if (err < 0)
+ hsp->shared_irq = 0;
+ else
+ hsp->shared_irq = err;
hsp->mbox.of_xlate = of_tegra_hsp_xlate;
+ /* First nSM are reserved for mailboxes */
hsp->mbox.num_chans = 32;
hsp->mbox.dev = &pdev->dev;
hsp->mbox.txdone_irq = false;
@@ -451,10 +588,22 @@ static int tegra_hsp_probe(struct platform_device *pdev)
if (!hsp->mbox.chans)
return -ENOMEM;
- err = tegra_hsp_add_doorbells(hsp);
- if (err < 0) {
- dev_err(&pdev->dev, "failed to add doorbells: %d\n", err);
- return err;
+ if (hsp->doorbell_irq) {
+ err = tegra_hsp_add_doorbells(hsp);
+ if (err < 0) {
+ dev_err(&pdev->dev, "failed to add doorbells: %d\n",
+ err);
+ return err;
+ }
+ }
+
+ if (hsp->shared_irq) {
+ err = tegra_hsp_add_mailboxes(hsp, &pdev->dev);
+ if (err < 0) {
+ dev_err(&pdev->dev, "failed to add mailboxes: %d\n",
+ err);
+ goto remove_doorbells;
+ }
}
platform_set_drvdata(pdev, hsp);
@@ -462,20 +611,40 @@ static int tegra_hsp_probe(struct platform_device *pdev)
err = mbox_controller_register(&hsp->mbox);
if (err) {
dev_err(&pdev->dev, "failed to register mailbox: %d\n", err);
- tegra_hsp_remove_doorbells(hsp);
- return err;
+ goto remove_doorbells;
}
- err = devm_request_irq(&pdev->dev, hsp->doorbell_irq,
- tegra_hsp_doorbell_irq, IRQF_NO_SUSPEND,
- dev_name(&pdev->dev), hsp);
- if (err < 0) {
- dev_err(&pdev->dev, "failed to request doorbell IRQ#%u: %d\n",
- hsp->doorbell_irq, err);
- return err;
+ if (hsp->doorbell_irq) {
+ err = devm_request_irq(&pdev->dev, hsp->doorbell_irq,
+ tegra_hsp_doorbell_irq, IRQF_NO_SUSPEND,
+ dev_name(&pdev->dev), hsp);
+ if (err < 0) {
+ dev_err(&pdev->dev,
+ "failed to request doorbell IRQ#%u: %d\n",
+ hsp->doorbell_irq, err);
+ return err;
+ }
+ }
+
+ if (hsp->shared_irq) {
+ err = devm_request_irq(&pdev->dev, hsp->shared_irq,
+ tegra_hsp_shared_irq, 0,
+ dev_name(&pdev->dev), hsp);
+ if (err < 0) {
+ dev_err(&pdev->dev,
+ "failed to request shared0 IRQ%u: %d\n",
+ hsp->shared_irq, err);
+ return err;
+ }
}
return 0;
+
+remove_doorbells:
+ if (hsp->doorbell_irq)
+ tegra_hsp_remove_doorbells(hsp);
+
+ return err;
}
static int tegra_hsp_remove(struct platform_device *pdev)
@@ -483,7 +652,8 @@ static int tegra_hsp_remove(struct platform_device *pdev)
struct tegra_hsp *hsp = platform_get_drvdata(pdev);
mbox_controller_unregister(&hsp->mbox);
- tegra_hsp_remove_doorbells(hsp);
+ if (hsp->doorbell_irq)
+ tegra_hsp_remove_doorbells(hsp);
return 0;
}
--
2.16.1
^ permalink raw reply related
* [PATCH 6/8] serial: Add Tegra Combined UART driver
From: Mikko Perttunen @ 2018-05-08 11:44 UTC (permalink / raw)
To: robh+dt, mark.rutland, jassisinghbrar, gregkh, thierry.reding,
jonathanh
Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
linux-kernel, Mikko Perttunen
In-Reply-To: <20180508114403.14499-1-mperttunen@nvidia.com>
The Tegra Combined UART (TCU) is a mailbox-based mechanism that allows
multiplexing multiple "virtual UARTs" into a single hardware serial
port. The TCU is the primary serial port on Tegra194 devices.
Add a TCU driver utilizing the mailbox framework, as the used mailboxes
are part of Tegra HSP blocks that are already controlled by the Tegra
HSP mailbox driver.
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
drivers/tty/serial/Kconfig | 9 ++
drivers/tty/serial/Makefile | 1 +
drivers/tty/serial/tegra-tcu.c | 302 +++++++++++++++++++++++++++++++++++++++
include/uapi/linux/serial_core.h | 3 +
4 files changed, 315 insertions(+)
create mode 100644 drivers/tty/serial/tegra-tcu.c
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index eca55187539a..494db7afe230 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -322,6 +322,15 @@ config SERIAL_TEGRA
are enabled). This driver uses the APB DMA to achieve higher baudrate
and better performance.
+config SERIAL_TEGRA_TCU
+ tristate "NVIDIA Tegra Combined UART"
+ depends on ARCH_TEGRA && MAILBOX
+ select SERIAL_CORE
+ help
+ Support for the mailbox-based TCU (Tegra Combined UART) serial port.
+ TCU is a virtual serial port that allows multiplexing multiple data
+ streams into a single hardware serial port.
+
config SERIAL_MAX3100
tristate "MAX3100 support"
depends on SPI
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index daac675612df..4ad82231ff8a 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_SERIAL_LANTIQ) += lantiq.o
obj-$(CONFIG_SERIAL_XILINX_PS_UART) += xilinx_uartps.o
obj-$(CONFIG_SERIAL_SIRFSOC) += sirfsoc_uart.o
obj-$(CONFIG_SERIAL_TEGRA) += serial-tegra.o
+obj-$(CONFIG_SERIAL_TEGRA_TCU) += tegra-tcu.o
obj-$(CONFIG_SERIAL_AR933X) += ar933x_uart.o
obj-$(CONFIG_SERIAL_EFM32_UART) += efm32-uart.o
obj-$(CONFIG_SERIAL_ARC) += arc_uart.o
diff --git a/drivers/tty/serial/tegra-tcu.c b/drivers/tty/serial/tegra-tcu.c
new file mode 100644
index 000000000000..8b46fe3a4b0c
--- /dev/null
+++ b/drivers/tty/serial/tegra-tcu.c
@@ -0,0 +1,302 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
+ */
+
+#include <linux/console.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/serial.h>
+#include <linux/serial_core.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+
+static struct uart_driver tegra_tcu_uart_driver;
+static struct uart_port tegra_tcu_uart_port;
+
+struct tegra_tcu {
+ struct mbox_client tx_client, rx_client;
+ struct mbox_chan *tx, *rx;
+};
+
+static unsigned int tegra_tcu_uart_tx_empty(struct uart_port *port)
+{
+ return TIOCSER_TEMT;
+}
+
+static void tegra_tcu_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+ (void)port;
+ (void)mctrl;
+}
+
+static unsigned int tegra_tcu_uart_get_mctrl(struct uart_port *port)
+{
+ return 0;
+}
+
+static void tegra_tcu_uart_stop_tx(struct uart_port *port)
+{
+ (void)port;
+}
+
+static void tegra_tcu_write(const char *s, unsigned int count)
+{
+ struct tegra_tcu *tcu = tegra_tcu_uart_port.private_data;
+ unsigned int written = 0, i = 0;
+ bool insert_nl = false;
+ uint32_t value = 0;
+
+ while (i < count) {
+ if (insert_nl) {
+ value |= '\n' << (written++ * 8);
+ insert_nl = false;
+ i++;
+ } else if (s[i] == '\n') {
+ value |= '\r' << (written++ * 8);
+ insert_nl = true;
+ } else {
+ value |= s[i++] << (written++ * 8);
+ }
+
+ if (written == 3) {
+ value |= 3 << 24;
+ value |= BIT(26);
+ mbox_send_message(tcu->tx, &value);
+ value = 0;
+ written = 0;
+ }
+ }
+
+ if (written) {
+ value |= written << 24;
+ value |= BIT(26);
+ mbox_send_message(tcu->tx, &value);
+ }
+}
+
+static void tegra_tcu_uart_start_tx(struct uart_port *port)
+{
+ struct circ_buf *xmit = &port->state->xmit;
+ unsigned long count;
+
+ for (;;) {
+ count = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
+ if (!count)
+ break;
+
+ tegra_tcu_write(&xmit->buf[xmit->tail], count);
+ xmit->tail = (xmit->tail + count) & (UART_XMIT_SIZE - 1);
+ }
+
+ uart_write_wakeup(port);
+}
+
+static void tegra_tcu_uart_stop_rx(struct uart_port *port)
+{
+ (void)port;
+}
+
+static void tegra_tcu_uart_break_ctl(struct uart_port *port, int ctl)
+{
+ (void)port;
+ (void)ctl;
+}
+
+static int tegra_tcu_uart_startup(struct uart_port *port)
+{
+ (void)port;
+
+ return 0;
+}
+
+static void tegra_tcu_uart_shutdown(struct uart_port *port)
+{
+ (void)port;
+}
+
+static void tegra_tcu_uart_set_termios(struct uart_port *port,
+ struct ktermios *new,
+ struct ktermios *old)
+{
+ (void)port;
+ (void)new;
+ (void)old;
+}
+
+static const struct uart_ops tegra_tcu_uart_ops = {
+ .tx_empty = tegra_tcu_uart_tx_empty,
+ .set_mctrl = tegra_tcu_uart_set_mctrl,
+ .get_mctrl = tegra_tcu_uart_get_mctrl,
+ .stop_tx = tegra_tcu_uart_stop_tx,
+ .start_tx = tegra_tcu_uart_start_tx,
+ .stop_rx = tegra_tcu_uart_stop_rx,
+ .break_ctl = tegra_tcu_uart_break_ctl,
+ .startup = tegra_tcu_uart_startup,
+ .shutdown = tegra_tcu_uart_shutdown,
+ .set_termios = tegra_tcu_uart_set_termios,
+};
+
+static void tegra_tcu_console_write(struct console *cons, const char *s,
+ unsigned int count)
+{
+ (void)cons;
+
+ tegra_tcu_write(s, count);
+}
+
+static int tegra_tcu_console_setup(struct console *cons, char *options)
+{
+ (void)options;
+
+ if (!tegra_tcu_uart_port.private_data)
+ return -ENODEV;
+
+ return uart_set_options(&tegra_tcu_uart_port, cons,
+ 115200, 'n', 8, 'n');
+}
+
+static struct console tegra_tcu_console = {
+ .name = "ttyTCU",
+ .device = uart_console_device,
+ .flags = CON_PRINTBUFFER | CON_ANYTIME,
+ .index = -1,
+ .write = tegra_tcu_console_write,
+ .setup = tegra_tcu_console_setup,
+ .data = &tegra_tcu_uart_driver,
+};
+
+static struct uart_driver tegra_tcu_uart_driver = {
+ .owner = THIS_MODULE,
+ .driver_name = "tegra-tcu",
+ .dev_name = "ttyTCU",
+ .cons = &tegra_tcu_console,
+ .nr = 1,
+};
+
+static void tegra_tcu_receive(struct mbox_client *client, void *msg_p)
+{
+ struct tty_port *port = &tegra_tcu_uart_port.state->port;
+ uint32_t msg = *(uint32_t *)msg_p;
+ unsigned int num_bytes;
+ int i;
+
+ num_bytes = (msg >> 24) & 0x3;
+ for (i = 0; i < num_bytes; i++)
+ tty_insert_flip_char(port, (msg >> (i*8)) & 0xff, TTY_NORMAL);
+
+ tty_flip_buffer_push(port);
+}
+
+static int tegra_tcu_probe(struct platform_device *pdev)
+{
+ struct uart_port *port = &tegra_tcu_uart_port;
+ struct tegra_tcu *tcu;
+ int err;
+
+ tcu = devm_kzalloc(&pdev->dev, sizeof(*tcu), GFP_KERNEL);
+ if (!tcu)
+ return -ENOMEM;
+
+ tcu->tx_client.dev = &pdev->dev;
+ tcu->rx_client.dev = &pdev->dev;
+ tcu->rx_client.rx_callback = tegra_tcu_receive;
+
+ tcu->tx = mbox_request_channel_byname(&tcu->tx_client, "tx");
+ if (IS_ERR(tcu->tx)) {
+ err = PTR_ERR(tcu->tx);
+ dev_err(&pdev->dev, "failed to get tx mailbox: %d\n", err);
+ return err;
+ }
+
+ tcu->rx = mbox_request_channel_byname(&tcu->rx_client, "rx");
+ if (IS_ERR(tcu->rx)) {
+ err = PTR_ERR(tcu->rx);
+ dev_err(&pdev->dev, "failed to get rx mailbox: %d\n", err);
+ goto free_tx;
+ }
+
+ err = uart_register_driver(&tegra_tcu_uart_driver);
+ if (err) {
+ dev_err(&pdev->dev, "failed to register UART driver: %d\n",
+ err);
+ goto free_rx;
+ }
+
+ spin_lock_init(&port->lock);
+ port->dev = &pdev->dev;
+ port->type = PORT_TEGRA_TCU;
+ port->ops = &tegra_tcu_uart_ops;
+ port->fifosize = 1;
+ port->iotype = UPIO_MEM;
+ port->flags = UPF_BOOT_AUTOCONF;
+ port->private_data = tcu;
+
+ err = uart_add_one_port(&tegra_tcu_uart_driver, port);
+ if (err) {
+ dev_err(&pdev->dev, "failed to add UART port: %d\n", err);
+ goto unregister_uart;
+ }
+
+ return 0;
+
+unregister_uart:
+ uart_unregister_driver(&tegra_tcu_uart_driver);
+free_rx:
+ mbox_free_channel(tcu->rx);
+free_tx:
+ mbox_free_channel(tcu->tx);
+
+ return err;
+}
+
+static int tegra_tcu_remove(struct platform_device *pdev)
+{
+ uart_remove_one_port(&tegra_tcu_uart_driver, &tegra_tcu_uart_port);
+ uart_unregister_driver(&tegra_tcu_uart_driver);
+
+ return 0;
+}
+
+static const struct of_device_id tegra_tcu_match[] = {
+ { .compatible = "nvidia,tegra194-tcu" },
+ { }
+};
+
+static struct platform_driver tegra_tcu_driver = {
+ .driver = {
+ .name = "tegra-tcu",
+ .of_match_table = tegra_tcu_match,
+ },
+ .probe = tegra_tcu_probe,
+ .remove = tegra_tcu_remove,
+};
+
+static int __init tegra_tcu_init(void)
+{
+ int err;
+
+ err = platform_driver_register(&tegra_tcu_driver);
+ if (err)
+ return err;
+
+ register_console(&tegra_tcu_console);
+
+ return 0;
+}
+module_init(tegra_tcu_init);
+
+static void __exit tegra_tcu_exit(void)
+{
+ unregister_console(&tegra_tcu_console);
+ platform_driver_unregister(&tegra_tcu_driver);
+}
+module_exit(tegra_tcu_exit);
+
+MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("NVIDIA Tegra Combined UART driver");
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index dce5f9dae121..eaf3c303cba6 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -281,4 +281,7 @@
/* MediaTek BTIF */
#define PORT_MTK_BTIF 117
+/* NVIDIA Tegra Combined UART */
+#define PORT_TEGRA_TCU 118
+
#endif /* _UAPILINUX_SERIAL_CORE_H */
--
2.16.1
^ permalink raw reply related
* [PATCH 7/8] arm64: tegra: Add nodes for tcu on Tegra194
From: Mikko Perttunen @ 2018-05-08 11:44 UTC (permalink / raw)
To: robh+dt, mark.rutland, jassisinghbrar, gregkh, thierry.reding,
jonathanh
Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
linux-kernel, Mikko Perttunen
In-Reply-To: <20180508114403.14499-1-mperttunen@nvidia.com>
Add nodes required for communication through the Tegra Combined UART.
This includes the AON HSP instance, addition of shared interrupts
for the TOP0 HSP instance, and finally the TCU node itself. Also
mark the HSP instances as compatible to tegra194-hsp, as the hardware
is not identical but is compatible to tegra186-hsp.
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
arch/arm64/boot/dts/nvidia/tegra194.dtsi | 34 +++++++++++++++++++++++++++++---
1 file changed, 31 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index 6d699815a84f..d7f780b06fe2 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -217,10 +217,31 @@
};
hsp_top0: hsp@3c00000 {
- compatible = "nvidia,tegra186-hsp";
+ compatible = "nvidia,tegra194-hsp", "nvidia,tegra186-hsp";
reg = <0x03c00000 0xa0000>;
- interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
- interrupt-names = "doorbell";
+ interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 127 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "doorbell", "shared0", "shared1", "shared2",
+ "shared3", "shared4", "shared5", "shared6",
+ "shared7";
+ #mbox-cells = <2>;
+ };
+
+ hsp_aon: hsp@c150000 {
+ compatible = "nvidia,tegra194-hsp", "nvidia,tegra186-hsp";
+ reg = <0x0c150000 0xa0000>;
+ interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "shared0", "shared1", "shared2", "shared3";
#mbox-cells = <2>;
};
@@ -382,6 +403,13 @@
};
};
+ tcu: tcu {
+ compatible = "nvidia,tegra194-tcu";
+ mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_SM 0>,
+ <&hsp_aon TEGRA_HSP_MBOX_TYPE_SM 1>;
+ mbox-names = "rx", "tx";
+ };
+
timer {
compatible = "arm,armv8-timer";
interrupts = <GIC_PPI 13
--
2.16.1
^ permalink raw reply related
* [PATCH 8/8] arm64: tegra: Mark tcu as primary serial port on Tegra194 P2888
From: Mikko Perttunen @ 2018-05-08 11:44 UTC (permalink / raw)
To: robh+dt, mark.rutland, jassisinghbrar, gregkh, thierry.reding,
jonathanh
Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
linux-kernel, Mikko Perttunen
In-Reply-To: <20180508114403.14499-1-mperttunen@nvidia.com>
The Tegra Combined UART is the proper primary serial port on P2888,
so use it.
Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi b/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi
index 859ab5af17c1..95e2433984f7 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi
@@ -10,7 +10,7 @@
aliases {
sdhci0 = "/cbb/sdhci@3460000";
sdhci1 = "/cbb/sdhci@3400000";
- serial0 = &uartb;
+ serial0 = &tcu;
i2c0 = "/bpmp/i2c";
i2c1 = "/cbb/i2c@3160000";
i2c2 = "/cbb/i2c@c240000";
--
2.16.1
^ permalink raw reply related
* Re: [PATCH] tty: pl011: Avoid spuriously stuck-off interrupts
From: Dave Martin @ 2018-05-08 12:44 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Peter Maydell, Andrew Jones, Ciro Santilli, Linus Walleij, Wei Xu,
linux-serial, linux-arm-kernel
In-Reply-To: <20180508110119.GA16141@n2100.armlinux.org.uk>
On Tue, May 08, 2018 at 12:01:19PM +0100, Russell King - ARM Linux wrote:
> On Fri, Apr 27, 2018 at 11:05:45AM +0100, Dave Martin wrote:
> > Commit 9b96fbacda34 ("serial: PL011: clear pending interrupts")
> > clears the RX and receive timeout interrupts on pl011 startup, to
> > avoid a screaming-interrupt scenario that can occur when the
> > firmware or bootloader leaves these interrupts asserted.
> >
> > This has been noted as an issue when running Linux on qemu [1].
> >
> > Unfortunately, the above fix seems to lead to potential
> > misbehaviour if the RX FIFO interrupt is asserted _non_ spuriously
> > on driver startup, if the RX FIFO is also already full to the
> > trigger level.
> >
> > Clearing the RX FIFO interrupt does not change the FIFO fill level.
> > In this scenario, because the interrupt is now clear and because
> > the FIFO is already full to the trigger level, no new assertion of
> > the RX FIFO interrupt can occur unless the FIFO is drained back
> > below the trigger level. This never occurs because the pl011
> > driver is waiting for an RX FIFO interrupt to tell it that there is
> > something to read, and does not read the FIFO at all until that
> > interrupt occurs.
> >
> > Thus, simply clearing "spurious" interrupts on startup may be
> > misguided, since there is no way to be sure that the interrupts are
> > truly spurious, and things can go wrong if they are not.
> >
> > This patch instead clears the interrupt condition by draining the
> > RX FIFO during UART startup, after clearing any potentially
> > spurious interrupt. This should ensure that an interrupt will
> > definitely be asserted if the RX FIFO subsequently becomes
> > sufficiently full.
> >
> > The drain is done at the point of enabling interrupts only. This
> > means that it will occur any time the UART is newly opened through
> > the tty layer. It will not apply to polled-mode use of the UART by
> > kgdboc: since that scenario cannot use interrupts by design, this
> > should not matter. kgdboc will interact badly with "normal" use of
> > the UART in any case: this patch makes no attempt to paper over
> > such issues.
> >
> > This patch does not attempt to address the case where the RX FIFO
> > fills faster than it can be drained: that is a pathological
> > hardware design problem that is beyond the scope of the driver to
> > work around.
> >
> > [1] [Qemu-devel] [Qemu-arm] [PATCH] pl011: do not put into fifo
> > before enabled the interruption
> > https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06446.html
> >
> > Reported-by: Wei Xu <xuwei5@hisilicon.com>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Fixes: 9b96fbacda34 ("serial: PL011: clear pending interrupts")
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > Tested-by: Andrew Jones <drjones@redhat.com>
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > ---
> > drivers/tty/serial/amba-pl011.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> > index 4b40a5b..a6ccb85 100644
> > --- a/drivers/tty/serial/amba-pl011.c
> > +++ b/drivers/tty/serial/amba-pl011.c
> > @@ -1731,6 +1731,16 @@ static void pl011_enable_interrupts(struct uart_amba_port *uap)
> >
> > /* Clear out any spuriously appearing RX interrupts */
> > pl011_write(UART011_RTIS | UART011_RXIS, uap, REG_ICR);
> > +
> > + /*
> > + * RXIS is asserted only when the RX FIFO transitions from below
> > + * to above the trigger threshold. If the RX FIFO is already
> > + * full to the threshold this can't happen and RXIS will now be
> > + * stuck off. Drain the RX FIFO explicitly to fix this:
> > + */
> > + while (!(pl011_read(uap, REG_FR) & UART01x_FR_RXFE))
> > + pl011_read(uap, REG_DR);
>
> You probably ought to limit this in case of a stuck receive not empty
> case, maybe to (eg) twice the depth of the FIFO?
My argument would have been that if the FIFO fills too fast to be
drained, then you have more serious problems... but it would be better
for this to show as character loss than a lockup.
So, I guess draining twice the FIFO depth before giving up is a
reasonable compromise.
I'll hack that in and repost.
Cheers
---Dave
^ permalink raw reply
* Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown
From: Sebastian Reichel @ 2018-05-08 13:40 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
Shawn Guo, linux-kernel
In-Reply-To: <20180508064351.ioczgw2v4jtryr3x@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1861 bytes --]
Hi,
On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > This properly unmaps DMA SG on device shutdown.
> >
> > Reported-by: Nandor Han <nandor.han@ge.com>
> > Suggested-by: Nandor Han <nandor.han@ge.com>
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > ---
> > drivers/tty/serial/imx.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 3ca767b1162a..6c53e74244ec 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> > u32 ucr1, ucr2;
> >
> > if (sport->dma_is_enabled) {
> > - sport->dma_is_rxing = 0;
> > - sport->dma_is_txing = 0;
> > dmaengine_terminate_sync(sport->dma_chan_tx);
> > + if (sport->dma_is_txing) {
> > + dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
> > + sport->dma_tx_nents, DMA_TO_DEVICE);
> > + sport->dma_is_txing = 0;
> > + }
>
> did you find this because the kernel crashed or consumed more and more
> memory, or is this "only" a finding of reading the source code? If the
> former it would be great to point out in the commit log, if the latter,
> I wonder if this is a real problem that warrants a stable backport.
A bit of both. One of Collabora's customers had a (scarce) kernel crash
in imx-serial and modified multiple things in the driver. The crash is
gone, but it's not clear which change fixed it. I could not
reproduce the crash so far and I'm currently rebasing and splitting
their changes into upstreamable portions with proper patch
descriptions. From reading the source this looked like a real issue.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown
From: Uwe Kleine-König @ 2018-05-08 18:46 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
Shawn Guo, linux-kernel
In-Reply-To: <20180508134047.zocurxwelw3a24ti@earth.universe>
Hello Sebastian,
On Tue, May 08, 2018 at 03:40:47PM +0200, Sebastian Reichel wrote:
> On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> > On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > > This properly unmaps DMA SG on device shutdown.
> > >
> > > Reported-by: Nandor Han <nandor.han@ge.com>
> > > Suggested-by: Nandor Han <nandor.han@ge.com>
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > ---
> > > drivers/tty/serial/imx.c | 12 ++++++++++--
> > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > index 3ca767b1162a..6c53e74244ec 100644
> > > --- a/drivers/tty/serial/imx.c
> > > +++ b/drivers/tty/serial/imx.c
> > > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> > > u32 ucr1, ucr2;
> > >
> > > if (sport->dma_is_enabled) {
> > > - sport->dma_is_rxing = 0;
> > > - sport->dma_is_txing = 0;
> > > dmaengine_terminate_sync(sport->dma_chan_tx);
> > > + if (sport->dma_is_txing) {
> > > + dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
> > > + sport->dma_tx_nents, DMA_TO_DEVICE);
> > > + sport->dma_is_txing = 0;
> > > + }
> >
> > did you find this because the kernel crashed or consumed more and more
> > memory, or is this "only" a finding of reading the source code? If the
> > former it would be great to point out in the commit log, if the latter,
> > I wonder if this is a real problem that warrants a stable backport.
>
> A bit of both. One of Collabora's customers had a (scarce) kernel crash
> in imx-serial and modified multiple things in the driver. The crash is
> gone, but it's not clear which change fixed it. I could not
> reproduce the crash so far and I'm currently rebasing and splitting
> their changes into upstreamable portions with proper patch
> descriptions. From reading the source this looked like a real issue.
In which context (kernel version, operating mode (e.g. rs485)) did these
happen? What does "crash" mean? The kernel did just hang or produced an
oops? If the latter, can you show it/them?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()
From: Jacek Anaszewski @ 2018-05-08 19:33 UTC (permalink / raw)
To: Uwe Kleine-König, Greg Kroah-Hartman, Jiri Slaby,
Johan Hovold, Pavel Machek
Cc: linux-serial, linux-leds, linux-can, kernel, One Thousand Gnomes,
Florian Fainelli, Mathieu Poirier, linux-kernel, Robin Murphy,
linux-arm-kernel
In-Reply-To: <20180508100543.12559-2-u.kleine-koenig@pengutronix.de>
Hi Uwe,
Thank you for the patch. It looks fine, but please split
the drivers/net/can/led.c related changes into a separate one.
Best regards,
Jacek Anaszewski
On 05/08/2018 12:05 PM, Uwe Kleine-König wrote:
> This allows one to simplify drivers that provide a trigger with a
> non-constant name (e.g. one trigger per device with the trigger name
> depending on the device's name).
>
> Internally the memory the name member of struct led_trigger points to
> now always allocated dynamically instead of just taken from the caller.
>
> The function led_trigger_rename_static() must be changed accordingly and
> was renamed to led_trigger_rename() for consistency, with the only user
> adapted.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> drivers/leds/led-triggers.c | 84 +++++++++++++++++++++++++++----------
> drivers/net/can/led.c | 6 +--
> include/linux/leds.h | 30 +++++++------
> 3 files changed, 81 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 431123b048a2..5d8bb504b07b 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -175,18 +175,34 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
> }
> EXPORT_SYMBOL_GPL(led_trigger_set_default);
>
> -void led_trigger_rename_static(const char *name, struct led_trigger *trig)
> +int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...)
> {
> - /* new name must be on a temporary string to prevent races */
> - BUG_ON(name == trig->name);
> + const char *prevname;
> + const char *newname;
> + va_list args;
> +
> + if (!trig)
> + return 0;
> +
> + va_start(args, fmt);
> + newname = kvasprintf_const(GFP_KERNEL, fmt, args);
> + va_end(args);
> +
> + if (!newname) {
> + pr_err("Failed to allocate new name for trigger %s\n", trig->name);
> + return -ENOMEM;
> + }
>
> down_write(&triggers_list_lock);
> - /* this assumes that trig->name was originaly allocated to
> - * non constant storage */
> - strcpy((char *)trig->name, name);
> + prevname = trig->name;
> + trig->name = newname;
> up_write(&triggers_list_lock);
> +
> + kfree_const(prevname);
> +
> + return 0;
> }
> -EXPORT_SYMBOL_GPL(led_trigger_rename_static);
> +EXPORT_SYMBOL_GPL(led_trigger_rename);
>
> /* LED Trigger Interface */
>
> @@ -333,34 +349,56 @@ void led_trigger_blink_oneshot(struct led_trigger *trig,
> }
> EXPORT_SYMBOL_GPL(led_trigger_blink_oneshot);
>
> -void led_trigger_register_simple(const char *name, struct led_trigger **tp)
> +int led_trigger_register_format(struct led_trigger **tp, const char *fmt, ...)
> {
> + va_list args;
> struct led_trigger *trig;
> - int err;
> + int err = -ENOMEM;
> + const char *name;
> +
> + va_start(args, fmt);
> + name = kvasprintf_const(GFP_KERNEL, fmt, args);
> + va_end(args);
>
> trig = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);
>
> - if (trig) {
> - trig->name = name;
> - err = led_trigger_register(trig);
> - if (err < 0) {
> - kfree(trig);
> - trig = NULL;
> - pr_warn("LED trigger %s failed to register (%d)\n",
> - name, err);
> - }
> - } else {
> - pr_warn("LED trigger %s failed to register (no memory)\n",
> - name);
> - }
> + if (!name || !trig)
> + goto err;
> +
> + trig->name = name;
> +
> + err = led_trigger_register(trig);
> + if (err < 0)
> + goto err;
> +
> *tp = trig;
> +
> + return 0;
> +
> +err:
> + kfree(trig);
> + kfree_const(name);
> +
> + *tp = NULL;
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(led_trigger_register_format);
> +
> +void led_trigger_register_simple(const char *name, struct led_trigger **tp)
> +{
> + int ret = led_trigger_register_format(tp, "%s", name);
> + if (ret < 0)
> + pr_warn("LED trigger %s failed to register (%d)\n", name, ret);
> }
> EXPORT_SYMBOL_GPL(led_trigger_register_simple);
>
> void led_trigger_unregister_simple(struct led_trigger *trig)
> {
> - if (trig)
> + if (trig) {
> led_trigger_unregister(trig);
> + kfree_const(trig->name);
> + }
> kfree(trig);
> }
> EXPORT_SYMBOL_GPL(led_trigger_unregister_simple);
> diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
> index c1b667675fa1..2d7d1b0d20f9 100644
> --- a/drivers/net/can/led.c
> +++ b/drivers/net/can/led.c
> @@ -115,13 +115,13 @@ static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
>
> if (msg == NETDEV_CHANGENAME) {
> snprintf(name, sizeof(name), "%s-tx", netdev->name);
> - led_trigger_rename_static(name, priv->tx_led_trig);
> + led_trigger_rename(priv->tx_led_trig, name);
>
> snprintf(name, sizeof(name), "%s-rx", netdev->name);
> - led_trigger_rename_static(name, priv->rx_led_trig);
> + led_trigger_rename(priv->rx_led_trig, name);
>
> snprintf(name, sizeof(name), "%s-rxtx", netdev->name);
> - led_trigger_rename_static(name, priv->rxtx_led_trig);
> + led_trigger_rename(priv->rxtx_led_trig, name);
> }
>
> return NOTIFY_DONE;
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index b7e82550e655..e706c28bb35b 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -275,6 +275,8 @@ extern void led_trigger_unregister(struct led_trigger *trigger);
> extern int devm_led_trigger_register(struct device *dev,
> struct led_trigger *trigger);
>
> +extern int led_trigger_register_format(struct led_trigger **trigger,
> + const char *fmt, ...);
> extern void led_trigger_register_simple(const char *name,
> struct led_trigger **trigger);
> extern void led_trigger_unregister_simple(struct led_trigger *trigger);
> @@ -298,28 +300,25 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
> }
>
> /**
> - * led_trigger_rename_static - rename a trigger
> - * @name: the new trigger name
> + * led_trigger_rename - rename a trigger
> * @trig: the LED trigger to rename
> + * @fmt: format string for new name
> *
> - * Change a LED trigger name by copying the string passed in
> - * name into current trigger name, which MUST be large
> - * enough for the new string.
> - *
> - * Note that name must NOT point to the same string used
> - * during LED registration, as that could lead to races.
> - *
> - * This is meant to be used on triggers with statically
> - * allocated name.
> + * rebaptize the given trigger.
> */
> -extern void led_trigger_rename_static(const char *name,
> - struct led_trigger *trig);
> +extern int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...);
>
> #else
>
> /* Trigger has no members */
> struct led_trigger {};
>
> +static inline int led_trigger_register_format(struct led_trigger **trigger,
> + const char *fmt, ...)
> +{
> + return 0;
> +}
> +
> /* Trigger inline empty functions */
> static inline void led_trigger_register_simple(const char *name,
> struct led_trigger **trigger) {}
> @@ -342,6 +341,11 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
> return NULL;
> }
>
> +static inline int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...)
> +{
> + return 0;
> +}
> +
> #endif /* CONFIG_LEDS_TRIGGERS */
>
> /* Trigger specific functions */
>
^ permalink raw reply
* Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()
From: Uwe Kleine-König @ 2018-05-08 20:17 UTC (permalink / raw)
To: Jacek Anaszewski, Marc Kleine-Budde
Cc: Greg Kroah-Hartman, Jiri Slaby, Johan Hovold, Pavel Machek,
One Thousand Gnomes, Florian Fainelli, linux-serial,
Mathieu Poirier, linux-kernel, linux-can, linux-arm-kernel,
kernel, Robin Murphy, linux-leds
In-Reply-To: <ea34ce99-d446-03d2-b38d-d40a22480337@gmail.com>
Hello Jacek,
On Tue, May 08, 2018 at 09:33:14PM +0200, Jacek Anaszewski wrote:
> Thank you for the patch. It looks fine, but please split
> the drivers/net/can/led.c related changes into a separate one.
I renamed led_trigger_rename_static() to led_trigger_rename() (and
changed the parameters). The can change just adapts the only user of
led_trigger_rename_static() to use the new one.
It's not impossible to separate this patches, but I wonder if it's worth
the effort.
The first patch would be like the patch under discussion, just without
the can bits and introducing something like:
/*
* compat stuff to be removed once the only caller is converted
*/
static inline led_trigger_rename_static(const char *name, struct led_trigger *trig)
{
(void)led_trigger_rename(trig, "%s", name);
}
Then the second patch would just be the 6-line can hunk. And a third
patch would remove the compat function. (Maybe I'd choose to squash the
two can patches together then, but this doesn't reduce the overhead
considerably.) The only upside I can see here is that it increases my
patch count, but it's otherwise not worth the effort for such an easy
change. Further more as there is a strict dependency on these three
patches this either delays the cleanup or (IMHO more likely) the can
change would go in via the led tree anyhow. (Mark already acked patch 2
of this series and in private confirmed that the agrees to let this
change go in via the led tree, too.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)
From: Johan Hovold @ 2018-05-09 9:18 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Johan Hovold, Tony Lindgren, H. Nikolaus Schaller,
Andreas Kemnade, Mark Rutland, Arnd Bergmann, Pavel Machek,
linux-kernel@vger.kernel.org,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Rob Herring, linux-serial, linux-pm
In-Reply-To: <20180508155608.3bzcbepsmoskhlox@earth.universe>
[ Changing the subject line as this is thread is no longer about DT
bindings.
Also adding linux-serial and linux-pm while keeping some context. ]
On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> Hi,
>
> On Mon, May 07, 2018 at 06:34:39PM +0200, Johan Hovold wrote:
> > On Mon, May 07, 2018 at 08:45:15AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan@kernel.org> [180507 03:03]:
> > > > On Fri, May 04, 2018 at 01:42:13PM +0200, Sebastian Reichel wrote:
> > > >
> > > > > Having said all of this, serdev does not yet support runtime PM (at
> > > > > all). Tony is currently looking into it. Fortunately serdev allows
> > > > > us to enable runtime PM by default (once implemented), since we know
> > > > > the remote side and can (hopefully) avoid losing characters (i.e.
> > > > > with sideband wakeup gpios).
> > > >
> > > > I'm not sure we want generic runtime-pm support for the controllers in
> > > > the sense that the slave device state is always reflected by the serial
> > > > controller. Similar as for i2c and spi, we really only want to keep the
> > > > controller active when we are doing I/O, but we may want to keep a
> > > > client active for longer.
> > >
> > > Yeah i2c seems to do the right thing where the bus takes care
> > > of runtime PM.
> >
> > Yeah, but since serial is async in contrast to i2c/spi, we may not be
> > able to push this entirely into core. The serdev drivers may need to
> > indicate when they expect or need to do I/O by opening and closing the
> > port. And this is how I implemented these first couple of gnss drivers.
> >
> > Also note that most serial driver do not do runtime pm while the port is
> > open (as OMAP does), and doing so also has the drawbacks of lost
> > characters etc. as Sebastian mentioned.
>
> I think using open/close for runtime pm is good enough for GPS,
> since it regularly sends data and draws lots of power anyways.
> But devices, that have an out-of-band wakeup signal can do proper
> runtime PM of the serial port without loosing characters.
Yeah, there may be some applications where this is possible. And this is
not the case for GPS, but not just because of a generally higher power
consumption, but due to the fact that we cannot afford having the first
message in every report burst be dropped.
> Note, that OMAP does not reach deep idle states with active
> serial port. This is not acceptable for low power devices.
Sure, but note that OMAP is the only serial driver which currently
implements this kind of aggressive runtime PM (besides a couple of
usb-serial drivers). This means that a serdev driver can never rely on
this being the case, and therefore needs to be restrictive about how
long the port is kept open if it cares about power at all.
> > > > Take the u-blox driver in this series for example. As I'm using runtime
> > > > PM to manage device power, user-space can chose to prevent the receiver
> > > > from runtime suspending in order to avoid lengthy (re-)acquisition times
> > > > in setups without a backup battery (by means of the power/control
> > > > attribute).
> > >
> > > Sorry I don't seem to have that one, care to paste the subject
> > > line of that patch?
> >
> > "[PATCH 5/7] gnss: add driver for u-blox receivers"
> >
> > https://lkml.kernel.org/r/20180424163458.11947-6-johan@kernel.org
> >
> > > > Note that serdev not enabling runtime pm for controllers is roughly
> > > > equivalent to setting the .ignore_children flag, which is what we do for
> > > > i2c and spi controller, and possibly what we want here too.
>
> For I2C/SPI this works, since receive operations are initiated by
> the controller. Unfortunately its harder to implement for async
> serial. But I agree, that we may want to have runtime PM for the
> serdev client and .ignore_children is the way to go.
It's really about adding runtime PM support to serdev controllers.
Serdev client drivers can already use runtime PM (as mentioned above).
The ignore_children flag would then allow the controller RPM state to be
managed independently of the child/slave device state when more fine
grained RPM control is desired.
> I think the client API should allow two things:
>
> 1. minimal runtime PM support: The controller is runtime enabled
> on serdev open and disabled on serdev close. This may be enough
> for some clients and useful for writing new drivers.
Right, and we already have something like this today by means of how the
serial driver implement runtime PM (i.e. they are generally active while
the port is open).
> 2. full runtime PM support: The controller is sleeping by default
> even with serdev open. The calls to write/change port settings/...
> automatically enables the device, similar to i2c/spi. But there
> must be additional functions to enable/disable runtime PM based
> on a wakeup gpio or similar out-of-band information. It may be
> enough to just provide:
>
> int serdev_pm_runtime_get_sync(struct serdev_device *serdev) {
> pm_runtime_get_sync(&serdev->dev);
> }
>
> int serdev_pm_runtime_put_autosuspend(struct serdev_device *serdev) {
> pm_runtime_put_autosuspend(&serdev->dev);
> }
I'm not a big fan of rpm wrappers and prefer using the RPM interface
directly, but that's a side note. In the above case we really want to
manage the controller (&serdev->ctrl->dev) rather than the client
however (.ignore_children should be set, remember).
Also note that a serial driver implementing aggressive RPM (e.g. using
autosuspend) would manage the RPM counts itself when changing terminal
settings etc, so the only thing that would be needed is for the
client/slave device to resume the controller pro-actively when it is
expecting incoming data.
To make this more concrete; an example could be a device with an OOB
wake-up signal, but using a request-response type protocol so that the
client driver knows when it's safe to allow the controller to again
suspend.
We can model this similarly to how we do it for usb-serial, namely that
the core takes an extra RPM reference at open, which a sufficiently
power aware driver can then chose to drop in order to allow for more
aggressive controller runtime PM (should the underlying device and driver
support it).
I've cooked up a patch which I'll be sending as a reply to this mail.
Thanks,
Johan
^ permalink raw reply
* [PATCH 1/2] serdev: add controller runtime PM support
From: Johan Hovold @ 2018-05-09 9:44 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rob Herring, Sebastian Reichel, Tony Lindgren,
H. Nikolaus Schaller, Andreas Kemnade, Mark Rutland,
Arnd Bergmann, Pavel Machek, linux-kernel, linux-serial, linux-pm,
Johan Hovold
Add support for controller runtime power management to serdev core. This
is needed to allow slave drivers to manage the runtime PM state of the
underlying serial controller when its driver, in turn, implements more
aggressive runtime power management (e.g. using autosuspend).
For some applications, for example, where loss off initial data after a
remote-wakeup event is acceptable or where rx is not used at all,
aggressive serial controller runtime PM may be used without further
involvement of the slave driver. But when this is not the case, the
slave driver must be able to indicate when incoming data is expected in
order to avoid data loss.
To facilitate the common case, where the serial controller power state
is active whenever the port is open (which is the case with just about
every serial driver), and where data loss is not acceptable and cannot
even be prevented by explicit controller runtime power management, an
RPM reference is taken in serdev open and put again at close. This
reference can later be balanced by any serdev driver which wants and/or
can handle aggressive controller runtime PM.
Note that the .ignore_children flag is set for the serdev controller to
allow the underlying hardware to idle when no I/O is expected, regardless
of the slave device RPM state.
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/tty/serdev/core.c | 33 ++++++++++++++++++++++++++++++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index df93b727e984..e5e84303faca 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -13,6 +13,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/pm_runtime.h>
#include <linux/serdev.h>
#include <linux/slab.h>
@@ -143,11 +144,28 @@ EXPORT_SYMBOL_GPL(serdev_device_remove);
int serdev_device_open(struct serdev_device *serdev)
{
struct serdev_controller *ctrl = serdev->ctrl;
+ int ret;
if (!ctrl || !ctrl->ops->open)
return -EINVAL;
- return ctrl->ops->open(ctrl);
+ ret = ctrl->ops->open(ctrl);
+ if (ret)
+ return ret;
+
+ ret = pm_runtime_get_sync(&ctrl->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(&ctrl->dev);
+ goto err_close;
+ }
+
+ return 0;
+
+err_close:
+ if (ctrl->ops->close)
+ ctrl->ops->close(ctrl);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(serdev_device_open);
@@ -158,6 +176,8 @@ void serdev_device_close(struct serdev_device *serdev)
if (!ctrl || !ctrl->ops->close)
return;
+ pm_runtime_put(&ctrl->dev);
+
ctrl->ops->close(ctrl);
}
EXPORT_SYMBOL_GPL(serdev_device_close);
@@ -416,6 +436,9 @@ struct serdev_controller *serdev_controller_alloc(struct device *parent,
dev_set_name(&ctrl->dev, "serial%d", id);
+ pm_runtime_no_callbacks(&ctrl->dev);
+ pm_suspend_ignore_children(&ctrl->dev, true);
+
dev_dbg(&ctrl->dev, "allocated controller 0x%p id %d\n", ctrl, id);
return ctrl;
@@ -547,20 +570,23 @@ int serdev_controller_add(struct serdev_controller *ctrl)
if (ret)
return ret;
+ pm_runtime_enable(&ctrl->dev);
+
ret_of = of_serdev_register_devices(ctrl);
ret_acpi = acpi_serdev_register_devices(ctrl);
if (ret_of && ret_acpi) {
dev_dbg(&ctrl->dev, "no devices registered: of:%d acpi:%d\n",
ret_of, ret_acpi);
ret = -ENODEV;
- goto out_dev_del;
+ goto err_rpm_disable;
}
dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
ctrl->nr, &ctrl->dev);
return 0;
-out_dev_del:
+err_rpm_disable:
+ pm_runtime_disable(&ctrl->dev);
device_del(&ctrl->dev);
return ret;
};
@@ -591,6 +617,7 @@ void serdev_controller_remove(struct serdev_controller *ctrl)
dummy = device_for_each_child(&ctrl->dev, NULL,
serdev_remove_device);
+ pm_runtime_disable(&ctrl->dev);
device_del(&ctrl->dev);
}
EXPORT_SYMBOL_GPL(serdev_controller_remove);
--
2.17.0
^ permalink raw reply related
* [PATCH EXAMPLE 2/2] dbg: gnss: sirf: allow aggressive controller runtime PM
From: Johan Hovold @ 2018-05-09 9:44 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rob Herring, Sebastian Reichel, Tony Lindgren,
H. Nikolaus Schaller, Andreas Kemnade, Mark Rutland,
Arnd Bergmann, Pavel Machek, linux-kernel, linux-serial, linux-pm,
Johan Hovold
In-Reply-To: <20180509094419.13470-1-johan@kernel.org>
This is just an example of how a serdev driver could go about to allow
aggressive controller runtime PM by dropping the RPM reference taken by
serdev core in serdev_device_open().
Note that for most GNSS devices this does not make any sense, as
allowing the controller to suspend this way would cause the
first message of every report burst to be corrupted (and discarded).
This one applies on top of the GNSS series available here:
https://lkml.kernel.org/r/20180424163458.11947-1-johan@kernel.org
Not-Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/gnss/sirf.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index 497f8eb8467f..31b2cbccd194 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -57,6 +57,9 @@ static int sirf_open(struct gnss_device *gdev)
goto err_close;
}
+ /* Allow aggresive controller runtime PM. */
+ pm_runtime_put(&serdev->ctrl->dev);
+
return 0;
err_close:
@@ -70,6 +73,9 @@ static void sirf_close(struct gnss_device *gdev)
struct sirf_data *data = gnss_get_drvdata(gdev);
struct serdev_device *serdev = data->serdev;
+ /* Balance the put in open() */
+ pm_runtime_get(&serdev->ctrl->dev);
+
serdev_device_close(serdev);
pm_runtime_put(&serdev->dev);
--
2.17.0
^ permalink raw reply related
* Re: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)
From: Johan Hovold @ 2018-05-09 9:49 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Johan Hovold, Tony Lindgren, H. Nikolaus Schaller,
Andreas Kemnade, Mark Rutland, Arnd Bergmann, Pavel Machek,
linux-kernel@vger.kernel.org,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Rob Herring, linux-serial, linux-pm
In-Reply-To: <20180509091831.GA2285@localhost>
On Wed, May 09, 2018 at 11:18:31AM +0200, Johan Hovold wrote:
> I've cooked up a patch which I'll be sending as a reply to this mail.
Forgot to add the in-reply-to header of course. For the interested, the
patch can be found here:
https://lkml.kernel.org/r/20180509094419.13470-1-johan@kernel.org
Johan
^ permalink raw reply
* Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown
From: Sebastian Reichel @ 2018-05-09 10:20 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
Shawn Guo, linux-kernel
In-Reply-To: <20180508184612.iixr3psap4ik5fdr@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 9210 bytes --]
Hi,
On Tue, May 08, 2018 at 08:46:12PM +0200, Uwe Kleine-König wrote:
> On Tue, May 08, 2018 at 03:40:47PM +0200, Sebastian Reichel wrote:
> > On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> > > On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > > > This properly unmaps DMA SG on device shutdown.
> > > >
> > > > Reported-by: Nandor Han <nandor.han@ge.com>
> > > > Suggested-by: Nandor Han <nandor.han@ge.com>
> > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > > ---
> > > > drivers/tty/serial/imx.c | 12 ++++++++++--
> > > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > index 3ca767b1162a..6c53e74244ec 100644
> > > > --- a/drivers/tty/serial/imx.c
> > > > +++ b/drivers/tty/serial/imx.c
> > > > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> > > > u32 ucr1, ucr2;
> > > >
> > > > if (sport->dma_is_enabled) {
> > > > - sport->dma_is_rxing = 0;
> > > > - sport->dma_is_txing = 0;
> > > > dmaengine_terminate_sync(sport->dma_chan_tx);
> > > > + if (sport->dma_is_txing) {
> > > > + dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
> > > > + sport->dma_tx_nents, DMA_TO_DEVICE);
> > > > + sport->dma_is_txing = 0;
> > > > + }
> > >
> > > did you find this because the kernel crashed or consumed more and more
> > > memory, or is this "only" a finding of reading the source code? If the
> > > former it would be great to point out in the commit log, if the latter,
> > > I wonder if this is a real problem that warrants a stable backport.
> >
> > A bit of both. One of Collabora's customers had a (scarce) kernel crash
> > in imx-serial and modified multiple things in the driver. The crash is
> > gone, but it's not clear which change fixed it. I could not
> > reproduce the crash so far and I'm currently rebasing and splitting
> > their changes into upstreamable portions with proper patch
> > descriptions. From reading the source this looked like a real issue.
>
> In which context (kernel version, operating mode (e.g. rs485)) did these
> happen? What does "crash" mean? The kernel did just hang or produced an
> oops? If the latter, can you show it/them?
I pasted the oops, that triggered writing the patches (Linux 4.8, no
rs485, 4MHz baudrate). I think, that the actual issue has already been
fixed upstream between 4.13 and current master.
-- Sebastian
...
[ 302.516696] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0938000
[ 302.524394] pgd = 80004000
[ 302.527111] [f0938000] *pgd=de81a811, *pte=53fc0653, *ppte=53fc0453
[ 302.533451] Internal error: : 1008 [#1] SMP ARM
[ 302.537994] Modules linked in: smsc95xx usbnet atmel_mxt_ts
[ 302.543651] CPU: 0 PID: 357 Comm: _ACFRead Not tainted 4.8.0 #1
[ 302.549578] Hardware name: Freescale i.MX53 (Device Tree Support)
[ 302.555679] task: edbf1f00 task.stack: ed5ce000
[ 302.560228] PC is at imx_rxint+0x5c/0x228
[ 302.564261] LR is at lock_acquired+0x494/0x57c
...
[ 303.055953] Backtrace:
[ 303.058430] [<804b3b48>] (imx_rxint) from [<804b4df8>] (imx_int+0x2ec/0x404)
[ 303.065484] r10:00000000 r9:00000000 r8:00000030 r7:00000030 r6:00005099 r5:00007240
[ 303.073406] r4:eeafdc10
[ 303.075975] [<804b4b0c>] (imx_int) from [<80182f10>] (__handle_irq_event_percpu+0x4c/0x3e8)
[ 303.084331] r10:00000000 r9:810025c4 r8:00000030 r7:00000001 r6:ee81c210 r5:ee81c200
[ 303.092252] r4:edcbe040
[ 303.094814] [<80182ec4>] (__handle_irq_event_percpu) from [<801832d8>] (handle_irq_event_percpu+0x2
c/0x68)
[ 303.104472] r10:1240dc08 r9:00000001 r8:ee81e400 r7:00000001 r6:ee81c210 r5:ee81c200
[ 303.112393] r4:ee81c200
[ 303.114954] [<801832ac>] (handle_irq_event_percpu) from [<8018335c>] (handle_irq_event+0x48/0x6c)
[ 303.123832] r5:ee81c260 r4:ee81c200
[ 303.127454] [<80183314>] (handle_irq_event) from [<80186ba0>] (handle_level_irq+0xb8/0x154)
[ 303.135810] r7:00000001 r6:ee81c210 r5:ee81c260 r4:ee81c200
[ 303.141547] [<80186ae8>] (handle_level_irq) from [<80182420>] (generic_handle_irq+0x30/0x44)
[ 303.149990] r7:00000001 r6:00000000 r5:00000030 r4:80ff1e8c
[ 303.155728] [<801823f0>] (generic_handle_irq) from [<801827a0>] (__handle_domain_irq+0x60/0xc8)
[ 303.164439] [<80182740>] (__handle_domain_irq) from [<80101530>] (tzic_handle_irq+0x74/0x9c)
[ 303.172882] r9:00000001 r8:ed5cfae8 r7:00000001 r6:00000020 r5:8108a50c r4:00000000
[ 303.180734] [<801014bc>] (tzic_handle_irq) from [<8094c630>] (__irq_svc+0x70/0x98)
[ 303.188311] Exception stack(0xed5cfae8 to 0xed5cfb30)
[ 303.193374] fae0: 00000283 edbf23c0 edbf1f00 600b0113 edbf2458 00000004
[ 303.201563] fb00: 00000014 00000004 818762ec edbf23c8 1240dc08 ed5cfb94 eee55200 ed5cfb38
[ 303.209748] fb20: 00000282 80177d1c 600b0113 ffffffff
[ 303.214805] r9:ed5ce000 r8:818762ec r7:ed5cfb1c r6:ffffffff r5:600b0113 r4:80177d1c
[ 303.222649] [<80177a50>] (lock_release) from [<8094bba0>] (_raw_spin_unlock+0x28/0x34)
[ 303.230572] r10:00000008 r9:ed5a5500 r8:eeafdc10 r7:ef07c000 r6:00000000 r5:edb86f40
[ 303.238493] r4:8101bd68
[ 303.241057] [<8094bb78>] (_raw_spin_unlock) from [<8025fdb4>] (remove_vm_area+0x54/0x70)
[ 303.249153] r5:edb86f40 r4:edb86100
[ 303.252771] [<8025fd60>] (remove_vm_area) from [<8025fdfc>] (__vunmap+0x2c/0xf8)
[ 303.260173] r5:f0c0b000 r4:f0c0b000
[ 303.263791] [<8025fdd0>] (__vunmap) from [<8026000c>] (vunmap+0x50/0x5c)
[ 303.270497] r7:ef07c000 r6:00001000 r5:f0c0b000 r4:00000000
[ 303.276240] [<8025ffbc>] (vunmap) from [<805262e4>] (dma_common_free_remap+0x64/0x74)
[ 303.284076] r5:20000008 r4:f0c0b000
[ 303.287697] [<80526280>] (dma_common_free_remap) from [<80117404>] (cma_allocator_free+0x7c/0x84)
[ 303.296574] r7:ef07c000 r6:00000000 r5:00001000 r4:effd9f80
[ 303.302312] [<80117388>] (cma_allocator_free) from [<80116dcc>] (__arm_dma_free+0xf0/0x13c)
[ 303.310668] r7:ef07c000 r6:f0c0b000 r5:f0c0b000 r4:edb869c0
[ 303.316405] [<80116cdc>] (__arm_dma_free) from [<80116e78>] (arm_dma_free+0x2c/0x34)
[ 303.324153] r5:edcc2010 r4:80116e4c
[ 303.327779] [<80116e4c>] (arm_dma_free) from [<8047dba8>] (sdma_free_chan_resources+0xc4/0x110)
[ 303.336490] [<8047dae4>] (sdma_free_chan_resources) from [<8047a9d8>] (dma_chan_put+0x88/0xbc)
[ 303.345107] r7:600b0113 r6:00000000 r5:edcc07ec r4:edcc07ec
[ 303.350845] [<8047a950>] (dma_chan_put) from [<8047aa44>] (dma_release_channel+0x38/0xa8)
[ 303.359028] r5:edcc07ec r4:edcc07ec
[ 303.362647] [<8047aa0c>] (dma_release_channel) from [<804b3e1c>] (imx_uart_dma_exit+0x50/0xfc)
[ 303.371263] r5:edcc07ec r4:eeafdc10
[ 303.374882] [<804b3dcc>] (imx_uart_dma_exit) from [<804b5028>] (imx_shutdown+0x118/0x20c)
[ 303.383065] r5:00000b01 r4:eeafdc10
[ 303.386689] [<804b4f10>] (imx_shutdown) from [<804ae834>] (uart_shutdown+0x120/0x17c)
[ 303.394525] r7:81031774 r6:ee8863a4 r5:eeafdc10 r4:ee886258
[ 303.400264] [<804ae714>] (uart_shutdown) from [<804b0624>] (uart_close+0x164/0x254)
[ 303.407926] r9:ed5a5500 r8:ee8863ac r7:ee886310 r6:eeafdc10 r5:edba9800 r4:ee886258
[ 303.415768] [<804b04c0>] (uart_close) from [<80492300>] (tty_release+0x104/0x498)
[ 303.423256] r9:ed5a5500 r8:00000000 r7:ee00e000 r6:00000000 r5:eeac2e60 r4:edba9800
[ 303.431100] [<804921fc>] (tty_release) from [<80271404>] (__fput+0x98/0x1e8)
[ 303.438154] r10:00000008 r9:eeac2e60 r8:00000000 r7:ee00e000 r6:edebea10 r5:eeac2e60
[ 303.446075] r4:ed5a5500
[ 303.448635] [<8027136c>] (__fput) from [<802715c4>] (____fput+0x18/0x1c)
[ 303.455342] r10:ed5cfedc r9:ed496780 r8:edbf1f00 r7:edbf2340 r6:8108b054 r5:00000000
[ 303.463263] r4:edbf2300
[ 303.465832] [<802715ac>] (____fput) from [<80146034>] (task_work_run+0xc8/0xf8)
[ 303.473165] [<80145f6c>] (task_work_run) from [<801265e0>] (do_exit+0x330/0xb74)
[ 303.480567] r9:edfd07dc r8:00000000 r7:81084dcc r6:810025c4 r5:81083a25 r4:edbf1f00
[ 303.488408] [<801262b0>] (do_exit) from [<80128678>] (do_group_exit+0x4c/0xcc)
[ 303.495636] r7:00000009
[ 303.498205] [<8012862c>] (do_group_exit) from [<80135450>] (get_signal+0x2a8/0x990)
[ 303.505866] r7:00000009 r6:00418004 r5:ed5cfec8 r4:ed5eeca0
[ 303.511616] [<801351a8>] (get_signal) from [<8010c6e4>] (do_signal+0xd8/0x47c)
[ 303.518844] r10:00000000 r9:ed5ce000 r8:00000001 r7:766d443c r6:766d4438 r5:ed5cfec8
[ 303.526764] r4:ed5cffb0
[ 303.529326] [<8010c60c>] (do_signal) from [<8010cc78>] (do_work_pending+0xc0/0xd0)
[ 303.536901] r10:00000000 r9:ed5ce000 r8:801086c4 r7:801086c4 r6:ed5cffb0 r5:ed5ce000
[ 303.544824] r4:00000001
[ 303.547385] [<8010cbb8>] (do_work_pending) from [<80108548>] (slow_work_pending+0xc/0x20)
[ 303.555568] r7:0000008e r6:747ad088 r5:747ad188 r4:747ad188
[ 303.561306] Code: e5943094 e594202c e2833001 e5843094 (e592a000)
[ 303.567419] ---[ end trace 741daf1a1655e1be ]---
[ 303.572048] Kernel panic - not syncing: Fatal exception in interrupt
[ 303.578432] ---[ end Kernel panic - not syncing: Fatal exception in interrupt
...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown
From: Uwe Kleine-König @ 2018-05-09 10:42 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
Shawn Guo, linux-kernel
In-Reply-To: <20180509102058.6mshag4y7aq4w2n2@earth.universe>
On Wed, May 09, 2018 at 12:20:58PM +0200, Sebastian Reichel wrote:
> Hi,
>
> On Tue, May 08, 2018 at 08:46:12PM +0200, Uwe Kleine-König wrote:
> > On Tue, May 08, 2018 at 03:40:47PM +0200, Sebastian Reichel wrote:
> > > On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> > > > On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > > > > This properly unmaps DMA SG on device shutdown.
> > > > >
> > > > > Reported-by: Nandor Han <nandor.han@ge.com>
> > > > > Suggested-by: Nandor Han <nandor.han@ge.com>
> > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > > > ---
> > > > > drivers/tty/serial/imx.c | 12 ++++++++++--
> > > > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > > index 3ca767b1162a..6c53e74244ec 100644
> > > > > --- a/drivers/tty/serial/imx.c
> > > > > +++ b/drivers/tty/serial/imx.c
> > > > > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> > > > > u32 ucr1, ucr2;
> > > > >
> > > > > if (sport->dma_is_enabled) {
> > > > > - sport->dma_is_rxing = 0;
> > > > > - sport->dma_is_txing = 0;
> > > > > dmaengine_terminate_sync(sport->dma_chan_tx);
> > > > > + if (sport->dma_is_txing) {
> > > > > + dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
> > > > > + sport->dma_tx_nents, DMA_TO_DEVICE);
> > > > > + sport->dma_is_txing = 0;
> > > > > + }
> > > >
> > > > did you find this because the kernel crashed or consumed more and more
> > > > memory, or is this "only" a finding of reading the source code? If the
> > > > former it would be great to point out in the commit log, if the latter,
> > > > I wonder if this is a real problem that warrants a stable backport.
> > >
> > > A bit of both. One of Collabora's customers had a (scarce) kernel crash
> > > in imx-serial and modified multiple things in the driver. The crash is
> > > gone, but it's not clear which change fixed it. I could not
> > > reproduce the crash so far and I'm currently rebasing and splitting
> > > their changes into upstreamable portions with proper patch
> > > descriptions. From reading the source this looked like a real issue.
> >
> > In which context (kernel version, operating mode (e.g. rs485)) did these
> > happen? What does "crash" mean? The kernel did just hang or produced an
> > oops? If the latter, can you show it/them?
>
> I pasted the oops, that triggered writing the patches (Linux 4.8, no
> rs485, 4MHz baudrate). I think, that the actual issue has already been
> fixed upstream between 4.13 and current master.
>
> -- Sebastian
>
> ...
> [ 302.516696] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0938000
This is usually a missing clk. Alternatively RX is disabled and the
RXDATA register is read. Scrolling through
git log v4.13..linus/master -- drivers/tty/serial/imx.c
I didn't find a candidate for fixing that.
> [ 302.524394] pgd = 80004000
> [ 302.527111] [f0938000] *pgd=de81a811, *pte=53fc0653, *ppte=53fc0453
> [ 302.533451] Internal error: : 1008 [#1] SMP ARM
> [ 302.537994] Modules linked in: smsc95xx usbnet atmel_mxt_ts
> [ 302.543651] CPU: 0 PID: 357 Comm: _ACFRead Not tainted 4.8.0 #1
> [ 302.549578] Hardware name: Freescale i.MX53 (Device Tree Support)
> [ 302.555679] task: edbf1f00 task.stack: ed5ce000
> [ 302.560228] PC is at imx_rxint+0x5c/0x228
> [ 302.564261] LR is at lock_acquired+0x494/0x57c
If you still have this kernel, disabling imx_rxint would be helpful.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown
From: Sebastian Reichel @ 2018-05-09 11:28 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
Shawn Guo, linux-kernel
In-Reply-To: <20180509104256.slkzauicnhsktqqf@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 3680 bytes --]
Hi,
On Wed, May 09, 2018 at 12:42:56PM +0200, Uwe Kleine-König wrote:
> On Wed, May 09, 2018 at 12:20:58PM +0200, Sebastian Reichel wrote:
> > Hi,
> >
> > On Tue, May 08, 2018 at 08:46:12PM +0200, Uwe Kleine-König wrote:
> > > On Tue, May 08, 2018 at 03:40:47PM +0200, Sebastian Reichel wrote:
> > > > On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> > > > > On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > > > > > This properly unmaps DMA SG on device shutdown.
> > > > > >
> > > > > > Reported-by: Nandor Han <nandor.han@ge.com>
> > > > > > Suggested-by: Nandor Han <nandor.han@ge.com>
> > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > > > > ---
> > > > > > drivers/tty/serial/imx.c | 12 ++++++++++--
> > > > > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > > > index 3ca767b1162a..6c53e74244ec 100644
> > > > > > --- a/drivers/tty/serial/imx.c
> > > > > > +++ b/drivers/tty/serial/imx.c
> > > > > > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> > > > > > u32 ucr1, ucr2;
> > > > > >
> > > > > > if (sport->dma_is_enabled) {
> > > > > > - sport->dma_is_rxing = 0;
> > > > > > - sport->dma_is_txing = 0;
> > > > > > dmaengine_terminate_sync(sport->dma_chan_tx);
> > > > > > + if (sport->dma_is_txing) {
> > > > > > + dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
> > > > > > + sport->dma_tx_nents, DMA_TO_DEVICE);
> > > > > > + sport->dma_is_txing = 0;
> > > > > > + }
> > > > >
> > > > > did you find this because the kernel crashed or consumed more and more
> > > > > memory, or is this "only" a finding of reading the source code? If the
> > > > > former it would be great to point out in the commit log, if the latter,
> > > > > I wonder if this is a real problem that warrants a stable backport.
> > > >
> > > > A bit of both. One of Collabora's customers had a (scarce) kernel crash
> > > > in imx-serial and modified multiple things in the driver. The crash is
> > > > gone, but it's not clear which change fixed it. I could not
> > > > reproduce the crash so far and I'm currently rebasing and splitting
> > > > their changes into upstreamable portions with proper patch
> > > > descriptions. From reading the source this looked like a real issue.
> > >
> > > In which context (kernel version, operating mode (e.g. rs485)) did these
> > > happen? What does "crash" mean? The kernel did just hang or produced an
> > > oops? If the latter, can you show it/them?
> >
> > I pasted the oops, that triggered writing the patches (Linux 4.8, no
> > rs485, 4MHz baudrate). I think, that the actual issue has already been
> > fixed upstream between 4.13 and current master.
> >
> > -- Sebastian
> >
> > ...
> > [ 302.516696] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0938000
>
> This is usually a missing clk. Alternatively RX is disabled and the
> RXDATA register is read. Scrolling through
>
> git log v4.13..linus/master -- drivers/tty/serial/imx.c
>
> I didn't find a candidate for fixing that.
It happens while the port is being torn apart. I think the following
patches from you are very good candidates. Especially since the
remaining diff fixing the issue in the customer's old kernel has a
smilar change:
76821e222c18 - serial: imx: ensure that RX irqs are off if RX is off (9 weeks ago) <Uwe Kleine-König>
dedc64e02f5d - serial: imx: Stop to receive in .stop_rx() (9 weeks ago) <Uwe Kleine-König>
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 0/3] serial: uartps: Add run time support for more IPs than hardcoded 2
From: Michal Simek @ 2018-05-09 12:09 UTC (permalink / raw)
To: Maarten Brock, Michal Simek
Cc: linux-kernel, monstr, gnomes, Alexander Graf, devicetree,
Jiri Slaby, Greg Kroah-Hartman, Rob Herring, linux-serial,
Frank Rowand, linux-arm-kernel, linux-serial-owner
In-Reply-To: <1d3bc2082972bb1d4abc2c15db396257@vanmierlo.com>
Hi,
On 5.5.2018 15:10, Maarten Brock wrote:
> On 2018-04-26 16:08, Michal Simek wrote:
>> Hi,
>>
>> this series is trying to address discussion I had with Alan in past
>> https://patchwork.kernel.org/patch/9738445/.
>>
>> It is moving uart_register_driver() to probe function like it is done in
>> pl011 driver.
>> And also introducing new function for alias compatibility checking to
>> resolve cases where some IPs have alias and some of them not.
>> This case is detected in pl011_probe_dt_alias() but not properly solved.
>>
>> Also keep status of free ids/minor numbers in bitmap to know what's the
>> next unallocated number.
>>
>> The same solution can be used in pl011, uart16550 and uartlite to really
>> get unified kernel.
>>
>> Tested on these use cases:
>> Notes:
>> ff000000 is the first PS uart listed in dtsi
>> ff010000 is the second PS uart listed in dtsi
>>
>> Standard zcu102 setting
>> serial0 = &uart0;
>> serial1 = &uart1;
>> [ 0.196628] ff000000.serial: ttyPS0 at MMIO 0xff000000 (irq = 33,
>> base_baud = 6250000) is a xuartps
>> [ 0.642542] ff010000.serial: ttyPS1 at MMIO 0xff010000 (irq = 34,
>> base_baud = 6250000) is a xuartps
>>
>> Swapped zcu102 setting
>> serial0 = &uart1;
>> serial1 = &uart0;
>> [ 0.196472] ff000000.serial: ttyPS1 at MMIO 0xff000000 (irq = 33,
>> base_baud = 6250000) is a xuartps
>> [ 0.196824] ff010000.serial: ttyPS0 at MMIO 0xff010000 (irq = 34,
>> base_baud = 6250000) is a xuartps
>>
>> uart0 on alias higher then MAX uart ports
>> serial0 = &uart1;
>> serial200 = &uart0;
>> [ 0.176793] ff000000.serial: ttyPS200 at MMIO 0xff000000 (irq = 33,
>> base_baud = 6250000) is a xuartps
>> [ 0.177288] ff010000.serial: ttyPS0 at MMIO 0xff010000 (irq = 34,
>> base_baud = 6250000) is a xuartps
>>
>> Both uarts on higher aliases
>> serial1 = &uart1;
>> serial2 = &uart0;
>> [ 0.196470] ff000000.serial: ttyPS2 at MMIO 0xff000000 (irq = 33,
>> base_baud = 6250000) is a xuartps
>> [ 0.196823] ff010000.serial: ttyPS1 at MMIO 0xff010000 (irq = 34,
>> base_baud = 6250000) is a xuartps
>>
>> uart0 not listed but it is probed first that's why should be ttyPS0 but
>> there is uart1 via alias
>> serial0 = &uart1;
>> [ 0.176656] xuartps ff000000.serial: No serial alias passed. Using
>> the first free id
>> [ 0.176671] xuartps ff000000.serial: Validate id 0
>> [ 0.176680] xuartps ff000000.serial: The empty id is 0
>> [ 0.176692] xuartps ff000000.serial: ID 0 already taken - skipped
>> [ 0.176701] xuartps ff000000.serial: Validate id 1
>> [ 0.176710] xuartps ff000000.serial: The empty id is 1
>> [ 0.176719] xuartps ff000000.serial: Selected ID 1 allocation passed
>> [ 0.176760] ff000000.serial: ttyPS1 at MMIO 0xff000000 (irq = 33,
>> base_baud = 6250000) is a xuartps
>> [ 0.177104] ff010000.serial: ttyPS0 at MMIO 0xff010000 (irq = 34,
>> base_baud = 6250000) is a xuartps
>>
>> uart0 not listed but it is probed first that's why should be ttyPS0
>> serial1 = &uart1;
>> [ 0.176661] xuartps ff000000.serial: No serial alias passed. Using
>> the first free id
>> [ 0.176676] xuartps ff000000.serial: Validate id 0
>> [ 0.176686] xuartps ff000000.serial: The empty id is 0
>> [ 0.176696] xuartps ff000000.serial: Selected ID 0 allocation passed
>> [ 0.176737] ff000000.serial: ttyPS0 at MMIO 0xff000000 (irq = 33,
>> base_baud = 6250000) is a xuartps
>> [ 0.177069] ff010000.serial: ttyPS1 at MMIO 0xff010000 (irq = 34,
>> base_baud = 6250000) is a xuartps
>>
>> uarts not listed in aliases list
>> [ 0.176673] xuartps ff000000.serial: No serial alias passed. Using
>> the first free id
>> [ 0.176687] xuartps ff000000.serial: Validate id 0
>> [ 0.176697] xuartps ff000000.serial: The empty id is 0
>> [ 0.176707] xuartps ff000000.serial: Selected ID 0 allocation passed
>> [ 0.176746] ff000000.serial: ttyPS0 at MMIO 0xff000000 (irq = 33,
>> base_baud = 6250000) is a xuartps
>> [ 0.177057] xuartps ff010000.serial: No serial alias passed. Using
>> the first free id
>> [ 0.177070] xuartps ff010000.serial: Validate id 0
>> [ 0.177079] xuartps ff010000.serial: The empty id is 0
>> [ 0.177089] xuartps ff010000.serial: Selected ID 0 allocation failed
>> [ 0.177098] xuartps ff010000.serial: Validate id 1
>> [ 0.177107] xuartps ff010000.serial: The empty id is 1
>> [ 0.177116] xuartps ff010000.serial: Selected ID 1 allocation passed
>> [ 0.177149] ff010000.serial: ttyPS1 at MMIO 0xff010000 (irq = 34,
>> base_baud = 6250000) is a xuartps
>>
>> Thanks,
>> Michal
>
> Hello Michal,
>
> How will this interact with ns16550 based UARTs?
>
> Can we have both /dev/ttyS0 and /dev/ttyPS0?
> Currently we can't unless we create *no* serialN alias for the ns16550.
> And there is no other means to lock the /dev/ttySx name to a device either.
>
> Or will the xuartps driver eventually use /dev/ttySx as well?
I am not changing any current behavior in this case. uartps is using
ttyPS. ns16550 is using ttyS and on xilinx devices we are using also
ttyUL for uartlite. Other drivers are using different names.
Thanks,
Michal
^ permalink raw reply
* OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))
From: Johan Hovold @ 2018-05-09 13:10 UTC (permalink / raw)
To: Tony Lindgren
Cc: Johan Hovold, Sebastian Reichel, H. Nikolaus Schaller,
Andreas Kemnade, Mark Rutland, Arnd Bergmann, Pavel Machek,
linux-kernel@vger.kernel.org,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Rob Herring, linux-serial, linux-omap,
linux-pm
In-Reply-To: <20180508164904.GZ98604@atomide.com>
[ Updating subject and adding linux-serial, linux-pm and linux-omap. ]
On Tue, May 08, 2018 at 09:49:04AM -0700, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [180508 08:54]:
> > * Tony Lindgren <tony@atomide.com> [180508 08:47]:
> > > * Tony Lindgren <tony@atomide.com> [180508 08:22]:
> > > > * Johan Hovold <johan@kernel.org> [180508 07:00]:
> > > > > With the negative autosuspend set in both omap drivers probe functions,
> > > > > this is the expected behaviour. Which I think we must fix.
> > > >
> > > > Yes indeed. I've been using my script for years now and have
> > > > completely missed the fact that the unused ports are not idled
> > > > at all on start-up when unused.
> > >
> > > This might be all that's needed, care to try it and if it works
> > > I'll send out two separate proper patches?
> > >
> > > I'm seeing this now on my bbb after temporarily disabling my
> > > UART idle init script:
> > >
> > > # rwmem -s32 0x44e004b4 # uart 1 on l4_wkup
> > > 0x44e004b4 = 0x00000002
> > >
> > > # rwmem -s32 0x44e0006c+0x10 # uart 2 - 5 on l4_per
> > > 0x44e0006c = 0x00030000
> > > 0x44e00070 = 0x00030000
> > > 0x44e00074 = 0x00030000
> > > 0x44e00078 = 0x00030000
> > >
> > > # rwmem -s32 0x44e00038 # uart 6 on l4_per
> > > 0x44e00038 = 0x00030000
> >
> > Hmm I forgot to remove status = "disabled" from the other ports,
> > still not idling the unused ports with the patch below it seems.
>
> No need to enable/disable autosuspend except in startup and shutdown
> I think. The patch below works for me, now includes removal of the
> status = "disabled" flags too. Only tested with 8250_omap on bbb
> so far.
>
> I wonder if other places still need fixing for autosuspend
> like console support?
While this seems to fix the idling of closed ports here too, I'm not
sure we can move use_autosuspend to startup() like this.
First, this flag should be set before registering the tty so that udev
can be used to update the attributes.
Second, this prevents setting the autosuspend delay through sysfs when
the port is closed (when autosuspend is disabled).
It seems we really should not be using the negative autosuspend to
configure the RPM behaviour the way these drivers do. Perhaps a new
mechanism is needed.
But I'm afraid I don't have time to look at this today.
Thanks,
Johan
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -605,6 +605,7 @@ static int omap_8250_startup(struct uart_port *port)
> return ret;
> }
>
> + pm_runtime_use_autosuspend(port->dev);
> pm_runtime_get_sync(port->dev);
>
> up->mcr = 0;
> @@ -685,8 +686,8 @@ static void omap_8250_shutdown(struct uart_port *port)
> serial_out(up, UART_LCR, up->lcr & ~UART_LCR_SBC);
> serial_out(up, UART_FCR, UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
>
> - pm_runtime_mark_last_busy(port->dev);
> - pm_runtime_put_autosuspend(port->dev);
> + pm_runtime_dont_use_autosuspend(port->dev);
> + pm_runtime_put_sync(port->dev);
> free_irq(port->irq, port);
> dev_pm_clear_wake_irq(port->dev);
> }
> @@ -1226,7 +1227,7 @@ static int omap8250_probe(struct platform_device *pdev)
> spin_lock_init(&priv->rx_dma_lock);
>
> device_init_wakeup(&pdev->dev, true);
> - pm_runtime_use_autosuspend(&pdev->dev);
> + /* See omap_8250_startup and shutdown for autosuspend */
> pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
>
> pm_runtime_irq_safe(&pdev->dev);
> @@ -1265,8 +1266,8 @@ static int omap8250_probe(struct platform_device *pdev)
> }
> priv->line = ret;
> platform_set_drvdata(pdev, priv);
> - pm_runtime_mark_last_busy(&pdev->dev);
> - pm_runtime_put_autosuspend(&pdev->dev);
> + pm_runtime_put_sync(&pdev->dev);
> +
> return 0;
> err:
^ permalink raw reply
* Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))
From: Tony Lindgren @ 2018-05-09 13:57 UTC (permalink / raw)
To: Johan Hovold
Cc: Sebastian Reichel, H. Nikolaus Schaller, Andreas Kemnade,
Mark Rutland, Arnd Bergmann, Pavel Machek,
linux-kernel@vger.kernel.org,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Rob Herring, linux-serial, linux-omap,
linux-pm
In-Reply-To: <20180509131003.GC2285@localhost>
* Johan Hovold <johan@kernel.org> [180509 13:12]:
> While this seems to fix the idling of closed ports here too, I'm not
> sure we can move use_autosuspend to startup() like this.
>
> First, this flag should be set before registering the tty so that udev
> can be used to update the attributes.
>
> Second, this prevents setting the autosuspend delay through sysfs when
> the port is closed (when autosuspend is disabled).
Yeah I noticed that too yesterday.
> It seems we really should not be using the negative autosuspend to
> configure the RPM behaviour the way these drivers do. Perhaps a new
> mechanism is needed.
Hmm well simply defaulting to "on" instead of "auto" and setting the
autosuspend_ms to 3000 by default might be doable. I think that way
we can keep use_autosuspend() in probe. Let's hope there are no
existing use cases that would break with that.
> But I'm afraid I don't have time to look at this today.
Sure np thanks,
Tony
^ permalink raw reply
* Re: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)
From: Tony Lindgren @ 2018-05-09 14:05 UTC (permalink / raw)
To: Johan Hovold
Cc: Sebastian Reichel, H. Nikolaus Schaller, Andreas Kemnade,
Mark Rutland, Arnd Bergmann, Pavel Machek,
linux-kernel@vger.kernel.org,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Rob Herring, linux-serial, linux-pm
In-Reply-To: <20180509091831.GA2285@localhost>
* Johan Hovold <johan@kernel.org> [180509 09:20]:
> On Tue, May 08, 2018 at 05:56:08PM +0200, Sebastian Reichel wrote:
> > I think using open/close for runtime pm is good enough for GPS,
> > since it regularly sends data and draws lots of power anyways.
> > But devices, that have an out-of-band wakeup signal can do proper
> > runtime PM of the serial port without loosing characters.
>
> Yeah, there may be some applications where this is possible. And this is
> not the case for GPS, but not just because of a generally higher power
> consumption, but due to the fact that we cannot afford having the first
> message in every report burst be dropped.
Well most of the phone implementations use one or two out of band
GPIOs to first wake the UART before any data is sent. For serdev
this can be called from the serdev consumer write function for TX.
For RX, the serdev consumer needs to implement an interrupt handler
and wake up the parent UART before serdev RX.
> > Note, that OMAP does not reach deep idle states with active
> > serial port. This is not acceptable for low power devices.
>
> Sure, but note that OMAP is the only serial driver which currently
> implements this kind of aggressive runtime PM (besides a couple of
> usb-serial drivers). This means that a serdev driver can never rely on
> this being the case, and therefore needs to be restrictive about how
> long the port is kept open if it cares about power at all.
Well by default we don't allow lossy UART. It needs to be manually
configured via /sys for the timeout. With serdev, this can all be
done with no /sys configuration needed for the cases with GPIO
wake irqs :)
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()
From: Jacek Anaszewski @ 2018-05-09 19:57 UTC (permalink / raw)
To: Uwe Kleine-König, Marc Kleine-Budde
Cc: Greg Kroah-Hartman, Jiri Slaby, Johan Hovold, Pavel Machek,
One Thousand Gnomes, Florian Fainelli, linux-serial,
Mathieu Poirier, linux-kernel, linux-can, linux-arm-kernel,
kernel, Robin Murphy, linux-leds
In-Reply-To: <20180508201725.3alpkcpaxebupzqv@pengutronix.de>
Hi Uwe,
On 05/08/2018 10:17 PM, Uwe Kleine-König wrote:
> Hello Jacek,
>
> On Tue, May 08, 2018 at 09:33:14PM +0200, Jacek Anaszewski wrote:
>> Thank you for the patch. It looks fine, but please split
>> the drivers/net/can/led.c related changes into a separate one.
>
> I renamed led_trigger_rename_static() to led_trigger_rename() (and
> changed the parameters). The can change just adapts the only user of
> led_trigger_rename_static() to use the new one.
>
> It's not impossible to separate this patches, but I wonder if it's worth
> the effort.
>
> The first patch would be like the patch under discussion, just without
> the can bits and introducing something like:
>
> /*
> * compat stuff to be removed once the only caller is converted
> */
> static inline led_trigger_rename_static(const char *name, struct led_trigger *trig)
> {
> (void)led_trigger_rename(trig, "%s", name);
> }
>
> Then the second patch would just be the 6-line can hunk. And a third
> patch would remove the compat function. (Maybe I'd choose to squash the
> two can patches together then, but this doesn't reduce the overhead
> considerably.) The only upside I can see here is that it increases my
> patch count, but it's otherwise not worth the effort for such an easy
> change. Further more as there is a strict dependency on these three
> patches this either delays the cleanup or (IMHO more likely) the can
> change would go in via the led tree anyhow. (Mark already acked patch 2
> of this series and in private confirmed that the agrees to let this
> change go in via the led tree, too.)
OK, makes sense. I'll wait also for ack on 3/3 since it should
go through the LED tree as well - uses a new led_trigger_register_format().
--
Best regards,
Jacek Anaszewski
^ permalink raw reply
* [PATCH] tty: serial: msm_geni_serial: Fix TX infinite loop
From: Evan Green @ 2018-05-09 20:44 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Karthikeyan Ramasubramanian, Doug Anderson
Cc: Evan Green
The GENI serial driver handled transmit by leaving stuff in the
common circular buffer until it had completely caught up to the head,
then clearing it out all at once. This is a suboptimal way to do
transmit, as it leaves data in the circular buffer that could be
freed. Moreover, the logic implementing it is wrong, and it is easy to
get into a situation where the UART infinitely writes out the same buffer.
I could reproduce infinite serial output of the same buffer by running
dmesg, then hitting Ctrl-C. I believe what happened is xmit_size was
something large, marching towards a larger value. Then the generic OS
code flushed out the buffer and replaced it with two characters. Now the
xmit_size is a large value marching towards a small value, which it wasn't
expecting. The driver subtracts xmit_size (very large) from
uart_circ_chars_pending (2), underflows, and repeats ad nauseum. The
locking isn't wrong here, as the locks are held whenever the buffer is
manipulated, it's just that the driver wasn't expecting the buffer to be
flushed out from underneath it in between transmits.
This change reworks transmit to grab what it can from the circular buffer,
and then update ->tail, both fixing the underflow and freeing up space
for a smoother circular experience.
Signed-off-by: Evan Green <evgreen@chromium.org>
---
Note: This patch applies on top of Karthik's series of 8 fixup patches,
which seem basically ready to go, at:
https://www.spinics.net/lists/linux-arm-msm/msg36561.html
Karthik had some concerns here in that apparently he had done it the way it
was on purpose in order to avoid a watchdog timeout with very large kernel
logs. Doug's and my best interpretation of his explanation is that maybe the
UART is so fast, and the FIFO is potentially only 16 bytes wide, so by the
time the data has been loaded up into the FIFO and the interrupt handler
returns, the UART has finished and the interrupt fires again immediately.
His original solution works because the buffer fills up completely,
and handle_tx calls stop_tx and uart_write_wakeup, which usually happens to
take enough time to let the interrupting core schedule something.
The old way this driver was doing it, artificially letting the circular buffer
balloon up and eventually pop, still seems like the wrong approach to me.
Perhaps this means that DMA mode should be considered, or a threaded irq,
or at least a larger FIFO. Or perhaps smaller kernel logs.
drivers/tty/serial/qcom_geni_serial.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 9d773a991369..f296a62bd811 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -98,7 +98,6 @@ struct qcom_geni_serial_port {
enum geni_se_xfer_mode xfer_mode;
bool setup;
int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
- unsigned int xmit_size;
unsigned int baud;
unsigned int tx_bytes_pw;
unsigned int rx_bytes_pw;
@@ -462,7 +461,6 @@ static void qcom_geni_serial_stop_tx(struct uart_port *uport)
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. */
@@ -592,16 +590,13 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport)
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);
+ if (!chunk && !status) {
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);
+ tail = xmit->tail;
chunk = min3((size_t)chunk, (size_t)(UART_XMIT_SIZE - tail), avail);
if (!chunk)
goto out_write_wakeup;
@@ -622,14 +617,16 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport)
iowrite32_rep(uport->membase + SE_GENI_TX_FIFOn, buf, 1);
i += tx_bytes;
- tail = (tail + tx_bytes) & (UART_XMIT_SIZE - 1);
+ tail += tx_bytes;
uport->icount.tx += tx_bytes;
remaining -= tx_bytes;
}
+
+ xmit->tail = tail & (UART_XMIT_SIZE - 1);
qcom_geni_serial_poll_tx_done(uport);
- port->xmit_size += chunk;
out_write_wakeup:
- uart_write_wakeup(uport);
+ if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(uport);
}
static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
--
2.17.0.441.gb46fe60e1d-goog
^ permalink raw reply related
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