* Re: [PATCH] [RFC] genirq: Check irq_data_get_irq_chip() return value before use
From: Alexandre Torgue @ 2020-05-14 12:16 UTC (permalink / raw)
To: Marek Vasut, Thomas Gleixner, linux-arm-kernel
Cc: Marc Zyngier, Linus Walleij, Stephen Boyd, Fabien Dessenne
In-Reply-To: <d3b76d43-675b-c699-06fa-aab448504f25@denx.de>
Hi Marek
On 5/10/20 4:49 PM, Marek Vasut wrote:
> On 5/7/20 11:51 PM, Thomas Gleixner wrote:
>> Marek,
>>
>> Marek Vasut <marex@denx.de> writes:
>>> On 5/7/20 7:30 PM, Thomas Gleixner wrote:
>>>> Marek Vasut <marex@denx.de> writes:
>>>>> NOTE: I don't know whether this is a correct fix. Maybe the
>>>>> irq_data_get_irq_chip() should never return NULL, and
>>>>> I have some other issue?
>>>>
>>>> What's the callchain?
>>>
>>> Hmm, I'm currently unable to replicate it on linux-next, but on 5.4.39 I
>>> get what's at the end of the email.
>>>
>>> On next I just noticed I get i2c: Transfer while suspended, which is
>>> what I suspect would be the real root cause of my problem, and why
>>> irq_data_get_irq_chip() returns NULL?
>>
>> Looks like.
>>
>>> Unable to handle kernel NULL pointer dereference at virtual address 00000070
>>> PC is at __irq_get_irqchip_state+0x4/0x30
>>> LR is at __synchronize_hardirq+0x7c/0xe8
>>> [<c0166758>] (__irq_get_irqchip_state) from [<c0166800>] (__synchronize_hardirq+0x7c/0xe8)
>>> [<c0166800>] (__synchronize_hardirq) from [<c01668f0>] (synchronize_irq+0x2c/0x9c)
>>> [<c01668f0>] (synchronize_irq) from [<c016cba0>] (suspend_device_irqs+0xd8/0xf4)
>>> [<c016cba0>] (suspend_device_irqs) from [<c04e3388>] (dpm_suspend_noirq+0x18/0x194)
>>> [<c04e3388>] (dpm_suspend_noirq) from [<c015f174>] (suspend_devices_and_enter+0x170/0x514)
>>> [<c015f174>] (suspend_devices_and_enter) from [<c015f6f4>] (pm_suspend+0x1dc/0x278)
>>> [<c015f6f4>] (pm_suspend) from [<c015e100>] (state_store+0x9c/0xcc)
>>> [<c015e100>] (state_store) from [<c029a424>] (kernfs_fop_write+0x124/0x1e0)
>>> [<c029a424>] (kernfs_fop_write) from [<c02301c8>] (__vfs_write+0x2c/0xe8)
>>> [<c02301c8>] (__vfs_write) from [<c0231658>] (vfs_write+0x98/0xbc)
>>> [<c0231658>] (vfs_write) from [<c02317fc>] (ksys_write+0x74/0xc4)
>>> [<c02317fc>] (ksys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x54)
>>
>> I assume that the i2c controller in question tears down the interrupt on
>> suspend. The changelog of that i2c driver should give you a few hints.
>
> All right, so I found out the root cause is already fixed in next, and
> just needs to be backported to stable. I'll ping the patch author about
> that.
>
> It's this patch:
> 69269446ccbf ("mailbox: stm32-ipcc: Update wakeup management")
Just to inform you that I got same issue some times ago. As far I
understood, issue was related to the fact that we used
"dedicated_wakeup" API for Exti interrupt. Those Exti interrupts had no
parent which seems to generate this issue (sorry for the lake of details).
Adding a check is a good thing, but note that I have patches in my
backlog which change a bit hw abstraction done in Exti irqchip driver.
To better fit with HW, each Exti wakeup interrupt will have a GIC irq
parent (and issue is no more observed).
regards
Alex
> I also need to revisit the regulator suspend topic next, that seems to
> be a separate issue after all.
>
> Sorry for the noise.
>
> That said, do you want to take this patch to add the missing check
> anyway or is there a reason the check is missing ?
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 0/2] irqchip/gic-v3-its: Balance LPI affinity across CPUs
From: John Garry @ 2020-05-14 12:05 UTC (permalink / raw)
To: Marc Zyngier, linux-kernel, linux-arm-kernel
Cc: Jason Cooper, chenxiang, Will Deacon, luojiaxing@huawei.com,
Ming Lei, Zhou Wang, Thomas Gleixner, Robin Murphy
In-Reply-To: <f2971d1c-50f8-bf5a-8b16-8d84a631b0ba@huawei.com>
>
> + its_inc_lpi_count(d, cpu);
> +
> return IRQ_SET_MASK_OK_DONE;
> }
>
> Results look ok:
> nvme.use_threaded_interrupts=1 =0*
> Before 950K IOPs 1000K IOPs
> After 1100K IOPs 1150K IOPs
>
> * as mentioned before, this is quite unstable and causes lockups. JFYI,
> there was an attempt to fix this:
>
> https://lore.kernel.org/linux-nvme/20191209175622.1964-1-kbusch@kernel.org/
>
Hi Marc,
Just wondering if we can try to get this series over the line?
So I tested the patches on v5.7-rc5, and get similar performance
improvement to above.
I did apply a couple of patches, below, to remedy the issues I
experienced for my D06CS.
Thanks,
John
---->8
[PATCH 1/2] irqchip/gic-v3-its: Don't double account for target CPU
assignment
In its_set_affinity(), when a managed irq is already assigned to a CPU,
we may needlessly reassign the irq to another CPU.
This is because when selecting the target CPU, being the least loaded
CPU in the mask, we account of that irq still being assigned to a CPU;
thereby we may unfairly select another CPU.
Modify this behaviour to pre-decrement the current target CPU LPI count
when finding the least loaded CPU.
Alternatively we may be able to just bail out early when the current
target CPU already falls within the requested mask.
---
drivers/irqchip/irq-gic-v3-its.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 73f5c12..2b18feb 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1636,6 +1636,8 @@ static int its_set_affinity(struct irq_data *d,
const struct cpumask *mask_val,
if (irqd_is_forwarded_to_vcpu(d))
return -EINVAL;
+ its_dec_lpi_count(d, its_dev->event_map.col_map[id]);
+
if (!force)
cpu = its_select_cpu(d, mask_val);
else
@@ -1646,14 +1648,14 @@ static int its_set_affinity(struct irq_data *d,
const struct cpumask *mask_val,
/* don't set the affinity when the target cpu is same as current one */
if (cpu != its_dev->event_map.col_map[id]) {
- its_inc_lpi_count(d, cpu);
- its_dec_lpi_count(d, its_dev->event_map.col_map[id]);
target_col = &its_dev->its->collections[cpu];
its_send_movi(its_dev, target_col, id);
its_dev->event_map.col_map[id] = cpu;
irq_data_update_effective_affinity(d, cpumask_of(cpu));
}
+ its_inc_lpi_count(d, cpu);
+
return IRQ_SET_MASK_OK_DONE;
}
---
[PATCH 2/2] irqchip/gic-v3-its: Handle no overlap of non-managed irq
affinity mask
In selecting the target CPU for a non-managed interrupt, we may select a
target CPU outside the requested affinity mask.
This is because there may be no overlap of the ITS node mask and the
requested CPU affinity mask. The requested affinity mask may be coming
from userspace or some drivers which try to set irq affinity, see [0].
In this case, just ignore the ITS node cpumask. This is a deviation from
what Thomas described. Having said that, I am not sure if the interrupt
is ever bound to a node for us.
[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/hisilicon/hisi_uncore_pmu.c#n417
---
drivers/irqchip/irq-gic-v3-its.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 2b18feb..12d5d4b4 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1584,10 +1584,6 @@ static int its_select_cpu(struct irq_data *d,
cpumask_and(tmpmask, cpumask_of_node(node), aff_mask);
cpumask_and(tmpmask, tmpmask, cpu_online_mask);
- /* If that doesn't work, try the nodemask itself */
- if (cpumask_empty(tmpmask))
- cpumask_and(tmpmask, cpumask_of_node(node), cpu_online_mask);
-
cpu = cpumask_pick_least_loaded(d, tmpmask);
if (cpu < nr_cpu_ids)
goto out;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v4 2/2] PCI: uniphier: Add Socionext UniPhier Pro5 PCIe endpoint controller driver
From: Kunihiko Hayashi @ 2020-05-14 12:03 UTC (permalink / raw)
To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Masahiro Yamada
Cc: devicetree, Kunihiko Hayashi, Masami Hiramatsu, linux-pci,
linux-kernel, Jassi Brar, linux-arm-kernel
In-Reply-To: <1589457801-12796-1-git-send-email-hayashi.kunihiko@socionext.com>
Add driver for the Socionext UniPhier Pro5 SoC endpoint controller.
This controller is based on the DesignWare PCIe core.
And add "host" to existing controller descriontions for the host controller
in Kconfig.
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
MAINTAINERS | 2 +-
drivers/pci/controller/dwc/Kconfig | 13 +-
drivers/pci/controller/dwc/Makefile | 1 +
drivers/pci/controller/dwc/pcie-uniphier-ep.c | 383 ++++++++++++++++++++++++++
4 files changed, 396 insertions(+), 3 deletions(-)
create mode 100644 drivers/pci/controller/dwc/pcie-uniphier-ep.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7f26748..b5ef0e7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13212,7 +13212,7 @@ M: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
L: linux-pci@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/pci/uniphier-pcie*
-F: drivers/pci/controller/dwc/pcie-uniphier.c
+F: drivers/pci/controller/dwc/pcie-uniphier*
PCIE DRIVER FOR ST SPEAR13XX
M: Pratyush Anand <pratyush.anand@gmail.com>
diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index ce13fd6..43a29f7 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -280,15 +280,24 @@ config PCIE_TEGRA194_EP
selected. This uses the DesignWare core.
config PCIE_UNIPHIER
- bool "Socionext UniPhier PCIe controllers"
+ bool "Socionext UniPhier PCIe host controllers"
depends on ARCH_UNIPHIER || COMPILE_TEST
depends on OF && HAS_IOMEM
depends on PCI_MSI_IRQ_DOMAIN
select PCIE_DW_HOST
help
- Say Y here if you want PCIe controller support on UniPhier SoCs.
+ Say Y here if you want PCIe host controller support on UniPhier SoCs.
This driver supports LD20 and PXs3 SoCs.
+config PCIE_UNIPHIER_EP
+ bool "Socionext UniPhier PCIe endpoint controllers"
+ depends on ARCH_UNIPHIER || COMPILE_TEST
+ depends on OF && HAS_IOMEM
+ select PCIE_DW_EP
+ help
+ Say Y here if you want PCIe endpoint controller support on
+ UniPhier SoCs. This driver supports Pro5 SoC.
+
config PCIE_AL
bool "Amazon Annapurna Labs PCIe controller"
depends on OF && (ARM64 || COMPILE_TEST)
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index 8a637cf..a751553 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
obj-$(CONFIG_PCI_MESON) += pci-meson.o
obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
+obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
# The following drivers are for devices that use the generic ACPI
# pci_root.c driver but don't support standard ECAM config access.
diff --git a/drivers/pci/controller/dwc/pcie-uniphier-ep.c b/drivers/pci/controller/dwc/pcie-uniphier-ep.c
new file mode 100644
index 0000000..0f36aa3
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-uniphier-ep.c
@@ -0,0 +1,383 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe endpoint controller driver for UniPhier SoCs
+ * Copyright 2018 Socionext Inc.
+ * Author: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/of_device.h>
+#include <linux/pci.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+
+#include "pcie-designware.h"
+
+/* Link Glue registers */
+#define PCL_RSTCTRL0 0x0010
+#define PCL_RSTCTRL_AXI_REG BIT(3)
+#define PCL_RSTCTRL_AXI_SLAVE BIT(2)
+#define PCL_RSTCTRL_AXI_MASTER BIT(1)
+#define PCL_RSTCTRL_PIPE3 BIT(0)
+
+#define PCL_RSTCTRL1 0x0020
+#define PCL_RSTCTRL_PERST BIT(0)
+
+#define PCL_RSTCTRL2 0x0024
+#define PCL_RSTCTRL_PHY_RESET BIT(0)
+
+#define PCL_MODE 0x8000
+#define PCL_MODE_REGEN BIT(8)
+#define PCL_MODE_REGVAL BIT(0)
+
+#define PCL_APP_CLK_CTRL 0x8004
+#define PCL_APP_CLK_REQ BIT(0)
+
+#define PCL_APP_READY_CTRL 0x8008
+#define PCL_APP_LTSSM_ENABLE BIT(0)
+
+#define PCL_APP_MSI0 0x8040
+#define PCL_APP_VEN_MSI_TC_MASK GENMASK(10, 8)
+#define PCL_APP_VEN_MSI_VECTOR_MASK GENMASK(4, 0)
+
+#define PCL_APP_MSI1 0x8044
+#define PCL_APP_MSI_REQ BIT(0)
+
+#define PCL_APP_INTX 0x8074
+#define PCL_APP_INTX_SYS_INT BIT(0)
+
+/* assertion time of INTx in usec */
+#define PCL_INTX_WIDTH_USEC 30
+
+struct uniphier_pcie_ep_priv {
+ void __iomem *base;
+ struct dw_pcie pci;
+ struct clk *clk, *clk_gio;
+ struct reset_control *rst, *rst_gio;
+ struct phy *phy;
+ const struct pci_epc_features *features;
+};
+
+#define to_uniphier_pcie(x) dev_get_drvdata((x)->dev)
+
+static void uniphier_pcie_ltssm_enable(struct uniphier_pcie_ep_priv *priv,
+ bool enable)
+{
+ u32 val;
+
+ val = readl(priv->base + PCL_APP_READY_CTRL);
+ if (enable)
+ val |= PCL_APP_LTSSM_ENABLE;
+ else
+ val &= ~PCL_APP_LTSSM_ENABLE;
+ writel(val, priv->base + PCL_APP_READY_CTRL);
+}
+
+static void uniphier_pcie_phy_reset(struct uniphier_pcie_ep_priv *priv,
+ bool assert)
+{
+ u32 val;
+
+ val = readl(priv->base + PCL_RSTCTRL2);
+ if (assert)
+ val |= PCL_RSTCTRL_PHY_RESET;
+ else
+ val &= ~PCL_RSTCTRL_PHY_RESET;
+ writel(val, priv->base + PCL_RSTCTRL2);
+}
+
+static void uniphier_pcie_init_ep(struct uniphier_pcie_ep_priv *priv)
+{
+ u32 val;
+
+ /* set EP mode */
+ val = readl(priv->base + PCL_MODE);
+ val |= PCL_MODE_REGEN | PCL_MODE_REGVAL;
+ writel(val, priv->base + PCL_MODE);
+
+ /* clock request */
+ val = readl(priv->base + PCL_APP_CLK_CTRL);
+ val &= ~PCL_APP_CLK_REQ;
+ writel(val, priv->base + PCL_APP_CLK_CTRL);
+
+ /* deassert PIPE3 and AXI reset */
+ val = readl(priv->base + PCL_RSTCTRL0);
+ val |= PCL_RSTCTRL_AXI_REG | PCL_RSTCTRL_AXI_SLAVE
+ | PCL_RSTCTRL_AXI_MASTER | PCL_RSTCTRL_PIPE3;
+ writel(val, priv->base + PCL_RSTCTRL0);
+
+ uniphier_pcie_ltssm_enable(priv, false);
+
+ msleep(100);
+}
+
+static int uniphier_pcie_start_link(struct dw_pcie *pci)
+{
+ struct uniphier_pcie_ep_priv *priv = to_uniphier_pcie(pci);
+
+ uniphier_pcie_ltssm_enable(priv, true);
+
+ return 0;
+}
+
+static void uniphier_pcie_stop_link(struct dw_pcie *pci)
+{
+ struct uniphier_pcie_ep_priv *priv = to_uniphier_pcie(pci);
+
+ uniphier_pcie_ltssm_enable(priv, false);
+}
+
+static void uniphier_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+ enum pci_barno bar;
+
+ for (bar = BAR_0; bar <= BAR_5; bar++)
+ dw_pcie_ep_reset_bar(pci, bar);
+}
+
+static int uniphier_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+ struct uniphier_pcie_ep_priv *priv = to_uniphier_pcie(pci);
+ u32 val;
+
+ /*
+ * This makes pulse signal to send INTx to the RC, so this should
+ * be cleared as soon as possible. This sequence is covered with
+ * mutex in pci_epc_raise_irq().
+ */
+ /* assert INTx */
+ val = readl(priv->base + PCL_APP_INTX);
+ val |= PCL_APP_INTX_SYS_INT;
+ writel(val, priv->base + PCL_APP_INTX);
+
+ udelay(PCL_INTX_WIDTH_USEC);
+
+ /* deassert INTx */
+ val &= ~PCL_APP_INTX_SYS_INT;
+ writel(val, priv->base + PCL_APP_INTX);
+
+ return 0;
+}
+
+static int uniphier_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep,
+ u8 func_no, u16 interrupt_num)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+ struct uniphier_pcie_ep_priv *priv = to_uniphier_pcie(pci);
+ u32 val;
+
+ val = FIELD_PREP(PCL_APP_VEN_MSI_TC_MASK, func_no)
+ | FIELD_PREP(PCL_APP_VEN_MSI_VECTOR_MASK, interrupt_num - 1);
+ writel(val, priv->base + PCL_APP_MSI0);
+
+ val = readl(priv->base + PCL_APP_MSI1);
+ val |= PCL_APP_MSI_REQ;
+ writel(val, priv->base + PCL_APP_MSI1);
+
+ return 0;
+}
+
+static int uniphier_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
+ enum pci_epc_irq_type type,
+ u16 interrupt_num)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+
+ switch (type) {
+ case PCI_EPC_IRQ_LEGACY:
+ return uniphier_pcie_ep_raise_legacy_irq(ep);
+ case PCI_EPC_IRQ_MSI:
+ return uniphier_pcie_ep_raise_msi_irq(ep, func_no,
+ interrupt_num);
+ default:
+ dev_err(pci->dev, "UNKNOWN IRQ type (%d)\n", type);
+ }
+
+ return 0;
+}
+
+static const struct pci_epc_features*
+uniphier_pcie_get_features(struct dw_pcie_ep *ep)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+ struct uniphier_pcie_ep_priv *priv = to_uniphier_pcie(pci);
+
+ return priv->features;
+}
+
+static const struct dw_pcie_ep_ops uniphier_pcie_ep_ops = {
+ .ep_init = uniphier_pcie_ep_init,
+ .raise_irq = uniphier_pcie_ep_raise_irq,
+ .get_features = uniphier_pcie_get_features,
+};
+
+static int uniphier_add_pcie_ep(struct uniphier_pcie_ep_priv *priv,
+ struct platform_device *pdev)
+{
+ struct dw_pcie *pci = &priv->pci;
+ struct dw_pcie_ep *ep = &pci->ep;
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ int ret;
+
+ ep->ops = &uniphier_pcie_ep_ops;
+
+ pci->dbi_base2 = devm_platform_ioremap_resource_byname(pdev, "dbi2");
+ if (IS_ERR(pci->dbi_base2))
+ return PTR_ERR(pci->dbi_base2);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
+ if (!res)
+ return -EINVAL;
+
+ ep->phys_base = res->start;
+ ep->addr_size = resource_size(res);
+
+ ret = dw_pcie_ep_init(ep);
+ if (ret)
+ dev_err(dev, "Failed to initialize endpoint (%d)\n", ret);
+
+ return ret;
+}
+
+static int uniphier_pcie_ep_enable(struct uniphier_pcie_ep_priv *priv)
+{
+ int ret;
+
+ ret = clk_prepare_enable(priv->clk);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(priv->clk_gio);
+ if (ret)
+ goto out_clk_disable;
+
+ ret = reset_control_deassert(priv->rst);
+ if (ret)
+ goto out_clk_gio_disable;
+
+ ret = reset_control_deassert(priv->rst_gio);
+ if (ret)
+ goto out_rst_assert;
+
+ uniphier_pcie_init_ep(priv);
+
+ uniphier_pcie_phy_reset(priv, true);
+
+ ret = phy_init(priv->phy);
+ if (ret)
+ goto out_rst_gio_assert;
+
+ uniphier_pcie_phy_reset(priv, false);
+
+ return 0;
+
+out_rst_gio_assert:
+ reset_control_assert(priv->rst_gio);
+out_rst_assert:
+ reset_control_assert(priv->rst);
+out_clk_gio_disable:
+ clk_disable_unprepare(priv->clk_gio);
+out_clk_disable:
+ clk_disable_unprepare(priv->clk);
+
+ return ret;
+}
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+ .start_link = uniphier_pcie_start_link,
+ .stop_link = uniphier_pcie_stop_link,
+};
+
+static int uniphier_pcie_ep_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct uniphier_pcie_ep_priv *priv;
+ struct resource *res;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->features = of_device_get_match_data(dev);
+ if (WARN_ON(!priv->features))
+ return -EINVAL;
+
+ priv->pci.dev = dev;
+ priv->pci.ops = &dw_pcie_ops;
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
+ priv->pci.dbi_base = devm_pci_remap_cfg_resource(dev, res);
+ if (IS_ERR(priv->pci.dbi_base))
+ return PTR_ERR(priv->pci.dbi_base);
+
+ priv->base = devm_platform_ioremap_resource_byname(pdev, "link");
+ if (IS_ERR(priv->base))
+ return PTR_ERR(priv->base);
+
+ priv->clk_gio = devm_clk_get(dev, "gio");
+ if (IS_ERR(priv->clk))
+ return PTR_ERR(priv->clk);
+
+ priv->rst_gio = devm_reset_control_get_shared(dev, "gio");
+ if (IS_ERR(priv->rst_gio))
+ return PTR_ERR(priv->rst_gio);
+
+ priv->clk = devm_clk_get(dev, "link");
+ if (IS_ERR(priv->clk))
+ return PTR_ERR(priv->clk);
+
+ priv->rst = devm_reset_control_get_shared(dev, "link");
+ if (IS_ERR(priv->rst))
+ return PTR_ERR(priv->rst);
+
+ priv->phy = devm_phy_optional_get(dev, "pcie-phy");
+ if (IS_ERR(priv->phy)) {
+ ret = PTR_ERR(priv->phy);
+ dev_err(dev, "Failed to get phy (%d)\n", ret);
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, priv);
+
+ ret = uniphier_pcie_ep_enable(priv);
+ if (ret)
+ return ret;
+
+ return uniphier_add_pcie_ep(priv, pdev);
+}
+
+static const struct pci_epc_features uniphier_pro5_data = {
+ .linkup_notifier = false,
+ .msi_capable = true,
+ .msix_capable = false,
+ .align = 1 << 16,
+ .bar_fixed_64bit = BIT(BAR_0) | BIT(BAR_2) | BIT(BAR_4),
+ .reserved_bar = BIT(BAR_4),
+};
+
+static const struct of_device_id uniphier_pcie_ep_match[] = {
+ {
+ .compatible = "socionext,uniphier-pro5-pcie-ep",
+ .data = &uniphier_pro5_data,
+ },
+ { /* sentinel */ },
+};
+
+static struct platform_driver uniphier_pcie_ep_driver = {
+ .probe = uniphier_pcie_ep_probe,
+ .driver = {
+ .name = "uniphier-pcie-ep",
+ .of_match_table = uniphier_pcie_ep_match,
+ .suppress_bind_attrs = true,
+ },
+};
+builtin_platform_driver(uniphier_pcie_ep_driver);
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v4 1/2] dt-bindings: PCI: Add UniPhier PCIe endpoint controller description
From: Kunihiko Hayashi @ 2020-05-14 12:03 UTC (permalink / raw)
To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Masahiro Yamada
Cc: devicetree, Kunihiko Hayashi, Masami Hiramatsu, linux-pci,
linux-kernel, Jassi Brar, linux-arm-kernel
In-Reply-To: <1589457801-12796-1-git-send-email-hayashi.kunihiko@socionext.com>
Add DT bindings for PCIe controller implemented in UniPhier SoCs
when configured in endpoint mode. This controller is based on
the DesignWare PCIe core.
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
.../bindings/pci/socionext,uniphier-pcie-ep.yaml | 92 ++++++++++++++++++++++
MAINTAINERS | 2 +-
2 files changed, 93 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/pci/socionext,uniphier-pcie-ep.yaml
diff --git a/Documentation/devicetree/bindings/pci/socionext,uniphier-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/socionext,uniphier-pcie-ep.yaml
new file mode 100644
index 0000000..f0558b9
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/socionext,uniphier-pcie-ep.yaml
@@ -0,0 +1,92 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/socionext,uniphier-pcie-ep.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Socionext UniPhier PCIe endpoint controller
+
+description: |
+ UniPhier PCIe endpoint controller is based on the Synopsys DesignWare
+ PCI core. It shares common features with the PCIe DesignWare core and
+ inherits common properties defined in
+ Documentation/devicetree/bindings/pci/designware-pcie.txt.
+
+maintainers:
+ - Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
+
+allOf:
+ - $ref: "pci-ep.yaml#"
+
+properties:
+ compatible:
+ const: socionext,uniphier-pro5-pcie-ep
+
+ reg:
+ maxItems: 4
+
+ reg-names:
+ items:
+ - const: dbi
+ - const: dbi2
+ - const: link
+ - const: addr_space
+
+ clocks:
+ maxItems: 2
+
+ clock-names:
+ items:
+ - const: gio
+ - const: link
+
+ resets:
+ maxItems: 2
+
+ reset-names:
+ items:
+ - const: gio
+ - const: link
+
+ num-ib-windows:
+ const: 16
+
+ num-ob-windows:
+ const: 16
+
+ num-lanes: true
+
+ phys:
+ maxItems: 1
+
+ phy-names:
+ const: pcie-phy
+
+required:
+ - compatible
+ - reg
+ - reg-names
+ - clocks
+ - clock-names
+ - resets
+ - reset-names
+
+additionalProperties: false
+
+examples:
+ - |
+ pcie_ep: pcie-ep@66000000 {
+ compatible = "socionext,uniphier-pro5-pcie-ep";
+ reg-names = "dbi", "dbi2", "link", "addr_space";
+ reg = <0x66000000 0x1000>, <0x66001000 0x1000>,
+ <0x66010000 0x10000>, <0x67000000 0x400000>;
+ clock-names = "gio", "link";
+ clocks = <&sys_clk 12>, <&sys_clk 24>;
+ reset-names = "gio", "link";
+ resets = <&sys_rst 12>, <&sys_rst 24>;
+ num-ib-windows = <16>;
+ num-ob-windows = <16>;
+ num-lanes = <4>;
+ phy-names = "pcie-phy";
+ phys = <&pcie_phy>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 92657a1..7f26748 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13211,7 +13211,7 @@ PCIE DRIVER FOR SOCIONEXT UNIPHIER
M: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
L: linux-pci@vger.kernel.org
S: Maintained
-F: Documentation/devicetree/bindings/pci/uniphier-pcie.txt
+F: Documentation/devicetree/bindings/pci/uniphier-pcie*
F: drivers/pci/controller/dwc/pcie-uniphier.c
PCIE DRIVER FOR ST SPEAR13XX
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v4 0/2] PCI: Add new UniPhier PCIe endpoint driver
From: Kunihiko Hayashi @ 2020-05-14 12:03 UTC (permalink / raw)
To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Masahiro Yamada
Cc: devicetree, Kunihiko Hayashi, Masami Hiramatsu, linux-pci,
linux-kernel, Jassi Brar, linux-arm-kernel
This series adds PCIe endpoint controller driver for Socionext UniPhier
SoCs. This controller is based on the DesignWare PCIe core.
This driver supports Pro5 SoC only, so Pro5 needs multiple clocks and
resets in devicetree node.
Changes since v3:
- dt-bindings: Convert with dt-schema
- Replace with devm_platform_ioremap_resource()
- Add a commnet that mutex covers raising legacy IRQ
Changes since v2:
- dt-bindings: Add clock-names, reset-names, and fix example for Pro5
- Remove 'is_legacy' indicating that the compatible is for legacy SoC
- Use pci_epc_features instead of defining uniphier_soc_data
- Remove redundant register read access
- Clean up return code on uniphier_add_pcie_ep()
- typo: intx -> INTx
Changes since v1:
- dt-bindings: Add Reviewed-by line
- Fix register value to set EP mode
- Add error message when failed to get phy
- Replace INTx assertion time with macro
Kunihiko Hayashi (2):
dt-bindings: PCI: Add UniPhier PCIe endpoint controller description
PCI: uniphier: Add Socionext UniPhier Pro5 PCIe endpoint controller
driver
.../bindings/pci/socionext,uniphier-pcie-ep.yaml | 92 +++++
MAINTAINERS | 4 +-
drivers/pci/controller/dwc/Kconfig | 13 +-
drivers/pci/controller/dwc/Makefile | 1 +
drivers/pci/controller/dwc/pcie-uniphier-ep.c | 383 +++++++++++++++++++++
5 files changed, 489 insertions(+), 4 deletions(-)
create mode 100644 Documentation/devicetree/bindings/pci/socionext,uniphier-pcie-ep.yaml
create mode 100644 drivers/pci/controller/dwc/pcie-uniphier-ep.c
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] cpuidle: psci: Fixup execution order when entering a domain idle state
From: Rafael J. Wysocki @ 2020-05-14 11:52 UTC (permalink / raw)
To: Ulf Hansson
Cc: Lorenzo Pieralisi, Benjamin Gaignard, Linux PM, Stephen Boyd,
Daniel Lezcano, Rafael J . Wysocki, Lina Iyer, Bjorn Andersson,
Sudeep Holla, Linux ARM
In-Reply-To: <20200511133346.21706-1-ulf.hansson@linaro.org>
On Mon, May 11, 2020 at 3:33 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Moving forward, platforms are going to need to execute specific "last-man"
> operations before a domain idle state can be entered. In one way or the
> other, these operations needs to be triggered while walking the
> hierarchical topology via runtime PM and genpd, as it's at that point the
> last-man becomes known.
>
> Moreover, executing last-man operations needs to be done after the CPU PM
> notifications are sent through cpu_pm_enter(), as otherwise it's likely
> that some notifications would fail. Therefore, let's re-order the sequence
> in psci_enter_domain_idle_state(), so cpu_pm_enter() gets called prior
> pm_runtime_put_sync().
>
> Fixes: ce85aef570df ("cpuidle: psci: Manage runtime PM in the idle path")
> Reported-by: Lina Iyer <ilina@codeaurora.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
I can pick this up provided an ACK from a maintainer of this driver.
> ---
> drivers/cpuidle/cpuidle-psci.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index bae9140a65a5..d0fb585073c6 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -58,6 +58,10 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> u32 state;
> int ret;
>
> + ret = cpu_pm_enter();
> + if (ret)
> + return -1;
> +
> /* Do runtime PM to manage a hierarchical CPU toplogy. */
> pm_runtime_put_sync_suspend(pd_dev);
>
> @@ -65,10 +69,12 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> if (!state)
> state = states[idx];
>
> - ret = psci_enter_state(idx, state);
> + ret = psci_cpu_suspend_enter(state) ? -1 : idx;
>
> pm_runtime_get_sync(pd_dev);
>
> + cpu_pm_exit();
> +
> /* Clear the domain state to start fresh when back from idle. */
> psci_set_domain_state(0);
> return ret;
> --
> 2.20.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 00/10] drivers, provide a way to add sysfs groups easily
From: Emil Velikov @ 2020-05-14 11:48 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: platform-driver-x86, linux-fbdev, x86, H. Peter Anvin,
Bartlomiej Zolnierkiewicz, Dmitry Torokhov,
Linux-Kernel@Vger. Kernel. Org, ML dri-devel, Sudeep Holla,
Tony Prisk, Andy Shevchenko, Florian Fainelli, Borislav Petkov,
linux-input, Darren Hart, Thomas Gleixner, Andy Shevchenko,
Ingo Molnar, LAKML, Richard Gong
In-Reply-To: <20200514071631.GA1566388@kroah.com>
On Thu, 14 May 2020 at 08:16, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, May 13, 2020 at 11:18:15PM +0100, Emil Velikov wrote:
> > Hi Greg,
> >
> > On Fri, 2 Aug 2019 at 11:46, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> >
> > >
> > > I have now done this with patch 1/10. Here's the pull info if any
> > > subsystem maintainer wants to suck this into their tree to provide the
> > > ability for drivers to add/remove attribute groups easily.
> > >
> > > This is part of my driver-core tree now, and will go to Linus for
> > > 5.4-rc1, along with a few platform drivers that have been acked by their
> > > various subsystem maintainers that convert them to use this new
> > > functionality.
> > >
> > > If anyone has any questions about this, please let me know.
> > >
> > > thanks,
> > >
> > > greg k-h
> > >
> > > -------------------
> > >
> > > The following changes since commit 5f9e832c137075045d15cd6899ab0505cfb2ca4b:
> > >
> > > Linus 5.3-rc1 (2019-07-21 14:05:38 -0700)
> > >
> > > are available in the Git repository at:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git tags/dev_groups_all_drivers
> > >
> > > for you to fetch changes up to 23b6904442d08b7dbed7622ed33b236d41a3aa8b:
> > >
> > > driver core: add dev_groups to all drivers (2019-08-02 12:37:53 +0200)
> > >
> > > ----------------------------------------------------------------
> > > dev_groups added to struct driver
> > >
> > > Persistent tag for others to pull this branch from
> > >
> > > This is the first patch in a longer series that adds the ability for the
> > > driver core to create and remove a list of attribute groups
> > > automatically when the device is bound/unbound from a specific driver.
> > >
> > > See:
> > > https://lore.kernel.org/r/20190731124349.4474-2-gregkh@linuxfoundation.org
> > > for details on this patch, and examples of how to use it in other
> > > drivers.
> > >
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >
> > > ----------------------------------------------------------------
> > > Dmitry Torokhov (1):
> > > driver core: add dev_groups to all drivers
> > >
> > > drivers/base/dd.c | 14 ++++++++++++++
> > > include/linux/device.h | 3 +++
> > > 2 files changed, 17 insertions(+)
> > > _______________________________________________
> >
> > Was planning to re-spin DRM a series which uses .dev_groups, although
> > I cannot see the core patch.
> > Did the it get reverted or simply fell though the cracks?
>
> Nope, it's in there:
> 23b6904442d0 ("driver core: add dev_groups to all drivers")
> which showed up in the 5.4 kernel release.
>
> Lots of other subsystems have already been converted to use this, do you
> not see it in your tree?
>
A case of PEBKAC it seems - I was looking at a 5.3 checkout somehow.
Thanks for the core work. Will check/merge the fbdev patches over the
next few days and polish drm land.
-Emil
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] dt-bindings: pwm: Convert mxs pwm to json-schema
From: Anson Huang @ 2020-05-14 11:41 UTC (permalink / raw)
To: thierry.reding, u.kleine-koenig, robh+dt, shawnguo, s.hauer,
kernel, festevam, linux-pwm, devicetree, linux-arm-kernel,
linux-kernel
Cc: Linux-imx
Convert the mxs pwm binding to DT schema format using json-schema.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Documentation/devicetree/bindings/pwm/mxs-pwm.txt | 17 ---------
Documentation/devicetree/bindings/pwm/mxs-pwm.yaml | 43 ++++++++++++++++++++++
2 files changed, 43 insertions(+), 17 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/pwm/mxs-pwm.txt
create mode 100644 Documentation/devicetree/bindings/pwm/mxs-pwm.yaml
diff --git a/Documentation/devicetree/bindings/pwm/mxs-pwm.txt b/Documentation/devicetree/bindings/pwm/mxs-pwm.txt
deleted file mode 100644
index a1b8a48..0000000
--- a/Documentation/devicetree/bindings/pwm/mxs-pwm.txt
+++ /dev/null
@@ -1,17 +0,0 @@
-Freescale MXS PWM controller
-
-Required properties:
-- compatible: should be "fsl,imx23-pwm"
-- reg: physical base address and length of the controller's registers
-- #pwm-cells: should be 3. See pwm.yaml in this directory for a description of
- the cells format.
-- fsl,pwm-number: the number of PWM devices
-
-Example:
-
-pwm: pwm@80064000 {
- compatible = "fsl,imx28-pwm", "fsl,imx23-pwm";
- reg = <0x80064000 0x2000>;
- #pwm-cells = <3>;
- fsl,pwm-number = <8>;
-};
diff --git a/Documentation/devicetree/bindings/pwm/mxs-pwm.yaml b/Documentation/devicetree/bindings/pwm/mxs-pwm.yaml
new file mode 100644
index 0000000..da68f4a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/mxs-pwm.yaml
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/mxs-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale MXS PWM controller
+
+maintainers:
+ - Shawn Guo <shawn.guo@linaro.org>
+ - Anson Huang <anson.huang@nxp.com>
+
+properties:
+ compatible:
+ enum:
+ - fsl,imx23-pwm
+
+ reg:
+ maxItems: 1
+
+ "#pwm-cells":
+ const: 3
+
+ fsl,pwm-number:
+ $ref: '/schemas/types.yaml#/definitions/uint32'
+ description: u32 value representing the number of PWM devices
+
+required:
+ - compatible
+ - reg
+ - "#pwm-cells"
+ - fsl,pwm-number
+
+additionalProperties: false
+
+examples:
+ - |
+ pwm@80064000 {
+ compatible = "fsl,imx23-pwm";
+ reg = <0x80064000 0x2000>;
+ #pwm-cells = <3>;
+ fsl,pwm-number = <8>;
+ };
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v3 23/23] arm64: mte: Add Memory Tagging Extension documentation
From: Catalin Marinas @ 2020-05-14 11:37 UTC (permalink / raw)
To: Dave Martin
Cc: linux-arch, Richard Earnshaw, Will Deacon, Szabolcs Nagy,
Andrey Konovalov, Kevin Brodsky, linux-mm, Vincenzo Frascino,
Peter Collingbourne, linux-arm-kernel
In-Reply-To: <20200513154845.GT21779@arm.com>
On Wed, May 13, 2020 at 04:48:46PM +0100, Dave P Martin wrote:
> On Mon, May 11, 2020 at 05:40:19PM +0100, Catalin Marinas wrote:
> > On Mon, May 04, 2020 at 05:46:17PM +0100, Dave P Martin wrote:
> > > On Thu, Apr 30, 2020 at 05:23:17PM +0100, Catalin Marinas wrote:
> > > > On Wed, Apr 29, 2020 at 05:47:05PM +0100, Dave P Martin wrote:
> > > > > On Tue, Apr 21, 2020 at 03:26:03PM +0100, Catalin Marinas wrote:
> > > > > > +- *Asynchronous* - The kernel raises a ``SIGSEGV``, in the current
> > > > > > + thread, asynchronously following one or multiple tag check faults,
> > > > > > + with ``.si_code = SEGV_MTEAERR`` and ``.si_addr = 0``.
> > > > >
> > > > > For "current thread": that's a kernel concept. For user-facing
> > > > > documentation, can we say "the offending thread" or similar?
> > > > >
> > > > > For clarity, it's worth saying that the faulting address is not
> > > > > reported. Or, we could be optimistic that someday this information will
> > > > > be available and say that si_addr is the faulting address if available,
> > > > > with 0 meaning the address is not available.
> > > > >
> > > > > Maybe (void *)-1 would be better duff address, but I can't see it
> > > > > mattering much. If there's already precedent for si_addr==0 elsewhere,
> > > > > it makes sense to follow it.
> > > >
> > > > At a quick grep, I can see a few instances on other architectures where
> > > > si_addr==0. I'll add a comment here.
> > >
> > > OK, cool
> > >
> > > Except: what if we're in PR_MTE_TCF_ASYNC mode. If the SIGSEGV handler
> > > triggers an asynchronous MTE fault itself, we could then get into a
> > > spin. Hmm.
[...]
> > > In that case, an asynchronous MTE fault pending at sigreturn must have
> > > been caused by the signal handler. We could make that particular case
> > > of MTE_AERR a force_sig.
> >
> > We clear the TIF flag when delivering the signal. I don't think there is
> > a way for the kernel to detect when it is running in a signal handler.
> > sigreturn() is not mandatory either.
>
> I guess we can put up with this signal not being fatal then.
>
> If you have a SEGV handler at all, you're supposed to code it carefully.
>
> This brings us back to force_sig for SERR and a normal signal for AERR.
> That's probably OK.
I think we are in agreement now but please check the patches when I post
the v4.
> > > > > > +**Note**: Kernel accesses to user memory (e.g. ``read()`` system call)
> > > > > > +are only checked if the current thread tag checking mode is
> > > > > > +PR_MTE_TCF_SYNC.
> > > > >
> > > > > Vague? Can we make a precise statement about when the kernel will and
> > > > > won't check such accesses? And aren't there limitations (like use of
> > > > > get_user_pages() etc.)?
> > > >
> > > > We could make it slightly clearer by say "kernel accesses to the user
> > > > address space".
> > >
> > > That's not the ambiguity.
> > >
> > > My question is
> > >
> > > 1) Does the kernel guarantee not to check tags on kernel accesses to
> > > user memory without PR_MTE_TCF_SYNC?
[...]
> > > 2) Does the kernel guarantee to check tags on kernel accesses to user
> > > memory with PR_MTE_TCF_SYNC?
[...]
> > > In practice, this note sounds to be more like a kernel implementation
> > > detail rather than advice to userspace.
> > >
> > > Would it make sense to say something like:
> > >
> > > * PR_MTE_TCF_NONE: the kernel does not check tags for kernel accesses
> > > to use memory done by syscalls in the thread.
> > >
> > > * PR_MTE_TCF_ASYNC: the kernel may check some tags for kernel accesses
> > > to user memory done by syscalls. (Should we guarantee that such
> > > faults are reported synchronously on syscall exit? In practice I
> > > think they are. Should we use SEGV_MTESERR in this case? Perhaps
> > > it's not worth making this a special case.)
> >
> > Both NONE and ASYNC are now the same for kernel uaccess - not checked.
> >
> > For background information, I decided against ASYNC uaccess checking
> > since (1) there are some cases where the kernel overreads
> > (strncpy_from_user) and (2) we don't normally generate SIGSEGV on
> > uaccess but rather return -EFAULT. The latter is not possible to contain
> > since we only learn about the fault asynchronously, usually after the
> > transfer.
>
> I may be missing something here. Do we still rely on the hardware to
> detect tag mismatches in kernel accesses to user memory? I was assuming
> we do some kind of explicit checking, but now I think that's nonsense
> (except for get_user_pages() etc.)
For synchronous tag checking, we expect the uaccess (via the user
address, e.g. copy_from_user()) to be checked by the hardware. If the
access happens via a kernel mapping (get_user_pages()), the access is
unchecked. There is no point in an explicit tag access+check from the
kernel since the get_user_pages() accesses are not expected to generate
faults anyway (once the pages have been returned). We also most likely
lost the actual user address at the point of access, so not easy to
infer the original tag.
> Since MTE is a new opt-in feature, I think we might have the option to
> report failures with SIGSEGV instead of -EFAULT. This seems exactly to
> implement the concept of an asynchronous versus synchronous error.
With synchronous checking, we return -EFAULT, smaller number of bytes
etc. since no/less data was copied. With async, the uaccess would
perform all the accesses, only that the user may get a SIGSEGV delivered
on return from the syscall.
> The kernel may not normally do this, but software usually doesn't use
> raw syscalls. In reality "syscalls" can trigger a SIGSEGV in the libc
> wrapper anyway. From the caller's point of view the whole thing is a
> black box.
>
> Probably needs discussion with the bionic / glibc folks though (though
> likely this has been discussed already...)
The initial plan was to generate SIGSEGV on asynchronous faults for
uaccess (on syscall return). This changed when we noticed (in version 3
I think) that the kernel over-reads buffers in some cases
(strncpy_from_user(), copy_mount_options()) and triggers false
positives.
We could fix the above two cases, though in different ways:
strncpy_from_user() can align its source (user) address and would no
longer be expected to trigger a fault if the string is correctly tagged.
copy_mount_options(), OTOH, always reads 4K (not zero-terminated), so it
will trip over some tag mismatch. The workaround is to contain the async
tag check fault (with DSB before and after the access) and ignore it.
However, are these the only two cases where the kernel over-reads user
buffers? Without MTE, such faults on uaccess (page faults) were handled
by the kernel transparently. We may now start delivering SIGSEGV every
time some piece of uaccess kernel code changes and over-reads.
> My concern is that the spirit of asynchrous checking in the
> architecture is that accesses _are_ checked, and we seem to be
> breaking that principle here.
I agree with you on the principle but my concern is about the
practicality of chasing any future code changes and plugging potentially
fatal SIGSEGVs sent to the user.
Maybe we need a way to log this so that user (admin) can do something
about it like force synchronous. Or we could also toggle synchronous
uaccesses irrespective of the user mode or expose this option as a
prctl().
Also, do we want some big knob (sysctl) to force some of these modes for
all user processes: e.g. force-upgrade async to sync?
> > > > > > +excludes all tags other than 0. A user thread can enable specific tags
> > > > > > +in the randomly generated set using the ``prctl(PR_SET_TAGGED_ADDR_CTRL,
> > > > > > +flags, 0, 0, 0)`` system call where ``flags`` contains the tags bitmap
> > > > > > +in the ``PR_MTE_TAG_MASK`` bit-field.
> > > > > > +
> > > > > > +**Note**: The hardware uses an exclude mask but the ``prctl()``
> > > > > > +interface provides an include mask. An include mask of ``0`` (exclusion
> > > > > > +mask ``0xffff``) results in the CPU always generating tag ``0``.
> > > > >
> > > > > Is there no way to make this default to 1 rather than having a magic
> > > > > meaning for 0?
> > > >
> > > > We follow the hardware behaviour where 0xffff and 0xfffe give the same
> > > > result.
> > >
> > > Exposing this through a purely software interface seems a bit odd:
> > > because the exclude mask is privileged-access-only, the architecture
> > > could amend it to assign a different meaning to 0xffff, providing this
> > > was an opt-in change. Then we'd have to make a mess here.
> >
> > You have a point. An include mask of 0 translates to an exclude mask of
> > 0xffff as per the current patches. If the hardware gains support for one
> > more bit (32 colours), old software running on new hardware may run into
> > unexpected results with an exclude mask of 0xffff.
> >
> > > Can't we just forbid the nonsense value 0 here, or are there other
> > > reasons why that's problematic?
> >
> > It was just easier to start with a default. I wonder whether we should
> > actually switch back to the exclude mask, as per the hardware
> > definition. This way 0 would mean all tags allowed. We can still
> > disallow 0xffff as an exclude mask.
[...]
> The only configuration that doesn't make sense is "no tags allowed", so
> I'd argue for explicity blocking that, even if the architeture aliases
> that encoding to something else.
>
> If we prefer 0 as a default value so that init inherits the correct
> value from the kernel without any special acrobatics, then we make it an
> exclude mask, with the semantics that the hardware is allowed to
> generate any of these tags, but does not have to be capable of
> generating all of them.
That's more of a question to the libc people and their preference.
We have two options with suboptions:
1. prctl() gets an exclude mask with 0xffff illegal even though the
hardware accepts it:
a) default exclude mask 0, allowing all tags to be generated by IRG
b) default exclude mask of 0xfffe so that only tag 0 is generated
2. prctl() gets an include mask with 0 illegal:
a) default include mask is 0xffff, allowing all tags to be generated
b) default include mask 0f 0x0001 so that only tag 0 is generated
We currently have (2) with mask 0 but could be changed to (2.b). If we
are to follow the hardware description (which makes more sense to me but
I don't write the C library), (1.a) is the most appropriate.
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 1/4] KEYS: trusted: Add generic trusted keys framework
From: Sumit Garg @ 2020-05-14 11:23 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: tee-dev @ lists . linaro . org, Daniel Thompson, op-tee,
Jonathan Corbet, James Bottomley, Janne Karhunen,
Linux Doc Mailing List, James Morris, Mimi Zohar,
Linux Kernel Mailing List, dhowells, linux-security-module,
open list:ASYMMETRIC KEYS, Markus Wamser, Casey Schaufler,
linux-integrity, Jens Wiklander, linux-arm-kernel,
Serge E. Hallyn
In-Reply-To: <07bb6080f8be9f6613f460e2d6e19f3d456e219c.camel@linux.intel.com>
On Thu, 14 May 2020 at 05:55, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Wed, 2020-05-06 at 15:10 +0530, Sumit Garg wrote:
> > Current trusted keys framework is tightly coupled to use TPM device as
> > an underlying implementation which makes it difficult for implementations
> > like Trusted Execution Environment (TEE) etc. to provide trusked keys
> > support in case platform doesn't posses a TPM device.
> >
> > So this patch tries to add generic trusted keys framework where underlying
> > implemtations like TPM, TEE etc. could be easily plugged-in.
> >
> > Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>
> I tend to agree how this is implemented and could merge it as such.
>
> I'm just thinking if we could refine this patch in a way that instead of
> copying TRUSTED_DEBUG macro we could just replace pr_info() statements
> with pr_debug()?
AFAIU, TRUSTED_DEBUG being a security sensitive operation is only
meant to be used in development environments and should be strictly
disabled in production environments. But it may not always be true
with pr_debug() with CONFIG_DYNAMIC_DEBUG=y which allows the debug
paths to be compiled into the kernel which can be enabled/disabled at
runtime.
IMO we should keep this TRUSTED_DEBUG macro, so that users are aware
of its security sensitive nature and need to explicitly enable it to
debug.
-Sumit
>
> /Jarkko
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: ARM: static kernel in vmalloc space
From: afzal mohammed @ 2020-05-14 11:17 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Russell King, Linux ARM, linux-kernel@vger.kernel.org
In-Reply-To: <CAK8P3a1DQWG1+ab2+vQ2XCAKYxPUjJk5g3W3094j-adDXSQfzQ@mail.gmail.com>
Hi,
On Tue, May 12, 2020 at 09:49:59PM +0200, Arnd Bergmann wrote:
> Any idea which bit you want to try next?
My plan has been to next post patches for the static kernel migration
to vmalloc space (currently the code is rigid, taking easy route
wherever possible & not of high quality) as that feature has an
independent existence & adds value by itself. And then start working
on other steps towards VMSPLIT_4G_4G.
Now that you mentioned about other things, i will slowly start those
as well.
> Creating a raw_copy_{from,to}_user()
> based on get_user_pages()/kmap_atomic()/memcpy() is probably a good
> next thing to do. I think it can be done one page at a time with only
> checking for
> get_fs(), access_ok(), and page permissions, while get_user()/put_user()
> need to handle a few more corner cases.
Before starting w/ other things, i would like to align on the high
level design,
My understanding (mostly based on your comments) as follows,
(i currently do not have a firm grip over these things, hope to have
it once started w/ the implementation)
1. SoC w/ LPAE
2. TTBR1 (top 256MB) for static kernel, modules, io mappings, vmalloc,
kmap, fixmap & vectors
3. TTBR0 (low 3768MB) for user space & lowmem (kernel lowmem to have
separate ASID)
4. for user space to/from copy
a. pin user pages
b. kmap user page (can't corresponding lowmem be used instead ?)
c. copy
Main points are as above, right ?, anything missed ?, or anything more
you want to add ?, let me know your opinion.
Regards
afzal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] soc: mediatek: mmsys: Drop <linux/clk-provider.h>
From: Matthias Brugger @ 2020-05-14 11:03 UTC (permalink / raw)
To: Stephen Boyd, CK Hu, Enric Balletbo i Serra, Geert Uytterhoeven
Cc: linux-mediatek, linux-clk, linux-arm-kernel
In-Reply-To: <158931806972.215346.15226593152103612182@swboyd.mtv.corp.google.com>
On 12/05/2020 23:14, Stephen Boyd wrote:
> Quoting Geert Uytterhoeven (2020-05-06 05:02:04)
>> After the split, the mt8173 MMSYS driver is no longer a clock provider,
>> and thus does not need to include <linux/clk-provider.h>.
>>
>> Fixes: 13032709e2328553 ("clk / soc: mediatek: Move mt8173 MMSYS to platform driver")
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>
Pushed now to v5.7-next/soc
Thanks!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] coresight: etm4x: Add support to disable trace unit power up
From: Sai Prakash Ranjan @ 2020-05-14 10:59 UTC (permalink / raw)
To: Mathieu Poirier, Suzuki K Poulose, Mike Leach
Cc: Sai Prakash Ranjan, linux-arm-msm, coresight, linux-kernel,
Stephen Boyd, Tingwei Zhang, Leo Yan, linux-arm-kernel
From: Tingwei Zhang <tingwei@codeaurora.org>
On some Qualcomm Technologies Inc. SoCs like SC7180, there
exists a hardware errata where the APSS (Application Processor
SubSystem)/CPU watchdog counter is stopped when ETM register
TRCPDCR.PU=1. Since the ETMs share the same power domain as
that of respective CPU cores, they are powered on when the
CPU core is powered on. So we can disable powering up of the
trace unit after checking for this errata via new property
called "qcom,tupwr-disable".
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
Co-developed-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
---
.../devicetree/bindings/arm/coresight.txt | 6 ++++
drivers/hwtracing/coresight/coresight-etm4x.c | 29 ++++++++++++-------
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
index 846f6daae71b..d2030128fe46 100644
--- a/Documentation/devicetree/bindings/arm/coresight.txt
+++ b/Documentation/devicetree/bindings/arm/coresight.txt
@@ -108,6 +108,12 @@ its hardware characteristcs.
* arm,cp14: must be present if the system accesses ETM/PTM management
registers via co-processor 14.
+ * qcom,tupwr-disable: boolean. Indicates that trace unit power up can
+ be disabled on Qualcomm Technologies Inc. systems where ETMs are in
+ the same power domain as their CPU cores. This property is required
+ to identify such systems with hardware errata where the CPU watchdog
+ counter is stopped when TRCPDCR.PU=1.
+
* Optional property for TMC:
* arm,buffer-size: size of contiguous buffer space for TMC ETR
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
index fb0f5f4f3a91..6886b44f6947 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x.c
@@ -104,6 +104,11 @@ struct etm4_enable_arg {
int rc;
};
+static inline bool etm4_can_disable_tupwr(struct device *dev)
+{
+ return fwnode_property_present(dev_fwnode(dev), "qcom,tupwr-disable");
+}
+
static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
{
int i, rc;
@@ -196,12 +201,14 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
writel_relaxed(config->vmid_mask0, drvdata->base + TRCVMIDCCTLR0);
writel_relaxed(config->vmid_mask1, drvdata->base + TRCVMIDCCTLR1);
- /*
- * Request to keep the trace unit powered and also
- * emulation of powerdown
- */
- writel_relaxed(readl_relaxed(drvdata->base + TRCPDCR) | TRCPDCR_PU,
- drvdata->base + TRCPDCR);
+ if (!etm4_can_disable_tupwr(etm_dev)) {
+ /*
+ * Request to keep the trace unit powered and also
+ * emulation of powerdown
+ */
+ writel_relaxed(readl_relaxed(drvdata->base + TRCPDCR) | TRCPDCR_PU,
+ drvdata->base + TRCPDCR);
+ }
/* Enable the trace unit */
writel_relaxed(1, drvdata->base + TRCPRGCTLR);
@@ -476,10 +483,12 @@ static void etm4_disable_hw(void *info)
CS_UNLOCK(drvdata->base);
- /* power can be removed from the trace unit now */
- control = readl_relaxed(drvdata->base + TRCPDCR);
- control &= ~TRCPDCR_PU;
- writel_relaxed(control, drvdata->base + TRCPDCR);
+ if (!etm4_can_disable_tupwr(etm_dev)) {
+ /* power can be removed from the trace unit now */
+ control = readl_relaxed(drvdata->base + TRCPDCR);
+ control &= ~TRCPDCR_PU;
+ writel_relaxed(control, drvdata->base + TRCPDCR);
+ }
control = readl_relaxed(drvdata->base + TRCPRGCTLR);
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v2 1/2] module: allow arch overrides for .init section names
From: Vincent Whitchurch @ 2020-05-14 10:46 UTC (permalink / raw)
To: Jessica Yu, linux
Cc: kernel, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20200511154459.GA26699@linux-8ccs.fritz.box>
On Mon, May 11, 2020 at 05:45:00PM +0200, Jessica Yu wrote:
> +++ Vincent Whitchurch [11/05/20 13:48 +0200]:
> >ARM stores unwind information for .init.text in sections named
> >.ARM.extab.init.text and .ARM.exidx.init.text. Since those aren't
> >currently recognized as init sections, they're allocated along with the
> >core section, and relocation fails if the core and the init section are
> >allocated from different regions and can't reach other.
> >
> > final section addresses:
> > ...
> > 0x7f800000 .init.text
> > ..
> > 0xcbb54078 .ARM.exidx.init.text
> > ..
> >
> > section 16 reloc 0 sym '': relocation 42 out of range (0xcbb54078 ->
> > 0x7f800000)
> >
> >Allow architectures to override the section name so that ARM can fix
> >this.
> >
> >Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> >---
> >v2: Add comment and move module_init_section() next to module_exit_section().
>
> Thanks, this patch looks fine to me. You could add my:
>
> Acked-by: Jessica Yu <jeyu@kernel.org>
>
> Alternatively, I can take this through modules-next if the second
> patch gets a review and ack from an ARM maintainer.
Thanks! I've put the patches in Russell's patch system.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v7 2/3] drivers: input: keyboard: Add mtk keypad driver
From: Andy Shevchenko @ 2020-05-14 10:30 UTC (permalink / raw)
To: Fengping Yu
Cc: Dmitry Torokhov, Marco Felsch, linux-kernel, linux-mediatek,
Yingjoe Chen, linux-arm-kernel
In-Reply-To: <20200514102701.GZ185537@smile.fi.intel.com>
On Thu, May 14, 2020 at 01:27:01PM +0300, Andy Shevchenko wrote:
> On Thu, May 14, 2020 at 02:17:48PM +0800, Fengping Yu wrote:
> > From: "fengping.yu" <fengping.yu@mediatek.com>
> >
> > This adds matrix keypad support for Mediatek SoCs.
>
> ...
>
> > +config KEYBOARD_MTK_KPD
> > + tristate "MediaTek Keypad Support"
>
> > + depends on OF && HAVE_CLK
>
> What makes it OF dependent?
Actually you missed regmap dependencies or selection. I dunno which one should
be used (IIRC selection).
> > + help
> > + Say Y here if you want to use the keypad on MediaTek SoCs.
> > + If unsure, say N.
> > + To compile this driver as a module, choose M here: the
> > + module will be called mtk-kpd.
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v7 2/3] drivers: input: keyboard: Add mtk keypad driver
From: Andy Shevchenko @ 2020-05-14 10:27 UTC (permalink / raw)
To: Fengping Yu
Cc: Dmitry Torokhov, Marco Felsch, linux-kernel, linux-mediatek,
Yingjoe Chen, linux-arm-kernel
In-Reply-To: <20200514061747.25466-3-fengping.yu@mediatek.com>
On Thu, May 14, 2020 at 02:17:48PM +0800, Fengping Yu wrote:
> From: "fengping.yu" <fengping.yu@mediatek.com>
>
> This adds matrix keypad support for Mediatek SoCs.
...
> +config KEYBOARD_MTK_KPD
> + tristate "MediaTek Keypad Support"
> + depends on OF && HAVE_CLK
What makes it OF dependent?
> + help
> + Say Y here if you want to use the keypad on MediaTek SoCs.
> + If unsure, say N.
> + To compile this driver as a module, choose M here: the
> + module will be called mtk-kpd.
...
> +#define KPD_DEBOUNCE_MAX_US 256000 /*256ms */
Comment, besides missed space, is redundant. That's how we use unit suffixes in
the definitions.
...
> +static const struct regmap_config keypad_regmap_cfg = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = sizeof(u32),
> + .max_register = 0x0024,
Can it be definition?
> +};
...
> + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
> + if (!keypad)
> + return -ENOMEM;
+ blank line here.
> + keypad->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(keypad->base))
> + return PTR_ERR(keypad->base);
...
> + if (debounce > KPD_DEBOUNCE_MAX_US) {
> + dev_err(&pdev->dev, "Debounce time exceeds the maximum allowed time 256ms\n");
...%dus\n", KPD_DEBOUNCE_MAX_US);
or
...%dms\n", KPD_DEBOUNCE_MAX_US / USEC_PER_MSEC);
> + return -EINVAL;
> + }
...
> + keypad_pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (IS_ERR(keypad_pinctrl)) {
> + return PTR_ERR(keypad_pinctrl);
> + }
Extra {}.
...
> + kpd_default = pinctrl_lookup_state(keypad_pinctrl, "default");
> + if (IS_ERR(kpd_default)) {
> + dev_err(&pdev->dev, "No default pinctrl state\n");
Isn't it done by pin control core?
> + return PTR_ERR(kpd_default);
> + }
> +
> + pinctrl_select_state(keypad_pinctrl, kpd_default);
And basically entire part is duplicating device core part? (Look at dd.c)
...
> + irqnr = platform_get_irq(pdev, 0);
> + if (irqnr < 0) {
> + dev_err(&pdev->dev, "Failed to get irq\n");
This duplicates what platform core does.
> + return -irqnr;
- ?!
> + }
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 07/10] spi: spi-dw-mchp: Add Sparx5 support
From: Mark Brown @ 2020-05-14 10:25 UTC (permalink / raw)
To: Lars Povlsen
Cc: devicetree, Alexandre Belloni, linux-kernel, linux-spi, SoC Team,
Microchip Linux Driver Support, linux-arm-kernel
In-Reply-To: <20200513140031.25633-8-lars.povlsen@microchip.com>
[-- Attachment #1.1: Type: text/plain, Size: 1425 bytes --]
On Wed, May 13, 2020 at 04:00:28PM +0200, Lars Povlsen wrote:
> +static void dw_spi_mchp_set_cs_owner(struct dw_spi_mchp *dwsmchp,
> + const struct dw_spi_mchp_props *props,
> + u8 cs, u8 owner)
> {
> + u8 dummy = (owner == MSCC_IF_SI_OWNER_SIBM ?
> + MSCC_IF_SI_OWNER_SIMC : MSCC_IF_SI_OWNER_SIBM);
Please write normal conditional statements to improve legibility.
> +static void dw_spi_mchp_set_cs(struct spi_device *spi, bool nEnable)
> +{
> + bool enable = !nEnable; /* This keeps changing in the API... */
No, it doesn't. The API has not changed for more than a decade.
> + } else if (props->ss_force_ena_off) {
> + if (enable) {
> + /* Ensure CS toggles, so start off all disabled */
> + regmap_write(dwsmchp->syscon, props->ss_force_val_off,
> + ~0);
What's all this force_ena_off stuff about? The controller should not be
making decisions about management of the chip select, this will break
users.
> + if (pdev->dev.of_node) {
> + int i;
> +
> + for (i = 0; i < dws->num_cs; i++) {
> + int cs_gpio = of_get_named_gpio(pdev->dev.of_node,
> + "cs-gpios", i);
> +
> + if (cs_gpio == -EPROBE_DEFER) {
> + ret = cs_gpio;
> + goto out;
> + }
> +
> + if (gpio_is_valid(cs_gpio)) {
> + ret = devm_gpio_request(&pdev->dev, cs_gpio,
> + dev_name(&pdev->dev));
> + if (ret)
> + goto out;
Set use_gpio_descriptors and let the core manage the GPIO.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 00/13] usb: dwc3: meson: add OTG support for GXL/GXM
From: Felipe Balbi @ 2020-05-14 10:23 UTC (permalink / raw)
To: Neil Armstrong, kishon, khilman, martin.blumenstingl
Cc: linux-amlogic, linux-usb, linux-kernel, linux-arm-kernel,
Neil Armstrong
In-Reply-To: <87369rfo7l.fsf@kernel.org>
[-- Attachment #1.1: Type: text/plain, Size: 2105 bytes --]
Felipe Balbi <balbi@kernel.org> writes:
> Neil Armstrong <narmstrong@baylibre.com> writes:
>
>> The USB support was initialy done with a set of PHYs and dwc3-of-simple
>> because the architecture of the USB complex was not understood correctly
>> at the time (and proper documentation was missing...).
>>
>> But with the G12A family, the USB complex was correctly understood and
>> implemented correctly.
>> But seems the G12A architecture was derived for the GXL USB architecture,
>> with minor differences and looks we can share most of the USB DWC3 glue
>> driver.
>>
>> This patchset refactors and adds callbacks to handle the architecture
>> difference while keeping the main code shared.
>>
>> The main difference is that on GXL/GXM the USB2 PHY control registers
>> are mixed with the PHY registers (we already handle correctly), and
>> the GLUE registers are allmost (99%) the same as G12A.
>>
>> But, the GXL/GXM HW is buggy, here are the quirks :
>> - for the DWC2 controller to reset correctly, the GLUE mux must be switched
>> to peripheral when the DWC2 controlle probes. For now it's handled by simply
>> switching to device when probing the subnodes, but it may be not enough
>> - when manually switching from Host to Device when the USB port is not
>> populated (should not happen with proper Micro-USB/USB-C OTG switch), it
>> makes the DWC3 to crash. The only way to avoid that is to use the Host
>> Disconnect bit to disconnect the DWC3 controller from the port, but we can't
>> recover the Host functionnality unless resetting the DWC3 controller.
>> This bit is set when only manual switch is done, and a warning is printed
>> on manual switching.
>>
>> The patches 1-8 should be applied first, then either waiting the next release
>> or if the usb maintainer can provide us a stable tag, we can use it to merge
>> the DT and bindings.
>
> it's unclear to me if this series is ready to be merged. Can someone
> confirm? If it is, can you resend with all reviewed by tags in place?
Are we getting a v2 for this?
--
balbi
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 5/5] drm/panel: Add ilitek ili9341 driver
From: dillon min @ 2020-05-14 10:22 UTC (permalink / raw)
To: Linus Walleij
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Alexandre TORGUE, Dave Airlie, Michael Turquette,
linux-kernel@vger.kernel.org, open list:DRM PANEL DRIVERS,
linux-clk, Stephen Boyd, Rob Herring, thierry.reding@gmail.com,
Maxime Coquelin, Daniel Vetter, Sam Ravnborg, linux-stm32,
Linux ARM
In-Reply-To: <CACRpkdbZoMDC-D12CByKJUZbu4shqixC=QrKwJUd8x=nyK7seQ@mail.gmail.com>
Hi Linus,
Thanks for reviewing.
On Thu, May 14, 2020 at 4:14 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Dillon,
>
> thanks for your patch! Overall this looks like a good start.
>
> On Tue, May 12, 2020 at 9:04 AM <dillon.minfei@gmail.com> wrote:
>
> > #define ILI9341_SLEEP_OUT 0x11 /* Sleep out register */
>
> This is not a register, also just use MIPI_DCS_EXIT_SLEEP_MODE
> in the code.
>
Yes, i will try to reuse MIPI_DCS_xxx.
> > +#define ILI9341_DFC 0xb6 /* Display Function Control
> > + * register
> > + */
>
> This commenting style doesn't work, either just put it after /* the define */
> and don't care if the line gets a bit long and checkpatch complains,
> or
>
> /*
> * Put it above the define like this
> */
> #define FOO 0x00
>
Ok, will change this comments.
> > +/**
> > + * struct ili9341_config - the system specific ILI9341 configuration
>
> Nice with this per-system config, it makes the driver easy to maintain
> for new users.
>
Yes, will try to move more system related configurations to this part,
instead of hard code.
> > +static int ili9341_dpi_init(struct ili9341 *ili)
> > +{
> > + ili9341_command(ili, 0xca, 0xc3, 0x08, 0x50);
>
> This stuff is a bit hard to understand, don't you think?
>
> But given that register 0xCA seems undocumented I don't
> know if there is anything more you can do, so it is OK
> I suppose.
>
> > + ili9341_command(ili, ILI9341_POWERB, 0x00, 0xc1, 0x30);
>
> This command is described in the manual page 196.
> Version: V1.11
> Document No.: ILI9341_DS_V1.11.pdf
> https://dflund.se/~triad/ILI9341_v1.11.pdf
>
Yes, "ili9341_command(ili, 0xca, 0xc3, 0x08, 0x50);" i ported from st's sdk.
will use ILI9341_XXX to replace these magic numbers
> And this goes for all the below commands. Please add some more defines
> from the datasheet and have less magic numbers in the driver.
>
> > + ili9341_command(ili, ILI9341_POWER_SEQ, 0x64, 0x03, 0x12, 0x81);
> > + ili9341_command(ili, ILI9341_DTCA, 0x85, 0x00, 0x78);
> > + ili9341_command(ili, ILI9341_POWERA, 0x39, 0x2c, 0x00, 0x34, 0x02);
> > + ili9341_command(ili, ILI9341_PRC, 0x20);
> > + ili9341_command(ili, ILI9341_DTCB, 0x00, 0x00);
> > + ili9341_command(ili, ILI9341_FRC, 0x00, 0x1b);
> > + ili9341_command(ili, ILI9341_DFC, 0x0a, 0xa2);
> > + ili9341_command(ili, ILI9341_POWER1, 0x10);
> > + ili9341_command(ili, ILI9341_POWER2, 0x10);
> > + ili9341_command(ili, ILI9341_VCOM1, 0x45, 0x15);
> > + ili9341_command(ili, ILI9341_VCOM2, 0x90);
> > + ili9341_command(ili, ILI9341_MAC, 0xc8);
> > + ili9341_command(ili, ILI9341_3GAMMA_EN, 0x00);
> > + ili9341_command(ili, ILI9341_RGB_INTERFACE, 0xc2);
> > + ili9341_command(ili, ILI9341_DFC, 0x0a, 0xa7, 0x27, 0x04);
> > + ili9341_command(ili, ILI9341_COLUMN_ADDR, 0x00, 0x00, 0x00, 0xef);
> > + ili9341_command(ili, ILI9341_PAGE_ADDR, 0x00, 0x00, 0x01, 0x3f);
> > + ili9341_command(ili, ILI9341_INTERFACE, 0x01, 0x00, 0x06);
> > + if (ili->input == ILI9341_INPUT_PRGB_18_BITS)
> > + ili9341_command(ili, ILI9341_PIXEL_FORMAT, 0x66);
> > + else
> > + ili9341_command(ili, ILI9341_PIXEL_FORMAT, 0x56);
> > + ili9341_command(ili, ILI9341_GRAM);
> > + msleep(200);
>
> I think some of the above should not be hard coded but should instead
> be stored in fields in struct ili9341_config. I know it can be a bit
> tedious but it makes things much more clear.
>
Ok, will go deeper to find out some register configuration move to
system config like rgb bus 16/18 bits
> > + ili9341_command(ili, ILI9341_GAMMA, 0x01);
> > + ili9341_command(ili, ILI9341_PGAMMA, 0x0f, 0x29, 0x24, 0x0c, 0x0e,
> > + 0x09, 0x4e, 0x78, 0x3c, 0x09,
> > + 0x13, 0x05, 0x17, 0x11, 0x00);
> > + ili9341_command(ili, ILI9341_NGAMMA, 0x00, 0x16, 0x1b, 0x04, 0x11,
> > + 0x07, 0x31, 0x33, 0x42, 0x05,
> > + 0x0c, 0x0a, 0x28, 0x2f, 0x0f);
>
> This should definately be in ili9341_config, as it is a screen property.
>
> In the long run I would like these panels to support setting gamma
> from userspace etc but it is a big tedious work to get that right
> so hard-coding a default per-variant is fine.
>
ok, will refer to panel-ilitek-ili9322 and panel-novatek-nt35510 driver.
> You can check in e.g. panel-novatek-nt35510.c how I encoded
> such sequences in per-variant data.
>
> > +static int ili9341_dpi_power_off(struct ili9341 *ili)
> > +{
> > + /* Disable power */
> > + if (!IS_ERR(ili->vcc))
> > + return regulator_disable(ili->vcc);
> > +
> > + return 0;
> > +}
>
> Usually you should also assert RESET when disabling
> power.
>
ok
> > +/* This is the only mode listed for parallel RGB in the datasheet */
> > +static const struct drm_display_mode rgb_240x320_mode = {
> > + .clock = 6100,
> > + .hdisplay = 240,
> > + .hsync_start = 240 + 10,
> > + .hsync_end = 240 + 10 + 10,
> > + .htotal = 240 + 10 + 10 + 20,
> > + .vdisplay = 320,
> > + .vsync_start = 320 + 4,
> > + .vsync_end = 320 + 4 + 2,
> > + .vtotal = 320 + 4 + 2 + 2,
> > + .vrefresh = 60,
> > + .flags = 0,
> > + .width_mm = 65,
> > + .height_mm = 50,
>
> The width and height should certainly be om the ili9341_config
> as it is a per-panel property. You can just fill in in in
> the below .get_modes() function. Or assign the whole
> mode as part of the ili9341_config if that is easier.
>
ok, lcd timing part will move to ili9341 config
> > + return drm_panel_add(&ili->panel);
> > +}
> > +
> > +
> > +
>
> Surplus whitespace here.
ok, will delete it.
>
> > + mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_OFF);
> > +
> > + mipi_dbi_command(dbi, ILI9341_POWERB, 0x00, 0xc1, 0x30);
> > + mipi_dbi_command(dbi, ILI9341_POWER_SEQ, 0x64, 0x03, 0x12, 0x81);
>
> Some of these are just copies of the above init sequence, so it makes
> even more sense to just have these settings stored in
> ili9341_config.
ok.
>
> > + mipi_dbi_command(dbi, ILI9341_DTCA, 0x85, 0x00, 0x78);
> > + mipi_dbi_command(dbi, ILI9341_POWERA, 0x39, 0x2c, 0x00, 0x34, 0x02);
> > + mipi_dbi_command(dbi, ILI9341_PRC, 0x20);
> > + mipi_dbi_command(dbi, ILI9341_DTCB, 0x00, 0x00);
> > +
> > + /* Power Control */
> > + mipi_dbi_command(dbi, ILI9341_POWER1, 0x23);
> > + mipi_dbi_command(dbi, ILI9341_POWER2, 0x10);
> > + /* VCOM */
> > + mipi_dbi_command(dbi, ILI9341_VCOM1, 0x3e, 0x28);
> > + mipi_dbi_command(dbi, ILI9341_VCOM2, 0x86);
> > +
> > + /* Memory Access Control */
> > + mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT,
> > + MIPI_DCS_PIXEL_FMT_16BIT);
> > +
> > + /* Frame Rate */
> > + mipi_dbi_command(dbi, ILI9341_FRC, 0x00, 0x1b);
> > +
> > + /* Gamma */
> > + mipi_dbi_command(dbi, ILI9341_3GAMMA_EN, 0x00);
> > + mipi_dbi_command(dbi, MIPI_DCS_SET_GAMMA_CURVE, 0x01);
> > + mipi_dbi_command(dbi, ILI9341_PGAMMA,
> > + 0x0f, 0x31, 0x2b, 0x0c, 0x0e, 0x08, 0x4e, 0xf1,
> > + 0x37, 0x07, 0x10, 0x03, 0x0e, 0x09, 0x00);
> > + mipi_dbi_command(dbi, ILI9341_NGAMMA,
> > + 0x00, 0x0e, 0x14, 0x03, 0x11, 0x07, 0x31, 0xc1,
> > + 0x48, 0x08, 0x0f, 0x0c, 0x31, 0x36, 0x0f);
>
> It seems to be copies of the stuff above, but why is there a different
> gamma if you use DBI?
for dbi interface, currently i just copy the code from tiny/ili9341.c.
as so many boards use this driver now, like raspberry pi, etc
i'm afraid it's will not work after modification. so, just leave the
original code there.
>
> I suspect only one of them is really needed and it is not even
> necessary to set if up in two places.
>
as i know, dbi interface use spi to transfer video data and register
set to panel.
but dpi use rgb bus transfer video data, spi set register, they are
two different type for drm.
i can't register two different interface into drm at the same time.
so, i use two path.
for code management, they have some common part, like register init.
power on/off process. i will try to reuse most common functions. to make code
easier to be understand.
anther question:
is there any panel driver have dbi & dpi or dpi & dsi supported? which
i mean support
two different panel interface in one driver.
thanks
> > +out_enable:
> > + switch (dbidev->rotation) {
> > + default:
> > + addr_mode = ILI9341_MADCTL_MX;
> > + break;> +out_enable:
> > + switch (dbidev->rotation) {
> > + default:
> > + addr_mode = ILI9341_MADCTL_MX;
> > + break;
> > + case 90:
> > + addr_mode = ILI9341_MADCTL_MV;
> > + break;
> > + case 180:
> > + addr_mode = ILI9341_MADCTL_MY;
> > + break;
> > + case 270:
> > + addr_mode = ILI9341_MADCTL_MV | ILI9341_MADCTL_MY |
> > + ILI9341_MADCTL_MX;
> > + break;
> > + }
> > + addr_mode |= ILI9341_MADCTL_BGR;
> > + mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> > + mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
> > + DRM_DEBUG_KMS("initialized display serial interface\n");
> > +out_exit:
> > + drm_dev_exit(idx);
> > +}
> > +
>
> > + case 90:
> > + addr_mode = ILI9341_MADCTL_MV;
> > + break;
> > + case 180:
> > + addr_mode = ILI9341_MADCTL_MY;
> > + break;
> > + case 270:
> > + addr_mode = ILI9341_MADCTL_MV | ILI9341_MADCTL_MY |
> > + ILI9341_MADCTL_MX;
> > + break;
> > + }
> > + addr_mode |= ILI9341_MADCTL_BGR;
> > + mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
>
> Since you use MIPI_DCS_* define here, check if this applies
> to more of the commands above so you don't need custom
> defines for them. e.g.
> ILI9341_SLEEP_OUT 0x11 = MIPI_DCS_EXIT_SLEEP_MODE
> and that isn't even a register right, it is just a command?
> (Noted in the beginning.)
>
ok, will try to reuse MIPI_DCS_xxx
> Yours,
> Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] i2c: mediatek: Add i2c ac-timing adjust support
From: Qii Wang @ 2020-05-14 10:16 UTC (permalink / raw)
To: Wolfram Sang
Cc: srv_heupstream, linux-mediatek, linux-i2c, linux-arm-kernel,
linux-kernel
In-Reply-To: <20200514062951.GA1145@kunai>
On Thu, 2020-05-14 at 08:29 +0200, Wolfram Sang wrote:
> > > Last question: You seem to be the one doing major updates to this
> > > driver. Thanks for that! Are you maybe interested in becoming the
> > > maintainer for this driver? I think there won't be much patches to
> > > review and reports to handle but it will speed up processing for me.
> >
> > Yes, It is my honor to be the maintainer for this driver.
>
> Awesome! :) Can you prepare a patch for MAINTAINERS or shall I?
>
ok, I will send with this patch.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v7 1/3] dt-bindings: Add keypad devicetree documentation
From: Matthias Brugger @ 2020-05-14 10:04 UTC (permalink / raw)
To: Fengping Yu, Yingjoe Chen, Dmitry Torokhov, Andy Shevchenko,
Marco Felsch
Cc: linux-mediatek, linux-kernel, linux-arm-kernel
In-Reply-To: <20200514061747.25466-2-fengping.yu@mediatek.com>
On 14/05/2020 08:17, Fengping Yu wrote:
> From: "fengping.yu" <fengping.yu@mediatek.com>
>
> Add Mediatek matrix keypad dt-binding doc as yaml schema.
>
> Signed-off-by: fengping.yu <fengping.yu@mediatek.com>
> ---
> .../devicetree/bindings/input/mtk-kpd.yaml | 102 ++++++++++++++++++
> 1 file changed, 102 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/mtk-kpd.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/mtk-kpd.yaml b/Documentation/devicetree/bindings/input/mtk-kpd.yaml
> new file mode 100644
> index 000000000000..8f594fe0bfc2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/mtk-kpd.yaml
> @@ -0,0 +1,102 @@
> +%YAML 1.2
> +---
> +version: 1
> +
> +$id: http://devicetree.org/schemas/input/mtk-keypad.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek's Keypad Controller device tree bindings
> +
> +maintainer:
> + - Fengping Yu <fengping.yu@mediatek.com>
> +
> +description: |
> + Mediatek's Keypad controller is used to interface a SoC with a matrix-type
> + keypad device. The keypad controller supports multiple row and column lines.
> + A key can be placed at each intersection of a unique row and a unique column.
> + The keypad controller can sense a key-press and key-release and report the
> + event using a interrupt to the cpu.
> +
> +properties:
> + compatible:
> + constraint: |
> + "mediatek,mt6779-keypad"
> + "mediatek, kp"
"mediatek, kp" is too generic. Are you aware that the keypad can be used by
other SoCs. Then we should add them later with a fallback to mt6779-keypad.
Regards,
Matthias
> +
> + clock-names:
> + description: Names of the clocks listed in clocks property in the same order
> +
> + clocks:
> + description: Must contain one entry, for the module clock
> + refs: devicetree/bindings/clocks/clock-bindings.txt for details.
> +
> + interrupts:
> + description: A single interrupt specifier
> +
> + linux,keymap:
> + description: The keymap for keys as described in the binding document
> + refs: devicetree/bindings/input/matrix-keymap.txt
> +
> + pinctrl-0:
> + description: Specify pin control groups used for this controller
> + refs: devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +
> + pinctrl-names:
> + description: Names for optional pin modes
> +
> + reg:
> + description: The base address of the Keypad register bank
> +
> + wakeup-source:
> + description: use any event on keypad as wakeup event
> +
> + keypad,num-columns:
> + description: Number of column lines connected to the keypad controller,
> + it is not equal to PCB columns number, instead you should add required value
> + for each IC
> +
> + keypad,num-rows:
> + description: Number of row lines connected to the keypad controller, it is
> + not equal to PCB rows number, instead you should add required value for each IC
> +
> + mediatek,debounce-us:
> + description: Debounce interval in microseconds
> + maximum: 256000
> +
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - mediatek,debounce-us
> + - keypad,num-rows
> + - keypad,num-columns
> + - linux,keymap
> + - pinctrl
> + - clocks
> + - clock-names
> +
> +optional:
> + - wakeup-source:
> +
> +examples:
> + - |
> +
> + keypad: kp@10010000 {
> + compatible = "mediatek,kp";
> + reg = <0 0x10010000 0 0x1000>;
> + wakeup-source;
> + interrupts = <GIC_SPI 75 IRQ_TYPE_EDGE_FALLING>;
> + clocks = <&clk26m>;
> + clock-names = "kpd";
> + };
> +
> + &keypad {
> + mediatek,debounce-us = <32000>;
> + keypad,num-rows = <8>;
> + keypad,num-columns = <9>;
> + linux,keymap = < MATRIX_KEY(0x00, 0x00, KEY_VOLUMEDOWN) >;
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&kpd_gpios_def_cfg>;
> + };
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] usb: gadget: u_serial: fix coverity warning: negative index at array
From: Felipe Balbi @ 2020-05-14 9:38 UTC (permalink / raw)
To: Macpaul Lin, Greg Kroah-Hartman, Matthias Brugger,
Michał Mirosław, Sergey Organov, Macpaul Lin, linux-usb,
linux-kernel, linux-arm-kernel, linux-mediatek
Cc: Stan Lu, Macpaul Lin, Mediatek WSD Upstream
In-Reply-To: <1589443500-3990-1-git-send-email-macpaul.lin@mediatek.com>
[-- Attachment #1.1: Type: text/plain, Size: 919 bytes --]
Macpaul Lin <macpaul.lin@mediatek.com> writes:
> This issue has been reported by coverity scanner.
> Replace "int portnum" by "unsigned int", this void negative index at
> array.
>
> Signed-off-by: Stan Lu <stan.lu@mediatek.com>
> Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
please rebase on testing/fixes
error: patch failed: drivers/usb/gadget/function/u_serial.c:587
error: drivers/usb/gadget/function/u_serial.c: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: usb: gadget: u_serial: fix coverity warning: negative index at array
Patch failed at 0001 usb: gadget: u_serial: fix coverity warning: negative index at array
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
--
balbi
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 6/6] dt-bindings: drm: bridge: adi,adv7511.txt: convert to yaml
From: Ricardo Cañuelo @ 2020-05-14 9:36 UTC (permalink / raw)
To: Laurent Pinchart
Cc: devicetree, geert+renesas, xuwei5, robh+dt, kernel,
linux-arm-kernel
In-Reply-To: <20200514015412.GF7425@pendragon.ideasonboard.com>
Hi Laurent, thanks for the thorough review. Some comments below:
On jue 14-05-2020 04:54:12, Laurent Pinchart wrote:
> > +description: |
> > + The ADV7511, ADV7511W and ADV7513 are HDMI audio and video
> > + transmitters compatible with HDMI 1.4 and DVI 1.0. They support color
> > + space conversion, S/PDIF, CEC and HDCP. They support RGB input
> > + interface.
>
> I would write the last sentence as "The transmitter input is parallel
> RGB or YUV data." as YUV is also supported.
Ok.
> > + adi,input-colorspace:
> > + description: Input color space.
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/string
> > + - enum: [ rgb, yuv422, yuv444 ]
>
> Isn't string implied ? Can't you write
>
> adi,input-colorspace:
> description: Input color space.
> enum: [ rgb, yuv422, yuv444 ]
example-schema.yaml says that
Vendor specific properties have slightly different schema
requirements than common properties. They must have at least a type
definition and 'description'.
However, dt_binding_check doesn't seem to enforce this rule for string
properties, and I saw a couple of vendor-specific string properties in
other bindings that don't define the type either, so I'm going to follow
your suggestion but only for string properties, the rest need a type
definition.
I noticed I can remove the "allOf" keywords from these too.
> > + adi,embedded-sync:
> > + description:
> > + The input uses synchronization signals embedded in the data
> > + stream (similar to BT.656). Defaults to 0 (separate H/V
> > + synchronization signals).
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/uint32
> > + - enum: [ 0, 1 ]
> > + - default: 0
>
> This be a boolean property (it is read as a bool by the driver, the
> property being absent means false, the property being present means
> true).
You're completely right.
> > + ports:
> > + description:
> > + The ADV7511(W)/13 has two video ports and one audio port. This node
> > + models their connections as documented in
> > + Documentation/devicetree/bindings/media/video-interfaces.txt
> > + Documentation/devicetree/bindings/graph.txt
> > + type: object
> > + properties:
> > + port@0:
> > + description: Video port for the RGB, YUV or DSI input.
>
> s/RGB, YUV or DSI/RGB or YUV/
Ok.
> > +if:
> > + not:
> > + properties:
> > + adi,input-colorspace:
> > + contains:
> > + enum: [ rgb, yuv444 ]
> > + adi,input-clock:
> > + contains:
> > + const: 1x
>
> As both properties take a single value, I think you can omit
> "contains:".
I think it's required here, removing it makes the test fail.
> > +required:
> > + - compatible
> > + - reg
> > + - ports
> > + - adi,input-depth
> > + - adi,input-colorspace
> > + - adi,input-clock
>
> Shouldn't the power supplies be required ?
Good question. The original binding is not completely clear on
this. There's a "Required properties" section at the top that doesn't
include the supplies, the supplies block for the ADV7511/11w/13 looks
like a gray area in that regard, while the same block for ADV7533/35 is
more explicit in that they are required, and they are not listed in the
"Optional properties" section.
However, most of the DTs that use this binding don't define any
supplies. And AFAICT from looking at the code, regulator_get() will use
a dummy regulator and show a warning for every expected regulator that's
not defined in the DT.
If we want to be more strict and require the definition of all the
supplies, there will be many more DTs changes in the series, and I'm not
sure I'll be able to do that in a reasonable amount of time. I'm looking
at them and it's not always clear which regulators to use or if they are
even defined.
> > +description: |
> > + The ADV7533 and ADV7535 are HDMI audio and video transmitters
> > + compatible with HDMI 1.4 and DVI 1.0. They support color space
> > + conversion, S/PDIF, CEC and HDCP. They support DSI for input pixels.
>
> I would write the last sentence as "The transmitter input is MIPI DSI.".
>
> ...
>
> > + Disables the interal timing generator. The chip will rely on the
>
> s/interal/internal/
>
> > + sync signals in the DSI data lanes, rather than generate its own
>
> s/generate/generating/
>
> ...
>
> > + properties:
> > + port@0:
> > + description:
> > + Video port for the RGB, YUV or DSI input. The remote endpoint
>
> s/RGB, YUV or //
Ok.
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > + adv7511w_in: endpoint {
> > + remote-endpoint = <&dpi_out>;
> > + };
> > + };
> > +
> > + port@1 {
> > + reg = <1>;
> > + adv7511_out: endpoint {
> > + remote-endpoint = <&hdmi_connector_in>;
> > + };
> > + };
>
> The name of the two endpoints doesn't match the adv7533. The remote
> endpoint phandle for port 0 should have dsi in its name.
Oops, thanks for catching these.
Cheers,
Ricardo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v7 1/3] dt-bindings: Add keypad devicetree documentation
From: Marco Felsch @ 2020-05-14 9:31 UTC (permalink / raw)
To: Fengping Yu
Cc: Dmitry Torokhov, linux-kernel, linux-mediatek, Yingjoe Chen,
Andy Shevchenko, linux-arm-kernel
In-Reply-To: <20200514061747.25466-2-fengping.yu@mediatek.com>
Hi,
On 20-05-14 14:17, Fengping Yu wrote:
> From: "fengping.yu" <fengping.yu@mediatek.com>
>
> Add Mediatek matrix keypad dt-binding doc as yaml schema.
>
> Signed-off-by: fengping.yu <fengping.yu@mediatek.com>
> ---
> .../devicetree/bindings/input/mtk-kpd.yaml | 102 ++++++++++++++++++
> 1 file changed, 102 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/mtk-kpd.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/mtk-kpd.yaml b/Documentation/devicetree/bindings/input/mtk-kpd.yaml
> new file mode 100644
> index 000000000000..8f594fe0bfc2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/mtk-kpd.yaml
> @@ -0,0 +1,102 @@
> +%YAML 1.2
> +---
> +version: 1
> +
> +$id: http://devicetree.org/schemas/input/mtk-keypad.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek's Keypad Controller device tree bindings
> +
> +maintainer:
> + - Fengping Yu <fengping.yu@mediatek.com>
> +
> +description: |
> + Mediatek's Keypad controller is used to interface a SoC with a matrix-type
> + keypad device. The keypad controller supports multiple row and column lines.
> + A key can be placed at each intersection of a unique row and a unique column.
> + The keypad controller can sense a key-press and key-release and report the
> + event using a interrupt to the cpu.
> +
> +properties:
> + compatible:
> + constraint: |
enum or const ?
> + "mediatek,mt6779-keypad"
> + "mediatek, kp"
> +
> + clock-names:
> + description: Names of the clocks listed in clocks property in the same order
> +
> + clocks:
> + description: Must contain one entry, for the module clock
> + refs: devicetree/bindings/clocks/clock-bindings.txt for details.
> +
> + interrupts:
> + description: A single interrupt specifier
> +
> + linux,keymap:
> + description: The keymap for keys as described in the binding document
> + refs: devicetree/bindings/input/matrix-keymap.txt
> +
> + pinctrl-0:
> + description: Specify pin control groups used for this controller
> + refs: devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +
> + pinctrl-names:
> + description: Names for optional pin modes
> +
> + reg:
> + description: The base address of the Keypad register bank
> +
> + wakeup-source:
> + description: use any event on keypad as wakeup event
> +
> + keypad,num-columns:
> + description: Number of column lines connected to the keypad controller,
> + it is not equal to PCB columns number, instead you should add required value
> + for each IC
> +
> + keypad,num-rows:
> + description: Number of row lines connected to the keypad controller, it is
> + not equal to PCB rows number, instead you should add required value for each IC
> +
> + mediatek,debounce-us:
> + description: Debounce interval in microseconds
> + maximum: 256000
I think that you are missing the maxItems on all your properties so we
can't make any validation. Also the indent looks wrong.
> +
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - mediatek,debounce-us
> + - keypad,num-rows
> + - keypad,num-columns
> + - linux,keymap
> + - pinctrl
> + - clocks
> + - clock-names
> +
> +optional:
> + - wakeup-source:
I think the optional can be dropped.
> +examples:
> + - |
> +
> + keypad: kp@10010000 {
> + compatible = "mediatek,kp";
> + reg = <0 0x10010000 0 0x1000>;
> + wakeup-source;
> + interrupts = <GIC_SPI 75 IRQ_TYPE_EDGE_FALLING>;
> + clocks = <&clk26m>;
> + clock-names = "kpd";
> + };
> +
> + &keypad {
> + mediatek,debounce-us = <32000>;
> + keypad,num-rows = <8>;
> + keypad,num-columns = <9>;
> + linux,keymap = < MATRIX_KEY(0x00, 0x00, KEY_VOLUMEDOWN) >;
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&kpd_gpios_def_cfg>;
> + };
Pls squash those nodes.
Regards,
Marco
> --
> 2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3] ACPI/IORT: Fix PMCG node always look for a single ID mapping.
From: Lorenzo Pieralisi @ 2020-05-14 9:29 UTC (permalink / raw)
To: Tuan Phan
Cc: Robin Murphy, Neil Leeder, Hanjun Guo, Rafael J. Wysocki,
linux-kernel, Shameer Kolothum, linux-acpi, Sudeep Holla, patches,
Will Deacon, linux-arm-kernel, Len Brown
In-Reply-To: <1589415122-5899-1-git-send-email-tuanphan@os.amperecomputing.com>
Please update the subject:
Subject: "ACPI/IORT: Fix PMCG node single ID mapping handling"
On Wed, May 13, 2020 at 05:12:02PM -0700, Tuan Phan wrote:
> PMCG node can have zero ID mapping if its overflow interrupt
> is wire based. The code to parse PMCG node can not assume it will
> have a single ID mapping.
"An IORT PMCG node can have no ID mapping if its overflow interrupt is
wire based therefore the code that parses the PMCG node can not assume
the node will always have a single mapping present at index 0.
Fix iort_get_id_mapping_index() by checking for an overflow interrupt
and mapping count."
> Fixes: 24e516049360 ("ACPI/IORT: Add support for PMCG")
> Reviewed-by: Hanjun Guo <guoahanjun@huawei.com>
> Signed-off-by: Tuan Phan <tuanphan@os.amperecomputing.com>
> ---
> v1 -> v2:
> - Use pmcg node to detect wired base overflow interrupt.
>
> v2 -> v3:
> - Address Hanjun and Robin's comments.
>
> drivers/acpi/arm64/iort.c | 5 +++++
> 1 file changed, 5 insertions(+)
Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index ed3d2d1..12bb70e 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -414,6 +414,7 @@ static struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
> static int iort_get_id_mapping_index(struct acpi_iort_node *node)
> {
> struct acpi_iort_smmu_v3 *smmu;
> + struct acpi_iort_pmcg *pmcg;
>
> switch (node->type) {
> case ACPI_IORT_NODE_SMMU_V3:
> @@ -441,6 +442,10 @@ static int iort_get_id_mapping_index(struct acpi_iort_node *node)
>
> return smmu->id_mapping_index;
> case ACPI_IORT_NODE_PMCG:
> + pmcg = (struct acpi_iort_pmcg *)node->node_data;
> + if (pmcg->overflow_gsiv || node->mapping_count == 0)
> + return -EINVAL;
> +
> return 0;
> default:
> return -EINVAL;
> --
> 2.7.4
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox