* [PATCH V2 3/4] ARM: imx_v6_v7_defconfig: Enable CONFIG_IMX7ULP_WDT by default
From: Anson.Huang @ 2019-08-12 8:53 UTC (permalink / raw)
To: wim, linux, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
festevam, linux, otavio, leonard.crestez, u.kleine-koenig,
schnitzeltony, jan.tuerk, linux-watchdog, devicetree,
linux-arm-kernel, linux-kernel
Cc: Linux-imx
In-Reply-To: <20190812085321.13823-1-Anson.Huang@nxp.com>
From: Anson Huang <Anson.Huang@nxp.com>
Select CONFIG_IMX7ULP_WDT by default to support i.MX7ULP watchdog.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
arch/arm/configs/imx_v6_v7_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index bd2e2f5..f69075b 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -235,6 +235,7 @@ CONFIG_DA9062_WATCHDOG=y
CONFIG_DA9063_WATCHDOG=m
CONFIG_RN5T618_WATCHDOG=y
CONFIG_IMX2_WDT=y
+CONFIG_IMX7ULP_WDT=y
CONFIG_MFD_DA9052_I2C=y
CONFIG_MFD_DA9062=y
CONFIG_MFD_DA9063=y
--
2.7.4
^ permalink raw reply related
* [PATCH V2 2/4] watchdog: Add i.MX7ULP watchdog support
From: Anson.Huang @ 2019-08-12 8:53 UTC (permalink / raw)
To: wim, linux, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
festevam, linux, otavio, leonard.crestez, u.kleine-koenig,
schnitzeltony, jan.tuerk, linux-watchdog, devicetree,
linux-arm-kernel, linux-kernel
Cc: Linux-imx
In-Reply-To: <20190812085321.13823-1-Anson.Huang@nxp.com>
From: Anson Huang <Anson.Huang@nxp.com>
The i.MX7ULP Watchdog Timer (WDOG) module is an independent timer
that is available for system use.
It provides a safety feature to ensure that software is executing
as planned and that the CPU is not stuck in an infinite loop or
executing unintended code. If the WDOG module is not serviced
(refreshed) within a certain period, it resets the MCU.
Add driver support for i.MX7ULP watchdog.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Changes since V1:
- Add clock operation;
- Remove unneccsary error message when registering watchdog device failed;
- Use BIT() instead of hard code;
---
drivers/watchdog/Kconfig | 13 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/imx7ulp_wdt.c | 244 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 258 insertions(+)
create mode 100644 drivers/watchdog/imx7ulp_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 8188963..0884e53 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -740,6 +740,19 @@ config IMX_SC_WDT
To compile this driver as a module, choose M here: the
module will be called imx_sc_wdt.
+config IMX7ULP_WDT
+ tristate "IMX7ULP Watchdog"
+ depends on ARCH_MXC || COMPILE_TEST
+ select WATCHDOG_CORE
+ help
+ This is the driver for the hardware watchdog on the Freescale
+ IMX7ULP and later processors. If you have one of these
+ processors and wish to have watchdog support enabled,
+ say Y, otherwise say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called imx7ulp_wdt.
+
config UX500_WATCHDOG
tristate "ST-Ericsson Ux500 watchdog"
depends on MFD_DB8500_PRCMU
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 7caa920..7d32537 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o
+obj-$(CONFIG_IMX7ULP_WDT) += imx7ulp_wdt.o
obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
new file mode 100644
index 0000000..c20fba4
--- /dev/null
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -0,0 +1,244 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP.
+ */
+
+#include <linux/clk.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/watchdog.h>
+
+#define WDOG_CS 0x0
+#define WDOG_CS_CMD32EN BIT(13)
+#define WDOG_CS_ULK BIT(11)
+#define WDOG_CS_RCS BIT(10)
+#define WDOG_CS_EN BIT(7)
+#define WDOG_CS_UPDATE BIT(5)
+
+#define WDOG_CNT 0x4
+#define WDOG_TOVAL 0x8
+
+#define REFRESH_SEQ0 0xA602
+#define REFRESH_SEQ1 0xB480
+#define REFRESH ((REFRESH_SEQ1 << 16) | REFRESH_SEQ0)
+
+#define UNLOCK_SEQ0 0xC520
+#define UNLOCK_SEQ1 0xD928
+#define UNLOCK ((UNLOCK_SEQ1 << 16) | UNLOCK_SEQ0)
+
+#define DEFAULT_TIMEOUT 60
+#define MAX_TIMEOUT 128
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0000);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+struct imx7ulp_wdt_device {
+ struct notifier_block restart_handler;
+ struct watchdog_device wdd;
+ void __iomem *base;
+ struct clk *clk;
+ int rate;
+};
+
+static inline void imx7ulp_wdt_enable(void __iomem *base, bool enable)
+{
+ u32 val = readl(base + WDOG_CS);
+
+ writel(UNLOCK, base + WDOG_CNT);
+ if (enable)
+ writel(val | WDOG_CS_EN, base + WDOG_CS);
+ else
+ writel(val & ~WDOG_CS_EN, base + WDOG_CS);
+}
+
+static inline bool imx7ulp_wdt_is_enabled(void __iomem *base)
+{
+ u32 val = readl(base + WDOG_CS);
+
+ return val & WDOG_CS_EN;
+}
+
+static int imx7ulp_wdt_ping(struct watchdog_device *wdog)
+{
+ struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+
+ writel(REFRESH, wdt->base + WDOG_CNT);
+
+ return 0;
+}
+
+static int imx7ulp_wdt_start(struct watchdog_device *wdog)
+{
+ struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+
+ imx7ulp_wdt_enable(wdt->base, true);
+
+ return 0;
+}
+
+static int imx7ulp_wdt_stop(struct watchdog_device *wdog)
+{
+ struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+
+ imx7ulp_wdt_enable(wdt->base, false);
+
+ return 0;
+}
+
+static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
+ unsigned int timeout)
+{
+ struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+ u32 val = wdt->rate * timeout;
+
+ writel(UNLOCK, wdt->base + WDOG_CNT);
+ writel(val, wdt->base + WDOG_TOVAL);
+
+ wdog->timeout = timeout;
+
+ return 0;
+}
+
+static const struct watchdog_ops imx7ulp_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = imx7ulp_wdt_start,
+ .stop = imx7ulp_wdt_stop,
+ .ping = imx7ulp_wdt_ping,
+ .set_timeout = imx7ulp_wdt_set_timeout,
+};
+
+static const struct watchdog_info imx7ulp_wdt_info = {
+ .identity = "i.MX7ULP watchdog timer",
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
+ WDIOF_MAGICCLOSE,
+};
+
+static inline void imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
+{
+ u32 val;
+
+ /* unlock the wdog for reconfiguration */
+ writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
+ writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
+
+ /* set an initial timeout value in TOVAL */
+ writel(timeout, base + WDOG_TOVAL);
+ /* enable 32bit command sequence and reconfigure */
+ val = BIT(13) | BIT(8) | BIT(5);
+ writel(val, base + WDOG_CS);
+}
+
+static int imx7ulp_wdt_probe(struct platform_device *pdev)
+{
+ struct imx7ulp_wdt_device *imx7ulp_wdt;
+ struct device *dev = &pdev->dev;
+ struct watchdog_device *wdog;
+ int ret;
+
+ imx7ulp_wdt = devm_kzalloc(dev, sizeof(*imx7ulp_wdt), GFP_KERNEL);
+ if (!imx7ulp_wdt)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, imx7ulp_wdt);
+
+ imx7ulp_wdt->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(imx7ulp_wdt->base))
+ return PTR_ERR(imx7ulp_wdt->base);
+
+ imx7ulp_wdt->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(imx7ulp_wdt->clk)) {
+ dev_err(dev, "Failed to get watchdog clock\n");
+ return PTR_ERR(imx7ulp_wdt->clk);
+ }
+
+ ret = clk_prepare_enable(imx7ulp_wdt->clk);
+ if (ret)
+ return ret;
+
+ imx7ulp_wdt->rate = 1000;
+ wdog = &imx7ulp_wdt->wdd;
+ wdog->info = &imx7ulp_wdt_info;
+ wdog->ops = &imx7ulp_wdt_ops;
+ wdog->min_timeout = 1;
+ wdog->max_timeout = MAX_TIMEOUT;
+ wdog->parent = dev;
+ wdog->timeout = DEFAULT_TIMEOUT;
+
+ watchdog_init_timeout(wdog, 0, dev);
+ watchdog_stop_on_reboot(wdog);
+ watchdog_stop_on_unregister(wdog);
+ watchdog_set_drvdata(wdog, imx7ulp_wdt);
+ imx7ulp_wdt_init(imx7ulp_wdt->base, wdog->timeout * imx7ulp_wdt->rate);
+
+ ret = devm_watchdog_register_device(dev, wdog);
+ if (ret)
+ goto disable_clk;
+
+ return 0;
+
+disable_clk:
+ clk_disable_unprepare(imx7ulp_wdt->clk);
+
+ return ret;
+}
+
+static int __maybe_unused imx7ulp_wdt_suspend(struct device *dev)
+{
+ struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
+
+ if (watchdog_active(&imx7ulp_wdt->wdd))
+ imx7ulp_wdt_stop(&imx7ulp_wdt->wdd);
+
+ clk_disable_unprepare(imx7ulp_wdt->clk);
+
+ return 0;
+}
+
+static int __maybe_unused imx7ulp_wdt_resume(struct device *dev)
+{
+ struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
+ u32 timeout = imx7ulp_wdt->wdd.timeout * imx7ulp_wdt->rate;
+ int ret;
+
+ ret = clk_prepare_enable(imx7ulp_wdt->clk);
+ if (ret)
+ return ret;
+
+ if (imx7ulp_wdt_is_enabled(imx7ulp_wdt->base))
+ imx7ulp_wdt_init(imx7ulp_wdt->base, timeout);
+
+ if (watchdog_active(&imx7ulp_wdt->wdd))
+ imx7ulp_wdt_start(&imx7ulp_wdt->wdd);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(imx7ulp_wdt_pm_ops, imx7ulp_wdt_suspend,
+ imx7ulp_wdt_resume);
+
+static const struct of_device_id imx7ulp_wdt_dt_ids[] = {
+ { .compatible = "fsl,imx7ulp-wdt", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx7ulp_wdt_dt_ids);
+
+static struct platform_driver imx7ulp_wdt_driver = {
+ .probe = imx7ulp_wdt_probe,
+ .driver = {
+ .name = "imx7ulp-wdt",
+ .pm = &imx7ulp_wdt_pm_ops,
+ .of_match_table = imx7ulp_wdt_dt_ids,
+ },
+};
+module_platform_driver(imx7ulp_wdt_driver);
+
+MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
+MODULE_DESCRIPTION("Freescale i.MX7ULP watchdog driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
^ permalink raw reply related
* [PATCH V2 1/4] dt-bindings: watchdog: Add i.MX7ULP bindings
From: Anson.Huang @ 2019-08-12 8:53 UTC (permalink / raw)
To: wim, linux, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
festevam, linux, otavio, leonard.crestez, u.kleine-koenig,
schnitzeltony, jan.tuerk, linux-watchdog, devicetree,
linux-arm-kernel, linux-kernel
Cc: Linux-imx
From: Anson Huang <Anson.Huang@nxp.com>
Add the watchdog bindings for Freescale i.MX7ULP.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
.../bindings/watchdog/fsl-imx7ulp-wdt.txt | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
new file mode 100644
index 0000000..d83fc5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
@@ -0,0 +1,22 @@
+* Freescale i.MX7ULP Watchdog Timer (WDT) Controller
+
+Required properties:
+- compatible : Should be "fsl,imx7ulp-wdt"
+- reg : Should contain WDT registers location and length
+- interrupts : Should contain WDT interrupt
+- clocks: Should contain a phandle pointing to the gated peripheral clock.
+
+Optional properties:
+- timeout-sec : Contains the watchdog timeout in seconds
+
+Examples:
+
+wdog1: wdog@403d0000 {
+ compatible = "fsl,imx7ulp-wdt";
+ reg = <0x403d0000 0x10000>;
+ interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&pcc2 IMX7ULP_CLK_WDG1>;
+ assigned-clocks = <&pcc2 IMX7ULP_CLK_WDG1>;
+ assigned-clocks-parents = <&scg1 IMX7ULP_CLK_FIRC_BUS_CLK>;
+ timeout-sec = <40>;
+};
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] ARM: dts: imx7d: sbc-iot-imx7: add recovery for i2c3/4
From: Philippe Schenker @ 2019-08-12 8:50 UTC (permalink / raw)
To: git@andred.net
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
Max Krummenacher, Marcel Ziswiler, festevam@gmail.com,
s.hauer@pengutronix.de, linux-kernel@vger.kernel.org,
Oleksandr Suvorov, robh+dt@kernel.org, linux-imx@nxp.com,
kernel@pengutronix.de, shawnguo@kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190810215817.5118-1-git@andred.net>
On Sat, 2019-08-10 at 22:58 +0100, André Draszik wrote:
> On Wed, 07 Aug 2019 08:26:15 +0000, Philippe Schenker wrote:
> > From: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> >
> > - add recovery mode for applicable i2c buses for
> > Colibri iMX7 module.
> >
> > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> > ---
> >
> > Changes in v3: None
> > Changes in v2: None
> >
> > arch/arm/boot/dts/imx7-colibri.dtsi | 25 +++++++++++++++++++++++--
> > 1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/imx7-colibri.dtsi
> > b/arch/arm/boot/dts/imx7-colibri.dtsi
> > index a8d992f3e897..2480623c92ff 100644
> > --- a/arch/arm/boot/dts/imx7-colibri.dtsi
> > +++ b/arch/arm/boot/dts/imx7-colibri.dtsi
> > @@ -140,8 +140,12 @@
> >
> > &i2c1 {
> > clock-frequency = <100000>;
> > - pinctrl-names = "default";
> > + pinctrl-names = "default", "gpio";
> > pinctrl-0 = <&pinctrl_i2c1 &pinctrl_i2c1_int>;
> > + pinctrl-1 = <&pinctrl_i2c1_recovery &pinctrl_i2c1_int>;
> > + scl-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
>
> scl-gpios should be (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN) since
> commit d2d0ad2aec4a ("i2c: imx: use open drain for recovery GPIO")
>
> Otherwise you'll get a boot-time warning:
> enforced open drain please flag it properly in DT/ACPI DSDT/board
> file
Thanks for pointing this out, I added this for v4.
Philippe
>
> > + sda-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>;
> > +
> > status = "okay";
> >
> > codec: sgtl5000@a {
> > @@ -242,8 +246,11 @@
> >
> > &i2c4 {
> > clock-frequency = <100000>;
> > - pinctrl-names = "default";
> > + pinctrl-names = "default", "gpio";
> > pinctrl-0 = <&pinctrl_i2c4>;
> > + pinctrl-1 = <&pinctrl_i2c4_recovery>;
> > + scl-gpios = <&gpio7 8 GPIO_ACTIVE_HIGH>;
>
> and here, too.
>
> Cheers,
> André
_______________________________________________
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 v5 15/18] thermal: sun8i: allow to use custom temperature calculation function
From: Maxime Ripard @ 2019-08-12 8:49 UTC (permalink / raw)
To: Yangtao Li
Cc: rui.zhang, edubezval, daniel.lezcano, robh+dt, mark.rutland, wens,
mchehab+samsung, davem, gregkh, Jonathan.Cameron, nicolas.ferre,
linux-pm, devicetree, linux-arm-kernel, linux-kernel,
Icenowy Zheng
In-Reply-To: <20190810052829.6032-16-tiny.windzz@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2076 bytes --]
Hi,
On Sat, Aug 10, 2019 at 05:28:26AM +0000, Yangtao Li wrote:
> From: Icenowy Zheng <icenowy@aosc.io>
>
> The H5 temperature calculation function is strange. Firstly, it's
> segmented. Secondly, the formula of two sensors are different in the
> second segment.
>
> Allow to use a custom temperature calculation function, in case of
> the function is complex.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
When you send a patch on someone else's behalf, you need to put your
Signed-off-by as well.
> ---
> drivers/thermal/sun8i_thermal.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> index 3259081da841..a761e2afda08 100644
> --- a/drivers/thermal/sun8i_thermal.c
> +++ b/drivers/thermal/sun8i_thermal.c
> @@ -76,6 +76,7 @@ struct ths_thermal_chip {
> u16 *caldata, int callen);
> int (*init)(struct ths_device *tmdev);
> int (*irq_ack)(struct ths_device *tmdev);
> + int (*calc_temp)(int id, int reg);
> };
>
> struct ths_device {
> @@ -90,9 +91,12 @@ struct ths_device {
>
> /* Temp Unit: millidegree Celsius */
> static int sun8i_ths_reg2temp(struct ths_device *tmdev,
> - int reg)
> + int id, int reg)
> {
> - return (reg + tmdev->chip->offset) * tmdev->chip->scale;
> + if (tmdev->chip->calc_temp)
> + return tmdev->chip->calc_temp(id, reg);
> + else
> + return (reg + tmdev->chip->offset) * tmdev->chip->scale;
You're not consistent here compared to the other callbacks you have
introduced: calibrate, init and irq_ack all need to be set and will
fail (hard) if you don't set them, yet this one will have a different
behaviour (that behaviour being to use the H6 formula, which is the
latest SoC, which is a bit odd in itself).
I guess we should either make it mandatory as the rest of the
callbacks, or document which callbacks are mandatory and which are
optional (and the behaviour when it's optional).
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] dt-bingings: PCI: Remove the num-lanes from Required properties
From: Andrew Murray @ 2019-08-12 8:45 UTC (permalink / raw)
To: Z.q. Hou
Cc: linux-pci@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, gustavo.pimentel@synopsys.com,
jingoohan1@gmail.com, bhelgaas@google.com, robh+dt@kernel.org,
mark.rutland@arm.com, shawnguo@kernel.org, Leo Li,
lorenzo.pieralisi@arm.com, M.h. Lian, Kishon Vijay Abraham I,
Gabriele Paoloni
In-Reply-To: <20190812042435.25102-2-Zhiqiang.Hou@nxp.com>
On Mon, Aug 12, 2019 at 04:22:16AM +0000, Z.q. Hou wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>
> The num-lanes is not a mandatory property, e.g. on FSL
> Layerscape SoCs, the PCIe link training is completed
> automatically base on the selected SerDes protocol, it
> doesn't need the num-lanes to set-up the link width.
>
> It has been added in the Optional properties. This
> patch is to remove it from the Required properties.
For clarity, maybe this paragraph can be reworded to:
"It is previously in both Required and Optional properties,
let's remove it from the Required properties".
I don't understand why this property is previously in
both required and optional...
It looks like num-lanes was first made optional back in
2015 and removed from the Required section (907fce090253).
But then re-added back into the Required section in 2017
with the adition of bindings for EP mode (b12befecd7de).
Is num-lanes actually required for EP mode?
Thanks,
Andrew Murray
>
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> ---
> Documentation/devicetree/bindings/pci/designware-pcie.txt | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index 5561a1c060d0..bd880df39a79 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -11,7 +11,6 @@ Required properties:
> the ATU address space.
> (The old way of getting the configuration address space from "ranges"
> is deprecated and should be avoided.)
> -- num-lanes: number of lanes to use
> RC mode:
> - #address-cells: set to <3>
> - #size-cells: set to <2>
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v6 2/7] drm/mediatek: fixes CMDQ reg address of mt8173 is different with mt2701
From: CK Hu @ 2019-08-12 8:40 UTC (permalink / raw)
To: Jitao Shi
Cc: Mark Rutland, devicetree, David Airlie, stonea168, dri-devel,
yingjoe.chen, Ajay Kumar, Vincent Palatin, cawa.cheng,
Russell King, Thierry Reding, linux-pwm, Sascha Hauer, Pawel Moll,
Ian Campbell, Rob Herring, linux-mediatek, Andy Yan,
Matthias Brugger, eddie.huang, linux-arm-kernel, Rahul Sharma,
srv_heupstream, linux-kernel, Sean Paul
In-Reply-To: <20190811104008.53372-3-jitao.shi@mediatek.com>
Hi, Jitao:
On Sun, 2019-08-11 at 18:40 +0800, Jitao Shi wrote:
> Config the different CMDQ reg address in driver data.
>
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 29 ++++++++++++++++++++++++-----
> 1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 52b49daeed9f..ac8e80e379f7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -123,7 +123,6 @@
> #define VM_CMD_EN BIT(0)
> #define TS_VFP_EN BIT(5)
>
> -#define DSI_CMDQ0 0x180
> #define CONFIG (0xff << 0)
> #define SHORT_PACKET 0
> #define LONG_PACKET 2
> @@ -148,6 +147,10 @@
>
> struct phy;
>
> +struct mtk_dsi_driver_data {
> + const u32 reg_cmdq_off;
> +};
> +
> struct mtk_dsi {
> struct mtk_ddp_comp ddp_comp;
> struct device *dev;
> @@ -174,6 +177,7 @@ struct mtk_dsi {
> bool enabled;
> u32 irq_data;
> wait_queue_head_t irq_wait_queue;
> + const struct mtk_dsi_driver_data *driver_data;
> };
>
> static inline struct mtk_dsi *encoder_to_dsi(struct drm_encoder *e)
> @@ -936,6 +940,7 @@ static void mtk_dsi_cmdq(struct mtk_dsi *dsi, const struct mipi_dsi_msg *msg)
> const char *tx_buf = msg->tx_buf;
> u8 config, cmdq_size, cmdq_off, type = msg->type;
> u32 reg_val, cmdq_mask, i;
> + u32 reg_cmdq_off = dsi->driver_data->reg_cmdq_off;
>
> if (MTK_DSI_HOST_IS_READ(type))
> config = BTA;
> @@ -955,9 +960,11 @@ static void mtk_dsi_cmdq(struct mtk_dsi *dsi, const struct mipi_dsi_msg *msg)
> }
>
> for (i = 0; i < msg->tx_len; i++)
> - writeb(tx_buf[i], dsi->regs + DSI_CMDQ0 + cmdq_off + i);
> + mtk_dsi_mask(dsi, (reg_cmdq_off + cmdq_off + i) & (~0x3U),
> + (0xffUL << (((i + cmdq_off) & 3U) * 8U)),
> + tx_buf[i] << (((i + cmdq_off) & 3U) * 8U));
If writeb() has the same problem in MT2701, I think we need a patch that
just change writeb() to mtk_dsi_mask(), and then a patch to fix CMDQ reg
address of MT8173. So break this patch into two patches.
Regards,
CK
>
> - mtk_dsi_mask(dsi, DSI_CMDQ0, cmdq_mask, reg_val);
> + mtk_dsi_mask(dsi, reg_cmdq_off, cmdq_mask, reg_val);
> mtk_dsi_mask(dsi, DSI_CMDQ_SIZE, CMDQ_SIZE, cmdq_size);
> }
>
> @@ -1101,6 +1108,8 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> if (ret)
> goto err_unregister_host;
>
> + dsi->driver_data = of_device_get_match_data(dev);
> +
> dsi->engine_clk = devm_clk_get(dev, "engine");
> if (IS_ERR(dsi->engine_clk)) {
> ret = PTR_ERR(dsi->engine_clk);
> @@ -1194,9 +1203,19 @@ static int mtk_dsi_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static const struct mtk_dsi_driver_data mt8173_dsi_driver_data = {
> + .reg_cmdq_off = 0x200,
> +};
> +
> +static const struct mtk_dsi_driver_data mt2701_dsi_driver_data = {
> + .reg_cmdq_off = 0x180,
> +};
> +
> static const struct of_device_id mtk_dsi_of_match[] = {
> - { .compatible = "mediatek,mt2701-dsi" },
> - { .compatible = "mediatek,mt8173-dsi" },
> + { .compatible = "mediatek,mt2701-dsi",
> + .data = &mt2701_dsi_driver_data },
> + { .compatible = "mediatek,mt8173-dsi",
> + .data = &mt8173_dsi_driver_data },
> { },
> };
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH 2/2] pwm: sprd: Add Spreadtrum PWM support
From: Uwe Kleine-König @ 2019-08-12 8:35 UTC (permalink / raw)
To: Baolin Wang
Cc: Thierry Reding, Rob Herring, Mark Rutland, Orson Zhai,
Chunyan Zhang, Vincent Guittot, linux-pwm, DTML, LKML, kernel
In-Reply-To: <CAMz4ku+o6dCyxhR3-5yM+zr2nBpTQG5A8Pbnxpo7yRciwPbv3Q@mail.gmail.com>
Hello Baolin,
On Mon, Aug 12, 2019 at 03:29:07PM +0800, Baolin Wang wrote:
> Hi Uwe,
>
> On Fri, 9 Aug 2019 at 22:41, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Aug 09, 2019 at 06:06:21PM +0800, Baolin Wang wrote:
> > > On Fri, 9 Aug 2019 at 17:10, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Thu, Aug 08, 2019 at 04:59:39PM +0800, Baolin Wang wrote:
> > > > > +static void sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > + struct pwm_state *state)
> > > > > +{
> > > > > + struct sprd_pwm_chip *spc =
> > > > > + container_of(chip, struct sprd_pwm_chip, chip);
> > > > > + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > > > + u32 enabled, duty, prescale;
> > > > > + u64 tmp;
> > > > > + int ret;
> > > > > +
> > > > > + ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
> > > > > + if (ret) {
> > > > > + dev_err(spc->dev, "failed to enable pwm%u clocks\n",
> > > > > + pwm->hwpwm);
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + chn->clk_enabled = true;
> > > > > +
> > > > > + duty = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY) & SPRD_PWM_REG_MSK;
> > > > > + prescale = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE) & SPRD_PWM_REG_MSK;
> > > > > + enabled = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE) & SPRD_PWM_ENABLE_BIT;
> > > > > +
> > > > > + /*
> > > > > + * According to the datasheet, the period_ns and duty_ns calculation
> > > > > + * formula should be:
> > > > > + * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> > > > > + * duty_ns = 10^9 * (prescale + 1) * duty / clk_rate
> > > > > + */
> > > > > + tmp = (prescale + 1) * 1000000000ULL * SPRD_PWM_MOD_MAX;
> > > > > + state->period = div64_u64(tmp, chn->clk_rate);
> > > >
> > > > This is not idempotent. If you apply the configuration that is returned
> > > > here this shouldn't result in a reconfiguration.
> > >
> > > Since we may configure the PWM in bootloader, so in kernel part we
> > > should get current PWM state to avoid reconfiguration if state
> > > configuration are same.
> >
> > This is also important as some consumers might do something like:
> >
> > state = pwm_get_state(mypwm)
> > if (something):
> > state->duty = 0
> > else:
> > state->duty = state->period / 2
> > pwm_set_state(mypwm, state)
> >
> > and then period shouldn't get smaller in each step.
> > (This won't happen as of now because the PWM framework caches the last
> > state that was set and returns this for pwm_get_state. Still getting
> > this right would be good.)
>
> I understood your concern, but the period can be configured in
> bootloader, we have no software things to save the accurate period.
I don't understand what you're saying here. The bootloader configuring
the hardware is a usual use-case. That's why we have the .get_state
callback in the first place.
> Moreover I think I can change to use DIV_ROUND_CLOSET_ULL to keep the
> accuracy.
DIV_ROUND_CLOSEST_ULL still doesn't match what the apply callback uses.
With the lack of an official statement from the maintainer I'd prefer
.apply to round down and implement .get_state such that
pwm_apply(pwm, pwm_get_state(pwm))
is a no-op.
> > > > > +
> > > > > + dev_err(spc->dev, "failed to get channel clocks\n");
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + clk_pwm = chn->clks[1].clk;
> > > >
> > > > This 1 looks suspicious. Are you using all clocks provided in the dtb at
> > > > all? You're not using i in the loop at all, this doesn't look right.
> > >
> > > Like I said above, each channel has 2 clocks: enable clock and pwm
> > > clock, the 2nd clock of each channel's bulk clocks is the pwm clock,
> > > which is used to set the source clock. I know this's not easy to read,
> > > so do you have any good suggestion?
> >
> > Not sure this is easily possible to rework to make this clearer.
> >
> > Do these clks have different uses? e.g. one to enable register access
> > and the other to enable the pwm output? If so just using
>
> Yes.
So assuming one of the clocks is for operation of the output and the
other for accessing the registers, the latter can be disabled at the end
of each callback?
> > devm_clk_bulk_get isn't the right thing because you should be able know
> > if clks[0] or clks[1] is the one you need to enable the output (or
> > register access).
>
> We've fixed the clock order in bulk clocks by the array
> 'sprd_pwm_clks', maybe I should define one readable macro instead of
> magic number.
ack.
> > > > > + if (!clk_set_parent(clk_pwm, clk_parent))
> > > > > + chn->clk_rate = clk_get_rate(clk_pwm);
> > > > > + else
> > > > > + chn->clk_rate = SPRD_PWM_DEFAULT_CLK;
> > > >
> > > > I don't know all the clock framework details, but I think there are
> > > > better ways to ensure that a given clock is used as parent for another
> > > > given clock. Please read the chapter about "Assigned clock parents and
> > > > rates" in the clock bindings and check if this could be used for the
> > > > purpose here and so simplify the driver.
> > >
> > > Actually there are many other drivers set the parent clock like this,
> > > and we want a default clock if failed to set the parent clock.
> >
> > These might be older than the clk framework capabilities, or the
> > reviewers didn't pay attention to this detail; both shouldn't be a
> > reason to not make it better here.
>
> The clock framework supplies 'assigned-clocks' and
> 'assigned-clock-parents' properties to set parent, but for our case we
> still want to set a default clock rate if failed to set parent when
> met some abnormal things.
Without understanding the complete problem I'd say this is out of the
area the driver should care about.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 4/4] arm64: dts: fsl: Remove num-lanes property from PCIe nodes
From: Andrew Murray @ 2019-08-12 8:35 UTC (permalink / raw)
To: Z.q. Hou
Cc: linux-pci@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, gustavo.pimentel@synopsys.com,
jingoohan1@gmail.com, bhelgaas@google.com, robh+dt@kernel.org,
mark.rutland@arm.com, shawnguo@kernel.org, Leo Li,
lorenzo.pieralisi@arm.com, M.h. Lian
In-Reply-To: <20190812042435.25102-5-Zhiqiang.Hou@nxp.com>
On Mon, Aug 12, 2019 at 04:22:33AM +0000, Z.q. Hou wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>
> On FSL Layerscape SoCs, the number of lanes assigned to PCIe
> controller is not fixed, it is determined by the selected
> SerDes protocol in the RCW (Reset Configuration Word), and
> the PCIe link training is completed automatically base on
> the selected SerDes protocol, and the link width set-up is
> updated by hardware. So the num-lanes is not needed to
> specify the link width.
>
> The current num-lanes indicates the max lanes PCIe controller
> can support up to, instead of the lanes assigned to the PCIe
> controller. This can result in PCIe link training fail after
> hot-reset. So remove the num-lanes to avoid set-up to incorrect
> link width.
>
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> ---
> arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi | 1 -
> arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi | 3 ---
> arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 6 ------
> arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 3 ---
> arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 4 ----
> 5 files changed, 17 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi
> index ec6257a5b251..119c597ca867 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi
> @@ -486,7 +486,6 @@
> #address-cells = <3>;
> #size-cells = <2>;
> device_type = "pci";
> - num-lanes = <4>;
> num-viewport = <2>;
> bus-range = <0x0 0xff>;
> ranges = <0x81000000 0x0 0x00000000 0x40 0x00010000 0x0 0x00010000 /* downstream I/O */
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> index 71d9ed9ff985..c084c7a4b6a6 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> @@ -677,7 +677,6 @@
> #size-cells = <2>;
> device_type = "pci";
> dma-coherent;
> - num-lanes = <4>;
> num-viewport = <6>;
> bus-range = <0x0 0xff>;
> ranges = <0x81000000 0x0 0x00000000 0x40 0x00010000 0x0 0x00010000 /* downstream I/O */
> @@ -704,7 +703,6 @@
> #size-cells = <2>;
> device_type = "pci";
> dma-coherent;
> - num-lanes = <2>;
> num-viewport = <6>;
> bus-range = <0x0 0xff>;
> ranges = <0x81000000 0x0 0x00000000 0x48 0x00010000 0x0 0x00010000 /* downstream I/O */
> @@ -731,7 +729,6 @@
> #size-cells = <2>;
> device_type = "pci";
> dma-coherent;
> - num-lanes = <2>;
> num-viewport = <6>;
> bus-range = <0x0 0xff>;
> ranges = <0x81000000 0x0 0x00000000 0x50 0x00010000 0x0 0x00010000 /* downstream I/O */
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
> index b0ef08b090dd..d4c1da3d4bde 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi
> @@ -649,7 +649,6 @@
> #size-cells = <2>;
> device_type = "pci";
> dma-coherent;
> - num-lanes = <4>;
> num-viewport = <8>;
> bus-range = <0x0 0xff>;
> ranges = <0x81000000 0x0 0x00000000 0x40 0x00010000 0x0 0x00010000 /* downstream I/O */
> @@ -671,7 +670,6 @@
> reg-names = "regs", "addr_space";
> num-ib-windows = <6>;
> num-ob-windows = <8>;
> - num-lanes = <2>;
> status = "disabled";
> };
>
> @@ -687,7 +685,6 @@
> #size-cells = <2>;
> device_type = "pci";
> dma-coherent;
> - num-lanes = <2>;
> num-viewport = <8>;
> bus-range = <0x0 0xff>;
> ranges = <0x81000000 0x0 0x00000000 0x48 0x00010000 0x0 0x00010000 /* downstream I/O */
> @@ -709,7 +706,6 @@
> reg-names = "regs", "addr_space";
> num-ib-windows = <6>;
> num-ob-windows = <8>;
> - num-lanes = <2>;
> status = "disabled";
> };
>
> @@ -725,7 +721,6 @@
> #size-cells = <2>;
> device_type = "pci";
> dma-coherent;
> - num-lanes = <2>;
> num-viewport = <8>;
> bus-range = <0x0 0xff>;
> ranges = <0x81000000 0x0 0x00000000 0x50 0x00010000 0x0 0x00010000 /* downstream I/O */
> @@ -747,7 +742,6 @@
> reg-names = "regs", "addr_space";
> num-ib-windows = <6>;
> num-ob-windows = <8>;
> - num-lanes = <2>;
> status = "disabled";
> };
>
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
> index dfbead405783..76c87afeba1e 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
> @@ -456,7 +456,6 @@
> #size-cells = <2>;
> device_type = "pci";
> dma-coherent;
> - num-lanes = <4>;
> num-viewport = <256>;
> bus-range = <0x0 0xff>;
> ranges = <0x81000000 0x0 0x00000000 0x20 0x00010000 0x0 0x00010000 /* downstream I/O */
> @@ -482,7 +481,6 @@
> #size-cells = <2>;
> device_type = "pci";
> dma-coherent;
> - num-lanes = <4>;
> num-viewport = <6>;
> bus-range = <0x0 0xff>;
> ranges = <0x81000000 0x0 0x00000000 0x28 0x00010000 0x0 0x00010000 /* downstream I/O */
> @@ -508,7 +506,6 @@
> #size-cells = <2>;
> device_type = "pci";
> dma-coherent;
> - num-lanes = <8>;
> num-viewport = <6>;
> bus-range = <0x0 0xff>;
> ranges = <0x81000000 0x0 0x00000000 0x30 0x00010000 0x0 0x00010000 /* downstream I/O */
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
> index 64101c9962ce..7a0be8eaa84a 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
> @@ -639,7 +639,6 @@
> #size-cells = <2>;
> device_type = "pci";
> dma-coherent;
> - num-lanes = <4>;
> num-viewport = <6>;
> bus-range = <0x0 0xff>;
> msi-parent = <&its>;
> @@ -661,7 +660,6 @@
> #size-cells = <2>;
> device_type = "pci";
> dma-coherent;
> - num-lanes = <4>;
> num-viewport = <6>;
> bus-range = <0x0 0xff>;
> msi-parent = <&its>;
> @@ -683,7 +681,6 @@
> #size-cells = <2>;
> device_type = "pci";
> dma-coherent;
> - num-lanes = <8>;
> num-viewport = <256>;
> bus-range = <0x0 0xff>;
> msi-parent = <&its>;
> @@ -705,7 +702,6 @@
> #size-cells = <2>;
> device_type = "pci";
> dma-coherent;
> - num-lanes = <4>;
> num-viewport = <6>;
> bus-range = <0x0 0xff>;
> msi-parent = <&its>;
Reviewed-by: Andrew Murray <andrew.murray@arm.com>
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 3/4] ARM: dts: ls1021a: Remove num-lanes property from PCIe nodes
From: Andrew Murray @ 2019-08-12 8:35 UTC (permalink / raw)
To: Z.q. Hou
Cc: linux-pci@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, gustavo.pimentel@synopsys.com,
jingoohan1@gmail.com, bhelgaas@google.com, robh+dt@kernel.org,
mark.rutland@arm.com, shawnguo@kernel.org, Leo Li,
lorenzo.pieralisi@arm.com, M.h. Lian
In-Reply-To: <20190812042435.25102-4-Zhiqiang.Hou@nxp.com>
On Mon, Aug 12, 2019 at 04:22:27AM +0000, Z.q. Hou wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>
> On FSL Layerscape SoCs, the number of lanes assigned to PCIe
> controller is not fixed, it is determined by the selected
> SerDes protocol in the RCW (Reset Configuration Word), and
> the PCIe link training is completed automatically base on
> the selected SerDes protocol, and the link width set-up is
> updated by hardware. So the num-lanes is not needed to
> specify the link width.
>
> The current num-lanes indicates the max lanes PCIe controller
> can support up to, instead of the lanes assigned to the PCIe
> controller. This can result in PCIe link training fail after
> hot-reset. So remove the num-lanes to avoid set-up to incorrect
> link width.
>
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> ---
> arch/arm/boot/dts/ls1021a.dtsi | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
> index 464df4290ffc..2f6977ada447 100644
> --- a/arch/arm/boot/dts/ls1021a.dtsi
> +++ b/arch/arm/boot/dts/ls1021a.dtsi
> @@ -874,7 +874,6 @@
> #address-cells = <3>;
> #size-cells = <2>;
> device_type = "pci";
> - num-lanes = <4>;
> num-viewport = <6>;
> bus-range = <0x0 0xff>;
> ranges = <0x81000000 0x0 0x00000000 0x40 0x00010000 0x0 0x00010000 /* downstream I/O */
> @@ -899,7 +898,6 @@
> #address-cells = <3>;
> #size-cells = <2>;
> device_type = "pci";
> - num-lanes = <4>;
> num-viewport = <6>;
> bus-range = <0x0 0xff>;
> ranges = <0x81000000 0x0 0x00000000 0x48 0x00010000 0x0 0x00010000 /* downstream I/O */
Reviewed-by: Andrew Murray <andrew.murray@arm.com>
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 2/4] PCI: dwc: Return directly when num-lanes is not found
From: Andrew Murray @ 2019-08-12 8:34 UTC (permalink / raw)
To: Z.q. Hou
Cc: linux-pci@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, gustavo.pimentel@synopsys.com,
jingoohan1@gmail.com, bhelgaas@google.com, robh+dt@kernel.org,
mark.rutland@arm.com, shawnguo@kernel.org, Leo Li,
lorenzo.pieralisi@arm.com, M.h. Lian
In-Reply-To: <20190812042435.25102-3-Zhiqiang.Hou@nxp.com>
On Mon, Aug 12, 2019 at 04:22:22AM +0000, Z.q. Hou wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>
> The num-lanes is optional, so probably it isn't added
> on some platforms. The subsequent programming is base
> on the num-lanes, hence return when it is not found.
>
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> ---
> drivers/pci/controller/dwc/pcie-designware.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 7d25102c304c..0a89bfd1636e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -423,8 +423,10 @@ void dw_pcie_setup(struct dw_pcie *pci)
>
>
> ret = of_property_read_u32(np, "num-lanes", &lanes);
> - if (ret)
> - lanes = 0;
> + if (ret) {
> + dev_dbg(pci->dev, "property num-lanes isn't found\n");
> + return;
> + }
The existing code would assign a value of 0 to lanes when num-lanes isn't
specified, however this value isn't supported by the following switch
statement - thus we'd also print an error and return.
Therefore the only and subtle effect effect of this patch is to change
a dev_err to a dev_dbg when num-lanes isn't specified and avoid a read of
PCIE_PORT_LINK_CONTROL.
Given that num-lanes is described as optional this makes perfect sense.
Though the commit message/hunk does give the apperance that this provides
a more functional change.
Anyway:
Reviewed-by: Andrew Murray <andrew.murray@arm.com>
>
> /* Set the number of lanes */
> val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 2/6] irqchip: Add Realtek RTD129x intc driver
From: Aleix Roca Nonell @ 2019-08-12 8:26 UTC (permalink / raw)
To: Marc Zyngier
Cc: Andreas Färber, Rob Herring, Mark Rutland, Thomas Gleixner,
Jason Cooper, Matthias Brugger, linux-arm-kernel, devicetree,
linux-kernel
In-Reply-To: <5efa2ccb-9659-443c-7986-8ceb01aa64b9@arm.com>
Hi Mark and everyone! Sorry for the large delay, I'm doing this in my
free time, which is not that abundant. In this mail, I'm focusing only
on the largest change mentioned by Mark. I will answer the rest later.
On Mon, Jul 08, 2019 at 10:36:14AM +0100, Marc Zyngier wrote:
> On 07/07/2019 14:22, Aleix Roca Nonell wrote:
> > This driver adds support for the RTD1296 and RTD1295 interrupt
> > controller (intc). It is based on both the BPI-SINOVOIP project and
> > Andreas Färber's previous attempt to submit a similar driver.
> >
> > There is currently no publicly available datasheet on this SoC and the
> > exact behaviour of the registers controlling the intc remain uncertain.
> >
> > This driver controls two intcs: "iso" and "misc". Each intc has its own
> > Interrupt Enable Register (IER) and Interrupt Status Resgister (ISR).
>
> Register
>
> > However, not all "misc" intc irqs have the same offsets for both ISR and
> > IER. For this reason an ISR to IER offsets table is defined.
> >
> > The driver catches the IER value to reduce accesses to the table inside the
> > interrupt handler. Actually, the driver stores the ISR offsets of currently
> > enabled interrupts in a variable.
> >
> > Signed-off-by: Aleix Roca Nonell <kernelrocks@gmail.com>
>
> I expect Andreas and you to sort the attribution issue. I'm certainly
> not going to take this in if things are unclear.
>
> > ---
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/irq-rtd129x.c | 371 ++++++++++++++++++++++++++++++++++
> > 2 files changed, 372 insertions(+)
> > create mode 100644 drivers/irqchip/irq-rtd129x.c
> >
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 606a003a0000..0689c3956250 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -100,3 +100,4 @@ obj-$(CONFIG_MADERA_IRQ) += irq-madera.o
> > obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o
> > obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o
> > obj-$(CONFIG_TI_SCI_INTA_IRQCHIP) += irq-ti-sci-inta.o
> > +obj-$(CONFIG_ARCH_REALTEK) += irq-rtd129x.o
> > diff --git a/drivers/irqchip/irq-rtd129x.c b/drivers/irqchip/irq-rtd129x.c
> > new file mode 100644
> > index 000000000000..76358ca50f10
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-rtd129x.c
> > @@ -0,0 +1,371 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/irqchip.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/io.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/irqchip.h>
> > +#include <linux/bits.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +
> > +#define RTD129X_INTC_NR_IRQS 32
> > +#define DEV_NAME "RTD1296_INTC"
> > +
> > +/*
> > + * This interrupt controller (hereinafter intc) driver controls two intcs: "iso"
> > + * and "misc". Each intc has its own Interrupt Enable Register (IER) and
> > + * Interrupt Status Resgister (ISR). However, not all "misc" intc irqs have the
> > + * same offsets for both ISR and IER. For this reason an ISR to IER offsets
> > + * table is defined. Also, to reduce accesses to this table in the interrupt
> > + * handler, the driver stores the ISR offsets of currently enabled interrupts in
> > + * a variable.
> > + */
> > +
> > +enum misc_int_en {
> > + MISC_INT_FAIL = 0xFF,
> > + MISC_INT_RVD = 0xFE,
> > + MISC_INT_EN_FAN = 29,
> > + MISC_INT_EN_I2C3 = 28,
> > + MISC_INT_EN_GSPI = 27,
> > + MISC_INT_EN_I2C2 = 26,
> > + MISC_INT_EN_SC0 = 24,
> > + MISC_INT_EN_LSADC1 = 22,
> > + MISC_INT_EN_LSADC0 = 21,
> > + MISC_INT_EN_GPIODA = 20,
> > + MISC_INT_EN_GPIOA = 19,
> > + MISC_INT_EN_I2C4 = 15,
> > + MISC_INT_EN_I2C5 = 14,
> > + MISC_INT_EN_RTC_DATA = 12,
> > + MISC_INT_EN_RTC_HOUR = 11,
> > + MISC_INT_EN_RTC_MIN = 10,
> > + MISC_INT_EN_UR2 = 7,
> > + MISC_INT_EN_UR2_TO = 6,
> > + MISC_INT_EN_UR1_TO = 5,
> > + MISC_INT_EN_UR1 = 3,
> > +};
> > +
> > +enum iso_int_en {
> > + ISO_INT_FAIL = 0xFF,
> > + ISO_INT_RVD = 0xFE,
> > + ISO_INT_EN_I2C1_REQ = 31,
> > + ISO_INT_EN_GPHY_AV = 30,
> > + ISO_INT_EN_GPHY_DV = 29,
> > + ISO_INT_EN_GPIODA = 20,
> > + ISO_INT_EN_GPIOA = 19,
> > + ISO_INT_EN_RTC_ALARM = 13,
> > + ISO_INT_EN_RTC_HSEC = 12,
> > + ISO_INT_EN_I2C1 = 11,
> > + ISO_INT_EN_I2C0 = 8,
> > + ISO_INT_EN_IRDA = 5,
> > + ISO_INT_EN_UR0 = 2,
> > +};
> > +
> > +unsigned char rtd129x_intc_enable_map_misc[RTD129X_INTC_NR_IRQS] = {
> > + MISC_INT_FAIL, /* Bit0 */
> > + MISC_INT_FAIL, /* Bit1 */
> > + MISC_INT_RVD, /* Bit2 */
> > + MISC_INT_EN_UR1, /* Bit3 */
> > + MISC_INT_FAIL, /* Bit4 */
> > + MISC_INT_EN_UR1_TO, /* Bit5 */
> > + MISC_INT_RVD, /* Bit6 */
> > + MISC_INT_RVD, /* Bit7 */
> > + MISC_INT_EN_UR2, /* Bit8 */
> > + MISC_INT_RVD, /* Bit9 */
> > + MISC_INT_EN_RTC_MIN, /* Bit10 */
> > + MISC_INT_EN_RTC_HOUR, /* Bit11 */
> > + MISC_INT_EN_RTC_DATA, /* Bit12 */
> > + MISC_INT_EN_UR2_TO, /* Bit13 */
> > + MISC_INT_EN_I2C5, /* Bit14 */
> > + MISC_INT_EN_I2C4, /* Bit15 */
> > + MISC_INT_FAIL, /* Bit16 */
> > + MISC_INT_FAIL, /* Bit17 */
> > + MISC_INT_FAIL, /* Bit18 */
> > + MISC_INT_EN_GPIOA, /* Bit19 */
> > + MISC_INT_EN_GPIODA, /* Bit20 */
> > + MISC_INT_EN_LSADC0, /* Bit21 */
> > + MISC_INT_EN_LSADC1, /* Bit22 */
> > + MISC_INT_EN_I2C3, /* Bit23 */
> > + MISC_INT_EN_SC0, /* Bit24 */
> > + MISC_INT_FAIL, /* Bit25 */
> > + MISC_INT_EN_I2C2, /* Bit26 */
> > + MISC_INT_EN_GSPI, /* Bit27 */
> > + MISC_INT_FAIL, /* Bit28 */
> > + MISC_INT_EN_FAN, /* Bit29 */
> > + MISC_INT_FAIL, /* Bit30 */
> > + MISC_INT_FAIL /* Bit31 */
> > +};
> > +
> > +unsigned char rtd129x_intc_enable_map_iso[RTD129X_INTC_NR_IRQS] = {
> > + ISO_INT_FAIL, /* Bit0 */
> > + ISO_INT_RVD, /* Bit1 */
> > + ISO_INT_EN_UR0, /* Bit2 */
> > + ISO_INT_FAIL, /* Bit3 */
> > + ISO_INT_FAIL, /* Bit4 */
> > + ISO_INT_EN_IRDA, /* Bit5 */
> > + ISO_INT_FAIL, /* Bit6 */
> > + ISO_INT_RVD, /* Bit7 */
> > + ISO_INT_EN_I2C0, /* Bit8 */
> > + ISO_INT_RVD, /* Bit9 */
> > + ISO_INT_FAIL, /* Bit10 */
> > + ISO_INT_EN_I2C1, /* Bit11 */
> > + ISO_INT_EN_RTC_HSEC, /* Bit12 */
> > + ISO_INT_EN_RTC_ALARM, /* Bit13 */
> > + ISO_INT_FAIL, /* Bit14 */
> > + ISO_INT_FAIL, /* Bit15 */
> > + ISO_INT_FAIL, /* Bit16 */
> > + ISO_INT_FAIL, /* Bit17 */
> > + ISO_INT_FAIL, /* Bit18 */
> > + ISO_INT_EN_GPIOA, /* Bit19 */
> > + ISO_INT_EN_GPIODA, /* Bit20 */
> > + ISO_INT_RVD, /* Bit21 */
> > + ISO_INT_RVD, /* Bit22 */
> > + ISO_INT_RVD, /* Bit23 */
> > + ISO_INT_RVD, /* Bit24 */
> > + ISO_INT_FAIL, /* Bit25 */
> > + ISO_INT_FAIL, /* Bit26 */
> > + ISO_INT_FAIL, /* Bit27 */
> > + ISO_INT_FAIL, /* Bit28 */
> > + ISO_INT_EN_GPHY_DV, /* Bit29 */
> > + ISO_INT_EN_GPHY_AV, /* Bit30 */
> > + ISO_INT_EN_I2C1_REQ /* Bit31 */
> > +};
> > +
> > +struct rtd129x_intc_data {
> > + void __iomem *unmask;
> > + void __iomem *isr;
> > + void __iomem *ier;
> > + u32 ier_cached;
> > + u32 isr_en;
> > + raw_spinlock_t lock;
> > + unsigned int parent_irq;
> > + const unsigned char *en_map;
> > +};
> > +
> > +static struct irq_domain *rtd129x_intc_domain;
> > +
> > +static void rtd129x_intc_irq_handle(struct irq_desc *desc)
> > +{
> > + struct rtd129x_intc_data *priv = irq_desc_get_handler_data(desc);
> > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > + unsigned int local_irq;
> > + u32 status;
> > + int i;
> > +
> > + chained_irq_enter(chip, desc);
> > +
> > + raw_spin_lock(&priv->lock);
> > + status = readl_relaxed(priv->isr);
> > + status &= priv->isr_en;
> > + raw_spin_unlock(&priv->lock);
>
> What is this lock protecting? isr_en?
>
> > +
> > + while (status) {
> > + i = __ffs(status);
> > + status &= ~BIT(i);
> > +
> > + local_irq = irq_find_mapping(rtd129x_intc_domain, i);
> > + if (likely(local_irq)) {
> > + if (!generic_handle_irq(local_irq))
> > + writel_relaxed(BIT(i), priv->isr);
>
> What are the write semantics of the ISR register? Hot bit clear? How
> does it work since mask() does the same thing? Clearly, something is
> wrong here.
Sorry but I have not been able to found the definition of "hot bit
clear", could you explain it? Anyways, you were right, apparently the
mask/unmask code were doing nothing useful. More on this below.
>
> > + } else {
> > + handle_bad_irq(desc);
> > + }
> > + }
> > +
> > + chained_irq_exit(chip, desc);
> > +}
> > +
> > +static void rtd129x_intc_mask(struct irq_data *data)
> > +{
> > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> > +
> > + writel_relaxed(BIT(data->hwirq), priv->isr);
> > +}
> > +
> > +static void rtd129x_intc_unmask(struct irq_data *data)
> > +{
> > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> > +
> > + writel_relaxed(BIT(data->hwirq), priv->unmask);
>
> What effect does this have on the isr register? The whole mask/unmask
> thing seems to be pretty dodgy...
>
> > +}
> > +
> > +static void rtd129x_intc_enable(struct irq_data *data)
> > +{
> > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> > + unsigned long flags;
> > + u8 en_offset;
> > +
> > + en_offset = priv->en_map[data->hwirq];
> > +
> > + if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) {
> > + raw_spin_lock_irqsave(&priv->lock, flags);
> > +
> > + priv->isr_en |= BIT(data->hwirq);
> > + priv->ier_cached |= BIT(en_offset);
> > + writel_relaxed(priv->ier_cached, priv->ier);
> > +
> > + raw_spin_unlock_irqrestore(&priv->lock, flags);
> > + } else if (en_offset == MISC_INT_FAIL) {
> > + pr_err("[%s] Enable irq(%lu) failed\n", DEV_NAME, data->hwirq);
> > + }
> > +}
> > +
> > +static void rtd129x_intc_disable(struct irq_data *data)
> > +{
> > + struct rtd129x_intc_data *priv = irq_data_get_irq_chip_data(data);
> > + unsigned long flags;
> > + u8 en_offset;
> > +
> > + en_offset = priv->en_map[data->hwirq];
> > +
> > + if ((en_offset != MISC_INT_RVD) && (en_offset != MISC_INT_FAIL)) {
> > + raw_spin_lock_irqsave(&priv->lock, flags);
> > +
> > + priv->isr_en &= ~BIT(data->hwirq);
> > + priv->ier_cached &= ~BIT(en_offset);
> > + writel_relaxed(priv->ier_cached, priv->ier);
> > +
> > + raw_spin_unlock_irqrestore(&priv->lock, flags);
> > + } else if (en_offset == MISC_INT_FAIL) {
> > + pr_err("[%s] Disable irq(%lu) failed\n", DEV_NAME, data->hwirq);
> > + }
> > +}
>
> So here's a thought: Why do we need all of this? If mask/unmask do their
> job correctly, we could just enable all interrupts in one go (just a
> 32bit write) at probe time, and leave all interrupts masked until they
> are in use. You could then drop all these silly tables that don't bring
> much...
The idea of dropping all those tables look really good to me, that
would greatly simplify the code! I have been trying to mask all
interrupts on the probe function using the ISR register but while
doing so, I realized that it does not work. Writing to ISR does not
mask interrupts, apparently it only acknowledges them once they have
been triggered. On the scarse available documentation of this Soc I
cannot find a mask-like register. It seems interrupts are managed with
an ISR and an IER register. So it should be posible to use the enable
register to maks/unmask instead. These do work. However, that would
mean that we have to keep those ugly tables.
Nonetheless we might still be able to do something else. Please,
correct me if I'm wrong, but do we really need to mask/unamsk in this
scenario? This is the devised board layout:
+------+ +----------+ +---------+
| | | | | |
| UART |-------|2 INTC |-------|c GIC |
| | +----|1 | +----|b |
+------+ | +--|0 | | +--|a |
| | | | | | | |
| | +----------+ | | +---------+
| |
Once the UART generates an interrupt it passes through the line 2 of
the custom realtek interrupt contoller before reaching the GIC's line
"c". On the INTC interrupt handler, we call chained_irq_enter/exit
to mask/unmask the GIC's "c" line. Because all of this realtek INTC
interrupt lines (2,1,0,...) are muxed on the GIC's line "c", this
means that while on the INTC interrupt handler it is not possible to
send further interrupts on the CPU. Given that interrupts are masked
on the GIC, it seems safe to just remove INTC's mask/unmask functions.
Therefore, the only work that this INTC handler would needs to do is
to acknowledge the interrupt by writing to the ISR, which it could be
done in the respective irq_ack callback of struct irq_chip instead of
in the handler body.
I have implemented this solution and it seems to work. What do you
think? I'm missing something crucial?
>
> > +
> > +static struct irq_chip rtd129x_intc_chip = {
> > + .name = DEV_NAME,
> > + .irq_mask = rtd129x_intc_mask,
> > + .irq_unmask = rtd129x_intc_unmask,
> > + .irq_enable = rtd129x_intc_enable,
> > + .irq_disable = rtd129x_intc_disable,
> > +};
> > +
> > +static int rtd129x_intc_map(struct irq_domain *d, unsigned int virq,
> > + irq_hw_number_t hw_irq)
> > +{
> > + struct rtd129x_intc_data *priv = d->host_data;
> > +
> > + irq_set_chip_and_handler(virq, &rtd129x_intc_chip, handle_level_irq);
> > + irq_set_chip_data(virq, priv);
> > + irq_set_probe(virq);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct irq_domain_ops rtd129x_intc_domain_ops = {
> > + .xlate = irq_domain_xlate_onecell,
> > + .map = rtd129x_intc_map,
> > +};
> > +
> > +static const struct of_device_id rtd129x_intc_matches[] = {
> > + { .compatible = "realtek,rtd129x-intc-misc",
> > + .data = rtd129x_intc_enable_map_misc
> > + },
> > + { .compatible = "realtek,rtd129x-intc-iso",
> > + .data = rtd129x_intc_enable_map_iso
> > + },
> > + { }
> > +};
> > +
> > +static int rtd129x_intc_of_init(struct device_node *node,
> > + struct device_node *parent)
> > +{
> > + struct rtd129x_intc_data *priv;
> > + const struct of_device_id *match;
> > + u32 isr_tmp, ier_tmp, ier_bit;
> > + int ret, i;
> > +
> > + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > + if (!priv) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > +
> > + raw_spin_lock_init(&priv->lock);
> > +
> > + priv->isr = of_iomap(node, 0);
> > + if (!priv->isr) {
> > + pr_err("unable to obtain status reg iomap address\n");
> > + ret = -ENOMEM;
> > + goto free_priv;
> > + }
> > +
> > + priv->ier = of_iomap(node, 1);
> > + if (!priv->ier) {
> > + pr_err("unable to obtain enable reg iomap address\n");
> > + ret = -ENOMEM;
> > + goto iounmap_status;
> > + }
> > +
> > + priv->unmask = of_iomap(node, 2);
> > + if (!priv->unmask) {
> > + pr_err("unable to obtain unmask reg iomap address\n");
> > + ret = -ENOMEM;
> > + goto iounmap_enable;
> > + }
> > +
> > + priv->parent_irq = irq_of_parse_and_map(node, 0);
> > + if (!priv->parent_irq) {
> > + pr_err("failed to map parent interrupt %d\n", priv->parent_irq);
> > + ret = -EINVAL;
> > + goto iounmap_all;
> > + }
> > +
> > + match = of_match_node(rtd129x_intc_matches, node);
> > + if (!match) {
> > + pr_err("failed to find matching node\n");
> > + ret = -ENODEV;
> > + goto iounmap_all;
> > + }
> > + priv->en_map = match->data;
> > +
> > + // initialize enabled irq's map to its matching status bit in isr by
> > + // inverse walking the enable to status offsets map. Only needed for
> > + // misc
>
> Why do we need any of this? The kernel is supposed to start from a clean
> slate, not to inherit whatever has been set before, unless there is a
> very compelling reason.
>
> > + priv->ier_cached = readl_relaxed(priv->ier);
> > + if (priv->en_map == rtd129x_intc_enable_map_misc) {
> > + ier_tmp = priv->ier_cached;
> > + isr_tmp = 0;
> > + while (ier_tmp) {
> > + ier_bit = __ffs(ier_tmp);
> > + ier_tmp &= ~BIT(ier_bit);
> > + for (i = 0; i < RTD129X_INTC_NR_IRQS; i++)
> > + if (priv->en_map[i] == ier_bit)
> > + isr_tmp |= BIT(i);
> > + }
> > + priv->isr_en = isr_tmp;
> > + } else {
> > + priv->isr_en = priv->ier_cached;
> > + }
> > +
> > + rtd129x_intc_domain = irq_domain_add_linear(node, RTD129X_INTC_NR_IRQS,
> > + &rtd129x_intc_domain_ops,
> > + priv);
> > + if (!rtd129x_intc_domain) {
> > + pr_err("failed to create irq domain\n");
> > + ret = -ENOMEM;
> > + goto iounmap_all;
> > + }
> > +
> > + irq_set_chained_handler_and_data(priv->parent_irq,
> > + rtd129x_intc_irq_handle, priv);
> > +
> > + return 0;
> > +
> > +iounmap_all:
> > + iounmap(priv->unmask);
> > +iounmap_enable:
> > + iounmap(priv->ier);
> > +iounmap_status:
> > + iounmap(priv->isr);
> > +free_priv:
> > + kfree(priv);
> > +err:
> > + return ret;
> > +}
> > +
> > +IRQCHIP_DECLARE(rtd129x_intc_misc, "realtek,rtd129x-intc-misc",
> > + rtd129x_intc_of_init);
> > +IRQCHIP_DECLARE(rtd129x_intc_iso, "realtek,rtd129x-intc-iso",
> > + rtd129x_intc_of_init);
> >
>
> Thanks,
Thank you very much for your time!
>
> M.
> --
> Jazz is not dead. It just smells funny...
^ permalink raw reply
* RE: [PATCH v9 5/6] usb:cdns3 Add Cadence USB3 DRD Driver
From: Felipe Balbi @ 2019-08-12 8:19 UTC (permalink / raw)
To: Pawel Laszczak, devicetree@vger.kernel.org
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
hdegoede@redhat.com, heikki.krogerus@linux.intel.com,
robh+dt@kernel.org, rogerq@ti.com, linux-kernel@vger.kernel.org,
jbergsagel@ti.com, nsekhar@ti.com, nm@ti.com, Suresh Punnoose,
peter.chen@nxp.com, Jayshri Dajiram Pawar, Rahul Kumar
In-Reply-To: <BYAPR07MB470926DA6241B54FC5AF3C2ADDD30@BYAPR07MB4709.namprd07.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 4883 bytes --]
Hi,
Pawel Laszczak <pawell@cadence.com> writes:
>>> I have such situation in which one interrupt line is shared with ehci and cdns3 driver.
>>> In such case this function returns error code.
>>
>>which function returns error code?
>
> devm_request_threaded_irq, of course if I set IRQF_SHARED | IRQF_ONESHOT.
> As I remember it was EBUSY error.
oh, right. That's probably because the handlers must agree on IRQ flags.
>>> So probably I will need to mask only the reported interrupts.
>>
>>you should mask all interrupts from your device, otherwise you top-halt
>>may still end up reentrant.
>>
>>> I can't mask all interrupt using controller register because I can miss some of them.
>>
>>why would you miss them? They would be left in the register until you
>>unmask them and the line is raised again.
>
> I consult this with author of controller.
> We have:
> USB_IEN and USB_ISTS for generic interrupts
> EP_IEN and EP_ISTS for endpoint interrupts
>
> Both these group works different.
> For endpoint I can disable all interrupt and I don't miss any of them.
> So it's normal behavior.
>
> But USB_ISTS work little different. If we mask all interrupt in USB_IEN
> then when new event occurs the EP_ISTS will not be updated.
wait a minute. When you mask USB_ISTS, then EP_ISTS isn't updated? Is
this a quirk on the controller or a design choice?
> It's not standard and not expected behavior but it works in this way.
Yeah, sounds rather odd.
>>>>>>> + /* check USB device interrupt */
>>>>>>> + reg = readl(&priv_dev->regs->usb_ists);
>>>>>>> +
>>>>>>> + if (reg) {
>>>>>>> + writel(reg, &priv_dev->regs->usb_ists);
>>>>>>> + cdns3_check_usb_interrupt_proceed(priv_dev, reg);
>>>>>>> + ret = IRQ_HANDLED;
>>>>>>
>>>>>>now, because you _don't_ mask this interrupt, you're gonna have
>>>>>>issues. Say we actually get both device and endpoint interrupts while
>>>>>>the thread is already running with previous endpoint interrupts. Now
>>>>>>we're gonna reenter the top half, because device interrupts are *not*
>>>>>>masked, which will read usb_ists and handle it here.
>>>>>
>>>>> Endpoint interrupts are masked in cdns3_device_irq_handler and stay masked
>>>>> until they are not handled in threaded handler.
>>>>
>>>>Quick question, then: these ISTS registers, are they masked interrupt
>>>>status or raw interrupt status?
>>>
>>> Yes it's masked, but after masking them the new interrupts will not be reported
>>> In ISTS registers. Form this reason I can mask only reported interrupt.
>>
>>and what happens when you unmask the registers? Do they get reported?
>
> No they are not reported in case of USB_ISTS register.
> They should be reported in case EP_ISTS, but I need to test it.
okay, please _do_ test and verify the behavior. The description above
sounds really surprising to me. Does it really mean that if you mask all
USB_ISTS and then disconnect the cable while interrupt is masked, you
won't know cable was disconnected?
>>>>>>> + struct cdns3_aligned_buf *buf, *tmp;
>>>>>>> +
>>>>>>> + list_for_each_entry_safe(buf, tmp, &priv_dev->aligned_buf_list,
>>>>>>> + list) {
>>>>>>> + if (!buf->in_use) {
>>>>>>> + list_del(&buf->list);
>>>>>>> +
>>>>>>> + spin_unlock_irqrestore(&priv_dev->lock, flags);
>>>>>>
>>>>>>creates the possibility of a race condition
>>>>> Why? In this place the buf can't be used.
>>>>
>>>>but you're reenabling interrupts, right?
>>>
>>> Yes, driver frees not used buffers here.
>>> I think that it's the safest place for this purpose.
>>
>>I guess you missed the point a little. Since you reenable interrupts
>>just to free the buffer, you end up creating the possibility for a race
>>condition. Specially since you don't mask all interrupt events. The
>>moment you reenable interrupts, one of your not-unmasked interrupt
>>sources could trigger, then top-half gets scheduled which tries to wake
>>up the IRQ thread again and things go boom.
>
> Ok, I think I understand. So I have 3 options:
> 1. Mask the USB_IEN and EP_IEN interrupts, but then I can lost some USB_ISTS
> events. It's dangerous options.
sure sounds dangerous, but also sounds quite "peculiar" :-)
> 2. Remove implementation of handling unaligned buffers and assume that
> upper layer will worry about this. What with vendor specific drivers that
> can be used by companies and not upstreamed ?
> It could be good to have such safety mechanism even if it is not currently used.
dunno. It may become dead code that's NEVER used :-)
> 3. Delegate this part of code for instance to separate thread that will be called
> In free time.
Yet another thread? Can't you just run this right before giving back the
USB request? So, don't do it from IRQ handler, but from giveback path?
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH 10/11] mfd: Drop obsolete JZ4740 driver
From: Lee Jones @ 2019-08-12 8:16 UTC (permalink / raw)
To: Paul Cercueil
Cc: Ralf Baechle, Paul Burton, James Hogan, Rob Herring, Mark Rutland,
Vinod Koul, Jean Delvare, Guenter Roeck, Miquel Raynal,
Richard Weinberger, Sebastian Reichel, Bartlomiej Zolnierkiewicz,
Liam Girdwood, Mark Brown, od, devicetree, linux-mips,
linux-kernel, dmaengine, linux-hwmon, linux-mtd, linux-pm,
dri-devel
In-Reply-To: <20190725220215.460-11-paul@crapouillou.net>
On Thu, 25 Jul 2019, Paul Cercueil wrote:
> It has been replaced with the ingenic-iio driver for the ADC.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>
> ---
> drivers/mfd/Kconfig | 9 --
> drivers/mfd/Makefile | 1 -
> drivers/mfd/jz4740-adc.c | 324 ---------------------------------------
> 3 files changed, 334 deletions(-)
> delete mode 100644 drivers/mfd/jz4740-adc.c
Applied, thanks.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v8 0/4] Add support for Orange Pi 3
From: Maxime Ripard @ 2019-08-12 8:14 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Jernej Skrabec, linux-sunxi, Ondřej Jirman, Rob Herring,
David Airlie, Daniel Vetter, Mark Rutland, dri-devel, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <CAGb2v67JVG2rhOdUwBmfsO0+RYb4DNOPmUo=Q_UhL3N+niLiEg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]
On Mon, Aug 12, 2019 at 03:54:03PM +0800, Chen-Yu Tsai wrote:
> On Mon, Aug 12, 2019 at 3:45 PM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
> >
> > Dne torek, 06. avgust 2019 ob 17:57:39 CEST je megous-5qf/QAjKc83QT0dZR+AlfA@public.gmane.org napisal(a):
> > > From: Ondrej Jirman <megous-5qf/QAjKc83QT0dZR+AlfA@public.gmane.org>
> > >
> > > This series implements support for Xunlong Orange Pi 3 board. There
> > > are only a few patches remaining.
> > >
> > > - ethernet support - just a DT change (patch 1)
> > > - HDMI support (patches 2-4)
> > >
> > > For some people, ethernet doesn't work after reboot because u-boot doesn't
> > > support AXP805 PMIC, and will not turn off the etherent PHY regulators.
> > > So the regulator controlled by gpio will be shut down, but the other one
> > > controlled by the AXP PMIC will not.
> > >
> > > This is a problem only when running with a builtin driver. This needs
> > > to be fixed in u-boot.
> > >
> > >
> > > Please take a look.
> >
> > Is there anything missing? It would be nice to get this in 5.4. There is a lot
> > of H6 boards which needs DDC bus enable mechanism (part of H6 reference
> > design), including Beelink GS1 which already has HDMI node in mainline kernel
> > DT, but due to disabled DDC lines works only with 1024x768 (fallback
> > resolution in DRM core).
>
> I have a few minor comments about patch 1.
>
> I think the HDMI bits are good, but I don't have maintainership / commit
> permissions for drm-misc, so I'll have to wait until someone applies patches
> 2 and 3 before I apply patch 4.
I've applied 2,3 and 4
Thanks!
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190812081445.kfsbikfrt3pmsh6d%40flea.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] arm64: dts: allwinner: a64: Drop PMU node
From: Maxime Ripard @ 2019-08-12 8:04 UTC (permalink / raw)
To: Vasily Khoruzhick
Cc: Mark Rutland, devicetree, Jared D . McNeill, Chen-Yu Tsai,
Rob Herring, Harald Geyer, Robin Murphy, arm-linux
In-Reply-To: <CA+E=qVeiWoRGn05HpMzx_5yidit4GM18tBrziW5MBo00f_-PKQ@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 3330 bytes --]
On Thu, Aug 08, 2019 at 12:59:07PM -0700, Vasily Khoruzhick wrote:
> On Thu, Aug 8, 2019 at 9:26 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Wed, Aug 07, 2019 at 10:36:08AM -0700, Vasily Khoruzhick wrote:
> > > On Wed, Aug 7, 2019 at 4:56 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > On Tue, Aug 06, 2019 at 07:39:26PM -0700, Vasily Khoruzhick wrote:
> > > > > On Tue, Aug 6, 2019 at 2:14 PM Robin Murphy <robin.murphy@arm.com> wrote:
> > > > > >
> > > > > > On 2019-08-06 9:52 pm, Vasily Khoruzhick wrote:
> > > > > > > On Tue, Aug 6, 2019 at 1:19 PM Harald Geyer <harald@ccbib.org> wrote:
> > > > > > >>
> > > > > > >> Vasily Khoruzhick writes:
> > > > > > >>> On Tue, Aug 6, 2019 at 7:35 AM Robin Murphy <robin.murphy@arm.com> wrote:
> > > > > > >>>>
> > > > > > >>>> On 06/08/2019 15:01, Vasily Khoruzhick wrote:
> > > > > > >>>>> Looks like PMU in A64 is broken, it generates no interrupts at all and
> > > > > > >>>>> as result 'perf top' shows no events.
> > > > > > >>>>
> > > > > > >>>> Does something like 'perf stat sleep 1' at least count cycles correctly?
> > > > > > >>>> It could well just be that the interrupt numbers are wrong...
> > > > > > >>>
> > > > > > >>> Looks like it does, at least result looks plausible:
> > > > > > >>
> > > > > > >> I'm using perf stat regularly (cache benchmarks) and it works fine.
> > > > > > >>
> > > > > > >> Unfortunately I wasn't aware that perf stat is a poor test for
> > > > > > >> the interrupts part of the node, when I added it. So I'm not too
> > > > > > >> surprised I got it wrong.
> > > > > > >>
> > > > > > >> However, it would be unfortunate if the node got removed completely,
> > > > > > >> because perf stat would not work anymore. Maybe we can only remove
> > > > > > >> the interrupts or just fix them even if the HW doesn't work?
> > > > > > >
> > > > > > > I'm not familiar with PMU driver. Is it possible to get it working
> > > > > > > without interrupts?
> > > > > >
> > > > > > Yup - you get a grumpy message from the driver, it will refuse sampling
> > > > > > events (the ones which weren't working anyway), and if you measure
> > > > > > anything for long enough that a counter overflows you'll get wonky
> > > > > > results. But for counting hardware events over relatively short periods
> > > > > > it'll still do the job.
> > > > >
> > > > > I tried to drop interrupts completely from the node but 'perf top' is
> > > > > still broken. Though now in different way: it complains "cycles: PMU
> > > > > Hardware doesn't support sampling/overflow-interrupts. Try 'perf
> > > > > stat'"
> > > >
> > > > I have no idea if that's the culprit, but what is the state of the
> > > > 0x09010000 register?
> > >
> > > What register is that and how do I check it?
> >
> > It's in the CPUX Configuration block, and the bits are labelled as CPU
> > Debug Reset.
> >
> > And if you have busybox, you can use devmem.
>
> CPUX configuration block is at 0x01700000 according to A64 user
> manual, and particular register you're interested in is at 0x01700080,
> its value is 0x1110110F.
>
> Bits 16-19 are not defined in user manual and are not set.
Sorry, I somehow thought this was for the H6...
I don't have any idea then :/
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 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 v8 0/4] Add support for Orange Pi 3
From: Chen-Yu Tsai @ 2019-08-12 7:54 UTC (permalink / raw)
To: Jernej Skrabec
Cc: linux-sunxi, Ondřej Jirman, Maxime Ripard, Rob Herring,
David Airlie, Daniel Vetter, Mark Rutland, dri-devel, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <2218280.0sI6yjypBf@jernej-laptop>
On Mon, Aug 12, 2019 at 3:45 PM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
>
> Dne torek, 06. avgust 2019 ob 17:57:39 CEST je megous-5qf/QAjKc83QT0dZR+AlfA@public.gmane.org napisal(a):
> > From: Ondrej Jirman <megous-5qf/QAjKc83QT0dZR+AlfA@public.gmane.org>
> >
> > This series implements support for Xunlong Orange Pi 3 board. There
> > are only a few patches remaining.
> >
> > - ethernet support - just a DT change (patch 1)
> > - HDMI support (patches 2-4)
> >
> > For some people, ethernet doesn't work after reboot because u-boot doesn't
> > support AXP805 PMIC, and will not turn off the etherent PHY regulators.
> > So the regulator controlled by gpio will be shut down, but the other one
> > controlled by the AXP PMIC will not.
> >
> > This is a problem only when running with a builtin driver. This needs
> > to be fixed in u-boot.
> >
> >
> > Please take a look.
>
> Is there anything missing? It would be nice to get this in 5.4. There is a lot
> of H6 boards which needs DDC bus enable mechanism (part of H6 reference
> design), including Beelink GS1 which already has HDMI node in mainline kernel
> DT, but due to disabled DDC lines works only with 1024x768 (fallback
> resolution in DRM core).
I have a few minor comments about patch 1.
I think the HDMI bits are good, but I don't have maintainership / commit
permissions for drm-misc, so I'll have to wait until someone applies patches
2 and 3 before I apply patch 4.
ChenYu
> Best regards,
> Jernej
>
> >
> > thank you and regards,
> > Ondrej Jirman
> >
> > Changes in v8:
> > - added reviewed-by tags
> > - dropped already applied patches
> > - added more info about the phy initialization issue after reset
> >
> > Changes in v7:
> > - dropped stored reference to connector_pdev as suggested by Jernej
> > - added forgotten dt-bindings reviewed-by tag
> >
> > Changes in v6:
> > - added dt-bindings reviewed-by tag
> > - fix wording in stmmac commit (as suggested by Sergei)
> >
> > Changes in v5:
> > - dropped already applied patches (pinctrl patches, mmc1 pinconf patch)
> > - rename GMAC-3V3 -> GMAC-3V to match the schematic (Jagan)
> > - changed hdmi-connector's ddc-supply property to ddc-en-gpios
> > (Rob Herring)
> >
> > Changes in v4:
> > - fix checkpatch warnings/style issues
> > - use enum in struct sunxi_desc_function for io_bias_cfg_variant
> > - collected acked-by's
> > - fix compile error in drivers/pinctrl/sunxi/pinctrl-sun9i-a80-r.c:156
> > caused by missing conversion from has_io_bias_cfg struct member
> > (I've kept the acked-by, because it's a trivial change, but feel free
> > to object.) (reported by Martin A. on github)
> > I did not have A80 pinctrl enabled for some reason, so I did not catch
> > this sooner.
> > - dropped brcm firmware patch (was already applied)
> > - dropped the wifi dts patch (will re-send after H6 RTC gets merged,
> > along with bluetooth support, in a separate series)
> >
> > Changes in v3:
> > - dropped already applied patches
> > - changed pinctrl I/O bias selection constants to enum and renamed
> > - added /omit-if-no-ref/ to mmc1_pins
> > - made mmc1_pins default pinconf for mmc1 in H6 dtsi
> > - move ddc-supply to HDMI connector node, updated patch descriptions,
> > changed dt-bindings docs
> >
> > Changes in v2:
> > - added dt-bindings documentation for the board's compatible string
> > (suggested by Clement)
> > - addressed checkpatch warnings and code formatting issues (on Maxime's
> > suggestions)
> > - stmmac: dropped useless parenthesis, reworded description of the patch
> > (suggested by Sergei)
> > - drop useles dev_info() about the selected io bias voltage
> > - docummented io voltage bias selection variant macros
> > - wifi: marked WiFi DTS patch and realted mmc1_pins as "DO NOT MERGE",
> > because wifi depends on H6 RTC support that's not merged yet (suggested
> > by Clement)
> > - added missing signed-of-bys
> > - changed &usb2otg dr_mode to otg, and added a note about VBUS
> > - improved wording of HDMI driver's DDC power supply patch
> >
> > Ondrej Jirman (4):
> > arm64: dts: allwinner: orange-pi-3: Enable ethernet
> > dt-bindings: display: hdmi-connector: Support DDC bus enable
> > drm: sun4i: Add support for enabling DDC I2C bus to sun8i_dw_hdmi glue
> > arm64: dts: allwinner: orange-pi-3: Enable HDMI output
> >
> > .../display/connector/hdmi-connector.txt | 1 +
> > .../dts/allwinner/sun50i-h6-orangepi-3.dts | 70 +++++++++++++++++++
> > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 54 ++++++++++++--
> > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 2 +
> > 4 files changed, 123 insertions(+), 4 deletions(-)
>
>
>
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/2218280.0sI6yjypBf%40jernej-laptop.
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/CAGb2v67JVG2rhOdUwBmfsO0%2BRYb4DNOPmUo%3DQ_UhL3N%2BniLiEg%40mail.gmail.com.
^ permalink raw reply
* Re: [PATCH v8 0/4] Add support for Orange Pi 3
From: Jernej Škrabec @ 2019-08-12 7:44 UTC (permalink / raw)
To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, megous-5qf/QAjKc83QT0dZR+AlfA
Cc: Maxime Ripard, Chen-Yu Tsai, Rob Herring, David Airlie,
Daniel Vetter, Mark Rutland,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20190806155744.10263-1-megous-5qf/QAjKc83QT0dZR+AlfA@public.gmane.org>
Dne torek, 06. avgust 2019 ob 17:57:39 CEST je megous-5qf/QAjKc83QT0dZR+AlfA@public.gmane.org napisal(a):
> From: Ondrej Jirman <megous-5qf/QAjKc83QT0dZR+AlfA@public.gmane.org>
>
> This series implements support for Xunlong Orange Pi 3 board. There
> are only a few patches remaining.
>
> - ethernet support - just a DT change (patch 1)
> - HDMI support (patches 2-4)
>
> For some people, ethernet doesn't work after reboot because u-boot doesn't
> support AXP805 PMIC, and will not turn off the etherent PHY regulators.
> So the regulator controlled by gpio will be shut down, but the other one
> controlled by the AXP PMIC will not.
>
> This is a problem only when running with a builtin driver. This needs
> to be fixed in u-boot.
>
>
> Please take a look.
Is there anything missing? It would be nice to get this in 5.4. There is a lot
of H6 boards which needs DDC bus enable mechanism (part of H6 reference
design), including Beelink GS1 which already has HDMI node in mainline kernel
DT, but due to disabled DDC lines works only with 1024x768 (fallback
resolution in DRM core).
Best regards,
Jernej
>
> thank you and regards,
> Ondrej Jirman
>
> Changes in v8:
> - added reviewed-by tags
> - dropped already applied patches
> - added more info about the phy initialization issue after reset
>
> Changes in v7:
> - dropped stored reference to connector_pdev as suggested by Jernej
> - added forgotten dt-bindings reviewed-by tag
>
> Changes in v6:
> - added dt-bindings reviewed-by tag
> - fix wording in stmmac commit (as suggested by Sergei)
>
> Changes in v5:
> - dropped already applied patches (pinctrl patches, mmc1 pinconf patch)
> - rename GMAC-3V3 -> GMAC-3V to match the schematic (Jagan)
> - changed hdmi-connector's ddc-supply property to ddc-en-gpios
> (Rob Herring)
>
> Changes in v4:
> - fix checkpatch warnings/style issues
> - use enum in struct sunxi_desc_function for io_bias_cfg_variant
> - collected acked-by's
> - fix compile error in drivers/pinctrl/sunxi/pinctrl-sun9i-a80-r.c:156
> caused by missing conversion from has_io_bias_cfg struct member
> (I've kept the acked-by, because it's a trivial change, but feel free
> to object.) (reported by Martin A. on github)
> I did not have A80 pinctrl enabled for some reason, so I did not catch
> this sooner.
> - dropped brcm firmware patch (was already applied)
> - dropped the wifi dts patch (will re-send after H6 RTC gets merged,
> along with bluetooth support, in a separate series)
>
> Changes in v3:
> - dropped already applied patches
> - changed pinctrl I/O bias selection constants to enum and renamed
> - added /omit-if-no-ref/ to mmc1_pins
> - made mmc1_pins default pinconf for mmc1 in H6 dtsi
> - move ddc-supply to HDMI connector node, updated patch descriptions,
> changed dt-bindings docs
>
> Changes in v2:
> - added dt-bindings documentation for the board's compatible string
> (suggested by Clement)
> - addressed checkpatch warnings and code formatting issues (on Maxime's
> suggestions)
> - stmmac: dropped useless parenthesis, reworded description of the patch
> (suggested by Sergei)
> - drop useles dev_info() about the selected io bias voltage
> - docummented io voltage bias selection variant macros
> - wifi: marked WiFi DTS patch and realted mmc1_pins as "DO NOT MERGE",
> because wifi depends on H6 RTC support that's not merged yet (suggested
> by Clement)
> - added missing signed-of-bys
> - changed &usb2otg dr_mode to otg, and added a note about VBUS
> - improved wording of HDMI driver's DDC power supply patch
>
> Ondrej Jirman (4):
> arm64: dts: allwinner: orange-pi-3: Enable ethernet
> dt-bindings: display: hdmi-connector: Support DDC bus enable
> drm: sun4i: Add support for enabling DDC I2C bus to sun8i_dw_hdmi glue
> arm64: dts: allwinner: orange-pi-3: Enable HDMI output
>
> .../display/connector/hdmi-connector.txt | 1 +
> .../dts/allwinner/sun50i-h6-orangepi-3.dts | 70 +++++++++++++++++++
> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 54 ++++++++++++--
> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 2 +
> 4 files changed, 123 insertions(+), 4 deletions(-)
^ permalink raw reply
* Re: [PING 2] [PATCH v5 1/7] nfc: pn533: i2c: "pn532" as dt compatible string
From: Lars Poeschel @ 2019-08-12 7:40 UTC (permalink / raw)
To: Johan Hovold
Cc: devicetree, Samuel Ortiz, open list:NFC SUBSYSTEM, open list,
netdev
In-Reply-To: <20190805124236.GG3574@localhost>
On Mon, Aug 05, 2019 at 02:42:36PM +0200, Johan Hovold wrote:
> You may want to resend this series to netdev now. David Miller will be
> picking up NFC patches directly from there.
Thank you very much for this information. Johannes Berg did reach out to
me already.
Rebase, test and resend is queued up for one of my next free timeslots.
Thanks again,
Lars
^ permalink raw reply
* Re: [PATCH 2/2] pwm: sprd: Add Spreadtrum PWM support
From: Baolin Wang @ 2019-08-12 7:29 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Thierry Reding, Rob Herring, Mark Rutland, Orson Zhai,
Chunyan Zhang, Vincent Guittot, linux-pwm, DTML, LKML, kernel
In-Reply-To: <20190809144124.3as3rtctlywxkudr@pengutronix.de>
Hi Uwe,
On Fri, 9 Aug 2019 at 22:41, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Baolin,
>
> On Fri, Aug 09, 2019 at 06:06:21PM +0800, Baolin Wang wrote:
> > On Fri, 9 Aug 2019 at 17:10, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Thu, Aug 08, 2019 at 04:59:39PM +0800, Baolin Wang wrote:
> > > > +{
> > > > + struct sprd_pwm_chip *spc =
> > > > + container_of(chip, struct sprd_pwm_chip, chip);
> > > > + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > > + u64 div, tmp;
> > > > + u32 prescale, duty;
> > > > + int ret;
> > > > +
> > > > + /*
> > > > + * NOTE: the clocks to PWM channel has to be enabled first before
> > > > + * writing to the registers.
> > > > + */
> > > > + if (!chn->clk_enabled) {
> > > > + ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
> > >
> > > Do you really need to enable all 8 clocks to configure a single channel?
> >
> > We have 4 channels, and each channel use 2 clocks, so here only enable
> > 2 clocks, see SPRD_PWM_NUM_CLKS.
>
> Ah, I thought SPRD_PWM_NUM_CLKS was 8.
>
> > > > + if (ret) {
> > > > + dev_err(spc->dev, "failed to enable pwm%u clock\n",
> > > > + pwm->hwpwm);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + chn->clk_enabled = true;
> > > > + }
> > > > +
> > > > + duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> > >
> > > @Baolin: until we're there that there are framework requirements how to
> > > round, please document at least how you do it here. Also describing the
> > > underlying concepts would be good for the driver reader.
> > >
> > > Something like:
> > >
> > > /*
> > > * The hardware provides a counter that is feed by the source clock.
> > > * The period length is (PRESCALE + 1) * MOD counter steps.
> > > * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > > *
> > > * To keep the maths simple we're always using MOD = MOD_MAX.
> > > * The value for PRESCALE is selected such that the resulting period
> > > * gets the maximal length not bigger than the requested one with the
> > > * given settings (MOD = MOD_MAX and input clock).
> > > */
> >
> > Yes, totally agree with you. I will add some documentation for our
> > controller's setting.
> >
> > >
> > > @Thierry: having a framework guideline here would be good. Or still
> > > better a guideline and a debug setting that notices drivers stepping out
> > > of line.
> > >
> > > I assume using MOD = 0 is forbidden?
> > >
> > > > + /*
> > > > + * According to the datasheet, the period_ns calculation formula
> > > > + * should be:
> > > > + * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> > > > + *
> > > > + * Then we can get the prescale formula:
> > > > + * prescale = (period_ns * clk_rate) / (10^9 * mod) -1
> > > > + */
> > > > + tmp = chn->clk_rate * period_ns;
> > > > + div = 1000000000ULL * SPRD_PWM_MOD_MAX;
> > >
> > > Please use NSEC_PER_SEC instead of 1000000000ULL.
> >
> > Sure.
> >
> > >
> > > > + prescale = div64_u64(tmp, div) - 1;
> > >
> > > If tmp is smaller than div you end up with prescale = 0xffffffff which
> > > should be catched. What if prescale > 0xffff?
> >
> > Usually we can not meet this case, but you are right, the prescale has
> > a limit according to the register definition. So will add a validation
> > here.
> >
> > >
> > > > + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
> > > > + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
> > >
> > > You're losing precision here as you always use SPRD_PWM_MOD_MAX, right?
> > > (Just for my understanding, if this simpler approach is good enough
> > > here that's fine.)
> >
> > Yes, SPRD_PWM_MOD_MAX is good enough.
>
> ok.
>
> > >
> > > > + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PAT_LOW, SPRD_PWM_REG_MSK);
> > > > + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PAT_HIGH, SPRD_PWM_REG_MSK);
> > >
> > > Please describe these two registers in a short comment.
> >
> > Sure.
> >
> > >
> > > > + sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
> > >
> > > Is the configuration here atomic in the sense that the write of DUTY
> > > above only gets effective when PRESCALE is written? If not, please add
> >
> > Yes.
> >
> > > a describing paragraph at the top of the driver similar to what is
> > > written in pwm-sifive.c. When the PWM is already running before, how
> > > does a configuration change effects the output? Is the currently running
> > > period completed?
> >
> > Anyway, I'll add some comments to explain how it works.
>
> I'd apreciate if you'd stick to the format in pwm-sifive.c to make it
> easier to grep for that.
OK.
>
> > > > +static void sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > + struct pwm_state *state)
> > > > +{
> > > > + struct sprd_pwm_chip *spc =
> > > > + container_of(chip, struct sprd_pwm_chip, chip);
> > > > + struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > > + u32 enabled, duty, prescale;
> > > > + u64 tmp;
> > > > + int ret;
> > > > +
> > > > + ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
> > > > + if (ret) {
> > > > + dev_err(spc->dev, "failed to enable pwm%u clocks\n",
> > > > + pwm->hwpwm);
> > > > + return;
> > > > + }
> > > > +
> > > > + chn->clk_enabled = true;
> > > > +
> > > > + duty = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY) & SPRD_PWM_REG_MSK;
> > > > + prescale = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE) & SPRD_PWM_REG_MSK;
> > > > + enabled = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE) & SPRD_PWM_ENABLE_BIT;
> > > > +
> > > > + /*
> > > > + * According to the datasheet, the period_ns and duty_ns calculation
> > > > + * formula should be:
> > > > + * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> > > > + * duty_ns = 10^9 * (prescale + 1) * duty / clk_rate
> > > > + */
> > > > + tmp = (prescale + 1) * 1000000000ULL * SPRD_PWM_MOD_MAX;
> > > > + state->period = div64_u64(tmp, chn->clk_rate);
> > >
> > > This is not idempotent. If you apply the configuration that is returned
> > > here this shouldn't result in a reconfiguration.
> >
> > Since we may configure the PWM in bootloader, so in kernel part we
> > should get current PWM state to avoid reconfiguration if state
> > configuration are same.
>
> This is also important as some consumers might do something like:
>
> state = pwm_get_state(mypwm)
> if (something):
> state->duty = 0
> else:
> state->duty = state->period / 2
> pwm_set_state(mypwm, state)
>
> and then period shouldn't get smaller in each step.
> (This won't happen as of now because the PWM framework caches the last
> state that was set and returns this for pwm_get_state. Still getting
> this right would be good.)
I understood your concern, but the period can be configured in
bootloader, we have no software things to save the accurate period.
Moreover I think I can change to use DIV_ROUND_CLOSET_ULL to keep the
accuracy.
>
> > > > + tmp = (prescale + 1) * 1000000000ULL * duty;
> > > > + state->duty_cycle = div64_u64(tmp, chn->clk_rate);
> > > > +
> > > > + state->enabled = !!enabled;
> > > > +
> > > > + /* Disable PWM clocks if the PWM channel is not in enable state. */
> > > > + if (!enabled) {
> > > > + clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > > > + chn->clk_enabled = false;
> > > > + }
> > > > +}
> > > > +
> > > > +static const struct pwm_ops sprd_pwm_ops = {
> > > > + .config = sprd_pwm_config,
> > > > + .enable = sprd_pwm_enable,
> > > > + .disable = sprd_pwm_disable,
> > > > + .get_state = sprd_pwm_get_state,
> > > > + .owner = THIS_MODULE,
> > > > +};
> > > > +
> > > > +static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
> > > > +{
> > > > + struct clk *clk_parent, *clk_pwm;
> > > > + int ret, i, clk_index = 0;
> > > > +
> > > > + clk_parent = devm_clk_get(spc->dev, "source");
> > > > + if (IS_ERR(clk_parent)) {
> > > > + dev_err(spc->dev, "failed to get source clock\n");
> > > > + return PTR_ERR(clk_parent);
> > > > + }
> > > > +
> > > > + for (i = 0; i < SPRD_PWM_NUM; i++) {
> > > > + struct sprd_pwm_chn *chn = &spc->chn[i];
> > > > + int j;
> > > > +
> > > > + for (j = 0; j < SPRD_PWM_NUM_CLKS; ++j)
> > > > + chn->clks[j].id = sprd_pwm_clks[clk_index++];
> > > > +
> > > > + ret = devm_clk_bulk_get(spc->dev, SPRD_PWM_NUM_CLKS, chn->clks);
> > > > + if (ret) {
> > > > + if (ret == -ENOENT)
> > > > + break;
> > >
> > > devm_clk_bulk_get_optional ? I'm sure you don't want to get all 8 clocks
> > > 8 times.
> >
> > This is not optional, each channel has 2 required clocks, and we have
> > 4 channels. (SPRD_PWM_NUM_CLKS == 2)
>
> I see. Currently it is not possible to use channel 2 if there are no
> clocks for channel 0, right? There is no hardware related problem here,
Yes.
> just a shortcoming of the driver?
Yes.
>
> > > > +
> > > > + dev_err(spc->dev, "failed to get channel clocks\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + clk_pwm = chn->clks[1].clk;
> > >
> > > This 1 looks suspicious. Are you using all clocks provided in the dtb at
> > > all? You're not using i in the loop at all, this doesn't look right.
> >
> > Like I said above, each channel has 2 clocks: enable clock and pwm
> > clock, the 2nd clock of each channel's bulk clocks is the pwm clock,
> > which is used to set the source clock. I know this's not easy to read,
> > so do you have any good suggestion?
>
> Not sure this is easily possible to rework to make this clearer.
>
> Do these clks have different uses? e.g. one to enable register access
> and the other to enable the pwm output? If so just using
Yes.
> devm_clk_bulk_get isn't the right thing because you should be able know
> if clks[0] or clks[1] is the one you need to enable the output (or
> register access).
We've fixed the clock order in bulk clocks by the array
'sprd_pwm_clks', maybe I should define one readable macro instead of
magic number.
>
> > > > + if (!clk_set_parent(clk_pwm, clk_parent))
> > > > + chn->clk_rate = clk_get_rate(clk_pwm);
> > > > + else
> > > > + chn->clk_rate = SPRD_PWM_DEFAULT_CLK;
> > >
> > > I don't know all the clock framework details, but I think there are
> > > better ways to ensure that a given clock is used as parent for another
> > > given clock. Please read the chapter about "Assigned clock parents and
> > > rates" in the clock bindings and check if this could be used for the
> > > purpose here and so simplify the driver.
> >
> > Actually there are many other drivers set the parent clock like this,
> > and we want a default clock if failed to set the parent clock.
>
> These might be older than the clk framework capabilities, or the
> reviewers didn't pay attention to this detail; both shouldn't be a
> reason to not make it better here.
The clock framework supplies 'assigned-clocks' and
'assigned-clock-parents' properties to set parent, but for our case we
still want to set a default clock rate if failed to set parent when
met some abnormal things.
>
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int sprd_pwm_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < spc->num_pwms; i++)
> > > > + pwm_disable(&spc->chip.pwms[i]);
> > >
> > > This is wrong. You must not call pwm_disable here. It's the consumer's
> > > responsibility to disable the hardware.
> >
> > Emm, I saw some drivers did like this, like pwm-spear.c.
> > Could you elaborate on what's the problem if the driver issues pwm_disable?
>
> This is a function to be called from code that called pwm_get before
> (i.e. pwm consumers). This just doesn't explode because up to now the
> PWM framework is only a thin wrapper around the lowlevel driver
> callbacks.
>
> The reasoning is similar to what I wrote in commit
> f82d15e223403b05fffb33ba792b87a8620f6fee. I'd like to have a PWM_DEBUG
> setting that catches such problems but the discussion with Thierry to
> even document the expectations is stuck, see
> https://patchwork.ozlabs.org/patch/1021566/.
>
Thanks for your explanation, and I'll remove pwm_disable from driver.
Thanks for your comments.
--
Baolin Wang
Best Regards
^ permalink raw reply
* RE: [PATCH v9 5/6] usb:cdns3 Add Cadence USB3 DRD Driver
From: Pawel Laszczak @ 2019-08-12 7:13 UTC (permalink / raw)
To: Felipe Balbi, devicetree@vger.kernel.org
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
hdegoede@redhat.com, heikki.krogerus@linux.intel.com,
robh+dt@kernel.org, rogerq@ti.com, linux-kernel@vger.kernel.org,
jbergsagel@ti.com, nsekhar@ti.com, nm@ti.com, Suresh Punnoose,
peter.chen@nxp.com, Jayshri Dajiram Pawar, Rahul Kumar
In-Reply-To: <87k1bjvtvi.fsf@gmail.com>
Hi,
>
>Hi,
>
>Pawel Laszczak <pawell@cadence.com> writes:
>
>> Hi,
>>
>>>
>>>Pawel Laszczak <pawell@cadence.com> writes:
>>>>>> +static int cdns3_gadget_start(struct cdns3 *cdns)
>>>>>> +{
>>>>>> + struct cdns3_device *priv_dev;
>>>>>> + u32 max_speed;
>>>>>> + int ret;
>>>>>> +
>>>>>> + priv_dev = kzalloc(sizeof(*priv_dev), GFP_KERNEL);
>>>>>> + if (!priv_dev)
>>>>>> + return -ENOMEM;
>>>>>> +
>>>>>> + cdns->gadget_dev = priv_dev;
>>>>>> + priv_dev->sysdev = cdns->dev;
>>>>>> + priv_dev->dev = cdns->dev;
>>>>>> + priv_dev->regs = cdns->dev_regs;
>>>>>> +
>>>>>> + device_property_read_u16(priv_dev->dev, "cdns,on-chip-buff-size",
>>>>>> + &priv_dev->onchip_buffers);
>>>>>> +
>>>>>> + if (priv_dev->onchip_buffers <= 0) {
>>>>>> + u32 reg = readl(&priv_dev->regs->usb_cap2);
>>>>>> +
>>>>>> + priv_dev->onchip_buffers = USB_CAP2_ACTUAL_MEM_SIZE(reg);
>>>>>> + }
>>>>>> +
>>>>>> + if (!priv_dev->onchip_buffers)
>>>>>> + priv_dev->onchip_buffers = 256;
>>>>>> +
>>>>>> + max_speed = usb_get_maximum_speed(cdns->dev);
>>>>>> +
>>>>>> + /* Check the maximum_speed parameter */
>>>>>> + switch (max_speed) {
>>>>>> + case USB_SPEED_FULL:
>>>>>> + case USB_SPEED_HIGH:
>>>>>> + case USB_SPEED_SUPER:
>>>>>> + break;
>>>>>> + default:
>>>>>> + dev_err(cdns->dev, "invalid maximum_speed parameter %d\n",
>>>>>> + max_speed);
>>>>>> + /* fall through */
>>>>>> + case USB_SPEED_UNKNOWN:
>>>>>> + /* default to superspeed */
>>>>>> + max_speed = USB_SPEED_SUPER;
>>>>>> + break;
>>>>>> + }
>>>>>> +
>>>>>> + /* fill gadget fields */
>>>>>> + priv_dev->gadget.max_speed = max_speed;
>>>>>> + priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
>>>>>> + priv_dev->gadget.ops = &cdns3_gadget_ops;
>>>>>> + priv_dev->gadget.name = "usb-ss-gadget";
>>>>>> + priv_dev->gadget.sg_supported = 1;
>>>>>> +
>>>>>> + spin_lock_init(&priv_dev->lock);
>>>>>> + INIT_WORK(&priv_dev->pending_status_wq,
>>>>>> + cdns3_pending_setup_status_handler);
>>>>>> +
>>>>>> + /* initialize endpoint container */
>>>>>> + INIT_LIST_HEAD(&priv_dev->gadget.ep_list);
>>>>>> + INIT_LIST_HEAD(&priv_dev->aligned_buf_list);
>>>>>> +
>>>>>> + ret = cdns3_init_eps(priv_dev);
>>>>>> + if (ret) {
>>>>>> + dev_err(priv_dev->dev, "Failed to create endpoints\n");
>>>>>> + goto err1;
>>>>>> + }
>>>>>> +
>>>>>> + /* allocate memory for setup packet buffer */
>>>>>> + priv_dev->setup_buf = dma_alloc_coherent(priv_dev->sysdev, 8,
>>>>>> + &priv_dev->setup_dma, GFP_DMA);
>>>>>> + if (!priv_dev->setup_buf) {
>>>>>> + ret = -ENOMEM;
>>>>>> + goto err2;
>>>>>> + }
>>>>>> +
>>>>>> + priv_dev->dev_ver = readl(&priv_dev->regs->usb_cap6);
>>>>>> +
>>>>>> + dev_dbg(priv_dev->dev, "Device Controller version: %08x\n",
>>>>>> + readl(&priv_dev->regs->usb_cap6));
>>>>>> + dev_dbg(priv_dev->dev, "USB Capabilities:: %08x\n",
>>>>>> + readl(&priv_dev->regs->usb_cap1));
>>>>>> + dev_dbg(priv_dev->dev, "On-Chip memory cnfiguration: %08x\n",
>>>>>> + readl(&priv_dev->regs->usb_cap2));
>>>>>> +
>>>>>> + priv_dev->dev_ver = GET_DEV_BASE_VERSION(priv_dev->dev_ver);
>>>>>> +
>>>>>> + priv_dev->zlp_buf = kzalloc(CDNS3_EP_ZLP_BUF_SIZE, GFP_KERNEL);
>>>>>> + if (!priv_dev->zlp_buf) {
>>>>>> + ret = -ENOMEM;
>>>>>> + goto err3;
>>>>>> + }
>>>>>> +
>>>>>> + /* add USB gadget device */
>>>>>> + ret = usb_add_gadget_udc(priv_dev->dev, &priv_dev->gadget);
>>>>>> + if (ret < 0) {
>>>>>> + dev_err(priv_dev->dev,
>>>>>> + "Failed to register USB device controller\n");
>>>>>> + goto err4;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +err4:
>>>>>> + kfree(priv_dev->zlp_buf);
>>>>>> +err3:
>>>>>> + dma_free_coherent(priv_dev->sysdev, 8, priv_dev->setup_buf,
>>>>>> + priv_dev->setup_dma);
>>>>>> +err2:
>>>>>> + cdns3_free_all_eps(priv_dev);
>>>>>> +err1:
>>>>>> + cdns->gadget_dev = NULL;
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int __cdns3_gadget_init(struct cdns3 *cdns)
>>>>>> +{
>>>>>> + struct cdns3_device *priv_dev;
>>>>>> + int ret = 0;
>>>>>> +
>>>>>> + cdns3_drd_switch_gadget(cdns, 1);
>>>>>> + pm_runtime_get_sync(cdns->dev);
>>>>>> +
>>>>>> + ret = cdns3_gadget_start(cdns);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + priv_dev = cdns->gadget_dev;
>>>>>> + ret = devm_request_threaded_irq(cdns->dev, cdns->dev_irq,
>>>>>> + cdns3_device_irq_handler,
>>>>>> + cdns3_device_thread_irq_handler,
>>>>>> + IRQF_SHARED, dev_name(cdns->dev), cdns);
>>>>>
>>>>>copied handlers here for commenting. Note that you don't have
>>>>>IRQF_ONESHOT:
>>>>
>>>> I know, I can't use IRQF_ ONESHOT flag in this case. I have implemented
>>>> some code for masking/unmasking interrupts in cdns3_device_irq_handler.
>>>>
>>>> Some priority interrupts should be handled ASAP so I can't blocked interrupt
>>>> Line.
>>>
>>>You're completely missing my comment. Your top half should be as short
>>>as possile. It should only check if current device generated
>>>interrupts. If it did, then you should wake the thread handler.
>>>
>>>This is to improve realtime behavior but not keeping preemption disabled
>>>for longer than necessary.
>>
>> Ok, I understand. I will move it to thread handler.
>>
>> I can't use IRQF_ONESHOT flag because it doesn't work when interrupt line is shared.
>
>yeah, you should try to avoid ONESHOT :-)
>
>> I have such situation in which one interrupt line is shared with ehci and cdns3 driver.
>> In such case this function returns error code.
>
>which function returns error code?
devm_request_threaded_irq, of course if I set IRQF_SHARED | IRQF_ONESHOT.
As I remember it was EBUSY error.
>
>> So probably I will need to mask only the reported interrupts.
>
>you should mask all interrupts from your device, otherwise you top-halt
>may still end up reentrant.
>
>> I can't mask all interrupt using controller register because I can miss some of them.
>
>why would you miss them? They would be left in the register until you
>unmask them and the line is raised again.
I consult this with author of controller.
We have:
USB_IEN and USB_ISTS for generic interrupts
EP_IEN and EP_ISTS for endpoint interrupts
Both these group works different.
For endpoint I can disable all interrupt and I don't miss any of them.
So it's normal behavior.
But USB_ISTS work little different. If we mask all interrupt in USB_IEN
then when new event occurs the EP_ISTS will not be updated.
It's not standard and not expected behavior but it works in this way.
I thought that EP_IEN/EP_ISTS works in the same way.
I update driver according with this rules. I hope that it will work
>
>> After masking all interrupt the next new event will not be reported in usb_ists, ep_ists
>> registers.
>
>why not? Masking means that new events won't cause the IRQ line to be
>asserted (or MSI DWORD write won't be initiated), but the event itself
>should still be in the register.
>
>>>>>> +static irqreturn_t cdns3_device_irq_handler(int irq, void *data)
>>>>>> +{
>>>>>> + struct cdns3_device *priv_dev;
>>>>>> + struct cdns3 *cdns = data;
>>>>>> + irqreturn_t ret = IRQ_NONE;
>>>>>> + unsigned long flags;
>>>>>> + u32 reg;
>>>>>> +
>>>>>> + priv_dev = cdns->gadget_dev;
>>>>>> + spin_lock_irqsave(&priv_dev->lock, flags);
>>>>>
>>>>>the top half handler runs in hardirq context. You don't need any locks
>>>>>here. Also IRQs are *already* disabled, you don't need to disable them again.
>>>>
>>>> I will remove spin_lock_irqsave but I need to disable only some of the interrupts.
>>>> I disable interrupts associated with USB endpoints. Handling of them can be
>>>> deferred to thread handled.
>>>
>>>you should defer all of them to thread. Endpoints or otherwise.
>>
>> I will do this.
>>
>> Also I remove spin_lock_irqsave(&priv_dev->lock, flags);
>> As I remember it's not needed here.
>
>right
>
>>>>>> + /* check USB device interrupt */
>>>>>> + reg = readl(&priv_dev->regs->usb_ists);
>>>>>> +
>>>>>> + if (reg) {
>>>>>> + writel(reg, &priv_dev->regs->usb_ists);
>>>>>> + cdns3_check_usb_interrupt_proceed(priv_dev, reg);
>>>>>> + ret = IRQ_HANDLED;
>>>>>
>>>>>now, because you _don't_ mask this interrupt, you're gonna have
>>>>>issues. Say we actually get both device and endpoint interrupts while
>>>>>the thread is already running with previous endpoint interrupts. Now
>>>>>we're gonna reenter the top half, because device interrupts are *not*
>>>>>masked, which will read usb_ists and handle it here.
>>>>
>>>> Endpoint interrupts are masked in cdns3_device_irq_handler and stay masked
>>>> until they are not handled in threaded handler.
>>>
>>>Quick question, then: these ISTS registers, are they masked interrupt
>>>status or raw interrupt status?
>>
>> Yes it's masked, but after masking them the new interrupts will not be reported
>> In ISTS registers. Form this reason I can mask only reported interrupt.
>
>and what happens when you unmask the registers? Do they get reported?
No they are not reported in case of USB_ISTS register.
They should be reported in case EP_ISTS, but I need to test it.
>
>>>> Of course, not all endpoint interrupts are masked, but only reported in ep_ists.
>>>> USB interrupt will be handled immediately.
>>>>
>>>> Also, I can get next endpoint interrupt from not masked endpoint and driver also again wake
>>>> the thread. I saw such situation, but threaded interrupt handler has been working correct
>>>> in such situations.
>>>>
>>>> In thread handler driver checks again which endpoint should be handled in ep_ists.
>>>>
>>>> I think that such situation should also occurs during our LPM enter/exit test.
>>>> So, driver has been tested for such case. During this test driver during
>>>> transferring data generate a huge number of LPM interrupts which
>>>> are usb interrupts.
>>>>
>>>> I can't block usb interrupts interrupts because:
>>>> /*
>>>> * WORKAROUND: CDNS3 controller has issue with hardware resuming
>>>> * from L1. To fix it, if any DMA transfer is pending driver
>>>> * must starts driving resume signal immediately.
>>>> */
>>>
>>>I can't see why this would prevent you from defering handling to thread
>>>handler.
>>>
>>
>> I also will try to move it, but this change can has impact on performance.
>
>how much is the impact? What's the impact? Why does this impact performance?
It's only my guess.
I can't come up with the right scenario now.
Sorry, I think that it was my mistake.
>
>>>>>> + struct cdns3_aligned_buf *buf, *tmp;
>>>>>> +
>>>>>> + list_for_each_entry_safe(buf, tmp, &priv_dev->aligned_buf_list,
>>>>>> + list) {
>>>>>> + if (!buf->in_use) {
>>>>>> + list_del(&buf->list);
>>>>>> +
>>>>>> + spin_unlock_irqrestore(&priv_dev->lock, flags);
>>>>>
>>>>>creates the possibility of a race condition
>>>> Why? In this place the buf can't be used.
>>>
>>>but you're reenabling interrupts, right?
>>
>> Yes, driver frees not used buffers here.
>> I think that it's the safest place for this purpose.
>
>I guess you missed the point a little. Since you reenable interrupts
>just to free the buffer, you end up creating the possibility for a race
>condition. Specially since you don't mask all interrupt events. The
>moment you reenable interrupts, one of your not-unmasked interrupt
>sources could trigger, then top-half gets scheduled which tries to wake
>up the IRQ thread again and things go boom.
Ok, I think I understand. So I have 3 options:
1. Mask the USB_IEN and EP_IEN interrupts, but then I can lost some USB_ISTS
events. It's dangerous options.
2. Remove implementation of handling unaligned buffers and assume that
upper layer will worry about this. What with vendor specific drivers that
can be used by companies and not upstreamed ?
It could be good to have such safety mechanism even if it is not currently used.
3. Delegate this part of code for instance to separate thread that will be called
In free time.
In my opinion the third option is safest.
>
>>>>>> + dma_free_coherent(priv_dev->sysdev, buf->size,
>>>>>> + buf->buf,
>>>>>> + buf->dma);
>>>>>> + spin_lock_irqsave(&priv_dev->lock, flags);
>>>>>> +
>>>>>> + kfree(buf);
>>>>>
>>>>>why do you even need this "garbage collector"?
>>>>
>>>> I need to free not used memory. The once allocated buffer will be associated with
>>>> request, but if request.length will be increased in usb_request then driver will
>>>> must allocate the bigger buffer. As I remember I couldn't call dma_free_coherent
>>>> in interrupt context so I had to move it to thread handled. This flag was used to avoid
>>>> going through whole aligned_buf_list every time.
>>>> In most cases this part will never called int this place
>>>
>>>Did you try, btw, setting the quirk flag which tells gadget drivers to
>>>always allocate buffers aligned to MaxPacketSize? Wouldn't that be enough?
>>
>> If found only quirk_ep_out_aligned_size flag, but it align only buffer size.
>>
>> DMA used by this controller must have buffer address aligned to 8.
>> I think that on most architecture kmalloc should guarantee such aligned.
>
>right, it should be aligned on PAGE_SIZE
>
>> The problem was detected on NXP testing board.
>
>and what was the alignment on that? IIRC, ARM had the same alignment
>requirements as x86. Where you sing SLUB allocator on that NXP board,
>perhaps?
>
Peter Chan explanation:
"
This un-aligned request buffer address for 8 occurs for Ethernet Gadget (eg, NCM),
it allocates socket buffer with NET_IP_ALIGN, so the last byte of buffer address
is always 2. Although this can be workaround by setting quirk_avoids_skb_reserve,
but we are not sure if all gadget request buffers can be 8 or Max Packet Size aligned.
"
--
Pawell
^ permalink raw reply
* Re: [linux-sunxi] [PATCH v5 1/3] arm64: dts: allwinner: Add SPDIF node for Allwinner H6
From: Clément Péron @ 2019-08-12 7:11 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Maxime Ripard, Rob Herring, linux-arm-kernel, devicetree,
linux-kernel, linux-sunxi
In-Reply-To: <CAGb2v67N5Ykzo6myKqrNMgu6PCH2pJTzw9JJ5t0MGP_p=0ae9g@mail.gmail.com>
Hi Chen-Yu,
On Mon, 12 Aug 2019 at 08:35, Chen-Yu Tsai <wens@kernel.org> wrote:
>
> On Mon, Aug 12, 2019 at 12:52 PM Chen-Yu Tsai <wens@kernel.org> wrote:
> >
> > Hi,
> >
> > On Mon, Aug 12, 2019 at 4:31 AM Clément Péron <peron.clem@gmail.com> wrote:
> > >
> > > The Allwinner H6 has a SPDIF controller called OWA (One Wire Audio).
> > >
> > > Only one pinmuxing is available so set it as default.
> > >
> > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > ---
> > > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 38 ++++++++++++++++++++
> > > 1 file changed, 38 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > index 7628a7c83096..677eb374678d 100644
> > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > @@ -83,6 +83,24 @@
> > > method = "smc";
> > > };
> > >
> > > + sound-spdif {
> > > + compatible = "simple-audio-card";
> > > + simple-audio-card,name = "sun50i-h6-spdif";
> > > +
> > > + simple-audio-card,cpu {
> > > + sound-dai = <&spdif>;
> > > + };
> > > +
> > > + simple-audio-card,codec {
> > > + sound-dai = <&spdif_out>;
> > > + };
> > > + };
> > > +
> > > + spdif_out: spdif-out {
> > > + #sound-dai-cells = <0>;
> > > + compatible = "linux,spdif-dit";
> > > + };
> > > +
> >
> > We've always had this part in the board dts. It isn't relevant to boards
> > that don't have SPDIF output.
> >
> > Also, not so relevant here, but there are different simple sound card
> > constructs. Some support multiple audio streams with dynamic PCM routing.
> > How these are configured really depends on what interfaces are usable.
> >
> > So keeping this at the board level is IMO a better choice.
I Agree, I try to keep coherency with sun50i-a64.dtsi.
But sound routing is really at board level not SoC one.
Regards,
Clément
>
> Forgot to mention. Both patches and all parts in this patch are OK. It's
> just the parts the need to be moved.
>
>
> > ChenYu
> >
> >
> > > timer {
> > > compatible = "arm,armv8-timer";
> > > interrupts = <GIC_PPI 13
> > > @@ -282,6 +300,11 @@
> > > bias-pull-up;
> > > };
> > >
> > > + spdif_tx_pin: spdif-tx-pin {
> > > + pins = "PH7";
> > > + function = "spdif";
> > > + };
> > > +
> > > uart0_ph_pins: uart0-ph-pins {
> > > pins = "PH0", "PH1";
> > > function = "uart0";
> > > @@ -411,6 +434,21 @@
> > > };
> > > };
> > >
> > > + spdif: spdif@5093000 {
> > > + #sound-dai-cells = <0>;
> > > + compatible = "allwinner,sun50i-h6-spdif";
> > > + reg = <0x05093000 0x400>;
> > > + interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> > > + clocks = <&ccu CLK_BUS_SPDIF>, <&ccu CLK_SPDIF>;
> > > + clock-names = "apb", "spdif";
> > > + resets = <&ccu RST_BUS_SPDIF>;
> > > + dmas = <&dma 2>;
> > > + dma-names = "tx";
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&spdif_tx_pin>;
> > > + status = "disabled";
> > > + };
> > > +
> > > usb2otg: usb@5100000 {
> > > compatible = "allwinner,sun50i-h6-musb",
> > > "allwinner,sun8i-a33-musb";
> > > --
> > > 2.20.1
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> > > To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190811203144.5999-2-peron.clem%40gmail.com.
^ permalink raw reply
* Re: [PATCH] dt-bindings: mfd: rn5t618: Document optional property system-power-controller
From: Jonathan Neuschäfer @ 2019-08-12 6:53 UTC (permalink / raw)
To: Lee Jones
Cc: Jonathan Neuschäfer, devicetree, Rob Herring, Mark Rutland,
linux-kernel
In-Reply-To: <20190812062520.GB4594@dell>
[-- Attachment #1: Type: text/plain, Size: 583 bytes --]
On Mon, Aug 12, 2019 at 07:25:20AM +0100, Lee Jones wrote:
> On Thu, 08 Aug 2019, Jonathan Neuschäfer wrote:
> > On Fri, Feb 01, 2019 at 09:24:11AM +0000, Lee Jones wrote:
> > > On Tue, 29 Jan 2019, Jonathan Neuschäfer wrote:
> > > > Documentation/devicetree/bindings/mfd/rn5t618.txt | 5 +++++
> > > > 1 file changed, 5 insertions(+)
> > >
> > > Applied, thanks.
> >
> > Hi,
> >
> > apparently this patch got lost somehow (I can't find it in mainline or
> > -next). Should I resend it?
>
> Yes, it appears that it did.
>
> I've applied it again.
Thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 2/3] dt-bindings: eeprom: at25: Add Anvo ANV32E61W
From: Krzysztof Kozlowski @ 2019-08-12 6:43 UTC (permalink / raw)
To: Schrempf Frieder
Cc: Mark Rutland, devicetree@vger.kernel.org, Shawn Guo, Sascha Hauer,
linux-kernel@vger.kernel.org, Rob Herring, NXP Linux Team,
Pengutronix Kernel Team, Fabio Estevam,
linux-arm-kernel@lists.infradead.org, notify@kernel.org
In-Reply-To: <de032954-2b6e-5aa9-0d91-c37417c8e162@kontron.de>
On Thu, 8 Aug 2019 at 20:09, Schrempf Frieder
<frieder.schrempf@kontron.de> wrote:
>
> On 08.08.19 19:26, Krzysztof Kozlowski wrote:
> > Document the compatible for ANV32E61W EEPROM chip.
>
> This chip is actually not an EEPROM, but a SPI nvSRAM. It can be
> interfaced by the at25 driver similar to an EEPROM. This is not the
> ideal solution, but it works until there's a proper driver for such
> chips. Maybe you can add some of these details to the commit message
> here. Also there is more information on this topic here:
> https://patchwork.ozlabs.org/patch/1043950/.
Indeed, I'll correct the description of commit.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [linux-sunxi] [PATCH v5 3/3] arm64: defconfig: Enable Sun4i SPDIF module
From: Chen-Yu Tsai @ 2019-08-12 6:37 UTC (permalink / raw)
To: Clément Péron
Cc: devicetree, Maxime Ripard, linux-kernel, linux-sunxi, Rob Herring,
linux-arm-kernel
In-Reply-To: <20190811203144.5999-4-peron.clem@gmail.com>
On Mon, Aug 12, 2019 at 4:32 AM Clément Péron <peron.clem@gmail.com> wrote:
>
> Allwinner A64 and H6 use the Sun4i SPDIF driver.
>
> Enable this to allow a proper support.
>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
Applied. Thanks.
_______________________________________________
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