* Re: [PATCH 5/8] mailbox: tegra-hsp: Add support for shared mailboxes
From: Jon Hunter @ 2018-05-22 16:20 UTC (permalink / raw)
To: Mikko Perttunen, robh+dt, mark.rutland, jassisinghbrar, gregkh,
thierry.reding
Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
linux-kernel
In-Reply-To: <20180508114403.14499-6-mperttunen@nvidia.com>
On 08/05/18 12:44, Mikko Perttunen wrote:
> 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;
Maybe add some definitions for the above.
Should we qualify 'mask' against the HSP_INT_IE register as well?
> +
> + for_each_set_bit(bit, &mask, 16) {
> + unsigned int mb_i = bit % 8;
If you right-shifted the mask above, you could avoid this modulo.
> +
> + mb = &hsp->mailboxes[mb_i];
> +
> + if (!mb->sending) {
> + value = tegra_hsp_channel_readl(&mb->channel,
> + HSP_SM_SHRD_MBOX);
> + value &= ~BIT(31);
Similarly a definition for bit 31 may add some clarity.
> + 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();
Could be nice to use readx_poll_timeout() here.
> +
> + 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;
It should not be necessary to set to zero as it should already be zero.
> + else
> + hsp->doorbell_irq = err;
>
> - hsp->doorbell_irq = err;
> + err = platform_get_irq_byname(pdev, "shared0");
> + if (err < 0)
> + hsp->shared_irq = 0;
It should not be necessary to set to zero as it should already be zero.
> + 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;
Clean-up?
> + }
> + }
> +
> + 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;
Clean-up?
> + }
> }
>
> 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;
> }
>
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH 4/8] mailbox: tegra-hsp: Refactor in preparation of mailboxes
From: Jon Hunter @ 2018-05-22 15:36 UTC (permalink / raw)
To: Mikko Perttunen, robh+dt, mark.rutland, jassisinghbrar, gregkh,
thierry.reding
Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
linux-kernel
In-Reply-To: <20180508114403.14499-5-mperttunen@nvidia.com>
On 08/05/18 12:43, Mikko Perttunen wrote:
> 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);
The above appears to be redundant. We go from channel to db and then end
up passing channels. Do we really need the 'db' struct above?
> + }
> +
> + return -EINVAL;
Does this function always 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;
I see that you are making the above only used for doorbells, but don't
we still need to set the chan->con_priv for shared mailboxes as well?
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH 2/8] dt-bindings: serial: Add bindings for nvidia,tegra194-tcu
From: Jon Hunter @ 2018-05-22 15:15 UTC (permalink / raw)
To: Mikko Perttunen, robh+dt, mark.rutland, jassisinghbrar, gregkh,
thierry.reding
Cc: devicetree, araza, linux-kernel, linux-serial, linux-tegra,
linux-arm-kernel
In-Reply-To: <20180508114403.14499-3-mperttunen@nvidia.com>
On 08/05/18 12:43, Mikko Perttunen wrote:
> 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"
Nit. We should say what device the above compatibility is applicable for ...
- "nvidia,tegra194-tcu": for Tegra194
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH 1/8] dt-bindings: tegra186-hsp: Add shared interrupts
From: Jon Hunter @ 2018-05-22 15:15 UTC (permalink / raw)
To: Mikko Perttunen, robh+dt, mark.rutland, jassisinghbrar, gregkh,
thierry.reding
Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
linux-kernel
In-Reply-To: <20180508114403.14499-2-mperttunen@nvidia.com>
On 08/05/18 12:43, Mikko Perttunen wrote:
> Non-doorbell interrupts are routed through "shared interrupts". These
> interrupts can be mapped to various internal interrupt lines. Add
> interrupt properties for shared interrupts to the tegra186-hsp device
> tree bindings.
Reading the Tegra documentation, although the doorbells have dedicated
interrupts, it appears that the doorbell interrupts can also be routed
via these shared interrupts.
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt b/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
> index b99d25fc2f26..9edcdf82d719 100644
> --- a/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
> +++ b/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
> @@ -21,6 +21,8 @@ Required properties:
> Contains a list of names for the interrupts described by the interrupt
> property. May contain the following entries, in any order:
> - "doorbell"
> + - "sharedN", where 'N' is a number from zero up to the number of
> + external interrupts supported by the HSP instance minus one.
> Users of this binding MUST look up entries in the interrupt property
> by name, using this interrupt-names property to do so.
> - interrupts
How is the mapping of shared-mailboxes interrupts to the actual
'sharedN' interrupt managed?
Cheers
Jon
--
nvpublic
^ permalink raw reply
* RE: [patch v21 1/4] drivers: jtag: Add JTAG core driver
From: Oleksandr Shamray @ 2018-05-22 15:00 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Arnd Bergmann, Linux Kernel Mailing List,
linux-arm Mailing List, devicetree, openbmc@lists.ozlabs.org,
Joel Stanley, Jiří Pírko, Tobias Klauser,
open list:SERIAL DRIVERS, Vadim Pasternak, system-sw-low-level,
Rob Herring, openocd-devel-owner@lists.sourceforge.net,
linux-api@vger.kernel.org
In-Reply-To: <CAHp75VctG5RBtLF3nT5e_j7_e9O12u=5gerJ6OMxWbABM9ft2w@mail.gmail.com>
Hi Andy.
Thanks for review.
Please read my answers inline.
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: 16 мая 2018 г. 0:22
> To: Oleksandr Shamray <oleksandrs@mellanox.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Arnd Bergmann
> <arnd@arndb.de>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; linux-arm Mailing List
> <linux-arm-kernel@lists.infradead.org>; devicetree
> <devicetree@vger.kernel.org>; openbmc@lists.ozlabs.org; Joel Stanley
> <joel@jms.id.au>; Jiří Pírko <jiri@resnulli.us>; Tobias Klauser
> <tklauser@distanz.ch>; open list:SERIAL DRIVERS <linux-
> serial@vger.kernel.org>; Vadim Pasternak <vadimp@mellanox.com>;
> system-sw-low-level <system-sw-low-level@mellanox.com>; Rob Herring
> <robh+dt@kernel.org>; openocd-devel-owner@lists.sourceforge.net;
> linux- api@vger.kernel.org; David S. Miller <davem@davemloft.net>;
> Mauro Carvalho Chehab <mchehab@kernel.org>; Jiri Pirko
> <jiri@mellanox.com>
> Subject: Re: [patch v21 1/4] drivers: jtag: Add JTAG core driver
>
> On Tue, May 15, 2018 at 5:21 PM, Oleksandr Shamray
> <oleksandrs@mellanox.com> wrote:
> > Initial patch for JTAG driver
> > JTAG class driver provide infrastructure to support
> > hardware/software JTAG platform drivers. It provide user layer API
> > interface for flashing and debugging external devices which equipped
> > with JTAG interface using standard transactions.
> >
> > Driver exposes set of IOCTL to user space for:
> > - XFER:
> > - SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
> > - SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
> > - RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified
> > number of clocks).
> > - SIOCFREQ/GIOCFREQ for setting and reading JTAG frequency.
> >
> > Driver core provides set of internal APIs for allocation and
> > registration:
> > - jtag_register;
> > - jtag_unregister;
> > - jtag_alloc;
> > - jtag_free;
> >
> > Platform driver on registration with jtag-core creates the next
> > entry in dev folder:
> > /dev/jtagX
>
> > 0xB0 all RATIO devices in development:
> > <mailto:vgo@ratio.de>
> > 0xB1 00-1F PPPoX <mailto:mostrows@styx.uwaterloo.ca>
> > +0xB2 00-0f linux/jtag.h JTAG driver
> > +
> > +<mailto:oleksandrs@mellanox.com>
>
> Consider to preserve style (upper vs. lower).
JTAG in code is lower (jtag) cane but in descriptions and notes it is upper (JTAG).
In all places which do not correspond to this I will fix.
>
> > + This provides basic core functionality support for JTAG class devices.
> > + Hardware that is equipped with a JTAG microcontroller can be
> > + supported by using this driver's interfaces.
> > + This driver exposes a set of IOCTLs to the user space for
> > + the following commands:
> > + SDR: (Scan Data Register) Performs an IEEE 1149.1 Data Register scan
> > + SIR: (Scan Instruction Register) Performs an IEEE 1149.1 Instruction
> > + Register scan.
> > + RUNTEST: Forces the IEEE 1149.1 bus to a run state for a specified
> > + number of clocks or a specified time period.
>
> Something feels wrong with formatting here.
>
Will fix
> > +#define MAX_JTAG_NAME_LEN (sizeof("jtag") + 5)
>
> Interesting definition. Why not to set to 10 explicitly? And why 10?
> (16 sounds better)
>
5 - is a max len of JTAG device id in device name. I will add define to it.
In general I don't see the case for the system with hundreds JTAG interfaces.
I will limit maximum jtag master to 255 devices and change id len to 3
#define MAX_JTAG_ID_STR_LEN 5
#define MAX_JTAG_NAME_LEN (sizeof("jtag") + MAX_JTAG_ID_STR_LEN)
> > +struct jtag {
> > + struct miscdevice miscdev;
>
> > + struct device *dev;
>
> Doesn't miscdev parent contain exactly this one?
Yes.
Will fix.
>
> > + const struct jtag_ops *ops;
> > + int id;
> > + bool opened;
> > + struct mutex open_lock;
> > + unsigned long priv[0];
> > +};
>
> > + err = copy_to_user(u64_to_user_ptr(xfer.tdio),
> > + (void *)(xfer_data), data_size);
>
> Redundant parens in one case. Check the rest similar places.
>
Yes.
> > +static int jtag_open(struct inode *inode, struct file *file) {
>
> > + struct jtag *jtag = container_of(file->private_data, struct jtag,
> > + miscdev);
>
> I would don't care about length and put it on one line.
>
But following to LINUX kernel style, it should be no longer than 80 symbols.
It will not pass by ./scripts/checkpatch.pl
Will it be OK to send a patch which failed 80 symbols limitation check?
> > + if (jtag->opened) {
> > + jtag->opened = true;
> > + jtag->opened = false;
>
> Can it be opened non exclusively several times? If so, this needs to
> be a ref counter instead.
It can be opened only once.
>
> > + if (!ops->idle || !ops->mode_set || !ops->status_get || !ops->xfer)
> > + return NULL;
>
> Are all of them mandatory?
>
Yes, except "mode_set"
Will remove mode_set from check
> > +int jtag_register(struct jtag *jtag)
>
> Perhaps devm_ variant.
Jtag driver uses miscdevice and related misc_register and misc_deregister
calls for creation and destruction. There is no device object prior to
call to misc_register, which could be used in devm_jtag_register.
>
> > +#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg)
>
Redundant. Removed.
> Where is this used or supposed to be used?
>
> > +#define JTAG_MAX_XFER_DATA_LEN 65535
>
> Is this limitation from some spec?
> Otherwise why not to allow 64K?
>
It not limited by specification.
But we enforce an upper bound for the length here, to prevent users from draining
kernel memory with giant buffers.
So it was limited by size of unsigned short int (65535)
> > +/**
> > + * struct jtag_ops - callbacks for jtag control functions:
> > + *
> > + * @freq_get: get frequency function. Filled by device driver
> > + * @freq_set: set frequency function. Filled by device driver
> > + * @status_get: set status function. Filled by device driver
> > + * @idle: set JTAG to idle state function. Filled by device driver
> > + * @xfer: send JTAG xfer function. Filled by device driver */
>
> Perhaps you need to describe which of them are _really_ mandatory and
> which are optional.
>
Ok
> --
> With Best Regards,
> Andy Shevchenko
Best Regards,
Oleksandr Shamray
^ permalink raw reply
* RE: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver
From: Oleksandr Shamray @ 2018-05-22 15:00 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Arnd Bergmann, Linux Kernel Mailing List,
linux-arm Mailing List, devicetree, openbmc@lists.ozlabs.org,
Joel Stanley, Jiří Pírko, Tobias Klauser,
open list:SERIAL DRIVERS, Vadim Pasternak, system-sw-low-level,
Rob Herring, openocd-devel-owner@lists.sourceforge.net,
linux-api@vger.kernel.org
In-Reply-To: <CAHp75Ve7EYiWBiE73i0CmyJhPMOdSKsGzCr0SUvta3Uya=Ov1Q@mail.gmail.com>
Hi Andy.
Thanks for review.
Please read my answers inline.
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: 16 мая 2018 г. 0:00
> To: Oleksandr Shamray <oleksandrs@mellanox.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Arnd Bergmann
> <arnd@arndb.de>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; linux-arm Mailing List
> <linux-arm-kernel@lists.infradead.org>; devicetree
> <devicetree@vger.kernel.org>; openbmc@lists.ozlabs.org; Joel Stanley
> <joel@jms.id.au>; Jiří Pírko <jiri@resnulli.us>; Tobias Klauser
> <tklauser@distanz.ch>; open list:SERIAL DRIVERS <linux-
> serial@vger.kernel.org>; Vadim Pasternak <vadimp@mellanox.com>;
> system-sw-low-level <system-sw-low-level@mellanox.com>; Rob Herring
> <robh+dt@kernel.org>; openocd-devel-owner@lists.sourceforge.net;
> linux- api@vger.kernel.org; David S. Miller <davem@davemloft.net>;
> Mauro Carvalho Chehab <mchehab@kernel.org>; Jiri Pirko
> <jiri@mellanox.com>
> Subject: Re: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and
> 25xx families JTAG master driver
>
> On Tue, May 15, 2018 at 5:21 PM, Oleksandr Shamray
> <oleksandrs@mellanox.com> wrote:
> > Driver adds support of Aspeed 2500/2400 series SOC JTAG master
> controller.
> >
> > Driver implements the following jtag ops:
> > - freq_get;
> > - freq_set;
> > - status_get;
> > - idle;
> > - xfer;
> >
> > It has been tested on Mellanox system with BMC equipped with Aspeed
> > 2520 SoC for programming CPLD devices.
>
> > +#define ASPEED_JTAG_DATA 0x00
> > +#define ASPEED_JTAG_INST 0x04
> > +#define ASPEED_JTAG_CTRL 0x08
> > +#define ASPEED_JTAG_ISR 0x0C
> > +#define ASPEED_JTAG_SW 0x10
> > +#define ASPEED_JTAG_TCK 0x14
> > +#define ASPEED_JTAG_EC 0x18
> > +
> > +#define ASPEED_JTAG_DATA_MSB 0x01
> > +#define ASPEED_JTAG_DATA_CHUNK_SIZE 0x20
>
>
> > +#define ASPEED_JTAG_IOUT_LEN(len) (ASPEED_JTAG_CTL_ENG_EN |\
> > + ASPEED_JTAG_CTL_ENG_OUT_EN
> > +|\
> > +
> > +ASPEED_JTAG_CTL_INST_LEN(len))
>
> Better to read
>
> #define MY_COOL_CONST_OR_MACRO(xxx) \
> ...
>
> > +#define ASPEED_JTAG_DOUT_LEN(len) (ASPEED_JTAG_CTL_ENG_EN
> |\
> > + ASPEED_JTAG_CTL_ENG_OUT_EN
> > + |\
> > +
> > +ASPEED_JTAG_CTL_DATA_LEN(len))
>
> Ditto.
Ok. Changed to:
#define ASPEED_JTAG_IOUT_LEN(len) \
(ASPEED_JTAG_CTL_ENG_EN | \
ASPEED_JTAG_CTL_ENG_OUT_EN | \
ASPEED_JTAG_CTL_INST_LEN(len))
#define ASPEED_JTAG_DOUT_LEN(len) \
(ASPEED_JTAG_CTL_ENG_EN | \
ASPEED_JTAG_CTL_ENG_OUT_EN | \
ASPEED_JTAG_CTL_DATA_LEN(len))
>
> > +static char *end_status_str[] = {"idle", "ir pause", "drpause"};
>
> > +static int aspeed_jtag_freq_set(struct jtag *jtag, u32 freq) {
> > + struct aspeed_jtag *aspeed_jtag = jtag_priv(jtag);
> > + unsigned long apb_frq;
> > + u32 tck_val;
> > + u16 div;
> > +
> > + apb_frq = clk_get_rate(aspeed_jtag->pclk);
>
> > + div = (apb_frq % freq == 0) ? (apb_frq / freq) - 1 :
> > + (apb_frq / freq);
>
> Isn't it the same as
>
> div = (apb_frq - 1) / freq;
>
> ?
>
Seems it is same. Thanks.
> > + tck_val = aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_TCK);
> > + aspeed_jtag_write(aspeed_jtag,
> > + (tck_val & ASPEED_JTAG_TCK_DIVISOR_MASK) | div,
> > + ASPEED_JTAG_TCK);
> > + return 0;
> > +}
>
> > +static void aspeed_jtag_sw_delay(struct aspeed_jtag *aspeed_jtag,
> > +int
> > +cnt) {
> > + int i;
> > +
> > + for (i = 0; i < cnt; i++)
> > + aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_SW);
>
> Isn't it readsl() (or how it's called, I don't remember).
>
No, readsl reads data into buffer. But in this place read used for make
software delay.
Aspeed jtag driver supports 2 modes:
1 - hw mode with the hardware controlled JTAG states
and pins
2 - with software controlled pins.
This part of code used in sw-mode and generates delay for JTAG bit-bang .
I will change it to ndelay().
> > +}
>
> > +static void aspeed_jtag_wait_instruction_pause(struct aspeed_jtag
> > +*aspeed_jtag) {
> > + wait_event_interruptible(aspeed_jtag->jtag_wq, aspeed_jtag->flag &
> > + ASPEED_JTAG_ISR_INST_PAUSE);
>
> In such cases I prefer to see a new line with a parameter in full.
> Check all places.
>
> > + aspeed_jtag->flag &= ~ASPEED_JTAG_ISR_INST_PAUSE; }
>
> > +static void aspeed_jtag_sm_cycle(struct aspeed_jtag *aspeed_jtag,
> > +const
> u8 *tms,
> > + int len) {
> > + int i;
> > +
> > + for (i = 0; i < len; i++)
> > + aspeed_jtag_tck_cycle(aspeed_jtag, tms[i], 0); }
> > +
> > +static void aspeed_jtag_run_test_idle_sw(struct aspeed_jtag
> *aspeed_jtag,
> > + struct jtag_run_test_idle
> > +*runtest) {
> > + static const u8 sm_pause_irpause[] = {1, 1, 1, 1, 0, 1, 0};
> > + static const u8 sm_pause_drpause[] = {1, 1, 1, 0, 1, 0};
> > + static const u8 sm_idle_irpause[] = {1, 1, 0, 1, 0};
> > + static const u8 sm_idle_drpause[] = {1, 0, 1, 0};
> > + static const u8 sm_pause_idle[] = {1, 1, 0};
> > + int i;
> > +
> > + /* SW mode from idle/pause-> to pause/idle */
> > + if (runtest->reset) {
> > + for (i = 0; i < ASPEED_JTAG_RESET_CNTR; i++)
> > + aspeed_jtag_tck_cycle(aspeed_jtag, 1, 0);
> > + }
>
> I would rather split this big switch to a few helper functions per
> each status of surrounding switch.
>
Ok.
Will do it.
> > +
> > + /* Stay on IDLE for at least TCK cycle */
> > + for (i = 0; i < runtest->tck; i++)
> > + aspeed_jtag_tck_cycle(aspeed_jtag, 0, 0); }
>
>
> > +/**
> > + * aspeed_jtag_run_test_idle:
> > + * JTAG reset: generates at least 9 TMS high and 1 TMS low to force
> > + * devices into Run-Test/Idle State.
> > + */
>
> It's rather broken kernel doc.
Deleted.
>
> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_CTL_ENG_EN |
> > + ASPEED_JTAG_CTL_ENG_OUT_EN |
> > + ASPEED_JTAG_CTL_FORCE_TMS,
> > + ASPEED_JTAG_CTRL);
>
> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_EC_GO_IDLE,
> > + ASPEED_JTAG_EC);
>
> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_SW_MODE_EN |
> > + ASPEED_JTAG_SW_MODE_TDIO, ASPEED_JTAG_SW);
>
> Here you have permutations of flag some of which are repeatetive in
> the code. Perhaps make additional definitions instead.
> Check other similar places.
>
Ok. Will add defined for repeated flags
>
> > + char tdo;
>
> Indentation.
Ok.
>
> > + if (xfer->direction == JTAG_READ_XFER)
> > + tdi = UINT_MAX;
> > + else
> > + tdi = data[index];
>
> > + if (xfer->direction == JTAG_READ_XFER)
> > + tdi = UINT_MAX;
> > + else
> > + tdi = data[index];
>
> Take your time to think how the above duplication can be avoided.
>
In both cases data[] is different, so I should check it twice, but I will
change it to, macro like:
#define ASPEED_JTAG_GET_TDI(direction, data) \
(direction == JTAG_READ_XFER) ? UNIT_MAX : data
> > + }
> > + }
> > +
> > + tdo = aspeed_jtag_tck_cycle(aspeed_jtag, 1, tdi &
> ASPEED_JTAG_DATA_MSB);
> > + data[index] |= tdo << (shift_bits %
> > +ASPEED_JTAG_DATA_CHUNK_SIZE); }
>
>
> > + if (endstate != JTAG_STATE_IDLE) {
>
> Why not to use positive check?
>
Will restructure to have positive check
if (endstate == JTAG_STATE_IDLE) {
...
} else {
...
}
> > + int i;
> > +
> > + for (i = 0; i <= xfer->length / BITS_PER_BYTE; i++) {
> > + pos += snprintf(&dbg_str[pos], sizeof(dbg_str) - pos,
> > + "0x%02x ", xfer_data[i]);
> > + }
>
> Oh, NO! Consider reading printk-formats (for %*ph) and other
> documentation about available APIs.
Ok.
>
> > + if (!(aspeed_jtag->mode & JTAG_XFER_HW_MODE)) {
> > + /* SW mode */
>
> This is rather too complex to be in one function.
>
Will split to separate functions.
> > + } else {
>
> > + /* hw mode */
> > + aspeed_jtag_write(aspeed_jtag, 0, ASPEED_JTAG_SW);
> > + aspeed_jtag_xfer_hw(aspeed_jtag, xfer, data);
>
> For symmetry it might be another function.
>
> > + }
>
> > + dev_dbg(aspeed_jtag->dev, "status %x\n", status);
>
> Perhaps someone should become familiar with tracepoints?
>
> > + dev_err(aspeed_jtag->dev, "irq status:%x\n",
> > + status);
>
>
> Huh, really?! SPAM.
I will review and delete redundant debug messages.
>
> (I would drop it completely, though you may use ratelimited variant)
>
> > + ret = IRQ_NONE;
> > + }
> > + return ret;
> > +}
>
> > + clk_prepare_enable(aspeed_jtag->pclk);
>
> This might fail.
Will add error check
>
> > + dev_dbg(&pdev->dev, "IRQ %d.\n", aspeed_jtag->irq);
>
> Noise even for debug.
Agree.
>
> > + err = jtag_register(jtag);
>
> Perhaps we might have devm_ variant of this. Check how SPI framework
> deal with a such.
>
Jtag driver uses miscdevice and related misc_register and misc_deregister
calls for creation and destruction. There is no device object prior
to call to misc_register, which could be used in devm_jtag_register.
> > +static int aspeed_jtag_remove(struct platform_device *pdev) {
>
> > + struct jtag *jtag;
> > +
> > + jtag = platform_get_drvdata(pdev);
>
> Usually we put this on one line
+
>
> > + aspeed_jtag_deinit(pdev, jtag_priv(jtag));
> > + jtag_unregister(jtag);
> > + jtag_free(jtag);
> > + return 0;
> > +}
>
>
> --
> With Best Regards,
> Andy Shevchenko
Best Regards,
Oleksandr Shamray
^ 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-21 15:48 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: <20180521134830.GR30172@localhost>
* Johan Hovold <johan@kernel.org> [180521 13:50]:
> On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan@kernel.org> [180517 10:12]:
> > > No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> > > either as that would also prevent the device from runtime suspending
> > > just as the current negative autosuspend delay does.
> >
> > Well in that case we should just stick with -1 value for the
> > autosuspend. And just do pm_runtime_put_sync_suspend() after
> > probe and on close.
>
> That won't work either as a negative autosuspend delay prevents runtime
> suspend completely (by grabbing an extra RPM reference).
Well so negative autosuspend delay is working as documented then :)
> > > I fail to see how we can implement this using the current toolbox. What
> > > you're after here is really a mechanism for selecting between two
> > > different runtime PM schemes at runtime:
> > >
> > > 1. normal serial RPM, where the controller is active while the
> > > port is open (this should be the safe default)
> >
> > Agreed. And that is the case already.
>
> Yes, but it's not really the case today as since omap-serial (and
> 8250-omap) sets a negative autosuspend at probe and hence does not
> runtime-suspend when the port is closed. So that's the long-standing bug
> which needs fixing.
Yes the bug for closed ports needs to be fixed for sure.
> > > 2. aggressive serial RPM, where the controller is allowed to
> > > suspend while the port is open even though this may result in
> > > lost characters when waking up on incoming data
> >
> > In this case it seems that the only thing needed is to just
> > configure the autosuspend delay for the parent port. The use of
> > -1 has been around since the start of runtime PM AFAIK, so maybe
> > we should just document it. I guess we could also introduce
> > pm_runtime_block_autoidle_unless_configured() :)
>
> The implications of a negative autosuspend delay are already documented
> (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> gets it wrong when trying to do things which aren't currently supported
> (and never have been).
>
> So I still think we need a new mechanism for this.
Well if you have some better mechanism in mind let's try it out. Short of
sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
right now.
> > > For normal ttys, we need a user-space interface for selecting between
> > > the two, and for serdev we may want a way to select the RPM scheme from
> > > within the kernel.
> > >
> > > Note that with my serdev controller runtime PM patch, serdev core could
> > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > reference for the controller while the port is open).
> >
> > So if your serdev controller was to set the parent autosuspend
> > delay on open() and set it back on close() this should work?
>
> Is it really the job of a serdev driver to set the autosuspend delay of
> a parent controller? Isn't this somethings which depends on the
> characteristics of the controller (possibly configurable by user space)
> such as the cost of runtime suspending and resuming?
Only in some cases will the serdev driver know it's safe to configure
the parent controller. Configuring the parent controller from userspace
works just fine as we've seen for years now.
> The patch I posted works with what we have today; if a parent serial
> controller driver uses aggressive runtime PM by default or after having
> been configured through sysfs to do so.
Yeah let's stick with configuring the parent controller from userspace
for now at least.
> What I'm getting at here is that the delay should be set by the serial
> driver implementing aggressive runtime PM. Then all we need is a
> mechanism to determine whether an extra RPM reference should be taken at
> tty open or not (configurable by user space, defaulting to yes).
OK yeah some additional on/off switch seems to be missing here.
> Specifically, the serial drivers themselves would always use
> autosuspend and not have to deal with supporting the two RPM schemes
> (normal vs aggressive runtime PM).
OK. So if I understand your idea right, we could have autosuspend timeout
set to 3000ms in the 8250_omap.c but still default to RPM blocked?
Then user can enable aggressive PM via /sys as desired, right?
Regards,
Tony
^ permalink raw reply
* Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))
From: Johan Hovold @ 2018-05-21 13:48 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: <20180517171038.GL98604@atomide.com>
On Thu, May 17, 2018 at 10:10:38AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180517 10:12]:
> > [ Sorry about the late reply. ]
> >
> > On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan@kernel.org> [180509 13:12]:
> >
> > > > 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.
> >
> > No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> > either as that would also prevent the device from runtime suspending
> > just as the current negative autosuspend delay does.
>
> Well in that case we should just stick with -1 value for the
> autosuspend. And just do pm_runtime_put_sync_suspend() after
> probe and on close.
That won't work either as a negative autosuspend delay prevents runtime
suspend completely (by grabbing an extra RPM reference).
> > I fail to see how we can implement this using the current toolbox. What
> > you're after here is really a mechanism for selecting between two
> > different runtime PM schemes at runtime:
> >
> > 1. normal serial RPM, where the controller is active while the
> > port is open (this should be the safe default)
>
> Agreed. And that is the case already.
Yes, but it's not really the case today as since omap-serial (and
8250-omap) sets a negative autosuspend at probe and hence does not
runtime-suspend when the port is closed. So that's the long-standing bug
which needs fixing.
> > 2. aggressive serial RPM, where the controller is allowed to
> > suspend while the port is open even though this may result in
> > lost characters when waking up on incoming data
>
> In this case it seems that the only thing needed is to just
> configure the autosuspend delay for the parent port. The use of
> -1 has been around since the start of runtime PM AFAIK, so maybe
> we should just document it. I guess we could also introduce
> pm_runtime_block_autoidle_unless_configured() :)
The implications of a negative autosuspend delay are already documented
(in Documentation/power/runtime_pm.txt); it's just the omap drivers that
gets it wrong when trying to do things which aren't currently supported
(and never have been).
So I still think we need a new mechanism for this.
> > For normal ttys, we need a user-space interface for selecting between
> > the two, and for serdev we may want a way to select the RPM scheme from
> > within the kernel.
> >
> > Note that with my serdev controller runtime PM patch, serdev core could
> > always opt for aggressive PM (as by default serdev core holds an RPM
> > reference for the controller while the port is open).
>
> So if your serdev controller was to set the parent autosuspend
> delay on open() and set it back on close() this should work?
Is it really the job of a serdev driver to set the autosuspend delay of
a parent controller? Isn't this somethings which depends on the
characteristics of the controller (possibly configurable by user space)
such as the cost of runtime suspending and resuming?
The patch I posted works with what we have today; if a parent serial
controller driver uses aggressive runtime PM by default or after having
been configured through sysfs to do so.
What I'm getting at here is that the delay should be set by the serial
driver implementing aggressive runtime PM. Then all we need is a
mechanism to determine whether an extra RPM reference should be taken at
tty open or not (configurable by user space, defaulting to yes).
Specifically, the serial drivers themselves would always use
autosuspend and not have to deal with supporting the two RPM schemes
(normal vs aggressive runtime PM).
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH v3 4/4] arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and Makefile
From: kbuild test robot @ 2018-05-19 10:07 UTC (permalink / raw)
Cc: kbuild-all, Matthias Brugger, Rob Herring, Mark Rutland,
Thomas Gleixner, Jason Cooper, Marc Zyngier, Greg Kroah-Hartman,
devicetree, srv_heupstream, linux-kernel, linux-serial,
linux-mediatek, linux-arm-kernel, yingjoe.chen, erin.lo,
mars.cheng, Ben Ho
In-Reply-To: <1526538126-51497-5-git-send-email-erin.lo@mediatek.com>
[-- Attachment #1: Type: text/plain, Size: 1108 bytes --]
Hi Ben,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.17-rc5 next-20180517]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Erin-Lo/Add-basic-support-for-Mediatek-MT8183-SoC/20180519-160349
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm64-alldefconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
All errors (new ones prefixed by >>):
>> Error: arch/arm64/boot/dts/mediatek/mt8183.dtsi:137.9-10 syntax error
FATAL ERROR: Unable to parse input tree
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 8591 bytes --]
^ permalink raw reply
* Re: [PATCH v3 2/6] mfd: at91-usart: added mfd driver for usart
From: Alexandre Belloni @ 2018-05-19 7:08 UTC (permalink / raw)
To: Rob Herring
Cc: Radu Pirea, devicetree, linux-serial, linux-kernel,
linux-arm-kernel, linux-spi, mark.rutland, lee.jones, gregkh,
jslaby, richard.genoud, nicolas.ferre, broonie
In-Reply-To: <20180518221949.GA13443@rob-hp-laptop>
On 18/05/2018 17:19:49-0500, Rob Herring wrote:
> On Fri, May 11, 2018 at 01:38:18PM +0300, Radu Pirea wrote:
> > This mfd driver is just a wrapper over atmel_serial driver and
> > spi-at91-usart driver. Selection of one of the drivers is based on a
> > property from device tree. If the property is not specified, the default
> > driver is atmel_serial.
> >
> > Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
> > ---
> > drivers/mfd/Kconfig | 10 ++++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/at91-usart.c | 75 ++++++++++++++++++++++++++++
> > include/dt-bindings/mfd/at91-usart.h | 17 +++++++
> > 4 files changed, 103 insertions(+)
> > create mode 100644 drivers/mfd/at91-usart.c
> > create mode 100644 include/dt-bindings/mfd/at91-usart.h
> >
>
> > +#ifndef __DT_BINDINGS_AT91_USART_H__
> > +#define __DT_BINDINGS_AT91_USART_H__
> > +
> > +#define AT91_USART_MODE_SERIAL 1
> > +#define AT91_USART_MODE_SPI 2
>
> Won't this require a DT update for serial mode to add the mode property?
> That breaks compatibility.
>
If the mode property is not present, it defaults to serial to keep
compatibility.
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v3 2/6] mfd: at91-usart: added mfd driver for usart
From: Rob Herring @ 2018-05-18 22:19 UTC (permalink / raw)
To: Radu Pirea
Cc: devicetree, linux-serial, linux-kernel, linux-arm-kernel,
linux-spi, mark.rutland, lee.jones, gregkh, jslaby,
richard.genoud, alexandre.belloni, nicolas.ferre, broonie
In-Reply-To: <20180511103822.31698-3-radu.pirea@microchip.com>
On Fri, May 11, 2018 at 01:38:18PM +0300, Radu Pirea wrote:
> This mfd driver is just a wrapper over atmel_serial driver and
> spi-at91-usart driver. Selection of one of the drivers is based on a
> property from device tree. If the property is not specified, the default
> driver is atmel_serial.
>
> Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
> ---
> drivers/mfd/Kconfig | 10 ++++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/at91-usart.c | 75 ++++++++++++++++++++++++++++
> include/dt-bindings/mfd/at91-usart.h | 17 +++++++
> 4 files changed, 103 insertions(+)
> create mode 100644 drivers/mfd/at91-usart.c
> create mode 100644 include/dt-bindings/mfd/at91-usart.h
>
> +#ifndef __DT_BINDINGS_AT91_USART_H__
> +#define __DT_BINDINGS_AT91_USART_H__
> +
> +#define AT91_USART_MODE_SERIAL 1
> +#define AT91_USART_MODE_SPI 2
Won't this require a DT update for serial mode to add the mode property?
That breaks compatibility.
Rob
^ permalink raw reply
* Re: [PATCH v3 4/6] dt-bindings: add binding for at91-usart in spi mode
From: Rob Herring @ 2018-05-18 21:40 UTC (permalink / raw)
To: Radu Pirea
Cc: devicetree, linux-serial, linux-kernel, linux-arm-kernel,
linux-spi, mark.rutland, lee.jones, gregkh, jslaby,
richard.genoud, alexandre.belloni, nicolas.ferre, broonie
In-Reply-To: <20180511103822.31698-5-radu.pirea@microchip.com>
On Fri, May 11, 2018 at 01:38:20PM +0300, Radu Pirea wrote:
> These are bindings for at91-usart IP in spi spi mode. There is no support for
s/spi spi/SPI/
> internal chip select. Only kind of chip selects available are gpio chip
GPIO
> selects.
>
> Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
> ---
> .../bindings/spi/microchip,at91-usart-spi.txt | 28 +++++++++++++++++++
> 1 file changed, 28 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt
>
> diff --git a/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt b/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt
> new file mode 100644
> index 000000000000..b68a3bec4121
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/microchip,at91-usart-spi.txt
So now we have 2 copies of the same thing that varies by 2 properties?
Please make this one doc.
> @@ -0,0 +1,28 @@
> +* Universal Synchronous Asynchronous Receiver/Transmitter (USART) in SPI mode
> +
> +Required properties:
> +- #size-cells : Must be <0>
> +- #address-cells : Must be <1>
> +- compatible: Should be "atmel,at91rm9200-usart" or "atmel,at91sam9260-usart"
> +- reg: Should contain registers location and length
> +- interrupts: Should contain interrupt
> +- clocks: phandles to input clocks.
> +- clock-names: tuple listing input clock names.
> + Required elements: "usart"
> +- cs-gpios: chipselects (internal cs not supported)
> +- at91,usart-mode: AT91_USART_MODE_SPI (found in dt-bindings/mfd/at91-usart.h)
at91 is not a vendor.
> +
> +Example:
> + #include <dt-bindings/mfd/at91-usart.h>
> +
> + spi0: spi@f001c000 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "atmel,at91rm9200-usart", "atmel,at91sam9260-usart";
> + at91,usart-mode = <AT91_USART_MODE_SPI>;
> + reg = <0xf001c000 0x100>;
> + interrupts = <12 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&usart0_clk>;
> + clock-names = "usart";
> + cs-gpios = <&pioB 3 0>;
> + };
> --
> 2.17.0
>
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Tony Lindgren @ 2018-05-17 19:48 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Sebastian Andrzej Siewior, Andy Shevchenko, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt, Linux Kernel Mailing List,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Arnd Bergmann
In-Reply-To: <CAHp75Veb+zBfh=44qmMevvrfCSRYhfUMJUMArsDBtgM-F_FknQ@mail.gmail.com>
* Andy Shevchenko <andy.shevchenko@gmail.com> [180517 16:38]:
> On Thu, May 17, 2018 at 4:48 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Sebastian Andrzej Siewior <bigeasy@linutronix.de> [180516 10:49]:
> >> On 2018-05-16 13:17:36 [+0300], Andy Shevchenko wrote:
> >> > > The output is usually short so there
> >> > > shouldn't be much benefit from using it.
> >> >
> >> > > I remember Tony wanted runtime-pm on the kernel console, too. And he
> >> > > told me explicit how to test it so that it works. Once the UART goes
> >> > > into PM (down), the whole IP block can go into power save mode. The
> >> > > board can be woken up by sending a character via the UART. The first few
> >> > > (incoming / read) characters are lost until the IP block is up again the
> >> > > frequency stable. This is known / expected.
> >> >
> >> > Don't consider world the OMAP only. The things more complicated if we
> >> > go out of it. Which I tried to explain in the commit message of patch
> >> > 2.
> >>
> >> I am not saying the world is OMAP only. I just tried to explain how any
> >> why it got there and its purpose. I haven't NACKed it. I would believe
> >> that with that information and Tony not defending his use case or
> >> possible change it somehow so it is not standing in your way, Greg would
> >> have enough information to go your way.
> >
> > The idea breaking PM seems silly to me considering that we've had
> > it working for years already.
>
> Same question / note. World is much more complex than just being OMAP.
Sorry but you are making assumptions about hardware being powered on
all the time.
It may have been a safe assumption up to mid-90's and might still be
valid assumption for hardware providing on MS-DOS boot floppy
compability.
But that is not a safe assumption for Linux kernel at all. Especially
for console TX where there's no need to keep it powered unless there
is actually something to write.
If there are runtime PM issues for consoles, let's just fix them.
Then for really seeing console messages on next reboot from a hung
system, CONFIG_PSTORE_CONSOLE option seems to do a very good job.
With a warm reset after triggered by watchdog the console messages
are readable most of the time even when using DDR as the back end.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Tony Lindgren @ 2018-05-17 19:30 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Sebastian Andrzej Siewior, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt, Linux Kernel Mailing List,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Arnd Bergmann
In-Reply-To: <CAHp75VfqAK6+dPWWkQXjy0V-AFSGLSDJ9CmnUv=ba0F66TDTVg@mail.gmail.com>
* Andy Shevchenko <andy.shevchenko@gmail.com> [180517 16:41]:
> On Thu, May 17, 2018 at 4:56 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [180516 13:12]:
> >> On Wed, 2018-05-16 at 12:47 +0200, Sebastian Andrzej Siewior wrote:
> >> > But since I am on it. You have to enable runtime-PM for the UART. So
> >> > what is the problem if you simply don't enable it for the UART which
> >> > used as the kernel console?
> >>
> >> How do I know at the ->probe() time that device in question is going to
> >> be kernel console? Maybe I missed simple way of it.
> >
> > Hmm parse the kernel cmdline maybe? :)
> >
> > BTW, kernel already has earlycon doing exactly what you're trying to do.
>
> I'm sorry, I didn't follow. What exactly earlycon does?
It provides a console very early on, see earlycon in
Documentation/admin-guide/kernel-parameters.txt
> The problem is in 8250 driver. The issue with runtime PM used in atomic context.
So how about add some "noidle" kernel command line parameter for console
that calls pm_runtime_forbid() and then you have the UART permanently
on. Hmm I guess you could make also serial8250_rpm_get() do nothing
based on that.
I do agree the serial runtime PM has an issue if it depends on
pm_runtime_irq_safe() being set.
> So, I can, of course just remove callbacks from the console ->write().
> Though it will prevent to use kernel console anyway.
Please et's not start breaking things, we already see a constant
flow of regressions on weekly basis.
Regards,
Tony
^ 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-17 17:10 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: <20180517100948.GI30172@localhost>
* Johan Hovold <johan@kernel.org> [180517 10:12]:
> [ Sorry about the late reply. ]
>
> On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan@kernel.org> [180509 13:12]:
>
> > > 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.
>
> No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
> either as that would also prevent the device from runtime suspending
> just as the current negative autosuspend delay does.
Well in that case we should just stick with -1 value for the
autosuspend. And just do pm_runtime_put_sync_suspend() after
probe and on close.
> I fail to see how we can implement this using the current toolbox. What
> you're after here is really a mechanism for selecting between two
> different runtime PM schemes at runtime:
>
> 1. normal serial RPM, where the controller is active while the
> port is open (this should be the safe default)
Agreed. And that is the case already.
> 2. aggressive serial RPM, where the controller is allowed to
> suspend while the port is open even though this may result in
> lost characters when waking up on incoming data
In this case it seems that the only thing needed is to just
configure the autosuspend delay for the parent port. The use of
-1 has been around since the start of runtime PM AFAIK, so maybe
we should just document it. I guess we could also introduce
pm_runtime_block_autoidle_unless_configured() :)
> For normal ttys, we need a user-space interface for selecting between
> the two, and for serdev we may want a way to select the RPM scheme from
> within the kernel.
>
> Note that with my serdev controller runtime PM patch, serdev core could
> always opt for aggressive PM (as by default serdev core holds an RPM
> reference for the controller while the port is open).
So if your serdev controller was to set the parent autosuspend
delay on open() and set it back on close() this should work?
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Tony Lindgren @ 2018-05-17 17:04 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Sebastian Andrzej Siewior, Andy Shevchenko, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt, Linux Kernel Mailing List,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Arnd Bergmann
In-Reply-To: <20180517135621.GJ98604@atomide.com>
* Tony Lindgren <tony@atomide.com> [180517 06:56]:
> * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [180516 13:12]:
> > On Wed, 2018-05-16 at 12:47 +0200, Sebastian Andrzej Siewior wrote:
> > > But since I am on it. You have to enable runtime-PM for the UART. So
> > > what is the problem if you simply don't enable it for the UART which
> > > used as the kernel console?
> >
> > How do I know at the ->probe() time that device in question is going to
> > be kernel console? Maybe I missed simple way of it.
>
> Hmm parse the kernel cmdline maybe? :)
>
> BTW, kernel already has earlycon doing exactly what you're trying to do.
And if earlycon does not do the job, maybe just set up some
kernel command line parameter like "noidle" that makes serial
probe call pm_runtime_forbid().
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Andy Shevchenko @ 2018-05-17 16:38 UTC (permalink / raw)
To: Tony Lindgren
Cc: Andy Shevchenko, Sebastian Andrzej Siewior, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt, Linux Kernel Mailing List,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Arnd Bergmann
In-Reply-To: <20180517135621.GJ98604@atomide.com>
On Thu, May 17, 2018 at 4:56 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [180516 13:12]:
>> On Wed, 2018-05-16 at 12:47 +0200, Sebastian Andrzej Siewior wrote:
>> > But since I am on it. You have to enable runtime-PM for the UART. So
>> > what is the problem if you simply don't enable it for the UART which
>> > used as the kernel console?
>>
>> How do I know at the ->probe() time that device in question is going to
>> be kernel console? Maybe I missed simple way of it.
>
> Hmm parse the kernel cmdline maybe? :)
>
> BTW, kernel already has earlycon doing exactly what you're trying to do.
I'm sorry, I didn't follow. What exactly earlycon does?
The problem is in 8250 driver. The issue with runtime PM used in atomic context.
So, I can, of course just remove callbacks from the console ->write().
Though it will prevent to use kernel console anyway.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Andy Shevchenko @ 2018-05-17 16:36 UTC (permalink / raw)
To: Tony Lindgren
Cc: Sebastian Andrzej Siewior, Andy Shevchenko, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt, Linux Kernel Mailing List,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Arnd Bergmann
In-Reply-To: <20180517134820.GI98604@atomide.com>
On Thu, May 17, 2018 at 4:48 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Sebastian Andrzej Siewior <bigeasy@linutronix.de> [180516 10:49]:
>> On 2018-05-16 13:17:36 [+0300], Andy Shevchenko wrote:
>> > > The output is usually short so there
>> > > shouldn't be much benefit from using it.
>> >
>> > > I remember Tony wanted runtime-pm on the kernel console, too. And he
>> > > told me explicit how to test it so that it works. Once the UART goes
>> > > into PM (down), the whole IP block can go into power save mode. The
>> > > board can be woken up by sending a character via the UART. The first few
>> > > (incoming / read) characters are lost until the IP block is up again the
>> > > frequency stable. This is known / expected.
>> >
>> > Don't consider world the OMAP only. The things more complicated if we
>> > go out of it. Which I tried to explain in the commit message of patch
>> > 2.
>>
>> I am not saying the world is OMAP only. I just tried to explain how any
>> why it got there and its purpose. I haven't NACKed it. I would believe
>> that with that information and Tony not defending his use case or
>> possible change it somehow so it is not standing in your way, Greg would
>> have enough information to go your way.
>
> The idea breaking PM seems silly to me considering that we've had
> it working for years already.
Same question / note. World is much more complex than just being OMAP.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Tony Lindgren @ 2018-05-17 13:56 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Sebastian Andrzej Siewior, Andy Shevchenko, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt, Linux Kernel Mailing List,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Arnd Bergmann
In-Reply-To: <ed9879ef42a70d2609f60f73834cabb7e3df9e2f.camel@linux.intel.com>
* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [180516 13:12]:
> On Wed, 2018-05-16 at 12:47 +0200, Sebastian Andrzej Siewior wrote:
> > But since I am on it. You have to enable runtime-PM for the UART. So
> > what is the problem if you simply don't enable it for the UART which
> > used as the kernel console?
>
> How do I know at the ->probe() time that device in question is going to
> be kernel console? Maybe I missed simple way of it.
Hmm parse the kernel cmdline maybe? :)
BTW, kernel already has earlycon doing exactly what you're trying to do.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Tony Lindgren @ 2018-05-17 13:48 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Andy Shevchenko, Andy Shevchenko, Petr Mladek, Sergey Senozhatsky,
Steven Rostedt, Linux Kernel Mailing List, Greg Kroah-Hartman,
Jiri Slaby, open list:SERIAL DRIVERS, Arnd Bergmann
In-Reply-To: <20180516104734.357oevogppu5bsg4@linutronix.de>
* Sebastian Andrzej Siewior <bigeasy@linutronix.de> [180516 10:49]:
> On 2018-05-16 13:17:36 [+0300], Andy Shevchenko wrote:
> > > The output is usually short so there
> > > shouldn't be much benefit from using it.
> >
> > > I remember Tony wanted runtime-pm on the kernel console, too. And he
> > > told me explicit how to test it so that it works. Once the UART goes
> > > into PM (down), the whole IP block can go into power save mode. The
> > > board can be woken up by sending a character via the UART. The first few
> > > (incoming / read) characters are lost until the IP block is up again the
> > > frequency stable. This is known / expected.
> >
> > Don't consider world the OMAP only. The things more complicated if we
> > go out of it. Which I tried to explain in the commit message of patch
> > 2.
>
> I am not saying the world is OMAP only. I just tried to explain how any
> why it got there and its purpose. I haven't NACKed it. I would believe
> that with that information and Tony not defending his use case or
> possible change it somehow so it is not standing in your way, Greg would
> have enough information to go your way.
The idea breaking PM seems silly to me considering that we've had
it working for years already.
Regards,
Tony
^ permalink raw reply
* Re: Serdev runtime PM (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding)
From: Johan Hovold @ 2018-05-17 10:25 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-pm
In-Reply-To: <20180509140550.GC98604@atomide.com>
On Wed, May 09, 2018 at 07:05:50AM -0700, Tony Lindgren wrote:
> * 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.
Right, but the client driver would then wake the parent controller when
expecting RX, while waking its own (serial-attached) device when wanting
to do TX.
Just for the record, we also cleared this up here:
https://marc.info/?l=linux-kernel&m=152604434504868&w=2
> > > 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 :)
Indeed, but serdev client drivers must also work with serial drivers
which do not implement this kind of aggressive runtime PM, and then the
only way to save power is to close the port when not in use.
Thanks,
Johan
^ permalink raw reply
* Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))
From: Johan Hovold @ 2018-05-17 10:09 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: <20180509135706.GB98604@atomide.com>
[ Sorry about the late reply. ]
On Wed, May 09, 2018 at 06:57:06AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@kernel.org> [180509 13:12]:
> > 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.
No, defaulting to "on" (i.e. calling pm_runtime_forbid()) wouldn't work
either as that would also prevent the device from runtime suspending
just as the current negative autosuspend delay does.
I fail to see how we can implement this using the current toolbox. What
you're after here is really a mechanism for selecting between two
different runtime PM schemes at runtime:
1. normal serial RPM, where the controller is active while the
port is open (this should be the safe default)
2. aggressive serial RPM, where the controller is allowed to
suspend while the port is open even though this may result in
lost characters when waking up on incoming data
For normal ttys, we need a user-space interface for selecting between
the two, and for serdev we may want a way to select the RPM scheme from
within the kernel.
Note that with my serdev controller runtime PM patch, serdev core could
always opt for aggressive PM (as by default serdev core holds an RPM
reference for the controller while the port is open).
Thanks,
Johan
^ permalink raw reply
* [PATCH v3 4/4] arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and Makefile
From: Erin Lo @ 2018-05-17 6:22 UTC (permalink / raw)
To: Matthias Brugger, Rob Herring, Mark Rutland, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Greg Kroah-Hartman
Cc: devicetree, srv_heupstream, linux-kernel, linux-serial,
linux-mediatek, linux-arm-kernel, yingjoe.chen, erin.lo,
mars.cheng, Ben Ho
In-Reply-To: <1526538126-51497-1-git-send-email-erin.lo@mediatek.com>
From: Ben Ho <Ben.Ho@mediatek.com>
Add basic chip support for Mediatek 8183
Signed-off-by: Ben Ho <Ben.Ho@mediatek.com>
Signed-off-by: Erin Lo <erin.lo@mediatek.com>
---
arch/arm64/boot/dts/mediatek/Makefile | 1 +
arch/arm64/boot/dts/mediatek/mt8183-evb.dts | 31 +++++
arch/arm64/boot/dts/mediatek/mt8183.dtsi | 182 ++++++++++++++++++++++++++++
3 files changed, 214 insertions(+)
create mode 100644 arch/arm64/boot/dts/mediatek/mt8183-evb.dts
create mode 100644 arch/arm64/boot/dts/mediatek/mt8183.dtsi
diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
index ac17f60..2836261 100644
--- a/arch/arm64/boot/dts/mediatek/Makefile
+++ b/arch/arm64/boot/dts/mediatek/Makefile
@@ -5,3 +5,4 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt6795-evb.dtb
dtb-$(CONFIG_ARCH_MEDIATEK) += mt6797-evb.dtb
dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-rfb1.dtb
dtb-$(CONFIG_ARCH_MEDIATEK) += mt8173-evb.dtb
+dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-evb.dtb
diff --git a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
new file mode 100644
index 0000000..9b52559
--- /dev/null
+++ b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ * Author: Ben Ho <ben.ho@mediatek.com>
+ * Erin Lo <erin.lo@mediatek.com>
+ */
+
+/dts-v1/;
+#include "mt8183.dtsi"
+
+/ {
+ model = "MediaTek MT8183 evaluation board";
+ compatible = "mediatek,mt8183-evb", "mediatek,mt8183";
+
+ aliases {
+ serial0 = &uart0;
+ };
+
+ memory@40000000 {
+ device_type = "memory";
+ reg = <0 0x40000000 0 0x80000000>;
+ };
+
+ chosen {
+ stdout-path = "serial0:921600n8";
+ };
+};
+
+&uart0 {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
new file mode 100644
index 0000000..03edf9c
--- /dev/null
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ * Author: Ben Ho <ben.ho@mediatek.com>
+ * Erin Lo <erin.lo@mediatek.com>
+ */
+
+#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+/ {
+ compatible = "mediatek,mt8183";
+ interrupt-parent = <&sysirq>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ cpus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ cpu-map {
+ cluster0 {
+ core0 {
+ cpu = <&cpu0>;
+ };
+ core1 {
+ cpu = <&cpu1>;
+ };
+ core2 {
+ cpu = <&cpu2>;
+ };
+ core3 {
+ cpu = <&cpu3>;
+ };
+ };
+
+ cluster1 {
+ core0 {
+ cpu = <&cpu4>;
+ };
+ core1 {
+ cpu = <&cpu5>;
+ };
+ core2 {
+ cpu = <&cpu6>;
+ };
+ core3 {
+ cpu = <&cpu7>;
+ };
+ };
+ };
+
+ cpu0: cpu@000 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x000>;
+ enable-method = "psci";
+ };
+
+ cpu1: cpu@001 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x001>;
+ enable-method = "psci";
+ };
+
+ cpu2: cpu@002 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x002>;
+ enable-method = "psci";
+ };
+
+ cpu3: cpu@003 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x003>;
+ enable-method = "psci";
+ };
+
+ cpu4: cpu@100 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a73";
+ reg = <0x100>;
+ enable-method = "psci";
+ };
+
+ cpu5: cpu@101 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a73";
+ reg = <0x101>;
+ enable-method = "psci";
+ };
+
+ cpu6: cpu@102 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a73";
+ reg = <0x102>;
+ enable-method = "psci";
+ };
+
+ cpu7: cpu@103 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a73";
+ reg = <0x103>;
+ enable-method = "psci";
+ };
+ };
+
+ psci {
+ compatible = "arm,psci-1.0";
+ method = "smc";
+ };
+
+ uart_clk: dummy26m {
+ compatible = "fixed-clock";
+ clock-frequency = <26000000>;
+ #clock-cells = <0>;
+ };
+
+ timer {
+ compatible = "arm,armv8-timer";
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
+ <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
+ };
+
+ gic: interrupt-controller@0c000000 {
+ compatible = "arm,gic-v3";
+ #interrupt-cells = <3>;
+ interrupt-parent = <&gic>;
+ interrupt-controller;
+ reg = <0 0x0c000000 0 0x40000>, /* GICD */
+ <0 0x0c100000 0 0x200000>; /* GICR */
+ <0 0x0c400000 0 0x2000>; /* GICC */
+ <0 0x0c410000 0 0x1000>; /* GICH */
+ <0 0x0c420000 0 0x2000>; /* GICV */
+
+ interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
+ };
+
+ sysirq: intpol-controller@0c530a80 {
+ compatible = "mediatek,mt8183-sysirq",
+ "mediatek,mt6577-sysirq";
+ interrupt-controller;
+ #interrupt-cells = <3>;
+ interrupt-parent = <&gic>;
+ reg = <0 0x0c530a80 0 0x50>;
+ };
+
+ uart0: serial@11002000 {
+ compatible = "mediatek,mt8183-uart",
+ "mediatek,mt6577-uart";
+ reg = <0 0x11002000 0 0x1000>;
+ interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&uart_clk>, <&uart_clk>;
+ clock-names = "baud", "bus";
+ status = "disabled";
+ };
+
+ uart1: serial@11003000 {
+ compatible = "mediatek,mt8183-uart",
+ "mediatek,mt6577-uart";
+ reg = <0 0x11003000 0 0x1000>;
+ interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&uart_clk>, <&uart_clk>;
+ clock-names = "baud", "bus";
+ status = "disabled";
+ };
+
+ uart2: serial@11004000 {
+ compatible = "mediatek,mt8183-uart",
+ "mediatek,mt6577-uart";
+ reg = <0 0x11004000 0 0x1000>;
+ interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&uart_clk>, <&uart_clk>;
+ clock-names = "baud", "bus";
+ status = "disabled";
+ };
+};
--
1.9.1
^ permalink raw reply related
* [PATCH v3 3/4] dt-bindings: serial: Add compatible for Mediatek MT8183
From: Erin Lo @ 2018-05-17 6:22 UTC (permalink / raw)
To: Matthias Brugger, Rob Herring, Mark Rutland, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Greg Kroah-Hartman
Cc: devicetree, srv_heupstream, linux-kernel, linux-serial,
linux-mediatek, linux-arm-kernel, yingjoe.chen, erin.lo,
mars.cheng
In-Reply-To: <1526538126-51497-1-git-send-email-erin.lo@mediatek.com>
This adds dt-binding documentation of uart for Mediatek MT8183 SoC
Platform.
Signed-off-by: Erin Lo <erin.lo@mediatek.com>
---
Documentation/devicetree/bindings/serial/mtk-uart.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/serial/mtk-uart.txt b/Documentation/devicetree/bindings/serial/mtk-uart.txt
index f73abff..4783336 100644
--- a/Documentation/devicetree/bindings/serial/mtk-uart.txt
+++ b/Documentation/devicetree/bindings/serial/mtk-uart.txt
@@ -15,6 +15,7 @@ Required properties:
* "mediatek,mt8127-uart" for MT8127 compatible UARTS
* "mediatek,mt8135-uart" for MT8135 compatible UARTS
* "mediatek,mt8173-uart" for MT8173 compatible UARTS
+ * "mediatek,mt8183-uart", "mediatek,mt6577-uart" for MT8183 compatible UARTS
* "mediatek,mt6577-uart" for MT6577 and all of the above
- reg: The base address of the UART register bank.
--
1.9.1
^ permalink raw reply related
* [PATCH v3 2/4] dt-bindings: mtk-sysirq: Add compatible for Mediatek MT8183
From: Erin Lo @ 2018-05-17 6:22 UTC (permalink / raw)
To: Matthias Brugger, Rob Herring, Mark Rutland, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Greg Kroah-Hartman
Cc: devicetree, srv_heupstream, linux-kernel, linux-serial,
linux-mediatek, linux-arm-kernel, yingjoe.chen, erin.lo,
mars.cheng
In-Reply-To: <1526538126-51497-1-git-send-email-erin.lo@mediatek.com>
This adds dt-binding documentation of SYSIRQ for Mediatek MT8183 SoC
Platform.
Signed-off-by: Erin Lo <erin.lo@mediatek.com>
---
.../devicetree/bindings/interrupt-controller/mediatek,sysirq.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,sysirq.txt b/Documentation/devicetree/bindings/interrupt-controller/mediatek,sysirq.txt
index 07bf0b9..5ff48a8 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/mediatek,sysirq.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,sysirq.txt
@@ -5,6 +5,7 @@ interrupt.
Required properties:
- compatible: should be
+ "mediatek,mt8183-sysirq", "mediatek,mt6577-sysirq": for MT8183
"mediatek,mt8173-sysirq", "mediatek,mt6577-sysirq": for MT8173
"mediatek,mt8135-sysirq", "mediatek,mt6577-sysirq": for MT8135
"mediatek,mt8127-sysirq", "mediatek,mt6577-sysirq": for MT8127
--
1.9.1
^ 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