Re: [PATCH] clk: renesas: mstp: Support 8-bit registers for r7s72100

[Kuninori Morimoto]

Hi Chris

> > > I don't think using global variable is a good idea.
> > > For example, how about add reg_width_8bit into mstp_clock_group, and
> > > cpg_mstp_read/write requests it, or something like that ?
> > 
> > If I make a separate CLK_OF_DECLARE like this:
> > 
> > static void __init cpg_mstp_clocks_init8(struct device_node *np) {
> > reg_width_8bit = true;
> > cpg_mstp_clocks_init(np);
> > }
> > CLK_OF_DECLARE(cpg_mstp_clks8, "renesas,r7s72100-mstp-clocks",
> >cpg_mstp_clocks_init8);
> > 
> > The only way I can pass the 8-bit/32-bit choice into cpg_mstp_clocks_init()
> > is using a global variable.
> 
> How about this ?
> # I didn't test this, compile test only

Oops, I misread your email.
Yes, your 2nd idea (= using true, false) is better idea.

Best regards
---
Kuninori Morimoto

Re: [PATCH] clk: renesas: mstp: Support 8-bit registers for r7s72100

[Kuninori Morimoto]

Hi Chris

> > I don't think using global variable is a good idea.
> > For example, how about add reg_width_8bit into mstp_clock_group, and
> > cpg_mstp_read/write requests it, or something like that ?
> 
> If I make a separate CLK_OF_DECLARE like this:
> 
> static void __init cpg_mstp_clocks_init8(struct device_node *np) {
>   reg_width_8bit = true;
>   cpg_mstp_clocks_init(np);
> }
> CLK_OF_DECLARE(cpg_mstp_clks8, "renesas,r7s72100-mstp-clocks",
>  cpg_mstp_clocks_init8);
> 
> The only way I can pass the 8-bit/32-bit choice into cpg_mstp_clocks_init()
> is using a global variable.

How about this ?
# I didn't test this, compile test only

--
diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index 9375777..488ff56 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -43,6 +43,7 @@ struct mstp_clock_group {
void __iomem *smstpcr;
void __iomem *mstpsr;
spinlock_t lock;
+   bool reg_width_8bit;
 };
 
 /**
@@ -58,6 +59,14 @@ struct mstp_clock {
 };
 
 #define to_mstp_clock(_hw) container_of(_hw, struct mstp_clock, hw)
+#define cpg_mstp_read(group, reg)  \
+   (group)->reg_width_8bit ?   \
+  readb((group)->reg) :\
+  clk_readl((group)->reg)
+#define cpg_mstp_write(group, val, reg)\
+   (group)->reg_width_8bit ?   \
+  writeb(val, (group)->reg) :  \
+  clk_writel(val, (group)->reg)
 
 static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
 {
@@ -70,12 +79,12 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool 
enable)
 
spin_lock_irqsave(>lock, flags);
 
-   value = clk_readl(group->smstpcr);
+   value = cpg_mstp_read(group, smstpcr);
if (enable)
value &= ~bitmask;
else
value |= bitmask;
-   clk_writel(value, group->smstpcr);
+   cpg_mstp_write(group, value, smstpcr);
 
spin_unlock_irqrestore(>lock, flags);
 
@@ -83,7 +92,7 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool 
enable)
return 0;
 
for (i = 1000; i > 0; --i) {
-   if (!(clk_readl(group->mstpsr) & bitmask))
+   if (!(cpg_mstp_read(group, mstpsr) & bitmask))
break;
cpu_relax();
}
@@ -114,9 +123,9 @@ static int cpg_mstp_clock_is_enabled(struct clk_hw *hw)
u32 value;
 
if (group->mstpsr)
-   value = clk_readl(group->mstpsr);
+   value = cpg_mstp_read(group, mstpsr);
else
-   value = clk_readl(group->smstpcr);
+   value = cpg_mstp_read(group, smstpcr);
 
return !(value & BIT(clock->bit_index));
 }
@@ -159,7 +168,8 @@ static int cpg_mstp_clock_is_enabled(struct clk_hw *hw)
return clk;
 }
 
-static void __init cpg_mstp_clocks_init(struct device_node *np)
+static struct mstp_clock_group* __init
+__cpg_mstp_clocks_init(struct device_node *np)
 {
struct mstp_clock_group *group;
const char *idxname;
@@ -172,7 +182,7 @@ static void __init cpg_mstp_clocks_init(struct device_node 
*np)
kfree(group);
kfree(clks);
pr_err("%s: failed to allocate group\n", __func__);
-   return;
+   return NULL;
}
 
spin_lock_init(>lock);
@@ -185,7 +195,7 @@ static void __init cpg_mstp_clocks_init(struct device_node 
*np)
pr_err("%s: failed to remap SMSTPCR\n", __func__);
kfree(group);
kfree(clks);
-   return;
+   return NULL;
}
 
for (i = 0; i < MSTP_MAX_CLOCKS; ++i)
@@ -240,9 +250,26 @@ static void __init cpg_mstp_clocks_init(struct device_node 
*np)
}
 
of_clk_add_provider(np, of_clk_src_onecell_get, >data);
+
+   return group;
+}
+
+static void __init cpg_mstp_clocks_init(struct device_node *np)
+{
+   __cpg_mstp_clocks_init(np);
 }
 CLK_OF_DECLARE(cpg_mstp_clks, "renesas,cpg-mstp-clocks", cpg_mstp_clocks_init);
 
+static void __init cpg_mstp_clocks_init8(struct device_node *np)
+{
+   struct mstp_clock_group *group = __cpg_mstp_clocks_init(np);
+
+   if (group)
+   group->reg_width_8bit = true;
+}
+CLK_OF_DECLARE(cpg_mstp_clks8, "renesas,r7s72100-mstp-clocks",
+  cpg_mstp_clocks_init8);
+
 int cpg_mstp_attach_dev(struct generic_pm_domain *unused, struct device *dev)
 {
struct device_node *np = dev->of_node;

RE: [PATCH] clk: renesas: mstp: Support 8-bit registers for r7s72100

[Chris Brandt]

Hi Morimoto-san,

On Dec 14, 2016, Kuninori Morimoto wrote:
> I don't think using global variable is a good idea.
> For example, how about add reg_width_8bit into mstp_clock_group, and
> cpg_mstp_read/write requests it, or something like that ?

If I make a separate CLK_OF_DECLARE like this:

static void __init cpg_mstp_clocks_init8(struct device_node *np) {
reg_width_8bit = true;
cpg_mstp_clocks_init(np);
}
CLK_OF_DECLARE(cpg_mstp_clks8, "renesas,r7s72100-mstp-clocks",
   cpg_mstp_clocks_init8);


The only way I can pass the 8-bit/32-bit choice into cpg_mstp_clocks_init()
is using a global variable.


Unless, I change the API to cpg_mstp_clocks_init(np, reg_width_8bit)

But, that means adding another function:

CLK_OF_DECLARE(cpg_mstp_clks32, "renesas,cpg-mstp-clocks",
   cpg_mstp_clocks_init32);

CLK_OF_DECLARE(cpg_mstp_clks8, "renesas,r7s72100-mstp-clocks",
   cpg_mstp_clocks_init8);

static void __init cpg_mstp_clocks_init32(struct device_node *np)
{
cpg_mstp_clocks_init(np, false);
}
static void __init cpg_mstp_clocks_init8(struct device_node *np)
{
cpg_mstp_clocks_init(np, true);
}


A global variable is much easier, but if no one likes it, I can change to
mstp_clock_group.reg_width_8bit instead.


Chris

Re: [PATCH] clk: renesas: mstp: Support 8-bit registers for r7s72100

[Kuninori Morimoto]

Hi Chris

> The RZ/A1 is different than the other Renesas SOCs because the MSTP
> registers are 8-bit instead of 32-bit and if you try writing values as
> 32-bit nothing happens...meaning this driver never worked for r7s72100.
> 
> Fixes: b6face404f38 ("ARM: shmobile: r7s72100: add essential clock nodes to 
> dtsi")
> Signed-off-by: Chris Brandt 
> ---
(snip)
> +/**
> + * Some devices only have 8-bit registers
> + */
> +bool reg_width_8bit;
(snip)
> +static void __init cpg_mstp_clocks_init8(struct device_node *np)
> +{
> + reg_width_8bit = true;
> + cpg_mstp_clocks_init(np);
> +}
> +CLK_OF_DECLARE(cpg_mstp_clks8, "renesas,r7s72100-mstp-clocks",
> +cpg_mstp_clocks_init8);

I don't think using global variable is a good idea.
For example, how about add reg_width_8bit into mstp_clock_group,
and cpg_mstp_read/write requests it, or something like that ?

Best regards
---
Kuninori Morimoto

Re: [PATCH] drm: rcar-du: Fix R-Car Gen3 crash when VSP is disabled

[Laurent Pinchart]

Hi Magnus,

On Tuesday 06 Dec 2016 16:07:02 Laurent Pinchart wrote:
> On Wednesday 26 Oct 2016 18:13:23 Magnus Damm wrote:
> > On Wed, Oct 26, 2016 at 4:31 PM, Geert Uytterhoeven wrote:
> >> On Wed, Oct 26, 2016 at 5:31 AM, Magnus Damm wrote:
> >>> From: Magnus Damm 
> >>> 
> >>> For the DU to operate on R-Car Gen3 hardware a combination of DU
> >>> and VSP devices are required. Since the DU driver also supports
> >>> earlier generations hardware the VSP portion is enabled via Kconfig.
> >>> 
> >>> The arm64 defconfig is as of v4.9-rc1 having the DU driver enabled
> >>> as a module, however this is not enough to support R-Car Gen3. In
> >>> the current case of CONFIG_DRM_RCAR_VSP=n then the kernel crashes
> >>> when loading the module. This patch is fixing that particular case.
> >>> 
> >>> In more detail, the crash triggers in drm_atomic_get_plane_state()
> >>> when __drm_atomic_helper_set_config() passes NULL as crtc->primary.
> >>> 
> >>> This patch corrects this issue by failing to load the DU driver on
> >>> R-Car Gen3 when VSP is not available.
> >>> 
> >>> Signed-off-by: Magnus Damm 
> >>> ---
> >>> 
> >>>  drivers/gpu/drm/rcar-du/rcar_du_vsp.h |2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>> 
> >>> --- 0001/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
> >>> +++ work/drivers/gpu/drm/rcar-du/rcar_du_vsp.h  2016-10-26
> >>> 00:01:12.920607110 +0900 @@ -70,7 +70,7 @@ void
> >>> rcar_du_vsp_disable(struct rcar_du_
> >>> 
> >>>  void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc);
> >>>  void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc);
> >>>  #else
> >>> 
> >>> -static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return
> >>> 0;
> >>> };
> >>> +static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return
> >>> -ENXIO; };
> 
> With this patch applied the DU will fail to probe on Gen2 if DRM_RCAR_VSP is
> disabled.
> 
> DRM_RCAR_DU should instead depend on VIDEO_RENESAS_VSP1 (with DRM_RCAR_VSP
> always set) on Gen3.

Do you plan to work on a v2 that fixes this in Kconfig ? I gave it a quick try 
but ran into a Kconfig circular dependency that I don't have time to debug 
now.

> > > -ENODEV sounds more appropriate
> > 
> > Ok, however -ENXIO is the same error code as the DU driver currently
> > returns when dealing with the VSP and not finding DT nodes.
> > 
> > To avoid dealing with this again I would prefer skipping per-driver
> > Kconfig options entirely, for instance something like this:
> > 
> > [PATCH/RFC] Simplify Gen3 DU and VSP Kconfig bits
> > https://www.spinics.net/lists/linux-sh/msg45978.html
> > 
> >>>  static inline void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) { };
> >>>  static inline void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) { };
> >>>  static inline void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
> >>>  {
> >>>  };
> >> 
> >> Alternatively,  DRM_RCAR_DU can force DRM_RCAR_VSP to y if ARCH_R8A7795
> >> or ARCH_R8A7796 is enabled.
> > 
> > The DU driver has symbol dependencies on VSP and FCP driver code
> > (V4L2), so we need to take the state of driver support for those
> > modules into consideration as well instead of only depending on
> > SoC-specific Kconfig symbols.
> > 
> > On Wed, Oct 26, 2016 at 4:31 PM, Geert Uytterhoeven wrote:
> >> On Wed, Oct 26, 2016 at 5:31 AM, Magnus Damm wrote:
> >>> The arm64 defconfig is as of v4.9-rc1 having the DU driver enabled
> >>> as a module, however this is not enough to support R-Car Gen3. In
> >> 
> >> Nope, arm64 defconfig on v4.9-rc1:
> >> # CONFIG_DRM_RCAR_DU is not set
> > 
> > Right, you are correct. Sorry for the noise. It seems that I was using
> > renesas-drivers-2016-10-18-v4.9-rc1 - not mainline v4.9-rc1.
> > 
> > Without this fix the driver still crashes, but not due to defconfig in
> > v4.9-rc1.

-- 
Regards,

Laurent Pinchart

[PATCH] ARM: dts: r8a7791: link DU to VSPDs

[Sergei Shtylyov]

Add the "vsps" property to the DU device node in order to link this node to
the VSPD nodes.

Signed-off-by: Sergei Shtylyov 

---
This patch is against the 'renesas-devel-20161212-v4.9' of Simon Horman's
'renesas.git' repo.  It's  only meaningful if the DU driver patch I've just
posted is applied.

 arch/arm/boot/dts/r8a7791.dtsi |5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: renesas/arch/arm/boot/dts/r8a7791.dtsi
===
--- renesas.orig/arch/arm/boot/dts/r8a7791.dtsi
+++ renesas/arch/arm/boot/dts/r8a7791.dtsi
@@ -989,7 +989,7 @@
power-domains = < R8A7791_PD_ALWAYS_ON>;
};
 
-   vsp1@fe93 {
+   vspd0: vsp1@fe93 {
compatible = "renesas,vsp1";
reg = <0 0xfe93 0 0x8000>;
interrupts = ;
@@ -997,7 +997,7 @@
power-domains = < R8A7791_PD_ALWAYS_ON>;
};
 
-   vsp1@fe938000 {
+   vspd1: vsp1@fe938000 {
compatible = "renesas,vsp1";
reg = <0 0xfe938000 0 0x8000>;
interrupts = ;
@@ -1016,6 +1016,7 @@
 <_clks R8A7791_CLK_DU1>,
 <_clks R8A7791_CLK_LVDS0>;
clock-names = "du.0", "du.1", "lvds.0";
+   vsps = < >;
status = "disabled";
 
ports {

Re: [PATCHv3 RFC 4/4] media: Catch null pipes on pipeline stop

[Laurent Pinchart]

Hi Kieran,

On Wednesday 14 Dec 2016 19:56:51 Kieran Bingham wrote:
> Hello Me.
> 
> Ok, so a bit of investigation into *why* we have an unbalanced
> media_pipeline stop call.
> 
> After a suspend/resume cycle, the function vsp1_pm_runtime_resume() is
> called by the PM framework.
> 
> The hardware is unable to reset successfully and the reset call returns
> -ETIMEDOUT which gets passed back to the PM framework as a failure and
> thus the device is not 'resumed'
> 
> This process is commenced, as the DU driver tries to enable the VSP, we
> get the following call stack:
> 
> rcar_du_crtc_resume()
>   rcar_du_vsp_enable()
> vsp1_du_setup_lif() // returns void
>   vsp1_device_get() // returns -ETIMEDOUT,

I suspect the call stack to not be entirely accurate as the 
rcar_du_crtc_resume() is never called :-)

> As the vsp1_du_setup_lif() returns void, the fact that the hardware
> failed to start is ignored.
> 
> Later we get a call to  rcar_du_vsp_disable(), which again calls into
> vsp1_du_setup_lif(), this time to disable the pipeline. And it is here,
> that the call to media_pipeline_stop() is an unbalanced call, as the
> corresponding media_pipeline_start() would only have been called if the
> vsp1_device_get() had succeeded, thus we find ourselves with a kernel
> panic on a null dereference.
> 
> Sorry for the terse notes, they are possibly/probably really for me if I
> get tasked to look back at this.
> --
> Regards
> 
> Kieran
> 
> On 13/12/16 17:59, Kieran Bingham wrote:
> > media_entity_pipeline_stop() can be called through error paths with a
> > NULL entity pipe object. In this instance, stopping is a no-op, so
> > simply return without any action
> > 
> > Signed-off-by: Kieran Bingham 
> > ---
> > 
> > I've marked this patch as RFC, although if deemed suitable, by all means
> > integrate it as is.
> > 
> > When testing suspend/resume operations on VSP1, I encountered a segfault
> > on the WARN_ON(!pipe->streaming_count) line, where 'pipe == NULL'. The
> > simple protection fix is to return early in this instance, as this patch
> > does however:
> > 
> > A) Does this early return path warrant a WARN() statement itself, to
> > identify drivers which are incorrectly calling
> > media_entity_pipeline_stop() with an invalid entity, or would this just
> > be noise ...
> > 
> > and therefore..
> > 
> > B) I also partly assume this patch could simply get NAK'd with a request
> > to go and dig out the root cause of calling media_entity_pipeline_stop()
> > with an invalid entity.
> > 
> > My brief investigation so far here so far shows that it's almost a second
> > order fault - where the first suspend resume cycle completes but leaves
> > the entity in an invalid state having followed an error path - and then
> > on a second suspend/resume - the stop fails with the affected segfault.
> > 
> > If statement A) or B) apply here, please drop this patch from the series,
> > and don't consider it a blocking issue for the other 3 patches.
> > 
> > Kieran
> > 
> >  drivers/media/media-entity.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index c68239e60487..93c9cbf4bf46 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -508,6 +508,8 @@ void __media_entity_pipeline_stop(struct media_entity
> > *entity)> 
> > struct media_entity_graph *graph = >pipe->graph;
> > struct media_pipeline *pipe = entity->pipe;
> > 
> > +   if (!pipe)
> > +   return;
> > 
> > WARN_ON(!pipe->streaming_count);
> > media_entity_graph_walk_start(graph, entity);

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: rcar-du: enable VSPDs on R8A7791

[Laurent Pinchart]

Hi Sergei,

Thank you for the patch.

On Wednesday 14 Dec 2016 23:37:08 Sergei Shtylyov wrote:
> We're going to use R8A7791 VSPDs to control DU, so set the corresponding
> flag.
> 
> Signed-off-by: Sergei Shtylyov 

For the same reason I nacked the corresponding patch to the VSP1 driver, I 
have to nack this one as well. The Gen2 DU has native planes, this patch would 
prevent using them. I don't see a good reason to do so.

> ---
> The patch is against David Airlie's 'linux.git' repo's 'drm-next' branch.
> 
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c |3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Index: linux/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> ===
> --- linux.orig/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ linux/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -90,7 +90,8 @@ static const struct rcar_du_device_info
>  static const struct rcar_du_device_info rcar_du_r8a7791_info = {
>   .gen = 2,
>   .features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
> -   | RCAR_DU_FEATURE_EXT_CTRL_REGS,
> +   | RCAR_DU_FEATURE_EXT_CTRL_REGS
> +   | RCAR_DU_FEATURE_VSP1_SOURCE,
>   .num_crtcs = 2,
>   .routes = {
>   /* R8A779[13] has one RGB output, one LVDS output and one

-- 
Regards,

Laurent Pinchart

[PATCH] drm: rcar-du: enable VSPDs on R8A7791

[Sergei Shtylyov]

We're going to use R8A7791 VSPDs to control DU, so set the corresponding
flag.

Signed-off-by: Sergei Shtylyov 

---
The patch is against David Airlie's 'linux.git' repo's 'drm-next' branch.

 drivers/gpu/drm/rcar-du/rcar_du_drv.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux/drivers/gpu/drm/rcar-du/rcar_du_drv.c
===
--- linux.orig/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ linux/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -90,7 +90,8 @@ static const struct rcar_du_device_info
 static const struct rcar_du_device_info rcar_du_r8a7791_info = {
.gen = 2,
.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
- | RCAR_DU_FEATURE_EXT_CTRL_REGS,
+ | RCAR_DU_FEATURE_EXT_CTRL_REGS
+ | RCAR_DU_FEATURE_VSP1_SOURCE,
.num_crtcs = 2,
.routes = {
/* R8A779[13] has one RGB output, one LVDS output and one

Re: [PATCHv3 1/4] v4l: vsp1: Move vsp1_video_setup_pipeline()

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Tuesday 13 Dec 2016 17:59:41 Kieran Bingham wrote:
> Move the static vsp1_video_setup_pipeline() function in preparation for
> the callee updates so that the vsp1_video_pipeline_run() call can
> configure pipelines following suspend resume actions.
> 
> This commit is just a code move for clarity performing no functional
> change.
> 
> Signed-off-by: Kieran Bingham 

Reviewed-by: Laurent Pinchart 

provided of course we still need this after the rework of 2/4.

> ---
>  drivers/media/platform/vsp1/vsp1_video.c | 82 ++---
>  1 file changed, 41 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index d351b9c768d2..44b687c0b8df
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -350,6 +350,47 @@ static void vsp1_video_frame_end(struct vsp1_pipeline
> *pipe, pipe->buffers_ready |= 1 << video->pipe_index;
>  }
> 
> +static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe)
> +{
> + struct vsp1_entity *entity;
> +
> + /* Determine this pipelines sizes for image partitioning support. */
> + vsp1_video_pipeline_setup_partitions(pipe);
> +
> + /* Prepare the display list. */
> + pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
> + if (!pipe->dl)
> + return -ENOMEM;
> +
> + if (pipe->uds) {
> + struct vsp1_uds *uds = to_uds(>uds->subdev);
> +
> + /* If a BRU is present in the pipeline before the UDS, the 
alpha
> +  * component doesn't need to be scaled as the BRU output alpha
> +  * value is fixed to 255. Otherwise we need to scale the alpha
> +  * component only when available at the input RPF.
> +  */
> + if (pipe->uds_input->type == VSP1_ENTITY_BRU) {
> + uds->scale_alpha = false;
> + } else {
> + struct vsp1_rwpf *rpf =
> + to_rwpf(>uds_input->subdev);
> +
> + uds->scale_alpha = rpf->fmtinfo->alpha;
> + }
> + }
> +
> + list_for_each_entry(entity, >entities, list_pipe) {
> + vsp1_entity_route_setup(entity, pipe->dl);
> +
> + if (entity->ops->configure)
> + entity->ops->configure(entity, pipe, pipe->dl,
> +VSP1_ENTITY_PARAMS_INIT);
> + }
> +
> + return 0;
> +}
> +
>  static void vsp1_video_pipeline_run_partition(struct vsp1_pipeline *pipe,
> struct vsp1_dl_list *dl)
>  {
> @@ -747,47 +788,6 @@ static void vsp1_video_buffer_queue(struct vb2_buffer
> *vb) spin_unlock_irqrestore(>irqlock, flags);
>  }
> 
> -static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe)
> -{
> - struct vsp1_entity *entity;
> -
> - /* Determine this pipelines sizes for image partitioning support. */
> - vsp1_video_pipeline_setup_partitions(pipe);
> -
> - /* Prepare the display list. */
> - pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
> - if (!pipe->dl)
> - return -ENOMEM;
> -
> - if (pipe->uds) {
> - struct vsp1_uds *uds = to_uds(>uds->subdev);
> -
> - /* If a BRU is present in the pipeline before the UDS, the 
alpha
> -  * component doesn't need to be scaled as the BRU output alpha
> -  * value is fixed to 255. Otherwise we need to scale the alpha
> -  * component only when available at the input RPF.
> -  */
> - if (pipe->uds_input->type == VSP1_ENTITY_BRU) {
> - uds->scale_alpha = false;
> - } else {
> - struct vsp1_rwpf *rpf =
> - to_rwpf(>uds_input->subdev);
> -
> - uds->scale_alpha = rpf->fmtinfo->alpha;
> - }
> - }
> -
> - list_for_each_entry(entity, >entities, list_pipe) {
> - vsp1_entity_route_setup(entity, pipe->dl);
> -
> - if (entity->ops->configure)
> - entity->ops->configure(entity, pipe, pipe->dl,
> -VSP1_ENTITY_PARAMS_INIT);
> - }
> -
> - return 0;
> -}
> -
>  static int vsp1_video_start_streaming(struct vb2_queue *vq, unsigned int
> count) {
>   struct vsp1_video *video = vb2_get_drv_priv(vq);

-- 
Regards,

Laurent Pinchart

Re: [PATCHv3 RFC 4/4] media: Catch null pipes on pipeline stop

[Kieran Bingham]

Hello Me.

Ok, so a bit of investigation into *why* we have an unbalanced
media_pipeline stop call.

After a suspend/resume cycle, the function vsp1_pm_runtime_resume() is
called by the PM framework.

The hardware is unable to reset successfully and the reset call returns
-ETIMEDOUT which gets passed back to the PM framework as a failure and
thus the device is not 'resumed'

This process is commenced, as the DU driver tries to enable the VSP, we
get the following call stack:

rcar_du_crtc_resume()
  rcar_du_vsp_enable()
vsp1_du_setup_lif() // returns void
  vsp1_device_get() // returns -ETIMEDOUT,

As the vsp1_du_setup_lif() returns void, the fact that the hardware
failed to start is ignored.

Later we get a call to  rcar_du_vsp_disable(), which again calls into
vsp1_du_setup_lif(), this time to disable the pipeline. And it is here,
that the call to media_pipeline_stop() is an unbalanced call, as the
corresponding media_pipeline_start() would only have been called if the
vsp1_device_get() had succeeded, thus we find ourselves with a kernel
panic on a null dereference.

Sorry for the terse notes, they are possibly/probably really for me if I
get tasked to look back at this.
--
Regards

Kieran


On 13/12/16 17:59, Kieran Bingham wrote:
> media_entity_pipeline_stop() can be called through error paths with a
> NULL entity pipe object. In this instance, stopping is a no-op, so
> simply return without any action
> 
> Signed-off-by: Kieran Bingham 
> ---
> 
> I've marked this patch as RFC, although if deemed suitable, by all means
> integrate it as is.
> 
> When testing suspend/resume operations on VSP1, I encountered a segfault on 
> the
> WARN_ON(!pipe->streaming_count) line, where 'pipe == NULL'. The simple
> protection fix is to return early in this instance, as this patch does 
> however:
> 
> A) Does this early return path warrant a WARN() statement itself, to identify
> drivers which are incorrectly calling media_entity_pipeline_stop() with an
> invalid entity, or would this just be noise ...
> 
> and therefore..
> 
> B) I also partly assume this patch could simply get NAK'd with a request to go
> and dig out the root cause of calling media_entity_pipeline_stop() with an
> invalid entity. 
> 
> My brief investigation so far here so far shows that it's almost a second 
> order
> fault - where the first suspend resume cycle completes but leaves the entity 
> in
> an invalid state having followed an error path - and then on a second
> suspend/resume - the stop fails with the affected segfault.
> 
> If statement A) or B) apply here, please drop this patch from the series, and
> don't consider it a blocking issue for the other 3 patches.
> 
> Kieran
> 
> 
>  drivers/media/media-entity.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index c68239e60487..93c9cbf4bf46 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -508,6 +508,8 @@ void __media_entity_pipeline_stop(struct media_entity 
> *entity)
>   struct media_entity_graph *graph = >pipe->graph;
>   struct media_pipeline *pipe = entity->pipe;
>  
> + if (!pipe)
> + return;
>  
>   WARN_ON(!pipe->streaming_count);
>   media_entity_graph_walk_start(graph, entity);
> 

Applied "spi: sh-msiof: Add R-Car Gen 2 and 3 fallback bindings" to the spi tree

[Mark Brown]

The patch

   spi: sh-msiof: Add R-Car Gen 2 and 3 fallback bindings

has been applied to the spi tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 4286db8456f4fa0c6af2b6b9abc5991a7e7da69c Mon Sep 17 00:00:00 2001
From: Simon Horman 
Date: Mon, 12 Dec 2016 10:49:35 +0100
Subject: [PATCH] spi: sh-msiof: Add R-Car Gen 2 and 3 fallback bindings

In the case of Renesas R-Car hardware we know that there are generations of
SoCs, e.g. Gen 2 and Gen 3. But beyond that it's not clear what the
relationship between IP blocks might be. For example, I believe that
r8a7790 is older than r8a7791 but that doesn't imply that the latter is a
descendant of the former or vice versa.

We can, however, by examining the documentation and behaviour of the
hardware at run-time observe that the current driver implementation appears
to be compatible with the IP blocks on SoCs within a given generation.

For the above reasons and convenience when enabling new SoCs a
per-generation fallback compatibility string scheme is being adopted for
drivers for Renesas SoCs.

Also:
* Deprecate renesas,sh-msiof. It seems poorly named as it is only
  compatible with SH-Mobile. It also appears unused in mainline.

Signed-off-by: Simon Horman 
Reviewed-by: Geert Uytterhoeven 
Signed-off-by: Mark Brown 
---
 Documentation/devicetree/bindings/spi/sh-msiof.txt | 19 +--
 drivers/spi/spi-sh-msiof.c |  4 +++-
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/sh-msiof.txt 
b/Documentation/devicetree/bindings/spi/sh-msiof.txt
index da6614c63796..dc975064fa27 100644
--- a/Documentation/devicetree/bindings/spi/sh-msiof.txt
+++ b/Documentation/devicetree/bindings/spi/sh-msiof.txt
@@ -1,17 +1,23 @@
 Renesas MSIOF spi controller
 
 Required properties:
-- compatible   : "renesas,msiof-" for SoCs,
-"renesas,sh-msiof" for SuperH, or
-"renesas,sh-mobile-msiof" for SH Mobile series.
-Examples with soctypes are:
-"renesas,msiof-r8a7790" (R-Car H2)
+- compatible   : "renesas,msiof-r8a7790" (R-Car H2)
 "renesas,msiof-r8a7791" (R-Car M2-W)
 "renesas,msiof-r8a7792" (R-Car V2H)
 "renesas,msiof-r8a7793" (R-Car M2-N)
 "renesas,msiof-r8a7794" (R-Car E2)
 "renesas,msiof-r8a7796" (R-Car M3-W)
 "renesas,msiof-sh73a0" (SH-Mobile AG5)
+"renesas,sh-mobile-msiof" (generic SH-Mobile 
compatibile device)
+"renesas,rcar-gen2-msiof" (generic R-Car Gen2 
compatible device)
+"renesas,rcar-gen3-msiof" (generic R-Car Gen3 
compatible device)
+"renesas,sh-msiof"  (deprecated)
+
+When compatible with the generic version, nodes
+must list the SoC-specific version corresponding
+to the platform first followed by the generic
+version.
+
 - reg  : A list of offsets and lengths of the register sets for
 the device.
 If only one register set is present, it is to be used
@@ -61,7 +67,8 @@ Documentation/devicetree/bindings/pinctrl/renesas,*.
 Example:
 
msiof0: spi@e6e2 {
-   compatible = "renesas,msiof-r8a7791";
+   compatible = "renesas,msiof-r8a7791",
+"renesas,rcar-gen2-msiof";
reg = <0 0xe6e2 0 0x0064>;
interrupts = <0 156 IRQ_TYPE_LEVEL_HIGH>;
clocks = <_clks R8A7791_CLK_MSIOF0>;
diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index 0012ad02e569..471ca211b76c 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -973,14 +973,16 @@ static const struct sh_msiof_chipdata r8a779x_data = {
 };
 
 static const struct of_device_id sh_msiof_match[] = {
-   { 

Re: [PATCHv3 RFC 4/4] media: Catch null pipes on pipeline stop

[Kieran Bingham]

Hi Sakari,

On 14/12/16 12:43, Sakari Ailus wrote:
> Hi Kieran,
> 
> On Wed, Dec 14, 2016 at 12:27:37PM +, Kieran Bingham wrote:
>> Hi Sakari,
>>
>> On 14/12/16 07:28, Sakari Ailus wrote:
>>> Hi Kieran,
>>>
>>> On Tue, Dec 13, 2016 at 05:59:44PM +, Kieran Bingham wrote:
 media_entity_pipeline_stop() can be called through error paths with a
 NULL entity pipe object. In this instance, stopping is a no-op, so
 simply return without any action
>>>
>>> The approach of returning silently is wrong; the streaming count is indeed a
>>> count: you have to decrement it the exactly same number of times it's been
>>> increased. Code that attempts to call __media_entity_pipeline_stop() on an
>>> entity that's not streaming is simply buggy.
>>
>> Ok, Thanks for the heads up on where to look, as I suspected we are in
>> section B) of my comments below :D
>>
>>> I've got a patch here that adds a warning to graph traversal on streaming
>>> count being zero. I sent a pull request including that some days ago:
>>>
>>> 
>>> 
>>
>> Excellent, thanks, I've merged your branch into mine, and I'll
>> investigate where the error is actually coming from.
>>
>> --
>> Ok - so I've merged your patches, (and dropped this patch) but they
>> don't fire any warning before I hit my null-pointer dereference in
>> __media_pipeline_stop(), on the WARN_ON(!pipe->streaming_count);
>>
>> So I think this is a case of calling stop on an invalid entity rather
>> than an unbalanced start/stop somehow:
> 
> Not necessarily. The pipe is set to NULL if the count goes to zero.
> 
> This check should be dropped, it's ill-placed anyway: if streaming count is
> zero the pipe is expected to be NULL anyway in which case you get a NULL
> pointer exception (that you saw there). With the check removed, you should
> get the warning instead.

Ahh, yes, I'd missed the part there that was setting it to NULL.

However, it will still segfault ... (though hopefully after the warning)
as the last line of the function states:

if (!--pipe->streaming_count)
media_entity_graph_walk_cleanup(graph);

So we will hit a null deref on the pipe->streaming_count there, which
still leaves an unbalanced stop as a kernel panic.

In fact, I've just tested removing that WARN_ON(!pipe->streaming_count);
 - it doesn't even get that far - and segfaults in

media_entity_graph_walk_cleanup(graph);

[   80.916558] Unable to handle kernel NULL pointer dereference at
virtual address 0110

[   81.769492] [] media_graph_walk_start+0x20/0xf0
[   81.776615] [] __media_pipeline_stop+0x3c/0xd8
[   81.783644] [] media_pipeline_stop+0x34/0x48
[   81.790505] [] vsp1_du_setup_lif+0x68/0x5a8
[   81.797279] [] rcar_du_vsp_disable+0x2c/0x38
[   81.804137] [] rcar_du_crtc_stop+0x198/0x1e8
[   81.810984] [] rcar_du_crtc_disable+0x20/0x38

due to the fact that 'graph' is dereferenced through the 'pipe'.

{
/* Entity->pipe is NULL, therefore 'graph' is invalid */
struct media_graph *graph = >pipe->graph;
struct media_pipeline *pipe = entity->pipe;

media_graph_walk_start(graph, entity);
...
}

So I suspect we do still need a warning or check in there somewhere.
Something to tell the developer that there is an unbalanced stop, would
be much better than a panic/segfault...

(And of course, we still need to look at the actual unbalanced stop in
vsp1_du_setup_lif!)

--
Kieran

Re: [PATCH spi/for-next] spi: sh-msiof: Add R-Car Gen 2 and 3 fallback bindings

[Mark Brown]

On Mon, Dec 12, 2016 at 10:49:35AM +0100, Simon Horman wrote:

> + { .compatible = "renesas,sh-msiof",.data = _data }, // 
> Deprecated

C++ style comment here, please send a followup fixing this.


signature.asc
Description: PGP signature

Re: [PATCHv3 2/4] v4l: vsp1: Refactor video pipeline configuration

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Tuesday 13 Dec 2016 17:59:42 Kieran Bingham wrote:
> With multiple inputs through the BRU it is feasible for the streams to
> race each other at stream-on.

Could you please explain the race condition in the commit message ? The issue 
is that multiple VIDIOC_STREAMON calls racing each other could have process 
N-1 skipping over the pipeline setup section and then start the pipeline, if 
videobuf2 has already enqueued buffers to the driver for process N but not 
called the .start_streaming() operation yet.

> In the case of the video pipelines, this
> can present two serious issues.
> 
>  1) A null-dereference if the pipe->dl is committed at the same time as
> the vsp1_video_setup_pipeline() is processing
> 
>  2) A hardware hang, where a display list is committed without having
> called vsp1_video_setup_pipeline() first
> 
> Along side these race conditions, the work done by
> vsp1_video_setup_pipeline() is undone by the re-initialisation during a
> suspend resume cycle, and an active pipeline does not attempt to
> reconfigure the correct routing and init parameters for the entities.
> 
> To repair all of these issues, we can move the call to a conditional
> inside vsp1_video_pipeline_run() and ensure that this can only be called
> on the last stream which calls into vsp1_video_start_streaming()
> 
> As a convenient side effect of this, by specifying that the
> configuration has been lost during suspend/resume actions - the
> vsp1_video_pipeline_run() call can re-initialise pipelines when
> necessary thus repairing resume actions for active m2m pipelines.
> 
> Signed-off-by: Kieran Bingham 
> 
> ---
> v3:
>  - Move 'flag reset' to be inside the vsp1_reset_wpf() function call
>  - Tidy up the wpf->pipe reference for the configured flag
> 
>  drivers/media/platform/vsp1/vsp1_drv.c   |  4 
>  drivers/media/platform/vsp1/vsp1_pipe.c  |  1 +
>  drivers/media/platform/vsp1/vsp1_pipe.h  |  2 ++
>  drivers/media/platform/vsp1/vsp1_video.c | 20 +---
>  4 files changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> b/drivers/media/platform/vsp1/vsp1_drv.c index 57c713a4e1df..1dc3726c4e83
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -413,6 +413,7 @@ static int vsp1_create_entities(struct vsp1_device
> *vsp1)
> 
>  int vsp1_reset_wpf(struct vsp1_device *vsp1, unsigned int index)
>  {
> + struct vsp1_rwpf *wpf = vsp1->wpf[index];
>   unsigned int timeout;
>   u32 status;
> 
> @@ -429,6 +430,9 @@ int vsp1_reset_wpf(struct vsp1_device *vsp1, unsigned
> int index) usleep_range(1000, 2000);
>   }
> 
> + if (wpf->pipe)
> + wpf->pipe->configured = false;
> +
>   if (!timeout) {
>   dev_err(vsp1->dev, "failed to reset wpf.%u\n", index);
>   return -ETIMEDOUT;
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c
> b/drivers/media/platform/vsp1/vsp1_pipe.c index 756ca4ea7668..7ddf862ee403
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.c
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.c
> @@ -208,6 +208,7 @@ void vsp1_pipeline_init(struct vsp1_pipeline *pipe)
> 
>   INIT_LIST_HEAD(>entities);
>   pipe->state = VSP1_PIPELINE_STOPPED;
> + pipe->configured = false;
>  }
> 
>  /* Must be called with the pipe irqlock held. */
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
> b/drivers/media/platform/vsp1/vsp1_pipe.h index ac4ad261..0743b9fcb655
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> @@ -61,6 +61,7 @@ enum vsp1_pipeline_state {
>   * @pipe: the media pipeline
>   * @irqlock: protects the pipeline state
>   * @state: current state
> + * @configured: determines routing configuration active on cell.

I'm not sure to understand that. How about "true if the pipeline has been set 
up" ? Or maybe "true if the pipeline has been set up for video streaming" as 
it only applies to pipelines handled through the V4L2 API ?

>   * @wq: wait queue to wait for state change completion
>   * @frame_end: frame end interrupt handler
>   * @lock: protects the pipeline use count and stream count
> @@ -86,6 +87,7 @@ struct vsp1_pipeline {
> 
>   spinlock_t irqlock;
>   enum vsp1_pipeline_state state;
> + bool configured;
>   wait_queue_head_t wq;
> 
>   void (*frame_end)(struct vsp1_pipeline *pipe);
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index 44b687c0b8df..7ff9f4c19ff0
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -388,6 +388,8 @@ static int vsp1_video_setup_pipeline(struct
> vsp1_pipeline *pipe) VSP1_ENTITY_PARAMS_INIT);
>   }
> 
> + pipe->configured = true;
> +
>   return 0;
>  }
> 
> @@ -411,6 +413,9 @@ static void 

RE: clk: clk-mstp has never worked for RZ/A1

[Chris Brandt]

Hi Geert,

On Dec 14, 2016, Geert Uytterhoeven wrote:
> Another option is to let the driver bind against "renesas,r7s72100-mstp-
> clocks", and switch to 8-bit access mode.
> 
> CLK_OF_DECLARE(cpg_mstp_clks, "renesas,r7s72100-mstp-clocks",
> cpg_mstp_clocks_init8);
> 
> cpg_mstp_clocks_init8() can set a gloal flag, and call
> cpg_mstp_clocks_init().
> 
> The latter means no DT update is needed, and thus preserves compatibility
> with old DTBs.

I just coded that and it works good. Thank you.


Now, one more question before I submit a patch for review:

I would like to add a "Fixes:" to the commit log so it can be considered for 
4.9-stable (in reality it could go all the way back to when r7s72100 support 
was added)

But, the current file path didn't exist until after commit b3a33077c0dd ("clk: 
renesas: move drivers to renesas directory") on 2016-03-02.

So should I use that commit for my 'Fixes'?


Thanks,
Chris

ravb/sh_eth/b44: BUG: sleeping function called from invalid context

[Geert Uytterhoeven]

Hi,

When CONFIG_DEBUG_ATOMIC_SLEEP=y, running "ethtool -s eth0 speed 100"
on Salvator-X gives:

BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
in_atomic(): 1, irqs_disabled(): 128, pid: 1683, name: ethtool
CPU: 0 PID: 1683 Comm: ethtool Tainted: GW
4.9.0-salvator-x-00426-g326519df42c65007-dirty #976
Hardware name: Renesas Salvator-X board based on r8a7796 (DT)
Call trace:
[] dump_backtrace+0x0/0x208
[] show_stack+0x14/0x1c
[] dump_stack+0x94/0xb4
[] ___might_sleep+0x108/0x11c
[] __might_sleep+0x84/0x94
[] mutex_lock+0x24/0x40
[] phy_start_aneg+0x20/0x130
[] phy_ethtool_ksettings_set+0xd0/0xe8
[] ravb_set_link_ksettings+0x4c/0xa4
[] ethtool_set_settings+0xec/0xfc
[] dev_ethtool+0x188/0x17c4
[] dev_ioctl+0x53c/0x6b8
[] sock_do_ioctl.constprop.45+0x3c/0x4c
[] sock_ioctl+0x33c/0x370
[] vfs_ioctl+0x20/0x38
[] do_vfs_ioctl+0x844/0x954
[] SyS_ioctl+0x44/0x68
[] el0_svc_naked+0x24/0x28

ravb_set_link_ksettings() calls phy_ethtool_ksettings_set() with a spinlock
held and interrupts disabled, while phy_start_aneg() tries to obtain a mutex.

The same issue is present in sh_eth_set_link_ksettings() (verified) and
b44_set_link_ksettings() (code inspection).

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

Re: [PATCH 5/7] serial: sh-sci: SCIFA/B RX FIFO software timeout

[Geert Uytterhoeven]

Hi Uli.

On Fri, Dec 9, 2016 at 1:36 PM, Ulrich Hecht
 wrote:
> Implements support for FIFO fill thresholds greater than one with software
> timeout.
>
> This mechanism is not possible (or at least not useful) on SCIF family
> hardware other than SCIFA and SCIFB because they do not support turning off
> the DR hardware timeout interrupt separately from the RI interrupt.

What about HSCIF? Your code does handle HSCIF (cfr. HSRTRGR below)?

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c

> @@ -1159,6 +1162,24 @@ static int scif_set_rtrg(struct uart_port *port, int 
> rx_trig)
> return rx_trig;
>  }
>
> +static int scif_rtrg_enabled(struct uart_port *port)
> +{
> +   if (sci_getreg(port, HSRTRGR)->size)
> +   return serial_port_in(port, HSRTRGR) != 0;
> +   else
> +   return (serial_port_in(port, SCFCR) &
> +   (SCFCR_RTRG0 | SCFCR_RTRG1)) != 0;
> +}

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

Re: [PATCH 4/7] serial: sh-sci: increase RX FIFO trigger defaults for (H)SCIF

[Geert Uytterhoeven]

Hi Uli,

On Fri, Dec 9, 2016 at 1:36 PM, Ulrich Hecht
 wrote:
> Signed-off-by: Ulrich Hecht 
> ---
>  drivers/tty/serial/sh-sci.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 844288f..ce3cf03 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c

> @@ -2729,6 +2734,7 @@ static int sci_init_single(struct platform_device *dev,
> sci_port->overrun_reg = SCLSR;
> sci_port->overrun_mask = SCLSR_ORER;
> sci_port->sampling_rate_mask = SCI_SR_RANGE(8, 32);
> +   sci_port->rx_trigger = 64;
> break;
> case PORT_SCIFA:
> port->fifosize = 64;
> @@ -2747,12 +2753,14 @@ static int sci_init_single(struct platform_device 
> *dev,
> sci_port->overrun_mask = SCLSR_ORER;
> sci_port->sampling_rate_mask = SCI_SR(32);
> }
> +   sci_port->rx_trigger = 8;

PORT_SCIF is also used with SCIx_SH7705_SCIF_REGTYPE, so this should
be moved inside the "else" branch above.
Then, what to do with the "if" branch?

>From the sh7705/sh7720/sh7721 datasheets, it looks a lot like SCIFA, and thus
would need "sci_port->rx_trigger = 32", and the same handling as SCIFA in
scif_set_rtrg().

See also "[RFC] serial: sh-sci: Correct FIFO stages on sh7705/sh7720/sh7721"
(https://patchwork.kernel.org/patch/6810191/).

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

Re: [PATCH 7/7] arm64: dts: r8a7796: salvator-x: add SCIF1 (DEBUG1)

[Geert Uytterhoeven]

Hi Uli,

On Fri, Dec 9, 2016 at 1:36 PM, Ulrich Hecht
 wrote:
> Enables the SCIF hooked up to the DEBUG1 connector.
>
> Signed-off-by: Ulrich Hecht 
> ---
>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts | 12 
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts 
> b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
> index da46ac7..9204691 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
> @@ -18,6 +18,7 @@
>
> aliases {
> serial0 = 
> +   serial1 = 

Is there any specific reason you chose scif1 over hscif1?

> ethernet0 = 
> };
>
> @@ -140,6 +141,11 @@
> pinctrl-0 = <_clk_pins>;
> pinctrl-names = "default";
>
> +   scif1_pins: scif1 {
> +   groups = "scif1_data_a";

Please add "scif1_ctrl_a".

> +   function = "scif1";
> +   };
> +
> scif2_pins: scif2 {
> groups = "scif2_data_a";
> function = "scif2";
> @@ -280,6 +286,12 @@
> status = "okay";
>  };
>
> + {
> +   pinctrl-0 = <_pins>;
> +   pinctrl-names = "default";

Please add "uart-has-rtscts".

> +   status = "okay";
> +};

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

Re: [PATCH 6/7] serial: sh-sci: make RX FIFO parameters tunable via sysfs

[Geert Uytterhoeven]

Hi Uli,

On Fri, Dec 9, 2016 at 1:36 PM, Ulrich Hecht
 wrote:
> Allows tuning of the RX FIFO fill threshold and timeout. (The latter is
> only applicable to SCIFA and SCIFB).
>
> Signed-off-by: Ulrich Hecht 

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c

> @@ -3193,6 +3262,19 @@ static int sci_probe(struct platform_device *dev)
> if (ret)
> return ret;
>
> +   if (sp->port.fifosize > 1) {
> +   ret = sysfs_create_file(>dev.kobj,
> +   _attr_rx_fifo_trigger.attr);
> +   if (ret)
> +   return ret;
> +   }
> +   if (sp->port.type == PORT_SCIFA || sp->port.type ==  PORT_SCIFB) {
> +   ret = sysfs_create_file(>dev.kobj,
> +   _attr_rx_fifo_timeout.attr);
> +   if (ret)
> +   return ret;

Returning here may leak the rx_fifo_trigger file created above.

> +   }

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

Re: [PATCH 3/7] serial: sh-sci: implement FIFO threshold register setting

[Geert Uytterhoeven]

Hi Uli,

On Fri, Dec 9, 2016 at 1:36 PM, Ulrich Hecht
 wrote:
> Sets the closest match for a desired RX trigger level.
>
> Signed-off-by: Ulrich Hecht 

Reviewed-by: Geert Uytterhoeven 

> index de25db8..844288f 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1103,6 +1103,61 @@ static int sci_handle_breaks(struct uart_port *port)
> return copied;
>  }
>
> +static int scif_set_rtrg(struct uart_port *port, int rx_trig)
> +{

> +   if (port->type == PORT_SCIF) {

> +   } else if (port->type == PORT_SCIFA || port->type == PORT_SCIFB) {

> +   } else {
> +   WARN(1, "unknown FIFO configuration");

This cannot happen, right?

> +   return 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

Re: [PATCH 2/7] serial: sh-sci: consider DR (data ready) bit adequately

[Geert Uytterhoeven]

Hi Uli,

On Fri, Dec 9, 2016 at 1:36 PM, Ulrich Hecht
 wrote:
> To allow operation with a higher RX FIFO interrupt threshold in PIO
> mode, it is necessary to consider the DR bit ("FIFO not full, but no
> data received for 1.5 frames") as an indicator that data can be read.
> Otherwise the driver will let data rot in the FIFO until the threshold
> is reached.
>
> Signed-off-by: Ulrich Hecht 

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -677,7 +677,7 @@ static int sci_poll_get_char(struct uart_port *port)
> break;
> } while (1);
>
> -   if (!(status & SCxSR_RDxF(port)))
> +   if (!(status & (SCxSR_RDxF(port) | SCxSR_DR(port
> return NO_POLL_CHAR;
>
> c = serial_port_in(port, SCxRDR);

> --- a/drivers/tty/serial/sh-sci.h
> +++ b/drivers/tty/serial/sh-sci.h
> @@ -156,6 +156,7 @@ enum {
>  #define SCxSR_FER(port)(((port)->type == PORT_SCI) ? SCI_FER 
>: SCIF_FER)
>  #define SCxSR_PER(port)(((port)->type == PORT_SCI) ? SCI_PER 
>: SCIF_PER)
>  #define SCxSR_BRK(port)(((port)->type == PORT_SCI) ? 0x00
>: SCIF_BRK)
> +#define SCxSR_DR(port) (((port)->type == PORT_SCI) ? 0x00   : 
> SCIF_DR)

Makes sense, as SCIF_RDxF_CLEAR already includes SCIF_DR, and we thus already
clear both RDF and DR.

However, if you would handle this inside the SCxSR_RDxF() macro, your patch
would reduce to a single line:

-#define SCxSR_RDxF(port)(((port)->type == PORT_SCI) ?
SCI_RDRF   : SCIF_RDF)
+#define SCxSR_RDxF(port)(((port)->type == PORT_SCI) ?
SCI_RDRF   : SCIF_DR | SCIF_RDF)

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

Re: [PATCHv2 2/5] sh_eth: enable wake-on-lan for Gen2 devices

[Sergei Shtylyov]

Hello!

   You forgot "R-Car" before "Gen2" in the subject.

On 12/12/2016 07:09 PM, Niklas Söderlund wrote:


Tested on Gen2 r8a7791/Koelsch.

Signed-off-by: Niklas Söderlund 
---
 drivers/net/ethernet/renesas/sh_eth.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c 
b/drivers/net/ethernet/renesas/sh_eth.c
index 87640b9..348ed22 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -624,8 +624,9 @@ static struct sh_eth_cpu_data r8a779x_data = {

.register_type  = SH_ETH_REG_FAST_RCAR,

-   .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD,
-   .ecsipr_value   = ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP,
+   .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD | ECSR_MPD,
+   .ecsipr_value   = ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP |
+ ECSIPR_MPDIP,


  These expressions seem to have been sorted by the bit # before your patch, 
now they aren't... care to fix? :-)


[...]

MBR, Sergei

Re: [PATCH 1/7] serial: sh-sci: add FIFO trigger bits

[Geert Uytterhoeven]

Hi Ulrich,

On Fri, Dec 9, 2016 at 1:36 PM, Ulrich Hecht
 wrote:
> Defines the bits controlling FIFO thresholds, adds the additional
> HSCIF registers to the register map.
>
> Signed-off-by: Ulrich Hecht 

Reviewed-by: Geert Uytterhoeven 

Minor comment below.

> --- a/drivers/tty/serial/sh-sci.h
> +++ b/drivers/tty/serial/sh-sci.h
> @@ -29,6 +29,8 @@ enum {
> SCPDR,  /* Serial Port Data Register */
> SCDL,   /* BRG Frequency Division Register */
> SCCKS,  /* BRG Clock Select Register */
> +   HSRTRGR,/* Receive FIFO Data Count Register */

Receive FIFO Data Count Trigger Register

(to avoid confusion with the FIFO Data Count Register)

> +   HSTTRGR,/* Transmit FIFO Data Count Register 
> */

Transmit FIFO Data Count Trigger Register

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

Re: [RFCv2] dts: arm: renesas: koelsh: Add MAX11100 device

[Geert Uytterhoeven]

On Tue, Dec 13, 2016 at 1:43 PM, Jacopo Mondi  wrote:
>
> diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts 
> b/arch/arm/boot/dts/r8a7791-koelsch.dts
> index 5405d33..1756b86 100644
> --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
> +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts

> +   spi_gpio: spi-gpio@0 {
> +   compatible = "spi-gpio";
> +   #address-cells = <1>;
> +   #size-cells = <0>;
> +   gpio-sck = < 27 GPIO_ACTIVE_HIGH>;
> +   gpio-miso = < 25 GPIO_ACTIVE_HIGH>;
> +   num-chipselects = <1>;
> +   cs-gpios = < 26 GPIO_ACTIVE_LOW>;
> +   status = "okay";
> +
> +   adc0: max11100@0 {

The node name should be a generic name, the label should be the specific name.
I.e. "max11100: adc@0"

> +   compatible = "maxim,max11100";
> +   reg = <0>;
> +   spi-max-frequency = <250>;
> +   vref-supply = <_adc0>;

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

Re: [PATCHv3 RFC 4/4] media: Catch null pipes on pipeline stop

[Sakari Ailus]

Hi Kieran,

On Wed, Dec 14, 2016 at 12:27:37PM +, Kieran Bingham wrote:
> Hi Sakari,
> 
> On 14/12/16 07:28, Sakari Ailus wrote:
> > Hi Kieran,
> > 
> > On Tue, Dec 13, 2016 at 05:59:44PM +, Kieran Bingham wrote:
> >> media_entity_pipeline_stop() can be called through error paths with a
> >> NULL entity pipe object. In this instance, stopping is a no-op, so
> >> simply return without any action
> > 
> > The approach of returning silently is wrong; the streaming count is indeed a
> > count: you have to decrement it the exactly same number of times it's been
> > increased. Code that attempts to call __media_entity_pipeline_stop() on an
> > entity that's not streaming is simply buggy.
> 
> Ok, Thanks for the heads up on where to look, as I suspected we are in
> section B) of my comments below :D
> 
> > I've got a patch here that adds a warning to graph traversal on streaming
> > count being zero. I sent a pull request including that some days ago:
> > 
> > 
> > 
> 
> Excellent, thanks, I've merged your branch into mine, and I'll
> investigate where the error is actually coming from.
> 
> --
> Ok - so I've merged your patches, (and dropped this patch) but they
> don't fire any warning before I hit my null-pointer dereference in
> __media_pipeline_stop(), on the WARN_ON(!pipe->streaming_count);
> 
> So I think this is a case of calling stop on an invalid entity rather
> than an unbalanced start/stop somehow:

Not necessarily. The pipe is set to NULL if the count goes to zero.

This check should be dropped, it's ill-placed anyway: if streaming count is
zero the pipe is expected to be NULL anyway in which case you get a NULL
pointer exception (that you saw there). With the check removed, you should
get the warning instead.

> 
> [  613.830334] vsp1 fea38000.vsp: failed to reset wpf.0
> [  613.838137] PM: resume of devices complete after 124.410 msecs
> [  613.847390] PM: Finishing wakeup.
> [  613.852247] Restarting tasks ... done.
> [  615.864801] ravb e680.ethernet eth0: Link is Up - 100Mbps/Full -
> flow control rx/tx
> [  617.011299] vsp1 fea38000.vsp: failed to reset wpf.0
> [  617.01] vsp1 fea38000.vsp: DRM pipeline stop timeout
> [  617.024649] Unable to handle kernel NULL pointer dereference at
> virtual address 
> [  617.034273] pgd = ff80098c5000
> [  617.039171] [] *pgd=00073fffe003[  617.043336] ,
> *pud=00073fffe003
> , *pmd=[  617.050353]
> [  617.053282] Internal error: Oops: 9605 [#1] PREEMPT SMP
> [  617.060309] Modules linked in:
> [  617.064793] CPU: 1 PID: 683 Comm: kworker/1:2 Not tainted
> 4.9.0-00506-g4d2a939ea654 #350
> [  617.074320] Hardware name: Renesas Salvator-X board based on r8a7795 (DT)
> [  617.082578] Workqueue: events drm_mode_rmfb_work_fn
> [  617.04] task: ffc6fabd2b00 task.stack: ffc6f9d9
> [  617.096246] PC is at __media_pipeline_stop+0x24/0xe8
> [  617.102644] LR is at media_pipeline_stop+0x34/0x48
> 
> 
> 
> [  617.863386] [] __media_pipeline_stop+0x24/0xe8
> [  617.870417] [] media_pipeline_stop+0x34/0x48
> [  617.877272] [] vsp1_du_setup_lif+0x68/0x5a8
> [  617.884047] [] rcar_du_vsp_disable+0x2c/0x38
> [  617.890898] [] rcar_du_crtc_stop+0x198/0x1e8
> [  617.897749] [] rcar_du_crtc_disable+0x20/0x38
> [  617.904683] []
> drm_atomic_helper_commit_modeset_disables+0x1b0/0x3d8
> [  617.913634] [] rcar_du_atomic_complete+0x34/0xc8
> [  617.920828] [] rcar_du_atomic_commit+0x2c0/0x2d0
> [  617.928012] [] drm_atomic_commit+0x5c/0x68
> [  617.934663] [] drm_atomic_helper_set_config+0x90/0xd0
> [  617.942288] [] drm_mode_set_config_internal+0x70/0x100
> [  617.949996] [] drm_crtc_force_disable+0x30/0x40
> [  617.957084] [] drm_framebuffer_remove+0x100/0x128
> [  617.964347] [] drm_mode_rmfb_work_fn+0x48/0x60
> [  617.971352] [] process_one_work+0x1e0/0x738
> [  617.978084] [] worker_thread+0x25c/0x4a0
> [  617.984546] [] kthread+0xd4/0xe8
> [  617.990305] [] ret_from_fork+0x10/0x50
> 
> 
> So, I'll have to schedule some time to investigate this deeper and find
> out where we are calling stop on an invalid entity object.
> 
> I agree that this patch should simply be dropped though, it was more to
> promote a bit of discussion on the area to help me understand what was
> going on, which it has!.
> 
> Thankyou Sakari!
> 
> --
> Regards
> 
> Kieran
> 
> 
> > I think the check here could simply be removed as the check is done for
> > every entity in the pipeline with the above patch.
> > 
> > If there's still a wish to check for the pipe field which should not be
> > written by drivers, it should be done during pipeline traversal so that it
> > applies to all entities in the pipeline, not just where traversal starts.
> > 
> >>
> >> Signed-off-by: Kieran Bingham 
> >> ---
> >>
> >> I've marked this patch as RFC, although if deemed suitable, by 

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[Geert Uytterhoeven]

Hi Jacopo,

On Wed, Dec 14, 2016 at 12:54 PM, jac...@jmondi.org  wrote:
>>> +struct max11100_state {
>>> +   const struct max11100_chip_desc *desc;
>>> +   struct spi_device *spi;
>>> +   int vref_uv;
>>
>> unsi
>
> Was this a suggestion to turn "vref_uv" into unsigned?

It was a suggestion I started writing, and aborted improperly when noticing
the below ;-)

> As you can see in probe function it can get assigned to -EINVAL when a dummy
> regulator is returned from regulator-core.
> I had to make it signed for this reason

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

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[jac...@jmondi.org]

Hi Geert,

On 13/12/2016 14:21, Geert Uytterhoeven wrote:

Hi Jacopo,

On Tue, Dec 13, 2016 at 1:37 PM, Jacopo Mondi  wrote:

Add IIO driver for Maxim MAX11100 single-channel ADC.
Add DT bindings documentation.

Signed-off-by: Jacopo Mondi 



--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
 obj-$(CONFIG_LTC2485) += ltc2485.o
 obj-$(CONFIG_MAX1027) += max1027.o
+obj-$(CONFIG_MAX11100) += max11100.o
 obj-$(CONFIG_MAX1363) += max1363.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
 obj-$(CONFIG_MCP3422) += mcp3422.o
diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
new file mode 100644
index 000..f372ad8
--- /dev/null
+++ b/drivers/iio/adc/max11100.c



+/*
+ * LSB is the ADC single digital step
+ * 1 LSB = (vref / 2 ^ 16)
+ * AIN = (DIN * LSB)
+ */
+#define MAX11100_LSB_DIV   (1 << 16)
+#define MAX11100_LSB(vref) (vref / MAX11100_LSB_DIV)


DIV_ROUND_CLOSEST()?


+struct max11100_state {
+   const struct max11100_chip_desc *desc;
+   struct spi_device *spi;
+   int vref_uv;


unsi


Was this a suggestion to turn "vref_uv" into unsigned?
As you can see in probe function it can get assigned to -EINVAL when a 
dummy regulator is returned from regulator-core.

I had to make it signed for this reason

Thanks
  j



It's good practice to move the smaller members to the end of the structure,
to avoid gaps due to alignment rules (struct mutex needs to be 64-bit aligned
on 64-bit platforms).


+   struct mutex lock;
+};



+static struct max11100_chip_desc {
+   unsigned int num_chan;
+   const struct iio_chan_spec *channels;


Same here (but it won't have any effect for now).


+} max11100_desc = {
+   .num_chan = ARRAY_SIZE(max11100_channels),
+   .channels = max11100_channels,
+};



+static int max11100_read_raw(struct iio_dev *indio_dev,
+struct iio_chan_spec const *chan,
+int *val, int *val2, long mask)
+{
+   int ret;
+   struct max11100_state *state = iio_priv(indio_dev);
+   uint8_t buffer[3];



+   *val = be16_to_cpu(*(uint16_t *)[1]);


Reading the uint16_t will be an unaligned load, which is not supported on all
platforms. So you either have to use get_unaligned_le16(), or assemble the
value yourself, like you did in v1.


+   *val = *val * MAX11100_LSB(state->vref_uv) / 1000;


DIV_ROUND_CLOSEST()?

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

Re: [PATCHv3 RFC 4/4] media: Catch null pipes on pipeline stop

[Kieran Bingham]

Hi Sakari,

On 14/12/16 07:28, Sakari Ailus wrote:
> Hi Kieran,
> 
> On Tue, Dec 13, 2016 at 05:59:44PM +, Kieran Bingham wrote:
>> media_entity_pipeline_stop() can be called through error paths with a
>> NULL entity pipe object. In this instance, stopping is a no-op, so
>> simply return without any action
> 
> The approach of returning silently is wrong; the streaming count is indeed a
> count: you have to decrement it the exactly same number of times it's been
> increased. Code that attempts to call __media_entity_pipeline_stop() on an
> entity that's not streaming is simply buggy.

Ok, Thanks for the heads up on where to look, as I suspected we are in
section B) of my comments below :D

> I've got a patch here that adds a warning to graph traversal on streaming
> count being zero. I sent a pull request including that some days ago:
> 
> 
> 

Excellent, thanks, I've merged your branch into mine, and I'll
investigate where the error is actually coming from.

--
Ok - so I've merged your patches, (and dropped this patch) but they
don't fire any warning before I hit my null-pointer dereference in
__media_pipeline_stop(), on the WARN_ON(!pipe->streaming_count);

So I think this is a case of calling stop on an invalid entity rather
than an unbalanced start/stop somehow:

[  613.830334] vsp1 fea38000.vsp: failed to reset wpf.0
[  613.838137] PM: resume of devices complete after 124.410 msecs
[  613.847390] PM: Finishing wakeup.
[  613.852247] Restarting tasks ... done.
[  615.864801] ravb e680.ethernet eth0: Link is Up - 100Mbps/Full -
flow control rx/tx
[  617.011299] vsp1 fea38000.vsp: failed to reset wpf.0
[  617.01] vsp1 fea38000.vsp: DRM pipeline stop timeout
[  617.024649] Unable to handle kernel NULL pointer dereference at
virtual address 
[  617.034273] pgd = ff80098c5000
[  617.039171] [] *pgd=00073fffe003[  617.043336] ,
*pud=00073fffe003
, *pmd=[  617.050353]
[  617.053282] Internal error: Oops: 9605 [#1] PREEMPT SMP
[  617.060309] Modules linked in:
[  617.064793] CPU: 1 PID: 683 Comm: kworker/1:2 Not tainted
4.9.0-00506-g4d2a939ea654 #350
[  617.074320] Hardware name: Renesas Salvator-X board based on r8a7795 (DT)
[  617.082578] Workqueue: events drm_mode_rmfb_work_fn
[  617.04] task: ffc6fabd2b00 task.stack: ffc6f9d9
[  617.096246] PC is at __media_pipeline_stop+0x24/0xe8
[  617.102644] LR is at media_pipeline_stop+0x34/0x48



[  617.863386] [] __media_pipeline_stop+0x24/0xe8
[  617.870417] [] media_pipeline_stop+0x34/0x48
[  617.877272] [] vsp1_du_setup_lif+0x68/0x5a8
[  617.884047] [] rcar_du_vsp_disable+0x2c/0x38
[  617.890898] [] rcar_du_crtc_stop+0x198/0x1e8
[  617.897749] [] rcar_du_crtc_disable+0x20/0x38
[  617.904683] []
drm_atomic_helper_commit_modeset_disables+0x1b0/0x3d8
[  617.913634] [] rcar_du_atomic_complete+0x34/0xc8
[  617.920828] [] rcar_du_atomic_commit+0x2c0/0x2d0
[  617.928012] [] drm_atomic_commit+0x5c/0x68
[  617.934663] [] drm_atomic_helper_set_config+0x90/0xd0
[  617.942288] [] drm_mode_set_config_internal+0x70/0x100
[  617.949996] [] drm_crtc_force_disable+0x30/0x40
[  617.957084] [] drm_framebuffer_remove+0x100/0x128
[  617.964347] [] drm_mode_rmfb_work_fn+0x48/0x60
[  617.971352] [] process_one_work+0x1e0/0x738
[  617.978084] [] worker_thread+0x25c/0x4a0
[  617.984546] [] kthread+0xd4/0xe8
[  617.990305] [] ret_from_fork+0x10/0x50


So, I'll have to schedule some time to investigate this deeper and find
out where we are calling stop on an invalid entity object.

I agree that this patch should simply be dropped though, it was more to
promote a bit of discussion on the area to help me understand what was
going on, which it has!.

Thankyou Sakari!

--
Regards

Kieran


> I think the check here could simply be removed as the check is done for
> every entity in the pipeline with the above patch.
> 
> If there's still a wish to check for the pipe field which should not be
> written by drivers, it should be done during pipeline traversal so that it
> applies to all entities in the pipeline, not just where traversal starts.
> 
>>
>> Signed-off-by: Kieran Bingham 
>> ---
>>
>> I've marked this patch as RFC, although if deemed suitable, by all means
>> integrate it as is.
>>
>> When testing suspend/resume operations on VSP1, I encountered a segfault on 
>> the
>> WARN_ON(!pipe->streaming_count) line, where 'pipe == NULL'. The simple
>> protection fix is to return early in this instance, as this patch does 
>> however:
>>
>> A) Does this early return path warrant a WARN() statement itself, to identify
>> drivers which are incorrectly calling media_entity_pipeline_stop() with an
>> invalid entity, or would this just be noise ...
>>
>> and therefore..
>>
>> B) I also partly assume this patch could simply get NAK'd with a request to 
>> go
>> and 

Re: [PATCHv2] iio: adc: Add Maxim MAX11100 driver

[jac...@jmondi.org]

Hello Peter,
   thanks for review

On 13/12/2016 21:33, Peter Meerwald-Stadler wrote:



Add IIO driver for Maxim MAX11100 single-channel ADC.
Add DT bindings documentation.


some more comments


Signed-off-by: Jacopo Mondi 
---

v1 -> v2:
- incorporated pmeerw's review comments
- retrieve vref from dts and use that to convert read_raw result
  to mV
- add device tree bindings documentation

---
 .../devicetree/bindings/iio/adc/max11100.txt   |  17 +++
 drivers/iio/adc/Kconfig|   9 ++
 drivers/iio/adc/Makefile   |   1 +
 drivers/iio/adc/max11100.c | 166 +
 4 files changed, 193 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/max11100.txt
 create mode 100644 drivers/iio/adc/max11100.c

diff --git a/Documentation/devicetree/bindings/iio/adc/max11100.txt 
b/Documentation/devicetree/bindings/iio/adc/max11100.txt
new file mode 100644
index 000..6877c11
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/max11100.txt
@@ -0,0 +1,17 @@
+* Maxim max11100 Analog to Digital Converter (ADC)
+
+Required properties:
+  - compatible: Should be "maxim,max11100"
+  - vref-supply: phandle to the regulator that provides reference voltage
+
+Optional properties:
+  - spi-max-frequency: SPI maximum frequency
+
+Example:
+
+adc0: max11100@0 {
+compatible = "maxim,max11100";
+vref-supply = <_vref>;
+spi-max-frequency = <24>;
+};
+
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 99c0514..a909484 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -285,6 +285,15 @@ config MAX1027
  To compile this driver as a module, choose M here: the module will be
  called max1027.

+config MAX11100
+   tristate "Maxim max11100 ADC driver"
+   depends on SPI


SPI_MASTER is more precise I think


+   help
+ Say yes here to build support for Maxim max11100 SPI ADC
+
+ To compile this driver as a module, choose M here: the module will be
+ called max11100.
+
 config MAX1363
tristate "Maxim max1363 ADC driver"
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 7a40c04..1463044 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
 obj-$(CONFIG_LTC2485) += ltc2485.o
 obj-$(CONFIG_MAX1027) += max1027.o
+obj-$(CONFIG_MAX11100) += max11100.o
 obj-$(CONFIG_MAX1363) += max1363.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
 obj-$(CONFIG_MCP3422) += mcp3422.o
diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c
new file mode 100644
index 000..f372ad8
--- /dev/null
+++ b/drivers/iio/adc/max11100.c
@@ -0,0 +1,166 @@
+/*
+ * iio/adc/max11100.c
+ * Maxim max11100 ADC Driver with IIO interface
+ *
+ * Copyright (C) 2016 Renesas Electronics Corporation
+ * Copyright (C) 2016 Jacopo Mondi
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+/*
+ * LSB is the ADC single digital step
+ * 1 LSB = (vref / 2 ^ 16)
+ * AIN = (DIN * LSB)
+ */
+#define MAX11100_LSB_DIV   (1 << 16)
+#define MAX11100_LSB(vref) (vref / MAX11100_LSB_DIV)


maybe parenthesis around vref


+
+struct max11100_state {
+   const struct max11100_chip_desc *desc;
+   struct spi_device *spi;
+   int vref_uv;
+   struct mutex lock;
+};
+
+static struct iio_chan_spec max11100_channels[] = {
+   { /* [0] */
+   .type = IIO_VOLTAGE,
+   .scan_type = {


scan_type not needed since driver does not support buffered reads


+   .sign = 'u',
+   .realbits = 16,
+   .storagebits = 24,
+   .shift = 8,
+   .repeat = 1,
+   .endianness = IIO_BE,
+   },
+   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+   },
+};
+
+static struct max11100_chip_desc {
+   unsigned int num_chan;
+   const struct iio_chan_spec *channels;
+} max11100_desc = {
+   .num_chan = ARRAY_SIZE(max11100_channels),
+   .channels = max11100_channels,
+};
+
+static int max11100_read_raw(struct iio_dev *indio_dev,
+struct iio_chan_spec const *chan,
+int *val, int *val2, long mask)
+{
+   int ret;
+   struct max11100_state *state = iio_priv(indio_dev);
+   uint8_t buffer[3];
+
+   mutex_lock(>lock);
+
+   ret = spi_read(state->spi, buffer, sizeof(buffer));
+   if (ret) {
+   mutex_unlock(>lock);
+   

RE: clk: clk-mstp has never worked for RZ/A1

[Chris Brandt]

Hi Geert,

On 12/14/206, Geert Uytterhoeven wrote:
> Oops... Where did your (offlist) bug report for rspi needing a delay after
> clock enable come from?

Because the RSPI driver in our current BSP is almost exactly the same so I was 
making the assumption that it would be a problem (but I have not tested it yet).


> I was aware the registers are documented to be 8-bit wide, but I always
> assumed 32-bit accesses do work.

At first, I did too.


> Another option is to let the driver bind against "renesas,r7s72100-mstp-
> clocks", and switch to 8-bit access mode.
> 
> CLK_OF_DECLARE(cpg_mstp_clks, "renesas,r7s72100-mstp-clocks",
> cpg_mstp_clocks_init8);
> 
> cpg_mstp_clocks_init8() can set a gloal flag, and call
> cpg_mstp_clocks_init().
> 
> The latter means no DT update is needed, and thus preserves compatibility
> with old DTBs.

I like this idea. I'll code it up and give it a shot.

Thank you.

Chris

Re: clk: clk-mstp has never worked for RZ/A1

[Geert Uytterhoeven]

Hi Chris,

CC devicetree

On Wed, Dec 14, 2016 at 3:14 AM, Chris Brandt  wrote:
> Today I realized that the clock control using clk-mstp has never actually 
> worked for RZ/A1 (r7s72100).

Oops... Where did your (offlist) bug report for rspi needing a delay after
clock enable come from?

> The reason is that the MSTP registers are 8-bit instead of the normal 32-bit, 
> and if you do a 32-bit write to them, nothing happens.
> If you look in drivers/clk/renesas/clk-mstp.c, you'll see that clk_readl and 
> clk_writel are used.

I was aware the registers are documented to be 8-bit wide, but I always
assumed 32-bit accesses do work.

> The reason why I have gotten this far is because u-boot has been enabling 
> everything early in boot, so all the clocks were already on.
> Of course if I disable everything again before I booting the 
> kernelnothing works.

Oops, I only have MSTP early disable debug code for boards I do have
physical access to (cfr. my topic/renesas-debug branch)...

> If I change the accesses in clk-mspt.c to 8-bit, clocks get enabled/disabled 
> as they should.
> #define clk_readl readb
> #define clk_writel writeb
>
>
> So, any suggestions on the best way to fix this???
> Ideally I would like something that can easily be integrated back into 4.9.x
>
>
>
> Maybe add a new DT property named "reg_width":
>
> mstp3_clks: mstp3_clks@fcfe0420 {
> #clock-cells = <1>;
> compatible = "renesas,r7s72100-mstp-clocks", 
> "renesas,cpg-mstp-clocks";
> reg = <0xfcfe0420 4>;
> reg_width = 8;
> clocks = <_clk>;
> clock-indices = ;
> clock-output-names = "mtu2";
> };
>
> and then in cpg_mstp_clocks_init(), do:
>
> if (of_find_property(np, "reg_width", ))
> group->reg_width = i;
> else
> group->reg_width = 32;  /* default */
>
>
> In cpg_mstp_clock_endisable(), do:
>
> if (group->reg_width == 8)
> writeb(value, group->smstpcr);
> else
> clk_writel(value, group->smstpcr);

That's one option. Note that the standard name for such property seems to be
"reg-io-width".

Another option is to let the driver bind against
"renesas,r7s72100-mstp-clocks", and switch to 8-bit access mode.

CLK_OF_DECLARE(cpg_mstp_clks, "renesas,r7s72100-mstp-clocks",
cpg_mstp_clocks_init8);

cpg_mstp_clocks_init8() can set a gloal flag, and call cpg_mstp_clocks_init().

The latter means no DT update is needed, and thus preserves compatibility
with old DTBs.

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

Re: [PATCH 2/2] irqchip/renesas-intc-irqpin: Add R-Car Gen1 fallback binding

[Simon Horman]

On Mon, Dec 12, 2016 at 12:43:11PM -0600, Rob Herring wrote:
> On Fri, Dec 09, 2016 at 01:52:20PM +0100, Geert Uytterhoeven wrote:
> > Hi Simon,
> > 
> > On Fri, Dec 9, 2016 at 11:50 AM, Simon Horman
> >  wrote:
> > > In the case of Renesas R-Car hardware we know that there are generations 
> > > of
> > > SoCs, e.g. Gen 1, Gen 2 and Gen 3. But beyond that its not clear what the
> > 
> > it's
> > 
> > > relationship between IP blocks might be. For example, I believe that
> > > r8a7779 is older than r8a7778 but that doesn't imply that the latter is a
> > > descendant of the former or vice versa.
> > >
> > > We can, however, by examining the documentation and behaviour of the
> > > hardware at run-time observe that the current driver implementation 
> > > appears
> > > to be compatible with the IP blocks on SoCs within a given generation.
> > >
> > > For the above reasons and convenience when enabling new SoCs a
> > > per-generation fallback compatibility string scheme being adopted for
> > > drivers for Renesas SoCs.
> > >
> > > Signed-off-by: Simon Horman 
> > > ---
> > >  .../interrupt-controller/renesas,intc-irqpin.txt   | 44 
> > > --
> > >  drivers/irqchip/irq-renesas-intc-irqpin.c  |  2 +
> > >  2 files changed, 26 insertions(+), 20 deletions(-)
> > >
> > > diff --git 
> > > a/Documentation/devicetree/bindings/interrupt-controller/renesas,intc-irqpin.txt
> > >  
> > > b/Documentation/devicetree/bindings/interrupt-controller/renesas,intc-irqpin.txt
> > > index 772c550d3b4b..e5a5251be9f5 100644
> > > --- 
> > > a/Documentation/devicetree/bindings/interrupt-controller/renesas,intc-irqpin.txt
> > > +++ 
> > > b/Documentation/devicetree/bindings/interrupt-controller/renesas,intc-irqpin.txt
> > > @@ -2,13 +2,19 @@ DT bindings for the R-/SH-Mobile irqpin controller
> > >
> > >  Required properties:
> > >
> > > -- compatible: has to be "renesas,intc-irqpin-", 
> > > "renesas,intc-irqpin"
> > > -  as fallback.
> > > -  Examples with soctypes are:
> > > +- compatible:
> > >  - "renesas,intc-irqpin-r8a7740" (R-Mobile A1)
> > >  - "renesas,intc-irqpin-r8a7778" (R-Car M1A)
> > >  - "renesas,intc-irqpin-r8a7779" (R-Car H1)
> > >  - "renesas,intc-irqpin-sh73a0" (SH-Mobile AG5)
> > > +- "renesas,rcar-gen1-intc-irqpin" (generic R-Car Gen1 compatible 
> > > device)
> > 
> > Does it make sense to add a new family-specific compatible value to a driver
> > that's unlikely to receive more users in the future? More recent SoCs use
> > renesas,irqc.
> 
> If that's the case, then no. Please don't go crazy with your generic 
> strings. I don't mind them, but I don't know that I'd call it best 
> practice.

Understood. I do not see any new users of this driver on the horizon
and given the above I withdraw this patch.