Devicetree
 help / color / mirror / Atom feed
* [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function
From: Paul Cercueil @ 2017-12-30 13:51 UTC (permalink / raw)
  To: Ralf Baechle, Rob Herring, Mark Rutland, Wim Van Sebroeck,
	Guenter Roeck
  Cc: devicetree, linux-mips, linux-kernel, linux-watchdog,
	Paul Cercueil
In-Reply-To: <20171230135108.6834-1-paul@crapouillou.net>

When the watchdog was configured for nowayout, and after the
userspace watchdog daemon closed the dev node without sending the
magic character, unloading this module stopped the watchdog
hardware, which was clearly a problem.

Besides, unloading the module is not possible when the userspace
watchdog daemon is running, so it's safe to assume that we don't
need to stop the watchdog hardware in the jz4740_wdt_remove()
function.

For this reason, the jz4740_wdt_remove() function can then be
dropped alltogether.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/watchdog/jz4740_wdt.c | 8 --------
 1 file changed, 8 deletions(-)

 v2: New patch in this series

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index fa7f49a3212c..02b9b8e946a2 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int jz4740_wdt_remove(struct platform_device *pdev)
-{
-	struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
-
-	return jz4740_wdt_stop(&drvdata->wdt);
-}
-
 static struct platform_driver jz4740_wdt_driver = {
 	.probe = jz4740_wdt_probe,
-	.remove = jz4740_wdt_remove,
 	.driver = {
 		.name = "jz4740-wdt",
 		.of_match_table = of_match_ptr(jz4740_wdt_of_matches),
-- 
2.11.0

^ permalink raw reply related

* [PATCH v2 3/8] watchdog: JZ4740: Register a restart handler
From: Paul Cercueil @ 2017-12-30 13:51 UTC (permalink / raw)
  To: Ralf Baechle, Rob Herring, Mark Rutland, Wim Van Sebroeck,
	Guenter Roeck
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-mips-6z/3iImG2C8G8FEW9MqTrA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-watchdog-u79uwXL29TY76Z2rM5mHXA, Paul Cercueil
In-Reply-To: <20171230135108.6834-1-paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>

The watchdog driver can restart the system by simply configuring the
hardware for a timeout of 0 seconds.

Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 drivers/watchdog/jz4740_wdt.c | 9 +++++++++
 1 file changed, 9 insertions(+)

 v2: No change

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 92d6ca8ceb49..fa7f49a3212c 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -130,6 +130,14 @@ static int jz4740_wdt_stop(struct watchdog_device *wdt_dev)
 	return 0;
 }
 
+static int jz4740_wdt_restart(struct watchdog_device *wdt_dev,
+			      unsigned long action, void *data)
+{
+	wdt_dev->timeout = 0;
+	jz4740_wdt_start(wdt_dev);
+	return 0;
+}
+
 static const struct watchdog_info jz4740_wdt_info = {
 	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
 	.identity = "jz4740 Watchdog",
@@ -141,6 +149,7 @@ static const struct watchdog_ops jz4740_wdt_ops = {
 	.stop = jz4740_wdt_stop,
 	.ping = jz4740_wdt_ping,
 	.set_timeout = jz4740_wdt_set_timeout,
+	.restart = jz4740_wdt_restart,
 };
 
 #ifdef CONFIG_OF
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v2 2/8] watchdog: jz4740: Use devm_* functions
From: Paul Cercueil @ 2017-12-30 13:51 UTC (permalink / raw)
  To: Ralf Baechle, Rob Herring, Mark Rutland, Wim Van Sebroeck,
	Guenter Roeck
  Cc: devicetree, linux-mips, linux-kernel, linux-watchdog,
	Paul Cercueil
In-Reply-To: <20171230135108.6834-1-paul@crapouillou.net>

- Use devm_clk_get instead of clk_get
- Use devm_watchdog_register_device instead of watchdog_register_device

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/watchdog/jz4740_wdt.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

 v2: No change

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 6955deb100ef..92d6ca8ceb49 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -178,40 +178,29 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	drvdata->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(drvdata->base)) {
-		ret = PTR_ERR(drvdata->base);
-		goto err_out;
-	}
+	if (IS_ERR(drvdata->base))
+		return PTR_ERR(drvdata->base);
 
-	drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
+	drvdata->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
 	if (IS_ERR(drvdata->rtc_clk)) {
 		dev_err(&pdev->dev, "cannot find RTC clock\n");
-		ret = PTR_ERR(drvdata->rtc_clk);
-		goto err_out;
+		return PTR_ERR(drvdata->rtc_clk);
 	}
 
-	ret = watchdog_register_device(&drvdata->wdt);
+	ret = devm_watchdog_register_device(&pdev->dev, &drvdata->wdt);
 	if (ret < 0)
-		goto err_disable_clk;
+		return ret;
 
 	platform_set_drvdata(pdev, drvdata);
-	return 0;
 
-err_disable_clk:
-	clk_put(drvdata->rtc_clk);
-err_out:
-	return ret;
+	return 0;
 }
 
 static int jz4740_wdt_remove(struct platform_device *pdev)
 {
 	struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
 
-	jz4740_wdt_stop(&drvdata->wdt);
-	watchdog_unregister_device(&drvdata->wdt);
-	clk_put(drvdata->rtc_clk);
-
-	return 0;
+	return jz4740_wdt_stop(&drvdata->wdt);
 }
 
 static struct platform_driver jz4740_wdt_driver = {
-- 
2.11.0

^ permalink raw reply related

* [PATCH v2 1/8] watchdog: JZ4740: Disable clock after stopping counter
From: Paul Cercueil @ 2017-12-30 13:51 UTC (permalink / raw)
  To: Ralf Baechle, Rob Herring, Mark Rutland, Wim Van Sebroeck,
	Guenter Roeck
  Cc: devicetree, linux-mips, linux-kernel, linux-watchdog,
	Paul Cercueil
In-Reply-To: <20171228162939.3928-2-paul@crapouillou.net>

Previously, the clock was disabled first, which makes the watchdog
component insensitive to register writes.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/jz4740_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

 v2: No change

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 20627f22baf6..6955deb100ef 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -124,8 +124,8 @@ static int jz4740_wdt_stop(struct watchdog_device *wdt_dev)
 {
 	struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
 
-	jz4740_timer_disable_watchdog();
 	writeb(0x0, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
+	jz4740_timer_disable_watchdog();
 
 	return 0;
 }
-- 
2.11.0

^ permalink raw reply related

* Re: [PATCH v5 03/12] dt-bindings: display: sun4i-drm: Add LVDS properties
From: Jernej Škrabec @ 2017-12-30 11:45 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Daniel Vetter, David Airlie, Chen-Yu Tsai,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Rutland, Rob Herring,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	plaes-q/aMd4JkU83YtjvyW6yDsg, icenowy-h8G6r0blFSE,
	Thomas Petazzoni, devicetree-u79uwXL29TY76Z2rM5mHXA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, Laurent Pinchart
In-Reply-To: <ddbde28fe2e4f21412974e4c69fbfe1c5ff9383f.1513854122.git-series.maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Hi Maxime,

Dne četrtek, 21. december 2017 ob 12:02:29 CET je Maxime Ripard napisal(a):
> Some clocks and resets supposed to drive the LVDS logic in the display
> engine have been overlooked when the driver was first introduced.
> 
> Add those additional resources to the binding, and we'll deal with the ABI
> stability in the code.
> 
> Reviewed-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt |  9 +++++++-
> 1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt index
> 50cc72ee1168..1e21cfaac9e2 100644
> --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> @@ -121,6 +121,15 @@ Required properties:
>  On SoCs other than the A33 and V3s, there is one more clock required:
>     - 'tcon-ch1': The clock driving the TCON channel 1
> 
> +On SoCs that support LVDS (all SoCs but the A13, H3, H5 and V3s), you
> +need one more reset line:
> +   - 'lvds': The reset line driving the LVDS logic
> +
> +And on the SoCs newer than the A31 (sun6i and sun8i families), you
> +need one more clock line:
> +   - 'lvds-alt': An alternative clock source, separate from the TCON
> channel 0 +                 clock, that can be used to drive the LVDS clock

I think this wording is imprecise, since A83T is part of the sun8i family, but 
from the code (patch 7) and DT changes (patch 9) you do, it doesn't need this 
property.

Maybe it would be just easier to enumerate all compatibles which needs this 
property? 

Best regards,
Jernej


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 1/6] phy: sun4i-usb: add support for R40 USB PHY
From: Icenowy Zheng @ 2017-12-30 11:38 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Maxime Ripard, Chen-Yu Tsai, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <020a2b04-dbcc-032a-89d1-75479dd436e7-l0cyMroinI0@public.gmane.org>

在 2017年10月18日星期三 CST 下午7:46:08,Kishon Vijay Abraham I 写道:
> On Wednesday 18 October 2017 05:12 PM, Maxime Ripard wrote:
> > On Wed, Oct 18, 2017 at 05:09:00PM +0530, Kishon Vijay Abraham I wrote:
> >> Hi,
> >> 
> >> On Tuesday 10 October 2017 02:28 AM, Maxime Ripard wrote:
> >>> On Sun, Oct 08, 2017 at 04:29:01AM +0000, Icenowy Zheng wrote:
> >>>> Allwinner R40 features a USB PHY like the one in A64, but with 3 PHYs.
> >>>> 
> >>>> Add support for it.
> >>>> 
> >>>> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> >>> 
> >>> Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> >> 
> >> Is this patch good to be merged? I see you have pending comments on the
> >> other patch in the series.
> > 
> > Yeah, but I guess you can merge this one, it's pretty harmless, and it
> > will reduce the amount of patches to review / merge later on.
> 
> Thank you for the quick reply.
> 
> merged with Maxime's and Rob's Ack.

Sorry, but I didn't see the patch appears in linux-next for such long time.

Is it lost?

> 
> Thanks
> Kishon


-- 
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
For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply

* [PATCH v4 6/6] arm64: allwinner: a64: add HDMI regulator to all DTs' simplefb_hdmi
From: Icenowy Zheng @ 2017-12-30 11:30 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai
  Cc: linux-clk, devicetree, linux-arm-kernel, linux-kernel,
	linux-sunxi, Icenowy Zheng
In-Reply-To: <20171230113043.30237-1-icenowy@aosc.io>

On usual A64 board design the power of HDMI controller is connected to
DLDO1 of the AXP803 PMIC. If this regulator is shut down, the HDMI
output will be blank. Therefore the simplefb driver should keep this
regulator on.

Add the regulator to all currently available A64 boards' simplefb_hdmi
device node.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
New patch introduced in v4.

 arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts     | 4 ++++
 arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts       | 4 ++++
 arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts        | 4 ++++
 arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts     | 4 ++++
 arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts           | 4 ++++
 arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts | 4 ++++
 6 files changed, 24 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
index a6975670cd1c..f98c496e1f30 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
@@ -283,6 +283,10 @@
 	regulator-name = "vcc-rtc";
 };
 
+&simplefb_hdmi {
+	vcc-hdmi-supply = <&reg_dldo1>;
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pins_a>;
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts
index 2beef9e6cb88..7cbbbf238f4f 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts
@@ -196,6 +196,10 @@
 	regulator-name = "vcc-rtc";
 };
 
+&simplefb_hdmi {
+	vcc-hdmi-supply = <&reg_dldo1>;
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pins_a>;
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
index 8807664f363a..568de83427d0 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
@@ -215,6 +215,10 @@
 	regulator-name = "vcc-rtc";
 };
 
+&simplefb_hdmi {
+	vcc-hdmi-supply = <&reg_dldo1>;
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pins_a>;
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts
index 240d35731d10..ce38080db324 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts
@@ -192,6 +192,10 @@
 	regulator-name = "vcc-rtc";
 };
 
+&simplefb_hdmi {
+	vcc-hdmi-supply = <&reg_dldo1>;
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pins_a>;
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
index 604cdaedac38..40d9802959c4 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
@@ -230,6 +230,10 @@
 	regulator-name = "vcc-rtc";
 };
 
+&simplefb_hdmi {
+	vcc-hdmi-supply = <&reg_dldo1>;
+};
+
 /* On Exp and Euler connectors */
 &uart0 {
 	pinctrl-names = "default";
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
index abe179de35d7..c21f2331add6 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
@@ -134,6 +134,10 @@
 	regulator-name = "vcc-wifi";
 };
 
+&simplefb_hdmi {
+	vcc-hdmi-supply = <&reg_dldo1>;
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pins_a>;
-- 
2.14.2

^ permalink raw reply related

* [PATCH v4 5/6] arm64: allwinner: a64: add simplefb for A64 SoC
From: Icenowy Zheng @ 2017-12-30 11:30 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai
  Cc: linux-clk-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Icenowy Zheng
In-Reply-To: <20171230113043.30237-1-icenowy-h8G6r0blFSE@public.gmane.org>

The A64 SoC features two display pipelines, one has a LCD output, the
other has a HDMI output.

Add support for simplefb for these pipelines on A64 SoC.

Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
---
Changes in v4:
- Dropped extra clocks.
- Added labels to the SimpleFB device tree nodes as boards may have
  extra regulator for display pipeline.

 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index fb8ea7c414e1..d803c115d362 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -42,9 +42,11 @@
  *     OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#include <dt-bindings/clock/sun8i-de2.h>
 #include <dt-bindings/clock/sun50i-a64-ccu.h>
 #include <dt-bindings/clock/sun8i-r-ccu.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/reset/sun8i-de2.h>
 #include <dt-bindings/reset/sun50i-a64-ccu.h>
 
 / {
@@ -52,6 +54,30 @@
 	#address-cells = <1>;
 	#size-cells = <1>;
 
+	chosen {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		simplefb_lcd: framebuffer-lcd {
+			compatible = "allwinner,simple-framebuffer",
+				     "simple-framebuffer";
+			allwinner,pipeline = "mixer0-lcd0";
+			clocks = <&display_clocks CLK_MIXER0>,
+				 <&ccu CLK_TCON0>;
+			status = "disabled";
+		};
+
+		simplefb_hdmi: framebuffer-hdmi {
+			compatible = "allwinner,simple-framebuffer",
+				     "simple-framebuffer";
+			allwinner,pipeline = "mixer1-lcd1-hdmi";
+			clocks = <&display_clocks CLK_MIXER1>,
+				 <&ccu CLK_TCON1>, <&ccu CLK_HDMI>;
+			status = "disabled";
+		};
+	};
+
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
-- 
2.14.2

^ permalink raw reply related

* [PATCH v4 4/6] arm64: allwinner: a64: add DE2 CCU for A64 SoC
From: Icenowy Zheng @ 2017-12-30 11:30 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai
  Cc: linux-clk, devicetree, linux-arm-kernel, linux-kernel,
	linux-sunxi, Icenowy Zheng
In-Reply-To: <20171230113043.30237-1-icenowy@aosc.io>

The A64 SoC features a DE2 CCU like the one in H5, but needs to claim a
section of SRAM (SRAM C) to be accessed.

Adds the device tree nodes for the SRAM controller and the DE2 CCU.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
Changes in v3:
- Fixed the alliwnner,sram property (the 1 after SRAM phadle is missing
  in v2).

 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 34 +++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index d783d164b9c3..fb8ea7c414e1 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -130,6 +130,40 @@
 		#size-cells = <1>;
 		ranges;
 
+		display_clocks: clock@1000000 {
+			compatible = "allwinner,sun50i-a64-de2-clk";
+			reg = <0x01000000 0x100000>;
+			clocks = <&ccu CLK_DE>,
+				 <&ccu CLK_BUS_DE>;
+			clock-names = "mod",
+				      "bus";
+			resets = <&ccu RST_BUS_DE>;
+			allwinner,sram = <&de2_sram 1>;
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+		};
+
+		sram-controller@1c00000 {
+			compatible = "allwinner,sun50i-a64-sram-controller";
+			reg = <0x01c00000 0x1000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+
+			sram_c: sram@18000 {
+				compatible = "mmio-sram";
+				reg = <0x00018000 0x28000>;
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0 0x00018000 0x28000>;
+
+				de2_sram: sram-section@0 {
+					compatible = "allwinner,sun50i-a64-sram-c";
+					reg = <0x0000 0x28000>;
+				};
+			};
+		};
+
 		syscon: syscon@1c00000 {
 			compatible = "allwinner,sun50i-a64-system-controller",
 				"syscon";
-- 
2.14.2

^ permalink raw reply related

* [PATCH v4 3/6] clk: sunxi-ng: add support for Allwinner A64 DE2 CCU
From: Icenowy Zheng @ 2017-12-30 11:30 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai
  Cc: linux-clk-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Icenowy Zheng
In-Reply-To: <20171230113043.30237-1-icenowy-h8G6r0blFSE@public.gmane.org>

Allwinner A64's DE2 needs to claim a section of SRAM (SRAM C) to work.

Add support for it.

Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
---
Changes in v4:
- Use a struct to maintain both ccu desc and quirks as Chen-Yu Tsai
  suggested.

 drivers/clk/sunxi-ng/ccu-sun8i-de2.c | 117 +++++++++++++++++++++++------------
 1 file changed, 77 insertions(+), 40 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
index 468d1abaf0ee..b65953b32bd0 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
@@ -17,6 +17,7 @@
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/reset.h>
+#include <linux/soc/sunxi/sunxi_sram.h>
 
 #include "ccu_common.h"
 #include "ccu_div.h"
@@ -156,44 +157,70 @@ static struct ccu_reset_map sun50i_a64_de2_resets[] = {
 	[RST_WB]	= { 0x08, BIT(2) },
 };
 
-static const struct sunxi_ccu_desc sun8i_a83t_de2_clk_desc = {
-	.ccu_clks	= sun8i_a83t_de2_clks,
-	.num_ccu_clks	= ARRAY_SIZE(sun8i_a83t_de2_clks),
+struct de2_ccu {
+	struct sunxi_ccu_desc desc;
+	bool sram_needed;
+};
+
+static const struct de2_ccu sun8i_a83t_de2_clk = {
+	.desc = {
+		.ccu_clks	= sun8i_a83t_de2_clks,
+		.num_ccu_clks	= ARRAY_SIZE(sun8i_a83t_de2_clks),
+
+		.hw_clks	= &sun8i_a83t_de2_hw_clks,
+
+		.resets		= sun8i_a83t_de2_resets,
+		.num_resets	= ARRAY_SIZE(sun8i_a83t_de2_resets),
+	},
+};
+
+static const struct de2_ccu sun8i_h3_de2_clk = {
+	.desc = {
+		.ccu_clks	= sun8i_h3_de2_clks,
+		.num_ccu_clks	= ARRAY_SIZE(sun8i_h3_de2_clks),
 
-	.hw_clks	= &sun8i_a83t_de2_hw_clks,
+		.hw_clks	= &sun8i_h3_de2_hw_clks,
 
-	.resets		= sun8i_a83t_de2_resets,
-	.num_resets	= ARRAY_SIZE(sun8i_a83t_de2_resets),
+		.resets		= sun8i_a83t_de2_resets,
+		.num_resets	= ARRAY_SIZE(sun8i_a83t_de2_resets),
+	},
 };
 
-static const struct sunxi_ccu_desc sun8i_h3_de2_clk_desc = {
-	.ccu_clks	= sun8i_h3_de2_clks,
-	.num_ccu_clks	= ARRAY_SIZE(sun8i_h3_de2_clks),
+static const struct de2_ccu sun50i_a64_de2_clk = {
+	.desc = {
+		.ccu_clks	= sun8i_h3_de2_clks,
+		.num_ccu_clks	= ARRAY_SIZE(sun8i_h3_de2_clks),
 
-	.hw_clks	= &sun8i_h3_de2_hw_clks,
+		.hw_clks	= &sun8i_h3_de2_hw_clks,
 
-	.resets		= sun8i_a83t_de2_resets,
-	.num_resets	= ARRAY_SIZE(sun8i_a83t_de2_resets),
+		.resets		= sun50i_a64_de2_resets,
+		.num_resets	= ARRAY_SIZE(sun50i_a64_de2_resets),
+	},
+	.sram_needed = true,
 };
 
-static const struct sunxi_ccu_desc sun50i_a64_de2_clk_desc = {
-	.ccu_clks	= sun8i_h3_de2_clks,
-	.num_ccu_clks	= ARRAY_SIZE(sun8i_h3_de2_clks),
+static const struct de2_ccu sun50i_h5_de2_clk = {
+	.desc = {
+		.ccu_clks	= sun8i_h3_de2_clks,
+		.num_ccu_clks	= ARRAY_SIZE(sun8i_h3_de2_clks),
 
-	.hw_clks	= &sun8i_h3_de2_hw_clks,
+		.hw_clks	= &sun8i_h3_de2_hw_clks,
 
-	.resets		= sun50i_a64_de2_resets,
-	.num_resets	= ARRAY_SIZE(sun50i_a64_de2_resets),
+		.resets		= sun50i_a64_de2_resets,
+		.num_resets	= ARRAY_SIZE(sun50i_a64_de2_resets),
+	},
 };
 
-static const struct sunxi_ccu_desc sun8i_v3s_de2_clk_desc = {
-	.ccu_clks	= sun8i_v3s_de2_clks,
-	.num_ccu_clks	= ARRAY_SIZE(sun8i_v3s_de2_clks),
+static const struct de2_ccu sun8i_v3s_de2_clk = {
+	.desc = {
+		.ccu_clks	= sun8i_v3s_de2_clks,
+		.num_ccu_clks	= ARRAY_SIZE(sun8i_v3s_de2_clks),
 
-	.hw_clks	= &sun8i_v3s_de2_hw_clks,
+		.hw_clks	= &sun8i_v3s_de2_hw_clks,
 
-	.resets		= sun8i_a83t_de2_resets,
-	.num_resets	= ARRAY_SIZE(sun8i_a83t_de2_resets),
+		.resets		= sun8i_a83t_de2_resets,
+		.num_resets	= ARRAY_SIZE(sun8i_a83t_de2_resets),
+	},
 };
 
 static int sunxi_de2_clk_probe(struct platform_device *pdev)
@@ -202,11 +229,11 @@ static int sunxi_de2_clk_probe(struct platform_device *pdev)
 	struct clk *bus_clk, *mod_clk;
 	struct reset_control *rstc;
 	void __iomem *reg;
-	const struct sunxi_ccu_desc *ccu_desc;
+	const struct de2_ccu *ccu;
 	int ret;
 
-	ccu_desc = of_device_get_match_data(&pdev->dev);
-	if (!ccu_desc)
+	ccu = of_device_get_match_data(&pdev->dev);
+	if (!ccu)
 		return -EINVAL;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -239,11 +266,20 @@ static int sunxi_de2_clk_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	if (ccu->sram_needed) {
+		ret = sunxi_sram_claim(&pdev->dev);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"Error couldn't map SRAM to device\n");
+			return ret;
+		}
+	}
+
 	/* The clocks need to be enabled for us to access the registers */
 	ret = clk_prepare_enable(bus_clk);
 	if (ret) {
 		dev_err(&pdev->dev, "Couldn't enable bus clk: %d\n", ret);
-		return ret;
+		goto err_release_sram;
 	}
 
 	ret = clk_prepare_enable(mod_clk);
@@ -260,7 +296,7 @@ static int sunxi_de2_clk_probe(struct platform_device *pdev)
 		goto err_disable_mod_clk;
 	}
 
-	ret = sunxi_ccu_probe(pdev->dev.of_node, reg, ccu_desc);
+	ret = sunxi_ccu_probe(pdev->dev.of_node, reg, &ccu->desc);
 	if (ret)
 		goto err_assert_reset;
 
@@ -272,33 +308,34 @@ static int sunxi_de2_clk_probe(struct platform_device *pdev)
 	clk_disable_unprepare(mod_clk);
 err_disable_bus_clk:
 	clk_disable_unprepare(bus_clk);
+err_release_sram:
+	if (ccu->sram_needed)
+		sunxi_sram_release(&pdev->dev);
+
 	return ret;
 }
 
 static const struct of_device_id sunxi_de2_clk_ids[] = {
 	{
 		.compatible = "allwinner,sun8i-a83t-de2-clk",
-		.data = &sun8i_a83t_de2_clk_desc,
+		.data = &sun8i_a83t_de2_clk,
 	},
 	{
 		.compatible = "allwinner,sun8i-h3-de2-clk",
-		.data = &sun8i_h3_de2_clk_desc,
+		.data = &sun8i_h3_de2_clk,
 	},
 	{
 		.compatible = "allwinner,sun8i-v3s-de2-clk",
-		.data = &sun8i_v3s_de2_clk_desc,
+		.data = &sun8i_v3s_de2_clk,
+	},
+	{
+		.compatible = "allwinner,sun50i-a64-de2-clk",
+		.data = &sun50i_a64_de2_clk,
 	},
 	{
 		.compatible = "allwinner,sun50i-h5-de2-clk",
-		.data = &sun50i_a64_de2_clk_desc,
+		.data = &sun50i_h5_de2_clk,
 	},
-	/*
-	 * The Allwinner A64 SoC needs some bit to be poke in syscon to make
-	 * DE2 really working.
-	 * So there's currently no A64 compatible here.
-	 * H5 shares the same reset line with A64, so here H5 is using the
-	 * clock description of A64.
-	 */
 	{ }
 };
 
-- 
2.14.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v4 2/6] dt-bindings: add binding for A64 DE2 CCU SRAM
From: Icenowy Zheng @ 2017-12-30 11:30 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai
  Cc: linux-clk-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Icenowy Zheng
In-Reply-To: <20171230113043.30237-1-icenowy-h8G6r0blFSE@public.gmane.org>

A64's Display Engine 2.0 needs a section of SRAM (SRAM C) to be claimed,
otherwise the whole DE2 memory zone cannot be accessed (kept to all 0).

Add binding for this, in order to make the DE2 CCU able to claim the
SRAM and enable access to the DE2 clock and reset registers.

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
---
Changes in v4:
- Added Rob's ACK.

Changes in v2:
- Adds description of the situation when the SRAM is not claimed.

 Documentation/devicetree/bindings/clock/sun8i-de2.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/sun8i-de2.txt b/Documentation/devicetree/bindings/clock/sun8i-de2.txt
index f2fa87c4765c..a7d558a2b9b2 100644
--- a/Documentation/devicetree/bindings/clock/sun8i-de2.txt
+++ b/Documentation/devicetree/bindings/clock/sun8i-de2.txt
@@ -6,6 +6,7 @@ Required properties :
 		- "allwinner,sun8i-a83t-de2-clk"
 		- "allwinner,sun8i-h3-de2-clk"
 		- "allwinner,sun8i-v3s-de2-clk"
+		- "allwinner,sun50i-a64-de2-clk"
 		- "allwinner,sun50i-h5-de2-clk"
 
 - reg: Must contain the registers base address and length
@@ -18,6 +19,10 @@ Required properties :
 - #clock-cells : must contain 1
 - #reset-cells : must contain 1
 
+Additional required properties for "allwinner,sun50i-a64-de2-clk" :
+- allwinner,sram: See Documentation/devicetree/bindings/sram/sunxi-sram.txt,
+		  should be the SRAM C section on A64 SoC.
+
 Example:
 de2_clocks: clock@1000000 {
 	compatible = "allwinner,sun8i-h3-de2-clk";
-- 
2.14.2

^ permalink raw reply related

* [PATCH v4 1/6] ARM: sunxi: h3/h5: add simplefb nodes
From: Icenowy Zheng @ 2017-12-30 11:30 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai
  Cc: linux-clk-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Icenowy Zheng
In-Reply-To: <20171230113043.30237-1-icenowy-h8G6r0blFSE@public.gmane.org>

The H3/H5 SoCs have a HDMI output and a TV Composite output.

Add simplefb nodes for these outputs.

Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
---
Changes in v4:
- Dropped extra clocks (bus clocks and HDMI DDC clocks), only keep the
  clocks that are needed to display framebuffer to the monitor.

 arch/arm/boot/dts/sunxi-h3-h5.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
index fcb909658cf0..7a83b15225c7 100644
--- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
+++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
@@ -53,6 +53,30 @@
 	#address-cells = <1>;
 	#size-cells = <1>;
 
+	chosen {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		framebuffer-hdmi {
+			compatible = "allwinner,simple-framebuffer",
+				     "simple-framebuffer";
+			allwinner,pipeline = "mixer0-lcd0-hdmi";
+			clocks = <&display_clocks CLK_MIXER0>,
+				 <&ccu CLK_TCON0>, <&ccu CLK_HDMI>;
+			status = "disabled";
+		};
+
+		framebuffer-tve {
+			compatible = "allwinner,simple-framebuffer",
+				     "simple-framebuffer";
+			allwinner,pipeline = "mixer1-lcd1-tve";
+			clocks = <&display_clocks CLK_MIXER1>,
+				 <&ccu CLK_TVE>;
+			status = "disabled";
+		};
+	};
+
 	clocks {
 		#address-cells = <1>;
 		#size-cells = <1>;
-- 
2.14.2

^ permalink raw reply related

* [PATCH v4 0/6] Allwinner H3/H5/A64(DE2) SimpleFB support (Part 2)
From: Icenowy Zheng @ 2017-12-30 11:30 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai
  Cc: linux-clk-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Icenowy Zheng

This patchset is the remaining part of Allwinner DE2 SimpleFB support in
Linux. Some H3/5-related patches are already applied into the sunxi
tree.

PATCH 1 adds the final SimpleFB nodes to the H3/H5 device tree. With it
applied the SimpleFB will be usable on H3/H5.

PATCH 2/3 are for the SRAM requirment of the A64 SoC; PATCH 2 is the
device tree binding of it and PATCH 3 is the implementation in DE2 CCU
of it.

PATCH 4/5 are A64 DTSI file parts; PATCH 4 adds DE2 CCU device node and
PATCH 5 adds SimpleFB device node.

PATCH 6 are A64 per-board DTS file changes, which keeps the regulator
for HVCC pin (a power supply pin on A64 SoC that is used by the HDMI
controller) on these boards.

Icenowy Zheng (6):
  ARM: sunxi: h3/h5: add simplefb nodes
  dt-bindings: add binding for A64 DE2 CCU SRAM
  clk: sunxi-ng: add support for Allwinner A64 DE2 CCU
  arm64: allwinner: a64: add DE2 CCU for A64 SoC
  arm64: allwinner: a64: add simplefb for A64 SoC
  arm64: allwinner: a64: add HDMI regulator to all DTs' simplefb_hdmi

 .../devicetree/bindings/clock/sun8i-de2.txt        |   5 +
 arch/arm/boot/dts/sunxi-h3-h5.dtsi                 |  24 +++++
 .../boot/dts/allwinner/sun50i-a64-bananapi-m64.dts |   4 +
 .../boot/dts/allwinner/sun50i-a64-nanopi-a64.dts   |   4 +
 .../boot/dts/allwinner/sun50i-a64-olinuxino.dts    |   4 +
 .../boot/dts/allwinner/sun50i-a64-orangepi-win.dts |   4 +
 .../arm64/boot/dts/allwinner/sun50i-a64-pine64.dts |   4 +
 .../dts/allwinner/sun50i-a64-sopine-baseboard.dts  |   4 +
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi      |  60 +++++++++++
 drivers/clk/sunxi-ng/ccu-sun8i-de2.c               | 117 ++++++++++++++-------
 10 files changed, 190 insertions(+), 40 deletions(-)

-- 
2.14.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH] Input: fix semicolon.cocci warnings
From: kbuild test robot @ 2017-12-30  3:15 UTC (permalink / raw)
  Cc: kbuild-all, dmitry.torokhov, robh+dt, mark.rutland, linux,
	maxime.ripard, wens, linux-arm-kernel, linux-input, devicetree,
	linux-kernel, mylene.josserand, thomas.petazzoni, quentin.schulz
In-Reply-To: <20171228163336.28131-2-mylene.josserand@free-electrons.com>

From: Fengguang Wu <fengguang.wu@intel.com>

drivers/input/touchscreen/edt-ft5x06.c:1004:2-3: Unneeded semicolon


 Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

Fixes: 5969d946e8aa ("Input: edt-ft5x06 - Add support for regulator")
CC: Mylène Josserand <mylene.josserand@free-electrons.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 edt-ft5x06.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1001,7 +1001,7 @@ static int edt_ft5x06_ts_probe(struct i2
 		dev_err(&client->dev, "failed to request regulator: %d\n",
 			error);
 		return error;
-	};
+	}
 
 	if (tsdata->vcc) {
 		error = regulator_enable(tsdata->vcc);

^ permalink raw reply

* Re: [PATCH v2 1/2] Input: edt-ft5x06 - Add support for regulator
From: kbuild test robot @ 2017-12-30  3:15 UTC (permalink / raw)
  Cc: kbuild-all, dmitry.torokhov, robh+dt, mark.rutland, linux,
	maxime.ripard, wens, linux-arm-kernel, linux-input, devicetree,
	linux-kernel, mylene.josserand, thomas.petazzoni, quentin.schulz
In-Reply-To: <20171228163336.28131-2-mylene.josserand@free-electrons.com>

Hi Mylène,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.15-rc5 next-20171222]
[cannot apply to input/next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Myl-ne-Josserand/sun8i-a83t-Add-touchscreen-support-on-TBS-A711/20171230-091331
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next


coccinelle warnings: (new ones prefixed by >>)

>> drivers/input/touchscreen/edt-ft5x06.c:1004:2-3: Unneeded semicolon

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

^ permalink raw reply

* Re: [RFC PATCH v12 1/5] dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
From: Rafael J. Wysocki @ 2017-12-30  0:31 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Jeffy Chen, Linux Kernel Mailing List, Bjorn Helgaas, Linux PM,
	Shawn Lin, Brian Norris, Rafael J. Wysocki, Doug Anderson,
	devicetree@vger.kernel.org, Linux PCI, Rob Herring, Mark Rutland
In-Reply-To: <CAJZ5v0iRRAQWSji0ioJ5B5M5RjbYAyb1VmuWhTZw4qy0G7_a4g@mail.gmail.com>

On Sat, Dec 30, 2017 at 12:50 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Dec 29, 2017 at 6:57 PM, Tony Lindgren <tony@atomide.com> wrote:
>> * Jeffy Chen <jeffy.chen@rock-chips.com> [171226 02:11]:
>>> We are going to handle PCIe WAKE# pin for PCI devices in the pci core,
>>> so add definitions of the optional PCIe WAKE# pin for PCI devices.
>>>
>>> Also add an definition of the optional PCI interrupt pin for PCI
>>> devices to distinguish it from the PCIe WAKE# pin.
>>
>>> --- a/Documentation/devicetree/bindings/pci/pci.txt
>>> +++ b/Documentation/devicetree/bindings/pci/pci.txt
>>> @@ -24,3 +24,13 @@ driver implementation may support the following properties:
>>>     unsupported link speed, for instance, trying to do training for
>>>     unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
>>>     for gen2, and '1' for gen1. Any other values are invalid.
>>> +
>>> +PCI devices may support the following properties:
>>
>> This should say PCI ports instead of PCI devices.
>
> No, it is more accurate to say "PCI devices".
>
> Well, it actually gets somewhat confusing, because in the PCI
> terminology a "PCI device" means a physical piece of hardware that can
> be put into a single "slot" (think socket on a board) and may consist
> up to 8 functional units called "functions" which are each represented
> by a struct pci_dev.  So there may be up to 8 struct pci_dev objects
> per "PCI device" (as per the standard language) and, BTW, drivers bind
> to functions (via the struct pci_dev objects).
>
> Now, WAKE# is shared by all functions within the same "PCI device"
> (I'm not sure if the standard specifies that directly, but at least it
> appears to be treated as an obvious physical limitation), so it may be
> useful to represent the "slot" or "device" level in the DT even though
> it has no struct device based representation in the kernel.

Within the convention that bridges represent "everything below them"
as far as WAKE# is concerned, it can say "The following properties may
be provided for PCI bridges:" and the description below should explain
the convention.

Thanks,
Rafael

^ permalink raw reply

* Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF
From: Rafael J. Wysocki @ 2017-12-30  0:21 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: JeffyChen, Linux Kernel Mailing List, Bjorn Helgaas, Linux PM,
	Shawn Lin, Brian Norris, Doug Anderson,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux PCI,
	Rob Herring, Frank Rowand
In-Reply-To: <CAJZ5v0icePurJoGdVtX06j=XHPdZSqXgm+vhL47ngv7OZoL3fw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On Sat, Dec 30, 2017 at 12:39 AM, Rafael J. Wysocki <rafael-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Dec 29, 2017 at 6:15 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
>> * Rafael J. Wysocki <rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org> [171228 17:33]:
>>> On Thursday, December 28, 2017 5:51:34 PM CET Tony Lindgren wrote:
>>> >
>>> > Well Brian had a concern where we would have to implement PM runtime
>>> > for all device drivers for PCI devices.
>>>
>>> Why would we?
>>
>> Seems at least I was a bit confused. In the PCIe case the WAKE# is
>> owned by the PCIe slot, not the child PCIe device.
>
> Well, it depends on what you mean by "slot" and "child device", but if
> my understanding of it is correct, WAKE# actually is "owned" by the
> latter.
>
> First, let me clarify the terminology.  PCI slots are not really
> represented in the device hierarchy in Linux.  They are represented by
> kernel objects for hotplug purposes, but these objects are not based
> on struct device.
>
> Generally, there are two kinds of PCI entities represented by struct
> pci_dev, bridges and endpoints (functions).  Bridges may represent
> physical devices, like PCI-to-PCI bridges, or parts of physical
> devices, like PCIe ports.  In PCIe, every port is logically
> represented by a bridge (and a PCI config space region with a Type 1
> header).  However, ports do not take actions like generating
> interrupts; the pieces of hardware containing them (switches, Root
> Complex) do that.
>
> Endpoints (functions) are children of bridges (e.g. PCIe ports) and
> bridges may be children of other bridges (like in a switch that is
> represented by a bus segment with one upstream bridge - the upstream
> port - and possibly multiple downstream bridges - downstream ports).
> So in PCI a parent always is a bridge (either a PCI bridge - a bridge
> between to PCI bus segments - or a host bridge) and if that is a PCIe
> port, it cannot "own" anything like WAKE#, because it is not affected
> by it in any way and doesn't take part in the handling of it.
>
> In the context of "Figure 5-4" in the spec, Case 1, what matters is
> that every "Slot" in the figure represents a bunch (up to 8) of
> endpoints (functions), but the "Slot" is not the parent of them.  The
> port of the switch the "Slot" is connected to is the parent.  WAKE#
> basically comes from one of the endpoints belonging to the "Slot" and
> you need to look into the config space regions for all of these
> endpoints to check which one has PME Status set and clear it (to
> acknowledge the PME and make the hardware stop asserting the WAKE#
> signal).  So, from the software perspective, the endpoint (child) is
> the source of WAKE# and that should be reflected by DT properties IMO.
>
>> So you're right, there should be no need for the child PCIe device drivers to
>> implement runtime PM.
>
> There should be no need for that regardless.  You only need an
> interrupt handler that will look for the endpoint with PME Status set,
> acknowledge it and possibly invoke runtime PM for the endpoint in
> question (if supported).  All of that is standard and can happen at
> the bus type level and the interrupt handler I'm talking about may be
> based on pci_pme_wakeup() or pci_acpi_wake_dev().
>
>> I was thinking the wakeirq case with WLAN on SDIO bus. Some WLAN
>> devices can have a hardwired OOB wakeirq wired to a GPIO controller.
>> In that case the wakeirq is owned by the child device driver
>> (WLAN controller) and not by the SDIO slot. I was earlier
>> thinking this is the same as the "Figure 5-4" case 1, but it's
>> not.
>
> Well, it is not in the sense that the endpoint driver is not expected
> to handle the wakeup interrupt by itself.  The PCI bus type is
> responsible for that, but technically WAKE# comes from the endpoint
> (child).
>
>> So in the PCIe WAKE# case for device tree, we must have the
>> wakeirq property for the PCIe slot for the struct device managing
>> that slot,
>
> Which doesn't exist.
>
>> and not for the child device driver. I think it's
>> already this way in the most recent set of patches, I need to
>> look again.
>
> No, you need a wakeirq properly for the child *device* and that
> property will be consumed by the PCI layer.

Or, if you use the convention mentioned in another message in this
thread, you can make the wakeirq be a property of a bridge (port) with
the clarification of the assumption that WAKE# is shared by all
functions below the bridge.  So the presence of the "wakeirq" property
for a bridge (in addition to providing the wakeup IRQ) will mean that
it applies to all devices below the bridge.

In the case of parallel PCI (not PCIe), there may be multiple "slots"
(or "PCI devices" consisting each of multiple functions) under one
bridge and in theory each of them may use a different IRQ for WAKE#
signaling, so the above convention will not work then.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [RFC PATCH v12 5/5] arm64: dts: rockchip: Move PCIe WAKE# irq to pcie port for Gru
From: Rafael J. Wysocki @ 2017-12-30  0:10 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Jeffy Chen, Linux Kernel Mailing List, Bjorn Helgaas, Linux PM,
	Shawn Lin, Brian Norris, Rafael J. Wysocki, Doug Anderson,
	Matthias Kaehlcke, Heiko Stuebner, devicetree@vger.kernel.org,
	Klaus Goger, linux-rockchip, Rob Herring, linux-arm-kernel,
	Will Deacon, Mark Rutland, Catalin Marinas
In-Reply-To: <20171229175539.GK3875@atomide.com>

On Fri, Dec 29, 2017 at 6:55 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Jeffy Chen <jeffy.chen@rock-chips.com> [171226 02:41]:
>> Currently we are handling PCIe WAKE# irq in mrvl wifi driver.
>>
>> Move it to rockchip pcie port since we are going to handle it in the
>> pci core.
>
> Yes in the PCIe case, the pcie port node is the right place for
> the wakeirq instead of the child the mvl_wifi node. So one
> question further down below to verify this..

You seem to be using a convention by which the port represents the
whole "slot" or "PCI device" (as an entity consisting of up to 8
functions) connected to it.

That is fair enough as long as the port is not the top of a more
complex branch of the PCIe hierarchy, so maybe that case needs to be
made special somehow?

Also, I would document the convention by mentioning that the wakeup
signaled via that interrupt doesn't apply to the port itself, but to
the functions (endpoints) below it.

>> Also avoid this irq been considered as the PCI interrupt pin in the
>> of_irq_parse_pci().
>
> The above paragraph needs a bit more clarification to be
> readable :)
>
>> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
>> @@ -719,15 +719,16 @@ ap_i2c_audio: &i2c8 {
>>               #size-cells = <2>;
>>               ranges;
>>
>> +             interrupts-extended = <&pcie0 1>, <&gpio0 8 IRQ_TYPE_LEVEL_LOW>;
>> +             interrupt-names = "pci", "wakeup";
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&wlan_host_wake_l>;
>> +             wakeup-source;
>> +
>>               mvl_wifi: wifi@0,0 {
>>                       compatible = "pci1b4b,2b42";
>>                       reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
>>                              0x83010000 0x0 0x00100000 0x0 0x00100000>;
>> -                     interrupt-parent = <&gpio0>;
>> -                     interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
>> -                     pinctrl-names = "default";
>> -                     pinctrl-0 = <&wlan_host_wake_l>;
>> -                     wakeup-source;
>>               };
>>       };
>>  };
>
> So the above modifies pcie@0,0 node. And that node describes
> the particular PCIe port that the WLAN is connected to instead
> of describing the whole PCIe controller device, right?
>
> If so, then yeah it's totally where the wakeirq should be
> defined for a PCIe device in the dts file :)

As long as the convention used here is clear to everybody, that is.

Thanks,
Rafael

^ permalink raw reply

* Re: [RFC PATCH v12 1/5] dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
From: Rafael J. Wysocki @ 2017-12-29 23:50 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Jeffy Chen, Linux Kernel Mailing List, Bjorn Helgaas, Linux PM,
	Shawn Lin, Brian Norris, Rafael J. Wysocki, Doug Anderson,
	devicetree@vger.kernel.org, Linux PCI, Rob Herring, Mark Rutland
In-Reply-To: <20171229175717.GL3875@atomide.com>

On Fri, Dec 29, 2017 at 6:57 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Jeffy Chen <jeffy.chen@rock-chips.com> [171226 02:11]:
>> We are going to handle PCIe WAKE# pin for PCI devices in the pci core,
>> so add definitions of the optional PCIe WAKE# pin for PCI devices.
>>
>> Also add an definition of the optional PCI interrupt pin for PCI
>> devices to distinguish it from the PCIe WAKE# pin.
>
>> --- a/Documentation/devicetree/bindings/pci/pci.txt
>> +++ b/Documentation/devicetree/bindings/pci/pci.txt
>> @@ -24,3 +24,13 @@ driver implementation may support the following properties:
>>     unsupported link speed, for instance, trying to do training for
>>     unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
>>     for gen2, and '1' for gen1. Any other values are invalid.
>> +
>> +PCI devices may support the following properties:
>
> This should say PCI ports instead of PCI devices.

No, it is more accurate to say "PCI devices".

Well, it actually gets somewhat confusing, because in the PCI
terminology a "PCI device" means a physical piece of hardware that can
be put into a single "slot" (think socket on a board) and may consist
up to 8 functional units called "functions" which are each represented
by a struct pci_dev.  So there may be up to 8 struct pci_dev objects
per "PCI device" (as per the standard language) and, BTW, drivers bind
to functions (via the struct pci_dev objects).

Now, WAKE# is shared by all functions within the same "PCI device"
(I'm not sure if the standard specifies that directly, but at least it
appears to be treated as an obvious physical limitation), so it may be
useful to represent the "slot" or "device" level in the DT even though
it has no struct device based representation in the kernel.

>> +- interrupts: Interrupt specifier for each name in interrupt-names.
>> +- interrupt-names:
>> +    May contain "wakeup" for PCIe WAKE# interrupt and "pci" for PCI interrupt.
>> +    The PCI devices may optionally include an 'interrupts' property that
>> +    represents the legacy PCI interrupt. And when we try to specify the PCIe
>> +    WAKE# pin, a corresponding 'interrupt-names' property is required to
>> +    distinguish them.
>> --
>> 2.11.0
>>
>>

^ permalink raw reply

* Re: [RFC PATCH v11 4/5] PCI / PM: Add support for the PCIe WAKE# signal for OF
From: Rafael J. Wysocki @ 2017-12-29 23:39 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, JeffyChen,
	Linux Kernel Mailing List, Bjorn Helgaas, Linux PM, Shawn Lin,
	Brian Norris, Doug Anderson, devicetree@vger.kernel.org,
	Linux PCI, Rob Herring, Frank Rowand
In-Reply-To: <20171229171548.GI3875@atomide.com>

On Fri, Dec 29, 2017 at 6:15 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Rafael J. Wysocki <rjw@rjwysocki.net> [171228 17:33]:
>> On Thursday, December 28, 2017 5:51:34 PM CET Tony Lindgren wrote:
>> >
>> > Well Brian had a concern where we would have to implement PM runtime
>> > for all device drivers for PCI devices.
>>
>> Why would we?
>
> Seems at least I was a bit confused. In the PCIe case the WAKE# is
> owned by the PCIe slot, not the child PCIe device.

Well, it depends on what you mean by "slot" and "child device", but if
my understanding of it is correct, WAKE# actually is "owned" by the
latter.

First, let me clarify the terminology.  PCI slots are not really
represented in the device hierarchy in Linux.  They are represented by
kernel objects for hotplug purposes, but these objects are not based
on struct device.

Generally, there are two kinds of PCI entities represented by struct
pci_dev, bridges and endpoints (functions).  Bridges may represent
physical devices, like PCI-to-PCI bridges, or parts of physical
devices, like PCIe ports.  In PCIe, every port is logically
represented by a bridge (and a PCI config space region with a Type 1
header).  However, ports do not take actions like generating
interrupts; the pieces of hardware containing them (switches, Root
Complex) do that.

Endpoints (functions) are children of bridges (e.g. PCIe ports) and
bridges may be children of other bridges (like in a switch that is
represented by a bus segment with one upstream bridge - the upstream
port - and possibly multiple downstream bridges - downstream ports).
So in PCI a parent always is a bridge (either a PCI bridge - a bridge
between to PCI bus segments - or a host bridge) and if that is a PCIe
port, it cannot "own" anything like WAKE#, because it is not affected
by it in any way and doesn't take part in the handling of it.

In the context of "Figure 5-4" in the spec, Case 1, what matters is
that every "Slot" in the figure represents a bunch (up to 8) of
endpoints (functions), but the "Slot" is not the parent of them.  The
port of the switch the "Slot" is connected to is the parent.  WAKE#
basically comes from one of the endpoints belonging to the "Slot" and
you need to look into the config space regions for all of these
endpoints to check which one has PME Status set and clear it (to
acknowledge the PME and make the hardware stop asserting the WAKE#
signal).  So, from the software perspective, the endpoint (child) is
the source of WAKE# and that should be reflected by DT properties IMO.

> So you're right, there should be no need for the child PCIe device drivers to
> implement runtime PM.

There should be no need for that regardless.  You only need an
interrupt handler that will look for the endpoint with PME Status set,
acknowledge it and possibly invoke runtime PM for the endpoint in
question (if supported).  All of that is standard and can happen at
the bus type level and the interrupt handler I'm talking about may be
based on pci_pme_wakeup() or pci_acpi_wake_dev().

> I was thinking the wakeirq case with WLAN on SDIO bus. Some WLAN
> devices can have a hardwired OOB wakeirq wired to a GPIO controller.
> In that case the wakeirq is owned by the child device driver
> (WLAN controller) and not by the SDIO slot. I was earlier
> thinking this is the same as the "Figure 5-4" case 1, but it's
> not.

Well, it is not in the sense that the endpoint driver is not expected
to handle the wakeup interrupt by itself.  The PCI bus type is
responsible for that, but technically WAKE# comes from the endpoint
(child).

> So in the PCIe WAKE# case for device tree, we must have the
> wakeirq property for the PCIe slot for the struct device managing
> that slot,

Which doesn't exist.

> and not for the child device driver. I think it's
> already this way in the most recent set of patches, I need to
> look again.

No, you need a wakeirq properly for the child *device* and that
property will be consumed by the PCI layer.

>> > So isn't my option 1 above similar to the PCIe spec "Figure 5-4"
>> > case 2?
>>
>> No, it isn't, because in that case there is no practical difference
>> between WAKE# and an in-band PME message sent by the device (Beacon)
>> from the software perspective.
>>
>> WAKE# causes the switch to send a PME message upstream and that is
>> handled by the Root Complex through the standard mechanism already
>> supported by our existing PME driver (drivers/pci/pcie/pme.c).
>
> OK. So if "Figure 5-4" case 2 is already handled then and we need
> to just deal with "Figure 5-4" case 1 :)

Right.

>> > Yeah. FYI, for the dedicated wakeirq cases I have, we need to keep
>> > them masked during runtime to avoid tons of interrupts as they
>> > are often wired to the RX pins.
>>
>> OK
>>
>> BTW, enable_irq_wake() should take care of the sharing, shouldn't it?
>
> That can be used to tell us which device has wakeirq enabled for
> wake-up events, but only for resume not runtiem PM. We still have the
> shared IRQ problem to deal with. And the PCIe subsystem still needs
> to go through the child devices.

Right.

It actually has to walk the bus below each of them too in case it is a
bridge, like pci_acpi_wake_dev().

>> But the WAKE# thing is not just for waking up the system from sleep states,
>> it is for runtime PM's wakeup signaling too.
>
> Yes my test cases have it working for runtime PM and for waking
> up system from suspend.

OK

>> > > > Currently nothing happens with wakeirqs if there's no struct
>> > > > wakeup_source. On device_wakeup_enable() we call device_wakeup_attach()
>> > > > that just copies dev->power.wakeirq to ws->wakeirq. And when struct
>> > > > wake_source is freed the device should be active and wakeirq
>> > > > disabled. Or are you seeing other issues here?
>> > >
>> > > I'm suspicious about one thing, but I need to look deeper into the code. :-)
>>
>> So we are fine except for the race and we need the wakeirq field in wakeup
>> sources to automatically arm the wakeup IRQs during suspend.
>
> OK.
>
>> If I'm not mistaken, we only need something like the patch below (untested).
>
> Seems like it should fix the race, I'll do some testing next week.

OK, thanks!

Rafael

^ permalink raw reply

* Re: [PATCH v2 6/9] PCI: cadence: Add host driver for Cadence PCIe controller
From: Cyrille Pitchen @ 2017-12-29 22:08 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: bhelgaas-hpIqsD4AKlfQT0dZR+AlfA, kishon-l0cyMroinI0,
	lorenzo.pieralisi-5wv7dgnIgG8, linux-pci-u79uwXL29TY76Z2rM5mHXA,
	adouglas-vna1KIf7WgpBDgjK7y7TUQ, stelford-vna1KIf7WgpBDgjK7y7TUQ,
	dgary-vna1KIf7WgpBDgjK7y7TUQ, kgopi-vna1KIf7WgpBDgjK7y7TUQ,
	eandrews-vna1KIf7WgpBDgjK7y7TUQ,
	thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	sureshp-vna1KIf7WgpBDgjK7y7TUQ, nsekhar-l0cyMroinI0,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, robh-DgEjT+Ai2ygdnm+yROfE0A,
	devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171228230111.GB19819-1RhO1Y9PlrlHTL0Zs8A6p5iNqAH0jzoTYJqu5kTmcBRl57MIdRCFDg@public.gmane.org>

Hi Bjorn,

Le 29/12/2017 à 00:01, Bjorn Helgaas a écrit :
> On Mon, Dec 18, 2017 at 07:16:06PM +0100, Cyrille Pitchen wrote:
>> This patch adds support to the Cadence PCIe controller in host mode.
>>
>> The "cadence/" entry in drivers/pci/Makefile is placed after the
>> "endpoint/" entry so when the next patch introduces a EPC driver for the
>> Cadence PCIe controller, drivers/pci/cadence/pcie-cadence-ep.o will be
>> linked after drivers/pci/endpoint/*.o objects, otherwise the built-in
>> pci-cadence-ep driver would be probed before the PCI endpoint libraries
>> would have been initialized, which would result in a kernel crash.
>>
>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> 
>> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
>> index 7284a7f6ad1e..a66ddb347798 100644
>> --- a/drivers/pci/Makefile
>> +++ b/drivers/pci/Makefile
>> @@ -54,5 +54,6 @@ obj-y += host/
>>  obj-y += switch/
>>  
>>  obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
>> +obj-$(CONFIG_PCI_CADENCE)	+= cadence/
>>  # PCI dwc controller drivers
>>  obj-y				+= dwc/
> 
> I don't like the fact that the cadence/ rule looks different than the
> dwc/ rule for no obvious reason.  With some work, the dwc/ rule could
> maybe be made to look like:

I've tried to understand why dwc uses obj-y instead of
obj-$(CONFIG_PCIE_DW), here is what I've found:

In drivers/pci/dwc/Makefile there is some obj-$(CONFIG_ARM64) rule to
generate the pcie-hisi.o object like there are other obj-$(CONFIG_ARM64)
rules in drivers/pci/host/Makefile produce objects like pci-thunder-ecam.o
for instance.

Then I compared both drivers/pci/dwc/pcie-hisi.c and
drivers/pci/host/pci-thunder-ecam.c:

Both files are structured like this:

#if defined(CONFIG_PCI_<controller_name>) || (defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS))

[...]
struct pci_ecam_ops <controller_name>_ops = {
	.bus_shift	= 20,
	.pci_ops	= {
		[...]
	}
};

#ifdef CONFIG_PCI_<controller_name>

[...]

static struct platform_driver <controller_name>_driver = {
	.driver = {
		[...]
	},
	.probe = <controller_name>_probe,
};
builtin_platform_driver(<controller_name>_driver);

#endif
#endif


Then the 'struct pci_ecam_ops' <controller_name>_ops is declared in
include/linux/pci-ecam.h:

#if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
extern struct pci_ecam_ops pci_32b_ops;		/* 32-bit accesses only */
extern struct pci_ecam_ops hisi_pcie_ops;	/* HiSilicon */
extern struct pci_ecam_ops thunder_pem_ecam_ops; /* Cavium ThunderX 1.x & 2.x */
extern struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */
extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene PCIe v1 */
extern struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene PCIe v2.x */
#endif

And referenced by drivers/acpi/pci_mcfg.c.

So I guess this is the reason why the 'dwc' folder uses obj-y like the
'host' folder does in drivers/pci/Makefile: files like pcie-hisi.c must
be compiled when all 3 CONFIG_ARM64, CONFIG_ACPI and CONFIG_PCI_QUIRKS are
defined even if their associated CONFIG_PCI_<controller_name> is not defined.

So is it okay for you to leave the dwc rule as is for now?

> 
>   obj-$(CONFIG_PCIE_DW)                 += dwc/
> 
> I *think* that should actually be pretty easy.  Everything in
> drivers/pci/dwc/Kconfig selects PCIE_DW if set, either via
> PCIE_DW_HOST or PCIE_DW_EP.
> 
>> diff --git a/drivers/pci/cadence/Kconfig b/drivers/pci/cadence/Kconfig
>> new file mode 100644
>> index 000000000000..0d15b40861e9
>> --- /dev/null
>> +++ b/drivers/pci/cadence/Kconfig
>> @@ -0,0 +1,24 @@
>> +menuconfig PCI_CADENCE
>> +	bool "Cadence PCI controllers support"
>> +	depends on PCI && HAS_IOMEM
>> +	help
>> +	  Say Y here if you want to support some Cadence PCI controller.
>> +
>> +	  When in doubt, say N.
>> +
>> +if PCI_CADENCE
>> +
>> +config PCIE_CADENCE
>> +	bool
>> +
>> +config PCIE_CADENCE_HOST
>> +	bool "Cadence PCIe host controller"
>> +	depends on OF
>> +	select IRQ_DOMAIN
>> +	select PCIE_CADENCE
>> +	help
>> +	  Say Y here if you want to support the Cadence PCIe controller in host
>> +	  mode. This PCIe controller may be embedded into many different vendors
>> +	  SoCs.
>> +
>> +endif # PCI_CADENCE
> 
> Can you just use the same strategy as pci/dwc/Kconfig does, i.e., omit
> the top-level PCI_CADENCE symbol?  If we don't need it for dwc, with
> its dozen drivers, we probably don't need it for the one or two
> Cadence drivers.
>

done for the next version of the series.

Best regards,

Cyrille
 
> Bjorn
> 


-- 
Cyrille Pitchen, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 0/9] PCI: Add support to the Cadence PCIe controller
From: Cyrille Pitchen @ 2017-12-29 20:53 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, bhelgaas-hpIqsD4AKlfQT0dZR+AlfA,
	lorenzo.pieralisi-5wv7dgnIgG8, linux-pci-u79uwXL29TY76Z2rM5mHXA
  Cc: adouglas-vna1KIf7WgpBDgjK7y7TUQ, stelford-vna1KIf7WgpBDgjK7y7TUQ,
	dgary-vna1KIf7WgpBDgjK7y7TUQ, kgopi-vna1KIf7WgpBDgjK7y7TUQ,
	eandrews-vna1KIf7WgpBDgjK7y7TUQ,
	thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	sureshp-vna1KIf7WgpBDgjK7y7TUQ, nsekhar-l0cyMroinI0,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, robh-DgEjT+Ai2ygdnm+yROfE0A,
	devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <d30e0abe-8b08-b2bf-8410-e0fc2b18cacc-l0cyMroinI0@public.gmane.org>

Hi Kishon,

Le 28/12/2017 à 14:00, Kishon Vijay Abraham I a écrit :
> Hi Cyrille,
> 
> On Monday 18 December 2017 11:46 PM, Cyrille Pitchen wrote:
>> Hi all,
>>
>> this series of patches adds support to the Cadence PCIe controller.
>> It was tested on a ARM64 platform emulated by a Palladium running the
>> pci-next kernel.
>>
>> The host mode was tested with some PCIe devices connected to the Palladium
>> through a speed-bridge. Some of those devices were a USB host controller
>> and a SATA controller. The PCIe host controller was also tested with a
>> second controller configured in endpoint mode and connected back to back
>> to the first controller.
>>
>> The EndPoint Controller (EPC) driver of this series was tested with the
>> pci-epf-test.c EndPoint Function (EPF) driver and the pcitest userspace
>> program.
> 
> Did you get to test multi function EP?
>

No I didn't: I tested only with a single function to check for regression
but currently I'm not able to test with multiple functions.

With devmem, I've tried to read then write the Physical Function Configuration
Register (offset 0x2c0 in the Local Management registers) to enable
functions other than function 0.

This is the CDNS_PCIE_LM_EP_FUNC_CFG register that the pcie-cadence_ep.c
driver updates in cdns_pcie_ep_write_header() since v2 of the series.

As written in the datasheet, BIT(0) is actually hard-wired to 1, hence
function 0 can't be disabled: that makes sense. However other function
enable bits were read as 0 whereas the datasheet claims they should be set
at power up. Besides, I can't set any of them with devmem.

Actually, I have 2 slightly different datasheets, in the first one I should
have 4 functions but only 2 based on the second datasheet.

Then I guess it's a design parameter used when synthesizing the controller.

So I've asked Cadence whether I've missed or misunderstood something in the
datasheets or whether the IP they provided me with has a single function
for now. I'm waiting for their answers.

Best regards,

Cyrille
 
> Thanks
> Kishon
>>
>> For pci-next, I applied this series on top of Kishon's patch
>> ("PCI: endpoint: Use EPC's device in dma_alloc_coherent/dma_free_coherent")
>> otherwise dma_alloc_coherent() fails when called by pci_epf_alloc_space().
>>
>> Best regards,
>>
>> Cyrille
>>
>> ChangeLog:
>>
>> v1 -> v2:
>> - add new properties in the device-tree bindings: 'cdns,max-outbound-regions'
>>   and 'cdns,no-bar-match-nbits'.
>> - add a new patch to regroup all makefile rules in drivers/pci/Makefile, hence
>>   cleaning drivers/Makefile up.
>> - change the license text to use the recommanded format:
>>   // SPDX-License-Identifier: GPL-2.0
>> - add a new patch updating the API of the EPC library to add support to
>>   multi-function devices.
>> - add a 2 new patches to share more common code between host controller drivers
>> - remove some useless tests
>> - add more comments in both drivers.
>> - fix DT bindings examples
>> - remove useless init of the primary, secondary and sub-ordinate bus numbers in
>>   the PCI configuration space of the root port.
>> - remove cdns_pcie_ep_stop() function and rework cdns_pcie_ep_start() function
>>
>> Cyrille Pitchen (8):
>>   PCI: Regroup all PCI related entries into drivers/pci/Makefile
>>   PCI: OF: Add generic function to parse and allocate PCI resources
>>   PCI: Add generic function to probe PCI host controllers
>>   PCI: Add vendor ID for Cadence
>>   PCI: cadence: Add host driver for Cadence PCIe controller
>>   PCI: endpoint: Add the function number as argument to EPC ops
>>   dt-bindings: PCI: cadence: Add DT bindings for Cadence PCIe endpoint
>>     controller
>>   PCI: cadence: Add EndPoint Controller driver for Cadence PCIe
>>     controller
>>
>> Scott Telford (1):
>>   dt-bindings: PCI: cadence: Add DT bindings for Cadence PCIe host
>>     controller
>>
>>  .../devicetree/bindings/pci/cdns,cdns-pcie-ep.txt  |  23 +
>>  .../bindings/pci/cdns,cdns-pcie-host.txt           |  60 +++
>>  MAINTAINERS                                        |   7 +
>>  drivers/Makefile                                   |   5 +-
>>  drivers/pci/Kconfig                                |   2 +
>>  drivers/pci/Makefile                               |  12 +-
>>  drivers/pci/cadence/Kconfig                        |  33 ++
>>  drivers/pci/cadence/Makefile                       |   4 +
>>  drivers/pci/cadence/pcie-cadence-ep.c              | 531 +++++++++++++++++++++
>>  drivers/pci/cadence/pcie-cadence-host.c            | 330 +++++++++++++
>>  drivers/pci/cadence/pcie-cadence.c                 |  95 ++++
>>  drivers/pci/cadence/pcie-cadence.h                 | 310 ++++++++++++
>>  drivers/pci/dwc/pcie-designware-ep.c               |  20 +-
>>  drivers/pci/endpoint/functions/pci-epf-test.c      |  41 +-
>>  drivers/pci/endpoint/pci-epc-core.c                |  62 ++-
>>  drivers/pci/host/Makefile                          |   2 +
>>  drivers/pci/host/pci-host-common.c                 |  87 +---
>>  drivers/pci/of.c                                   |  51 ++
>>  drivers/pci/probe.c                                |  50 ++
>>  include/linux/pci-epc.h                            |  43 +-
>>  include/linux/pci.h                                |  12 +
>>  include/linux/pci_ids.h                            |   2 +
>>  22 files changed, 1624 insertions(+), 158 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.txt
>>  create mode 100644 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
>>  create mode 100644 drivers/pci/cadence/Kconfig
>>  create mode 100644 drivers/pci/cadence/Makefile
>>  create mode 100644 drivers/pci/cadence/pcie-cadence-ep.c
>>  create mode 100644 drivers/pci/cadence/pcie-cadence-host.c
>>  create mode 100644 drivers/pci/cadence/pcie-cadence.c
>>  create mode 100644 drivers/pci/cadence/pcie-cadence.h
>>
> 


-- 
Cyrille Pitchen, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 7/9] PCI: endpoint: Add the function number as argument to EPC ops
From: Cyrille Pitchen @ 2017-12-29 20:30 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, bhelgaas-hpIqsD4AKlfQT0dZR+AlfA,
	lorenzo.pieralisi-5wv7dgnIgG8, linux-pci-u79uwXL29TY76Z2rM5mHXA
  Cc: adouglas-vna1KIf7WgpBDgjK7y7TUQ, stelford-vna1KIf7WgpBDgjK7y7TUQ,
	dgary-vna1KIf7WgpBDgjK7y7TUQ, kgopi-vna1KIf7WgpBDgjK7y7TUQ,
	eandrews-vna1KIf7WgpBDgjK7y7TUQ,
	thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	sureshp-vna1KIf7WgpBDgjK7y7TUQ, nsekhar-l0cyMroinI0,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, robh-DgEjT+Ai2ygdnm+yROfE0A,
	devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <ca8f5a74-3db5-87f4-3af5-ed5ee0d3b634-l0cyMroinI0@public.gmane.org>

Hi Kishon,

Le 29/12/2017 à 10:23, Kishon Vijay Abraham I a écrit :
> Hi,
> 
> On Monday 18 December 2017 11:46 PM, Cyrille Pitchen wrote:
>> This patch updates the prototype of most handlers from 'struct
>> pci_epc_ops' so the EPC library can now support multi-function devices.
>>
>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> ---
>>  drivers/pci/dwc/pcie-designware-ep.c          | 20 +++++----
>>  drivers/pci/endpoint/functions/pci-epf-test.c | 41 ++++++++++--------
>>  drivers/pci/endpoint/pci-epc-core.c           | 62 ++++++++++++++++-----------
>>  include/linux/pci-epc.h                       | 43 +++++++++++--------
>>  4 files changed, 96 insertions(+), 70 deletions(-)
>>
[...]
>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
>> index cd7d4788b94d..77420364a728 100644
>> --- a/drivers/pci/endpoint/pci-epc-core.c
>> +++ b/drivers/pci/endpoint/pci-epc-core.c
>> @@ -141,25 +141,26 @@ EXPORT_SYMBOL_GPL(pci_epc_start);
>>  /**
>>   * pci_epc_raise_irq() - interrupt the host system
>>   * @epc: the EPC device which has to interrupt the host
>> + * @func_no: the endpoint function number in the EPC device
>>   * @type: specify the type of interrupt; legacy or MSI
>>   * @interrupt_num: the MSI interrupt number
>>   *
>>   * Invoke to raise an MSI or legacy interrupt
>>   */
>> -int pci_epc_raise_irq(struct pci_epc *epc, enum pci_epc_irq_type type,
>> -		      u8 interrupt_num)
>> +int pci_epc_raise_irq(struct pci_epc *epc, u8 func_no,
>> +		      enum pci_epc_irq_type type, u8 interrupt_num)
>>  {
>>  	int ret;
>>  	unsigned long flags;
>>  
>> -	if (IS_ERR(epc))
>> +	if (IS_ERR(epc) || func_no > BAR_5)
> 
> why is function number compared with BAR? here and everywhere below..
>

Oops! sorry for that. Actually it's a silly mistake, thanks for
catching it!

I plan to fix it replacing with something like this:

s/func_no > BAR_5/func_no >= epc->max_functions/

Then I guess if we already test with IS_ERR(), I should now replace
IS_ERR() by IS_ERR_OR_NULL() before dereferencing epc when reading
max_functions.

Best regards,

Cyrille
 
> Thanks
> Kishon
> 


-- 
Cyrille Pitchen, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 1/9] PCI: Regroup all PCI related entries into drivers/pci/Makefile
From: Cyrille Pitchen @ 2017-12-29 20:21 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: bhelgaas, kishon, lorenzo.pieralisi, linux-pci, adouglas,
	stelford, dgary, kgopi, eandrews, thomas.petazzoni, sureshp,
	nsekhar, linux-kernel, robh, devicetree
In-Reply-To: <20171228224718.GA19819@bhelgaas-glaptop.roam.corp.google.com>

Hi Bjorn,

Le 28/12/2017 à 23:47, Bjorn Helgaas a écrit :
> On Mon, Dec 18, 2017 at 07:16:01PM +0100, Cyrille Pitchen wrote:
>> This patch cleans drivers/Makefile up by moving the pci/endpoint and
>> pci/dwc entries from drivers/Makefile into drivers/pci/Makefile.
> 
> Thanks a lot for doing this!
> 
> s/This patch cleans/Clean up/
>

updated for the next series
 
> Speaking of cleanup, this Makefile has useless comments and blank
> lines.  Maybe you could add a new patch to remove them and reorder it
> into a sensible order, with the Intel MID special case at the end and
> the host/dwc/cadence stuff together?
>

OK, I'm working on it. So right now I'm trying to sort entries by
alphabetical order but the first test has just failed: kernel oops
when calling pcied_init().

I guess there are more hidden dependencies than expected, solved by the
link order. This cleanup might bit risky after all, especially since I
won't be able to test all combinations or with all possible hardwares. 

Best regards,

Cyrille
 
>> Since we don't want to introduce any dependency between CONFIG_PCI and
>> CONFIG_PCI_ENDPOINT, we now always execute drivers/pci/Makefile.
>>
>> Hence all Makefiles in drivers/pci/ were updated accordingly so no file is
>> compiled when CONFIG_PCI is not defined.
>>
>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@free-electrons.com>
>> ---
>>  drivers/Makefile          |  5 +----
>>  drivers/pci/Kconfig       |  1 +
>>  drivers/pci/Makefile      | 11 ++++++++---
>>  drivers/pci/host/Makefile |  2 ++
>>  4 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 1d034b680431..9757199b9a65 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -16,10 +16,7 @@ obj-$(CONFIG_PINCTRL)		+= pinctrl/
>>  obj-$(CONFIG_GPIOLIB)		+= gpio/
>>  obj-y				+= pwm/
>>  
>> -obj-$(CONFIG_PCI)		+= pci/
>> -obj-$(CONFIG_PCI_ENDPOINT)	+= pci/endpoint/
>> -# PCI dwc controller drivers
>> -obj-y				+= pci/dwc/
>> +obj-y				+= pci/
>>
>>  obj-$(CONFIG_PARISC)		+= parisc/
>>  obj-$(CONFIG_RAPIDIO)		+= rapidio/
>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>> index bda151788f3f..7eeb969ab86a 100644
>> --- a/drivers/pci/Kconfig
>> +++ b/drivers/pci/Kconfig
>> @@ -125,6 +125,7 @@ config PCI_PASID
>>  
>>  config PCI_LABEL
>>  	def_bool y if (DMI || ACPI)
>> +	depends on PCI
>>  	select NLS
>>  
>>  config PCI_HYPERV
>> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
>> index c7819b973df7..7284a7f6ad1e 100644
>> --- a/drivers/pci/Makefile
>> +++ b/drivers/pci/Makefile
>> @@ -3,12 +3,15 @@
>>  # Makefile for the PCI bus specific drivers.
>>  #
>>  
>> -obj-y		+= access.o bus.o probe.o host-bridge.o remove.o pci.o \
>> +obj-$(CONFIG_PCI)	+= access.o bus.o probe.o host-bridge.o remove.o pci.o \
>>  			pci-driver.o search.o pci-sysfs.o rom.o setup-res.o \
>>  			irq.o vpd.o setup-bus.o vc.o mmap.o setup-irq.o
>>  
>> +ifdef CONFIG_PCI
>>  obj-$(CONFIG_PROC_FS) += proc.o
>>  obj-$(CONFIG_SYSFS) += slot.o
>> +obj-$(CONFIG_OF) += of.o
>> +endif
>>  
>>  obj-$(CONFIG_PCI_QUIRKS) += quirks.o
>>  
>> @@ -44,10 +47,12 @@ obj-$(CONFIG_PCI_ECAM) += ecam.o
>>  
>>  obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
>>  
>> -obj-$(CONFIG_OF) += of.o
>> -
>>  ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>>  
>>  # PCI host controller drivers
>>  obj-y += host/
>>  obj-y += switch/
>> +
>> +obj-$(CONFIG_PCI_ENDPOINT)	+= endpoint/
>> +# PCI dwc controller drivers
>> +obj-y				+= dwc/
>> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
>> index 34ec1d88f961..3b1059190867 100644
>> --- a/drivers/pci/host/Makefile
>> +++ b/drivers/pci/host/Makefile
>> @@ -34,6 +34,8 @@ obj-$(CONFIG_VMD) += vmd.o
>>  # ARM64 and use internal ifdefs to only build the pieces we need
>>  # depending on whether ACPI, the DT driver, or both are enabled.
>>  
>> +ifdef CONFIG_PCI
>>  obj-$(CONFIG_ARM64) += pci-thunder-ecam.o
>>  obj-$(CONFIG_ARM64) += pci-thunder-pem.o
>>  obj-$(CONFIG_ARM64) += pci-xgene.o
>> +endif
>> -- 
>> 2.11.0
>>
> 


-- 
Cyrille Pitchen, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply

* Re: [PATCH 1/3] mtd: spi-nor: add optional DMA-safe bounce buffer for data transfer
From: Trent Piepho @ 2017-12-29 18:03 UTC (permalink / raw)
  To: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org,
	broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	cyrille.pitchen-yU5RGvR974pGWvitb5QawA@public.gmane.org,
	dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org,
	computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
	vigneshr-l0cyMroinI0@public.gmane.org,
	boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org,
	richard-/L3Ra7n9ekc@public.gmane.org,
	marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
  Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org,
	radu.pirea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org,
	robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <08be8b42-732a-bf28-40c4-f46bf9d71c80-l0cyMroinI0@public.gmane.org>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2328 bytes --]

On Fri, 2017-12-29 at 15:46 +0530, Vignesh R wrote:
> On Friday 29 December 2017 12:24 AM, Trent Piepho wrote:
> > 
> > > Vignesh has suggested to call virt_addr_valid() instead.
> > > I think Boris has also told me about this function.
> > > So it might be the right solution. What do you think about their proposal?
> > 
> > Not sure what exactly the differences are between these methods.  The
> > fact that each of the many existing DMA fixes uses slightly different
> > code to detect what is unsafe speaks to the difficulty of this problem!
> 
> My understanding based on Documentation/DMA-API-HOWTO.txt and
> Documentation/arm/memory.txt is that
> virt_addr_valid() will guarantee that address is in range of
> PAGE_OFFSET to high_memory-1 (Kernel direct-mapped RAM region) which is
> address range of buffers that are DMA'able.

There's code in gpmi-nand.c that does:

        /* first try to map the upper buffer directly */
        if (virt_addr_valid(this->upper_buf) &&
                !object_is_on_stack(this->upper_buf)) {
                sg_init_one(sgl, this->upper_buf, this->upper_len);

So whoever wrote that thought that stack objects needed an additional
test beyond virt_addr_valid.  But it does appear to be far more common
to depend on just virt_addr_valid, so perhaps the code in gpmi-nand is
in error.

> >  virt_addr_valid() is already used by spi-ti-qspi.  spi core uses for
> > the buffer map helper, but that code path is for buffers which are NOT
> > vmalloc or highmem, but are still not virt_addr_valid() for some other
> > reason.
> > 
> 
> 	if (vmalloced_buf || kmap_buf) {
> 		/* Handle vmalloc'd or kmap'd buffers */
> 		...
This stuff does get DMAed.  So I have to wonder, if spi.c thinks it can
use DMA with vmalloc or highmem, couldn't spi-not do the same instead
of the bounce buffer?

>         } else if (virt_addr_valid(buf)) {
> 		/* Handle kmalloc'd and such buffers */
>                 ...
> 	} else {
> 		/* Error if none of the above */

So what is this case here for?  It's some class that does not have a
valid virtual address and yet is not vmalloc or highmem.

> 		return -EINVAL;
> 	}
> N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±²˜¢žØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox