* [PATCH v5 2/2] mailbox: sprd: Add Spreadtrum mailbox driver
From: Baolin Wang @ 2020-05-22 13:31 UTC (permalink / raw)
To: robh+dt, jassisinghbrar
Cc: orsonzhai, baolin.wang7, zhang.lyra, devicetree, linux-kernel
In-Reply-To: <8d29eba045ef18c5489e122b3668afc20431f15d.1590153779.git.baolin.wang7@gmail.com>
From: Baolin Wang <baolin.wang@unisoc.com>
The Spreadtrum mailbox controller supports 8 channels to communicate
with MCUs, and it contains 2 different parts: inbox and outbox, which
are used to send and receive messages by IRQ mode.
Signed-off-by: Baolin Wang <baolin.wang@unisoc.com>
Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
---
Changes from v4:
- Implement flush() to make sure the message has been fetched by
remote.
Changes from v3:
- Save the id in mbox_chan.con_priv and remove the 'sprd_mbox_chan'
Changes from v2:
- None.
Changes from v1:
- None
---
drivers/mailbox/Kconfig | 8 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/sprd-mailbox.c | 361 +++++++++++++++++++++++++++++++++
3 files changed, 371 insertions(+)
create mode 100644 drivers/mailbox/sprd-mailbox.c
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 5a577a6734cf..e03f3fb5caed 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -236,4 +236,12 @@ config SUN6I_MSGBOX
various Allwinner SoCs. This mailbox is used for communication
between the application CPUs and the power management coprocessor.
+config SPRD_MBOX
+ tristate "Spreadtrum Mailbox"
+ depends on ARCH_SPRD || COMPILE_TEST
+ help
+ Mailbox driver implementation for the Spreadtrum platform. It is used
+ to send message between application processors and MCU. Say Y here if
+ you want to build the Spreatrum mailbox controller driver.
+
endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 2e4364ef5c47..9caf4ede6ce0 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -50,3 +50,5 @@ obj-$(CONFIG_MTK_CMDQ_MBOX) += mtk-cmdq-mailbox.o
obj-$(CONFIG_ZYNQMP_IPI_MBOX) += zynqmp-ipi-mailbox.o
obj-$(CONFIG_SUN6I_MSGBOX) += sun6i-msgbox.o
+
+obj-$(CONFIG_SPRD_MBOX) += sprd-mailbox.o
diff --git a/drivers/mailbox/sprd-mailbox.c b/drivers/mailbox/sprd-mailbox.c
new file mode 100644
index 000000000000..f6fab24ae8a9
--- /dev/null
+++ b/drivers/mailbox/sprd-mailbox.c
@@ -0,0 +1,361 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Spreadtrum mailbox driver
+ *
+ * Copyright (c) 2020 Spreadtrum Communications Inc.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+
+#define SPRD_MBOX_ID 0x0
+#define SPRD_MBOX_MSG_LOW 0x4
+#define SPRD_MBOX_MSG_HIGH 0x8
+#define SPRD_MBOX_TRIGGER 0xc
+#define SPRD_MBOX_FIFO_RST 0x10
+#define SPRD_MBOX_FIFO_STS 0x14
+#define SPRD_MBOX_IRQ_STS 0x18
+#define SPRD_MBOX_IRQ_MSK 0x1c
+#define SPRD_MBOX_LOCK 0x20
+#define SPRD_MBOX_FIFO_DEPTH 0x24
+
+/* Bit and mask definiation for inbox's SPRD_MBOX_FIFO_STS register */
+#define SPRD_INBOX_FIFO_DELIVER_MASK GENMASK(23, 16)
+#define SPRD_INBOX_FIFO_OVERLOW_MASK GENMASK(15, 8)
+#define SPRD_INBOX_FIFO_DELIVER_SHIFT 16
+#define SPRD_INBOX_FIFO_BUSY_MASK GENMASK(7, 0)
+
+/* Bit and mask definiation for SPRD_MBOX_IRQ_STS register */
+#define SPRD_MBOX_IRQ_CLR BIT(0)
+
+/* Bit and mask definiation for outbox's SPRD_MBOX_FIFO_STS register */
+#define SPRD_OUTBOX_FIFO_FULL BIT(0)
+#define SPRD_OUTBOX_FIFO_WR_SHIFT 16
+#define SPRD_OUTBOX_FIFO_RD_SHIFT 24
+#define SPRD_OUTBOX_FIFO_POS_MASK GENMASK(7, 0)
+
+/* Bit and mask definiation for inbox's SPRD_MBOX_IRQ_MSK register */
+#define SPRD_INBOX_FIFO_BLOCK_IRQ BIT(0)
+#define SPRD_INBOX_FIFO_OVERFLOW_IRQ BIT(1)
+#define SPRD_INBOX_FIFO_DELIVER_IRQ BIT(2)
+#define SPRD_INBOX_FIFO_IRQ_MASK GENMASK(2, 0)
+
+/* Bit and mask definiation for outbox's SPRD_MBOX_IRQ_MSK register */
+#define SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ BIT(0)
+#define SPRD_OUTBOX_FIFO_IRQ_MASK GENMASK(4, 0)
+
+#define SPRD_MBOX_CHAN_MAX 8
+
+struct sprd_mbox_priv {
+ struct mbox_controller mbox;
+ struct device *dev;
+ void __iomem *inbox_base;
+ void __iomem *outbox_base;
+ struct clk *clk;
+ u32 outbox_fifo_depth;
+
+ struct mbox_chan chan[SPRD_MBOX_CHAN_MAX];
+};
+
+static struct sprd_mbox_priv *to_sprd_mbox_priv(struct mbox_controller *mbox)
+{
+ return container_of(mbox, struct sprd_mbox_priv, mbox);
+}
+
+static u32 sprd_mbox_get_fifo_len(struct sprd_mbox_priv *priv, u32 fifo_sts)
+{
+ u32 wr_pos = (fifo_sts >> SPRD_OUTBOX_FIFO_WR_SHIFT) &
+ SPRD_OUTBOX_FIFO_POS_MASK;
+ u32 rd_pos = (fifo_sts >> SPRD_OUTBOX_FIFO_RD_SHIFT) &
+ SPRD_OUTBOX_FIFO_POS_MASK;
+ u32 fifo_len;
+
+ /*
+ * If the read pointer is equal with write pointer, which means the fifo
+ * is full or empty.
+ */
+ if (wr_pos == rd_pos) {
+ if (fifo_sts & SPRD_OUTBOX_FIFO_FULL)
+ fifo_len = priv->outbox_fifo_depth;
+ else
+ fifo_len = 0;
+ } else if (wr_pos > rd_pos) {
+ fifo_len = wr_pos - rd_pos;
+ } else {
+ fifo_len = priv->outbox_fifo_depth - rd_pos + wr_pos;
+ }
+
+ return fifo_len;
+}
+
+static irqreturn_t sprd_mbox_outbox_isr(int irq, void *data)
+{
+ struct sprd_mbox_priv *priv = data;
+ struct mbox_chan *chan;
+ u32 fifo_sts, fifo_len, msg[2];
+ int i, id;
+
+ fifo_sts = readl(priv->outbox_base + SPRD_MBOX_FIFO_STS);
+
+ fifo_len = sprd_mbox_get_fifo_len(priv, fifo_sts);
+ if (!fifo_len) {
+ dev_warn_ratelimited(priv->dev, "spurious outbox interrupt\n");
+ return IRQ_NONE;
+ }
+
+ for (i = 0; i < fifo_len; i++) {
+ msg[0] = readl(priv->outbox_base + SPRD_MBOX_MSG_LOW);
+ msg[1] = readl(priv->outbox_base + SPRD_MBOX_MSG_HIGH);
+ id = readl(priv->outbox_base + SPRD_MBOX_ID);
+
+ chan = &priv->chan[id];
+ mbox_chan_received_data(chan, (void *)msg);
+
+ /* Trigger to update outbox FIFO pointer */
+ writel(0x1, priv->outbox_base + SPRD_MBOX_TRIGGER);
+ }
+
+ /* Clear irq status after reading all message. */
+ writel(SPRD_MBOX_IRQ_CLR, priv->outbox_base + SPRD_MBOX_IRQ_STS);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t sprd_mbox_inbox_isr(int irq, void *data)
+{
+ struct sprd_mbox_priv *priv = data;
+ struct mbox_chan *chan;
+ u32 fifo_sts, send_sts, busy, id;
+
+ fifo_sts = readl(priv->inbox_base + SPRD_MBOX_FIFO_STS);
+
+ /* Get the inbox data delivery status */
+ send_sts = (fifo_sts & SPRD_INBOX_FIFO_DELIVER_MASK) >>
+ SPRD_INBOX_FIFO_DELIVER_SHIFT;
+ if (!send_sts) {
+ dev_warn_ratelimited(priv->dev, "spurious inbox interrupt\n");
+ return IRQ_NONE;
+ }
+
+ while (send_sts) {
+ id = __ffs(send_sts);
+ send_sts &= (send_sts - 1);
+
+ chan = &priv->chan[id];
+
+ /*
+ * Check if the message was fetched by remote traget, if yes,
+ * that means the transmission has been completed.
+ */
+ busy = fifo_sts & SPRD_INBOX_FIFO_BUSY_MASK;
+ if (!(busy & BIT(id)))
+ mbox_chan_txdone(chan, 0);
+ }
+
+ /* Clear FIFO delivery and overflow status */
+ writel(fifo_sts &
+ (SPRD_INBOX_FIFO_DELIVER_MASK | SPRD_INBOX_FIFO_OVERLOW_MASK),
+ priv->inbox_base + SPRD_MBOX_FIFO_RST);
+
+ /* Clear irq status */
+ writel(SPRD_MBOX_IRQ_CLR, priv->inbox_base + SPRD_MBOX_IRQ_STS);
+
+ return IRQ_HANDLED;
+}
+
+static int sprd_mbox_send_data(struct mbox_chan *chan, void *msg)
+{
+ struct sprd_mbox_priv *priv = to_sprd_mbox_priv(chan->mbox);
+ unsigned long id = (unsigned long)chan->con_priv;
+ u32 *data = msg;
+
+ /* Write data into inbox FIFO, and only support 8 bytes every time */
+ writel(data[0], priv->inbox_base + SPRD_MBOX_MSG_LOW);
+ writel(data[1], priv->inbox_base + SPRD_MBOX_MSG_HIGH);
+
+ /* Set target core id */
+ writel(id, priv->inbox_base + SPRD_MBOX_ID);
+
+ /* Trigger remote request */
+ writel(0x1, priv->inbox_base + SPRD_MBOX_TRIGGER);
+
+ return 0;
+}
+
+static int sprd_mbox_flush(struct mbox_chan *chan, unsigned long timeout)
+{
+ struct sprd_mbox_priv *priv = to_sprd_mbox_priv(chan->mbox);
+ unsigned long id = (unsigned long)chan->con_priv;
+ u32 busy;
+
+ timeout = jiffies + msecs_to_jiffies(timeout);
+
+ while (time_before(jiffies, timeout)) {
+ busy = readl(priv->inbox_base + SPRD_MBOX_FIFO_STS) &
+ SPRD_INBOX_FIFO_BUSY_MASK;
+ if (!(busy & BIT(id))) {
+ mbox_chan_txdone(chan, 0);
+ return 0;
+ }
+
+ udelay(1);
+ }
+
+ return -ETIME;
+}
+
+static int sprd_mbox_startup(struct mbox_chan *chan)
+{
+ struct sprd_mbox_priv *priv = to_sprd_mbox_priv(chan->mbox);
+ u32 val;
+
+ /* Select outbox FIFO mode and reset the outbox FIFO status */
+ writel(0x0, priv->outbox_base + SPRD_MBOX_FIFO_RST);
+
+ /* Enable inbox FIFO overflow and delivery interrupt */
+ val = readl(priv->inbox_base + SPRD_MBOX_IRQ_MSK);
+ val &= ~(SPRD_INBOX_FIFO_OVERFLOW_IRQ | SPRD_INBOX_FIFO_DELIVER_IRQ);
+ writel(val, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
+
+ /* Enable outbox FIFO not empty interrupt */
+ val = readl(priv->outbox_base + SPRD_MBOX_IRQ_MSK);
+ val &= ~SPRD_OUTBOX_FIFO_NOT_EMPTY_IRQ;
+ writel(val, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
+
+ return 0;
+}
+
+static void sprd_mbox_shutdown(struct mbox_chan *chan)
+{
+ struct sprd_mbox_priv *priv = to_sprd_mbox_priv(chan->mbox);
+
+ /* Disable inbox & outbox interrupt */
+ writel(SPRD_INBOX_FIFO_IRQ_MASK, priv->inbox_base + SPRD_MBOX_IRQ_MSK);
+ writel(SPRD_OUTBOX_FIFO_IRQ_MASK, priv->outbox_base + SPRD_MBOX_IRQ_MSK);
+}
+
+static const struct mbox_chan_ops sprd_mbox_ops = {
+ .send_data = sprd_mbox_send_data,
+ .flush = sprd_mbox_flush,
+ .startup = sprd_mbox_startup,
+ .shutdown = sprd_mbox_shutdown,
+};
+
+static void sprd_mbox_disable(void *data)
+{
+ struct sprd_mbox_priv *priv = data;
+
+ clk_disable_unprepare(priv->clk);
+}
+
+static int sprd_mbox_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct sprd_mbox_priv *priv;
+ int ret, inbox_irq, outbox_irq;
+ unsigned long id;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->dev = dev;
+
+ /*
+ * The Spreadtrum mailbox uses an inbox to send messages to the target
+ * core, and uses an outbox to receive messages from other cores.
+ *
+ * Thus the mailbox controller supplies 2 different register addresses
+ * and IRQ numbers for inbox and outbox.
+ */
+ priv->inbox_base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(priv->inbox_base))
+ return PTR_ERR(priv->inbox_base);
+
+ priv->outbox_base = devm_platform_ioremap_resource(pdev, 1);
+ if (IS_ERR(priv->outbox_base))
+ return PTR_ERR(priv->outbox_base);
+
+ priv->clk = devm_clk_get(dev, "enable");
+ if (IS_ERR(priv->clk)) {
+ dev_err(dev, "failed to get mailbox clock\n");
+ return PTR_ERR(priv->clk);
+ }
+
+ ret = clk_prepare_enable(priv->clk);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(dev, sprd_mbox_disable, priv);
+ if (ret) {
+ dev_err(dev, "failed to add mailbox disable action\n");
+ return ret;
+ }
+
+ inbox_irq = platform_get_irq(pdev, 0);
+ if (inbox_irq < 0)
+ return inbox_irq;
+
+ ret = devm_request_irq(dev, inbox_irq, sprd_mbox_inbox_isr,
+ IRQF_NO_SUSPEND, dev_name(dev), priv);
+ if (ret) {
+ dev_err(dev, "failed to request inbox IRQ: %d\n", ret);
+ return ret;
+ }
+
+ outbox_irq = platform_get_irq(pdev, 1);
+ if (outbox_irq < 0)
+ return outbox_irq;
+
+ ret = devm_request_irq(dev, outbox_irq, sprd_mbox_outbox_isr,
+ IRQF_NO_SUSPEND, dev_name(dev), priv);
+ if (ret) {
+ dev_err(dev, "failed to request outbox IRQ: %d\n", ret);
+ return ret;
+ }
+
+ /* Get the default outbox FIFO depth */
+ priv->outbox_fifo_depth =
+ readl(priv->outbox_base + SPRD_MBOX_FIFO_DEPTH) + 1;
+ priv->mbox.dev = dev;
+ priv->mbox.chans = &priv->chan[0];
+ priv->mbox.num_chans = SPRD_MBOX_CHAN_MAX;
+ priv->mbox.ops = &sprd_mbox_ops;
+ priv->mbox.txdone_irq = true;
+
+ for (id = 0; id < SPRD_MBOX_CHAN_MAX; id++)
+ priv->chan[id].con_priv = (void *)id;
+
+ ret = devm_mbox_controller_register(dev, &priv->mbox);
+ if (ret) {
+ dev_err(dev, "failed to register mailbox: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct of_device_id sprd_mbox_of_match[] = {
+ { .compatible = "sprd,sc9860-mailbox", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, sprd_mbox_of_match);
+
+static struct platform_driver sprd_mbox_driver = {
+ .driver = {
+ .name = "sprd-mailbox",
+ .of_match_table = sprd_mbox_of_match,
+ },
+ .probe = sprd_mbox_probe,
+};
+module_platform_driver(sprd_mbox_driver);
+
+MODULE_AUTHOR("Baolin Wang <baolin.wang@unisoc.com>");
+MODULE_DESCRIPTION("Spreadtrum mailbox driver");
+MODULE_LICENSE("GPL v2");
--
2.17.1
^ permalink raw reply related
* [PATCH v5 1/2] dt-bindings: mailbox: Add the Spreadtrum mailbox documentation
From: Baolin Wang @ 2020-05-22 13:31 UTC (permalink / raw)
To: robh+dt, jassisinghbrar
Cc: orsonzhai, baolin.wang7, zhang.lyra, devicetree, linux-kernel
From: Baolin Wang <baolin.wang@unisoc.com>
Add the Spreadtrum mailbox documentation.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Baolin Wang <baolin.wang@unisoc.com>
Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
---
Changes from v4:
-None.
Changes from v3:
- None.
Changes from v2:
- Add reviewed tag from Rob.
- Remove redundant 'minItems'.
Changes from v1:
- Add 'additionalProperties'.
- Split description for each entry.
---
.../bindings/mailbox/sprd-mailbox.yaml | 60 +++++++++++++++++++
1 file changed, 60 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/sprd-mailbox.yaml
diff --git a/Documentation/devicetree/bindings/mailbox/sprd-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/sprd-mailbox.yaml
new file mode 100644
index 000000000000..0f7451b42d7e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/sprd-mailbox.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/mailbox/sprd-mailbox.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Spreadtrum mailbox controller bindings
+
+maintainers:
+ - Orson Zhai <orsonzhai@gmail.com>
+ - Baolin Wang <baolin.wang7@gmail.com>
+ - Chunyan Zhang <zhang.lyra@gmail.com>
+
+properties:
+ compatible:
+ enum:
+ - sprd,sc9860-mailbox
+
+ reg:
+ items:
+ - description: inbox registers' base address
+ - description: outbox registers' base address
+
+ interrupts:
+ items:
+ - description: inbox interrupt
+ - description: outbox interrupt
+
+ clocks:
+ maxItems: 1
+
+ clock-names:
+ items:
+ - const: enable
+
+ "#mbox-cells":
+ const: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - "#mbox-cells"
+ - clocks
+ - clock-names
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ mailbox: mailbox@400a0000 {
+ compatible = "sprd,sc9860-mailbox";
+ reg = <0 0x400a0000 0 0x8000>, <0 0x400a8000 0 0x8000>;
+ #mbox-cells = <1>;
+ clock-names = "enable";
+ clocks = <&aon_gate 53>;
+ interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
+ };
+...
--
2.17.1
^ permalink raw reply related
* Re: [PATCH V2 2/3] mmc: sdhci-msm: Use internal voltage control
From: Veerabhadrarao Badiganti @ 2020-05-22 13:27 UTC (permalink / raw)
To: Bjorn Andersson
Cc: adrian.hunter, ulf.hansson, robh+dt, linux-mmc, linux-kernel,
linux-arm-msm, devicetree, Asutosh Das, Vijay Viswanath,
Andy Gross
In-Reply-To: <20200521190739.GC1331782@builder.lan>
Hi Bjorn,
On 5/22/2020 12:37 AM, Bjorn Andersson wrote:
> On Thu 21 May 08:23 PDT 2020, Veerabhadrarao Badiganti wrote:
>
>> On qcom SD host controllers voltage switching be done after the HW
>> is ready for it. The HW informs its readiness through power irq.
>> The voltage switching should happen only then.
>>
>> Use the internal voltage switching and then control the voltage
>> switching using power irq.
>>
>> Set the regulator load as well so that regulator can be configured
>> in LPM mode when in is not being used.
>>
>> Co-developed-by: Asutosh Das <asutoshd@codeaurora.org>
>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>> Co-developed-by: Vijay Viswanath <vviswana@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> Co-developed-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
>> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> Looks better, thanks.
>
>> ---
>> drivers/mmc/host/sdhci-msm.c | 207 +++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 198 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> [..]
>> static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>> @@ -1298,6 +1302,71 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>> sdhci_msm_hs400(host, &mmc->ios);
>> }
>>
>> +static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
>> +{
>> + int ret;
>> +
>> + if (IS_ERR(mmc->supply.vmmc))
>> + return 0;
>> +
>> + ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
>> + if (ret)
>> + dev_err(mmc_dev(mmc), "%s: vmmc set ocr with vdd=%d failed: %d\n",
>> + mmc_hostname(mmc), mmc->ios.vdd, ret);
> Missed this one on v1, in the event that mmc_regulator_set_ocr() return
> a non-zero value it has already printed an error message. So please
> replace the tail with just:
>
> return mmc_regulator_set_ocr(...);
>
>> +
>> + return ret;
>> +}
>> +
>> +static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
>> + struct mmc_host *mmc, bool level)
>> +{
>> + int load, ret;
>> + struct mmc_ios ios;
>> +
>> + if (IS_ERR(mmc->supply.vqmmc) ||
>> + (mmc->ios.power_mode == MMC_POWER_UNDEFINED) ||
>> + (msm_host->vqmmc_enabled == level))
>> + return 0;
>> +
>> + if (msm_host->vqmmc_load) {
>> + load = level ? msm_host->vqmmc_load : 0;
>> + ret = regulator_set_load(mmc->supply.vqmmc, load);
> Sorry for the late reply on v1, but please see my explanation regarding
> load and always-on regulators there.
<Merging your comment from V1 here>
>> You should still call regulator_enable()/regulator_disable() on your
>> consumer regulator in this driver. When you do this the regulator core
>> will conclude that the regulator_dev (i.e. the part that represents the
>> hardware) is marked always_on and will not enable/disable the regulator.
>> But it will still invoke _regulator_handle_consumer_enable() and
>> _regulator_handle_consumer_disable(), which will aggregate the "load" of
>> all client regulators and update the regulator's load.
>> So this will apply the load as you expect regardless of it being
>> supplied by a regulator marked as always_on.
Since I'm not turning off this regulator for eMMC, I wanted to keep it
in LPM mode
to save some power.
When the regulator configured in auto mode (RPMH_REGULATOR_MODE_AUTO) it
switches to LPM/HPM mode based on the active load.
So i have to minimize my driver load requirement so that I can let this
regulator
in LPM mode.
So i need to set load every-time I disable/enable the regulator.
>> + if (ret) {
>> + dev_err(mmc_dev(mmc), "%s: vqmmc set load failed: %d\n",
>> + mmc_hostname(mmc), ret);
>> + goto out;
>> + }
>> + }
>> +
>> + if (level) {
>> + /* Set the IO voltage regulator to default voltage level */
>> + if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
>> + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_330;
>> + else if (msm_host->caps_0 & CORE_1_8V_SUPPORT)
>> + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_180;
>> +
>> + if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
>> + ret = mmc_regulator_set_vqmmc(mmc, &ios);
>> + if (ret < 0) {
>> + dev_err(mmc_dev(mmc), "%s: vqmmc set volgate failed: %d\n",
>> + mmc_hostname(mmc), ret);
>> + goto out;
>> + }
>> + }
>> + ret = regulator_enable(mmc->supply.vqmmc);
>> + } else {
>> + ret = regulator_disable(mmc->supply.vqmmc);
>> + }
>> +
>> + if (ret)
>> + dev_err(mmc_dev(mmc), "%s: vqmm %sable failed: %d\n",
>> + mmc_hostname(mmc), level ? "en":"dis", ret);
>> + else
>> + msm_host->vqmmc_enabled = level;
>> +out:
>> + return ret;
>> +}
> [..]
>> +static int sdhci_msm_start_signal_voltage_switch(struct mmc_host *mmc,
>> + struct mmc_ios *ios)
>> +{
>> + struct sdhci_host *host = mmc_priv(mmc);
>> + u16 ctrl, status;
>> +
>> + /*
>> + * Signal Voltage Switching is only applicable for Host Controllers
>> + * v3.00 and above.
>> + */
>> + if (host->version < SDHCI_SPEC_300)
>> + return 0;
>> +
>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>> +
>> + switch (ios->signal_voltage) {
>> + case MMC_SIGNAL_VOLTAGE_330:
>> + if (!(host->flags & SDHCI_SIGNALING_330))
>> + return -EINVAL;
>> +
>> + /* Set 1.8V Signal Enable in the Host Control2 register to 0 */
>> + ctrl &= ~SDHCI_CTRL_VDD_180;
>> + break;
>> + case MMC_SIGNAL_VOLTAGE_180:
>> + if (!(host->flags & SDHCI_SIGNALING_180))
>> + return -EINVAL;
>> +
>> + /*
>> + * Enable 1.8V Signal Enable in the Host Control2
>> + * register
>> + */
>> + ctrl |= SDHCI_CTRL_VDD_180;
>> + break;
>> + case MMC_SIGNAL_VOLTAGE_120:
>> + if (!(host->flags & SDHCI_SIGNALING_120))
>> + return -EINVAL;
>> + return 0;
>> + default:
>> + /* No signal voltage switch required */
>> + return 0;
>> + }
>> +
>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> +
>> + /* Wait for 5ms */
>> + usleep_range(5000, 5500);
>> +
>> + /* regulator output should be stable within 5 ms */
>> + status = !!(ctrl & SDHCI_CTRL_VDD_180);
>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>> + if (!!(ctrl & SDHCI_CTRL_VDD_180) == status)
> You should be able to drop the !! both here and when assigning status.
>
> Overall this looks neater, thanks for reworking it.
>
> Regards,
> Bjorn
Thanks
Veera
^ permalink raw reply
* Re: [PATCH v4 03/13] mips: Add MIPS Release 5 support
From: Serge Semin @ 2020-05-22 13:15 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Serge Semin, Alexey Malahov, Paul Burton, Ralf Baechle,
Arnd Bergmann, Rob Herring, devicetree, Jiaxun Yang,
Alexander Lobakin, Huacai Chen, Nathan Chancellor, Ard Biesheuvel,
Cedric Hombourger, Thomas Gleixner, Ingo Molnar,
Sebastian Andrzej Siewior, Philippe Mathieu-Daudé,
Guenter Roeck, Paul Cercueil, Zhou Yanjie, Masahiro Yamada,
Greg Kroah-Hartman, Allison Randal, Liangliang Huang,
周琰杰 (Zhou Yanjie), YunQiang Su, Zou Wei,
Oleksij Rempel, Kamal Dasu, linux-mips, linux-kernel, kvm
In-Reply-To: <20200522072743.GA7331@alpha.franken.de>
On Fri, May 22, 2020 at 09:27:43AM +0200, Thomas Bogendoerfer wrote:
> On Thu, May 21, 2020 at 05:07:14PM +0300, Serge Semin wrote:
> > There are five MIPS32/64 architecture releases currently available:
> > from 1 to 6 except fourth one, which was intentionally skipped.
> > Three of them can be called as major: 1st, 2nd and 6th, that not only
> > have some system level alterations, but also introduced significant
> > core/ISA level updates. The rest of the MIPS architecture releases are
> > minor.
> >
> > Even though they don't have as much ISA/system/core level changes
> > as the major ones with respect to the previous releases, they still
> > provide a set of updates (I'd say they were intended to be the
> > intermediate releases before a major one) that might be useful for the
> > kernel and user-level code, when activated by the kernel or compiler.
> > In particular the following features were introduced or ended up being
> > available at/after MIPS32/64 Release 5 architecture:
> > + the last release of the misaligned memory access instructions,
> > + virtualisation - VZ ASE - is optional component of the arch,
> > + SIMD - MSA ASE - is optional component of the arch,
> > + DSP ASE is optional component of the arch,
> > + CP0.Status.FR=1 for CP1.FIR.F64=1 (pure 64-bit FPU general registers)
> > must be available if FPU is implemented,
> > + CP1.FIR.Has2008 support is required so CP1.FCSR.{ABS2008,NAN2008} bits
> > are available.
> > + UFR/UNFR aliases to access CP0.Status.FR from user-space by means of
> > ctc1/cfc1 instructions (enabled by CP0.Config5.UFR),
> > + CP0.COnfig5.LLB=1 and eretnc instruction are implemented to without
> > accidentally clearing LL-bit when returning from an interrupt,
> > exception, or error trap,
> > + XPA feature together with extended versions of CPx registers is
> > introduced, which needs to have mfhc0/mthc0 instructions available.
> >
> > So due to these changes GNU GCC provides an extended instructions set
> > support for MIPS32/64 Release 5 by default like eretnc/mfhc0/mthc0. Even
> > though the architecture alteration isn't that big, it still worth to be
> > taken into account by the kernel software. Finally we can't deny that
> > some optimization/limitations might be found in future and implemented
> > on some level in kernel or compiler. In this case having even
> > intermediate MIPS architecture releases support would be more than
> > useful.
> >
> > So the most of the changes provided by this commit can be split into
> > either compile- or runtime configs related. The compile-time related
> > changes are caused by adding the new CONFIG_CPU_MIPS32_R5/CONFIG_CPU_MIPSR5
> > configs and concern the code activating MIPSR2 or MIPSR6 already
> > implemented features (like eretnc/LLbit, mthc0/mfhc0). In addition
> > CPU_HAS_MSA can be now freely enabled for MIPS32/64 release 5 based
> > platforms as this is done for CPU_MIPS32_R6 CPUs. The runtime changes
> > concerns the features which are handled with respect to the MIPS ISA
> > revision detected at run-time by means of CP0.Config.{AT,AR} bits. Alas
> > these fields can be used to detect either r1 or r2 or r6 releases.
> > But since we know which CPUs in fact support the R5 arch, we can manually
> > set MIPS_CPU_ISA_M32R5/MIPS_CPU_ISA_M64R5 bit of c->isa_level and then
> > use cpu_has_mips32r5/cpu_has_mips64r5 where it's appropriate.
> >
> > Since XPA/EVA provide too complex alterationss and to have them used with
> > MIPS32 Release 2 charged kernels (for compatibility with current platform
> > configs) they are left to be setup as a separate kernel configs.
> >
> > Co-developed-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > ---
> > arch/mips/Kconfig | 56 +++++++++++++++++++++++++---
> > arch/mips/Makefile | 2 +
> > arch/mips/include/asm/asmmacro.h | 18 +++++----
> > arch/mips/include/asm/compiler.h | 5 +++
> > arch/mips/include/asm/cpu-features.h | 27 ++++++++++----
> > arch/mips/include/asm/cpu-info.h | 2 +-
> > arch/mips/include/asm/cpu-type.h | 7 +++-
> > arch/mips/include/asm/cpu.h | 10 +++--
> > arch/mips/include/asm/fpu.h | 4 +-
> > arch/mips/include/asm/hazards.h | 8 ++--
> > arch/mips/include/asm/module.h | 4 ++
> > arch/mips/include/asm/stackframe.h | 2 +-
> > arch/mips/include/asm/switch_to.h | 8 ++--
> > arch/mips/kernel/cpu-probe.c | 17 +++++++++
> > arch/mips/kernel/entry.S | 6 +--
> > arch/mips/kernel/proc.c | 4 ++
> > arch/mips/kernel/r4k_fpu.S | 14 +++----
> > arch/mips/kvm/vz.c | 6 +--
> > arch/mips/lib/csum_partial.S | 6 ++-
> > arch/mips/mm/c-r4k.c | 7 ++--
> > arch/mips/mm/sc-mips.c | 7 ++--
> > 21 files changed, 163 insertions(+), 57 deletions(-)
>
> applied to mips-next. I've changed the two /* fall through */ by fallthrough;
> while appliny. Running checkpatch would have caught that ;-)
Good. Thanks. Actually I've seen that warning, but just didn't know what way to
choose.) So I've decided to leave the comment-based Fall-through fixup seeing
the rest of the file is using the older way. By doing so I've kept the locally
implemented coding style. Though I've heard the explicit attribute "fallthrough;"
utilization is a preferred way of marking combined case statements.
-Sergey
>
> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea. [ RFC1925, 2.3 ]
^ permalink raw reply
* Re: [PATCH v10 2/5] PCI: Add Loongson PCI Controller support
From: Lorenzo Pieralisi @ 2020-05-22 13:10 UTC (permalink / raw)
To: Jiaxun Yang
Cc: linux-pci, Rob Herring, Bjorn Helgaas, Rob Herring,
Thomas Bogendoerfer, Huacai Chen, Paul Burton, devicetree,
linux-kernel, linux-mips
In-Reply-To: <AC29D474-D846-41AF-9900-759CE430A744@flygoat.com>
On Wed, May 20, 2020 at 07:57:29PM +0800, Jiaxun Yang wrote:
>
>
> 于 2020年5月14日 GMT+08:00 下午9:16:38, Jiaxun Yang <jiaxun.yang@flygoat.com> 写到:
> >This controller can be found on Loongson-2K SoC, Loongson-3
> >systems with RS780E/LS7A PCH.
> >
> >The RS780E part of code was previously located at
> >arch/mips/pci/ops-loongson3.c and now it can use generic PCI
> >driver implementation.
> >
> >Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> >Reviewed-by: Rob Herring <robh@kernel.org>
> >
>
> Hi there,
>
> Is it possible to let this series go into next tree soon?
>
> As LS7A dts patch would depend on this series, and I want to
> make the whole LS7A basic support as a part of 5.8 release.
I think you have all necessary tags to take this in the MIPS
tree, please let me know if that's the way we want this to go
upstream - I would not pull MIPS/dts changes into the PCI tree
and I don't think it is needed for this series.
Thanks,
Lorenzo
^ permalink raw reply
* Re: [PATCH v4 2/2] mailbox: sprd: Add Spreadtrum mailbox driver
From: Baolin Wang @ 2020-05-22 13:07 UTC (permalink / raw)
To: Jassi Brar; +Cc: Rob Herring, Orson Zhai, Chunyan Zhang, Devicetree List, LKML
In-Reply-To: <CABb+yY244ZCOk5kDtOR0oEYajwUVbXoSZdNiid__UuYbU=yB-Q@mail.gmail.com>
On Fri, May 22, 2020 at 11:48 AM Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Thu, May 21, 2020 at 7:24 AM Baolin Wang <baolin.wang7@gmail.com> wrote:
> >
> > Hi Jassi,
> >
> > On Wed, May 13, 2020 at 2:32 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
> > >
> > > On Wed, May 13, 2020 at 2:05 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > >
> > > > On Tue, May 12, 2020 at 11:14 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
> > > > >
> > > > > Hi Jassi,
> > > > >
> > > > > On Thu, May 7, 2020 at 11:23 AM Baolin Wang <baolin.wang7@gmail.com> wrote:
> > > > > >
> > > > > > Hi Jassi,
> > > > > >
> > > > > > On Thu, May 7, 2020 at 7:25 AM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > > > > >
> > > > > > > On Wed, May 6, 2020 at 8:29 AM Baolin Wang <baolin.wang7@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Jassi,
> > > > > > > >
> > > > > > > > On Tue, Apr 28, 2020 at 11:10 AM Baolin Wang <baolin.wang7@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > From: Baolin Wang <baolin.wang@unisoc.com>
> > > > > > > > >
> > > > > > > > > The Spreadtrum mailbox controller supports 8 channels to communicate
> > > > > > > > > with MCUs, and it contains 2 different parts: inbox and outbox, which
> > > > > > > > > are used to send and receive messages by IRQ mode.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Baolin Wang <baolin.wang@unisoc.com>
> > > > > > > > > Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
> > > > > > > > > ---
> > > > > > > > > Changes from v3:
> > > > > > > > > - Save the id in mbox_chan.con_priv and remove the 'sprd_mbox_chan'
> > > > > > > > >
> > > > > > > > > Changes from v2:
> > > > > > > > > - None.
> > > > > > > > >
> > > > > > > > > Changes from v1:
> > > > > > > > > - None
> > > > > > > >
> > > > > > > > Gentle ping, do you have any other comments? Thanks.
> > > > > > > >
> > > > > > > Yea, I am still not sure about the error returned in send_data(). It
> > > > > > > will either never hit or there will be no easy recovery from it. The
> > > > > > > api expects the driver to tell it the last-tx was done only when it
> > > > > > > can send the next message. (There may be case like sending depend on
> > > > > > > remote, which can't be ensured before hand).
> > > > > >
> > > > > > Actually this is an unusual case, suppose the remote target did not
> > > > > > fetch the message as soon as possile, which will cause the FIFO
> > > > > > overflow, so in this case we can not send messages to the remote
> > > > > > target any more, otherwise messages will be lost. Thus we can return
> > > > > > errors to users to indicate that something wrong with the remote
> > > > > > target need to be checked.
> > > > > >
> > > > > > So this validation in send_data() is mostly for debugging for this
> > > > > > abnormal case and we will not trigger this issue if the remote target
> > > > > > works well. So I think it is useful to keep this validation in
> > > > > > send_data(). Thanks.
> > > > >
> > > > > Any comments? Thanks.
> > > > >
> > > > Same as my last post.
> > >
> > > I think I've explained the reason why we need add this validation in
> > > my previous email, I am not sure how do you think? You still want to
> > > remove this validation?
> >
> > Gentle ping.
> >
> > As I explained in previous email, this validation is for an unusual
> > case, suppose the remote target did not fetch the message as soon as
> > possile, which will cause the FIFO overflow, so in this case we can
> > not send messages to the remote
> > target any more, otherwise messages will be lost. Thus we can return
> > errors to users to indicate that something wrong with the remote
> > target need to be checked.
> >
> > So this validation in send_data() is mostly for debugging for this
> > abnormal case and we will not trigger this issue if the remote target
> > works well. So I think it is useful to keep this validation in
> > send_data(). What do you think? Thanks.
> >
> I still think the same as before.
> You should do this check before you call mbox_chan_txdone() and wait
> if busy ... which is exactly the purpose of txdone().
> It seems harmless to be paranoid and place a block of code in
> practically "if 0", but that sets bad precedence for other drivers. So
> please move the check before txdone().
OK. I realized I can implement the flush() to make sure the
transmission has been completed. Thanks.
--
Baolin Wang
^ permalink raw reply
* Re: [PATCH] PM / devfreq: fix odd_ptr_err.cocci warnings
From: Julia Lawall @ 2020-05-22 12:59 UTC (permalink / raw)
To: kbuild-all, Andrew-sh.Cheng, MyungJoo Ham, Kyungmin Park,
Chanwoo Choi, Rob Herring, Mark Rutland, Matthias Brugger,
Rafael J . Wysocki, Viresh Kumar, Nishanth Menon, Stephen Boyd,
Liam Girdwood, Mark Brown, devicetree, Andrew-sh . Cheng,
srv_heupstream, linux-pm, linux-kernel, Saravana Kannan,
linux-mediatek, Sibi Sankar, linux-arm-kernel
In-Reply-To: <20200521160908.GA88022@052716d1a29e>
Hello,
This provides a patch, but it doesn't look like the right one. It looks
like the if test should be testing opp_table,
julia
On Fri, 22 May 2020, kbuild test robot wrote:
> From: kbuild test robot <lkp@intel.com>
>
> drivers/devfreq/governor_passive.c:336:7-13: inconsistent IS_ERR and PTR_ERR on line 337.
>
> PTR_ERR should access the value just tested by IS_ERR
>
> Semantic patch information:
> There can be false positives in the patch case, where it is the call to
> IS_ERR that is wrong.
>
> Generated by: scripts/coccinelle/tests/odd_ptr_err.cocci
>
> CC: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: kbuild test robot <lkp@intel.com>
> ---
>
> url: https://github.com/0day-ci/linux/commits/Andrew-sh-Cheng/Add-cpufreq-and-cci-devfreq-for-mt8183-and-SVS-support/20200520-222709
> base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> :::::: branch date: 26 hours ago
> :::::: commit date: 26 hours ago
>
> Please take the patch only if it's a positive warning. Thanks!
>
> governor_passive.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -334,7 +334,7 @@ static int cpufreq_passive_register(stru
>
> opp_table = dev_pm_opp_get_opp_table(cpu_dev);
> if (IS_ERR(devfreq->opp_table)) {
> - ret = PTR_ERR(opp_table);
> + ret = PTR_ERR(devfreq->opp_table);
> goto out;
> }
>
>
^ permalink raw reply
* Re: [PATCH 1/2] soc/tegra: pmc: Enable PMIC wake event on Tegra210
From: Thierry Reding @ 2020-05-22 12:58 UTC (permalink / raw)
To: Jon Hunter; +Cc: devicetree, linux-tegra
In-Reply-To: <20200520151318.15493-1-jonathanh@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 380 bytes --]
On Wed, May 20, 2020 at 04:13:17PM +0100, Jon Hunter wrote:
> The PMIC wake event can be used to bring the system out of suspend based
> on certain events happening on the PMIC (such as an RTC alarm).
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> drivers/soc/tegra/pmc.c | 1 +
> 1 file changed, 1 insertion(+)
Both patches applied, thanks.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
From: Andy Shevchenko @ 2020-05-22 12:34 UTC (permalink / raw)
To: Mark Brown
Cc: Serge Semin, Serge Semin, Linus Walleij, Vinod Koul, Feng Tang,
Grant Likely, Alan Cox, Georgy Vlasov, Ramil Zaripov,
Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
Arnd Bergmann, Rob Herring, linux-mips, devicetree,
Wan Ahmad Zainie, Thomas Gleixner, Jarkko Nikula, wuxu.wu,
Clement Leger, Linus Walleij, linux-spi, linux-kernel
In-Reply-To: <20200522121820.GG5801@sirena.org.uk>
On Fri, May 22, 2020 at 01:18:20PM +0100, Mark Brown wrote:
> On Fri, May 22, 2020 at 03:12:21PM +0300, Andy Shevchenko wrote:
> > On Fri, May 22, 2020 at 02:52:35PM +0300, Serge Semin wrote:
>
> > > Please, see it's implementation. It does atomic delay when the delay value
> > > is less than 10us. But selectively gets to the usleep_range() if value is
> > > greater than that.
>
> > Oh, than it means we may do a very long busy loop here which is not good at
> > all. If we have 10Hz clock, it might take seconds of doing nothing!
>
> Realistically it seems unlikely that the clock will be even as slow as
> double digit kHz though, and if we do I'd not be surprised to see other
> problems kicking in. It's definitely good to handle such things if we
> can but so long as everything is OK for realistic use cases I'm not sure
> it should be a blocker.
Perhaps some kind of warning? Funny that using spi_delay_exec() will issue such
a warning as a side effect of its implementation.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v13 3/3] i2c: npcm7xx: Add support for slave mode for Nuvoton
From: Andy Shevchenko @ 2020-05-22 12:32 UTC (permalink / raw)
To: Tali Perry
Cc: ofery, brendanhiggins, avifishman70, tmaimon77, kfting, venture,
yuenn, benjaminfair, robh+dt, wsa, linux-arm-kernel, linux-i2c,
openbmc, devicetree, linux-kernel
In-Reply-To: <20200522113312.181413-4-tali.perry1@gmail.com>
On Fri, May 22, 2020 at 02:33:12PM +0300, Tali Perry wrote:
> Add support for slave mode for Nuvoton
> NPCM BMC I2C controller driver.
I guess it will require v14, so, few nits below.
...
> +const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
> + NPCM_I2CADDR1, NPCM_I2CADDR2,
> + NPCM_I2CADDR3, NPCM_I2CADDR4,
> + NPCM_I2CADDR5, NPCM_I2CADDR6,
> + NPCM_I2CADDR7, NPCM_I2CADDR8,
> + NPCM_I2CADDR9, NPCM_I2CADDR10,
One TAB is enough.
> + };
No need to indent at all.
...
> + /* Set and enable the address */
> + iowrite8(sa_reg, bus->reg + npcm_i2caddr[(int)addr_type]);
I'm wondering why you need a casting here.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM driver
From: Jon Hunter @ 2020-05-22 12:28 UTC (permalink / raw)
To: Sandipan Patra, Thierry Reding, robh+dt@kernel.org,
u.kleine-koenig@pengutronix.de
Cc: Bibek Basu, Laxman Dewangan, linux-pwm@vger.kernel.org,
devicetree@vger.kernel.org, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <BYAPR12MB30149CEB64B1BC3F9727AA68ADB40@BYAPR12MB3014.namprd12.prod.outlook.com>
On 22/05/2020 13:12, Sandipan Patra wrote:
...
>>>>> /*
>>>>> * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
>>>>> * cycles at the PWM clock rate will take period_ns nanoseconds.
>>>>> */
>>>>> - rate = pc->clk_rate >> PWM_DUTY_WIDTH;
>>>>> + if (pc->soc->num_channels == 1) {
>>>>
>>>> Are you using num_channels to determine if Tegra uses the BPMP? If so
>>>> then the above is not really correct, because num_channels is not
>>>> really related to what is being done here. So maybe you need a new SoC
>> attribute in the soc data.
>>>
>>> Here, it tries to find if pwm controller uses multiple channels (like
>>> in Tegra210 or older) or single channel for every pwm instance (i.e.
>>> T186, T194). If found multiple channels on a single controller then it
>>> is not correct to configure separate clock rates to each of the channels. So to
>> distinguish the controller and channel type, num_channels is referred.
>>
>> OK, then that makes sense. Maybe add this detail to the comment about why
>> num_channels is used.
>
> Ok. Will update comment.
>
>>
>>>>
>>>>> + /*
>>>>> + * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it
>>>> matches
>>>>> + * with the hieghest applicable rate that the controller can
>>>>
>>>> s/hieghest/highest/
>>>
>>> Got it.
>>>
>>>>
>>>>> + * provide. Any further lower value can be derived by setting
>>>>> + * PFM bits[0:12].
>>>>> + * Higher mark is taken since BPMP has round-up mechanism
>>>>> + * implemented.
>>>>> + */
>>>>> + required_clk_rate =
>>>>> + (NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
>>>>> +
>>>>
>>>> Should be we checking the rate against the max rate supported?
>>>
>>> If the request rate is beyond max supported rate, then the
>>> clk_set_rate will be failing and can get caught with error check
>>> followed by. Otherwise it will fail through fitting in the register's frequency
>> divider filed. So I think it is not required to check against max rate.
>>> Please advise if I am not able to follow with what you are suggesting.
>>
>> I think that it would be better to update the cached value so that it is not
>> incorrectly used else where by any future change. Furthermore, this simplifies
>> matters a bit because you can do the following for all devices, but only update
>> the clk_rate for those you wish to ...
>>
>> rate = pc->clk_rate >> PWM_DUTY_WIDTH;
>>
> What I understood from above is, we will always use max rate for any further configurations.
> If this is the suggestion above, then I think its not the right way.
I am not saying that.
> If we consider only max rate then the pwm output can only be ranging from:
> Possible max output rate: rate
> Possible min output rate: rate/2^13 (13 bits frequency divisor)
>
> But if we consider the min rate supported by the source clock then,
> min output rate can go beyond the current min possible and
> that should be considered for finding actual limit of min output rate.
>
> Based on this, in the driver it tries to find a suitable clock rate to achieve
> requested output rate.
> Please suggest if you think we can still improve this further.
What I am suggesting is you ...
if (pc->soc->num_channels == 1) {
required_clk_rate = (NSEC_PER_SEC / period_ns) <<
PWM_DUTY_WIDTH;
err = clk_set_rate(pc->clk, required_clk_rate);
if (err < 0)
return -EINVAL;
pc->clk_rate = clk_get_rate(pc->clk);
}
rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
That's all. I think this is simpler.
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH v13 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
From: Andy Shevchenko @ 2020-05-22 12:27 UTC (permalink / raw)
To: Tali Perry
Cc: ofery, brendanhiggins, avifishman70, tmaimon77, kfting, venture,
yuenn, benjaminfair, robh+dt, wsa, linux-arm-kernel, linux-i2c,
openbmc, devicetree, linux-kernel
In-Reply-To: <20200522113312.181413-3-tali.perry1@gmail.com>
On Fri, May 22, 2020 at 02:33:11PM +0300, Tali Perry wrote:
> Add Nuvoton NPCM BMC I2C controller driver.
I thought we are waiting for Wolfram finishing his review...
In any case see couple of comments below.
...
> +#ifdef CONFIG_DEBUG_FS
Now, do we need the rest of DEBUG_FS guards?
> + if (status) {
> + if (bus->rec_fail_cnt == ULLONG_MAX) {
> + dev_dbg(bus->dev, "rec_fail_cnt reach max, reset to 0");
> + bus->rec_fail_cnt = 0;
It's redundant, since we will anyway roll over when incrementing.
https://stackoverflow.com/q/18195715/2511795
> + }
> + bus->rec_fail_cnt++;
> + } else {
> + if (bus->rec_succ_cnt == ULLONG_MAX) {
> + dev_dbg(bus->dev, "rec_succ_cnt reach max, reset to 0");
> + bus->rec_succ_cnt = 0;
Ditto.
> + }
> + bus->rec_succ_cnt++;
> + }
> +#endif
...
> +static int npcm_i2c_remove_bus(struct platform_device *pdev)
> +{
> + unsigned long lock_flags;
> + struct npcm_i2c *bus = platform_get_drvdata(pdev);
> +
> + spin_lock_irqsave(&bus->lock, lock_flags);
> + npcm_i2c_disable(bus);
> + spin_unlock_irqrestore(&bus->lock, lock_flags);
> + i2c_del_adapter(&bus->adap);
> + debugfs_remove_recursive(bus->debugfs);
This should be in reversed order, i.e. you inited last in ->probe(), thus
should remove first in ->remove().
> + return 0;
> +}
...
> +static int __init npcm_i2c_init(void)
> +{
> + struct dentry *dir;
> +
> + dir = debugfs_create_dir("i2c", NULL);
> + if (IS_ERR_OR_NULL(dir))
IS_ERR() is redundant. And NULL already being checked inside i2c_init_debugfs()
or how do you call it?
> + return 0;
> +
> + npcm_i2c_debugfs_dir = dir;
> + return 0;
> +}
> +
> +static void __exit npcm_i2c_exit(void)
> +{
> + debugfs_remove_recursive(npcm_i2c_debugfs_dir);
> +}
> +
> +module_init(npcm_i2c_init);
> +module_exit(npcm_i2c_exit);
Slightly better to attach to the respective function, like other macros above
do.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH net-next v2 2/4] net: phy: Add a helper to return the index for of the internal delay
From: Dan Murphy @ 2020-05-22 12:25 UTC (permalink / raw)
To: andrew, f.fainelli, hkallweit1, davem, robh
Cc: netdev, linux-kernel, devicetree, Dan Murphy
In-Reply-To: <20200522122534.3353-1-dmurphy@ti.com>
Add a helper function that will return the index in the array for the
passed in internal delay value. The helper requires the array, size and
delay value.
The helper will then return the index for the exact match or return the
index for the index to the closest smaller value.
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
drivers/net/phy/phy_device.c | 45 ++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 2 ++
2 files changed, 47 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 7481135d27ab..40f53b379d2b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2661,6 +2661,51 @@ void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause)
}
EXPORT_SYMBOL(phy_get_pause);
+/**
+ * phy_get_delay_index - returns the index of the internal delay
+ * @phydev: phy_device struct
+ * @delay_values: array of delays the PHY supports
+ * @size: the size of the delay array
+ * @delay: the delay to be looked up
+ *
+ * Returns the index within the array of internal delay passed in.
+ */
+int phy_get_delay_index(struct phy_device *phydev, int *delay_values, int size,
+ int delay)
+{
+ int i;
+
+ if (size <= 0)
+ return -EINVAL;
+
+ if (delay <= delay_values[0])
+ return 0;
+
+ if (delay > delay_values[size - 1])
+ return size - 1;
+
+ for (i = 0; i < size; i++) {
+ if (delay == delay_values[i])
+ return i;
+
+ /* Find an approximate index by looking up the table */
+ if (delay > delay_values[i - 1] &&
+ delay < delay_values[i]) {
+ if (delay - delay_values[i - 1] < delay_values[i] - delay)
+ return i - 1;
+ else
+ return i;
+ }
+
+ }
+
+ phydev_err(phydev, "error finding internal delay index for %d\n",
+ delay);
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(phy_get_delay_index);
+
static bool phy_drv_supports_irq(struct phy_driver *phydrv)
{
return phydrv->config_intr && phydrv->ack_interrupt;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2bcdf19ed3b4..73552612c189 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1408,6 +1408,8 @@ void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
bool phy_validate_pause(struct phy_device *phydev,
struct ethtool_pauseparam *pp);
void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause);
+int phy_get_delay_index(struct phy_device *phydev, int *delay_values,
+ int size, int delay);
void phy_resolve_pause(unsigned long *local_adv, unsigned long *partner_adv,
bool *tx_pause, bool *rx_pause);
--
2.26.2
^ permalink raw reply related
* [PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay configuration
From: Dan Murphy @ 2020-05-22 12:25 UTC (permalink / raw)
To: andrew, f.fainelli, hkallweit1, davem, robh
Cc: netdev, linux-kernel, devicetree, Dan Murphy
In-Reply-To: <20200522122534.3353-1-dmurphy@ti.com>
Add RGMII internal delay configuration for Rx and Tx.
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
drivers/net/phy/dp83869.c | 101 ++++++++++++++++++++++++++++++++++++++
1 file changed, 101 insertions(+)
diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index cfb22a21a2e6..a9008d32e2b6 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -99,6 +99,14 @@
#define DP83869_OP_MODE_MII BIT(5)
#define DP83869_SGMII_RGMII_BRIDGE BIT(6)
+/* RGMIIDCTL bits */
+#define DP83869_RGMII_TX_CLK_DELAY_SHIFT 4
+#define DP83869_RGMII_CLK_DELAY_INV 0
+
+static int dp83869_internal_delay[] = {250, 500, 750, 1000, 1250, 1500, 1750,
+ 2000, 2250, 2500, 2750, 3000, 3250,
+ 3500, 3750, 4000};
+
enum {
DP83869_PORT_MIRRORING_KEEP,
DP83869_PORT_MIRRORING_EN,
@@ -108,6 +116,8 @@ enum {
struct dp83869_private {
int tx_fifo_depth;
int rx_fifo_depth;
+ u32 rx_id_delay;
+ u32 tx_id_delay;
int io_impedance;
int port_mirroring;
bool rxctrl_strap_quirk;
@@ -182,6 +192,7 @@ static int dp83869_of_init(struct phy_device *phydev)
struct dp83869_private *dp83869 = phydev->priv;
struct device *dev = &phydev->mdio.dev;
struct device_node *of_node = dev->of_node;
+ int delay_size = ARRAY_SIZE(dp83869_internal_delay);
int ret;
if (!of_node)
@@ -232,6 +243,26 @@ static int dp83869_of_init(struct phy_device *phydev)
&dp83869->tx_fifo_depth))
dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
+ dp83869->rx_id_delay = DP83869_RGMII_CLK_DELAY_INV;
+ ret = of_property_read_u32(of_node, "rx-internal-delay-ps",
+ &dp83869->rx_id_delay);
+ if (!ret && dp83869->rx_id_delay > dp83869_internal_delay[delay_size]) {
+ phydev_err(phydev,
+ "rx-internal-delay value of %u out of range\n",
+ dp83869->rx_id_delay);
+ return -EINVAL;
+ }
+
+ dp83869->tx_id_delay = DP83869_RGMII_CLK_DELAY_INV;
+ ret = of_property_read_u32(of_node, "tx-internal-delay-ps",
+ &dp83869->tx_id_delay);
+ if (!ret && dp83869->tx_id_delay > dp83869_internal_delay[delay_size]) {
+ phydev_err(phydev,
+ "tx-internal-delay value of %u out of range\n",
+ dp83869->tx_id_delay);
+ return -EINVAL;
+ }
+
return ret;
}
#else
@@ -270,6 +301,29 @@ static int dp83869_configure_rgmii(struct phy_device *phydev,
return ret;
}
+static int dp83869_verify_rgmii_cfg(struct phy_device *phydev)
+{
+ struct dp83869_private *dp83869 = phydev->priv;
+
+ /* RX delay *must* be specified if internal delay of RX is used. */
+ if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+ phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) &&
+ dp83869->rx_id_delay == DP83869_RGMII_CLK_DELAY_INV) {
+ phydev_err(phydev, "ti,rx-internal-delay must be specified\n");
+ return -EINVAL;
+ }
+
+ /* TX delay *must* be specified if internal delay of TX is used. */
+ if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+ phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) &&
+ dp83869->tx_id_delay == DP83869_RGMII_CLK_DELAY_INV) {
+ phydev_err(phydev, "ti,tx-internal-delay must be specified\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int dp83869_configure_mode(struct phy_device *phydev,
struct dp83869_private *dp83869)
{
@@ -371,6 +425,12 @@ static int dp83869_config_init(struct phy_device *phydev)
{
struct dp83869_private *dp83869 = phydev->priv;
int ret, val;
+ int delay_size = ARRAY_SIZE(dp83869_internal_delay);
+ int delay = 0;
+
+ ret = dp83869_verify_rgmii_cfg(phydev);
+ if (ret)
+ return ret;
ret = dp83869_configure_mode(phydev, dp83869);
if (ret)
@@ -394,6 +454,47 @@ static int dp83869_config_init(struct phy_device *phydev)
dp83869->clk_output_sel <<
DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT);
+ if (phy_interface_is_rgmii(phydev)) {
+ val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL);
+
+ val &= ~(DP83869_RGMII_TX_CLK_DELAY_EN | DP83869_RGMII_RX_CLK_DELAY_EN);
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
+ val |= (DP83869_RGMII_TX_CLK_DELAY_EN | DP83869_RGMII_RX_CLK_DELAY_EN);
+
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+ val |= DP83869_RGMII_TX_CLK_DELAY_EN;
+
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+ val |= DP83869_RGMII_RX_CLK_DELAY_EN;
+
+ phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL, val);
+
+ if (dp83869->rx_id_delay) {
+ val = phy_get_delay_index(phydev,
+ &dp83869_internal_delay[0],
+ delay_size,
+ dp83869->rx_id_delay);
+ if (val < 0)
+ return val;
+
+ delay |= val;
+ }
+
+ if (dp83869->tx_id_delay) {
+ val = phy_get_delay_index(phydev,
+ &dp83869_internal_delay[0],
+ delay_size,
+ dp83869->tx_id_delay);
+ if (val < 0)
+ return val;
+
+ delay |= val << DP83869_RGMII_TX_CLK_DELAY_SHIFT;
+ }
+
+ phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIIDCTL,
+ delay);
+ }
+
return ret;
}
--
2.26.2
^ permalink raw reply related
* [PATCH net-next v2 3/4] dt-bindings: net: Add RGMII internal delay for DP83869
From: Dan Murphy @ 2020-05-22 12:25 UTC (permalink / raw)
To: andrew, f.fainelli, hkallweit1, davem, robh
Cc: netdev, linux-kernel, devicetree, Dan Murphy
In-Reply-To: <20200522122534.3353-1-dmurphy@ti.com>
Add the internal delay values into the header and update the binding
with the internal delay properties.
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
.../devicetree/bindings/net/ti,dp83869.yaml | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/ti,dp83869.yaml b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
index 5b69ef03bbf7..2971dd3fc039 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83869.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
@@ -64,6 +64,20 @@ properties:
Operational mode for the PHY. If this is not set then the operational
mode is set by the straps. see dt-bindings/net/ti-dp83869.h for values
+ rx-internal-delay-ps:
+ $ref: "#/properties/rx-internal-delay-ps"
+ description: Delay is in pico seconds
+ enum: [ 250, 500, 750, 1000, 1250, 1500, 1750, 2000, 2250, 2500, 2750, 3000,
+ 3250, 3500, 3750, 4000 ]
+ default: 2000
+
+ tx-internal-delay-ps:
+ $ref: "#/properties/tx-internal-delay-ps"
+ description: Delay is in pico seconds
+ enum: [ 250, 500, 750, 1000, 1250, 1500, 1750, 2000, 2250, 2500, 2750, 3000,
+ 3250, 3500, 3750, 4000 ]
+ default: 2000
+
required:
- reg
@@ -80,5 +94,7 @@ examples:
ti,op-mode = <DP83869_RGMII_COPPER_ETHERNET>;
ti,max-output-impedance = "true";
ti,clk-output-sel = <DP83869_CLK_O_SEL_CHN_A_RCLK>;
+ rx-internal-delay-ps = <2000>;
+ tx-internal-delay-ps = <2000>;
};
};
--
2.26.2
^ permalink raw reply related
* [PATCH net-next v2 1/4] dt-bindings: net: Add tx and rx internal delays
From: Dan Murphy @ 2020-05-22 12:25 UTC (permalink / raw)
To: andrew, f.fainelli, hkallweit1, davem, robh
Cc: netdev, linux-kernel, devicetree, Dan Murphy
In-Reply-To: <20200522122534.3353-1-dmurphy@ti.com>
tx-internal-delays and rx-internal-delays are a common setting for RGMII
capable devices.
These properties are used when the phy-mode or phy-controller is set to
rgmii-id, rgmii-rxid or rgmii-txid. These modes indicate to the
controller that the PHY will add the internal delay for the connection.
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
v2 - updated to add -ps
.../bindings/net/ethernet-controller.yaml | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index ac471b60ed6a..70702a4ef5e8 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -143,6 +143,20 @@ properties:
Specifies the PHY management type. If auto is set and fixed-link
is not specified, it uses MDIO for management.
+ rx-internal-delay-ps:
+ $ref: /schemas/types.yaml#definitions/uint32
+ description: |
+ RGMII Receive PHY Clock Delay defined in pico seconds. This is used for
+ PHY's that have configurable RX internal delays. This property is only
+ used when the phy-mode or phy-connection-type is rgmii-id or rgmii-rxid.
+
+ tx-internal-delay-ps:
+ $ref: /schemas/types.yaml#definitions/uint32
+ description: |
+ RGMII Transmit PHY Clock Delay defined in pico seconds. This is used for
+ PHY's that have configurable TX internal delays. This property is only
+ used when the phy-mode or phy-connection-type is rgmii-id or rgmii-txid.
+
fixed-link:
allOf:
- if:
--
2.26.2
^ permalink raw reply related
* [PATCH net-next v2 0/4] RGMII Internal delay common property
From: Dan Murphy @ 2020-05-22 12:25 UTC (permalink / raw)
To: andrew, f.fainelli, hkallweit1, davem, robh
Cc: netdev, linux-kernel, devicetree, Dan Murphy
Hello
The RGMII internal delay is a common setting found in most RGMII capable PHY
devices. It was found that many vendor specific device tree properties exist
to do the same function. This creates a common property to be used for PHY's
that have tunable internal delays for the Rx and Tx paths.
Dan Murphy (4):
dt-bindings: net: Add tx and rx internal delays
net: phy: Add a helper to return the index for of the internal delay
dt-bindings: net: Add RGMII internal delay for DP83869
net: dp83869: Add RGMII internal delay configuration
.../bindings/net/ethernet-controller.yaml | 14 +++
.../devicetree/bindings/net/ti,dp83869.yaml | 16 +++
drivers/net/phy/dp83869.c | 101 ++++++++++++++++++
drivers/net/phy/phy_device.c | 45 ++++++++
include/linux/phy.h | 2 +
5 files changed, 178 insertions(+)
--
2.26.2
^ permalink raw reply
* Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
From: Serge Semin @ 2020-05-22 12:25 UTC (permalink / raw)
To: Feng Tang
Cc: Serge Semin, Mark Brown, Grant Likely, Vinod Koul, Alan Cox,
Linus Walleij, Georgy Vlasov, Ramil Zaripov, Alexey Malahov,
Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
Andy Shevchenko, Rob Herring, linux-mips, devicetree,
Jarkko Nikula, Thomas Gleixner, Wan Ahmad Zainie, Linus Walleij,
Clement Leger, linux-spi, linux-kernel
In-Reply-To: <20200522120325.GD12568@shbuild999.sh.intel.com>
On Fri, May 22, 2020 at 08:03:25PM +0800, Feng Tang wrote:
> On Fri, May 22, 2020 at 02:32:35PM +0300, Serge Semin wrote:
> > On Fri, May 22, 2020 at 03:58:44PM +0800, Feng Tang wrote:
> > > Hi Serge,
> > >
> > > On Thu, May 21, 2020 at 06:33:17PM +0300, Serge Semin wrote:
> > > > > > > > + dw_spi_dma_wait_rx_done(dws);
> > > > > > >
> > > > > > > I can understand the problem about TX, but I don't see how RX
> > > > > > > will get hurt, can you elaborate more? thanks
> > > > > > >
> > > > > > > - Feng
> > > > > >
> > > > > > Your question is correct. You are right with your hypothesis. Ideally upon the
> > > > > > dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's why the
> > > > > > commit log signifies the error being mostly related with Tx FIFO. But
> > > > > > practically there are many reasons why Rx FIFO might be left with data:
> > > > > > DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA driver
> > > > > > messed something up), controller hanging up, and so on. It's better to catch
> > > > > > an error at this stage while propagating it up to the SPI device drivers.
> > > > > > Especially seeing the wait-check implementation doesn't gives us much of the
> > > > > > execution overhead in normal conditions. So by calling dw_spi_dma_wait_rx_done()
> > > > > > we make sure that all the data has been fetched and we may freely get the
> > > > > > buffers back to the client driver.
> > > > >
> > > > > I see your point about checking RX. But I still don't think checking
> > > > > RX FIFO level is the right way to detect error. Some data left in
> > > > > RX FIFO doesn't always mean a error, say for some case if there is
> > > > > 20 words in RX FIFO, and the driver starts a DMA request for 16
> > > > > words, then after a sucessful DMA transaction, there are 4 words
> > > > > left without any error.
> > > >
> > > > Neither Tx nor Rx FIFO should be left with any data after transaction is
> > > > finished. If they are then something has been wrong.
> > > >
> > > > See, every SPI transfer starts with FIFO clearance since we disable/enable the
> > > > SPI controller by means of the SSIENR (spi_enable_chip(dws, 0) and
> > > > spi_enable_chip(dws, 1) called in the dw_spi_transfer_one() callback). Here is the
> > > > SSIENR register description: "It enables and disables all SPI Controller operations.
> > > > When disabled, all serial transfers are halted immediately. Transmit and receive
> > > > FIFO buffers are cleared when the device is disabled. It is impossible to program
> > > > some of the SPI Controller control registers when enabled"
> > > >
> > > > No mater whether we start DMA request or perform the normal IRQ-based PIO, we
> > > > request as much data as we need and neither Tx nor Rx FIFO are supposed to
> > > > be left with any data after the request is finished. If data is left, then
> > > > either we didn't push all of the necessary data to the SPI bus, or we didn't
> > > > pull all the data from the FIFO, and this could have happened only due to some
> > > > component mulfunction (drivers, DMA engine, SPI device). In any case the SPI
> > > > device driver should be notified about the problem.
> > >
> > > Data left in TX FIFO and Data left in RX FIFO are 2 different stories. The
> > > former in dma case means the dma hw/driver has done its job, and spi hw/driver
> > > hasn't done its job of pushing out the data to spi slave devices,
> >
> > Agreed.
> >
> > > while the
> > > latter means the spi hw/driver has done its job, while the dma hw/driver hasn't.
> >
> > In this particular case agreed, that the data left in the Rx FIFO means DMA
> > hw/driver hasn't done its work right. Though SPI hw could be also a reason of
> > the data left in FIFO (though this only a theoretical consideration).
>
> Right, that's why I was initially very curious about this RX FIFO thing,
> and if possible, please give some details in commit log about the data
> left in TX FIFO problem, which will help future developers when they
> met simliar bugs.
Ok. I'll add a more descriptive patch log.
>
> And I'm fine with adding the rx check, no matter the problem is in
> dma side or spi side.
>
> > >
> > > And the code is called inside the dma rx channel callback, which means the
> > > dma driver is saying "hey, I've done my job", but apparently it hasn't if
> > > there is data left.
> >
> > Right, either it hasn't, or the DMA engine claimed it has, but still is doing
> > something (asynchronously or something, depending on the hardware implementation),
> > or it think it has, but in fact it hasn't due to whatever problem happened
> > (software/hardware/etc.). In anyway we have to at least check whether it's
> > really done with fetching data and to be on a safe side give it some time to
> > make sure that the Rx FIFO isn't going to be emptied. Whatever problem it is
> > having a non empty Rx FIFO at the stage of calling spi_finalize_current_transfer()
> > means a certain error.
> >
> > >
> > > As for the wait time
> > >
> > > + nents = dw_readl(dws, DW_SPI_RXFLR);
> > > + ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
> > > + BITS_PER_BYTE;
> > >
> > > Using this formula for checking TX makes sense, but it doesn't for RX.
> > > Because the time of pushing data in TX FIFO to spi device depends on
> > > the clk, but the time of transferring RX FIFO to memory is up to
> > > the DMA controller and peripheral bus.
> >
> > On this I agree with you. That formulae doesn't describe exactly the time left
> > before the Rx FIFO gets empty. But at least it provides an upper limit on the
> > time needed for the peripheral bus to fetch the data from FIFO. If for some
> > reason the internal APB bus is slower than the SPI bus, then the hardware
> > engineers screwed, since the CPU/DMA won't keep up with pulling data from Rx
> > FIFO on time so the FIFO may get overflown. Though in this case CPU/DMA won't
> > be able to push data to the Tx FIFO fast enough to cause the Rx FIFO overflown,
> > so the problem might be unnoticeable until we enable the EEPROM-read or Rx-only
> > modes of the DW APB SSI controller. Anyway I am pretty much sure all the systems
> > have the internal bus much faster than the external SPI bus.
> >
> > Getting back to the formulae. I was thinking of how to make it better and here
> > is what we can do. We can't predict neither the DMA controller performance,
> > nor the performance of its driver. In this case we have no choice but to add
> > some assumption to clarify the task. Let's assume that the reason why Rx FIFO is
> > non-empty is that even though we are at the DMA completion callback, but the
> > DMA controller is still fetching data in background (any other reason might be
> > related with a bug, so we'll detect it here anyway). In this case we need to
> > give it a time to finish its work. As far as I can see the DW_apb_ssi interface
> > doesn't use PREADY APB signal, which means the IO access cycle will take 4
> > reference clock periods for each read and write accesses. Thus taking all of
> > these into account we can create the next formulae to measure the time needed to
> > read all the data from the Rx FIFO:
> >
> > - ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
> > - BITS_PER_BYTE;
> > + ns = (NSEC_PER_SEC / dws->max_freq) * nents * 4;
> >
> > By doing several busy-wait loop iteration we'll cover the DMA controller and
> > it's driver possible latency.
> >
> > Feng, does it now makes sense for you now? If so, I'll replace the delay
> > calculation formulae in the patch.
>
> Frankly I don't have a good idea, if it really happens which means
> something is abnormal, explicitly waiting for some micro-seconds may
> also be acceptable?
Well, If we can estimate the real delay, then it will be more preferred solution.
Hard-coding a single number is only an option if there isn't any other way.
>
> > >
> > > Also for the
> > >
> > > + while (dw_spi_dma_rx_busy(dws) && retry--)
> > > + ndelay(ns);
> > > +
> > >
> > > the rx busy bit is cleared after this rx/tx checking, and it should
> > > be always true at this point. Am I mis-reading the code?
> >
> > Sorry I don't get your logic here. I am not checking the Rx busy bit here,
> > but the Rx FIFO non-empty bit. Also SR register bits aren't cleared on read,
> > so the status bits are left pending until the reason is cleared. In our case
> > until Rx FIFO gets empty, which will happen eventually either at the point of
> > all data finally being extracted from it or when the controller is disabled
> > by means of the SSIENR register.
>
> I did misread the code, I thought it is checking the busy bits, sorry
> for that. Though the dw_spi_dma_rx_busy() name is a little confusing,
> as checking the emptiness of RX FIFO is not dma bound.
dw_spi_dma_* is a common prefix for all methods implemented in this module.
As I said having the Rx FIFO non-empty could mean that DMA in fact busy reading
data from the Rx FIFO or a bug or etc. Also the naming correlates with the
dw_spi_dma_tx_busy() method, which also doesn't mean DMA is busy with doing
something, but SPI Tx engine is busy with pushing data out to the SPI bus.
-Sergey
>
> Thanks,
> Feng
^ permalink raw reply
* Re: [PATCH v4 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
From: Mark Brown @ 2020-05-22 12:18 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Serge Semin, Serge Semin, Linus Walleij, Vinod Koul, Feng Tang,
Grant Likely, Alan Cox, Georgy Vlasov, Ramil Zaripov,
Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
Arnd Bergmann, Rob Herring, linux-mips, devicetree,
Wan Ahmad Zainie, Thomas Gleixner, Jarkko Nikula, wuxu.wu,
Clement Leger, Linus Walleij, linux-spi, linux-kernel
In-Reply-To: <20200522121221.GA1634618@smile.fi.intel.com>
[-- Attachment #1: Type: text/plain, Size: 770 bytes --]
On Fri, May 22, 2020 at 03:12:21PM +0300, Andy Shevchenko wrote:
> On Fri, May 22, 2020 at 02:52:35PM +0300, Serge Semin wrote:
> > Please, see it's implementation. It does atomic delay when the delay value
> > is less than 10us. But selectively gets to the usleep_range() if value is
> > greater than that.
> Oh, than it means we may do a very long busy loop here which is not good at
> all. If we have 10Hz clock, it might take seconds of doing nothing!
Realistically it seems unlikely that the clock will be even as slow as
double digit kHz though, and if we do I'd not be surprised to see other
problems kicking in. It's definitely good to handle such things if we
can but so long as everything is OK for realistic use cases I'm not sure
it should be a blocker.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* RE: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM driver
From: Sandipan Patra @ 2020-05-22 12:12 UTC (permalink / raw)
To: Jonathan Hunter, Thierry Reding, robh+dt@kernel.org,
u.kleine-koenig@pengutronix.de
Cc: Bibek Basu, Laxman Dewangan, linux-pwm@vger.kernel.org,
devicetree@vger.kernel.org, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <21428954-41cc-c01a-bca2-7eb19f444272@nvidia.com>
Hi Jon,
> -----Original Message-----
> From: Jonathan Hunter <jonathanh@nvidia.com>
> Sent: Friday, May 22, 2020 5:20 PM
> To: Sandipan Patra <spatra@nvidia.com>; Thierry Reding
> <treding@nvidia.com>; robh+dt@kernel.org; u.kleine-koenig@pengutronix.de
> Cc: Bibek Basu <bbasu@nvidia.com>; Laxman Dewangan
> <ldewangan@nvidia.com>; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-tegra@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH V2] pwm: tegra: dynamic clk freq configuration by PWM
> driver
>
>
>
> On 22/05/2020 12:01, Sandipan Patra wrote:
> > Thanks Jonathan,
> > Please help reviewing further with my replies inline.
> >
> >
> > Thanks & Regards,
> > Sandipan
> >
> >> -----Original Message-----
> >> From: Jonathan Hunter <jonathanh@nvidia.com>
> >> Sent: Friday, May 22, 2020 3:54 PM
> >> To: Sandipan Patra <spatra@nvidia.com>; Thierry Reding
> >> <treding@nvidia.com>; robh+dt@kernel.org;
> >> u.kleine-koenig@pengutronix.de
> >> Cc: Bibek Basu <bbasu@nvidia.com>; Laxman Dewangan
> >> <ldewangan@nvidia.com>; linux-pwm@vger.kernel.org;
> >> devicetree@vger.kernel.org; linux-tegra@vger.kernel.org; linux-
> >> kernel@vger.kernel.org
> >> Subject: Re: [PATCH V2] pwm: tegra: dynamic clk freq configuration by
> >> PWM driver
> >>
> >>
> >> On 20/04/2020 16:54, Sandipan Patra wrote:
> >>> Added support for dynamic clock freq configuration in pwm kernel driver.
> >>> Earlier the pwm driver used to cache boot time clock rate by pwm
> >>> clock parent during probe. Hence dynamically changing pwm frequency
> >>> was not possible for all the possible ranges. With this change,
> >>> dynamic calculation is enabled and it is able to set the requested
> >>> period from sysfs knob provided the value is supported by clock source.
> >>>
> >>> Changes mainly have 2 parts:
> >>> - T186 and later chips [1]
> >>> - T210 and prior chips [2]
> >>>
> >>> For [1] - Changes implemented to set pwm period dynamically and
> >>> also checks added to allow only if requested period(ns) is
> >>> below or equals to higher range.
> >>>
> >>> For [2] - Only checks if the requested period(ns) is below or equals
> >>> to higher range defined by max clock limit. The limitation
> >>> in T210 or prior chips are due to the reason of having only
> >>> one pwm-controller supporting multiple channels. But later
> >>> chips have multiple pwm controller instances each having
> >>> single channel support.
> >>>
> >>> Signed-off-by: Sandipan Patra <spatra@nvidia.com>
> >>> ---
> >>> V2:
> >>> 1. Min period_ns calculation is moved to probe.
> >>> 2. Added descriptioins for PWM register bits and regarding behaviour
> >>> of the controller when new configuration is applied or pwm is disabled.
> >>> 3. Setting period with possible value when supplied period is below limit.
> >>> 4. Corrected the earlier code comment:
> >>> plus 1 instead of minus 1 during pwm calculation
> >>>
> >>> drivers/pwm/pwm-tegra.c | 110
> >>> +++++++++++++++++++++++++++++++++++++++++-------
> >>> 1 file changed, 94 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c index
> >>> d26ed8f..7a36325 100644
> >>> --- a/drivers/pwm/pwm-tegra.c
> >>> +++ b/drivers/pwm/pwm-tegra.c
> >>> @@ -4,8 +4,39 @@
> >>> *
> >>> * Tegra pulse-width-modulation controller driver
> >>> *
> >>> - * Copyright (c) 2010, NVIDIA Corporation.
> >>> - * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer
> >>> <s.hauer@pengutronix.de>
> >>> + * Copyright (c) 2010-2020, NVIDIA Corporation.
> >>> + *
> >>> + * Overview of Tegra Pulse Width Modulator Register:
> >>> + * 1. 13-bit: Frequency division (SCALE)
> >>> + * 2. 8-bit : Puls division (DUTY)
> >>> + * 3. 1-bit : Enable bit
> >>> + *
> >>> + * The PWM clock frequency is divided by 256 before subdividing it
> >>> + based
> >>> + * on the programmable frequency division value to generate the
> >>> + required
> >>> + * frequency for PWM output. The maximum output frequency that can
> >>> + be
> >>> + * achieved is (max rate of source clock) / 256.
> >>> + * i.e. if source clock rate is 408 MHz, maximum output frequency cab be:
> >>> + * 408 MHz/256 = 1.6 MHz.
> >>> + * This 1.6 MHz frequency can further be divided using SCALE value in
> PWM.
> >>> + *
> >>> + * PWM pulse width: 8 bits are usable [23:16] for varying pulse width.
> >>> + * To achieve 100% duty cycle, program Bit [24] of this register to
> >>> + * 1’b1. In which case the other bits [23:16] are set to don't care.
> >>> + *
> >>> + * Limitations and known facts:
> >>> + * - When PWM is disabled, the output is driven to 0.
> >>> + * - It does not allow the current PWM period to complete and
> >>> + * stops abruptly.
> >>> + *
> >>> + * - If the register is reconfigured while pwm is running,
> >>> + * It does not let the currently running period to complete.
> >>> + *
> >>> + * - Pulse width of the pwm can never be out of bound.
> >>> + * It's taken care at HW and SW
> >>> + * - If the user input duty is below limit, then driver sets it to
> >>> + * minimum possible value.
> >>> + * - If anything else goes wrong for setting duty or period,
> >>> + * -EINVAL is returned.
> >>> */
> >>>
> >>> #include <linux/clk.h>
> >>> @@ -41,6 +72,7 @@ struct tegra_pwm_chip {
> >>> struct reset_control*rst;
> >>>
> >>> unsigned long clk_rate;
> >>> + unsigned long min_period_ns;
> >>>
> >>> void __iomem *regs;
> >>>
> >>> @@ -67,8 +99,9 @@ static int tegra_pwm_config(struct pwm_chip *chip,
> >> struct pwm_device *pwm,
> >>> int duty_ns, int period_ns)
> >>> {
> >>> struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> >>> - unsigned long long c = duty_ns, hz;
> >>> - unsigned long rate;
> >>> + unsigned long long p_width = duty_ns, period_hz;
> >>> + unsigned long rate, required_clk_rate;
> >>> + unsigned long pfm; /* Frequency divider */
> >>
> >> If it is not necessary to change the variable names, then I would
> >> prefer we keep them as is as then changes would be less.
> >
> > The earlier name was misleading so thought to use a specific name for
> > which it can be helpful to follow up with the TRM. Since its
> > recommended to retain the variable names, I will change this in next patch.
>
> I was just wondering if was necessary to change 'c' to 'p_width'. This could
> reduce the diff a bit.
Yes, noted to revert back both the variables name change.
>
> >>
> >>> u32 val = 0;
> >>> int err;
> >>>
> >>> @@ -77,37 +110,77 @@ static int tegra_pwm_config(struct pwm_chip
> >>> *chip,
> >> struct pwm_device *pwm,
> >>> * per (1 << PWM_DUTY_WIDTH) cycles and make sure to round to the
> >>> * nearest integer during division.
> >>> */
> >>> - c *= (1 << PWM_DUTY_WIDTH);
> >>> - c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
> >>> + p_width *= (1 << PWM_DUTY_WIDTH);
> >>> + p_width = DIV_ROUND_CLOSEST_ULL(p_width, period_ns);
> >>>
> >>> - val = (u32)c << PWM_DUTY_SHIFT;
> >>> + val = (u32)p_width << PWM_DUTY_SHIFT;
> >>> +
> >>> + /*
> >>> + * Period in nano second has to be <= highest allowed period
> >>> + * based on max clock rate of the pwm controller.
> >>> + *
> >>> + * higher limit = max clock limit >> PWM_DUTY_WIDTH
> >>> + * lower limit = min clock limit >> PWM_DUTY_WIDTH >>
> >> PWM_SCALE_WIDTH
> >>> + */
> >>> + if (period_ns < pc->min_period_ns) {
> >>> + period_ns = pc->min_period_ns;
> >>> + pr_warn("Period is adjusted to allowed value (%d ns)\n",
> >>> + period_ns);
> >>
> >> I see that other drivers (pwm-img.c) consider this to be an error and
> >> return an error. I wonder if adjusting the period makes sense here?
> >>
> >> I wonder if the handling of the min_period, should be a separate change?
> >
> > I think I misunderstood one of the discussions in initial patch and
> > added this change to apply the minimum possible value. Understood and
> > will revert this change with returning error in such case.
> >
> >>
> >>> + }
> >>>
> >>> /*
> >>> * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
> >>> * cycles at the PWM clock rate will take period_ns nanoseconds.
> >>> */
> >>> - rate = pc->clk_rate >> PWM_DUTY_WIDTH;
> >>> + if (pc->soc->num_channels == 1) {
> >>
> >> Are you using num_channels to determine if Tegra uses the BPMP? If so
> >> then the above is not really correct, because num_channels is not
> >> really related to what is being done here. So maybe you need a new SoC
> attribute in the soc data.
> >
> > Here, it tries to find if pwm controller uses multiple channels (like
> > in Tegra210 or older) or single channel for every pwm instance (i.e.
> > T186, T194). If found multiple channels on a single controller then it
> > is not correct to configure separate clock rates to each of the channels. So to
> distinguish the controller and channel type, num_channels is referred.
>
> OK, then that makes sense. Maybe add this detail to the comment about why
> num_channels is used.
Ok. Will update comment.
>
> >>
> >>> + /*
> >>> + * Rate is multiplied with 2^PWM_DUTY_WIDTH so that it
> >> matches
> >>> + * with the hieghest applicable rate that the controller can
> >>
> >> s/hieghest/highest/
> >
> > Got it.
> >
> >>
> >>> + * provide. Any further lower value can be derived by setting
> >>> + * PFM bits[0:12].
> >>> + * Higher mark is taken since BPMP has round-up mechanism
> >>> + * implemented.
> >>> + */
> >>> + required_clk_rate =
> >>> + (NSEC_PER_SEC / period_ns) << PWM_DUTY_WIDTH;
> >>> +
> >>
> >> Should be we checking the rate against the max rate supported?
> >
> > If the request rate is beyond max supported rate, then the
> > clk_set_rate will be failing and can get caught with error check
> > followed by. Otherwise it will fail through fitting in the register's frequency
> divider filed. So I think it is not required to check against max rate.
> > Please advise if I am not able to follow with what you are suggesting.
>
> I think that it would be better to update the cached value so that it is not
> incorrectly used else where by any future change. Furthermore, this simplifies
> matters a bit because you can do the following for all devices, but only update
> the clk_rate for those you wish to ...
>
> rate = pc->clk_rate >> PWM_DUTY_WIDTH;
>
What I understood from above is, we will always use max rate for any further configurations.
If this is the suggestion above, then I think its not the right way.
If we consider only max rate then the pwm output can only be ranging from:
Possible max output rate: rate
Possible min output rate: rate/2^13 (13 bits frequency divisor)
But if we consider the min rate supported by the source clock then,
min output rate can go beyond the current min possible and
that should be considered for finding actual limit of min output rate.
Based on this, in the driver it tries to find a suitable clock rate to achieve
requested output rate.
Please suggest if you think we can still improve this further.
> >>
> >>> + err = clk_set_rate(pc->clk, required_clk_rate);
> >>> + if (err < 0)
> >>> + return -EINVAL;
> >>> +
> >>> + rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
> >>
> >> Do we need to update the pwm->clk_rate here?
> >
> > This return rate is basically from the factor that requested
> > clk_set_rate and the actual rate set mostly will have a little
> > deviation based on the clock divider and other factors while setting a new
> rate. So capturing the actual rate for further calculation and conversion to Hz.
> > Whenever it is required to use pwm->clk_rate we are no longer
> > depending upon the cached value for num_channels == 1. So in my
> > opinion it does not need to be cached. However it is kept stored for the SoCs
> having num_channels > 1.
> > Please suggest if I am missing any case where we need to keep the value
> stored.
>
> OK sounds fine.
>
> >>
> >>> + } else {
> >>> + /*
> >>> + * This is the case for SoCs who support multiple channels:
> >>> + *
> >>> + * clk_set_rate() can not be called again in config because
> >>> + * T210 or any prior chip supports one pwm-controller and
> >>> + * multiple channels. Hence in this case cached clock rate
> >>> + * will be considered which was stored during probe.
> >>> + */
> >>> + rate = pc->clk_rate >> PWM_DUTY_WIDTH;
> >>> + }
> >>>
> >>> /* Consider precision in PWM_SCALE_WIDTH rate calculation */
> >>> - hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
> >>> - rate = DIV_ROUND_CLOSEST_ULL(100ULL * rate, hz);
> >>> + period_hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC,
> >> period_ns);
> >>> + pfm = DIV_ROUND_CLOSEST_ULL(100ULL * rate, period_hz);
> >>>
> >>> /*
> >>> * Since the actual PWM divider is the register's frequency divider
> >>> - * field minus 1, we need to decrement to get the correct value to
> >>> + * field plus 1, we need to decrement to get the correct value to
> >>> * write to the register.
> >>> */
> >>> - if (rate > 0)
> >>> - rate--;
> >>> + if (pfm > 0)
> >>> + pfm--;
> >>>
> >>> /*
> >>> - * Make sure that the rate will fit in the register's frequency
> >>> + * Make sure that pfm will fit in the register's frequency
> >>> * divider field.
> >>> */
> >>> - if (rate >> PWM_SCALE_WIDTH)
> >>> + if (pfm >> PWM_SCALE_WIDTH)
> >>> return -EINVAL;
> >>>
> >>> - val |= rate << PWM_SCALE_SHIFT;
> >>> + val |= pfm << PWM_SCALE_SHIFT;
> >>>
> >>> /*
> >>> * If the PWM channel is disabled, make sure to turn on the clock
> >>> @@
> >>> -205,6 +278,10 @@ static int tegra_pwm_probe(struct platform_device
> >> *pdev)
> >>> */
> >>> pwm->clk_rate = clk_get_rate(pwm->clk);
> >>>
> >>> + /* Set minimum limit of PWM period for the IP */
> >>> + pwm->min_period_ns =
> >>> + (NSEC_PER_SEC / (pwm->soc->max_frequency >>
> >> PWM_DUTY_WIDTH)) +
> >>> +1;
> >>> +
> >>> pwm->rst = devm_reset_control_get_exclusive(&pdev->dev, "pwm");
> >>> if (IS_ERR(pwm->rst)) {
> >>> ret = PTR_ERR(pwm->rst);
> >>> @@ -313,4 +390,5 @@ module_platform_driver(tegra_pwm_driver);
> >>>
> >>> MODULE_LICENSE("GPL");
> >>> MODULE_AUTHOR("NVIDIA Corporation");
> >>> +MODULE_AUTHOR("Sandipan Patra <spatra@nvidia.com>");
> >>> MODULE_ALIAS("platform:tegra-pwm");
> >>>
> >>
> >> --
> >> nvpublic
>
> --
> nvpublic
^ permalink raw reply
* Re: [PATCH v4 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
From: Andy Shevchenko @ 2020-05-22 12:12 UTC (permalink / raw)
To: Serge Semin
Cc: Serge Semin, Mark Brown, Linus Walleij, Vinod Koul, Feng Tang,
Grant Likely, Alan Cox, Georgy Vlasov, Ramil Zaripov,
Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
Arnd Bergmann, Rob Herring, linux-mips, devicetree,
Wan Ahmad Zainie, Thomas Gleixner, Jarkko Nikula, wuxu.wu,
Clement Leger, Linus Walleij, linux-spi, linux-kernel
In-Reply-To: <20200522115235.rt3ay7lveimrgooa@mobilestation>
On Fri, May 22, 2020 at 02:52:35PM +0300, Serge Semin wrote:
> On Fri, May 22, 2020 at 02:13:40PM +0300, Andy Shevchenko wrote:
> > On Fri, May 22, 2020 at 03:07:50AM +0300, Serge Semin wrote:
> > > Since DMA transfers are performed asynchronously with actual SPI
> > > transaction, then even if DMA transfers are finished it doesn't mean
> > > all data is actually pushed to the SPI bus. Some data might still be
> > > in the controller FIFO. This is specifically true for Tx-only
> > > transfers. In this case if the next SPI transfer is recharged while
> > > a tail of the previous one is still in FIFO, we'll loose that tail
> > > data. In order to fix this lets add the wait procedure of the Tx/Rx
> > > SPI transfers completion after the corresponding DMA transactions
> > > are finished.
...
> > > Changelog v4:
> > > - Get back ndelay() method to wait for an SPI transfer completion.
> > > spi_delay_exec() isn't suitable for the atomic context.
> >
> > OTOH we may teach spi_delay_exec() to perform atomic sleeps.
>
> Please, see it's implementation. It does atomic delay when the delay value
> is less than 10us. But selectively gets to the usleep_range() if value is
> greater than that.
Oh, than it means we may do a very long busy loop here which is not good at
all. If we have 10Hz clock, it might take seconds of doing nothing!
...
> > > + while (dw_spi_dma_tx_busy(dws) && retry--)
> > > + ndelay(ns);
> >
> > I might be mistaken, but I think I told that this one misses to keep power
> > management in mind.
>
> Here we already in nearly atomic context due to the callback executed in the
> tasklet. What power management could be during a tasklet execution? Again we
> can't call sleeping methods in here. What do you suggest in substitution?
>
> > Have you read Documentation/process/volatile-considered-harmful.rst ?
>
> That's mentoring tone is redundant. Please, stop it.
I simple gave you pointers to where you may read about power management in busy
loops. Yes, I admit that documentation title and the relation to busy loops is
not obvious.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
From: Mark Brown @ 2020-05-22 12:10 UTC (permalink / raw)
To: Serge Semin
Cc: Andy Shevchenko, Serge Semin, Linus Walleij, Vinod Koul,
Feng Tang, Grant Likely, Alan Cox, Georgy Vlasov, Ramil Zaripov,
Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
Arnd Bergmann, Rob Herring, linux-mips, devicetree,
Wan Ahmad Zainie, Thomas Gleixner, Jarkko Nikula, wuxu.wu,
Clement Leger, Linus Walleij, linux-spi, linux-kernel
In-Reply-To: <20200522115235.rt3ay7lveimrgooa@mobilestation>
[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]
On Fri, May 22, 2020 at 02:52:35PM +0300, Serge Semin wrote:
> On Fri, May 22, 2020 at 02:13:40PM +0300, Andy Shevchenko wrote:
> > > Changelog v4:
> > > - Get back ndelay() method to wait for an SPI transfer completion.
> > > spi_delay_exec() isn't suitable for the atomic context.
> > OTOH we may teach spi_delay_exec() to perform atomic sleeps.
> Please, see it's implementation. It does atomic delay when the delay value
> is less than 10us. But selectively gets to the usleep_range() if value is
> greater than that.
Yes, I hadn't realised this was in atomic context - _delay_exec() is
just not safe to use there, it'll swich to a sleeping delay if the time
is long enough.
> > > + while (dw_spi_dma_tx_busy(dws) && retry--)
> > > + ndelay(ns);
> > I might be mistaken, but I think I told that this one misses to keep power
> > management in mind.
> Here we already in nearly atomic context due to the callback executed in the
> tasklet. What power management could be during a tasklet execution? Again we
> can't call sleeping methods in here. What do you suggest in substitution?
You'd typically have a cpu_relax() in there as well as the ndelay().
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH v5 01/11] dt-bindings: convert the binding document for mediatek PERICFG to yaml
From: Bartosz Golaszewski @ 2020-05-22 12:06 UTC (permalink / raw)
To: Rob Herring, David S . Miller, Matthias Brugger, John Crispin,
Sean Wang, Mark Lee, Jakub Kicinski, Arnd Bergmann, Fabien Parent,
Heiner Kallweit, Edwin Peer
Cc: devicetree, linux-kernel, netdev, linux-arm-kernel,
linux-mediatek, Stephane Le Provost, Pedro Tsai, Andrew Perepech,
Bartosz Golaszewski
In-Reply-To: <20200522120700.838-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Convert the DT binding .txt file for MediaTek's peripheral configuration
controller to YAML. There's one special case where the compatible has
three positions. Otherwise, it's a pretty normal syscon.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
.../arm/mediatek/mediatek,pericfg.txt | 36 -----------
.../arm/mediatek/mediatek,pericfg.yaml | 63 +++++++++++++++++++
2 files changed, 63 insertions(+), 36 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,pericfg.txt
create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,pericfg.yaml
diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,pericfg.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,pericfg.txt
deleted file mode 100644
index ecf027a9003a..000000000000
--- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,pericfg.txt
+++ /dev/null
@@ -1,36 +0,0 @@
-Mediatek pericfg controller
-===========================
-
-The Mediatek pericfg controller provides various clocks and reset
-outputs to the system.
-
-Required Properties:
-
-- compatible: Should be one of:
- - "mediatek,mt2701-pericfg", "syscon"
- - "mediatek,mt2712-pericfg", "syscon"
- - "mediatek,mt7622-pericfg", "syscon"
- - "mediatek,mt7623-pericfg", "mediatek,mt2701-pericfg", "syscon"
- - "mediatek,mt7629-pericfg", "syscon"
- - "mediatek,mt8135-pericfg", "syscon"
- - "mediatek,mt8173-pericfg", "syscon"
- - "mediatek,mt8183-pericfg", "syscon"
-- #clock-cells: Must be 1
-- #reset-cells: Must be 1
-
-The pericfg controller uses the common clk binding from
-Documentation/devicetree/bindings/clock/clock-bindings.txt
-The available clocks are defined in dt-bindings/clock/mt*-clk.h.
-Also it uses the common reset controller binding from
-Documentation/devicetree/bindings/reset/reset.txt.
-The available reset outputs are defined in
-dt-bindings/reset/mt*-resets.h
-
-Example:
-
-pericfg: power-controller@10003000 {
- compatible = "mediatek,mt8173-pericfg", "syscon";
- reg = <0 0x10003000 0 0x1000>;
- #clock-cells = <1>;
- #reset-cells = <1>;
-};
diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,pericfg.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,pericfg.yaml
new file mode 100644
index 000000000000..1340c6288024
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,pericfg.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/arm/mediatek/mediatek,pericfg.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: MediaTek Peripheral Configuration Controller
+
+maintainers:
+ - Bartosz Golaszewski <bgolaszewski@baylibre.com>
+
+description:
+ The Mediatek pericfg controller provides various clocks and reset outputs
+ to the system.
+
+properties:
+ compatible:
+ oneOf:
+ - items:
+ - enum:
+ - mediatek,mt2701-pericfg
+ - mediatek,mt2712-pericfg
+ - mediatek,mt7622-pericfg
+ - mediatek,mt7629-pericfg
+ - mediatek,mt8135-pericfg
+ - mediatek,mt8173-pericfg
+ - mediatek,mt8183-pericfg
+ - const: syscon
+ - items:
+ # Special case for mt7623 for backward compatibility
+ - const: mediatek,mt7623-pericfg
+ - const: mediatek,mt2701-pericfg
+ - const: syscon
+
+ reg:
+ maxItems: 1
+
+ '#clock-cells':
+ const: 1
+
+ '#reset-cells':
+ const: 1
+
+required:
+ - compatible
+ - reg
+
+examples:
+ - |
+ pericfg@10003000 {
+ compatible = "mediatek,mt8173-pericfg", "syscon";
+ reg = <0x10003000 0x1000>;
+ #clock-cells = <1>;
+ #reset-cells = <1>;
+ };
+
+ - |
+ pericfg@10003000 {
+ compatible = "mediatek,mt7623-pericfg", "mediatek,mt2701-pericfg", "syscon";
+ reg = <0x10003000 0x1000>;
+ #clock-cells = <1>;
+ #reset-cells = <1>;
+ };
--
2.25.0
^ permalink raw reply related
* [PATCH v5 02/11] dt-bindings: add new compatible to mediatek,pericfg
From: Bartosz Golaszewski @ 2020-05-22 12:06 UTC (permalink / raw)
To: Rob Herring, David S . Miller, Matthias Brugger, John Crispin,
Sean Wang, Mark Lee, Jakub Kicinski, Arnd Bergmann, Fabien Parent,
Heiner Kallweit, Edwin Peer
Cc: devicetree, linux-kernel, netdev, linux-arm-kernel,
linux-mediatek, Stephane Le Provost, Pedro Tsai, Andrew Perepech,
Bartosz Golaszewski
In-Reply-To: <20200522120700.838-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
The PERICFG controller is present on the MT8516 SoC. Add an appropriate
compatible variant.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
.../devicetree/bindings/arm/mediatek/mediatek,pericfg.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,pericfg.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,pericfg.yaml
index 1340c6288024..55209a2baedc 100644
--- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,pericfg.yaml
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,pericfg.yaml
@@ -25,6 +25,7 @@ properties:
- mediatek,mt8135-pericfg
- mediatek,mt8173-pericfg
- mediatek,mt8183-pericfg
+ - mediatek,mt8516-pericfg
- const: syscon
- items:
# Special case for mt7623 for backward compatibility
--
2.25.0
^ permalink raw reply related
* [PATCH v5 03/11] dt-bindings: net: add a binding document for MediaTek STAR Ethernet MAC
From: Bartosz Golaszewski @ 2020-05-22 12:06 UTC (permalink / raw)
To: Rob Herring, David S . Miller, Matthias Brugger, John Crispin,
Sean Wang, Mark Lee, Jakub Kicinski, Arnd Bergmann, Fabien Parent,
Heiner Kallweit, Edwin Peer
Cc: devicetree, linux-kernel, netdev, linux-arm-kernel,
linux-mediatek, Stephane Le Provost, Pedro Tsai, Andrew Perepech,
Bartosz Golaszewski
In-Reply-To: <20200522120700.838-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
This adds yaml DT bindings for the MediaTek STAR Ethernet MAC present
on the mt8* family of SoCs.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
.../bindings/net/mediatek,eth-mac.yaml | 89 +++++++++++++++++++
1 file changed, 89 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/mediatek,eth-mac.yaml
diff --git a/Documentation/devicetree/bindings/net/mediatek,eth-mac.yaml b/Documentation/devicetree/bindings/net/mediatek,eth-mac.yaml
new file mode 100644
index 000000000000..f85d91a9d6e5
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mediatek,eth-mac.yaml
@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/mediatek,eth-mac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek STAR Ethernet MAC Controller
+
+maintainers:
+ - Bartosz Golaszewski <bgolaszewski@baylibre.com>
+
+description:
+ This Ethernet MAC is used on the MT8* family of SoCs from MediaTek.
+ It's compliant with 802.3 standards and supports half- and full-duplex
+ modes with flow-control as well as CRC offloading and VLAN tags.
+
+allOf:
+ - $ref: "ethernet-controller.yaml#"
+
+properties:
+ compatible:
+ enum:
+ - mediatek,mt8516-eth
+ - mediatek,mt8518-eth
+ - mediatek,mt8175-eth
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ minItems: 3
+ maxItems: 3
+
+ clock-names:
+ additionalItems: false
+ items:
+ - const: core
+ - const: reg
+ - const: trans
+
+ mediatek,pericfg:
+ $ref: /schemas/types.yaml#definitions/phandle
+ description:
+ Phandle to the device containing the PERICFG register range. This is used
+ to control the MII mode.
+
+ mdio:
+ type: object
+ description:
+ Creates and registers an MDIO bus.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+ - mediatek,pericfg
+ - phy-handle
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/clock/mt8516-clk.h>
+
+ ethernet: ethernet@11180000 {
+ compatible = "mediatek,mt8516-eth";
+ reg = <0x11180000 0x1000>;
+ mediatek,pericfg = <&pericfg>;
+ interrupts = <GIC_SPI 111 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&topckgen CLK_TOP_RG_ETH>,
+ <&topckgen CLK_TOP_66M_ETH>,
+ <&topckgen CLK_TOP_133M_ETH>;
+ clock-names = "core", "reg", "trans";
+ phy-handle = <ð_phy>;
+ phy-mode = "rmii";
+
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ eth_phy: ethernet-phy@0 {
+ reg = <0>;
+ };
+ };
+ };
--
2.25.0
^ 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