* Re: [PATCH v3] arm64: dts: rk3399: add support for firefly-rk3399 board
From: Marc Zyngier @ 2017-04-18 19:31 UTC (permalink / raw)
To: Kever Yang
Cc: heiko-4mtYJXux2i+zQB+pC5nmwQ, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA, Catalin Marinas, Brian Norris,
Will Deacon, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Matthias Brugger, Andy Yan, Jianqun Xu, Liang Chen
In-Reply-To: <1491796213-4115-1-git-send-email-kever.yang-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
On Mon, 10 Apr 2017 11:50:13 +0800
Kever Yang <kever.yang-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> Firefly-rk3399 is a bord from T-Firefly, you can find detail about
> it here:
> http://en.t-firefly.com/en/firenow/Firefly_RK3399/
>
> This patch add basic node for the board and make it able to bring
> up.
>
> Peripheral works:
> - usb hub which connect to ehci controller;
> - UART2 debug
> - eMMC
> - PCIe
>
> Not work:
> - USB 3.0 HOST, type-C port
> - sdio, sd-card
>
> Not test for other peripheral:
[...]
> - Ethernet
[...]
FWIW, I've tested the Ethernet port, which seems to work nicely.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
--
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] arm64: dts: allwinner: pine64: Prepare optional UART nodes with pinctrl
From: Andreas Färber @ 2017-04-18 19:25 UTC (permalink / raw)
To: Maxime Ripard
Cc: Andreas Färber, Chen-Yu Tsai, Rob Herring, Mark Rutland,
Catalin Marinas, Will Deacon, linux-arm-kernel, devicetree,
linux-kernel
In-Reply-To: <20170418053458.phcbp4da3vatejpg@lukather>
Pine64 exposes all A64 UARTs, not just UART0.
Since the pins can be used as GPIO, don't enable the new UART nodes by
default, but prepare the pinctrl settings to aid in activating them via
overlays, i.e., overriding the status property of &uartX nodes.
For UART4 (Euler) the safer route of not including RTS/CTS pins is chosen,
whereas for UART1 (Bluetooth) they are included.
Add the corresponding pinctrl nodes where missing.
Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
.../arm64/boot/dts/allwinner/sun50i-a64-pine64.dts | 29 ++++++++++++++++++++++
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 15 +++++++++++
2 files changed, 44 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
index db6c0f36999e..260fb12ac3fc 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
@@ -99,12 +99,41 @@
status = "okay";
};
+/* On Exp and Euler connectors */
&uart0 {
pinctrl-names = "default";
pinctrl-0 = <&uart0_pins_a>;
status = "okay";
};
+/* On Wifi/BT connector, with RTS/CTS */
+&uart1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
+ status = "disabled";
+};
+
+/* On Pi-2 connector */
+&uart2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart2_pins>;
+ status = "disabled";
+};
+
+/* On Euler connector */
+&uart3 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart3_pins>;
+ status = "disabled";
+};
+
+/* On Euler connector, RTS/CTS optional */
+&uart4 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart4_pins>;
+ status = "disabled";
+};
+
&usb_otg {
dr_mode = "host";
status = "okay";
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 133810df06b9..c172c62447e8 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -292,6 +292,21 @@
pins = "PB0", "PB1";
function = "uart2";
};
+
+ uart3_pins: uart3-pins {
+ pins = "PD0", "PD1";
+ function = "uart3";
+ };
+
+ uart4_pins: uart4-pins {
+ pins = "PD2", "PD3";
+ function = "uart4";
+ };
+
+ uart4_rts_cts_pins: uart4-rts-cts-pins {
+ pins = "PD4", "PD5";
+ function = "uart4";
+ };
};
uart0: serial@1c28000 {
--
2.10.2
^ permalink raw reply related
* Re: [PATCH 1/3] drm/vc4: Turn the V3D clock on at runtime.
From: Eric Anholt @ 2017-04-18 19:23 UTC (permalink / raw)
To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Rob Herring,
Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170418191157.18517-1-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 720 bytes --]
Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> writes:
> For the Raspberry Pi's bindings, the power domain also implicitly
> turns on the clock and deasserts reset, but for the new Cygnus port we
> start representing the clock in the devicetree.
> + v3d->clk = devm_clk_get(dev, "v3d_clk");
> + if (IS_ERR(v3d->clk)) {
> + int ret = PTR_ERR(v3d->clk);
> +
> + if (ret == -ENODEV) {
Apparently I hadn't booted this on RPi yet. This is supposed to be
-ENOENT.
> + /* bcm2835 didn't have a clock reference in the DT. */
> + ret = 0;
> + v3d->clk = NULL;
> + } else {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Failed to get V3D clock: %d\n",
> + ret);
> + return ret;
> + }
> + }
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* [PATCH 3/4] of/fdt: introduce of_scan_flat_dt_subnodes and of_get_flat_dt_phandle
From: Nicholas Piggin @ 2017-04-18 19:12 UTC (permalink / raw)
To: linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ
Cc: Nicholas Piggin, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170418191220.3166-1-npiggin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Introduce primitives for FDT parsing. These will be used for powerpc
cpufeatures node scanning, which has quite complex structure but should
be processed early.
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Nicholas Piggin <npiggin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/of/fdt.c | 38 ++++++++++++++++++++++++++++++++++++++
include/linux/of_fdt.h | 6 ++++++
2 files changed, 44 insertions(+)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index e5ce4b59e162..961ca97072a9 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -754,6 +754,36 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
}
/**
+ * of_scan_flat_dt_subnodes - scan sub-nodes of a node call callback on each.
+ * @it: callback function
+ * @data: context data pointer
+ *
+ * This function is used to scan sub-nodes of a node.
+ */
+int __init of_scan_flat_dt_subnodes(unsigned long parent,
+ int (*it)(unsigned long node,
+ const char *uname,
+ void *data),
+ void *data)
+{
+ const void *blob = initial_boot_params;
+ int node;
+
+ fdt_for_each_subnode(node, blob, parent) {
+ const char *pathp;
+ int rc;
+
+ pathp = fdt_get_name(blob, node, NULL);
+ if (*pathp == '/')
+ pathp = kbasename(pathp);
+ rc = it(node, pathp, data);
+ if (rc)
+ return rc;
+ }
+ return 0;
+}
+
+/**
* of_get_flat_dt_subnode_by_name - get the subnode by given name
*
* @node: the parent node
@@ -812,6 +842,14 @@ int __init of_flat_dt_match(unsigned long node, const char *const *compat)
return of_fdt_match(initial_boot_params, node, compat);
}
+/**
+ * of_get_flat_dt_prop - Given a node in the flat blob, return the phandle
+ */
+uint32_t __init of_get_flat_dt_phandle(unsigned long node)
+{
+ return fdt_get_phandle(initial_boot_params, node);
+}
+
struct fdt_scan_status {
const char *name;
int namelen;
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index 271b3fdf0070..1dfbfd0d8040 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -54,6 +54,11 @@ extern char __dtb_end[];
extern int of_scan_flat_dt(int (*it)(unsigned long node, const char *uname,
int depth, void *data),
void *data);
+extern int of_scan_flat_dt_subnodes(unsigned long node,
+ int (*it)(unsigned long node,
+ const char *uname,
+ void *data),
+ void *data);
extern int of_get_flat_dt_subnode_by_name(unsigned long node,
const char *uname);
extern const void *of_get_flat_dt_prop(unsigned long node, const char *name,
@@ -62,6 +67,7 @@ extern int of_flat_dt_is_compatible(unsigned long node, const char *name);
extern int of_flat_dt_match(unsigned long node, const char *const *matches);
extern unsigned long of_get_flat_dt_root(void);
extern int of_get_flat_dt_size(void);
+extern uint32_t of_get_flat_dt_phandle(unsigned long node);
extern int early_init_dt_scan_chosen(unsigned long node, const char *uname,
int depth, void *data);
--
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 3/3] drm/vc4: Add specific compatible strings for Cygnus.
From: Eric Anholt @ 2017-04-18 19:11 UTC (permalink / raw)
To: dri-devel, Rob Herring, Mark Rutland, devicetree; +Cc: linux-kernel
In-Reply-To: <20170418191157.18517-1-eric@anholt.net>
Cygnus has V3D 2.6 instead of 2.1, and doesn't use the VC4 display
modules. The V3D can be uniquely identified by the IDENT[01]
registers, and there's nothing to key off of for the display change
other than the lack of DT nodes for the display components, but it's
convention to have new compatible strings anyway.
Signed-off-by: Eric Anholt <eric@anholt.net>
---
Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt | 4 ++--
drivers/gpu/drm/vc4/vc4_drv.c | 1 +
drivers/gpu/drm/vc4/vc4_v3d.c | 1 +
3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
index bc1756f4f791..284e2b14cfbe 100644
--- a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
+++ b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
@@ -5,7 +5,7 @@ with HDMI output and the HVS (Hardware Video Scaler) for compositing
display planes.
Required properties for VC4:
-- compatible: Should be "brcm,bcm2835-vc4"
+- compatible: Should be "brcm,bcm2835-vc4" or "brcm,cygnus-vc4"
Required properties for Pixel Valve:
- compatible: Should be one of "brcm,bcm2835-pixelvalve0",
@@ -54,7 +54,7 @@ Required properties for VEC:
See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
Required properties for V3D:
-- compatible: Should be "brcm,bcm2835-v3d"
+- compatible: Should be "brcm,bcm2835-v3d" or "brcm,cygnus-v3d"
- reg: Physical base address and length of the V3D's registers
- interrupts: The interrupt number
See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 92fb9a41fe7c..754ce76d4b98 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -335,6 +335,7 @@ static int vc4_platform_drm_remove(struct platform_device *pdev)
static const struct of_device_id vc4_of_match[] = {
{ .compatible = "brcm,bcm2835-vc4", },
+ { .compatible = "brcm,cygnus-vc4", },
{},
};
MODULE_DEVICE_TABLE(of, vc4_of_match);
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index 2442622e6bff..3abcd27aa46f 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -304,6 +304,7 @@ static int vc4_v3d_dev_remove(struct platform_device *pdev)
static const struct of_device_id vc4_v3d_dt_match[] = {
{ .compatible = "brcm,bcm2835-v3d" },
+ { .compatible = "brcm,cygnus-v3d" },
{ .compatible = "brcm,vc4-v3d" },
{}
};
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH 2/3] drm/vc4: Don't try to initialize FBDEV if we're only bound to V3D.
From: Eric Anholt @ 2017-04-18 19:11 UTC (permalink / raw)
To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Rob Herring,
Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Eric Anholt
In-Reply-To: <20170418191157.18517-1-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
The FBDEV initialization would throw an error in dmesg, when we just
want to silently not initialize fbdev on a V3D-only VC4 instance.
Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
---
drivers/gpu/drm/vc4/vc4_kms.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index ad7925a9e0ea..237a504f11f0 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -230,10 +230,12 @@ int vc4_kms_load(struct drm_device *dev)
drm_mode_config_reset(dev);
- vc4->fbdev = drm_fbdev_cma_init(dev, 32,
- dev->mode_config.num_connector);
- if (IS_ERR(vc4->fbdev))
- vc4->fbdev = NULL;
+ if (dev->mode_config.num_connector) {
+ vc4->fbdev = drm_fbdev_cma_init(dev, 32,
+ dev->mode_config.num_connector);
+ if (IS_ERR(vc4->fbdev))
+ vc4->fbdev = NULL;
+ }
drm_kms_helper_poll_init(dev);
--
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 1/3] drm/vc4: Turn the V3D clock on at runtime.
From: Eric Anholt @ 2017-04-18 19:11 UTC (permalink / raw)
To: dri-devel, Rob Herring, Mark Rutland, devicetree; +Cc: linux-kernel
For the Raspberry Pi's bindings, the power domain also implicitly
turns on the clock and deasserts reset, but for the new Cygnus port we
start representing the clock in the devicetree.
Signed-off-by: Eric Anholt <eric@anholt.net>
---
.../devicetree/bindings/display/brcm,bcm-vc4.txt | 3 ++
drivers/gpu/drm/vc4/vc4_drv.h | 1 +
drivers/gpu/drm/vc4/vc4_v3d.c | 33 ++++++++++++++++++++++
3 files changed, 37 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
index ca02d3e4db91..bc1756f4f791 100644
--- a/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
+++ b/Documentation/devicetree/bindings/display/brcm,bcm-vc4.txt
@@ -59,6 +59,9 @@ Required properties for V3D:
- interrupts: The interrupt number
See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
+Optional properties for V3D:
+- clocks: The clock the unit runs on
+
Required properties for DSI:
- compatible: Should be "brcm,bcm2835-dsi0" or "brcm,bcm2835-dsi1"
- reg: Physical base address and length of the DSI block's registers
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 81d2bc08e766..08d5c2213c80 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -189,6 +189,7 @@ struct vc4_v3d {
struct vc4_dev *vc4;
struct platform_device *pdev;
void __iomem *regs;
+ struct clk *clk;
};
struct vc4_hvs {
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index 7cc346ad9b0b..2442622e6bff 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -16,6 +16,7 @@
* this program. If not, see <http://www.gnu.org/licenses/>.
*/
+#include "linux/clk.h"
#include "linux/component.h"
#include "linux/pm_runtime.h"
#include "vc4_drv.h"
@@ -164,6 +165,9 @@ static int vc4_v3d_runtime_suspend(struct device *dev)
vc4_irq_uninstall(vc4->dev);
+ if (v3d->clk)
+ clk_disable_unprepare(v3d->clk);
+
return 0;
}
@@ -172,6 +176,13 @@ static int vc4_v3d_runtime_resume(struct device *dev)
struct vc4_v3d *v3d = dev_get_drvdata(dev);
struct vc4_dev *vc4 = v3d->vc4;
+ if (v3d->clk) {
+ int ret = clk_prepare_enable(v3d->clk);
+
+ if (ret != 0)
+ return ret;
+ }
+
vc4_v3d_init_hw(vc4->dev);
vc4_irq_postinstall(vc4->dev);
@@ -202,12 +213,34 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
vc4->v3d = v3d;
v3d->vc4 = vc4;
+ v3d->clk = devm_clk_get(dev, "v3d_clk");
+ if (IS_ERR(v3d->clk)) {
+ int ret = PTR_ERR(v3d->clk);
+
+ if (ret == -ENODEV) {
+ /* bcm2835 didn't have a clock reference in the DT. */
+ ret = 0;
+ v3d->clk = NULL;
+ } else {
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "Failed to get V3D clock: %d\n",
+ ret);
+ return ret;
+ }
+ }
+
if (V3D_READ(V3D_IDENT0) != V3D_EXPECTED_IDENT0) {
DRM_ERROR("V3D_IDENT0 read 0x%08x instead of 0x%08x\n",
V3D_READ(V3D_IDENT0), V3D_EXPECTED_IDENT0);
return -EINVAL;
}
+ if (v3d->clk) {
+ ret = clk_prepare_enable(v3d->clk);
+ if (ret != 0)
+ return ret;
+ }
+
/* Reset the binner overflow address/size at setup, to be sure
* we don't reuse an old one.
*/
--
2.11.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* [PATCH 2/2] arm64: dts: juno: add information about L1 and L2 caches
From: Sudeep Holla @ 2017-04-18 17:57 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: Sudeep Holla, Will Deacon, Liviu Dudau
In-Reply-To: <1492538279-16405-1-git-send-email-sudeep.holla-5wv7dgnIgG8@public.gmane.org>
Commit a8d4636f96ad ("arm64: cacheinfo: Remove CCSIDR-based cache
information probing") removed mechanism to extract cache information
based on CCSIDR register as the architecture explicitly states no
inference about the actual sizes of caches based on CCSIDR registers.
Commit 9a802431c527 ("arm64: cacheinfo: add support to override cache
levels via device tree") had already provided options to override cache
information from the device tree.
This patch adds the information about L1 and L2 caches on all variants
of Juno platform.
Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Cc: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
---
arch/arm64/boot/dts/arm/juno-r1.dts | 42 +++++++++++++++++++++++++++++++++++++
arch/arm64/boot/dts/arm/juno-r2.dts | 42 +++++++++++++++++++++++++++++++++++++
arch/arm64/boot/dts/arm/juno.dts | 42 +++++++++++++++++++++++++++++++++++++
3 files changed, 126 insertions(+)
diff --git a/arch/arm64/boot/dts/arm/juno-r1.dts b/arch/arm64/boot/dts/arm/juno-r1.dts
index 0033c59a64b5..0e8943ab94d7 100644
--- a/arch/arm64/boot/dts/arm/juno-r1.dts
+++ b/arch/arm64/boot/dts/arm/juno-r1.dts
@@ -89,6 +89,12 @@
reg = <0x0 0x0>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A57_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -100,6 +106,12 @@
reg = <0x0 0x1>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A57_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -111,6 +123,12 @@
reg = <0x0 0x100>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -122,6 +140,12 @@
reg = <0x0 0x101>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -133,6 +157,12 @@
reg = <0x0 0x102>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -144,6 +174,12 @@
reg = <0x0 0x103>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -152,10 +188,16 @@
A57_L2: l2-cache0 {
compatible = "cache";
+ cache-size = <0x200000>;
+ cache-line-size = <64>;
+ cache-sets = <2048>;
};
A53_L2: l2-cache1 {
compatible = "cache";
+ cache-size = <0x100000>;
+ cache-line-size = <64>;
+ cache-sets = <1024>;
};
};
diff --git a/arch/arm64/boot/dts/arm/juno-r2.dts b/arch/arm64/boot/dts/arm/juno-r2.dts
index 218d0e4736a8..405e2fba025b 100644
--- a/arch/arm64/boot/dts/arm/juno-r2.dts
+++ b/arch/arm64/boot/dts/arm/juno-r2.dts
@@ -89,6 +89,12 @@
reg = <0x0 0x0>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A72_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -100,6 +106,12 @@
reg = <0x0 0x1>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A72_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -111,6 +123,12 @@
reg = <0x0 0x100>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -122,6 +140,12 @@
reg = <0x0 0x101>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -133,6 +157,12 @@
reg = <0x0 0x102>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -144,6 +174,12 @@
reg = <0x0 0x103>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -152,10 +188,16 @@
A72_L2: l2-cache0 {
compatible = "cache";
+ cache-size = <0x200000>;
+ cache-line-size = <64>;
+ cache-sets = <2048>;
};
A53_L2: l2-cache1 {
compatible = "cache";
+ cache-size = <0x100000>;
+ cache-line-size = <64>;
+ cache-sets = <1024>;
};
};
diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
index bb2820ef3d5b..0220494c9b80 100644
--- a/arch/arm64/boot/dts/arm/juno.dts
+++ b/arch/arm64/boot/dts/arm/juno.dts
@@ -88,6 +88,12 @@
reg = <0x0 0x0>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A57_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -99,6 +105,12 @@
reg = <0x0 0x1>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0xc000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <256>;
next-level-cache = <&A57_L2>;
clocks = <&scpi_dvfs 0>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -110,6 +122,12 @@
reg = <0x0 0x100>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -121,6 +139,12 @@
reg = <0x0 0x101>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -132,6 +156,12 @@
reg = <0x0 0x102>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -143,6 +173,12 @@
reg = <0x0 0x103>;
device_type = "cpu";
enable-method = "psci";
+ i-cache-size = <0x8000>;
+ i-cache-line-size = <64>;
+ i-cache-sets = <256>;
+ d-cache-size = <0x8000>;
+ d-cache-line-size = <64>;
+ d-cache-sets = <128>;
next-level-cache = <&A53_L2>;
clocks = <&scpi_dvfs 1>;
cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
@@ -151,10 +187,16 @@
A57_L2: l2-cache0 {
compatible = "cache";
+ cache-size = <0x200000>;
+ cache-line-size = <64>;
+ cache-sets = <2048>;
};
A53_L2: l2-cache1 {
compatible = "cache";
+ cache-size = <0x100000>;
+ cache-line-size = <64>;
+ cache-sets = <1024>;
};
};
--
2.7.4
--
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 1/2] arm64: dts: juno: fix few unit address format warnings
From: Sudeep Holla @ 2017-04-18 17:57 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: Sudeep Holla, Liviu Dudau
This patch fixes the following set of warnings on juno.
smb@08000000 unit name should not have leading 0s
sysctl@020000 simple-bus unit address format error, expected "20000"
apbregs@010000 simple-bus unit address format error, expected "10000"
mmci@050000 simple-bus unit address format error, expected "50000"
kmi@060000 simple-bus unit address format error, expected "60000"
kmi@070000 simple-bus unit address format error, expected "70000"
wdt@0f0000 simple-bus unit address format error, expected "f0000"
Cc: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
---
arch/arm64/boot/dts/arm/juno-base.dtsi | 2 +-
arch/arm64/boot/dts/arm/juno-motherboard.dtsi | 12 ++++++------
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi b/arch/arm64/boot/dts/arm/juno-base.dtsi
index 8ffaff2043d0..bfe7d683a42e 100644
--- a/arch/arm64/boot/dts/arm/juno-base.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-base.dtsi
@@ -699,7 +699,7 @@
<0x00000008 0x80000000 0x1 0x80000000>;
};
- smb@08000000 {
+ smb@8000000 {
compatible = "simple-bus";
#address-cells = <2>;
#size-cells = <1>;
diff --git a/arch/arm64/boot/dts/arm/juno-motherboard.dtsi b/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
index 098601657f82..2ac43221ddb6 100644
--- a/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
@@ -137,7 +137,7 @@
#size-cells = <1>;
ranges = <0 3 0 0x200000>;
- v2m_sysctl: sysctl@020000 {
+ v2m_sysctl: sysctl@20000 {
compatible = "arm,sp810", "arm,primecell";
reg = <0x020000 0x1000>;
clocks = <&v2m_refclk32khz>, <&v2m_refclk1mhz>, <&mb_clk24mhz>;
@@ -148,7 +148,7 @@
assigned-clock-parents = <&v2m_refclk1mhz>, <&v2m_refclk1mhz>, <&v2m_refclk1mhz>, <&v2m_refclk1mhz>;
};
- apbregs@010000 {
+ apbregs@10000 {
compatible = "syscon", "simple-mfd";
reg = <0x010000 0x1000>;
@@ -216,7 +216,7 @@
};
};
- mmci@050000 {
+ mmci@50000 {
compatible = "arm,pl180", "arm,primecell";
reg = <0x050000 0x1000>;
interrupts = <5>;
@@ -228,7 +228,7 @@
clock-names = "mclk", "apb_pclk";
};
- kmi@060000 {
+ kmi@60000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x060000 0x1000>;
interrupts = <8>;
@@ -236,7 +236,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- kmi@070000 {
+ kmi@70000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x070000 0x1000>;
interrupts = <8>;
@@ -244,7 +244,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- wdt@0f0000 {
+ wdt@f0000 {
compatible = "arm,sp805", "arm,primecell";
reg = <0x0f0000 0x10000>;
interrupts = <7>;
--
2.7.4
--
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] ARM: dts: vexpress: fix few unit address format warnings
From: Sudeep Holla @ 2017-04-18 17:45 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: Sudeep Holla, Liviu Dudau, Lorenzo Pieralisi
This patch fixes the following set of warnings on vexpress platforms:
sysreg@010000 simple-bus unit address format error, expected "10000"
sysctl@020000 simple-bus unit address format error, expected "20000"
i2c@030000 simple-bus unit address format error, expected "30000"
aaci@040000 simple-bus unit address format error, expected "40000"
mmci@050000 simple-bus unit address format error, expected "50000"
kmi@060000 simple-bus unit address format error, expected "60000"
kmi@070000 simple-bus unit address format error, expected "70000"
uart@090000 simple-bus unit address format error, expected "90000"
uart@0a0000 simple-bus unit address format error, expected "a0000"
uart@0b0000 simple-bus unit address format error, expected "b0000"
uart@0c0000 simple-bus unit address format error, expected "c0000"
wdt@0f0000 simple-bus unit address format error, expected "f0000"
Cc: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
---
arch/arm/boot/dts/vexpress-v2m-rs1.dtsi | 24 ++++++++++++------------
arch/arm/boot/dts/vexpress-v2m.dtsi | 24 ++++++++++++------------
arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts | 2 +-
arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts | 18 +++++++++---------
arch/arm/boot/dts/vexpress-v2p-ca5s.dts | 2 +-
arch/arm/boot/dts/vexpress-v2p-ca9.dts | 2 +-
6 files changed, 36 insertions(+), 36 deletions(-)
Hi,
I observed few warning in linux-next due to the enhanced DTC checks
introduced with DTC upgrade in linux-next. The patch fixes few warnings
Regards,
Sudeep
diff --git a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
index 3086efacd00e..35714ff6f467 100644
--- a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
@@ -71,7 +71,7 @@
#size-cells = <1>;
ranges = <0 3 0 0x200000>;
- v2m_sysreg: sysreg@010000 {
+ v2m_sysreg: sysreg@10000 {
compatible = "arm,vexpress-sysreg";
reg = <0x010000 0x1000>;
@@ -94,7 +94,7 @@
};
};
- v2m_sysctl: sysctl@020000 {
+ v2m_sysctl: sysctl@20000 {
compatible = "arm,sp810", "arm,primecell";
reg = <0x020000 0x1000>;
clocks = <&v2m_refclk32khz>, <&v2m_refclk1mhz>, <&smbclk>;
@@ -106,7 +106,7 @@
};
/* PCI-E I2C bus */
- v2m_i2c_pcie: i2c@030000 {
+ v2m_i2c_pcie: i2c@30000 {
compatible = "arm,versatile-i2c";
reg = <0x030000 0x1000>;
@@ -119,7 +119,7 @@
};
};
- aaci@040000 {
+ aaci@40000 {
compatible = "arm,pl041", "arm,primecell";
reg = <0x040000 0x1000>;
interrupts = <11>;
@@ -127,7 +127,7 @@
clock-names = "apb_pclk";
};
- mmci@050000 {
+ mmci@50000 {
compatible = "arm,pl180", "arm,primecell";
reg = <0x050000 0x1000>;
interrupts = <9 10>;
@@ -139,7 +139,7 @@
clock-names = "mclk", "apb_pclk";
};
- kmi@060000 {
+ kmi@60000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x060000 0x1000>;
interrupts = <12>;
@@ -147,7 +147,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- kmi@070000 {
+ kmi@70000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x070000 0x1000>;
interrupts = <13>;
@@ -155,7 +155,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- v2m_serial0: uart@090000 {
+ v2m_serial0: uart@90000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x090000 0x1000>;
interrupts = <5>;
@@ -163,7 +163,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial1: uart@0a0000 {
+ v2m_serial1: uart@a0000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0a0000 0x1000>;
interrupts = <6>;
@@ -171,7 +171,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial2: uart@0b0000 {
+ v2m_serial2: uart@b0000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0b0000 0x1000>;
interrupts = <7>;
@@ -179,7 +179,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial3: uart@0c0000 {
+ v2m_serial3: uart@c0000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0c0000 0x1000>;
interrupts = <8>;
@@ -187,7 +187,7 @@
clock-names = "uartclk", "apb_pclk";
};
- wdt@0f0000 {
+ wdt@f0000 {
compatible = "arm,sp805", "arm,primecell";
reg = <0x0f0000 0x1000>;
interrupts = <0>;
diff --git a/arch/arm/boot/dts/vexpress-v2m.dtsi b/arch/arm/boot/dts/vexpress-v2m.dtsi
index c6393d3f1719..1b6f6393be93 100644
--- a/arch/arm/boot/dts/vexpress-v2m.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m.dtsi
@@ -70,7 +70,7 @@
#size-cells = <1>;
ranges = <0 7 0 0x20000>;
- v2m_sysreg: sysreg@00000 {
+ v2m_sysreg: sysreg@0 {
compatible = "arm,vexpress-sysreg";
reg = <0x00000 0x1000>;
@@ -93,7 +93,7 @@
};
};
- v2m_sysctl: sysctl@01000 {
+ v2m_sysctl: sysctl@1000 {
compatible = "arm,sp810", "arm,primecell";
reg = <0x01000 0x1000>;
clocks = <&v2m_refclk32khz>, <&v2m_refclk1mhz>, <&smbclk>;
@@ -105,7 +105,7 @@
};
/* PCI-E I2C bus */
- v2m_i2c_pcie: i2c@02000 {
+ v2m_i2c_pcie: i2c@2000 {
compatible = "arm,versatile-i2c";
reg = <0x02000 0x1000>;
@@ -118,7 +118,7 @@
};
};
- aaci@04000 {
+ aaci@4000 {
compatible = "arm,pl041", "arm,primecell";
reg = <0x04000 0x1000>;
interrupts = <11>;
@@ -126,7 +126,7 @@
clock-names = "apb_pclk";
};
- mmci@05000 {
+ mmci@5000 {
compatible = "arm,pl180", "arm,primecell";
reg = <0x05000 0x1000>;
interrupts = <9 10>;
@@ -138,7 +138,7 @@
clock-names = "mclk", "apb_pclk";
};
- kmi@06000 {
+ kmi@6000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x06000 0x1000>;
interrupts = <12>;
@@ -146,7 +146,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- kmi@07000 {
+ kmi@7000 {
compatible = "arm,pl050", "arm,primecell";
reg = <0x07000 0x1000>;
interrupts = <13>;
@@ -154,7 +154,7 @@
clock-names = "KMIREFCLK", "apb_pclk";
};
- v2m_serial0: uart@09000 {
+ v2m_serial0: uart@9000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x09000 0x1000>;
interrupts = <5>;
@@ -162,7 +162,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial1: uart@0a000 {
+ v2m_serial1: uart@a000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0a000 0x1000>;
interrupts = <6>;
@@ -170,7 +170,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial2: uart@0b000 {
+ v2m_serial2: uart@b000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0b000 0x1000>;
interrupts = <7>;
@@ -178,7 +178,7 @@
clock-names = "uartclk", "apb_pclk";
};
- v2m_serial3: uart@0c000 {
+ v2m_serial3: uart@c000 {
compatible = "arm,pl011", "arm,primecell";
reg = <0x0c000 0x1000>;
interrupts = <8>;
@@ -186,7 +186,7 @@
clock-names = "uartclk", "apb_pclk";
};
- wdt@0f000 {
+ wdt@f000 {
compatible = "arm,sp805", "arm,primecell";
reg = <0x0f000 0x1000>;
interrupts = <0>;
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts b/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
index 15f4fd3f4695..0c8de0ca73ee 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca15-tc1.dts
@@ -220,7 +220,7 @@
};
};
- smb@08000000 {
+ smb@8000000 {
compatible = "simple-bus";
#address-cells = <2>;
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts b/arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
index bd107c5a0226..65ecf206388c 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts
@@ -385,7 +385,7 @@
};
};
- etb@0,20010000 {
+ etb@20010000 {
compatible = "arm,coresight-etb10", "arm,primecell";
reg = <0 0x20010000 0 0x1000>;
@@ -399,7 +399,7 @@
};
};
- tpiu@0,20030000 {
+ tpiu@20030000 {
compatible = "arm,coresight-tpiu", "arm,primecell";
reg = <0 0x20030000 0 0x1000>;
@@ -449,7 +449,7 @@
};
};
- funnel@0,20040000 {
+ funnel@20040000 {
compatible = "arm,coresight-funnel", "arm,primecell";
reg = <0 0x20040000 0 0x1000>;
@@ -513,7 +513,7 @@
};
};
- ptm@0,2201c000 {
+ ptm@2201c000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2201c000 0 0x1000>;
@@ -527,7 +527,7 @@
};
};
- ptm@0,2201d000 {
+ ptm@2201d000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2201d000 0 0x1000>;
@@ -541,7 +541,7 @@
};
};
- etm@0,2203c000 {
+ etm@2203c000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2203c000 0 0x1000>;
@@ -555,7 +555,7 @@
};
};
- etm@0,2203d000 {
+ etm@2203d000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2203d000 0 0x1000>;
@@ -569,7 +569,7 @@
};
};
- etm@0,2203e000 {
+ etm@2203e000 {
compatible = "arm,coresight-etm3x", "arm,primecell";
reg = <0 0x2203e000 0 0x1000>;
@@ -583,7 +583,7 @@
};
};
- smb@08000000 {
+ smb@8000000 {
compatible = "simple-bus";
#address-cells = <2>;
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca5s.dts b/arch/arm/boot/dts/vexpress-v2p-ca5s.dts
index 1acecaf4b13d..6e69b8e6c1a7 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca5s.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca5s.dts
@@ -190,7 +190,7 @@
};
};
- smb@08000000 {
+ smb@8000000 {
compatible = "simple-bus";
#address-cells = <2>;
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
index b608a03ee02f..c9305b58afc2 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca9.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
@@ -300,7 +300,7 @@
};
};
- smb@04000000 {
+ smb@4000000 {
compatible = "simple-bus";
#address-cells = <2>;
--
2.7.4
--
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
* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
From: Mathieu Poirier @ 2017-04-18 17:40 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-soc-u79uwXL29TY76Z2rM5mHXA, Mike Leach, Sudeep Holla
In-Reply-To: <1491485461-22800-7-git-send-email-leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On Thu, Apr 06, 2017 at 09:30:59PM +0800, Leo Yan wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has
> description for related info in "Part H: External Debug".
>
> Chapter H7 "The Sample-based Profiling Extension" introduces several
> sampling registers, e.g. we can check program counter value with
> combined CPU exception level, secure state, etc. So this is helpful for
> analysis CPU lockup scenarios, e.g. if one CPU has run into infinite
> loop with IRQ disabled. In this case the CPU cannot switch context and
> handle any interrupt (including IPIs), as the result it cannot handle
> SMP call for stack dump.
>
> This patch is to enable coresight debug module, so firstly this driver
> is to bind apb clock for debug module and this is to ensure the debug
> module can be accessed from program or external debugger. And the driver
> uses sample-based registers for debug purpose, e.g. when system triggers
> panic, the driver will dump program counter and combined context
> registers (EDCIDSR, EDVIDSR); by parsing context registers so can
> quickly get to know CPU secure state, exception level, etc.
>
> Some of the debug module registers are located in CPU power domain, so
> this requires the CPU power domain stays on when access related debug
> registers, but the power management for CPU power domain is quite
> dependent on SoC integration for power management. For the platforms
> which with sane power controller implementations, this driver follows
> the method to set EDPRCR to try to pull the CPU out of low power state
> and then set 'no power down request' bit so the CPU has no chance to
> lose power.
>
> If the SoC has not followed up this design well for power management
> controller, the user should use the command line parameter or sysfs
> to constrain all or partial idle states to ensure the CPU power
> domain is enabled and access coresight CPU debug component safely.
>
> Signed-off-by: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
This is coming along well - a few comment below. In your next revision please
add GKH to the 'To' list.
Thanks,
Mathieu
> ---
> drivers/hwtracing/coresight/Kconfig | 14 +
> drivers/hwtracing/coresight/Makefile | 1 +
> drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 ++++++++++++++++++++++
> 3 files changed, 682 insertions(+)
> create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
>
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 130cb21..8d55d6d 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -89,4 +89,18 @@ config CORESIGHT_STM
> logging useful software events or data coming from various entities
> in the system, possibly running different OSs
>
> +config CORESIGHT_CPU_DEBUG
> + tristate "CoreSight CPU Debug driver"
> + depends on ARM || ARM64
> + depends on DEBUG_FS
> + help
> + This driver provides support for coresight debugging module. This
> + is primarily used to dump sample-based profiling registers when
> + system triggers panic, the driver will parse context registers so
> + can quickly get to know program counter (PC), secure state,
> + exception level, etc. Before use debugging functionality, platform
> + needs to ensure the clock domain and power domain are enabled
> + properly, please refer Documentation/trace/coresight-cpu-debug.txt
> + for detailed description and the example for usage.
> +
> endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index af480d9..433d590 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
> coresight-etm4x-sysfs.o
> obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
> obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> +obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> new file mode 100644
> index 0000000..8470e31
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -0,0 +1,667 @@
> +/*
> + * Copyright (c) 2017 Linaro Limited. All rights reserved.
> + *
> + * Author: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +#include <linux/amba/bus.h>
> +#include <linux/coresight.h>
> +#include <linux/cpu.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/pm_qos.h>
> +#include <linux/slab.h>
> +#include <linux/smp.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +
> +#include "coresight-priv.h"
> +
> +#define EDPCSR 0x0A0
> +#define EDCIDSR 0x0A4
> +#define EDVIDSR 0x0A8
> +#define EDPCSR_HI 0x0AC
> +#define EDOSLAR 0x300
> +#define EDPRCR 0x310
> +#define EDPRSR 0x314
> +#define EDDEVID1 0xFC4
> +#define EDDEVID 0xFC8
> +
> +#define EDPCSR_PROHIBITED 0xFFFFFFFF
> +
> +/* bits definition for EDPCSR */
> +#define EDPCSR_THUMB BIT(0)
> +#define EDPCSR_ARM_INST_MASK GENMASK(31, 2)
> +#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1)
> +
> +/* bits definition for EDPRCR */
> +#define EDPRCR_COREPURQ BIT(3)
> +#define EDPRCR_CORENPDRQ BIT(0)
> +
> +/* bits definition for EDPRSR */
> +#define EDPRSR_DLK BIT(6)
> +#define EDPRSR_PU BIT(0)
> +
> +/* bits definition for EDVIDSR */
> +#define EDVIDSR_NS BIT(31)
> +#define EDVIDSR_E2 BIT(30)
> +#define EDVIDSR_E3 BIT(29)
> +#define EDVIDSR_HV BIT(28)
> +#define EDVIDSR_VMID GENMASK(7, 0)
> +
> +/*
> + * bits definition for EDDEVID1:PSCROffset
> + *
> + * NOTE: armv8 and armv7 have different definition for the register,
> + * so consolidate the bits definition as below:
> + *
> + * 0b0000 - Sample offset applies based on the instruction state, we
> + * rely on EDDEVID to check if EDPCSR is implemented or not
> + * 0b0001 - No offset applies.
> + * 0b0010 - No offset applies, but do not use in AArch32 mode
> + *
> + */
> +#define EDDEVID1_PCSR_OFFSET_MASK GENMASK(3, 0)
> +#define EDDEVID1_PCSR_OFFSET_INS_SET (0x0)
> +#define EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 (0x2)
> +
> +/* bits definition for EDDEVID */
> +#define EDDEVID_PCSAMPLE_MODE GENMASK(3, 0)
> +#define EDDEVID_IMPL_EDPCSR (0x1)
> +#define EDDEVID_IMPL_EDPCSR_EDCIDSR (0x2)
> +#define EDDEVID_IMPL_FULL (0x3)
> +
> +#define DEBUG_WAIT_SLEEP 1000
> +#define DEBUG_WAIT_TIMEOUT 32000
> +
> +struct debug_drvdata {
> + void __iomem *base;
> e struct device *dev;
> + int cpu;
> +
> + bool edpcsr_present;
> + bool edcidsr_present;
> + bool edvidsr_present;
> + bool pc_has_offset;
> +
> + u32 edpcsr;
> + u32 edpcsr_hi;
> + u32 edprsr;
> + u32 edvidsr;
> + u32 edcidsr;
> +};
> +
> +static DEFINE_MUTEX(debug_lock);
> +static DEFINE_PER_CPU(struct debug_drvdata *, debug_drvdata);
> +static int debug_count;
> +static struct dentry *debug_debugfs_dir;
> +
> +static bool debug_enable;
> +module_param_named(enable, debug_enable, bool, 0600);
> +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
> + "(default is 0, which means is disabled by default)");
For this driver we have a debugFS interface so I question the validity of a
kernel module parameter. Other than adding complexity to the code it offers no
real added value. If a user is to insmod a module, it is just as easy to switch
on the functionality using debugFS in a second step.
> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> + /* Unlocks the debug registers */
> + writel_relaxed(0x0, drvdata->base + EDOSLAR);
Here the wmb is to make sure reordering (either at compile time or in the CPU)
doesn't happend. You need a comment here otherwise checkpatch will complain.
Speaking of which, did you run this through checkpatch?
> + wmb();
> +}
> +
> +/*
> + * According to ARM DDI 0487A.k, before access external debug
> + * registers should firstly check the access permission; if any
> + * below condition has been met then cannot access debug
> + * registers to avoid lockup issue:
> + *
> + * - CPU power domain is powered off;
> + * - The OS Double Lock is locked;
> + *
> + * By checking EDPRSR can get to know if meet these conditions.
> + */
> +static bool debug_access_permitted(struct debug_drvdata *drvdata)
> +{
> + /* CPU is powered off */
> + if (!(drvdata->edprsr & EDPRSR_PU))
> + return false;
> +
> + /* The OS Double Lock is locked */
> + if (drvdata->edprsr & EDPRSR_DLK)
> + return false;
> +
> + return true;
> +}
> +
> +static void debug_force_cpu_powered_up(struct debug_drvdata *drvdata)
> +{
> + bool retried = false;
> + u32 edprcr;
> +
> +try_again:
> +
> + /*
> + * Send request to power management controller and assert
> + * DBGPWRUPREQ signal; if power management controller has
> + * sane implementation, it should enable CPU power domain
> + * in case CPU is in low power state.
> + */
> + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> + edprcr |= EDPRCR_COREPURQ;
> + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> +
> + /* Wait for CPU to be powered up (timeout~=32ms) */
> + if (readx_poll_timeout_atomic(readl_relaxed, drvdata->base + EDPRSR,
> + drvdata->edprsr, (drvdata->edprsr & EDPRSR_PU),
> + DEBUG_WAIT_SLEEP, DEBUG_WAIT_TIMEOUT)) {
> + /*
> + * Unfortunately the CPU cannot be powered up, so return
> + * back and later has no permission to access other
> + * registers. For this case, should disable CPU low power
> + * states to ensure CPU power domain is enabled!
> + */
> + pr_err("%s: power up request for CPU%d failed\n",
> + __func__, drvdata->cpu);
> + return;
> + }
> +
> + /*
> + * At this point the CPU is powered up, so set the no powerdown
> + * request bit so we don't lose power and emulate power down.
> + */
> + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> + edprcr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
> + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> +
> + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> +
> + /* Bail out if CPU is powered up */
> + if (likely(drvdata->edprsr & EDPRSR_PU))
> + return;
/* The core power domain got switched off on use, try again */
if (unlikely(!drvdata->edprsr & EDPRSR_PU))
goto try_again;
I understand you don't want to introduce a infinite loop but if that happens
here, something else has gone very wrong. The above readx_poll_timeout_atomic
loop should take care of bailing out in case of problems. That way you also get
to rid of the retried variable and the code is more simple.
> +
> + /*
> + * Handle race condition if CPU has been waken up but it sleeps
> + * again if EDPRCR_CORENPDRQ has been flipped, so try to run
> + * waken flow one more time.
> + */
> + if (!retried) {
> + retried = true;
> + goto try_again;
> + }
> +}
> +
> +static void debug_read_regs(struct debug_drvdata *drvdata)
> +{
> + u32 save_edprcr;
> +
> + CS_UNLOCK(drvdata->base);
> +
> + /* Unlock os lock */
> + debug_os_unlock(drvdata);
> +
> + /* Save EDPRCR register */
> + save_edprcr = readl_relaxed(drvdata->base + EDPRCR);
> +
> + /*
> + * Ensure CPU power domain is enabled to let registers
> + * are accessiable.
> + */
> + debug_force_cpu_powered_up(drvdata);
> +
> + if (!debug_access_permitted(drvdata))
> + goto out;
> +
> + drvdata->edpcsr = readl_relaxed(drvdata->base + EDPCSR);
> +
> + /*
> + * As described in ARM DDI 0487A.k, if the processing
> + * element (PE) is in debug state, or sample-based
> + * profiling is prohibited, EDPCSR reads as 0xFFFFFFFF;
> + * EDCIDSR, EDVIDSR and EDPCSR_HI registers also become
> + * UNKNOWN state. So directly bail out for this case.
> + */
> + if (drvdata->edpcsr == EDPCSR_PROHIBITED)
> + goto out;
> +
> + /*
> + * A read of the EDPCSR normally has the side-effect of
> + * indirectly writing to EDCIDSR, EDVIDSR and EDPCSR_HI;
> + * at this point it's safe to read value from them.
> + */
> + if (IS_ENABLED(CONFIG_64BIT))
> + drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> + if (drvdata->edcidsr_present)
> + drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR);
> +
> + if (drvdata->edvidsr_present)
> + drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR);
> +
> +out:
> + /* Restore EDPRCR register */
> + writel_relaxed(save_edprcr, drvdata->base + EDPRCR);
> +
> + CS_LOCK(drvdata->base);
> +}
> +
> +static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata)
> +{
> + unsigned long arm_inst_offset = 0, thumb_inst_offset = 0;
> + unsigned long pc;
> +
> + if (IS_ENABLED(CONFIG_64BIT))
> + return (unsigned long)drvdata->edpcsr_hi << 32 |
> + (unsigned long)drvdata->edpcsr;
> +
> + pc = (unsigned long)drvdata->edpcsr;
> +
> + if (drvdata->pc_has_offset) {
> + arm_inst_offset = 8;
> + thumb_inst_offset = 4;
> + }
> +
> + /* Handle thumb instruction */
> + if (pc & EDPCSR_THUMB) {
> + pc = (pc & EDPCSR_THUMB_INST_MASK) - thumb_inst_offset;
> + return pc;
> + }
> +
> + /*
> + * Handle arm instruction offset, if the arm instruction
> + * is not 4 byte alignment then it's possible the case
> + * for implementation defined; keep original value for this
> + * case and print info for notice.
> + */
> + if (pc & BIT(1))
> + pr_emerg("Instruction offset is implementation defined\n");
> + else
> + pc = (pc & EDPCSR_ARM_INST_MASK) - arm_inst_offset;
> +
> + return pc;
> +}
> +
> +static void debug_dump_regs(struct debug_drvdata *drvdata)
> +{
> + unsigned long pc;
> +
> + pr_emerg("\tEDPRSR: %08x (Power:%s DLK:%s)\n", drvdata->edprsr,
> + drvdata->edprsr & EDPRSR_PU ? "On" : "Off",
> + drvdata->edprsr & EDPRSR_DLK ? "Lock" : "Unlock");
> +
> + if (!debug_access_permitted(drvdata)) {
> + pr_emerg("No permission to access debug registers!\n");
> + return;
> + }
> +
> + if (drvdata->edpcsr == EDPCSR_PROHIBITED) {
> + pr_emerg("CPU is in Debug state or profiling is prohibited!\n");
> + return;
> + }
> +
> + pc = debug_adjust_pc(drvdata);
> + pr_emerg("\tEDPCSR: [<%p>] %pS\n", (void *)pc, (void *)pc);
> +
> + if (drvdata->edcidsr_present)
> + pr_emerg("\tEDCIDSR: %08x\n", drvdata->edcidsr);
> +
> + if (drvdata->edvidsr_present)
> + pr_emerg("\tEDVIDSR: %08x (State:%s Mode:%s Width:%dbits VMID:%x)\n",
> + drvdata->edvidsr,
> + drvdata->edvidsr & EDVIDSR_NS ? "Non-secure" : "Secure",
> + drvdata->edvidsr & EDVIDSR_E3 ? "EL3" :
> + (drvdata->edvidsr & EDVIDSR_E2 ? "EL2" : "EL1/0"),
> + drvdata->edvidsr & EDVIDSR_HV ? 64 : 32,
> + drvdata->edvidsr & (u32)EDVIDSR_VMID);
> +}
> +
> +static void debug_init_arch_data(void *info)
> +{
> + struct debug_drvdata *drvdata = info;
> + u32 mode, pcsr_offset;
> + u32 eddevid, eddevid1;
> +
> + CS_UNLOCK(drvdata->base);
> +
> + /* Read device info */
> + eddevid = readl_relaxed(drvdata->base + EDDEVID);
> + eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
> +
> + CS_LOCK(drvdata->base);
> +
> + /* Parse implementation feature */
> + mode = eddevid & EDDEVID_PCSAMPLE_MODE;
> + pcsr_offset = eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
> +
> + drvdata->edpcsr_present = false;
> + drvdata->edcidsr_present = false;
> + drvdata->edvidsr_present = false;
> + drvdata->pc_has_offset = false;
> +
> + switch (mode) {
> + case EDDEVID_IMPL_FULL:
> + drvdata->edvidsr_present = true;
> + /* Fall through */
> + case EDDEVID_IMPL_EDPCSR_EDCIDSR:
> + drvdata->edcidsr_present = true;
> + /* Fall through */
> + case EDDEVID_IMPL_EDPCSR:
> + /*
> + * In ARM DDI 0487A.k, the EDDEVID1.PCSROffset is used to
> + * define if has the offset for PC sampling value; if read
> + * back EDDEVID1.PCSROffset == 0x2, then this means the debug
> + * module does not sample the instruction set state when
> + * armv8 CPU in AArch32 state.
> + */
> + drvdata->edpcsr_present = (IS_ENABLED(CONFIG_64BIT) ||
> + (pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32));
> +
> + drvdata->pc_has_offset =
> + (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET);
> + break;
> + default:
> + break;
> + }
> +}
> +
> +/*
> + * Dump out information on panic.
> + */
> +static int debug_notifier_call(struct notifier_block *self,
> + unsigned long v, void *p)
> +{
> + int cpu;
> + struct debug_drvdata *drvdata;
> +
> + pr_emerg("ARM external debug module:\n");
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pr_emerg("CPU[%d]:\n", drvdata->cpu);
> +
> + debug_read_regs(drvdata);
> + debug_dump_regs(drvdata);
> + }
> +
> + return 0;
> +}
> +
> +static struct notifier_block debug_notifier = {
> + .notifier_call = debug_notifier_call,
> +};
> +
> +static int debug_enable_func(void)
> +{
> + struct debug_drvdata *drvdata;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pm_runtime_get_sync(drvdata->dev);
> + }
> +
> + return atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static int debug_disable_func(void)
> +{
> + struct debug_drvdata *drvdata;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pm_runtime_put(drvdata->dev);
> + }
> +
> + return atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static ssize_t debug_func_knob_write(struct file *f,
> + const char __user *buf, size_t count, loff_t *ppos)
> +{
> + u8 val;
> + int ret;
> +
> + ret = kstrtou8_from_user(buf, count, 2, &val);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&debug_lock);
> +
> + if (val == debug_enable)
> + goto out;
> +
> + if (val)
> + ret = debug_enable_func();
> + else
> + ret = debug_disable_func();
I don't think you need to install the handler every time the functionality is
switched on/off. I suggest to install the handler at boot time (or module
insertion time) and in the notifier handler, check the debug_enable flag before
moving on with the output.
> +
> + if (ret) {
> + pr_err("%s: unable to %s debug function: %d\n",
> + __func__, val ? "enable" : "disable", ret);
> + goto err;
> + }
> +
> + debug_enable = val;
Using a true/false value is probably better here. That way you don't end up
with miscellaneous values in debugFS.
> +out:
> + ret = count;
> +err:
> + mutex_unlock(&debug_lock);
> + return ret;
> +}
> +
> +static ssize_t debug_func_knob_read(struct file *f,
> + char __user *ubuf, size_t count, loff_t *ppos)
> +{
> + ssize_t ret;
> + char buf[2];
> +
> + mutex_lock(&debug_lock);
> +
> + buf[0] = '0' + debug_enable;
> + buf[1] = '\n';
snprintf()
> + ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
> +
> + mutex_unlock(&debug_lock);
> + return ret;
> +}
> +
> +static const struct file_operations debug_func_knob_fops = {
> + .open = simple_open,
> + .read = debug_func_knob_read,
> + .write = debug_func_knob_write,
> +};
> +
> +static int debug_func_init(void)
> +{
> + struct dentry *file;
> + int ret;
> +
> + /* Create debugfs node */
> + debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
> + if (!debug_debugfs_dir) {
> + pr_err("%s: unable to create debugfs directory\n", __func__);
> + return -ENOMEM;
> + }
> +
> + file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
> + debug_debugfs_dir, NULL, &debug_func_knob_fops);
Please align this properly.
> + if (!file) {
> + pr_err("%s: unable to create enable knob file\n", __func__);
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + /* Use sysfs node to enable functionality */
> + if (!debug_enable)
> + return 0;
> +
> + /* Register function to be called for panic */
> + ret = atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> + if (ret) {
> + pr_err("%s: unable to register notifier: %d\n",
> + __func__, ret);
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + debugfs_remove_recursive(debug_debugfs_dir);
> + return ret;
> +}
> +
> +static void debug_func_exit(void)
> +{
> + debugfs_remove_recursive(debug_debugfs_dir);
> +
> + /* Unregister panic notifier callback */
> + if (debug_enable)
> + atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> +{
> + void __iomem *base;
> + struct device *dev = &adev->dev;
> + struct debug_drvdata *drvdata;
> + struct resource *res = &adev->res;
> + struct device_node *np = adev->dev.of_node;
> + int ret;
> +
> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> + if (per_cpu(debug_drvdata, drvdata->cpu)) {
> + dev_err(dev, "CPU%d drvdata has been initialized\n",
> + drvdata->cpu);
> + return -EBUSY;
> + }
> +
> + drvdata->dev = &adev->dev;
> + amba_set_drvdata(adev, drvdata);
> +
> + /* Validity for the resource is already checked by the AMBA core */
> + base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + drvdata->base = base;
> +
> + get_online_cpus();
> + per_cpu(debug_drvdata, drvdata->cpu) = drvdata;
> + ret = smp_call_function_single(drvdata->cpu,
> + debug_init_arch_data, drvdata, 1);
> + put_online_cpus();
> +
> + if (ret) {
> + dev_err(dev, "CPU%d debug arch init failed\n", drvdata->cpu);
> + goto err;
> + }
> +
> + if (!drvdata->edpcsr_present) {
> + dev_err(dev, "CPU%d sample-based profiling isn't implemented\n",
> + drvdata->cpu);
> + ret = -ENXIO;
> + goto err;
> + }
> +
> + if (!debug_count++) {
> + ret = debug_func_init();
> + if (ret)
> + goto err_func_init;
> + }
> +
> + if (!debug_enable)
> + pm_runtime_put(dev);
> +
> + dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu);
> + return 0;
> +
> +err_func_init:
> + debug_count--;
> +err:
> + per_cpu(debug_drvdata, drvdata->cpu) = NULL;
> + return ret;
> +}
> +
> +static int debug_remove(struct amba_device *adev)
> +{
> + struct device *dev = &adev->dev;
> + struct debug_drvdata *drvdata = amba_get_drvdata(adev);
> +
> + per_cpu(debug_drvdata, drvdata->cpu) = NULL;
> +
> + if (debug_enable)
> + pm_runtime_put(dev);
> +
> + if (!--debug_count)
> + debug_func_exit();
> +
> + return 0;
> +}
> +
> +static struct amba_id debug_ids[] = {
> + { /* Debug for Cortex-A53 */
> + .id = 0x000bbd03,
> + .mask = 0x000fffff,
> + },
> + { /* Debug for Cortex-A57 */
> + .id = 0x000bbd07,
> + .mask = 0x000fffff,
> + },
> + { /* Debug for Cortex-A72 */
> + .id = 0x000bbd08,
> + .mask = 0x000fffff,
> + },
> + { 0, 0 },
> +};
> +
> +static struct amba_driver debug_driver = {
> + .drv = {
> + .name = "coresight-cpu-debug",
> + .suppress_bind_attrs = true,
> + },
> + .probe = debug_probe,
> + .remove = debug_remove,
> + .id_table = debug_ids,
> +};
> +
> +module_amba_driver(debug_driver);
> +
> +MODULE_AUTHOR("Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>");
> +MODULE_DESCRIPTION("ARM Coresight CPU Debug Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>
--
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] drm: rcar-du: Document the vsps property in the DT bindings
From: Geert Uytterhoeven @ 2017-04-18 17:36 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Laurent Pinchart, Sergei Shtylyov, devicetree@vger.kernel.org,
DRI Development, Linux-Renesas
In-Reply-To: <8105647.ANSr0hWnZy@avalon>
Hi Laurent,
On Fri, Mar 31, 2017 at 11:19 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 27 Mar 2017 13:05:48 Geert Uytterhoeven wrote:
>> On Mon, Mar 27, 2017 at 11:56 AM, Laurent Pinchart wrote:
>> > The property is used by the driver but is missing from the DT bindings.
>> > Document it.
>> >
>> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> > Signed-off-by: Laurent Pinchart
>> > <laurent.pinchart+renesas@ideasonboard.com>
>> > ---
>> >
>> > Documentation/devicetree/bindings/display/renesas,du.txt | 5 +++++
>> > 1 file changed, 5 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt
>> > b/Documentation/devicetree/bindings/display/renesas,du.txt index
>> > 1a02f099a0ff..cf34893a1b53 100644
>> > --- a/Documentation/devicetree/bindings/display/renesas,du.txt
>> > +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
>> >
>> > @@ -36,6 +36,11 @@ Required Properties:
>> > When supplied they must be named "dclkin.x" with "x" being the
>> > input
>> > clock numerical index.
>> >
>> > +Optional Properties:
>> > +
>> > + - vsps: A list of phandles to the VSP nodes that handle the memory
>> > + interfaces for the DU channels (Gen3 only).
>>
>> ", one per channel"?
>>
>> Required for Gen3, optional for Gen2? (cfr. Sergei's patches).
>
> How about making it mandatory on Gen2 as well ? The VSPs are there, even if
> the driver doesn't use them, it makes sense to describe the connection. Of
> course the driver will treat the property as optional for backward
> compatibility.
Now it's mandatory, the vsps property should be present in the example, too.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* RE: [PATCH v3 5/7] doc_rst: media: New SDR formats PC16, PC18 & PC20
From: Ramesh Shanmugasundaram @ 2017-04-18 17:13 UTC (permalink / raw)
To: Laurent Pinchart
Cc: robh+dt@kernel.org, mark.rutland@arm.com, mchehab@kernel.org,
hverkuil@xs4all.nl, sakari.ailus@linux.intel.com, crope@iki.fi,
Chris Paterson, geert+renesas@glider.be,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <213133284.36Jzg1zrIM@avalon>
Hi Laurent,
Thanks for the review comments.
> On Tuesday 07 Feb 2017 15:02:35 Ramesh Shanmugasundaram wrote:
> > This patch adds documentation for the three new SDR formats
> >
> > V4L2_SDR_FMT_PCU16BE
> > V4L2_SDR_FMT_PCU18BE
> > V4L2_SDR_FMT_PCU20BE
> >
> > Signed-off-by: Ramesh Shanmugasundaram
> > <ramesh.shanmugasundaram@bp.renesas.com> ---
> > .../media/uapi/v4l/pixfmt-sdr-pcu16be.rst | 55
> +++++++++++++++++++
> > .../media/uapi/v4l/pixfmt-sdr-pcu18be.rst | 55
> +++++++++++++++++++
> > .../media/uapi/v4l/pixfmt-sdr-pcu20be.rst | 54
> +++++++++++++++++++
> > Documentation/media/uapi/v4l/sdr-formats.rst | 3 ++
> > 4 files changed, 167 insertions(+)
> > create mode 100644
> > Documentation/media/uapi/v4l/pixfmt-sdr-pcu16be.rst
> > create mode 100644
> > Documentation/media/uapi/v4l/pixfmt-sdr-pcu18be.rst
> > create mode 100644
> > Documentation/media/uapi/v4l/pixfmt-sdr-pcu20be.rst
> >
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-sdr-pcu16be.rst
> > b/Documentation/media/uapi/v4l/pixfmt-sdr-pcu16be.rst new file mode
> > 100644 index 0000000..2de1b1a
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-sdr-pcu16be.rst
> > @@ -0,0 +1,55 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _V4L2-SDR-FMT-PCU16BE:
> > +
> > +******************************
> > +V4L2_SDR_FMT_PCU16BE ('PC16')
> > +******************************
> > +
> > +Planar complex unsigned 16-bit big endian IQ sample
> > +
> > +Description
> > +===========
> > +
> > +This format contains a sequence of complex number samples. Each
> > +complex number consist of two parts called In-phase and Quadrature
> > +(IQ). Both I and Q are represented as a 16 bit unsigned big endian
> > +number stored in
> > +32 bit space. The remaining unused bits within the 32 bit space will
> > +be padded with 0. I value starts first and Q value starts at an
> > +offset equalling half of the buffer size (i.e.) offset =
> > +buffersize/2. Out of the 16 bits, bit 15:2 (14 bit) is data and bit
> > +1:0 (2 bit) can be any value.
>
> This sounds very strange to me. Are the two lower bits always random ?
> What is that used for ?
It could be zeros or it could be status bits in case of MAX2175 (if enabled). I mentioned any value because the user app does not have any assumptions on these bits value.
>
> > +**Byte Order.**
> > +Each cell is one byte.
> > +
> > +.. flat-table::
> > + :header-rows: 1
> > + :stub-columns: 0
> > +
> > + * - Offset:
> > + - Byte B0
> > + - Byte B1
> > + - Byte B2
> > + - Byte B3
> > + * - start + 0:
> > + - I'\ :sub:`0[13:6]`
> > + - I'\ :sub:`0[5:0]; B1[1:0]=pad`
> > + - pad
> > + - pad
> > + * - start + 4:
> > + - I'\ :sub:`1[13:6]`
> > + - I'\ :sub:`1[5:0]; B1[1:0]=pad`
> > + - pad
> > + - pad
> > + * - ...
> > + * - start + offset:
> > + - Q'\ :sub:`0[13:6]`
> > + - Q'\ :sub:`0[5:0]; B1[1:0]=pad`
> > + - pad
> > + - pad
> > + * - start + offset + 4:
> > + - Q'\ :sub:`1[13:6]`
> > + - Q'\ :sub:`1[5:0]; B1[1:0]=pad`
> > + - pad
> > + - pad
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-sdr-pcu18be.rst
> > b/Documentation/media/uapi/v4l/pixfmt-sdr-pcu18be.rst new file mode
> > 100644 index 0000000..da8b26b
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-sdr-pcu18be.rst
> > @@ -0,0 +1,55 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _V4L2-SDR-FMT-PCU18BE:
> > +
> > +******************************
> > +V4L2_SDR_FMT_PCU18BE ('PC18')
> > +******************************
> > +
> > +Planar complex unsigned 18-bit big endian IQ sample
> > +
> > +Description
> > +===========
> > +
> > +This format contains a sequence of complex number samples. Each
> > +complex number consist of two parts called In-phase and Quadrature
> > +(IQ). Both I and Q are represented as a 18 bit unsigned big endian
> > +number stored in
> > +32 bit space. The remaining unused bits within the 32 bit space will
> > +be padded with 0. I value starts first and Q value starts at an
> > +offset equalling half of the buffer size (i.e.) offset =
> > +buffersize/2. Out of the 18 bits, bit 17:2 (16 bit) is data and bit
> > +1:0 (2 bit) can be any value.
> > +
> > +**Byte Order.**
> > +Each cell is one byte.
> > +
> > +.. flat-table::
> > + :header-rows: 1
> > + :stub-columns: 0
> > +
> > + * - Offset:
> > + - Byte B0
> > + - Byte B1
> > + - Byte B2
> > + - Byte B3
> > + * - start + 0:
> > + - I'\ :sub:`0[17:10]`
> > + - I'\ :sub:`0[9:2]`
> > + - I'\ :sub:`0[1:0]; B2[5:0]=pad`
> > + - pad
> > + * - start + 4:
> > + - I'\ :sub:`1[17:10]`
> > + - I'\ :sub:`1[9:2]`
> > + - I'\ :sub:`1[1:0]; B2[5:0]=pad`
> > + - pad
> > + * - ...
> > + * - start + offset:
> > + - Q'\ :sub:`0[17:10]`
> > + - Q'\ :sub:`0[9:2]`
> > + - Q'\ :sub:`0[1:0]; B2[5:0]=pad`
> > + - pad
> > + * - start + offset + 4:
> > + - Q'\ :sub:`1[17:10]`
> > + - Q'\ :sub:`1[9:2]`
> > + - Q'\ :sub:`1[1:0]; B2[5:0]=pad`
> > + - pad
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-sdr-pcu20be.rst
> > b/Documentation/media/uapi/v4l/pixfmt-sdr-pcu20be.rst new file mode
> > 100644 index 0000000..5499eed
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-sdr-pcu20be.rst
> > @@ -0,0 +1,54 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +.. _V4L2-SDR-FMT-PCU20BE:
> > +
> > +******************************
> > +V4L2_SDR_FMT_PCU20BE ('PC20')
> > +******************************
> > +
> > +Planar complex unsigned 20-bit big endian IQ sample
> > +
> > +Description
> > +===========
> > +
> > +This format contains a sequence of complex number samples. Each
> > +complex number consist of two parts called In-phase and Quadrature
> > +(IQ). Both I and Q are represented as a 20 bit unsigned big endian
> > +number stored in
> > +32 bit space. The remaining unused bits within the 32 bit space will
> > +be padded with 0. I value starts first and Q value starts at an
> > +offset equalling half of the buffer size (i.e.) offset =
> > +buffersize/2. Out of the 20 bits, bit 19:2 (18 bit) is data and bit
> > +1:0 (2 bit) can be any value.
> > +
> > +**Byte Order.**
> > +Each cell is one byte.
> > +
> > +.. flat-table::
> > + :header-rows: 1
> > + :stub-columns: 0
> > +
> > + * - Offset:
> > + - Byte B0
> > + - Byte B1
> > + - Byte B2
> > + - Byte B3
> > + * - start + 0:
> > + - I'\ :sub:`0[19:12]`
> > + - I'\ :sub:`0[11:4]`
> > + - I'\ :sub:`0[3:0]; B2[3:0]=pad`
> > + - pad
> > + * - start + 4:
> > + - I'\ :sub:`1[19:12]`
> > + - I'\ :sub:`1[11:4]`
> > + - I'\ :sub:`1[3:0]; B2[3:0]=pad`
> > + - pad
> > + * - ...
> > + * - start + offset:
> > + - Q'\ :sub:`0[19:12]`
> > + - Q'\ :sub:`0[11:4]`
> > + - Q'\ :sub:`0[3:0]; B2[3:0]=pad`
> > + - pad
> > + * - start + offset + 4:
> > + - Q'\ :sub:`1[19:12]`
> > + - Q'\ :sub:`1[11:4]`
> > + - Q'\ :sub:`1[3:0]; B2[3:0]=pad`
> > + - pad
> > diff --git a/Documentation/media/uapi/v4l/sdr-formats.rst
> > b/Documentation/media/uapi/v4l/sdr-formats.rst index f863c08..2037f5b
> > 100644
> > --- a/Documentation/media/uapi/v4l/sdr-formats.rst
> > +++ b/Documentation/media/uapi/v4l/sdr-formats.rst
> > @@ -17,3 +17,6 @@ These formats are used for :ref:`SDR <sdr>`
> > interface only. pixfmt-sdr-cs08
> > pixfmt-sdr-cs14le
> > pixfmt-sdr-ru12le
> > + pixfmt-sdr-pcu16be
> > + pixfmt-sdr-pcu18be
> > + pixfmt-sdr-pcu20be
Thanks,
Ramesh
^ permalink raw reply
* RE: [PATCH v3 7/7] media: platform: rcar_drif: Add DRIF support
From: Ramesh Shanmugasundaram @ 2017-04-18 17:12 UTC (permalink / raw)
To: Laurent Pinchart
Cc: robh+dt@kernel.org, mark.rutland@arm.com, mchehab@kernel.org,
hverkuil@xs4all.nl, sakari.ailus@linux.intel.com, crope@iki.fi,
Chris Paterson, geert+renesas@glider.be,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <2330193.GrgnD4vKf8@avalon>
Hi Laurent,
Many thanks for your time & the review comments. I have agreed to most of the comments and a few need further discussion. Could you please take a look at those?
> On Tuesday 07 Feb 2017 15:02:37 Ramesh Shanmugasundaram wrote:
> > This patch adds Digital Radio Interface (DRIF) support to R-Car Gen3
> SoCs.
> > The driver exposes each instance of DRIF as a V4L2 SDR device. A DRIF
> > device represents a channel and each channel can have one or two
> > sub-channels respectively depending on the target board.
> >
> > DRIF supports only Rx functionality. It receives samples from a RF
> > frontend tuner chip it is interfaced with. The combination of DRIF and
> > the tuner device, which is registered as a sub-device, determines the
> > receive sample rate and format.
> >
> > In order to be compliant as a V4L2 SDR device, DRIF needs to bind with
> > the tuner device, which can be provided by a third party vendor. DRIF
> > acts as a slave device and the tuner device acts as a master
> > transmitting the samples. The driver allows asynchronous binding of a
> > tuner device that is registered as a v4l2 sub-device. The driver can
> > learn about the tuner it is interfaced with based on port endpoint
> > properties of the device in device tree. The V4L2 SDR device inherits
> > the controls exposed by the tuner device.
> >
> > The device can also be configured to use either one or both of the
> > data pins at runtime based on the master (tuner) configuration.
> >
> > Signed-off-by: Ramesh Shanmugasundaram
> > <ramesh.shanmugasundaram@bp.renesas.com>
> > ---
> > drivers/media/platform/Kconfig | 25 +
> > drivers/media/platform/Makefile | 1 +
> > drivers/media/platform/rcar_drif.c | 1534
> > +++++++++++++++++++++++++++++++++
> > 3 files changed, 1560 insertions(+)
> > create mode 100644 drivers/media/platform/rcar_drif.c
>
> [snip]
>
> > diff --git a/drivers/media/platform/rcar_drif.c
> > b/drivers/media/platform/rcar_drif.c new file mode 100644 index
> > 0000000..88950e3
> > --- /dev/null
> > +++ b/drivers/media/platform/rcar_drif.c
> > @@ -0,0 +1,1534 @@
>
> [snip]
>
> > +/*
> > + * The R-Car DRIF is a receive only MSIOF like controller with an
> > + * external master device driving the SCK. It receives data into a
> > +FIFO,
> > + * then this driver uses the SYS-DMAC engine to move the data from
> > + * the device to memory.
> > + *
> > + * Each DRIF channel DRIFx (as per datasheet) contains two internal
> > + * channels DRIFx0 & DRIFx1 within itself with each having its own
> > resources
> > + * like module clk, register set, irq and dma. These internal
> > + channels
> > share
> > + * common CLK & SYNC from master. The two data pins D0 & D1 shall be
> > + * considered to represent the two internal channels. This internal
> > + split
> > + * is not visible to the master device.
> > + *
> > + * Depending on the master device, a DRIF channel can use
> > + * (1) both internal channels (D0 & D1) to receive data in parallel
> > + (or)
> > + * (2) one internal channel (D0 or D1) to receive data
> > + *
> > + * The primary design goal of this controller is to act as Digitial
> > + Radio
>
> s/Digitial/Digital/
Agreed
>
> > + * Interface that receives digital samples from a tuner device. Hence
> > + the
> > + * driver exposes the device as a V4L2 SDR device. In order to
> > + qualify as
> > + * a V4L2 SDR device, it should possess tuner interface as mandated
> > + by the
> > + * framework. This driver expects a tuner driver (sub-device) to bind
> > + * asynchronously with this device and the combined drivers shall
> > + expose
> > + * a V4L2 compliant SDR device. The DRIF driver is independent of the
> > + * tuner vendor.
> > + *
> > + * The DRIF h/w can support I2S mode and Frame start synchronization
> > + pulse
> > mode.
> > + * This driver is tested for I2S mode only because of the
> > + availability of
> > + * suitable master devices. Hence, not all configurable options of
> > + DRIF h/w
> > + * like lsb/msb first, syncdl, dtdl etc. are exposed via DT and I2S
> > defaults
> > + * are used. These can be exposed later if needed after testing.
> > + */
>
> [snip]
>
> > +#define to_rcar_drif_buf_pair(sdr, ch_num,
> > idx) (sdr->ch[!(ch_num)]->buf[idx])
>
> You should enclose both sdr and idx in parenthesis, as they can be
> expressions.
Agreed.
>
> > +
> > +#define for_each_rcar_drif_channel(ch, ch_mask) \
> > + for_each_set_bit(ch, ch_mask, RCAR_DRIF_MAX_CHANNEL)
> > +
> > +static const unsigned int num_hwbufs = 32;
>
> Is there a specific reason to make this a static const instead of a
> #define ?
Just style only. The #define needs a RCAR_DRIF_ prefix and I used this value in few places. The #define makes the statements longer.
>
> > +/* Debug */
> > +static unsigned int debug;
> > +module_param(debug, uint, 0644);
> > +MODULE_PARM_DESC(debug, "activate debug info");
> > +
> > +#define rdrif_dbg(level, sdr, fmt, arg...) \
> > + v4l2_dbg(level, debug, &sdr->v4l2_dev, fmt, ## arg)
> > +
> > +#define rdrif_err(sdr, fmt, arg...) \
> > + dev_err(sdr->v4l2_dev.dev, fmt, ## arg)
> > +
> > +/* Stream formats */
> > +struct rcar_drif_format {
> > + u32 pixelformat;
> > + u32 buffersize;
> > + u32 wdlen;
> > + u32 num_ch;
> > +};
> > +
> > +/* Format descriptions for capture */ static const struct
> > +rcar_drif_format formats[] = {
> > + {
> > + .pixelformat = V4L2_SDR_FMT_PCU16BE,
> > + .buffersize = RCAR_SDR_BUFFER_SIZE,
> > + .wdlen = 16,
> > + .num_ch = 2,
>
> How about aligning the = as in the other lines ?
Agreed
>
> num_ch is always set to 2. Should we remove it for now, and add it back
> later when we'll support single-channel formats ? I think we should avoid
> carrying dead code.
Actually single channel support is tested internally. If single & dual channels are already supported any future change will be just adding the new SDR format only, which could be relatively trivial for this driver. To add new SDR formats today I need to enable appropriate format in the tuner code too, which could be done later on a need basis by us or others.
>
> > + },
> > + {
> > + .pixelformat = V4L2_SDR_FMT_PCU18BE,
> > + .buffersize = RCAR_SDR_BUFFER_SIZE,
> > + .wdlen = 18,
> > + .num_ch = 2,
> > + },
> > + {
> > + .pixelformat = V4L2_SDR_FMT_PCU20BE,
> > + .buffersize = RCAR_SDR_BUFFER_SIZE,
> > + .wdlen = 20,
> > + .num_ch = 2,
> > + },
> > +};
> > +
> > +static const unsigned int NUM_FORMATS = ARRAY_SIZE(formats);
>
> Same question here, can't this be a define ? I think I'd even avoid
> NUM_FORMATS completely and use ARRAY_SIZE(formats) directly in the code,
> to make the boundary check more explicit when iterating over the array.
Agreed.
>
> > +
> > +/* Buffer for a received frame from one or both internal channels */
> > +struct rcar_drif_frame_buf {
> > + /* Common v4l buffer stuff -- must be first */
> > + struct vb2_v4l2_buffer vb;
> > + struct list_head list;
> > +};
> > +
> > +struct rcar_drif_async_subdev {
> > + struct v4l2_subdev *sd;
> > + struct v4l2_async_subdev asd;
> > +};
> > +
> > +/* DMA buffer */
> > +struct rcar_drif_hwbuf {
> > + void *addr; /* CPU-side address */
> > + unsigned int status; /* Buffer status flags */
> > +};
> > +
> > +/* Internal channel */
> > +struct rcar_drif {
> > + struct rcar_drif_sdr *sdr; /* Group device */
> > + struct platform_device *pdev; /* Channel's pdev */
> > + void __iomem *base; /* Base register address */
> > + resource_size_t start; /* I/O resource offset */
> > + struct dma_chan *dmach; /* Reserved DMA channel */
> > + struct clk *clkp; /* Module clock */
> > + struct rcar_drif_hwbuf *buf[RCAR_DRIF_MAX_NUM_HWBUFS]; /* H/W bufs
> */
> > + dma_addr_t dma_handle; /* Handle for all bufs */
> > + unsigned int num; /* Channel number */
> > + bool acting_sdr; /* Channel acting as SDR device */
> > +};
> > +
> > +/* DRIF V4L2 SDR */
> > +struct rcar_drif_sdr {
> > + struct device *dev; /* Platform device */
> > + struct video_device *vdev; /* V4L2 SDR device */
> > + struct v4l2_device v4l2_dev; /* V4L2 device */
> > +
> > + /* Videobuf2 queue and queued buffers list */
> > + struct vb2_queue vb_queue;
> > + struct list_head queued_bufs;
> > + spinlock_t queued_bufs_lock; /* Protects queued_bufs */
> > +
> > + struct mutex v4l2_mutex; /* To serialize ioctls */
> > + struct mutex vb_queue_mutex; /* To serialize streaming ioctls */
> > + struct v4l2_ctrl_handler ctrl_hdl; /* SDR control handler */
> > + struct v4l2_async_notifier notifier; /* For subdev (tuner) */
> > +
> > + /* Current V4L2 SDR format array index */
> > + unsigned int fmt_idx;
>
> Instead of storing the index I would store a pointer to the corresponding
> rcar_drif_format, looking up information about the current format will
> then be easier.
>
Agreed
> > +
> > + /* Device tree SYNC properties */
> > + u32 mdr1;
> > +
> > + /* Internals */
> > + struct rcar_drif *ch[RCAR_DRIF_MAX_CHANNEL]; /* DRIFx0,1 */
> > + unsigned long hw_ch_mask; /* Enabled channels per DT */
> > + unsigned long cur_ch_mask; /* Used channels for an SDR FMT */
> > + u32 num_hw_ch; /* Num of DT enabled channels */
> > + u32 num_cur_ch; /* Num of used channels */
> > + u32 hwbuf_size; /* Each DMA buffer size */
> > + u32 produced; /* Buffers produced by sdr dev */
> > +};
> > +
> > +/* Allocate buffer context */
> > +static int rcar_drif_alloc_bufctxt(struct rcar_drif_sdr *sdr) {
> > + struct rcar_drif_hwbuf *bufctx;
> > + unsigned int i, idx;
> > +
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + bufctx = kcalloc(num_hwbufs, sizeof(*bufctx), GFP_KERNEL);
>
> How about embedding the buffer contexts in the rcar_drif structure instead
> of just storing pointers there ? The rcar_drif_hwbuf structure is pretty
> small, it won't make a big difference, and will simplify the code.
Agreed
>
> > + if (!bufctx)
> > + return -ENOMEM;
> > +
> > + for (idx = 0; idx < num_hwbufs; idx++)
> > + sdr->ch[i]->buf[idx] = bufctx + idx;
> > + }
> > + return 0;
> > +}
>
> [snip]
>
> > +/* Release DMA channel */
> > +static void rcar_drif_release_dmachannel(struct rcar_drif_sdr *sdr)
>
> I would name the function rcar_drif_release_dma_channels as it handles all
> channels. Same for rcar_drif_alloc_dma_channels.
Agreed.
>
> > +{
> > + unsigned int i;
> > +
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask)
> > + if (sdr->ch[i]->dmach) {
> > + dma_release_channel(sdr->ch[i]->dmach);
> > + sdr->ch[i]->dmach = NULL;
> > + }
> > +}
> > +
> > +/* Allocate DMA channel */
> > +static int rcar_drif_alloc_dmachannel(struct rcar_drif_sdr *sdr) {
> > + struct dma_slave_config dma_cfg;
> > + unsigned int i;
> > + int ret = -ENODEV;
> > +
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + struct rcar_drif *ch = sdr->ch[i];
> > +
> > + ch->dmach = dma_request_slave_channel(&ch->pdev->dev, "rx");
> > + if (!ch->dmach) {
> > + rdrif_err(sdr, "ch%u: dma channel req failed\n", i);
> > + goto dmach_error;
> > + }
> > +
> > + /* Configure slave */
> > + memset(&dma_cfg, 0, sizeof(dma_cfg));
> > + dma_cfg.src_addr = (phys_addr_t)(ch->start +
> RCAR_DRIF_SIRFDR);
> > + dma_cfg.dst_addr = 0;
>
> This isn't needed as you memset the whole structure to 0.
Agreed
>
> > + dma_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > + ret = dmaengine_slave_config(ch->dmach, &dma_cfg);
> > + if (ret) {
> > + rdrif_err(sdr, "ch%u: dma slave config failed\n", i);
> > + goto dmach_error;
> > + }
> > + }
> > + return 0;
> > +
> > +dmach_error:
> > + rcar_drif_release_dmachannel(sdr);
> > + return ret;
> > +}
>
> [snip]
>
> > +/* Set MDR defaults */
> > +static inline void rcar_drif_set_mdr1(struct rcar_drif_sdr *sdr) {
> > + unsigned int i;
> > +
> > + /* Set defaults for enabled internal channels */
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + /* Refer MSIOF section in manual for this register setting */
> > + writel(RCAR_DRIF_SITMDR1_PCON,
> > + sdr->ch[i]->base + RCAR_DRIF_SITMDR1);
>
> I would create a rcar_drif_write(struct rcar_drif *ch, u32 offset, u32
> data) function, the code will become clearer. Same for the read operation.
Agreed
>
> > + /* Setup MDR1 value */
> > + writel(sdr->mdr1, sdr->ch[i]->base + RCAR_DRIF_SIRMDR1);
> > +
> > + rdrif_dbg(2, sdr, "ch%u: mdr1 = 0x%08x",
> > + i, readl(sdr->ch[i]->base + RCAR_DRIF_SIRMDR1));
>
> Once you've debugged the driver I'm not sure those debugging statements
> are still needed.
I would like to keep this statement please. This single register print would clarify if a user selected DT options and this could be enabled at runtime on a deployed board.
>
> > + }
> > +}
> > +
> > +/* Extract bitlen and wdcnt from given word length */ static int
> > +rcar_drif_convert_wdlen(struct rcar_drif_sdr *sdr,
> > + u32 wdlen, u32 *bitlen, u32 *wdcnt) {
> > + unsigned int i, nr_wds;
> > +
> > + /* FIFO register size is 32 bits */
> > + for (i = 0; i < 32; i++) {
> > + nr_wds = wdlen % (32 - i);
> > + if (nr_wds == 0) {
> > + *bitlen = 32 - i;
> > + *wdcnt = wdlen / *bitlen;
>
> Can't you store the bitlen and wdcnt values in the rcar_drif_format
> structure instead of recomputing them every time ?
Agreed
>
> > + break;
> > + }
> > + }
> > +
> > + /* Sanity check range */
> > + if (i == 32 || !(*bitlen >= 8 && *bitlen <= 32) ||
> > + !(*wdcnt >= 1 && *wdcnt <= 64)) {
> > + rdrif_err(sdr, "invalid wdlen %u configured\n", wdlen);
> > + return -EINVAL;
>
> You shouldn't have invalid wdlen values in the driver. I would remove this
> check as it makes error handling in the caller more complex.
Agreed
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/* Set DRIF receive format */
> > +static int rcar_drif_set_format(struct rcar_drif_sdr *sdr) {
> > + u32 bitlen, wdcnt, wdlen;
> > + unsigned int i;
> > + int ret = -EINVAL;
> > +
> > + wdlen = formats[sdr->fmt_idx].wdlen;
> > + rdrif_dbg(2, sdr, "setfmt: idx %u, wdlen %u, num_ch %u\n",
> > + sdr->fmt_idx, wdlen, formats[sdr->fmt_idx].num_ch);
> > +
> > + /* Sanity check */
> > + if (formats[sdr->fmt_idx].num_ch > sdr->num_cur_ch) {
> > + rdrif_err(sdr, "fmt idx %u current ch %u mismatch\n",
> > + sdr->fmt_idx, sdr->num_cur_ch);
> > + return ret;
>
> This should never happen, it should be caught at set format time.
But, this is the set format function?
>
> > + }
> > +
> > + /* Get bitlen & wdcnt from wdlen */
> > + ret = rcar_drif_convert_wdlen(sdr, wdlen, &bitlen, &wdcnt);
> > + if (ret)
> > + return ret;
> > +
> > + /* Setup group, bitlen & wdcnt */
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + u32 mdr;
> > +
> > + /* Two groups */
> > + mdr = RCAR_DRIF_MDR_GRPCNT(2) | RCAR_DRIF_MDR_BITLEN(bitlen) |
> > + RCAR_DRIF_MDR_WDCNT(wdcnt);
> > + writel(mdr, sdr->ch[i]->base + RCAR_DRIF_SIRMDR2);
> > +
> > + mdr = RCAR_DRIF_MDR_BITLEN(bitlen) |
> RCAR_DRIF_MDR_WDCNT(wdcnt);
> > + writel(mdr, sdr->ch[i]->base + RCAR_DRIF_SIRMDR3);
> > +
> > + rdrif_dbg(2, sdr, "ch%u: new mdr[2,3] = 0x%08x, 0x%08x\n",
> > + i, readl(sdr->ch[i]->base + RCAR_DRIF_SIRMDR2),
> > + readl(sdr->ch[i]->base + RCAR_DRIF_SIRMDR3));
> > + }
> > + return ret;
> > +}
> > +
> > +/* Release DMA buffers */
> > +static void rcar_drif_release_buf(struct rcar_drif_sdr *sdr) {
> > + unsigned int i;
> > +
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + struct rcar_drif *ch = sdr->ch[i];
> > +
> > + /* First entry contains the dma buf ptr */
> > + if (ch->buf[0] && ch->buf[0]->addr) {
> > + dma_free_coherent(&ch->pdev->dev,
> > + sdr->hwbuf_size * num_hwbufs,
> > + ch->buf[0]->addr, ch->dma_handle);
> > + ch->buf[0]->addr = NULL;
> > + }
> > + }
> > +}
> > +
> > +/* Request DMA buffers */
> > +static int rcar_drif_request_buf(struct rcar_drif_sdr *sdr) {
> > + int ret = -ENOMEM;
> > + unsigned int i, j;
> > + void *addr;
> > +
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + struct rcar_drif *ch = sdr->ch[i];
> > +
> > + /* Allocate DMA buffers */
> > + addr = dma_alloc_coherent(&ch->pdev->dev,
> > + sdr->hwbuf_size * num_hwbufs,
> > + &ch->dma_handle, GFP_KERNEL);
> > + if (!addr) {
> > + rdrif_err(sdr,
> > + "ch%u: dma alloc failed. num_hwbufs %u size %u\n",
> > + i, num_hwbufs, sdr->hwbuf_size);
> > + goto alloc_error;
> > + }
> > +
> > + /* Split the chunk and populate bufctxt */
> > + for (j = 0; j < num_hwbufs; j++) {
> > + ch->buf[j]->addr = addr + (j * sdr->hwbuf_size);
> > + ch->buf[j]->status = 0;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +alloc_error:
> > + return ret;
> > +}
> > +
> > +/* Setup vb_queue minimum buffer requirements */ static int
> > +rcar_drif_queue_setup(struct vb2_queue *vq,
> > + unsigned int *num_buffers, unsigned int *num_planes,
> > + unsigned int sizes[], struct device *alloc_devs[]) {
> > + struct rcar_drif_sdr *sdr = vb2_get_drv_priv(vq);
> > +
> > + /* Need at least 16 buffers */
> > + if (vq->num_buffers + *num_buffers < 16)
> > + *num_buffers = 16 - vq->num_buffers;
> > +
> > + *num_planes = 1;
> > + sizes[0] = PAGE_ALIGN(formats[sdr->fmt_idx].buffersize);
> > +
> > + rdrif_dbg(2, sdr, "num_bufs %d sizes[0] %d\n", *num_buffers,
> sizes[0]);
> > + return 0;
> > +}
> > +
> > +/* Enqueue buffer */
> > +static void rcar_drif_buf_queue(struct vb2_buffer *vb) {
> > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > + struct rcar_drif_sdr *sdr = vb2_get_drv_priv(vb->vb2_queue);
> > + struct rcar_drif_frame_buf *fbuf =
> > + container_of(vbuf, struct rcar_drif_frame_buf, vb);
> > + unsigned long flags;
> > +
> > + rdrif_dbg(2, sdr, "buf_queue idx %u\n", vb->index);
> > + spin_lock_irqsave(&sdr->queued_bufs_lock, flags);
> > + list_add_tail(&fbuf->list, &sdr->queued_bufs);
> > + spin_unlock_irqrestore(&sdr->queued_bufs_lock, flags); }
> > +
> > +/* Get a frame buf from list */
> > +static struct rcar_drif_frame_buf *
> > +rcar_drif_get_fbuf(struct rcar_drif_sdr *sdr) {
> > + struct rcar_drif_frame_buf *fbuf;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&sdr->queued_bufs_lock, flags);
> > + fbuf = list_first_entry_or_null(&sdr->queued_bufs, struct
> > + rcar_drif_frame_buf, list);
> > + if (!fbuf) {
> > + /*
> > + * App is late in enqueing buffers. Samples lost & there will
> > + * be a gap in sequence number when app recovers
> > + */
> > + rdrif_dbg(1, sdr, "\napp late: prod %u\n", sdr->produced);
> > + sdr->produced++; /* Increment the produced count anyway */
> > + spin_unlock_irqrestore(&sdr->queued_bufs_lock, flags);
> > + return NULL;
> > + }
> > + list_del(&fbuf->list);
> > + spin_unlock_irqrestore(&sdr->queued_bufs_lock, flags);
> > +
> > + return fbuf;
> > +}
> > +
> > +static inline bool rcar_drif_buf_pairs_done(struct rcar_drif_hwbuf
> *buf1,
> > + struct rcar_drif_hwbuf *buf2) {
> > + return (buf1->status & buf2->status & RCAR_DRIF_BUF_DONE); }
> > +
> > +/* Channel DMA complete */
> > +static void rcar_drif_channel_complete(struct rcar_drif *ch, u32 idx)
> > +{
> > + u32 str;
> > +
> > + ch->buf[idx]->status |= RCAR_DRIF_BUF_DONE;
> > +
> > + /* Check for DRIF errors */
> > + str = readl(ch->base + RCAR_DRIF_SISTR);
> > + if (unlikely(str & RCAR_DRIF_RFOVF)) {
> > + /* Writing the same clears it */
> > + writel(str, ch->base + RCAR_DRIF_SISTR);
> > +
> > + /* Overflow: some samples are lost */
> > + ch->buf[idx]->status |= RCAR_DRIF_BUF_OVERFLOW;
> > + }
> > +}
> > +
> > +/* Deliver buffer to user */
> > +static void rcar_drif_deliver_buf(struct rcar_drif *ch) {
> > + struct rcar_drif_sdr *sdr = ch->sdr;
> > + u32 idx = sdr->produced % num_hwbufs;
> > + struct rcar_drif_frame_buf *fbuf;
> > + bool overflow = false;
> > +
> > + rcar_drif_channel_complete(ch, idx);
> > +
> > + if (sdr->num_cur_ch == RCAR_DRIF_MAX_CHANNEL) {
> > + struct rcar_drif_hwbuf *bufi, *bufq;
> > +
> > + if (ch->num) {
> > + bufi = to_rcar_drif_buf_pair(sdr, ch->num, idx);
> > + bufq = ch->buf[idx];
> > + } else {
> > + bufi = ch->buf[idx];
> > + bufq = to_rcar_drif_buf_pair(sdr, ch->num, idx);
> > + }
> > +
> > + /* Check if both DMA buffers are done */
> > + if (!rcar_drif_buf_pairs_done(bufi, bufq))
> > + return;
> > +
> > + /* Clear buf done status */
> > + bufi->status &= ~RCAR_DRIF_BUF_DONE;
> > + bufq->status &= ~RCAR_DRIF_BUF_DONE;
> > +
> > + /* Get fbuf */
> > + fbuf = rcar_drif_get_fbuf(sdr);
> > + if (!fbuf)
> > + return;
> > +
> > + memcpy(vb2_plane_vaddr(&fbuf->vb.vb2_buf, 0),
> > + bufi->addr, sdr->hwbuf_size);
> > + memcpy(vb2_plane_vaddr(&fbuf->vb.vb2_buf, 0) + sdr-
> >hwbuf_size,
> > + bufq->addr, sdr->hwbuf_size);
>
> Ouch ! That's a high data rate memcpy that can be avoided. Why don't you
> DMA directly to the vb2 buffers ? You will need to use videobuf2-dma-
> contig instead of videobuf2-vmalloc, but apart from that there should be
> no issue.
Yes. We thought about this issue and considered this approach as a trade-off to avoid DRIF overflow issue. Overflow happens DMAC fails to consume from DRIF FIFO on time (i.e.) when user app is busy processing samples and fails to queue buffers to DMAC. After an overflow, the device needs to reset (streaming stop/restart sequence). With cyclic DMA, we de-coupled the h/w and user buffers to avoid a costly device reset when user app is busy or not scheduled. Some samples will be lost but that is identified with the sequence numbers and the action/policy can be left to the user. Are you OK with this please?
>
> > + if ((bufi->status | bufq->status) & RCAR_DRIF_BUF_OVERFLOW) {
> > + overflow = true;
> > + /* Clear the flag in status */
> > + bufi->status &= ~RCAR_DRIF_BUF_OVERFLOW;
> > + bufq->status &= ~RCAR_DRIF_BUF_OVERFLOW;
> > + }
> > + } else {
> > + struct rcar_drif_hwbuf *bufiq;
> > +
> > + /* Get fbuf */
> > + fbuf = rcar_drif_get_fbuf(sdr);
> > + if (!fbuf)
> > + return;
> > +
> > + bufiq = ch->buf[idx];
> > +
> > + memcpy(vb2_plane_vaddr(&fbuf->vb.vb2_buf, 0),
> > + bufiq->addr, sdr->hwbuf_size);
> > +
> > + if (bufiq->status & RCAR_DRIF_BUF_OVERFLOW) {
> > + overflow = true;
> > + /* Clear the flag in status */
> > + bufiq->status &= ~RCAR_DRIF_BUF_OVERFLOW;
> > + }
> > + }
> > +
> > + rdrif_dbg(2, sdr, "ch%u: prod %u\n", ch->num, sdr->produced);
> > +
> > + fbuf->vb.field = V4L2_FIELD_NONE;
> > + fbuf->vb.sequence = sdr->produced++;
> > + fbuf->vb.vb2_buf.timestamp = ktime_get_ns();
> > + vb2_set_plane_payload(&fbuf->vb.vb2_buf, 0,
> > + formats[sdr->fmt_idx].buffersize);
> > +
> > + /* Set error state on overflow */
> > + if (overflow)
> > + vb2_buffer_done(&fbuf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> > + else
> > + vb2_buffer_done(&fbuf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>
> Maybe
>
> vb2_buffer_done(&fbuf->vb.vb2_buf,
> overflow ? VB2_BUF_STATE_ERROR: VB2_BUF_STATE_DONE);
Agreed
>
> > +}
> > +
> > +/* DMA callback for each stage */
> > +static void rcar_drif_dma_complete(void *dma_async_param) {
> > + struct rcar_drif *ch = dma_async_param;
> > + struct rcar_drif_sdr *sdr = ch->sdr;
> > +
> > + mutex_lock(&sdr->vb_queue_mutex);
>
> Isn't the complete callback potentially called in interrupt context ? I
> know the rcar-dmac driver uses a threaded interrupt handler for that, but
> is that a guarantee of the DMA engine API ?
>
DMA engine API mentions the callback will be called in tasklet context. Hmm... I could convert that into spin_lock_irq() if that's OK.
> > +
> > + /* DMA can be terminated while the callback was waiting on lock */
> > + if (!vb2_is_streaming(&sdr->vb_queue))
>
> Can it ? The streaming flag is cleared after the stop_streaming operation
> is called, which will terminate all DMA transfers synchronously.
rcar-dmac did not have device_synchronize support, when I started with this patch set. There is a comment in the terminal_all call
1224 /*
1225 * FIXME: No new interrupt can occur now, but the IRQ thread might still
1226 * be running.
1227 */
Hence, I was a bit paranoid.
>
> > + goto stopped;
> > +
> > + rcar_drif_deliver_buf(ch);
> > +stopped:
> > + mutex_unlock(&sdr->vb_queue_mutex);
> > +}
> > +
> > +static int rcar_drif_qbuf(struct rcar_drif *ch) {
> > + struct rcar_drif_sdr *sdr = ch->sdr;
> > + dma_addr_t addr = ch->dma_handle;
> > + struct dma_async_tx_descriptor *rxd;
> > + dma_cookie_t cookie;
> > + int ret = -EIO;
> > +
> > + /* Setup cyclic DMA with given buffers */
> > + rxd = dmaengine_prep_dma_cyclic(ch->dmach, addr,
> > + sdr->hwbuf_size * num_hwbufs,
> > + sdr->hwbuf_size, DMA_DEV_TO_MEM,
> > + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> > + if (!rxd) {
> > + rdrif_err(sdr, "ch%u: prep dma cyclic failed\n", ch->num);
> > + return ret;
> > + }
> > +
> > + /* Submit descriptor */
> > + rxd->callback = rcar_drif_dma_complete;
> > + rxd->callback_param = ch;
> > + cookie = dmaengine_submit(rxd);
> > + if (dma_submit_error(cookie)) {
> > + rdrif_err(sdr, "ch%u: dma submit failed\n", ch->num);
> > + return ret;
> > + }
> > +
> > + dma_async_issue_pending(ch->dmach);
> > + return 0;
> > +}
> > +
> > +/* Enable reception */
> > +static int rcar_drif_enable_rx(struct rcar_drif_sdr *sdr) {
> > + unsigned int i;
> > + u32 ctr;
> > + int ret;
> > +
> > + /*
> > + * When both internal channels are enabled, they can be synchronized
> > + * only by the master
> > + */
> > +
> > + /* Enable receive */
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + ctr = readl(sdr->ch[i]->base + RCAR_DRIF_SICTR);
> > + ctr |= (RCAR_DRIF_SICTR_RX_RISING_EDGE |
> > + RCAR_DRIF_SICTR_RX_EN);
> > + writel(ctr, sdr->ch[i]->base + RCAR_DRIF_SICTR);
> > + }
> > +
> > + /* Check receive enabled */
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + ret = readl_poll_timeout(sdr->ch[i]->base + RCAR_DRIF_SICTR,
> > + ctr, ctr & RCAR_DRIF_SICTR_RX_EN,
> > + 2, 500000);
>
> A 2µs sleep for a 500ms total timeout seems very low to me, that will
> stress the CPU. Same comment for the other locations where you use
> readl_poll_timeout.
>
> How long does the channel typically take to get enabled ?
It takes ~6-7µs on my board. The manual did not specify the expected time and I used a worst case of 500ms as this is used in the on/off path only.
Would 7µs sleep time be acceptable instead of 2µs?
>
> > + if (ret) {
> > + rdrif_err(sdr, "ch%u: rx en failed. ctr 0x%08x\n",
> > + i, readl(sdr->ch[i]->base +
> RCAR_DRIF_SICTR));
> > + break;
> > + }
> > + }
> > + return ret;
> > +}
> > +
> > +/* Disable reception */
> > +static void rcar_drif_disable_rx(struct rcar_drif_sdr *sdr) {
> > + unsigned int i;
> > + u32 ctr;
> > + int ret;
> > +
> > + /* Disable receive */
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + ctr = readl(sdr->ch[i]->base + RCAR_DRIF_SICTR);
> > + ctr &= ~RCAR_DRIF_SICTR_RX_EN;
> > + writel(ctr, sdr->ch[i]->base + RCAR_DRIF_SICTR);
> > + }
> > +
> > + /* Check receive disabled */
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + ret = readl_poll_timeout(sdr->ch[i]->base + RCAR_DRIF_SICTR,
> > + ctr, !(ctr & RCAR_DRIF_SICTR_RX_EN),
> > + 2, 500000);
>
> How long does the channel typically take to get disabled ?
Same as above comment.
>
> > + if (ret)
> > + dev_warn(&sdr->vdev->dev,
> > + "ch%u: failed to disable rx. ctr 0x%08x\n",
> > + i, readl(sdr->ch[i]->base + RCAR_DRIF_SICTR));
> > + }
> > +}
> > +
> > +/* Start channel */
> > +static int rcar_drif_start_channel(struct rcar_drif *ch) {
> > + struct rcar_drif_sdr *sdr = ch->sdr;
> > + u32 ctr, str;
> > + int ret;
> > +
> > + /* Reset receive */
> > + writel(RCAR_DRIF_SICTR_RESET, ch->base + RCAR_DRIF_SICTR);
> > + ret = readl_poll_timeout(ch->base + RCAR_DRIF_SICTR,
> > + ctr, !(ctr & RCAR_DRIF_SICTR_RESET),
>
> The alignment is weird.
If I remember correctly, it is a checkpatch warning when I started with the patch set (Last year Oct timeframe).
>
> > + 2, 500000);
> > + if (ret) {
> > + rdrif_err(sdr, "ch%u: failed to reset rx. ctr 0x%08x\n",
> > + ch->num, readl(ch->base + RCAR_DRIF_SICTR));
> > + return ret;
> > + }
> > +
> > + /* Queue buffers for DMA */
> > + ret = rcar_drif_qbuf(ch);
> > + if (ret)
> > + return ret;
> > +
> > + /* Clear status register flags */
> > + str = RCAR_DRIF_RFFUL | RCAR_DRIF_REOF | RCAR_DRIF_RFSERR |
> > + RCAR_DRIF_RFUDF | RCAR_DRIF_RFOVF;
> > + writel(str, ch->base + RCAR_DRIF_SISTR);
> > +
> > + /* Enable DMA receive interrupt */
> > + writel(0x00009000, ch->base + RCAR_DRIF_SIIER);
> > +
> > + return ret;
> > +}
> > +
> > +/* Start receive operation */
> > +static int rcar_drif_start(struct rcar_drif_sdr *sdr) {
> > + unsigned int i;
> > + int ret;
> > +
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + ret = rcar_drif_start_channel(sdr->ch[i]);
> > + if (ret)
> > + goto start_error;
> > + }
> > +
> > + sdr->produced = 0;
> > + ret = rcar_drif_enable_rx(sdr);
> > +start_error:
>
> Don't you need to stop the channels that were successfully started if an
> error occurs ?
Thank you. I missed this :-(.
>
> > + return ret;
> > +}
> > +
> > +/* Stop channel */
> > +static void rcar_drif_stop_channel(struct rcar_drif *ch) {
> > + struct rcar_drif_sdr *sdr = ch->sdr;
> > + int ret, retries = 3;
> > +
> > + /* Disable DMA receive interrupt */
> > + writel(0x00000000, ch->base + RCAR_DRIF_SIIER);
> > +
> > + do {
> > + /* Terminate all DMA transfers */
> > + ret = dmaengine_terminate_sync(ch->dmach);
> > + if (!ret)
> > + break;
> > + rdrif_dbg(2, sdr, "stop retry\n");
> > + } while (--retries);
>
> Why do you need to retry the terminate operation, why does it fail ?
Yes, I think it can be removed. I cannot remember why I added retry here because rcar-dmac terminate_all always seem to return 0. Last year, there used to be a WARN_ON in rcar-dmac code when channel halt fails (i.e.) it is not guaranteed that all transfers are stopped even after calling dmaengine_terminate_sync(). With the character version of this driver, I used to have a test app to start/stream/stop/restart sequence & hit the WARN_ON in dmac code sometimes (https://www.spinics.net/lists/linux-renesas-soc/msg04840.html). I may have added this return value check assuming it is passed on but I cannot see that even in the old code.
So yes, I agree with your comment.
>
> > + WARN_ON(!retries);
> > +}
>
> [snip]
>
> > +/* Start streaming */
> > +static int rcar_drif_start_streaming(struct vb2_queue *vq, unsigned
> > +int
> > count)
> > +{
> > + struct rcar_drif_sdr *sdr = vb2_get_drv_priv(vq);
> > + unsigned int i, j;
> > + int ret;
> > +
> > + mutex_lock(&sdr->v4l2_mutex);
>
> I'm surprised, aren't the start_streaming and stop_streaming operations
> called with the video device lock held already by the v4l2-ioctl layer ? I
> think they should be, if they're not there's probably a bug somewhere.
I did not see it. Please correct me if this is wrong
v4l_streamon
vb2_ioctl_streamon
vb2_streamon
vb2_core_streamon
vb2_start_streaming
q->ops->start_streaming => rcar_drif_start_streaming
>
> > + for_each_rcar_drif_channel(i, &sdr->cur_ch_mask) {
> > + ret = clk_prepare_enable(sdr->ch[i]->clkp);
> > + if (ret)
> > + goto start_error;
> > + }
> > +
> > + /* Set default MDRx settings */
> > + rcar_drif_set_mdr1(sdr);
> > +
> > + /* Set new format */
> > + ret = rcar_drif_set_format(sdr);
> > + if (ret)
> > + goto start_error;
> > +
> > + if (sdr->num_cur_ch == RCAR_DRIF_MAX_CHANNEL)
> > + sdr->hwbuf_size =
> > + formats[sdr->fmt_idx].buffersize / RCAR_DRIF_MAX_CHANNEL;
> > + else
> > + sdr->hwbuf_size = formats[sdr->fmt_idx].buffersize;
> > +
> > + rdrif_dbg(1, sdr, "num_hwbufs %u, hwbuf_size %u\n",
> > + num_hwbufs, sdr->hwbuf_size);
> > +
> > + /* Alloc DMA channel */
> > + ret = rcar_drif_alloc_dmachannel(sdr);
> > + if (ret)
> > + goto start_error;
> > +
> > + /* Alloc buf context */
> > + ret = rcar_drif_alloc_bufctxt(sdr);
> > + if (ret)
> > + goto start_error;
> > +
> > + /* Request buffers */
> > + ret = rcar_drif_request_buf(sdr);
> > + if (ret)
> > + goto start_error;
> > +
> > + /* Start Rx */
> > + ret = rcar_drif_start(sdr);
> > + if (ret)
> > + goto start_error;
> > +
> > + mutex_unlock(&sdr->v4l2_mutex);
> > + rdrif_dbg(1, sdr, "started\n");
> > + return ret;
> > +
> > +start_error:
>
> As there's a single error label I would call this "error". Up to you.
Agreed.
>
> > + rcar_drif_release_queued_bufs(sdr, VB2_BUF_STATE_QUEUED);
> > + rcar_drif_release_buf(sdr);
> > + rcar_drif_release_bufctxt(sdr);
> > + rcar_drif_release_dmachannel(sdr);
> > + for (j = 0; j < i; j++)
> > + clk_disable_unprepare(sdr->ch[j]->clkp);
> > +
> > + mutex_unlock(&sdr->v4l2_mutex);
> > + return ret;
> > +}
>
> [snip]
>
> > +/* Vb2 ops */
> > +static struct vb2_ops rcar_drif_vb2_ops = {
>
> You can make this static const.
Agreed
>
> > + .queue_setup = rcar_drif_queue_setup,
> > + .buf_queue = rcar_drif_buf_queue,
> > + .start_streaming = rcar_drif_start_streaming,
> > + .stop_streaming = rcar_drif_stop_streaming,
> > + .wait_prepare = vb2_ops_wait_prepare,
> > + .wait_finish = vb2_ops_wait_finish,
> > +};
>
> [snip]
>
> > +static int rcar_drif_g_fmt_sdr_cap(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + struct rcar_drif_sdr *sdr = video_drvdata(file);
> > +
> > + f->fmt.sdr.pixelformat = formats[sdr->fmt_idx].pixelformat;
> > + f->fmt.sdr.buffersize = formats[sdr->fmt_idx].buffersize;
> > + memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
>
> I believe the core ioctl handling code already does this for you. Same for
> the other ioctl handlers in
Agreed
>
> > + return 0;
> > +}
> > +
> > +static int rcar_drif_s_fmt_sdr_cap(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + struct rcar_drif_sdr *sdr = video_drvdata(file);
> > + struct vb2_queue *q = &sdr->vb_queue;
> > + unsigned int i;
> > +
> > + if (vb2_is_busy(q))
> > + return -EBUSY;
> > +
> > + memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
> > + for (i = 0; i < NUM_FORMATS; i++) {
> > + if (formats[i].pixelformat == f->fmt.sdr.pixelformat) {
>
> The code would become more readable (at least in my opinion) if you just
> added a break here, and moved the code below after the loop. In case the
> requested format isn't found (i == NUM_FORMATS) you can then set i to 0
> and proceed, that will select the first available format as a default.
Agreed
>
> > + sdr->fmt_idx = i;
> > + f->fmt.sdr.buffersize = formats[i].buffersize;
> > +
> > + /*
> > + * If a format demands one channel only out of two
> > + * enabled channels, pick the 0th channel.
> > + */
> > + if (formats[i].num_ch < sdr->num_hw_ch) {
> > + sdr->cur_ch_mask = BIT(0);
> > + sdr->num_cur_ch = formats[i].num_ch;
> > + } else {
> > + sdr->cur_ch_mask = sdr->hw_ch_mask;
> > + sdr->num_cur_ch = sdr->num_hw_ch;
> > + }
> > +
> > + rdrif_dbg(1, sdr, "cur: idx %u mask %lu num %u\n",
> > + i, sdr->cur_ch_mask, sdr->num_cur_ch);
> > + return 0;
> > + }
> > + }
> > +
> > + if (rcar_drif_set_default_format(sdr)) {
> > + rdrif_err(sdr, "cannot set default format\n");
> > + return -EINVAL;
> > + }
> > +
> > + f->fmt.sdr.pixelformat = formats[sdr->fmt_idx].pixelformat;
> > + f->fmt.sdr.buffersize = formats[sdr->fmt_idx].buffersize;
> > + return 0;
> > +}
> > +
> > +static int rcar_drif_try_fmt_sdr_cap(struct file *file, void *priv,
> > + struct v4l2_format *f)
> > +{
> > + struct rcar_drif_sdr *sdr = video_drvdata(file);
> > + unsigned int i;
> > +
> > + memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
> > + for (i = 0; i < NUM_FORMATS; i++) {
> > + if (formats[i].pixelformat == f->fmt.sdr.pixelformat) {
> > + f->fmt.sdr.buffersize = formats[i].buffersize;
> > + return 0;
> > + }
> > + }
> > +
> > + f->fmt.sdr.pixelformat = formats[sdr->fmt_idx].pixelformat;
> > + f->fmt.sdr.buffersize = formats[sdr->fmt_idx].buffersize;
>
> The result of the TRY_FMT ioctl should not depend on the currently
> configured format. I would return a fixed format (for instance the first
> one in the formats array) in the default case.
Agreed
>
> > + return 0;
> > +}
> > +
> > +/* Tuner subdev ioctls */
> > +static int rcar_drif_enum_freq_bands(struct file *file, void *priv,
> > + struct v4l2_frequency_band *band) {
> > + struct rcar_drif_sdr *sdr = video_drvdata(file);
> > + struct v4l2_subdev *sd;
> > + int ret = 0;
> > +
> > + v4l2_device_for_each_subdev(sd, &sdr->v4l2_dev) {
> > + ret = v4l2_subdev_call(sd, tuner, enum_freq_bands, band);
>
> This won't work as-is when you'll have multiple subdevs. As the driver
> only supports a single connected subdev at the moment, I suggest storing a
> pointer to that subdev in the rcar_drif_sdr structure, and calling
> operations on that subdev explicitly instead of looping over all subdevs.
> The comment holds for all other ioctls.
Agreed.
>
> > + if (ret)
> > + break;
> > + }
> > + return ret;
> > +}
>
> [snip]
>
> > +static int rcar_drif_notify_bound(struct v4l2_async_notifier *notifier,
> > + struct v4l2_subdev *subdev,
> > + struct v4l2_async_subdev *asd) {
> > + struct rcar_drif_sdr *sdr =
> > + container_of(notifier, struct rcar_drif_sdr, notifier);
> > +
> > + /* Nothing to do at this point */
>
> If there's nothing to do you can just leave the bound callback
> unimplemented, it's optional.
Agreed.
>
> > + rdrif_dbg(2, sdr, "bound asd: %s\n", asd->match.of.node->name);
> > + return 0;
> > +}
> > +
> > +/* Sub-device registered notification callback */ static int
> > +rcar_drif_notify_complete(struct v4l2_async_notifier *notifier) {
> > + struct rcar_drif_sdr *sdr =
> > + container_of(notifier, struct rcar_drif_sdr, notifier);
> > + struct v4l2_subdev *sd;
> > + int ret;
> > +
> > + sdr->v4l2_dev.ctrl_handler = &sdr->ctrl_hdl;
> > +
> > + ret = v4l2_device_register_subdev_nodes(&sdr->v4l2_dev);
> > + if (ret) {
> > + rdrif_err(sdr, "failed register subdev nodes ret %d\n", ret);
> > + return ret;
> > + }
>
> Do you need to expose subdev nodes to userspace ? Can't everything be
> handled from the V4L2 SDR node ?
As of today, everything can be handled from the V4L2 SDR node with current MAX2175 subdev. If the tuner driver is enhanced later, this would help.
>
> > + v4l2_device_for_each_subdev(sd, &sdr->v4l2_dev) {
> > + ret = v4l2_ctrl_add_handler(sdr->v4l2_dev.ctrl_handler,
> > + sd->ctrl_handler, NULL);
>
> Shouldn't you undo this somewhere when unbinding the subdevs ?
>
> > + if (ret) {
> > + rdrif_err(sdr, "failed ctrl add hdlr ret %d\n", ret);
> > + return ret;
> > + }
> > + }
> > + rdrif_dbg(2, sdr, "notify complete\n");
> > + return 0;
> > +}
>
> [snip]
>
> > +/* Parse sub-devs (tuner) to find a matching device */ static int
> > +rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr,
> > + struct device *dev)
> > +{
> > + struct v4l2_async_notifier *notifier = &sdr->notifier;
> > + struct rcar_drif_async_subdev *rsd;
> > + struct device_node *node;
> > +
> > + notifier->subdevs = devm_kzalloc(dev, sizeof(*notifier->subdevs),
> > + GFP_KERNEL);
> > + if (!notifier->subdevs)
> > + return -ENOMEM;
> > +
> > + node = of_graph_get_next_endpoint(dev->of_node, NULL);
> > + if (!node)
> > + return 0;
> > +
> > + rsd = devm_kzalloc(dev, sizeof(*rsd), GFP_KERNEL);
> > + if (!rsd) {
> > + of_node_put(node);
>
> If you move the allocation above of_graph_get_next_endpoint() you won't
> have to call of_node_put() in the error path.
Agreed
>
> > + return -ENOMEM;
> > + }
> > +
> > + notifier->subdevs[notifier->num_subdevs] = &rsd->asd;
> > + rsd->asd.match.of.node = of_graph_get_remote_port_parent(node);
>
> Aren't you missing an of_node_put() on the returned node ? Or does the
> async framework take care of that ?
You are right. of_node_put() on the returned node is missing. I will add it.
>
> > + of_node_put(node);
> > + if (!rsd->asd.match.of.node) {
> > + dev_warn(dev, "bad remote port parent\n");
> > + return -EINVAL;
> > + }
> > +
> > + rsd->asd.match_type = V4L2_ASYNC_MATCH_OF;
> > + notifier->num_subdevs++;
> > +
> > + /* Get the endpoint properties */
> > + rcar_drif_get_ep_properties(sdr, node);
> > + return 0;
> > +}
> > +
> > +/* Check if the given device is the primary bond */ static bool
> > +rcar_drif_primary_bond(struct platform_device *pdev) {
> > + if (of_find_property(pdev->dev.of_node, "renesas,primary-bond",
> NULL))
> > + return true;
> > +
> > + return false;
>
> How about
>
> return of_property_read_bool(pdev->dev.of_node,
> "renesas,primary-bond");
Shall I remove this function itself and just use the property in the "if" condition please? It's used in one place only. I tend to not touch the bindings name/type as we have some sort of agreement after ~4months time :-)
>
> > +}
> > +
> > +/* Get the bonded platform dev if enabled */ static struct
> > +platform_device *rcar_drif_enabled_bond(struct
> > platform_device *p)
> > +{
> > + struct device_node *np;
> > +
> > + np = of_parse_phandle(p->dev.of_node, "renesas,bonding", 0);
>
> The function takes a reference to np, you need to call of_node_put() on it
> (only if the returned pointer isn't NULL).
Agreed
>
> > + if (np && of_device_is_available(np))
> > + return of_find_device_by_node(np);
>
> of_find_device_by_node() takes a reference to the returned device, you
> need to call device_put() on it when you don't need it anymore.
Agreed & Thanks again.
>
>
> > + return NULL;
> > +}
> > +
> > +/* Proble internal channel */
> > +static int rcar_drif_channel_probe(struct platform_device *pdev)
> > +{
> > + struct rcar_drif *ch;
> > + struct resource *res;
> > + void __iomem *base;
> > + struct clk *clkp;
>
> Maybe s/clkp/clk/ ?
Agreed
>
> > + int ret;
> > +
> > + /* Peripheral clock */
> > + clkp = devm_clk_get(&pdev->dev, "fck");
> > + if (IS_ERR(clkp)) {
> > + ret = PTR_ERR(clkp);
> > + dev_err(&pdev->dev, "clk get failed (%d)\n", ret);
> > + return ret;
> > + }
>
> Isn't the clock managed automatically by runtime PM ?
I think the driver need to support runtime PM in order to manage the clock. Otherwise it just gets adds the clk to genpd (_prepare) without enable/disable. I need to double check this.
>
> > + /* Register map */
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(base)) {
> > + ret = PTR_ERR(base);
> > + dev_err(&pdev->dev, "ioremap failed (%d)\n", ret);
> > + return ret;
>
> devm_ioremap_resource() already prints an error message, you can remove
> this
> one.
Agreed
>
> > + }
> > +
> > + /* Reserve memory for enabled channel */
> > + ch = devm_kzalloc(&pdev->dev, sizeof(*ch), GFP_KERNEL);
> > + if (!ch) {
> > + ret = PTR_ERR(ch);
> > + dev_err(&pdev->dev, "failed alloc channel\n");
>
> Memory allocation failures already print error messages, you can remove
> this
> one.
Agreed.
>
> > + return ret;
> > + }
> > + ch->pdev = pdev;
> > + ch->clkp = clkp;
> > + ch->base = base;
> > + ch->start = res->start;
>
> If you allocated the ch structure first you could set the fields directly
> without a need for local variables.
Agreed.
>
> > + platform_set_drvdata(pdev, ch);
> > + return 0;
> > +}
> > +
> > +static int rcar_drif_probe(struct platform_device *pdev)
> > +{
> > + struct rcar_drif *ch, *b_ch = NULL;
> > + struct platform_device *b_pdev;
> > + struct rcar_drif_sdr *sdr;
> > + int ret;
> > +
> > + /* Probe internal channel */
> > + ret = rcar_drif_channel_probe(pdev);
> > + if (ret)
> > + return ret;
>
> I would have done it the other way around, inlining the
> rcar_drif_channel_probe() function here as that's the common case, and
> moving
> the V4L2 SDR device initialization code to a different function.
Agreed
>
> > + /* Check if both channels of the bond are enabled */
> > + b_pdev = rcar_drif_enabled_bond(pdev);
> > + if (b_pdev) {
> > + /* Check if current channel acting as primary-bond */
> > + if (!rcar_drif_primary_bond(pdev)) {
> > + dev_notice(&pdev->dev, "probed\n");
> > + return 0;
> > + }
> > +
> > + /* Check if the other device is probed */
> > + b_ch = platform_get_drvdata(b_pdev);
> > + if (!b_ch) {
> > + dev_info(&pdev->dev, "defer probe\n");
> > + return -EPROBE_DEFER;
> > + }
>
> Isn't this all very racy ? What if the other channel's device is removed
> while
> this one is probed ?
OK. Will holding the device_lock(&b_pdev->dev) is sufficient?
>
> > + /* Set the other channel number */
> > + b_ch->num = 1;
>
> Reading data from the other channel's private structure is one thing, but
> writing it makes me shiver :-S Could we make it so that 0 is the slave and
> 1
> the master ? That way you would set ch->num = 1 instead of b_ch->num = 1,
> keeping all modifications to the private structure local to the device
> being
> probed.
Agreed.
>
> > + }
> > +
> > + /* Channel acting as SDR instance */
> > + ch = platform_get_drvdata(pdev);
> > + ch->acting_sdr = true;
> > +
> > + /* Reserve memory for SDR structure */
> > + sdr = devm_kzalloc(&pdev->dev, sizeof(*sdr), GFP_KERNEL);
> > + if (!sdr) {
> > + ret = PTR_ERR(sdr);
> > + dev_err(&pdev->dev, "failed alloc drif context\n");
> > + return ret;
> > + }
> > + sdr->dev = &pdev->dev;
> > + sdr->hw_ch_mask = BIT(ch->num);
> > +
> > + /* Establish links between SDR and channel(s) */
> > + ch->sdr = sdr;
> > + sdr->ch[ch->num] = ch;
> > + if (b_ch) {
> > + sdr->ch[b_ch->num] = b_ch;
> > + b_ch->sdr = sdr;
> > + sdr->hw_ch_mask |= BIT(b_ch->num);
> > + }
> > + sdr->num_hw_ch = hweight_long(sdr->hw_ch_mask);
> > +
> > + /* Validate any supported format for enabled channels */
> > + ret = rcar_drif_set_default_format(sdr);
> > + if (ret) {
> > + dev_err(sdr->dev, "failed to set default format\n");
> > + return ret;
> > + }
> > +
> > + /* Set defaults */
> > + sdr->hwbuf_size = RCAR_DRIF_DEFAULT_HWBUF_SIZE;
> > +
> > + mutex_init(&sdr->v4l2_mutex);
> > + mutex_init(&sdr->vb_queue_mutex);
> > + spin_lock_init(&sdr->queued_bufs_lock);
> > + INIT_LIST_HEAD(&sdr->queued_bufs);
> > +
> > + /* Init videobuf2 queue structure */
> > + sdr->vb_queue.type = V4L2_BUF_TYPE_SDR_CAPTURE;
> > + sdr->vb_queue.io_modes = VB2_READ | VB2_MMAP | VB2_DMABUF;
> > + sdr->vb_queue.drv_priv = sdr;
> > + sdr->vb_queue.buf_struct_size = sizeof(struct rcar_drif_frame_buf);
> > + sdr->vb_queue.ops = &rcar_drif_vb2_ops;
> > + sdr->vb_queue.mem_ops = &vb2_vmalloc_memops;
> > + sdr->vb_queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > +
> > + /* Init videobuf2 queue */
> > + ret = vb2_queue_init(&sdr->vb_queue);
> > + if (ret) {
> > + dev_err(sdr->dev, "could not initialize vb2 queue\n");
> > + return ret;
> > + }
> > +
> > + /* Register the v4l2_device */
> > + ret = v4l2_device_register(&pdev->dev, &sdr->v4l2_dev);
> > + if (ret) {
> > + dev_err(sdr->dev, "failed v4l2_device_register (%d)\n", ret);
>
> Maybe "failed to register V4L2 device" to make it a real sentence ? :-)
Agreed
>
> > + return ret;
> > + }
> > +
> > + /*
> > + * Parse subdevs after v4l2_device_register because if the subdev
> > + * is already probed, bound and complete will be called immediately
> > + */
> > + ret = rcar_drif_parse_subdevs(sdr, &pdev->dev);
> > + if (ret)
> > + goto err_unreg_v4l2;
> > +
> > + sdr->notifier.bound = rcar_drif_notify_bound;
> > + sdr->notifier.complete = rcar_drif_notify_complete;
> > +
> > + v4l2_ctrl_handler_init(&sdr->ctrl_hdl, 10);
>
> Possibly a stupid question, why 10, if you don't create any control in
> this
> driver ?
To accommodate the subdev controls.
>
> > + /* Register notifier */
> > + ret = v4l2_async_notifier_register(&sdr->v4l2_dev, &sdr->notifier);
> > + if (ret < 0) {
> > + dev_err(sdr->dev, "notifier registration failed (%d)\n", ret);
> > + goto err_free_ctrls;
> > + }
> > +
> > + /* Init video_device structure */
> > + sdr->vdev = video_device_alloc();
> > + if (!sdr->vdev) {
> > + ret = -ENOMEM;
> > + goto err_unreg_notif;
> > + }
> > + snprintf(sdr->vdev->name, sizeof(sdr->vdev->name), "R-Car DRIF");
> > + sdr->vdev->fops = &rcar_drif_fops;
> > + sdr->vdev->ioctl_ops = &rcar_drif_ioctl_ops;
> > + sdr->vdev->release = video_device_release;
> > + sdr->vdev->lock = &sdr->v4l2_mutex;
> > + sdr->vdev->queue = &sdr->vb_queue;
> > + sdr->vdev->queue->lock = &sdr->vb_queue_mutex;
> > + sdr->vdev->ctrl_handler = &sdr->ctrl_hdl;
> > + sdr->vdev->v4l2_dev = &sdr->v4l2_dev;
> > + sdr->vdev->device_caps = V4L2_CAP_SDR_CAPTURE | V4L2_CAP_TUNER |
> > + V4L2_CAP_STREAMING | V4L2_CAP_READWRITE;
> > + video_set_drvdata(sdr->vdev, sdr);
> > +
> > + /* Register V4L2 SDR device */
> > + ret = video_register_device(sdr->vdev, VFL_TYPE_SDR, -1);
> > + if (ret) {
> > + dev_err(sdr->dev, "failed video_register_device (%d)\n", ret);
>
> Same here, "failed to register video device" ?
Agreed.
>
> > + goto err_unreg_notif;
> > + }
> > +
> > + dev_notice(sdr->dev, "probed\n");
>
> Do you think this message is really useful ? I believe it would just add a
> bit
> more noise to the kernel log, without any real use.
OK. Will remove it.
>
> > + return 0;
> > +
> > +err_unreg_notif:
> > + video_device_release(sdr->vdev);
> > + v4l2_async_notifier_unregister(&sdr->notifier);
> > +err_free_ctrls:
> > + v4l2_ctrl_handler_free(&sdr->ctrl_hdl);
> > +err_unreg_v4l2:
> > + v4l2_device_unregister(&sdr->v4l2_dev);
> > + return ret;
> > +}
> > +
> > +static int rcar_drif_remove(struct platform_device *pdev)
> > +{
> > + struct rcar_drif *ch = platform_get_drvdata(pdev);
> > + struct rcar_drif_sdr *sdr = ch->sdr;
> > +
> > + if (!ch->acting_sdr) {
>
> Isn't it possible to check the channel number instead and remove the
> acting_sdr field ?
Agreed.
>
> > + /* Nothing to do */
> > + dev_notice(&pdev->dev, "removed\n");
> > + return 0;
> > + }
> > +
> > + /* SDR instance */
> > + v4l2_ctrl_handler_free(sdr->vdev->ctrl_handler);
> > + v4l2_async_notifier_unregister(&sdr->notifier);
> > + v4l2_device_unregister(&sdr->v4l2_dev);
> > + video_unregister_device(sdr->vdev);
> > + dev_notice(&pdev->dev, "removed\n");
>
> Even more than the probed message, I think this one can go away.
Agreed.
>
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused rcar_drif_suspend(struct device *dev)
> > +{
> > + return 0;
>
> Maybe a /* FIXME: Implement suspend/resume support */ ?
Agreed.
>
> > +}
> > +
> > +static int __maybe_unused rcar_drif_resume(struct device *dev)
> > +{
> > + return 0;
>
> Same here ?
Agreed.
Thanks,
Ramesh
^ permalink raw reply
* Re: [PATCH] of: introduce event tracepoints for dynamic device_node lifecyle
From: Rob Herring @ 2017-04-18 16:46 UTC (permalink / raw)
To: Tyrel Datwyler
Cc: linuxppc-dev,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Nathan Fontenot, Michael Ellerman, Frank Rowand,
rostedt-nx8X9YLhiw1AfugRpC6u6w, Ingo Molnar
In-Reply-To: <1492475525-10827-1-git-send-email-tyreld-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
On Mon, Apr 17, 2017 at 7:32 PM, Tyrel Datwyler
<tyreld-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> This patch introduces event tracepoints for tracking a device_nodes
> reference cycle as well as reconfig notifications generated in response
> to node/property manipulations.
>
> With the recent upstreaming of the refcount API several device_node
> underflows and leaks have come to my attention in the pseries (DLPAR) dynamic
> logical partitioning code (ie. POWER speak for hotplugging virtual and physcial
> resources at runtime such as cpus or IOAs). These tracepoints provide a
> easy and quick mechanism for validating the reference counting of
> device_nodes during their lifetime.
Not really relevant for this patch, but since you are looking at
pseries and refcounting, the refcounting largely exists for pseries.
It's also hard to get right as this type of fix is fairly common. It's
now used for overlays, but we really probably only need to refcount
the overlays or changesets as a whole, not at a node level. If you
have any thoughts on how a different model of refcounting could work
for pseries, I'd like to discuss it.
> Further, when pseries lpars are migrated to a different machine we
> perform a live update of our device tree to bring it into alignment with the
> configuration of the new machine. The of_reconfig_notify trace point
> provides a mechanism that can be turned for debuging the device tree
> modifications with out having to build a custom kernel to get at the
> DEBUG code introduced by commit 00aa3720.
>
> The following trace events are provided: of_node_get, of_node_put,
> of_node_release, and of_reconfig_notify. These trace points require a kernel
> built with ftrace support to be enabled. In a typical environment where
> debugfs is mounted at /sys/kernel/debug the entire set of tracepoints
> can be set with the following:
>
> echo "of:*" > /sys/kernel/debug/tracing/set_event
>
> or
>
> echo 1 > /sys/kernel/debug/tracing/of/enable
>
> The following shows the trace point data from a DLPAR remove of a cpu
> from a pseries lpar:
>
> cat /sys/kernel/debug/tracing/trace | grep "POWER8@10"
>
> cpuhp/23-147 [023] .... 128.324827:
> of_node_put: refcount=5, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147 [023] .... 128.324829:
> of_node_put: refcount=4, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147 [023] .... 128.324829:
> of_node_put: refcount=3, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147 [023] .... 128.324831:
> of_node_put: refcount=2, dn->full_name=/cpus/PowerPC,POWER8@10
> drmgr-7284 [009] .... 128.439000:
> of_node_put: refcount=1, dn->full_name=/cpus/PowerPC,POWER8@10
> drmgr-7284 [009] .... 128.439002:
> of_reconfig_notify: action=DETACH_NODE, dn->full_name=/cpus/PowerPC,POWER8@10,
> prop->name=null, old_prop->name=null
> drmgr-7284 [009] .... 128.439015:
> of_node_put: refcount=0, dn->full_name=/cpus/PowerPC,POWER8@10
> drmgr-7284 [009] .... 128.439016:
> of_node_release: dn->full_name=/cpus/PowerPC,POWER8@10, dn->_flags=4
>
> Signed-off-by: Tyrel Datwyler <tyreld-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
> drivers/of/dynamic.c | 30 ++++++---------
> include/trace/events/of.h | 93 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 105 insertions(+), 18 deletions(-)
> create mode 100644 include/trace/events/of.h
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 888fdbc..85c0966 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -16,6 +16,9 @@
>
> #include "of_private.h"
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/of.h>
> +
> /**
> * of_node_get() - Increment refcount of a node
> * @node: Node to inc refcount, NULL is supported to simplify writing of
> @@ -25,8 +28,10 @@
> */
> struct device_node *of_node_get(struct device_node *node)
> {
> - if (node)
> + if (node) {
> kobject_get(&node->kobj);
> + trace_of_node_get(refcount_read(&node->kobj.kref.refcount), node->full_name);
Seems like there should be a kobj wrapper to read the refcount.
> + }
> return node;
> }
> EXPORT_SYMBOL(of_node_get);
> @@ -38,8 +43,10 @@ struct device_node *of_node_get(struct device_node *node)
> */
> void of_node_put(struct device_node *node)
> {
> - if (node)
> + if (node) {
> + trace_of_node_put(refcount_read(&node->kobj.kref.refcount) - 1, node->full_name);
> kobject_put(&node->kobj);
> + }
> }
> EXPORT_SYMBOL(of_node_put);
>
> @@ -92,24 +99,9 @@ int of_reconfig_notifier_unregister(struct notifier_block *nb)
> int of_reconfig_notify(unsigned long action, struct of_reconfig_data *p)
> {
> int rc;
> -#ifdef DEBUG
> - struct of_reconfig_data *pr = p;
>
> - switch (action) {
> - case OF_RECONFIG_ATTACH_NODE:
> - case OF_RECONFIG_DETACH_NODE:
> - pr_debug("notify %-15s %s\n", action_names[action],
> - pr->dn->full_name);
> - break;
> - case OF_RECONFIG_ADD_PROPERTY:
> - case OF_RECONFIG_REMOVE_PROPERTY:
> - case OF_RECONFIG_UPDATE_PROPERTY:
> - pr_debug("notify %-15s %s:%s\n", action_names[action],
> - pr->dn->full_name, pr->prop->name);
> - break;
> + trace_of_reconfig_notify(action, p);
>
> - }
> -#endif
> rc = blocking_notifier_call_chain(&of_reconfig_chain, action, p);
> return notifier_to_errno(rc);
> }
> @@ -326,6 +318,8 @@ void of_node_release(struct kobject *kobj)
> struct device_node *node = kobj_to_device_node(kobj);
> struct property *prop = node->properties;
>
> + trace_of_node_release(node);
> +
> /* We should never be releasing nodes that haven't been detached. */
> if (!of_node_check_flag(node, OF_DETACHED)) {
> pr_err("ERROR: Bad of_node_put() on %s\n", node->full_name);
> diff --git a/include/trace/events/of.h b/include/trace/events/of.h
> new file mode 100644
> index 0000000..0d53271
> --- /dev/null
> +++ b/include/trace/events/of.h
> @@ -0,0 +1,93 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM of
> +
> +#if !defined(_TRACE_OF_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_OF_H
> +
> +#include <linux/of.h>
> +#include <linux/tracepoint.h>
> +
> +DECLARE_EVENT_CLASS(of_node_ref_template,
> +
> + TP_PROTO(int refcount, const char* dn_name),
> +
> + TP_ARGS(refcount, dn_name),
> +
> + TP_STRUCT__entry(
> + __string(dn_name, dn_name)
> + __field(int, refcount)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(dn_name, dn_name);
> + __entry->refcount = refcount;
> + ),
> +
> + TP_printk("refcount=%d, dn->full_name=%s",
> + __entry->refcount, __get_str(dn_name))
> +);
> +
> +DEFINE_EVENT(of_node_ref_template, of_node_get,
> + TP_PROTO(int refcount, const char* dn_name),
> + TP_ARGS(refcount, dn_name));
> +
> +DEFINE_EVENT(of_node_ref_template, of_node_put,
> + TP_PROTO(int refcount, const char* dn_name),
> + TP_ARGS(refcount, dn_name));
> +
> +TRACE_EVENT(of_node_release,
> +
> + TP_PROTO(struct device_node *dn),
> +
> + TP_ARGS(dn),
> +
> + TP_STRUCT__entry(
> + __string(dn_name, dn->full_name)
> + __field(unsigned long, flags)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(dn_name, dn->full_name);
> + __entry->flags = dn->_flags;
> + ),
> +
> + TP_printk("dn->full_name=%s, dn->_flags=%lu",
> + __get_str(dn_name), __entry->flags)
> +);
> +
> +#define of_reconfig_action_names \
> + {OF_RECONFIG_ATTACH_NODE, "ATTACH_NODE"}, \
> + {OF_RECONFIG_DETACH_NODE, "DETACH_NODE"}, \
> + {OF_RECONFIG_ADD_PROPERTY, "ADD_PROPERTY"}, \
> + {OF_RECONFIG_REMOVE_PROPERTY, "REMOVE_PROPERTY"}, \
> + {OF_RECONFIG_UPDATE_PROPERTY, "UPDATE_PROPERTY"}
> +
> +TRACE_EVENT(of_reconfig_notify,
> +
> + TP_PROTO(unsigned long action, struct of_reconfig_data *ord),
> +
> + TP_ARGS(action, ord),
> +
> + TP_STRUCT__entry(
> + __field(unsigned long, action)
> + __string(dn_name, ord->dn->full_name)
> + __string(prop_name, ord->prop ? ord->prop->name : "null")
> + __string(oldprop_name, ord->old_prop ? ord->old_prop->name : "null")
> + ),
> +
> + TP_fast_assign(
> + __entry->action = action;
> + __assign_str(dn_name, ord->dn->full_name);
> + __assign_str(prop_name, ord->prop ? ord->prop->name : "null");
> + __assign_str(oldprop_name, ord->old_prop ? ord->old_prop->name : "null");
> + ),
> +
> + TP_printk("action=%s, dn->full_name=%s, prop->name=%s, old_prop->name=%s",
> + __print_symbolic(__entry->action, of_reconfig_action_names),
> + __get_str(dn_name), __get_str(prop_name), __get_str(oldprop_name))
> +);
> +
> +#endif /* _TRACE_OF_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> --
> 1.8.3.1
>
--
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] of: fix "/cpus" reference leak in of_numa_parse_cpu_nodes()
From: Rob Herring @ 2017-04-18 16:08 UTC (permalink / raw)
To: David Daney, Tyrel Datwyler
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Frank Rowand,
David Daney, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1005a5c3-22d1-b082-0383-d8f74e711315-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
On Tue, Apr 18, 2017 at 10:16 AM, David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org> wrote:
> On 04/17/2017 05:29 PM, Tyrel Datwyler wrote:
>>
>> The call to of_find_node_by_path("/cpus") returns the cpus device_node
>> with its reference count incremented. There is no matching of_node_put()
>> call in of_numa_parse_cpu_nodes() which results in a leaked reference
>> to the "/cpus" node.
>>
>> This patch adds an of_node_put() to release the reference.
>
>
> Good catch:
> Acked-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
Applied, thanks.
Rob
--
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 V4 1/9] PM / OPP: Allow OPP table to be used for power-domains
From: Sudeep Holla @ 2017-04-18 16:03 UTC (permalink / raw)
To: Viresh Kumar
Cc: Sudeep Holla, Rafael Wysocki, ulf.hansson-QSEj5FYQhm4dnm+yROfE0A,
Kevin Hilman, Viresh Kumar, Nishanth Menon, Stephen Boyd,
linaro-kernel-cunTk1MwBs8s++Sfvej+rw,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Vincent Guittot,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, lina.iyer-QSEj5FYQhm4dnm+yROfE0A,
rnayak-sgV2jX0FEOL9JmXXK+q4OQ, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170417053303.GG28191@vireshk-i7>
On 17/04/17 06:33, Viresh Kumar wrote:
> On 13-04-17, 14:43, Sudeep Holla wrote:
>> Interesting. My understand of power domain and in particular power
>> domain performance was that it would control both. The abstract number
>> you introduce would hide clocks and regulators.
>>
>> But if the concept treats it just as yet another regulator, we do we
>> need these at all. Why don't we relate this performance to regulator
>> values and be done with it ?
>>
>> Sorry if I am missing to understand something here. I would look this as
>> replacement for both clocks and regulators, something similar to ACPI
>> CPPC. If not, it looks unnecessary to me with the information I have got
>> so far.
>
> I kind of answered that in the other email.
>
> Some background may be good here. So Qcom tried to solve all this with virtual
> regulators, but the problem was that they need to talk in terms of integer
> values (1, 2, 3..) and not voltages and so they can't use the regulator
> framework straight away. And so we are doing all this.
>
Was it posted externally ? Was there any objections for that approach ?
IMO that's better approach but if I am late to the party, I would like
to read through the discussions that happened on it(if any)
--
Regards,
Sudeep
--
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 V4 1/9] PM / OPP: Allow OPP table to be used for power-domains
From: Sudeep Holla @ 2017-04-18 16:01 UTC (permalink / raw)
To: Viresh Kumar
Cc: Sudeep Holla, Rafael Wysocki, ulf.hansson, Kevin Hilman,
Viresh Kumar, Nishanth Menon, Stephen Boyd, linaro-kernel,
linux-pm, linux-kernel, Vincent Guittot, robh+dt, lina.iyer,
rnayak, devicetree
In-Reply-To: <20170417052758.GF28191@vireshk-i7>
On 17/04/17 06:27, Viresh Kumar wrote:
> On 13-04-17, 14:42, Sudeep Holla wrote:
>> What I was referring is about power domain provider with multiple power
>> domains(simply #power-domain-cells=<1> case as explained in the
>> power-domain specification.
>
> I am not sure if we should be looking to target such a situation for now, as
> that would be like this:
>
> Device controlled by Domain A. Domain A itself is controlled by Domain B and
> Domain C.
>
No, may be I was not so clear. I am just referring a power controller
that provides say 3 different power domains and are indexed 0 - 2.
The consumer just passes the index along with the phandle for the power
domain info.
> Though we will end up converting the domain-performance-state property to an
> array if that is required in near future.
>
OK, better to document that so that we know how to extend it. We have
#power-domain-cells=<1> on Juno with SCPI.
>> Yes. To simplify what not we just have power-domain for a device and
>> change state of that domain to change the performance of that device.
>
> Consider this case to understand what I have in Mind.
>
> The power domain have its states as A, B, C, D. There can be multiple devices
> regulated by that domain and one of the devices have its power states as: A1,
> A2, A3, B1, B2, B3, C1, C2, C3, D1, D2, D3 and all these states have different
> frequency/voltages.
>
> IOW, the devices can have regulators as well and may want to fine tune within
> the domain performance-state.
>
Understood. I would incline towards reusing regulators we that's what is
changed behind the scene. Calling this operating performance point
is misleading and doesn't align well with existing specs/features.
>> Then put this in the hierarchy. Some thing similar to what we already
>> have with new domain-idle states. In that way, we can move any
>> performance control to the domain and abstract the clocks and regulators
>> from the devices as the first step and from the OSPM view if there's
>> firmware support.
>>
>> If we are looking this power-domains with performance as just some
>> *advanced regulators*, I don't like the complexity added.
>
> In the particular case I am trying to solve (Qcom), we have some sort of
> regulators which are only programmed by a M3 core. The M3 core needs integer
> numbers representing state we want the domain to be in and it will put the
> regulators (or whatever) in a particular state.
>
Understood. We have exactly same thing with SCPI but it controls both
frequency and voltage referred as operating points. In general, this OPP
terminology is used in SCPI/ACPI/SCMI specifications as both frequency
and voltage control. I am bit worried that this binding might introduce
confusions on the definitions. But it can be reworded/renamed easily if
required.
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH 0/4] staging: add ccree crypto driver
From: Mark Rutland @ 2017-04-18 15:43 UTC (permalink / raw)
To: Gilad Ben-Yossef
Cc: Herbert Xu, David S. Miller, Rob Herring, Greg Kroah-Hartman,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux kernel mailing list,
Gilad Ben-Yossef, Binoy Jayan, Ofir Drang
In-Reply-To: <CAOtvUMeB8ub-e6PXxgJWyK=TW4HseZgSN6NO=kSmVax514ufdg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Apr 18, 2017 at 06:29:22PM +0300, Gilad Ben-Yossef wrote:
> On Tue, Apr 18, 2017 at 6:13 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Tue, Apr 18, 2017 at 05:07:50PM +0300, Gilad Ben-Yossef wrote:
> >> Arm TrustZone CryptoCell 700 is a family of cryptographic hardware
> >> accelerators. It is supported by a long lived series of out of tree
> >> drivers, which I am now in the process of unifying and upstreaming.
> >> This is the first drop, supporting the new CryptoCell 712 REE.
> >>
> >> The code still needs some cleanup before maturing to a proper
> >> upstream driver, which I am in the process of doing. However,
> >> as discussion of some of the capabilities of the hardware and
> >> its application to some dm-crypt and dm-verity features recently
> >> took place I though it is better to do this in the open via the
> >> staging tree.
> >>
> >> A Git repository based off of Linux 4.11-rc7 is available at
> >> https://github.com/gby/linux.git branch ccree.
> >>
> >> Signed-off-by: Gilad Ben-Yossef <gilad-6S/DczAoZh3WXxRugSxzZg@public.gmane.org>
> >> CC: Binoy Jayan <binoy.jayan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> CC: Ofir Drang <ofir.drang-5wv7dgnIgG8@public.gmane.org>
> >>
> >> Gilad Ben-Yossef (4):
> >> staging: add ccree crypto driver
> >> staging: ccree: add TODO list
> >> staging: ccree: add devicetree bindings
> >> MAINTAINERS: add Gilad BY as maintainer for ccree
> >>
> >> .../devicetree/bindings/crypto/arm-cryptocell.txt | 23 +
> >
> > I'm interested in looking at the DT binding, but for some reason I've
> > only recevied the cover letter so far.
> >
> > Are my mail servers being particularly slow today, or has something gone
> > wrong with the Cc list for the remaining patches?
> >
>
> Thanks a bunch for the willingness to review this.
>
> My apologies for not communicating this clearly enough - Since it is
> an pre-existing driver it is rather big.
> I did not want to inflict a 600K patch on the mailing list so opted to
> provide a git repo instead, as stated in the
> email.
Should this have been a [GIT PULL], then?
I was confused by the [PATCH 0/4].
> The patch in question is here:
> https://github.com/gby/linux/commit/12d92e0bc19aa9bbd891cdab765eaaeabe0b6967
>
> Do let me know if you prefer that I will send at least the smaller
> patch file via email in the normal fashion.
Sending patches is the usual way to have things reviewed. Linking to an
external site doesn't fit with the workflows of everyone.
If you wish to have patches reviewed, please send them as patches in the
usual fashion.
I will note that on the patch you linked, the compatible string is far
too vague, per common conventions. Per your description above, that
should be something like "arm,cryptocell-712-ree" (and each variant
should have its own string).
I don't have documentation to hand to attempt to review the rest.
I would appreciate if you could ensure that the DT binding was reviewed
as part of the staging TODOs. That needs to follow the usual DT review
process of sending patches to the list. Until that has occurred, it
shouldn't be in Documentation/devicetree/.
Thanks,
Mark.
--
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 0/4] staging: add ccree crypto driver
From: Greg Kroah-Hartman @ 2017-04-18 15:39 UTC (permalink / raw)
To: Gilad Ben-Yossef
Cc: Herbert Xu, David S. Miller, Rob Herring, Mark Rutland,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, gilad.benyossef-5wv7dgnIgG8,
Binoy Jayan, Ofir Drang
In-Reply-To: <1492524470-13847-1-git-send-email-gilad-6S/DczAoZh3WXxRugSxzZg@public.gmane.org>
On Tue, Apr 18, 2017 at 05:07:50PM +0300, Gilad Ben-Yossef wrote:
> Arm TrustZone CryptoCell 700 is a family of cryptographic hardware
> accelerators. It is supported by a long lived series of out of tree
> drivers, which I am now in the process of unifying and upstreaming.
> This is the first drop, supporting the new CryptoCell 712 REE.
>
> The code still needs some cleanup before maturing to a proper
> upstream driver, which I am in the process of doing. However,
> as discussion of some of the capabilities of the hardware and
> its application to some dm-crypt and dm-verity features recently
> took place I though it is better to do this in the open via the
> staging tree.
>
> A Git repository based off of Linux 4.11-rc7 is available at
> https://github.com/gby/linux.git branch ccree.
>
> Signed-off-by: Gilad Ben-Yossef <gilad-6S/DczAoZh3WXxRugSxzZg@public.gmane.org>
> CC: Binoy Jayan <binoy.jayan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> CC: Ofir Drang <ofir.drang-5wv7dgnIgG8@public.gmane.org>
>
> Gilad Ben-Yossef (4):
> staging: add ccree crypto driver
> staging: ccree: add TODO list
> staging: ccree: add devicetree bindings
> MAINTAINERS: add Gilad BY as maintainer for ccree
We can't do much without a real patch, sorry. Digging in random git
trees doesn't work :(
I can't take this as-is, I need patches.
thanks,
greg k-h
--
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 0/4] staging: add ccree crypto driver
From: Gilad Ben-Yossef @ 2017-04-18 15:29 UTC (permalink / raw)
To: Mark Rutland
Cc: Herbert Xu, David S. Miller, Rob Herring, Greg Kroah-Hartman,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux kernel mailing list,
Gilad Ben-Yossef, Binoy Jayan, Ofir Drang
In-Reply-To: <20170418151355.GI17866@leverpostej>
Hi Mark,
On Tue, Apr 18, 2017 at 6:13 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi,
>
> On Tue, Apr 18, 2017 at 05:07:50PM +0300, Gilad Ben-Yossef wrote:
>> Arm TrustZone CryptoCell 700 is a family of cryptographic hardware
>> accelerators. It is supported by a long lived series of out of tree
>> drivers, which I am now in the process of unifying and upstreaming.
>> This is the first drop, supporting the new CryptoCell 712 REE.
>>
>> The code still needs some cleanup before maturing to a proper
>> upstream driver, which I am in the process of doing. However,
>> as discussion of some of the capabilities of the hardware and
>> its application to some dm-crypt and dm-verity features recently
>> took place I though it is better to do this in the open via the
>> staging tree.
>>
>> A Git repository based off of Linux 4.11-rc7 is available at
>> https://github.com/gby/linux.git branch ccree.
>>
>> Signed-off-by: Gilad Ben-Yossef <gilad-6S/DczAoZh3WXxRugSxzZg@public.gmane.org>
>> CC: Binoy Jayan <binoy.jayan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> CC: Ofir Drang <ofir.drang-5wv7dgnIgG8@public.gmane.org>
>>
>> Gilad Ben-Yossef (4):
>> staging: add ccree crypto driver
>> staging: ccree: add TODO list
>> staging: ccree: add devicetree bindings
>> MAINTAINERS: add Gilad BY as maintainer for ccree
>>
>> .../devicetree/bindings/crypto/arm-cryptocell.txt | 23 +
>
> I'm interested in looking at the DT binding, but for some reason I've
> only recevied the cover letter so far.
>
> Are my mail servers being particularly slow today, or has something gone
> wrong with the Cc list for the remaining patches?
>
Thanks a bunch for the willingness to review this.
My apologies for not communicating this clearly enough - Since it is
an pre-existing driver it is rather big.
I did not want to inflict a 600K patch on the mailing list so opted to
provide a git repo instead, as stated in the
email.
The patch in question is here:
https://github.com/gby/linux/commit/12d92e0bc19aa9bbd891cdab765eaaeabe0b6967
Do let me know if you prefer that I will send at least the smaller
patch file via email in the normal fashion.
Many thanks,
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
-- Jean-Baptiste Queru
--
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 v6 5/8] coresight: use const for device_node structures
From: Mathieu Poirier @ 2017-04-18 15:24 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-soc-u79uwXL29TY76Z2rM5mHXA, Mike Leach, Sudeep Holla
In-Reply-To: <1491485461-22800-6-git-send-email-leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On Thu, Apr 06, 2017 at 09:30:58PM +0800, Leo Yan wrote:
> Almost low level functions from open firmware have used const to
> qualify device_node structures, so add const for device_node
> parameters in of_coresight related functions.
>
> Reviewed-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
I agree with these changes but the patch needs to be split up - please see
below.
> ---
> drivers/hwtracing/coresight/of_coresight.c | 6 +++---
> include/linux/coresight.h | 8 ++++----
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
> index 78d2399..46eec0f 100644
> --- a/drivers/hwtracing/coresight/of_coresight.c
> +++ b/drivers/hwtracing/coresight/of_coresight.c
> @@ -52,7 +52,7 @@ of_coresight_get_endpoint_device(struct device_node *endpoint)
> endpoint, of_dev_node_match);
> }
>
> -static void of_coresight_get_ports(struct device_node *node,
> +static void of_coresight_get_ports(const struct device_node *node,
> int *nr_inport, int *nr_outport)
Move this to a patch by itself as it is not related to this driver.
> {
> struct device_node *ep = NULL;
> @@ -101,7 +101,7 @@ static int of_coresight_alloc_memory(struct device *dev,
> return 0;
> }
>
> -int of_coresight_get_cpu(struct device_node *node)
> +int of_coresight_get_cpu(const struct device_node *node)
Move this to the previous patch in this set. There is not need to undo what you
just did there.
> {
> int cpu;
> bool found;
> @@ -128,7 +128,7 @@ int of_coresight_get_cpu(struct device_node *node)
> EXPORT_SYMBOL_GPL(of_coresight_get_cpu);
>
> struct coresight_platform_data *of_get_coresight_platform_data(
> - struct device *dev, struct device_node *node)
> + struct device *dev, const struct device_node *node)
Same here, move this to a new patch (the same one as of_coresight_get_ports()).
> {
> int i = 0, ret = 0;
> struct coresight_platform_data *pdata;
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index bf96678..4915254 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -263,13 +263,13 @@ static inline int coresight_timeout(void __iomem *addr, u32 offset,
> #endif
>
> #ifdef CONFIG_OF
> -extern int of_coresight_get_cpu(struct device_node *node);
> +extern int of_coresight_get_cpu(const struct device_node *node);
> extern struct coresight_platform_data *of_get_coresight_platform_data(
> - struct device *dev, struct device_node *node);
> + struct device *dev, const struct device_node *node);
> #else
> -static inline int of_coresight_get_cpu(struct device_node *node) { return 0; }
> +static inline int of_coresight_get_cpu(const struct device_node *node) { return 0; }
> static inline struct coresight_platform_data *of_get_coresight_platform_data(
> - struct device *dev, struct device_node *node) { return NULL; }
> + struct device *dev, const struct device_node *node) { return NULL; }
> #endif
>
> #ifdef CONFIG_PID_NS
> --
> 2.7.4
>
--
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] of: fix "/cpus" reference leak in of_numa_parse_cpu_nodes()
From: David Daney @ 2017-04-18 15:16 UTC (permalink / raw)
To: Tyrel Datwyler, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
frowand.list-Re5JQEeQqe8AvxtiuMwx3w,
david.daney-YGCgFSpz5w/QT0dZR+AlfA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1492475357-10784-1-git-send-email-tyreld-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
On 04/17/2017 05:29 PM, Tyrel Datwyler wrote:
> The call to of_find_node_by_path("/cpus") returns the cpus device_node
> with its reference count incremented. There is no matching of_node_put()
> call in of_numa_parse_cpu_nodes() which results in a leaked reference
> to the "/cpus" node.
>
> This patch adds an of_node_put() to release the reference.
Good catch:
Acked-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>
> fixes: 298535c00a2c ("of, numa: Add NUMA of binding implementation.")
> Signed-off-by: Tyrel Datwyler <tyreld-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
> drivers/of/of_numa.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/of/of_numa.c b/drivers/of/of_numa.c
> index a53982a..2db1f7a 100644
> --- a/drivers/of/of_numa.c
> +++ b/drivers/of/of_numa.c
> @@ -57,6 +57,8 @@ static void __init of_numa_parse_cpu_nodes(void)
> else
> node_set(nid, numa_nodes_parsed);
> }
> +
> + of_node_put(cpus);
> }
>
> static int __init of_numa_parse_memory_nodes(void)
>
--
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 0/4] staging: add ccree crypto driver
From: Mark Rutland @ 2017-04-18 15:13 UTC (permalink / raw)
To: Gilad Ben-Yossef
Cc: Herbert Xu, David S. Miller, Rob Herring, Greg Kroah-Hartman,
devel, linux-crypto, devicetree, linux-kernel, gilad.benyossef,
Binoy Jayan, Ofir Drang
In-Reply-To: <1492524470-13847-1-git-send-email-gilad@benyossef.com>
Hi,
On Tue, Apr 18, 2017 at 05:07:50PM +0300, Gilad Ben-Yossef wrote:
> Arm TrustZone CryptoCell 700 is a family of cryptographic hardware
> accelerators. It is supported by a long lived series of out of tree
> drivers, which I am now in the process of unifying and upstreaming.
> This is the first drop, supporting the new CryptoCell 712 REE.
>
> The code still needs some cleanup before maturing to a proper
> upstream driver, which I am in the process of doing. However,
> as discussion of some of the capabilities of the hardware and
> its application to some dm-crypt and dm-verity features recently
> took place I though it is better to do this in the open via the
> staging tree.
>
> A Git repository based off of Linux 4.11-rc7 is available at
> https://github.com/gby/linux.git branch ccree.
>
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> CC: Binoy Jayan <binoy.jayan@linaro.org>
> CC: Ofir Drang <ofir.drang@arm.com>
>
> Gilad Ben-Yossef (4):
> staging: add ccree crypto driver
> staging: ccree: add TODO list
> staging: ccree: add devicetree bindings
> MAINTAINERS: add Gilad BY as maintainer for ccree
>
> .../devicetree/bindings/crypto/arm-cryptocell.txt | 23 +
I'm interested in looking at the DT binding, but for some reason I've
only recevied the cover letter so far.
Are my mail servers being particularly slow today, or has something gone
wrong with the Cc list for the remaining patches?
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v6 3/8] coresight: of_get_coresight_platform_data: Add missing of_node_put
From: Mathieu Poirier @ 2017-04-18 15:09 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc, linux-kernel,
devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
In-Reply-To: <1491485461-22800-4-git-send-email-leo.yan@linaro.org>
On Thu, Apr 06, 2017 at 09:30:56PM +0800, Leo Yan wrote:
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
>
> The of_get_coresight_platform_data iterates over the possible CPU nodes
> to find a given cpu phandle. However it does not drop the reference
> to the node pointer returned by the of_get_coresight_platform_data.
>
> This patch also introduces another minor fix is to use
> of_cpu_device_node_get() to replace of_get_cpu_node().
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> [Leo: minor tweaks for of_get_coresight_platform_data]
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
Suzuki sent a Reviewed-by for this, it should be added here.
> ---
> drivers/hwtracing/coresight/of_coresight.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
> index 629e031..1a77280 100644
> --- a/drivers/hwtracing/coresight/of_coresight.c
> +++ b/drivers/hwtracing/coresight/of_coresight.c
> @@ -108,7 +108,8 @@ struct coresight_platform_data *of_get_coresight_platform_data(
> struct coresight_platform_data *pdata;
> struct of_endpoint endpoint, rendpoint;
> struct device *rdev;
> - struct device_node *dn;
> + bool found;
> + struct device_node *dn, *np;
> struct device_node *ep = NULL;
> struct device_node *rparent = NULL;
> struct device_node *rport = NULL;
> @@ -175,17 +176,19 @@ struct coresight_platform_data *of_get_coresight_platform_data(
> } while (ep);
> }
>
> - /* Affinity defaults to CPU0 */
> - pdata->cpu = 0;
> dn = of_parse_phandle(node, "cpu", 0);
> - for (cpu = 0; dn && cpu < nr_cpu_ids; cpu++) {
> - if (dn == of_get_cpu_node(cpu, NULL)) {
> - pdata->cpu = cpu;
> + for_each_possible_cpu(cpu) {
> + np = of_cpu_device_node_get(cpu);
> + found = (dn == np);
> + of_node_put(np);
> + if (found)
> break;
> - }
> }
> of_node_put(dn);
>
> + /* Affinity to CPU0 if no cpu nodes are found */
> + pdata->cpu = found ? cpu : 0;
> +
> return pdata;
> }
> EXPORT_SYMBOL_GPL(of_get_coresight_platform_data);
> --
> 2.7.4
>
^ 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