Re: [PATCH v7 0/8] vsp1: TLB optimisation and DL caching

[Laurent Pinchart]

Hi Kieran,

I've finished reviewing the series. For your convenience, I've rebased it on 
top of the BRU/BRS dynamic allocation patches, and pushed the result to

git://linuxtv.org/pinchartl/media.git v4l2/vsp1/tlb-optimise

(Please note it has been compile-tested only)

I have also taken the liberty to incorporate both my review comments and my 
Reviewed-by line for the patches that have received a conditional Reviewed-by, 
that is patches 1/8, 5/8 and 7/8. Patches 3/8, 4/8, 6/8 and 8/8 have open 
questions so I haven't touched them.

Please don't despair, v8 might well be the last version we will need :-)

On Thursday, 8 March 2018 02:05:23 EEST Kieran Bingham wrote:
> Each display list currently allocates an area of DMA memory to store
> register settings for the VSP1 to process. Each of these allocations adds
> pressure to the IPMMU TLB entries.
> 
> We can reduce the pressure by pre-allocating larger areas and dividing the
> area across multiple bodies represented as a pool.
> 
> With this reconfiguration of bodies, we can adapt the configuration code to
> separate out constant hardware configuration and cache it for re-use.
> 
> The patches provided in this series can be found at:
>   git://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git 
> tags/vsp1/tlb-optimise/v7
> 
> I hope that this series is at a stage where it could be integrated now.  It
> has had some thorough testing and is already integrated in both
> renesas-drivers and renesas-bsp. (except for the minor changes in v7 that
> is...)
> 
> Please note that checkpatch complains on patch 6/8 in this series:
> 
> v7-0006-media-vsp1-Refactor-display-list-configure-operations.patch
> 
> -- WARNING: function definition argument 'struct
> vsp1_entity *' should also have an identifier name #290: FILE:
> drivers/media/platform/vsp1/vsp1_entity.h:82:
> +   void (*configure_stream)(struct vsp1_entity *, struct vsp1_pipeline
> *,
> 
> However - this complaint is regarding pre-existing code. I have only renamed
> the function pointers.  I do also disagree with checkpatch here - as there
> is no need to provide an identifier name, and it does not improve
> readability in this instance to state:
>   ...(vsp1_entity *entity, struct vsp1_pipeline *pipe)
> 
> Thus - I have ignored these warnings.
> 
> 
> Changelog:
> --
> 
> v7:
>  - Rebased on to linux-media/master (v4.16-rc4)
>  - Clean up the formatting of the vsp1_dl_list_add_body()
>  - Fix formatting and white space
>  -  s/prepare/configure_stream/
>  -  s/configure/configure_frame/
> 
> v6:
>  - Rebased on to linux-media/master (v4.16-rc1)
>  - Removed DRM/UIF (DISCOM/ColorKey) updates
> 
> v5:
>  - Rebased on to renesas-drivers-2018-01-09-v4.15-rc7 to fix conflicts
>with DRM and UIF updates on VSP1 driver
> 
> v4:
>  - Rebased to v4.14
>  * v4l: vsp1: Use reference counting for bodies
>- Fix up reference handling comments
> 
>  * v4l: vsp1: Provide a body pool
>- Provide comment explaining extra allocation on body pool
>  highlighting area for optimisation later.
> 
>  * v4l: vsp1: Refactor display list configure operations
>- Fix up comment to describe yuv_mode caching rather than format
> 
>  * vsp1: Adapt entities to configure into a body
>- Rename vsp1_dl_list_get_body() to vsp1_dl_list_get_body0()
> 
>  * v4l: vsp1: Move video configuration to a cached dlb
>- Adjust pipe configured flag to be reset on resume rather than suspend
>- rename dl_child, dl_next
> 
> Testing:
> 
> The VSP unit tests have been run on this patch set with the following
> results:
> 
> --- Test loop 1 ---
> - vsp-unit-test-.sh
> Test Conditions:
>   Platform  Renesas Salvator-X 2nd version board based on r8a7795
> ES2.0+ Kernel release4.16.0-rc4-arm64-renesas-01067-g397eb3811ec0
>   convert   /usr/bin/convert
>   compare   /usr/bin/compare
>   killall   /usr/bin/killall
>   raw2rgbpnm/usr/bin/raw2rgbpnm
>   stress/usr/bin/stress
>   yavta /usr/bin/yavta
> - vsp-unit-test-0001.sh
> Testing WPF packing in RGB332: pass
> Testing WPF packing in ARGB555: pass
> Testing WPF packing in XRGB555: pass
> Testing WPF packing in RGB565: pass
> Testing WPF packing in BGR24: pass
> Testing WPF packing in RGB24: pass
> Testing WPF packing in ABGR32: pass
> Testing WPF packing in ARGB32: pass
> Testing WPF packing in XBGR32: pass
> Testing WPF packing in XRGB32: pass
> - vsp-unit-test-0002.sh
> Testing WPF packing in NV12M: pass
> Testing WPF packing in NV16M: pass
> Testing WPF packing in NV21M: pass
> Testing WPF packing in NV61M: pass
> Testing WPF packing in UYVY: pass
> Testing WPF packing in VYUY: skip
> Testing WPF packing in YUV420M: pass
> Testing WPF packing in YUV422M: pass
> Testing WPF packing in YUV444M: pass
> Testing WPF packing in YVU420M: pass
> Testing WPF packing in YVU422M: 

Re: [PATCH v7 8/8] media: vsp1: Move video configuration to a cached dlb

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 8 March 2018 02:05:31 EEST Kieran Bingham wrote:
> We are now able to configure a pipeline directly into a local display
> list body. Take advantage of this fact, and create a cacheable body to
> store the configuration of the pipeline in the video object.
> 
> vsp1_video_pipeline_run() is now the last user of the pipe->dl object.
> Convert this function to use the cached video->config body and obtain a
> local display list reference.
> 
> Attach the video->config body to the display list when needed before
> committing to hardware.
> 
> The pipe object is marked as un-configured when resuming from a suspend.
> This ensures that when the hardware is reset - our cached configuration
> will be re-attached to the next committed DL.
> 
> Signed-off-by: Kieran Bingham 
> ---
> 
> v3:
>  - 's/fragment/body/', 's/fragments/bodies/'
>  - video dlb cache allocation increased from 2 to 3 dlbs
> 
> Our video DL usage now looks like the below output:
> 
> dl->body0 contains our disposable runtime configuration. Max 41.
> dl_child->body0 is our partition specific configuration. Max 12.
> dl->bodies shows our constant configuration and LUTs.
> 
>   These two are LUT/CLU:
>  * dl->bodies[x]->num_entries 256 / max 256
>  * dl->bodies[x]->num_entries 4914 / max 4914
> 
> Which shows that our 'constant' configuration cache is currently
> utilised to a maximum of 64 entries.
> 
> trace-cmd report | \
> grep max | sed 's/.*vsp1_dl_list_commit://g' | sort | uniq;
> 
>   dl->body0->num_entries 13 / max 128
>   dl->body0->num_entries 14 / max 128
>   dl->body0->num_entries 16 / max 128
>   dl->body0->num_entries 20 / max 128
>   dl->body0->num_entries 27 / max 128
>   dl->body0->num_entries 34 / max 128
>   dl->body0->num_entries 41 / max 128
>   dl_child->body0->num_entries 10 / max 128
>   dl_child->body0->num_entries 12 / max 128
>   dl->bodies[x]->num_entries 15 / max 128
>   dl->bodies[x]->num_entries 16 / max 128
>   dl->bodies[x]->num_entries 17 / max 128
>   dl->bodies[x]->num_entries 18 / max 128
>   dl->bodies[x]->num_entries 20 / max 128
>   dl->bodies[x]->num_entries 21 / max 128
>   dl->bodies[x]->num_entries 256 / max 256
>   dl->bodies[x]->num_entries 31 / max 128
>   dl->bodies[x]->num_entries 32 / max 128
>   dl->bodies[x]->num_entries 39 / max 128
>   dl->bodies[x]->num_entries 40 / max 128
>   dl->bodies[x]->num_entries 47 / max 128
>   dl->bodies[x]->num_entries 48 / max 128
>   dl->bodies[x]->num_entries 4914 / max 4914
>   dl->bodies[x]->num_entries 55 / max 128
>   dl->bodies[x]->num_entries 56 / max 128
>   dl->bodies[x]->num_entries 63 / max 128
>   dl->bodies[x]->num_entries 64 / max 128

This might be useful to capture in the main part of the commit message.

> v4:
>  - Adjust pipe configured flag to be reset on resume rather than suspend
>  - rename dl_child, dl_next
> 
>  drivers/media/platform/vsp1/vsp1_pipe.c  |  7 +++-
>  drivers/media/platform/vsp1/vsp1_pipe.h  |  4 +-
>  drivers/media/platform/vsp1/vsp1_video.c | 67 -
>  drivers/media/platform/vsp1/vsp1_video.h |  2 +-
>  4 files changed, 54 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c
> b/drivers/media/platform/vsp1/vsp1_pipe.c index 5012643583b6..fa445b1a2e38
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.c
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.c
> @@ -249,6 +249,7 @@ void vsp1_pipeline_run(struct vsp1_pipeline *pipe)
>   vsp1_write(vsp1, VI6_CMD(pipe->output->entity.index),
>  VI6_CMD_STRCMD);
>   pipe->state = VSP1_PIPELINE_RUNNING;
> + pipe->configured = true;
>   }
> 
>   pipe->buffers_ready = 0;
> @@ -470,6 +471,12 @@ void vsp1_pipelines_resume(struct vsp1_device *vsp1)
>   continue;
> 
>   spin_lock_irqsave(>irqlock, flags);
> + /*
> +  * The hardware may have been reset during a suspend and will
> +  * need a full reconfiguration
> +  */

s/reconfiguration/reconfiguration./

> + pipe->configured = false;
> +

Where does that full reconfiguration occur, given that the vsp1_pipeline_run() 
right below sets pipe->configured to true without performing reconfiguration ?

>   if (vsp1_pipeline_ready(pipe))
>   vsp1_pipeline_run(pipe);
>   spin_unlock_irqrestore(>irqlock, flags);
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
> b/drivers/media/platform/vsp1/vsp1_pipe.h index 90d29492b9b9..e7ad6211b4d0
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> @@ -90,6 +90,7 @@ struct vsp1_partition {
>   * @irqlock: protects the pipeline state
>   * @state: current state
>   * @wq: wait queue to wait for state change completion
> + * @configured: flag determining if the hardware has run since reset
>   

Re: [PATCH v7 7/8] media: vsp1: Adapt entities to configure into a body

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 8 March 2018 02:05:30 EEST Kieran Bingham wrote:
> Currently the entities store their configurations into a display list.
> Adapt this such that the code can be configured into a body directly,
> allowing greater flexibility and control of the content.
> 
> All users of vsp1_dl_list_write() are removed in this process, thus it
> too is removed.
> 
> A helper, vsp1_dl_list_get_body0() is provided to access the internal body0
> from the display list.
> 
> Signed-off-by: Kieran Bingham 
> 
> ---
> v7
>  - Rebase
>  - s/prepare/configure_stream/
>  - s/configure/configure_frame/
> 
>  drivers/media/platform/vsp1/vsp1_bru.c| 22 ++---
>  drivers/media/platform/vsp1/vsp1_clu.c| 22 ++---
>  drivers/media/platform/vsp1/vsp1_dl.c | 12 ++-
>  drivers/media/platform/vsp1/vsp1_dl.h |  2 +-
>  drivers/media/platform/vsp1/vsp1_drm.c| 20 +++
>  drivers/media/platform/vsp1/vsp1_entity.c | 15 -
>  drivers/media/platform/vsp1/vsp1_entity.h | 11 +++---
>  drivers/media/platform/vsp1/vsp1_hgo.c| 16 -
>  drivers/media/platform/vsp1/vsp1_hgt.c| 18 +-
>  drivers/media/platform/vsp1/vsp1_hsit.c   | 10 +++---
>  drivers/media/platform/vsp1/vsp1_lif.c| 15 -
>  drivers/media/platform/vsp1/vsp1_lut.c| 21 ++--
>  drivers/media/platform/vsp1/vsp1_pipe.c   |  4 +-
>  drivers/media/platform/vsp1/vsp1_pipe.h   |  3 +-
>  drivers/media/platform/vsp1/vsp1_rpf.c| 39 +++---
>  drivers/media/platform/vsp1/vsp1_sru.c| 14 
>  drivers/media/platform/vsp1/vsp1_uds.c| 24 +++---
>  drivers/media/platform/vsp1/vsp1_uds.h|  2 +-
>  drivers/media/platform/vsp1/vsp1_video.c  | 11 --
>  drivers/media/platform/vsp1/vsp1_wpf.c| 42 
>  20 files changed, 172 insertions(+), 151 deletions(-)

[snip]

> diff --git a/drivers/media/platform/vsp1/vsp1_uds.c
> b/drivers/media/platform/vsp1/vsp1_uds.c index ce1731c2b3a9..6ddfce4bd095
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_uds.c
> +++ b/drivers/media/platform/vsp1/vsp1_uds.c
> @@ -31,22 +31,23 @@
>   * Device Access
>   */
> 
> -static inline void vsp1_uds_write(struct vsp1_uds *uds, struct vsp1_dl_list
> *dl,
> -   u32 reg, u32 data)
> +static inline void vsp1_uds_write(struct vsp1_uds *uds,
> +   struct vsp1_dl_body *dlb, u32 reg, u32 data)
>  {
> - vsp1_dl_list_write(dl, reg + uds->entity.index * VI6_UDS_OFFSET, data);
> + vsp1_dl_body_write(dlb, reg + uds->entity.index * VI6_UDS_OFFSET,
> +data);

This can hold on a single line.

>  }

[snip]

> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index 1b5a31734834..b47708660e53
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c

[snip]

> @@ -802,6 +804,9 @@ static int vsp1_video_setup_pipeline(struct
> vsp1_pipeline *pipe) if (!pipe->dl)
>   return -ENOMEM;
> 
> + /* Retrieve the default DLB from the list */

s/list/list./

> + dlb = vsp1_dl_list_get_body0(pipe->dl);
> +
>   if (pipe->uds) {
>   struct vsp1_uds *uds = to_uds(>uds->subdev);
> 
> @@ -824,8 +829,8 @@ static int vsp1_video_setup_pipeline(struct
> vsp1_pipeline *pipe) }
> 
>   list_for_each_entry(entity, >entities, list_pipe) {
> - vsp1_entity_route_setup(entity, pipe, pipe->dl);
> - vsp1_entity_configure_stream(entity, pipe, pipe->dl);
> + vsp1_entity_route_setup(entity, pipe, dlb);
> + vsp1_entity_configure_stream(entity, pipe, dlb);
>   }
> 
>   return 0;
> diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c
> b/drivers/media/platform/vsp1/vsp1_wpf.c index 6a6cdf0fb5f1..68218625549e
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_wpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
> @@ -31,9 +31,10 @@
>   */
> 
>  static inline void vsp1_wpf_write(struct vsp1_rwpf *wpf,
> -   struct vsp1_dl_list *dl, u32 reg, u32 data)
> +   struct vsp1_dl_body *dlb, u32 reg, u32 data)
>  {
> - vsp1_dl_list_write(dl, reg + wpf->entity.index * VI6_WPF_OFFSET, data);
> + vsp1_dl_body_write(dlb, reg + wpf->entity.index * VI6_WPF_OFFSET,
> +data);

This can hold on a single line.

>  }

[snip]

> @@ -292,10 +293,10 @@ static void wpf_configure_stream(struct vsp1_entity
> *entity,
> 
>   wpf->outfmt = outfmt;
> 
> - vsp1_dl_list_write(dl, VI6_DPR_WPF_FPORCH(wpf->entity.index),
> -VI6_DPR_WPF_FPORCH_FP_WPFN);
> + vsp1_dl_body_write(dlb, VI6_DPR_WPF_FPORCH(wpf->entity.index),
> +VI6_DPR_WPF_FPORCH_FP_WPFN);

Strange indentation.

> 
> - vsp1_dl_list_write(dl, VI6_WPF_WRBCK_CTRL, 0);
> + 

Re: [PATCH v7 6/8] media: vsp1: Refactor display list configure operations

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 8 March 2018 02:05:29 EEST Kieran Bingham wrote:
> The entities provide a single .configure operation which configures the
> object into the target display list, based on the vsp1_entity_params
> selection.
> 
> This restricts us to a single function prototype for both static
> configuration (the pre-stream INIT stage) and the dynamic runtime stages
> for both each frame - and each partition therein.
> 
> Split the configure function into two parts, '.configure_stream()' and
> '.configure_frame()', merging both the VSP1_ENTITY_PARAMS_RUNTIME and
> VSP1_ENTITY_PARAMS_PARTITION stages into a single call through the
> .configure_frame(). The configuration for individual partitions is
> handled by passing the partition number to the configure call, and
> processing any runtime stage actions on the first partition only.
> 
> Signed-off-by: Kieran Bingham 
> 
> ---
> v7
>  - Fix formatting and white space
>  - s/prepare/configure_stream/
>  - s/configure/configure_frame/
> 
>  drivers/media/platform/vsp1/vsp1_bru.c|  12 +-
>  drivers/media/platform/vsp1/vsp1_clu.c|  50 +---
>  drivers/media/platform/vsp1/vsp1_dl.h |   1 +-
>  drivers/media/platform/vsp1/vsp1_drm.c|  21 +--
>  drivers/media/platform/vsp1/vsp1_entity.c |  17 +-
>  drivers/media/platform/vsp1/vsp1_entity.h |  33 +--
>  drivers/media/platform/vsp1/vsp1_hgo.c|  12 +-
>  drivers/media/platform/vsp1/vsp1_hgt.c|  12 +-
>  drivers/media/platform/vsp1/vsp1_hsit.c   |  12 +-
>  drivers/media/platform/vsp1/vsp1_lif.c|  12 +-
>  drivers/media/platform/vsp1/vsp1_lut.c|  32 +-
>  drivers/media/platform/vsp1/vsp1_rpf.c| 164 ++---
>  drivers/media/platform/vsp1/vsp1_sru.c|  12 +-
>  drivers/media/platform/vsp1/vsp1_uds.c|  57 ++--
>  drivers/media/platform/vsp1/vsp1_video.c  |  24 +--
>  drivers/media/platform/vsp1/vsp1_wpf.c| 299 ---
>  16 files changed, 378 insertions(+), 392 deletions(-)

[snip]

> diff --git a/drivers/media/platform/vsp1/vsp1_clu.c
> b/drivers/media/platform/vsp1/vsp1_clu.c index b2a39a6ef7e4..b8d8af6d4910
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_clu.c
> +++ b/drivers/media/platform/vsp1/vsp1_clu.c
> @@ -213,37 +213,36 @@ static const struct v4l2_subdev_ops clu_ops = {
>  /*
> ---
> -- * VSP1 Entity Operations
>   */
> +static void clu_configure_stream(struct vsp1_entity *entity,
> +  struct vsp1_pipeline *pipe,
> +  struct vsp1_dl_list *dl)
> +{
> + struct vsp1_clu *clu = to_clu(>subdev);
> +
> + /*
> +  * The yuv_mode can't be changed during streaming. Cache it internally
> +  * for future runtime configuration calls.
> +  */

I'd move this comment right before the vsp1_entity_get_pad_format() call to 
keep all variable declarations together.

> + struct v4l2_mbus_framefmt *format;
> +
> + format = vsp1_entity_get_pad_format(>entity,
> + clu->entity.config,
> + CLU_PAD_SINK);
> + clu->yuv_mode = format->code == MEDIA_BUS_FMT_AYUV8_1X32;
> +}

[snip]


> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h
> b/drivers/media/platform/vsp1/vsp1_dl.h index 7e820ac6865a..f45083251644
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> @@ -41,7 +41,6 @@ vsp1_dl_body_pool_create(struct vsp1_device *vsp1,
> unsigned int num_bodies, void vsp1_dl_body_pool_destroy(struct
> vsp1_dl_body_pool *pool);
>  struct vsp1_dl_body *vsp1_dl_body_get(struct vsp1_dl_body_pool *pool);
>  void vsp1_dl_body_put(struct vsp1_dl_body *dlb);
> -

This is an unrelated change.

>  void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32 reg, u32 data);
>  int vsp1_dl_list_add_body(struct vsp1_dl_list *dl, struct vsp1_dl_body
> *dlb);
>  int vsp1_dl_list_add_chain(struct vsp1_dl_list *head, struct vsp1_dl_list
>  *dl);

[snip]

> diff --git a/drivers/media/platform/vsp1/vsp1_entity.h
> b/drivers/media/platform/vsp1/vsp1_entity.h index
> 408602ebeb97..b44ed5414fc3 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.h
> +++ b/drivers/media/platform/vsp1/vsp1_entity.h

[snip]

> @@ -80,8 +68,10 @@ struct vsp1_route {
>  /**
>   * struct vsp1_entity_operations - Entity operations
>   * @destroy: Destroy the entity.
> - * @configure:   Setup the hardware based on the entity state (pipeline,
> formats,
> - *   selection rectangles, ...)
> + * @configure_stream:Setup the initial hardware parameters for the 
stream
> + *   (pipeline, formats)

Instead of initial I would say "Setup hardware parameters that stay constant 
for the whole stream (pipeline, formats)", or possible "that don't vary 
between frames" instead.

> + * @configure_frame: Configure the runtime parameters for each 

Re: [PATCH v7 5/8] media: vsp1: Use reference counting for bodies

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 8 March 2018 02:05:28 EEST Kieran Bingham wrote:
> Extend the display list body with a reference count, allowing bodies to
> be kept as long as a reference is maintained. This provides the ability
> to keep a cached copy of bodies which will not change, so that they can
> be re-applied to multiple display lists.
> 
> Signed-off-by: Kieran Bingham 
> 
> ---
> This could be squashed into the body update code, but it's not a
> straightforward squash as the refcounts will affect both:
>   v4l: vsp1: Provide a body pool
> and
>   v4l: vsp1: Convert display lists to use new body pool
> therefore, I have kept this separate to prevent breaking bisectability
> of the vsp-tests.
> 
> v3:
>  - 's/fragment/body/'
> 
> v4:
>  - Fix up reference handling comments.
> 
>  drivers/media/platform/vsp1/vsp1_clu.c |  7 ++-
>  drivers/media/platform/vsp1/vsp1_dl.c  | 15 ++-
>  drivers/media/platform/vsp1/vsp1_lut.c |  7 ++-
>  3 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_clu.c
> b/drivers/media/platform/vsp1/vsp1_clu.c index 2018144470c5..b2a39a6ef7e4
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_clu.c
> +++ b/drivers/media/platform/vsp1/vsp1_clu.c
> @@ -257,8 +257,13 @@ static void clu_configure(struct vsp1_entity *entity,
>   clu->clu = NULL;
>   spin_unlock_irqrestore(>lock, flags);
> 
> - if (dlb)
> + if (dlb) {
>   vsp1_dl_list_add_body(dl, dlb);
> +
> + /* release our local reference */

s/release/Release/
s/reference/reference./

> + vsp1_dl_body_put(dlb);
> + }
> +
>   break;
>   }
>  }
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> b/drivers/media/platform/vsp1/vsp1_dl.c index 74476726451c..134865287c02
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -14,6 +14,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
> 
> @@ -58,6 +59,8 @@ struct vsp1_dl_body {
>   struct list_head list;
>   struct list_head free;
> 
> + refcount_t refcnt;
> +
>   struct vsp1_dl_body_pool *pool;
>   struct vsp1_device *vsp1;
> 
> @@ -259,6 +262,7 @@ struct vsp1_dl_body *vsp1_dl_body_get(struct
> vsp1_dl_body_pool *pool)
>   if (!list_empty(>free)) {
>   dlb = list_first_entry(>free, struct vsp1_dl_body, free);
>   list_del(>free);
> + refcount_set(>refcnt, 1);
>   }
> 
>   spin_unlock_irqrestore(>lock, flags);
> @@ -279,6 +283,9 @@ void vsp1_dl_body_put(struct vsp1_dl_body *dlb)
>   if (!dlb)
>   return;
> 
> + if (!refcount_dec_and_test(>refcnt))
> + return;
> +
>   dlb->num_entries = 0;
> 
>   spin_lock_irqsave(>pool->lock, flags);
> @@ -465,7 +472,11 @@ void vsp1_dl_list_write(struct vsp1_dl_list *dl, u32
> reg, u32 data) * in the order in which bodies are added.
>   *
>   * Adding a body to a display list passes ownership of the body to the
> list. The
> - * caller must not touch the body after this call.
> + * caller retains its reference to the fragment when adding it to the
> display
> + * list, but is not allowed to add new entries to the body.
> + *
> + * The reference must be explicitly released by a call to
> vsp1_dl_body_put()
> + * when the body isn't needed anymore.
>   *
>   * Additional bodies are only usable for display lists in header mode.
>   * Attempting to add a body to a header-less display list will return an
> error. @@ -476,6 +487,8 @@ int vsp1_dl_list_add_body(struct vsp1_dl_list
> *dl, struct vsp1_dl_body *dlb)
>   if (dl->dlm->mode != VSP1_DL_MODE_HEADER)
>   return -EINVAL;
> 
> + refcount_inc(>refcnt);
> +
>   list_add_tail(>list, >bodies);
> 
>   return 0;
> diff --git a/drivers/media/platform/vsp1/vsp1_lut.c
> b/drivers/media/platform/vsp1/vsp1_lut.c index 262cb72139d6..77cf7137a0f2
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_lut.c
> +++ b/drivers/media/platform/vsp1/vsp1_lut.c
> @@ -213,8 +213,13 @@ static void lut_configure(struct vsp1_entity *entity,
>   lut->lut = NULL;
>   spin_unlock_irqrestore(>lock, flags);
> 
> - if (dlb)
> + if (dlb) {
>   vsp1_dl_list_add_body(dl, dlb);
> +
> + /* release our local reference */

s/release/Release/
s/reference/reference./

With these small issues fixed,

Reviewed-by: Laurent Pinchart 

> + vsp1_dl_body_put(dlb);
> + }
> +
>   break;
>   }
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 4/8] media: vsp1: Convert display lists to use new body pool

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 8 March 2018 02:05:27 EEST Kieran Bingham wrote:
> Adapt the dl->body0 object to use an object from the body pool. This
> greatly reduces the pressure on the TLB for IPMMU use cases, as all of
> the lists use a single allocation for the main body.
> 
> The CLU and LUT objects pre-allocate a pool containing three bodies,
> allowing a userspace update before the hardware has committed a previous
> set of tables.
> 
> Bodies are no longer 'freed' in interrupt context, but instead released
> back to their respective pools. This allows us to remove the garbage
> collector in the DLM.
> 
> Signed-off-by: Kieran Bingham 
> 
> ---
> v3:
>  - 's/fragment/body', 's/fragments/bodies/'
>  - CLU/LUT now allocate 3 bodies
>  - vsp1_dl_list_fragments_free -> vsp1_dl_list_bodies_put
> 
> v2:
>  - Use dl->body0->max_entries to determine header offset, instead of the
>global constant VSP1_DL_NUM_ENTRIES which is incorrect.
>  - squash updates for LUT, CLU, and fragment cleanup into single patch.
>(Not fully bisectable when separated)
> 
>  drivers/media/platform/vsp1/vsp1_clu.c |  27 ++-
>  drivers/media/platform/vsp1/vsp1_clu.h |   1 +-
>  drivers/media/platform/vsp1/vsp1_dl.c  | 223 ++
>  drivers/media/platform/vsp1/vsp1_dl.h  |   3 +-
>  drivers/media/platform/vsp1/vsp1_lut.c |  27 ++-
>  drivers/media/platform/vsp1/vsp1_lut.h |   1 +-
>  6 files changed, 101 insertions(+), 181 deletions(-)

Still a nice diffstart :-)

[snip]

> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> b/drivers/media/platform/vsp1/vsp1_dl.c index 0208e72cb356..74476726451c
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c

[snip]

> @@ -399,11 +311,10 @@ void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32
> reg, u32 data) * Display List Transaction Management
>   */
> 
> -static struct vsp1_dl_list *vsp1_dl_list_alloc(struct vsp1_dl_manager *dlm)
> +static struct vsp1_dl_list *vsp1_dl_list_alloc(struct vsp1_dl_manager
> *dlm,
> +struct vsp1_dl_body_pool *pool)

Given that the only caller of this function passes dlm->pool as the second 
argument, can't you remove the second argument ?

>  {
>   struct vsp1_dl_list *dl;
> - size_t header_size;
> - int ret;
> 
>   dl = kzalloc(sizeof(*dl), GFP_KERNEL);
>   if (!dl)
> @@ -412,41 +323,39 @@ static struct vsp1_dl_list *vsp1_dl_list_alloc(struct
> vsp1_dl_manager *dlm) INIT_LIST_HEAD(>bodies);
>   dl->dlm = dlm;
> 
> - /*
> -  * Initialize the display list body and allocate DMA memory for the body
> -  * and the optional header. Both are allocated together to avoid memory
> -  * fragmentation, with the header located right after the body in
> -  * memory.
> -  */
> - header_size = dlm->mode == VSP1_DL_MODE_HEADER
> - ? ALIGN(sizeof(struct vsp1_dl_header), 8)
> - : 0;
> -
> - ret = vsp1_dl_body_init(dlm->vsp1, >body0, VSP1_DL_NUM_ENTRIES,
> - header_size);
> - if (ret < 0) {
> - kfree(dl);
> + /* Retrieve a body from our DLM body pool */

s/body pool/body pool./

(And I would have said "Get a body" but that's up to you)

> + dl->body0 = vsp1_dl_body_get(pool);
> + if (!dl->body0)
>   return NULL;
> - }
> -
>   if (dlm->mode == VSP1_DL_MODE_HEADER) {
> - size_t header_offset = VSP1_DL_NUM_ENTRIES
> -  * sizeof(*dl->body0.entries);
> + size_t header_offset = dl->body0->max_entries
> +  * sizeof(*dl->body0->entries);
> 
> - dl->header = ((void *)dl->body0.entries) + header_offset;
> - dl->dma = dl->body0.dma + header_offset;
> + dl->header = ((void *)dl->body0->entries) + header_offset;
> + dl->dma = dl->body0->dma + header_offset;
> 
>   memset(dl->header, 0, sizeof(*dl->header));
> - dl->header->lists[0].addr = dl->body0.dma;
> + dl->header->lists[0].addr = dl->body0->dma;
>   }
> 
>   return dl;
>  }
> 
> +static void vsp1_dl_list_bodies_put(struct vsp1_dl_list *dl)
> +{
> + struct vsp1_dl_body *dlb, *tmp;
> +
> + list_for_each_entry_safe(dlb, tmp, >bodies, list) {
> + list_del(>list);
> + vsp1_dl_body_put(dlb);
> + }
> +}
> +
>  static void vsp1_dl_list_free(struct vsp1_dl_list *dl)
>  {
> - vsp1_dl_body_cleanup(>body0);
> - list_splice_init(>bodies, >dlm->gc_bodies);
> + vsp1_dl_body_put(dl->body0);
> + vsp1_dl_list_bodies_put(dl);

Too bad we can't keep the list splice, it's more efficient than iterating over 
the list, but I suppose it's unavoidable if we want to reset the number of 
used entries to 0 for each body. Beside, we should have a small number of 
bodies only, so hopefully it 

Re: [PATCH v7 3/8] media: vsp1: Provide a body pool

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 8 March 2018 02:05:26 EEST Kieran Bingham wrote:
> Each display list allocates a body to store register values in a dma
> accessible buffer from a dma_alloc_wc() allocation. Each of these
> results in an entry in the TLB, and a large number of display list

I'd write it as "IOMMU TLB" to make it clear we're not concerned about CPU MMU 
TLB pressure.

> allocations adds pressure to this resource.
> 
> Reduce TLB pressure on the IPMMUs by allocating multiple display list
> bodies in a single allocation, and providing these to the display list
> through a 'body pool'. A pool can be allocated by the display list
> manager or entities which require their own body allocations.
> 
> Signed-off-by: Kieran Bingham 
> 
> ---
> v4:
>  - Provide comment explaining extra allocation on body pool
>highlighting area for optimisation later.
> 
> v3:
>  - s/fragment/body/, s/fragments/bodies/
>  - qty -> num_bodies
>  - indentation fix
>  - s/vsp1_dl_body_pool_{alloc,free}/vsp1_dl_body_pool_{create,destroy}/'
>  - Add kerneldoc to non-static functions
> 
> v2:
>  - assign dlb->dma correctly
> 
>  drivers/media/platform/vsp1/vsp1_dl.c | 163 +++-
>  drivers/media/platform/vsp1/vsp1_dl.h |   8 +-
>  2 files changed, 171 insertions(+)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> b/drivers/media/platform/vsp1/vsp1_dl.c index 67cc16c1b8e3..0208e72cb356
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -45,6 +45,8 @@ struct vsp1_dl_entry {
>  /**
>   * struct vsp1_dl_body - Display list body
>   * @list: entry in the display list list of bodies
> + * @free: entry in the pool free body list

Could we reuse @list for this purpose ? Unless I'm mistaken, when a body is in 
a pool it doesn't belong to any particular display list, and when it is in a 
display list it isn't in the pool anymore.

> + * @pool: pool to which this body belongs
>   * @vsp1: the VSP1 device
>   * @entries: array of entries
>   * @dma: DMA address of the entries
> @@ -54,6 +56,9 @@ struct vsp1_dl_entry {
>   */
>  struct vsp1_dl_body {
>   struct list_head list;
> + struct list_head free;
> +
> + struct vsp1_dl_body_pool *pool;
>   struct vsp1_device *vsp1;
> 
>   struct vsp1_dl_entry *entries;
> @@ -65,6 +70,30 @@ struct vsp1_dl_body {
>  };
> 
>  /**
> + * struct vsp1_dl_body_pool - display list body pool
> + * @dma: DMA address of the entries
> + * @size: size of the full DMA memory pool in bytes
> + * @mem: CPU memory pointer for the pool
> + * @bodies: Array of DLB structures for the pool
> + * @free: List of free DLB entries
> + * @lock: Protects the pool and free list

The pool and free list ? As far as I can tell the lock only protects the free 
list.

> + * @vsp1: the VSP1 device
> + */
> +struct vsp1_dl_body_pool {
> + /* DMA allocation */
> + dma_addr_t dma;
> + size_t size;
> + void *mem;
> +
> + /* Body management */
> + struct vsp1_dl_body *bodies;
> + struct list_head free;
> + spinlock_t lock;
> +
> + struct vsp1_device *vsp1;
> +};
> +
> +/**
>   * struct vsp1_dl_list - Display list
>   * @list: entry in the display list manager lists
>   * @dlm: the display list manager
> @@ -105,6 +134,7 @@ enum vsp1_dl_mode {
>   * @active: list currently being processed (loaded) by hardware
>   * @queued: list queued to the hardware (written to the DL registers)
>   * @pending: list waiting to be queued to the hardware
> + * @pool: body pool for the display list bodies
>   * @gc_work: bodies garbage collector work struct
>   * @gc_bodies: array of display list bodies waiting to be freed
>   */
> @@ -120,6 +150,8 @@ struct vsp1_dl_manager {
>   struct vsp1_dl_list *queued;
>   struct vsp1_dl_list *pending;
> 
> + struct vsp1_dl_body_pool *pool;
> +
>   struct work_struct gc_work;
>   struct list_head gc_bodies;
>  };
> @@ -128,6 +160,137 @@ struct vsp1_dl_manager {
>   * Display List Body Management
>   */
> 
> +/**
> + * vsp1_dl_body_pool_create - Create a pool of bodies from a single
> allocation
> + * @vsp1: The VSP1 device
> + * @num_bodies: The quantity of bodies to allocate

For consistency, s/quantity/number/

> + * @num_entries: The maximum number of entries that the body can contain

Maybe s/the body/a body/ ?

> + * @extra_size: Extra allocation provided for the bodies
> + *
> + * Allocate a pool of display list bodies each with enough memory to
> contain the
> + * requested number of entries.

How about

the requested number of entries plus the @extra_size.

> + *
> + * Return a pointer to a pool on success or NULL if memory can't be
> allocated.
> + */
> +struct vsp1_dl_body_pool *
> +vsp1_dl_body_pool_create(struct vsp1_device *vsp1, unsigned int num_bodies,
> +  unsigned int num_entries, size_t extra_size)
> +{
> + struct vsp1_dl_body_pool *pool;
> + 

Re: [PATCH v7 1/8] media: vsp1: Reword uses of 'fragment' as 'body'

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 8 March 2018 02:05:24 EEST Kieran Bingham wrote:
> Throughout the codebase, the term 'fragment' is used to represent a
> display list body. This term duplicates the 'body' which is already in
> use.
> 
> The datasheet references these objects as a body, therefore replace all
> mentions of a fragment with a body, along with the corresponding
> pluralised terms.

I like this, the code seems less confusing to me this way. Please see below 
for a few minor comments.

> Signed-off-by: Kieran Bingham 
> 
> ---
> v7
>  - Clean up the formatting of the vsp1_dl_list_add_body()
> 
>  drivers/media/platform/vsp1/vsp1_clu.c |  10 +-
>  drivers/media/platform/vsp1/vsp1_dl.c  | 109 --
>  drivers/media/platform/vsp1/vsp1_dl.h  |  13 +--
>  drivers/media/platform/vsp1/vsp1_lut.c |   8 +-
>  4 files changed, 69 insertions(+), 71 deletions(-)

[snip]

> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> b/drivers/media/platform/vsp1/vsp1_dl.c index 0b86ed01e85d..caed441f5f0c
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c

[snip]

> @@ -157,17 +157,16 @@ static void vsp1_dl_body_cleanup(struct
> vsp1_dl_body *dlb) }
> 
>  /**
> - * vsp1_dl_fragment_alloc - Allocate a display list fragment
> + * vsp1_dl_body_alloc - Allocate a display list body
>   * @vsp1: The VSP1 device
> - * @num_entries: The maximum number of entries that the fragment can
> contain
> + * @num_entries: The maximum number of entries that the body can contain
>   *
> - * Allocate a display list fragment with enough memory to contain the
> requested
> + * Allocate a display list body with enough memory to contain the requested
>   * number of entries.
>   *
> - * Return a pointer to a fragment on success or NULL if memory can't be
> - * allocated.
> + * Return a pointer to a body on success or NULL if memory can't be
> allocated.
>   */
> -struct vsp1_dl_body *vsp1_dl_fragment_alloc(struct vsp1_device *vsp1,
> +struct vsp1_dl_body *vsp1_dl_body_alloc(struct vsp1_device *vsp1,
>   unsigned int num_entries)

The indentation of the second line now looks wrong.

[snip]

> @@ -379,33 +378,33 @@ void vsp1_dl_list_put(struct vsp1_dl_list *dl)
>   */
>  void vsp1_dl_list_write(struct vsp1_dl_list *dl, u32 reg, u32 data)
>  {
> - vsp1_dl_fragment_write(>body0, reg, data);
> + vsp1_dl_body_write(>body0, reg, data);
>  }
> 
>  /**
> - * vsp1_dl_list_add_fragment - Add a fragment to the display list
> + * vsp1_dl_list_add_body - Add a body to the display list
>   * @dl: The display list
> - * @dlb: The fragment
> + * @dlb: The body
>   *
> - * Add a display list body as a fragment to a display list. Registers
> contained
> - * in fragments are processed after registers contained in the main display
> - * list, in the order in which fragments are added.
> + * Add a display list body as a body to a display list. Registers contained

"body as a body" sounds strange. How about just "Add a display list body to 
the display list." ?

> + * in bodies are processed after registers contained in the main display
> list,
> + * in the order in which bodies are added.
>   *
> - * Adding a fragment to a display list passes ownership of the fragment to
> the
> - * list. The caller must not touch the fragment after this call, and
> must not
> - * free it explicitly with vsp1_dl_fragment_free().
> + * Adding a body to a display list passes ownership of the body to the
> list. The
> + * caller must not touch the body after this call, and must not free it
> + * explicitly with vsp1_dl_body_free().
>   *
> - * Fragments are only usable for display lists in header mode. Attempt to
> - * add a fragment to a header-less display list will return an error.
> + * Additional bodies are only usable for display lists in header mode.
> + * Attempting to add a body to a header-less display list will return an
> error.
>   */

[snip]

With those two small issues fixed,

Reviewed-by: Laurent Pinchart 

-- 
Regards,

Laurent Pinchart

[PATCH 3/3] arm64: dts: renesas: condor: add PCIe support

[Sergei Shtylyov]

Enable PCIe PHY and PCIEC and specify the PCIe bus clock for the Condor
board.

Signed-off-by: Sergei Shtylyov 

---
 arch/arm64/boot/dts/renesas/r8a77980-condor.dts |   12 
 1 file changed, 12 insertions(+)

Index: renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
===
--- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
+++ renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
@@ -49,6 +49,18 @@
clock-frequency = <32768>;
 };
 
+ {
+   status = "okay";
+};
+
+_bus_clk {
+   clock-frequency = <1>;
+};
+
+_phy {
+   status = "okay";
+};
+
  {
status = "okay";
 };

[PATCH 2/3] arm64: dts: renesas: r8a77980: add PCIEC support

[Sergei Shtylyov]

Describe PCIEC and PCIe bus clock in the R8A77980 device tree.

Signed-off-by: Sergei Shtylyov 

---
 arch/arm64/boot/dts/renesas/r8a77980.dtsi |   39 ++
 1 file changed, 39 insertions(+)

Index: renesas/arch/arm64/boot/dts/renesas/r8a77980.dtsi
===
--- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980.dtsi
+++ renesas/arch/arm64/boot/dts/renesas/r8a77980.dtsi
@@ -51,6 +51,13 @@
clock-frequency = <0>;
};
 
+   /* External PCIe clock - can be overridden by the board */
+   pcie_bus_clk: pcie_bus {
+   compatible = "fixed-clock";
+   #clock-cells = <0>;
+   clock-frequency = <0>;
+   };
+
psci {
compatible = "arm,psci-1.0", "arm,psci-0.2";
method = "smc";
@@ -376,6 +383,38 @@
resets = < 408>;
};
 
+   pciec: pcie@fe00 {
+   compatible = "renesas,pcie-r8a77980",
+"renesas,pcie-rcar-gen3";
+   reg = <0 0xfe00 0 0x8>;
+   #address-cells = <3>;
+   #size-cells = <2>;
+   bus-range = <0x00 0xff>;
+   device_type = "pci";
+   ranges = <
+   0x0100 0 0x 0 0xfe10 0 0x010
+   0x0200 0 0xfe20 0 0xfe20 0 0x020
+   0x0200 0 0x3000 0 0x3000 0 0x800
+   0x4200 0 0x3800 0 0x3800 0 0x800
+   >;
+   dma-ranges = <0x4200 0 0x4000 0 0x4000
+ 0 0x8000>;
+   interrupts = ,
+,
+;
+   #interrupt-cells = <1>;
+   interrupt-map-mask = <0 0 0 0>;
+   interrupt-map = <0 0 0 0  GIC_SPI 148
+IRQ_TYPE_LEVEL_HIGH>;
+   clocks = < CPG_MOD 319>, <_bus_clk>;
+   clock-names = "pcie", "pcie_bus";
+   power-domains = < 32>;
+   resets = < 319>;
+   phys = <_phy>;
+   phy-names = "pcie";
+   status = "disabled";
+   };
+
prr: chipid@fff00044 {
compatible = "renesas,prr";
reg = <0 0xfff00044 0 4>;

[PATCH 1/3] arm64: dts: renesas: r8a77980: add PCIe PHY support

[Sergei Shtylyov]

Describe the PCIe PHY in the R8A77980 device tree; it will be used by PCIEC
in the next patch...

Signed-off-by: Sergei Shtylyov 

---
 arch/arm64/boot/dts/renesas/r8a77980.dtsi |   50 ++
 1 file changed, 50 insertions(+)

Index: renesas/arch/arm64/boot/dts/renesas/r8a77980.dtsi
===
--- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980.dtsi
+++ renesas/arch/arm64/boot/dts/renesas/r8a77980.dtsi
@@ -164,6 +171,17 @@
status = "disabled";
};
 
+   pcie_phy: pcie-phy@e65d {
+   compatible = "renesas,r8a77980-pcie-phy",
+"renesas,rcar-gen3-pcie-phy";
+   reg = <0 0xe65d 0 0x8000>;
+   #phy-cells = <0>;
+   clocks = < CPG_MOD 319>;
+   power-domains = < 32>;
+   resets = < 319>;
+   status = "disabled";
+   };
+
avb: ethernet@e680 {
compatible = "renesas,etheravb-r8a77980",
 "renesas,etheravb-rcar-gen3";

[PATCH 0/3] Add R8A77980/Condor PCIe support

[Sergei Shtylyov]

Hello!

Here's the set of 3 patches against Simon Horman's 'renesas.git' repo's
'renesas-devel-20180330-v4.16-rc7' tag. We're adding the R8A77980 PCIe related
device nodes and then enable PCIe on the Condor board. These patches depend on
the R8A77980 PCIe PHY and PCIEC driver support in order to work properly...

[1/3] arm64: dts: renesas: r8a77980: add PCIe PHY support
[2/3] arm64: dts: renesas: r8a77980: add PCIEC support
[3/3] arm64: dts: renesas: condor: add PCIe support

WBR, Sergei

[PATCH v2.1 09/15] v4l: vsp1: Move DRM pipeline output setup code to a function

[Laurent Pinchart]

In order to make the vsp1_du_setup_lif() easier to read, and for
symmetry with the DRM pipeline input setup, move the pipeline output
setup code to a separate function.

Signed-off-by: Laurent Pinchart 
Reviewed-by: Kieran Bingham 
--
Changes since v2:

- Moved vsp1_du_pipeline_setup_input() rename to earlier patch

Changes since v1:

- Rename vsp1_du_pipeline_setup_input() to
  vsp1_du_pipeline_setup_inputs()
- Initialize format local variable to 0 in
  vsp1_du_pipeline_setup_output()
---
 drivers/media/platform/vsp1/vsp1_drm.c | 106 +++--
 1 file changed, 60 insertions(+), 46 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index 4a628bbf7e47..a79b05ef 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -276,6 +276,65 @@ static int vsp1_du_pipeline_setup_inputs(struct 
vsp1_device *vsp1,
return 0;
 }
 
+/* Setup the output side of the pipeline (WPF and LIF). */
+static int vsp1_du_pipeline_setup_output(struct vsp1_device *vsp1,
+struct vsp1_pipeline *pipe)
+{
+   struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
+   struct v4l2_subdev_format format = { 0, };
+   int ret;
+
+   format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+   format.pad = RWPF_PAD_SINK;
+   format.format.width = drm_pipe->width;
+   format.format.height = drm_pipe->height;
+   format.format.code = MEDIA_BUS_FMT_ARGB_1X32;
+   format.format.field = V4L2_FIELD_NONE;
+
+   ret = v4l2_subdev_call(>output->entity.subdev, pad, set_fmt, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on WPF%u sink\n",
+   __func__, format.format.width, format.format.height,
+   format.format.code, pipe->output->entity.index);
+
+   format.pad = RWPF_PAD_SOURCE;
+   ret = v4l2_subdev_call(>output->entity.subdev, pad, get_fmt, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   dev_dbg(vsp1->dev, "%s: got format %ux%u (%x) on WPF%u source\n",
+   __func__, format.format.width, format.format.height,
+   format.format.code, pipe->output->entity.index);
+
+   format.pad = LIF_PAD_SINK;
+   ret = v4l2_subdev_call(>lif->subdev, pad, set_fmt, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on LIF%u sink\n",
+   __func__, format.format.width, format.format.height,
+   format.format.code, pipe->lif->index);
+
+   /*
+* Verify that the format at the output of the pipeline matches the
+* requested frame size and media bus code.
+*/
+   if (format.format.width != drm_pipe->width ||
+   format.format.height != drm_pipe->height ||
+   format.format.code != MEDIA_BUS_FMT_ARGB_1X32) {
+   dev_dbg(vsp1->dev, "%s: format mismatch on LIF%u\n", __func__,
+   pipe->lif->index);
+   return -EPIPE;
+   }
+
+   return 0;
+}
+
 /* Configure all entities in the pipeline. */
 static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
 {
@@ -356,7 +415,6 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int 
pipe_index,
struct vsp1_drm_pipeline *drm_pipe;
struct vsp1_pipeline *pipe;
struct vsp1_bru *bru;
-   struct v4l2_subdev_format format;
unsigned long flags;
unsigned int i;
int ret;
@@ -417,54 +475,10 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int 
pipe_index,
if (ret < 0)
return ret;
 
-   memset(, 0, sizeof(format));
-   format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-   format.pad = RWPF_PAD_SINK;
-   format.format.width = cfg->width;
-   format.format.height = cfg->height;
-   format.format.code = MEDIA_BUS_FMT_ARGB_1X32;
-   format.format.field = V4L2_FIELD_NONE;
-
-   ret = v4l2_subdev_call(>output->entity.subdev, pad, set_fmt, NULL,
-  );
-   if (ret < 0)
-   return ret;
-
-   dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on WPF%u sink\n",
-   __func__, format.format.width, format.format.height,
-   format.format.code, pipe->output->entity.index);
-
-   format.pad = RWPF_PAD_SOURCE;
-   ret = v4l2_subdev_call(>output->entity.subdev, pad, get_fmt, NULL,
-  );
-   if (ret < 0)
-   return ret;
-
-   dev_dbg(vsp1->dev, "%s: got format %ux%u (%x) on WPF%u source\n",
-   __func__, format.format.width, format.format.height,
-   format.format.code, 

[PATCH v2.1 06/15] v4l: vsp1: Move DRM atomic commit pipeline setup to separate function

[Laurent Pinchart]

The DRM pipeline setup code used at atomic commit time is similar to the
setup code used when enabling the pipeline. Move it to a separate
function in order to share it.

Signed-off-by: Laurent Pinchart 
Reviewed-by: Kieran Bingham 
---
Changes since v2:

- Rename vsp1_du_pipeline_setup_input() to
  vsp1_du_pipeline_setup_inputs()
---
 drivers/media/platform/vsp1/vsp1_drm.c | 347 +
 1 file changed, 180 insertions(+), 167 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index 9a043a915c0b..d99278f45bd8 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -46,6 +46,185 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline 
*pipe,
  * Pipeline Configuration
  */
 
+/* Setup one RPF and the connected BRU sink pad. */
+static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1,
+ struct vsp1_pipeline *pipe,
+ struct vsp1_rwpf *rpf,
+ unsigned int bru_input)
+{
+   struct v4l2_subdev_selection sel;
+   struct v4l2_subdev_format format;
+   const struct v4l2_rect *crop;
+   int ret;
+
+   /*
+* Configure the format on the RPF sink pad and propagate it up to the
+* BRU sink pad.
+*/
+   crop = >drm->inputs[rpf->entity.index].crop;
+
+   memset(, 0, sizeof(format));
+   format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+   format.pad = RWPF_PAD_SINK;
+   format.format.width = crop->width + crop->left;
+   format.format.height = crop->height + crop->top;
+   format.format.code = rpf->fmtinfo->mbus;
+   format.format.field = V4L2_FIELD_NONE;
+
+   ret = v4l2_subdev_call(>entity.subdev, pad, set_fmt, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   dev_dbg(vsp1->dev,
+   "%s: set format %ux%u (%x) on RPF%u sink\n",
+   __func__, format.format.width, format.format.height,
+   format.format.code, rpf->entity.index);
+
+   memset(, 0, sizeof(sel));
+   sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+   sel.pad = RWPF_PAD_SINK;
+   sel.target = V4L2_SEL_TGT_CROP;
+   sel.r = *crop;
+
+   ret = v4l2_subdev_call(>entity.subdev, pad, set_selection, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   dev_dbg(vsp1->dev,
+   "%s: set selection (%u,%u)/%ux%u on RPF%u sink\n",
+   __func__, sel.r.left, sel.r.top, sel.r.width, sel.r.height,
+   rpf->entity.index);
+
+   /*
+* RPF source, hardcode the format to ARGB to turn on format
+* conversion if needed.
+*/
+   format.pad = RWPF_PAD_SOURCE;
+
+   ret = v4l2_subdev_call(>entity.subdev, pad, get_fmt, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   dev_dbg(vsp1->dev,
+   "%s: got format %ux%u (%x) on RPF%u source\n",
+   __func__, format.format.width, format.format.height,
+   format.format.code, rpf->entity.index);
+
+   format.format.code = MEDIA_BUS_FMT_ARGB_1X32;
+
+   ret = v4l2_subdev_call(>entity.subdev, pad, set_fmt, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   /* BRU sink, propagate the format from the RPF source. */
+   format.pad = bru_input;
+
+   ret = v4l2_subdev_call(>bru->subdev, pad, set_fmt, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on %s pad %u\n",
+   __func__, format.format.width, format.format.height,
+   format.format.code, BRU_NAME(pipe->bru), format.pad);
+
+   sel.pad = bru_input;
+   sel.target = V4L2_SEL_TGT_COMPOSE;
+   sel.r = vsp1->drm->inputs[rpf->entity.index].compose;
+
+   ret = v4l2_subdev_call(>bru->subdev, pad, set_selection, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   dev_dbg(vsp1->dev, "%s: set selection (%u,%u)/%ux%u on %s pad %u\n",
+   __func__, sel.r.left, sel.r.top, sel.r.width, sel.r.height,
+   BRU_NAME(pipe->bru), sel.pad);
+
+   return 0;
+}
+
+static unsigned int rpf_zpos(struct vsp1_device *vsp1, struct vsp1_rwpf *rpf)
+{
+   return vsp1->drm->inputs[rpf->entity.index].zpos;
+}
+
+/* Setup the input side of the pipeline (RPFs and BRU sink pads). */
+static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1,
+struct vsp1_pipeline *pipe)
+{
+   struct vsp1_rwpf *inputs[VSP1_MAX_RPF] = { NULL, };
+   struct vsp1_bru *bru = to_bru(>bru->subdev);
+   unsigned int 

[PATCH v5 1/3] ARM: dts: r8a7790: Convert to new LVDS DT bindings

[Laurent Pinchart]

The internal LVDS encoder now has DT bindings separate from the DU. Port
the device tree over to the new model.

Fixes: c6a27fa41fab ("drm: rcar-du: Convert LVDS encoder code to bridge driver")
Fixes: 4bdb7aa7dcd0 ("ARM: dts: r8a7790: add soc node")
Signed-off-by: Laurent Pinchart 
---
Changes since v3:

- Added power-domains and resets properties to LVDS nodes

Changes since v2:

- Fixed LVDS compatible string

Changes since v1:

- Remove the DU reg-names property
---
 arch/arm/boot/dts/r8a7790-lager.dts | 22 ++---
 arch/arm/boot/dts/r8a7790.dtsi  | 65 -
 2 files changed, 74 insertions(+), 13 deletions(-)

diff --git a/arch/arm/boot/dts/r8a7790-lager.dts 
b/arch/arm/boot/dts/r8a7790-lager.dts
index 063fdb65dc60..f07f9018c3e7 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -379,7 +379,7 @@
port@0 {
reg = <0>;
adv7511_in: endpoint {
-   remote-endpoint = 
<_out_lvds0>;
+   remote-endpoint = <_out>;
};
};
 
@@ -467,10 +467,8 @@
status = "okay";
 
clocks = < CPG_MOD 724>, < CPG_MOD 723>, < CPG_MOD 722>,
-< CPG_MOD 726>, < CPG_MOD 725>,
 <_clk>, <_clk>;
-   clock-names = "du.0", "du.1", "du.2", "lvds.0", "lvds.1",
- "dclkin.0", "dclkin.1";
+   clock-names = "du.0", "du.1", "du.2", "dclkin.0", "dclkin.1";
 
ports {
port@0 {
@@ -478,12 +476,26 @@
remote-endpoint = <_in>;
};
};
+   };
+};
+
+ {
+   status = "okay";
+
+   ports {
port@1 {
endpoint {
remote-endpoint = <_in>;
};
};
-   port@2 {
+   };
+};
+
+ {
+   status = "okay";
+
+   ports {
+   port@1 {
lvds_connector: endpoint {
};
};
diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
index e4367cecad18..05a0fc23ac88 100644
--- a/arch/arm/boot/dts/r8a7790.dtsi
+++ b/arch/arm/boot/dts/r8a7790.dtsi
@@ -1627,18 +1627,13 @@
 
du: display@feb0 {
compatible = "renesas,du-r8a7790";
-   reg = <0 0xfeb0 0 0x7>,
- <0 0xfeb9 0 0x1c>,
- <0 0xfeb94000 0 0x1c>;
-   reg-names = "du", "lvds.0", "lvds.1";
+   reg = <0 0xfeb0 0 0x7>;
interrupts = ,
 ,
 ;
clocks = < CPG_MOD 724>, < CPG_MOD 723>,
-< CPG_MOD 722>, < CPG_MOD 726>,
-< CPG_MOD 725>;
-   clock-names = "du.0", "du.1", "du.2", "lvds.0",
- "lvds.1";
+< CPG_MOD 722>;
+   clock-names = "du.0", "du.1", "du.2";
status = "disabled";
 
ports {
@@ -1653,11 +1648,65 @@
port@1 {
reg = <1>;
du_out_lvds0: endpoint {
+   remote-endpoint = <_in>;
};
};
port@2 {
reg = <2>;
du_out_lvds1: endpoint {
+   remote-endpoint = <_in>;
+   };
+   };
+   };
+   };
+
+   lvds0: lvds@feb9 {
+   compatible = "renesas,r8a7790-lvds";
+   reg = <0 0xfeb9 0 0x1c>;
+   clocks = < CPG_MOD 726>;
+   power-domains = < R8A7790_PD_ALWAYS_ON>;
+   resets = < 726>;
+   status = "disabled";
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port@0 {
+   reg = <0>;
+   lvds0_in: endpoint {
+   remote-endpoint = 
<_out_lvds0>;
+   };
+   

[PATCH v5 3/3] ARM: dts: r8a7793: Convert to new LVDS DT bindings

[Laurent Pinchart]

The internal LVDS encoder now has DT bindings separate from the DU. Port
the device tree over to the new model.

Fixes: c6a27fa41fab ("drm: rcar-du: Convert LVDS encoder code to bridge driver")
Fixes: bff8f8c2feb7 ("ARM: dts: r8a7793: add soc node")
Signed-off-by: Laurent Pinchart 
---
Changes since v3:

- Added power-domains and resets properties to LVDS nodes

Changes since v2:

- Fixed LVDS compatible string

Changes since v1:

- Remove the DU reg-names property
---
 arch/arm/boot/dts/r8a7793-gose.dts | 10 +++---
 arch/arm/boot/dts/r8a7793.dtsi | 37 +++--
 2 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/arch/arm/boot/dts/r8a7793-gose.dts 
b/arch/arm/boot/dts/r8a7793-gose.dts
index 9ed6961f2d9a..96e117d8b2cc 100644
--- a/arch/arm/boot/dts/r8a7793-gose.dts
+++ b/arch/arm/boot/dts/r8a7793-gose.dts
@@ -447,10 +447,9 @@
pinctrl-names = "default";
status = "okay";
 
-   clocks = < CPG_MOD 724>, < CPG_MOD 723>, < CPG_MOD 726>,
+   clocks = < CPG_MOD 724>, < CPG_MOD 723>,
 <_clk>, <_clk>;
-   clock-names = "du.0", "du.1", "lvds.0",
- "dclkin.0", "dclkin.1";
+   clock-names = "du.0", "du.1", "dclkin.0", "dclkin.1";
 
ports {
port@0 {
@@ -458,6 +457,11 @@
remote-endpoint = <_in>;
};
};
+   };
+};
+
+ {
+   ports {
port@1 {
lvds_connector: endpoint {
};
diff --git a/arch/arm/boot/dts/r8a7793.dtsi b/arch/arm/boot/dts/r8a7793.dtsi
index f9c5a557107d..4f526030dc7c 100644
--- a/arch/arm/boot/dts/r8a7793.dtsi
+++ b/arch/arm/boot/dts/r8a7793.dtsi
@@ -1292,15 +1292,12 @@
 
du: display@feb0 {
compatible = "renesas,du-r8a7793";
-   reg = <0 0xfeb0 0 0x4>,
- <0 0xfeb9 0 0x1c>;
-   reg-names = "du", "lvds.0";
+   reg = <0 0xfeb0 0 0x4>;
interrupts = ,
 ;
clocks = < CPG_MOD 724>,
-< CPG_MOD 723>,
-< CPG_MOD 726>;
-   clock-names = "du.0", "du.1", "lvds.0";
+< CPG_MOD 723>;
+   clock-names = "du.0", "du.1";
status = "disabled";
 
ports {
@@ -1315,6 +1312,34 @@
port@1 {
reg = <1>;
du_out_lvds0: endpoint {
+   remote-endpoint = <_in>;
+   };
+   };
+   };
+   };
+
+   lvds0: lvds@feb9 {
+   compatible = "renesas,r8a7793-lvds";
+   reg = <0 0xfeb9 0 0x1c>;
+   clocks = < CPG_MOD 726>;
+   power-domains = < R8A7793_PD_ALWAYS_ON>;
+   resets = < 726>;
+
+   status = "disabled";
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port@0 {
+   reg = <0>;
+   lvds0_in: endpoint {
+   remote-endpoint = 
<_out_lvds0>;
+   };
+   };
+   port@1 {
+   reg = <1>;
+   lvds0_out: endpoint {
};
};
};
-- 
Regards,

Laurent Pinchart

[PATCH v5 2/3] ARM: dts: r8a7791: Convert to new LVDS DT bindings

[Laurent Pinchart]

The internal LVDS encoder now has DT bindings separate from the DU. Port
the device tree over to the new model.

Fixes: c6a27fa41fab ("drm: rcar-du: Convert LVDS encoder code to bridge driver")
Fixes: bb21803ea440 ("ARM: dts: r8a7791: add soc node")
Signed-off-by: Laurent Pinchart 
---
Changes since v3:

- Added power-domains and resets properties to LVDS nodes

Changes since v2:

- Fixed LVDS compatible string

Changes since v1:

- Remove the DU reg-names property
---
 arch/arm/boot/dts/r8a7791-koelsch.dts | 10 --
 arch/arm/boot/dts/r8a7791-porter.dts  | 16 +---
 arch/arm/boot/dts/r8a7791.dtsi| 36 +--
 3 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts 
b/arch/arm/boot/dts/r8a7791-koelsch.dts
index f40321a1c917..69d66bad9a6a 100644
--- a/arch/arm/boot/dts/r8a7791-koelsch.dts
+++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
@@ -470,8 +470,7 @@
 
clocks = < CPG_MOD 724>, < CPG_MOD 723>, < CPG_MOD 726>,
 <_clk>, <_clk>;
-   clock-names = "du.0", "du.1", "lvds.0",
- "dclkin.0", "dclkin.1";
+   clock-names = "du.0", "du.1", "dclkin.0", "dclkin.1";
 
ports {
port@0 {
@@ -479,6 +478,13 @@
remote-endpoint = <_in>;
};
};
+   };
+};
+
+ {
+   status = "okay";
+
+   ports {
port@1 {
lvds_connector: endpoint {
};
diff --git a/arch/arm/boot/dts/r8a7791-porter.dts 
b/arch/arm/boot/dts/r8a7791-porter.dts
index c14e6fe9e4f6..ae9ed9ff53ef 100644
--- a/arch/arm/boot/dts/r8a7791-porter.dts
+++ b/arch/arm/boot/dts/r8a7791-porter.dts
@@ -441,10 +441,9 @@
pinctrl-names = "default";
status = "okay";
 
-   clocks = < CPG_MOD 724>, < CPG_MOD 723>, < CPG_MOD 726>,
+   clocks = < CPG_MOD 724>, < CPG_MOD 723>,
 <_clk>, <_clk>;
-   clock-names = "du.0", "du.1", "lvds.0",
- "dclkin.0", "dclkin.1";
+   clock-names = "du.0", "du.1", "dclkin.0", "dclkin.1";
 
ports {
port@0 {
@@ -455,6 +454,17 @@
};
 };
 
+ {
+   status = "okay";
+
+   ports {
+   port@1 {
+   lvds_connector: endpoint {
+   };
+   };
+   };
+};
+
 _sound {
pinctrl-0 = <_pins _clk_pins>;
pinctrl-names = "default";
diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
index f11dab71b03a..506b20885413 100644
--- a/arch/arm/boot/dts/r8a7791.dtsi
+++ b/arch/arm/boot/dts/r8a7791.dtsi
@@ -1633,15 +1633,12 @@
 
du: display@feb0 {
compatible = "renesas,du-r8a7791";
-   reg = <0 0xfeb0 0 0x4>,
- <0 0xfeb9 0 0x1c>;
-   reg-names = "du", "lvds.0";
+   reg = <0 0xfeb0 0 0x4>;
interrupts = ,
 ;
clocks = < CPG_MOD 724>,
-< CPG_MOD 723>,
-< CPG_MOD 726>;
-   clock-names = "du.0", "du.1", "lvds.0";
+< CPG_MOD 723>;
+   clock-names = "du.0", "du.1";
status = "disabled";
 
ports {
@@ -1656,6 +1653,33 @@
port@1 {
reg = <1>;
du_out_lvds0: endpoint {
+   remote-endpoint = <_in>;
+   };
+   };
+   };
+   };
+
+   lvds0: lvds@feb9 {
+   compatible = "renesas,r8a7791-lvds";
+   reg = <0 0xfeb9 0 0x1c>;
+   clocks = < CPG_MOD 726>;
+   power-domains = < R8A7791_PD_ALWAYS_ON>;
+   resets = < 726>;
+   status = "disabled";
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port@0 {
+   reg = <0>;
+   lvds0_in: endpoint {
+   remote-endpoint = 
<_out_lvds0>;
+   };
+   };
+   port@1 {
+   reg = <1>;
+   lvds0_out: endpoint {
};
};
};
-- 

[PATCH v5 0/3] R-Car DU: Fix LVDS output on Gen2 boards

[Laurent Pinchart]

Hello,

This patch series fixes LVDS output support on the Lager, Koelsh, Porter and
Gose boards that broke in v4.17-rc1 due to the combination of the R-Car DU
LVDS driver rework and the DT move of all on-SoC peripherals to a /soc node.

We could handle the problem in the R-Car DU LVDS DT backward compatibility
code, but that fix would only be used for v4.17 as in v4.18 the Gen2 DT will
move to the new LVDS DT bindings. I thus propose merging these three patches
in v4.17 already to fix the problem as this is the simplest solution.

The patches are based on top of Linus' master that includes both the R-Car DU
changes and the ARM DT changes for v4.17-rc1. I can rebase them on top of
v4.17-rc1 when it will be released, but I don't expect any change.

Laurent Pinchart (3):
  ARM: dts: r8a7790: Convert to new LVDS DT bindings
  ARM: dts: r8a7791: Convert to new LVDS DT bindings
  ARM: dts: r8a7793: Convert to new LVDS DT bindings

 arch/arm/boot/dts/r8a7790-lager.dts   | 22 +---
 arch/arm/boot/dts/r8a7790.dtsi| 65 ++-
 arch/arm/boot/dts/r8a7791-koelsch.dts | 10 --
 arch/arm/boot/dts/r8a7791-porter.dts  | 16 +++--
 arch/arm/boot/dts/r8a7791.dtsi| 36 +++
 arch/arm/boot/dts/r8a7793-gose.dts| 10 --
 arch/arm/boot/dts/r8a7793.dtsi| 37 
 7 files changed, 163 insertions(+), 33 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 11/15] v4l: vsp1: Add per-display list internal completion notification support

[Kieran Bingham]

Hi Laurent,

On 05/04/18 10:18, Laurent Pinchart wrote:
> Display list completion is already reported to the frame end handler,
> but that mechanism is global to all display lists. In order to implement
> BRU and BRS reassignment in DRM pipelines we will need to commit a
> display list and wait for its completion internally, without reporting
> it to the DRM driver. Extend the display list API to support such an
> internal use of the display list.
> 
> Signed-off-by: Laurent Pinchart 
> ---
> Changes since v1:
> 
> - Use the frame end status flags to report notification
> - Rename the notify flag to internal

This reads much better to me.

Thanks for the respin.

Reviewed-by: Kieran Bingham 


> ---
>  drivers/media/platform/vsp1/vsp1_dl.c| 23 ++-
>  drivers/media/platform/vsp1/vsp1_dl.h|  3 ++-
>  drivers/media/platform/vsp1/vsp1_drm.c   |  2 +-
>  drivers/media/platform/vsp1/vsp1_video.c |  2 +-
>  4 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
> b/drivers/media/platform/vsp1/vsp1_dl.c
> index 662fa2a347c9..30ad491605ff 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -72,6 +72,7 @@ struct vsp1_dl_body {
>   * @fragments: list of extra display list bodies
>   * @has_chain: if true, indicates that there's a partition chain
>   * @chain: entry in the display list partition chain
> + * @internal: whether the display list is used for internal purpose
>   */
>  struct vsp1_dl_list {
>   struct list_head list;
> @@ -85,6 +86,8 @@ struct vsp1_dl_list {
>  
>   bool has_chain;
>   struct list_head chain;
> +
> + bool internal;
>  };
>  
>  enum vsp1_dl_mode {
> @@ -550,8 +553,16 @@ static void vsp1_dl_list_commit_continuous(struct 
> vsp1_dl_list *dl)
>* case we can't replace the queued list by the new one, as we could
>* race with the hardware. We thus mark the update as pending, it will
>* be queued up to the hardware by the frame end interrupt handler.
> +  *
> +  * If a display list is already pending we simply drop it as the new
> +  * display list is assumed to contain a more recent configuration. It is
> +  * an error if the already pending list has the internal flag set, as
> +  * there is then a process waiting for that list to complete. This
> +  * shouldn't happen as the waiting process should perform proper
> +  * locking, but warn just in case.
>*/
>   if (vsp1_dl_list_hw_update_pending(dlm)) {
> + WARN_ON(dlm->pending && dlm->pending->internal);
>   __vsp1_dl_list_put(dlm->pending);
>   dlm->pending = dl;
>   return;
> @@ -581,7 +592,7 @@ static void vsp1_dl_list_commit_singleshot(struct 
> vsp1_dl_list *dl)
>   dlm->active = dl;
>  }
>  
> -void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
> +void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal)
>  {
>   struct vsp1_dl_manager *dlm = dl->dlm;
>   struct vsp1_dl_list *dl_child;
> @@ -598,6 +609,8 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
>   }
>   }
>  
> + dl->internal = internal;
> +
>   spin_lock_irqsave(>lock, flags);
>  
>   if (dlm->singleshot)
> @@ -624,6 +637,10 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
>   * raced with the frame end interrupt. The function always returns with the 
> flag
>   * set in header mode as display list processing is then not continuous and
>   * races never occur.
> + *
> + * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the previous display 
> list
> + * has completed and had been queued with the internal notification flag.
> + * Internal notification is only supported for continuous mode.
>   */
>  unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>  {
> @@ -656,6 +673,10 @@ unsigned int vsp1_dlm_irq_frame_end(struct 
> vsp1_dl_manager *dlm)
>* frame end interrupt. The display list thus becomes active.
>*/
>   if (dlm->queued) {
> + if (dlm->queued->internal)
> + flags |= VSP1_DL_FRAME_END_INTERNAL;
> + dlm->queued->internal = false;
> +
>   __vsp1_dl_list_put(dlm->active);
>   dlm->active = dlm->queued;
>   dlm->queued = NULL;
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h 
> b/drivers/media/platform/vsp1/vsp1_dl.h
> index cbc2fc53e10b..1a5bbd5ddb7b 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> @@ -21,6 +21,7 @@ struct vsp1_dl_list;
>  struct vsp1_dl_manager;
>  
>  #define VSP1_DL_FRAME_END_COMPLETED  BIT(0)
> +#define VSP1_DL_FRAME_END_INTERNAL   BIT(1)
>  
>  void vsp1_dlm_setup(struct vsp1_device *vsp1);
>  
> @@ -34,7 +35,7 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager 
> 

Re: [PATCH v2 09/15] v4l: vsp1: Move DRM pipeline output setup code to a function

[Kieran Bingham]

Hi Laurent,

Thanks for the updates

On 05/04/18 10:18, Laurent Pinchart wrote:
> In order to make the vsp1_du_setup_lif() easier to read, and for
> symmetry with the DRM pipeline input setup, move the pipeline output
> setup code to a separate function.
> 
> Signed-off-by: Laurent Pinchart 
> Reviewed-by: Kieran Bingham 
> --
> Changes since v1:
> 
> - Rename vsp1_du_pipeline_setup_input() to
>   vsp1_du_pipeline_setup_inputs()

Hrm ... I perhaps would have expected this to happen in

[PATCH 06/15] v4l: vsp1: Move DRM atomic commit pipeline setup to separate 
function

But I think that's being quite pedantic - so unless you have a need to respin, I
wouldn't worry about it.

--
Kieran


> - Initialize format local variable to 0 in
>   vsp1_du_pipeline_setup_output()
> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 114 
> ++---
>  1 file changed, 64 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index 00ce99bd1605..a79b05ef 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -193,8 +193,8 @@ static unsigned int rpf_zpos(struct vsp1_device *vsp1, 
> struct vsp1_rwpf *rpf)
>  }
>  
>  /* Setup the input side of the pipeline (RPFs and BRU). */
> -static int vsp1_du_pipeline_setup_input(struct vsp1_device *vsp1,
> - struct vsp1_pipeline *pipe)
> +static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1,
> +  struct vsp1_pipeline *pipe)
>  {
>   struct vsp1_rwpf *inputs[VSP1_MAX_RPF] = { NULL, };
>   struct vsp1_bru *bru = to_bru(>bru->subdev);
> @@ -276,6 +276,65 @@ static int vsp1_du_pipeline_setup_input(struct 
> vsp1_device *vsp1,
>   return 0;
>  }
>  
> +/* Setup the output side of the pipeline (WPF and LIF). */
> +static int vsp1_du_pipeline_setup_output(struct vsp1_device *vsp1,
> +  struct vsp1_pipeline *pipe)
> +{
> + struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
> + struct v4l2_subdev_format format = { 0, };
> + int ret;
> +
> + format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + format.pad = RWPF_PAD_SINK;
> + format.format.width = drm_pipe->width;
> + format.format.height = drm_pipe->height;
> + format.format.code = MEDIA_BUS_FMT_ARGB_1X32;
> + format.format.field = V4L2_FIELD_NONE;
> +
> + ret = v4l2_subdev_call(>output->entity.subdev, pad, set_fmt, NULL,
> +);
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on WPF%u sink\n",
> + __func__, format.format.width, format.format.height,
> + format.format.code, pipe->output->entity.index);
> +
> + format.pad = RWPF_PAD_SOURCE;
> + ret = v4l2_subdev_call(>output->entity.subdev, pad, get_fmt, NULL,
> +);
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(vsp1->dev, "%s: got format %ux%u (%x) on WPF%u source\n",
> + __func__, format.format.width, format.format.height,
> + format.format.code, pipe->output->entity.index);
> +
> + format.pad = LIF_PAD_SINK;
> + ret = v4l2_subdev_call(>lif->subdev, pad, set_fmt, NULL,
> +);
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on LIF%u sink\n",
> + __func__, format.format.width, format.format.height,
> + format.format.code, pipe->lif->index);
> +
> + /*
> +  * Verify that the format at the output of the pipeline matches the
> +  * requested frame size and media bus code.
> +  */
> + if (format.format.width != drm_pipe->width ||
> + format.format.height != drm_pipe->height ||
> + format.format.code != MEDIA_BUS_FMT_ARGB_1X32) {
> + dev_dbg(vsp1->dev, "%s: format mismatch on LIF%u\n", __func__,
> + pipe->lif->index);
> + return -EPIPE;
> + }
> +
> + return 0;
> +}
> +
>  /* Configure all entities in the pipeline. */
>  static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
>  {
> @@ -356,7 +415,6 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int 
> pipe_index,
>   struct vsp1_drm_pipeline *drm_pipe;
>   struct vsp1_pipeline *pipe;
>   struct vsp1_bru *bru;
> - struct v4l2_subdev_format format;
>   unsigned long flags;
>   unsigned int i;
>   int ret;
> @@ -413,58 +471,14 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int 
> pipe_index,
>   __func__, pipe_index, cfg->width, cfg->height);
>  
>   /* Setup formats through the pipeline. */
> - ret = vsp1_du_pipeline_setup_input(vsp1, pipe);
> - if (ret < 0)
> -   

Re: [PATCH v2 10/15] v4l: vsp1: Turn frame end completion status into a bitfield

[Kieran Bingham]

Hi Laurent,

Thanks for this enhancement.

On 05/04/18 10:18, Laurent Pinchart wrote:
> We will soon need to return more than a boolean completion status from
> the vsp1_dlm_irq_frame_end() IRQ handler. Turn the return value into a
> bitfield to prepare for that. No functional change is introduced here.

I think this is a good solution!

Reviewed-by: Kieran Bingham 


> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/media/platform/vsp1/vsp1_dl.c| 22 +-
>  drivers/media/platform/vsp1/vsp1_dl.h|  4 +++-
>  drivers/media/platform/vsp1/vsp1_drm.c   |  5 +++--
>  drivers/media/platform/vsp1/vsp1_pipe.c  |  8 
>  drivers/media/platform/vsp1/vsp1_pipe.h  |  2 +-
>  drivers/media/platform/vsp1/vsp1_video.c |  4 ++--
>  6 files changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
> b/drivers/media/platform/vsp1/vsp1_dl.c
> index 0b86ed01e85d..662fa2a347c9 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -616,14 +616,18 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
>   * vsp1_dlm_irq_frame_end - Display list handler for the frame end interrupt
>   * @dlm: the display list manager
>   *
> - * Return true if the previous display list has completed at frame end, or 
> false
> - * if it has been delayed by one frame because the display list commit raced
> - * with the frame end interrupt. The function always returns true in header 
> mode
> - * as display list processing is then not continuous and races never occur.
> + * Return a set of flags that indicates display list completion status.
> + *
> + * The VSP1_DL_FRAME_END_COMPLETED flag indicates that the previous display 
> list
> + * has completed at frame end. If the flag is not returned display list
> + * completion has been delayed by one frame because the display list commit
> + * raced with the frame end interrupt. The function always returns with the 
> flag
> + * set in header mode as display list processing is then not continuous and
> + * races never occur.
>   */
> -bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> +unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>  {
> - bool completed = false;
> + unsigned int flags = 0;
>  
>   spin_lock(>lock);
>  
> @@ -634,7 +638,7 @@ bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>   if (dlm->singleshot) {
>   __vsp1_dl_list_put(dlm->active);
>   dlm->active = NULL;
> - completed = true;
> + flags |= VSP1_DL_FRAME_END_COMPLETED;
>   goto done;
>   }
>  
> @@ -655,7 +659,7 @@ bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>   __vsp1_dl_list_put(dlm->active);
>   dlm->active = dlm->queued;
>   dlm->queued = NULL;
> - completed = true;
> + flags |= VSP1_DL_FRAME_END_COMPLETED;
>   }
>  
>   /*
> @@ -672,7 +676,7 @@ bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>  done:
>   spin_unlock(>lock);
>  
> - return completed;
> + return flags;
>  }
>  
>  /* Hardware Setup */
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h 
> b/drivers/media/platform/vsp1/vsp1_dl.h
> index ee3508172f0a..cbc2fc53e10b 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> @@ -20,6 +20,8 @@ struct vsp1_dl_fragment;
>  struct vsp1_dl_list;
>  struct vsp1_dl_manager;
>  
> +#define VSP1_DL_FRAME_END_COMPLETED  BIT(0)
> +
>  void vsp1_dlm_setup(struct vsp1_device *vsp1);
>  
>  struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
> @@ -27,7 +29,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device 
> *vsp1,
>   unsigned int prealloc);
>  void vsp1_dlm_destroy(struct vsp1_dl_manager *dlm);
>  void vsp1_dlm_reset(struct vsp1_dl_manager *dlm);
> -bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
> +unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
>  
>  struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm);
>  void vsp1_dl_list_put(struct vsp1_dl_list *dl);
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index a79b05ef..541473b1df67 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -34,12 +34,13 @@
>   */
>  
>  static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
> -bool completed)
> +unsigned int completion)
>  {
>   struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
>  
>   if (drm_pipe->du_complete)
> - drm_pipe->du_complete(drm_pipe->du_private, completed);
> + drm_pipe->du_complete(drm_pipe->du_private,
> +

Re: [PATCH v7 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Laurent Pinchart]

Hi Jacopo,

(CC'ing Mark Brown)

On Friday, 6 April 2018 17:25:58 EEST jacopo mondi wrote:
> On Fri, Apr 06, 2018 at 04:15:35PM +0300, Laurent Pinchart wrote:
> > On Friday, 6 April 2018 15:41:56 EEST 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 
> >> Reviewed-by: Laurent Pinchart 
> >> ---
> >> 
> >>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60
> >>  +++
> >>  1 file changed, 60 insertions(+)
> >>  create mode 100644
> >> 
> >> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.tx
> >> t
> >> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.tx
> >> t
> >> new file mode 100644
> >> index 000..1191f17
> >> --- /dev/null
> >> +++
> >> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.tx
> >> t
> >> @@ -0,0 +1,60 @@
> >> +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
> > 
> > s/to two two/to two/
> > s/stream/streams/
> > 
> >> 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, TTL CLOCKOUT signal, LVDS
> >> input,
> >> +  PPL and digital circuitry
> >> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> >> +- enable-gpios: Output enable GPIO signal, pin name "OE". Active high
> > 
> > As Rob mentioned in a reply to v6, we currently use "enable" as the
> > inverse of "powerdown". I would call this one oe-gpios instead. Quoting
> > Rob:
> > 
> > "Debating "oe" vs. "output-enable" is bikeshedding IMO. Anyone familiar
> > with h/w design should recognize OE."
> 
> I got a different understanding of what Rob meant. I thought "anyone
> familiar with h/w design should recognize OE" as that nobody would get
> confused if a pin named OE in the chip manual is descibed by an
> 'enable' property.
> 
> But as discussed offline, enable has probably to be used as the
> opposite of powerdown for complete chip sleep, not just for output
> pad.
> 
> Anyway, we spent enough time on naming issues, starting from my first
> stupid 'pdwn' permutations then on this semi-standard names.
> 
> I'll send next version with 'powerdown-gpios' and 'oe-gpios'
> properties hoping that would be finally accepted by everyone.

I certainly won't complain (as long as you write pwdn instead of pdwn in the 
driver :-)).

> Same on the mandatory/optional VCC supply thing. Let's try to make
> next version the final one. If the optional property with the dummy
> regulator doesn't satisfy you and it is preferred to have a fixed-regulator
> anyhow in DT I'll do in next version, othewise let's try not to change
> it again. I'll just remark here that in the current Eagle design vcc is
> connected to a power rail with no regulator at all :)

I don't like the dummy regulator much, as it generates a dev_warn(), which 
makes me believe that it's a hack rather than a proper solution. You might 
want to ask Mark Brown for his opinion.

> >> +
> >> +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>;
> >> +  powerdown-gpios = < 15 GPIO_ACTIVE_LOW>;
> >> +
> >> +  ports {
> >> +  #address-cells = <1>;
> >> +  #size-cells = <0>;
> >> +
> >> +  port@0 {
> >> +  reg = <0>;
> >> +
> >> +  lvds_dec_in_0: endpoint {
> >> +  remote-endpoint = <_out>;
> >> +  };
> >> +  };
> >> +
> >> +  port@2{
> >> +  reg = <2>;
> >> +
> >> 

Re: [PATCH 1/7] arm64: dts: renesas: r8a77970: add FCPVD support

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Friday, 6 April 2018 16:08:06 EEST Jacopo Mondi wrote:
> From: Sergei Shtylyov 
> 
> Describe FCPVD0 in the R8A77970 device tree; it will be used by VSPD0 in
> the next patch...
> 
> Based on the original (and large) patch by Daisuke Matsushita
> .
> 
> Signed-off-by: Vladimir Barinov 
> Signed-off-by: Sergei Shtylyov 
> Signed-off-by: Niklas Söderlund 

Reviewed-by: Laurent Pinchart 

> ---
>  arch/arm64/boot/dts/renesas/r8a77970.dtsi | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> b/arch/arm64/boot/dts/renesas/r8a77970.dtsi index e8358d9..71f466d 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> @@ -617,6 +617,14 @@
>   #address-cells = <1>;
>   #size-cells = <0>;
>   };
> +
> + fcpvd0: fcp@fea27000 {
> + compatible = "renesas,fcpv";
> + reg = <0 0xfea27000 0 0x200>;
> + clocks = < CPG_MOD 603>;
> + power-domains = < R8A77970_PD_ALWAYS_ON>;
> + resets = < 603>;
> + };
>   };
> 
>   timer {


-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/7] arm64: dts: renesas: r8a77970: add DU support

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Friday, 6 April 2018 16:08:08 EEST Jacopo Mondi wrote:
> From: Sergei Shtylyov 
> 
> Define the generic R8A77970 part of the DU device node.
> 
> Based on the original (and large) patch by Daisuke Matsushita
> .
> 
> Signed-off-by: Vladimir Barinov 
> Signed-off-by: Sergei Shtylyov 
> Signed-off-by: Niklas Söderlund 

Reviewed-by: Laurent Pinchart 

> ---
>  arch/arm64/boot/dts/renesas/r8a77970.dtsi | 28 
> 1 file changed, 28 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> b/arch/arm64/boot/dts/renesas/r8a77970.dtsi index db06c94..e649e86 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> @@ -635,6 +635,34 @@
>   resets = < 623>;
>   renesas,fcp = <>;
>   };
> +
> + du: display@feb0 {
> + compatible = "renesas,du-r8a77970";
> + reg = <0 0xfeb0 0 0x8>;
> + interrupts = ;
> + clocks = < CPG_MOD 724>;
> + clock-names = "du.0";
> + power-domains = < R8A77970_PD_ALWAYS_ON>;
> + vsps = <>;
> + status = "disabled";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + du_out_rgb: endpoint {
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + du_out_lvds: endpoint {
> + };
> + };
> + };
> + };
>   };
> 
>   timer {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[jacopo mondi]

Hi Laurent,

On Fri, Apr 06, 2018 at 04:15:35PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Friday, 6 April 2018 15:41:56 EEST 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 
> > Reviewed-by: Laurent Pinchart 
> > ---
> >  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 +++
> >  1 file changed, 60 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..1191f17
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > @@ -0,0 +1,60 @@
> > +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
>
> s/to two two/to two/
> s/stream/streams/
>
> > 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, TTL CLOCKOUT signal, LVDS input,
> > +  PPL and digital circuitry
> > +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> > +- enable-gpios: Output enable GPIO signal, pin name "OE". Active high
>
> As Rob mentioned in a reply to v6, we currently use "enable" as the inverse of
> "powerdown". I would call this one oe-gpios instead. Quoting Rob:
>
> "Debating "oe" vs. "output-enable" is bikeshedding IMO. Anyone familiar with
> h/w design should recognize OE."
>

I got a different understanding of what Rob meant. I thought "anyone
familiar with h/w design should recognize OE" as that nobody would get
confused if a pin named OE in the chip manual is descibed by an
'enable' property.

But as discussed offline, enable has probably to be used as the
opposite of powerdown for complete chip sleep, not just for output
pad.

Anyway, we spent enough time on naming issues, starting from my first
stupid 'pdwn' permutations then on this semi-standard names.

I'll send next version with 'powerdown-gpios' and 'oe-gpios'
properties hoping that would be finally accepted by everyone.

Same on the mandatory/optional VCC supply thing. Let's try to make
next version the final one. If the optional property with the dummy
regulator doesn't satisfy you and it is preferred to have a fixed-regulator
anyhow in DT I'll do in next version, othewise let's try not to change
it again. I'll just remark here that in the current Eagle design vcc is
connected to a power rail with no regulator at all :)

Thanks
   j

> > +
> > +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>;
> > +   powerdown-gpios = < 15 GPIO_ACTIVE_LOW>;
> > +
> > +   ports {
> > +   #address-cells = <1>;
> > +   #size-cells = <0>;
> > +
> > +   port@0 {
> > +   reg = <0>;
> > +
> > +   lvds_dec_in_0: endpoint {
> > +   remote-endpoint = <_out>;
> > +   };
> > +   };
> > +
> > +   port@2{
> > +   reg = <2>;
> > +
> > +   lvds_dec_out_2: endpoint {
> > +   remote-endpoint = <_in>;
> > +   };
> > +   };
> > +   };
> > +   };
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>


signature.asc
Description: PGP signature

Re: [PATCH 4/7] arm64: dts: renesas: r8a77970: add the LVDS instance

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Friday, 6 April 2018 16:08:09 EEST Jacopo Mondi wrote:
> From: Niklas Söderlund 
> 
> Add the LVDS device to r8a77970.dtsi in a disabled state. Also connect
> the it to the LVDS output of the DU. While at it align the endpoint name
> of the du to du_out_lvds0 which is used in other Renesas DTS files to
> describe this link.

The endpoint could be renamed in patch 3/7, but it's not a big deal.

> Signed-off-by: Niklas Söderlund 

Reviewed-by: Laurent Pinchart 

> ---
>  arch/arm64/boot/dts/renesas/r8a77970.dtsi | 29 +++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> b/arch/arm64/boot/dts/renesas/r8a77970.dtsi index e649e86..b48d62c 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> @@ -658,7 +658,34 @@
> 
>   port@1 {
>   reg = <1>;
> - du_out_lvds: endpoint {
> + du_out_lvds0: endpoint {
> + remote-endpoint = <_in>;
> + };
> + };
> + };
> + };
> +
> + lvds0: lvds@feb9 {
> + compatible = "renesas,r8a77970-lvds";
> + reg = <0 0xfeb9 0 0x14>;
> + clocks = < CPG_MOD 727>;
> + power-domains = < R8A77970_PD_ALWAYS_ON>;
> + resets = < 727>;
> + status = "disabled";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + lvds0_in: endpoint {
> + remote-endpoint = 
> <_out_lvds0>;
> + };
> + };
> + port@1 {
> + reg = <1>;
> + lvds0_out: endpoint {
>   };
>   };
>   };

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/7] arm64: dts: renesas: eagle: Add ADV7511W and HDMI output

[jacopo mondi]

Hi Laurent,

On Fri, Apr 06, 2018 at 04:51:11PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Friday, 6 April 2018 16:08:12 EEST Jacopo Mondi wrote:
> > From: Niklas Söderlund 
> >
> > Enable HDMI output adding the HDMI connector and the ADV7511W, connected
> > to THC63LVD1024 LVDS decoder output.
> >
> > Signed-off-by: Niklas Söderlund 
> > Signed-off-by: Jacopo Mondi 
> > ---
> >  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 51 ++-
> >  1 file changed, 50 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> > b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts index 9d0e65d..e9f7b83
> > 100644
> > --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> > +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> > @@ -32,6 +32,17 @@
> > reg = <0x0 0x4800 0x0 0x3800>;
> > };
> >
> > +   hdmi-out {
> > +   compatible = "hdmi-connector";
> > +   type = "a";
> > +
> > +   port {
> > +   hdmi_con_out: endpoint {
> > +   remote-endpoint = <_out>;
> > +   };
> > +   };
> > +   };
> > +
> > thc63lvd1024: lvds-decoder {
> > compatible = "thine,thc63lvd1024";
> >
> > @@ -41,11 +52,17 @@
> >
> > port@0 {
> > reg = <0>;
> > -
>
> This is unrelated, if you don't want a blank line here remove it from patch
> 6/7 :-)

No, you're right, this is a leftover from me splitting a single a
patch in 3. According to your comments on other patches in the series
I shouldn't have done that to begin with :)

Thanks
  j

>
> > thc63lvd1024_in: endpoint {
> > remote-endpoint = <_out>;
> > };
> > };
> > +
> > +   port@2 {
> > +   reg = <2>;
> > +   thc63lvd1024_out: endpoint {
> > +   remote-endpoint = <_in>;
> > +   };
> > +   };
> > };
> > };
> >  };
> > @@ -85,6 +102,38 @@
> > gpio-controller;
> > #gpio-cells = <2>;
> > };
> > +
> > +   hdmi@39 {
> > +   compatible = "adi,adv7511w";
> > +   reg = <0x39>;
> > +   interrupt-parent = <>;
> > +   interrupts = <20 IRQ_TYPE_LEVEL_LOW>;
> > +
> > +   adi,input-depth = <8>;
> > +   adi,input-colorspace = "rgb";
> > +   adi,input-clock = "1x";
> > +   adi,input-style = <1>;
> > +   adi,input-justification = "evenly";
> > +
> > +   ports {
> > +   #address-cells = <1>;
> > +   #size-cells = <0>;
> > +
> > +   port@0 {
> > +   reg = <0>;
> > +   adv7511_in: endpoint {
> > +   remote-endpoint = <_out>;
> > +   };
> > +   };
> > +
> > +   port@1 {
> > +   reg = <1>;
> > +   adv7511_out: endpoint {
> > +   remote-endpoint = <_con_out>;
> > +   };
> > +   };
> > +   };
> > +   };
> >  };
> >
> >   {
>
> With patches 5/7, 6/7 and 7/7 merged together and the pinmux removed,
>
> Reviewed-by: Laurent Pinchart 
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>


signature.asc
Description: PGP signature

Re: [PATCH v7 0/2] drm: Add Thine THC63LVD1024 LVDS decoder bridge

[jacopo mondi]

Hi Laurent,

On Fri, Apr 06, 2018 at 04:53:43PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Friday, 6 April 2018 16:08:05 EEST Jacopo Mondi wrote:
> > Hello,
> >this series enables HDMI display on V3M Eagle board.
> >
> > The series is based on Geert's "renesas-drivers-2018-04-03-v4.16" with
> > THC63LVD1024 driver on top (cfr. my in review series:
> > "[PATCH v7 0/2]  drm: Add Thine THC63LVD1024 LVDS decoder bridge")
>
> This isn't a good base for development, as you would pull way too many
> dependencies in. Could you please base v8 on top of v4.17-rc1 (or if you get
> to post it before v4.17-rc1 gets merged, you can use Linus' master, as the
> ARM64 DT pull requests for v4.17-rc1 have been merged) ? It will then be ready
> for Simon to pull in his v4.18 branch.

I used renesas-drivers as it already contains partial r8a77970 support which
is not there in v4.16 (PFC, GPIO, SCIF...)

I should wait for v4.17-rc1 to come out and re-propose on top of that
probably.

>
> > This series includes some preliminary work from Sergei and Niklas. I have
> > reworked the two final patches from Niklas to enable DU first, add the LVDS
> > decoder node, and finally add the ADV7511W chip and enable HDMI output.
> >
> > A branch for testing is available at:
> > git://jmondi.org/linux v3m/renesas-drivers-2018-04-03-v4.16/v7-eagle-dts
> >
> > Thanks
> >j
> >
> > Jacopo Mondi (2):
> >   arm64: dts: renesas: eagle: Enable DU
> >   arm64: dts: renesas: eagle: Add LVDS decoder
> >
> > Niklas Söderlund (2):
> >   arm64: dts: renesas: r8a77970: add the LVDS instance
> >   arm64: dts: renesas: eagle: Add ADV7511W and HDMI output
> >
> > Sergei Shtylyov (3):
> >   arm64: dts: renesas: r8a77970: add FCPVD support
> >   arm64: dts: renesas: r8a77970: add VSPD support
> >   arm64: dts: renesas: r8a77970: add DU support
> >
> >  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 89 +++
> >  arch/arm64/boot/dts/renesas/r8a77970.dtsi  | 73 +
> >  2 files changed, 162 insertions(+)
> >
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>


signature.asc
Description: PGP signature

RE: [RFC PATCH] mmc: renesas_sdhi_internal_dmac: limit DMA RX for old SoCs

[Yoshihiro Shimoda]

Hi Wolfram-san,

Thank you for the patch!

> From: Wolfram Sang, Sent: Thursday, April 5, 2018 1:45 AM
> 
> From: Wolfram Sang 
> 
> Early revisions of certain SoCs cannot do multiple DMA RX streams in
> parallel. To avoid data corruption, only allow one DMA RX channel and
> fall back to PIO, if needed.
> 
> Signed-off-by: Wolfram Sang 
> ---

> Shimoda-san: what do you think of this approach? Please note that I didn't
> remove the 'revision' from the whitelist yet. This will be done incrementally.
> I thought to first fix the data corruption issue.

This approach is nice to me.

I checked this patch and looks good. So,

Reviewed-by: Yoshihiro Shimoda 

And, our test team tested this patch and works correctly. So,

Tested-by: Nguyen Viet Dung 

Best regards,
Yoshihiro Shimoda

RE: [RFC PATCH] mmc: renesas_sdhi_internal_dmac: use more generic whitelisting

[Yoshihiro Shimoda]

Hi Wolfram-san,

Thank you for the patch!

> From: Wolfram Sang, Sent: Friday, April 6, 2018 1:42 AM
> 
> From: Wolfram Sang 
> 
> Whitelisting every ES version does not scale. So, we whitelist whole
> SoCs independent of ES version. If we need specific handling for an ES
> version, we put it to the front, so it will be matched first.
> 
> Signed-off-by: Wolfram Sang 
> ---
> 
> Shimoda-san: here is my example how to re-arrange the whitelisting if we 
> assume
> that the DMA RX fix is going to work out (so this patch depends on it). I
> tested this patch on H3 ES1.0 and ES2.0 with some additional debug output to
> prove that the correct table entry was matched.
> 
> What do you think?

It looks like nice to me! So, I checked this patch and looks good.

Reviewed-by: Yoshihiro Shimoda 

And, our test team tested this patch and works correctly. So,

Tested-by: Nguyen Viet Dung 

Best regards,
Yoshihiro Shimoda

RE: [PATCH] pwm: rcar: simplify getting .drvdata

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Friday, April 6, 2018 2:26 AM
> 
> We should get drvdata from struct device directly. Going via
> platform_device is an unneeded step back and forth.
> 
> Signed-off-by: Wolfram Sang 
> ---
> 
> Only build tested. Fixed numerous times in other drivers, however...

Thank you for the patch!

Acked-by: Yoshihiro Shimoda 

Best regards,
Yoshihiro Shimoda

>  drivers/pwm/pwm-rcar.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> index 91d11f2e2fef..748f614d5375 100644
> --- a/drivers/pwm/pwm-rcar.c
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -261,8 +261,7 @@ MODULE_DEVICE_TABLE(of, rcar_pwm_of_table);
>  #ifdef CONFIG_PM_SLEEP
>  static struct pwm_device *rcar_pwm_dev_to_pwm_dev(struct device *dev)
>  {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct rcar_pwm_chip *rcar_pwm = platform_get_drvdata(pdev);
> + struct rcar_pwm_chip *rcar_pwm = dev_get_drvdata(dev);
>   struct pwm_chip *chip = _pwm->chip;
> 
>   return >pwms[0];
> --
> 2.11.0

RE: [RFC PATCH] mmc: renesas_sdhi_internal_dmac: fall back to PIO if scatterlist doesn't match

[Yoshihiro Shimoda]

Hi Wolfram-san,

Thank you for the patch!

> From: Wolfram Sang, Sent: Thursday, April 5, 2018 2:01 AM
> 
> From: Wolfram Sang 
> 
> If we detect an incompatible scatterlist, we should fall back to PIO,
> too.
> 
> Signed-off-by: Wolfram Sang 
> ---
> 
> I found this while working on the RX DMA issue. I don't see a reason why we
> shouldn't fall back in this case as well. But maybe I am missing something.
> 
> Shimoda-san: what do you think?
> 
>  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c 
> b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> index 380570a26a09..561e90755a3b 100644
> --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> @@ -161,11 +161,7 @@ renesas_sdhi_internal_dmac_start_dma(struct 
> tmio_mmc_host *host,
>   enum dma_data_direction dir;
>   int ret;
> 
> - /* This DMAC cannot handle if sg_len is not 1 */

I would like to keep this comment.

> - WARN_ON(host->sg_len > 1);
> -
> - /* This DMAC cannot handle if buffer is not 8-bytes alignment */
> - if (!IS_ALIGNED(sg->offset, 8))
> + if (WARN_ON(host->sg_len > 1) || !IS_ALIGNED(sg->offset, 8))

As Ulf-san said on other thread about this WARN_ON(),
I think dropping the WARN_ON() is better.

Best regards,
Yoshihiro Shimoda

Re: [PATCH v7 0/2] drm: Add Thine THC63LVD1024 LVDS decoder bridge

[Laurent Pinchart]

Hi Jacopo,

On Friday, 6 April 2018 16:08:05 EEST Jacopo Mondi wrote:
> Hello,
>this series enables HDMI display on V3M Eagle board.
> 
> The series is based on Geert's "renesas-drivers-2018-04-03-v4.16" with
> THC63LVD1024 driver on top (cfr. my in review series:
> "[PATCH v7 0/2]  drm: Add Thine THC63LVD1024 LVDS decoder bridge")

This isn't a good base for development, as you would pull way too many 
dependencies in. Could you please base v8 on top of v4.17-rc1 (or if you get 
to post it before v4.17-rc1 gets merged, you can use Linus' master, as the 
ARM64 DT pull requests for v4.17-rc1 have been merged) ? It will then be ready 
for Simon to pull in his v4.18 branch.

> This series includes some preliminary work from Sergei and Niklas. I have
> reworked the two final patches from Niklas to enable DU first, add the LVDS
> decoder node, and finally add the ADV7511W chip and enable HDMI output.
> 
> A branch for testing is available at:
> git://jmondi.org/linux v3m/renesas-drivers-2018-04-03-v4.16/v7-eagle-dts
> 
> Thanks
>j
> 
> Jacopo Mondi (2):
>   arm64: dts: renesas: eagle: Enable DU
>   arm64: dts: renesas: eagle: Add LVDS decoder
> 
> Niklas Söderlund (2):
>   arm64: dts: renesas: r8a77970: add the LVDS instance
>   arm64: dts: renesas: eagle: Add ADV7511W and HDMI output
> 
> Sergei Shtylyov (3):
>   arm64: dts: renesas: r8a77970: add FCPVD support
>   arm64: dts: renesas: r8a77970: add VSPD support
>   arm64: dts: renesas: r8a77970: add DU support
> 
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 89 +++
>  arch/arm64/boot/dts/renesas/r8a77970.dtsi  | 73 +
>  2 files changed, 162 insertions(+)
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/7] arm64: dts: renesas: eagle: Add ADV7511W and HDMI output

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Friday, 6 April 2018 16:08:12 EEST Jacopo Mondi wrote:
> From: Niklas Söderlund 
> 
> Enable HDMI output adding the HDMI connector and the ADV7511W, connected
> to THC63LVD1024 LVDS decoder output.
> 
> Signed-off-by: Niklas Söderlund 
> Signed-off-by: Jacopo Mondi 
> ---
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 51 ++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts index 9d0e65d..e9f7b83
> 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -32,6 +32,17 @@
>   reg = <0x0 0x4800 0x0 0x3800>;
>   };
> 
> + hdmi-out {
> + compatible = "hdmi-connector";
> + type = "a";
> +
> + port {
> + hdmi_con_out: endpoint {
> + remote-endpoint = <_out>;
> + };
> + };
> + };
> +
>   thc63lvd1024: lvds-decoder {
>   compatible = "thine,thc63lvd1024";
> 
> @@ -41,11 +52,17 @@
> 
>   port@0 {
>   reg = <0>;
> -

This is unrelated, if you don't want a blank line here remove it from patch 
6/7 :-)

>   thc63lvd1024_in: endpoint {
>   remote-endpoint = <_out>;
>   };
>   };
> +
> + port@2 {
> + reg = <2>;
> + thc63lvd1024_out: endpoint {
> + remote-endpoint = <_in>;
> + };
> + };
>   };
>   };
>  };
> @@ -85,6 +102,38 @@
>   gpio-controller;
>   #gpio-cells = <2>;
>   };
> +
> + hdmi@39 {
> + compatible = "adi,adv7511w";
> + reg = <0x39>;
> + interrupt-parent = <>;
> + interrupts = <20 IRQ_TYPE_LEVEL_LOW>;
> +
> + adi,input-depth = <8>;
> + adi,input-colorspace = "rgb";
> + adi,input-clock = "1x";
> + adi,input-style = <1>;
> + adi,input-justification = "evenly";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + adv7511_in: endpoint {
> + remote-endpoint = <_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> + adv7511_out: endpoint {
> + remote-endpoint = <_con_out>;
> + };
> + };
> + };
> + };
>  };
> 
>   {

With patches 5/7, 6/7 and 7/7 merged together and the pinmux removed,

Reviewed-by: Laurent Pinchart 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/7] arm64: dts: renesas: eagle: Add LVDS decoder

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Friday, 6 April 2018 16:08:11 EEST 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 the not-yet-described 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.

As explained in my review of patch 5/7, I'd merge 5/7, 6/7 and 7/7 all 
together as there's little use for enabling the LVDS decoder if there's 
nothing connected at its output. Note also how this patch alone, without 7/7, 
wouldn't comply with the LVDS decoder DT bindings that state that port@2 is 
mandatory.

> Signed-off-by: Jacopo Mondi 
> Reviewed-by: Andrzej Hajda 
> ---
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 29 +++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts index 144b847..9d0e65d
> 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -31,6 +31,23 @@
>   /* first 128MB is reserved for secure area. */
>   reg = <0x0 0x4800 0x0 0x3800>;
>   };
> +
> + thc63lvd1024: lvds-decoder {
> + compatible = "thine,thc63lvd1024";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + thc63lvd1024_in: endpoint {
> + remote-endpoint = <_out>;
> + };
> + };
> + };
> + };
>  };
> 
>   {
> @@ -104,3 +121,15 @@
>   pinctrl-names = "default";
>   status = "okay";
>  };
> +
> + {
> + status = "okay";
> +
> + ports {
> + port@1 {
> + lvds0_out: endpoint {
> + remote-endpoint = <_in>;
> + };
> + };
> + };
> +};

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/7] arm64: dts: renesas: eagle: Enable DU

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Friday, 6 April 2018 16:08:10 EEST Jacopo Mondi wrote:
> Enable DU for Renesas R-Car V3M Eagle board.
> 
> Signed-off-by: Jacopo Mondi 
> ---
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts index 3c5f598..144b847
> 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -76,6 +76,11 @@
>   function = "i2c0";
>   };
> 
> + du_pins: du {
> + groups = "du_rgb666", "du_sync", "du_oddf", "du_clk_out";
> + function = "du";
> + };

As far as I can tell the DU parallel output isn't used on the Eagle board, but 
is used on the Eagle expansion board. I would move this to patch 7/7 in this 
series.

>   scif0_pins: scif0 {
>   groups = "scif0_data";
>   function = "scif0";
> @@ -93,3 +98,9 @@
> 
>   status = "okay";
>  };
> +
> + {
> + pinctrl-0 = <_pins>;
> + pinctrl-names = "default";

These two properties should be moved to patch 7/7 too.

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

There's little use for enabling the DU in DT if you have no output port 
described. I'd move this to patch 6/7.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/7] arm64: dts: renesas: eagle: Enable DU

[Laurent Pinchart]

Hi again,

On Friday, 6 April 2018 16:45:16 EEST Laurent Pinchart wrote:
> On Friday, 6 April 2018 16:08:10 EEST Jacopo Mondi wrote:
> > Enable DU for Renesas R-Car V3M Eagle board.
> > 
> > Signed-off-by: Jacopo Mondi 
> > ---
> > 
> >  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 11 +++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> > b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts index 3c5f598..144b847
> > 100644
> > --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> > +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> > @@ -76,6 +76,11 @@
> > 
> > function = "i2c0";
> > 
> > };
> > 
> > +   du_pins: du {
> > +   groups = "du_rgb666", "du_sync", "du_oddf", "du_clk_out";
> > +   function = "du";
> > +   };
> 
> As far as I can tell the DU parallel output isn't used on the Eagle board,
> but is used on the Eagle expansion board. I would move this to patch 7/7 in
> this series.

My bad, patch 7/7 describes the on-board HDMI encoder, not the one on the 
expansion board. I would thus drop pinmux completely for now until we add 
support for the expansion board.

> > scif0_pins: scif0 {
> > 
> > groups = "scif0_data";
> > function = "scif0";
> > 
> > @@ -93,3 +98,9 @@
> > 
> > status = "okay";
> >  
> >  };
> > 
> > +
> > + {
> > +   pinctrl-0 = <_pins>;
> > +   pinctrl-names = "default";
> 
> These two properties should be moved to patch 7/7 too.

So this should be removed.

> > +   status = "okay";
> > +};
> 
> There's little use for enabling the DU in DT if you have no output port
> described. I'd move this to patch 6/7.

And I'd merge the status attribute and patches 6/7 and 7/7 all together.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/7] arm64: dts: renesas: r8a77970: add VSPD support

[Laurent Pinchart]

On Friday, 6 April 2018 16:08:07 EEST Jacopo Mondi wrote:
> From: Sergei Shtylyov 
> 
> Describe VSPD0 in the R8A77970 device tree; it will be used by DU in
> the next patch...
> 
> Based on the original (and large) patch by Daisuke Matsushita
> .
> 
> Signed-off-by: Vladimir Barinov 
> Signed-off-by: Sergei Shtylyov 
> Signed-off-by: Niklas Söderlund 
> ---
>  arch/arm64/boot/dts/renesas/r8a77970.dtsi | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> b/arch/arm64/boot/dts/renesas/r8a77970.dtsi index 71f466d..db06c94 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
> @@ -625,6 +625,16 @@
>   power-domains = < R8A77970_PD_ALWAYS_ON>;
>   resets = < 603>;
>   };
> +
> + vspd0: vsp@fea2 {
> + compatible = "renesas,vsp2";
> + reg = <0 0xfea2 0 0x4000>;

You need to extend the memory region to include the V6_CLUTn_TBL* registers. I 
would recommend simply extending it to 0x8000 as all other VSP instances, even 
if the registers at 0x7000-0x7fff are not implemented.

Apart from that,

Reviewed-by: Laurent Pinchart 

> + interrupts = ;
> + clocks = < CPG_MOD 623>;
> + power-domains = < R8A77970_PD_ALWAYS_ON>;
> + resets = < 623>;
> + renesas,fcp = <>;
> + };
>   };
> 
>   timer {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 2/2] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Friday, 6 April 2018 15:41:57 EEST Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output converter.
> 
> Signed-off-by: Jacopo Mondi 
> Reviewed-by: Andrzej Hajda 
> Reviewed-by: Niklas Söderlund 
> ---
>  drivers/gpu/drm/bridge/Kconfig|   6 +
>  drivers/gpu/drm/bridge/Makefile   |   1 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 212 +++
>  3 files changed, 219 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3aa65bd..42c9c2d 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -93,6 +93,12 @@ config DRM_SII9234
> It is an I2C driver, that detects connection of MHL bridge
> and starts encapsulation of HDMI signal.
> 
> +config DRM_THINE_THC63LVD1024
> + tristate "Thine THC63LVD1024 LVDS decoder bridge"
> + depends on OF
> + ---help---
> +   Thine THC63LVD1024 LVDS/parallel converter driver.
> +
>  config DRM_TOSHIBA_TC358767
>   tristate "Toshiba TC358767 eDP bridge"
>   depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile
> b/drivers/gpu/drm/bridge/Makefile index 373eb28..fd90b16 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> b/drivers/gpu/drm/bridge/thc63lvd1024.c new file mode 100644
> index 000..c8fad9c
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi 
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 

You should include slab.h as you're using devm_kzalloc().

> +
> +enum thc63_ports {
> + THC63_LVDS_IN0,
> + THC63_LVDS_IN1,
> + THC63_RGB_OUT0,
> + THC63_RGB_OUT1,
> +};
> +
> +struct thc63_dev {
> + struct device *dev;
> +
> + struct regulator *vcc;
> +
> + struct gpio_desc *pdwn;

pwdn ?

> + struct gpio_desc *oe;
> +
> + struct drm_bridge bridge;
> + struct drm_bridge *next;
> +};
> +
> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> +{
> + return container_of(bridge, struct thc63_dev, bridge);
> +}
> +
> +static int thc63_attach(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> +
> + return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
> +}
> +
> +static void thc63_enable(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> + int ret;
> +
> + ret = regulator_enable(thc63->vcc);
> + if (ret) {
> + dev_err(thc63->dev,
> + "Failed to enable regulator \"vcc\": %d\n", ret);
> + return;
> + }
> +
> + if (thc63->pdwn)
> + gpiod_set_value(thc63->pdwn, 0);
> +
> + if (thc63->oe)
> + gpiod_set_value(thc63->oe, 1);

You don't need to check the gpio_desc pointers first, gpiod_set_value() is a 
no-op if the pointer is NULL.

> +}
> +
> +static void thc63_disable(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> + int ret;
> +
> + if (thc63->oe)
> + gpiod_set_value(thc63->oe, 0);
> +
> + if (thc63->pdwn)
> + gpiod_set_value(thc63->pdwn, 1);

Same here.

> + ret = regulator_disable(thc63->vcc);
> + if (ret)
> + dev_err(thc63->dev,
> + "Failed to disable regulator \"vcc\": %d\n", ret);
> +}
> +
> +static const 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;
> +
> + thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> +   THC63_RGB_OUT0, -1);
> + if (!thc63_out) {
> + dev_err(thc63->dev, "Missing endpoint in port@%u\n",
> + THC63_RGB_OUT0);
> + return -ENODEV;
> + }
> +
> + remote = of_graph_get_remote_port_parent(thc63_out);
> + of_node_put(thc63_out);
> + if 

Re: [PATCH v7 0/2] drm: Add Thine THC63LVD1024 LVDS decoder bridge

[jacopo mondi]

Sorry for the mess

subject should have been

   Subject: [PATCH 0/7] V3M-Eagle display enablement

I copied the wrong one from another cover letter...

On Fri, Apr 06, 2018 at 03:08:05PM +0200, Jacopo Mondi wrote:
> Hello,
>this series enables HDMI display on V3M Eagle board.
>
> The series is based on Geert's "renesas-drivers-2018-04-03-v4.16" with
> THC63LVD1024 driver on top (cfr. my in review series:
> "[PATCH v7 0/2]  drm: Add Thine THC63LVD1024 LVDS decoder bridge")
>
> This series includes some preliminary work from Sergei and Niklas. I have
> reworked the two final patches from Niklas to enable DU first, add the LVDS
> decoder node, and finally add the ADV7511W chip and enable HDMI output.
>
> A branch for testing is available at:
> git://jmondi.org/linux v3m/renesas-drivers-2018-04-03-v4.16/v7-eagle-dts
>
> Thanks
>j
>
> Jacopo Mondi (2):
>   arm64: dts: renesas: eagle: Enable DU
>   arm64: dts: renesas: eagle: Add LVDS decoder
>
> Niklas Söderlund (2):
>   arm64: dts: renesas: r8a77970: add the LVDS instance
>   arm64: dts: renesas: eagle: Add ADV7511W and HDMI output
>
> Sergei Shtylyov (3):
>   arm64: dts: renesas: r8a77970: add FCPVD support
>   arm64: dts: renesas: r8a77970: add VSPD support
>   arm64: dts: renesas: r8a77970: add DU support
>
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 89 
> ++
>  arch/arm64/boot/dts/renesas/r8a77970.dtsi  | 73 +
>  2 files changed, 162 insertions(+)
>
> --
> 2.7.4
>


signature.asc
Description: PGP signature

Re: [PATCH v7 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Friday, 6 April 2018 15:41:56 EEST 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 
> Reviewed-by: Laurent Pinchart 
> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 +++
>  1 file changed, 60 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..1191f17
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -0,0 +1,60 @@
> +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

s/to two two/to two/
s/stream/streams/

> 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, TTL CLOCKOUT signal, LVDS input,
> +  PPL and digital circuitry
> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> +- enable-gpios: Output enable GPIO signal, pin name "OE". Active high

As Rob mentioned in a reply to v6, we currently use "enable" as the inverse of 
"powerdown". I would call this one oe-gpios instead. Quoting Rob:

"Debating "oe" vs. "output-enable" is bikeshedding IMO. Anyone familiar with 
h/w design should recognize OE."

> +
> +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>;
> + powerdown-gpios = < 15 GPIO_ACTIVE_LOW>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + lvds_dec_in_0: endpoint {
> + remote-endpoint = <_out>;
> + };
> + };
> +
> + port@2{
> + reg = <2>;
> +
> + lvds_dec_out_2: endpoint {
> + remote-endpoint = <_in>;
> + };
> + };
> + };
> + };

-- 
Regards,

Laurent Pinchart

[PATCH 2/7] arm64: dts: renesas: r8a77970: add VSPD support

[Jacopo Mondi]

From: Sergei Shtylyov 

Describe VSPD0 in the R8A77970 device tree; it will be used by DU in
the next patch...

Based on the original (and large) patch by Daisuke Matsushita
.

Signed-off-by: Vladimir Barinov 
Signed-off-by: Sergei Shtylyov 
Signed-off-by: Niklas Söderlund 
---
 arch/arm64/boot/dts/renesas/r8a77970.dtsi | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi 
b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
index 71f466d..db06c94 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
@@ -625,6 +625,16 @@
power-domains = < R8A77970_PD_ALWAYS_ON>;
resets = < 603>;
};
+
+   vspd0: vsp@fea2 {
+   compatible = "renesas,vsp2";
+   reg = <0 0xfea2 0 0x4000>;
+   interrupts = ;
+   clocks = < CPG_MOD 623>;
+   power-domains = < R8A77970_PD_ALWAYS_ON>;
+   resets = < 623>;
+   renesas,fcp = <>;
+   };
};
 
timer {
-- 
2.7.4

[PATCH 1/7] arm64: dts: renesas: r8a77970: add FCPVD support

[Jacopo Mondi]

From: Sergei Shtylyov 

Describe FCPVD0 in the R8A77970 device tree; it will be used by VSPD0 in
the next patch...

Based on the original (and large) patch by Daisuke Matsushita
.

Signed-off-by: Vladimir Barinov 
Signed-off-by: Sergei Shtylyov 
Signed-off-by: Niklas Söderlund 
---
 arch/arm64/boot/dts/renesas/r8a77970.dtsi | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi 
b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
index e8358d9..71f466d 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
@@ -617,6 +617,14 @@
#address-cells = <1>;
#size-cells = <0>;
};
+
+   fcpvd0: fcp@fea27000 {
+   compatible = "renesas,fcpv";
+   reg = <0 0xfea27000 0 0x200>;
+   clocks = < CPG_MOD 603>;
+   power-domains = < R8A77970_PD_ALWAYS_ON>;
+   resets = < 603>;
+   };
};
 
timer {
-- 
2.7.4

[PATCH 4/7] arm64: dts: renesas: r8a77970: add the LVDS instance

[Jacopo Mondi]

From: Niklas Söderlund 

Add the LVDS device to r8a77970.dtsi in a disabled state. Also connect
the it to the LVDS output of the DU. While at it align the endpoint name
of the du to du_out_lvds0 which is used in other Renesas DTS files to
describe this link.

Signed-off-by: Niklas Söderlund 
---
 arch/arm64/boot/dts/renesas/r8a77970.dtsi | 29 -
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi 
b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
index e649e86..b48d62c 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
@@ -658,7 +658,34 @@
 
port@1 {
reg = <1>;
-   du_out_lvds: endpoint {
+   du_out_lvds0: endpoint {
+   remote-endpoint = <_in>;
+   };
+   };
+   };
+   };
+
+   lvds0: lvds@feb9 {
+   compatible = "renesas,r8a77970-lvds";
+   reg = <0 0xfeb9 0 0x14>;
+   clocks = < CPG_MOD 727>;
+   power-domains = < R8A77970_PD_ALWAYS_ON>;
+   resets = < 727>;
+   status = "disabled";
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port@0 {
+   reg = <0>;
+   lvds0_in: endpoint {
+   remote-endpoint = 
<_out_lvds0>;
+   };
+   };
+   port@1 {
+   reg = <1>;
+   lvds0_out: endpoint {
};
};
};
-- 
2.7.4

[PATCH 3/7] arm64: dts: renesas: r8a77970: add DU support

[Jacopo Mondi]

From: Sergei Shtylyov 

Define the generic R8A77970 part of the DU device node.

Based on the original (and large) patch by Daisuke Matsushita
.

Signed-off-by: Vladimir Barinov 
Signed-off-by: Sergei Shtylyov 
Signed-off-by: Niklas Söderlund 
---
 arch/arm64/boot/dts/renesas/r8a77970.dtsi | 28 
 1 file changed, 28 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a77970.dtsi 
b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
index db06c94..e649e86 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77970.dtsi
@@ -635,6 +635,34 @@
resets = < 623>;
renesas,fcp = <>;
};
+
+   du: display@feb0 {
+   compatible = "renesas,du-r8a77970";
+   reg = <0 0xfeb0 0 0x8>;
+   interrupts = ;
+   clocks = < CPG_MOD 724>;
+   clock-names = "du.0";
+   power-domains = < R8A77970_PD_ALWAYS_ON>;
+   vsps = <>;
+   status = "disabled";
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port@0 {
+   reg = <0>;
+   du_out_rgb: endpoint {
+   };
+   };
+
+   port@1 {
+   reg = <1>;
+   du_out_lvds: endpoint {
+   };
+   };
+   };
+   };
};
 
timer {
-- 
2.7.4

[PATCH 6/7] arm64: dts: renesas: eagle: Add LVDS decoder

[Jacopo Mondi]

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 the not-yet-described 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 | 29 ++
 1 file changed, 29 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts 
b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index 144b847..9d0e65d 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -31,6 +31,23 @@
/* first 128MB is reserved for secure area. */
reg = <0x0 0x4800 0x0 0x3800>;
};
+
+   thc63lvd1024: lvds-decoder {
+   compatible = "thine,thc63lvd1024";
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port@0 {
+   reg = <0>;
+
+   thc63lvd1024_in: endpoint {
+   remote-endpoint = <_out>;
+   };
+   };
+   };
+   };
 };
 
  {
@@ -104,3 +121,15 @@
pinctrl-names = "default";
status = "okay";
 };
+
+ {
+   status = "okay";
+
+   ports {
+   port@1 {
+   lvds0_out: endpoint {
+   remote-endpoint = <_in>;
+   };
+   };
+   };
+};
-- 
2.7.4

[PATCH 7/7] arm64: dts: renesas: eagle: Add ADV7511W and HDMI output

[Jacopo Mondi]

From: Niklas Söderlund 

Enable HDMI output adding the HDMI connector and the ADV7511W, connected
to THC63LVD1024 LVDS decoder output.

Signed-off-by: Niklas Söderlund 
Signed-off-by: Jacopo Mondi 
---
 arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 51 +-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts 
b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index 9d0e65d..e9f7b83 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -32,6 +32,17 @@
reg = <0x0 0x4800 0x0 0x3800>;
};
 
+   hdmi-out {
+   compatible = "hdmi-connector";
+   type = "a";
+
+   port {
+   hdmi_con_out: endpoint {
+   remote-endpoint = <_out>;
+   };
+   };
+   };
+
thc63lvd1024: lvds-decoder {
compatible = "thine,thc63lvd1024";
 
@@ -41,11 +52,17 @@
 
port@0 {
reg = <0>;
-
thc63lvd1024_in: endpoint {
remote-endpoint = <_out>;
};
};
+
+   port@2 {
+   reg = <2>;
+   thc63lvd1024_out: endpoint {
+   remote-endpoint = <_in>;
+   };
+   };
};
};
 };
@@ -85,6 +102,38 @@
gpio-controller;
#gpio-cells = <2>;
};
+
+   hdmi@39 {
+   compatible = "adi,adv7511w";
+   reg = <0x39>;
+   interrupt-parent = <>;
+   interrupts = <20 IRQ_TYPE_LEVEL_LOW>;
+
+   adi,input-depth = <8>;
+   adi,input-colorspace = "rgb";
+   adi,input-clock = "1x";
+   adi,input-style = <1>;
+   adi,input-justification = "evenly";
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port@0 {
+   reg = <0>;
+   adv7511_in: endpoint {
+   remote-endpoint = <_out>;
+   };
+   };
+
+   port@1 {
+   reg = <1>;
+   adv7511_out: endpoint {
+   remote-endpoint = <_con_out>;
+   };
+   };
+   };
+   };
 };
 
  {
-- 
2.7.4

[PATCH 5/7] arm64: dts: renesas: eagle: Enable DU

[Jacopo Mondi]

Enable DU for Renesas R-Car V3M Eagle board.

Signed-off-by: Jacopo Mondi 
---
 arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts 
b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index 3c5f598..144b847 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -76,6 +76,11 @@
function = "i2c0";
};
 
+   du_pins: du {
+   groups = "du_rgb666", "du_sync", "du_oddf", "du_clk_out";
+   function = "du";
+   };
+
scif0_pins: scif0 {
groups = "scif0_data";
function = "scif0";
@@ -93,3 +98,9 @@
 
status = "okay";
 };
+
+ {
+   pinctrl-0 = <_pins>;
+   pinctrl-names = "default";
+   status = "okay";
+};
-- 
2.7.4

[PATCH v7 0/2] drm: Add Thine THC63LVD1024 LVDS decoder bridge

[Jacopo Mondi]

Hello,
   this series enables HDMI display on V3M Eagle board.

The series is based on Geert's "renesas-drivers-2018-04-03-v4.16" with
THC63LVD1024 driver on top (cfr. my in review series:
"[PATCH v7 0/2]  drm: Add Thine THC63LVD1024 LVDS decoder bridge")

This series includes some preliminary work from Sergei and Niklas. I have
reworked the two final patches from Niklas to enable DU first, add the LVDS
decoder node, and finally add the ADV7511W chip and enable HDMI output.

A branch for testing is available at:
git://jmondi.org/linux v3m/renesas-drivers-2018-04-03-v4.16/v7-eagle-dts

Thanks
   j

Jacopo Mondi (2):
  arm64: dts: renesas: eagle: Enable DU
  arm64: dts: renesas: eagle: Add LVDS decoder

Niklas Söderlund (2):
  arm64: dts: renesas: r8a77970: add the LVDS instance
  arm64: dts: renesas: eagle: Add ADV7511W and HDMI output

Sergei Shtylyov (3):
  arm64: dts: renesas: r8a77970: add FCPVD support
  arm64: dts: renesas: r8a77970: add VSPD support
  arm64: dts: renesas: r8a77970: add DU support

 arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 89 ++
 arch/arm64/boot/dts/renesas/r8a77970.dtsi  | 73 +
 2 files changed, 162 insertions(+)

--
2.7.4

[PATCH v7 2/2] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Jacopo Mondi]

Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
output converter.

Signed-off-by: Jacopo Mondi 
Reviewed-by: Andrzej Hajda 
Reviewed-by: Niklas Söderlund 
---
 drivers/gpu/drm/bridge/Kconfig|   6 +
 drivers/gpu/drm/bridge/Makefile   |   1 +
 drivers/gpu/drm/bridge/thc63lvd1024.c | 212 ++
 3 files changed, 219 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 3aa65bd..42c9c2d 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -93,6 +93,12 @@ config DRM_SII9234
  It is an I2C driver, that detects connection of MHL bridge
  and starts encapsulation of HDMI signal.
 
+config DRM_THINE_THC63LVD1024
+   tristate "Thine THC63LVD1024 LVDS decoder bridge"
+   depends on OF
+   ---help---
+ Thine THC63LVD1024 LVDS/parallel converter driver.
+
 config DRM_TOSHIBA_TC358767
tristate "Toshiba TC358767 eDP bridge"
depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 373eb28..fd90b16 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
 obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
 obj-$(CONFIG_DRM_SII902X) += sii902x.o
 obj-$(CONFIG_DRM_SII9234) += sii9234.o
+obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
 obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
 obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
 obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
b/drivers/gpu/drm/bridge/thc63lvd1024.c
new file mode 100644
index 000..c8fad9c
--- /dev/null
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * THC63LVD1024 LVDS to parallel data DRM bridge driver.
+ *
+ * Copyright (C) 2018 Jacopo Mondi 
+ */
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+enum thc63_ports {
+   THC63_LVDS_IN0,
+   THC63_LVDS_IN1,
+   THC63_RGB_OUT0,
+   THC63_RGB_OUT1,
+};
+
+struct thc63_dev {
+   struct device *dev;
+
+   struct regulator *vcc;
+
+   struct gpio_desc *pdwn;
+   struct gpio_desc *oe;
+
+   struct drm_bridge bridge;
+   struct drm_bridge *next;
+};
+
+static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
+{
+   return container_of(bridge, struct thc63_dev, bridge);
+}
+
+static int thc63_attach(struct drm_bridge *bridge)
+{
+   struct thc63_dev *thc63 = to_thc63(bridge);
+
+   return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
+}
+
+static void thc63_enable(struct drm_bridge *bridge)
+{
+   struct thc63_dev *thc63 = to_thc63(bridge);
+   int ret;
+
+   ret = regulator_enable(thc63->vcc);
+   if (ret) {
+   dev_err(thc63->dev,
+   "Failed to enable regulator \"vcc\": %d\n", ret);
+   return;
+   }
+
+   if (thc63->pdwn)
+   gpiod_set_value(thc63->pdwn, 0);
+
+   if (thc63->oe)
+   gpiod_set_value(thc63->oe, 1);
+}
+
+static void thc63_disable(struct drm_bridge *bridge)
+{
+   struct thc63_dev *thc63 = to_thc63(bridge);
+   int ret;
+
+   if (thc63->oe)
+   gpiod_set_value(thc63->oe, 0);
+
+   if (thc63->pdwn)
+   gpiod_set_value(thc63->pdwn, 1);
+
+   ret = regulator_disable(thc63->vcc);
+   if (ret)
+   dev_err(thc63->dev,
+   "Failed to disable regulator \"vcc\": %d\n", ret);
+}
+
+static const 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;
+
+   thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
+ THC63_RGB_OUT0, -1);
+   if (!thc63_out) {
+   dev_err(thc63->dev, "Missing endpoint in port@%u\n",
+   THC63_RGB_OUT0);
+   return -ENODEV;
+   }
+
+   remote = of_graph_get_remote_port_parent(thc63_out);
+   of_node_put(thc63_out);
+   if (!remote) {
+   dev_err(thc63->dev, "Endpoint in port@%u unconnected\n",
+   THC63_RGB_OUT0);
+   return -ENODEV;
+   }
+
+   if (!of_device_is_available(remote)) {
+   dev_err(thc63->dev, "port@%u remote endpoint is disabled\n",
+   THC63_RGB_OUT0);
+   of_node_put(remote);
+   return -ENODEV;
+   }
+
+   thc63->next = of_drm_find_bridge(remote);
+   

[PATCH v7 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Jacopo Mondi]

Document Thine THC63LVD1024 LVDS decoder device tree bindings.

Signed-off-by: Jacopo Mondi 
Reviewed-by: Andrzej Hajda 
Reviewed-by: Niklas Söderlund 
Reviewed-by: Laurent Pinchart 
---
 .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 ++
 1 file changed, 60 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..1191f17
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
@@ -0,0 +1,60 @@
+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, TTL CLOCKOUT signal, LVDS input,
+  PPL and digital circuitry
+- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
+- enable-gpios: Output enable GPIO signal, pin name "OE". 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>;
+   powerdown-gpios = < 15 GPIO_ACTIVE_LOW>;
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port@0 {
+   reg = <0>;
+
+   lvds_dec_in_0: endpoint {
+   remote-endpoint = <_out>;
+   };
+   };
+
+   port@2{
+   reg = <2>;
+
+   lvds_dec_out_2: endpoint {
+   remote-endpoint = <_in>;
+   };
+   };
+   };
+   };
--
2.7.4

[PATCH v7 0/2] drm: Add Thine THC63LVD1024 LVDS decoder bridge

[Jacopo Mondi]

Hello,
this new version moves the driver and its bindings to use semi-standard
names for powerdown and output enable GPIOs, as result of the discussion with
Laurent, Vladimir and Rob. I kept the actual pin names in the bindings
description for reference, even if there are no huge ambiguities on which
chip pin is actually an enable and which one a power down.

I have reworked the regulator management, making the 'vcc' supply the only
requested one, and all other optional supplies have been removed as suggested
by Laurent. It is unlikely a dedicated regulator is to be installed for each
power supply, and in case some HW design requires this, it's an easy add to be
implemented in future.

Contrary to what discussed on v6, the 'vcc' supply is still described
as optional in dt bindings, and the driver is now using
'regulator_get(NORMAL_GET)' in place of the _optional() version that was used
before. With the 'NORMAL_GET' version the regulator core provides a dummy
regulator in case an actual one is not available. This simplifies integration
in designs where the chip power supplies are directly connected to some power
rail. At the same time it makes easier to forget to add a regulator if there's
actually one there, and someone could find herself wondering why the chip does
not work even if probe completes properly.

I removed the Eagle display enablement patch from the series, I'll send it
separately squashed on top of Niklas' series that addresses the issue.

Thanks
   j

v6 -> v7:
- Use semi-standard names for powerdown and output enable GPIOs as suggested
  by Rob and Vladimir
- Use 'regulator_get()' not the optional version, and list only 'vcc' as
  requested supply
- Addressed Laurent's review comments and removed Eagle display enablement patch
  to be sent separately

v5 -> v6:
- Drop check for CONFIG_OF as it is a Kconfig dependency
- Add Niklas Reviewed-by tags
- List [3/3] depenencies below commit message to ease integration

v4 -> v5:
- Fix punctuation in bindings documentation
- Add small statement to bindings document to clarify the chip has no
  control bus
- Print regulator name in enable/disable routines error path
- Add Andrzej Reviewed-by tag

v3 -> v4:
- Rename permutations of "pdwn" to just "pdwn" everywhere in the series
- Improve power enable/disable routines as suggested by Andrzej and Sergei
- Change "pdwn" gpio initialization to use the logical output level
- Change Kconfig description

v2 -> v3:
- Drop support for "lvds-decoder" and make the driver THC63LVD1024 specific
-- Rework bindings to describe multiple input/output ports
-- Rename driver and remove "lvds-decoder" references
-- Rework Eagle DTS to use new bindings

v1 -> v2:
- Drop support for THC63LVD1024

Jacopo Mondi (2):
  dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder
  drm: bridge: Add thc63lvd1024 LVDS decoder driver

 .../bindings/display/bridge/thine,thc63lvd1024.txt |  60 ++
 drivers/gpu/drm/bridge/Kconfig |   6 +
 drivers/gpu/drm/bridge/Makefile|   1 +
 drivers/gpu/drm/bridge/thc63lvd1024.c  | 212 +
 4 files changed, 279 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
 create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c

--
2.7.4

Re: [PATCH 0/4] Add R8A77980 PCIe support & some driver cleanups

[Sergei Shtylyov]

On 04/06/2018 01:58 PM, Sergei Shtylyov wrote:
> Hello!
> 
> Here's a set of 4 patches against the 'pci.rcar' branch of Lorenzo Pieralisi's

   I meant to type 'pci/rcar'. :-)

[...]

MBR, Sergei 

[PATCH 4/4] pcie-rcar: factor out rcar_pcie_hw_init() call

[Sergei Shtylyov]

We now have rcar_pcie_hw_init_{h1|gen2|gen3}() differing only in the PCIe
PHY init code and all ending with a call to rcar_pcie_hw_init(), thus it
makes  sense to move that call into the driver's probe() method and then
rename those functions to rcar_pcie_phy_init_{h1|gen2|gen3}() -- doing
this saves 48 bytes of object code (AArch64 gcc 4.8.5)...

Signed-off-by: Sergei Shtylyov 

---
 drivers/pci/host/pcie-rcar.c |   42 ++
 1 file changed, 22 insertions(+), 20 deletions(-)

Index: pci/drivers/pci/host/pcie-rcar.c
===
--- pci.orig/drivers/pci/host/pcie-rcar.c
+++ pci/drivers/pci/host/pcie-rcar.c
@@ -626,7 +626,7 @@ static int rcar_pcie_hw_init(struct rcar
return 0;
 }
 
-static int rcar_pcie_hw_init_h1(struct rcar_pcie *pcie)
+static int rcar_pcie_phy_init_h1(struct rcar_pcie *pcie)
 {
/* Initialize the phy */
phy_write_reg(pcie, 0, 0x42, 0x1, 0x0EC34191);
@@ -646,10 +646,10 @@ static int rcar_pcie_hw_init_h1(struct r
phy_write_reg(pcie, 0, 0x64, 0x1, 0x3F0F1F0F);
phy_write_reg(pcie, 0, 0x66, 0x1, 0x8000);
 
-   return rcar_pcie_hw_init(pcie);
+   return 0;
 }
 
-static int rcar_pcie_hw_init_gen2(struct rcar_pcie *pcie)
+static int rcar_pcie_phy_init_gen2(struct rcar_pcie *pcie)
 {
/*
 * These settings come from the R-Car Series, 2nd Generation User's
@@ -666,10 +666,10 @@ static int rcar_pcie_hw_init_gen2(struct
rcar_pci_write_reg(pcie, 0x0001, GEN2_PCIEPHYCTRL);
rcar_pci_write_reg(pcie, 0x0006, GEN2_PCIEPHYCTRL);
 
-   return rcar_pcie_hw_init(pcie);
+   return 0;
 }
 
-static int rcar_pcie_hw_init_gen3(struct rcar_pcie *pcie)
+static int rcar_pcie_phy_init_gen3(struct rcar_pcie *pcie)
 {
int err;
 
@@ -677,11 +677,7 @@ static int rcar_pcie_hw_init_gen3(struct
if (err)
return err;
 
-   err = phy_power_on(pcie->phy);
-   if (err)
-   return err;
-
-   return rcar_pcie_hw_init(pcie);
+   return phy_power_on(pcie->phy);
 }
 
 static int rcar_msi_alloc(struct rcar_msi *chip)
@@ -1082,17 +1078,18 @@ static int rcar_pcie_parse_map_dma_range
 }
 
 static const struct of_device_id rcar_pcie_of_match[] = {
-   { .compatible = "renesas,pcie-r8a7779", .data = rcar_pcie_hw_init_h1 },
+   { .compatible = "renesas,pcie-r8a7779",
+ .data = rcar_pcie_phy_init_h1 },
{ .compatible = "renesas,pcie-r8a7790",
- .data = rcar_pcie_hw_init_gen2 },
+ .data = rcar_pcie_phy_init_gen2 },
{ .compatible = "renesas,pcie-r8a7791",
- .data = rcar_pcie_hw_init_gen2 },
+ .data = rcar_pcie_phy_init_gen2 },
{ .compatible = "renesas,pcie-rcar-gen2",
- .data = rcar_pcie_hw_init_gen2 },
+ .data = rcar_pcie_phy_init_gen2 },
{ .compatible = "renesas,pcie-r8a7795",
- .data = rcar_pcie_hw_init_gen3 },
+ .data = rcar_pcie_phy_init_gen3 },
{ .compatible = "renesas,pcie-rcar-gen3",
- .data = rcar_pcie_hw_init_gen3 },
+ .data = rcar_pcie_phy_init_gen3 },
{},
 };
 
@@ -1140,7 +1137,7 @@ static int rcar_pcie_probe(struct platfo
struct rcar_pcie *pcie;
unsigned int data;
int err;
-   int (*hw_init_fn)(struct rcar_pcie *);
+   int (*phy_init_fn)(struct rcar_pcie *);
struct pci_host_bridge *bridge;
 
bridge = pci_alloc_host_bridge(sizeof(*pcie));
@@ -1174,10 +1171,15 @@ static int rcar_pcie_probe(struct platfo
goto err_pm_disable;
}
 
-   /* Failure to get a link might just be that no cards are inserted */
-   hw_init_fn = of_device_get_match_data(dev);
-   err = hw_init_fn(pcie);
+   phy_init_fn = of_device_get_match_data(dev);
+   err = phy_init_fn(pcie);
if (err) {
+   dev_err(dev, "failed to init PCIe PHY\n");
+   goto err_pm_put;
+   }
+
+   /* Failure to get a link might just be that no cards are inserted */
+   if (rcar_pcie_hw_init(pcie)) {
dev_info(dev, "PCIe link down\n");
err = -ENODEV;
goto err_pm_put;

[PATCH 3/4] pcie-rcar: add R-Car gen3 PHY support

[Sergei Shtylyov]

On R-Car gen3 SoCs the PCIe PHY has its own register region -- and I have
written  a generic PHY driver for it, thus we need to add the corresponding
code in  rcar_pcie_hw_init_gen3() and call devm_phy_optional_get() at the
driver's probing time,  so that the existing R-Car gen3 device trees (not
having a PHY node) would still work (we only need  to power up the PHY on
R-Car V3H).

Signed-off-by: Sergei Shtylyov 

---
 drivers/pci/host/pcie-rcar.c |   27 +--
 1 file changed, 25 insertions(+), 2 deletions(-)

Index: pci/drivers/pci/host/pcie-rcar.c
===
--- pci.orig/drivers/pci/host/pcie-rcar.c
+++ pci/drivers/pci/host/pcie-rcar.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -140,6 +141,7 @@ static inline struct rcar_msi *to_rcar_m
 /* Structure representing the PCIe interface */
 struct rcar_pcie {
struct device   *dev;
+   struct phy  *phy;
void __iomem*base;
struct list_headresources;
int root_bus_nr;
@@ -667,6 +669,21 @@ static int rcar_pcie_hw_init_gen2(struct
return rcar_pcie_hw_init(pcie);
 }
 
+static int rcar_pcie_hw_init_gen3(struct rcar_pcie *pcie)
+{
+   int err;
+
+   err = phy_init(pcie->phy);
+   if (err)
+   return err;
+
+   err = phy_power_on(pcie->phy);
+   if (err)
+   return err;
+
+   return rcar_pcie_hw_init(pcie);
+}
+
 static int rcar_msi_alloc(struct rcar_msi *chip)
 {
int msi;
@@ -916,6 +933,10 @@ static int rcar_pcie_get_resources(struc
struct resource res;
int err, i;
 
+   pcie->phy = devm_phy_optional_get(dev, "pcie");
+   if (IS_ERR(pcie->phy))
+   return PTR_ERR(pcie->phy);
+
err = of_address_to_resource(dev->of_node, 0, );
if (err)
return err;
@@ -1068,8 +1089,10 @@ static const struct of_device_id rcar_pc
  .data = rcar_pcie_hw_init_gen2 },
{ .compatible = "renesas,pcie-rcar-gen2",
  .data = rcar_pcie_hw_init_gen2 },
-   { .compatible = "renesas,pcie-r8a7795", .data = rcar_pcie_hw_init },
-   { .compatible = "renesas,pcie-rcar-gen3", .data = rcar_pcie_hw_init },
+   { .compatible = "renesas,pcie-r8a7795",
+ .data = rcar_pcie_hw_init_gen3 },
+   { .compatible = "renesas,pcie-rcar-gen3",
+ .data = rcar_pcie_hw_init_gen3 },
{},
 };
 

[PATCH 2/4] pcie-rcar: remove PHYRDY polling from rcar_pcie_hw_init_h1()

[Sergei Shtylyov]

Now that we've added PCIEPHYSR.PHYRDY polling to rcar_pcie_hw_init(),
there is no need anymore  for polling the PHY specific register in
rcar_pcie_hw_init_h1() -- remove it.

Signed-off-by: Sergei Shtylyov 

---
 drivers/pci/host/pcie-rcar.c |   12 +---
 1 file changed, 1 insertion(+), 11 deletions(-)

Index: pci/drivers/pci/host/pcie-rcar.c
===
--- pci.orig/drivers/pci/host/pcie-rcar.c
+++ pci/drivers/pci/host/pcie-rcar.c
@@ -102,7 +102,6 @@
 #define  LANE_POS  8
 #define  ADR_POS   0
 #define H1_PCIEPHYDOUTR0x040014
-#define H1_PCIEPHYSR   0x040018
 
 /* R-Car Gen2 PHY */
 #define GEN2_PCIEPHYADDR   0x780
@@ -627,8 +626,6 @@ static int rcar_pcie_hw_init(struct rcar
 
 static int rcar_pcie_hw_init_h1(struct rcar_pcie *pcie)
 {
-   unsigned int timeout = 10;
-
/* Initialize the phy */
phy_write_reg(pcie, 0, 0x42, 0x1, 0x0EC34191);
phy_write_reg(pcie, 1, 0x42, 0x1, 0x0EC34180);
@@ -647,14 +644,7 @@ static int rcar_pcie_hw_init_h1(struct r
phy_write_reg(pcie, 0, 0x64, 0x1, 0x3F0F1F0F);
phy_write_reg(pcie, 0, 0x66, 0x1, 0x8000);
 
-   while (timeout--) {
-   if (rcar_pci_read_reg(pcie, H1_PCIEPHYSR))
-   return rcar_pcie_hw_init(pcie);
-
-   msleep(5);
-   }
-
-   return -ETIMEDOUT;
+   return rcar_pcie_hw_init(pcie);
 }
 
 static int rcar_pcie_hw_init_gen2(struct rcar_pcie *pcie)

[PATCH 1/4] pcie-rcar: poll PHYRDY in rcar_pcie_hw_init()

[Sergei Shtylyov]

In  all the R-Car gen1/2/3 manuals, we are instructed to poll PCIEPHYSR
for PHYRDY=1  at  an early stage of the PCIEC initialization -- while
the driver only does this on R-Car H1 (polling a PHY specific register).
Add the PHYRDY polling to rcar_pcie_hw_init(). Note that without the
special PHY driver on the R-Car V3H the PCIEC initialization just freezes
the kernel --  adding the PHYRDY polling allows the init code to exit
gracefully on timeout (PHY starts powered down after reset on this SoC).

Signed-off-by: Sergei Shtylyov 

---
 drivers/pci/host/pcie-rcar.c |   20 
 1 file changed, 20 insertions(+)

Index: pci/drivers/pci/host/pcie-rcar.c
===
--- pci.orig/drivers/pci/host/pcie-rcar.c
+++ pci/drivers/pci/host/pcie-rcar.c
@@ -36,6 +36,8 @@
 #define PCIECDR0x20
 #define PCIEMSR0x28
 #define PCIEINTXR  0x000400
+#define PCIEPHYSR  0x0007f0
+#define  PHYRDY1
 #define PCIEMSITXR 0x000840
 
 /* Transfer control */
@@ -527,6 +529,20 @@ static void phy_write_reg(struct rcar_pc
phy_wait_for_ack(pcie);
 }
 
+static int rcar_pcie_wait_for_phyrdy(struct rcar_pcie *pcie)
+{
+   unsigned int timeout = 10;
+
+   while (timeout--) {
+   if (rcar_pci_read_reg(pcie, PCIEPHYSR) & PHYRDY)
+   return 0;
+
+   msleep(5);
+   }
+
+   return -ETIMEDOUT;
+}
+
 static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
 {
unsigned int timeout = 10;
@@ -551,6 +567,10 @@ static int rcar_pcie_hw_init(struct rcar
/* Set mode */
rcar_pci_write_reg(pcie, 1, PCIEMSR);
 
+   err = rcar_pcie_wait_for_phyrdy(pcie);
+   if (err)
+   return err;
+
/*
 * Initial header for port config space is type 1, set the device
 * class to match. Hardware takes care of propagating the IDSETR

[PATCH 0/4] Add R8A77980 PCIe support & some driver cleanups

[Sergei Shtylyov]

Hello!

Here's a set of 4 patches against the 'pci.rcar' branch of Lorenzo Pieralisi's
'pci.git' repo. These are the changes needed for better R-Car gen3 support
(namely for R8A77980 support) plus some PCIe driver re-factoring done in
the process...

[1/4] pcie-rcar: poll PHYRDY in rcar_pcie_hw_init()
[2/4] pcie-rcar: remove PHYRDY polling from rcar_pcie_hw_init_h1()
[3/4] pcie-rcar: add R-Car gen3 PHY support
[4/4] pcie-rcar: factor out rcar_pcie_hw_init() call

MBR, Sergei

RE: [PATCH] gpio: dwapb: Add support for 32 interrupts

[Phil Edworthy]

Hi Geert,

On 06 April 2018 10:57 Geert Uytterhoeven wrote:
> On Thu, Apr 5, 2018 at 11:42 AM, Phil Edworthy wrote:
> > On 30 March 2018 22:26 Andy Shevchenko wrote:
> >> On Wed, Mar 28, 2018 at 5:22 PM, Phil Edworthy wrote:
> >> > The DesignWare GPIO IP can be configured for either 1 or 32
> >> > interrupts,
> >>
> >> 1 to 32, or just a choice between two?
> > Just a choice of 1 or 32.
> > Note that by 'configured' I am talking about the hardware being
> > configured in RTL prior to manufacturing a device. Once made, you cannot
> change it.
> > This configuration affects the number of output interrupt signals from
> > the GPIO Controller block that are connected to an interrupt controller.
> 
> Differentiating between different versions of an IP block using DT properties
> is usually a bad idea, for several reasons:
>   - What if you discover another difference later?
>   - You cannot add differentiating properties retroactively, because of
> backwards
>  compatibility with old DTBS.
> 
> Hence I think you should introduce a new compatible value instead.

This is not a different version of the IP, just a different configuration 
option.
Most IP blocks have a huge number of knobs that can be twiddled by the HW
people, such as cache size, UART fifo depth. I think this is no different.

Thanks
Phil

Re: [PATCH] gpio: dwapb: Add support for 32 interrupts

[Geert Uytterhoeven]

Hi Phil,

On Thu, Apr 5, 2018 at 11:42 AM, Phil Edworthy
 wrote:
> On 30 March 2018 22:26 Andy Shevchenko wrote:
>> On Wed, Mar 28, 2018 at 5:22 PM, Phil Edworthy wrote:
>> > The DesignWare GPIO IP can be configured for either 1 or 32
>> > interrupts,
>>
>> 1 to 32, or just a choice between two?
> Just a choice of 1 or 32.
> Note that by 'configured' I am talking about the hardware being configured in
> RTL prior to manufacturing a device. Once made, you cannot change it.
> This configuration affects the number of output interrupt signals from the 
> GPIO
> Controller block that are connected to an interrupt controller.

Differentiating between different versions of an IP block using DT properties
is usually a bad idea, for several reasons:
  - What if you discover another difference later?
  - You cannot add differentiating properties retroactively, because
of backwards
 compatibility with old DTBS.

Hence I think you should introduce a new compatible value instead.

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