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

[Rob Herring]

On Thu, Apr 5, 2018 at 1:53 PM, Laurent Pinchart
 wrote:
> Hi Rob,
>
> On Thursday, 5 April 2018 19:33:33 EEST Rob Herring wrote:
>> On Mon, Apr 2, 2018 at 8:36 AM, Laurent Pinchart wrote:
>> > On Tuesday, 27 March 2018 14:03:25 EEST Vladimir Zapolskiy wrote:
>> >> On 03/27/2018 01:10 PM, jacopo mondi wrote:
>> >>> On Tue, Mar 27, 2018 at 12:37:31PM +0300, Vladimir Zapolskiy wrote:
>>  On 03/27/2018 11:57 AM, jacopo mondi wrote:
>> > On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote:
>> >> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote:
>> >>> On 3/27/2018 10:33 AM, jacopo mondi wrote:
>> >>> [...]
>> >>>
>> > Document Thine THC63LVD1024 LVDS decoder device tree
>> > bindings.
>> >
>> > Signed-off-by: Jacopo Mondi 
>> > Reviewed-by: Andrzej Hajda 
>> > Reviewed-by: Niklas Söderlund
>> > 
>> > ---
>> >
>> >   .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++
>> >   1 file changed, 66 insertions(+)
>> >   create mode 100644
>> >
>> > Documentation/devicetree/bindings/display/bridge/thine,thc63l
>> > vd1024.txt
>> > diff --git
>> > a/Documentation/devicetree/bindings/display/bridge/thine,thc6
>> > 3lvd1024.txt
>> > b/Documentation/devicetree/bindings/display/bridge/thine,thc6
>> > 3lvd1024.txt
>> > new file mode 100644
>> > index 000..8225c6a
>> > --- /dev/null
>> > +++
>> > b/Documentation/devicetree/bindings/display/bridge/thine,thc6
>> > 3lvd1024.txt
>> > @@ -0,0 +1,66 @@
>> > +Thine Electronics THC63LVD1024 LVDS decoder
>> > +---
>> > +
>> > +The THC63LVD1024 is a dual link LVDS receiver designed to
>> > convert LVDS streams
>> > +to parallel data outputs. The chip supports single/dual
>> > input/output modes,
>> > +handling up to two two input LVDS stream and up to two
>> > digital CMOS/TTL outputs.
>> > +
>> > +Single or dual operation modes, output data mapping and DDR
>> > output modes are
>> > +configured through input signals and the chip does not
>> > expose any control bus.
>> > +
>> > +Required properties:
>> > +- compatible: Shall be "thine,thc63lvd1024"
>> > +
>> > +Optional properties:
>> > +- vcc-supply: Power supply for TTL output and digital
>> > circuitry
>> > +- cvcc-supply: Power supply for TTL CLOCKOUT signal
>> > +- lvcc-supply: Power supply for LVDS inputs
>> > +- pvcc-supply: Power supply for PLL circuitry
>> 
>>  As explained in a comment to one of the previous versions of
>>  this series, I'm tempted to make vcc-supply mandatory and drop
>>  the three other power supplies for now, as I believe there's
>>  very little chance they will be connected to separately
>>  controllable regulators (all supplies use the same voltage). In
>>  the very unlikely event that this occurs in design we need to
>>  support in the future, the cvcc, lvcc and pvcc supplies can be
>>  added later as optional without breaking backward
>>  compatibility.
>> >>>
>> >>> I'm okay with that.
>> >>>
>>  Apart from that,
>> 
>>  Reviewed-by: Laurent Pinchart
>>  
>> 
>> > +- pdwn-gpios: Power down GPIO signal. Active low
>> >>>
>> >>> powerdown-gpios is the semi-standard name.
>> >>
>> >> right, I've also noticed it. If possible please avoid
>> >> shortenings in property names.
>> >
>> > It is not shortening, it just follow pin name from decoder's
>> > datasheet.
>> >
>> > +- oe-gpios: Output enable GPIO signal. Active high
>> > +
>> >>
>> >> And this one is also a not ever met property name, please
>> >> consider to rename it to 'enable-gpios', for instance display
>> >> panels define it.
>> >
>> > Again, it follows datasheet naming scheme. Has something changed
>> > in DT conventions?
>> 
>>  Seconded. My understanding is that the property name should
>>  reflect what reported in the the chip manual. For THC63LVD1024 the
>>  enable and power down pins are named 'OE' and 

Re: [PATCH v13 16/33] rcar-vin: simplify how formats are set and reset

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

I have incorporated your suggestions for the next and hopefully last 
version of this patch-set, a few followups on your review bellow.

On 2018-04-04 01:09:29 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tuesday, 27 March 2018 00:44:39 EEST Niklas Söderlund wrote:
> > With the recent cleanup of the format code to prepare for Gen3 it's
> > possible to simplify the Gen2 format code path as well. Clean up the
> > process by defining two functions to handle the set format and reset of
> > format when the standard is changed.
> > 
> > While at it replace the driver local struct rvin_source_fmt with a
> > struct v4l2_rect as all it's used for is keep track of the source
> > dimensions.
> 
> I wonder whether we should introduce v4l2_size (or  name 
> here>) when all we need is width and height, as v4l2_rect stores the top and 
> left offsets too. This doesn't have to be fixed here though.

Yes that would have been useful for this change. I have added this to my 
ever growing TODO list :-)

[snip]

> 
> > -   vin->format.bytesperline = rvin_format_bytesperline(>format);
> > -   vin->format.sizeimage = rvin_format_sizeimage(>format);
> > +   vin->source.top = vin->crop.top = 0;
> > +   vin->source.left = vin->crop.left = 0;
> > +   vin->source.width = vin->crop.width = vin->format.width;
> > +   vin->source.height = vin->crop.height = vin->format.height;
> 
> I find multiple assignations on the same line hard to read. How about
> 
>   vin->source.top = 0;
>   vin->source.left = 0;
>   vin->source.width = vin->format.width;
>   vin->source.height = vin->format.height;
> 
>   vin->crop = vin->source;
>   vin->compose = vin->source;

This looks much better and I will use it, thanks !

[snip]

> 
> > +   /*
> > +* If source is ALTERNATE the driver will use the VIN hardware
> > +* to INTERLACE it. The crop height then needs to be doubled.
> > +*/
> > +   if (pix->field == V4L2_FIELD_ALTERNATE)
> > +   crop->height *= 2;
> 
> You're duplicating this logic in rvin_format_align() so it makes me feel that 
> there's still room for some simplification, but that can be done in a 
> separate 
> patch (or I could simply be wrong).

I'm sure someone more clever then me can simplify this more. In this 
particular case rvin_format_align() deals with the video device format 
while here we deal with the crop rectangle. I will keep this in mind for 
future work and see if this can be unified in simpler way.

[snip]

> > -   return __rvin_try_format(vin, V4L2_SUBDEV_FORMAT_TRY, 
> > >fmt.pix,
> > -);
> > +   return rvin_try_format(vin, V4L2_SUBDEV_FORMAT_TRY, >fmt.pix, ,
> > +  );
> 
> How about making crop and compose optional in rvin_try_format() to avoid a 
> need for the two local variables here ?

Great suggestion, I have incorporated this for the next version.

> 
> Apart from these small issues, I think this is a nice simplification,
> 
> Reviewed-by: Laurent Pinchart 

Thanks!

-- 
Regards,
Niklas Söderlund

Re: [PATCH v13 00/33] rcar-vin: Add Gen3 with media controller

[Niklas Söderlund]

Hi Hans,

On 2018-04-04 12:28:30 +0200, Hans Verkuil wrote:
> Hi Niklas,
> 
> It might be a good idea if you can rebase the patch series on the latest
> master (we've just synced to Linus' master tree) and incorporate the few
> comments that Laurent had.

I have now rebased this and incorporate the comments from Laurent. I 
still need to test the rebased work so will post this tomorrow or early 
next week.

> 
> Then once the merge window closes I can make the pull request, probably on
> the 16th.

Thanks for that and thanks again for all your help in my pursuit to get 
this series accepted!

> 
> Regards,
> 
>   Hans
> 
> On 03/04/18 14:30, Hans Verkuil wrote:
> > On 26/03/18 23:44, Niklas Söderlund wrote:
> >> Hi,
> >>
> >> This series adds Gen3 VIN support to rcar-vin driver for Renesas r8a7795,
> >> r8a7796 and r8a77970. It is based on the media-tree and depends on
> >> Fabrizio Castro patches as they touches the order of the compatible
> >> strings in the documentation to reduce merge conflicts. The dependencies
> >> are included in this series.
> > 
> > Laurent, Kieran,
> > 
> > Unless there are any objections I want to make a pull request for this
> > series once 4.17-rc1 has been merged back into our master tree. It all
> > looks good to me, and it will be nice to get this in (finally!).
> > 
> > I don't want to postpone the pull request for small improvements, they
> > can be applied later. But if there are more serious concerns, then let
> > us know.
> > 
> > Regards,
> > 
> > Hans
> > 
> >>
> >> The driver is tested on Renesas H3 (r8a7795, ES2.0),
> >> M3-W (r8a7796) together with the rcar-csi2 driver (posted separately and
> >> not yet upstream) and the Salvator-X onboard ADV7482. It is also tested
> >> on the V3M (r8a77970) on the Eagle board together with its expansion
> >> board with a ADV7482 and out of tree patches for GMSL capture using the
> >> max9286 and rdacm20 drivers.
> >>
> >> It is possible to capture both CVBS and HDMI video streams,
> >> v4l2-compliance passes with no errors and media-ctl can be used to
> >> change the routing and formats for the different entities in the media
> >> graph.
> >>
> >> Gen2 compatibility is verified on Koelsch and no problems where found,
> >> video can be captured just like before and v4l2-compliance passes
> >> without errors or warnings just like before this series.
> >>
> >> For convenience the series can be fetched from:
> >>
> >>   git://git.ragnatech.se/linux rcar/vin/mc-v13
> >>
> >> I have started on a very basic test suite for the VIN driver at:
> >>
> >>   https://git.ragnatech.se/vin-tests
> >>
> >> And as before the state of the driver and information about how to test
> >> it can be found on the elinux wiki:
> >>
> >>   http://elinux.org/R-Car/Tests:rcar-vin
> >>
> >> * Changes from v12
> >> - Rebase to latest media-tree/master changed a 'return ret' to a 'goto
> >>   out' in rvin_start_streaming() to take recent changes to the VIN 
> >>   driver into account.
> >> - Moved field != V4L2_FIELD_ANY in 'rcar-vin: set a default field to  
> >>   fallback on' check from a later commit 'rcar-vin: simplify how formats 
> >>   are set and reset' in the series. This is to avoid ignoring the field 
> >>   returned from the sensor if FIELD_ANY was requested by the user. This 
> >>   was only a problem between this change and a few patches later, but 
> >>   better to fix it now. Reported by Hans, thanks for spotting this.
> >> - Fix spelling.
> >> - Add review tags from Hans.
> >>
> >> * Changes since v11
> >> - Rewrote commit message for '[PATCH v11 22/32] rcar-vin: force default
> >>   colorspace for media centric mode'. Also set fixed values for
> >>   xfer_func, quantization and ycbcr_enc.
> >> - Reorderd filed order in struct rvin_group_route.
> >> - Renamed chan to channel in struct rvin_group_route.
> >> - Rework 'rcar-vin: read subdevice format for crop only when
> >>   needed' into 'rcar-vin: simplify how formats are set and reset'.
> >> - Keep caching the source dimensions and drop all changes to
> >>   rvin_g_selection() and rvin_s_selection().
> >> - Inline rvin_get_vin_format_from_source() into rvin_reset_format()
> >>   which now is the only user left.
> >> - Add patch to cache the video standard instead of reading it at stream
> >>   on.
> >> - Fix error labels in rvin_mc_open().
> >> - Fixed spelling in commit messages and comment, thanks Laurent!
> >> - Added reviewed tags from Laurent, Thanks!
> >>
> >> * Changes since v10
> >> - Corrected spelling in comments and commit messages.
> >> - Reworked 'rcar-vin: read subdevice format for crop only when needed'
> >>   to only get the source format once per operation.
> >> - Moved some patches around to make it easier to review, moved:
> >> - rcar-vin: set a default field to fallback on
> >> - rcar-vin: fix handling of single field frames (top, bottom and 
> >> alternate fields)
> >> - rcar-vin: update bytesperline and sizeimage calculation
> >> - rcar-vin: 

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

[Laurent Pinchart]

Hi Rob,

On Thursday, 5 April 2018 19:33:33 EEST Rob Herring wrote:
> On Mon, Apr 2, 2018 at 8:36 AM, Laurent Pinchart wrote:
> > On Tuesday, 27 March 2018 14:03:25 EEST Vladimir Zapolskiy wrote:
> >> On 03/27/2018 01:10 PM, jacopo mondi wrote:
> >>> On Tue, Mar 27, 2018 at 12:37:31PM +0300, Vladimir Zapolskiy wrote:
>  On 03/27/2018 11:57 AM, jacopo mondi wrote:
> > On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote:
> >> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote:
> >>> On 3/27/2018 10:33 AM, jacopo mondi wrote:
> >>> [...]
> >>> 
> > Document Thine THC63LVD1024 LVDS decoder device tree
> > bindings.
> > 
> > Signed-off-by: Jacopo Mondi 
> > Reviewed-by: Andrzej Hajda 
> > Reviewed-by: Niklas Söderlund
> > 
> > ---
> > 
> >   .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++
> >   1 file changed, 66 insertions(+)
> >   create mode 100644
> > 
> > Documentation/devicetree/bindings/display/bridge/thine,thc63l
> > vd1024.txt
> > diff --git
> > a/Documentation/devicetree/bindings/display/bridge/thine,thc6
> > 3lvd1024.txt
> > b/Documentation/devicetree/bindings/display/bridge/thine,thc6
> > 3lvd1024.txt
> > new file mode 100644
> > index 000..8225c6a
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/display/bridge/thine,thc6
> > 3lvd1024.txt
> > @@ -0,0 +1,66 @@
> > +Thine Electronics THC63LVD1024 LVDS decoder
> > +---
> > +
> > +The THC63LVD1024 is a dual link LVDS receiver designed to
> > convert LVDS streams
> > +to parallel data outputs. The chip supports single/dual
> > input/output modes,
> > +handling up to two two input LVDS stream and up to two
> > digital CMOS/TTL outputs.
> > +
> > +Single or dual operation modes, output data mapping and DDR
> > output modes are
> > +configured through input signals and the chip does not
> > expose any control bus.
> > +
> > +Required properties:
> > +- compatible: Shall be "thine,thc63lvd1024"
> > +
> > +Optional properties:
> > +- vcc-supply: Power supply for TTL output and digital
> > circuitry
> > +- cvcc-supply: Power supply for TTL CLOCKOUT signal
> > +- lvcc-supply: Power supply for LVDS inputs
> > +- pvcc-supply: Power supply for PLL circuitry
>  
>  As explained in a comment to one of the previous versions of
>  this series, I'm tempted to make vcc-supply mandatory and drop
>  the three other power supplies for now, as I believe there's
>  very little chance they will be connected to separately
>  controllable regulators (all supplies use the same voltage). In
>  the very unlikely event that this occurs in design we need to
>  support in the future, the cvcc, lvcc and pvcc supplies can be
>  added later as optional without breaking backward
>  compatibility.
> >>> 
> >>> I'm okay with that.
> >>> 
>  Apart from that,
>  
>  Reviewed-by: Laurent Pinchart
>  
>  
> > +- pdwn-gpios: Power down GPIO signal. Active low
> >>> 
> >>> powerdown-gpios is the semi-standard name.
> >> 
> >> right, I've also noticed it. If possible please avoid
> >> shortenings in property names.
> > 
> > It is not shortening, it just follow pin name from decoder's
> > datasheet.
> > 
> > +- oe-gpios: Output enable GPIO signal. Active high
> > +
> >> 
> >> And this one is also a not ever met property name, please
> >> consider to rename it to 'enable-gpios', for instance display
> >> panels define it.
> > 
> > Again, it follows datasheet naming scheme. Has something changed
> > in DT conventions?
>  
>  Seconded. My understanding is that the property name should
>  reflect what reported in the the chip manual. For THC63LVD1024 the
>  enable and power down pins are named 'OE' and 'PDWN' respectively.
> >>> 
> >>> But don't we need the vendor prefix in the prop names then, like
> >>> "renesas,oe-gpios" then?
> >> 
> >> Seconded, with a 

[PATCH] pwm: rcar: simplify getting .drvdata

[Wolfram Sang]

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...

 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

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

[Wolfram Sang]

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?

Happy hacking,

   Wolfram


 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c 
b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index 561e90755a3b..19ab88b348c1 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -269,12 +269,15 @@ static const struct tmio_mmc_dma_ops 
renesas_sdhi_internal_dmac_dma_ops = {
  * implementation as others may use a different implementation.
  */
 static const struct soc_device_attribute gen3_soc_whitelist[] = {
+   /* specific ones */
 { .soc_id = "r8a7795", .revision = "ES1.*",
  .data = (void *)BIT(SDHI_INTERNAL_DMAC_ONE_RX_ONLY) },
-{ .soc_id = "r8a7795", .revision = "ES2.0" },
 { .soc_id = "r8a7796", .revision = "ES1.0",
  .data = (void *)BIT(SDHI_INTERNAL_DMAC_ONE_RX_ONLY) },
-{ .soc_id = "r8a77995", .revision = "ES1.0" },
+   /* generic ones */
+{ .soc_id = "r8a7795" },
+{ .soc_id = "r8a7796" },
+{ .soc_id = "r8a77995" },
 { /* sentinel */ }
 };
 
-- 
2.11.0

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

[Rob Herring]

On Mon, Apr 2, 2018 at 8:36 AM, Laurent Pinchart
 wrote:
> Hi Vladimir,
>
> On Tuesday, 27 March 2018 14:03:25 EEST Vladimir Zapolskiy wrote:
>> On 03/27/2018 01:10 PM, jacopo mondi wrote:
>> > On Tue, Mar 27, 2018 at 12:37:31PM +0300, Vladimir Zapolskiy wrote:
>> >> On 03/27/2018 11:57 AM, jacopo mondi wrote:
>> >>> On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote:
>>  On 03/27/2018 11:27 AM, Sergei Shtylyov wrote:
>> > On 3/27/2018 10:33 AM, jacopo mondi wrote:
>> > [...]
>> >
>> >>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>> >>>
>> >>> Signed-off-by: Jacopo Mondi 
>> >>> Reviewed-by: Andrzej Hajda 
>> >>> Reviewed-by: Niklas Söderlund
>> >>> 
>> >>> ---
>> >>>
>> >>>   .../bindings/display/bridge/thine,thc63lvd1024.txt | 66
>> >>>   +++
>> >>>   1 file changed, 66 insertions(+)
>> >>>   create mode 100644
>> >>>
>> >>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1
>> >>> 024.txt
>> >>>
>> >>> diff --git
>> >>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lv
>> >>> d1024.txt
>> >>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lv
>> >>> d1024.txt
>> >>> new file mode 100644
>> >>> index 000..8225c6a
>> >>> --- /dev/null
>> >>> +++
>> >>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lv
>> >>> d1024.txt
>> >>> @@ -0,0 +1,66 @@
>> >>> +Thine Electronics THC63LVD1024 LVDS decoder
>> >>> +---
>> >>> +
>> >>> +The THC63LVD1024 is a dual link LVDS receiver designed to
>> >>> convert LVDS streams
>> >>> +to parallel data outputs. The chip supports single/dual
>> >>> input/output modes, +handling up to two two input LVDS stream
>> >>> and up to two digital CMOS/TTL outputs.
>> >>> +
>> >>> +Single or dual operation modes, output data mapping and DDR
>> >>> output modes are
>> >>> +configured through input signals and the chip does not expose
>> >>> any control bus.
>> >>> +
>> >>> +Required properties:
>> >>> +- compatible: Shall be "thine,thc63lvd1024"
>> >>> +
>> >>> +Optional properties:
>> >>> +- vcc-supply: Power supply for TTL output and digital circuitry
>> >>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
>> >>> +- lvcc-supply: Power supply for LVDS inputs
>> >>> +- pvcc-supply: Power supply for PLL circuitry
>> >>
>> >> As explained in a comment to one of the previous versions of this
>> >> series, I'm tempted to make vcc-supply mandatory and drop the
>> >> three other power supplies for now, as I believe there's very
>> >> little chance they will be connected to separately controllable
>> >> regulators (all supplies use the same voltage). In the very
>> >> unlikely event that this occurs in design we need to support in
>> >> the future, the cvcc, lvcc and pvcc supplies can be added later
>> >> as optional without breaking backward compatibility.
>> >
>> > I'm okay with that.
>> >
>> >> Apart from that,
>> >>
>> >> Reviewed-by: Laurent Pinchart 
>> >>
>> >>> +- pdwn-gpios: Power down GPIO signal. Active low
>> >
>> > powerdown-gpios is the semi-standard name.
>> 
>>  right, I've also noticed it. If possible please avoid shortenings
>>  in property names.
>> >>>
>> >>> It is not shortening, it just follow pin name from decoder's
>> >>> datasheet.
>> >>>
>> >>> +- oe-gpios: Output enable GPIO signal. Active high
>> >>> +
>> 
>>  And this one is also a not ever met property name, please consider
>>  to rename it to 'enable-gpios', for instance display panels define
>>  it.
>> >>>
>> >>> Again, it follows datasheet naming scheme. Has something changed in
>> >>> DT conventions?
>> >>
>> >> Seconded. My understanding is that the property name should reflect
>> >> what reported in the the chip manual. For THC63LVD1024 the enable and
>> >> power down pins are named 'OE' and 'PDWN' respectively.
>> >>
>> > But don't we need the vendor prefix in the prop names then, like
>> > "renesas,oe-gpios" then?
>> 
>>  Seconded, with a correction to "thine,oe-gpios".
>> >>>
>> >>> mmm, okay then...
>> >>>
>> >>> A grep for that semi-standard properties names in Documentation/
>> >>> returns only usage 

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

[Wolfram Sang]

> > -   /* This DMAC cannot handle if sg_len is not 1 */
> > -   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))
> 
> The WARN_ON becomes a bit misleading being a part of the if statement,
> as it should never happen when the driver has set ->max_segs = 1.

Uh, yeah, right. So, we could simply drop the WARN_ON?



signature.asc
Description: PGP signature

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

[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.
> 
> I assume this is material for stable as well, right?

Yes. But I'd like to get Shimoda-san's confirmation first.

> > +static unsigned long global_flags = 0;
> 
> There's no need to initialize static variables to zero.

Yup, classic one :)

Thanks,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[jacopo mondi]

A few corrections,

On Thu, Apr 05, 2018 at 11:10:01AM +0200, jacopo mondi wrote:
> Hi Niklas,
> thanks for the VIN and CSI-2 effort!
>
> On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote:
> > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
> > connected between the video sources and the video grabbers (VIN).
> >
> > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
> >
> > Signed-off-by: Niklas Söderlund 
> > Reviewed-by: Hans Verkuil 
> > ---
> >  drivers/media/platform/rcar-vin/Kconfig |  12 +
> >  drivers/media/platform/rcar-vin/Makefile|   1 +
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 884 
> > 
> >  3 files changed, 897 insertions(+)
> >  create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c
> >
>
> [snip]
>
> > +
> > +static const struct rcar_csi2_format rcar_csi2_formats[] = {
> > +   { .code = MEDIA_BUS_FMT_RGB888_1X24,.datatype = 0x24, .bpp = 24 },
> > +   { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 },
> > +   { .code = MEDIA_BUS_FMT_UYVY8_2X8,  .datatype = 0x1e, .bpp = 16 },
> > +   { .code = MEDIA_BUS_FMT_YUYV10_2X10,.datatype = 0x1e, .bpp = 16 },
>
> Shouldn't YUYV10_2X10 format have 20 bits per pixel?
>
> > +};
> > +
> > +static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int 
> > code)
> > +{
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> > +   if (rcar_csi2_formats[i].code == code)
> > +   return rcar_csi2_formats + i;
> > +
> > +   return NULL;
> > +}
> > +
> > +enum rcar_csi2_pads {
> > +   RCAR_CSI2_SINK,
> > +   RCAR_CSI2_SOURCE_VC0,
> > +   RCAR_CSI2_SOURCE_VC1,
> > +   RCAR_CSI2_SOURCE_VC2,
> > +   RCAR_CSI2_SOURCE_VC3,
> > +   NR_OF_RCAR_CSI2_PAD,
> > +};
> > +
> > +struct rcar_csi2_info {
> > +   const struct phypll_hsfreqrange *hsfreqrange;
> > +   unsigned int csi0clkfreqrange;
> > +   bool clear_ulps;
> > +   bool init_phtw;
> > +};
> > +
> > +struct rcar_csi2 {
> > +   struct device *dev;
> > +   void __iomem *base;
> > +   const struct rcar_csi2_info *info;
> > +
> > +   struct v4l2_subdev subdev;
> > +   struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
> > +
> > +   struct v4l2_async_notifier notifier;
> > +   struct v4l2_async_subdev asd;
> > +   struct v4l2_subdev *remote;
> > +
> > +   struct v4l2_mbus_framefmt mf;
> > +
> > +   struct mutex lock;
> > +   int stream_count;
> > +
> > +   unsigned short lanes;
> > +   unsigned char lane_swap[4];
> > +};
> > +
> > +static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> > +{
> > +   return container_of(sd, struct rcar_csi2, subdev);
> > +}
> > +
> > +static inline struct rcar_csi2 *notifier_to_csi2(struct 
> > v4l2_async_notifier *n)
> > +{
> > +   return container_of(n, struct rcar_csi2, notifier);
> > +}
> > +
> > +static u32 rcar_csi2_read(struct rcar_csi2 *priv, unsigned int reg)
> > +{
> > +   return ioread32(priv->base + reg);
> > +}
> > +
> > +static void rcar_csi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 
> > data)
> > +{
> > +   iowrite32(data, priv->base + reg);
> > +}
> > +
> > +static void rcar_csi2_reset(struct rcar_csi2 *priv)
> > +{
> > +   rcar_csi2_write(priv, SRST_REG, SRST_SRST);
> > +   usleep_range(100, 150);
> > +   rcar_csi2_write(priv, SRST_REG, 0);
> > +}
> > +
> > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
> > +{
> > +   int timeout;
> > +
> > +   /* Wait for the clock and data lanes to enter LP-11 state. */
> > +   for (timeout = 100; timeout > 0; timeout--) {
> > +   const u32 lane_mask = (1 << priv->lanes) - 1;
> > +
> > +   if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 &&
>
> Nitpicking:
>   if ((rcar_csi2_read(priv, PHCLM_REG) & 0x01) &&
>
> Don't you prefer to provide defines also for bit fields instead of
> using magic values? In this case something like
> PHCLM_REG_STOPSTATE_CLK would do.
>
> Also, from tables 25.[17-20] it seems to me that for H3 and V3 you
> have to set INSTATE to an hardcoded value after having validated PHDLM.
> Maybe it is not necessary, just pointing it out.
>
> > +   (rcar_csi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> > +   return 0;
> > +
> > +   msleep(20);
> > +   }
> > +
> > +   dev_err(priv->dev, "Timeout waiting for LP-11 state\n");
> > +
> > +   return -ETIMEDOUT;
> > +}
> > +
> > +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv, unsigned int bpp,
> > +u32 *phypll)
> > +{
> > +   const struct phypll_hsfreqrange *hsfreq;
> > +   struct v4l2_subdev *source;
> > +   struct v4l2_ctrl *ctrl;
> > +   u64 mbps;
> > +
> > +   if (!priv->remote)
> > +   return -ENODEV;
> > +
> > +   source = priv->remote;
> > +
> > +   /* Read the pixel rate control from 

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

[Phil Edworthy]

Hi Andy,

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.

> > but the driver currently only supports 1 interrupt. See the DesignWare
> > DW_apb_gpio Databook description of the 'GPIO_INTR_IO' parameter.
> 
> Will see after holiday and perhaps make more comments. Here is just a brief
> review.
> 
> > +- interrupts : The interrupts to the parent controller raised when
> > +GPIOs
> > +  generate the interrupts. If the controller provides one combined
> > +interrupt
> > +  for all GPIOs, specify a single interrupt. If the controller
> > +provides one
> > +  interrupt for each GPIO, provide a list of interrupts that
> > +correspond to each
> > +  of the GPIO pins. When specifying multiple interrupts, if any of
> > +the GPIOs are
> > +  not connected to an interrupt, use the interrupt-mask property.
> > +- interrupt-mask : a 32-bit bit mask that specifies which interrupts
> > +in the list
> > +  of interrupts is valid, bit is 1 for a valid irq.
> 
> So, but why one will need that in practice? GPIO driver usually provides a pin
> based IRQ chip which maps each pin to the corresponding offset inside
> specific IRQ domain.
On an ARM device we have this GPIO block connected to the GIC interrupt
controller, i.e. the Synopsys GPIO controller interrupts can* have a 1 to 1
mapping to the GIC interrupts. At the moment, the GPIO driver only allows a
single irq signal to specified.
* this is not strictly accurate on the device I am working on, there is another
block of IP between the two, but that doesn't matter in this case.

> > +   struct device_node *np = to_of_node(fwnode);
> > +   u32 irq_mask = 0x;
> 
> Why? Shouldn't it be dependent to the amount of actual pins / ports?
> Intel Quark has only 8 AFAIR.
It's just a default which can be overridden via device tree.
For Quark, since you currently only use a single irq, I guess the HW was
configured that way. In which case, you wouldn't use any of this.

> > +   int j;
> > +
> > +   /* Optional irq mask */
> > +   fwnode_property_read_u32(fwnode,
> > + "interrupt-mask", _mask);
> > +
> > +   /*
> > +* The IP has configuration options to allow a 
> > single
> > +* combined interrupt or one per gpio. If one per 
> > gpio,
> > +* some might not be used.
> > +*/
> 
> > +   for (j = 0; j < pp->ngpio; j++) {
> > +   if (irq_mask & BIT(j)) {
> 
> for_each_set_bit() is in kernel for ages!
There's lot of stuff in the kernel for ages that I can't remember!
I'll fix this :)

> > +   pp->irq[j] = 
> > irq_of_parse_and_map(np, j);
> > +   if (pp->irq[j])
> > +   pp->has_irq = true;
> > +   }
> > +   }
> 
> 
> So, on the first glance the patch looks either superfluous or taking wrong
> approach. Please, elaborate more why it's done in this way and what the
> case for all this in practice.
Hopefully I have explained it a bit better above.

Thanks for your comments
Phil

Re: [PATCH] dt-bindings: irqchip: renesas-irqc: Document r8a77470 support

[Marc Zyngier]

On 29/03/18 11:17, Biju Das wrote:
> Renesas RZ/G SoC have the R-Car gen2 compatible IRQC interrupt
> controllers. Document RZ/G1C (also known as R8A77470) SoC bindings.
> 
> Signed-off-by: Biju Das 
> Reviewed-by: Fabrizio Castro 
> ---
>  Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git 
> a/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt 
> b/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt
> index 20f121d..1d5891a 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,irqc.txt
> @@ -7,6 +7,7 @@ Required properties:
>  - "renesas,irqc-r8a73a4" (R-Mobile APE6)
>  - "renesas,irqc-r8a7743" (RZ/G1M)
>  - "renesas,irqc-r8a7745" (RZ/G1E)
> +- "renesas,irqc-r8a77470" (RZ/G1C)
>  - "renesas,irqc-r8a7790" (R-Car H2)
>  - "renesas,irqc-r8a7791" (R-Car M2-W)
>  - "renesas,irqc-r8a7792" (R-Car V2H)
> 

Queued with Simon and Geert's RB tags.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v2 15/15] v4l: vsp1: Rename BRU to BRx

[Laurent Pinchart]

Some VSP instances have two blending units named BRU (Blend/ROP Unit)
and BRS (Blend/ROP Sub unit). The BRS is a smaller version of the BRU
with only two inputs, but otherwise offers similar features and offers
the same register interface. The BRU and BRS can be used exchangeably in
VSP pipelines (provided no more than two inputs are needed).

Due to historical reasons, the VSP1 driver implements support for both
the BRU and BRS through objects named vsp1_bru. The code uses the name
BRU to refer to either the BRU or the BRS, except in a few places where
noted explicitly. This creates confusion.

In an effort to avoid confusion, rename the vsp1_bru object and the
corresponding API to vsp1_brx, and use BRx to refer to blend unit
instances regardless of their type. The names BRU and BRS are retained
where reference to a particular blend unit type is needed, as well as in
hardware registers to stay close to the datasheet.

Signed-off-by: Laurent Pinchart 
Acked-by: Kieran Bingham 
---
 drivers/media/platform/vsp1/Makefile   |   2 +-
 drivers/media/platform/vsp1/vsp1.h |   6 +-
 .../media/platform/vsp1/{vsp1_bru.c => vsp1_brx.c} | 202 ++---
 .../media/platform/vsp1/{vsp1_bru.h => vsp1_brx.h} |  18 +-
 drivers/media/platform/vsp1/vsp1_drm.c | 174 +-
 drivers/media/platform/vsp1/vsp1_drm.h |   6 +-
 drivers/media/platform/vsp1/vsp1_drv.c |   6 +-
 drivers/media/platform/vsp1/vsp1_pipe.c|  12 +-
 drivers/media/platform/vsp1/vsp1_pipe.h|   4 +-
 drivers/media/platform/vsp1/vsp1_rpf.c |  12 +-
 drivers/media/platform/vsp1/vsp1_rwpf.h|   2 +-
 drivers/media/platform/vsp1/vsp1_video.c   |  16 +-
 drivers/media/platform/vsp1/vsp1_wpf.c |   8 +-
 13 files changed, 234 insertions(+), 234 deletions(-)
 rename drivers/media/platform/vsp1/{vsp1_bru.c => vsp1_brx.c} (63%)
 rename drivers/media/platform/vsp1/{vsp1_bru.h => vsp1_brx.h} (66%)

diff --git a/drivers/media/platform/vsp1/Makefile 
b/drivers/media/platform/vsp1/Makefile
index f5cd6f0491cb..596775f932c0 100644
--- a/drivers/media/platform/vsp1/Makefile
+++ b/drivers/media/platform/vsp1/Makefile
@@ -3,7 +3,7 @@ vsp1-y  := vsp1_drv.o 
vsp1_entity.o vsp1_pipe.o
 vsp1-y += vsp1_dl.o vsp1_drm.o vsp1_video.o
 vsp1-y += vsp1_rpf.o vsp1_rwpf.o vsp1_wpf.o
 vsp1-y += vsp1_clu.o vsp1_hsit.o vsp1_lut.o
-vsp1-y += vsp1_bru.o vsp1_sru.o vsp1_uds.o
+vsp1-y += vsp1_brx.o vsp1_sru.o vsp1_uds.o
 vsp1-y += vsp1_hgo.o vsp1_hgt.o vsp1_histo.o
 vsp1-y += vsp1_lif.o
 
diff --git a/drivers/media/platform/vsp1/vsp1.h 
b/drivers/media/platform/vsp1/vsp1.h
index 78ef838416b3..894cc725c2d4 100644
--- a/drivers/media/platform/vsp1/vsp1.h
+++ b/drivers/media/platform/vsp1/vsp1.h
@@ -30,7 +30,7 @@ struct rcar_fcp_device;
 struct vsp1_drm;
 struct vsp1_entity;
 struct vsp1_platform_data;
-struct vsp1_bru;
+struct vsp1_brx;
 struct vsp1_clu;
 struct vsp1_hgo;
 struct vsp1_hgt;
@@ -78,8 +78,8 @@ struct vsp1_device {
struct rcar_fcp_device *fcp;
struct device *bus_master;
 
-   struct vsp1_bru *brs;
-   struct vsp1_bru *bru;
+   struct vsp1_brx *brs;
+   struct vsp1_brx *bru;
struct vsp1_clu *clu;
struct vsp1_hgo *hgo;
struct vsp1_hgt *hgt;
diff --git a/drivers/media/platform/vsp1/vsp1_bru.c 
b/drivers/media/platform/vsp1/vsp1_brx.c
similarity index 63%
rename from drivers/media/platform/vsp1/vsp1_bru.c
rename to drivers/media/platform/vsp1/vsp1_brx.c
index e8fd2ae3b3eb..b4af1d546022 100644
--- a/drivers/media/platform/vsp1/vsp1_bru.c
+++ b/drivers/media/platform/vsp1/vsp1_brx.c
@@ -1,5 +1,5 @@
 /*
- * vsp1_bru.c  --  R-Car VSP1 Blend ROP Unit
+ * vsp1_brx.c  --  R-Car VSP1 Blend ROP Unit (BRU and BRS)
  *
  * Copyright (C) 2013 Renesas Corporation
  *
@@ -17,45 +17,45 @@
 #include 
 
 #include "vsp1.h"
-#include "vsp1_bru.h"
+#include "vsp1_brx.h"
 #include "vsp1_dl.h"
 #include "vsp1_pipe.h"
 #include "vsp1_rwpf.h"
 #include "vsp1_video.h"
 
-#define BRU_MIN_SIZE   1U
-#define BRU_MAX_SIZE   8190U
+#define BRX_MIN_SIZE   1U
+#define BRX_MAX_SIZE   8190U
 
 /* 
-
  * Device Access
  */
 
-static inline void vsp1_bru_write(struct vsp1_bru *bru, struct vsp1_dl_list 
*dl,
+static inline void vsp1_brx_write(struct vsp1_brx *brx, struct vsp1_dl_list 
*dl,
  u32 reg, u32 data)
 {
-   vsp1_dl_list_write(dl, bru->base + reg, data);
+   

[PATCH v2 14/15] v4l: vsp1: Add BRx dynamic assignment debugging messages

[Laurent Pinchart]

Dynamic assignment of the BRU and BRS to pipelines is prone to
regressions, add messages to make debugging easier. Keep it as a
separate commit to ease removal of those messages once the code will
deem to be completely stable.

Signed-off-by: Laurent Pinchart 
Acked-by: Kieran Bingham 
---
 drivers/media/platform/vsp1/vsp1_drm.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index f82f83b6d4ff..b43d6dc0d5f5 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -190,6 +190,10 @@ static int vsp1_du_pipeline_setup_bru(struct vsp1_device 
*vsp1,
 
/* Release our BRU if we have one. */
if (pipe->bru) {
+   dev_dbg(vsp1->dev, "%s: pipe %u: releasing %s\n",
+   __func__, pipe->lif->index,
+   BRU_NAME(pipe->bru));
+
/*
 * The BRU might be acquired by the other pipeline in
 * the next step. We must thus remove it from the list
@@ -219,6 +223,9 @@ static int vsp1_du_pipeline_setup_bru(struct vsp1_device 
*vsp1,
if (bru->pipe) {
struct vsp1_drm_pipeline *owner_pipe;
 
+   dev_dbg(vsp1->dev, "%s: pipe %u: waiting for %s\n",
+   __func__, pipe->lif->index, BRU_NAME(bru));
+
owner_pipe = to_vsp1_drm_pipeline(bru->pipe);
owner_pipe->force_bru_release = true;
 
@@ -245,6 +252,9 @@ static int vsp1_du_pipeline_setup_bru(struct vsp1_device 
*vsp1,
  >entities);
 
/* Add the BRU to the pipeline. */
+   dev_dbg(vsp1->dev, "%s: pipe %u: acquired %s\n",
+   __func__, pipe->lif->index, BRU_NAME(bru));
+
pipe->bru = bru;
pipe->bru->pipe = pipe;
pipe->bru->sink = >output->entity;
@@ -548,6 +558,10 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int 
pipe_index,
drm_pipe->du_complete = NULL;
pipe->num_inputs = 0;
 
+   dev_dbg(vsp1->dev, "%s: pipe %u: releasing %s\n",
+   __func__, pipe->lif->index,
+   BRU_NAME(pipe->bru));
+
list_del(>bru->list_pipe);
pipe->bru->pipe = NULL;
pipe->bru = NULL;
-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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.

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_COMPLETEDBIT(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,
+ completion & VSP1_DL_FRAME_END_COMPLETED);
 }
 
 /* 
-
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c 
b/drivers/media/platform/vsp1/vsp1_pipe.c
index 99ccbac3256a..1134f14ed4aa 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.c
+++ 

[PATCH v2 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 
---
 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..7bf697ba7969 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_input(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 i;
+   int ret;
+
+   /* Count the number of enabled inputs and sort them by Z-order. */
+ 

[PATCH v2 02/15] v4l: vsp1: Remove unused field from vsp1_drm_pipeline structure

[Laurent Pinchart]

The vsp1_drm_pipeline enabled field is set but never used. Remove it.

Signed-off-by: Laurent Pinchart 
Reviewed-by: Kieran Bingham 
---
 drivers/media/platform/vsp1/vsp1_drm.c | 4 
 drivers/media/platform/vsp1/vsp1_drm.h | 2 --
 2 files changed, 6 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index a1f2ba044092..a267f12f0cc8 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -273,10 +273,6 @@ EXPORT_SYMBOL_GPL(vsp1_du_setup_lif);
  */
 void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index)
 {
-   struct vsp1_device *vsp1 = dev_get_drvdata(dev);
-   struct vsp1_drm_pipeline *drm_pipe = >drm->pipe[pipe_index];
-
-   drm_pipe->enabled = drm_pipe->pipe.num_inputs != 0;
 }
 EXPORT_SYMBOL_GPL(vsp1_du_atomic_begin);
 
diff --git a/drivers/media/platform/vsp1/vsp1_drm.h 
b/drivers/media/platform/vsp1/vsp1_drm.h
index 1cd9db785bf7..9aa19325cbe9 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.h
+++ b/drivers/media/platform/vsp1/vsp1_drm.h
@@ -20,13 +20,11 @@
 /**
  * vsp1_drm_pipeline - State for the API exposed to the DRM driver
  * @pipe: the VSP1 pipeline used for display
- * @enabled: pipeline state at the beginning of an update
  * @du_complete: frame completion callback for the DU driver (optional)
  * @du_private: data to be passed to the du_complete callback
  */
 struct vsp1_drm_pipeline {
struct vsp1_pipeline pipe;
-   bool enabled;
 
/* Frame synchronisation */
void (*du_complete)(void *, bool);
-- 
Regards,

Laurent Pinchart

[PATCH v2 04/15] v4l: vsp1: Use vsp1_entity.pipe to check if entity belongs to a pipeline

[Laurent Pinchart]

The DRM pipeline handling code uses the entity's pipe list head to check
whether the entity is already included in a pipeline. This method is a
bit fragile in the sense that it uses list_empty() on a list_head that
is a list member. Replace it by a simpler check for the entity pipe
pointer.

Signed-off-by: Laurent Pinchart 
Reviewed-by: Kieran Bingham 
---
 drivers/media/platform/vsp1/vsp1_drm.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index a7ad85ab0b08..e210917fdc3f 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -119,9 +119,9 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int 
pipe_index,
 * Remove the RPF from the pipe and the list of BRU
 * inputs.
 */
-   WARN_ON(list_empty(>entity.list_pipe));
+   WARN_ON(!rpf->entity.pipe);
rpf->entity.pipe = NULL;
-   list_del_init(>entity.list_pipe);
+   list_del(>entity.list_pipe);
pipe->inputs[i] = NULL;
 
bru->inputs[rpf->bru_input].rpf = NULL;
@@ -537,7 +537,7 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int 
pipe_index)
continue;
}
 
-   if (list_empty(>entity.list_pipe)) {
+   if (!rpf->entity.pipe) {
rpf->entity.pipe = pipe;
list_add_tail(>entity.list_pipe, >entities);
}
@@ -566,7 +566,7 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int 
pipe_index)
   VI6_DPR_NODE_UNUSED);
 
entity->pipe = NULL;
-   list_del_init(>list_pipe);
+   list_del(>list_pipe);
 
continue;
}
-- 
Regards,

Laurent Pinchart

[PATCH v2 07/15] v4l: vsp1: Setup BRU at atomic commit time

[Laurent Pinchart]

To implement fully dynamic plane assignment to pipelines, we need to
reassign the BRU and BRS to the DRM pipelines in the atomic commit
handler. In preparation for this setup factor out the BRU source pad
code and call it both at LIF setup and atomic commit time.

Signed-off-by: Laurent Pinchart 
Reviewed-by: Kieran Bingham 
---
 drivers/media/platform/vsp1/vsp1_drm.c | 56 +-
 drivers/media/platform/vsp1/vsp1_drm.h |  5 +++
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index 7bf697ba7969..6ad8aa6c8138 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -148,12 +148,51 @@ static int vsp1_du_pipeline_setup_rpf(struct vsp1_device 
*vsp1,
return 0;
 }
 
+/* Setup the BRU source pad. */
+static int vsp1_du_pipeline_setup_bru(struct vsp1_device *vsp1,
+ struct vsp1_pipeline *pipe)
+{
+   struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
+   struct v4l2_subdev_format format = {
+   .which = V4L2_SUBDEV_FORMAT_ACTIVE,
+   };
+   int ret;
+
+   /*
+* Configure the format on the BRU source and verify that it matches the
+* requested format. We don't set the media bus code as it is configured
+* on the BRU sink pad 0 and propagated inside the entity, not on the
+* source pad.
+*/
+   format.pad = pipe->bru->source_pad;
+   format.format.width = drm_pipe->width;
+   format.format.height = drm_pipe->height;
+   format.format.field = V4L2_FIELD_NONE;
+
+   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), pipe->bru->source_pad);
+
+   if (format.format.width != drm_pipe->width ||
+   format.format.height != drm_pipe->height) {
+   dev_dbg(vsp1->dev, "%s: format mismatch\n", __func__);
+   return -EPIPE;
+   }
+
+   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). */
+/* 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)
 {
@@ -191,6 +230,18 @@ static int vsp1_du_pipeline_setup_input(struct vsp1_device 
*vsp1,
inputs[j] = rpf;
}
 
+   /*
+* Setup the BRU. This must be done before setting up the RPF input
+* pipelines as the BRU sink compose rectangles depend on the BRU source
+* format.
+*/
+   ret = vsp1_du_pipeline_setup_bru(vsp1, pipe);
+   if (ret < 0) {
+   dev_err(vsp1->dev, "%s: failed to setup %s source\n", __func__,
+   BRU_NAME(pipe->bru));
+   return ret;
+   }
+
/* Setup the RPF input pipeline for every enabled input. */
for (i = 0; i < pipe->bru->source_pad; ++i) {
struct vsp1_rwpf *rpf = inputs[i];
@@ -355,6 +406,9 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int 
pipe_index,
return 0;
}
 
+   drm_pipe->width = cfg->width;
+   drm_pipe->height = cfg->height;
+
dev_dbg(vsp1->dev, "%s: configuring LIF%u with format %ux%u\n",
__func__, pipe_index, cfg->width, cfg->height);
 
diff --git a/drivers/media/platform/vsp1/vsp1_drm.h 
b/drivers/media/platform/vsp1/vsp1_drm.h
index 9aa19325cbe9..c8dd75ba01f6 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.h
+++ b/drivers/media/platform/vsp1/vsp1_drm.h
@@ -20,12 +20,17 @@
 /**
  * vsp1_drm_pipeline - State for the API exposed to the DRM driver
  * @pipe: the VSP1 pipeline used for display
+ * @width: output display width
+ * @height: output display height
  * @du_complete: frame completion callback for the DU driver (optional)
  * @du_private: data to be passed to the du_complete callback
  */
 struct vsp1_drm_pipeline {
struct vsp1_pipeline pipe;
 
+   unsigned int width;
+   unsigned int height;
+
/* Frame synchronisation */
void (*du_complete)(void *, bool);
void *du_private;
-- 
Regards,

Laurent Pinchart

[PATCH v2 12/15] v4l: vsp1: Generalize detection of entity removal from DRM pipeline

[Laurent Pinchart]

When disabling a DRM plane, the corresponding RPF is only marked as
removed from the pipeline in the atomic update handler, with the actual
removal happening when configuring the pipeline at atomic commit time.
This is required as the RPF has to be disabled in the hardware, which
can't be done from the atomic update handler.

The current implementation is RPF-specific. Make it independent of the
entity type by using the entity's pipe pointer to mark removal from the
pipeline. This will allow using the mechanism to remove BRU instances.

Signed-off-by: Laurent Pinchart 
Reviewed-by: Kieran Bingham 
---
 drivers/media/platform/vsp1/vsp1_drm.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index 68b126044ea1..a9c53892a9ea 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -346,13 +346,12 @@ static void vsp1_du_pipeline_configure(struct 
vsp1_pipeline *pipe)
dl = vsp1_dl_list_get(pipe->output->dlm);
 
list_for_each_entry_safe(entity, next, >entities, list_pipe) {
-   /* Disconnect unused RPFs from the pipeline. */
-   if (entity->type == VSP1_ENTITY_RPF &&
-   !pipe->inputs[entity->index]) {
+   /* Disconnect unused entities from the pipeline. */
+   if (!entity->pipe) {
vsp1_dl_list_write(dl, entity->route->reg,
   VI6_DPR_NODE_UNUSED);
 
-   entity->pipe = NULL;
+   entity->sink = NULL;
list_del(>list_pipe);
 
continue;
@@ -569,10 +568,11 @@ int vsp1_du_atomic_update(struct device *dev, unsigned 
int pipe_index,
rpf_index);
 
/*
-* Remove the RPF from the pipe's inputs. The atomic flush
-* handler will disable the input and remove the entity from the
-* pipe's entities list.
+* Remove the RPF from the pipeline's inputs. Keep it in the
+* pipeline's entity list to let vsp1_du_pipeline_configure()
+* remove it from the hardware pipeline.
 */
+   rpf->entity.pipe = NULL;
drm_pipe->pipe.inputs[rpf_index] = NULL;
return 0;
}
-- 
Regards,

Laurent Pinchart

[PATCH v2 13/15] v4l: vsp1: Assign BRU and BRS to pipelines dynamically

[Laurent Pinchart]

The VSPDL variant drives two DU channels through two LIF and two
blenders, BRU and BRS. The DU channels thus share the five available
VSPDL inputs and expose them as five KMS planes.

The current implementation assigns the BRS to the second LIF and thus
artificially limits the number of planes for the second display channel
to two at most.

Lift this artificial limitation by assigning the BRU and BRS to the
display pipelines on demand based on the number of planes used by each
pipeline. When a display pipeline needs more than two inputs and the BRU
is already in use by the other pipeline, this requires reconfiguring the
other pipeline to free the BRU before processing, which can result in
frame drop on both pipelines.

Signed-off-by: Laurent Pinchart 
Reviewed-by: Kieran Bingham 
---
 drivers/media/platform/vsp1/vsp1_drm.c | 161 +++--
 drivers/media/platform/vsp1/vsp1_drm.h |   9 ++
 2 files changed, 144 insertions(+), 26 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index a9c53892a9ea..f82f83b6d4ff 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -37,10 +37,15 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline 
*pipe,
   unsigned int completion)
 {
struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
+   bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;
 
if (drm_pipe->du_complete)
-   drm_pipe->du_complete(drm_pipe->du_private,
- completion & VSP1_DL_FRAME_END_COMPLETED);
+   drm_pipe->du_complete(drm_pipe->du_private, complete);
+
+   if (completion & VSP1_DL_FRAME_END_INTERNAL) {
+   drm_pipe->force_bru_release = false;
+   wake_up(_pipe->wait_queue);
+   }
 }
 
 /* 
-
@@ -150,6 +155,10 @@ static int vsp1_du_pipeline_setup_rpf(struct vsp1_device 
*vsp1,
 }
 
 /* Setup the BRU source pad. */
+static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1,
+struct vsp1_pipeline *pipe);
+static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe);
+
 static int vsp1_du_pipeline_setup_bru(struct vsp1_device *vsp1,
  struct vsp1_pipeline *pipe)
 {
@@ -157,8 +166,93 @@ static int vsp1_du_pipeline_setup_bru(struct vsp1_device 
*vsp1,
struct v4l2_subdev_format format = {
.which = V4L2_SUBDEV_FORMAT_ACTIVE,
};
+   struct vsp1_entity *bru;
int ret;
 
+   /*
+* Pick a BRU:
+* - If we need more than two inputs, use the main BRU.
+* - Otherwise, if we are not forced to release our BRU, keep it.
+* - Else, use any free BRU (randomly starting with the main BRU).
+*/
+   if (pipe->num_inputs > 2)
+   bru = >bru->entity;
+   else if (pipe->bru && !drm_pipe->force_bru_release)
+   bru = pipe->bru;
+   else if (!vsp1->bru->entity.pipe)
+   bru = >bru->entity;
+   else
+   bru = >brs->entity;
+
+   /* Switch BRU if needed. */
+   if (bru != pipe->bru) {
+   struct vsp1_entity *released_bru = NULL;
+
+   /* Release our BRU if we have one. */
+   if (pipe->bru) {
+   /*
+* The BRU might be acquired by the other pipeline in
+* the next step. We must thus remove it from the list
+* of entities for this pipeline. The other pipeline's
+* hardware configuration will reconfigure the BRU
+* routing.
+*
+* However, if the other pipeline doesn't acquire our
+* BRU, we need to keep it in the list, otherwise the
+* hardware configuration step won't disconnect it from
+* the pipeline. To solve this, store the released BRU
+* pointer to add it back to the list of entities later
+* if it isn't acquired by the other pipeline.
+*/
+   released_bru = pipe->bru;
+
+   list_del(>bru->list_pipe);
+   pipe->bru->sink = NULL;
+   pipe->bru->pipe = NULL;
+   pipe->bru = NULL;
+   }
+
+   /*
+* If the BRU we need is in use, force the owner pipeline to
+* switch to the other BRU and wait until the switch completes.
+*/
+   if (bru->pipe) {
+   struct vsp1_drm_pipeline 

[PATCH v2 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 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 | 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)
-   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 

[PATCH v2 01/15] v4l: vsp1: Don't start/stop media pipeline for DRM

[Laurent Pinchart]

The DRM support code manages a pipeline of VSP entities, each backed by
a media entity. When starting or stopping the pipeline, it starts and
stops the media pipeline through the media API in order to store the
pipeline pointer in every entity.

The driver doesn't use the pipe pointer in media entities, neither does
it rely on the other effects of the media_pipeline_start() and
media_pipeline_stop() functions. Furthermore, as the media links for the
DRM pipeline are never set up correctly, and as the pipeline can be
modified dynamically when enabling or disabling planes, the current
implementation is not correct. Remove the incorrect and unneeded code.

While at it remove the outdated comment that states that entities are
not started when the LIF is setup, as they now are.

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

- Remove outdated comment
---
 drivers/media/platform/vsp1/vsp1_drm.c | 18 +-
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index b8fee1834253..a1f2ba044092 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -109,8 +109,6 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int 
pipe_index,
if (ret == -ETIMEDOUT)
dev_err(vsp1->dev, "DRM pipeline stop timeout\n");
 
-   media_pipeline_stop(>output->entity.subdev.entity);
-
for (i = 0; i < ARRAY_SIZE(pipe->inputs); ++i) {
struct vsp1_rwpf *rpf = pipe->inputs[i];
 
@@ -223,13 +221,7 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int 
pipe_index,
return -EPIPE;
}
 
-   /*
-* Mark the pipeline as streaming and enable the VSP1. This will store
-* the pipeline pointer in all entities, which the s_stream handlers
-* will need. We don't start the entities themselves right at this point
-* as there's no plane configured yet, so we can't start processing
-* buffers.
-*/
+   /* Enable the VSP1. */
ret = vsp1_device_get(vsp1);
if (ret < 0)
return ret;
@@ -241,14 +233,6 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int 
pipe_index,
drm_pipe->du_complete = cfg->callback;
drm_pipe->du_private = cfg->callback_data;
 
-   ret = media_pipeline_start(>output->entity.subdev.entity,
- >pipe);
-   if (ret < 0) {
-   dev_dbg(vsp1->dev, "%s: pipeline start failed\n", __func__);
-   vsp1_device_put(vsp1);
-   return ret;
-   }
-
/* Disable the display interrupts. */
vsp1_write(vsp1, VI6_DISP_IRQ_STA, 0);
vsp1_write(vsp1, VI6_DISP_IRQ_ENB, 0);
-- 
Regards,

Laurent Pinchart

[PATCH v2 03/15] v4l: vsp1: Store pipeline pointer in vsp1_entity

[Laurent Pinchart]

Various types of objects subclassing vsp1_entity currently store a
pointer to the pipeline. Move the pointer to vsp1_entity to simplify the
code and avoid storing the pipeline in more entity subclasses later.

Signed-off-by: Laurent Pinchart 
Reviewed-by: Kieran Bingham 
---
 drivers/media/platform/vsp1/vsp1_drm.c| 20 +--
 drivers/media/platform/vsp1/vsp1_drv.c|  2 +-
 drivers/media/platform/vsp1/vsp1_entity.h |  2 ++
 drivers/media/platform/vsp1/vsp1_histo.c  |  2 +-
 drivers/media/platform/vsp1/vsp1_histo.h  |  3 ---
 drivers/media/platform/vsp1/vsp1_pipe.c   | 33 +--
 drivers/media/platform/vsp1/vsp1_rwpf.h   |  2 --
 drivers/media/platform/vsp1/vsp1_video.c  | 17 +++-
 8 files changed, 34 insertions(+), 47 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index a267f12f0cc8..a7ad85ab0b08 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -120,6 +120,7 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int 
pipe_index,
 * inputs.
 */
WARN_ON(list_empty(>entity.list_pipe));
+   rpf->entity.pipe = NULL;
list_del_init(>entity.list_pipe);
pipe->inputs[i] = NULL;
 
@@ -536,8 +537,10 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int 
pipe_index)
continue;
}
 
-   if (list_empty(>entity.list_pipe))
+   if (list_empty(>entity.list_pipe)) {
+   rpf->entity.pipe = pipe;
list_add_tail(>entity.list_pipe, >entities);
+   }
 
bru->inputs[i].rpf = rpf;
rpf->bru_input = i;
@@ -562,6 +565,7 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int 
pipe_index)
vsp1_dl_list_write(dl, entity->route->reg,
   VI6_DPR_NODE_UNUSED);
 
+   entity->pipe = NULL;
list_del_init(>list_pipe);
 
continue;
@@ -625,24 +629,28 @@ int vsp1_drm_init(struct vsp1_device *vsp1)
 
vsp1_pipeline_init(pipe);
 
+   pipe->frame_end = vsp1_du_pipeline_frame_end;
+
/*
 * The DRM pipeline is static, add entities manually. The first
 * pipeline uses the BRU and the second pipeline the BRS.
 */
pipe->bru = i == 0 ? >bru->entity : >brs->entity;
-   pipe->lif = >lif[i]->entity;
pipe->output = vsp1->wpf[i];
-   pipe->output->pipe = pipe;
-   pipe->frame_end = vsp1_du_pipeline_frame_end;
+   pipe->lif = >lif[i]->entity;
 
+   pipe->bru->pipe = pipe;
pipe->bru->sink = >output->entity;
pipe->bru->sink_pad = 0;
+   list_add_tail(>bru->list_pipe, >entities);
+
+   pipe->output->entity.pipe = pipe;
pipe->output->entity.sink = pipe->lif;
pipe->output->entity.sink_pad = 0;
+   list_add_tail(>output->entity.list_pipe, >entities);
 
-   list_add_tail(>bru->list_pipe, >entities);
+   pipe->lif->pipe = pipe;
list_add_tail(>lif->list_pipe, >entities);
-   list_add_tail(>output->entity.list_pipe, >entities);
}
 
/* Disable all RPFs initially. */
diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
b/drivers/media/platform/vsp1/vsp1_drv.c
index eed9516e25e1..58a7993f2306 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -63,7 +63,7 @@ static irqreturn_t vsp1_irq_handler(int irq, void *data)
vsp1_write(vsp1, VI6_WPF_IRQ_STA(i), ~status & mask);
 
if (status & VI6_WFP_IRQ_STA_DFE) {
-   vsp1_pipeline_frame_end(wpf->pipe);
+   vsp1_pipeline_frame_end(wpf->entity.pipe);
ret = IRQ_HANDLED;
}
}
diff --git a/drivers/media/platform/vsp1/vsp1_entity.h 
b/drivers/media/platform/vsp1/vsp1_entity.h
index 408602ebeb97..c26523c56c05 100644
--- a/drivers/media/platform/vsp1/vsp1_entity.h
+++ b/drivers/media/platform/vsp1/vsp1_entity.h
@@ -106,6 +106,8 @@ struct vsp1_entity {
unsigned int index;
const struct vsp1_route *route;
 
+   struct vsp1_pipeline *pipe;
+
struct list_head list_dev;
struct list_head list_pipe;
 
diff --git a/drivers/media/platform/vsp1/vsp1_histo.c 
b/drivers/media/platform/vsp1/vsp1_histo.c
index afab77cf4fa5..8638ebc514b4 100644
--- a/drivers/media/platform/vsp1/vsp1_histo.c
+++ b/drivers/media/platform/vsp1/vsp1_histo.c
@@ 

[PATCH v2 05/15] v4l: vsp1: Share duplicated DRM pipeline configuration code

[Laurent Pinchart]

Move the duplicated DRM pipeline configuration code to a function and
call it from vsp1_du_setup_lif() and vsp1_du_atomic_flush().

Signed-off-by: Laurent Pinchart 
Reviewed-by: Kieran Bingham 
---
 drivers/media/platform/vsp1/vsp1_drm.c | 95 +++---
 1 file changed, 43 insertions(+), 52 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index e210917fdc3f..9a043a915c0b 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -42,6 +42,47 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline 
*pipe,
drm_pipe->du_complete(drm_pipe->du_private, completed);
 }
 
+/* 
-
+ * Pipeline Configuration
+ */
+
+/* Configure all entities in the pipeline. */
+static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
+{
+   struct vsp1_entity *entity;
+   struct vsp1_entity *next;
+   struct vsp1_dl_list *dl;
+
+   dl = vsp1_dl_list_get(pipe->output->dlm);
+
+   list_for_each_entry_safe(entity, next, >entities, list_pipe) {
+   /* Disconnect unused RPFs from the pipeline. */
+   if (entity->type == VSP1_ENTITY_RPF &&
+   !pipe->inputs[entity->index]) {
+   vsp1_dl_list_write(dl, entity->route->reg,
+  VI6_DPR_NODE_UNUSED);
+
+   entity->pipe = NULL;
+   list_del(>list_pipe);
+
+   continue;
+   }
+
+   vsp1_entity_route_setup(entity, pipe, dl);
+
+   if (entity->ops->configure) {
+   entity->ops->configure(entity, pipe, dl,
+  VSP1_ENTITY_PARAMS_INIT);
+   entity->ops->configure(entity, pipe, dl,
+  VSP1_ENTITY_PARAMS_RUNTIME);
+   entity->ops->configure(entity, pipe, dl,
+  VSP1_ENTITY_PARAMS_PARTITION);
+   }
+   }
+
+   vsp1_dl_list_commit(dl);
+}
+
 /* 
-
  * DU Driver API
  */
@@ -85,9 +126,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 vsp1_entity *entity;
-   struct vsp1_entity *next;
-   struct vsp1_dl_list *dl;
struct v4l2_subdev_format format;
unsigned long flags;
unsigned int i;
@@ -239,22 +277,7 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int 
pipe_index,
vsp1_write(vsp1, VI6_DISP_IRQ_ENB, 0);
 
/* Configure all entities in the pipeline. */
-   dl = vsp1_dl_list_get(pipe->output->dlm);
-
-   list_for_each_entry_safe(entity, next, >entities, list_pipe) {
-   vsp1_entity_route_setup(entity, pipe, dl);
-
-   if (entity->ops->configure) {
-   entity->ops->configure(entity, pipe, dl,
-  VSP1_ENTITY_PARAMS_INIT);
-   entity->ops->configure(entity, pipe, dl,
-  VSP1_ENTITY_PARAMS_RUNTIME);
-   entity->ops->configure(entity, pipe, dl,
-  VSP1_ENTITY_PARAMS_PARTITION);
-   }
-   }
-
-   vsp1_dl_list_commit(dl);
+   vsp1_du_pipeline_configure(pipe);
 
/* Start the pipeline. */
spin_lock_irqsave(>irqlock, flags);
@@ -490,15 +513,9 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int 
pipe_index)
struct vsp1_pipeline *pipe = _pipe->pipe;
struct vsp1_rwpf *inputs[VSP1_MAX_RPF] = { NULL, };
struct vsp1_bru *bru = to_bru(>bru->subdev);
-   struct vsp1_entity *entity;
-   struct vsp1_entity *next;
-   struct vsp1_dl_list *dl;
unsigned int i;
int ret;
 
-   /* Prepare the display list. */
-   dl = vsp1_dl_list_get(pipe->output->dlm);
-
/* Count the number of enabled inputs and sort them by Z-order. */
pipe->num_inputs = 0;
 
@@ -557,33 +574,7 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int 
pipe_index)
__func__, rpf->entity.index);
}
 
-   /* Configure all entities in the pipeline. */
-   list_for_each_entry_safe(entity, next, >entities, list_pipe) {
-   /* Disconnect unused RPFs from the pipeline. */
-   if (entity->type == VSP1_ENTITY_RPF &&
-   !pipe->inputs[entity->index]) {
-   vsp1_dl_list_write(dl, entity->route->reg,
-   

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

[Laurent Pinchart]

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
---
 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_COMPLETEDBIT(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 
*dlm);
 struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm);
 void vsp1_dl_list_put(struct vsp1_dl_list *dl);
 void vsp1_dl_list_write(struct vsp1_dl_list *dl, u32 reg, u32 data);
-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_body 

[PATCH v2 08/15] v4l: vsp1: Replace manual DRM pipeline input setup in vsp1_du_setup_lif

[Laurent Pinchart]

The vsp1_du_setup_lif() function sets up the DRM pipeline input
manually. This duplicates the code from the
vsp1_du_pipeline_setup_input() function. Replace the manual
implementation by a call to the function.

As the pipeline has no enabled input in vsp1_du_setup_lif(), the
vsp1_du_pipeline_setup_input() function will not setup any RPF, and will
thus not setup formats on the BRU sink pads. This isn't a problem as all
inputs are disabled, and the BRU sink pads will be reconfigured from the
atomic commit handler when inputs will be enabled.

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

- Fixed spelling in commit message
---
 drivers/media/platform/vsp1/vsp1_drm.c | 40 +-
 1 file changed, 6 insertions(+), 34 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index 6ad8aa6c8138..00ce99bd1605 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -412,47 +412,19 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int 
pipe_index,
dev_dbg(vsp1->dev, "%s: configuring LIF%u with format %ux%u\n",
__func__, pipe_index, cfg->width, cfg->height);
 
-   /*
-* Configure the format at the BRU sinks and propagate it through the
-* pipeline.
-*/
+   /* Setup formats through the pipeline. */
+   ret = vsp1_du_pipeline_setup_input(vsp1, pipe);
+   if (ret < 0)
+   return ret;
+
memset(, 0, sizeof(format));
format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-
-   for (i = 0; i < pipe->bru->source_pad; ++i) {
-   format.pad = i;
-
-   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(>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), i);
-   }
-
-   format.pad = pipe->bru->source_pad;
+   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(>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), i);
-
-   format.pad = RWPF_PAD_SINK;
ret = v4l2_subdev_call(>output->entity.subdev, pad, set_fmt, NULL,
   );
if (ret < 0)
-- 
Regards,

Laurent Pinchart

[PATCH v2 00/15] R-Car VSP1: Dynamically assign blend units to display pipelines

[Laurent Pinchart]

Hello,

On R-Car H3 ES2.0+ and M3-N SoCs, two display pipelines are served by the same
VSP instance (named VSPDL). The VSPDL includes two blending units named BRU
and BRS, unlike the other display-related VSPD that serves a single display
channel with a single blending unit.

The VSPDL has five inputs and can freely assign them at runtime to two display
pipelines, using the BRU and BRS to blend multiple inputs. The BRU supports
blending up to five inputs, while the BRS is limited to two inputs.

Each display pipeline requires a blending unit, and the BRU and BRS are
currently assigned statically to the first and second pipeline respectively.
This artificially limits the number of inputs for the second pipeline to two.
To overcome that limitation, the BRU and BRS need to be dynamically assigned
to the display pipelines, which is what this patch series does.

Patches 01/15 to 10/15 perform small cleanups and refactoring to prepare for
the rest of the series. Patches 11/15 and 12/15 implement new internal
features for the same purpose, and patch 13/15 performs the bulk of the work
by implementing dynamic BRU and BRS assignment to pipelines.

Reassigning the BRU and BRS when two pipelines are running results in flicker
as one pipeline has to first release its blending unit. Synchronization
between the two pipelines also require locking that effectively serializes
page flips for the two pipelines, even when not interacting with each other.
This is currently believed to be unavoidable due to the hardware design, but
please feel free to prove me wrong (ideally with a patch - one can always
dream).

Patch 14/15 then adds messages useful for debugging this new feature. I have
kept it separate from 13/15 to make it easier to remove those messages once
dynamic assignment of blending units will be deemed perfectly stable, but I
won't oppose squashing it with 13/15 if that is preferred.

Patch 15/15 finally rename BRU to BRx to avoid confusion between the BRU terms
that refer to the BRU in particular, and the ones that refer to any of the BRU
or BRS. As this might be a bit controversial I've put the patch last in the
series in case it needs to be dropped.

I have decided to CC the dri-devel mailing list even though the code doesn't
touch the R-Car DU driver and will be merged through the Linux media tree as
the display is involved and the series could benefit from the expertise of the
DRM/KMS community from that point of view.

The patches are based on top of the latest Linux media master branch. For
convenience their are available from

git://linuxtv.org/pinchartl/media.git v4l2-vsp1-bru-brs-v2-20180405

The series passes the DU test suite with the new BRx dynamic assignment
test available from

git://git.ideasonboard.com/renesas/kms-tests.git bru-brs

The VSP test suite also runs without any noticed regression.

Since v1, patch 10/15 has been added an the v1 02/15 patch merged with
01/15. Other small changes are documented in individual changelogs.

Laurent Pinchart (15):
  v4l: vsp1: Don't start/stop media pipeline for DRM
  v4l: vsp1: Remove unused field from vsp1_drm_pipeline structure
  v4l: vsp1: Store pipeline pointer in vsp1_entity
  v4l: vsp1: Use vsp1_entity.pipe to check if entity belongs to a
pipeline
  v4l: vsp1: Share duplicated DRM pipeline configuration code
  v4l: vsp1: Move DRM atomic commit pipeline setup to separate function
  v4l: vsp1: Setup BRU at atomic commit time
  v4l: vsp1: Replace manual DRM pipeline input setup in
vsp1_du_setup_lif
  v4l: vsp1: Move DRM pipeline output setup code to a function
  v4l: vsp1: Turn frame end completion status into a bitfield
  v4l: vsp1: Add per-display list internal completion notification
support
  v4l: vsp1: Generalize detection of entity removal from DRM pipeline
  v4l: vsp1: Assign BRU and BRS to pipelines dynamically
  v4l: vsp1: Add BRx dynamic assignment debugging messages
  v4l: vsp1: Rename BRU to BRx

 drivers/media/platform/vsp1/Makefile   |   2 +-
 drivers/media/platform/vsp1/vsp1.h |   6 +-
 .../media/platform/vsp1/{vsp1_bru.c => vsp1_brx.c} | 202 ++---
 .../media/platform/vsp1/{vsp1_bru.h => vsp1_brx.h} |  18 +-
 drivers/media/platform/vsp1/vsp1_dl.c  |  45 +-
 drivers/media/platform/vsp1/vsp1_dl.h  |   7 +-
 drivers/media/platform/vsp1/vsp1_drm.c | 828 -
 drivers/media/platform/vsp1/vsp1_drm.h |  16 +-
 drivers/media/platform/vsp1/vsp1_drv.c |   8 +-
 drivers/media/platform/vsp1/vsp1_entity.h  |   2 +
 drivers/media/platform/vsp1/vsp1_histo.c   |   2 +-
 drivers/media/platform/vsp1/vsp1_histo.h   |   3 -
 drivers/media/platform/vsp1/vsp1_pipe.c|  53 +-
 drivers/media/platform/vsp1/vsp1_pipe.h|   6 +-
 drivers/media/platform/vsp1/vsp1_rpf.c |  12 +-
 drivers/media/platform/vsp1/vsp1_rwpf.h|   4 +-
 drivers/media/platform/vsp1/vsp1_v

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[jacopo mondi]

Hi Niklas,
thanks for the VIN and CSI-2 effort!

On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote:
> A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are
> connected between the video sources and the video grabbers (VIN).
>
> Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
>
> Signed-off-by: Niklas Söderlund 
> Reviewed-by: Hans Verkuil 
> ---
>  drivers/media/platform/rcar-vin/Kconfig |  12 +
>  drivers/media/platform/rcar-vin/Makefile|   1 +
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 884 
> 
>  3 files changed, 897 insertions(+)
>  create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c
>

[snip]

> +
> +static const struct rcar_csi2_format rcar_csi2_formats[] = {
> + { .code = MEDIA_BUS_FMT_RGB888_1X24,.datatype = 0x24, .bpp = 24 },
> + { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 },
> + { .code = MEDIA_BUS_FMT_UYVY8_2X8,  .datatype = 0x1e, .bpp = 16 },
> + { .code = MEDIA_BUS_FMT_YUYV10_2X10,.datatype = 0x1e, .bpp = 16 },

Shouldn't YUYV10_2X10 format have 20 bits per pixel?

> +};
> +
> +static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int 
> code)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> + if (rcar_csi2_formats[i].code == code)
> + return rcar_csi2_formats + i;
> +
> + return NULL;
> +}
> +
> +enum rcar_csi2_pads {
> + RCAR_CSI2_SINK,
> + RCAR_CSI2_SOURCE_VC0,
> + RCAR_CSI2_SOURCE_VC1,
> + RCAR_CSI2_SOURCE_VC2,
> + RCAR_CSI2_SOURCE_VC3,
> + NR_OF_RCAR_CSI2_PAD,
> +};
> +
> +struct rcar_csi2_info {
> + const struct phypll_hsfreqrange *hsfreqrange;
> + unsigned int csi0clkfreqrange;
> + bool clear_ulps;
> + bool init_phtw;
> +};
> +
> +struct rcar_csi2 {
> + struct device *dev;
> + void __iomem *base;
> + const struct rcar_csi2_info *info;
> +
> + struct v4l2_subdev subdev;
> + struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
> +
> + struct v4l2_async_notifier notifier;
> + struct v4l2_async_subdev asd;
> + struct v4l2_subdev *remote;
> +
> + struct v4l2_mbus_framefmt mf;
> +
> + struct mutex lock;
> + int stream_count;
> +
> + unsigned short lanes;
> + unsigned char lane_swap[4];
> +};
> +
> +static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> +{
> + return container_of(sd, struct rcar_csi2, subdev);
> +}
> +
> +static inline struct rcar_csi2 *notifier_to_csi2(struct v4l2_async_notifier 
> *n)
> +{
> + return container_of(n, struct rcar_csi2, notifier);
> +}
> +
> +static u32 rcar_csi2_read(struct rcar_csi2 *priv, unsigned int reg)
> +{
> + return ioread32(priv->base + reg);
> +}
> +
> +static void rcar_csi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 
> data)
> +{
> + iowrite32(data, priv->base + reg);
> +}
> +
> +static void rcar_csi2_reset(struct rcar_csi2 *priv)
> +{
> + rcar_csi2_write(priv, SRST_REG, SRST_SRST);
> + usleep_range(100, 150);
> + rcar_csi2_write(priv, SRST_REG, 0);
> +}
> +
> +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv)
> +{
> + int timeout;
> +
> + /* Wait for the clock and data lanes to enter LP-11 state. */
> + for (timeout = 100; timeout > 0; timeout--) {
> + const u32 lane_mask = (1 << priv->lanes) - 1;
> +
> + if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 &&

Nitpicking:
if ((rcar_csi2_read(priv, PHCLM_REG) & 0x01) &&

Don't you prefer to provide defines also for bit fields instead of
using magic values? In this case something like
PHCLM_REG_STOPSTATE_CLK would do.

Also, from tables 25.[17-20] it seems to me that for H3 and V3 you
have to set INSTATE to an hardcoded value after having validated PHDLM.
Maybe it is not necessary, just pointing it out.

> + (rcar_csi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> + return 0;
> +
> + msleep(20);
> + }
> +
> + dev_err(priv->dev, "Timeout waiting for LP-11 state\n");
> +
> + return -ETIMEDOUT;
> +}
> +
> +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv, unsigned int bpp,
> +  u32 *phypll)
> +{
> + const struct phypll_hsfreqrange *hsfreq;
> + struct v4l2_subdev *source;
> + struct v4l2_ctrl *ctrl;
> + u64 mbps;
> +
> + if (!priv->remote)
> + return -ENODEV;
> +
> + source = priv->remote;
> +
> + /* Read the pixel rate control from remote */
> + ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> + if (!ctrl) {
> + dev_err(priv->dev, "no pixel rate control in subdev %s\n",
> + source->name);
> + return -EINVAL;
> + }
> +
> +  

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

[Kieran Bingham]

Hi Laurent,

On 04/04/18 22:43, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Wednesday, 4 April 2018 19:16:46 EEST Kieran Bingham wrote:
>> On 26/02/18 21:45, Laurent Pinchart wrote:



>>>
>>> -void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
>>> +void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool notify)
>>
>> Rather than changing the vsp1_dl_list_commit() function - would it be nicer
>> to have an API to request or set the notify property? :
>>
>> @@..@@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
>> ...
>> +/* The BRx will be released, without sending an update to DRM */
>> +if (drm_pipe->force_bru_release)
>> +vsp1_dl_list_request_internal_notify(dl);
>>
>>  vsp1_dl_list_commit(dl);
>> ...
> 
> That's not a bad idea, but I wonder if it's worth it as we'll have to call an 
> extra function for what is essentially an internal API. On the other hand 
> this 
> isn't a common case, so it's not a hot code path. We could also argue equally 
> that it is the commit that is internal or that it is the display list that is 

Aha, yes - it is more so that the commit is internal ...

> for internal purpose. Do you think an extra function call is better ? If you 
> do I'll change it.

so it could also instead be just a separate commit() function:


void vsp1_dl_list_commit_internal(struct vsp1_dl_list *dl)
{
dl->internal = true;
vsp1_dl_list_commit(dl);
}


...


{
/* The BRx will be released, without sending an update to DRM */
if (drm_pipe->force_bru_release)
vsp1_dl_list_commit_internal(dl);
else
vsp1_dl_list_commit(dl);
}

I'll leave the final implementation decision with you - I just thought that
extending the commit call with a notify flag seemed odd.

Of course - that could also have been due to the naming of the 'notify' - if it
was 'internal' as discussed in the other patches, then perhaps a flag on the
function call is still a sensible way. It just affects the other commit usages,
but there's only a total of three calls - so it's really not a big deal. If
there were 12 call locations perhaps the function wrapper would have more merit
- but probably not so much at 3 :D

--
Kieran

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Laurent Pinchart]

Hi Geert,

On Thursday, 5 April 2018 10:33:55 EEST Geert Uytterhoeven wrote:
> On Wed, Apr 4, 2018 at 5:26 PM, Laurent Pinchart wrote:
> > On Thursday, 29 March 2018 14:30:39 EEST Maxime Ripard wrote:
> >> On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote:
> >> > +   switch (priv->lanes) {
> >> > +   case 1:
> >> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
> >> > +   break;
> >> > +   case 2:
> >> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 |
> >> > PHYCNT_ENABLE_0;
> >> > +   break;
> >> > +   case 4:
> >> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 |
> >> > PHYCNT_ENABLE_2 |
> >> > +   PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
> >> > +   break;
> >> > +   default:
> >> > +   return -EINVAL;
> >> > +   }
> >> 
> >> I guess you could have a simpler construct here using this:
> >> 
> >> phycnt = PHYCNT_ENABLECLK;
> >> 
> >> switch (priv->lanes) {
> >> 
> >> case 4:
> >>   phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2;
> >> 
> >> case 2:
> >>   phycnt |= PHYCNT_ENABLE_1;
> >> 
> >> case 1:
> >>   phycnt |= PHYCNT_ENABLE_0;
> >>   break;
> >> 
> >> default:
> >>   return -EINVAL;
> >> 
> >> }
> >> 
> >> But that's really up to you.
> > 
> > Wouldn't Niklas' version generate simpler code as it uses direct
> > assignments ?
> Alternatively, you could check for a valid number of lanes, and use
> knowledge about the internal lane bits:
> 
> phycnt = PHYCNT_ENABLECLK;
> phycnt |= (1 << priv->lanes) - 1;

If Niklas is fine with that, I like it too.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v13 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Geert Uytterhoeven]

On Wed, Apr 4, 2018 at 5:26 PM, Laurent Pinchart
 wrote:
> On Thursday, 29 March 2018 14:30:39 EEST Maxime Ripard wrote:
>> On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote:
>> > +   switch (priv->lanes) {
>> > +   case 1:
>> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0;
>> > +   break;
>> > +   case 2:
>> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
>> > +   break;
>> > +   case 4:
>> > +   phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 |
>> > +   PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0;
>> > +   break;
>> > +   default:
>> > +   return -EINVAL;
>> > +   }
>>
>> I guess you could have a simpler construct here using this:
>>
>> phycnt = PHYCNT_ENABLECLK;
>>
>> switch (priv->lanes) {
>> case 4:
>>   phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2;
>> case 2:
>>   phycnt |= PHYCNT_ENABLE_1;
>> case 1:
>>   phycnt |= PHYCNT_ENABLE_0;
>>   break;
>>
>> default:
>>   return -EINVAL;
>> }
>>
>> But that's really up to you.
>
> Wouldn't Niklas' version generate simpler code as it uses direct assignments ?

Alternatively, you could check for a valid number of lanes, and use knowledge
about the internal lane bits:

phycnt = PHYCNT_ENABLECLK;
phycnt |= (1 << priv->lanes) - 1;

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v3 0/2] mmc: renesas_sdhi: add eMMC HS400 mode support

[Ulf Hansson]

On 27 March 2018 at 13:12, Simon Horman  wrote:
> On Tue, Feb 27, 2018 at 02:02:23PM +0100, Wolfram Sang wrote:
>>
>> > Perhaps someone need to explain in more detail what the HW controller
>> > needs to manage tuning for HS400? I don't like that we may end up
>> > getting it magically to work, then it's we better understand the
>> > details and if the current sequence provided by the mmc core can't
>> > fulfill the need for this case, we may instead consider adding an
>> > additional pre/post host ops.
>>
>> Thanks. Exactly this thinking prevented me from saying "It works, let's
>> ship it" ;)
>
> Thanks, I've dug a little deeper into the history of this code and
> the associated hardware documentation and it seems that the desired
> sequence is as follows:
>
> 1. Reset HS400 mode (currently executed near beginning of tmio_mmc_set_ios())
> 2. Set clock for HS200 mode (tmio_mmc_set_ios())
> 3. Tune in HS200 mode
> 4. Set clock for HS200 mode (tmio_mmc_set_ios())
> 5. Set to HS400 mode (currently renesas_sdhi_prepare_hs400_tuning() which
>   is executed near the end of tmio_mmc_set_ios())

This seems to also refer to how the eMMC spec describes the sequence,
and thus also implemented by the mmc core.

>
> X. It is also necessary to disable the SCC after setting HS400 mode
>has occurred before setting to High Speed mode.
>This is currently executed near the beginning of tmio_mmc_set_ios().
>
> The current implementation differs a little from the above and its
> hard to tell if that is because of hardware requirements or as
> an artefact of the implementation of steps 1 and 5 being hooked into
> tmio_mmc_set_ios().
>
> It seems to me that step 1 could use the MMC core prepare_hs400_tuning
> hook. Its less clear to me where steps 5 and X should hook in but perhaps
> tmio_mmc_set_ios() is the right place.

In ->set_ios() you are relying on checking the value of ->ios.timing
(MMC_TIMING_MMC_HS200 and MMC_TIMING_MMC_HS400) to understand what
actions to take.

For tuning, we found this was not sufficient when trying to cope with
all different MMC controller constraints, which is why we added the
->prepare_hs400_tuning() ops. I am guessing it should work for your
case as well.

However, if that isn't the case and you find reasons to add another
->[xyz]_hs400_tuning() callback, then I am open to discuss that.

Kind regards
Uffe

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

[Ulf Hansson]

On 4 April 2018 at 19:00, Wolfram Sang  wrote:
> 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 */
> -   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))

The WARN_ON becomes a bit misleading being a part of the if statement,
as it should never happen when the driver has set ->max_segs = 1.

For the alignment check, and the moving to PIO, this certainly makes
sense to me.

> goto force_pio;
>
> if (data->flags & MMC_DATA_READ) {

Kind regards
Uffe

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

[Ulf Hansson]

On 4 April 2018 at 18:45, Wolfram Sang  wrote:
> 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 
> ---
>
> Okay, here is what I came up with to solve this problem. Compared to
> Shimoda-san's sketch, I replaced the semaphore with atomic bit operations. I
> think this should do because we really need only a flag telling us if RX is in
> use already. And bitops should be more lightweight and less complex.
>
> I used this test on the target with SD cards in both slots:
>
> ===
> #! /bin/sh -e
>
> IF="/dev/mmcblk$1p1"
> PTH="/mnt/mmcblk$1"
> CHK='/tmp/check'
>
> while :; do
> mkdir -p $PTH
> mount $IF $PTH &> /dev/null
> cd $PTH
> time md5sum -c $CHK >> /tmp/mmc$1
> cd
> umount $PTH
> done
> ===
>
> CHK was manually created beforehand. Before this patch, I got checksum 
> failures
> within seconds. Now, it runs for hours without checksum problems. Of course,
> there is a performance regression because of falling back to PIO now, but we
> can't help that.
>
> I also ran the test on a H3 ES2.0 which doesn't suffer from the problem. No
> regressions, it works out of the box there.
>
> 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.

I assume this is material for stable as well, right?

>
> Kind regards,
>
>Wolfram
>
>  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 35 
> ---
>  1 file changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c 
> b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> index 8e0acd197c43..380570a26a09 100644
> --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> @@ -9,6 +9,7 @@
>   * published by the Free Software Foundation.
>   */
>
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -62,6 +63,17 @@
>   *   need a custom accessor.
>   */
>
> +static unsigned long global_flags = 0;

There's no need to initialize static variables to zero.

[...]

Kind regards
Uffe