[PATCH] media: staging: max96712: Add basic support for MAX96712 GMSL2 deserializer
Add basic support for Maxim MAX96712 quad GMSL2 deserializers. The
driver is capable of powering on the device and configuring the MIPI
CSI-2 bus in a DPHY 4-lane configuration as well as operating the
internal VTG (Video Timing Generator) and VPG (Video Pattern Generator).
Using these features the driver is able to act as a 1080p @ 30 fps V4L2
video source. Producing either a checkerboard or gradient pattern on the
CSI-2 bus, selectable thru a V4L2 control.
While the driver is useful as-is and have been used to prove the correct
operation of the MAX96712 itself and "downstream" devices using the
MAX96712 as a video source there are a lot of features missing. Most
notably the ability to operate the GMSL bus.
Signed-off-by: Niklas Söderlund
---
MAINTAINERS | 6 +
drivers/staging/media/Kconfig | 2 +
drivers/staging/media/Makefile| 1 +
drivers/staging/media/max96712/Kconfig| 13 +
drivers/staging/media/max96712/Makefile | 2 +
drivers/staging/media/max96712/max96712.c | 429 ++
6 files changed, 453 insertions(+)
create mode 100644 drivers/staging/media/max96712/Kconfig
create mode 100644 drivers/staging/media/max96712/Makefile
create mode 100644 drivers/staging/media/max96712/max96712.c
diff --git a/MAINTAINERS b/MAINTAINERS
index cb727179826b1996..b265a7ba60e709f3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10831,6 +10831,12 @@ S: Maintained
F: Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
F: drivers/media/i2c/max9286.c
+MAX96712 QUAD GMSL2 DESERIALIZER DRIVER
+M: Niklas Söderlund
+L: linux-me...@vger.kernel.org
+S: Maintained
+F: drivers/staging/media/max96712/max96712.c
+
MAX9860 MONO AUDIO VOICE CODEC DRIVER
M: Peter Rosin
L: alsa-de...@alsa-project.org (moderated for non-subscribers)
diff --git a/drivers/staging/media/Kconfig b/drivers/staging/media/Kconfig
index ca59986b20f8a898..f104da4b5a358ffe 100644
--- a/drivers/staging/media/Kconfig
+++ b/drivers/staging/media/Kconfig
@@ -26,6 +26,8 @@ source "drivers/staging/media/hantro/Kconfig"
source "drivers/staging/media/imx/Kconfig"
+source "drivers/staging/media/max96712/Kconfig"
+
source "drivers/staging/media/meson/vdec/Kconfig"
source "drivers/staging/media/omap4iss/Kconfig"
diff --git a/drivers/staging/media/Makefile b/drivers/staging/media/Makefile
index 716929a1a313000f..37e0b1f85b5f7286 100644
--- a/drivers/staging/media/Makefile
+++ b/drivers/staging/media/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_INTEL_ATOMISP) += atomisp/
obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx/
+obj-$(CONFIG_VIDEO_MAX96712) += max96712/
obj-$(CONFIG_VIDEO_MESON_VDEC) += meson/vdec/
obj-$(CONFIG_VIDEO_OMAP4) += omap4iss/
obj-$(CONFIG_VIDEO_ROCKCHIP_VDEC) += rkvdec/
diff --git a/drivers/staging/media/max96712/Kconfig
b/drivers/staging/media/max96712/Kconfig
new file mode 100644
index ..258d47644cbd8459
--- /dev/null
+++ b/drivers/staging/media/max96712/Kconfig
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+config VIDEO_MAX96712
+ tristate "Maxim MAX96712 Quad GMSL2 Deserializer support"
+ depends on I2C
+ depends on OF_GPIO
+ select V4L2_FWNODE
+ select VIDEO_V4L2_SUBDEV_API
+ select MEDIA_CONTROLLER
+ help
+ This driver supports the Maxim MAX96712 Quad GMSL2 Deserializer.
+
+ To compile this driver as a module, choose M here: the
+ module will be called max96712.
diff --git a/drivers/staging/media/max96712/Makefile
b/drivers/staging/media/max96712/Makefile
new file mode 100644
index ..70c1974ce3f05fe4
--- /dev/null
+++ b/drivers/staging/media/max96712/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_VIDEO_MAX96712) += max96712.o
diff --git a/drivers/staging/media/max96712/max96712.c
b/drivers/staging/media/max96712/max96712.c
new file mode 100644
index ..6a993e8ee9259fda
--- /dev/null
+++ b/drivers/staging/media/max96712/max96712.c
@@ -0,0 +1,429 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Maxim MAX9286 Quad GMSL2 Deserializer Driver
+ *
+ * Copyright (C) 2021 Renesas Electronics Corporation
+ * Copyright (C) 2021 Niklas Söderlund
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#define MAX96712_ID 0x20
+
+#define MAX96712_DPLL_FREQ 1000
+
+enum max96712_pattern {
+ MAX96712_PATTERN_CHECKERBOARD = 0,
+ MAX96712_PATTERN_GRADIENT,
+};
+
+struct max96712_priv {
+ struct i2c_client *client;
+ struct regmap *regmap;
+ struct gpio_desc *gpiod_pwdn;
+
+ struct v4l2_fwnode_bus_mipi_csi2 mipi;
+
+ struct v4l2_subdev sd;
+ struct v4l2_ctrl_handler ctrl_handler;
+ struct media_pad pads[1];
+
+ enum max96712_pattern pattern;
+};
+
+static
[tip: timers/core] dt-bindings: timer: renesas,tmu: Document missing Gen3 SoCs
The following commit has been merged into the timers/core branch of tip: Commit-ID: c4d814416fe3f5eb27438209a83582d7508ba80a Gitweb: https://git.kernel.org/tip/c4d814416fe3f5eb27438209a83582d7508ba80a Author:Niklas Söderlund AuthorDate:Thu, 11 Feb 2021 15:31:02 +01:00 Committer: Daniel Lezcano CommitterDate: Thu, 08 Apr 2021 13:23:21 +02:00 dt-bindings: timer: renesas,tmu: Document missing Gen3 SoCs Add missing bindings for Gen3 SoCs. Signed-off-by: Niklas Söderlund Reviewed-by: Rob Herring Reviewed-by: Geert Uytterhoeven Signed-off-by: Daniel Lezcano Link: https://lore.kernel.org/r/20210211143102.350719-1-niklas.soderlund+rene...@ragnatech.se --- Documentation/devicetree/bindings/timer/renesas,tmu.yaml | 6 ++ 1 file changed, 6 insertions(+) diff --git a/Documentation/devicetree/bindings/timer/renesas,tmu.yaml b/Documentation/devicetree/bindings/timer/renesas,tmu.yaml index c541887..f0f0f12 100644 --- a/Documentation/devicetree/bindings/timer/renesas,tmu.yaml +++ b/Documentation/devicetree/bindings/timer/renesas,tmu.yaml @@ -28,8 +28,14 @@ properties: - renesas,tmu-r8a774e1 # RZ/G2H - renesas,tmu-r8a7778 # R-Car M1A - renesas,tmu-r8a7779 # R-Car H1 + - renesas,tmu-r8a7795 # R-Car H3 + - renesas,tmu-r8a7796 # R-Car M3-W + - renesas,tmu-r8a77961 # R-Car M3-W+ + - renesas,tmu-r8a77965 # R-Car M3-N - renesas,tmu-r8a77970 # R-Car V3M - renesas,tmu-r8a77980 # R-Car V3H + - renesas,tmu-r8a77990 # R-Car E3 + - renesas,tmu-r8a77995 # R-Car D3 - const: renesas,tmu reg:
[tip: timers/core] dt-bindings: timer: renesas,cmt: Document R8A77961
The following commit has been merged into the timers/core branch of tip: Commit-ID: 446e1a943554e07a4e6431fff6dd06c0f130895a Gitweb: https://git.kernel.org/tip/446e1a943554e07a4e6431fff6dd06c0f130895a Author:Niklas Söderlund AuthorDate:Thu, 11 Feb 2021 15:33:44 +01:00 Committer: Daniel Lezcano CommitterDate: Thu, 08 Apr 2021 13:23:24 +02:00 dt-bindings: timer: renesas,cmt: Document R8A77961 Add missing bindings for M3-W+. Signed-off-by: Niklas Söderlund Reviewed-by: Rob Herring Reviewed-by: Geert Uytterhoeven Signed-off-by: Daniel Lezcano Link: https://lore.kernel.org/r/20210211143344.352588-1-niklas.soderlund+rene...@ragnatech.se --- Documentation/devicetree/bindings/timer/renesas,cmt.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/timer/renesas,cmt.yaml b/Documentation/devicetree/bindings/timer/renesas,cmt.yaml index 363ec28..53dd6d9 100644 --- a/Documentation/devicetree/bindings/timer/renesas,cmt.yaml +++ b/Documentation/devicetree/bindings/timer/renesas,cmt.yaml @@ -74,6 +74,7 @@ properties: - renesas,r8a774e1-cmt0 # 32-bit CMT0 on RZ/G2H - renesas,r8a7795-cmt0 # 32-bit CMT0 on R-Car H3 - renesas,r8a7796-cmt0 # 32-bit CMT0 on R-Car M3-W + - renesas,r8a77961-cmt0 # 32-bit CMT0 on R-Car M3-W+ - renesas,r8a77965-cmt0 # 32-bit CMT0 on R-Car M3-N - renesas,r8a77970-cmt0 # 32-bit CMT0 on R-Car V3M - renesas,r8a77980-cmt0 # 32-bit CMT0 on R-Car V3H @@ -90,6 +91,7 @@ properties: - renesas,r8a774e1-cmt1 # 48-bit CMT on RZ/G2H - renesas,r8a7795-cmt1 # 48-bit CMT on R-Car H3 - renesas,r8a7796-cmt1 # 48-bit CMT on R-Car M3-W + - renesas,r8a77961-cmt1 # 48-bit CMT on R-Car M3-W+ - renesas,r8a77965-cmt1 # 48-bit CMT on R-Car M3-N - renesas,r8a77970-cmt1 # 48-bit CMT on R-Car V3M - renesas,r8a77980-cmt1 # 48-bit CMT on R-Car V3H
[tip: timers/core] timekeeping: Allow runtime PM from change_clocksource()
The following commit has been merged into the timers/core branch of tip:
Commit-ID: d4c7c28806616809e3baa0b7cd8c665516b2726d
Gitweb:
https://git.kernel.org/tip/d4c7c28806616809e3baa0b7cd8c665516b2726d
Author:Niklas Söderlund
AuthorDate:Thu, 11 Feb 2021 14:43:18 +01:00
Committer: Thomas Gleixner
CommitterDate: Mon, 29 Mar 2021 16:41:59 +02:00
timekeeping: Allow runtime PM from change_clocksource()
The struct clocksource callbacks enable() and disable() are described as a
way to allow clock sources to enter a power save mode. See commit
4614e6adafa2 ("clocksource: add enable() and disable() callbacks")
But using runtime PM from these callbacks triggers a cyclic lockdep warning when
switching clock source using change_clocksource().
# echo e60f.timer >
/sys/devices/system/clocksource/clocksource0/current_clocksource
==
WARNING: possible circular locking dependency detected
--
migration/0/11 is trying to acquire lock:
403ed220 (&dev->power.lock){-...}-{2:2}, at:
__pm_runtime_resume+0x40/0x74
but task is already holding lock:
8000113c8f88 (tk_core.seq.seqcount){}-{0:0}, at:
multi_cpu_stop+0xa4/0x190
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (tk_core.seq.seqcount){}-{0:0}:
ktime_get+0x28/0xa0
hrtimer_start_range_ns+0x210/0x2dc
generic_sched_clock_init+0x70/0x88
sched_clock_init+0x40/0x64
start_kernel+0x494/0x524
-> #1 (hrtimer_bases.lock){-.-.}-{2:2}:
hrtimer_start_range_ns+0x68/0x2dc
rpm_suspend+0x308/0x5dc
rpm_idle+0xc4/0x2a4
pm_runtime_work+0x98/0xc0
process_one_work+0x294/0x6f0
worker_thread+0x70/0x45c
kthread+0x154/0x160
ret_from_fork+0x10/0x20
-> #0 (&dev->power.lock){-...}-{2:2}:
_raw_spin_lock_irqsave+0x7c/0xc4
__pm_runtime_resume+0x40/0x74
sh_cmt_start+0x1c4/0x260
sh_cmt_clocksource_enable+0x28/0x50
change_clocksource+0x9c/0x160
multi_cpu_stop+0xa4/0x190
cpu_stopper_thread+0x90/0x154
smpboot_thread_fn+0x244/0x270
kthread+0x154/0x160
ret_from_fork+0x10/0x20
other info that might help us debug this:
Chain exists of:
&dev->power.lock --> hrtimer_bases.lock --> tk_core.seq.seqcount
Possible unsafe locking scenario:
CPU0CPU1
lock(tk_core.seq.seqcount);
lock(hrtimer_bases.lock);
lock(tk_core.seq.seqcount);
lock(&dev->power.lock);
*** DEADLOCK ***
2 locks held by migration/0/11:
#0: 8000113c9278 (timekeeper_lock){-.-.}-{2:2}, at:
change_clocksource+0x2c/0x160
#1: 8000113c8f88 (tk_core.seq.seqcount){}-{0:0}, at:
multi_cpu_stop+0xa4/0x190
Rework change_clocksource() so it enables the new clocksource and disables
the old clocksource outside of the timekeeper_lock and seqcount write held
region. There is no requirement that these callbacks are invoked from the
lock held region.
Signed-off-by: Niklas Söderlund
Signed-off-by: Thomas Gleixner
Tested-by: Wolfram Sang
Link:
https://lore.kernel.org/r/20210211134318.323910-1-niklas.soderlund+rene...@ragnatech.se
---
kernel/time/timekeeping.c | 36 +++-
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 77bafd8..81fe2a3 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1427,35 +1427,45 @@ static void __timekeeping_set_tai_offset(struct
timekeeper *tk, s32 tai_offset)
static int change_clocksource(void *data)
{
struct timekeeper *tk = &tk_core.timekeeper;
- struct clocksource *new, *old;
+ struct clocksource *new, *old = NULL;
unsigned long flags;
+ bool change = false;
new = (struct clocksource *) data;
- raw_spin_lock_irqsave(&timekeeper_lock, flags);
- write_seqcount_begin(&tk_core.seq);
-
- timekeeping_forward_now(tk);
/*
* If the cs is in module, get a module reference. Succeeds
* for built-in code (owner == NULL) as well.
*/
if (try_module_get(new->owner)) {
- if (!new->enable || new->enable(new) == 0) {
- old = tk->tkr_mono.clock;
- tk_setup_internals(tk, new);
- if (old->disable)
- old->disable(old);
- module_put(old->owner);
- } else {
+ if (!new->enable || new->enable(new) == 0)
+
Re: [PATCH v2] dt-bindings: timer: renesas,cmt: add r8a779a0 CMT support
Hi Wolfram, Thanks for your patch. On 2021-03-11 10:09:18 +0100, Wolfram Sang wrote: > Signed-off-by: Wolfram Sang Reviewed-by: Niklas Söderlund > --- > > Changes since V1: > * none, but additional testing was done which revealed that this CMT > in deed behaves the same as other Gen3 SoCs. There was one hickup > which seemed unique to V3U but has been reproduced with M3N meanwhile. > This is something we need to tackle, but no reason to prevent adding > V3U support. > > Documentation/devicetree/bindings/timer/renesas,cmt.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/timer/renesas,cmt.yaml > b/Documentation/devicetree/bindings/timer/renesas,cmt.yaml > index 428db3a21bb9..363ec28e07da 100644 > --- a/Documentation/devicetree/bindings/timer/renesas,cmt.yaml > +++ b/Documentation/devicetree/bindings/timer/renesas,cmt.yaml > @@ -79,6 +79,7 @@ properties: >- renesas,r8a77980-cmt0 # 32-bit CMT0 on R-Car V3H >- renesas,r8a77990-cmt0 # 32-bit CMT0 on R-Car E3 >- renesas,r8a77995-cmt0 # 32-bit CMT0 on R-Car D3 > + - renesas,r8a779a0-cmt0 # 32-bit CMT0 on R-Car V3U >- const: renesas,rcar-gen3-cmt0 # 32-bit CMT0 on R-Car Gen3 and > RZ/G2 > >- items: > @@ -94,6 +95,7 @@ properties: >- renesas,r8a77980-cmt1 # 48-bit CMT on R-Car V3H >- renesas,r8a77990-cmt1 # 48-bit CMT on R-Car E3 >- renesas,r8a77995-cmt1 # 48-bit CMT on R-Car D3 > + - renesas,r8a779a0-cmt1 # 48-bit CMT on R-Car V3U >- const: renesas,rcar-gen3-cmt1 # 48-bit CMT on R-Car Gen3 and > RZ/G2 > >reg: > -- > 2.30.0 > -- Regards, Niklas Söderlund
Re: [PATCH] clocksource: sh_cmt: use CMTOUT_IE only when supported
Hi Wolfram,
Thanks for your patch.
On 2021-03-05 14:28:59 +0100, Wolfram Sang wrote:
> CMTOUT_IE is only supported for older SoCs. Newer SoCs shall not set
> this bit. So, add a version check.
>
> Reported-by: Phong Hoang
> Signed-off-by: Wolfram Sang
Reviewed-by: Niklas Söderlund
> ---
>
> Confirmed with datasheets and could successfully repeat Niklas' CMT
> tests on an R-Car M3N based Salvator-XS.
>
> drivers/clocksource/sh_cmt.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
> index c98f8851fd68..3b53c6cb1da9 100644
> --- a/drivers/clocksource/sh_cmt.c
> +++ b/drivers/clocksource/sh_cmt.c
> @@ -143,6 +143,7 @@ struct sh_cmt_device {
> #define SH_CMT32_CMCSR_SSIE (1 << 10)
> #define SH_CMT32_CMCSR_CMS (1 << 9)
> #define SH_CMT32_CMCSR_CMM (1 << 8)
> +/* CMTOUT_IE only for SH_CMT_32BIT and SH_CMT_48BIT */
> #define SH_CMT32_CMCSR_CMTOUT_IE (1 << 7)
> #define SH_CMT32_CMCSR_CMR_NONE (0 << 4)
> #define SH_CMT32_CMCSR_CMR_DMA (1 << 4)
> @@ -339,8 +340,9 @@ static int sh_cmt_enable(struct sh_cmt_channel *ch)
> sh_cmt_write_cmcsr(ch, SH_CMT16_CMCSR_CMIE |
> SH_CMT16_CMCSR_CKS512);
> } else {
> - sh_cmt_write_cmcsr(ch, SH_CMT32_CMCSR_CMM |
> -SH_CMT32_CMCSR_CMTOUT_IE |
> + u32 cmtout = ch->cmt->info->model <= SH_CMT_48BIT ?
> + SH_CMT32_CMCSR_CMTOUT_IE : 0;
> + sh_cmt_write_cmcsr(ch, cmtout | SH_CMT32_CMCSR_CMM |
> SH_CMT32_CMCSR_CMR_IRQ |
> SH_CMT32_CMCSR_CKS_RCLK8);
> }
> --
> 2.29.2
>
--
Regards,
Niklas Söderlund
Re: [PATCH] dt-bindings: timer: renesas,tmu: add r8a779a0 TMU support
Hi Wolfram, Thanks for your work. On 2021-03-05 15:23:59 +0100, Wolfram Sang wrote: > Signed-off-by: Wolfram Sang Reviewed-by: Niklas Söderlund > --- > > This is the correct one. TMU passed the testsuite, CMT needs a second > look. > > Documentation/devicetree/bindings/timer/renesas,tmu.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/timer/renesas,tmu.yaml > b/Documentation/devicetree/bindings/timer/renesas,tmu.yaml > index c54188731a1b..20af9ce05ae5 100644 > --- a/Documentation/devicetree/bindings/timer/renesas,tmu.yaml > +++ b/Documentation/devicetree/bindings/timer/renesas,tmu.yaml > @@ -30,6 +30,7 @@ properties: >- renesas,tmu-r8a7779 # R-Car H1 >- renesas,tmu-r8a77970 # R-Car V3M >- renesas,tmu-r8a77980 # R-Car V3H > + - renesas,tmu-r8a779a0 # R-Car V3U >- const: renesas,tmu > >reg: > -- > 2.29.2 > -- Regards, Niklas Söderlund
[PATCH v2] timekeeping: Allow runtime PM from change_clocksource()
[ 118.625231] __lock_acquire+0x13b0/0x204c
[ 118.633451] lock_acquire.part.0+0x120/0x330
[ 118.641958] lock_acquire+0x64/0x80
[ 118.649630] _raw_spin_lock_irqsave+0x7c/0xc4
[ 118.658186] __pm_runtime_resume+0x40/0x74
[ 118.666483] sh_cmt_start+0x1c4/0x260
[ 118.674327] sh_cmt_clocksource_enable+0x28/0x50
[ 118.683170] change_clocksource+0x9c/0x160
[ 118.691460] multi_cpu_stop+0xa4/0x190
[ 118.699384] cpu_stopper_thread+0x90/0x154
[ 118.707672] smpboot_thread_fn+0x244/0x270
[ 118.715961] kthread+0x154/0x160
[ 118.723360] ret_from_fork+0x10/0x20
[ 118.731465] clocksource: Switched to clocksource e60f.timer
1. commit 4614e6adafa2c5e6 ("clocksource: add enable() and disable() callbacks")
Signed-off-by: Niklas Söderlund
---
kernel/time/timekeeping.c | 36 +++-
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 6aee5768c86ff7db..26ef4c9ef5c57081 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1427,35 +1427,45 @@ static void __timekeeping_set_tai_offset(struct
timekeeper *tk, s32 tai_offset)
static int change_clocksource(void *data)
{
struct timekeeper *tk = &tk_core.timekeeper;
- struct clocksource *new, *old;
+ struct clocksource *new, *old = NULL;
unsigned long flags;
+ bool change = false;
new = (struct clocksource *) data;
- raw_spin_lock_irqsave(&timekeeper_lock, flags);
- write_seqcount_begin(&tk_core.seq);
-
- timekeeping_forward_now(tk);
/*
* If the cs is in module, get a module reference. Succeeds
* for built-in code (owner == NULL) as well.
*/
if (try_module_get(new->owner)) {
- if (!new->enable || new->enable(new) == 0) {
- old = tk->tkr_mono.clock;
- tk_setup_internals(tk, new);
- if (old->disable)
- old->disable(old);
- module_put(old->owner);
- } else {
+ if (!new->enable || new->enable(new) == 0)
+ change = true;
+ else
module_put(new->owner);
- }
}
+
+ raw_spin_lock_irqsave(&timekeeper_lock, flags);
+ write_seqcount_begin(&tk_core.seq);
+
+ timekeeping_forward_now(tk);
+
+ if (change) {
+ old = tk->tkr_mono.clock;
+ tk_setup_internals(tk, new);
+ }
+
timekeeping_update(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);
write_seqcount_end(&tk_core.seq);
raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
+ if (old) {
+ if (old->disable)
+ old->disable(old);
+
+ module_put(old->owner);
+ }
+
return 0;
}
--
2.30.0
Re: [PATCH] sh_eth: Fix power down vs. is_opened flag ordering
Hi Geert,
Thanks for your work.
On 2021-01-18 16:08:12 +0100, Geert Uytterhoeven wrote:
> sh_eth_close() does a synchronous power down of the device before
> marking it closed. Revert the order, to make sure the device is never
> marked opened while suspended.
>
> While at it, use pm_runtime_put() instead of pm_runtime_put_sync(), as
> there is no reason to do a synchronous power down.
>
> Fixes: 7fa2955ff70ce453 ("sh_eth: Fix sleeping function called from invalid
> context")
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Niklas Söderlund
> ---
> drivers/net/ethernet/renesas/sh_eth.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
> b/drivers/net/ethernet/renesas/sh_eth.c
> index 9b52d350e21a9f2b..590b088bc4c7f3e2 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -2606,10 +2606,10 @@ static int sh_eth_close(struct net_device *ndev)
> /* Free all the skbuffs in the Rx queue and the DMA buffer. */
> sh_eth_ring_free(ndev);
>
> - pm_runtime_put_sync(&mdp->pdev->dev);
> -
> mdp->is_opened = 0;
>
> + pm_runtime_put(&mdp->pdev->dev);
> +
> return 0;
> }
>
> --
> 2.25.1
>
--
Regards,
Niklas Söderlund
Re: [PATCH] clocksource/drivers/sh_cmt: Make sure channel clock supply is enabled
Hi Geert,
Thanks for your patch.
On 2020-12-10 20:46:48 +0100, Geert Uytterhoeven wrote:
> The Renesas Compare Match Timer 0 and 1 (CMT0/1) variants have a
> register to control the clock supply to the individual channels.
> Currently the driver does not touch this register, and relies on the
> documented initial value, which has the clock supply enabled for all
> channels present.
>
> However, when Linux starts on the APE6-EVM development board, only the
> clock supply to the first CMT1 channel is enabled. Hence the first
> channel (used as a clockevent) works, while the second channel (used as
> a clocksource) does not. Note that the default system clocksource is
> the Cortex-A15 architectured timer, and the user needs to manually
> switch to the CMT1 clocksource to trigger the broken behavior.
>
> Fix this by removing the fragile dependency on implicit reset and/or
> boot loader state, and by enabling the clock supply explicitly for all
> channels used instead. This requires postponing the clk_disable() call,
> else the timer's registers cannot be accessed in sh_cmt_setup_channel().
>
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Niklas Söderlund
> ---
> Tested on R-Mobile APE6, R-Car M2-W, and R-Car H3 ES2.0.
> ---
> drivers/clocksource/sh_cmt.c | 16 +---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
> index e258230d432c0002..c98f8851fd680454 100644
> --- a/drivers/clocksource/sh_cmt.c
> +++ b/drivers/clocksource/sh_cmt.c
> @@ -235,6 +235,8 @@ static const struct sh_cmt_info sh_cmt_info[] = {
> #define CMCNT 1 /* channel register */
> #define CMCOR 2 /* channel register */
>
> +#define CMCLKE 0x1000 /* CLK Enable Register (R-Car Gen2) */
> +
> static inline u32 sh_cmt_read_cmstr(struct sh_cmt_channel *ch)
> {
> if (ch->iostart)
> @@ -853,6 +855,7 @@ static int sh_cmt_setup_channel(struct sh_cmt_channel
> *ch, unsigned int index,
> unsigned int hwidx, bool clockevent,
> bool clocksource, struct sh_cmt_device *cmt)
> {
> + u32 value;
> int ret;
>
> /* Skip unused channels. */
> @@ -882,6 +885,11 @@ static int sh_cmt_setup_channel(struct sh_cmt_channel
> *ch, unsigned int index,
> ch->iostart = cmt->mapbase + ch->hwidx * 0x100;
> ch->ioctrl = ch->iostart + 0x10;
> ch->timer_bit = 0;
> +
> + /* Enable the clock supply to the channel */
> + value = ioread32(cmt->mapbase + CMCLKE);
> + value |= BIT(hwidx);
> + iowrite32(value, cmt->mapbase + CMCLKE);
> break;
> }
>
> @@ -1014,12 +1022,10 @@ static int sh_cmt_setup(struct sh_cmt_device *cmt,
> struct platform_device *pdev)
> else
> cmt->rate = clk_get_rate(cmt->clk) / 8;
>
> - clk_disable(cmt->clk);
> -
> /* Map the memory resource(s). */
> ret = sh_cmt_map_memory(cmt);
> if (ret < 0)
> - goto err_clk_unprepare;
> + goto err_clk_disable;
>
> /* Allocate and setup the channels. */
> cmt->num_channels = hweight8(cmt->hw_channels);
> @@ -1047,6 +1053,8 @@ static int sh_cmt_setup(struct sh_cmt_device *cmt,
> struct platform_device *pdev)
> mask &= ~(1 << hwidx);
> }
>
> + clk_disable(cmt->clk);
> +
> platform_set_drvdata(pdev, cmt);
>
> return 0;
> @@ -1054,6 +1062,8 @@ static int sh_cmt_setup(struct sh_cmt_device *cmt,
> struct platform_device *pdev)
> err_unmap:
> kfree(cmt->channels);
> iounmap(cmt->mapbase);
> +err_clk_disable:
> + clk_disable(cmt->clk);
> err_clk_unprepare:
> clk_unprepare(cmt->clk);
> err_clk_put:
> --
> 2.25.1
>
--
Regards,
Niklas Söderlund
[tip: timers/core] clocksource/drivers/sh_cmt: Fix potential deadlock when calling runtime PM
The following commit has been merged into the timers/core branch of tip:
Commit-ID: 8ae954caf49ac403c177d117fb8e05cbc866aa3c
Gitweb:
https://git.kernel.org/tip/8ae954caf49ac403c177d117fb8e05cbc866aa3c
Author:Niklas Söderlund
AuthorDate:Sat, 05 Dec 2020 03:19:20 +01:00
Committer: Daniel Lezcano
CommitterDate: Mon, 07 Dec 2020 20:10:05 +01:00
clocksource/drivers/sh_cmt: Fix potential deadlock when calling runtime PM
The ch->lock is used to protect the whole enable() and read() of
sh_cmt's implementation of struct clocksource. The enable()
implementation calls pm_runtime_get_sync() which may result in the clock
source to be read() triggering a cyclic lockdep warning for the
ch->lock.
The sh_cmt driver implement its own balancing of calls to
sh_cmt_{enable,disable}() with flags in sh_cmt_{start,stop}(). It does
this to deal with that start and stop are shared between the clock
source and clock event providers. While this could be improved on
verifying corner cases based on any substantial rework on all devices
this driver supports might prove hard.
As a first step separate the PM handling for clock event and clock
source. Always put/get the device when enabling/disabling the clock
source but keep the clock event logic unchanged. This allows the sh_cmt
implementation of struct clocksource to call PM without holding the
ch->lock and avoiding the deadlock.
Triggering and log of the deadlock warning,
# echo e60f.timer >
/sys/devices/system/clocksource/clocksource0/current_clocksource
[ 46.948370] ==
[ 46.954730] WARNING: possible circular locking dependency detected
[ 46.961094] 5.10.0-rc6-arm64-renesas-1-g0e5fd7414e8b #36 Not tainted
[ 46.967985] --
[ 46.974342] migration/0/11 is trying to acquire lock:
[ 46.979543] 403ed220 (&dev->power.lock){-...}-{2:2}, at:
__pm_runtime_resume+0x40/0x74
[ 46.988445]
[ 46.988445] but task is already holding lock:
[ 46.994441] 40ad0298 (&ch->lock){}-{2:2}, at:
sh_cmt_start+0x28/0x210
[ 47.002173]
[ 47.002173] which lock already depends on the new lock.
[ 47.002173]
[ 47.010573]
[ 47.010573] the existing dependency chain (in reverse order) is:
[ 47.018262]
[ 47.018262] -> #3 (&ch->lock){}-{2:2}:
[ 47.024033]lock_acquire.part.0+0x120/0x330
[ 47.028970]lock_acquire+0x64/0x80
[ 47.033105]_raw_spin_lock_irqsave+0x7c/0xc4
[ 47.038130]sh_cmt_start+0x28/0x210
[ 47.042352]sh_cmt_clocksource_enable+0x28/0x50
[ 47.047644]change_clocksource+0x9c/0x160
[ 47.052402]multi_cpu_stop+0xa4/0x190
[ 47.056799]cpu_stopper_thread+0x90/0x154
[ 47.061557]smpboot_thread_fn+0x244/0x270
[ 47.066310]kthread+0x154/0x160
[ 47.070175]ret_from_fork+0x10/0x20
[ 47.074390]
[ 47.074390] -> #2 (tk_core.seq.seqcount){}-{0:0}:
[ 47.081136]lock_acquire.part.0+0x120/0x330
[ 47.086070]lock_acquire+0x64/0x80
[ 47.090203]seqcount_lockdep_reader_access.constprop.0+0x74/0x100
[ 47.097096]ktime_get+0x28/0xa0
[ 47.100960]hrtimer_start_range_ns+0x210/0x2dc
[ 47.106164]generic_sched_clock_init+0x70/0x88
[ 47.111364]sched_clock_init+0x40/0x64
[ 47.115853]start_kernel+0x494/0x524
[ 47.120156]
[ 47.120156] -> #1 (hrtimer_bases.lock){-.-.}-{2:2}:
[ 47.126721]lock_acquire.part.0+0x120/0x330
[ 47.136042]lock_acquire+0x64/0x80
[ 47.144461]_raw_spin_lock_irqsave+0x7c/0xc4
[ 47.153721]hrtimer_start_range_ns+0x68/0x2dc
[ 47.163054]rpm_suspend+0x308/0x5dc
[ 47.171473]rpm_idle+0xc4/0x2a4
[ 47.179550]pm_runtime_work+0x98/0xc0
[ 47.188209]process_one_work+0x294/0x6f0
[ 47.197142]worker_thread+0x70/0x45c
[ 47.205661]kthread+0x154/0x160
[ 47.213673]ret_from_fork+0x10/0x20
[ 47.221957]
[ 47.221957] -> #0 (&dev->power.lock){-...}-{2:2}:
[ 47.236292]check_noncircular+0x128/0x140
[ 47.244907]__lock_acquire+0x13b0/0x204c
[ 47.253332]lock_acquire.part.0+0x120/0x330
[ 47.262033]lock_acquire+0x64/0x80
[ 47.269826]_raw_spin_lock_irqsave+0x7c/0xc4
[ 47.278430]__pm_runtime_resume+0x40/0x74
[ 47.286758]sh_cmt_start+0x84/0x210
[ 47.294537]sh_cmt_clocksource_enable+0x28/0x50
[ 47.303449]change_clocksource+0x9c/0x160
[ 47.311783]multi_cpu_stop+0xa4/0x190
[ 47.319720]cpu_stopper_thread+0x90/0x154
[ 47.328022]smpboot_thread_fn+0x244/0x270
[ 47.336298]kthread+0x154/0x160
[ 47.343708]ret_from_fork+0x10/0x20
[ 47.351
Re: [PATCH 4/5] thermal/drivers/rcar: Remove notification usage
Hi Daniel,
Thanks for your work.
On 2020-12-10 13:15:13 +0100, Daniel Lezcano wrote:
> The ops is only showing a trace telling a critical trip point is
> crossed. The same information is given by the thermal framework.
>
> This is redundant, remove the code.
>
> Signed-off-by: Daniel Lezcano
Reviewed-by: Niklas Söderlund
> ---
> drivers/thermal/rcar_thermal.c | 19 ---
> 1 file changed, 19 deletions(-)
>
> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> index 6ae757d66f46..b49f04daaf47 100644
> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -323,24 +323,6 @@ static int rcar_thermal_get_trip_temp(struct
> thermal_zone_device *zone,
> return 0;
> }
>
> -static int rcar_thermal_notify(struct thermal_zone_device *zone,
> -int trip, enum thermal_trip_type type)
> -{
> - struct rcar_thermal_priv *priv = rcar_zone_to_priv(zone);
> - struct device *dev = rcar_priv_to_dev(priv);
> -
> - switch (type) {
> - case THERMAL_TRIP_CRITICAL:
> - /* FIXME */
> - dev_warn(dev, "Thermal reached to critical temperature\n");
> - break;
> - default:
> - break;
> - }
> -
> - return 0;
> -}
> -
> static const struct thermal_zone_of_device_ops rcar_thermal_zone_of_ops = {
> .get_temp = rcar_thermal_of_get_temp,
> };
> @@ -349,7 +331,6 @@ static struct thermal_zone_device_ops
> rcar_thermal_zone_ops = {
> .get_temp = rcar_thermal_get_temp,
> .get_trip_type = rcar_thermal_get_trip_type,
> .get_trip_temp = rcar_thermal_get_trip_temp,
> - .notify = rcar_thermal_notify,
> };
>
> /*
> --
> 2.17.1
>
--
Regards,
Niklas Söderlund
[PATCH 1/2] clocksource/drivers/sh_cmt: Fix potential deadlock when calling runtime PM
7.428425]lock(tk_core.seq.seqcount);
[ 47.438701]lock(&ch->lock);
[ 47.447930] lock(&dev->power.lock);
[ 47.455172]
[ 47.455172] *** DEADLOCK ***
[ 47.455172]
[ 47.471433] 3 locks held by migration/0/11:
[ 47.479099] #0: 8000113c9278 (timekeeper_lock){-.-.}-{2:2}, at:
change_clocksource+0x2c/0x160
[ 47.491834] #1: 8000113c8f88 (tk_core.seq.seqcount){}-{0:0}, at:
multi_cpu_stop+0xa4/0x190
[ 47.504727] #2: 40ad0298 (&ch->lock){}-{2:2}, at:
sh_cmt_start+0x28/0x210
[ 47.516541]
[ 47.516541] stack backtrace:
[ 47.528480] CPU: 0 PID: 11 Comm: migration/0 Not tainted
5.10.0-rc6-arm64-renesas-1-g0e5fd7414e8b #36
[ 47.542147] Hardware name: Renesas Salvator-X 2nd version board based on
r8a77965 (DT)
[ 47.554241] Call trace:
[ 47.560832] dump_backtrace+0x0/0x190
[ 47.568670] show_stack+0x14/0x30
[ 47.576144] dump_stack+0xe8/0x130
[ 47.583670] print_circular_bug+0x1f0/0x200
[ 47.592015] check_noncircular+0x128/0x140
[ 47.600289] __lock_acquire+0x13b0/0x204c
[ 47.608486] lock_acquire.part.0+0x120/0x330
[ 47.616953] lock_acquire+0x64/0x80
[ 47.624582] _raw_spin_lock_irqsave+0x7c/0xc4
[ 47.633114] __pm_runtime_resume+0x40/0x74
[ 47.641371] sh_cmt_start+0x84/0x210
[ 47.649115] sh_cmt_clocksource_enable+0x28/0x50
[ 47.657916] change_clocksource+0x9c/0x160
[ 47.666165] multi_cpu_stop+0xa4/0x190
[ 47.674056] cpu_stopper_thread+0x90/0x154
[ 47.682308] smpboot_thread_fn+0x244/0x270
[ 47.690560] kthread+0x154/0x160
[ 47.697927] ret_from_fork+0x10/0x20
[ 47.708447] clocksource: Switched to clocksource e60f.timer
Signed-off-by: Niklas Söderlund
---
drivers/clocksource/sh_cmt.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
index 760777458a9091cd..19fa3ef75e3bd62b 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
@@ -319,7 +319,6 @@ static int sh_cmt_enable(struct sh_cmt_channel *ch)
{
int k, ret;
- pm_runtime_get_sync(&ch->cmt->pdev->dev);
dev_pm_syscore_device(&ch->cmt->pdev->dev, true);
/* enable clock */
@@ -394,7 +393,6 @@ static void sh_cmt_disable(struct sh_cmt_channel *ch)
clk_disable(ch->cmt->clk);
dev_pm_syscore_device(&ch->cmt->pdev->dev, false);
- pm_runtime_put(&ch->cmt->pdev->dev);
}
/* private flags */
@@ -562,10 +560,16 @@ static int sh_cmt_start(struct sh_cmt_channel *ch,
unsigned long flag)
int ret = 0;
unsigned long flags;
+ if (flag & FLAG_CLOCKSOURCE)
+ pm_runtime_get_sync(&ch->cmt->pdev->dev);
+
raw_spin_lock_irqsave(&ch->lock, flags);
- if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE)))
+ if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) {
+ if (flag & FLAG_CLOCKEVENT)
+ pm_runtime_get_sync(&ch->cmt->pdev->dev);
ret = sh_cmt_enable(ch);
+ }
if (ret)
goto out;
@@ -590,14 +594,20 @@ static void sh_cmt_stop(struct sh_cmt_channel *ch,
unsigned long flag)
f = ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE);
ch->flags &= ~flag;
- if (f && !(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE)))
+ if (f && !(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) {
sh_cmt_disable(ch);
+ if (flag & FLAG_CLOCKEVENT)
+ pm_runtime_put(&ch->cmt->pdev->dev);
+ }
/* adjust the timeout to maximum if only clocksource left */
if ((flag == FLAG_CLOCKEVENT) && (ch->flags & FLAG_CLOCKSOURCE))
__sh_cmt_set_next(ch, ch->max_match_value);
raw_spin_unlock_irqrestore(&ch->lock, flags);
+
+ if (flag & FLAG_CLOCKSOURCE)
+ pm_runtime_put(&ch->cmt->pdev->dev);
}
static struct sh_cmt_channel *cs_to_sh_cmt(struct clocksource *cs)
--
2.29.2
[PATCH 0/2] timekeeping: Fix change_clocksource() for PM and sh_cmt
[OK] Consistent CLOCK_BOOTTIME [OK] Consistent CLOCK_TAI [OK] # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:0 error:0 Nanosleep CLOCK_REALTIME [OK] Nanosleep CLOCK_MONOTONIC [OK] Nanosleep CLOCK_MONOTONIC_RAW [UNSUPPORTED] Nanosleep CLOCK_REALTIME_COARSE [UNSUPPORTED] Nanosleep CLOCK_MONOTONIC_COARSE [UNSUPPORTED] Nanosleep CLOCK_BOOTTIME [OK] Nanosleep CLOCK_REALTIME_ALARM[UNSUPPORTED] Nanosleep CLOCK_BOOTTIME_ALARM[UNSUPPORTED] Nanosleep CLOCK_TAI [OK] # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:0 error:0 # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:0 error:0 Niklas Söderlund (2): clocksource/drivers/sh_cmt: Fix potential deadlock when calling runtime PM timekeeping: Allow runtime PM from change_clocksource() drivers/clocksource/sh_cmt.c | 18 ++ kernel/time/timekeeping.c| 36 +++- 2 files changed, 37 insertions(+), 17 deletions(-) -- 2.29.2
[PATCH 2/2] timekeeping: Allow runtime PM from change_clocksource()
[ 118.625231] __lock_acquire+0x13b0/0x204c
[ 118.633451] lock_acquire.part.0+0x120/0x330
[ 118.641958] lock_acquire+0x64/0x80
[ 118.649630] _raw_spin_lock_irqsave+0x7c/0xc4
[ 118.658186] __pm_runtime_resume+0x40/0x74
[ 118.666483] sh_cmt_start+0x1c4/0x260
[ 118.674327] sh_cmt_clocksource_enable+0x28/0x50
[ 118.683170] change_clocksource+0x9c/0x160
[ 118.691460] multi_cpu_stop+0xa4/0x190
[ 118.699384] cpu_stopper_thread+0x90/0x154
[ 118.707672] smpboot_thread_fn+0x244/0x270
[ 118.715961] kthread+0x154/0x160
[ 118.723360] ret_from_fork+0x10/0x20
[ 118.731465] clocksource: Switched to clocksource e60f.timer
1. commit 4614e6adafa2c5e6 ("clocksource: add enable() and disable() callbacks")
Signed-off-by: Niklas Söderlund
---
kernel/time/timekeeping.c | 36 +++-
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 6858a31364b6456f..41f587359ca8db0e 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1433,35 +1433,45 @@ static void __timekeeping_set_tai_offset(struct
timekeeper *tk, s32 tai_offset)
static int change_clocksource(void *data)
{
struct timekeeper *tk = &tk_core.timekeeper;
- struct clocksource *new, *old;
+ struct clocksource *new, *old = NULL;
unsigned long flags;
+ bool change = false;
new = (struct clocksource *) data;
- raw_spin_lock_irqsave(&timekeeper_lock, flags);
- write_seqcount_begin(&tk_core.seq);
-
- timekeeping_forward_now(tk);
/*
* If the cs is in module, get a module reference. Succeeds
* for built-in code (owner == NULL) as well.
*/
if (try_module_get(new->owner)) {
- if (!new->enable || new->enable(new) == 0) {
- old = tk->tkr_mono.clock;
- tk_setup_internals(tk, new);
- if (old->disable)
- old->disable(old);
- module_put(old->owner);
- } else {
+ if (!new->enable || new->enable(new) == 0)
+ change = true;
+ else
module_put(new->owner);
- }
}
+
+ raw_spin_lock_irqsave(&timekeeper_lock, flags);
+ write_seqcount_begin(&tk_core.seq);
+
+ timekeeping_forward_now(tk);
+
+ if (change) {
+ old = tk->tkr_mono.clock;
+ tk_setup_internals(tk, new);
+ }
+
timekeeping_update(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);
write_seqcount_end(&tk_core.seq);
raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
+ if (old) {
+ if (old->disable)
+ old->disable(old);
+
+ module_put(old->owner);
+ }
+
return 0;
}
--
2.29.2
Re: [PATCH][next] media: rcar-vin: fix return, use ret instead of zero
Hi Colin,
Thanks for catching this!
On 2020-12-04 17:38:33 +, Colin King wrote:
> From: Colin Ian King
>
> Currently the return error code is in ret is being assigned but not
> used. It and should be returned by the return statement and currently
> just 0 is being returned. Fix this.
>
> Addresses-Coverity: ("Unused value")
> Fixes: b9ad52aafe38 ("media: rcar-vin: Rework parallel firmware parsing")
> Signed-off-by: Colin Ian King
Reviewed-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c
> index 98bff765b02e..e48d666f2c63 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -654,7 +654,7 @@ static int rvin_parallel_parse_of(struct rvin_dev *vin)
> out:
> fwnode_handle_put(fwnode);
>
> - return 0;
> + return ret;
> }
>
> static int rvin_parallel_init(struct rvin_dev *vin)
> --
> 2.29.2
>
--
Regards,
Niklas Söderlund
Re: [PATCH v5 0/2] dt-bindings: timer: renesas: tmu: Document r8a774e1 and
Hi Geert,
Thanks for your work.
On 2020-11-10 17:20:12 +0100, Geert Uytterhoeven wrote:
> Hi Daniel, Thomas,
>
> This patch series picks up missing Device Tree binding updates for the
> Renesas Timer Unit (TMU), and converts the bindings to json-schema.
>
> Thanks for applying!
For the whole series,
Reviewed-by: Niklas Söderlund
>
> Geert Uytterhoeven (1):
> dt-bindings: timer: renesas: tmu: Convert to json-schema
>
> Marian-Cristian Rotariu (1):
> dt-bindings: timer: renesas: tmu: Document r8a774e1 bindings
>
> .../devicetree/bindings/timer/renesas,tmu.txt | 49 -
> .../bindings/timer/renesas,tmu.yaml | 99 +++
> 2 files changed, 99 insertions(+), 49 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/timer/renesas,tmu.txt
> create mode 100644 Documentation/devicetree/bindings/timer/renesas,tmu.yaml
>
> --
> 2.25.1
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@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
--
Regards,
Niklas Söderlund
Re: [PATCH v2 2/4] arm64: dts: renesas: Add support for MIPI Adapter V2.1 connected to HiHope RZ/G2H
Hi Lad,
Thanks for your work.
On 2020-10-20 13:51:32 +0100, Lad Prabhakar wrote:
> Add support for AISTARVISION MIPI Adapter V2.1 board connected to HiHope
> RZ/G2H board.
>
> Common file hihope-rzg2-ex-aistarvision-mipi-adapter-2.1.dtsi is created
> which will be used by RZ/G2{HMN}, by default the CSI20 node is tied to
> ov5645 camera endpoint and the imx219 camera endpoint is tied to CSI40.
>
> Signed-off-by: Lad Prabhakar
> Reviewed-by: Biju Das
I have not reviewed the schematics but the patch in itself looks good,
Acked-by: Niklas Söderlund
> ---
> v1->v2 - No change
> ---
> arch/arm64/boot/dts/renesas/Makefile | 1 +
> ...rzg2-ex-aistarvision-mipi-adapter-2.1.dtsi | 109 ++
> .../r8a774e1-hihope-rzg2h-ex-mipi-2.1.dts | 16 +++
> 3 files changed, 126 insertions(+)
> create mode 100644
> arch/arm64/boot/dts/renesas/hihope-rzg2-ex-aistarvision-mipi-adapter-2.1.dtsi
> create mode 100644
> arch/arm64/boot/dts/renesas/r8a774e1-hihope-rzg2h-ex-mipi-2.1.dts
>
> diff --git a/arch/arm64/boot/dts/renesas/Makefile
> b/arch/arm64/boot/dts/renesas/Makefile
> index dffefe030a76..f98e9e2e520d 100644
> --- a/arch/arm64/boot/dts/renesas/Makefile
> +++ b/arch/arm64/boot/dts/renesas/Makefile
> @@ -22,6 +22,7 @@ dtb-$(CONFIG_ARCH_R8A774C0) += r8a774c0-ek874-mipi-2.1.dtb
> dtb-$(CONFIG_ARCH_R8A774E1) += r8a774e1-hihope-rzg2h.dtb
> dtb-$(CONFIG_ARCH_R8A774E1) += r8a774e1-hihope-rzg2h-ex.dtb
> dtb-$(CONFIG_ARCH_R8A774E1) += r8a774e1-hihope-rzg2h-ex-idk-1110wr.dtb
> +dtb-$(CONFIG_ARCH_R8A774E1) += r8a774e1-hihope-rzg2h-ex-mipi-2.1.dtb
>
> dtb-$(CONFIG_ARCH_R8A77950) += r8a77950-salvator-x.dtb
> dtb-$(CONFIG_ARCH_R8A77950) += r8a77950-ulcb.dtb
> diff --git
> a/arch/arm64/boot/dts/renesas/hihope-rzg2-ex-aistarvision-mipi-adapter-2.1.dtsi
>
> b/arch/arm64/boot/dts/renesas/hihope-rzg2-ex-aistarvision-mipi-adapter-2.1.dtsi
> new file mode 100644
> index ..c62ddb9b2ba5
> --- /dev/null
> +++
> b/arch/arm64/boot/dts/renesas/hihope-rzg2-ex-aistarvision-mipi-adapter-2.1.dtsi
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree Source for the HiHope RZ/G2[HMN] MIPI common parts
> + *
> + * Copyright (C) 2020 Renesas Electronics Corp.
> + */
> +
> +#define MIPI_OV5645_PARENT_I2C i2c2
> +#define MIPI_IMX219_PARENT_I2C i2c3
> +#include "aistarvision-mipi-adapter-2.1.dtsi"
> +
> +&csi20 {
> + status = "okay";
> +
> + ports {
> + port@0 {
> + reg = <0>;
> + csi20_in: endpoint {
> + clock-lanes = <0>;
> + data-lanes = <1 2>;
> + remote-endpoint = <&ov5645_ep>;
> + };
> + };
> + };
> +};
> +
> +&csi40 {
> + status = "okay";
> +
> + ports {
> + port@0 {
> + reg = <0>;
> + csi40_in: endpoint {
> + clock-lanes = <0>;
> + data-lanes = <1 2>;
> + remote-endpoint = <&imx219_ep>;
> + };
> + };
> + };
> +};
> +
> +&i2c3 {
> + pinctrl-0 = <&i2c3_pins>;
> + pinctrl-names = "default";
> + status = "okay";
> +};
> +
> +&imx219 {
> + port {
> + imx219_ep: endpoint {
> + clock-lanes = <0>;
> + data-lanes = <1 2>;
> + link-frequencies = /bits/ 64 <45600>;
> + remote-endpoint = <&csi40_in>;
> + };
> + };
> +};
> +
> +&ov5645 {
> + enable-gpios = <&gpio6 4 GPIO_ACTIVE_HIGH>;
> + reset-gpios = <&gpio6 8 GPIO_ACTIVE_LOW>;
> +
> + port {
> + ov5645_ep: endpoint {
> + clock-lanes = <0>;
> + data-lanes = <1 2>;
> + remote-endpoint = <&csi20_in>;
> + };
> + };
> +};
> +
> +&pfc {
> + i2c3_pins: i2c3 {
> + groups = "i2c3";
> + function = "i2c3";
> + };
> +};
> +
> +&vin0 {
> + status = "okay";
> +};
> +
> +&vin1 {
> + status = "okay";
> +};
> +
> +&vin2 {
> + status = "okay";
> +};
> +
> +&vin3 {
> + status = "okay";
> +};
> +
> +&a
Re: [PATCH] thermal: replace spin_lock_irqsave by spin_lock in hard IRQ
Hi Tian,
Thanks for your work.
On 2020-10-21 11:05:30 +0800, Tian Tao wrote:
> The code has been in a irq-disabled context since it is hard IRQ. There
> is no necessity to do it again.
>
> Signed-off-by: Tian Tao
Tested-by: Niklas Söderlund
> ---
> drivers/thermal/rcar_thermal.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> index 5c2a13b..6ae757d 100644
> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -409,16 +409,15 @@ static irqreturn_t rcar_thermal_irq(int irq, void *data)
> {
> struct rcar_thermal_common *common = data;
> struct rcar_thermal_priv *priv;
> - unsigned long flags;
> u32 status, mask;
>
> - spin_lock_irqsave(&common->lock, flags);
> + spin_lock(&common->lock);
>
> mask= rcar_thermal_common_read(common, INTMSK);
> status = rcar_thermal_common_read(common, STR);
> rcar_thermal_common_write(common, STR, 0x000F0F0F & mask);
>
> - spin_unlock_irqrestore(&common->lock, flags);
> + spin_unlock(&common->lock);
>
> status = status & ~mask;
>
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH v2 5/6] dt-bindings: media: renesas,vin: Add V3U support
Hi Jacopo, Thanks for your work. On 2020-10-16 13:11:57 +0200, Jacopo Mondi wrote: > Add compatible string definition for R-Car V3U. > > Signed-off-by: Jacopo Mondi > --- > > This and the following patch are still under discussion as the > proper support for the V3U VIN might require the implementation > of a different driver. > > I'm fine deferring it to when we'll have a more clear plan for V3U support. I think it's best we defer it until we have a plan for how we are going to support it on the driver side. > > v2: > - Increase maxium value for the 'renesas,id' property to support > the V3U's 31 VIN instances > > --- > Documentation/devicetree/bindings/media/renesas,vin.yaml | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml > b/Documentation/devicetree/bindings/media/renesas,vin.yaml > index ad2fe660364b..f7ac1fd3df25 100644 > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml > @@ -51,6 +51,7 @@ properties: >- renesas,vin-r8a77980 # R-Car V3H >- renesas,vin-r8a77990 # R-Car E3 >- renesas,vin-r8a77995 # R-Car D3 > + - renesas,vin-r8a779a0 # R-Car V3U > >reg: > maxItems: 1 > @@ -120,7 +121,7 @@ properties: > description: VIN channel number > $ref: /schemas/types.yaml#/definitions/uint32 > minimum: 0 > -maximum: 15 > +maximum: 31 > >ports: > type: object > -- > 2.28.0 > -- Regards, Niklas Söderlund
Re: [PATCH 6/6] arm64: dts: r8a779a0: Add VIN nodes
+ reg = <0 0xe6ed1000 0 0x1000>;
> + interrupts = ;
> + clocks = <&cpg CPG_MOD 815>;
> + power-domains = <&sysc R8A779A0_PD_ALWAYS_ON>;
> + resets = <&cpg 815>;
> + status = "disabled";
> + };
> +
> + vin18: video@e6ed2000 {
> + compatible = "renesas,vin-r8a779a0";
> + reg = <0 0xe6ed2000 0 0x1000>;
> + interrupts = ;
> + clocks = <&cpg CPG_MOD 816>;
> + power-domains = <&sysc R8A779A0_PD_ALWAYS_ON>;
> + resets = <&cpg 816>;
> + status = "disabled";
> + };
> +
> + vin19: video@e6ed3000 {
> + compatible = "renesas,vin-r8a779a0";
> + reg = <0 0xe6ed3000 0 0x1000>;
> + interrupts = ;
> + clocks = <&cpg CPG_MOD 817>;
> + power-domains = <&sysc R8A779A0_PD_ALWAYS_ON>;
> + resets = <&cpg 817>;
> + status = "disabled";
> + };
> +
> + vin20: video@e6ed4000 {
> + compatible = "renesas,vin-r8a779a0";
> + reg = <0 0xe6ed4000 0 0x1000>;
> + interrupts = ;
> + clocks = <&cpg CPG_MOD 818>;
> + power-domains = <&sysc R8A779A0_PD_ALWAYS_ON>;
> + resets = <&cpg 818>;
> + status = "disabled";
> + };
> +
> + vin21: video@e6ed5000 {
> + compatible = "renesas,vin-r8a779a0";
> + reg = <0 0xe6ed5000 0 0x1000>;
> + interrupts = ;
> + clocks = <&cpg CPG_MOD 819>;
> + power-domains = <&sysc R8A779A0_PD_ALWAYS_ON>;
> + resets = <&cpg 819>;
> + status = "disabled";
> + };
> +
> + vin22: video@e6ed6000 {
> + compatible = "renesas,vin-r8a779a0";
> + reg = <0 0xe6ed6000 0 0x1000>;
> + interrupts = ;
> + clocks = <&cpg CPG_MOD 820>;
> + power-domains = <&sysc R8A779A0_PD_ALWAYS_ON>;
> + resets = <&cpg 820>;
> + status = "disabled";
> + };
> +
> + vin23: video@e6ed7000 {
> + compatible = "renesas,vin-r8a779a0";
> + reg = <0 0xe6ed7000 0 0x1000>;
> + interrupts = ;
> + clocks = <&cpg CPG_MOD 821>;
> + power-domains = <&sysc R8A779A0_PD_ALWAYS_ON>;
> + resets = <&cpg 821>;
> + status = "disabled";
> + };
> +
> + vin24: video@e6ed8000 {
> + compatible = "renesas,vin-r8a779a0";
> + reg = <0 0xe6ed8000 0 0x1000>;
> + interrupts = ;
> + clocks = <&cpg CPG_MOD 822>;
> + power-domains = <&sysc R8A779A0_PD_ALWAYS_ON>;
> + resets = <&cpg 822>;
> + status = "disabled";
> + };
> +
> + vin25: video@e6ed9000 {
> + compatible = "renesas,vin-r8a779a0";
> + reg = <0 0xe6ed9000 0 0x1000>;
> + interrupts = ;
> + clocks = <&cpg CPG_MOD 823>;
> + power-domains = <&sysc R8A779A0_PD_ALWAYS_ON>;
> + resets = <&cpg 823>;
> + status = "disabled";
> + };
> +
> + vin26: video@e6eda000 {
> + compatible = "renesas,vin-r8a779a0";
> + reg = <0 0xe6eda000 0 0x1000>;
> + interrupts = ;
> + clocks = <&cpg CPG_MOD 824>;
> + power-domains = <&sysc R8A779A0_PD_ALWAYS_ON>;
> + resets = <&cpg 824>;
> + status = "disab
Re: [PATCH 5/6] dt-bindings: media: renesas,vin: Add V3U support
Hi Jacopo, Thanks for your patch. On 2020-10-14 11:44:42 +0200, Jacopo Mondi wrote: > Add compatible string definition for R-Car V3U. > > Signed-off-by: Jacopo Mondi > --- > Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml > b/Documentation/devicetree/bindings/media/renesas,vin.yaml > index ad2fe660364b..7b629a6ea035 100644 > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml > @@ -51,6 +51,7 @@ properties: >- renesas,vin-r8a77980 # R-Car V3H >- renesas,vin-r8a77990 # R-Car E3 >- renesas,vin-r8a77995 # R-Car D3 > + - renesas,vin-r8a779a0 # R-Car V3U Documenting the compat string here might give the wrong impression the IP is supported. Which it's not as the V3U is quiet different then the it's siblings. Whit a new pipeline and small but important changes in in the register set. I'm primarily thinking about that the IFMD register is gone and dealing with that will be quiet significant work in the driver. I'm not yet 100% sure the best idea is to support V3U with this driver, maybe it's finally time to make it more modular, with more distinct support for Gen2, Gen3 and a new module for V3U? > >reg: > maxItems: 1 > -- > 2.28.0 > -- Regards, Niklas Söderlund
Re: [PATCH] media: rcar-vin: rcar-dma: Fix setting VNIS_REG for RAW8 formats
Hi Lad,
Thanks for catching and fixing this.
On 2020-10-02 11:26:52 +0100, Lad Prabhakar wrote:
> pixelformat in vin priv structure holds V4L2_PIX_FMT_* and not
> MEDIA_BUS_FMT_* so make sure we check against V4L2_PIX_FMT_* formats
> while setting the VNIS_REG.
>
> Fixes: 8c3e0f67df6c9 ("media: rcar-vin: Extend RAW8 support to all RGB
> layouts")
> Signed-off-by: Lad Prabhakar
> Reviewed-by: Biju Das
Reviewed-by: Niklas Söderlund
> ---
> Hi Hans,
>
> If it isn't too late for v5.10 could you please queue up this patch.
>
> Cheers,
> Prabhakar
> ---
> drivers/media/platform/rcar-vin/rcar-dma.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c
> index e9a47b705acc..692dea300b0d 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -600,10 +600,10 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
>* format in 2 pixel unit hence configure VNIS_REG as stride / 2.
>*/
> switch (vin->format.pixelformat) {
> - case MEDIA_BUS_FMT_SBGGR8_1X8:
> - case MEDIA_BUS_FMT_SGBRG8_1X8:
> - case MEDIA_BUS_FMT_SGRBG8_1X8:
> - case MEDIA_BUS_FMT_SRGGB8_1X8:
> + case V4L2_PIX_FMT_SBGGR8:
> + case V4L2_PIX_FMT_SGBRG8:
> + case V4L2_PIX_FMT_SGRBG8:
> + case V4L2_PIX_FMT_SRGGB8:
> stride /= 2;
> break;
> default:
> --
> 2.17.1
>
--
Regards,
Niklas Söderlund
Re: [PATCH v2] of: address: Work around missing device_type property in pcie nodes
Hi Bjorn, On 2020-09-30 17:51:54 -0500, Bjorn Helgaas wrote: > On Wed, Sep 30, 2020 at 03:34:10PM -0500, Rob Herring wrote: > > On Wed, Sep 30, 2020 at 12:37 PM Niklas Söderlund > > wrote: > > > > > > Hi Marc, > > > > > > On 2020-09-30 18:23:21 +0100, Marc Zyngier wrote: > > > > Hi Niklas, > > > > > > > > [+ Samuel] > > > > > > > > On 2020-09-30 17:27, Niklas Söderlund wrote: > > > > > Hi Marc, > > > > > > > > > > I'm afraid this commit breaks booting my rk3399 device. > > > > > > > > > > I bisected the problem to this patch merged as [1]. I'm testing on a > > > > > Scarlet device and I'm using the unmodified upstream > > > > > rk3399-gru-scarlet-inx.dtb for my tests. > > > > > > > > > > The problem I'm experience is a black screen after the bootloader and > > > > > the device is none responsive over the network. I have no serial > > > > > console > > > > > to this device so I'm afraid I can't tell you if there is anything > > > > > useful on to aid debugging there. > > > > > > > > > > If I try to test one commit earlier [2] the system boots as expected > > > > > and > > > > > everything works as it did for me in v5.8 and earlier. I have worked > > > > > little with this device and have no clue about what is really on the > > > > > PCI > > > > > buss. But running from [2] I have this info about PCI if it's helpful, > > > > > please ask if somethings missing. > > > > > > > > Please see the thread at [1]. The problem was reported a few weeks back > > > > by Samuel, and I was expecting Rob and Lorenzo to push a fix for this. > > > > > > Thanks for providing a solution. > > > > > > > > > > > Rob, Lorenzo, any update on this? > > > > The fix is in Bjorn's tree[1]. > > > > Bjorn, going to send this to Linus before v5.9 is out? > > Definitely, thanks for the reminder. I'm assuming the fix in question > is > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=for-linus&id=e338eecf3fe79054e8a31b8c39a1234b5acfdabe That patch solves my boot problem. Cherry-picked and tested directly on-top of v5.9-rc7 (which fails to boot without the patch in question). > > > [1] > > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=for-linus -- Regards, Niklas Söderlund
Re: [PATCH v2] of: address: Work around missing device_type property in pcie nodes
Hi Marc, On 2020-09-30 18:23:21 +0100, Marc Zyngier wrote: > Hi Niklas, > > [+ Samuel] > > On 2020-09-30 17:27, Niklas Söderlund wrote: > > Hi Marc, > > > > I'm afraid this commit breaks booting my rk3399 device. > > > > I bisected the problem to this patch merged as [1]. I'm testing on a > > Scarlet device and I'm using the unmodified upstream > > rk3399-gru-scarlet-inx.dtb for my tests. > > > > The problem I'm experience is a black screen after the bootloader and > > the device is none responsive over the network. I have no serial console > > to this device so I'm afraid I can't tell you if there is anything > > useful on to aid debugging there. > > > > If I try to test one commit earlier [2] the system boots as expected and > > everything works as it did for me in v5.8 and earlier. I have worked > > little with this device and have no clue about what is really on the PCI > > buss. But running from [2] I have this info about PCI if it's helpful, > > please ask if somethings missing. > > Please see the thread at [1]. The problem was reported a few weeks back > by Samuel, and I was expecting Rob and Lorenzo to push a fix for this. Thanks for providing a solution. > > Rob, Lorenzo, any update on this? > > M. > > [1] > https://lore.kernel.org/linux-devicetree/20200829164920.7d28e01a@DUFFMAN/ > -- > Jazz is not dead. It just smells funny... -- Regards, Niklas Söderlund
Re: [PATCH v2] of: address: Work around missing device_type property in pcie nodes
Hi Marc,
I'm afraid this commit breaks booting my rk3399 device.
I bisected the problem to this patch merged as [1]. I'm testing on a
Scarlet device and I'm using the unmodified upstream
rk3399-gru-scarlet-inx.dtb for my tests.
The problem I'm experience is a black screen after the bootloader and
the device is none responsive over the network. I have no serial console
to this device so I'm afraid I can't tell you if there is anything
useful on to aid debugging there.
If I try to test one commit earlier [2] the system boots as expected and
everything works as it did for me in v5.8 and earlier. I have worked
little with this device and have no clue about what is really on the PCI
buss. But running from [2] I have this info about PCI if it's helpful,
please ask if somethings missing.
# dmesg | grep -i pci
[0.003943] PCI/MSI:
/interrupt-controller@fee0/interrupt-controller@fee2 domain created
[0.922022] PCI: CLS 0 bytes, default 64
[0.941517] rockchip-pcie f800.pcie: host bridge /pcie@f800 ranges:
[0.941577] rockchip-pcie f800.pcie: MEM 0x00fa00..0x00fbef
-> 0x00fa00
[0.941962] rockchip-pcie f800.pcie: GPIO lookup for consumer ep
[0.941981] rockchip-pcie f800.pcie: using device tree for GPIO lookup
[0.942018] of_get_named_gpiod_flags: parsed 'ep-gpios' property of node
'/pcie@f800[0]' - status (0)
[0.942255] rockchip-pcie f800.pcie: no vpcie12v regulator found
[4.196248] ehci-pci: EHCI PCI platform driver
[4.214639] ohci-pci: OHCI PCI platform driver
# ls /sys/bus/{pci,pci_express}/devices
/sys/bus/pci/devices:
/sys/bus/pci_express/devices:
# ls /sys/bus/{pci,pci_express}/drivers
/sys/bus/pci/drivers:
cavium_rng_pf cavium_rng_vf dwc3-haps ehci-pci exar_serial ohci-pci
pcieport serial xhci_hcd
/sys/bus/pci_express/drivers:
pcie_pme
# ls /sys/bus/platform/drivers/rockchip-{pcie,pcie-phy}
/sys/bus/platform/drivers/rockchip-pcie:
bind uevent unbind
/sys/bus/platform/drivers/rockchip-pcie-phy:
bind ff77.syscon:pcie-phy uevent unbind
1. d1ac0002dd297069 ("of: address: Work around missing device_type property in
pcie nodes")
2. 43647929175e2cd3 ("dt: writing-schema: Miscellaneous grammar fixes")
On 2020-08-19 10:42:55 +0100, Marc Zyngier wrote:
> Recent changes to the DT PCI bus parsing made it mandatory for
> device tree nodes describing a PCI controller to have the
> 'device_type = "pci"' property for the node to be matched.
>
> Although this follows the letter of the specification, it
> breaks existing device-trees that have been working fine
> for years. Rockchip rk3399-based systems are a prime example
> of such collateral damage, and have stopped discovering their
> PCI bus.
>
> In order to paper over it, let's add a workaround to the code
> matching the device type, and accept as PCI any node that is
> named "pcie",
>
> A warning will hopefully nudge the user into updating their
> DT to a fixed version if they can, but the incentive is
> obviously pretty small.
>
> Fixes: 2f96593ecc37 ("of_address: Add bus type match for pci ranges parser")
> Suggested-by: Rob Herring
> Signed-off-by: Marc Zyngier
> ---
> drivers/of/address.c | 16 +++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 590493e04b01..b37bd9cc2810 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -128,15 +128,29 @@ static unsigned int of_bus_pci_get_flags(const __be32
> *addr)
> * PCI bus specific translator
> */
>
> +static bool of_node_is_pcie(struct device_node *np)
> +{
> + bool is_pcie = of_node_name_eq(np, "pcie");
> +
> + if (is_pcie)
> + pr_warn_once("%pOF: Missing device_type\n", np);
> +
> + return is_pcie;
> +}
> +
> static int of_bus_pci_match(struct device_node *np)
> {
> /*
>* "pciex" is PCI Express
>* "vci" is for the /chaos bridge on 1st-gen PCI powermacs
>* "ht" is hypertransport
> + *
> + * If none of the device_type match, and that the node name is
> + * "pcie", accept the device as PCI (with a warning).
>*/
> return of_node_is_type(np, "pci") || of_node_is_type(np, "pciex") ||
> - of_node_is_type(np, "vci") || of_node_is_type(np, "ht");
> + of_node_is_type(np, "vci") || of_node_is_type(np, "ht") ||
> + of_node_is_pcie(np);
> }
>
> static void of_bus_pci_count_cells(struct device_node *np,
> --
> 2.27.0
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Regards,
Niklas Söderlund
Re: [PATCH v3] media: rcar-vin: Enable YDS bit depending on bus_width and data_shift
;parallel->mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
> >> + if (!(vin->parallel->bus.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
> >>dmr2 |= VNDMR2_VPS;
> >>
> >>/* Data Enable Polarity Select */
> >> - if (vin->parallel->mbus_flags & V4L2_MBUS_DATA_ENABLE_LOW)
> >> + if (vin->parallel->bus.flags & V4L2_MBUS_DATA_ENABLE_LOW)
> >>dmr2 |= VNDMR2_CES;
> >> +
> >> + switch (vin->mbus_code) {
> >> + case MEDIA_BUS_FMT_UYVY8_2X8:
> >> + if (vin->parallel->bus.bus_width == 8 &&
> >> + vin->parallel->bus.data_shift == 8)
> >> + dmr2 |= VNDMR2_YDS;
> >> + break;
> >> + default:
> >> + break;
> >> + }
> >>}
> >>
> >>/*
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> >> b/drivers/media/platform/rcar-vin/rcar-vin.h
> >> index c19d077ce1cb..8396e0e45478 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> >> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> >> @@ -19,6 +19,7 @@
> >> #include
> >> #include
> >> #include
> >> +#include
> >> #include
> >>
> >> /* Number of HW buffers */
> >> @@ -92,7 +93,7 @@ struct rvin_video_format {
> >> * @asd: sub-device descriptor for async framework
> >> * @subdev: subdevice matched using async framework
> >> * @mbus_type:media bus type
> >> - * @mbus_flags: media bus configuration flags
> >> + * @bus: media bus parallel configuration
> >> * @source_pad: source pad of remote subdevice
> >> * @sink_pad: sink pad of remote subdevice
> >> *
> >> @@ -102,7 +103,7 @@ struct rvin_parallel_entity {
> >>struct v4l2_subdev *subdev;
> >>
> >>enum v4l2_mbus_type mbus_type;
> >> - unsigned int mbus_flags;
> >> + struct v4l2_fwnode_bus_parallel bus;
> >>
> >>unsigned int source_pad;
> >>unsigned int sink_pad;
> >>
> >
>
--
Regards,
Niklas Söderlund
Re: [PATCH v2] media: rcar-vin: Update crop and compose settings for every s_fmt call
{
> struct rvin_dev *vin = video_drvdata(file);
> - struct v4l2_rect fmt_rect, src_rect;
> + struct v4l2_rect src_rect;
> int ret;
>
> if (vb2_is_busy(&vin->queue))
> @@ -317,14 +317,11 @@ static int rvin_s_fmt_vid_cap(struct file *file, void
> *priv,
> return ret;
>
> vin->format = f->fmt.pix;
> -
> - fmt_rect.top = 0;
> - fmt_rect.left = 0;
> - fmt_rect.width = vin->format.width;
> - fmt_rect.height = vin->format.height;
> -
> - v4l2_rect_map_inside(&vin->crop, &src_rect);
> - v4l2_rect_map_inside(&vin->compose, &fmt_rect);
> + vin->crop.top = 0;
> + vin->crop.left = 0;
> + vin->crop.width = vin->format.width;
> + vin->crop.height = vin->format.height;
> + vin->compose = vin->crop;
> vin->src_rect = src_rect;
>
> return 0;
> --
> 2.17.1
>
--
Regards,
Niklas Söderlund
Re: [PATCH v3] media: rcar-vin: Enable YDS bit depending on bus_width and data_shift
platform/rcar-vin/rcar-vin.h
> > b/drivers/media/platform/rcar-vin/rcar-vin.h
> > index c19d077ce1cb..8396e0e45478 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -19,6 +19,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > /* Number of HW buffers */
> > @@ -92,7 +93,7 @@ struct rvin_video_format {
> > * @asd: sub-device descriptor for async framework
> > * @subdev:subdevice matched using async framework
> > * @mbus_type: media bus type
> > - * @mbus_flags:media bus configuration flags
> > + * @bus: media bus parallel configuration
> > * @source_pad:source pad of remote subdevice
> > * @sink_pad: sink pad of remote subdevice
> > *
> > @@ -102,7 +103,7 @@ struct rvin_parallel_entity {
> > struct v4l2_subdev *subdev;
> >
> > enum v4l2_mbus_type mbus_type;
> > - unsigned int mbus_flags;
> > + struct v4l2_fwnode_bus_parallel bus;
I think you could break this change (and the fallout) out to a separate
patch to make the functional change clearer.
> >
> > unsigned int source_pad;
> > unsigned int sink_pad;
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
Re: [PATCH v2] pinctrl: sh-pfc: r8a7790: Add VIN1-B and VIN2-G pins, groups and functions
Hi Geert, Lad,
On 2020-09-14 16:47:27 +0200, Geert Uytterhoeven wrote:
> Hi Prabhakar,
>
> CC Laurent, Niklas
>
> On Sun, Sep 13, 2020 at 8:29 PM Lad Prabhakar
> wrote:
> > Add pins, groups and functions for the VIN1-B [data/clk/sync] and VIN2-G
> > [data].
> >
> > Signed-off-by: Lad Prabhakar
> > Reviewed-by: Biju Das
> > ---
> > Changes for v2:
> > * Added complete list of VIN1-B pins
> > * Renamed vin2_data8_g to vin2_data8g
> > * Sorted vin1_sync_b pins
> >
> > v1 - https://patchwork.kernel.org/patch/11761191/
>
> Thanks for the update!
>
> > --- a/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
> > +++ b/drivers/pinctrl/sh-pfc/pfc-r8a7790.c
>
> > @@ -3874,6 +3940,14 @@ static const unsigned int vin1_sync_mux[] = {
> > VI1_HSYNC_N_MARK,
> > VI1_VSYNC_N_MARK,
> > };
> > +static const unsigned int vin1_sync_b_pins[] = {
> > + RCAR_GP_PIN(1, 24), /* HSYNC */
> > + RCAR_GP_PIN(1, 25), /* VSYNC */
> > +};
> > +static const unsigned int vin1_sync_b_mux[] = {
> > + VI1_HSYNC_N_B_MARK,
> > + VI1_VSYNC_N_B_MARK,
> > +};
> > static const unsigned int vin1_field_pins[] = {
> > RCAR_GP_PIN(1, 13),
> > };
>
> Missing field_b and clkenb_b.
>
> > @@ -3959,6 +4039,18 @@ static const unsigned int vin2_data18_mux[] = {
> > VI2_R4_MARK, VI2_R5_MARK,
> > VI2_R6_MARK, VI2_R7_MARK,
> > };
> > +static const unsigned int vin2_data8g_pins[] = {
> > + RCAR_GP_PIN(0, 27), RCAR_GP_PIN(0, 28),
> > + RCAR_GP_PIN(0, 29), RCAR_GP_PIN(1, 10),
> > + RCAR_GP_PIN(1, 4), RCAR_GP_PIN(1, 5),
> > + RCAR_GP_PIN(1, 6), RCAR_GP_PIN(1, 7),
> > +};
> > +static const unsigned int vin2_data8g_mux[] = {
> > + VI2_G0_MARK, VI2_G1_MARK,
> > + VI2_G2_MARK, VI2_G3_MARK,
> > + VI2_G4_MARK, VI2_G5_MARK,
> > + VI2_G6_MARK, VI2_G7_MARK,
> > +};
>
> Laurent, Niklas: are you happy with the name "vin2_data8g", or do
> you have a better suggestion?
I learnt recently that traditionally for single 8-bit RAW inputs are
named R8 (fist in RGB). But as this really is the G pins and they are
labeled as such I'm OK with the name, but I would like to hear Laurent's
view as well.
>
> > static const unsigned int vin2_sync_pins[] = {
> > RCAR_GP_PIN(1, 16), /* HSYNC */
> > RCAR_GP_PIN(1, 21), /* VSYNC */
>
> > @@ -4310,15 +4402,25 @@ static const struct {
> > VIN_DATA_PIN_GROUP(vin1_data, 10),
> > VIN_DATA_PIN_GROUP(vin1_data, 8),
> > VIN_DATA_PIN_GROUP(vin1_data, 4),
> > + VIN_DATA_PIN_GROUP(vin1_data, 24, _b),
> > + VIN_DATA_PIN_GROUP(vin1_data, 20, _b),
> > + SH_PFC_PIN_GROUP(vin1_data18_b),
> > + VIN_DATA_PIN_GROUP(vin1_data, 16, _b),
> > + VIN_DATA_PIN_GROUP(vin1_data, 12, _b),
> > + VIN_DATA_PIN_GROUP(vin1_data, 10, _b),
> > + VIN_DATA_PIN_GROUP(vin1_data, 8, _b),
>
> Missing vin1_data4_b.
>
> > SH_PFC_PIN_GROUP(vin1_sync),
> > + SH_PFC_PIN_GROUP(vin1_sync_b),
> > SH_PFC_PIN_GROUP(vin1_field),
> > SH_PFC_PIN_GROUP(vin1_clkenb),
> > SH_PFC_PIN_GROUP(vin1_clk),
> > + SH_PFC_PIN_GROUP(vin1_clk_b),
> > VIN_DATA_PIN_GROUP(vin2_data, 24),
> > SH_PFC_PIN_GROUP(vin2_data18),
> > VIN_DATA_PIN_GROUP(vin2_data, 16),
> > VIN_DATA_PIN_GROUP(vin2_data, 8),
> > VIN_DATA_PIN_GROUP(vin2_data, 4),
> > + SH_PFC_PIN_GROUP(vin2_data8g),
> > SH_PFC_PIN_GROUP(vin2_sync),
> > SH_PFC_PIN_GROUP(vin2_field),
> > SH_PFC_PIN_GROUP(vin2_clkenb),
> > @@ -4784,10 +4886,19 @@ static const char * const vin1_groups[] = {
> > "vin1_data10",
> > "vin1_data8",
> > "vin1_data4",
> > + "vin1_data24_b",
> > + "vin1_data20_b",
> > + "vin1_data18_b",
> > + "vin1_data16_b",
> > + "vin1_data12_b",
> > + "vin1_data10_b",
> > + "vin1_data8_b",
>
> Missing vin1_data4_b.
>
> > "vin1_sync",
> > + "vin1_sync_b",
> > "vin1_field",
> > "vin1_clkenb",
> > "vin1_clk",
> > + "vin1_clk_b",
> > };
> >
> > static const char * const vin2_groups[] = {
>
> The rest looks good to me.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@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
--
Regards,
Niklas Söderlund
Re: [PATCH] thermal: Kconfig: Update description for RCAR_GEN3_THERMAL config
Hi Lad, Thanks for your work. On 2020-09-11 11:01:48 +0100, Lad Prabhakar wrote: > rcar_gen3_thermal driver is also used on Renesas RZ/G2 SoC's, update the > same to reflect the description for RCAR_GEN3_THERMAL config. Same comment about the commit message as for the VIN and CSI2 series with same commit message. With that fixed, Reviewed-by: Niklas Söderlund > > Signed-off-by: Lad Prabhakar > Reviewed-by: Chris Paterson > --- > drivers/thermal/Kconfig | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > index b668224f906d..7edc8dc6bbab 100644 > --- a/drivers/thermal/Kconfig > +++ b/drivers/thermal/Kconfig > @@ -346,13 +346,13 @@ config RCAR_THERMAL > thermal framework. > > config RCAR_GEN3_THERMAL > - tristate "Renesas R-Car Gen3 thermal driver" > + tristate "Renesas R-Car Gen3 and RZ/G2 thermal driver" > depends on ARCH_RENESAS || COMPILE_TEST > depends on HAS_IOMEM > depends on OF > help > - Enable this to plug the R-Car Gen3 thermal sensor driver into the > Linux > - thermal framework. > + Enable this to plug the R-Car Gen3 or RZ/G2 thermal sensor driver into > + the Linux thermal framework. > > config KIRKWOOD_THERMAL > tristate "Temperature sensor on Marvell Kirkwood SoCs" > -- > 2.17.1 > -- Regards, Niklas Söderlund
Re: [PATCH 1/2] media: rcar-vin: Kconfig: Update help description for VIDEO_RCAR_CSI2 config
Hi Lad, Thanks for your patch. On 2020-09-11 11:14:21 +0100, Lad Prabhakar wrote: > rcar-csi2 driver is also used on Renesas RZ/G2 SoC's, update the > same to reflect the description for VIDEO_RCAR_CSI2 config. The commit message in this and 2/2 are a bit inconsistent, how about something like this? The rcar-csi2 driver also supports RZ/G2 SoC's, update the description to reflect this. With the commit message fixed for both patches, Reviewed-by: Niklas Söderlund > > Signed-off-by: Lad Prabhakar > Reviewed-by: Chris Paterson > --- > drivers/media/platform/rcar-vin/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/rcar-vin/Kconfig > b/drivers/media/platform/rcar-vin/Kconfig > index ca0d906dce2f..2669c13a0423 100644 > --- a/drivers/media/platform/rcar-vin/Kconfig > +++ b/drivers/media/platform/rcar-vin/Kconfig > @@ -9,7 +9,7 @@ config VIDEO_RCAR_CSI2 > select V4L2_FWNODE > help > Support for Renesas R-Car MIPI CSI-2 receiver. > - Supports R-Car Gen3 SoCs. > + Supports R-Car Gen3 and RZ/G2 SoCs. > > To compile this driver as a module, choose M here: the > module will be called rcar-csi2. > -- > 2.17.1 > -- Regards, Niklas Söderlund
Re: [PATCH 0/2] Add VIN instances to r8a7742 SoC
Hi Lad, Thanks for your work. On 2020-09-07 15:45:07 +0100, Lad Prabhakar wrote: > Hi All, > > This patch series adds support for VIN to r8a7742 (RZ/G1H) SoC. > > Patches apply on top of [1]. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/geert/ > renesas-devel.git/log/?h=renesas-arm-dt-for-v5.10 For the whole series, Reviewed-by: Niklas Söderlund > > Cheers, > Prabhakar > > Lad Prabhakar (2): > dt-bindings: media: renesas,vin: Add device tree support for r8a7742 > ARM: dts: r8a7742: Add VIN DT nodes > > .../bindings/media/renesas,vin.yaml | 1 + > arch/arm/boot/dts/r8a7742.dtsi| 44 +++ > 2 files changed, 45 insertions(+) > > -- > 2.17.1 > -- Regards, Niklas Söderlund
Re: [PATCH 20/20] arm64: dts: renesas: r8a774e1: Add VIN and CSI-2 nodes
Hi Geert and Lad,
On 2020-08-07 13:36:46 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
>
> On Fri, Aug 7, 2020 at 1:27 PM Niklas Söderlund
> wrote:
> > On 2020-08-06 13:47:58 +0200, Geert Uytterhoeven wrote:
> > > On Thu, Aug 6, 2020 at 1:17 PM Lad, Prabhakar
> > > wrote:
> > > > On Wed, Aug 5, 2020 at 12:19 PM Geert Uytterhoeven
> > > > wrote:
> > > > > On Thu, Jul 16, 2020 at 7:20 PM Lad Prabhakar
> > > > > wrote:
> > > > > > Add VIN and CSI-2 nodes to RZ/G2H (R8A774E1) SoC dtsi.
> > > > > >
> > > > > > Signed-off-by: Lad Prabhakar
> > > > > >
> > > > > > Reviewed-by: Marian-Cristian Rotariu
> > > > > >
> > > > >
> > > > > Reviewed-by: Geert Uytterhoeven
> > > > >
> > > > > However, before I queue this in renesas-devel for v5.10, I'd like to
> > > > > have some clarification about the issue below.
> > > > >
> > > > > > --- a/arch/arm64/boot/dts/renesas/r8a774e1.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/renesas/r8a774e1.dtsi
> > > > >
> > > > > > + vin4: video@e6ef4000 {
> > > > > > + compatible = "renesas,vin-r8a774e1";
> > > > > > + reg = <0 0xe6ef4000 0 0x1000>;
> > > > > > + interrupts = > > > > > IRQ_TYPE_LEVEL_HIGH>;
> > > > > > + clocks = <&cpg CPG_MOD 807>;
> > > > > > + power-domains = <&sysc
> > > > > > R8A774E1_PD_ALWAYS_ON>;
> > > > > > + resets = <&cpg 807>;
> > > > > > + renesas,id = <4>;
> > > > > > + status = "disabled";
> > > > > > +
> > > > > > + ports {
> > > > > > + #address-cells = <1>;
> > > > > > + #size-cells = <0>;
> > > > > > +
> > > > > > + port@1 {
> > > > > > + #address-cells = <1>;
> > > > > > + #size-cells = <0>;
> > > > >
> > > > > "make dtbs W=1" says:
> > > > >
> > > > > arch/arm64/boot/dts/renesas/r8a774e1.dtsi:1562.12-1572.7: Warning
> > > > > (graph_child_address): /soc/video@e6ef4000/ports/port@1: graph node
> > > > > has single child node 'endpoint@0', #address-cells/#size-cells are not
> > > > > necessary
> > > > >
> > > > > (same for vin5-7 below)
> > > > >
> > > > Referring to commit 5e53dbf4edb4d ("arm64: dts: renesas: r8a77990: Fix
> > > > VIN endpoint numbering") we definitely need endpoint numbering.
> > > > Probably the driver needs to be fixed to handle such cases.
> > >
> > > > > > +
> > > > > > + reg = <1>;
> > > > > > +
> > > > > > + vin4csi20: endpoint@0 {
> > > > > > + reg = <0>;
> > > > > > + remote-endpoint =
> > > > > > <&csi20vin4>;
> > >
> > > On R-Car E3, the single endpoint is at address 2, so "make dtbs
> > > W=1"doesn't
> > > complain. Here it is at address 0.
> > >
> > > Niklas?
> >
> > First the R-Car VIN driver makes decisions based on which endpoint is
> > described, each endpoint 0-3 represents a different CSI-2 block on the
> > other end (0: CSI20, 1: CSI21, 2: CSI40 and 3: CSI41).
>
> That's my understanding, too.
>
> > Then how to handle the warning I'm not sure. I can only really see 2
> > options.
> >
> > 1. Ignore the warning.
> > 2. Remove #address-cells, #size-cells and reg properties from port@ if
> >the only endpoint described is endpoint@0.
> >
> > I would prefers option 2. that is what we do in other cases (for example
> > on Gen2 boards that only have a single parallel sensor in some early DTS
> > files we don't have the ports node and just describe a single port with
> > the same reasoning.
> >
> > We are not at risk at someone describing a second CSI-2 bock as an
> > overlay so I see no real harm in option 2.
>
> Yeah, no overlay possible for on-SoC wiring ;-)
>
> > What are your thoughts Geert?
> > You know more about DT then me.
>
> You have too much faith in me ;-)
>
> AFAIK we don't get this warning for e.g. SPI buses, which can have a
> single device at address 0, and #{address,size}-cells is mandatory
> there. So endpoints (or SPI?) are treated special?
That is a good question, I don't know if either of those are treated
special. Lad could you look into this?
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@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
--
Regards,
Niklas Söderlund
Re: [PATCH 20/20] arm64: dts: renesas: r8a774e1: Add VIN and CSI-2 nodes
Hi Geert, Lad,
On 2020-08-06 13:47:58 +0200, Geert Uytterhoeven wrote:
> Hi Prabhakar,
>
> On Thu, Aug 6, 2020 at 1:17 PM Lad, Prabhakar
> wrote:
> > On Wed, Aug 5, 2020 at 12:19 PM Geert Uytterhoeven
> > wrote:
> > > On Thu, Jul 16, 2020 at 7:20 PM Lad Prabhakar
> > > wrote:
> > > > Add VIN and CSI-2 nodes to RZ/G2H (R8A774E1) SoC dtsi.
> > > >
> > > > Signed-off-by: Lad Prabhakar
> > > > Reviewed-by: Marian-Cristian Rotariu
> > > >
> > >
> > > Reviewed-by: Geert Uytterhoeven
> > >
> > > However, before I queue this in renesas-devel for v5.10, I'd like to
> > > have some clarification about the issue below.
> > >
> > > > --- a/arch/arm64/boot/dts/renesas/r8a774e1.dtsi
> > > > +++ b/arch/arm64/boot/dts/renesas/r8a774e1.dtsi
> > >
> > > > + vin4: video@e6ef4000 {
> > > > + compatible = "renesas,vin-r8a774e1";
> > > > + reg = <0 0xe6ef4000 0 0x1000>;
> > > > + interrupts = ;
> > > > + clocks = <&cpg CPG_MOD 807>;
> > > > + power-domains = <&sysc R8A774E1_PD_ALWAYS_ON>;
> > > > + resets = <&cpg 807>;
> > > > + renesas,id = <4>;
> > > > + status = "disabled";
> > > > +
> > > > + ports {
> > > > + #address-cells = <1>;
> > > > + #size-cells = <0>;
> > > > +
> > > > + port@1 {
> > > > + #address-cells = <1>;
> > > > + #size-cells = <0>;
> > >
> > > "make dtbs W=1" says:
> > >
> > > arch/arm64/boot/dts/renesas/r8a774e1.dtsi:1562.12-1572.7: Warning
> > > (graph_child_address): /soc/video@e6ef4000/ports/port@1: graph node
> > > has single child node 'endpoint@0', #address-cells/#size-cells are not
> > > necessary
> > >
> > > (same for vin5-7 below)
> > >
> > Referring to commit 5e53dbf4edb4d ("arm64: dts: renesas: r8a77990: Fix
> > VIN endpoint numbering") we definitely need endpoint numbering.
> > Probably the driver needs to be fixed to handle such cases.
>
> > > > +
> > > > + reg = <1>;
> > > > +
> > > > + vin4csi20: endpoint@0 {
> > > > + reg = <0>;
> > > > + remote-endpoint =
> > > > <&csi20vin4>;
>
> On R-Car E3, the single endpoint is at address 2, so "make dtbs W=1"doesn't
> complain. Here it is at address 0.
>
> Niklas?
First the R-Car VIN driver makes decisions based on which endpoint is
described, each endpoint 0-3 represents a different CSI-2 block on the
other end (0: CSI20, 1: CSI21, 2: CSI40 and 3: CSI41).
Then how to handle the warning I'm not sure. I can only really see 2
options.
1. Ignore the warning.
2. Remove #address-cells, #size-cells and reg properties from port@ if
the only endpoint described is endpoint@0.
I would prefers option 2. that is what we do in other cases (for example
on Gen2 boards that only have a single parallel sensor in some early DTS
files we don't have the ports node and just describe a single port with
the same reasoning.
We are not at risk at someone describing a second CSI-2 bock as an
overlay so I see no real harm in option 2. What are your thoughts Geert?
You know more about DT then me.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@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
--
Regards,
Niklas Söderlund
Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor
Hi Kaaira, Thanks for your work. On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote: > This is version 2 of the patch series posted by Niklas for allowing > multiple streams in VIMC. > The original series can be found here: > https://patchwork.kernel.org/cover/10948831/ > > This series adds support for two (or more) capture devices to be > connected to the same sensors and run simultaneously. Each capture device > can be started and stopped independent of each other. > > Patch 1/3 and 2/3 deals with solving the issues that arises once two > capture devices can be part of the same pipeline. While 3/3 allows for > two capture devices to be part of the same pipeline and thus allows for > simultaneously use. I'm just curious if you are aware of this series? It would replace the need for 1/3 and 2/3 of this series right? 1. https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschf...@collabora.com/ > > Changes since v1: > - All three patches rebased on latest media-tree. > Patch 3: > - Search for an entity with a non-NULL pipe instead of searching > for sensor. This terminates the search at output itself. > > Kaaira Gupta (3): > media: vimc: Add usage count to subdevices > media: vimc: Serialize vimc_streamer_s_stream() > media: vimc: Join pipeline if one already exists > > .../media/test-drivers/vimc/vimc-capture.c| 35 ++- > .../media/test-drivers/vimc/vimc-debayer.c| 8 + > drivers/media/test-drivers/vimc/vimc-scaler.c | 8 + > drivers/media/test-drivers/vimc/vimc-sensor.c | 9 - > .../media/test-drivers/vimc/vimc-streamer.c | 23 +++- > 5 files changed, 73 insertions(+), 10 deletions(-) > > -- > 2.17.1 > -- Regards, Niklas Söderlund
Re: [PATCH] media: rcar-vin: Move media_device_register to async completion
ice_initialize+0x40/0x150 [ 51.052396] device_register+0x14/0x30 [ 51.057554] __video_register_device+0xcf4/0x1780 [ 51.063674] rvin_v4l2_register+0xe4/0x150 [ 51.069182] rvin_group_notify_complete+0x58/0x18c [ 51.075408] v4l2_async_notifier_try_complete.part.0+0x50/0x60 [ 51.082676] v4l2_async_register_subdev+0xfc/0x1b0 [ 51.088857] rcsi2_probe+0x338/0x41c [ 51.093772] platform_drv_probe+0x50/0xa0 [ 51.099084] really_probe+0xdc/0x430 [ 51.103928] driver_probe_device+0x54/0xb0 [ 51.109299] device_driver_attach+0xbc/0xc4 [ 51.114766] bind_store+0xb8/0x11c [ 51.119453] drv_attr_store+0x20/0x30 [ 51.124408] sysfs_kf_write+0x48/0x70 [ 51.129353] kernfs_fop_write+0x120/0x230 [ 51.134656] __vfs_write+0x18/0x40 [ 51.139336] vfs_write+0xdc/0x1c4 [ 51.143921] ksys_write+0x68/0xf0 [ 51.148496] __arm64_sys_write+0x18/0x20 [ 51.153678] el0_svc_common.constprop.0+0x70/0x170 [ 51.159735] do_el0_svc+0x20/0x80 [ 51.164300] el0_sync_handler+0x94/0x1d0 [ 51.169479] el0_sync+0x140/0x180 [ 51.175178] Unable to handle kernel NULL pointer dereference at virtual address [ 51.185832] Mem abort info: [ 51.190414] ESR = 0x9644 [ 51.195202] EC = 0x25: DABT (current EL), IL = 32 bits [ 51.204679] SET = 0, FnV = 0 [ 51.209464] EA = 0, S1PTW = 0 [ 51.214841] Data abort info: [ 51.219542] ISV = 0, ISS = 0x0044 [ 51.229557] CM = 0, WnR = 1 [ 51.235004] user pgtable: 4k pages, 48-bit VAs, pgdp=a9422000 [ 51.243309] [] pgd=, p4d= [ 51.252227] Internal error: Oops: 9644 [#1] PREEMPT SMP [ 51.259230] CPU: 0 PID: 399 Comm: bash Not tainted 5.8.0-rc1-arm64-renesas-00122-g6f01dfb760c027d5 #1 [ 51.269914] Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT) [ 51.279327] pstate: 6005 (nZCv daif -PAN -UAO BTYPE=--) [ 51.286391] pc : media_create_intf_link+0x50/0xc0 [ 51.292542] lr : media_create_intf_link+0x34/0xc0 [ 51.298631] sp : 800012ecba10 [ 51.303282] x29: 800012ecba10 x28: 735cd250 [ 51.309923] x27: 800011ea54c8 x26: 735cd098 [ 51.316557] x25: 800010f30a60 x24: 0010 [ 51.323192] x23: 0001 x22: [ 51.329831] x21: 735cd098 x20: 0003 [ 51.336468] x19: 7cc5f600 x18: 005a215e8dc3 [ 51.343101] x17: 00016a37eb2cda4e x16: 0014 [ 51.349741] x15: 800010c06f60 x14: 0005 [ 51.356376] x13: 026a x12: 8000112c5c28 [ 51.363006] x11: 007e x10: 0001 [ 51.369628] x9 : x8 : 7cc5f680 [ 51.376237] x7 : x6 : 003f [ 51.382831] x5 : 0040 x4 : 0dc0 [ 51.389407] x3 : x2 : 1003 [ 51.395973] x1 : 7cc5f620 x0 : 7cc5f600 [ 51.402539] Call trace: [ 51.406223] media_create_intf_link+0x50/0xc0 [ 51.411843] __video_register_device+0xffc/0x1780 [ 51.417814] rvin_v4l2_register+0xe4/0x150 [ 51.423176] rvin_group_notify_complete+0x58/0x18c [ 51.429252] v4l2_async_notifier_try_complete.part.0+0x50/0x60 [ 51.436390] v4l2_async_register_subdev+0xfc/0x1b0 [ 51.442479] rcsi2_probe+0x338/0x41c [ 51.447356] platform_drv_probe+0x50/0xa0 [ 51.452672] really_probe+0xdc/0x430 [ 51.457550] driver_probe_device+0x54/0xb0 [ 51.462944] device_driver_attach+0xbc/0xc4 [ 51.468426] bind_store+0xb8/0x11c [ 51.473116] drv_attr_store+0x20/0x30 [ 51.478077] sysfs_kf_write+0x48/0x70 [ 51.482993] kernfs_fop_write+0x120/0x230 [ 51.488229] __vfs_write+0x18/0x40 [ 51.492852] vfs_write+0xdc/0x1c4 [ 51.497375] ksys_write+0x68/0xf0 [ 51.501885] __arm64_sys_write+0x18/0x20 [ 51.507019] el0_svc_common.constprop.0+0x70/0x170 [ 51.513037] do_el0_svc+0x20/0x80 [ 51.517573] el0_sync_handler+0x94/0x1d0 [ 51.522722] el0_sync+0x140/0x180 [ 51.527237] Code: 91008262 a9020c02 32040282 f9001661 (f961) [ 51.534555] ---[ end trace 94f72cbb1552c089 ]--- -- Regards, Niklas Söderlund
Re: [PATCH 1/2] [RFC] media: rcar-vin: send a V4L2 event to vdev if no frame captured after a timeout
Hi Michael,
On 2020-07-02 14:33:41 +0200, Michael Rodin wrote:
> Hi Niklas,
>
> On Wed, Jul 01, 2020 at 12:17:10AM +0200, Niklas Söderlund wrote:
> > Hi Michael,
> >
> > Thanks for your RFC.
>
> Thanks your your feedback!
>
> > On 2020-06-19 19:46:10 +0200, Michael Rodin wrote:
> > > Data flow from an upstream subdevice can stop permanently due to:
> > > - CSI2 transmission errors
> > > - silent failure of the source subdevice
> > > - disconnection of the source subdevice
> > > In those cases userspace waits for new buffers for an infinitely long
> > > time.
> > > In order to address this issue, use a timer to monitor, that rvin_irq() is
> > > capturing at least one frame within a IRQ_TIMEOUT_MS period. Otherwise
> > > send
> > > a new private v4l2 event to userspace. This event is exported to userspace
> > > via a new uapi header.
> >
> > I think there is value for user-space to detecting the error cases
> > above. But I think the problem could be addressed at a different lever.
> > Defining a VIN specific events and controls for something that applies
> > any video device might not be the neatest solution.
> >
> > Another thing hits me when reading this series, could this not be done
> > in user-space? In 2/2 you add a control which sets the timeout based on
> > the framerate, so user-space must know about that to be able to set the
> > control. User-space also knows when it receives/dequeus a buffer from
> > the video device so the timeout logic could be implemented in the
> > application. Given that the application anyhow needs special care to
> > handle the VIN specific event and control I wonder if it's not neater to
> > make it handle all of it. Do you see any specific benefit of having
> > parts of it in the driver?
>
> Originally I have started this patch series to implement a replacement for
> the CSI-2 error handling you have added in
> commit 4ab44ff ("media: rcar-csi2: restart CSI-2 link if error is detected"),
> which is not correct for multiple reasons:
> 1. The commit message states that the user is informed that something is
>not right. But you have just added new messages which only appear in
>dmesg. This might be sufficient for a desktop PC but not for an embedded
>system, where the user normally can not see dmesg log. So I think that
>V4L2 events are the correct solution for this kind of notification,
>because they are passed directly to the application and the developer
>can implement handling for this issue or display an error in the
>custom human-machine interface.
> 2. It is not correct to restart the CSI-2 link if you don't restart VIN
>module as well. Renesas HW manual R19UH0105EJ0200 Rev.2.00
>(Jul 31, 2019) requires a reset or stop of capture in the VIN module
>before a reset of CSI-2 module (chapter 25.3.13 "Software Reset"). This
>also applies to CSI-2 error handling.
> 3. The CSI-2 driver restarts CSI-2 module for any CSI-2 error. However not
>all CSI2 errors are critical. In some setups they are really harmless so
>streaming can continue without any unnecessary restart. In some setups
>they _always_ occur at each start of streaming and are harmless as
>well, so automatic restart in CSI-2 module ends in an endless restart
>loop, which never comes to an end and breaks streaming instead of any
>help. It is better to leave such errors unhandled and therefore it is
>important to detect whether DMA transfers really stop in rvin_irq().
> 4. Video streaming applications in the automotive/embedded industry often
>want to control when the video streaming pipeline is stopped or started
>to be able to do some tasks in between, so an automatic restart of the
>pipeline is not acceptable for them. It should be at least optional and
>we should do this in rcar-dma.c, e.g. in the proposed irq timeout
>function. However I am not sure yet how to implement a restart of
>streaming inside of the rcar-vin driver correctly.
>
> I think, my patch series provides technically a good solution for the
> described issues. Also it is generic enough to allow also handling of
> failures in upstream subdevices connected to an R-Car3 CSI2 receiver or
> even parallel video input devices in cases when such failures can not be
> fixed or detected in the subdevice drivers and result in a stop of data
> flow on the chip level.
>
> Theoretically, applications also could use timeout parameter of the poll()
> syscall to implement the timeout (which can be e.g. a multiple of the frame
> interval), but the prob
Re: [PATCH 1/2] [RFC] media: rcar-vin: send a V4L2 event to vdev if no frame captured after a timeout
n v4l2_ctrl_subscribe_event(fh, sub);
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> b/drivers/media/platform/rcar-vin/rcar-vin.h
> index c19d077..7408f67 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -14,12 +14,14 @@
> #define __RCAR_VIN__
>
> #include
> +#include
>
> #include
> #include
> #include
> #include
> #include
> +#include
>
> /* Number of HW buffers */
> #define HW_BUFFER_NUM 3
> @@ -30,6 +32,8 @@
> /* Max number on VIN instances that can be in a system */
> #define RCAR_VIN_NUM 8
>
> +#define IRQ_TIMEOUT_MS 1000
> +
> struct rvin_group;
>
> enum model_id {
> @@ -196,6 +200,7 @@ struct rvin_info {
> * @compose: active composing
> * @src_rect:active size of the video source
> * @std: active video standard of the video source
> + * @irq_timer: monitors regular capturing of frames in
> rvin_irq()
> *
> * @alpha: Alpha component to fill in for supported pixel formats
> */
> @@ -240,6 +245,7 @@ struct rvin_dev {
> struct v4l2_rect src_rect;
> v4l2_std_id std;
>
> + struct timer_list irq_timer;
> unsigned int alpha;
> };
>
> diff --git a/include/uapi/linux/rcar-vin.h b/include/uapi/linux/rcar-vin.h
> new file mode 100644
> index ..4eb7f5e
> --- /dev/null
> +++ b/include/uapi/linux/rcar-vin.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +
> +#ifndef RCAR_VIN_USER_H
> +#define RCAR_VIN_USER_H
> +
> +/* class for events sent by the rcar-vin driver */
> +#define V4L2_EVENT_RCAR_VIN_CLASSV4L2_EVENT_PRIVATE_START
> +#define V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT (V4L2_EVENT_RCAR_VIN_CLASS |
> 0x1)
> +
> +#endif /* RCAR_VIN_USER_H */
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH v8 3/3] media: rcar-csi2: Optimize the selection PHTW register
Hi Suresh,
Thanks for your work.
On 2020-06-18 19:34:32 +0900, Suresh Udipi wrote:
> PHTW register is selected based on default bit rate from Table[1].
> for the bit rates less than or equal to 250. Currently first
> value of default bit rate which is greater than or equal to
> the caculated mbps is selected.This selection can be further
Missing space 'selected.This'.
> improved by selecting the default bit rate which is nearest to
> the calculated value.
>
> [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.12]
>
> Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver
> driver")
Nit-pick: No need for a blank line between fixes and SoB.
>
> Signed-off-by: Suresh Udipi
> Signed-off-by: Michael Rodin
> ---
> drivers/media/platform/rcar-vin/rcar-csi2.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 1184527..d7bf59f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -917,10 +917,18 @@ static int rcsi2_phtw_write_mbps(struct rcar_csi2
> *priv, unsigned int mbps,
>const struct rcsi2_mbps_reg *values, u16 code)
> {
> const struct rcsi2_mbps_reg *value;
> + const struct rcsi2_mbps_reg *prev_value = NULL;
>
> - for (value = values; value->mbps; value++)
> + for (value = values; value->mbps; value++) {
> if (value->mbps >= mbps)
> break;
> + prev_value = value;
> + }
> +
> + if (prev_value &&
> + ((mbps - prev_value->mbps) <= (value->mbps - mbps)))
> + value = prev_value;
> +
One to many blank lines. With this and the commit message fixed,
Reviewed-by: Niklas Söderlund
Nice work, thanks again for being persistent with this!
>
> if (!value->mbps) {
> dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH v8 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum
Hi Suresh,
Thanks for your patch.
On 2020-06-18 19:34:31 +0900, Suresh Udipi wrote:
> Add a warning message when the selected PHY speed is less
> than supported minimum PHY speed given in the hsfreq table[1].
>
> For raspberry pi camera capture on Kingfisher board with resolution
> 640x480, the calculated PHY speed is 48 mbps which is less than
> the minimum PHY speed 80 Mbps from the table[1]. But in this cases
> capture is successful.
>
> [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
>
> Signed-off-by: Suresh Udipi
> Signed-off-by: Michael Rodin
Reviewed-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-csi2.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index f18dedc..1184527 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -433,6 +433,10 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv,
> unsigned int mbps)
> const struct rcsi2_mbps_reg *hsfreq;
> const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
>
> + if (mbps < priv->info->hsfreqrange->mbps)
> + dev_warn(priv->dev, "%u Mbps less than min PHY speed %u Mbps",
> + mbps, priv->info->hsfreqrange->mbps);
> +
> for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
> if (hsfreq->mbps >= mbps)
> break;
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH v8 1/3] media: rcar-csi2: Correct the selection of hsfreqrange
Hi Suresh,
Thanks for your work.
On 2020-06-18 19:34:30 +0900, Suresh Udipi wrote:
> hsfreqrange should be chosen based on the calculated mbps which
> is closer to the default bit rate and within the range as per
> table[1]. But current calculation always selects first value which
> is greater than or equal to the calculated mbps which may lead
> to chosing a wrong range in some cases.
>
> For example for 360 mbps for H3/M3N
> Existing logic selects
> Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
>
> This hsfreqrange is out of range.
>
> The logic is changed to get the default value which is closest to the
> calculated value [1]
>
> Calculated value 360Mbps : Default 350Mbps Range [320.625 -380.625 mpbs]
>
> [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
>
> Please note that According to Renesas in Table 25.9 the range for
> 220 default value is corrected as below
>
> |Range (Mbps) | Default Bit rate (Mbps) |
> ---
> | 197.125-244.125 | 220 |
> ---
>
> Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver
> driver")
>
> Signed-off-by: Suresh Udipi
> Signed-off-by: Kazuyoshi Akiyama
> Signed-off-by: Michael Rodin
Reviewed-by: Niklas Söderlund
> ---
> Changes in v2:
> - Added the boundary check for the maximum bit rate.
>
> - Simplified the logic by remmoving range check
> as only the closest default value covers most
> of the use cases.
>
> - Aligning the commit message based on the above change
>
>
> Changes in v3:
> - Added max member from struct rcsi2_mbps_reg.
> mbps varialbe cannot be removed from rcsi2_mbps_reg,
> since this structure is reused for
> phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
> used.
>
>
>- Update the walk of the array in rcsi2_set_phypll() so that it finds
> the first entry where the calculated bit rate is less than the max.
>
>- Support lower bit rates less than 80Mbps like 48Mbps
> (Raspberry pi camera 640x480 connected to Kingfisher)
> can also be supported by selecting the lowest default bit rate 80Mbps
> as done before this fix
>
>- Alignement of the commit message based on above changes.
>
> Changes in v4:
> - Remove unncessary braces.
>
> Changes in v5:
>- Removed mbps variable in rcsi2_mbps_reg and aligned all
> tables accordingly
>
>
> Changes in v6
>- Renesas correct the range of default value 220Mbps. Now
> if we select the nearest value to the default value all
> the values are in range. So reverting back to original patch
>
>- Added warning for values less than Minimum 80Mbps
>
> Changes in v7
> - Create a seperate commit for the warning message less
> than minimum 80Mbps
>
> - Reorder the statements to increase readability
>
> Changes in v8
> - Extended the logic of selection of nearest mbps to
> PHTW registers value less than 250Mbps. A new commit
> is added
>
> drivers/media/platform/rcar-vin/rcar-csi2.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 151e6a9..f18dedc 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -431,16 +431,23 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
> {
> const struct rcsi2_mbps_reg *hsfreq;
> + const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
>
> - for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> + for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
> if (hsfreq->mbps >= mbps)
> break;
> + hsfreq_prev = hsfreq;
> + }
>
> if (!hsfreq->mbps) {
> dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> return -ERANGE;
> }
>
> + if (hsfreq_prev &&
> + ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
> + hsfreq = hsfreq_prev;
> +
> rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
>
> return 0;
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH] media: rcar-vin: Move media_device_register to async completion
Hi Hans, On 2020-06-24 15:31:01 +0200, Hans Verkuil wrote: > On 17/06/2020 17:28, Niklas Söderlund wrote: > > Hi Michael, > > > > On 2020-06-17 17:15:37 +0200, Michael Rodin wrote: > >> Hi Niklas and Steve, > >> > >> On Wed, Jun 17, 2020 at 12:56:46PM +0200, Niklas Söderlund wrote: > >>> Hi Michael and Steve, > >>> > >>> On 2020-06-16 19:31:36 +0200, Michael Rodin wrote: > >>>> From: Steve Longerbeam > >>>> > >>>> The media_device is registered during driver probe, before async > >>>> completion, so it is possible for .link_notify to be called before > >>>> all devices are bound. > >>>> > >>>> Fix this by moving media_device_register() to > >>>> rvin_group_notify_complete(). > >>>> This ensures that all devices are now bound (the rcar-csi2 subdevices and > >>>> and video capture devices) before .link_notify can be called. > >>> > >>> I'm curious to what situation created the need for this change. I'm > >>> currently trying to take the VIN driver in the opposite direction [1] > >>> with the end goal of registering video devices at probe time and then > >>> allow the media graph to populate as devices becomes available. > >> > >> It looks like almost all platform drivers call media_device_register() in > >> the completion callback. From my understaning it is necessary to ensure > >> that all subdevices are bound and all links are created before the user > >> can enable any link (which would trigger link_notify callback execution) > >> and set formats. If I am not mistaken, Steve could observe an "OOPS" or > >> at least it is theoretically possible. > > > > If an OOPS have been observed I would be interested to see it. That way > > we can fix the OOPS and keep the media graph registration where it is > > today. > > > >> > >> Actually I found that this patch alone is not enough even if it is correct, > >> because we also have to register the media device in > >> rvin_parallel_notify_complete() > >> in case if there is only a parallel video input device attached. > >> > >>> My reason for this is that we could have a functional pipeline inside > >>> the graph even if it's not complete. This came out of the GMSL work done > >>> a while pack where I had a faulty camera that would prevent the other 7 > >>> in the system to function. > >> > >> I agree that if a probe of a faulty subdevice fails, this should not affect > >> functionality of the other attached subdevices. The "complete" callback of > >> the async notifier is probably not executed in this case, so I guess, we > >> would have to register the media device in the "bound" callback after the > >> first > >> subdevice has been probed? Otherwise there is not much sense to have video > >> capture devices, which are not connected to any source. > > > > Calling it in the bound callback is mostly the same as it is today, as > > link_notify could then be called when not all entities are in the graph. > > In fact even if we where tp move the media device registration to the t > > complete callback we have this problem if any of the subdevices are > > unbound. Then we are back to the state with a registerd media device > > where not all entities are present. > > > > I think the solution here is to address the issue (if any) in the > > link_notify callback when the graph is not fully populated. > > As I have mentioned in other threads in this mailinglist (e.g. > https://www.spinics.net/lists/linux-media/msg171067.html), this is part > of a bigger problem: what to do if only part of a graph comes up, or > a part disappears. Today this is not supported. > > It is certainly desirable to support this, but I want to see an RFC first > detailing the interaction with userspace. And remember that today the > expectation of applications is that device nodes are only created if all > components of the device probed successfully. > > My preference is that rcar-vin is first changed to conform to current > expectations (i.e. /dev/mediaX is only registered at async completion). I do not object to first making rcar-vin behave in this way. However for that work to be truly useful it should also solve what happens with all video, subdevices and media devices in the scenario where one device of the graph is unbound after complete have been called and how they ca
Re: [PATCH] media: rcar-vin: Move media_device_register to async completion
Hi Michael, On 2020-06-17 17:15:37 +0200, Michael Rodin wrote: > Hi Niklas and Steve, > > On Wed, Jun 17, 2020 at 12:56:46PM +0200, Niklas Söderlund wrote: > > Hi Michael and Steve, > > > > On 2020-06-16 19:31:36 +0200, Michael Rodin wrote: > > > From: Steve Longerbeam > > > > > > The media_device is registered during driver probe, before async > > > completion, so it is possible for .link_notify to be called before > > > all devices are bound. > > > > > > Fix this by moving media_device_register() to > > > rvin_group_notify_complete(). > > > This ensures that all devices are now bound (the rcar-csi2 subdevices and > > > and video capture devices) before .link_notify can be called. > > > > I'm curious to what situation created the need for this change. I'm > > currently trying to take the VIN driver in the opposite direction [1] > > with the end goal of registering video devices at probe time and then > > allow the media graph to populate as devices becomes available. > > It looks like almost all platform drivers call media_device_register() in > the completion callback. From my understaning it is necessary to ensure > that all subdevices are bound and all links are created before the user > can enable any link (which would trigger link_notify callback execution) > and set formats. If I am not mistaken, Steve could observe an "OOPS" or > at least it is theoretically possible. If an OOPS have been observed I would be interested to see it. That way we can fix the OOPS and keep the media graph registration where it is today. > > Actually I found that this patch alone is not enough even if it is correct, > because we also have to register the media device in > rvin_parallel_notify_complete() > in case if there is only a parallel video input device attached. > > > My reason for this is that we could have a functional pipeline inside > > the graph even if it's not complete. This came out of the GMSL work done > > a while pack where I had a faulty camera that would prevent the other 7 > > in the system to function. > > I agree that if a probe of a faulty subdevice fails, this should not affect > functionality of the other attached subdevices. The "complete" callback of > the async notifier is probably not executed in this case, so I guess, we > would have to register the media device in the "bound" callback after the > first > subdevice has been probed? Otherwise there is not much sense to have video > capture devices, which are not connected to any source. Calling it in the bound callback is mostly the same as it is today, as link_notify could then be called when not all entities are in the graph. In fact even if we where tp move the media device registration to the t complete callback we have this problem if any of the subdevices are unbound. Then we are back to the state with a registerd media device where not all entities are present. I think the solution here is to address the issue (if any) in the link_notify callback when the graph is not fully populated. > > (Delayed) population of the media graph after media device registration > sounds also like a requirement for device tree overlay support, which would > also be a nice feature. > > > 1. [PATCH 0/5] media-device: Report if graph is complete > > > > > > > > Signed-off-by: Steve Longerbeam > > > Signed-off-by: Michael Rodin > > > --- > > > drivers/media/platform/rcar-vin/rcar-core.c | 14 ++ > > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c > > > b/drivers/media/platform/rcar-vin/rcar-core.c > > > index 7440c89..e70f83b 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > > @@ -253,7 +253,6 @@ static int rvin_group_init(struct rvin_group *group, > > > struct rvin_dev *vin) > > > struct media_device *mdev = &group->mdev; > > > const struct of_device_id *match; > > > struct device_node *np; > > > - int ret; > > > > > > mutex_init(&group->lock); > > > > > > @@ -266,7 +265,6 @@ static int rvin_group_init(struct rvin_group *group, > > > struct rvin_dev *vin) > > > vin_dbg(vin, "found %u enabled VIN's in DT", group->count); > > > > > > mdev->dev = vin->dev; > > > - mdev->ops = &rvin_media_ops; > > > > > > match = of_match_n
Re: [PATCH] media: rcar-vin: Move media_device_register to async completion
Hi Michael and Steve,
On 2020-06-16 19:31:36 +0200, Michael Rodin wrote:
> From: Steve Longerbeam
>
> The media_device is registered during driver probe, before async
> completion, so it is possible for .link_notify to be called before
> all devices are bound.
>
> Fix this by moving media_device_register() to rvin_group_notify_complete().
> This ensures that all devices are now bound (the rcar-csi2 subdevices and
> and video capture devices) before .link_notify can be called.
I'm curious to what situation created the need for this change. I'm
currently trying to take the VIN driver in the opposite direction [1]
with the end goal of registering video devices at probe time and then
allow the media graph to populate as devices becomes available.
My reason for this is that we could have a functional pipeline inside
the graph even if it's not complete. This came out of the GMSL work done
a while pack where I had a faulty camera that would prevent the other 7
in the system to function.
1. [PATCH 0/5] media-device: Report if graph is complete
>
> Signed-off-by: Steve Longerbeam
> Signed-off-by: Michael Rodin
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 14 ++
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c
> index 7440c89..e70f83b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -253,7 +253,6 @@ static int rvin_group_init(struct rvin_group *group,
> struct rvin_dev *vin)
> struct media_device *mdev = &group->mdev;
> const struct of_device_id *match;
> struct device_node *np;
> - int ret;
>
> mutex_init(&group->lock);
>
> @@ -266,7 +265,6 @@ static int rvin_group_init(struct rvin_group *group,
> struct rvin_dev *vin)
> vin_dbg(vin, "found %u enabled VIN's in DT", group->count);
>
> mdev->dev = vin->dev;
> - mdev->ops = &rvin_media_ops;
>
> match = of_match_node(vin->dev->driver->of_match_table,
> vin->dev->of_node);
> @@ -278,11 +276,7 @@ static int rvin_group_init(struct rvin_group *group,
> struct rvin_dev *vin)
>
> media_device_init(mdev);
>
> - ret = media_device_register(&group->mdev);
> - if (ret)
> - rvin_group_cleanup(group);
> -
> - return ret;
> + return 0;
> }
>
> static void rvin_group_release(struct kref *kref)
> @@ -688,6 +682,8 @@ static int rvin_group_notify_complete(struct
> v4l2_async_notifier *notifier)
> return ret;
> }
>
> + vin->group->mdev.ops = &rvin_media_ops;
> +
> /* Register all video nodes for the group. */
> for (i = 0; i < RCAR_VIN_NUM; i++) {
> if (vin->group->vin[i] &&
> @@ -736,8 +732,10 @@ static int rvin_group_notify_complete(struct
> v4l2_async_notifier *notifier)
> }
> }
> mutex_unlock(&vin->group->lock);
> + if (ret)
> + return ret;
>
> - return ret;
> + return media_device_register(&vin->group->mdev);
> }
>
> static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH v6] media: rcar-csi2: Correct the selection of hsfreqrange
Hello,
On 2020-06-12 19:28:06 +0200, Michael Rodin wrote:
> Hi Niklas,
> Hi Suresh,
>
> On Fri, Jun 12, 2020 at 12:10:51PM +0900, Suresh Udipi wrote:
> > On Wed, Jun 10, 2020 at 03:40:04PM +0200, Niklas Söderlund wrote:
> > > Hi Suresh,
> > >
> > > Thanks for your persistent work!
> > >
> > > On 2020-06-08 12:25:03 +0900, Suresh Udipi wrote:
> > > > hsfreqrange should be chosen based on the calculated mbps which
> > > > is closer to the default bit rate and within the range as per
> > > > table[1]. But current calculation always selects first value which
> > > > is greater than or equal to the calculated mbps which may lead
> > > > to chosing a wrong range in some cases.
> > > >
> > > > For example for 360 mbps for H3/M3N
> > > > Existing logic selects
> > > > Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> > > >
> > > > This hsfreqrange is out of range.
> > > >
> > > > The logic is changed to get the default value which is closest to the
> > > > calculated value [1]
> > > >
> > > > Calculated value 360Mbps : Default 350Mbps Range [320.625 -380.625
> > > > mpbs]
> > > >
> > > > [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> > > >
> > > > Please note that According to Renesas in Table 25.9 the range for
> > > > 220 default value is corrected as below
> > > >
> > > > |Range (Mbps) | Default Bit rate (Mbps) |
> > > > ---
> > > > | 197.125-244.125 | 220 |
> > > > ---
> > > >
> > > > Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2
> > > > receiver driver")
> > > >
> > > > Signed-off-by: Suresh Udipi
> > > > Signed-off-by: Kazuyoshi Akiyama
> > > > Signed-off-by: Michael Rodin
> > > > ---
> > > > Changes in v2:
> > > > - Added the boundary check for the maximum bit rate.
> > > >
> > > > - Simplified the logic by remmoving range check
> > > > as only the closest default value covers most
> > > > of the use cases.
> > > >
> > > > - Aligning the commit message based on the above change
> > > >
> > > >
> > > > Changes in v3:
> > > > - Added max member from struct rcsi2_mbps_reg.
> > > > mbps varialbe cannot be removed from rcsi2_mbps_reg,
> > > > since this structure is reused for
> > > > phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
> > > > used.
> > > >
> > > >
> > > >- Update the walk of the array in rcsi2_set_phypll() so that it
> > > > finds
> > > > the first entry where the calculated bit rate is less than the
> > > > max.
> > > >
> > > >- Support lower bit rates less than 80Mbps like 48Mbps
> > > > (Raspberry pi camera 640x480 connected to Kingfisher)
> > > > can also be supported by selecting the lowest default bit rate
> > > > 80Mbps
> > > > as done before this fix
> > > >
> > > >- Alignement of the commit message based on above changes.
> > > >
> > > > Changes in v4:
> > > > - Remove unncessary braces.
> > > >
> > > > Changes in v5:
> > > >- Removed mbps variable in rcsi2_mbps_reg and aligned all
> > > > tables accordingly
> > > >
> > > > Changes in v6
> > > >- Renesas correct the range of default value 220Mbps. Now
> > > > if we select the nearest value to the default value all
> > > > the values are in range. So reverting back to original patch
> > > >
> > > >- Added warning for values less than Minimum 80Mbps
> > > >
> > > >
> > > > drivers/media/platform/rcar-vin/rcar-csi2.c | 23
> > > > ++-
> > > > 1 file changed, 18 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > index 151e6a9..8c502b7 100644
&
Re: [PATCH v6] media: rcar-csi2: Correct the selection of hsfreqrange
hsfreq_prev = hsfreq;
> + }
> +
> + if (hsfreq_prev &&
> + ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
Longer lines are now OK [1] and I think it would add to the readability
here.
> + hsfreq = hsfreq_prev;
> +
How about
static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
{
const struct rcsi2_mbps_reg *hsfreq;
const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
if (hsfreq->mbps >= mbps)
break;
hsfreq_prev = hsfreq;
}
if (!hsfreq->mbps) {
dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
return -ERANGE;
}
if (hsfreq_prev && ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
hsfreq = hsfreq_prev;
rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
return 0;
}
> rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
>
> return 0;
> --
> 2.7.4
>
1. https://lkml.org/lkml/2020/5/29/1038
--
Regards,
Niklas Söderlund
Re: [PATCH v5 1/3] media: rcar-vin: Invalidate pipeline if conversion is not possible on input formats
Hi Lad,
Thanks for your work.
On 2020-05-27 13:16:48 +0100, Lad Prabhakar wrote:
> Up until now the VIN was capable to convert any of its supported input mbus
> formats to any of it's supported output pixel formats. With the addition of
> RAW formats this is no longer true.
>
> This patch invalidates the pipeline by adding a check if given vin input
> format can be converted to supported output pixel format.
>
> Signed-off-by: Lad Prabhakar
Reviewed-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-dma.c | 6 +-
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 11 ---
> 2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 1a30cd036371..2b26204910fd 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1110,11 +1110,15 @@ static int rvin_mc_validate_format(struct rvin_dev
> *vin, struct v4l2_subdev *sd,
> case MEDIA_BUS_FMT_UYVY8_2X8:
> case MEDIA_BUS_FMT_UYVY10_2X10:
> case MEDIA_BUS_FMT_RGB888_1X24:
> - vin->mbus_code = fmt.format.code;
> + break;
> + case MEDIA_BUS_FMT_SRGGB8_1X8:
> + if (vin->format.pixelformat != V4L2_PIX_FMT_SRGGB8)
> + return -EPIPE;
> break;
> default:
> return -EPIPE;
> }
> + vin->mbus_code = fmt.format.code;
>
> switch (fmt.format.field) {
> case V4L2_FIELD_TOP:
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index f421e2584875..d3b6a992b4a2 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -350,9 +350,9 @@ static int rvin_enum_fmt_vid_cap(struct file *file, void
> *priv,
>* all of the related pixel formats. If mbus_code is not set enumerate
>* all possible pixelformats.
>*
> - * TODO: Once raw capture formats are added to the driver this needs
> - * to be extended so raw media bus codes only result in raw pixel
> - * formats.
> + * TODO: Once raw MEDIA_BUS_FMT_SRGGB12_1X12 format is added to the
> + * driver this needs to be extended so raw media bus code only result in
> + * raw pixel format.
>*/
> switch (f->mbus_code) {
> case 0:
> @@ -362,6 +362,11 @@ static int rvin_enum_fmt_vid_cap(struct file *file, void
> *priv,
> case MEDIA_BUS_FMT_UYVY10_2X10:
> case MEDIA_BUS_FMT_RGB888_1X24:
> break;
> + case MEDIA_BUS_FMT_SRGGB8_1X8:
> + if (f->index)
> + return -EINVAL;
> + f->pixelformat = V4L2_PIX_FMT_SRGGB8;
> + return 0;
> default:
> return -EINVAL;
> }
> --
> 2.17.1
>
--
Regards,
Niklas Söderlund
Re: [PATCH] dt-bindings: irqchip: renesas-rza1-irqc: Convert to json-schema
Hi Geert,
Thanks for your work.
On 2020-05-28 15:28:53 +0200, Geert Uytterhoeven wrote:
> Convert the Renesas RZ/A1 Interrupt Controller Device Tree binding
> documentation to json-schema.
>
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Niklas Söderlund
> ---
> Validation depends on "[PATCH dt-schema] Fix interrupt controllers with
> interrupt-map".
> http://lore.kernel.org/r/20200528132323.30288-1-geert+rene...@glider.be
> ---
> .../renesas,rza1-irqc.txt | 43 --
> .../renesas,rza1-irqc.yaml| 80 +++
> 2 files changed, 80 insertions(+), 43 deletions(-)
> delete mode 100644
> Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> create mode 100644
> Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.yaml
>
> diff --git
> a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>
> b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> deleted file mode 100644
> index 727b7e4cd6e01110..
> ---
> a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> +++ /dev/null
> @@ -1,43 +0,0 @@
> -DT bindings for the Renesas RZ/A1 Interrupt Controller
> -
> -The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> -RZ/A1 and RZ/A2 SoCs:
> - - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> -interrupts,
> - - NMI edge select.
> -
> -Required properties:
> - - compatible: Must be "renesas,-irqc", and "renesas,rza1-irqc" as
> - fallback.
> - Examples with soctypes are:
> - - "renesas,r7s72100-irqc" (RZ/A1H)
> - - "renesas,r7s9210-irqc" (RZ/A2M)
> - - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> - in interrupts.txt in this directory)
> - - #address-cells: Must be zero
> - - interrupt-controller: Marks the device as an interrupt controller
> - - reg: Base address and length of the memory resource used by the interrupt
> - controller
> - - interrupt-map: Specifies the mapping from external interrupts to GIC
> -interrupts
> - - interrupt-map-mask: Must be <7 0>
> -
> -Example:
> -
> - irqc: interrupt-controller@fcfef800 {
> - compatible = "renesas,r7s72100-irqc", "renesas,rza1-irqc";
> - #interrupt-cells = <2>;
> - #address-cells = <0>;
> - interrupt-controller;
> - reg = <0xfcfef800 0x6>;
> - interrupt-map =
> - <0 0 &gic GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
> - <1 0 &gic GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> - <2 0 &gic GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> - <3 0 &gic GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> - <4 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> - <5 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> - <6 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> - <7 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> - interrupt-map-mask = <7 0>;
> - };
> diff --git
> a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.yaml
>
> b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.yaml
> new file mode 100644
> index ..755cdfabfcd06c85
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id:
> http://devicetree.org/schemas/interrupt-controller/renesas,rza1-irqc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RZ/A1 Interrupt Controller
> +
> +maintainers:
> + - Chris Brandt
> + - Geert Uytterhoeven
> +
> +description: |
> + The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> RZ/A1 and
> + RZ/A2 SoCs:
> +- IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> interrupts,
> +- NMI edge select.
> +
> +allOf:
> + - $ref: /schemas/interrupt-controller.yaml#
> +
> +properties:
> + compatible:
> +items:
> + - enum:
> + - renesas,r7s72100-irqc # RZ/A1H
> + - renesas,r7s9210-irqc # RZ/A2M
> + - const: renesas,rza1-irqc
> +
> + '#interrupt-cells':
> +const: 2
Re: [PATCH] dt-bindings: timer: Add renesas,em-sti bindings
Hi Geert,
Thanks for your work.
On 2020-05-19 10:11:01 +0200, Geert Uytterhoeven wrote:
> Document Device Tree bindings for the Renesas EMMA Mobile System Timer.
>
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Niklas Söderlund
> ---
> .../bindings/timer/renesas,em-sti.yaml| 46 +++
> 1 file changed, 46 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/timer/renesas,em-sti.yaml
>
> diff --git a/Documentation/devicetree/bindings/timer/renesas,em-sti.yaml
> b/Documentation/devicetree/bindings/timer/renesas,em-sti.yaml
> new file mode 100644
> index ..233d74d5402cf734
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/renesas,em-sti.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/timer/renesas,em-sti.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas EMMA Mobile System Timer
> +
> +maintainers:
> + - Magnus Damm
> +
> +properties:
> + compatible:
> +const: renesas,em-sti
> +
> + reg:
> +maxItems: 1
> +
> + interrupts:
> +maxItems: 1
> +
> + clocks:
> +maxItems: 1
> +
> + clock-names:
> +const: sclk
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +#include
> +timer@e018 {
> +compatible = "renesas,em-sti";
> + reg = <0xe018 0x54>;
> +interrupts = ;
> +clocks = <&sti_sclk>;
> +clock-names = "sclk";
> +};
> --
> 2.17.1
>
--
Regards,
Niklas Söderlund
Re: [PATCH v5] media: rcar-csi2: Correct the selection of hsfreqrange
.reg = 0x39 },
> > - { .mbps = 950, .reg = 0x0a },
> > - { .mbps = 1000, .reg = 0x1a },
> > - { .mbps = 1050, .reg = 0x2a },
> > - { .mbps = 1100, .reg = 0x3a },
> > - { .mbps = 1150, .reg = 0x0b },
> > - { .mbps = 1200, .reg = 0x1b },
> > - { .mbps = 1250, .reg = 0x2b },
> > - { .mbps = 1300, .reg = 0x3b },
> > - { .mbps = 1350, .reg = 0x0c },
> > - { .mbps = 1400, .reg = 0x1c },
> > - { .mbps = 1450, .reg = 0x2c },
> > - { .mbps = 1500, .reg = 0x3c },
> > + { .reg = 0x00, .max = 110 },
> > + { .reg = 0x10, .max = 120 },
> > + { .reg = 0x20, .max = 131 },
> > + { .reg = 0x30, .max = 141 },
> > + { .reg = 0x01, .max = 152 },
> > + { .reg = 0x11, .max = 162 },
> > + { .reg = 0x21, .max = 173 },
> > + { .reg = 0x31, .max = 183 },
> > + { .reg = 0x02, .max = 194 },
> > + { .reg = 0x12, .max = 204 },
> > + { .reg = 0x22, .max = 215 },
> > + { .reg = 0x32, .max = 225 },
> > + { .reg = 0x03, .max = 241 },
> > + { .reg = 0x13, .max = 257 },
> > + { .reg = 0x23, .max = 273 },
> > + { .reg = 0x33, .max = 275 },
> > + { .reg = 0x04, .max = 301 },
> > + { .reg = 0x14, .max = 328 },
> > + { .reg = 0x05, .max = 354 },
> > + { .reg = 0x15, .max = 393 },
> > + { .reg = 0x25, .max = 446 },
> > + { .reg = 0x06, .max = 498 },
> > + { .reg = 0x16, .max = 551 },
> > + { .reg = 0x07, .max = 603 },
> > + { .reg = 0x17, .max = 656 },
> > + { .reg = 0x08, .max = 708 },
> > + { .reg = 0x18, .max = 761 },
> > + { .reg = 0x09, .max = 813 },
> > + { .reg = 0x19, .max = 866 },
> > + { .reg = 0x29, .max = 918 },
> > + { .reg = 0x39, .max = 971 },
> > + { .reg = 0x0a, .max = 1023 },
> > + { .reg = 0x1a, .max = 1076 },
> > + { .reg = 0x2a, .max = 1128 },
> > + { .reg = 0x3a, .max = 1181 },
> > + { .reg = 0x0b, .max = 1233 },
> > + { .reg = 0x1b, .max = 1286 },
> > + { .reg = 0x2b, .max = 1338 },
> > + { .reg = 0x3b, .max = 1391 },
> > + { .reg = 0x0c, .max = 1443 },
> > + { .reg = 0x1c, .max = 1496 },
> > + { .reg = 0x2c, .max = 1500 },
> > + { .reg = 0x3c, .max = 1500 },
> > { /* sentinel */ },
> > };
> >
> > @@ -432,11 +432,11 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv,
> > unsigned int mbps)
> > {
> > const struct rcsi2_mbps_reg *hsfreq;
> >
> > - for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> > - if (hsfreq->mbps >= mbps)
> > + for (hsfreq = priv->info->hsfreqrange; hsfreq->max != 0; hsfreq++)
> > + if (hsfreq->max >= mbps)
> > break;
> >
> > - if (!hsfreq->mbps) {
> > + if (!hsfreq->max) {
> > dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> > return -ERANGE;
> > }
> > @@ -907,11 +907,11 @@ static int rcsi2_phtw_write_mbps(struct rcar_csi2
> > *priv, unsigned int mbps,
> > {
> > const struct rcsi2_mbps_reg *value;
> >
> > - for (value = values; value->mbps; value++)
> > - if (value->mbps >= mbps)
> > + for (value = values; value->max; value++)
> > + if (value->max >= mbps)
> > break;
> >
> > - if (!value->mbps) {
> > + if (!value->max) {
> > dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> > return -ERANGE;
> > }
> > --
> > 2.7.4
> >
>
> --
> Best Regards,
> Michael
--
Regards,
Niklas Söderlund
Re: [PATCH] pinctrl: sh-pfc: Do not use platform_get_irq() to count interrupts
Hi Geert,
Thanks for your work.
On 2019-10-01 20:05:47 +0200, Geert Uytterhoeven wrote:
> As platform_get_irq() now prints an error when the interrupt does not
> exist, counting interrupts by looping until failure causes the printing
> of scary messages like:
>
> sh-pfc e606.pin-controller: IRQ index 0 not found
>
> Fix this by using the platform_irq_count() helper instead.
>
> Fixes: 7723f4c5ecdb8d83 ("driver core: platform: Add an error message to
> platform_get_irq*()")
> Signed-off-by: Geert Uytterhoeven
Nice change.
Reviewed-by: Niklas Söderlund
> ---
> This is a fix for v5.4-rc1.
> ---
> drivers/pinctrl/sh-pfc/core.c | 16 ++--
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
> index b8640ad41bef26be..ce983247c9e28bfe 100644
> --- a/drivers/pinctrl/sh-pfc/core.c
> +++ b/drivers/pinctrl/sh-pfc/core.c
> @@ -29,12 +29,12 @@
> static int sh_pfc_map_resources(struct sh_pfc *pfc,
> struct platform_device *pdev)
> {
> - unsigned int num_windows, num_irqs;
> struct sh_pfc_window *windows;
> unsigned int *irqs = NULL;
> + unsigned int num_windows;
> struct resource *res;
> unsigned int i;
> - int irq;
> + int num_irqs;
>
> /* Count the MEM and IRQ resources. */
> for (num_windows = 0;; num_windows++) {
> @@ -42,17 +42,13 @@ static int sh_pfc_map_resources(struct sh_pfc *pfc,
> if (!res)
> break;
> }
> - for (num_irqs = 0;; num_irqs++) {
> - irq = platform_get_irq(pdev, num_irqs);
> - if (irq == -EPROBE_DEFER)
> - return irq;
> - if (irq < 0)
> - break;
> - }
> -
> if (num_windows == 0)
> return -EINVAL;
>
> + num_irqs = platform_irq_count(pdev);
> + if (num_irqs < 0)
> + return num_irqs;
> +
> /* Allocate memory windows and IRQs arrays. */
> windows = devm_kcalloc(pfc->dev, num_windows, sizeof(*windows),
> GFP_KERNEL);
> --
> 2.17.1
>
--
Regards,
Niklas Söderlund
Re: [PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility
Hi Tim, On 2019-09-27 12:26:40 -0700, Tim Harvey wrote: > On Fri, Sep 27, 2019 at 12:04 PM Niklas Söderlund > wrote: > > > > Hi Tim, > > > > Sorry for taking to so long to look at this. > > > > On 2019-09-23 15:04:47 -0700, Tim Harvey wrote: > > > On Thu, Aug 29, 2019 at 7:29 AM Niklas Söderlund > > > wrote: > > > > > > > > Hi, > > > > > > > > On 2019-08-29 13:43:49 +0200, Hans Verkuil wrote: > > > > > Adding Niklas. > > > > > > > > > > Niklas, can you take a look at this? > > > > > > > > I'm happy to have a look at this. I'm currently moving so all my boards > > > > are in a box somewhere. I hope to have my lab up and running next week, > > > > so if this is not urgent I will look at it then. > > > > > > > > > > Niklas, > > > > > > Have you looked at this yet? Without this patch the ADV7280A does not > > > output proper BT.656. We tested this on a Gateworks Ventana GW5404-G > > > which uses the ADV7280A connected to the IMX6 CSI parallel bus. I'm > > > hoping to see this get merged and perhaps backported to older kernels. > > > > I only have access to an adv7180 so I was unable to test this patch. > > After reviewing the documentation I think the patch is OK if what you > > want is to unconditionally switch the driver from outputting BT.656-3 to > > outputting BT.656-4. > > > > As this change would effect a large number of compat strings (adv7280, > > adv7280-m, adv7281, adv7281-m, adv7281-ma, adv7282, adv7282-m) and the > > goal is to back port it I'm a bit reluctant to adding my tag to this > > patch as I'm not sure if this will break other setups. > > > > From the documentation about the BT.656-4 register (address 0x04 bit 7): > > > > Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, > > the ITU has changed the toggling position for the V bit within > > the SAV EAV codes for NTSC. The ITU-R BT.656-4 standard > > bit allows the user to select an output mode that is compliant > > with either the previous or new standard. For further information, > > visit the International Telecommunication Union website. > > > > Note that the standard change only affects NTSC and has no > > bearing on PAL. > > > > When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 > > specification is used. The V bit goes low at EAV of Line 10 > > and Line 273. > > > > When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is > > used. The V bit goes low at EAV of Line 20 and Line 283. > > > > Do you know what effects such a change would bring? Looking at the > > driver BT.656-4 seems to be set unconditionally for some adv7180 chips. > > > > Niklas, > > Quite simply, we have a board that has an ADV7180 attached to the > parallel CSI of an IMX6 that worked fine with mainline drivers then > when we revised this board to attach an ADV7280A in the same way > capture failed to sync. Investigation showed that the NEWAVMODE > differed between the two. I understand your problem, the driver configures adv7180 and adv7280 differently. > > So if the point of the driver is to configure the variants in the same > way, this patch needs to be applied. I'm not sure that is the point of the driver. As the driver today configures different compatible strings differently. Some as ITU-R BT.656-3 and some as ITU-R BT.656-4, I can only assume there is a reason for that. > > I would maintain that the adv7180 comes up with NEWAVMODE enabled and > in order to be compatible we must configure the adv7282 the same. > > The same argument can be made for setting the V bit end position in > NTSC mode - its done for the adv7180 so for compatible output it > should be done for the adv7282. I understand that this is needed to make it a drop-in replacement for the adv7180 in your use-case. But I'm not sure it is a good idea for other users of the driver. What if someone is already using a adv7282 on a board and depends on it providing ITU-R BT.656-3 and the old settings? If this patch is picked up there use-cases may break. I'm not sure what the best way forward is I'm afraid. Looking at video-interfaces.txt we have a device tree property bus-type which is used to describe the bus is a BT.656 bus but not which revision of it. I'm not really found of driver specific bus descriptions, but maybe this is a case where one might consider adding one? Hans what do you think? > > > > > > >
Re: [PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility
Hi Tim,
Sorry for taking to so long to look at this.
On 2019-09-23 15:04:47 -0700, Tim Harvey wrote:
> On Thu, Aug 29, 2019 at 7:29 AM Niklas Söderlund
> wrote:
> >
> > Hi,
> >
> > On 2019-08-29 13:43:49 +0200, Hans Verkuil wrote:
> > > Adding Niklas.
> > >
> > > Niklas, can you take a look at this?
> >
> > I'm happy to have a look at this. I'm currently moving so all my boards
> > are in a box somewhere. I hope to have my lab up and running next week,
> > so if this is not urgent I will look at it then.
> >
>
> Niklas,
>
> Have you looked at this yet? Without this patch the ADV7280A does not
> output proper BT.656. We tested this on a Gateworks Ventana GW5404-G
> which uses the ADV7280A connected to the IMX6 CSI parallel bus. I'm
> hoping to see this get merged and perhaps backported to older kernels.
I only have access to an adv7180 so I was unable to test this patch.
After reviewing the documentation I think the patch is OK if what you
want is to unconditionally switch the driver from outputting BT.656-3 to
outputting BT.656-4.
As this change would effect a large number of compat strings (adv7280,
adv7280-m, adv7281, adv7281-m, adv7281-ma, adv7282, adv7282-m) and the
goal is to back port it I'm a bit reluctant to adding my tag to this
patch as I'm not sure if this will break other setups.
>From the documentation about the BT.656-4 register (address 0x04 bit 7):
Between Revision 3 and Revision 4 of the ITU-R BT.656 standards,
the ITU has changed the toggling position for the V bit within
the SAV EAV codes for NTSC. The ITU-R BT.656-4 standard
bit allows the user to select an output mode that is compliant
with either the previous or new standard. For further information,
visit the International Telecommunication Union website.
Note that the standard change only affects NTSC and has no
bearing on PAL.
When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3
specification is used. The V bit goes low at EAV of Line 10
and Line 273.
When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is
used. The V bit goes low at EAV of Line 20 and Line 283.
Do you know what effects such a change would bring? Looking at the
driver BT.656-4 seems to be set unconditionally for some adv7180 chips.
>
> Regards,
>
> Tim
>
> > >
> > > Regards,
> > >
> > > Hans
> > >
> > > On 8/27/19 11:55 PM, Matthew Michilot wrote:
> > > > From: Matthew Michilot
> > > >
> > > > Captured video would be out of sync when using the adv7280 with
> > > > the BT.656-4 protocol. Certain registers (0x04, 0x31, 0xE6) had to
> > > > be configured properly to ensure BT.656-4 compatibility.
> > > >
> > > > An error in the adv7280 reference manual suggested that EAV/SAV mode
> > > > was enabled by default, however upon inspecting register 0x31, it was
> > > > determined to be disabled by default.
The manual I have [1] states that NEWAVMODE is switched off by default.
I'm only asking as I would like to know if there is an error in that
datasheet or not.
1.
https://www.analog.com/media/en/technical-documentation/user-guides/ADV7280_7281_7282_7283_UG-637.pdf
> > > >
> > > > Signed-off-by: Matthew Michilot
> > > > Reviewed-by: Tim Harvey
> > > > ---
> > > > drivers/media/i2c/adv7180.c | 15 +--
> > > > 1 file changed, 13 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > > > index 99697baad2ea..27da424dce76 100644
> > > > --- a/drivers/media/i2c/adv7180.c
> > > > +++ b/drivers/media/i2c/adv7180.c
> > > > @@ -94,6 +94,7 @@
> > > > #define ADV7180_REG_SHAP_FILTER_CTL_1 0x0017
> > > > #define ADV7180_REG_CTRL_2 0x001d
> > > > #define ADV7180_REG_VSYNC_FIELD_CTL_1 0x0031
> > > > +#define ADV7180_VSYNC_FIELD_CTL_1_NEWAV 0x12
> > > > #define ADV7180_REG_MANUAL_WIN_CTL_1 0x003d
> > > > #define ADV7180_REG_MANUAL_WIN_CTL_2 0x003e
> > > > #define ADV7180_REG_MANUAL_WIN_CTL_3 0x003f
> > > > @@ -935,10 +936,20 @@ static int adv7182_init(struct adv7180_state
> > > > *state)
> > > > adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> > > > 0x57);
> > > > adv7180_write(state, ADV7180_REG_CTRL_2, 0xc0);
> > > > } else {
> > > > -
Re: [PATCH] media: platform: Use devm_platform_ioremap_resource() in two functions
Hi Markus,
Thanks for your patch.
On 2019-09-18 11:30:30 +0200, Markus Elfring wrote:
> From: Markus Elfring
> Date: Wed, 18 Sep 2019 11:20:48 +0200
>
> Simplify these function implementations by using a known wrapper function.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring
> ---
> drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 8 +---
> drivers/media/platform/rcar-vin/rcar-core.c| 7 +--
For rcar-vin:
Tested-by: Niklas Söderlund
> 2 files changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> index 00d090df11bb..944771ee5f5c 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> @@ -253,13 +253,7 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
> }
>
> for (i = 0; i < NUM_MAX_VDEC_REG_BASE; i++) {
> - res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> - if (res == NULL) {
> - dev_err(&pdev->dev, "get memory resource failed.");
> - ret = -ENXIO;
> - goto err_res;
> - }
> - dev->reg_base[i] = devm_ioremap_resource(&pdev->dev, res);
> + dev->reg_base[i] = devm_platform_ioremap_resource(pdev, i);
> if (IS_ERR((__force void *)dev->reg_base[i])) {
> ret = PTR_ERR((__force void *)dev->reg_base[i]);
> goto err_res;
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c
> index 6993484ff0f3..334c62805959 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -1282,7 +1282,6 @@ static int rcar_vin_probe(struct platform_device *pdev)
> {
> const struct soc_device_attribute *attr;
> struct rvin_dev *vin;
> - struct resource *mem;
> int irq, ret;
>
> vin = devm_kzalloc(&pdev->dev, sizeof(*vin), GFP_KERNEL);
> @@ -1301,11 +1300,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
> if (attr)
> vin->info = attr->data;
>
> - mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (mem == NULL)
> - return -EINVAL;
> -
> - vin->base = devm_ioremap_resource(vin->dev, mem);
> + vin->base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(vin->base))
> return PTR_ERR(vin->base);
>
> --
> 2.23.0
>
--
Regards,
Niklas Söderlund
Re: [PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility
Hi,
On 2019-08-29 13:43:49 +0200, Hans Verkuil wrote:
> Adding Niklas.
>
> Niklas, can you take a look at this?
I'm happy to have a look at this. I'm currently moving so all my boards
are in a box somewhere. I hope to have my lab up and running next week,
so if this is not urgent I will look at it then.
>
> Regards,
>
> Hans
>
> On 8/27/19 11:55 PM, Matthew Michilot wrote:
> > From: Matthew Michilot
> >
> > Captured video would be out of sync when using the adv7280 with
> > the BT.656-4 protocol. Certain registers (0x04, 0x31, 0xE6) had to
> > be configured properly to ensure BT.656-4 compatibility.
> >
> > An error in the adv7280 reference manual suggested that EAV/SAV mode
> > was enabled by default, however upon inspecting register 0x31, it was
> > determined to be disabled by default.
> >
> > Signed-off-by: Matthew Michilot
> > Reviewed-by: Tim Harvey
> > ---
> > drivers/media/i2c/adv7180.c | 15 +--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > index 99697baad2ea..27da424dce76 100644
> > --- a/drivers/media/i2c/adv7180.c
> > +++ b/drivers/media/i2c/adv7180.c
> > @@ -94,6 +94,7 @@
> > #define ADV7180_REG_SHAP_FILTER_CTL_1 0x0017
> > #define ADV7180_REG_CTRL_2 0x001d
> > #define ADV7180_REG_VSYNC_FIELD_CTL_1 0x0031
> > +#define ADV7180_VSYNC_FIELD_CTL_1_NEWAV 0x12
> > #define ADV7180_REG_MANUAL_WIN_CTL_1 0x003d
> > #define ADV7180_REG_MANUAL_WIN_CTL_2 0x003e
> > #define ADV7180_REG_MANUAL_WIN_CTL_3 0x003f
> > @@ -935,10 +936,20 @@ static int adv7182_init(struct adv7180_state *state)
> > adv7180_write(state, ADV7180_REG_EXTENDED_OUTPUT_CONTROL, 0x57);
> > adv7180_write(state, ADV7180_REG_CTRL_2, 0xc0);
> > } else {
> > - if (state->chip_info->flags & ADV7180_FLAG_V2)
> > + if (state->chip_info->flags & ADV7180_FLAG_V2) {
> > + /* ITU-R BT.656-4 compatible */
> > adv7180_write(state,
> > ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> > - 0x17);
> > + ADV7180_EXTENDED_OUTPUT_CONTROL_NTSCDIS);
> > + /* Manually set NEWAVMODE */
> > + adv7180_write(state,
> > + ADV7180_REG_VSYNC_FIELD_CTL_1,
> > + ADV7180_VSYNC_FIELD_CTL_1_NEWAV);
> > + /* Manually set V bit end position in NTSC mode */
> > + adv7180_write(state,
> > + ADV7180_REG_NTSC_V_BIT_END,
> > + ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
> > + }
> > else
> > adv7180_write(state,
> > ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
> >
>
--
Regards,
Niklas Söderlund
Re: [PATCH] thermal: rcar_gen3_thermal: Use devm_add_action_or_reset() helper
Hi Geert,
Thanks for your work.
On 2019-07-31 14:50:53 +0200, Geert Uytterhoeven wrote:
> Use the devm_add_action_or_reset() helper instead of open-coding the
> same operations.
>
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Niklas Söderlund
> ---
> drivers/thermal/rcar_gen3_thermal.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thermal/rcar_gen3_thermal.c
> b/drivers/thermal/rcar_gen3_thermal.c
> index a56463308694e937..2db7e7f8baf939fd 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -443,11 +443,10 @@ static int rcar_gen3_thermal_probe(struct
> platform_device *pdev)
> if (ret)
> goto error_unregister;
>
> - ret = devm_add_action(dev, rcar_gen3_hwmon_action, zone);
> - if (ret) {
> - rcar_gen3_hwmon_action(zone);
> + ret = devm_add_action_or_reset(dev, rcar_gen3_hwmon_action,
> +zone);
> + if (ret)
> goto error_unregister;
> - }
>
> ret = of_thermal_get_ntrips(tsc->zone);
> if (ret < 0)
> --
> 2.17.1
>
--
Regards,
Niklas Söderlund
Re: [PATCH] MAINTAINERS: Add Geert as Renesas SoC Co-Maintainer
Hi Geert, Simon, On 2019-07-29 19:56:58 +0200, Geert Uytterhoeven wrote: > At the end of the v5.3 upstream kernel development cycle, Simon will be > stepping down from his role as Renesas SoC maintainer. Starting with > the v5.4 development cycle, Geert is taking over this role. I would like to thank Simon for his good humor and support in all my interactions with him as a maintainer. And wish Geert the best of luck picking up the baton, not that you need it I know you will be just as fun to work with. > > Add Geert as a co-maintainer, and add his git repository and branch. > > Signed-off-by: Geert Uytterhoeven Reviewed-by: Niklas Söderlund > --- > Stephen: Can you please add my branch to linux-next, after Simon's >branch, which may still receive fixes for v5.3? > >Thanks! > --- > MAINTAINERS | 4 > 1 file changed, 4 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 6426db5198f05377..6de667021591fb52 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2155,10 +2155,12 @@ F: > Documentation/devicetree/bindings/arm/realtek.txt > > ARM/RENESAS ARM64 ARCHITECTURE > M: Simon Horman > +M: Geert Uytterhoeven > M: Magnus Damm > L: linux-renesas-...@vger.kernel.org > Q: http://patchwork.kernel.org/project/linux-renesas-soc/list/ > T: git git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git next > +T: git > git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git next > S: Supported > F: arch/arm64/boot/dts/renesas/ > F: Documentation/devicetree/bindings/arm/renesas.yaml > @@ -2269,10 +2271,12 @@ F:drivers/media/platform/s5p-mfc/ > > ARM/SHMOBILE ARM ARCHITECTURE > M: Simon Horman > +M: Geert Uytterhoeven > M: Magnus Damm > L: linux-renesas-...@vger.kernel.org > Q: http://patchwork.kernel.org/project/linux-renesas-soc/list/ > T: git git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git next > +T: git > git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git next > S: Supported > F: arch/arm/boot/dts/emev2* > F: arch/arm/boot/dts/gr-peach* > -- > 2.17.1 > -- Regards, Niklas Söderlund
Re: [PATCH] media: i2c: adv748x: Convert to new i2c device probe()
Hi Kieran,
Thanks for your patch.
On 2019-07-10 13:37:19 +0100, Kieran Bingham wrote:
> The I2C core framework provides a simplified probe framework from commit
> b8a1a4cd5a98 ("i2c: Provide a temporary .probe_new() call-back type").
>
> Convert the ADV748x to utilise this simplfied i2c driver registration.
>
> Signed-off-by: Kieran Bingham
Looks good,
Reviewed-by: Niklas Söderlund
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 13 ++---
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c
> index 097e94279cf7..ae2b6eb93e09 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -677,8 +677,7 @@ static void adv748x_dt_cleanup(struct adv748x_state
> *state)
> of_node_put(state->endpoints[i]);
> }
>
> -static int adv748x_probe(struct i2c_client *client,
> - const struct i2c_device_id *id)
> +static int adv748x_probe(struct i2c_client *client)
> {
> struct adv748x_state *state;
> int ret;
> @@ -806,13 +805,6 @@ static int adv748x_remove(struct i2c_client *client)
> return 0;
> }
>
> -static const struct i2c_device_id adv748x_id[] = {
> - { "adv7481", 0 },
> - { "adv7482", 0 },
> - { },
> -};
> -MODULE_DEVICE_TABLE(i2c, adv748x_id);
> -
> static const struct of_device_id adv748x_of_table[] = {
> { .compatible = "adi,adv7481", },
> { .compatible = "adi,adv7482", },
> @@ -825,9 +817,8 @@ static struct i2c_driver adv748x_driver = {
> .name = "adv748x",
> .of_match_table = adv748x_of_table,
> },
> - .probe = adv748x_probe,
> + .probe_new = adv748x_probe,
> .remove = adv748x_remove,
> - .id_table = adv748x_id,
> };
>
> module_i2c_driver(adv748x_driver);
> --
> 2.20.1
>
--
Regards,
Niklas Söderlund
Re: [PATCH 0/2] Use Media Dev Allocator to fix vimc dev lifetime bugs
removed > > vimc_device and the embedded media_device goes with it, while the active > > stream and vimc_capture continue to access it. > > The issue isn't so much that media_devic is embedded in vimc_device, but > that vimc_device is released too early. Not only does the thread need to > access the graph_mutex lock in the media_device structure, but it can > potentially access fields of the device-specific structures as well. The > proper solution is to propagate media_device_release() one level up, in > order to only release the top-level structure containing media_device > when the last reference to the media_device is dropped. I have seen similar problems with rcar-vin, the device specific data is released to early. In my case it was not triggered by the struct media_device but with a struct v4l2_device embedded in the device specific data IIRC. This was when I tried to address the lifetime issues of the video device when binding/unbinding the device to the driver and not when unloading the module. This was quiet a while ago so I don't recall specifics, sorry about that. One finding was that there are also unsolved problems when it comes async notifiers and unloading/unbinding and then loading/binding subdevices as well as the driver controlling the video device. It was such a mess I gave up. I'm happy to see activity in this area but I fear it might need work on a higher level and not trying to work around the problem in drivers. > > > If we chose to keep these drivers as component drivers, media device > > needs to stick around until all components stop using it. This is tricky > > because there is no tie between these set of drivers. vimc module can > > be deleted while others are still active. As vimc gets removed, other > > component drivers start wanting to access the media device tree. > > Reference-counting is the key. > > > This is classic media device lifetime problem which could be solved > > easily with the way I solved it with this series. I saw this as a > > variation on the same use-case we had with sound and media drivers > > sharing the media device. > > This isn't about solving it easily, it's about solving it properly. The > media device allocator as used here is a hack and takes us in the > opposite direction of a proper fix. > > > I have a TODO request from you asking to extend Media Device Allocator > > API to generic case and not restrict it to USB devices. My thinking is > > that this gives a perfect test case to extend the API to be generic > > and use to solve this problem. > > The biggest issue at the moment with the media device allocator, which I > have pointed out numerous times and has never been addressed (and which > explains why I didn't think the code was ready to be merged) is that the > media_device contains operations that are based on having a single > driver controlling the media device. A proper shared media device > allocator needs to drop the concept of a single master for the media > device, and thus needs to refactor those operations to allow any user of > the media device to implement them (the .link_notify() operation is a > prime example, and the recently added request operations will make this > even more challenging - think of how this patch series would prevent > vimc from properly implementing the request API). As long as these issue > are not fixed I will be firmly opposed to spreading the usage of the > media device allocator beyond what exists today. > > > Collapsing the drivers into one might be lot more difficult and complex > > than solving this problem with Media Device Allocator API. This approach > > has an added benefit of extending the API to be generic and not just for > > USB. > > I've never disputed the fact that fixing a problem correctly is usually > more work than hacking around it :-) > > > I looked at this as a good way to add generic API and have a great test > > case for it. This patch series fixes the problem for the current vimc > > architecture. > > NAK, for the reasons above. Please drop this series and fix the problem > properly. > > -- > Regards, > > Laurent Pinchart -- Regards, Niklas Söderlund
Re: [PATCH v7 0/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()
Hi,
This patch breaks rcar-vin. I'm sorry I did not find out before it was
merged as a8fa55078a7784a9 ("media: v4l2-subdev: Verify arguments in
v4l2_subdev_call()").
The problem is that rcar-vin calls enum_mbus_code in its bound callback.
At this point call_enum_mbus_code() is invoked which then calls
check_pad(). At this point sd->entity.graph_obj.mdev is not set so the
check if (pad > 0) fails and the binding of the subdevice in rcar-vin
fails.
I'm not sure how to best solve this, suggestions are appreciated. I see
two options, move the call to enum_mbus_code from the bound to the
complete callback or make sure the mdev is associated with the subdev
before the bound callback is invoked. I don't like the former as I think
the complete callback should be removed ;-)
On 2019-05-20 23:27:44 +0200, Janusz Krzysztofik wrote:
> Correctness of format type (try or active) and pad ID parameters passed
> to subdevice operation callbacks is now verified only for IOCTL calls.
> However, those callbacks are also used by drivers, e.g., V4L2 host
> interfaces.
>
> Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call()
> macro while calling subdevice operations, move those parameter checks
> from subdev_do_ioctl() to v4l2_subdev_call(). Also, add check for
> non-NULL pointers, including pad config if V4L2_SUBDEV_FORMAT_TRY is
> requested.
>
> Having that done, we can avoid taking care of those checks inside
> drivers.
>
> Janusz Krzysztofik (3):
> media: v4l2-subdev: Verify arguments in v4l2_subdev_call()
> media: v4l2-subdev: Verify v4l2_subdev_call() pointer arguments
> media: v4l2-subdev: Verify v4l2_subdev_call() pad config argument
>
> drivers/media/v4l2-core/v4l2-subdev.c | 268 +-
> include/media/v4l2-subdev.h | 6 +
> 2 files changed, 188 insertions(+), 86 deletions(-)
>
> Changelog:
> v6->v7:
> Changes suggested by Sakari - thanks!
> - never succeed pad check on media entities with pad_num == 0,
> - allow pad 0 on subdevies not registered as media entities.
>
> v5->v6:
> - rename wrappers to call_something() as suggested by Sakari - thanks!
> - make check_ functions inline - also on Sakari's suggestion, thanks!
> - drop patch 2/4 and remove WARN_ONs from remaining patches to avoid
> kernel WARNs on non-kernel bugs - thanks Hans for pointing this out!
>
> v4->v5:
> - a few coding style and code formatting changes,
> - require CONFIG_MEDIA_CONTROLLER, not CONFIG_VIDEO_V4L2_SUBDEV_API,
> for a valid pad ID check,
> - perform pad ID check only if at least one pad is configured so
> drivers which don't configure pads are not affected if built with
> CONFIG_MEDIA_CONTROLLER defined,
> - issue kernel warnings on invalid parameters (new patch - 2/4),
> - validate pointers before using them (new patch - 3/4).
>
> v3->v4:
> - fix 'struct' keyword missing from patch 2/2,
> - fix checkpatch reported style issue in patch 2/2
> Sorry for that.
>
> v2->v3:
> - add patch 2/2 with pad config check,
> - adjust continuation line alignments in patch 1/2 to match those
> used in 2/2.
>
> v1->v2:
> - replace the horrible macro with a structure of wrapper functions;
> inspired by Hans' and Sakari's comments - thanks!
>
> --
> 2.21.0
>
Re: linux-next: Fixes tag needs some work in the v4l-dvb tree
Hi Stephen, Mauro,
On 2019-05-29 08:04:54 +1000, Stephen Rothwell wrote:
> Hi Mauro,
>
> In commit
>
> 0c310868826e ("media: rcar-csi2: Fix coccinelle warning for
> PTR_ERR_OR_ZERO()")
>
> Fixes tag
>
> Fixes: 3ae854cafd76 ("rcar-csi2: Use standby mode instead of resetting")
>
> has these problem(s):
>
> - Target SHA1 does not exist
>
> Did you mean
>
> Fixes: d245a940d97b ("media: rcar-csi2: Use standby mode instead of
> resetting")
Yes I meant d245a940d97b commit, for some reason I was on the wrong
branch when looking up the sha1 for the fixes tag and used one from a
local development branch.
This is my mess, sorry about that. What can I do to help fix it?
--
Regards,
Niklas Söderlund
Re: [PATCH] media: i2c: adv748x: initialize bit 7 of csi_tx_top_reg_1f
Hi Kieran and Michael,
On 2019-05-10 17:25:24 +0100, Kieran Bingham wrote:
>
> Niklas, how does RCar-CSI determine the top/bottom sequence?
That patch just got merged in the media tree a few days ago,
9f7983bdc4925ae2 ("media: rcar-csi2: Propagate the FLD signal for NTSC and
PAL")
>
> Do we have field inversion currently? (or one which is perhaps swapped
> somewhere along the pipeline in rcar-vin?)
>
I'm not sure which tree this patch is developed on but Steve Longerbeam
posted a RFC which IMHO had the fields inverted, there was a discussion
in his thread [1] where I tried to get to the bottom of the issue. My
conclusions there might be wrong due to the issues addressed in this
patch.
Michael: Did you have Steve's patch in your tree when testing this?
1. https://patchwork.kernel.org/patch/10904263/
--
Regards,
Niklas Söderlund
Re: [PATCH v4 0/2] thermal: rcar_gen3_thermal: fix IRQ issues
Hi Eugeniu, On 2019-05-10 12:42:31 +0200, Eugeniu Rosca wrote: > Hi Niklas, > > On Wed, May 08, 2019 at 01:54:03AM +0200, Niklas Söderlund wrote: > > Hi Jiada, > [..] > > I really like this series, nice work. > > > > Tested-by: Niklas Söderlund > > Reviewed-by: Niklas Söderlund > > Is there anything off-the-shelf available for testing the rcar3 > thermal driver, to avoid reinventing the wheel via > https://patchwork.kernel.org/cover/10913163/#22602335 Not that I know of, unfortunately :-( I have a private home hacked testing framework (don't we all?) based on tcl+expect where I have two basic tests for rcar_gen3_thermal. I'm willing to share the tests if you by chance want them, but be warned that they are highly specialised for my needs and I'm reluctant to publish my whole hack tool as it just a ugly hack ;-) On a high level the tests I have are 1. thermal-load Generates load on target and observes the temperature is increased using the /sys/class/thermal/thermal_zone*/temp" interface. This seems similar to the test case your reference using stress-ng. 2. thermal-cooling Emulate the passive trip point temperatures using the /sys/class/thermal/*/emul_temp interface and observe that the specified cooling state is achieved. I should add a third test to make sure IRQ fires but this is just a pet project for me so maybe I will get around to it sometime... If you know of anything around to test thermal drivers or if you create something please let me know so I can add it to my tests. And let me know if you want my hacks for inspiration for your own testing. -- Regards, Niklas Söderlund
Re: [PATCH v4 0/2] thermal: rcar_gen3_thermal: fix IRQ issues
Hi Jiada,
Thanks for your patches.
On 2019-04-24 14:11:43 +0900, Jiada Wang wrote:
> There are issues with interrupt handling in rcar_gen3_thermal driver.
>
> Currently IRQ is remain enabled after .remove, later if device is probed,
> IRQ is requested before .thermal_init, this may cause IRQ function be
> triggered but not able to clear IRQ status, thus cause system to hang.
>
> Since the irq line isn't shared between different devices,
> so the proper interrupt type flag should be IRQF_ONESHOT.
>
> This patch-set fix these interrupt handling retated issues.
I really like this series, nice work.
Tested-by: Niklas Söderlund
Reviewed-by: Niklas Söderlund
>
> ---
> v4: remove 'spinlock_t lock'
> add Fixes tag in ("thermal: rcar_gen3_thermal: fix interrupt type")
> fix typos in ("thermal: rcar_gen3_thermal: disable interrupt in .remove")
>
> v3: fix to use correct code base
> remove unused "flag" variable in rcar_gen3_thermal_irq
>
> v2: use irq type IRQF_ONESHOT instead of IRQF_SHARED
> disable interrupt in .remove
>
> v1: initial version
>
> Jiada Wang (2):
> thermal: rcar_gen3_thermal: fix interrupt type
> thermal: rcar_gen3_thermal: disable interrupt in .remove
>
> drivers/thermal/rcar_gen3_thermal.c | 41 +++--
> 1 file changed, 9 insertions(+), 32 deletions(-)
>
> --
> 2.19.2
>
--
Regards,
Niklas Söderlund
Re: [PATCH v4 0/2] thermal: rcar_gen3_thermal: fix IRQ issues
Hi Jiada,
I think this series looks good. Unfortunately I'm out of office for the
next week and are thus unable to test it. Please don't let this block
this series but if it's still on the ML when I'm back I will do a proper
review and test of it.
On 2019-04-24 14:11:43 +0900, Jiada Wang wrote:
> There are issues with interrupt handling in rcar_gen3_thermal driver.
>
> Currently IRQ is remain enabled after .remove, later if device is probed,
> IRQ is requested before .thermal_init, this may cause IRQ function be
> triggered but not able to clear IRQ status, thus cause system to hang.
>
> Since the irq line isn't shared between different devices,
> so the proper interrupt type flag should be IRQF_ONESHOT.
>
> This patch-set fix these interrupt handling retated issues.
>
> ---
> v4: remove 'spinlock_t lock'
> add Fixes tag in ("thermal: rcar_gen3_thermal: fix interrupt type")
> fix typos in ("thermal: rcar_gen3_thermal: disable interrupt in .remove")
>
> v3: fix to use correct code base
> remove unused "flag" variable in rcar_gen3_thermal_irq
>
> v2: use irq type IRQF_ONESHOT instead of IRQF_SHARED
> disable interrupt in .remove
>
> v1: initial version
>
> Jiada Wang (2):
> thermal: rcar_gen3_thermal: fix interrupt type
> thermal: rcar_gen3_thermal: disable interrupt in .remove
>
> drivers/thermal/rcar_gen3_thermal.c | 41 +++--
> 1 file changed, 9 insertions(+), 32 deletions(-)
>
> --
> 2.19.2
>
--
Regards,
Niklas Söderlund
Re: [RESEND PATCH] thermal: rcar_gen3_thermal: Fix init value of IRQCTL register
Hi Hoan, Thanks for your work, and sorry for dropping the ball on this in v2. On 2019-03-27 18:03:18 +0900, Nguyen An Hoan wrote: > From: Hoan Nguyen An > > Fix setting value for IRQCTL register. We are setting the last 6 bits > of (IRQCTL) to be 1 (0x3f), this is only suitable for H3ES1.*, according > to Hardware manual values 1 are "setting prohibited" for Gen3. > > Signed-off-by: Hoan Nguyen An Reviewed-by: Niklas Söderlund > --- > drivers/thermal/rcar_gen3_thermal.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/thermal/rcar_gen3_thermal.c > b/drivers/thermal/rcar_gen3_thermal.c > index 88fa41c..2482795 100644 > --- a/drivers/thermal/rcar_gen3_thermal.c > +++ b/drivers/thermal/rcar_gen3_thermal.c > @@ -307,7 +307,7 @@ static void rcar_gen3_thermal_init(struct > rcar_gen3_thermal_tsc *tsc) > > usleep_range(1000, 2000); > > - rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F); > + rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0); > rcar_gen3_thermal_write(tsc, REG_GEN3_IRQMSK, 0); > rcar_gen3_thermal_write(tsc, REG_GEN3_IRQEN, IRQ_TEMPD1 | IRQ_TEMP2); > > -- > 2.7.4 > -- Regards, Niklas Söderlund
Re: [PATCH] media: rcar-vin: fix a potential NULL pointer dereference
Hi Kangjie, Thanks for your patch. On 2019-03-09 01:05:27 -0600, Kangjie Lu wrote: > In case of_match_node cannot find a match, the fix returns > -EINVAL to avoid NULL pointer dereference. > > Signed-off-by: Kangjie Lu > --- > drivers/media/platform/rcar-vin/rcar-core.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c > b/drivers/media/platform/rcar-vin/rcar-core.c > index f0719ce24b97..a058e2023ca8 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -266,6 +266,8 @@ static int rvin_group_init(struct rvin_group *group, > struct rvin_dev *vin) > > match = of_match_node(vin->dev->driver->of_match_table, > vin->dev->of_node); > + if (unlikely(!match)) > + return -EINVAL; I don't think this is needed. The driver depends on selects OF and if we get this far we it is because we had a match already. The reason to call of_match_node() here is simply to retrieve which of the possible compatible strings was matched. Am I missing something? What scenario do you see where this can fail? > > strscpy(mdev->driver_name, KBUILD_MODNAME, sizeof(mdev->driver_name)); > strscpy(mdev->model, match->compatible, sizeof(mdev->model)); > -- > 2.17.1 > -- Regards, Niklas Söderlund
Re: [PATCH] media: i2c: adv748x: select V4L2_FWNODE
Hi Arnd,
Thanks for your work.
On 2019-03-05 14:23:13 +0100, Arnd Bergmann wrote:
> Building adv748x fails now unless V4L2_FWNODE is selected:
>
> drivers/media/i2c/adv748x/adv748x-core.o: In function `adv748x_probe':
> adv748x-core.c:(.text+0x1b2c): undefined reference to
> `v4l2_fwnode_endpoint_parse'
>
> Fixes: 6a18865da8e3 ("media: i2c: adv748x: store number of CSI-2 lanes
> described in device tree")
> Signed-off-by: Arnd Bergmann
Reviewed-by: Niklas Söderlund
> ---
> drivers/media/i2c/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 6d32f8dcf83b..3f5dd80e14f8 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -221,6 +221,7 @@ config VIDEO_ADV748X
> depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> depends on OF
> select REGMAP_I2C
> + select V4L2_FWNODE
> ---help---
> V4L2 subdevice driver for the Analog Devices
> ADV7481 and ADV7482 HDMI/Analog video decoders.
> --
> 2.20.0
>
--
Regards,
Niklas Söderlund
Re: [PATCH] au0828: minor fix to a misleading comment in _close()
Hi Shuah, Thanks for your patch. On 2019-02-22 10:45:59 -0700, Shuah Khan wrote: > Fix misleading comment in _close() > > Signed-off-by: Shuah Khan > --- > drivers/media/usb/au0828/au0828-video.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/usb/au0828/au0828-video.c > b/drivers/media/usb/au0828/au0828-video.c > index 7876c897cc1d..08f566006a1f 100644 > --- a/drivers/media/usb/au0828/au0828-video.c > +++ b/drivers/media/usb/au0828/au0828-video.c > @@ -1074,7 +1074,7 @@ static int au0828_v4l2_close(struct file *filp) >* so the s_power callback are silently ignored. >* So, the current logic here does the following: >* Disable (put tuner to sleep) when > - * - ALSA and DVB aren't not streaming; > + * - ALSA and DVB aren't streaming; Nit-picking, as you are modifying the line anyhow I would s/;/./ With or without this changed, Reviewed-by: Niklas Söderlund >* - the last V4L2 file handler is closed. > * >* FIXME: > -- > 2.17.1 > -- Regards, Niklas Söderlund
Re: [RFC PATCH v2] media: rcar-vin: Allow independent VIN link enablement
Hi Steve,
Thanks for your patch, I think it looks good.
On 2019-01-06 13:20:18 -0800, Steve Longerbeam wrote:
> There is a block of code in rvin_group_link_notify() that loops through
> all entities in the media graph, and prevents enabling a link to a VIN
> node if any entity is in use. This prevents enabling a VIN link even if
> there is an in-use entity somewhere in the graph that is independent of
> the link's pipeline.
>
> For example, the code will prevent enabling a link from the first
> rcar-csi2 receiver to a VIN node even if there is an enabled link
> somewhere far upstream on the second independent rcar-csi2 receiver
> pipeline.
>
> If this code is meant to prevent modifying a link if any entity in the
> graph is actively involved in streaming (because modifying the CHSEL
> register fields can disrupt any/all running streams), then the entities
> stream counts should be checked rather than the use counts.
>
> (There is already such a check in __media_entity_setup_link() that verifies
> the stream_count of the link's source and sink entities are both zero,
> but that is insufficient, since there should be no running streams in
> the entire graph).
>
> Modify the media_device_for_each_entity() loop to check the entity
> stream_count instead of the use_count, and elaborate on the comment.
> VIN node links can now be enabled even if there are other independent
> in-use entities that are not streaming.
>
> Fixes: c0cc5aef31 ("media: rcar-vin: add link notify for Gen3")
>
> Signed-off-by: Steve Longerbeam
Reviewed-by: Niklas Söderlund
> ---
> Changes in v2:
> - bring back the media_device_for_each_entity() loop but check the
> stream_count not the use_count.
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c
> index f0719ce24b97..6dd6b11c1b2b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -131,9 +131,13 @@ static int rvin_group_link_notify(struct media_link
> *link, u32 flags,
> !is_media_entity_v4l2_video_device(link->sink->entity))
> return 0;
>
> - /* If any entity is in use don't allow link changes. */
> + /*
> + * Don't allow link changes if any entity in the graph is
> + * streaming, because modifying the CHSEL register fields
> + * can disrupt running streams.
> + */
> media_device_for_each_entity(entity, &group->mdev)
> - if (entity->use_count)
> + if (entity->stream_count)
> return -EBUSY;
>
> mutex_lock(&group->lock);
> --
> 2.17.1
>
--
Regards,
Niklas Söderlund
Re: [RFC PATCH] media: rcar-vin: Allow independent VIN link enablement
Hi Steve,
Thanks for your patch.
On 2018-12-25 15:27:25 -0800, Steve Longerbeam wrote:
> There is a block of code in rvin_group_link_notify() that prevents
> enabling a link to a VIN node if any entity in the media graph is
> in use. This prevents enabling a VIN link even if there is an in-use
> entity somewhere in the graph that is independent of the link's
> pipeline.
>
> For example, the code block will prevent enabling a link from
> the first rcar-csi2 receiver to a VIN node even if there is an
> enabled link somewhere far upstream on the second independent
> rcar-csi2 receiver pipeline.
Unfortunately this is by design and needed due to the hardware design.
The different VIN endpoints shares a configuration register which
controls the routing from the CSI-2 receivers to the VIN (register name
CHSEL). Modifying the CHSEL register which is what happens when a link
is enabled/disabled can have side effects on running streams even if
they are not shown to be dependent in the media graph.
There is a CHSEL register in VIN0 which controls the routing from all
CSI-2 receivers to VIN0-3 and a CHSEL register in VIN4 which controls
the same for VIN4-7.
>
> If this code block is meant to prevent modifying a link if the
> link is actively involved in streaming, there is already such a
> check in __media_entity_setup_link() that verifies the stream_count
> of the link's source and sink entities are both zero.
For the reason above the check in __media_entity_setup_link() is not
enough :-( This register sharing is my least favorite thing about the
VIN on Gen3 and forces the driver to become more complex as all VIN
instances needs to know about each other and interact.
>
> Remove the code block so that VIN node links can be enabled even if
> there are other independent in-use entities.
There is room for some improvement in this area disregarding the odd
hardware design. It *could* be allowed to change a link terminating in
VIN4-7 even if there is a stream running for one or more in VIN0-3.
I would be interested to test such a patch but to allow any link change
which is allowed by __media_entity_setup_link() is unfortunately not
possible, as I understand it. Maybe someone more clever then me can find
ways to unlock even more then just the split between VIN0-3 and VIn4-7.
In essence the CHSEL register can not be changed if it's involved in a
running pipeline even if the end result would be that the running
pipeline would look the same. This is possible as there are multiple
CHSEL settings where the same source is connected to a specific VIN
while other members of the "subgroup of VINs" (e.g. VIN0-3) is routed to
something else for the two CHSEL settings.
>
> Fixes: c0cc5aef31 ("media: rcar-vin: add link notify for Gen3")
>
> Signed-off-by: Steve Longerbeam
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c
> index f0719ce24b97..b2c9a876969e 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -116,7 +116,6 @@ static int rvin_group_link_notify(struct media_link
> *link, u32 flags,
> struct rvin_group, mdev);
> unsigned int master_id, channel, mask_new, i;
> unsigned int mask = ~0;
> - struct media_entity *entity;
> struct video_device *vdev;
> struct media_pad *csi_pad;
> struct rvin_dev *vin = NULL;
> @@ -131,11 +130,6 @@ static int rvin_group_link_notify(struct media_link
> *link, u32 flags,
> !is_media_entity_v4l2_video_device(link->sink->entity))
> return 0;
>
> - /* If any entity is in use don't allow link changes. */
> - media_device_for_each_entity(entity, &group->mdev)
> - if (entity->use_count)
> - return -EBUSY;
> -
> mutex_lock(&group->lock);
>
> /* Find the master VIN that controls the routes. */
> --
> 2.17.1
>
--
Regards,
Niklas Söderlund
Re: [PATCH] media: rcar-csi2: Add support for RZ/G2E
Hi Fabrizio,
Thanks.
On 2018-12-13 20:24:12 +, Fabrizio Castro wrote:
> According to the RZ/G2 User's manual, RZ/G2E and R-Car E3 CSI-2
> blocks are identical, therefore use R-Car E3 definitions to add
> RZ/G2E support.
>
> Signed-off-by: Fabrizio Castro
Reviewed-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-csi2.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 80ad906..be7508b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -979,6 +979,10 @@ static const struct rcar_csi2_info
> rcar_csi2_info_r8a77990 = {
>
> static const struct of_device_id rcar_csi2_of_table[] = {
> {
> + .compatible = "renesas,r8a774c0-csi2",
> + .data = &rcar_csi2_info_r8a77990,
> + },
> + {
> .compatible = "renesas,r8a7795-csi2",
> .data = &rcar_csi2_info_r8a7795,
> },
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH] media: dt-bindings: rcar-vin: Add R8A774C0 support
Hi Fabrizio, Thanks for your patch. On 2018-12-13 20:21:59 +, Fabrizio Castro wrote: > Add the compatible string for RZ/G2E (a.k.a. R8A774C0) to the list > of SoCs supported by rcar-vin driver. > > Signed-off-by: Fabrizio Castro Reviewed-by: Niklas Söderlund > --- > Documentation/devicetree/bindings/media/rcar_vin.txt | 9 + > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt > b/Documentation/devicetree/bindings/media/rcar_vin.txt > index 7c878ca..34dcd28 100644 > --- a/Documentation/devicetree/bindings/media/rcar_vin.txt > +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt > @@ -7,12 +7,13 @@ family of devices. > Each VIN instance has a single parallel input that supports RGB and YUV > video, > with both external synchronization and BT.656 synchronization for the latter. > Depending on the instance the VIN input is connected to external SoC pins, or > -on Gen3 platforms to a CSI-2 receiver. > +on Gen3 and RZ/G2 platforms to a CSI-2 receiver. > > - compatible: Must be one or more of the following > - "renesas,vin-r8a7743" for the R8A7743 device > - "renesas,vin-r8a7744" for the R8A7744 device > - "renesas,vin-r8a7745" for the R8A7745 device > + - "renesas,vin-r8a774c0" for the R8A774C0 device > - "renesas,vin-r8a7778" for the R8A7778 device > - "renesas,vin-r8a7779" for the R8A7779 device > - "renesas,vin-r8a7790" for the R8A7790 device > @@ -60,10 +61,10 @@ The per-board settings Gen2 platforms: > - data-enable-active: polarity of CLKENB signal, see [1] for >description. Default is active high. > > -The per-board settings Gen3 platforms: > +The per-board settings Gen3 and RZ/G2 platforms: > > -Gen3 platforms can support both a single connected parallel input source > -from external SoC pins (port@0) and/or multiple parallel input sources > +Gen3 and RZ/G2 platforms can support both a single connected parallel input > +source from external SoC pins (port@0) and/or multiple parallel input sources > from local SoC CSI-2 receivers (port@1) depending on SoC. > > - renesas,id - ID number of the VIN, VINx in the documentation. > -- > 2.7.4 > -- Regards, Niklas Söderlund
Re: [PATCH] media: rcar-vin: Add support for RZ/G2E
Hi Fabrizio,
Thanks for your work.
On 2018-12-13 20:24:04 +, Fabrizio Castro wrote:
> According to the RZ/G2 User's manual, RZ/G2E and R-Car E3 VIN
> blocks are identical, therefore use R-Car E3 definitions to add
> RZ/G2E support.
>
> Signed-off-by: Fabrizio Castro
Reviewed-by: Niklas Söderlund
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c
> index cae2166..137edad 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -1118,6 +1118,10 @@ static const struct rvin_info rcar_info_r8a77995 = {
>
> static const struct of_device_id rvin_of_id_table[] = {
> {
> + .compatible = "renesas,vin-r8a774c0",
> + .data = &rcar_info_r8a77990,
> + },
> + {
> .compatible = "renesas,vin-r8a7778",
> .data = &rcar_info_m1,
> },
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH] media: dt-bindings: rcar-csi2: Add r8a774c0
Hi Fabrizio, Thanks for your work. On 2018-12-13 20:21:49 +, Fabrizio Castro wrote: > Add the compatible string for RZ/G2E (a.k.a. R8A774C0) to the > list of supported SoCs. > > Signed-off-by: Fabrizio Castro Reviewed-by: Niklas Söderlund > --- > Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt > b/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt > index 2824489..11cf38d 100644 > --- a/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt > +++ b/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt > @@ -2,12 +2,13 @@ Renesas R-Car MIPI CSI-2 > > > The R-Car CSI-2 receiver device provides MIPI CSI-2 capabilities for the > -Renesas R-Car family of devices. It is used in conjunction with the > +Renesas R-Car and RZ/G2 family of devices. It is used in conjunction with the > R-Car VIN module, which provides the video capture capabilities. > > Mandatory properties > > - compatible: Must be one or more of the following > + - "renesas,r8a774c0-csi2" for the R8A774C0 device. > - "renesas,r8a7795-csi2" for the R8A7795 device. > - "renesas,r8a7796-csi2" for the R8A7796 device. > - "renesas,r8a77965-csi2" for the R8A77965 device. > -- > 2.7.4 > -- Regards, Niklas Söderlund
Re: [PATCH 2/3] thermal: rcar_thermal: Prevent doing work after unbind
Hi Geert,
Thanks for your work.
On 2018-09-27 13:32:34 +0200, Geert Uytterhoeven wrote:
> When testing bind/unbind on r8a7791/koelsch:
>
> WARNING: CPU: 1 PID: 697 at lib/debugobjects.c:329
> debug_print_object+0x8c/0xb4
> ODEBUG: free active (active state 0) object type: timer_list hint:
> delayed_work_timer_fn+0x0/0x10
>
> This happens if the workqueue runs after the device has been unbound.
> Fix this by cancelling any queued work during remove.
>
> Fixes: e0a5172e9eec7f0d ("thermal: rcar: add interrupt support")
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Niklas Söderlund
> ---
> drivers/thermal/rcar_thermal.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> index ea132e122b174757..616ba2fccf410d3b 100644
> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -453,6 +453,7 @@ static int rcar_thermal_remove(struct platform_device
> *pdev)
>
> rcar_thermal_for_each_priv(priv, common) {
> rcar_thermal_irq_disable(priv);
> + cancel_delayed_work_sync(&priv->work);
> if (priv->chip->use_of_thermal)
> thermal_remove_hwmon_sysfs(priv->zone);
> else
> --
> 2.17.1
>
--
Regards,
Niklas Söderlund
Re: [PATCH v2 3/3] arm64: dts: renesas: draak: Describe HDMI input
Hi Jacopo,
Thanks for your patch.
On 2018-05-16 15:42:09 +0200, Jacopo Mondi wrote:
> Describe HDMI input connector and ADV7612 HDMI decoder installed on
> R-Car Gen3 Draak board.
>
> The video signal routing to the HDMI decoder to the video input interface
> VIN4 is multiplexed with CVBS input path, and enabled/disabled through
> on-board switches SW-49, SW-50, SW-51 and SW-52.
>
> As the default board switches configuration connects CVBS input to VIN4,
> leave the HDMI decoder unconnected in DTS.
>
> Signed-off-by: Jacopo Mondi
I'm not sure we have a policy about describing hardware which can't be
used without flipping switches. I have no opinion on if we should do
that or not I leave that to others, but for the change itself.
Reviewed-by: Niklas Söderlund
I think it's good we describe it as it's part of the Draak board itself
and not an expansion board which we have seen a lot of :-) Maybe even
add a commented out line in the adv7612 port@2 which hints which VIN
this is connected to if the switches are flipped?
> ---
> arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 38
> ++
> 1 file changed, 38 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> index 9aba28f..ea99dc9 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> @@ -59,6 +59,17 @@
> };
> };
>
> + hdmi-in {
> + compatible = "hdmi-connector";
> + type = "a";
> +
> + port {
> + hdmi_con_in: endpoint {
> + remote-endpoint = <&adv7612_in>;
> + };
> + };
> + };
> +
> memory@4800 {
> device_type = "memory";
> /* first 128MB is reserved for secure area. */
> @@ -170,6 +181,33 @@
> };
> };
> };
> +
> + hdmi-decoder@4c {
> + compatible = "adi,adv7612";
> + reg = <0x4c>;
> + default-input = <0>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + adv7612_in: endpoint {
> + remote-endpoint = <&hdmi_con_in>;
> + };
> + };
> +
> + port@2 {
> + reg = <2>;
> + adv7612_out: endpoint {
> + pclk-sample = <0>;
> + hsync-active = <0>;
> + vsync-active = <0>;
> + };
> + };
> + };
> + };
> };
>
> &i2c1 {
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH v2 2/3] arm64: dts: renesas: draak: Describe CVBS input
Hi Jacopo,
Thanks for your patch.
On 2018-05-16 15:42:08 +0200, Jacopo Mondi wrote:
> Describe CVBS video input through analog video decoder ADV7180
> connected to video input interface VIN4.
>
> The video input signal path is shared with HDMI video input, and
> selected by on-board switches SW-53 and SW-54 with CVBS input selected
> by the default switches configuration.
You are missing your SoB line :-)
Reviewed-by: Niklas Söderlund
> ---
> arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 36
> ++
> 1 file changed, 36 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> index d03f194..9aba28f 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> @@ -142,6 +142,11 @@
> groups = "usb0";
> function = "usb0";
> };
> +
> + vin4_pins_cvbs: vin4 {
> + groups = "vin4_data8", "vin4_sync", "vin4_clk";
> + function = "vin4";
> + };
> };
>
> &i2c0 {
> @@ -154,6 +159,17 @@
> reg = <0x50>;
> pagesize = <8>;
> };
> +
> + analog-video@20 {
> + compatible = "adi,adv7180";
> + reg = <0x20>;
> +
> + port {
> + adv7180_out: endpoint {
> + remote-endpoint = <&vin4_in>;
> + };
> + };
> + };
> };
>
> &i2c1 {
> @@ -246,3 +262,23 @@
> timeout-sec = <60>;
> status = "okay";
> };
> +
> +&vin4 {
> + pinctrl-0 = <&vin4_pins_cvbs>;
> + pinctrl-names = "default";
> +
> + status = "okay";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + vin4_in: endpoint {
> + remote-endpoint = <&adv7180_out>;
> + };
> + };
> + };
> +};
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH 3/3] arm64: dts: renesas: draak: Describe HDMI input
Hi Jacopo,
On 2018-05-14 09:39:34 +0200, Jacopo Mondi wrote:
> Hi Niklas,
>
> On Sun, May 13, 2018 at 02:57:55PM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your patch.
> >
> > On 2018-05-11 12:00:02 +0200, Jacopo Mondi wrote:
> > > Describe HDMI input connected to VIN4 interface for R-Car D3 Draak
> > > development board.
> > >
> > > Signed-off-by: Jacopo Mondi
> > > ---
> > > arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 68
> > > ++
> > > 1 file changed, 68 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> > > b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> > > index d03f194..e0ce462 100644
> > > --- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> > > +++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> > > @@ -59,6 +59,17 @@
> > > };
> > > };
> > >
> > > + hdmi-in {
> > > + compatible = "hdmi-connector";
> > > + type = "a";
> > > +
> > > + port {
> > > + hdmi_con_in: endpoint {
> > > + remote-endpoint = <&adv7612_in>;
> > > + };
> > > + };
> > > + };
> > > +
> > > memory@4800 {
> > > device_type = "memory";
> > > /* first 128MB is reserved for secure area. */
> > > @@ -142,6 +153,11 @@
> > > groups = "usb0";
> > > function = "usb0";
> > > };
> > > +
> > > + vin4_pins: vin4 {
> > > + groups = "vin4_data24", "vin4_sync", "vin4_clk", "vin4_clkenb";
> > > + function = "vin4";
> > > + };
> > > };
> > >
> > > &i2c0 {
> > > @@ -154,6 +170,35 @@
> > > reg = <0x50>;
> > > pagesize = <8>;
> > > };
> > > +
> > > + hdmi-decoder@4c {
> > > + compatible = "adi,adv7612";
> > > + reg = <0x4c>;
> > > + default-input = <0>;
> > > +
> > > + ports {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + port@0 {
> > > + reg = <0>;
> > > + adv7612_in: endpoint {
> > > + remote-endpoint = <&hdmi_con_in>;
> > > + };
> > > + };
> > > +
> > > + port@2 {
> > > + reg = <2>;
> > > + adv7612_out: endpoint {
> > > + pclk-sample = <0>;
> > > + hsync-active = <0>;
> > > + vsync-active = <0>;
> >
> > This differs from the Gen2 DT bindings which is a very similar hardware
> > setup using the same components. Defining these properties will make the
> > bus marked as V4L2_MBUS_PARALLEL instead of V4L2_MBUS_BT656.
>
> And that's what we want
>
> >
> > This will change how the hardware is configured for capture if the media
> > bus is in a UYVY format, see VNMC_INF register in rvin_setup(). Maybe
> > this it not an issue here but still I'm curious to why this differ
> > between Gen2 and Gen3 :-)
>
> Actually this won't impact the VIN configuration as this is the
> 'remote endpoint' from VIN perspective and the properties used to
> configure the interface are the ones in the 'local endpoint'.
You are right, sorry for the confusion and thanks for educating me :-)
>
> >
> > > +
> > > + remote-endpoint = <&vin4_in>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > };
> > >
> > > &i2c1 {
> > > @@ -246,3 +291,26 @@
> > > timeout-sec = <60>;
> > > status = "okay";
> > > };
> > > +
> > > +&vin4 {
> > > + pinctrl-0 = <&vin4_pins>;
> > > + pinctrl-names = "default";
> > > +
> > > + status = "
Re: [PATCH 3/3] arm64: dts: renesas: draak: Describe HDMI input
Hi again,
On 2018-05-14 11:49:00 +0200, Niklas Söderlund wrote:
> Hi Laurent,
>
> On 2018-05-14 05:49:41 +0300, Laurent Pinchart wrote:
>
> [snip]
>
> > > > +&vin4 {
> > > > + pinctrl-0 = <&vin4_pins>;
> > > > + pinctrl-names = "default";
> > > > +
> > > > + status = "okay";
> > > > +
> > > > + ports {
> > > > + #address-cells = <1>;
> > > > + #size-cells = <0>;
> > > > +
> > > > + port@0 {
> > > > + reg = <0>;
> > > > +
> > > > + vin4_in: endpoint {
> > > > + hsync-active = <0>;
> > > > + vsync-active = <0>;
> > >
> > > Comparing this to the Gen2 bindings some properties are missing,
> > >
> > > bus-width = <24>;
> > > pclk-sample = <1>;
> > > data-active = <1>;
> > >
> > > This is not a big deal as the VIN driver don't use these properties so
> > > no functional change should come of this but still a difference.
> >
> > I think the VIN DT bindings should be updated to explicitly list the
> > endpoint
> > properties that are mandatory, optional, or not allowed.
>
> I think it's documented as it reference video-interfaces.txt which lists
> all these properties as optional. And in deed they are all optional. If
> the VIN driver makes use of all the optional ones is another matter. How
> do we know that the remote subdevice is not looking at its remote
> endpoint for bus parameters not considered by the rcar-vin driver?
>
> The thing is that the rcar-vin driver only looks at the remote endpoint
> for these properties and ignores the on its local endpoint. Maybe some
> v4l2 framework change is needed here to make sure the bus properties are
> the same on both endpoints of a link. But I fear such a change would
> break a lot of stuff.
Jacopo pointed out this statement is untrue. The rcar-vin only looks at
it's local endpoint not the remote endpoint for it's bus parameters. The
callback provided to v4l2_async_notifier_parse_fwnode_endpoints()
confused me as the subdevice passed to it is the one describe the remote
endpoint while the v4l2_fwnode_endpoint argument is that of the local
endpoint. Sorry for the confusion and thanks Jacopo for correcting me.
--
Regards,
Niklas Söderlund
Re: [PATCH 3/3] arm64: dts: renesas: draak: Describe HDMI input
Hi Laurent,
On 2018-05-14 05:49:41 +0300, Laurent Pinchart wrote:
[snip]
> > > +&vin4 {
> > > + pinctrl-0 = <&vin4_pins>;
> > > + pinctrl-names = "default";
> > > +
> > > + status = "okay";
> > > +
> > > + ports {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + port@0 {
> > > + reg = <0>;
> > > +
> > > + vin4_in: endpoint {
> > > + hsync-active = <0>;
> > > + vsync-active = <0>;
> >
> > Comparing this to the Gen2 bindings some properties are missing,
> >
> > bus-width = <24>;
> > pclk-sample = <1>;
> > data-active = <1>;
> >
> > This is not a big deal as the VIN driver don't use these properties so
> > no functional change should come of this but still a difference.
>
> I think the VIN DT bindings should be updated to explicitly list the endpoint
> properties that are mandatory, optional, or not allowed.
I think it's documented as it reference video-interfaces.txt which lists
all these properties as optional. And in deed they are all optional. If
the VIN driver makes use of all the optional ones is another matter. How
do we know that the remote subdevice is not looking at its remote
endpoint for bus parameters not considered by the rcar-vin driver?
The thing is that the rcar-vin driver only looks at the remote endpoint
for these properties and ignores the on its local endpoint. Maybe some
v4l2 framework change is needed here to make sure the bus properties are
the same on both endpoints of a link. But I fear such a change would
break a lot of stuff.
--
Regards,
Niklas Söderlund
Re: [PATCH 3/3] arm64: dts: renesas: draak: Describe HDMI input
Hi Jacopo,
Thanks for your patch.
On 2018-05-11 12:00:02 +0200, Jacopo Mondi wrote:
> Describe HDMI input connected to VIN4 interface for R-Car D3 Draak
> development board.
>
> Signed-off-by: Jacopo Mondi
> ---
> arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 68
> ++
> 1 file changed, 68 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> index d03f194..e0ce462 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> @@ -59,6 +59,17 @@
> };
> };
>
> + hdmi-in {
> + compatible = "hdmi-connector";
> + type = "a";
> +
> + port {
> + hdmi_con_in: endpoint {
> + remote-endpoint = <&adv7612_in>;
> + };
> + };
> + };
> +
> memory@4800 {
> device_type = "memory";
> /* first 128MB is reserved for secure area. */
> @@ -142,6 +153,11 @@
> groups = "usb0";
> function = "usb0";
> };
> +
> + vin4_pins: vin4 {
> + groups = "vin4_data24", "vin4_sync", "vin4_clk", "vin4_clkenb";
> + function = "vin4";
> + };
> };
>
> &i2c0 {
> @@ -154,6 +170,35 @@
> reg = <0x50>;
> pagesize = <8>;
> };
> +
> + hdmi-decoder@4c {
> + compatible = "adi,adv7612";
> + reg = <0x4c>;
> + default-input = <0>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + adv7612_in: endpoint {
> + remote-endpoint = <&hdmi_con_in>;
> + };
> + };
> +
> + port@2 {
> + reg = <2>;
> + adv7612_out: endpoint {
> + pclk-sample = <0>;
> + hsync-active = <0>;
> + vsync-active = <0>;
This differs from the Gen2 DT bindings which is a very similar hardware
setup using the same components. Defining these properties will make the
bus marked as V4L2_MBUS_PARALLEL instead of V4L2_MBUS_BT656.
This will change how the hardware is configured for capture if the media
bus is in a UYVY format, see VNMC_INF register in rvin_setup(). Maybe
this it not an issue here but still I'm curious to why this differ
between Gen2 and Gen3 :-)
> +
> + remote-endpoint = <&vin4_in>;
> + };
> + };
> + };
> + };
> };
>
> &i2c1 {
> @@ -246,3 +291,26 @@
> timeout-sec = <60>;
> status = "okay";
> };
> +
> +&vin4 {
> + pinctrl-0 = <&vin4_pins>;
> + pinctrl-names = "default";
> +
> + status = "okay";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + vin4_in: endpoint {
> + hsync-active = <0>;
> + vsync-active = <0>;
Comparing this to the Gen2 bindings some properties are missing,
bus-width = <24>;
pclk-sample = <1>;
data-active = <1>;
This is not a big deal as the VIN driver don't use these properties so
no functional change should come of this but still a difference.
Over all I'm happy with this change but before I add my tag I would like
to understand why it differs from the Gen2 configuration for the adv7612
properties.
Also on a side not it is possible with hardware switches on the board
switch the VIN4 source to a completely different pipeline CVBS connector
-> adv7180 -> VIN4. But I think it's best we keep the HDMI as default as
this seems to be how the boards are shipped. But maybe mentioning this
in the commit message would not hurt if you end-up resending the patch.
> +
> + remote-endpoint = <&adv7612_out>;
> + };
> + };
> + };
> +};
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH 3/3] arm64: dts: renesas: draak: Describe HDMI input
Hi Simon, On 2018-05-13 10:17:50 +0200, Simon Horman wrote: > On Fri, May 11, 2018 at 12:00:02PM +0200, Jacopo Mondi wrote: > > Describe HDMI input connected to VIN4 interface for R-Car D3 Draak > > development board. > > > > Signed-off-by: Jacopo Mondi > > Hi Niklas, > > As you reviewed the rest of the series I'm wondering if you're planning > to review this patch too. Yes, I did not have schematics for D3 on hand when reviewing the rest of the series. Will review it now that I do, thanks for the ping :-) -- Regards, Niklas Söderlund
Re: [PATCH 2/3] arm64: dts: renesas: r8a77995: Add VIN4
Hi Jacopo,
Thanks for your work.
On 2018-05-11 12:00:01 +0200, Jacopo Mondi wrote:
> Describe VIN4 interface for R-Car D3 R8A77995 SoC.
>
> Signed-off-by: Jacopo Mondi
Acked-by: Niklas Söderlund
> ---
> arch/arm64/boot/dts/renesas/r8a77995.dtsi | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77995.dtsi
> b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
> index 82aed7e..bdf7017 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77995.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
> @@ -783,6 +783,17 @@
> };
> };
> };
> +
> + vin4: video@e6ef4000 {
> + compatible = "renesas,vin-r8a77995";
> + reg = <0 0xe6ef4000 0 0x1000>;
> + interrupts = ;
> + clocks = <&cpg CPG_MOD 807>;
> + power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
> + resets = <&cpg 807>;
> + renesas,id = <4>;
> + status = "disabled";
> + };
> };
>
> timer {
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH 1/3] dt-bindings: media: rcar-vin: Add R8A77995 support
Hi Jacopo, Thanks for your work. On 2018-05-11 12:00:00 +0200, Jacopo Mondi wrote: > Add compatible string for R-Car D3 R8A7795 to list of SoCs supported by > rcar-vin driver. > > Signed-off-by: Jacopo Mondi Acked-by: Niklas Söderlund > --- > Documentation/devicetree/bindings/media/rcar_vin.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt > b/Documentation/devicetree/bindings/media/rcar_vin.txt > index a19517e1..5c6f2a7 100644 > --- a/Documentation/devicetree/bindings/media/rcar_vin.txt > +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt > @@ -22,6 +22,7 @@ on Gen3 platforms to a CSI-2 receiver. > - "renesas,vin-r8a7795" for the R8A7795 device > - "renesas,vin-r8a7796" for the R8A7796 device > - "renesas,vin-r8a77970" for the R8A77970 device > + - "renesas,vin-r8a77995" for the R8A77995 device > - "renesas,rcar-gen2-vin" for a generic R-Car Gen2 or RZ/G1 compatible > device. > > -- > 2.7.4 > -- Regards, Niklas Söderlund
Re: Potential problem with 31e77c93e432dec7 ("sched/fair: Update blocked load when newly idle")
Hi Vincent,
On 2018-04-26 17:27:24 +0200, Vincent Guittot wrote:
> Hi Niklas,
>
> >> Thanks for the trace, I have been able to catch a problem with it.
> >> Could you test the patch below to confirm that the problem is solved ?
> >> The patch apply on-top of
> >> c18bb396d3d261eb ("Merge
> >> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net")
> >
> > I can confirm that with the patch bellow I can no longer produce the
> > problem. Thanks!
>
> Thanks for testing
> Do you mind if I add
> Tested-by: Niklas Söderlund
Please do.
>
> Peter, Ingo,
> Do you want me to re-send the patch with all tags or you will take
> this version ?
>
> Regards,
> Vincent
>
> >
> >>
> >> From: Vincent Guittot
> >> Date: Thu, 26 Apr 2018 12:19:32 +0200
> >> Subject: [PATCH] sched/fair: fix the update of blocked load when newly idle
> >> MIME-Version: 1.0
> >> Content-Type: text/plain; charset=UTF-8
> >> Content-Transfer-Encoding: 8bit
> >>
> >> With commit 31e77c93e432 ("sched/fair: Update blocked load when newly
> >> idle"),
> >> we release the rq->lock when updating blocked load of idle CPUs. This open
> >> a time window during which another CPU can add a task to this CPU's cfs_rq.
> >> The check for newly added task of idle_balance() is not in the common path.
> >> Move the out label to include this check.
> >>
> >> Fixes: 31e77c93e432 ("sched/fair: Update blocked load when newly idle")
> >> Reported-by: Heiner Kallweit
> >> Reported-by: Niklas Söderlund
> >> Signed-off-by: Vincent Guittot
> >> ---
> >> kernel/sched/fair.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> index 0951d1c..15a9f5e 100644
> >> --- a/kernel/sched/fair.c
> >> +++ b/kernel/sched/fair.c
> >> @@ -9847,6 +9847,7 @@ static int idle_balance(struct rq *this_rq, struct
> >> rq_flags *rf)
> >> if (curr_cost > this_rq->max_idle_balance_cost)
> >> this_rq->max_idle_balance_cost = curr_cost;
> >>
> >> +out:
> >> /*
> >>* While browsing the domains, we released the rq lock, a task could
> >>* have been enqueued in the meantime. Since we're not going idle,
> >> @@ -9855,7 +9856,6 @@ static int idle_balance(struct rq *this_rq, struct
> >> rq_flags *rf)
> >> if (this_rq->cfs.h_nr_running && !pulled_task)
> >> pulled_task = 1;
> >>
> >> -out:
> >> /* Move the next balance forward */
> >> if (time_after(this_rq->next_balance, next_balance))
> >> this_rq->next_balance = next_balance;
> >> --
> >> 2.7.4
> >>
> >>
> >>
> >> [snip]
> >>
> >
> > --
> > Regards,
> > Niklas Söderlund
--
Regards,
Niklas Söderlund
Re: Potential problem with 31e77c93e432dec7 ("sched/fair: Update blocked load when newly idle")
Hi Vincent,
Thanks for all your help.
On 2018-04-26 12:31:33 +0200, Vincent Guittot wrote:
> Hi Niklas,
>
> Le Thursday 26 Apr 2018 à 00:56:03 (+0200), Niklas Söderlund a écrit :
> > Hi Vincent,
> >
> > Here are the result, sorry for the delay.
> >
> > On 2018-04-23 11:54:20 +0200, Vincent Guittot wrote:
> >
> > [snip]
> >
> > >
> > > Thanks for the report. Can you re run with the following trace-cmd
> > > sequence ? My previous sequence disables ftrace events
> > >
> > > trace-cmd reset > /dev/null
> > > trace-cmd start -b 4 -p function -l dump_backtrace:traceoff -e sched
> > > -e cpu_idle -e cpu_frequency -e timer -e ipi -e irq -e printk
> > > trace-cmd start -b 4 -p function -l dump_backtrace -e sched -e
> > > cpu_idle -e cpu_frequency -e timer -e ipi -e irq -e printk
> > >
> > > I have updated the patch and added traces to check that scheduler returns
> > > from idle_balance function and doesn't stay stuck
> >
> > Once more I applied the change bellow on-top of c18bb396d3d261eb ("Merge
> > git://git.kernel.org/pub/scm/linux/kernel/git/davem/net").
> >
> > This time the result of 'trace-cmd report' is so large I do not include
> > it here, but I attach the trace.dat file. Not sure why but the timing of
> > sending the NMI to the backtrace print is different (but content the
> > same AFIK) so in the odd change it can help figure this out:
> >
>
> Thanks for the trace, I have been able to catch a problem with it.
> Could you test the patch below to confirm that the problem is solved ?
> The patch apply on-top of
> c18bb396d3d261eb ("Merge
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net")
I can confirm that with the patch bellow I can no longer produce the
problem. Thanks!
>
> From: Vincent Guittot
> Date: Thu, 26 Apr 2018 12:19:32 +0200
> Subject: [PATCH] sched/fair: fix the update of blocked load when newly idle
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> With commit 31e77c93e432 ("sched/fair: Update blocked load when newly idle"),
> we release the rq->lock when updating blocked load of idle CPUs. This open
> a time window during which another CPU can add a task to this CPU's cfs_rq.
> The check for newly added task of idle_balance() is not in the common path.
> Move the out label to include this check.
>
> Fixes: 31e77c93e432 ("sched/fair: Update blocked load when newly idle")
> Reported-by: Heiner Kallweit
> Reported-by: Niklas Söderlund
> Signed-off-by: Vincent Guittot
> ---
> kernel/sched/fair.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 0951d1c..15a9f5e 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9847,6 +9847,7 @@ static int idle_balance(struct rq *this_rq, struct
> rq_flags *rf)
> if (curr_cost > this_rq->max_idle_balance_cost)
> this_rq->max_idle_balance_cost = curr_cost;
>
> +out:
> /*
>* While browsing the domains, we released the rq lock, a task could
>* have been enqueued in the meantime. Since we're not going idle,
> @@ -9855,7 +9856,6 @@ static int idle_balance(struct rq *this_rq, struct
> rq_flags *rf)
> if (this_rq->cfs.h_nr_running && !pulled_task)
> pulled_task = 1;
>
> -out:
> /* Move the next balance forward */
> if (time_after(this_rq->next_balance, next_balance))
> this_rq->next_balance = next_balance;
> --
> 2.7.4
>
>
>
> [snip]
>
--
Regards,
Niklas Söderlund
Re: media: rcar-vin: add group allocator functions
Hi Colin,
Thanks for reporting this. I wonder why smatch and sparse did not catch
this, the fault can't be mine for writing such a obviously bad thing
right :-)
I have a patch to address this, just need to test it before posting.
On 2018-04-24 14:14:02 +0100, Colin Ian King wrote:
> Hi there,
>
> While running static analysis on linux-next today a null pointer
> dereference issue was detected by CoverityScan. The following commit
> introduced the issue:
>
> commit 3bb4c3bc85bf77a76c921671800bde2e1bf82a88
> Author: Niklas Söderlund
> Date: Sat Apr 14 07:57:18 2018 -0400
>
> media: rcar-vin: add group allocator functions
>
> The analysis is as follows:
>
> 339 static void rvin_group_put(struct rvin_dev *vin)
> 340 {
> 341mutex_lock(&vin->group->lock);
> 342
>1. assign_zero: Assigning: vin->group = NULL.
>
> 343vin->group = NULL;
> 344vin->v4l2_dev.mdev = NULL;
> 345
> CID 1468359 (#1 of 1): Explicit null dereferenced (FORWARD_NULL)
> 2. var_deref_op: Dereferencing null pointer vin->group.
>
> 346if (WARN_ON(vin->group->vin[vin->id] != vin))
> 347goto out;
> 348
>
> vin->group is NULL however the WARN_ON is dereferencing it, causing an
> OOPS. I don't know how this should be fixed, hence I am sending this
> bug report.
>
> Regards,
>
> Colin
--
Regards,
Niklas Söderlund
Re: Potential problem with 31e77c93e432dec7 ("sched/fair: Update blocked load when newly idle")
Hi Vincent, On 2018-04-12 13:15:19 +0200, Niklas Söderlund wrote: > Hi Vincent, > > Thanks for your feedback. > > On 2018-04-12 12:33:27 +0200, Vincent Guittot wrote: > > Hi Niklas, > > > > On 12 April 2018 at 11:18, Niklas Söderlund > > wrote: > > > Hi Vincent, > > > > > > I have observed issues running on linus/master from a few days back [1]. > > > I'm running on a Renesas Koelsch board (arm32) and I can trigger a issue > > > by X forwarding the v4l2 test application qv4l2 over ssh and moving the > > > courser around in the GUI (best test case description award...). I'm > > > sorry about the really bad way I trigger this but I can't do it in any > > > other way, I'm happy to try other methods if you got some ideas. The > > > symptom of the issue is a complete hang of the system for more then 30 > > > seconds and then this information is printed in the console: > > > > Heiner (edded cc) also reported similar problem with his platform: a > > dual core celeron > > > > Do you confirm that your platform is a dual cortex-A15 ? At least that > > what I have seen on web > > This would confirm that dual system is a key point. > > I can confirm that my platform is a dual core. I tested another dual core system today Renesas M3-W ARM64 system and I can observe the same lockups-on that system if it helps you understand the problem. It seems to be much harder to trigger the issue on this system for some reason. Hitting return in a ssh session don't seem to produce the lockup while starting a GUI using X forwarding over ssh it's possible. [ 392.306441] INFO: rcu_preempt detected stalls on CPUs/tasks: [ 392.312201] (detected by 0, t=19366 jiffies, g=7177, c=7176, q=35) [ 392.318555] All QSes seen, last rcu_preempt kthread activity 19368 (4294990375-4294971007), jiffies_till_next_fqs=1, root ->qsmask 0x0 [ 392.330758] swapper/0 R running task0 0 0 0x0022 [ 392.337883] Call trace: [ 392.340365] dump_backtrace+0x0/0x1c8 [ 392.344065] show_stack+0x14/0x20 [ 392.347416] sched_show_task+0x224/0x2e8 [ 392.351377] rcu_check_callbacks+0x8ac/0x8b0 [ 392.355686] update_process_times+0x2c/0x58 [ 392.359908] tick_sched_handle.isra.5+0x30/0x50 [ 392.364479] tick_sched_timer+0x40/0x90 [ 392.368351] __hrtimer_run_queues+0xfc/0x208 [ 392.372659] hrtimer_interrupt+0xd4/0x258 [ 392.376710] arch_timer_handler_virt+0x28/0x48 [ 392.381194] handle_percpu_devid_irq+0x80/0x138 [ 392.385767] generic_handle_irq+0x28/0x40 [ 392.389813] __handle_domain_irq+0x5c/0xb8 [ 392.393946] gic_handle_irq+0x58/0xa8 [ 392.397640] el1_irq+0xb4/0x130 [ 392.400810] arch_cpu_idle+0x14/0x20 [ 392.404422] default_idle_call+0x1c/0x38 [ 392.408381] do_idle+0x17c/0x1f8 [ 392.411640] cpu_startup_entry+0x20/0x28 [ 392.415598] rest_init+0x24c/0x260 [ 392.419037] start_kernel+0x3e8/0x414 I was running the same tests on another ARM64 platform earlier using the same build which have more then two cores and there I could not observe this issue. -- Regards, Niklas Söderlund
Re: Potential problem with 31e77c93e432dec7 ("sched/fair: Update blocked load when newly idle")
d3d261eb-dirty #43
[ 204.028155] Hardware name: Generic R8A7791 (Flattened Device Tree)
[ 204.028164] PC is at _raw_spin_unlock_irqrestore+0x28/0x2c
[ 204.028169] LR is at rcu_report_qs_rnp+0x4c/0x20c
[ 204.028172] pc : []lr : []psr: 200e0113
[ 204.028175] sp : e70a3db0 ip : e70a3dc0 fp : e70a3dbc
[ 204.028177] r10: 0264 r9 : c0b787eb r8 : 0001
[ 204.028179] r7 : 0002 r6 : c0b10980 r5 : c0b10980 r4 : c0b10980
[ 204.028182] r3 : 22a3 r2 : 0264 r1 : 600e0113 r0 : c0b10980
[ 204.028187] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
[ 204.028189] Control: 10c5387d Table: 66b9806a DAC: 0051
[ 204.028193] CPU: 1 PID: 0 Comm: swapper/1 Not tainted
4.16.0-10929-gc18bb396d3d261eb-dirty #43
[ 204.028195] Hardware name: Generic R8A7791 (Flattened Device Tree)
[ 204.028197] Backtrace:
[ 204.028204] [] (dump_backtrace) from []
(show_stack+0x18/0x1c)
[ 204.028209] r7:c0b47278 r6:600e0193 r5: r4:c0b73d80
[ 204.028215] [] (show_stack) from []
(dump_stack+0x84/0xa4)
[ 204.028228] [] (dump_stack) from [] (show_regs+0x14/0x18)
[ 204.028231] r7:c0b47278 r6:e70a3d60 r5:0001 r4:c0bc62c8
[ 204.028238] [] (show_regs) from []
(nmi_cpu_backtrace+0xfc/0x118)
[ 204.028244] [] (nmi_cpu_backtrace) from []
(handle_IPI+0x22c/0x294)
[ 204.028247] r7:c0b47278 r6:e70a3d60 r5:0007 r4:c0a775fc
[ 204.028254] [] (handle_IPI) from []
(gic_handle_irq+0x8c/0x98)
[ 204.028259] r10:0264 r9:e70a2000 r8:f0803000 r7:c0b47278 r6:e70a3d60
r5:c0b05244
[ 204.028261] r4:f0802000 r3:0007
[ 204.028267] [] (gic_handle_irq) from []
(__irq_svc+0x6c/0x90)
[ 204.028270] Exception stack(0xe70a3d60 to 0xe70a3da8)
[ 204.028275] 3d60: c0b10980 600e0113 0264 22a3 c0b10980 c0b10980
c0b10980 0002
[ 204.028280] 3d80: 0001 c0b787eb 0264 e70a3dbc e70a3dc0 e70a3db0
c01796d4 c07653d8
[ 204.028283] 3da0: 200e0113
[ 204.028287] r9:e70a2000 r8:0001 r7:e70a3d94 r6: r5:200e0113
r4:c07653d8
[ 204.028295] [] (_raw_spin_unlock_irqrestore) from []
(rcu_report_qs_rnp+0x4c/0x20c)
[ 204.028301] [] (rcu_report_qs_rnp) from []
(rcu_process_callbacks+0x364/0x8cc)
[ 204.028305] r10:c0b03080 r9: r8:0002 r7:600e0113 r6:c0b10980
r5:c0b10980
[ 204.028306] r4:e7795080
[ 204.028312] [] (rcu_process_callbacks) from []
(__do_softirq+0x23c/0x338)
[ 204.028316] r10:c0b03080 r9:0100 r8:c0b80ec0 r7:0002 r6:0009
r5:c0b030a4
[ 204.028318] r4:e000
[ 204.028325] [] (__do_softirq) from []
(irq_exit+0x6c/0xd4)
[ 204.028329] r10: r9:e70a2000 r8:e7008800 r7:0013 r6:c0a775fc
r5:
[ 204.028331] r4:
[ 204.028336] [] (irq_exit) from []
(__handle_domain_irq+0x88/0xbc)
[ 204.028338] r5: r4:
[ 204.028344] [] (__handle_domain_irq) from []
(gic_handle_irq+0x60/0x98)
[ 204.028348] r9:e70a2000 r8:f0803000 r7:c0b47278 r6:e70a3f40 r5:c0b05244
r4:f0802000
[ 204.028354] [] (gic_handle_irq) from []
(__irq_svc+0x6c/0x90)
[ 204.028356] Exception stack(0xe70a3f40 to 0xe70a3f88)
[ 204.028361] 3f40: 00013844 e77903a8 c0119100 0002 e000
c0b04c2c c0b04c78
[ 204.028367] 3f60: 4000406a 413fc0f2 e70a3f9c e70a3fa0 e70a3f90
c0108564 c0108554
[ 204.028370] 3f80: 600e0013
[ 204.028374] r9:e70a2000 r8:4000406a r7:e70a3f74 r6: r5:600e0013
r4:c0108554
[ 204.028381] [] (arch_cpu_idle) from []
(default_idle_call+0x30/0x34)
[ 204.028388] [] (default_idle_call) from []
(do_idle+0xd8/0x128)
[ 204.028392] [] (do_idle) from []
(cpu_startup_entry+0x20/0x24)
[ 204.028396] r7:c0b80750 r6:10c0387d r5:0051 r4:0085
[ 204.028401] [] (cpu_startup_entry) from []
(secondary_start_kernel+0x10c/0x12c)
[ 204.028406] [] (secondary_start_kernel) from [<4010250c>]
(0x4010250c)
[ 204.028408] r5:0051 r4:6709406a
>
> --
> 2.7.4
>
>
> Thanks,
> Vincent
>
> > >
> > >
> > > Regards,
> > > Vincent
> > > >
> > > > 1. c18bb396d3d261eb ("Merge
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/davem/net"))
> > > > 2. 31e77c93e432dec7 ("sched/fair: Update blocked load when newly idle")
> > > >
> > > > --
> > > > Regards,
> > > > Niklas Söderlund
> >
> > --
> > Regards,
> > Niklas Söderlund
--
Regards,
Niklas Söderlund
Re: Potential problem with 31e77c93e432dec7 ("sched/fair: Update blocked load when newly idle")
Hi Vincent, On 2018-04-12 12:33:27 +0200, Vincent Guittot wrote: [snip[ > > Can you send me your config ? > > I'm going to prepare a debug patch to spy what's happening when entering idle I'm sorry i missed this request when first reading your reply, I'm using arch/arm/configs/shmobile_defconfig for my tests. -- Regards, Niklas Söderlund
Re: Potential problem with 31e77c93e432dec7 ("sched/fair: Update blocked load when newly idle")
Hi Vincent,
Thanks for your feedback.
On 2018-04-12 12:33:27 +0200, Vincent Guittot wrote:
> Hi Niklas,
>
> On 12 April 2018 at 11:18, Niklas Söderlund
> wrote:
> > Hi Vincent,
> >
> > I have observed issues running on linus/master from a few days back [1].
> > I'm running on a Renesas Koelsch board (arm32) and I can trigger a issue
> > by X forwarding the v4l2 test application qv4l2 over ssh and moving the
> > courser around in the GUI (best test case description award...). I'm
> > sorry about the really bad way I trigger this but I can't do it in any
> > other way, I'm happy to try other methods if you got some ideas. The
> > symptom of the issue is a complete hang of the system for more then 30
> > seconds and then this information is printed in the console:
>
> Heiner (edded cc) also reported similar problem with his platform: a
> dual core celeron
>
> Do you confirm that your platform is a dual cortex-A15 ? At least that
> what I have seen on web
> This would confirm that dual system is a key point.
I can confirm that my platform is a dual core.
>
> The ssh connection is also common with Heiner's setup
Interesting, I found Heiner's mail and I can confirm that I too
experience ssh sessions lockups. I ssh into the system and by repeatedly
hitting the return key I can lockup the board, while locked up starting
another ssh session unblocks the first. If I don't start another ssh
session but keep hitting return key sporadically in the first one I can
get the trace I reported in my first mail to be printed on the serial
console.
When locked up the symptoms are that both the single ssh session is dead
and the serial console. But attempting another ssh connection
immediately unblocks both ssh and serial console. And if I allow enough
time before starting the second ssh connection I can trigger a trace to
be printed on the serial console, it's similar but different from the
first I reported.
[ 207.548610] 1-...!: (0 ticks this GP) idle=79a/1/1073741824
softirq=2146/2146 fqs=0
[ 207.556442] (detected by 0, t=12645 jiffies, g=333, c=332, q=20)
[ 207.562546] rcu_sched kthread starved for 12645 jiffies! g333 c332 f0x2
RCU_GP_WAIT_FQS(3) ->state=0x0 ->cpu=0
[ 207.572548] RCU grace-period kthread stack dump:
[ 207.577166] rcu_sched R running task0 9 2 0x
[ 207.584389] Backtrace:
[ 207.586849] [] (__schedule) from [] (schedule+0x94/0xb8)
[ 207.593901] r10:e77813c0 r9:e77813c0 r8: r7:e709bed4 r6:aa80
r5:
[ 207.601732] r4:e000
[ 207.604269] [] (schedule) from []
(schedule_timeout+0x380/0x3dc)
[ 207.612013] r5: r4:
[ 207.615596] [] (schedule_timeout) from []
(rcu_gp_kthread+0x668/0xe2c)
[ 207.623863] r10:c0b79018 r9:014d r8:014c r7:0001 r6:
r5:c0b10ad0
[ 207.631693] r4:c0b10980
[ 207.634230] [] (rcu_gp_kthread) from []
(kthread+0x148/0x160)
[ 207.641712] r7:c0b10980
[ 207.644249] [] (kthread) from []
(ret_from_fork+0x14/0x2c)
[ 207.651472] Exception stack(0xe709bfb0 to 0xe709bff8)
[ 207.656527] bfa0:
[ 207.664709] bfc0:
[ 207.672890] bfe0: 0013
[ 207.679508] r10: r9: r8: r7: r6:
r5:c013dc90
[ 207.687340] r4:e7026f4
Continuing the anecdotal testing, I can't seem to be able to trigger the
lockup if i have ever had two ssh sessions open to the systems. And
about half the time I can't trigger it at all but after a reset of the
system it triggers with just hitting the return key 2-5 times of opening
a ssh session and just hitting the return key. But please take this part
with a grain of salt as it's done by the monkey testing method :-)
All tests above have been run base on c18bb396d3d261eb ("Merge
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net").
>
> >
> > [ 142.849390] INFO: rcu_sched detected stalls on CPUs/tasks:
> > [ 142.854972] 1-...!: (1 GPs behind) idle=7a4/0/0 softirq=3214/3217 fqs=0
> > [ 142.861976] (detected by 0, t=8232 jiffies, g=930, c=929, q=11)
> > [ 142.868042] Sending NMI from CPU 0 to CPUs 1:
> > [ 142.872443] NMI backtrace for cpu 1
> > [ 142.872452] CPU: 1 PID: 0 Comm: swapper/1 Not tainted
> > 4.16.0-05506-g28aba11c1393691a #14
> > [ 142.872455] Hardware name: Generic R8A7791 (Flattened Device Tree)
> > [ 142.872473] PC is at arch_cpu_idle+0x28/0x44
> > [ 142.872484] LR is at trace_hardirqs_on_caller+0x1a4/0x1d4
> > [ 142.872488] pc : []lr : []psr: 20070013
> > [ 142.872491] sp : eb0b9f90 ip : eb0b9f60 fp : eb0b9f9c
> &g
Potential problem with 31e77c93e432dec7 ("sched/fair: Update blocked load when newly idle")
reproduce the issue. I can
also not reproduce the issue on v4.16. I can't figure out if reverting
[2] is just treating a symptom or the root cause of my troubles and
would appreciate your input. Also my "test-case" do not trigger every
time but I have tested this scenario quiet a lot and the result seems to
be constant.
My test setup involves a NFS root filesystem, I ssh in and launch the
GUI application over X forwarding. From what I know the application is
not doing any ioctl calls to the v4l2 framework it's just sitting there
idle as I move the courser around showing tool tips and such as I hover
over elements and then it freeze up. I have not observed this issue by
just booting the system and leaving it idle, movement in the GUI seems
to be the key to trigger this.
I'm a bit lost on how to progress with this issue and would appreciate
any help you can provide to help me figure this out.
1. c18bb396d3d261eb ("Merge
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net"))
2. 31e77c93e432dec7 ("sched/fair: Update blocked load when newly idle")
--
Regards,
Niklas Söderlund
Re: [PATCH] clk: renesas: rcar-gen2: Centralize quirks handling
Hi Geert,
Thanks for your patch.
On 2018-04-10 15:04:58 +0200, Geert Uytterhoeven wrote:
> Introduce centralized quirks handling like on R-Car Gen3, and convert
> the RZ/G1C SD clock table handling over to it.
>
> This makes it easier to add more quirks later, if/when needed.
>
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Niklas Söderlund
> ---
> To be queued in clk-renesas-for-v4.18.
>
> drivers/clk/renesas/rcar-gen2-cpg.c | 20
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/renesas/rcar-gen2-cpg.c
> b/drivers/clk/renesas/rcar-gen2-cpg.c
> index 0c49f59d5074b1c4..daf88bc2cdae177b 100644
> --- a/drivers/clk/renesas/rcar-gen2-cpg.c
> +++ b/drivers/clk/renesas/rcar-gen2-cpg.c
> @@ -261,9 +261,15 @@ static const struct clk_div_table cpg_sd01_div_table[] =
> {
> static const struct rcar_gen2_cpg_pll_config *cpg_pll_config __initdata;
> static unsigned int cpg_pll0_div __initdata;
> static u32 cpg_mode __initdata;
> +static u32 cpg_quirks __initdata;
>
> -static const struct soc_device_attribute soc_r8a77470[] = {
> - { .soc_id = "r8a77470" },
> +#define SD_SKIP_FIRSTBIT(0) /* Skip first clock in SD table
> */
> +
> +static const struct soc_device_attribute cpg_quirks_match[] __initconst = {
> + {
> + .soc_id = "r8a77470",
> + .data = (void *)SD_SKIP_FIRST,
> + },
> { /* sentinel */ }
> };
>
> @@ -333,7 +339,7 @@ struct clk * __init rcar_gen2_cpg_clk_register(struct
> device *dev,
>
> case CLK_TYPE_GEN2_SD0:
> table = cpg_sd01_div_table;
> - if (soc_device_match(soc_r8a77470))
> + if (cpg_quirks & SD_SKIP_FIRST)
> table++;
>
> shift = 4;
> @@ -341,7 +347,7 @@ struct clk * __init rcar_gen2_cpg_clk_register(struct
> device *dev,
>
> case CLK_TYPE_GEN2_SD1:
> table = cpg_sd01_div_table;
> - if (soc_device_match(soc_r8a77470))
> + if (cpg_quirks & SD_SKIP_FIRST)
> table++;
>
> shift = 0;
> @@ -372,9 +378,15 @@ struct clk * __init rcar_gen2_cpg_clk_register(struct
> device *dev,
> int __init rcar_gen2_cpg_init(const struct rcar_gen2_cpg_pll_config *config,
> unsigned int pll0_div, u32 mode)
> {
> + const struct soc_device_attribute *attr;
> +
> cpg_pll_config = config;
> cpg_pll0_div = pll0_div;
> cpg_mode = mode;
> + attr = soc_device_match(cpg_quirks_match);
> + if (attr)
> + cpg_quirks = (uintptr_t)attr->data;
> + pr_debug("%s: mode = 0x%x quirks = 0x%x\n", __func__, mode, cpg_quirks);
>
> spin_lock_init(&cpg_lock);
>
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH v5 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
Hi Jacopo,
Thanks for your patch.
This one must depend on '[PATCH v2 0/5] arm64: dts: renesas: r8a77970:
enable HDMI output' or something similar not yet in renesas-drivers
repository correct?
In the next version would you care to include the LVDS commit from the
dependency series and squash this change into that one or in some other
good manger stack to two? Laurent told me he did not like 5/5 in that
patch-set as it did not yet have the LVDS decoder node due to no driver
existed at that time when I posted that even if it's not strictly needed
to get the display working :-)
I also think you should split this last patch out to a separate series
as it should go in Simon's tree while the driver and documentation is
going in earlier in a different tree right?
On a side note, do you plan to update the Gen2 boards DTS files which
also have a decoder which are not yet described in DT?
On 2018-03-15 17:11:56 +0100, Jacopo Mondi wrote:
> The R-Car V3M Eagle board includes a transparent THC63LVD1024 LVDS
> decoder, connected to the on-chip LVDS encoder output on one side
> and to HDMI encoder ADV7511w on the other one.
>
> As the decoder does not need any configuration it has been so-far
> omitted from DTS. Now that a driver is available, describe it in DT
> as well.
>
> Signed-off-by: Jacopo Mondi
> Reviewed-by: Andrzej Hajda
> ---
> arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 33
> +++---
> 1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> index c0fd144..69f43b8 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -42,6 +42,33 @@
> };
> };
> };
> +
> + thc63lvd1024: lvds-decoder {
> + compatible = "thine,thc63lvd1024";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + thc63lvd1024_in_0: endpoint {
> + remote-endpoint = <&lvds0_out>;
> + };
> + };
> +
> + port@2{
> + reg = <2>;
> +
> + thc63lvd1024_out_2: endpoint {
> + remote-endpoint = <&adv7511_in>;
> + };
> +
> + };
> +
> + };
> + };
> };
>
> &avb {
> @@ -98,7 +125,7 @@
> port@0 {
> reg = <0>;
> adv7511_in: endpoint {
> - remote-endpoint = <&lvds0_out>;
> + remote-endpoint = <&thc63lvd1024_out_2>;
> };
> };
>
> @@ -152,8 +179,8 @@
>
> ports {
> port@1 {
> - endpoint {
> - remote-endpoint = <&adv7511_in>;
> + lvds0_out: endpoint {
> + remote-endpoint = <&thc63lvd1024_in_0>;
> };
> };
> };
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH v5 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver
if (!vcc)
> + continue;
> +
> + if (regulator_disable(vcc))
> + dev_dbg(thc63->dev,
> + "Failed to disable regulator %s\n",
> + thc63_reg_names[i]);
> + }
> +}
> +
> +struct drm_bridge_funcs thc63_bridge_func = {
> + .attach = thc63_attach,
> + .enable = thc63_enable,
> + .disable = thc63_disable,
> +};
> +
> +static int thc63_parse_dt(struct thc63_dev *thc63)
> +{
> + struct device_node *thc63_out;
> + struct device_node *remote;
> + int ret = 0;
> +
> + thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> + THC63_DIGITAL_OUT0, -1);
> + if (!thc63_out) {
> + dev_err(thc63->dev, "Missing endpoint in Port@%u\n",
> + THC63_DIGITAL_OUT0);
> + return -ENODEV;
> + }
> +
> + remote = of_graph_get_remote_port_parent(thc63_out);
> + if (!remote) {
> + dev_err(thc63->dev, "Endpoint in Port@%u unconnected\n",
> + THC63_DIGITAL_OUT0);
> + ret = -ENODEV;
> + goto error_put_out_node;
> + }
> +
> + if (!of_device_is_available(remote)) {
> + dev_err(thc63->dev, "Port@%u remote endpoint is disabled\n",
> + THC63_DIGITAL_OUT0);
> + ret = -ENODEV;
> + goto error_put_remote_node;
> + }
> +
> + thc63->next = of_drm_find_bridge(remote);
> + if (!thc63->next)
> + ret = -EPROBE_DEFER;
> +
> +error_put_remote_node:
> + of_node_put(remote);
> +error_put_out_node:
> + of_node_put(thc63_out);
> +
> + return ret;
> +}
> +
> +static int thc63_gpio_init(struct thc63_dev *thc63)
> +{
> + thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> + if (IS_ERR(thc63->oe)) {
> + dev_err(thc63->dev, "Unable to get \"oe-gpios\"\n");
> + return PTR_ERR(thc63->oe);
> + }
> +
> + thc63->pdwn = devm_gpiod_get_optional(thc63->dev, "pdwn",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(thc63->pdwn)) {
> + dev_err(thc63->dev, "Unable to get \"pdwn-gpios\"\n");
> + return PTR_ERR(thc63->pdwn);
> + }
> +
> + return 0;
> +}
> +
> +static int thc63_regulator_init(struct thc63_dev *thc63)
> +{
> + struct regulator **reg;
> + int i;
> +
> + reg = &thc63->vccs[0];
> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
> + *reg = devm_regulator_get_optional(thc63->dev,
> +thc63_reg_names[i]);
> + if (IS_ERR(*reg)) {
> + if (PTR_ERR(*reg) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + *reg = NULL;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int thc63_probe(struct platform_device *pdev)
> +{
> + struct thc63_dev *thc63;
> + int ret;
> +
> + thc63 = devm_kzalloc(&pdev->dev, sizeof(*thc63), GFP_KERNEL);
> + if (!thc63)
> + return -ENOMEM;
> +
> + thc63->dev = &pdev->dev;
> + platform_set_drvdata(pdev, thc63);
> +
> + ret = thc63_regulator_init(thc63);
> + if (ret)
> + return ret;
> +
> + ret = thc63_gpio_init(thc63);
> + if (ret)
> + return ret;
> +
> + ret = thc63_parse_dt(thc63);
> + if (ret)
> + return ret;
> +
> + thc63->bridge.driver_private = thc63;
> + thc63->bridge.of_node = pdev->dev.of_node;
> + thc63->bridge.funcs = &thc63_bridge_func;
> +
> + drm_bridge_add(&thc63->bridge);
> +
> + return 0;
> +}
> +
> +static int thc63_remove(struct platform_device *pdev)
> +{
> + struct thc63_dev *thc63 = platform_get_drvdata(pdev);
> +
> + drm_bridge_remove(&thc63->bridge);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id thc63_match[] = {
> + { .compatible = "thine,thc63lvd1024", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, thc63_match);
> +#endif
This driver depends on OF so you don't need the ifdef here. If you
choose to remove the dependency on OF and make it optional you need to
handle .of_match_table bellow in case OF is not defined :-)
With this fixed feel free to add my
Reviewed-by: Niklas Söderlund
> +
> +static struct platform_driver thc63_driver = {
> + .probe = thc63_probe,
> + .remove = thc63_remove,
> + .driver = {
> + .name = "thc63lvd1024",
> + .of_match_table = thc63_match,
> + },
> +};
> +module_platform_driver(thc63_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi ");
> +MODULE_DESCRIPTION("Thine THC63LVD1024 LVDS decoder DRM bridge driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH v5 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder
Hi Jacopo,
Thanks for your patch,
On 2018-03-15 17:11:54 +0100, Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>
> Signed-off-by: Jacopo Mondi
> Reviewed-by: Andrzej Hajda
Reviewed-by: Niklas Söderlund
> ---
> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66
> ++
> 1 file changed, 66 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> new file mode 100644
> index 000..8225c6a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -0,0 +1,66 @@
> +Thine Electronics THC63LVD1024 LVDS decoder
> +---
> +
> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS
> streams
> +to parallel data outputs. The chip supports single/dual input/output modes,
> +handling up to two two input LVDS stream and up to two digital CMOS/TTL
> outputs.
> +
> +Single or dual operation modes, output data mapping and DDR output modes are
> +configured through input signals and the chip does not expose any control
> bus.
> +
> +Required properties:
> +- compatible: Shall be "thine,thc63lvd1024"
> +
> +Optional properties:
> +- vcc-supply: Power supply for TTL output and digital circuitry
> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
> +- lvcc-supply: Power supply for LVDS inputs
> +- pvcc-supply: Power supply for PLL circuitry
> +- pdwn-gpios: Power down GPIO signal. Active low
> +- oe-gpios: Output enable GPIO signal. Active high
> +
> +The THC63LVD1024 video port connections are modeled according
> +to OF graph bindings specified by Documentation/devicetree/bindings/graph.txt
> +
> +Required video port nodes:
> +- Port@0: First LVDS input port
> +- Port@2: First digital CMOS/TTL parallel output
> +
> +Optional video port nodes:
> +- Port@1: Second LVDS input port
> +- Port@3: Second digital CMOS/TTL parallel output
> +
> +Example:
> +
> +
> + thc63lvd1024: lvds-decoder {
> + compatible = "thine,thc63lvd1024";
> +
> + vcc-supply = <®_lvds_vcc>;
> + lvcc-supply = <®_lvds_lvcc>;
> +
> + pdwn-gpio = <&gpio4 15 GPIO_ACTIVE_LOW>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + lvds_dec_in_0: endpoint {
> + remote-endpoint = <&lvds_out>;
> + };
> + };
> +
> + port@2{
> + reg = <2>;
> +
> + lvds_dec_out_2: endpoint {
> + remote-endpoint = <&adv7511_in>;
> + };
> +
> + };
> +
> + };
> + };
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
Re: [PATCH v2 3/3] media: i2c: adv748x: Add support for i2c_new_secondary_device
Hi Kieran,
I like this change :-)
On 2018-02-27 15:05:50 +, Kieran Bingham wrote:
> From: Kieran Bingham
>
> The ADV748x has twelve 256-byte maps that can be accessed via the main
> I2C ports. Each map has it own I2C address and acts as a standard slave
> device on the I2C bus.
>
> Allow a device tree node to override the default addresses so that
> address conflicts with other devices on the same bus may be resolved at
> the board description level.
>
> Signed-off-by: Kieran Bingham
Reviewed-by: Niklas Söderlund
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 77
> +++-
> drivers/media/i2c/adv748x/adv748x.h | 14 --
> 2 files changed, 36 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c
> index 9dcaadaca217..911ccf6eb68c 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -80,21 +80,24 @@ static int adv748x_configure_regmap(struct adv748x_state
> *state, int region)
>
> return 0;
> }
> +struct adv748x_register_map {
> + const char *name;
> + u8 default_addr;
> +};
>
> -/* Default addresses for the I2C pages */
> -static int adv748x_i2c_addresses[ADV748X_PAGE_MAX] = {
> - ADV748X_I2C_IO,
> - ADV748X_I2C_DPLL,
> - ADV748X_I2C_CP,
> - ADV748X_I2C_HDMI,
> - ADV748X_I2C_EDID,
> - ADV748X_I2C_REPEATER,
> - ADV748X_I2C_INFOFRAME,
> - ADV748X_I2C_CBUS,
> - ADV748X_I2C_CEC,
> - ADV748X_I2C_SDP,
> - ADV748X_I2C_TXB,
> - ADV748X_I2C_TXA,
> +static const struct adv748x_register_map adv748x_default_addresses[] = {
> + [ADV748X_PAGE_IO] = { "main", 0x70 },
> + [ADV748X_PAGE_DPLL] = { "dpll", 0x26 },
> + [ADV748X_PAGE_CP] = { "cp", 0x22 },
> + [ADV748X_PAGE_HDMI] = { "hdmi", 0x34 },
> + [ADV748X_PAGE_EDID] = { "edid", 0x36 },
> + [ADV748X_PAGE_REPEATER] = { "repeater", 0x32 },
> + [ADV748X_PAGE_INFOFRAME] = { "infoframe", 0x31 },
> + [ADV748X_PAGE_CBUS] = { "cbus", 0x30 },
> + [ADV748X_PAGE_CEC] = { "cec", 0x41 },
> + [ADV748X_PAGE_SDP] = { "sdp", 0x79 },
> + [ADV748X_PAGE_TXB] = { "txb", 0x48 },
> + [ADV748X_PAGE_TXA] = { "txa", 0x4a },
> };
>
> static int adv748x_read_check(struct adv748x_state *state,
> @@ -143,15 +146,20 @@ int adv748x_write_block(struct adv748x_state *state,
> int client_page,
> return regmap_raw_write(regmap, init_reg, val, val_len);
> }
>
> -static struct i2c_client *adv748x_dummy_client(struct adv748x_state *state,
> -u8 addr, u8 io_reg)
> +static int adv748x_set_slave_addresses(struct adv748x_state *state)
> {
> - struct i2c_client *client = state->client;
> + struct i2c_client *client;
> + unsigned int i;
> + u8 io_reg;
> +
> + for (i = ADV748X_PAGE_DPLL; i < ADV748X_PAGE_MAX; ++i) {
> + io_reg = ADV748X_IO_SLAVE_ADDR_BASE + i;
> + client = state->i2c_clients[i];
>
> - if (addr)
> - io_write(state, io_reg, addr << 1);
> + io_write(state, io_reg, client->addr << 1);
> + }
>
> - return i2c_new_dummy(client->adapter, io_read(state, io_reg) >> 1);
> + return 0;
> }
>
> static void adv748x_unregister_clients(struct adv748x_state *state)
> @@ -164,13 +172,15 @@ static void adv748x_unregister_clients(struct
> adv748x_state *state)
>
> static int adv748x_initialise_clients(struct adv748x_state *state)
> {
> - int i;
> + unsigned int i;
> int ret;
>
> for (i = ADV748X_PAGE_DPLL; i < ADV748X_PAGE_MAX; ++i) {
> - state->i2c_clients[i] =
> - adv748x_dummy_client(state, adv748x_i2c_addresses[i],
> - ADV748X_IO_SLAVE_ADDR_BASE + i);
> + state->i2c_clients[i] = i2c_new_secondary_device(
> + state->client,
> + adv748x_default_addresses[i].name,
> + adv748x_default_addresses[i].default_addr);
> +
> if (state->i2c_clients[i] == NULL) {
> adv_err(state, "failed to create i2c client %u\n", i);
> return -ENOMEM;
> @@ -181,7 +191,7 @@ static int adv748x_initialise_clients(struct
> adv748x_state *state)
> return ret;
> }
>
> - retur
Re: [PATCH v2 1/3] media: i2c: adv748x: Simplify regmap configuration
Hi Kieran,
Thanks for your patch,
On 2018-02-27 15:05:48 +, Kieran Bingham wrote:
> From: Kieran Bingham
>
> The ADV748x has identical map configurations for each register map. The
> duplication of each map can be simplified using a helper macro such that
> each map is represented on a single line.
>
> Define ADV748X_REGMAP_CONF for this purpose use it to create the tables.
>
> Signed-off-by: Kieran Bingham
Reviewed-by: Niklas Söderlund
>
> ---
> v2:
> - Remove unnecessary #undef
>
> drivers/media/i2c/adv748x/adv748x-core.c | 109
> ++-
> 1 file changed, 20 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c
> index fd92c9e4b519..faf73949962b 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -35,96 +35,27 @@
> * Register manipulation
> */
>
> -static const struct regmap_config adv748x_regmap_cnf[] = {
> - {
> - .name = "io",
> - .reg_bits = 8,
> - .val_bits = 8,
> -
> - .max_register = 0xff,
> - .cache_type = REGCACHE_NONE,
> - },
> - {
> - .name = "dpll",
> - .reg_bits = 8,
> - .val_bits = 8,
> -
> - .max_register = 0xff,
> - .cache_type = REGCACHE_NONE,
> - },
> - {
> - .name = "cp",
> - .reg_bits = 8,
> - .val_bits = 8,
> -
> - .max_register = 0xff,
> - .cache_type = REGCACHE_NONE,
> - },
> - {
> - .name = "hdmi",
> - .reg_bits = 8,
> - .val_bits = 8,
> -
> - .max_register = 0xff,
> - .cache_type = REGCACHE_NONE,
> - },
> - {
> - .name = "edid",
> - .reg_bits = 8,
> - .val_bits = 8,
> -
> - .max_register = 0xff,
> - .cache_type = REGCACHE_NONE,
> - },
> - {
> - .name = "repeater",
> - .reg_bits = 8,
> - .val_bits = 8,
> -
> - .max_register = 0xff,
> - .cache_type = REGCACHE_NONE,
> - },
> - {
> - .name = "infoframe",
> - .reg_bits = 8,
> - .val_bits = 8,
> -
> - .max_register = 0xff,
> - .cache_type = REGCACHE_NONE,
> - },
> - {
> - .name = "cec",
> - .reg_bits = 8,
> - .val_bits = 8,
> -
> - .max_register = 0xff,
> - .cache_type = REGCACHE_NONE,
> - },
> - {
> - .name = "sdp",
> - .reg_bits = 8,
> - .val_bits = 8,
> -
> - .max_register = 0xff,
> - .cache_type = REGCACHE_NONE,
> - },
> -
> - {
> - .name = "txb",
> - .reg_bits = 8,
> - .val_bits = 8,
> -
> - .max_register = 0xff,
> - .cache_type = REGCACHE_NONE,
> - },
> - {
> - .name = "txa",
> - .reg_bits = 8,
> - .val_bits = 8,
> +#define ADV748X_REGMAP_CONF(n) \
> +{ \
> + .name = n, \
> + .reg_bits = 8, \
> + .val_bits = 8, \
> + .max_register = 0xff, \
> + .cache_type = REGCACHE_NONE, \
> +}
>
> - .max_register = 0xff,
> - .cache_type = REGCACHE_NONE,
> - },
> +static const struct regmap_config adv748x_regmap_cnf[] = {
> + ADV748X_REGMAP_CONF("io"),
> + ADV748X_REGMAP_CONF("dpll"),
> + ADV748X_REGMAP_CONF("cp"),
> + ADV748X_REGMAP_CONF("hdmi"),
> + ADV748X_REGMAP_CONF("edid"),
> + ADV748X_REGMAP_CONF("repeater"),
> + ADV748X_REGMAP_CONF("infoframe"),
> + ADV748X_REGMAP_CONF("cec"),
> + ADV748X_REGMAP_CONF("sdp"),
> + ADV748X_REGMAP_CONF("txa"),
> + ADV748X_REGMAP_CONF("txb"),
> };
>
> static int adv748x_configure_regmap(struct adv748x_state *state, int region)
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
